diff --git a/.changeset/3776-fix-acquirestatelock-retry-allowlist.md b/.changeset/3776-fix-acquirestatelock-retry-allowlist.md new file mode 100644 index 000000000..b80a20031 --- /dev/null +++ b/.changeset/3776-fix-acquirestatelock-retry-allowlist.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3777 +--- +acquireStateLock and withPlanningLock now retry on transient Docker overlay-fs (ENOENT/EINVAL/EIO) and NFS (ESTALE) errno codes in addition to the existing EPERM/EBUSY; truly fatal codes (EMFILE/ENOSPC/EROFS/EACCES) still throw immediately. Closes #3776. diff --git a/get-shit-done/bin/lib/planning-workspace.cjs b/get-shit-done/bin/lib/planning-workspace.cjs index 86e3efa83..79106ab45 100644 --- a/get-shit-done/bin/lib/planning-workspace.cjs +++ b/get-shit-done/bin/lib/planning-workspace.cjs @@ -40,6 +40,23 @@ process.on('exit', () => { } }); +// Transient errno codes that indicate a temporary filesystem condition under +// concurrent O_EXCL races — Docker overlay-fs (ENOENT/EINVAL/EIO), NFS +// (ESTALE), and OS-level interrupt/retry signals (EAGAIN/EINTR). These are +// recoverable; withPlanningLock retries instead of propagating them. +// Truly fatal codes (EMFILE, ENOSPC, EROFS, EACCES) are NOT in this set and +// will still throw immediately. +const PLANNING_LOCK_RETRY_ERRNOS = new Set([ + 'EPERM', // Windows / macOS AV scanner holds the file open during delete + 'EBUSY', // Windows: file in use by another process + 'EAGAIN', // POSIX: resource temporarily unavailable + 'EINTR', // POSIX: syscall interrupted by signal + 'EINVAL', // Docker overlay-fs: transient during concurrent O_EXCL creation + 'EIO', // Docker overlay-fs / NFS: transient I/O error + 'ENOENT', // Docker overlay-fs: parent dir transiently missing during race + 'ESTALE', // NFS: stale file handle (self-resolves on retry) +]); + function planningDir(cwd, ws, project) { if (project === undefined) project = process.env.GSD_PROJECT || null; if (ws === undefined) ws = process.env.GSD_WORKSTREAM || null; @@ -259,10 +276,10 @@ function withPlanningLock(cwd, fn) { try { return runWithHeldLock(); } catch (err) { - // EPERM / EBUSY occur transiently on some OS + AV scanner combinations when - // the lock file is briefly held open by the deleting process. Treat as EEXIST - // (file contention) — wait and retry rather than propagating. - if (err.code === 'EPERM' || err.code === 'EBUSY') { + // Transient filesystem errors (Docker overlay-fs, NFS, OS signals, AV scanners) + // are recoverable — wait and retry rather than propagating. + // See PLANNING_LOCK_RETRY_ERRNOS for the full list and rationale. + if (PLANNING_LOCK_RETRY_ERRNOS.has(err.code)) { Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 100); continue; } diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs index 55860550f..df6d0118b 100644 --- a/get-shit-done/bin/lib/state.cjs +++ b/get-shit-done/bin/lib/state.cjs @@ -913,6 +913,23 @@ function syncStateFrontmatter(content, cwd) { return `---\n${yamlStr}\n---\n\n${body}`; } +// Transient errno codes that indicate a temporary filesystem condition under +// concurrent O_EXCL races — Docker overlay-fs (ENOENT/EINVAL/EIO), NFS +// (ESTALE), and OS-level interrupt/retry signals (EAGAIN/EINTR). These are +// recoverable; acquireStateLock retries instead of propagating them. +// Truly fatal codes (EMFILE, ENOSPC, EROFS, EACCES) are NOT in this set and +// will still throw immediately. +const ACQUIRE_LOCK_RETRY_ERRNOS = new Set([ + 'EPERM', // Windows / macOS AV scanner holds the file open during delete + 'EBUSY', // Windows: file in use by another process + 'EAGAIN', // POSIX: resource temporarily unavailable + 'EINTR', // POSIX: syscall interrupted by signal + 'EINVAL', // Docker overlay-fs: transient during concurrent O_EXCL creation + 'EIO', // Docker overlay-fs / NFS: transient I/O error + 'ENOENT', // Docker overlay-fs: parent dir transiently missing during race + 'ESTALE', // NFS: stale file handle (self-resolves on retry) +]); + /** * Acquire a lockfile for STATE.md operations. * Returns the lock path for later release. @@ -934,10 +951,10 @@ function acquireStateLock(statePath) { _heldStateLocks.add(lockPath); return lockPath; } catch (err) { - // EPERM / EBUSY occur transiently on some OS + AV scanner combinations when - // the lock file is briefly held open by the process that is deleting it. - // These are recoverable — retry the acquisition loop. - if (err.code === 'EPERM' || err.code === 'EBUSY') { continue; } + // Transient filesystem errors (Docker overlay-fs, NFS, OS signals, AV scanners) + // are recoverable — retry the acquisition loop rather than propagating. + // See ACQUIRE_LOCK_RETRY_ERRNOS for the full list and rationale. + if (ACQUIRE_LOCK_RETRY_ERRNOS.has(err.code)) { continue; } if (err.code !== 'EEXIST') throw err; // propagate — silent bypass causes lost updates // Only unlink a lock we did not place when it has crossed the staleness // threshold (crashed holder). Nuking a fresh lock held by a slow-but-live diff --git a/tests/state-acquirestatelock-non-eexist.test.cjs b/tests/state-acquirestatelock-non-eexist.test.cjs index 8e2de2e59..62aaff761 100644 --- a/tests/state-acquirestatelock-non-eexist.test.cjs +++ b/tests/state-acquirestatelock-non-eexist.test.cjs @@ -9,10 +9,16 @@ * Regression tests for #3772 — acquireStateLock silently returns false-success * on non-EEXIST openSync errors (EMFILE / EINTR / ENOSPC under load). * + * Extended in #3776 to cover Docker overlay-fs and NFS transient errno codes. + * * Contract under test: * C1. Non-EEXIST error from fs.openSync → must throw, not return lockPath * C2. Success path (openSync succeeds) → must return lockPath * C3. EEXIST error → retry / wait semantics unchanged (not impacted by this fix) + * C4. RETRY_ERRNOS set: EAGAIN/EINTR/EINVAL/EIO/ENOENT/ESTALE must be in the retry allowlist + * C5. Fatal codes: EMFILE/ENOSPC/EROFS/EACCES must NOT be in the retry allowlist + * C6. Unknown errno codes must NOT be in the retry allowlist (conservative default) + * C7. EPERM/EBUSY must remain in the retry allowlist (from #3773) */ const { test, describe } = require('node:test'); @@ -124,3 +130,142 @@ describe('acquireStateLock: EEXIST retry semantics unchanged', () => { ); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// C4. RETRY_ERRNOS set: new Docker/NFS transient codes must be present (#3776) +// ───────────────────────────────────────────────────────────────────────────── + +/** Extract the ACQUIRE_LOCK_RETRY_ERRNOS Set literal from the source file. */ +function extractRetryErrnosSource(src) { + const constStart = src.indexOf('const ACQUIRE_LOCK_RETRY_ERRNOS'); + assert.ok(constStart !== -1, 'ACQUIRE_LOCK_RETRY_ERRNOS constant must exist in state.cjs'); + // Extract to the end of the Set(...) constructor — find the closing ]); + const setEnd = src.indexOf(']);', constStart); + assert.ok(setEnd !== -1, 'ACQUIRE_LOCK_RETRY_ERRNOS Set must have closing ]);'); + return src.slice(constStart, setEnd + 2); +} + +describe('acquireStateLock: RETRY_ERRNOS Set contains expected transient codes (#3776)', () => { + test('C4a: EAGAIN is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(setBlock.includes("'EAGAIN'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EAGAIN (resource temporarily unavailable)'); + }); + + test('C4b: EINTR is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(setBlock.includes("'EINTR'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EINTR (syscall interrupted)'); + }); + + test('C4c: EINVAL is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(setBlock.includes("'EINVAL'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EINVAL (Docker overlay-fs transient)'); + }); + + test('C4d: EIO is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(setBlock.includes("'EIO'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EIO (Docker overlay-fs / NFS transient)'); + }); + + test('C4e: ENOENT is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(setBlock.includes("'ENOENT'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include ENOENT (Docker overlay-fs parent dir transient)'); + }); + + test('C4f: ESTALE is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(setBlock.includes("'ESTALE'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include ESTALE (NFS stale file handle)'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// C5. Fatal codes: EMFILE/ENOSPC/EROFS/EACCES must NOT be in the retry set +// ───────────────────────────────────────────────────────────────────────────── + +describe('acquireStateLock: fatal errno codes NOT in RETRY_ERRNOS (#3776)', () => { + test('C5a: EMFILE is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (fd limit exhausted — fatal)', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(!setBlock.includes("'EMFILE'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include EMFILE (fatal: fd limit)'); + }); + + test('C5b: ENOSPC is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (disk full — fatal)', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(!setBlock.includes("'ENOSPC'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include ENOSPC (fatal: disk full)'); + }); + + test('C5c: EROFS is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (read-only fs — fatal)', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(!setBlock.includes("'EROFS'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include EROFS (fatal: read-only fs)'); + }); + + test('C5d: EACCES is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (permission denied — fatal)', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(!setBlock.includes("'EACCES'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include EACCES (fatal: no permission)'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// C6. Unknown errno codes are not in the retry set (conservative default) +// ───────────────────────────────────────────────────────────────────────────── + +describe('acquireStateLock: unknown errno codes not retried (conservative default, #3776)', () => { + test('C6: ACQUIRE_LOCK_RETRY_ERRNOS does not include ESOMETHING (unknown code)', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok( + !setBlock.includes("'ESOMETHING'"), + 'ACQUIRE_LOCK_RETRY_ERRNOS must not include unknown errno ESOMETHING (conservative: surface unknowns)' + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// C7. EPERM/EBUSY remain in the retry set (regression guard from #3773) +// ───────────────────────────────────────────────────────────────────────────── + +describe('acquireStateLock: EPERM/EBUSY still in RETRY_ERRNOS (regression guard, #3773)', () => { + test('C7a: EPERM is in ACQUIRE_LOCK_RETRY_ERRNOS (Windows / macOS AV scanner)', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(setBlock.includes("'EPERM'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must still include EPERM (#3773 regression guard)'); + }); + + test('C7b: EBUSY is in ACQUIRE_LOCK_RETRY_ERRNOS (Windows file in use)', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const setBlock = extractRetryErrnosSource(src); + assert.ok(setBlock.includes("'EBUSY'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must still include EBUSY (#3773 regression guard)'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// C8. acquireStateLock uses Set-based retry check (not inline literal) +// ───────────────────────────────────────────────────────────────────────────── + +describe('acquireStateLock: uses Set-based retry check (grep-able, not inline literal)', () => { + test('C8: retry check uses ACQUIRE_LOCK_RETRY_ERRNOS.has() not hardcoded comparisons', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const fnSrc = extractAcquireStateLockSource(src); + + // Must use the named Set for the check inside the function + assert.ok( + fnSrc.includes('ACQUIRE_LOCK_RETRY_ERRNOS.has('), + 'acquireStateLock catch block must use ACQUIRE_LOCK_RETRY_ERRNOS.has() for retry decision' + ); + + // Must NOT have the old inline EPERM/EBUSY literal check + const oldPattern = /err\.code\s*===\s*['"]EPERM['"]\s*\|\|\s*err\.code\s*===\s*['"]EBUSY['"]/; + assert.ok( + !oldPattern.test(fnSrc), + 'acquireStateLock must not use old inline EPERM||EBUSY check (should use ACQUIRE_LOCK_RETRY_ERRNOS.has())' + ); + }); +});