diff --git a/src/lib/actions/sandbox/destroy-execution.ts b/src/lib/actions/sandbox/destroy-execution.ts index 91bf4cd38a..865fae0773 100644 --- a/src/lib/actions/sandbox/destroy-execution.ts +++ b/src/lib/actions/sandbox/destroy-execution.ts @@ -20,6 +20,7 @@ import type { SandboxEntry } from "../../state/registry"; import type { DestroyRunOpenshell } from "./destroy-gateway"; import { finalizeMcpBridgesAfterSandboxDelete, + McpBridgeError, type McpDestroyPreparation, prepareMcpBridgesForAbsentSandboxDestroy, prepareMcpBridgesForDestroy, @@ -228,7 +229,7 @@ async function finalizeMcpDestroy( try { await finalizeMcpBridgesAfterSandboxDelete(sandboxName, preparation, { force }); } catch (error) { - const detail = error instanceof Error ? error.message : String(error); + const detail = redactDestroyError(error); console.error( ` Sandbox '${sandboxName}' is gone, but authenticated MCP provider cleanup is incomplete: ${detail}`, ); @@ -269,12 +270,22 @@ export async function executeSandboxDestroy({ }; } } - const mcpPreparation = await prepareMcpDestroy( - sandboxName, - sandbox, - sandboxConfirmedAbsent, - force, - ); + let mcpPreparation: McpDestroyPreparation; + try { + mcpPreparation = await prepareMcpDestroy(sandboxName, sandbox, sandboxConfirmedAbsent, force); + } catch (error) { + if (error instanceof McpBridgeError) { + return { + ok: false as const, + deleteOutput: redactDestroyError(error), + exitCode: error.exitCode, + gatewayUnreachable: false, + mcpOwnershipRequiresGateway: false, + shieldsRelockRequiresGateway: false, + }; + } + throw error; + } // Prepared-only/incomplete adds have no external resources and are safely // discarded during preparation. Remaining entries are the durable exact // provider ownership manifest and must survive an unconfirmed delete. @@ -329,7 +340,21 @@ export async function executeSandboxDestroy({ // stale timer state cannot target a same-name replacement. cleanupShieldsArtifacts(sandboxName); if (!forcedLocalCleanup) { - await finalizeMcpDestroy(sandboxName, mcpPreparation, force); + try { + await finalizeMcpDestroy(sandboxName, mcpPreparation, force); + } catch (error) { + if (error instanceof McpBridgeError) { + return { + ok: false as const, + deleteOutput: redactDestroyError(error), + exitCode: error.exitCode, + gatewayUnreachable: false, + mcpOwnershipRequiresGateway: false, + shieldsRelockRequiresGateway: false, + }; + } + throw error; + } } return { ok: true as const, diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 22c68b57c1..08c3e01d3c 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -12,7 +12,9 @@ import { expectFailedHardeningStillDeletes, expectFailedMcpFinalizePreservesRegistry, expectFailedMcpRestorePreservesDestroyFailure, + expectMcpFinalizeBridgeErrorReturnsFailure, expectMcpFinalizeAfterDelete, + expectMcpPrepareBridgeErrorAborts, expectMcpRestoreAfterDeleteFailure, expectShieldsUpRefusalBeforeMutation, expectStrictSandboxPresenceClassification, @@ -371,4 +373,54 @@ describe("destroySandbox flow", () => { expectAbsentSandboxMcpFinalize(harness); }); + + it("exits with code 1 when MCP bridge prepare throws McpBridgeError, gateway down (#8103)", async () => { + const harness = createDestroyHarness({ + mcpServers: ["github"], + prepareMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expectMcpPrepareBridgeErrorAborts(harness); + }); + + it("redacts MCP bridge finalize errors after sandbox deletion (#8103)", async () => { + const secretMarker = "destroy-secret-marker"; + const harness = createDestroyHarness({ + mcpServers: ["github"], + finalizeMcpBridgeError: `Could not inspect OpenShell provider: OPENAI_API_KEY=${secretMarker}`, + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expectMcpFinalizeBridgeErrorReturnsFailure(harness, secretMarker); + }); + + it("retires retained MCP state when destroy retries after finalization failure (#8103)", async () => { + const harness = createDestroyHarness({ + mcpServers: ["github"], + finalizeMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + harness.setSandboxPresent(false); + harness.finalizeMcpBridgesAfterSandboxDeleteSpy.mockResolvedValue(undefined); + + await expect( + harness.destroySandbox("alpha", { yes: true, cleanupGateway: true }), + ).resolves.toBeUndefined(); + + expect(harness.prepareMcpBridgesForAbsentSandboxDestroySpy).toHaveBeenCalledWith("alpha", { + force: false, + }); + expect(harness.finalizeMcpBridgesAfterSandboxDeleteSpy).toHaveBeenCalledTimes(2); + expect(harness.removeSandboxSpy).toHaveBeenCalledWith("alpha"); + expect(harness.updateSessionSpy).toHaveBeenCalledOnce(); + expect(harness.cleanupGatewaySpy).toHaveBeenCalledWith( + "nemoclaw-19080", + harness.runOpenshellSpy, + ); + }); }); diff --git a/test/helpers/destroy-flow-test-assertions.ts b/test/helpers/destroy-flow-test-assertions.ts index 66b12c86aa..28e2b73dc0 100644 --- a/test/helpers/destroy-flow-test-assertions.ts +++ b/test/helpers/destroy-flow-test-assertions.ts @@ -219,6 +219,36 @@ export function expectFailedMcpFinalizePreservesRegistry(harness: DestroyHarness expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); } +export function expectMcpPrepareBridgeErrorAborts(harness: DestroyHarness): void { + expect(harness.prepareMcpBridgesForDestroySpy).toHaveBeenCalled(); + // No delete should happen when MCP prepare itself throws McpBridgeError. + expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( + expect.arrayContaining(["sandbox", "delete"]), + expect.anything(), + ); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); +} + +export function expectMcpFinalizeBridgeErrorReturnsFailure( + harness: DestroyHarness, + secretMarker: string, +): void { + expect(harness.finalizeMcpBridgesAfterSandboxDeleteSpy).toHaveBeenCalled(); + const deleteCall = harness.runOpenshellSpy.mock.calls.findIndex( + (call) => Array.isArray(call[0]) && call[0].join(" ") === "sandbox delete alpha", + ); + expect(deleteCall).toBeGreaterThanOrEqual(0); + expect( + harness.finalizeMcpBridgesAfterSandboxDeleteSpy.mock.invocationCallOrder.at(-1), + ).toBeGreaterThan(harness.runOpenshellSpy.mock.invocationCallOrder[deleteCall]); + const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errorOutput).not.toContain(secretMarker); + expect(errorOutput).toContain(""); + // Registry must not be cleaned up when post-delete MCP finalize throws McpBridgeError. + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); +} + export function expectAbsentSandboxMcpFinalize(harness: DestroyHarness): void { expect(harness.prepareMcpBridgesForDestroySpy).not.toHaveBeenCalled(); expect(harness.prepareMcpBridgesForAbsentSandboxDestroySpy).toHaveBeenCalledWith("alpha", { diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index ec7dd59f83..96cd7dd71d 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -35,6 +35,7 @@ export type DestroyHarness = { restoreMcpBridgesAfterDestroyAbortSpy: MockInstance; runOpenshellSpy: MockInstance; selectGatewaySpy: MockInstance; + setSandboxPresent: (present: boolean) => void; shieldsDownSpy: MockInstance; stopAllSpy: MockInstance; stopNimByNameSpy: MockInstance; @@ -50,12 +51,14 @@ type DestroyHarnessOptions = { deleteStatus?: number; dockerPsOutput?: string; endpointUrl?: string; + finalizeMcpBridgeError?: string; finalizeMcpError?: string; imageTag?: string | null; liveListOutput?: string; mcpAddState?: "prepared"; mcpServers?: string[]; openshellDriver?: string; + prepareMcpBridgeError?: string; promptResponses?: string[]; registeredSandboxCount?: number; restoreMcpError?: string; @@ -110,6 +113,7 @@ export function loadDestroySandboxPresenceClassifier(): DestroySandboxPresenceCl export function createDestroyHarness(options: DestroyHarnessOptions = {}): DestroyHarness { resetDestroyModuleCache(); const events: string[] = []; + let sandboxPresent = options.sandboxPresent !== false; const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); @@ -199,7 +203,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr gatewayPinsAtSandboxList.push(process.env.OPENSHELL_GATEWAY); return { status: 0, - stdout: sandboxListJson(options.sandboxPresent === false ? [] : ["alpha"]), + stdout: sandboxListJson(sandboxPresent ? ["alpha"] : []), stderr: "", }; case "sandbox:delete": @@ -294,9 +298,14 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr destroyAlreadyPending: false, }; const gatewayPinsAtMcpPrepare: Array = []; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const { McpBridgeError } = mcpBridge as any; const prepareMcpBridgesForDestroySpy = vi .spyOn(mcpBridge, "prepareMcpBridgesForDestroy") .mockImplementation(async () => { + if (options.prepareMcpBridgeError !== undefined) { + throw new McpBridgeError(options.prepareMcpBridgeError); + } gatewayPinsAtMcpPrepare.push(process.env.OPENSHELL_GATEWAY); return mcpPreparation; }); @@ -316,11 +325,14 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr }); const finalizeMcpBridgesAfterSandboxDeleteSpy = vi .spyOn(mcpBridge, "finalizeMcpBridgesAfterSandboxDelete") - .mockImplementation(() => - options.finalizeMcpError + .mockImplementation(() => { + if (options.finalizeMcpBridgeError !== undefined) { + return Promise.reject(new McpBridgeError(options.finalizeMcpBridgeError)); + } + return options.finalizeMcpError ? Promise.reject(new Error(options.finalizeMcpError)) - : Promise.resolve(), - ); + : Promise.resolve(); + }); logSpy.mockClear(); @@ -346,6 +358,9 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr restoreMcpBridgesAfterDestroyAbortSpy, runOpenshellSpy, selectGatewaySpy, + setSandboxPresent: (present: boolean) => { + sandboxPresent = present; + }, shieldsDownSpy, stopAllSpy, stopNimByNameSpy,