host_build_graph: fail-fast boot barriers on an out-of-range AICPU thread - #1566
Open
ChaoZheng109 wants to merge 1 commit into
Open
host_build_graph: fail-fast boot barriers on an out-of-range AICPU thread#1566ChaoZheng109 wants to merge 1 commit into
ChaoZheng109 wants to merge 1 commit into
Conversation
…read An out-of-range thread_idx (a broken affinity mapping, or run() invoked more than aicpu_thread_num_ times) returned -1 before reaching the classify barrier and the finish barrier. The boot leader then spun forever on `classify_arrived_ < aicpu_thread_num_`, and `finished_` never reached the thread count — so the whole op hung into the op-execute timeout (507018) that masks the real cause. Route the invalid thread through a shared abort instead: - A `boot_abort_` flag is published by any run() that cannot participate. Every boot wait (classify_ready_, classify_arrived_, runtime_init_ready_) also breaks on it, so a missing participant never leaves the rest spinning. - Dispatch is skipped under boot_abort_ on every thread: a run missing a core-owning thread can never reach is_completed(), so the survivors would otherwise spin the dispatch loop forever rather than fail fast. - The invalid thread skips the index-keyed work (boot, classify slice, shutdown) but still reaches the shared finish barrier, so finished_ publishes and the host observes the failure promptly. - Every thread returns -1 on abort so the host reads no partial success. This closes both boot-barrier hangs (classify_arrived_ and finished_count_) with one mechanism. The path is only reachable via a fatal platform misconfiguration; the change converts the resulting hang into a clean, legible failure. Scope: a2a3 host_build_graph only.
📝 WalkthroughWalkthrough
ChangesAICPU boot-abort coordination
Estimated code review effort: 3 (Moderate) | ~20 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/a2a3/runtime/host_build_graph/aicpu/aicpu_executor.cpp`:
- Around line 229-246: Update the run admission and shared finish-barrier logic
around valid_idx and finished_count_ so only the first aicpu_thread_num_
participants are admitted to the per-run cohort and may contribute to classify
or finish counters. Overflow invocations must set boot_abort_, return -1, and
bypass both barriers, preventing them from satisfying the configured finish
count or triggering premature runtime destruction; preserve normal behavior for
admitted participants.
🪄 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: ced891c4-3fdc-4629-b41e-05551f422c0a
📒 Files selected for processing (1)
src/a2a3/runtime/host_build_graph/aicpu/aicpu_executor.cpp
Comment on lines
+229
to
+246
| // An out-of-range thread_idx — a broken affinity mapping, or run() invoked | ||
| // more than aicpu_thread_num_ times — cannot own the core slice or the | ||
| // per-index boot work, so it skips boot / classify / dispatch / shutdown. It | ||
| // is still one of the aicpu_thread_num_ run() invocations the finish barrier | ||
| // counts, and the boot barriers expect every participant to arrive. Publish | ||
| // boot_abort_ so peers stop spinning on classify_arrived_ / runtime_init_ready_ | ||
| // and skip dispatch — a run missing a core-owning thread can never reach | ||
| // is_completed() — then fall through to the shared finish barrier so finished_ | ||
| // still publishes and the host sees the failure instead of hanging into the | ||
| // op-execute timeout (507018). | ||
| const bool valid_idx = thread_idx >= 0 && thread_idx < aicpu_thread_num_ && thread_idx < MAX_AICPU_THREADS; | ||
| if (!valid_idx) { | ||
| LOG_ERROR( | ||
| "Thread index %d out of bounds (active=%d max=%d exec_idx=%d)", thread_idx, aicpu_thread_num_, | ||
| MAX_AICPU_THREADS, affinity_exec_idx | ||
| "Thread index %d out of bounds (active=%d max=%d exec_idx=%d) — aborting boot", thread_idx, | ||
| aicpu_thread_num_, MAX_AICPU_THREADS, affinity_exec_idx | ||
| ); | ||
| return -1; | ||
| boot_abort_.store(true, std::memory_order_release); | ||
| run_rc = -1; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Do not let an extra invocation satisfy the configured finish-barrier count.
For aicpu_thread_num_ = N, an (N+1)th call sets boot_abort_ but still reaches finished_count_.fetch_add(). If it finishes first, the Nth arrival destroys the runtime and triggers deinit() while one valid participant is still running. Track an admitted per-run cohort separately: only the first N participants may enter classify/finish counters; overflow calls should set the abort flag and return -1 without contributing to either barrier.
Also applies to: 334-340
🤖 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 `@src/a2a3/runtime/host_build_graph/aicpu/aicpu_executor.cpp` around lines 229
- 246, Update the run admission and shared finish-barrier logic around valid_idx
and finished_count_ so only the first aicpu_thread_num_ participants are
admitted to the per-run cohort and may contribute to classify or finish
counters. Overflow invocations must set boot_abort_, return -1, and bypass both
barriers, preventing them from satisfying the configured finish count or
triggering premature runtime destruction; preserve normal behavior for admitted
participants.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Make the
host_build_graphdevice-boot barriers fail fast instead of hanging when an AICPU thread gets an out-of-rangethread_idx.Follow-up to #1553 — addresses the boot-barrier deadlock flagged in review there (discussion).
Why
run()starts by validatingthread_idx(from the affinity mapping, or thethread_idx_++counter). On an out-of-range index it used toLOG_ERRORandreturn -1before reaching either boot barrier:classify_arrived_ < aicpu_thread_num_(the missing thread never arrives), andfinished_count_never reachesaicpu_thread_num_, sofinished_never publishes.Either way the whole op hangs into the op-execute timeout (507018) — the generic host code that masks the real cause, so it reads as a runtime hang rather than the misconfiguration it is.
The path is only reachable via a fatal platform misconfiguration (a broken affinity mapping, or
run()invoked more thanaicpu_thread_num_times). It cannot happen under correct configuration — every thread gets a distinct valid index. This change is purely about turning that hang into a legible, fast failure.How
One shared abort, closing both boot barriers (
classify_arrived_andfinished_count_):boot_abort_flag, records the error, and falls through.classify_ready_,classify_arrived_,runtime_init_ready_) also breaks onboot_abort_, so a missing participant can never leave the rest spinning.boot_abort_on every thread. A run missing a core-owning thread can never reachis_completed(), so merely un-hanging the boot barriers would just move the hang into the dispatch loop — the survivors must fail fast, not run a graph that can never complete.shutdown) but still reaches the shared finish barrier, sofinished_count_always reachesaicpu_thread_num_andfinished_publishes.-1on abort, so the host reads no partial success.boot_abort_is one-shot per run, reset indeinit()alongside the other boot flags. Boot/init path only — no dispatch-hot-path cost.Note on the review suggestion
The reviewer proposed "a shared abort state checked by every boot wait, and route participants through normal shutdown." This does exactly that, with one refinement worth calling out: an out-of-range thread is routed to the finish barrier (so
finished_still publishes) but not throughshutdown(thread_idx), because it owns no core slice keyed by that index — callingshutdownwith the bad index would be the same class of bug. Valid threads stillshutdownnormally.Scope
a2a3host_build_graphonly.Test
matmul,vector_example,predicated_dispatchpass on a2a3 (device-locked) — the normal (all-valid-index) boot path is unchanged. The abort path is a misconfiguration guard; it is not exercised by the functional suite, which never produces an out-of-range index.🤖 Generated with Claude Code