Category
Technical Debt (cleanup, refactor)
Component
Host Runtime
Description
launched in simpler_finalize_run was silently redefined from phase != NativeRunPhase::Prepared to state->active_execution != nullptr by PR #1694 ("Refactor: run native progress on the child loop", 4f417732). Neither PR #1694's nor PR #1725's ("Attach the device before drain and retire the dead dep_gen capture API", 0d202898) commit message mentions launched, Prepared, or active_execution — verified by reading both commit messages in full.
The new definition looks correct: a NotStarted failure never touched a stream, so it should take the -1 / set_gm_sm_ptr(nullptr) path rather than being treated as launched. This isn't a report of a functional bug — it's that the invariant behind launched has never been stated anywhere (no code comment, no commit message), so a future regression on this path has no explanation to git bisect back to, and the next person to touch this function has to re-derive the invariant from scratch.
Originally surfaced during PR #1694 review as one of four non-blocking follow-ups; PR #1725 closed the other three (dep_gen dead API removal, two restored tests, catch (...) {} diagnostics) but did not touch this one.
Location
src/common/platform/onboard/host/c_api_shared.cpp:856 (simpler_finalize_run)src/common/platform/sim/host/c_api_shared.cpp:762 (same function, sim variant)
Proposed Fix
Add a comment at the launched definition stating the present-tense invariant it encodes — e.g. that launched means "the native run reached the point of owning an active_execution and therefore needs drain_execution", as opposed to the pre-#1694 phase-based definition. No behavior change is being requested; this is purely recording the existing invariant so it survives the next refactor.
Priority
Low (no impact today, good to fix eventually)
Category
Technical Debt (cleanup, refactor)
Component
Host Runtime
Description
launchedinsimpler_finalize_runwas silently redefined fromphase != NativeRunPhase::Preparedtostate->active_execution != nullptrby PR #1694 ("Refactor: run native progress on the child loop",4f417732). Neither PR #1694's nor PR #1725's ("Attach the device before drain and retire the dead dep_gen capture API",0d202898) commit message mentionslaunched,Prepared, oractive_execution— verified by reading both commit messages in full.The new definition looks correct: a
NotStartedfailure never touched a stream, so it should take the-1/set_gm_sm_ptr(nullptr)path rather than being treated as launched. This isn't a report of a functional bug — it's that the invariant behindlaunchedhas never been stated anywhere (no code comment, no commit message), so a future regression on this path has no explanation togit bisectback to, and the next person to touch this function has to re-derive the invariant from scratch.Originally surfaced during PR #1694 review as one of four non-blocking follow-ups; PR #1725 closed the other three (dep_gen dead API removal, two restored tests,
catch (...) {}diagnostics) but did not touch this one.Location
src/common/platform/onboard/host/c_api_shared.cpp:856(simpler_finalize_run)src/common/platform/sim/host/c_api_shared.cpp:762(same function, sim variant)Proposed Fix
Add a comment at the
launcheddefinition stating the present-tense invariant it encodes — e.g. thatlaunchedmeans "the native run reached the point of owning anactive_executionand therefore needsdrain_execution", as opposed to the pre-#1694 phase-based definition. No behavior change is being requested; this is purely recording the existing invariant so it survives the next refactor.Priority
Low (no impact today, good to fix eventually)