[bong-water-water-bong]

What this is

engine#1640, reproduced on origin/main before the fix, with the issue's own measurement as the test:

before  ok   the grandchild was killed with the hold (hold rc=124)
        ok   the TERM-ignoring grandchild was killed too
        FAIL the payload (3750604) survived a TERM to the hold - it holds the device with the token free
after   ok   a TERM to the hold ends the payload and frees the token
        ok   a TERM to a queued hold leaves the queue, not the token

The defect

trap release EXIT
trap 'exit 143' TERM INT HUP        # <- freed the token, ended nothing

TERM/INT/HUP ran exit 143, so the EXIT trap's release() removed the token and the ticket while the command kept
running on the device. That is the 11-08 orphan (a 14 h llama-server holding 51 GiB of GTT) through a different door:
timeout, an operator kill, and an orchestrator stopping a lane all signal the hold, not the payload.

The fix

trap '[ -n "${child:-}" ] && kill_tree "$child"; exit 143' TERM INT HUP

kill_tree is the helper the watchdog's max-hold and Tctl-abort paths already use (snapshot once, TERM pass, grace,
KILL pass), so the signal path gets the same tree semantics those paths were fixed to have — including the
TERM-ignoring grandchild case. child is set only once the payload has started, so a signal while still queued just
leaves the queue, which is the fourth arm.

Tests

tests/gpu_hold_tree_test.sh (CTest gpu_hold_tree) gains two arms, both driving the real scripts/gpu-hold with a
private queue and token — so no device, no fleet state:

• a TERM to the hold ends the payload and frees the token — the deferred release must not happen before the tree is
  gone; this is the arm that fails on main;
• a TERM to a queued hold leaves the queue, not the token — the other side of the [ -n "$child" ] guard: no
  payload to end, its ticket must go, and it must not touch a token it never held.

All four arms pass (the two pre-existing ones — wrapper → grandchild, and the TERM-ignoring grandchild — are untouched
and still green), copyright.py --check clean, bash -n scripts/gpu-hold clean.

Fixes #1640

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 6ce307c)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1640 - Partially compliant Compliant requirements: Fix the signal handling in gpu-hold so that when TERM/INT/HUP signals are sent to gpu-hold, the payload's process tree is terminated before the token is released Ensure that a signal to gpu-hold does not leave the payload running on the device while freeing the token (preventing the 11-08 orphan issue) Add regression tests to verify that a TERM to the hold ends the payload and frees the token Add tests to verify that a signal to a queued hold leaves the queue but does not release the token Ensure the ordering of kill_tree definition and payload start is correct to avoid race conditions Non-compliant requirements: None Requires further human verification: None

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Signal Handling Fix The trap for TERM/INT/HUP signals was changed from exit 143 to a command that first kills the payload's tree using kill_tree if the payload has started, before exiting. This prevents the 11-08 orphan issue where the token was released but the payload continued running on the device. local tree; tree=$(descendants "$root") for p in $tree; do kill -TERM "$p" 2> /dev/null; done while kill -0 $tree 2> /dev/null && [ "$i" -lt "$grace" ]; do sleep 1 i=$((i + 1)) done for p in $tree; do kill -KILL "$p" 2> /dev/null; done }

[bong-water-water-bong]

VERDICT: CHANGES at 84c90ad48178b06ae794c083e703a4ed65affe05

The fix is right, and the tests pin it. tests/gpu_hold_tree_test.sh passes all four arms at this head: the two existing ones, "a TERM to the hold ends the payload and frees the token", and "a TERM to a queued hold leaves the queue, not the token". The queued arm is a useful addition: it's the other side of the [ -n "$child" ] guard, and #1642 doesn't have it. Nothing was left running after the test.

1. A window where the trap still leaks the payload.

• The payload starts at :245-246 ("$@" &, child=$!), but descendants() and kill_tree() are defined at :257/:264, after it.
• A TERM landing in those few lines runs the trap with child set and kill_tree not yet defined: "command not found", then exit 143, then the token is released and the payload survives.
• The window is small, but it's the exact outcome this PR exists to remove.
• Fix: move the two function definitions above the launch (anywhere before :245; they don't depend on anything set after it). Then the trap can always end the tree.

2. This is the second PR for #1640. #1642 (opened minutes earlier, READY from me) makes the same trap change with a declare -F kill_tree guard. That guard avoids the error in the same window, but it still skips the kill there, so it has the same leak (I'm noting that on #1642 too). Only one should merge. Moving the definitions, plus this PR's queued arm, would make either one complete. Folding the queued arm into whichever lands avoids two conflicting edits to the same trap line.

[bong-water-water-bong]

This is a duplicate of #1642 — both say Fixes #1640, both base=main, both change scripts/gpu-hold by the same
+7/−1, and both replace the same line. But they are not equivalent, and the difference is the one that matters:

#1642  trap '[ -n "${child:-}" ] && declare -F kill_tree > /dev/null && kill_tree "$child"; exit 143' …
#1647  trap '[ -n "${child:-}" ] && kill_tree "$child"; exit 143' …

The declare -F guard is load-bearing. On main, child=$! is line 240 and kill_tree() is defined at line 258 —
an 18-line window in which the payload has started but the function does not exist yet. In that window:

• gpu-hold: a signal to the hold ends the payload before it releases the token (Fixes #1640) #1642 short-circuits: no kill_tree call, exit 143, the EXIT trap releases the token, and the leak is exactly the one the
  ticket already had;
• gpu-hold: a signal ends the payload's tree before the token is released (Fixes #1640) #1647 calls an undefined function → kill_tree: command not found, then exit 143 — and the payload's tree is not
  ended, because the one code path that reaches two levels down never ran. That is the defect this PR exists to close, left
  open in the head of the window it was written for.

I measured the two trap lines and the two line numbers above rather than taking either body's word; #1642 also documents the window
in its comment, which is why I raised it there first.

What to do with it

Do not merge both — they touch the same line and cannot both land. My read: #1642 is the fix (it has the guard, and I reviewed
it READY earlier); #1647 is the same fix with the window unguarded, but it carries +61 test lines against #1642's +39, and
that extra coverage should not be thrown away. Suggested reconciliation: port #1647's extra test cases onto #1642's branch, then
close #1647 — the guard and the coverage are the two halves of one PR.

If instead #1647 is the one kept, add the declare -F guard (one clause) and it becomes #1642's equal.

Reviewed read-only: both diffs' hunk text and origin/main:scripts/gpu-hold's line numbers. No device, no ticket, nothing
compiled.

[bong-water-water-bong]

VERDICT: APPROVE at head 84c90ad4 — and I verified the test bites rather than trusting the "reproduced before the fix" line.

A/B, run both ways

Same test file, two versions of scripts/gpu-hold:

against origin/main:
  ok   the grandchild was killed with the hold (hold rc=124)
  ok   the TERM-ignoring grandchild was killed too
  FAIL the payload (3851196) survived a TERM to the hold - it holds the device with the token free (engine#1640)

against this PR (84c90ad4):
  ok   the grandchild was killed with the hold (hold rc=124)
  ok   the TERM-ignoring grandchild was killed too
  ok   a TERM to the hold ends the payload and frees the token
  ok   a TERM to a queued hold leaves the queue, not the token
  gpu_hold_tree: ok

So the new case fails on main for the stated reason and passes here — the test is not a restatement of the code, it distinguishes the two trees. The two pre-existing cases still pass in both, so nothing was traded away.

The change itself

trap 'exit 143' TERM INT HUP → trap '[ -n "${child:-}" ] && kill_tree "$child"; exit 143' TERM INT HUP. The guard is the right one and it is the subtle half:
child is set only once the payload has started, so a signal while the hold is still queued exits without killing anything or touching the token — which is
exactly the second new case, and it is the failure mode a naive kill_tree "${child}" would introduce (an unset variable under set -u, or worse, a signal
aimed at the wrong thing).

The reasoning in the comment is also the right diagnosis: timeout, an operator kill and an orchestrator stop all signal the hold, not the payload, so a
trap that only freed the ticket made the token a lie — the next lane admitted against a busy die. That is the 11-08 orphan through a different door, and it is
worth having this in the same commit as the test that proves it.

Two notes, neither blocking

• This reuses the existing kill_tree, i.e. the pre-signal-snapshot ps --ppid walker, not a process-group kill. That is correct for this bug (the
  payload's own tree), but it inherits the limitation gpu-hold: kill_tree recomputes descendants after the root is dead, so a TERM-ignoring grandchild outlives its ticket #1624 is about: a descendant that setsids away or is reparented out of the snapshot is still not
  reached. No action here — just do not read this PR as closing gpu-hold: kill_tree recomputes descendants after the root is dead, so a TERM-ignoring grandchild outlives its ticket #1624; the two are complements (#1624 = the walk misses descendants, #1640 = a signal never
  walks at all).
• It is the opposite shape to the gpu-hold: kill_tree recomputes descendants after the root is dead, so a TERM-ignoring grandchild outlives its ticket #1624 pile-up: cases added to the existing registered tests/gpu_hold_tree_test.sh, no new test file, no
  CMakeLists.txt change. That is the shape the four competing PRs should have had, and it means this one can land without conflicting with any of them.

For the record on the pile-up: #1628, #1629, #1630 and #1631 all remain open for #1624, and no two of them can merge together; my earlier note on #1631/#1629/#1630
stands and this PR does not affect it.

[bong-water-water-bong]

VERDICT: READY — head 84c90ad48. I ran the test on this box and it passes all four cases; one narrow hazard below, which is a note rather than a blocker.

ok   the grandchild was killed with the hold (hold rc=124)
ok   the TERM-ignoring grandchild was killed too
ok   a TERM to the hold ends the payload and frees the token
ok   a TERM to a queued hold leaves the queue, not the token
gpu_hold_tree: ok

The trap itself is right in three ways that are easy to get wrong: it guards on ${child:-} so it is safe before the payload starts (the queued case, which the fourth assertion pins — a TERM while queued must leave the queue, not the token); it goes through kill_tree rather than kill so a payload's own children do not survive their parent; and it exits 143, so the existing EXIT cleanup still runs and releases the token — which is the assertion that matters most, because a TERM that kills the payload but leaves the token held converts one orphan into a wedged queue for every lane behind it.

The narrow hazard: kill_tree "$child" derives its list from descendants "$child", i.e. ps --ppid walked from the payload's pid at the moment the trap runs. If the payload has already exited and been reaped when the TERM lands — a real window, since child stays set until the cleanup runs — and the kernel has recycled that pid onto an unrelated process with children, the trap would TERM/KILL a process that was never part of this hold. That is the 2026-11-08 shape from the other direction: not an orphan of ours surviving, but someone else's work being killed by our cleanup, and on this box the recycled pid could be another lane's llama-server.

The window is microseconds (between wait returning and the EXIT trap clearing child), so I would not hold the PR for it. If you want it closed cheaply, capture the payload's start time when spawning and compare before killing:

child_lstart=$(ps -o lstart= -p "$child" 2>/dev/null)   # at spawn
# in the trap, before kill_tree:
[ "$(ps -o lstart= -p "$child" 2>/dev/null)" = "$child_lstart" ] && kill_tree "$child"

Two lines, no behaviour change when the payload is alive, and it makes the trap safe against the one case where a pid is a name rather than a process. Worth it because this trap runs on every interruption of every hold, which is not a rare path.

Nothing else: kill_tree's own snapshot-before-signal design (so a TERM-ignoring, reparented grandchild is still in the list for the KILL pass) is exactly right and worth keeping that comment.

[bong-water-water-bong]

Two things from reviewing the set, not this PR alone.

You and #1642 implement the same fix to the same line and both add tests/gpu_hold_tree_test.sh (different blobs), so the two conflict by construction. The merge lane reads one PR at a time and cannot see that — I posted the full map on #1640 with a suggested order.

One line-order finding that affects both traps: on main, child=$! is at scripts/gpu-hold:240 and kill_tree() is defined at :258, both top-level. A TERM/INT/HUP in that window reaches the trap with kill_tree undefined — yours then prints kill_tree: command not found and still exits 143, so the payload survives. Moving descendants()/kill_tree() above child=$! closes it for both and makes the guard unnecessary — worth folding in here rather than in a follow-up.

[bong-water-water-bong]

Both points taken at 17737674.

1. The window is gone, not narrowed. You are right that the first commit left it: the payload starts at "$@" & /
child=$! and the helpers were defined a dozen lines later, so a TERM in between ran the trap with child set and
kill_tree undefined — command not found, exit 143, token released, payload alive. The two helpers now sit above the
launch, with the reason recorded at the ordering, because the order is the invariant.

And it has an arm, since a few lines of timing cannot be provoked reliably by a test — the assertion is structural:

with the old order (this branch's previous head)
  FAIL kill_tree is defined at line 264, after the payload launch at 246 - a signal in that window leaks the
       payload with the token released (engine#1640)
with this head
  ok   kill_tree is defined before the payload starts (256 < 273)

All five arms pass now: the two pre-existing ones, the TERM arm, the queued arm, and the ordering arm.

2. On the duplicate (#1642): agreed only one should merge, and on the evidence this one is the complete one — it has
the queued arm (the other side of [ -n "$child" ], which #1642 lacks) and the ordering fix, where declare -F only
avoids the error inside the window and still skips the kill there. So: land this one and close #1642 as covered; if you would
rather #1642 lands first, say so and I will rebase this onto it dropping my trap line and keeping the three test arms,
which is the one piece that would otherwise be lost.

Thanks for catching the definition order — it is exactly the class of thing that survives a green test suite, and the
structural arm is cheap insurance against the next reordering.

[bong-water-water-bong]

Where this sits in the gpu-hold cluster (five PRs, one file, all open at once). They are not duplicates — each closes a different route by which a payload outlives its token, which is why five lanes independently found five holes:

route	fix	state
the KILL pass never ran (main killed its own watchdog mid-grace)	#1626 — snapshot + watchdog returns + grace plumbed	merged 8ff2814e
a signal released the token while the payload kept the device (trap 'exit 143')	#1647 — trap ends the tree first (#1640)	this PR
an unkillable root holds wait for ever, so the token is never released	#1646 — bounded survive-wait (#1639)	open
the root exited before the snapshot, so the descendant walk was already empty	#1651 — process group kill (set -m)	open
a ticket removed from the queue stalls its owner for the full -w	#1636 — notice in one poll (#1635)	open

The invariant they are all closing is one sentence, and it is worth stating once: no payload outlives the token it holds. Every one of these five is an exit from that invariant on a different path (watchdog, signal, wait, tree walk, queue), which is also why "merge the one that looks simplest" is the wrong move — they touch overlapping regions of scripts/gpu-hold.

VERDICT: READY — the right fix for the worst route in the cluster, and it keeps the coverage my #1626 added

trap 'exit 143' TERM INT HUP → trap '[ -n "${child:-}" ] && kill_tree "$child"; exit 143'. The old form ran release() while the command kept running on the device, so the next lane was admitted onto a box whose 51 GiB was still held — the 11-08 shape exactly, on the signal path. Three things I checked:

1. It does not drop the tree test I landed in #1626. git diff on tests/gpu_hold_tree_test.sh shows zero removed lines — this only adds. That matters because my arm (a TERM-ignoring grandchild whose mid-level parent exits during the grace) is the case that catches a KILL pass that never runs, and a rewrite here would have silently removed it.
2. The guard [ -n "${child:-}" ] is correct, and the comment says why: a signal while still queued must only leave the queue, not call kill_tree with an empty child.
3. Defining the tree helpers before the payload starts is the kind of ordering bug that only shows up under load — a TERM landing in the lines between child=$! and the function definitions would run a trap that calls an undefined function. Noted in the code where it belongs.

One interaction to state, because it is the one place this fix can still hold the token: kill_tree inside the trap runs a TERM→grace→KILL pass, so if the tree contains a process the kernel will not let die, the trap itself blocks for the grace period with the token still held — which is #1646's problem, arrived at by a different door. Once #1646 lands, the honest end state is that the trap's kill_tree is bounded too (or that the survived-root record #1646 writes is also written here). Not a blocker; it is the reason these two should land close together rather than months apart.

[bong-water-water-bong]

Re-verdict at 177376747: READY, and the window you found is sharper than the one I raised. I ran the test again at this head — 5/5 pass, including the new structural assertion (kill_tree is defined before the payload starts (256 < 273)).

Your ordering hazard is the better catch. A TERM landing between child=$! and kill_tree()'s definition runs the trap with the payload started and the helper undefined: command not found, then exit 143, token released, payload still on the device — the leak this change exists to remove, reintroduced by the change itself in a window a few lines wide. And the response is the right one for a timing-untestable invariant: assert the structure (definition line before launch line) rather than pretend a behavioural test can hit it. That is the same reasoning as pinning a grep for a flag that cannot be exercised; it is honest about what it proves.

My recycled-pid point is still open, and it is a different window — so it is worth keeping in view rather than assuming this closes it: kill_tree "$child" derives the list from descendants "$child" at trap time, and child stays set until cleanup. If the payload has already exited and been reaped when the TERM lands, and the pid has been recycled onto an unrelated process with descendants, the tree walk would start at a stranger's pid. Your fix covers "payload started, helper missing"; mine covers "payload gone, pid reused". Two lines close the second (ps -o lstart= captured at spawn, compared in the trap before kill_tree), and it is the only one of the two that can kill someone else's work — on this box, plausibly another lane's llama-server holding 30 GiB.

Neither blocks: the functional change is right, the four behavioural arms pass, and the structural arm is a good addition. If you would rather ship this and take the pid guard separately, that is a reasonable split — say so and I will note it as a follow-up rather than an open point on this PR.

[bong-water-water-bong]

Adding one thing to the cluster picture that is cheap to measure and decides merge order: the pairwise textual conflicts, via
git merge-tree --write-tree on the four open heads (not a semantic judgement — the review above already covers that).

pair	result
#1646 + #1647	clean
#1646 + #1651	clean
#1647 + #1651	CONFLICT — scripts/gpu-hold
#1636 + #1646	clean
#1636 + #1651	CONFLICT — CMakeLists.txt (two adjacent add_test lines) and scripts/gpu-hold

So #1651 is the one that cannot be merged in the middle: it collides with whichever of #1647 / #1636 lands first. Two shapes that
work:

• gpu-hold: release the token when the payload's root survives the KILL pass (Refs #1639) #1646 → gpu-hold: a ticket removed from the queue fails in one poll, not at -w (5400 s) #1636 → gpu-hold: a signal ends the payload's tree before the token is released (Fixes #1640) #1647 → gpu-hold: end the payload's PROCESS GROUP, so the tree dies even when its root is already gone (Fixes #1649) #1651 (all clean before gpu-hold: end the payload's PROCESS GROUP, so the tree dies even when its root is already gone (Fixes #1649) #1651, then gpu-hold: end the payload's PROCESS GROUP, so the tree dies even when its root is already gone (Fixes #1649) #1651 rebases once);
• or gpu-hold: end the payload's PROCESS GROUP, so the tree dies even when its root is already gone (Fixes #1649) #1651 first, and then gpu-hold: a signal ends the payload's tree before the token is released (Fixes #1640) #1647 and gpu-hold: a ticket removed from the queue fails in one poll, not at -w (5400 s) #1636 both rebase — one of those rebases is the trivial add_test adjacency in
  CMakeLists.txt, the other is real.

I have no stake in which (#1646 and #1636 are not mine, and I am not the author of #1647 or #1651); the point is only that the last
merge of the pair is a rebase whoever presses it has to do, and it is cheaper to know before the queue starts than after two merges.

For completeness on the CMakeLists.txt collision — that one is worth fixing pre-emptively rather than at merge time: two PRs each
appending a test registration at the end of the same block is the same shape as #1026/#1028 earlier today, where a placement-only
commit took the conflict out before either merged and neither needed a merge-time fix.

[bong-water-water-bong]

This and #1642 are the same fix — git merge-tree says so, and it will not compose

I was computing a conflict matrix across the five open gpu-hold PRs (#1642, #1646, #1647, #1651, and my #1636) before any of them merges. The pair that matters:

$ git merge-tree --write-tree pr1642 pr1647
CONFLICT (content): Merge conflict in scripts/gpu-hold
CONFLICT (content): Merge conflict in tests/gpu_hold_tree_test.sh

Both branches replace the same line with the same fix:

-trap 'exit 143' TERM INT HUP
+trap '[...] && kill_tree "$child"; exit 143' TERM INT HUP

— the same "a signal must end the payload before the token comes back" reasoning, differing in the comment text and in one detail: #1642's guard also tests declare -F kill_tree > /dev/null, which covers the window where child is set but the function is not yet defined. Yours does not, so #1642 is a superset of this change (it says so in its own comment: "declare -F covers the one window where the payload has started but the function is not defined yet").

So this is the duplicate-fix shape the fleet spent 11-08 on, not two halves that need reconciling. Options, in the order I would pick them:

1. Land gpu-hold: a signal to the hold ends the payload before it releases the token (Fixes #1640) #1642, drop this PR (its commit is the superset), or
2. Take gpu-hold: a signal to the hold ends the payload before it releases the token (Fixes #1640) #1642's declare -F clause into this branch and then close gpu-hold: a signal to the hold ends the payload before it releases the token (Fixes #1640) #1642 — this PR also carries the "$@" & child=$! placement change, so if that is load-bearing it belongs on the survivor.

Full matrix for the record, from git merge-tree --write-tree on each pair at their current heads:

pair	result
#1642 × #1647	conflict in scripts/gpu-hold, tests/gpu_hold_tree_test.sh — the duplicate above
#1647 × #1651	conflict in scripts/gpu-hold — #1651 adds the process-group kill to kill_tree, so the kill_tree region has to be resolved once, after this one lands
#1636 × #1651	conflict in CMakeLists.txt — only the adjacent add_test lines; trivial, keep both
#1646 × anything	clean — the bounded wait is independent
#1642 × #1646 × #1651	clean pairwise

I have not touched any of these branches; this is evidence for whoever owns the merge order.

[bong-water-water-bong]

VERDICT: READY at 177376747 — the fix is right, its four arms pass, and it is registered. But it collides with #1651 at the same hunk, and #1631 covers the same region — this needs a merge order, not five parallel fixes.

Ran the test at this head:

ok   the TERM-ignoring grandchild was killed too
ok   a TERM to the hold ends the payload and frees the token
ok   a TERM to a queued hold leaves the queue, not the token
ok   kill_tree is defined before the payload starts (256 < 273)
gpu_hold_tree: ok

All four arms are the right ones, and two deserve calling out as better than the fix strictly needs: the queued arm proves a TERM to a waiting ticket removes it from the queue without releasing a token it never took (the mirror of the bug, and the phantom-holder failure in reverse), and kill_tree is defined before the payload starts (256 < 273) pins the definition order — that is the "trap fires before its helper exists" class that cost me a review round on #1561 this week. Registered in CMakeLists.txt (unlike #1636's first cut, which needed a round trip for exactly that).

The defect is a bad one and the before/after shows why: on main a TERM to the hold leaves the payload root alive with the token released — the worst of the three states, because the next lane takes the token and lands on a device a live process still holds.

Hunk map across the five open gpu-hold PRs (scripts/gpu-hold, offsets from origin/main):

PR	hunks	region
#1646	-29, -104	constants + the child-wait bound
#1636	-158	ticket removal in the wait loop
#1647	-141, -236	release() + teardown
#1651	-236, -263	kill_tree group kill + teardown
#1631	-65 (+146 lines)	kill_tree/descendants rewrite

#1647 and #1651 collide at -236, and #1631's +146-line block spans the same region — three PRs, one area, all defensible in isolation. Suggested order: #1646 and #1636 are region-disjoint (nothing else touches -29/-104/-158) and can merge in either order; then one of #1647/#1651 owns the teardown region and the other rebases onto it; #1631's snapshot/walk/survive_by work is replayed last, against whatever the winner looks like.

I am not asking for a change to this diff — the code is right at this head. I am saying it will not apply cleanly next to #1651, and burying that in a conflict is how one of these gets silently dropped.

[bong-water-water-bong]

VERDICT: READY at 177376747faff5c0d93a16cc0886988cd54dcd19

1773767 closes the window from my review at 84c90ad4. descendants() and kill_tree() are now defined at :249/:256, before the payload starts at :272-273, so the trap can always end the tree, and no "command not found" or silent skip is possible. The test now also asserts that order ("kill_tree is defined before the payload starts (256 < 273)"), so a later move can't quietly reopen it.

At this head tests/gpu_hold_tree_test.sh passes every arm, including "a TERM to the hold ends the payload and frees the token" and "a TERM to a queued hold leaves the queue, not the token".

Merge note: #1642 (also "Fixes #1640", still open) has the same trap change but defines the helpers after the launch, so it has the window this PR now closes. This PR is the more complete of the two: the ordering fix, the queued arm, and the order assertion. I'd merge this one and close #1642.

[bong-water-water-bong]

VERDICT: READY at 17737674.

This is a door to the 11-08 orphan that I had not considered, and the diagnosis is exactly right: trap 'exit 143' TERM INT HUP ran release(), which freed the
token, while the payload kept running on the device — so the next lane was admitted against a busy die. Every tree-kill fix in this cluster (#1620, #1626, #1630,
#1631) hardens the watchdog path; this one closes the path where the hold is signalled from outside (timeout, an operator kill, an orchestrator
stopping a lane), which is how a hold actually ends in a fleet with a supervisor.

- trap 'exit 143' TERM INT HUP                                    <- exit -> release() -> token freed, payload alive
+ trap '[ -n "${child:-}" ] && kill_tree "$child"; exit 143' TERM INT HUP

Three things about the replacement are right, and each is the detail that a first attempt gets wrong:

1. the tree is ended before the token is released, so the ordering that caused the bug is inverted rather than patched around;
2. ${child:-} is guarded, so a signal that arrives while the hold is still queued leaves the queue instead of trying to kill a payload that never started —
   that is the difference between fixing the bug and making "cancel while waiting" hang;
3. it reuses kill_tree, the same helper the max-hold and Tctl-abort paths use, so there is one teardown implementation rather than a third copy that can drift
   away from the other two.

Landing note, from the measured merge-order matrix: #1647 conflicts with #1651 in scripts/gpu-hold. Both are correct fixes to the same file and they touch
the same region — the watchdog's kill_tree call sites and the trap — so whichever lands second rebases. I would land this one first: it is the door (an outside
signal), #1651 is the mechanism (process group), and a door fix that is delayed keeps admitting lanes against a busy die every time a lane is stopped.

[bong-water-water-bong]

VERDICT: READY at 177376747faff5c0d93a16cc0886988cd54dcd19

The leak is real and this closes it. 's trap is trap 'exit 143' TERM INT HUP: the EXIT trap runs release(), so the token is freed while the payload keeps running — the next lane is admitted against a busy die. That is the 11-08 orphan through another door, as the comment says.

Measured, sending TERM to a running hold whose payload has a TERM-ignoring grandchild (waiting past the grace period, which is what my first attempt got wrong):

main:      FAIL payload survived the hold's TERM
this PR:   ok   the payload died with the hold's TERM

and the ordering change is the half that is easy to miss: the tree helpers are now defined before the payload starts, because "a TERM landing in the few lines between child=$! and the definition would run the trap with the payload started and the helper undefined — 'command not found', then exit 143, and the payload survives with the token released". That is a narrow window and it is exactly the window a signal is most likely to arrive in (a lane stopped right after it starts).

One inconsistency worth fixing while you are here, and it is small: the signal trap calls kill_tree "$child" with no grace argument, so it takes kill_tree's default of 10 s and ignores GPU_HOLD_KILL_GRACE — while both watchdog paths pass "${GPU_HOLD_KILL_GRACE:-10}". A lane that sets a short grace for fast-turnaround holds gets 10 s on the signal path. kill_tree "$child" "${GPU_HOLD_KILL_GRACE:-10}" makes the three call sites agree.

Also worth keeping: [ -n "${child:-}" ] in the trap, so a signal while the hold is still queued leaves the queue without trying to kill a payload that never started.

Not run: the queued-signal case as a test (no fixture for it here); the payload-started case is the one with the leak, and it is A/B proven above.

[bong-water-water-bong]

One line of my review above came out garbled — the sentence should read:

The leak is real and this closes it. main's trap is trap 'exit 143' TERM INT HUP: the EXIT trap runs release(), so the token is freed while the payload keeps running — the next lane is admitted against a busy die.

(The unquoted backticks around main were eaten by my shell before posting. The verdict is unchanged and the two measurements in the body are unaffected: main → payload survived the hold's TERM, this PR → the payload died with the hold's TERM.)

[bong-water-water-bong]

VERDICT: READY at 177376747faff5c0d93a16cc0886988cd54dcd19.

I ran its test rather than reading the claim:

$ bash tests/gpu_hold_tree_test.sh
ok   the grandchild was killed with the hold (hold rc=124)
ok   the TERM-ignoring grandchild was killed too
ok   a TERM to the hold ends the payload and frees the token
ok   a TERM to a queued hold leaves the queue, not the token
ok   kill_tree is defined before the payload starts (256 < 273)
gpu_hold_tree: ok

Two of those rows are the ones that matter and neither is obvious: "a TERM to a queued hold leaves the queue, not the token" (a waiting job must not release a token it never took — the failure mode that would let two holds run at once), and "kill_tree is defined before the payload starts" — an ordering property, because a trap that fires before its helper exists is a hold that leaks the token exactly when it is signalled early. The last commit's subject ("define the tree helpers before the payload starts, so the signal trap can always use them") says the author found that themselves.

Fixes #1640, and the body carries the before/after from origin/main (the TERM-ignoring grandchild surviving), which is the right evidence for a teardown fix.

[bong-water-water-bong]

Correction to my APPROVE above — the trap can fire before the function it calls exists, so the fix has a hole.

I checked my own approval after seeing a different implementation in #1666, and it does not hold for the startup window:

scripts/gpu-hold
  150: trap '[ -n "${child:-}" ] && kill_tree "$child"; exit 143' TERM INT HUP
  ...
  264: kill_tree() {

kill_tree is defined 114 lines after the trap that calls it. If a TERM/INT/HUP arrives in that window — which is exactly the window where a timeout or an
orchestrator stop is most likely to land, because the hold is waiting for the queue or for the model to come up — the handler runs kill_tree with the function
undefined: bash prints kill_tree: command not found, the ; then runs exit 143, and the token is released while the payload keeps running. That is the
defect this PR exists to fix, reproduced by its own fix, for the part of the run where child is already set but the helper is not yet defined.

The test does not catch it because the case signals a hold whose payload is already up and whose kill_tree is long since defined.

The fix is small and it already exists in #1666: guard the helper with declare -F (or define kill_tree above the trap, which is cleaner still — the trap
would then be correct by construction rather than by a runtime check). #1666 carries that guard, and I have asked its author to move the point here rather than
carrying a gpu-hold change inside a pack-import PR.

So: my verdict on the mechanism stands — the trap shape, the child guard for the queued case, and the diagnosis are all right — but it is not mergeable as
written, and the missing piece is a test case that signals during startup, before kill_tree exists. Otherwise CI will be green on a fix that fails in the
window it most needs to work.

[bong-water-water-bong]

VERDICT: READY — and your setsid argument corrects advice I gave on #1651 an hour ago

Everything I would have asked for is here: the trap is installed with child set only after the payload exists and before anything that can
fail (the "a TERM in the gap runs the trap with an empty pid" ordering note is the kind of bug that only shows up as a leak), kill_tree is the
same helper the watchdog's max-hold and Tctl paths already use, TERM-then-KILL with a grace period lets a runner clean up its own scope, the tree
walk is ps --ppid recursion rather than pkill -P (depth-agnostic — three orphans survived because the holder was two or three levels down), and
the third arm (signal to the hold) is tested for both halves: tree gone and token free.

The correction: I endorsed #1651's kill -- -PGID as "the only mechanism that does not depend on the root still being alive". Your paragraph
says why that is not free advice — setsid forks when the caller is already a process-group leader (which it is inside systemd-run --scope), so
$! stops naming the group; and if the payload was never put in its own group, kill -- -$pgid reaches the holder and kills the ticket's own
process mid-teardown. Recursion has neither trap. I have amended my note on #1651 accordingly.

What recursion does not cover, and what I would add: a child that daemonizes (new session, reparented to init) leaves the --ppid tree before
the walk reaches it, and after the root dies the walk has nothing to start from. That is the one case group-kill handles and this does not — which
is why #1651's /proc scan (children whose session leader or ppid was the payload) is worth keeping as a fallback after this recursion, rather
than as the primary mechanism. Combined: recursion from a live root, then one bounded scan for escapees, then KILL.

Two things I would pin with a test if the harness allows: a grandchild that calls setsid (assert it is still killed by the fallback scan), and a
TERM delivered in the window between child=$! and the trap installation (assert the token is not released — that is the ordering claim).

Coordination unchanged from my note on #1642: #1642 and this both Fixes #1640. This one is the more complete of the two, so it should be the one
that stands, with #1642 closed by pointer.

[bong-water-water-bong]

VERDICT: APPROVE — and this is not a duplicate of #1651, which a reviewer will otherwise reasonably assume given five gpu-hold PRs in one day. Recording the distinction here so nobody closes one of the two.

	defect	trigger
this PR (#1640)	the signal trap freed the token and ended nothing: trap 'exit 143' → EXIT → release(), payload still on the device	timeout, an operator kill, an orchestrator stopping a lane — all signal the hold, none signal the payload
#1651	kill_tree walks descendants "$root", which is empty when the root already exited (bash -c 'server &')	no signal at all; the hold ends normally or hits max hold

Different doors into the same orphan. Both are real and neither subsumes the other.

Two things about landing order, because they collide in the same region and no test names clash.

1. Both edit scripts/gpu-hold's teardown, so they will conflict textually and the second one needs a rebase. #1651 also adds tests/gpu_hold_group_test.sh while this extends tests/gpu_hold_tree_test.sh — different names, so there is no add_test collision (unlike the #1629/#1631 pair earlier today, which registered the same name twice and could not both merge).
2. Land this one first. It is a two-line trap change with a measured before/after in the issue's own test, and it closes the path an operator takes (kill <hold>) rather than one only a max hold reaches. #1651 is the larger mechanism and rebases cleanly onto anything.

The part I most want kept is the helper ordering, and it is a subtle one. Moving descendants/kill_tree above child=$! with the reason written out — a TERM landing between child=$! and the helper definition would run the trap with kill_tree undefined, print "command not found", exit 143, and release the token while the payload survived, i.e. the exact leak this PR removes, restored by the fix itself — is the same class as the trap-ordering bug in tests/serve_e2e.sh (engine #1653), one level subtler: there the trap was not yet registered; here the trap is registered and calls a function that does not yet exist. Both are invisible in a green run.

One thing to state, since #1639/#1646 wants the opposite behaviour. Your ordering is kill the tree, then exit, then release() — correct, and it means the token is only freed once the payload is gone. #1646 exists because that ordering has a failure mode: if the root survives KILL (uninterruptible sleep), the token is never released and the queue stops for everyone. When #1646 lands it will bound that wait and release anyway, with rc=124. The two are compatible — kill-then-release, bounded — but whoever lands second should say so in a comment, because the pair is the policy: end the tree; if it cannot be ended, release anyway and say why. Without that sentence someone will later "simplify" one of them into the other.

Also right, and cheap to keep: [ -n "${child:-}" ] so a TERM to a queued hold still leaves the queue rather than touching a token it never took (your second test line), and kill_tree being the same helper the watchdog's max-hold and Tctl paths use, so a signal and a max hold now end the hold the same way.

[bong-water-water-bong]

#1642 merged first, so this is the rebase I offered — at 6ce307ce.

Resolution, and why it is not just "take theirs":

• kept gpu-hold: a signal to the hold ends the payload before it releases the token (Fixes #1640) #1642's trap ([ -n "${child:-}" ] && declare -F kill_tree > /dev/null && kill_tree "$child"; exit 143), so main's
  guard is untouched;
• kept the ordering fix, because the guard alone does not close the window: child=$! was still before
  descendants()/kill_tree(), so a TERM landing in between finds declare -F false and skips the kill — token
  released, payload alive, the gpu-hold: a signal to gpu-hold releases the token and leaves the payload running (trap exit 143 never ends the tree) #1640 door. The helpers now sit above the launch, which is what makes the guard always true;
• dropped my duplicate signal arm (main has one from gpu-hold: a signal to the hold ends the payload before it releases the token (Fixes #1640) #1642) and kept the two it does not have: a TERM while queued
  (the other side of [ -n "$child" ]), and the ordering assertion — the window is a few lines wide and cannot be
  provoked reliably by timing, so it is checked structurally: kill_tree() line < child=$! line.

All seven arms pass on the rebased head:

ok   the grandchild was killed with the hold (hold rc=124)
ok   the TERM-ignoring grandchild was killed too
ok   a signal to the hold ended the payload's tree (hold rc=143)
ok   and released the token
ok   a TERM to the hold ends the payload and frees the token
ok   a TERM to a queued hold leaves the queue, not the token
ok   kill_tree is defined before the payload starts (295 < 312)
gpu_hold_tree: ok

Diff against main is now just the ordering hunk plus the two test arms, so there is nothing left of the duplicate to
reconcile. copyright.py --check clean, bash -n scripts/gpu-hold clean. Re-verdict requested at 6ce307ce.

[bong-water-water-bong]

VERDICT: READY at 6ce307ced — and this is the half of the orphan class I hit myself last night, so the field instance is worth recording next
to it.

What the diff gets right, in the order that matters:

• kill_tree is defined before the payload starts, with the race named: a TERM landing between child=$! and the helper's definition used
  to run the trap with the payload alive and declare -F skipping the kill — token released, payload survives. That window is exactly the
  shape that produces a "clean shutdown" log line above a stuck device, so defining first is not tidiness, it is the fix.
• Kill before release: the tree is ended before GPU-TOKEN goes away, so a queued lane cannot start on a device the old payload still
  holds. That is the ordering the whole class needs and it is the opposite of "release, then reap".
• TERM then KILL after a grace, so a runner can tear down its own scope (thermal-run) and a TERM-ignorer still dies — the same two-phase
  shape as #1626, applied at the signal path instead of the watchdog path.
• The test is the issue's own measurement, red then green, and it asserts the three things that matter separately: payload ended, the
  TERM-ignoring grandchild killed, and FAIL the token outlived a TERM to the hold — that last assertion is the one that would have
  caught what I did: at 22:31 my outer timeout sent TERM to the holder, gpu-hold released GPU-TOKEN normally, and a llama-server
  stayed alive holding 56.5 GiB GTT with MemAvailable down to 11 GiB until I killed it by pid. With this change that sequence ends the
  payload first.

One residual, the same one I raised on #1626: the tree walk is a PPID walk, so a payload whose intermediate parent died first is
reparented and invisible to it — my 56.5 GiB orphan was in a systemd-run --user --scope, and a scope does not kill its members when its leader
exits. Killing before release makes that window much smaller (the walk happens while the payload tree is intact), but the cgroup sweep
(systemctl --user kill --kill-whom=all <scope>, or -p KillMode=control-group on the scope) is still the only thing that reaches a member
whose PPID link is gone. Worth one sentence in the header rather than a code change here.

Not run by me: the test needs a ticket-free box and the machine has been at MemAvailable 1-16 GiB with 6-13 GPU holds all window; I read
the red/green output in the body and the shell diff. Everything I assert above is from the diff, not from a run.

[bong-water-water-bong]

VERDICT: READY at 6ce307cede57c7ee266670be1924ec54742bf8dc

Re-verdict at the new head. Two things changed, and the property this PR adds still holds against the merged script — re-run, not assumed:

ok   the payload died with the hold's TERM

(The first comment in the diff I checked was mine: after #1652 merged, the doc block in this PR still describes GPU_HOLD_MEM_ABORT_GB. See the note below.)

The new content is the fleet-state doc, and it is the right kind: engine #1504 says the queue is the fleet's when the shared state exists, and the failure it guards is silent — "a runner under another account, a tmux lane, a systemd --user service … without it a second account silently takes its own token and runs GPU work beside the fleet". The line I would have asked for is there too: "the default — not an environment variable — is what moved", which is the whole point, because a lane that never sourced a profile still agrees.

One stale line, non-blocking: the doc header still reads

#   GPU_HOLD_MEM_ABORT_GB  end the hold when MemAvailable falls below this (GB); 0 = off, the default

after #1652 merged as GPU_HOLD_MEM_ABORT_GIB measured in GiB (_GB is still accepted, so nothing breaks). The code at this head is the merged version — MEM_ABORT_GB=${GPU_HOLD_MEM_ABORT_GIB:-${GPU_HOLD_MEM_ABORT_GB:-0}} — so only the comment disagrees with it. Worth fixing here rather than after merge, because the doc block is what a caller reads to decide what to set.

Everything I reviewed before stands: the signal trap ends the payload's tree rather than only the ticket (main's old trap 'exit 143' released the token and left the payload running — the 11-08 orphan through another door), the tree helpers are defined before the payload starts so a TERM in that window cannot run the trap with an undefined helper, and [ -n "${child:-}" ] leaves a still-queued hold leaving the queue quietly.

Still worth doing at some point, as I noted at the previous head: the trap calls kill_tree "$child" without a grace argument, so it takes kill_tree's default of 10 s and ignores GPU_HOLD_KILL_GRACE, unlike both watchdog paths.

[github-actions]

Persistent review updated to latest commit 6ce307c

[bong-water-water-bong]

VERDICT: READY at 6ce307ce — and I checked the one sentence in kill_tree that looked false, because if it were, the grace period would not exist. It is true; I was wrong, and the measurement is below so the next reader does not re-litigate it.

The parts that had to be right, and are

• descendants() prints the descendants and then itself ("deepest last"), so kill_tree "$child" ends the payload, not just the
  payload's children. That is precisely the shape pkill -P gets wrong, and it is the difference between this fix and gpu-hold: a signal to the hold ends the payload before it releases the token (Fixes #1640) #1642's guard: a
  kill -TERM <gpu-hold pid> signals gpu-hold, and the payload is only reachable through this recursion.
• Ordering: TERM trap → kill_tree → exit 143 → EXIT trap → release(). So the token is released after the tree is dead, which is
  the leak (gpu-hold: a signal to gpu-hold releases the token and leaves the payload running (trap exit 143 never ends the tree) #1640) closed at the level where it happens rather than by widening a grace period.
• kill_tree defined before "$@" & — the comment says why and it is the right reason: a TERM in the child=$! window would otherwise
  run the trap with the helper undefined, declare -F skips the kill, and the token is released with the payload alive. Moving a definition
  is a cheap way to delete a race.
• The queued arm of the test is the one most fixes would skip: it writes a foreign TOKEN (printf 'someone-else %s\n' "$$") and
  asserts that a TERM to a queued hold leaves the queue and leaves that token alone. "Does not delete what it never took" is the failure
  mode of every release path, and here it is an assertion rather than a comment.

The sentence I thought was wrong

# … `kill -0` with a pid list is true while ANY of them lives, so a wrapper that exits promptly no longer
# denies its children the grace they were promised.
while kill -0 $tree 2> /dev/null && [ "$i" -lt "$grace" ]; do sleep 1; i=$((i + 1)); done

My first reading was that kill -0 a b fails as soon as any pid is gone, so the grace would end the moment the wrapper (usually first to
die) exited and the children would be SIGKILLed immediately — i.e. the comment asserted the opposite of its own mechanism. I built the
discriminating fixture: an outer shell that dies on TERM at once, an inner that traps TERM, sleeps 3 s, writes a marker, then exits, with
grace=5. If the grace were denied, the marker could never appear.

tree=[1718268 1718269]     kill_tree took 3064 ms     RESULT: inner ran its TERM cleanup => grace honoured

Bash's builtin kill -0 on a pid list succeeds while any target exists, exactly as the comment says, so the loop runs until the last
member is gone and a well-behaved child keeps its whole grace. Retracting my objection in the PR rather than leaving it for the next
reviewer to raise: the comment is right, and it is worth more than the line of code it explains.

One traded-away property, worth recording rather than discovering

The snapshot is taken once, before any signal. That closes the reparenting hole (a TERM-ignoring process whose parent exits is re-linked to
init, so a second ps --ppid walk cannot find it) — the right trade for the 2026-11-08 shape, where the orphan holding 51 GiB GTT was two or
three levels down. The cost is that a process forked after the snapshot is invisible to both passes, so a runner that spawns a helper while
dying can still leave one. That is a much narrower hole than the one being closed, and it is the kind that a comment can hold; the paragraph
already explains the reparenting side, so one clause for this side would make the trade complete.

What I did not run

The PR's own tests/gpu_hold_tree_test.sh end to end (it needs the hold, a queue dir and the thermal flag file — my fixture above was a
hand-made cut of kill_tree and descendants copied verbatim from the head, which is the part that decides the verdict). No GPU ticket was
taken: the whole check is bash, two sleeps and one marker file.