golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(#1531): race-safe steal for the two core-path locks (PR #1532 review)

Browse Source 
trek-e's review found the M1 PID-liveness backport dropped two pieces of
capability-lock.cts's steal-safety machinery, reopening the #500/#905/#1230
lost-update family:

- Empty-body window (state.cts): acquireStateLock creates the lock with O_EXCL
  and writes the pid in a separate writeSync; a lock observed in that gap has an
  empty body, reads as not-verified-live, and was stolen at age ~0 — robbing a
  holder mid-creation. Add a fresh-create floor scoped to the unverifiable-body
  case: an empty/unparseable body that is fresh is treated as mid-creation and is
  NOT stolen, while a COMPLETE dead-pid body is still stolen promptly (preserves
  the prompt-dead-steal contract). planning-workspace writes its body atomically
  (flag:'wx') so it has no empty-body window.

- Double-steal (both locks): the steal was a bare fs.unlinkSync with no identity
  re-confirm, so two waiters could both reclaim a dead holder and end up holding
  concurrently. Replace with an atomic renameSync (only one racer wins the inode)
  guarded by a (dev,ino,body) identity re-confirm immediately before the steal;
  body content is part of the identity to defeat inode reuse.

Tests (seam-driven, no wall-clock, each proven RED-before-GREEN):
- clock-seam: fresh empty-body lock is not stolen at age ~0; a racer-recreated
  live lock is not double-stolen (identity re-confirm). Adds a beforeSteal seam.
- planning-workspace: racer-recreated live lock is not double-stolen.
- Updated the two #1217 unlinkSync-failure tests to the renameSync steal path
  (the bounded-backoff/no-busy-spin guarantee is preserved and re-asserted).

Uncontended acquire path is byte-for-byte unchanged.

Claude-Session: https://claude.ai/code/session_01R88n7Q54bAaVHFkDbbH1yz
This commit is contained in:
Dave
2026-06-21 23:22:17 -04:00
parent 4c677f2d92
commit fc019b2688
5 changed files with 367 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

67
src/planning-workspace.cts
View File

@@ -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<T>(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;
},
};

129
src/state.cts
View File

@@ -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<string, unknown>)?.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;
},
};