From 9979bf289f0903dc6573b117149ad4bd103b222b Mon Sep 17 00:00:00 2001 From: Arzaan <25bee110@nith.ac.in> Date: Wed, 26 Aug 2026 13:48:15 +0530 Subject: [PATCH 1/4] Improve file lock handling with heartbeat mechanism Enhance file lock mechanism with heartbeat and mtime refresh to prevent premature lock expiration. --- src/site-memory/file-lock.ts | 60 +++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 7 deletions(-) diff --git a/src/site-memory/file-lock.ts b/src/site-memory/file-lock.ts index 5e1761f4..b085ac11 100644 --- a/src/site-memory/file-lock.ts +++ b/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(target: string, fn: () => Promise, options: FileLockOptions = {}): Promise { 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 { + 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 { 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 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); From 38a0b94b9c974166776783a25a1808008a5a03b7 Mon Sep 17 00:00:00 2001 From: Arzaan <25bee110@nith.ac.in> Date: Wed, 26 Aug 2026 13:49:01 +0530 Subject: [PATCH 2/4] Add tests for file lock behavior with stale locks --- src/site-memory/file-lock.test.ts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/site-memory/file-lock.test.ts b/src/site-memory/file-lock.test.ts index d323eab2..424c0e99 100644 --- a/src/site-memory/file-lock.test.ts +++ b/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); From 6b7cc1260046144db54b509e22c2c9277a2e6d9a Mon Sep 17 00:00:00 2001 From: Syed Arzaan <25bee110@nith.ac.in> Date: Sat, 29 Aug 2026 10:51:23 +0530 Subject: [PATCH 3/4] fix(site-memory): add heartbeat to file lock so live holders aren't broken past staleMs --- src/site-memory/file-lock.ts | 74 +++++++++++++++--------------------- 1 file changed, 31 insertions(+), 43 deletions(-) diff --git a/src/site-memory/file-lock.ts b/src/site-memory/file-lock.ts index 9182b783..338dadcf 100644 --- a/src/site-memory/file-lock.ts +++ b/src/site-memory/file-lock.ts @@ -9,14 +9,13 @@ * 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 - * 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 + * Abandoned locks never wedge site memory. A lock whose owner process is gone + * 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. */ @@ -27,10 +26,12 @@ 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; @@ -57,42 +58,38 @@ export async function withFileLock(target: string, fn: () => Promise, opti const lockPath = lockPathFor(target); const staleMs = options.staleMs ?? LOCK_STALE_MS; const token = await acquire(lockPath, options); - const heartbeat = startHeartbeat(lockPath, token, staleMs); + const heartbeat = startHeartbeat(lockPath, staleMs); try { return await fn(); } finally { - clearInterval(heartbeat); + heartbeat.stop(); 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. + * 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, token: string, staleMs: number): NodeJS.Timeout { - const intervalMs = Math.max(1_000, Math.floor(staleMs / 3)); +function startHeartbeat(lockPath: string, staleMs: number): { stop: () => void } { + const intervalMs = Math.max(1, Math.floor(staleMs / HEARTBEAT_DIVISOR)); + let stopped = false; const timer = setInterval(() => { - void touchIfOwned(lockPath, token); + if (stopped) return; + const now = new Date(); + utimes(lockPath, now, now).catch(() => undefined); }, 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 { - 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. - } + return { + stop() { + stopped = true; + clearInterval(timer); + }, + }; } async function acquire(lockPath: string, options: FileLockOptions): Promise { @@ -142,17 +139,8 @@ async function breakIfAbandoned(lockPath: string, staleMs: number): Promise staleMs; - const ownerGone = checkable && !isPidAlive(owner.pid); + const ownerGone = owner.host === hostname() && isActionablePid(owner.pid) && !isPidAlive(owner.pid); if (!expired && !ownerGone) return false; const after = await statOrUndefined(lockPath); From 6278217875b2512bbba825120d67425937d86d3b Mon Sep 17 00:00:00 2001 From: Syed Arzaan <25bee110@nith.ac.in> Date: Sat, 29 Aug 2026 10:52:20 +0530 Subject: [PATCH 4/4] test(site-memory): add regression test for live-holder lock breaking under staleMs --- src/site-memory/file-lock.test.ts | 55 +++++++++++++++---------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/src/site-memory/file-lock.test.ts b/src/site-memory/file-lock.test.ts index a9ad864b..657db491 100644 --- a/src/site-memory/file-lock.test.ts +++ b/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( @@ -84,34 +111,6 @@ 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);