[r0x0r]

Important

Whoever squash-merges this: write the squash body by hand, from the final
diff — do not inherit the per-commit trail. This repo's squash setting is
COMMIT_MESSAGES, and about 16 of this branch's commits narrate work that is
no longer in the diff: the reuse pre-gate and its serve-26..30 scenarios
(moved to #703), the --gpu validation / auto-select work (already on main
via #251), and 956891f's claim that the fix-16 checker is "gated to Linux"
(reversed by 13bd32c). 1de61359's message also contains one sentence that
was false when written. The final diff is: the serve-summary OOM note, the
crates/rocm-core/src/vllm_oom.rs reader, the shell-quoting guard's move into
rocm-core, the e2e-oom-fault-injection feature and the release-feature
guard, serve-28, and the docs. History is not being rewritten for this
(it would need a force-push to a shared branch).

Note

Merged with main on 2026-11-02 and re-verified. main's Phase-6x/7x
modularization moved most of what this branch touches, so several conflict
hunks had an empty origin/main side; each was re-applied into the module
that now owns the code (see the merge commit message for the file-by-file
account). main took serve-26/serve-27 for the AIM lanes, so this
branch's OOM-launch scenario is now serve-28 — the feature file, the
step file, capability.rs and docs/testing.md all moved with it.
The sections below that still describe serve-29 and the reuse pre-gate
describe work that now lives in #703, not in this branch.

Summary

OOM guidance on a failed rocm serve (ROCMAI-419, ex EAI-8059). When a
managed vLLM launch fails to become ready and its own log carries a real
allocator OOM signature, 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>'.

Note

The serve-behaviour half of this PR has been split out to #703.
Reusing an already-running managed service is now exempt from the no-GPU
refusal — a user-visible change outside ROCMAI-419's "diagnostics only" scope.
Both a human reviewer and pr-review-watcher asked for it to ship on its own
under AGENTS.md section 11, and 7bc22302 removes it from this branch. The
mechanics are unchanged; only the PR they live in moved. What this branch
loses with it is serve-25 (Reuse succeeds on a GPU-less host and the summary
carries no memory guidance), and it loses no coverage doing so: that
scenario's memory assertion was already vacuous, because the real reuse path
reports log_path: None and append_oom_serve_note is withheld on that ground
before the already_running clause is reached. The clause itself stays pinned
here by the unit test
append_oom_serve_note_ignores_an_already_running_services_log, which feeds it
an OOM-bearing log path directly.

Detection is deliberately narrow: each tail line is classified with the same
rule as the rocm diagnose vLLM-OOM checker (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_oom_diagnose_symptom, is both the OOM gate and the line
selector for the engine hint and the CLI note, so neither surface can apply a
rule the other does not. That is a shared rule, not a shared answer: the two
read the shared log at different points, and two measured consequences of that
are stated under Correction and Known gap below. An earlier version of this
paragraph claimed the two "cannot drift"; that claim was false and is withdrawn.

quotable_in_single_quotes moves out of engines/vllm/src/process.rs into the
existing crates/rocm-core/src/terminal.rs, so both surfaces share one copy of
the rule that decides whether the user's line can be put in a shell-quoted
argument. (strip_terminal_control_sequences and rendered_lines already lived
there on main; an earlier version of this paragraph described all three as
moving into a new file, which was wrong on both counts.)

Base is main. #251 merged on 2026-10-06 and is no longer a dependency.

Behavior coverage

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-28)	GitHub-hosted mock lane, gated @requires-oom-fault-injection

The four reuse-exemption scenarios this table used to list moved to
#703 with the exemption. serve-28 is this PR's only new scenario; it
sits behind main's own serve-25 (The recommended model list prints
ASCII-only text, from #683, merged after this branch forked).

@id:diagnose-vllm-oom-is-conditional and @id:serve-vllm-low-vram-oom-guidance
(serve-23) came from #251, now merged; 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.

That hook proves rendering, not the launch path: it returns before the spawn,
the record claim and the readiness wait, so serve-28 shows that a failed
launch report renders the note, not that a real failed launch produces one.

Stated gap: the vLLM engine's own startup-failure text has no scenario

AGENTS.md section 3 asks that a change to command output be covered by a Gherkin
scenario, and that a gap be stated in the PR text with the lane named. Stating it:
no scenario covers the vLLM engine's startup-failure message, and none of
today's lanes would run one.

The text in question is what a user sees through rocm services logs <service-id>, --verbose or --foreground when a managed vLLM launch dies.
This PR changes it four ways (startup_log_context / oom_utilization_hint in
engines/vllm/src/process.rs):

• the header now reads Startup log, last 80 lines as the terminal drew them;
• the tail is echoed as rendered rows rather than raw bytes;
• the memory hint no longer fires for a bare HIP error: out of memory, a kernel
  OOM-killer line, or vLLM's generic EngineCore wrapper (the deliberate
  narrowing described above, which keeps the hint in step with rocm diagnose);
• the hint now fires on \r / ESC[2K repaint lines it previously missed.

Nothing under tests/e2e-cucumber/features/ reaches that code: grepping for
exited before becoming ready and Startup log finds no match, and serve-28's
log fixture is written by the CLI's own fault-injection hook, so it never calls
the engine.

The lane that would cover it is E2E tests (MI300X) in
e2e-selfhosted.yml — the only lane
whose GPU family makes vLLM the effective serve engine, and so the only one where
a real vLLM process can be started and made to fail. It does not cover it today,
and this PR does not add a scenario there, for two reasons worth stating rather
than burying: that lane runs the prebuilt release binary, which has the
fault-injection hook compiled out by design, so the failure would have to be a
genuine VRAM exhaustion; and provoking one reliably on a 192 GB MI300X means
either a deliberately oversized model download or a --gpu-memory-utilization
value tuned against whatever else is resident, which is GPU-expensive and racy
against the other scenarios sharing that card.

What does hold the changed text today: rocm-engine-vllm's unit tests
(the_echoed_tail_never_welds_two_rendered_rows_into_one_line,
the_hint_reads_the_raw_tail_and_displays_the_row_that_scored,
the_colourised_round_trip_quotes_the_row_the_terminal_drew,
oom_utilization_hint_stays_quiet_for_unrelated_failures) exercise
startup_log_context end to end over a real file. Per AGENTS.md section 3 that
proves the function, not the behaviour, so it is a gap and not a discharge.

Known gap: the note can go missing when the OOM line is early in the window

Reported by the automated review and reproduced here (PROBE engine_hinted=true cli=None). Both surfaces use the same tail budget
(rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES = 80), but not the same window.
The engine scans the last 80 raw lines and bails; the supervisor appends that
whole bail — a header, the echoed tail, the hint — to the same shared log; the
CLI then reads 80 lines of the now-longer file. Its window therefore always
reaches less far back into the original log than the engine's did, and when the
bail is itself longer than 80 rows it holds nothing the engine wrote directly.

Measured: a log with the OOM line followed by 77 further lines is diagnosed by
the engine and produces no serve note at all.

This is not fixed here, and the reason is that the obvious fix trades one defect
for another. Re-windowing the CLI to the 80 lines before the bail restores
agreement, but it brings the raw colourised line back into view, and the note
stops quoting the user's own rendered row — the regression
the_colourised_round_trip_quotes_the_row_the_terminal_drew exists to prevent.
The clean fix is structural: have the engine record its selected symptom as data
(in the service record or a sidecar) instead of having the CLI re-parse text the
engine already diagnosed and echoed. That is a larger change than this PR, and I
would rather raise it than guess at it — say the word if it should land here.

What changed instead: the gap is now pinned by
the_cli_window_can_miss_an_oom_the_engine_saw, the comment in
append_oom_serve_note that claimed the two surfaces "agree on what counts as
'the tail'" states the gap, and the README's "absence is not evidence" list names
this condition alongside the others.

Verification

Full gate green on Linux at the pushed head 7bc22302, all four lanes:
cargo test --workspace --all-targets --no-fail-fast (3805 run across 33 test
binaries; 0 real failures — the only red was
cli_progress::tests::animated_spinner_keeps_ticking_without_progress_calls, a
known timing flake under parallel load, which passes in isolation: 1 passed, exit
0), cargo clippy --workspace --all-targets -- -D warnings (exit 0), and the
separate cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings
(exit 0), which --all-targets skips because that harness is harness = false.
cargo fmt --all clean. cargo test -p e2e-cucumber --test feature_naming
(exit 0, 4 passed) pins the renumbering.

Merged main (f0d6ce3b) in 43032a37, and two silent semantic conflicts
only a real build caught — git took main's copy of each with no marker:

• engines/vllm/src/process.rs referenced MAX_TAIL_READ by bare name in its
  test module; main had dropped that import in favour of the fully-qualified
  path. Fixed in 860f906e.
• fix(output): replace non-ASCII punctuation with ASCII in CLI output text #683 landed a whole-tree ASCII-output contract after this branch forked, and
  the OOM note's em dash plus the one in rocm serve --help tripped
  every_literal_that_can_reach_a_redirectable_sink_is_ascii and
  every_help_text_in_the_command_tree_is_ascii. Fixed in 7bc22302.

Changes in the latest round (2026-10-15 review)

The round was COMMENTED — nothing gated. Every claim was checked against the
code before acting; all of them held.

• The gpu_preferred guard from 1de61359 is reverted (blocking, and the
  automation retracted its own earlier advice that produced it). parse_device_policy
  (main.rs) maps auto, gpu, gpu_required and gpu_preferred onto
  DevicePolicy::GpuRequired and rejects cpu_only with an error, so
  GpuRequired is the only policy serve() can hand
  managed_service_reuse_refusal — its only caller that supplies one. The
  !matches!(.., None | Some(CpuOnly)) predicate was therefore identical at
  runtime to the GpuRequired check it replaced, and three artefacts
  described a bypass that cannot happen: the production comment, the test
  comment, and 1de61359's own commit message (see the banner above).

  Reverted rather than kept as defence in depth. The broader match guards
  nothing a caller can produce, it contradicts README.md's "that is the only
  policy pair refused" — and the README governs, since GpuRequired is the only
  value the CLI can pass — and this repo does not carry branches for states that
  cannot occur. The term is back on Some(GpuRequired) with the exhaustiveness
  argument written out and pointed at
  parse_device_policy_defaults_to_gpu_required_without_cpu_fallback. The two
  unreachable-input assertions go with it, one of which was a verbatim duplicate
  of the CpuOnly case already below it (a separate non-blocking finding,
  resolved by the same revert). Falsified: dropping the device-policy branch
  makes reuse_is_refused_on_every_term_the_spawn_and_the_gpu_bail_depend_on
  fail (1 failed), restored green (1 passed).

• serve-29 closes the untested seam at serve()'s pre-gate call site
  (blocking). The claim was reproduced exactly: restoring the old
  !cpu_only && any_live_managed_service_for_model(..) call site leaves the
  whole rocm bin suite green — 1170 passed, 0 failed. The unit test pins
  reuse_pregate_opens but re-derives the host predicate in a local closure, and
  serve-25/27/28 all run on GPU-less hosts where the gate opens either way.

  A unit test cannot close this: the host GPU probe is real hardware with no
  injection point, so the hand-off is only observable end-to-end. serve-29
  (@id:serve-live-reuse-candidate-does-not-preempt-gpu-validation,
  @requires-gpu @requires-os:linux) plants a live vLLM record matching on
  liveness, engine and canonical model id — so every pre-gate term except
  host_has_no_usable_gpu is satisfied and that predicate alone holds the gate
  shut — then serves the same model with --gpu 99. It borrows serve-27's
  setup-venv fixture, and deliberately passes no --env-id, so
  recover_setup_runtime_registration is reachable: with the gate wrongly open
  the manifest is re-filed before the refusal.

  Falsified on a real GPU host (platform=mi300x os=linux gpu=true): green as
  written (1 scenario, 6 steps passed); with the call site mutated back, red at
  exactly the discriminating step — no runtime registry manifest was written
  (1 scenario failed, 5 of 6 steps passed, recover_setup_runtime_registration
  re-filed therock-release-gfx942-1.0.0.json). Restored, green again.

• docs/testing.md was stale (blocking). The quoted serve-25 name predated its
  retitle in 1de61359 — corrected, in the passage that exists to warn that -n
  matches the scenario name. "The pre-gate has three conditions" omitted
  host_has_no_usable_gpu and cpu_only; it now documents all five, splits them
  into the match terms and the admission terms, and records serve-29 with its
  lane.

Non-blocking items taken:

• reuse_pregate_opens's doc said the gate opens "exactly when the bail is in
  play". The bail also requires !scripted_backend_failure and
  !e2e_simulate_oom_launch(), so in test-hook builds the gate can open where
  the bail cannot fire. Qualified, with why that is deliberate and why it costs
  nothing in a shipped binary.
• README.md listed "a progress-bar repaint" among the cases where the engine hint
  and the serve note print different commands. Confirmed overstated against
  every_rendered_line_break_keeps_the_gate_in_step_with_the_scorer: the log is
  split on \r, so a bare-CR repaint yields a clean segment both surfaces quote
  identically. Narrowed to repaints carrying an escape (ESC[1A, ESC[2K).
• crates/rocm-core/src/terminal.rs claimed the rejected line "stays visible in
  the human-readable sentence beside the command (and in the log tail printed
  with it)". True of the vLLM engine hint, which renders the row and prints the
  tail above it; oom_memory_note is one sentence plus the command and shows
  neither. Split per surface, pointing at rocm services logs.
• docs/manual-testing.md's colour-codes bullet cited the README as plain text;
  now a relative link, as AGENTS.md §5 asks.
• tui_driver.rs named ROCM_E2E_SIMULATE_OOM_LAUNCH as a user of
  spawn_with_env. Confirmed stale: the switch goes onto world.command_env,
  which the shared spawn path already applies, and the only spawn_with_env
  caller is the fix-prompt step in diagnose_steps.rs.

Non-blocking items declined:

• Also requiring !matches!(gpu_selection, GpuSelection::Index(_)) in
  reuse_pregate_opens. The reasoning is sound — on a GPU-less host every
  --gpu <index> is out of range, so the gate can still install Lemonade and
  write the registry before refusing. But it is a behaviour change on a path no
  finding in this round calls wrong, it needs its own scenario to be worth
  having, and this PR is already carrying one out-of-ticket behaviour change
  (see Open question). Follow-up, not this round.
• Workflow comments vs xtask features (e2e-selfhosted.yml, nightly.yml).
  Still true, still unrelated to this PR's hunks; left in Known deferrals.

Changes in the latest round (2026-10-14 review)

Three gating findings, all confirmed by reading before any change.

• The reuse pre-gate no longer does engine and runtime work before --gpu is
  validated. This was a real ordering regression this PR introduced: the gate
  was !cpu_only && any_live_managed_service_for_model(..), but its only
  product, reuse_existing, is a term of a bail that can fire only when
  visible_gpu_indices == Some([]). So on any host with an overlapping live
  service name — GPU or no GPU — validate_engine_selection_runtime (which can
  write the runtime registry), ensure_self_managed_engine_ready (which can
  install Lemonade) and ResolveModel all ran ahead of
  validate_pinned_gpu_index, and a ? from any of them pre-empted the
  GPU-specific refusal. The comments at the --gpu validation and at the
  runtime resolution that claimed otherwise were false, and are corrected.

  The bail's host predicate is now computed once and handed to both through
  reuse_pregate_opens, which takes the record probe as a closure — on a
  host with a GPU the managed-service read, and the load_managed_services
  liveness refresh it entails, is not paid at all.
  the_reuse_pregate_stays_shut_on_a_host_that_has_a_usable_gpu was proved to
  fail first: with the predicate dropped from the gate the test panics (exit
  101), and it asserts both halves — the gate is shut, and the probe is never
  consulted.

• --help no longer promises that nothing is prepared for a live server this
  run cannot reuse. A same-model service that managed_service_reuse_refusal
  rejects (recipe mismatch, or --require-api-key against a keyless server)
  still passes any_live_managed_service_for_model, so preparation has already
  run by the time the refusal is known. The README was already correct; the help
  was the copy that drifted, which made the previous round's claim that all three
  surfaces had been corrected untrue. It is true now.

• docs/manual-testing.md §5a no longer tells testers a colourised line falls
  back to the canonical symptom. That is the engine's behaviour; the note
  quotes the row the terminal drew. §5a now splits the apostrophe case from the
  colour case and names
  the_colourised_round_trip_quotes_the_row_the_terminal_drew. This was the
  fourth surface of a claim a previous round fixed on three; the tree was grepped
  for the claim's meaning (a multiline search for colour/control-byte wording
  near "canonical"/"falls back"), and README, docs/vllm.md and
  rocm-core/src/lib.rs already state it correctly — there is no fifth.

Two of the four non-gating Blocking items were taken as well:

• The reuse refusal's device-policy term now covers every policy that reaches
  the GPU bail, not just gpu_required. serve() keys cpu_only off
  matches!(policy, CpuOnly), so a --device gpu_preferred serve goes through
  the bail and could have bought its bypass from a cpu_only service. The unit
  test now discriminates gpu_preferred and cpu_only separately. (The e2e
  record writer still plants device_policy: null, so the term remains without
  e2e coverage — that half is not taken; the state it guards needs a cpu_only
  record the CLI never writes.)
• serve-25 is retitled after what it proves. The reviewer is right that the
  OOM-absence assertion cannot fail on that path — the reuse path reports
  log_path: None, so the note is withheld on that ground alone — and the
  comment now says so. What only serve-25 proves is the exemption itself.
• the_utilization_hint_states_the_bound_the_parser_accepts now names
  gpu_memory_utilization_rejects_out_of_range_and_unparsable_values rather than
  the function, as AGENTS.md §3 asks.

Left, with reasons: the duplicate tail reader (main.rs's read_tail_lines
uses BufReader::lines() and errors on invalid UTF-8, so the note silently
disappears) is real, but it is shared by six call sites including rocm services logs, so swapping it for rocm_core::tail_lines is a wider change than a
gating-fix round should carry — raised rather than guessed at. The serve-25
already_running call-site argument is still held by nothing; covering it
means a new seam, which is the thing the previous round deliberately removed.

Changes in the previous round (automated review follow-up)

• The classifier now keeps step with the diagnose scorer across the whole
  line-break set. The log was split on \n/\r, but the scorer 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/U+2029. A real progress-bar
  repaint, ...90%\r\x1b[2Ktorch.OutOfMemoryError: ..., therefore left the
  vllm: anchor on an empty row, scored 0, and silently dropped both the
  engine hint and the serve note. Scoring is now done per rendered row while the
  raw segment is still what gets returned, so the quoting guard keeps
  rejecting colourised lines and the canonical fallback keeps firing. A segment
  that renders as more than one row must contain a control byte, and
  quotable_in_single_quotes rejects every one of those — so within one read
  it cannot reach the printed command as a stripped lookalike of the user's
  error. See the correction below for the bound on that, which is narrower than
  this paragraph originally claimed.

• The note now quotes the user's own failing line in real use. The supervisor
  and the engine share one log. When vLLM OOMs the engine bails with its
  startup-log context — which embeds rocm-cli's own OOM hint — and the CLI
  writes that error back into the same file, after the real line. That hint
  scores as an OOM and contains ', so the newest-first scan always selected it
  and the note fell back to the canonical symptom in every real run: the
  headline "we quote your failing line" promise never actually held in the field.
  Lines rocm-cli authored are now skipped, keyed off shared constants so the hint
  and the filter cannot drift. The e2e fixture now writes the engine's full error
  block, so serve-28 can see this shape.

• The exact contract is now stated, because the stronger one is false.
  vllm_oom_diagnose_symptom does not guarantee every returned line is
  diagnosable, and cannot without reintroducing the control-byte hole. What holds
  is: a line that passes quotable_in_single_quotes is diagnosable, and one that
  does not is replaced by the canonical symptom, which is. That is exactly what
  the callers gate on. Found by the new break-set test falsifying the old claim.

• Dead code removed. vllm_log_shows_oom was a pub fn with no production
  caller; its threshold assertions moved onto the gate that actually ships. The
  two serve_summary.rs tests that only re-tested it now go through
  oom_memory_note, so they can fail when that module changes.

• Pre-gate test gap closed. Two of the pre-gate's three conditions were
  deletable with the suite green. Added a case where only record.model_ref
  matches and one with a non-live record; both mutants are now killed.

• Doc/comment corrections. --help, README and docs/manual-testing.md no
  longer promise that nothing is prepared before the no-GPU refusal.
  docs/vllm.md states the narrowed hint trigger. The serve_cmd.rs comment
  claiming the pre-gate "reads the managed-service records and nothing else" is
  qualified against the liveness refresh it actually does.

Correction: the raw-split guarantee is per-read, not end-to-end

The automated round at 40e7b5c6 was right that for a colourised OOM line
the engine and the CLI print different rocm diagnose --symptom arguments, and
that the code and this description both claimed otherwise. Measured:

engine, read 1 : --symptom 'vllm: torch.OutOfMemoryError: HIP out of memory'   (canonical fallback)
CLI,    read 2 : vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.

The cause is the engine's own bail. It echoes the tail as rendered rows — it has
to, that text goes to a terminal — and the supervisor appends the whole bail to
the same shared log. By the time the CLI reads, the echo is the only copy of
the failing line left in the 80-line window: on a 200-line log the raw line sits
84 lines from the end, and the echo header has scrolled out as well.

Suppressing the echo to make the two agree is not available — measured, it
leaves no OOM line in the window at all and the serve note disappears for every
real managed launch. So the guarantee is stated for what it is, a property of a
single read, and the real two-read sequence is pinned by
the_colourised_round_trip_quotes_the_row_the_terminal_drew.

The difference is benign: the CLI's argument is a row the terminal really drew,
carries no quotes or control bytes, and quotable still implies diagnosable.

Scenario numbering. An earlier round of this PR had taken serve-24 — the
landed --require-api-key reuse refusal — for a new scenario and renumbered the
landed one, leaving two references on main pointing at the wrong scenario. The
landed scenario now keeps serve-24 and the new ones take 25-28 below it, so
both the landed id and the file's positional numbering hold.

Split out of this PR

The reuse exemption now ships as #703, off current main. Nothing about
its mechanics changed in the move; the scenarios are renumbered against main
(serve-28 through serve-29 there) and the OOM-flavoured dressing on the reuse
scenario — the planted OOM log, the vacuous "does not blame this invocation for
GPU memory" assertion — stayed here as OOM work rather than travelling with it.

Review history below that discusses the pre-gate, reuse_pregate_opens,
managed_service_reuse_refusal and the former serve-25/27/28/29 is kept for the
record, but that code is no longer in this branch. The
[!IMPORTANT] banner at the top still applies: 1de61359 is still in this
branch's history and this repo builds the squash body from commit messages.

Known deferrals

Non-blocking review items not addressed here, listed so they are not lost:
reuse is still decided without the launch lock; the note can in principle fire on
a still-loading server; nightly.yml's publishing builds are not covered by the
release-feature guard; and some feature-file comments still name drifted scenario
numbers. See the review reply comment for the full list and rationale.

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

[r0x0r]

Split done — the reuse exemption now ships as #703

Taking the scoping finding: ROCMAI-419 says this changes diagnostics only, not
serve behaviour, and the reuse-exemption half is a user-visible serve-behaviour
change. Under AGENTS.md section 11 it is now its own PR, #703, branched off
current main. Mechanics unchanged — the review found them correct and
well-tested, so nothing was redesigned in the move.

Removed from this branch (7bc22302, additive commits only, no rewrite):

• apps/rocm/src/serve_cmd.rs — reuse_pregate_opens, the reuse_existing
  pre-gate block ahead of the no-usable-GPU bail, and the
  any_live_managed_service_for_model / existing_live_managed_service imports
• apps/rocm/src/main.rs — any_live_managed_service_for_model and its tests,
  and the --help paragraph describing the exemption
• apps/rocm/src/service_launch.rs — the managed_service_reuse_refusal changes
• tests/e2e-cucumber/src/mock_server.rs — the ServiceRecordOptions::engine
  override
• README's Reuse is exempt from the no-GPU refusal, docs/testing.md's
  pre-gate passages, docs/manual-testing.md section 5c
• Scenarios serve-25, serve-27, serve-28, serve-29 (this PR's old
  numbering) and their steps in serving_steps.rs

Nothing was lost by giving up serve-25. Its memory assertion was already
vacuous, and the scenario's own comment conceded as much: the real reuse path
reports log_path: None, so append_oom_serve_note returns before the
already_running clause is ever reached. Replacing the whole call with a no-op
left it green — serve-26 was the one that went red. The already_running
clause is pinned here, directly, by the unit test
append_oom_serve_note_ignores_an_already_running_services_log, which hands
it a log path that does contain an OOM signature together with
already_running: true. append_oom_serve_note's doc comment, which used to
cite that scenario, now says plainly that no E2E scenario can discriminate the
clause and why.

Numbering. #683 merged after this branch forked and took serve-25 for The
recommended model list prints ASCII-only text. After merging main (43032a37)
and removing the four scenarios, this PR's surviving OOM scenario lands at
position 26, so it is serve-26 — the number it already had, by coincidence.
Updated references: docs/testing.md (prose and the -n example),
tests/e2e-cucumber/src/capability.rs, serving_steps.rs, and the feature
file's own cross-references. cargo test -p e2e-cucumber --test feature_naming
is green (4 passed). #703 takes serve-26 through serve-29 against main.

Two silent semantic conflicts from the main merge, neither marked by git,
both caught only by a real Linux build:

• engines/vllm/src/process.rs's test module used MAX_TAIL_READ by bare name;
  main had dropped that import for the fully-qualified path (860f906e).
• fix(output): replace non-ASCII punctuation with ASCII in CLI output text #683's ASCII-output contract flagged the OOM note's em dash and the one in
  rocm serve --help (7bc22302).

Gate at 7bc22302: cargo test --workspace --all-targets --no-fail-fast
clean apart from the known animated_spinner_keeps_ticking_without_progress_calls
load flake (passes in isolation, exit 0); cargo clippy --workspace --all-targets -- -D warnings exit 0; cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings exit 0; cargo fmt --all clean.

The [!IMPORTANT] banner about 1de61359 still applies and is untouched — that
commit remains in this branch's history and the squash body is built from commit
messages. No re-review requested; the existing request stands.

[rominf]

🔴 Automated review · pr-review-watcher · ebc2671

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.
This implements the ROCMAI-419 / EAI-8059 OOM guidance: the serve-summary note, the engine hint and the rocm diagnose routing. The reuse no-GPU exemption is no longer in this branch; the description says it moved to #703. The ticket itself was not retrieved (see What this covered).

Blocking

• [standing, widened] The squash body will describe work this change does not contain (commit history from prw-base..HEAD)
  • The PR description says this repo builds the squash commit body from the commit messages. That is also the documented merge mechanic for this repo.
  • About 16 of the 77 non-merge commits describe behaviour that is absent from the final diff:
    • Reuse pre-gate, cpu_only refusal and serve-26..30 reuse scenarios: 6b6cd18, 76b0580, 1de6135, a3fb51c, d0fee30, a1e15a8, f49f670 (part). 5567142 removed all of this.
    • --gpu validation against sysfs/DRM/usable devices, and auto-select: 38257fe, af059ae, 235adb9, 7c09129, 73b8bfb, ba2b741, 7e4f664, 90ce2a6. The final main.rs diff adds only append_oom_serve_note and its tests; no validate_pinned_gpu_index or usable-set change remains.
    • 956891f, the already-merged #290: its message says the fix-16 checker is "gated to Linux". 13bd32c later reversed that.
  • The description's banner asks the merger to fix one false sentence in 1de6135. The problem is much wider: the commit landing on main would claim a GPU-validation and reuse-exemption change that it does not make.
  • Confidence 85 · architectural · Fix: at merge time, supply a squash body written from the final diff (OOM note, vllm_oom.rs reader, quoting guard move, fault-injection feature and release guard, serve-26, docs). Do not inherit the per-commit trail.
  • Driver note — why this now gates when the a3fb51cd round did not:
    • That round gated nothing on one false sentence in 1de61359, expecting the merger to reword it.
    • Two things are new. Since the split, about 16 commits narrate work that is not in this diff. Some of it is in #703; validate_pinned_gpu_index is already on prw-base, so those commits re-narrate #251.
    • The repository's squash setting is COMMIT_MESSAGES, read from the API. The driver knows of no merge-queue route that edits the body, so the banner's "reword at merge" cannot be relied on, and these sentences reach main.
    • What lifts this block: rewrite the branch into a few commits whose messages describe the final diff, then force-push. A merger can instead commit to writing the body by hand on a path that allows it; if so, say so here.
    • The code itself is not in question.

Driver: the a3fb51cd round's other non-gating objections are settled.

• The reuse exemption is out of scope, and three of its refusals were never exercised through serve().
  • Resolved. This round did not re-find either one blind. A separate check against the code then found none of the reuse code at this head.
  • Both describe code that now lives in #703.
• The description claimed terminal.rs helpers moved.
  • Resolved. The description now says the file already existed.

Non-blocking

• [new] A doc comment's example contradicts the new rule — crates/rocm-core/src/vllm_oom.rs:190-199
  • It says a bare RuntimeError: ... killed: out of memory that follows a detailed torch.OutOfMemoryError "wins" and that "both are diagnosable".
  • A worker's run showed the opposite: vllm_oom_symptom_is_diagnosable("vllm: RuntimeError: worker killed: out of memory") returns false, and the selector returns the torch line. sub_threshold_lines_carry_no_hint_at_all asserts the same; the code governs.
  • "Callers print the whole log tail beside the hint" is false for the rocm serve note. terminal.rs's quotable_in_single_quotes doc says so itself.
  • Confidence 100 · mechanical · Fix: use two lines that both score as the example, and scope the "whole tail" sentence to the engine surface.
• [new] A comment cites a path that does not exist — engines/vllm/src/process.rs:684
  • It says "See rocm_core::vllm_log_segments". That function is private, at rocm_core::vllm_oom::vllm_log_segments (crates/rocm-core/src/vllm_oom.rs:140).
  • Confidence 85 · mechanical · Fix: correct the path.
• [new] The same two-reads / own-line-skip / quotable-implies-diagnosable rationale is restated in about six places
  • Locations: vllm_oom.rs (module doc, is_rocm_authored_log_line, segment_shows_oom, vllm_log_segments, vllm_oom_diagnose_symptom), terminal.rs, process.rs (oom_utilization_hint, startup_log_context), two diagnose.rs doc comments, serve_summary.rs and docs/vllm.md.
  • The first finding above shows one copy has already drifted.
  • Confidence 70 · mechanical · Fix: state it once in the vllm_oom module doc and link to it from the others.
• [new] serve-26 leaves a rocm daemon running after the scenario ends — apps/rocm/src/service_launch.rs (the simulate_oom_managed_launch return), apps/rocm/src/serve_cmd.rs (ensure_background_helper_running_quiet after start_managed_service), tests/e2e-cucumber/tests/e2e/serving_steps.rs:1317-1340
  • The fault hook returns a launch report and serve then starts the background helper. No teardown stops it, so it keeps watching a starting record whose pid has exited.
  • Confirmed on the review host: rocm daemon processes from earlier serve-26 runs of this PR are still alive after more than 36 hours. Their binaries come from deleted checkouts, and their environment has HOME=/tmp/rocm-e2e-* and ROCM_E2E_SIMULATE_OOM_LAUNCH=1.
  • A companion daemon without that variable suggests the leak predates this PR for other managed-serve scenarios. This change adds one more.
  • Confidence 85 · logic · Fix: kill the scenario's daemon in teardown, or skip starting the helper when the simulated launch is armed.
• [new] docs/testing.md:1592-1601 says no focused command can run serve-26 correctly
  • It claims a run "would run on a binary built without the hook". A worker ran cargo xtask e2e -- --name "serve-26": xtask builds with the hook and sets the capability, and the scenario passed 5/5 steps with exit 0. The CI dispatch filter uses the same route.
  • The risk is real only with a prebuilt ROCM_CLI_BINARY.
  • Confidence 100 · mechanical · Fix: document the xtask --name route and state the prebuilt-binary caveat.
• [new] Workflow comments are now stale — .github/workflows/nightly.yml:434,524,600,682,769,1026, .github/workflows/e2e-selfhosted.yml:333,511,846,1399,1581
  • They still say the e2e-test-hooks set "must match what cargo xtask e2e would build". xtask now also enables e2e-oom-fault-injection, and xtask/src/workflow_contract.rs was relaxed to check only the shared floor.
  • This is the AGENTS.md §5 "same claim repeated across surfaces" rule. The description lists it as a deferral.
  • Confidence 85 · mechanical.
• [new] The release-feature guard covers release.yml only — xtask/src/workflow_contract.rs (the_release_workflow_builds_no_feature_gated_binaries)
  • nightly.yml:129,232 publishes prereleases and is not checked. No path enables the feature there today.
  • The workspace clippy job never compiles the #[cfg(feature = "e2e-oom-fault-injection")] code at service_launch.rs:567-639. A local run with that feature was clean (exit 0).
  • Confidence 85 · architectural · Fix: follow-up guard over the nightly publishing jobs, and a clippy invocation with the feature on.
• [new] rocm serve --help promises the user's own failing line without the fallback caveat — apps/rocm/src/main.rs:473-483
  • The command falls back to the canonical symptom when the line carries a quote or control bytes. The README states this; --help does not.
  • Confidence 70 · mechanical · Fix: add the clause to --help.
• [standing] New note-gating logic grows main.rs — apps/rocm/src/main.rs:3733-3831 (append_oom_serve_note, plus about 230 lines of tests)
  • AGENTS.md §6 asks new domain logic not to grow main.rs. Its natural home, serve_summary.rs, already holds oom_memory_note and serve_failed_to_become_ready.
  • Confidence 70 · architectural · Fix: move the function and its tests there.
• [new] The description is stale against the head
  • Its Verification section reports the full gate at 7bc22302. d4a1cc4 and ebc2671 came after it and extract a new module, crates/rocm-core/src/vllm_oom.rs, which the description never mentions. The description also still says, in its own words, that no new file was involved.
  • The "Changes in the latest round" sections describe serve-29 and reuse-gate work that is no longer in the branch. A banner discloses this, but those sections still read as current.
  • Confidence 85 · mechanical · Fix: add the vllm_oom.rs extraction and re-state verification at the current head.

Decisions for the author

• The OOM note only reaches interactive users — tradeoff
  • The note renders only in summary_mode (background serve on a terminal). Scripted, CI and assistant runs take print_managed_launch_plain and get no post-launch OOM confirmation or diagnose command. The comment at serve_cmd.rs (the append_oom_serve_note call) documents this.
  • Machine-readable output stays prose-free. The same failed launch is explained in one invocation mode and not the other.
• The fault injection is a second Cargo feature rather than part of e2e-test-hooks — tradeoff
  • Keeping it separate isolates the hook. It also creates the two guard gaps above, plus stale workflow comments, which a single feature would not have.
• The serve note narrows to the diagnose threshold — tradeoff
  • RuntimeError: HIP error: out of memory (raised when hipMalloc fails outside torch's caching allocator) now gets no hint at all, where it used to get the canonical one.
  • This keeps the note consistent with rocm diagnose and is documented in docs/vllm.md. Some genuine OOMs now go unexplained.

Positive signals

• The own-line filter is keyed off the shared VLLM_OOM_DETECTED_PREFIX / VLLM_OOM_DIAGNOSE_COMMAND constants, so the hint and the filter that skips it cannot drift apart.
• The tests hold up under mutation. Over about 30 single-branch mutations across rocm-core, rocm-engine-vllm, rocm and xtask, every new test failed when the branch it covers was broken. The only survivors were the .trim() in vllm_log_segments and the redundant outer status check in append_oom_serve_note.
• The fix.rs wording pin names the test that proves the behaviour, as AGENTS.md §3 asks.

Deployment notes

• The vLLM engine's startup-failure text changes. The header becomes "Startup log, last N lines as the terminal drew them", the tail is echoed as rendered rows, and the hint no longer fires for a bare HIP error: out of memory, a kernel OOM-killer line or vLLM's EngineCore wrapper. No scenario covers this text; the description says so.
• New Cargo feature e2e-oom-fault-injection in apps/rocm. It must never be enabled in a published build, and only release.yml is guarded.

What this covered

• Read: every file in prw-base...HEAD, completely, split across two independent workers.
  • The vLLM OOM core: vllm_oom.rs, terminal.rs, lib.rs, diagnose.rs, fix.rs, engines/vllm/src/process.rs, docs/vllm.md, and skills/rocm-doctor/reference.md for consistency.
  • Serve, xtask and e2e: main.rs, serve_cmd.rs, serve_summary.rs, service_launch.rs, Cargo.toml, xtask, e2e-report, e2e-cucumber, README and docs/*.
  • Commits: 6186f159…ebc26718.
• What was run:
  • Unit suites for rocm-core (135), rocm-engine-vllm process:: (43), and the rocm, xtask and e2e-cucumber groups, plus single-branch mutation runs.
  • cargo xtask e2e -- --name serve-26.
  • Feature-on clippy for rocm.
• CI: all checks are green except "E2E tests (Strix Halo, Windows, current)".
  • The failed scenarios are setx PATH, full model name ×2 and the nested runtime folder. None of them touches the OOM path.
  • The reviewer found the same four failing on main's 2026-10-16 runs. The driver confirmed main's own run of this lane at c7a0d65f also failed. This does not gate.
  • CI ran on 2026-10-17. The head is now 9 commits behind prw-base; it merges cleanly with git merge-tree, but the merged result was not built or tested here.
• Not run:
  • The full e2e suite, and any Windows-target build or check.
  • The prior-changes pass: earlier PR reviews are deliberately out of bounds for this run.
  • The ROCMAI-419 / EAI-8059 ticket was not retrieved: no tracker tool was available.
• Fan-out: two workers, per the operator budget. They shared one checkout and briefly raced on a mutation; the affected run was repeated on a clean tree.
• Prompt injection: none found in the checkout.
• Reconciliation: the findings were not reconciled against the PR's discussion. The driver settled the earlier objections afterwards, as noted under Blocking.

[nowycondro]

🔴 Automated review · pr-review-watcher · ebc2671

Review — no blocking findings

Read the full diff between main (merge-base d4e7c751) and head ebc26718 (25 files, +3078/-284): the new crates/rocm-core/src/vllm_oom.rs OOM-selection module, the terminal.rs quoting guard it shares with rocm-engine-vllm, the engines/vllm/src/process.rs rework, the new apps/rocm wiring (serve_summary.rs, serve_cmd.rs, service_launch.rs, main.rs), the e2e fault-injection plumbing (xtask, e2e-report, tests/e2e-cucumber), and the README/docs updates. This implements the OOM-guidance half of ROCMAI-419 (ex EAI-8059); the serve-behaviour (reuse-exemption) half is stated to have moved to #703, and I confirmed the current diff carries none of that logic (no reuse_pregate_opens/managed_service_reuse_refusal/gpu_preferred hunks remain). Ticket ROCMAI-419 is named but not retrievable from this environment (no tracker access here), so I could not check it directly — the assessment below is against the code and the PR's own description only.

Overall this is unusually well-substantiated: I spot-checked several of the description's more specific claims against the actual code/history (the (0, 1] domain of parse_gpu_memory_utilization, the ready/running/starting mapping in status_for_readiness, the already_running ⇒ log_path: None invariant, the "duplicate tail reader is pre-existing and already used for service logs" claim, the release.yml/nightly.yml feature-flag claims, and the presence of commits 1de61359/a3fb51cd in history) and every one held up. I did not find a defect that survives verification.

Blocking

None

Non-blocking

None

Decisions for the author

• append_oom_serve_note's log read uses the UTF-8-strict read_tail_lines (via read_optional_tail_lines), not the lossy rocm_core::tail_lines the engine's own OOM detection uses — non-blocking-improvement. If a service log ever contains invalid UTF-8 near the failure (a genuine possibility for a crashing subprocess's output), this call site returns a single <failed to read ...> placeholder and the OOM note silently disappears, while the engine's own hint (which reads via rocm_core::tail_lines) would still have fired. I verified this is not a new risk introduced here, though: read_optional_tail_lines was already used on main for this exact kind of data (main.rs's existing rocm services logs-style service-log read at the pre-PR equivalent of line 6749), and the PR's own "Known deferrals" section already names the duplicate-tail-reader gap and explains why unifying it is out of scope for this change. Flagging only so the asymmetry with the engine-side reader is visible in one place.
• The CLI's OOM-note read window can miss an OOM the engine itself diagnosed, when the engine's own appended bail is long enough to push the raw line out of the shared tail budget — answered-by-intent. This is a real, measured gap (engine and CLI share one log but read it at different points), and the PR's own text states it plainly: "the obvious fix trades one defect for another... I would rather raise it than guess at it". It's pinned by a dedicated regression test (the_cli_window_can_miss_an_oom_the_engine_saw) so a future fix can't silently regress the case it's trading away, and it's called out in the README's "absence is not evidence" list for users. Would otherwise be a blocking gap in a less transparent PR; here it's fully disclosed with a structural fix proposed (recording the engine's selected symptom as data) and explicitly deferred. Confirm that's still the intended scope for this PR.
• The release-feature-leak CI guard (the_release_workflow_builds_no_feature_gated_binaries) only scopes release.yml, not nightly.yml's own publishing build — tradeoff. I checked nightly.yml's two cargo build --release -p rocm -p rocmd [-p xtask] publish-path invocations directly and confirmed neither currently passes any --features/-F flag, so there's no live leak today. The PR text already states why a blanket scan of nightly.yml isn't the right fix (that file legitimately builds --features rocm/e2e-test-hooks for its own E2E lanes). Worth a follow-up guard aimed specifically at the publish steps if that file is expected to stay feature-free going forward, but not something this PR needs to carry.

Positive signals

• The shared-rule design (rocm_core::vllm_oom_diagnose_symptom / vllm_oom_display_line used identically by the engine's startup-failure hint and the CLI's serve summary) is exactly the right way to prevent the two surfaces from silently drifting apart on what counts as a vLLM OOM, and it's backed by tests that exercise the real two-read sequence (engine writes, then CLI re-reads the same growing log file), not just each function in isolation.
• is_rocm_authored_log_line and its regression test (the_engine_hint_is_never_quoted_back_as_the_users_own_line) catch a genuinely subtle self-referential bug: without it, the newest-first scan would always select rocm-cli's own previously-written hint sentence (which contains ') instead of the user's real error, silently defeating the entire "quote the user's own failing line" feature in every real run.
• xtask/src/workflow_contract.rs's new the_release_feature_guard_catches_every_spelling test is a nice piece of defensive test design — it pins the guard against compact (-Fname) and line-continuation-wrapped --features spellings specifically because those are the forms a real workflow edit is likely to produce, not just the obvious case.

Deployment notes

• gh pr checks shows one failing check: E2E tests (Strix Halo, Windows, current) (self-hosted hardware lane). I could not fetch its job logs from this environment (blocked), so I can't confirm whether it's related to this change or a pre-existing flake on that runner. The new scenario here (serve-26) is explicitly @requires-os:linux, and vLLM-OOM detection code is cross-platform (compiled for Windows too), so I'd want this confirmed green or explicitly identified as an unrelated flake before merge. Every other check (Build Linux PR binaries, clippy, Analyze rust/python/actions, CodeQL, Commit signatures + sign-off, Test (affected crates), E2E consolidated report, the three other E2E self-hosted lanes including MI300X, Markdown links, license headers, docs build) is green.
• The new e2e-oom-fault-injection Cargo feature is test-only and is verified compiled out of release.yml by a new CI test in this PR; it's deliberately absent from the self-hosted/nightly prebuilt-binary lanes, where the new scenario reports a skip rather than a false pass.
• No schema/migration concerns; this only changes diagnostic text and adds a dev/test-only feature flag.

What this covered

Read the full diff (merge-base d4e7c751194d1643eacc3b1bda33afe10766def73 … head ebc267182a7d77042f1f27a72e1bc41680d7b7e6) across all 25 changed files, completely, plus the surrounding non-diff code needed to verify specific claims (parse_gpu_memory_utilization, status_for_readiness, ManagedLaunchReport's reuse short-circuit, summarize_startup_log_tail, collect_serve_notes' hint insertion, and nightly.yml's publish-build invocations). Checked gh pr checks for CI status. This was a single-context review without subagent fan-out, per this task's instructions — coverage is accordingly narrower than a split multi-pass review would achieve, though the change's small number of tightly-coupled modules and its unusually extensive self-documentation made single-pass verification tractable here.

Did not run: cargo test/cargo clippy/cargo fmt (no Rust toolchain or package-registry network available in this environment) — relied on the CI results reported above instead of re-deriving them. Did not retrieve the named ticket (ROCMAI-419 / ex EAI-8059) — no tracker tool available here. Not reconciled against the existing human review thread on this PR (from r0x0r) or its many prior automated rounds; this is a fresh, independent read of the current head only.

[jussielo-amd]

🔴 AI-assisted review (Claude Code) · head ebc26718

This is an assisted human review, not an autonomous merge gate: no GitHub approval or change-request is being filed — the merge decision stays with maintainers.

Review — needs work

Full review of the PR's own diff (base origin/main 3dec0eb2, merge-base d4e7c751, head ebc26718; 25 files, +3078/−284). I did not narrow it to what changed since my earlier review at a3fb51cd, because four of that review's five findings could not be tied to a single file.

The ticket named in the description could not be retrieved from here, so the ticket's stated scope was not checked. The description's own scope claim is "diagnostics only".

Blocking

• The head cannot merge as it stands, and one check on it is red — PR state CONFLICTING; tests/e2e-cucumber/features/model_serving.feature:358, docs/testing.md (six serve-26 references), tests/e2e-cucumber/src/capability.rs, tests/e2e-cucumber/tests/e2e/serving_steps.rs
  • Conflicts. git merge-tree against current main shows 12 conflict hunks across 10 files: README.md, docs/vllm.md, engines/vllm/src/process.rs, xtask/src/workflow_contract.rs and six e2e-harness files.
  • Scenario id. main has since landed serve-26 (AIM inference) and serve-27 in model_serving.feature, and this PR's OOM scenario is also serve-26. After a rebase, tests/e2e-cucumber/tests/feature_naming.rs (scenario_names_are_indexed_sequentially_per_feature, scenario_indexes_are_unique_across_the_suite) goes red until it is renumbered.
  • Red check. E2E tests (Strix Halo, Windows, current) failed 4 of 145 scenarios (serve-01, serve-02, diagnose-38, runtime-02); the other 29 checks passed and 2 were skipped. That run is from 2026-10-16 against the pre-rebase head, main's Windows job was green on 2026-10-29, and the job log shows pre-warm download failures (llama.cpp HTTP 404, TheRock HTTP 403). The PR's net change to serve_cmd.rs is only the test-hook bypass plus the append_oom_serve_note call, so I could not tie serve-01/serve-02 to this diff by reading. I did not reproduce the failures. A red check still blocks until it is re-run on a rebased head.
  • Confidence 100 · mechanical · Fix: rebase onto main, renumber the scenario to the next free index (serve-28) and update every reference, then re-run CI.

Non-blocking

• The serve note vanishes silently on a log with one invalid UTF-8 byte — apps/rocm/src/main.rs:3823, :7569, :7641
append_oom_serve_note reads the log with read_optional_tail_lines/read_tail_lines, which use BufRead::lines(). That returns Err on invalid UTF-8, the error becomes one <failed to read service log…> line, and vllm_oom_diagnose_symptom returns None. The engine reads the same file with rocm_core::tail_lines, which decodes lossily on purpose (crates/rocm-core/src/log_tail.rs explains why), and the CLI path also reads the whole file unbounded. The description defers this as "wider … six call sites", but that reason does not apply to a change local to append_oom_serve_note, which touches none of the other five. No test plants an invalid byte.
  Confidence 85 · logic · Fix: call rocm_core::tail_lines with a byte cap there. The engine's reader governs, because it is the shared one and says why it is lossy.

• The summary can print "it may still be loading" and "ran out of GPU memory" together, and the help text promises that cannot happen — apps/rocm/src/serve_summary.rs:170-181, main.rs:472-483, README "failed to become ready" bullet
  status_for_readiness maps an unreachable endpoint to starting, so starting means only "not answering at the 45 s timeout". render_summary always adds the generic "may still be loading" note for it, and the OOM note then states the attempt ran out of memory. The --help text says the note is "never shown for a healthy or still-loading server", while the description's own "Known deferrals" concedes it "can in principle fire on a still-loading server". The README's list of conditions also omits timing: the log is read once, after the fixed 45 s wait, so an OOM that happens later yields starting with no note.
  Confidence 70 · logic · Fix: the code governs (the gate is starting plus log text). Narrow the help and README to say the running status is excluded, add the timing condition, and drop or qualify the generic "may still be loading" line when the OOM note applies.

• A doc paragraph on vllm_oom_diagnose_symptom is false — crates/rocm-core/src/vllm_oom.rs:193-199
  It says that after a real allocator line, a bare RuntimeError: … killed: out of memory line "wins" and that "both are diagnosable". A bare out of memory scores 25, below MIN_SCORE_FOR_MATCH (50) in KEYWORDS_VLLM_OOM, and the module's own test asserts !log_shows_oom("HIP error: out of memory"), so such a line is never selected. The comment was carried over from the old loose detector.
  Confidence 85 · mechanical · Fix: the code and its tests govern, so rewrite the paragraph.

• terminal.rs docs are stale, and the stripper now has no reachable production caller — crates/rocm-core/src/terminal.rs:13-15, :409-416, vllm_oom.rs:251-257, lib.rs:176
  The module doc still says "The vLLM engine strips the sequences out". The stripper's doc says "every caller here has already split its input into lines", which is false for the one production use left and is exactly the welding hazard this PR fixes. After this PR the stripper's only non-test reference is the unwrap_or_else fallback in vllm_oom_display_line, and both production callers pass a segment the selector already chose, so that branch is unreachable from them. The root re-export at lib.rs:176 is consumed only by a process.rs test.
  Confidence 75 · mechanical · Fix: update both docs, and drop the re-export or the fallback.

• Comments narrate review or edit history, or cite a path that does not exist — engines/vllm/src/process.rs:684, crates/rocm-core/src/fix.rs (hint test, ~2723-2736), crates/rocm-core/src/lib.rs:336-340, tests/e2e-cucumber/tests/e2e/serving_steps.rs:1303-1304, crates/e2e-report/src/lib.rs:33-36, model_serving.feature:344
process.rs:684 cites rocm_core::vllm_log_segments, which is private to vllm_oom and not a crate-root path. The fix.rs test and lib.rs comments narrate branch and merge history, and the others say "previously …" or "(a review asked for it …)".
  Confidence 75 · mechanical · Fix: state the invariant and drop the history.

• The engine and the CLI each hand-render the hint and command, so "the same guard" holds by convention only — engines/vllm/src/process.rs:713-726, apps/rocm/src/serve_summary.rs:245-268, apps/rocm/src/service_launch.rs (~611-627), vllm_oom.rs:99,216,255
  Both consumers repeat if quotable { s } else { VLLM_OOM_CANONICAL_SYMPTOM } and build `{VLLM_OOM_DIAGNOSE_COMMAND} '{symptom}'` by hand. The "vllm: " prefix is a bare literal in four places, and process.rs:714 does strip_prefix("vllm: ").unwrap_or(&symptom). The e2e fixture in service_launch.rs hand-copies the engine's bail text, and only the two marker constants are shared, so the engine header can change while serve-26 stays green. docs/vllm.md and terminal.rs claim neither surface can hand out a command the other would reject. Separately, origin/main now has crates/rocm-core/src/quoting.rs (#743); this branch predates it, and the new reject-not-escape predicate does not reference it.
  Confidence 70 · architectural · Fix: one function in vllm_oom.rs that returns the rendered hint or {display_line, command_symptom}, used by the engine, the serve note and the fixture.

• New serve logic and about 220 test lines went into the 21k-line main.rs — apps/rocm/src/main.rs:3771-3830, tests ~:17835-18050
serve_summary.rs already holds the sibling oom_memory_note, and AGENTS.md §6 says new domain code defaults to its own file rather than growing main.rs. I rate this lower than the earlier lib.rs finding because it extends an existing serve-domain file's subject rather than adding a new subsystem.
  Confidence 70 · architectural · Fix: move append_oom_serve_note and its tests beside oom_memory_note.

• serve-26 pins the note's wording without proving the command it prints — serving_steps.rs:1345-1418, model_serving.feature:358-363
  AGENTS.md §3 requires remediation advice naming a command to be asserted together with the behaviour, or the wording pin to carry a comment naming the proving test. None of the three Then steps does either. Running rocm diagnose --symptom '<printed line>' by hand routes to fix-16-vllm-oom, and the quotable ⇒ diagnosable contract is covered by every_rendered_line_break_keeps_the_gate_in_step_with_the_scorer, so the claim is true today but unpinned in the scenario.
  Confidence 70 · logic · Fix: add a step that runs the printed command and asserts fix-16-vllm-oom, or add the pointer comments.

• The last block of the_colourised_round_trip_quotes_the_row_the_terminal_drew tests nothing in production — engines/vllm/src/process.rs:1499-1521
  The inside_echo filter is built inside the test and then asserted to find nothing. It documents a rejected design as executable text, under a comment saying it "forecloses the tightening". Test verdicts: that block passes with the production change reverted, because it exercises none of the production code; the first half of the test does go red.
  Confidence 85 · mechanical · Fix: delete the block and keep the comment as prose.

• The "never reaches a shipped binary" guard covers only release.yml, and the docs list the wrong test — docs/testing.md ~1578-1583 and ~1633-1636, xtask/src/workflow_contract.rs:3934-3957, xtask/src/e2e.rs:108-119, .github/workflows/ci.yml:709,925
docs/testing.md lists cargo test -p xtask prebuilt_e2e_lanes_enable_test_hooks under "The fault-injection feature must never reach a shipped binary", but that test asserts the prebuilt lanes do enable e2e-test-hooks, the opposite purpose. Only the_release_workflow_builds_no_feature_gated_binaries guards release, and it reads release.yml only; the PR's downloadable bundle builds in ci.yml are unguarded, and the description concedes nightly.yml is. Comments in xtask/src/e2e.rs, serving_steps.rs:1181 and the workflows still say the lanes must "match what cargo xtask e2e builds", but xtask now builds a superset. The feature is off by default and release.yml passes no flags, so I see no leak today; nothing proves a default build ignores the arming env var.
  Confidence 75 · mechanical · Fix: correct the docs, extend the guard to every workflow build that is packaged, and add a no-feature test that the env var is inert.

• The description is not the final diff — PR body
  The verification section is pinned to head 7bc22302, and the head has since moved (the vllm_oom.rs extraction, a doc-link fix and another main merge). Large sections describe code that is no longer in this branch (the reuse pre-gate, reuse_pregate_opens, serve-25/27/28/29); the body says so, but AGENTS.md §5 asks for the body to be written from the final diff. The body also names internal review-automation tools, which AGENTS.md §2 asks to keep out of upstream text, and it does not name the new vllm_oom.rs module.
  Confidence 75 · mechanical · Fix: rewrite the body from the final diff and re-run the verification at the new head.

Decisions for the author

• The CLI's 80-line window can miss an OOM the engine itself diagnosed — answered-by-intent
  append_oom_serve_note reads after the engine has appended its own longer bail. In the PR's fixture an OOM followed by 77 lines gives an engine hint and no serve note, and the_cli_window_can_miss_an_oom_the_engine_saw pins is_none(). The description answers it: "This is not fixed here … the obvious fix trades one defect for another … say the word if it should land here." The README's conditions list names it too. This would otherwise be blocking, because a note that exists to help on OOM is silent in the case the PR itself measured. Please confirm the structured-record fix (the engine writing its selected symptom as data next to the log) is the intended follow-up.

• No Gherkin scenario covers the vLLM engine's changed startup-failure text — answered-by-intent
  The header wording, rendered-row echo, narrowed hint trigger and the \r/ESC[2K repaint case are covered only by unit tests. The description's "Stated gap" names the lane that would cover it (E2E tests (MI300X)) and why it is not added: "provoking one reliably on a 192 GB MI300X … is GPU-expensive and racy." This would otherwise be blocking under AGENTS.md §3. Please confirm the tradeoff is still intended.

• The engine hint no longer fires for a bare HIP error: out of memory — tradeoff
  On main the engine hinted on any out of memory substring; now only an allocator-shaped signature scoring ≥ 50 does, to stay in step with rocm diagnose. One side: the hint and the diagnosis can never disagree, and kernel OOM-killer lines and the EngineCore wrapper stop producing false hints. The other side: a user whose log says only HIP error: out of memory now gets no memory guidance and no pointer to rocm diagnose. docs/vllm.md justifies this with "each of those also appears for failures that have nothing to do with memory", and nothing in the repo supports that for this particular string, which I believe is PyTorch-ROCm's rendering of hipErrorOutOfMemory (not verified here). Please confirm this was deliberate.

• A second Cargo feature plus an env var plus a capability tag for the fault-injection hook — non-blocking-improvement
  The existing e2e-test-hooks feature already reaches the prebuilt lanes through the ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE seam, with no capability plumbing. Separately, if the arming env var were lost on the mock lane, serve-26 would resolve to a reported skip rather than a failure; the skip is recorded in platform.json but nothing fails.

Positive signals

• The vLLM OOM reader now lives in its own module, crates/rocm-core/src/vllm_oom.rs (new file; lib.rs only declares and re-exports it).
• Quoting that rejects rather than escapes unquotable log text keeps untrusted bytes out of the printed command, and the shared Cf/Zl/Zp predicate has a per-predicate test.
• the_release_workflow_builds_no_feature_gated_binaries joins \ continuations first and asserts a build was found, so a renamed build step fails loudly instead of passing vacuously.
• The fault-injection hook compiles to const false in a default build, and xtask e2e clears the arming variable for prebuilt binaries. Building the hooked release binary and running serve-26's exact arguments under a PTY printed the note, and the printed rocm diagnose --symptom '…' routed to fix-16-vllm-oom; the same binary without the variable refused at "no usable AMD GPU", so a missing arm fails loudly.

Deployment notes

• Rebase onto current main before anything else. It has extracted large parts of main.rs since, and has its own serve-26/serve-27 and quoting.rs.
• The squash-merge note in the PR banner still applies: commit 1de61359 is still in the PR history and its message still contains the false gpu_preferred sentence the banner flags. The repo builds the squash body from commit messages.
• The engine's startup-failure message header changed from "Last 80 lines of startup log:" to "Startup log, last N lines as the terminal drew them:". git grep finds no in-repo parser of the old header, but anything outside the repo scraping it would break.
• A new Cargo feature, e2e-oom-fault-injection, is built only by cargo xtask e2e.

Since my earlier review at a3fb51cd

• Resolved: the OOM subsystem inline in lib.rs (blocking then). d4a1cc41 extracts it to crates/rocm-core/src/vllm_oom.rs.
• Resolved: shipping the GPU-reuse exemption separately. 55671427 splits it out, and I found no pre-gate or reuse-exemption code at the head; the net serve_cmd.rs change is only the test-hook bypass and the note call. I did not check the state of #703.
• Unchanged: the missing scenario for the engine's startup-failure text is still answered only by the description's "Stated gap" (see Decisions for the author).
• No longer applies here: the deployment note about GPU-less hosts, because it moved with the exemption.
• Still applies: the squash-merge hygiene note.

The earlier needs work rested on the lib.rs finding, which is resolved; the current needs work rests on the head being unmergeable and a check being red.

What this covered

I read the PR's own diff at ebc26718 in three coupled groups, each by an independent review pass: OOM core plus the vLLM engine, the serve CLI plus docs, and the e2e scenarios plus xtask. Each pass also checked AGENTS.md adherence, history and code comments within its group. I re-verified every finding above against the PR head myself.

• Read completely: vllm_oom.rs, terminal.rs, serve_summary.rs, model_serving.feature, serving_steps.rs, capability.rs, xtask/src/e2e.rs, crates/e2e-report/src/lib.rs, docs/vllm.md, docs/architecture.md, and engines/vllm/src/process.rs lines 1–1330 (all production code and all OOM/sanitize tests).
• Read in part:
  • serve_cmd.rs: lines 1–140 and 380–1130 in detail, the rest skimmed.
  • service_launch.rs: lines 1–1300, which cover every PR change and its callers; the remaining ~570 lines are pre-existing tests.
  • main.rs, diagnose.rs, fix.rs, workflow_contract.rs, tui_driver.rs: the PR hunks plus every touched function, test and in-file caller.
  • lib.rs: lines 1–420 plus a keyword grep of the rest.
  • expectation.rs: header, tag parsing, resolve and the new test.
  • README, docs/manual-testing.md, docs/testing.md: hunks and surrounding sections.
• Ran: cargo test -p rocm-engine-vllm --lib process:: (43 passed), the rocm-core OOM/terminal tests (19 passed), and cargo test -p xtask for e2e:: (3), release_ (12) and test_hooks (2), all passing. I also built the hooked release binary and exercised serve-26's arguments by hand (see Positive signals), ran the diff leak scan (clean), and read the CI rollup and the Windows job log.
• Did not run:
  • mutation checks, so every test verdict above is reasoned from code, except that the round-trip block's verdict is established by reading its construction;
  • the cucumber suite, the full gate, and python scripts/smoke_local.py;
  • a prior-changes pass;
  • separate design-review passes (the group passes' design questions fed Decisions for the author).
• Several lower-confidence candidates were dropped under the 70 cutoff: the unlinted e2e-oom-fault-injection code path, a redundant outer status check, and \t handling in the echo.
• The fixes on this PR were written by its author, not by me, and the findings were reached from the code alone, not reconciled against the review threads.

[nowycondro]

🤖 Automated review (Claude Code) · ebc2671

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 — no blocking findings

Full review of the whole change: git diff d4e7c75194d1643eacc3b1bda33afe10766def73...ebc267182a7d77042f1f27a72e1bc41680d7b7e6 (25 files, +3078/-284), every changed file read completely plus the surrounding production code it calls into (apps/rocm/src/main.rs's read_optional_tail_lines, serve_cmd.rs's full call site, rocm-engine-protocol's DEFAULT_LOG_TAIL_LINES, engines/vllm/src/process.rs::resolve_model_response, AGENTS.md §3). This change implements the diagnostics scope of ROCMAI-419/ex-EAI-8059 ("tell the user which memory knobs to reach for when a serve runs out of VRAM") — detecting a vLLM OOM signature in a failed managed serve's log and surfacing --gpu-memory-utilization/--gpu guidance plus a rocm diagnose --symptom command. The ticket's explicit scope ("diagnostics only, not serve behaviour") matches what's in front of this diff; the serve-behaviour reuse-exemption piece is confirmed absent here (split to #703, per the PR body). One deliberate deviation from the ticket's text: the ticket lists Engine core initialization failed as an OOM signature to detect, but vllm_oom.rs/serve_summary.rs explicitly and correctly exclude it (it's vLLM's generic crash wrapper, not OOM-specific) — informational, not a defect.

Blocking

None.

Non-blocking

None.

Decisions for the author

• No Gherkin scenario covers the vLLM engine's own startup-failure text (the header rewording, the per-row echo, and the narrowed OOM-hint trigger in engines/vllm/src/process.rs's startup_log_context/oom_utilization_hint) — answered-by-intent. AGENTS.md:88 ("User-observable behavior needs a scenario, not only a unit test... a unit test asserting the internal helper does NOT discharge this") would otherwise make this blocking, since this text is observable via rocm services logs/--verbose/--foreground and only unit-tested (e.g. the_colourised_round_trip_quotes_the_row_the_terminal_drew, the_echoed_tail_never_welds_two_rendered_rows_into_one_line). The PR body's own "Stated gap" section names the exact rule, explains why the one lane that could exercise it (E2E tests (MI300X)) can't do so without a GPU-expensive, racy provoked OOM, and explicitly asks "say the word if it should land here." Confirm this is still the accepted tradeoff rather than a TODO that slipped through.
• The documented tail-window gap (append_oom_serve_note in apps/rocm/src/main.rs:3771, pinned by the_cli_window_can_miss_an_oom_the_engine_saw in engines/vllm/src/process.rs) is a real, measured limitation — an OOM followed by ~77 more log lines silently drops the CLI note even though the engine's own message still shows it — tradeoff, not a defect: the alternative (re-windowing to agree with the engine) was shown to reintroduce the colourised-line quoting hole the_colourised_round_trip_quotes_the_row_the_terminal_drew exists to prevent, and this one is pinned by a dedicated regression test plus a README "absence is not evidence" callout. No action needed beyond confirming the tradeoff still reads as acceptable.

Positive signals

• vllm_oom_diagnose_symptom (crates/rocm-core/src/vllm_oom.rs:209) and quotable_in_single_quotes (crates/rocm-core/src/terminal.rs:221) are shared verbatim between the vLLM engine's own startup-failure hint and the rocm serve CLI summary note, closing a real class of bug (the two surfaces silently disagreeing on what counts as an OOM or what's safe to shell-quote) rather than just documenting the risk.
• is_rocm_authored_log_line (crates/rocm-core/src/vllm_oom.rs:54) correctly keeps a newest-first log scan from quoting rocm-cli's own previously-written hint back at the user as though it were the engine's error — a subtle self-referential bug that unit tests (the_engine_hint_is_never_quoted_back_as_the_users_own_line) prove was real before the fix.
• The e2e-oom-fault-injection feature is defensively gated three different ways (per-crate feature flag defaulting off, release.yml shape-based guard in xtask/src/workflow_contract.rs catching -Fname/continuation-line spellings, and a runtime env-var check requiring the exact string "1"), so a test-only fault-injection hook has no plausible path into a shipped binary.

Deployment notes

• gh pr view reports mergeStateStatus: DIRTY / mergeable: CONFLICTING against the current main tip — main has moved since this PR's recorded base and the branch needs a rebase before it can land; this doesn't reflect a defect in the reviewed diff itself.
• CI at this exact head (ebc26718) is green on every required lane. The one failing check, E2E tests (Strix Halo, Windows, current) in the E2E self-hosted workflow, is confirmed non-required: .github/workflows/e2e-selfhosted.yml's own header comment states this workflow was removed from branch protection's required-status-check list (confirmed 2026-09-11), independent of continue-on-error (which is false throughout).

What this covered

Every file in the base...head diff, read completely (25 files, 4163-line diff), plus directly-verified surrounding context: read_optional_tail_lines, resolve_engine_selection/resolve_model_response, AGENTS.md §3's Gherkin-scenario rule, DEFAULT_LOG_TAIL_LINES's value, and the serve_cmd.rs call site confirming append_oom_serve_note only runs on the interactive-summary path. CI checked via gh pr view --json statusCheckRollup against the exact head SHA; the one failure was traced to a workflow confirmed non-required by its own in-repo documentation.

This was a single-context pass by deliberate instruction for this run (fan-out to independent sub-reviewers was available but explicitly disallowed here), not a fan-out failure — coverage is accordingly narrower than the skill's multi-pass method would achieve, and a follow-up multi-pass run is recommended if higher assurance is wanted. Did not run: History/prior-changes passes (no in-scope git blame anomalies surfaced during reading; not separately fanned out). Not reconciled against the PR's extensive prior review-comment history, per the blind-review method — this report was reached from the code and the PR's own stated intent only.

[nowycondro]

🤖 Automated review (Claude Code) · ebc2671

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 — no blocking findings

Full review of the whole change: git diff d4e7c75194d1643eacc3b1bda33afe10766def73...ebc267182a7d77042f1f27a72e1bc41680d7b7e6 (25 files, +3078/-284), every changed file read completely plus the surrounding production code it calls into (apps/rocm/src/main.rs's read_optional_tail_lines, serve_cmd.rs's full call site, rocm-engine-protocol's DEFAULT_LOG_TAIL_LINES, engines/vllm/src/process.rs::resolve_model_response, AGENTS.md §3). This change implements the diagnostics scope of ROCMAI-419/ex-EAI-8059 ("tell the user which memory knobs to reach for when a serve runs out of VRAM") — detecting a vLLM OOM signature in a failed managed serve's log and surfacing --gpu-memory-utilization/--gpu guidance plus a rocm diagnose --symptom command. The ticket's explicit scope ("diagnostics only, not serve behaviour") matches what's in front of this diff; the serve-behaviour reuse-exemption piece is confirmed absent here (split to #703, per the PR body). One deliberate deviation from the ticket's text: the ticket lists Engine core initialization failed as an OOM signature to detect, but vllm_oom.rs/serve_summary.rs explicitly and correctly exclude it (it's vLLM's generic crash wrapper, not OOM-specific) — informational, not a defect.

Blocking

None.

Non-blocking

None.

Decisions for the author

• No Gherkin scenario covers the vLLM engine's own startup-failure text (the header rewording, the per-row echo, and the narrowed OOM-hint trigger in engines/vllm/src/process.rs's startup_log_context/oom_utilization_hint) — answered-by-intent. AGENTS.md:88 ("User-observable behavior needs a scenario, not only a unit test... a unit test asserting the internal helper does NOT discharge this") would otherwise make this blocking, since this text is observable via rocm services logs/--verbose/--foreground and only unit-tested (e.g. the_colourised_round_trip_quotes_the_row_the_terminal_drew, the_echoed_tail_never_welds_two_rendered_rows_into_one_line). The PR body's own "Stated gap" section names the exact rule, explains why the one lane that could exercise it (E2E tests (MI300X)) can't do so without a GPU-expensive, racy provoked OOM, and explicitly asks "say the word if it should land here." Confirm this is still the accepted tradeoff rather than a TODO that slipped through.
• The documented tail-window gap (append_oom_serve_note in apps/rocm/src/main.rs:3771, pinned by the_cli_window_can_miss_an_oom_the_engine_saw in engines/vllm/src/process.rs) is a real, measured limitation — an OOM followed by ~77 more log lines silently drops the CLI note even though the engine's own message still shows it — tradeoff, not a defect: the alternative (re-windowing to agree with the engine) was shown to reintroduce the colourised-line quoting hole the_colourised_round_trip_quotes_the_row_the_terminal_drew exists to prevent, and this one is pinned by a dedicated regression test plus a README "absence is not evidence" callout. No action needed beyond confirming the tradeoff still reads as acceptable.

Positive signals

• vllm_oom_diagnose_symptom (crates/rocm-core/src/vllm_oom.rs:209) and quotable_in_single_quotes (crates/rocm-core/src/terminal.rs:221) are shared verbatim between the vLLM engine's own startup-failure hint and the rocm serve CLI summary note, closing a real class of bug (the two surfaces silently disagreeing on what counts as an OOM or what's safe to shell-quote) rather than just documenting the risk.
• is_rocm_authored_log_line (crates/rocm-core/src/vllm_oom.rs:54) correctly keeps a newest-first log scan from quoting rocm-cli's own previously-written hint back at the user as though it were the engine's error — a subtle self-referential bug that unit tests (the_engine_hint_is_never_quoted_back_as_the_users_own_line) prove was real before the fix.
• The e2e-oom-fault-injection feature is defensively gated three different ways (per-crate feature flag defaulting off, release.yml shape-based guard in xtask/src/workflow_contract.rs catching -Fname/continuation-line spellings, and a runtime env-var check requiring the exact string "1"), so a test-only fault-injection hook has no plausible path into a shipped binary.

Deployment notes

• gh pr view reports mergeStateStatus: DIRTY / mergeable: CONFLICTING against the current main tip — main has moved since this PR's recorded base and the branch needs a rebase before it can land; this doesn't reflect a defect in the reviewed diff itself.
• CI at this exact head (ebc26718) is green on every required lane. The one failing check, E2E tests (Strix Halo, Windows, current) in the E2E self-hosted workflow, is confirmed non-required: .github/workflows/e2e-selfhosted.yml's own header comment states this workflow was removed from branch protection's required-status-check list (confirmed 2026-09-11), independent of continue-on-error (which is false throughout).

What this covered

Every file in the base...head diff, read completely (25 files, 4163-line diff), plus directly-verified surrounding context: read_optional_tail_lines, resolve_engine_selection/resolve_model_response, AGENTS.md §3's Gherkin-scenario rule, DEFAULT_LOG_TAIL_LINES's value, and the serve_cmd.rs call site confirming append_oom_serve_note only runs on the interactive-summary path. CI checked via gh pr view --json statusCheckRollup against the exact head SHA; the one failure was traced to a workflow confirmed non-required by its own in-repo documentation.

This was a single-context pass by deliberate instruction for this run (fan-out to independent sub-reviewers was available but explicitly disallowed here), not a fan-out failure — coverage is accordingly narrower than the skill's multi-pass method would achieve, and a follow-up multi-pass run is recommended if higher assurance is wanted. Did not run: History/prior-changes passes (no in-scope git blame anomalies surfaced during reading; not separately fanned out). Not reconciled against the PR's extensive prior review-comment history, per the blind-review method — this report was reached from the code and the PR's own stated intent only.

(Retry: an earlier submission of this same review may not have been recorded — see note below if duplicates appear.)

[r0x0r]

Review round: triage of the 37 open threads, merge with main, and the automated round

Pushed 4187652a. The branch now merges cleanly (mergeable: MERGEABLE).

Triage of the 37 unresolved review threads

Five had @rominf speaking last, all filed 2026-09-15T10:22Z and all now marked
outdated. All five were already fixed — by 40c3567e, pushed about three
and a half hours after that review, which is why none of them got a reply at
the time. Checked against the current code rather than against the commit
message:

Thread	State now
main.rs — the already_hinted early return destroys the whole OOM note	Fixed. De-duplication moved to composition level exactly as suggested: main.rs computes already_hinted and passes it into oom_memory_note, which omits the one shared hint fragment. The note itself — the OOM confirmation, the doesn't-fit branch, the diagnose command — is never suppressed.
main.rs — the test cements the suppression (notes == pre_launch_notes)	Fixed. The equality assertion is gone. append_oom_serve_note_prints_the_shared_hint_exactly_once now pins "the hint appears exactly once across the whole summary", with a sibling in serve_summary.rs counting the same thing.
rocm-core/lib.rs — <0-1> is still here	Fixed. crates/rocm-core/src/lib.rs:345 reads --gpu-memory-utilization <fraction greater than 0 and at most 1>. The pin was extended as asked: crates/rocm-core/src/fix.rs:2739-2744 asserts both the new wording and !hint.contains("<0-1>"), so the merge-reversion failure mode you traced cannot recur silently.
tui_driver.rs — duplicate extra_env loop inverts the credential strip	Fixed. There is one loop, at :288, above the env_remove block — your "simplest fix is to delete this loop". The comment now says the position is deliberate and why.
serving_steps.rs — hardcoded 30s instead of default_timeout()	Fixed. interactive_summary_timeout() is default_timeout(), with a doc comment giving your reason (E2E_TUI_TIMEOUT_SECS on a contended lane).

The other 32 threads have @r0x0r last. Re-reading them for "answered with a
promise rather than a change", the ones still genuinely open are design
decisions, not unaddressed findings:

• The interactive-only asymmetry (print_managed_launch_plain prints no
  post-launch note) — documented, not closed, and explicitly handed over for a
  decision on whether that path grows a note: key.
• Whether a compile-gated seam waiving a production safety bail is acceptable
  (test(e2e): drive the Lemonade recovery scenario with a planted runtime (ROCMAI-669) #351 vs this PR) — above this PR.
• diagnose.rs's gpu[-_]memory[-_]utilization keyword not tolerating spaces —
  accepted by the reviewer as a fast-follow.

Everything else in that set was fixed with a commit cited in the reply; I
spot-checked the mechanical ones (screen_without_whitespace on both
assertions, OOM_FAULT_INJECTION_ENV single-sourced in e2e-report,
STARTUP_FAILURE_LOG_TAIL_LINES derived from the protocol constant,
the_release_workflow_builds_no_feature_gated_binaries handling \
continuations and compact -F) and they hold at this head.

Merge with main

Eight conflicted files. main's Phase-6x/7x modularization moved most of what
this branch touches, so several hunks had an empty origin/main side — the
code was relocated, not deleted. Each was re-applied into its new home; the
merge commit message has the file-by-file account. The ones worth calling out:

• apps/rocm/src/serve_cmd.rs — the branch's wide use crate::{...} is dead:
  resolve_engine_selection, gpu_vram_usage, parse_device_policy and the
  rest now come from engine_runtime_select / gpu_policy / engine_ipc.
  Kept both sides of the notes block (main's engine_env_note push and this
  branch's OOM note).
• engines/vllm/src/process.rs — main added import_failure_hint /
  import_failure_line right next to the local quotable_in_single_quotes and
  log_tail_shows_oom that this branch moved into rocm-core. Kept the import
  helpers, dropped the two relocated ones, and repointed the import helper at
  rocm_core::quotable_in_single_quotes /
  rocm_core::strip_terminal_control_sequences.
• docs/manual-testing.md — kept main's new 4a; sections renumbered around
  it (AIM went 9 → 10, where it had collided with Uninstall).

No PR test was lost. I enumerated every function name this branch adds
(59, tests included) and grepped the merged tree for each: all present.
the_stripper_follows_the_escape_grammar_not_just_the_colour_case looks dropped
from engines/vllm/src/process.rs but was deliberately relocated by this branch
into crates/rocm-core/src/terminal.rs.

Scenario renumbering: serve-26 → serve-28. main took
serve-26/serve-27 for the AIM lanes.
feature_naming::scenario_names_are_indexed_sequentially_per_feature requires
the index to equal the declaration position, so appending is the only legal
choice. Every reference moved: the feature file, capability.rs,
serving_steps.rs, docs/testing.md. The @id: is unchanged
(serve-oom-launch-memory-guidance).

Two tests main added after this branch last merged turned red against it, and
both were right to:

• advised_commands::every_advised_command_parses — VLLM_OOM_DIAGNOSE_COMMAND
  ("rocm diagnose --symptom") is a marker prefix, not a runnable invocation.
  Added a NOT_INVOCATIONS entry saying so.
• import_failure_hint_is_part_of_the_startup_failure_message — asserted
  Last 80 lines of startup log:, which this branch replaced with
  Startup log, last 80 lines as the terminal drew them:.

The 2026-10-22 automated round

Taken, with the file each landed in:

• rocm daemon leaked by serve-28 (apps/rocm/src/serve_cmd.rs:954) —
  the fault hook writes a starting record for a process that never existed,
  and serve then started the background helper to supervise it. Nothing in
  teardown stopped it. Now skipped when the launch is fabricated.
  e2e_simulate_oom_launch is a const fn returning false without the
  feature, so the branch folds away in shipped builds. Falsified rather than
  assumed: pgrep -af "rocm daemon" before and after
  cargo xtask e2e -- -n "serve-28" on the Linux box — nothing left behind.
• append_oom_serve_note grows main.rs (AGENTS.md §6) — moved, with its
  six unit tests, into apps/rocm/src/serve_summary.rs:378, next to
  oom_memory_note and serve_failed_to_become_ready. Pure relocation.
• crates/rocm-core/src/vllm_oom.rs:190 — the doc example contradicted the
  rule. The "losing" line it used (RuntimeError: ... killed: out of memory) is
  sub-threshold and is not a candidate at all, which
  sub_threshold_lines_carry_no_hint_at_all already pins. Both example lines
  now score, and the false "callers print the whole log tail beside the hint"
  is scoped to the engine surface — the serve note does not.
• apps/rocm/src/main.rs:514 — rocm serve --help promised the user's own
  failing line with no fallback caveat. Added.
• engines/vllm/src/process.rs:690 — cited rocm_core::vllm_log_segments,
  a path that does not resolve (private to rocm_core::vllm_oom). Cites the
  file now, and says why it is spelled that way.
• docs/testing.md:1963 — the claim that there is deliberately no focused
  command was wrong. cargo xtask e2e -- -n "serve-28" works, because xtask
  builds the binary with the hook and sets the capability. Verified by
  running it: 1 scenario, 5/5 steps, exit 0. The filter-bypass hazard is real
  only against a prebuilt ROCM_CLI_BINARY; the warning now says that.
• Stale workflow comments (nightly.yml, e2e-selfhosted.yml) — they still
  said the prebuilt e2e-test-hooks set "must match what cargo xtask e2e
  would build". It is a floor, not equality, since xtask also enables
  e2e-oom-fault-injection. Reworded, with a note on why the fault-injection
  hook is deliberately absent there (reported skip, not silent pass).
• The blocking squash-body finding — taken the route the review offers:
  the PR description's banner now instructs whoever merges to write the
  squash body by hand from the final diff, and lists what the stale commits
  narrate (the reuse pre-gate, now in feat(serve): exempt reuse of a running service from the no-GPU refusal #703; the --gpu validation work, already
  on main via feat(vllm): Tackle out of memory errors (EAI-8058) #251; 956891f's "gated to Linux" claim, reversed by
  13bd32c). History is not being rewritten — that needs a force-push to a
  shared branch.
• The stale description — banner rewritten, serve-26 → serve-28
  throughout, and the vllm_oom.rs extraction named.

Not taken, deliberately: the ~6-way duplicated two-reads rationale (a
docs-consolidation pass across six files, better on its own), and the
nightly.yml publishing-job feature guard (the same follow-up already agreed
upstream in the workflow_contract.rs thread).

Verification

Darwin cannot build this workspace (engines/lemonade — st_mode is u16
there, u32 on Linux), so the Darwin pre-push hooks cargo-clippy and
cargo-test were skipped (SKIP=cargo-clippy,cargo-test). Everything else
ran — verify-commits, license-headers, cargo fmt, the e2e-harness clippy,
shellcheck — and passed. The two skipped hooks were run for real on a Linux
MI300X box, against the exact merged tree (git diff origin/main..HEAD applied
onto origin/main):

cargo build --workspace --all-targets                      -> exit 0
cargo clippy --workspace --all-targets -- -D warnings      -> exit 0
cargo test --workspace --all-targets --no-fail-fast        -> exit 0
   76 test binaries, 4840 passed, 0 failed
cargo fmt --all -- --check                                 -> exit 0
cargo xtask e2e -- -n "serve-28"                           -> exit 0
   1 scenario (1 passed), 5 steps (5 passed), 0 unexpected failures

What I could not verify. That box has KFD nodes but no amd-smi/rocm-smi
and no usable GPU telemetry, so a real GPU OOM was not reproducible: the
end-to-end evidence is the fault-injected launch (serve-28) plus the unit
coverage, not a card that actually ran out of memory. I also could not run the
self-hosted E2E tests (GPU) or E2E tests (Strix Halo, Ubuntu) lanes — CI
remains the only authority on those.

[nowycondro]

🤖 Automated review (Claude Code) · 4187652

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 (28 files, +3174/-310). Implements OOM guidance on a failed rocm serve (ROCMAI-419, ex EAI-8059): the serve-summary OOM note, the shared rocm_core::vllm_oom reader, the shell-quoting guard's move into rocm-core, the e2e-oom-fault-injection feature, serve-28, and docs. The serve-behavior/reuse-exemption half was correctly split out to #703 per the PR's own note; this review covers only what remains in this branch.

Blocking

• simulate_oom_managed_launch's fabricated service record never pairs its PID with a start-time token, so a later rocm services stop can SIGKILL an unrelated process tree once that PID is recycled — apps/rocm/src/service_launch.rs:587-603 (builds the record via ManagedServiceRecord::new(..., std::process::id(), ...) and never calls record_supervisor_identity/record_engine_identity afterward, unlike the real spawn path at :532-534), crates/rocm-core/src/proc_lifecycle.rs:103 (identity_state's (None, _) => IdentityState::Matches — a PID recorded without a token is trusted on sight), apps/rocm/src/services_text.rs:712-734 (terminate_recorded_service_pids builds ProcessIdentity::new(record.supervisor_pid, record.supervisor_start_ticks) for exactly this record and calls terminate_verified(..., KillScope::Tree, force: true)), tests/e2e-cucumber/tests/e2e.rs:708-751 (stop_managed_services teardown runs rocm services stop <id> --yes unconditionally against every on-disk record it finds, with no status-based guard before terminate).
  record_supervisor_identity's own doc comment names this exact hazard ("a PID written on its own is signalled on trust alone once it has been recycled"). The short-lived rocm serve invocation that writes std::process::id() exits almost immediately, freeing the PID for reuse; on a Linux CI host (serve-28 is @requires-os:linux) that reassigns the stale PID to any live process before scenario teardown, that unrelated process and its whole descendant tree would receive SIGTERM/SIGKILL.
  Confidence 85 · architectural · traced end-to-end against the source (reachability and match-arm behavior confirmed directly; PID-reuse itself not reproduced live — no toolchain in this environment). Fix: call record.record_supervisor_identity(std::process::id()) immediately after constructing the record in simulate_oom_managed_launch, while the process is genuinely still alive, so identity_state can later report Recycled/Gone instead of always Matches.

• import_failure_hint was not given the same "rendered-row" fix as its stated sibling oom_utilization_hint, so it can still weld two terminal rows into one fabricated displayed line — engines/vllm/src/process.rs:751-769 (import_failure_hint, computes symptom_line via the old flat rocm_core::strip_terminal_control_sequences(raw_line)) vs. engines/vllm/src/process.rs:710-722 (oom_utilization_hint, rewritten in this PR to render via rocm_core::vllm_oom_display_line, specifically to avoid welding a bare-\r progress-bar segment into the diagnosed line — the exact bug three new tests in this PR guard against for the OOM path).
  import_failure_hint's own doc comment calls itself "the sibling of oom_utilization_hint," but a one-\n-terminated physical line containing an embedded bare \r (e.g. a progress-bar repaint immediately followed by an import traceback) would still render as one welded, never-actually-printed string for the import path, while the OOM path now correctly splits and picks the scoring row. The printed --symptom command itself stays shell-safe either way (\r is char::is_control(), so quotable_in_single_quotes still rejects it and falls back to the canonical symptom) — this is a display-only inconsistency, not a shell-safety issue, and the real-world trigger window is narrow (vLLM's import failures occur before any weight-loading progress bar exists). No test exercises this path's control-byte handling either way.
  Confidence 75 · logic/consistency · Fix: render import_failure_hint's symptom_line through the same rendered-row-aware path as oom_utilization_hint (reuse terminal::rendered_lines directly, or add a sibling to vllm_oom_display_line), and add a test analogous to the_echoed_tail_never_welds_two_rendered_rows_into_one_line for the import-failure case.

Non-blocking

None.

Decisions for the author

• The CLI's note can miss an OOM the engine saw, when the engine's own bail pushes the original line outside the CLI's 80-line re-read window — answered-by-intent. The PR's own "Known gap" section states this exactly (measured: 77 lines after the OOM line already suppresses the note entirely), explains why the obvious fix (re-windowing before the bail) trades it for the colourised-quoting regression this PR's own tests prevent, names the correct structural fix as out of scope, and pins the gap with a dedicated test (the_cli_window_can_miss_an_oom_the_engine_saw). Nothing above adds to this; it would otherwise have been a blocking finding.
• README.md:934's new OOM-note link to docs/vllm.md uses an absolute GitHub blob URL rather than a relative link, while the identical target is linked relatively three lines away (README.md:43) and every other new cross-doc link this PR adds is relative — non-blocking-improvement. AGENTS.md §5's stated rationale for preferring relative links is specifically about a bare backtick path defeating the docs-links/lychee checks; an absolute URL is still a real, checked link (lychee would catch it going stale), so this reads as a style inconsistency rather than a rule violation. Worth tidying for consistency, not blocking.

Positive signals

• The OOM-detection rewrite is mutation-tested with its results recorded directly in a doc comment (which specific mutations were confirmed to survive the pre-existing suite before the new tests were added) — a notably rigorous level of self-verification.
• The self-quoting bug (the engine's own previously-written hint being re-selected as "the user's error" on a second read of the shared log) is fixed via is_rocm_authored_log_line, keyed off shared constants so the hint and the filter cannot drift, and backed by a dedicated regression test that reproduces the exact two-read scenario rather than unit-testing the exclusion in isolation.
• quotable_in_single_quotes is invoked before interpolation at every call site that builds a rocm diagnose --symptom '...' recommendation (both engine hints and the CLI's serve_summary.rs::oom_memory_note), so a control-byte or apostrophe-bearing line can never reach a shell-quoted command raw on any surface.
• simulate_oom_managed_launch's fixture deliberately reproduces the full shared-log shape (real line, echo, header, engine's own hint sentence) rather than just a raw OOM line, specifically so the self-quoting bug class above would have been caught by this fixture had it still existed — good adversarial test design, independent of the identity-pairing gap noted above.
• The PR description itself is unusually rigorous: it tracks its own review history verbatim, withdraws claims later found false rather than quietly dropping them, and states every known gap and deferral explicitly with the reasoning for not fixing it now.

Deployment notes

• e2e-oom-fault-injection is confirmed feature-gated out of release builds (release.yml never passes it) — the PID-identity gap above is confined to the mock E2E lane and its CI runners, not shipped behavior.
• No other migration, new dependency, or user-facing default change beyond what the PR description's own "Behavior coverage"/"Stated gap" sections already disclose.

What this covered

Every one of the 28 changed files, read completely, across 6 independent blind fan-out workers grouped by coupling: (1) crates/rocm-core/src/vllm_oom.rs+terminal.rs+lib.rs; (2) crates/rocm-core/src/diagnose.rs+fix.rs; (3) apps/rocm/src/serve_summary.rs+serve_cmd.rs+advised_commands.rs+Cargo.toml+main.rs diff; (4) engines/vllm/src/process.rs+apps/rocm/src/service_launch.rs (this group initially failed to return and was re-run after the first attempt was lost to a session-level concurrency limit — two independent passes over this group ultimately landed, one finding each, both included above); (5) the e2e test infrastructure (model_serving.feature, capability.rs, expectation.rs, serving_steps.rs, tui_driver.rs, crates/e2e-report/src/lib.rs, xtask/src/e2e.rs); (6) docs (README.md, docs/architecture.md, docs/manual-testing.md, docs/testing.md, docs/vllm.md) and the two CI workflow comment-only diffs. Each worker's candidates were re-verified against source before inclusion.

CI at the reviewed commit: all checks that can report on a GitHub-hosted lane are green (build-and-test, windows-build-and-test, Test (affected crates), E2E tests, E2E consolidated report, CodeQL, clippy, coverage, Sphinx docs build, lychee, license/notices, commit-signatures). Two self-hosted Strix Halo Ubuntu lanes (nightly, release) show cancelled rather than a pass; MI300X and W7900 lanes were still queued at check time — none had failed.

Did not run as separate passes: "history" (git log/git blame) — the PR's own extensive, already-reconciled review history covers this better than a fresh blame sweep would for a 123-commit branch; "prior-changes" — this method does not read prior review threads, so whether earlier review comments apply here could not be and was not checked. No Rust toolchain was available in this environment, so no finding above rests on an executed run; each is argued from direct source tracing instead. Ticket ROCMAI-419 was not independently retrieved (no Jira tool available in this environment); the PR's own description was treated as the account of its scope, per the method's rule that a stated intent is read but not substituted for retrieval where retrieval is possible — it was not possible here, so this is recorded as a gap rather than assumed. Not reconciled against any discussion on this PR beyond the mandatory claim-check, which surfaced no reviews at this exact head.

---

🤖 Generated with Claude Code

[r0x0r]

Merged main again (it moved 4 commits under this branch). Pushed as 28843727.

One conflict, in tests/e2e-cucumber/src/capability.rs: both sides added tests to the same module — this branch's only_the_exact_value_the_producer_writes_arms_the_hook (the value half of the @requires-oom-fault-injection contract) and main's three permission_probe_* tests. Kept both; nothing was dropped.

Verified on the Linux MI300X box with the merged tree applied onto main (81f4a46f): cargo clippy --workspace --all-targets -- -D warnings exit 0, cargo test --workspace --all-targets exit 0 across 76 suites with 0 failures, cargo fmt --all -- --check exit 0.

Heads-up on a scenario-number collision

This branch and #703 both currently declare serve-28. feature_naming requires the number to equal the scenario's declaration position, so this cannot be pre-emptively resolved on both sides — renumbering here before #703 lands would turn this branch's own check red.

#703 is approved and adds serve-28..32, so the natural order is: #703 merges first, then this branch renumbers its scenario to serve-33 on its next main merge. Flagging it so whoever merges does not hit it cold.

[jussielo-amd]

🔴 AI-assisted review (Claude Code) · head 28843727

This is an assisted human review, not an autonomous merge gate: no GitHub approval is being filed, and the merge decision stays with maintainers. The change request below is for the blocking items only.

Review — needs work

Full review of the PR's own diff at 28843727 (base 6eeec620, merge-base 81f4a46f; 28 files, +3174/−310). The ticket was not retrievable from here, so its scope was not checked. This round re-checks the earlier review at ebc26718 against the author's commit 4187652a and the main merge.

Blocking

1. The head cannot merge and no CI has run on it. engines/vllm/src/process.rs has 4 conflict hunks against main, which has since moved the startup diagnostics into engines/vllm/src/startup_diagnostics.rs. The PR's hunks and about 900 lines of tests have to be ported there. No workflow run exists for this head (GitHub skips PR workflows on a conflicting PR), and the previous commit's E2E self-hosted run was cancelled, so the Windows E2E that was red on ebc26718 has not been re-run to a result. Fix: rebase, port, get a full CI run.
2. The release-guard test passes when its guard is broken. xtask/src/workflow_contract.rs:4246-4275 vs :4317-4350: the_release_feature_guard_catches_every_spelling re-spells the predicate in its own closure instead of calling the real loop. Ran in a scratch copy: with offenders.push made a no-op and the feature added to release.yml, both tests stayed green. Fix: extract one release_feature_offenders() and call it from both.
3. The new helper-spawn guard is untested and its comment is false. apps/rocm/src/serve_cmd.rs:954-966: deleting if !e2e_simulate_oom_launch() leaves serve-28 green and a rocm daemon process running (ran). The comment says the note "is computed before it would have run", but the helper call (line 965) comes before the note computation (line 986 onward). Fix: assert no daemon state or autostart claim remains after the scenario, and correct the comment.

Non-blocking

1. Note vanishes on one invalid UTF-8 byte (open from last round): serve_summary.rs:429-435, services_text.rs:1289-1303. The CLI uses strict lines() over the whole file; the engine uses lossy rocm_core::tail_lines. Ran: a stray byte on line 1 gave no note. Use the engine's reader.
2. "may still be loading" prints beside "ran out of GPU memory" (open): serve_summary.rs:166-185, main.rs:510-523. Ran. --help says the note is never shown for a still-loading server, but for vLLM that state is normally starting, which is eligible. The README also omits that the log is read once after a fixed 45 s wait.
3. OOM selector is slow on the failure path (new): vllm_oom.rs:96-100, 221-229, diagnose.rs:2268-2285. It runs the whole diagnose registry on two hosts per row. Measured by a review pass: 234 ms for 80 lines, 14.3 s for 6,000 \r rows. Pre-filter, or score only fix-16.
4. Stale docs: terminal.rs:13-15, 415 and docs/architecture.md:160 ("two callers", now three). The stripper's only production use is an unreachable fallback.
5. Fault-injection guard covers release.yml only, and docs/testing.md:1957-1961 lists the wrong test. Ran: adding the feature to the nightly publishing builds and the ci.yml bundle build kept all 76 contract tests green.
6. docs/testing.md:1979-1981 is wrong: the only CI dispatch filter runs a prebuilt binary, so name_filter=serve-28 runs without the hook.
7. Inaccurate or history-narrating comments:
   • capability.rs, expectation.rs, xtask/src/e2e.rs, the feature file and the README call the lane binary "the shipping release build" and name the wrong skip gate (requires-no-gpu is checked first).
   • lib.rs:272, fix.rs:3142, serving_steps.rs:1378 and e2e-report narrate history.
   • service_launch.rs:568 links to crate::append_oom_serve_note, which has moved to serve_summary.
8. Hint/command rendering is duplicated in process.rs:721-727 and serve_summary.rs:246-251, and the fixture hand-copies the engine's bail text. One shared function would make "same guard" true by construction.
9. Two tests claim more than they check: the last block of the_colourised_round_trip… tests a filter built inside the test, and the_cli_window_can_miss_an_oom… pins a defect with a hard-coded 77.
10. serve-28 never runs the printed rocm diagnose command (it does route to fix-16, checked by hand), and the "no note when piped" promise has no test.
11. PR body is partly stale: verification is pinned to 7bc22302, it says "re-verified" with no CI behind it, and it names an internal review tool.

Decisions for the author

• CLI window can miss an OOM the engine diagnosed (answered-by-intent): the description's "Known gap" says this is deliberate. Confirm the structured-record follow-up.
• No scenario for the engine's startup-failure text (answered-by-intent): the description's "Stated gap" names the lane and why it is not added. Confirm.
• A bare HIP error: out of memory no longer gets a hint (tradeoff): the engine hint and rocm diagnose stay in step, but the docs/vllm.md reason for dropping this string is unsupported. Confirm it was deliberate.

Since the last review

• Fixed: serve-26 renumbered to serve-28; append_oom_serve_note moved to serve_summary.rs; the false vllm_oom.rs doc paragraph; the process.rs comment path; the --help apostrophe caveat; the advised_commands.rs exclusion; the workflow comments now say "floor".
• Open: non-blocking items 1, 2, 4, 5, 8, 9, 10, 11, and the conflict/CI gap (now on a different file).

Deployment notes

• The rebase is real work: main also has quoting.rs and the AIM serve-26/serve-27.
• The squash-body note in the PR banner still applies.
• The engine's startup-failure header text changed.
• The e2e-oom-fault-injection feature is built only by cargo xtask e2e.

What this covered

Three independent passes read the PR diff, and I re-verified the key findings against the PR head. Ran in scratch copies: engine and serve-summary tests, xtask and contract tests, a hooked serve-28 run, clippy, and mutation edits.

Not run: the full workspace gate and smoke_local.py, Windows paths, and a history pass on pre-existing regions. The review threads were not read or reconciled. The fixes on this PR were written by its author.

[r0x0r]

@jussielo-amd — all three blocking items addressed. Pushed as e229c692.

1. The head cannot merge / no CI has run

Merged. main had moved the vLLM startup diagnostics into engines/vllm/src/startup_diagnostics.rs, so all 4 conflict hunks in process.rs had an empty main side — code moved, not deleted.

Resolved by comparing bodies rather than names, because name-presence is not body-equivalence here: of the ~32 same-named symbols in the conflicted regions, 3 genuinely differ and carry this PR's whole contribution — startup_log_context, oom_utilization_hint, import_failure_hint. Those three are ported over main's copies in startup_diagnostics.rs; the rest take main's. In particular startup_log_context's switch to rocm_core::terminal::rendered_lines survived — that is the fix for stripping welding Loading weights: 90%\rtorch.OutOfMemoryError… into one fake line that then got scored and quoted into the rocm diagnose --symptom command.

All 7 PR-authored tests in those hunks were relocated into startup_diagnostics.rs and verified present by name in the final tree.

2. The release-guard test passed when its guard was broken

You were right, and the mutation reproduces exactly as you described. Extracted one release_feature_offenders(&str) -> (usize, Vec<String>); both the_release_workflow_builds_no_feature_gated_binaries and the_release_feature_guard_catches_every_spelling now call it, so the spelling test exercises the real loop instead of a copy.

Proven falsifiable rather than asserted: with offenders.push made a no-op, the_release_feature_guard_catches_every_spelling now FAILS (assertion failed: caught("cargo build --release -p rocm --features rocm/e2e-test-hooks"), workflow_contract.rs:4351), and passes again on restore. Before this change the same mutation left both tests green.

3. The helper-spawn guard was untested and its comment was false

Both fixed.

serve-28 now ends with And no background automation daemon was left behind, which asserts neither artefact the autostart path can leave survives the fabricated launch: no running: true in automations/runtime-state.json, and no autostart.claim. Both are checked because the daemon publishes its runtime-state only once it is up, so a spawn caught in the window before that shows up as a claim and nothing else.

On the comment — you were right that it was backwards. The suppression runs before the note is computed in the summary_mode block, so "the note is computed before it would have run" could not have justified it. It now states the real justification: there is no spawned process to supervise, and serve-28's new assertion is what holds the branch.

Verification

Linux MI300X box, merged tree applied onto main (6eeec620):

• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo test --workspace --all-targets — exit 0, 77 suites, 0 failures
• cargo fmt --all -- --check — exit 0

cargo-clippy/cargo-test were skipped on the push only because engines/lemonade cannot compile on Darwin (st_mode is u16 there, u32 on Linux); both ran on Linux as above. Every other hook ran, including the signature/sign-off gate.

Scenario numbering

This branch still declares serve-28, deliberately. Sibling PR #703 is approved and declares serve-28..32, and feature_naming requires the number to equal the declaration position — so renumbering here before #703 lands would turn this branch's own check red. Once #703 merges, this becomes serve-33 on the next main merge. Flagging so it is not read as an oversight.

Your three decision questions — all confirmed deliberate

• CLI window can miss an OOM the engine diagnosed — deliberate, and the structured-record follow-up is the intended fix rather than widening the window here.
• No scenario for the engine's startup-failure text — deliberate; the lane and the reason are named in the description.
• A bare HIP error: out of memory no longer gets a hint — deliberate, so the engine hint and rocm diagnose stay in step. You are right that the docs/vllm.md reason is thin; the real reason is step-keeping, not the one stated there.

Still open from your non-blocking list

Not fixed in this round, and I would rather say so than imply otherwise: #1 (strict lines() vs the engine's lossy reader — a real correctness bug), #2, #5, #8, #9, #10 and the remaining parts of #11. #4 and #7 are partly done. Happy to take #1 in the next round since it is the only one with user-visible wrong behaviour.

Not verifiable here either way: the box has no amd-smi/rocm-smi and no GPU telemetry, so a real GPU OOM — this PR's subject — rests on fault injection and unit coverage, not hardware.

All three blocking items addressed in e229c69: the branch merges again (main moved vLLM startup diagnostics to startup_diagnostics.rs; 3 production fns ported, 7 tests relocated and verified present); the release-feature guard is now one extracted release_feature_offenders called by both tests, proven by mutation to fail where it previously stayed green; and serve-28 now asserts no daemon runtime-state or autostart claim survives, with the false ordering comment corrected. Non-blocking items 1, 2, 5, 8, 9, 10 and parts of 11 remain open and are listed as such in the comment. Re-requesting your review.

[r0x0r]

Merged main now that #703 has landed, so this branch's scenario finally renumbers: serve-28 → serve-33, with every reference that belongs to this branch moved with it (docs/testing.md, capability.rs, serving_steps.rs, serve_cmd.rs). #703's own cross-references to serve-28/30/31/32 are deliberately left alone.

The substantive overlap was the no-usable-GPU bail in serve(). #703 refactored it onto no_usable_gpu_refusal(&device_policy) and reuse_existing && host_has_no_usable_gpu; this branch adds !e2e_simulate_oom_launch() so the fault-injected launch is not refused before it can fabricate its OOM. The merged condition carries both terms. In docs/manual-testing.md both sides claimed ## 5, so the reuse section keeps it and the memory-guidance section becomes ## 6, with the sections after it shifted to 7..11.

One thing worth recording, because it nearly shipped: my first resolution of serving_steps.rs welded two unrelated step functions together, leaving the file syntactically broken. Neither cargo check -p e2e-cucumber --all-targets nor cargo test --workspace --all-targets caught it — the e2e test target sets harness = false and is excluded from the default test set, so it is not built by either. Only cargo fmt --check noticed. The correct gate is cargo clippy -p e2e-cucumber --test e2e, which is now part of this branch's verification and is green.

Verified on a Linux MI300X box with the merged tree applied onto main (c39ee0c6): cargo clippy --workspace --all-targets -- -D warnings exit 0; cargo clippy -p e2e-cucumber --test e2e -- -D warnings exit 0; cargo test --workspace --all-targets exit 0 across 76 suites with 0 failures; cargo fmt --all -- --check exit 0.

[jussielo-amd]

Thanks for the work on this, and for the last round. I confirmed the release-guard test now calls the real release_feature_offenders, and the helper-spawn guard has its serve-33 assertion. I read the whole diff at fe213ce7 from the code. Two things block, and both are cheap:

Blocking

1. Today's renumber merge left the docs wrong.
   • The existing reuse scenario is still serve-28 - Reuse succeeds on a GPU-less host in model_serving.feature (L395), but docs/testing.md now calls it serve-33 in several places (~L2063, 2068, 2072, 2093-94, 2114). So serve-33 names two different scenarios, and -n "serve-33 - Reuse..." selects nothing. The new OOM scenario is the real serve-33; the reuse text should be serve-28 again.
   • The new "Serve OOM Guidance" section is spliced into the middle of a sentence: L1993 ends "E2E scenarios, in" and the sentence only resumes at ~L2047, so the reuse scenario list now sits under the OOM heading. Moving the OOM block after the serve-31 bullet fixes it.
   • docs/manual-testing.md: ## 6 has subsections ### 5a/### 5b, and terminal.rs:203 cites "§5a". They should be 6a/6b and §6a.
   • Still open from last round: "The CI dispatch filter takes the same route" (~L2142) is wrong. The dispatch filter runs a prebuilt binary, so name_filter=serve-33 runs without the hook.
2. The branch conflicts with main again (crates/rocm-core/src/lib.rs; GitHub shows it as dirty). It needs another merge and a full CI run.

Non-blocking

• The two notes can still print together for the exact launch this targets: "may still be loading" (serve_summary.rs ~L166-182) beside "ran out of GPU memory". No test asserts the first is absent.
• serve_summary.rs's module doc still says it is engine-agnostic, but this adds ~600 lines of vLLM-specific OOM logic. AGENTS.md §6 points to its own file.
• Still open from last round: the OOM selector's cost on the failure path, the note vanishing on one invalid UTF-8 byte, and no test for the "no note when piped" promise.
• service_launch.rs:701 links crate::append_oom_serve_note; it now lives in serve_summary.
• The release guard misses PowerShell backtick continuations (release.yml has a shell: pwsh job), counts each run: | build twice and doesn't strip comments.
• docs/architecture.md still says the terminal stripper has "two callers" and describes startup_diagnostics.rs as stripping the tail.
• The fixtures hand-copy the engine's bail header, so serve-33 would stay green if the real text drifts.

CI: hosted checks are green; the Strix Halo Windows lane is red (it fails on unrelated PRs too) and MI300X is still queued. Not run: tests (read-only review).

---

🤖 by agenthub on AMD AgentHub