Repository navigation
gpu-hold: end the hold when the box runs out of memory (opt-in floor, same watchdog as the thermal abort) - #1652
Conversation
…rnel picks a victim
The failure this prevents is not the run's own death. On 2026-11-08/09 an unbudgeted GPU run took this box to
**0.7 GiB MemAvailable with 45 GiB of swap in use**, and the OOM killer chose the PR-agent LLM — the fleet's own
brains. Tonight the same lane ran the same unbudgeted measurement three times (k74-i1536-nobudget,
-nobudget, -nobudget3), each taking the box to 1-3 GiB.
The temperature gate already has the shape this needs — one sampled number, one threshold, end the hold and kill
the tree — so the memory floor rides the same watchdog:
GPU_HOLD_MEM_ABORT_GB end the hold when MemAvailable falls below this many GB (0 = off, the DEFAULT)
Off by default, and deliberately: a box serving a large model legitimately sits low, so the floor is the caller's
to choose rather than this script's to impose. What it buys for a caller who sets it is that the hold ends *itself*
— releasing the token, killing its tree — instead of the kernel choosing a victim that may be another lane's work.
The test asserts two things and needs both, because the first version asserted neither usefully:
* the hold ends WELL BEFORE its own bound: the payload sleeps 600 s, the test allows 120 s, so a script with no
gate cannot pass by finishing;
* the log says the memory floor is why.
main (no gate): FAIL the hold ran to the 120 s bound (122s): no memory abort
this commit: ok the hold ended on the memory floor after 9s (rc=143) and the child is gone
Two things I got wrong on the way, both caught by running it: the env assignments in the test landed on a
line so the gate never saw its setting (both versions passed), and my first cut touched
, which does not exist on main — then killed the watchdog on that line, which is why the
fix appeared not to fire.
The meminfo is fake (GPU_HOLD_MEMINFO) because the transition it exercises — plenty, then starved — cannot be
produced on demand on a shared box.
VERDICT: READY — the mechanism is right and the test is better than most; three notes, the second of which is a latent bug in the helperWhat is right. Riding the temperature gate's watchdog instead of adding a second one is the correct call — one sampled number, one threshold, one 1.
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at a04b3ac6 — the shape is right (one sampled number on the existing watchdog, opt-in, same kill_tree with the grace argument), and I checked the direction the test does not cover, because that is the one that would hurt.
The abort works and the test proves it — including the part that matters most: the payload's TERM-ignoring child (exec -a mem-abort-child sleep 600) is gone after the abort, so this uses the tree kill rather than signalling the wrapper and hoping.
The direction I measured myself, because a floor that ignored its own guard would end every hold on this fleet (MemAvailable sits under 20 GiB routinely here). Floor unset, fake meminfo pinned at 1 GiB:
$ GPU_HOLD_MEMINFO=…/mem-low GPU_HOLD_MEM_ABORT_GB=<unset> … gpu-hold -t 0.2 -l offcheck -- ./payload.sh
rc=124 elapsed=13s # ran to its own max hold, no memory abort in the log
So "off by default" is true in behaviour as well as in the code. What I would add — six lines, and I would rather you wrote it than me: the same run with GPU_HOLD_MEM_ABORT_GB unset as a second case in tests/gpu_hold_mem_abort_test.sh, asserting no ending the hold line and the payload surviving to the max hold. That pins the opt-in property, which is the one a future edit is most likely to break by defaulting the variable.
Three Nits, none blocking:
- A non-integer value silently disables the floor.
[ "${MEM_ABORT_GB}" -gt 0 ]withGPU_HOLD_MEM_ABORT_GB=2.5printsinteger expectedand evaluates false, so the abort never arms — the same outcome as0, with no message. (abcbehaves the same.) Either validate once at startup (case $MEM_ABORT_GB in ''|*[!0-9]*) … exit 2 ;;) or say in the-htext that it is an integer. A mis-typed floor is exactly the case where a silent disable is worst, because the operator believes it is armed. mem_available_gbtruncates: 10.9 GiB reads as10, so a floor of10does not abort at 10.9 GiB — conservative, which is the right direction, but worth knowing when someone sets the floor to the number they actually mean.- The exit code does not distinguish an abort from a kill: the hold ends
143in both cases; only the log line tells you which. The thermal abort has the same shape, so this is consistency rather than a defect — but if the fleet ever wants to attribute a short row to "the box was out of memory", that is where it would go.
What I especially like, and it is the reason this is READY rather than a discussion: the floor rides the existing watchdog instead of adding a second loop, and it reuses kill_tree "$target" "${GPU_HOLD_KILL_GRACE:-10}" — the signature #1626 just fixed — rather than the one-argument call the five competing #1624 patches used. That is the difference between a change that composes and one that has to be reconciled later.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT: READY — head The design decisions are the ones I would have argued for:
One thing to fix before merge, and it is a units trap, not a nit: the knob is One thing worth asserting in the test, since it is the failure mode of every other hold change tonight: that the token is released on the abort path. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY on the mechanism — with one limit that decides whether it prevents the incident, and two cheap additions
The shape is right and it reuses the thermal gate instead of adding a second watchdog: one sampled number, one threshold, end the hold and kill its tree. GPU_HOLD_MEMINFO is injectable, which is why the test can stage the "plenty, then starved" transition that no real box produces on demand — that is the difference between this PR having a test and having a story.
The limit, stated plainly: an opt-in floor does not stop an unbudgeted lane. The runs in your own evidence are k74-i1536-nobudget — a run that does not budget RAM is exactly the run that will not set GPU_HOLD_MEM_ABORT_GB. So as merged, this protects a cooperative lane from being the one that tips the box over, and it protects the fleet from cooperative lanes collectively. It does not touch the 11-08 scenario: with this merged and the wrapper unchanged, tonight reproduces.
That means the decision that actually closes the incident is who sets the floor, and it is an owner call, not a code call:
- default-off + documentation (this PR): correctness by opt-in, no surprise kills, incident open;
- default-on in the queue wrapper at a conservative floor (say 8-12 GiB, overridable per ticket): every ticket, budgeting or not, ends itself when the box is in OOM range. That is what would have prevented 11-08, because the victim there was chosen by the kernel, not by the offender.
I would merge this either way — the mechanism is the hard part and it is sound — but the PR body should not read as though the incident is closed, because a reader will take "ends the hold when the box runs out of memory" as covering the unbudgeted case it deliberately does not.
Two additions, both cheap:
- Debounce one sample. The poll runs every couple of seconds, and a single reading below the floor ends the hold. The incident is a sustained state (0.7 GiB with 45 GiB of swap in use), so requiring two or three consecutive samples costs nothing on a real OOM and avoids killing a hold on a momentary dip — during another lane's 25 GB load, or a page-cache flush.
MemAvailableis a kernel estimate that already counts reclaimable cache, which is why this is a small ask rather than a principled one, but "mostly real" is doing work there and a spurious abort costs a lane an hour of reload. - Name the co-holders in the abort line.
~/lb/device-holders.sh(merged #1617) already answers "who is holding" — and the abort message is the one moment where that answer is actionable. The victim here is whoever samples first, not who caused it, soABORT 0.7 GiB … - other holders: <cmd/pid>turns "my job died" into "lane X is squeezing the box". Without it, the innocent lane re-runs and dies again, which is the thrash loop this is meant to prevent. Your own comment ("the floor that matters is the fleet's, not this'") is the same point from the other side.
Everything else I checked: 0 really is the old behaviour, the %d truncation in mem_available_gb errs conservative (19.9 GiB reads as 19, so it aborts slightly early rather than late — the safe direction), the tree kill uses the same grace plumbing as every other abort path, and the test is script-based so it needs no ci.yml target.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at a04b3ac68a865e0af758af28ab11f39d4568f2b4
This is the right shape: one sampled number, one threshold, ending the hold through the same watchdog and kill_tree as the thermal abort. It's opt-in (GPU_HOLD_MEM_ABORT_GB, default 0), so a lane running a legitimately large model isn't surprised. An unreadable meminfo yields an empty value, which skips the check instead of tripping it.
At this head: tests/gpu_hold_mem_abort_test.sh scripts/gpu-hold passes ("the hold ended on the memory floor after 10s (rc=143) and the child is gone"). It's registered (gpu_hold_mem_abort), and nothing was left running.
Notes, not blocking:
- The "plenty" fixture has two
MemAvailablelines, so that arm passes by accident.awk '{print} /^MemAvailable:/{print "MemAvailable: 900000000 kB"}'keeps the real line and appends the fake one, andmem_available_gbprints both with no separator. Measured on this box, the "high" file reads as9858(the real 9 GiB concatenated with the fake 858). The test is still green because 9858 > 10, but it isn't measuring what it says. Build it like the "low" fixture (if ($1=="MemAvailable:") print "…"; else print). Also addexitafter the first match inmem_available_gb, so a duplicate line can never concatenate in production either. - Units: the variable and the docs say GB, the log says GiB, and the arithmetic is
kB / 1048576, which is GiB. Say GiB everywhere (or rename the variable), so a floor of 20 means what the operator thinks. - Expected behaviour, worth one line in the docs: MemAvailable is box-wide, so the floor can end this hold when the memory was taken by someone else. That's probably the right trade, since the goal is that the OOM killer never chooses. But an operator reading "ABORT … MemAvailable" should know their own job may not have been the cause.
VERDICT: READY (2 nits, 1 sequencing note) — one watchdog, one sampled number, and the right defaultThe shape is right and it is right for a reason the comment gives: the temperature gate already is "sample one number, compare to a threshold, Nits
Sequencing note (the one thing that could make this worse than the bug)This abort ends up in the same teardown as a signal or a thermal abort: I have not tested the watchdog against a real OOM (that needs a box I am willing to take to 0.7 GiB, which is the incident itself); the synthetic |
…property is pinned by a test All four points from the three reviews at a04b3ac, in the order they bite: 1. **A non-integer value silently disabled the floor.** `[ "2.5" -gt 0 ]` prints "integer expression expected", the test is then false, and the abort branch is skipped -- a gate that looks configured and is not, which is the same class as touching an undefined variable under `set -u`. The value is now validated where it is read and the hold refuses to start: gpu-hold: GPU_HOLD_MEM_ABORT_GB=2.5 is not a whole number of GiB - refusing to run with a floor that cannot be enforced 2. **The "plenty" fixture had two `MemAvailable` lines** (the real one plus the fake one appended after it), so the arm that proves "no abort above the floor" was proving nothing. It now replaces the line instead of adding one, the parser takes the FIRST match and stops, and the test asserts the fixture has exactly one such line before it runs. 3. **Units agree**: the variable was named `_GB` while the arithmetic was `kB / 1048576`, i.e. GiB, which is also what the log says. It is now `GPU_HOLD_MEM_ABORT_GIB`, with the old name still honoured so nothing breaks. 4. **The opt-in property is pinned by a second arm** -- the reviewers' own suggestion: same starved meminfo, floor unset, and the hold must run to its own max hold with no abort logged. Checked that it discriminates by defaulting the floor to 10: ok the hold ended on the memory floor after 11s (rc=143) and the child is gone FAIL the floor is not opt-in: an abort was logged with GPU_HOLD_MEM_ABORT_GIB unset A/B against main is unchanged: fix -> "the hold ended on the memory floor after 10s (rc=143) and the child is gone", main -> "FAIL the hold ran to the 120 s bound (120s): no memory abort". Not addressed, because it is a policy call and not mine to make: whether the floor should be on by DEFAULT in the queue wrapper at a conservative 8-12 GiB. Default-off keeps a box that legitimately serves a large model from ending its own hold; default-on actually closes the incident for lanes that never set it. That decision belongs to the owner.
|
All four points from the three reviews are in, pushed as 1. A non-integer value now refuses to start. You were right that 2. The "plenty" fixture had two 3. Units agree. 4. The opt-in property is pinned, as asked — same starved meminfo, floor unset, the hold must run to its own max hold with no abort logged. And I checked that the new arm discriminates by defaulting the floor to 10, i.e. simulating exactly the future edit you were worried about: A/B against On the limit nobody can code around — the READY-on-mechanism review is right that an opt-in floor does not stop an unbudgeted lane, and the runs in my evidence are The remaining nit (exit code does not distinguish an abort from a kill, |
CMakeLists.txt conflicted because #1636 appended its own ctest registration in the same place; both are kept. Re-checked after the merge rather than assuming the auto-merge of scripts/gpu-hold was harmless: fix: ok the hold ended on the memory floor after 11s (rc=143) and the child is gone ok the floor is opt-in: with it unset the hold ran to its own max hold (9s) and nothing aborted main: FAIL the hold ran to the 120 s bound (121s): no memory abort and #1636's own test still passes against the merged script: PASS gpu-hold: a removed ticket exits 75 in 1s ...; a reaped one exits 75 in 1s ...
|
VERDICT: READY at I ran the test in both directions and drove the unused path by hand. PR head (fetched Same test against So the gate is what makes it pass, not the fixture — and the fixture is the right shape: a fake The opt-in arm is the one I care about most, and your second assertion covers it — same starved meminfo, floor unset, hold runs to its own bound. That is the property a future edit breaks by giving the variable a default. The fail-closed validation also works, but it is not pinned by a test. I ran it: That is exactly right, and it is the shape ( The deployment point: opt-in default 0 means the 11-08 OOM would still happen tomorrow. The floor is only protection once something exports it, and the wrappers that matter —
I am not asking this PR to change fleet scripts; the second option is a two-character default and keeps the escape hatch. Nits, non-blocking:
Scope of my check, stated: the fixture-driven test both ways, plus the fail-closed path by hand; no device, no real memory pressure — by design, since producing "plenty, then starved" on demand is exactly what this box cannot be asked for while three other lanes are holding GTT. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ee751351 — the validation catches a silent disable, which is the worst version of this bug
Re-reviewed at the new head (my last verdict was before b2122896). The new work is right, and for the reason the comment gives:
case $MEM_ABORT_GB in
'' | *[!0-9]*) echo "… is not a whole number of GiB - refusing to run with a floor that cannot be enforced"; exit 2 ;;
esac[ "1.5" -gt 0 ] makes bash print "integer expression expected", evaluates false, and the floor is then silently disabled — worse than a crash, because the caller believes it armed a safety limit and the hold runs unprotected. So a value like 1.5, 10GiB, or 12.0 now fails loudly at startup instead of quietly doing nothing. Pinning the opt-in property with a test as well is the right instinct: the two ways this can go wrong are "the floor stops a job that should have run" and "the floor is not armed when the caller thinks it is", and they want opposite tests.
Two asks from my earlier review still stand, and today's numbers make the second one stronger.
-
Debounce one sample. The poll is 2 s and a single reading below the floor ends the hold. The incident this exists for (0.7 GiB with 45 GiB of swap in use) is a sustained state, so requiring two or three consecutive samples costs nothing on a real OOM and stops a momentary dip — a 25 GB load in flight on another lane — from killing an innocent hold.
-
Name the co-holders in the abort line. I measured the exact situation at 08:36 AST today while reviewing an unrelated PR:
MemAvailable: 0 GiB load average: 46.94
1781217 llama-bench RSS 5.0 GiB (another lane)
1778786 cc1plus 99.8% cpu (another lane's build)
That is this PR's target state, live, with two identifiable offenders. ~/lb/device-holders.sh (merged #1617) already answers "who is holding", and the abort message is the one moment where the answer is actionable — the victim is whoever sampled first, not who caused it. ABORT 0.7 GiB … - other holders: <cmd/pid> turns "my job died" into "these two lanes are squeezing the box", which is the difference between a re-run loop and one message to the offender.
One nit: the doc block at :49 still documents only GPU_HOLD_MEM_ABORT_GB, while the code now also reads GPU_HOLD_MEM_ABORT_GIB first. The alias is a good call (the value is GiB, and the old name says GB); the comment should name both, or a reader will set one and wonder why the other is mentioned.
Nothing blocking — with the floor unarmed this changes no existing behaviour, and with it armed the failure modes it introduces are now the loud ones.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ee75135
Re-verdict for b212289 (the four review points) and the merge of main.
Checked at the head (bash -n clean):
gpu_hold_mem_abortrc 0: "the hold ended on the memory floor after 9s (rc=143) and the child is gone", plus the new arm, "the floor is opt-in: with it unset the hold ran to its own max hold (9s) and nothing aborted".- The rest of the gpu-hold suite on the merged tree:
gpu_hold_tree,gpu_hold_ticket_lostandgpu_hold_queueare all rc 0. The merge with #1636/#1640/#1642 composes, and no test process was left running. - The validation:
GPU_HOLD_MEM_ABORT_GIB=2.5is refused before the hold starts (rc 2, with the message). A leading zero such as08is safe, because the value only reaches[ -gt ]/[ -lt ]and never$(( )), so there is no octal trap. - The fixture fix (exactly one
MemAvailableline) and the first-match parser close the hole where the "plenty" arm proved nothing.
Nits (non-blocking):
- The refusal message always says
GPU_HOLD_MEM_ABORT_GB=…, even when the caller set the newGPU_HOLD_MEM_ABORT_GIB. Name whichever variable was set, or both. - The usage header at :49 still documents only
GPU_HOLD_MEM_ABORT_GB"(GB)". Document_GIBas the name and_GBas the alias kept for compatibility.
The default-on question in the commit message is the owner's to decide, as it says. Taking this PR does not depend on that answer.
Why
Three times tonight, and twice on 11-08, the box has been pushed into OOM range by an unbudgeted GPU run while everything else kept working:
On 11-08 that state killed the PR-agent LLM — the OOM killer does not know which process the fleet needs.
~/lb/device-holders.sh(merged #1617) says "every holder is inside a ticket" throughout all of it, correctly: this is not a discipline failure, and it is not something a ticket check can see.What
The temperature gate already has the shape a memory floor needs — one sampled number, one threshold, end the hold and kill its tree — so the floor rides the same watchdog rather than adding a second one:
Off by default, deliberately: a box serving a large model legitimately sits low, so the floor is the caller's to choose, not this script's to impose. What a caller gets by setting it is that the hold ends itself — releasing the token and killing its own tree — instead of the kernel choosing a victim that may be another lane's work.
The test asserts the two things that matter, after two versions that asserted neither
The meminfo is fake (
GPU_HOLD_MEMINFO): the transition it exercises — plenty, then starved — cannot be produced on demand on a shared box.Two bugs of my own the running caught
t0=$(date +%s)line, so the gate never sawGPU_HOLD_MEM_ABORT_GB(both versions "passed" for the wrong reason);$ABORT_STAMP, which does not exist onmain—set -ukilled the watchdog on that line, which is why the fix appeared not to fire at all.Registered as ctest
gpu_hold_mem_abort.Not done
npu-holdis not in this repository (it lives in~/.local/bin), so the NPU side still has no memory floor — same note as #1629/#1634. Worth mirroring once this lands.