fix(worker): bound hierarchical startup with readiness protocol - #1396
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:
📝 WalkthroughWalkthroughHierarchical worker startup now uses ChangesHierarchical startup readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ParentWorker
participant ChildWorker
participant Mailbox
participant Scheduler
ChildWorker->>Mailbox: Publish INIT_READY or INIT_FAILED
ParentWorker->>Mailbox: Await child readiness
Mailbox-->>ParentWorker: Return state or error message
ParentWorker->>Scheduler: Start dispatch after all children are ready
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust startup readiness handshake protocol for hierarchical workers, replacing the previous unbounded spin-wait with a bounded readiness barrier. It adds INIT_READY and INIT_FAILED states, monitors child process liveness using waitpid, and enforces a configurable startup timeout to prevent hangs. It also ensures proper cleanup of spawned processes and shared memory on startup failure, and includes comprehensive unit tests to verify these changes. I have no feedback to provide as the implementation is solid and well-tested.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/simpler/worker.py`:
- Around line 3581-3589: Update the hierarchical startup failure handler
surrounding the readiness wait to catch BaseException rather than only
Exception, ensuring KeyboardInterrupt and SystemExit enter the existing cleanup
path. Preserve the _abort_hierarchical call for pre-scheduler failures, set
_hierarchical_start_state to "failed", notify _hierarchical_start_cv, and
re-raise the original exception.
- Around line 1774-1778: Update the startup timeout validation immediately after
assigning self._startup_timeout_s in the worker initialization flow to reject
non-finite values as well as values less than or equal to zero. Use the existing
numeric validation context around self._startup_timeout_s and raise the same
configuration error for NaN or infinity, preserving acceptance of only finite
positive timeouts.
- Around line 3616-3626: Update the initialization wait logic around os.waitpid
and _abort_hierarchical so any PID consumed by waitpid, including both normal
exited-worker handling and ChildProcessError paths as applicable, is marked as
reaped and removed or excluded from the retained PID collection before rollback.
Ensure _abort_hierarchical filters these consumed PIDs and never sends SIGKILL
to a PID already reaped by waitpid.
In `@tests/ut/py/test_worker/test_startup_readiness.py`:
- Around line 202-215: Guarantee teardown for both happy-path worker tests by
moving w4.close() into each test’s finally block before shared-memory cleanup.
Update the sites at tests/ut/py/test_worker/test_startup_readiness.py lines
202-215 and 233-249; preserve the existing assertions and cleanup behavior.
- Around line 52-54: Update the two-child startup readiness test and its
_increment_counter helper to eliminate the shared counter read-modify-write
race. Give each child process its own shared-memory counter slot, or synchronize
access so increments are atomic, and adjust the assertions around the affected
test flow to verify both child increments reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c867ebe-2d1f-4a26-84e6-830d08876e39
📒 Files selected for processing (3)
python/simpler/worker.pysrc/common/hierarchical/worker_manager.htests/ut/py/test_worker/test_startup_readiness.py
74c51cb to
23194d6
Compare
Hierarchical Worker startup could hang forever. A chip (L2) child that failed ChipWorker.init only wrote its error and exited, and the parent spun on `while state != INIT_DONE` with no timeout and no liveness check, so a dead or slow child deadlocked startup (the hw-native-sys#1003 / hw-native-sys#980 class of hang). Next-level (L4->L3) children had no readiness barrier at all: a failing inner init unwound back through the forked copy of the caller's frames instead of reporting a bounded error. Make startup a strong READY boundary: - States: INIT_READY / INIT_FAILED (renamed INIT_DONE), mirrored in the C++ MailboxState enum for parity. - Every forked child (sub, chip, next-level) publishes INIT_READY after its own init succeeds, or INIT_FAILED carrying the original error. A shared `_forked_child_main` guarantees a child always terminates via os._exit and never unwinds an exception into the parent's startup frames (which would let it SIGKILL its inherited-PID siblings); READY is published only after all fallible setup (inner init AND identity tables). - Parent barrier `_await_children_ready` waits per child with a deadline (`startup_timeout_s`, default 300s, validated positive+finite) and a waitpid(WNOHANG) liveness check; INIT_FAILED, a dead child, or a blown deadline raises a bounded RuntimeError. Applied to the sub, chip, and next-level edges (chip and next-level had no/weak barriers before). - Rollback (`_abort_hierarchical`, run on any pre-scheduler BaseException, including KeyboardInterrupt): next-level children that reached their serve loop are closed gracefully (SHUTDOWN + bounded wait) so they unlink their own nested shms; the rest are SIGKILLed; PIDs the barrier already reaped are excluded from the kill (no reused-PID signal); all are reaped and the mailboxes freed. A sibling still mid-init when another fails is SIGKILLed and its nested shms are reclaimed by the resource_tracker at exit — closing that race needs the eager-init handshake (follow-up). Next-level children are still started lazily on first run(); the barrier is recursive plumbing so a later eager-init change makes INIT_READY propagate up only after the whole subtree is ready, with no protocol change. Adds device-free ut coverage (next-level + sub init injection, faked ChipWorker on a2a3sim): bounded init-failure error, child-exit-before- ready, deadline-fires, rollback-reaps-no-leak, graceful-close of a READY sibling, sub-edge liveness, timeout config validation, and healthy-tree happy paths. Every failure test is wrapped in a hard SIGALRM budget so a regression fails fast instead of hanging CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Hierarchical
Workerstartup could hang forever. A chip (L2) child that failedChipWorker.initonly wrote its error and exited, and the parent spun onwhile state != INIT_DONEwith no timeout and no liveness check, so a dead or slow child deadlocked startup (the #1003 / #980 class of hang). Next-level (L4→L3) children had no readiness barrier at all: a failing inner init unwound back through the forked copy of the caller's frames instead of reporting a bounded error.This makes startup a strong READY boundary — it brings the worker subtree up READY, or returns a bounded error in finite time, and rolls a failed epoch back. It is the P0 lifecycle groundwork the downstream memory-ABI / directed-scheduling / EdgeChannel / cross-machine work all sit on.
Changes
INIT_READY/INIT_FAILED(renamedINIT_DONE), mirrored in the C++MailboxStateenum for parity.INIT_READYafter its own init succeeds, orINIT_FAILEDcarrying the original error. A shared_forked_child_mainguarantees a child always terminates viaos._exitand never unwinds an exception into the parent's startup frames (which would let it SIGKILL its inherited-PID siblings);INIT_READYis published only after all fallible setup (inner init and identity-table build)._await_children_ready— waits per child with a deadline (startup_timeout_s, default 300s, validated positive + finite) and awaitpid(WNOHANG)liveness check.INIT_FAILED, a dead child, or a blown deadline raises a boundedRuntimeError. Applied to the sub, chip, and next-level edges._abort_hierarchical, on any pre-schedulerBaseExceptionincl. KeyboardInterrupt) — next-level children that reached their serve loop are closed gracefully (SHUTDOWN + bounded wait) so they unlink their own nested shms; the rest are SIGKILLed; PIDs the barrier already reaped are excluded from the kill (no reused-PID signal); all are reaped and the parent mailboxes freed.Scope / non-goals (honest boundaries)
run()— this PR does not move_start_hierarchicalintoinit(). The barrier is deliberately recursive plumbing so a later eager-init change makesINIT_READYpropagate up only after the whole subtree is ready, with no protocol change. Landing eager-init first would only move the hang from firstrun()toinit().inner.init()when another child fails cannot service SHUTDOWN, so it is SIGKILLed and its partially-created nested shms are reclaimed by the multiprocessing resource_tracker at interpreter exit rather than at rollback. Closing that race needs the eager-init handshake (follow-up). Children that reached READY leak nothing.startup_timeout_sis a per-Worker config kwarg (mirrorspy_control_timeout_s/remote_session_timeout_s), not a behavior gate — the bounded wait is unconditional. Default is generous so a legitimately slow init is never falsely reaped; the point is to bound hangs.Testing
New device-free ut coverage (
tests/ut/py/test_worker/test_startup_readiness.py, next-level + sub init injection, fakedChipWorkerona2a3sim): bounded init-failure error, child-exit-before-ready, deadline-fires, rollback-reaps-no-leak, graceful-close of a READY sibling, sub-edge liveness, timeout config validation, and healthy-tree happy paths. Every failure test is wrapped in a hard SIGALRM budget so a regression fails fast instead of hanging CI; confirmed the pre-fix baseline hangs on these.tests/ut/py/test_worker/+test_remote_l3_lifecycle— 186 passed, 2 skippedpre-commit(ruff check/format, clang-format, clang-tidy, cpplint, pyright) clean on changed files/dev/shmleak across the rollback tests