fix(core): acquireStateLock must not leak fd + orphan lock on write error (M9)
Root cause: in acquireStateLock, once openSync(O_CREAT|O_EXCL) created the lock file, the subsequent writeSync(pid)/closeSync were unguarded. A recoverable errno (EAGAIN etc., in ACQUIRE_LOCK_RETRY_ERRNOS) made the catch do checkBudgetAndSleep + continue WITHOUT closing the fd or unlinking the just-created empty lock — leaking a descriptor every occurrence and stranding a content-less lock (the #500/#905/#1230 STATE.md write-corruption family). Fix: wrap writeSync/closeSync in an inner try that guardedly closeSync(fd) + unlinkSync(lockPath) then re-throws to the existing outer catch (DRY errno classification). Recoverable errno retries from a clean slate; a FATAL errno (e.g. ENOSPC, not recoverable) still propagates after cleanup — not masked. Mirrors the already-shipped capability-lock.cts:415-425 pattern. Extends the M8 test seam with a one-shot simulateWriteError errno + an onLoopIteration snapshot hook so the orphan-before-retry is deterministically observable. New tests prove RED (orphan stranded / fatal leaves orphan) before the cleanup and GREEN after. Source of truth src/state.cts (ADR-457); bin/lib/state.cjs is generated. Claude-Session: https://claude.ai/code/session_01R88n7Q54bAaVHFkDbbH1yz
This commit is contained in:
Dave
@@ -178,23 +178,46 @@ function _realIsPidAlive(pid: number): boolean {
|
||||
const _stateLockProbes: { isPidAlive: (pid: number) => boolean } = { isPidAlive: _realIsPidAlive };
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// State-lock test hooks (test seam) — audit M8
|
||||
// State-lock test hooks (test seam) — audit M8 / M9
|
||||
//
|
||||
// M8 (scan-before-lock TOCTOU in writeStateMd) is a concurrency issue a single-
|
||||
// threaded test cannot otherwise observe. The afterAcquire hook makes the
|
||||
// failure window deterministic (mirrors the M1 _setLockProbes seam above):
|
||||
// Both M8 (scan-before-lock TOCTOU in writeStateMd) and M9 (orphan empty lock +
|
||||
// fd leak on a recoverable writeSync/closeSync error in acquireStateLock) are
|
||||
// concurrency / resource-safety issues a single-threaded test cannot otherwise
|
||||
// observe. These purpose-built hooks make the failure windows deterministic
|
||||
// (mirrors the M1 _setLockProbes seam above):
|
||||
//
|
||||
// afterAcquire(lockPath) — fired inside writeStateMd immediately AFTER the lock
|
||||
// is acquired. A test can mutate the disk here (simulate a concurrent writer
|
||||
// landing in the scan→lock window) to prove the disk scan runs INSIDE the lock.
|
||||
// simulateWriteError — a ONE-SHOT errno string. When set, the next writeSync
|
||||
// inside acquireStateLock throws it (and the hook self-clears), forcing the
|
||||
// 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.
|
||||
//
|
||||
// All hooks default to no-ops; real callers are byte-for-behaviour unchanged.
|
||||
// ---------------------------------------------------------------------------
|
||||
interface StateLockTestHooks {
|
||||
afterAcquire?: (lockPath: string) => void;
|
||||
simulateWriteError?: string | null;
|
||||
onLoopIteration?: (ctx: { iteration: number }) => void;
|
||||
}
|
||||
const _stateLockTestHooks: StateLockTestHooks = {};
|
||||
|
||||
/**
|
||||
* Consume the one-shot simulateWriteError errno, if set. Returns an Error with the
|
||||
* configured `.code` and self-clears so only the NEXT writeSync throws (the retry
|
||||
* then succeeds). Returns null when no injection is pending.
|
||||
*/
|
||||
function _consumeSimulatedWriteError(): NodeJS.ErrnoException | null {
|
||||
const code = _stateLockTestHooks.simulateWriteError;
|
||||
if (!code) return null;
|
||||
_stateLockTestHooks.simulateWriteError = null; // one-shot
|
||||
const e = new Error('simulated writeSync failure (' + code + ')') as NodeJS.ErrnoException;
|
||||
e.code = code;
|
||||
return e;
|
||||
}
|
||||
|
||||
function _stateLockIsPidAlive(pid: number): boolean {
|
||||
return _stateLockProbes.isPidAlive(pid);
|
||||
}
|
||||
@@ -1679,11 +1702,33 @@ function acquireStateLock(statePath: string, clock?: StateLockClock): string {
|
||||
clock.sleep(retryDelay + jitter);
|
||||
};
|
||||
|
||||
let _loopIteration = 0;
|
||||
while (true) {
|
||||
if (_stateLockTestHooks.onLoopIteration) _stateLockTestHooks.onLoopIteration({ iteration: _loopIteration++ });
|
||||
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);
|
||||
// Audit M9 (resource-safety): once the exclusive create SUCCEEDS, a
|
||||
// writeSync/closeSync failure must NOT leak the fd or strand the just-created
|
||||
// (now empty) lock — an orphan body self-blocks every later acquirer until a
|
||||
// liveness steal or the deadman. On any write/close error, guardedly close the
|
||||
// fd and unlink the file we created, then re-throw to the existing outer catch
|
||||
// (which keeps classifying recoverable vs fatal errnos — DRY). A FATAL errno
|
||||
// still propagates after cleanup; a RECOVERABLE one retries from a clean slate.
|
||||
// Mirrors capability-lock.cts:415-425.
|
||||
try {
|
||||
const injected = _consumeSimulatedWriteError();
|
||||
if (injected) throw injected; // test seam: one-shot writeSync failure (M9)
|
||||
fs.writeSync(fd, String(process.pid));
|
||||
fs.closeSync(fd);
|
||||
} catch (writeErr) {
|
||||
try { fs.closeSync(fd); } catch { /* best-effort — fd may already be closed */ }
|
||||
// Best-effort unlink of the lock WE just created. Guarded so we never throw
|
||||
// here; if another acquirer already stole the empty lock the unlink is a
|
||||
// harmless ENOENT no-op (we do not double-unlink someone else's lock — the
|
||||
// open(O_EXCL) above guarantees we created this path this iteration).
|
||||
try { fs.unlinkSync(lockPath); } catch { /* best-effort — no orphan */ }
|
||||
throw writeErr; // re-throw to the outer catch for recoverable/fatal classification
|
||||
}
|
||||
// Exit-time cleanup keeps a crashed locked region from leaving a stale file (#1916).
|
||||
_heldStateLocks.add(lockPath);
|
||||
return lockPath;
|
||||
@@ -2988,12 +3033,17 @@ export = {
|
||||
_resetLockProbes(): void {
|
||||
_stateLockProbes.isPidAlive = _realIsPidAlive;
|
||||
},
|
||||
// Test seam (audit M8): inject the deterministic scan-in-lock hook (afterAcquire).
|
||||
// See _stateLockTestHooks.
|
||||
// Test seam (audit M8/M9): inject deterministic hooks for the scan-in-lock window
|
||||
// (afterAcquire), the one-shot recoverable writeSync failure (simulateWriteError),
|
||||
// and per-iteration orphan-lock snapshots (onLoopIteration). See _stateLockTestHooks.
|
||||
_setStateLockTestHooks(hooks: StateLockTestHooks): void {
|
||||
if ('afterAcquire' in hooks) _stateLockTestHooks.afterAcquire = hooks.afterAcquire;
|
||||
if ('simulateWriteError' in hooks) _stateLockTestHooks.simulateWriteError = hooks.simulateWriteError;
|
||||
if ('onLoopIteration' in hooks) _stateLockTestHooks.onLoopIteration = hooks.onLoopIteration;
|
||||
},
|
||||
_resetStateLockTestHooks(): void {
|
||||
delete _stateLockTestHooks.afterAcquire;
|
||||
delete _stateLockTestHooks.simulateWriteError;
|
||||
delete _stateLockTestHooks.onLoopIteration;
|
||||
},
|
||||
};
|
||||
|
||||
142
tests/m9-statelock-write-error-orphan.test.cjs
Normal file
142
tests/m9-statelock-write-error-orphan.test.cjs
Normal file
@@ -0,0 +1,142 @@
|
||||
'use strict';
|
||||
// allow-test-rule: architectural-invariant
|
||||
// acquireStateLock's "no orphan empty lock + no fd leak on a recoverable
|
||||
// writeSync/closeSync error" property is a resource-safety invariant of a private
|
||||
// function. A single-threaded test cannot otherwise force the openSync-succeeds-
|
||||
// then-writeSync-throws window. The simulateWriteError seam injects exactly that
|
||||
// one-shot failure; the onLoopIteration seam snapshots the lock file's existence
|
||||
// at the top of the retry that follows — the only level at which the orphan is
|
||||
// observable deterministically (no wall-clock, no threads).
|
||||
|
||||
/**
|
||||
* M9 — acquireStateLock leaks the fd AND strands the just-created empty lock
|
||||
* when writeSync/closeSync throws a RECOVERABLE errno (e.g. EAGAIN) after
|
||||
* openSync(O_CREAT|O_EXCL) already created the lock file. The pre-fix catch did
|
||||
* checkBudgetAndSleep + continue WITHOUT closeSync(fd) or unlinkSync(lockPath),
|
||||
* so every occurrence leaked a descriptor and left a content-less lock behind.
|
||||
*
|
||||
* capability-lock.cts:415-425 already ships the cleanup-before-bail pattern this
|
||||
* mirrors. The fix wraps the writeSync/closeSync in an inner try that
|
||||
* closeSync(fd) (guarded) + unlinkSync(lockPath) (guarded), then re-throws to the
|
||||
* existing outer catch (which keeps classifying recoverable vs fatal errnos — DRY).
|
||||
*
|
||||
* Deterministic repro (no wall-clock, no threads):
|
||||
* - simulateWriteError: 'EAGAIN' injects a ONE-SHOT writeSync failure.
|
||||
* - onLoopIteration snapshots fs.existsSync(lockPath) at the top of each retry.
|
||||
* On the retry iteration that follows the injected error:
|
||||
* RED (pre-fix): the empty lock is still stranded → lockExists === true.
|
||||
* GREEN (post-fix): cleanup unlinked it → lockExists === false.
|
||||
* And in BOTH the call still ultimately succeeds (M1's liveness steal recovers an
|
||||
* orphan) — so the orphan PRESENCE on the retry is the discriminating signal.
|
||||
*
|
||||
* A FATAL errno (e.g. ENOSPC, not in ACQUIRE_LOCK_RETRY_ERRNOS) must still
|
||||
* propagate after cleanup — covered by the fatal-propagation test below.
|
||||
*
|
||||
* Recurring closed family this guards: #500 / #905 / #1230 (STATE.md write
|
||||
* corruption); #453 deleted the flaky race tests so this path was under-tested.
|
||||
*/
|
||||
|
||||
const { test, describe, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const os = require('node:os');
|
||||
|
||||
const { makeFakeClock } = require('./helpers/clock.cjs');
|
||||
const stateMod = require('../gsd-core/bin/lib/state.cjs');
|
||||
const { acquireStateLock, releaseStateLock } = stateMod;
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
|
||||
describe('M9: acquireStateLock cleans up fd + orphan lock on recoverable write error', () => {
|
||||
let tmpDir;
|
||||
let statePath;
|
||||
let lockPath;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-m9-'));
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true });
|
||||
statePath = path.join(tmpDir, '.planning', 'STATE.md');
|
||||
lockPath = statePath + '.lock';
|
||||
fs.writeFileSync(statePath, '# State\n');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
stateMod._resetStateLockTestHooks();
|
||||
try { fs.unlinkSync(lockPath); } catch { /* ok */ }
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('a one-shot recoverable writeSync error leaves NO stranded empty lock before the retry', () => {
|
||||
const clock = makeFakeClock(0);
|
||||
const lockExistsAtIterationTop = [];
|
||||
|
||||
stateMod._setStateLockTestHooks({
|
||||
simulateWriteError: 'EAGAIN', // one-shot: thrown by the first writeSync
|
||||
onLoopIteration() {
|
||||
lockExistsAtIterationTop.push(fs.existsSync(lockPath));
|
||||
},
|
||||
});
|
||||
|
||||
const acquired = acquireStateLock(statePath, clock);
|
||||
|
||||
// The call must still ultimately succeed and hold the lock.
|
||||
assert.equal(acquired, lockPath, 'acquireStateLock must succeed after recovering from the write error');
|
||||
assert.ok(fs.existsSync(lockPath), 'a real lock must be held when acquire returns');
|
||||
|
||||
// At least two iterations: the failing attempt, then the recovery retry.
|
||||
assert.ok(
|
||||
lockExistsAtIterationTop.length >= 2,
|
||||
'expected the injected write error to force at least one retry iteration'
|
||||
);
|
||||
|
||||
// The discriminator: on the retry that FOLLOWS the injected write error, no
|
||||
// orphan empty lock may remain. Pre-fix it is still stranded (true); post-fix
|
||||
// the inner cleanup unlinked it (false).
|
||||
assert.equal(
|
||||
lockExistsAtIterationTop[1], false,
|
||||
'the empty lock created by the failed attempt must be unlinked (cleanup-before-retry) — ' +
|
||||
'no orphan lock may be stranded after a recoverable writeSync error (M9 / capability-lock.cts:415-425)'
|
||||
);
|
||||
|
||||
releaseStateLock(acquired);
|
||||
assert.ok(!fs.existsSync(lockPath), 'lock removed after release');
|
||||
});
|
||||
|
||||
test('the held lock body is a valid pid after recovery (write actually completed on retry)', () => {
|
||||
const clock = makeFakeClock(0);
|
||||
stateMod._setStateLockTestHooks({ simulateWriteError: 'EAGAIN' });
|
||||
|
||||
const acquired = acquireStateLock(statePath, clock);
|
||||
const body = fs.readFileSync(lockPath, 'utf-8').trim();
|
||||
assert.equal(body, String(process.pid), 'recovered lock must carry the real pid (no content-less lock survives)');
|
||||
releaseStateLock(acquired);
|
||||
});
|
||||
|
||||
test('a FATAL (non-recoverable) write error still propagates after cleanup — orphan not masked', () => {
|
||||
const clock = makeFakeClock(0);
|
||||
let iterations = 0;
|
||||
|
||||
stateMod._setStateLockTestHooks({
|
||||
simulateWriteError: 'ENOSPC', // fatal: NOT in ACQUIRE_LOCK_RETRY_ERRNOS
|
||||
onLoopIteration() {
|
||||
// A fatal error must propagate on the FIRST attempt — never retried.
|
||||
iterations++;
|
||||
},
|
||||
});
|
||||
|
||||
assert.throws(
|
||||
() => acquireStateLock(statePath, clock),
|
||||
(err) => err && err.code === 'ENOSPC',
|
||||
'a fatal write errno must propagate (not be masked by cleanup or retried)'
|
||||
);
|
||||
|
||||
assert.equal(iterations, 1, 'a fatal write errno must NOT be retried (single attempt then propagate)');
|
||||
|
||||
// After the throw, the empty lock created by the failed openSync must NOT be
|
||||
// left behind — cleanup runs even on the fatal path before re-throw.
|
||||
assert.ok(
|
||||
!fs.existsSync(lockPath),
|
||||
'fatal write error must still unlink the orphan lock before propagating (no stranded lock)'
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user