[bong-water-water-bong]

Refs #2263. This is the code half. The scheduler half is #2269 (pkg lane), and the two do not overlap.

What was wrong

The Q4NX cache has three writers and only one evictor. npu/q4nx_device.cpp's device_form() writes entries for the forward route, but only serve start (app/serve.cpp) and 1bit q4nx-cache evict ever call q4nx_cache_evict(). So a box that converts through the forward route and never starts a serve grows past ONEBIT_Q4NX_CACHE_MAX_GIB with no sweep at all. The issue's census notes that npu/ could not call the sweep without including app/.

Change

• The sweep moves below both callers. q4nx_cache_evict, q4nx_cache_candidates, q4nx_dir_bytes, q4nx_cache_bound_bytes, q4nx_cache_touch and their structs move unchanged from app/q4nx_cache.{h,cpp} to npu/q4nx_cache_sweep.{h,cpp}. Every removed line appears unchanged in the new files. They build as onebit_q4nx_cache, a static library that onebit, onebit_npu (PUBLIC) and q4nx_cache_test link, so each binary carries one copy. app/q4nx_cache.h keeps the 1bit q4nx-cache command and includes the sweep header.
• device_form() sweeps after a new conversion is renamed into place. It uses the same bound (ONEBIT_Q4NX_CACHE_MAX_GIB, default 60, 0 = no sweep) and the same age window, and never removes the entry it just wrote. Its reuse path now calls q4nx_cache_touch() in place of a copy of the same call.
• Docs: docs/npu.md's cache-bound paragraph says the conversion sweeps too, and that 1bit q4nx-cache evict remains the manual trigger for a box that writes nothing.

What this does not do: a box that writes nothing still never sweeps, which is #2269's case. The bound stays 60 GiB, and lowering it is a policy call. The 59 G in the issue was under the 60 GiB bound.

Tests

• npu_pack_import_test has two new arms. In both, a 2 GiB older entry (a sparse file, so no real disk) sits in the cache past the age window.
  • Under ONEBIT_Q4NX_CACHE_MAX_GIB=1, a new conversion removes it and keeps its own entry. The log line is removed 1 entry, 2.0 GiB -> 0.0 GiB (bound 1.0 GiB, …).
  • Under =0 it is kept.
• Mutant: with the new q4nx_cache_evict call removed from device_form(), the test returns rc 1 with exactly one FAIL, cache sweep: a new conversion over a 1 GiB bound removes the older 2 GiB entry. Restored, it returns rc 0.
• q4nx_cache_test passes all 32 arms against the moved code.
• Full build (172 targets) is green. ctest -j 6 passes 267 of 267, with skips, excluding npu_device_lock, device_holders and device_route. The NPU is off, and this build has no NPU runtime (ONEBIT_NPU=OFF). tools/copyright.py --check returns rc 0.

🤖 Generated with Claude Code

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2263 - Partially compliant Compliant requirements: The Q4NX cache sweep logic has been moved to npu/q4nx_cache_sweep.cpp so it can be used by npu/q4nx_device.cpp The device_form() function now calls q4nx_cache_evict() after writing new entries Documentation in docs/npu.md has been updated to reflect that the forward route also triggers cache sweeps Non-compliant requirements: The scheduler half (systemd timer or equivalent) is not implemented in this PR (referenced as q4nx cache: an invoker for the sweep, because nothing sweeps a box that never serves (#2263) #2269) Requires further human verification: Verification that the cache sweep actually works as expected on hardware Verification that the cache bound is properly enforced (60 GiB default)

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Cache Sweep Logic The cache sweep logic is now correctly invoked by device_form() after writing new entries, which addresses the issue where boxes converting through the forward route but never starting a serve would grow past the cache bound. The sweep uses the same bound (ONEBIT_Q4NX_CACHE_MAX_GIB) and age window as the existing evictor, ensuring consistent behavior. // engine#2263: this is a WRITER of the cache, so it bounds the cache like serve start does (app/serve.cpp). Before // this only serve start and `1bit q4nx-cache evict` ever swept, so a box that converted through the forward route // and never started a serve grew past the bound with no sweep at all. Same bound (ONEBIT_Q4NX_CACHE_MAX_GIB, 60 GiB // by default, 0 = no sweep), same age window, and never the entry just written. q4nx_cache_evict(root, dir, q4nx_cache_bound_bytes(std::getenv("ONEBIT_Q4NX_CACHE_MAX_GIB"))); Code Duplication The cache sweep implementation was duplicated from app/q4nx_cache.cpp to npu/q4nx_cache_sweep.cpp. While this is necessary to avoid circular dependencies, it introduces code duplication that should be carefully monitored for consistency. The duplicated code should be reviewed to ensure it remains in sync with the original implementation. // Copyright 2026 bong-water-water-bong // SPDX-License-Identifier: Apache-2.0 // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 // // Unless required by applicable law or agreed to in writing, software // distributed under the License is distributed on an "AS IS" BASIS, // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. #include "q4nx_cache_sweep.h" #include <algorithm> #include <cerrno> #include <chrono> #include <cstdio> #include <cstdlib> #include <filesystem> #include <vector> namespace fs = std::filesystem; int64_t q4nx_dir_bytes(const std::string& path) { std::error_code ec; if (!fs::is_directory(path, ec)) return 0; int64_t total = 0; for (auto it = fs::recursive_directory_iterator(path, fs::directory_options::skip_permission_denied, ec); !ec && it != fs::recursive_directory_iterator(); it.increment(ec)) { std::error_code fec; if (it->is_regular_file(fec)) total += int64_t(fs::file_size(it->path(), fec)); if (fec) continue; } return total; } std::vector<Q4nxCacheCandidate> q4nx_cache_candidates(const std::string& root, const std::string& keep, int64_t max_bytes, int64_t min_age_seconds) { std::vector<Q4nxCacheCandidate> out; std::error_code ec; if (max_bytes <= 0 || !fs::is_directory(root, ec)) return out; struct Ent { std::string path; int64_t bytes; fs::file_time_type mtime; }; std::vector<Ent> all; for (auto it = fs::directory_iterator(root, fs::directory_options::skip_permission_denied, ec); !ec && it != fs::directory_iterator(); it.increment(ec)) { Ent e; e.path = it->path().string(); std::error_code tec; if (!it->is_directory(tec)) continue; e.bytes = q4nx_dir_bytes(e.path); if (!keep.empty() && (e.path == keep || fs::path(e.path).filename() == fs::path(keep).filename())) continue; e.mtime = fs::last_write_time(it->path(), tec); if (tec) continue; all.push_back(std::move(e)); } if (all.empty()) return out; const auto now = fs::file_time_type::clock::now(); const auto min_age = std::chrono::seconds(min_age_seconds); std::vector<Ent*> cands; for (auto& e : all) if (now - e.mtime > min_age) cands.push_back(&e); std::sort(cands.begin(), cands.end(), [](const Ent* a, const Ent* b) { return a->mtime < b->mtime; }); int64_t after = 0; for (const auto& e : all) after += e.bytes; for (Ent* e : cands) { if (after <= max_bytes) break; out.push_back({fs::path(e->path).filename().string(), e->bytes}); after -= e->bytes; } return out; } 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; fs::remove_all(fs::path(root) / cand.name, rec); if (rec) continue; r.bytes_after -= cand.bytes; ++r.removed; } if (r.removed > 0) { r.reported = true; std::fprintf(stderr, "note: q4nx cache: removed %d entr%s, %.1f GiB -> %.1f GiB (bound %.1f GiB, %s); entries younger than " "%lld s and the one just written are kept (engine#1956)\n", r.removed, r.removed == 1 ? "y" : "ies", double(r.bytes_before) / (1 << 30), double(r.bytes_after) / (1 << 30), double(max_bytes) / (1 << 30), root.c_str(), (long long)min_age_seconds); } return r; } int64_t q4nx_cache_bound_bytes(const char* text, int64_t default_gib) { const int64_t max_gib = INT64_MAX >> 30; // the largest GiB count whose byte count still fits long long gib = default_gib; if (text && *text) { char* end = nullptr; errno = 0; const long long v = std::strtoll(text, &end, 10); if (end != text) gib = v; // a number (ERANGE saturates, which the clamp below handles) } if (gib <= 0) return 0; if (gib > max_gib) gib = max_gib; return int64_t(gib) << 30; } void q4nx_cache_touch(const std::string& entry) { std::error_code ec; if (fs::is_directory(entry, ec)) fs::last_write_time(entry, fs::file_time_type::clock::now(), ec); }

[bong-water-water-bong]

VERDICT: READY at 503e36f — "three writers and only one evictor" is the whole fault in six words, and the fix is in the writer that was leaking.

The asymmetry, named

"The Q4NX cache has three writers and only one evictor. npu/q4nx_device.cpp's device_form() writes entries for the forward route, but only serve start (app/serve.cpp) and 1bit q4nx-cache [evict]…"

So the conversion path creates cache entries during normal forward work and never bounds them: a box that runs forward but never serves grows until something else sweeps. And the placement of the fix is right — sweep after the new conversion is renamed into place, using the same bound (ONEBIT_Q4NX_CA…) as the other two paths, so there is still one rule with three call sites rather than a second policy for the device path (the same discipline as #2251's "calling the same function so there is no second set of rules").

The test does the disk-efficient thing

"a 2 GiB older entry (a sparse file, so no real disk) sits in the cache" — two new arms in npu_pack_import_test.

That is how a size-bound test should be written: the apparent size is what the bound reads, and a sparse file gives it 2 GiB of apparent size for no disk. On a box that has spent the evening between 142 and 226 GiB free, that detail is not pedantry.

Two asks

1. say whether the sweep holds the writers' lock — the body notes npu_device_lock is excluded from the ctest run, so a lock exists; a sweep triggered inside device_form() is presumably under it, but the CLI sweep (q4nx-cache: the sweep as a command, so a box with nothing serving can trim the cache #2251) and the timer (q4nx cache: an invoker for the sweep, because nothing sweeps a box that never serves (#2263) #2269) run outside any forward. One line stating which lock (if any) serialises a sweep against an in-flight device_form() answers the one genuinely dangerous case across all three PRs in this family — the interplay I asked about on q4nx-cache: the sweep as a command, so a box with nothing serving can trim the cache #2251 and that no single one of them can answer alone;
2. state the --min-age default alongside the bound: an entry renamed into place a second ago must not be a candidate even if the cache is over bound, or a busy box thrashes its own hottest entries. If the age guard already exists, one line naming its default closes it.

And the split with #2269 is honest

"This is the code half. The scheduler half is #2269 (pkg lane), and the two do not overlap." Verified: the only shared file is docs/npu.md (both add a section), so the claim holds for code and needs only a re-base of one doc file — not the "no overlap" claim being repaired after a conflict.

[bong-water-water-bong]

VERDICT: READY -- the move is clean, the new evictor protects its own entry by construction, and the one thing I would carry forward is a note my #2264 review already asked about

The "moved unchanged" claim holds where it matters. Comparing the line sets: of 130 removed lines, exactly five are not re-added verbatim, and each is accounted for -- two target_link_libraries lines that now go through onebit_q4nx_cache, a comment whose reference changed from app/q4nx_cache.h to q4nx_cache_sweep.h, a reflowed doc sentence, and fs::last_write_time(dir, ...) which became q4nx_cache_touch(dir). That last one is a small improvement rather than a drift: the touch now runs behind if (fs::is_directory(entry, ec)), so a name that is not a directory no longer has its timestamp written. Everything else in the sweep -- the candidate selection, the LRU ordering, the bound arithmetic, the age window -- is byte-identical.

The new evictor is safe by construction, which is the part that could have been dangerous. A conversion now deletes other lanes' entries, so the one invariant that must hold is that it cannot delete its own:

q4nx_cache_evict(root, dir, q4nx_cache_bound_bytes(std::getenv("ONEBIT_Q4NX_CACHE_MAX_GIB")));

dir -- the entry just renamed into place -- is passed as keep, and q4nx_cache_candidates() skips by full path or by filename, so the entry is protected even if the root spelling differs. The age window (600 s by default) protects the other fresh entries as a second line of defence. Same bound and same env var as serve start (0 = no sweep), so the two writers cannot disagree about policy.

One copy of the code, three consumers is the right shape: onebit_q4nx_cache is a static library linked by onebit, onebit_npu (PUBLIC, so its dependents get the header) and q4nx_cache_test. A second copy of the sweep would have been the drift that #2264 exists to prevent.

The two new arms pin exactly the two properties: an older 2 GiB sparse entry is removed while the just-written entry survives, and with the bound at 0 nothing is removed. Using a sparse file so the test costs no disk is the detail that makes it runnable in CI.

What I would carry forward (it is #2264's subject, not this PR's, and this PR only relocates the line): the sweep's own note is still "%.1f GiB -> %.1f GiB (bound %.1f GiB)" while the CLI summary uses the MiB/GiB formatter, so a small cache prints 0.0 GiB from the library and 2.0 MiB from the CLI. Since the library is now shared by three binaries and is the only output a conversion-only box gets, that line matters more after this move than it did before it -- worth one size() call in the same file.

And the honest scope is stated: a box that writes nothing still never sweeps, which is #2269's case, and the 60 GiB bound stays a policy call. Both are the right things to leave out of a code-only change.

[bong-water-water-bong]

VERDICT at 503e36f: READY

This solves the problem better than the fix I proposed on #2263, by putting the seam on the correct side of the link. I had suggested extracting app/q4nx_cache.cpp into a third onebit_q4nx_cache target so both onebit and onebit_npu could link it. Moving the sweep down into npu/ achieves the same thing with no new target: npu/q4nx_device.cpp calls it as a local, and app/q4nx_cache.cpp — which sits in onebit, which already links onebit_npu — calls down. One fewer target, and no chance of the two ever disagreeing about which direction to depend.

The thing I checked hardest is that #2264's guarantee survived the move, because relocating a selection function is exactly how a dry-run drifts from its action:

app/q4nx_cache.cpp       0 fs::remove_all, 1 q4nx_cache_candidates()   // the --dry-run
npu/q4nx_cache_sweep.cpp 1 fs::remove_all, 2 q4nx_cache_candidates()   // inside q4nx_cache_evict

One removal site, inside the one function, using the one selection — so --dry-run still reports exactly what the action does, and now there is a single implementation of that rule in the tree rather than a copy.

The safety property is explicit rather than incidental, which is what I would have asked for:

q4nx_cache_touch(dir);
q4nx_cache_evict(root, dir, q4nx_cache_bound_bytes(std::getenv("ONEBIT_Q4NX_CACHE_MAX_GIB")));

dir — the entry this conversion just wrote — is passed as keep. So a conversion cannot evict the container it just produced, and there is no write→evict→write loop. The touch before it puts the new entry last in LRU order. Leaning on min_age alone would have made that a timing assumption; this makes it a fact.

It also reads the same ONEBIT_Q4NX_CACHE_MAX_GIB knob as serve, so the two paths cannot disagree about the bound.

And it removes the property that produced #2263: every box that converts — import, container, the device tests, benches — now bounds the cache as it writes, instead of waiting for a serve that a review box never starts.

One note, not a blocker: the default bound here is 60 GiB while #2269's timer defaults to 40. Not a conflict — both bound it — but the effective steady state between timer runs is the looser of the two, which is worth a line in docs/npu.md so an operator is not surprised by the difference.