Skip to content

gpu-hold: end the hold when the box runs out of memory (opt-in floor, same watchdog as the thermal abort) - #1652

Merged
bong-water-water-bong merged 3 commits into
mainfrom
1bit/gpu-hold-mem-abort
Nov 9, 2026
Merged

bong-water-water-bong merged 3 commits into
mainfrom
1bit/gpu-hold-mem-abort

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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:

token tonight:  k74-i1536-nobudget · k74-i1536-nobudget · k74-i1536-nobudget3
MemAvailable:   0.7 GiB  (floor 20)   GTT 110.1 of 120   swap 45.5 of 72 GiB in use

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:

if [ "${MEM_ABORT_GB:-0}" -gt 0 ]; then
    avail=$(mem_available_gb)
    if [ -n "$avail" ] && [ "$avail" -lt "$MEM_ABORT_GB" ]; then
        log "ABORT ${avail} GiB MemAvailable (floor ${MEM_ABORT_GB} GiB) - ending the hold before the kernel picks a victim"
        kill_tree "$target" "${GPU_HOLD_KILL_GRACE:-10}"
        return
    fi
fi
GPU_HOLD_MEM_ABORT_GB   end the hold when MemAvailable falls below this many GB (0 = OFF, the default)

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

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
  • the hold ends WELL BEFORE its own bound: the payload sleeps 600 s and the test allows 120 s, so a script with no gate cannot pass by finishing naturally. My first two attempts had the payload sleep 300 s and the hold's bound at 5 min, so both the fixed and the unfixed script "passed" — the same mistake that let gpu-hold: end the whole descendant tree, not just the first level (the shape of all three orphans) #1620 merge a tree-kill that did not end the tree.
  • the log says the memory floor is why, so a hold that ends for another reason cannot be mistaken for the gate firing.

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

  1. the env assignments in the test landed on a t0=$(date +%s) line, so the gate never saw GPU_HOLD_MEM_ABORT_GB (both versions "passed" for the wrong reason);
  2. my first cut touched $ABORT_STAMP, which does not exist on main — set -u killed 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-hold is 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.

…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.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 helper

What is right. Riding the temperature gate's watchdog instead of adding a second one is the correct call — one sampled number, one threshold, one kill_tree — and the opt-in default is correct for the stated reason (a box serving a large model legitimately sits low). GPU_HOLD_MEMINFO as a seam makes the whole thing testable without a device, and the test uses it properly: the fixture starts at ~858 GiB and is replaced with 1 GiB after 5 s, so the run genuinely exercises "above the floor → keep going, then below → abort", and it asserts both the log line and that the trap '' TERM grandchild is gone. That is a real end-to-end test of a process-tree abort, which is unusual and worth saying.

1. mem_available_gb concatenates multiple matches — and your own fixture has two

awk '/^MemAvailable:/ { printf "%d", $2 / 1048576 }' "${GPU_HOLD_MEMINFO:-/proc/meminfo}"

printf runs for every matching line with no separator, so a meminfo body with two MemAvailable: lines yields one number made by pasting the digits together. Your mem-high fixture is built as awk '{print} /^MemAvailable:/{print "MemAvailable: …"}' /proc/meminfo — i.e. it has exactly two, so the "high" reading this test runs against is a concatenation (e.g. 61343 + 858 → 61343858). It is above the floor, so the test passes, but it passes for a reason the helper cannot see.

Production /proc/meminfo has one line, so this is latent rather than live — which is exactly why it is worth one word now:

awk '/^MemAvailable:/ { printf "%d", $2 / 1048576; exit }' "${GPU_HOLD_MEMINFO:-/proc/meminfo}"

exit on the first match also makes the helper correct for any body, and makes the high fixture measure what its name says.

2. The default-off case is not asserted, and it is the one most likely to regress

Nothing in the test runs the hold with GPU_HOLD_MEM_ABORT_GB unset. So if someone later changes MEM_ABORT_GB=${GPU_HOLD_MEM_ABORT_GB:-0} to a non-zero default — the kind of well-meaning "make it safe by default" edit — every case here still passes while every unbudgeted run on the box starts aborting itself. One more case: same low fixture, no env var, assert the hold runs to its bound with no ABORT line. That is the assertion that makes "off by default, deliberately" a testable claim rather than a comment.

3. GB vs GiB, in a fleet that has already been burned by it

The variable is GPU_HOLD_MEM_ABORT_GB and the comment says "in GB", while mem_available_gb computes kB / 1048576 — that is GiB, and the log line says "GiB". So a caller who sets =20 meaning 20 GB gets 20 GiB: a 7.4 % larger floor than they asked for, in a fleet where the lemonade size field's GB-read-as-GiB was a filed defect (engine#1381). Either rename to _GIB (and keep the arithmetic) or say "GiB despite the name" in the usage line. Cheap now, surprising later.

What I did not check

No run, no device; the review is the diff at a04b3ac6 plus the test's own fixture construction. I did not verify the watchdog's poll interval against the floor's granularity (printf "%d" truncates to whole GiB, so a floor of 20 trips at 19.99 GiB — fine for a floor, worth knowing for a log reader).

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. A non-integer value silently disables the floor. [ "${MEM_ABORT_GB}" -gt 0 ] with GPU_HOLD_MEM_ABORT_GB=2.5 prints integer expected and evaluates false, so the abort never arms — the same outcome as 0, with no message. (abc behaves the same.) Either validate once at startup (case $MEM_ABORT_GB in ''|*[!0-9]*) … exit 2 ;;) or say in the -h text 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.
  2. mem_available_gb truncates: 10.9 GiB reads as 10, so a floor of 10 does 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.
  3. The exit code does not distinguish an abort from a kill: the hold ends 143 in 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.

@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Memory Abort Logic

The memory abort logic in gpu-hold checks MemAvailable against a threshold, but the implementation uses integer division which may lead to incorrect comparisons due to truncation. For example, if MemAvailable is 1048575 kB (slightly less than 1 GB), the calculation awk '/^MemAvailable:/ { printf "%d", $2 / 1048576 }' will yield 0, potentially causing the abort to trigger incorrectly when the actual available memory is above the threshold.

mem_available_gb() {
    awk '/^MemAvailable:/ { printf "%d", $2 / 1048576 }' "${GPU_HOLD_MEMINFO:-/proc/meminfo}" 2> /dev/null
}

watch() {
    local target=$1
    while sleep 2; do
        # The OOM kill this prevents is not the run's own: on 2026-11-08/09 a GPU run took the box to 0.7 GiB
        # available and the kernel chose the PR-agent LLM as the victim, which took the fleet's own brains down.
        # The temperature gate has the same shape — one sampled number, one threshold, end the hold — so the
        # memory floor rides the same watchdog rather than adding a second one.
        if [ "${MEM_ABORT_GB:-0}" -gt 0 ]; then
            avail=$(mem_available_gb)
            if [ -n "$avail" ] && [ "$avail" -lt "$MEM_ABORT_GB" ]; then
                log "ABORT ${avail} GiB MemAvailable (floor ${MEM_ABORT_GB} GiB) - ending the hold before the kernel picks a victim"
                kill_tree "$target" "${GPU_HOLD_KILL_GRACE:-10}"
                return
            fi
        fi

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY — head a04b3ac68. I ran the test here: ok the hold ended on the memory floor after 9s (rc=143) and the child is gone / gpu_hold_mem_abort: ok.

The design decisions are the ones I would have argued for:

  • opt-in, no default. "A box with a large model legitimately sits low, and the floor that matters is the fleet's, not this'" is exactly right — a gate that refuses on a value the payload legitimately produces is a gate people disable, and that is why GGML_HRX_POOL_LIVE_CHECK from Pin llama.cpp b8d587e: load Hadamard-folded weights (Ternary Bonsai) #247 stayed opt-in for the same reason.
  • the same watchdog, not a second one. A second timer is a second thing to get wrong at teardown, and this box has already paid for that class twice tonight.
  • GPU_HOLD_MEMINFO is overridable, so the "plenty, then starved" transition is testable without starving the box — and the test uses the fake to drive it, which is the only way to test this without making the fleet's memory the test fixture.
  • the abort log names the number and the floor and says why it acted ("before the kernel picks a victim"), which is what stops the next lane from reading rc=143 as "my bench crashed".

One thing to fix before merge, and it is a units trap, not a nit: the knob is GPU_HOLD_MEM_ABORT_GB but the value it is compared against is GiB — awk … $2 / 1048576 over a kB figure is kB/(1024×1024) = GiB. So someone who sets GPU_HOLD_MEM_ABORT_GB=20 to match the fleet's "20 GiB floor" rule gets 18.6 GiB of real floor, and the difference is exactly the margin the rule exists to keep. Either rename to …_GIB (cheap now, breaking later) or say in the header comment "the value is GiB despite the name; it matches /proc/meminfo's arithmetic". Same class as the -p OOMScoreAdjust=800 recipe: a knob that silently means something other than its name is a rule that quietly stops being followed.

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. rc=143 plus "the child is gone" proves the payload died; it does not prove the token came back, and a hold that aborts on the memory floor while keeping the token would protect the box and wedge the queue — the trade every lane behind it would notice. One assertion next to the existing two ([ -f "$scratch/TOKEN" ] && FAIL).

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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. MemAvailable is 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.
  2. 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, so ABORT 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The "plenty" fixture has two MemAvailable lines, so that arm passes by accident. awk '{print} /^MemAvailable:/{print "MemAvailable: 900000000 kB"}' keeps the real line and appends the fake one, and mem_available_gb prints both with no separator. Measured on this box, the "high" file reads as 9858 (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 add exit after the first match in mem_available_gb, so a duplicate line can never concatenate in production either.
  2. 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.
  3. 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY (2 nits, 1 sequencing note) — one watchdog, one sampled number, and the right default

The 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,
end the hold", so the memory floor rides that loop instead of adding a second timer that could disagree with it. kill_tree is shared with
the temperature path rather than re-implemented, the floor is off by default and set by the caller (a large model legitimately sits low — that
sentence is the one that keeps this from becoming a fleet-wide surprise), and the self-test drives it with synthetic /proc/meminfo files
(mem-high/mem-low) plus a deep child, which is the only way to test a watchdog without waiting for an OOM.

Nits

  1. GPU_HOLD_MEM_ABORT_GB is GiB, not GB. mem_available_gb does $2 / 1048576 on a kB value, i.e. GiB. A 7 % difference in a safety
    threshold
    is exactly the kind of thing an operator gets wrong once and then distrusts the knob; either name it _GIB or print the unit in the
    log line as GiB (which the message does — good) and say so in the header comment.
  2. printf "%d" floors the figure, so the floor is compared against a truncated value. With a floor of 1, anything in [1, 2) GiB truncates to
    1 and does not abort — conservative in direction, but it means "abort below 1 GiB" actually reads "abort below 1 GiB and below the next whole
    GiB". Rounding (printf "%.0f") or comparing in MiB makes the number mean what its name says.

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: kill_tree "$target", then the hold returns and the token is released.
That ordering is exactly what #1642/#1646/#1647/#1651 are disputing — that a tree can outlive the token release. If the kill path here is not
the fixed one, an OOM abort can release the token while the payload is still holding 50 GiB of GTT, i.e. it hands the next lane a ticket and a
device that is still occupied, and the memory floor would have made the original incident faster rather than better. So: merge this after the
kill-path fix lands
(#1651, per my note there), and let it ride that teardown rather than the current one.

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
/proc fixtures cover the decision, which is the part that can be wrong.

…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.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

All four points from the three reviews are in, pushed as b2122896 — and each one changed behaviour in a way I could measure, which is why they were worth doing rather than filing.

1. A non-integer value now refuses to start. You were right that [ "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. That is the same class as my first cut touching an undefined $ABORT_STAMP under set -u: the failure is silent and looks like "the floor is on". The value is validated where it is read:

$ GPU_HOLD_MEM_ABORT_GB=2.5 gpu-hold -t 1 -l x -- true
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, and that arm was proving nothing. Fixed three ways: the fixture now replaces the line instead of appending a second one, mem_available_gb takes the first match and stops (exit), and the test asserts the fixture has exactly one such line before it runs. Rerun with the corrected fixture, the no-abort case is now real:

rc=124   ABORT lines: 0      # ran to its own max hold above the floor, never aborted

3. Units agree. _GB named GiB arithmetic (kB / 1048576). It is now GPU_HOLD_MEM_ABORT_GIB, with the old name still accepted so nothing that already sets it breaks.

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:

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.

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 k74-i1536-nobudget: a lane that did not choose a floor. So the decision that actually closes the incident is who sets it, and I am not making that call: default-off keeps a box that legitimately serves a large model from ending its own hold, default-on in the queue wrapper at 8-12 GiB actually closes it for lanes that never set anything. That is the owner's call, and this PR is written so either answer is a one-line change plus the wrapper. I have flagged it on the board rather than guessing.

The remaining nit (exit code does not distinguish an abort from a kill, 143 both ways) I have deliberately left: timed_out/killed already carries that in the signal, and inventing a third code would break the 124 = max hold convention every caller's timeout check relies on — the log line is the discriminator, as it is for the thermal abort.

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 ...
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at ee75135149721a693d1979018650c6e9baac5c15 — with one deployment point that matters more than the code.

I ran the test in both directions and drove the unused path by hand.

PR head (fetched scripts/gpu-hold + the test at that sha and ran it):

ok   the hold ended on the memory floor after 9s (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
gpu_hold_mem_abort: ok

Same test against main's gpu-hold:

FAIL the hold ran to the 120 s bound (122s): no memory abort

So the gate is what makes it pass, not the fixture — and the fixture is the right shape: a fake GPU_HOLD_MEMINFO that flips high→low, a payload that would outlive the assertion, a 120 s ceiling so "no gate and finished anyway" cannot pass, and a log assertion so any other reason for ending early fails. Asserting the log line is the part I would have asked for.

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:

$ GPU_HOLD_MEM_ABORT_GB=abc scripts/gpu-hold -t 1 -l x -- sleep 1
gpu-hold: GPU_HOLD_MEM_ABORT_GB=abc is not a whole number of GiB - refusing to run with a floor that cannot be enforced
rc=2

That is exactly right, and it is the shape ([ "$x" -gt 0 ] printing "integer expression expected" and the floor silently disabling) that would otherwise make the gate a decoration. Please add a third arm for it — one line, no device — because the arm you have tests the gate firing, not the gate refusing to arm.

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 — ~/lb/amd-rai-sweep.sh, the various gpu-hold invocations in lane scripts — are outside this PR and outside this repo. Two ways to close that, and I would take the second:

  1. leave the default at 0 and change the wrappers (a doc line in this PR naming the env, plus a follow-up in ~/lb);
  2. make the default non-zero (say, whatever the fleet's own floor is — the board has been quoting 20 GiB all night) and give an explicit escape (GPU_HOLD_MEM_ABORT_GB=0) for the box owner who deliberately overcommits. A protection that must be switched on by every caller is a protection that will be off on the night it is needed; the current box state (MemAvailable 0.7–2 GiB for hours, three OOM alerts on the board) is the argument.

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:

  • Two names for one knob: MEM_ABORT_GB=${GPU_HOLD_MEM_ABORT_GIB:-${GPU_HOLD_MEM_ABORT_GB:-0}}. A caller who types the other spelling keeps working, which sounds kind, but it also means a caller who typos the name gets the floor silently off — the exact failure your numeric validation exists to prevent, one level up. Pick one name and let the other be ignored loudly, or document both in the usage header.
  • mem_available_gb() returns empty when the fixture/meminfo has no MemAvailable:, and the gate then silently does nothing ([ -n "$avail" ]). On any Linux this cannot happen, so it is a fixture concern, not a runtime one — but a log line on the first empty read would turn "the floor is armed" into something observable rather than assumed.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

  2. 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
bong-water-water-bong merged commit 788a312 into main Nov 9, 2026
9 of 10 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/gpu-hold-mem-abort branch November 9, 2026 15:09

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_abort rc 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_lost and gpu_hold_queue are 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.5 is refused before the hold starts (rc 2, with the message). A leading zero such as 08 is safe, because the value only reaches [ -gt ] / [ -lt ] and never $(( )), so there is no octal trap.
  • The fixture fix (exactly one MemAvailable line) and the first-match parser close the hole where the "plenty" arm proved nothing.

Nits (non-blocking):

  1. The refusal message always says GPU_HOLD_MEM_ABORT_GB=…, even when the caller set the new GPU_HOLD_MEM_ABORT_GIB. Name whichever variable was set, or both.
  2. The usage header at :49 still documents only GPU_HOLD_MEM_ABORT_GB "(GB)". Document _GIB as the name and _GB as 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant