feat(vllm): enhance OOM diagnostics and guidance in serve summary (eai-8059)

[r0x0r]

Summary

Adds GPU out-of-memory (OOM) guidance to rocm serve for vLLM: when a managed
launch fails to become ready with a real allocator OOM signature in its own log,
the interactive deployment summary names the memory knobs (--gpu-memory-utilization,
--gpu <index>), says plainly that lowering the reservation does not help a
model that simply does not fit, and routes the user's actual failing line into
rocm diagnose --symptom '<line>'.

Detection is deliberately narrow: the serve summary classifies each tail line with
the same rule as the rocm diagnose vLLM-OOM checker (each line scored as
vllm: <line> against the shared match threshold), so a bare out of memory from
a kernel OOM-killer line, a dependency's log, or vLLM's generic EngineCore
wrapper never gets reported as memory exhaustion. One shared helper
(rocm_core::vllm_log_shows_oom) feeds both the engine hint and the CLI note, so
the two surfaces cannot drift.

Depends on #251 (gpu-out-of-memory). This PR is stacked on it and targets
that branch; it must not merge before #251 does, after which it will be rebased
onto main.

Behavior coverage

Scenarios this PR adds:

User-observable behavior	Scenario (@id:)	Lane
A managed launch that OOMs and never becomes ready names the memory knobs	serve-oom-launch-memory-guidance (serve-25)	GitHub-hosted mock lane, gated @requires-oom-fault-injection
Reusing an already-running service reaches the summary with no OOM note	serve-oom-memory-guidance (serve-24)	GitHub-hosted mock lane, @requires-no-gpu
A live service for a different model does not soften the no-GPU refusal	serve-unrelated-live-service-still-fails-fast (serve-26)	GitHub-hosted mock lane, @requires-no-gpu

@id:diagnose-vllm-oom-is-conditional and @id:serve-vllm-low-vram-oom-guidance
(serve-23) come from the base branch (#251), not from this PR; this PR does
not modify them.

The positive scenario cannot run a real launch on a GPU-less host, so the mock-lane
binary compiles in a test-only fault-injection hook (feature e2e-oom-fault-injection,
armed per child process by ROCM_E2E_SIMULATE_OOM_LAUNCH) that fabricates exactly
the failed-launch state. The hook is absent from shipped binaries (release.yml
never passes the feature), so the scenario reports a skip on the self-hosted
(prebuilt-release) lanes rather than a silent green, and runs where xtask builds
the binary itself.

Verification, and what could not be run

Green at the pushed head on a Linux MI300X box:

• cargo test --workspace --all-targets
• cargo clippy --workspace --all-targets -- -D warnings
• cargo fmt --all --check
• cargo xtask e2e — including this PR's own scenarios: serve-24
  (serve-oom-memory-guidance), serve-25 (serve-oom-launch-memory-guidance)
  and serve-26 (serve-unrelated-live-service-still-fails-fast) all pass, as
  does serve-16 (serve-absent-gpu-index-rejected), the lane
  evidence for the --gpu-ordering fix below.

CI. Every lane is green at this head; there are no failing checks.

Scenario	Cause	Status at 83ecfaad
chat-06	vLLM 400: "auto" tool choice needs --enable-auto-tool-choice	pre-existing
serve-01, serve-02	no resolved model line	fails identically
serve-07, serve-08	lemonade endpoint never came up	fails identically
serve-13	@requires-no-gpu scenario, forced onto a GPU host by the name filter	fails identically
serve-19	belongs to the base branch (#251); refused with "no active ROCm runtime is configured"	fails identically
serve-10, bench-04	pass in isolation at this head; contend on shared model downloads	flake, not a regression
runtime-02	no Folder: line in examine output	pre-existing

Scenario numbers in that table are positional as of 83ecfaad and have
since drifted; serve-19 there is @id:serve-vllm-low-vram-oom-guidance,
which is serve-23 at this head.

E2E tests (Strix Halo, WSL2) is a 24h runner timeout — infrastructure, not an
assertion failure.

Changes since the last review

• --gpu validation ordering (blocking). resolve_gpu_indices had moved
  after the no-runtime bail, so --gpu 99 surfaced as "no active ROCm runtime
  is configured" instead of an out-of-range argument error, and
  serve-absent-gpu-index-rejected failed on the GPU lane. Index validation now
  runs immediately after the no-usable-GPU pre-flight and before any runtime or
  engine work; a GPU-less host still refuses with "no usable AMD GPU" first.
  serve-16 passes on the GPU lane at the reviewed head.

• OOM detector threshold reconciled with rocm diagnose. The serve-summary
  detector used a looser bare-substring scan than feat(vllm): Tackle out of memory errors (EAI-8058) #251's check_16_vllm_oom,
  which deliberately scores a bare out of memory below MIN_SCORE_FOR_MATCH.
  Both now classify with the same rule through a single shared helper. The note
  also carries the shared-vs-dedicated caveat and points at rocm diagnose for
  the case-appropriate fix, and the --gpu-memory-utilization wording is
  corrected to "a fraction greater than 0 and at most 1" (the parser enforces
  (0, 1], so <0-1> wrongly implied 0 was accepted).

• GPU fail-fast contract made true. The comment above the no-usable-GPU bail
  promised no engine work could precede it, while the reuse detection above it
  ran a ResolveModel round-trip and — for a self-managing engine — an
  ensure_self_managed_engine_ready that can print "Preparing lemonade for GPU
  serving..." and install. The pre-gate keyed on the engine alone, so a live
  service for an unrelated model, which this invocation can never reuse, pulled
  that work in front of the bail. It now also keys on the model
  (any_live_managed_service_for_model), using the same lenient name relation the
  service-listing surfaces already use so a short-vs-canonical spelling still
  reaches the probe (which then decides reuse authoritatively on the canonical
  id). The comment now states the real contract instead of an absolute one.
  Regression tests: reuse_pregate_skips_engine_work_for_an_unrelated_live_model,
  reuse_pregate_admits_the_same_model_including_a_short_spelling,
  reuse_pregate_still_keys_on_the_engine.

• Shared log-tail budget actually shared. The serve summary's comment claimed
  it read the same budget as the engine's OOM surface, but
  engines/vllm's STARTUP_FAILURE_LOG_TAIL_LINES was an independent 80
  literal. It is now derived from rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES,
  so a change to the protocol budget moves both surfaces and the claim holds.

• xtask feature collision. cargo xtask e2e builds the mock-lane binary with
  a single --features "rocm/e2e-test-hooks rocm/e2e-oom-fault-injection" value
  instead of overwriting e2e-test-hooks (fix(lemonade): retry interrupted backend setup #249). workflow_contract.rs's
  prebuilt-lane assertion is a contains on --features rocm/e2e-test-hooks and
  still holds; its doc and failure message now describe that string as the shared
  hook floor (xtask builds a superset) rather than an exact match, and record
  why the fault-injection hook is deliberately absent from prebuilt lanes.

• Duplicated env constant. ROCM_E2E_OOM_FAULT_INJECTION was declared twice,
  in xtask/src/e2e.rs and tests/e2e-cucumber/src/capability.rs, held together
  only by a "kept in sync" comment — a typo in either would have silently turned
  the scenario into a skip. Hoisted into the shared e2e-report crate both
  already depend on; the two former copies re-export the single source.

• Negative assertion no longer vacuous. "does not blame this invocation for
  GPU memory" collapses whitespace exactly like the positive checks, so it catches
  a note that is wrongly rendered and soft-wrapped across the 80-column PTY grid
  instead of passing against a literal substring that could never match.

• Scenario numbering. This PR's scenarios are serve-24 / serve-25 /
  serve-26 at this head. They were serve-20 / serve-21 when written; the
  base has since inserted scenarios ahead of them, so the numbers drifted — and
  the base's own low-VRAM scenario is now serve-23. The stable @id: tags are
  unchanged and are what to match on — expect the numbers to drift again before
  this lands.

Coordination

#290's diagnose catalog has merged, so rocm diagnose's vLLM-OOM checker and this
PR's serve-summary detector now share one classification rule rather than being
two independent detectors.

[volen-silo]

A few observations, mostly around the signature list and the stacking:

• vllm_log_shows_oom treats engine core initialization failed as an OOM signature, but vLLM emits that as the terminal wrapper for any EngineCore startup crash (unsupported arch, shm size, TP misconfig, missing weights). Being the last line, it reliably lands in the 80-line tail — so unrelated failures get reported as "the serve attempt ran out of GPU memory".
• The e2e scenario plants exactly that string as its OOM log, and a unit test asserts it must produce a hint — so the only end-to-end coverage never exercises a real OOM message.
• This branch removes the DRM sysfs VRAM fallback, the --gpu auto count derivation, their tests and the docs bullet — all added by its base feat(vllm): Tackle out of memory errors (EAI-8058) #251, which is still open. Intentional? auto_select_gpu_indices' doc still refers to that fallback.
• collect_serve_notes and append_oom_serve_note each emit the full hint verbatim, so a low-VRAM serve that then OOMs prints it twice.
• Setting log_path/manifest_path on the already-running branch makes the OOM note reachable when no launch happened — re-running serve against a starting service attributes that process's OOM to the new invocation.
• feat(diagnose): add the vLLM out-of-memory failure mode to the catalog (EAI-8060) #290 edits the same oom_utilization_hint and still calls log_tail_shows_oom, which this PR deletes.

[r0x0r]

Thanks for the detailed review — addressed all of these in b9316f3 (rebased onto the current gpu-out-of-memory tip):

• Generic "engine core initialization failed" signature: removed it from vllm_log_shows_oom. It really is vLLM's terminal wrapper for any EngineCore startup crash, not just OOM, so treating it as an OOM signature was wrong. Detection now only fires on the allocator-level signatures (torch.OutOfMemoryError, hip out of memory, out of memory).
• E2E coverage only exercising the wrapper string: the e2e scenario now plants a real allocator OOM signature (torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.) instead of the generic wrapper phrase, and unit tests were updated the same way.
• DRM sysfs fallback / --gpu auto count derivation removed: that was purely a stale-base artifact — this branch was still on top of an older point in gpu-out-of-memory than your feat(vllm): Tackle out of memory errors (EAI-8058) #251 fix commit. Rebased onto the current tip (a97a39f) and restored the gpu_vram_usage/gpu_vram_usage_amd_smi/gpu_vram_usage_sysfs split that got flattened by the rebase's merge resolution — verified with cargo test/clippy that the dispatcher and DRM fallback tests are back and passing.
• Duplicate hint when a low-VRAM serve then OOMs: append_oom_serve_note now skips adding the note if the shared VLLM_GPU_MEMORY_UTILIZATION_HINT text is already present in the notes collected pre-launch, so it's never printed twice.
• log_path/manifest_path on the already-running branch misattributing OOM: append_oom_serve_note now also takes already_running and bails out immediately when true, so re-running serve against a live/starting service never blames that invocation for whatever the other process's log contains. Added a regression test for this (append_oom_serve_note_ignores_an_already_running_services_log) and repointed the e2e scenario at this exact case (serve-oom-memory-guidance), asserting the summary does not show the OOM note when reusing an already-running service.
• feat(diagnose): add the vLLM out-of-memory failure mode to the catalog (EAI-8060) #290 touching the same oom_utilization_hint/log_tail_shows_oom: noted for awareness — nothing in this repo currently defines log_tail_shows_oom (this PR's function is vllm_log_shows_oom), so there's no conflict today, but that PR will need to reconcile with whichever version of this lands first.

All changes covered by unit tests; cargo test -p rocm --bin rocm, cargo clippy -p rocm -p rocm-core -p rocm-engine-vllm --all-targets -- -D warnings, and cargo fmt --check all pass locally.

[volen-silo]

Round 2. Items 1, 4 and 5 are genuinely fixed — I checked the mechanisms, not just the claims. The dedup works (VLLM_GPU_MEMORY_UTILIZATION_HINT collapses to one line, notes holds raw text at the dedup point, render_summary does no wrapping, so contains really fires), and the already_running bail-out loses nothing (ManagedLaunchReport.already_running has exactly two producers, start_managed_service's final Ok is always false, and load_managed_services refreshes liveness before the check).

Blocking:

• The note fires on status == "running", which the codebase documents as healthy. Both append_oom_serve_note and oom_memory_note gate on status == "ready", but status_for_readiness produces three values and the comment four lines above it describes "running" as "the engine is up and loading normally" — deliberately distinguished from "starting" so rocmd doesn't kill a slow-loading model. So a serve whose endpoint is up and whose model is still loading, with out of memory anywhere in its last 80 log lines, is told "the serve attempt ran out of GPU memory." Both rustdocs claim they only fire for a serve that "failed to become ready" — the code contradicts its own contract. Gate on "starting" or an explicit failure predicate, not != "ready".

• The positive e2e case now has no scenario at all. @id:serve-oom-memory-guidance is the only scenario touching OOM guidance in the whole feature directory, and it now asserts the note is absent. Round 1's point was that the positive scenario planted a fake OOM string; repointing it at an unrelated negative case rather than repairing it leaves the PR's headline user-visible behavior unit-tested only. AGENTS.md §3: "User-observable behavior needs a scenario, not only a unit test... a unit test asserting the internal helper does NOT discharge this." Keep the negative, add a positive.

• Round-1 item 3 is only partly fixed. The production code is byte-identical to a97a39f0 — gpu_vram_usage/_amd_smi/_sysfs, effective_gpu_count, validate_pinned_gpu_index, auto_select_gpu_indices, and that last one's doc comment does match its code. But auto_selection_uses_vram_row_count_when_amd_smi_count_is_unknown is still deleted with no replacement — it's the only test removed anywhere in this PR, and effective_gpu_count still falls back to vram.map(<[GpuVramUsage]>::len), so it's a live path with no selection-level coverage for detected: None + populated vram. Separately, the new comment at main.rs:16882 ("No GPU count from amd-smi (unavailable, or genuinely zero devices)") contradicts the unchanged comment eight lines above it in the same function, which explains that vram may be populated precisely when amd-smi is absent. Reaching count == 0 needs both sources to fail. Revert the comment edit and restore the test.

• docs/vllm.md:123 says "two ways", the list below it has three items and is byte-identical to the base. Residue of the reverted-then-restored sysfs bullet — the bullet came back, the count word didn't.

• Dead signature entry. "hip out of memory" contains "out of memory", so .any() can never reach it. Cleanest fix that also un-narrows detection: ["outofmemory", "out of memory"]. The old log_tail_shows_oom matched bare "outofmemory"; replacing that with "torch.outofmemoryerror" means torch.cuda.OutOfMemoryError (the .cuda. breaks contiguity) and hipErrorOutOfMemory no longer match on their own. Usually the spaced phrase is also in the tail so impact is limited, but neither commit message mentions the narrowing.

• The spawn_managed_engine_child hunk is now inert. log_path/manifest_path on the already-running branch has zero reachable consumer: the three reads in the tree are append_oom_serve_note (bails on already_running) and print_managed_launch_plain twice (returns before reaching them). The danger round 1 flagged was fixed elsewhere; the now-purposeless hunk was left in.

• @requires-gpu puts the new scenario on a lane that gates nothing. ci.yml:675 skips those on the blocking mock job and the self-hosted GPU lane is continue-on-error. It isn't gratuitous as written — main.rs:4857 bails GPU-required serves before the already-running short-circuit — but that check is skipped for --device cpu_only, and existing_live_managed_service matches on (engine, canonical_model_id) only, so device policy is irrelevant to the reuse this scenario depends on. --device cpu_only + @requires-no-gpu (as scenario 11 already does) would put it on the blocking lane. The step's own comment flags the mismatch: "allocates no GPU memory despite running on the GPU lane."

On #290: the claim that nothing defines log_tail_shows_oom is wrong — git show origin/oom-diagnostics-catalog:engines/vllm/src/lib.rs still defines it at 1846, calls it at 1832 and asserts on it at 2252. The conclusion holds anyway: merge-tree is clean, #290 edits only the format string two lines below the if. The real coordination issue is that once both land there are two independent OOM detectors — rocm_core::vllm_log_shows_oom and #290's KEYWORDS_VLLM_OOM/check_16_vllm_oom table. Whichever merges second should fold the checker onto the shared helper; worth saying so in the body now.

Smaller things:

• vllm_log_shows_oom lives in rocm-core but has zero tests there — near-identical assertions now sit in serve_summary.rs:510 and engines/vllm/src/lib.rs:2227. cargo test -p rocm-core alone covers none of it.
• assert_no_oom_memory_guidance also asserts screen.contains("already running"), which the Gherkin doesn't state. Good guard against a vacuous negative, but it belongs in the feature file as its own And.
• read_optional_tail_lines runs before the already_hinted check — swap them.
• oom_memory_note is a policy function with no rendering, in serve_summary.rs, while its sibling collect_serve_notes is in main.rs, stitched together three lines apart at the call site.

Checked and clean: log lifecycle (File::create truncates per launch, service_id is timestamp-unique, so no stale OOM can be attributed), the e2e plumbing is real rather than vacuous, no orphaned steps, no temp-file collision.

Centralising the signature into rocm_core::vllm_log_shows_oom so the engine error and the CLI summary can't drift is the right call — it's what makes #290's duplicate detector visible as a problem.

[r0x0r]

Thanks for the Round-2 review — pushed 1adc674 addressing it.

Note could fire for a healthy running (still-loading) service. Fixed. Introduced serve_summary::serve_failed_to_become_ready(status) (status == "starting") and gated both append_oom_serve_note and oom_memory_note on it, instead of status != "ready". A healthy running service is no longer treated as a failed launch. New test: append_oom_serve_note_is_withheld_for_a_healthy_still_loading_service.

Dead signature / narrowing regression. vllm_log_shows_oom signatures are now ["outofmemory", "out of memory"]. The old "hip out of memory" was dead (any log with it already contains "out of memory"), and the space-free "outofmemory" matches the allocator error types whatever module path they carry (torch.OutOfMemoryError, torch.cuda.OutOfMemoryError, hipErrorOutOfMemory). Added rocm-core coverage: vllm_log_shows_oom_matches_allocator_signatures_but_not_generic_failures (also asserts the generic EngineCore wrapper is not matched).

Inert spawn_managed_engine_child hunk / deleted test / misleading comment. Reverted the already-running branch's log_path/manifest_path change back to None, reverted the select_auto_gpu_index comment churn, and restored auto_selection_uses_vram_row_count_when_amd_smi_count_is_unknown.

docs/vllm.md said "two ways" but the list has three. Fixed to "three ways".

The @requires-gpu reuse scenario gated nothing. Moved @id:serve-oom-memory-guidance onto the blocking no-GPU mock lane: @requires-no-gpu @requires-os:linux, with --device cpu_only so the GPU-required pre-flight is skipped and the already-running reuse short-circuit is reached. It now gates every PR and allocates no GPU memory. Also split the "already running" assertion into its own explicit Gherkin Then step.

Positive e2e scenario — not added, and here's why. The negative case (reuse must not blame this invocation) is the one the harness can drive without a GPU, because it reuses a pre-planted live service rather than launching. A positive end-to-end (this invocation launches, the launch OOMs, the note renders) needs the mock engine to emit a real allocator OOM signature in a log it owns (already_running == false, status == "starting"), which the mock can't do today. The positive behavior is currently covered by unit tests (append_oom_serve_note_reads_the_failed_serve_log_and_names_the_knobs and serve_summary::oom_note_*). If you'd like it in e2e, I'll add a mock hook to inject a failed-launch OOM log and verify it on the Linux lane as a follow-up — flagging the gap rather than shipping an unverifiable scenario.

Smaller items: added the missing rocm-core test for vllm_log_shows_oom; reordered append_oom_serve_note so the cheap already_hinted check runs before the log read; the "already running" assertion is now explicit in the feature file.

Coordination with #290: once both land there are two OOM detectors (vllm_log_shows_oom here and the diagnose catalog there) — happy to fold them onto a shared signature source in a follow-up if you'd prefer.

[r0x0r]

@volen-silo — correction on the scenario-15 lane, because my Round-2 reply was wrong and CI called it out.

--device cpu_only does not skip the GPU pre-flight — it's rejected before either branch is reached. parse_device_policy refuses cpu/cpu_only outright under the strict no-CPU-fallback policy ("rocm serve requires ROCm GPU execution; CPU mode is not a fallback path"), so serve() bails at argument parsing, before both the GPU-required pre-flight and the already-running reuse short-circuit. That's exactly why @id:serve-oom-memory-guidance went red on the no-GPU lane in 1adc674. Both my earlier claim and the "cpu_only skips the pre-flight" assumption were incorrect.

Proper fix (c0f39cf): reuse detection now runs before the GPU pre-flight, at the correct layer. Reusing an already-running managed service launches nothing and pins no GPU, so it should never be blocked by the GPU-required check. I moved model resolution + reuse detection ahead of that bail, gated by a cheap any_live_managed_service_for_engine pre-check so the common launch path still fails fast before any engine work. When a live managed service for the engine already exists and the model ref resolves to it (existing_live_managed_service match), the GPU bail is skipped; otherwise everything is byte-identical — including scenario 11's no-GPU/no-runtime refusal, which still fires because no live service exists.

Scenario 15 now runs on the blocking lane with the default device — @requires-no-gpu @requires-os:linux, no --device flag. It reaches the reuse short-circuit on the no-GPU mock host purely because the planted live service is reused, gates every PR, and allocates no GPU memory. Feature/step comments updated to match.

Follow-up 395ed9e is a fixup! (clippy map_unwrap_or → is_ok_and on the new helper); it'll autosquash into c0f39cf on the final rebase.

Note: the current red E2E tests (GPU) lane is unrelated — 9 scenarios fail identically with vLLM RuntimeError: Failed to infer device type (the runner's HIP device wasn't visible), spanning serve/chat/benchmark/lemonade cases this change doesn't touch; the prior commit's GPU lane was green. Re-running it.

Your other two Round-2 items (restore the deleted auto_selection_uses_vram_row_count_... test + revert the main.rs:16882 comment; positive-launch e2e scenario) are tracked separately and not part of this lane fix.

[volen-silo]

Round 3. I re-verified every Round-2 item against the code at 395ed9e, not against the claims. Genuinely fixed: the serve_failed_to_become_ready gate (both call sites), the restored auto_selection_uses_vram_row_count_... test, the reverted select_auto_gpu_index comment (now byte-identical to base), docs/vllm.md "three ways", the ["outofmemory", "out of memory"] signature pair, the reverted spawn_managed_engine_child hunk, and the lane move. cargo fmt --check, clippy --workspace --all-targets -D warnings and cargo test --workspace --all-targets are green locally at this head, and the blocking GitHub-hosted mock lane runs @id:serve-oom-memory-guidance and passes it. The two red checks (self-hosted GPU, readthedocs) fail identically on unrelated recently-merged PRs.

I also checked the new gate the other way, for over-narrowing: starting really is the only failure value that can reach append_oom_serve_note. start_managed_service always derives report.status from status_for_readiness, and its spawn-failure path returns Err before a report exists; the "failed"/"stopped" literals elsewhere belong to services restart/stop and never feed this. Gate is correct.

Blocking

The positive e2e scenario is still missing, and the infeasibility argument only covers half of it.

You are right about the post-failure path. append_oom_serve_note requires already_running == false; parse_device_policy rejects cpu_only outright; and the only other way past the GPU pre-flight is the reuse short-circuit, which forces already_running == true. There is no mock-lane route to that half — it needs a harness change, and I am not asking for one here.

The pre-launch half is coverable today, with no new harness capability:

• collect_serve_notes (apps/rocm/src/main.rs:5100) has no already_running gate — it runs on the exact reuse path scenario 15 already drives on the blocking no-GPU lane, and serve_gpu_low_memory_warning -> VLLM_GPU_MEMORY_UTILIZATION_HINT (main.rs:5190-5197) is reachable from there.
• The only missing ingredient is VRAM telemetry, and rocm_core::resolve_amd_smi_binary (crates/rocm-core/src/lib.rs:7301) checks $HOME/.rocm/runtimes/registry before PATH — and HOME is already isolated per scenario by the harness (tests/e2e-cucumber/tests/e2e.rs:259).
• So: plant a registry manifest plus a stub amd-smi emitting the {"gpu_data":[{"gpu":0,"mem_usage":{...}}]} shape parse_gpu_vram_usage accepts (main.rs:17128), and the hint renders into the same captured summary scenario 15 already asserts against. vram_capacity_is_meaningful (main.rs:17202) does not block it — it only suppresses on a single-GPU APU.

That is a fixture, not a new primitive. AGENTS.md §3 is explicit that a unit test does not discharge user-observable behavior, and its escape hatch — "state the gap in PR text and name the lane" — is also unmet: the explanation lives in a PR comment, while §2 treats bodies and comments as distinct surfaces. Minimum to unblock: put the post-failure gap and its lane in the PR body. Preferred: also add the pre-launch scenario, which then makes the body statement scoped to the one path that genuinely cannot run.

Nits (non-blocking)

• any_live_managed_service_for_engine (main.rs:14815) pre-gates on engine only, so when a live service exists for a different model of the same engine the probe runs and reuse_existing still ends up false — and the GPU fail-fast fires afterwards, weakening the invariant its own comment states ("BEFORE preparing or launching any engine"). Harmless for vLLM (ResolveModel is a pure in-process echo, and the "no usable AMD GPU" message is preserved), but for a self-managing engine ensure_self_managed_engine_ready can run a real install first when the pinned version is stale. ManagedServiceRecord already stores the raw model_ref, so the pre-gate could compare that and skip the probe entirely.
• Same block re-reads load_managed_services up to three times per serve (pre-gate, existing_live_managed_service, then again inside spawn_managed_engine_child), each with the 750 ms liveness probe budget.
• Widening to bare outofmemory reopens one plausible false positive: a torch/vLLM import-time crash whose traceback frame is from torch.cuda import OutOfMemoryError (missing libamdhip64.so, arch mismatch) lands in the tail with status == "starting", so it gets the memory-knob hint for a non-memory failure. Only a misleading hint, so not blocking — but worth a line in the doc comment.
• oom_memory_note is policy with no rendering, sitting in serve_summary.rs while its sibling collect_serve_notes is in main.rs; the two are stitched together three lines apart at the call site.
• The two-independent-OOM-detectors coordination point with the diagnose-catalog PR is still only in a comment thread; worth a line in the body so whichever lands second knows to fold onto the shared helper.

Everything else I checked came back clean: the shared-constant centralisation means the engine hint and the CLI note cannot drift, render_summary emits notes unwrapped so the dedup contains check really fires, the split Gherkin step is non-vacuous, and no doc or comment outside the diff still describes the old status != "ready" gating or the old signature list.

[r0x0r]

Thanks for the Round-3 review — and for re-verifying the Round-2 items against the code rather than the claims.

Blocking item — the positive scenario. You're right on both halves, so I've split them explicitly in the PR body (new "e2e coverage and the scenario gap" section), per §3's escape hatch and §2's body-vs-comment distinction:

• Post-failure hint (append_oom_serve_note): we agree this is genuinely unreachable on any current lane — already_running == false is required, parse_device_policy rejects cpu_only, and the reuse short-circuit forces already_running == true. It needs a harness capability that doesn't exist yet (a mock that clears the GPU pre-flight and reaches the post-launch summary), so it stays unit-covered; the gap + "no lane yet" is now stated in the body.
• Pre-launch hint (collect_serve_notes → serve_gpu_low_memory_warning → VLLM_GPU_MEMORY_UTILIZATION_HINT): your recipe is correct — the HOME-isolated runtimes/registry is resolved before PATH, so a stub amd-smi reporting a busy GPU makes the hint render into the same captured summary Scenario 15 asserts on, all on the @requires-no-gpu mock lane. The one thing it needs is a new harness step-def (a low-VRAM stub-amd-smi fixture); there's no existing step to reuse. I've deliberately not pushed that unverified: the e2e cucumber harness can't be built on my macOS box (engines/lemonade has an st_mode u16/u32 libc mismatch, and macOS is unsupported per §6), so a new step-def would go in blind and iterate through red CI. I've recorded it as a tracked follow-up in the body and I'm glad to land it as a dedicated, CI-verified change — say the word if you'd rather it block here and I'll add the fixture + scenario and drive it green on the Linux lanes.

Nits. Noted and deferred as non-blocking: the any_live_managed_service_for_engine engine-only pre-gate (harmless for vLLM's pure-echo ResolveModel, matters only for a self-managing engine's stale-version install), the repeated load_managed_services reads, the bare-outofmemory import-time-crash false positive (a misleading-but-additive hint), and the oom_memory_note policy/rendering split. The diagnose-catalog coordination point is now concrete — #290 merged, so whichever of the two OOM detectors lands here should fold onto the shared helper; I'll note that on the follow-up.

Two red checks (self-hosted GPU, readthedocs) fail identically on unrelated recently-merged PRs; the blocking GitHub-hosted mock lane runs and passes @id:serve-oom-memory-guidance.

[rominf]

I read through this PR's diff (base gpu-out-of-memory vs the current head), plus the surrounding code it touches in apps/rocm/src/main.rs, apps/rocm/src/serve_summary.rs, crates/rocm-core/src/lib.rs, and engines/vllm/src/lib.rs.

The change is well-scoped and I didn't find anything blocking:

• The new vllm_log_shows_oom signature detection is shared correctly between the CLI (append_oom_serve_note/oom_memory_note) and the vLLM engine's own startup-log summary (oom_utilization_hint), so the two surfaces can't drift in wording or matching logic. The exclusion of vLLM's generic "engine core initialization failed" wrapper as an OOM signature is deliberate and well-tested (it's the terminal line for any startup crash, not just OOM).
• append_oom_serve_note is correctly gated: vLLM only, real launch failure only (status == "starting", not the healthy still-loading running state), and already_running excluded so a reused service's log is never misattributed to the current invocation. The dedup check against the already-present hint text also works correctly against how collect_serve_notes stores it.
• The reordering that moves model resolution/reuse detection ahead of the GPU pre-flight bail is a real, deliberate fix (per the commit history) for a genuine ordering bug: reusing an already-running managed service pins no GPU and shouldn't be blocked by the GPU-required check. The any_live_managed_service_for_engine pre-gate keeps the common (non-reuse) launch path failing fast as before, and the non-reuse behavior (no live service, no runtime configured, etc.) is unchanged.
• Test coverage is solid: unit tests for the OOM signature matcher, the note builder in both withheld/fired cases, and a new Gherkin scenario (serve-oom-memory-guidance) that runs on the no-GPU mock lane and asserts a reused already-running service is never blamed for another process's OOM.
• No internal/company-only text made it into the diff.

This looks safe to approve after the usual final checks — passing CI and a maintainer's own pass — since this is a diff-only read and doesn't replace running the test suite or verifying against real hardware.

[r0x0r]

Rebased onto the updated gpu-out-of-memory base (now at e94ca5f) and force-pushed (e8e19e8).

One semantic merge decision worth flagging: the base branch had independently evolved oom_utilization_hint to route the user's actual failing line into a rocm diagnose --symptom 'vllm: …' example. The rebase converges that with this PR's shared detector, so the engine hint now uses rocm_core::vllm_log_shows_oom (shared with the CLI's serve summary) and keeps the base's --symptom routing. The local log_tail_shows_oom the base added is gone in favor of the shared function, and the engine tests assert both the shared-detection and the --symptom output.

The rest of the series replayed unchanged; cargo test --workspace and cargo clippy --workspace --all-targets -- -D warnings are green. This stays stacked on #251, so it depends on that landing first.

[rominf]

Re-reviewed the update pushed since the last review (commit e8e19e8a, rebased onto the current gpu-out-of-memory tip and force-pushed). This is a diff-only read of the new commit, e2e: cover positive OOM-launch memory guidance via feature-gated fault injection.

What's new since the last pass: it adds the positive e2e scenario the earlier review rounds flagged as missing (Scenario 16, serve-oom-launch-memory-guidance) — a fresh vLLM launch that runs out of GPU memory and whose interactive summary must blame this launch and name the --gpu-memory-utilization knob. Since a real OOM can't be produced deterministically on the no-GPU mock lane (the GPU pre-flight bails before a launch even starts) and a live-GPU OOM would be flaky, it introduces a compile-gated fault-injection seam: a new Cargo feature e2e-oom-fault-injection on the rocm package, off by default, enabled only by xtask e2e's mock-lane build (verified release.yml does not pass this feature, so shipped binaries never carry it). When armed per-child via ROCM_E2E_SIMULATE_OOM_LAUNCH=1, start_managed_service short-circuits to a helper that fabricates a starting, not-already_running managed-service record with a real allocator OOM signature in its own log, driving the exact append_oom_serve_note path the earlier unit tests already covered only in isolation.

I checked:

• The feature is correctly scoped (rocm/e2e-oom-fault-injection, not applied to rocmd) and compiled out of release builds.
• The fault-injection bypass is placed correctly relative to the GPU-required pre-flight (!e2e_simulate_oom_launch()), consistent with how the existing reuse_existing bypass works, and doesn't affect any non-e2e-feature build.
• The fabricated state (status: "starting", already_running: false, log carrying torch.OutOfMemoryError: HIP out of memory...) lines up exactly with the gating conditions in append_oom_serve_note/oom_memory_note fixed in earlier rounds, so the new scenario genuinely exercises the intended code path rather than a shortcut.
• --env-id on the new scenario resolves directly from the CLI flag (resolve_engine_selection's cli_env_id branch) with no on-disk manifest dependency, so it isn't fragile.
• The new TuiSession::spawn_with_env/spawn_binary_with_env helpers apply the env var per child process only (never a global set_var), which is the right way to keep this safe under concurrent no-GPU scenarios.
• The two OOM e2e scenarios use distinct model ids so their planted service records can't collide.

No findings from this pass. This looks safe to approve after the usual final checks — passing CI and a maintainer's own pass — since this is a diff-only read and doesn't replace running the test suite or verifying on real hardware.

[r0x0r]

Pushed `5bd1bfc`: gated the `serve-oom-launch-memory-guidance` scenario on a new `@requires-oom-fault-injection` capability so the WSL2 self-hosted E2E lane skips it instead of failing.

Why the lane failed: that scenario drives the test-only `e2e-oom-fault-injection` hook to fabricate a GPU-less OOM launch. The hook is compiled out of the shipping release binary. The self-hosted lanes test a prebuilt `ROCM_CLI_BINARY`, so xtask e2e skips its own feature build and the real GPU-required `serve` pre-flight ran instead — on WSL2 (no usable GPU) that bailed with "no usable AMD GPU detected" and the scenario exited 1. Only the GitHub-hosted mock lane, where `xtask` builds the binary itself with the feature, could ever pass it. This was a pre-existing design gap, not a rebase regression.

The gate: `xtask e2e` now sets `ROCM_E2E_OOM_FAULT_INJECTION=1` only when it built the binary with the feature; the harness probe reads it into `HostCapability.oom_fault_injection`, and `resolve()` skips the tagged scenario when the hook is absent. This preserves the mock-lane coverage that gates every PR and cleanly skips the feature-less self-hosted lanes. Added unit tests for the resolver gate (both directions) and the xtask env signal.

[rominf]

Re-reviewed the update pushed since the last review (commit 5bd1bfc, on top of e8e19e8a). This is a diff-only read of the delta plus a fresh full pass over the whole PR diff against gpu-out-of-memory.

What's new: the author gated the positive serve-oom-launch-memory-guidance scenario (added in the prior push) behind a new @requires-oom-fault-injection tag. That scenario drives the test-only e2e-oom-fault-injection hook, which is compiled out of the shipping release binary. The self-hosted lanes test a prebuilt ROCM_CLI_BINARY, so xtask e2e there skips its own feature build and the scenario was hitting the real GPU-required pre-flight instead of the fault-injection path — which correctly bailed with "no usable AMD GPU detected" on WSL2 and failed the scenario. That's a pre-existing lane-coverage gap surfacing when the positive scenario landed, not a rebase regression.

The fix: xtask e2e now sets ROCM_E2E_OOM_FAULT_INJECTION=1 only when it built the binary itself (with the feature); a prebuilt binary path clears/never sets it. The harness's HostCapability picks that signal up as oom_fault_injection, and resolve() skips @requires-oom-fault-injection scenarios when it's false. I checked:

• The env var name is kept in sync between xtask::e2e and e2e_cucumber::capability (both define the same ROCM_E2E_OOM_FAULT_INJECTION string, matched by a code comment cross-reference in each).
• configure_harness_env sets the var to "1" on the release-build path and explicitly env_removes it on the prebuilt-binary path, so a prebuilt lane can't accidentally inherit a stray 1 from the ambient environment.
• The new resolver test exercises both directions (hook absent → skip, hook present → run) against the same host capability, isolating the gate to the binary's build rather than the platform.
• release.yml never passes --features rocm/e2e-oom-fault-injection, so the hook stays out of shipped binaries, consistent with the doc comments' claims.
• The mock lane (where xtask builds the binary itself, so the feature and the env signal are both present) still runs and passes the scenario — confirmed on the current head's checks.

I also re-read the full diff against base again (not just this delta) given how long this PR has been through review rounds: the OOM signature matcher, the append_oom_serve_note/oom_memory_note gating (status == "starting", already_running excluded), the reuse-before-GPU-preflight reordering, and the two Gherkin scenarios (15 negative / 16 positive) are all unchanged from the last pass and still look correct together. No new findings.

This looks safe to approve after the usual final checks — passing CI (the self-hosted GPU/WSL lanes plus the mock lane) and a maintainer's own pass — since this remains a diff-only read.

[r0x0r]

The attribution reasoning here is the strongest part: gating the post-failure note on already_running == false and status == "starting" and the serve's own log_path is exactly the right defense against blaming this invocation for another process's OOM, and serve_failed_to_become_ready correctly keeps a healthy still-loading running service out of the failure path. The const false fault-injection seam compiling out of release builds is also clean.

One substantive question on precision: vllm_log_shows_oom is a case-insensitive substring scan for out of memory / outofmemory over the last 80 log lines, and the note fires whenever that substring appears in the tail of a failed-to-become-ready serve. It isn't anchored to the terminal/fatal error line. So a serve that died for an unrelated reason but whose tail happens to mention "out of memory" somewhere (a benign warning, a retried allocation that later succeeded, a model/path name containing the token) would be misreported as an OOM failure. The already_running/starting/own-log triad prevents cross-process misattribution, but not "wrong cause, same process." Is anchoring to the last matching line (the way oom_utilization_hint already picks the failing line) worth it here, or is substring-in-tail deemed precise enough in practice?

Smaller related point: the tail is read at 80 lines. If an OOM traceback is followed by more than 80 lines of shutdown/teardown noise, the signature scrolls out of the window and the note is silently withheld. Worth confirming 80 matches the engine's own DEFAULT_LOG_TAIL_LINES budget so the two surfaces don't disagree on what counts as "the tail."

[r0x0r]

Pushed c337c7c addressing the tail-window point; keeping the substring-in-tail detector on purpose, reasoning below.

Tail budget: the serve summary read a bare 80. It now reads rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES (the same budget the engine's OOM surfaces use), so the two surfaces can't silently drift on what counts as "the tail" of a failed launch. Added append_oom_serve_note_reads_the_shared_engine_tail_budget, which plants the OOM signature on the oldest line still inside the budget (must fire) and one row past it (must fall silent), pinning the read to the shared constant rather than a literal.

Anchoring vs substring-in-tail: deliberately keeping the whole-tail scan.

• Anchoring the gate to the last matching line wouldn't change whether the note fires — "any line matches" and "the last matching line matches" are the same boolean. Only tightening to fatal-line-only would change firing, and that trades a cheap, self-correcting false positive (an extra memory hint on an already-failed launch) for the far worse false negative of missing a real OOM whose traceback is followed by teardown noise — which is exactly the case the tail-window point warns about. The two pull in opposite directions.
• The two signatures (outofmemory, out of memory) are error-context tokens in vLLM/PyTorch — exception class names (torch.OutOfMemoryError, hipErrorOutOfMemory) and the RuntimeError: HIP/CUDA out of memory phrasing — not general prose, so a benign-mention false positive is unlikely in practice.
• Cross-process misattribution is already fully blocked by the already_running == false + status == "starting" + own-log_path triad, so the residual risk is bounded to "wrong cause, same already-failed process": an extra diagnostic line, never a wrong action on a healthy serve.

Meanwhile oom_utilization_hint still anchors the quoted --symptom line to the last matching line (.rev().find(...)), so the user always sees the actual failing line as evidence even though the gate stays whole-tail.

[rominf]

Reviewed as the delta against gpu-out-of-memory (#251), so #251's own work isn't re-litigated here — I left that separately on #251.

The note logic is good and the fault-injection seam is a nice piece of design: feature-gated with a #[cfg(not(feature))] const fn -> false fallback, armed per-invocation by an env var, reported to the harness as a capability so the scenario skips rather than silently passes on a prebuilt binary. oom_memory_note is properly unit-tested and the reuse-vs-launch distinction it draws is exactly right.

Three things I'd want fixed before merge.

The reorder is the significant one. To decide reuse_existing, the new block runs load_managed_services (side-effecting — liveness probes, record writes), a full ResolveModel round-trip, and ensure_self_managed_engine_ready before the no-usable-GPU bail. The comment right below it still says "BEFORE preparing or launching any engine (no wasted engine download …)", and that's no longer true. It also drags resolve_gpu_indices from before the no-runtime bail to after it — and the GPU lane on this head is failing serve-absent-gpu-index-rejected with exactly the message that predicts:

Step panicked. Captured output: expected the absent GPU index to be reported unavailable, got:
Error: device_policy: gpu_required; no active ROCm runtime is configured; ...

I checked #251's GPU lane as the control — same runner, same base — and it doesn't fail that scenario (its failures are all serve-timeout/resolved model shaped). So this looks caused by the reorder rather than inherited.

Second: xtask/src/e2e.rs replaces the single --features value with rocm/e2e-oom-fault-injection, but main now passes rocm/e2e-test-hooks there — added by #249, which isn't an ancestor of this branch. On rebase those compete for one slot, and workflow_contract.rs asserts the literal --features rocm/e2e-test-hooks string, so a naive resolution either breaks the contract test or silently compiles out lemonade's failure seams.

Third, smaller: the negative OOM assertion uses a raw contains while both positive ones normalize whitespace — by the PR's own stated reason (80-col soft wrap), so Scenario 15 can pass without testing anything.

On process — this is stacked on #251's branch but isn't draft and has no Depends on #251 in the body. AGENTS.md §11 asks for both, and it matters here: about half of what a reviewer sees belongs to the other PR, and #284's delta relocates code #251 introduces. Worth considering whether the rocm-core placement should just land in #251 directly.

Leak scan is clean and the EAI-8059 reference is fine per AGENTS.md §2.

[rominf]

This comment is now false, and the work it promises won't happen is exactly what runs above it.

"BEFORE preparing or launching any engine (no wasted engine download, and an actionable message instead of a late engine crash)" — but the reuse block at :4877-4897 already ran:

• any_live_managed_service_for_engine → load_managed_services, which is side-effecting: refresh_from_engine_state, refresh_managed_service_runtime_liveness (HTTP readiness/inference probes with a 750ms timeout each), and record.write();
• a full ResolveModel engine round-trip;
• ensure_self_managed_engine_ready for self-managed engines.

That last one is the expensive one. On a GPU-less host with lemonade selected, the user now sees "Preparing lemonade for GPU serving…" and potentially a multi-GiB install, and then gets told there's no usable AMD GPU. AGENTS.md §6's "preserve strict GPU-required behavior" is about the outcome, but the fail-fast part is what this comment is claiming and it's what's lost.

The !reuse_existing exemption itself is correct — nothing is launched when reusing, so the gate shouldn't fire. It's the cost of computing reuse_existing that's the problem.

A cheap ordering fix: gate the whole reuse block on usable_amd_gpu_indices() being non-empty (or None), so on a GPU-less host you fall straight through to the bail. Reuse can't be the right answer on a host with no GPU under gpu_required anyway. Alternatively determine reuse from a non-mutating read of the records and defer the ResolveModel/prepare work until after the gate.

Whichever way it goes, the comment needs to match — if the contract genuinely changed, say what the new one is.

[r0x0r]

Fixed in 4a108b9 — you were right that the comment promised something the block above it had already spent.

The reuse pre-gate keyed on the engine alone, so a live service for a model this invocation can never reuse was enough to pull the ResolveModel round-trip and — the expensive one you named — ensure_self_managed_engine_ready, complete with Preparing lemonade for GPU serving... and a possible multi-GiB install, in front of the no-usable-GPU bail. It now keys on the model too (any_live_managed_service_for_model), so on a GPU-less host with a live lemonade service for some other model the block is skipped entirely and the bail is once again the first thing that runs.

On the two options you offered: gating the whole reuse block on usable_amd_gpu_indices() being non-empty would have been cheaper, but it breaks @id:serve-oom-memory-guidance (serve-20), which reaches the reuse short-circuit precisely on a GPU-less host. So I took the second one — decide reuse from the records and let only a plausible match pay for the engine round-trip. Matching uses service_model_names_match, the same lenient relation the service-listing surfaces already use, so a short-vs-canonical spelling still reaches the probe; the probe then still decides reuse authoritatively on the canonical id via existing_live_managed_service. Reuse detection is not narrowed in practice.

The residual, stated plainly: when a live service does match this engine and model, ensure_self_managed_engine_ready still runs before the bail. That is the case where this invocation is about to reuse and legitimately skip the bail, so the work is not wasted — and the comment now says exactly that instead of claiming an absolute.

Regression tests (apps/rocm/src/main.rs): reuse_pregate_skips_engine_work_for_an_unrelated_live_model — this is the one that fails before the fix, since the old function body had no model term at all and returned true for any live service of the engine; plus reuse_pregate_admits_the_same_model_including_a_short_spelling, reuse_pregate_still_keys_on_the_engine, and reuse_pregate_admits_the_model_id_the_oom_reuse_scenario_plants, which pins the literal id serve-20 plants so a matcher change cannot silently turn that scenario into a launch attempt on the one lane it runs on.

Verified on a Linux MI300X box: full workspace cargo test + clippy -D warnings + fmt --check, and cargo xtask e2e with serve-20 and serve-21 both passing. I could not run the self-hosted E2E tests (GPU) or E2E tests (Strix Halo, Ubuntu) lanes — that hardware is not available to me, so CI is the only authority for those two.

[rominf]

resolve_gpu_indices moved from before the no-runtime bail to after it, and the GPU lane on this head is failing on exactly that.

On the base (:4873-4878 there), resolve_gpu_indices — the source of --gpu index N is out of range — ran ahead of the no active ROCm runtime is configured bail. Here it's at :4934, after. So a bad --gpu value now surfaces as an environment complaint instead of an argument complaint.

From run 33489993381 on c337c7c4:

Step panicked. Captured output: expected the absent GPU index to be reported unavailable, got:

Error: device_policy: gpu_required; no active ROCm runtime is configured; run `rocm runtimes list` ...

I pulled #251's GPU lane (run 33166192967, job 98832217235) as the control, since that lane has other unrelated problems and I didn't want to over-attribute. It does not fail serve-absent-gpu-index-rejected — its failures are all serve-timeout / missing-resolved model shaped. So the regression appears with this delta.

User-facing, argument validation before environment validation is the right order regardless: --gpu 99 is wrong no matter what runtime is active, and telling the user to go activate a runtime sends them down the wrong path.

The reorder that reuse needs shouldn't have to drag resolve_gpu_indices with it — gpu_vram_usage()/resolve_gpu_indices don't depend on resolved_model, so they can go back above the runtime bail.

[r0x0r]

Fixed in 7c09129 (already on the branch when you filed this); re-verified against the current head rather than the claim.

resolve_gpu_indices now runs at apps/rocm/src/main.rs:5209, immediately after the no-usable-GPU pre-flight and before the no-runtime bail at :5211. So --gpu 99 is an out-of-range argument error again regardless of runtime state, and a GPU-less host still refuses with "no usable AMD GPU" first. Your point that argument validation belongs ahead of environment validation is the ordering that shipped.

Lane evidence, since this was a GPU-lane regression: serve-16 / @id:serve-absent-gpu-index-rejected ran and passed on E2E tests (GPU) at 83ecfaad, and it also passes in my own cargo xtask e2e run on a Linux MI300X box at the new head 4a108b9. It is not among that lane's current failures.

Thanks for pulling #251's run as a control before attributing it — that was the right call and it is what made the delta unambiguous.

[rominf]

This replaces a --features value that main added, and there's a contract test asserting the old string.

On origin/main this call site is --features rocm/e2e-test-hooks (from #249, not an ancestor of this branch). Here it becomes --features rocm/e2e-oom-fault-injection, and apps/rocm/Cargo.toml in this diff declares only e2e-oom-fault-injection. git merge-tree origin/main pr/284 conflicts in apps/rocm/Cargo.toml, apps/rocm/src/main.rs, model_serving.feature, serving_steps.rs, and this file — and this one is literally the two feature strings competing for one slot.

The part that makes it more than a routine conflict: xtask/src/workflow_contract.rs:255-274 asserts every prebuilt E2E lane block contains the exact string

cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks

So a resolution that keeps only the new feature fails that test, and one that keeps only the old feature silently disables the new scenario (it degrades to a skip via the capability gate — a green run that tested nothing). Both failure modes are quiet in different ways.

On rebase this wants: a combined list (rocm/e2e-test-hooks,rocm/e2e-oom-fault-injection), e2e-test-hooks restored in apps/rocm/Cargo.toml, and the workflow_contract.rs expectation updated to match. Worth flagging in the PR text too, since whoever resolves the conflict may not know the contract test exists.

The configure_harness_env half is right — clearing the env var for a prebuilt binary rather than letting an inherited value through is the correct call.

[r0x0r]

Addressed across 7c09129 and 4a108b9.

The overwrite is gone: xtask/src/e2e.rs:125 now passes a single space-separated value, "rocm/e2e-test-hooks rocm/e2e-oom-fault-injection", so e2e-test-hooks (#249) survives alongside the new hook rather than losing the one --features slot to it. apps/rocm/Cargo.toml declares both.

On workflow_contract.rs:255-274: the assertion is a contains on cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks, and every prebuilt lane still spells exactly that, so it passes unchanged. But your framing exposed that its doc and failure message had become false — they claimed the asserted string matched what cargo xtask e2e builds for itself, which is now a superset. 4a108b9 rewrites both to describe it as the shared hook floor, and records why the fault-injection hook is deliberately absent from the prebuilt lanes: those scenarios carry @requires-oom-fault-injection, so a missing hook is a reported skip, not the silent green you were worried about. That closes the third failure mode — a stale comment nobody re-reads on rebase.

Your two other resolution notes are now moot for this branch: this PR targets gpu-out-of-memory, not main, and the base already carries the serve-NN scenario naming, so merge-tree no longer conflicts in model_serving.feature. I have put the combined-feature-list requirement in the PR body anyway, since whoever eventually rebases this onto main will hit exactly the collision you describe and should not have to rediscover the contract test.

Agreed on configure_harness_env — it env_removes on the prebuilt path rather than letting an ambient 1 through, and that is unchanged here.

[rominf]

This negative assertion is whitespace-sensitive while both positive ones aren't, so Scenario 15 can pass vacuously.

assert_oom_launch_memory_guidance (:954) uses screen_without_whitespace(&screen).contains("ranoutofGPUmemory"), and the comment above it explains why: "The note is a long line the 80-column PTY wraps across grid rows … otherwise a soft wrap between 'GPU' and 'memory' would break a literal contains and mask a rendered note."

That reasoning applies identically here, in the direction that matters more. If the note were wrongly emitted on the reuse path and happened to wrap — which the positive step says is the normal case for this string — the raw contains wouldn't see it and the scenario would pass. So the regression guard is disarmed precisely when the note renders the way the code actually renders it.

screen_without_whitespace(&screen).contains("ranoutofGPUmemory") with the ! is the one-line fix, and it makes the pair symmetric.

The scenario itself is a good one — "don't blame this invocation for another process's OOM" is a real distinction and worth pinning.

[r0x0r]

Fixed in 7c09129; verified against the current code, not the claim.

tests/e2e-cucumber/tests/e2e/serving_steps.rs:1218 is now:

!screen_without_whitespace(&screen).contains("ranoutofGPUmemory"),

exactly the one-line fix you proposed, so the negative and the positive (:1272) are symmetric and the guard survives the soft wrap the positive step documents as the normal rendering. The comment above it now spells out why the whitespace collapse is load-bearing in the negative direction specifically — that a literal contains would have passed even if the note were wrongly emitted.

The scenario is now serve-20 (renumbered when the base landed serve-19); the @id: is unchanged. It ran and passed in cargo xtask e2e on a Linux MI300X box at 4a108b9, and it also runs on the blocking GitHub-hosted mock lane.

Thanks — that one really would have been a silently disarmed regression guard.

[rominf]

Two coupled issues with this detector and the note it drives.

The threshold contradicts #251's. This matches a bare out of memory substring anywhere in the tail. In the same stack, check_16_vllm_oom deliberately scores a bare "out of memory" at 25 — below MIN_SCORE_FOR_MATCH — with a comment explaining that it "is deliberately sub-threshold so this only claims the failure mode when the vLLM/HIP shape of the error is present," and gates the whole table behind VLLM_ANCHOR_PATTERN. Same failure class, same repo, two thresholds, and the looser one is the one driving user-facing serve output.

A managed-service log tail containing out of memory from any source — a kernel OOM-killer line, a dependency's log, an unrelated subprocess — produces the vLLM OOM note. The doc comment here is careful about excluding vLLM's generic wrapper line, which suggests the false-positive risk was on your mind; the bare-phrase case is the bigger one.

The guidance is wrong for the model-too-large case. The note only ever suggests --gpu-memory-utilization and --gpu <index>. Both help when the GPU is shared or busy. Neither helps when the model simply doesn't fit — and lowering the reservation there actively makes it worse, trading an earlier OOM for a later one.

#251 already gets this right: check_16_vllm_oom's summary says "Only lower --gpu-memory-utilization when the GPU is shared or already busy; on a GPU dedicated to this server it does not create room a too-large model needs." That caveat doesn't survive into the note this PR shows users, and --max-model-len isn't mentioned anywhere in the new guidance.

I'd reuse #251's anchored matcher rather than maintain a second looser one, and either carry the shared-vs-dedicated caveat into the note or have it point at rocm diagnose --symptom 'vllm: …', which already does the nuanced version. Pointing at diagnose is the lighter option, and it exercises the handoff the two PRs are building.

Small related note on the wording: the hint says --gpu-memory-utilization <0-1>, but parse_gpu_memory_utilization enforces (0, 1] — 0 is rejected.

[r0x0r]

All three points fixed in 83ecfaa. Taking them in your order:

Threshold. You were right that maintaining a second, looser matcher for the same failure class was the bug, and that the looser one driving user-facing output made it worse. rocm_core::vllm_log_shows_oom no longer does a bare substring scan — it classifies each tail line with the same rule as check_16_vllm_oom, scoring the line as vllm: <line> against the shared MIN_SCORE_FOR_MATCH threshold (crates/rocm-core/src/lib.rs:7424). The tail is vLLM's own process output, so the checker's required VLLM_ANCHOR_PATTERN is supplied by context rather than dropped. Sub-threshold lines that used to fire now correctly do not: a kernel OOM-killer line, a bare out of memory, a bare HIP error: out of memory, a bare torch.OutOfMemoryError with no allocator corroboration. Genuine allocator failures still clear it. One helper feeds both the engine hint and the CLI note, so there is no second matcher left to drift.

Model-too-large guidance. Also fixed, and I took your lighter option and carried the caveat, since the note is what the user actually reads. oom_memory_note (apps/rocm/src/serve_summary.rs:219) now says that if the model simply does not fit, lowering the reservation will not help, and to serve a smaller or quantized model instead — then routes to rocm diagnose --symptom '<their actual failing line>' for the case-appropriate fix, which exercises the handoff you point out the two changes are building. --max-model-len is deliberately left to diagnose, which does the nuanced version, rather than duplicated into a fixed string here.

Wording. <0-1> is gone. The shared hint now reads "a fraction greater than 0 and at most 1", matching what parse_gpu_memory_utilization actually enforces on (0, 1].

Verified at 4a108b9 on a Linux MI300X box: full-workspace cargo test + clippy -D warnings, and cargo xtask e2e with diagnose-13 (@id:diagnose-vllm-oom-is-conditional), serve-20 and serve-21 all passing. I could not run the self-hosted E2E tests (GPU) or E2E tests (Strix Halo, Ubuntu) lanes — no such hardware available to me — so CI remains the only authority on those two.

[rominf]

This comment claims an invariant that isn't enforced anywhere.

"Read the same tail budget the engine's own OOM surfaces use (rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES) … rather than drifting apart on independent magic literals" — but the vLLM engine's OOM surface reads STARTUP_FAILURE_LOG_TAIL_LINES, a local const … = 80 at engines/vllm/src/lib.rs:34, used at :1817 and :1823. That's precisely the independent magic literal the comment says has been eliminated.

They're both 80 today, so nothing is broken. But the comment (and the commit message) describes an enforced coupling, and a future change to DEFAULT_LOG_TAIL_LINES would silently desynchronize the two surfaces — with the comment still asserting they can't.

Pointing STARTUP_FAILURE_LOG_TAIL_LINES at the protocol constant is a one-liner and makes the claim true. Otherwise the comment should describe the intent rather than a guarantee.

[r0x0r]

You were right, and this one was genuinely still open — the earlier response commits did not touch it. Fixed in 4a108b9.

engines/vllm/src/lib.rs:35 was still const STARTUP_FAILURE_LOG_TAIL_LINES: usize = 80;, the exact independent literal the serve-summary comment claimed had been eliminated. It now reads:

const STARTUP_FAILURE_LOG_TAIL_LINES: usize = DEFAULT_LOG_TAIL_LINES;

DEFAULT_LOG_TAIL_LINES was already imported in that file, so it is the one-liner you described. I took your first option rather than weakening the comment to intent, because the coupling is the thing worth having: a change to the protocol budget now moves both surfaces, and append_oom_serve_note and oom_utilization_hint cannot silently disagree on what "the tail" of a failed launch means. The const carries a doc comment saying so, so the next person is not tempted to re-inline the literal.

Deliberately scoped to vLLM: engines/lemonade has its own STARTUP_FAILURE_LOG_TAIL_LINES alongside a local DEFAULT_LOG_TAIL_LINES = 200, and it is not part of the OOM surface this PR couples, so I left it alone rather than widen the change.

Verified at 4a108b9: full-workspace cargo test + clippy -D warnings + fmt --check on a Linux MI300X box, plus the vLLM engine's own tail-budget tests. Not verified on E2E tests (GPU) or E2E tests (Strix Halo, Ubuntu) — that hardware is unavailable to me.

[rominf]

This constant is declared twice — here and in xtask/src/e2e.rs:24 — held together only by a "kept in sync" comment on each side.

A typo in either doesn't fail to compile and doesn't fail a test. xtask sets one name, the harness reads another, oom_fault_injection is false, @requires-oom-fault-injection maps to Expectation::Skip, and the scenario silently stops running. Green suite, zero coverage — the same failure mode the comment on probe_host_capability is careful about elsewhere.

Since the harness crate is already a build dependency relationship away, hoisting the constant into a shared crate would be cleanest. Failing that, a test in either crate asserting the two literals are equal costs three lines and closes it.

Minor, but this scenario is the only thing exercising the fault-injection path, so a silent skip is expensive.

[r0x0r]

Fixed in 83ecfaa, and I took the cleaner of your two options.

ROCM_E2E_OOM_FAULT_INJECTION is now declared exactly once, in the e2e-report crate that both xtask and e2e-cucumber already depend on:

• crates/e2e-report/src/lib.rs:30 — pub const OOM_FAULT_INJECTION_ENV: &str = "ROCM_E2E_OOM_FAULT_INJECTION";
• xtask/src/e2e.rs:30 — use e2e_report::OOM_FAULT_INJECTION_ENV;
• tests/e2e-cucumber/src/capability.rs:134 — pub use e2e_report::OOM_FAULT_INJECTION_ENV;

so the two former copies are re-exports of one source and a typo is now a compile error rather than a silent oom_fault_injection = false. Since it is a single constant, the equality test you offered as the fallback is no longer needed — there is nothing left to hold in sync.

Your framing of the cost is what decided it: this is the only thing exercising the fault-injection path, so a @requires-oom-fault-injection scenario degrading to Expectation::Skip would have been a green suite with zero coverage of the PR's headline behavior. The resolver still has tests for both directions (hook present → run, hook absent → skip), and serve-21 ran and passed under cargo xtask e2e on a Linux box at 4a108b9, so the signal is reaching the harness for real rather than only in principle.

[rominf]

These reuse scenario numbers that already exist on main.

origin/main's model_serving.feature has a Scenario: 15 at line 143 ("Serving with a temperature below zero is refused…"), another Scenario: 15 at 173 ("Selecting both a runtime and an environment at once…" — main already has a duplicate), and a Scenario: 16 at 184. This PR adds a third 15 and a second 16, in different hunks, so git merges them cleanly and nothing complains.

The cost is triage: a lane reports a scenario by name and number, and "Scenario 15 failed" stops identifying anything. Worth renumbering on rebase — or dropping the manual numbers in favor of the @id: tags the repo already uses everywhere, which is what the tooling actually keys on.

Both new scenarios read well otherwise; the positive/negative pair covers the distinction properly.

[r0x0r]

Fixed — the collision is gone as of 83ecfaa, though partly by the base moving under us.

origin/main no longer carries bare Scenario: 15 / 16 headings; every scenario in model_serving.feature is now Scenario: serve-NN - ..., running to serve-18 on main. The base branch (#251) adds serve-19, and this PR's two are serve-20 and serve-21, so the numbering is unique and sequential across the file and feature_naming is satisfied. Verified against origin/main and origin/gpu-out-of-memory at the current head rather than from memory.

On dropping the manual numbers in favour of @id:: agreed that @id: is what the tooling keys on, and the two scenarios here carry stable ones (serve-oom-memory-guidance, serve-oom-launch-memory-guidance) that survived both renumberings untouched — I have quoted the @id: rather than the number everywhere in the PR body for exactly the triage reason you give. Removing the numeric prefixes wholesale is a rename across every feature file plus the feature_naming test, which is a bigger and unrelated change than this PR should carry; happy to raise it separately.

Both scenarios ran and passed under cargo xtask e2e on a Linux MI300X box at 4a108b9. Thanks for the note on the pair reading well — the positive/negative split was the thing earlier rounds spent the most time getting right.

[r0x0r]

Thanks for the review. Rebased onto the updated #251 head and addressed the points; the branch is now two commits (the squashed feature + a review-response commit) and back in draft while it depends on #251.

• --gpu validation ordering (blocking). You were right that the reorder ran ResolveModel/ensure_self_managed_engine_ready before the pinned-index check, so serve-absent-gpu-index-rejected ("refused before any engine starts") would fail on the GPU lane. resolve_gpu_indices now runs immediately after the no-usable-GPU pre-flight and before any runtime/engine work, so a nonexistent --gpu index is rejected up front; a GPU-less host still refuses with "no usable AMD GPU" first.
• xtask feature collision. The e2e build no longer overwrites rocm/e2e-test-hooks (fix(lemonade): retry interrupted backend setup #249). It now passes a single --features "rocm/e2e-test-hooks rocm/e2e-oom-fault-injection" value so both hook surfaces are compiled into the mock-lane binary; workflow_contract.rs's prebuilt-lane assertion is unaffected (those lanes intentionally carry only e2e-test-hooks and skip the fault-injection scenario).
• Negative assertion. does not blame this invocation for GPU memory now collapses whitespace exactly like the positive checks, so it genuinely catches a note that wraps across the 80-column PTY grid rather than passing vacuously against a literal substring.
• Draft + dependency. PR is back in draft and the body now states Depends on feat(vllm): Tackle out of memory errors (EAI-8058) #251; I'll rebase onto main and mark it ready once feat(vllm): Tackle out of memory errors (EAI-8058) #251 merges.

Also renumbered the two OOM scenarios to 17/18 to sit alongside the runtime/env and Lemonade-recovery scenarios that landed on the base, and fixed a stale log_tail_shows_oom reference (the detector now lives in rocm-core as vllm_log_shows_oom). Full workspace cargo test + clippy -D warnings + smoke_local.py are green locally.

[rominf]

🔴 Automated review · pr-review-watcher · c951af5

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 adds vLLM out-of-memory detection. One shared rule in rocm-core now decides what counts as an OOM. The vLLM engine's startup hint and the rocm serve summary both use that rule to show memory guidance, along with a safely quoted rocm diagnose --symptom command. The PR also adds a model-keyed check that lets reuse of a running service skip the no-GPU refusal, a test-only e2e-oom-fault-injection hook, and scenarios serve-24/25/26. Outcome: No blocking findings.

Round scope. Every commit since the last round (fe419818) merges the base branch into this one. Comparing the PR's own added and removed lines file by file shows they are unchanged since that round. The one conflict resolution, in crates/rocm-core/src/fix.rs in 3e2d551, keeps this PR's range-pin test beside the base's renamed a_recipe_that_acts_has_a_runner. This round therefore re-read the PR's own diff and how it interacts with the base code that arrived since.

Reviewed: the PR's own diff git diff prw-base...HEAD (merge-base 31f2186), all 18 files. Also read for context:

• in apps/rocm/src/main.rs: serve() in full, spawn_managed_engine_child, start_managed_service, existing_live_managed_service, service_model_names_match, status_for_readiness and parse_device_policy
• diagnose.rs, around vllm_oom_symptom_is_diagnosable / check_16_vllm_oom
• the e2e lanes in .github/workflows/{ci,e2e-selfhosted,nightly}.yml
• the base movement 5da11d5..31f2186 for main.rs, fix.rs, xtask, e2e and the workflows

Verified:

• Tests pass: rocm-core lib (576), rocm bins (1028), rocm-engine-vllm (92), xtask (271).
• cargo check -p e2e-cucumber --tests is clean.
• cargo clippy -D warnings --all-targets is clean on rocm with the fault-injection feature, and on rocm-core, vllm, e2e-cucumber, xtask and e2e-report.
• The mock lane builds the feature and exports ROCM_E2E_OOM_FAULT_INJECTION=1.
• cpu_only cannot reach a managed service record, because parse_device_policy refuses it.
• The e2e suite and smoke_local.py were not run.

Blocking: 0 · Non-blocking: 11.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• apps/rocm/src/main.rs:6640-6646 — The comment says --gpu is validated "before engine/runtime resolution". That is false whenever the reuse check opens: runtime selection, ResolveModel and ensure_self_managed_engine_ready all run before validate_pinned_gpu_index. A ResolveModel error can then pre-empt the GPU-specific refusal the comment promises. Either validate the pinned index before the reuse check, or fix the comment.
• apps/rocm/src/main.rs:6586-6595 — The comment says the side-effecting block runs "only when this invocation is about to reuse it". But any_live_managed_service_for_model uses the substring match in service_model_names_match. A live Qwen/Qwen3-8B therefore lets rocm serve qwen3, a different model, run engine work ahead of the no-GPU bail. Reword it to "plausibly matches", or tighten the match.
• apps/rocm/src/main.rs:6544-6576 — If the reused service dies between the reuse check and the duplicate check in spawn_managed_engine_child, the launch goes ahead with the no-GPU bail already skipped. Only the engine's own GPU check stops it then; the window is narrow.
• crates/rocm-core/src/lib.rs:8198-8203,8239-8246 — vllm_log_shows_oom and vllm_oom_diagnose_symptom split on str::lines() and add vllm: to each line. The checker splits on terminal::rendered_lines, which also breaks on a bare \r (and NEL, CUD and similar). So an OOM that follows a \r progress-bar repaint on the same \n line loses its vllm anchor and is missed. The base engine's per-line substring check would have caught that line. Iterate rendered_lines here so both sides draw line boundaries the same way, as the doc's "never disagree" claim implies.
• tests/e2e-cucumber/README.md:145, tests/e2e-cucumber/src/capability.rs:126-134, tests/e2e-cucumber/src/expectation.rs:133-142, xtask/src/e2e.rs:21-25 — These call the prebuilt self-hosted lanes the "shipping release build". The workflow commands govern, and they build with the rocm crate's e2e-test-hooks feature (e2e-selfhosted.yml:331, nightly.yml:458, and others). Say "a prebuilt binary without the fault-injection feature" instead.
• .github/workflows/e2e-selfhosted.yml:324-330, nightly.yml:457,567,665,874, ci.yml:668-669 — These comments still say the prebuilt feature set "must match what cargo xtask e2e builds". xtask now builds a superset; workflow_contract.rs was updated to say so, but these comments were not.
• apps/rocm/Cargo.toml:15-20 (design) — The separate e2e-oom-fault-injection feature needs its own capability tag, env-var contract, shared constant in e2e-report, README row and contract-test carve-out. Every e2e lane already builds e2e-test-hooks, and the existing ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE hook uses exactly that feature-plus-env-var pattern. Gating this hook the same way would remove the capability that cannot be probed; serve-25 is already confined by @requires-no-gpu @requires-os:linux.
• tests/e2e-cucumber/features/model_serving.feature:316 — The serve-24 title promises it "never blames it for another process's OOM", but no step can fail on that attribution. As its own comment admits, the reuse path reports log_path: None, so the planted OOM log is never read. What it does prove is that reuse reaches the summary on a GPU-less host with no OOM note; retitle it to say that.
• tests/e2e-cucumber/features/model_serving.feature:322,353 — Both comments say "Scenario 23" but mean serve-24. serve-23 is the low-VRAM serve-plan scenario and plants no live service.
• apps/rocm/src/serve_summary.rs tests oom_signatures_are_detected_case_insensitively / unrelated_failures_are_not_flagged_as_oom — These only exercise rocm_core::vllm_log_shows_oom, which rocm-core's own tests already cover, and they sit in a crate that does not own the function.
• PR description — The review agent was not given the description, so this was checked separately. Base drift has renumbered the scenarios again. The description's table and verification list still call the reuse scenario serve-23 and the launch-OOM scenario serve-24; at this head they are serve-24 and serve-25, and serve-23 is the base's low-VRAM plan. The fail-fast regression scenario serve-26 (serve-unrelated-live-service-still-fails-fast) is not mentioned at all.

[rominf]

Automated review updated for c951af5 (the head only merged base-branch updates since the last round; no blocking findings, 11 non-blocking). The full report is in the review comment above: #284 (comment)

[siloteemu]

🔴 Automated review · pr-review-watcher · a1e15a8

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.

Round history: every finding below is new this round. Our previous round (at e47d0d6d) raised no blocking findings; this one is a full re-review of the whole change against main, which the PR now targets.

Review — needs work

Full review of the whole change.
The description says the PR adds vLLM out-of-memory guidance to the rocm serve summary. The ticket named in the title could not be retrieved from here. The description no longer matches the branch (see Blocking).

Blocking

• The summary's rocm diagnose command quotes the engine's own hint, not the user's failing line, on a real managed launch — apps/rocm/src/main.rs:7129-7163, crates/rocm-core/src/lib.rs:8244-8251, engines/vllm/src/process.rs:732-783, apps/rocm/src/main.rs:7352-7361

  • The managed engine is rocm itself, started with log_path = record.log_path, and its stdout and stderr are appended to that same file (attach_background_stdio).
  • When vLLM exits early, wait_for_vllm_ready bails with startup_log_context. exit_code_for prints that error to stderr, so the service log ends with the engine's own block: …Detected an out-of-memory failure (…) followed by For conditional remediation, run `rocm diagnose --symptom 'vllm: …'` .
  • append_oom_serve_note then reads that log after the 45 s wait. vllm_oom_diagnose_symptom scans with .rev() and picks the last line that clears the classifier, which is the engine's hint line.
  • That line contains ', so it fails quotable_in_single_quotes and the canonical symptom is printed instead.
  • Reproduced by building the real startup_log_context output into a log and calling the shared selector:
    • Selected: Some("vllm: For conditional remediation, run `rocm diagnose --symptom 'vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.'`."), quotable: false.
    • With only the vLLM output in the log, the real line is selected.
  • So "routes the user's actual failing line" fails on the path it was built for. Every test feeds a clean synthetic tail, so none catch this.
  • Confidence 85 · logic · Fix: stop the CLI-side selection at the engine's Last N lines of startup log: marker, or skip lines the engine emits. Add a test whose tail ends with the engine's hint block.

• The reuse pre-gate's liveness clause is not covered by any test — apps/rocm/src/main.rs:20027-20036, tests at main.rs:30752-30880

  • Deleting && managed_service_is_live(record) leaves all four reuse_pregate_* tests and every append_oom_serve_note_* test green. That was run: 10 passed.
  • reuse_pregate_for only plants live records (status = "starting", engine_pid = the test process).
  • Without the clause, a stopped or dead record for the same model unlocks the ResolveModel round-trip and the self-managed install ahead of the no-usable-GPU bail.
  • Confidence 100 · logic · Fix: add a case with a stopped record or a dead pid that must return false.

• the_release_workflow_builds_no_feature_gated_binaries passes when a release build enables the test-only feature — xtask/src/workflow_contract.rs:2034-2046

  • Changing release.yml:127 to cargo build --release --all-features -p rocm -p rocmd -p xtask leaves the test green (run). That build would compile in e2e-oom-fault-injection.
  • The filter is line.contains("cargo build") && line.contains("--features"). It also misses -F … and a --features placed on a \ continuation line.
  • It has no positive control: a release.yml with no cargo build line passes too.
  • Its doc says it "covers every future test-only feature", which it does not.
  • Confidence 100 · logic · Fix: match --features, --all-features and -F over the joined run blocks (multiline_run_blocks is already in this file), and assert at least one cargo build was seen.

• With a matching live service, the reuse pre-gate does runtime and engine work before the explicit --gpu check — apps/rocm/src/main.rs:6564-6625 against main.rs:6663-6679

  • The existing contract at 6663 reads: "Validate an explicit --gpu <index> up front — before engine/runtime resolution — so an out-of-range or masked-out ordinal produces a GPU-specific refusal even when no ROCm runtime is configured."
  • When any_live_managed_service_for_model matches (by the lenient both-ways contains relation, so qwen matches a live qwen3-8b-instruct), the new block first runs three steps:
    • validate_engine_selection_runtime(...)?, which can write a runtime manifest or fail on it through recover_setup_runtime_registration;
    • ensure_self_managed_engine_ready, which can install;
    • ResolveModel.
  • So rocm serve qwen --gpu 99 next to a live qwen3-8b-instruct service can report a runtime-manifest error, or install, before the GPU-specific refusal. Two independent passes reached this by reading; it was not run.
  • Confidence 70 · logic · Fix: run the pre-gate only on the path that would otherwise refuse (visible_gpu_indices == Some([])). This also removes the pre-gate's cost from every host with a usable GPU. Alternatively, validate --gpu first.

• A Given step is a no-op, and its precondition is set up in the When — tests/e2e-cucumber/tests/e2e/serving_steps.rs:1451-1459, :1461-1494

  • plant_oom_launch has an empty body; its comment says it "exists to state the scenario's premise".
  • The fault injection is armed inside open_oom_launch_summary (spawn_with_env(..., &[("ROCM_E2E_SIMULATE_OOM_LAUNCH", "1")])). So the Given establishes nothing and the When both sets up the state and acts.
  • This file already has the right pattern: serve-18's lemonade_preparation_cannot_complete (:855-860) pushes its arming variable into world.command_env.
  • Confidence 85 · mechanical · Fix: have the Given push the variable into world.command_env, and have the When spawn plainly.

• Several comments added or invalidated by this change are false, and would mislead the next reader — locations below

  • xtask/src/workflow_contract.rs:2005-2048: the new test and its doc were inserted between ci_windows_lifecycle_lane_reuses_the_binaries_it_built's doc block and its #[test]. The Windows-lane doc ("…must build WITHOUT them…") now documents the release-workflow test, and the lifecycle test at :2049 has none.
  • tests/e2e-cucumber/features/model_serving.feature:322 ("Positive counterpart of Scenario 23") and :353 ("scenario 23 plants the SAME model"): serve-23 is the low-VRAM plan preview and plants nothing. The scenario that plants a same-model service is serve-24.
  • crates/rocm-core/src/lib.rs:8225-8236: says that on a torch OOM followed by a bare RuntimeError: ... killed: out of memory, "the vaguer line wins… Both are diagnosable".
    • The selector only accepts lines that clear the checker.
    • The PR's own sub_threshold_lines_carry_no_hint_at_all (process.rs) pins "the process was killed: out of memory" as not diagnosable, so the torch line wins.
    • The paragraph was correct for the old substring detector it was moved from.
  • crates/rocm-core/src/diagnose.rs:1556-1558 and :1640-1643: these still say the engine falls back to the canonical symptom "when a user's actual failing line would not itself be diagnosable" and that "the vLLM engine calls this".
    • After this change the only production caller is vllm_log_shows_oom.
    • The fallback now fires when a line is not quotable.
  • xtask/src/e2e.rs:113-116: "cargo takes one --features argument, so both must be listed together rather than in two overriding flags". Cargo accepts repeated --features and adds them together. The single value works; the stated reason is wrong.
  • Confidence 85 · mechanical · Fix: move the new test and its doc out of the lifecycle doc, point the two feature comments at serve-24 or its @id, and rewrite the three false paragraphs to match the code.

• The description does not match the branch — PR description

  • It says "Depends on #251… This PR is stacked on it and targets that branch; it must not merge before #251 does, after which it will be rebased onto main." The PR targets main, and #251 is already in main.
  • The Behavior coverage table lists two scenarios, both with stale numbers:
    • serve-oom-launch-memory-guidance is listed as serve-24 and is serve-25.
    • serve-oom-memory-guidance is listed as serve-23 and is serve-24.
  • It omits the third scenario this PR adds, serve-unrelated-live-service-still-fails-fast (serve-26).
  • It does not mention a user-observable behaviour this branch introduces against main: re-running serve for a model whose managed service is already live now skips the no-usable-GPU refusal. reuse_existing does not exist on main; serve-24 depends on it.
  • The verification section has a failing-scenario table headed "Status at 83ecfaad", an older head, with no lead-in. It sits directly under "Every lane is green at this head", so a reader cannot tell what it describes.
  • Confidence 85 · mechanical · Fix: narrow and refresh the description to this head. Drop the stacking note, list all three scenarios by @id, state the reuse-skips-the-GPU-refusal behaviour, and remove or explain the stale table.

Non-blocking

• An OOM line with a carriage-return or erase-line prefix is no longer detected — crates/rocm-core/src/lib.rs:8202-8206

  • vllm_log_shows_oom splits on \n and scores vllm: {line}. The checker then splits on rendered lines, so only the first segment keeps the vllm: anchor.
  • Run: vllm_log_shows_oom("\u{1b}[2K\rtorch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.") and vllm_log_shows_oom("Loading 40%\rtorch.OutOfMemoryError: …") both return false. The old engine substring scan matched both.
  • This is consistent with rocm diagnose, and in a vLLM traceback the OOM line is normally its own line, so the practical impact is likely small.
  • Confidence 100 · logic · Fix: classify per rendered segment, prefixing each segment with vllm: .

• If the capability signal breaks, serve-25 silently becomes a skip that reports green — tests/e2e-cucumber/src/capability.rs:395-396

  • Nothing tests the env parse (is_some_and(|value| value == "1")). The unit tests build HostCapability by hand, and nothing asserts the mock lane actually runs serve-oom-launch-memory-guidance.
  • The shared constant prevents a typo in the variable's name. It does not prevent a broken parse or a lane that stops setting the variable.
  • Confidence 70 · architectural · Fix: either extract the parse into a pure function and test it, or fail the report when that @id is skip on the mock platform.

• The scenario id serve-oom-memory-guidance names the opposite of what it asserts — model_serving.feature:315

  • It asserts the summary carries no memory guidance. Its positive counterpart is serve-oom-launch-memory-guidance.
  • The PR description already pairs the ids with the wrong scenario numbers, so this confusion is likely to happen again.
  • Confidence 70 · mechanical · Fix: rename it (for example serve-reuse-not-blamed-for-oom), if the id is not yet relied on elsewhere.

• Two serve_summary tests only exercise rocm_core — apps/rocm/src/serve_summary.rs:504-528

  • oom_signatures_are_detected_case_insensitively and unrelated_failures_are_not_flagged_as_oom call only rocm_core::vllm_log_shows_oom, which rocm-core already tests.
  • Confidence 85 · mechanical · Fix: delete them, or move any case rocm-core lacks into rocm-core.

• The status gate is applied twice — apps/rocm/src/main.rs:7136, apps/rocm/src/serve_summary.rs:227

  • Deleting || !serve_summary::serve_failed_to_become_ready(status) from append_oom_serve_note leaves every test green (run), because oom_memory_note re-checks the status.
  • The behaviour holds either way. The early return only saves the log read, and nothing pins which copy is meant to own the rule.
  • Confidence 100 · mechanical · Fix: keep one gate, and say in the remaining doc that it owns the rule.

Decisions for the author

• Where the reuse pre-gate runs — tradeoff

  • As written, reuse works for any spelling and reuses the probe's ResolveModel answer.
  • The cost is that engine work can run on GPU-equipped hosts whenever a lenient name match exists, including ahead of --gpu validation (see Blocking).
  • Scoping it to the no-usable-GPU case removes that cost everywhere else. An exact model_ref/canonical_model_id match there would also avoid the engine round-trip, but a user who types a different alias would get the refusal instead of reuse.
  • Note the decision is made before the launch lock. If the matching service exits before spawn_managed_engine_child re-checks, a fresh launch proceeds with the bail already skipped, and relies on the engine's own GPU check.
  • The existing reuse short-circuit compares only engine, canonical id, recipe and auth, not --port, --gpu or --runtime-id. This change extends that short-circuit past the no-GPU refusal.

• Non-interactive serves get no post-failure OOM guidance — tradeoff

  • print_managed_launch_plain prints only readiness: starting; the in-code comment acknowledges this.
  • For: the plain output is machine-readable by design.
  • Against: assistant-driven and scripted rocm serve is a first-class surface, and the same failed launch is explained in one invocation and not the other.
  • A single note: line, emitted only when the launch failed and the log shows OOM, would close the gap.

• The narrower detector also silences the engine hint — tradeoff

  • HIP error: out of memory, a bare torch.OutOfMemoryError, and kernel OOM-killer lines now get no memory hint on any surface; the engine surface used to fall back to the canonical symptom for them.
  • For: consistent with rocm diagnose.
  • Against: HIP error: out of memory is a plausible real HIP-runtime failure line.
  • Confirm this is intended; the other option is widening the diagnose table.

• What serve-25 proves — tradeoff

  • The hook replaces the whole spawn → readiness-wait → status path, and hard-codes status: "starting" and the log path.
  • serve-25 therefore verifies how the note is composed and rendered for a given failed-launch state. It does not verify that a real failed launch produces that state; only GPU lanes or unit tests can.

• Scope and history — non-blocking-improvement

  • The branch combines separable threads:
    • the OOM detector, note and diagnose routing;
    • the reuse pre-gate that bypasses the GPU refusal;
    • moving the quoting guard into rocm-core's terminal module;
    • the vLLM engine refactor;
    • the e2e fault-injection scaffolding;
    • a new release-workflow contract test.
  • The first-parent history has about 20 merge commits from a feature branch. Splitting out the reuse/GPU-refusal change and the test scaffolding, or at least squashing into meaningful commits, would make each part reviewable.

Positive signals

• vllm_oom_diagnose_symptom returns Option, so selecting the line and gating on OOM are one predicate shared by the engine and the CLI, not two copies that can drift.
• e2e_simulate_oom_launch is a const fn returning false without the feature, and nothing in the workspace enables that feature except xtask e2e.
• serve-26 pins the pre-gate's model keying end to end. That is behaviour no unit test of the predicate could show.
• append_oom_serve_note_reads_the_shared_engine_tail_budget places the OOM line at the oldest in-window line and then one line past it, which pins the shared constant exactly.
• The quoting tests assert the exact canonical fallback rather than just "no ' survived", which rules out a stripped lookalike passing.

Deployment notes

None

What this covered

What was read and checked:

• Scope: every file in prw-base...HEAD (merge base 1845df3…, head a1e15a8), 18 files.
• Read in full: serve_summary.rs, terminal.rs, engines/vllm/src/process.rs, the feature file, serving_steps.rs, capability.rs, expectation.rs, tui_driver.rs and xtask/src/e2e.rs.
• Read in part: apps/rocm/src/main.rs (~40k lines) and crates/rocm-core/src/lib.rs (~13.7k lines) were read as every changed item plus its callers, callees and the surrounding serve, launch and reuse code, not the whole files. Same for workflow_contract.rs, diagnose.rs and the CI workflows.
• Passes run: coupled-group code passes, plus instruction adherence, history, prior reviews on earlier PRs that touched these files, and code comments. Three design questions were investigated.
• Run:
  • targeted mutation checks on the pre-gate liveness clause, the duplicated status gate and the release-workflow guard;
  • a probe of the shared selector against the engine's real failure output;
  • the CR/erase-line classifier probe.

Not run or not checked:

• No full test or e2e run: the e2e suite was not run locally, so the scenario verdicts are by reading.
• Ticket: the ticket named in the title could not be retrieved from here.
• CI: CI was read from the status rollup, where every reported check passed. Skill checks (skillscope) and Regenerate manifests reported skipped. No GPU or self-hosted e2e lane appears in the rollup at this head, so serve-24 to serve-26 are exercised only on the mock lane here.

Not reconciled against any discussion on this PR.

[r0x0r]

@rominf — description finding addressed (description only; no code change, so the head is still 88e3d18e).

You were right that the numbering had drifted again. Resolving every @id: this PR's description cites against the feature file at this head turned up four stale numbers rather than two — the base-branch scenario was wrong as well:

@id:	description said	actually at 88e3d18e
serve-oom-memory-guidance	serve-23	serve-24
serve-oom-launch-memory-guidance	serve-24	serve-25
serve-vllm-low-vram-oom-guidance (base, #251)	serve-19	serve-23
serve-unrelated-live-service-still-fails-fast	not mentioned	serve-26

serve-16 (serve-absent-gpu-index-rejected) was the one number that was still correct, so I left it.

serve-26 is now in the scenario table, the verification list and the numbering note.

Two things I deliberately did not do:

• The CI-triage table's serve-19 stays as it is. Its header pins it to 83ecfaad, so the numbers in it are a historical record of what that lane reported; rewriting them to today's numbering would make the record say something that never happened. I added a note under the table scoping the numbers to that SHA and naming the stable @id: instead.
• The serve-24 scenario title is unchanged in the feature file. I took your point that it promises an attribution no step can fail on, and the description's row now says what the scenario actually proves — "reaches the summary with no OOM note" — rather than repeating the claim. Retitling the scenario itself touches model_serving.feature, and my build host is down, so I would rather not push an untested change to a file the naming-contract tests read. It is on the list.

Same reason for the rest of your non-blocking list, which is all in Rust or workflow YAML: the comment at main.rs:6640-6646, the "shipping release build" wording across the README/capability.rs/expectation.rs/e2e.rs, the "must match what cargo xtask e2e builds" comments, the rendered_lines vs str::lines() split, and the serve_summary.rs tests that only exercise a rocm-core function. None of those are forgotten; they are queued behind a working builder.

[rominf]

🔴 Automated review · pr-review-watcher · 88e3d18

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.
Implements part of EAI-8059 (now keyed ROCMAI-419): the post-failure OOM note naming --gpu-memory-utilization and --gpu. It departs from the ticket in two places. It deliberately does not treat Engine core initialization failed as an OOM signature. And it also changes serve behaviour (a reused service skips the no-GPU bail), which the ticket says is out of scope ("This changes diagnostics only, not serve behaviour").

Round note: each finding is marked new (first raised this round) or standing (raised in the previous automated round at c951af53 and still present). The previous round's report had no blocking findings, so there were no standing blockers to settle.

Blocking

• [new] The release-feature guard does not catch what its doc and commit say it catches — xtask/src/workflow_contract.rs:188-207 (multiline_run_blocks), and the_release_workflow_builds_no_feature_gated_binaries with its doc comment (added at about :2086)

  • Continuation lines are missed. The doc says it reads "joined run: | blocks so a flag on a \ continuation line is still seen", and commit 88e3d18 says all four evasions were "falsified … each is caught".
    • multiline_run_blocks only trims lines and joins them with newlines. It never merges \ continuations, and the guard tests cargo build and --features on the same line.
    • I changed release.yml:127 to cargo build --release -p rocm -p rocmd -p xtask \ with --features rocm/e2e-oom-fault-injection on the next line. The test passed.
    • With the flag on the same line, the test fails, so it is live and only the continuation shape escapes. That multi-line bash -lc '…' block is exactly where someone would add a continuation.
  • nightly.yml is not covered. The doc says "Nothing shipped may be built with a test-only feature", but the guard reads only release.yml.
    • nightly.yml:123 and :226 build the same shape (cargo build --release -p rocm -p rocmd -p xtask).
    • That workflow publishes a public prerelease (gh release create … --prerelease, publish-nightly).
  • A glued -Frocm/… also slips past the w == "-F" check.
  • Confidence 100 · logic · Fix: join \-continued lines before scanning, and run the same check over nightly.yml. Otherwise narrow the doc and the "falsified against all four cases" claim to what is actually checked.

• [new] Two branches of the new reuse pre-gate are pinned by no test — apps/rocm/src/main.rs:20028-20036 (any_live_managed_service_for_model), tests at main.rs:30755-30880

  • The liveness filter. I replaced && managed_service_is_live(record) with && true and ran cargo test -p rocm --bin rocm -- reuse_pregate: all 4 tests passed.
    • That filter is what stops a stale or stopped record from pulling ensure_self_managed_engine_ready (an install) and ResolveModel in front of the no-usable-GPU bail. The fail-fast contract this change says it "made true" rests on it.
  • The record.model_ref arm. Dropping it also leaves all 4 green.
    • The "exact ref" case's canonical id qwen-canonical already matches qwen through the lenient relation, so that arm is never needed. A worker found this by mutation; I re-derived it from the fixture.
  • Confidence 100 · logic · Fix: add a stopped-record case that must return false, and a case where only model_ref matches.

• [new] The PR description misstates the change — PR body

  • Stale stacking. It says "Depends on #251 … targets that branch; it must not merge before #251 does".
    • #251 merged on 2026-10-06, and this PR targets main.
  • Wrong ordering claim. It says index validation "now runs immediately after the no-usable-GPU pre-flight and before any runtime or engine work; a GPU-less host still refuses with 'no usable AMD GPU' first".
    • When a live service with a matching name exists, serve() runs validate_engine_selection_runtime, ensure_self_managed_engine_ready and ResolveModel before both the bail and --gpu validation (main.rs:6566-6600).
    • On reuse the bail is skipped entirely (main.rs:6637).
  • Wrong CI claim. It says "Every lane is green at this head".
    • At 88e3d18, both "E2E tests (Strix Halo, Ubuntu)" lanes are CANCELLED and "E2E tests (MI300X)" is QUEUED.
    • The text describing the WSL2 lane as a "24h runner timeout" is also stale: that lane is now SUCCESS.
  • Unmentioned work. The new release.yml feature guard (a1e15a8, 88e3d18) is not described.
  • Behaviour change described as a fix. "Reuse skips the GPU bail" is a new serve behaviour that the ticket rules out of scope. The description presents it as a fix to existing reuse detection, but at prw-base the bail runs first and there is no pre-gate.
  • Confidence 85 · architectural · Fix: update the description (the change itself does not need to grow).

Non-blocking

• [standing] Skipping the bail is not tied to an actual reuse (TOCTOU) — main.rs:6598-6600, 6637, 7440-7470

  • reuse_existing is decided unlocked. spawn_managed_engine_child re-checks under the launch lock and falls through to a real launch if the service died in between.
  • That launches an engine on a GPU-less host with the bail skipped. Only the engine's own backstop catches it.
  • Confidence 75 · architectural · Fix: when the bail was skipped for reuse, refuse in the spawn path unless it returns AlreadyRunning (or re-check the no-GPU condition under the lock).

• [standing] The serve() comment overstates when engine work precedes the bail — main.rs:6608-6622 against main.rs:23272-23285

  • The comment says the work runs first "only when this invocation is about to reuse it".
  • The doc on service_model_names_match in this same diff says qwen matches a live qwen3-8b-instruct. In that case the install and ResolveModel run, the canonical ids then differ, and the bail fires after the work.
  • The matcher doc governs, because the matcher is what the code calls.
  • Confidence 85 · mechanical · Fix: narrow the comment to "when a similarly-named live service exists".

• [standing] Wrong scenario cross-references — tests/e2e-cucumber/features/model_serving.feature, the comments above serve-25 and serve-26

  • "Positive counterpart of Scenario 23" and "scenario 23 plants the SAME model" both mean serve-24. serve-23 is the low-VRAM plan scenario and plants nothing.
  • Confidence 90 · mechanical · Fix: refer to @id:serve-oom-memory-guidance rather than a positional number, since the numbers have already drifted twice.

• [new] Docs made stale by moving the classifier

  • crates/rocm-core/src/diagnose.rs:1553-1561 and :1637-1644 still say the engine calls vllm_oom_symptom_is_diagnosable to decide between routing and the canonical fallback. The fallback is now reached only through quotable_in_single_quotes.
  • crates/rocm-core/src/lib.rs:8184-8190 says the pre-launch low-VRAM note shares this classification. That note is driven by VRAM telemetry and never calls it.
  • Confidence 85 · mechanical · Fix: update the three doc comments.

• [new] docs/vllm.md no longer matches the hint's reach — docs/vllm.md:236-248

  • "When a startup failure log shows an out-of-memory error, the failure message suggests…" now overstates. A bare HIP error: out of memory gets no hint (sub_threshold_lines_carry_no_hint_at_all).
  • The serve-summary note reaches only an interactive TTY. The plain path prints no notes (the comment at main.rs:6908).
  • AGENTS.md §5 asks that behaviour claims be kept in sync across docs.
  • Confidence 75 · mechanical · Fix: say "allocator-shaped OOM errors" and state the interactive-only scope.

• [standing] serve-24 cannot fail on what its title and Then claim — model_serving.feature:316, serving_steps.rs assert_no_oom_memory_guidance

  • A worker made append_oom_serve_note always return early: serve-24 stayed green while serve-25 went red.
  • What serve-24 actually discriminates is the reuse bypass of the GPU bail. The comment discloses this, and the unit test append_oom_serve_note_ignores_an_already_running_services_log pins the clause.
  • Confidence 90 · logic · Fix: retitle the scenario to the behaviour it proves, or give it a premise that reaches the clause.

Decisions for the author

• Matching rocm diagnose's threshold drops a real OOM form — tradeoff

  • The gain: no false memory advice for kernel OOM-killer lines or bare substrings.
  • The cost: RuntimeError: HIP error: out of memory, a genuine PyTorch-on-ROCm allocation failure, scores 25 and now gets no hint at all, where it used to get the canonical one.
  • The ticket also listed Engine core initialization failed as a signature to detect; the PR excludes it on purpose.
  • The alternative is to raise the scorer's weight for that HIP form, so both surfaces keep sharing one rule.

• Reuse skipping the no-GPU bail exists mainly so serve-24 can run on the GPU-less lane — non-blocking-improvement

  • The real user case (a live service, then a later invocation with a different HIP_VISIBLE_DEVICES) is plausible but nothing cites it.
  • The cost is the pre-gate itself:
    • engine work before the bail on lenient name matches;
    • the TOCTOU above;
    • --gpu validation now running after the install;
    • about 100 lines of comments explaining the ordering.
  • An alternative is to keep the bail first and reach the reuse summary through the fault-injection seam, which already exists for serve-25.

• The OOM note only fires when vLLM dies within the 45 s readiness wait, and only on a TTY — tradeoff

  • wait_for_service_http_ready_with_progress never checks the process. An OOM after 45 s (for example, during weight loading for a large model) renders a summary with no note.
  • Nothing else surfaces the guidance afterwards; rocmd recovery does not read the log for OOM.
  • Within this ticket's scope that is acceptable. It is worth stating so it is not mistaken for full coverage.

Positive signals

• quotable_in_single_quotes and vllm_oom_diagnose_symptom now live once in rocm-core. Both surfaces that print rocm diagnose --symptom '…' share one selector and one guard, which closes the twin-implementation drift.
• The fault-injection seam compiles out to a const fn returning false. Clippy is clean with and without the feature.
• The capability gate makes a missing hook a reported skip rather than a silent pass.
• The OOM_FAULT_INJECTION_ENV producer/consumer literal is hoisted into e2e-report, so xtask and the harness cannot drift.

Deployment notes

• Behaviour change: on a host with no usable AMD GPU, rocm serve <model> now reuses a live managed service for the same engine and model instead of refusing.
• Behaviour change: the vLLM startup-failure message no longer carries the memory hint for sub-threshold OOM lines, such as HIP error: out of memory or a bare torch.cuda.OutOfMemoryError.
• A new test-only cargo feature, rocm/e2e-oom-fault-injection, must never be enabled in a release or nightly build. Today's guard does not fully enforce that (see Blocking).

What this covered

• Diff reviewed: git diff prw-base...HEAD, 18 files, every changed hunk read completely. The range is merge-base 1845df3 … head 88e3d18 (prw-base tip f9a8011). Workers also read the surrounding code:
  • serve() in full, plus spawn_managed_engine_child, start_managed_service, run_attached_service, load_managed_services and the readiness wait;
  • the diagnose vLLM-OOM checker;
  • the release.yml, nightly.yml, ci.yml and e2e-selfhosted.yml workflows.
• Fan-out: three code workers grouped by coupling (OOM core; serve CLI; e2e/xtask), one worker for the four source-reading passes (agent instructions, history, prior changes via the #251 review comments, code comments), and one design worker.
• Shared checkout: the workers mutated the same checkout concurrently, so I re-ran the liveness-filter mutation alone. A merge of prw-base into the head is clean.
• Ticket: EAI-8059 was retrieved (title, body and state; it is now keyed ROCMAI-419).
• Runs:
  • cargo test -p xtask on the guard, with mutations of release.yml;
  • cargo test -p rocm --bin rocm -- reuse_pregate, with mutations;
  • targeted rocm-core, rocm and xtask tests;
  • clippy on rocm with and without the feature;
  • serve-24, -25 and -26 against a locally built feature binary, which all pass (on a WSL host, not the actual mock lane).
• Did not run: the full workspace test suite and the full e2e suite. I relied on CI for those (build-and-test, windows-build-and-test, clippy, E2E mock lane, Windows and WSL2 lanes are all green).
• CI gaps at this head: both Strix Halo Ubuntu lanes were cancelled and MI300X is queued, so no GPU-Linux lane has run.
• Discussion: not reconciled against the PR's discussion here; its comments and earlier reviews were not read.
• Prompt injection: no prompt-injection content was found.

[volen-silo]

I reviewed this PR at a513aa6 and found three issues to address before merge: allocator OOM guidance can disappear after a progress-bar carriage return, the release-feature guard misses ordinary command formatting, and the required behavior documentation is missing. Details are inline.

This was a read-only diff and source review. I reproduced the feature guard false passes using its loop logic, but did not run PR-head Rust tests or a GPU launch. Three CI jobs remain queued, so final CI validation is still pending.

[volen-silo]

The synthetic vLLM anchor is attached to a physical line, but the diagnostic splits that line again with terminal::rendered_lines. For example, Loading weights: 90% torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB. becomes an anchored progress segment and an unanchored allocator exception. The exception is discarded, so vllm_oom_diagnose_symptom returns None and both the engine hint and serve-summary note disappear. The previous engine detector retained the OOM hint and used the canonical symptom for this control-bearing input. Please apply the vLLM context to each rendered segment while preserving safe command rendering, and add a regression case for a progress-bar CR followed by the allocator exception.

[volen-silo]

This loop checks physical lines, so a normal shell continuation defeats the new guard: cargo build --release -p rocm -p rocmd \ followed on the next line by --features rocm/e2e-oom-fault-injection increments builds but leaves offenders empty. The adjacent comment explicitly promises to catch this shape. Compact -Frocm/e2e-oom-fault-injection also passes the exact w == "-F" check. I reproduced both false passes using this loop logic; current release build commands do not enable the hook. Please inspect logical commands with continuations joined, handle compact -F arguments, and add fixtures covering these forms.

[volen-silo]

This adds observable serve behavior, and the reuse pre-gate also introduces an exception to the no-usable-GPU refusal. AGENTS.md section 5 requires README.md, the command help/doc comment, docs/testing.md, and docs/manual-testing.md to be updated in the same change. None of those four surfaces changes here; the docs/vllm.md addition only explains the shared quoting guard. Please document the OOM note conditions and interactive-only scope, the no-GPU reuse exception, and the focused automated/manual checks, including the fault-injection capability and prebuilt-lane skips.

[r0x0r]

@volen-silo — you were right, and the four surfaces are now updated in f49f670c.

README.md — two new subsections under Model serving:

• When a serve runs out of GPU memory states the note's conditions as a list, each one falsifiable: vLLM only; the interactive deployment summary only (summary_mode is background && stdout().is_terminal(), so a piped/redirected run takes print_managed_launch_plain, which prints readiness: <status> and carries no notes at all, and --verbose/--foreground streams the traceback instead); only when the service failed to become ready (status starting — a ready or still-loading running service is healthy); only on a real allocator signature (HIP out of memory, CUDA out of memory, hipErrorOutOfMemory, or torch.OutOfMemoryError corroborated by an allocator message — a kernel OOM-killer line does not qualify); and only when this invocation launched the process. On the reuse path I wrote the mechanism rather than the conclusion: the reuse summary carries no log path at all, so the running service's log is never read and the note cannot fire there whatever that log contains — with rocm services logs <service-id> as the thing to do instead. I also recorded that the shared --gpu-memory-utilization hint is de-duplicated as a fragment, not as a suppression of the note.
• Reuse is exempt from the no-GPU refusal says it plainly, as you asked: this is a change to a refusal users rely on, and on a GPU-less host rocm serve <model> can now succeed where it previously always failed. It then bounds the exemption — same engine and model, launches nothing, pins no GPU; a live service for a different model or engine still fails fast and prepares nothing.

rocm serve --help — the doc comment on Command::Serve (apps/rocm/src/main.rs:421) gains two paragraphs covering the same two things in short form. Rendered output verified on Linux; -h is unchanged (clap still shows only the summary line).

docs/testing.md — new section Serve OOM Guidance And The Reuse Exemption: the focused cargo test invocations for the classifier/selector (rocm-core), the note and its quoting guard, the append_oom_serve_note_ gate and the reuse_pregate_ pre-gate, and the two release-guard tests. It records that append_oom_serve_note_ignores_an_already_running_services_log is the only check that discriminates the already_running clause, since the E2E reuse scenario goes through a path with no log path and the note is withheld on that ground alone. Then serve-24/25/26 with their @requires-* tags, and a subsection on the fault-injection capability: the e2e-oom-fault-injection feature armed by ROCM_E2E_SIMULATE_OOM_LAUNCH=1, that only cargo xtask e2e builds with it and only it sets ROCM_E2E_OOM_FAULT_INJECTION=1, that this is the one capability the harness cannot probe for, and that every prebuilt-ROCM_CLI_BINARY lane (self-hosted, nightly, and the Windows lifecycle lane in ci.yml) therefore resolves serve-25 to a reported skip rather than a silent pass — it gates PRs on the mock lane. Every test filter in that section was run on Linux (see the caveat below).

docs/manual-testing.md — new section 5, vLLM Serve Memory Guidance And The Reuse Exemption (5–7 renumbered to 6–8): 5a drives a real OOM with --gpu-memory-utilization 1 and checks the note's content, including pasting the printed rocm diagnose --symptom back and confirming it reports a cause and stays one balanced quoted argument; 5b walks the cases where the note is correctly absent so its scope is not mistaken for flakiness; 5c checks the reuse exemption and the refusal it does not change, using HIP_VISIBLE_DEVICES= to stand in for a GPU-less host — reuse goes through, a different model/engine is refused with no Preparing lemonade for GPU serving… and nothing downloaded, and after stopping the service the masked run refuses again.

docs/vllm.md is left as-is; it already carries the shared quoting guard and the flag itself, and the new README section links to it rather than restating it.

Your other two findings were fixed in fd34ad77, both with tests that fail without the fix: the carriage-return one by splitting the tail with terminal::rendered_lines and anchoring each segment, pinned by the Loading weights: 90%\rtorch.OutOfMemoryError: … case in vllm_oom_diagnose_symptom_selects_the_failing_line_or_reports_no_oom (which asserts the exception segment is quoted, not the progress-bar segment that carried the anchor); and the release-feature guard by joining \ continuations before matching and by catching -F in both its detached and compact spellings, plus --all-features, pinned by the_release_workflow_builds_no_feature_gated_binaries.

One thing I did not fix, and it needs your eye. That first fix has a side effect that is currently red on this branch. rendered_lines strips terminal control sequences, so vllm_log_segments now hands quotable_in_single_quotes an already-clean string and the canonical-symptom fallback never fires for a colourised line. serve_summary::tests::control_bytes_from_the_log_never_reach_the_printed_command fails at f28718c7 — it gets "vllm: RuntimeError: HIP out of memory" where it asserts VLLM_OOM_CANONICAL_SYMPTOM — which is exactly the "stripped into a lookalike" outcome that test was written to reject, and it contradicts what docs/vllm.md in this PR says a control-byte-bearing line does. I reproduced it on Linux at f28718c7 with my commit absent, so it is not from the docs change, and CI's Test (affected crates) is already failing on it. I left it alone because choosing between "quote the stripped line" and "fall back to canonical" is a behaviour call that belongs to this PR's author, not to a docs pass. Everything else I cite in docs/testing.md passes on Linux, and cargo clippy --locked --workspace --all-targets -- -D warnings is clean at f49f670c.

[r0x0r]

@volen-silo — correcting the record on something a follow-up commit claimed, because it was wrong and it was my regression.

The agent that added the docs reported serve_summary::tests::control_bytes_from_the_log_never_reach_the_printed_command as pre-existing and independent of its change. The second half is true; the first is not. It ruled out its own commit, not mine. Bisected on Linux:

commit	result
a513aa67 (before my CR fix)	1 passed
fd34ad77 (my CR fix)	1 failed

So I broke it, in the very commit that fixed your first finding.

Mechanism. My fix routed each log line through terminal::rendered_lines to match how the diagnostic re-splits. That fixed the anchoring — but rendered_lines also discards escape sequences and control bytes, and the segment it returns becomes the --symptom argument. A colourised OOM line therefore reached quotable_in_single_quotes already sanitised, was never rejected, and VLLM_OOM_CANONICAL_SYMPTOM stopped firing. The printed command carried a stripped lookalike instead of refusing the line.

That is precisely what the test exists to catch, and its own comment warns against a stripper that "leaves 31m/0m behind". Your review said to apply the vLLM context to each rendered segment "while preserving safe command rendering" — I did the first half and broke the second.

Fix (a5e259a0): segment on a raw \r rather than rendered_lines. Each segment still gets its own vllm: anchor, so the progress-bar case stays fixed, but the bytes survive into the symptom so the quotability guard can still reject them. Scoring is unaffected — the diagnostic strips its own copy regardless.

Verified on Linux at a5e259a0:

• control_bytes_from_the_log_never_reach_the_printed_command — 1 passed
• rocm-core vllm tests including the CR regression case — 20 passed
• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo test --workspace --all-targets — only the known engines/vllm ETXTBSY flake (Text file busy launching a temp python stub), which passes in isolation and is unrelated to this PR

How I caused it is worth stating plainly: after fixing your first two findings I ran the two tests I had just written instead of the suite, so neither this nor a clippy redundant closure was caught before pushing. Both are fixed; all three of your findings now stand addressed.

[rominf]

🔴 Automated review · pr-review-watcher · a5e259a

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.
Ticket EAI-8059 (now ROCMAI-419): this PR implements its post-failure OOM note. It also changes serve behaviour (reuse now skips the no-GPU refusal), which the ticket rules out: "This changes diagnostics only, not serve behaviour." The description has several mismatches; they are listed under Blocking.

Round note: each finding is marked new (first raised this round) or standing (raised at the previous automated round at 88e3d18e and still present). That round's three blockers were settled as follows. The pre-gate test gap and the description mismatch were re-found independently by this round's blind review. The release-feature guard was not re-found; it was put to the reviewer after its review ended (an anchored check, not a blind re-finding). That check found parts (1) continuation lines and (3) the compact -F spelling fixed, and part (2) still holding. It is appended last under Blocking as standing, narrowed to that part.

Blocking

• [new] Required CI check Sphinx docs build (-W) fails because of a link this PR adds — README.md:624
  The failing job log (run 37924095626) has exactly one warning: README.md:9799: WARNING: 'myst' cross-reference target not found: 'docs/vllm.md#shared-or-busy-gpus'. The README is pulled into the Sphinx tree with {include}, and the docs config cannot resolve that fragment. README.md:41 links the same file without a fragment and builds fine.
  Confidence 100 · mechanical · Fix: drop the #shared-or-busy-gpus fragment, or use the absolute GitHub URL form the README already uses at line 209.

• [standing] The PR description does not match the change — PR body

  • It says "Every lane is green at this head; there are no failing checks." The Sphinx check above is failing.
  • It says "Depends on #251 … targets that branch". The base is main, and #251 is already in prw-base (5ed9365).
  • It says "One shared helper (rocm_core::vllm_log_shows_oom) feeds both the engine hint and the CLI note". No production code calls that function (see the dead-code finding below). Both surfaces use vllm_oom_diagnose_symptom.
  • It presents the reuse detection ahead of the GPU bail as already existing. It is new here: prw-base's serve_cmd.rs has no reuse check before the bail. The README this PR adds calls it "a real change to a refusal you may be relying on", but the description never says so, and the ticket scopes the work to diagnostics only.

  Confidence 85 · architectural · Fix: correct the description. State the reuse/no-GPU exemption as a behaviour change, or move it to its own PR (see Decisions).

• [new] The --help text and docs promise that nothing is prepared before the no-GPU refusal; the code breaks that for overlapping model names — apps/rocm/src/main.rs:441-445, README.md:659-673, docs/manual-testing.md:393-399, apps/rocm/src/serve_cmd.rs:678

  • The pre-gate any_live_managed_service_for_model uses service_model_names_match, which is a two-way contains.
  • So a live qwen3-8b-instruct service lets rocm serve qwen pass the pre-gate. On a GPU-less host it then runs validate_engine_selection_runtime (which can write a runtime manifest), ensure_self_managed_engine_ready (which prints "Preparing…" and can install) and a ResolveModel call, all before the refusal.
  • The code admits this in two places: the doc on service_model_names_match, and serve_cmd.rs:752-758 ("it is not 'nothing runs before the pre-flight'").
  • serve_cmd.rs:678 ("reads the managed-service records and nothing else") contradicts that later comment.
  • The code governs here: its leniency is a recorded, deliberate choice. The user-facing text is what is wrong.
  • AGENTS.md §3 requires promises that something will not happen to be backed by the behaviour. Scenario serve-26 only uses a model name that does not overlap, so this case is never exercised.

  Confidence 85 · logic · Fix: qualify the help text, README and manual-testing step 5c.3 for overlapping names, and fix the line-678 comment. Alternatively, tighten the pre-gate so the promise becomes true.

• [new] In real use, the serve-summary note never quotes the user's own failing line — apps/rocm/src/main.rs (append_oom_serve_note), crates/rocm-core/src/lib.rs:4507 (vllm_oom_diagnose_symptom), engines/vllm/src/process.rs:776-783, README.md:618-621, docs/vllm.md:258-261

  1. The managed supervisor's stderr is attached to record.log_path (attach_background_stdio, main.rs:4019).
  2. When vLLM OOMs, the engine bails with startup_log_context, which includes its own hint. exit_code_for (main.rs:1425) then writes Error: {e:?} into that same log.
  3. The engine reports the failure within milliseconds, while the CLI waits out its full 45s readiness window. So whenever the OOM line is in the log, the engine's hint is there too, after it.
  4. That hint's last line, For conditional remediation, run `rocm diagnose --symptom 'vllm: torch.OutOfMemoryError: HIP out of memory…'`., carries the same allocator keywords and clears the scorer.
  5. .rev() therefore selects that line. It contains ' and backticks, so quotable_in_single_quotes rejects it and the note always falls back to VLLM_OOM_CANONICAL_SYMPTOM.

  The README ("rocm diagnose --symptom '<your failing line>'"), docs/vllm.md ("builds the same command from the same engine log"), the description and the oom_memory_note doc all claim the user's real line. The guidance itself stays correct, because the canonical symptom is diagnosable. The fault-injection fixture (simulate_oom_managed_launch) writes only the raw OOM line and none of the engine's error block, so serve-25 cannot see this.
  Traced statically on the Linux spawn path; not run. Confidence 85 · logic · Fix: ignore lines from rocm's own hint (or read only up to the engine's Error: block) when choosing the symptom. Make the fixture include the engine's error block, or narrow the claims.

• [new] Splitting the log only on \r does not keep the classifier in step with the diagnose scorer, despite what its doc says — crates/rocm-core/src/lib.rs:4440-4467, consumed via diagnose.rs:1628-1652, terminal.rs:280-322

  • vllm_log_segments adds the vllm: anchor per \r segment.
  • vllm_anchored_lines then re-splits with rendered_lines, which also breaks on every non-SGR CSI (ESC[2K, ESC[1A), ESC E/ESC D, VT/FF, U+0085 and U+2028/2029.
  • Take a rich-style repaint …90%\r\x1b[2Ktorch.OutOfMemoryError: HIP out of memory. The segment becomes vllm: \x1b[2Ktorch…, which renders as ["vllm: ", "torch.OutOfMemoryError…"]. Only the empty anchored piece is scored, so the tail reads as "no OOM", and both the engine hint and the summary note disappear.
  • The doc's promise ("keeps the two sides agreeing about where this line ends") holds only for a bare \r. Tests cover only \r.

  Confidence 70 (argued from the code, not run) · logic · Fix: for scoring, anchor each rendered_lines piece; keep the raw segment only for the quoting check. Add table rows for \r\x1b[2K, \x0c and U+2028.

• [new] vllm_log_shows_oom is a new pub fn that no production code calls — crates/rocm-core/src/lib.rs:4436; callers are only tests (lib.rs:7692-7761, engines/vllm/src/process.rs:1069-1366, apps/rocm/src/serve_summary.rs:507-525)
  The production paths gate on vllm_oom_diagnose_symptom(..) directly, and their own comments say calling vllm_log_shows_oom first would be "a branch that can never be taken". Its doc also wrongly says the pre-launch low-VRAM note uses this rule; that note is driven by VRAM telemetry. Two serve_summary.rs tests (oom_signatures_are_detected_case_insensitively, unrelated_failures_are_not_flagged_as_oom) only re-test this rocm_core function from apps/rocm.
  Confidence 85 · mechanical · Fix: delete it, along with its mention in docs/testing.md, and point the tests at vllm_oom_diagnose_symptom(..).is_some(). Remove the duplicate serve_summary.rs tests.

• [standing] The reuse pre-gate tests leave two of its three conditions untested — apps/rocm/src/main.rs (reuse_pregate_admits_the_same_model_including_a_short_spelling, reuse_pregate_admits_the_model_id_the_oom_reuse_scenario_plants)

  • In every positive case canonical_model_id also matches the query ("qwen-canonical" contains "qwen"; the other cases use identical fields). Deleting the record.model_ref check stays green.
  • No test plants a dead or stopped record, so deleting managed_service_is_live(record) also stays green.
  • Verdict 1: all four tests fail with the function removed.
  • Verdict 2: two of its three conditions survive being broken.

  Confidence 85 (read, not mutated) · logic · Fix: add a case where only record.model_ref matches and one with a non-live record.

• [new] The Given step for serve-25 does nothing; the setup happens in the When step — tests/e2e-cucumber/tests/e2e/serving_steps.rs:1451-1459
plant_oom_launch has an empty body. The fault is actually armed by ROCM_E2E_SIMULATE_OOM_LAUNCH in the When step (:1476), so the Given text "a managed vLLM launch will run out of GPU memory" sets up nothing, which breaks the Given/When/Then pattern the suite uses.
  Confidence 70 · mechanical · Fix: have the Given record the arming in the world (for example world.command_env), and have the When step consume it.

• [standing] The release-feature guard's doc claims more than it checks: nightly.yml's published builds are not covered — xtask/src/workflow_contract.rs, the_release_workflow_builds_no_feature_gated_binaries and its doc comment

  • Settled by putting the earlier objection to the reviewer after its review ended (anchored, not re-found blind). Parts (1) and (3) of that objection are fixed: logical_commands joins \ continuations, and the compact -F spelling is caught.
  • Still holding: the doc opens "Nothing shipped may be built with a test-only feature", but the test reads only read_workflow("release.yml"). nightly.yml:123 and :226 build cargo build --release -p rocm -p rocmd -p xtask and publish a public prerelease (gh release create … --prerelease). Nothing guards those builds. They carry no features today.
  • A wholesale scan of nightly.yml would fire on its legitimate --features rocm/e2e-test-hooks e2e builds (:464, :580, :684, :899, :1131, :1550), so a fix has to target the publishing jobs only.

  Static, not run · logic · Fix: extend the check to nightly.yml's publishing builds, or narrow the doc to release.yml.

Non-blocking

• [standing] Reuse is decided without the lock, so a service that dies before launch defeats the fail-fast refusal — apps/rocm/src/serve_cmd.rs:~700-735, apps/rocm/src/main.rs:4136
  If the service dies between reuse_existing being set and spawn_managed_engine_child's own re-check, the code writes a record and spawns a child on a GPU-less host. The engine's own refusal (engines/vllm/src/process.rs:354-365, engines/lemonade/src/state.rs:368-383) stops it late, instead of the clear early message.
  Confidence 85 · logic · Fix: once the launch lock is held, rerun the no-GPU bail if reuse no longer applies.

• [new] When a live service matches the model name, pre-gate errors now surface before --gpu validation and the refusal — serve_cmd.rs:~700-735 vs the kept comments at serve_cmd.rs:~795-800 and main.rs validate_pinned_gpu_index ("serve()'s fail-fast normally reports this first")
  A runtime-manifest or ResolveModel failure now reports first.
  Confidence 70 · logic · Fix: validate --gpu before the pre-gate, or reword those comments.

• [standing] The engine's OOM hint now fires on fewer lines, and docs/vllm.md still describes the broad trigger — engines/vllm/src/process.rs (oom_utilization_hint), docs/vllm.md:~253
HIP error: out of memory, a bare torch.cuda.OutOfMemoryError and the kernel OOM-killer line now get no hint at all. The doc still says "When a startup failure log shows an out-of-memory error, the failure message suggests…".
  Confidence 70 · mechanical · Fix: state the narrower trigger in the doc.

• [new] quotable_in_single_quotes doc says the user's line stays visible beside the command, which is false for the new serve-summary note — crates/rocm-core/src/terminal.rs:194-196
oom_memory_note prints neither the line nor a log tail.
  Confidence 85 · mechanical · Fix: limit that sentence to the engine hint.

• [standing] Feature-file comments point at the wrong scenario number — tests/e2e-cucumber/features/model_serving.feature:~322 ("Positive counterpart of Scenario 23"), :~353 ("scenario 23 plants the SAME model")
  serve-23 is the low-VRAM plan scenario. The reuse scenario is serve-24, which is what docs/testing.md correctly says.
  Confidence 100 · mechanical · Fix: refer to @id:serve-oom-memory-guidance rather than a number, since the numbers keep drifting.

• [standing] serve-24's title says it guards attribution, but its check cannot fail on the OOM-note logic — model_serving.feature:~316, serving_steps.rs (assert_no_oom_memory_guidance, assert_reused_running_service)
  The real reuse path reports log_path: None, so deleting append_oom_serve_note keeps it green. The comment admits this, but the scenario title still claims the guard. assert_reused_running_service also matches the bare text alreadyrunning.
  Confidence 70 · mechanical · Fix: retitle the Then step to what it proves, and match the summary heading.

• [new] The release-feature guard's spelling test copies the guard loop instead of calling it — xtask/src/workflow_contract.rs:~2609 (caught closure)
  The closure duplicates the loop body of the_release_workflow_builds_no_feature_gated_binaries, so a change to the real loop would not turn the spelling test red. Separately, the guard scans both the run blocks and the whole file, so every build is counted twice.
  Confidence 70 · mechanical · Fix: extract one offending_builds(text) and call it from both tests.

• [new] Prebuilt e2e lanes are called "shipping"/"release" binaries, but they build with rocm/e2e-test-hooks — tests/e2e-cucumber/README.md:146, tests/e2e-cucumber/src/capability.rs:~133, src/expectation.rs:~137, xtask/src/e2e.rs:~19-25, crates/e2e-report/src/lib.rs
workflow_contract requires those hooks on the self-hosted and nightly prebuilt lanes.
  Confidence 85 · mechanical · Fix: say "a prebuilt binary, built without e2e-oom-fault-injection".

• [new] A step comment names the wrong file — serving_steps.rs:~1357
  The "Preparing for GPU serving..." line comes from apps/rocm/src/engines_cmd.rs:355, not main.rs.
  Confidence 85 · mechanical.

• [new] One test assertion can never fail — apps/rocm/src/serve_summary.rs (the_shared_hint_is_de_duplicated_without_losing_the_rest_of_the_oom_note)
  The final printed == 1 count seeds the hint itself and follows a !note.contains(hint) check, so it always holds. Another test, oom_note_renders_in_the_summary_notes, pushes the note into the summary by hand and passes with the serve_cmd.rs wiring reverted; only serve-25 covers that wiring.
  Confidence 85 · mechanical.

Decisions for the author

• Should the reuse exemption from the no-GPU refusal ship in a separate PR? — non-blocking-improvement
  It is its own user-visible change, with its own scenario (serve-26) and README section. It sits outside the ticket's "diagnostics only" scope. serve-24 needs it in order to run on the GPU-less mock lane, but that dependency argues for landing it first as a separate PR, not for bundling it here (AGENTS.md §11).

• Lenient vs exact model match in the reuse pre-gate — tradeoff
  Lenient matching keeps short-name reuse working, but lets an overlapping name run engine work before the refusal. Exact matching makes the docs' promise true, but can miss a legitimate reuse. The code records lenient as deliberate. The docs then have to say so (Blocking above).

• Piped, scripted and assistant runs get no OOM guidance — tradeoff
  print_managed_launch_plain prints only readiness: starting. A failed OOM launch then looks the same as a model still loading, and the chat assistant (an AGENTS.md §7 consumer) gets no pointer to rocm diagnose or rocm services logs. The comment at serve_cmd.rs:1047-1052 records this asymmetry. Please confirm it is still intended.

• The note only appears if the OOM is already logged when the 45s readiness wait ends — tradeoff
  A slow weight load that OOMs later gets no note, and those are the likeliest OOM cases. Status starting also stands in for "failed" while rocmd may still retry the service.

• The memory advice has two renderers — non-blocking-improvement
  Classification is now shared, but oom_utilization_hint (process.rs) and oom_memory_note (serve_summary.rs) each phrase the advice and repeat the quote-or-fallback step. The engine hint lacks the "model does not fit" branch that the note and fix-16 carry. A single rocm_core renderer would stop the two drifting apart.

• The engine hint no longer fires on sub-threshold OOM lines — answered-by-intent
  The description says: "a bare out of memory from a kernel OOM-killer line, a dependency's log, or vLLM's generic EngineCore wrapper never gets reported as memory exhaustion. One shared helper … feeds both the engine hint and the CLI note". Without that, dropping the hint for HIP error: out of memory and the bare torch.cuda.OutOfMemoryError would have been a blocking regression finding. Confirm those lines should get no guidance at all. The stale docs/vllm.md sentence is still listed under Non-blocking.

Positive signals

• e2e_report::OOM_FAULT_INJECTION_ENV is shared by producer (xtask) and consumer (harness), and xtask clears it for a prebuilt binary. A typo can no longer turn a scenario into a silent skip, and the env_remove branch is unit-tested.
• The release-feature guard asserts builds > 0, so if the build step moves, the guard fails instead of passing on nothing.
• quotable_in_single_quotes moved into rocm-core intact, with its rationale and tests, so the engine hint and the summary note share one quoting guard.

Deployment notes

• On a GPU-less host, rocm serve <model> now succeeds instead of refusing when a live managed service for the same engine and model exists. Scripts that relied on the refusal will see a different result.
• New cargo feature rocm/e2e-oom-fault-injection (test-only). Only cargo xtask e2e builds it, and release.yml is guarded against it.

What this covered

• Read every file in git diff prw-base...HEAD: merge-base 03afcf43, head a5e259a0, 22 files. Also read the surrounding serve, launch, reuse, diagnose and terminal code paths, and the engine refusal backstops.
• The fan-out ran as independent workers:
  • four coupled-group code passes: vLLM/rocm-core, the serve app, e2e/xtask, and docs
  • agent-instruction adherence, code-comments, history and prior-changes passes
  • two design workers
• History: git merge-tree against prw-base is clean, and no merge reverted main's work.
• All verification was static. Free disk read 39G, under the 40G floor, so nothing was built, tested or mutated. The only run evidence is CI's statusCheckRollup and the failing Sphinx job log. All other CI checks were success or skipped, and no GPU lanes appear in the rollup for this head.
• What was not done:
  • The vLLM/rocm-core worker did not read the method's reference files before its pass.
  • The prior-changes pass sampled only #251's review comments.
  • Mutation checks were argued from the code, not executed.
• The PR's review discussion was not read, so this report is not reconciled against it.

[volen-silo]

One additional issue, still present at 73dabf50: the three focused e2e commands in docs/testing.md:1283–1285 select no scenarios.

They pass serve-oom-memory-guidance, serve-oom-launch-memory-guidance, and serve-unrelated-live-service-still-fails-fast to -n. Those are @id tag values, while cucumber 0.23 matches -n against scenario names. I checked all three patterns against the feature at this head: each matches zero scenarios. The actual names start with serve-24, serve-25, and serve-26.

Please document unfiltered cargo xtask e2e, or add an ID selector inside the capability-aware harness filter before documenting focused ID commands. Simply replacing these with --tags or matching scenario names is not sufficient: tests/e2e-cucumber/tests/e2e.rs:1206–1209 explains that cucumber uses its CLI filter instead of the custom filter, bypassing OS, capability, and expectation resolution. This matters particularly for the fault-injection-gated launch scenario.

[rominf]

🔴 Automated review · pr-review-watcher · 73dabf5

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 (prw-base...HEAD, 22 files).
Implements the out-of-memory memory-knob note from EAI-8059 (now ROCMAI-419, Sub-task, status QA). It also changes when rocm serve refuses on a host with no GPU, which that ticket rules out: "This changes diagnostics only, not serve behaviour." The ticket also lists Engine core initialization failed as a signature to detect; the change deliberately excludes that generic wrapper.

Round note: the only commit since the previous automated round (a5e259a0) is 73dabf50, a one-line README link change. This round is still a full blind review, because several open findings (description, missing tests) are not tied to one file. Each finding is marked new (first raised this round) or standing (raised at an earlier automated round and still present). Of the previous round's nine blockers, five were re-found independently by this blind review. The other four were not re-found. Each was put to the reviewer after its review had ended. That is an anchored check, not a blind re-finding. All four still hold, and they are listed last under Blocking. This change request replaces the earlier ones as the automation's complete position.

Blocking

• [standing] The README link breaks the required Sphinx -W build — README.md:624
docs/rocm-docs/commands.md pulls in README from ## Commands to ## Contributing, so [docs/vllm.md](docs/vllm.md) is resolved inside the Sphinx tree, where no such page exists. CI at this head fails with README.md:9799: WARNING: 'myst' cross-reference target not found: 'docs/vllm.md' (check Sphinx docs build (-W), run 37930656620). The last commit, "link docs/vllm.md without an anchor Sphinx cannot resolve", did not fix it.
  Confidence 100 · mechanical · Fix: follow the README's existing convention for files outside the Sphinx tree — an absolute GitHub URL (as at README.md:35) or a plain code span (as at :921-925).

• [standing] A non-colour escape at the start of a log line hides a real OOM, and the comment saying it cannot is false — crates/rocm-core/src/lib.rs:4465-4470 (vllm_log_segments), :4436-4438, :4521-4528; crates/rocm-core/src/terminal.rs:287-293, :357-367
  Segments are split only on \n/\r and then scored as vllm: {segment}. The checker re-splits with rendered_lines, where every CSI escape except …m becomes a line break (skip_csi_body returns true only for final byte m).
  Traced case: the segment \x1b[2KRuntimeError: HIP out of memory renders as two rows, ["vllm: ", "RuntimeError: HIP out of memory"]. Only the empty first row carries the anchor, so the score is 0. Both the engine hint and the serve-summary note are then silently dropped.
  This is a regression: on base, oom_utilization_hint gated on a substring scan and fired for this line. The same applies to ESC[A, ESC E, \x0b, \x0c and \u{85}. The doc comment's "Scoring is unaffected, because the diagnostic strips its own copy regardless" holds only for colour (SGR) and string sequences. No test uses a non-SGR escape.
  Confidence 85 · logic · Fix: anchor and score each rendered_lines row of the segment, and keep the raw segment only for the quotability gate. Add a fixture with a leading \x1b[2K.

• [standing] vllm_log_shows_oom is dead in production, and the tests built on it test the wrong module — crates/rocm-core/src/lib.rs:4436; apps/rocm/src/serve_summary.rs:505-527; engines/vllm/src/process.rs:1069-1366 (test uses)
  A repo-wide grep finds only test callers, plus comments at serve_summary.rs:239, process.rs:765 and main.rs:3741 that explain why production code avoids it. Both surfaces actually gate on vllm_oom_diagnose_symptom.
  The function is new in this change and pub, so the compiler does not warn. Its doc also says it keeps "the pre-launch low-VRAM note … and diagnose" in agreement, but the pre-launch note (main.rs:~3672) is driven by telemetry, not by this function.
  The two serve_summary.rs tests (oom_signatures_are_detected_case_insensitively, unrelated_failures_are_not_flagged_as_oom) call only rocm_core::vllm_log_shows_oom, so they would pass with every serve_summary.rs change reverted.
  Confidence 85 · mechanical · Fix: delete the function and move its threshold assertions onto vllm_oom_diagnose_symptom(..).is_some(). Or, if it is meant to be the shared predicate the PR description says it is, make both surfaces call it.

• [new] Reuse skips the no-GPU refusal without checking the reused service's device policy — apps/rocm/src/serve_cmd.rs:~700-775; apps/rocm/src/main.rs:16699-16712, 16738-16747
existing_live_managed_service matches on engine, canonical model id and liveness only. The record's device_policy, which main.rs:15804 shows is stored, is never compared.
  So rocm serve <model> under the default gpu_required, on a host with no GPU or with every GPU masked by HIP_VISIBLE_DEVICES, now succeeds by reusing any live service for that model, including one launched --device cpu_only. Nothing tells the user it is not on a GPU.
  AGENTS.md §6 requires "preserve strict GPU-required behavior; do not introduce silent CPU fallback". README's "The reused service was vetted against the policy at its own launch" is true of that service's own policy, not the caller's.
  Confidence 70 (argued from the match code; I did not trace whether a CPU-only launch yields the same canonical id) · architectural · Fix: require the matched record's policy to satisfy the caller's, and its pinned GPU to still be visible. Or drop the exemption (see Decisions).

• [standing, extended] The documented reuse exemption does not match the code in two cases — README.md:657-672; --help doc comment at apps/rocm/src/main.rs:441-445; docs/manual-testing.md:390-397; docs/testing.md:1294-1297

  1. "before preparing any engine" and "those still fail fast … and prepare nothing" are false when the model names overlap. any_live_managed_service_for_model uses service_model_names_match, a two-way contains. A live Lemonade service for qwen3-8b-instruct therefore opens the pre-gate for rocm serve qwen, which runs ensure_self_managed_engine_ready ("Preparing…" plus install) and ResolveModel before the refusal. The code itself admits this at main.rs:19983-19996. serve-26 and the unit tests use only names that do not overlap.
  2. (new this round) "Reusing … is the one exception: it launches nothing and pins no GPU, so it is allowed through" is false with an explicit --gpu N. validate_pinned_gpu_index still runs after the bail and refuses when no GPU is visible (serve_cmd.rs:~802).

  The code governs, because its matching relation is deliberately lenient and documented as such. AGENTS.md §3 requires a promise that something will not happen to be asserted together with the behaviour, and §5 requires each copy of a claim to be checked against the code path.
  Confidence 85 · logic · Fix: narrow the README, --help, manual-testing and testing.md wording to "a model whose name does not overlap" and "default --gpu auto". Alternatively, tighten the pre-gate match.

• [standing] The PR description misstates the change — the PR body

  • It says "One shared helper (rocm_core::vllm_log_shows_oom) feeds both the engine hint and the CLI note". That helper has no production caller (see above).
  • It says "CI. Every lane is green at this head; there are no failing checks." Sphinx -W is failing at 73dabf5.
  • It says "Depends on #251 … targets that branch; it must not merge before #251 does". The base is main, and #251 merged on 2026-10-06.
  • The Summary does not mention the user-visible change to the no-GPU refusal. README itself calls it "a real change to a refusal you may be relying on", and the ticket rules it out. It appears only inside "Changes since the last review". The move of the quoting and stripping helpers into rocm-core/src/terminal.rs is also unmentioned.

  Confidence 85 · mechanical · Fix: rewrite the description to match the change — the helper actually shared, the current CI state, base main, and the refusal change stated up front.

• [standing] In real use, the serve-summary note never quotes the user's own failing line — apps/rocm/src/main.rs:4019-4028, :1425, :4389-4397; engines/vllm/src/process.rs:274-280, :787-791, :808-812; crates/rocm-core/src/lib.rs:4521-4528; README.md:618-621; docs/vllm.md:258-261
  Not re-found blind this round. It was put to the reviewer after its review ended (an anchored check), and it still holds whenever vLLM runs out of memory within the 45s readiness wait:

  • The supervisor's stderr and vLLM's output share one log.
  • On an early exit, the engine's error, including its own OOM hint, is the last text written (via exit_code_for).
  • The CLI waits out the full 45s without checking whether the process is alive, and only then reads the tail.
  • The newest-first scan selects the hint's own For conditional remediation, run `rocm diagnose --symptom '…'` line. That line contains ', so the note always falls back to the canonical symptom.

  The README, docs/vllm.md and the oom_memory_note doc all claim the user's real line. serve-25's fixture writes only the raw OOM line, so it cannot see this.
  Static, not run · logic · Fix: ignore rocm's own hint lines (or read only up to the engine's Error: block) when choosing the symptom. Make the fixture include the engine's error block, or narrow the claims.

• [standing] The reuse pre-gate tests leave two of its three conditions untested — apps/rocm/src/main.rs:16742, :16744; test helper reuse_pregate_for at :27623-27653
  Not re-found blind this round. It was put to the reviewer after its review ended (an anchored check), and it still holds for both conditions:

  • The helper always plants a live record (status starting, the test's own pid, :27646-27647). Deleting managed_service_is_live(record) keeps every test green.
  • In every positive case the canonical_model_id arm matches on its own. Deleting the record.model_ref arm also stays green.
  • serve-26 is closed by the model mismatch alone, so it discriminates neither condition.

  Static, not run · logic · Fix: add a case where only record.model_ref matches (e.g. a canonical id that shares nothing with the query), and one with a stopped or dead record.

• [standing] The Given step for serve-25 does nothing; the setup happens in the When step — tests/e2e-cucumber/tests/e2e/serving_steps.rs:1451-1459, :1477-1488
  Not re-found blind this round. It was put to the reviewer after its review ended (an anchored check), and it still holds: plant_oom_launch has an empty body. The fault is armed only by ROCM_E2E_SIMULATE_OOM_LAUNCH in the When step, so deleting the Given changes nothing.
  Static, not run · mechanical · Fix: have the Given record the arming in the world, and have the When consume it.

• [standing] The release-feature guard's doc claims more than it checks: nightly.yml's published builds are not covered — xtask/src/workflow_contract.rs (the_release_workflow_builds_no_feature_gated_binaries and its doc comment); .github/workflows/nightly.yml:123, :226
  Not re-found blind this round. It was put to the reviewer after its review ended (an anchored check), and it still holds:

  • The doc opens "Nothing shipped may be built with a test-only feature", but the test reads only release.yml.
  • nightly.yml:123 and :226 run cargo build --release -p rocm -p rocmd -p xtask in the jobs that stage and upload the public nightly prerelease (:175/:198, :308/:311). Nothing guards those builds. They carry no features today.
  • The other nightly builds legitimately use --features rocm/e2e-test-hooks, so a fix has to target only the publishing jobs, for example jobs that run gh release create or gh release upload.

  Static, not run · logic · Fix: extend the check to nightly.yml's publishing builds, or narrow the doc to release.yml.

Non-blocking

• [standing] The reuse pre-gate also runs on hosts with GPUs, ahead of --gpu validation — apps/rocm/src/serve_cmd.rs:701 vs :796-801
  The block's only purpose is to skip the no-GPU bail, but it runs whenever a matching live service exists. Runtime selection, Lemonade install and ResolveModel can then run, or fail, before the --gpu check. The comment at :796 says that check comes "before engine/runtime resolution".
  Confidence 70 · logic · Fix: also require visible_gpu_indices.as_deref().is_some_and(<[u32]>::is_empty).

• [new] The note can tell a still-loading server it ran out of memory — apps/rocm/src/serve_summary.rs:191-196; apps/rocm/src/main.rs:~3799-3830, ~4390, 19822; --help at main.rs:436-437
starting means "nothing answered on HTTP within 45 s", which a large vLLM model that is still loading also hits. append_oom_serve_note does not check whether the process is still alive.
  An OOM-signature line in a live server's tail would therefore produce "the serve attempt ran out of GPU memory", against the help's "never shown for a … still-loading server". The log is truncated per launch (main.rs:4228), so an earlier launch cannot be blamed. I did not establish that vLLM emits recoverable allocator-OOM lines.
  Confidence 70 · logic · Fix: pass child_pid and require the process to have exited before asserting, or soften the wording when it is alive.

• [new] The documented scenario commands select nothing — docs/testing.md:1283-1285
cargo xtask e2e -- -n serve-oom-memory-guidance passes an @id to cucumber's -n, which filters on the scenario name ("serve-24 - Reusing …"). The name does not contain the id. The working precedent at :1420 uses -n diagnose-2, which is in the name.
  Confidence 70 (based on cucumber-rs --name semantics; I could not read the crate source locally) · mechanical · Fix: use -n serve-24 etc., or a --tags '@id:…' filter.

• [standing, extended] Comments made stale by this change — tests/e2e-cucumber/features/model_serving.feature:322, :353; .github/workflows/e2e-selfhosted.yml:329-330
  The feature comments say "Positive counterpart of Scenario 23" and "scenario 23 plants the SAME model". serve-23 is the low-VRAM plan scenario; both mean serve-24.
  (new this round) The workflow comment still says --features rocm/e2e-test-hooks "must match what cargo xtask e2e builds". xtask now builds a superset and deliberately leaves the OOM hook out of prebuilt lanes; workflow_contract.rs was updated to say this and the workflow comment was not.
  Confidence 85 · mechanical.

• [standing] docs/vllm.md still says any OOM error triggers the hint — docs/vllm.md:~251
  "When a startup failure log shows an out-of-memory error, the failure message suggests…". The change narrows this to lines that clear the checker's threshold: a bare HIP error: out of memory or a kernel OOM-killer line no longer qualifies. README documents the narrowing; this file was edited in the same change but not for it.
  Confidence 70 · mechanical.

Decisions for the author

• Whether to change the no-GPU refusal in this PR at all — tradeoff
  • The feature file's own comment shows the exemption exists so serve-24 can reach the reuse summary on the no-GPU mock lane (model_serving.feature:~311-314). That comment also says serve-24 cannot tell apart the already_running clause it is about; the unit test append_oom_serve_note_ignores_an_already_running_services_log is what pins it.
  • For: a reuse launches nothing, and serve-24 then runs on every PR.
  • Against: a shipped refusal is loosened (a GPU-less rocm serve can now succeed), for test coverage, in a PR whose ticket says "diagnostics only".
  • The alternatives keep the refusal strict: unit coverage plus serve-24 on a GPU lane, or an e2e-only bail waiver like the existing scripted_backend_failure.
• The fault-injection seam proves rendering, not the launch path — non-blocking-improvement
  • The e2e-oom-fault-injection hook returns at main.rs:4347, before the spawn, the record claim, the readiness wait and status_for_readiness. serve-25 therefore proves that a hand-built ManagedLaunchReport renders the note, not that a real failed launch produces one.
  • The vLLM launcher is the runtime manifest's command (engines/vllm/src/process.rs:255). A fake launcher that writes the traceback and exits would drive the real path using only the existing e2e-test-hooks waiver, without a second feature and a second capability tag.
  • At least, say in the scenario and docs that it covers rendering only.
• Scope — non-blocking-improvement
  The PR does several things: the OOM note, the refusal change, extraction of the terminal module, ordering of --gpu validation, and the xtask release-feature guard. The refusal change is the piece that most needs its own review and discussion; consider splitting it out.

Positive signals

• quotable_in_single_quotes now lives once in rocm-core and is used by both the engine hint and the serve summary. That removes the risk of two copies of the quoting rule drifting apart. The adversarial tests (apostrophe, control bytes, Cf, Zl/Zp) pin the fallback to the canonical symptom rather than to a stripped lookalike.
• The managed log is truncated per launch (main.rs:4228), and the reuse path never reads a log, so another launch's OOM is never blamed on this one.
• The release-feature guard in xtask/src/workflow_contract.rs can actually fail: it joins \ continuations and catches --features, --all-features and both -F spellings, with a test for each.
• OOM_FAULT_INJECTION_ENV is defined once in e2e-report for both xtask (producer) and the harness (consumer). The CI mock lane log shows serve-24/25/26 executing their steps rather than being skipped.

Deployment notes

• Behaviour change: on a host with no usable GPU, under the default gpu_required, rocm serve <model> now succeeds when a live managed service for the same engine and model exists. Scripts relying on the refusal will see success.
• New cargo feature rocm/e2e-oom-fault-injection. It must never be enabled in a release build; workflow_contract.rs guards release.yml against it.

What this covered

What was read: every file in prw-base...HEAD (c1c2aeb…73dabf50, 22 files), read by six read-only workers in coupled groups:

• vLLM detection and the core terminal module;
• the serve path, summary and README;
• the e2e harness and xtask;
• docs checked against the code;
• history, prior PRs, AGENTS.md and code comments;
• three design questions.

Two of those reads were partial:

• apps/rocm/src/main.rs was read by changed hunk plus the enclosing functions and the callers and callees of changed symbols.
• For apps/rocm/src/serve_cmd.rs, the serve() body and every hunk were read in full; the unchanged helpers and its test module were not.

Verification was static only, because the shared disk is critically low. No cargo build, test, clippy, e2e or Sphinx run happened. CI results were read with gh instead:

• Sphinx -W fails. It passes on main (c1c2aeb), so this PR causes the failure.
• Every other reported check passes.
• No self-hosted GPU-lane check appears in the rollup for this commit. The new scenarios are all @requires-no-gpu, so no lane exercised them on GPU hardware.

Test verdicts (reasoned, not run):

• serve-24 would still pass with append_oom_serve_note reverted (its own comment says so), but fails with the reuse exemption reverted.
• serve-25 fails if the note is removed, but not if the status or engine gates are broken alone, because its record is fabricated.
• serve-26 fails with the pre-gate reverted.
• The two serve_summary.rs tests above pass with that module reverted.
• The rest of the new unit tests fail when their production change is reverted.

Also checked:

• Ticket EAI-8059 was retrieved: title, body and state only.
• The history and prior-change passes ran, with no reversal of work on main.
• Whether rocm-engine-vllm stays above its coverage floor after moving tests out could not be established statically.
• I found no prompt-injection content.

Not reconciled: the blind review was not compared against the PR's existing discussion. The four standing objections above were settled after it, as described in the round note.