fix: don't break a live lock on staleness alone (#432)

src/site-memory/file-lock.test.ts

@@ -34,6 +34,33 @@ describe('site memory file lock', () => {
     await expect(exists(lockPathFor(target))).resolves.toBe(false);
   });
 
+  it('does not break a live holder\'s lock even when its critical section outlives staleMs', async () => {
+    const target = await tempTarget();
+    const staleMs = 40;
+    let inside = 0;
+    let overlapped = false;
+
+    async function worker(holdMs: number) {
+      await withFileLock(target, async () => {
+        inside += 1;
+        if (inside > 1) overlapped = true;
+        await new Promise((resolve) => { setTimeout(resolve, holdMs); });
+        inside -= 1;
+      }, { staleMs, timeoutMs: 5_000 });
+    }
+
+    // Worker A's critical section (150ms) runs well past staleMs (40ms).
+    // Without a heartbeat refreshing the lock's mtime, worker B would see
+    // the lock as abandoned and break it out from under A.
+    const a = worker(150);
+    await new Promise((resolve) => { setTimeout(resolve, 80); });
+    const b = worker(20);
+    await Promise.all([a, b]);
+
+    expect(overlapped).toBe(false);
+    await expect(exists(lockPathFor(target))).resolves.toBe(false);
+  });
+
   it('retries a transient Windows EPERM when creating the lock', async () => {
     const target = await tempTarget();
     vi.mocked(open).mockRejectedValueOnce(

src/site-memory/file-lock.ts

@@ -10,22 +10,28 @@
  * when it does not already exist, atomically, on every platform we support.
  *
  * Abandoned locks never wedge site memory. A lock whose owner process is gone
- * is broken on the next attempt, and any lock older than `staleMs` is broken
- * regardless — the critical section itself is a small read plus a rename, which
- * takes milliseconds. `timeoutMs` is deliberately longer than `staleMs` so an
- * abandoned lock is always broken rather than surfaced to the user as an error.
+ * is broken on the next attempt. A lock is otherwise only broken once it has
+ * gone `staleMs` without a heartbeat: the holder in `withFileLock` refreshes
+ * the lock file's mtime on an interval well inside `staleMs` for as long as
+ * `fn()` runs, so a live holder's lock never looks abandoned regardless of how
+ * long its critical section actually takes — only a holder that has stopped
+ * heartbeating (crashed, or the process died) goes stale. `timeoutMs` is
+ * deliberately longer than `staleMs` so an abandoned lock is always broken
+ * rather than surfaced to the user as an error.
  */
 import { randomUUID } from 'node:crypto';
-import { open, readFile, stat, unlink } from 'node:fs/promises';
+import { open, readFile, stat, unlink, utimes } from 'node:fs/promises';
 import { hostname } from 'node:os';
 import { basename } from 'node:path';
 import { CliError, EXIT_CODES } from '../errors.js';
 import { isActionablePid, isPidAlive } from '../session-lease.js';
 
-/** A lock held longer than this is treated as abandoned by a crashed process. */
+/** A lock held longer than this without a heartbeat is treated as abandoned. */
 export const LOCK_STALE_MS = 10_000;
 /** Total acquire budget. Longer than LOCK_STALE_MS so stale locks are broken, not reported. */
 export const LOCK_TIMEOUT_MS = 15_000;
+/** Heartbeats land at a fraction of staleMs, so one missed tick can't cause a false break. */
+const HEARTBEAT_DIVISOR = 3;
 
 const RETRY_MIN_MS = 5;
 const RETRY_MAX_MS = 50;
@@ -50,14 +56,42 @@ export function lockPathFor(target: string): string {
 /** Run `fn` while holding the cross-process lock for `target`. */
 export async function withFileLock<T>(target: string, fn: () => Promise<T>, options: FileLockOptions = {}): Promise<T> {
   const lockPath = lockPathFor(target);
+  const staleMs = options.staleMs ?? LOCK_STALE_MS;
   const token = await acquire(lockPath, options);
+  const heartbeat = startHeartbeat(lockPath, staleMs);
   try {
     return await fn();
   } finally {
+    heartbeat.stop();
     await release(lockPath, token);
   }
 }
 
+/**
+ * Keeps `lockPath`'s mtime fresh for as long as the caller holds the lock, so
+ * `breakIfAbandoned` never mistakes a live, still-working holder for a crashed
+ * one just because its critical section is slow. Best-effort: a missed touch
+ * (e.g. the file briefly unreadable under load) is not fatal — it only risks
+ * the lock being broken early, the same failure mode this replaces, not a new
+ * one — and a touch is never attempted once `stop()` has been called.
+ */
+function startHeartbeat(lockPath: string, staleMs: number): { stop: () => void } {
+  const intervalMs = Math.max(1, Math.floor(staleMs / HEARTBEAT_DIVISOR));
+  let stopped = false;
+  const timer = setInterval(() => {
+    if (stopped) return;
+    const now = new Date();
+    utimes(lockPath, now, now).catch(() => undefined);
+  }, intervalMs);
+  timer.unref?.();
+  return {
+    stop() {
+      stopped = true;
+      clearInterval(timer);
+    },
+  };
+}
+
 async function acquire(lockPath: string, options: FileLockOptions): Promise<string> {
   const staleMs = options.staleMs ?? LOCK_STALE_MS;
   const timeoutMs = options.timeoutMs ?? LOCK_TIMEOUT_MS;