Docs: align the threading model with the single progress owner - #1761
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes update documentation and comments for the non-blocking Scheduler and worker-endpoint model. They clarify retry progress, polling behavior, single-threaded child progress, and mailbox synchronization effects. ChangesScheduler and worker progress
Estimated code review effort: 1 (Trivial) | ~5 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/remote-l3-worker-design.md`:
- Around line 140-141: Update the WorkerThread design description to remove the
`run()` references unless they represent a separately supported interface; align
the documented progress flow with the endpoint methods `submit_progress`,
`activate_progress`, and `poll_progress` used by `WorkerEndpoint`,
`LocalMailboxEndpoint`, and `RemoteL3Endpoint`.
In `@docs/worker-manager.md`:
- Around line 451-457: Qualify the opening description of submit_progress and
poll_progress in the “one thread per child” alternative to state that they do
not wait for child completion, while acknowledging they may block on mailbox_mu_
contention. Keep the existing explanation of run_control_command and the
resulting stall unchanged.
🪄 Autofix
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 Plus
Run ID: dbcdf737-055d-4e21-ad40-f1ce87434c9e
📒 Files selected for processing (5)
docs/orchestrator.mddocs/remote-l3-worker-design.mddocs/scheduler.mddocs/worker-manager.mdsrc/common/hierarchical/worker_manager.cpp
The async-pipeline track changed who runs endpoint progress and what it costs. Six places still described the previous world, and two of them were reasoning rather than description — a rationale section arguing against the design that was subsequently adopted, and a race explanation whose premise had become false while its conclusion stayed true. Both lead a reader to the wrong conclusion about thread safety, which is worse than saying nothing. The stall window is recorded where it happens rather than described from outside. submit_progress takes mailbox_mu_ and runs on the Scheduler thread, which holds loop_mu_ and is the sole progress owner for every endpoint; run_control_command holds that same mutex while it blocks on the child, with no deadline by default. So an outstanding control command on any one endpoint stops progress on all of them and keeps loop_mu_ held, which Orchestrator shares with allocator compaction. Nothing in the function bounds that wait. This is a property of the current arrangement, not a defect being introduced or fixed: the wait already existed per endpoint, and only the set of work it blocks is wider. Removing it needs a wakeup primitive on the mailbox, which is a change to the host/child contract and out of scope here. worker-manager.md 5.3 asked why each child had its own thread and answered with zero queue contention and one-thread-per-child simplicity. Neither is a property of the code any more, and the alternative it rejected is close to what now exists. It now asks why there is one progress owner and answers with the reason a dedicated thread cannot help: submit_progress and poll_progress never wait for child completion, so such a thread has no completion to block on and spins for as long as its child is busy. That is stated as the narrow claim it is — not waiting for completion is not the same as never blocking, and the same section names mailbox_mu_ as the case where it does block. scheduler.md attributed the missed-edge hazard to changes landing outside the Scheduler thread and closed by crediting the on_idle callback for the retry. Completion is now published on the Scheduler thread and on_idle no longer exists. The ordering it documents is still real and still load-bearing for the stop path, so that survives; what replaces the edge is that the Scheduler does not park while any lane is busy, so an occupied lane gets another round without needing one. It also cited active_inflight_, removed when the lane array replaced the parallel counters. Four remaining references named endpoint entry points that no longer exist. orchestrator.md described a group member through WorkerEndpoint::run; remote-l3-worker-design.md used it for both the mailbox description and the required parent-side completion behavior; buffers-and-transports.md listed WorkerEndpoint::run and WorkerEndpoint::control as the surface the Orchestrator, Scheduler and WorkerThread see. Each now names the progress calls that replaced them. The PR-sequence entry in remote-l3-worker-design/implementation-plan.md keeps its WorkerEndpoint::run wording: it records what that step delivered at the time, and rewriting it would misstate the history rather than correct a claim about the present. No behavior change: the only edit under src/ is a comment, and the diff there contains no non-comment lines. Verification: 91/91 C++ unit tests (ctest -LE requires_hardware), clang-format clean, markdownlint-cli2 clean against tests/lint/.markdownlint.yaml, and a grep audit confirming no stale spelling survives -- searched by concept (Endpoint::run, Endpoint::control, WorkerThread::run, worker->run) rather than only the spellings already found, with every 0-hit result paired with a non-zero positive control so an empty result cannot pass for a correct one. Python and onboard suites were not run: nothing executable changes.
f485c8c to
7cd18ea
Compare
|
@coderabbitai Both inline findings were valid and both were mine — fixed, each answered on its thread.
The root cause of the miss is now fixed in the audit itself: it greps Re-verified after the fixes and a rebase onto |
|
Tip For best results, initiate chat on the files or code changes.
The revised wording distinguishes “never wait for child completion” from “never blocking.” The audit now checks the exact obsolete API spellings and validates each expected zero-hit result with a positive control. The historical entry in ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
1 participant
Summary
The async-pipeline track (W1a #1650 → W1b #1748 → W1c #1750 → W1d #1754, plus E1′ #1739) changed who runs endpoint progress and what it costs. Five places in the tree still describe the previous world.
Two of them are reasoning, not description, which is why this is worth a PR rather than a drive-by:
docs/worker-manager.md§5.3 asked "Why one WorkerThread per child?" and justified it with "one child = one thread that drives it" and "zero contention on queue access". Both describe machinery Refactor: centralize endpoint progress in Scheduler #1754 deleted — and the alternative that section rejects ("N children share one dispatch queue") is close to the design that was then adopted. Refactor: centralize endpoint progress in Scheduler #1754 updated §2 but left §5.3 arguing against its own change.docs/scheduler.mdexplained the missed-edge hazard by saying the change happens "outside the Scheduler thread", and closed by crediting theon_idlecallback for the retry. Completion is now published on the Scheduler thread, andon_idleno longer exists.A reader trusting either draws a wrong conclusion about thread safety. That is worse than the docs saying nothing.
The one item that is mine
I raised the
mailbox_mu_stall widening as a Should-fix on my #1754 review and asked for it to be recorded before merge. It merged without it, so the behavior change is onmainwith nothing describing it. This PR closes that.submit_progresstakesmailbox_mu_and now runs on the Scheduler thread, which holdsloop_mu_and is the sole progress owner for every endpoint.run_control_commandholds that same mutex while blocking on the child, with no deadline by default. So an outstanding control command on any one endpoint stops progress on all of them and keepsloop_mu_held — whichOrchestrator::set_scheduler_loop_mutexshares with allocator compaction.To be precise about provenance: this is not a defect being introduced or fixed. The wait already existed per endpoint before #1754; only the set of work it blocks got wider. Removing it needs a wakeup primitive on the mailbox, which changes the host↔child contract and is deliberately out of scope for W1d and for this PR.
Changes
src/common/hierarchical/worker_manager.cppdocs/worker-manager.md§5.3docs/scheduler.mdon_idle+ deletedactive_inflight_; ordering conclusion preserved because it is still load-bearing for the stop pathdocs/orchestrator.mdworker->run" —WorkerEndpoint::runretired by #1739docs/remote-l3-worker-design.mdTASK_DONE" — same, and it sits in a current file-map sectionorchestrator.mdandremote-l3-worker-design.mdtrace to my own #1739;worker-manager.mdandscheduler.mdto #1754, which I reviewed and approved.No behavior change
The only edit under
src/is a comment. Verified mechanically — the diff contains no non-comment lines there:Testing
ctest -LE requires_hardware, CI's filter) after rebuildingclang-format --dry-run --Werrorcleanmarkdownlint-cli2clean againsttests/lint/.markdownlint.yamlone thread that drives it,active_inflight_,worker->run,blocks until TASK_DONE,on_idle) — each 0-hit result paired with a non-zero positive control, so an empty result cannot pass for a correct onePython UT and the onboard sweep were not run: nothing executable changes, and running them would be theatre rather than evidence.