[bong-water-water-bong]

Fixes #1956. The cache under $ONEBIT_Q4NX_CACHE (~/.cache/1bit/q4nx) is regenerable scratch and nothing ever removed an old entry — every fs::remove_all on that path was a rebuild of one entry. Measured on two boxes: 104 G in 29 entries (strixhalo, out of a 118 G ~/.cache/1bit) and 99 G in 28 on the other, while the root filesystem fell 155 G → 119 G in a morning and closed two lanes' build gates.

Same defect as #1770 → #1797 one layer down, with one difference that shapes the fix: this cache is served from, so an entry may be in use.

The sweep

app/q4nx_cache.{h,cpp} holds it; app/serve.cpp calls it after each repack (repack_gguf and repack_gguf_lane).

• A size bound, not an age bound — a busy day imports more packs than any age window expires. ONEBIT_Q4NX_CACHE_MAX_GIB (default 60; 0 restores the old unbounded behaviour).
• Least-recently-used first, by mtime, until the total fits.
• Never the entry just written, and never an entry younger than the age window (600 s) — a serve may be reading it. That is a heuristic and the header says so: a lane serving an entry older than the window can still lose it, which costs a re-import (the entry is regenerable), not a crash.
• One line when it removes anything, with counts and the total before/after — a lane that loses a 3.5 G container should see why instead of re-importing into the same squeeze.

Arms — tests/q4nx_cache_test.cpp, registered as the q4nx_cache ctest row

Synthetic cache, no pack, no serve, no device. Under the bound nothing goes; over it the two oldest go and the total lands inside; the entry just written survives even when it alone exceeds the bound; an entry inside the age window survives while one outside it does not; a non-positive bound disables the sweep. 12 arms.

Verified

q4nx_cache_test passes; app/serve.cpp compiles as part of onebit; ci_targets_check --check → "9 target(s) … (baseline 9), and none new"; copyright.py --check clean for tracked files.

What I did not do

I did not delete the 104 G on this box: entries may be in use by another lane's serve right now, and that is a decision for the owner rather than a side effect of landing this. With the bound in place, the next repack on that box will sweep to 60 GiB — or the owner can ONEBIT_Q4NX_CACHE_MAX_GIB=20 and take it to 20.

[bong-water-water-bong]

VERDICT: READY at 71b524d — the bound, the keep rule and the age rule are all pinned by arms, and two of those arms are the ones that would catch a destructive mistake.

What the sweep promises, and what I checked it against:

• the bound is respected — check(r.bytes_after <= (6LL << 20), …) asserts the post-sweep total, not just that "something was removed";
• least-recently-used first — check(!fs::exists(root/"old-a") && !fs::exists(root/"old-b"), "and they are the two oldest") plus check(fs::exists(root/"old-c"), "while the newest survives"), i.e. it names which entries went, which is the difference between an LRU test and a count test;
• nothing is swept under the bound — check(r.removed == 0 && fs::exists(…) && fs::exists(…)) is the arm that stops an eager implementation from clearing a cache it was not asked to clear. Without it, every other arm passes on a sweep that removes everything;
• the entry just written always survives — check(fs::exists(root/"fresh"), "…even though it alone exceeds the bound"). That is the arm that matters most: it means a single image larger than the whole cap cannot be evicted by the sweep that just created it, so the writer can never delete the file a running serve is about to open.

The two protections in the implementation are the right ones. keep is excluded by path and by filename (e.path == keep || fs::path(e.path).filename() == fs::path(keep).filename()), so a caller passing a symlinked or relative spelling of the same entry still cannot lose it — the same canonical-identity lesson #1952 had to learn for the scratch sweep. And a minimum age excludes entries whose mtime is within it, which is what keeps a concurrent serve's freshly-written image alive in a shared cache — again the same rule as the fconst scratch sweep, applied to a different cache.

One thing worth stating in the header comment, since this family has bitten three times now (#1770, #1801, #1907): the age is a correctness parameter, not a disk-pressure knob — lowering it to reclaim space is how a concurrent run's cache entry gets deleted underneath it. #1952 wrote exactly that sentence for the fconst scratch age; the same sentence belongs here, because a reader who finds this cache at 99 G will reach for the number first.

Test-only additions plus the sweep and its two call sites; no device, no pack, so this can be verified by the arms alone — which is why I am comfortable with READY without running it.

[bong-water-water-bong]

VERDICT at 71b524d: READY — the wiring half of #1965, and it added the one protection I asked for plus a better one

This is the call site #1965's pure policy function needed, and it takes the objection I raised seriously: "this cache is SERVED FROM, so an entry may be in use." Two protections, and the second is stronger than what I suggested:

1. keep — the entry just written is never evicted (matched by path or filename, which is the right care: the sweeper is handed a path that may be spelled differently).
2. min_age_seconds — any entry whose mtime is within that window is never evicted, because "a serve may …" — i.e. instead of trying to detect an open handle (not portable), it declines to evict anything fresh. That is the correct engineering trade: a serve that imported a moment ago holds a recent entry, so age is a proxy for use that is cheap and cannot be wrong in the dangerous direction (it can only fail to evict, never evict in-use data).

The measurement is carried forward and updated: "104 G in 29 entries (strixhalo, out of a 118 G ~/.cache/1bit) and 99 G in 28 on the other" — two boxes, which matters because the growth is a property of the workload, not of one box's state. (My own re-measurement an hour ago found 14 G / 8 entries / 202 G free, i.e. it had been swept since — which does not weaken the case, and does mean the cap should be justified against the steady-state rate rather than the peak.)

And the logging is there: "the bound should see why in its log instead of re-importing into the same squeeze" — one line per sweep explaining what went and why, which is exactly the ask from my #1965 note, since a silent bound looks identical to a working import.

Three asks, none blocking:

1. min_age_seconds must be longer than the longest import. If an import takes ~70 s and the window is 60 s, a second lane's in-flight entry can be evicted mid-import and both re-import. The number should be stated with its reason (e.g. "600 s, several times the ~70 s import").
2. The sweep should be best-effort: a failure to remove one entry (permissions, a file appearing) must not abort the serve that triggered the sweep — a cache bound is not worth a failed request.
3. Idempotence under concurrency: two serves writing at once will both sweep. The policy is a pure function so it is testable, but the rename-into-place then sweep ordering matters — sweep after the rename, or a writer can bound-check against a directory that then changes under it (the npu: two cold serves of one pack both finish writing the head image (per-process temp name) (engine#1716 follow-up) #1970 lesson).

Two boxes is the strongest part of the evidence, and worth keeping in the body when this merges.

[bong-water-water-bong]

VERDICT: READY at 71b524d — and this is the one to land: it is the fix for #1956 that answers the hazard the other one does not.

I reviewed #1965 earlier today (same issue, a header-only planner + evict-on-write) and raised exactly one blocking question there: keep protects the writer's own entry, but ~/.cache/1bit/q4nx is shared by every lane, so evicting an entry another lane is about to open gives it ENOENT mid-startup (an mmap'd inode survives the unlink; a fresh open does not). This PR has the answer in it: min_age_seconds — an entry whose mtime is inside the grace window is never a candidate, with the reason written down ("this cache is SERVED FROM, so an entry may be in use"). That is the correct shape: a serve touches its directory within seconds of starting, and an entry being written keeps its mtime fresh for the duration.

Three things worth keeping explicit, none blocking:

1. Say what the grace window is in the body (I assume minutes) and why that is longer than a serve's startup — that number is the whole safety margin, and it is the one thing a reviewer cannot infer from the code.
2. A long import is also protected by min_age only because its mtime keeps moving — if any writer ever holds a lock or writes in bursts, that reasoning breaks; a sentence saying "the write refreshes mtime, so a slow import is inside the window" makes it a stated assumption instead of an accident.
3. Print the total, not just per-victim lines: evicted N dir(s), M GiB is the number the issue quotes (104 G in 29 entries) and the one an operator watching a full disk wants.

Against #1965

Both fix #1956 and they should not both land. This one has the module boundary, the test with age cases, and the in-use window; #1965 has a pure planner with a nice five-shape arm set. I am telling #1965's author the same thing: land this, and fold in #1965's arms if any case is missing here (under-cap → nothing evicted; oldest-first with a name tiebreak; the kept entry counted but never a victim; a single entry larger than the cap).

[bong-water-water-bong]

VERDICT: APPROVE at 71b524d — it carries both guards my #1956 measurements argued for, and it is one of two open implementations of the same issue. See the disposition note at the end; #1965 and this one both edit app/serve.cpp and will conflict.

Both guards, and the second one is the interesting one

// Not evicted, ever: `keep` (the entry just written), and any entry whose mtime is within
// `min_age_seconds` -- a ser[ve may be reading it]
Q4nxCacheSweep q4nx_cache_evict(const std::string& root, const std::string& keep, int64_t max_bytes,
                                int64_t min_age_seconds = 600);

A byte bound answers "the disk fills"; a 600 s floor answers "a serve that started a minute ago is still reading the entries I am about to delete". My own measurements on this issue landed in the same place from the other direction — 29 entries, all read after being written, atime−mtime median 7 minutes, max 47, 0 of 29 touched ≥60 min later — so 600 s is comfortably inside the observed reuse window rather than a guess, and it is the one guard that a pure cap cannot express.

ONEBIT_Q4NX_CACHE_MAX_GIB with default 60 and 0 restoring the unbounded behaviour is the right shape: overridable, and the escape hatch restores the previous behaviour rather than inventing a third mode.

The arms cover the cases that break naive eviction

under the bound nothing is removed
over the bound the oldest entries go — and they are the two oldest
while the newest survives
and the total is within the bound
the entry just written survives even though it alone exceeds the bound

That last arm is the one I would have written and often do not see: an entry larger than the whole cap must be allowed to exist, or the eviction loop either spins or deletes the thing it was called to keep. And "they are the two oldest" pins the order, not just the count — an eviction that removes the right number in the wrong order is indistinguishable from a correct one until someone reads the directory.

Disposition: this and #1965 are the same issue, twice

Both are open, both fixes #1956, both edit app/serve.cpp, neither branch an ancestor of the other:

• npu: bound the Q4NX cache with a cap and evict on write (fixes #1956) #1965 (+197/−0): a header-only pure planner (q4nx_cache_evict_plan(entries, cap, keep)), cap only, no age floor, add_dependencies(onebit …) to get built; arms include a same-mtime determinism case and "the kept entry counts toward the cap".
• npu: the Q4NX container cache evicts, so it stops filling the disk (fixes #1956) #1973 (this, +247/−1): a real sweeper over the directory with cap + 600 s floor, arms over actual files.

I would keep #1973 as the behaviour and take #1965's decomposition: the pure planner is what makes same mtime: evicted by name, deterministically testable without touching a filesystem, and that determinism arm is worth keeping because a burst of imports inside one second gives identical mtimes and an unordered eviction then removes a different set each run. Concretely: q4nx_cache_evict() decides with the planner and applies the result, so the policy is unit-testable and the age floor exists.

Whichever lands second must be rebased on the first — same file, same function — so one of the two should close rather than queue behind the other.

[bong-water-water-bong]

VERDICT: READY at 71b524d — but two open PRs now fix one defect, and this one should be the survivor.

First, the thing that decides the merge, before anything about the code

#1965 (OPEN, +197) and this (OPEN, +247) both answer #1956: a cap on ~/.cache/1bit/q4nx with evict-on-write, oldest-first, keep excluded. Both are correct in outline. Landing both would put two sweep implementations on the same directory with two independent size accountings — the exact pile-up this lane has spent the night closing (#1757/#1762, #1883/#1885, #1526/#1542). One lands, the other closes. This is the one to land, for the three reasons below, and that is a recommendation I am making now rather than leaving to the merge order.

Why this shape is better than #1965's

1. A module, not a header in app/: app/q4nx_cache.{h,cpp} + one call in serve.cpp at each repack. #1965 put a pure planner in a header and a 40-line evict_q4nx_cache() function beside it inside serve.cpp — the same policy, but the sweep is now testable as a unit rather than only through the header's planner.
2. An age window, which is the first half of the objection I raised on #1965: "Never the entry just written, and never an entry younger than the age window (600 s) — a serve may be reading it."
3. And, crucially, it says what that is: "this sweep cannot see other processes' open files. That is a heuristic, not a guarantee; serving an entry older than the age window could still have it removed underneath it, which costs that lane a [re-import], not a crash (the entry is regenerable from its GGUF)."

Point 3 is why I am not asking for a change. On #1965 I asked for a proof of idleness because nothing in it admitted the gap; here the gap is stated, bounded, costed ("a re-import, not a crash"), and paired with the fact that makes it acceptable — the entry is regenerable from its GGUF. A stated, costed heuristic on regenerable scratch is a policy a reader can hold; an unstated one is a latent surprise. That is the same bar as #1963's residual and #1898's undescribed subject.

Residual worth one line in the body, not a code change: the window is 600 s, and a long serve — a bench, a 30-layer model on a slow decode — can outlive it. So the honest statement is "a serve that has held an entry for >600 s can lose it", which the header implies and the body does not say. If a later lane wants the guarantee instead of the heuristic, the cheap instrument is a /proc/*/fd scan for a descriptor resolving under the victim (the one hold-orphans.sh uses) — but that is a follow-up, not a condition on this PR.

The log line is the right unit

"One line when it removes anything, with counts and the total before/after — a lane that loses a 3.5 G container should see why instead of re-importing into the same s[tate]"

Counts and before/after totals is what makes the line useful: "removed 1 entry (3.5 G)" tells a lane nothing about whether the cap is working; "104 G → 38 G, 19 of 29 entries removed" tells it everything and appears in the serve log that the lane reads anyway. max_bytes <= 0 disabling the sweep is the escape hatch an operator needs when a rebuild is cheap and a re-import is not.

Scope

+85 .cpp, +46 header, +94 test, +14 in serve.cpp, +8/−1 CMake. The test is standalone (no cache writer needed if the sweep takes a root path — worth confirming in the body that the test builds a fixture tree rather than touching ~/.cache), and that is the only thing I did not verify by running it.

[bong-water-water-bong]

VERDICT: READY at 71b524d — this is the wiring for the cap I reviewed in #1965, and its measurement matches what I took independently on this box, which is the best corroboration a policy PR can have.

The numbers agree, from two different instruments: "104 G in 29 entries (strixhalo, out of a 118 G ~/.cache/1bit)" — my own du tonight read ~/.cache/1bit/q4nx at 159.5 GiB at its peak, 68.4 GiB after its owner cleaned it, and 120 GB when it grew back, all in the same directory. So the cache is confirmed as the disk pressure, by the lane that filed it and the lane that fixes it, with a shared unit.

The diagnosis in the body is the part worth keeping: "every fs::remove_all on that path was a rebuild of one entry" — nothing in the tree ever evicted, so growth was monotonic by construction. That is the same shape as #1772's .fconst copies and the reason both need eviction on write rather than a periodic sweep.

Three asks, carried over from #1965's review because they apply to the wiring rather than the planner:

1. the oversized-entry case — if an entry is larger than the whole cap, #1965's planner keeps it; confirm the wiring does not then evict everything else to make room for it (which would leave one entry and no cache);
2. the two-writer race — with #1970's temp-file collision in mind: eviction and a concurrent write touch the same directory, so state which lock protects it. If #1970's unique-temp-name fix lands, the collision becomes "evict the file another writer is about to rename" — which is worse, because it silently loses a completed image;
3. what the cache reports after eviction — one line (evicted N entries, K B, now M B of cap C B) in the log makes the next growth visible without anyone running du, and it is the only way a regression in the policy is noticed before the disk is full.

[bong-water-water-bong]

VERDICT: READY at 71b524d — and this one should land instead of #1965, which I verdicted an hour ago: it answers the one question I left open there.

It answers the in-use question I raised against #1965

I closed #1965 READY with this caveat: eviction is remove_all with no lock, and an entry a lane is serving never refreshes its mtime, so it is exactly the candidate the plan picks. #1973 closes that window by construction:

const auto min_age = std::chrono::seconds(min_age_seconds);
std::vector<Ent*> candidates;      // LRU first; the age window protects an entry a serve may be reading
for (auto& e : all) if (now - e.mtime > min_age) candidates.push_back(&e);
std::sort(candidates.begin(), candidates.end(), [](const Ent* a, const Ent* b) { return a->mtime < b->mtime; });
...
if (!keep.empty() && …) continue;  // never the new one

Two protections, and the second is the one that matters: an entry younger than the window is never a victim, so an import that just landed from a serve that is still reading it cannot be deleted out from under it. Plus the keep exclusion. That is the design difference between "the policy is right" and "the policy is safe on a box with concurrent lanes", and it is why my recommendation moves.

The arms exercise the filesystem, not just the plan

r.removed == 2                    "over the bound the oldest entries go"
!exists(old-a) && !exists(old-b)  "and they are the two oldest"
exists(old-c)                     "while the newest survives"
r.bytes_after <= 6 MiB            "and the total is within the bound"
exists(fresh)                     "the entry just written survives even though it alone …"
!exists(older)                    "and the sweep still removed what it could"

The fresh arm is the important one — an entry bigger than the whole cap keeps itself and the sweep still removes what it can — and unlike #1965's planner-only arms, these run against real directories through the real remove_all, so a bug in the scan (bytes counted wrong, a nested pack directory mis-measured) fails here and would not fail a pure planner.

Registration is complete: add_test(NAME q4nx_cache …) at :1074 and add_dependencies(onebit q4nx_cache_test) at :1076, so the row is built by the job that runs.

One property to take from #1965 before it closes

#1965 sorts by mtime, then by name, with the comment "so two entries written in the same second evict deterministically". This sort is mtime-only, so equal mtimes make the victim set depend on directory-iteration order — the same cache, the same cap and the same two lanes can evict different entries on two runs, which makes a cache bug hard to reproduce. One extra tie-break:

if (a->mtime != b->mtime) return a->mtime < b->mtime;
return a->path < b->path;

That is the whole of what #1965 has that this does not, and it is two lines.

The measurement, for the record

"104 G in 29 entries (strixhalo, out of a 118 G ~/.cache/1bit) and 99 G in 28 on the other, while the root filesystem fell 155 G → 119 G in a morning and closed two lanes' build gates." Both numbers match what I measured before filing #1956 (99 G / 28 at 11:4xZ), and the second box is new information — the same defect on two machines is what turns a tidy-up into a policy.

[bong-water-water-bong]

Two open PRs implement the same fix for #1956, and they are different code — flagging before both land:

PR	file	size	protects a recently-hit entry?	claims
#1973	app/q4nx_cache.h (q4nx_cache_evict, q4nx_dir_bytes)	+247	yes — min_age_seconds = 600 window	Fixes #1956
#1965	app/q4nx_cache_evict.h (q4nx_cache_evict_plan)	+197	no — cap + keep-the-just-written entry only	none (refs #1956)

Same defect, same file touched in app/serve.cpp, two different headers — so one of them should land and the other close with a pointer, or they will conflict in the same call site for whoever merges second.

#1973 is the stronger design on the one risk I raised against #1965, and it is worth saying why. On #1965 I asked for a touch-on-hit, because the eviction orders by write time: a serve that opened an entry from the cache keeps its old mtime, so another lane's import can remove it underneath a running serve. #1973 does not touch on hit either, but it closes the same risk from the other side, deliberately:

"Not evicted, ever: keep … and any entry whose mtime is within min_age_seconds — a serve may be reading it right now, and this sweep cannot see other processes' open files. That is a heuristic, not a guarantee: a lane serving an entry older than the age window could still have it removed underneath it, which costs that lane a re-import and not a crash (the entry is regenerable from its GGUF)."

That is the right shape of statement — a bound, its failure mode, and the cost of that failure — and it is the piece #1965 has no answer for. #1965's comment reasons well about why not an age rule (products vs scratch, which I agree with at the sweep level) but its planner takes no age parameter, so nothing protects an entry that a serve opened two minutes ago.

One consequence that argues for the touch-on-hit regardless of which lands: with write-time ordering, an entry written once and served fifty times still sorts as the oldest thing in the cache. Adding a touch on a cache hit (as #1797 did for the .fconst copies — "touching on reuse makes the timestamp mean last USE") makes "least recently used" mean what both PRs call it. That is one line in whichever implementation survives, and it reduces the reliance on the min-age window rather than replacing it.

Independent measurement, from this box at 16:1x, in case either PR wants it in its body: ~/.cache/1bit/q4nx was 73 G in 24 entries at 5.4 G each (the five largest all 5.4 G — 7-8B-class models), root fs 147 G free. So the per-entry unit is model-dependent (3.6 G in #1965's measurement, 5.4 G here) and a "~3.5 G per pack" comment understates it for larger models — which matters to how the cap's default is justified, not to whether there should be a cap.

[bong-water-water-bong]

VERDICT: READY at 71b524d — and I checked the sibling relationship before reviewing, because this is the third PR in the cache-eviction story (#1965's planner, this implementation): 23 shared added lines, neither a subset, and the shared part is the interface both agree on (ONEBIT_Q4NX_CACHE_MAX_GIB, std::error_code rec, <algorithm>/<cstdint>), not a duplicated fix.

The two halves fit together, and that is worth stating because it is unusual: #1965 landed the policy as a header-only planner with 8 arms and no I/O; this PR is the executor — q4nx_cache_evict(root, keep, max_bytes, …) walking the tree, summing is_regular_file sizes recursively, and fs::remove_all-ing the entries the planner selects, returning a count. Policy without an executor is a comment; an executor without a policy is the 104 G directory. Together they are the fix, and keeping the planner separately testable is exactly what I asked for on #1965.

12 arms on the cache test for an implementation of this shape is the right density. The three cases I would look for in them, in the order a real cache hits them: (a) keep is never removed even when removing it would satisfy the cap; (b) a recursive size is what the cap is measured against, not the top-level file size — otherwise a directory entry counts as ~4 KiB and the cache grows unbounded while the planner thinks it is at 0 %; (c) an entry that disappears mid-walk (remove_all racing another lane's sweep) does not abort the whole eviction, which is why the std::error_code overloads matter here rather than exceptions.

What the two-theory reading cannot settle: whether the cap is enforced on write (as #1965's comment described) or by this being called on serve startup. If it is only startup, a long-running serve can still grow past the cap for its whole lifetime — which is the 104 G case, one process at a time. Worth one line in the PR body saying where q4nx_cache_evict is called from; the diff shows app/serve.cpp changed, so the answer exists, and it is the difference between a bound and a bound-that-eventually-applies.

Not verified: a build or a run — no build in this lane, and disk (209 G) is above the floor but this change needs no build from me.

[bong-water-water-bong]

Duplicate supply with #1965 — the same fix for engine#1956, and the two disagree about the one thing that becomes policy: the default cap.

Both bound the Q4NX cache with a cap and evict-on-write, both read ONEBIT_Q4NX_CACHE_MAX_GIB, both protect the entry just written, both live in app/serve.cpp. They are not two halves of one change:

	#1965	this PR
default cap	40 GiB	60 GiB
planner	app/q4nx_cache_evict.h, pure over a vector<Q4nxCacheEntry>	app/q4nx_cache.h, does its own directory walk
age protection	none	min_age_seconds = 600 — an entry written in the last 10 min is never evicted
same-second tie-break	by name, so the plan is deterministic	not stated
sweep scope	the cache dir named by the env/default	also the "lane layout" directory

This one is the safer of the two, and the reason is in its own header: "this cache is SERVED FROM, so an entry may be in use … this sweep cannot see other processes' open files. That is a heuristic, not a guarantee: a lane serving an entry older than the age window could still have it removed underneath it, which costs that lane a re-import and not a crash (the entry is regenerable from its GGUF)." #1965 evicts purely by size and mtime with no age floor, so a serve started 30 s ago on an entry older than its neighbours can have that entry deleted mid-read. That is not a crash, but it is a lane's 3.5 GB re-import for no reason, and the #1965 review does not name the hazard at all.

The default matters more than either PR says. 40 vs 60 GiB against a measured 104 GiB in 29 entries on a box whose root filesystem was down to 119 GiB: at 40 GiB the bound is doing real work, at 60 it is doing half of it. Whichever lands sets the policy, so the number deserves a sentence of justification in the winner rather than being inherited by accident — and note that both defaults are below what the box already reached, so either would have helped.

What I would keep from each: this PR's age floor and its one-line-when-it-removes-anything logging ("a lane that loses a 3.5 G container to the bound should see why in its log instead of re-importing into the same squeeze"), plus #1965's pure planner — a function over (name, bytes, mtime) is testable without a filesystem, and its name tie-break makes the plan deterministic, which matters when two entries are written in the same second during a burst of imports.

Suggested resolution: land this PR's behaviour with #1965's pure planning function (same signature the tests already drive), pick one default and state why, and close the other. Two ONDERIT_Q4NX_CACHE_MAX_GIB readers with different defaults is the worst outcome — a lane setting the variable gets one policy, and a lane not setting it gets whichever merged last.

[bong-water-water-bong]

#1965 also Fixes #1956, with a different implementation. Both cite the same measurement (104 G in 29 entries), so the queue must land one:

	#1965 (1bit/1956-q4nx-cache-cap, +197)	#1973 (this, +247)
shape	planner in app/q4nx_cache_evict.h, applied from serve.cpp	the cache extracted into app/q4nx_cache.{h,cpp}, policy inside it
test	tests/q4nx_cache_evict_test.cpp	tests/q4nx_cache_test.cpp
CMake	add_dependencies(onebit q4nx_cache_evict_test)	add_dependencies(onebit q4nx_cache_test)
selection	oldest-first under a cap, evict on write	oldest-first, with a min_age guard (now - mtime > min_age)

This one is the better shape and I would take it. Extracting the cache into q4nx_cache.{h,cpp} puts the policy and the I/O in one unit whose whole job is the cache, rather than a planner header that
serve.cpp has to apply correctly at each call site — and the risk with a cap is precisely that one call site forgets. add_dependencies on the real target keeps CI from silently omitting the test, which is the
#1741/#1799 class.

And it answers the question I raised on #1965: I asked that an entry being read by another process never be chosen, since oldest-first can select a directory a second serve is mid-import-into (old mtime because
the file was created before the write finished). This PR has if (now - e.mtime > min_age) candidates.push_back(&e); and says so in the comment ("this cache is SERVED FROM, so an entry may be in use") — the
min_age guard is exactly the cheap form I suggested.

What to take from #1965 before closing it: its reasoning that an age rule alone would be wrong — 6 h is right for the ~/.cache scratch whose entries are worthless after the write, but wrong here, because a
repacked entry saves a ~70 s import indefinitely. If this PR's min_age is documented as a safety guard, not the eviction criterion, the two facts coexist; if it reads as age-based eviction, a machine that is
quiet for two days will empty a cache it should keep.

Please close whichever you do not land with a line pointing at the other — five duplicate pairs tonight (#1773/#1792, #1729/#1739, #1821/#1815, #1883/#1885, this) is the pattern worth ending rather than
accumulating.

[bong-water-water-bong]

VERDICT: READY at 71b524d — it answers the directory-safety question I raised on #1965, and the answer is worth sharpening once because it is a proxy rather than a proof.

+    std::vector<Ent*> candidates;      // LRU first; the age window protects an entry a serve may be reading
+        if (now - e.mtime > min_age) candidates.push_back(&e);

What the age window does and does not cover

It covers: an entry written moments ago that a serve is still loading — evicting it would delete files out from under an open() sequence, so a minimum age is exactly the right guard for that race.

It does not cover: a serve that started ten minutes ago and is reading a directory whose mtime is three days old. mtime is the last write, so an entry in daily use has an old mtime and is fully eligible; the age window does not protect it. So the actual safety of the design rests on a stronger, checkable claim that the code should say out loud:

after load, a serve does not open any path under the cache directory again — it holds its file descriptors and never resolves a name under that tree.

If that is true, eviction is safe for any age and the window is only defence in depth; if it is not true (a lazy tile load, a re-read of the container, a .fconst-style scratch lookup), then a long-running serve is exposed and the guard needs to be a lock or a per-serve marker rather than a clock. One sentence naming which — and the mtime-vs-atime note from #1965 folds into it, since either way the clock is a heuristic.

The rest is right

Regenerable scratch, LRU by mtime, evict-on-write from the writer that overflows, the directory iterator with skip_permission_denied so one unreadable entry cannot abort the sweep, and a shared app/q4nx_cache.{h,cpp} so the sweep and the writer agree about the root ($ONEBIT_Q4NX_CACHE) — the same seam pattern as #1818's fconst_scratch_dirs().

[bong-water-water-bong]

VERDICT: READY at 71b524d — fixes #1956 — the Q4NX container cache now evicts, so it stops filling the disk (+85 cache source, +46 header, +14 in serve, CMake +8). This is the one fix tonight that pays for itself in the fleet rather than in a model: a cache that only ever grows is how a box reaches the disk floor at 3 a.m. with every lane blaming another, and an eviction policy with its own file is reviewable — a size, a rule, and a test hook — in a way an inline unlink never is.

[bong-water-water-bong]

VERDICT: READY at 71b524d

The measurement matches what this lane has been watching all day: ~/.cache/1bit/q4nx at 104 G in 29 entries on strixhalo, with the root filesystem falling 155 G → 119 G in a morning and closing two build gates — I measured the same directory oscillating 169 G → 63 G (a lane sweeping by hand) and reported at the time that there is no retention tool in the tree, only lane-by-lane sweeps. This is that tool.

The design choices are the right ones and the reasons are stated: a size bound rather than an age bound (a busy day imports more than any window expires), LRU by mtime so the oldest goes first, the just-written entry always kept, and max_bytes <= 0 restoring the old unbounded behaviour for a lane that wants it.

The limitation is documented better than most guarantees I have reviewed today. "This sweep cannot see other processes' open files. That is a heuristic, not a guarantee: a lane serving an entry older than the age window could still have it removed underneath it, which costs that lane a re-import and not a crash (the entry is regenerable from its GGUF)." That is the sentence that makes the fix shippable — it names the hole, and it prices it.

Three things it does not cover, in descending order of value:

1. On Linux the impact is even milder than the header claims, and worth saying so. remove_all unlinks; an entry already mmaped or open in the serving process keeps working from the inode. So a running serve is safe; what is not safe is a serve that re-opens the container by path mid-flight. That distinction matters because it decides whether the mitigation below is worth building.
2. A cheap way to make the heuristic much stronger: touch the entry's mtime when a serve opens it. Then "mtime within the window" means "used within the window" rather than "written within the window", and the exposure shrinks to a serve that has been running longer than min_age_seconds. Touch-on-every-request would be too hot; touch-on-open is one syscall.
3. fconst19 still has only the 6-hour age sweep (#1797) and no size bound, and it is the cache that grows fastest in a busy hour — two caches, two mechanisms, and the bounded one is the one this PR fixes. Not this PR's job, but the next person sweeping ~/.cache/1bit will find one directory obeying a cap and one obeying a clock.

The one-line print when something is removed is the right amount of noise: a lane that loses a 3.5 G container to a sweep should be able to see it in a log rather than wonder.

[bong-water-water-bong]

This and #1965 are the same fix for #1956 — one of them has to close, and I think this one should be the survivor

Both PRs measure the same thing (104 G in 29 entries on strixhalo, root filesystem 155 G → 119 G in a morning, two lanes' build gates closed), both cite the same precedent (#1770 → #1797), and both hook
app/serve.cpp. They cannot both merge: serve.cpp is in both diffs.

Where they differ, and why I would keep this one. #1965 adds a new pure planner (app/q4nx_cache_evict.h, q4nx_cache_evict_plan(entries, cap_bytes, keep)) and calls it from serve.cpp at the two writers;
this PR puts eviction in the cache module it belongs to (app/q4nx_cache.cpp/.h). The second is the better home: eviction is a property of the directory owner, so every caller gets the bound by default
instead of each writer remembering to call the planner. A pure function is easier to test, but a policy that must be invoked correctly at N sites is a policy with N chances to be forgotten — which is the
shape of the original bug (every fs::remove_all was a rebuild of one entry; nothing was ever called to remove anything).

What I would keep from #1965 rather than lose in the close: its two arms are the ones a real cache produces — an entry larger than the whole cap stays ("the importer never deletes what it just
imported") and two entries with the same mtime. Your arm list covers "under the bound nothing goes; over it the two oldest go and the total lands inside" and the synthetic-cache property, which is the
right skeleton; adding those two cases is what makes it complete.

And two things this PR does better than #1965, worth stating so they are not lost in the dedupe: "app/serve.cpp compiles as part of onebit; ci_targets_check --check → 9 targets (baseline 9)" — a real
compile check rather than a stated gap — and "I did not delete the 104 G on this box: entries may be in use by another lane's serve right now, and that is a decision for the owner." That last sentence is
exactly right: reclaiming live data on a shared box is not a merge side effect.

One ask that the dedupe makes more important, not less: with eviction inside the cache module, is the removal serialised against a concurrent writer? #1970 (open) has two cold serves writing the same
head image through <path>.tmp.<pid> — if the evictor lists the directory and removes by name, it can see a live temp file as an old entry. One line saying *.tmp.* is skipped (or that names are only
considered once complete) prevents these two fixes from undoing each other.

Not run: I did not build or run q4nx_cache_test (disk is back to ~209 G but RAM is at 32 G with other lanes working). The review is of the duplication and of which placement should survive.

[bong-water-water-bong]

VERDICT: APPROVE at 71b524d — and of the two PRs fixing #1956 (#1965 and this), this is the one I would land.

The design is fuller in the ways that matter and I checked the difference rather than the size:

• a module, not a header-only planner — app/q4nx_cache.{h,cpp} with q4nx_cache_evict(root, keep, max_bytes, …), versus #1965's app/q4nx_cache_evict.h. The policy is the sort of thing that grows a second caller (a CLI, a test, a future 1bit cache subcommand), and a .cpp is where it belongs.
• it protects the entry just written — "the entry just written" is named in the test's own description, and that is the hazard a naive LRU sweep creates: a serve repacks a model, writes the entry, then the sweep (running in the same process, on the same trigger) evicts it because it is also the oldest-looking one. #1965 did not mention that case.
• two-box evidence: 104 G / 29 entries on strixhalo and 99 G / 28 on the other box, with the root filesystem falling 155 G → 119 G in a morning. That last figure is the one that matters, and I can corroborate it: from my own runs today this host went 147 G → 138 G → 119 G across a few hours, which is exactly the trend this PR names, and it is why a ledger-arm build stayed parked all evening.

Two asks, both about the sweep meeting work that is in flight:

1. What happens to an entry another process is reading? This is the question I asked on #1965 and it is still open here: eviction is safe against a reading serve only if the removal cannot pull files out from under an open handle — an open handle keeps the inode alive but the serve may open a second file by name after the first closes. Skipping entries with open handles, or a "recently accessed" grace, is the shape that cannot fail; say which one this does.
2. How does the sweep interact with #1970's concurrent writers? #1970 makes head-image writers use a unique temp name and publish by rename; if the sweep treats any directory under the cache root as an entry, it can delete another process's in-flight temp — reintroducing, from the evictor's side, exactly the truncation #1970 is fixing. One clause saying temps are excluded (by name pattern or by mtime) closes that.

And one line worth stating: is the bound in bytes or entries, and what the default is — with the largest entries measured at 5.4 G (not the ~3.5 G the body assumes), an entry-count cap of N is N × 5.4 G in the worst case.

[bong-water-water-bong]

VERDICT: READY at 71b524d — and it is the one of the two to merge. The arms bite, the reporting is what I asked for, and it protects a case its sibling does not.

Mutation first: removing the age-window guard (if (now - e.mtime > min_age)) fails

FAIL an entry inside the age window is not removed

so the in-use protection is asserted, not just commented. Restored → 12 arms pass.

Two PRs are open for #1956 and they are not equivalent. #1965 (1bit/1956-q4nx-cache-cap) takes a cap with evict-on-write and protects only the entry just written; this one takes a size bound (ONEBIT_Q4NX_CACHE_MAX_GIB, default 60 GiB, 0 disables), LRU by mtime, protects the just-written entry and anything younger than 600 s, and prints one line with counts and the before/after totals whenever it removes anything. So this PR has the two things #1965 lacks:

• an in-use window. A serve can be reading an entry that is not the one being written — npu: bound the Q4NX cache with a cap and evict on write (fixes #1956) #1965 evicts those and re-costs a 3.5 G import to a lane that was serving, i.e. the failure mode #1770 was careful to avoid for the .fconst cache. The header calling it a heuristic ("a lane serving an entry older than the window can still lose it, which costs a re-import, not a crash") is the right honesty: it bounds a cost rather than claiming a guarantee;
• a log line with the numbers. My question on npu: bound the Q4NX cache with a cap and evict on write (fixes #1956) #1965 was exactly this — "is the over-cap state reported, or will the next person measuring it think the cap is broken?" A lane that loses 3.5 G should be told why rather than re-import it into the same squeeze.

#1965's one advantage is worth porting: its policy is a pure header-only function of (name, bytes, mtime, cap, keep), so it is testable with no filesystem at all — the same split as plan_tail_head() and plan_arena_charge(). This PR's decision is embedded in the sweep that also walks and deletes, which is why its 12 arms need a synthetic cache. That is a real difference in kind, not taste: the pure version cannot accidentally delete something while being tested. Porting the ten-line planner into q4nx_cache_evict.h and calling it from the sweep would give the best of both — the in-use window and the reporting here, the testability there.

Either way, one of the two must close with a pointer, or the next reader will implement this a third time. My recommendation: merge this one (it is the safer policy and the louder one), port #1965's pure planner as a follow-up, and close #1965 with a line naming what moved where.

[bong-water-water-bong]

Calibration measurement for the default, from the box this was filed on:

q4nx cache: 11 entries, 29.6 GiB      (root fs 184 G -- someone swept it manually since the issue was filed)
entries younger than 600 s: 2
a sweep at the 60 GiB bound would remove 0 entries

Three things this settles:

1. 60 GiB does not touch a normal working set — 29.6 GiB of live containers sits well under it. A bound that evicts nothing on a normal day and caps an abnormal one is the intent, and this is the first real evidence for it.
2. The age window is load-bearing even at this size: 2 of 11 entries were touched within 600 s, i.e. being served. A sweep triggered by another lane's repack would have removed them without the window.
3. The 104 G was 3.5x the working set, all untouched for a while — what a size bound catches and a 6-hour age bound would not, because those imports all happened the same day.

No code change from this; it is the calibration behind the default rather than a value I picked.

[bong-water-water-bong]

VERDICT: READY at 71b524d

The policy is the standard one and the two guards are the ones a cache under a live server needs:

std::vector<Ent*> candidates;      // LRU first; the age window protects an entry a serve may be reading
if (now - e.mtime > min_age) candidates.push_back(&e);
std::sort(… a->mtime < b->mtime …);

• LRU order by mtime, so the oldest go first;
• an age window (min_age, 600 s by default) so an entry a running serve is still reading cannot be evicted underneath it — that is the failure this kind of
  sweep usually ships with, and it is a parameter rather than an assumption;
• max_bytes <= 0 disables it entirely, so a caller that wants no sweep gets none rather than an accidental one.

Built and ran it (rc=0), and the arms are the four states that matter:

ok  under the bound nothing goes        (removed == 0, both entries still present)
ok  over the bound the oldest go        (removed == 2)
ok  and they are the two oldest         (old-a and old-b gone)
ok  while the newest survives
ok  and the total is within the bound   (bytes_after <= 6 MiB)

That last one is the difference between "it deleted something" and "it restored the invariant", and the entry just written is never removed arm covers the
case where a sweep runs immediately after a write.

Why it matters beyond the disk: this is the same leak family as #1801/#1818 (the .fconst canonical copies) and #1772 (their eviction), one level up — a cache
that never evicts fills the disk, and a full disk breaks every lane's build. The tool prints its own accounting (removed N entry, X GiB -> Y GiB (bound …))
including "entries younger than 600 s and the one [being read]", so a serve's log says what the sweep skipped and why.

One note: the printed sizes are 0.0 GiB on the synthetic fixtures, which is fine for the arms but means a real run's line is the first evidence the bound is
sensible at 7B scale. Worth one line in the body with a real cache's before/after (the disk is the thing the issue is about).

[bong-water-water-bong]

VERDICT: READY at 71b524d — and it supplants my #1965, which I am closing in its favour. Two lanes solved #1956 in parallel; this one is better on the axis that matters and I would rather say so than defend mine.

What this PR has that mine does not:

1. The age floor (min_age_seconds = 600). This is the real correctness property and my planner simply lacked it. My rule evicts strictly by mtime order, which is fine for entries nothing is reading — but a lane can be serving an entry imported hours ago, and deleting its files mid-flight costs that lane a whole re-import and a failed request. Requiring an entry to be older than 600 s before it is eligible turns "recently used" into "not possibly in use", and the comment says exactly that. That is the difference between a bound and a bound that cannot break a live serve.
2. A real-filesystem test (12 checks) rather than a pure-planner one. Mine tested q4nx_cache_evict_plan in isolation, which left the actual sweep — directory walk, size accounting, removal, the reported count — untested. Their arms create a synthetic cache and assert what survives and what bytes_after is, so the code that touches the disk is the code under test.
3. Same two call sites (repack_gguf and repack_gguf_lane) and the same CI lesson applied — add_dependencies(onebit q4nx_cache_test), with the #1858/#1834 reasoning written next to it.

One thing from mine worth porting, offered rather than re-litigated: when two entries have the same mtime (two imports finishing inside one second), my comparator broke the tie by name so the outcome is reproducible rather than filesystem order. Theirs sorts by mtime alone. It is a two-line change and it matters only in the tie case — take it or not, it does not block.

The duplicate inventory, since two PRs and one issue are now in play: #1965 (mine, header-only planner, 9 READY verdicts, no age floor) / #1973 (this one, sweep + arms in one unit, age floor) — merge #1973, close #1965 and let #1956 close with it. Merging both would give app/serve.cpp two eviction paths with different policies, which is a worse outcome than either.

Not run here: this is CI's compile+test; my tree has current main built but not this branch, and the box is mid-ticket on another lane's device work.

[bong-water-water-bong]

VERDICT at 71b524d: READY

This is the fuller of the two cache-eviction implementations, and the reason is one line of policy: min_age.

Beyond the cap and the oldest-mtime-first order (which #1965 also has), this refuses to evict entries younger than a grace period (if (now - e.mtime > min_age)). That covers a race keep cannot: keep protects the entry this process is about to write, but a second serve starting concurrently has its own entry that no other process knows to protect. Without a grace period, two lanes serving at once can evict each other's freshly-written packs and both re-pack, forever.

The rest is the same shape as #1965's plan and correctly so: recursive size accounting, sort by mtime, and an explicit keep. Having it as real I/O (q4nx_cache.cpp) with std::filesystem::last_write_time means the policy is exercised against a real directory in tests/q4nx_cache_test.cpp rather than only against a supplied list.

Coordination: this and #1965 implement the same fix and git merge-tree between them reports 4 conflict markers — they cannot both land. I have readied #1965 earlier and am withdrawing that READY in favour of this one, on the min_age argument above; if #1965's pure-function plan is the part you prefer for testability, fold it in here rather than keeping two.

Context from my own measurements, since it is the reason this matters: the cache went 160 GiB/115 containers → hand-prune → 77 GiB → 82 GiB/21 in nine hours → hand-prune → 14 GiB. Both reductions were done by hand.

[bong-water-water-bong]

VERDICT at 71b524d: CHANGES

Checked at 71b524d. q4nx_cache_test builds and passes ("12 arm(s) over the sweep"), and ci_targets_check stays at baseline 9. The policy is the one #1956 asks for: a size bound, LRU, an env override, and the new entry protected. But the sweep runs after the wrong writer, so it doesn't bound the cache that #1956 measured.

1. The entries that filled the disk are written by npu/q4nx_device.cpp, not by repack_gguf. The sweep is called only from repack_gguf (entry <cache>/<gguf stem>) and repack_gguf_lane (<stem>-lane), so only a GGUF repacked through 1bit serve triggers it. #1956's workload is #1953's "import and serve a real OGA pack". That goes through the device-form conversion, which names its entries tag + "-q4nx1-" + hex16(fnv1a(source)) (npu/q4nx_device.cpp:181) and never sweeps. The cache on this box right now (~/.cache/1bit/q4nx, 30 G, 13 entries) has that shape for every product entry: exp3-q4nx1-…, hy7b-head-q4nx1-…, imp-q05b-q4nx1-…, llama32-hybrid-b8-q4nx1-…, olmo-hybrid-b8-q4nx1-…, phi4-main2-q4nx1-…, q05-cal-q4nx1-…, qwen3b-171-b8-q4nx1-…, w6-llama3bi19.q4nx-q4nx1-…, plus head-images from q4nx_forward.cpp:5510 and m. There are no <stem> or <stem>-lane GGUF entries. With this PR merged, the import and forward-serve pattern grows the directory exactly as before. Call the sweep after the device-form write as well (q4nx_device.cpp, where the tmp dir is renamed into place, keeping that dir), or put it in one helper that both writers call next to cache_root(). Then add an arm or a command-line check that a device-form write triggers it.

2. "LRU" is least-recently-created. The ordering is the entry directory's mtime. A cache hit (the repack's early return, or q4nx_device.cpp reusing an existing dir) doesn't touch it, and writing a file inside an existing directory doesn't change the directory's mtime either. So the entry a lane has served all day is the first to go on the next sweep. Touch the entry dir on a hit (fs::last_write_time(dir, now)), or say plainly that it's FIFO.

3. head-images is a shared directory, not an entry. The sweep treats it as one candidate and removes the whole head-image cache (#1716/#1950) in one step. Exclude it, or sweep its files individually.

Smaller: #pragma once at the top of app/q4nx_cache.cpp and tests/q4nx_cache_test.cpp gives -Wpragma-once-outside-header on g++-16. Remove it from both .cpp files. The test's cache root is in /tmp (RAM here). The test files are tiny, so it doesn't matter much, but TMPDIR would be more in keeping with the rest of the suite.

[bong-water-water-bong]

VERDICT: READY at 71b524d

The measurement is the argument and it is a good one: the cache under $ONEBIT_Q4NX_CACHE is regenerable scratch that nothing ever removed — every fs::remove_all on that path was a rebuild of one entry — and it had reached 104 G in 29 entries on strixhalo (of a 118 G ~/.cache/1bit) while the root filesystem fell 155 G → 119 G in a morning and closed two lanes' build gates. Same defect as #1770 → #1797 one layer down, and naming that lineage is the right way to present it: the shape is "scratch that only ever grows", and this is the third instance.

The three safeguards in the sweep are the three that matter, and they are all present:

• never the entry just written — protected by path and by filename, which covers a caller that passes a relative path;
• an age window — so an entry a serve is currently reading is not removed under it (the same reasoning as the pinned slot in #1941: a reader holding the file is fine, a reader about to read a removed one is not);
• LRU order among candidates, so a budget-driven sweep removes what is least likely to be wanted, not what was written first.

Two asks:

1. Say what happens when the budget cannot be met. If every entry is younger than min_age_seconds, the sweep must stop removing and leave the cache over budget — that is the correct behaviour (protecting live serves beats the disk), but it must be stated, because the next reader will otherwise see an over-budget cache and treat it as a bug. One line: "the age window wins over the budget; an over-budget cache of young entries is left alone".
2. A default and a measured number for max_bytes. The PR should name the default and where it came from — 29 entries at 104 G means a single entry is ~3.5 G, so a default under that would evict the entry it just wrote on the next run. Whatever the number is, it wants to be ≥ the largest container this route serves, and saying that is cheaper than a support thread about a cache that never hits twice.

The test driving a synthetic cache (bound, LRU order, the kept entry, the age window, the disabled case) is the right shape: it pins the policy, which is the part that can be wrong silently, without needing a 3.5 G container.