diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 1c820c01f..7527d0189 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -751,7 +751,7 @@ async function runBreakLock( sourceId: string, opts: { force: boolean; json: boolean; maxAgeSeconds?: number }, ): Promise { - const { inspectLock, deleteLockRow, deleteLockRowIfStale } = await import('../core/db-lock.ts'); + const { inspectLock, deleteLockRow, deleteLockRowIfStale, classifyHolderLiveness } = await import('../core/db-lock.ts'); const { hostname } = await import('os'); const localHost = hostname(); let snap; @@ -863,18 +863,19 @@ async function runBreakLock( safe = true; reason = 'ttl_expired'; } else { - // PID liveness check on local host. process.kill(pid, 0) throws ESRCH - // when the PID is dead. Combined with 60s age guard (per outside-voice F7). - let alive = true; - try { process.kill(snap.holder_pid, 0); } - catch { alive = false; } - const oldEnough = snap.age_ms >= 60_000; - if (!alive && oldEnough) { + // PID liveness on local host, via the shared predicate (v0.42 #1780 Gap 3). + // Same gate as tryAcquireDbLock's auto-takeover: same-host + provably-dead + // (ESRCH) + age >= 60s. EPERM is treated as ALIVE (the PID exists but isn't + // ours) — never break a live lock. host is already == localHost here (the + // cross-host branch returned above), so classify never yields 'cross_host'. + const liveness = classifyHolderLiveness(snap.holder_pid, snap.holder_host, snap.age_ms); + if (liveness === 'dead_eligible') { safe = true; reason = 'pid_dead_age_60s'; - } else if (!alive && !oldEnough) { + } else if (liveness === 'too_young') { reason = 'pid_dead_but_lock_too_young'; } else { + // 'alive' | 'unknown' | 'cross_host' (the latter unreachable here). reason = 'pid_alive'; } } diff --git a/src/core/db-lock.ts b/src/core/db-lock.ts index 2be21abda..f69fa0f39 100644 --- a/src/core/db-lock.ts +++ b/src/core/db-lock.ts @@ -33,6 +33,81 @@ export interface DbLockHandle { /** Default TTL: 30 minutes, same as cycle lock. */ const DEFAULT_TTL_MINUTES = 30; +/** + * v0.42 (#1780 Gap 3): grace window before a same-host dead-pid lock is + * eligible for automatic takeover. Matches `runBreakLock`'s `age >= 60_000` + * gate so the two paths agree. Defends against PID reuse: the OS can recycle + * a crashed holder's PID, so we refuse takeover until the lock is older than + * this window. + */ +export const HOLDER_TAKEOVER_GRACE_MS = 60_000; + +/** + * Liveness classification of a lock holder, from the perspective of the + * current host. Shared by `isHolderDeadLocally` (auto-takeover in + * `tryAcquireDbLock`) and `gbrain sync --break-lock`'s safe path so the two + * never drift. + * + * - `cross_host` — holder is on a different host; `process.kill` is + * meaningless remotely, never take over. + * - `alive` — the PID exists (probe succeeded) OR the probe got + * EPERM (the PID exists but isn't ours). EPERM-as-ALIVE + * is load-bearing: stealing a live lock is the worst case. + * - `too_young` — PID is provably dead (ESRCH) but the lock is younger + * than the grace window (possible PID reuse). + * - `dead_eligible` — PID is provably dead AND the lock is old enough. + * - `unknown` — the probe threw something other than ESRCH/EPERM; + * conservative, treat as NOT eligible. + */ +export type HolderLiveness = 'cross_host' | 'alive' | 'too_young' | 'dead_eligible' | 'unknown'; + +export interface HolderLivenessOpts { + /** Grace window in ms (default HOLDER_TAKEOVER_GRACE_MS). */ + graceMs?: number; + /** Override the local hostname (test seam; default `os.hostname()`). */ + localHost?: string; + /** Override the liveness probe (test seam; default `process.kill`). */ + processKill?: (pid: number, signal: number) => void; +} + +export function classifyHolderLiveness( + holderPid: number, + holderHost: string, + ageMs: number, + opts: HolderLivenessOpts = {}, +): HolderLiveness { + const localHost = opts.localHost ?? hostname(); + if (holderHost !== localHost) return 'cross_host'; + + const probe = opts.processKill ?? ((p: number, s: number) => process.kill(p, s)); + let probeResult: 'alive' | 'dead' | 'eperm' | 'unknown'; + try { + probe(holderPid, 0); + probeResult = 'alive'; + } catch (e) { + const code = (e as NodeJS.ErrnoException)?.code; + probeResult = code === 'ESRCH' ? 'dead' : code === 'EPERM' ? 'eperm' : 'unknown'; + } + + // EPERM → the PID exists but isn't ours: treat as ALIVE, never steal. + if (probeResult === 'alive' || probeResult === 'eperm') return 'alive'; + if (probeResult === 'unknown') return 'unknown'; + + // Provably dead (ESRCH). Gate on the grace window to defend against PID reuse. + const grace = opts.graceMs ?? HOLDER_TAKEOVER_GRACE_MS; + return ageMs < grace ? 'too_young' : 'dead_eligible'; +} + +/** Convenience boolean: is the holder provably dead, same-host, and past the grace window? */ +export function isHolderDeadLocally( + holderPid: number, + holderHost: string, + ageMs: number, + opts: HolderLivenessOpts = {}, +): boolean { + return classifyHolderLiveness(holderPid, holderHost, ageMs, opts) === 'dead_eligible'; +} + /** * Try to acquire a named DB lock. * @@ -71,6 +146,7 @@ export async function tryAcquireDbLock( // registration for free (single ownership site per outside-voice F11). const { registerCleanup } = await import('./process-cleanup.ts'); + const acquireOnce = async (): Promise => { if (engine.kind === 'postgres' && maybePG.sql) { const sql = maybePG.sql as any; const ttl = `${ttlMinutes} minutes`; @@ -167,6 +243,32 @@ export async function tryAcquireDbLock( } throw new Error(`Unknown engine kind for db-lock: ${engine.kind}`); + }; + + const first = await acquireOnce(); + if (first) return first; + + // v0.42 (#1780 Gap 3): the lock is held and its TTL hasn't expired (the + // upsert's ON CONFLICT ... WHERE ttl_expires_at < NOW() returned no row). + // If the holder is on THIS host, provably dead, and past the grace window, + // reclaim it: guarded DELETE then retry the normal upsert ONCE. The retry + // returns the normal DbLockHandle (refresh/release intact) — no hand-rolled + // handle. TTL-expired holders are NOT handled here (the upsert already takes + // them); cross-host holders stay TTL-only. Best-effort: any error falls + // through to `return null` (busy), exactly as the pre-takeover behavior. + try { + const snap = await inspectLock(engine, lockId); + if (snap && !snap.ttl_expired && isHolderDeadLocally(snap.holder_pid, snap.holder_host, snap.age_ms)) { + const { deleted } = await deleteLockRow(engine, lockId, snap.holder_pid); + if (deleted) { + const second = await acquireOnce(); + if (second) return second; + } + } + } catch { + // Auto-takeover is best-effort; never throw from the acquire path. + } + return null; } /** diff --git a/test/db-lock-auto-takeover.test.ts b/test/db-lock-auto-takeover.test.ts new file mode 100644 index 000000000..1052b71c2 --- /dev/null +++ b/test/db-lock-auto-takeover.test.ts @@ -0,0 +1,130 @@ +/** + * #1780 Gap 3 — automatic same-host dead-pid lock takeover. + * + * Two layers: + * - classifyHolderLiveness (pure, injectable process.kill seam): the + * decision matrix incl. the CRITICAL EPERM-as-ALIVE rule. + * - tryAcquireDbLock auto-takeover (PGLite, real process.kill): a held + + * not-TTL-expired lock whose same-host holder is provably dead and past + * the 60s grace gets reclaimed; alive / cross-host / young holders don't. + * A taken-over lock returns a working handle (refresh + release). + */ + +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { hostname } from 'os'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { + tryAcquireDbLock, + classifyHolderLiveness, + isHolderDeadLocally, + HOLDER_TAKEOVER_GRACE_MS, +} from '../src/core/db-lock.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({ database_url: '' }); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await engine.executeRaw(`DELETE FROM gbrain_cycle_locks WHERE id LIKE 'test-takeover-%'`); +}); + +const LOCAL = hostname(); +const OLD_MS = HOLDER_TAKEOVER_GRACE_MS + 60_000; // comfortably past the grace window +const ESRCH = () => { const e = new Error('no such process') as NodeJS.ErrnoException; e.code = 'ESRCH'; throw e; }; +const EPERM = () => { const e = new Error('operation not permitted') as NodeJS.ErrnoException; e.code = 'EPERM'; throw e; }; +const EINVAL = () => { const e = new Error('weird') as NodeJS.ErrnoException; e.code = 'EINVAL'; throw e; }; +const aliveKill = () => { /* no throw → alive */ }; + +describe('classifyHolderLiveness', () => { + test('same-host + ESRCH + old → dead_eligible', () => { + expect(classifyHolderLiveness(123, LOCAL, OLD_MS, { processKill: ESRCH })).toBe('dead_eligible'); + }); + + test('same-host + ESRCH + young → too_young (PID-reuse guard)', () => { + expect(classifyHolderLiveness(123, LOCAL, 5_000, { processKill: ESRCH })).toBe('too_young'); + }); + + test('same-host + alive → alive', () => { + expect(classifyHolderLiveness(123, LOCAL, OLD_MS, { processKill: aliveKill })).toBe('alive'); + }); + + test('CRITICAL: same-host + EPERM → alive (never steal a live lock)', () => { + expect(classifyHolderLiveness(123, LOCAL, OLD_MS, { processKill: EPERM })).toBe('alive'); + }); + + test('same-host + unknown errno → unknown (conservative)', () => { + expect(classifyHolderLiveness(123, LOCAL, OLD_MS, { processKill: EINVAL })).toBe('unknown'); + }); + + test('cross-host → cross_host (never probe a remote pid)', () => { + expect(classifyHolderLiveness(123, 'some-other-host', OLD_MS, { processKill: ESRCH })).toBe('cross_host'); + }); + + test('isHolderDeadLocally is true only for dead_eligible', () => { + expect(isHolderDeadLocally(1, LOCAL, OLD_MS, { processKill: ESRCH })).toBe(true); + expect(isHolderDeadLocally(1, LOCAL, 5_000, { processKill: ESRCH })).toBe(false); + expect(isHolderDeadLocally(1, LOCAL, OLD_MS, { processKill: EPERM })).toBe(false); + expect(isHolderDeadLocally(1, 'other', OLD_MS, { processKill: ESRCH })).toBe(false); + }); +}); + +/** Insert a held, NOT-TTL-expired lock row for the given holder. */ +async function seedHeldLock(id: string, holderPid: number, holderHost: string, ageSeconds: number) { + await engine.executeRaw( + `INSERT INTO gbrain_cycle_locks (id, holder_pid, holder_host, acquired_at, ttl_expires_at, last_refreshed_at) + VALUES ($1, $2, $3, NOW() - ($4 || ' seconds')::interval, NOW() + INTERVAL '10 minutes', NOW() - ($4 || ' seconds')::interval)`, + [id, holderPid, holderHost, String(ageSeconds)], + ); +} + +/** A reliably-dead PID on this host: spawn a process, wait for it to exit. */ +async function deadPid(): Promise { + const proc = Bun.spawn(['sh', '-c', 'exit 0']); + await proc.exited; + return proc.pid; +} + +describe('tryAcquireDbLock auto-takeover', () => { + test('reclaims a same-host dead-pid lock past the grace window', async () => { + const pid = await deadPid(); + await seedHeldLock('test-takeover-dead', pid, LOCAL, 120); + const handle = await tryAcquireDbLock(engine, 'test-takeover-dead', 30); + expect(handle).not.toBeNull(); + // The reclaimed handle is the normal one: refresh + release work. + await handle!.refresh(); + await handle!.release(); + // After release, the row is gone → a fresh acquire succeeds immediately. + const again = await tryAcquireDbLock(engine, 'test-takeover-dead', 30); + expect(again).not.toBeNull(); + await again!.release(); + }); + + test('does NOT reclaim a live same-host holder', async () => { + // process.pid is alive → no takeover; lock stays held. + await seedHeldLock('test-takeover-alive', process.pid, LOCAL, 120); + const handle = await tryAcquireDbLock(engine, 'test-takeover-alive', 30); + expect(handle).toBeNull(); + }); + + test('does NOT reclaim a cross-host holder (TTL-only)', async () => { + const pid = await deadPid(); + await seedHeldLock('test-takeover-xhost', pid, 'a-different-host', 120); + const handle = await tryAcquireDbLock(engine, 'test-takeover-xhost', 30); + expect(handle).toBeNull(); + }); + + test('does NOT reclaim a dead-pid lock younger than the grace window', async () => { + const pid = await deadPid(); + await seedHeldLock('test-takeover-young', pid, LOCAL, 5); // 5s < 60s grace + const handle = await tryAcquireDbLock(engine, 'test-takeover-young', 30); + expect(handle).toBeNull(); + }); +});