diff --git a/.changeset/3772-fix-acquirestatelock-non-eexist.md b/.changeset/3772-fix-acquirestatelock-non-eexist.md new file mode 100644 index 000000000..f96c122a3 --- /dev/null +++ b/.changeset/3772-fix-acquirestatelock-non-eexist.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3773 +--- +acquireStateLock no longer silently grants false-success lock acquisition on non-EEXIST openSync errors (EMFILE/EINTR/ENOSPC under load) — the error is now propagated to the caller, preventing two concurrent processes from simultaneously holding the same lock and producing lost-update STATE.md overwrites. (#3773) diff --git a/get-shit-done/bin/lib/planning-workspace.cjs b/get-shit-done/bin/lib/planning-workspace.cjs index f0a8e2424..86e3efa83 100644 --- a/get-shit-done/bin/lib/planning-workspace.cjs +++ b/get-shit-done/bin/lib/planning-workspace.cjs @@ -259,6 +259,13 @@ 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') { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 100); + continue; + } if (err.code === 'EEXIST') { // Lock exists — check if stale (>30s old) try { diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs index 344550317..55860550f 100644 --- a/get-shit-done/bin/lib/state.cjs +++ b/get-shit-done/bin/lib/state.cjs @@ -934,7 +934,11 @@ function acquireStateLock(statePath) { _heldStateLocks.add(lockPath); return lockPath; } catch (err) { - if (err.code !== 'EEXIST') return lockPath; + // 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; } + 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 // writer causes lost updates (#3711 regression). diff --git a/tests/state-acquirestatelock-non-eexist.test.cjs b/tests/state-acquirestatelock-non-eexist.test.cjs new file mode 100644 index 000000000..8e2de2e59 --- /dev/null +++ b/tests/state-acquirestatelock-non-eexist.test.cjs @@ -0,0 +1,126 @@ +// allow-test-rule: architectural-invariant +// acquireStateLock is a private function (not exported). The behavioral contract — +// throw on non-EEXIST errors rather than returning a false-success lockPath — is +// an implementation invariant that cannot be verified through the public CLI API +// without introducing timing-sensitive mocks. Source inspection is the correct +// and authoritative level for this contract. + +/** + * Regression tests for #3772 — acquireStateLock silently returns false-success + * on non-EEXIST openSync errors (EMFILE / EINTR / ENOSPC under load). + * + * 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) + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const STATE_CJS_PATH = path.join( + __dirname, '..', 'get-shit-done', 'bin', 'lib', 'state.cjs' +); + +// ───────────────────────────────────────────────────────────────────────────── +// Helpers +// ───────────────────────────────────────────────────────────────────────────── + +/** Extract the text of acquireStateLock from the source file. */ +function extractAcquireStateLockSource(src) { + const fnStart = src.indexOf('function acquireStateLock('); + assert.ok(fnStart !== -1, 'acquireStateLock function must exist in state.cjs'); + // Find the closing brace by counting open/close braces from the function start + let depth = 0; + let i = fnStart; + let foundOpen = false; + while (i < src.length) { + if (src[i] === '{') { depth++; foundOpen = true; } + if (src[i] === '}') { depth--; } + if (foundOpen && depth === 0) { return src.slice(fnStart, i + 1); } + i++; + } + throw new Error('Could not find closing brace of acquireStateLock'); +} + +// ───────────────────────────────────────────────────────────────────────────── +// C1. Non-EEXIST error → must throw, not return lockPath +// ───────────────────────────────────────────────────────────────────────────── + +describe('acquireStateLock: non-EEXIST openSync errors (#3772)', () => { + test('C1: source contains throw-not-return for non-EEXIST errors', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const fnSrc = extractAcquireStateLockSource(src); + + // The bug pattern: silently returning the lockPath on non-EEXIST error. + // This branch must NOT appear in the fixed code. + const bugPattern = /if\s*\(\s*err\.code\s*!==\s*['"]EEXIST['"]\s*\)\s*return\s+lockPath/; + assert.ok( + !bugPattern.test(fnSrc), + 'acquireStateLock must NOT return lockPath on non-EEXIST errors (silent false-success — #3772)' + ); + + // The fix: throw the error so callers get the real OS-level failure. + const fixPattern = /if\s*\(\s*err\.code\s*!==\s*['"]EEXIST['"]\s*\)\s*throw\s+err/; + assert.ok( + fixPattern.test(fnSrc), + 'acquireStateLock must throw err on non-EEXIST openSync errors (EMFILE/EINTR/ENOSPC — #3772)' + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// C2. Success path → returns lockPath (regression guard — fix must not break success) +// ───────────────────────────────────────────────────────────────────────────── + +describe('acquireStateLock: success path still returns lockPath', () => { + test('C2: source contains return lockPath in the success (try) branch', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const fnSrc = extractAcquireStateLockSource(src); + + // The success path: openSync succeeds → write PID → close → add to held set → return lockPath. + // Verify the return is still present inside the try block (before the catch). + const tryBlock = fnSrc.slice(fnSrc.indexOf('try {'), fnSrc.indexOf('} catch (')); + assert.ok( + tryBlock.includes('return lockPath'), + 'acquireStateLock must still return lockPath when fs.openSync succeeds (success path intact)' + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// C3. EEXIST error → retry semantics unchanged +// ───────────────────────────────────────────────────────────────────────────── + +describe('acquireStateLock: EEXIST retry semantics unchanged', () => { + test('C3: source still handles EEXIST with retry / stale-lock removal', () => { + const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); + const fnSrc = extractAcquireStateLockSource(src); + + // The EEXIST branch falls through to stale-lock detection and Atomics.wait retry. + // These must still be present after the non-EEXIST fix. + assert.ok( + fnSrc.includes('Atomics.wait'), + 'acquireStateLock must still use Atomics.wait() for EEXIST retry sleep' + ); + + assert.ok( + fnSrc.includes('staleThresholdMs'), + 'acquireStateLock must still check stale lock threshold on EEXIST' + ); + + assert.ok( + fnSrc.includes('maxWaitMs'), + 'acquireStateLock must still enforce max wait budget on EEXIST retry exhaustion' + ); + + // The fix only affects the non-EEXIST branch; the EEXIST guard must still exist. + const eexistGuard = /err\.code\s*!==\s*['"]EEXIST['"]/; + assert.ok( + eexistGuard.test(fnSrc), + 'acquireStateLock must still distinguish EEXIST from other errors' + ); + }); +});