[rominf]

Symptom

On Windows, rocm serve --managed reported success when the managed engine process had already died during startup. It exited 0 and printed a deployment summary with readiness: starting. The user was left waiting on a server that would never come up, and nothing pointed at the reason.

On Linux the same death already failed the serve, with a tail of the engine's service log. But the failed launch's record kept claiming the engine and model, so the next rocm serve for the same pair was answered with "already running" and spawned nothing.

Root cause

• Windows: the launch spawned the engine with rocm_core::spawn_detached_no_inherit. That returns a bare pid, with no Child and no exit check. After the spawn the launch waited up to 45 s for HTTP readiness, recorded the status and returned Ok either way. A dead engine looked exactly like a slow one.
• Both platforms: the launch writes its record before the spawn, to claim the GPU: "starting" with supervisor pid 0. A failed launch never moved it out of that state. "starting" counts as live, and pid 0 is skipped by the liveness refresh, so the record blocked every later serve of that engine and model for good.

Fix

Watched spawn (Windows). crates/rocm-core gains spawn_detached_no_inherit_watching_startup.

• It keeps the existing creation flags and handle inheritance, so the child is still fully detached and still outlives the CLI.
• It waits on the process handle CreateProcessW returned, for a bounded window, before closing it. It returns the exit code (DetachedSpawn::early_exit_code) if the child has already exited.
• Checking through the still-open handle avoids the pid-reuse race that re-opening the process by pid would have.
• GetExitCodeProcess is read only after WAIT_OBJECT_0. Any ambiguous result is reported as "still running", so the check can fall back to the old behaviour but never invent a failure.

One startup check for the launch (apps/rocm/src/service_launch.rs). The launch now spawns through spawn_managed_engine_process and settles the child with watch_managed_engine_startup:

• On Windows the watched spawn has already answered. On Unix the launch settles, then calls try_wait.
• Both platforms use the same 200 ms window (MANAGED_ENGINE_STARTUP_SETTLE) and produce the same error: managed engine exited immediately with status …; inspect <log>. It names the engine's own service log, with a tail of it when the engine wrote one.
• The blind 200 ms sleep after the Windows spawn is gone: the checked wait replaces it. That wait now falls inside the launch lock, as it already did on Unix, so a concurrent rocm serve waiting for that lock waits up to 200 ms longer.

A failed launch frees its engine and model.

• A launch that fails before its spawn, or whose engine is seen dead at startup, retires its record as "failed". The next rocm serve for the same engine and model launches again.
• A child the Windows spawn saw die is never put on record. Windows records carry no start-time token, so a recorded dead pid could later be signalled after the OS reused it.
• A Unix try_wait that cannot answer leaves the child on record as "starting" rather than retiring it. The child's state is unknown, and freeing the engine and model could put a rival engine on its port.

Every launch write after the claim is now a guarded read-modify-write. This is a behaviour change beyond the startup-death fix, and it applies to every managed launch, not only failed ones.

• The writes go through LaunchRecord and update_managed_record_on_disk: putting the child on record, retiring the record, the promotion to "running", and the readiness verdict.
• Each write is applied to the record as it is on disk at that moment, and only while the launch still owns it, instead of writing back the copy the launch claimed with. A stop records its marker before it signals anything, and writing back the old copy would erase that marker. The daemon's server-recover watcher would then restart a service the user had asked to stop.
• What users can see:
  • If a stop, restart or removal changes the record while the engine is spawning or starting, the launch now fails with "managed service … changed on disk while it was launching … so the engine it started (pid N) was not recorded on it". If it happens after the startup check, the error ends "… so the launch did not mark it running". Before, the launch overwrote whatever had been written.
  • A readiness verdict for a record the launch no longer owns is not written.
• This narrows the race; it does not close it. The read and the write are separate, unlocked steps, and stop takes no lock on the manifest, so a stop that writes between them is still overwritten.

The serve spinner is cleared on every failing path before anything else prints, including a failed background-helper start (launch_then_start_helper in serve_cmd.rs).

E2E hook. A build with rocm/e2e-test-hooks can script the engine's death with ROCM_E2E_MANAGED_ENGINE_STARTUP_FAILURE.

• The launch swaps the engine's arguments for a flag the CLI rejects, so the spawned rocm really exits non-zero at startup.
• The hook also skips the no-GPU pre-flight and engine preparation, so the scenario reaches a real spawn without a GPU or a runtime download.
• A build without that feature contains no code that reads the variable. The name is the shared constant rocm_core::MANAGED_ENGINE_STARTUP_FAILURE_TEST_ENV, so the binary and the e2e suite cannot drift apart.

What this does not close: the 200 ms detection limit

The check only catches an engine that exits within the 200 ms settle window. An engine that dies later is still reported as starting, after the readiness wait, exactly as before on both platforms. That covers most real failures: a runtime or model load error usually takes longer than 200 ms.

So in practice this catches immediate deaths: a bad argument, a missing binary or DLL, a crash on start. It does not make the launch report every engine that never becomes ready.

serve-36 relies on a fresh rocm.exe rejecting its argument within 200 ms. On a heavily loaded Windows runner with real-time scanning, a slow process start could push that past the window. The scenario would then fail on its first assertion (the serve exits 0 instead of failing), not pass by accident. Reporting every engine that never becomes ready is a separate change and is not attempted here.

The restart path moved to #859

Restarting a managed service (rocm services restart, and the assistant's restart tool) still uses its own spawn code here. On Windows that is a bare pid with no startup check. #859 moves the restart onto the same spawn seam and startup check, with its own guarded RestartStartupRecord, in services_text.rs. It is stacked on this PR and stays a draft until this merges. #581, the end-to-end restart scenario, will be restacked on #859.

This PR no longer touches apps/rocm/src/main.rs.

Tests

Unit tests (service_launch.rs). These drive spawn_managed_engine_child with an injected stand-in spawn, so each launch outcome is pinned at the real call site without starting an engine:

• death at startup, through both the Unix-style and the Windows-style check;
• a live engine;
• a check that cannot answer;
• each pre-spawn failure (file preparation, missing runtime, the spawn itself);
• a stop, restart or removal racing the launch's writes;
• the readiness-verdict write;
• a failed retirement write being reported, not swallowed;
• the child-record write failing:
  • a_child_that_cannot_be_recorded_is_neither_checked_nor_retired
  • a_launch_whose_child_cannot_be_recorded_fails_with_the_write_error

Windows-only unit tests (process.rs, run on the Windows CI job):

• watched_detached_spawn_reports_a_child_that_exits_immediately
• watched_detached_spawn_does_not_report_a_child_that_keeps_running
• wait_timeout_millis_keeps_the_startup_wait_bounded

Scenario serve-36 (@id:serve-managed-engine-dies-at-startup):

• The hook scripts the engine's death.
• The serve must fail and name the engine's own log.
• The record left on disk, read raw, must say failed. On Windows it must also name no pid.
• Serving the same model again must reach the engine launch again and fail the same way.
• On Unix the retry alone cannot prove the retirement, because the liveness refresh would let it through anyway. The raw failed assertion is what proves it.
• The scenario carries no host tag, so it runs on every lane, including E2E tests (Strix Halo, Windows, current), the only lane where the watched spawn runs.

Manual check: docs/manual-testing.md §4 has steps for it.

Gate, run locally on Linux at this head:

• cargo fmt --check and cargo clippy --workspace --all-targets -- -D warnings: clean.
• cargo test --workspace --all-targets: 4914 passed, 3 failed.
  • services_text::tests::stop_of_a_container_engine_is_unconfirmed_when_the_engine_stop_fails also fails on main on this host, because its docker is a podman shim.
  • The two build_metadata git_watch_paths_* tests fail on a git commit inside a scratch repository. That looks like this host's commit-signing setup; this PR does not touch build.rs or apps/rocm/tests.

Not verified locally: the Windows build and the Win32 path. This host has no Windows target, so the Windows CI job and the Windows E2E lane are the check. That lane has not yet run green at this head.

[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.)

Dismissed by the author as stale: this approval is of 083e7e3, and five of the branch's six commits came after it, plus a rebase (now 69fa765). Please re-review the current head.

[jussielo-amd]

Read through the full diff (crates/rocm-core/src/lib.rs, apps/rocm/src/main.rs, the new scenario, and the step definitions) plus the whole review thread. This is a real, long-standing gap — Windows had no equivalent of Unix's try_wait(), so a managed engine that died at startup was reported as a successful rocm serve. The fix is careful where it matters: observe_early_exit only reads GetExitCodeProcess after WaitForSingleObject returns WAIT_OBJECT_0 on the still-open handle from CreateProcessW, which sidesteps both the STILL_ACTIVE ambiguity and the PID-reuse race a later OpenProcess would have, and a failed query degrades to "assume alive" rather than inventing a failure. The service-record wedge fix (retiring the pid-0 "starting" record so a dead launch doesn't permanently block the next rocm serve) is the right companion catch, and I like that it covers both platforms, not just Windows.

I traced the retire_record_on_error wiring by hand: all six fallible steps between the claiming record.write() and the spawn are wrapped on the launch path, try_wait's failure is deliberately left unwrapped with the reasoning recorded at the call sites (a failed liveness query isn't a confirmed death), and the restart path's "only 3 of 11 bails persist the marker" comment is now accurate. Confirmed the new serve-23 scenario and windows-build-and-test/Strix-Halo-Windows lanes are green, and this PR doesn't touch the iGPU/dGPU auto_applicable area at all.

One thing worth a quick look before merging: the MI300X E2E lane failed (serve-vllm-inference and friends) with a vLLM EngineCoreClient crash during the readiness wait — well past the 200ms startup-check window this PR adds, and unchanged on the Unix side regardless. Reads like GPU/environment flakiness on the self-hosted runner rather than anything this PR touches, and it isn't a required check, but worth confirming with a rerun. Also, the branch currently shows a merge conflict against main (just ci.yml workflow drift from commits that landed after the last rebase) — nothing to do with this PR's logic, just needs another rebase before merge.

Really thorough PR description and review trail — the call-site table, the mutation-tested claims, and the willingness to correct earlier overclaims (the Windows-coverage tag mistake, the restart-marker comment) made this fast to verify rather than just taking on faith.

[rominf]

Rebased onto main at 1845df34 (now c06ffbda) to clear the conflict. The ci.yml drift merged cleanly. Main's #251 added its own serve-23 in model_serving.feature, so this PR's scenario is now serve-24, with the same @id:serve-managed-engine-dies-at-startup, tags and steps. The description now uses the new index. No code changed.

Re-ran on the rebased head, all clean:

• feature_naming, e2e-cucumber --lib, and cargo check --test e2e
• rocm-core --lib (575 passed), rocm --bin rocm (1015) and xtask (255)
• both clippy invocations, fmt and manifest --check

[rominf]

I pushed fixes for the review at 7f7e0f56; the head is now a53d2085.

• Blocking, launch-path writes: fixed in 6a07b7e1 and a53d2085, with details in the inline reply. The launch and the restart now share one re-read-and-reconcile guard. There are seven new launch-path tests, and mutation checks cover each condition.
• Spinner: fixed in 4457db00.
• early_exit_code doc: fixed in db34a5e5. It now says the exit is reported as soon as it happens, and that the wait delays the caller by at most the window. The Windows early-exit test now asserts that the spawn returns well inside a 30 s window. That test runs only on the Windows lane; here it was type-checked against the Windows target.
• From the approval:
  • Added a_restart_whose_startup_check_cannot_answer_keeps_the_child_on_record.
  • Kept RestartEngineSeams. Its Rc lets the per-thread test override hand out the closures without holding a borrow across the restart. That keeps production and the test on one call path, which is the point of the wrapper test: catching a dropped runtime_pin. Happy to revisit if you see a simpler shape that keeps one call path.

Verified locally:

• fmt;
• tests: rocm (1277), rocm-core (747) and rocmd (153), all passing;
• xtask: 311 on the branch, and 317 on a merge with current main;
• e2e-cucumber lib and feature_naming;
• serve-22, serve-25 and serve-26;
• clippy, both invocations, plus clippy for the Windows target.

Not verified locally: the GPU-gated serve scenarios and Windows runtime behaviour, which the CI lanes cover.

The earlier red E2E runs on the dashboard-quit scenarios came from the slow-prompt race in the e2e driver. #713 fixed it on main.

Stale: approved 7f7e0f5; the head is now a53d208. Since then, 6a07b7e, 4457db0, db34a5e and a53d208 changed launch-path record writes, the spinner and a doc, all verified locally. A fresh review is needed to merge.

[juhovainio]

Reviewed the launch/restart-record hardening, the Windows early-exit detection, and the new serve-26 e2e coverage. The overall direction is solid and the restart/launch record-write and spinner-teardown fixes are real, well-targeted bug fixes. Two things before this lands:

Blocking

• The serve_cmd.rs spinner fix (folding start_managed_service and ensure_background_helper_running_quiet into one .and_then so a single spinner.clear() covers both failure sources) is correct, but it's untested and undocumented. There's no regression test that exercises the ensure_background_helper_running_quiet failure branch through this new combinator, and the 439-line PR description never mentions the spinner behavior at all, so a reviewer (or future maintainer) has no way to tell this was intentional versus a refactor side effect. See inline comment.
• CI is currently red: Test (affected crates) fails on workflow_contract::tests::preflight_runs_the_real_reclaim_assert_clear (xtask/src/workflow_contract.rs:2925). This doesn't match any of the known-flaky patterns in this repo (port races, Windows PATH mutation, self-hosted runner contention) — it's a workflow-contract assertion failure in xtask, which reads as a real regression rather than infrastructure flakiness. Worth confirming before merge.

Non-blocking

• The new/retry e2e step functions (user_serves_with_lemonade, user_serves_the_same_model_again) don't call ensure_serve_port_free() the way sibling steps do. Not a problem today since nothing currently exercises them back-to-back with a port-bound scenario, but worth lining up with the sibling steps if they get reused.
• persist_spawned_engine_child's documented race (a concurrent stop/restart can leave an orphaned, unrecorded child process) is a pre-existing, deliberate tradeoff per the code's own comments — not introduced by this PR, flagging only for visibility.

No issues found with the restart-path record writes, the Windows STILL_ACTIVE/exit-code handling, or the wait_timeout_millis clamping.

[juhovainio]

This .and_then is the right fix for the spinner-clear gap (one spinner.clear() now covers both start_managed_service and ensure_background_helper_running_quiet failures), but there's no test exercising the ensure_background_helper_running_quiet failure path through this chain, and the PR description doesn't mention this behavior change. Worth a regression test plus a line in the description so this doesn't read as an unrelated refactor.

[rominf]

Added in d93e8f13. You were right that the earlier "no unit test is possible" was too quick. Spinner only draws on a terminal, but the logic that matters is the step that runs the two calls, and that step can be tested on its own.

The launch, the helper start and the clear-on-error are now in launch_then_start_helper. It is generic over a small LaunchProgress trait, and serve() passes the real Spinner. The function calls clear() itself, so a call site cannot hand in a clear that does nothing. Three tests use a recording progress line:

• a_background_helper_that_fails_to_start_clears_the_launch_progress: the launch succeeds, the helper fails, and the line is cleared exactly once.
• a_failed_launch_clears_its_progress_and_never_starts_the_helper
• a_successful_launch_leaves_its_progress_for_the_caller_to_clear: the smoke test still relabels the line, so serve() clears it afterwards.

I checked each one by mutation:

Mutation	Tests that fail
Helper called outside the guarded Result, the shape before the fix	the helper-failure test
Clear removed	the helper-failure and failed-launch tests
Clear also on success	the success test
Helper started even after a failed launch	the failed-launch test

One limit: if serve() stopped calling the function and went back to bare ?, these tests would still pass. The function would then have no caller outside the tests, and clippy -D warnings would reject it as dead code. So that case is caught by the lint, not by a test.

The PR description now has a section on this.

[rominf]

Re: Test (affected crates) red on preflight_runs_the_real_reclaim_assert_clear. This was the branch's stale base, not a regression in this PR.

• Why it failed. That job tests the PR head, not the merge with main. The old head a53d2085 was based on fcc646e9, which predates fix(ci): keep the GPU reclaim decoys alive on multicall coreutils #712 (5dfa9a85). fix(ci): keep the GPU reclaim decoys alive on multicall coreutils #712 fixed this test for multicall coreutils on ubuntu-26.04, where it failed with coreutils: unknown program 'llama-server'. This PR doesn't touch xtask/ or the workflow files at all.
• The fix. I rebased onto ea1d8713 (current main, which includes fix(ci): keep the GPU reclaim decoys alive on multicall coreutils #712). The new head is 8ce98f54. The rebase had no conflicts.
• The PR's content is unchanged. I compared the PR's own diff before and after (fcc646e9...a53d2085 against ea1d8713...be15afb9, i.e. before the two new commits). The sorted added and removed lines are identical, 3300 each, and the file list is the same.
• Main's new work doesn't clash with it. The changes that landed since overlap this PR in main.rs, docs/architecture.md, docs/testing.md and model_serving.feature. The symbols this PR relies on still resolve, and each has a single definition. serve-26 is still the next free index after main's serve-25, and feature_naming passes.
• Local checks. workflow_contract::tests::preflight_runs_the_real_reclaim_assert_clear passes on the rebased tree. So do the full xtask suite (317 tests) and the other checks listed in the description.

CI on 8ce98f54 is the confirmation; I'll watch it.

[rominf]

Re: user_serves_with_lemonade / user_serves_the_same_model_again not freeing the serve port (non-blocking). Fixed in 8ce98f54, with one deliberate difference from the sibling steps.

ensure_serve_port_free() does two things. It kills listeners on the serve and assistant ports, then waits for the GPU to report enough free VRAM, for up to 2 minutes. Neither current user of this step ever loads a model. serve-18 fails at the backend install, and in serve-26 the engine exits at startup. serve-26 has no host tag, so it runs on every lane, including the GPU lanes, and it calls this step twice. On a device that never reaches the VRAM floor, that drain wait would add up to about 4 minutes to the lane and protect nothing.

So I split the port half out as free_serve_ports(). ensure_serve_port_free() now calls it, then the VRAM wait, so the existing steps behave exactly as before. The Lemonade step calls free_serve_ports(). The retry step goes through the Lemonade step, so it is covered too. The step's comment says that a scenario which loads a model through this step should switch to the full ensure_serve_port_free().

Checked locally: serve-18 and serve-26 pass. The local -n serve- run matches the commit before this change, with the same 11 passes and 15 GPU-hardware failures, since this host has no usable GPU.

One correction to the premise: not every sibling serve step calls it. The refusal steps and user_serves_gpu_required don't. The steps that start a real engine do.

[rominf]

Re: the orphaned-child race in persist_spawned_engine_child (non-blocking, for visibility). Acknowledged. No change in this PR.

The race: a stop can confirm between the spawn and the first record write that names the child. That leaves the child running but unrecorded. Two things already cover it:

• The launch's error names the child's pid, so the child can be found.
• The restart path has the same gap, from before this PR.

The follow-up is the one described in my earlier reply on the record-write thread: both paths should stop the child they just spawned when that write finds the record has been taken over. It changes the launch and the restart together, so I've kept it out of this PR's scope.

Addressed on 8ce98f5 (reviewed a53d208). The red Test (affected crates) was the stale base: the branch is rebased onto ea1d871, which includes #712, and the check is green. The spinner fix is now launch_then_start_helper, with three tests and four mutation checks, and is described in the PR body. The Lemonade serve steps free the serve ports first. The replies are inline and in the PR comments.

[juhovainio]

Automated review summary

Reviewed via two independent passes: a 4-way file fan-out code review (rocm-core process.rs/lib.rs, service_launch.rs, main.rs+runtimes_cmd.rs, serve_cmd.rs+e2e+docs) and a two-axis mattpocock-skills review (standards, spec-fidelity). No blocking findings.

review-code (4 independent passes)

No finding scored >=70. Non-blocking notes:

• watched_detached_spawn_does_not_report_a_child_that_keeps_running (Windows-only, reviewed by inspection) is a legitimate but weak regression guard — it would also pass if the whole watched-spawn feature were reverted. The positive-direction sibling test does properly pin the behavior.
• observe_early_exit's safety-doc comment says PROCESS_QUERY_INFORMATION access is required; the only call site already holds a full-access handle from CreateProcessW. Doc-precision nit only.
• Self-disclosed depth limit: of service_launch.rs's ~90+ tests, 6 load-bearing ones covering TOCTOU-sensitive retire/race paths were individually mutation-traced (all sound); the rest were read in full but not individually traced.
• All runnable tests pass on Linux (cargo test -p rocm --bin rocm, cargo test -p rocm-core --lib wait_timeout_millis); cargo check --tests clean for both crates. Windows-cfg-gated tests reviewed by inspection only (not executable on this host).

mattpocock-skills — standards axis

No hard violations. Commits are Conventional-Commits + DCO-signed-off; new behavior has unit-test and Gherkin (serve-26) coverage per AGENTS.md; docs (architecture.md, testing.md) updated in the same change.

Judgement-call smells (non-blocking):

1. Duplicated literal — the e2e trigger env-var name is defined independently in two places: MANAGED_ENGINE_STARTUP_FAILURE_TEST_ENV (service_launch.rs) vs. MANAGED_ENGINE_STARTUP_FAILURE_ENV / "ROCM_E2E_MANAGED_ENGINE_STARTUP_FAILURE" (serving_steps.rs). A rename on one side would silently desync the e2e hook from the flag it's meant to trigger. Worth a shared pub(crate) const if convenient, not a blocker.
2. Parallel test-fixture scaffolding in main.rs and service_launch.rs for the two call sites of the now-unified spawn seam — the production-side duplication this PR removed wasn't mirrored on the test side.
3. LaunchProgress trait (single production implementor, exists for test-double injection) — consistent with existing repo seam convention (e.g. SpawnManagedEngine), not flagged as a violation.
4. Minor: exit_status_from_code test helper's #[cfg(windows)] / #[cfg(not(windows))] bodies differ by one line — likely unavoidable given platform ExitStatusExt differences.

mattpocock-skills — spec axis

No missing/partial requirements, no scope creep, no implemented-but-wrong findings. Every concrete claim in the PR description was checked against the diff and matches (flag preservation, shared 200ms settle budget, both-platform retire-before-bail, shared spinner clear-on-error, port-freeing, serve-26 shape). Caveat: targeted spot-check against the description's claims, not an exhaustive line-by-line audit.

CI

Clippy, build-and-test, affected-crate tests, both Windows build-and-test jobs, and most E2E lanes pass. 4 checks show failure, none attributable to this diff:

• Hosted-runner E2E: 2 of the 4 "failures" are serve-01/serve-02, marked expect_xfail: true by the harness itself. The other 2 are TUI dashboard quit-confirm/timing scenarios the diff doesn't touch.
• Strix Halo Windows E2E (+ its downstream consolidated-report job): failed on real Windows hardware; logs unavailable. The new Windows logic itself was traced and found sound (exit-code read gated strictly on WAIT_OBJECT_0, no TOCTOU, safe degrade-to-"alive" on wait failure) — most consistent with this repo's known history of Strix Halo Windows runner flakiness, but not confirmed as flake vs. regression since logs weren't available.

No Jira/ticket ID is referenced in the PR body, commits, or branch name, so no separate ticket-scope check applies.

---

Automated multi-pass review (review-code + mattpocock-skills:code-review, both axes). Flagging the env-var literal duplication as the one easily-actionable item; everything else is informational.

[rominf]

Rebased onto main at 1c33248c to clear the docs/architecture.md conflict with #535; the head is now bc1b68e2.

• The conflict: test: check that every advised rocm command parses; fix dashboard labels that don't #535 reworded the service_hints.rs entry, and this PR rewrites the neighbouring service_launch.rs entry. Both edits are kept. Otherwise the PR's own diff is unchanged.
• Interactions checked:
  • test: check that every advised rocm command parses; fix dashboard labels that don't #535: this PR prints no new rocm … advice, and the advised-commands checker passes.
  • vllm: point import failures at rocm diagnose #646, feat(dash): offer a Doctor report on the Diagnose screen, review first #647, fix(therock): order runtime versions by PEP 440 so rocm update never offers a downgrade #566 and test(e2e): don't count harness stalls against the quit timeout #719: no overlap.
  • fix(output): replace non-ASCII punctuation with ASCII in CLI output text #683: the ASCII contract passes.

Two new commits on top, from @juhovainio's review:

• 35e30298: the env var that scripts the startup death is now one constant, rocm_core::MANAGED_ENGINE_STARTUP_FAILURE_TEST_ENV, used by both service_launch.rs and the serve-26 steps. Renaming it on one side is now a compile error rather than a silently dead hook. No new crate dependency: the e2e target already depends on rocm-core.
• bc1b68e2: observe_early_exit's safety note now says the only caller passes the full-access handle CreateProcessW returned, and names the minimum rights each call needs.

On the other notes:

• watched_detached_spawn_does_not_report_a_child_that_keeps_running is a weak guard on its own. It only shows that a running child isn't reported as dead; …_reports_a_child_that_exits is the test that pins the feature.
• The parallel test fixtures in main.rs and service_launch.rs are deliberate for now. Merging them means moving the restart tests out of main.rs, which is better as its own change.

Verified locally:

• fmt;
• tests: rocm, rocm-core, rocmd, xtask, e2e-cucumber, and feature_naming;
• clippy, both invocations, plus clippy with --features rocm/e2e-test-hooks, on Linux and the Windows target;
• the serve-* scenarios that don't need a GPU, including serve-26.

Still left to CI: the GPU serve scenarios, and the Windows half of the startup check on its hardware lane.

Stale: approved 8ce98f5; the head is now bc1b68e. It was rebased onto 1c33248 with its diff otherwise unchanged, plus 35e3029 (one shared env-var constant) and bc1b68e (safety-doc wording), both verified locally. A fresh review is needed to merge.

[juhovainio]

Requesting changes on one item, the rest is non-blocking.

Blocking

1. The PR description omits a user-visible behaviour change. Every managed launch and restart now writes the record through an ownership-guarded read-modify-write (update_managed_record_on_disk, LaunchRecord, record_launch_readiness, persist_spawned_engine_child), not only the failed-startup path. Consequences that are not in the description: the launch's "running" promotion and the child-persist step now bail with a new "changed on disk while its engine ... was starting" error if a concurrent stop, restart or removal intervened, and the readiness verdict is silently not written when ownership is lost. Please describe this in the body, or split it into its own PR so it can be reviewed as the behaviour change it is.

Non-blocking

• Detection only covers an engine that dies within the 200 ms settle window. serve-26 passes only if a fresh rocm.exe rejecting an unknown flag exits inside that window. On a loaded Windows runner with real-time scanning a slow process start would turn the scenario into a 45 s readiness timeout instead of a fast failure, which is a plausible flake. Real engine deaths (runtime or model load) usually take longer than 200 ms, so the practical reach of this check is narrow. Worth stating plainly in the description.
• The PR description is stale. It names the scenario serve-25 and quotes an old retry step, and it keeps per-round narration (older head SHAs, test counts, "never executed" next to "confirmed executing"). Please refresh it to the final state of the branch.
• Size and shape: about 4k diff lines and 33 commits under a fix(serve) title. The spawn-seam and restart refactor and the spinner-clear change in serve_cmd.rs are separate concerns from the Windows death check and would be easiest to review as their own PRs.
• docs/manual-testing.md does not describe the new rocm serve failure behaviour (AGENTS.md asks for observable-behaviour changes to be reflected there).
• CI: E2E tests (Strix Halo, Windows, current) failed before any scenario ran, on a 403 downloading the TheRock tarball from repo.amd.com (infra, not this change). That means serve-26 and the Win32 watched-spawn path have not executed on the Windows E2E lane at this head, so please re-run it and confirm. MI300X and W7900 were still pending when I checked.

Inline comments follow.

[juhovainio]

Blocking (see summary): this guarded read-modify-write now sits under every managed launch and restart, and it changes behaviour beyond the failed-startup fix. A stop, restart or removal that lands meanwhile now makes the launch bail with a new error, and the readiness verdict is dropped silently. Please document it in the PR description or split it out.

[rominf]

Addressed two ways. The restart half of the guarded writes (RestartStartupRecord, the restart's use of the shared seam) moved out to #859, so this PR only changes the launch path. The PR description now describes the launch's guarded writes as the behaviour change they are, in "Every launch write after the claim is now a guarded read-modify-write". It applies to every managed launch, not only failed ones. It covers the new "changed on disk while … launching / starting" failures, says the readiness verdict is not written once the launch no longer owns the record, and says the race is narrowed, not closed.

[juhovainio]

Not atomic. The doc comment says the change is applied to the record "as it is on disk now", but this is an unlocked read (read_managed_service_manifest) followed by current.write(), and stop takes no lock. A stop that writes between the read and this write is still lost; the window narrows but does not close. Either soften the comment to say it narrows the window, or lock the manifest. The tests simulate the race by writing before the update, not concurrently.

[rominf]

Agreed. The read and the write are separate unlocked steps, and stop takes no lock, so a stop that writes between them is still lost. In 44f5e54 the doc on update_managed_record_on_disk says so: it narrows the window (compared with writing back a copy read before the spawn or the readiness wait), it does not close it. The PR description says the same. Locking the manifest would mean stop, rocmd and the liveness refresh all taking that lock, which is a larger change than this PR.

[juhovainio]

When the log stays empty this re-reads on every poll (120 ms interval), adding roughly 600 ms before the error is reported. On Windows the detached spawn redirects nothing and the child only fills the log if it opens it itself, so most Windows failures would pay this wait. Consider skipping the polling on Windows, or polling only when the log file exists.

[rominf]

I'd keep the poll on both platforms. The launch creates the log before the spawn, so "poll only when the file exists" would always poll. On Windows the child is passed --log-path, and the engine adapters write their own output there (Lemonade through spawn_hidden_console_with_log), so a death after argument parsing can leave a tail a moment after the exit. The ≤600 ms is paid only on this failure path, which used to cost the full 45 s readiness wait before reporting a false success.

[juhovainio]

This module doc is now self-contradictory: it says "relocated from main.rs, mechanically and with no behavior change" and then goes on to describe behaviour it has since gained, while keeping the old Phase 6a/6b narrative. The same phrasing is in docs/architecture.md, where the bullet is now one very long line. Please rewrite it to describe the module as it is.

[rominf]

Rewritten in 44f5e54. The service_launch.rs module doc now lists what the module does today: the launch, the spawn seam and startup check, the record bookkeeping, and the key and autostart checks. The relocation history is cut to one sentence. In docs/architecture.md the service_launch.rs bullet now builds on main's corrected text and adds the launch's startup-death check and guarded writes as a separate sentence, instead of one long run-on line.

[juhovainio]

About 700 lines land in main.rs here (the restart seams, RestartStartupRecord, and their tests), against the AGENTS.md guidance not to grow it while an extraction pass is pending. RestartStartupRecord could live in service_launch.rs next to LaunchRecord. Related smell: restart_engine_seams has a #[cfg(test)] branch inside a production function and allocates Rc<dyn Fn> seams on the production path.

[rominf]

Resolved by the split. This PR no longer touches apps/rocm/src/main.rs. The stand-in spawn helpers the tests share now live in service_launch.rs's test module. The restart seams, RestartStartupRecord and their tests moved to #859, which puts them in services_text.rs (where main has since moved the restart), not main.rs. The #[cfg(test)] override in restart_engine_seams and the Rc<dyn Fn> seams went with them, so that discussion continues on #859.

[juhovainio]

A test-only switch exported unconditionally from a production crate. The other e2e hook (ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE) stays a string literal in both crates. Consider gating this behind cfg(test) or a feature, or say why it differs.

[rominf]

The constant is inert data. The code that reads it is behind rocm/e2e-test-hooks, so a build without that feature contains nothing that reads the variable. Gating the constant behind rocm-core/e2e-test-hooks would mean the e2e crate enabling that feature. Cargo merges features across a workspace build, so that would also switch on rocm-core's other test hooks (hardware_root.rs) in any build that includes the e2e crate. AGENTS.md §3 asks for one shared constant when a value is matched across crates. ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE is still two string literals, and could move to the same pattern in a follow-up.

[juhovainio]

This retry step cannot prove the retirement on Unix: as the comment notes, the liveness refresh would let a retry through even with the record unretired, so "reaches the engine launch again" passes either way. The retirement claim rests entirely on the earlier failed status assertion. That is fine, but the scenario's last line reads as stronger than it is.

[rominf]

Right. On Unix the retry passes even without the retirement, because of the liveness refresh. The proof is the step before it, which reads the record raw and requires failed (and, on Windows, no pid). The scenario comment says so, and the PR description now says it explicitly under serve-36 (renumbered from serve-26, since main took 26 to 35). The last line claims only what it checks: the retry reaches the engine launch again.

[nowycondro]

🤖 Automated review (Claude Code) · bc1b68e

Scope of this pass: manual static read of the full diff (get_diff, 3985 lines across 10 files) — no Rust toolchain or Windows host available in this sandbox, so nothing was compiled, run, or tested; this is a reading-only review. I read in full: crates/rocm-core/src/process.rs (the new DetachedSpawn/observe_early_exit/wait_timeout_millis Win32 logic), crates/rocm-core/src/lib.rs, and apps/rocm/src/serve_cmd.rs's new launch_then_start_helper. I read the core new abstractions in apps/rocm/src/service_launch.rs and the restart_internal_managed_service_with rewrite in apps/rocm/src/main.rs closely (retire_record_on_error, fail_managed_launch_if_engine_died, watch_managed_engine_startup, LaunchRecord/RestartStartupRecord, update_managed_record_on_disk) but did not line-by-line audit every one of the ~25 new unit tests in those two files, nor the full tests/e2e-cucumber step wiring beyond the new serve-26 scenario and its steps.

Findings: no blocking issues found.

This PR is unusually thorough — the Windows watched-spawn primitive (process.rs) correctly observes the child while hProcess is still open (avoiding the PID-reuse race the PR description calls out), only trusts GetExitCodeProcess after WAIT_OBJECT_0, and degrades to "assume alive" on any ambiguous Win32 result rather than inventing a failure. The record-retirement bookkeeping (retire_record_on_error, fail_managed_launch_if_engine_died, watch_managed_engine_startup, and the parallel LaunchRecord/RestartStartupRecord paths) is internally consistent with its own extensive doc comments: I traced the three outcomes of watch_managed_engine_startup (Observed-dead / Observed-alive / Pending) against ManagedStartupRecord::persist_spawned_child and retire, and the "child the spawn saw die is never put on record, but a child discovered dead by a pending check is retired naming it" asymmetry (which is the crux of avoiding a stale-PID wedge) holds in both the launch and restart call sites.

Two low-confidence, non-blocking observations, offered for completeness rather than as action items:

1. crates/rocm-core/src/process.rs, in watched_detached_spawn_does_not_report_a_child_that_keeps_running (new Windows-only test): cleanup is let _ = terminate_process(spawn.pid); — if termination fails the spawned ping.exe leaks for the rest of the CI job. Minor test-hygiene nit, not a correctness issue in shipped code, and the test asserts before any cleanup could matter to its own pass/fail. Confidence: low (pre-existing pattern likely elsewhere in this test file too).
2. The restart path's RestartStartupRecord::retire only calls current.reset_for_restart() when !self.child_on_record, so a restart whose engine survives its startup check but whose eventual readiness verdict fails (the documented pre-existing gap: "port binding and model loading happen after the 200 ms window") is never retired/reset at all by this code path — which the PR description explicitly acknowledges as an intentional, pre-existing, out-of-scope limitation shared with Unix and the pre-PR main. Not a regression; flagging only because it's the one place the diff's own "what this does not close" caveat is load-bearing for understanding blast radius. Confidence: low (matches stated intent, not a bug).

I could not verify: actual runtime behavior of the new Win32 path on a real Windows host (the PR body itself states this is unverified beyond the required CI lane and type/borrow-checking against windows-sys in a scratch crate), and I did not independently re-run or mutate the test suite to confirm the mutation-testing claims in the PR description.

🤖 Generated with Claude Code

[nowycondro]

🔴 Automated review · pr-review-watcher · bc1b68e

Review — needs work

Full review of the whole change (d59f596...bc1b68e).
Implements ROCMAI-675 ("serve: on Windows a managed engine that dies at startup is reported as success"); ticket retrieved (title/description/status only, status "In Progress"), and the change's scope matches it — the PR does not claim to close it.

Blocking

• watch_managed_engine_startup's own record-write failure is new, untested, and its doc comment overclaims what the error contains — apps/rocm/src/service_launch.rs:789,794 (call sites), doc at :780.
  persist_spawned_child(pid)? (both branches) and the function itself are new in this PR (confirmed via git show <base>:apps/rocm/src/service_launch.rs, which has no such function — only a bare try_wait()). The doc comment at line 780 claims that when "the write itself fail[s]... the error names its pid," covering both of the two listed failure exits. Tracing the write path: persist_spawned_engine_child → LaunchRecord::update → update_managed_record_on_disk → ManagedServiceRecord::write(). Only the "write refused because something else changed the record" branch actually bail!s with a message naming the pid (apps/rocm/src/service_launch.rs:473-487); a genuine I/O failure instead propagates write()'s own error context, which names only the manifest path (crates/rocm-core/src/service_record.rs:333-350), never the pid. So the comment's claim is wrong for one of the two cases it describes.
  More importantly, this is genuinely new behavior this change introduces, and it is untested: no test in the file's mod tests forces persist_spawned_child's write itself to fail (as distinct from the well-covered "refused because a stop/restart/removal raced it" case, e.g. a_stop_asked_for_while_the_launch_spawns_survives_its_writes). This is not a case where testing it would be impractical — the exact technique needed (fs::create_dir_all(&record.manifest_path) to force the write to fail) is already used in this same file for an analogous case, a_retirement_that_cannot_be_written_is_reported_not_swallowed (line ~2096). A regression that silently swallowed this error, or that routed it through retire_record_on_error (freeing the claim out from under a child that might still be alive — the opposite of what the surrounding design intends), would go undetected by the current suite.
  Confidence 85 · logic/test-coverage · Fix: add a test exercising this branch (reusing the directory-in-place-of-manifest technique) asserting the launch fails, the error is NOT swallowed, and the child is not retired/freed while possibly still alive; narrow the doc comment's "the error names its pid" claim to the branch where it's actually true, or make it true for both.

Non-blocking

• Two more pre-spawn fallible steps have no dedicated failure-path test — apps/rocm/src/service_launch.rs:1037-1039 (managed_service_launcher_path()) and :1040-1054 (builtin_engine_serve_http_args(...)). Every other step between the claim-write and the spawn (dir/log creation, missing runtime, invalid key file, the spawn itself) has its own test driving spawn_managed_engine_child to fail at exactly that point and asserting the record is freed; these two don't. Low severity: both are hard to fail deterministically in practice (one resolves the running binary's own path, the other only fails on a serialization type that essentially can't error), so the gap is a minor completeness item rather than a live risk.
• Three new tests in apps/rocm/src/main.rs exercise an unnecessary ~600ms real-time delay (the production 5×120ms log re-read poll against a log file the test never writes to), making them slower than they need to be. Not incorrect, just avoidable overhead.

Decisions for the author

None

Positive signals

• observe_early_exit (crates/rocm-core/src/process.rs) checks GetExitCodeProcess only after WaitForSingleObject confirms WAIT_OBJECT_0, so the STILL_ACTIVE/exit-code-259 ambiguity genuinely cannot arise, and the liveness check is done entirely through the already-open process handle rather than by re-opening by PID — the PID-reuse race the design claims to avoid is actually avoided, traced concretely.
• The "retire the claim record on every fallible step before the spawn" discipline (retire_record_on_error) is applied completely and correctly across all seven pre-spawn steps in spawn_managed_engine_child, each backed by its own real end-to-end test driving the public function rather than a generic smoke test — a notably thorough and precisely-scoped test suite overall (three independent reviewers, briefed to find gaps, converged on the same single real gap above).
• The e2e scenario's final assertion ties to the exact log_path recorded on disk rather than a loose .log-suffix match, and its retry step reuses the identical engine+model key, so it genuinely exercises the record-retirement fix rather than passing for an unrelated reason.

Deployment notes

• Two self-hosted CI lanes are currently red on this exact head (bc1b68e): E2E tests (Strix Halo, Windows, current) (fails in ~2m41s producing no test results) and E2E tests (MI300X, current) (and the E2E consolidated report (self-hosted) that depends on them). This is the one CI lane that exercises the Windows half of this PR's own fix, so it matters that it hasn't re-confirmed at this head. However, comparing against five other recently-merged, unrelated PRs (#762, #755, and others) shows the identical failure signature and duration recurring independently of this change — strong evidence this is a pre-existing self-hosted-runner flake rather than a regression this PR introduces, so it is not raised as a blocking finding. Whoever merges should still get a green run of the Windows lane at this head (or an explicit maintainer waiver) before relying on the PR's own claim that the scenario passed on Windows — that claim was confirmed at two earlier commits in this branch's history, not at the current head.
• User-observable behavior change (already disclosed in the PR body): a Windows managed launch now holds the cross-process launch lock ~200ms longer than before, and now fails fast (within the existing 200ms settle window) instead of waiting up to 45s to misreport success.
• Explicitly disclosed, legitimate gap: the restart path's startup-death handling has unit coverage but no end-to-end scenario coverage (blocked on a separate dependency per the PR text).

What this covered

Every file in the diff, read completely, with two deliberate exceptions stated below: crates/rocm-core/src/lib.rs, crates/rocm-core/src/process.rs, apps/rocm/src/service_launch.rs, apps/rocm/src/serve_cmd.rs, apps/rocm/src/runtimes_cmd.rs, tests/e2e-cucumber/features/model_serving.feature, tests/e2e-cucumber/tests/e2e/serving_steps.rs, docs/architecture.md, docs/testing.md were each read in full via five independent fan-out workers (one per coupled file-group) plus my own direct reads for the two doc files. apps/rocm/src/main.rs is 22,046 lines total with only ~1,300 of them touched; reading the entire file was infeasible, so its reviewer was scoped to the touched production region (restart_internal_managed_service/_with, record_restarted_child) plus every new/changed test, read completely with surrounding context — this is a deliberate, stated narrowing, not a skip. d59f596...bc1b68e.

Agent-instruction adherence checked against this repo's AGENTS.md (411 lines, read in full): the user-observable-behavior-needs-a-scenario rule (§3) is satisfied by the new serve-26 scenario; module-placement and shared-seam documentation in docs/architecture.md matches the actual code; commit signing/DCO is confirmed via the green "Commit signatures + sign-off" CI check rather than local verification (this sandbox's git lacks the SSH allowed-signers file needed to verify locally). Commit history (33 commits) was read for message quality and structure — each commit is atomic and descriptive, consistent with iterative response to earlier review rounds disclosed in the PR body, not incremental noise.

Did not run: history pass was limited to confirming (via git show <base>:<path>) that the functions in the one blocking finding are new rather than pre-existing, rather than an exhaustive blame-pass over every touched region. Prior-changes pass did not run — no prior review-comment history was read, by this method's own rule (discussion is never read during a review). CI status was read via gh pr view --json statusCheckRollup and gh pr checks, taken at face value, and cross-checked against five other recently-merged PRs' checks to distinguish a pre-existing flake from a regression (see Deployment notes).

Not reconciled against any discussion on this PR — none was read, per this method's rule that discussion is never part of a review.

[rominf]

Fixed in 44f5e54.

• a_child_that_cannot_be_recorded_is_neither_checked_nor_retired drives watch_managed_engine_startup with a record whose write fails, on both the Windows-style and the Unix-style branch. It asserts that the write's own error is returned, that no startup check runs, and that nothing is retired. Dropping the ? on persist_spawned_child in either branch turns it red.
• a_launch_whose_child_cannot_be_recorded_fails_with_the_write_error covers the same through spawn_managed_engine_child, with a directory in place of the manifest.
• The doc on watch_managed_engine_startup now says only the ownership refusal names the pid. A failed write reports its I/O error, which names the manifest.

The restart's side of this check moved to #859.

[nowycondro]

🤖 Automated review (Claude Code) · 4f6dd4d

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.

Review — needs work

Full review of the whole change (diff 9875940...4f6dd4d). Implements the Windows startup-death detection and record-retirement fix described in ROCMAI-675; the PR's own description explicitly scopes the restart path out to a stacked follow-up (#859).

Blocking

• A readiness-verdict write silently returns stale, pre-wait record data when the service record was fully removed mid-launch — apps/rocm/src/service_launch.rs:588-608 (record_launch_readiness).
  When the guarded write finds the launch no longer owns the record (recorded == false), it falls back to services_text::read_managed_service_manifest(...).map_or(launch.record, |(current, _)| current). That re-read fails (returns Err) when the manifest file is gone entirely — reachable via a concurrent rocm services remove during the up-to-45s readiness wait — and Result::map_or silently discards the Err and returns launch.record: the stale, pre-wait copy (e.g. still "running", pointing at paths that no longer exist). This contradicts the function's own doc comment ("the record as it is now is returned untouched") and is surfaced straight to the user/dashboard via ManagedLaunchReport. The three launch_readiness_write_* tests cover stop/restart/liveness-refresh but not full removal, so nothing catches it. Every other write-after-claim site in this PR (persist_spawned_engine_child, the "running" promotion) fails loudly instead of silently reporting stale data.
  Confidence 78 · logic · Fix: on a failed re-read, propagate that the record is gone (or treat absence as its own terminal status) instead of defaulting to the stale in-memory copy.

• The new e2e scenario (serve-36) has no corresponding entry in docs/testing.md — AGENTS.md §5 requires updating docs/testing.md "in the same change" when observable behavior changes, "not leave user-facing docs for a follow-up." docs/testing.md already documents every other scenario with comparable platform/tag nuance in this same feature (serve-24, serve-28 through serve-35), each with the same kind of "why this tag, why this lane" rationale that serve-36's own Gherkin comment carries. serve-36 is the one scenario of this shape missing a docs/testing.md entry.
  Confidence 72 · mechanical · Fix: add a short paragraph for serve-36 alongside the serve-28..35 entries, mirroring the existing style.

Non-blocking

None.

Decisions for the author

• The guarded read-modify-write "narrows, does not close" the record race — tradeoff, already disclosed in both the PR description and the code's own doc comments (update_managed_record_on_disk, LaunchRecord). Verified the claim against the code: the read and the write are indeed separate, unlocked steps, and a concurrent stop can still land between them. A full manifest lock would close it but is a larger change; confirm narrowing is the intended scope for now.
• A child whose spawn is observed alive, but then loses the ownership race to a concurrent stop/restart/removal before persist_spawned_engine_child writes, is left running with nothing on record that rocm services stop can ever reach (watch_managed_engine_startup/persist_spawned_engine_child). This is explicitly disclosed in the function's own doc comment and is a deliberate choice (retiring here would be worse — see the doc's reasoning), not a silent gap. Worth confirming the error message naming the orphaned pid is considered sufficient mitigation.

Positive signals

• LaunchRecord::update's ownership check correlates the on-disk record to the launch's own copy by actual identity (supervisor_pid + supervisor_start_ticks + liveness), never by position or a blind overwrite — exactly avoiding the id-vs-position pitfall this kind of concurrent-write code commonly falls into.
• ManagedStartupRecord/retire_record_on_error/fail_managed_launch_if_engine_died give Windows (Observed) and Unix (Pending) startup checks one shared choke point, so both platforms produce identical user-facing errors and record bookkeeping instead of duplicated, divergent logic.
• scripted_managed_engine_startup_failure() has two cfg-gated definitions (real check vs. a const fn false), so a release build (without e2e-test-hooks) provably contains no code path that reads the test-only env var; release.yml independently confirms the shipped binary is built without that feature.
• The regression tests for spawn_managed_engine_child inject a stand-in spawn at the real call site (not a mock several layers removed), pinning what the record looks like at each step of a dead-at-startup, ambiguous-check, and racing-stop/restart/removal launch — traced several of these by hand and confirmed they'd fail if the specific branch they target were broken.

Deployment notes

• The author's own PR description states plainly that the Windows build and Win32 path are "not verified locally" and that the Windows E2E lane "has not yet run green at this head" — confirmed independently: as of this review, no CI/CodeQL/E2E workflow run exists yet for commit 4f6dd4d at all. The most recent available signal on this branch (commit bc1b68e, Oct 26) showed both the Windows self-hosted E2E lane and the MI300X lane failing, but that commit predates a rebase onto a much newer main (the diff between it and the current head touches ~260 unrelated files), so that failure cannot be attributed to the current head with any confidence either way. This fix should not be considered verified on Windows — the one platform it targets — until its own CI/E2E run is green at this commit.
• Restarting a managed service (rocm services restart, and the assistant's restart tool) still spawns via the old, unwatched path on Windows; the exact bug this PR fixes for rocm serve --managed is still open for restarts until the stacked PR (#859) merges. Already disclosed in the PR description, repeating here only as an operational heads-up.

What this covered

Every file in the 9-file diff was read in full (not just diff hunks), plus surrounding/consumer code needed to judge it in place: engine_ipc.rs, engine_runtime_select.rs, engines/lemonade/src/lib.rs and process.rs, and services_text.rs, reached via targeted identifier searches (spawn_managed_engine_process, MANAGED_ENGINE_STARTUP_FAILURE_TEST_ENV, scripted_managed_engine_startup_failure, spawn_detached_no_inherit) to confirm the managed-launch spawn is a single level (not routed through the separate, unrelated engines/lemonade::launch_service/spawn_serve_http_background path) and that the restart path's continued use of the old spawn matches the PR's stated scope.

apps/rocm/src/service_launch.rs (the largest, most important file) was covered by one genuinely independent fan-out worker, reading the entire file and entire diff blind, in addition to my own direct reading of its production code paths. The other four groups — crates/rocm-core/src/process.rs+lib.rs, apps/rocm/src/serve_cmd.rs+services_text.rs, the e2e feature+step files, and the two docs files — were reviewed single-context by the consolidating pass after four further fan-out attempts were rejected by the harness ("concurrent subagent limit reached... do not retry"), a shared-capacity condition outside this review's control. Those four passes are therefore not independent of this write-up; weight them accordingly.

Ticket ROCMAI-675 was retrieved directly from Jira: "serve: on Windows a managed engine that dies at startup is reported as success," status In Progress, description matching this PR's stated symptom. CI status was read live from the GitHub Actions API for the current and most recent prior commit on this branch (see Deployment notes), not assumed or re-derived from workflow YAML.

Did not run: the "prior changes" pass (this method does not read earlier review threads) and no local cargo build/cargo test (no Rust toolchain in this review environment) — test-quality conclusions above are argued from reading the assertions against the implementation, not from executing them. Not reconciled against any PR discussion or earlier review round.

🤖 Generated with Claude Code