From bf727173e7e113eafa5287353f9ca2ce2831efab Mon Sep 17 00:00:00 2001 From: Mikael Hugo Date: Tue, 28 Apr 2026 05:28:36 +0200 Subject: [PATCH] cherry-pick(file-lock): make file-lock actually lock and throw on contention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cherry-pick of gsd-build/gsd-2 a09e01640 — withFileLockSync now actually acquires a proper-lockfile (was previously a no-op when proper-lockfile wasn't required) and throws on ELOCKED contention by default. Adds onLocked: 'skip' option for best-effort callers that tolerate dropped entries (audit, journal). Modernizes import style (createRequire/join from imports rather than ad-hoc require). Path-renames preserved (gsd-pi → sf-run). Co-Authored-By: Jeremy Co-Authored-By: Claude Sonnet 4.6 --- src/resources/extensions/sf/file-lock.ts | 95 ++++++++++++++-- .../extensions/sf/tests/file-lock.test.ts | 102 +++++++++++++++--- 2 files changed, 172 insertions(+), 25 deletions(-) diff --git a/src/resources/extensions/sf/file-lock.ts b/src/resources/extensions/sf/file-lock.ts index 68b5ad172..5b270f48c 100644 --- a/src/resources/extensions/sf/file-lock.ts +++ b/src/resources/extensions/sf/file-lock.ts @@ -1,12 +1,21 @@ import { existsSync } from "node:fs"; +import { createRequire } from "node:module"; +import { join } from "node:path"; -function _require(name: string) { +// The file-lock module is loaded in both CJS builds and ESM sources. Under ESM +// the bare `require` identifier is not defined, so we always go through +// createRequire. We try the current module's resolution context first and fall +// back to the installed sf-run package if we are running from a consumer +// project that does not hoist proper-lockfile. +const localRequire = createRequire(import.meta.url); + +function _require(name: string): any { try { - return require(name); + return localRequire(name); } catch { try { - const sfPiRequire = require("module").createRequire( - require("path").join(process.cwd(), "node_modules", "sf-run", "index.js") + const sfPiRequire = createRequire( + join(process.cwd(), "node_modules", "sf-run", "index.js"), ); return sfPiRequire(name); } catch { @@ -15,43 +24,107 @@ function _require(name: string) { } } -export function withFileLockSync(filePath: string, fn: () => T): T { +export type OnLocked = "fail" | "skip"; + +export interface FileLockOptions { + /** + * Behavior when the lock cannot be acquired after retries (ELOCKED). + * - "fail" (default): rethrow the ELOCKED error so the caller can react. + * - "skip": run fn() unlocked. Only choose this for best-effort writes + * that genuinely tolerate contention (e.g. high-frequency audit appends + * where dropping one entry is acceptable). Silent unlocked execution was + * the legacy behavior and is a correctness hazard for shared state. + */ + onLocked?: OnLocked; + /** proper-lockfile retries (default 5). */ + retries?: number; + /** proper-lockfile stale threshold in ms (default 10000). */ + stale?: number; +} + +const DEFAULT_RETRIES = 5; +const DEFAULT_STALE_MS = 10000; +const SYNC_RETRY_DELAY_MS = 50; + +// Block the thread for `ms` milliseconds without spinning the CPU. +// Used by the sync lock retry loop, since proper-lockfile's lockSync does not +// accept a `retries` option (only the async `lock` does). +function sleepSync(ms: number): void { + if (ms <= 0) return; + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +function acquireLockSyncWithRetry( + lockfile: any, + filePath: string, + retries: number, + stale: number, +): () => void { + let lastErr: any; + for (let attempt = 0; attempt <= retries; attempt++) { + try { + return lockfile.lockSync(filePath, { stale }); + } catch (err: any) { + lastErr = err; + if (err?.code !== "ELOCKED") throw err; + if (attempt < retries) sleepSync(SYNC_RETRY_DELAY_MS); + } + } + throw lastErr; +} + +export function withFileLockSync( + filePath: string, + fn: () => T, + opts: FileLockOptions = {}, +): T { const lockfile = _require("proper-lockfile"); if (!lockfile) return fn(); if (!existsSync(filePath)) return fn(); + const retries = opts.retries ?? DEFAULT_RETRIES; + const stale = opts.stale ?? DEFAULT_STALE_MS; + const onLocked: OnLocked = opts.onLocked ?? "fail"; + try { - const release = lockfile.lockSync(filePath, { retries: 5, stale: 10000 }); + const release = acquireLockSyncWithRetry(lockfile, filePath, retries, stale); try { return fn(); } finally { release(); } } catch (err: any) { - if (err.code === "ELOCKED") { - // Could not get lock after retries, let's fallback to un-locked instead of crashing the whole state machine + if (err?.code === "ELOCKED" && onLocked === "skip") { return fn(); } throw err; } } -export async function withFileLock(filePath: string, fn: () => Promise | T): Promise { +export async function withFileLock( + filePath: string, + fn: () => Promise | T, + opts: FileLockOptions = {}, +): Promise { const lockfile = _require("proper-lockfile"); if (!lockfile) return await fn(); if (!existsSync(filePath)) return await fn(); + const retries = opts.retries ?? DEFAULT_RETRIES; + const stale = opts.stale ?? DEFAULT_STALE_MS; + const onLocked: OnLocked = opts.onLocked ?? "fail"; + try { - const release = await lockfile.lock(filePath, { retries: 5, stale: 10000 }); + const release = await lockfile.lock(filePath, { retries, stale }); try { return await fn(); } finally { await release(); } } catch (err: any) { - if (err.code === "ELOCKED") { + if (err?.code === "ELOCKED" && onLocked === "skip") { return await fn(); } throw err; diff --git a/src/resources/extensions/sf/tests/file-lock.test.ts b/src/resources/extensions/sf/tests/file-lock.test.ts index d7209fc44..687859da9 100644 --- a/src/resources/extensions/sf/tests/file-lock.test.ts +++ b/src/resources/extensions/sf/tests/file-lock.test.ts @@ -52,7 +52,7 @@ test("withFileLock: executes callback when file does not exist", async () => { } }); -test("withFileLockSync: falls back to unlocked callback on ELOCKED", () => { +test("withFileLockSync: throws ELOCKED by default (no silent fallback)", () => { if (!hasProperLockfile() || process.platform === "win32") { return; } @@ -65,19 +65,56 @@ test("withFileLockSync: falls back to unlocked callback on ELOCKED", () => { const release = lockfile.lockSync(filePath, { retries: 0, stale: 10000 }); try { let called = 0; - const result = withFileLockSync(filePath, () => { - called++; - return "fallback-ok"; - }); - assert.equal(result, "fallback-ok"); - assert.equal(called, 1, "callback should run even when lock acquisition fails"); + assert.throws( + () => { + withFileLockSync( + filePath, + () => { + called++; + return "should-not-return"; + }, + { retries: 0 }, + ); + }, + (err: any) => err?.code === "ELOCKED", + ); + assert.equal(called, 0, "callback must not run when lock cannot be acquired"); } finally { release(); rmSync(dir, { recursive: true, force: true }); } }); -test("withFileLock: falls back to unlocked callback on ELOCKED", async () => { +test("withFileLockSync: onLocked=\"skip\" runs callback unlocked on ELOCKED", () => { + if (!hasProperLockfile() || process.platform === "win32") { + return; + } + + const lockfile = require("proper-lockfile"); + const dir = mkdtempSync(join(tmpdir(), "gsd-file-lock-test-")); + const filePath = join(dir, "locked.jsonl"); + writeFileSync(filePath, "{}\n", "utf-8"); + + const release = lockfile.lockSync(filePath, { retries: 0, stale: 10000 }); + try { + let called = 0; + const result = withFileLockSync( + filePath, + () => { + called++; + return "fallback-ok"; + }, + { retries: 0, onLocked: "skip" }, + ); + assert.equal(result, "fallback-ok"); + assert.equal(called, 1, "callback should run when onLocked is skip"); + } finally { + release(); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("withFileLock: throws ELOCKED by default (no silent fallback)", async () => { if (!hasProperLockfile() || process.platform === "win32") { return; } @@ -90,12 +127,49 @@ test("withFileLock: falls back to unlocked callback on ELOCKED", async () => { const release = await lockfile.lock(filePath, { retries: 0, stale: 10000 }); try { let called = 0; - const result = await withFileLock(filePath, async () => { - called++; - return "fallback-ok"; - }); - assert.equal(result, "fallback-ok"); - assert.equal(called, 1, "callback should run even when lock acquisition fails"); + await assert.rejects( + async () => { + await withFileLock( + filePath, + async () => { + called++; + return "should-not-return"; + }, + { retries: 0 }, + ); + }, + (err: any) => err?.code === "ELOCKED", + ); + assert.equal(called, 0, "callback must not run when lock cannot be acquired"); + } finally { + await release(); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("withFileLock: onLocked=\"skip\" runs callback unlocked on ELOCKED", async () => { + if (!hasProperLockfile() || process.platform === "win32") { + return; + } + + const lockfile = require("proper-lockfile"); + const dir = mkdtempSync(join(tmpdir(), "gsd-file-lock-test-")); + const filePath = join(dir, "locked.jsonl"); + writeFileSync(filePath, "{}\n", "utf-8"); + + const release = await lockfile.lock(filePath, { retries: 0, stale: 10000 }); + try { + let called = 0; + const result = await withFileLock( + filePath, + async () => { + called++; + return "fallback-ok"; + }, + { retries: 0, onLocked: "skip" }, + ); + assert.equal(result, "fallback-ok"); + assert.equal(called, 1, "callback should run when onLocked is skip"); } finally { await release(); rmSync(dir, { recursive: true, force: true });