From c4f11db5e950a6f82bdb3ab2a8361b7fe5ded68a Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 6 May 2026 23:34:29 -0400 Subject: [PATCH] fix(build-hooks): atomic rename to prevent race with concurrent install reads scripts/build-hooks.js used fs.copyFileSync (truncate-then-write, non-atomic). Under --test-concurrency=4, multiple builder invocations raced; a parallel install.js subprocess could readFileSync between truncate and write and observe an empty file, then write that emptiness into the install target. Surfaced as the release-blocking bug-2136-sh-hook-version part 4 failure on main even though the same SHA passed every install-smoke matrix entry. Fix: stage outputs to hooks/.dist-staging/ then fs.renameSync into hooks/dist/. POSIX rename(2) is atomic, so concurrent readers always observe a complete file. The existing bug-2136 part 4 test locks the post-fix invariant. Failing run: https://github.com/gsd-build/get-shit-done/actions/runs/25472202941/job/74738276687 Closes #3214 Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/build-hooks-atomic-write.md | 4 +++ .gitignore | 3 ++ CHANGELOG.md | 1 + scripts/build-hooks.js | 42 +++++++++++++++++++++++--- 4 files changed, 46 insertions(+), 4 deletions(-) create mode 100644 .changeset/build-hooks-atomic-write.md diff --git a/.changeset/build-hooks-atomic-write.md b/.changeset/build-hooks-atomic-write.md new file mode 100644 index 000000000..e31442123 --- /dev/null +++ b/.changeset/build-hooks-atomic-write.md @@ -0,0 +1,4 @@ +--- +type: Fixed +--- +**Atomic writes in `scripts/build-hooks.js` to fix flaky release CI** — nine test files invoke `build-hooks.js` from their `before()` hooks, and `scripts/run-tests.cjs` runs test files with `--test-concurrency=4`, so multiple builders raced to rewrite the same files in `hooks/dist/`. `fs.copyFileSync(src, dest)` truncates `dest` then writes it; a parallel `bin/install.js` subprocess (spawned by another install test) could `fs.readFileSync` between the truncate and the write and observe an empty file. install.js then wrote that empty content into the install target, so installed `.sh` hooks lacked their `# gsd-hook-version:` header. This surfaced as the release-blocking failure in `tests/bug-2136-sh-hook-version.test.cjs` part 4 even though the same SHA passed on every other Node-22/Node-24 install-smoke matrix run. `build-hooks.js` now stages each output to a sibling `hooks/.dist-staging/` directory (same filesystem as `hooks/dist/`) and uses `fs.renameSync` to swap into place — POSIX `rename(2)` is atomic, so concurrent readers always observe a complete file. The existing `tests/bug-2136-sh-hook-version.test.cjs` part 4 already locks the post-fix invariant. (Failing run: https://github.com/gsd-build/get-shit-done/actions/runs/25472202941/job/74738276687) diff --git a/.gitignore b/.gitignore index 959684d22..391bca1e2 100644 --- a/.gitignore +++ b/.gitignore @@ -14,6 +14,9 @@ commands.html # Build artifacts (committed to npm, not git) hooks/dist/ +# Atomic-write staging dir used by scripts/build-hooks.js (see comment there) +hooks/.dist-staging/ + # Coverage artifacts coverage/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 179c4e4a0..f71e93f78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **Atomic writes in `scripts/build-hooks.js` to fix flaky release CI** — nine test files invoke `build-hooks.js` from their `before()` hooks, and `scripts/run-tests.cjs` runs test files with `--test-concurrency=4`, so multiple builders raced to rewrite the same files in `hooks/dist/`. `fs.copyFileSync(src, dest)` truncates `dest` then writes it; a parallel `bin/install.js` subprocess (spawned by another install test) could `fs.readFileSync` between the truncate and the write and observe an empty file. install.js then wrote that empty content into the install target, so installed `.sh` hooks lacked their `# gsd-hook-version:` header. This surfaced as the release-blocking failure in `tests/bug-2136-sh-hook-version.test.cjs` part 4 even though the same SHA passed on every other Node-22/Node-24 install-smoke matrix run. `build-hooks.js` now stages each output to a sibling `hooks/.dist-staging/` directory (same filesystem as `hooks/dist/`) and uses `fs.renameSync` to swap into place — POSIX `rename(2)` is atomic, so concurrent readers always observe a complete file. (Failing run: https://github.com/gsd-build/get-shit-done/actions/runs/25472202941/job/74738276687) - **Stable node path on Homebrew** — `resolveNodeRunner()` now maps versioned Homebrew Cellar paths (e.g. `/usr/local/Cellar/node/25.8.1/bin/node`) to the stable Homebrew symlinks (`/usr/local/bin/node` on Intel, `/opt/homebrew/bin/node` on Apple Silicon). `rewriteLegacyManagedNodeHookCommands()` applies the same normalization to baked Cellar paths in existing hook commands. This prevents `dyld: Library not loaded` errors after `brew upgrade node`. (#3181) - **Milestone-archive layout support** — `validate consistency`, `validate health`, and `find-phase` now scan `.planning/milestones/v*-phases/` directories in addition to the flat `.planning/phases/` layout. Projects that have graduated to milestone-archive layout no longer receive spurious W006 "Phase N in ROADMAP.md but no directory on disk" warnings for every active phase. (#3164) diff --git a/scripts/build-hooks.js b/scripts/build-hooks.js index e2f6b5c25..3cdcf9864 100644 --- a/scripts/build-hooks.js +++ b/scripts/build-hooks.js @@ -12,6 +12,12 @@ const vm = require('vm'); const HOOKS_DIR = path.join(__dirname, '..', 'hooks'); const DIST_DIR = path.join(HOOKS_DIR, 'dist'); +// Sibling directory used to stage atomic writes. Lives under hooks/ so it +// shares a filesystem with DIST_DIR (POSIX rename(2) is only atomic within +// the same filesystem) but is NOT inside DIST_DIR — so readers that +// readdirSync(DIST_DIR) (e.g. bin/install.js, install-hooks-copy tests) +// never observe a transient ".tmp" sibling file there. +const STAGE_DIR = path.join(HOOKS_DIR, '.dist-staging'); // Hooks to copy (pure Node.js, no bundling needed) const HOOKS_TO_COPY = [ @@ -50,10 +56,14 @@ function validateSyntax(filePath) { } function build() { - // Ensure dist directory exists + // Ensure dist and staging directories exist (staging is a sibling of dist + // used to make writes atomic — see STAGE_DIR comment above). if (!fs.existsSync(DIST_DIR)) { fs.mkdirSync(DIST_DIR, { recursive: true }); } + if (!fs.existsSync(STAGE_DIR)) { + fs.mkdirSync(STAGE_DIR, { recursive: true }); + } let hasErrors = false; @@ -78,13 +88,37 @@ function build() { } console.log(`\x1b[32m✓\x1b[0m Copying ${hook}...`); - fs.copyFileSync(src, dest); - // Preserve executable bit for shell scripts + // Atomic write: copy to a per-process staging file in the sibling + // STAGE_DIR (same filesystem as DIST_DIR so rename(2) is atomic), then + // rename into place. Multiple test files invoke this script concurrently + // from their before() hooks; fs.copyFileSync truncates then writes the + // destination — readers (install.js subprocesses spawned by parallel + // install tests) can observe the dest empty or partial mid-write, + // producing flaky failures such as bug-2136 part 4 where installed .sh + // hooks lacked their "# gsd-hook-version:" header. POSIX rename(2) + // makes the swap atomic so readers see either the old file or the new + // file. The staging file lives outside DIST_DIR so readdirSync(DIST_DIR) + // (in install.js and tests) never observes a transient ".tmp" sibling. + const stagedDest = path.join(STAGE_DIR, `${hook}.${process.pid}.${Date.now()}`); + fs.copyFileSync(src, stagedDest); + // Preserve executable bit for shell scripts before rename so the + // installed file is executable from the very first observation. if (hook.endsWith('.sh')) { - try { fs.chmodSync(dest, 0o755); } catch (e) { /* Windows */ } + try { fs.chmodSync(stagedDest, 0o755); } catch (e) { /* Windows */ } } + fs.renameSync(stagedDest, dest); } + // Best-effort cleanup of the staging dir. If concurrent builders are still + // running, leftover files belong to them and will be cleaned up on their + // own renames; rmdir-on-non-empty is a no-op so this is race-safe. + try { + const leftovers = fs.readdirSync(STAGE_DIR); + if (leftovers.length === 0) { + fs.rmdirSync(STAGE_DIR); + } + } catch (e) { /* tolerate races / missing dir */ } + if (hasErrors) { console.error('\n\x1b[31mBuild failed: fix syntax errors above before publishing.\x1b[0m'); process.exit(1);