build: run the analysis gates under --profile fast, matching the dev loop - #592
Merged
Conversation
…loop The documented dev loop iterates with `--profile fast` (target/fast) while `make verify` and CI's check/clippy jobs ran the default `dev` profile (target/debug). Nothing shared: crossing from iterating into gating triggered a full dependency rebuild -- measured at 153/153 units -- and the host carried two near-identical artifact universes, 12 GB of target/fast beside 14 GB of target/debug. The two profiles differ only in debuginfo. `fast` inherits `dev`, so opt-level, debug-assertions and overflow checks are identical, and no lint reads debuginfo. Moving the analysis gates onto `fast` collapses the duplication with no change in what they assert. Scoped to the gates that emit no artifact a later step reads by path: - check + clippy (root workspace and nub-native) move to --profile fast, in CI and in `make verify`. - The test jobs stay on `dev`. That job builds the addon in debug and reuses those artifacts in `cargo test` by design (its own comment says so), and ~15 hardcoded target/debug paths across ci.yml, aube-parity.yml and busybox-run-probe.yml consume them. - The nub-phantom check stays on `dev`: its workspace defines no `fast` profile, and `cargo check --profile fast` there fails with "profile `fast` is not defined". It is its own workspace with its own target dir, so there is no duplication to collapse anyway. Also fixes PROFILE handling in the Makefile, which this change would otherwise have tripped: CARGO_FLAGS was empty for every non-release profile, so PROFILE=fast built into target/debug while the copy steps read target/fast and the addon never landed. Only `debug` may take no flag -- cargo's profile is named `dev` but its output dir is `debug`. Verified: `cargo clippy --all-targets --all-features --profile fast -- -D warnings` exits 0 with no warnings, and `make PROFILE=fast addon` builds and lands the addon.
|
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}
Contributor
There was a problem hiding this comment.
ℹ️ No critical issues — one stale comment worth a look.
Reviewed changes — unifies --profile fast across the dev-loop docs, make verify, and CI's check/clippy gates so the analysis gates reuse the dev loop's artifact universe instead of driving a second full dev dependency build.
- CI
check/clippymove to--profile fast— root andnub-nativecargo check+cargo clippyinvocations gain--profile fast; thenub-phantomcheck and alltestjobs correctly stay on the defaultdevprofile. make verifyonPROFILE=fast— the addon build and both clippy runs now go throughPROFILE=fast, and a newdebugbranch is added to theCARGO_FLAGSselector.- Makefile
PROFILEbug fix — non-release profiles previously got emptyCARGO_FLAGS, soPROFILE=fastbuilt intotarget/debugwhile the copy steps readtarget/fast; theelsebranch now emits--profile $(PROFILE). - Doc sync —
dev-loop/SKILL.mdandAGENTS.mdpre-push gate invocations updated to match.
I verified the profile invariants the change relies on: [profile.fast] inheriting dev exists in both the root Cargo.toml and crates/nub-native/Cargo.toml, and crates/nub-phantom/Cargo.toml defines only [profile.release] — so leaving the nub-phantom check on the default profile is correct (a --profile fast there would fail with an undefined-profile error). The Makefile selector fix is sound. One informational nit inline.
Note: 1 inline comment(s) dropped because they did not anchor to lines inside the PR diff:
.github/workflows/ci.yml:164(RIGHT) — line 164 (RIGHT) is not inside a diff hunk
Claude Opus ｜ 𝕏
Contributor
|
One informational nit that couldn't anchor inline (the line is outside the diff hunk): the cache-key rationale at
|
The comment described the check artifacts as debug-profile; the root and nub-native checks now build under --profile fast, with only the nub-phantom leg on the default profile. The ci-check key itself is unchanged.
colinhacks
added a commit
that referenced
this pull request
Jul 29, 2026
…t key Content-keying broke `make verify`. Verify passes NUB_SHARED_TARGET="$(CURDIR)/target" to get a repo-local dir isolated from the shared cache, and keying silently redirected that to target-<hash> while the addon recipe still copied from target/<profile>/ -- so the copy missed and verify aborted at `@test -s runtime/addons/nub-native.node`. The keyed dir was matched by no .gitignore rule either, so the tree went dirty. An explicit NUB_SHARED_TARGET is now used exactly as given. Keying exists to make SHARING sound -- same path implies same content across the worktrees that converge on the one default cache -- and a caller naming a specific path is not sharing. Silently redirecting it breaks every consumer that knows where its artifacts land, of which the Makefile has three (TARGET, addon, addon-fast); honoring the name fixes all of them at once rather than teaching each to resolve through --print-target. The default path is still keyed. Neither PR could surface this alone: the Makefile changes were verified in a worktree without keying, the keying in a worktree that never ran `make verify`, and the interaction only became reachable once #592 merged and this branch synced. `make verify` is not a CI job, so the 30 green checks would not have caught it. Verified: verify's invocation resolves to .../target with no suffix, the default is still content-keyed, committed-divergence seeding still fires (re-tested via a HOME override, since the env var no longer exercises the keyed path), and the real addon build lands 13041296 bytes where verify asserts.
colinhacks
added a commit
that referenced
this pull request
Jul 29, 2026
…he buckets (#589) * build: seed isolated target dirs from the shared cache; content-key the buckets The wrapper claimed a relocated target dir gets 0% artifact reuse ("rustc bakes the target path into its fingerprints"), so isolation was treated as synonymous with a cold build. That claim is false. Measured on this workspace with controls in both directions: cloning a warm target dir to a new path rebuilt 0 crates, a genuinely empty dir rebuilt all 13, and touching one source in the clone rebuilt exactly 1. Cargo revalidates a relocated dir in place. The true version of that statement is about sccache, which keys on the rustc command line -- that embeds absolute --out-dir / -L paths, so a different target dir is a guaranteed miss there. The two mechanisms were conflated. The cost was real: 22 of 28 worktrees are isolated at any time, each paying a full rebuild of ~700 dependency rlibs identical to ones already in the shared cache. Three changes: - Seed a fresh private target dir from the matching shared bucket via CoW clone (cp -c on APFS, --reflink on btrfs/XFS; both fail rather than falling back to a real multi-GB copy). Only on first creation, never over an existing dir. - Key the shared bucket on a hash of the depended-on crate content. The old per-worktree merge-base proved only that a worktree made no changes against its OWN base, not that two sharers agreed with each other -- measured at 4 distinct contents among 6 worktrees that all read "shared", which is rebuild ping-pong plus the phantom-E0063 tail risk the rule exists to prevent. The first keyed bucket is migrated from the legacy dir so landing this costs no cold build, and buckets are content-addressed so mtime GC needs no liveness check. - Narrow the depended-on set by the two leaf artifacts nothing links: crates/nub-native (cdylib, own workspace) and crates/nub-phantom (bin, own workspace). nub-phantom-core and nub-phantom-scan are NOT leaves -- nub-cli depends on both -- and the pathspec :(exclude)crates/nub-phantom matches only that directory, not those siblings (verified). Also documents a third residue family in the cpu-reduction skill: orphaned synthetic load generators. Ten `while :; do :; done` shells from an interrupted probe ran 3h55m at 234% CPU, drove load to 277 and starved every build on the box. Includes the signature to tell them from a real build wrapper, the `pgrep -f` blind spot that hid them, and the trap that prevents them. * build: make the bucket GC actually self-retiring, and claim seed dirs atomically Review findings on the seeding change, all three valid: - The GC glob was `shared-target-*`, which requires a `-<hash>` suffix and so never matched the bare legacy `shared-target` dir -- contradicting the migration comment's claim that the legacy dir "just ages out". Broadened to `shared-target*`. mtime is a sound liveness proxy for the legacy dir too: anything still building there keeps it fresh, and a collected dir costs one cold build, never correctness. Verified: the old glob skips the legacy dir, the new one collects it, and fresh buckets plus unrelated dirs survive. - The check-then-copy seed was a TOCTOU race. Two worktrees with identical content could both pass `[ ! -d "$target" ]` and `cp` into the same bucket, and the loser would copy INTO the winner's dir, nesting a second full copy inside it. Replaced with a `seed_from` helper that claims the destination with `mkdir` -- atomic, fails if the dir exists -- then clones the contents. Verified: seeds once, no nested dir, and a re-run does not re-seed. - The content-key pipeline lacked the `|| true` its sibling git queries carry. Added for parity. * build: publish seeds by rename, and make the GC's liveness signal real Two further review findings, both against claims this change had introduced. The GC could collect a LIVE bucket. The comment argued mtime was a sound liveness proxy because "anything still building there keeps it fresh" and because `mkdir -p` touched the dir in use. Both are false, verified here: `mkdir -p` on an existing directory does not update its mtime, and neither does a nested write. A cargo target dir's top level goes quiescent after the first build, so a bucket in continuous use ages past the window and gets collected -- and an `rm -rf` landing mid-build is a failed build, not the cold build the comment promised. Fixed with an explicit `touch "$target"`, which is what makes the mtime rule mean anything. `seed_from` could hand cargo a half-copied target dir. The `mkdir` claim serialized the copiers but not the consumers: the loser fell straight through to `exec cargo` against a directory the winner was still filling, and an interrupted copy left a permanently half-populated dir that looked claimed forever. Agent builds here are killed routinely, so that is a real case. Now the clone lands in a `.seeding.<pid>` sibling and is renamed into place only after it returns 0, so the destination is never visible in a partial state; the exposure window is one rename rather than minutes of tree-walking, and a killed clone leaves only an unreferenced temp, swept on the same GC pass. The GC also matches exact names now (the keyed buckets plus the bare legacy dir) rather than a `*` prefix glob, which would have swallowed an unrelated prefix-sharing sibling such as a hand-made `shared-target.bak`. Verified: a touched (in-use) bucket and a `.bak` sibling both survive a sweep that collects the stale legacy dir and a stale bucket; seeding still produces no nested copy and leaves no temp behind. * build: make seeding fire for committed divergence; one claim, one clone Three more review findings, all valid. Seeding could not fire for the ordinary case. $bucket is keyed by THIS worktree's index, but the isolated branch is reached precisely because the content diverges, and buckets are only ever created by non-diverged worktrees -- so whenever the divergence is COMMITTED or staged (any feature branch) no bucket carries that key and the seed silently no-opped. Verified: base index key 822cd5234796, staged 596f08cc40bd. It only ever worked for unstaged edits, which is exactly what the earlier test exercised. The isolated seed now falls back to any bucket on disk. That is sound here in a way it would not be on the shared branch: the destination is PRIVATE to one worktree, so a stale workspace-crate artifact cannot clobber a sibling -- cargo fingerprints it as changed and rebuilds it -- and the ~700 crates.io rlibs we are actually after are identical across every bucket. The test-then-`mv` was still a race, and dropping the mkdir claim made every racer pay a full clone. `mv a b` with `b` an existing dir yields `b/a`, so two racers clearing the guard would nest a complete clone inside the winner's bucket where the GC's -maxdepth 1 never reaches it. Restored a claim, now a FIXED-name dir that doubles as the staging area: exactly one racer clones, and the publish is still a rename of a fully-populated tree. An abandoned claim is retired in-place after 2h, which covers the private target dir too -- it lives in the worktree, where the sweep beside the shared dir cannot see it. The temp sweep matched `*.seeding.*` unanchored, so it could collect any neighbour containing that substring once NUB_SHARED_TARGET points somewhere with siblings. Anchored to this script's own names. Verified: committed divergence now seeds, unstaged still seeds, no nesting, no claim left behind. * build: retire an abandoned seed claim even after the destination exists The retire block sat after `seed_from`'s destination-exists return, so it was unreachable on every run after the destination was published -- and `mkdir -p "$target"` publishes it on the very first run. An orphaned claim at the isolated call site was therefore permanent: it lives in the worktree root, which neither sweep scans, and showed up untracked in `git status` forever, against the clean-tree requirement for pushing. Hoisted above the return; a claim older than the window is abandoned by definition, so retiring it when the destination already exists is harmless. Added a .gitignore entry so a transient claim is never stageable either. Two comments had also gone stale against their own code: the header still said temps were swept by the GC below (true only for the bucket call site -- the private one is retired by the function itself), and the sweep comment still described `.seeding.<pid>` names collected on the same pass as the buckets, when the claim name is now fixed and it is a second pass on a much shorter window. Verified by reproducing the reported sequence: an aged claim planted at the isolated site is retired on the run after the destination exists, and git reports nothing untracked. * build: trim the seeding comments to the load-bearing why Six review rounds each added a paragraph justifying that round's fix, and they compounded: 129 comment lines against 44 lines of code, well past the sparse-and-dense bar this repo sets. Cut 45 lines of narration -- the retired premise, the content-key rationale, the mtime finding and the claim-ordering constraint all stay, since each records a decision a reader would otherwise re-litigate. No code changed; every wrapper branch re-verified, including the committed-divergence seed. * build: honor an explicit NUB_SHARED_TARGET verbatim, without a content key Content-keying broke `make verify`. Verify passes NUB_SHARED_TARGET="$(CURDIR)/target" to get a repo-local dir isolated from the shared cache, and keying silently redirected that to target-<hash> while the addon recipe still copied from target/<profile>/ -- so the copy missed and verify aborted at `@test -s runtime/addons/nub-native.node`. The keyed dir was matched by no .gitignore rule either, so the tree went dirty. An explicit NUB_SHARED_TARGET is now used exactly as given. Keying exists to make SHARING sound -- same path implies same content across the worktrees that converge on the one default cache -- and a caller naming a specific path is not sharing. Silently redirecting it breaks every consumer that knows where its artifacts land, of which the Makefile has three (TARGET, addon, addon-fast); honoring the name fixes all of them at once rather than teaching each to resolve through --print-target. The default path is still keyed. Neither PR could surface this alone: the Makefile changes were verified in a worktree without keying, the keying in a worktree that never ran `make verify`, and the interaction only became reachable once #592 merged and this branch synced. `make verify` is not a CI job, so the 30 green checks would not have caught it. Verified: verify's invocation resolves to .../target with no suffix, the default is still content-keyed, committed-divergence seeding still fires (re-tested via a HOME override, since the env var no longer exercises the keyed path), and the real addon build lands 13041296 bytes where verify asserts. * build: don't seed from an in-flight claim, and let the winner publish Two findings I missed on an earlier round (I truncated the comment fetch and dropped them), plus the doc fallout of the last fix. newest_bucket could seed from a half-copied tree. `ls -dt "$shared"-*` matches a claim dir as well as real buckets, and -t sorts newest-first, so an actively filling `$shared-<hash>.seeding` was the MOST likely pick -- pairing a fresh-looking fingerprint with a truncated rlib, the exact failure this design exists to prevent. Filtered out. A losing racer defeated seeding for everyone. Its mkdir fails, it returns 1, and the caller's unconditional `mkdir -p "$target"` then creates the destination empty -- so the winner's publish guard failed and it discarded a completed clone, leaving BOTH cold, worse than no claim at all. The winner now clears an empty placeholder with rmdir, which by definition cannot touch a destination something is already building in. The NUB_SHARED_TARGET carve-out also un-keyed a knob the rust-build skill still advertised as a plain relocation switch, which would put N worktrees on one unkeyed multi-tenant dir -- the phantom-E0063 clobber this PR exists to remove. Documented as per-checkout-private, with the multi-worktree recipe (move ~/.cache/nub itself) called out. Verified: newest_bucket skips an in-flight claim; the winner publishes over an empty placeholder and leaves a non-empty destination alone.
Contributor
Author
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The dev loop uses
--profile fast;make verifyand CI's check/clippy useddev, sharing nothing — crossing between them rebuilt 153/153 units, and the host held 12 GB oftarget/fastbeside 14 GB oftarget/debug.fastinheritsdev, differing only in debuginfo.nub-native) move to--profile fast, in CI andmake verify.nub-phantomcheck stay ondev: the former reuse debug artifacts by path, the latter defines nofastprofile.PROFILE:CARGO_FLAGSwas empty for non-release profiles, soPROFILE=fastbuilt intotarget/debugwhile the copy readtarget/fast.