[tomastola]

Summary

The GPU reclaim step that runs before every self-hosted E2E lane could not see a leaked llama-server, so the leak held the card until the runner itself was replaced. On the R9700 lane that meant 16 consecutive jobs failing GPU preflight over 2h20m, every one reporting the same only 8 GiB free (< 16 GiB).

Root cause. The reclaim matched a leaked engine two ways only — anchored to a scenario temp dir (/tmp/rocm-e2e) or to the shared cache (e2e-shared):

pkill -f '/tmp/rocm-e2e.*llama-server'
pkill -f 'e2e-shared.*llama-server'

A managed serve runs the engine binary out of the shared pre-warm runtime tree — $RUNNER_WORKSPACE/e2e-prewarm-*/data/runtimes/wheel/.../engines/lemonade/runtime/bin/llamacpp/.../llama-server — which is neither. The lane exports E2E_SHARED_CACHE_DIR and E2E_SHARED_RUNTIMES_DIR as two different directories; the patterns were written against the cache, the engines live under the pre-warm.

Both patterns were also order-sensitive. pkill -f matches the whole command line and argv[0] is the binary path, so a /tmp/rocm-e2e-* model argument later in the line could not rescue them.

This is expensive rather than untidy because of how the container lanes recycle: the pod is usually fresh per job but the work volume is not, and two jobs can share one pod's life — so one unmatched leak fails every job scheduled to that runner until the pod is recreated.

What changed.

• scripts/reclaim-gpu.sh replaces the inline pkill list everywhere one existed — eight bash call sites, alongside the two PowerShell mirrors below. (Ten before this branch was rebased onto main; see the scope note.) A process is reclaimed when its command line names an E2E-owned root and an engine marker, in any order. Requiring both halves is what spares a manual /workload serve on a shared runner in the ordinary case — it names no E2E root, which is the property the old NOTE comment was protecting by hand. That is a narrowing, not a guarantee: the roots are unanchored substrings of the whole command line, so a manual serve IS selected when an argument happens to name one (--model-path /home/dev/e2e-shared-models/x.gguf, measured). The script's header documents that exposure, and the seven step comments now say the same thing rather than promising a serve is "never touched".
• TERM, then SIGKILL for anything still alive after a grace period, and it reports each process it acts on. The previous step printed an unconditional reclaimed whether or not it did anything, which is a large part of why this went unnoticed for hours.
• Both signals re-verify that the pid still carries the command line it was selected with. select_leaked walks the whole of /proc, so a pid can be freed and reissued in between; a bystander is just as dead from TERM as from KILL, so the check cannot be reserved for the escalation.
• cmdline_of flattens NEWLINE along with the NUL separators the kernel inserts. select_leaked emits pid<TAB>cmdline<NEWLINE> and reclaim reads it back with IFS=$'\t' read -r, and a newline inside an argv element is content, not a separator — --chat-template and --prompt carry them. Left intact, one such process emits a record that splits in two: the genuine pid arrives with a truncated command line, fails the identity check against its own full one, and is passed over as "recycled", so the leak survives TERM and KILL alike while the run signs off with 0 process(es) signalled. Fixed at the single point every consumer already reads through rather than at each of them.
• The TAB is deliberately not flattened, and the distinction is measured rather than reasoned. read hands leftover words and their intervening separators to the last name, and the trailing space left by the final NUL keeps an embedded tab from ever being stripped as a trailing one — so a tab lands in cmdline verbatim and breaks nothing. An earlier revision of this PR claimed both bytes were flattened; deleting the tab from the flattening set left every check green, which is what exposed the claim as coverage no mutation could falsify. The tab fixture now guards the record format, not a flattening step. The PowerShell mirrors pass CIM objects straight to Stop-Process and never serialise a record, so they are unaffected either way.
• report_holders derives its engine filter from ENGINE_MARKERS instead of repeating the alternation. A second copy of that list is the drift the PowerShell mirrors demonstrated; deriving it removes the copy rather than guarding it. -F, not an -E alternation, so the markers keep the literal-substring meaning they have in the rule.
• Native Windows has no bash, so its two steps mirror the same rule in PowerShell. Their existing matcher was already order-independent and only needed the pre-warm root. Their summary line says signalled too: Stop-Process -ErrorAction SilentlyContinue swallows a failure exactly as kill ... 2>/dev/null || true does, and the Windows count is weaker still — it is $leaked.Count, the selected set, with no identity re-verification, so a process that survived is counted.
• On preflight failure the lanes now call --report-holders, printing per-process VRAM (rocm-smi --showpids) and the engine processes present. Reconstructing that by hand is what made the original incident slow to diagnose.

Test plan

--self-test runs on the GitHub-hosted lane alongside the existing script self-tests, so the matching rule is gated without GPU hardware. That step sits behind the same heavy path filter as its neighbours there, which any change under scripts/ reaches — so it always runs when this script itself changes, though a docs-only PR skips it. It spawns decoys whose argv[0] is a real-shaped path, plus a zombie and an out-of-scope bystander, and asserts across thirteen checks:

1. the superseded patterns do not match a pre-warm engine process — the defect itself, asked of the decoys' own command lines so the check cannot be answered by an unrelated process on the machine;
2. the new rule selects it;
3. the record format survives a command line carrying both delimiter bytes — a decoy whose argv holds a real newline and a real tab, asserted to carry both so the check cannot pass having examined a string that could not break it. The two are not symmetric and the check says so: the newline arm pins cmdline_of's flattening, the tab arm pins the record shape that makes flattening the tab unnecessary;
4. a /workload manual serve (engine marker, no E2E root) is not selected;
5. an E2E-owned path with no engine marker — the suite's own binary under CARGO_TARGET_DIR — is not selected;
6. a SIGTERM-ignoring engine at a pre-warm-shaped path is selected;
7. containment: a bystander that matches the rule in full but lives outside SELFTEST_SCOPE is not selected, and a breach refuses to run the real reclaim at all;
8. the escalation guard reports same / recycled / gone, with gone driven by both routes into it — a failed /proc open and an empty read from a real zombie;
9. the recycling guard's two call sites: the comparison is overridden to force a "recycled" verdict through a real reclaim, which must then signal nothing — deleting either call site kills decoys here;
10. end to end, reclaim kills all three leaks — including the one whose command line carries the delimiters — and spares all three bystanders;
11. escalation to SIGKILL was reported for the stubborn decoy and not for the ordinary one — which is what pins the kill -TERM preceding it;
12. report_holders lists a process carrying an engine marker, leaves out an E2E-owned one carrying none, and matches markers as literal substrings — the last driven by an override marker that only an -E alternation would match, since no real marker holds a metacharacter;
13. the bare invocation's rm -rf root: a relative or empty TMPDIR falls back to /tmp, an absolute one is honoured with or without a trailing slash. Asserted on the resolver rather than by running the deletion, which would clear the host's real scenario dirs.

Every check is mutation-verified — an assertion that cannot fail is worse than no assertion, and two rounds of review here found exactly that:

mutation	self-test
remove e2e-prewarm from E2E_ROOTS (the pre-fix behaviour)	fails 2, 9
remove any single root or marker from either list	fails, naming that entry
gut the has_engine half of the AND rule	fails 5
delete cmdline_of's emptiness check	fails 8 (zombie arm)
delete the SELFTEST_SCOPE filter from select_leaked	fails 7
make the bystander stop matching the rule, or not run	fails 7, by reason
make a probed decoy unreadable	fails 1 (rather than passing having probed nothing)
delete either pid-recycling guard call site	fails 9
empty selection	fails 7, by reason, without a false containment cause
delete the SIGKILL escalation block	fails 10, 11
replace kill -TERM with a no-op	fails 11
revert cmdline_of's NEWLINE flattening (the defect)	fails 3, naming the malformed record before containment misattributes it
drop the TAB from the flattening set	passes — which is why the tab is not claimed as flattened; the check pins the record shape instead
give the delimiter decoy a command line missing the tab, the newline, or both	fails 3, naming the absent byte
stop the delimiter decoy being selected	fails 3, 10
make the delimiter decoy unreadable	fails 3, naming a dead fixture rather than a malformed one
empty ENGINE_MARKERS	fails the list assertion, which is what keeps report_holders' derived filter from being handed no patterns

A new xtask contract test pins the bash E2E_ROOTS to both PowerShell mirrors, so a root added to the script cannot silently miss the Windows lanes. It extracts the -match alternation rather than substring-matching the workflow: the whole-file form false-passes, because the roots also appear there as env vars (caught by mutating it). The __engine-serve-http exemption is itself asserted, so it fails — demanding its own deletion — once the tracked divergence is fixed, rather than rotting into dead code.

Its parser is hardened after review found it mis-attributing legal reformatting as mirror drift. It located the array's closing ) in raw text before stripping comments, and accepted single quotes only, one root per physical line — so an inline comment containing a ) truncated the array and the test then demanded the deletion of e2e-prewarm from both PowerShell mirrors, the root this PR exists to add. Comments now come off first, both quotings are accepted, a parsed root may not contain its own delimiter (which rejects the whole array folded onto one line), and every non-empty line inside the body must parse into exactly one root — so an unrecognised spelling fails as a parser that did not understand the line, not as fabricated drift. Deliberately not tokenised on whitespace: that would split a future root containing a space into two bogus roots, the same defect class one level down. Five mutations re-run — the three spellings, plus a root dropped from a mirror and a bogus token added to one — each now either parses or fails with the correct cause.

The parsing half is now a pure parse_e2e_roots(text, origin), which is what makes any of this testable: every spelling above was found by editing the tracked script by hand, and none of them could have failed CI afterwards — the same "covered but not exercised" gap that check 13's call-site assertion closes on the bash side. Three tests pin it: the legal respellings that must parse identically, the spellings that must be rejected as the parser's own misunderstanding rather than as mirror drift (each asserting the message does not send a reader to delete a root from Windows), and the empty array. Anchoring happens after comment-stripping so a header comment quoting the literal E2E_ROOTS=( cannot redirect the parse into prose. Mutation-verified: dropping the own-delimiter guard, silently skipping an unparsable line, and anchoring before decommenting each red the test that covers them.

cargo test -p xtask 159 passed; prek run clean including shellcheck, apart from license-headers — hawkeye is not installable in this container, and this change adds no new files, so it introduces no header.

Hardware verification. All six GPU lanes — MI300X, MI350P, rad3 R9700, and Strix Halo on Ubuntu, Windows and WSL2 — pass at the current head 2a665ae1 (2026-09-30). That supersedes the older 89a64a39 evidence this section used to lean on, and the caveats below are kept only because they explain how the branch got here.

Both Strix lanes first came back red on this head and are not a defect in this change. Both were cancelled at their 45-minute timeout-minutes cap (which gh pr checks renders as "fail"), and both passed on re-run at identical content, in 12m32s and 38m21s. The two causes are different and both sit upstream of anything this PR touches — the reclaim step completed cleanly in each, ~25 and ~40 minutes before the respective cancels:

• Ubuntu: dead upstream download URLs. 10× HTTP 404 on lemonade's llama-b9752-bin-ubuntu-rocm-7.14-x64.tar.gz and 10× HTTP 403 on repo.amd.com/.../therock-dist-linux-gfx1151-7.14.1.tar.gz, still retrying 39.5 minutes later when the cap hit during serve-08. Repo-wide — the same two URLs took out this lane on ci(e2e): add a W7900 self-hosted E2E lane (EAI-8763) #454 the same day across ~20 branches. The passing re-run hit the 404 only twice in one second and the 403 not at all.
• Windows: a transient serve-02 step failure (no 'resolved model' in output) followed by ~17 minutes of zero log output before the cap — the unbounded-teardown shape fix(core,ci): bound local-service HTTP reads; raise strix-ubuntu E2E job timeout #348 describes. No HTTP errors.

For anyone else who hits this: the Ubuntu lane finishes only 6m39s inside its 45-minute cap even on a clean run, so it has little slack to absorb either of the above.

The older caveats, retained:

• Most review commits since then touch only the self-test and the xtask contract test. The commit adding the delimiter fix is the exception — it changes two production paths (cmdline_of's flattening, which every consumer reads through, and report_holders' filter), so it is not a test-only delta.
• The latest two rounds (96f65917, 2a665ae1) are comments, the self-test and the contract test, with one production change that is a string: the summary line now reads "signalled" rather than "terminated", in the bash script and both PowerShell mirrors. Nothing in the matching, signalling or escalation paths moved, so neither round can change what the GPU lanes do.
• The branch has since been rebased onto main, which rewrote every SHA and moved the two WSL lanes to a different pool. The WSL2 half of that original six-lane result therefore no longer describes a lane this branch runs.
• On the pre-rebase head, MI300X, MI350P, rad3 R9700 and Strix Halo/Windows passed; Strix Halo/WSL2 was failed deliberately by the pool operators for requesting retired labels (the reason for the rebase), and Strix Halo/Ubuntu failed its GPU preflight at 0 GiB free with rocm-smi reporting no KFD PIDs running at all — nothing holding the card, so that is the preflight's VRAM read rather than a leak this script could reclaim. It failed the same way on the previous head too, before any of this round's changes existed. That read has since been confirmed and fixed on main by ci(e2e): gate the Strix Halo GPU preflight on GTT when VRAM is a carveout #442, which gates the APU preflight on GTT when VRAM is a carveout; the lane passes at the current head. Read the lanes there.

Scope note. The nightly Linux Strix lane hides its reclaim inside a step named Prepare writable dirs on the nvme + reclaim GPU from stray E2E procs — same defective patterns, easy to miss by grepping step names, and included.

The two WSL lanes no longer carry a reclaim step at all. #403 rebuilt them on the DevLab Dispatch pool: they run on a Windows host, provision WSL2 fresh per job and unregister the distro afterwards, so no prior run's processes can survive into them and their GPU preflight is advisory. Rebasing onto that dropped those two sites rather than porting a step whose premise no longer holds — and run: bash … would have been wrong on a Windows runner besides. docs/testing.md states this.

No Gherkin scenario: this is CI plumbing with no user-observable CLI behaviour change (AGENTS.md §3). The behaviour it does change is CI's own, which the self-test covers.

Risk: low. The matching rule strictly widens what reclaim catches; the one narrowing risk — killing a bystander — is pinned by self-test checks 4, 5 and 7, by check 10's three survival assertions, by the "must signal nothing" assertions in checks 8 and 9, and by the identity re-verification before both signals — itself pinned at both call sites by check 9. Checks 4 and 5 are negative assertions, so they now report SKIP rather than ok when the selection is empty, instead of affirming on a run that examined nothing.

Known gaps, stated rather than implied:

• The self/parent-pid exclusion in select_leaked and --dry-run have no self-test coverage. report_holders and the bare invocation's rm -rf root were in this list until checks 12 and 13 closed them; check 13 now also asserts that the rm -rf statement still calls scenario_tmp_root, since a covered helper nothing is obliged to use is not coverage — reverting that one line to an inline ${TMPDIR:-/tmp} reds the suite. Each new assertion was mutation-tested.
• The summary line counts processes signalled, not terminated, and now says so. The count is incremented where TERM is sent, both kill calls swallow their errors, and the SIGKILL loop never touches it — so a signal this process was not permitted to send stays invisible in the total. The per-process lines carry the detail; the counter itself still has no self-test coverage.
• Flattening the newline makes the identity comparison marginally coarser: two command lines differing only in that byte now compare equal, where before they differed. Both sides come through cmdline_of, so they are flattened alike, and the alternative is the silent miss above.
• The Windows mirrors' engine half cannot match rocm.exe daemon, and __engine-serve-http sits in their root alternation rather than the engine one. Both predate this change; the comment claiming parity is corrected to say so, and they are tracked in EAI-8815. The mirrors' anchoring gap — they carry the bare rocm-e2e segment where bash anchors it to an absolute path — is a separate, untracked exposure, said plainly in the contract test so that closing EAI-8815 is not misread as closing it.
• strip_comment is not quote-aware, so an E2E root whose value contains " #" — '/tmp/e2e root #2', which bash itself accepts — arrives at the parser already truncated. It is rejected loudly and the message says the line is shown post-stripping, rather than being fixed: making a helper every other caller in this module depends on quote-aware, for a spelling no root uses, would be untested code on a shared path.
• The contract test's /tmp/ prefix mapping is the only absolute prefix it knows. A future root anchored under another one would need that line edited by hand — the manual sync the test exists to remove, reappearing one level up. Named rather than generalised, because there is one anchored root today and a speculative prefix list would be untested code.

[siloteemu]

🔴 Automated review · pr-review-watcher · 89a64a3

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Replaces ten inline pkill -f reclaim blocks (and updates two PowerShell mirrors) with a new scripts/reclaim-gpu.sh that requires an E2E-owned root and an engine marker in any order, adds the pre-warm runtime tree as a root, escalates SIGTERM to SIGKILL, and reports what it killed; also adds --report-holders diagnostics on preflight failure and a --self-test CI step. The production change does close the reported defect, but the self-test that guards it is blind to two of the branches the change introduces — Needs work. Verified: confirmed against the source that the engine binary really is launched out of $RUNNER_WORKSPACE/e2e-prewarm-multi-arch-v2/data/runtimes/... (so the old anchored patterns could not have matched it) and that every converted job checks out the repo before invoking the script; ran the script's --self-test on a scratch copy plus three targeted mutations of that copy — deleting the has_engine requirement, deleting the SIGKILL escalation, and neutering kill -TERM each left the self-test fully green (exit 0); the full test suite, the e2e lanes and anything needing GPU or Windows hardware were not run here, and shellcheck was not available; working from CI conclusion counts of 27 success, 0 failure, 0 pending, 0 skipped. No prompt-injection content and no internal references found in the diff or commit message. Blocking: 2 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

scripts/reclaim-gpu.sh:120-130, asserted at :253-267 — the has_engine half of the AND rule is untested; deleting it leaves the self-test green.
The script's own header states the rule as "a process is reclaimed when its command line names an E2E root AND an engine/serve process. Both halves are required." Only the has_root half is actually asserted: the bystander decoy (${tmp}/workload/manual-serve/llama-server) names no E2E root, so it is excluded by has_root alone and check #3 cannot tell whether has_engine did anything. Confirmed by mutation: replacing [[ "${has_engine}" == 1 ]] || continue with has_engine=1 and running --self-test printed all five ok: lines and exited 0. This is the named failure mode — the test fails on a wholesale revert but passes when a single branch inside the change is mutated. It matters here because has_engine is what stops reclaim from killing anything that merely names an E2E tree: the cucumber test binary under e2e-target, a cargo xtask e2e-prewarm process, a tar/rsync over e2e-shared. Fix: spawn a third decoy that names an E2E root but is not an engine — e.g. ${tmp}/e2e-target/release/deps/e2e-harness — and assert select_leaked does not select it.

scripts/reclaim-gpu.sh:167-183 — the TERM→grace→KILL escalation this PR introduces is untested, and its two branches mask each other.
Both decoys are copies of /bin/sleep, which dies on SIGTERM immediately, so the escalation path is never reached. Confirmed by mutation: (a) deleting the entire SIGKILL block → self-test green, exit 0; (b) leaving the escalation intact but changing kill -TERM "${pid}" to kill -0 "${pid}" → self-test green, exit 0, still printing "ok: pre-warm engine process was reclaimed". Each branch silently covers for the other, so either can regress unnoticed — and this is the branch whose entire purpose is freeing VRAM held by a process that ignores SIGTERM, which is the harm the PR exists to prevent. Fix: add one decoy at the same path shape that ignores SIGTERM (a small script containing trap '' TERM; sleep 300, so the path still supplies both markers), set RECLAIM_TERM_GRACE_SECS low for the test, and assert it survives the grace period and is then killed.

Non-blocking

• scripts/reclaim-gpu.sh:177-183 — the SIGKILL loop iterates the pre-TERM snapshot and only checks liveness, so a PID recycled during the grace window could be force-killed; re-read /proc/<pid>/cmdline and compare against the captured line before escalating.
• .github/workflows/e2e-selfhosted.yml:559-567 and .github/workflows/nightly.yml:765-773 — the new "Same rule as scripts/reclaim-gpu.sh" comment overstates parity: rocm daemon can never match on Windows (rocm.exe daemon satisfies neither regex, because the engine half needs a literal rocm with a trailing space) and __engine-serve-http sits in the root half rather than the engine half; the regexes are pre-existing, the parity claim is new and will mislead the next reader.
• xtask/src/workflow_contract.rs — the repo already greps workflow text in contract tests, yet nothing pins the two PowerShell mirrors to the bash E2E_ROOTS; a test asserting each root appears in both regexes would prevent exactly the silent drift the PR's own comment warns about, which is the shape of the defect being fixed.
• scripts/reclaim-gpu.sh:98 — tr '\0' ' ' <"${cmdline_file}" 2>/dev/null sets up the input redirection before the stderr redirection, so the expected exit-between-glob-and-read race still prints a "No such file or directory" line (observed in a live run); swapping the redirection order silences it.
• scripts/reclaim-gpu.sh:88-134 — matching is an unanchored substring search over the whole command line, so a bystander such as tee /tmp/rocm-e2e-x/llama-server.log satisfies both halves from its log path alone; conversely a ComfyUI server (python main.py) has no marker, latent today since no scenario starts one. Anchoring the engine check to the /proc/<pid>/comm basename would close both.

[tomastola]

Both blocking findings were correct. I reproduced all three mutations before changing anything — has_engine gutted, SIGKILL block deleted, and kill -TERM neutered each left the self-test green at 89a64a39. The test only ever failed on a wholesale revert of the roots list, so the PR description's claim that it "fails before the fix and passes after" was true for one branch and false for the two this PR adds. Fixed in 4179db1.

Blocking 1 — has_engine untested. Added a fourth decoy at ${tmp}/e2e-target/release/deps/e2e-harness: an E2E-owned root with no engine marker, asserted both not-selected and surviving reclaim. Your reasoning about why it matters is the part I'd underweighted — e2e-target is CARGO_TARGET_DIR, so without has_engine reclaim selects the suite's own running test binary.

Blocking 2 — escalation untested. Added a decoy that ignores SIGTERM (trap '' TERM) at a pre-warm-shaped path, with TERM_GRACE_SECS dropped to 2 for the test. Asserting only "the stubborn decoy dies" would not have been enough — that still passes with kill -TERM replaced by a no-op, since everything then reaches SIGKILL. So the test also asserts the ordinary decoy did not reach escalation, which is what pins the TERM. Post-fix: gutting has_engine fails 2 checks, deleting the SIGKILL block fails 2, neutering kill -TERM fails 1.

Non-blocking, all four taken:

• PID recycling — the SIGKILL loop now re-reads /proc/<pid>/cmdline and skips escalation if it changed. Not killing a real holder is recoverable; killing a bystander is not.
• Parity comment — corrected rather than defended. You're right on both counts: rocm cannot match rocm.exe daemon, and __engine-serve-http is in the root alternation. The comment now names both divergences instead of asserting equivalence. The regexes themselves are pre-existing and changing Windows matching felt out of scope here, so they're tracked separately.
• Contract test — added, pinning the bash E2E_ROOTS to both PowerShell mirrors. Worth noting my first version was a false pass: it used text.contains(root) against the whole workflow, which stays green with the root deleted from the alternation, because the roots also appear as env vars and in prose. Caught by mutating it. It now extracts the -match alternation and fails in both directions.
• tr redirection order — confirmed and swapped.

One I'm not taking as proposed: anchoring the engine check to the /proc/<pid>/comm basename. The bystander you describe is real (tee /tmp/rocm-e2e-x/llama-server.log does satisfy both halves), but comm breaks three of the four markers: it cannot contain a space, so rocm daemon never matches; __engine-serve-http is an argument rather than a binary name; and vllm is a console script, so comm is the interpreter. That reintroduces exactly the miss class this PR exists to fix, in exchange for a narrower one. Happy to take a different formulation if you have one — otherwise I'd rather leave the known-wide rule than trade it for a known-narrow one.

Both counts in this objection are resolved at 4179db1, verified against the tree rather than from the commit message. The untested half of the selection rule now has a decoy of its own — a path that names an E2E root but carries no engine marker — and deleting that half of the rule makes the self-test select it and fail. The escalation path's two branches are now covered separately by a decoy that ignores the terminate signal, so neither branch can mask the other: removing the forced-kill step leaves that decoy alive and fails, and neutering the terminate step lets the ordinary decoy reach escalation and fails a different assertion. I also confirmed the self-test cannot pass vacuously on an empty selection, and that a failure actually propagates out of the step. Four of the five earlier non-blocking items were addressed too. Withdrawing this change request; a separate non-gating review records what remains, none of it blocking.

[siloteemu]

🔴 Automated review · pr-review-watcher · 4179db1

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

The follow-up commit adds the two missing decoys (an E2E-owned path with no engine marker, and a SIGTERM-ignoring engine at a pre-warm-shaped path), asserts escalation happened for the stubborn one and not for the ordinary one, adds a recycled-PID guard before SIGKILL, replaces the overstated PowerShell "same rule" comment with an accurate one, and pins the bash root list to both PowerShell mirrors with a new contract test — No blocking findings. Verified: read the whole 404-line script and the whole new test rather than the hunks, because the delta's entire claim is about which branches the self-test can fail on; re-derived each prior count against the current tree by reading the matching and escalation logic line by line, and confirmed the bash semantics the new recycled-PID guard depends on with standalone snippets (a shebang-bash decoy's /proc cmdline is bash <path> with a trailing space, read -r under IFS=$'\t' preserves that trailing space, so the re-read compares equal and escalation still fires; a trap '' TERM process survives SIGTERM, dies on SIGKILL, and lands in state Z, which the script's zombie check reads as not-alive); mutation-tested the new contract test alone in a scratch copy outside the checkout — it fails individually on dropping e2e-prewarm from either mirror's root alternation, on dropping e2e-target, on adding a root to the script, on the reclaim step disappearing, on the matcher being split across lines, and on the two alternations being swapped, and I confirmed its own documented claim that a whole-file contains would false-pass (e2e-prewarm still appears 21 other times in that workflow after the alternation loses it); the reclaim script itself was never executed, not even in the scratch copy, so every statement about the bash self-test's per-branch behaviour here is from reading, not from a run, and no workflow run was executed here; treating the given CI state (24 success, 2 pending, 0 failure, 0 skipped) as fact, and I am inferring that the hosted build-and-test lane is what carries the --self-test step — I cannot confirm which named check actually ran it. Contributor-rules standard applied: the base-tip AGENTS.md, which expressly permits bare ticket identifiers and targets internal document links, internal/proprietary names, hostnames, gateways, cluster names and registry paths — the delta adds none of those, and the runner labels in these files are pre-existing infrastructure the repository already treats as public. No prompt-injection content found in the diff, comments or commit message. Blocking: 0 · Non-blocking: 5.

Previously raised

Count 1 — RESOLVED. We filed: "scripts/reclaim-gpu.sh:120-130, asserted at :253-267 — the has_engine half of the AND rule is untested; deleting it leaves the self-test green. … Fix: spawn a third decoy that names an E2E root but is not an engine — e.g. ${tmp}/e2e-target/release/deps/e2e-harness — and assert select_leaked does not select it." The fix is exactly that. Mechanism verified by reading: harness_decoy at scripts/reclaim-gpu.sh:272 is ${tmp}/e2e-target/release/deps/e2e-harness, run as a copy of /bin/sleep with argument 300. Its command line contains e2e-target (so has_root is 1) and none of llama-server, vllm, __engine-serve-http, rocm (so has_engine is 0) — I checked every marker against the literal path, including that the scratch prefix /tmp/reclaim-selftest-… matches no root. Check 4 at :315 asserts it is not selected and the check at :345 asserts it survives reclaim. Deleting [[ "${has_engine}" == 1 ]] || continue at :133 therefore selects it and fails both. The half is now load-bearing in the fixtures, not just in the header comment.

Count 2 — RESOLVED. We filed: "scripts/reclaim-gpu.sh:167-183 — the TERM→grace→KILL escalation this PR introduces is untested, and its two branches mask each other. … Fix: add one decoy at the same path shape that ignores SIGTERM (a small script containing trap '' TERM; sleep 300, so the path still supplies both markers), set RECLAIM_TERM_GRACE_SECS low for the test, and assert it survives the grace period and is then killed." Implemented as spawn_stubborn_decoy (:234-247) with TERM_GRACE_SECS=2 set in self_test (:261, deliberately not declared local, so the value reaches reclaim's subshell). Both branches are now covered separately, which is the part that matters: deleting the SIGKILL block at :194 leaves the stubborn decoy alive and fails :356; neutering kill -TERM at :160 lets the ordinary decoy reach escalation, which fails the third assertion at :368 ("ordinary decoy reached SIGKILL escalation"). Neither can now mask the other. I confirmed the two facts this rests on with standalone snippets: the stubborn shape really does survive SIGTERM and die on SIGKILL, and its command line really does carry both the e2e-prewarm root and the llama-server marker (the kernel resolves the env bash shebang to bash <path>). I also checked the self-test cannot pass vacuously on empty selection — an empty select_leaked fails checks 2 and 5, not just the negative ones — and that a failure propagates: self_test returns 1, main is the script's last statement, and the CI step invoking --self-test carries no || true or continue-on-error.

Prior non-blocking items (5). Recycled-PID escalation — addressed at :183-192. Overstated "Same rule as scripts/reclaim-gpu.sh" parity comment — replaced in both workflows with a comment that is factually correct (I verified rocm genuinely cannot match rocm.exe daemon, and that __engine-serve-http genuinely sits in the root alternation). Missing contract test pinning the roots to the PowerShell mirrors — added and mutation-verified. Redirection order on the /proc read — fixed at :101. Unanchored substring matching — not addressed; still stands, repeated below.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• .github/workflows/e2e-selfhosted.yml:567-570 and .github/workflows/nightly.yml:773-776 — the new comment lists the two Windows divergences neutrally as "diverges", but they fail in opposite directions: rocm vs rocm.exe daemon is a miss, while __engine-serve-http sitting in the root alternation is an over-match — on Windows a non-E2E managed serve satisfies the root half from that marker alone and can be killed. Worth saying which is which, since only one risks a bystander.
• scripts/reclaim-gpu.sh:183-192 — the recycled-PID guard is new production code in the kill path with no branch of its own in the self-test: deleting it entirely leaves the self-test green (mutating it to always-recycle does fail, via the stubborn decoy). Given the rest of this commit, it is worth either a fixture that exercises the "recycled" path or a comment stating plainly that this branch is guarded only in the always-skip direction.
• scripts/reclaim-gpu.sh:188-191 — a process that simply exits between process_alive and the cmdline re-read yields an empty current, so the log says "was recycled during the grace period, not escalating" for something that was not recycled at all. Distinguish empty-cmdline (gone or zombie) from a genuinely different command line.
• scripts/reclaim-gpu.sh:38-52 — coverage is per-item thin on both lists: removing e2e-shared, /tmp/rocm-e2e or e2e-target from E2E_ROOTS, or vllm, __engine-serve-http or rocm daemon from ENGINE_MARKERS, leaves the self-test green, and the new contract test is one-way by construction (it catches a root added to the script and missing from a mirror, not one removed — confirmed by mutation). Only e2e-prewarm and llama-server are actually exercised.
• scripts/reclaim-gpu.sh:88-134 — carried forward and still standing: matching is an unanchored substring search over the whole command line, so a bystander such as tee /tmp/rocm-e2e-x/llama-server.log satisfies both halves from its log path alone, while a ComfyUI server (python main.py) carries no marker. Anchoring the engine check to the /proc/<pid>/comm basename would close both. Relatedly, self_test performs real kills confined only by SELFTEST_SCOPE, with no assertion that selection stayed inside the scratch tree — low blast radius today only because that step runs on a hosted ephemeral lane rather than the GPU pool.

[tomastola]

Four of the five non-blocking items are addressed in 79e3e67; the fifth is the carried-forward comm one, still declined for the reason given above.

Per-item coverage. Confirmed all six of your mutations first — dropping e2e-shared, /tmp/rocm-e2e or e2e-target, or vllm, __engine-serve-http or rocm daemon, each left the self-test green. The matching rule is now split out of select_leaked into cmdline_matches_rule, so the self-test drives each entry directly instead of needing a process per list item, and both lists are asserted against a written-out expectation. That duplication is deliberate: a loop over E2E_ROOTS cannot notice a root deleted from E2E_ROOTS, which is the exact drift at issue. All six now fail, each naming the specific entry.

Contract test, missing direction. You were right that it was one-way by construction. It now also rejects a root left behind in a mirror after being removed from the script, and a stray token added to a mirror — both mutation-verified. __engine-serve-http is skipped by an explicit arm naming EAI-8815, so fixing that ticket makes the assertion hold rather than ambushing whoever does it.

Exited vs recycled. Real mislabel in the code I added last round — an empty read took the "was recycled" branch. same_selected_process now returns 0 same / 1 recycled / 2 gone, the caller reports each separately, and all three arms are asserted.

Scratch-tree containment. Added, and worth flagging that you found it: self_test issues real kills and SELFTEST_SCOPE was the only thing confining them, with nothing checking that held.

Windows comment. Now says which is which — rocm vs rocm.exe daemon is a miss, __engine-serve-http in the root alternation is an over-match, and only the second can cost a bystander.

One thing I did not close, deliberately. The recycled-pid guard's call site is still covered only in the always-escalate direction — deleting the call site passes the self-test, and I verified that after the refactor too. Making a pid be reused by a different process on demand isn't reproducible in a test. The comparison itself now has all three directions asserted, and the gap is written into a comment at the assertion rather than left implied by a green run. If you have a way to drive real pid reuse deterministically I'll take it.

Earlier mutations re-checked after the refactor: gutting has_engine, deleting the SIGKILL arm and neutering kill -TERM all still fail. shellcheck --enable=all --exclude=SC2310,SC2312 clean, cargo test -p xtask 138 passed, prek clean.

[siloteemu]

🔴 Automated review · pr-review-watcher · 79e3e67

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

The PR replaces inline pkill -f GPU reclaim with scripts/reclaim-gpu.sh plus a workflow-contract test; the new commit answers the previous round by asserting every E2E root and every engine marker individually, splitting the escalation guard into same/recycled/gone, adding a scratch-tree containment check, making the PowerShell mirror test two-way, and correcting the Windows divergence comment — no blocking findings. Verified: ran the reclaim self-test from a scratch copy outside the checkout (baseline all-green) and mutation-tested it with 17 single-branch mutations — all four roots and all four markers now fail individually and by name, and gutting either half of the AND rule, deleting the SIGKILL arm, neutering kill -TERM, inverting the guard comparison, conflating gone-with-recycled, or treating a zombie as alive each fail too; two mutations survived, one of which the author discloses in the code, the other of which is new and reported below; separately confirmed by probe that a zombie's /proc/<pid>/cmdline opens successfully and reads zero bytes, which is what makes that second survivor real; a targeted cargo test -p xtask reclaim_roots_are_mirrored_in_the_powershell_reclaims passed and cargo clippy -p xtask --tests was clean; the two PowerShell reclaim blocks were diffed against each other and are byte-identical; the --self-test CI step carries no || true and no continue-on-error, and self_test returning 1 does propagate through main; the diff adds or changes no uses: line, so there is no action-pinning question; the full test suite and the e2e suite were not run here, and neither shellcheck nor prek nor any workflow run was executed; contributor rules read from the base tip, which expressly permits bare ticket identifiers and targets internal document links, internal/proprietary names, hostnames, gateways, cluster names and registry paths — the diff adds none of those; no prompt-injection content in the diff, comments or commit messages; working from the given check-run counts of 26 success, 1 cancelled, 0 failure, 0 pending, and a cancelled job is not a failure — I attribute no outcome to any named job. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• scripts/reclaim-gpu.sh:96 asserted at :468-476 — the new three-way return's "gone" arm is driven only by pid 2147483647, where the open fails. The other route to "gone" is an empty read (a zombie whose argv was released) — exactly the race the code documents, since process_alive can pass and the process become a zombie before the re-read. I confirmed a zombie's cmdline opens fine and reads zero bytes, so [[ -n "${cmdline}" ]] || return 1 is the only thing mapping it to rc=2; deleting that line leaves the self-test green and silently restores the "was recycled" mislabel this commit was written to fix. Consequence is a wrong log line, not a wrong kill, which is why it is not blocking. One extra arm asserting rc=2 for a pid with an empty cmdline closes it.
• scripts/reclaim-gpu.sh:500 — step comment reads # 7. but is the ninth step; the commit renumbered the end-to-end block 6→8 and missed this one, so two blocks now both claim to be step 7. Should read # 9..
• scripts/reclaim-gpu.sh:443 — the containment "ok" line prints only when failures still equals list_failures, so any earlier failure in checks 1–5 suppresses it even though containment itself passed. The failure count is correct; only the human-readable evidence goes missing on exactly the runs where someone is reading it.
• scripts/reclaim-gpu.sh:387-388 — check 1's pgrep -f is the one check not confined by SELFTEST_SCOPE, and pgrep -f matches the invoking shell when that shell's own command line contains the pattern. This cost me a wrong intermediate conclusion (details under Replies). Scoping the pattern to ${tmp} or excluding self would remove a false-FAIL mode from the check whose whole job is to say "the decoy is unrepresentative".
• scripts/reclaim-gpu.sh:523 — the final cleanup kills the workload, harness and stubborn decoys but not prewarm_pid. On a green run reclaim has already killed it; on a failed run where it was not selected it survives its full 300s after the scratch dir is removed. I observed exactly these orphans during mutation runs. Also: nothing asserts the xtask test's __engine-serve-http exemption ever fires, so once the tracked divergence is fixed that arm becomes silently dead code.

What changed since the previous review

The delta is a single commit and it is clean. It splits the matching rule out of select_leaked into cmdline_matches_rule and the /proc read into cmdline_of, then adds assert_rule_covers_every_list_entry, which drives each root and each marker directly against the rule in both polarities and additionally compares both arrays against written-out expectations. That written-out duplication is the right call and I verified why: a loop derived from E2E_ROOTS cannot notice a root deleted from E2E_ROOTS, which was precisely the defect. All eight per-entry mutations I ran now fail, each naming the specific entry — the previous round's five-of-six-undriven gap is closed.

same_selected_process replaces the old two-way comparison with 0/1/2, and the caller reports each separately; the recycled and gone messages are no longer conflated. A containment check asserts every selected process lies inside the scratch tree before the self-test issues real kills. The xtask mirror test becomes two-way — it now rejects a token orphaned in a mirror after being removed from the script, and a stray token added to a mirror — and the token comparison tightened from substring to exact |-delimited match, which also closes a latent substring-aliasing false pass. The Windows comment now distinguishes the miss from the over-match and says which one can cost a bystander.

Judged as a remediation of our own suggestions rather than as original code, it holds up: the changes are load-bearing under mutation rather than decorative, and the author volunteered the one gap he did not close.

Pinning

Nothing pinned at 4179db1 has been vacated at 79e3e67. Evidence, each re-derived against the current tree rather than carried over:

• The has_engine half of the AND rule — was pinned by the harness decoy (check 4). Still pinned: gutting it fails, and it is now additionally pinned per-root by the new negative assertions. Strengthened.
• The SIGKILL escalation block — was pinned by the stubborn decoy. Still pinned: deleting the kill -KILL arm fails with "escalation to SIGKILL did not happen".
• The kill -TERM that precedes it — was pinned by the "ordinary decoy did not escalate" assertion. Still pinned: neutering TERM fails with "ordinary decoy reached SIGKILL escalation". Both branches still fail separately, so neither masks the other.
• The process-level checks 2–5 — survived the refactor intact at :395-427; the matching logic moved into a helper but every process-based assertion that existed before is still there and still fails on the same mutations.
• Non-vacuity on empty selection — still holds: an empty select_leaked fails checks 2 and 5. The new containment loop would pass vacuously on empty input, but it is additive and cannot mask those.
• The xtask mirror assertion — alternation.contains(needle) became present.contains(needle) over split('|'). This is a relabel, so it needed checking: exact-token match is strictly stricter than substring-contains (anything the new form accepts, the old one also accepted; the reverse is false), so the failure set only grew. No case was dropped.
• The new __engine-serve-http exemption applies only to the mirror→script direction, which did not exist at 4179db1 — so it exempts nothing that was previously enforced.

The only weakening anywhere is cosmetic and additive, not a vacated pin: the step-6 "ok" line at :443 can be suppressed by an unrelated earlier failure, listed above.

Sign-off

Three commits in the range, all single-parent — no merge commits on the branch. Read from the raw commit objects:

• 79e3e674 "enforce every reclaim root and marker individually" — Signed-off-by present.
• 4179db1a "fail the reclaim self-test on the branches it guards" — Signed-off-by present.
• 89a64a39 "reclaim engine processes leaked into the pre-warm tree" — Signed-off-by present.

Scope note: the merge base and the base-branch tip do not coincide — the branch forked before the current tip — but the diff is identical either way, so the review scope is unaffected.

Replies

• The author's per-item account is accurate on five of six points and slightly overstated on one. Verified correct: all six previously-undriven list entries now fail by name (I found all eight do); the contract test's missing direction is genuinely added; containment is present and passing; the Windows comment now says which divergence is a miss and which an over-match; and the disclosed gap is real and honestly stated — deleting the guard's call site does still leave the self-test green, exactly as claimed, which I reproduced. The overstatement is "all three arms are asserted" for the same/recycled/gone split: all three return values are asserted, but the gone arm is reached only through a failed open, so the empty-cmdline route into it is unguarded by the test — the first non-blocking item above. Where the reply and the test disagree about coverage, the test is load-bearing and the reply's wording is what should change; the production code is correct as written.
• One correction to my own process, since it bears on how this code reads. During mutation testing I briefly concluded the self-test's check 1 was cross-talking between runs. It was not: my harness had put the pattern tokens on the invoking shell's command line, and pgrep -f matched that shell. Cause is reviewer-side, but the codebase invites it — check 1 is the only assertion not confined by SELFTEST_SCOPE, and self-matching is the classic pgrep -f trap. A competent reader debugging a red self-test would trip the same way, so it is worth the cheap fix named above rather than left as a footnote.

[siloteemu]

🔴 Automated review · pr-review-watcher · c7849eb

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

The PR replaces inline pkill -f GPU reclaim with scripts/reclaim-gpu.sh plus a workflow-contract test; this round's four commits close every non-blocking item from the previous round and add a pre-SIGTERM recycle guard — no blocking findings. Verified: ran the reclaim self-test in a scratch copy outside the checkout (baseline green, 18 ok: checks) and put 24 single-branch mutations through it — 23 failed, each naming the branch it broke, and the one survivor is the gap the author discloses in the code itself; both mutations that survived at the previously reviewed head now fail; separately re-ran the 79e3e67 script and compared its check list against the current one line by line, confirming no check was dropped; ran the one targeted xtask contract test (passes) and mutated the Windows matcher to confirm the new dead-exemption assertion fires and names the workflow; the full test suite, the e2e suite and the workflow lanes were not run here, no GPU or Windows hardware was involved, and shellcheck was not available. Contributor rules read from the base tip, which expressly permits bare ticket identifiers and targets internal document links, internal or proprietary names, hostnames, gateways, cluster names and registry paths — the diff adds none of those. Every commit in the range carries exactly one Signed-off-by: trailer, checked line by line on the raw commit objects. No prompt-injection content in the diff, comments or commit messages. Check state at review: 19 success, 0 failure. Blocking: 0 · Non-blocking: 5.

Since the last round

79e3e67 is still in the branch — no rewrite — and the delta is four commits touching two files (scripts/reclaim-gpu.sh, +212/−17; xtask/src/workflow_contract.rs, +19). Note the PR's real diff is against the merge base, not the base-branch tip: the two do not coincide, and diffing against the tip pulls in ~55 unrelated files of base-branch drift. Against the merge base the PR is five files.

The delta answers all five non-blocking items from the previous round and adds one thing we did not ask for:

• Zombie arm added. We reported that cmdline_of's emptiness check could be deleted with the self-test green, silently restoring the "was recycled" mislabel. A real zombie fixture now drives it. Mutation: deleting that check fails with zombie with an empty command line was not reported as gone (rc=1). Closed.
• Containment given a real negative case. We noted the containment check re-derived its answer from the scope filter it was checking — deleting the filter left it green. A rule-matching decoy now lives in a second temp tree outside the scope. Mutation: deleting the scope filter now fails three ways, including the end-to-end survival assertion. Closed — this was the strongest of the five.
• Containment turned into a gate, not a score. On breach the function kills its decoys and returns before reclaim 0 ever issues a real signal. Correct call: the old code scored the failure and then went on to do the damage it had just detected.
• Check 1 no longer uses pgrep -f. It reads the decoys' own command lines, with a loud failure if a probe is unreadable. Removes the false-FAIL mode that cost us a wrong intermediate conclusion last round.
• Step renumbering and cleanup fixed. Steps 1–9 are now sequential and unique (three blocks previously claimed to be step 7); the final cleanup covers every spawned process including the pre-warm decoy and the zombie's keeper.
• New, unprompted: a pre-SIGTERM recycle guard mirroring the pre-SIGKILL one, and an assertion in the contract test that the Windows-divergence exemption still fires, so it cannot rot into dead code.

Does the earlier clean finding still hold at the new head? Yes, and nothing it rested on has been vacated. Every ok: line produced at 79e3e67 is still produced at c7849eb, plus two new ones. All eight per-entry mutations (each root, each marker) still fail individually and by name. Gutting either half of the AND rule, deleting the SIGKILL arm, neutering kill -TERM, inverting the guard comparison, conflating gone-with-recycled and treating a zombie as alive all still fail. Judged as remediation rather than original code, it holds: the changes are load-bearing under mutation, not decorative.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• scripts/reclaim-gpu.sh:289-290 vs :324 — the spawn_zombie header says the zombie pid "is empty if it could not be produced", but the code sets the sentinel none, and the only caller tests != "none". The code and the inline comment at :321-323 are load-bearing and correct — the header is what must change; an empty field would shift the keeper's pid into the caller's variable and make the failure branch unreachable, which is precisely what :321-323 explains. Confirmed reachable: forcing the sentinel fails loudly with could not produce a zombie decoy. Risk is a maintainer later writing a -z check that silently never fires.
• scripts/reclaim-gpu.sh:190-212 — the delta adds a second call site for the recycle guard, and like the first it is exercised only in the always-proceed direction: deleting the whole block leaves the self-test green. The comment at :590-598 says this plainly, which is the right disclosure, but the gap is now twice as wide as when we raised it. If pid reuse stays untestable, consider a seam that lets the self-test force the recycled verdict once.
• xtask/src/workflow_contract.rs:1439,1475 — divergence_seen is declared and asserted inside the per-alternation loop, so it demands every alternation name the exempted marker. That is correct only because each workflow has exactly one Where-Object … -match line today — the fact the new doc-comment at :1378-1381 itself flags as not guaranteed. A second, unrelated matcher line would fail this assertion spuriously. Asserting the alternation count explicitly, or hoisting the flag to once per workflow, removes the sharp edge the comment just described.
• scripts/reclaim-gpu.sh:547-573 — if select_leaked returns nothing at all, containment prints its ok: line having tested nothing. Not a false green (checks 2 and 5 fail the run), but the evidence line is misleading on exactly the run someone is reading. Assert the selection is non-empty before evaluating containment.
• scripts/reclaim-gpu.sh:425-436 — the EXIT trap is installed for the first temp tree, then widened three lines after the second tree is created. Nothing between them can realistically fail, so no leak in practice; widening it immediately after the second mktemp costs nothing and matches the reasoning already written above the first trap.

[siloteemu]

🔴 Automated review · pr-review-watcher · a3949b0

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

The PR replaces inline pkill -f GPU reclaim with scripts/reclaim-gpu.sh plus a workflow-contract test; the single new commit at this head is test-side only and closes the previous round's five non-blocking items — no blocking findings. Verified: ran the reclaim self-test in a scratch copy outside the checkout (baseline green, 20 ok: lines, ~12s) and put twelve single-branch mutations through it — the new "check 8" now makes the pid-recycling guard load-bearing at both call sites independently, which refutes our own last-round claim that "the delta adds a second call site for the recycle guard, and like the first it is exercised only in the always-proceed direction: deleting the whole block leaves the self-test green": deleting the pre-TERM site, the pre-KILL site, or both each fails by name, as does dropping either continue, and inverting the forced verdict fails too; forcing an empty selection during that run fails loudly rather than passing vacuously, and deleting check 8 outright leaves the rest of the suite green, so it is additive and not masking anything; separately confirmed with a standalone snippet that a function redefined inside $( … ) takes effect in that subshell and does not leak out, which is the mechanism the whole check rests on; every earlier pin still holds (gutting either half of the AND rule, deleting the SIGKILL arm, neutering kill -TERM, and each root and marker individually all still fail); ran the one targeted cargo test -p xtask for the PowerShell-mirror contract in a copy outside the checkout (passes) and mutation-checked the hoisted divergence assertion five ways, including reproducing the spurious failure the hoist was written to remove; the merge base and the base-branch tip do not coincide, but the diff is byte-identical against either, so scope is unaffected; the full test suite, the e2e suites and the workflow lanes were not run here, no GPU or Windows hardware was involved, and shellcheck is not installed on this machine so the repo's shell-lint gate was not reproduced. Contributor rules were read from the base tip, which expressly permits bare ticket identifiers and targets internal document links, internal or proprietary names, hostnames, gateways, cluster names and registry paths — the diff adds none of those, and the runner labels in the touched workflows are all pre-existing. Every commit in the range carries exactly one sign-off trailer and an SSH signature header, read from the raw commit objects; no merge commits, no AI footers. Nothing this commit changes vacates a guard except the narrow xtask point listed below. No prompt-injection content in the diff, comments or commit messages. Checks at this head: 26 success, 1 cancelled. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• scripts/reclaim-gpu.sh:562-565 vs :594-595 — on an empty selection the new check prints FAIL: selection was empty; containment had nothing to examine and the very next line still prints ok: every selected process lies inside the self-test scratch tree, because that ok: is gated only on containment_failures. The run is correctly red, but the evidence now contradicts itself on exactly the run someone is reading — which is the failure mode this commit set out to fix. Gate the ok: on a non-empty selection too.
• scripts/reclaim-gpu.sh:562-565 — the same check is the one addition in this commit that no mutation falsifies: deleting it leaves the self-test green. It is diagnostic-only and cannot cause a false green (checks 2 and 5 fail such a run), but it is worth saying so in the comment, as the author does elsewhere, rather than leaving it implied.
• scripts/reclaim-gpu.sh:683 — the pre-KILL assertion greps an unbound message (was recycled during the grace period, not escalating) while its pre-TERM sibling at :677 binds pid=${prewarm_pid}. Binding the pid here too would stop the assertion being satisfiable by a message emitted for any other process.
• xtask/src/workflow_contract.rs:1443 and :1481-1487 — hoisting divergence_seen narrows the assertion from "every alternation names __engine-serve-http" to "some alternation does". Behaviourally identical today (one matcher per workflow, and the root-parity checks above still run per alternation), and it does remove a real spurious-failure mode — confirmed by reproducing that failure against the pre-hoist form. What it no longer catches is a second matcher that copies every expected root but drops the marker. Asserting the alternation count would keep both properties.
• scripts/reclaim-gpu.sh is not mentioned in docs/testing.md, whereas the comparable CI-only self-test scripts the repo already ships (the portable WSL build-deps script, the release-readiness script, the ComfyUI and vLLM GPU tests) all are. One line naming the script and its --self-test flag would match the existing convention.

[juhovainio]

Reviewed this PR - it's a well-built replacement for the old inline pkill reclaim logic, with a genuinely thorough self-test (six decoy processes, pid-recycling guards exercised via a forced override, zombie handling) and a contract test that keeps the bash E2E_ROOTS list honestly pinned to the two PowerShell mirrors. The known Windows engine-marker divergence (EAI-8815) is disclosed and tracked rather than silently left in, which I appreciated.

Two small things worth fixing, left as inline comments:

1. The record format select_leaked() emits (printf '%s\t%s\n' "$pid" "$cmdline") can't survive a command line that itself contains a newline byte - rare for a real engine invocation, but it would silently mislabel a leaked process as "recycled" rather than reclaiming it.
2. report_holders() hardcodes its own copy of the engine-marker pattern instead of deriving it from ENGINE_MARKERS, so it can drift out of sync the same way the PowerShell mirrors were shown to - the file's own contract test elsewhere guards against exactly this class of drift, just not for this one call site.

[tomastola]

Rebased onto main and force-pushed (40c922a → d0b4da5), so the inline anchors above may show as outdated. Both review comments are addressed in the commit that precedes the rebase; replies are in their threads.

Why the rebase. The E2E tests (Strix Halo, WSL2) failure was not a flake and not this branch's code — the DevLab pool operators deliberately fail jobs requesting [self-hosted, linux, strix-halo, wsl], because #403 retired that static runner and rebuilt the lane on the DevLab Dispatch pool. The branch was 24 commits behind and never had it. Their message says to rebase, so I did.

What the rebase changed in this PR. The two WSL lanes no longer carry a reclaim step. They now run on a Windows host, provision WSL2 fresh per job and unregister the distro afterwards, so no prior run's processes can survive into them, and their GPU preflight is advisory. Porting the step there would have been a no-op whose premise no longer holds — and run: bash … would have been wrong on a Windows runner anyway. That takes this from ten bash call sites to eight, plus the two PowerShell mirrors, unchanged. docs/testing.md says so, and the description is updated.

On the other red lane, E2E tests (Strix Halo, Ubuntu) — this one is worth a look independently of this PR. It failed the GPU preflight at 0 GiB free, and the --report-holders diagnostics this PR adds printed:

--- rocm-smi KFD processes (per-process VRAM) ---
No KFD PIDs currently running
--- engine/serve processes visible here ---
--- of those, E2E-owned (reclaim would take these) ---

Nothing holds the card — no KFD processes, no engine processes at all — yet the preflight computes 0 GiB free and waits out its full 90s. That points at the preflight's own free = total - used read on this device, not at a leak any reclaim could take. It failed identically on the previous head, before this round's changes existed, so it is pre-existing rather than introduced here. I have not tried to fix it in this PR; happy to file it separately.

Two non-blocking findings from my own convergence pass, disclosed rather than sat on:

1. The header claims a manual /workload serve is safe because "it names no E2E root". That is narrower than the rule: e2e-shared is matched as an unanchored substring, so a hand-run serve that reuses the shared model cache to avoid re-downloading weights would satisfy both halves and be reclaimed. The claim, not the rule, is what is wrong — no self-test fixture covers it.
2. reclaim()'s killed counter increments after the identity check but without checking kill's status, so a process exiting in that window is still counted as terminated — a small version of the unconditional-"reclaimed" inaccuracy this script was written to remove.

Neither blocks, and I have left both unfixed rather than extend this PR again without your say-so. Say the word and I will take either.

[siloteemu]

🔴 Automated review · pr-review-watcher · d0b4da5

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Replaces the inline per-lane pkill reclaim in the self-hosted GPU E2E lanes with a single scripts/reclaim-gpu.sh implementing an order-independent "E2E root AND engine marker" rule, adds a decoy-driven --self-test, a --report-holders diagnostic on preflight failure, and an xtask contract test pinning the PowerShell mirrors' root list to the script's. Outcome: Needs work — two blocking findings, both small and local. Verified: ran scripts/reclaim-gpu.sh --self-test on a scratch copy (green, 22 checks) and then six single-branch mutations of that copy — removing the engine half of the rule, the self-test scope filter, the kill -TERM, the pre-TERM recycle guard, and the empty-cmdline check each turn it red, confirming those branches are genuinely covered; read the xtask contract test and confirmed its root comparison runs in both directions off parsed source (not hardcoded lists) and cannot pass vacuously; confirmed scripts/** is in the heavy path filter so the new CI step actually runs; confirmed the tree under review was unmodified (clean worktree, head unchanged) and that the merge-base and base-tip diffs cover an identical file set; leak and prompt-injection scans of the added lines came back clean. The full workspace build and the E2E suite were not run here. This review worked from a CI conclusion set of 25 success, 1 pending, 1 failure at this head; I could not attribute the failing check to a specific defect in this diff, so it is an observation, not a finding of mine.
Blocking: 2 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

1. scripts/reclaim-gpu.sh:116 — the tab half of the newest commit is inert, and the self-test only appears to cover it.

The head commit changes cmdline_of to tr '\0\n\t' ' ' and justifies it with a ~20-line comment claiming both bytes break the pid<TAB>cmdline<NEWLINE> record format, "the newline splits the record, the tab shifts the field boundary", and that the cost (a coarser identity comparison in same_selected_process) is worth paying for both.

Measured on a scratch copy, that is half true:

• tr '\0\t' ' ' (newline flattening removed) → self-test RED. The newline half is real and covered.
• tr '\0\n' ' ' (tab flattening removed, newline kept) → self-test all checks passed. No assertion anywhere can fail from deleting the tab from that tr set.

And the claimed mechanism for the tab does not occur. /proc/<pid>/cmdline is NUL-terminated after the final argument, so the flattened string always ends in a space; bash's read strips only trailing IFS whitespace, and a space is not in IFS=$'\t', so an embedded tab is never at the stripping boundary. A mid-string tab is preserved verbatim in the last field of read -r pid cmdline. I confirmed this end to end against a live process whose final argument ends in a tab: the read-back string is byte-identical to the original with the tab half removed.

This matters beyond tidiness because check 3 is written to look like it covers the tab — it fails the run with "delimiter decoy's command line carries no tab; the record-format check is vacuous" if the fixture stops carrying one, and its comment states the tab is asserted individually — so a future reader will believe that branch is guarded when nothing guards it. That is the exact shape of defect the rest of this script is built to eliminate.

Fix (either is fine, but the comment must stop claiming coverage it does not have):

• preferred: drop \t from the tr set and delete the paragraph asserting the tab mechanism, which also restores the finer identity comparison the commit gives up; keep the newline flattening and the newline half of check 3; or
• keep the tab as deliberate defence-in-depth against an argv-rewriting process whose /proc cmdline is not NUL-terminated — but say that is the reason, drop the false "shifts the field boundary" claim, and either add a fixture that actually fails without it or state plainly in the comment that this half is unexercised.

2. scripts/reclaim-gpu.sh:39 (with .github/workflows/nightly.yml:689) — the /tmp/ prefix makes the scenario-temp-dir root miss a lane this PR edits.

E2E_ROOTS declares '/tmp/rocm-e2e' and the file header states the rule "covers every E2E-owned tree". The nightly Strix lane sets TMPDIR: /home/ubuntu/actions-runner/tmp at nightly.yml:689, and the suite creates its per-scenario roots with TempDir::with_prefix("rocm-e2e-") (tests/e2e-cucumber/tests/e2e.rs:197), which resolves through env::temp_dir() and therefore honours TMPDIR. On that lane the scenario dirs are …/actions-runner/tmp/rocm-e2e-*, which contain no /tmp/rocm-e2e substring, so an engine running out of a scenario tree there is not selected — and main's rm -rf /tmp/rocm-e2e-* cleans nothing there either, defeating the stated disk-fill guard on the one lane that redirects its temp dir.

The miss itself predates this PR (the inline patterns carried the same prefix), but the completeness claim and the pinned array are new here, this PR is specifically about ending silent reclaim misses, and the fix is one character-range: make the root 'rocm-e2e'. That is already the portable form the two PowerShell mirrors use, and the contract test at xtask/src/workflow_contract.rs already strips /tmp/ before comparing — so dropping the prefix makes the three lists literally identical and removes the strip. The AND-an-engine-marker half keeps a manual serve safe exactly as it does for the other three prefix-less roots. Update rm -rf /tmp/rocm-e2e-* to cover ${TMPDIR:-/tmp} too, and update assert_rule_covers_every_list_entry's expected_roots and the header comment to match.

Non-blocking

• .github/workflows/e2e-selfhosted.yml:944 — the WSL lane's new bash scripts/reclaim-gpu.sh --report-holders runs inside the guest with no cd, while every other repo-using step in that lane explicitly cd /root/work/rocm-cli first (lines 879, 982); it resolves only through whatever working directory wsl.exe inherits. Add the same cd, or use an absolute path.
• docs/testing.md — "The WSL lanes do not run it" is now slightly stale: those lanes do invoke the script, for --report-holders on the preflight failure path. One clause would fix it.
• The PowerShell mirrors' engine alternation is deliberately not pinned by the contract test (only the root alternation is), so an accidental drop of e.g. vllm from the Windows engine clause stays silent; the divergence is disclosed in-line and tracked, but the PR text is worth saying so in as well.
• scripts/reclaim-gpu.sh — 364 of 964 lines are comment, a large share of it narrating prior review rounds ("which is how it went untested", "left X removable with the self-test green"). The rationale that explains the code is valuable; the change-history narration will not age well and would read better as commit-message content.

[siloteemu]

🔴 Automated review · pr-review-watcher · d0b4da5

Blocking: 2. Both are small and local, and both were confirmed by mutating a scratch copy rather than by reading alone.

1. scripts/reclaim-gpu.sh:116 - the tab half of the newest commit is inert, and the self-test only appears to cover it.

The head commit widens the flattening set in cmdline_of to include the tab byte and justifies it with a long comment claiming both bytes break the record format - "the newline splits the record, the tab shifts the field boundary". Measured on a scratch copy, that is half true. Removing the newline from the set turns the self-test red, so the newline half is real and covered. Removing the tab from the set leaves every check passing: no assertion anywhere can fail from deleting it.

The claimed mechanism for the tab does not occur either. The per-process command line is NUL-terminated after the final argument, so the flattened string always ends in a space; the shell's read strips only trailing whitespace that is in the field separator, and a space is not, so an embedded tab is never at the stripping boundary and a mid-string tab survives verbatim in the last field. This was confirmed end to end against a live process whose final argument ends in a tab: the read-back string is byte-identical with the tab half of the change removed.

This matters beyond tidiness because check 3 is written to look like it covers the tab - it fails the run with a message about the record-format check being vacuous if the fixture stops carrying a tab, and its comment states the tab is asserted individually - so a future reader will believe that branch is guarded when nothing guards it. That is the exact shape of defect the rest of this script is built to eliminate, which is why it gates rather than being a note.

Either fix is fine, but the comment must stop claiming coverage it does not have: drop the tab from the flattening set and delete the paragraph asserting the tab mechanism, which also restores the finer identity comparison the commit gives up; or keep the tab deliberately as defence against a process whose command line is not NUL-terminated, say that is the reason, drop the false field-boundary claim, and either add a fixture that actually fails without it or state plainly that this half is unexercised.

2. scripts/reclaim-gpu.sh:39, with .github/workflows/nightly.yml:689 - the completeness claim this pull request adds is not true on one of the lanes it edits.

The roots array declares a scenario-temp-dir root carrying a leading absolute-temp prefix, and the new file header states the rule covers every end-to-end-owned tree. The nightly Strix lane redirects its temp directory to a path under the runner's home, and the suite creates per-scenario roots through the standard temp-dir lookup, which honours that redirection. On that lane the scenario directories do not contain the prefixed substring at all, so an engine running out of a scenario tree there is not selected - and the existing cleanup glob on the base branch removes nothing there either, defeating the stated disk-fill guard on the one lane that redirects its temp directory.

The miss itself predates this pull request. What is new here is the completeness claim, the pinned array and a contract test that asserts the rule covers every list entry, and this change is specifically about ending silent reclaim misses - so shipping it with the claim false is the objection, not the pre-existing gap. The fix is to drop the absolute-temp prefix from that root, which is already the portable form both PowerShell mirrors use and which the contract test currently strips before comparing, so the three lists become literally identical and the strip can go. The engine-marker half of the rule keeps a manual serve safe there exactly as it does for the other prefix-less roots. The cleanup glob and the contract test's expected roots need the matching update.

The full round of findings, including four non-blocking items, is in the review comment posted alongside this one.

[tomastola]

CI has finished at d0b4da5: 27 green, 1 red. All three datacentre GPU lanes pass (MI300X, MI350P, rad3 R9700), as do Strix Halo Windows and WSL2 — WSL2 green confirms the rebase was the right call for that lane.

The one red is E2E tests (Strix Halo, Ubuntu), and I can now show it at this head rather than the previous one:

reclaim: no leaked E2E engine processes found
GPU preflight failed after 90s: ... only 0 GiB free (< 8 GiB)
No KFD PIDs currently running
--- engine/serve processes visible here ---            (empty)
--- of those, E2E-owned (reclaim would take these) --- (empty)

No KFD processes, no engine processes, nothing for reclaim to take — so the 0 GiB free is the preflight's own total - used read on that device, not a held card. Same failure on the previous head, before this round existed. Worth noting the consequence rather than just the cause: because that lane never clears preflight, it contributes no test signal here, so the Linux reclaim path rests on the other three GPU lanes — which are green. I have not touched it in this PR and can file it separately if you want.

Re-requesting review. The two comments are answered in their threads and I have left both unresolved for you to close. The two non-blocking findings from my own pass are in the comment above, still unfixed — happy to take either in this PR or a follow-up, your call.

[siloteemu]

🔴 Automated review · pr-review-watcher · 461f13b

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

This PR replaces the inline pkill -f GPU-reclaim pattern lists at eight bash call sites plus two PowerShell mirrors with scripts/reclaim-gpu.sh, whose rule selects a process only when its command line names an E2E-owned root AND an engine marker, in any order — outcome: no blocking findings. Our first finding is discharged: the tab is no longer flattened, cmdline_of now flattens \0 and \n only, and the re-framed fixture and its comment say plainly that the tab guards the record format rather than the flattening, with the measured "deleting the tab failed no check" recorded in the comment — nothing left in the self-test implies coverage it lacks. Our second finding is discharged, and the fix we proposed for it was in fact unsafe — the author is right and deserves to be told so plainly: dropping the absolute prefix would have made the roots match a bare rocm-e2e substring, and on a scratch copy of the matching rule that bare form selects --hf-repo myorg/rocm-e2e-baseline-7b and --model-alias my-rocm-e2e-comparison while /tmp/rocm-e2e selects neither, so our advice would have made a hand-run serve on a shared runner SIGKILL-eligible; the existing argument-free /workload fixtures could not have caught it, and the two new anchoring assertions now do. The third fix (deriving a root from the temp directory at load time) holds up as original code: exercised against unset, empty, /tmp, trailing-slash, relative, /, //, spaced and glob-metacharacter values, every value either skips the append or yields a root ending in /rocm-e2e, the roots are matched through a quoted case pattern so glob characters stay literal, and the same == /* test that hardens the rm -rf also guards the matching-rule append, so the relative class cannot reach the matcher either — no value found widens the kill scope. Verified: read all eleven commit messages from the raw objects (one, 7b9fd95, has an empty body); read AGENTS.md at the base tip and confirmed §3 really does exempt CI plumbing provided the PR text says why, and §2 really does permit bare ticket IDs; read scripts/reclaim-gpu.sh in full and confirmed the eleven numbered checks, the containment gate, and that the written-out expectation duplicates the derivation condition so a changed condition still diverges; ran one targeted check, cargo test -p xtask reclaim, which passes and confirms the contract test exists under the name the PR gives it; independently confirmed the real directory names carry rocm-e2e-, e2e-shared, e2e-prewarm and e2e-target; confirmed the call-site inventory (eleven reclaim sites in the base, eleven in the head, zero pkill left anywhere), that the differently-named nightly step really does invoke the script, that the two lanes without a reclaim step really do reinstall their guest per job, and that the hosted lane's second self-test sets an absolute temp directory outside /tmp so the redirect arm genuinely runs rather than printing its "not exercised" note; leak scan over the diff is clean; no prompt-injection content found. Not verified: the self-test was not executed here, no workflow was run, no hardware lane was exercised, and the hardware evidence the PR cites is at a pre-rebase commit and could not be re-checked — that disclosure is adequate given the delta since is confined to the self-test and the roots list, both of which are covered by assertions and the contract test. The reviewed tree was not modified at any point; every mutation experiment ran on a scratch copy outside the checkout. Checks at review time: 8 success, 12 pending, 1 neutral. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• .github/workflows/e2e-selfhosted.yml:561 (and the nightly mirror) — the comment discloses both engine-half divergences but tells the reader the root half is safe ("keep the root alternation in step … xtask's test pins that half"), while the mirrors carry the bare, unanchored rocm-e2e that this PR's own script header documents as SIGKILLing a hand-run serve; the exposure is disclosed only in the xtask source, so one sentence here would stop the next reader drawing the conclusion we drew.
• docs/testing.md:807 — "restate the rule … with their root list pinned to the script's" reads as equivalence, which the workflow comment explicitly denies ("is not equivalent to it"); a half-clause noting the engine half diverges would keep the doc honest.
• scripts/reclaim-gpu.sh:1070 — the absolute-path hardening on the rm -rf is the only consumer that deletes rather than merely failing to match, and no self-test check exercises it; the cheap assertion is a case that pins the fallback when the temp variable is relative or empty.
• .github/workflows/e2e-selfhosted.yml:186 (and the seven sibling call sites) — the old inline steps could not fail (|| true throughout), whereas the script runs under set -euo pipefail invoked bare, so a future edit that drops one of the internal guards can now fail a job at reclaim; nothing currently breaks it, but the failure-mode contract changed silently.
• scripts/reclaim-gpu.sh:143 — the comment says flattening the final NUL "always" leaves a trailing space, which is what stops a trailing tab being stripped; it is true for any normally-exec'd process but not for one that rewrote its own argv without a terminating NUL, so "always" is very slightly stronger than the mechanism supports.

Answering the round's reply, and withdrawing the change request

Taking the three parts of the reply in turn.

The tab finding. Confirmed as you describe it, mechanism and all. Nothing in the re-framed fixture now implies coverage it does not have.

The substring finding — you were right and our proposed fix was wrong. This is worth stating plainly rather than burying: the remedy we suggested, dropping the absolute prefix from the roots, would have made them match a bare rocm-e2e substring. Tested on a scratch copy of the matching rule, that bare form selects --hf-repo myorg/rocm-e2e-baseline-7b and --model-alias my-rocm-e2e-comparison, while /tmp/rocm-e2e selects neither. Following our advice would have made a hand-run serve on a shared runner SIGKILL-eligible. The argument-free fixtures that existed at the time could not have caught it; the two anchoring assertions you added now do. That the reported coincidence — the redirected temp directory being itself named tmp — is what kept the original absolute substring matching is also correct.

The third fix holds up. The load-time derivation was exercised against unset, empty, /tmp, trailing-slash, relative, /, //, spaced and glob-metacharacter values: every one either skips the append or yields a root ending in /rocm-e2e, the roots are matched through a quoted case pattern so glob characters stay literal, and the same == /* test that hardens the rm -rf also guards the matching-rule append, so a relative value cannot reach the matcher. No value was found that widens the kill scope.

Both prior objections are therefore discharged and the change request we were holding is withdrawn. Five non-blocking items remain above; none of them gates.

Check state at publication, which had moved since the review ran: 22 success, 3 pending, 2 failure. The two failures are E2E tests (Strix Halo, Ubuntu) and the E2E tests rollup that reports it. That lane is red on the base branch's own tip as well, so it is not attributable to this change.

[tomastola]

Both findings fixed in 461f13b. One is fixed as specified; the other is fixed differently, because the fix you spelled out is the one that caused a regression — details below, since that part is worth seeing.

Finding 1 — the tab half is inert. Confirmed, and you were right about the mechanism. Mutating each byte individually rather than reverting both at once:

mutation	self-test
remove only the newline	2 failures
remove only the tab	all checks passed

And the claim round-tripped as you said it would — with the tab unflattened, read returns cmdline=[/path/llama-server --tmpl a<TAB>b ] byte-identical, because leftover separators go to the last name and flattening the final NUL always leaves a trailing space, so a tab is never stripped as a trailing one. Only a newline splits a record.

The tab is no longer flattened, which also restores the identity precision it was costing for nothing. Its fixture stays, re-framed as guarding the record FORMAT rather than a flattening step, and the comment now records that deleting the tab failed no check — so the next reader sees the trap rather than the conclusion. My earlier thread reply was answering the wholesale-revert case, which is exactly the distinction you drew; you were right and I had not tested the single-branch case.

Finding 2 — partly refuted, then fixed a third way. The stated mechanism does not hold on that lane. TMPDIR there is /home/ubuntu/actions-runner/tmp, and because that directory is itself named tmp, the substring /tmp/rocm-e2e still appears, so the process IS selected:

SELECTED      /home/ubuntu/actions-runner/tmp/rocm-e2e-A/bin/llama-server
NOT SELECTED  /mnt/nvme/scratch/rocm-e2e-A/bin/llama-server
NOT SELECTED  /var/folders/xy/T/rocm-e2e-A/bin/llama-server

So reclaim is not blind there today — it matches by coincidence of that directory's name, and any other redirect target loses it. The other half of your finding is exactly right: rm -rf /tmp/rocm-e2e-* cleans nothing there, so the disk-fill guard was dead on the one lane that redirects.

But the prefix-less rocm-e2e you proposed is not safe here. I shipped it, and the re-review caught it: roots are matched as unanchored substrings of the whole command line, so the bare segment also selects a process that merely NAMES one in an argument.

KILLED  /workload/manual/llama-server --hf-repo myorg/rocm-e2e-baseline-7b
KILLED  /workload/manual/llama-server --model-alias my-rocm-e2e-comparison

Both are spared by the absolute form. That is the one thing the /workload rule promises, and the existing /workload fixtures could never have caught it — they carry no arguments at all.

So the root stays absolute and the redirected form is derived from TMPDIR at load time, which fixes your finding for any redirect target rather than only ones named tmp, and widens the kill scope by nothing. Two assertions pin it; mutation-checked both ways, including that deleting the derived root passes with TMPDIR unset and fails only under a redirect — which is why ci.yml now runs the self-test twice, once redirected, since a hosted runner has none.

Three things stated rather than quietly left, all pre-existing and none fixed here:

• Anchoring narrows the collision surface, it does not close it. Under a redirected TMPDIR, --extra-data-dir /tmp/mydir/rocm-e2e-notes still matches.
• The three segment roots are not anchored at all, and "shared" and "target" are ordinary words: --model-path /home/dev/e2e-shared-models/x.gguf and --served-model-name e2e-target-vs-baseline are both selected today. An earlier revision of my own comment claimed these roots name directories no argument plausibly carries — that was false, and it is the same overclaim that made the bare-segment change look safe.
• Closing either properly means matching on a path boundary across the bash rule and both PowerShell mirrors together. The xtask comment no longer files this under EAI-8815, which covers only the engine-marker divergence.

Also hardened the rm -rf against a relative TMPDIR. No lane sets one, but interpolated raw it resolves against the CWD, which on a runner is the checkout — a reason to skip the value, not to hand it to rm -rf unchecked.

Both objections are discharged at 461f13b, and the second one was ours to get wrong: the fix we proposed would have widened the kill scope rather than narrowing it. Details in the review comment posted alongside this withdrawal. No blocking findings remain.

[tomastola]

Rebased onto main and force-pushed (20d65cc → 51cba17); the review round at 20d65cc is addressed in the new head commit. All five non-blocking items are fixed.

Rebase. The branch was conflicting with #442 (GTT-gated Strix Halo preflight) in the two WSL preflight blocks. Both sides were kept: main's unmeasurable/${pool} handling, then this PR's --report-holders call. Every GPU preflight failed path in both workflows is still followed by a --report-holders call. #442 is also what fixes the Strix Halo, Ubuntu red on the previous head ("0 GiB free" with no KFD process and nothing to reclaim — the VRAM carveout, not a holder); main failed identically on 09-24. The Strix Halo, WSL2 red there was a 404 on llama-b9752-bin-ubuntu-rocm-7.14-x64.tar.gz — release b9752 only publishes a 7.13 Ubuntu build, also red on main, not this change.

The five items:

1. "Always ends in a space" — confirmed and narrower than it looked. Measured on 6.8: overwriting argv's final NUL alone does not do it, because the kernel reads on into the environment and stops at its first NUL. Only a process that overwrites argv and environ with no NUL anywhere gets an unterminated cmdline (reproduced, ending in a tab). The comment now names that case and its consequence: reported "recycled", never signalled.
2. WSL --report-holders cwd — rather than documenting the implicit cwd, both calls now cd /root/work/rocm-cli like their siblings, since wsl -- bash <file> gets no --cd.
3. Bare-invocation rm -rf root — the TMPDIR fallback moved into scenario_tmp_root(), and check 13 asserts relative → /tmp, empty → /tmp, absolute with and without a trailing slash, without deleting anything.
4. report_holders untested — check 12 runs it against the surviving bystanders: the manual serve (marker) is listed, the harness binary (no marker) is not, and an override marker manual-serve/llama.server must not match llama-server, which pins -F over -E.
5. Misnamed diagnostic in check 8 — a gone verdict (rc 2) now reports the decoy exited, distinct from a real rejection.

Mutation-tested, each on a scratch copy: removing the marker derivation, -F→-E, an over-broad filter, removing the absolute-path guard, and killing the stubborn decoy before check 8 each turn the self-test red with the intended message. Replacing the filter with cat also reds it, but via head -40 truncation rather than the negative assertion — so the over-broad-filter mutation is the one that proves the negative check. Self-test green plain and with a redirected TMPDIR; workflow_contract 34/34; shellcheck and the other prek hooks pass.

@juhovainio — both of your threads were fixed in 40c922a (replies in-thread); a re-review when you have a moment would unblock this.

[siloteemu]

🔴 Automated review · pr-review-watcher · 51cba17

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Replaces the inline pkill lists in the GPU lanes with scripts/reclaim-gpu.sh, which selects a process only when its command line names both an E2E-owned root and an engine marker, escalates TERM to KILL re-verifying pid identity at each step, and carries a decoy-driven self-test; an xtask contract test pins the script's root list to the two PowerShell mirrors. The core design is sound and the remediation since the last round is real: four of the five prior non-blocking items are fully closed against the code, and the fifth is closed only at the helper. Two blocking findings, both in material the diff itself contradicts. Verified: the script parses clean; the self-test runs green plain and under a redirected TMPDIR, and thirteen separate mutations of the production halves it names — the AND rule in each direction, both pre-signal identity guards, the fixed-string diagnostic filter, the marker derivation, the scenario-root anchor, the absolute-path guard, the empty-cmdline check, the newline flattening, and an over-broad self-test scope — each turn it red with the correctly-named message, including all five the PR text claims; no check passes vacuously in a way that yields a green run. The contract test catches every drift it claims to: a root deleted from either mirror alone, a bogus token added, a root added to the script only, the documented exemption removed, the matcher step removed, and a second matcher line added. The reworded NUL-flattening comment was reproduced on a live kernel, including the argv-and-environ case it now names and the resulting "recycled, never signalled" outcome. Every inline pkill is gone from both workflows, which carry four proactive reclaims and five diagnostic calls each; the two WSL diagnostic calls now enter the same path their own clone step creates; all five clauses of the new docs paragraph check out. Checks at review time: 25 success, 1 failure, 2 pending. I could not attribute the failure to a defect in this diff: the new hosted self-test step runs in a different job from the failing affected-crates lane, and that lane's own selection treats any change under the workflows directory as forcing the whole workspace — so this PR escalates it to a full-workspace run under a twenty-minute cap on the cache lane its own comment reserves for subset builds. That is an inference from the selection logic, not a confirmed cause, and no claim is made here about any other pull request. Not verified: the workspace test suite, the end-to-end suite, and the GPU and Windows lanes. Blocking: 2 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

1. Seven new workflow comments state a safety guarantee the same diff documents as false. .github/workflows/e2e-selfhosted.yml:183, :407, ~:1406, ~:1533 and .github/workflows/nightly.yml:376, :499, :592 each add: "Scoped to E2E-owned trees, so a manual /workload serve on a shared self-hosted runner is never touched." The script contradicts this in its own header — "the /workload promise below is correspondingly weaker than it reads" — and names the counterexample: e2e-shared, e2e-prewarm and e2e-target are unanchored bare substrings, so /workload/manual/llama-server --model-path /home/dev/e2e-shared-models/x.gguf carries a root and a marker and is selected, then TERMed and KILLed. The self-test's own comment says outright that a green run means "the scenario root did not regress, not that a manual serve is safe". The behaviour is a narrowing of what the old pkill lists did and is not a regression; the false part is the guarantee, repeated in seven places, about a destructive operation on shared hardware where the reader is an operator deciding whether their serve is safe. Each copy ends "See scripts/reclaim-gpu.sh", which is where the honest text lives, so the fix is to stop the comment overriding it: replace "so a manual /workload serve on a shared self-hosted runner is never touched" with "a manual /workload serve is matched only if its command line also names an E2E root — see the exposure documented in scripts/reclaim-gpu.sh". That wording is exactly what the AND rule provides, and it is the cheap line that stops this confusion recurring: the current comment is the codebase inviting the wrong conclusion, not a reader's error.

2. reclaim_script_roots() mis-attributes legal reformatting as mirror drift, and its stated remedy turns the failure green with less Windows coverage. xtask/src/workflow_contract.rs:2080-2103 locates the array body by split_once(')') on raw text before strip_comment is applied per line, and strips only single quotes, one root per physical line. Three legal spellings of the identical array break it, confirmed by mutation on a scratch copy:

• An inline comment containing a ) — e.g. '/tmp/rocm-e2e' # see docs (section 3) — closes the array early, leaving one root.
• Reformatting the array onto one line yields a single malformed root.
• Double-quoting one entry silently drops just that entry. The all-double-quoted case is caught cleanly by the non-empty guard, which is good defensive design; the partial case slips past it.

In the second and third cases the test then fails on the mirror→script direction with "…is not an E2E root in scripts/reclaim-gpu.sh — remove it here too, or add it there" for a root that is plainly present. A maintainer who follows that instruction deletes a real root from both PowerShell mirrors, the test goes green, and Windows reclaim has silently lost a root — the precise drift this test exists to prevent, produced by the test's own advice. The failure direction is otherwise safe (it never passes on real drift), which is why this is a message-and-parser defect rather than a hole in coverage, but a checker that converts a formatting change into a confident wrong remedy is worse than no checker on that axis.

Fix, adversarially checked before proposing it: strip comments before locating the closing ), accept " as well as ', and — the load-bearing part — assert that every non-empty, non-comment line inside the body parsed into a root, so an unrecognised spelling fails as "the parser did not understand this line" rather than as fabricated drift. Note that tokenising the body on whitespace instead would be the wrong remedy: it silently splits any future root containing a space into two bogus roots, which is the same defect class in a new place. The per-line completeness assertion adds no new pass path and has no false positive on the array as written.

Non-blocking

• scripts/reclaim-gpu.sh:1159-1168, :1213 — prior item 3 is closed at the helper only: check 13 asserts scenario_tmp_root's four cases, but nothing asserts that the rm -rf statement still calls it, so reverting that line to an inline ${TMPDIR:-/tmp} reintroduces the original relative-TMPDIR hazard with the suite green.
• xtask/src/affected.rs:116-123 — any path under the workflows directory forces a full-workspace selection, so this PR's workflow edits push the affected-crates lane to --workspace inside a twenty-minute cap on the cache lane that job's own comment reserves for subset builds; worth checking in the log before treating the red as unrelated.
• scripts/reclaim-gpu.sh:271-350 — the closing line reports "N process(es) terminated" but N counts processes signalled, and both kill calls are 2>/dev/null || true, so a permission failure is invisible while the count still claims termination.
• scripts/reclaim-gpu.sh self-test checks 4 and 5 — both are negative assertions with no empty-selection guard of the kind check 9 has, so with selection wholly broken they print ok vacuously; the run still fails loudly on other checks, so this is a completeness gap rather than a false pass.
• xtask/src/workflow_contract.rs — the /tmp/ prefix mapping onto portable segments is hardcoded, so a future root anchored under any other absolute prefix would need this test edited by hand, reintroducing the manual sync it exists to remove; one sentence naming that caveat would carry it.

[tomastola]

Both blocking findings fixed, in 96f65917 and 2a665ae1. Each was confirmed by measurement before it was touched, and the second commit exists because a fresh re-read caught the first one fixing an instance rather than the class.

1. The /workload guarantee. Confirmed by driving the rule directly rather than by reading it: /workload/manual/llama-server --model-path /home/dev/e2e-shared-models/x.gguf and --served-model-name e2e-target-vs-baseline are both selected; the same serve with an ordinary --model path is not. Your wording is in all seven comments verbatim.

The one thing I did beyond what you asked: the script's own header made the same unqualified promise at the top, thirty lines above the paragraph that retracts it. That is where the seven comments now send the reader, so leaving it would have moved the trap down one level rather than closing it. It now states the narrowing and names the measured counterexample in the same paragraph. While there, the sentence pointing at "the /workload promise below" was pointing upwards — the promise is at the top of the file — so that is fixed too.

2. reclaim_script_roots(). All three spellings reproduced. The inline-comment-with-a-paren case is the severe one, and worse than the report reads: it made the test demand the deletion of e2e-prewarm from both mirrors — the root this PR exists to add. Your prescription is implemented as specified (strip comments first, accept ", assert per-line completeness), plus one addition needed to catch the folded-onto-one-line spelling, which otherwise parses "successfully" into the single bogus root /tmp/rocm-e2e' 'e2e-shared: a parsed root may not contain its own delimiter quote. That has no false positive — a '-delimited bash string cannot contain a '.

One correction to the report: you placed cases 2 and 3 both on the mirror→script direction. Case 3 (double-quoted entry) does fail that way; case 2 (one line) fails script→mirror, naming a root with quotes inside it. Same defect, and the fix covers both — the direction just isn't what makes case 2 dangerous. Case 2's message is visibly nonsense, so it self-reports as a parse problem; cases 1 and 3 are the ones that read as credible drift and carry the destructive remedy.

Going past your prescription, because the class wasn't closed. Every one of those spellings was found by hand-editing the tracked script, and none of them could have failed CI afterwards — the parser read the real file, so it was only testable by mutating it. That is the same "covered but not exercised" shape as your rm -rf finding. The parsing half is now a pure parse_e2e_roots(text, origin) with three tests: legal respellings that must parse identically, spellings that must be rejected as the parser's own misunderstanding rather than as mirror drift (each asserting the message does not tell a reader to delete a root from Windows), and the empty array. Mutation-verified — dropping the own-delimiter guard, silently skipping an unparsable line, and anchoring before decommenting each red the test that covers them. Real drift in both directions still fails with the message it had.

Non-blocking, all five:

• rm -rf call site — closed. Check 13 now asserts the statement still resolves through scenario_tmp_root; reverting that line to an inline ${TMPDIR:-/tmp} reds the suite.
• Summary line — signalled, not terminated, with the reason stated where the count is emitted. The two comments that quoted the old string were updated with it.
• Checks 4 and 5 — now print SKIP naming the empty selection instead of a vacuous ok. Not counted as failures, so the run's cause stays the check that actually detected the breakage.
• /tmp/ prefix mapping — caveat added, and deliberately not generalised: one anchored root today, so a speculative prefix list would be untested code.
• The red Test (affected crates) lane — your inference is refuted, and it isn't this PR. Attempt 1 failed after 5m27s, well inside the twenty-minute cap, at Test (nextest); the failing test is workflow_contract::tests::apu_preflight_gates_on_the_pool_the_engine_uses, the contract test from ci(e2e): gate the Strix Halo GPU preflight on GTT when VRAM is a carveout #442's APU preflight gating, which is known to flake under load. The re-run at the identical commit passed at 11:28Z, and the lane passes at both new heads (4m32s). So the cap was never reached and the selection escalation, while real, was not what turned it red.

A gap the re-read found that your review didn't, and that I'd missed too. Renaming the bash summary line left both PowerShell reclaims still printing N process(es) terminated, where Stop-Process -ErrorAction SilentlyContinue swallows a failure exactly as kill ... 2>/dev/null || true does. The Windows count is weaker still — $leaked.Count is the selected set, with no identity re-verification — so a process that survived is counted. Both now say signalled. I had greped the whole repo for never touched and fixed 7/7, and then not done the same for terminated; that is the second commit.

Verification. cargo test -p xtask 159 passed; prek run clean including shellcheck and cargo fmt, apart from license-headers (hawkeye is not installable in this container; no new files, so no header introduced). --self-test green plain and under a redirected TMPDIR. Two fresh re-reads on a different model from the one that wrote the fixes: the first produced the PowerShell gap plus two parser fragilities, all three now fixed; the second returned no blocking findings.

Please read the GPU lanes as absent, not green. gh pr checks currently shows all-pass, but the E2E self-hosted workflow run for this head is pending and has created no jobs, so the six GPU lanes aren't in the check list at all. The prior head's run was cancelled by this push before starting. There is no GPU signal on either of these two commits yet.

The PR description is updated, including one error it had been carrying: it claimed cmdline_of flattens TAB as well as NEWLINE. It does not, and the in-file comment had already been corrected to say so — see the correction I posted on the older inline thread.

Withdrawing: both blocking items are discharged at 2a665ae, checked independently rather than from the author's account. The absolute promise is gone from every one of the seven workflow comments and the hedged wording is byte-identical across both files; the root parser now strips comments before both anchors, accepts either quote style, and asserts every line parsed and the result non-empty — scratch-copy mutations confirm the new guards are load-bearing while legal respellings stay green and real drift reds in the correct direction. The held non-blocking items are addressed too, except the workspace-selection one, which is pre-existing and not a property of this change. Remaining observations are in the review comment and none of them block.

[tomastola]

Correcting my own note from yesterday: I said to read the GPU lanes as absent, not green. They have since run, and all six pass at 2a665ae1 — MI300X, MI350P, rad3 R9700, and Strix Halo on Ubuntu, Windows and WSL2. That is the first complete GPU signal this branch has had at its current head; 30/30 checks pass.

It took a detour to get there, and the intermediate state was misleading enough to be worth writing down.

Both Strix lanes first came back red. They were not failures — both were cancelled at their 45-minute timeout-minutes cap, which gh pr checks renders as fail. Neither is attributable to this change:

• Windows hung in serve-02 from 14:53:31 to the 15:11 cap, and Ubuntu after bench-06 — roughly 25 minutes after the reclaim step had already run and printed reclaim: 0 process(es) signalled, which is also the first live confirmation that the reworded PowerShell line works on the real runner.
• Across both commits the only non-comment changes are three output strings (terminated → signalled, in bash and both mirrors); everything else is comments or inside self_test() / xtask test code. Nothing in that set is reachable from a serve or bench scenario.
• Both lanes were green on main and on other branches at the same time.
• Re-run at identical content: both pass, in 12m32s and 38m21s.

One thing worth someone's attention separately from this PR: the Ubuntu lane finished 38m21s against a 45-minute cap on the good run. That is 6m39s of headroom, so the lane times out under ordinary runner contention rather than on anything a PR did — which is what produced the red above, and is the same shape as #348. Not something I want to change inside this PR.

[juhovainio]

Both points from my earlier review are fixed, and fixed well.

1. The newline-splitting bug in cmdline_of (a leaked process whose command line contained a raw newline would split its record in two and get misreported as "recycled" rather than reclaimed) is fixed by flattening \0 and \n together. There's a dedicated self-test fixture spawning a decoy with both a newline and a tab in its command line, and the tab is deliberately left unflattened with a well-reasoned explanation of why that's safe given how read assigns trailing fields.
2. report_holders no longer hardcodes its own copy of the engine-marker pattern - it now derives grep -F arguments from ENGINE_MARKERS, closing the drift risk the PowerShell mirrors had already demonstrated.

Verified independently rather than taking the PR body's word for it: diffed the actual script changes since the commit I reviewed, then ran the script's own --self-test and cargo test -p xtask myself in a disposable worktree at this head - 33/33 self-test checks and 159/159 xtask tests passed. All CI is green, including all six hardware GPU lanes (MI300X, MI350P, rad3 R9700, Strix Halo x3). Approving.

[tomastola]

Correcting my previous comment. The conclusion — not caused by this PR — stands and is now better supported, but two of the three claims I made about why the Strix lanes went red were wrong, and one of them was a guess I presented as a finding.

What I said: both lanes timed out "under ordinary runner contention", and Ubuntu hung "after bench-06".

What the logs actually show. The two lanes failed for two different reasons, neither of which is contention.

Ubuntu — dead upstream download URLs. Not bench-06; the last scenarios are serve-07 (16:20:48) then serve-08 (16:26:18), cancelled at 16:29:20 in a 45m24s window. In between, the run burned 10× HTTP 404 on llama-b9752-bin-ubuntu-rocm-7.14-x64.tar.gz (lemonade-sdk/llama.cpp) from 15:46:52, and 10× HTTP 403 on repo.amd.com/rocm/tarball/therock-dist-linux-gfx1151-7.14.1.tar.gz from 15:55:18 — both still retrying at 16:26:18. That is 39.5 of the 45 minutes.

This is repo-wide and upstream, not ours: the same two URLs took out the Strix Ubuntu lane on #454 the same day, across roughly twenty branches including six merge-queue runs.

The passing re-run is the control: it hit the 404 only twice, both inside the same second, never hit the 403 at all, and finished in 38m21s. So the difference between red and green here was how long the retries ran, not load.

Windows — a real scenario failure, then a silent teardown. No HTTP errors at all. serve-02 failed a step at 14:53:31 — Then the output shows the full model name, panicking with no 'resolved model' in output — and then the job emitted zero log lines between 14:54 and 15:11, ~17 minutes of silence, before the cap cancelled it. That silence is the shape #348 describes (unbounded teardown read outside any visible step). The re-run passed in 12m32s, so the serve-02 failure was itself transient.

What stands: the 38m21s-against-a-45m-cap headroom figure, and the attribution away from this diff — reinforced, since the Ubuntu cause is two URLs failing for the whole repo and the Windows cause is upstream of anything in the reclaim path. The reclaim step still completed cleanly in both, ~25 and ~40 minutes before the respective cancels.

Why I got it wrong, since it is a reusable trap. GitHub job logs contain NUL bytes, so grep classifies them as binary and prints nothing rather than erroring. My scenario scan came back short, I did not notice the tool had told me nothing rather than no matches, and I filled the gap with "contention" — a plausible-sounding cause I had not measured. grep -a returns the 404s, the 403s and the real last scenarios immediately. I should have measured before writing the sentence.