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

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

@@ -67,6 +67,34 @@ describe('site memory file lock', () => {
     await expect(readFile(lockPathFor(target), 'utf8')).resolves.toContain('held');
   });
 
+  it('does not break a live same-host lock just because it is older than staleMs', async () => {
+    const target = await tempTarget();
+    const lockPath = lockPathFor(target);
+    await writeFile(lockPath, `${JSON.stringify({ pid: process.pid, host: hostname(), token: 'still-working' })}\n`);
+    const past = new Date(Date.now() - 60_000);
+    await utimes(lockPath, past, past);
+
+    // Same bug as the header comment describes: a legitimate critical section that
+    // outlives staleMs must never be conceded to a second writer while it's still running.
+    await expect(withFileLock(target, async () => 'written', { staleMs: 1_000, timeoutMs: 50 }))
+      .rejects.toMatchObject({ code: 'SITE_MEMORY_BUSY' });
+    await expect(readFile(lockPath, 'utf8')).resolves.toContain('still-working');
+  });
+
+  it('heartbeats the mtime of a lock held across a critical section longer than staleMs', async () => {
+    const target = await tempTarget();
+    const lockPath = lockPathFor(target);
+
+    await withFileLock(target, async () => {
+      await new Promise((resolve) => { setTimeout(resolve, 120); });
+      // A concurrent second acquirer must not be able to break this lock mid-flight.
+      await expect(withFileLock(target, async () => 'written', { staleMs: 30, timeoutMs: 20 }))
+        .rejects.toMatchObject({ code: 'SITE_MEMORY_BUSY' });
+    }, { staleMs: 30, timeoutMs: 1_000 });
+
+    await expect(exists(lockPath)).resolves.toBe(false);
+  });
+
   it('does not delete a lock that was broken and taken over by someone else', async () => {
     const target = await tempTarget();
     const lockPath = lockPathFor(target);

src/site-memory/file-lock.ts

@@ -9,14 +9,19 @@
  * marker the filesystem can see: `open(..., 'wx')` creates the lock file only
  * 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.
+ * Abandoned locks never wedge site memory. A lock whose owner process is
+ * confirmed gone is broken on the next attempt. `staleMs` only breaks a lock
+ * when we cannot confirm the owner is still running (different host, or an
+ * owner field we can't check) — a same-host owner we can see is alive is never
+ * broken on staleness alone, so a critical section that runs long (a big file,
+ * a slow disk, a loaded machine) is never mistaken for a crash. The holder also
+ * refreshes the lock's mtime with a heartbeat while it works, as a second line
+ * of defense for the cross-host/unconfirmed-owner case. `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';
@@ -50,14 +55,46 @@ 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, token, staleMs);
   try {
     return await fn();
   } finally {
+    clearInterval(heartbeat);
     await release(lockPath, token);
   }
 }
 
+/**
+ * Refresh the lock file's mtime while the critical section is still running, so a
+ * critical section that legitimately runs longer than `staleMs` (a big file, a slow
+ * disk, a loaded machine) does not look abandoned to another process. Ticks at a
+ * fraction of `staleMs` so at least one refresh lands before the file would otherwise
+ * go stale. `.unref()`d so a stuck interval never keeps the process alive.
+ */
+function startHeartbeat(lockPath: string, token: string, staleMs: number): NodeJS.Timeout {
+  const intervalMs = Math.max(1_000, Math.floor(staleMs / 3));
+  const timer = setInterval(() => {
+    void touchIfOwned(lockPath, token);
+  }, intervalMs);
+  timer.unref?.();
+  return timer;
+}
+
+/** Only refresh the mtime if we still hold this lock — never extend a lock reassigned to someone else. */
+async function touchIfOwned(lockPath: string, token: string): Promise<void> {
+  try {
+    const owner = await readOwner(lockPath);
+    if (owner.token !== token) return;
+    const now = new Date();
+    await utimes(lockPath, now, now);
+  } catch {
+    // Best-effort: if the touch fails, breakIfAbandoned's owner-liveness check is
+    // still there to stop a live holder's lock from being broken out from under it.
+  }
+}
+
 async function acquire(lockPath: string, options: FileLockOptions): Promise<string> {
   const staleMs = options.staleMs ?? LOCK_STALE_MS;
   const timeoutMs = options.timeoutMs ?? LOCK_TIMEOUT_MS;
@@ -101,8 +138,17 @@ async function breakIfAbandoned(lockPath: string, staleMs: number): Promise<bool
   const before = await statOrUndefined(lockPath);
   if (!before) return true;
   const owner = await readOwner(lockPath);
+
+  // A same-host owner we can still see running is decisive: never break its lock on
+  // staleness alone, no matter how old the mtime looks. This is what actually stops a
+  // slow-but-alive critical section from being mistaken for a crash — the heartbeat in
+  // withFileLock keeps mtime fresh too, but this check is what closes the bug even if a
+  // heartbeat tick is ever missed (GC pause, event-loop stall, etc).
+  const checkable = owner.host === hostname() && isActionablePid(owner.pid);
+  if (checkable && isPidAlive(owner.pid)) return false;
+
   const expired = Date.now() - before.mtimeMs > staleMs;
-  const ownerGone = owner.host === hostname() && isActionablePid(owner.pid) && !isPidAlive(owner.pid);
+  const ownerGone = checkable && !isPidAlive(owner.pid);
   if (!expired && !ownerGone) return false;
 
   const after = await statOrUndefined(lockPath);