fix(hopper): allow restart after verified owned-provider shutdown

Co-authored-by: YouWish <29498139+Y0UW1SH@users.noreply.github.com>

.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

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
 

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;

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();
+      }
     },
   );
 });

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(

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<void> => {
+  if (child.exitCode !== null || child.signalCode !== null) return;
+  const exited = new Promise<void>((resolve) =>
+    child.once("exit", () => resolve()),
+  );
+  child.kill("SIGKILL");
+  await exited;
+};
+
+const expectStoppedFixture = (
+  child: Pick<ChildProcess, "exitCode" | "signalCode">,
+): 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);
+  },
+);