diff --git a/src/domain/analysisErrorPresentation.ts b/src/domain/analysisErrorPresentation.ts index 5b10a5fd8..5fc042d4a 100644 --- a/src/domain/analysisErrorPresentation.ts +++ b/src/domain/analysisErrorPresentation.ts @@ -209,6 +209,8 @@ const RESTART_OWNED_PROVIDER = const hopperProcessRemediation = (error: HopperProcessError): string => { if (error.failureCode !== undefined && error.userMessage !== undefined) return error.userMessage; + if (error.stage === "launch") + return "Review the captured launcher diagnostics and Hopper setup, then retry opening the target."; if (providerRetryAction(error.providerState) === "restart_provider") return RESTART_OWNED_PROVIDER; return "Read binary_session.provider_operation_health for this request before retrying or restarting the owned provider."; @@ -229,7 +231,9 @@ const hopperProcessMessage = (error: HopperProcessError): string => { ? "no exit code was observed" : `exit code ${String(error.exitCode)}`; const recovery = - error.failureCode === undefined ? ` ${RESTART_OWNED_PROVIDER}` : ""; + error.failureCode === undefined && stage !== "launch" + ? ` ${RESTART_OWNED_PROVIDER}` + : ""; return `Hopper exited during ${where} (${exit}).${startup}${recovery}${request}`; } if (error.providerState === "unreachable") diff --git a/src/domain/analysisErrorProjection.ts b/src/domain/analysisErrorProjection.ts index 6b1ca1710..673c17a6d 100644 --- a/src/domain/analysisErrorProjection.ts +++ b/src/domain/analysisErrorProjection.ts @@ -352,7 +352,7 @@ const providerErrorDetails = ( provider_state: error.providerState, retry_action: providerRetryAction( error.providerState, - error.failureCode !== undefined, + error.failureCode !== undefined || error.stage === "launch", ), ...(error.failureCode === undefined ? {} @@ -362,6 +362,9 @@ const providerErrorDetails = ( ...(error.diagnostic === undefined ? {} : { diagnostics: { ...error.diagnostic } }), + ...(error.launcherFailure === undefined + ? {} + : { launcher: error.launcherFailure }), }; } if (error instanceof HopperStartError) @@ -372,6 +375,9 @@ const providerErrorDetails = ( ...(error.ownerRunId === undefined ? {} : { owner_run_id: error.ownerRunId }), + ...(error.launcherFailure === undefined + ? {} + : { launcher: error.launcherFailure }), }; return undefined; }; diff --git a/src/domain/hopperErrors.ts b/src/domain/hopperErrors.ts index b582f6ac2..2246bdec5 100644 --- a/src/domain/hopperErrors.ts +++ b/src/domain/hopperErrors.ts @@ -13,6 +13,16 @@ import { /** Base class for failures produced specifically by the Hopper provider. */ export abstract class HopperError extends AnalysisError {} +/** Observed launcher outcome; a helper exit does not establish GUI lifetime. */ +export type HopperLauncherOutcome = { + readonly exit_code: number | null; + readonly signal: string | null; + readonly stdout: { readonly text: string; readonly bytes: number }; + readonly stderr: { readonly text: string; readonly bytes: number }; + readonly output_closed: boolean; + readonly diagnostic_truncated: boolean; +}; + /** Hopper did not respond within the configured operation deadline. */ export class HopperTimeoutError extends HopperError { readonly _tag = "HopperTimeoutError"; @@ -93,6 +103,7 @@ export class HopperProcessError extends HopperError { readonly requestId?: number, providerState?: HopperProcessProviderState, stage?: ProviderFailureStage, + readonly launcherFailure?: HopperLauncherOutcome, ) { super(`Hopper bridge stopped unexpectedly with code ${String(exitCode)}`); const failure = hopperStartupFailure(exitCode); @@ -115,15 +126,18 @@ export class HopperStartError extends HopperError { readonly _tag = "HopperStartError"; override readonly userMessage: string | undefined; readonly ownerRunId: string | undefined; + readonly launcherFailure: HopperLauncherOutcome | undefined; constructor( options?: ErrorOptions & { readonly userMessage?: string; readonly ownerRunId?: string; + readonly launcherFailure?: HopperLauncherOutcome; }, ) { super("Hopper application bridge could not be started", options); this.userMessage = options?.userMessage; this.ownerRunId = options?.ownerRunId; + this.launcherFailure = options?.launcherFailure; } } diff --git a/src/hopper/BridgeLauncher.ts b/src/hopper/BridgeLauncher.ts index af60844e2..8e4e7559b 100644 --- a/src/hopper/BridgeLauncher.ts +++ b/src/hopper/BridgeLauncher.ts @@ -39,9 +39,14 @@ export interface BridgeSession { /** Process handle returned by a bridge launcher. */ export type BridgeLaunch = - | (ProviderProcessLaunch & { readonly shutdownMode: "bridge-request" }) + | (ProviderProcessLaunch & { + readonly shutdownMode: "bridge-request"; + /** The owned helper may hand off to an external GUI application. */ + readonly providerLifetime: "launcher-process" | "external-application"; + }) | (Extract & { readonly shutdownMode: "process-cleanup"; + readonly providerLifetime: "launcher-process"; readonly cleanup: NonNullable; }); @@ -211,15 +216,23 @@ export class HopperApplicationLauncher implements BridgeLauncher { await cleanupOwnedProcessGroup(started.ownership); return err(new HopperStartError({ cause })); } - return ok({ + const ownedLauncher = { process: started.process, - ownsProcessLifetime: true, + ownsProcessLifetime: true as const, ownership: started.ownership, - shutdownMode: ownsProcessLifetime - ? "process-cleanup" - : "bridge-request", cleanup, - }); + }; + return ownsProcessLifetime + ? ok({ + ...ownedLauncher, + providerLifetime: "launcher-process", + shutdownMode: "process-cleanup", + }) + : ok({ + ...ownedLauncher, + providerLifetime: "external-application", + shutdownMode: "bridge-request", + }); } catch (cause: unknown) { return err(hopperLaunchFailure(cause, signal)); } diff --git a/src/hopper/HopperCleanup.test.ts b/src/hopper/HopperCleanup.test.ts index 9d2a7c633..6e8018f9f 100644 --- a/src/hopper/HopperCleanup.test.ts +++ b/src/hopper/HopperCleanup.test.ts @@ -30,6 +30,7 @@ const processCleanupLaunch = (): BridgeLaunch => ({ kill: () => true, }), ownsProcessLifetime: true, + providerLifetime: "launcher-process", shutdownMode: "process-cleanup", cleanup: () => Promise.resolve({ diff --git a/src/hopper/HopperClient.ts b/src/hopper/HopperClient.ts index 62c82ff87..c8c49958c 100644 --- a/src/hopper/HopperClient.ts +++ b/src/hopper/HopperClient.ts @@ -8,6 +8,7 @@ import { AnalysisCapabilityUnavailableError } from "../domain/analysisErrorCore. import { HopperCancelledError, type HopperError, + type HopperLauncherOutcome, HopperProcessError, HopperProtocolError, HopperRemoteError, @@ -76,7 +77,6 @@ type HopperClientCloseOptions = { readonly retainDocument?: boolean; }; -/** Safe launcher telemetry; stderr content is intentionally never exposed. */ /** * Owns one authenticated NDJSON-over-Unix-socket bridge session. * @@ -122,9 +122,12 @@ export class HopperClient { readonly #onSocketClose = (): void => { if (this.#closing) return; const launch = this.#launch; - if (launch?.ownsProcessLifetime && this.#launcherExitCode !== undefined) + if ( + launch?.providerLifetime === "launcher-process" && + this.#launcherExitCode !== undefined + ) return; - if (launch?.ownsProcessLifetime) { + if (launch?.providerLifetime === "launcher-process") { setImmediate(() => { if (this.#closing || this.#launcherExitCode !== undefined) return; const exitCode = launch.process.exitCode ?? null; @@ -503,17 +506,19 @@ export class HopperClient { if (this.#closing) return err(new HopperProcessError(null)); if ( this.#launcherExitCode !== undefined && - hopperStartupFailure(this.#launcherExitCode) !== undefined - ) - return err( - new HopperProcessError( - this.#launcherExitCode, - this.#launcherFailureDiagnostic, - undefined, - undefined, - "exited", - ), + this.#launch !== undefined && + (this.#launch.providerLifetime === "launcher-process" || + this.#launcherExitCode !== 0) + ) { + const failure = await this.#launcherStartupFailure( + this.#launch.providerLifetime === "launcher-process", + this.#launcherExitCode, + deadline, ); + return deadline.interruption === "cancelled" + ? err(startupInterruption(deadline)) + : err(failure); + } try { await access(socketPath); } catch (cause: unknown) { @@ -538,6 +543,52 @@ export class HopperClient { return err(new HopperTimeoutError(this.#options.startupTimeoutMs)); } + async #launcherStartupFailure( + ownsProviderLifetime: boolean, + exitCode: number | null, + deadline: ProviderStartupDeadline, + ): Promise { + const process = this.#process; + const token = this.#token; + // Descendants may inherit the helper's pipes. Bound drainage independently + // from readiness, and report whether producer output actually closed. + const outputClosed = + (await process?.waitForOutputClose( + Math.min(1_000, deadline.remainingMs()), + )) ?? false; + const snapshot = process?.snapshot(); + const redact = (text: string): string => + token === undefined + ? text + : text.replaceAll(token, "[redacted transport credential]"); + const launcherFailure: HopperLauncherOutcome | undefined = + snapshot === undefined + ? undefined + : { + exit_code: exitCode, + signal: snapshot.signal ?? null, + stdout: { ...snapshot.stdout, text: redact(snapshot.stdout.text) }, + stderr: { ...snapshot.stderr, text: redact(snapshot.stderr.text) }, + output_closed: outputClosed, + diagnostic_truncated: snapshot.diagnosticTruncated === true, + }; + if (ownsProviderLifetime) + return new HopperProcessError( + exitCode, + this.#launcherFailureDiagnostic, + undefined, + undefined, + "exited", + "launch", + launcherFailure, + ); + return new HopperStartError({ + userMessage: + "Hopper's launcher helper failed before bridge readiness. Review details.launcher and complete Hopper setup before retrying.", + ...(launcherFailure === undefined ? {} : { launcherFailure }), + }); + } + async #request( method: string, params: JsonValue, @@ -654,7 +705,7 @@ export class HopperClient { type: "launcher-exit", code: event.code, }); - if (launch.ownsProcessLifetime && !this.#closing) { + if (launch.providerLifetime === "launcher-process" && !this.#closing) { this.#rememberExit(event.code); this.#failAll( new HopperProcessError( @@ -726,7 +777,9 @@ export class HopperClient { const previous = this.#operationFailure; const state = processFailureState(error); const startupFailure = - error.failureCode !== undefined || previous?.startupFailure === true; + error.stage === "launch" || + error.failureCode !== undefined || + previous?.startupFailure === true; const merged = mergeFailedRequests(previous?.requests ?? [], requests); if ( previous !== undefined && diff --git a/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts b/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts index 4ec64f0c2..736b34b8d 100644 --- a/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts @@ -45,6 +45,7 @@ class OwnedFixtureLauncher implements BridgeLauncher { return ok({ process: started.process, ownsProcessLifetime: true as const, + providerLifetime: "launcher-process" as const, ownership: started.ownership, shutdownMode: "process-cleanup" as const, cleanup: () => cleanupOwnedProcessGroup(started.ownership), @@ -68,6 +69,7 @@ class UnconfirmedFixtureLauncher implements BridgeLauncher { { stdio: ["ignore", "ignore", "pipe"] }, ), ownsProcessLifetime: false, + providerLifetime: "external-application" as const, shutdownMode: "bridge-request" as const, }), ); @@ -90,6 +92,7 @@ class UnconfirmedOwnedFixtureLauncher implements BridgeLauncher { { stdio: ["ignore", "ignore", "pipe"] }, ), ownsProcessLifetime: true as const, + providerLifetime: "launcher-process" as const, shutdownMode: "bridge-request" as const, }), ); diff --git a/tests/boundary/providers/hopper/hopperClient.fixture.ts b/tests/boundary/providers/hopper/hopperClient.fixture.ts index 6a4e9f51d..13c820b4e 100644 --- a/tests/boundary/providers/hopper/hopperClient.fixture.ts +++ b/tests/boundary/providers/hopper/hopperClient.fixture.ts @@ -55,6 +55,7 @@ export class HopperFixtureLauncher implements BridgeLauncher { ok({ process: child, ownsProcessLifetime: true as const, + providerLifetime: "launcher-process" as const, shutdownMode: "bridge-request" as const, }), ); diff --git a/tests/boundary/providers/hopper/hopperClient.health.test.ts b/tests/boundary/providers/hopper/hopperClient.health.test.ts index 7265dfba2..8e8bd1930 100644 --- a/tests/boundary/providers/hopper/hopperClient.health.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.health.test.ts @@ -40,7 +40,10 @@ class LifecycleLauncher implements BridgeLauncher { await this.fixture.launch(session); return ok({ process: this.observed, - ownsProcessLifetime: this.owned, + ownsProcessLifetime: true as const, + providerLifetime: this.owned + ? ("launcher-process" as const) + : ("external-application" as const), shutdownMode: "bridge-request" as const, }); } diff --git a/tests/boundary/providers/hopper/hopperClient.helperFailure.test.ts b/tests/boundary/providers/hopper/hopperClient.helperFailure.test.ts new file mode 100644 index 000000000..a99542fc4 --- /dev/null +++ b/tests/boundary/providers/hopper/hopperClient.helperFailure.test.ts @@ -0,0 +1,136 @@ +import { spawn } from "node:child_process"; + +import { expect, it, onTestFinished } from "vitest"; + +import { projectAnalysisError } from "../../../../src/domain/analysisErrorProjection.js"; +import { ok } from "../../../../src/domain/result.js"; +import type { BridgeLauncher } from "../../../../src/hopper/BridgeLauncher.js"; +import { HopperApplicationLauncher } from "../../../../src/hopper/BridgeLauncher.js"; +import { HopperClient } from "../../../../src/hopper/HopperClient.js"; + +it("does not equate an owned native helper exit with an external GUI exit", async () => { + const launcher = new HopperApplicationLauncher({ + launcherPath: process.execPath, + targetPath: "/target", + targetKind: "executable", + loaderArgs: [ + "-e", + 'process.stderr.write("Hopper has not been found.\\n", () => process.exit(3))', + "--", + ], + bridgeScriptPath: "/rea/hopper_bridge.py", + launchMode: "native", + }); + const client = new HopperClient({ launcher, startupTimeoutMs: 10_000 }); + onTestFinished(() => client.close()); + const result = await client.start(); + expect(result).toMatchObject({ + ok: false, + error: { _tag: "HopperStartError" }, + }); + if (result.ok) throw new Error("Expected the native helper to fail"); + expect(projectAnalysisError(result.error)).toMatchObject({ + details: { + provider_state: "unknown", + launcher: { exit_code: 3 }, + }, + }); + expect(client.operationHealth()).toMatchObject({ + state: "unknown", + exitCode: null, + }); +}); + +it.each(["exit", "signal", "exit75"] as const)( + "reports a failed non-owning launcher %s without waiting for bridge timeout or inventing a GUI exit", + async (termination) => { + let token = ""; + const launcher: BridgeLauncher = { + launch(session) { + token = session.token; + const stdout = `API_KEY=local-evidence-kept ${token}\n`; + const stderr = `Hopper has not been found. Spotlight is unavailable. ${token}\n`; + const stop = + termination === "signal" + ? 'process.kill(process.pid, "SIGTERM")' + : `process.exit(${termination === "exit75" ? 75 : 1})`; + return Promise.resolve( + ok({ + process: spawn( + process.execPath, + [ + "-e", + `process.stdout.write(${JSON.stringify(stdout)}, () => process.stderr.write(${JSON.stringify(stderr)}, () => ${stop}));`, + ], + { stdio: ["ignore", "pipe", "pipe"] }, + ), + ownsProcessLifetime: false as const, + providerLifetime: "external-application" as const, + shutdownMode: "bridge-request" as const, + }), + ); + }, + }; + const client = new HopperClient({ launcher, startupTimeoutMs: 10_000 }); + onTestFinished(() => client.close()); + const result = await client.start(); + expect(result).toMatchObject({ + ok: false, + error: { _tag: "HopperStartError" }, + }); + if (result.ok) throw new Error("Expected the launcher to fail"); + const projected = projectAnalysisError(result.error); + expect(projected).toMatchObject({ + code: "provider_unavailable", + details: { + stage: "launch", + launcher: { + exit_code: + termination === "signal" ? null : termination === "exit75" ? 75 : 1, + signal: termination === "signal" ? "SIGTERM" : null, + stdout: { + text: expect.stringContaining("API_KEY=local-evidence-kept"), + }, + stderr: { text: expect.stringContaining("Spotlight is unavailable") }, + }, + }, + }); + expect(JSON.stringify(projected).includes(token)).toBe(false); + expect(client.operationHealth()).toMatchObject({ + state: "unknown", + stage: "launch", + exitCode: null, + }); + }, +); + +it.each([0, 1])( + "reports owned process exit %s before bridge readiness as an observed exit", + async (code) => { + const launcher: BridgeLauncher = { + launch() { + return Promise.resolve( + ok({ + process: spawn(process.execPath, ["-e", `process.exit(${code})`], { + stdio: ["ignore", "pipe", "pipe"], + }), + ownsProcessLifetime: true as const, + providerLifetime: "launcher-process" as const, + shutdownMode: "bridge-request" as const, + }), + ); + }, + }; + const client = new HopperClient({ launcher, startupTimeoutMs: 10_000 }); + onTestFinished(() => client.close()); + const result = await client.start(); + expect(result).toMatchObject({ + ok: false, + error: { _tag: "HopperProcessError", exitCode: code, stage: "launch" }, + }); + expect(client.operationHealth()).toMatchObject({ + state: "exited", + exitCode: code, + }); + }, +); diff --git a/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts b/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts index 40b2871f8..0e7871123 100644 --- a/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts @@ -27,6 +27,7 @@ class SilentLauncher implements BridgeLauncher { }, ), ownsProcessLifetime: true as const, + providerLifetime: "launcher-process" as const, shutdownMode: "bridge-request" as const, }), ); @@ -52,7 +53,10 @@ class CancelThenFixtureLauncher implements BridgeLauncher { } class ExitingLauncher implements BridgeLauncher { - constructor(readonly code: number) {} + constructor( + readonly code: number, + readonly ownsProcessLifetime = true, + ) {} launch() { return Promise.resolve( @@ -60,7 +64,10 @@ class ExitingLauncher implements BridgeLauncher { process: spawn(process.execPath, ["-e", `process.exit(${this.code})`], { stdio: ["ignore", "ignore", "pipe"], }), - ownsProcessLifetime: true as const, + ownsProcessLifetime: this.ownsProcessLifetime, + providerLifetime: this.ownsProcessLifetime + ? ("launcher-process" as const) + : ("external-application" as const), shutdownMode: "bridge-request" as const, }), ); @@ -97,6 +104,7 @@ class DiagnosticExitingLauncher implements BridgeLauncher { { stdio: ["ignore", "ignore", "pipe"] }, ), ownsProcessLifetime: true as const, + providerLifetime: "launcher-process" as const, shutdownMode: "bridge-request" as const, }), ); @@ -281,7 +289,7 @@ describe("HopperClient startup failures", () => { it("allows a short-lived launcher to hand off bridge startup", async () => { const client = new HopperClient({ - launcher: new ExitingLauncher(0), + launcher: new ExitingLauncher(0, false), startupTimeoutMs: 100, }); clients.push(client);