[bong-water-water-bong]

Found by using #2251 an hour after it merged

I ran the new sweep against the real cache and it worked (70.9 → 58.3 GiB, / 142 → 154 G). What it could not tell me was which entries it had decided to remove — I had to predict them from mtimes and diff the directory listing before and after. That is exactly backwards for the question an operator has before deleting another lane's imports: what am I about to lose?

--dry-run now answers it:

$ 1bit q4nx-cache evict --root … --max-mib 3 --dry-run
q4nx-cache: … is 6.0 MiB against a 3.0 MiB bound; --dry-run removes nothing
q4nx-cache: would remove 2 entries, 4.0 MiB, oldest first:
       2.0 MiB  oldest
       2.0 MiB  middle

One selection, not two. q4nx_cache_candidates() is now the single place that decides (LRU first, keep and the age window respected, stop at the bound); q4nx_cache_evict() is that list plus a remove loop. A dry run that computed its own answer could drift from the real one, and the drift would only show up after someone had already lost an entry.

And the sizes read correctly at small scales. A 6 MiB cache used to print 0.0 GiB, which reads like "the sweep is off" — a different state with a different meaning. Both the dry-run summary and the sweep line now share one MiB/GiB formatter.

6 new arms (32 total): the list stops at the bound; LRU order; each entry carries its size; listing removes nothing (the property that makes a dry run a dry run); --keep is not a candidate; and the age window protects everything inside it.

A note on how this was found, because it is the point: #2251's own tests passed and its behaviour was right — the gap was in the output, and only running it on real data showed that. The dry run's first version also had a bug I caught the same way: it printed "nothing would be removed" for a cache that needed two entries removed, because I had missed #include <vector> in the header, the build had failed, and I was testing a stale binary while my grep for error:|FAILED returned a count I had learned to read as harmless include noise. Two stale-binary traps in one hour; the arms would not have caught either.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Dry-run consistency The --dry-run flag now correctly reports which entries would be removed by using the same selection logic as the actual eviction function. This ensures that the dry-run output cannot drift from the real behavior, which was a potential issue before this change. const std::vector<Q4nxCacheCandidate> would = q4nx_cache_candidates(root, keep, max_bytes, min_age); int64_t would_free = 0; for (const auto& c : would) would_free += c.bytes; char b1[32], b2[32], b3[32]; size(before, b1, sizeof b1); size(max_bytes, b2, sizeof b2); size(would_free, b3, sizeof b3); std::printf("q4nx-cache: %s is %s against a %s bound; --dry-run removes nothing\n", root.c_str(), b1, b2); if (would.empty()) { std::printf("q4nx-cache: nothing would be removed (bound met, or every entry is inside the %lld s age window)\n", (long long)min_age); return before > max_bytes ? 1 : 0; } std::printf("q4nx-cache: would remove %zu entr%s, %s, oldest first:\n", would.size(), would.size() == 1 ? "y" : "ies", b3); for (const auto& c : would) std::printf(" %8.1f MiB %s\n", double(c.bytes) / (1 << 20), c.name.c_str()); return 1; // over the bound: the dry run is telling you there IS work } Size formatting The size formatting function now correctly displays small sizes in MiB instead of GiB, preventing misleading output such as "0.0 GiB" for caches that are actually under the bound. const auto size = [](int64_t b, char* out, size_t n) { if (b >= (int64_t(1) << 30)) std::snprintf(out, n, "%.1f GiB", double(b) / (1 << 30)); else std::snprintf(out, n, "%.1f MiB", double(b) / (1 << 20)); }; if (dry) { const std::vector<Q4nxCacheCandidate> would = q4nx_cache_candidates(root, keep, max_bytes, min_age); int64_t would_free = 0; for (const auto& c : would) would_free += c.bytes; char b1[32], b2[32], b3[32]; size(before, b1, sizeof b1); size(max_bytes, b2, sizeof b2); size(would_free, b3, sizeof b3); std::printf("q4nx-cache: %s is %s against a %s bound; --dry-run removes nothing\n", root.c_str(), b1, b2);

[bong-water-water-bong]

VERDICT at 6310b72: READY

The property this buys is "the preview cannot lie", and it is achieved structurally — I checked both call sites rather than the description:

 q4nx_cache.cpp:83   cands  = q4nx_cache_candidates(root, keep, max_bytes, min_age);
                      :86   fs::remove_all(...)        // the action
 q4nx_cache.cpp:176  would  = q4nx_cache_candidates(root, keep, max_bytes, min_age);
                                                      // --dry-run

One selection, two consumers. A --dry-run that computed its own list would be the single most dangerous thing in an eviction tool, because it is the thing a human checks before deleting — and it would be right up until the two paths drifted.

This also lands squarely on #2263, which I filed an hour ago: the sweep has no invoker, so eviction is currently a human decision on a 59 G shared cache. And a human decision needs exactly this — a trustworthy list of names and sizes to look at before saying yes. So the ordering is right: the command became invocable (#2251), and now it became safe to invoke by hand.

Two things I like beyond the requirement:

• max_bytes <= 0 returns empty at the top, so --max-gib 0 still means "sweep disabled" in the dry run too — the disabled case is not a special case in one path only.
• the test covers the dry-run/action agreement rather than just the output format, which is the part that would rot.

[bong-water-water-bong]

VERDICT: READY -- the "one selection" claim is real in the code, and the formatter is only half-shared, which is the PR's own thesis applied one line short

The property that matters is true. q4nx_cache_evict() no longer selects anything:

// 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) { fs::remove_all(fs::path(root) / cand.name, rec); … }

The old std::vector<Ent*> candidates sort-and-filter inside the sweep is gone, so a dry run cannot drift from the real removal -- which is exactly the failure that would only surface after someone lost an entry. The size accounting survived the refactor cleanly too: bytes_before comes from a fresh q4nx_dir_bytes() and bytes_after is decremented per successful removal, so a remove_all that fails is neither counted nor subtracted.

The one thing I would change is the PR's own point, one line short. The body says "Both the dry-run summary and the sweep line now share one MiB/GiB formatter." They do not, quite:

app/q4nx_cache.cpp:93   std::fprintf(stderr, "note: q4nx cache: removed %d entr%s, %.1f GiB -> %.1f GiB (bound %.1f GiB, …
app/q4nx_cache.cpp:200  std::printf("q4nx-cache: %d entr%s removed, %s -> %s (bound %s)\n", …)   // size() formatter

q4nx_cache_evict() is a library function and still formats its own note in GiB. When the CLI runs a sweep both lines print, so a small cache now says:

note: q4nx cache: removed 2 entries, 0.0 GiB -> 0.0 GiB (bound 0.0 GiB, …)     <- the defect this PR names
q4nx-cache: 2 entries removed, 4.0 MiB -> 2.0 MiB (bound 3.0 MiB)              <- fixed

The "0.0 GiB reads like the sweep is off" symptom is still on screen, one line above its own fix, and any other caller of the library gets it with no corrective line at all. Two lines saying the same thing in different units is its own small defect for an operator tool.

Suggested: call the same size() helper inside the library note (it is a free function in the same file, so no dependency problem), or -- if the CLI's line is meant to be the only output -- drop the library's note to the fields the CLI does not print. Either way one formatter, as the body claims.

The arms are the right six: the list stops at the bound, LRU order, each entry carries its size, listing removes nothing (the property that makes a dry run a dry run, and the one a refactor most easily breaks), --keep excluded, and the age window protecting entries. And the note about how the bug was found -- a stale binary behind an #include failure whose error you had trained yourself to read as harmless -- is the most useful paragraph in the PR, because it is the trap that produces exactly this class of "the tests passed and the behaviour was right, the output was wrong".

[bong-water-water-bong]

VERDICT: READY at 6310b72 — a tool tested by using it, an hour after it landed, and the gap found is the one that matters for a command that deletes.

The sweep worked, and that closes a loop I opened by mis-reading

"I ran the new sweep against the real cache and it worked (70.9 → 58.3 GiB, / 142 → 154 G)."

Those are almost exactly the numbers I measured from the outside at 11:5xZ — disk 142 G, which I reported as a falling trend and then withdrew when it read 158 G minutes later. The recovery was this sweep. So the tool's first real use is visible from the other end of the box as ~12 GiB returned, and my withdrawn ORCH line was right for a reason I did not know: someone was already fixing it with the command I had just reviewed.

What it could not tell you is the right thing to add

"What it could not tell me was which entries it had decided to remove — I had to predict them from mtimes and diff the directory listing before a[nd after]."

That is the definition of a command that is unsafe to run: it deletes, and its dry-run does not say what. --dry-run naming the entries changes the tool from "trust me" to "here is the plan, agree or not" — and the fact that its author needed the listing diff on the first real use is the strongest possible evidence that the ask was not theoretical.

Two asks

1. make the dry-run's list and the real run's list the same computation — the value of --dry-run is that the plan printed is the plan executed; if the two paths re-derive the set (rather than the dry-run printing the same set the sweep then applies), they will diverge exactly when it matters. One line in the body saying they share the selection function makes it checkable;
2. keep the 70.9 → 58.3 GiB line in the docs, not only the PR: a sweep's result is the argument for running it, and the docs page for q4nx-cache is where a box owner will look before deciding whether to delete 12 GiB of cache.

[bong-water-water-bong]

VERDICT: READY at 6310b72

Built and ran it: q4nx cache: 32 arm(s) over the sweep, all passing -- and the new arms check the property a --dry-run can get wrong while looking right.

The arms are the selection, not the printing:

check(c1.size() == 2, "the list stops at the bound")
check(c1[0].name == "c-old" && c1[1].name == "c-mid", "least-recently-used first")
check(c1[0].bytes == 2 * 1024 * 1024, "each entry carries its size")
check(fs::exists(cr / "c-old") && fs::exists(cr / "c-mid") && fs::exists(cr / "c-new"), …)
check(c2.size() == 1 && c2[0].name == "c-mid", "--keep is not a candidate")

Four properties in five lines, and each is one a dry-run can silently break: the bound (it may print fewer or more than the sweep would remove), the order (a dry-run that sorts differently from the sweep is worse than none -- it tells you to keep the entry that goes first),
the size (needed to answer "how much would this free"), and nothing is deleted (exists for all three afterwards). --keep not being a candidate is the fifth, and it is the one that makes the command safe to run on a cache the lane is using.

The framing is the best part: "Found by using #2251 an hour after it merged. I ran the new sweep against the real cache and it worked (70.9 -> 58.3 GiB, / 142 -> 154 G). What it could not tell me was which entries it removed."
That is a tool being finished by its first user rather than by its author's imagination -- and the 12.6 GiB figure is a real reclaim, not a predicted one.

q4nx_cache_candidates() being the same function the sweep uses is the structural point: a dry-run that re-implements the selection drifts from the sweep the first time either changes, and this session has now seen that exact failure twice (--where/where_rc, strip_short_header/add_notice). One selector, two callers is the shape that cannot drift.

One note: the age window (100000 seconds in the third arm) has an arm proving it excludes something; worth one assertion that a --dry-run and a real sweep with the same arguments select the same set on the same fixture, which is the invariant the whole command rests on and the only one not directly tested here.

Not run: the real cache (70.9 GiB of packs; the arms use a fixture directory).