Fix: block scheduler while group heads wait

[puddingfjz]

Summary

• replace ready-queue/stop level predicates with a consumed wake generation
• keep completions as queued wake events while consuming readiness and stop notifications once
• wake on incomplete group-member completion so newly idle targets can run queued singles
• remove the blocked group busy-spin path and document the wake model

Why

A blocked group leaves its queue non-empty, so the previous condition-variable predicate remained permanently true and repeatedly ran dispatch. The stop flag had the same level-triggered behavior while workers were still busy.

Review context

#1565 (comment)

Testing

• pre-fix regression observed thousands of extra dispatch rounds in 30 ms
• partial group-member wake regression repeated 20 times
• test_scheduler: 27/27 passed
• non-hardware C++ UT: 67/67 passed

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 04b1b7d3-c1ef-4a79-8324-32f218604ffb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The scheduler now coordinates wake-ups through a consumed generation counter and queued completions. It avoids repeated dispatch for blocked groups and exposes dispatch-round counts for validation.

Changes

Scheduler wake-generation flow

Layer / File(s)	Summary
Wake-generation state and signaling src/common/hierarchical/scheduler.h, src/common/hierarchical/scheduler.cpp, docs/scheduler.md	Scheduler startup, stop handling, readiness notifications, and incomplete group completions update the wake generation. The scheduler documentation describes the new wait behavior.
Wait predicate and dispatch validation src/common/hierarchical/scheduler.cpp, src/common/hierarchical/types.h, src/common/hierarchical/types.cpp, tests/ut/cpp/hierarchical/test_scheduler.cpp	The scheduler waits for queued completions or a new wake generation. Dispatch rounds are counted. Aggregate queue emptiness inspection was removed. The regression test verifies that blocked groups do not trigger repeated dispatch rounds.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Worker
  participant Scheduler
  participant CompletionQueue
  participant dispatch_ready
  Worker->>Scheduler: report readiness or completion
  Scheduler->>CompletionQueue: queue completion
  Scheduler->>Scheduler: observe wake generation
  Scheduler->>dispatch_ready: dispatch after completion or new generation

Loading

Poem

I’m a rabbit by the queue,
Watching wake counts rise anew.
Blocked groups wait without a fuss,
Completions wake the scheduler bus.
Round counts hop; the flow is clear.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Description check	✅ Passed	The description clearly explains the scheduler wake-generation changes, the blocked-group busy-spin issue, and the test results.
Title check	✅ Passed	The title clearly identifies the main change: preventing scheduler activity while group heads are blocked.

---

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: 1

🧹 Nitpick comments (1)

tests/ut/cpp/hierarchical/test_scheduler.cpp (1)

866-875: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert progress after one group member completes.

The test completes worker_b immediately after worker_a. The aggregate group completion can then wake the scheduler and make this test pass.

Keep worker_b running after worker_a.complete(). Assert that the pending single for worker A dispatches while worker B remains busy. This validates the wake-only path for incomplete group-member completion.

🤖 Prompt for 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.

In `@tests/ut/cpp/hierarchical/test_scheduler.cpp` around lines 866 - 875, Update
the test around worker_a.complete() so worker_b remains running while waiting
for worker_a’s second dispatch; assert worker_a reaches two dispatches and
worker_b remains at one before completing worker_b. Move worker_b.complete()
after these assertions to validate progress without aggregate group completion.

🤖 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/scheduler.md`:
- Around line 40-45: Update the scheduler documentation near the description of
worker completion and wake generations to state that an incomplete group-member
completion advances wake_generation_ and notifies the scheduler without pushing
an entry to the completion FIFO. Keep the existing completion FIFO behavior for
completions that produce a completion event.

---

Nitpick comments:
In `@tests/ut/cpp/hierarchical/test_scheduler.cpp`:
- Around line 866-875: Update the test around worker_a.complete() so worker_b
remains running while waiting for worker_a’s second dispatch; assert worker_a
reaches two dispatches and worker_b remains at one before completing worker_b.
Move worker_b.complete() after these assertions to validate progress without
aggregate group completion.

🪄 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 Plus

Run ID: 20564ba4-a989-454f-9bba-2591b22a7e69

📥 Commits

Reviewing files that changed from the base of the PR and between a07bc90 and 56e6181.

📒 Files selected for processing (6)

• docs/scheduler.md
• src/common/hierarchical/scheduler.cpp
• src/common/hierarchical/scheduler.h
• src/common/hierarchical/types.cpp
• src/common/hierarchical/types.h
• tests/ut/cpp/hierarchical/test_scheduler.cpp

💤 Files with no reviewable changes (2)

• src/common/hierarchical/types.h
• src/common/hierarchical/types.cpp

[ChaoWao]

Reconciled with #1541 (whole-run FIFO admission) and addressed review feedback. Summary:

Merge with #1541

The branch was rebased onto current main, which had moved with #1541. The wake-generation model now sits on top of the run-partitioned ready queues:

• The wait predicate is completion_queue non-empty || wake_generation != observed — queue-emptiness and the stop flag are gone from it (a blocked group head no longer keeps the predicate true; stop() and every submit advance the generation instead).
• Every ready-queue push site wakes the scheduler: submit and run admission via ready_notify_cb_ (generation bump), incomplete group-member completions via the new notify_ready() calls in worker_done, terminal completions via the completion push + notify.
• NextLevelReadyQueues::empty() / empty(RunId) are deleted — their only consumer was the old predicate.
• ActiveRunSwitchCannotBypassSuccessorGroupReservation (from Update: make whole-run FIFO admission failure-safe #1541): the before_claim_cb seam now signals the run switch with sched.notify_ready(), mirroring production run admission, which advances the wake generation via ready_notify_cb_. In the edge-triggered model the switch itself is the wake event.
• Rebased the merged head through the full local suite: 40 tests × 100 iterations clean.

Review items

• CodeRabbit docs thread — already satisfied by the current wording ("group-member completions that do not enqueue a terminal task completion advance a wake generation"); replied and resolved.
• CodeRabbit nitpick (test 866-875) — already addressed in the current revision: BlockedGroupReservesTargetsThatBecomeIdleOneAtATime completes worker_a and asserts its single progresses (group still RUNNING, worker_b still at 2) before worker_b completes.
• Flake risk (my review) — BlockedGroupSleepsUntilWorkerCompletion no longer sleeps a fixed 30 ms and asserts the round count is frozen; it polls for a quiet window (20 ms steps, 500 ms deadline). A busy-spinning scheduler still fails it; a merely slow one does not.
• Empty "CI: rerun failed checks" commit — dropped; the PR is one commit again.

Testing

• Local: test_scheduler 40 tests × 100 iterations pass; full tests/ut/cpp build clean.
• CI: will re-run on this push.

The wake-loss audit: all generation bumps and completion pushes happen under completion_mu_, and the waiter evaluates the predicate under the same mutex, so no notify can be lost between predicate check and sleep. The one path that used to rely on the level-triggered predicate — a run switch without an admission notify — exists only in the test seam and is now signaled explicitly.

[ChaoWao]

Rebased onto current main (which gained #1583, generation-safe two-frame local endpoint).

The wake-generation model now sits on top of #1583 staging/activation machinery:

• request_stop() carries the generation bump (the split from Add: generation-safe two-frame local endpoint #1583); stop() joins as before.
• The level-triggered predicate additions from Add: generation-safe two-frame local endpoint #1583 (needs_activation(active), stageable_successor_ready(...)) are removed along with the queue-emptiness checks — the wake model covers them: run admission, submission, and run-close all advance the generation via ready_notify_cb_, and the mailbox ordering (a successor frame stages before the previous frame completes) guarantees the scheduler sees staging/activation state on the completion wake. dispatch_preparable_next_level_singles() and groups_empty/single_empty/singles_empty remain and are driven from every dispatch round as before.
• One Add: generation-safe two-frame local endpoint #1583 test (PreparedSuccessorGroupRemainsQueuedUntilPromotion) migrated from the removed NextLevelReadyQueues::empty(run) to groups_empty(run) — same assertion, new API.
• Dead empty() / empty(run) variants on NextLevelReadyQueues removed.

Local: 56 tests x 50 iterations clean; full tests/ut/cpp build clean; clang-format clean. CI re-running on this push.