Skip to content

Commit 17ffdaa

Browse files
shawndugganshawnduggan
committed
fix(daemon): verify launchd restart port ownership
1 parent 0004091 commit 17ffdaa

2 files changed

Lines changed: 150 additions & 21 deletions

File tree

‎src/daemon/launchd.test.ts‎

Lines changed: 120 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,15 @@ const launchdRestartHandoffState = vi.hoisted(() => ({
5959
>(() => ({ ok: true, value: 7331 })),
6060
}));
6161
const cleanStaleGatewayProcessesSync = vi.hoisted(() =>
62-
vi.fn<(port?: number) => number[]>(() => []),
62+
vi.fn<(port?: number, options?: { protectedPid?: number }) => number[]>(() => []),
6363
);
6464
const inspectPortUsage = vi.hoisted(() =>
65-
vi.fn(async () => ({ port: 18789, status: "free", listeners: [], hints: [] })),
65+
vi.fn<typeof import("../infra/ports-inspect.js").inspectPortUsage>(async () => ({
66+
port: 18789,
67+
status: "free",
68+
listeners: [],
69+
hints: [],
70+
})),
6671
);
6772
const probePortUsage = vi.hoisted(() =>
6873
vi.fn<typeof import("../infra/ports-probe.js").probePortUsage>(async () => "free"),
@@ -309,7 +314,10 @@ vi.mock("./launchd-restart-handoff.js", () => ({
309314
}));
310315

311316
vi.mock("../infra/restart-stale-pids.js", () => ({
312-
cleanStaleGatewayProcessesSync: (port?: number) => cleanStaleGatewayProcessesSync(port),
317+
cleanStaleGatewayProcessesSync: (port?: number, options?: { protectedPid?: number }) =>
318+
options === undefined
319+
? cleanStaleGatewayProcessesSync(port)
320+
: cleanStaleGatewayProcessesSync(port, options),
313321
}));
314322

315323
vi.mock("../infra/ports.js", () => ({
@@ -1345,7 +1353,9 @@ describe("launchd install", () => {
13451353
envFilePath,
13461354
...defaultProgramArguments,
13471355
]);
1348-
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19007);
1356+
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19007, {
1357+
protectedPid: 4242,
1358+
});
13491359
});
13501360

13511361
it("repairs a mangled label-derived service-env wrapper path on restart", async () => {
@@ -1989,8 +1999,11 @@ describe("launchd install", () => {
19891999
const label = "ai.openclaw.gateway";
19902000
const serviceId = `${domain}/${label}`;
19912001
expect(result).toEqual({ outcome: "completed" });
1992-
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(18789);
2002+
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(18789, {
2003+
protectedPid: 4242,
2004+
});
19932005
expect(state.launchctlCalls).toEqual([
2006+
["print", serviceId],
19942007
["enable", serviceId],
19952008
["kickstart", "-k", serviceId],
19962009
]);
@@ -2113,7 +2126,7 @@ describe("launchd install", () => {
21132126
expect(plist).toContain("<key>StandardInPath</key>");
21142127
expect(plist).toContain("<string>/dev/null</string>");
21152128
expect(plist).toContain("<string>/Users/test/Library/Logs/openclaw/gateway.log</string>");
2116-
expect(launchctlCommandNames()).toEqual(["enable", "bootout", "enable", "bootstrap"]);
2129+
expect(launchctlCommandNames()).toEqual(["print", "enable", "bootout", "enable", "bootstrap"]);
21172130
expect(launchctlCommandNames()).not.toContain("kickstart");
21182131
expect(onMutation.mock.calls).toEqual([
21192132
[{ mode: "enable" }],
@@ -2169,7 +2182,7 @@ describe("launchd install", () => {
21692182
restartLaunchAgent({ env, stdout: new PassThrough(), onMutation }),
21702183
).resolves.toEqual({ outcome: "completed" });
21712184

2172-
expect(launchctlCommandNames()).toEqual(["enable", "bootout", "enable", "bootstrap"]);
2185+
expect(launchctlCommandNames()).toEqual(["print", "enable", "bootout", "enable", "bootstrap"]);
21732186
expect(onMutation).toHaveBeenCalledWith({ mode: "bootout" });
21742187
expect(onMutation).toHaveBeenCalledWith({ mode: "bootstrap" });
21752188
});
@@ -2208,7 +2221,14 @@ describe("launchd install", () => {
22082221
stdout: new PassThrough(),
22092222
});
22102223

2211-
expect(launchctlCommandNames()).toEqual(["enable", "bootout", "enable", "bootstrap", "print"]);
2224+
expect(launchctlCommandNames()).toEqual([
2225+
"print",
2226+
"enable",
2227+
"bootout",
2228+
"enable",
2229+
"bootstrap",
2230+
"print",
2231+
]);
22122232
expect(launchctlCommandNames()).not.toContain("kickstart");
22132233
});
22142234

@@ -2223,7 +2243,9 @@ describe("launchd install", () => {
22232243
stdout: new PassThrough(),
22242244
});
22252245

2226-
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19001);
2246+
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19001, {
2247+
protectedPid: 4242,
2248+
});
22272249
});
22282250

22292251
it("ignores invalid configured gateway ports for stale cleanup", async () => {
@@ -2257,8 +2279,10 @@ describe("launchd install", () => {
22572279
stdout: new PassThrough(),
22582280
});
22592281

2260-
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19007);
2261-
expect(inspectPortUsage).not.toHaveBeenCalled();
2282+
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19007, {
2283+
protectedPid: 4242,
2284+
});
2285+
expect(inspectPortUsage).toHaveBeenCalledWith(19007);
22622286
});
22632287

22642288
it("uses the final repeated LaunchAgent port flag for restart stale cleanup", async () => {
@@ -2276,8 +2300,10 @@ describe("launchd install", () => {
22762300
stdout: new PassThrough(),
22772301
});
22782302

2279-
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19008);
2280-
expect(inspectPortUsage).not.toHaveBeenCalled();
2303+
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19008, {
2304+
protectedPid: 4242,
2305+
});
2306+
expect(inspectPortUsage).toHaveBeenCalledWith(19008);
22812307
});
22822308

22832309
it("ignores invalid stored LaunchAgent environment ports for stale cleanup", async () => {
@@ -2299,15 +2325,28 @@ describe("launchd install", () => {
22992325
expect(inspectPortUsage).not.toHaveBeenCalled();
23002326
});
23012327

2302-
it("does not require the configured gateway port to be free before kickstart", async () => {
2328+
it("protects and allows the managed LaunchAgent that owns the gateway port", async () => {
23032329
const env = {
23042330
...createDefaultLaunchdEnv(),
23052331
OPENCLAW_GATEWAY_PORT: "19002",
23062332
};
23072333
inspectPortUsage.mockResolvedValue({
23082334
port: 19002,
23092335
status: "busy",
2310-
listeners: [],
2336+
listeners: [
2337+
{
2338+
pid: 4242,
2339+
command: "node",
2340+
commandLine: "node openclaw.mjs gateway --port 19002",
2341+
address: "TCP 127.0.0.1:19002 (LISTEN)",
2342+
},
2343+
{
2344+
pid: 4242,
2345+
command: "node",
2346+
commandLine: "node openclaw.mjs gateway --port 19002",
2347+
address: "TCP [::1]:19002 (LISTEN)",
2348+
},
2349+
],
23112350
hints: [],
23122351
});
23132352

@@ -2319,14 +2358,78 @@ describe("launchd install", () => {
23192358
const domain = typeof process.getuid === "function" ? `gui/${process.getuid()}` : "gui/501";
23202359
const serviceId = `${domain}/ai.openclaw.gateway`;
23212360
expect(result).toEqual({ outcome: "completed" });
2322-
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19002);
2323-
expect(inspectPortUsage).not.toHaveBeenCalled();
2361+
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19002, {
2362+
protectedPid: 4242,
2363+
});
2364+
expect(inspectPortUsage).toHaveBeenCalledWith(19002);
23242365
expect(state.launchctlCalls).toEqual([
2366+
["print", serviceId],
2367+
["print", serviceId],
23252368
["enable", serviceId],
23262369
["kickstart", "-k", serviceId],
23272370
]);
23282371
});
23292372

2373+
it("rejects an unrelated gateway port owner before mutating the LaunchAgent", async () => {
2374+
const env = {
2375+
...createDefaultLaunchdEnv(),
2376+
OPENCLAW_GATEWAY_PORT: "19002",
2377+
};
2378+
setLaunchAgentPlist({
2379+
env,
2380+
label: "ai.openclaw.gateway",
2381+
programArguments: ["node", "gateway.js"],
2382+
});
2383+
const plistPath = resolveLaunchAgentPlistPath(env);
2384+
const originalPlist = state.files.get(plistPath);
2385+
inspectPortUsage.mockResolvedValue({
2386+
port: 19002,
2387+
status: "busy",
2388+
listeners: [
2389+
{
2390+
pid: 5151,
2391+
command: "python3",
2392+
commandLine: "python3 -m http.server 19002",
2393+
address: "TCP 127.0.0.1:19002 (LISTEN)",
2394+
},
2395+
],
2396+
hints: ["Another process is listening on this port."],
2397+
});
2398+
formatPortDiagnostics.mockReturnValue([
2399+
"Port 19002 is already in use.",
2400+
"- pid 5151: python3 -m http.server 19002",
2401+
]);
2402+
2403+
await expect(
2404+
restartLaunchAgent({
2405+
env,
2406+
stdout: new PassThrough(),
2407+
}),
2408+
).rejects.toThrow(
2409+
[
2410+
"gateway port 19002 is busy but is not verifiably owned by LaunchAgent ai.openclaw.gateway",
2411+
"Port 19002 is already in use.",
2412+
"- pid 5151: python3 -m http.server 19002",
2413+
].join("\n"),
2414+
);
2415+
2416+
const domain = typeof process.getuid === "function" ? `gui/${process.getuid()}` : "gui/501";
2417+
const serviceId = `${domain}/ai.openclaw.gateway`;
2418+
expect(cleanStaleGatewayProcessesSync).toHaveBeenCalledWith(19002, {
2419+
protectedPid: 4242,
2420+
});
2421+
expect(inspectPortUsage).toHaveBeenCalledWith(19002);
2422+
expect(state.launchctlCalls).toEqual([
2423+
["print", serviceId],
2424+
["print", serviceId],
2425+
]);
2426+
expect(state.files.get(plistPath)).toBe(originalPlist);
2427+
expect(state.fileWrites).toHaveLength(0);
2428+
expect(launchctlCommandNames()).not.toContain("enable");
2429+
expect(launchctlCommandNames()).not.toContain("bootout");
2430+
expect(launchctlCommandNames()).not.toContain("kickstart");
2431+
});
2432+
23302433
it("skips stale cleanup when no explicit launch agent port can be resolved", async () => {
23312434
const env = createDefaultLaunchdEnv();
23322435
state.files.clear();

‎src/daemon/launchd.ts‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1334,10 +1334,36 @@ export async function restartLaunchAgent({
13341334

13351335
const cleanupPort = await resolveLaunchAgentGatewayPort(serviceEnv);
13361336
if (cleanupPort !== null) {
1337-
// During a launchd restart, the active LaunchAgent is expected to own the
1338-
// port until `kickstart -k` replaces it. Reap stale gateway listeners, but
1339-
// do not require the port to be free before asking launchd to restart.
1340-
cleanStaleGatewayProcessesSync(cleanupPort);
1337+
const runtimeBeforeCleanup = await readLaunchAgentRuntime(serviceEnv);
1338+
// The supervised process is not stale. Protect its authoritative launchd
1339+
// PID while still removing any other verified gateway process on the port.
1340+
if (runtimeBeforeCleanup.pid === undefined) {
1341+
cleanStaleGatewayProcessesSync(cleanupPort);
1342+
} else {
1343+
cleanStaleGatewayProcessesSync(cleanupPort, {
1344+
protectedPid: runtimeBeforeCleanup.pid,
1345+
});
1346+
}
1347+
const diagnostics = await inspectPortUsage(cleanupPort).catch(() => null);
1348+
if (diagnostics?.status === "busy") {
1349+
const runtime = await readLaunchAgentRuntime(serviceEnv);
1350+
const managedPid = runtime.pid;
1351+
// Only the current supervised PID may keep the port busy before a
1352+
// disruptive restart. Re-read after cleanup to close over a concurrent
1353+
// launchd respawn rather than trusting the protected pre-cleanup PID.
1354+
const ownedByLaunchAgent =
1355+
managedPid !== undefined &&
1356+
diagnostics.listeners.length > 0 &&
1357+
diagnostics.listeners.every((listener) => listener.pid === managedPid);
1358+
if (!ownedByLaunchAgent) {
1359+
throw new Error(
1360+
[
1361+
`gateway port ${cleanupPort} is busy but is not verifiably owned by LaunchAgent ${label}`,
1362+
...formatPortDiagnostics(diagnostics),
1363+
].join("\n"),
1364+
);
1365+
}
1366+
}
13411367
}
13421368
const plistReloadNeeded = await rewriteLaunchAgentPlistForRestart({
13431369
env: serviceEnv,

0 commit comments

Comments
 (0)