fix(hopper): wait for late bridges during cleanup (#1866)

This commit is contained in:
morluto
2026-10-11 22:50:27 +08:00
committed by GitHub
parent 1f531fc257
commit cf7477eb46
8 changed files with 323 additions and 205 deletions
+25 -5
View File
@@ -34,7 +34,10 @@ import type { ProgressReporter } from "../application/ProgressReporter.js";
import { silentLogger } from "../logger.js";
import type { Logger } from "pino";
import { PrivateRuntimeRoot } from "../process/PrivateRuntimeRoot.js";
import { ProviderStartupDeadline } from "../process/ProviderDeadline.js";
import {
ProviderStartupDeadline,
waitForAbortableDelay,
} from "../process/ProviderDeadline.js";
import { redactCapturedTransportCredential } from "../process/ProviderDiagnosticRedaction.js";
import { ProviderRunLineage } from "../process/ProviderRunLineage.js";
import {
@@ -423,7 +426,7 @@ export class HopperClient {
);
if (!health.ok) {
return this.#startupFailure(
deadline.signal.aborted
deadline.signal.aborted || health.error instanceof HopperTimeoutError
? await this.#startupInterruption(deadline)
: health.error,
);
@@ -542,11 +545,28 @@ export class HopperClient {
);
this.#closing = true;
try {
const connected = await connectHopperSocketOnce(
join(root.path, "bridge.sock"),
const socketPath = join(root.path, "bridge.sock");
let connected = await connectHopperSocketOnce(
socketPath,
controller.signal,
);
if (!connected.ok) return;
while (!connected.ok) {
const cause = connected.error.cause;
if (
!(
cause instanceof Error &&
"code" in cause &&
(cause.code === "ENOENT" || cause.code === "ECONNREFUSED")
)
)
return;
if ((await waitForAbortableDelay(50, controller.signal)) === "aborted")
return;
connected = await connectHopperSocketOnce(
socketPath,
controller.signal,
);
}
const socket = connected.value;
this.#socket = socket;
this.#attachSocket(socket);
@@ -1,6 +1,7 @@
import { spawn, type ChildProcess } from "node:child_process";
import { access, copyFile, readFile, rm, writeFile } from "node:fs/promises";
import { dirname, resolve } from "node:path";
import { setTimeout as delay } from "node:timers/promises";
import { Client } from "@modelcontextprotocol/client";
import { StdioClientTransport } from "@modelcontextprotocol/client/stdio";
@@ -14,91 +15,106 @@ import { acquireHopperTargetLease } from "../../../src/hopper/HopperTargetLease.
const statusSchema = z.object({ result: z.object({ open: z.boolean() }) });
const test = workspaceCliTest.skipIf(process.platform !== "darwin");
test("retains a denial-shaped failure when caller arguments can alter the helper's AppleScript", async ({
workspace,
}) => {
for (const source of ["target", "loader"] as const) {
const target = workspace.path(
source === "target" ? 'quoted"target' : "loader-target",
);
await copyFile("/usr/bin/true", target);
const mode = await workspace.write(
`${source}-mode`,
"denied-after-handoff",
);
const receipt = workspace.path(`${source}-bootstrap`);
const loaderArgs = [
resolve("tests/fixtures/hopperLaunchAdmission.mjs"),
mode,
receipt,
...(source === "loader" ? ['caller"loader'] : []),
];
const transport = new StdioClientTransport({
command: process.execPath,
args: [resolve("dist/main.js")],
stderr: "pipe",
env: {
...workspace.environment,
PATH: process.env.PATH ?? "",
REA_ANALYSIS_PROVIDER: "hopper",
HOPPER_LAUNCHER_PATH: process.execPath,
HOPPER_LOADER_ARGS_JSON: JSON.stringify(loaderArgs),
REA_LOG_LEVEL: "silent",
},
});
const mcp = new Client({ name: "hopper-literal-boundary", version: "1" });
let runtime: string | undefined;
onTestFinished(async () => {
await mcp.close();
await transport.close();
// The fixture created only this marker, never a native GUI document.
if (runtime !== undefined)
await rm(runtime, { recursive: true, force: true });
const acquired = await acquireHopperTargetLease({
targetPath: target,
targetKind: "executable",
loaderArgs,
runId: "literal-boundary-test-cleanup",
test(
"retains a denial-shaped failure when caller arguments can alter the helper's AppleScript",
{ timeout: 90_000 },
async ({ workspace }) => {
for (const source of ["target", "loader"] as const) {
const target = workspace.path(
source === "target" ? 'quoted"target' : "loader-target",
);
await copyFile("/usr/bin/true", target);
const mode = await workspace.write(
`${source}-mode`,
"denied-after-handoff",
);
const receipt = workspace.path(`${source}-bootstrap`);
const loaderArgs = [
resolve("tests/fixtures/hopperLaunchAdmission.mjs"),
mode,
receipt,
...(source === "loader" ? ['caller"loader'] : []),
];
const transport = new StdioClientTransport({
command: process.execPath,
args: [resolve("dist/main.js")],
stderr: "pipe",
env: {
...workspace.environment,
PATH: process.env.PATH ?? "",
REA_ANALYSIS_PROVIDER: "hopper",
HOPPER_LAUNCHER_PATH: process.execPath,
HOPPER_LOADER_ARGS_JSON: JSON.stringify(loaderArgs),
REA_LOG_LEVEL: "silent",
},
});
if (!acquired.acquired) throw new Error("Fixture lease remained live");
await acquired.lease.release();
});
await mcp.connect(transport);
const call = (name: string, arguments_: Record<string, string> = {}) =>
mcp.callTool({ name, arguments: arguments_ });
expect((await call("open_binary", { path: target })).isError).not.toBe(
true,
);
const first = parseMcpToolError(await call("current_document"));
runtime = dirname(await readFile(receipt, "utf8"));
expect(first.error).toMatchObject({
code: "cleanup_incomplete",
details: {
diagnostics: {
primary_error: {
code: "provider_unavailable",
details: {
launcher: {
exit_code: 2,
stderr: {
text: expect.stringContaining(
"Not authorized to send Apple events",
),
const mcp = new Client({ name: "hopper-literal-boundary", version: "1" });
let runtime: string | undefined;
onTestFinished(async () => {
// SDK close can return immediately after SIGKILL. Wait for its actual
// child-close event before recovering this process's retained lease.
let serverExited: Promise<void> | undefined;
if (transport.pid !== null) {
const onclose = transport.onclose;
serverExited = new Promise<void>((resolveExit) => {
transport.onclose = () => {
onclose?.();
resolveExit();
};
});
}
await mcp.close();
await transport.close();
await serverExited;
// The fixture created only this marker, never a native GUI document.
if (runtime !== undefined)
await rm(runtime, { recursive: true, force: true });
const acquired = await acquireHopperTargetLease({
targetPath: target,
targetKind: "executable",
loaderArgs,
runId: "literal-boundary-test-cleanup",
});
if (!acquired.acquired) throw new Error("Fixture lease remained live");
await acquired.lease.release();
});
await mcp.connect(transport);
const call = (name: string, arguments_: Record<string, string> = {}) =>
mcp.callTool({ name, arguments: arguments_ });
expect((await call("open_binary", { path: target })).isError).not.toBe(
true,
);
const first = parseMcpToolError(await call("current_document"));
runtime = dirname(await readFile(receipt, "utf8"));
expect(first.error).toMatchObject({
code: "cleanup_incomplete",
details: {
diagnostics: {
primary_error: {
code: "provider_unavailable",
details: {
launcher: {
exit_code: 2,
stderr: {
text: expect.stringContaining(
"Not authorized to send Apple events",
),
},
},
},
},
},
},
},
});
await access(`${receipt}.document-created`);
await access(runtime);
expect(parseMcpToolError(await call("list_procedures"))).toEqual(first);
expect(parseMcpToolError(await call("close_binary")).error.code).toBe(
"cleanup_incomplete",
);
}
});
});
await access(`${receipt}.document-created`);
await access(runtime);
expect(parseMcpToolError(await call("list_procedures"))).toEqual(first);
expect(parseMcpToolError(await call("close_binary")).error.code).toBe(
"cleanup_incomplete",
);
}
},
);
test("recovers a denied Hopper launch through the same stdio MCP session and CLI", async ({
workspace,
@@ -185,120 +201,133 @@ test("recovers a denied Hopper launch through the same stdio MCP session and CLI
).toBe(false);
});
test("preserves a timed-out handoff and closes only a late bridge with matching run identity", async ({
workspace,
}) => {
const target = workspace.path("target");
await copyFile("/usr/bin/true", target);
const mode = await workspace.write("mode", "handoff");
const receipt = workspace.path("bootstrap-path");
const transport = new StdioClientTransport({
command: process.execPath,
args: [resolve("dist/main.js")],
stderr: "pipe",
env: {
...workspace.environment,
PATH: process.env.PATH ?? "",
REA_ANALYSIS_PROVIDER: "hopper",
HOPPER_LAUNCHER_PATH: process.execPath,
HOPPER_LOADER_ARGS_JSON: JSON.stringify([
resolve("tests/fixtures/hopperLaunchAdmission.mjs"),
mode,
receipt,
]),
REA_HOPPER_STARTUP_TIMEOUT_MS: "2000",
REA_LOG_LEVEL: "silent",
},
});
const mcp = new Client({ name: "hopper-late-readiness", version: "1" });
const children: ChildProcess[] = [];
let runtime: string | undefined;
const stop = async (child: ChildProcess) => {
if (child.exitCode !== null || child.signalCode !== null) return;
const exited = new Promise<void>((resolveExit) =>
child.once("exit", () => resolveExit()),
test(
"preserves a timed-out handoff and closes only a late bridge with matching run identity",
{ timeout: 60_000 },
async ({ workspace }) => {
const target = workspace.path("target");
await copyFile("/usr/bin/true", target);
const mode = await workspace.write("mode", "handoff");
const receipt = workspace.path("bootstrap-path");
const transport = new StdioClientTransport({
command: process.execPath,
args: [resolve("dist/main.js")],
stderr: "pipe",
env: {
...workspace.environment,
PATH: process.env.PATH ?? "",
REA_ANALYSIS_PROVIDER: "hopper",
HOPPER_LAUNCHER_PATH: process.execPath,
HOPPER_LOADER_ARGS_JSON: JSON.stringify([
resolve("tests/fixtures/hopperLaunchAdmission.mjs"),
mode,
receipt,
]),
REA_HOPPER_STARTUP_TIMEOUT_MS: "2000",
REA_LOG_LEVEL: "silent",
},
});
const mcp = new Client({ name: "hopper-late-readiness", version: "1" });
const children: ChildProcess[] = [];
let runtime: string | undefined;
const stop = async (child: ChildProcess) => {
if (child.exitCode !== null || child.signalCode !== null) return;
const exited = new Promise<void>((resolveExit) =>
child.once("exit", () => resolveExit()),
);
child.kill("SIGKILL");
await exited;
};
onTestFinished(async () => {
await Promise.all(children.map(stop));
await mcp.close();
await transport.close();
if (runtime !== undefined)
await rm(runtime, { recursive: true, force: true });
});
await mcp.connect(transport);
const call = (name: string, arguments_: Record<string, string> = {}) =>
mcp.callTool({ name, arguments: arguments_ });
expect((await call("open_binary", { path: target })).isError).not.toBe(
true,
);
child.kill("SIGKILL");
await exited;
};
onTestFinished(async () => {
await Promise.all(children.map(stop));
await mcp.close();
await transport.close();
if (runtime !== undefined)
await rm(runtime, { recursive: true, force: true });
});
await mcp.connect(transport);
const call = (name: string, arguments_: Record<string, string> = {}) =>
mcp.callTool({ name, arguments: arguments_ });
expect((await call("open_binary", { path: target })).isError).not.toBe(true);
const initial = parseMcpToolError(await call("current_document"));
expect(initial.error).toMatchObject({
code: "cleanup_incomplete",
details: {
diagnostics: {
primary_error: {
code: "provider_timeout",
details: { timeout_ms: 2000, launcher: { exit_code: 0 } },
const initial = parseMcpToolError(await call("current_document"));
expect(initial.error).toMatchObject({
code: "cleanup_incomplete",
details: {
diagnostics: {
primary_error: {
code: "provider_timeout",
details: { timeout_ms: 2000, launcher: { exit_code: 0 } },
},
},
},
},
});
expect(parseMcpToolError(await call("list_procedures"))).toEqual(initial);
const status = z.object({
result: z.object({
provider_operation_health: z.object({
state: z.literal("not_started"),
stage: z.literal("launch"),
});
expect(parseMcpToolError(await call("list_procedures"))).toEqual(initial);
const status = z.object({
result: z.object({
provider_operation_health: z.object({
state: z.literal("not_started"),
stage: z.literal("launch"),
}),
}),
}),
});
status.parse((await call("binary_session")).structuredContent);
expect(parseMcpToolError(await call("close_binary")).error.code).toBe(
"cleanup_incomplete",
);
const bootstrap = await readFile(await readFile(receipt, "utf8"), "utf8");
const setting = (name: string): string => {
const line = bootstrap
.split("\n")
.find((value) => value.startsWith(`${name} = `));
if (line === undefined)
throw new Error(`Missing bootstrap setting ${name}`);
return z.string().parse(JSON.parse(line.slice(name.length + 3)));
};
const socket = setting("REA_SOCKET");
const token = setting("REA_TOKEN");
const runId = setting("REA_RUN_ID");
runtime = dirname(socket);
const lateBridge = async (identity: string, credential = token) => {
const child = spawn(
process.execPath,
[resolve("tests/fixtures/fakeHopper.mjs"), socket, credential, identity],
{ stdio: "ignore" },
);
children.push(child);
await vi.waitFor(() => access(socket));
return child;
};
for (const [identity, credential] of [
["different-run", token],
[runId, "different-token"],
] as const) {
const wrong = await lateBridge(identity, credential);
});
status.parse((await call("binary_session")).structuredContent);
expect(parseMcpToolError(await call("close_binary")).error.code).toBe(
"cleanup_incomplete",
);
expect(wrong.exitCode).toBeNull();
await access(runtime);
await stop(wrong);
await rm(socket, { force: true });
}
const owned = await lateBridge(runId);
expect((await call("close_binary")).isError).not.toBe(true);
await vi.waitFor(() => expect(owned.exitCode).toBe(0));
await expect(access(runtime)).rejects.toMatchObject({ code: "ENOENT" });
await writeFile(mode, "bridge");
expect((await call("open_binary", { path: target })).isError).not.toBe(true);
expect((await call("current_document")).isError).not.toBe(true);
expect((await call("close_binary")).isError).not.toBe(true);
});
const bootstrap = await readFile(await readFile(receipt, "utf8"), "utf8");
const setting = (name: string): string => {
const line = bootstrap
.split("\n")
.find((value) => value.startsWith(`${name} = `));
if (line === undefined)
throw new Error(`Missing bootstrap setting ${name}`);
return z.string().parse(JSON.parse(line.slice(name.length + 3)));
};
const socket = setting("REA_SOCKET");
const token = setting("REA_TOKEN");
const runId = setting("REA_RUN_ID");
runtime = dirname(socket);
const lateBridge = async (identity: string, credential = token) => {
const child = spawn(
process.execPath,
[
resolve("tests/fixtures/fakeHopper.mjs"),
socket,
credential,
identity,
],
{ stdio: "ignore" },
);
children.push(child);
await vi.waitFor(() => access(socket));
return child;
};
for (const [identity, credential] of [
["different-run", token],
[runId, "different-token"],
] as const) {
const wrong = await lateBridge(identity, credential);
expect(parseMcpToolError(await call("close_binary")).error.code).toBe(
"cleanup_incomplete",
);
expect(wrong.exitCode).toBeNull();
await access(runtime);
await stop(wrong);
await rm(socket, { force: true });
}
const closing = call("close_binary");
await delay(150);
const owned = await lateBridge(runId);
expect((await closing).isError).not.toBe(true);
await vi.waitFor(() => expect(owned.exitCode).toBe(0));
await expect(access(runtime)).rejects.toMatchObject({ code: "ENOENT" });
await writeFile(mode, "bridge");
expect((await call("open_binary", { path: target })).isError).not.toBe(
true,
);
expect((await call("current_document")).isError).not.toBe(true);
expect((await call("close_binary")).isError).not.toBe(true);
},
);
@@ -29,6 +29,7 @@ describe("Hopper launcher failure through the one-shot CLI", () => {
// Hopper target leases are acquired only for the macOS launcher route.
workspaceCliTest.skipIf(process.platform !== "darwin")(
"exits after a retained-cleanup failure and releases the target to a retry",
{ timeout: 90_000 },
async ({ cli, workspace }) => {
const launcherPath = await workspace.write(
"failure-helper",
@@ -58,7 +59,7 @@ describe("Hopper launcher failure through the one-shot CLI", () => {
HOPPER_LOADER_ARGS_JSON: JSON.stringify(loaderArgs),
REA_LOG_LEVEL: "silent",
},
timeoutMs: 30_000,
timeoutMs: 45_000,
});
const retained = RetainedRuntime.safeParse(result.json);
if (retained.success)
@@ -82,7 +82,7 @@ const clients: HopperClient[] = [];
afterEach(async () => {
await Promise.all(clients.splice(0).map((client) => client.close()));
});
}, 45_000);
describe("HopperClient cleanup", () => {
it("waits for an operation reply without a fixed request deadline", async () => {
@@ -1,5 +1,7 @@
import { spawn, type ChildProcess } from "node:child_process";
import { execFile, spawn, type ChildProcess } from "node:child_process";
import { access, rm } from "node:fs/promises";
import { setTimeout as delay } from "node:timers/promises";
import { promisify } from "node:util";
import { expect, it, onTestFinished, vi } from "vitest";
@@ -64,19 +66,85 @@ const setup = (
const launcher = new ExternalBridgeLauncher(identity, healthMode);
const client = new HopperClient({ launcher, startupTimeoutMs: 2_000 });
onTestFinished(async () => {
await client.close();
await Promise.all(
[...launcher.helpers, ...launcher.fixture.processes].map(stop),
);
await client.close();
await Promise.all(
launcher.sessions.map(({ directory }) =>
rm(directory, { recursive: true, force: true }),
),
);
});
}, 45_000);
return { client, launcher };
};
it.each(["missing", "not-listening"] as const)(
"waits for a matching bridge published during close after a %s socket failure",
async (socketState) => {
const fixture = new HopperFixtureLauncher();
let session: BridgeSession | undefined;
let helper: ChildProcess | undefined;
const client = new HopperClient({
startupTimeoutMs: 2_000,
launcher: {
launch(value) {
session = value;
helper = spawn(process.execPath, ["-e", "process.exit(0)"], {
stdio: "ignore",
});
return Promise.resolve(
ok({
process: helper,
ownsProcessLifetime: false as const,
providerLifetime: "external-application" as const,
shutdownMode: "bridge-request" as const,
}),
);
},
},
});
onTestFinished(async () => {
await Promise.all(
[...(helper === undefined ? [] : [helper]), ...fixture.processes].map(
stop,
),
);
if (session !== undefined)
await rm(session.directory, { recursive: true, force: true });
});
const started = await client.start();
if (started.ok) throw new Error("Expected readiness to time out");
expect(projectAnalysisError(started.error)).toMatchObject({
code: "cleanup_incomplete",
details: { diagnostics: { primary_error: { code: "provider_timeout" } } },
});
if (session === undefined) throw new Error("Fixture did not launch");
if (socketState === "not-listening")
await promisify(execFile)(process.execPath, [
"-e",
'require("node:net").createServer().listen(process.argv[1], () => process.exit(0))',
session.socketPath,
]);
const closing = client.close();
// Publish after close has encountered an unavailable endpoint, within its
// cleanup window rather than before the caller requests cleanup.
await delay(150);
await rm(session.socketPath, { force: true });
await fixture.launch(session);
const closed = await closing;
if (!closed.ok) throw closed.error;
expect(fixture.requests.map(({ method }) => method)).toEqual([
"health",
"shutdown_document",
]);
await vi.waitFor(() => expect(fixture.processes[0]?.exitCode).toBe(0));
await expect(access(session.directory)).rejects.toMatchObject({
code: "ENOENT",
});
},
);
it.each(["wrong-run", "wrong-token"] as const)(
"never requests document shutdown after the first handshake rejects %s",
async (identity) => {
@@ -55,7 +55,7 @@ const start = async (owned: boolean) => {
onTestFinished(async () => {
await client.close();
for (const child of launcher.fixture.processes) child.kill();
});
}, 45_000);
expect((await client.start()).ok).toBe(true);
return { client, launcher };
};
@@ -35,7 +35,7 @@ it("does not equate an owned native helper exit with an external GUI exit", asyn
const client = new HopperClient({ launcher, startupTimeoutMs: 10_000 });
onTestFinished(async () => {
await client.close();
});
}, 45_000);
const result = await client.start();
expect(result).toMatchObject({
ok: false,
@@ -87,7 +87,7 @@ it.each(["exit", "signal", "exit75"] as const)(
const client = new HopperClient({ launcher, startupTimeoutMs: 10_000 });
onTestFinished(async () => {
await client.close();
});
}, 45_000);
const result = await client.start();
expect(result).toMatchObject({
ok: false,
@@ -139,7 +139,7 @@ it.each([0, 1])(
const client = new HopperClient({ launcher, startupTimeoutMs: 10_000 });
onTestFinished(async () => {
await client.close();
});
}, 45_000);
const result = await client.start();
expect(result).toMatchObject({
ok: false,
@@ -177,7 +177,7 @@ it("preserves a successful helper's diagnostics when bridge readiness is never o
const client = new HopperClient({ launcher, startupTimeoutMs: 1500 });
onTestFinished(async () => {
await client.close();
});
}, 45_000);
const result = await client.start();
if (result.ok)
throw new Error("Expected missing bridge readiness to time out");
@@ -128,7 +128,7 @@ const clients: HopperClient[] = [];
afterEach(async () => {
await Promise.all(clients.splice(0).map((client) => client.close()));
});
}, 45_000);
describe("HopperClient startup deadline", () => {
it("applies one deadline to launcher, socket, and health startup phases", async () => {