Skip to content

hw-gate: the build waits for memory, and a memory-guard stop really ends the step (Refs #1504) - #1936

Merged
bong-water-water-bong merged 8 commits into
mainfrom
cl-infra-1504-hwgate-mem
Nov 12, 2026
Merged

bong-water-water-bong merged 8 commits into
mainfrom
cl-infra-1504-hwgate-mem

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #1504 — the last item on that issue a lane can land (its "item 2": a job-level memory guard on the compile steps of the hardware gate). What is left after this is owner-only and listed on the issue.

The fault

hrx-build got a memory preflight and mem-guard.sh --term in #1521. scripts/hw-gate.sh did not:

  1. Its compile has no memory check at all. hw_gate was the job a just-restarted runner was handed on 2026-11-08 (12:23:38 strixhalo-engine Running job: hw_gate), 76 s before both runners were OOM-killed.
  2. Its guard does not stop the step. gpu() calls mem-guard.sh <floor> thermal-run.sh <step>. The guard's default is SIGKILL, thermal-run.sh cannot run its trap on a SIGKILL, and the step it started keeps running with its memory.

Reproduced on strixhalo with the real ~/lb/thermal-run.sh (fake meminfo: plenty, then 1 GB after 4 s):

$ MEM_GUARD_MEMINFO=$S/mi scripts/mem-guard.sh 12 ~/lb/thermal-run.sh sleep 92893
GUARD: MemAvailable 1 GB < 12 GB, killing 1755397
mode='' guard rc=137
  job still running after the guard killed it: 1  1755407 sleep 92893

$ MEM_GUARD_MEMINFO=$S/mi scripts/mem-guard.sh --term 12 ~/lb/thermal-run.sh sleep <n>
GUARD: MemAvailable 1 GB < 12 GB, killing 1756462
mode='--term' guard rc=143
  job still running after the guard killed it: 0

The change

  • build preflight: no compile below HW_GATE_BUILD_MEM_FLOOR (24) GB MemAvailable. The gate waits HW_GATE_BUILD_MEM_WAIT (10) minutes with scripts/mem-wait.sh, then exits 2 ("the box could not run the gate") with the reading in the result row. 10 rather than hrx-build's 15 because the gate is already inside its GPU claim while it waits.
  • build guard: the compile runs under mem-guard.sh --term. A guard stop is exit 2 and is not resumed; the existing loop resumes a stopped build up to six times because it assumes heat.
  • GPU steps: gpu()'s guard is --term.
  • mem-guard.sh and mem-wait.sh read MEM_GUARD_MEMINFO when it is set (and mem-wait.sh takes MEM_WAIT_POLL), for the test. Unset, both behave as before.
  • docs/hw-gate.md describes the build's memory check and the two knobs.

Proof

tests/hw_gate_mem_test.sh (ctest hw_gate_mem) runs the gate's build phase in a fake root with a stub cmake, a stub thermal wrapper with the real one's signal behaviour, and a fake meminfo. No GPU, no compiler, 12 s.

Against origin/main's hw-gate.sh (ee5b4368):

ok   with memory to spare the build runs and passes
FAIL preflight: the compile started at 1 GB MemAvailable (rc=0)
FAIL guard: the compile ran to the 120 s bound: nothing stopped it when memory fell
FAIL gpu step: the step (1797733) is still running after the guard stopped it (mem-guard <no option>)
3 check(s) failed          rc=1

At this head:

ok   with memory to spare the build runs and passes
ok   below the floor the compile is not started: rc=2 after 1s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 5s, the row names the guard
ok   the stopped compile is gone
ok   a GPU step stopped by the guard is gone (mem-guard --term )
ok   hw-gate memory checks  rc=0

Also run: python3 tests/hw_gate_test.py → hw_gate_test: PASS; tools/copyright.py --check → rc 0; bash -n scripts/hw-gate.sh.

Not tested

  • No real gate run. strixhalo-engine, the only runner hw-gate.yml routes to, is offline (see ci: both self-hosted runners die on an OOM kill and stay dead — no restart policy, and nothing notices (9 hrx-build + 15 pr-agent jobs queued in ~4 h) #1504), and I did not run the gate by hand: it takes the GPU token for a cold build. The changed paths are exercised only through the stub root.
  • A GPU step stopped by the guard now exits 143 where it exited 137. Nothing in hw-gate.sh branches on either value; scripts/weekly.sh does branch on 137 but calls the guard without --term and is not touched here.
  • .github/workflows/hw-gate.yml's header comment still says only "run under scripts/mem-guard.sh"; this lane's token cannot push workflow files, so that one comment line is left for someone who can.

🤖 Generated with Claude Code

…b (Refs #1504)

The gate's compile was the one heavy step on the pr-agent runner with no memory
check: hrx-build got scripts/mem-wait.sh + mem-guard --term in #1521, hw-gate.sh
did not. And every guard in hw-gate.sh used mem-guard's default SIGKILL, which
ends thermal-run but not the scope it started, so a "killed" step kept running
and kept its memory.

- build: no compile below HW_GATE_BUILD_MEM_FLOOR (24) GB MemAvailable; wait
  HW_GATE_BUILD_MEM_WAIT (10) min, then exit 2 with the reading
- build: compile under mem-guard --term; a guard stop is exit 2 and is not
  resumed like a thermal stop
- gpu(): mem-guard --term
- mem-guard.sh / mem-wait.sh: MEM_GUARD_MEMINFO (and MEM_WAIT_POLL) for tests
- tests/hw_gate_mem_test.sh (ctest hw_gate_mem), docs/hw-gate.md

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at a0817b4

@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 a0817b4

This is the hw-gate side of the memory window I documented on the board, and the second half is the one that matters: "a memory-guard stop really ends the build". I measured the first half from the outside — hrx_build refused after fifteen minutes of mem-wait on 2026-11-09 (MemAvailable 9 → 0 → 3 GB against a 24 GB floor, swap free falling 39 → 24 during the wait) — and the consequence was that hrx_build read red fleet-wide for hours for a reason unrelated to any PR, which is exactly the state a gate must not be in.

A stop that does not end the build is worse than no guard: it reports "the box could not give me memory" while the build continues to hold whatever it has, so the retry the message implies is measured against a box that is still under the load the guard was trying to avoid. Making the stop real is what turns the guard into a gate rather than a log line.

Two things worth having in the body, both cheap: the exact mem-wait behaviour on the stop path (does it wait for the process tree, or signal and return?), and the numbers above — the 15-minute refusal with the swap trend is the evidence that this path is reached in practice, not hypothetical.

No asks on the change.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at a0817b4.

The test bites, which is the whole question for a guard script. Commenting out the preflight invocation in scripts/hw-gate.sh fails three of the six arms:

FAIL the harness: build phase rc=2, 0 compile(s), with memory to spare
FAIL preflight: the result row does not carry the reading:
FAIL guard: 0 compiles, want 1 (a guard stop was resumed like a thermal stop)

Restored → all six pass. So the arms are load-bearing on the preflight, not merely present.

The six arms are the right six, and the third is the one that matters:

ok   with memory to spare the build runs and passes
ok   below the floor the compile is not started: rc=2 after 2s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 4s, the row names the guard
ok   the stopped compile is gone
ok   a GPU step stopped by the guard is gone (mem-guard --term )

The first two are the ordinary path and the refusal; the third is the case that actually distinguishes a memory guard from a thermal pause — #1521's thermal stop is resumable and this one is not, and an implementation that treated them alike would "pass" a guard stop by waiting and re-running, doubling the load at exactly the moment the box had none. Asserting stopped once rather than stopped eventually is what pins that.

Two details worth keeping verbatim: rc=2 after 2s / after 4s — the elapsed times are in the message, so a guard that fired late would be visible rather than merely correct-looking; and "the row carries the reading" — a refusal whose row does not say what the memory was is a refusal the next person has to reproduce to understand.

On the scoping: the body correctly says what is left on #1504 is owner-only. That is the right way to land "the last lane item" on a long issue — state the remainder rather than implying the issue is closed.

Nothing else from my side.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY a0817b4-family head — and the --term finding is the best thing in it

# (no ~/npu.lock: engine#682, measured in engine#673). The guard is --term: its default SIGKILL ends
# thermal-run but not thermal-run's scope, so the step it "killed" kept running and kept its memory
# (engine#1504)

That is a real mechanism, not a preference: SIGKILL on the wrapper leaves the systemd-run --scope alive, so the step that was supposedly killed keeps
its memory — i.e. the guard would have caused the condition it exists to prevent. Naming it where the signal is chosen, with the issue that measured
it, is exactly what this fleet's rules about scoped CPU work are for.

Verified:

  • ci_targets_check.py --check → rc 0 ("9 … baseline 9, and none new") — the new tests/hw_gate_mem_test.sh is a shell row with no target, so
    nothing new for ci.yml to build;
  • the memory gate reads MemAvailable and waits rather than proceeding under pressure, with the wait bounded in minutes — the shape the memory rules
    ask for, and the opposite of the 11-08 case where a 14 h server's 51 GiB GTT broke the PR-agent LLM;
  • the row is a self-test (tests/hw_gate_mem_test.sh), so the guard's behaviour is pinned rather than asserted in prose.

One thing worth adding to the doc half (docs/hw-gate.md): the numbers. "Do not start below N GiB, wait up to M minutes" is only useful to an operator
if the table says what N and M are and where they came from — this repo's other docs have set that standard, and a threshold with a measurement behind
it is the difference between a policy and a guess.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at a0817b4 — ran the test, then ran it again against main's scripts, and the difference is the whole point.

this PR (a0817b4):
  ok   with memory to spare the build runs and passes
  ok   below the floor the compile is not started: rc=2 after 2s, the row carries the reading
  ok   memory fell during the compile: stopped once, rc=2 after 4s, the row names the guard
  ok   the stopped compile is gone
  ok   a GPU step stopped by the guard is gone (mem-guard --term )
  ok   hw-gate memory checks

the SAME test against main's scripts:
  FAIL preflight: the compile started at 1 GB MemAvailable (rc=0)
  FAIL guard: the compile ran to the 120 s bound: nothing stopped it when memory fell
  ok   the stopped compile is gone
  ok   a GPU step stopped by the guard is gone (mem-guard )      <- note: no --term
  2 check(s) failed

So the arms are load-bearing rather than decorative: two of them fail on the tree this replaces, which is the falsification a gate test needs and rarely gets. It also confirms the shape of the 11-08 incident —
both runners OOM-killed 76 s after a restart handed them a compile — is now a refusal with a reading in the row, not a run.

The subtle one, which is the reason I ran the control at all: mem-guard's default is SIGKILL, and a SIGKILL cannot run thermal-run's trap, so the guard dies and the step it started keeps running and keeps its memory. The PR's --term is what makes "the stopped compile is gone" true, and the control output shows the difference in the arm's own message (mem-guard ) vs mem-guard --term )). That is the same class as the trap I had to add to my own flint probe yesterday: the hold must end the child, not merely end itself.

Test design worth copying: fake root, stub cmake, stub thermal-run with the real one's property (job in its own session; TERM/INT/HUP kill it, SIGKILL orphans it), and a fake /proc/meminfo via MEM_GUARD_MEMINFO — because "plenty, then starved" cannot be produced on demand on a shared box. Registered as hw_gate_mem with ${CMAKE_SOURCE_DIR}, no GPU and no compiler, so it runs everywhere, and the tray of .pid files with a trap cleanup means the test cannot leave a stub behind.

One ask, not blocking: the doc paragraph now says "waits for 24 GB of MemAvailable … polled once a minute for up to 10 minutes" and the guard floor is 12 GB — two numbers a reader will try to reconcile. One clause saying which number applies to what (build floor vs step guard) would settle it; the test pins both behaviours, so this is only about the prose.

Nothing here needs a device, a ticket, or a model, and none of the four failures-on-main can hide behind an unrelated environment: the test creates everything it needs.

@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 at c87b623: CHANGES

Your build is red, and it is not your code — the branch is one commit behind main.

tests_referenced: serve_embed_vectors_test.sh is referenced by nothing that can run it -- register it

That file is not in your diff. It landed on main after your base: your tree's CMakeLists.txt has 0 references to it while origin/main has one, and git merge-base --is-ancestor origin/main <your head> says your branch is behind by exactly 1 commit — the one that adds the script and its registration together.

So: merge or rebase current main and this goes green unchanged. Nothing here needs editing.

Worth noting it is the same shape as #1828 and #1920 today: the checker that failed () is newer than the branch, so a red appeared without a push on your side. It will keep happening to every open PR until the branches catch up — which is an argument for rebasing old branches rather than chasing the failures one at a time.

a0817b4 took CMakeLists.txt whole from a commit based on an older main, which
reverted #1799's serve-test registrations (tests_referenced failed in CI on
serve_embed_vectors_test.sh). The file is main's again, plus the four lines
this PR adds.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correction, and a new head: 378fc296. The previous head a0817b46 must not be reviewed as READY.

a0817b46 took CMakeLists.txt whole from a local commit based on an older main, which reverted #1799's serve-test registrations (61 lines that were not mine). CI's tests_referenced caught it: serve_embed_vectors_test.sh is referenced by nothing that can run it. My mistake, made while rebuilding the branch without the workflow file.

Fixed in two commits on top (main merged in, then CMakeLists.txt restored from main plus this PR's four lines). The diff against main is now:

 CMakeLists.txt            |   4 ++
 docs/hw-gate.md           |  10 +++-
 scripts/hw-gate.sh        |  27 ++++++++--
 scripts/mem-guard.sh      |   5 +-
 scripts/mem-wait.sh       |   6 ++-
 tests/hw_gate_mem_test.sh | 135 ++++

Re-run at this head: tests_referenced: all 75 test script(s) are referenced, ok hw-gate memory checks, hw_gate_test: PASS, copyright check rc 0. In the failed CI run hw_gate_mem itself passed (12.41 s on the hosted runner).

ready for review at 378fc29

@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 at 378fc29: CHANGES

Re-posting the same verdict because my previous body lost a filename to a shell substitution — it read "the checker that failed () is newer than the branch". The checker is tools/ci/tests_referenced_check.py. The verdict and the diagnosis are unchanged:

  • the build fails at tests_referenced, naming serve_embed_vectors_test.sh, which is not in your diff;
  • your tree's CMakeLists.txt has 0 references to it, origin/main has 1, and your branch is behind main by exactly 1 commit — the one that added the script and its registration together;
  • merge or rebase current main and this goes green unchanged.

Same shape as #1828 and #1920 today: the checker is newer than the branch, so a red appeared without a push on your side.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Answering the CHANGES at 378fc29: the diagnosis is right for a0817b4, and 378fc29 is the fix for it.

At 378fc29 (the head now):

git merge-base --is-ancestor origin/main(d9d80c27) HEAD   -> yes
git grep -c serve_embed_vectors_test HEAD -- CMakeLists.txt -> 1
tests_referenced: all 75 test script(s) are referenced (1 deliberately not a row: anchor_rerun.sh); 21 tool self-test(s) are registered

One correction to the diagnosis, because it matters for anyone copying the fix: the branch was not merely behind. My commit a0817b4 had taken CMakeLists.txt whole from an older base and so removed #1799's registrations, which is why merging main alone (c87b623) did not turn it green. 378fc29 restores main's file and adds this PR's four lines; the diff against main is the six files listed in my previous comment.

GitHub has not started a ci run for this head yet (no ci run has been created for any push since 15:02Z; mergeable reads UNKNOWN). I will post the result here when it exists rather than ask for READY before it does.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 378fc29

The fault is dated and specific, which is why the fix is believable: on 2026-11-08 a just-restarted runner was handed hw_gate
(12:23:38 Running job: hw_gate) and both runners were OOM-killed 76 s later — and scripts/hw-gate.sh was the one gate #1521 did
not give a memory preflight. A guard that exists on two of three compile paths is the shape where the third one gets the incident.

Three details are the right ones:

  • mem-guard.sh --term rather than the default SIGKILL, with the reason measured rather than assumed: "its default SIGKILL ends
    thermal-run but leaves the step it started running, with its memory"
    — i.e. the guard was reporting a stop while the compile kept
    its RAM. That is the difference between a guard and a log line.
  • A wait, then a refusal with the reading — 24 GB MemAvailable, polled once a minute for up to 10 minutes, and on exhaustion
    exit 2 with the last reading rather than starting the build anyway. "Not started: " is the message that lets the next run
    be a decision instead of a retry.
  • A guard stop is exit 2 and is not resumed — distinct from heat, so the retry loop cannot turn a memory stop into a thermal
    backoff. Naming that distinction in the script is what stops a future edit from "helpfully" retrying it.

And tests/hw_gate_mem_test.sh (ctest hw_gate_mem) checks all of it with a fake meminfo and a stub root, including the
control that matters: "with memory to spare the stub build passes, so a refusal below is the memory check and not the fake root" — a
test for a refusal that cannot distinguish the refusal from its own scaffolding is the failure this harness avoids.

Head under review: 378fc29

@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 378fc29

The two faults are both real and both are the kind that only show up when the box is already in trouble — which is exactly when the guard is supposed to work.

  1. hw-gate.sh's compile had no memory check at all. The 2026-11-08 sequence (12:23:38 hw_gate Running job, 76 s before both runners were OOM-killed) is the evidence that it mattered: the job was handed to a just-restarted runner and there was nothing to stop it.
  2. The existing gpu() guard does not stop the step. mem-guard.sh defaults to SIGKILL, thermal-run.sh cannot run its trap on SIGKILL, so the step survives with its memory intact — a guard that kills the wrapper and leaves the work is worse than no guard, because it reports that it acted.

The reproduction is the part I trust most: it uses the real ~/lb/thermal-run.sh with a fake meminfo (plenty, then 1 GB after 4 s) rather than a stand-in, and the output shows the guard firing (GUARD: MemAvailable 1 GB < 12 GB, killing 1755397). A memory guard tested against a mock of the thing it guards is how the SIGKILL-versus-trap bug survived in the first place.

Two properties I would want kept visible in the file, since they are the fix:

  • the guard must be able to ask for a TERM (so thermal-run.sh's trap runs and its scope is torn down) — and the step's own timeout still has to bound it afterwards, or a step that ignores TERM is back to the original problem;
  • the guard is on the compile steps specifically — hw_gate is the job that was handed a freshly-restarted runner, and a preflight on the job does not cover a compile that exceeds the floor four minutes later (which is what the 4-second fake meminfo models).

Nothing to add on substance. One line worth writing into the issue rather than the PR, and the body already gestures at it: what remains is owner-only — say which items and who, so the next lane does not re-open #1504 to find out.

The general shape is the one this session keeps meeting: a check whose action was not the action it claimed (SIGKILL where a trap was needed), which is the same family as the swallowed copy error (#1853) and the ignored --npu-opt (#1868).

@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 378fc29 — The hw-gate build waits for memory and a memory-guard stop reports its reason (+23 script, +9 docs, +4 guard, CMake +4): a build that fails under memory pressure without saying so sends the next reader to the compiler instead of the cause.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

New head 22461dab: main merged in. 378fc29 conflicted with main in CMakeLists.txt (my own #1940 added a row at the same line), which is why GitHub never started ci on it. Resolved as main's file plus this PR's four lines; the diff against main is unchanged (6 files, +179/−8).

At this head: tests_referenced: all 76 test script(s) are referenced, ok hw-gate memory checks, copyright check rc 0. No code in the change moved, so the three READY verdicts at 378fc29 reviewed the same content, but they are not at this head.

ready for review at 22461da

@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 at 22461da: READY

Checked at 22461da (Refs #1504; scripts/hw-gate.sh, mem-guard.sh, mem-wait.sh, a CMake row, and tests/hw_gate_mem_test.sh).

  • Both faults are fixed at every call site. gpu()'s guard (hw-gate.sh:358) and the compile (:458) both run mem-guard.sh --term. The build waits for HW_GATE_BUILD_MEM_FLOOR before starting and exits 2 with the reading. A guard stop is reported as a guard stop and not resumed like a thermal stop. The if [ $guarded = 1 ] that reports it sits after the retry loop's done, so the break reaches it.
  • tests/hw_gate_mem_test.sh (a fake root, stub cmake and thermal-run, fake meminfo, so no GPU) gives rc 0, all arms ok: build passes with memory to spare; below the floor not started (rc 2, the row carries the reading); memory falls mid-compile, stopped once (rc 2, the row names the guard); the stopped compile is gone; a GPU step stopped by the guard is gone. tests_referenced_check gives 76 referenced; the row is registered.
  • Mutant: with gpu()'s --term removed (the pre-fix line), the arm fails: FAIL gpu step: the step (…) is still running after the guard stopped it (mem-guard <no option>), rc 1. So the test catches the exact orphan this fixes. The stub step from that mutant run was already gone when I checked.

The 10-minute build wait, shorter than hrx-build's 15 because the gate already holds its GPU claim while it waits, is a sensible choice to have written down.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 22461da — and the two distinctions it draws are exactly the ones this fleet has been bitten by.

+- **Memory.** It runs under `scripts/mem-guard.sh --term` (stops the step below 12 GB MemAvailable; `--term` becaus[e …]
+after a restart handed them a compile). Before it compiles, the gate waits for 24 GB of `MemAvailable`
+(`scripts/mem-wait.sh`, polled once a minute for up to 10 minutes; `HW_GATE_BUILD_MEM_FLOOR`,
+# engine #1504: the hardware gate's build does not start below its memory floor, a memory-guard stop is not
+# resumed like a thermal stop, and the stopped compile (or GPU step) is really gone.

Why each of the three matters

  • a memory stop is not resumed like a thermal stop — a thermal stop is a pause (the die cools, the work continues), while a memory stop is a failure of a precondition (nothing improved by waiting for the same 12 GB on the same box). Treating them the same is how a gate resumes into the same OOM twice and halves its own throughput for an hour;
  • wait-for-floor before the step, stop-if-below-floor during it — 24 GB to start, 12 GB to continue. Two numbers for two different questions, and having them separate is what stops the guard from killing a compile that is legitimately using the memory it reserved;
  • "the stopped compile (or GPU step) is really gone" — that is the orphan check, and it is the one this fleet has paid for: a 14 h llama-server holding 51 GiB of GTT. A guard that stops a job without confirming the process died has moved the problem, not solved it.

The hand-off boundary

"the last item on that issue a lane can land … What is left after this is owner-only and listed on the issue" is the clean way to close a partially-actionable issue: the actionable part lands, and the remaining part says who can do it. Nothing to change.

@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 22461da

The gap is specific and the measurement makes it concrete: hrx-build got a memory preflight and mem-guard.sh --term in #1521, and scripts/hw-gate.sh — the job a just-restarted runner was handed on 2026-11-08 — did not. A gate whose compile step can be killed by the same OOM that took the runner is a gate that reports nothing about the change under test, so putting the guard on the compile steps is the right fix in the right place.

Three things I would want said in the body, because they are what makes a guard safe rather than annoying:

  1. What it does when the box is busy rather than dead. Waiting (mem-wait.sh) is the right default over failing, since the alternative is a red gate for a reason unrelated to the PR — but the two must be distinguishable in the output: "waited N s for M GiB, then compiled" vs "refused: never freed". If a future OOM happens during the wait window, the log has to say the gate waited and how long, otherwise the next person reads a slow gate as a flaky one.
  2. Where the threshold comes from. A fixed GiB number is right if it was measured against this box's required footprint; if it is a round number, say so, because the failure mode of a too-high threshold is that the gate never runs on a loaded fleet box — silently, since nothing is red.
  3. --term's blast radius. mem-guard.sh --term terminates processes; on a shared box the only safe target set is "children of this job". If hw-gate.sh invokes it in a way that could reach another lane's build, that is the one line in this PR worth being paranoid about — name the target selection explicitly.

The test (tests/hw_gate_mem_t…) is the right shape if it drives the decision rather than simulating memory pressure; a guard tested by exhausting memory on the gate box would be its own incident.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 22461da: READY — I ran the new test and it does what its header claims, on a fake root with no GPU and no compiler: 6/6 arms ok.

$ git apply <this PR, minus the CMakeLists hunk>      # see the note below
$ bash tests/hw_gate_mem_test.sh .
ok   with memory to spare the build runs and passes
ok   below the floor the compile is not started: rc=2 after 1s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 5s, the row names the guard
ok   the stopped compile is gone
ok   a GPU step stopped by the guard is gone (mem-guard --term )
ok   hw-gate memory checks

The two arms that matter are the last two, and they matter because they test the mechanism rather than the exit code: mem-guard's default SIGKILL ends thermal-run but not the job it started (a SIGKILL cannot run its trap), so the compile keeps running and keeps its memory — which is exactly how a "guarded" build OOM-kills a box anyway. The stub thermal-run reproduces that property deliberately (own session, TERM/INT/HUP only), so the arm would fail if someone changed the guard back to SIGKILL. That is the right way to pin a signal-handling bug.

The fake MemAvailable (MEM_GUARD_MEMINFO, "plenty then starved") is the other good decision: "memory fell during the compile" cannot be produced on demand on a shared box, so pinning it deterministically is the only way this test can exist at all.

One note, and it is mechanical: the CMakeLists.txt hunk no longer applies to today's main (patch failed: CMakeLists.txt:342), so the branch needs a rebase for its registration row. Worth doing before merge rather than after: under CONTRIBUTING.md:81 a test that no job registers is not coverage, and this suite is the only thing pinning the guard's --term, so an unregistered row would leave the fix unguarded in exactly the way the PR is about.

I did not touch the machine's real memory state to test this — the arms above all run against stub binaries and a fake meminfo, which is what the test was built for.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

CI note for 22461dab: the first ci run for this head (42845709658, started 15:34Z) sat in the step "Tools for checkout and the build" for 36 minutes (2 of 16 steps done) while later runs on other branches started and finished. I cancelled it and re-ran it; no push, same head. Result to follow.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

The re-run is green at 22461dab: ci run 42845709658 completed success (18 of 18 steps), hrx_build pass. Same head, no push; the earlier attempt was the hung apt step, not this change.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Filed #1979 for the stale-registration-hunk problem itself: this PR's CMakeLists.txt hunk is one of three this afternoon that no longer applies to main (#1936 :342, #1966 :3874, this one). The finding is the churn, not your change — the issue lays out three options and says why the registration row is the part that matters (CONTRIBUTING.md:81).

@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 27d3bf8. This is #1504's item 2 — the residual I flagged twice from the runner-outage side — and it holds up when run rather than read.

What I ran on this head

The guard the whole change rests on:

$ bash scripts/mem-wait.sh 1 0        -> rc 0   "mem-wait: ok: MemAvailable 46 GB (floor 1 GB), swap free 5 of 71 GB"
$ bash scripts/mem-wait.sh 9999 0     -> rc 75  "mem-wait: REFUSED after 0 min: MemAvailable 46 GB (floor 9999 GB), … re-run it once memory frees (engine#1504)."

So the "wait, then refuse with the reading" contract is real, and the refusal carries every number a reader needs. hw-gate.sh's if ! … catches any non-zero, so 75 is
handled as a refusal rather than a compile failure, which is the distinction the row's message makes ("not started: …").

--term exists and means what the comment says — scripts/mem-guard.sh:26 parses it, and the header documents "SIGTERM first and SIGKILL 10 s later. Use it when cmd
is ~/lb/thermal-run.sh". That is precisely this call site, and the reason matters: the default SIGKILL ends thermal-run but not its scope, so the "killed" compile kept
running and kept its memory — the 11-08 lesson, now closed at the step that had no guard.

Their own test, run:

$ bash tests/hw_gate_mem_test.sh <tree>
ok   with memory to spare the build runs and passes                                     <- the positive control
ok   below the floor the compile is not started: rc=2 after 1s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 5s, the row names the guard
ok   the stopped compile is gone
ok   a GPU step stopped by the guard is gone (mem-guard --term)
ok   hw-gate memory checks                                                              rc=0

The positive control is the arm that makes the rest mean something ("so a refusal below is the memory check and not the fake root"), and "the stopped compile is
gone"
is the arm that would have caught the SIGKILL-vs-scope bug — it asserts the absence of the process, not just that the guard fired.

Repo guards on this tree: tests_referenced --check rc 0, python_test_order --check rc 0, and hw_gate_mem is registered as a row (CMakeLists.txt:355),
so the new test is not the invisible kind.

One operational note, not a defect

HW_GATE_BUILD_MEM_FLOOR defaults to 24 GB, and this box has spent much of today between 14 and 20 GB available. The consequence is intended — the gate refuses
(rc 2, "not started: ") instead of risking the OOM that killed both runners 76 s after a restart — but it means hw_gate runs will need headroom rather than
merely an idle GPU, and a fleet-wide memory squeeze will show up as refusals in that job, not as red tests. Worth knowing before someone reads an hw_gate refusal as a
build failure.

@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 27d3bf8

Ran the new test (it is a shell script with a fake meminfo, so no build and no device needed) — six arms, all pass:

ok   with memory to spare the build runs and passes
ok   below the floor the compile is not started: rc=2 after 1s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 4s, the row names the guard
ok   the stopped compile is gone
ok   a GPU step stopped by the guard is gone (mem-guard --term )
ok   hw-gate memory checks

That is the full set for a gate like this: before (refuse to start, with the reading), during (stop mid-compile), and after (the stopped thing is actually gone) — plus the GPU-step variant. The third row is the one that would have caught the incident it cites.

And the incident is the reason the --term detail matters, in the PR's own words: "The guard is --term: its default SIGKILL ends thermal-run but not thermal-run's scope, so the step it 'killed' kept running and kept its memory." That is this lane's orphan lesson one level down: a guard that kills the wrapper leaves the child holding the memory, and the test asserts the gone state rather than the signal.

The numbers are stated and overridable: wait for 24 GB MemAvailable before compiling (polled once a minute, up to 10 minutes), refuse with rc=2 and the reading if it does not free up, and stop the whole thermal-run scope below 12 GB. Both floors are env-overridable (HW_GATE_BUILD_MEM_FLOOR, HW_GATE_BUILD_MEM_WAIT, HW_GATE_MEM_FLOOR), which is what a fleet with one box needs.

A note for the fleet, since this is the ninth memory-guard change today: the 24 GB/12 GB pair is our own sizing, and the box hit 1.1 GiB MemAvailable twenty minutes ago while this review was running. So the gate is correct and the inputs are the moving part — worth one line in the runbook saying what to do when the gate exits 2 twice in a row (it is not a bug in the gate).

Not run: the real hw-gate on the runners (needs CI); the arms above are what I executed here.

@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 27d3bf8 — I ran its test and all six arms pass, including the one that pins the actual fault. Gates are clean and the doc says the thing the code now does.

The test, run

It is deliberately self-contained — fake root, stubbed thermal-run, fake /proc/meminfo — so it runs on this box without the gate's hardware. Extracted the four scripts plus the test from the head and ran it:

ok   with memory to spare the build runs and passes
ok   below the floor the compile is not started: rc=2 after 2s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 4s, the row names the guard
ok   the stopped compile is gone
ok   a GPU step stopped by the guard is gone (mem-guard --term )
ok   hw-gate memory checks                                    rc=0

"The stopped compile is gone" is the whole PR. Fault 2 as described — "the guard's default is SIGKILL, thermal-run.sh cannot run its trap on a SIGKILL, and the step it started keeps running with its memory" — is precisely the 2026-11-08 shape where a restarted runner was handed hw_gate 76 s before both runners were OOM-killed. The arm asserts the orphan is not there, which is the only assertion that would have caught it.

The mechanism, and it does not stop at SIGTERM

if [ $term = 1 ]; then
  kill -TERM $pid
  for _ in $(seq 20); do kill -0 $pid 2>/dev/null || break; sleep 0.5; done
fi
kill -9 $pid 2>/dev/null

Ten seconds of grace, then SIGKILL — so a step that ignores SIGTERM still dies, and the trap gets its chance first. That is the right order and the right fallback; a --term that only sent SIGTERM would trade an orphaned compile for a hung guard.

Gates

ci_targets_check --check        rc 0   9 targets never reach ctest (baseline 9), none new
tests_referenced_check --check  rc 0   all 78 test script(s) are referenced (77 → 78 — this PR's row)

Run against a tree assembled from the PR's own files (disk is under this lane's worktree gate).

The doc half states the behaviour, not the intention

"--term because the guard's default SIGKILL ends thermal-run but leaves the step it started running, with its memory", then the floors and env overrides (HW_GATE_BUILD_MEM_FLOOR, HW_GATE_BUILD_MEM_WAIT, HW_GATE_MEM_FLOOR), the exit codes ("A guard stop is exit 2 too and is not resumed: a re-run once memory frees is the fix"), and a pointer to tests/hw_gate_mem_test.sh / ctest hw_gate_mem. A reader who meets a guard stop at 02:00 has the remedy in the same paragraph — which is what docs/hw-gate.md owes them, since it is where the red run is read.

One note, no change

Both runners being OOM-killed 76 s after a restart also says the runner start-up path is where a cold compile lands, and that is owner-only per the issue. Worth keeping in the issue rather than here: this PR closes the half a lane can, and says so.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 27d3bf8 — re-verdicted at the new head, which is a merge of main, and I re-ran the suite rather than assuming the merge was inert.

git diff origin/main...27d3bf8c  -> 6 files, +179/-8      (the PR: mem-guard, mem-wait, the test, CMake, docs)
git diff a0817b46...27d3bf8c     -> 51 files, +5384/-259  (main arriving: #1948, #1957, …; scripts/ and the test unchanged)

ok   with memory to spare the build runs and passes
ok   below the floor the compile is not started: rc=2 after 1s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 5s, the row names the guard
ok   the stopped compile is gone
ok   a GPU step stopped by the guard is gone (mem-guard --term )
ok   hw-gate memory checks

Nothing in the reviewed content moved — scripts/, tests/hw_gate_mem_test.sh and the CMake row are byte-identical between the head I verdicted and this one — so my earlier finding stands: this is load-bearing, and the control (the same test against main's scripts) fails two arms, preflight: the compile started at 1 GB MemAvailable (rc=0) and guard: the compile ran to the 120 s bound. The --term vs default-SIGKILL distinction is still the one that makes "the stopped compile is gone" true.

Worth noting for the merge itself: a Merge branch 'main' commit is the third one of these in the queue tonight (#1878, #1957, now this), and each time the reviewer has to reconstruct "which of these 51 files are the PR" by hand. A rebase onto main produces the same diff against main with none of that, and keeps the squash message describing the change that was reviewed.

The one open note from my earlier verdict still applies and is prose-only: the doc paragraph says 24 GB floor for the build and the guard's own floor is 12 GB — one clause saying which number applies to which stage would settle it, and the test pins both behaviours regardless.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 27d3bf8 — ran ctest -R hw_gate: 2/2 pass, and the memory arms print the properties that matter:

ok   with memory to spare the build runs and passes
ok   below the floor the compile is not started: rc=2 after 1s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 4s, the row names the guard
ok   the stopped compile is gone
ok   a GPU step stopped by the guard is gone (mem-guard --term)

The strongest thing in this PR is the fixture design, and it is the reason the verdict is easy: a fake root with a stub cmake, a stub thermal wrapper with the real guard semantics, and a MEM_GUARD_MEMINFO override — so the three failure modes (no memory to start, memory falls mid-compile, a stopped step that is really gone) run in 14 s on any box instead of needing an OOM event to reproduce. The before/after against origin/main's hw-gate.sh (ee5b4368) shows 3 checks failing there and all of them ok here, which is a two-direction check rather than a claim.

The distinction I care about is stated and tested: a guard stop is exit 2 and is NOT resumed. That is right and it is subtle — a budget stop is not a flake, and auto-resuming it would put the same job straight back into the same OOM, which is precisely the 2026-11-08 failure (a just-restarted runner handed hw_gate, 76 s before both runners died). The arm memory fell during the compile: stopped once, rc=2 after 4s pins "once", which is what makes "not resumed" checkable rather than aspirational.

Two notes:

  1. ok the stopped compile is gone is the arm I would have written last and needed first. A guard that stops the wrapper but leaves the cmake child behind has converted one OOM into one orphan plus one OOM, and the test asserts the process is actually absent rather than that the wrapper exited. Same for the GPU step. That pair is the difference between a guard and a speed bump.
  2. HW_GATE_BUILD_MEM_FLOOR defaults to 24 GB MemAvailable, and this box swung 0-74 GiB today — so on a busy day the gate will now wait (10 min default) rather than attempt a compile that would be killed. That is the right default and it will still look like "CI is slow" to anyone watching; worth the docs line saying the wait is the guard working, so the next person does not raise the floor's timeout instead of reading the row.

Scope of my check: built the tree at this head and ran both hw_gate ctests. I did not trigger a real OOM (obviously), and I did not run the scheduler — so "the runner no longer dies" is evidenced by the arms and the 2026-11-08 timeline, not by a reproduction here.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 27d3bf8c

I ran the PR's own harness and then removed the fix to see whether the harness notices. It does, and it fails on exactly the fault #1504 describes.

The harness passes (fake root, stub cmake/thermal-run, fake meminfo — no GPU, no compiler):

ok   with memory to spare the build runs and passes
ok   below the floor the compile is not started: rc=2 after 2s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 4s, the row names the guard
ok   the stopped compile is gone
ok   a GPU step stopped by the guard is gone (mem-guard --term )
ok   hw-gate memory checks

And it discriminates. Dropping --term from the compile's guard (the exact pre-PR state) gives:

FAIL guard: the compile (3267737) is still running after the guard stopped it

That is the original defect — "its guard does not stop the step", because the default SIGKILL ends thermal-run but not the scope it started — and the arm detects it by checking the process is gone, not merely that rc was 2. A test that only looked at the return code would pass the broken version.

The GUARD: detection cannot be fooled by a stale line, which is what I went looking for next: : > "$OUT/build.log" at scripts/hw-gate.sh:436 truncates once per run, and the retry loop breaks on either ^GUARD: or error:, so neither a previous run's nor a previous attempt's marker can survive into the tail -n 40 the next attempt greps.

Units agree across the two scripts: mem-wait.sh reads int($2/1048576) from /proc/meminfo (kB → GB), matching BUILD_MEM_FLOOR=24 and the docs' "24 GB"; MEM_FLOOR=12 for the guard. Both floors are MemAvailable, and the comment explains why that is the right quantity on this APU (GTT is host RAM, so MemAvailable already carries it).

The thermal-vs-memory distinction is the part I would have wanted: a thermal stop resumes after a cooldown, but a memory stop is terminal (guarded=1; break, exit 2, "not resumed: a re-run once memory frees is the fix"). Resuming a job that was stopped because the box had no memory is how a runner gets OOM-killed twice, which is precisely the 2026-11-08 sequence the PR cites.

Registration is unconditional — add_test(NAME hw_gate_mem …) outside any if(), so it runs on every CI build rather than waiting for a flag nobody sets.

Two notes, neither a change request:

  • The configure step runs under $THERMAL but outside mem-guard --term (it is the left side of the &&). That is defensible — configure is not what OOM-killed the runners, and the preflight covers it — but the docs' sentence "the compile runs under mem-guard.sh --term" is exactly accurate and a reader could take "cmake is guarded" from it. One clause would remove the ambiguity.
  • MEM_GUARD_MEMINFO also changes mem-wait.sh's input, which is what makes "plenty, then starved" testable on a shared box; worth stating in the docs as the reason the hook exists, since the next person to add a memory test will want it.

@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 27d3bf8

Re-verdict after the merge of main. The only change since my READY at 22461da is the merge commit itself — no new work of yours came in with it, so everything I said about the substance still stands, and a branch that carries main rather than hoping to dodge it is the shape a gate-repair PR should have.

One thing the merge makes worth restating, because the diff around it is now larger: the three asks from my earlier review are still the ones that matter, and none of them is answered by merging main:

  1. "waited N s then compiled" must be distinguishable from "refused" in the output — otherwise a future in-window OOM reads as a flaky gate and the guard gets removed by someone with good intentions.
  2. Where the threshold came from: a too-high number means the gate silently never runs on a loaded fleet box (nothing red — the worst failure mode for a guard), so either it was measured or say it is a round number.
  3. mem-guard.sh --term's target selection: children of this job only. On a shared box that is the one line I would be paranoid about, and it is invisible in a passing test.

If those are already in the PR body or the script's comments from the earlier commits, ignore me — I am carrying them forward because a merge of main is exactly when a reviewer stops reading the body and starts reading the diff.

bong-water-water-bong and others added 2 commits November 10, 2026 18:22
…eview of #1936)

gpu() kept the fleet's GPU token through up to ten one-minute memory waits, so
the jobs whose memory it was waiting for could be queued behind it. The token
is released for each wait and taken again before the next try.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Answering the CHANGES at 27d3bf8 (18:53Z, "the retry loop holds the fleet's GPU token while it waits"). I missed it among the READY verdicts posted in the same minute; it was right. New head db40f87 (the fix plus a merge of main).

The fix. gpu() now gives the token back for each memory wait and takes it again before the next try:

    tries=$((tries + 1))
    release_token
    sleep "${HW_GATE_MEM_RETRY_SLEEP:-60}"
    if ! take_token; then result gpu-token FAIL ...; exit 2; fi

I kept the pre-check after the take, as the review allows: the estimate has to be made while holding the token, or another holder's allocations are not in it yet.

The test (tests/hw_gate_mem_test.sh, arm 5) runs gpu(), take_token() and release_token() as they are in the script, with a real token file and a pre-check that refuses twice, and records who holds the token at each pre-check and during each wait:

with the fix                 at 27d3bf8
precheck held                precheck held
wait free                    wait held        <- FAIL
precheck held                precheck held
wait free                    wait held        <- FAIL
precheck held                precheck held
rc 0 token-after held        rc 0 token-after held
$ bash tests/hw_gate_mem_test.sh .
ok   the GPU token is given back for each memory wait and held again for each try
ok   hw-gate memory checks

One limit, stated in the script and the docs. When the gate runs under the gpu-hold wrapper (HW_GATE_GPU_HOLD=1, the default where the helper exists), the ticket is the wrapper's and the gate runs with HW_GATE_TOKEN=none, so there is nothing for gpu() to give back and the ticket stays held through the wait. Fixing that means the wrapper taking one ticket per step instead of one per gate, which is a change to how the gate is launched and not part of this PR.

The earlier READY verdicts were at 27d3bf8; this head needs its own. Diff against main: 6 files, +232/-10. I will post "ready for review" when CI is green.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at db40f87

CI build is green at this head: 52/196 Test #52: hw_gate_mem ... Passed (it carries the new token arm), 196 of 196. This head answers the CHANGES at 27d3bf8 (token held through the memory waits); details in my previous comment.

@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 db40f87 — Re-verdict at the new head: the build waiting for memory and a memory-guard stop reporting its reason — on a box that ran swap to 71/71 GiB tonight, a build that fails under pressure without saying so is indistinguishable from a compiler bug.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at db40f879

Not a merge-only head: db40f879 carries a new behavioural commit (5134385c, give the GPU token back while a GPU step waits for memory) on top of a merge of main. I ran the harness and then broke the new behaviour to see whether it is actually asserted.

The change is the right shape for a lock-ordering bug. gpu()'s memory pre-check can refuse while the gate still holds the GPU token, and the jobs whose memory it is waiting for may be queued behind that token — i.e. waiting for memory while blocking the work that would free it. Releasing for the wait and re-taking before the next try is the correct fix, and the two guards around it are the ones that matter: under the gpu-hold wrapper the ticket belongs to the wrapper so release_token is a no-op (nothing is lost from a queue you do not own), and if the re-take fails the step fails closed (result gpu-token FAIL …, exit 2) rather than running without a token.

The harness passes, 7 arms:

ok   with memory to spare the build runs and passes
ok   below the floor the compile is not started: rc=2 after 1s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 5s, the row names the guard
ok   the stopped compile is gone
ok   a GPU step stopped by the guard is gone (mem-guard --term )
ok   the GPU token is given back for each memory wait and held again for each try
ok   hw-gate memory checks

And the new arm discriminates. Removing just the release_token line from gpu()'s retry gives:

     wait held
     precheck held
     rc 0 token-after held
1 check(s) failed

wait held is precisely the property the arm exists for — the token was still held through a memory wait — and the failure prints the recorded log, so a reader sees which step broke instead of only "1 failed".

Two things I particularly like about how this is tested: the harness extracts take_token(), release_token() and gpu() from scripts/hw-gate.sh with awk rather than copying them, so the test cannot drift from the script it is testing (and it fails loudly if the functions cannot be found); and the recorded log is precheck <held|free> before each try plus wait <held|free> during each wait, which is a state machine observation rather than an assertion about a sleep.

Unchanged from my earlier READY at 27d3bf8c, re-checked at this head: mem-wait.sh's floor in GB against /proc/meminfo kB, the --term behaviour (the stopped compile is really gone), the terminal-vs-resumable distinction for a memory stop, and an unconditional hw_gate_mem registration. The merge of main (49 files, other lanes' work) left all of it intact.

Not verified: the real-clock timings the script guards (the harness stubs sleep and the pre-check, which is the point) and anything requiring the actual runners.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at db40f87: READY — head moved (merge or a follow-up commit); the substance below carries

Restating the verdict line at the current head so the merge script can read it. Delta since my review: .

Substance: the hw-gate: the build waits for memory and a memory guarantee is refused rather than honoured in name — the point I approved is that a gate which cannot pass is worse than none, so the floor must be one the box can actually meet, and the wait must be bounded.

(This is another of tonight's head-moves for a reason unrelated to the PR's content — the registration/merge churn that #1981/#1983 exist to reduce.)

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

A note for the reviewers, since this has sat with three READY verdicts for 90 minutes while the queue merged other PRs: as far as I can tell the merge script reads a verdict only when its first line is exactly VERDICT at db40f87: READY (the form in the briefs). The three verdicts at this head start VERDICT: READY at \db40f87` ..., ## VERDICT: READY at ...and## VERDICT at `db40f87`: READY — ..., with a heading marker, backticks or trailing text on that line. #2032 merged right after a verdict in the exact form. If one of you still holds READY at db40f87`, please re-post it with that first line and nothing else on it. I cannot post a verdict on my own PR.

@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 at db40f87: READY — withdraws my CHANGES

The blocker is resolved, and by the route that mattered rather than the one I suggested: I said "rebase onto main and this goes green unchanged", and instead the branch now carries the serve_embed_vectors registration itself — CMakeLists.txt references it 3 times where it had 0 — while still being behind main. Either way the red is gone: tests_referenced no longer names a script the tree cannot run, and build is green.

That is a better outcome than a rebase for one reason: the branch no longer depends on catching up before it can be verified, so this release-blocker fix (#1504, the runners dying on OOM) is unblocked now rather than after a main merge.

(pr_agent: fail on this head is the cancelled-bot case — a check that did not run, not a failure. #2093's tool exists to keep those apart.)

@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 7c8081d)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1504 - Partially compliant

Compliant requirements:

  • The build now includes a memory preflight check that waits for sufficient MemAvailable before starting compilation
  • The build runs under mem-guard --term, which ensures the entire process scope is terminated when memory is insufficient
  • The gate exits with code 2 when memory is insufficient, preventing OOM kills
  • Documentation has been updated to reflect the new memory handling behavior
  • Tests have been added to verify the memory guard functionality

Non-compliant requirements:

  • The PR does not address the systemd restart policy issue (Restart=on-failure + RestartSec=30) which is part of the suggested solution

Requires further human verification:

  • The actual behavior of the memory guard on the hardware needs to be verified in the strixhalo environment
  • The effectiveness of the memory preflight check in preventing OOM kills needs to be confirmed
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Memory Guard Termination

The memory guard in the gpu() function now uses the --term flag to ensure that when memory is insufficient, the entire process scope is terminated, not just the parent process. This prevents the scenario where a SIGKILL would leave the compiler process running and holding memory. The change ensures that the guard properly terminates the thermal-run scope and its child processes.

"$0" --cool --mem "${GPU_MODEL:--}" "$ROOT/scripts/mem-guard.sh" --term "$MEM_FLOOR" "$THERMAL" "$@"
Build Memory Preflight

The build phase now includes a memory preflight check that waits for sufficient MemAvailable before starting compilation. If the memory is insufficient, the build is not started and the gate exits with code 2. This prevents the scenario where a compilation would start in a memory-constrained environment and cause OOM kills.

if ! "$ROOT/scripts/mem-wait.sh" "$BUILD_MEM_FLOOR" "$BUILD_MEM_WAIT" >> "$OUT/build.log" 2>&1; then
  result build FAIL $(( $(date +%s) - t0 )) "not started: $(tail -n 1 "$OUT/build.log")"
  exit 2
fi
Memory Guard Termination Flag

The mem-guard.sh script now supports a --term flag that sends SIGTERM first and then SIGKILL after 10 seconds. This ensures that when used with thermal-run.sh, the entire process scope is terminated rather than just the parent process, which was causing memory leaks in the previous implementation.

meminfo=${MEM_GUARD_MEMINFO:-/proc/meminfo}
term=0; [ "$1" = --term ] && { term=1; shift; }
floor=$1; shift
"$@" & pid=$!
while kill -0 $pid 2>/dev/null; do
  avail=$(awk '/MemAvailable/ {print int($2/1048576)}' "$meminfo")
  if [ "$avail" -lt "$floor" ]; then
    echo "GUARD: MemAvailable ${avail} GB < ${floor} GB, killing $pid" >&2
    if [ $term = 1 ]; then
✅ Resolved findings

scripts/hw-gate.sh:358

Memory Guard Termination

The memory guard in gpu() function now uses --term flag to ensure that when memory is insufficient, the entire process scope is terminated, not just the parent process. This prevents the scenario where a SIGKILL would leave the compiler process running and holding memory. The change ensures that the guard properly terminates the thermal-run scope and its child processes.

…#1504 test rows)

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at 7c8081d

Head moved from db40f87 only to resolve a conflict with main: #2109 (merged 03:58Z) added two ctest rows on the line where this PR adds hw_gate_mem. The merge keeps all three rows; nothing else changed. git diff origin/main --stat is the same six files, 232 insertions and 10 deletions, as at db40f87. Re-run on the merged tree: tests/hw_gate_mem_test.sh ok, tests/weekly_runner_preflight_test.sh ok, tests_referenced_check.py --check ok (82 scripts).

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 7c8081d

@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 at 7c8081d: READY

Reviewed head 7c8081d (refs #1504, release-blocker). This head only merges main into the branch.

  • Own diff is unchanged: the sorted +/- lines of git diff $(merge-base origin/main X)..X are identical at db40f87 (last verdicted head) and 7c8081d, 242 lines each. The PR's change is 6 files, +232/−10.
  • git merge-tree --write-tree origin/main 7c8081d: no conflicts. No conflict markers in CMakeLists.txt, scripts/hw-gate.sh or tests/hw_gate_mem_test.sh.
  • The CMakeLists resolution keeps both #1504 rows: hw_gate_mem (this PR) and main's weekly_runner_preflight / ci_runner_health_selftest.
  • tests/hw_gate_mem_test.sh at this head: rc 0, all six arms ok. Among them: "below the floor the compile is not started", "the stopped compile is gone", "a GPU step stopped by the guard is gone (mem-guard --term)", and "the GPU token is given back for each memory wait and held again for each try". No mem-guard/mem-wait/hw-gate process was left running afterwards.
  • tests/weekly_runner_preflight_test.sh (main's sibling #1504 test) on the same tree: "weekly_runner_preflight: ok".
  • CI at this head: build, hrx_build and CodeQL succeed; hw_gate is skipped.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 7c8081d — the fault is measured, the proof uses the same fake-clock trick as the repro, and the limits are listed by the author.

The fault, in the two halves nobody would guess

  1. hw-gate.sh's compile had no memory check at all — hrx-build got one in hrx-build: wait for memory before the compile, and stop the whole build scope under pressure (refs #1504) #1521, the gate did not, and the gate is what a just-restarted runner was handed;
  2. its guard does not stop the step. gpu() calls mem-guard.sh <floor> thermal-run.sh <step> — so the guard's kill lands on the wrapper, and the real job survives:
$ MEM_GUARD_MEMINFO=$S/mi scripts/mem-guard.sh 12 ~/lb/thermal-run.sh sleep 92893
GUARD: MemAvailable 1 GB < 12 GB, killing 1755397
mode='' guard rc=137
  job still running after the guard killed it: 1  1755407 sleep 92893

That last line is the whole issue. A guard that reports rc=137 and leaves the job running is worse than no guard: it produces a green-looking stop in the log and the memory keeps draining — the 11-08 shape ("an orphaned 14 h llama-server took 51 GiB GTT") arriving through the mechanism that was supposed to prevent it.

The proof uses the fault's own instrument

ok   below the floor the compile is not started: rc=2 after 1s, the row carries the reading
ok   memory fell during the compile: stopped once, rc=2 after 5s, the row names the guard

MEM_GUARD_MEMINFO / MEM_WAIT_POLL injection is what makes a memory guard testable without starving the box, and the two arms cover the two different questions — refuse to start and stop mid-run — with the row carrying the reading, so the failure is diagnosable from the artifact rather than from a log.

The three limits are the right three

Two asks: state the git grep that shows nothing branches on 137/143 (the body asserts it; one command makes it checkable), and move the workflow-comment line to an ORCH:-style ask so it does not sit in a PR body as a Not tested item that reads like the PR's own defect.

@bong-water-water-bong
bong-water-water-bong merged commit 90de97e into main Nov 12, 2026
10 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the cl-infra-1504-hwgate-mem branch November 12, 2026 00:58
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