diff --git a/.changeset/1532-core-lock-liveness.md b/.changeset/1532-core-lock-liveness.md index fb3a3994a..a42f81bf0 100644 --- a/.changeset/1532-core-lock-liveness.md +++ b/.changeset/1532-core-lock-liveness.md @@ -2,4 +2,4 @@ type: Fixed pr: 1532 --- -**Core-path file locks now verify the holder process is alive before stealing a stale lock (#1532)** — the STATE.md write lock (`acquireStateLock`) and the `.planning/` workspace lock (`withPlanningLock`) previously stole locks on a bare `mtime` timer with no liveness check, so a live-but-slow holder (e.g. a deep `.planning/` scan on slow NFS) could have its lock stolen mid-write, corrupting STATE.md or losing an update. Both locks now gate stealing on `process.kill(pid,0)` liveness with a deadman ceiling above the wait budget (pid-reuse backstop), `withPlanningLock` no longer force-steals a live holder on timeout (and can no longer leak an uncaught `EEXIST`), `writeStateMd` computes its disk scan inside the lock, and `acquireStateLock` no longer leaks a file descriptor or strands an empty lock on a recoverable write error. The uncontended path is unchanged. +**Core-path file locks now verify the holder process is alive before stealing a stale lock (#1532)** — the STATE.md write lock (`acquireStateLock`) and the `.planning/` workspace lock (`withPlanningLock`) previously stole locks on a bare `mtime` timer with no liveness check, so a live-but-slow holder (e.g. a deep `.planning/` scan on slow NFS) could have its lock stolen mid-write, corrupting STATE.md or losing an update. Both locks now gate stealing on `process.kill(pid,0)` liveness with a deadman ceiling above the wait budget (pid-reuse backstop), `withPlanningLock` no longer force-steals a live holder on timeout (and can no longer leak an uncaught `EEXIST`), `writeStateMd` computes its disk scan inside the lock, and `acquireStateLock` no longer leaks a file descriptor or strands an empty lock on a recoverable write error. The steal itself is now race-safe: a lock is never stolen while its body is still being written (the create→pid-write window), and stealing uses an atomic rename with an identity re-confirm so two waiters can no longer both reclaim the same lock and end up holding it concurrently. The uncontended path is unchanged. diff --git a/src/planning-workspace.cts b/src/planning-workspace.cts index a8d11a718..b86147eb4 100644 --- a/src/planning-workspace.cts +++ b/src/planning-workspace.cts @@ -65,6 +65,18 @@ function _planningLockIsPidAlive(pid: number): boolean { return _planningLockProbes.isPidAlive(pid); } +// Test seam (PR #1532 review): beforeSteal fires AFTER the steal decision but BEFORE +// the identity re-confirm + atomic rename-steal, so a test can recreate a fresh lock +// in the decision→steal gap and prove the identity re-confirm aborts a double-steal. +// Defaults to a no-op; real callers are byte-for-behaviour unchanged. +interface PlanningLockTestHooks { + beforeSteal?: (ctx: { lockPath: string }) => void; +} +const _planningLockTestHooks: PlanningLockTestHooks = {}; + +// Monotonic sequence for unique stale-steal rename targets (no crypto dependency). +let _planningStealSeq = 0; + /** * Is the holder recorded in the .lock body VERIFIED-LIVE? The body is JSON * { pid, cwd, acquired }. Returns true ONLY when the body parses AND the recorded @@ -217,18 +229,60 @@ function withPlanningLock(cwd: string, fn: () => T, clock?: Clock): T { // recorded holder is NOT verified-live (crashed/dead pid or garbage body). // A verified-live holder is waited on — never force-stolen — because nuking // a slow-but-live writer's lock corrupts the .planning/ critical section. + // The steal is an ATOMIC rename-then-recreate guarded by an identity re-confirm + // so a racer that recreates a fresh lock in the decision→steal gap never has + // its replacement deleted (audit M2 / PR #1532 review, window b). The body is + // written atomically (writeFileSync …{flag:'wx'}) so there is no empty-body + // create window here — only the double-steal needs hardening. try { + const decisionStat = fs.statSync(lockPath); + // Snapshot the decision-time body too: (dev, ino) alone is defeated by inode + // REUSE (a racer's unlink+recreate can land on the same inode), so the body + // content binds the identity as well — mirrors capability-lock.cts's (dev, + // ino, ts) re-confirm. + let decisionBody: string | null; + try { decisionBody = fs.readFileSync(lockPath, 'utf-8'); } catch { decisionBody = null; } let stealable = !_planningHolderVerifiedLive(lockPath); if (!stealable) { // Verified-live, but recover anyway once the lock crosses the absolute // deadman ceiling — defeats a pid-reuse false-alive that would otherwise // block forever (R4-FIX; mtime age is from lock creation, not this call). - const age = clock.now() - fs.statSync(lockPath).mtimeMs; + const age = clock.now() - decisionStat.mtimeMs; stealable = age > deadmanCeilingMs; } if (stealable) { - fs.unlinkSync(lockPath); - continue; // dead/garbage/expired holder — retry immediately to grab the freed lock + if (_planningLockTestHooks.beforeSteal) _planningLockTestHooks.beforeSteal({ lockPath }); + // Identity re-confirm immediately before the steal: a racer that stole + + // recreated a fresh lock in the decision→steal gap changes (dev, ino) → do + // NOT delete the replacement; back off and re-evaluate. + let confirmStat: fs.Stats; + try { + confirmStat = fs.statSync(lockPath); + } catch { + continue; // vanished between decision and steal — retry the create. + } + let confirmBody: string | null; + try { confirmBody = fs.readFileSync(lockPath, 'utf-8'); } catch { confirmBody = null; } + const sameInstance = + typeof decisionStat.dev === 'number' && typeof decisionStat.ino === 'number' && + confirmStat.dev === decisionStat.dev && confirmStat.ino === decisionStat.ino && + decisionBody !== null && confirmBody === decisionBody; + if (!sameInstance) { + clock.sleep(100); // a racer won the steal + recreated — re-evaluate, don't delete it. + continue; + } + // Atomic steal: rename the inode aside, then remove it. Only ONE racer can + // win the rename; a failed rename means another process already stole it, so + // we must NOT fall through to a delete — back off and retry the create. + const stolen = lockPath + '.stale-' + process.pid + '-' + clock.now() + '-' + (_planningStealSeq++); + let renamed = false; + try { fs.renameSync(lockPath, stolen); renamed = true; } catch { /* another racer won */ } + if (renamed) { + try { fs.rmSync(stolen, { force: true }); } catch { /* best-effort */ } + continue; // dead/garbage/expired holder freed — retry immediately to grab it. + } + clock.sleep(100); // lost the steal race — back off and retry. + continue; } } catch { continue; } @@ -348,4 +402,11 @@ export = { _resetLockProbes(): void { _planningLockProbes.isPidAlive = _realIsPidAlive; }, + // Test seam (PR #1532 review): script the steal decision→steal gap (window b). + _setPlanningLockTestHooks(hooks: PlanningLockTestHooks): void { + if ('beforeSteal' in hooks) _planningLockTestHooks.beforeSteal = hooks.beforeSteal; + }, + _resetPlanningLockTestHooks(): void { + delete _planningLockTestHooks.beforeSteal; + }, }; diff --git a/src/state.cts b/src/state.cts index 28637eac2..ba74b5931 100644 --- a/src/state.cts +++ b/src/state.cts @@ -194,6 +194,10 @@ const _stateLockProbes: { isPidAlive: (pid: number) => boolean } = { isPidAlive: // openSync-succeeds-then-write-fails cleanup path without an OS-level fault. // onLoopIteration(ctx) — fired at the TOP of each acquireStateLock retry // iteration so a test can snapshot whether an orphan lock is stranded. +// beforeSteal(ctx) — fired AFTER the steal decision but BEFORE the identity +// re-confirm + atomic rename-steal. A test can recreate a fresh lock here to +// simulate a racer winning the steal in the decision→steal gap, proving the +// identity re-confirm aborts a double-steal (PR #1532 review window b). // // All hooks default to no-ops; real callers are byte-for-behaviour unchanged. // --------------------------------------------------------------------------- @@ -201,6 +205,7 @@ interface StateLockTestHooks { afterAcquire?: (lockPath: string) => void; simulateWriteError?: string | null; onLoopIteration?: (ctx: { iteration: number }) => void; + beforeSteal?: (ctx: { lockPath: string }) => void; } const _stateLockTestHooks: StateLockTestHooks = {}; @@ -230,17 +235,33 @@ function _stateLockIsPidAlive(pid: number): boolean { * locks never block forever, and a live holder is never stolen. */ function _stateHolderVerifiedLive(lockPath: string): boolean { + const pid = _stateLockBodyPid(lockPath); + return pid !== null && _stateLockIsPidAlive(pid); +} + +/** + * Parse the lock body to its recorded pid, or null when the body is empty / non-numeric + * / unreadable (legacy or mid-creation). Distinguishing a COMPLETE dead-pid body (steal + * promptly) from an EMPTY/unparseable one (the create→write window — do not steal while + * fresh) is what `_stateHolderVerifiedLive` alone cannot express, so the steal decision + * in acquireStateLock reads the pid directly (PR #1532 review, window a). + */ +function _stateLockBodyPid(lockPath: string): number | null { let body: string; try { body = fs.readFileSync(lockPath, 'utf-8'); } catch { - return false; // unreadable body → cannot verify → not verified-live (stealable under ceiling) + return null; // unreadable body → cannot verify } - const pid = parseInt(body.trim(), 10); - if (!Number.isInteger(pid) || pid <= 0 || String(pid) !== body.trim()) return false; - return _stateLockIsPidAlive(pid); + const trimmed = body.trim(); + const pid = parseInt(trimmed, 10); + if (!Number.isInteger(pid) || pid <= 0 || String(pid) !== trimmed) return null; + return pid; } +// Monotonic sequence for unique stale-steal rename targets (no crypto dependency). +let _stateStealSeq = 0; + // Hoisted to module scope — compiled once, not per call (#320). Stateless (/i, used with .match). const byPhaseTablePattern = /(\|\s*Phase\s*\|\s*Plans\s*\|\s*Total\s*\|\s*Avg\/Plan\s*\|[ \t]*\n\|(?:[- :\t]+\|)+[ \t]*\n)((?:[ \t]*\|[^\n]*\n)*)(?=\n|$)/i; @@ -1684,6 +1705,15 @@ function acquireStateLock(statePath: string, clock?: StateLockClock): string { // than blocking forever. The prior mtime-only `staleThresholdMs = 10000` gate // was BELOW maxWaitMs, so a live-but-slow holder >10 s was robbed mid-write. const deadmanCeilingMs = 60000; + // Fresh-create floor (PR #1532 review, window a) — a lock with an EMPTY/unparseable + // body is either mid-creation (O_EXCL create done, pid not yet written by the holder) + // or a genuine orphan. While such a body is younger than this floor it is treated as + // mid-creation and is NEVER stolen — stealing it at age ≈ 0 robs a holder still + // writing its pid (the lost-update window capability-lock.cts's `age <= LOCK_STALE_MS` + // floor closes). The create→write gap is sub-millisecond; this floor is orders of + // magnitude larger yet well under maxWaitMs so a real orphan still clears within budget. + // A COMPLETE dead-pid body is NOT subject to this floor — it is stolen promptly. + const freshCreateFloorMs = 1000; const startedAt = clock.now(); // Shared helper: check the time budget then back off with jitter before the @@ -1742,37 +1772,80 @@ function acquireStateLock(statePath: string, clock?: StateLockClock): string { continue; } if ((err as NodeJS.ErrnoException).code !== 'EEXIST') throw err; // propagate — silent bypass causes lost updates - // Liveness-gated steal (audit M1). Only unlink a lock we did not place when - // either (a) its recorded holder is NOT verified-live — a crashed/dead pid or - // a garbage/legacy body — so it is stolen PROMPTLY regardless of age, or - // (b) its age has crossed the absolute deadman ceiling (set above maxWaitMs) - // — the pid-reuse backstop. A VERIFIED-LIVE holder under the ceiling is NEVER - // stolen, even if older than the old mtime-only threshold: nuking a slow-but- - // live writer's lock causes lost updates (#3711 / #500/#905/#1230 family). + // Liveness-gated steal (audit M1) + steal-safety (PR #1532 review). The steal + // decision is three-way on the lock body: + // - VERIFIED-LIVE holder (parseable pid that signals alive): NEVER stolen until + // its age crosses the absolute deadman ceiling (the pid-reuse backstop) — + // nuking a slow-but-live writer's lock causes lost updates (#3711 / #500/#905/ + // #1230 family). + // - COMPLETE DEAD pid (parseable pid, not alive): stolen PROMPTLY regardless of + // age — a crashed holder left a full body. + // - EMPTY / unparseable body: liveness is unknowable. While FRESH (age <= + // freshCreateFloorMs) it is a lock still mid-creation (O_EXCL done, pid not yet + // written) and is NOT stolen (window a); only once aged past the floor is it a + // genuine orphan and stealable. + // The steal itself is an ATOMIC rename-then-recreate (only one racer can rename the + // inode) guarded by an identity re-confirm, so a racer that recreates a fresh lock + // in the decision→steal gap never has its replacement deleted (window b). Mirrors + // capability-lock.cts:455-499. try { const stat = fs.statSync(lockPath); const ageMs = clock.now() - stat.mtimeMs; - const holderLive = _stateHolderVerifiedLive(lockPath); - if (!holderLive || ageMs > deadmanCeilingMs) { - let removed = false; - try { fs.unlinkSync(lockPath); removed = true; } catch { /* swallow: bounded below */ } - if (removed) { - // Successful steal — retry immediately to grab the just-freed lock. - // Must NOT call checkBudgetAndSleep here: a throw-after-delete would - // corrupt the filesystem state, and the budget is already bounded on - // the next iteration's EEXIST or open attempt (#1217 regression fix). + const bodyPid = _stateLockBodyPid(lockPath); + const holderLive = bodyPid !== null && _stateLockIsPidAlive(bodyPid); + let steal: boolean; + if (holderLive) { + steal = ageMs > deadmanCeilingMs; // pid-reuse backstop only + } else if (bodyPid !== null) { + steal = true; // complete dead pid → prompt steal + } else { + steal = ageMs > freshCreateFloorMs; // empty/garbage → protect the create window + } + if (steal) { + if (_stateLockTestHooks.beforeSteal) _stateLockTestHooks.beforeSteal({ lockPath }); + // Identity re-confirm immediately before the steal: a racer that stole + + // recreated a fresh lock in the decision→steal gap changes (dev, ino) and/or + // the body pid → do NOT delete the replacement; re-evaluate from scratch. + let confirmStat: fs.Stats; + try { + confirmStat = fs.statSync(lockPath); + } catch { + continue; // lock vanished between decision and steal — retry the create. + } + const sameInstance = + typeof stat.dev === 'number' && typeof stat.ino === 'number' && + confirmStat.dev === stat.dev && confirmStat.ino === stat.ino && + _stateLockBodyPid(lockPath) === bodyPid; + if (!sameInstance) { + // The lock changed under us (a racer won the steal + recreated). Back off + // and re-evaluate rather than deleting the racer's fresh replacement. + checkBudgetAndSleep('lock changed before steal'); continue; } - // Persistent unlinkSync failure — apply budget + backoff so it cannot - // busy-spin (#1217). - checkBudgetAndSleep('stale lock removal failed'); + // Atomic steal: rename the inode aside, then remove it. Only ONE racer can + // win the rename; a failed rename means another process already stole it, so + // we must NOT fall through to a delete — back off and retry the create. + const stolen = lockPath + '.stale-' + process.pid + '-' + clock.now() + '-' + (_stateStealSeq++); + let renamed = false; + try { fs.renameSync(lockPath, stolen); renamed = true; } catch { /* another racer won */ } + if (renamed) { + try { fs.rmSync(stolen, { force: true }); } catch { /* best-effort */ } + // Successful steal — retry immediately to grab the just-freed lock. + // Must NOT call checkBudgetAndSleep here: a throw-after-rename would + // corrupt filesystem state, and the budget is already bounded on the next + // iteration's EEXIST or open attempt (#1217 regression fix). + continue; + } + // Lost the steal race (or a transient rename failure) — apply budget + backoff + // so it cannot busy-spin (#1217). + checkBudgetAndSleep('stale lock steal lost to racer'); continue; } } catch (err) { - // Re-throw a budget-exceeded error from the unlinkSync failure path above - // unchanged — its message already names the real cause ("stale lock removal - // failed") and double-wrapping it would replace that with the misleading - // "statSync failed after EEXIST" context string (#1217 diagnostic fix). + // Re-throw a budget-exceeded error from the steal path above unchanged — its + // message already names the real cause ("lock changed before steal" / "stale + // lock steal lost to racer") and double-wrapping it would replace that with the + // misleading "statSync failed after EEXIST" context string (#1217 diagnostic fix). if ((err as Record)?.lockBudgetExceeded) throw err; // statSync failed — lock was likely released between our EEXIST and this // stat call. Apply budget + backoff so a persistent statSync failure @@ -3040,10 +3113,12 @@ export = { if ('afterAcquire' in hooks) _stateLockTestHooks.afterAcquire = hooks.afterAcquire; if ('simulateWriteError' in hooks) _stateLockTestHooks.simulateWriteError = hooks.simulateWriteError; if ('onLoopIteration' in hooks) _stateLockTestHooks.onLoopIteration = hooks.onLoopIteration; + if ('beforeSteal' in hooks) _stateLockTestHooks.beforeSteal = hooks.beforeSteal; }, _resetStateLockTestHooks(): void { delete _stateLockTestHooks.afterAcquire; delete _stateLockTestHooks.simulateWriteError; delete _stateLockTestHooks.onLoopIteration; + delete _stateLockTestHooks.beforeSteal; }, }; diff --git a/tests/clock-seam.test.cjs b/tests/clock-seam.test.cjs index 334cb70e9..c4fd34470 100644 --- a/tests/clock-seam.test.cjs +++ b/tests/clock-seam.test.cjs @@ -223,6 +223,122 @@ describe('acquireStateLock PID-liveness staleness (audit M1)', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// 1c. Steal-safety windows (PR #1532 review — trek-e) +// +// The PID-liveness backport (audit M1) dropped two pieces of capability-lock.cts's +// race-free steal machinery, reopening the #500/#905/#1230 lost-update family: +// +// (a) Empty-body create window — acquireStateLock creates the lock with O_EXCL and +// writes the pid in a SEPARATE writeSync. A lock observed in that window has an +// EMPTY body → _stateHolderVerifiedLive('') is false → the no-floor steal gate +// robs it at age ≈ 0, mid-creation. capability-lock never steals a FRESH lock +// (age <= LOCK_STALE_MS) regardless of body, which is what protects that window. +// +// (b) Double-steal — the steal is a bare fs.unlinkSync with no identity re-confirm +// between the decision and the unlink. A racer that steals + recreates a fresh +// lock in that gap has its replacement deleted by the first stealer's unlink → +// two concurrent holders. capability-lock re-confirms (dev,ino) immediately +// before an ATOMIC rename-steal so only one racer can win. +// +// Both are driven deterministically through the lock seams (clock + pid probe + +// onLoopIteration + beforeSteal) — no wall-clock, no real concurrency. +// ───────────────────────────────────────────────────────────────────────────── + +describe('acquireStateLock steal-safety windows (PR #1532)', () => { + let tmpDir; + let statePath; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-stealsafety-state-')); + fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true }); + statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, '# State\n'); + }); + + afterEach(() => { + stateMod._resetLockProbes(); + stateMod._resetStateLockTestHooks(); + try { fs.unlinkSync(statePath + '.lock'); } catch { /* ok */ } + cleanup(tmpDir); + }); + + test('a FRESH empty-body lock (mid-creation) is NOT stolen at age ~0 — acquirer backs off', () => { + const lockPath = statePath + '.lock'; + // Simulate the create→pid-write window of a CONCURRENT acquirer: the lockfile + // exists (O_EXCL create succeeded) but the pid has not been written yet → empty body. + fs.writeFileSync(lockPath, ''); + const freshTime = new Date(); + fs.utimesSync(lockPath, freshTime, freshTime); // mtime ≈ now → age ≈ 0 (fresh) + + // The body is empty, so liveness cannot be determined from it — the probe value is + // irrelevant. The (buggy) no-floor gate steals it regardless; the fix must wait. + stateMod._setLockProbes({ isPidAlive: () => false }); + + // After the first encounter, clear the empty lock so the (correctly-waiting) acquirer + // can complete instead of budgeting out — keeps the test bounded and the assertion + // about the FIRST decision, not the eventual outcome. + stateMod._setStateLockTestHooks({ + onLoopIteration: ({ iteration }) => { + if (iteration >= 1) { try { fs.unlinkSync(lockPath); } catch { /* already gone */ } } + }, + }); + + const clock = makeFakeClock(freshTime.getTime()); + const acquired = acquireStateLock(statePath, clock); + + assert.ok(fs.existsSync(acquired), 'lock must eventually be acquired'); + assert.ok( + clock.sleepCalls.length >= 1, + 'a fresh empty-body lock is mid-creation and must NOT be stolen at age ~0 — ' + + 'the acquirer must back off (sleep) at least once, not unlink + steal immediately' + ); + releaseStateLock(acquired); + }); + + test('a dead holder whose lock is recreated by a racer mid-steal is NOT double-stolen (identity re-confirm)', () => { + const lockPath = statePath + '.lock'; + const deadPid = 4040; + const livePid = 5050; + // Decision-time holder: a DEAD pid → eligible for steal. + fs.writeFileSync(lockPath, String(deadPid)); + const t = new Date(); + fs.utimesSync(lockPath, t, t); + + stateMod._setLockProbes({ isPidAlive: (pid) => pid === livePid }); + + // Inject a concurrent waiter that, in the gap between our steal-DECISION and our + // steal, already stole + recreated a FRESH lock owned by a LIVE pid. A correct + // (identity-re-confirming) acquirer must notice the lock instance changed and must + // NOT delete the racer's live replacement. + let injected = false; + stateMod._setStateLockTestHooks({ + beforeSteal: () => { + if (injected) return; + injected = true; + try { fs.unlinkSync(lockPath); } catch { /* ok */ } + fs.writeFileSync(lockPath, String(livePid)); // different identity + live holder + const f = new Date(); + fs.utimesSync(lockPath, f, f); + }, + }); + + const clock = makeFakeClock(t.getTime()); + // The racer's replacement is held by a LIVE pid → the acquirer must wait on it and + // budget out rather than stealing it. (A double-steal would instead delete it and + // succeed.) + assert.throws( + () => acquireStateLock(statePath, clock), + (err) => err && err.lockBudgetExceeded === true, + 'acquirer must not double-steal the racer\'s live replacement — it must wait + budget out' + ); + assert.strictEqual( + fs.readFileSync(lockPath, 'utf-8'), String(livePid), + 'the racer\'s freshly-recreated live lock must survive — never deleted by a stale-decision unlink' + ); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // 1b. Regression #1217 — acquireStateLock ENOENT (recoverable errno) busy-spin // @@ -477,11 +593,12 @@ describe('acquireStateLock boundary coverage — recoverable-errno budget (#1217 // before continuing, so they throw within maxWaitMs. // ───────────────────────────────────────────────────────────────────────────── -describe('acquireStateLock statSync/unlinkSync spin paths bounded (#1217)', () => { +describe('acquireStateLock statSync/steal spin paths bounded (#1217)', () => { let tmpDir; let statePath; let origStatSync; let origUnlinkSync; + let origRenameSync; beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-clock-spin-')); @@ -490,11 +607,17 @@ describe('acquireStateLock statSync/unlinkSync spin paths bounded (#1217)', () = fs.writeFileSync(statePath, '# State\n'); origStatSync = fs.statSync; origUnlinkSync = fs.unlinkSync; + origRenameSync = fs.renameSync; + // Force the recorded holder (pid 99999) DEAD so the steal path is exercised + // deterministically — these tests probe the steal's bounded-backoff, not liveness. + stateMod._setLockProbes({ isPidAlive: () => false }); }); afterEach(() => { fs.statSync = origStatSync; fs.unlinkSync = origUnlinkSync; + fs.renameSync = origRenameSync; + stateMod._resetLockProbes(); try { fs.unlinkSync(statePath + '.lock'); } catch { /* ok */ } cleanup(tmpDir); }); @@ -534,29 +657,26 @@ describe('acquireStateLock statSync/unlinkSync spin paths bounded (#1217)', () = try { origUnlinkSync(lockPath); } catch { /* ok */ } }); - test('persistent unlinkSync failure in stale-lock path throws budget-exceeded (not busy-spin)', () => { - // Set up an EEXIST condition with a STALE lock (mtime well in the past) + test('persistent renameSync failure in steal path throws budget-exceeded (not busy-spin)', () => { + // Set up an EEXIST condition with a steal-eligible DEAD holder (pid 99999 — not us, + // not alive). The steal is an ATOMIC rename (PR #1532); a persistent rename failure + // (e.g. EPERM — file locked by an AV scanner) must back off + budget out, not spin. const lockPath = statePath + '.lock'; fs.writeFileSync(lockPath, '99999'); - // Back-date mtime by 15 000 ms so the stale-threshold (10 000 ms) is exceeded - const staleMs = 15000; - const staledTime = new Date(Date.now() - staleMs); - fs.utimesSync(lockPath, staledTime, staledTime); - // Make unlinkSync always fail (e.g. EPERM — file locked by AV scanner) - const unlinkErr = Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' }); - fs.unlinkSync = (p) => { - if (p === lockPath) throw unlinkErr; - return origUnlinkSync(p); + // Make renameSync always fail for the steal of our lock path. + const renameErr = Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' }); + fs.renameSync = (from, to) => { + if (from === lockPath) throw renameErr; + return origRenameSync(from, to); }; - // Clock where now() returns current real time so the stale check fires, + // Clock where now() returns current real time so the steal branch fires, // but sleep advances a fixed 1000ms per call so budget is hit deterministically. const realNow = Date.now(); let _elapsed = 0; const sleepCalls = []; const clock = { - // Return a time far past the stale threshold so the stale branch is taken now() { return realNow + _elapsed; }, sleep(ms) { sleepCalls.push(ms); _elapsed += 1000; }, }; @@ -564,34 +684,31 @@ describe('acquireStateLock statSync/unlinkSync spin paths bounded (#1217)', () = assert.throws( () => acquireStateLock(statePath, clock), /acquireStateLock.*exceeded.*30000ms budget/, - 'persistent unlinkSync failure in stale-lock path must throw budget-exceeded, not spin forever' + 'persistent renameSync failure in steal path must throw budget-exceeded, not spin forever' ); assert.ok(sleepCalls.length >= 1, `sleep must have been called at least once (got ${sleepCalls.length}); zero means busy-spin`); assert.ok(_elapsed >= 30000, `elapsed must reach 30 000 ms budget (got ${_elapsed}ms)`); - // Restore unlinkSync for cleanup - fs.unlinkSync = origUnlinkSync; + // Restore renameSync for cleanup + fs.renameSync = origRenameSync; try { origUnlinkSync(lockPath); } catch { /* ok */ } }); - test('persistent unlinkSync failure error message names stale-lock-removal cause, not statSync (#1217 diagnostic)', () => { - // Regression guard for the misleading-error-context bug: when unlinkSync - // fails on the stale-lock path and checkBudgetAndSleep throws at the budget - // boundary, the outer statSync catch must NOT re-wrap it with - // "statSync failed after EEXIST". The thrown error must contain the original - // context "stale lock removal failed" so operators can identify the real cause. + test('persistent renameSync failure error message names steal cause, not statSync (#1217 diagnostic)', () => { + // Regression guard for the misleading-error-context bug: when the steal's renameSync + // fails and checkBudgetAndSleep throws at the budget boundary, the outer statSync + // catch must NOT re-wrap it with "statSync failed after EEXIST". The thrown error + // must name the real cause ("stale lock steal lost to racer") so operators can + // identify it. const lockPath = statePath + '.lock'; fs.writeFileSync(lockPath, '99999'); - const staleMs = 15000; - const staledTime = new Date(Date.now() - staleMs); - fs.utimesSync(lockPath, staledTime, staledTime); - // unlinkSync always fails — the budget will be exhausted on the first sleep. - const unlinkErr = Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' }); - fs.unlinkSync = (p) => { - if (p === lockPath) throw unlinkErr; - return origUnlinkSync(p); + // renameSync always fails — the budget will be exhausted on the first sleep. + const renameErr = Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' }); + fs.renameSync = (from, to) => { + if (from === lockPath) throw renameErr; + return origRenameSync(from, to); }; const realNow = Date.now(); @@ -608,17 +725,17 @@ describe('acquireStateLock statSync/unlinkSync spin paths bounded (#1217)', () = thrownErr = e; } - assert.ok(thrownErr, 'must throw when unlinkSync persistently fails and budget is exhausted'); + assert.ok(thrownErr, 'must throw when renameSync persistently fails and budget is exhausted'); assert.ok( - /stale lock removal failed/.test(thrownErr.message), - `error message must contain "stale lock removal failed" (got: ${thrownErr.message})` + /stale lock steal lost to racer/.test(thrownErr.message), + `error message must contain "stale lock steal lost to racer" (got: ${thrownErr.message})` ); assert.ok( !/statSync failed after EEXIST/.test(thrownErr.message), `error message must NOT contain "statSync failed after EEXIST" (the misleading re-wrap) (got: ${thrownErr.message})` ); - fs.unlinkSync = origUnlinkSync; + fs.renameSync = origRenameSync; try { origUnlinkSync(lockPath); } catch { /* ok */ } }); diff --git a/tests/planning-workspace.test.cjs b/tests/planning-workspace.test.cjs index 73ef70898..4e3cbd850 100644 --- a/tests/planning-workspace.test.cjs +++ b/tests/planning-workspace.test.cjs @@ -216,10 +216,58 @@ describe('withPlanningLock PID-liveness staleness + EEXIST safety (audit M1+M2)' afterEach(() => { planningWorkspaceDirect._resetLockProbes(); + if (typeof planningWorkspaceDirect._resetPlanningLockTestHooks === 'function') { + planningWorkspaceDirect._resetPlanningLockTestHooks(); + } try { fs.unlinkSync(lockPath); } catch { /* ok */ } cleanup(tmpDir); }); + test('a dead holder recreated by a racer mid-steal is NOT double-stolen (identity re-confirm — PR #1532)', () => { + const deadPid = 4040; + const livePid = 5050; + // Decision-time holder: a DEAD pid → eligible for steal inside the polite loop. + fs.writeFileSync(lockPath, JSON.stringify({ + pid: deadPid, + cwd: tmpDir, + acquired: new Date().toISOString(), + })); + + planningWorkspaceDirect._setLockProbes({ isPidAlive: (pid) => pid === livePid }); + + // Inject a concurrent waiter that, in the gap between our steal-DECISION and our + // steal, already stole + recreated a FRESH lock owned by a LIVE pid. A correct + // (identity-re-confirming) acquirer must notice the instance changed and must NOT + // delete the racer's live replacement. + let injected = false; + planningWorkspaceDirect._setPlanningLockTestHooks({ + beforeSteal: () => { + if (injected) return; + injected = true; + try { fs.unlinkSync(lockPath); } catch { /* ok */ } + fs.writeFileSync(lockPath, JSON.stringify({ + pid: livePid, + cwd: tmpDir, + acquired: new Date().toISOString(), + })); + }, + }); + + let ranCriticalSection = false; + const clock = makeFakeClock(0); + // The racer's replacement is held by a LIVE pid → the acquirer must wait on it and + // budget out, NOT delete it and run the critical section (which a double-steal does). + assert.throws( + () => withPlanningLock(tmpDir, () => { ranCriticalSection = true; return 'x'; }, clock), + (err) => err && err.lockTimeout === true, + 'acquirer must not double-steal the racer\'s live replacement — it must wait + time out' + ); + assert.strictEqual(ranCriticalSection, false, 'critical section must NOT run — the live replacement was not stolen'); + assert.ok(fs.existsSync(lockPath), 'the racer\'s live replacement lock must survive'); + const body = JSON.parse(fs.readFileSync(lockPath, 'utf-8')); + assert.strictEqual(body.pid, livePid, 'the racer\'s freshly-recreated live lock body must be intact (never deleted by a stale-decision unlink)'); + }); + test('exports _setLockProbes / _resetLockProbes seams', () => { assert.ok(typeof planningWorkspaceDirect._setLockProbes === 'function', '_setLockProbes seam must be exported'); assert.ok(typeof planningWorkspaceDirect._resetLockProbes === 'function', '_resetLockProbes seam must be exported');