b791896e032a7df960702d3f55bbf94d9e28098f
6
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 claude
|
5b46986cd7
|
test(hardlink): make the errexit invariant real, and stop gpg deciding a gate
CI / shellcheck + selftests (pull_request) Successful in 1m23s
Two review findings on #13. THE STATED INVARIANT WAS NOT IN FORCE. The header claimed a `set -e` abort could never be mistaken for the `1` that means "measured, and the answer is mtime". It could not fire at all: `checksum_freshness_probe || probe_rc=$?` runs the left side with errexit suppressed, and that suppression propagates into the subshell. An unguarded step there fell through to `exit 0` and reported ACTIVE — a fourth outcome the header said was impossible. The guard audit was complete, so nothing was broken; the wrong mechanism had the credit, in a comment inviting the next editor to add an unguarded step and rely on it. The suggested fix — `set -e` as the first line of the subshell — does not work, and measured on bash 5.3 it makes things worse rather than not-better: set -e inside a subshell called via `||` or `if` still falls through: rc=0 set -e inside, called with errexit off at the site aborts, but with the FAILING COMMAND's status — `false` gives 1, which is exactly the value that means "mtime" So both halves are needed and neither is decoration: `set +e` around the call site, so the subshell can arm its own errexit at all, and `trap 'exit 2' ERR` inside it, so an abort lands on "not measured" instead of on an answer. The explicit `|| exit 2` guards stay as the first line of defence. All of that is now written down where the false claim was. Red-proven with one unguarded `false` between the `touch` and the second build, on a box where freshness is live: without the fix, mode `on` and 4 assertions — the reviewer's fourth outcome, reproduced; with it, mode `unmeasured`, the warning, 3 assertions; unmodified, mode `on` and 4. Recorded in the same block, since the next person to hit a `2` will ask: why it skips rather than fails. The gated scenario is the only thing here that depends on freshness mode, and failing would turn a statement about one machine into a red gate reading "the hardlink scheme is broken" across three consuming repos. What would change the answer is `2` becoming the everyday CI outcome, and run 2583 measures that it is not — gitdan-ci reports `1`. GPG. The two suites that commit in scratch repos inherited the developer's GLOBAL `commit.gpgsign`, so whether this gate passes could depend on their gpg agent — seen as a red `restore-mtimes-selftest.sh` caused by a full disk breaking gpg, in a suite with nothing to say about either. Pinned off locally, in the throwaway repos only: `git config commit.gpgsign false` beside the identity the scratch repo already sets, and `-c commit.gpgsign=false` on prune-cache-selftest's three commits, matching its existing `-c` style. Red-proven under a GIT_CONFIG_GLOBAL with gpgsign on and a nonexistent gpg program: without it `fatal: failed to write commit object`, with it both suites pass. |
||
|
claude
|
b0d63c807b
|
style(prune-cache): rewrap the settle-window comment; drop an overclaim
Review nits on #2, both cosmetic. The sentence added last round left its paragraph at 127 characters in a file that otherwise wraps comments at 78-79 — the rewrap after the insert simply did not happen. `awk 'length($0)>80'` over both files now reports one comment line, the pre-existing 93-character usage string at :4. Scenario 14's header called its fixture "the only shape that can tell the two timestamps apart". The property it needs is old mtime with fresh ctime; an old directory renamed a moment ago is the production instance of that, not the only construction of it. "Production's shape" was already carrying the argument. No behaviour change and no assertion change. |
||
|
claude
|
dcd73dd82d
|
test(prune-cache): build scenario 14's aside the way the pass builds one
Review finding on #2. Swapping the settle window's `stat -c %Z` for `%Y` — the exact substitution the comment beside it calls the wrong signal — left the suite green at 37/37. The scenario built its aside with `mkdir`, so the fixture's mtime was also ~now and the two timestamps agreed; a fixture whose clocks agree cannot tell them apart. Every real aside is the opposite shape: a cache last written days ago, renamed a moment ago. Under `%Y` the window would never fire for one, the sweeper would silently return to reclaiming asides another pass is still deciding about, and nothing would say so — the mechanism guarded by a comment again, which is what the previous finding was about. So the fixture is now built the way the pass builds one: an old directory `mv`d into the aside name. `%Z` -> `%Y` now fails scenario 14, as does deleting the guard outright. Also sharpens why the PID alternative was rejected: the `$$` in an aside's name was that pass's PID inside its own job container, so testing it from another one is not unreliable, it is meaningless. No change to prune-cache.sh's behaviour; the assertions are untouched. |
||
|
claude
|
65f0782233
|
fix(prune-cache): stop the aside sweeper depending on timing it cannot see
Review findings on #2. The first is the one that mattered: the sweeper this PR added had the shape the PR exists to remove. Pass A renames a candidate aside; pass B's sweep sees an aside with no readers and reclaims it; A then finds a reader and restores. `rm -rf` traverses fd-relative, so the rename does not stop it and A can republish a half-emptied tree under a live cache name. `capacity: 1` bounds it today, which is exactly the kind of reason this PR was written to stop relying on. The unlink itself was never the problem — the ordering proof covers it under any interleaving, since the aside name only exists after the evicting pass's rename. What was missing is that an aside with no readers is indistinguishable from one a pass has just created and not yet decided about. The sweeper now leaves an aside alone until it has settled (EVICTION_ASIDE_SETTLE_SECONDS, default 60), which separates the two without having to identify the pass that created it — a PID is meaningless across the job containers these passes run in, and recycles. Read from ctime, not mtime: rename(2) updates the first and leaves the second at whenever the cache was last written, which is the signal list_by_lru wants and the wrong one here. That is a bound, not a construction, and both the code comment and the README now say which of the two properties is which instead of asserting the broader one. Also from the review: a pass that declined every dead cache it found no longer signs off with "no dead-branch caches found", and evict_dir no longer promises a later reclamation of an aside that is already gone. Scenario 14 covers the settle window against the script's own default, with nothing faked — the directory really was set aside a moment ago. Scenario 4 gains the summary assertion. 31 -> 37 assertions; each new gate verified red by defeating it alone in a scratch copy. |
||
|
claude
|
88ab063b64
|
fix(prune-cache): close the reader-marker check-then-delete window
Both eviction sites checked for a consumer's `.reading-` marker and then, seconds later — `usage_gb` runs `du -sk` over a multi-GB tree between the two — unlinked the directory. A consumer that started a clone inside that gap had its source removed mid-walk, which `cp -al` does not report: a subtree unlinked before its parent is listed is silently omitted. Unreachable today, and only by policy: snapshots belong to protected refs and protected refs never reach the marker check. `cargo-cache` and `cargo-cache-publish` take that ref list as two independent inputs, so a workflow listing a publisher in one and not the other arms this with no code change at all. Closed structurally, with publish-snapshot.sh's rotation rather than a new mechanism: the candidate is renamed aside and only then re-examined, so the scan the unlink rests on happens strictly after the rename. A consumer that resolved the directory published its marker before that scan and cannot be missed; one arriving after cannot resolve the path and starts cold, the same degrade the publisher's swap window already produces. Renaming disturbs no clone in flight — no entry is unlinked and the inode is unchanged — so a declined eviction costs a deferred eviction and nothing else. A reprieved cache is put back under its own name; one whose name a concurrent seed has retaken is left aside and swept by a later pass once its readers drain, since nothing else globs a dotted name. prune-cache-selftest gains three scenarios (21 -> 31 assertions). Scenario 12 is the one that bites: the `du` the pass runs on its candidate publishes the marker, placing it strictly after the check and strictly before the unlink. Against check-then-delete, 1-11 pass and 12 fails; breaking only the second look and leaving the rename fails it too. |
||
 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 |