Add: Worker.detach_persistent_domain public retention API - #2120
Add: Worker.detach_persistent_domain public retention API#2120georgebisbas wants to merge 1 commit into
Conversation
Callers that want a CommDomain to outlive one run's completion fence (dispatching many requests against the same domain) currently have no supported way to do it: transferring a domain's ownership from the run-local journal to the Worker requires reaching into _RunResources' private live_domains/domain_lock/retired fields. detach_persistent_domain encapsulates that transfer as a public method, matching the existing live_domains property's visibility.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ChangesPersistent domain lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change adds persistent communication-domain detachment with lifecycle validation and simulation coverage for ownership transfer, cleanup, and invalid calls. No concrete merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant Worker
participant RunResources
participant WorkerLiveDomains
Orchestrator->>Worker: detach_persistent_domain(handle)
Worker->>RunResources: validate active live claim
Worker->>RunResources: remove handle
Worker->>WorkerLiveDomains: retain handle
Worker->>WorkerLiveDomains: reclaim handle on close()
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 1 files. (1 skipped: 1 too large.)
Warning Some tools did not complete. Review the errors below. 🔧 Ruff (0.16.3)python/simpler/worker.py�[1;31mruff failed�[0m tests/ut/py/test_worker/test_detach_persistent_domain.py�[1;31mruff failed�[0m Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
Worker.detach_persistent_domain(handle): transfers one CommDomainfrom the current run's local journal (
_RunResources.live_domains) toWorker-level ownership, so a caller can outlive one run's completion fence
and dispatch many requests against the same retained domain.
operation on
Worker, next to the existing publiclive_domainsproperty.Why
pypto's persistent distributed execution mode needs exactly this transfer,
and currently does it by reaching directly into
_RunResources's privatelive_domains,domain_lock, andretiredfields from outsidesimpler.Nothing in
simplerpromises that shape, so any refactor insidesimplercan silently invalidate pypto's persistent path.
Relates to hw-native-sys/pypto#2284, which documents this exact private-API
dependency and proposes moving the contract into
simpleras a publicretention API. A follow-up pypto PR will migrate pypto's runtime to call
this method instead of reaching into
_RunResourcesdirectly, and will bethe one to close that issue.
Testing
tests/ut/py/test_worker/test_detach_persistent_domain.py(new, 3tests): transfers ownership and survives the run, keeping the handle
reachable for
close()to reclaim; raises when called outside anin-flight run; raises on a second detach of an already-detached
handle.
tests/ut/py/test_worker/test_dynamic_alloc_sim.py(existing siblingsuite over the same
Worker/CommDomainHandlemachinery, 7 tests) —no regression.
ruff check/ruff format/pyrightclean viapre-commit.