fix(serve): fail the Windows managed launch when the engine dies at startup

[rominf]

Symptom

On Windows, rocm serve reported success — exit code 0 and a deployment summary
showing readiness: starting — when the managed engine process had already died
during startup. The user saw a "launched, still warming up" message for a server
that would never come up, and nothing pointed at the reason.

The identical situation on Linux fails the command with a clear error that
includes a tail of the engine's own service log.

Root cause

spawn_managed_engine_child in apps/rocm/src/main.rs spawns the engine
differently per platform:

• the cfg(not(windows)) branch gets a std::process::Child, sleeps 200 ms and
  calls try_wait(). A child that has already exited fails the launch via
  managed_engine_startup_failure_detail, which tails up to 80 lines of the
  child's log into the error.
• the cfg(windows) branch calls rocm_core::spawn_detached_no_inherit, which
  returns a bare PID. There is no Child, and no liveness or exit check of
  any kind.

After the spawn the parent waits up to 45 s for HTTP readiness, records the
status and returns Ok(..) regardless, so on Windows a dead engine was
indistinguishable from a slow one. This is long-standing rather than a
regression — the Windows spawn line dates to the initial import.

Fix

crates/rocm-core/src/lib.rs gains spawn_detached_no_inherit_watching_startup
next to the existing helper. It keeps the creation flags and handle-inheritance
behaviour byte-for-byte (DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW | CREATE_UNICODE_ENVIRONMENT, no inherited handles), so the
child is still fully detached and still outlives the CLI. What it adds is a
bounded WaitForSingleObject plus GetExitCodeProcess while the process
handle returned by CreateProcessW is still open, before that handle is
closed.

Doing the check there, rather than re-opening the process by PID afterwards, is
deliberate: a later OpenProcess(pid) would carry a PID-reuse race, because
Windows recycles PIDs and the PID could by then name an unrelated process. The
Unix side does not have that problem — it holds a Child — and this keeps
Windows equally race-free. GetExitCodeProcess is only consulted after the wait
returns WAIT_OBJECT_0, so the STILL_ACTIVE (259) ambiguity cannot arise; if
the exit code still cannot be read the helper reports "still running", so the
check can only ever degrade to today's behaviour, never invent a failure.

Both platforms now use one shared 200 ms budget
(MANAGED_ENGINE_STARTUP_SETTLE) and produce the same
managed_engine_startup_failure_detail error, so the child's log reaches the
user either way.

What this does and does not close. The check is a bounded window, not a
guarantee. An engine that survives the 200 ms settle and dies afterwards is
still reported as a successful launch, because start_managed_service records
the readiness status and returns Ok regardless — so on a loaded machine a
normally-fast death can slip past. That limitation is pre-existing on Unix and
unchanged by this PR; what the PR does is give Windows the same window Unix
already had, so the two platforms fail identically. Closing the wider class
(reporting success for an engine that never becomes ready) is a separate change
and is not attempted here.

The blind #[cfg(windows)] thread::sleep(Duration::from_millis(200)) that
followed each Windows spawn is removed: the checked wait is the same duration at
the same point, so the wall-clock delay before the readiness poll is unchanged.
It does now fall inside the launch lock rather than just after it — which is
exactly where Unix already paid it, so this aligns the two rather than adding
wall-clock cost to a single launch. It is not free for concurrent ones,
though: a Windows launch now holds the cross-process launch lock ~200 ms longer,
so a second rocm serve racing for the same lock waits that much longer to
start. Bounded and deadlock-free, and small next to the multi-second readiness
wait the lock already guards, but worth stating rather than leaving implied.

Second defect this exposed

Making the launch fail surfaced a latent problem that affected both
platforms. The service record is written before the spawn, to claim the GPU
for concurrent auto-selection, and at that point it still carries the
constructor's "starting" status and a supervisor_pid of 0. Bailing out and
leaving it that way wedges the service permanently:

• recorded_service_pids filters out pid 0, so refresh_managed_service_runtime_liveness finds no tracked pid and never demotes the record;
• managed_service_is_live counts "starting" as alive;
• so existing_live_managed_service keeps returning the corpse, and the idempotency guard makes every later rocm serve for that engine + model report AlreadyRunning instead of relaunching.

On Windows this was previously masked — the code never bailed there, so the
record got a non-zero (dead) pid and the liveness refresh self-healed it to
"stopped". Both arms now retire the record as "failed" before bailing, which
is what restart_internal_managed_service already did.

The spawn_detached_no_inherit call sites

Call site	Verdict
spawn_managed_engine_child — managed launch, also used by the attached --verbose/--foreground path (apps/rocm/src/main.rs)	Had the gap. Fixed. Windows had no check at all where Unix bails with the log tail.
restart_internal_managed_service — managed restart (apps/rocm/src/main.rs)	Had the gap. Fixed. The try_wait() sitting a few lines below is inside the cfg(not(windows)) branch, so Windows restarts silently "succeeded" too.
ensure_background_helper_running_quiet — automation daemon autostart (apps/rocm/src/main.rs)	No gap — left alone. Neither platform checks liveness, and by design the spawn result is logged rather than propagated, so the branches are already symmetric. Adding a check would be a behaviour change, not a fix.
spawn_serve_http_background — Lemonade serve-http background spawn (engines/lemonade/src/lib.rs)	No gap — left alone. Symmetric for the same reason: the Unix branch drops its Child immediately without a try_wait().

spawn_hidden_console_no_inherit was checked for the same shape and has no
callers outside rocm-core, so there is nothing to fix. Its sibling
spawn_hidden_console_with_log is used by the Lemonade adapter, but that is a
different lifecycle — the caller keeps and polls the result — and is out of
scope here.

Tests

In crates/rocm-core/src/lib.rs, #[cfg(windows)], running on the required
windows-build-and-test lane:

• watched_detached_spawn_reports_a_child_that_exits_immediately — spawns a
  process that exits with code 7 and asserts the spawn reports it.
• watched_detached_spawn_does_not_report_a_child_that_keeps_running — spawns a
  long-lived process and asserts it is not reported as a startup failure, so
  the check cannot reject healthy launches.

In crates/rocm-core/src/lib.rs, running everywhere:

• wait_timeout_millis_keeps_the_startup_wait_bounded — INFINITE is
  u32::MAX, so a saturating conversion of a long budget would silently turn the
  bounded check into a blocking join. This pins the clamp one below it.

In apps/rocm/src/main.rs, running everywhere:

• a_dead_engine_fails_the_launch_and_frees_the_engine_for_the_next_serve —
  the user-visible fix end to end at the unit level: an engine that already
  exited must fail the launch, name its log, and release the engine + model.
• a_live_engine_leaves_the_launch_running — the converse, so the check cannot
  reject a healthy launch or retire its record.
• a_spawn_that_never_started_frees_the_engine_for_the_next_serve — covers the
  spawn-failure path described below, and asserts the spawn's own error is what
  survives.
• a_failed_launch_record_stops_blocking_the_next_serve — regression test for
  the record-wedge above. Its precondition assertion demonstrates that a pid-0
  "starting" record really does read as live, then shows that retiring it as
  "failed" frees the engine + model for the next launch.
• managed_engine_startup_failure_detail_carries_the_child_log_tail and
  managed_engine_startup_failure_detail_without_a_log_still_points_at_it —
  cover the error both platforms now build, including the empty-log case. Note
  these are new coverage on Windows only: the function was cfg-gated away
  there before this PR, so on Linux they characterise unchanged code and pass
  with the rest of the fix reverted.

In apps/rocm/src/main.rs, #[cfg(windows)]:

• managed_startup_exit_reads_the_watched_exit_code — the one remaining
  cfg-gated step, converting the watched spawn's u32 into an ExitStatus.

Why the wiring is tested, not just the two halves

The first review round made the fair point that detection and record retirement
were each proven in isolation while the branch joining them — the thing that
actually turns a dead engine into a failed rocm serve — was reachable from
neither side, so deleting it left every runnable test green. It was cfg-gated
to Windows in two places, which put it out of reach of any Linux test and of
every test that does run on the Windows lane.

Rather than leave that to an end-to-end test, the four near-identical copies of
the check (two Windows, two Unix) are now one function,
fail_managed_launch_if_engine_died, which takes an Option<ExitStatus> and is
therefore platform-neutral and directly testable on every lane. Only the
u32 → ExitStatus conversion stays cfg-gated, and it has its own test.

Mutation-checked, each failing only its own test:

Mutation	Result
Neuter the bail in fail_managed_launch_if_engine_died (the review's mutation)	a_dead_engine_fails_the_launch_and_frees_the_engine_for_the_next_serve fails
Drop the retirement in retire_record_on_error	a_spawn_that_never_started_frees_the_engine_for_the_next_serve fails

That statement was corrected in review. An earlier revision of this section
claimed the residual gap was Windows-only. It was not: deleting both Unix call
sites — the two that execute on Linux — also left all 765 binary tests green. The
helper was pinned; the wiring to it was unpinned on every platform. Scenario
serve-23 below closes that for the launch path, and is verified to fail when a call
site is deleted.

To be explicit about what the unit tests do and do not prove: none of the four
real call sites of the new helpers — two on the launch path, two on the restart
path — is reached by any unit test, on any platform. The record and retirement
tests pin the helpers themselves, and the two managed_engine_startup_failure_detail_*
tests pin a renderer this PR only un-gates for Windows. The launch-path wiring
rests on serve-23; the restart-path wiring is not exercised end to end (see
Scenario coverage).

A spawn that never starts wedges the same record

Review also caught that this PR's own wedge fix had a hole beside it: if the
spawn fails outright (the exe cannot be launched at all), the ? returned
straight past the retirement, stranding the pre-spawn record with its
"starting" status and pid 0 — the exact corpse the early-exit path was fixed
to avoid.

The second review round found the hole was wider: six further fallible steps
sit between record.write() and the spawn (create_dir_all, File::create,
managed_service_launcher_path, builtin_engine_serve_http_args,
env_root_for_service, ensure_public_service_has_endpoint_key), each stranding
the identical record. Every one now goes through retire_record_on_error, which
replaces spawn_or_retire_record and takes an already-evaluated Result so it
can wrap the whole straight-line prefix without holding a second borrow of the
record.

It drops the retirement error rather than propagating it, and
fail_managed_launch_if_engine_died now does the same — previously it used ?
there, so a failed record write would have replaced the engine's exit status and
log tail with a bookkeeping error, which is the opposite of what diagnoses the
launch.

The mark_managed_launch_failed doc comment is also scoped to the launch path,
since the restart callers hold a pre-existing record whose pid is merely stale
rather than 0 — the behaviour was right at both, only the narrative was
launch-specific.

Scenario coverage

Per AGENTS.md §3 this alters what a user can observe — the exit code and error
text of rocm serve — so it carries a scenario rather than only unit tests.

@id:serve-managed-engine-dies-at-startup (serve-23; it was serve-22 until main
added its own serve-22 and this one was renumbered on rebase) in
tests/e2e-cucumber/features/model_serving.feature: the engine dies during
startup, the serve fails naming the engine's own log, and the failed launch does
not block the next attempt. It carries no host tag, so it runs on every
lane — including Strix Halo Windows, which is the only place the Windows half of
the startup check executes at all, since no unit test can reach those call sites
from Linux.

An earlier revision of this PR tagged it @requires-no-gpu and claimed on that
basis that it covered Windows. That was wrong: @requires-no-gpu is skipped on
any host that has a GPU, so the scenario ran only on the Linux mock lane and
the Windows call sites stayed unexercised. The tag was also wrong on its own
terms — the premise is a dead engine, not an absent GPU. The test hook's waivers
are what make the premise hold on any host, so no host tag is needed.

It pins the wiring, verified by mutation: deleting the Unix launch call site
turns the scenario red, where the same deletion leaves all 765 binary tests green.

Its final assertion reads the log_path off the service record the failed launch
leaves behind and requires the output to name that exact path, rather than
matching a .log suffix. Checked by mutation: a build that names some other
.log file passed the old suffix check and fails this one.

Not covered end to end: the restart path. restart_internal_managed_service
gets the same startup check, but the scripted-death switch exists only on the
launch path, and restart is reachable only through the assistant's
restart_server sandbox tool. Covering it would mean adding another consumer of
e2e-test-hooks while #349 is working to retire that feature, so it is stated
here as a gap instead. Restart shares retire_record_on_error and
fail_managed_launch_if_engine_died with the launch path; only its two call
sites are unexercised.

How the death is scripted, since the child is detached and cannot simply be told
to fail: an e2e-test-hooks switch read in the parent swaps the child's
arguments for ones the CLI rejects outright. The child is the real rocm binary,
really spawned and really dead — only the trigger is scripted, so the launch takes
the same path a genuinely broken engine takes. Reading the switch in the parent
keeps all test-only behaviour out of the child. It is an unrecognised flag and
not an unrecognised subcommand on purpose: the CLI parses unknown subcommands as
natural-language requests and exits 0, which is a slower and different thing from
an engine dying.

Like the existing Lemonade seam it waives the no-GPU pre-flight, and it waives
engine preparation too, so the scenario reaches a real spawn on a lane with
neither GPU hardware nor an installed runtime. Both waivers are behind
e2e-test-hooks, which release builds do not enable.

Confirmed executing on Windows, read from the lane's own log rather than
inferred from the lane's colour: at ef67aa7d, E2E tests (Strix Halo, Windows)
ran the scenario (then serve-22) and passed all four steps, as did WSL2; and again
at 72b0b5e3, after the rebase, as serve-23.

One asymmetry worth knowing: the scripted path replaces the whole argument
vector, so the child never receives --log. On Unix the parent redirects the
child's stdio into the service log, so the failure exercises the log-tail
branch of the detail renderer; on Windows the detached spawn does not redirect,
so the log is empty and the "no tail, still name the path" branch runs. The
scenario's assertion — that the exact service log path is named — holds on both
branches, but the two platforms do not prove the same branch. Asserting the tail on Windows would mean making the detached spawn
redirect stdio, which is a behaviour change out of scope here.

serve-18 (@id:serve-lemonade-preparation-recovery) remains unrelated to this
path: it injects inside the parent's Install RPC and aborts before any spawn.
#351 is likewise independent — this PR neither depends on it nor touches it.

Searched tests/e2e-cucumber/expectations.toml: no xfail row relates to this
behaviour — the existing serve-* rows are Linux/Lemonade shutdown issues.

Verified vs not verified

Run locally on Linux, all clean:

• cargo check --workspace --all-targets, and cargo check -p e2e-cucumber --test e2e separately (that target sets test = false)
• cargo clippy --locked --workspace --all-targets -- -D warnings and cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings, with both sources touched first so nothing came from the clippy cache
• cargo fmt --all --check
• cargo test -p rocm -p rocm-core --all-targets, plus the rest of the workspace
• cargo xtask manifest --check, cargo xtask check-crate-edges, python3 scripts/smoke_local.py

Not verified locally — there is no Windows host here: the runtime behaviour
of the new Win32 path. The two #[cfg(windows)] tests have never executed; the
required windows-build-and-test lane (which runs cargo test --workspace --all-targets) is their first real run and is the verification for this change.

Cross-compiling the crate outright is blocked on this machine — ring's build
script needs an MSVC C toolchain — so two narrower checks were used instead, and
neither is a substitute for the Windows lane:

1. The new Win32 code (DetachedSpawn, the modified spawn_windows_no_inherit,
   observe_early_exit, wait_timeout_millis, the three wrappers and both new
   tests) was extracted verbatim into a scratch crate and type-checked against
   windows-sys 0.61 for x86_64-pc-windows-msvc. The same was done for the
   #[cfg(windows)] arms rewritten in the review round — managed_startup_exit,
   and the spawn_or_retire_record closures (since replaced by
   retire_record_on_error) with their record borrows — so those were type- and
   borrow-checked for x86_64-pc-windows-msvc, not merely reasoned about. The
   current retire_record_on_error arms were not re-checked that way; the
   required windows-build-and-test lane compiled and ran them at 72b0b5e3, and
   that is their verification. (Confirmed live by injecting a deliberate i32/u32 mismatch and
   watching it fail.)
2. apps/rocm was compiled with --cfg windows forced on, purely to check that
   the cfg arms resolve. That yields 13 errors against 9 on unmodified main,
   and the four new ones are exactly std::os::windows, ExitStatus::from_raw
   and the rocm-core helper being absent from a Linux build — no unresolved
   names of our own.

No new dependency: windows-sys already enables Win32_Foundation and
Win32_System_Threading, which is all this needs.

Related

• test(e2e): drive the Lemonade recovery scenario with a planted runtime #351 (planted runtime for serve-18) surfaced this gap. Independent of this PR.
• fix(serve): wait on the engine's readiness budget, fail fast on a dead engine #202 also touches start_managed_service and restart_internal_managed_service,
  but addresses the readiness wait — an engine-supplied budget and an early exit
  once the engine records a terminal state — rather than the spawn-time liveness
  check, and adds no Windows check. Complementary; whichever lands second needs a
  small rebase.

---

• Searched tests/e2e-cucumber/expectations.toml for stale xfail rows — none relate to this behaviour.
• No new subcommand or subsystem; the change stays in the existing spawn helpers and their two call sites.

[juhovainio]

Reviewed crates/rocm-core/src/lib.rs and apps/rocm/src/main.rs in full.

This closes a real gap: the Windows managed-launch path had no equivalent of Unix's try_wait(), so an engine that died immediately at startup was reported as a successful launch. The fix is careful about the two things that actually matter here:

• observe_early_exit consults GetExitCodeProcess only after WaitForSingleObject returns WAIT_OBJECT_0, on the same handle CreateProcessW returned, before it's closed. That rules out both the STILL_ACTIVE (259) ambiguity and a PID-reuse race from re-opening by PID later, exactly as the PR description claims.
• It fails open throughout: a timed-out wait or a failed GetExitCodeProcess both come back as "still running," never as a fabricated failure.
• The service-record wedge fix is a nice catch, and I like that it's applied to the Unix branch too, not just Windows, since the same "starting" + pid-0 record could wedge future rocm serve calls on either platform.
• wait_timeout_millis's clamp away from u32::MAX (INFINITE) is tested on every platform, not just Windows, which is a smart way to get real CI coverage on the one piece of this that doesn't need a Windows host.

Two minor, non-blocking notes for the record, neither needs action:

• The Windows launch_lock is now held for the ~200ms settle window instead of releasing immediately, moving that cost inside the lock. This is called out in the PR description as intentional parity with Unix (which already paid this cost under the lock), and it's negligible next to the multi-second readiness wait the lock already guards.
• There's no end-to-end test of the main.rs glue (ExitStatus::from_raw -> failure detail -> record retirement -> bail) since that requires a real Windows host; the two rocm-core primitives are unit-tested directly instead. Given no Windows dev host was available, this is a reasonable trade-off, and the required windows-build-and-test CI lane is the real exercise of it, and it passed.

Checked the failing "E2E tests (Strix Halo, Ubuntu)" lane: it's a GPU-preflight infrastructure failure on the self-hosted runner (VRAM never freed from a prior serve), unrelated to this change.

Nice write-up in the PR description too, the call-site table and the "verified vs not verified" section made this easy to review.

[siloteemu]

🔴 Automated review · pr-review-watcher · f2611ff

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Summary

Gives the Windows managed-engine launch the startup-liveness check Unix already had, shares one settle budget and one failure message across both platforms, and retires the pre-spawn service record so a failed launch stops claiming its engine + model. Outcome: Needs work — the previous round's objection about unpinned wiring is only partly closed, and two prose claims in this round's own change are contradicted by the code beside them. Verified: read the full diff and the surrounding call graph, confirmed the Win32 wrapper preserves the creation flags and handle-inheritance behaviour unchanged, confirmed the flag-vs-subcommand reasoning behind the scripted death by reading the parser's freeform fallback, and traced every new test to the function it actually calls; the test suites were not run here, so nothing below rests on a test run. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/main.rs:6634 — the rule the comment states is broken twice in its own function. The doc reads "Every fallible step after record.write() has to go through here. Each one that does not is another way to strand the pid-0 starting corpse." After the claiming write at main.rs:6875, six steps do go through retire_record_on_error — and two do not: attach_background_stdio(&mut command, Some(&record.log_path))? at main.rs:6977, and child.try_wait().context(..)? at main.rs:6993. Both are in the arm that actually runs on the lane that runs, both are plausible failures (the child log cannot be created; reading process state fails), and both leave behind exactly the pid-0 "starting" record this change exists to prevent — managed_service_is_live counts "starting" as alive and recorded_service_pids filters pid 0, so nothing demotes it and every later serve for that engine + model reports already-running. The same pair is mirrored in the restart path at main.rs:18159 and main.rs:18174. The PR description repeats the same omission when it enumerates "six further fallible steps". Fix: bind each to a Result and pass it through retire_record_on_error, or narrow the comment to the steps it truly covers — but the comment as written is the stronger claim and the code should meet it.

tests/e2e-cucumber/features/model_serving.feature:266-267 — the scenario cannot run where the comment says it does. The comment claims the scenario is "Ungated, so it gates every PR and covers Windows and WSL2 too, where the startup check is newest", and the description states this more strongly still ("which is how the Windows call sites get covered at all"). @requires-no-gpu at model_serving.feature:268 resolves to a Skip whenever the host reports an AMD GPU (tests/e2e-cucumber/src/expectation.rs:541), and the only Windows and WSL2 end-to-end lanes are GPU-bearing self-hosted ones (.github/workflows/e2e-selfhosted.yml:633 and :914). I am inferring the lane mapping from the workflow definitions and cannot confirm which lane executed what. But the tag semantics alone settle it: a scenario that skips on any GPU host cannot cover a lane whose premise is GPU hardware. The consequence is that the two Windows call sites — the precise thing this PR fixes — have no scenario coverage at all; only the u32-to-ExitStatus adapter is pinned, by a unit test. This is the same shape of inaccuracy the previous round found: a disclosure wrong in the direction that makes the change look safer. Fix: correct both sentences to say the scenario covers the Linux no-GPU lane only, and state plainly that the Windows wiring remains unpinned — or add a gated Windows scenario and name the lane, as the contributor rules require.

apps/rocm/src/main.rs:29103 — a_spawn_that_never_started_frees_the_engine_for_the_next_serve passes with the production change it names reverted. Its doc says a spawn that fails outright would otherwise strand the record "so returning past it leaves a pid-0 starting corpse" — the production change that makes that false is the retire_record_on_error wrapping at main.rs:6969/main.rs:6989. The test never reaches either; it calls retire_record_on_error directly with a synthetic Err. Revert any of the eleven wrappings to a bare ? and this test still passes, as do the other three new ones — a_dead_engine_fails_the_launch_and_frees_the_engine_for_the_next_serve, a_live_engine_leaves_the_launch_running and a_failed_launch_record_stops_blocking_the_next_serve all call the helpers directly too. No test in the tree calls spawn_managed_engine_child past the record write (the three that call it bail at the reuse and authentication guards above it), and both restart_internal_managed_service tests bail at the endpoint-key guard. The description's mutation table asserts otherwise, and its second row names spawn_or_retire_record — a function that does not exist anywhere in the tree, having been replaced by retire_record_on_error in this same round. That row cannot be reproduced as written. Fix: make at least one test drive a call site rather than the helper, and correct the table.

Non-blocking

• apps/rocm/src/main.rs:18151,18171 — the restart path's startup check is unpinned on both platforms, not just Windows; nothing exercises it end to end either.
• apps/rocm/src/main.rs:7004 and :18200 — the final record.write()? is unwrapped; a failure there leaves a live child with a stale on-disk record. Not claimed closed by this PR, noted for the record.
• apps/rocm/src/main.rs:28930-ish — managed_engine_startup_failure_detail_without_a_log_still_points_at_it walks the whole five-iteration retry loop, paying roughly 600 ms of real sleeps in a unit test; injecting the budget would make it instant.
• tests/e2e-cucumber/tests/e2e/serving_steps.rs — the .log substring assertion adds nothing over the "managed engine exited immediately" assertion, which already contains the path.
• The scripted-failure environment variable name is written as a bare literal in both apps/rocm/src/main.rs:6582 and the step file; a shared constant would stop the two drifting apart.

[siloteemu]

🔴 Automated review · pr-review-watcher · 083e7e3

Blocking: 1. This is the first round of review from this account on this pull request.

apps/rocm/src/main.rs:6728 (and the identical site at 17860) — the branch that turns a detected startup death into a failed serve is the entire user-visible fix, and nothing exercises it on any platform.

Deleting both bail blocks restores the exact bug this pull request describes, and every test that can run stays green. The three tests added to main.rs reach the branch from neither side: two call a formatting function whose body this change does not touch, and the third calls the record-retirement helper directly on a hand-built record, bypassing the spawn path. Detection is proven in isolation and retirement is proven in isolation; the wiring between them is not, so a mis-wired or dropped call site would ship green.

Two mutations confirm the rest is real: neutering the record retirement, and dropping the wait clamp, each turn their own named test red. The implementation itself reads as sound, and the platform-parity reasoning in the pull request text is accurate.

The pull request text is explicit and correct about scenario coverage — it names the existing scenario, explains why that scenario does not reach the detached child, and points at the planted-runtime work that would. That account was read before this was filed and is not in dispute. The difficulty is that the referenced pull request is itself still open and unmerged against main, so the coverage it would supply does not exist yet.

Concrete fix and the full round of findings, including five non-blocking items, are in the review comment posted alongside this one.

[rominf]

Thanks — the blocking finding was right, and the mutation evidence was the useful part of it: deleting the branch that joins detection to response did leave every runnable test green. Addressed in 7848fc5.

Blocking — the wiring is now covered, though not the way you proposed

I agree the gap was real and I don't think it should wait. I went a different route than the suggested scenario, and want to be explicit about why and about what it does not reach.

The reason nothing could reach that branch is that it was cfg-gated to Windows in two places: out of reach of any Linux test, and not executed by anything on the Windows lane either. So rather than add a test that tries to reach cfg-gated code, I removed the reason it was gated. The four near-identical copies of the check (two Windows, two Unix) are now one function, fail_managed_launch_if_engine_died, taking an Option<ExitStatus> — platform-neutral, and directly testable on every lane. Only the u32 → ExitStatus conversion stays gated, in managed_startup_exit, and it has its own test.

Re-ran your two mutations plus the new one; each fails only its own test:

Mutation	Result
Neuter the bail in fail_managed_launch_if_engine_died (yours)	a_dead_engine_fails_the_launch_and_frees_the_engine_for_the_next_serve fails
Drop the retirement in spawn_or_retire_record	a_spawn_that_never_started_frees_the_engine_for_the_next_serve fails

The residual, stated plainly rather than implied: deleting the call to either helper at one of the two Windows sites would still ship green on Linux. Those two lines are compiled by windows-build-and-test but not executed by it. So this is narrower coverage than the scenario would give, not equivalent to it.

On the proposed scenario specifically: the shape you describe is right, and it is what should exist. The obstacle is the Given — making a detached child die on demand. serve-18's injection works because it fires inside the parent's Install RPC; there is no existing lever that reaches the child. Building one here would be building #351's mechanism a second time, in a way that would conflict with it on the same code. Between that and holding this fix until #351 merges, I'd rather land the fix with the wiring covered at the unit level now and let #351 supply the scenario, since the two are independent and #351 has to touch that path anyway. Happy to be overruled if you'd prefer to hold it — that was a genuine judgment call and I'd take your read on it.

Non-blocking

• spawn failure leaves the record wedged (main.rs:6721) — good catch, and it was the same defect this PR exists to fix, one line above where I fixed it. spawn_or_retire_record now retires the record on that path too, at all four sites, while still surfacing the spawn's own error rather than a bookkeeping error behind it. Covered by a_spawn_that_never_started_frees_the_engine_for_the_next_serve.
• the fix narrows the window rather than closing it (main.rs:6825) — agreed, and you're right that the text overclaimed. The description now says outright that an engine dying after the 200 ms settle is still reported as a success, that this is pre-existing on Unix and unchanged, and that closing the wider class is a separate change not attempted here.
• launch lock held ~200 ms longer (main.rs:6823) — the text noted the move but not the consequence. It now says a concurrent rocm serve racing for the same lock waits that much longer.
• doc comment is call-site-specific (main.rs:6513) — reworded. The pid-0/pre-spawn narrative is now scoped to the launch path, with a sentence on why the restart path differs (pre-existing record, merely stale pid) and why retiring it there is still the right call.
• the two managed_engine_startup_failure_detail_* tests are Windows-only new coverage (main.rs:28427) — stated in the description now, including that on Linux they characterise unchanged code and pass with the rest of the fix reverted.

On the CI observation

You were right not to attribute it to this diff. E2E tests (Strix Halo, Ubuntu) fails in the GPU preflight before any test runs — waiting: 0 GiB free (< 8 GiB) on repeat until the ceiling. That floor reads VRAM, which on an APU is a carveout rather than the memory the engine actually uses, so the lane reports a busy GPU on an idle host. It is unrelated to this change and is being fixed on the CI side.

Verification

cargo check --workspace --all-targets, cargo clippy --locked --workspace --all-targets -- -D warnings (sources touched first so nothing came from the clippy cache), cargo fmt --all --check, cargo test --workspace, cargo xtask manifest --check, cargo xtask check-crate-edges, scripts/smoke_local.py — all clean. Three pre-existing rocm-dash-tui agent::tests failures reproduce identically on the parent commit; they are env-mutating tests that only collide under local cargo test.

The rewritten #[cfg(windows)] arms were type- and borrow-checked for x86_64-pc-windows-msvc in a scratch crate (the full crate still can't cross-compile here — ring and aws-lc-sys need an MSVC C toolchain), and I confirmed that check was live by injecting a deliberate i32/u32 mismatch and watching it fail. Their runtime behaviour is still unverified locally; windows-build-and-test remains the real exercise.

[siloteemu]

pr-review-watcher · 7848fc5 — new round, full findings above.

The earlier objection is genuinely discharged: the shared check's logic is now pinned, and each of the four mutations reddens only its own test, exactly as the description claims.

The change request stays open for a different reason. The description says the residual gap is Windows-only, and it is not: deleting both Unix call sites of the new helper — the two that actually execute on Linux — leaves the whole binary's test set green, 765 passed. The helper is pinned; the wiring to it is not, on either platform. Separately, this alters the exit code and error text of a user-facing command, which the contributor rules require a scenario to cover, and none does.

One blocking item, five notes.

[rominf]

Round 2 addressed in 5846319. Both halves of the blocking finding were correct and I've fixed rather than argued them.

Blocking — you were right on both counts

The residual was not Windows-only. I checked your mutation before acting on it: deleting both Unix call sites leaves the whole binary test set green, 765 passed. My description said the exposure was confined to Windows, and that was wrong — the helper was pinned, the wiring to it was unpinned on every platform. The description now says so plainly instead.

§3 was unmet. I reread it, and your reading is right: this changes the exit code and error text of rocm serve, a unit test on the helper explicitly does not discharge it, and naming serve-18 while saying it doesn't reach this path is an acknowledgement of the gap rather than one of the two permitted outs.

So I took your preferred option and added the scenario — @id:serve-managed-engine-dies-at-startup (serve-22). It pins what the unit tests could not: deleting the Unix launch call site turns serve-22 red, where the same deletion left all 765 binary tests green. Being ungated it also runs on the Windows and WSL2 lanes, so it reaches the two Windows call sites that no Linux test can.

On your doubt about whether the harness can inject a death post-spawn — it can, but not the way I first tried, and the detail is worth recording. The switch is read in the parent, which is what spawns the engine, and swaps the child's arguments for ones the CLI rejects; the child is the real rocm binary, really spawned and really dead. My first attempt used an unrecognised subcommand and it silently didn't work: the CLI parses unknown subcommands as natural-language requests and exits 0. An unrecognised flag exits 2 immediately, which is what shipped.

Two things that scenario needed which are worth flagging for review: reaching a real spawn on the ungated lane meant waiving engine preparation as well as the no-GPU pre-flight — no @requires-no-gpu serve scenario reached a spawn before this one, which is why the gap survived. Both waivers sit behind e2e-test-hooks, which release builds do not enable.

Non-blocking

• six fallible steps strand the same corpse (main.rs:6726-6765) — you're right that my own doc comment's "every path" rule was broken by its own caller. All six now go through retire_record_on_error, which replaces spawn_or_retire_record and takes an evaluated Result so it can wrap the whole prefix without a second borrow of the record. The doc comment is now a rule the code actually keeps.
• ? vs let _ = inconsistency (main.rs:6558) — agreed, and it was the more harmful direction: a failed record write would have displaced the engine's own exit status and log tail, which are the only things that diagnose the launch. Now dropped there too, matching the sibling, with the reason stated at both.
• lock-drop comment (main.rs:6865-6869) — the code now says what only the PR text said: the settle runs inside the lock, deliberately, and is bounded unlike the readiness wait.
• lemonade's unwatched spawn (engines/lemonade/src/lib.rs:4376) and the prior round's five items — no action needed; thanks for re-confirming the latter rather than taking the description's word for it.

Verification

cargo check --workspace --all-targets, cargo fmt --all --check, cargo test --workspace, cargo xtask manifest --check, cargo xtask check-crate-edges, scripts/smoke_local.py — clean. Clippy run in both feature configurations (with and without e2e-test-hooks) plus the separate -p e2e-cucumber --test e2e invocation, sources touched first so nothing came from the cache; the feature-off build wanted the switch stub to be const fn, which is why it is two cfg'd definitions rather than one.

Full local E2E suite: 129 scenarios, 127 passed, 2 failed — both pre-existing expected xfails, 0 unexpected failures. I ran the whole suite rather than just the new scenario because this repo has a prior case of one extra index-resolving scenario tipping unrelated dash scenarios over; it did not recur. feature_naming passes — serve-22 appends rather than renumbering.

Three rocm-dash-tui agent::tests failures reproduce identically on the parent commit; they are env-mutating tests that only collide under local cargo test.

Still not verified locally: the Windows runtime path, unchanged from before. The #[cfg(windows)] arms are type- and borrow-checked for x86_64-pc-windows-msvc in a scratch crate; windows-build-and-test and the Windows E2E lane are the real exercise, and serve-22 now gives the latter something to exercise.

[siloteemu]

Round 2 posted — the review above has been updated in place for f2611ff1.

[rominf]

Correction — the Windows-coverage claim in my last comment was wrong

In the round-2 comment I wrote that serve-22 "runs on the Windows and WSL2 lanes, so it reaches the two Windows call sites that no Linux test can." That is not true, and I want to flag it rather than quietly edit it.

I had tagged the scenario @requires-no-gpu. That tag is skipped on any host that has a GPU — expectation.rs resolves requires_no_gpu && has_amd_gpu to Skip, and there is a unit test pinning exactly that. The Strix Halo Windows box has a GPU, so serve-22 was skipped there. It only ever ran on the Linux mock lane, and the Windows call sites stayed exactly as unexercised as you originally described. The green Windows lane on my previous push did not mean the scenario had run.

What was true in that comment, and remains true: serve-22 discharges §3 with a real assertion on the user-visible exit code and error text, and it pins the Unix call sites — deleting the Unix launch call site turns it red, where the same deletion leaves all binary tests green. The half I overstated was the Windows half.

Fixed in ef67aa7 by dropping the tag, so the scenario now runs on every lane including Strix Halo Windows. The tag was wrong on its own terms too: the premise is an engine that dies at startup, not a host without a GPU, and the test hook's waivers are what make that premise hold regardless of the host's hardware.

Worth stating plainly about the new state: the Windows lanes are the first real execution of this scenario on a GPU host — I cannot run that path locally, as this machine reports no GPU. If a GPU lane fails on it, the scenario is at fault rather than the product code, and I will say so and fix it rather than paper over it.

[siloteemu]

🔴 Automated review · pr-review-watcher · ef67aa7

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Adds a bounded startup-liveness check to the Windows detached managed-engine spawn so a child that dies on the way up fails the serve the way it already did on Unix, funnels both platforms through one shared check, and retires the pre-spawn service record on the failure paths. Outcome: Needs work — one newly introduced prose guarantee the code does not provide. Verified: ran one targeted check, cargo test -p rocm-core --lib wait_timeout_millis (1 passed); the full suite, the e2e suite and any Windows build were not run here. Confirmed by reading: @requires-no-gpu is genuinely gone from the new scenario at this head and no other tag gates it, the e2e lanes apply no tag filter so an untagged scenario is attempted everywhere, the exit-code read is reachable only after WAIT_OBJECT_0 so the PR body's "no ambiguity" claim holds, --e2e-managed-engine-startup-failure is defined nowhere and so is rejected by argument parsing as the code comment claims, and the workspace unit-test job on a Windows runner is what will execute the new Windows-only spawn tests. Checks at review time: 23 success, 1 skipped, 4 pending. Blocking: 1 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

apps/rocm/src/main.rs:6877 (comment) vs 6977 and 6994 — the new exhaustiveness guarantee is false on the Unix launch path.

Line 6877 states "From here to a live child, every fallible step goes through retire_record_on_error", and the helper's own doc at 6634 repeats it as a rule: "Every fallible step after record.write() has to go through here. Each one that does not is another way to strand the pid-0 starting corpse." Inside that span, two fallible steps use a bare ?:

• 6977 attach_background_stdio(&mut command, Some(&record.log_path))?;
• 6994 .context("failed to check managed engine startup state")?;

Either returns past the retirement, leaving exactly the pid-0 "starting" record the PR spends its longest comment explaining is permanently un-demotable — recorded_service_pids skips pid 0, so no liveness refresh can clean it and every later serve of that engine+model is refused. The Windows branch of the same function wraps every step, so the gap is Unix-only. Nothing tests it: all six new main.rs tests call helpers directly, so the claim is asserted only in prose.

This blocks because the sentence is the thing a future maintainer will rely on when adding the next fallible step, and it is wrong as written at the moment it is introduced.

Fix, and note the two halves are not symmetric:

• 6977 is pre-spawn, no child exists, so wrapping it is unambiguously safe: evaluate into a local first (let attached = attach_background_stdio(...);) then retire_record_on_error(&mut record, attached)?;. The borrow of record.log_path ends before the &mut borrow begins, matching the pattern already used at 6885–6890.
• 6994 is post-spawn and must NOT simply be wrapped: if try_wait errors, the child may well be alive, and retiring the record there would let a second serve start a rival engine on the same port. Either narrow the comment so its span ends at the spawn rather than "to a live child", or align Unix with the policy the Windows side already documents — observe_early_exit deliberately returns None when the query fails, degrading to "assume it is alive" instead of failing the launch, while Unix bails. Whichever is chosen, fail_managed_launch_if_engine_died's doc ("a child that dies on the way up produces the same error ... whichever platform the user is on") should stop implying the two platforms handle a failed query the same way, because they do not.

Non-blocking

• apps/rocm/src/main.rs:28905 and :28927 — the two managed_engine_startup_failure_detail_* tests exercise a function whose body this PR does not touch; on Linux they pass unchanged with the production change reverted. Their real contribution is compiling that function on Windows now that its cfg(not(windows)) gate is gone. The four record/retirement tests and the Windows adapter test do fail on a revert, but only because the helpers they name would not exist — none of them reaches a call site.
• apps/rocm/src/main.rs:6570 — the 200 ms settle is a fixed sleep on Unix but an early-returning bounded wait on Windows, and it now has to cover a cold rocm.exe start plus argument parsing on a Windows CI host. If it times out the scenario fails loudly (the launch is reported as started, the 45 s readiness wait runs, and the "managed engine exited immediately" assertion goes red) rather than passing silently — so the risk is flakiness, not a false green, but it is worth knowing before the first GPU-lane run.
• tests/e2e-cucumber/tests/e2e/serving_steps.rs:1402 — the scripted path replaces the whole argument vector, so the child never receives the --log argument. On Unix the parent redirects the child's stdio into the service log, so the tail branch of the detail renderer is exercised; on Windows the detached spawn does not, so the log stays empty and only the "no tail, still name the path" branch runs. The scenario's .log assertion holds either way, but the two platforms do not prove the same thing.
• apps/rocm/src/main.rs:18094 — the comment "if the restart bails before one of those, the marker stays set on disk" is inaccurate: record.stop_requested_unix_ms = None is set before the spawn and the new bail paths call record.write(), so a failed restart now persists the cleared marker. Pre-existing on the Unix startup-death bail, widened here to Windows and to the spawn-failure path, so noting rather than blocking.

Status of our prior change request

We raised one blocking objection: "the branch that turns a detected startup death into a failed serve is the entire user-visible fix, and nothing exercises it on any platform", with the supporting claim that the three added tests "reach the branch from neither side".

Discharged for the branch itself. The bail is now extracted into fail_managed_launch_if_engine_died and asserted from both sides: a_dead_engine_fails_the_launch_and_frees_the_engine_for_the_next_serve requires the error, the child's log tail, and the record released; a_live_engine_leaves_the_launch_running requires the converse. Removing the bail turns the first red. On Windows the detection half is now covered in its own right by watched_detached_spawn_reports_a_child_that_exits_immediately and its converse, plus managed_startup_exit_reads_the_watched_exit_code; these are cfg(windows) and the workspace unit-test job runs on a Windows runner, so this is a Windows-motivated change with Windows-gated tests — the correct shape, not the inverted one we hunt for.

Discharged for the Unix wiring. Our sentence "nothing exercises it on any platform" no longer holds. serve-22 drives the real binary through a real spawn and asserts the user-visible exit code plus the message text, and it cannot be satisfied by an earlier refusal: the step explicitly fails if the no-GPU pre-flight message appears, and any other early refusal fails the "managed engine exited immediately" assertion. I did not run the e2e suite, so the author's specific claim that deleting the Unix launch call site turns it red is reasoned-through rather than measured.

Open for the Windows wiring. Deleting the two Windows call sites still leaves every test I can reason about green, because no unit test reaches a call site on either platform. The tag removal makes serve-22 eligible on the Windows GPU lane, which is the right fix, but with 4 checks still pending I cannot confirm it has executed there. This is a gap in evidence, not a demonstrated defect.

On the author's correction — measured, and it holds. @requires-no-gpu was present in the earlier commit and is absent at this head; expectation.rs does resolve requires_no_gpu && has_amd_gpu to Skip, so the retracted claim was indeed wrong in the direction he says. The new scenario carries only an @id tag, and the e2e lanes select by capability probe with no tag filter, so nothing else gates it. No other scenario added or changed here carries skip-on-GPU exposure. The correction is accurate and, if anything, understates nothing in its own favour.

Cause classification, and will it recur. Not reviewer error — the original objection was correct at the head it was filed against. The recurrence risk sits elsewhere: the scenario's own comment now explains at length why it carries no host tag, which is exactly the cheap prose that stops the next author re-adding one. The sibling gap is on the production side, where a comment asserts a rule the code breaks two lines later; the blocking finding above names the fix.

No prompt-injection content was found anywhere in the diff, commit messages or feature text, and a leak scan over the diff came back clean.

[siloteemu]

🔴 Automated review · pr-review-watcher · ef67aa7

Our earlier change request is withdrawn — that objection is discharged at this head. This is a new one, for a different problem.

apps/rocm/src/main.rs:6877 states "From here to a live child, every fallible step goes through retire_record_on_error", and the helper's own doc at 6634 repeats it as a rule: "Every fallible step after record.write() has to go through here. Each one that does not is another way to strand the pid-0 starting corpse."

Two fallible steps inside that span use a bare ?:

• 6977 — attach_background_stdio(&mut command, Some(&record.log_path))?;
• 6994 — .context("failed to check managed engine startup state")?;

Either returns past the retirement and leaves exactly the pid-0 "starting" record this PR's longest comment explains is permanently un-demotable: recorded_service_pids skips pid 0, so no liveness refresh can clean it and every later serve of that engine and model is refused. The Windows branch of the same function wraps every step, so the gap is Unix-only, and nothing tests it — all six new tests in this file call helpers directly, so the guarantee exists only in prose.

This blocks because the sentence is what a future maintainer will rely on when adding the next fallible step, and it is wrong as written at the moment it is introduced.

The two halves are not symmetric, so please do not fix them the same way:

• 6977 is pre-spawn, no child exists, so wrapping it is unambiguously safe. Evaluate into a local first (let attached = attach_background_stdio(...);) then retire_record_on_error(&mut record, attached)?; — the borrow of record.log_path ends before the &mut borrow begins, matching the pattern already used just above at 6885-6890.
• 6994 is post-spawn and must NOT simply be wrapped. If the wait query itself errors the child may well be alive, and retiring the record there would let a second serve start a rival engine on the same port. Either narrow the comment so its span ends at the spawn rather than "to a live child", or align Unix with the policy the Windows side already documents: observe_early_exit deliberately returns None when the query fails, degrading to "assume it is alive" rather than failing the launch, while Unix bails.

Whichever you choose, fail_managed_launch_if_engine_died's doc ("a child that dies on the way up produces the same error ... whichever platform the user is on") should stop implying the two platforms handle a failed query the same way, because they do not.

[rominf]

Round 3 addressed in 09a20a9, and thanks for the precision on the blocking one — the naive reading of it would have introduced a bug.

Blocking — the rule was false; fixed asymmetrically, as you set out

You're right that the sentence was wrong at the moment it was introduced, and right that it is the sentence a future maintainer will lean on. Both offenders confirmed by reading the code.

attach_background_stdio — wrapped. Pre-spawn, no child exists, so retiring is unambiguously correct. Done at the launch site you named and at the restart site, which has the identical shape and whose adjacent spawn failure already retires.

try_wait — deliberately left unwrapped, and this is the part worth getting right. Wrapping it would mean treating "the liveness query failed" as "the child died", releasing the engine + model to the next rocm serve while this child may still hold the port. So the rule is narrowed to end at the spawn rather than "to a live child", and the reason is now recorded in three places: the helper's doc, and both query sites. Recording it at the sites is deliberate — the rule exists for whoever adds the next fallible step, and they will be reading the call site, not the helper.

The platform-parity doc no longer overclaims. It now says the platforms agree about an observed death, and states plainly that on a failed query they do not: Windows degrades to "assume alive" via observe_early_exit returning None, Unix propagates the error and bails. Neither retires, for the reason above.

Your one open item — here is the evidence

You wrote that you could not confirm serve-22 had executed on the Windows lane, with 4 checks pending. It has, and I checked the lane's own log rather than inferring it from the lane going green — that inference is exactly what produced my earlier retracted claim.

From E2E tests (Strix Halo, Windows) at ef67aa7:

Scenario: serve-22 - An engine that dies at startup fails the serve and names its log
 ✔  Given the managed engine dies during startup
 ✔  When the user serves a model with Lemonade
 ✔  Then serving fails and names the engine's own log
 ✔  And the failed launch does not block the next serve

Same four steps green on WSL2. Strix Ubuntu, MI350P and rad3 R9700 also passed at that head; MI300X was still queued.

One correction to the conclusion you drew from it, in the same spirit as the standard you applied to me: "deleting the two Windows call sites still leaves every test green" was true when you wrote it, but serve-22 now executes on Windows, so deleting a Windows call site should turn that scenario red there. I have not measured it — doing so needs a mutation pushed to a Windows lane — so treat it as reasoned, not measured, exactly as you treated my Unix mutation claim.

Non-blocking

• restart marker comment — you're right, and the cause is mine: the bail paths retire the record, retiring writes it, so a failed restart now persists the cleared marker where the comment promised the opposite. Comment corrected to describe what actually happens, including that the key now outlives a failed restart, which is the safe direction since the record is left "failed" and so is not live. Behaviour deliberately unchanged — that is a separate decision from making the comment true.
• the two managed_engine_startup_failure_detail_* tests characterise unchanged code on Linux — agreed, and the PR text already says so; their contribution is compiling that function on Windows now the cfg gate is gone.
• 200 ms settle vs a cold rocm.exe start — a fair thing to have flagged before the first GPU-lane run, and your analysis of the failure mode was right: it fails loudly rather than passing silently. It has now run on Windows, where the serve step took about 3 s wall-clock and the scenario passed, so the budget holds on that host. Still worth watching, since it is a fixed sleep on Unix and an early-returning wait on Windows.
• the scripted path drops --log, so the platforms exercise different branches of the detail renderer — correct, and a genuinely useful observation: Unix redirects the child's stdio into the service log so the tail branch runs, Windows does not so only the "name the path" branch runs. The .log assertion holds either way. Noted in the PR text rather than changed, because asserting the tail on Windows would mean making the detached spawn redirect stdio, which is a behaviour change well outside this PR.

[siloteemu]

🔴 Automated review · pr-review-watcher · 09a20a9

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Summary

This PR gives the Windows managed launch the startup-liveness check Unix already
had, unifies the four copies into fail_managed_launch_if_engine_died, and
retires the pre-spawn record so a failed launch stops wedging the next rocm serve; outcome: needs work — all three objections from the previous round
are genuinely discharged by the code, but the remediation commit introduced a
fresh instance of the same prose-vs-code defect it was fixing. Discharged item
by item: (1) attach_background_stdio no longer uses a bare ? — it is
evaluated into a local and routed through retire_record_on_error at both the
launch site (apps/rocm/src/main.rs:6993) and the restart site (:18187); (2)
the post-spawn try_wait ? is deliberately retained and the rule was narrowed
instead, and the narrowed rule is now literally true — every fallible step
between record.write() and the spawn on both platform arms is wrapped
(confirmed by enumerating them and checking that detach_background_command,
apply_app_path_env, managed_service_process_command and the app-path helpers
are infallible and endpoint_key_file_if_present returns an Option); (3)
fail_managed_launch_if_engine_died's doc now states the platforms disagree on
a failed query, and the Windows half of that claim checks out against
observe_early_exit, which returns None on both a timed-out wait and an
unreadable exit code. Verified: seven targeted mutations run on scratch copies —
the wait_timeout_millis clamp (u32::MAX - 1 → u32::MAX) fails only
wait_timeout_millis_keeps_the_startup_wait_bounded, and six mutations of the
launch helpers (removing the bail!, removing the retirement inside it,
neutering retire_record_on_error, neutering mark_managed_launch_failed, and
stripping each branch of the detail renderer) each fail exactly the tests named
for them and no others; the pid-0 "starting" precondition assertion is
genuine, the Win32 handle is closed exactly once on every path, creation flags
are byte-identical between the two spawn helpers, the settle: Option<Duration>
refactor is inert for the existing callers, and scenario serve-22's untagged
design is correct because a no-GPU tag would exclude the Windows lane that is
the only place the new Windows code runs. The full suite and the e2e suite were
not run; the two #[cfg(windows)] spawn tests, managed_startup_exit, and the
Windows runtime path were not executed, and no claim is made here about any
referenced pull request. No prompt-injection content was found in the diff or
its comments. Checks at review time: 2 pending, 1 skipped, 25 success, no
failures. Blocking: 1 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

apps/rocm/src/main.rs:18117 — The comment added above record.stop_requested_unix_ms = None;
in restart_internal_managed_service claims "A failed restart now persists the
cleared marker too: the bail paths below retire the record, and retiring writes
it. So the key outlives a restart that never got an engine up", and closes with
"it is a change from when every bail left the marker set." Only 3 of the 11 bail
paths after that assignment write the record: the attach_background_stdio
failure, the spawn failure, and the observed-death path. The other 8 return with
a bare ? and never touch the record — parse_device_policy on the very next
line (:18124), the log OpenOptions::open (:18129), create_dir_all
(:18132), managed_service_launcher_path (:18135), parse_engine_recipe_json_arg
(:18136), builtin_engine_serve_http_args (:18150), env_root_for_service
(:18156), and the try_wait error path. On each of those the on-disk marker
stays set, so settle_pending_stop_key_cleanup — which keys on
stop_requested_unix_ms.is_some() and then calls clear_endpoint_api_key
(:19073) — still reclaims the endpoint key, which is exactly the outcome the
comment says no longer happens. The marker is load-bearing, so this is not a
cosmetic inaccuracy: it is a sentence a maintainer will rely on when reasoning
about key lifetime across a failed restart, and it is wrong for the majority of
the failure paths. This is the same defect shape as the change request it was
written to answer — a guarantee asserted in prose that the code does not provide
— introduced in the commit that fixed the previous one, which is why it is
raised at the same severity rather than as a nit. The comment it replaced (that
a bail leaves the marker set on disk, which is correct since then the stop is
what stands) was accurate. Fix, cheaply: qualify the span to the paths that
actually persist it — e.g. "a restart that fails at or after the spawn persists
the cleared marker, because those bails retire the record and retiring writes
it; an earlier bail still leaves the marker set on disk, and the stop stands" —
or, if uniform behaviour is wanted, route the earlier fallible steps through the
retirement as the launch path now does.

Non-blocking

• PR description, mutation table — still names spawn_or_retire_record, which no longer exists at this head; it was replaced by retire_record_on_error.
• tests/e2e-cucumber/tests/e2e/serving_steps.rs:801 — the shared When step is user_serves_with_failing_lemonade_preparation, a name that no longer describes serve-22's premise (a dead engine, not failed preparation).
• tests/e2e-cucumber/tests/e2e/serving_steps.rs — output.contains(".log") is loose enough to pass on any incidental mention of a log suffix; asserting the service log's file stem would pin the intended claim.
• apps/rocm/src/main.rs:6986, :7018, :18180, :18209 — the four real call sites of the new helper are exercised by no unit test, so the wiring rests entirely on serve-22; worth stating in the description alongside the mutation evidence rather than leaving implied.

[siloteemu]

🔴 Automated review · pr-review-watcher · 09a20a9

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

All three objections from the previous round are genuinely discharged by the code at this head — that was checked item by item and is set out in the round comment. This change request is for one new problem the remediation commit introduced.

apps/rocm/src/main.rs:18117 — the comment added above record.stop_requested_unix_ms = None; in restart_internal_managed_service asserts a guarantee the code does not provide. It reads: "A failed restart now persists the cleared marker too: the bail paths below retire the record, and retiring writes it. So the key outlives a restart that never got an engine up", and closes "it is a change from when every bail left the marker set."

Only 3 of the 11 bail paths after that assignment write the record — the attach_background_stdio failure, the spawn failure, and the observed-death path. The other 8 return with a bare ? and never touch it: parse_device_policy on the very next line (:18124), the log OpenOptions::open (:18129), create_dir_all (:18132), managed_service_launcher_path (:18135), parse_engine_recipe_json_arg (:18136), builtin_engine_serve_http_args (:18150), env_root_for_service (:18156), and the try_wait error path. On each of those the on-disk marker stays set, so settle_pending_stop_key_cleanup — which keys on stop_requested_unix_ms.is_some() and then calls clear_endpoint_api_key (:19073) — still reclaims the endpoint key, which is exactly the outcome the comment says no longer happens.

The marker is load-bearing, so this is not a cosmetic inaccuracy: it is a sentence a maintainer will rely on when reasoning about key lifetime across a failed restart, and it is wrong for the majority of the failure paths. It is also the same defect shape as the objection it was written to answer — a guarantee asserted in prose that the code does not provide — introduced in the commit that fixed the previous one, which is why it is raised at this severity rather than as a nit. The comment it replaced (that a bail leaves the marker set on disk, which is correct, since then the stop is what stands) was accurate.

Suggested fix, cheaply: qualify the span to the paths that actually persist it — e.g. "a restart that fails at or after the spawn persists the cleared marker, because those bails retire the record and retiring writes it; an earlier bail still leaves the marker set on disk, and the stop stands". Alternatively, if uniform behaviour is wanted, route the earlier fallible steps through the retirement the launch path now uses.

The rest of the change stands up well. Seven targeted mutations were run on scratch copies: the startup-wait clamp and six mutations of the launch helpers each fail exactly the tests named for them and no others. The Win32 handle is closed exactly once on every path, creation flags are byte-identical between the two spawn helpers, the settle refactor is inert for existing callers, and leaving the new scenario untagged is right, because a no-GPU tag would exclude the Windows lane that is the only place the new Windows code runs.

[rominf]

Addressed in 8075d1e.

The finding is right, and I checked it against the code rather than taking it at face value: only three bails after the clear write the record — the attach_background_stdio failure, the spawn failure and the observed-death path, all of which route through retire_record_on_error/fail_managed_launch_if_engine_died, and retirement writes. Everything between the clear and the spawn returns with a bare ? and never touches the record, so the on-disk marker stays set and settle_pending_stop_key_cleanup still reclaims the endpoint key.

The comment now qualifies the span to the bails that actually persist the clear, and states the earlier-bail behaviour rather than leaving it unsaid — it is correct on its own terms: nothing was restarted, so the stop stands. I kept the behaviour as-is rather than routing the earlier steps through retirement, since the two outcomes are each right for their own case; what was wrong was only the prose.

Addressed in 8075d1e; see the reply comment for what was verified and how. Dismissing so the PR is not held by a review the automation never reconverts to an approval.

[rominf]

@r0x0r ready for your review when you have a moment — all 19 required checks are green on 8075d1e5 (the MI300X/MI350P lanes are still queued; neither is required). The automated reviewer's change request on this head has been addressed — what it flagged and how it was verified is in the comment above. Nothing outstanding from my side.

[rominf]

Rebased onto current main (01e7625) — it had gone conflicting after #432/#456/#409 landed.

One change worth flagging beyond the rebase: main added its own serve-22 (serve-past-attempts-surfaced), which collided with the number this PR used. Main's keeps 22 — local_server_records.feature cross-references it by that number — so this PR's scenario is now serve-23; its @id:serve-managed-engine-dies-at-startup is unchanged. No other file referenced the old number. No behaviour or test-logic changes in the rebase.

[rominf]

Addressed the non-blocking items from all three review rounds in 69fa765c. Here is each one, deduplicated across rounds:

Fixed in code:

• The .log assertion is loose / adds nothing — the step now reads log_path off the service record the failed launch leaves behind and requires the output to name that exact path. Because it is the path rocm itself recorded, a Windows 8.3 short form can't make the comparison disagree with itself. Mutation-checked: a build that names some other .log file passed the old suffix check and fails the new one, which prints the real path it expected. serve-23 passes all four steps locally against a hooks build.
• The no-log detail test pays ~600 ms of real sleeps — the poll interval is now injected through a _polling variant. Production keeps 120 ms (MANAGED_ENGINE_STARTUP_LOG_POLL_INTERVAL), the test passes zero, and the loop still runs every iteration: 0.66 s down to under 10 ms.
• The shared When step is misnamed — renamed to user_serves_with_lemonade. Cucumber matches on the attribute text, so no step binding changes.
• Platform asymmetry in which branch runs — stated at the assertion, as well as in the description: Unix redirects the child's stdio into the log, so it carries a tail, while Windows doesn't, so only the no-tail branch runs. Naming the path is what both branches share, and that's what is pinned.

Fixed in the description:

• The mutation table and type-check notes named spawn_or_retire_record, which no longer exists; it is retire_record_on_error now. The type-check note also says plainly that the current arms were not re-checked in a scratch crate; their verification is the required windows-build-and-test lane, green at 72b0b5e3.
• It now states that none of the four real call sites is reached by any unit test. The launch-path wiring rests on serve-23, and the two detail-renderer tests pin a function this PR only un-gates for Windows.
• The scenario is now serve-23, renumbered on rebase when main added its own serve-22. I confirmed from the Windows lane's own log that it ran all four steps at 72b0b5e3.

Already done earlier:

• The "marker stays set on disk" comment is inaccurate — escalated to blocking in a later round and fixed in 8075d1e5 (now rebased).

Left as-is, with reason:

• The restart path's startup check is unexercised end to end — this is a real gap, now stated in the description. The scripted-death switch exists only on the launch path, and restart is reachable only through the assistant's restart_server sandbox tool. Covering it would add another consumer of e2e-test-hooks while Remove the e2e-test-hooks feature #349 is working to retire that feature. Restart shares both helpers with the launch path; only its two call sites are unexercised.
• The env-var name is written in both places — both sides now name it through a constant. Sharing one constant across apps/rocm and the harness is what check-crate-edges exists to prevent, and drift would be loud rather than silent: the engine would not die and "serving fails" would go red.
• The 200 ms settle on a cold Windows start — no flake so far. The Windows lane has passed serve-23 at both ef67aa7d and 72b0b5e3, and as you noted, a timeout here fails loudly rather than passing silently.
• The final record.write()? is unwrapped — pre-existing, and as the round said, not claimed closed by this PR.

[rominf]

A correction to my reply on the env-var item: I said sharing one constant between apps/rocm and the harness is what check-crate-edges exists to prevent. It isn't — dev-dependencies are exempt from that check, and since #419 the harness has one on rocm-core. The real constraint is that the constant lives in apps/rocm, a binary crate nothing can import.

The decision stands, for a different reason. Moving the name into rocm-core would put a switch that exists only under e2e-test-hooks into rocm-core's ungated public API, and drift would be loud rather than silent: the engine wouldn't die, so "serving fails" goes red. (#466 did make the opposite move for the dash clock file, because there drift was silent; it turned out dash-09 passed even with the clock ignored.)