Repository navigation
build: isolate the target dir when runtime/ diverges - #801
Conversation
A shared-bucket nub binary resolves runtime/*.cjs from the tree that compiled nub-core (baked CARGO_MANIFEST_DIR), so a worktree editing only runtime files tested whichever sibling's copy was compiled last. Add runtime/ to the divergence and content-key pathspecs so such a worktree gets an isolated target, keeping shared-bucket binaries pointed at base-identical runtime content.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ The fix itself checks out. Two follow-ups: a stale skill doc, and a re-key cost the PR body understates.
Reviewed changes — the single commit adding runtime to scripts/rust-build.sh's pathspec set.
runtimeadded to all three pathspecs — thegit diff --name-onlydivergence probe, thels-files --othersuntracked probe, and thels-files -scontent key, so a worktree with any tracked or untrackedruntime/change gets a private target dir.- A 7-line comment block explaining why — that
runtime/is in the set for a different reason than the crates: run-time resolution through a bakedCARGO_MANIFEST_DIR, not the rlib-clobber hazard.
I traced the premise and it holds. crates/nub-core/src/node/spawn.rs:3382 resolves runtime/preload.mjs via env!("CARGO_MANIFEST_DIR") under #[cfg(not(feature = "embed-runtime"))], and route 1 (walk up from the binary dir) cannot fire for a shared-bucket binary because ~/.cache/nub/shared-target-* has no runtime/ ancestor — so the baked sibling path really is what a shared-bucket dev binary reads. I also checked the one blocking-shaped risk, that something writes an unignored file into runtime/ and pins every worktree to permanent isolation: nothing does. runtime/addons/*.node and runtime/node_modules/** are gitignored and all 19 other entries are tracked, confirmed across the Makefile, .github/workflows/**, scripts/** and the Rust integration tests. Plain runtime is also a leading-path match, so it does not catch site/content/docs/runtime/.
⚠️ The rust-build skill doc still describes the old pathspec set
scripts/rust-build.sh:63 names .claude/skills/rust-build/SKILL.md as "the full model", and that doc now contradicts the script in three places. The worst is The common case: feature work in nub-cli, integration tests, docs, non-Rust files — runtime .cjs/.mjs files are exactly the non-Rust files that now isolate, so an agent reading the skill mispredicts the wrapper's behavior on the change this PR was written for.
Technical details
# Sync `.claude/skills/rust-build/SKILL.md` with the new pathspec set
## Affected sites
- `.claude/skills/rust-build/SKILL.md` — frontmatter `description`: "auto-isolates the moment a worktree diverges a depended-on crate". Isolation now also fires on a `runtime/` change, which is not a crate.
- `.claude/skills/rust-build/SKILL.md` — "The rule the wrapper enforces": `The common case: feature work in nub-cli, integration tests, docs, non-Rust files.` Actively wrong now — editing `runtime/preload-common.cjs` isolates.
- `.claude/skills/rust-build/SKILL.md` — "Depended-on crates = every workspace/vendored crate **except leaf artifacts nothing links** … So: `crates/nub-core`, `crates/nub-cache-key`, `crates/nub-phantom-core`, `crates/nub-phantom-scan`, and all of `vendor/aube`." An exhaustive enumeration of the hashed set, now missing `runtime/`.
## Required outcome
- A reader of the skill can predict which edits isolate and which share, matching what the script does at head.
- The `runtime/` entry is described by its own mechanism (baked `CARGO_MANIFEST_DIR` run-time resolution) rather than folded into the rlib-clobber story, mirroring the comment this PR added at `scripts/rust-build.sh:100-106`.ℹ️ Nitpicks
scripts/rust-build.sh:123— "so this is a pure content hash of the depended-on crates" now understates the input;runtime/is hashed too and is not a crate.scripts/rust-build.sh:181— a worktree that edited onlyruntime/preload.cjsnow printsisolated — this worktree diverges a depended-on crate from origin/main, which points the reader at the wrong directory to go look in.scripts/rust-build.sh:197has the matching wording on the shared branch.
Claude Opus ｜ 𝕏
The legacy migration seeded only from the bare pre-keying dir, which the 14-day GC retires — so any later key move (origin/main advancing a hashed path, or a pathspec addition) landed in an empty bucket and cost every worktree a cold build. Fall back to the newest keyed bucket, and clear an empty placeholder dir first (an early mkdir otherwise blocks seed_from permanently). Sync the rust-build skill doc with the runtime/ pathspec.
There was a problem hiding this comment.
Important
The doc sync is clean. The new re-key seed makes foreign-content seeding of a shared bucket a standing behavior, which reopens the hazard the keying exists to close — and the duplicated key formula in the disk-reduction tool was not updated.
Reviewed changes — the delta since the prior review at 269ec87 is one commit, daea05e, plus the skill-doc sync it carries.
- Generalized the shared-branch seed into a re-key migration —
scripts/rust-build.sh:198-219replaces the one-time "migrate from the legacy un-keyed dir" seed withrmdirof an empty placeholder bucket, then a CoW seed fromnewest_bucket()falling back to$shared. - Synced
.claude/skills/rust-build/SKILL.mdwith the new pathspec set — the frontmatterdescription, the share/isolate bullets, and the hashed-set enumeration now nameruntime/and describe its distinct baked-CARGO_MANIFEST_DIRmechanism rather than folding it into the rlib-clobber story.
The doc sync resolves the prior review's stale-skill finding at every site it named, and the seed fallback is what the prior inline comment asked for. Both prior points are retired.
⚠️ The disk-reduction bucket sweep duplicates the key formula and this PR breaks its positive control
.claude/skills/disk-reduction/scripts/clean-shared-buckets.py recomputes the bucket key from its own copy of the pathspecs (CRATE_PATHS = ["vendor/aube", "crates"]), which this PR does not touch. Its docstring anticipates exactly this drift and guards against it: the script compares its recomputed key against what scripts/rust-build.sh --print-target resolves and refuses --apply when they disagree. After this merges they always disagree, so the sweep the skill credits with 87 GiB of reclaim is audit-only until CRATE_PATHS gains runtime.
Technical details
# Update the duplicated key formula in the disk-reduction bucket sweep
## Affected sites
- `.claude/skills/disk-reduction/scripts/clean-shared-buckets.py:41` — `CRATE_PATHS = ["vendor/aube", "crates"]` omits `runtime`, so `bucket_key()` (L55-60) hashes a different input than `scripts/rust-build.sh:125`.
- `.claude/skills/disk-reduction/scripts/clean-shared-buckets.py:63-73` — `is_diverged()` uses the same `CRATE_PATHS`, so a worktree that diverges only `runtime/` is classified non-diverged and contributes a key to `live` that no bucket carries.
- `.claude/skills/disk-reduction/scripts/clean-shared-buckets.py:121-138, 175-180` — `control_passes()` compares the recomputed key with `rust-build.sh --print-target`; the mismatch makes it return `False` and `--apply` self-disables.
- `.claude/skills/disk-reduction/SKILL.md:46` — "`<key>` hashes the content of the depended-on crates (`vendor/aube` and `crates`, excluding the leaves …)" is the same stale enumeration in prose.
## Required outcome
- The Python key computation and the shell one produce the same hash for the same tree, so the positive control passes and `--apply` works.
- The prose description of the hashed set in `SKILL.md` matches `scripts/rust-build.sh` at head.
## Open questions for the human
- `--print-target` now runs the `rmdir` + CoW seed block (`scripts/rust-build.sh:206-219`) before the `--print-target` early return at L271. `SKILL.md:80` already warns that `--print-target` is not a read, and the script's docstring uses it as the control — but now the control call itself can materialize a whole new bucket, on a run whose purpose is to reclaim disk. Worth deciding whether `--print-target` should short-circuit above the seed block.Claude Opus ｜ 𝕏
A seed source's hashed content differs from the new bucket's by construction, CoW preserves mtimes, and cargo's path-dep freshness check compares mtimes — so an older-checkout sharer would accept a foreign rlib as fresh, inside a bucket other worktrees join. A key move now costs the first worktree one cold build, as originally designed. Also make --print-target exit before the mkdir/GC block (a pure read), and sync the duplicated key formula in the disk-reduction sweep so its positive control passes again.
There was a problem hiding this comment.
ℹ️ Both prior findings are properly closed. Two accuracy gaps remain around
--print-targetand the stated re-key cost — inline.
Reviewed changes — the delta since the prior review at daea05e is one commit, 672a6c1.
- Deleted the shared-branch re-key seed —
scripts/rust-build.sh:195-208now assignstarget="$bucket"and seeds nothing, with a comment grounding the refusal in cargo's mtime-basedLocalFingerprint::CheckDepInfopath-dep freshness check and naming the private-target property the isolated branch relies on. - Moved the
--print-targetearly return abovemkdir/touch/GC —scripts/rust-build.sh:210-220, so the disk-reduction sweep's positive control no longer materializes a bucket by asking. - Synced the duplicated key formula in the disk-reduction tool —
CRATE_PATHS = ["vendor/aube", "crates", "runtime"]inclean-shared-buckets.py:44plus the matching prose indisk-reduction/SKILL.md:46, which repairscontrol_passes()and re-enables--apply.
Both prior findings are genuinely retired, not papered over. I re-derived the key equivalence rather than assuming it: Python's hashlib.sha1(...)[:12] matches shell shasum | cut -c1-12 (shasum defaults to SHA-1), and git ls-files -s output is path-sorted, so the differing pathspec argument order between the two implementations cannot move the hash. I also re-confirmed the no-spurious-isolation premise at head — git check-ignore -v puts runtime/node_modules/** under .gitignore:101 and runtime/addons/*.node under .gitignore:105, with the other 19 entries tracked.
ℹ️ Nitpicks
scripts/rust-build.sh:232— "while the bare legacy dir still is — which is what lets the migration retire itself" points at a migration this commit deleted. The legacy$shareddir now survives only as the isolated branch's last-resort seed (L191), so its GC is no longer retiring anything..claude/skills/disk-reduction/scripts/clean-shared-buckets.py:4-5— the module docstring still says the key is "a content hash of the depended-on crates". The new comment at L41-43 coversruntime, but the docstring is what a reader hits first.
Claude Opus ｜ 𝕏
| # sharer whose checkout is older than the seeded dep-info would accept a | ||
| # foreign rlib as fresh: the phantom E0063 this keying exists to prevent, | ||
| # persisting inside a bucket other worktrees join. A key move therefore costs | ||
| # the FIRST worktree one cold build and everyone else joins warm — that is the |
There was a problem hiding this comment.
The cost accounting covers sharers but not isolated worktrees. mkdir -p (L222) and touch (L227) publish the new bucket before cargo writes anything into it, and newest_bucket() (L174-177) ranks purely by mtime with no warmth check — so for the duration of that first cold build, every isolated worktree seeds from an empty or half-built tree and prints (seeded from shared-target-…) while getting a cold build of its own.
Technical details
# The re-key cold build is not confined to the first sharer
## Affected sites
- `scripts/rust-build.sh:204-207` — "A key move therefore costs the FIRST worktree one cold build and everyone else joins warm" accounts only for the shared branch.
- `scripts/rust-build.sh:222,227` — `mkdir -p "$target"` then `touch "$target"` make the brand-new, still-empty bucket the newest by mtime before cargo has produced a single artifact.
- `scripts/rust-build.sh:174-177` — `newest_bucket()` sorts `ls -dt` and filters only `.seeding`, so that empty bucket is the top hit.
- `scripts/rust-build.sh:189-194` — a diverged worktree's own `$bucket` is keyed by its index and therefore never exists on disk, so `newest_bucket()` is the seed source for essentially every isolated worktree.
- `scripts/rust-build.sh:140-166` — `seed_from` succeeds on an empty source (`cp -a src/. dst/` copies nothing and exits 0), so `$why` reports a successful seed that inherited nothing.
## Required outcome
- An isolated worktree does not adopt a bucket that has no artifacts in it yet, and the `(seeded from ...)` message is not printed when nothing was actually inherited.
## Open questions for the human
- Seeding from a bucket a sibling is *actively building into* pairs a fresh-looking fingerprint with a partial rlib — the same hazard `newest_bucket()`'s `.seeding` filter already names in its own comment. That exposure is pre-existing in kind, but deleting the shared-branch seed makes a from-scratch bucket the guaranteed state after every merge that moves the key. Is bounding it worth a warmth check (e.g. requiring a non-empty `deps/`) here, or is the window short enough to accept?The isolated branch's seed ran before the early return, so asking a diverged worktree for its target still CoW-cloned a bucket into its private target. Move all seeding below the --print-target exit, skip empty buckets in newest_bucket (a just-published bucket is newest by mtime while holding nothing), and correct the re-key cost comment and the disk-reduction skill's contract line.
There was a problem hiding this comment.
Caution
This PR introduces a silent hard failure in scripts/rust-build.sh. newest_bucket()'s new return 1 is not exempt from set -e, so an isolated worktree with no non-empty bucket to seed from exits 1 before the banner and before exec cargo, with zero output. Under make verify the fallback can never succeed, so the pre-push gate fails outright on a fresh worktree.
Reviewed changes — the delta since the prior review at 672a6c1 is one commit, fa2d907.
- Split the target decision from the seeding —
scripts/rust-build.sh:188-236introduces anisolatedflag so both branches assigntarget/whyfirst, and the seeding moved below intoif [ -n "$isolated" ]. - Moved the
--print-targetearly return above the seed block —scripts/rust-build.sh:217-220, making it a genuine pure read on both branches. Verified: everything above the return is git reads, nothing touches the filesystem. - Rewrote
newest_bucket()to skip empty buckets —scripts/rust-build.sh:177-186replaces thels | grep | head -1pipeline with a loop that testsls -Aper candidate andreturn 1s when none qualify, plus a matching condition at the_seedfallback (L231). - Rewrote the
disk-reductionskill's--print-targetwarning —.claude/skills/disk-reduction/SKILL.md:80now documents it as a pure read instead of a write.
The --print-target half is correct and complete, and the prior thread on it is retired. The newest_bucket half is where both remaining findings sit.
ℹ️ scripts/rust-build.sh has no test coverage, and its failure mode is silence
Two consecutive review rounds on this PR have found behavior defects in the same twelve-line block, and the current one produces no output at all — an agent or human hitting it sees make verify exit 1 with an empty log and no way to tell the wrapper from cargo. A smoke test that runs the script against a scratch NUB_SHARED_TARGET in both the shared and isolated shapes and asserts it reaches exec would have caught this and the previous round's issue in under a second.
Technical details
# Give `scripts/rust-build.sh` a minimal behavioral smoke test
## Affected sites
- `scripts/rust-build.sh` — no test anywhere in the repo exercises it; `grep -rn "rust-build.sh"` finds only callers and prose.
- `Makefile:68,117-136` — `make install-dev` and every `make verify` leg route through it, so a wrapper that exits early takes the documented pre-push gate down with it.
## Required outcome
- A cheap, hermetic check that the wrapper reaches `exec cargo` (or a stubbed `cargo` on `PATH`) for the four reachable states: shared/isolated x seed-source-present/absent.
- The check must fail on the L231 defect as it stands today, i.e. it is a positive control before it is a regression guard.
## Suggested approach (optional)
A `sh` test that points `NUB_SHARED_TARGET` at a scratch dir, puts a `cargo` stub earlier on `PATH` that prints a sentinel, and asserts the sentinel appears. Divergence can be forced with an untracked file under `runtime/`, which is how the defect below was reproduced. Whether this belongs in the Rust integration suite or as a standalone `tests/<feature>/` harness is your call.ℹ️ Nitpicks
.claude/skills/disk-reduction/scripts/clean-shared-buckets.py:14-16and:175still protect the newest bucket by mtime (buckets[0]), which no longer matchesnewest_bucket()'s newest-non-empty semantics — the same duplicated-logic drift this PR fixes forCRATE_PATHS. Low impact in practice, sincebuilds_running()refuses--applyin exactly the window where an empty bucket exists.
Claude Opus ｜ 𝕏
| # after are identical across buckets. | ||
| _seed="$bucket" | ||
| [ -d "$_seed" ] || _seed=$(newest_bucket) | ||
| [ -d "$_seed" ] && [ -n "$(ls -A "$_seed" 2>/dev/null)" ] || _seed=$(newest_bucket) |
There was a problem hiding this comment.
newest_bucket now return 1s, and this assignment is the last command of the A && B || C list — so set -e (L65) is not suppressed and the script exits 1 here, before mkdir, before the rust-build: banner, and before exec cargo. Nothing is printed. Baseline 672a6c1 could not hit this: newest_bucket ended in head, which always exits 0.
Under make verify this is not an edge case. NUB_SHARED_TARGET="$(CURDIR)/target" makes newest_bucket glob $(CURDIR)/target-*, a name that never exists, so it always returns 1 — any diverged worktree whose target/ is absent or empty fails the pre-push gate with an empty log.
Technical details
# `newest_bucket`'s failure exit must not propagate through `set -e`
## Affected sites
- `scripts/rust-build.sh:231` — `_seed=$(newest_bucket)` terminates the `&& ... ||` list, so its status is the list's status and `set -e` fires.
- `scripts/rust-build.sh:177-186` — `newest_bucket()` gained `return 1`; the pre-`fa2d907` pipeline form could not fail.
- `scripts/rust-build.sh:232` — `[ -n "$_seed" ] && [ -d "$_seed" ] || _seed="$shared"` already normalizes an empty `$_seed`, so the callers downstream need no change.
- `Makefile:117-136` (`make verify`), `Makefile:68` (`make install-dev`) — the two gates that go down with it.
## Reproduction (run in this repo, both shapes confirmed on sh/dash/bash)
```sh
touch runtime/__probe.txt # force divergence
rm -rf /tmp/nb-a
NUB_SHARED_TARGET=/tmp/nb-a/target sh scripts/rust-build.sh --version # rc=1, no output
mkdir -p /tmp/nb-c/target # second shape: present but EMPTY
NUB_SHARED_TARGET=/tmp/nb-c/target sh scripts/rust-build.sh --version # rc=1, no output
```
Restoring only the pre-`fa2d907` `newest_bucket` body and the old `_seed` line into a copy of the script makes both cases print the banner and run cargo (rc=0), which isolates the cause to this change rather than to the surrounding restructure.
## Required outcome
- A diverged worktree with no usable seed source falls through to `$shared` and builds cold, instead of aborting.
- The `present-but-empty` seed dir behaves the same way.| [ -d "$_seed" ] && [ -n "$(ls -A "$_seed" 2>/dev/null)" ] || _seed=$(newest_bucket) | |
| [ -d "$_seed" ] && [ -n "$(ls -A "$_seed" 2>/dev/null)" ] || _seed=$(newest_bucket || true) |
| # shellcheck disable=SC2012 # names are ours and contain no newlines | ||
| ls -dt "$shared"-* 2>/dev/null | grep -v '\.seeding$' | head -1 | ||
| for _b in $(ls -dt "$shared"-* 2>/dev/null | grep -v '\.seeding$'); do | ||
| if [ -n "$(ls -A "$_b" 2>/dev/null)" ]; then |
There was a problem hiding this comment.
ls -A tests for any entry, and cargo makes a pre-created target dir non-empty almost immediately: measured on a clean dir here, .rustc_info.json lands 57 ms after cargo build starts. So this skip closes a ~50 ms window, not "during its first cold build" as L173-176 and L203-205 both claim — for the remaining minutes of that cold build the near-empty bucket is still newest-by-mtime, still non-empty, and still the top pick.
Technical details
# The emptiness filter is not a warmth check
## Affected sites
- `scripts/rust-build.sh:180` — `[ -n "$(ls -A "$_b" 2>/dev/null)" ]` is satisfied by `.rustc_info.json` alone.
- `scripts/rust-build.sh:173-176` — "during its first cold build it is the newest by mtime while holding nothing worth cloning" describes a window that ends ~50 ms in.
- `scripts/rust-build.sh:203-205` — "an isolated worktree seeding during that window falls back to the newest non-empty bucket" states the cost accounting is now covered; it is not.
- `scripts/rust-build.sh:231-235` — an isolated worktree therefore still CoW-clones an artifact-less bucket and prints `(seeded from shared-target-...)` while paying a full cold build.
## Measurement
```sh
cargo new --lib /tmp/cp2 && cd /tmp/cp2 && rm -rf target && mkdir target
(cargo build --offline &) ; poll ls -A target every 50ms
# -> non-empty after 57ms, first entry: .rustc_info.json
```
## Required outcome
- Either the check distinguishes a bucket with compiled artifacts from one cargo has merely touched, or the two comments stop claiming it covers the cold build.
## Suggested approach (optional)
A warmth test wants the artifact directories rather than the root — e.g. requiring a non-empty `"$_b"/*/deps` or `"$_b"/*/.fingerprint`. If you would rather not add that, narrowing both comments to what `ls -A` actually buys is a legitimate resolution; the cost is a cold build, not a wrong build.
## Open questions for the human
- The prior round asked whether bounding this is worth a warmth check at all. If the answer is still "the window is short enough to accept", then the `ls -A` condition is not earning its complexity and the comments are the only thing that needs to change.Its return 1 propagated through the seed-selection AND-OR list and killed the script silently before any banner — always, under make verify's NUB_SHARED_TARGET, where the bucket glob can never match. No candidate is an ordinary outcome, answered with empty output. Also replace the any-entry emptiness test with an rlib probe: cargo makes a new bucket non-empty within ~50ms, so the old test closed almost none of the first-cold-build window.
|
Shipped in v0.8.1: https://github.com/nubjs/nub/releases/tag/v0.8.1 |
A shared-bucket
nubbinary resolvesruntime/*.cjsfrom the tree that compilednub-core(its bakedCARGO_MANIFEST_DIR), so a worktree editing only runtime files ran a sibling worktree's copy — three concurrent fix branches hit this, with red/green verdicts flipping as siblings rebuilt the shared bucket.Fix: add
runtimeto the divergence and content-key pathspecs, so a worktree with any tracked or untrackedruntime/change gets an isolated target dir. A key move costs the first worktree one cold build (a shared bucket is never seeded from foreign content — cargo's mtime-based path-dep freshness would accept foreign rlibs).--print-targetis now a pure read, and the disk-reduction sweep's duplicated key formula is synced so its positive control passes.