Adds scenario 11 to scripts/seed-target-dir-selftest.sh, which pins reader_lock_acquire in hardlink_clone_into (scripts/cache-lib.sh:486)
by racing a real hardlink_clone_into clone against a real, concurrent publish-snapshot.sh republish of the exact snapshot being cloned, and
asserting the publisher's own log reports entering its reader-drain
wait — not merely that the run succeeded.
Why
Fixes#10. Deleting reader_lock_acquire from hardlink_clone_into left all five suites green at the identical 129 assertions: the sibling call, reader_lock_release, is already pinned (removing it reddens two suites via
the drain timing out), but nothing exercised the marker's creation. The
consumer's marker is what publish-snapshot.sh's wait_for_readers checks
before it reclaims a rotated-away snapshot generation
(scripts/cache-lib.sh:493-524, scripts/publish-snapshot.sh's post-swap
scan) — without it, a publisher finishing a swap while a consumer is
mid-cp -al can unlink the generation being read, the silent-truncation mode seed-target-dir-selftest.sh's scenario 8b reproduces directly.
The trap the issue calls out: this line is not dormant. Scenario 8a already
forces the publisher to block on this consumer's marker mid-race, and its log
already contains lines like readers: waiting for 1 in-flight clone(s) of snapshot-dev-<key> (grace 300s) / readers: snapshot-dev-<key> drained after 1s — but nothing reads them back. Remove the line and those lines vanish,
the publisher never waits, and 8a still passes with the same assertions.
Design
The issue flags a real tension: 8a currently pins exactly one property (the
identity check) and is reddened by exactly one mutant — the suite's
one-scenario-one-mutant diagonal across scenarios 8a to 8d and 10 is
deliberate, and folding a marker assertion into 8a would cost it. Scenario 11
is a separate scenario, reusing 8a's technique for forcing the
interleaving deterministically (a stubbed cp that fires once on the
consumer's own top-level clone, launches the publisher, and blocks return
until the swap is visible on disk) but asserting a different, orthogonal
property: not what the consumer ends up holding, but that the publisher's
log shows it entered wait_for_readers because it found a live reader.
The assertion is deliberately narrow — only the readers: waiting for 1 in-flight clone(s) of snapshot-<key> line, not drained after or the
reclaim outcome — so the scenario reddens for reader_lock_acquire alone and
not for reader_lock_release (already pinned elsewhere) or any of the
tear-detection guards 8a-8d/10 pin.
Mutation proof
Before (unmutated, baseline):seed-target-dir-selftest: 49 assertions passed (and 24 / 37 / 5 / 14 across the other four suites — 129 total).
Red — reader_lock_acquire "$parent" "$src_name" "$tag" deleted from hardlink_clone_into (scripts/cache-lib.sh:486):
=== 11: the consumer's marker actually stops the publisher ===
PASS: the consumer's clone raced a real, concurrent republish of its own source
PASS: both sides of the race completed
publish: /tmp/tmp.b4EuDV2cHB/cache/snapshot-release-5-8f440bbf (0.0 GB) published in 0s
ASSERTION FAILED: the publisher never reported waiting on the consumer's reader marker — the interlock did not observably engage
All other scenarios in seed-target-dir-selftest.sh (1 through 10, including
8a-8d) still pass under this mutation, and the other four suites
(hardlink-clone-selftest.sh, prune-cache-selftest.sh, publish-snapshot-selftest.sh, restore-mtimes-selftest.sh) are unaffected —
5 / 37 / 24 / 14 assertions, unchanged — confirming scenario 11 fails for its
own reason and the existing diagonal is undisturbed.
52 = 49 + 3 (the three new ok/PASS calls scenario 11 adds). Total across
all five suites: 132 (129 + 3), with only seed-target-dir-selftest.sh
touched.
Files affected
scripts/seed-target-dir-selftest.sh — new scenario 11, plus a header
comment entry describing it. No other file is touched; scripts/cache-lib.sh is unmodified (verified with git diff --name-only
after the mutation-and-restore cycle above).
Test plan
bash scripts/selftest.sh — all 5 suites pass, 132 assertions total (see
green output above).
Mutation-proved as documented above: the new scenario is red with reader_lock_acquire removed and green with it restored, and no other
scenario's assertion count changes under that mutation.
Not run: any interactive/live-CI-volume check — this repo's suites are
offline fixtures only, and no live host was touched.
## Summary
- Adds scenario 11 to `scripts/seed-target-dir-selftest.sh`, which pins
`reader_lock_acquire` in `hardlink_clone_into` (`scripts/cache-lib.sh:486`)
by racing a real `hardlink_clone_into` clone against a real, concurrent
`publish-snapshot.sh` republish of the exact snapshot being cloned, and
asserting the **publisher's own log** reports entering its reader-drain
wait — not merely that the run succeeded.
## Why
Fixes #10. Deleting `reader_lock_acquire` from `hardlink_clone_into` left
**all five suites green at the identical 129 assertions**: the sibling call,
`reader_lock_release`, is already pinned (removing it reddens two suites via
the drain timing out), but nothing exercised the marker's *creation*. The
consumer's marker is what `publish-snapshot.sh`'s `wait_for_readers` checks
before it reclaims a rotated-away snapshot generation
(`scripts/cache-lib.sh:493-524`, `scripts/publish-snapshot.sh`'s post-swap
scan) — without it, a publisher finishing a swap while a consumer is
mid-`cp -al` can unlink the generation being read, the silent-truncation mode
`seed-target-dir-selftest.sh`'s scenario 8b reproduces directly.
The trap the issue calls out: this line is *not* dormant. Scenario 8a already
forces the publisher to block on this consumer's marker mid-race, and its log
already contains lines like `readers: waiting for 1 in-flight clone(s) of
snapshot-dev-<key> (grace 300s)` / `readers: snapshot-dev-<key> drained after
1s` — but nothing reads them back. Remove the line and those lines vanish,
the publisher never waits, and 8a still passes with the same assertions.
## Design
The issue flags a real tension: 8a currently pins exactly one property (the
identity check) and is reddened by exactly one mutant — the suite's
one-scenario-one-mutant diagonal across scenarios 8a to 8d and 10 is
deliberate, and folding a marker assertion into 8a would cost it. Scenario 11
is a **separate** scenario, reusing 8a's technique for forcing the
interleaving deterministically (a stubbed `cp` that fires once on the
consumer's own top-level clone, launches the publisher, and blocks return
until the swap is visible on disk) but asserting a different, orthogonal
property: not what the consumer ends up holding, but that the **publisher's**
log shows it entered `wait_for_readers` because it found a live reader.
The assertion is deliberately narrow — only the `readers: waiting for 1
in-flight clone(s) of snapshot-<key>` line, not `drained after` or the
reclaim outcome — so the scenario reddens for `reader_lock_acquire` alone and
not for `reader_lock_release` (already pinned elsewhere) or any of the
tear-detection guards 8a-8d/10 pin.
## Mutation proof
**Before (unmutated, baseline):** `seed-target-dir-selftest: 49 assertions
passed` (and 24 / 37 / 5 / 14 across the other four suites — 129 total).
**Red — `reader_lock_acquire "$parent" "$src_name" "$tag"` deleted from
`hardlink_clone_into` (`scripts/cache-lib.sh:486`):**
```
=== 11: the consumer's marker actually stops the publisher ===
PASS: the consumer's clone raced a real, concurrent republish of its own source
PASS: both sides of the race completed
publish: /tmp/tmp.b4EuDV2cHB/cache/snapshot-release-5-8f440bbf (0.0 GB) published in 0s
ASSERTION FAILED: the publisher never reported waiting on the consumer's reader marker — the interlock did not observably engage
```
All other scenarios in `seed-target-dir-selftest.sh` (1 through 10, including
8a-8d) still pass under this mutation, and the other four suites
(`hardlink-clone-selftest.sh`, `prune-cache-selftest.sh`,
`publish-snapshot-selftest.sh`, `restore-mtimes-selftest.sh`) are unaffected —
5 / 37 / 24 / 14 assertions, unchanged — confirming scenario 11 fails for its
own reason and the existing diagonal is undisturbed.
**Green — line restored:**
```
seed-target-dir-selftest: 52 assertions passed
publish-snapshot-selftest: 24 assertions passed
prune-cache-selftest: 37 assertions passed
hardlink-clone-selftest: 5 assertions passed
ALL 14 ASSERTIONS PASSED
selftest: all 5 suites passed
```
52 = 49 + 3 (the three new `ok`/`PASS` calls scenario 11 adds). Total across
all five suites: 132 (129 + 3), with only `seed-target-dir-selftest.sh`
touched.
## Files affected
- `scripts/seed-target-dir-selftest.sh` — new scenario 11, plus a header
comment entry describing it. No other file is touched;
`scripts/cache-lib.sh` is unmodified (verified with `git diff --name-only`
after the mutation-and-restore cycle above).
## Test plan
- `bash scripts/selftest.sh` — all 5 suites pass, 132 assertions total (see
green output above).
- Mutation-proved as documented above: the new scenario is red with
`reader_lock_acquire` removed and green with it restored, and no other
scenario's assertion count changes under that mutation.
- Not run: any interactive/live-CI-volume check — this repo's suites are
offline fixtures only, and no live host was touched.
Adds scenario 11, which races a real hardlink_clone_into against a real,
concurrent publish-snapshot.sh republish of the exact snapshot being
cloned, and asserts the publisher's own log reports entering its
reader-drain wait. Deleting reader_lock_acquire left every suite green
at the identical 129 assertions (issue #10): the marker it writes is
what the publisher's wait_for_readers checks, and with it gone the
publisher sees zero readers on its first check and proceeds straight to
reclaiming the rotated generation, silently.
Kept as its own scenario rather than folded into 8a: 8a already pins
exactly one property (the identity check) for exactly one mutant, and
the suite's one-scenario-one-mutant diagonal across 8a to 8d and 10 is
deliberate.
claude
added the bug label 2026-08-24 22:15:36 +00:00
The test itself is well-built and I could not break it. Mutation coverage, determinism, and the AC1/AC3/AC4 properties all verify cleanly. The one blocking finding is a docs-ship-with-code gap with direct, same-file precedent in this repo's own history.
Wide pass
Right scenario, right file, right shape. Scenario 11 is additive-only (git diff confirms two hunks: a header-comment insertion and a new final block; no existing scenario's code is touched), which is the correct call given the one-scenario-one-mutant diagonal the issue explicitly protects. It reuses 8a's forced-interleaving technique (a blocking stub cp that polls stat until the swap is visible on disk) rather than inventing a new, less-proven mechanism.
Blocking finding — README.md is not updated, and this repo has direct precedent that it should be.README.md's suite-coverage table (line 425, seed-target-dir-selftest.sh row) and the "three shapes of concurrency scenarios" prose (lines 434–458, 473–481) both catalog every scenario in seed-target-dir-selftest.sh by number and by property pinned, and neither mentions scenario 11. This isn't a stale claim going false, it's an inventory doc silently becoming incomplete — but the precedent removes any ambiguity about whether that's in scope: PR #9 (test/pin-clone-guards, the ticket's own predecessor, same file, same table, same author) touched README.md for exactly this reason — 40 lines updating this exact table row and this exact prose block when it added scenarios 8b/8c/8d/10 (git diff 68da3e6 31b4113 -- README.md). #12 makes a materially similar addition (a new scenario, a new pinned property) and skips the update PR #9 made routine. Per docs-ship-with-code, this is worth fixing before merge, not deferring.
Narrow pass
Verified directly, not sampled: hardlink_clone_into (scripts/cache-lib.sh:483-524) is unchanged; reader_lock_acquire is at line 486. Shell mechanics in the new stub (heredoc escaping, subshell scoping, exit-code capture via rcPubIntlk) exactly mirror 8a's already-proven pattern — no swallowed failures, no vacuous grep target (a missing or empty logPubIntlk cannot satisfy grep -q, both paths route through explicit fail with a tail -40). One non-blocking nit: unlike 8a and the ALIAS scenario, scenario 11 has no closing find … -name '.stage-*' -o -name '.reading-*' leftover check — since it's the last scenario in the file, nothing downstream would independently catch a marker the consumer leaked. Not a real leak (the whole $scratch, which contains $root, is removed by the file's trap … EXIT regardless), just a missed opportunity to self-check the way its siblings do.
AC1 ("fails if reader_lock_acquire is removed... demonstrate red/green explicitly") — reproduced independently. Isolated scenario 11 into a standalone harness (same preamble/helpers, script_dir pinned at the worktree) so it runs without the earlier scenarios in the file — control is GREEN (3/3), removing reader_lock_acquire (line 486) turns it RED with the exact failure the PR quotes: the publisher never reported waiting on the consumer's reader marker.
AC2 ("fails for its own reason... does not disturb the 8a-8d/10 diagonal") — ran the isolated harness against five sibling mutations (methodology matches PR #9's own "scratch copy, run in isolation" precedent):
mutation removed/broken
scenario 11 (isolated)
(none, control)
GREEN
reader_lock_acquire
RED
reader_lock_release (sibling)
GREEN
i_before = i_after (torn-clone identity check)
GREEN
n_after -eq n_before (torn-clone entry count)
GREEN
unshare_mutable_paths failure path
GREEN
write_cache_lock
GREEN
Only the targeted guard reddens it. Also ran the reader_lock_release-removed mutation against the full --fast suite (not isolated): seed-target-dir-selftest.sh and publish-snapshot-selftest.sh both fail as the issue predicted — but at scenario 8a (wait_for_file "the publisher never finished", because the drain now runs to its ~300s default grace and 8a's own 60s wait_for_file times out first), before the script ever reaches scenario 11's code — confirming isolation was the right way to test AC2's "for its own reason" claim, since the un-isolated suite can't distinguish "11 stayed green" from "11 never ran." prune-cache-selftest.sh is unaffected (37/37) under that mutation. The 8a-8d/10 diagonal is untouched by construction: scenario 11 is pure append with its own cache keys (INTLK/INTLK_BASE, distinct from BASE_KEY), so it cannot change what those scenarios exercise.
AC3 ("asserts the interlock actually engaged... not that the run merely succeeded") — confirmed both by reading wait_for_readers (scripts/cache-lib.sh:372-388: the waiting for N in-flight clone(s) line prints once, on the first iteration where live_reader_count is nonzero — independent of whether the wait later drains or times out) and by the mutation table above: reader_lock_release removal leaves the line intact (marker seen at the first check) even though it changes the eventual drain outcome, which is exactly the narrowing the PR claims and the reason scenario 11 doesn't accidentally re-pin the already-pinned sibling.
AC4 ("comment states which property it pins") — confirmed, scripts/seed-target-dir-selftest.sh:650-661 states it plainly, including the same-scenario-not-folded-into-8a rationale.
Determinism
30 consecutive clean runs on a heavily contended, capacity-1 machine (20× the isolated harness, 10× the full seed-target-dir-selftest.sh), 0 flakes. The interleaving is forced by construction, not timed: the stub cp polls stat -c '%i' on the snapshot path until the swap lands (a bounded busy-wait against a real filesystem state change), the same shape already proven stable by 8a — not a fixed sleep. Checked for orphans after the runs: no stray seed-target-dir/publish-snapshot/cache-lib processes, no leftover tmp.* scratch dirs attributable to these runs (the file's trap … EXIT covers $root since it's a subdirectory of $scratch).
Other checks
Ticket cites cache-lib.sh:399 for reader_lock_acquire; current line is 486. The PR description correctly cites 486 — the drift is from PR #9 landing 249 lines in this file between the issue being filed and #12 branching, not a wrong pin.
Sibling-track boundary: gitdan-actions#11 touches README.md, cargo-cache/action.yml, scripts/cache-lock.sh — zero file overlap with #12's single-file diff. No merge conflict expected between the two.
Diff is exactly one file (scripts/seed-target-dir-selftest.sh, +87/-0); scripts/cache-lib.sh is untouched, confirmed via git diff origin/main...HEAD.
Scratch mutations in the worktree
Made and reverted, per the brief. scripts/cache-lib.sh was mutated and restored from a saved-outside-the-worktree baseline copy for each of the six mutations above; git status --short and git diff --stat in /home/daniel/src/gitdan-actions-reader-lock are both clean as of this review. No other file in the worktree was touched.
Fix: add the scenario-11 row/mention to README.md's suite table and the concurrency-shapes prose (mirroring PR #9's own diff to the same sections), then this is a clean APPROVE from me.
VERDICT: REQUEST-CHANGES
The test itself is well-built and I could not break it. Mutation coverage, determinism, and the AC1/AC3/AC4 properties all verify cleanly. The one blocking finding is a docs-ship-with-code gap with direct, same-file precedent in this repo's own history.
## Wide pass
Right scenario, right file, right shape. Scenario 11 is additive-only (git diff confirms two hunks: a header-comment insertion and a new final block; no existing scenario's code is touched), which is the correct call given the one-scenario-one-mutant diagonal the issue explicitly protects. It reuses 8a's forced-interleaving technique (a blocking stub `cp` that polls `stat` until the swap is visible on disk) rather than inventing a new, less-proven mechanism.
**Blocking finding — README.md is not updated, and this repo has direct precedent that it should be.** `README.md`'s suite-coverage table (line 425, `seed-target-dir-selftest.sh` row) and the "three shapes of concurrency scenarios" prose (lines 434–458, 473–481) both catalog every scenario in `seed-target-dir-selftest.sh` by number and by property pinned, and neither mentions scenario 11. This isn't a stale claim going false, it's an inventory doc silently becoming incomplete — but the precedent removes any ambiguity about whether that's in scope: PR #9 (`test/pin-clone-guards`, the ticket's own predecessor, same file, same table, same author) touched `README.md` for exactly this reason — 40 lines updating this exact table row and this exact prose block when it added scenarios 8b/8c/8d/10 (`git diff 68da3e6 31b4113 -- README.md`). #12 makes a materially similar addition (a new scenario, a new pinned property) and skips the update PR #9 made routine. Per `docs-ship-with-code`, this is worth fixing before merge, not deferring.
## Narrow pass
Verified directly, not sampled: `hardlink_clone_into` (`scripts/cache-lib.sh:483-524`) is unchanged; `reader_lock_acquire` is at line 486. Shell mechanics in the new stub (heredoc escaping, subshell scoping, exit-code capture via `rcPubIntlk`) exactly mirror 8a's already-proven pattern — no swallowed failures, no vacuous grep target (a missing or empty `logPubIntlk` cannot satisfy `grep -q`, both paths route through explicit `fail` with a `tail -40`). One non-blocking nit: unlike 8a and the ALIAS scenario, scenario 11 has no closing `find … -name '.stage-*' -o -name '.reading-*'` leftover check — since it's the last scenario in the file, nothing downstream would independently catch a marker the consumer leaked. Not a real leak (the whole `$scratch`, which contains `$root`, is removed by the file's `trap … EXIT` regardless), just a missed opportunity to self-check the way its siblings do.
## AC verification (source ticket #10, quoted)
- **AC1** ("fails if `reader_lock_acquire` is removed... demonstrate red/green explicitly") — reproduced independently. Isolated scenario 11 into a standalone harness (same preamble/helpers, `script_dir` pinned at the worktree) so it runs without the earlier scenarios in the file — control is GREEN (3/3), removing `reader_lock_acquire` (line 486) turns it RED with the exact failure the PR quotes: `the publisher never reported waiting on the consumer's reader marker`.
- **AC2** ("fails for its own reason... does not disturb the 8a-8d/10 diagonal") — ran the isolated harness against five sibling mutations (methodology matches PR #9's own "scratch copy, run in isolation" precedent):
| mutation removed/broken | scenario 11 (isolated) |
|---|---|
| *(none, control)* | GREEN |
| `reader_lock_acquire` | **RED** |
| `reader_lock_release` (sibling) | GREEN |
| `i_before = i_after` (torn-clone identity check) | GREEN |
| `n_after -eq n_before` (torn-clone entry count) | GREEN |
| `unshare_mutable_paths` failure path | GREEN |
| `write_cache_lock` | GREEN |
Only the targeted guard reddens it. Also ran the `reader_lock_release`-removed mutation against the full `--fast` suite (not isolated): `seed-target-dir-selftest.sh` and `publish-snapshot-selftest.sh` both fail as the issue predicted — but at **scenario 8a** (`wait_for_file "the publisher never finished"`, because the drain now runs to its ~300s default grace and 8a's own 60s wait_for_file times out first), before the script ever reaches scenario 11's code — confirming isolation was the right way to test AC2's "for its own reason" claim, since the un-isolated suite can't distinguish "11 stayed green" from "11 never ran." `prune-cache-selftest.sh` is unaffected (37/37) under that mutation. The 8a-8d/10 diagonal is untouched by construction: scenario 11 is pure append with its own cache keys (`INTLK`/`INTLK_BASE`, distinct from `BASE_KEY`), so it cannot change what those scenarios exercise.
- **AC3** ("asserts the interlock actually engaged... not that the run merely succeeded") — confirmed both by reading `wait_for_readers` (`scripts/cache-lib.sh:372-388`: the `waiting for N in-flight clone(s)` line prints once, on the first iteration where `live_reader_count` is nonzero — independent of whether the wait later drains or times out) and by the mutation table above: `reader_lock_release` removal leaves the line intact (marker seen at the first check) even though it changes the eventual drain outcome, which is exactly the narrowing the PR claims and the reason scenario 11 doesn't accidentally re-pin the already-pinned sibling.
- **AC4** ("comment states which property it pins") — confirmed, `scripts/seed-target-dir-selftest.sh:650-661` states it plainly, including the same-scenario-not-folded-into-8a rationale.
## Determinism
30 consecutive clean runs on a heavily contended, capacity-1 machine (20× the isolated harness, 10× the full `seed-target-dir-selftest.sh`), 0 flakes. The interleaving is forced by construction, not timed: the stub `cp` polls `stat -c '%i'` on the snapshot path until the swap lands (a bounded busy-wait against a real filesystem state change), the same shape already proven stable by 8a — not a fixed `sleep`. Checked for orphans after the runs: no stray `seed-target-dir`/`publish-snapshot`/`cache-lib` processes, no leftover `tmp.*` scratch dirs attributable to these runs (the file's `trap … EXIT` covers `$root` since it's a subdirectory of `$scratch`).
## Other checks
- Ticket cites `cache-lib.sh:399` for `reader_lock_acquire`; current line is 486. The PR description correctly cites 486 — the drift is from PR #9 landing 249 lines in this file between the issue being filed and #12 branching, not a wrong pin.
- Sibling-track boundary: `gitdan-actions#11` touches `README.md`, `cargo-cache/action.yml`, `scripts/cache-lock.sh` — zero file overlap with #12's single-file diff. No merge conflict expected between the two.
- Diff is exactly one file (`scripts/seed-target-dir-selftest.sh`, +87/-0); `scripts/cache-lib.sh` is untouched, confirmed via `git diff origin/main...HEAD`.
## Scratch mutations in the worktree
Made and reverted, per the brief. `scripts/cache-lib.sh` was mutated and restored from a saved-outside-the-worktree baseline copy for each of the six mutations above; `git status --short` and `git diff --stat` in `/home/daniel/src/gitdan-actions-reader-lock` are both clean as of this review. No other file in the worktree was touched.
Fix: add the scenario-11 row/mention to `README.md`'s suite table and the concurrency-shapes prose (mirroring PR #9's own diff to the same sections), then this is a clean APPROVE from me.
The suite-coverage table and the concurrency-scenario prose enumerate
every scenario by number and by the property it pins, so adding one
without listing it leaves the catalogue claiming complete coverage it
no longer has. PR #9 updated the same table for the same reason when it
added 8b/8c/8d/10; this follows it.
Also adds the closing no-scratch-left-behind check that 8a and ALIAS
end with. Scenario 11 is last in the file, so nothing downstream would
have caught a leaked staging path or reader marker.
claude-reviewer
approved these changes 2026-08-24 23:04:24 +00:00
Re-review at head aee337a (docs(readme): catalogue scenario 11 and close its scratch self-check). Both items from the prior REQUEST-CHANGES are addressed, and I re-verified rather than took the fixer's summary at face value.
The blocking finding — resolved
README.md's suite-coverage table (line 425) and concurrency-scenario prose now name scenario 11: the table row gains a clause on the publisher's-log witness, the PATH-stub paragraph gets a new sentence placed directly after its description of 8a's stub-plus-real-process technique ("Scenario 11 reuses 8a's exact stub and the same forced rotation, but reads a different witness..."), the "four times over" scenario count is correctly bumped to "five" (8a/8b/8c/10/11 all stub cp), and a new paragraph after the issue-#5 discussion extends the "assert which guard fired" discipline to scenario 11's off-clone witness.
Placement, checked specifically as asked. "That is the standing hazard here, and it is not hypothetical..." (line 488) is untouched and still immediately follows the scenario-9 paragraph it anchors to (line 478-486, "Scenario 9 does not [assert which check fired]... the assertion is satisfied down a path it was not written for"). The new scenario-11 paragraph is appended after that pair, not spliced between them — confirmed by reading the committed text, not the diff hunk headers alone. No repointed pronoun/anchor.
Accuracy, checked against the mechanism I established last round. The new prose says the assertion is "one line in the publisher's log reporting that it waited" and "a run with the marker deleted still succeeds" — it does not conflate "waited" with "drained" or "succeeded," which is exactly the distinction that keeps this scenario from re-pinning reader_lock_release (already pinned elsewhere). No loose wording that overclaims.
The nit — resolved, and I checked it isn't vacuous
The closing check (scripts/seed-target-dir-selftest.sh:723-725) matches 8a/ALIAS's pattern exactly (find "$root" -maxdepth 1 \( -name '.stage-*' -o -name '.reading-*' -o -name '.publish-*' \) -print). I didn't just confirm it exists — I gave it a positive control: removed reader_lock_release in a scratch copy of cache-lib.sh and re-ran the isolated scenario 11. It correctly failed with a real leak: .publish-old-release-5-8f440bbf-pubIntlk, .reading-target-release-5-8f440bbf-pubIntlk, .reading-snapshot-release-5-8f440bbf-jobIntlk. Not a wrong-directory false-clean; it caught an actual marker/deferred-generation leak. (Reverted immediately; scratch copy, not the worktree's cache-lib.sh.)
One nuance worth surfacing, not a defect. Under that same mutation, scenario 11 as a whole now reddens (via this new leftover check) where before my last review it stayed fully green (the pre-existing version had no leftover check). The specifically-targeted assertion — the "waiting for" grep — still passes unchanged; only the new, generically-scoped hygiene check catches it. This is not new or unique to scenario 11: 8a's own identical leftover check has always had this same secondary sensitivity to reader_lock_release (it also runs hardlink_clone_into), and PR #9's diagonal was scoped to the four torn-clone comparisons plus the unshare guard, never to reader_lock_release. Scenario 11 now matches its siblings' existing shape rather than diverging from it.
Determinism re-check (not assumed)
Rebuilt the isolated harness against the new head (line offsets shifted +3) and reran: 20/20 clean in isolation, 30/30 clean for the full seed-target-dir-selftest.sh file (53 assertions each time, matching the fixer's count), all on the same contended machine as last round. The new find-based check runs after wait_for_file on rcPubIntlk and the publisher's own exit-code check, so it observes state strictly after both sides of the race have exited — no structural race against the trap's cleanup, and the runs bear that out. No orphaned processes or temp dirs after.
gitdan-actions#11's README hunks land at lines ~115 and ~232-325 (new "Scratch names..." subsection + its "Names this repo doesn't reclaim" child section); #12's hunks are at lines ~422-502. Clear separation, confirmed via git diff main origin/chore/leftover-contract-names -- README.md. No merge-conflict risk between the two regardless of merge order.
Not re-litigated
AC1-AC4, the five counter-mutations, and the isolated-vs-un-isolated-suite finding from the prior round are unaffected by a docs-only change plus one appended self-check that doesn't touch the pinned assertion — confirmed by re-running the reader_lock_acquire-removed positive control on the new head: identical red, same message, same location.
Worktree
scripts/cache-lib.sh was mutated and reverted twice this round (the leftover-check positive control, the acquire-removed sanity check), each from a saved-outside-the-worktree copy. git status --short and git diff --stat in /home/daniel/src/gitdan-actions-reader-lock are clean as of this review. No other file touched.
VERDICT: APPROVE
Re-review at head aee337a (`docs(readme): catalogue scenario 11 and close its scratch self-check`). Both items from the prior REQUEST-CHANGES are addressed, and I re-verified rather than took the fixer's summary at face value.
## The blocking finding — resolved
README.md's suite-coverage table (line 425) and concurrency-scenario prose now name scenario 11: the table row gains a clause on the publisher's-log witness, the PATH-stub paragraph gets a new sentence placed directly after its description of 8a's stub-plus-real-process technique ("Scenario 11 reuses 8a's exact stub and the same forced rotation, but reads a different witness..."), the "four times over" scenario count is correctly bumped to "five" (8a/8b/8c/10/11 all stub `cp`), and a new paragraph after the issue-#5 discussion extends the "assert which guard fired" discipline to scenario 11's off-clone witness.
**Placement, checked specifically as asked.** "That is the standing hazard here, and it is not hypothetical..." (line 488) is untouched and still immediately follows the scenario-9 paragraph it anchors to (line 478-486, "Scenario 9 does not [assert which check fired]... the assertion is satisfied down a path it was not written for"). The new scenario-11 paragraph is appended *after* that pair, not spliced between them — confirmed by reading the committed text, not the diff hunk headers alone. No repointed pronoun/anchor.
**Accuracy, checked against the mechanism I established last round.** The new prose says the assertion is "one line in the publisher's log reporting that it waited" and "a run with the marker deleted still succeeds" — it does not conflate "waited" with "drained" or "succeeded," which is exactly the distinction that keeps this scenario from re-pinning `reader_lock_release` (already pinned elsewhere). No loose wording that overclaims.
## The nit — resolved, and I checked it isn't vacuous
The closing check (`scripts/seed-target-dir-selftest.sh:723-725`) matches 8a/ALIAS's pattern exactly (`find "$root" -maxdepth 1 \( -name '.stage-*' -o -name '.reading-*' -o -name '.publish-*' \) -print`). I didn't just confirm it exists — I gave it a positive control: removed `reader_lock_release` in a scratch copy of `cache-lib.sh` and re-ran the isolated scenario 11. It correctly failed with a real leak: `.publish-old-release-5-8f440bbf-pubIntlk`, `.reading-target-release-5-8f440bbf-pubIntlk`, `.reading-snapshot-release-5-8f440bbf-jobIntlk`. Not a wrong-directory false-clean; it caught an actual marker/deferred-generation leak. (Reverted immediately; scratch copy, not the worktree's `cache-lib.sh`.)
**One nuance worth surfacing, not a defect.** Under that same mutation, scenario 11 as a *whole* now reddens (via this new leftover check) where before my last review it stayed fully green (the pre-existing version had no leftover check). The specifically-targeted assertion — the "waiting for" grep — still passes unchanged; only the new, generically-scoped hygiene check catches it. This is not new or unique to scenario 11: 8a's own identical leftover check has always had this same secondary sensitivity to `reader_lock_release` (it also runs `hardlink_clone_into`), and PR #9's diagonal was scoped to the four torn-clone comparisons plus the unshare guard, never to `reader_lock_release`. Scenario 11 now matches its siblings' existing shape rather than diverging from it.
## Determinism re-check (not assumed)
Rebuilt the isolated harness against the new head (line offsets shifted +3) and reran: **20/20 clean** in isolation, **30/30 clean** for the full `seed-target-dir-selftest.sh` file (53 assertions each time, matching the fixer's count), all on the same contended machine as last round. The new `find`-based check runs after `wait_for_file` on `rcPubIntlk` and the publisher's own exit-code check, so it observes state strictly after both sides of the race have exited — no structural race against the trap's cleanup, and the runs bear that out. No orphaned processes or temp dirs after.
## Sibling-track boundary (#11)
`gitdan-actions#11`'s README hunks land at lines ~115 and ~232-325 (new "Scratch names..." subsection + its "Names this repo doesn't reclaim" child section); #12's hunks are at lines ~422-502. Clear separation, confirmed via `git diff main origin/chore/leftover-contract-names -- README.md`. No merge-conflict risk between the two regardless of merge order.
## Not re-litigated
AC1-AC4, the five counter-mutations, and the isolated-vs-un-isolated-suite finding from the prior round are unaffected by a docs-only change plus one appended self-check that doesn't touch the pinned assertion — confirmed by re-running the `reader_lock_acquire`-removed positive control on the new head: identical red, same message, same location.
## Worktree
`scripts/cache-lib.sh` was mutated and reverted twice this round (the leftover-check positive control, the acquire-removed sanity check), each from a saved-outside-the-worktree copy. `git status --short` and `git diff --stat` in `/home/daniel/src/gitdan-actions-reader-lock` are clean as of this review. No other file touched.
claude
changed title from WIP: test(seed): pin reader_lock_acquire in hardlink_clone_into to test(seed): pin reader_lock_acquire in hardlink_clone_into (#12)2026-08-24 23:05:16 +00:00
claude
merged commit 1aca90b461 into main2026-08-24 23:05:18 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
claude-reviewer
Summary
scripts/seed-target-dir-selftest.sh, which pinsreader_lock_acquireinhardlink_clone_into(scripts/cache-lib.sh:486)by racing a real
hardlink_clone_intoclone against a real, concurrentpublish-snapshot.shrepublish of the exact snapshot being cloned, andasserting the publisher's own log reports entering its reader-drain
wait — not merely that the run succeeded.
Why
Fixes #10. Deleting
reader_lock_acquirefromhardlink_clone_intoleftall five suites green at the identical 129 assertions: the sibling call,
reader_lock_release, is already pinned (removing it reddens two suites viathe drain timing out), but nothing exercised the marker's creation. The
consumer's marker is what
publish-snapshot.sh'swait_for_readerschecksbefore it reclaims a rotated-away snapshot generation
(
scripts/cache-lib.sh:493-524,scripts/publish-snapshot.sh's post-swapscan) — without it, a publisher finishing a swap while a consumer is
mid-
cp -alcan unlink the generation being read, the silent-truncation modeseed-target-dir-selftest.sh's scenario 8b reproduces directly.The trap the issue calls out: this line is not dormant. Scenario 8a already
forces the publisher to block on this consumer's marker mid-race, and its log
already contains lines like
readers: waiting for 1 in-flight clone(s) of snapshot-dev-<key> (grace 300s)/readers: snapshot-dev-<key> drained after 1s— but nothing reads them back. Remove the line and those lines vanish,the publisher never waits, and 8a still passes with the same assertions.
Design
The issue flags a real tension: 8a currently pins exactly one property (the
identity check) and is reddened by exactly one mutant — the suite's
one-scenario-one-mutant diagonal across scenarios 8a to 8d and 10 is
deliberate, and folding a marker assertion into 8a would cost it. Scenario 11
is a separate scenario, reusing 8a's technique for forcing the
interleaving deterministically (a stubbed
cpthat fires once on theconsumer's own top-level clone, launches the publisher, and blocks return
until the swap is visible on disk) but asserting a different, orthogonal
property: not what the consumer ends up holding, but that the publisher's
log shows it entered
wait_for_readersbecause it found a live reader.The assertion is deliberately narrow — only the
readers: waiting for 1 in-flight clone(s) of snapshot-<key>line, notdrained afteror thereclaim outcome — so the scenario reddens for
reader_lock_acquirealone andnot for
reader_lock_release(already pinned elsewhere) or any of thetear-detection guards 8a-8d/10 pin.
Mutation proof
Before (unmutated, baseline):
seed-target-dir-selftest: 49 assertions passed(and 24 / 37 / 5 / 14 across the other four suites — 129 total).Red —
reader_lock_acquire "$parent" "$src_name" "$tag"deleted fromhardlink_clone_into(scripts/cache-lib.sh:486):All other scenarios in
seed-target-dir-selftest.sh(1 through 10, including8a-8d) still pass under this mutation, and the other four suites
(
hardlink-clone-selftest.sh,prune-cache-selftest.sh,publish-snapshot-selftest.sh,restore-mtimes-selftest.sh) are unaffected —5 / 37 / 24 / 14 assertions, unchanged — confirming scenario 11 fails for its
own reason and the existing diagonal is undisturbed.
Green — line restored:
52 = 49 + 3 (the three new
ok/PASScalls scenario 11 adds). Total acrossall five suites: 132 (129 + 3), with only
seed-target-dir-selftest.shtouched.
Files affected
scripts/seed-target-dir-selftest.sh— new scenario 11, plus a headercomment entry describing it. No other file is touched;
scripts/cache-lib.shis unmodified (verified withgit diff --name-onlyafter the mutation-and-restore cycle above).
Test plan
bash scripts/selftest.sh— all 5 suites pass, 132 assertions total (seegreen output above).
reader_lock_acquireremoved and green with it restored, and no otherscenario's assertion count changes under that mutation.
offline fixtures only, and no live host was touched.
VERDICT: REQUEST-CHANGES
The test itself is well-built and I could not break it. Mutation coverage, determinism, and the AC1/AC3/AC4 properties all verify cleanly. The one blocking finding is a docs-ship-with-code gap with direct, same-file precedent in this repo's own history.
Wide pass
Right scenario, right file, right shape. Scenario 11 is additive-only (git diff confirms two hunks: a header-comment insertion and a new final block; no existing scenario's code is touched), which is the correct call given the one-scenario-one-mutant diagonal the issue explicitly protects. It reuses 8a's forced-interleaving technique (a blocking stub
cpthat pollsstatuntil the swap is visible on disk) rather than inventing a new, less-proven mechanism.Blocking finding — README.md is not updated, and this repo has direct precedent that it should be.
README.md's suite-coverage table (line 425,seed-target-dir-selftest.shrow) and the "three shapes of concurrency scenarios" prose (lines 434–458, 473–481) both catalog every scenario inseed-target-dir-selftest.shby number and by property pinned, and neither mentions scenario 11. This isn't a stale claim going false, it's an inventory doc silently becoming incomplete — but the precedent removes any ambiguity about whether that's in scope: PR #9 (test/pin-clone-guards, the ticket's own predecessor, same file, same table, same author) touchedREADME.mdfor exactly this reason — 40 lines updating this exact table row and this exact prose block when it added scenarios 8b/8c/8d/10 (git diff 68da3e6 31b4113 -- README.md). #12 makes a materially similar addition (a new scenario, a new pinned property) and skips the update PR #9 made routine. Perdocs-ship-with-code, this is worth fixing before merge, not deferring.Narrow pass
Verified directly, not sampled:
hardlink_clone_into(scripts/cache-lib.sh:483-524) is unchanged;reader_lock_acquireis at line 486. Shell mechanics in the new stub (heredoc escaping, subshell scoping, exit-code capture viarcPubIntlk) exactly mirror 8a's already-proven pattern — no swallowed failures, no vacuous grep target (a missing or emptylogPubIntlkcannot satisfygrep -q, both paths route through explicitfailwith atail -40). One non-blocking nit: unlike 8a and the ALIAS scenario, scenario 11 has no closingfind … -name '.stage-*' -o -name '.reading-*'leftover check — since it's the last scenario in the file, nothing downstream would independently catch a marker the consumer leaked. Not a real leak (the whole$scratch, which contains$root, is removed by the file'strap … EXITregardless), just a missed opportunity to self-check the way its siblings do.AC verification (source ticket #10, quoted)
AC1 ("fails if
reader_lock_acquireis removed... demonstrate red/green explicitly") — reproduced independently. Isolated scenario 11 into a standalone harness (same preamble/helpers,script_dirpinned at the worktree) so it runs without the earlier scenarios in the file — control is GREEN (3/3), removingreader_lock_acquire(line 486) turns it RED with the exact failure the PR quotes:the publisher never reported waiting on the consumer's reader marker.AC2 ("fails for its own reason... does not disturb the 8a-8d/10 diagonal") — ran the isolated harness against five sibling mutations (methodology matches PR #9's own "scratch copy, run in isolation" precedent):
reader_lock_acquirereader_lock_release(sibling)i_before = i_after(torn-clone identity check)n_after -eq n_before(torn-clone entry count)unshare_mutable_pathsfailure pathwrite_cache_lockOnly the targeted guard reddens it. Also ran the
reader_lock_release-removed mutation against the full--fastsuite (not isolated):seed-target-dir-selftest.shandpublish-snapshot-selftest.shboth fail as the issue predicted — but at scenario 8a (wait_for_file "the publisher never finished", because the drain now runs to its ~300s default grace and 8a's own 60s wait_for_file times out first), before the script ever reaches scenario 11's code — confirming isolation was the right way to test AC2's "for its own reason" claim, since the un-isolated suite can't distinguish "11 stayed green" from "11 never ran."prune-cache-selftest.shis unaffected (37/37) under that mutation. The 8a-8d/10 diagonal is untouched by construction: scenario 11 is pure append with its own cache keys (INTLK/INTLK_BASE, distinct fromBASE_KEY), so it cannot change what those scenarios exercise.AC3 ("asserts the interlock actually engaged... not that the run merely succeeded") — confirmed both by reading
wait_for_readers(scripts/cache-lib.sh:372-388: thewaiting for N in-flight clone(s)line prints once, on the first iteration wherelive_reader_countis nonzero — independent of whether the wait later drains or times out) and by the mutation table above:reader_lock_releaseremoval leaves the line intact (marker seen at the first check) even though it changes the eventual drain outcome, which is exactly the narrowing the PR claims and the reason scenario 11 doesn't accidentally re-pin the already-pinned sibling.AC4 ("comment states which property it pins") — confirmed,
scripts/seed-target-dir-selftest.sh:650-661states it plainly, including the same-scenario-not-folded-into-8a rationale.Determinism
30 consecutive clean runs on a heavily contended, capacity-1 machine (20× the isolated harness, 10× the full
seed-target-dir-selftest.sh), 0 flakes. The interleaving is forced by construction, not timed: the stubcppollsstat -c '%i'on the snapshot path until the swap lands (a bounded busy-wait against a real filesystem state change), the same shape already proven stable by 8a — not a fixedsleep. Checked for orphans after the runs: no strayseed-target-dir/publish-snapshot/cache-libprocesses, no leftovertmp.*scratch dirs attributable to these runs (the file'strap … EXITcovers$rootsince it's a subdirectory of$scratch).Other checks
cache-lib.sh:399forreader_lock_acquire; current line is 486. The PR description correctly cites 486 — the drift is from PR #9 landing 249 lines in this file between the issue being filed and #12 branching, not a wrong pin.gitdan-actions#11touchesREADME.md,cargo-cache/action.yml,scripts/cache-lock.sh— zero file overlap with #12's single-file diff. No merge conflict expected between the two.scripts/seed-target-dir-selftest.sh, +87/-0);scripts/cache-lib.shis untouched, confirmed viagit diff origin/main...HEAD.Scratch mutations in the worktree
Made and reverted, per the brief.
scripts/cache-lib.shwas mutated and restored from a saved-outside-the-worktree baseline copy for each of the six mutations above;git status --shortandgit diff --statin/home/daniel/src/gitdan-actions-reader-lockare both clean as of this review. No other file in the worktree was touched.Fix: add the scenario-11 row/mention to
README.md's suite table and the concurrency-shapes prose (mirroring PR #9's own diff to the same sections), then this is a clean APPROVE from me.VERDICT: APPROVE
Re-review at head
aee337a(docs(readme): catalogue scenario 11 and close its scratch self-check). Both items from the prior REQUEST-CHANGES are addressed, and I re-verified rather than took the fixer's summary at face value.The blocking finding — resolved
README.md's suite-coverage table (line 425) and concurrency-scenario prose now name scenario 11: the table row gains a clause on the publisher's-log witness, the PATH-stub paragraph gets a new sentence placed directly after its description of 8a's stub-plus-real-process technique ("Scenario 11 reuses 8a's exact stub and the same forced rotation, but reads a different witness..."), the "four times over" scenario count is correctly bumped to "five" (8a/8b/8c/10/11 all stub
cp), and a new paragraph after the issue-#5 discussion extends the "assert which guard fired" discipline to scenario 11's off-clone witness.Placement, checked specifically as asked. "That is the standing hazard here, and it is not hypothetical..." (line 488) is untouched and still immediately follows the scenario-9 paragraph it anchors to (line 478-486, "Scenario 9 does not [assert which check fired]... the assertion is satisfied down a path it was not written for"). The new scenario-11 paragraph is appended after that pair, not spliced between them — confirmed by reading the committed text, not the diff hunk headers alone. No repointed pronoun/anchor.
Accuracy, checked against the mechanism I established last round. The new prose says the assertion is "one line in the publisher's log reporting that it waited" and "a run with the marker deleted still succeeds" — it does not conflate "waited" with "drained" or "succeeded," which is exactly the distinction that keeps this scenario from re-pinning
reader_lock_release(already pinned elsewhere). No loose wording that overclaims.The nit — resolved, and I checked it isn't vacuous
The closing check (
scripts/seed-target-dir-selftest.sh:723-725) matches 8a/ALIAS's pattern exactly (find "$root" -maxdepth 1 \( -name '.stage-*' -o -name '.reading-*' -o -name '.publish-*' \) -print). I didn't just confirm it exists — I gave it a positive control: removedreader_lock_releasein a scratch copy ofcache-lib.shand re-ran the isolated scenario 11. It correctly failed with a real leak:.publish-old-release-5-8f440bbf-pubIntlk,.reading-target-release-5-8f440bbf-pubIntlk,.reading-snapshot-release-5-8f440bbf-jobIntlk. Not a wrong-directory false-clean; it caught an actual marker/deferred-generation leak. (Reverted immediately; scratch copy, not the worktree'scache-lib.sh.)One nuance worth surfacing, not a defect. Under that same mutation, scenario 11 as a whole now reddens (via this new leftover check) where before my last review it stayed fully green (the pre-existing version had no leftover check). The specifically-targeted assertion — the "waiting for" grep — still passes unchanged; only the new, generically-scoped hygiene check catches it. This is not new or unique to scenario 11: 8a's own identical leftover check has always had this same secondary sensitivity to
reader_lock_release(it also runshardlink_clone_into), and PR #9's diagonal was scoped to the four torn-clone comparisons plus the unshare guard, never toreader_lock_release. Scenario 11 now matches its siblings' existing shape rather than diverging from it.Determinism re-check (not assumed)
Rebuilt the isolated harness against the new head (line offsets shifted +3) and reran: 20/20 clean in isolation, 30/30 clean for the full
seed-target-dir-selftest.shfile (53 assertions each time, matching the fixer's count), all on the same contended machine as last round. The newfind-based check runs afterwait_for_fileonrcPubIntlkand the publisher's own exit-code check, so it observes state strictly after both sides of the race have exited — no structural race against the trap's cleanup, and the runs bear that out. No orphaned processes or temp dirs after.Sibling-track boundary (#11)
gitdan-actions#11's README hunks land at lines ~115 and ~232-325 (new "Scratch names..." subsection + its "Names this repo doesn't reclaim" child section); #12's hunks are at lines ~422-502. Clear separation, confirmed viagit diff main origin/chore/leftover-contract-names -- README.md. No merge-conflict risk between the two regardless of merge order.Not re-litigated
AC1-AC4, the five counter-mutations, and the isolated-vs-un-isolated-suite finding from the prior round are unaffected by a docs-only change plus one appended self-check that doesn't touch the pinned assertion — confirmed by re-running the
reader_lock_acquire-removed positive control on the new head: identical red, same message, same location.Worktree
scripts/cache-lib.shwas mutated and reverted twice this round (the leftover-check positive control, the acquire-removed sanity check), each from a saved-outside-the-worktree copy.git status --shortandgit diff --statin/home/daniel/src/gitdan-actions-reader-lockare clean as of this review. No other file touched.WIP: test(seed): pin reader_lock_acquire in hardlink_clone_intoto test(seed): pin reader_lock_acquire in hardlink_clone_into (#12)