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) <noreply@anthropic.com>
This commit is contained in:
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user