diff --git a/cli/__tests__/atomic-write-lock.test.mts b/cli/__tests__/atomic-write-lock.test.mts new file mode 100644 index 00000000..4de58e05 --- /dev/null +++ b/cli/__tests__/atomic-write-lock.test.mts @@ -0,0 +1,117 @@ +/** + * withLock ownership safety. The mutex must never hand two read-modify-write callers the manifest + * at once, which means a stale lock may only be reaped when its holder is PROVABLY gone — age alone + * would evict a live-but-paused writer — and a release may only remove the caller's OWN acquisition. + * Every case below drives the real filesystem: the lock dir, its holder stamp, and its mtime. + */ +import { spawnSync } from 'node:child_process'; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + utimesSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { withLock } from '../lib/atomic-write.mts'; + +const roots: string[] = []; +const mkTmp = () => { + const d = mkdtempSync(join(tmpdir(), 'devkit-lock-')); + roots.push(d); + return d; +}; + +afterEach(() => { + for (const d of roots.splice(0)) rmSync(d, { recursive: true, force: true }); +}); + +/** Plant a held lock: dir + `:` stamp, aged `ageMs` into the past (mtime set LAST). */ +const plantLock = ( + lockDir: string, + { pid, ageMs, stamped = true }: { pid: number; ageMs: number; stamped?: boolean }, +) => { + mkdirSync(lockDir); + if (stamped) writeFileSync(join(lockDir, 'holder'), `${pid}:planted-uuid`, 'utf8'); + const when = new Date(Date.now() - ageMs); + utimesSync(lockDir, when, when); +}; + +/** A pid that is definitely not running: spawn a trivial process and reuse its pid after it exits. */ +const deadPid = () => { + const p = spawnSync(process.execPath, ['-e', ''], { stdio: 'ignore' }); + if (typeof p.pid !== 'number') throw new Error('could not obtain a pid'); + return p.pid; +}; + +const STALE_MS = 90_000; // > the 60s LOCK_STALE_MS +const FRESH_MS = 1_000; + +describe('withLock', () => { + it('runs the callback under the lock and releases it afterwards', () => { + const lockDir = join(mkTmp(), 'manifest.json.lock'); + const seen = withLock(lockDir, () => { + expect(existsSync(lockDir)).toBe(true); + return readFileSync(join(lockDir, 'holder'), 'utf8'); + }); + expect(seen.startsWith(`${process.pid}:`)).toBe(true); + expect(existsSync(lockDir)).toBe(false); + }); + + it('reaps a stale lock whose holder is gone', () => { + const lockDir = join(mkTmp(), 'manifest.json.lock'); + plantLock(lockDir, { pid: deadPid(), ageMs: STALE_MS }); + expect(withLock(lockDir, () => 'acquired')).toBe('acquired'); + expect(existsSync(lockDir)).toBe(false); + }); + + it('does NOT reap a stale lock whose holder is still alive', () => { + // The reviewer's case: a live writer paused past the stale window still owns its lock. Our own + // pid stands in for it — reaping here would run a second read-modify-write concurrently. + const lockDir = join(mkTmp(), 'manifest.json.lock'); + plantLock(lockDir, { pid: process.pid, ageMs: STALE_MS }); + expect(() => withLock(lockDir, () => 'acquired')).toThrow(/timed out acquiring manifest lock/); + expect(existsSync(lockDir)).toBe(true); + expect(readFileSync(join(lockDir, 'holder'), 'utf8')).toBe(`${process.pid}:planted-uuid`); + }); + + it('does NOT reap a fresh lock even when its holder is gone', () => { + // A young lock is presumed live: the holder may be mid-acquire, and the caller can afford to wait. + const lockDir = join(mkTmp(), 'manifest.json.lock'); + plantLock(lockDir, { pid: deadPid(), ageMs: FRESH_MS }); + expect(() => withLock(lockDir, () => 'acquired')).toThrow(/timed out acquiring manifest lock/); + expect(existsSync(lockDir)).toBe(true); + }); + + it('reaps a stale UNSTAMPED lock (acquirer died between its mkdir and its stamp write)', () => { + const lockDir = join(mkTmp(), 'manifest.json.lock'); + plantLock(lockDir, { pid: 0, ageMs: STALE_MS, stamped: false }); + expect(withLock(lockDir, () => 'acquired')).toBe('acquired'); + expect(existsSync(lockDir)).toBe(false); + }); + + it('does not release a lock that is no longer ours', () => { + // Simulates being wrongly reaped mid-section: another holder now owns lockDir. An unconditional + // rmSync in the finally would strip THEIR lock and admit a third writer. + const lockDir = join(mkTmp(), 'manifest.json.lock'); + withLock(lockDir, () => { + writeFileSync(join(lockDir, 'holder'), '999999:someone-elses-uuid', 'utf8'); + }); + expect(existsSync(lockDir)).toBe(true); + expect(readFileSync(join(lockDir, 'holder'), 'utf8')).toBe('999999:someone-elses-uuid'); + }); + + it('releases the lock when the callback throws', () => { + const lockDir = join(mkTmp(), 'manifest.json.lock'); + expect(() => + withLock(lockDir, () => { + throw new Error('boom'); + }), + ).toThrow('boom'); + expect(existsSync(lockDir)).toBe(false); + }); +}); diff --git a/cli/lib/atomic-write.mts b/cli/lib/atomic-write.mts index 3b0255ab..188267e4 100644 --- a/cli/lib/atomic-write.mts +++ b/cli/lib/atomic-write.mts @@ -5,17 +5,124 @@ * UNIQUE (pid + timestamp) so two callers writing the same target never collide on the temp name. * * Shared by the ship manifest writer (cli/lib/ship/reconcile-manifest-write.mjs) and reconcile's - * pruneBranch (cli/lib/reconcile.mjs) — both mutate .devkit/reconcile-manifest.json. The lost-update - * race (two read-modify-write callers) is guarded SEPARATELY by each caller's mkdir-mutex; this - * function only guarantees a single write is never torn. + * pruneBranch (cli/lib/reconcile.mjs) — both mutate .devkit/reconcile-manifest.json. writeFileAtomic + * only guarantees a single write is never torn; the lost-update race (two read-modify-write callers) + * is guarded by `withLock` below, which both callers wrap their read→modify→write in. * * Distinct from the two gate-engine//atomic-write.mjs copies on purpose: a gate-engine ships * its own copy to stay independently vendorable (no cross-engine import), whereas cli/ has one home. */ -import { renameSync, writeFileSync } from 'node:fs'; +import { randomUUID } from 'node:crypto'; +import { lstatSync, mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; export function writeFileAtomic(path: string, contents: string): void { const tmp = `${path}.${process.pid}.${Date.now()}.tmp`; writeFileSync(tmp, contents, 'utf8'); renameSync(tmp, path); } + +const LOCK_STALE_MS = 60_000; // age below which a lock is presumed live regardless of its holder +const LOCK_WAIT_MS = 5_000; // total time to retry a contended lock before throwing (never write unlocked) +const LOCK_RETRY_MS = 25; // pause between mkdir attempts, so a contended wait does not spin the CPU + +/** The acquisition stamp lives INSIDE the lock dir, so it is created and removed with the lock. */ +const holderFile = (lockDir: string) => join(lockDir, 'holder'); + +/** Block the calling thread — the critical section is synchronous, so there is no loop to yield to. */ +const sleepSync = (ms: number) => { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +}; + +/** + * Is `pid` still running? EPERM means the process exists but belongs to another uid — still alive. + * Only ESRCH (no such process) proves death, so every ambiguous answer errs toward "keep waiting". + */ +function pidAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (e: unknown) { + return e instanceof Error && 'code' in e && e.code === 'EPERM'; + } +} + +/** The `:` stamp of the CURRENT acquisition, or null if unreadable (mid-acquire/reaped). */ +function readHolder(lockDir: string): { pid: number; stamp: string } | null { + try { + const stamp = readFileSync(holderFile(lockDir), 'utf8'); + const pid = Number(stamp.split(':')[0]); + return Number.isInteger(pid) && pid > 0 ? { pid, stamp } : null; + } catch { + return null; + } +} + +/** + * Remove `lockDir` only when it is PROVABLY a dead holder's. Age alone is not proof: a live holder + * that was paused (SIGSTOP, a suspended laptop, a long GC) still owns its lock at 60s, and reaping + * it would let a second read-modify-write run concurrently with the first. + * + * So a reap needs three agreeing facts — stale age, a pid that no longer exists, and a stamp that + * did not change while we checked. The re-read is what defeats release-and-reacquire: if the holder + * released and someone else acquired between our two reads, the stamp differs and we leave their + * (live) lock alone. Residual: a reacquire landing between the re-read and the rmSync below is + * still possible, but it requires the dead holder's lock to be reaped by a third party inside that + * window; every wider path is closed. Refusing to reap is always safe — the caller just times out. + */ +function reapIfDead(lockDir: string): void { + let mtimeMs: number; + try { + mtimeMs = lstatSync(lockDir).mtimeMs; + } catch { + return; // lock vanished under us — the loop retries the mkdir + } + if (Date.now() - mtimeMs <= LOCK_STALE_MS) return; + const before = readHolder(lockDir); + // An unstamped stale lock is an acquirer that died between its mkdir and its stamp write (a + // microsecond window); there is no pid to interrogate, so staleness is all the evidence there is. + if (before && pidAlive(before.pid)) return; + if (readHolder(lockDir)?.stamp !== before?.stamp) return; + rmSync(lockDir, { recursive: true, force: true }); +} + +/** + * Atomic-mkdir mutex (flock is absent on macOS — verified). The dir IS the lock; mkdir is + * atomic create-or-fail on every POSIX fs, and the holder stamps its pid inside so ownership can be + * checked rather than inferred. We only guard a sub-ms read→write→rename, so on contention we retry + * up to LOCK_WAIT_MS; if still unheld we THROW rather than write unlocked (an unlocked + * read-modify-write would lose a parallel ship's branch entry). Shared by the two reconcile-manifest + * mutators named above; both lock a path under the repo's own .devkit/, so the pid check always + * refers to a process on this machine. + */ +export function withLock(lockDir: string, fn: () => T): T { + const stamp = `${process.pid}:${randomUUID()}`; + const deadline = Date.now() + LOCK_WAIT_MS; + let held = false; + while (Date.now() <= deadline) { + try { + mkdirSync(lockDir); + } catch (e: unknown) { + if (!(e instanceof Error && 'code' in e && e.code === 'EEXIST')) throw e; + reapIfDead(lockDir); + sleepSync(LOCK_RETRY_MS); + continue; + } + try { + writeFileSync(holderFile(lockDir), stamp, 'utf8'); + } catch (e: unknown) { + rmSync(lockDir, { recursive: true, force: true }); // never leave a lock we cannot prove is ours + throw e; + } + held = true; + break; + } + if (!held) throw new Error(`timed out acquiring manifest lock: ${lockDir}`); + try { + return fn(); + } finally { + // Release OUR acquisition only. If this lock was wrongly reaped and another process now holds + // it, an unconditional rmSync here would strip a live holder's lock and admit a third writer. + if (readHolder(lockDir)?.stamp === stamp) rmSync(lockDir, { recursive: true, force: true }); + } +} diff --git a/cli/lib/reconcile.mts b/cli/lib/reconcile.mts index a80ddc89..045173ff 100644 --- a/cli/lib/reconcile.mts +++ b/cli/lib/reconcile.mts @@ -29,13 +29,11 @@ * exact concurrent work this module declined to touch one step earlier. */ import { execFileSync } from 'node:child_process'; -import { existsSync, lstatSync, mkdirSync, readFileSync, rmSync } from 'node:fs'; +import { existsSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; -import { writeFileAtomic } from './atomic-write.mts'; +import { withLock, writeFileAtomic } from './atomic-write.mts'; const ABSENT = Symbol('absent'); // a file/blob that does not exist on a given side (≠ any sha) -const LOCK_STALE_MS = 60_000; -const LOCK_WAIT_MS = 5_000; /** A blob sha (git object id) or the ABSENT sentinel for a side where the path does not exist. */ type Blob = string | typeof ABSENT; @@ -165,35 +163,6 @@ export function loadManifest(mainRepo: string): ReconcileManifest { return { version: 1, branches: {} }; } -/** Atomic-mkdir mutex (flock is absent on macOS) — see the manifest writer for the rationale. */ -function withLock(lockDir: string, fn: () => T): T { - const deadline = Date.now() + LOCK_WAIT_MS; - let held = false; - while (Date.now() <= deadline) { - try { - mkdirSync(lockDir); - held = true; - break; - } catch (e: unknown) { - if (!(e instanceof Error && 'code' in e && e.code === 'EEXIST')) throw e; - try { - if (Date.now() - lstatSync(lockDir).mtimeMs > LOCK_STALE_MS) - rmSync(lockDir, { recursive: true, force: true }); - } catch { - /* lock vanished — retry */ - } - } - } - // Never run fn() unlocked: an unsynchronized read-modify-write would let a concurrent ship or - // reconcile clobber another branch's entry (the rename is atomic, but the read+merge is not). - if (!held) throw new Error(`timed out acquiring manifest lock: ${lockDir}`); - try { - return fn(); - } finally { - rmSync(lockDir, { recursive: true, force: true }); - } -} - /** Remove a fully-reconciled branch entry (atomic temp+rename under the lock). */ export function pruneBranch(mainRepo: string, branch: string): void { const file = manifestFile(mainRepo); diff --git a/cli/lib/ship/reconcile-manifest-write.mts b/cli/lib/ship/reconcile-manifest-write.mts index c765a7e4..0dccb4d4 100644 --- a/cli/lib/ship/reconcile-manifest-write.mts +++ b/cli/lib/ship/reconcile-manifest-write.mts @@ -36,14 +36,12 @@ * miss only costs a manual reconcile later; it must never unwind a shipped PR. */ import { execFileSync } from 'node:child_process'; -import { existsSync, lstatSync, mkdirSync, readFileSync, realpathSync, rmSync } from 'node:fs'; +import { existsSync, lstatSync, mkdirSync, readFileSync, realpathSync } from 'node:fs'; import { dirname, join } from 'node:path'; import { pathToFileURL } from 'node:url'; -import { writeFileAtomic } from '../atomic-write.mts'; +import { withLock, writeFileAtomic } from '../atomic-write.mts'; import type { ReconcileManifest, ReconcilePath } from '../reconcile.mts'; -const LOCK_STALE_MS = 60_000; // a lock dir older than this is a dead writer/reader — reap it -const LOCK_WAIT_MS = 5_000; // total time to retry a contended lock before throwing (never write unlocked) const WS_SPLIT = /\s+/; // split a `git ls-tree` line into its mode/type/sha/path columns const PR_DIGITS = /^\d+$/; // a non-empty --pr is an integer; anything else → null @@ -112,39 +110,6 @@ function classify(gitRoot: string, baseSha: string, p: string): ReconcilePath | return { path: p, blobSha, mode, op: 'delete' }; } -/** - * Atomic-mkdir mutex (flock is absent on macOS — verified). The dir IS the lock; mkdir is - * atomic create-or-fail on every POSIX fs. We only guard a sub-ms read→write→rename, so on - * contention we retry up to LOCK_WAIT_MS; if still unheld we THROW rather than write unlocked - * (an unlocked read-modify-write would lose a parallel ship's branch entry). A lock older than - * 60s is a crashed holder — reap it. - */ -function withLock(lockDir: string, fn: () => T): T { - const deadline = Date.now() + LOCK_WAIT_MS; - let held = false; - while (Date.now() <= deadline) { - try { - mkdirSync(lockDir); - held = true; - break; - } catch (e: unknown) { - if (!(e instanceof Error && 'code' in e && e.code === 'EEXIST')) throw e; - try { - if (Date.now() - lstatSync(lockDir).mtimeMs > LOCK_STALE_MS) - rmSync(lockDir, { recursive: true, force: true }); - } catch { - /* lock vanished under us — loop retries the mkdir */ - } - } - } - if (!held) throw new Error(`timed out acquiring manifest lock: ${lockDir}`); - try { - return fn(); - } finally { - rmSync(lockDir, { recursive: true, force: true }); - } -} - /** A well-formed v1 manifest: version 1 with a plain (non-array) branches object. */ const isValidV1 = (m: unknown): m is ReconcileManifest => typeof m === 'object' && diff --git a/gate-engine/critique/evidence-bindings.mts b/gate-engine/critique/evidence-bindings.mts index 1d3e699e..82f28bd1 100644 --- a/gate-engine/critique/evidence-bindings.mts +++ b/gate-engine/critique/evidence-bindings.mts @@ -2,6 +2,7 @@ import { canonicalPlanCritiqueRecordJson, type PlanCritiqueRecordV1, sha256Bytes, + validText, } from './evidence-record.mts'; import { readPlanCritiqueRecord } from './evidence-store.mts'; import { @@ -34,7 +35,6 @@ const CRITIQUE_ID = /^pc1_[0-9a-f]{64}$/; const GIT_OBJECT_ID = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/; const ISO_TIMESTAMP = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/; const BINDING_FILE = /^[0-9a-f]{64}\.json$/; -const MAX_TEXT_BYTES = 4 * 1024; const MAX_BINDING_BYTES = 16 * 1024; const BINDING_PATH = ['devkit', 'plan-critique-bindings', 'v1'] as const; @@ -98,18 +98,6 @@ function exactObject(value: unknown, fields: readonly string[]): Record fields.includes(key)) ? value : null; } -function validText(value: unknown): value is string { - return ( - typeof value === 'string' && - value.trim().length > 0 && - Buffer.byteLength(value, 'utf8') <= MAX_TEXT_BYTES && - ![...value].some((character) => { - const code = character.charCodeAt(0); - return code <= 0x1f || (code >= 0x7f && code <= 0x9f); - }) - ); -} - function canonicalBindingJson(binding: PlanCritiqueBindingV1): Buffer { return Buffer.from(canonicalPlanCritiqueRecordJson(binding)); } diff --git a/gate-engine/critique/evidence-record.mts b/gate-engine/critique/evidence-record.mts index cfaa763e..64e1e030 100644 --- a/gate-engine/critique/evidence-record.mts +++ b/gate-engine/critique/evidence-record.mts @@ -1,4 +1,5 @@ import { createHash } from 'node:crypto'; +import { types as utilTypes } from 'node:util'; export type Sha256 = string; export type PlanCritiqueId = `pc1_${string}`; @@ -227,3 +228,32 @@ export function canonicalPlanCritiqueRecordJson(value: unknown): string { ); })}\n`; } + +/** Text fields accepted into critique evidence records: bounded, non-blank, no control chars. */ +export const PLAN_CRITIQUE_MAX_TEXT_BYTES = 4 * 1024; + +export function validText(value: unknown): value is string { + return ( + typeof value === 'string' && + value.trim().length > 0 && + Buffer.byteLength(value, 'utf8') <= PLAN_CRITIQUE_MAX_TEXT_BYTES && + ![...value].some((character) => { + const code = character.charCodeAt(0); + return code <= 0x1f || (code >= 0x7f && code <= 0x9f); + }) + ); +} + +/** Hardened plain-record guard for untrusted values: rejects proxies, arrays, and anything with + * a non-null, non-Object prototype (getter traps and prototype tricks must not reach readers). */ +export function plainRecord(value: unknown): Record | null { + if (value === null || typeof value !== 'object') return null; + try { + if (utilTypes.isProxy(value) || Array.isArray(value)) return null; + const prototype = Object.getPrototypeOf(value); + if (prototype !== Object.prototype && prototype !== null) return null; + return value as Record; + } catch { + return null; + } +} diff --git a/gate-engine/critique/lifecycle/work-quarantine.mts b/gate-engine/critique/lifecycle/work-quarantine.mts index cf6b79ff..afe4c6b3 100644 --- a/gate-engine/critique/lifecycle/work-quarantine.mts +++ b/gate-engine/critique/lifecycle/work-quarantine.mts @@ -1,11 +1,12 @@ import { rmSync } from 'node:fs'; import { join } from 'node:path'; -import { types as utilTypes } from 'node:util'; import { canonicalPlanCritiqueRecordJson, PLAN_CRITIQUE_PROVIDERS, type PlanCritiqueProvider, + plainRecord, sha256Bytes, + validText, } from '../evidence-record.mts'; import { managedPath, publishImmutable, readPrivateFileBounded } from '../immutable-file.mts'; import { @@ -15,7 +16,6 @@ import { } from '../persistence-lock.mts'; const SHA256 = /^[0-9a-f]{64}$/; -const MAX_TEXT_BYTES = 4 * 1024; const QUARANTINE_PATH = ['work-quarantines'] as const; export interface PlanCritiqueWorkQuarantineV1 { @@ -38,39 +38,25 @@ export type PlanCritiqueWorkQuarantineIdentityV1 = Pick< >; function exactObject(value: unknown, fields: readonly string[]): Record | null { - if (value === null || typeof value !== 'object') return null; + const record = plainRecord(value); + if (record === null) return null; try { - if (utilTypes.isProxy(value) || Array.isArray(value)) return null; - const prototype = Object.getPrototypeOf(value); - if (prototype !== Object.prototype && prototype !== null) return null; - const keys = Reflect.ownKeys(value); + const keys = Reflect.ownKeys(record); if ( keys.length !== fields.length || keys.some((key) => typeof key !== 'string' || !fields.includes(key)) ) return null; for (const key of keys) { - const descriptor = Object.getOwnPropertyDescriptor(value, key); + const descriptor = Object.getOwnPropertyDescriptor(record, key); if (!descriptor?.enumerable || !Object.hasOwn(descriptor, 'value')) return null; } - return value as Record; + return record; } catch { return null; } } -function validText(value: unknown): value is string { - return ( - typeof value === 'string' && - value.trim().length > 0 && - Buffer.byteLength(value, 'utf8') <= MAX_TEXT_BYTES && - ![...value].some((character) => { - const code = character.charCodeAt(0); - return code <= 0x1f || (code >= 0x7f && code <= 0x9f); - }) - ); -} - function parseIdentity(value: unknown): PlanCritiqueWorkQuarantineIdentityV1 | null { const identity = exactObject(value, ['provider', 'repositoryFingerprint', 'workId']); if ( diff --git a/gate-engine/critique/provider-adapters/claude-subagent-stop.mts b/gate-engine/critique/provider-adapters/claude-subagent-stop.mts index 2cfa7ff5..eb83eb02 100644 --- a/gate-engine/critique/provider-adapters/claude-subagent-stop.mts +++ b/gate-engine/critique/provider-adapters/claude-subagent-stop.mts @@ -1,9 +1,12 @@ -import { types as utilTypes } from 'node:util'; import { PLAN_CRITIQUE_CALLBACK_IDENTITY_MAX_BYTES, type PlanCritiqueCompletedCallbackV1, } from '../capture-normalizer.mts'; -import { PLAN_CRITIQUE_EXACT_RESPONSE_MAX_BYTES, sha256Bytes } from '../evidence-record.mts'; +import { + PLAN_CRITIQUE_EXACT_RESPONSE_MAX_BYTES, + plainRecord, + sha256Bytes, +} from '../evidence-record.mts'; export const CLAUDE_PLAN_CRITIQUE_IDENTITY_MAX_BYTES = 1024; @@ -30,18 +33,6 @@ export interface ClaudePlanCritiqueAdapterContextV1 { repository: PlanCritiqueCompletedCallbackV1['repository']; } -function plainRecord(value: unknown): Record | null { - if (value === null || typeof value !== 'object') return null; - try { - if (utilTypes.isProxy(value) || Array.isArray(value)) return null; - const prototype = Object.getPrototypeOf(value); - if (prototype !== Object.prototype && prototype !== null) return null; - return value as Record; - } catch { - return null; - } -} - function ownDataValue(record: Record, key: string): unknown { try { const descriptor = Object.getOwnPropertyDescriptor(record, key);