diff --git a/packages/coding-agent/src/cli/daemon-update-restart.ts b/packages/coding-agent/src/cli/daemon-update-restart.ts index 6afcae9fdb..399b19deb7 100644 --- a/packages/coding-agent/src/cli/daemon-update-restart.ts +++ b/packages/coding-agent/src/cli/daemon-update-restart.ts @@ -335,6 +335,8 @@ async function withCoordinatorRegistryGuard(registryDir: string, action: () = minTimeout: COORDINATOR_REGISTRY_LOCK_RETRY_MS, maxTimeout: COORDINATOR_REGISTRY_LOCK_RETRY_MS, }, + // Never rethrow: proper-lockfile invokes this from a filesystem callback. + onCompromised: () => {}, }); try { return await action(); diff --git a/packages/coding-agent/src/core/session-lease.ts b/packages/coding-agent/src/core/session-lease.ts index 6c4e2975cf..7e6a9cf047 100644 --- a/packages/coding-agent/src/core/session-lease.ts +++ b/packages/coding-agent/src/core/session-lease.ts @@ -193,6 +193,8 @@ function withLeaseGuard(directory: string, action: () => T): T { realpath: false, lockfilePath: `${directory}.guard`, stale: 5000, + // Never rethrow: proper-lockfile invokes this from a filesystem callback. + onCompromised: () => {}, }); break; } catch (error) { diff --git a/packages/coding-agent/src/modes/daemon/daemon-socket.ts b/packages/coding-agent/src/modes/daemon/daemon-socket.ts index bed8a705ac..b9f813f655 100644 --- a/packages/coding-agent/src/modes/daemon/daemon-socket.ts +++ b/packages/coding-agent/src/modes/daemon/daemon-socket.ts @@ -15,12 +15,26 @@ const DAEMON_SOCKET_LOCK_UPDATE_MS = 1000; export class DaemonSocketPathLease { private released = false; + private compromised: Error | undefined; constructor( readonly socketPath: string, private readonly releaseLock: () => Promise, ) {} + /** + * Record that another process stole this lease after judging it stale. Callers + * observe it through assertSocketLease, which fails where the lease is actually + * relied on rather than from inside a filesystem callback. + */ + markCompromised(error: Error): void { + this.compromised ??= error; + } + + get compromisedError(): Error | undefined { + return this.compromised; + } + async release(): Promise { if (this.released) { return; @@ -47,6 +61,7 @@ export async function acquireDaemonSocketPathLease(socketPath: string): Promise< if (process.platform === "win32") { return undefined; } + let lease: DaemonSocketPathLease | undefined; const releaseLock = await lockfile.lock(socketPath, { realpath: false, stale: DAEMON_SOCKET_LOCK_STALE_MS, @@ -57,8 +72,14 @@ export async function acquireDaemonSocketPathLease(socketPath: string): Promise< minTimeout: DAEMON_SOCKET_RELEASE_POLL_MS, maxTimeout: DAEMON_SOCKET_RELEASE_POLL_MS, }, + // proper-lockfile defaults onCompromised to `throw err`, and it calls it from + // inside the mtime-refresh filesystem callback. This lease is held for the + // supervisor's whole lifetime, so a stall past `stale` that lets another + // process steal it would take the supervisor down with an uncaught exception. + onCompromised: (error) => lease?.markCompromised(error), }); - return new DaemonSocketPathLease(socketPath, releaseLock); + lease = new DaemonSocketPathLease(socketPath, releaseLock); + return lease; } export async function prepareDaemonSocketPath(socketPath: string, lease?: DaemonSocketPathLease): Promise { @@ -69,6 +90,10 @@ export async function prepareDaemonSocketPath(socketPath: string, lease?: Daemon } if (lease) { assertSocketLease(socketPath, lease); + const compromised = lease.compromisedError; + if (compromised) { + throw new Error(`Daemon socket lease for ${socketPath} was compromised: ${compromised.message}`); + } await prepareUnixDaemonSocketPath(socketPath); return; } @@ -159,6 +184,11 @@ export function cleanupDaemonSocketPath( } if (lease) { assertSocketLease(socketPath, lease); + if (lease.compromisedError) { + // Another process took the lease over, so the socket at this path may be + // its own. Leave it alone rather than unlinking a live successor's socket. + return; + } try { cleanupUnixDaemonSocketPath(socketPath, expectedIdentity); } catch { @@ -173,6 +203,7 @@ export function cleanupDaemonSocketPath( stale: DAEMON_SOCKET_LOCK_STALE_MS, update: DAEMON_SOCKET_LOCK_UPDATE_MS, retries: 0, + onCompromised: () => {}, }); } catch { return; diff --git a/packages/coding-agent/src/modes/daemon/daemon-supervisor-ownership.ts b/packages/coding-agent/src/modes/daemon/daemon-supervisor-ownership.ts index 18fbd43f76..45340ad6c9 100644 --- a/packages/coding-agent/src/modes/daemon/daemon-supervisor-ownership.ts +++ b/packages/coding-agent/src/modes/daemon/daemon-supervisor-ownership.ts @@ -367,6 +367,10 @@ async function withDaemonSupervisorRegistryGuard(registryDir: string, action: minTimeout: REGISTRY_LOCK_RETRY_MS, maxTimeout: REGISTRY_LOCK_RETRY_MS, }, + // Never rethrow: proper-lockfile invokes this from a filesystem callback, so a + // throw here is an uncaught exception rather than a failure the guard can + // report. The guarded action still runs and the release below is a no-op. + onCompromised: () => {}, }); try { return await action(); diff --git a/packages/coding-agent/test/daemon-socket-lease-compromise.test.ts b/packages/coding-agent/test/daemon-socket-lease-compromise.test.ts new file mode 100644 index 0000000000..f9e881f887 --- /dev/null +++ b/packages/coding-agent/test/daemon-socket-lease-compromise.test.ts @@ -0,0 +1,59 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { + acquireDaemonSocketPathLease, + cleanupDaemonSocketPath, + prepareDaemonSocketPath, +} from "../src/modes/daemon/daemon-socket.js"; + +const isWindows = process.platform === "win32"; + +function delay(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +describe.skipIf(isWindows)("daemon socket path lease", () => { + // proper-lockfile refreshes the lock's mtime on a timer and, when that refresh + // finds the lock gone, calls onCompromised - which defaults to rethrowing from + // inside the filesystem callback. The supervisor holds this lease for its whole + // lifetime and installs no uncaughtException handler, so a lock steal used to + // take the entire control plane down. Any regression surfaces here as an + // unhandled error rather than as a failed assertion. + it("records a stolen lease instead of throwing from the refresh callback", async () => { + const dir = mkdtempSync(join(tmpdir(), "pi-socket-lease-")); + const socketPath = join(dir, "daemon.sock"); + + const lease = await acquireDaemonSocketPathLease(socketPath); + expect(lease).toBeDefined(); + if (!lease) return; + + // Simulate another process judging the lock stale and taking it over. + rmSync(`${socketPath}.lock`, { recursive: true, force: true }); + await delay(2_000); + + expect(lease.compromisedError).toBeDefined(); + await lease.release().catch(() => undefined); + rmSync(dir, { recursive: true, force: true }); + }, 15_000); + + it("fails startup and skips cleanup once the lease is compromised", async () => { + const dir = mkdtempSync(join(tmpdir(), "pi-socket-lease-")); + const socketPath = join(dir, "daemon.sock"); + + const lease = await acquireDaemonSocketPathLease(socketPath); + expect(lease).toBeDefined(); + if (!lease) return; + + rmSync(`${socketPath}.lock`, { recursive: true, force: true }); + await delay(2_000); + + await expect(prepareDaemonSocketPath(socketPath, lease)).rejects.toThrow(/compromised/); + // Cleanup must not unlink a path a successor may already own, and must not throw. + expect(() => cleanupDaemonSocketPath(socketPath, undefined, lease)).not.toThrow(); + + await lease.release().catch(() => undefined); + rmSync(dir, { recursive: true, force: true }); + }, 15_000); +});