From 1c123493a21cc579ba7532c40e7ef30df45db3e8 Mon Sep 17 00:00:00 2001 From: You Wish <29498139+Y0UW1SH@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:12:33 -0400 Subject: [PATCH] fix(hopper): allow restart after verified owned-provider shutdown Co-authored-by: YouWish <29498139+Y0UW1SH@users.noreply.github.com> --- .github/workflows/ci.yml | 1 + docs/installation.md | 11 + src/hopper/HopperCleanup.ts | 2 +- .../providers/ghidra/ghidraLauncher.test.ts | 56 +-- .../hopper/hopperClient.cleanup.test.ts | 103 ++--- .../hopper/hopperClient.cleanupRetry.test.ts | 437 ++++++++++++++++++ 6 files changed, 513 insertions(+), 97 deletions(-) create mode 100644 tests/boundary/providers/hopper/hopperClient.cleanupRetry.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 78530bca3..ea474dbf0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -463,6 +463,7 @@ jobs: tests/boundary/providers/ghidra/ghidraTransport.test.ts tests/boundary/providers/ghidra/ghidraTargetSnapshot.test.ts tests/boundary/providers/ghidra/ghidraLauncher.test.ts + tests/boundary/providers/hopper/hopperClient.cleanupRetry.test.ts src/windows/WindowsNativeLoader.test.ts src/windows/WindowsOwnedProcess.test.ts src/ghidra/GhidraLauncherEnvironment.test.ts diff --git a/docs/installation.md b/docs/installation.md index 2b9c78e0d..89fcbba45 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -416,6 +416,17 @@ Closing or switching a target closes its bound Hopper document, shuts down REA's bridge and removes its temporary socket directory while preserving the Hopper application and unrelated documents. A `cleanup_incomplete` result identifies resources whose cleanup could not be verified. +REA retains unresolved cleanup ownership and any confirmed shutdown phases. +After addressing the reported failure, retry `close_binary` on the same +connection. Unconfirmed document or process cleanup retains the target/application +lease and prevents another client from launching against that owned resource. +Once document and process closure are confirmed, the lease can be released even +if temporary-file cleanup fails. Another client can then launch, but the owning +client must finish its retained cleanup before starting again. +An unconfirmed external document can be retried while its authenticated bridge +remains connected. If that bridge has disconnected, another close cannot confirm +the document: inspect and close the reported document in Hopper before ending +the owning REA connection. ### Hopper in CI diff --git a/src/hopper/HopperCleanup.ts b/src/hopper/HopperCleanup.ts index d3c79dc26..346e0b894 100644 --- a/src/hopper/HopperCleanup.ts +++ b/src/hopper/HopperCleanup.ts @@ -269,7 +269,7 @@ const stopProcess = async ( input.logger.info(diagnostic, "Owned Hopper launcher shutdown completed"); if (stopped.status !== "incomplete") { if ( - input.resources.launch?.shutdownMode === "process-cleanup" && + input.resources.launch?.providerLifetime === "launcher-process" && stopped.status !== "not-owned" ) input.resources.shutdownConfirmed = true; diff --git a/tests/boundary/providers/ghidra/ghidraLauncher.test.ts b/tests/boundary/providers/ghidra/ghidraLauncher.test.ts index 87850070d..28d9800f4 100644 --- a/tests/boundary/providers/ghidra/ghidraLauncher.test.ts +++ b/tests/boundary/providers/ghidra/ghidraLauncher.test.ts @@ -235,33 +235,35 @@ describe("Ghidra headless launcher", () => { profileDigest: "a".repeat(64), }); if (!launched.ok) throw launched.error; - const capturePath = join(runtimeRoot, "launch-capture.json"); - await vi.waitFor(() => access(`${capturePath}.ready`), { - timeout: 10_000, - }); - const capture = launchCaptureSchema.parse( - JSON.parse(await readFile(capturePath, "utf8")), - ); - const encodedArguments = JSON.stringify(capture.arguments); - const encodedEnvironment = JSON.stringify(capture.environment); - expect(encodedArguments).not.toContain(token); - expect(encodedEnvironment).not.toContain(token); - expect(capture).toMatchObject({ - ...(process.platform === "win32" ? {} : { descriptor_mode: 0o600 }), - descriptor_has_token: true, - }); - expectIsolatedEnvironment(capture.environment, runtimeRoot, javaHome); - expect(capture.environment.GHIDRA_HEADLESS_JAVA_OPTIONS).toBe(""); - if (process.platform !== "win32") - expect( - (await stat(join(runtimeRoot, "ownership.json"))).mode & 0o777, - ).toBe(0o600); - - const cleaned = await launched.value.cleanup?.(); - expect(cleaned).toMatchObject({ cleaned: true }); - await expect( - access(join(runtimeRoot, "project")), - ).resolves.toBeUndefined(); + try { + const capturePath = join(runtimeRoot, "launch-capture.json"); + await vi.waitFor(() => access(`${capturePath}.ready`), { + timeout: 10_000, + }); + const capture = launchCaptureSchema.parse( + JSON.parse(await readFile(capturePath, "utf8")), + ); + const encodedArguments = JSON.stringify(capture.arguments); + const encodedEnvironment = JSON.stringify(capture.environment); + expect(encodedArguments).not.toContain(token); + expect(encodedEnvironment).not.toContain(token); + expect(capture).toMatchObject({ + ...(process.platform === "win32" ? {} : { descriptor_mode: 0o600 }), + descriptor_has_token: true, + }); + expectIsolatedEnvironment(capture.environment, runtimeRoot, javaHome); + expect(capture.environment.GHIDRA_HEADLESS_JAVA_OPTIONS).toBe(""); + if (process.platform !== "win32") + expect( + (await stat(join(runtimeRoot, "ownership.json"))).mode & 0o777, + ).toBe(0o600); + } finally { + const cleaned = await launched.value.cleanup?.(); + expect(cleaned).toMatchObject({ cleaned: true }); + await expect( + access(join(runtimeRoot, "project")), + ).resolves.toBeUndefined(); + } }, ); }); diff --git a/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts b/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts index f595db837..6c8c88a3b 100644 --- a/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts @@ -54,29 +54,8 @@ class OwnedFixtureLauncher implements BridgeLauncher { } class UnconfirmedFixtureLauncher implements BridgeLauncher { - launch(session: BridgeSession) { - return Promise.resolve( - ok({ - process: spawn( - process.execPath, - [ - fixturePath, - session.socketPath, - session.token, - session.runId, - "unconfirmed", - ], - { stdio: ["ignore", "ignore", "pipe"] }, - ), - ownsProcessLifetime: false, - providerLifetime: "external-application" as const, - shutdownMode: "bridge-request" as const, - }), - ); - } -} + constructor(readonly ownsProcessLifetime = false) {} -class UnconfirmedOwnedFixtureLauncher implements BridgeLauncher { launch(session: BridgeSession) { return Promise.resolve( ok({ @@ -91,8 +70,8 @@ class UnconfirmedOwnedFixtureLauncher implements BridgeLauncher { ], { stdio: ["ignore", "ignore", "pipe"] }, ), - ownsProcessLifetime: true as const, - providerLifetime: "launcher-process" as const, + ownsProcessLifetime: this.ownsProcessLifetime, + providerLifetime: "external-application" as const, shutdownMode: "bridge-request" as const, }), ); @@ -158,55 +137,41 @@ describe("HopperClient cleanup", () => { }); }); - it("reports unconfirmed unowned document cleanup as a typed close failure", async () => { - const client = new HopperClient({ - launcher: new UnconfirmedFixtureLauncher(), - startupTimeoutMs: 1_000, - }); - clients.push(client); - expect((await client.start()).ok).toBe(true); + it.each([ + ["unowned", false], + ["owned helper", true], + ] as const)( + "reports unconfirmed external document cleanup for an %s process", + async (_description, ownsProcessLifetime) => { + const client = new HopperClient({ + launcher: new UnconfirmedFixtureLauncher(ownsProcessLifetime), + startupTimeoutMs: 1_000, + }); + clients.push(client); + expect((await client.start()).ok).toBe(true); - const closed = await client.close(); + const closed = await client.close(); - expect(closed).toMatchObject({ - ok: false, - error: { - _tag: "ProviderAdapterError", - cleanupIncomplete: true, - cleanupResources: expect.arrayContaining(["hopper-document"]), - }, - }); - if (!closed.ok) - expect(projectAnalysisError(closed.error)).toMatchObject({ - code: "cleanup_incomplete", - details: { - provider_id: "hopper", - operation: "close_binary", - cleanup: "incomplete", - resources: expect.arrayContaining(["hopper-document"]), + expect(closed).toMatchObject({ + ok: false, + error: { + _tag: "ProviderAdapterError", + cleanupIncomplete: true, + cleanupResources: expect.arrayContaining(["hopper-document"]), }, }); - }); - - it("does not treat an owned launcher exit as bridge document shutdown", async () => { - const client = new HopperClient({ - launcher: new UnconfirmedOwnedFixtureLauncher(), - startupTimeoutMs: 1_000, - }); - clients.push(client); - expect((await client.start()).ok).toBe(true); - - const closed = await client.close(); - - expect(closed).toMatchObject({ - ok: false, - error: { - _tag: "ProviderAdapterError", - cleanupIncomplete: true, - cleanupResources: expect.arrayContaining(["hopper-document"]), - }, - }); - }); + if (!closed.ok) + expect(projectAnalysisError(closed.error)).toMatchObject({ + code: "cleanup_incomplete", + details: { + provider_id: "hopper", + operation: "close_binary", + cleanup: "incomplete", + resources: expect.arrayContaining(["hopper-document"]), + }, + }); + }, + ); it("kills only its owned Hopper group and emits sanitized shutdown coordinates", async () => { const unrelated = spawn( diff --git a/tests/boundary/providers/hopper/hopperClient.cleanupRetry.test.ts b/tests/boundary/providers/hopper/hopperClient.cleanupRetry.test.ts new file mode 100644 index 000000000..3db6772e3 --- /dev/null +++ b/tests/boundary/providers/hopper/hopperClient.cleanupRetry.test.ts @@ -0,0 +1,437 @@ +import { spawn, type ChildProcess } from "node:child_process"; +import { access, readFile, rm, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { Socket } from "node:net"; + +import { expect, it, onTestFinished } from "vitest"; + +import { projectAnalysisError } from "../../../../src/domain/analysisErrorProjection.js"; +import { HopperStartError } from "../../../../src/domain/hopperErrors.js"; +import { err, ok } from "../../../../src/domain/result.js"; +import type { + BridgeLaunch, + BridgeLauncher, + BridgeSession, +} from "../../../../src/hopper/BridgeLauncher.js"; +import { + HopperClient, + type HopperClientOptions, +} from "../../../../src/hopper/HopperClient.js"; +import { + type HopperOwnedResources, + cleanupHopperSession, +} from "../../../../src/hopper/HopperCleanup.js"; +import { silentLogger } from "../../../../src/logger.js"; +import { PrivateRuntimeRoot } from "../../../../src/process/PrivateRuntimeRoot.js"; +import { ProviderProcessSupervisor } from "../../../../src/process/ProviderProcess.js"; + +class RetryCleanupLauncher implements BridgeLauncher { + readonly sessions: BridgeSession[] = []; + readonly processes: ChildProcess[] = []; + allowCleanup = false; + + constructor(readonly externalDocument = false) { + onTestFinished(async () => { + for (const process of this.processes) await stopFixture(process); + for (const session of this.sessions) + await rm(session.directory, { recursive: true, force: true }); + }); + } + + async launch(session: BridgeSession) { + this.sessions.push(session); + const preparedImagePath = join(session.directory, "image.macho"); + await writeFile(preparedImagePath, "owned prepared backing image"); + const child = spawn( + process.execPath, + ["-e", "setInterval(() => undefined, 1000)"], + { stdio: "ignore" }, + ); + this.processes.push(child); + const launch: BridgeLaunch = this.externalDocument + ? { + process: child, + ownsProcessLifetime: false, + providerLifetime: "external-application", + shutdownMode: "bridge-request", + preparedImagePath, + } + : { + process: child, + ownsProcessLifetime: true, + providerLifetime: "launcher-process", + shutdownMode: "process-cleanup", + preparedImagePath, + cleanup: async () => { + if (!this.allowCleanup) + return { + cleaned: false, + reason: "fixture transient cleanup denial", + }; + await stopFixture(child); + return { cleaned: true, signaled: true }; + }, + }; + return ok(launch); + } +} + +const stopFixture = async (child: ChildProcess): Promise => { + if (child.exitCode !== null || child.signalCode !== null) return; + const exited = new Promise((resolve) => + child.once("exit", () => resolve()), + ); + child.kill("SIGKILL"); + await exited; +}; + +const expectStoppedFixture = ( + child: Pick, +): void => { + expect(child.exitCode !== null || child.signalCode !== null).toBe(true); + if (process.platform !== "win32") expect(child.signalCode).toBe("SIGKILL"); +}; + +const createClient = (options: HopperClientOptions) => { + const client = new HopperClient({ startupTimeoutMs: 100, ...options }); + onTestFinished(async () => { + await client.close(); + }); + return client; +}; + +const failedStartup = async (launcher: RetryCleanupLauncher) => { + const client = createClient({ launcher }); + await expect(client.start()).resolves.toMatchObject({ + ok: false, + error: { + _tag: "ProviderAdapterError", + cleanupIncomplete: true, + cause: { _tag: "HopperTimeoutError" }, + }, + }); + const session = launcher.sessions[0]; + const child = launcher.processes[0]; + if (session === undefined || child === undefined) + throw new Error("Fixture launch was not observed"); + return { client, session, child }; +}; + +it("retains failed startup cleanup until an owned process and its backing image are closed", async () => { + const launcher = new RetryCleanupLauncher(); + const { client, session, child } = await failedStartup(launcher); + const closed = await client.close(); + expect(closed).toMatchObject({ + ok: false, + error: { + cleanupIncomplete: true, + cleanupResources: [ + "hopper-process", + "hopper-document", + session.directory, + ], + }, + }); + if (closed.ok) throw new Error("Expected incomplete fixture cleanup"); + expect(projectAnalysisError(closed.error)).toMatchObject({ + code: "cleanup_incomplete", + details: { + resources: ["hopper-process", "hopper-document", session.directory], + }, + }); + expect(child.exitCode).toBeNull(); + expect(child.signalCode).toBeNull(); + await expect( + readFile(join(session.directory, "image.macho"), "utf8"), + ).resolves.toBe("owned prepared backing image"); + + launcher.allowCleanup = true; + const first = client.close(); + const second = client.close(); + await expect(first).resolves.toEqual({ ok: true, value: null }); + await expect(second).resolves.toEqual({ ok: true, value: null }); + expectStoppedFixture(child); + await expect(access(session.directory)).rejects.toMatchObject({ + code: "ENOENT", + }); + await expect(client.close()).resolves.toEqual({ + ok: true, + value: null, + }); +}); + +it("keeps an unconfirmed external document visible across sequential closes", async () => { + const launcher = new RetryCleanupLauncher(true); + const runtimeRoot = await PrivateRuntimeRoot.create(); + onTestFinished(() => runtimeRoot.close()); + const launched = await launcher.launch({ + directory: runtimeRoot.path, + socketPath: join(runtimeRoot.path, "bridge.sock"), + token: "fixture-token", + runId: "fixture-run", + }); + if (!launched.ok) throw launched.error; + const child = launched.value.process; + const supervisor = new ProviderProcessSupervisor(launched.value); + onTestFinished(() => supervisor.dispose()); + const resources: HopperOwnedResources = { + launch: launched.value, + processSupervisor: supervisor, + runtimeRoot, + shutdownConfirmed: false, + }; + for (let attempt = 0; attempt < 2; attempt += 1) { + await expect( + cleanupHopperSession({ + socket: undefined, + resources, + activeRequest: null, + retainDocument: false, + progress: undefined, + logger: silentLogger, + onDiagnostic: undefined, + request: () => Promise.resolve(ok(null)), + releaseTransport: (socket) => socket?.destroy(), + }), + ).resolves.toMatchObject({ + ok: false, + error: { cleanupResources: ["hopper-document", runtimeRoot.path] }, + }); + } + expect(child.exitCode).toBeNull(); + expect(child.signalCode).toBeNull(); + await expect( + access(join(runtimeRoot.path, "image.macho")), + ).resolves.toBeUndefined(); +}); + +it("blocks a fresh launch while cleanup remains unverified", async () => { + const launcher = new RetryCleanupLauncher(); + const { client, child } = await failedStartup(launcher); + await expect(client.start()).resolves.toMatchObject({ + ok: false, + error: { _tag: "HopperProtocolError" }, + }); + expect(launcher.processes).toEqual([child]); + launcher.allowCleanup = true; + await expect(client.close()).resolves.toEqual({ + ok: true, + value: null, + }); +}); + +it("permits a fresh launch after stopping an owned bridge-request provider without an acknowledgement", async () => { + const fixture = new RetryCleanupLauncher(); + fixture.allowCleanup = true; + const launcher: BridgeLauncher = { + async launch(session) { + const launched = await fixture.launch(session); + if (!launched.ok) return launched; + return ok({ ...launched.value, shutdownMode: "bridge-request" as const }); + }, + }; + const client = createClient({ launcher }); + + for (let attempt = 0; attempt < 2; attempt += 1) { + await expect(client.start()).resolves.toMatchObject({ + ok: false, + error: { _tag: "HopperTimeoutError" }, + }); + await expect(client.close()).resolves.toEqual({ + ok: true, + value: null, + }); + } + expect(fixture.sessions).toHaveLength(2); + for (const session of fixture.sessions) + await expect(access(session.directory)).rejects.toMatchObject({ + code: "ENOENT", + }); + for (const child of fixture.processes) expectStoppedFixture(child); +}); + +it("retains a cross-client lease until owned cleanup succeeds", async () => { + const fixture = new RetryCleanupLauncher(); + let owner: string | undefined; + const launcher: BridgeLauncher = { + async launch(session) { + if (owner !== undefined) + return err(new HopperStartError({ ownerRunId: owner })); + const launched = await fixture.launch(session); + if (!launched.ok) return launched; + owner = session.runId; + return ok({ + ...launched.value, + releaseLease: async () => void (owner = undefined), + }); + }, + }; + const first = createClient({ launcher, runId: "first" }); + const second = createClient({ launcher, runId: "second" }); + await expect(first.start()).resolves.toMatchObject({ ok: false }); + await expect(second.start()).resolves.toMatchObject({ + ok: false, + error: { _tag: "HopperStartError", ownerRunId: "first" }, + }); + expect(fixture.processes).toHaveLength(1); + + fixture.allowCleanup = true; + await expect(first.close()).resolves.toEqual({ ok: true, value: null }); + await expect(second.start()).resolves.toMatchObject({ + ok: false, + error: { _tag: "HopperTimeoutError" }, + }); + expect(fixture.processes).toHaveLength(2); + expect(owner).toBeUndefined(); +}); + +it("retries a failed lease release after document, process, and runtime cleanup are confirmed", async () => { + const fixture = new RetryCleanupLauncher(); + fixture.allowCleanup = true; + let releaseAllowed = false; + const launcher: BridgeLauncher = { + async launch(session) { + const launched = await fixture.launch(session); + if (!launched.ok) return launched; + return ok({ + ...launched.value, + releaseLease: async () => { + if (!releaseAllowed) throw new Error("fixture lease release denial"); + }, + }); + }, + }; + const client = createClient({ launcher }); + await expect(client.start()).resolves.toMatchObject({ ok: false }); + await expect(client.close()).resolves.toMatchObject({ + ok: false, + error: { cleanupResources: ["hopper-lease"] }, + }); + await expect(client.start()).resolves.toMatchObject({ + ok: false, + error: { _tag: "HopperProtocolError" }, + }); + releaseAllowed = true; + await expect(client.close()).resolves.toEqual({ ok: true, value: null }); + expect(fixture.processes).toHaveLength(1); +}); + +it("preserves confirmed document shutdown while retrying owned process cleanup", async () => { + const launcher = new RetryCleanupLauncher(); + const runtimeRoot = await PrivateRuntimeRoot.create(); + onTestFinished(() => runtimeRoot.close()); + const launched = await launcher.launch({ + directory: runtimeRoot.path, + socketPath: join(runtimeRoot.path, "bridge.sock"), + token: "fixture-token", + runId: "fixture-run", + }); + if (!launched.ok) throw launched.error; + const supervisor = new ProviderProcessSupervisor(launched.value); + onTestFinished(() => supervisor.dispose()); + const input = { + resources: { + launch: launched.value, + processSupervisor: supervisor, + runtimeRoot, + shutdownConfirmed: false, + } satisfies HopperOwnedResources, + activeRequest: null, + retainDocument: false, + progress: undefined, + logger: silentLogger, + onDiagnostic: undefined, + request: () => + Promise.resolve( + ok({ + shutdown: true, + analysis_stopped: true, + document_closed: true, + }), + ), + releaseTransport: (socket: Socket | undefined) => socket?.destroy(), + }; + const first = await cleanupHopperSession({ ...input, socket: new Socket() }); + expect(first).toMatchObject({ + ok: false, + error: { cleanupResources: ["hopper-process", runtimeRoot.path] }, + }); + launcher.allowCleanup = true; + const second = await cleanupHopperSession({ + ...input, + socket: undefined, + }); + expect(second).toEqual({ ok: true, value: null }); + expectStoppedFixture(launched.value.process); + await expect(access(runtimeRoot.path)).rejects.toMatchObject({ + code: "ENOENT", + }); +}); + +it.each([ + ["launcher-process", "waiting"], + ["launcher-process", "cancelled"], + ["external-application", "waiting"], + ["external-application", "cancelled"], +] as const)( + "checks document closure for a %s provider with a %s request", + async (providerLifetime, callerState) => { + const launcher = new RetryCleanupLauncher(); + launcher.allowCleanup = true; + const runtimeRoot = await PrivateRuntimeRoot.create(); + onTestFinished(() => runtimeRoot.close()); + const launched = await launcher.launch({ + directory: runtimeRoot.path, + socketPath: join(runtimeRoot.path, "bridge.sock"), + token: "fixture-token", + runId: "fixture-run", + }); + if (!launched.ok) throw launched.error; + const launch: BridgeLaunch = { + ...launched.value, + providerLifetime, + shutdownMode: "bridge-request", + }; + const supervisor = new ProviderProcessSupervisor(launch); + onTestFinished(() => supervisor.dispose()); + const resources: HopperOwnedResources = { + launch, + processSupervisor: supervisor, + runtimeRoot, + shutdownConfirmed: false, + }; + const result = await cleanupHopperSession({ + socket: new Socket(), + resources, + activeRequest: { + requestId: 1, + operation: "partial_then_wait", + elapsedMs: 0, + callerState, + queuedRequests: 0, + }, + retainDocument: false, + progress: undefined, + logger: silentLogger, + onDiagnostic: undefined, + request: () => + Promise.reject(new Error("Active request prevents shutdown")), + releaseTransport: (socket) => socket?.destroy(), + }); + if (providerLifetime === "launcher-process") { + expect(result).toEqual({ ok: true, value: null }); + await expect(access(runtimeRoot.path)).rejects.toMatchObject({ + code: "ENOENT", + }); + } else { + expect(result).toMatchObject({ + ok: false, + error: { cleanupResources: ["hopper-document", runtimeRoot.path] }, + }); + await expect( + access(join(runtimeRoot.path, "image.macho")), + ).resolves.toBeUndefined(); + } + expectStoppedFixture(launch.process); + }, +);