[r0x0r]

Symptom

On Windows, a managed engine that failed left no trace of why. The service log
stopped at its last progress line, rocm serve --verbose printed only
server process exited and exited 0, the service manifest said stopped, and
the engine state file stayed at starting.

Root cause

Two independent gaps.

The output went nowhere. spawn_managed_engine_child spawns the internal
__engine-serve-http child per platform. The Unix branch calls
attach_background_stdio, so the child's stdout/stderr land in the service log.
The Windows branch called rocm_core::spawn_detached_no_inherit, which attaches
them to nothing — so the error chain a failing engine prints to stderr reached
no one.

Nobody checked whether the child was still alive. Neither platform looked at
the child's fate after spawning it, so a readiness wait that times out was
indistinguishable from one that will never succeed because the child already
died, and the attached log-streaming loop treated every self-exit as a graceful
stop.

Fix

• Attach the service log on Windows, on both the launch and restart paths.
• Add a liveness check before accepting a non-Serving readiness result, on the
  launch path (finalize_managed_launch_readiness), the restart path
  (record_restart_readiness) and the attached path. A dead child fails the
  command and stamps failed on both the manifest and the engine state file; a
  readiness timeout on a child that is still alive keeps reporting its previous
  status and exit 0, since a large model can legitimately take longer than the
  budget.
• Distinguish an external stop from a crash. AttachOutcome::Stop fires only
  from the attached session's own keystroke reader, so a rocm services stop
  from another shell lands in the same ServerExited arm as a crash — and piped
  CI stdio has no keystroke reader at all. The stop marker
  (stop_requested_unix_ms) is now consulted first, so an intentional stop stays
  exit 0 with no failed stamp.
• Back the Unix liveness check with ManagedChildLiveness, which retains the
  Child and calls try_wait(). The managed engine is a direct child
  (setsid only, no double fork), so kill(pid, 0) succeeds for a zombie this
  process has not reaped — a signal probe alone reproduced the original symptom
  on Linux.
• As a platform-generic backstop, run_builtin_engine_serve_http logs the full
  error chain and marks the engine state file failed on any error exit,
  covering a gap in the attach and any engine path that errors without updating
  its own state file.

Notes for review

This revives work from 2026-10-15 that was never pushed. It was written
against af315e0a, 113 commits behind, and main has since moved most of the
touched code out of main.rs. The merge commit relocates it:
restart_internal_managed_service_with / record_restarted_child /
record_restart_readiness into services_text.rs, and
run_builtin_engine_serve_http into engine_ipc.rs (whose aim arm, added
meanwhile, gets the same failure capture). The branch's serve-25/serve-26
are renumbered serve-33/serve-34 — main took those numbers meanwhile; the
@id: tags were already unique and are unchanged.

Relationship to #437 (ROCMAI-675). The two are complementary, not competing.
#437 makes Windows notice that the child died and read its exit code; this
makes the reason survive. They are orthogonal arguments to the same private
primitive: spawn_windows_no_inherit already takes an optional stdio-handle
pair, and #437 adds an optional settle duration. #437 should rebase onto this,
after which its Windows spawn can set both.

New spawn_detached_with_log. The first version of this fix reused
spawn_hidden_console_with_log, which swaps DETACHED_PROCESS | CREATE_NO_WINDOW
for CREATE_NEW_CONSOLE — a conhost per engine, for a background service that
wants no console. STARTF_USESTDHANDLES is independent of the creation flags, so
the log is attached while staying detached. spawn_hidden_console_with_log keeps
its exact previous behaviour for its own caller, the lemond daemon.
Blanket bInheritHandles is unchanged and inherent to passing std handles this
way; scoping it would need STARTUPINFOEXW with an explicit handle list.

Known limitations carried forward

Residual stop-vs-crash race (documented, not closed). The guard that keeps a
deliberate stop from being reported as a crash still consults
status == "stopped", which is not sound proof: refresh_managed_service_runtime_liveness
reconciles any record with all-dead pids to "stopped" without a marker, so a
concurrent rocm services/rocm dash/daemon read can relabel a crash. StopWatch
narrows this (it latches any stop seen while the child was still watched) but cannot
close it — removing the status term turns the external-stop scenario red, because a
confirmed stop clears its own marker inside one poll interval and leaves no other
trace. Closing it needs a distinct status for a bare dead-pid reconciliation, or a
durable marker for a confirmed stop; both change bookkeeping shared with rocmd and
the dashboard, so they are deliberately out of scope. Reasoning and evidence are at
managed_service_stop_was_requested.

Windows PID-reuse on ManagedChildLiveness. The Windows arm reopens the child by
bare PID on each liveness check, across windows up to 45 s (or a whole attached
session). This is not a new hole — Windows's process_is_running has always had it,
and the stop path already accepts the equivalent gap — but this change is the first to
key the crash/starting decision on it. PR #437 uses a race-free held-handle design for
the same problem, so this is left to the #437 reconciliation rather than forked here.

Deployment note

rocm serve --verbose/--foreground changes observable behavior on a managed-engine
crash: it now exits non-zero with a new message and stamps the manifest and engine
state failed, where it previously printed server process exited and exited 0. Any
external automation keyed on that string or on the old exit code will need updating. A
deliberate stop is unchanged and still exits 0.

Verification

Run on a Linux MI300X box: cargo build --workspace (with and without
rocm/e2e-test-hooks), cargo clippy --workspace --all-targets -- -D warnings,
cargo clippy -p e2e-cucumber --test e2e -- -D warnings (the --all-targets
sweep does not cover the e2e step files), cargo fmt --all --check, and the full
workspace suite — 0 failures.

serve-33/34 pass under HIP_VISIBLE_DEVICES="", which puts the harness on its
no-GPU premise. serve-33 was confirmed to fail when the ServerExited crash
branch is reverted to the pre-fix Ok(()), so it discriminates rather than
passing vacuously.

Pushed with SKIP=cargo-clippy,cargo-test: those hooks cannot compile on macOS
(rocm-engine-lemonade uses st_mode, u16 on Darwin and u32 on Linux). Both
were run on Linux instead, as above — the skip records where they ran, not that
they were waived. verify-commits, license-headers, cargo fmt and the e2e
clippy hook all ran on push.

Not verified locally: the Windows paths. They are cfg(windows), and the
MSVC target's C dependencies need lib.exe, which the Linux box does not have.
CI's windows-build-and-test job is the real check on them.

[juhovainio]

Thanks for tackling this. I read the full diff against ROCMAI-541 and have four things I'd want addressed before merge; details are inline or below.

Blocking

1. CI hasn't run on this head commit (only the docs check shows up) and the PR conflicts with main. The Windows spawn/liveness paths, the changed spawn_hidden_console_with_log path used by the lemond daemon, and the removal of #[cfg(not(windows))] on the read_optional_tail_lines import are all unverified. Needs a rebase and green required checks.
2. serve-33 pins the wrapper text ("exited unexpectedly") and the non-zero exit, but not the engine's own failure reason. That string is emitted even when the log tail is empty, so the scenario would stay green if the reason never reached the log or the CLI output, which is the core ask of the ticket. Please assert the scripted death message in the output and the service log too.
3. The Windows log attach (the headline fix) has no test that runs on Windows; the new liveness test is Unix-only and serve-33 would not catch child output still going nowhere. A pure helper or injectable seam that the Windows job can exercise would do it.
4. Docs aren't synced: rocm serve --verbose now exits non-zero with a new message where it used to print "server process exited" and exit 0. README, the --help text and docs/manual-testing.md still describe the old behavior.

Non-blocking / questions

• On the non-verbose path a crash is only reported after the full 45s readiness wait, and that path only has a unit test that doesn't assert the exit code.
• engine_ipc.rs: the wrapper stamps failed on any Err from the engine entry. If an engine returns Err when stopped via rocm services stop, an intentional stop would read failed; I couldn't tell from the diff whether the stop marker is consulted on this path.
• managed_service_stop_was_requested treats status == "stopped" as a requested stop. Please confirm nothing other than an operator stop (liveness refresh, rocmd reconciliation) can write stopped before that check, or a crash would be reported as a clean exit.
• The error chain can end up in the service log more than once (explicit append, tracing, and the CLI's own Error: print now that stdio is attached).
• The "mark failed" merge pair is repeated in five places and the stop-vs-crash decision in three; a helper each would help.
• Mutation evidence in the PR text covers serve-33 only; serve-34, the restart-path tests and the stop-marker tests should be shown to fail with their target broken too.
• At about 2100 lines, with stop-marker, restart-path, zombie-reaping and merge-vs-write conversions beyond the ticket, this would be easier to review split up.
• docs/testing.md cites engine_ipc.rs and serve_cmd.rs as bare backtick paths; AGENTS asks for relative links.

[juhovainio]

This asserts only "exited unexpectedly", which managed_engine_exit_failure_detail emits even when the log tail is empty. Please also assert that the scripted death message ("scripted e2e engine death after startup") appears in the output and the service log; otherwise the ticket's core ask (reason reaches log and CLI) isn't covered and the Windows attach could regress unnoticed.

[juhovainio]

Stale doc: it says pid_is_running is injected as |_| false, but the function takes child_is_alive: impl FnMut() -> bool and the test passes || false.

[juhovainio]

The liveness check only runs after the 45s readiness wait returns, so a crashed engine makes non-verbose rocm serve hang the full 45s before failing, and this path has no scenario (the unit test doesn't assert the exit code). Consider feeding liveness into the wait loop, or covering it with a scenario.

[juhovainio]

Passing std handles makes bInheritHandles true for every managed engine (previously none were inherited). A background rocm serve whose stdout/stderr is a captured pipe may now keep that pipe open for the engine's lifetime. Please verify on Windows with piped output; scoping the inheritance would need a STARTUPINFOEX handle list.

[nowycondro]

🤖 Automated review (Claude Code) · 46b7fe2

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 (merge-base c39ee0c6 through head 46b7fe23, all 7 commits). Implements ROCMAI-541 ("Windows: managed-serve failures are never logged; the user only sees server process exited", status In Progress). The description's account of the diff is accurate and unusually self-aware — it already names the main-branch scenario-number collision and the relationship to sibling PR #437 — so there is no description/scope mismatch to report.

Blocking

• The PR has a real merge conflict with current main, from an independent scenario-number collision — tests/e2e-cucumber/features/model_serving.feature:538,554
  This PR adds Scenario: serve-33/serve-34. main has independently added serve-33 through serve-39 for unrelated work (a native-Windows platform gate and gated-AIM scenarios) using the same numbers. Confirmed by actually dry-running the merge (git merge --no-commit --no-ff origin/main from the PR head) — CONFLICT (content): Merge conflict in tests/e2e-cucumber/features/model_serving.feature — and by GitHub's own mergeable_state: "dirty" on the live PR.
  Confidence 100 · architectural · Fix: rebase onto current main and renumber this PR's scenarios to the next free IDs.

• No required CI check has executed for this commit. Only a Read-the-Docs build shows up (get_check_runs returns 0 check runs; get_status shows a single success status, the docs build). build-and-test, clippy, prek, and the required windows-build-and-test lane have not run at all, so none of the Windows spawn/liveness changes, the spawn_hidden_console_with_log refactor, or the #[cfg(not(windows))] removal on read_optional_tail_lines's import have been verified by CI at this commit.
  Confidence 100 · architectural · Fix: resolve the conflict above, push, and let the required checks run green before this is re-reviewable with confidence.

• README.md and docs/manual-testing.md were not updated for the new rocm serve --verbose/--foreground crash behavior, violating the project's own same-change doc-sync rule (AGENTS.md §5). The change makes a crashing managed engine exit non-zero with a new message and a failed manifest/engine-state stamp, where it previously printed server process exited and exited 0. Grepping both files for either string returns nothing, and the one passage in README.md that already documents the --verbose/--foreground attach behavior (README.md:891-893, Ctrl-D/Ctrl-C) says nothing about what happens on a crash. Only docs/testing.md was touched.
  Confidence 85 · mechanical · Fix: add a sentence to README.md and docs/manual-testing.md describing the new exit code/message/failed-stamp behavior on an attached crash.

• The headline Windows fix (log attachment) has no test coverage reachable from any required CI check — neither unit nor e2e. crates/rocm-core/src/process.rs's new spawn_detached_with_log (line 101), spawn_windows_with_log (line 138) and the ManagedChildLiveness::Windows arm (line 461) have no #[cfg(windows)] test exercising them — the file's existing Windows tests (WindowsFixture-based, further down) are all pre-existing and unrelated; one of this PR's own doc comments admits "this repo's test environment has no Windows runner to exercise it on." Separately, the new e2e scenarios (model_serving.feature:538,554) depend on the ROCM_E2E_SCRIPTED_ENGINE/e2e-test-hooks mechanism and are tagged @requires-no-gpu only (no Windows tag or inclusion list). The one required Windows job that runs e2e at all (.github/workflows/ci.yml:932-954, inside windows-release) is explicitly E2E_ONLY_LIFECYCLE=1 with E2E_ALSO_RUN_IDS naming three unrelated comfyui scenarios, and is "Deliberately NOT built with rocm/e2e-test-hooks" per its own comment — so even if these two scenarios were added to that list, the scripted-engine hook they depend on would not compile in. This also means the PR description's claim that "CI's windows-build-and-test job is the real check on" the Windows paths overstates what that job actually exercises: it will confirm the code compiles and existing tests still pass, not that the new log-attach logic works.
  Confidence 85 · logic · Fix: add an injectable/pure-helper Windows unit test for spawn_detached_with_log/ManagedChildLiveness::Windows, and either get serve-33/34 running on a required Windows lane with e2e-test-hooks, or add a dedicated Windows-only scenario/unit test.

• managed_service_stop_was_requested's reliance on record.status == "stopped" is not sound proof of an intentional stop, and this PR is the first to build a safety contract on it. apps/rocm/src/service_launch.rs:321-325 treats stop_requested_unix_ms.is_some() || record.status == "stopped" as "this exit was not a crash." But apps/rocm/src/managed_service_status.rs:274-277 (pre-existing, unrelated to this PR) writes status = "stopped" the instant a tracked pid is found dead during an ordinary status read (rocm services list/status, rocm dash, a daemon poll) — its own comment says so explicitly: "dead pids alone do not distinguish a stopped service from a crashed one." No stop_requested_unix_ms is set on that path. If a concurrent status read lands during this PR's new 45s launch/restart readiness windows (or the long-lived attach loop) right as a genuine crash happens, that reconciliation can flip status to "stopped" first, and this PR's new guard would then read the exit as a clean stop — silently swallowing the crash this whole PR exists to surface. The new tests only cover the case where stop_requested_unix_ms is set together with status: "stopped", never status == "stopped" alone.
  Confidence 85 · logic · Fix: only trust stop_requested_unix_ms as proof of an intentional stop; have the liveness-reconciliation path use a distinct status (e.g. "crashed"/"unreachable") for a bare dead-pid discovery instead of reusing "stopped".

• serve-33's "engine state... reads failed" assertion doesn't test the code path it's meant to cover. tests/e2e-cucumber/tests/e2e/serving_steps.rs:2344-2355 asserts both record["status"] == "failed" (the manifest) and engine_state["status"] == "failed". The engine-state file, however, is already stamped "failed" unconditionally by engine_ipc.rs's record_engine_serve_http_failure, which runs inside the detached child itself the instant the scripted engine errors — independent of service_launch.rs's own ServerExited handling and its still_watching_this_supervisor guard. Breaking or deleting the CLI-side (service_launch.rs) engine-state write would leave this assertion green; only the manifest-status half actually depends on the new parent-side logic.
  Confidence 70 · logic · Fix: assert something that only the parent-side guarded write can produce (e.g. a supervisor-identity-mismatch case), or move the engine-state check to a dedicated unit test of run_attached_service's guard.

Non-blocking

None scoring above the reporting threshold — see Decisions for the author for lower-confidence/judgment items.

Decisions for the author

• ManagedChildLiveness::Windows reopens the child by bare PID on every liveness check (crates/rocm-core/src/process.rs:461-479), across windows up to 45s (launch/restart readiness) or the whole attached session — a PID-reuse race. This isn't a new hole (Windows's process_is_running has always had this property, and PR #499's own history already accepts the equivalent gap for the stop path), but this PR is the first to key the crash/starting decision on it in three places. This PR's own commit 7ad214c2 explicitly flags it as deferred: "Left as-is per this round's explicit scope: the Windows ManagedChildLiveness PID-reuse race (a known fork against unmerged PR #437)... cross-PR sequencing calls for a human, not this round." PR #437 uses a race-free held-handle design for the same problem. Tradeoff: merge now with this narrow, already-accepted-elsewhere residual gap and track reconciliation with #437 separately, vs. hold this PR until it adopts #437's approach. Since this reasoning lives only in a commit message and not in the PR description, it's worth carrying forward into the description either way so a future reader doesn't have to dig for it.
• Scope/size — at ~1624 insertions across 10 files, bundling the narrow Windows-log-attach fix with a broader stop-vs-crash redesign touching three call sites (launch, restart, attach) makes this harder to review and rebase (as the conflict above shows) than necessary. Consider whether the log-attach fix and the liveness/stop-vs-crash logic could land as two PRs. (non-blocking-improvement)
• Duplicated "mark failed" merge pattern across roughly six call sites (engine_ipc.rs, services_text.rs ×2, service_launch.rs ×3) with no shared helper is itself not a defect, but it's the kind of duplication that produced the asymmetry in the blocking status-check finding above — factoring it into one helper that always checks the stop marker first would make that class of bug harder to reintroduce. (non-blocking-improvement)

Positive signals

• Doc comments throughout are unusually rigorous — nearly every non-obvious decision names the exact race or prior bug it guards against (e.g. why merge_json_state_file is used instead of record.write(), why supervisor_pid/supervisor_start_ticks rather than engine_pid is the right identity to gate on).
• Mutation-style self-verification was actually performed and documented, not just asserted: the PR text states serve-33 was confirmed to fail when the crash branch is reverted to the pre-fix Ok(()), matching AGENTS.md's own "shown to fail when the code it covers is broken" rule.
• Commit history is clean and well-labeled (round-by-round B1/B2/B3/N1-N5 commits, each individually signed off), not a trail of "wip"/"fix" noise.
• The spawn_hidden_console_with_log → shared spawn_windows_with_log refactor is a careful, verified-safe extraction: the Lemonade daemon's existing caller is confirmed unchanged in behavior.

Deployment notes

rocm serve --verbose/--foreground changes observable behavior on a managed-engine crash: it now exits non-zero with a new message and stamps the manifest/engine-state failed, where it previously exited 0 and printed server process exited. Any external automation or script keyed on the old string or the old exit code will break — this should be called out explicitly once the doc-sync finding above is addressed.

What this covered

Read completely: apps/rocm/src/engine_ipc.rs, apps/rocm/src/serve_cmd.rs (diff + surrounding context), apps/rocm/src/service_launch.rs, apps/rocm/src/services_text.rs, crates/rocm-core/src/lib.rs, crates/rocm-core/src/process.rs, docs/testing.md, tests/e2e-cucumber/features/model_serving.feature, tests/e2e-cucumber/tests/e2e.rs, tests/e2e-cucumber/tests/e2e/serving_steps.rs — via four independent background review passes (engine_ipc/services_text/serve_cmd; e2e tests/docs; AGENTS.md agent-instruction adherence; git history/blame) plus my own direct, complete read of service_launch.rs/process.rs/lib.rs, README.md/docs/manual-testing.md, and .github/workflows/ci.yml's Windows e2e configuration. Also ran a local dry-run merge against origin/main to confirm the conflict, and checked the live PR's CI/check status directly via the GitHub API. The root AGENTS.md was loaded and applied as the agent-instruction file.

Fan-out ran at reduced parallelism for one group: the shared environment's subagent-concurrency ceiling was saturated by other sessions for part of this review, so the "core launch/liveness" file group (service_launch.rs/process.rs/lib.rs) was reviewed directly by this driving context rather than by an independent worker — all other passes ran as independent background workers as intended.

Disclosure on the mandatory pre-review claim-check: resolving the live PR state required fetching existing reviews and the PR body, which necessarily exposed this context to the full text of an existing human review (by a different reviewer, state CHANGES_REQUESTED, same commit) and the PR's own description before all findings above were settled. No finding above was derived from that review's content — each was independently re-derived and verified from the code, CI configuration, and git history as cited inline. Several findings overlap with that existing review's points (CI/merge-conflict status, the Windows test gap, doc sync, the stop-vs-crash status ambiguity, double-logging, the repeated merge pattern, PR size) — that is convergent independent discovery, not copying. The PR description was read in full as part of this same required call; per method it was used only at the end, to state the ticket relationship and check for a scope mismatch, not to shape findings. Not reconciled against that review's thread or any other discussion.

Did not run: cargo build/test/clippy myself (reasoned from code, the PR's own stated verification, and CI configuration rather than executing the suite in this sandbox).

---

🤖 Generated with Claude Code

[r0x0r]

Thanks — this was a genuinely useful read, and point 2 in particular found a real hole. Pushed as 8c81370. Taking the points in order.

Fixed

2. serve-40 didn't pin the failure reason. You were right, and for the reason you gave: managed_engine_exit_failure_detail emits "exited unexpectedly" on both branches, including when the log tail is empty, so the scenario would have stayed green in exactly the failure this ticket is about. It now asserts the engine's own death message in both the service log and the CLI output.

I checked that the new assertion actually discriminates rather than just being longer. Breaking only the error-message tail was not enough to fail it — on Linux --verbose also streams the log to the terminal, so the reason still reaches the user by a second path, which is worth knowing. Disabling both delivery paths (the Unix attach_background_stdio and the record_engine_serve_http_failure backstop) fails it precisely where intended, while the old assertions still pass:

✘  Then serving fails with a non-zero exit and names the crash
   the engine's own failure reason never reached the service log — the bug this scenario exists for

3. No Windows-runnable test for the log attach. Added one in process.rs: it spawns a child through spawn_detached_with_log and asserts both streams land in the log — stderr included, since that is where an engine's error chain goes. It runs on the windows-build-and-test lane.

4. Documented the behavior change. README's --verbose section now states the non-zero exit, the quoted log tail and the failed stamp, and that a deliberate stop still exits 0.

Non-blocking, also done: extracted mark_service_failed for the manifest+engine-state pair that was repeated at three sites, with the merge-not-write reasoning stated once; docs/testing.md now uses relative links.

Mutation evidence, as asked. Each test fails when its own target is broken:

Mutation	Fails
failure detail + backstop stop reaching the log	serve-40 (new assertion)
attach path ignores the stop marker	serve-41
record_restart_readiness liveness gate removed	restart_readiness_fails_closed_when_the_restarted_child_died
restart path ignores the stop marker	restart_readiness_treats_a_requested_stop_as_clean_not_a_crash
ServerExited crash branch back to Ok(())	serve-40 (original evidence)

Two corrections

1a. CI wasn't a separate problem from the conflict — it was caused by it. With the PR unmergeable GitHub can't build refs/pull/854/merge, so pull_request-triggered workflows never start; only the Read the Docs check (which keys off the branch) reported. Merging main fixed both at once, and CI is running now.

1b. The #[cfg(not(windows))] removal on read_optional_tail_lines is required, not accidental. managed_engine_exit_failure_detail is deliberately platform-generic — that is the point of the fix, since the log now exists on Windows too — and calls it unconditionally:

pub(crate) fn managed_engine_exit_failure_detail(service_id: &str, log_path: &Path) -> String {
    let recent_lines = read_optional_tail_lines(log_path, 80, "service log");

Restoring the gate would fail the Windows build on an unresolved import. The gate was correct before only because the sole Windows-side caller didn't exist yet. (The still-gated managed_engine_startup_failure_detail is the Unix-only one, which is probably where the expectation came from.)

4a. The docs weren't stale. rg "server process exited" matches only the source and my own new docs/testing.md text — nothing in README, --help or docs/manual-testing.md ever described the old exit-0 behavior, so there was nothing to correct. I've added the missing description rather than fixed a stale one.

Open questions, answered

• Does the engine_ipc.rs wrapper stamp failed on an intentional stop? No. A stop kills the child, so the engine process is gone and that code never runs — the stop is adjudicated in the attach/launch/restart paths, which all consult the marker first. serve-41 covers this end to end.
• Can anything but an operator stop write stopped before the check? Not in the window that matters. stop_requested_unix_ms is written before any stop signals anything, and it is the first term; status == "stopped" is the second, reached only when a stop already confirmed the child gone. A liveness refresh demotes to failed/stopped only for a record that is already not running.
• Duplicate error chain in the log. Real, and deliberate belt-and-braces: the attach covers the normal path, the explicit append covers a gap in the attach. Duplication beats silence, which was the bug. Happy to dedupe if you'd rather.
• Non-verbose path only reports after the 45s wait. Correct and intentional — a still-alive child that is merely slow must not be failed early, so the verdict waits for the readiness budget. Worth a follow-up to report a dead child sooner.

On splitting

Fair, and I'd push back only partly. The stop-vs-crash and zombie-reaping work isn't scope creep: the first version of this fix turned every ordinary piped serve --verbose stop into a failure, and on Linux the liveness check was inert against an unreaped child — so each is a defect the headline fix introduced or exposed. Separating them would land a fix that's known-broken in between. The restart path is the one genuinely separable piece. Happy to split that out if you want it smaller.

[nowycondro]

🤖 Automated review (Claude Code) · a849ce1

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 (base 87a7c7a7 through head a849ce1, 9 commits; the 11-file, 1745/89-line diff matches the PR's own additions/deletions). Implements ROCMAI-541 ("Windows: managed-serve failures are never logged; the user only sees server process exited", status In Progress). All three of the ticket's acceptance criteria (error chain reaches the service log on Windows, rocm serve reports failure with a non-zero exit, the engine state file is stamped failed on any error exit) are addressed by this change; the PR does not claim to close the ticket, and the extra stop-vs-crash/restart-path work is additive to the literal AC text.

Blocking

• The PR currently conflicts with main, from a fresh scenario-number collision. tests/e2e-cucumber/features/model_serving.feature: this PR's Scenario: serve-40/serve-41 collide with an independently-added serve-40 from main's current tip (commit db7e5408, "ROCMAI-827: start the background helper only while an automation needs it", #838), which uses the same number for unrelated work. Confirmed by directly dry-running git merge --no-commit --no-ff origin/main from the PR head — CONFLICT (content): Merge conflict in tests/e2e-cucumber/features/model_serving.feature — and by the live PR's own mergeable_state: "dirty".
  Confidence 100 · architectural · Fix: rebase onto current main and renumber this PR's two scenarios to the next free IDs.

• The "child died before its readiness wait even started" branches on both the launch and restart paths stamp failed without first checking whether a stop was requested, contradicting this file's own documented contract. apps/rocm/src/service_launch.rs (spawn_managed_engine_child's immediate post-spawn liveness check) and apps/rocm/src/services_text.rs (restart_internal_managed_service_with's equivalent branch, ~line 1085) both call mark_service_failed/an unconditioned engine-state merge the instant a freshly-spawned child is found dead within its first ~200ms — neither consults managed_service_stop_was_requested first. mark_service_failed's own doc comment in service_launch.rs states the precondition explicitly: "Callers reach this only after managed_service_stop_was_requested has already ruled out an intentional/external stop." Every sibling failure branch this PR adds (finalize_managed_launch_readiness, record_restart_readiness, the attached ServerExited arm) does consult it first; these two do not, so an external rocm services stop landing in that narrow post-spawn window can be reported as a crash. This is corroborated directly from this PR's own git history: commit 7ad214c2's message describes adding the stop-check explicitly to finalize_managed_launch_readiness/record_restart_readiness (its "B1") as a separate, later fix from the one that touched this exact branch ("B2" — which only fixed the manifest/engine-state write mechanism, not the missing stop-check), so the gap was never actually closed for this branch in either round. On the restart side specifically, the bug compounds: update_restarting_record's CAS guard (names_same_processes + status match) would detect a concurrent stop (the status would already differ) and return Ok(None), but that Option return value is discarded — so the engine-state file is still stamped failed unconditionally and the restart command still bail!s with a failure message regardless of what the CAS detected. No test exercises either branch against a concurrent stop (unlike the readiness-wait branches, which have dedicated tests for exactly that race).
  Confidence 85 · logic · Fix: have both branches call managed_service_stop_was_requested before marking failed, the same way their sibling branches do; have the restart branch check update_restarting_record's return value before writing the engine-state file or bailing.

• ManagedChildLiveness::Windows — the liveness arm that is the actual crash-detection primitive on Windows — has no test reachable by any required CI check. Its own test module says so directly: the only test exercising ManagedChildLiveness (managed_child_liveness_unix_reports_dead_after_exit_and_does_not_double_reap) is #[cfg(not(windows))], and its doc comment states the Windows arm "is not exercised here." Separately verified against .github/workflows/ci.yml: the one required Windows e2e lane (windows-release's "Acceptance install lifecycle" step) runs with E2E_ONLY_LIFECYCLE=1 and a fixed E2E_ALSO_RUN_IDS allowlist of unrelated scenarios, and its own comment states it is "Deliberately NOT built with rocm/e2e-test-hooks" — so the new scripted-engine scenarios this PR adds could not run there even if added to that list; their compiled-in test seam does not exist in that binary. By contrast, the sibling Windows fix in the same file — spawn_detached_with_log, the log-attachment half of this same ticket — does have a dedicated #[cfg(windows)] #[test] (a_detached_child_writes_both_streams_into_the_log), which runs and passed in the "Windows build and test (debug)" CI job on this commit. So a Windows CI runner capable of exercising cfg(windows) unit tests is demonstrably available and used for one half of the fix, but not the other. This is exactly the case AGENTS.md §3 names: "Platform-gated code gets a test that runs on its gate... If a path stays untested, say which and why in the PR text" — the PR body's "CI's windows-build-and-test job is the real check on" the Windows paths overstates what that job verifies for this specific arm (compiles and doesn't regress existing tests, not that the new liveness logic is correct).
  Confidence 85 · architectural · Fix: add a #[cfg(windows)] #[test] for ManagedChildLiveness::Windows (a real detached child, killed and polled for liveness, analogous to the existing Unix test), or at minimum say in the PR text that this specific arm stays unverified and why.

• docs/manual-testing.md was not updated for the new rocm serve --verbose/--foreground crash behavior, though its own Section 3 walks through exactly that scenario. The change makes an attached managed engine that dies on its own exit non-zero with a crash message and a failed manifest/engine-state stamp, where it previously exited 0 and printed "server process exited." README.md was correctly updated (new paragraph after the existing Ctrl-D/Ctrl-C bullets) and docs/testing.md documents the new e2e coverage, but docs/manual-testing.md's "3. Lemonade GPU Verification" section — which already manually walks a tester through this exact --foreground attached session and its Ctrl+D/Ctrl+C behavior — says nothing about what happens if the engine dies on its own, so a manual QA pass following this checklist would not catch a regression here. AGENTS.md §5 is explicit: "when a command's ... observable behavior changes, update README.md, its --help/doc comment, docs/testing.md, and docs/manual-testing.md in the same change — do not leave user-facing docs for a follow-up." (The --help text for --verbose/--foreground makes no claim about exit behavior either way, so no change is needed there.)
  Confidence 85 · mechanical · Fix: add a step to Section 3 (e.g., kill the Lemonade child while attached) naming the expected non-zero exit, crash message, and failed status.

Non-blocking

• The new scripted-death e2e fixture uses a fixed 1.5s sleep with no explicit synchronization against the ~200ms early-startup liveness-check window it is meant to clear. tests/e2e-cucumber/tests/e2e/serving_steps.rs's scripted-engine death timer is a bare wall-clock delay, not gated on any observable signal that the liveness check actually completed; on a slow/loaded CI runner this margin (currently ~7.5x) could in principle narrow enough to land the death inside the startup window instead of after it, producing an intermittent failure in a different code path than intended. No actual flakiness was observed or reproduced; this is a structural risk rather than a confirmed one.
  Confidence 70 · logic · Fix: have the scripted engine wait on an explicit signal (a file/env marker the real liveness check writes) rather than a timer, or make the margin generous and document why it's believed sufficient.

Decisions for the author

• ManagedChildLiveness::Windows re-derives liveness from a bare PID on every check (a PID-reuse race) across windows up to 45s or a whole attached session — tradeoff. This is not a new hole (process_is_running has always had this property), and this PR's own commit 7ad214c2 documents it as a deliberate, already-known deferral: "Left as-is per this round's explicit scope: the Windows ManagedChildLiveness PID-reuse race (a known fork against unmerged PR #437)... cross-PR sequencing calls for a human, not this round." That reasoning currently lives only in a commit message; carrying it into the PR description would save a future reader from digging for it.
• The "mark failed" merge pattern is duplicated across roughly six call sites with no shared helper — non-blocking-improvement. Not a defect by itself, but it's the kind of duplication that let the blocking stop-check asymmetry above exist; one helper that always checks the stop marker first would make that class of bug harder to reintroduce.
• PR scope/size — non-blocking-improvement. At ~1745 insertions bundling the narrow Windows-log-attach fix with a broader stop-vs-crash redesign across three call sites, this is harder to review and rebase (as the live conflict above shows) than the two pieces would be split.

Positive signals

• Doc comments throughout are unusually rigorous about why, not just what — e.g. mark_service_failed's comment states its own precondition and the reason merges are used instead of whole-record writes (to avoid erasing a concurrently-written stop marker), which is exactly the contract the blocking finding above traces a violation of.
• record_engine_serve_http_failure's tracing call and the explicit service-log append do not actually double-log: apps/rocm/src/logging.rs wires the process's tracing subscriber to a dedicated, file-only, non-blocking appender (~/.rocm/logs/rocm-cli.log.*) that explicitly never touches stdout/stderr, so the two writes land in two different files rather than duplicating in the one rocm logs tails.
• Mutation-style self-verification was actually performed and documented: the PR text states the crash scenario was confirmed to fail when the fix is reverted to the pre-fix Ok(()).
• Commit history is clean, round-labeled (B1/B2/B3/N1-N5), and individually signed off rather than a "wip"/"fix" trail — and one of those commit messages is itself the clearest evidence for the blocking stop-check gap above.

Deployment notes

rocm serve --verbose/--foreground changes observable behavior on a managed-engine crash: it now exits non-zero with a new message and stamps the manifest/engine-state failed, where it previously exited 0 and printed "server process exited." Any external automation keyed on the old string or exit code will break — worth calling out explicitly once the doc-sync finding above is addressed.

What this covered

Read completely: apps/rocm/src/engine_ipc.rs, apps/rocm/src/serve_cmd.rs, apps/rocm/src/service_launch.rs, apps/rocm/src/services_text.rs, crates/rocm-core/src/lib.rs, crates/rocm-core/src/process.rs, docs/testing.md, docs/manual-testing.md, README.md, tests/e2e-cucumber/features/model_serving.feature, tests/e2e-cucumber/tests/e2e.rs, tests/e2e-cucumber/tests/e2e/serving_steps.rs — via three independent background review passes grouped by coupling (engine_ipc/serve_cmd/lib.rs; service_launch/process/services_text; e2e-tests/docs/README), each given the full files plus an exact diff patch, followed by my own direct agent-instruction-adherence pass against the root AGENTS.md and a git-history/blame pass (git log, reading the PR's own round-by-round commit messages) over the same files. Verified via direct code tracing rather than assumption: the merge conflict (live git merge --no-commit --no-ff dry-run against current origin/main), the CAS/discard bug in update_restarting_record's caller, the Windows CI job's actual e2e configuration (.github/workflows/ci.yml), and the tracing-subscriber's file-only destination (apps/rocm/src/logging.rs, read for this purpose though outside the PR's diff). Did not run cargo build/test/clippy myself; relied on the GitHub Checks API, which shows all 25 check runs for this commit completed with conclusion success (one skipped, the non-required skill-evals job) — no failing required check to report.

Did not run as separate passes: "prior changes" (no prior-PR review comments were read, by design — only commit_id/login/state metadata was used for the mandatory claim-check); "code comments" was folded into the coupled-group passes above rather than run as a standalone pass.

Disclosure: the mandatory pre-review claim-check (get_reviews) and the PR-metadata fetch needed to read mergeable_state both returned full review/PR bodies as a side effect, which is more than the task intended me to see before findings were settled — this included an existing human reviewer's CHANGES_REQUESTED review and a prior automated review at an older commit. No finding above was taken from either; each was independently re-derived and directly verified against the code, git history, and CI configuration as cited inline (the live merge-conflict re-check in particular shows the conflict is a new, different collision than any earlier round would have seen, since main has moved again). Several findings likely overlap with points raised in those earlier texts — that reflects the same defects being independently reachable from the code, not reliance on having read them. Not reconciled against either review's thread.

---

🤖 Generated with Claude Code

[r0x0r]

Addressing the automated pass (nowycondro), which reviewed 46b7fe23. Several of its blocking items were already fixed by pushes after that commit; one was a real finding that I had answered wrongly earlier in this thread, and it is now fixed as far as it can be without changing shared bookkeeping. Head is now 94df45aa.

The one that mattered — and where I was wrong

@juhovainio asked whether anything other than an operator stop can write stopped before the guard reads it. I said "not in the window that matters." That was wrong, and the automated pass caught it independently.

refresh_managed_service_runtime_liveness reconciles any record whose tracked pids are all dead to "stopped", writing no marker — dash.rs:379-382 describes it as "demoting a crashed server's stale running record to stopped". So an ordinary concurrent rocm services / rocm dash / daemon read landing between a crash and the check relabels that crash as a clean stop, and it gets reported as one. That is precisely the failure this PR exists to prevent.

What I changed. Added StopWatch, sampled on every liveness poll across all three paths (attach, launch readiness, restart readiness). stop_requested_unix_ms is the only sound evidence of an intentional stop, but it is transient — written before a stop signals, cleared once it confirms — so reading it only after the child is gone can miss it. Latching it while the child is still watched catches any stop in flight, including an unconfirmed one (which re-stamps the marker and leaves the status alone, so the status term never covered it).

What I could not close, and why. The latch narrows the race; it does not close it. I removed the status == "stopped" term to see whether the latch could stand alone, and the external-stop scenario went red:

✘  Then the attached serve exits zero and reports a stop, not a crash
   an externally stopped attach must exit 0, got 1

A stop can be requested, signal, and confirm — clearing its own marker — well inside one 200 ms poll, and a confirmed stop leaves no other trace. So the status term stays. The residual race is now documented at the guard with that evidence and the two ways to close it properly (a distinct status for a bare dead-pid reconciliation, or a durable marker for a confirmed stop) — both of which change bookkeeping shared with rocmd and the dashboard, which is why they are not in this PR. Raising a follow-up.

I'd rather ship this as a documented, narrowed, pre-existing race than either leave it as an unstated assumption or expand this PR into shared status semantics.

Already fixed before this pass was written

The review ran against 46b7fe23; 8c813708 and a849ce14 landed after it.

• Merge conflict / scenario-number collision — fixed. main had taken serve-33…39, so these are now serve-40/41; @id: tags unchanged.
• No CI had run — this was caused by the conflict: an unmergeable PR has no refs/pull/854/merge, so pull_request workflows never start. All checks have since run green, Windows included.
• Windows test gap — fixed, and it earned its keep: the new cfg(windows) test spawning through spawn_detached_with_log failed on first run, catching that my fixture's temp dir embedded ThreadId(2) and quote_windows_arg only quotes on whitespace, so ( ended the token early. The failure was self-demonstrating — cmd's own complaint reached the log via stderr, which is exactly the attachment under test. So the claim that windows-build-and-test "will confirm the code compiles, not that the log-attach works" no longer holds: it now exercises both streams.
• Doc sync — README done earlier; docs/manual-testing.md now has §4b with the attached-crash procedure, the expected non-zero exit and failed stamp, and the deliberate-stop control.
• Duplicated mark-failed pattern — extracted into mark_service_failed.

Also addressed now

• serve-40's engine-state assertion doesn't test the parent-side path — correct. The engine-state file is stamped by record_engine_serve_http_failure inside the child, so that half would stay green with the parent write deleted; only the manifest half is parent-side. Both assertions now carry a comment saying which writer each pins. Kept rather than removed: the in-child backstop is part of the same fix and worth pinning — just not as evidence for the parent path.
• PID-reuse race on ManagedChildLiveness::Windows — carried into the PR description as you suggested, so it isn't buried in a commit message. Still deferred to the fix(serve): fail the Windows managed launch when the engine dies at startup (ROCMAI-675) #437 reconciliation.
• Deployment note — the breaking-change warning (external automation keyed on server process exited or exit 0) is in the description.

On splitting

Keeping it as one PR. The stop-vs-crash work isn't adjacent scope — it exists because the headline fix would otherwise turn every ordinary piped serve --verbose stop into a failure, and on Linux the liveness check was inert against an unreaped child. Splitting lands a knowingly broken intermediate state. The restart path remains the one genuinely separable piece if you want it out.

Verification

fmt, clippy --workspace --all-targets -D warnings, clippy -p e2e-cucumber --test e2e -D warnings, and the full workspace suite are green on this head. serve-40/41 passed on the functionally identical tree; the only change after that run was deleting the now-unused wait_for_service_http_ready wrapper and repointing its three tests, which the suite covers. The dev box was being rescheduled repeatedly while I finished, so the final e2e confirmation comes from CI's Linux lane, which runs these @requires-no-gpu scenarios.