[bong-water-water-bong]

Refs #2263. A follow-up to #2271 (merged), for the part of the cache that #2271's sweep still could not reach.

Problem

On this box ~/.cache/1bit/q4nx/head-images/ held 16 G of the 23 G cache: 36 keys, 424 tiles. The sweep counted the whole of head-images/ as one entry. Every load touches that directory, so it was always "recent" and never removed. The cache-size bound did not cover the biggest consumer.

Change

• Each head-image key is its own entry. Under head-images/, the sweep groups the headimg-<16hex>-tNNN.q4hi files by key. A group's size is the sum of its tiles, and its age is its newest tile. LRU eviction removes the oldest keys' tiles one file at a time, and keeps the container and the other keys.
• A hit marks its tile. q4nx_head_tile_cache.cpp sets the tile's mtime on a cache hit, so a key in use is not evicted.
• A load that needs no conversion still sweeps. npu-run and forward-serve call q4nx_cache_sweep_after_load() after a successful init. Before this change, only the device-form conversion swept (npu: the device-form conversion sweeps the Q4NX cache it writes (refs #2263) #2271).
• One root. q4nx_cache_default_root() checks ONEBIT_Q4NX_CACHE, then $HOME/.cache/1bit/q4nx, then the passwd home. serve's repack, the forward and 1bit q4nx-cache with no --root all use it, so the command sweeps the directory the writers fill.
• docs/npu.md is updated in the same PR.

Proof

• q4nx_cache_test: 40 of 40 arms pass (8 are new: per-key grouping, its size and age, eviction leaving the other key and the container, root order, and the command's default root).
• Mutants:
  • Removing the per-key branch fails 5 arms.
  • Taking the root from CACHE_DIR instead fails 3 arms.
• npu_pack_import and copyright pass.
• Host-only end to end (NPU_INFER_HOST_GEMM=1, run under strace with 0 /dev/accel opens):
  • The scratch cache held 3 keys, 4.0 GiB, bound 2 GiB. Two keys were 2 h and 3 h old; one was new.
  • 1bit npu-run -n 1 printed removed 2 entries, 4.0 GiB -> 1.6 GiB (bound 2.0 GiB).
  • Only the recent key's 2 tiles were left.

Overlap

#2275 (the size formatter) edits the same files. Whichever of the two merges second needs a small rebase. Neither PR changes the other's behaviour.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at f6b29a3

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 60204a2)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2263 - Partially compliant Compliant requirements: The cache is now swept per key, not as one entry, preventing unbounded growth on boxes that never serve The command now sweeps the directory that writers fill ($ONEBIT_Q4NX_CACHE, else ~/.cache/1bit/q4nx) A load that needs no conversion still sweeps, ensuring cache eviction happens even when no device conversion occurs The sweep after load is now called from forward_serve.cpp and serve.cpp, ensuring eviction happens on load Non-compliant requirements: The PR does not include a scheduler (systemd --user timer or equivalent) to run the command hourly Requires further human verification: The scheduler configuration (systemd timer) needs to be set up outside of this PR

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Head Image Grouping The implementation correctly groups head-image files by key, treating each key as a separate cache entry. This prevents the entire head-images directory from being treated as a single entry, which was causing the cache to grow unboundedly on boxes that never serve. The grouping logic correctly sums bytes and uses the newest file's timestamp for age calculation. namespace { struct HeadGroup { std::string path; int64_t bytes = 0; fs::file_time_type mtime{}; std::vector<std::string> files; }; // The key a head-image file belongs to: "headimg-<16 hex>" from "headimg-<16 hex>-t<tile>.q4hi" (or its ".tmp.<pid>" temp). // Any other name is its own key, so a file this sweep does not recognise is still counted and still evictable. std::string head_image_key(const std::string& name) { static const std::string pre = "headimg-"; const size_t n = pre.size() + 16; if (name.size() > n && name.compare(0, pre.size(), pre) == 0 && name[n] == '-') return name.substr(0, n); return name; } // The groups of one head-images directory: bytes summed over a key's files, age = its newest file (a hit marks its tile used, // npu/forward/src/q4nx_head_tile_cache.cpp), so a key a serve is still loading from is inside the age window. std::vector<HeadGroup> head_image_groups(const fs::path& dir) { std::vector<HeadGroup> out; std::vector<std::string> keys; std::error_code ec; for (auto it = fs::directory_iterator(dir, fs::directory_options::skip_permission_denied, ec); !ec && it != fs::directory_iterator(); it.increment(ec)) { std::error_code fec; if (!it->is_regular_file(fec)) continue; const int64_t b = int64_t(fs::file_size(it->path(), fec)); if (fec) continue; const auto t = fs::last_write_time(it->path(), fec); if (fec) continue; const std::string key = head_image_key(it->path().filename().string()); size_t i = 0; while (i < keys.size() && keys[i] != key) ++i; if (i == keys.size()) { keys.push_back(key); HeadGroup g; g.path = (dir / key).string(); g.mtime = t; out.push_back(std::move(g)); } HeadGroup& g = out[i]; g.bytes += b; if (t > g.mtime) g.mtime = t; g.files.push_back(it->path().string()); } return out; } } // namespace File Removal Logic The removal logic now correctly handles head-image keys by removing individual tile files rather than the entire directory. This ensures that when a key is evicted, only its tile files are removed, leaving other keys and the container directory intact. This is a critical change for proper cache eviction. Q4nxCacheSweep q4nx_cache_evict(const std::string& root, const std::string& keep, int64_t max_bytes, int64_t min_age_seconds) { Q4nxCacheSweep r; r.bytes_before = q4nx_dir_bytes(root); r.bytes_after = r.bytes_before; // One selection, shared with `--dry-run`: the list is what this function removes, in this order. const std::vector<Q4nxCacheCandidate> cands = q4nx_cache_candidates(root, keep, max_bytes, min_age_seconds); for (const auto& cand : cands) { std::error_code rec; if (!cand.files.empty()) { // A head-image key: its tile files (and any temp a crashed writer left under the same key), not the directory. bool any = false; for (const auto& f : cand.files) { std::error_code fec; if (fs::remove(f, fec)) any = true; } if (!any) continue; } else { fs::remove_all(fs::path(root) / cand.name, rec); if (rec) continue; } r.bytes_after -= cand.bytes; ++r.removed; } if (r.removed > 0) { Root Resolution Logic The root resolution logic in q4nx_cache_default_root() now correctly prioritizes ONEBIT_Q4NX_CACHE over HOME/.cache/1bit/q4nx, but the previous implementation used ONEBIT_Q4NX_CACHE_DIR and $XDG_CACHE_HOME which were not used by writers. This change ensures consistency with how writers resolve the cache root, fixing an inconsistency where the command would sweep a directory that no writer actually used.

✅ Resolved findings

app/q4nx_cache.cpp:194-202

Root Resolution Logic

The root resolution logic in q4nx_cache_default_root() now correctly prioritizes ONEBIT_Q4NX_CACHE over HOME/.cache/1bit/q4nx, but the previous implementation used ONEBIT_Q4NX_CACHE_DIR and $XDG_CACHE_HOME which were not used by writers. This change ensures consistency with how writers resolve the cache root, but the PR description mentions that the command used to resolve ONEBIT_Q4NX_CACHE_DIR and $XDG_CACHE_HOME which no writer reads, so this change fixes that inconsistency.

[bong-water-water-bong]

VERDICT: READY at f6b29a3 — the biggest consumer was unsweepable because the evictor saw a directory where the writers saw 424 tiles, and the in-use answer is recency rather than a lock.

The granularity asymmetry, with the numbers

"~/.cache/1bit/q4nx/head-images/ held 16 G of the 23 G cache: 36 keys, 424 tiles. The sweep counted the whole of head-images/ as one entry."

So three quarters of the cache was a single unsweepable item — the sweep could not trim it even when over bound, because "one entry" is smaller than the bound by definition. That is the same shape as #2270's key granularity and #2262's line ranges: the unit of accounting was coarser than the unit of use, and the coarser unit made the bound unenforceable exactly where it mattered most.

The in-use question — asked three times in this family — is answered by recency

"A hit marks its tile. q4nx_head_tile_cache.cpp sets the tile's mtime on a cache hit, so a key in use is not stale"

That is the right mechanism, and better than the lock I had been asking about: touching on read makes "in use" and "recently useful" the same signal, so a sweep can never delete something a live serve just read, without any lock ordering to get wrong and no writer starving.

And the demonstration is the discriminating case:

scratch cache: 3 keys, 4.0 GiB, bound 2 GiB — two keys 2 h and 3 h old, one new
1bit npu-run -n 1  ->  removed 2 entries, 4.0 GiB -> 1.6 GiB (bound 2.0 GiB)

The new key survived. A test that only showed "over bound → smaller" would pass with a sweep that deleted everything; this one shows the rule's choice, which is the part that can hurt.

Two asks

1. state the mtime granularity against the sweep's --min-age — a hit marks a tile, but filesystem mtime resolution (and relatime/noatime mounts) decide how finely "just used" is visible. One line saying the comparison is against the sweep's own --min-age (86400 in q4nx cache: an invoker for the sweep, because nothing sweeps a box that never serves (#2263) #2269's timer) and that resolution is irrelevant at that scale closes the only way this could still delete a live key;
2. say what a partial key looks like — 36 keys and 424 tiles means a key is a directory of tiles; if a sweep lands between a conversion writing tiles and its rename-into-place, is the partial directory visible to the sweep at all? Naming the boundary (only complete keys are candidates) makes that impossible rather than unlikely.

And "one root" is the quiet fix worth having

q4nx_cache_default_root() checking ONEBIT_Q4NX_CACHE → $HOME/.cache/1bit/q4nx → … in one place means the sweep, the writers and the timer (#2269) cannot disagree about which cache they are bounding — the precondition for every other claim in this family.

[bong-water-water-bong]

VERDICT at f6b29a3: READY

Reviewed at f6b29a32d4cabd561c90d032854ea8daf19d52b6. It is on main (0 behind) and merges clean. The diff is 8 files, +220/-30.

Checked

• Per-key grouping. head_image_key takes headimg-<16hex> off …-tNNN.q4hi and its .tmp.<pid> temps. Any other name is its own key, so it is still counted and still evictable. A group's size is the sum of its files, and its age is its newest file.
• Eviction removes files, not the directory. It removes the key's files (temps included) and leaves head-images/. Plain entries still use remove_all as before.
• A hit marks its tile. q4nx_head_tile_cache.cpp sets the tile's mtime on a hit, so a key in use stays inside the age window. q4nx_cache_sweep_after_load() passes no keep and relies on that window: a load's own entries were just touched, and the device-form reuse path touches its entry too (#2271).
• The command's default root. It now follows ONEBIT_Q4NX_CACHE → $HOME → passwd. It no longer reads ONEBIT_Q4NX_CACHE_DIR or XDG_CACHE_HOME, which git grep shows nothing else reads, and q4nx_cache_test pins that neither moves it.
• Tests. q4nx_cache_test passes all 40 arms and npu_pack_import_test passes. tools/copyright.py --check returns rc 0.

My mutant: I aged a key by its oldest tile (t < g.mtime). It fails "a key is aged by its newest tile (only the old key and the container qualify)". Restored, all 40 arms pass again.

Note (not blocking). "One root" is true today because two copies of the rule agree, not because there is one. npu/q4nx_device.cpp:42 cache_root() (used by the device-form conversion and q4nx_forward.cpp:6468's head-image dir) and the new npu/q4nx_cache_sweep.cpp q4nx_cache_default_root() are line-for-line the same: ONEBIT_Q4NX_CACHE, HOME, passwd. The only difference is that one throws and the other returns empty. Having cache_root() call q4nx_cache_default_root() (and throw on empty) would make the writer and the sweeper unable to drift apart again.

The end-to-end npu-run sweep (removed 2 entries, 4.0 GiB -> 1.6 GiB) is the author's run; I did not repeat it.

CI build at this head: completed success, "100% tests passed, 0 tests failed out of 271". q4nx_cache is test #169 there and passed.

[bong-water-water-bong]

ready for review at 60204a2 — rebased onto main after #2275 merged. The one conflict was tests/q4nx_cache_test.cpp: both sides are kept (#2275's size-text arms, then this PR's 8 arms), and it now runs 44 of 44 arms. npu_pack_import and copyright pass. The sweep's note and the CLI use #2275's q4nx_cache_size_text; this PR adds no GiB-only print. Code and behaviour are otherwise unchanged from f6b29a3.

[github-actions]

Persistent review updated to latest commit 60204a2

[bong-water-water-bong]

VERDICT: READY at 60204a2 — the shared resolution point is now stated in the code, and the test drives the same path the command does.

"One resolution … shared with 1bit q4nx-cache evict"

// engine#2263: one resolution (app/q4nx_cache.h), shared with `1bit q4nx-cache evict`, so the command sweeps thi[s]

That is the sentence that makes the whole family coherent: the writers (device_form()), the in-process sweep after a load (#2271), the CLI sweep (#2251) and the timer (#2269) all resolve the cache root through one function. Anything else would eventually bound one directory and write another.

And the test uses the real interface rather than a parallel one:

std::vector<std::string> a = {"evict", "--max-mib", "3", "--min-age", "0"};

--min-age 0 in a test is the right way to make the age guard not obscure the size rule, and driving evict through its own argv means the arm exercises the command an operator runs.

One ask, carried from my last review, now with a reason to be specific

What does the sweep see during a conversion? A key is a directory of tiles (36 keys, 424 tiles); if a load is midway through writing a new key and a sweep runs — in-process after another load, or from the timer — is a partial key a candidate? --min-age 0 in the test means the age guard is not doing that work in the fixture, so the property needs to be stated: either partial keys are written under a temporary name and renamed into place (so the sweep never sees them), or they are visible and their mtime is fresh enough to survive. One line naming which turns a plausible reading into a checked one.

[bong-water-water-bong]

VERDICT at 60204a2: READY

Reviewed at 60204a2c3349c4c0ae43eb1f27e14658a0142296.

This head is f6b29a3 (my READY) rebased onto main after #2275 (the size formatter) merged.

• The PR's own change did not move, except where it meets #2275. I sorted the +/- lines of each head's diff against its merge-base and compared them. The only difference is the test's summary line: the arm count goes to 44 (main's 36 plus this PR's 8), and "size text" joins the list.
• The merge is clean. There are no conflict markers outside third_party, and the PR merges clean (0 behind).
• Tests. q4nx_cache_test reports "44 arm(s) over the sweep (…, size text, head images per key, one root)", and npu_pack_import_test passes.

My f6b29a3 review carries over: per-key grouping, the age-by-newest-tile mutant failing its arm, and the dropped env vars that nothing reads. So does its non-blocking note: cache_root() and q4nx_cache_default_root() are still two copies of one rule.

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