fix(worker): unify lifecycle into one authoritative, terminal-close() machine#1398
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesWorker lifecycle and startup teardown
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Worker
participant ChildWorkers
participant ProcessGroups
Caller->>Worker: init()
Worker->>ChildWorkers: start and await readiness
Caller->>Worker: close() during initialization
Worker->>ChildWorkers: signal cancellation
ChildWorkers-->>Worker: unwind startup
Worker->>ProcessGroups: terminate remaining descendants
Worker-->>Caller: raise cancellation and remain CLOSED
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the worker lifecycle management by introducing a formal _Lifecycle enum state machine, replacing several ad-hoc state variables. It also implements active cooperative cancellation of in-progress initialization via close(), serializes rollback with a lock, cleans up orphaned grandchild processes, and adds a pre-init validation check to reject childless workers with pre-registered callables. The review feedback points out a critical race condition where _validate_eligible_targets() is called outside the _hierarchical_start_cv lock, which could allow a concurrent registration to bypass the check. Moving this validation inside the lock block is recommended to maintain safety invariants.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
34d7d4c to
a22010d
Compare
|
Addressed the review in
CI: the Local validation: device-free UT 643 passed / 2 skipped; sim a2a3sim L2/L3 green; ruff + pyright clean. |
a22010d to
eae6fe2
Compare
|
Thanks for the deep review — it correctly caught that the machine linearized INITIALIZING but left READY-teardown, L2/remote mutation, and the cleanup budget unlinearized. Addressed in F1 — close() not atomic (fixed). Added a F3 — L2 + remote mutation bypass (fixed). L2 F5 — cleanup not end-to-end (fixed). F2 — wedged backstop unreachable/unsafe under the GIL (fixed by removing it). You're right: F6a — original completion can fork (fixed). The CLOSING/CLOSED raise in F7 — eligible-target type-blind (fixed, with a correction). Now per-kind: a Docs (fixed). Verified both stale against code and updated: F4 — nested-shm leak on hard-kill (deferred, honestly). The remaining gap is real and matches P0.2 §1.8's recursive SHM journal: a next-level grandchild wedged in a C++ call and SIGKILLed leaks its own nested shm, and the scan-to-signal SIG_DFL window can default-kill a just-READY child before Validation: device-free UT 646 passed / 2 skipped; |
eae6fe2 to
f53daeb
Compare
|
This round caught that my previous CLOSING addition introduced a critical revival bug — thank you. Addressed in #1 (Critical — CLOSING revival, which I introduced). Fixed: #3 (concurrent close doesn't share completion). Fixed: #6 (cleanup deadline not end-to-end). Fixed: #7 (eligible-target — you were right, I had it backwards). Verified against the identity contract + Should-fix (all fixed). Remote frees now work during CLOSING teardown ( Not delivered — the P0.2 core that makes this partial (I am not marking P0.2 complete):
I've listed all three in the PR body as follow-ups and will not claim P0.2 done. Happy to take direction on whether to pursue #2/#4/#5 in this PR or split them. Validation: device-free UT 649 passed / 2 skipped; ruff + pyright clean; sim a2a3sim L2 |
d5319e7 to
9f516f3
Compare
|
Addressed the most dangerous findings in Fixed this round:
Still open (honestly not done — I don't want to under-claim):
Validation: device-free UT 652 passed / 2 skipped, pyright 0, ruff clean; sim a2a3sim L2/L3 pass. |
04463f5 to
80bde2b
Compare
|
Fixed the critical + clearly-correct findings in Fixed:
Still open (in the PR body): #4 full commit-barrier teardown journal; #6 (INITIALIZING-close into the shared transaction); the Validation: device-free UT 654 passed, pyright 0, ruff clean; sim a2a3sim L2/L3 pass. |
80bde2b to
33630aa
Compare
|
Narrowed the PR to a single-shot terminal
Deferred items are enumerated in the PR body. Device-free UT 653 passed / 2 skipped; a2a3sim L2/L3 + dynamic_register green. Watching CI on |
33630aa to
9190fe1
Compare
|
Pushed
Should-fix: stale "concurrent close cancels INITIALIZING" comments updated to the fail-fast contract (worker.py Device-free UT 653 passed / 2 skipped; a2a3sim L2/L3 + dynamic_register green. (The |
9190fe1 to
98bdf38
Compare
|
Pushed
Regression tests added for the two structural fixes (self-deadlock, NEW-worker registry release); the race-shaped cases (joiner-vs-drain-timeout, BaseException-mid-drain) are now correct by construction (single completion path). Device-free UT 656 passed / 2 skipped; a2a3sim L2/L3 + dynamic_register green. Deferred follow-up list unchanged (register/unregister admission txn, dynamic eligibility + fake-chip-L3 harness, init cancellation, commit-barrier journal, add_worker freeze, startup-rollback child, ChipWorker gate, nested-shm). |
98bdf38 to
09d49ac
Compare
|
Pushed
The "can never strand" comment is updated to state the precise remaining (non-blocking) window. Device-free UT 658 passed / 2 skipped; a2a3sim L2/L3 (real-fork teardown via the new two-phase reap) + dynamic_register green. Deferred follow-up list unchanged. |
1f0edb5 to
745d37c
Compare
|
Pushed
Also updated the PR body Testing section (was stale: 653 / onboard-pending → 659 / full matrix green). Device-free UT 659 passed / 2 skipped; a2a3sim L2/L3 (real-fork teardown) + dynamic_register green. Deferred follow-up list unchanged. |
745d37c to
b7a5fee
Compare
|
Pushed [high] Added the requested deterministic regression test Thanks for the earlier confirmations (detach-fold, SHUTDOWN-before-reap-grace, interleaved reap) — those stay as-is. Device-free UT 660 passed / 2 skipped; a2a3sim L2/L3 + dynamic_register green. PR body Testing count synced to 660. |
b7a5fee to
26218da
Compare
|
Good catch — the test was self-consistent but not regression-catching. Fixed in
Device-free UT 660 passed / 2 skipped; full startup-readiness suite green. |
… machine Fold the three overlapping lifecycle truth-sources (_hierarchical_start_state str, _hierarchical_started bool, _initialized bool) into ONE authoritative 5-state _Lifecycle (NEW/INITIALIZING/READY/FAILED/CLOSED — no CLOSING) guarded by _hierarchical_start_cv, and give close() a semantically complete, single-shot terminal contract. This is the narrowed, landable slice of P0.2; the concurrency- and journal-heavy pieces are explicitly deferred (see PR body). Lifecycle / close(): - Single 5-state lifecycle; _initialized / _hierarchical_started are read-only views. close() publishes CLOSED atomically (the admission fence for the leased live-tree APIs) and never reverts to READY. - close() while INITIALIZING fails fast. This worker does not cancel an in-progress init; the _cancel_requested wiring is removed (not dormant). - A caller that WAITS on an in-flight _CloseAttempt always resolves against THAT attempt (never re-reads _close_completion into a successor, never starts a retry) — fixes a concurrent double-teardown race. Only a fresh entry may retry a drain-timeout. The joiner re-checks `done` on a bounded timeout so a skipped notify self-heals. - Completion is published in an innermost resilient finally as three plain attribute assigns (error, incomplete, then done) followed by a locked notify_all(). `done` is set BEFORE the CV acquire, so a BaseException in the (interruptible) acquire or the notify cannot strand a joiner. Every fallible step — drain, teardown, residual synthesis, registry detach — runs before it and folds its error into the single result, so concurrent / later close()s never diverge (one success, one error). - On a terminal close the user-callable registries are detached under the lock but their old refs are released AFTER `done` and OUTSIDE the lock, so a callable __del__ that reenters close() resolves against the done attempt instead of self-deadlocking. Every terminal close releases the registries (incl. a NEW/FAILED worker); only a drain-timeout keeps them. - Teardown is single-shot, TERMINAL, and per-resource best-effort: every region / host-buffer / domain / child is attempted even if one fails; an un-reclaimed resource LEAKS and close() synthesizes a terminal error naming it (never returns success with a residual); a later close() replays the same result. - Child shutdown is two-phase: SHUTDOWN is broadcast to EVERY group first, then all groups are reaped together within one shared deadline (interleaved poll), so a wedged child in one group never starves the reap of healthy children in another. The reap grace starts only after that broadcast — not at teardown entry — so a blocking pre-child cleanup step cannot consume it. A surviving child, an abnormal exit (signal / non-zero), a kept remote-free or comm-domain all surface as errors. - Native teardown stays on the init-owner thread; _ChipWorker.init / _Worker.init release the GIL so a concurrent close() can observe INITIALIZING and fail fast. Admission: - The _operation_lease is the sole admission point for leased ops. Lease-internal re-checks now test resource presence, not public lifecycle (_require_remote_worker_started, _allocate_domain) — so an op admitted before a concurrent close() published CLOSED completes during the drain instead of spuriously failing. Eligibility: startup-only precheck factored into _eligible_target_need (LOCAL_PYTHON->SUB/next, LOCAL_CHIP->chip, REMOTE->its remote worker). Deferred to a follow-up (do NOT read the code/tests as the final contract): register/unregister admission-safety vs close (register publishes before its lease; unregister takes none); post-init dynamic register eligibility re-check; cooperative init cancellation; a genuinely retryable commit-barrier teardown journal; add_worker atomic child freeze; startup-rollback un-reaped child; public ChipWorker ownership gate; recursive nested-shm journal. Device-free UT 660 passed / 2 skipped; a2a3sim L2 vector_add, L3 multi_chip_dispatch, and dynamic_register green.
Scope
Narrowed, landable slice of P0.2. Folds the three overlapping lifecycle
truth-sources (
_hierarchical_start_statestr,_hierarchical_startedbool,_initializedbool) into one authoritative 5-state_Lifecycle(
NEW / INITIALIZING / READY / FAILED / CLOSED— no CLOSING) and givesclose()a semantically complete, single-shot terminal contract. Theconcurrency- and journal-heavy pieces are explicitly deferred (list below) —
please do not read the code or tests as the final contract for those.
Delivered
_hierarchical_start_cv;_initialized/_hierarchical_startedare read-only views.close()is apermanent commitment — publishes
CLOSEDatomically (the sole admission fencefor the leased live-tree APIs) and never reverts to READY.
close()while INITIALIZING fails fast. This worker does not cancel anin-progress init; the
_cancel_requestedwiring is removed (not dormant).A caller waits for READY or FAILED.
close()joins the in-flight_CloseAttemptit observed andsees that attempt's result;
_CloseAttempt.doneis set in afinallyso amid-teardown
BaseException(KeyboardInterrupt) cannot strand joiners.never torn down. A drain-timeout leaves teardown un-attempted with the
tree intact (the one retryable
close()path) so a laterclose()completesit once the op drains.
resource (surviving child, failed C++ close, unfreed shm / domain / region /
host-buffer / remote) leaks and is reported as an error; a later
close()re-raises the same stored result and never re-drives a half-torn tree. A
surviving child is now an error, so
close()never returns success while achild is alive.
aclrtSetDevice-bound);_ChipWorker.init/_Worker.initrelease the GIL._eligible_target_need(per callable kind:
LOCAL_PYTHON→SUB/next,LOCAL_CHIP→chip,REMOTE→itsremote worker), raising at
init()with namespace + hashid.Explicitly NOT delivered — deferred to a follow-up
Do not read the code/tests below as the final contract.
register/unregisteradmission-safety vs a concurrentclose().registerpublishes to the registry before it takes its lease, andunregistertakes no lease — so both can still raceclose(). This PRdoes not claim they are fenced. Fixing it means leasing the whole
READY gate → registry publication → native broadcasttransaction (needs adeadlock-safe registry-lock / lifecycle-lock order).
registerdoes not re-validate target eligibility(startup-only) — documented on
register(). A chip callable dynamicallyregistered on a chipless worker yields a non-dispatchable handle. Unifying the
pre/post-init paths needs a device-free fake-chip-L3 harness; the ~10
existing tests that register
ChipCallables on chipless workers only provethe empty-broadcast facade, not a product expectation.
terminal teardown, not a journal that drops a resource only after its
native free succeeds.
ChipWorkerownership gate; recursive nested-shm journal.add_worker()atomic child freeze (reads child lifecycle then locks only theparent, so a child can still
init()concurrently after the check)._abort_hierarchical) may drop an un-reaped direct child.Testing
vector_add, L3multi_chip_dispatch, STdynamic_register(5/5) — green.st-onboard-a2a3,st-onboard-a5,ut-a2a3,ut-a5).