feat(db-lock): automatic same-host dead-pid cycle-lock takeover (#1780 Gap 3)

tryAcquireDbLock now reclaims a held, not-TTL-expired lock when the same-host holder is provably dead (process.kill ESRCH) past a 60s grace, via guarded DELETE + one normal-upsert retry returning the normal handle. New shared injectable classifyHolderLiveness/isHolderDeadLocally (EPERM treated as ALIVE — never steals a live lock). runBreakLock's safe path consumes the shared predicate, fixing its prior EPERM-as-dead bug. Cross-host stays TTL-only.
2026-07-30 19:49:14 +00:00 · 2026-06-02 22:42:30 -07:00
parent b19c8496c5
commit 3c680b357f
3 changed files with 242 additions and 9 deletions
@@ -751,7 +751,7 @@ async function runBreakLock(
  sourceId: string,
  opts: { force: boolean; json: boolean; maxAgeSeconds?: number },
 ): Promise<number> {
-  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';
    }
  }
@@ -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<DbLockHandle | null> => {
  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;
 }

 /**
@@ -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<number> {
+  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();
+  });
+});