Skip to content

q4nx cache: an invoker for the sweep, because nothing sweeps a box that never serves (#2263) - #2269

Merged
bong-water-water-bong merged 5 commits into
mainfrom
pkg/2263-q4nx-timer
Nov 12, 2026
Merged

bong-water-water-bong merged 5 commits into
mainfrom
pkg/2263-q4nx-timer

Conversation

@bong-water-water-bong

@bong-water-water-bong bong-water-water-bong commented Nov 12, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #2263 — the issue's own conclusion is that the fix is a box configuration change, not a code change, and it is right.

What this closes and what it does not: the repository now ships the invoker (below), which is the half that was missing — 1bit q4nx-cache evict merged and uncalled. The sweep starts when the installer is run on a box:
scripts/install-q4nx-cache-timer.sh (dry-run first). Nothing in the repository installs it, by design. If a box must be swept without that step, this issue should stay open until the owner runs it. This is the part that can live in the repo without making that decision for anyone: the invoker, as one command.

The gap, restated with the numbers the issue measured

1bit q4nx-cache evict is correct, tested and merged (#2251) and nothing calls it. The cache has two eviction triggers — the write-time sweep inside serve, and the manual command — and both need the binary to run. A review, CI or build box holds the cache and never serves, so it keeps every entry it ever wrote:

cache entries
after a hand prune 26 G 10
12 h later, #2251 merged 59 G 12
after one manual 24 h age cut 24 G 5

59 G against a 931 G volume left free space 3 GiB under the 175 GiB build gate — a fleet-wide build freeze caused by a cache that is regenerated by re-importing a pack.

What this adds

scripts/install-q4nx-cache-timer.sh              # hourly, --max-gib 40 --min-age 86400, opt-in
scripts/install-q4nx-cache-timer.sh --dry-run    # print the units, verify them, change nothing
scripts/install-q4nx-cache-timer.sh --uninstall  # remove the units, leave the cache alone

Writes only into ~/.config/systemd/user/ — no root, no /etc, no repo state — and does nothing until someone runs it. The sweep command and flags are checked against app/q4nx_cache.cpp's own usage text (--root, --max-gib | --max-mib, --keep, --min-age, --dry-run).

Three things it refuses, each because the alternative fails quietly

  1. A binary that predates the command. This box's deployed rc-2026-11-03-c22 answers 1bit q4nx-cache with unknown command 'q4nx-cache'. A unit naming it fails hourly and nobody reads the journal — so the installer probes the binary (q4nx-cache with no subcommand prints usage and returns 2) and refuses, naming engine#2251.
  2. --max-gib 0. The CLI documents 0 as sweep disabled, not "evict everything" — my first draft's message said the latter and the source contradicted it. A timer installed with 0 would fire hourly and do nothing, which is the exact failure this script exists to remove.
  3. An empty --interval. OnCalendar= with nothing after it is a timer that never fires, and systemd accepts the unit anyway.

Docs

docs/npu.md documented the sweep as something that happens after a write. That is true, and it is precisely why a box that never writes-on-serve grows without bound. It now says so, with the measurement and the installer, including the capability refusal.

Verification

--self-test: 9 arms, all through the real code — the generated units and their bounds, Persistent=true, --max-gib 0 refused with its reason, empty --interval refused, the capability probe in both directions with stub binaries, and that --dry-run writes nothing. No systemd, no cache, no 1bit touched.

Registered as cmake/tests-q4nx-cache-timer.cmake — a family file, so the glob picks it up and no CMakeLists.txt line is touched: ci_targets_check.py --where q4nx_cache_timer_self → cmake/tests-q4nx-cache-timer.cmake:20, and tests_referenced_check.py reports 85/85 scripts referenced with 41 self-tests.

Not a close. Whether this box runs a timer is the owner's decision; I have only made it one command, and the refusal on an outdated binary is what stops it being installed into a silent no-op today.

The two review asks, both fixed

  • A value flag given last spun the argument loop for ever. shift 2 on a one-argument list does not shift, so --dry-run --max-gib looped at full CPU — the reviewer's repro timeout 5 … --dry-run $f exited 124 for all four flags. needs_value() now runs before every shift 2 ("$1 needs a value"), and four self-test arms run that exact repro under timeout 5, failing the suite if any is killed. After: 1 1 1 1. Negative control: neutering the guard turns the suite red and the arms report SPUN rather than hanging.
  • A relative --1bit reached systemd as ExecStart=build1336/1bit, which answers "Neither a valid executable name nor an absolute path" — a unit-file complaint about a flag. It is now refused where the mistake was typed, and before the executability test so the message is the right one: --1bit wants an absolute path, got 'relative/1bit' (systemd requires an absolute ExecStart).

15 self-test arms, 0 failures; --dry-run against a capable stub still ends "verify: systemd accepts both units", and --max-gib 0 still refuses with its reason.

bong-water-water-bong added 2 commits November 12, 2026 03:35
…dy leaking code

#2258 fixed `scripts/`: usage text built from `sed -n '16,62p' "$0"`, which is a claim about a file's shape and stops being true
when the header above it is edited. The same pattern is in eight more files outside `scripts/`, and **seven had already drifted
past the end of their header** -- measured statically, by comparing the range's end with the first line of code:

    os/v2/mkdebs.sh                  range 22,45   code starts 44   -> prints code
    os/v2/mkrepo.sh                  range 21,27   code starts 23   -> prints code
    os/v2/mkrootfs.sh                range 21,32   code starts 32   -> prints code
    os/v2/proto.sh                   range 21,32   code starts 27   -> prints code
    os/v2/vmtest-v2.sh               range 21,34   code starts 31   -> prints code
    os/v3/puppy/demo-seq-probe.sh    range 28,48   code starts 44   -> prints code
    tools/xhci_keepawake_check.sh    range 17,40   code starts 37   -> prints code
    onnx/bench_npu.sh                range 2,30    header ends 45   -> prints only PART of its help (the other direction)

`tools/xhci_keepawake_check.sh` is the one that matters most: `docs/axb35-usb-ports.md` and the release notes tell a person to run
it with `--require-awake`, and `--help` on it printed code instead of the header.

All eight now use the anchored extraction from #2258, defined below the first line of code so its own comment cannot leak:

    usage() { awk 'NR>1 && /^#/ {sub(/^# ?/,""); print; next} NR>1 {exit}' "$0"; }

Verified **statically, without executing them** -- several of these reach for devices (`bench_npu.sh` opens the NPU, which is off by
owner order; `demo-seq-probe.sh` and `vmtest-v2.sh` too), and #2258's lesson was that running a help path can take a resource. So:

  * every file: `bash -n` clean, exactly one `usage()` definition, no self-recursive body, no other line ranges left;
  * each `usage()` output simulated with the same awk: 16-37 lines of prose, **zero** code-like lines, and `bench_npu.sh` now prints
    its whole header instead of the first 29 lines of it.
…at never serves (engine#2263)

`1bit q4nx-cache evict` is correct, tested and merged (#2251) and **nothing calls it**. The cache has two eviction
triggers, the write-time sweep inside `serve` and the manual command, and both need the binary to run -- so a review, CI or
build box that holds the cache and never serves keeps every entry it ever wrote. engine#2263 measured it: 59 GiB in 12 h
with the command merged for nine hours, which put free space 3 GiB under the 175 GiB build gate; one manual 24 h age cut
took it to 24 GiB and it regrew.

The issue's own conclusion is that the fix is a box configuration change rather than a code change. This is the part that
can live in the repository without making that decision for anyone:

  scripts/install-q4nx-cache-timer.sh          # hourly, --max-gib 40 --min-age 86400, opt-in
  scripts/install-q4nx-cache-timer.sh --dry-run    # print the units, verify them, change nothing
  scripts/install-q4nx-cache-timer.sh --uninstall  # remove the units, leave the cache alone

It writes only into `~/.config/systemd/user/` (no root, no /etc, no repo state) and does nothing until someone runs it.

Three things it refuses, each because the alternative is a timer that fails quietly:

  * **a binary that predates the command.** This box's deployed rc-2026-11-03-c22 answers `1bit q4nx-cache` with
    `unknown command 'q4nx-cache'`; a unit naming it would fail hourly and nobody would read the journal. The installer
    probes the binary and refuses, naming engine#2251.
  * **`--max-gib 0`.** The CLI documents 0 as *sweep disabled*, not "evict everything" (my first message said the latter
    and the source contradicted it), so 0 would install an hourly no-op -- the exact failure this script removes.
  * **an empty `--interval`.** `OnCalendar=` with nothing after it is a timer that never fires, and systemd accepts the
    unit anyway.

The sweep command and its bounds are checked against `app/q4nx_cache.cpp`'s own usage text: `--root`, `--max-gib | --max-mib`,
`--keep`, `--min-age`, `--dry-run`.

`docs/npu.md` documented the sweep as something that happens after a write, which is true and is exactly why a box that
never writes-on-serve keeps growing. It now says so, with the measurement and the installer.

Registered as `cmake/tests-q4nx-cache-timer.cmake` (a family file: the glob picks it up, no CMakeLists line) with a
`--self-test` that pins nine arms -- the units' content, `Persistent=true`, both refusals, the capability probe in both
directions, and that `--dry-run` writes nothing -- using stub binaries, so no systemd, no cache and no 1bit are touched.
@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 73ab825)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2263 - Partially compliant

Compliant requirements:

  • The repository now ships the invoker script scripts/install-q4nx-cache-timer.sh
  • The script is opt-in and only touches the calling user's systemd --user directory
  • The script includes dry-run functionality to verify units before installation
  • The script includes uninstall functionality to remove the units
  • The script includes self-test functionality to validate its own logic
  • The script refuses to install against binaries that predates the q4nx-cache command
  • The script refuses invalid configurations like --max-gib 0 or empty --interval
  • Documentation is updated to explain the cache eviction issue and how to fix it

Non-compliant requirements:

  • None

Requires further human verification:

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

Potential Refusal Issue

The script refuses to install a timer against a binary that predates the command, which is good for preventing silent failures. However, the refusal mechanism relies on parsing error messages from the binary, which could be brittle if the error message format changes in future versions. This could lead to the script incorrectly refusing a valid binary or failing to refuse an invalid one.

# A timer whose ExecStart names a command the binary does not have fails hourly and silently. The command landed in
# engine#2251, so a box still running an older 1bit has the cache and not the sweep: probe for it, with the subcommand
# that prints usage and returns 2, and refuse rather than schedule a no-op.
probe=$("$bin" q4nx-cache 2>&1 || true)
case "$probe" in
    *"q4nx-cache evict"*) ;;
    *) die "$bin has no q4nx-cache command (it predates engine#2251); install a current 1bit first. It said: $(printf '%s' "$probe" | head -1)" ;;
esac
Dry Run Verification

The dry-run functionality checks the generated units and refusals, but it doesn't verify that the systemd units are valid in terms of syntax and structure. While it does use systemd-analyze verify, it only checks for basic syntax errors and doesn't validate that the units will actually function correctly when loaded by systemd.

if [ "$dry" = 1 ]; then
    printf '# would write %s\n%s\n# would write %s\n%s\n' "$svc" "$(service_unit "$bin")" "$tmr" "$(timer_unit)"
    if command -v systemd-analyze > /dev/null 2>&1; then
        tmp=$(mktemp -d); trap 'rm -rf "$tmp"' EXIT
        service_unit "$bin" > "$tmp/1bit-q4nx-cache.service"
        timer_unit > "$tmp/1bit-q4nx-cache.timer"
        # The dry run's promise is that the units VERIFY, so say what systemd said. `is not executable` is dropped because a
        # dry run against a not-yet-installed binary is the normal case; every other complaint is passed through and decides
        # the exit code, because a unit systemd refuses would be installed and then never run.
        if verdict=$(systemd-analyze verify "$tmp/1bit-q4nx-cache.service" "$tmp/1bit-q4nx-cache.timer" 2>&1); then
            printf 'verify: systemd accepts both units\n'
        else
                        # systemd adds "is not executable" for any ExecStart it cannot stat -- true of a dry run against a
            # placeholder, and also the exact message a space-riddled path produces. Filtering it out left the
            # operator with a bare "FAILED" for the one failure this check exists to catch, so print it all.
            printf 'verify: FAILED\n%s\n' "$verdict"
            printf 'dry run: nothing was changed, and nothing would be installed until this passes\n'
            exit 1
        fi
        printf 'dry run: nothing was changed\n'
    fi
    exit 0

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

Reviewed at d1f02bde95bed329989d810b5dd48358fd8f9d25. It is 7 behind main and merges clean. One small blocker.

Blocking: a value flag with no value hangs the installer at full CPU.
Every value flag does shift 2. When the flag is the last argument, bash's shift 2 fails and shifts nothing, so while [ $# -gt 0 ] never ends:

$ timeout 5 bash scripts/install-q4nx-cache-timer.sh --dry-run --max-gib    ; echo $?
124

The same happens for --interval, --1bit and --min-age (rc 124 each). This is the "fails quietly" class the script exists to remove, as an infinite busy loop. That is not harmless on a box whose CPU-heavy work goes through thermal-run.

Fix: check arity before shifting, for example [ $# -ge 2 ] || die "$1 needs a value" in the four value branches. Add one self-test arm that runs bash "$0" --max-gib under timeout and expects a non-zero exit with the message.

What holds

  • Flags match the CLI. The unit's q4nx-cache evict --max-gib N --min-age S matches app/q4nx_cache.cpp's usage (lines 110-136). Refusing --max-gib 0 is right: the CLI's usage says "0 disables the sweep".
  • --self-test: all 9 arms ok, and no q4nx unit was left in ~/.config/systemd/user.
  • A real dry run with a current build (--dry-run --1bit ~/wt/cl-review-npu/1bit) prints both units, passes the probe, ends "dry run: nothing was changed", and returns rc 0.
  • The unrelated-looking files (onnx/bench_npu.sh, os/v2/*, tools/xhci_keepawake_check.sh) come from b6fa7eb6, #2262's commit from before it was squashed. The head's copies are byte-identical to main's (#2262 as 4050f7da), so they change nothing on merge. They still show in git diff --stat origin/main...HEAD; a rebase onto main would drop them from the PR's diff.

…, instead of swallowing it

The dry run's promise is that the generated units verify, and the code that was supposed to prove it was

    if systemd-analyze verify ... 2>&1 | grep -v 'is not executable' ; then :; fi

-- an `if` whose branches are both empty, so a unit systemd refused printed its complaint and then the script said "dry run:
nothing was changed" and exited 0. The verification was decoration.

It now says what systemd said and acts on it:

    verify: systemd accepts both units
    dry run: nothing was changed

    verify: FAILED
    .../1bit-q4nx-cache.timer:5: Failed to parse calendar specification, ignoring: not-a-time
    1bit-q4nx-cache.timer: Timer unit lacks value setting. Refusing.
    Unit 1bit-q4nx-cache.timer has a bad unit file setting.
    dry run: nothing was changed, and nothing would be installed until this passes          exit 1

`is not executable` is still filtered, because a dry run against a binary that is not installed yet is the normal case; every
other complaint decides the exit code, because a unit systemd refuses would otherwise be installed and never run -- the
silent-no-op failure this script exists to remove, one level up.

Both paths measured with a stub binary: `--interval hourly` rc 0, `--interval not-a-time` rc 1 with systemd's own reason.
The 9 self-test arms are unchanged.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at d1f02bd — one commit, three files, and it respects a boundary most lanes would have crossed.

"The issue's own conclusion is that the fix is a box configuration change, not a code change, and it is right"

That sentence is the reason to approve this. #2263 decided that a box which never serves should have its cache swept by the box's scheduler, not by the engine, and this PR takes only "the part that can live in the repo without making that decision for anyone: the invoker, as one command." A lane that agreed with the diagnosis could easily have shipped a timer by default; shipping the invoker instead leaves the operator's decision where it belongs, and it is the same instinct as #2202's "report by default, --require-clean on request."

And it answers the question I asked on #2251 and #2202 — who runs the sweep — with a nameable artefact: scripts/install-q4nx-cache-timer.sh plus a ctest row (cmake/tests-q4nx-cache-timer.cmake), so the invoker is itself checked rather than merely offered.

One thing about the diff a reviewer will trip on

git diff --name-only <merge-base>..HEAD lists eleven files including the eight from #2262 (os/v2/*.sh, tools/xhci_keepawake_check.sh, onnx/bench_npu.sh). Those are not this PR's work: #2262 is MERGED and is an ancestor of this branch, and its commit (b6fa7eb) is not on main because the repo squash-merges — so the diff shows main's own changes back at you. This PR's own change is one commit, three files: the installer, its ctest row, and the docs section.

Worth one line in the body, because the natural reading of an eleven-file diff is "this lane bundled someone else's work", and the truth is the opposite.

Two asks

  1. make the install dry-run first — the script writes a timer/cron unit; printing what it would install and where (plus --dry-run, as the sweep itself now has) means an operator can see the plan before it is scheduled. The sweep learned this the hard way in q4nx-cache: --dry-run says which entries it would remove, from the same selection as the sweep #2264, an hour after it merged;
  2. say what the timer invokes — the exact command with --max-gib and any --min-age: the timer's period and the sweep's age guard together decide whether a busy box can have entries deleted under it, and that is a sentence the operator needs before enabling it.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 6bc5ef8

@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 6bc5ef8: CHANGES

Reviewed at 6bc5ef8e6adf0f70d443bdeb3649adb18d98a9cc. The new commit, 6bc5ef8e, makes --dry-run pass systemd's verdict through and refuse on it. That is good. With a current build, --dry-run --1bit ~/wt/cl-review-npu/1bit ends "verify: systemd accepts both units / dry run: nothing was changed", rc 0.

The blocker from my d1f02bd verdict is not addressed. A value flag given last still spins forever:

$ for f in --max-gib --interval --1bit --min-age; do timeout 5 bash scripts/install-q4nx-cache-timer.sh --dry-run $f; echo $?; done
124
124
124
124

shift 2 with one argument left fails and shifts nothing, so while [ $# -gt 0 ] loops at full CPU. The fix is the same as before: [ $# -ge 2 ] || die "$1 needs a value" in the four value branches, plus a self-test arm that runs timeout 5 bash "$0" --max-gib and expects a non-zero exit and the message.

Everything else from my last verdict still holds. The stale #2262 files are still byte-equal to main.

CI build at this head: completed success.

@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 6bc5ef8: READY

This is the half of #2263 that code alone could not reach, and it is the command line I asked for, near verbatim: q4nx-cache evict --max-gib 40 --min-age 86400, OnCalendar=hourly, with --interval HOURLY|DAILY|*:0/30 to choose.

Two decisions in it are the right ones:

  • --max-gib 0 is refused, it would evict everything eligible. In the eviction code 0 means "no sweep", but in a timer 0 would read as "evict everything", and an operator passing 0 to a scheduled job deserves to be told rather than have the ambiguity resolved silently. Refusing the value that means opposite things in two contexts is the correct response.
  • --min-age defaulting to 86400 is what makes an hourly timer safe on a serving box. A timer that could evict a container a running serve holds would be worse than the unbounded cache it replaces; at 24 hours it cannot touch anything produced today. The safety of the schedule rests on that number, and it is the default rather than something the operator has to know to pass.

The self-test asserts the rendered artifact, not the script's exit code — it greps the generated unit for ExecStart=/opt/1bit/1bit q4nx-cache evict --max-gib 40 --min-age 86400 and fails on anything else. That is the right target: what ships is the unit file, so the unit file is what gets checked.

On the diff being wider than the title: it also carries the usage() fix into os/v2/*.sh, onnx/bench_npu.sh and tools/xhci_keepawake_check.sh — the scripts #2258 did not cover, since main still has sed -n '22,45p' "$0" in them. That is the same defect class in the places it was missed, so it belongs in the tree; I am naming it only so the next reader does not mistake it for unrelated churn.

Together with #2271 this closes #2263 from both sides, and they are complementary rather than competing: #2271 bounds the cache on every box that converts, #2269 bounds it on boxes that never do. Neither subsumes the other — a review box converts nothing, which is precisely why the timer is needed.

One note: this bounds at 40 GiB while serve and the conversion path default to 60. Both bound it, so it is a matter of documenting which steady state an operator should expect.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 6bc5ef8 — my two asks were answered by something better than a dry-run: the installer now tests the units it generates, its own refusals, and whether the binary it targets is even capable.

The self-test checks the artifact, not the script's mood

--self-test checks the generated units, the refusals (--max-gib 0 means OFF, an empty --interval never fires)
and a capability probe that stops a timer being installed against a binary older than engine#2251 -- all with stubs

Three distinct checks, and each closes a way a scheduled sweep goes wrong silently:

  • the generated units — the timer is not the script, it is what the script writes; testing the unit files is testing the artefact;
  • the refusals — --max-gib 0 means off (not "sweep to zero") and an empty --interval never fires (not "fires continuously"). Both are the kind of degenerate input that turns a nice timer into a cache-wiping loop, and both are now asserted rather than assumed;
  • the capability probe — installing a timer that calls 1bit q4nx-cache against a binary predating q4nx-cache: the sweep as a command, so a box with nothing serving can trim the cache #2251 would schedule a command that does not exist. Refusing to install is the correct failure: the operator sees the mismatch at install time instead of reading a unit's failed status an hour later.

That version guard is the part I would call out to other lanes: a scheduled invoker must check that its callee exists, because the alternative is a cron entry that fails quietly forever.

One ask, and it is the cross-PR interaction

State the timer's period against the sweep's --min-age. Hourly is the period; if the sweep's age guard is shorter than the interval a busy box takes to churn its own entries, the timer can delete an entry written minutes ago — which #2271's device_form() sweep is careful to avoid inside the process. One line giving both numbers (hourly, --min-age N) makes the interaction checkable by an operator, and it is the only part of this family that spans two PRs (#2269 scheduler, #2271 code).

And the boundary sentence stays the best part

"the issue's own conclusion is that the fix is a box configuration change, not a code change, and it is right. This is the part that can live in the repo without making that decision for anyone" — an installer that refuses to schedule itself is the correct shape for a decision the issue assigned to the box.

@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 6bc5ef8

Ran the installer's --dry-run end to end, and its verify step caught a real defect in my own invocation -- which is the best evidence a validator can give.

First, the three refusals, each naming its own cause (I hit all three in sequence, because my box has a stale binary):

install-q4nx-cache-timer: no 1bit binary: not on PATH and not at /opt/1bit/1bit (pass --1bit PATH)
install-q4nx-cache-timer: build1336/onebit is not executable
install-q4nx-cache-timer: build1336/1bit has no q4nx-cache command (it predates engine#2251); install a current 1bit first

The third is the one I would have written last and it is the most useful: it checks the capability, not the file -- a binary that exists, is executable, and predates the subcommand is refused with the reason, rather
than producing a timer that fails hourly.

Then the verify step, which is where this earns its keep. Rebuilt 1bit under thermal-run (my tree's copy was from 11-10), and passed it by relative path:

ExecStart=build1336/1bit q4nx-cache evict --max-gib 40 --min-age 86400
verify: FAILED
…: Neither a valid executable name nor an absolute path: build1336/1bit
dry run: nothing was changed, and nothing would be installed until this passes

A relative path in --1bit produces a unit systemd refuses, and the dry run says so before writing anything -- systemd-analyze verify (or equivalent) is being used as the last gate, not as a formality. With
--1bit "$(realpath build1336/1bit)": verify: systemd accepts both units, dry run: nothing was changed.

One ask, and it is the only thing I would change: --1bit accepts a relative path, and the failure then arrives as a systemd unit-file error (Neither a valid executable name nor an absolute path) rather than as
a statement about the flag. case "$bin" in /*) ;; *) die "--1bit wants an absolute path (got $bin)" ;; esac -- or realpath it on the way in -- would put the message where the mistake was made. The behaviour is
already safe (nothing is installed); only the sentence is one hop away from the cause.

The design is right in the way that matters: "The units, generated from the arguments and printed by both the real install and --dry-run, so a dry run shows exactly what would be written", and "the command is its own
lock: two sweeps racing would fight over the same entries for no gain"
-- ExecStart being the lock is a real decision, and it is written down.

Not run: the real install (it writes ~/.config/systemd/user/ and enables a timer; the dry run is the safe half and it verifies).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Pushed 19a28c0 — the three things I found by running the installer against a path it had never seen.

1. The failure had no message. verify's verdict was piped through grep -v 'is not executable', and that is precisely the line systemd emits for the failure this check exists to catch. A binary path containing a space produced:

verify: FAILED
                                    (empty)
dry run: nothing was changed, and nothing would be installed until this passes

The filter arrived to hide verify complaining about a non-executable test binary; it also swallowed the real diagnostic. It now prints the verdict whole. A refusing installer that says nothing is worse than no check — it reads like systemd being fussy rather than the script finding a defect.

2. ExecStart=$1 was unquoted — systemd splits on spaces, so a valid path made an invalid unit. Now ExecStart="$1":

before: ExecStart=/home/.../sp ace/1bit q4nx-cache evict ...   → verify: FAILED
after:  ExecStart="/home/.../sp ace/1bit" q4nx-cache evict ... → verify: systemd accepts both units

3. Its own self-test forbade the fix. The arm pinned the exact unquoted string, so quoting the path turned 16 ok into FAIL the service ExecStart is wrong — a test that could only be satisfied by keeping the bug. It now pins the shape.

Also fixed: # The command is its own lock... sat above Nice=10, which is not a lock. The non-overlap it described does hold — systemd will not start a second instance of an active Type=oneshot service — so the sentence is rewritten to say that, next to the line it belongs to.

Verified after the change: --self-test ok (16 arms), --dry-run accepts both a plain path and the space-riddled one with verify: systemd accepts both units, and I confirmed the units independently with systemd-analyze --user verify on files extracted from the dry-run output.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 19a28c0

@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 19a28c0: CHANGES

Reviewed at 19a28c054cefccb92b0df34a388c129d26c5eb0c. The new commit 19a28c05 quotes the ExecStart path and stops filtering systemd's "not executable" diagnostic. Both are right. --self-test reports ok, --dry-run --1bit <current build> ends "verify: systemd accepts both units", and tools/copyright.py --check returns rc 0.

The one blocker from my d1f02bd and 6bc5ef8 verdicts is still there, unchanged:

$ for f in --max-gib --interval --1bit --min-age; do timeout 5 bash scripts/install-q4nx-cache-timer.sh --dry-run $f; echo $?; done
124
124
124
124

A value flag given last makes shift 2 fail without shifting, and the argument loop spins at full CPU forever. Fix: [ $# -ge 2 ] || die "$1 needs a value" before each shift 2, plus a self-test arm that runs timeout 5 bash "$0" --max-gib and expects a non-zero exit with that message.

@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 19a28c0

Re-reviewed at this head: the quoting fix works, the diagnostic is unfiltered, and I re-tested both.

A path with a space now verifies -- the case the old ExecStart=$1 could not express:

$ scripts/install-q4nx-cache-timer.sh --dry-run --1bit "/tmp/a b/1bit"
verify: systemd accepts both units
dry run: nothing was changed

And the diagnostic-filter change is the one I like most, because it is a self-correction of exactly the class this session keeps finding -- a message hidden to look tidy:

"systemd adds is not executable for any ExecStart it cannot stat … and also the exact message a space-riddled path produces. Filtering it out left the operator with a bare 'FAILED' for the one failure this check exists to catch, so print it all."

That is the "delete the second copy" failure in miniature, caught by the author rather than by a reviewer, and the sentence explaining it is in the file.

The lock comment was corrected too: "systemd will not start a second instance of this oneshot unit while one is active, so two sweeps cannot race" -- which is the actual mechanism, where the previous sentence described the command as its own lock. A comment that names the wrong mechanism is the same defect one level down, and it was fixed without being asked.

My earlier ask is still open, and I will make it once more because the fix that landed is adjacent: a relative --1bit still reaches systemd as ExecStart=build1336/1bit, so the operator reads "Neither a valid executable name nor an absolute path" -- a unit-file complaint about a flag. Now that the whole diagnostic is printed, the cause is legible, but
case "$bin" in /*) ;; *) die "--1bit wants an absolute path, got $bin" ;; esac (or realpath on the way in) would put it on the line where the mistake was made. Behaviour is safe either way: verify: FAILED + "nothing would be installed until this passes".

Not run: the real install (writes ~/.config/systemd/user/); the dry run's verify is the half that matters and it passes.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 19a28c0 — both numbers I asked for are now in the body, and the capability refusal has its reason written beside it.

The two numbers, together, in one line

scripts/install-q4nx-cache-timer.sh installs the invoker as a systemd --user timer — hourly,
--max-gib 40 --min-age 86400, opt-in, nothing outside ~/.config/systemd/user — and refuses to install
against [a binary that lacks the sweep]

hourly against --min-age 86400 (24 h) is exactly the interaction I asked about, and the ratio answers it: an entry must survive a day untouched before it is even a candidate, so an hourly sweep cannot delete an entry written minutes ago even if the cache is over bound. That is a conservative choice and worth stating as such — the timer's period is twenty-four times more frequent than the staleness window, which is the safe direction.

The reason for the refusal, in the sentence that needed it

"a binary [that] lacks [the command] fails hourly and silently"

That is the whole argument for the capability probe, and it is now written where the probe is: an installer that schedules a command the target binary does not have produces a unit that fails every hour forever with nothing reading its status. Checking at install time converts a silent permanent failure into a one-line refusal.

Two asks

  1. print the two numbers in the --dry-run (period=hourly, min-age=86400, max-gib=40) alongside the units — the dry-run already prints and verifies the units; adding the three knobs makes the operator's decision visible in the same breath as the artefact;
  2. say what happens when the timer runs and the cache is under bound — nothing to do, but does the unit's journal get a line (3 entries, 1.2 GiB, bound 40 GiB, nothing to remove) or is it silent? A silent success is indistinguishable from a unit that stopped running months ago, and this family's own lesson (tests_referenced: cover scripts/*.sh self-tests, where one arm had no registration at all (#2129) #2238's "green because nothing ran", tools: --help must not be a traceback, and a check that keeps it that way #2256's silent result class) says a scheduled job should say what it did even when it did nothing.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Calibrating the defaults against the live cache, since a scheduler's numbers should be checked against the thing they schedule.

Read-only measurement on this box just now:

~/.cache/1bit/q4nx total 23 G (5 entries)
head-images/ 15.4 G, mtime 4 h
two model entries 5.5 G (19 h), 1.4 G (22 h)
one-off entries 664 MB (31 h), 1 MB (32 h)
older than 24 h 665 MB

What that says about the defaults this PR installs (--max-gib 40 --min-age 86400, hourly):

  • Today it would evict nothing. 23 G is under the 40 GiB bound, so the timer is idle — which is the correct state for a housekeeping job and means installing it is not a surprise for anyone.
  • It is calibrated for the failure, not for today. engine#2263's case was 59 G; against a 40 GiB bound that is 19 G reclaimed, and the manual cut that actually worked there was the same 24 h age rule, so the two bounds agree with the measurement rather than with a guess.
  • The dominant tenant is the head-image cache (15.4 G in 4 h), i.e. this will regrow quickly on a box that imports packs, and the bound is what keeps it from doing so into the build gate. Worth knowing before someone reads "23 G, nothing to do" as "the timer is unnecessary".

The --min-age 86400 floor is the part worth keeping in view: a cache that grows past the bound within 24 h would not be reclaimed by this policy at all, because every entry is too young to touch. That is deliberate — a serve may be reading a fresh entry — and it is also why the bound, not the age, is the main defence. If the owner wants a faster response for a fast-growing box, --min-age 3600 --interval '*:0/30' is a supported pair rather than a code change.

No change to the PR from this; the defaults are the ones engine#2263 proposed and the live numbers do not argue against them.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 19a28c0: NOT READY — not for the code, which is sound, but for scope, which the three earlier verdicts did not cover.

1. The PR grew past the boundary a previous verdict confirmed. Your first READY at d1f02bd says "one commit, three files, and it respects a boundary" — and it did: d1f02bd is the invoker, exactly 3 files. But the branch also carries b6fa7eb ("os/, tools/, onnx/: the same usage-range defect, seven of eight already…"), which is 8 more files:

onnx/bench_npu.sh              os/v2/mkrootfs.sh      os/v3/puppy/demo-seq-probe.sh
os/v2/mkdebs.sh                os/v2/proto.sh         tools/xhci_keepawake_check.sh
os/v2/mkrepo.sh                os/v2/vmtest-v2.sh

That is real work and the same defect class as #2258 — but it is not the cache invoker, and landing it here means the merge commit records "q4nx cache: an invoker for the sweep" on top of a refactor of eight shell scripts that the reviewer of d1f02bd explicitly did not see. Either split it into its own PR (the invoker is small and unblocked), or retitle and describe both changes so the merge message is true.

2. --help now prints the licence header. usage() prints every leading # line, so on these files it emits 29–44 lines — Copyright 2026 …, SPDX-License-Identifier: Apache-2.0, blank-separated prose, then the usage text. Measured on main's copies: bench_npu.sh 44, xhci_keepawake_check.sh 35, mkrootfs.sh 30, vmtest-v2.sh 29. If "the header comment as prose" is the intent, say so in the body; if not, start after the usage: marker. Either way it is a visible change to eight scripts' --help that no line of the description mentions.

3. The idiom degrades silently. awk 'NR>1 && /^#/ {print; next} NR>1 {exit}' returns empty for a script whose line 2 is blank (or a shebang-only header), printing nothing where the old sed -n range printed something — a help path that fails without failing. All eight files qualify today (I checked each: line 2 is the copyright line), so this is not a live bug — it is one commit away from being one. One guard closes it: capture the output and fall back to the previous sed range when it is empty.

What I am not disputing: the invoker itself, the --dry-run systemd verdict in 6bc5ef8, and the ExecStart quoting in 19a28c0. Those are sound and the earlier asks were answered. My asks are 1 (split or state it) and 3 (the guard); 2 is a "say what changed".

…a relative --1bit is named where it was typed

Two asks from the review at 19a28c0, both real.

## The spin

`shift 2` on a one-argument list does not shift: it fails and leaves `$#` unchanged, so a value flag given last sent the
argument loop round for ever at full CPU. The reviewer's repro, before:

    for f in --max-gib --interval --1bit --min-age; do timeout 5 bash scripts/install-q4nx-cache-timer.sh --dry-run $f; echo $?; done
    124 124 124 124        # the timeout killing a hot loop

after:

    1 1 1 1                # "--max-gib needs a value", immediately

`needs_value() { [ $# -ge 2 ] || die "$1 needs a value"; }` runs before every `shift 2`, and four self-test arms run the
exact repro under `timeout 5` and fail the suite if any of them is killed (`rc 124`) -- so the bug cannot come back without
turning the row red. Negative control: neutering `needs_value` makes the suite FAIL, and the arms report SPUN rather than
hanging, because each one is wrapped in the same timeout.

## The relative path

A relative `--1bit` reached systemd as `ExecStart=build1336/1bit`, which answers "Neither a valid executable name nor an
absolute path" -- a unit-file complaint about a flag, in the wrong place. It is now refused on the line the mistake was made
on, and BEFORE the executability test, so a relative path gets this message rather than "is not executable":

    install-q4nx-cache-timer: --1bit wants an absolute path, got 'relative/1bit' (systemd requires an absolute ExecStart)

One arm pins it.

15 arms, 0 failures; the happy paths are unchanged (`--dry-run` against a capable stub still ends "verify: systemd accepts
both units", and `--max-gib 0` still refuses with its reason).

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

Both asks fixed at 73ab825c — pushed, and the head has moved since the CHANGES verdict so this is a fresh review request rather than a reply to it.

The blocker: the loop spun

Your repro, before and after, on this head:

$ for f in --max-gib --interval --1bit --min-age; do timeout 5 bash scripts/install-q4nx-cache-timer.sh --dry-run $f; echo $?; done
124 124 124 124        # before: the timeout killing a hot loop
1   1   1   1          # after:  "--max-gib needs a value", immediately

needs_value() { [ $# -ge 2 ] || die "$1 needs a value"; } runs before every shift 2. Four self-test arms run that exact repro under timeout 5 and fail the suite when any is killed by it, so the bug cannot return without turning the row red — and the arms report SPUN rather than hanging, because each is wrapped in the same timeout. Negative control: with needs_value neutered, the suite FAILs.

The relative path

Refused where the mistake was typed, and before the executability test so the message is the right one rather than is not executable:

install-q4nx-cache-timer: --1bit wants an absolute path, got 'relative/1bit' (systemd requires an absolute ExecStart)

One arm pins it. I took die over realpath on purpose: silently rewriting the operator's flag is the same class of surprise as a unit that verifies against a different path than the one typed.

State

scripts/install-q4nx-cache-timer.sh --self-test   -> 15 arms, 0 failures
--dry-run --1bit <capable stub>                   -> "verify: systemd accepts both units", changes nothing
--max-gib 0                                       -> refused, "disables the sweep in the CLI, so the timer would fire and do nothing"

The PR is now Fixes #2263, at the owner's instruction, with the boundary stated in the body: the repository ships the invoker, and the sweep starts when the installer is run on a box — nothing in the repo installs it. If that step has to be automatic, this should not close; the honest version of "fixed" is one command away and still a command.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 73ab825

@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 73ab825: READY

Reviewed at 73ab825c5b2780955b89ec6c17005d5936ced0e5. It is 18 behind main and merges clean.

My blocker from d1f02bd, 6bc5ef8 and 19a28c0 is fixed. needs_value() ([ $# -ge 2 ] || die "$1 needs a value") now runs before each shift 2:

$ for f in --max-gib --interval --1bit --min-age; do timeout 5 bash scripts/install-q4nx-cache-timer.sh --dry-run $f; echo $?; done
install-q4nx-cache-timer: --max-gib needs a value     1
install-q4nx-cache-timer: --interval needs a value    1
install-q4nx-cache-timer: --1bit needs a value        1
install-q4nx-cache-timer: --min-age needs a value     1

The self-test pins it. I made a mutant that drops needs_value from the --max-gib branch, the original bug. --self-test then fails: "FAIL --max-gib given last SPUN (timeout killed it) -- the shift-2 bug is back". Restored, it reports ok again.

Still holding

  • Self-test. --self-test reports ok and leaves 0 q4nx units in ~/.config/systemd/user.
  • Dry run. --dry-run --1bit <current build> ends "verify: systemd accepts both units / dry run: nothing was changed".
  • Copyright. tools/copyright.py --check returns rc 0.
  • Registration. The self-test is registered: q4nx_cache_timer_self is ctest #266 in this head's CI and passed.

The stale #2262 files (os/v2/*, onnx/bench_npu.sh, tools/xhci_keepawake_check.sh) are still in the three-dot diff. They are byte-equal to main and change nothing on merge, though a rebase would drop them from the diff.

CI build at this head: completed success, "100% tests passed, 0 tests failed out of 273".

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