test(seed): force scenario 8's interleaving instead of racing for it #4
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: daniel/gitdan-actions#4
Reference in New Issue
Block a user
Closes #3.
Summary
scripts/seed-target-dir-selftest.shscenario 8 no longer races for its interleaving. It is replaced by 8a and 8b, which each place the interference deterministically and assert which ofhardlink_clone_into's checks caught it.seed_with_stub,wait_for_file, andassert_tear(parses the clone's own torn-read report and asserts the shape of it).cache-lib.sh,seed-target-dir.shandpublish-snapshot.share unchanged — the defect was in the test's premise, not in the code under test.Why
Scenario 8 started a real publisher once the consumer's clone was observed past a fifth of the tree, then asserted the consumer ended up holding the new generation. That assertion encodes "the rotation must have interfered" as though it were guaranteed. It is not: when the publish lands after the consumer's last identity read — which it can, because
publish-snapshot.shstages its own clone before it renames anything — the consumer legitimately completes a whole, consistent generation 1, and the test called thatthe seeded tree is truncated.The design's actual guarantee is the weaker and more useful one: whatever generation the consumer ends up with is internally consistent.
scripts/selftest.shis this repo's only gate, and a flaky assertion in it that fails in the most alarming direction available trains everyone to re-run — which is how a real failure gets waved through.Which shape, and why not the other
The ticket offered two: assert internal consistency of whichever generation the consumer ends up with, or force the interleaving deterministically the way
prune-cache-selftest.shscenario 12 does. Forcing it, because it turned out to be forceable — and once it is, the strong assertion is legitimate rather than hopeful, so there is no reason to settle for the weaker one.The consume side's window is precisely delimited in
hardlink_clone_into:Everything the scenario wants to happen has to happen strictly inside it. Stubbing
cpputs the interference inside the consumer's own call, which makes the placement a fact rather than a scheduling outcome — the same trick scenario 12 uses when it makes the prune pass's owndupublish the marker.Rejecting option 1 is not free: "internal consistency of whichever generation" is a genuinely weaker property, and asserting it would have left the suite green whether or not the consumer ever noticed the substitution. That is the property the flaky version was reaching for and the one worth keeping.
What each scenario pins
8a — a rotation landing inside the identity window. The stub runs the real copy first, then starts the real
publish-snapshot.shconcurrently and returns only once the swap is on disk. Concurrently is load-bearing: the publisher's post-swap drain wait is against this consumer's marker, which is held until the identity read that follows thecp— running the publish inline would deadlock the two sides against each other.Because the copy runs before the rotation, it succeeds and the staging tree holds every entry the source had.
cp rc=0,737/737 entries— so neither the copy's status nor the entry count is a witness. The source's identity is the only one. The consumer re-clones and ends up with the whole generation now published (188 entries,gen2fingerprint), and nothing is left renamed aside.8b — a subtree unlinked out from under the walk, silently. The other tear mode, and the one with no error to report: a subtree that leaves the parent's listing before
cp -alreads it is simply never visited. The stub renames one out before the walk starts and back before the retry — back, because the real thing that removes entries, a publish rotating a generation away, has a whole generation at the path by the time a retry looks.cp rc=0,310/371 entries, source inode unchanged. The entry count is the only witness — this is the mode that used to rename a partial tree into place and report success.Both scenarios assert that their stub actually fired. A fixture that quietly failed to interfere would otherwise pass for the same reason the old one intermittently failed.
Red/green: mutation results
Each check removed in turn from a scratch copy of
cache-lib.sh(andpublish-snapshot.sh), full suites run against it, then restored:[ "$i_before" = "$i_after" ]removedno torn read was reported at all; the log showsseed: cloned in 0sagainst generation 1[ "$n_after" -eq "$n_before" ]removedwait_for_readersdrain removedthe previous generation was unlinked while a reader still held itThe first two rows are the point: neither scenario stays green when the check it isolates is deleted, and neither is masked by the other. For contrast, on
mainthe old scenario 8 caught neither of those mutations — 3/3 green for each. The split is a strict coverage gain, not a strong claim traded for a weak one.The third row is the publisher's half of the same race, and it is
publish-snapshot-selftest.shscenario 6's job by design — both file headers already say so. Reported here because the brief asked for it, not because scenario 8 should have caught it.One mutation neither scenario catches (pre-existing)
Removing the
[ "$cp_rc" -eq 0 ]term leaves the whole suite green — including scenario 9. 8a and 8b both force silent tears by construction, so the copy's own status is deliberately not their witness, and nothing else isolates it. Not a regression — the same mutation leaves all three fixture suites green onmaintoo — and outside #3's Ask, so it is left alone rather than folded in and is being tracked separately.Stability
bash scripts/selftest.sh, all 5 suites, idlebash scripts/seed-target-dir-selftest.shunder 24-way CPU saturationThe reported entry counts are byte-identical across every run (
737/737for 8a,310/371for 8b), which is the visible sign that the interleaving is fixed rather than merely lucky — the old scenario'scaught mid-walk at N/48805line moved by thousands of entries run to run.Files affected
scripts/seed-target-dir-selftest.shonly — scenario 8 replaced by 8a/8b, three helpers added, the header's scenario list and themake_wide_tree/tree_entriescomments rewritten to say what the fixture is now for.shellcheckis clean apart from the repo-wideSC1091on thecache-lib.shsource line.README.md— the Development section's methodology paragraph, rewritten twice. It stated the repo's rule for writing concurrency scenarios as "gate the interfering step on observed progress of the step it interferes with, so the window is hit deterministically", which described the progress poll this branch removes and claims the very property #3 records as false. Left standing it would have told the next contributor to build the next scenario the way this one had to be rewritten. Replaced with what the suites now do — stub a command the code under test calls at a known point, assert the stub fired, assert which guard caught the fault — and the seed suite's table row now names both tear modes.scripts/publish-snapshot-selftest.sh— one cross-reference, now pointing at 8a and 8b. (publish-snapshot.sh:32's similar mislabel predates this branch and is left alone.)Test plan
bash scripts/selftest.sh— all 5 suites pass (19s), run 20×.shellcheck scripts/seed-target-dir-selftest.sh— baselineSC1091only.mktemp -d.shellcheckclean, and 8b verified to run standalone with 8a excised and no fix-ups — the setup it used to borrow from 8a now lives next toseed_with_stub.shellcheckclean on both touched scripts.Review round 2
The methodology paragraph asserted three things as fact that hold for some scenarios and not others. Rewritten to census what the suites actually do:
PATHstub that places the interference inside the window (prune 12, seed 8a/8b); a synthetic stand-in where the artefact is itself the contract (publish 6-8). 8a's second process is called out as incidental to the determinism — the stub is what fixes where the swap lands..reading-*marker is the whole agreement between the two sides, and racing a real slow consumer would make the suite's runtime the thing under test — now sits beside the other two.No code changed this round; the mutation matrix and stability evidence above still describe the shipped scenarios.
Scenario 8 started a real publisher once a consumer's clone was observed past a fraction of the tree, then asserted the consumer ended up holding the NEW generation. That premise is racy: when the publish lands after the consumer's last identity read, the consumer legitimately completes a whole, consistent generation 1 — and the assertion called that "the seeded tree is truncated". The one gate this repo has was failing intermittently in the most alarming direction available. Replaced with two scenarios that place the interference deterministically, the way prune-cache-selftest.sh scenario 12 places a marker inside the check-to-unlink window: the consumer's own `cp` is stubbed, so whatever the stub does happens strictly after hardlink_clone_into read the source's entry count and inode and strictly before it reads that inode again. 8a the stub runs the real copy, then the real publish-snapshot.sh concurrently, and returns only once the swap is on disk. The copy succeeds and the staging tree is whole, so the source's identity is the only witness. 8b the stub renames a subtree out of the source's listing before the walk starts and back before the retry. The copy exits 0 and the source's identity never changes, so the entry count is the only witness — the silent truncation this whole guard exists for. Each asserts WHICH of the clone's checks reported the tear (assert_tear), so neither stays green if the check it exercises is deleted and another happens to fire in its place. Closes #3VERDICT: REQUEST-CHANGES
The substance of this PR is right, and it is verified rather than asserted — I reproduced every load-bearing claim independently, including the two that matter most (distinct mutation sensitivity, and 20-run stability under load). The single blocking finding is documentation: the diff falsifies
README.md's statement of how this repo's concurrency scenarios work, and the PR body affirmatively claims it does not.Blocking: the diff leaves
README.mdlying about its own methodologyREADME.md:370-374:That paragraph described the mechanism this PR deletes — the
threshold=$(( gen1_entries / 5 ))progress poll that scenario 8 used to gate its publisher on. After this diff, no scenario anywhere inscripts/gates an interfering step on observed progress; I grepped for it (progress/threshold/ poll loops across all five suites) and the only remaining interference mechanisms are the two PATH stubs,prune-cache-selftest.sh:238and the newseed_with_stub. The first clause fares no better: 8a stubscp, and 8b races nothing at all — it is a synchronous rename by the harness.This is not wording drift. The falsified sentence is the repo's stated methodology for writing concurrency scenarios, and the property it claims — determinism via observed-progress gating — is exactly the one issue #3 exists to record as false. Left as is, it instructs the next contributor to build the next scenario the way this one had to be rewritten. Per
docs-ship-with-code, a behavior change that leaves a doc lying is a request-changes finding rather than a nit.The PR body's "No documentation falsified: ... README.md does not name scenarios" is true but answers the wrong question — the README does not name scenario numbers, but it does describe the mechanism. A reviewer trusting that line would skip the check.
Secondary, same fix round:
publish-snapshot-selftest.sh:44still reads "is in seed-target-dir-selftest.sh scenario 8", which no longer exists under that name. (publish-snapshot.sh:32's "this script's own selftest (scenario 8)" is separately confusable —publish-snapshot-selftest.sh's own scenario 8 is the abandoned-marker one — but that mislabel predates this PR and is not yours to fix here.)Everything else verified
AC 1 — deterministic, 20 consecutive runs including under CPU load. Confirmed independently, not taken from the report:
bash scripts/selftest.sh, all 5 suites, idlebash scripts/seed-target-dir-selftest.shunder 24-wayyessaturationThe torn-read reports were byte-identical across all 20 loaded runs —
cp rc=0, 737/737 entriesfor 8a andcp rc=0, 310/371 entriesfor 8b, every time. That invariance under saturation is the real evidence the interleaving is forced rather than lucky, and it is a much stronger signal than the pass count alone.AC 2 — red/green, and that each scenario fails for its own reason. Mutated one guard at a time in
hardlink_clone_into, in a scratch copy, running from committed state:[ "$i_before" = "$i_after" ]removedseed: cloned in 0s[ "$n_after" -eq "$n_before" ]removedSo they pin genuinely distinct properties, in both directions. Both also have redundant downstream coverage — under mutation A, 8a's
rot_entries -eq snap_entrieswould have caught it hadassert_tearnot fired first (737 cloned against a 188-entry snapshot), and likewise for 8b's final count.Worth measuring against what was there before: on
main, the old scenario 8 caught neither mutation, 3/3 runs green for each. The split is a strict coverage gain, not a weakening — the strong claim it used to make was one it did not reliably enforce.The
cp_rcgap. Confirmed, and confirmed pre-existing: removing[ "$cp_rc" -eq 0 ]leaves all three fixture suites green (seed 30, publish 24, prune 37) — and equally green onmain, 3/3. One correction to the PR body: its parenthetical "the old scenario 8 had it in reverse: it caught cp_rc" did not reproduce for me. The conclusion it supports (not a regression) holds regardless. A follow-up issue for the uncovered guard would be worth filing; it is correctly out of #3's scope.Does the stub actually fire, and does it fail loudly if not? Both guards are real, not decorative — I neutered each stub's destination match so it could never fire, and each scenario failed on its own assertion (
the stubbed cp never fired: ... so this scenario proves nothing) rather than degrading to a silent pass. That is the failure mode being fixed, and it is closed. The stub also cannot be picked up by the wrongcp: of the three call sites incache-lib.sh, only line 403's last argument is the staging path, and the one-shot flag file guards the retry and the publisher's own staging clone independently.wait_for_file's 60s deadline. Not a new flake source in practice, and it degrades loudly. On timeout it callsfailwith a named reason. Two notes for the record: the publisher's ownwait_for_readersgrace isCACHE_READ_GRACE_SECONDS=300, five times the poll's deadline — but the consumer's retry releases its marker within a second or two of the swap, so the publisher exits far inside 60s (observed under saturation, 20/20). And the stub's internal 60s inode-change poll exits 90 on timeout, which surfaces asassert_tear's "expected cp to exit 0" rather than as the stub's own stderr line — loud, but a step less direct than it could be. Neither warrants a change.Scope.
git diff --name-only ba7a44c..dc0fef7is exactly one file.seed-target-dir.sh,cache-lib.sh,publish-snapshot.sh,cargo-cache/andcargo-cache-publish/are untouched, so no action input contract moved and nov2tag question arises.shellcheck scripts/seed-target-dir-selftest.shis identical tomain: one pre-existing SC1091 on thecache-lib.shsource line.Is +203 the right size? Yes. 96 of the 203 added lines are comment or blank, so the code delta is ~107 lines replacing 58.
assert_tearaccounts for most of it and is the component that makes the mutation-distinctness above possible — without it, either scenario would stay green on the other's guard. The ticket's cheaper option 1 would have been ~10 lines and, by the mutation table, would have caught neither mutant. The machinery is load-bearing.AC 3 — comments state the pinned property. Yes, at the header (8a/8b entries), at each scenario, and on all three helpers.
Nit, non-blocking
8b depends on
mkdir -p "$scratch/bin"andreal_cp=$(command -v cp), both defined inside 8a's block. I tripped over this isolating 8b for the mutation matrix. Hoisting them next toseed_with_stubwould make the two scenarios independently movable.VERDICT: APPROVE-WITH-NITS
Re-review of
5551e99against my priorREQUEST_CHANGES(review 526).The blocking finding is resolved, and more thoroughly than it needed to be. I asked for a paragraph that stopped describing a deleted mechanism. What landed also records why that mechanism was wrong and files the lesson as a rule for the next scenario. Preserving the negative result — "seeing that a walk has started says nothing about where it will be when the interference lands" — is worth more than the correction itself; that is the sentence that stops #3 from being rediscovered.
Re-verified, all still holding at
5551e99Mutation matrix, re-run against a scratch copy of
cache-lib.sh, one guard at a time — and this time with each scenario run in genuine isolation, which the hoist now makes possible:[ "$i_before" = "$i_after" ]removed[ "$n_after" -eq "$n_before" ]removedStub-fires guards: neutering each stub's destination match so it can never fire still produces
the stubbed cp never fired: ... so this scenario proves nothingin both scenarios, rather than a silent pass.Stability at the new head: 20/20 consecutive seed-suite runs under 24-way
yessaturation, plus 6/6 fullselftest.sh. All 40 torn-read reports across the two rounds are byte-identical —cp rc=0, 737/737andcp rc=0, 310/371.The hoist is clean, and the claim in its comment is exactly true. I ran 8a with 8b excised and 8b with 8a excised, adding nothing back in either direction; both pass standalone. It cannot disturb the earlier scenarios either: nothing in the suite scans
$scratch(every leftover assertion is scoped to$root),$scratch/binreachesPATHonly insideseed_with_stub, and no stub file exists there until 8a or 8b writes one.publish-snapshot-selftest.sh:44now reads "scenarios 8a and 8b".shellcheckon both touched files is SC1091 only. The PR body'scp_rcclaim now matches what I measured, and#5records the gap accurately, including that it reproduces onmain.Nits — none blocking
1. The new paragraph's opening clause is broader than the suites it generalizes over. "The concurrency scenarios run the real scripts as real concurrent processes" is true of exactly one of them. A census after this diff:
seed7seed8apublish-snapshot.shcpplaces itseed8bcp, synchronous renameprune12du, synchronous marker writepublish6/7/8The paragraph self-corrects two clauses later — "the consumer's own clone is what rotates the snapshot underneath it" and "the pass's own measurement publishes a reader marker" both say plainly that these are single-process — so a reader who finishes the paragraph is not misled. But a reader who stops at the topic sentence takes away "spawn real concurrent processes", which is the instinct that produced #3. Dropping three words ("as real concurrent processes") would close it.
2. A third mechanism is in use and unmentioned.
publish-snapshot-selftest.shscenarios 6-8 hold a synthetic.reading-*marker rather than stubbing anything, and scenario 6's own comment defends that choice on grounds the README's opening now contradicts: "Racing a real slow consumer would make the suite's runtime the thing under test; the marker IS the entire contract between the two sides, so holding one is being a reader." That is a good reason and belongs in the methodology paragraph alongside the stubbing one — right now the README implies the publish suite does something it does not.3. The "assert which guard fired" rule is a target in one place, not yet a description. It reads as present-tense fact, but
seed-target-dir-selftest.shscenario 9 does not do it: it assertsrc -eq 2and the log message without pinning which of the three checks produced the failure, which is precisely why#5's mutation survives it. Since the exception is already tracked, a trailing clause — "#5tracks the one scenario where this does not hold yet" — would keep the rule honest as written. This is the same shape as the sentence it replaced, at much lower intensity: following this rule leads to the right test, whereas following the old one reproduced the bug. That is why it is a nit and not a repeat of the blocking finding.4. Out of scope, agreed, but worth an issue.
publish-snapshot.sh:32's "the failure mode this script's own selftest (scenario 8) reproduces" is doubly confusable now: this script's own selftest ispublish-snapshot-selftest.sh, whose scenario 8 is the abandoned-marker one, while the truncated clone it means isseed-target-dir-selftest.sh8b. Pre-existing, correctly left alone here.Approving. Nits 1-3 are one paragraph's worth of tightening in the same file this PR already touches, so they are cheap to fold in if wanted, but none of them gate the merge.