From 4d310e825731dcfac716d8a4706b1879ea92b39f Mon Sep 17 00:00:00 2001 From: Jiaqi Liu Date: Wed, 19 Aug 2026 11:01:49 +0800 Subject: [PATCH] fix(daemon): do not let a compromised lockfile take the supervisor down proper-lockfile defaults onCompromised to `(err) => { throw err; }` and calls it from inside the lock's mtime-refresh filesystem callback, so the throw is an uncaught exception rather than something the caller can handle. None of the daemon's four lock sites passed the option; core/auth-storage.ts already does, which is what the correct handling looks like. The socket-path lease is the dangerous one. The supervisor acquires it at startup and holds it until it exits, while the refresh runs on a timer that cannot fire when the event loop is blocked - and the supervisor blocks it routinely (execFileSync("ps") for process identity, Atomics.wait in the session-lease guard, readFileSync journal loads). A stall past the 5s stale window lets a second supervisor declare the lock stale, rmdir it, and take over; the first supervisor's next refresh then throws. daemon-supervisor.ts installs no uncaughtException handler, so the process dies and every session's control plane goes with it. Record the loss on the lease instead of throwing, and act on it where it matters: prepareDaemonSocketPath fails startup loudly, and cleanupDaemonSocketPath skips the unlink, because the socket at that path may already belong to the successor that stole the lease. The other three sites - the supervisor ownership registry guard, the update-restart coordinator guard, and the session-lease guard - get a non-throwing handler for the same reason. Without this change the new tests report unhandled errors from the refresh callback. --- .../src/cli/daemon-update-restart.ts | 2 + .../coding-agent/src/core/session-lease.ts | 2 + .../src/modes/daemon/daemon-socket.ts | 33 ++++++++++- .../daemon/daemon-supervisor-ownership.ts | 4 ++ .../daemon-socket-lease-compromise.test.ts | 59 +++++++++++++++++++ 5 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 packages/coding-agent/test/daemon-socket-lease-compromise.test.ts 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); +});