Refactor: make stream launch exact - #1700
Conversation
📝 WalkthroughWalkthroughThe PR replaces the enqueue/poll/drain API with prepared and active execution objects. It adds identity-bound launch permits and receipts, transactional AICore/AICPU submission, per-execution kernel arguments, explicit cleanup, and unit tests for launch and receipt validation. ChangesNative execution lifecycle
Estimated code review effort: 5 (Critical) | ~120 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 |
337c960 to
1183755
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/common/platform/onboard/host/c_api_shared.cpp (1)
609-623: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA failure from
abandon_prepared_executionis discarded.The catch block at line 613 sets
resources_rc = -1. Therunner_resources_ownedblock at line 618 then assignsresources_rcagain and overwrites that failure. Accumulate the first failure instead.♻️ Proposed fix
if (state->runner_resources_owned) { try { - resources_rc = state->runner->abandon_native_run_resources(state->descriptor.pipeline_slot); + int abandon_rc = state->runner->abandon_native_run_resources(state->descriptor.pipeline_slot); + if (resources_rc == 0) resources_rc = abandon_rc; } catch (...) { resources_rc = -1; }🤖 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/common/platform/onboard/host/c_api_shared.cpp` around lines 609 - 623, Preserve the failure recorded by abandon_prepared_execution in the surrounding cleanup flow: update the runner_resources_owned block to assign its abandon_native_run_resources failure only when resources_rc has not already failed, rather than unconditionally overwriting resources_rc.src/a2a3/platform/onboard/host/device_runner.cpp (1)
291-294: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the stale error message.
The message names
enqueue_run, which no longer exists, and states thatactivate_launch_shapemust run first.activate_launch_shapenow runs later, insidelaunch_runat line 587. The message misdirects triage.📝 Proposed message fix
if (block_dim < 1) { - LOG_ERROR("enqueue_run reached with unresolved block_dim; activate_launch_shape must run first"); + LOG_ERROR("prepare_execution computed block_dim < 1 from worker_count=%d", runtime.get_worker_count()); return -1; }🤖 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/platform/onboard/host/device_runner.cpp` around lines 291 - 294, Update the LOG_ERROR message in the block_dim validation guard to identify the current caller/context instead of the removed enqueue_run and avoid claiming activate_launch_shape must run first, since it executes later within launch_run. Keep the existing error condition and return behavior unchanged.
🧹 Nitpick comments (6)
src/common/platform/onboard/host/device_runner_base.h (1)
578-602: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the doc comments to the new method names.
The comments still name
poll_run(),drain_run(), andenqueue_run(). These methods no longer exist. The current names arepoll_execution,drain_execution, andprepare_execution/launch_execution.♻️ Proposed doc fix
/** * Prepare host-owned execution state and submit the Runtime to the device. * Success means the platform's real kernel-launch boundary was crossed; - * all state needed by poll_run() and drain_run() remains owned by the + * all state needed by poll_execution() and drain_execution() remains owned by the * runner until drain completes. */ @@ /** * Query the active run without waiting. Returns one of the * SIMPLER_NATIVE_RUN_POLL_* values. This may run concurrently with - * drain_run() on the compatibility executor. + * drain_execution() on the compatibility executor. */ @@ /** * Wait for the enqueued run, publish DFX, and release its execution - * resources. Called on the same executor thread as enqueue_run(). + * resources. Called on the same executor thread as launch_execution(). */🤖 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/common/platform/onboard/host/device_runner_base.h` around lines 578 - 602, Update the documentation comments around prepare_execution, poll_execution, and drain_execution to reference only the current method names: replace poll_run() with poll_execution(), drain_run() with drain_execution(), and enqueue_run() with the prepare_execution()/launch_execution() flow. Do not alter method signatures or behavior.src/a2a3/platform/sim/host/device_runner.cpp (1)
524-574: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSimulated launch paths run side effects outside the launch transaction. Both simulated runners publish platform bases, start collectors, and reset
run_completion_beforeexact_launch_transactionconsumes the permit. A rejected permit returnsNotStartedwhile the collectors already run and the completion counter already expects threads that are never created. The onboard a5 path performs this setup inside the AICore submission callback.
src/a2a3/platform/sim/host/device_runner.cpp#L524-L574: move the base publication, the collector starts, and therun_completion_.resetcall into the AICore submission callback.src/a5/platform/sim/host/device_runner.cpp#L450-L499: apply the same move for the base publication, the collector starts, and therun_completion_.resetcall.🤖 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/platform/sim/host/device_runner.cpp` around lines 524 - 574, Move platform-base publication, collector startup, and run_completion_.reset from the pre-transaction setup into the AICore submission callback in src/a2a3/platform/sim/host/device_runner.cpp lines 524-574, and apply the same change in src/a5/platform/sim/host/device_runner.cpp lines 450-499. Ensure these side effects occur only after exact_launch_transaction accepts the permit, while preserving their existing ordering relative to submission.src/a5/platform/onboard/host/device_runner.cpp (1)
147-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused local.
selected_pipeline_slotis assigned and never read.pipeline_slotis used directly.♻️ Proposed fix
- const uint32_t selected_pipeline_slot = pipeline_slot; auto prepare_rollback = RAIIScopeGuard([this, &execution]() {🤖 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/a5/platform/onboard/host/device_runner.cpp` at line 147, Remove the unused local selected_pipeline_slot in the device_runner flow and keep using pipeline_slot directly where the value is needed. Update the nearby logic in the same function to reference the existing pipeline_slot symbol so the assignment is eliminated without changing behavior.tests/ut/cpp/common/test_native_run_execution.cpp (2)
112-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that a consumed permit blocks a second submission.
The test proves that
firstis invalidated after the move at line 119. It does not prove the one-shot property that the name states: that an already-consumed permit refuses to submit.Add a second transaction with the consumed permit and confirm no submission occurs.
✅ Proposed additional assertion
EXPECT_EQ(success.progress, LaunchProgress::Complete); EXPECT_FALSE(first.valid()); + + int submissions = 0; + LaunchTransactionResult replay = exact_launch_transaction( + kIdentity, std::move(first), + [&]() { + ++submissions; + return 0; + }, + [&]() { + ++submissions; + return 0; + } + ); + EXPECT_EQ(replay.progress, LaunchProgress::NotStarted); + EXPECT_EQ(submissions, 0); + EXPECT_FALSE(replay.receipt.valid()); }🤖 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_native_run_execution.cpp` around lines 112 - 129, Extend NativeRunExecutionTest.PermitIsOneShot after the successful exact_launch_transaction call by submitting the already-consumed first permit again, then assert the second transaction is rejected and does not report LaunchProgress::Complete. Preserve the existing validity checks and use the test’s established transaction helper and result type.
29-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend identity-mismatch coverage to
dispatch_idandpipeline_slot.The tests exercise a mismatched
run_epochat line 31 and a stalegenerationat line 108.dispatch_idandpipeline_slotare never mismatched.If the permit check or
LaunchReceipt::matchescompared only a subset of the four fields, every test in this file would still pass. The whole prepared-execution contract rests on full identity binding, so pin all four fields.✅ Proposed parameterized mismatch coverage
TEST(NativeRunExecutionTest, EveryIdentityFieldIsBound) { const NativeRunIdentity mismatches[] = { {kIdentity.run_epoch + 1, kIdentity.generation, kIdentity.dispatch_id, kIdentity.pipeline_slot}, {kIdentity.run_epoch, kIdentity.generation + 1, kIdentity.dispatch_id, kIdentity.pipeline_slot}, {kIdentity.run_epoch, kIdentity.generation, kIdentity.dispatch_id + 1, kIdentity.pipeline_slot}, {kIdentity.run_epoch, kIdentity.generation, kIdentity.dispatch_id, kIdentity.pipeline_slot + 1}, }; for (const NativeRunIdentity &other : mismatches) { int submissions = 0; LaunchTransactionResult result = exact_launch_transaction( other, NativeRunExecutionTestPeer::mint(kIdentity), [&]() { ++submissions; return 0; }, [&]() { ++submissions; return 0; } ); EXPECT_EQ(result.progress, LaunchProgress::NotStarted); EXPECT_EQ(submissions, 0); EXPECT_FALSE(result.receipt.valid()); } }Also applies to: 105-109
🤖 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_native_run_execution.cpp` around lines 29 - 50, Extend NativeRunExecutionTest identity-mismatch coverage to validate every NativeRunIdentity field. Replace or augment the existing run_epoch and generation cases with a test such as EveryIdentityFieldIsBound that exercises mismatches in run_epoch, generation, dispatch_id, and pipeline_slot, asserting each transaction remains NotStarted, performs no submissions, and returns an invalid receipt.src/a2a3/platform/onboard/host/device_runner.h (1)
305-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoxygen blocks were not updated when
KernelArgsHelper &kernel_argswas threaded through theinit_*declarations. Both onboard headers gained the same parameter oninit_chip_swimlane,init_args_dump,init_pmu,init_dep_gen, andinit_scope_stats. No@param kernel_argsentry was added in either header, and several existing@paramentries name parameters that the signatures do not have.
src/a2a3/platform/onboard/host/device_runner.h#L305-L305: add@param kernel_argsto all fiveinit_*blocks, and remove the@param runtimeentry from theinit_chip_swimlaneblock, which takes noRuntime.src/a5/platform/onboard/host/device_runner.h#L250-L250: add@param kernel_argsto all fiveinit_*blocks, remove the@param runtimeentry from theinit_chip_swimlaneblock and the@param num_aicoreentry from theinit_args_dumpblock, and move the PMU block at lines 262-268 directly aboveinit_pmuat line 276 so Doxygen stops attaching it toenable_dep_gen_.🤖 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/platform/onboard/host/device_runner.h` at line 305, Update the Doxygen blocks for init_chip_swimlane, init_args_dump, init_pmu, init_dep_gen, and init_scope_stats in src/a2a3/platform/onboard/host/device_runner.h: add `@param` kernel_args to each and remove `@param` runtime from init_chip_swimlane. Apply the same updates in src/a5/platform/onboard/host/device_runner.h, also remove `@param` num_aicore from init_args_dump and move the PMU documentation block directly above init_pmu so it documents that declaration rather than enable_dep_gen_.
🤖 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/a5/platform/onboard/host/device_runner.cpp`:
- Around line 148-150: Update the rollback guard in device-runner execution
preparation so a prepared successor does not invoke the active run’s drain
cleanup. Ensure prepare_execution() failures leave shared collector state and
the predecessor’s RunPollState intact, while finalized collector teardown and
the Drained transition remain owned by the drain path used by poll_execution().
In `@src/common/platform/onboard/host/c_api_shared.cpp`:
- Around line 727-732: Move the assignment that clears
state->runner_resources_owned to after the rc != 0 failure check in the
prepare_execution flow. Keep the flag set when prepare_execution fails so
cleanup_failed_prepare can release resources, and transfer ownership only after
a successful preparation.
In `@src/common/platform/onboard/host/device_runner_base.cpp`:
- Around line 1285-1315: The reset-failure path in
DeviceRunnerBase::arm_device_wall_buffer currently frees device_wall_dev_ptr_
after it has been published through KernelArgsHelper, which can race with other
runs still reading it. Update this flow to avoid freeing the shared device-wall
buffer in place: either defer release until prior runs are complete or switch
the per-run setup to a separate buffer before clearing
kernel_args.args.device_wall_data_base. Keep the existing copy_to_device failure
handling and pointer reset behavior, but remove the immediate free of the shared
buffer from this method.
In `@src/common/worker/native_run_execution.h`:
- Around line 134-139: Update the exception handler in the Aicore submission
flow around AicoreSubmit so a thrown submission sets result.progress to Partial
while retaining the failure return code. Preserve the existing nonzero-result
handling, allowing callers to use the poison-and-retain path instead of
classifying the execution as NotStarted.
---
Outside diff comments:
In `@src/a2a3/platform/onboard/host/device_runner.cpp`:
- Around line 291-294: Update the LOG_ERROR message in the block_dim validation
guard to identify the current caller/context instead of the removed enqueue_run
and avoid claiming activate_launch_shape must run first, since it executes later
within launch_run. Keep the existing error condition and return behavior
unchanged.
In `@src/common/platform/onboard/host/c_api_shared.cpp`:
- Around line 609-623: Preserve the failure recorded by
abandon_prepared_execution in the surrounding cleanup flow: update the
runner_resources_owned block to assign its abandon_native_run_resources failure
only when resources_rc has not already failed, rather than unconditionally
overwriting resources_rc.
---
Nitpick comments:
In `@src/a2a3/platform/onboard/host/device_runner.h`:
- Line 305: Update the Doxygen blocks for init_chip_swimlane, init_args_dump,
init_pmu, init_dep_gen, and init_scope_stats in
src/a2a3/platform/onboard/host/device_runner.h: add `@param` kernel_args to each
and remove `@param` runtime from init_chip_swimlane. Apply the same updates in
src/a5/platform/onboard/host/device_runner.h, also remove `@param` num_aicore from
init_args_dump and move the PMU documentation block directly above init_pmu so
it documents that declaration rather than enable_dep_gen_.
In `@src/a2a3/platform/sim/host/device_runner.cpp`:
- Around line 524-574: Move platform-base publication, collector startup, and
run_completion_.reset from the pre-transaction setup into the AICore submission
callback in src/a2a3/platform/sim/host/device_runner.cpp lines 524-574, and
apply the same change in src/a5/platform/sim/host/device_runner.cpp lines
450-499. Ensure these side effects occur only after exact_launch_transaction
accepts the permit, while preserving their existing ordering relative to
submission.
In `@src/a5/platform/onboard/host/device_runner.cpp`:
- Line 147: Remove the unused local selected_pipeline_slot in the device_runner
flow and keep using pipeline_slot directly where the value is needed. Update the
nearby logic in the same function to reference the existing pipeline_slot symbol
so the assignment is eliminated without changing behavior.
In `@src/common/platform/onboard/host/device_runner_base.h`:
- Around line 578-602: Update the documentation comments around
prepare_execution, poll_execution, and drain_execution to reference only the
current method names: replace poll_run() with poll_execution(), drain_run() with
drain_execution(), and enqueue_run() with the
prepare_execution()/launch_execution() flow. Do not alter method signatures or
behavior.
In `@tests/ut/cpp/common/test_native_run_execution.cpp`:
- Around line 112-129: Extend NativeRunExecutionTest.PermitIsOneShot after the
successful exact_launch_transaction call by submitting the already-consumed
first permit again, then assert the second transaction is rejected and does not
report LaunchProgress::Complete. Preserve the existing validity checks and use
the test’s established transaction helper and result type.
- Around line 29-50: Extend NativeRunExecutionTest identity-mismatch coverage to
validate every NativeRunIdentity field. Replace or augment the existing
run_epoch and generation cases with a test such as EveryIdentityFieldIsBound
that exercises mismatches in run_epoch, generation, dispatch_id, and
pipeline_slot, asserting each transaction remains NotStarted, performs no
submissions, and returns an invalid receipt.
🪄 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: f9781c42-a7fc-483a-99f1-cf5fb968fc09
📒 Files selected for processing (22)
src/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/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/common/platform/onboard/host/c_api_shared.cppsrc/common/platform/onboard/host/device_runner_base.cppsrc/common/platform/onboard/host/device_runner_base.hsrc/common/platform/onboard/host/device_runner_helpers.hsrc/common/platform/sim/host/c_api_shared.cppsrc/common/platform/sim/host/device_runner_base.cppsrc/common/platform/sim/host/device_runner_base.hsrc/common/worker/native_run_context.hsrc/common/worker/native_run_execution.hsrc/common/worker/native_run_launch_signal.htests/ut/cpp/CMakeLists.txttests/ut/cpp/common/native_run_execution_test_peer.htests/ut/cpp/common/test_native_run_execution.cpptests/ut/cpp/common/test_native_run_launch_signal.cpp
7e5227a to
92006cf
Compare
Review — B6b exact launch seamReviewed The shape is right, and two changes in particular solve the real problem rather
The onboard launch transaction now contains only what the plan's list allows Must fix1. if (attach_rc == 0) { ... entered_run = true; launch_execution(std::move(state->prepared_execution), ...); }
else { rc = attach_rc; }
...
if (!entered_run && state->runner_resources_owned) { abandon_native_run_resources(...); }
Before this PR the same path was covered: Sim is worse: Suggest gating finalize on 2. A device_wall reset failure now kills the run — a2a3:588, a5:342 if (arm_device_wall_buffer(prepared.kernel_args) != 0) return -1; // → rc=-1, whole run failsThe old code treated this as explicitly non-fatal: warn, clear the base, keep
and (Only reachable under 3. Partial-launch poison is not wired on onboard
Onboard poisons today only incidentally: a non-zero AICPU launch rc makes the
4.
// Diagnostic binding reads runner-global collector configuration. It
// is depth-one, while concurrent HBG preparation must leave the active
// run's configuration untouched until launch.
if (!overlaps_active_run) runner->apply_call_config(state->config);Two lines later It does not misbehave today, because overlap requires 5. a2a3/a5 divergence plus a dead runner member This PR removes 6. Renamed methods still referenced in ~20 places (doc-consistency §1)
a2a3:292 / a5:186 is wrong twice over: LOG_ERROR("enqueue_run reached with unresolved block_dim; activate_launch_shape must run first");The function is no longer Should fix7. Relatedly, sim's 8. Also worth stating in the PR body: on a partial launch the old code returned from 9. Dead surface added.
10. The 11. 12. Signatures diverge from contracts.md — documented as 13. Test coverage does not reach B6b's headline claim. The six new unit tests
CIThe previous head's
SummaryThe direction is right and the two load-bearing changes (receipt-only acceptance, |
Materialize platform execution state during native prepare, consume an identity-bound launch permit at the two-submission boundary, and publish acceptance only from a completed launch receipt. Retain and poison uncertain partial launches, isolate prepared cleanup from active-run state, and release resources on every launch failure path. Keep device-wall capture failures non-fatal and place simulation side effects inside the exact launch transaction. Extend the main a2a3 scene session budget to match the expanded corpus while retaining the shorter limits for isolated smoke and SDMA steps.
92006cf to
e401177
Compare
|
@ChaoWao Addressed the B6b review:
The progress-aware bounded drain/deadline work remains in the following B6c PR. Consolidating the onboard/sim execution type definitions and the larger PlatformRunAdapter signature cleanup also remain separate so this PR stays on the exact-launch boundary. |
Re-review —
|
| # | Finding | Status |
|---|---|---|
| 1 | attach_current_thread failure leaked the PreparedExecution |
fixed — !entered_run now abandons it on both onboard and sim, and simpler_finalize_run gates on prepared_execution != nullptr instead of !launched |
| 2 | device-wall reset failure became fatal | fixed — (void)arm_device_wall_buffer(...), non-fatal again, matching the doc comment |
| 3 | partial-launch poison not wired on onboard | fixed — if (transaction.poisoned()) recover_device_or_mark_unusable(transaction.rc); on both arches, per-lambda recover removed for the AICPU step; plus the transaction now treats an AICore throw as Partial |
| 4 | apply_call_config in prepare_execution defeated the overlap guard |
fixed — removed from both prepare_executions; the c_api's guarded call is now the only one |
| 5 | a5 kept kernel_args_.args.scope_stats_data_base = 0 / dead kernel_args_ member |
fixed — line removed, KernelArgsHelper kernel_args_ deleted from DeviceRunnerBase |
| 6 | ~20 stale enqueue_run / drain_run / poll_run references |
fixed — repo-wide grep is clean, docs/dynamic-linking.md call-flow diagrams updated, and the two misleading LOG_ERRORs replaced with "prepare_execution computed block_dim < 1 from worker_count=%d" |
| 10 | implicit runner_resources_owned handoff |
fixed — cleared only after the rc check, and prepare_rollback now passes retire_aicore=false so the stream stays the c_api's until prepare succeeds. Clean split |
| 7 (partial) | sim side effects landed before the permit was validated | fixed — all the set_platform_*_func_ calls, collector starts, phase-base publication and run_completion_.reset moved inside the AICore closure |
Also: st-onboard-a2a3 is green at 8m36s on the new head, which supports the
earlier read that the 600 s trip was budget-margin rather than a B6b slowdown.
New — blocking
finalize_collectors() is now skipped on every non-launched cleanup path.
void DeviceRunner::cleanup_execution(PreparedExecution &prepared, bool launched, bool retire_aicore) noexcept {
if (!prepared.resources_owned) return;
if (launched) finalize_collectors(); // ← a2a3:501, a5 equivalentabandon_prepared_execution() passes launched=false, and so does
prepare_rollback. But the collectors are initialized during prepare
(init_chip_swimlane / init_args_dump / init_pmu / init_dep_gen /
init_scope_stats, each under its enable_*_ flag), and on a2a3
start_shared_collectors_for_run() + dep_gen_collector_.start() run inside the
AICore closure, before launch_aicore_kernel. So three reachable paths now leak:
- prepare fails after some
init_*succeeded → rollback skips teardown → device
shm + host registration leak, andis_initialized()stays true so the next run's
init_*hits an already-initialized collector. - prepared but never launched (finalize without launch, or the new
!entered_runabandon after anattach_current_threadfailure) → same. - launch returns
NotStartedafter the closure already started the collectors —
mark_submittedfailure, orlaunch_aicore_kernelfailure (i.e. the 207001 path
this file has a whole recovery routine for). The mgmt/poll threads are already
running and nothing stops them.
Before this revision all three called finalize_collectors().
The stated rationale — "Prepared-only cleanup must not touch collector state owned
by an active predecessor" — does not hold, because an overlapping successor can
only exist when neither run has diagnostics:
allow_prepared_successor = concurrent_native_prepare_supported_impl() && !config->diagnostics_any()(the successor's own config), andtry_reserve_native_runadditionally requiresexisting->permits_prepared_successor, which is the predecessor'sallow_prepared_successor.
So whenever an overlap is in flight, no collector is initialized on either side, and
finalize_collectors() — whose every branch is guarded by is_initialized() — is
already a no-op. The gate cannot prevent a cross-run teardown that can't happen; it
only suppresses this run's own teardown.
Suggest dropping the launched parameter and restoring the unconditional call. If a
real overlap hazard does turn up later, the correct predicate is "an active
predecessor exists" (native_run_active()), not "this run launched". (The a5
run_poll_state_ = Drained gate is a different question and is correctly
launched-conditioned — keep that one.)
New — should fix
A pre-submission throw now poisons the device.
} catch (...) {
result.rc = -1;
// Submission callbacks may throw after starting work. Without a
// receipt, the caller must retain resources and poison admission.
result.progress = LaunchProgress::Partial;
return result;
}The intent is right for the AICPU step and for the actual submission call. But the
AICore closure is not only a submission — on a2a3 it also runs
activate_launch_shape, arm_device_wall_buffer, start_shared_collectors_for_run()
(thread creation), dep_gen_collector_.start() (thread creation), and
std::vector<CoreType> core_types(num_aicore); sim's closure is larger still. A
std::bad_alloc from that vector or a std::system_error from create_thread is
provably before the first stream submission, yet it now lands as Partial →
recover_device_or_mark_unusable() → device_unusable_ → force-reset at finalize
(and poison_launch() on sim).
contracts.md is explicit: "Failure before the first stream submission is
FAILED_SAFE." Cheapest fix that keeps the conservative default where it belongs:
wrap the arming prologue inside the closure in its own try { ... } catch (...) { return -1; } so only a throw from launch_aicore_kernel (and the sim thread spawns
that stand in for it) maps to Partial.
Still open from round one (non-blocking, your call)
- 8 —
drain_executionnever readsactive.progress; its RAII guard retires
resources even whenreap_runtimed out, i.e. without quiescence proven. - 9 — dead surface:
PreparedExecution's move ctor (and theKernelArgsHelper
move ctor added for it) is never invoked;PreparedExecution::configstill has no
reader in any of the four backends; sim'sPreparedExecutionstill has a
virtual ~with no derived class while onboard's does not;class NativeRunLaunchSignal;is still a dead forward declaration in both base headers. - 7 (rest) — sim's
poll_execution/drain_execution/
abandon_prepared_executionstill ignore their arguments and drive the runner's
active_run_, soPreparedExecutionremains a token rather than an owner there. - 11 —
PreparedExecution/ActiveExecution/LaunchOutcomestill defined
twice undersrc/common/. - 12 — signatures still diverge from
contracts.md; no B6b entry in
implementation-record.mdrecording the deviation. - 13 — the two new transaction tests (AICore throw, AICPU throw) are good
additions, but the plan's partial-launch poison evidence still stops at
result.poisoned(); nothing asserts the runner'scan_accept_run()goes false, and
sim's newcan_accept_run()gate insimpler_prepare_runremains untested. The
stage's headline claim — material preparation overlapping the predecessor, with only
the bounded transaction serialized — still has no measurement.
Summary
Six of six must-fixes from round one are closed, and the ownership split around
retire_aicore came out cleaner than what I proposed. The collector-teardown gate is
the one thing I'd hold the merge on — it turns a hazard that cannot occur into three
leaks that can, including on the AICore-launch-failure path. The throw→Partial
widening is worth narrowing in the same pass since it is two lines.
2 participants
Summary
CI budget
The a2a3 full-scene lane had reached 587 seconds on #1695 with a 600-second session limit. The earlier #1700 run reported every case passed at 100% and then exited 124 at the same session wall. The main a2a3 sweep now uses 1200 seconds, matching the a5 sweep; isolated smoke and SDMA limits remain unchanged.
Validation
Simulation scene execution was intentionally omitted; simulation targets were compile-validated.