Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/coding-agent/src/cli/daemon-update-restart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,8 @@ async function withCoordinatorRegistryGuard<T>(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();
Expand Down
2 changes: 2 additions & 0 deletions packages/coding-agent/src/core/session-lease.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ function withLeaseGuard<T>(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) {
Expand Down
33 changes: 32 additions & 1 deletion packages/coding-agent/src/modes/daemon/daemon-socket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>,
) {}

/**
* 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<void> {
if (this.released) {
return;
Expand All @@ -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,
Expand All @@ -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<void> {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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 {
Expand All @@ -173,6 +203,7 @@ export function cleanupDaemonSocketPath(
stale: DAEMON_SOCKET_LOCK_STALE_MS,
update: DAEMON_SOCKET_LOCK_UPDATE_MS,
retries: 0,
onCompromised: () => {},
});
} catch {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,10 @@ async function withDaemonSupervisorRegistryGuard<T>(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();
Expand Down
59 changes: 59 additions & 0 deletions packages/coding-agent/test/daemon-socket-lease-compromise.test.ts
Original file line number Diff line number Diff line change
@@ -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<void> {
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);
});