build: cap the host at two compiling builds over six rustc tokens - #808
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Important
NUB_BUILD_FG=1 no longer opts a build out of the global governor, but this PR's doc edit says it does. A latency-sensitive foreground build's workspace crates now queue behind the agent fleet for up to NUB_BUILD_WAIT (3600s).
Reviewed changes — a new machine-wide build-slot layer in the global cargo rustc-wrapper, plus the status/installer/doc changes around it.
- Build slots in
rustc-qos.sh— at mostNUB_BUILD_SLOTS(default 1) cargo invocations may be compiling at once host-wide, FIFO. State is a directory tree under~/.cache/nub/build-sem(slot/<n>/{pid,stamp,active/<pid>},queue/<cargo-pid>), withmkdiras the atomic claim and a rename-then-rm -rfretire so two racing waiters cannot double-free. - Build identity via process-ancestry walk — one
pssnapshot plus an awk walk picks the outermostcargoancestor, so nested cargo inherits rather than deadlocks; arust-analyzerancestor exempts the build. Cached per parent pid, keyed on the parent's start time to defeat pid recycling. - Idle reclaim — a slot is reclaimable once no compile is live and none has started for
NUB_BUILD_IDLE(90s), socargo testrunning tests or a cargo blocked on a target-dir lock does not hold the machine. qos-global.shwrite-then-rename — replacescpover the live wrapper, which would have corrupted every wrapper mid-read machine-wide.build-status.shslots section — shows the holder, per-slot live compile count, and the queue.- Skill docs + Makefile comment rewritten to describe three jobs instead of two.
I verified the ancestry walk myself against known-answer cases (no-cargo → fail-open, cargo → its pid, rust-analyzer→cargo → exempt, nested → outermost) and it is correct on both ps -o comm= shapes. I also confirmed empirically on sh, bash, dash and bash --posix that the new trailing [ -n "$_bslot" ] && rm -f … in the EXIT trap does not clobber rustc's exit status through exit $? — that one is clean.
⚠️ A concurrency state machine with no committed test
The PR body cites "a fake-cargo harness (8 scenarios)" that verified retire/reap/queue behavior, but the harness is not in the diff. This is precisely the code where a throwaway probe is worth the least: it is a lock-free protocol over a directory tree, its failure mode is a host-wide stall rather than a wrong answer, and it is exec'd hundreds of times per build in front of every compile on the machine. tests/ already holds ~35 per-feature harnesses and AGENTS.md asks for exactly this promotion; nothing in CI or .githooks/ currently lints or exercises scripts/*.sh at all.
Technical details
# Promote the fake-cargo harness into the tree
## Affected sites
- PR description — describes 8 verified scenarios; none are in the diff.
- `scripts/rustc-qos.sh:105-277` — new lock-free protocol, no test coverage.
- `tests/` — ~35 sibling harnesses (`tests/pnp/`, `tests/launcher/`, …) establish the convention.
## Required outcome
- The 8 scenarios run reproducibly from the repo, without a 10-core mac or a real cargo.
- The harness proves it can FAIL: at minimum a positive control showing a slot is
genuinely withheld, so a future refactor that silently disables the layer (e.g. the
ancestry walk returning empty and failing open) goes red instead of green.
## Suggested approach (optional)
- `tests/build-slots/` with a `run.sh`: a fake `cargo` shell script on `PATH` that does
NOT `exec` (an `exec`ing parent is not an ancestor and the walk correctly finds
nothing — this silently turns the whole layer off), a dummy `rustc`, and
`NUB_BUILD_SEM_DIR` / `NUB_BUILD_IDLE` / `NUB_BUILD_WAIT` set small so the matrix runs
in seconds. It is POSIX sh, so it runs on the Linux CI legs, not just darwin.
- Worth covering explicitly: FIFO order across three builds; idle reclaim then re-queue;
holder SIGKILLed mid-compile; a queued wrapper killed while its cargo survives.
## Open questions for the human
- Is a `shellcheck` gate over `scripts/*.sh` worth adding at the same time? There is none
today, though several scripts already carry `# shellcheck disable` pragmas.ℹ️ The cap does not hold until every host re-runs qos-global, and build-status reports those builds as governed
A build running through a pre-PR ~/.cargo/rustc-qos.sh creates no slot dir and takes no ticket, so it compiles freely alongside the slot holder — the serialization silently does not hold. That is expected during rollout, but build-status.sh's per-cargo verdict only distinguishes a blanked RUSTC_WRAPPER (partial (workspace)) from everything else (governed), so a stale-wrapper build is labelled governed while being outside the new layer entirely. Given qos-global.sh:23-32 records that 58 of 61 live worktrees carry a stale installer that cps the old wrapper back, this state is likely rather than hypothetical.
Technical details
# Make a stale wrapper visible
## Affected sites
- `scripts/build-status.sh:78-81` — `case "$w"` marks anything that is not an explicitly
empty `RUSTC_WRAPPER` as `governed`, with no notion of wrapper vintage.
- `scripts/qos-global.sh:23-32` — documents that stale worktrees re-install the old wrapper.
## Required outcome
- `make build-status` distinguishes "governed by a wrapper that implements build slots"
from "governed by an older wrapper that does not", so the first question after an
unexplained load spike has an answer.
## Suggested approach (optional)
- Stamp a version into the wrapper (a `# rustc-qos-version: N` line) and have
`build-status.sh` grep the installed copy, reporting `stale wrapper — run make
qos-global` when it is missing or older.
- A cheaper proxy: a cargo that has been alive for longer than a few seconds while
holding no slot and no queue ticket is outside the layer.ℹ️ Nitpicks
- The throughput justification is repeated in three files as an absolute — "Serialising builds costs no throughput (N builds sharing 10 cores take at least as long as N builds in series)". That holds only while each build saturates every core; real cargo builds have serialization points (dependency-graph bottlenecks, final link, single-crate codegen) where a sibling build would have used the idle cores. The memory argument for serializing is strong on its own, so consider softening to something like "costs little throughput" rather than stating a strict inequality that does not hold in general. Sites:
scripts/rustc-qos.sh:21-23,.claude/skills/rust-build-hygiene/SKILL.md:51. NUB_BUILD_SLOTS=0is the only genuine way to bypass the new layer, and it is documented only in therustc-qos.shheader — neither skill mentions it, so an agent or maintainer reading the skills has no escape hatch to reach for.
Two specialist investigations I dispatched — on whether an orphan queue/<cargo-pid> ticket can wedge the head of the queue until the 3600s ceiling, and on whether a recycled pid behind a stale active/<pid> marker can pin a slot — did not report back before this review was submitted. Neither was proven reachable, and neither is reflected in the findings above; both are livelock-shaped and worth settling before this is treated as load-bearing on the dev host.
Claude Opus | 𝕏
There was a problem hiding this comment.
Important
Two issues worth fixing before this ships. first/<cargo-pid> is never cleaned up on the happy path, so it grows without bound and a recycled pid inherits an ancient first-queued time and sits permanently at the head of the queue — reproduced. And three of the four numeric tunables are unvalidated, so a NUB_BUILD_WAIT=1h silently disables the fail-open ceiling this script's header promises — also reproduced, A/B.
All three threads from the previous review are addressed and resolved: the NUB_BUILD_FG slot exemption (rustc-qos.sh:177 + rust-build.sh:272), the build-status.sh ticket-read race, and the doc scoping in rust-build/SKILL.md. The body-level asks landed too — the harness is committed, and # rustc-qos-version: 2 plus the STALE WRAPPER check finally makes a stale installed copy visible. The two hardening fixes in this commit both hold up under test: the heartbeat prune does clear an orphaned ticket, and NUB_BUILD_MAXCOMPILE does stop a recycled pid from pinning a slot forever.
ℹ️ Informational
The harness has no runner. tests/build-slots/run.sh is referenced from no Makefile target, no CI workflow, and no README — 26 of 40 dirs under tests/ carry one; this one does not. Nothing in the repo lints or exercises scripts/*.sh at all, so the positive control that was asked for only runs when someone remembers to type the path. Relatedly, # rustc-qos-version: 2 is hand-maintained with nothing enforcing a bump, so the first semantics change that forgets it leaves every stale host reported as current — which is the exact failure the stamp was added to prevent.
Claude Opus | 𝕏
There was a problem hiding this comment.
Important
One regression to fix before this ships. The rewritten first/ read at rustc-qos.sh:280 moved its 2>/dev/null behind the input redirect, so every build now prints cannot open …/sem/first/<pid>: No such file to the wrapper's real stderr — 21 of them across 18 builds in one harness run, reproduced. Same class as the Illegal number: 5s spew this commit fixed.
Reviewed changes — 9b14b49 against the previously reviewed 4b9881b, 11 files, 3 commits.
All five prior threads are genuinely addressed, and I re-derived each rather than taking the diff's word for it: the three -ge 0 tunable fallbacks (rustc-qos.sh:132-134), the *.* mid-rename guard on _reap's queue loop, dead-pid GC for first/ plus an lstart line so a recycled pid cannot inherit a place, the NO SLOT (fg/stale?) heuristic removed, and the harness assertion flipped to -le 4 with NUB_BUILD_MAXCOMPILE unset alongside the others. Threads replied to and resolved.
I ran tests/build-slots/run.sh on a 4-core ubuntu runner: all 8 scenarios pass, 95s wall, rc=0. The new required gate is green and its timing margins hold here.
⚠️ The harness went green while the script errored on every single build
That same 95s run emitted 21 cannot open diagnostics and still printed build-slots: all scenarios passed. The scenarios assert only on the timeline they parse from the fake rustc; nothing inspects what the wrapper writes to stderr. Now that build-slots is in ci-gate's needs, that blind spot is the last thing between this defect class and main — and it is not hypothetical, since both stderr-noise bugs found on this PR (Illegal number: 5s last round, cannot open this round) were invisible to it.
Capturing the wrapper's stderr across the whole run and failing on anything the scenarios did not deliberately provoke would close it in one assertion, independent of the eight scenarios.
Two smaller wiring notes on the same job: it joins ci-gate with no path filter, so a docs-only PR pays the ~95s, and with no timeout-minutes, so a wedged scenario burns the job default instead of failing fast.
Claude Opus | 𝕏
There was a problem hiding this comment.
Important
The new path gate on the build-slots job skips it for the exact change class it guards. scripts/** is not in the rust paths-filter group, so a PR that touches only scripts/rustc-qos.sh runs neither the harness nor the version-stamp check, and ci-gate still goes green.
Reviewed changes — f946b93 against the previously reviewed 9b14b49.
- The
first/read no longer leaks to stderr —2>/dev/nullnow precedes the input redirect atrustc-qos.sh:283, and the same reordering was applied to the two sibling reads (_pcacheat:154, the ticket heartbeat at:237). No mis-ordered} <shape remains anywhere inscripts/*.sh. - Added a whole-run stderr assertion to the harness — every wrapper invocation appends its stderr to
$T/log/wrapper.err, and a scenario-independent check fails the run if that file is non-empty. - Rewired the FG tell in
build-status.sh—:102-106now treats a blankRUSTC_WORKSPACE_WRAPPER=paired with a blankRUSTC_WRAPPER=as therust-build.shforeground signature, keepingNUB_BUILD_FG=1for themake buildand bare-cargopaths. - Aligned the pre-push gate with CI —
.githooks/pre-push:217now always usesgit merge-base "$local_sha" origin/main, andNUB_SKIP_QOS_VERSION_CHECK=1is an escape hatch alongside the lat and site gates. - Bounded and path-gated the CI job —
timeout-minutes: 10, plusneeds: matrix-planand anif:onrun_rust. - Widened
serialize's pair-check bound from-le 4to-le 5.
I ran tests/build-slots/run.sh end to end on this branch: all 8 scenarios plus the new stderr assertion pass, rc=0, and wrapper.err is empty — the cannot open regression is genuinely gone rather than relocated. That run also measured last A start at exactly 4 in serialize, so the previous bound had zero margin; the widening is a justified tolerance change, not a masked failure. I separately confirmed the new capture has no hole: all six $W call sites in the harness carry the redirect, reset() never truncates wrapper.err, and nothing is still writing when the assertion reads it.
ℹ️ The now-required gate rests on absolute wall-clock bounds
build-slots sits in ci-gate's needs, so a timing miss on a contended runner paints the sole branch-protection check red. Most assertions are relations between logged events (at B start >= at A cargo-end) and are robust to a slow runner, but serialize's pair check and idle's gap check are absolute-second bounds — and serialize's measured exactly at its old limit this run, which is what forced the widening in this commit. Nothing is broken today; the exposure is that the next slow runner tightens the same screw again.
Technical details
# Absolute-second assertions on a required CI gate
## Affected sites
- `tests/build-slots/run.sh:87` — `[ "$(last A start)" -le 5 ]`. Measured 4 on a 4-core
ubuntu runner at `f946b93`; correct behavior is 3, over-throttled behavior is ~9.
- `tests/build-slots/run.sh:124` — `[ "$(last C start)" -le $(( $(at C start) + 7 )) ]`,
three 3s compiles under `NUB_BUILD_IDLE=2`.
## Required outcome
- The gate stays green on a runner slower than the one these bounds were tuned on,
without a future contributor widening the constant again each time.
## Suggested approach (optional)
- Where a relation expresses the same claim, prefer it: `serialize`'s "ran in pairs" is
really "the last start is closer to the first start than a serial run would allow",
which can be stated against `at A cargo-end` minus one compile duration rather than
against a literal.
- Where a literal is unavoidable, widen it toward the broken value's midpoint rather
than by one second, and put the correct/broken pair in a comment so the next person
can see how much slack is left.
## Open questions for the human
- Is `build-slots` intended to stay in `ci-gate`'s `needs` long-term, or was that a
rollout convenience? A timing-shaped harness is a different risk profile as a required
check than as a `make` target.ℹ️ Nitpicks
- The PR description says the slot is reclaimed after
NUB_BUILD_IDLE(90s); the default is 120s inrustc-qos.sh:126and in both skill docs. build-status.sh:88-91still justifies thepartial (workspace)label with "qos-global also registers the wrapper as rustc-workspace-wrapper, which those callers do not blank".rust-build.sh:272now blanks both keys, so that sentence no longer describes any in-tree caller — the label is reachable only from a stale checkout or a hand-rolled config.
Claude Opus | 𝕏
|
Your Claude subscription has hit its usage limit. It resets at 12:40am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
|
Your Claude subscription has hit its usage limit. It resets at 12:40am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
|
Your Claude subscription has hit its usage limit. It resets at 12:40am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
b2bcdea to
dad1ec4
Compare
|
Your Claude subscription has hit its usage limit. It resets at 12:40am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
There was a problem hiding this comment.
ℹ️ The torn-read defect this commit exists to fix is not covered by the harness. I reverted
_stamp/_markto the pre-fix in-place>writes and every slot-contention scenario stayed green. Nothing here is broken; the gap is that a future regression would land green through a required check.
Reviewed changes — 4c1b5ab against the previously reviewed dad1ec4.
- Write-then-rename for slot state —
_stamp/_mark(rustc-qos.sh:234-235) build a temp andmvit into place, so a reader can no longer land between a>'s truncate and its write.queue/andfirst/got the same treatment at:372-376. The dot-prefixed and.$$-suffixed temps are invisible to theactive/*glob and to the*.*case guards in_sweep,_head_of_queue,_say_queuedandbuild-status.sh:74— checked each. The three remaining plain>writes ($_mx/pid,slot/<n>/pid, the tokenpid) are all into a file that has just been created by a winningmkdir, so there is no truncate to tear. - Reader-side reordering —
_reapscansactive/*first and readsstamponly when it finds none live;_releasewrites the stamp first and drops its marker second. On a local filesystem one writer'srename()-then-unlink()is observed in that order, so "marker gone implies fresh stamp already there" holds — this closes the window rather than narrowing it. - Fallback directions — an unreadable marker age with a live pid counts as fresh (
:265), and_reapskips the whole poll whendatecould not fork (:251), instead of judging every age against an empty_tnow. _holdsplits its failures — 1 (lost the slot, keep waiting) vs 2 (table unwritable, fail open), and_releasenow stamps only a slot whosepidstill names our own cargo.NUB_RUSTC_SEM_HELD=1is exported unconditionally — I checked whether this can now let an inner workspace-wrapper hop skip a protocol it should have run. It cannot: the token block at:481-514runs regardless of slot exemption, so the outer shell always makes the definitive token decision before the export. The one path that skips everything (the-vV/--printprobe exec at:119) also skips the export, and the inner re-runs that same check independently. This is more correct than the old conditional, not less.- Harness —
fifo's B now compiles 2s so arrival order alone cannot satisfy the check; newdeadcargoscenario; three newidleassertions;kill10→12,serialize5→6,idle+7→+9 and +14→+15.
I ran the harness on a 4-core ubuntu runner and mutation-checked every assertion added or changed in this commit:
| assertion | measured | bound | mutation | result |
|---|---|---|---|---|
deadcargo — Q ran before A ended |
Q t+3, A end t+8 | relation | drop the kill -0 "$_cargo" guard at :422 |
red — Q never starts |
idle — A's re-queue took the slot promptly |
last A start 11 |
14 | make _ticket forget the persisted first/ time |
red — 43 |
idle — D never ran before C's cargo exited |
34 vs 34 | relation | same | red |
idle — A's second compile ran before D |
11 < 34 | relation | same | green (weaker) |
fifo, kill, serialize, twoslots, orphan and nested also pass, and wrapper.err is empty across every run. The version stamp is at 6 and consistent between rustc-qos.sh:2, build-status.sh:90 and the check script; defaults are NUB_BUILD_SLOTS=2 / NUB_RUSTC_LIMIT=6 / NUB_BUILD_IDLE=120, matching the title and body. The prior round's build-status.sh wording nitpick is gone.
ℹ️ The harness cannot fail on the bug this commit fixes
The headline change is the torn-read fix — an empty stamp read as "stampless" retired a live slot, reproduced at 30% per the commit. I reverted _stamp and _mark to their pre-fix in-place > form and ran the six scenarios that actually contend for a slot:
idle ok (x2) serialize ok orphan ok
deadcargo ok twoslots ok nested ok
build-slots: all scenarios passed
The reader-side defenses are unexercised for the same reason: no scenario ever produces an empty stamp or an empty active/<pid>, so flipping the :265 fallback direction or undoing _reap's marker-first ordering would also land green. build-slots is in ci-gate's needs, so this is the class of regression that reaches main unannounced.
A race is hard to schedule, but the state it produces is trivial to plant, and NUB_BUILD_SEM_DIR already points at the harness's own tree (run.sh:69). A scenario that lets A take slot/1, then does : > $T/sem/slot/1/stamp and : > $T/sem/slot/1/active/<pid> while A is mid-compile, and asserts B does not steal the slot, is deterministic and needs no timing at all. It would go red under either the pre-fix reader ordering or a flipped fallback.
ℹ️ Nitpicks
- This commit widens four absolute-second bounds (
run.sh:965→6,:11710→12,:143+7→+9,:146+14→+15) and adds a fifth at:149. I measured real slack in all of them on this runner (3–8s) and mutation-confirmed they still go red, so nothing is masked — but this is the second consecutive commit turning the same screw on a required check, which is the open question from the last round rather than a new finding. - The PR body says "12 scenarios"; there are 13.
Claude Opus | 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Incremental review of the three commits since the last Pullfrog review at 4c1b5ab.
Reviewed changes
- One validated clock reading per poll (
rustc-qos.sh:413-429)._tnowis read and range-checked once at the top of the loop, then reused by_reap,_ticket,_say_queued, the claim path and the wait ceiling. I traced every read and write of_tnow: only two writes exist (:423and_release's own at:565), both validated before use, and every helper that consumes it is reachable only from the validated loop or recomputes its own. The redundant internal guards in_reapare correctly gone. _holdmarker leak fixed (:342-357) — retires the open thread from the last round. The marker is dropped on both failure branches, and fail-open is gated on the slot'spidstill naming this build.Broken pipesuppression on_now()and the harness's fakerustc, withbuild.sh's owndatedeliberately left loud since its stderr does not feedwrapper.err. Consistent with the whole-run stderr assertion.- New
tornscenario (run.sh:199-211) — the first coverage for the reader-side torn-read defenses. Mutation-verified live on this run: blanking the marker's write-time fallback atrustc-qos.sh:265(_w=$_tnow→_w=0) turns it red (B starts at t+2 instead of t+8). Clean run passes withwrapper.errempty. idlerestructured around the newbuildpre-compile-delay argument, trading two absolute-second bounds for one relational place-keeping check (run.sh:157). Mutation-verified: making_ticketforget the persistedfirst/time fails three of its assertions. Measured slack on a 4-core ubuntu runner is 3–5s on each remaining bound. This is the first commit in three that reduces the count of absolute-second assertions on the required gate — the right direction.- Version stamp 6 → 8; PR body's scenario count now matches the harness's 14.
Note, not a finding
torn plants the post-torn state rather than producing it, so the write-side _stamp/_mark write-then-rename fix is still uncovered by construction. That is inherent to the remedy suggested last round, not a gap introduced here — the reader-side defenses are what became testable, and they now are.
Claude Opus | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Incremental review of the two commits since the last Pullfrog review at 2e621e8. Both of that review's findings are fixed, and the delta introduces nothing new.
Reviewed changes
_noclockis now consecutive (rustc-qos.sh:430)._noclock=0after a validated reading, exactly as suggested. The counter now means what its comment says — a minute of consecutive clock failures, not a lifetime tally that intermittentdatemisses could walk to 60 over a long queue wait and silently drop the slot cap.tornhas a positive control (run.sh:206,211). The plant counts the markers it truncates and assertsn > 0. Mutation-verified live on this run: pointing the glob atslot/9makes it a no-op and the new assertion goes red (FAIL A held slot 1 with a live marker to truncate, rc=1). It is a real gate, not a green-either-way line.check()evals in the current shell sonis in scope, andset -umakes an unset one fail rather than pass — the failure direction is safe. Clean run passes withwrapper.errempty.2>/dev/nullonunameand the ancestor-walkawk(:124,:211), completing the invariant the rewritten comment claims. I checked the claim rather than taking it: every$(in the file is either arithmetic$(( … ))or one ofuname:124,ps -o lstart=:195/:367, theps | awkpipeline:201-211,find:283, and$(_now):424/:567— the last silenced inside_nowitself. All six silence their stderr, so "EVERY command substitution" holds as written. This is what keeps the harness's whole-run stderr assertion meaningful when a wrapper is SIGKILLed mid-substitution.- Version stamp 8 → 10, one bump per commit. Both consumers
sed-extract it (build-status.sh:90,rustc-qos-version-check.sh:19-29), so nothing hardcodes the number and no companion edit is owed.
Still uncovered, not a blocker: the _noclock fail-open branch itself has no scenario — it is now the one path in the wait loop with no test, and covering it would need clock-failure injection into the harness's fake date. Worth a follow-up rather than this PR.
Claude Opus | 𝕏
a5e5dda to
e56d815
Compare
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
One commit since the last review (a5e5dda): e56d815, touching tests/build-slots/run.sh only (+16 lines). It adds a noclock scenario that starves the wrapper of a clock by putting a failing date first on PATH, and switches the fake cargo/rustc to /bin/date so the event log the assertions read stays populated. That /bin/date split is the right call and worth the comment it carries.
I ran the harness rather than reasoning about it. noclock passes in 61s (t+0s N cargo-start; t+59s N start; t+60s N end), stderr clean. Mutating the threshold [ "$_noclock" -ge 60 ] → -ge 6 at scripts/rustc-qos.sh:431 turns it red (t+5s N start, rc=1), so the scenario is a genuine gate and not theatre.
I also timed the full suite, since run.sh:12 now claims "~5 min" against a timeout-minutes: 10 required check. Measured 3m40s on a 4-core ubuntu runner across four batches (34.5s + 76.4s + 46.8s + 61.3s), all 15 scenarios green. ~2.7x margin, so the timeout is fine — but see the nitpick about where that estimate is recorded.
One coverage gap noted inline, non-blocking.
ℹ️ Nitpicks
run.sh:221and:223say "measured ~70s" and "(~60-70s)", but 70s isn't reachable: the 60th poll breaks before sleeping, so there are exactly 59 sleeps by construction and the run measures 59s. The-ge 55lower bound therefore has 4s of slack against a hard floor rather than the ~15s the comment implies. Worth restating as "59 sleeps, measured 59s" so a future reader doesn't loosen the bound on the strength of a wrong number.- The harness runtime is stated in three places and only one moved.
run.sh:12now says "~5 min";Makefile:122-124still says "~3 min" and.github/workflows/ci.yml:1384-1385still says "~2 min". Measured 3m40s.
Claude Opus | 𝕏
705cf95 to
79b9eb1
Compare
There was a problem hiding this comment.
✅ No new issues found.
Incremental review of the one commit since the prior Pullfrog review at e56d815: 79b9eb1, touching .github/workflows/ci.yml, scripts/rustc-qos.sh and tests/build-slots/run.sh. It closes last round's coverage finding exactly as suggested, and both nitpicks with it.
Reviewed changes
- Made the clock-miss threshold a tunable.
NUB_BUILD_NOCLOCK_MAX(default 60) atrustc-qos.sh:156, consumed at:431, listed in the header's tunable block, andunsetalongside its siblings atrun.sh:78so a host value cannot leak into the harness. Its validation is-ge 1rather than the siblings'-ge 0, which is the right call — a0threshold would fail open on the very first miss and silently disable the layer. Version stamp 10 → 11. noclocknow pins the CONSECUTIVE semantics. The scenario splits in two: a permanently-failingdatewithNUB_BUILD_NOCLOCK_MAX=5for the threshold, plus a newflipclockdatethat fails every other call while A holds the slot for 12s — so cumulative counting would trip the valve around poll 5 while consecutive counting never gets past one.- Reconciled the runtime estimates.
run.sh:12andci.yml:1384now both say "~3 min", matchingMakefile:122-124.
I ran the scenario rather than reasoning about it. Clean: t+4s N start for the threshold case, and t+13s B start after A's t+13s end for the consecutive case — rc=0, wrapper.err empty, 20.3s where the old scenario took 61.3s. That puts the suite near 3 min, so the three runtime comments are now accurate rather than merely consistent.
Mutation-verified: reverting _noclock=0 at rustc-qos.sh:434 to a no-op — the exact revert that stayed green last round — now turns the second case red (t+5s B start, B failing open onto A's live compile; 2 assertions red, rc=2). Restored and confirmed with diff -q. The gap is genuinely closed, not reworded.
Two things I checked rather than assumed. The -ge 4 lower bound has no nominal slack but cannot flake low: four sleep 1s make elapsed ≥ 4.0s, so the integer-second difference is ≥ 4 by construction — unlike the old -ge 55 sitting one nudge under a 59s floor. And [ "$(cat "$T/flipclock/n")" -ge 6 ] is a genuine positive control, not decoration: without it, a PATH prefix that failed to reach the wrapper would make "B waited for A" trivially green.
Claude Opus | 𝕏
The machine-global rustc wrapper capped how many rustc PROCESSES exist, and nothing else. Each still ran its own cargo's `jobs` worth of LLVM threads (10 rustc x 7 threads on 10 cores, measured 2026-08-28) and ten concurrent compiles of the big crates peaked at 1-3 GiB apiece on a host already carrying ~40 GiB of editors, browsers and agent sessions. Add a build-level layer in front of the rustc tokens: at most NUB_BUILD_SLOTS (default 1) cargo invocations may be compiling at once, machine-wide, served first-come first-served. A slot belongs to the OUTERMOST cargo in a rustc's ancestry, so a nested cargo inherits rather than deadlocks; it is reclaimed when the holder dies, or when no compile of its build is alive and none has started for NUB_BUILD_IDLE (90s), so `cargo test` running its tests or a cargo blocked on a target lock does not hold the machine. rust-analyzer is exempt. Reclaim is an atomic rename so two waiters cannot wipe a re-created slot; the ancestry walk is cached per parent pid, validated by start time, so the owned fast path costs ~10ms per rustc. Fail-open on every path, as before. qos-global now installs by write-then-rename: sh reads a script incrementally, so `cp` over the live wrapper would feed new bytes to every compile in flight. build-status shows the slot holder and the queue.
Review findings on the first cut, plus one defect seen live: - A build that lost its slot in a build-script gap re-queued at the back of the line (waited 380s more for its last crate). A ticket now carries the time the build FIRST queued, so a re-queue keeps its place. - The slot stamp was written only at compile start, so any compile longer than the idle window left its build looking idle the instant it ended, and a waiter polling the gap before the next rustc could take the slot from a build in full flight. The wrapper now stays alive around every slotted compile and stamps at end as well; idle default 90s -> 120s. - Two livelock shapes closed: a ticket whose waiters all died is ignored once its heartbeat is 15s stale; a live-compile marker records its write time and stops counting after NUB_BUILD_MAXCOMPILE (30 min), so a recycled pid cannot pin a slot. - NUB_BUILD_FG=1 is the human's foreground escape and now exempts the slot queue too; rust-build.sh blanks both wrapper keys under it. Skills say so, and document NUB_BUILD_SLOTS=0 / =2. - build-status reads a ticket once (the owner truncates and removes it), flags an installed wrapper older than the tree's (version stamp), and marks a long-lived cargo holding neither slot nor ticket. - tests/build-slots/run.sh: eight scenarios with assertions, driven by a fake cargo (a C shim compiled at run time so it is an ancestor NAMED cargo) and a fake rustc, in a private NUB_BUILD_SEM_DIR.
…ss in CI Second review round on the build-slot layer: - first/<cargo-pid> was never collected on the happy path, so the directory grew without bound and a recycled pid inherited an ancient first-queued time — permanently head of the queue. Collected in _reap by the death of its cargo (never on the happy path, which would forfeit a re-queued build's place), and the record now carries the cargo's start time so a recycled pid cannot inherit it, the same guard parent/ has. - NUB_BUILD_IDLE / NUB_BUILD_WAIT / NUB_BUILD_MAXCOMPILE fall back to their defaults when non-numeric; a bad NUB_BUILD_WAIT previously disabled the fail-open ceiling and spewed `[: Illegal number` once a second. - _reap skips a ticket mid-rename, as _head_of_queue already did. - build-status drops the "NO SLOT" heuristic (an idle-reclaimed build holds neither slot nor ticket by design) and marks a NUB_BUILD_FG=1 build. - Harness: the pairing assertion now fails on over-throttling; NUB_BUILD_MAXCOMPILE no longer leaks in from the shell. - The harness gets a runner: `make test-build-slots` and a `build-slots` CI job in the gate. scripts/rustc-qos-version-check.sh refuses a governor change that leaves `# rustc-qos-version:` unbumped, from both the pre-push hook (which now reads its ref lines once for both range loops) and that CI job. Stamp bumped to 3 for the first/ record format.
…tderr The rewritten first/ read put `2>/dev/null` after the input redirect; redirections apply left to right, so the (usually absent) file was opened with stderr still live and every build printed `cannot open …` into its cargo output. Same order fixed on the parent-cache and heartbeat reads. The harness now captures the wrapper's stderr across the whole run and fails on any line — both stderr-noise bugs found on this PR were invisible to the timeline assertions, and this one sees them (verified red with the regression re-introduced). The pairing assertion gets a second of slack so a loaded host cannot flake it. build-status recognises a rust-build.sh foreground build by both wrapper keys being blank (that path unsets NUB_BUILD_FG before exec). The pre-push version gate compares against the merge-base with trunk like CI, so it wants one bump per pull request rather than one per push, and takes NUB_SKIP_QOS_VERSION_CHECK=1. The CI job is path-gated with the Rust jobs and carries a 10-minute timeout.
…accident With a 2s idle window and ~0s gaps between the holder's compiles, a loaded host could stretch one gap past the window and a waiter would take the slot by design; the assertion read that as an interruption. The window is now 4s and the tolerance +8, which a real steal (a whole extra compile) still exceeds. Three consecutive runs green at load 75.
…s relational The build-slots job was path-gated on run_rust alone, and scripts/** lives in the `scripts` filter group — so a change touching only the governor ran neither the harness nor the version-stamp check while ci-gate went green. Gate on either group, like docs-links. The two wall-clock literals in the harness (pair check, idle gap check) are now stated relative to the run's own events, with the correct and broken values in a comment, so a slower runner has real slack and the next person can see how much. The job stays a required check on purpose: the harness is what makes a stall-shaped protocol change red before it lands. build-status: the `partial (workspace)` label is now documented as the mark of a stale checkout or hand-rolled config — no in-tree caller produces it.
…bling claims Strict one-build-at-a-time idled nine cores behind a single starved compile while eight builds queued 25 minutes (maintainer's call 2026-08-28): the default is now NUB_BUILD_SLOTS=2, with NUB_RUSTC_LIMIT dropping from ncpu to 6 so two slots cannot re-create the ten-rustc memory peak the slots exist to prevent. NUB_BUILD_SLOTS=1 restores strict serialization. Enabling the second slot surfaced a race one slot could never hit: two parallel first compiles of one build share a ticket, so both are head of queue at once, and each claimed a different slot — the build held both and starved everyone, and the yield guard cannot close it because neither sibling's pid is visible yet. Claims are now serialized per build through a claim mutex (dead-holder reclaimed); the lowest-owned-slot yield stays as backstop. The twoslots scenario asserts a second build overlaps the first; the mechanism scenarios pin NUB_BUILD_SLOTS=1. Stamp v4.
Under set -u the heading died with 'bslots: unbound variable', taking the whole slots section with it.
…place, harden the harness A five-lens review of the build-slot layer found, and the harness's mutation runs confirmed, gaps the earlier rounds had not reached: - `_hold` re-created a slot a sibling had just retired (`mkdir -p`), leaving a pid-less slot with a live marker that nothing could retire or claim; it also attached to a slot another build re-claimed in the same gap. It now never creates, and re-checks the pid after writing. - A slot with no stamp (a claimer killed or out of disk between mkdir and its first write) was skipped by every reaper forever. A dead holder is retired regardless of stamp, and a stampless slot once it is a minute old. - Fail-open was only checked at entry: a wiped or unwritable state dir, a cargo that died while its rustc queued, or a failed ticket/claim write each meant an hour per rustc. Each now exits the wait in place. `NUB_BUILD_SEM_DIR/off` is a host-wide off switch checked every iteration (`make build-slots-off/on`). - The body is one compound command: sh reads a script incrementally, and every other checkout's installer still copies over the live file, which ended a running wrapper at the new EOF with exit 0 — cargo recorded units no rustc produced. - Numbers read from the table are validated (a corrupt file was a fatal arithmetic error), every tunable takes its default when non-numeric, temp names and claim mutexes are swept by writer death, token reclaim renames before removing, the inner wrapper inherits a token fail-open instead of waiting again, and a build queued 20s says so once on cargo's stderr. Harness: the idle scenario's holder now compiles longer than the window and the late build queues before the re-queue, so deleting the end-stamp, the live marker or the place-keeping turns it red (none did before); the nested assertion can fail; new scenarios cover the token limit, exit-status propagation and the off switch; a missing C compiler fails under CI. The CI job also runs on macos-14, the platform the governor is installed on. The version check fails on a bad rev instead of passing. build-status tags rows with their worktree, shows the off switch, dead and stampless slots, and counts partial builds; skills and comments no longer describe strict one-at-a-time. Stamp v5.
…harden the idle scenario Round two of the review, driven by mutation runs and forced interleavings: - `_release` dropped its marker before refreshing the stamp while `_reap` read the stamp before scanning markers, so a waiter polling in the same second a compile ended retired the slot (seen under load: D took C's slot at t+20, the second C's compile ended). Stamp first now; the reaper reads the stamp only after finding no live marker. - A stamp rewritten in place reads as EMPTY between truncate and write (30% of reads in a tight loop here), and the new stampless rule then retired a live slot; an empty marker read the same way was deleted as expired. Both are written by rename now, an unreadable marker with a live pid counts as live, and the stampless check runs only once no live marker remains. - `_hold` reported a slot retired under it as "table unwritable" and failed open 1.8% of the time under a retire loop; it now keeps waiting unless the directory is really gone. `_release` refreshes only a slot that is still this build's. A `date` that cannot fork skips the poll instead of judging every age against an empty number. The remaining `> file 2>/dev/null` writes are brace-grouped. A claim mutex with no pid and a dead cargo is reclaimed. The re-entrancy mark is exported on every path. Harness: the idle scenario's holder compiles 5s on re-queue and two relational checks were added — D never runs before C's cargo exits (a reaper that ignores live markers, or a `_hold` that skips its pid check, let D steal inside the old bound) and A's re-queue starts promptly after B releases (a lost place was a coin flip on the pid tiebreak). `fifo`'s B compiles 2s so arrival order alone cannot satisfy it. New `deadcargo` scenario: a wrapper whose cargo is killed while queued fails open at once. Stamp v6.
…`Broken pipe` The macos-14 CI leg failed the harness's stderr assertion with `date: stdout: Broken pipe`: a wrapper SIGKILLed mid-`$(date +%s)` (the kill and orphan scenarios) leaves `date` writing to a closed pipe, and under a parent that ignores SIGPIPE — the Actions runner's Node, or any agent harness — that is an EPIPE message on cargo's stderr instead of a silent death. Reproduced locally by running the harness under `node`; green with `date`'s stderr dropped in `_now` and in the harness's fake rustc, which shares the wrapper's stderr. Stamp v7.
…s place-keeping by pid order Round three of the review verified the rename-based writes (0 torn reads in 20,000 versus 1,332 with in-place writes) and left four small items: - `_t0` and the claim path could still see an empty `date` result. The wait loop now reads the clock once per iteration, skips the poll when it is empty, and fails open after a minute of empties; every age in the loop — reap, claim, notice, the wait ceiling — judges against that one reading. - `_hold` on a slot re-created by another build mid-claim returned "table unwritable" (fail-open) and could leave its own marker behind; it now drops the marker and fails open only when the slot is still this build's. - The stampless-slot comment claimed writes never touch the dir's mtime; a rename into it does. Conservative either way; the comment says so now. - The harness's "A's re-queue took the slot promptly after B" check was a false red under phase drift: FCFS legitimately lets C go first when A re-queues after B's release. The idle scenario now launches C first (lowest pid, first compile delayed 2s) and has A re-queue while B still holds, so at B's release the line is A, C, D by first-queued time and C by pid; a re-queue that loses its place is red 3/3 on "A's second compile ran before C's first", and a correct wrapper is green with several seconds of margin. Stamp v8.
…es can go red The torn-read fix (stamps and markers written by rename; an unreadable marker with a live pid counts as live; the stamp read only after the marker scan) had no scenario that failed when it was reverted, because no run ever produced an empty stamp or marker. The race is hard to schedule; its state is trivial to plant: while A compiles, truncate its stamp and its live marker and age the slot dir past the stampless rule, then assert B does not take the slot. Green on the tree; red with the marker fallback flipped to "dead" (B steals at t+2).
The macos-14 leg failed the harness's stderr gate again, this time with `awk: i/o error occurred while closing /dev/stdout` from the ancestry walk of a SIGKILLed wrapper — the same EPIPE-under-an-ignored-SIGPIPE class as the `date` line, and a matter of timing which command the kill lands in. Every `$(…)` in the script now drops the child's stderr (`awk`, `uname`; `date`, `ps`, `find` already did), and the header states the rule. Kill, orphan and deadcargo ran five times under a Node parent with a clean stderr; full harness green. Stamp v9.
… plant landed `_noclock` was never reset after a good reading, so intermittent `date` failures on a fork-starved host accumulated over the whole queue wait and abandoned it after 60 misses total, far short of NUB_BUILD_WAIT — during exactly the overload the cap exists for. It is reset on every valid read. The torn scenario now asserts it truncated at least one live marker, so a change that moves A off slot 1 cannot leave it green while planting nothing. Stamp v10.
Fakes call /bin/date so a failing `date` first on PATH starves only the wrapper. A new `noclock` scenario asserts the build compiles after ~60 consecutive misses, not at the wait ceiling. Widens the header's runtime estimate to ~5 min.
…pins CONSECUTIVE counting NUB_BUILD_NOCLOCK_MAX (default 60, validated like its siblings) replaces the literal. Under a permanently failing clock, consecutive and cumulative counting reach the threshold on the same poll, so the scenario could not fail on the reset it was written for. It now runs two cases at a small threshold: a threshold case (four sleeps by construction, not the wait ceiling) and a holder-plus-alternating-clock case where cumulative counting would fail the waiter open onto the holder's compile. Mutation-checked: deleting the reset turns the second case red at t+6. Making the valve unreachable HANGS the first case rather than failing it — a clockless wrapper has no other exit, since the NUB_BUILD_WAIT ceiling itself needs a clock reading to fire; the CI job timeout is what catches that shape. Takes ~55s off the CI gate; the runtime estimate is ~3 min on a runner everywhere it is stated.
79b9eb1 to
d0c97cf
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — every commit SHA moved, but the delta since the last review (79b9eb1) is a pure rebase onto main's a247a22 (self-pruning target-dir GC). Net PR content change is zero: the tree diff 79b9eb1..d0c97cf touches only 4 of the PR's 13 files, and a path-scoped diff shows those 4 changed only by main's content.
Byte-identical to the reviewed revision: scripts/rustc-qos.sh (still # rustc-qos-version: 11), tests/build-slots/run.sh, scripts/build-status.sh, scripts/qos-global.sh, scripts/rustc-qos-version-check.sh, and all four SKILL.md files.
What I checked on the rebased tree, since a rebase is where hunks get silently dropped and where two new subsystems can collide:
| Check | Result |
|---|---|
| PR hunks survived | wrapper_off in rust-build.sh, the pre-push qos-version gate, build-slots-off/-on + test-build-slots + verify in the Makefile — all present |
| main's hunks survived | .PHONY keeps target-gc test-target-gc; main's Makefile targets and @tests/target-gc/run.sh in verify appear as unchanged context |
ci-gate needs |
carries both target-gc and build-slots; ci.yml parses, 17 jobs, identical run_rust || run_scripts gate on each |
| GC vs. slot state | target-gc.sh candidates are strictly shared-target / shared-target-* plus worktree target/ dirs — ~/.cache/nub/build-sem is never collected |
| GC probes vs. the wrapper | rust-build.sh --print-target exits at :223, before both the GC hook and the slot logic, so the collector's up-to-40-worktree probes cannot claim a slot or recurse |
| Harness on the rebased tree | tests/build-slots/run.sh serialize fifo kill nested → all scenarios passed, rc=0, wrapper stderr clean |
The prior thread on the noclock scenario is resolved and its fix (79b9eb1) is carried through unchanged.
Claude Opus | 𝕏

The rustc-count cap still let 10 rustc run 7 LLVM threads each at 1-3 GiB apiece. This adds a build-level layer: at most
NUB_BUILD_SLOTS(default 2) cargo invocations compile at once machine-wide, FIFO, sharingNUB_RUSTC_LIMIT(default 6) rustc tokens. Slot = outermost cargo ancestor; reclaimed when the holder dies or idlesNUB_BUILD_IDLE(120s), place kept on re-queue; rust-analyzer exempt; fail-open in place (wiped state dir, dead cargo,make build-slots-off); one compound command, so an older installer's copy cannot truncate a running wrapper; a build queued 20s says so once.qos-globalinstalls by rename;build-statusshows holders, queue, stale installs.Fake-cargo harness (15 scenarios, mutation-checked, ubuntu + macOS CI) and live on the host.