Fix: count a pending accept only for a slot that reaches an endpoint - #1776
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 submission path now increments pending acceptance counts only after successful publication. Failed producer propagation leaves poisoned consumers uncounted. A regression test verifies that the acceptance fence opens without waiting for the poisoned consumer. ChangesAcceptance accounting
Estimated code review effort: 2 (Simple) | ~10 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: 1
🤖 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/orchestrator.cpp`:
- Around line 899-906: Update the acceptance accounting around
increment_run_accepts() in submit_impl() to close the race with
Scheduler::poison_task(): synchronize acceptance registration with failure
claims, or compensate any already-published slots that fail before registration.
Ensure every increment_run_accepts() has exactly one matching
mark_task_accepted() and prevent pending_accepts from remaining elevated after a
poisoned slot.
🪄 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: b44a688a-e060-4115-9b40-4f9ddc5d5d59
📒 Files selected for processing (2)
src/common/hierarchical/orchestrator.cpptests/ut/cpp/hierarchical/test_orchestrator.cpp
Orchestrator::submit_task raised the run's accept count before deciding whether the slot was poisoned by a failed producer. A poisoned slot is marked failed and consumed without ever being dispatched, so mark_task_accepted -- the sole matching decrement -- is never called for it, and pending_accepts stays above zero for the rest of the run. The run still finishes: acceptance_ready() is submission_closed && (pending == 0 || is_terminal(phase)), so the terminal disjunct eventually satisfies it. What is lost is when the fence opens. It opens at completion instead of at acceptance, and Worker.submit() waits on the prior handle's acceptance before building the next graph, so a single poisoned producer serialises the next submission behind the whole run -- removing exactly the overlap the acceptance fence exists to provide, and with it the overlap the direct-chip pipeline was built for. The count now happens inside the publication lock, in the branch whose compare-exchange moves the slot out of BUILDING. That branch is the only moment at which the slot is certain to reach an endpoint, and both sides of it are wrong: counting earlier includes slots that fail instead, and counting after the lock is released leaves a window in which a published slot is already visible to Scheduler::poison_task, which can claim and fail it before the count lands -- the same permanently-elevated count in a narrower race. pending_accepts is incremented directly rather than through increment_run_accepts, whose get_run() would take runs_mu_ while this thread holds fanout_mu; that left increment_run_accepts with no callers, so it is removed. The regression test asserts the fence opens while the run is still live, with a healthy sibling owing the only accept that can still arrive; it fails against either wrong ordering with "Actual: false" on exactly that assertion. The sibling has to be submitted before the producer is failed, because a run that has recorded an error rethrows it from the next submit_next_level rather than admitting anything more. Fixes #1556. Verification: 92/92 C++ unit tests (ctest -LE requires_hardware), 1311 Python unit tests, and the a2a3 onboard sweep at 56 passed plus 24 passed / 2 skipped in the resource phase with zero failures.
9762ef4 to
ed1b4ec
Compare
|
@coderabbitai 那条 inline 成立并已修，细节写在 thread 上。摘要： 它指出的窗口是本 PR 新引入的。 连带清理：新写法不再经 本轮按要求只处理 comments，未动 CI。已 rebase 到 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Changed files =='
git diff --name-only HEAD~1..HEAD 2>/dev/null || true
echo '== Orchestrator outline =='
ast-grep outline src/common/hierarchical/orchestrator.cpp --items all --match 'submit|increment_run_accepts|poison' 2>/dev/null || true
echo '== submit implementation context =='
rg -n -C 35 'compare_exchange_strong|pending_accepts|increment_run_accepts|fanout_mu|poison' \
src/common/hierarchical/orchestrator.cpp
echo '== Remaining increment_run_accepts references =='
rg -n -C 2 '\bincrement_run_accepts\b' . \
-g '!**/.git/**' || true
echo '== Regression-test context =='
rg -n -C 25 'pending_accepts|acceptance fence|poisoned|producer.*fail|failed producer' \
tests/ut/cpp/hierarchical/test_orchestrator.cppLength of output: 41040
失败生产者路径和 CAS 失败路径均不会递增该计数。 ✏️ 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
Orchestrator::submit_taskraised the run's accept count before deciding whether the slot was poisoned by a failed producer. A poisoned slot is marked failed and consumed without ever being dispatched, somark_task_accepted— the sole matching decrement (orchestrator.cpp:517) — is never called for it.pending_acceptsthen stays above zero for the rest of the run.The run still finishes.
acceptance_ready()issubmission_closed && (pending == 0 || is_terminal(phase)), so the terminal disjunct eventually satisfies it. What is lost is when the fence opens.Why that matters more now than when #1556 was filed. The fence opens at completion instead of at acceptance, and
Worker.submit()waits on the prior handle's acceptance before building the next graph. So one poisoned producer silently serialises the next submission behind the entire run — removing exactly the overlap the acceptance fence exists to provide. #1556 was filed on 2026-07-29, before W1b/W1c; the direct-chip pipeline that landed since is built on that same overlap, so the cost is now higher than the issue describes.Fixes #1556.
The change
Move the increment past the poisoned branch, so only a slot certain to reach an endpoint owes an accept. Nothing else moves.
Two things I checked before trusting that:
returnin between is the poisoned one — the path that must skip the count. (Thecatchat:820is upstream of both.)run->completion_mu, unlikedecrement_run_accepts, which takes it specifically so a decrement cannot land between a waiter's predicate check and its block.acceptance_ready()requiressubmission_closed, which cannot be true whilesubmit_taskis still admitting into that run — so there is no waiter to race.Testing
The regression test asserts the fence opens while the run is still live, with a healthy sibling owing the only accept that can still arrive.
Verified it fails against the previous ordering, not just that it passes now — I reverted the fix, rebuilt, and got exactly the assertion this bug predicts:
One non-obvious constraint the test had to respect: the sibling must be submitted before the producer is failed, because a run that has recorded an error rethrows it from the next
submit_next_level(orchestrator.cpp:706) rather than admitting anything more. My first draft submitted it after and died withC++ exception with description "producer failed".ctest -LE requires_hardware)task-submit, device lock held)The onboard sweep is included because this changes the orchestrator's admission path, which every dispatched task crosses.