diff --git a/packages/pi-coding-agent/src/core/auth-storage.ts b/packages/pi-coding-agent/src/core/auth-storage.ts index 5ebffc12a..e921328f2 100644 --- a/packages/pi-coding-agent/src/core/auth-storage.ts +++ b/packages/pi-coding-agent/src/core/auth-storage.ts @@ -18,9 +18,9 @@ import { import { getOAuthApiKey, getOAuthProvider, getOAuthProviders } from "@gsd/pi-ai/oauth"; import { chmodSync, existsSync, mkdirSync, readFileSync, writeFileSync } from "fs"; import { dirname, join } from "path"; -import lockfile from "proper-lockfile"; import { getAgentDir } from "../config.js"; import { AUTH_LOCK_STALE_MS } from "./constants.js"; +import { acquireLockAsync, acquireLockSyncWithRetry } from "./lock-utils.js"; import { resolveConfigValue } from "./resolve-config-value.js"; export type ApiKeyCredential = { @@ -67,40 +67,13 @@ export class FileAuthStorageBackend implements AuthStorageBackend { } } - private acquireLockSyncWithRetry(path: string): () => void { - const maxAttempts = 10; - const delayMs = 20; - let lastError: unknown; - - for (let attempt = 1; attempt <= maxAttempts; attempt++) { - try { - return lockfile.lockSync(path, { realpath: false }); - } catch (error) { - const code = - typeof error === "object" && error !== null && "code" in error - ? String((error as { code?: unknown }).code) - : undefined; - if (code !== "ELOCKED" || attempt === maxAttempts) { - throw error; - } - lastError = error; - const start = Date.now(); - while (Date.now() - start < delayMs) { - // Sleep synchronously to avoid changing callers to async. - } - } - } - - throw (lastError as Error) ?? new Error("Failed to acquire auth storage lock"); - } - withLock(fn: (current: string | undefined) => LockResult): T { this.ensureParentDir(); this.ensureFileExists(); let release: (() => void) | undefined; try { - release = this.acquireLockSyncWithRetry(this.authPath); + release = acquireLockSyncWithRetry(this.authPath); const current = existsSync(this.authPath) ? readFileSync(this.authPath, "utf-8") : undefined; const { result, next } = fn(current); if (next !== undefined) { @@ -129,15 +102,8 @@ export class FileAuthStorageBackend implements AuthStorageBackend { }; try { - release = await lockfile.lock(this.authPath, { - retries: { - retries: 10, - factor: 2, - minTimeout: 100, - maxTimeout: 10000, - randomize: true, - }, - stale: AUTH_LOCK_STALE_MS, + release = await acquireLockAsync(this.authPath, { + staleMs: AUTH_LOCK_STALE_MS, onCompromised: (err) => { lockCompromised = true; lockCompromisedError = err; diff --git a/packages/pi-coding-agent/src/core/lock-utils.ts b/packages/pi-coding-agent/src/core/lock-utils.ts new file mode 100644 index 000000000..64f77aa7c --- /dev/null +++ b/packages/pi-coding-agent/src/core/lock-utils.ts @@ -0,0 +1,113 @@ +/** + * Shared file-locking utilities built on `proper-lockfile`. + * + * Centralises the synchronous retry-loop and async lock/release patterns + * that were previously duplicated across auth-storage, session-manager, + * settings-manager, and models-json-writer. + */ + +import lockfile from "proper-lockfile"; + +const DEFAULT_MAX_ATTEMPTS = 10; +const DEFAULT_DELAY_MS = 20; + +/** + * Acquire a synchronous file lock with retry. + * + * Retries up to `maxAttempts` times when the lock is held by another process + * (ELOCKED), using a busy-wait between attempts. + * + * @returns A release function to unlock. + * @throws On non-ELOCKED errors or when all attempts are exhausted. + */ +export function acquireLockSyncWithRetry( + lockPath: string, + maxAttempts: number = DEFAULT_MAX_ATTEMPTS, + delayMs: number = DEFAULT_DELAY_MS, +): () => void { + let lastError: unknown; + + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + return lockfile.lockSync(lockPath, { realpath: false }); + } catch (error) { + const code = + typeof error === "object" && error !== null && "code" in error + ? String((error as { code?: unknown }).code) + : undefined; + if (code !== "ELOCKED" || attempt === maxAttempts) { + throw error; + } + lastError = error; + const start = Date.now(); + while (Date.now() - start < delayMs) { + // Busy-wait to avoid changing callers to async. + } + } + } + + throw (lastError as Error) ?? new Error("Failed to acquire file lock"); +} + +/** + * Non-throwing variant of {@link acquireLockSyncWithRetry}. + * + * Returns `undefined` instead of throwing when the lock cannot be acquired, + * allowing callers to proceed without the lock rather than losing data. + */ +export function tryAcquireLockSync( + lockPath: string, + maxAttempts: number = DEFAULT_MAX_ATTEMPTS, + delayMs: number = DEFAULT_DELAY_MS, +): (() => void) | undefined { + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + return lockfile.lockSync(lockPath, { realpath: false }); + } catch (error) { + const code = + typeof error === "object" && error !== null && "code" in error + ? String((error as { code?: unknown }).code) + : undefined; + if (code !== "ELOCKED" || attempt === maxAttempts) { + // Non-fatal: proceed without lock rather than losing data + return undefined; + } + const start = Date.now(); + while (Date.now() - start < delayMs) { + // Busy-wait to avoid changing callers to async. + } + } + } + return undefined; +} + +export interface AsyncLockOptions { + /** Maximum staleness in ms before the lock is considered stale. */ + staleMs?: number; + /** Called if the lock is compromised while held. */ + onCompromised?: (err: Error) => void; +} + +/** + * Acquire an async file lock with retries and optional staleness detection. + * + * Uses `proper-lockfile`'s async API with exponential-backoff retries. + * + * @returns A release function (async) to unlock. + */ +export async function acquireLockAsync( + lockPath: string, + options?: AsyncLockOptions, +): Promise<() => Promise> { + return lockfile.lock(lockPath, { + retries: { + retries: 10, + factor: 2, + minTimeout: 100, + maxTimeout: 10000, + randomize: true, + }, + stale: options?.staleMs, + onCompromised: options?.onCompromised, + }); +} diff --git a/packages/pi-coding-agent/src/core/session-manager.ts b/packages/pi-coding-agent/src/core/session-manager.ts index ef60d4120..e0c54891b 100644 --- a/packages/pi-coding-agent/src/core/session-manager.ts +++ b/packages/pi-coding-agent/src/core/session-manager.ts @@ -16,8 +16,8 @@ import { import { atomicWriteFileSync } from "./fs-utils.js"; import { readdir, readFile, stat } from "fs/promises"; import { join, resolve } from "path"; -import lockfile from "proper-lockfile"; import { getAgentDir as getDefaultAgentDir, getBlobsDir, getSessionsDir } from "../config.js"; +import { tryAcquireLockSync } from "./lock-utils.js"; import { type BashExecutionMessage, type CustomMessage, @@ -955,39 +955,12 @@ export class SessionManager { } } - private acquireSessionLock(path: string): (() => void) | undefined { - const maxAttempts = 10; - const delayMs = 20; - let lastError: unknown; - - for (let attempt = 1; attempt <= maxAttempts; attempt++) { - try { - return lockfile.lockSync(path, { realpath: false }); - } catch (error) { - const code = - typeof error === "object" && error !== null && "code" in error - ? String((error as { code?: unknown }).code) - : undefined; - if (code !== "ELOCKED" || attempt === maxAttempts) { - // Non-fatal: proceed without lock rather than losing data - return undefined; - } - lastError = error; - const start = Date.now(); - while (Date.now() - start < delayMs) { - // Busy-wait to avoid async - } - } - } - return undefined; - } - private _rewriteFile(): void { if (!this.persist || !this.sessionFile) return; const content = `${this.fileEntries.map((e) => JSON.stringify(e)).join("\n")}\n`; let release: (() => void) | undefined; try { - release = this.acquireSessionLock(this.sessionFile); + release = tryAcquireLockSync(this.sessionFile); atomicWriteFileSync(this.sessionFile, content); } finally { release?.(); @@ -1026,7 +999,7 @@ export class SessionManager { let release: (() => void) | undefined; try { - release = this.acquireSessionLock(this.sessionFile); + release = tryAcquireLockSync(this.sessionFile); if (!this.flushed) { for (const e of this.fileEntries) { const prepared = prepareForPersistence(e, this.blobStore) as FileEntry;