Commit Graph
5 Commits
Author SHA1 Message Date
claudeandClaude Opus 5 719475831b fix(cargo-cache): scope the safety claim to what the code actually prevents
Follow-up to f57e2a6, addressing the reviewer's sharpest question: is the race
"closed by construction", or merely detected and retried? The honest answer is
"both, on different paths", and the README said only the first half.

* README now states three claims separately instead of collapsing them:
  a publisher rotating a snapshot cannot tear a clone of it (by construction —
  the marker ordering prevents the unlink, and the consumer's verification is
  a redundant second check on that path); every OTHER way the source can
  change mid-clone is detected, not prevented (the eviction pass's reader
  check is check-then-delete, and a seed-fallback-dir has no interlock at all
  — there, verification plus a bounded retry and a loud failure is the whole
  guard); and disk reclamation is bounded rather than immediate. Overclaiming
  this property once was the finding; overclaiming it twice would be worse.

* publish-snapshot-selftest.sh now covers the PUBLISHER's half of the race,
  where a reader of the swap belongs. Scenario 3 only ever covered a consumer
  that had already FINISHED cloning — safe for free, since its own hardlinks
  keep the inodes alive. New scenario 6 covers a reader still in flight past
  the grace period (the generation is left on disk, the deferral is warned
  about, and an earlier consumer is still unaffected); scenario 7 covers the
  sweep, so "we defer instead of forcing" cannot quietly become a disk leak.
  Red-proven against 248af306's scripts:

    ASSERTION FAILED: the previous generation was unlinked while a reader
    still held it

  The consumer's half stays in seed-target-dir-selftest.sh scenario 8, which
  still red-proves at 16693 of 48805 entries against the same scripts.

* seed-target-dir-selftest.sh now asserts what happens when the retries are
  EXHAUSTED, not just what hardlink_clone_into returns: an unreadable source
  makes the seed script exit non-zero, name the reason, leave no target dir,
  and — the one that matters — not fall through to its cold-start branch. A
  corrupt-cache bug degrading into an invisible 4x-slower CI job is the
  failure mode worth pinning down. Skipped when running as root, where mode
  bits deny nothing.

* usage_kb: a directory we cannot read measured as the empty string, which was
  then spliced into usage_gb's awk program and made it a syntax error at the
  exact moment something was already going wrong. Now measures 0.

Verification: `bash scripts/selftest.sh` — 5 suites, exit 0, 82 assertions
(was 75 after f57e2a6, 63 before). shellcheck over scripts/: no new findings.

Measured the cost the reviewer asked about, on ext4, warm cache, 78,554
entries: `cp -al` 3126 ms against 44 ms for one `find | wc -l`. Two counts per
attempt is ~2.8% on top of the clone. Not measured on the CI runner's volume.

Refs: daniel/gitdan#11, zemyna#911

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sqh2vscfzisk83VuPVQX9L
2026-08-23 16:49:44 -05:00
claudeandClaude Opus 5 f57e2a6013 fix(cargo-cache): close the seed-vs-republish race the design claimed to close
The shared action's justification over zemyna's and emowheel's schemes was
that hardlink-cloning from a published snapshot closes gitdan #911 "by
construction, not by the single job slot". Review disproved that. This makes
the claim true, and corrects the README where it could only be bounded.

Finding 1 (verdict-level) — silent partial clone
------------------------------------------------
`hardlink_clone_into` ran `cp -al` with no exit-status check, and both call
sites invoked it as a condition, which suppresses `set -e` for the whole call.
A publisher's `rm -rf` of the generation it rotated away therefore unlinked
entries beneath an in-flight consumer walk, and the truncated tree was renamed
into place and reported as success.

Both layers are fixed:

* The consumer verifies its own clone. Every attempt checks `cp -al`'s status
  explicitly, the source directory's inode before and after (a wholesale
  replacement mid-walk splices two generations), and the entry count — the
  only signal for a subtree unlinked before its parent was listed, since
  `cp -al` reports no error for one it never saw. Any failure discards the
  staging tree and retries; exhausting the attempts returns a distinct status
  2 and fails the job rather than seeding a partial cache. `unshare_subtree` /
  `_unshare_files` now propagate failure too — a swallowed unshare leaves the
  clone aliasing its source, the exact corruption that step exists to prevent.
* The publisher does not unlink under a reader. A consumer publishes a
  `.reading-<snapshot>-<tag>` marker before it resolves the snapshot path; the
  publisher scans for markers after its first rename. A consumer holding the
  old generation therefore published its marker before that scan and cannot be
  missed; one arriving after the scan necessarily resolves to the new
  generation. The publisher waits for readers to drain and, on timeout,
  DEFERS reclamation rather than forcing it — the old generation is left as
  `.publish-old-<key>-<tag>` and swept by a later publish.

So correctness is closed by construction; disk reclamation is bounded, not
immediate. The residual is capped at one deferred generation per publisher
ref, and the README now says exactly that instead of the disproved claim.

Finding 2 — restore-mtimes.sh ran with no errexit
-------------------------------------------------
`set -euo pipefail` was glued to the end of a comment (`# soundness.set -euo
pipefail`), so it was entirely commented out: a partial failure of the
`git log | awk` pipeline would have produced wrong mtimes across the whole
restore instead of failing loudly. Moved to its own line. Audited every other
script for the same defect — this was the only instance. Independent
confirmation: shellcheck's two SC2164 warnings on this file's `cd "$repo_root"`
disappear now that errexit is actually in effect.

Finding 3 — lock-acquire window
-------------------------------
A just-seeded directory was unlocked until a later action step, so a
concurrent job's prune pass could evict it. `seed-target-dir.sh` now takes an
optional lock-id and writes the lock marker on every path out of the script,
including into the staging tree before its rename, so the directory carries a
lock the instant it appears under its final name. The action's acquire step
stays (it is idempotent and stamps the LRU marker).

Also hardened `prune-cache.sh` to treat a directory with live reader markers
as locked. Today no reachable configuration prunes a snapshot — only protected
refs publish them and protected refs are excluded from every pass — so this is
redundant by policy; it is here so that stops being the reason it is safe.

Verification
------------
New selftest scenario 8 races a real seed against a real publish rotation,
gating the rotation on the seed's *observed* clone progress so the window is
hit deterministically rather than on a fast machine's coin flip. Red-proven
against the unguarded scripts, three consecutive runs:

  ASSERTION FAILED: the seeded tree is truncated: 15443 entries against the
  snapshot's 493 (was 48805 before the rotation)      (15443 / 16986 / 16498)

Green after the fix, six consecutive runs, catching the clone mid-walk at
~10.5k of 48805 entries each time. Scenario 9 covers deferred reclamation and
its later sweep; scenario 10 covers an unreadable source failing loudly.

`bash scripts/selftest.sh`: 5 suites, exit 0, 75 assertions (was 63).
shellcheck over `scripts/`: no new findings, two SC2164 warnings resolved.

Docs: README's republish-safety paragraph replaced with what the code now
guarantees, including the bounded disk residual stated explicitly; new
`read-grace-seconds` / `reader-stale-seconds` inputs documented in the
`cargo-cache-publish` table; the selftest table names the new race.

Refs: daniel/gitdan#11, zemyna#911

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sqh2vscfzisk83VuPVQX9L
2026-08-23 16:40:21 -05:00
claudeandClaude Opus 5 248af3061e feat(cargo-cache): hardlink-clone a per-ref Cargo cache from a published snapshot
Replaces the phase-0 resolution probe with the real actions, merging the two
independent per-branch Cargo cache implementations on this forge into the
design neither of them had.

## The merge

- zemyna seeds a PR branch by `cp -al` hardlink clone (near-free: cost scales
  with inode count, not bytes) from the base branch's LIVE target dir — a
  torn read waiting for a second job slot (its own #911).
- emowheel seeds from a PUBLISHED IMMUTABLE SNAPSHOT (no race by
  construction) but with `cp -a`, duplicating ~35 GB per branch.

This ships hardlink-clone FROM a published snapshot: zemyna's cost profile,
emowheel's soundness, and #911 closed structurally rather than by the runner
happening to have one execution slot.

## The bug both implementations have

A build inside a `cp -al` clone DOES mutate the directory it was cloned from.
Cargo replaces real artifacts, but writes its metadata — and build scripts
write their OUT_DIR — with a plain truncating write, straight through the
shared inode. Measured set: `.fingerprint/<unit>/dep-<target>` (under
CARGO_UNSTABLE_CHECKSUM_FRESHNESS), `build/<pkg>/{output,root-output,out/**}`,
`deps/*.d` and `<profile>/*.d`.

The checksum-freshness case is a wrong answer, not a slow build: a PR clone
rewrites the base's dep-info to describe the PR's sources while the base's
cache still holds the artifact built from the base's; once the PR merges, the
base's next run finds the checksums match, reports `Fresh`, and links a binary
built from the pre-merge code. Reproduced end to end.

Fix: hardlink the artifacts (the GB), real-copy the metadata (the MB) — about
3.7% of a 6.9 GB Bevy target dir, against 100% for a full copy.

## Contents

- `cargo-cache/action.yml` — consume: resolve keys, seed from the base's
  snapshot via staging + one atomic rename, strip Cargo lock files, unshare
  the mutable paths, restore mtimes from git history, lock, prune.
- `cargo-cache-publish/action.yml` — publish: record the build watermark,
  atomically republish the snapshot on a protected branch, release the lock
  (`mode: release-lock` for the `if: always()` step).
- `scripts/` — all logic, so it is testable standalone; the YAML is wiring.
- `scripts/*selftest.sh` + `selftest.sh` — five suites, 63 assertions, every
  fix paired with a control that reproduces the bug. All green locally.

Eviction merges emowheel's liveness pass (dead branches pruned
unconditionally, not gated on disk pressure) with LRU-under-pressure, but
inverts the order within the pressure pass: `target-*` before `snapshot-*`,
because a snapshot is hardlinked to everything cloned from it, so evicting one
frees almost no real bytes while costing every future PR its warm start.

restore-mtimes.sh is ported from emowheel (the watermark variant, which closes
the merge hazard zemyna's copy still has) with its provenance de-projectised.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sqh2vscfzisk83VuPVQX9L
2026-08-23 14:18:01 -05:00
claude 8503883138 feat: cargo-cache composite action skeleton (phase-0 resolution probe) 2026-08-23 11:52:31 -05:00
claude 059cefc401 Initial commit 2026-08-23 16:51:42 +00:00