fix: PID-liveness gate for the two core-path file locks (audit M1+M2)
The STATE.md write lock (acquireStateLock) and the .planning/ workspace lock (withPlanningLock) stole contended locks on mtime age alone with no process.kill(pid,0) liveness check, and mis-ordered stale-vs-wait so a live-but-slow holder could be robbed mid-critical-section. M1 (lost update / STATE.md corruption): a live writer whose critical section ran past the stale threshold aged out and a waiter unlinked its lock and acquired -> two writers in STATE.md's read-modify-write window. mtime is a leaky proxy for "holder is alive"; it leaks under exactly the slow-holder condition the lock guards against. M2 (uncaught EEXIST): withPlanningLock's timeout fallback unconditionally unlinked whatever lock existed (even a live holder's) and re-acquired OUTSIDE any try -- a concurrent re-create raced a raw EEXIST out of the helper. Fix backports capability-lock.cts's liveness gate (process.kill(pid,0) via a _setLockProbes/_resetLockProbes test seam): - acquireStateLock: steal when holder pid is DEAD (any age) OR age exceeds a deadman ceiling (60000ms, ABOVE maxWaitMs=30000) so a verified-live holder is never stolen within budget; garbage/legacy bodies stay recoverable. - withPlanningLock: same gate in the EEXIST path (dead stolen promptly, live waited on); removed the unconditional force-steal -> clear timeout throw, which also closes M2 (no re-acquire outside try). Uncontended path unchanged byte-for-behaviour; realClock + real process.kill remain the defaults. Pid-reuse residual fails safe (waits/times out, never corrupts) and recovers at the deadman ceiling. Tests: TDD red->green via the clock + new pid-liveness probe seams (no wall-clock; #453 deleted the race tests). 8 new behavioural tests across tests/clock-seam.test.cjs and tests/planning-workspace.test.cjs; the prior withPlanningLock timeout test rewritten to pin the no-force-steal contract. Claude-Session: https://claude.ai/code/session_01R88n7Q54bAaVHFkDbbH1yz
This commit is contained in:
Dave
@@ -36,7 +36,8 @@ const path = require('node:path');
|
||||
const os = require('node:os');
|
||||
|
||||
const { makeFakeClock } = require('./helpers/clock.cjs');
|
||||
const { acquireStateLock, releaseStateLock, readModifyWriteStateMd } = require('../gsd-core/bin/lib/state.cjs');
|
||||
const stateMod = require('../gsd-core/bin/lib/state.cjs');
|
||||
const { acquireStateLock, releaseStateLock, readModifyWriteStateMd } = stateMod;
|
||||
const { withPlanningLock } = require('../gsd-core/bin/lib/planning-workspace.cjs');
|
||||
const { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs');
|
||||
|
||||
@@ -123,6 +124,105 @@ describe('acquireStateLock clock seam', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// 1a. acquireStateLock PID-liveness staleness (audit M1)
|
||||
//
|
||||
// mtime is a leaky proxy for "holder is alive": a live-but-slow holder whose
|
||||
// critical section runs past staleThresholdMs ages out and gets its lock stolen
|
||||
// by a waiter → two writers in STATE.md's critical section → lost update.
|
||||
// The fix gates the steal on a real liveness signal (process.kill(pid,0),
|
||||
// injected via the _setLockProbes seam) and orders the deadman ceiling ABOVE the
|
||||
// wait budget so a verified-live holder is NEVER stolen within budget. A dead
|
||||
// holder is stolen promptly regardless of age. A garbage/legacy body is treated
|
||||
// as not-verified-live so corrupt locks stay recoverable under the deadman ceiling.
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('acquireStateLock PID-liveness staleness (audit M1)', () => {
|
||||
let tmpDir;
|
||||
let statePath;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-liveness-state-'));
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true });
|
||||
statePath = path.join(tmpDir, '.planning', 'STATE.md');
|
||||
fs.writeFileSync(statePath, '# State\n');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
stateMod._resetLockProbes();
|
||||
try { fs.unlinkSync(statePath + '.lock'); } catch { /* ok */ }
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('exports _setLockProbes / _resetLockProbes seams', () => {
|
||||
assert.ok(typeof stateMod._setLockProbes === 'function', '_setLockProbes seam must be exported');
|
||||
assert.ok(typeof stateMod._resetLockProbes === 'function', '_resetLockProbes seam must be exported');
|
||||
});
|
||||
|
||||
test('live holder is NOT stolen even when aged past the stale threshold (waiter budgets out)', () => {
|
||||
const lockPath = statePath + '.lock';
|
||||
const livePid = 4242;
|
||||
fs.writeFileSync(lockPath, String(livePid));
|
||||
|
||||
// Holder pid reads as ALIVE via the injected probe (deterministic, no real pid).
|
||||
stateMod._setLockProbes({ isPidAlive: (pid) => pid === livePid });
|
||||
|
||||
// Drive the clock so the lock is aged WELL past the 10 000 ms stale threshold
|
||||
// (stale < age) but the waiter only ever budgets out at maxWaitMs (30 000 ms).
|
||||
// sleep advances time; once the 30 000 ms budget is exhausted it must throw,
|
||||
// and it must NOT have unlinked the live holder's lock.
|
||||
const clock = makeFakeClock(60000); // age = now - mtime ≫ 10 000 ms
|
||||
assert.throws(
|
||||
() => acquireStateLock(statePath, clock),
|
||||
/acquireStateLock.*exceeded.*30000ms budget/,
|
||||
'a verified-live holder must never be stolen within the wait budget — waiter must time out instead'
|
||||
);
|
||||
|
||||
// The live holder's lock body must be intact (never unlinked + re-created).
|
||||
assert.ok(fs.existsSync(lockPath), 'live holder lock must still exist (not stolen)');
|
||||
assert.strictEqual(fs.readFileSync(lockPath, 'utf-8'), String(livePid), 'live holder lock body must be unchanged');
|
||||
|
||||
fs.unlinkSync(lockPath);
|
||||
});
|
||||
|
||||
test('dead holder is stolen promptly without waiting out the full budget', () => {
|
||||
const lockPath = statePath + '.lock';
|
||||
const deadPid = 777;
|
||||
fs.writeFileSync(lockPath, String(deadPid));
|
||||
|
||||
// Holder pid reads as DEAD via the injected probe → eligible for immediate steal.
|
||||
stateMod._setLockProbes({ isPidAlive: () => false });
|
||||
|
||||
// Fresh, NON-aged lock (mtime ≈ now). Without liveness the old mtime-only gate
|
||||
// would refuse to steal a <10 000 ms lock and force a long wait; with liveness
|
||||
// a dead holder is stolen immediately regardless of age.
|
||||
const clock = makeFakeClock(Date.now());
|
||||
const acquired = acquireStateLock(statePath, clock);
|
||||
assert.ok(fs.existsSync(acquired), 'dead holder lock must be stolen and re-acquired');
|
||||
assert.strictEqual(
|
||||
clock.sleepCalls.length, 0,
|
||||
'a dead holder must be stolen promptly — no wait/backoff sleeps before acquisition'
|
||||
);
|
||||
releaseStateLock(acquired);
|
||||
});
|
||||
|
||||
test('garbage/legacy lock body → not-verified-live → recoverable under the deadman ceiling, never an infinite block', () => {
|
||||
const lockPath = statePath + '.lock';
|
||||
fs.writeFileSync(lockPath, 'not-a-pid\x00garbage'); // unreadable / non-numeric body
|
||||
|
||||
// Probe would say "alive" for ANY pid — proves the steal does not depend on a
|
||||
// bogus parse succeeding: an unparseable body is treated as not-verified-live.
|
||||
stateMod._setLockProbes({ isPidAlive: () => true });
|
||||
|
||||
// Age the body past the deadman ceiling (above maxWaitMs) so the corrupt lock
|
||||
// is recoverable rather than blocking forever.
|
||||
const clock = makeFakeClock(Date.now() + 120000);
|
||||
const acquired = acquireStateLock(statePath, clock);
|
||||
assert.ok(fs.existsSync(acquired), 'corrupt/legacy lock must be recoverable (stolen under the deadman ceiling)');
|
||||
releaseStateLock(acquired);
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// 1b. Regression #1217 — acquireStateLock ENOENT (recoverable errno) busy-spin
|
||||
//
|
||||
@@ -649,58 +749,38 @@ describe('withPlanningLock clock seam', () => {
|
||||
assert.ok(!fs.existsSync(path.join(tmpDir, '.planning', '.lock')), 'lock must be released even when fn() throws');
|
||||
});
|
||||
|
||||
test('timeout fires when clock exceeds lockTimeout (10 000 ms)', () => {
|
||||
test('timeout fires (sleep seam exercised) when a LIVE holder is contended past lockTimeout', () => {
|
||||
// Audit M1 rewrite: the prior version asserted the now-REMOVED force-steal
|
||||
// fallback (timeout → unconditional unlink + re-acquire). That fallback robbed
|
||||
// live writers; the fix replaces it with a clear timeout throw. This test now
|
||||
// pins the new contract: a verified-LIVE holder held past lockTimeout makes the
|
||||
// waiter exercise the clock.sleep seam and then throw — never force-stolen.
|
||||
const lockPath = path.join(tmpDir, '.planning', '.lock');
|
||||
fs.writeFileSync(lockPath, String(process.pid)); // simulate held lock
|
||||
const livePid = 9191;
|
||||
fs.writeFileSync(lockPath, JSON.stringify({ pid: livePid, cwd: tmpDir, acquired: new Date().toISOString() }));
|
||||
|
||||
// Holder reads as ALIVE via the injected probe → waited on, never stolen.
|
||||
require('../gsd-core/bin/lib/planning-workspace.cjs')._setLockProbes({ isPidAlive: (pid) => pid === livePid });
|
||||
|
||||
// Clock that advances past lockTimeout on every sleep call so the while
|
||||
// condition trips immediately after the first retry.
|
||||
let nowValue = 0;
|
||||
|
||||
// withPlanningLock exits the while loop (timeout), deletes the lock, then
|
||||
// calls runWithHeldLock() which tries writeFileSync with { flag: 'wx' }.
|
||||
// Since our lock file is still there (we placed it), runWithHeldLock throws EEXIST.
|
||||
// That exception propagates — so we get an error (either EEXIST or the
|
||||
// function succeeds on the post-timeout acquisition attempt depending on timing).
|
||||
// What we need to assert: the clock.sleep was invoked (timeout path was reached).
|
||||
//
|
||||
// Because withPlanningLock removes the lock file at timeout and re-acquires,
|
||||
// and we placed the lock file ourselves (not via withPlanningLock), the re-acquire
|
||||
// will SUCCEED (wx open on an absent file). So the function returns normally.
|
||||
// Remove our self-placed lock so withPlanningLock can take it over.
|
||||
fs.unlinkSync(lockPath);
|
||||
|
||||
// Now seed the lock AFTER withPlanningLock starts by using a wrapper that
|
||||
// creates the lock file on the first sleep call.
|
||||
let seeded = false;
|
||||
nowValue = 0;
|
||||
const clock2 = {
|
||||
now() { return nowValue; },
|
||||
sleep(ms) {
|
||||
if (!seeded) {
|
||||
seeded = true;
|
||||
// The test: verify withPlanningLock calls clock.sleep when contended
|
||||
// (confirms the seam is wired, not that Atomics.wait is called).
|
||||
}
|
||||
nowValue += ms + 11000;
|
||||
},
|
||||
sleep(ms) { nowValue += ms + 11000; }, // advance past lockTimeout on first sleep
|
||||
};
|
||||
|
||||
// Re-seed the lock (simulating a competing process)
|
||||
fs.writeFileSync(lockPath, '12345'); // non-existent PID; stale check uses mtime
|
||||
|
||||
// Set mtime to now so the stale check (>30s) does NOT fire
|
||||
const now = new Date();
|
||||
fs.utimesSync(lockPath, now, now);
|
||||
|
||||
// With the lock fresh and held, withPlanningLock will enter the retry loop
|
||||
// and call clock2.sleep at least once. After advancing past lockTimeout,
|
||||
// it exits the while loop and tries to recover by unlinking and re-acquiring.
|
||||
const result = withPlanningLock(tmpDir, () => 'recovered', clock2);
|
||||
assert.strictEqual(result, 'recovered', 'must succeed after timeout recovery path');
|
||||
// clock2.sleep was called, confirming the seam was exercised
|
||||
// (the sleep method must have advanced nowValue past lockTimeout)
|
||||
assert.ok(nowValue > 10000, 'clock must have advanced past lockTimeout via sleep calls');
|
||||
try {
|
||||
assert.throws(
|
||||
() => withPlanningLock(tmpDir, () => 'should-not-run', clock2),
|
||||
/exceeded.*10000ms budget/,
|
||||
'a live holder held past lockTimeout must throw a clear timeout error (not force-steal)'
|
||||
);
|
||||
// The sleep seam must have been exercised (timeout path reached).
|
||||
assert.ok(nowValue > 10000, 'clock must have advanced past lockTimeout via the sleep seam');
|
||||
// The live holder's lock must be intact (never unlinked).
|
||||
assert.ok(fs.existsSync(lockPath), 'live holder lock must survive the timeout (not force-stolen)');
|
||||
} finally {
|
||||
require('../gsd-core/bin/lib/planning-workspace.cjs')._resetLockProbes();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -4,6 +4,9 @@ const fs = require('fs');
|
||||
const os = require('os');
|
||||
const path = require('path');
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
const { makeFakeClock } = require('./helpers/clock.cjs');
|
||||
|
||||
const planningWorkspaceDirect = require('../gsd-core/bin/lib/planning-workspace.cjs');
|
||||
|
||||
const {
|
||||
createPlanningWorkspace,
|
||||
@@ -13,9 +16,7 @@ const {
|
||||
withPlanningLock,
|
||||
getActiveWorkstream,
|
||||
setActiveWorkstream,
|
||||
} = require('../gsd-core/bin/lib/planning-workspace.cjs');
|
||||
|
||||
const planningWorkspaceDirect = require('../gsd-core/bin/lib/planning-workspace.cjs');
|
||||
} = planningWorkspaceDirect;
|
||||
|
||||
describe('planning-workspace: planningDir/planningPaths parity', () => {
|
||||
const cwd = '/fake/repo';
|
||||
@@ -185,3 +186,107 @@ describe('planning-workspace direct: functions expose matching behavior', () =>
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// withPlanningLock PID-liveness staleness + EEXIST safety (audit M1 + M2)
|
||||
//
|
||||
// M1: the prior timeout fallback unconditionally unlinked WHATEVER lock existed —
|
||||
// even a fresh, live holder's — then re-acquired. A legitimate op taking
|
||||
// longer than lockTimeout (10 000 ms) got its lock force-stolen. The fix gates
|
||||
// stealing on a real liveness signal (injected via _setLockProbes): a dead
|
||||
// holder is stolen promptly inside the polite loop; a LIVE holder is waited on
|
||||
// and, on genuine timeout, the waiter throws a clear timeout error rather than
|
||||
// corrupting the live holder's critical section.
|
||||
//
|
||||
// M2: the timeout-fallback re-acquire (acquireLock with { flag: 'wx' }) sat OUTSIDE
|
||||
// any try/catch — if another process re-created the lock between the unlink and
|
||||
// the wx write, a raw EEXIST escaped the helper and crashed the command. The
|
||||
// fix removes the unconditional force-steal so no raw EEXIST can escape.
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('withPlanningLock PID-liveness staleness + EEXIST safety (audit M1+M2)', () => {
|
||||
let tmpDir;
|
||||
let lockPath;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-liveness-planning-'));
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true });
|
||||
lockPath = path.join(tmpDir, '.planning', '.lock');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
planningWorkspaceDirect._resetLockProbes();
|
||||
try { fs.unlinkSync(lockPath); } catch { /* ok */ }
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
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');
|
||||
});
|
||||
|
||||
test('live holder held past lockTimeout is NOT force-stolen — waiter throws a clear timeout error', () => {
|
||||
const livePid = 5151;
|
||||
fs.writeFileSync(lockPath, JSON.stringify({
|
||||
pid: livePid,
|
||||
cwd: tmpDir,
|
||||
acquired: new Date().toISOString(),
|
||||
}));
|
||||
|
||||
// Holder pid reads as ALIVE → must never be force-stolen.
|
||||
planningWorkspaceDirect._setLockProbes({ isPidAlive: (pid) => pid === livePid });
|
||||
|
||||
let ranCriticalSection = false;
|
||||
// Fake clock whose sleep advances past lockTimeout (10 000 ms) so the polite
|
||||
// loop budgets out; the live holder must survive and the waiter must throw.
|
||||
const clock = makeFakeClock(0);
|
||||
assert.throws(
|
||||
() => withPlanningLock(tmpDir, () => { ranCriticalSection = true; return 'stolen'; }, clock),
|
||||
/lock/i,
|
||||
'a live holder must never be force-stolen on timeout — the waiter must throw a clear timeout error'
|
||||
);
|
||||
|
||||
assert.strictEqual(ranCriticalSection, false, 'critical section must NOT run against a live holder (no force-steal)');
|
||||
assert.ok(fs.existsSync(lockPath), 'live holder lock must still exist (not unlinked)');
|
||||
const body = JSON.parse(fs.readFileSync(lockPath, 'utf-8'));
|
||||
assert.strictEqual(body.pid, livePid, 'live holder lock body must be unchanged');
|
||||
});
|
||||
|
||||
test('dead holder is stolen promptly inside the polite loop (no full timeout wait)', () => {
|
||||
const deadPid = 888;
|
||||
fs.writeFileSync(lockPath, JSON.stringify({
|
||||
pid: deadPid,
|
||||
cwd: tmpDir,
|
||||
acquired: new Date().toISOString(),
|
||||
}));
|
||||
|
||||
// Holder pid reads as DEAD → eligible for prompt steal inside the loop.
|
||||
planningWorkspaceDirect._setLockProbes({ isPidAlive: () => false });
|
||||
|
||||
const clock = makeFakeClock(0);
|
||||
const result = withPlanningLock(tmpDir, () => 'acquired', clock);
|
||||
assert.strictEqual(result, 'acquired', 'dead holder lock must be stolen and the critical section must run');
|
||||
assert.ok(!fs.existsSync(lockPath), 'lock must be released after the critical section completes');
|
||||
});
|
||||
|
||||
test('M2: no raw EEXIST escapes the helper on the timeout path against a live holder', () => {
|
||||
const livePid = 6262;
|
||||
fs.writeFileSync(lockPath, JSON.stringify({
|
||||
pid: livePid,
|
||||
cwd: tmpDir,
|
||||
acquired: new Date().toISOString(),
|
||||
}));
|
||||
|
||||
planningWorkspaceDirect._setLockProbes({ isPidAlive: (pid) => pid === livePid });
|
||||
|
||||
const clock = makeFakeClock(0);
|
||||
let caught;
|
||||
try {
|
||||
withPlanningLock(tmpDir, () => 'x', clock);
|
||||
} catch (err) {
|
||||
caught = err;
|
||||
}
|
||||
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)');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user