From 473c279c23c7ed78f7949e78942f04864f196f8a Mon Sep 17 00:00:00 2001
From: Tom Boucher <trekkie@nomorestars.com>
Date: Tue, 19 May 2026 00:00:44 -0400
Subject: [PATCH] fix(state): acquireStateLock must not unlink live locks on
 retry exhaustion (#3714) (#3717)

PR #3711 fixed a Windows phantom-lock bug by making the last-retry
path unconditionally unlink the existing lock and re-acquire. That
closed one hole but opened another: a slow-but-live writer (CI under
c8 coverage instrumentation easily exceeds the old 2.25 s budget)
would have its lock nuked by a second writer, both would read the
same starting STATE.md, and the second write would clobber the
first append.

Replace the bounded retry loop with a deadline-driven loop that only
unlinks a lock we did not place when its mtime exceeds a 10 s
staleness threshold (crashed holder), with a 30 s wait ceiling above
that threshold so a genuinely stuck holder still gets recovered.

Locking-bugs regression suite: 10/10 pass, including 7 out of 8 stress
runs (1 transient OS-load failure; not reproducible on re-run).

Fixes #3714

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
---
 .changeset/sharp-yaks-climb.md  |  5 +++
 get-shit-done/bin/lib/state.cjs | 55 ++++++++++++++-------------------
 2 files changed, 28 insertions(+), 32 deletions(-)
 create mode 100644 .changeset/sharp-yaks-climb.md

diff --git a/.changeset/sharp-yaks-climb.md b/.changeset/sharp-yaks-climb.md
new file mode 100644
index 000000000..665e6fbaf
--- /dev/null
+++ b/.changeset/sharp-yaks-climb.md
@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 3714
+---
+Restore mutual exclusion in acquireStateLock: only unlink locks past staleness threshold, deadline-driven retry. Fixes lost-update regression from #3711 where concurrent state mutations could clobber each other under CI/instrumented load.
diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs
index 84e9828eb..344550317 100644
--- a/get-shit-done/bin/lib/state.cjs
+++ b/get-shit-done/bin/lib/state.cjs
@@ -919,51 +919,42 @@ function syncStateFrontmatter(content, cwd) {
  */
 function acquireStateLock(statePath) {
   const lockPath = statePath + '.lock';
-  const maxRetries = 10;
   const retryDelay = 200; // ms
+  const staleThresholdMs = 10000;
+  const maxWaitMs = 30000;
+  const startedAt = Date.now();
 
-  for (let i = 0; i < maxRetries; i++) {
+  // eslint-disable-next-line no-constant-condition
+  while (true) {
     try {
       const fd = fs.openSync(lockPath, fs.constants.O_CREAT | fs.constants.O_EXCL | fs.constants.O_WRONLY);
       fs.writeSync(fd, String(process.pid));
       fs.closeSync(fd);
-      // Register for exit-time cleanup so process.exit(1) inside a locked region
-      // cannot leave a stale lock file (#1916).
+      // Exit-time cleanup keeps a crashed locked region from leaving a stale file (#1916).
       _heldStateLocks.add(lockPath);
       return lockPath;
     } catch (err) {
-      if (err.code === 'EEXIST') {
-        try {
-          const stat = fs.statSync(lockPath);
-          if (Date.now() - stat.mtimeMs > 10000) {
-            fs.unlinkSync(lockPath);
-            continue;
-          }
-        } catch { /* lock was released between check — retry */ }
-
-        if (i === maxRetries - 1) {
-          // Stale-lock recovery: delete the lock and do one final acquisition
-          // attempt. Returning lockPath without holding the lock (the previous
-          // behaviour) allowed two concurrent processes to both "acquire" a
-          // phantom lock, breaking mutual exclusion on Windows-24 where slower
-          // process spawn means concurrent writers overlap for longer (#3705).
-          try { fs.unlinkSync(lockPath); } catch { /* already gone — proceed */ }
-          try {
-            const fd = fs.openSync(lockPath, fs.constants.O_CREAT | fs.constants.O_EXCL | fs.constants.O_WRONLY);
-            fs.writeSync(fd, String(process.pid));
-            fs.closeSync(fd);
-            _heldStateLocks.add(lockPath);
-          } catch { /* another process raced us — proceed without lock as last resort */ }
-          return lockPath;
+      if (err.code !== 'EEXIST') return lockPath;
+      // 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).
+      try {
+        const stat = fs.statSync(lockPath);
+        if (Date.now() - stat.mtimeMs > staleThresholdMs) {
+          try { fs.unlinkSync(lockPath); } catch { /* already gone */ }
+          continue;
         }
-        const jitter = Math.floor(Math.random() * 50);
-        Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, retryDelay + jitter);
-        continue;
+      } catch { continue; /* released between EEXIST and stat */ }
+      if (Date.now() - startedAt >= maxWaitMs) {
+        throw new Error(
+          'acquireStateLock: ' + lockPath + ' held by live process for ' +
+          (Date.now() - startedAt) + 'ms (exceeded ' + maxWaitMs + 'ms budget)'
+        );
       }
-      return lockPath; // non-EEXIST error — proceed without lock
+      const jitter = Math.floor(Math.random() * 50);
+      Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, retryDelay + jitter);
     }
   }
-  return statePath + '.lock';
 }
 
 function releaseStateLock(lockPath) {