fix(prune-cache): close the reader-marker check-then-delete window #2

Merged

claude merged 5 commits from fix/prune-cache-reader-window into main 2026-08-24 14:35:53 +00:00

Conversation 3 Commits 5 Files Changed 3

+328 -23

claude commented 2026-08-24 04:11:49 +00:00

Collaborator

Copy Link

Copy Source

Closes #1.

Summary

• scripts/prune-cache.sh: both eviction sites now unlink through evict_dir, which renames the candidate aside and re-examines it before the rm -rf, instead of trusting a marker check taken seconds earlier.
• A reprieved cache is restored under its own name; one whose name a concurrent seed has retaken is left aside and reclaimed by a deferred-eviction sweep at the top of the next pass.
• is_locked takes an optional name so the reader-marker lookup still keys on the name a consumer resolved, not on whatever the pass has since renamed the directory to.
• scripts/prune-cache-selftest.sh: three new scenarios, 21 → 31 assertions.
• README.md: the "what this does and does not guarantee" section named this window as a documented residual. It no longer is.

Why

The ticket's AC offers two shapes and asks for one, justified.

Chosen: structural (shape 1). Two reasons the tripwire was the weaker option here, both visible only from inside the code:

1. The property a tripwire would assert is false of this script. "Snapshots of non-protected refs are never eviction candidates" is not something prune-cache.sh holds — it evicts snapshot-* of any non-protected ref by design, and the existing selftest scenario 7 asserts exactly that eviction (it is where the deliberate target-before-snapshot ordering is observed). The property lives in cargo-cache-publish/action.yml's publisher gate, a different action, out of this track's scope. A tripwire written in prune-cache-selftest.sh would either fail on landing or assert something this script does not control.
2. The policy holding the window shut is weaker than the ticket assumed. cargo-cache and cargo-cache-publish each take protected-branches as their own independent input. Nothing ties the two lists together. A consuming workflow that lists a publisher ref in one and not the other arms this window with no change to any code in this repo — not even the "add a non-protected publisher ref" the ticket describes, just a mismatch between two strings in a caller's YAML.

And it was cheap and idiomatic, as the ticket hoped. No new mechanism: it is publish-snapshot.sh's rotation, applied to eviction. Renaming aside costs one rename(2); the re-scan is a directory listing. The multi-second du stays outside the window, before the rename.

Why the second look is conclusive, not just closer

The same ordering proof the publish side already rests on (cache-lib.sh, reader-marker section):

• A consumer publishes .reading-<dir>-<tag> before it resolves the source path.
• The eviction scan happens after the rename.
• So a consumer that resolved this directory did so before the rename, and therefore published its marker before the scan — it cannot be missed.
• A consumer arriving after the rename cannot resolve the path at all, and falls through to its cold-start path — the same safe degrade the publisher's swap window already produces.

Renaming disturbs nothing already in flight: it unlinks no entry and leaves the source inode unchanged, which is precisely why the publish side can rotate a snapshot out from under a live reader. A declined eviction therefore costs a deferred eviction and nothing else.

Red/green: the new test bites

Scenario 12 reproduces the window deterministically rather than by timing. A consumer can publish its marker at any instant, including between the check and the unlink; the pass runs a du -sk in exactly that gap. So the test puts a du shim on PATH that publishes the marker for the candidate it is measuring — strictly after the check, strictly before the unlink — then execs the real du.

Demonstrated both directions, in scratch copies:

Script under test	Result
This PR	prune-cache-selftest: 37 assertions passed
evict_dir "$dir" || continue reverted to rm -rf "$dir" at both sites, everything else identical	scenarios 1–11 pass, 12 fails: ASSERTION FAILED: expected kept: .../target-feat-dead-73dad4a7 (a cache claimed inside the eviction window is not unlinked) — log shows ::warning::pruned dead-branch cache target-feat-dead-73dad4a7
Rename-aside kept, only the second look defeated (if is_locked … → if false)	12 fails identically

The third row is the one that matters: it isolates the assertion onto the load-bearing line rather than onto the refactor. And scenario 11 (a marker present before the pass starts) passes in every row — the pre-existing check already worked, so the new coverage is aimed at the gap and not at re-testing what did not need fixing.

Scenario 13 covers the deferred sweep in both directions: not reclaimed while a reader is live, reclaimed once it is gone.

What is still not guaranteed

Stated plainly, since the ticket's alternative shape would have left the race intact:

• The reader-marker race in eviction is closed, not merely narrowed — that is the claim, and it holds only for consumers that follow the marker-before-resolve ordering (hardlink_clone_into does; it is the only cloner in this repo).
• .ci-lock-* gets re-checked too, since it moves with the rename — a free improvement, not the ticket's subject. It does not close the seed side's own [ -d "$OWN_DIR" ] → write_cache_lock gap in seed-target-dir.sh:48, which is that script's to hold and out of scope here.
• A seed-fallback-dir outside the cache root still has no interlock at all. Unchanged, still documented in the README, still relying on the consumer's per-attempt verification.
• Disk reclamation stays bounded, not immediate: a declined eviction leaves the cache on disk — under its own name, or aside until the next pass sweeps it.

Review round (findings 1–4)

1. The sweeper depended on the thing this PR exists to remove. Fair hit, and the sharpest finding in the review. Pass A renames a candidate aside; pass B's sweep sees an aside with no readers and reclaims it; A then finds a reader and restores — and since rm -rf traverses fd-relative, the rename does not stop it, so A can republish a half-emptied tree under a live cache name. capacity: 1 bounds it today, which is precisely the class of argument this PR was written to stop relying on.

Worth separating two properties the original comment ran together, because the fix only needs one of them:

• The unlink itself was never at risk. The aside name only comes into existence after the evicting pass's rename, so a consumer that resolved the directory published its marker strictly before it, and therefore before the sweeper's count. That holds under any interleaving and needs no window. The comment asserted this — correctly — and was silent on the other half.
• What was missing is that an aside with no readers is indistinguishable from one a pass has just created and is still deciding about.

Mechanism chosen: a settle window (EVICTION_ASIDE_SETTLE_SECONDS, default 60), not the PID check. A PID is meaningless across the job containers these passes run in and recycles besides, so it would be precision about the wrong thing. The age is read from ctime, not mtime: rename(2) updates ctime, while mtime stays at whenever the cache was last written — days ago for a stale cache, which is the signal list_by_lru wants and exactly the wrong one here. Verified rather than assumed (touch -d 2020-01-01, mv, then stat: mtime 1577858400, ctime now).

An aside is in flight for one rename plus one marker glob — milliseconds — so 60 s is three orders of magnitude of headroom, and short enough that a genuine leftover is reclaimed by the next run rather than lingering while the volume is under pressure.

This is a bound, not a construction, and the code and README now say so rather than asserting the broader property. The residual, stated plainly: an evicting pass suspended past the window and then resumed finds its aside reclaimed and fails its restore — and logs that it did.

2. prune-cache.sh — the pass summary contradicted the log above it. A pass that declined every dead cache it found printed no dead-branch caches found. Declines are now tracked separately, and that case reports every dead-branch cache found is still in use — none pruned this pass. This also covers the pre-existing instance of the same falsehood on the is_locked decline path, which predates this PR.

3. evict_dir — the warning promised a reclamation that might not happen. It now distinguishes the aside still being there (leaving it for a later pass) from its already being gone (another pass reclaimed it), instead of asserting the first.

4. README bullet 4 overstated. Rewritten to lead with the clause that actually holds — never unlinked under the job that claimed it — with "under its own name, or aside" demoted to the normal case rather than a promise.

Not fixed, as agreed: the reviewer's "starts cold" nit, and the cross-repo dotglob blind spot in daniel/gitdan (tracked separately; hardlink_clone_into's .stage-<tag> is the same class and strictly worse).

Each new gate proven red by defeating it alone in a scratch copy, everything else untouched:

Gate defeated	Result
if [ "$aside_age" -lt "$EVICTION_ASIDE_SETTLE_SECONDS" ] → if false	14 fails: expected kept: …/.evicting-target-feat-dead-73dad4a7-9999 (an aside younger than the settle window is not reclaimed)
stat -c '%Z' → %Y	14 fails identically (see the round below — it did not, at first)
decline tracking reverted to [ "$pruned_any" = "1" ] || echo "no dead-branch caches found"	4 fails: expected in log: none pruned this pass
if is_locked "$aside" "$name" → if false	12 fails, as before

Scenario 14 runs against the script's own default settle window with nothing faked — the directory really was set aside a moment ago. Scenario 13, which is about the reader gate, moves the window rather than ageing the directory, because a ctime cannot be backdated the way touch -d backdates an mtime.

What is not covered by a test: the full two-pass interleaving itself, which would need two concurrent prune passes with injected scheduling. What is tested is the gate that makes the interleaving harmless. Saying so rather than shipping something that passes either way.

Second review round: the test that did not pin its own mechanism

Required change, and a sharp catch. Swapping the settle window's stat -c '%Z' for %Y — the exact substitution the comment beside it calls the wrong signal — left the suite green at 37/37.

The cause was in the fixture, not the script. Scenario 14 built its aside with mkdir -p, so the directory's mtime was also ~now and the two timestamps agreed. A fixture whose clocks agree cannot tell them apart. Every real aside is the opposite shape — a cache last written days ago, renamed a moment ago — so under %Y the window would never fire for anything real, the sweeper would silently return to reclaiming asides another pass is still deciding about, and the suite would say nothing. The mechanism guarded by a comment again, which is what finding 1 was about.

The fixture is now built the way the pass builds one:

mkdir -p "$victim"; head -c 4096 /dev/zero > "$victim/blob"
touch -d '2020-01-01' "$victim"      # an old cache, which is every cache
mv -T "$victim" "$aside"             # set aside a moment ago

Red/green, with the assertions untouched. (prune-cache.sh is touched in that commit, but only in a comment — the sharpened PID rationale below; no behaviour changed.)

	Result
As shipped	scenario 14 passes; suite 37/37
%Z → %Y, new fixture	14 fails: expected kept: …/.evicting-target-feat-dead-73dad4a7-9999 (an aside younger than the settle window is not reclaimed)
Settle guard deleted outright	14 fails identically

Also sharpened the PID rationale, which was right for a weaker reason than the one that actually holds: the $$ in an aside's name was that pass's PID inside its own job container, so testing it with kill -0 from a different one is not unreliable — it is meaningless. PIDs recycling is the lesser objection.

Third round: two cosmetic nits

• The sentence above left its paragraph at 127 characters in a file that otherwise wraps comments at 78–79 — the rewrap after the insert simply did not happen. awk 'length($0)>80' over both scripts now reports one comment line, the pre-existing 93-character usage string at prune-cache.sh:4.
• Scenario 14's header called its fixture "the only shape that can tell the two timestamps apart." The property it needs is old mtime with fresh ctime; an old directory renamed a moment ago is the production instance of that, not the only construction of it (touch -d alone would discriminate too). "Production's shape" was already carrying the argument, so the overclaim is gone rather than defended.

Neither touches behaviour or an assertion.

Delivering it: the v1 tag

This fix does not reach any consumer when this PR merges. That is not a
caveat about rollout, it is the mechanism: consumers pin
https://gitdan.com/daniel/gitdan-actions/cargo-cache@v1, v1 is a
lightweight tag (git cat-file -t v1 → commit), and a lightweight tag
does not follow a branch. It currently points at 3b2dec6 — 0 commits
behind origin/main, which is why "on main" and "what consumers run" have
been indistinguishable so far. This merge is the first thing that makes them
diverge, and at that moment emowheel is still executing the old
prune-cache.sh.

So README.md's Versioning section now records the release step that the
moving-tag model always implied but never stated. It already asserted the
model ("a moving major tag") — what it did not say was who moves it, when, or
what a consumer is owed by pinning it. Now added:

• Why the moving pointer, and not immutable v1.0.x tags. Recommended
  with reasoning rather than described neutrally: immutable tags are the safer
  model in general, and the wrong one here. They buy the ability to hold one
  consumer back; they cost a PR in every consumer repo per fix, and their real
  failure mode with two consumers and one operator is that the second is never
  updated and quietly runs a version nobody tests. The moving pointer makes a
  release one action with one blast radius, which is the thing worth being
  deliberate about — and anyone wanting immutability already has it by pinning
  a SHA.
• What @v1 promises: that it works with the documented inputs, spelled
  as documented. Renaming or removing an input, changing a default's
  behaviour, or requiring something new of the consuming workflow (another
  container.volumes entry, another permission) forces v2. Correctness
  fixes, new optional inputs and anything internal to scripts/ move v1.
• That moving it is a deliberate post-merge step and the operator's, never
  something a merge does by itself, because it changes what another
  repository's CI executes on its next run.
• Who is downstream: emowheel pins @v1 in five places in its CI workflow
  on dev (verified); zemyna is migrating to the same pin.

The step for this PR, to be run by the user after merge — deliberately not
run here:

git fetch origin
git tag -f v1 origin/main
git push -f origin v1
git ls-remote --tags origin v1     # must equal git rev-parse origin/main

Until that runs, emowheel's prune pass keeps its check-then-delete window.

Files affected

• scripts/prune-cache.sh — evict_dir, the deferred-eviction sweep, is_locked's name parameter, both call sites. Two log strings moved from present participle to past tense, since they now print after the fact.
• scripts/prune-cache-selftest.sh — scenarios 11/12/13/14 and the summary assertions in scenario 4; scenario 7's grep follows the log-wording change. 21 → 37 assertions.
• README.md — two separate edits. In the safety section the eviction case moves from "detected, not prevented" to a claim of its own alongside the publisher's, and the selftest table row is updated. In the Versioning section, the release policy above.

Nothing under cargo-cache/ or cargo-cache-publish/ is touched, no other script changed, and no tag is moved by this PR.

Test plan

• bash scripts/selftest.sh — all 5 suites passed, including the two compiler-backed ones, run from committed state after each commit.
• shellcheck scripts/prune-cache.sh scripts/prune-cache-selftest.sh — clean apart from the pre-existing SC1091 (info) on the dynamic cache-lib.sh source, byte-identical to the finding on the untouched HEAD version of the file.
• Nothing was run against the live cache volume on gitdan-ci; every scenario is fixtures in a mktemp -d.
• Tag facts verified rather than assumed: git cat-file -t v1 → commit (lightweight), git rev-parse v1 → 3b2dec6, git rev-list --count v1..origin/main → 0, and emowheel's origin/dev .gitea/workflows/ci.yaml grepped for its five @v1 pins.

For the user to smoke: nothing runs on a screen here. The reviewable judgement calls are (a) the structural-over-tripwire choice and its two justifications, and (b) the moving-tag recommendation. The observable one is post-merge: after the re-tag, emowheel's next CI run fetches the new prune-cache.sh, and its prune step should log identically to today except where an eviction is declined.

Closes #1. ## Summary - `scripts/prune-cache.sh`: both eviction sites now unlink through `evict_dir`, which **renames the candidate aside and re-examines it** before the `rm -rf`, instead of trusting a marker check taken seconds earlier. - A reprieved cache is restored under its own name; one whose name a concurrent seed has retaken is left aside and reclaimed by a **deferred-eviction sweep** at the top of the next pass. - `is_locked` takes an optional name so the reader-marker lookup still keys on the name a consumer resolved, not on whatever the pass has since renamed the directory to. - `scripts/prune-cache-selftest.sh`: three new scenarios, 21 → 31 assertions. - `README.md`: the "what this does and does not guarantee" section named this window as a documented residual. It no longer is. ## Why The ticket's AC offers two shapes and asks for one, justified. **Chosen: structural (shape 1).** Two reasons the tripwire was the weaker option here, both visible only from inside the code: 1. **The property a tripwire would assert is false of this script.** "Snapshots of non-protected refs are never eviction candidates" is not something `prune-cache.sh` holds — it evicts `snapshot-*` of any non-protected ref by design, and the existing selftest scenario 7 asserts exactly that eviction (it is where the deliberate target-before-snapshot ordering is observed). The property lives in `cargo-cache-publish/action.yml`'s publisher gate, a different action, out of this track's scope. A tripwire written in `prune-cache-selftest.sh` would either fail on landing or assert something this script does not control. 2. **The policy holding the window shut is weaker than the ticket assumed.** `cargo-cache` and `cargo-cache-publish` each take `protected-branches` as their **own independent input**. Nothing ties the two lists together. A consuming workflow that lists a publisher ref in one and not the other arms this window with no change to any code in this repo — not even the "add a non-protected publisher ref" the ticket describes, just a mismatch between two strings in a caller's YAML. **And it was cheap and idiomatic, as the ticket hoped.** No new mechanism: it is `publish-snapshot.sh`'s rotation, applied to eviction. Renaming aside costs one `rename(2)`; the re-scan is a directory listing. The multi-second `du` stays *outside* the window, before the rename. ## Why the second look is conclusive, not just closer The same ordering proof the publish side already rests on (`cache-lib.sh`, reader-marker section): - A consumer publishes `.reading-<dir>-<tag>` **before** it resolves the source path. - The eviction scan happens **after** the rename. - So a consumer that resolved this directory did so before the rename, and therefore published its marker before the scan — it cannot be missed. - A consumer arriving after the rename cannot resolve the path at all, and falls through to its cold-start path — the same safe degrade the publisher's swap window already produces. Renaming disturbs nothing already in flight: it unlinks no entry and leaves the source inode unchanged, which is precisely why the publish side can rotate a snapshot out from under a live reader. A declined eviction therefore costs a deferred eviction and nothing else. ## Red/green: the new test bites Scenario 12 reproduces the window deterministically rather than by timing. A consumer can publish its marker at any instant, including between the check and the unlink; the pass runs a `du -sk` in exactly that gap. So the test puts a `du` shim on `PATH` that publishes the marker for the candidate it is measuring — strictly after the check, strictly before the unlink — then execs the real `du`. Demonstrated both directions, in scratch copies: | Script under test | Result | |---|---| | **This PR** | `prune-cache-selftest: 37 assertions passed` | | `evict_dir "$dir" \|\| continue` reverted to `rm -rf "$dir"` at both sites, **everything else identical** | scenarios 1–11 pass, **12 fails**: `ASSERTION FAILED: expected kept: .../target-feat-dead-73dad4a7 (a cache claimed inside the eviction window is not unlinked)` — log shows `::warning::pruned dead-branch cache target-feat-dead-73dad4a7` | | Rename-aside kept, only the second look defeated (`if is_locked …` → `if false`) | **12 fails** identically | The third row is the one that matters: it isolates the assertion onto the load-bearing line rather than onto the refactor. And scenario 11 (a marker present *before* the pass starts) passes in every row — the pre-existing check already worked, so the new coverage is aimed at the gap and not at re-testing what did not need fixing. Scenario 13 covers the deferred sweep in both directions: not reclaimed while a reader is live, reclaimed once it is gone. ## What is still not guaranteed Stated plainly, since the ticket's alternative shape would have left the race intact: - **The reader-marker race in eviction is closed, not merely narrowed** — that is the claim, and it holds only for consumers that follow the marker-before-resolve ordering (`hardlink_clone_into` does; it is the only cloner in this repo). - **`.ci-lock-*` gets re-checked too**, since it moves with the rename — a free improvement, not the ticket's subject. It does not close the seed side's own `[ -d "$OWN_DIR" ]` → `write_cache_lock` gap in `seed-target-dir.sh:48`, which is that script's to hold and out of scope here. - **A `seed-fallback-dir` outside the cache root still has no interlock at all.** Unchanged, still documented in the README, still relying on the consumer's per-attempt verification. - **Disk reclamation stays bounded, not immediate**: a declined eviction leaves the cache on disk — under its own name, or aside until the next pass sweeps it. ## Review round (findings 1–4) **1. The sweeper depended on the thing this PR exists to remove.** Fair hit, and the sharpest finding in the review. Pass A renames a candidate aside; pass B's sweep sees an aside with no readers and reclaims it; A then finds a reader and restores — and since `rm -rf` traverses fd-relative, the rename does not stop it, so A can republish a **half-emptied tree under a live cache name**. `capacity: 1` bounds it today, which is precisely the class of argument this PR was written to stop relying on. Worth separating two properties the original comment ran together, because the fix only needs one of them: - **The unlink itself was never at risk.** The aside name only comes into existence *after* the evicting pass's rename, so a consumer that resolved the directory published its marker strictly before it, and therefore before the sweeper's count. That holds under any interleaving and needs no window. The comment asserted this — correctly — and was silent on the other half. - **What was missing** is that an aside with no readers is indistinguishable from one a pass has just created and is still deciding about. **Mechanism chosen: a settle window** (`EVICTION_ASIDE_SETTLE_SECONDS`, default 60), not the PID check. A PID is meaningless across the job containers these passes run in and recycles besides, so it would be precision about the wrong thing. The age is read from **ctime, not mtime**: `rename(2)` updates ctime, while mtime stays at whenever the cache was last written — days ago for a stale cache, which is the signal `list_by_lru` wants and exactly the wrong one here. Verified rather than assumed (`touch -d 2020-01-01`, `mv`, then `stat`: mtime `1577858400`, ctime `now`). An aside is in flight for one rename plus one marker glob — milliseconds — so 60 s is three orders of magnitude of headroom, and short enough that a genuine leftover is reclaimed by the next run rather than lingering while the volume is under pressure. **This is a bound, not a construction, and the code and README now say so** rather than asserting the broader property. The residual, stated plainly: an evicting pass suspended past the window and then resumed finds its aside reclaimed and fails its restore — and logs that it did. **2. `prune-cache.sh` — the pass summary contradicted the log above it.** A pass that declined every dead cache it found printed `no dead-branch caches found`. Declines are now tracked separately, and that case reports `every dead-branch cache found is still in use — none pruned this pass`. This also covers the pre-existing instance of the same falsehood on the `is_locked` decline path, which predates this PR. **3. `evict_dir` — the warning promised a reclamation that might not happen.** It now distinguishes the aside still being there (leaving it for a later pass) from its already being gone (another pass reclaimed it), instead of asserting the first. **4. README bullet 4 overstated.** Rewritten to lead with the clause that actually holds — never unlinked under the job that claimed it — with "under its own name, or aside" demoted to the normal case rather than a promise. **Not fixed, as agreed**: the reviewer's "starts cold" nit, and the cross-repo `dotglob` blind spot in `daniel/gitdan` (tracked separately; `hardlink_clone_into`'s `.stage-<tag>` is the same class and strictly worse). **Each new gate proven red by defeating it alone** in a scratch copy, everything else untouched: | Gate defeated | Result | |---|---| | `if [ "$aside_age" -lt "$EVICTION_ASIDE_SETTLE_SECONDS" ]` → `if false` | **14 fails**: `expected kept: …/.evicting-target-feat-dead-73dad4a7-9999 (an aside younger than the settle window is not reclaimed)` | | `stat -c '%Z'` → `%Y` | **14 fails** identically (see the round below — it did not, at first) | | decline tracking reverted to `[ "$pruned_any" = "1" ] \|\| echo "no dead-branch caches found"` | **4 fails**: `expected in log: none pruned this pass` | | `if is_locked "$aside" "$name"` → `if false` | **12 fails**, as before | Scenario 14 runs against the script's **own** default settle window with nothing faked — the directory really was set aside a moment ago. Scenario 13, which is about the reader gate, moves the window rather than ageing the directory, because a ctime cannot be backdated the way `touch -d` backdates an mtime. **What is not covered by a test**: the full two-pass interleaving itself, which would need two concurrent prune passes with injected scheduling. What is tested is the gate that makes the interleaving harmless. Saying so rather than shipping something that passes either way. ## Second review round: the test that did not pin its own mechanism Required change, and a sharp catch. Swapping the settle window's `stat -c '%Z'` for `%Y` — the exact substitution the comment beside it calls the wrong signal — **left the suite green at 37/37**. The cause was in the fixture, not the script. Scenario 14 built its aside with `mkdir -p`, so the directory's mtime was also ~now and the two timestamps agreed. A fixture whose clocks agree cannot tell them apart. Every real aside is the opposite shape — a cache last written days ago, renamed a moment ago — so under `%Y` the window would never fire for anything real, the sweeper would silently return to reclaiming asides another pass is still deciding about, and the suite would say nothing. The mechanism guarded by a comment again, which is what finding 1 was about. The fixture is now built the way the pass builds one: ```bash mkdir -p "$victim"; head -c 4096 /dev/zero > "$victim/blob" touch -d '2020-01-01' "$victim" # an old cache, which is every cache mv -T "$victim" "$aside" # set aside a moment ago ``` Red/green, with the assertions untouched. (`prune-cache.sh` is touched in that commit, but only in a comment — the sharpened PID rationale below; no behaviour changed.) | | Result | |---|---| | As shipped | scenario 14 passes; suite 37/37 | | `%Z` → `%Y`, new fixture | **14 fails**: `expected kept: …/.evicting-target-feat-dead-73dad4a7-9999 (an aside younger than the settle window is not reclaimed)` | | Settle guard deleted outright | **14 fails** identically | Also sharpened the PID rationale, which was right for a weaker reason than the one that actually holds: the `$$` in an aside's name was that pass's PID *inside its own job container*, so testing it with `kill -0` from a different one is not unreliable — it is meaningless. PIDs recycling is the lesser objection. ## Third round: two cosmetic nits - The sentence above left its paragraph at **127 characters** in a file that otherwise wraps comments at 78–79 — the rewrap after the insert simply did not happen. `awk 'length($0)>80'` over both scripts now reports one comment line, the pre-existing 93-character usage string at `prune-cache.sh:4`. - Scenario 14's header called its fixture *"the only shape that can tell the two timestamps apart."* The property it needs is old mtime with fresh ctime; an old directory renamed a moment ago is the **production instance** of that, not the only construction of it (`touch -d` alone would discriminate too). "Production's shape" was already carrying the argument, so the overclaim is gone rather than defended. Neither touches behaviour or an assertion. ## Delivering it: the `v1` tag **This fix does not reach any consumer when this PR merges.** That is not a caveat about rollout, it is the mechanism: consumers pin `https://gitdan.com/daniel/gitdan-actions/cargo-cache@v1`, `v1` is a **lightweight tag** (`git cat-file -t v1` → `commit`), and a lightweight tag does not follow a branch. It currently points at `3b2dec6` — **0 commits behind `origin/main`**, which is why "on `main`" and "what consumers run" have been indistinguishable so far. This merge is the first thing that makes them diverge, and at that moment emowheel is still executing the old `prune-cache.sh`. So `README.md`'s Versioning section now records the release step that the moving-tag model always implied but never stated. It already asserted the model ("a moving major tag") — what it did not say was who moves it, when, or what a consumer is owed by pinning it. Now added: - **Why the moving pointer, and not immutable `v1.0.x` tags.** Recommended with reasoning rather than described neutrally: immutable tags are the safer model in general, and the wrong one here. They buy the ability to hold one consumer back; they cost a PR in every consumer repo per fix, and their real failure mode with two consumers and one operator is that the second is never updated and quietly runs a version nobody tests. The moving pointer makes a release one action with one blast radius, which is the thing worth being deliberate about — and anyone wanting immutability already has it by pinning a SHA. - **What `@v1` promises**: that it works with the documented inputs, spelled as documented. Renaming or removing an input, changing a default's behaviour, or requiring something new of the consuming workflow (another `container.volumes` entry, another permission) forces `v2`. Correctness fixes, new optional inputs and anything internal to `scripts/` move `v1`. - **That moving it is a deliberate post-merge step and the operator's**, never something a merge does by itself, because it changes what another repository's CI executes on its next run. - **Who is downstream**: emowheel pins `@v1` in five places in its CI workflow on `dev` (verified); zemyna is migrating to the same pin. **The step for this PR, to be run by the user after merge — deliberately not run here:** ```bash git fetch origin git tag -f v1 origin/main git push -f origin v1 git ls-remote --tags origin v1 # must equal git rev-parse origin/main ``` Until that runs, emowheel's prune pass keeps its check-then-delete window. ## Files affected - `scripts/prune-cache.sh` — `evict_dir`, the deferred-eviction sweep, `is_locked`'s name parameter, both call sites. Two log strings moved from present participle to past tense, since they now print after the fact. - `scripts/prune-cache-selftest.sh` — scenarios 11/12/13/14 and the summary assertions in scenario 4; scenario 7's grep follows the log-wording change. 21 → 37 assertions. - `README.md` — two separate edits. In the safety section the eviction case moves from "detected, not prevented" to a claim of its own alongside the publisher's, and the selftest table row is updated. In the Versioning section, the release policy above. Nothing under `cargo-cache/` or `cargo-cache-publish/` is touched, no other script changed, and **no tag is moved by this PR**. ## Test plan - `bash scripts/selftest.sh` — **all 5 suites passed**, including the two compiler-backed ones, run from committed state after each commit. - `shellcheck scripts/prune-cache.sh scripts/prune-cache-selftest.sh` — clean apart from the pre-existing `SC1091` (info) on the dynamic `cache-lib.sh` source, byte-identical to the finding on the untouched `HEAD` version of the file. - Nothing was run against the live cache volume on `gitdan-ci`; every scenario is fixtures in a `mktemp -d`. - Tag facts verified rather than assumed: `git cat-file -t v1` → `commit` (lightweight), `git rev-parse v1` → `3b2dec6`, `git rev-list --count v1..origin/main` → `0`, and emowheel's `origin/dev` `.gitea/workflows/ci.yaml` grepped for its five `@v1` pins. **For the user to smoke**: nothing runs on a screen here. The reviewable judgement calls are (a) the structural-over-tripwire choice and its two justifications, and (b) the moving-tag recommendation. The observable one is post-merge: after the re-tag, emowheel's next CI run fetches the new `prune-cache.sh`, and its prune step should log identically to today except where an eviction is declined.

claude added the bug label 2026-08-24 04:11:49 +00:00

claude added 1 commit 2026-08-24 04:11:50 +00:00

fix(prune-cache): close the reader-marker check-then-delete window ... 88ab063b64

Both eviction sites checked for a consumer's `.reading-` marker and then,
seconds later — `usage_gb` runs `du -sk` over a multi-GB tree between the
two — unlinked the directory. A consumer that started a clone inside that
gap had its source removed mid-walk, which `cp -al` does not report: a
subtree unlinked before its parent is listed is silently omitted.

Unreachable today, and only by policy: snapshots belong to protected refs
and protected refs never reach the marker check. `cargo-cache` and
`cargo-cache-publish` take that ref list as two independent inputs, so a
workflow listing a publisher in one and not the other arms this with no
code change at all.

Closed structurally, with publish-snapshot.sh's rotation rather than a new
mechanism: the candidate is renamed aside and only then re-examined, so the
scan the unlink rests on happens strictly after the rename. A consumer that
resolved the directory published its marker before that scan and cannot be
missed; one arriving after cannot resolve the path and starts cold, the same
degrade the publisher's swap window already produces. Renaming disturbs no
clone in flight — no entry is unlinked and the inode is unchanged — so a
declined eviction costs a deferred eviction and nothing else. A reprieved
cache is put back under its own name; one whose name a concurrent seed has
retaken is left aside and swept by a later pass once its readers drain,
since nothing else globs a dotted name.

prune-cache-selftest gains three scenarios (21 -> 31 assertions). Scenario
12 is the one that bites: the `du` the pass runs on its candidate publishes
the marker, placing it strictly after the check and strictly before the
unlink. Against check-then-delete, 1-11 pass and 12 fails; breaking only
the second look and leaving the rename fails it too.

claude added 1 commit 2026-08-24 04:14:07 +00:00

docs(readme): say who moves the v1 tag, when, and what it promises ... c6a3fa6d97

The Versioning section named the moving-major-tag model but not the release
step that model implies. This is the first change where that gap has a
consequence: `v1` and `origin/main` are the same commit today, so merging
the eviction fix is the first thing that makes them diverge — at which point
the fix is on `main` and every consumer is still fetching the old scripts.

Records what was previously only implicit: that merging ships nothing, that
re-pointing `v1` is a deliberate post-merge action because it changes what
another repository's CI runs next, the exact commands and the check that it
took, what a consumer is promised by pinning `@v1` and what forces a `v2`,
and who is actually downstream. Also states why the moving pointer is the
right model here rather than immutable release tags, since "safer in
general" is the obvious objection and it deserves an answer.

Docs only; no script or action definition is touched.

claude changed title from WIP: fix(prune-cache): close the reader-marker check-then-delete window to WIP: fix(prune-cache): close the reader-marker check-then-delete window + document the v1 release step 2026-08-24 04:14:43 +00:00

claude-reviewer approved these changes 2026-08-24 04:22:30 +00:00

Dismissed

claude-reviewer left a comment

Collaborator

Copy Link

Copy Source

VERDICT: APPROVE-WITH-NITS

Reviewed against the AC in daniel/gitdan-actions#1, git diff origin/main...fix/prune-cache-reader-window (88ab063, c6a3fa6).

Wide pass — is this the right shape?

Yes. The ticket's "What would close this" offers either the structural close or a tripwire test; the author delivered both, and picking the structural option is inside the AC, not around it. The reviewer recommendation the ticket quotes ("don't add locking complexity to an unreachable path") is satisfied on its own terms: no locking primitive was added. evict_dir is a rename plus a re-run of the check that already existed, and it is the same rotation publish-snapshot.sh already uses — so the added machinery is a reuse of an in-repo pattern, not new vocabulary. Per-eviction cost is one rename(2) plus one marker glob, against a du -sk on a multi-GB tree in the same loop. That is not live complexity in a hot path.

Ordering proof — verified, not accepted from the comment. cache-lib.sh:399-403 is reader_lock_acquire → _dir_inode "$src" → cp -al "$src" "$tmp". Publish strictly precedes the resolve the clone rests on, so a consumer still reading at unlink time necessarily has its marker on disk before evict_dir's post-rename scan. The proof holds. One imprecision worth recording: seed-target-dir.sh:71-72 does [ -d "$src" ] and usage_gb "$src" (a full du) before any marker exists — but that read is not the one the clone rests on (hardlink_clone_into re-checks [ -d "$src" ] at :384 and re-resolves under the marker), so a torn du costs a wrong GB figure in a log line and nothing else.

Per-AC

AC	Cite	Result
Close the window structurally, re-deriving safety immediately before acting	prune-cache.sh:162-183 — rename to .evicting-<name>-$$, then is_locked "$aside" "$name", then rm -rf "$aside"	PASS
Both eviction sites, not one	:260 (liveness pass) and :293 (pressure pass) both go through evict_dir	PASS
Marker lookup keyed on the resolved name, not the renamed path	is_locked gained a name param, :117 live_reader_count "$ROOT" "$name"	PASS
A real tripwire	selftest 12 — see red/green below	PASS
Docs updated in the same PR	README "Four separate claims" bullet 2 rewritten from the check-then-delete admission to the by-construction claim; matches the code	PASS

Test integrity — red/green verified

Not taken on the author's word. Two scratch copies of the tree at HEAD; one had scripts/prune-cache.sh replaced with origin/main's. The only edit was test 7's grep verb (evicted → evicting), since the log noun-rename is part of the fix and test 7 would otherwise abort the run before reaching 12.

• Pre-fix: tests 1–11 pass, 12 fails — expected kept: .../target-feat-dead-73dad4a7 (a cache claimed inside the eviction window is not unlinked), log showing ::warning::pruning dead-branch cache target-feat-dead-… and the directory gone.
• Post-fix: prune-cache-selftest: 31 assertions passed.

Test 12 is a genuine contract, not a tautology: the stubbed du publishes the marker from inside the pass's own measurement step, which places it deterministically after the check and before the unlink — the interleaving is forced, not hoped for. It also asserts the contents survive (target-$DEAD/blob) and that no .evicting-* is left behind, so a "fix" that reprieved the name while losing the tree would still fail. Test 11 passes both ways, correctly — it is the non-race baseline, not the tripwire.

Narrow pass — the six probes

1. Sweep vs. a live eviction — reachable, and the one real residual. Pass A renames aside; pass B's sweep counts zero readers and rm -rfs it; A then finds readers and restores. Two outcomes. If B's rm finishes first, A's mv -T fails with ENOENT and the warning claims it is "leaving <aside> for a later pass to reclaim" — a directory that no longer exists. If B's rm is mid-walk, A's restore succeeds and puts a partially-emptied tree back under a live name (GNU rm -rf traverses fd-relative, so the rename does not stop it). Neither truncates a live clone: a marker appearing after the rename belongs to a consumer that could not resolve the path and is cold-starting, so it is not reading the aside; a consumer that was reading published its marker before the rename and B would have seen it. So the blast radius is a lost or holed cache plus a false log line — degrades to slow, never to wrong. Bounded today by capacity: 1 (gitdan/ansible/roles/act_runner/templates/config.yaml.j2:10). Worth naming plainly: the PR's stated purpose is to stop depending on policy, and the sweeper it adds depends on policy. The comment at :213-215 asserts "Safe under a concurrent pass mid-eviction" — true for the truncation property it names, silent on the sweep-vs-restore interaction. Cheap close if you want it: skip an aside whose $$ suffix is a live PID, or skip one younger than a few minutes.
2. Cross-repo blind spot — confirmed, but pre-existing in class. gitdan/scripts/ci-cache-reclaim.sh:288 iterates for entry in "$dir"/*, and the script sets only nullglob, never dotglob — so a .evicting-* aside is invisible to the host arbiter regardless of BRANCH_DIR_RE. An orphan on a dormant repo's volume is therefore unreclaimable by the outer ring, which is precisely the disk the outer ring exists to reclaim. What keeps this a nit: hardlink_clone_into's .stage-<tag> (cache-lib.sh:391) is already exactly this class and is strictly worse — tags are per-job-per-run unique, so a killed job's staging tree is never swept by anything, ever. This PR adds a dotted-name instance that at least has a sweeper. Suggest a follow-up on daniel/gitdan to teach the arbiter about dotted leftovers, covering both.
3. Aside-name parsing — correct. ${n#.evicting-} then ${n%-*} round-trips for target-<slug>-<hash>, snapshot-<slug>-<hash>, the double-dash form a 48-char slug truncation can produce (target-a--<hash>), and PIDs from 1 to 7 digits. $$ is always digits and cache_key never emits a trailing dash, so the last-dash split cannot cut into the name.
4. Test 12 red/green — verified above.
5. "Name occupied again" branch — reachable and correct. A concurrent seed recreating target-K makes the restore's destination a non-empty directory; mv -T fails with ENOTEMPTY rather than clobbering the new seed. Verified directly: mv -T onto a missing dest succeeds, onto a non-empty dir fails, onto an empty dir replaces it, onto a file fails. The aside is then reclaimed by test 13's swept path once the readers drain.
6. mv -T failure handling — the set-aside failure returns 1 and the caller || continues, so a failed rename costs a deferred eviction. Correct.

Docs

v1 verified as a lightweight tag: git ls-remote --tags origin returns 3b2dec6… refs/tags/v1, git cat-file -t on it is commit, not tag, and it equals current origin/main. So "does not follow a branch" and "every consumer keeps fetching the commit it already named" are both literally true, and the git ls-remote --tags origin v1 verification line prints the single line it implies (no ^{} deref line for a lightweight tag). The moving-pointer recommendation is argued rather than asserted — cost stated (a PR per consumer per fix), failure mode stated (two consumers, one operator, the second silently never updated), escape hatch stated (pin a SHA). The @v1 / v2 split is coherent and testable: renamed or removed input, behaviour-changing default, or a new requirement on the consuming workflow. Downstream claim spot-checked — emowheel/.gitea/workflows/ci.yaml:169,366 do pin cargo-cache@v1; zemyna is not checked out here so "migrating" is unverified.

"Four separate claims" is now accurate (four bullets), and the rewritten eviction bullet matches the code. One overstatement in bullet 4: "the cache stays, either under its own name or aside awaiting the next pass" — under finding 1 it can stay neither. The clause that actually matters, "never unlinked under the job that claimed it," survives.

Nits, none blocking

• prune-cache.sh:176 — the warning promises a later reclamation of an aside that may already be gone. Since the branch is only reachable via the concurrency in finding 1, it will read as a mystery in a log.
• prune-cache.sh:265 — a pass where every dead candidate is declined still prints "no dead-branch caches found". The decline lines are above it, so recoverable, but the summary contradicts them.
• README bullet 2 says a consumer arriving after the rename "starts cold". For a renamed target-* it falls through to the snapshot and starts warm-from-snapshot; "cold" here means "without its own dir", which the surrounding prose uses consistently enough not to mislead.

Verification run

• bash scripts/selftest.sh (full, compiler-backed) — selftest: all 5 suites passed, exit 0.
• shellcheck scripts/prune-cache.sh scripts/prune-cache-selftest.sh — only SC1091 info on the cache-lib.sh source line, identical to every other script in the repo and present on main. No new finding.
• All experiments on scratch copies under a temp dir; nothing was run against a real cache volume. git status --porcelain in the worktree is empty.

VERDICT: APPROVE-WITH-NITS Reviewed against the AC in daniel/gitdan-actions#1, `git diff origin/main...fix/prune-cache-reader-window` (88ab063, c6a3fa6). ## Wide pass — is this the right shape? Yes. The ticket's "What would close this" offers **either** the structural close **or** a tripwire test; the author delivered both, and picking the structural option is inside the AC, not around it. The reviewer recommendation the ticket quotes ("don't add locking complexity to an unreachable path") is satisfied on its own terms: **no locking primitive was added**. `evict_dir` is a rename plus a re-run of the check that already existed, and it is the same rotation `publish-snapshot.sh` already uses — so the added machinery is a reuse of an in-repo pattern, not new vocabulary. Per-eviction cost is one `rename(2)` plus one marker glob, against a `du -sk` on a multi-GB tree in the same loop. That is not live complexity in a hot path. **Ordering proof — verified, not accepted from the comment.** `cache-lib.sh:399-403` is `reader_lock_acquire` → `_dir_inode "$src"` → `cp -al "$src" "$tmp"`. Publish strictly precedes the resolve the clone rests on, so a consumer still reading at unlink time necessarily has its marker on disk before `evict_dir`'s post-rename scan. The proof holds. One imprecision worth recording: `seed-target-dir.sh:71-72` does `[ -d "$src" ]` and `usage_gb "$src"` (a full `du`) **before** any marker exists — but that read is not the one the clone rests on (`hardlink_clone_into` re-checks `[ -d "$src" ]` at :384 and re-resolves under the marker), so a torn `du` costs a wrong GB figure in a log line and nothing else. ## Per-AC | AC | Cite | Result | |---|---|---| | Close the window structurally, re-deriving safety immediately before acting | `prune-cache.sh:162-183` — rename to `.evicting-<name>-$$`, then `is_locked "$aside" "$name"`, then `rm -rf "$aside"` | PASS | | Both eviction sites, not one | `:260` (liveness pass) and `:293` (pressure pass) both go through `evict_dir` | PASS | | Marker lookup keyed on the resolved name, not the renamed path | `is_locked` gained a `name` param, `:117` `live_reader_count "$ROOT" "$name"` | PASS | | A real tripwire | selftest 12 — see red/green below | PASS | | Docs updated in the same PR | README "Four separate claims" bullet 2 rewritten from the check-then-delete admission to the by-construction claim; matches the code | PASS | ## Test integrity — red/green verified Not taken on the author's word. Two scratch copies of the tree at HEAD; one had `scripts/prune-cache.sh` replaced with `origin/main`'s. **The only edit was test 7's `grep` verb** (`evicted` → `evicting`), since the log noun-rename is part of the fix and test 7 would otherwise abort the run before reaching 12. * **Pre-fix:** tests 1–11 pass, **12 fails** — `expected kept: .../target-feat-dead-73dad4a7 (a cache claimed inside the eviction window is not unlinked)`, log showing `::warning::pruning dead-branch cache target-feat-dead-…` and the directory gone. * **Post-fix:** `prune-cache-selftest: 31 assertions passed`. Test 12 is a genuine contract, not a tautology: the stubbed `du` publishes the marker from inside the pass's own measurement step, which places it deterministically after the check and before the unlink — the interleaving is forced, not hoped for. It also asserts the *contents* survive (`target-$DEAD/blob`) and that no `.evicting-*` is left behind, so a "fix" that reprieved the name while losing the tree would still fail. Test 11 passes both ways, correctly — it is the non-race baseline, not the tripwire. ## Narrow pass — the six probes 1. **Sweep vs. a live eviction — reachable, and the one real residual.** Pass A renames aside; pass B's sweep counts zero readers and `rm -rf`s it; A then finds readers and restores. Two outcomes. If B's `rm` finishes first, A's `mv -T` fails with ENOENT and the warning claims it is "leaving `<aside>` for a later pass to reclaim" — a directory that no longer exists. If B's `rm` is mid-walk, A's restore succeeds and puts a partially-emptied tree back under a live name (GNU `rm -rf` traverses fd-relative, so the rename does not stop it). **Neither truncates a live clone**: a marker appearing after the rename belongs to a consumer that could not resolve the path and is cold-starting, so it is not reading the aside; a consumer that *was* reading published its marker before the rename and B would have seen it. So the blast radius is a lost or holed cache plus a false log line — degrades to slow, never to wrong. Bounded today by `capacity: 1` (`gitdan/ansible/roles/act_runner/templates/config.yaml.j2:10`). Worth naming plainly: **the PR's stated purpose is to stop depending on policy, and the sweeper it adds depends on policy.** The comment at `:213-215` asserts "Safe under a concurrent pass mid-eviction" — true for the truncation property it names, silent on the sweep-vs-restore interaction. Cheap close if you want it: skip an aside whose `$$` suffix is a live PID, or skip one younger than a few minutes. 2. **Cross-repo blind spot — confirmed, but pre-existing in class.** `gitdan/scripts/ci-cache-reclaim.sh:288` iterates `for entry in "$dir"/*`, and the script sets only `nullglob`, never `dotglob` — so a `.evicting-*` aside is invisible to the host arbiter regardless of `BRANCH_DIR_RE`. An orphan on a dormant repo's volume is therefore unreclaimable by the outer ring, which is precisely the disk the outer ring exists to reclaim. **What keeps this a nit:** `hardlink_clone_into`'s `.stage-<tag>` (`cache-lib.sh:391`) is already exactly this class and is strictly worse — tags are per-job-per-run unique, so a killed job's staging tree is never swept by anything, ever. This PR adds a dotted-name instance that at least has a sweeper. Suggest a follow-up on `daniel/gitdan` to teach the arbiter about dotted leftovers, covering both. 3. **Aside-name parsing — correct.** `${n#.evicting-}` then `${n%-*}` round-trips for `target-<slug>-<hash>`, `snapshot-<slug>-<hash>`, the double-dash form a 48-char slug truncation can produce (`target-a--<hash>`), and PIDs from 1 to 7 digits. `$$` is always digits and `cache_key` never emits a trailing dash, so the last-dash split cannot cut into the name. 4. **Test 12 red/green** — verified above. 5. **"Name occupied again" branch — reachable and correct.** A concurrent seed recreating `target-K` makes the restore's destination a non-empty directory; `mv -T` fails with `ENOTEMPTY` rather than clobbering the new seed. Verified directly: `mv -T` onto a missing dest succeeds, onto a non-empty dir fails, onto an empty dir replaces it, onto a file fails. The aside is then reclaimed by test 13's swept path once the readers drain. 6. **`mv -T` failure handling** — the set-aside failure returns 1 and the caller `|| continue`s, so a failed rename costs a deferred eviction. Correct. ## Docs `v1` verified as a lightweight tag: `git ls-remote --tags origin` returns `3b2dec6… refs/tags/v1`, `git cat-file -t` on it is `commit`, not `tag`, and it equals current `origin/main`. So "does not follow a branch" and "every consumer keeps fetching the commit it already named" are both literally true, and the `git ls-remote --tags origin v1` verification line prints the single line it implies (no `^{}` deref line for a lightweight tag). The moving-pointer recommendation is argued rather than asserted — cost stated (a PR per consumer per fix), failure mode stated (two consumers, one operator, the second silently never updated), escape hatch stated (pin a SHA). The `@v1` / `v2` split is coherent and testable: renamed or removed input, behaviour-changing default, or a new requirement on the consuming workflow. Downstream claim spot-checked — `emowheel/.gitea/workflows/ci.yaml:169,366` do pin `cargo-cache@v1`; zemyna is not checked out here so "migrating" is unverified. "Four separate claims" is now accurate (four bullets), and the rewritten eviction bullet matches the code. One overstatement in bullet 4: *"the cache stays, either under its own name or aside awaiting the next pass"* — under finding 1 it can stay neither. The clause that actually matters, "never unlinked under the job that claimed it," survives. ## Nits, none blocking * `prune-cache.sh:176` — the warning promises a later reclamation of an aside that may already be gone. Since the branch is only reachable via the concurrency in finding 1, it will read as a mystery in a log. * `prune-cache.sh:265` — a pass where every dead candidate is *declined* still prints "no dead-branch caches found". The decline lines are above it, so recoverable, but the summary contradicts them. * README bullet 2 says a consumer arriving after the rename "starts cold". For a renamed `target-*` it falls through to the snapshot and starts warm-from-snapshot; "cold" here means "without its own dir", which the surrounding prose uses consistently enough not to mislead. ## Verification run * `bash scripts/selftest.sh` (full, compiler-backed) — `selftest: all 5 suites passed`, exit 0. * `shellcheck scripts/prune-cache.sh scripts/prune-cache-selftest.sh` — only `SC1091` info on the `cache-lib.sh` source line, identical to every other script in the repo and present on `main`. No new finding. * All experiments on scratch copies under a temp dir; nothing was run against a real cache volume. `git status --porcelain` in the worktree is empty.

claude referenced this issue from a commit 2026-08-24 04:32:25 +00:00

fix(prune-cache): stop the aside sweeper depending on timing it cannot see

claude added 1 commit 2026-08-24 04:32:25 +00:00

fix(prune-cache): stop the aside sweeper depending on timing it cannot see ... 65f0782233

Review findings on #2. The first is the one that mattered: the sweeper this
PR added had the shape the PR exists to remove. Pass A renames a candidate
aside; pass B's sweep sees an aside with no readers and reclaims it; A then
finds a reader and restores. `rm -rf` traverses fd-relative, so the rename
does not stop it and A can republish a half-emptied tree under a live cache
name. `capacity: 1` bounds it today, which is exactly the kind of reason
this PR was written to stop relying on.

The unlink itself was never the problem — the ordering proof covers it under
any interleaving, since the aside name only exists after the evicting pass's
rename. What was missing is that an aside with no readers is indistinguishable
from one a pass has just created and not yet decided about. The sweeper now
leaves an aside alone until it has settled (EVICTION_ASIDE_SETTLE_SECONDS,
default 60), which separates the two without having to identify the pass that
created it — a PID is meaningless across the job containers these passes run
in, and recycles. Read from ctime, not mtime: rename(2) updates the first and
leaves the second at whenever the cache was last written, which is the signal
list_by_lru wants and the wrong one here.

That is a bound, not a construction, and both the code comment and the README
now say which of the two properties is which instead of asserting the broader
one.

Also from the review: a pass that declined every dead cache it found no
longer signs off with "no dead-branch caches found", and evict_dir no longer
promises a later reclamation of an aside that is already gone.

Scenario 14 covers the settle window against the script's own default, with
nothing faked — the directory really was set aside a moment ago. Scenario 4
gains the summary assertion. 31 -> 37 assertions; each new gate verified red
by defeating it alone in a scratch copy.

claude-reviewer requested changes 2026-08-24 04:43:40 +00:00

Dismissed

claude-reviewer left a comment

Collaborator

Copy Link

Copy Source

VERDICT: REQUEST-CHANGES

Re-review of 65f0782 on top of the previously approved c6a3fa6. One required change, roughly two lines, in the test — not in the script. Everything else in this round verified clean, including both mechanical claims I was asked to check rather than accept.

Headline

The settle window is right. The test that guards it is not. Substituting %Z with %Y — the exact mistake the new comment names as "exactly the wrong signal here" — leaves the entire suite green at 37/37, while making the window inert against every aside production will ever create. The mechanism this round adds is protected by a comment again, which is the thing #1 was filed about.

The two mechanical claims — verified, not accepted

1. rename(2) updates ctime and leaves mtime alone. True, and load-bearing exactly as described. Measured on both filesystems in play:

ext4:   mkdir d; touch -d 2020-01-01 d; mv -T d e
        -> mtime=2020-01-01   ctime=now
tmpfs:  same result

This is POSIX-mandated for rename(), not an ext4 accident, so it is not fragile. %Y is genuinely the wrong field and %Z is genuinely the right one.

2. 60s bounds the operation. Sanity-checked against a deliberately unfavourable root — 200 cache dirs, 50 live reader markers, 251 entries, 8 lock files in the aside:

live_reader_count            9.79 ms   (mostly the date(1) fork, not the glob)
rename + scan + restore     20.56 ms   per full in-flight cycle

~2,900× headroom. "Three orders of magnitude" is accurate rather than rhetorical. The glob does not degrade with volume size in any way that matters; it is a readdir plus fnmatch, and the du -sk in the same loop dwarfs it. And the window only has to cover rename→decision — once rm -rf starts, a concurrent sweeper's rm -rf on the same tree is a benign double-unlink, not a hazard. Correctly scoped.

REQUIRED — test 14 does not pin the choice its own comment calls a trap

Mutation-tested two ways against 65f0782:

Mutation	Result
Delete the settle guard entirely	RED — expected kept: .../.evicting-target-feat-dead-…-9999 (an aside younger than the settle window is not reclaimed)
stat -c '%Z' → stat -c '%Y'	GREEN — 37 assertions passed

The second one is not hypothetical breakage. Test 14 builds its aside with mkdir -p and then writes blob into it, so the fixture's mtime is ~now and %Y and %Z agree. Every real aside is the opposite: a cache dir last written days ago, renamed a moment ago. Demonstrated end to end on a fixture shaped the production way — backdate the cache dir, then set it aside with a real mv, then run the script:

[%Z, as shipped] aside mtime age=209687820s  ctime age=0s
  prune: .evicting-target-…-9999 was set aside 0s ago — another pass may still
         be evicting it, leaving it alone
  RESULT: aside SURVIVED (settle window fired)

[%Y, the warned-against regression] aside mtime age=209687821s  ctime age=0s
  prune: reclaiming deferred eviction .evicting-target-…-9999
  RESULT: aside RECLAIMED immediately (settle window inert)

Under %Y the window never fires for anything real, and the round silently reverts to the state finding 1 described. The suite says nothing.

The change: build test 14's aside the way production does — backdate the directory and rename it into the aside name, rather than mkdir-ing the aside directly:

mkdir -p "$root/target-$DEAD-victim"; head -c 4096 /dev/zero > "$root/target-$DEAD-victim/blob"
touch -d '2020-01-01' "$root/target-$DEAD-victim"      # an old cache, which is every cache
mv -T "$root/target-$DEAD-victim" "$aside"             # set aside a moment ago

That is the same standard test 12 already meets, and the same standard I applied to it last round. Its red/green is demonstrable: with the fixture above, %Y computes a multi-year age, the window does not fire, and assert_kept "$aside" goes red. Nothing else needs to change — not the assertions, not the script.

Everything else this round — verified good

Is the sweeper now free of policy dependence? It has traded a topology bound for a local bound, and that is the right trade, not a lateral move. capacity: 1 was invisible to this script, lived in another repository's Ansible tree, and would have changed silently. 60 seconds is declared in the file, tunable by env var, documented in the header, and — decisively — the correctness property no longer rests on it at all. The comment's split is the honest one and I checked it holds: no clone can be truncated by the sweeper's unlink under any interleaving, because the aside name postdates the rename and a consumer that resolved the directory published its marker strictly before it. The settle window guards only "mid-eviction vs. leftover", whose worst case is disk and a half-emptied tree, never a torn clone. A bound guarding a disk property with a correctness property proved separately is exactly the right shape.

Rejecting the PID option is also right, and for a better reason than the comment gives: PIDs would have to agree across container boundaries, and the $$ in an aside name comes from whichever job container created it — kill -0 in a later container is not merely unreliable, it is meaningless.

The named residual is honest. A pass suspended past the window, resumed, finds its aside reclaimed and fails its restore. Reachable only under concurrent passes plus a 60s stall inside a 20ms window. Naming it in the code is sufficient; I would not push it into the README, which already carries the more useful sentence — that the settle window "is a bound rather than a construction, and it is the only part of this that is." That is the sentence a future reader needs.

No env leakage between scenarios. Not taken on inspection — traced the value every invocation actually saw:

runs 1-12: SETTLE_SEEN=60   (script default)
runs 13-14: SETTLE_SEEN=0   (scenario 13, which asks for it)
run 15:    SETTLE_SEEN=60   (back to the default)

The unset in the else branch does its job, including across scenario 12's subshell. Scenario 13's switch to an explicit 0 is the right call — its comment correctly notes ctime cannot be backdated the way touch -d backdates an mtime, so moving the window out of the way beats trying to age the fixture into it.

The three nits from last round are closed, and the summary one bites. Reverting :265 to [ "$pruned_any" = "1" ] || echo "no dead-branch caches found" turns scenario 4 red (expected in log: none pruned this pass). The test asserts the positive and greps for the absence of the old string, so it cannot pass by accident. spared_any is set on both decline paths. The :176 split between "name is taken again" (aside present) and "already gone" (another pass reclaimed it) is correct — the second branch is only reachable under the concurrency it describes, and being a log-string branch, leaving it untested is fine. README bullet 4's overstatement is gone.

Nit, not blocking, and NOT this PR's

bash scripts/selftest.sh failed once for me at 65f0782:

=== 8: seeding while the base republishes the snapshot underneath it ===
PASS: caught the consumer's clone mid-walk at 10372/48805 entries
PASS: the seed completed
ASSERTION FAILED: the seeded tree is truncated: 48805 entries against the
snapshot's 493 (was 48805 before the rotation)

This is not attributable to the diff — seed-target-dir-selftest.sh, seed-target-dir.sh and cache-lib.sh are untouched by both commits. It is a race in the test's own premise: it asserts the consumer must end up holding the new generation, which only holds if the rotation actually tears the walk. When the reader-marker interlock does its job and the publish finishes after the consumer's final inode check, the consumer legitimately completes a whole, consistent generation 1 — and the test calls that "truncated". I could not reproduce it: 6/6 clean in isolation and 6/6 under saturating CPU load, on both origin/main and this branch, plus subsequent clean full-suite runs on both. Flagging it because it contradicts a clean-run report, and because a flaky assertion in the repo's only gate is worth its own issue on daniel/gitdan-actions rather than folding into this PR.

Verification run

• bash scripts/prune-cache-selftest.sh at 65f0782 — 37 assertions passed.
• bash scripts/selftest.sh (full, compiler-backed) at 65f0782 — passed on re-run; see the nit above for the one-off.
• Same full suite on origin/main as a control — passed.
• shellcheck scripts/prune-cache.sh scripts/prune-cache-selftest.sh — nothing beyond the repo-wide pre-existing SC1091 on the cache-lib.sh source line.
• Every experiment on scratch copies under a temp dir; nothing run against a real cache volume. git status --porcelain in the worktree is empty.

VERDICT: REQUEST-CHANGES Re-review of `65f0782` on top of the previously approved `c6a3fa6`. **One required change, roughly two lines, in the test — not in the script.** Everything else in this round verified clean, including both mechanical claims I was asked to check rather than accept. ## Headline **The settle window is right. The test that guards it is not.** Substituting `%Z` with `%Y` — the exact mistake the new comment names as "exactly the wrong signal here" — leaves the entire suite green at 37/37, while making the window inert against every aside production will ever create. The mechanism this round adds is protected by a comment again, which is the thing #1 was filed about. ## The two mechanical claims — verified, not accepted **1. `rename(2)` updates ctime and leaves mtime alone.** True, and load-bearing exactly as described. Measured on both filesystems in play: ``` ext4: mkdir d; touch -d 2020-01-01 d; mv -T d e -> mtime=2020-01-01 ctime=now tmpfs: same result ``` This is POSIX-mandated for `rename()`, not an ext4 accident, so it is not fragile. `%Y` is genuinely the wrong field and `%Z` is genuinely the right one. **2. 60s bounds the operation.** Sanity-checked against a deliberately unfavourable root — 200 cache dirs, 50 live reader markers, 251 entries, 8 lock files in the aside: ``` live_reader_count 9.79 ms (mostly the date(1) fork, not the glob) rename + scan + restore 20.56 ms per full in-flight cycle ``` **~2,900× headroom.** "Three orders of magnitude" is accurate rather than rhetorical. The glob does not degrade with volume size in any way that matters; it is a `readdir` plus `fnmatch`, and the `du -sk` in the same loop dwarfs it. And the window only has to cover rename→decision — once `rm -rf` starts, a concurrent sweeper's `rm -rf` on the same tree is a benign double-unlink, not a hazard. Correctly scoped. ## REQUIRED — test 14 does not pin the choice its own comment calls a trap Mutation-tested two ways against `65f0782`: | Mutation | Result | |---|---| | Delete the settle guard entirely | **RED** — `expected kept: .../.evicting-target-feat-dead-…-9999 (an aside younger than the settle window is not reclaimed)` | | `stat -c '%Z'` → `stat -c '%Y'` | **GREEN — 37 assertions passed** | The second one is not hypothetical breakage. Test 14 builds its aside with `mkdir -p` and then writes `blob` into it, so the fixture's mtime *is* ~now and `%Y` and `%Z` agree. Every real aside is the opposite: a cache dir last written days ago, renamed a moment ago. Demonstrated end to end on a fixture shaped the production way — backdate the cache dir, then set it aside with a real `mv`, then run the script: ``` [%Z, as shipped] aside mtime age=209687820s ctime age=0s prune: .evicting-target-…-9999 was set aside 0s ago — another pass may still be evicting it, leaving it alone RESULT: aside SURVIVED (settle window fired) [%Y, the warned-against regression] aside mtime age=209687821s ctime age=0s prune: reclaiming deferred eviction .evicting-target-…-9999 RESULT: aside RECLAIMED immediately (settle window inert) ``` Under `%Y` the window never fires for anything real, and the round silently reverts to the state finding 1 described. The suite says nothing. **The change:** build test 14's aside the way production does — backdate the directory and rename it into the aside name, rather than `mkdir`-ing the aside directly: ```bash mkdir -p "$root/target-$DEAD-victim"; head -c 4096 /dev/zero > "$root/target-$DEAD-victim/blob" touch -d '2020-01-01' "$root/target-$DEAD-victim" # an old cache, which is every cache mv -T "$root/target-$DEAD-victim" "$aside" # set aside a moment ago ``` That is the same standard test 12 already meets, and the same standard I applied to it last round. Its red/green is demonstrable: with the fixture above, `%Y` computes a multi-year age, the window does not fire, and `assert_kept "$aside"` goes red. Nothing else needs to change — not the assertions, not the script. ## Everything else this round — verified good **Is the sweeper now free of policy dependence?** It has traded a *topology* bound for a *local* bound, and that is the right trade, not a lateral move. `capacity: 1` was invisible to this script, lived in another repository's Ansible tree, and would have changed silently. 60 seconds is declared in the file, tunable by env var, documented in the header, and — decisively — **the correctness property no longer rests on it at all.** The comment's split is the honest one and I checked it holds: no clone can be truncated by the sweeper's unlink under *any* interleaving, because the aside name postdates the rename and a consumer that resolved the directory published its marker strictly before it. The settle window guards only "mid-eviction vs. leftover", whose worst case is disk and a half-emptied tree, never a torn clone. A bound guarding a disk property with a correctness property proved separately is exactly the right shape. Rejecting the PID option is also right, and for a better reason than the comment gives: PIDs would have to agree across container boundaries, and the `$$` in an aside name comes from whichever job container created it — `kill -0` in a later container is not merely unreliable, it is meaningless. **The named residual is honest.** A pass suspended past the window, resumed, finds its aside reclaimed and fails its restore. Reachable only under concurrent passes plus a 60s stall inside a 20ms window. Naming it in the code is sufficient; I would not push it into the README, which already carries the more useful sentence — that the settle window "is a bound rather than a construction, and it is the only part of this that is." That is the sentence a future reader needs. **No env leakage between scenarios.** Not taken on inspection — traced the value every invocation actually saw: ``` runs 1-12: SETTLE_SEEN=60 (script default) runs 13-14: SETTLE_SEEN=0 (scenario 13, which asks for it) run 15: SETTLE_SEEN=60 (back to the default) ``` The `unset` in the else branch does its job, including across scenario 12's subshell. Scenario 13's switch to an explicit `0` is the right call — its comment correctly notes ctime cannot be backdated the way `touch -d` backdates an mtime, so moving the window out of the way beats trying to age the fixture into it. **The three nits from last round are closed, and the summary one bites.** Reverting `:265` to `[ "$pruned_any" = "1" ] || echo "no dead-branch caches found"` turns scenario 4 red (`expected in log: none pruned this pass`). The test asserts the positive *and* greps for the absence of the old string, so it cannot pass by accident. `spared_any` is set on both decline paths. The `:176` split between "name is taken again" (aside present) and "already gone" (another pass reclaimed it) is correct — the second branch is only reachable under the concurrency it describes, and being a log-string branch, leaving it untested is fine. README bullet 4's overstatement is gone. ## Nit, not blocking, and NOT this PR's `bash scripts/selftest.sh` failed once for me at `65f0782`: ``` === 8: seeding while the base republishes the snapshot underneath it === PASS: caught the consumer's clone mid-walk at 10372/48805 entries PASS: the seed completed ASSERTION FAILED: the seeded tree is truncated: 48805 entries against the snapshot's 493 (was 48805 before the rotation) ``` **This is not attributable to the diff** — `seed-target-dir-selftest.sh`, `seed-target-dir.sh` and `cache-lib.sh` are untouched by both commits. It is a race in the test's own premise: it asserts the consumer must end up holding the *new* generation, which only holds if the rotation actually tears the walk. When the reader-marker interlock does its job and the publish finishes after the consumer's final inode check, the consumer legitimately completes a whole, consistent generation 1 — and the test calls that "truncated". I could not reproduce it: 6/6 clean in isolation and 6/6 under saturating CPU load, on both `origin/main` and this branch, plus subsequent clean full-suite runs on both. Flagging it because it contradicts a clean-run report, and because a flaky assertion in the repo's only gate is worth its own issue on `daniel/gitdan-actions` rather than folding into this PR. ## Verification run * `bash scripts/prune-cache-selftest.sh` at `65f0782` — 37 assertions passed. * `bash scripts/selftest.sh` (full, compiler-backed) at `65f0782` — passed on re-run; see the nit above for the one-off. * Same full suite on `origin/main` as a control — passed. * `shellcheck scripts/prune-cache.sh scripts/prune-cache-selftest.sh` — nothing beyond the repo-wide pre-existing `SC1091` on the `cache-lib.sh` source line. * Every experiment on scratch copies under a temp dir; nothing run against a real cache volume. `git status --porcelain` in the worktree is empty.

claude referenced this pull request 2026-08-24 04:45:05 +00:00

seed-target-dir-selftest scenario 8 is flaky: it asserts the rotation tore the clone, which is not guaranteed #3

claude referenced this issue from a commit 2026-08-24 04:46:05 +00:00

test(prune-cache): build scenario 14's aside the way the pass builds one

claude added 1 commit 2026-08-24 04:46:05 +00:00

test(prune-cache): build scenario 14's aside the way the pass builds one ... dcd73dd82d

Review finding on #2. Swapping the settle window's `stat -c %Z` for `%Y` —
the exact substitution the comment beside it calls the wrong signal — left
the suite green at 37/37. The scenario built its aside with `mkdir`, so the
fixture's mtime was also ~now and the two timestamps agreed; a fixture whose
clocks agree cannot tell them apart.

Every real aside is the opposite shape: a cache last written days ago,
renamed a moment ago. Under `%Y` the window would never fire for one, the
sweeper would silently return to reclaiming asides another pass is still
deciding about, and nothing would say so — the mechanism guarded by a comment
again, which is what the previous finding was about.

So the fixture is now built the way the pass builds one: an old directory
`mv`d into the aside name. `%Z` -> `%Y` now fails scenario 14, as does
deleting the guard outright.

Also sharpens why the PID alternative was rejected: the `$$` in an aside's
name was that pass's PID inside its own job container, so testing it from
another one is not unreliable, it is meaningless.

No change to prune-cache.sh's behaviour; the assertions are untouched.

claude-reviewer approved these changes 2026-08-24 04:51:13 +00:00

claude-reviewer left a comment

Collaborator

Copy Link

Copy Source

VERDICT: APPROVE-WITH-NITS

Re-review of dcd73dd. The required change from the previous round is done and the tripwire now trips. Two cosmetic nits, neither gating.

The mutation matrix — re-run independently

Three scratch clones of dcd73dd, mutated and run without reference to the reported result:

Tree	Result
Baseline	GREEN, exit 0, 37 assertions passed
Settle guard deleted outright	RED, exit 1 — expected kept: …/.evicting-target-feat-dead-73dad4a7-9999 (an aside younger than the settle window is not reclaimed)
stat -c '%Z' → '%Y' at :253	RED, exit 1 — same assertion

The regression that was invisible last round is caught. Scenario 14 now bites in both directions, which is what makes it a tripwire rather than a description.

1. Does the new fixture leave the other scenarios alone? Yes — traced, not inspected

target-$DEAD-victim matches the "$ROOT"/target-* glob both eviction passes use, so "created inside the same root" is a fair thing to worry about. It never coexists with a running pass. Instrumented the script to dump ls -A "$ROOT" on entry to every invocation across a whole suite run:

invocations traced: 15
occurrences of '-victim' in any of them: 0

The victim is created and renamed inside scenario 14 before its single run_prune, so no pass ever sees the name. Root contents during that invocation are exactly reset_cache's eight directories plus the aside — no residue from scenario 13, whose own aside was reclaimed.

Two notes on ordering. Scenario 14 is the last one in the file; there is no scenario 15, so nothing inherits from it either way (the trailing reset_cache question is moot rather than answered). And scenario 14 turns out to do slightly more than it claims: target-$DEAD / snapshot-$DEAD are both live in the root during its run and are pruned normally through evict_dir, so the scenario incidentally covers "the sweep leaves one aside alone while the same pass evicts other directories through asides of its own." Those use a different $$ suffix and do not collide. Worth knowing, not worth asserting.

2. Does the fixture's premise hold on any filesystem the tests can run on? Yes, and by a wide margin

Correctly flagged: the fixture now depends on the same POSIX property as the code, so it is worth stating rather than assuming. rename(2) is specified to mark st_ctime for update while leaving st_mtime alone, and touch -d sets mtime while ctime follows the metadata write — so the two clocks disagree by construction. Measured on both filesystems in play:

tmpfs        mtime age=209688554s  ctime age=0s   -> %Z fires, %Y does not
ext4         mtime age=209688554s  ctime age=0s   -> %Z fires, %Y does not

mktemp -d with TMPDIR unset lands on tmpfs here, which is where the suite actually runs; the real cache volume is ext4. Both behave identically. And the discrimination is not marginal — ~6.6 years against 0 seconds, against a 60-second boundary — so there is no clock-granularity or slow-machine flakiness in it. This will not become an intermittent.

3. Is the comment accurate? Yes

"a fixture whose two clocks agree cannot tell %Z from %Y: the settle window would read the wrong one, never fire for any real aside, and this scenario would not notice" — that is exactly the failure, correctly attributed, and it reads for someone who was not in the review thread: it explains why the fixture is shaped the way it is instead of just asserting that it is. Dropping "Nothing here fakes an age" was right; that line described the old fixture and would now be actively misleading, since the fixture does deliberately fake an mtime.

Script delta

The script is not untouched — prune-cache.sh has a comment change in this commit, absorbing the point about $$ being a PID from another container. Verified it is comment-only: diffing both revisions with comment lines stripped produces no delta, so there is no behavioural change to re-verify. Flagging it only because a reviewer told the script was untouched might not look.

Nits

• prune-cache.sh:95 is 127 characters, where the file's prevailing comment wrap is 78–79 and its previous longest line was 93. The rewrap after the PID sentence was inserted didn't happen, leaving one line running well past every other in a file whose comments are otherwise meticulously wrapped. Cosmetic, but conspicuous in this file.
• Scenario 14's header calls the fixture "the only shape that can tell the two timestamps apart." The necessary property is "old mtime, fresh ctime"; an old directory renamed a moment ago is the production instance of it, not the only one (touch -d alone would also discriminate). "Production's shape" already carries the argument — the "only" claim is slightly stronger than true and doesn't need to be.
• Scenario 8's intermittent failure in seed-target-dir-selftest.sh is tracked as #3 with the diagnosis. Out of scope here, and correctly so.

Verification run

• bash scripts/prune-cache-selftest.sh at dcd73dd — 37 assertions passed.
• bash scripts/selftest.sh (full, compiler-backed) — all 5 suites passed, exit 0.
• shellcheck scripts/prune-cache.sh scripts/prune-cache-selftest.sh — nothing beyond the repo-wide pre-existing SC1091 on the cache-lib.sh source line.
• Everything verified in the previous two rounds stands: the ordering proof against cache-lib.sh:399-403, scenario 12's red/green, the settle window's ~2,900× headroom, no env leakage across scenarios, the :265 summary and :176 warning split (both mutation-checked), README bullet 4.
• Every experiment on scratch copies under a temp dir; nothing run against a real cache volume. git status --porcelain in the worktree is empty.

VERDICT: APPROVE-WITH-NITS Re-review of `dcd73dd`. The required change from the previous round is done and the tripwire now trips. Two cosmetic nits, neither gating. ## The mutation matrix — re-run independently Three scratch clones of `dcd73dd`, mutated and run without reference to the reported result: | Tree | Result | |---|---| | Baseline | **GREEN**, exit 0, `37 assertions passed` | | Settle guard deleted outright | **RED**, exit 1 — `expected kept: …/.evicting-target-feat-dead-73dad4a7-9999 (an aside younger than the settle window is not reclaimed)` | | `stat -c '%Z'` → `'%Y'` at `:253` | **RED**, exit 1 — same assertion | The regression that was invisible last round is caught. Scenario 14 now bites in both directions, which is what makes it a tripwire rather than a description. ## 1. Does the new fixture leave the other scenarios alone? Yes — traced, not inspected `target-$DEAD-victim` matches the `"$ROOT"/target-*` glob both eviction passes use, so "created inside the same root" is a fair thing to worry about. It never coexists with a running pass. Instrumented the script to dump `ls -A "$ROOT"` on entry to every invocation across a whole suite run: ``` invocations traced: 15 occurrences of '-victim' in any of them: 0 ``` The victim is created and renamed inside scenario 14 before its single `run_prune`, so no pass ever sees the name. Root contents during that invocation are exactly `reset_cache`'s eight directories plus the aside — no residue from scenario 13, whose own aside was reclaimed. Two notes on ordering. Scenario 14 is the last one in the file; there is no scenario 15, so nothing inherits from it either way (the trailing `reset_cache` question is moot rather than answered). And scenario 14 turns out to do slightly more than it claims: `target-$DEAD` / `snapshot-$DEAD` are both live in the root during its run and are pruned normally through `evict_dir`, so the scenario incidentally covers "the sweep leaves one aside alone while the same pass evicts other directories through asides of its own." Those use a different `$$` suffix and do not collide. Worth knowing, not worth asserting. ## 2. Does the fixture's premise hold on any filesystem the tests can run on? Yes, and by a wide margin Correctly flagged: **the fixture now depends on the same POSIX property as the code**, so it is worth stating rather than assuming. `rename(2)` is specified to mark `st_ctime` for update while leaving `st_mtime` alone, and `touch -d` sets mtime while ctime follows the metadata write — so the two clocks disagree by construction. Measured on both filesystems in play: ``` tmpfs mtime age=209688554s ctime age=0s -> %Z fires, %Y does not ext4 mtime age=209688554s ctime age=0s -> %Z fires, %Y does not ``` `mktemp -d` with `TMPDIR` unset lands on tmpfs here, which is where the suite actually runs; the real cache volume is ext4. Both behave identically. And the discrimination is not marginal — ~6.6 years against 0 seconds, against a 60-second boundary — so there is no clock-granularity or slow-machine flakiness in it. This will not become an intermittent. ## 3. Is the comment accurate? Yes *"a fixture whose two clocks agree cannot tell %Z from %Y: the settle window would read the wrong one, never fire for any real aside, and this scenario would not notice"* — that is exactly the failure, correctly attributed, and it reads for someone who was not in the review thread: it explains why the fixture is shaped the way it is instead of just asserting that it is. Dropping *"Nothing here fakes an age"* was right; that line described the old fixture and would now be actively misleading, since the fixture does deliberately fake an mtime. ## Script delta **The script is not untouched** — `prune-cache.sh` has a comment change in this commit, absorbing the point about `$$` being a PID from another container. Verified it is comment-only: diffing both revisions with comment lines stripped produces no delta, so there is no behavioural change to re-verify. Flagging it only because a reviewer told the script was untouched might not look. ## Nits * **`prune-cache.sh:95` is 127 characters**, where the file's prevailing comment wrap is 78–79 and its previous longest line was 93. The rewrap after the PID sentence was inserted didn't happen, leaving one line running well past every other in a file whose comments are otherwise meticulously wrapped. Cosmetic, but conspicuous in this file. * **Scenario 14's header calls the fixture "the only shape that can tell the two timestamps apart."** The necessary property is "old mtime, fresh ctime"; an old directory renamed a moment ago is the *production* instance of it, not the only one (`touch -d` alone would also discriminate). "Production's shape" already carries the argument — the "only" claim is slightly stronger than true and doesn't need to be. * Scenario 8's intermittent failure in `seed-target-dir-selftest.sh` is tracked as #3 with the diagnosis. Out of scope here, and correctly so. ## Verification run * `bash scripts/prune-cache-selftest.sh` at `dcd73dd` — 37 assertions passed. * `bash scripts/selftest.sh` (full, compiler-backed) — `all 5 suites passed`, exit 0. * `shellcheck scripts/prune-cache.sh scripts/prune-cache-selftest.sh` — nothing beyond the repo-wide pre-existing `SC1091` on the `cache-lib.sh` source line. * Everything verified in the previous two rounds stands: the ordering proof against `cache-lib.sh:399-403`, scenario 12's red/green, the settle window's ~2,900× headroom, no env leakage across scenarios, the `:265` summary and `:176` warning split (both mutation-checked), README bullet 4. * Every experiment on scratch copies under a temp dir; nothing run against a real cache volume. `git status --porcelain` in the worktree is empty.

claude referenced this issue from a commit 2026-08-24 14:10:46 +00:00

style(prune-cache): rewrap the settle-window comment; drop an overclaim

claude added 1 commit 2026-08-24 14:10:46 +00:00

style(prune-cache): rewrap the settle-window comment; drop an overclaim ... b0d63c807b

Review nits on #2, both cosmetic.

The sentence added last round left its paragraph at 127 characters in a file
that otherwise wraps comments at 78-79 — the rewrap after the insert simply
did not happen. `awk 'length($0)>80'` over both files now reports one comment
line, the pre-existing 93-character usage string at :4.

Scenario 14's header called its fixture "the only shape that can tell the two
timestamps apart". The property it needs is old mtime with fresh ctime; an
old directory renamed a moment ago is the production instance of that, not
the only construction of it. "Production's shape" was already carrying the
argument.

No behaviour change and no assertion change.

claude changed title from WIP: fix(prune-cache): close the reader-marker check-then-delete window + document the v1 release step to fix(prune-cache): close the reader-marker check-then-delete window 2026-08-24 14:13:08 +00:00

claude merged commit ba7a44cfb2 into main 2026-08-24 14:35:53 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

claude-reviewer

Labels

Clear labels

bug

Something is broken or not working as intended

docs-drift-catalog

The standing per-repo catalog of known stale documentation and comments.

enhancement

New feature or improvement

question

Further information is requested

No labels bug

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

claude claude-reviewer daniel (Daniel Lynn)

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: daniel/gitdan-actions#2