diff --git a/.cargo-husky/hooks/pre-push b/.cargo-husky/hooks/pre-push index 60da928f..bdcc15e1 100755 --- a/.cargo-husky/hooks/pre-push +++ b/.cargo-husky/hooks/pre-push @@ -32,6 +32,11 @@ if [ "${GATE_SKIP:-0}" = "1" ]; then exit 0 fi +# Queue behind any other heavy build on this machine, in any project, rather than +# race it for RAM. After the escape hatch, so a deliberate skip never waits. +# shellcheck source=scripts/lib/host-build-lock.sh +source "$(git rev-parse --show-toplevel)/scripts/lib/host-build-lock.sh" "pre-push" + cargo fmt --all -- --check # Crates owning the changed files. Falls back to the whole workspace when the upstream ref is diff --git a/docs/dev/reviews/review-1440-host-build-lock.md b/docs/dev/reviews/review-1440-host-build-lock.md new file mode 100644 index 00000000..25ec31fb --- /dev/null +++ b/docs/dev/reviews/review-1440-host-build-lock.md @@ -0,0 +1,77 @@ +--- +project: openpulsehf +doc: docs/dev/reviews/review-1440-host-build-lock.md +status: resolved +last_updated: 2026-09-27 +--- + +# Adversarial review — #1440: a host-wide lock for heavy builds + +One round by Fable, read-only, prompted for falsification, on commit 738ef528 (the PR rebased onto +main ed8ea647). A full gate had already passed on that commit (`GATE: PASS 738ef528`, 2596 tests). +Outcome below the verdict. + +## Consumer + +- `scripts/gate.sh` — run by hand and by CI (`ci.yml:95`, `post-merge-gate.yml:85`, both + `ubuntu-latest`). No script or hook calls gate.sh. +- `.cargo-husky/hooks/pre-push` — the SOURCE of the hook; git runs the copy cargo-husky 1.5.0 + installs in `.git/hooks/pre-push`. +- fnec-rust's `scripts/check-all.sh:34` and `.githooks/pre-push:9` source an equivalent helper + at the same path; that hook runs `cargo test --workspace` directly, never check-all.sh. + +## Prior art + +None: `grep flock|lock|mutex|semaphore` over `scripts/`, `.github/`, `.cargo-husky/` finds only +this commit and an unrelated `OTA_LOCK` in `scripts/run-twin-station-audio.sh`. + +## Twins + +Heavy cargo entry points that do not take the lock: `scripts/slow-tests.sh:46` (the ~83 min +held-out suites, including the `ota_channel_adaptation` run this PR names), `scripts/coverage.sh:35` +(`cargo llvm-cov --workspace`), `scripts/req-mutation.sh:184` (`cargo mutants`), +`scripts/run-test-matrix.sh:18`, the demo/deploy release-build scripts, and the bare +`cargo test --workspace` in `CLAUDE.md:24,60`. CI runners are fresh machines, not twins. + +## Prompt + +"Try to break it": deadlock (can gate.sh and the hook both take the lock in one process tree; +does anything call gate.sh while holding it); fd/flock correctness when the lib is sourced (fd +collisions, release on exit, `set -e`/`set -u`, children inheriting the fd); portability (flock +missing, XDG_RUNTIME_DIR unset, permissions); whether the placement before START_HEAD keeps waiting +out of the drift guard and which modes must be exempt; whether fnec-rust's helper actually +excludes this one. Consumer, prior art and twins by file:line. + +## Verdict + +**Sound with changes.** + +1. HIGH — the hook change cannot take effect: cargo-husky 1.5.0 does not reinstall a hook it + already installed (`build.rs:79-95,199-201`), and the installed `.git/hooks/pre-push` (Sep 18) + has no lock — nor #1418's or #1380's changes. +2. MEDIUM — fail-open with a false "lock acquired": when `exec 9>` fails (directory missing, file + owned by another user) both `flock` calls fail with EBADF and the helper prints "waiting" then + "lock acquired" and runs unlocked. +3. MEDIUM — the lock path is per environment: `$XDG_RUNTIME_DIR/heavy-build.lock` in a login + shell, `/tmp/heavy-build.lock` where XDG_RUNTIME_DIR is unset (cron, sudo, some GUI clients). +4. LOW — fd 9 is inherited by every child; a future test that leaves a server running would hold + the host lock after the gate exits. +5. LOW — `STAMP` is taken before the wait, so the log name records enqueue time. COMMIT/DIRTY and + START_TREE/START_HEAD are after the lock, so waiting stays outside the drift guard. + `--fingerprint` is correctly exempt; `--self-test` and `--quick` correctly take the lock. +6. Deadlock: none. Each entry point sources the lib once; neither calls the other; no other fd-9 + use. fnec-rust's helper is equivalent in path and fd semantics, so the projects exclude each + other (subject to 3). +7. INFO — the twins above should source the helper. + +## Outcome + +- 2: fixed in this PR — the helper refuses (exit 1) when the lock file cannot be opened or the + lock cannot be taken; a host without flock still runs unlocked and says so. Checked: a missing + lock directory exits 1 with the reason. The same fix was applied to fnec-rust's helper. +- 1: the maintainer's installed hook was refreshed from the versioned one; the drift check is + #1448. +- 3: kept by the maintainer's decision — the XDG path is the one the maintainer's global build + rule prescribes for manual `flock` runs, and no gate runs from cron or sudo today. Recorded here. +- 4, 5: accepted as low; not changed. +- 7: #1449. diff --git a/scripts/gate.sh b/scripts/gate.sh index 19a6b684..9295ee65 100755 --- a/scripts/gate.sh +++ b/scripts/gate.sh @@ -62,6 +62,15 @@ case "${1:-}" in *) echo "unknown argument: $1" >&2; exit 2 ;; esac +# Queue behind any other heavy build on this machine, in any project, before +# anything is timed or snapshotted — so time spent waiting is not part of the run, +# and START_HEAD below is taken after the wait. `--fingerprint` is exempt: it is +# the seconds-long primitive and builds nothing. +if [ "$MODE" != "fingerprint" ]; then + # shellcheck source=scripts/lib/host-build-lock.sh + source "$REPO_ROOT/scripts/lib/host-build-lock.sh" "gate" +fi + COMMIT=$(git rev-parse HEAD 2>/dev/null || echo "unknown") if [ -n "$(git status --porcelain 2>/dev/null)" ]; then DIRTY="dirty"; else DIRTY="clean"; fi diff --git a/scripts/lib/host-build-lock.sh b/scripts/lib/host-build-lock.sh new file mode 100644 index 00000000..8b6eca45 --- /dev/null +++ b/scripts/lib/host-build-lock.sh @@ -0,0 +1,43 @@ +# Take the HOST-WIDE heavy-build lock, held until the calling script exits. +# Sourced, not executed: `source scripts/lib/host-build-lock.sh ""`. +# +# Why host-wide and not per repo. Nothing coordinates two heavy cargo runs on one +# machine. On 2026-09-25 another project's gate (fnec-rust) was OOM-killed three +# times while this repo's `cargo test --workspace` ran alongside it, and each kill +# read as a failure of the code under test — the same misattribution +# `GATE: INVALID` exists to prevent, arriving by a channel the gate cannot see. +# The lock file is shared across projects: fnec-rust's `scripts/host-build-lock.sh` +# takes the SAME path, so the two gates queue instead of racing for RAM. +# +# Two rules that keep it from deadlocking: +# - a script takes it ONCE, near the top; nothing it calls takes it again +# (the pre-push hook does not call gate.sh, so each may take it); +# - do not wrap a script that takes it in `flock` yourself — the inner take +# would wait forever on the lock the outer one holds. +# +# Override the path with HEAVY_BUILD_LOCK (every project must agree on it). If +# `flock` is not installed, the caller runs unlocked and says so. + +_hbl_who="${1:-gate}" +_hbl_path="${HEAVY_BUILD_LOCK:-${XDG_RUNTIME_DIR:-/tmp}/heavy-build.lock}" + +# Fail CLOSED once flock exists: a lock file that cannot be opened (its directory +# missing, or the file owned by another user) made both `flock` calls fail with +# EBADF, and the old code printed "waiting" and then "lock acquired" and ran +# unlocked — the one outcome this helper exists to prevent, reported as success. +if command -v flock >/dev/null 2>&1; then + if ! exec 9>"$_hbl_path"; then + echo "$_hbl_who: cannot open the host-wide build lock $_hbl_path — refusing to run unlocked" >&2 + exit 1 + fi + if ! flock -n 9; then + echo "$_hbl_who: another heavy build holds $_hbl_path — waiting for it to finish" >&2 + if ! flock 9; then + echo "$_hbl_who: could not take $_hbl_path — refusing to run unlocked" >&2 + exit 1 + fi + echo "$_hbl_who: lock acquired, continuing" >&2 + fi +else + echo "$_hbl_who: flock not installed — running WITHOUT the host-wide build lock" >&2 +fi