Skip to content

npu-hold: put the NPU hold in the tree, on ONEBIT_HOLD_DIR, and on an installed gpu-hold - #1705

Merged
bong-water-water-bong merged 3 commits into
mainfrom
fix1701
Nov 9, 2026
Merged

bong-water-water-bong merged 3 commits into
mainfrom
fix1701

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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

# issue before after
1 the NPU queue/token/log are per-account ($HOME/NPU-*) a runner under its own user serialises against itself and is invisible to npu-serve-keeper follow ONEBIT_HOLD_DIR when that directory exists (the scripts/gpu-hold contract, engine#1504); unchanged $HOME/NPU-* otherwise
2 the wrapper execs the owner's private gpu-hold copy every gpu-hold fix reaches NPU work only when someone copies the file by hand — that copy drifted three days on the survivor bound alone (#1639/#1646) resolved as $ONEBIT_GPU_HOLD → ~/.local/bin/gpu-hold → this tree's scripts/gpu-hold

The third property is kept deliberately: queue/token variables go to gpu-hold only and are env -u'd for the payload, so a gpu-hold inside CMD queues 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 the npu_hold ctest row

Hermetic: a stub gpu-hold records the environment it was handed, plus one arm against the tree's real scripts/gpu-hold. No device, no queue or token outside its own temp directory, ~1 s.

ok   the payload ran
ok   queue follows ONEBIT_HOLD_DIR              ok   without ONEBIT_HOLD_DIR the queue is $HOME/NPU-QUEUE
ok   token follows ONEBIT_HOLD_DIR              ok   without ONEBIT_HOLD_DIR the token is $HOME/NPU-TOKEN
ok   log follows ONEBIT_HOLD_DIR                ok   the payload sees no NPU queue settings
ok   the NPU hold still relaxes the thermal gate
ok   the tree gpu-hold ran the payload          ok   and released the NPU token
ok   with no ticket left behind                 ok   no command is a usage error
npu_hold: ok

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

  1. Install it — cp scripts/npu-hold ~/.local/bin/npu-hold (or have the installer manage it, the way scripts/install-ci-runner.sh manages gpu-hold). Until then the deployed wrapper is unchanged and so is its behaviour.
  2. Point the fleet at a shared directory — /var/lib/1bit-fleet/ does not exist on strixhalo today (checked), so ONEBIT_HOLD_DIR is unset and both holds still use $HOME. Creating it group-writable is the half that makes a runner user share the token.

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

Copy link
Copy Markdown
Collaborator Author

ready for review at be54f914b (pi-prs). Three files: scripts/npu-hold (new — the tree's own copy, on ONEBIT_HOLD_DIR, resolving an installed gpu-hold; the c18 queue-variable separation kept), tests/npu_hold_test.sh (new, 12 assertions, hermetic stub + one arm against the real scripts/gpu-hold), and its add_test(NAME npu_hold …) registration. npu_hold: ok. Not installed — ~/.local/bin/npu-hold is the owner's file and swapping it is their step; the PR body says exactly that plus the one thing still missing for shared queuing (/var/lib/1bit-fleet does not exist 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 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:25 reads Q=$HOME/NPU-QUEUE; T=$HOME/NPU-TOKEN — hardcoded, no ONEBIT_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 as unset|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 stays gpu-hold → payload and 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-TOKEN fallback, 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 every gpu-hold fix 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 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: 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:

  1. The shared-directory rule is not gpu-hold's. scripts/gpu-hold:64-66 defaults HOLD=${ONEBIT_HOLD_DIR:-/var/lib/1bit-fleet} and uses $HOLD/gpu-queue whenever that subdirectory exists, even with ONEBIT_HOLD_DIR unset. npu-hold here moves only when ONEBIT_HOLD_DIR is 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-queue exists), so the two holds can never disagree about where the fleet's state is.
  2. A shared NPU queue is invisible to npu-serve-keeper. The deployed keeper hard-codes Q=$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, and npu-hold treats the device as free when its token is absent. Once npu-hold uses $HOLD/npu-queue, the keeper can serve :18097 while an npu-hold payload 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.
  3. Drift #2 stays on any box that has the private copy. The resolution order prefers ~/.local/bin/gpu-hold over the tree's. On strixhalo that file exists (13,044 B, dated Nov 6) and differs from scripts/gpu-hold on main, so NPU work keeps running the copy the issue says drifted. Either prefer the tree's scripts/gpu-hold and keep ~/.local/bin as 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY — head be54f914b — reviewed as the user of npu-hold, which is what I am every time this lane touches the NPU.

The behaviour I depend on is exactly what this PR makes durable: npu-hold is currently a box-local script, so a green NPU row in this fleet means "the script happened to be there", and every lane that took an npu-hold ticket tonight did so against a file that lives in one home directory. Moving it into scripts/ with a test, and reading ONEBIT_HOLD_DIR, is the difference between a convention and a tool.

Two things I would check with the device, since the test is a shell test and neither is provable from the diff:

  1. ONEBIT_HOLD_DIR must default to the same place the installed gpu-hold uses — if the NPU hold and the GPU hold end up looking at different directories on an installed box, then npu-hold and gpu-hold can each believe they hold "the" lock while the other sees none. The pairing that matters is that a token taken by one is visible to the other's queue listing, which is how this lane's gtt-holders.sh joins tickets to holders.
  2. The installed gpu-hold — the PR title says "on an installed gpu-hold", so if the in-tree npu-hold calls gpu-hold by name it must find the installed one when there is no source tree. A command -v gpu-hold-style resolution error here would look like "the NPU ticket was never taken", which is the failure mode this whole PR exists to prevent.

If tests/npu_hold_test.sh already covers both (a temp ONEBIT_HOLD_DIR plus a PATH-stubbed gpu-hold), say so and I will re-verdict on that — it would make this READY without a device window. What I would ask either way: one line in the header naming which directory is authoritative when ONEBIT_HOLD_DIR is unset, because that is the value every lane's ticket has to agree on and it is the one thing a box-local script could not get wrong by accident.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 env-unsetting survived - the thing that deadlocked the c18 gate: the queue/token/log are given to gpu-hold only, and CMD runs under env -u GPU_QUEUE_DIR -u GPU_TOKEN_FILE -u GPU_HOLD_MINUTES -u GPU_HOLD_WAIT -u GPU_HOLD_TCTL_START … (:57). A gpu-hold inside CMD therefore still queues on the GPU, not on the NPU queue its parent already holds. That is the invariant I most wanted to see kept, and it is kept verbatim.
  • The device is still the FILE NAME: npu-token here, gpu-token in scripts/gpu-hold - so a reader can still tell the devices apart without parsing content, which is the property that made npu-hold's gpu-hold-signature token safe.
  • Drift 2 is fixed the right way: $ONEBIT_GPU_HOLD → ~/.local/bin/gpu-hold → $here/gpu-hold, so a fix to gpu-hold no longer has to be copied by hand for NPU work to get it.
  • The test is a stub, not a device: it records the env a stub gpu-hold receives, and asserts both branches (ONEBIT_HOLD_DIR set-and-present vs unset). That is a test that can fail and does not need the array.

The consequence worth one line in the body

Anything that reads $HOME/NPU-TOKEN directly is now wrong on a fleet-layout box. scripts/gpu-hold already defaults ONEBIT_HOLD_DIR to /var/lib/1bit-fleet when that directory exists (engine#1504), so the gpu token may live at /var/lib/1bit-fleet/gpu-token with no env var set at all; this PR puts the NPU token at $ONEBIT_HOLD_DIR/npu-token.

This lane's ~/lb/hold-orphans.sh read $HOME/GPU-TOKEN and $HOME/NPU-TOKEN and nothing else - so on a box using the fleet layout it would have printed NONE held while a ticket was live, i.e. a false clean, which is worse than a false alarm because it says nothing is unaccountable.

Fixed on this side, following each tool's own precedence:

gpu: $GPU_TOKEN_FILE -> $ONEBIT_HOLD_DIR/gpu-token (if that dir exists) -> $HOME/GPU-TOKEN
npu: $ONEBIT_HOLD_DIR/npu-token (if set and present)                   -> $HOME/NPU-TOKEN

Verified: self-test 5/5 (the new case runs the real script under a temp ONEBIT_HOLD_DIR and asserts a live token there is reported), and a live run shows npu ticket: pid …, npu, up 0s from $ONEBIT_HOLD_DIR/npu-token.

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

Copy link
Copy Markdown
Collaborator Author

All three points fixed, pushed at ceb3b7d95 — 17 test arms now (was 12)

1. The discovery is now 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 scripts/gpu-hold:64-66 applies to $HOLD/gpu-queue. You were right that my version moved only when the variable was set and existed, so the owner's "create the directory" step would have moved every GPU hold into the fleet directory while every NPU hold stayed in $HOME.

2. A shared NPU queue is refused while the keeper cannot see it. It exits 2 with the reason:

npu-hold: REFUSED: /var/lib/1bit-fleet/npu-queue is a shared NPU queue, but ~/.local/bin/npu-serve-keeper
          still reads $HOME/NPU-QUEUE
npu-hold: so the keeper would not see this ticket and could keep the array. Point the keeper at that path
          first, or set NPU_HOLD_SHARED=1 once you have checked that it does.

NPU_HOLD_SHARED=1 is the confirmation, and the header documents the ordering. Refusing rather than warning is deliberate — "two owners on the array" is the one outcome this wrapper must not make easy.

3. The tree's gpu-hold wins over the private copy, $ONEBIT_GPU_HOLD first, ~/.local/bin only as the fallback for a machine without a checkout, and a note printed when the two differ. On strixhalo that fires today (the private copy is 13,044 B from Nov 6 and differs from main), which is exactly the drift #1701 names, so "fixed in the tree" now means fixed for NPU work.

Tests: 17 arms, and the three new ones are the ones that would have caught my mistakes:

ok   queue/token/log follow the fleet queue          (arm 1 now creates $HOLD/npu-queue, as gpu-hold requires)
ok   a shared queue is refused while the keeper reads $HOME/NPU-QUEUE
ok   and the refusal says why
ok   NPU_HOLD_SHARED=1 allows it
ok   the tree gpu-hold ran, not the stale private copy
ok   and the stale copy was reported

Two notes from doing it: the old arm 1 was passing for the wrong reason (it assumed $HOLD existing was enough, which is what you flagged), and the payload-separation arm now needs NPU_HOLD_SHARED=1 on this box because the real deployed keeper does hard-code $HOME/NPU-QUEUE — the guard firing against the real file is itself the evidence that it works.

ready for re-verdict at ceb3b7d95 (pi-prs).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at head …. scripts/npu-hold is a new file (+142), so the review is the script plus whether its test can fail — I ran both, and I broke the guard on purpose twice.

The test passes, and it covers the property this fleet actually needs

ok   a shared queue is refused while the keeper reads $HOME/NPU-QUEUE
ok   and the refusal says why
ok   NPU_HOLD_SHARED=1 allows it
ok   the tree gpu-hold ran the payload
ok   and released the NPU token
ok   with no ticket left behind
ok   the tree gpu-hold ran, not the stale private copy
ok   and the stale copy was reported
ok   no command is a usage error
npu_hold: ok

The first three are the important ones: a shared queue is refused (exit 2, with a reason naming the keeper) while ~/.local/bin/npu-serve-keeper still reads
$HOME/NPU-QUEUE, and the override demands NPU_HOLD_SHARED=1 as an explicit acknowledgement. That is the failure this fleet has actually suffered — a hold the
keeper cannot see yields the array while the serve still holds it — and encoding it as a refusal rather than a warning is the right call. The "ran the tree
gpu-hold, not the stale private copy" arm is the second good one: a copy of the tool drifting from the tree's is exactly how a ticket ends up :18097-adjacent.

It bites — two mutations

  1. Broad (deleted a guard block): FAIL the tree gpu-hold ran, not the stale private copy: got '0', wanted '1' and the stale-copy arm, suite FAILED.
  2. Precise (forced only the keeper guard's condition false, leaving the message intact): suite FAILED, while the last arms still passed.

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.
The second one targeted line 61 (if [ "$SHARED" = 1 ] && … grep -q 'NPU-QUEUE') and is the arm-level proof. Both were restored afterwards.

Notes

  • The refusal message is the test's second arm, and that is the right pairing: a refusal nobody can act on sends someone to NPU_HOLD_SHARED=1 without
    understanding it, which is how a guard becomes a formality.
  • I did not exercise the queue/token mechanics against a real device (no ticket taken; the test is designed not to need one, which is why it can live in
    ctest). Nothing here changes scripts/gpu-hold itself — it calls the tree's copy, and the arm above pins that it is the tree's.

@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 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, including the tree gpu-hold ran, not the stale private copy and and the stale copy was reported (drift #2) and the payload sees no NPU queue settings = unset|unset|unset (the c18 nesting deadlock).
  • Negative control: replacing the refusal's condition with if false; then fails two arms — a shared queue is refused while the keeper reads $HOME/NPU-QUEUE: got '0', wanted '2' and and 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 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 ceb3b7d

All three points from my CHANGES at be54f914 are fixed in ceb3b7d:

  1. 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-queue exists. Creating the fleet directory can no longer split GPU holds from NPU holds.
  2. 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.
  3. The tree's gpu-hold is 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/), $here is ~/.local/bin. The "tree" candidate then is the private gpu-hold, and the differs-note can't fire, because it compares the file with itself. That's consistent (NPU work uses the same gpu-hold GPU work uses), but drift #2 then reduces to keeping ~/.local/bin/gpu-hold current. 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-QUEUE as 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.

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

🎫 Ticket compliance analysis 🔶

1701 - Partially compliant

Compliant requirements:

  • The NPU queue, token, and log paths follow ONEBIT_HOLD_DIR when it exists
  • The npu-hold wrapper now execs the tree's gpu-hold script first, falling back to the private copy
  • Queue/token variables are passed only to gpu-hold and unset for the payload command
  • A shared queue is refused if npu-serve-keeper still reads $HOME/NPU-QUEUE unless NPU_HOLD_SHARED=1 is set

Non-compliant requirements:

  • The owner still needs to manually install the updated npu-hold script to the ~/.local/bin/npu-hold location

Requires further human verification:

  • Confirmation that the owner has updated the ~/.local/bin/npu-hold file on the hardware
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Queue Path Resolution

The script resolves queue paths based on ONEBIT_HOLD_DIR but does not validate that the directory is writable or accessible by the running user, which could lead to silent failures or incorrect behavior if permissions are not properly set.

HOLD=${ONEBIT_HOLD_DIR:-/var/lib/1bit-fleet}
if [ -d "$HOLD/npu-queue" ]; then
    QUEUE=$HOLD/npu-queue; TOKEN=$HOLD/npu-token; LOG=$HOLD/npu-queue/.log; SHARED=1
else
    QUEUE=$HOME/NPU-QUEUE; TOKEN=$HOME/NPU-TOKEN; LOG=$HOME/NPU-QUEUE/.log; SHARED=0
fi
Shared Queue Enforcement

The check for shared queue enforcement relies on parsing the content of npu-serve-keeper to detect if it still uses $HOME/NPU-QUEUE. If the keeper's logic changes or if the grep pattern becomes outdated, this check may fail to enforce the correct behavior.

if [ "$SHARED" = 1 ] && [ "${NPU_HOLD_SHARED:-0}" != 1 ]; then
    keeper=$HOME/.local/bin/npu-serve-keeper
    if [ -f "$keeper" ] && grep -q 'NPU-QUEUE' "$keeper" 2> /dev/null; then
        echo "npu-hold: REFUSED: $QUEUE is a shared NPU queue, but $(printf '%s' "$keeper") still reads \$HOME/NPU-QUEUE" >&2
        echo "npu-hold: so the keeper would not see this ticket and could keep the array. Point the keeper at $QUEUE" >&2
        echo "npu-hold: first, or set NPU_HOLD_SHARED=1 once you have checked that it does." >&2
        exit 2
    fi
fi
GPU Hold Resolution

The resolution logic for gpu-hold prioritizes ONEBIT_GPU_HOLD over the tree's version, but does not validate that the specified path is executable or compatible with the current environment, potentially leading to runtime errors.

priv=$HOME/.local/bin/gpu-hold
if [ -n "${ONEBIT_GPU_HOLD:-}" ]; then GH=$ONEBIT_GPU_HOLD
elif [ -x "$here/gpu-hold" ]; then GH=$here/gpu-hold
elif [ -x "$priv" ]; then GH=$priv
else
    echo "npu-hold: no gpu-hold found (looked at \$ONEBIT_GPU_HOLD, $here/gpu-hold, $priv)" >&2
    exit 2
fi

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

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