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