diff --git a/src/planning-workspace.cts b/src/planning-workspace.cts index b1ca382f4..a8d11a718 100644 --- a/src/planning-workspace.cts +++ b/src/planning-workspace.cts @@ -165,6 +165,12 @@ function withPlanningLock(cwd: string, fn: () => T, clock?: Clock): T { if (clock === undefined) clock = realClock; const lockPath = path.join(planningDir(cwd), '.lock'); const lockTimeout = 10000; // 10 seconds + // Deadman ceiling (audit M1 / R4-FIX) — set ABOVE lockTimeout so a holder that reads + // as alive but is actually a pid-reuse alias (the .lock body has no startTime, so + // liveness alone cannot detect reuse) is still recovered once its lock ages past this + // absolute ceiling. Without it, a false-alive holder would make withPlanningLock throw + // on every call with no self-heal. Mirrors acquireStateLock's deadmanCeilingMs. + const deadmanCeilingMs = 60000; const start = clock.now(); // Ensure .planning/ exists @@ -212,9 +218,17 @@ function withPlanningLock(cwd: string, fn: () => T, clock?: Clock): T { // A verified-live holder is waited on — never force-stolen — because nuking // a slow-but-live writer's lock corrupts the .planning/ critical section. try { - if (!_planningHolderVerifiedLive(lockPath)) { + 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; + stealable = age > deadmanCeilingMs; + } + if (stealable) { fs.unlinkSync(lockPath); - continue; // dead/garbage holder — retry immediately to grab the freed lock + continue; // dead/garbage/expired holder — retry immediately to grab the freed lock } } catch { continue; } diff --git a/tests/planning-workspace.test.cjs b/tests/planning-workspace.test.cjs index a996deec2..73ef70898 100644 --- a/tests/planning-workspace.test.cjs +++ b/tests/planning-workspace.test.cjs @@ -289,4 +289,26 @@ describe('withPlanningLock PID-liveness staleness + EEXIST safety (audit M1+M2)' assert.ok(caught, 'helper must surface a failure rather than silently force-stealing a live lock'); assert.notStrictEqual(caught.code, 'EEXIST', 'a raw EEXIST must never escape the lock helper (M2)'); }); + + test('R4-FIX: false-alive pid-reuse holder aged past the deadman ceiling IS stolen (self-heal)', () => { + const reusedPid = 7373; + fs.writeFileSync(lockPath, JSON.stringify({ + pid: reusedPid, + cwd: tmpDir, + acquired: new Date().toISOString(), + })); + + // Probe says the recorded pid is ALIVE — simulating pid-reuse: the original holder + // crashed but its pid was recycled by an unrelated live process. The .lock body has + // no startTime, so liveness alone cannot distinguish this from a genuine live holder. + planningWorkspaceDirect._setLockProbes({ isPidAlive: (pid) => pid === reusedPid }); + + // Lock mtime ≈ now (real); seed the fake clock ABOVE the 60 000 ms deadman ceiling so + // age = clock.now() - mtimeMs ≫ ceiling → the lock must be recovered despite "alive". + // Without the ceiling, withPlanningLock would throw on every call with no self-heal. + const clock = makeFakeClock(Date.now() + 120000); + const result = withPlanningLock(tmpDir, () => 'self-healed', clock); + assert.strictEqual(result, 'self-healed', 'a false-alive lock past the deadman ceiling must be stolen (no infinite block)'); + assert.ok(!fs.existsSync(lockPath), 'lock must be released after the critical section completes'); + }); });