Refactor: centralize endpoint progress in Scheduler

[Crane-Liu]

Summary

• make the Scheduler thread the single parent-side progress owner for every endpoint lane
• remove per-endpoint WorkerThread threads, dispatch queues, and idle-edge wakeups while preserving lane capacity and completion contracts
• linearize stop with endpoint publication/prepared activation and route submit failures through endpoint-owned quiescence
• prevent a newly published NEXT_LEVEL group from being bypassed between the group and single dispatch phases

Behavioral note

• remote control commands remain serialized behind an in-flight remote task because both use the same command socket; task progress deadlines bound that wait

Testing

• ctest --test-dir tests/ut/cpp/build --output-on-failure (91/91, local macOS)
• tests/ut/cpp/build/test_scheduler (66/66)
• tests/ut/cpp/build/test_remote_endpoint (18/18)
• relevant pre-commit hooks, including clang-tidy, cpplint, and clang-format
• remote Linux test_scheduler on liteserver-hps-148e at previous head aaaa6461 (62/62)

[coderabbitai]

📝 Walkthrough

Walkthrough

The change replaces worker-owned background loops and queues with Scheduler-driven endpoint-lane progress. WorkerManager now submits, polls, stops, and terminalizes endpoint work. Scheduler integration, tests, and runtime documentation now follow this model.

Changes

Endpoint progress refactor

Layer / File(s)	Summary
Endpoint-lane API and ownership contract src/common/hierarchical/worker_manager.h, docs/worker-manager.md, docs/callable-identity-registration.md	WorkerThread and WorkerManager now expose Scheduler-driven progress. Worker-owned threads, queues, and idle callbacks are removed. Dispatch identities are assigned before endpoint submission.
Endpoint submission and progress implementation src/common/hierarchical/worker_manager.cpp	Dispatches submit directly to endpoints. Progress activates, polls, stops, and reports terminal failures. WorkerManager::progress() advances all endpoint lanes.
Scheduler lifecycle and dispatch integration src/common/hierarchical/scheduler.cpp, src/common/hierarchical/scheduler.h, src/common/hierarchical/worker.cpp	The Scheduler advances worker progress before completion draining and dispatch. Stop requests stop workers while running. Completion and wake behavior use endpoint-lane state.
Progress behavior validation and execution documentation tests/ut/cpp/hierarchical/test_scheduler.cpp, docs/hierarchical-level-runtime.md, docs/task-flow.md, docs/remote-l3-worker-design.md	Tests explicitly drive progress and cover endpoint serialization, staging, mailbox completion, stopping, failures, reservations, and wakeups. Documentation describes endpoint-lane ownership and polling.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Scheduler
  participant WorkerManager
  participant WorkerThread
  participant WorkerEndpoint
  participant CompletionQueue
  Scheduler->>WorkerManager: progress()
  WorkerManager->>WorkerThread: progress()
  WorkerThread->>WorkerEndpoint: submit or poll endpoint work
  WorkerEndpoint-->>WorkerThread: acceptance or terminal result
  WorkerThread->>CompletionQueue: publish completion
  Scheduler->>CompletionQueue: drain completions

Loading

Possibly related PRs

• hw-native-sys/simpler#1583: Refactors the same WorkerThread and WorkerManager scheduler-progress architecture.
• hw-native-sys/simpler#1728: Introduces related endpoint-progress and lane-state changes.
• hw-native-sys/simpler#1739: Modifies the same worker progress and endpoint submission paths.

Poem

A rabbit hops where worker threads once ran,
Scheduler paws progress across the span.
Lanes submit, poll, and finish right,
Completions gather in orderly flight.
Stop flags rest; the queues are gone—
Endpoint progress carries on.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.71% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: centralizing endpoint progress in the Scheduler.
Description check	✅ Passed	The description directly explains the Scheduler refactor, behavior changes, shutdown handling, and testing performed.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 `@src/common/hierarchical/worker_manager.cpp`:
- Around line 389-408: The shutdown check and endpoint publication in
src/common/hierarchical/worker_manager.cpp:389-408 must share synchronization
with stop(), so a dispatch loses the admission race and returns
SubmitDispatchResult::STOPPING without calling submit_progress when shutdown
begins. In src/common/hierarchical/worker_manager.cpp:500-525, guard
activate_progress against shutdown after activate_prepared(); do not activate
staged successors once shutdown is set, but continue polling until terminal
progress. Add regression coverage for both stop-versus-submission and
stop-after-activate_prepared-before-progress races.
- Around line 407-427: Update the submit_progress exception handling around
endpoint_->submit_progress to route both std::exception and unknown exceptions
through the endpoint’s report_progress_error contract instead of constructing
WorkerCompletion endpoint failures and calling finish_progress_dispatch
directly. Preserve the exception details in the reported error, allowing
endpoints such as LocalMailboxEndpoint to perform their required poison and
quiescence handling for already-published work.

🪄 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: b1f0b698-c8fd-4430-ac62-a211ae9a1552

📥 Commits

Reviewing files that changed from the base of the PR and between 1f0f006 and aaaa646.

📒 Files selected for processing (11)

• docs/callable-identity-registration.md
• docs/hierarchical-level-runtime.md
• docs/remote-l3-worker-design.md
• docs/task-flow.md
• docs/worker-manager.md
• src/common/hierarchical/scheduler.cpp
• src/common/hierarchical/scheduler.h
• src/common/hierarchical/worker.cpp
• src/common/hierarchical/worker_manager.cpp
• src/common/hierarchical/worker_manager.h
• tests/ut/cpp/hierarchical/test_scheduler.cpp

[ChaoWao]

Review — W1d, fold endpoint progress into the Scheduler

Approve with one thing I'd like acknowledged before merge (details below). I verified by building and running rather than reading, and ran two suites the PR body doesn't claim.

Stated vs real goal

The body says: make the Scheduler the single parent-side progress owner, remove per-endpoint threads/queues/idle-edge wakeups while preserving lane capacity and completion contracts, route shutdown through Scheduler progress. That is what the code does — no goal downgrade.

One scope note, not an objection. The board's W1d wording is "WorkerThread 退化为 endpoint 持有者直至删除" — degrade to endpoint holder, delete later. This PR goes further in one step: it also removes the dispatch queue and the on_idle edge. Both removals are justified — with the scheduler owning progress, a queue between "scheduler decides to dispatch" and "scheduler submits" has no second thread to decouple, and on_idle existed only to tell a different thread that a lane freed up. The class itself survives, so the board's intent holds. Worth stating in the commit message that the queue and idle edge went with the thread, since the board reader will expect only the thread.

Mechanism, read independently

WorkerThread::loop() was: wait-if-idle → drain queue → submit_progress → service activation → poll_progress → finish_progress_dispatch. The waiting half is deleted and the working half becomes WorkerThread::progress(), which WorkerManager::progress() fans over both pools and Scheduler::run() calls once per iteration.

The scheduler's wait becomes conditional on any_busy(). This is the load-bearing line, and it's right: each WorkerThread previously slept only while it was idle and spun while inflight_ > 0. Guarding the completion_cv_ wait on any_busy() reproduces exactly that discipline with one thread — idle sleeps, busy polls. No sleep, no yield, so codestyle.md §5 is satisfied.

Dispatch loses its queue: submit_dispatch now calls endpoint_->submit_progress inline. The counter increments moved ahead of that fallible call, and failure is reported by synthesizing a COMPLETED-failure progress instead of rolling back. I checked the balance, because an unbalanced inflight_ here would pin any_busy() true and spin the scheduler forever: finish_progress_dispatch decrements inflight_ on the terminal path (worker_manager.cpp:590), so the early increment is matched. Correct.

next_dispatch_id_ lost its mutex along with the queue. That's safe — both touches are inside submit_dispatch, reachable only via dispatch/dispatch_prepared ← dispatch_claimed ← dispatch_ready, all on the scheduler thread. The now-unused mu_ member was removed rather than left dead.

Verified, not assumed

Check	Result
C++ UT, built at PR head	91/91 (ctest -LE requires_hardware)
Per-worker threads actually gone	confirmed — remaining std::thread uses are the transient control fan-out, not progress loops
inflight_ balanced on the new failure path	confirmed at worker_manager.cpp:590
Shutdown / poll-exception coverage	already present and passing: StopTerminalizesOutstandingProgress, ProgressStopRepeatsUntilOutstandingWorkTerminalizes, PollExceptionTerminalizesOutstandingProgressAndStopsTheDriver, StopKeepsWorkerBusyUntilItsLastCompletionIsPublished

Two suites the body doesn't claim, which matter here because this removes the threads that drove every dispatch:

• Python UT — 1297 passed / 13 skipped (3 consecutive clean runs).
• a2a3 onboard sweep under task-submit — 56 passed + 24 passed / 2 skipped, 0 failures. This is the evidence that matters: the whole parent-side dispatch path now runs on one thread, on real silicon, at baseline parity.

Docs: five runtime docs updated in the same commit, and the stale class/API comments (WorkerThread — one worker, one std::thread, the on_idle ordering paragraph, the orchestrator.cpp "Reached from a WorkerThread" note) moved with the code. That satisfies doc-consistency.md §4 — this is the part these refactors usually miss.

Issues

Should fix — or explicitly acknowledge in the commit message:

LocalMailboxEndpoint::submit_progress takes mailbox_mu_ (worker_manager.cpp:693), and it now runs on the scheduler thread while loop_mu_ is held. run_control_command holds that same mailbox_mu_ while blocking on the child, with an infinite default timeout by design ("Issue a control sub-command and block until the child publishes CONTROL_DONE. Caller must hold mailbox_mu_").

So a control command in flight on worker X, concurrent with a dispatch to worker X, now stalls every endpoint's progress rather than just worker X's — and holds loop_mu_, which Orchestrator::set_scheduler_loop_mutex shares with allocator compaction.

To be fair about provenance: the stall is not created here. Before this PR, WorkerThread X's loop blocked on the same mutex at the same call. What changes is blast radius — one worker becomes all workers plus compaction. I checked the paths that would make it a true cycle and they're clear: activate_progress and poll_progress take only progress_mu_, so a deferred control waiting on a staged frame can still be released.

I'm not asking for a redesign — the fix is the wakeup-primitive work that W1d deliberately isn't. I'd like one of: a note in the commit message that the stall window widened, or a test that pins it. Silence is what makes it a surprise later.

Consider:

1. progress() is called before Phase 1 (drain completions), while the board specifies between Phase 1 and Phase 2. Before-drain is strictly better — completions produced by the poll are drained in the same iteration instead of waiting for the next — so I read this as a deliberate improvement. Worth one line saying so, since it deviates from the written plan.
2. The deleted comment // A failed allocation consumes neither capacity nor dispatch identity. documented an invariant that genuinely changed: a failed submit now consumes a dispatch id. The behavior is fine (ids need only be unique), but the comment was deleted rather than restated. Per comments.md, the replacement would be a present-tense fact about what a failed submit now costs.

Not raised: test_second_child_failure_reaps_first failed once in my first full Python run here, then passed 3/3. I saw the same test flake once in six runs on the W1b branch, and it passes on main. Two unrelated branches, ~1 in 5 — I'm treating it as a pre-existing intermittent flake, not this PR's doing, and flagging it as a repo-level issue rather than a finding against you.

Verdict

Approve. The translation is faithful, any_busy() is the right guard, the invariants that could have broken silently (inflight_ balance, dispatch-id single-threading, shutdown drain) hold, and it's green at baseline parity through onboard silicon. The mailbox_mu_ widening is the one thing that should be written down before this merges.

Disclosure: I had independently planned this same change and was about to implement it, so I reviewed against the design I'd derived — including the places where this PR chose differently (before-drain placement, removing the queue and idle edge) and, on both counts, chose better than my plan did.