fix(state): acquireStateLock throws on non-EEXIST openSync errors (#3773)

* fix(state): acquireStateLock throws on non-EEXIST openSync errors

Closes #3772

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* chore(changeset): update pr reference to #3773

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(state): retry on EPERM/EBUSY in acquireStateLock and withPlanningLock

Resolves state update TOCTOU failure on macos-22 and config-set
concurrency failure on windows-24.
Root cause: openSync(O_CREAT|O_EXCL) can return EPERM or EBUSY
transiently on some CI OS+AV combinations when the lock file is
briefly held open by the deleting process; the new throw-on-non-EEXIST
guard from #3772 propagated these transient errors, killing child
processes and causing lost updates in the concurrency tests.
Fix: guard EPERM/EBUSY with an explicit continue before the
throw-on-non-EEXIST line in both acquireStateLock and withPlanningLock;
the C1 source-audit test still passes because the throw pattern is
preserved for all other non-EEXIST codes.

Refs #3772

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

.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)

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 {

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).

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'
+    );
+  });
+});