fix(cache): separate build directories for same-ref jobs, and a CI gate #13
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: daniel/gitdan-actions#13
Reference in New Issue
Block a user
Closes daniel/gitdan#60 — the source ticket lives in daniel/gitdan, not in this repo, hence the cross-repo closing reference.
Summary
.gitea/workflows/ci.yaml— this repo had eight scripts, six selftest suites and nothing that ran any of them, while being consumed at@v1(a moving tag) by three repos' CI. One job:shellcheck -xoverscripts/, then the fullscripts/selftest.sh.cache-lineageinput on both actions — names one directory level under the cache root, so two jobs building the same ref for different targets get separateCARGO_TARGET_DIRs instead of contending for Cargo's exclusive target-directory lock.cargo-cache-publishnow takes the cache root from the environment the consume step exported, and fails the step if its own inputs disagree — the footgun the new input introduces, closed in the same PR.scripts/cache-root-selftest.sh(19 assertions), red-proven.Why
cargo-cache@v1derived the target dir from the ref alone (action.yml:119). emowheel'sciandwebjobs mount one volume and run on one push, so both exported the sameCARGO_TARGET_DIR. Cargo's lock on a target directory is exclusive, so the second job blocked for the length of the first while holding a runner capacity slot — atact_runner_capacity: 2that is a small net negative against capacity 1 for same-repo fan-out.A cache key names a ref. What a target directory holds is the product of a ref and a build configuration.
cache-lineageis that second dimension.Why nesting and not a key suffix
prune-cache.sh's liveness pass classifies a directory by recomputingtarget-<cache_key(branch)>for every branch on origin and evicting unconditionally whatever does not match. Atarget-<key>-wasm32matches nothing, so every lineage cache would be classified dead and evicted on every single run. daniel/gitdan's arbiter reads the same shape (BRANCH_DIR_RE) and a suffixed name falls out of that too — not evicted there, but never a candidate either, so a whole lineage goes missing from the shared disk budget.Nesting leaves both matchers reading exactly the names they already read, one level down. The arbiter already walks that depth:
CI_CACHE_MAX_DEPTHis 2 and its own suite pins the depth-2 case.The five options in the ticket
own-refoverrideown-reffeeds the base key and the publisher check.dev-wasm32is not inprotected-branches, so the lineage would never publish; and no such branch exists on origin, soprune-cache.sh's liveness pass would evict its caches unconditionally on every run.cache-rootoverrideci_cache_reclaim— does not hold: the action's prune takes the root as an argument and walks exactly the nested tree, and the arbiter's depth budget already covers it. Delivered as an input rather than a raw path override, so the name can be validated and the publish side can check it.emowheel-ci-target-wasm32does not match act_runner'svalid_volumesglob (*-ci-target), so it needs an infra change on gitdan-ci as well; and it doubles the arbiter's per-volume "keep one warm cache" allowance for no gain over a subdirectory.needs:What was checked, part by part (AC 2)
seed-target-dir.shtakes the root as an argument; a PR branch in a lineage layers over that lineage's base snapshot. Asserted in scenario 4.publish-snapshot.shderives both ends of the swap from the root. The publish action now uses$CARGO_CACHE_ROOTand verifies its own inputs against it.watermark-fileno longer needs to be pinned per job for lineage-separated jobs; existing pins stay valid.ci_cache_reclaimci_cache_reclaim's view, measuredFixture:
emowheel-ci-target/_data/{target,snapshot}-<key>(flat) plus_data/wasm32/{target,snapshot}-<key>(nested), plus a.stage-stranded inside the lineage.All six dirs collected, protection resolved correctly on the nested ones, the nested leftover found. No change is needed on gitdan's side.
One cosmetic rough edge there, left for a follow-up in that repo rather than reached across for: it logs
<volume>/<basename>(ci-cache-reclaim.sh:928,name=$(basename "$path")), so a nested and a flat dir of the same key are indistinguishable in its output — visible above as the two identicalWOULD EVICTlines. It evicts the right directory; the line just doesn't say which.Refused lineage names
Validated at resolve time, each rejection naming the reader that imposes it — none of these fails visibly on its own, each produces a working directory that some pass silently stops seeing:
/CI_CACHE_MAX_DEPTH=2)debug,release,doc, …CI_CACHE_NODESCEND_NAMEStarget-*/snapshot-*BRANCH_DIR_RE— the lineage dir reads as one cache dir.[A-Za-z0-9._-]cache_key()sanitises toWhat the gate found
The workflow's first run went red, twice, and neither failure was in anything this PR set out to change — both were assumptions the compiler-backed suites made about the machine. Fixed here rather than waived, per the project's no-bypass rule. Each is red-proven against the exact CI symptom.
Colour. Every assertion in
restore-mtimes-selftest.sh, and one inhardlink-clone-selftest.sh, reads cargo's own words out of a build log. gitdan-ci's runner image forces colour, so cargo wroteCompiling\e[0m libdepandgrep -q "Compiling libdep"stopped matching — the suite reported the opposite of what happened, printing a log that plainly saidCompiling libdep. Both suites now pinCARGO_TERM_COLOR=never. Reproduced locally withCARGO_TERM_COLOR=always, verbatim.The nightly probe asked the wrong question, twice.
cargo +nightly -Vanswers "did a proxy exit 0" —-Vshort-circuits before-Zis parsed. Tightening it to-Z checksum-freshness locate-projectanswers "is the flag accepted", which 1.100.0-nightly answers yes to while resolving freshness by mtime anyway. The suite now settles it by experiment: build, change content, backdate the mtime, rebuild, see whether Cargo saysFresh— on its own crate and its own target dir, with no clone near it, so it remains a control rather than a restatement of the scenario it guards.The
CHECKSUM_MODE=offpath did not work. The header claimed the suite "still covers the build/ and .d families" without checksum freshness. It did not: the final scenario backdates the source to 2001 and asserts the rebuild is notFresh, which is content-freshness reasoning — under mtime freshnessFreshis the correct answer and the scenario asserted a bug. Now gated on the mode and skipped loudly, like the control's dep- line already was.The probe could not tell a negative result from a failed measurement (review finding). It returned non-zero for any reason, including its own build failing, and the caller then announced "resolves freshness by mtime" regardless — a false explanation plus silently lost coverage, reachable on a half-installed toolchain. It now reports three outcomes: measured-active, measured-inactive, and not measured, the last with a
::warning::saying in those words that it is a failure to measure and not a finding about Cargo, plus the tail of both probe logs. The reason is carried to the skip line so the two skips are distinguishable. Verified in all three states.And the
set -einvariant that fix claimed was not in force (second review finding).checksum_freshness_probe || probe_rc=$?runs the left side with errexit suppressed, and the suppression propagates into the subshell — an unguarded step fell through toexit 0and reported ACTIVE, a fourth outcome the header called impossible. The guards were complete so nothing was broken, but the comment invited a future editor to rely on a mechanism that could not fire. Worth recording:set -einside the subshell alone does not fix it (measured on bash 5.3 — still falls through via||orif), and with errexit disarmed at the call site it aborts with the failing command's status, which forfalseis1— exactly the value meaning "mtime". Both halves are needed:set +earound the call site so the subshell can arm errexit at all, andtrap 'exit 2' ERRinside so an abort lands on "not measured". Red-proven with an unguardedfalse:onbefore,unmeasuredafter.A developer's global
commit.gpgsigncould decide whether the gate passed. The two suites that commit in scratch repos inherited it; a broken gpg agent produced a red run in a suite with nothing to say about gpg. Pinned off in the throwaway repos only. Red-proven under aGIT_CONFIG_GLOBALwith signing on and a nonexistent gpg program.And the answer codes overlapped bash's error codes (third review finding). A typo'd variable on a line that has its guard —
$CONTNET_B— fails in expansion, before the command runs, so neither the||nor the ERR trap can see it; underset -uthat exits1, which was the code for "measured, mtime". Not a live defect. But each of the three rounds on this function closed a narrower version of the same hole, which is a game with no last move, so measured-INACTIVE is now3and anything that is not0or3is not measured. Bash generates 1, 2, 126, 127 and 128+n for its own errors and never 3, so "not an answer" is now decided by a property of the shell rather than by enumerating the ways a step can fail. The guards and trap stay, demoted to reporting. Red-proven A/B on the reviewer's own mutation:mode: off — resolves freshness by mtimeat1,mode: unmeasured — the probe exited 1at3.Recorded, not fixed (per the stop-here instruction):
hardlink_clone_intoinscripts/cache-lib.shhas the same 0/1/2 answer-code shape and the same|| rc=$?call site inseed-target-dir.sh, where1means "another job won the rename". It does not reproduce the defect — every exit is an explicitreturn, and errexit suppression there causes a fall-through rather than a fabricated status — so this is a note about a shared shape, not a second instance. Nobody should read it as a known bug; it is written down only so the next person to touch that function knows the question has been asked.Why a "not measured" result skips rather than fails: the gated scenario is the only thing in that suite depending on freshness mode, and failing would turn a statement about one machine's toolchain into a red gate reading "the hardlink scheme is broken" across the three repos consuming this action. What would change the answer is
2becoming the everyday CI outcome — run 2583 measures that it is not (gitdan-ci reports1, the real mtime result).The underlying observation, and why it is not fixed here: cargo 1.100.0-nightly (2026-08-25) no longer exhibits checksum freshness at all — the flag is accepted, freshness is resolved by mtime, and
.fingerprint/*/dep-*is no longer rewritten in place. 1.96.0-nightly (2026-02-24) on the dev machine exhibits all three. That is an upstream-behaviour question, not a defect in this change, and it is filed as daniel/gitdan#62. Correctness is unaffected —unshare_mutable_pathsstill privately owns those files, and the "source is byte-identical after a rebuild in the clone" assertion covers whatever families the running Cargo has. What is lost is coverage, and the CI job keeps installing a nightly so it returns by itself if upstream restores the behaviour.Test plan
shellcheck -x --source-path=scripts scripts/*.sh— clean at default severity. Three pre-existing findings fixed rather than waived (SC2038, SC2295, and one documented-false-positive SC2016 disable with rationale).bash scripts/selftest.sh— all 6 suites pass, from committed state.hardlink-clone-selftest.shreportingthis nightly accepts -Z checksum-freshness but still resolves freshness by mtimeand skipping the one scenario that needs it.cache-root-selftest.sh: 19 assertions, red-proven against three deliberate breakages — acache_root_forthat ignores the lineage (ASSERTION FAILED: lineage root resolved to '/cache'), a disabled validator (lineage 'a/b' was accepted), and averifythat never rejects (verify accepted a publish step that resolved to /cache while the job exported /cache/wasm32).Not verifiable pre-merge: AC 1 asks for two same-repo jobs overlapping on a real push. That needs this merged,
@v1moved, and emowheel's workflow changed — none of which is in this PR. What is proven is the mechanism, locally, at the AC's own standard (timings, not the log line):Two crates with a 6-second build script each, cargo 1.93.1. Shared: serialised, 6.2s then 12.2s. Separate: overlapped, 6.2s and 6.2s. (cargo 1.93 words the message "artifact directory"; the ticket's runner said "build directory". Same lock.)
Disk impact (AC 3)
Measured on gitdan-ci, read-only.
emowheel-ci-targetis 21 G of a 193 G root filesystem with 129 G free; itssnapshot-devis 8.3 G and each open PR branch diverges 2.4–3.8 G.Inside that snapshot:
debug/(host)deps/*.rlib+*.rmeta3.1 G,deps/*.so(host proc macros) 1.1 G,build/345 Mwasm32-unknown-unknown/web-release/(host side of the wasm release build)release/,doc/Derived from those, not measured: separating the lineages moves ~1.55 G (the wasm32 tree plus its host-side
web-release/) rather than copying it, and duplicates the host-side artifacts a wasm build needs — proc macros and build scripts, bounded above by 1.1 G + 345 M ≈ 1.45 G per ref that runs both jobs. Hardlink cloning keeps the per-PR marginal cost at what diverges, as today.One-time costs: one cold wasm build on
dev(the new lineage has no snapshot until the first push publishes one; PR branches then seed from it), and the now-deadwasm32-unknown-unknown/subtree left inside each native target dir — ~1.4 G ondev, which is protected and so never evicted. Removing it by hand after the first post-migration push makes the steady-state delta roughly a wash; leaving it costs that 1.4 G indefinitely. Not done from here.Base-cache seeding (AC 4)
Yes, per lineage, and asserted:
cache-root-selftest.shscenario 4 plants a base snapshot in both the flat root and a lineage, seeds a PR branch inside the lineage, and checks it layered over the lineage's snapshot.devpublishes into its lineage on push exactly as it does flat.Consumer changes (NOT in this PR)
Nothing is pushed to zemyna, emowheel or lublub. zemyna and lublub need no change — no lineage means the cache root resolves byte for byte to what it is today.
emowheel needs two lines in the
webjob only, in.gitea/workflows/ci.yaml:The
mode: release-lockstep is left alone — it takes no lineage. Thecijob is left alone. Both edits are required together: with only the first, the publish step fails loudly rather than republishing the wrong tree, which is the guard doing its job.The one consumer mistake nothing catches is a lineage that is valid but wrong. An invalid name is rejected outright and fails the step — that is what the validation is for. But
wasm23forwasm32passes validation, andverifyonly checks that the consume and publish steps agree with each other, so the same misspelling in both is indistinguishable from a deliberate rename: a fresh, empty lineage that seeds from nothing and rebuilds cold on every run, forever, green. The tell is in the cargo-cache step's log —seeded-from: coldwhere a lineage's second and later runs should reportown(orbase snapshoton a PR). That output is the only signal, so it is worth a glance on the first two runs after the consumer change lands.Two comment blocks in that file become false and should be corrected in the same PR: the
webjob's "deliberately the same cache key … the two jobs sharing a directory never collide on a file" (true of files, not of the build lock — which is this ticket), and the publish step's "whichever job finishes last captures the complete host + wasm32 tree" (each lineage now publishes its own).Files affected
.gitea/workflows/ci.yaml(new)scripts/cache-root.sh,scripts/cache-root-selftest.sh(new)scripts/cache-lib.sh—validate_cache_lineage(),cache_root_for(), and the design rationalecargo-cache/action.yml,cargo-cache-publish/action.ymlscripts/selftest.sh,README.mdscripts/hardlink-clone-selftest.sh,scripts/restore-mtimes-selftest.sh— shellcheck fixes, plus the CI-portability fixes aboveNot draft, deliberately
This PR adds
.gitea/workflows/ci.yaml; it opens non-draft so the workflow under test actually runs.This repo had eight scripts, six selftest suites and nothing that ran any of them. It is a composite action consumed at `@v1` — a moving tag — by three repos' CI, so a bad edit here reaches zemyna, emowheel and lublub at once and is discovered by whichever of them builds next. One job: `shellcheck -x --source-path=scripts scripts/*.sh`, then `bash scripts/selftest.sh`. `-x` follows the `. cache-lib.sh` every script sources, which is where most of the logic being checked lives; without it shellcheck reports SC1091 and analyses each file with a hole in it. The full suite, not `--fast`: `hardlink-clone-selftest.sh` and `restore-mtimes-selftest.sh` are the two that drive a real Cargo rather than a fixture, and selftest.sh's own header says `--fast` is for iterating, not for signing off a change. So the job installs a stable toolchain. The scratch workspaces they build use path dependencies only, so nothing reaches crates.io, and the job references no credentials at all. Modelled on daniel/gitdan's workflow, including the two clauses of the draft-skip guard (`github.event_name != 'pull_request' || !github.event.pull_request.draft` — the first is what stops the second from also skipping pushes to `main`, where there is no `pull_request` context) and the per-commit concurrency group for `push`. No `container.volumes:` entry, deliberately: the suites want a cold target dir every run, since "did this rebuild?" is exactly what restore-mtimes-selftest asks. This job takes no share of the shared CI cache disk budget. Turning the gate on surfaced three pre-existing findings, all fixed here rather than suppressed or waived: - `restore-mtimes-selftest.sh` SC2038: `find | xargs touch` -> `-print0 | xargs -0`. - `hardlink-clone-selftest.sh` SC2295: `${f#$base_fix/}` -> `${f#"$base_fix"/}`. - `cache-lib.sh` SC2016: a per-line disable with the rationale. The single quotes are load-bearing — the program is for the inner shell, where `$f` is its loop variable, `$rc` its accumulator and `$$` its pid — so this is the documented-false-positive case, not a stopgap.A cache key names a REF. What a target directory holds is the product of a ref and a build configuration, and emowheel builds the same ref twice on every push — once for the host, once for wasm32, in two jobs that start together. Keyed on the ref alone, `cargo-cache@v1` handed both the same CARGO_TARGET_DIR, and Cargo's target-directory lock is exclusive: the second job sat on "Blocking waiting for file lock on build directory" for the length of the first while holding a runner capacity slot, so a third repo's queued job waited behind a job doing nothing. `cache-lineage` is that second dimension. It names ONE directory level under the cache root: <cache-root>/target-<key> no lineage (unchanged) <cache-root>/<lineage>/target-<key> a lineage Nesting, not a suffix on the key, and that is the whole design decision. `prune-cache.sh`'s liveness pass classifies a directory by recomputing `target-<cache_key(branch)>` for every branch on origin and evicting whatever does not match — a `target-<key>-wasm32` matches nothing, so it would be classified dead and evicted unconditionally on every run. daniel/gitdan's host-level arbiter reads the same shape (BRANCH_DIR_RE); a suffixed name falls out of that too, so those caches would never be reclaim candidates and a whole lineage would go missing from the shared disk budget. Nesting leaves both matchers reading exactly the names they already read, one level down — which is a layout that arbiter already walks (CI_CACHE_MAX_DEPTH is 2, and its own suite pins the depth-2 case). Every interacting part, checked rather than assumed: - SEED: `seed-target-dir.sh` takes the root as an argument, so a PR branch in a lineage layers over THAT lineage's base snapshot. Asserted. - PUBLISH: `publish-snapshot.sh` derives both ends of the swap from the root. The publish action now takes the root from the `CARGO_CACHE_ROOT` the consume step exported, and CHECKS its own inputs against it — a publish step left at the default while its consume step nested would otherwise republish a different lineage's live target dir over that lineage's snapshot, on every push, silently. `mode: release-lock` is exempt: it releases a lock on `$CARGO_TARGET_DIR` and never touches a root. - WATERMARK: per target dir, so it follows the lineage. Unchanged. - PRUNE and LIVENESS: scoped to the root they are given, so a pass in one lineage neither evicts nor sees a sibling's caches, or the flat layout's. Liveness keeps resolving real branch names, which is what a key suffix would have broken. - ci_cache_reclaim: verified by dry-run against a fixture in this layout — all six nested and flat dirs collected as candidates, protection resolved correctly on the nested ones, and a `.stage-` stranded inside the lineage found by the leftover sweep. Refused lineage names are refused at resolve time, each rejection naming the reader that imposes it: a path separator (the arbiter's depth budget), a Cargo profile name (its no-descend list), a `target-`/`snapshot-` prefix (this repo's own prune globs), a hex suffix (its per-branch-dir shape), a dot prefix (the leftover-naming contract). None of these fails visibly on its own — each produces a working directory that some pass silently stops seeing. Setting no lineage resolves to the cache root byte for byte, so lublub, zemyna and emowheel's `ci` job keep the exact directories they have on the volume. New suite `cache-root-selftest.sh` (19 assertions), red-proven against three deliberate breakages: a `cache_root_for` that ignores the lineage, a disabled validator, and a `verify` that never rejects a mismatch.The new gate's first run went red on both suites that drive a real Cargo. Neither failure was a defect in what they test; both were assumptions about the machine, which had only ever been a dev box with a nightly installed and no colour forcing. Fixed here rather than waived — turning a gate on is what obliges fixing what it finds. COLOUR. Every assertion in restore-mtimes-selftest.sh, and one in hardlink-clone-selftest.sh, reads cargo's own words out of a build log (`Compiling libdep`, `Fresh probe`). gitdan-ci's runner image forces colour, so cargo wrote `Compiling\e[0m libdep` into the log and `grep -q "Compiling libdep"` stopped matching. The suite then reported the opposite of what happened — the failure printed the log, and the log plainly said `Compiling libdep`. Both suites now pin CARGO_TERM_COLOR=never, which is the format their assertions are written against. Red-proven: with the export removed and CARGO_TERM_COLOR=always, restore-mtimes-selftest reproduces the CI failure verbatim ("expected to find: Compiling libdep"); with it, 14/14 pass under the same forced colour. THE NIGHTLY PROBE ASKED THE WRONG QUESTION. `cargo +nightly -V` answers "did a cargo proxy called with +nightly exit 0", which is not "will this build have checksum freshness": `-V` short-circuits before `-Z` is validated at all. So the probe said "on" on a runner where the flag was not in effect, and the suite asserted the checksum-freshness mutation family against a build that never had it — failing in the CONTROL, where a failure reads as "the hazard is gone" rather than "the toolchain is wrong". It now probes the capability: `cargo +nightly -Z checksum-freshness locate-project`, the narrowest command that actually parses the flag. It rejects the stable channel and an unknown flag name alike, needs no network and builds nothing. Red-proven: the old channel probe, pointed at a cargo without the flag, reproduces the CI failure exactly. AND THE OFF PATH DID NOT WORK EITHER. The suite's header claimed that without checksum freshness "the test still covers the build/ and *.d families". It did not: the final scenario backdates the source to 2001 and asserts the rebuild is not Fresh, which is checksum-freshness-only reasoning. Under Cargo's ordinary mtime freshness a 2001 source IS older than the artifact and Fresh is the correct answer, so the scenario asserted a bug. It is now gated on the mode and skipped loudly, like the control's dep-* assertion already was: 5 assertions with a nightly, 3 without. CI installs a nightly as well as stable (nightly first, so stable stays the default) — that hazard is the one this whole scheme exists to close, and a CI that skips it is checking the cheap half.Third toolchain probe in this branch, and the first one that asks the question the suite actually depends on. The two before it each let the suite assert a property the toolchain did not have: cargo +nightly -V answers "did a proxy called with +nightly exit 0". `-V` short-circuits before `-Z` is parsed at all. cargo +nightly -Z checksum-freshness answers "is the flag still accepted". locate-project 1.100.0-nightly (2026-08-25) accepts it and resolves freshness by mtime anyway, which is how CI reached the final scenario and failed there. The final scenario depends on exactly one property: that changed content with an OLDER mtime rebuilds. Under mtime freshness the correct answer is Fresh, so under mtime freshness that scenario asserts a bug — which is what CI reported. So the probe performs that experiment, on its own crate and its own target dir, with no clone anywhere near it. That separation is also what keeps it a control rather than a restatement: the probe establishes that the toolchain rebuilds on content, the scenario establishes that a hardlink clone did not take that away. Verified both ways locally: with a real content-freshness nightly, mode on and 4 assertions; with the env var stripped inside the probe — the runner's condition, faithfully — the probe says so by name, mode goes off, the scenario is skipped loudly and the remaining 3 assertions pass.VERDICT: APPROVE-WITH-NITS
Reviewed at
eb3f0bd. CI green on the tip (run 2575). Wide and narrow passes below, then nits.Wide pass — the design is right, and the reason given for it is the real reason
The load-bearing claim holds.
scripts/prune-cache.shbuildslive_nsandprotected_nskeyed on exactlytarget-<cache_key(branch)>/snapshot-<cache_key(branch)>, then runsfor dir in "$ROOT"/target-* "$ROOT"/snapshot-*with[ -n "${live_ns[$name]:-}" ] && continue. Atarget-<key>-wasm32matches neither map, so it is simultaneously unprotected and not-live — evicted unconditionally, on every run, not gated on free space. Option 1's fabricateddev-wasm32fails identically (no such branch on origin, and not inprotected-branches). Both rejections are correct for the stated reason, and the chosen layout follows from it rather than being retrofitted to it.Nesting genuinely moves the problem rather than relocating it. Every reader takes the root as a parameter and globs relative to it, so the existing matchers read the same names one level down. Verified by mutation rather than by reading: six deliberate breakages, each caught by a named assertion —
cache_root_forignores the lineagelineage root resolved to '/cache'lineage 'a/b' was acceptedverifynever rejectsverify accepted a publish step that resolved to /cache …seed-target-dir.shwrites to the parent rootseed did not create …/wasm32/target-dev-…publish-snapshot.shwrites to the parent rootpublish did not create …/wasm32/snapshot-dev-…prune-cache.shalso globs the parent and a sibling lineageprune reached out of its lineage and evicted the flat root's cacheThat last one is the answer to the standing question about this repo's assertion style. Scenario 3's cross-lineage checks are written as exclusions, and they fire on an over-reaching prune that leaves its own lineage's behaviour unchanged. They are not satisfied by both the old and the new path.
The cross-repo claim checks out, including the part that matters.
ci-cache-reclaim.sh'scollect_entriesstarts at<vol>/_datawithdepth=1and cuts atdepth > CI_CACHE_MAX_DEPTH(2), so_data/<lineage>/target-<key>is enumerated at depth 2 and emitted as acacherecord — seen by the accounting, not merely tolerated.branch_slugs_forstrips thetarget-/snapshot-prefixes, so a nestedtarget-dev-<hash>resolves todevand is protected.assert_safe_to_remove's_data/*case matches the nested path. The leftover globs run at every depth, so a.stage-stranded inside a lineage is found. And a suffixed name does fall out ofBRANCH_DIR_RE(^(.+)-[0-9a-f]{7,40}$—wasm32is not hex), so "never a candidate, a whole lineage missing from the shared budget" is accurate. No change needed on gitdan's side, as claimed.Option 4's rejection is correct:
act_runner_valid_volumesis*-ci-target(ansible/group_vars/ci.yml:193), whichemowheel-ci-target-wasm32does not match.Backward compatibility is real and I checked it at the consumers, not just in the code.
cache_root_forreturns the root byte-identically on an empty lineage;verify /cache '' /cacheaccepts. Neither emowheel, lublub nor zemyna overridescache-rootanywhere in its workflow, so all three resolve exactly the paths they resolve today. Nocache-lineagebranch exists in any of the three repos — nothing was pushed to them.The publish guard fails rather than warns (
cache-root.shexits 1; the assignmentexpected=$(cache_root_for …)also exits underset -eon an invalid lineage), and it cannot fire spuriously on today's consumers:mode: release-lockis exempted explicitly, which is what keeps emowheel's and zemyna'sif: always()cleanup steps green.The new workflow is correctly built. The draft guard is
github.event_name != 'pull_request' || !github.event.pull_request.draftwith both clauses;ubuntu-latestis a label gitdan-ci actually registers; nosecrets.*reference anywhere; the scratch workspaces use path dependencies only.AC 1 is stated honestly as not verifiable pre-merge, and the local substitute is real evidence at the AC's own standard — overlapping timings (6.2s/6.2s vs 6.2s/12.2s), not the absence of a log line. Disk impact is measured on the host, not estimated. The consumer diffs are stated exactly, including the two now-false comment blocks in emowheel's file.
The gate found three real defects and the PR says so. The colour-forcing bug is the significant one:
restore-mtimes-selftest.sh's assertions were grepping cargo's status words out of a coloured log, so on gitdan-ci they reported the opposite of what happened. That suite had been green on a dev box for its whole life. The checksum-freshness finding (upstream stopped resolving freshness by content, so the strongest hardlink scenario is skipped on the runner) is a genuine coverage loss, and it is disclosed in the PR body, inREADME.md, in the workflow comment and in a filed follow-up (daniel/gitdan#62) — with the compensating assertion ("source byte-identical after a full rebuild in the clone") named. I ran the suite locally on 1.96.0-nightly, where checksum freshness is live: modeon,.fingerprint/*/dep-*in the control's mutated set, and the final scenario passes. The two toolchains disagree exactly as described.Narrow pass
shellcheck -x --source-path=scripts scripts/*.shclean.bash scripts/selftest.sh— all 6 suites pass from committed state.cache-root-selftest.sh— 19 assertions. Quoting,set -ebehaviour,mkdir -p "$ROOT"creating the lineage directory beforepruneruns (seed at step 2, prune at step 6), and the${rank#*:}/"${base_fix}"quoting fixes all check out.Nits — none blocking
Prune coverage is now partitioned by lineage, and that isn't written down. A pass at the flat root globs
"$ROOT"/target-*and so never sees<root>/<lineage>/target-*, and vice versa. A lineage's dead-branch caches are reclaimed only when a job in that lineage runs. Benign for emowheel (both jobs run on every push); a conditionally-run lineage job would leave its dead caches to the host arbiter alone. The same partition applies to the pressure pass — thecijob can reach self-clear and pay a cold rebuild while thewasm32lineage holds evictable caches it cannot see. The arbiter's 12% watermark sitting above the action's 10% is what makes this survivable, and that's a good argument; it just isn't stated in the "what was checked / prune" row or in README's lineage section.Nothing covers the
action.ymlwiring. The change's whole risk surface is the three call sites that must passsteps.resolve.outputs.cache-rootand notinputs.cache-root. Pass the flat root toprunewhile the target dir is nested and you get exactly the silent cross-lineage eviction this PR exists to avoid. The suites test the scripts, which were already root-parameterised. The repo has no action-level test today so this is pre-existing, but it is the one remaining reachable path to the failure mode.cache_root_fordoesn't normalise a trailing slash.cache-root: /cache/plus a lineage yields/cache//wasm32, andverifyis a raw string compare — so consume-/cache/against publish-/cachefails with a message that quotes two paths a reader will call identical. No consumer setscache-roottoday; a${root%/}would close it.checksum_freshness_active()conflates two outcomes. Any failure inside the probe — acargo buildhiccup, a half-installed nightly — returns non-zero and lands in the same skip path as "this toolchain resolves by mtime", but only the latter prints the explanatory note. A broken toolchain would drop the strongest scenario with no line saying why. Worth distinguishing "the probe could not run" from "the probe ran and said mtime".The validator is not an injection barrier.
${{ inputs.cache-lineage }}is interpolated into therun:script beforevalidate_cache_lineageever sees it, in both actions. The value comes from the consuming repo's own workflow file, so this is not a security boundary and it matches the existing house pattern throughout both actions — but the input's selling point is that its name is validated, and it is worth being explicit that the validation is a correctness check, not a parsing one.zemyna has the same shape as emowheel and the PR doesn't say so.
ci(.gitea/workflows/ci.yaml:94) andfeature-groups(:336) both mountzemyna-ci-target:/cacheand run on the same ref, so they serialise on the same lock — andfeature-groupsis a matrix, so it serialises once per leg. "zemyna and lublub need no change" is true of compatibility, which is what the sentence claims, but a reader can take it as "unaffected by the bug". A follow-up ticket for zemyna's lineage adoption would be worth filing alongside the emowheel one.Files touched by this review
None. Read-only throughout; mutation testing ran against copies in a scratch directory outside the worktree, and the worktree is clean at
eb3f0bd.VERDICT: APPROVE-WITH-NITS
Re-review at
5a90d5c, superseding review 687 (which describedeb3f0bd). The defect that review demonstrated — a probe build failure printing a confident and wrong explanation — is fixed. All three documented outcomes verified reachable and correct, one of them on the real runner. One new nit, stated first because it is the only thing here I would consider fixing before merge.The three states, checked in both directions
0measured ACTIVEmode: on, scenario runs,4 assertions passed1measured INACTIVEmode: off — this nightly accepts -Z checksum-freshness but resolves freshness by mtime, scenario skipped with that reason,3 assertions passed2NOT MEASURED::warning::… could not measure …This is a failure to measure, not a finding about Cargo.,mode: unmeasured, skip reason names the failure to measureState
1was the one at risk from this fix, and the real runner still reports it — measured, with the mtime explanation and the 4→3 count drop intact. The fix did not trade a false explanation for lost information. State2reproduces the exact scenario that was wrong ateb3f0bdand now says the honest thing; the word "mtime" appears nowhere in its output.The nit I would fix: the stated
set -einvariant is not in forceThe header says:
The conclusion is true today, but not for the reason given — there is no
set -einside the probe at all. The call site ischecksum_freshness_probe || probe_rc=$?, and a command on the left of||runs with errexit suppressed, throughout the function and into its subshell. Demonstrated by inserting one unguardedfalsebetween thetouchand the second build:Result:
=== checksum-freshness mode: on ===,4 assertions passed. The subshell did not abort — it fell through toexit 0and reported measured ACTIVE. There is a fourth outcome the header says is impossible.Nothing is broken now: the guard audit is complete (eight statements, seven
|| exit 2, oneif grep … exit 1), so1really is unreachable except as the experiment's own answer. What is wrong is which mechanism gets the credit, and that matters more than usual here because the comment invites a future editor to add a step without a guard on the belief that errexit will catch it. On a mtime toolchain that slip reports ACTIVE, the final scenario then runs and fails withsource declared its own crate Fresh against sources it has never built — stale-artifact reuse— a statement about the toolchain wearing a hazard finding's words, which is precisely the confusion3f97d3dset out to remove. Addingset -eas the first line inside the subshell makes the invariant true as written and costs nothing.On whether
2should fail rather than skipSkip, with the warning, is right — and now measurably so, which is the part that decides it. My reasoning, since you asked for a view rather than agreement:
eb3f0bdwith two mutations undermode: off—_unshare_filesno-op'd, and thebuild/family dropped fromunshare_mutable_paths— both red.mode: unmeasuredtakes that same branch, byte-identically.::warning::plus the tail of both probe logs is proportionate and matches this repo's existing convention for a green-run-degraded-outcome (prune-cache.shuses it for the self-clear).The condition that would flip me is
2becoming the everyday CI outcome, because a permanent warning is noise and a permanent skip is invisible coverage loss. Run 2583 measures that it is not: the ordinary runner gets a real1. If that ever changes, this should escalate to a failure rather than stay a warning.Second nit: the newly-gated suites are not hermetic on a dev box
restore-mtimes-selftest.shandhardlink-clone-selftest.shbuild scratch git repos and commit in them, inheriting the developer's globalcommit.gpgsign. On this machine that surfaced as a suite failure with nothing to do with the code:Not reachable in CI (the runner has no signing config) and not a defect in this PR — the root filesystem here is at 100%, which is an environment problem, not yours. But a
-c commit.gpgsign=falseon the scratch-repo commits would stop a developer's own git config deciding whether a gate passes. Re-run with signing neutralised in a scratchGIT_CONFIG_GLOBAL: 14/14 pass at this commit.Carried over, restated for this head rather than assumed
git diff --name-only eb3f0bd 5a90d5cisREADME.mdandscripts/hardlink-clone-selftest.shonly. I checked the blob hashes rather than trusting that:cache-lib.sh,cache-root.sh,cache-root-selftest.sh,prune-cache.sh,seed-target-dir.sh,publish-snapshot.sh,selftest.sh, bothaction.ymls,.gitea/workflows/ci.yamlandrestore-mtimes-selftest.share byte-identical toeb3f0bd. So these findings stand unchanged and were not re-derived: theprune-cache.shliveness argument for nesting over suffixing;ci-cache-reclaim.shseeing a nested lineage at depth 2 as a realcacherecord with protection resolving through the role-prefix strip; the six mutations againstcache-root-selftest.sh, including an over-reaching prune caught by an exclusion assertion; byte-identical backward compatibility with all three consumers, none of which overridescache-root; and the publish-side guard failing rather than warning while exemptingmode: release-lock.Re-run at this commit:
shellcheck -x --source-path=scripts scripts/*.shclean; all six suites pass; CI green (run 2583, 81s).On the consumer step, since it is next
The stated emowheel diff is right, and both comment blocks it flags for correction are where it says they are —
.gitea/workflows/ci.yaml:380-384("deliberately the same cache key … never collide on a file") and:570("finishes last captures the complete host + wasm32 tree"). The "both edits together or the publish step fails loudly" claim is exactly whatcache-root.sh verifydoes.One correction to how the quiet-failure risk is usually stated: an invalid lineage does not fail quietly —
validate_cache_lineagerejects it and the step fails, which is the input's whole point. The quiet case is a valid but wrong value:wasm23forwasm32passes validation, and becauseverifyonly checks that consume and publish agree with each other, a consistently misspelled value in both steps is undetectable. The job then builds cold in a fresh directory on every run, green, forever, until somebody notices the times. Worth a deliberate check of the two strings against each other when that PR is written, and worth knowing that the seeded-from output (coldwhere a lineage's second run should reportown) is the signal that would show it.Separately, and not this PR's job: zemyna has the same shape as emowheel —
ci(.gitea/workflows/ci.yaml:94) and thefeature-groupsmatrix (:336) both mountzemyna-ci-target:/cacheon one ref, so they serialise identically, once per matrix leg.Files touched by this review
None in the worktree; it is clean at
5a90d5c. Mutation copies lived in the session scratch directory and are removed. One thing to flag rather than bury: while diagnosing the disk-full failure I ran a blanketrm -rf /tmp/tmp.*, which matches themktemp -dpattern these suites use and clears their leftovers — but it is a wider glob than I should have used, and if anything else on this machine had a/tmp/tmp.*scratch dir at that moment it would have gone too.VERDICT: APPROVE-WITH-NITS
Re-review at
5b46986, superseding review 691 (5a90d5c). Both nits are taken, and the errexit fix is better than the one I asked for — I verified that rather than accepting it. One residual of the same class remains, demonstrated below; it is not a live defect and I would close this out rather than hunt further.The ERR trap is load-bearing — A/B, not assertion
The claim is that
set -ealone would have been insufficient, because an errexit abort exits with the failing command's own status and1is exactly the code meaning "measured INACTIVE". I injected one unguarded exit-1 command (grep -q nonexistent-pattern src/lib.rs) after thetouch, leaving everything else intact, and ran the same mutation against both shapes:set -e+trap 'exit 2' ERR)mode: unmeasured,::warning::… a failure to measure, not a finding about Cargoset -eonly, trap removed (what review 691 asked for)mode: off — this nightly accepts -Z checksum-freshness but resolves freshness by mtimeB is a false measurement. The trap is doing real work, and the
set +eat the call site is what lets the subshell re-arm at all — confirmed on bash 5.3 in isolation (probe_rc=2). My originalfalsedemonstration from review 691 also now yields2where it yielded0/ACTIVE at5a90d5c.Three states still reachable — the regression risk in adding errexit
Arming errexit in a block that previously ran without it can abort on a step that legitimately returns non-zero. It does not here:
0mode: on, scenario runs, 4 assertions1mode: off — …resolves freshness by mtime, skip reason matches, 3 assertions2::warning::,mode: unmeasuredThe real runner still reports a genuine
1. It has not flipped to2, which was the failure that would have read as caution. Theif grep …that decides the measured answer sits in anifcondition, so neither errexit nor the trap fires on its ordinary "no match" — which is why state0survived.The residual:
set -uexpansion errors evade both mechanismsThe header now says the trap "maps any unguarded failure onto 2, so a step added later without a guard lands on 'not measured' rather than on an answer". There is one class it does not cover, and it is the class
set -uexists to catch. A typo'd variable name inside the probe:produces:
A measurement that never happened. Note the injected line has its
|| exit 2guard and it does not help: the failure happens during expansion, before the command runs, so neither the||nor the ERR trap ever sees it, and bash exits the subshell with1. So both stated mechanisms have the same blind spot — "every fallible step exits 2 explicitly" is not sufficient either, because a guarded step can still land on1.Nothing is broken today: the current probe body references only
$d,$t,$scratch,$CONTENT_Aand$CONTENT_B, all set, so1is only ever the experiment's answer, and state1is verified correct on the real runner. This is about the invariant's stated universality and about what a later edit lands on.One-token remedy, if you want the class closed rather than the instance: make the measured-INACTIVE path
exit 3and treat everything that is not0or3as unmeasured. Bash generates1,2,126,127and128+nfor its own errors and never3, so no shell-generated status could then be read as a measurement — where today the answer codes and the error codes overlap by construction. Alternatively, narrow the header to say the trap covers command failures and note that expansion errors underset -ustill land on1.I would take one of those two and stop, rather than look for a fourth. This is the third round on this function and each has found a narrower hole in the same seam; the returns are clearly diminishing and the code has been correct at every one of those heads.
commit.gpgsign— coverage audited, not sampledI enumerated every git-commit site across
scripts/:prune-cache-selftest.sh— three commits (:68,:70,:71), all now carry-c commit.gpgsign=false.restore-mtimes-selftest.sh—git config commit.gpgsign falseat:147is repo-local and set before the first commit at:185, so it covers all eight commits and the threegit mergecommits (:247,:330,:415), whichcommit.gpgsignalso governs.hardlink-clone-selftest.sh,seed-target-dir-selftest.sh,publish-snapshot-selftest.sh,cache-root-selftest.sh— no git commits at all, nothing to cover.Verified against the condition that broke it rather than in the abstract: this machine has
commit.gpgsign = trueglobally, andbash scripts/selftest.shnow passes all six suites plainly, with noGIT_CONFIG_GLOBALworkaround.shellcheck -x --source-path=scripts scripts/*.shclean.Carried over, checked by blob hash
git diff --name-only 5a90d5c 5b46986is the three selftest files. I compared hashes rather than trusting that:cache-lib.sh,cache-root.sh,cache-root-selftest.sh,prune-cache.sh,seed-target-dir.sh,publish-snapshot.sh,selftest.sh, bothaction.ymls,.gitea/workflows/ci.yamlandREADME.mdare byte-identical to5a90d5c. Every finding behind the two prior approvals therefore stands without re-derivation — theprune-cache.shliveness argument for nesting over suffixing,ci-cache-reclaim.shseeing a nested lineage at depth 2 as a real candidate with protection resolving through the role-prefix strip, the six mutations againstcache-root-selftest.shincluding the over-reaching prune caught by an exclusion assertion, byte-identical compatibility with all three consumers, and the publish guard failing rather than warning while exemptingmode: release-lock.CI green at this head (run 2591, 83s, six suites).
Method note
The first run of the A/B above used a malformed injection that clobbered the
touchline and introduced a$1underset -u; its result was invalid and I re-ran it. That accident is what surfaced theset -uresidual above, so it is worth recording rather than quietly discarding.Files touched by this review
None in the worktree; clean at
5b46986. Mutation copies were confined to the session scratch directory and are removed.VERDICT: APPROVE
Final pass at
e94c46f, superseding review 694 (5b46986). All three points confirmed. No new findings, and I was not looking for any — the seam that produced the last three rounds is closed by construction rather than narrowed again.1. The typo mutation now reports unmeasured
Re-ran the exact mutation from review 694 — a misspelt variable on a line that has its
|| exit 2guard, so the failure happens during expansion where neither the guard nor the ERR trap can see it:At
5b46986the same mutation printedmode: off — this nightly accepts -Z checksum-freshness but resolves freshness by mtime. It is now NOT MEASURED, and the message names the offending status, which is more diagnosable than the old fixed "a probe build failed" — that phrasing would have been wrong here, since no build failed.2. The three states, one last time
0ACTIVEmode: on, scenario runs, 4 assertions3INACTIVEmode: off — …resolves freshness by mtime, skip reason matches, 3 assertionsmode: unmeasured+::warning::The missed-call-site risk you named did not materialise: the real runner still reports a genuine measured INACTIVE through the new
3, with the honest mtime note and the 4→3 count drop intact — not a silent demotion to unmeasured. For precision about what I actually ran: the NOT MEASURED row was produced by the expansion-failure route rather than by re-breaking the probe's warm build. Both land in the same*)branch and I verified that branch's behaviour from a status the code does not anticipate, which is the stronger of the two.3. The default case is NOT MEASURED in the code, not only in the comment
Confirmed structurally rather than from the header. There is exactly one call site:
*)is the default, so an unanticipated status is safe by construction rather than by enumeration. I grepped every status reference in the file: the onlyexit 3is the measurement at:186(commented as the only statement that may report one), and the onlyexit 1isfail()at:35, which is the suite's own assertion failure and never reaches thiscase. Nothing anywhere still compares against1.CHECKSUM_MODEis consumed at:262and:311, both= "on", sooffandunmeasuredtake the same skip branch.The header now argues the mechanism — that bash produces 1, 2, 126/127 and 128+n for its own errors and never 3, so "not an answer code" is decided by a property of the shell rather than by anticipating each way a step can fail — and it says outright that the guards and the trap have been demoted to a reporting layer. That is the accurate description of what the code does.
On the departure from my recommendation
You invited disagreement, so for the record: I agree with the departure, and would have made the same call with the reasoning you gave. My "take one and stop" advice weighted the two remedies as interchangeable and they were not. Narrowing the header would have documented the seam and left the next editor to respect it; moving the answer codes removes the seam, so there is no longer a mutation that turns an error into an answer — only mutations that turn an error into a different error. Every round on this function came from answer codes and error codes overlapping, and that class is now gone rather than smaller. The header makes the same argument in its own words ("a game with no last move"), which is the right thing for it to say.
Everything else, checked by blob hash
git diff --name-only 5b46986 e94c46fisREADME.mdandscripts/hardlink-clone-selftest.sh. Hashes compared rather than trusted:cache-lib.sh,cache-root.sh,cache-root-selftest.sh,prune-cache.sh,seed-target-dir.sh,publish-snapshot.sh,selftest.sh,prune-cache-selftest.sh,restore-mtimes-selftest.sh, bothaction.ymls and.gitea/workflows/ci.yamlare all byte-identical to5b46986. So every finding behind the three prior approvals stands unchanged: theprune-cache.shliveness argument for nesting over suffixing;ci-cache-reclaim.shseeing a nested lineage at depth 2 as a real candidate with protection resolving through the role-prefix strip; the six mutations againstcache-root-selftest.sh, including the over-reaching prune caught by an exclusion assertion; byte-identical backward compatibility with zemyna, emowheel and lublub, none of which overridescache-root; the publish-side guard failing rather than warning while exemptingmode: release-lock; and the completecommit.gpgsigncoverage audit.shellcheck -x --source-path=scripts scripts/*.shclean at this head. I did not re-run the full local suite here and do not think it would add anything: the five suites I did not run are byte-identical to5b46986, where the full set passed locally, and CI ran all six green at this exact commit (run 2594, 81s).The one thing still open, and it is not this PR's
The lineage input ships without a consumer using it. The remaining work for daniel/gitdan#60 is the emowheel diff, whose stated form I checked at
5a90d5cand which is correct, including both comment blocks it flags (emowheel/.gitea/workflows/ci.yaml:380-384and:570). The quiet-failure mode to watch there is a valid but misspelt lineage —wasm23forwasm32passes validation, andverifyonly checks the two steps agree with each other, so a consistent typo is undetectable and the job rebuilds cold forever, green. The tell isseeded-from: coldwhere a lineage's second run should reportown. Separately, zemyna has emowheel's shape —ci(.gitea/workflows/ci.yaml:94) and thefeature-groupsmatrix (:336) on one volume and one ref — and serialises the same way, once per leg.Files touched by this review
None in the worktree; clean at
e94c46f. Mutation copies were confined to the session scratch directory and are removed.