[jussielo-amd]

Summary

The wait that runs between GPU serve scenarios, wait_for_free_vram in the serving steps, judges the wrong memory pool on Strix Halo. On an APU the VRAM total that amd-smi and rocm-smi report is only the fixed BIOS carveout, while the engine allocates from GTT-backed system memory. On the Strix Halo lane hosts the preflight step prints VRAM 203321344/536870912, GTT 41787392/107374182400 (used/total in bytes), so the carveout is 512 MiB with 194 MiB already in use and the engine's pool is about 100 GiB. The harness took 90% of the 512 MiB total as its floor, which is 450 MiB, against 318 MiB free. That can never be met, so every GPU serve waited out the full two-minute VRAM_DRAIN_DEADLINE and then logged device state: NOT drained after 120s (319 MiB free of 512 MiB, floor 450 MiB) — a previous engine is still holding the GPU, even though the preflight step had just reported reclaim: no leaked E2E engine processes found and GPU ready: 99 GiB GTT free. The same 319/512 reading appears at every failing step on two different hosts, which is why I read it as a mis-sized gate and not as a leaked engine.

This change picks the pool the same way .github/scripts/gpu-preflight.sh already does: when the VRAM total is under the 8 GiB carveout limit (the floor the Strix Halo lanes pass as GPU_PREFLIGHT_MIN_FREE_GIB) and a non-empty GTT reading exists, the drain wait judges GTT, with the existing "model-sized cap, never more than 90% of the pool" floor applied to it. Otherwise it judges VRAM exactly as before, so discrete cards and hosts without any GTT figure behave as they did. GTT is read from rocm-smi --showmeminfo gtt --csv, and only when the VRAM total is small enough to be a carveout; amd-smi is not asked for GTT because there is no field in the repo that has been checked against its output, whereas the preflight step already reads rocm-smi's GTT rows on these hosts. The pool choice and the CSV parsing live in tests/e2e-cucumber/src/device_memory.rs, and the serving steps call it. Every amd-smi and rocm-smi call in the wait also goes through bounded_stdout in that module, which gives a call ten seconds and then kills and reaps the child, so a stalled tool cannot block the test worker. A probe reports one of three results: absent (the tool is not installed), unreadable (it ran but gave no usable reading: a timeout, a non-zero exit, output that does not parse, or a zero VRAM or GTT total) or a reading. Only absent skips the wait as no GPU tooling. An unreadable GTT read on a carveout host makes the whole sample unreadable, so the wait gives up after three unreadable samples instead of judging the small carveout, which could report drained while the engine still holds its GTT-backed memory; an installed tool that never gives a single reading is given up on after three samples and reported as unreadable, not as not drained; one that has read before is waited on to the deadline and reported with its last reading. Each probe is held to the time left, so the wait ends at two minutes and the line reports the elapsed time. When a tool runs but its output is not understood, the line also says what it printed (the first line, for example the CSV header), so the first Strix Halo lane run confirms or refutes the GTT column names. The decision to read GTT at all and the per-sample decision both live in device_memory.rs (sample_device_memory, judge) so they are unit-tested; the wait's deadline now starts before the first probe, and the probes run under spawn_blocking. The two device-state lines keep their shape on VRAM hosts and name the pool ( GTT) when GTT was used. I also reworded the timeout line from "is still holding the GPU" to "may still be holding the GPU", since a timeout shows the pool never reached the floor, not which process holds it.

Expected effect

I estimate this saves about 16 to 20 minutes per Strix Halo Ubuntu lane run, from the four scenarios that each spent about 295 seconds failing to start a serve (120 s of them in this wait) plus the same 120 s of padding on the passing serves. That figure is inferred from the lane logs and is not measured; the lane duration after this merges is the real check. It does not address the AIM scenarios, which are handled separately, and it does not change why those four scenarios fail.

Tests

The new unit tests in device_memory.rs use the real preflight reading for Strix Halo (idle host is drained by GTT; a host with most of its GTT held is not) and an MI300X-sized card (still judged by VRAM with the 150000 MiB cap, and not drained when most VRAM is held). They also cover a host with no GTT tool (keeps the VRAM behaviour and the 450 MiB floor), a failed GTT probe or a zero GTT total on a carveout host, and a zero VRAM total on any host (each an unreadable sample, as the preflight script treats a zero total; a zero total would otherwise give a floor of zero and an instant drained), whether the GTT probe is consulted on carveout and discrete hosts, absent versus unreadable tools in the amd-smi then rocm-smi fallback and in the first sample, a VRAM total exactly at the carveout limit (private memory), the rocm-smi CSV parsing for both pools, and bounded_stdout itself (output, non-zero exit, missing program, and a child that outlives its deadline and must be killed and reaped). I confirmed the new tests fail when their target is broken: forcing the GTT gate to always read, letting an unreadable GTT read fall back to VRAM, letting an absent tool mask an unreadable one in the fallback, accepting a zero VRAM total, not classifying a missing program as absent, and mapping an unreadable sample to no tooling each turn tests red, and each was restored. cargo test -p e2e-cucumber --lib, cargo clippy -p e2e-cucumber --all-targets -- -D warnings and cargo fmt --check pass, and the pre-push hooks (fmt, clippy including the e2e harness, cargo test) passed on push.

The 8 GiB carveout limit is documented as the floor the APU lanes pass as GPU_PREFLIGHT_MIN_FREE_GIB, and nothing tied the two together, so a workflow contract test in xtask/src/workflow_contract.rs (the_serve_drain_carveout_limit_is_the_apu_lanes_preflight_floor) now reads CARVEOUT_MAX_MIB from the harness source as text (no new crate edge) and requires it to equal the smallest preflight floor any lane sets. It goes red if either the constant or a lane's floor is changed on its own; I checked both.

The follow-up for review adds tests that drive samples through DrainWait and assert the line it produces: giving up after three samples from a tool that has never read, not giving up after a reading, each probe held to the time left, "NOT drained" only for a reading below its floor, and the timeout line's note when only the BIOS carveout could be read because rocm-smi is missing. I broke the give-up rule, the unreadable-versus-not-drained choice, the probe budget and the carveout note (six mutations in all); each turned the matching test red and was restored.

No Gherkin scenario: this is test-harness code only and does not change what the CLI prints or does, so there is no user-observable behaviour to cover.

Not run locally

The GPU lanes. There is no amd-smi or rocm-smi on the machine I used, so device_memory_mib and the wait loop were compiled and linted but not exercised against real tool output; the rocm-smi GTT CSV column names (GTT Total Memory, GTT Total Used Memory) come from the field names the preflight script and the Windows preflight already parse, and the parser is unit-tested against that shape. If a lane's rocm-smi words the GTT CSV differently, the GTT probe is unreadable and the wait gives up after three unreadable samples instead of judging the carveout, so the first Strix Halo lane run after this merges is where that would show up, and the timeout line now names the header the tool printed so that run settles it. The wait loop itself is async and spawns the real tools, so it has no unit test; the per-sample decisions, the verdict wording and the probe budget are the tested judge, DrainWait, describe_verdict and probe_budget; only the loop that calls them is untested. A host that has rocm-smi installed but no usable GPU reading now gives up after three unreadable samples and says it could not read device memory.

[Copilot]

🟡 Changes recommended

The synchronous rocm-smi GTT probe can stall the test worker beyond the drain deadline and needs a subprocess timeout that terminates and reaps the child.

1 open issue

• Make the GTT probe timeout-aware to avoid blocking the drain deadline · New

What changed in this PR

This PR updates the GPU serve test harness to judge memory drain against GTT on small-carveout APUs such as Strix Halo, avoiding unnecessary waits while preserving VRAM behavior elsewhere.

Changes:

• Selects GTT when VRAM is below 8 GiB and usable GTT data exists.
• Extracts pool selection and CSV parsing into a unit-tested module.
• Labels GTT diagnostics and softens the timeout message’s attribution to a previous engine.

Verification was limited to the supplied diff; repository files were unavailable and GPU checks were not run.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​serving_steps.rs	Integrates GTT probing, pool selection, and updated drain diagnostics.
tests/​e2e-cucumber/​src/​lib.rs	Exports the device-memory module.
tests/​e2e-cucumber/​src/​device_memory.rs	Adds pool selection, CSV parsing, and regression tests.

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The new GTT probe needs validation against real rocm-smi CSV output on an affected Strix Halo GPU lane to confirm it selects GTT and avoids the unnecessary two-minute delay.

No open issues 🎉

1 resolved since last review

• Make the GTT probe timeout-aware to avoid blocking the drain deadline

_{Review effort: Balanced}

[nowycondro]

🤖 Automated review (Claude Code) · 32daa97

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. Scope note: the task-supplied base sha (7633158) is not an ancestor of the head commit (it is a later, unrelated commit on main from PR #850), so the actual PR diff was taken against the real merge-base e6d2f7c0, matching GitHub's own three-dot diff (490 insertions / 71 deletions across 3 files, confirmed against the PR's additions/deletions metadata).
No ticket named. This is a self-contained E2E-harness reliability fix (serve drain-wait judges the wrong memory pool on APU hosts); the description accurately matches the code.

Blocking

• A transient/failed GTT probe is indistinguishable from "this host has no GTT pool," so it silently falls back to judging the (usually irrelevant) VRAM carveout and can report a false "drained." — tests/e2e-cucumber/src/device_memory.rs:102-107, tests/e2e-cucumber/tests/e2e/serving_steps.rs:337-349
device_memory_mib() only attempts the GTT probe when vram_total < CARVEOUT_MAX_MIB (the APU case), but bounded_stdout(...).and_then(parse_rocm_smi_csv_pool...) collapses every failure mode — spawn failure, non-zero exit, SMI_PROBE_TIMEOUT expiry (the exact "wedged driver" case this PR's own second commit was written to tolerate), or an unparseable/zero-total CSV — into the same gtt: None as "this host genuinely has no GTT pool." select_drain_target's catch-all arm (_ => (PoolKind::Vram, memory.vram)) then treats both identically and judges the tiny BIOS carveout instead. On a Strix Halo host this carveout is ~512 MiB with a ~450 MiB floor and (per the PR's own cited log) typically only ~318 MiB free, so it usually stays "not drained" — but the floor is reachable (e.g. with a different display/carveout usage), so a single flaky GTT read can make that iteration of wait_for_free_vram return "drained" while the engine's real GTT-backed allocation is still fully held, reintroducing the exact race this PR exists to close, specifically in the failure mode it was built to handle gracefully. The module's own doc comment claims the zero-GTT-total case "falls back to VRAM, as it does in gpu-preflight.sh" — but gpu-preflight.sh:197-205 does the opposite on a zero GTT total: it treats it as an unreadable sample and continues (retries), it never substitutes VRAM as the judged pool while VRAM is a known carveout. So the claimed equivalence to the script this module was modeled on does not hold, which is independent evidence the Rust side chose the wrong error-handling behavior here. No unit test exercises "GTT probe present-in-principle but fails/returns non-parseable/zero this one time while VRAM succeeds" as distinct from "host has no GTT at all" (a_host_without_gtt_data_keeps_the_vram_behaviour only covers the latter), so nothing would catch a regression here either.
  Confidence 80 · logic · Fix: have device_memory_mib/select_drain_target represent "GTT probe attempted but unreadable" distinctly from "no GTT pool" (e.g. return None for the whole sample, matching how a failed VRAM probe already makes the sample unreadable, or retry within the same tick) so a transient failure costs one sample, not a false "drained" verdict.

Non-blocking

None

Decisions for the author

• wait_for_free_vram probes once up front only to check is_none(), discards that reading, then immediately probes again as the loop's first iteration — non-blocking-improvement
  probe_device_memory().await is called before the loop purely for the "no GPU tooling" short-circuit; its Some(memory) result is thrown away and re-fetched milliseconds later. This doesn't break correctness (the deadline already starts before the first probe), but now that a carveout host's probe can cost up to 3 bounded subprocess calls instead of up to 2, it doubles the up-front cost against the 2-minute deadline for no benefit. Reusing the first Some(memory) to seed the loop would remove the duplicate call.
• Real rocm-smi --showmeminfo gtt --csv output was not validated on actual Strix Halo hardware — tradeoff
  The author's own description discloses this plainly (no rocm-smi/amd-smi on the dev machine) and the column names are unit-tested against the shape documented elsewhere in the repo (gpu-preflight.sh's non-CSV field names). If the real CSV header words GTT differently, parse_rocm_smi_csv_pool returns None and the code takes the same gtt: None path as the blocking finding above — so this tradeoff and that finding compound, but validating against real hardware is reasonably left to the lane run the author names as "the real check."
• bounded_stdout duplicates rocm_core::process::run_with_timeout's poll/kill/reap/abandoned-reader pattern instead of reusing it — tradeoff
  Verified: tests/e2e-cucumber/Cargo.toml lists rocm-core only under [dev-dependencies], so the library target device_memory.rs belongs to cannot reach it without promoting that to a normal dependency (a cargo xtask check-crate-edges-relevant decision per AGENTS.md). A smaller, single-stream duplicate of an already-reviewed pattern is a reasonable call given that constraint, but worth the author confirming no first-party edge change was intended instead.

Positive signals

• bounded_stdout/probe_device_memory correctly follow this repo's own documented subprocess/FFI rule (AGENTS.md's "subprocess and FFI code" checklist): the blocking call gets its own internal deadline (try_wait poll loop + explicit kill()+wait()) rather than being wrapped in an external tokio::time::timeout around spawn_blocking, which is exactly the leak this repo's convention warns against.
• The Strix Halo unit-test fixtures use the real used/total byte pairs from an actual observed CI preflight log (VRAM 203321344/536870912, GTT 41787392/107374182400), grounding the pool-selection tests in real field data rather than arbitrary numbers.

Deployment notes

None — the change is confined to tests/e2e-cucumber/{src/device_memory.rs,src/lib.rs,tests/e2e/serving_steps.rs} (test-harness only; verified via the diff stat), with no production/CLI-facing code touched.

What this covered

Read completely: tests/e2e-cucumber/src/device_memory.rs (new, 408 lines), tests/e2e-cucumber/src/lib.rs (1-line addition), tests/e2e-cucumber/tests/e2e/serving_steps.rs (full file, post-change), cross-referenced against .github/scripts/gpu-preflight.sh (the shell script this module's doc comments claim to mirror), crates/rocm-core/src/process.rs (the existing bounded-subprocess helper this module couldn't depend on), tests/e2e-cucumber/Cargo.toml (dependency placement), and the root AGENTS.md (subprocess/FFI and e2e-cucumber-relevant rules). Diffed e6d2f7c0…32daa972 (the real merge-base, not the task-supplied base sha — see note above). Two independent fan-out passes ran: a code-correctness pass over the three changed files, and a combined history/agent-instruction/code-comment/design-review pass; both converged independently on the GTT-probe-failure-vs-no-GTT conflation reported above as Blocking.
Did not run: the GPU-dependent E2E lanes or any real amd-smi/rocm-smi invocation — no such tooling exists in this review environment, consistent with the PR author's own disclosure that the GPU lanes were not run locally either. Reasoned from the code, the existing unit tests (recomputed by hand against required_free_mib/select_drain_target), and CI's green run instead. CI: all 25 checks for this commit are green or cleanly skipped (no failing required check). Prior-changes pass: no earlier review on these exact files to reconcile against. Not reconciled against any discussion on this PR.

---

🤖 Generated with Claude Code

[juhovainio]

I went through this and found nothing that is wrong outright, but a few things should be tightened before merge. Inline comments cover the first four.

• The vram_total < CARVEOUT_MAX_MIB gate in device_memory_mib is untested. Your mutation check covered select_drain_target only, so the call-site wiring can break without any test going red.
• A timed-out or failing first probe is now reported as "no GPU tooling (mock/local run)" and the wait is skipped, which misreports a wedged driver.
• bounded_stdout kills only the direct child, so a grandchild holding the pipe leaves the reader thread blocked.
• A test comment refers to "the Copilot finding", which readers of the code can't see.
• a_probe_that_outlives_its_deadline... uses a 1.5 s deadline and expects the pid file to exist; on a loaded runner that could flake. Polling for the file would be safer.

Two things for you to decide: GTT is read only via rocm-smi and only when VRAM is under 8 GiB, and the 90% GTT floor means anything else holding more than 10 GiB of GTT makes the wait time out (best-effort, serve proceeds). Also, the GTT CSV column names haven't been checked against real rocm-smi output, and the Strix Halo lane that exercises the real path isn't among this PR's checks.

CI is green, and no Gherkin scenario is needed for a harness-only change.

[juhovainio]

This vram_total < CARVEOUT_MAX_MIB check repeats the rule in select_drain_target, and no test covers it: making it never true keeps every unit test green. Consider moving the "read GTT?" decision into device_memory.rs next to the select rule, and testing it there.

[jussielo-amd]

Good catch: that gate is now a function and has tests. The "read GTT at all?" decision moved into sample_device_memory in device_memory.rs, next to the pool-selection rule, and both now call one is_carveout predicate so they cannot disagree. a_discrete_host_never_makes_the_gtt_call and a_carveout_host_reads_gtt_and_the_sample_carries_it assert whether the GTT probe is consulted. I confirmed they go red by forcing the gate to always read GTT. Fixed in a3b2e16 and its parent commit 393ca08.

[juhovainio]

is_none() now also fires when a probe times out or fails transiently, so a wedged amd-smi/rocm-smi is reported as "no GPU tooling (mock/local run)" and the wait is skipped. Worth telling "tool absent" apart from "tool present but unreadable".

[jussielo-amd]

Agreed, and the same class covered a second case: a failed GTT read on a carveout host used to look like "no GTT here", so the wait could judge the small carveout and report drained while the engine still held its GTT-backed memory. A probe now returns one of three results: absent (tool not installed), unreadable (ran but gave no usable reading, including a timeout, a non-zero exit, unparseable output or a zero GTT total), or a reading. Only absent skips the wait as "no GPU tooling". An unreadable first probe keeps waiting and says "the memory reading was unreadable" if it never reads, and an unreadable GTT read makes the whole sample unreadable instead of falling back to VRAM. The per-sample decision is judge() in device_memory.rs, and an_absent_tool_is_no_tooling_but_an_unreadable_one_is_not goes red if unreadable is mapped back to no tooling. The wait also reuses its first sample now instead of probing twice. Fixed in 393ca08 and a3b2e16. One cost to be aware of: a host that has rocm-smi installed but no usable GPU reading now waits out the 2 minute deadline per serve rather than returning at once, since that is the "present but unreadable" case.

[juhovainio]

"The Copilot finding on the GTT probe" points at a review thread future readers can't see. Please state the behavior being protected instead.

[jussielo-amd]

Reworded in 393ca08: the comment now states the behaviour the test protects (a tool that never returns costs one sample, not the worker, and the child is neither left running nor left unreaped) instead of pointing at a review thread. On the timing concern from the summary: the timed-out probe now has a 3 second deadline instead of 1.5 seconds, and the test polls for the child's pid file rather than reading it once, so a loaded runner has time to finish the write. It is still timing-dependent in the sense that the deadline must not expire before the shell writes its pid, which takes milliseconds, so I kept the real-process test rather than faking the child.

[juhovainio]

On timeout only the direct child is killed. A grandchild holding the stdout pipe keeps the reader thread blocked in read_to_end, leaking a thread per timed-out probe. Probably acceptable given the probe count, but worth a comment.

[jussielo-amd]

Documented in 393ca08. The doc comment on bounded_stdout now says only the direct child is killed, that a grandchild holding the stdout pipe can keep the reader thread blocked until it exits, and why that is accepted here: amd-smi and rocm-smi do not fork helpers that outlive them and the wait makes a handful of calls.

[nowycondro]

🤖 Automated review (Claude Code) · a3b2e16

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. Scope note: the task-supplied base sha (7633158) is not an ancestor of the head commit (it is a later, unrelated commit on main from PR #850), so the diff was taken against the real merge-base e6d2f7c0, matching GitHub's own three-dot diff (774 insertions / 86 deletions across 3 files, matching the PR's own additions/deletions metadata).
No ticket named. This is a self-contained E2E-harness reliability fix (serve drain-wait judges the wrong memory pool on APU hosts); the description accurately matches the code.

Blocking

• A VRAM total of 0 is accepted as a valid reading instead of a failed probe, unlike the symmetric GTT case — and can produce a false "drained" verdict. — tests/e2e-cucumber/src/device_memory.rs:234-249 (sample_device_memory), tests/e2e-cucumber/tests/e2e/serving_steps.rs:356-374 (vram_mib)
  sample_device_memory's VRAM branch takes whatever vram() returns with no zero-guard: let (vram_total, vram_free) = match vram() { Probe::Read(reading) => reading, ... }. Its GTT branch, a few lines later, explicitly treats a zero total as a failed read: Probe::Read((total_mib, free_mib)) if total_mib > 0 => ... , Probe::Read(_) | Probe::Unreadable => Probe::Unreadable. The module's own doc comment states the invariant this is supposed to guarantee — "a GTT read that failed never gets here... so a Some pool always has a non-zero total" — but says nothing about VRAM ever being zero, and nothing stops it: vram_mib() parses TOTAL_VRAM:/CSV output straight into a u64 with no > 0 check either. Traced the path: if vram() returns Probe::Read((0, _)) (a wedged amd-smi/rocm-smi printing a parseable 0, the same driver-wedge class this PR explicitly designed the GTT zero-guard around) and gtt() returns Probe::Absent (no rocm-smi on that host), sample_device_memory builds DeviceMemory { vram: Pool { total_mib: 0, ... }, gtt: None }. select_drain_target then falls back to VRAM (_ => (PoolKind::Vram, memory.vram)), required_free_mib(0) == MAX_FREE_FLOOR_MIB.min(0 / 100 * 90) == 0, and DrainTarget::drained() (free_mib >= floor_mib) is unconditionally true. wait_for_free_vram reports "drained" on the very first sample and the caller proceeds immediately — the same false positive the GTT zero-guard exists to prevent, just on the other pool. There is a unit test for a zero GTT total (a_zero_gtt_total_is_a_failed_reading_not_a_pool) but no equivalent for a zero VRAM total, so nothing would catch a regression here either.
  Confidence 85 · logic · Fix: give the VRAM branch of sample_device_memory the same total_mib > 0 guard as the GTT branch (treat a zero VRAM total as Probe::Unreadable), and add a test mirroring a_zero_gtt_total_is_a_failed_reading_not_a_pool for the VRAM case.

Non-blocking

None

Decisions for the author

• bounded_stdout reimplements rocm_core::process::run_with_timeout's poll/kill/reap pattern instead of reusing it, and only kills the direct child (a grandchild holding the pipe can still block the reader thread) — tradeoff
  Verified: tests/e2e-cucumber/Cargo.toml lists rocm-core under [dev-dependencies] only, so device_memory.rs's library code cannot reach it without promoting that to a normal dependency — a crate-edge decision the author would own, not this module. The code's own doc comment already discloses the grandchild-pipe caveat and argues it's acceptable because amd-smi/rocm-smi don't fork long-lived helpers. Reasonable given the constraint; worth confirming no first-party dependency change was intended instead.
• CARVEOUT_MAX_MIB (8 GiB) mirrors the CI workflows' GPU_PREFLIGHT_MIN_FREE_GIB: 8 for the Strix Halo lanes by convention only, with no contract test tying them together — non-blocking-improvement
  xtask/src/workflow_contract.rs has tests pinning GPU_PREFLIGHT_MIN_FREE_GIB behavior for the bash/PowerShell/WSL preflight scripts (e.g. apu_preflight_gates_on_the_pool_the_engine_uses), but none of them reference CARVEOUT_MAX_MIB, so the two constants can drift silently if either side's threshold ever changes. Not broken today — a one-line contract test would close the gap.

Positive signals

• Probe<T>'s three-state design (Absent/Unreadable/Read) is a deliberate, well-tested alternative to a boolean-blind Option — it keeps "no tool," "tool present but broken," and "a real reading" from ever collapsing into each other, backed by an exhaustive combinator test over the or_else truth table.
• bounded_stdout starts its reader thread before the try_wait poll loop, correctly avoiding the classic pipe-full deadlock rocm-core's own docs warn about, and this is proven with a real external check (kill -0 on the recorded child pid after timeout) rather than merely asserting the function's return value.
• The Strix Halo unit-test fixtures use the real used/total byte pairs from an actual observed CI preflight log, grounding the pool-selection tests in real field data rather than arbitrary numbers.

Deployment notes

None — the change is confined to tests/e2e-cucumber/{src/device_memory.rs,src/lib.rs,tests/e2e/serving_steps.rs} (test-harness only; verified via the diff stat), with no production/CLI-facing code touched.

What this covered

Read completely: tests/e2e-cucumber/src/device_memory.rs (new, 683 lines), tests/e2e-cucumber/src/lib.rs (1-line diff), tests/e2e-cucumber/tests/e2e/serving_steps.rs (full file, 2542 lines, post-change) — all at head a3b2e16. Cross-referenced against .github/scripts/gpu-preflight.sh (the shell script this module's doc comments claim to mirror), crates/rocm-core/src/process.rs (run_with_timeout, the bounded-subprocess helper this module couldn't depend on), tests/e2e-cucumber/Cargo.toml (dependency placement), the root AGENTS.md, xtask/src/workflow_contract.rs, and .github/workflows/{nightly,e2e-selfhosted}.yml. Diffed e6d2f7c0…a3b2e16 (the real merge-base, not the task-supplied base sha — see note above).

Five independent fan-out passes ran: a code-correctness pass reading both changed Rust files in full, an agent-instruction-adherence pass against AGENTS.md, a history/blame pass over the touched regions and this PR's own 4-commit sequence, a combined prior-changes/code-comments pass, and a beyond-diff impact-analysis pass. The code-correctness pass's one surviving candidate (the VRAM zero-total asymmetry) was independently re-verified by hand-tracing vram_mib → sample_device_memory → select_drain_target → required_free_mib → drained against the current source before being scored. Design review (Step 7) considered whether the probe-failure taxonomy should be type-enforced rather than doc-enforced, whether reading GTT only via rocm-smi is future-proof, and whether the carveout threshold should be wired into the CI contract checker; none of these rose to a fundamental approach problem.

CI: all 25 checks for this commit are green or cleanly skipped (no failing required check).
Did not run: any real amd-smi/rocm-smi invocation or the GPU-dependent e2e lanes — no such tooling exists in this review environment, consistent with the PR author's own disclosure that the GPU lanes were not exercised locally either. Reasoned from the code and the existing unit tests instead.
Not reconciled against any discussion on this PR — the blind passes did not read the review threads, the existing CHANGES_REQUESTED review, or the author's replies to it.

---

🤖 Generated with Claude Code

fixed

[juhovainio]

Nothing here is a correctness blocker, but a few things should be settled before merge.

Behaviour

• A host with amd-smi/rocm-smi installed but no usable GPU reading (non-zero exit, unparseable output) used to return "no GPU tooling" immediately. It now waits the full deadline on every serve. Consider treating "no reading ever obtained after a few samples" as no tooling.
• The deadline is checked only between samples, and one sample can take up to 30 s (three 10 s probes), so the wait can overshoot to ~150 s while the message says 120s.
• On a carveout host with amd-smi but no rocm-smi, the GTT probe is absent, so the wait falls back to VRAM and the original 120 s stall persists with no hint in the message.

Messages and tests

• "NOT drained" is printed when the harness never obtained a reading; "could not read device memory" would be accurate.
• describe_target (the " GTT" label, floor text) and the reworded timeout line have no test. Per the repo's rule that a message is asserted together with its behaviour, consider moving describe_target next to DrainTarget in device_memory.rs and unit-testing it.

Checked and fine: the carveout boundary, zero totals treated as unreadable, probe precedence, the GTT floor vs the preflight reading, and the Windows/Unix subprocess coverage. CI is green.

[juhovainio]

Behaviour change: a host with amd-smi/rocm-smi installed but no usable GPU (non-zero exit, unparseable output) previously returned "no GPU tooling" immediately; now it waits the full deadline on every serve. Consider treating "never produced a single reading" after a few samples as no tooling.

[jussielo-amd]

Agreed, that cost two minutes on every serve for a broken tool. 5099c1e gives up after three samples in a row with no reading from a tool that has never read, and reports the device as unreadable instead of waiting out the deadline. A tool that has read before is still waited on, since its next sample may succeed. Tests cover the give-up, the threshold, and not giving up after a reading.

[juhovainio]

Overshoot and wording: one sample can take up to 30 s (three 10 s probes), so this deadline can be passed by ~30 s while the message says 120s. And for an unreadable reading, "NOT drained" is a claim the harness never observed.

[jussielo-amd]

Both fixed in 5099c1e. Each probe is now held to the time left before the deadline and the sleep is capped to it, so the wait ends at the two-minute mark instead of up to about 30 s later, and the line reports the elapsed seconds. "NOT drained" is now printed only for a reading below its floor; a sample nobody could read says "could not read device memory", with the last reading if there was one.

[juhovainio]

describe_target (the " GTT" label and floor text) and the reworded timeout line have no test. It is pure and could live next to DrainTarget in device_memory.rs with a unit test, per the message-asserts-with-behaviour rule.

[jussielo-amd]

Moved next to DrainTarget as describe, and the timeout and unreadable lines are now describe_verdict in device_memory.rs. The tests drive samples through the wait and assert the resulting line, and I checked they fail when the give-up rule, the unreadable-versus-undrained choice, or the carveout note is broken. The third point in the review body, a carveout host with amd-smi but no rocm-smi, now gets a note on the timeout line saying only the BIOS carveout was read and asking whether rocm-smi is installed.

[juhovainio]

Nothing here is a correctness blocker, but a few things should be settled before merge.

Behaviour

• A host with amd-smi/rocm-smi installed but no usable GPU reading (non-zero exit, unparseable output) used to return "no GPU tooling" immediately. It now waits the full deadline on every serve. Consider treating "no reading ever obtained after a few samples" as no tooling.
• The deadline is checked only between samples, and one sample can take up to 30 s (three 10 s probes), so the wait can overshoot to ~150 s while the message says 120s.
• On a carveout host with amd-smi but no rocm-smi, the GTT probe is absent, so the wait falls back to VRAM and the original 120 s stall persists with no hint in the message.

Messages and tests

• "NOT drained" is printed when the harness never obtained a reading; "could not read device memory" would be accurate.
• describe_target (the " GTT" label, floor text) and the reworded timeout line have no test. Per the repo's rule that a message is asserted together with its behaviour, consider moving describe_target next to DrainTarget in device_memory.rs and unit-testing it.

Checked and fine: the carveout boundary, zero totals treated as unreadable, probe precedence, the GTT floor vs the preflight reading, and the Windows/Unix subprocess coverage. CI is green.

[juhovainio]

Behaviour change: a host with amd-smi/rocm-smi installed but no usable GPU (non-zero exit, unparseable output) previously returned "no GPU tooling" immediately; now it waits the full deadline on every serve. Consider treating "never produced a single reading" after a few samples as no tooling.

[jussielo-amd]

Agreed, that cost two minutes on every serve for a broken tool. 5099c1e gives up after three samples in a row with no reading from a tool that has never read, and reports the device as unreadable instead of waiting out the deadline. A tool that has read before is still waited on, since its next sample may succeed. Tests cover the give-up, the threshold, and not giving up after a reading.

[juhovainio]

Overshoot and wording: one sample can take up to 30 s (three 10 s probes), so this deadline can be passed by ~30 s while the message says 120s. And for an unreadable reading, "NOT drained" is a claim the harness never observed.

[jussielo-amd]

Both fixed in 5099c1e. Each probe is now held to the time left before the deadline and the sleep is capped to it, so the wait ends at the two-minute mark instead of up to about 30 s later, and the line reports the elapsed seconds. "NOT drained" is now printed only for a reading below its floor; a sample nobody could read says "could not read device memory", with the last reading if there was one.

[juhovainio]

describe_target (the " GTT" label and floor text) and the reworded timeout line have no test. It is pure and could live next to DrainTarget in device_memory.rs with a unit test, per the message-asserts-with-behaviour rule.

[jussielo-amd]

Moved next to DrainTarget as describe, and the timeout and unreadable lines are now describe_verdict in device_memory.rs. The tests drive samples through the wait and assert the resulting line, and I checked they fail when the give-up rule, the unreadable-versus-undrained choice, or the carveout note is broken. The third point in the review body, a carveout host with amd-smi but no rocm-smi, now gets a note on the timeout line saying only the BIOS carveout was read and asking whether rocm-smi is installed.

Duplicate of an identical review submitted in error; see the other review.

[juhovainio]

Reviewed the current head. I found no outright correctness bugs, and the pool choice, per-sample decision and verdict wording are well covered by pure-function tests. The points below are worth addressing before merge:

1. GTT CSV headers are unverified. The Strix Halo fix hinges on rocm-smi --showmeminfo gtt --csv emitting GTT Total Memory / GTT Total Used Memory. gpu-preflight.sh only confirms those names for the non-CSV text output. If they are wrong, every sample on a carveout host is unreadable and the wait gives up after 3 samples. That fails safe, but the fix silently does nothing. Logging the raw header/first line on a parse failure would make the first lane run diagnosable (inline comments below).
2. Untested glue. The wait_for_free_vram loop is disclosed as untested, but the amd-smi TOTAL_VRAM:/FREE_VRAM: parser was also moved into a closure and now has no test at all. A small parse_amd_smi_vram(text) helper next to parse_rocm_smi_csv_pool would make it testable.

Minor / for awareness:

• bounded_stdout can leak a reader thread when a grandchild holds the pipe (documented).
• A host with amd-smi but no rocm-smi keeps the old never-drainable behaviour (documented and tested).
• The floor of 90% of the whole GTT pool leaves only ~10 GiB of headroom for other GTT consumers before the wait runs the full 2 minutes.
• Probe<(u64, u64)> is a bare tuple where Pool already exists, and device_memory_mib is a one-line delegate.

CI is green on everything that ran; there is no GPU lane, so real-hardware validation happens post-merge as the PR notes.

[juhovainio]

The header names this parser relies on (GTT Total Memory / GTT Total Used Memory) are only confirmed against the non-CSV text that gpu-preflight.sh parses, not against rocm-smi --showmeminfo gtt --csv. Consider including the first line of the output in the unreadable path so the first lane run can confirm or refute the headers.

[jussielo-amd]

Good point, the header names are only confirmed against the text output of the preflight script, not the CSV. A probe whose output the parser rejects now carries what the tool printed (61e80b4): the final line reads could not read device memory (...; rocm-smi --showmeminfo gtt --csv printed "<header row>"), bounded to 120 printable characters, so the first Strix Halo run confirms or refutes the names. Tests cover the hint, its limits, and its path to the final line.

[juhovainio]

Making an unreadable GTT read turn the whole sample unreadable is a reasonable choice, but if the CSV headers are wrong this is the path every Strix sample takes, and the wait gives up after 3 samples with only the generic message. A raw-output hint here would make that diagnosable.

[jussielo-amd]

Same fix. The hint now travels from the probe through the sample and the wait to the verdict, is kept across later plain failures, and is dropped once a reading arrives, so the generic message is no longer all a Strix run would show if the headers are wrong. A parsed zero total stays a plain unreadable sample, because the header was readable there.

[jussielo-amd]

Thanks for the review. In 61e80b4: the amd-smi parser is now parse_amd_smi_vram, next to the CSV parser, parses exactly as before and has tests; the probes return a named Pool instead of a bare tuple; and the one-line device_memory_mib is inlined. The GTT header diagnostics are covered in the inline replies. On the 90% GTT floor: it is the cap the wait already applied to VRAM, now applied to the GTT pool, so a host whose other consumers hold more than the remaining tenth costs time (the wait runs to its two-minute deadline) rather than a failure. On bounded_stdout: it kills the direct child and its doc comment records the grandchild caveat; rocm-core's helper is only a dev-dependency of this crate, so reusing it would be a crate-edge change that belongs in a separate PR.

fixed

[juhovainio]

Overall the approach is sound: one predicate (is_carveout) picks the pool, every smi call is bounded and reaped, an unreadable GTT on a carveout host is never judged by VRAM, and the xtask test ties the carveout limit to the lanes' preflight floor. Requesting changes mainly because the lane this targets is not yet demonstrated, plus a few small items.

Blocking

• Both "E2E tests (Strix Halo, Ubuntu, nightly/release, current)" jobs are red at "Run E2E tests on Strix Halo", and that is the exact lane this change is for. I could not read the logs while the run was in progress, so I can't tell a runner quirk from a regression. Please share the failure output (or a green rerun) before this merges; the expected 16-20 min saving is also not yet shown.

Non-blocking

• The GTT column names ("GTT Total Memory" / "GTT Total Used Memory") are unverified against real rocm-smi output, and the glue in serving_steps.rs is untested. rocm-smi may also print warning lines before the CSV header, which parse_rocm_smi_csv_pool would take as the header. Scanning for the first line containing the column name would be sturdier.
• Probe::parse is only used by its own unit test; production uses parse_text. Remove it or use it.
• A probe truncated by probe_budget near the deadline makes timed_out() report Unreadable and can replace an earlier NotDrained.
• The 90% floor on a ~100 GiB GTT pool tolerates ~10 GiB still held; fine as a tradeoff, worth a sentence in the doc comment.
• The new 10s kill on amd-smi is a behavior change on discrete lanes (an 8-GPU MI300X metric call can be slow); the rocm-smi fallback bounds the effect.
• The state machine is ~700 non-test lines with Absent/Unreadable/Unparsed repeated across three enums; consider whether it can be slimmed for a best-effort wait.
• No docs touched; please grep docs/ for the drain-wait wording (e.g. "still holding the GPU") to confirm nothing drifted.

[juhovainio]

Probe::parse is only used by its own unit test; production code uses parse_text. Remove it (and its test) or use it.

[jussielo-amd]

Removed in b3229c3. Probe::parse and its test are gone; production already used parse_text, and the remaining parser tests cover the cases it checked.

[juhovainio]

timed_out() reports Unreadable when the final sample was unread, even after a below-floor reading, so a probe cut short by probe_budget in the last seconds can replace NotDrained with Unreadable. Consider not counting budget-truncated samples.

[jussielo-amd]

Fixed in b3229c3. A sample that starts with less than a full probe timeout left and reads nothing is no longer counted, so the wait ends on the last real reading instead of replacing a below-floor reading with Unreadable. A truncated sample that does read still counts. The check is a small pure function with tests for both cases and for the boundary, and I confirmed they fail when the flag is ignored, when truncated readings are dropped and when the boundary is off by one.

[juhovainio]

These GTT column names are unverified against real rocm-smi output (the PR body says so). rocm-smi may also print warning lines before the CSV header, which parse_rocm_smi_csv_pool would treat as the header; scanning for the first line containing the column name would be more robust.

[jussielo-amd]

Fixed in fc588ec. The header is now the first line that names both columns, so warning lines before the CSV are skipped, with tests for a preamble and for a header with no data row. The comment on the GTT probe now states which part is verified and which is assumed: the plain-text labels GTT Total Memory and GTT Total Used Memory are known from gpu-preflight.sh, while the CSV header wording is assumed by analogy with the VRAM CSV.

[jussielo-amd]

On the blocking item (the two Strix Halo Ubuntu lanes are red), these are the facts from the logs of the latest run on this head, run 42059649876, release job 127719472341 and nightly job 127719472440.

Both lanes fail 8 of 289 scenarios, the same 8 as the run on main before this change (run 41836355803, jobs 126596324987 and 126596325108): bench-06, chat-06, chat-07, runtime-02, serve-01, serve-02, serve-07 and serve-40. runtime-02 is already fixed on main by #873. In each lane the first failure is a served-model step, endpoint http://127.0.0.1:11435/v1/models did not serve model Qwen3-0.6B after 1 attempt(s) of 90s, with Backend load failed: llama-server failed to start in the engine log.

The drain-wait runs before each of those serve attempts and reads drained: device state: drained (102362 MiB free of 102400 MiB GTT, floor 92160 MiB), all four times in each job. So the failures are independent of it. The run before this change printed five NOT drained after 120s (319 MiB free of 512 MiB, floor 450 MiB) lines, one 120 s wait each. The "Run E2E tests on Strix Halo" step took 37m57s and 38m38s there, against 21m13s and 23m21s on this head, about 15 to 17 minutes less. That is not a controlled comparison, since the commits differ and the same scenarios fail in both, and a green rerun is not possible while the serve failure stands, so the saving is shown by the drain lines but not demonstrated end to end.

The other review-body notes are handled in fc588ec and b3229c3: the 90% tolerance on the roughly 100 GiB pool (about 10 GiB can still be held) is documented in required_free_mib, and the amd-smi bound is documented (main ran amd-smi metric -m with no timeout, now it is cut off at 10 s or the time left, and the rocm-smi fallback is asked next with its own bound, so a sample costs at most two bounds). I left the state machine as it is: its non-test region is 613 lines, 359 of them code, and the only repeated shape is Judgement, which duplicates the four variants of Probe; collapsing it would save about 25 lines but touch about 34 uses with no behaviour change. I grepped docs/ and found nothing that describes the serve drain-wait, so nothing drifted. Separately and unchanged here, docs/testing.md and the workflow comments call the Strix GTT pool about 62 GiB while both lane logs read 107374182400 bytes (100 GiB).