Fix: wake L3 scheduler on root ready tasks#1258
Conversation
![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 replaces the timed wait in the scheduler loop with an event-driven notification mechanism using a callback from the orchestrator. A critical race condition was identified in Scheduler::notify_ready(), where a lost wakeup could occur because the notification is triggered without holding the completion_mu_ mutex. To prevent this, the mutex should be acquired before calling completion_cv_.notify_one().
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.
|
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:
📝 WalkthroughWalkthroughThe Orchestrator now accepts a ready-notification callback invoked when a submitted task becomes immediately ready. The Scheduler exposes a notify_ready() method and replaces its timed polling wait with a predicate-based wait. ReadyQueue gains a thread-safe empty() accessor, and Worker wires these together. ChangesReady-Notification Wiring
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant ReadyQueue
participant Scheduler
participant Worker
Worker->>Orchestrator: init(..., ready_notify_cb = notify_ready lambda)
Orchestrator->>ReadyQueue: push slot (task READY, live_fanins == 0)
Orchestrator->>Scheduler: ready_notify_cb_() -> notify_ready()
Scheduler->>Scheduler: completion_cv_.notify_one()
Scheduler->>Scheduler: wait wakes on predicate (queue non-empty)
Scheduler->>ReadyQueue: check empty()
ReadyQueue-->>Scheduler: false
Scheduler->>Scheduler: process ready task
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 |
42d92b3 to
94429d1
Compare
Root L3 tasks were pushed into ReadyQueue without waking the Scheduler's own condition variable. The scheduler only noticed them after its 1 ms wait timeout, adding avoidable latency before L2 dispatch. - Notify the Scheduler when Orchestrator publishes a root ready task - Include ready queues in the Scheduler wait predicate - Switch the loop to event-driven wait instead of wait_for polling
…-sys#1260) hw-native-sys#1258 moved the Scheduler off the ReadyQueue's condition variable onto its own completion_cv_, and added a `ready_notify_cb` parameter to `Orchestrator::init` so `submit()` can wake the scheduler when a root task goes READY. Production wires it (worker.cpp: `[this]{ scheduler_.notify_ready(); }`), but the four `test_scheduler` fixtures were not updated and relied on the parameter's `{}` default — so submissions pushed to the ready queue but never notified the scheduler, which blocked forever on completion_cv_. test_scheduler then hung into the PR `ut (ubuntu)`/`ut (macos)` 15-min cap (issue hw-native-sys#1260), affecting every PR that runs that job. Pass `[this/&sched]{ sched.notify_ready(); }` at all four Orchestrator::init call sites in test_scheduler.cpp (3 TEST_F SetUp methods capture `this`; the one plain TEST captures the local `sched` by reference). Verified: test_scheduler 17/17 pass in 91 ms (was: hang); full hierarchical cpput suite 52/52.
* Refactor: remove orphaned WorkerManager idle-picker methods, rename survivor #1009 (remote endpoint eligibility scheduling) migrated the scheduler's worker selection from pick_idle_excluding to pick_idle_excluding_eligible but left the old entry points behind. Git history confirms these are migration residue, not in-progress scaffolding: - pick_idle_excluding — the scheduler's only call site was rewritten to pick_idle_excluding_eligible in #1009; the old method was kept as a thin forwarding shim (unconstrained eligible list) that nothing calls. - pick_idle / pick_n_idle — base variants with no call site anywhere in the tree's history (only ever declared/defined since #587). - shutdown_children — WorkerManager method with zero call sites (distinct from the live WorkerThread::shutdown_child, which it wrapped). All four are zero-call, host-only, non-virtual, and not part of any wire protocol or serialized struct. With its distinguishing siblings gone, the survivor's `_excluding_eligible` suffix is vestigial naming baggage — rename pick_idle_excluding_eligible -> pick_idle (reusing the freed name; the old pick_idle(type) had zero callers so there is no collision). Update the scheduler.h data-flow comment and the worker-manager.md / scheduler.md docs to match. Deliberately kept: pick_idle (the renamed live selector) and WorkerThread::shutdown_child (8 live call sites). Verified: both runtimes build+link clean (-Wall -Wextra, no warnings); WorkerManager python test passes. (The pre-existing gtest ABI link error in the local cpput tree reproduces on clean upstream/main and is unrelated; CI builds cpput in a clean container.) * Fix: wire ready_notify_cb in test_scheduler fixtures (fixes #1260) #1258 moved the Scheduler off the ReadyQueue's condition variable onto its own completion_cv_, and added a `ready_notify_cb` parameter to `Orchestrator::init` so `submit()` can wake the scheduler when a root task goes READY. Production wires it (worker.cpp: `[this]{ scheduler_.notify_ready(); }`), but the four `test_scheduler` fixtures were not updated and relied on the parameter's `{}` default — so submissions pushed to the ready queue but never notified the scheduler, which blocked forever on completion_cv_. test_scheduler then hung into the PR `ut (ubuntu)`/`ut (macos)` 15-min cap (issue #1260), affecting every PR that runs that job. Pass `[this/&sched]{ sched.notify_ready(); }` at all four Orchestrator::init call sites in test_scheduler.cpp (3 TEST_F SetUp methods capture `this`; the one plain TEST captures the local `sched` by reference). Verified: test_scheduler 17/17 pass in 91 ms (was: hang); full hierarchical cpput suite 52/52.
1 participant
Summary
wait_forpolling fallback with event-drivenwait.Why
Root L3 tasks were pushed into
ReadyQueue, but the Scheduler waited on itsown
completion_cv_. Since the wait predicate only checked completions andstop, root ready tasks could sit until the 1 ms timeout fired before dispatching
to the L2 child.
Measured on pypto-serving Qwen3-14B decode, the L3-to-L2 median gap dropped
from 1.627 ms to 0.602 ms. The pre-L2 startup portion dropped from 1.340 ms to
0.471 ms.
Testing
pip install --no-build-isolation -e .python -m pytest tests/ut/py/test_worker/test_host_worker.py tests/ut/py/test_worker/test_group_task.py tests/ut/py/test_worker/test_error_propagation.py -qpython -m pytest tests/st/a2a3/tensormap_and_ringbuffer/test_l3_dependency.py tests/st/a2a3/tensormap_and_ringbuffer/test_l3_group.py --platform a2a3sim --device 0-1 -qtask-submit, 16 tokens