diff --git a/.changeset/build-hooks-atomic-write.md b/.changeset/build-hooks-atomic-write.md new file mode 100644 index 000000000..3b254c9e8 --- /dev/null +++ b/.changeset/build-hooks-atomic-write.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3216 +--- +**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..e0563c3e5 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 = [ @@ -30,6 +36,61 @@ const HOOKS_TO_COPY = [ 'gsd-phase-boundary.sh' ]; +// Sync millisecond sleep using Atomics.wait on a throwaway SharedArrayBuffer. +// Used between Windows rename retries; this script is sync end-to-end so +// setTimeout would not work. Total worst-case backoff across MAX_ATTEMPTS +// is bounded (~400ms) — acceptable for a one-shot build script. +function sleepSync(ms) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +/** + * Atomic-replace via fs.renameSync, with Windows-only retry and fallback. + * + * POSIX rename(2) atomically replaces dest even when readers hold open + * handles on it. Windows MoveFileEx (which fs.renameSync uses with + * MOVEFILE_REPLACE_EXISTING) cannot — it throws EPERM/EBUSY when another + * process has the destination open. Concurrent install.js readers and + * antivirus scanners are the realistic triggers; both release handles + * within milliseconds, so a short backoff resolves the race. After + * retries are exhausted, fall back to copy-then-unlink (re-introduces + * the truncate-then-write race for this single file but keeps the build + * moving rather than crashing). If even copy fails because dest is hard- + * locked, log a non-fatal warning and leave the prior dest in place — a + * subsequent build invocation will retry from a fresh state. + */ +function renameAtomicWithRetry(stagedDest, dest, hook) { + if (process.platform !== 'win32') { + fs.renameSync(stagedDest, dest); + return; + } + const BACKOFFS_MS = [10, 30, 90, 270]; + for (let attempt = 0; attempt <= BACKOFFS_MS.length; attempt++) { + try { + fs.renameSync(stagedDest, dest); + return; + } catch (e) { + const transient = e && (e.code === 'EPERM' || e.code === 'EBUSY'); + if (!transient) throw e; + if (attempt < BACKOFFS_MS.length) { + sleepSync(BACKOFFS_MS[attempt]); + continue; + } + // Retries exhausted; fall back to copy-then-unlink. + try { + fs.copyFileSync(stagedDest, dest); + try { fs.unlinkSync(stagedDest); } catch (_) { /* tolerate */ } + console.warn(`\x1b[33m! ${hook}: rename failed (${e.code}) after ${BACKOFFS_MS.length} retries; used copy-fallback\x1b[0m`); + return; + } catch (fallbackErr) { + try { fs.unlinkSync(stagedDest); } catch (_) { /* tolerate */ } + console.warn(`\x1b[33m! ${hook}: rename + copy fallback both failed (${e.code} → ${fallbackErr.code || fallbackErr.message}); leaving prior dest in place\x1b[0m`); + return; + } + } + } +} + /** * Validate JavaScript syntax without executing the file. * Catches SyntaxError (duplicate const, missing brackets, etc.) @@ -50,10 +111,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 +143,44 @@ 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 */ } } + renameAtomicWithRetry(stagedDest, dest, hook); } + // Best-effort cleanup of the staging dir. If concurrent builders are still + // running, their staged files will be left in STAGE_DIR and cleaned up by + // whichever builder calls fs.rmdirSync last. fs.rmdirSync throws ENOTEMPTY + // on a non-empty directory (it is NOT a silent no-op), so we first read + // the directory via fs.readdirSync(STAGE_DIR) -> leftovers and only call + // fs.rmdirSync(STAGE_DIR) when leftovers.length === 0. A TOCTOU window + // remains: another builder can drop a staged file between the readdirSync + // and the rmdirSync, in which case rmdirSync still throws ENOTEMPTY — the + // outer try/catch swallows that, plus ENOENT if the dir was already + // removed by a peer. Either way, build proceeds; cleanup is best-effort. + try { + const leftovers = fs.readdirSync(STAGE_DIR); + if (leftovers.length === 0) { + fs.rmdirSync(STAGE_DIR); + } + } catch (e) { /* tolerate TOCTOU ENOTEMPTY or ENOENT from peer cleanup */ } + if (hasErrors) { console.error('\n\x1b[31mBuild failed: fix syntax errors above before publishing.\x1b[0m'); process.exit(1);