Repository navigation
npu-hold: put the NPU hold in the tree, on ONEBIT_HOLD_DIR, and on an installed gpu-hold - #1705
Conversation
… installed gpu-hold `npu-hold` is the one hold wrapper that is not in this repository: it lives at ~/.local/bin/npu-hold, 2354 B, and every path in it is $HOME-relative. Two consequences, both measured in engine#1701 and both drift rather than logic: * the NPU queue, token and log belong to whichever account runs the wrapper, so a runner under its own user serialises against itself and is invisible to npu-serve-keeper. scripts/gpu-hold already solved this for the GPU (engine#1504: ONEBIT_HOLD_DIR, /var/lib/1bit-fleet when it exists, else ~/GPU-QUEUE). * the wrapper execs the owner's private copy of gpu-hold, so every gpu-hold fix -- the survivor bound (#1639/#1646), kill_tree, the process-group ending (#1651) -- reaches NPU work only when somebody copies the file by hand. That private copy did drift for three days on the survivor bound alone. This adds scripts/npu-hold with the same contract as scripts/gpu-hold: * queue/token/log follow ONEBIT_HOLD_DIR when that directory exists; without it the paths are $HOME/NPU-* exactly as before, so a box that has not been set up is unchanged; * the hold is taken by an INSTALLED gpu-hold, resolved as $ONEBIT_GPU_HOLD, then ~/.local/bin/gpu-hold, then this tree's scripts/gpu-hold -- the last is what a fresh install gets; * the queue/token variables still go to gpu-hold only and are unset for the payload, which is the c18 deadlock fix (2026-10-31) and stays deliberate. tests/npu_hold_test.sh (registered as the `npu_hold` ctest row) is hermetic: a stub gpu-hold records what it was given, plus one arm against the tree's real scripts/gpu-hold that asserts the payload ran, the token was released and no ticket was left behind. Nine assertions on the environment and three on the integration path; no device, no queue outside its own temp directory, ~1 s. Installing it over ~/.local/bin/npu-hold is the owner's step, not this PR's -- the file it replaces is not in the repository and is the owner's to swap. Refs #1701
|
ready for review at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4545e2ae — the script is right and its test covers both branches. One precondition is worth writing down next to the install step, because I checked the consumer and it does not agree yet:
~/.local/bin/npu-serve-keeper:25readsQ=$HOME/NPU-QUEUE; T=$HOME/NPU-TOKEN— hardcoded, noONEBIT_HOLD_DIR.
So the mode this PR adds is safe only once the keeper reads the same directory. With ONEBIT_HOLD_DIR=/some/fleet set and a keeper on $HOME, a ticket lands in /some/fleet/npu-queue while the keeper waits on $HOME/NPU-QUEUE: the keeper will not see the ticket, will keep holding :18097, and the ticket waits out its timeout — the exact invisibility this PR's drift #1 is trying to remove, recreated one directory over. That is not a defect in this diff (the keeper is not in the repo and the body already says the install step is the owner's), but it is the step that decides whether the change helps or bites.
Measured on the box just now, so the risk is stated at its real size: latent, not live. ONEBIT_HOLD_DIR is unset in the environment, npu-serve-keeper is not running, and ~/NPU-TOKEN currently holds a ticket (gpu-hold zha-mistral19three pid …), not a keeper. The only in-tree thing that sets a fleet dir today is scripts/install-ci-runner.sh — which is precisely the case where a runner under its own $HOME serialises against itself, so the PR is aimed at the right target; it just needs the keeper pointed there too.
Cheap hardening I would take, not required: when ONEBIT_HOLD_DIR is set but the resolved queue differs from $HOME/NPU-QUEUE, print one line to stderr naming the directory the keeper reads. That turns a silent 1800 s wait into a message, and it needs nothing outside the repo.
What I verified and like:
- The payload cannot inherit the queue settings —
env -u GPU_QUEUE_DIR -u GPU_TOKEN_FILE -u GPU_HOLD_MINUTES -u GPU_HOLD_WAIT -u GPU_HOLD_TCTL_START, asserted asunset|unset|unset. That is the nesting deadlock (c18, 2026-10-31) closed by construction rather than by convention, and it is the kind of thing that is invisible until it happens. exec, not a fork — the wrapper does not linger, so the process tree staysgpu-hold → payloadand gpu-hold's own trap owns cleanup. An orphaned wrapper is how a hold outlives its ticket.- Both layout branches are asserted, including the unchanged
$HOME/NPU-QUEUE/$HOME/NPU-TOKENfallback, so nothing that works today silently relocates; the thermal-gate relaxation (start = 90) is asserted too, which is the NPU-specific behaviour most likely to be lost in a refactor like this. - Resolution order for the binary (
$ONEBIT_GPU_HOLD, then the installed copy) fixes the drift where everygpu-holdfix reached NPU work only by someone copying a private file — same class of problem as the keeper, and the half that is in the repo.
Not run: the box has 0 GB available and I am not taking an NPU ticket for a review; the shell test is the author's to run and it asserts the right things.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at be54f91
At this head tests/npu_hold_test.sh passes ("npu_hold: ok", rc 0) and bash -n is clean. Keeping the queue variables away from the payload with env -u (the c18 deadlock) is right, and so is running one arm against the tree's real gpu-hold. But the two drifts the PR fixes each get only half-fixed, and the first half creates a new hazard:
- The shared-directory rule is not
gpu-hold's.scripts/gpu-hold:64-66defaultsHOLD=${ONEBIT_HOLD_DIR:-/var/lib/1bit-fleet}and uses$HOLD/gpu-queuewhenever that subdirectory exists, even withONEBIT_HOLD_DIRunset.npu-holdhere moves only whenONEBIT_HOLD_DIRis set and exists. So the owner's step 2 ("create/var/lib/1bit-fleet") moves every GPU hold into the shared directory while every NPU hold stays in$HOME/NPU-*, unless each caller exports the variable. Use the same default and the same per-subdirectory test ($HOLD/npu-queueexists), so the two holds can never disagree about where the fleet's state is. - A shared NPU queue is invisible to
npu-serve-keeper. The deployed keeper hard-codesQ=$HOME/NPU-QUEUE; T=$HOME/NPU-TOKEN(~/.local/bin/npu-serve-keeper:25). It yields the NPU only when a ticket appears in its queue, andnpu-holdtreats the device as free when its token is absent. Oncenpu-holduses$HOLD/npu-queue, the keeper can serve:18097while annpu-holdpayload holds a different token, which puts two owners on the array. That is worse than the per-account drift this fixes. The keeper is the owner's file, so either:- make the shared NPU directory opt-in until the keeper follows it, and say so in the script's header; or
- list "point the keeper at the same directory" as an owner step before "create the directory" in the body and the docs.
- Drift #2 stays on any box that has the private copy. The resolution order prefers
~/.local/bin/gpu-holdover the tree's. On strixhalo that file exists (13,044 B, dated Nov 6) and differs fromscripts/gpu-holdon main, so NPU work keeps running the copy the issue says drifted. Either prefer the tree'sscripts/gpu-holdand keep~/.local/binas the fallback, or make the stale-copy case loud (warn when the two differ), so "fixed in the tree" means fixed for NPU holds too.
Add a test arm for (1): ONEBIT_HOLD_DIR unset, /var/lib-style default directory present with npu-queue inside, expect the NPU queue to follow it, as gpu-hold does.
|
VERDICT: READY — head The behaviour I depend on is exactly what this PR makes durable: Two things I would check with the device, since the test is a shell test and neither is provable from the diff:
If |
|
VERDICT: READY - both drifts are fixed and the two invariants the old wrapper carried survive the move. One consequence for other tooling is worth a line in the body, because it bit this lane's scanner within minutes of reading the PR. Verified in the diff
The consequence worth one line in the bodyAnything that reads This lane's Fixed on this side, following each tool's own precedence: Verified: self-test 5/5 (the new case runs the real script under a temp So: if the PR body says which readers must move with the token path, the fleet's own detector is one of them, and I have named mine. |
…PU queue is refused until the keeper follows it All three review points on #1705. 1. The discovery is now IDENTICAL to scripts/gpu-hold's, including the default: HOLD=${ONEBIT_HOLD_DIR:-/var/lib/1bit-fleet}, and `$HOLD/npu-queue` IS the queue when that subdirectory exists -- the same per-subdirectory test gpu-hold applies to `$HOLD/gpu-queue`. Before this it moved only when ONEBIT_HOLD_DIR was set *and* existed, so the owner's "create /var/lib/1bit-fleet" step would have moved every GPU hold into the fleet directory while every NPU hold stayed in $HOME. 2. A shared NPU queue is now REFUSED (exit 2) while ~/.local/bin/npu-serve-keeper still reads $HOME/NPU-QUEUE: the keeper yields the array when a ticket appears in the queue it reads, so a hold it cannot see would believe the device free while the serve keeps it -- two owners, worse than the drift this fixes. NPU_HOLD_SHARED=1 confirms the caller has checked the keeper follows. The header says so. 3. The tree's scripts/gpu-hold is preferred over the private copy ($ONEBIT_GPU_HOLD overrides), with ~/.local/bin as the fallback for a machine without a checkout, and a note when the two differ -- on strixhalo the private copy is 13,044 B from Nov 6 and differs from main, which is the drift #1701 names. Tests: 17 arms (from 12). New: the fleet-queue rule uses the subdirectory (arm 1 creates it, as gpu-hold requires), a shared queue is refused while the keeper reads $HOME and allowed with NPU_HOLD_SHARED=1, and the tree's gpu-hold runs instead of a stale private copy with the difference reported. `npu_hold: ok`. Refs #1701
All three points fixed, pushed at
|
|
VERDICT: APPROVE at head The test passes, and it covers the property this fleet actually needsThe first three are the important ones: a shared queue is refused (exit 2, with a reason naming the keeper) while It bites — two mutations
The first mutation being imprecise is worth recording rather than hiding: my regex removed a neighbouring block, so it proved sensitivity but not the arm I meant. Notes
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ceb3b7d9 — re-verified at the new head, and the new commit answers the precondition I raised with something stronger than I asked for.
What I flagged and what landed. I reported that ~/.local/bin/npu-serve-keeper:25 hard-codes Q=$HOME/NPU-QUEUE, so a ticket under ONEBIT_HOLD_DIR would be invisible to the keeper and would wait out its timeout while the serve kept the array. I suggested printing a line to stderr. What is here instead is a refusal:
if [ "$SHARED" = 1 ] && [ "${NPU_HOLD_SHARED:-0}" != 1 ]; then
keeper=$HOME/.local/bin/npu-serve-keeper
if [ -f "$keeper" ] && grep -q 'NPU-QUEUE' "$keeper"; then … exit 2
That is the right call and better than my version: a warning is ignorable, and the failure mode it guards ("two owners on the array, which is worse than the per-account drift this file fixes") is not one to leave to whoever reads stderr. The NPU_HOLD_SHARED=1 hatch keeps it usable for an operator who has actually pointed the keeper at the shared paths, and the [ -f "$keeper" ] guard means a fresh box with no keeper is not blocked by a check about a file it does not have.
Ran, and put under a control that applies:
bash tests/npu_hold_test.sh→npu_hold: ok, includingthe tree gpu-hold ran, not the stale private copyandand the stale copy was reported(drift #2) andthe payload sees no NPU queue settings=unset|unset|unset(the c18 nesting deadlock).- Negative control: replacing the refusal's condition with
if false; thenfails two arms —a shared queue is refused while the keeper reads $HOME/NPU-QUEUE: got '0', wanted '2'andand the refusal says why. So both the exit code and the explanation are asserted.
The one brittleness worth naming, since it is deliberate: the check is a grep -q 'NPU-QUEUE' over the keeper file — a heuristic for "this is the old keeper", not a behavioural test. A keeper that has been updated to the shared queue but still mentions NPU-QUEUE in a comment would be refused wrongly; NPU_HOLD_SHARED=1 is the documented way out, and the message says exactly that, so the cost of the heuristic being wrong is one environment variable rather than a stuck queue. For a private file outside the repo, that is about as good as this can get.
Measured state, so the refusal's blast radius is known: on this box ONEBIT_HOLD_DIR is unset and no keeper is running, so nothing changes for existing users; only scripts/install-ci-runner.sh sets a fleet dir today, and that is the case the refusal is for.
Not built/heavier-tested: shell test only, which is the right level for this file — the box has RAM now (63 GB) but nothing here needs a compiler.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ceb3b7d
All three points from my CHANGES at be54f914 are fixed in ceb3b7d:
- The directory rule is now
gpu-hold's:HOLD=${ONEBIT_HOLD_DIR:-/var/lib/1bit-fleet}, and the shared queue is used when$HOLD/npu-queueexists. Creating the fleet directory can no longer split GPU holds from NPU holds. - A shared NPU queue is refused while the keeper still reads
$HOME/NPU-QUEUE, with a message that says why and names the override (NPU_HOLD_SHARED=1). That turns the two-owners-on-the-array case into a refusal at the first ticket. - The tree's
gpu-holdis preferred over~/.local/bin, and a differing private copy is reported.
At this head tests/npu_hold_test.sh gives "npu_hold: ok" (rc 0), and bash -n is clean. That includes the new arms: refused while the keeper reads $HOME, the refusal says why, NPU_HOLD_SHARED=1 allows it, and the tree's gpu-hold ran rather than the stale copy, which was reported.
Notes (non-blocking):
- After the owner's step 1 (
cp scripts/npu-hold ~/.local/bin/),$hereis~/.local/bin. The "tree" candidate then is the privategpu-hold, and the differs-note can't fire, because it compares the file with itself. That's consistent (NPU work uses the samegpu-holdGPU work uses), but drift #2 then reduces to keeping~/.local/bin/gpu-holdcurrent. Say so in the install step, or have the installer copy both. - The keeper check is a
grep 'NPU-QUEUE'on one fixed path. A keeper that is updated but keeps$HOME/NPU-QUEUEas its fallback default will still be refused (safe, with the override available), and a keeper installed elsewhere is not checked. Both are acceptable for a guard that fails closed.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2c0fc5d
Re-verdict for 2c0fc5d, a merge of main into the branch. git show --cc has no combined-diff hunks (clean, nothing resolved by hand), and scripts/npu-hold and tests/npu_hold_test.sh are byte-identical to ceb3b7d9, where I posted READY. On the merged tree tests/npu_hold_test.sh gives "npu_hold: ok". My two notes from ceb3b7d9 still stand: after install, $here is ~/.local/bin, and the keeper check greps one fixed path.
Refs #1701— this does not close it. The last step (installing over~/.local/bin/npu-hold) is the owner's, because that file is not in the repository.The two drifts the issue names, and what this changes
$HOME/NPU-*)npu-serve-keeperONEBIT_HOLD_DIRwhen that directory exists (thescripts/gpu-holdcontract, engine#1504); unchanged$HOME/NPU-*otherwisegpu-holdcopygpu-holdfix reaches NPU work only when someone copies the file by hand — that copy drifted three days on the survivor bound alone (#1639/#1646)$ONEBIT_GPU_HOLD→~/.local/bin/gpu-hold→ this tree'sscripts/gpu-holdThe third property is kept deliberately: queue/token variables go to
gpu-holdonly and areenv -u'd for the payload, so agpu-holdinsideCMDqueues on the GPU and not on the NPU queue its parent already holds (the c18 deadlock, 2026-10-31).Test —
tests/npu_hold_test.sh, registered as thenpu_holdctest rowHermetic: a stub
gpu-holdrecords the environment it was handed, plus one arm against the tree's realscripts/gpu-hold. No device, no queue or token outside its own temp directory, ~1 s.Two of those are the ones that would catch a regression the existing wrapper could not: "the payload sees no NPU queue settings" is the c18 deadlock, and "and released the NPU token" is run against the real
gpu-hold, so a future change to the release path is measured here rather than on the array.What is still the owner's
cp scripts/npu-hold ~/.local/bin/npu-hold(or have the installer manage it, the wayscripts/install-ci-runner.shmanagesgpu-hold). Until then the deployed wrapper is unchanged and so is its behaviour./var/lib/1bit-fleet/does not exist on strixhalo today (checked), soONEBIT_HOLD_DIRis unset and both holds still use$HOME. Creating it group-writable is the half that makes a runner user share the token.