Refactor: split device enqueue from completion drain - #1683
Merged
ChaoWao merged 1 commit intoAug 4, 2026
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 runner API now separates execution into ChangesExecution lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (1)
tests/ut/cpp/common/test_sim_run_completion.cpp (1)
40-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert sticky terminal polling.
After the final task completes, call
completion.poll()a second time. The current tests verify the first terminal result only. They do not verify the sticky result.Proposed test coverage
task.join(); EXPECT_EQ(completion.poll(), SIMPLER_NATIVE_RUN_POLL_COMPLETE); + EXPECT_EQ(completion.poll(), SIMPLER_NATIVE_RUN_POLL_COMPLETE); EXPECT_EQ(completion.first_error(), 0); @@ completion.task_finished(-9); EXPECT_EQ(completion.poll(), SIMPLER_NATIVE_RUN_POLL_COMPLETE); + EXPECT_EQ(completion.poll(), SIMPLER_NATIVE_RUN_POLL_COMPLETE); EXPECT_EQ(completion.first_error(), -7);Also applies to: 53-53
🤖 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/common/test_sim_run_completion.cpp` at line 40, Extend the completion assertions in the test around completion.poll() to invoke it a second time after the final task completes, and assert that the repeated call also returns SIMPLER_NATIVE_RUN_POLL_COMPLETE, covering sticky terminal polling at both referenced test cases.
🤖 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/platform/sim/host/device_runner.cpp`:
- Around line 296-307: The enqueue_run method arms run_completion_.reset(1)
before the final task count is known, and if initialization fails,
cleanup_active_run() will null out active_run_ while poll_run() may still return
SIMPLER_NATIVE_RUN_POLL_NOT_READY, creating a state mismatch. Fix this in both
src/a2a3/platform/sim/host/device_runner.cpp (lines 296-307) and
src/a5/platform/sim/host/device_runner.cpp (lines 251-262) by either making
cleanup_active_run() publish a terminal completion state to run_completion_, or
making poll_run() check if active_run_ is nullptr and return a terminal error
code instead of NOT_READY. Apply the same solution to both files to keep them
consistent.
In `@src/a2a3/runtime/tensormap_and_ringbuffer/host/runtime_maker.cpp`:
- Line 969: Update the execution_rc parameter documentation to describe both
outcomes: it contains the drain result after a successful enqueue_run(), or the
enqueue_run() error when enqueueing fails and drain_run() is not called. Apply
this wording at the corresponding sites in
src/a2a3/runtime/tensormap_and_ringbuffer/host/runtime_maker.cpp (969-969),
src/a2a3/runtime/host_build_graph/host/runtime_maker.cpp (912-912),
src/a5/runtime/host_build_graph/host/runtime_maker.cpp (912-912), and
src/a5/runtime/tensormap_and_ringbuffer/host/runtime_maker.cpp (952-952),
preserving byte-for-byte parity between the corresponding a2a3 and a5
host_build_graph files.
In `@src/a5/platform/onboard/host/device_runner.cpp`:
- Around line 377-380: Move the run_poll_state_ store to RunPollState::Submitted
until after launch_aicpu_kernel(stream_aicpu_, ...) returns successfully,
ensuring poll_run cannot query streams before the current AICPU run is queued.
Preserve the existing ownership publication and launch-failure behavior, or
introduce a launched state only if poll_run uses it to gate DeviceComplete.
---
Nitpick comments:
In `@tests/ut/cpp/common/test_sim_run_completion.cpp`:
- Line 40: Extend the completion assertions in the test around completion.poll()
to invoke it a second time after the final task completes, and assert that the
repeated call also returns SIMPLER_NATIVE_RUN_POLL_COMPLETE, covering sticky
terminal polling at both referenced test cases.
🪄 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: da4372c6-0973-4e63-b087-a80f76c92c21
📒 Files selected for processing (35)
docs/chip-level-arch.mddocs/dfx/dep-gen.mddocs/dfx/host-trace.mddocs/dynamic-linking.mdsrc/a2a3/platform/docs/tpush-tpop-sim.mdsrc/a2a3/platform/onboard/host/device_runner.cppsrc/a2a3/platform/onboard/host/device_runner.hsrc/a2a3/platform/sim/host/device_runner.cppsrc/a2a3/platform/sim/host/device_runner.hsrc/a2a3/runtime/host_build_graph/host/runtime_maker.cppsrc/a2a3/runtime/host_build_graph/runtime/dep_gen_host_graph.hsrc/a2a3/runtime/tensormap_and_ringbuffer/host/runtime_maker.cppsrc/a5/platform/docs/tpush-tpop-sim.mdsrc/a5/platform/onboard/host/device_runner.cppsrc/a5/platform/onboard/host/device_runner.hsrc/a5/platform/sim/host/device_runner.cppsrc/a5/platform/sim/host/device_runner.hsrc/a5/runtime/host_build_graph/host/runtime_maker.cppsrc/a5/runtime/host_build_graph/runtime/dep_gen_host_graph.hsrc/a5/runtime/tensormap_and_ringbuffer/host/runtime_maker.cppsrc/common/platform/include/host/run_stream_slots.hsrc/common/platform/onboard/host/c_api_shared.cppsrc/common/platform/onboard/host/device_runner_base.hsrc/common/platform/onboard/host/device_runner_helpers.cppsrc/common/platform/onboard/host/device_runner_helpers.hsrc/common/platform/shared/host/args_dump_collector.cppsrc/common/platform/shared/host/l2_swimlane_collector.cppsrc/common/platform/sim/host/c_api_shared.cppsrc/common/platform/sim/host/device_runner_base.hsrc/common/platform/sim/host/sim_run_completion.hsrc/common/worker/native_run_state.hsrc/common/worker/pto_runtime_c_api.htests/ut/cpp/CMakeLists.txttests/ut/cpp/common/test_sim_run_completion.cpptests/ut/cpp/hierarchical/test_run_stream_slots.cpp
ChaoWao
force-pushed
the
worker-async-b6a-enqueue-drain
branch
from
August 4, 2026 11:40
70160a5 to
81b9b70
Compare
Collaborator
Author
|
@coderabbitai Addressed the review summary:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Changed files and summary =='
git diff --stat HEAD^ HEAD
git diff --name-only HEAD^ HEAD | sed -n '1,240p'
echo '== Focused outlines =='
for f in \
src/common/platform/sim/host/sim_run_completion.h \
tests/ut/cpp/common/test_sim_run_completion.cpp \
src/a5/platform/onboard/host/device_runner.cpp \
src/a5/platform/onboard/host/device_runner.h \
src/common/platform/onboard/host/c_api_shared.cpp \
src/common/platform/sim/host/c_api_shared.cpp \
src/a2a3/runtime/host_build_graph/host/runtime_maker.cpp \
src/a2a3/runtime/tensormap_and_ringbuffer/host/runtime_maker.cpp \
src/a5/runtime/host_build_graph/host/runtime_maker.cpp \
src/a5/runtime/tensormap_and_ringbuffer/host/runtime_maker.cpp
do
if [ -f "$f" ]; then
echo "---- $f ----"
ast-grep outline "$f" --items all 2>/dev/null || true
fi
done
echo '== Completion, rollback, and A5 lifecycle references =='
rg -n -C 5 -i \
'sticky|poll_run|simpler_poll_run|enqueue.*rollback|rollback.*enqueue|run_completion_|submitted|complete|completion|drain_run|rtStreamQuery' \
tests/ut/cpp/common/test_sim_run_completion.cpp \
src/a2a3/platform/sim/host/device_runner.cpp \
src/a5/platform/sim/host/device_runner.cpp \
src/a5/platform/onboard/host/device_runner.cpp \
src/a5/platform/onboard/host/device_runner.h \
src/common/platform/onboard/host/c_api_shared.cpp \
src/common/platform/sim/host/c_api_shared.cpp \
src/common/platform/sim/host/sim_run_completion.h \
2>/dev/null || true
echo '== execution_rc documentation =='
rg -n -C 3 'execution_rc' \
src/a2a3/runtime/host_build_graph/host/runtime_maker.cpp \
src/a2a3/runtime/tensormap_and_ringbuffer/host/runtime_maker.cpp \
src/a5/runtime/host_build_graph/host/runtime_maker.cpp \
src/a5/runtime/tensormap_and_ringbuffer/host/runtime_maker.cppLength of output: 97023 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== A5 enqueue launch ordering =='
sed -n '360,410p' src/a5/platform/onboard/host/device_runner.cpp
echo '== Onboard launch and polling gate =='
sed -n '739,854p' src/common/platform/onboard/host/c_api_shared.cpp
echo '== Launch marker implementation and call sites =='
rg -n -C 6 'publish_task_accepted|launch_signal|launch_aicpu_kernel\s*\(' \
src/common/platform/onboard/host \
src/a5/platform/onboard/host \
src/common/worker \
2>/dev/null || true
echo '== Native run phase and launch-signal definitions =='
rg -n -C 6 'enum class NativeRunPhase|NativeRunPhase|launch_signal' \
src/common/worker/native_run_state.h \
src/common/platform/onboard/host/c_api_shared.cpp \
2>/dev/null || trueLength of output: 44920
✏️ Learnings added
You are interacting with an AI system. |
ChaoWao
force-pushed
the
worker-async-b6a-enqueue-drain
branch
from
August 4, 2026 12:15
81b9b70 to
1309709
Compare
Collaborator
Author
|
CI triage for the remaining red
No B6a code change is justified by this failure. |
- Retain the compatibility executor while exposing real nonblocking completion queries in every backend. - Keep run-owned threads, stream handles, diagnostics, and cleanup alive until the explicit drain phase. - Serialize a2a3 query and retirement, and publish completion only after a proven device fence. - Publish a sticky simulation error when enqueue rolls back, preventing late task completion from masking the failure.
ChaoWao
force-pushed
the
worker-async-b6a-enqueue-drain
branch
from
August 4, 2026 14:37
1309709 to
9585a67
Compare
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.
Summary
DeviceRunnerexecution into enqueue, nonblocking poll, and drain across a2a3/a5 onboard and simulation backends.Testing
rtStreamQuery/drain path)