diff --git a/src/lib/actions/sandbox/gateway-state.ts b/src/lib/actions/sandbox/gateway-state.ts index 55c5f5c4b09..37e35a75d41 100644 --- a/src/lib/actions/sandbox/gateway-state.ts +++ b/src/lib/actions/sandbox/gateway-state.ts @@ -114,6 +114,12 @@ export function isExplicitMissingSandboxGatewayOutput( const exactNoSpec = /^(?:error:\s*)?status:\s*Internal,\s*message:\s*["']sandbox has no spec["'](?:,\s*details:\s*\[\])?(?:,\s*metadata:\s*MetadataMap\s*\{\s*\})?$/i; if (exactNoSpec.test(clean)) return true; + // OpenShell can omit the requested name from an owner-scoped lookup. + // Require both exact structured fields so gateway/provider absence and + // transport diagnostics remain ambiguous. + const exactStructuredNotFound = + /^(?:error:\s*)?(?:×\s*)?code:\s*["']Some requested entity was not found["']\s*,\s*message:\s*["']sandbox not found["']$/i; + if (exactStructuredNotFound.test(clean)) return true; const escapedName = sandboxName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const namedSandbox = `(?:['\"]${escapedName}['\"]|${escapedName})`; diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts index cd623ba688f..993d3a6a92a 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts @@ -6,7 +6,15 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const mocks = vi.hoisted(() => ({ captureOpenshell: vi.fn(), getSandbox: vi.fn( - (_name: string): { name: string; agent: string; nimContainer?: string | null } | null => null, + ( + _name: string, + ): { + name: string; + agent: string; + nimContainer?: string | null; + gatewayName?: string | null; + gatewayPort?: number | null; + } | null => null, ), listSandboxes: vi.fn(() => ({ sandboxes: [] })), prepareMcpForRebuild: vi.fn(), @@ -69,7 +77,10 @@ describe("rebuild destroy phase", () => { vi.clearAllMocks(); vi.spyOn(console, "log").mockImplementation(() => undefined); vi.spyOn(console, "error").mockImplementation(() => undefined); - mocks.getSandbox.mockReturnValue(null); + mocks.getSandbox.mockReturnValue({ + name: "alpha", + agent: "openclaw", + }); mocks.listSandboxes.mockReturnValue({ sandboxes: [] }); mocks.prepareMcpForRebuild.mockResolvedValue({ entries: [], @@ -200,6 +211,12 @@ describe("rebuild destroy phase", () => { it("pins deletion to the recorded gateway when ambient selection changes (#7062)", async () => { vi.stubEnv("OPENSHELL_GATEWAY", "nemoclaw-29080"); + mocks.getSandbox.mockReturnValue({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw-19080", + gatewayPort: 19080, + }); await runRebuildDestroyPhase({ sandboxName: "alpha", @@ -226,6 +243,71 @@ describe("rebuild destroy phase", () => { ); }); + it.each([ + [ + "sandbox name", + { + name: "beta", + agent: "openclaw", + gatewayName: "nemoclaw", + gatewayPort: 8080, + }, + ], + [ + "gateway binding", + { + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw-29080", + gatewayPort: 29080, + }, + ], + ])("refuses deletion when the registry %s changes before MCP preparation (#7062)", async (_label, currentEntry) => { + mocks.getSandbox.mockReturnValue(currentEntry); + mocks.prepareMcpForRebuild.mockResolvedValue({ + entries: [{ server: "github" }], + detachedProviderEntries: [{ server: "github" }], + scrubbedAdapterEntries: [], + }); + const relockShieldsIfNeeded = vi.fn(() => true); + + await expect( + runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + gatewayPort: 8080, + }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail: vi.fn((message: string): never => { + throw new Error(message); + }), + relockShieldsIfNeeded, + onDeleted: vi.fn(), + }), + ).rejects.toThrow("Sandbox delete target changed during rebuild preparation."); + + expect(mocks.getSandbox).toHaveBeenCalledTimes(2); + expect(mocks.prepareMcpForRebuild.mock.invocationCallOrder[0]).toBeLessThan( + mocks.getSandbox.mock.invocationCallOrder[1] ?? Number.POSITIVE_INFINITY, + ); + expect(mocks.runOpenshell).not.toHaveBeenCalled(); + expect(mocks.reattachMcpAfterDeleteFailure).toHaveBeenCalledWith( + "alpha", + [{ server: "github" }], + [], + ); + expect(mocks.removeSandboxRegistryEntryWithReceipt).not.toHaveBeenCalled(); + expect(mocks.stopNimContainer).not.toHaveBeenCalled(); + expect(mocks.stopNimContainerByName).not.toHaveBeenCalled(); + expect(relockShieldsIfNeeded).toHaveBeenCalledWith(true); + }); + it("refuses sandbox deletion when read-only MCP state drifts at the delete edge (#7062)", async () => { const revalidateBeforeDelete = vi.fn().mockRejectedValue(new Error("live policy drifted")); mocks.prepareMcpForRebuild.mockResolvedValue({ @@ -652,6 +734,23 @@ describe("rebuild destroy phase", () => { expect(currentMs).toBeLessThanOrEqual(15_000); }); + it("recognizes the exact structured OpenShell sandbox-absence response (#7062)", () => { + const log = vi.fn(); + + expect( + waitForRebuildDeleteAbsence("alpha", "nemoclaw", log, { + captureSandboxGet: vi.fn(() => ({ + status: 1, + stdout: "", + stderr: + "Error: × code: 'Some requested entity was not found', message: \"sandbox not found\"", + })), + }), + ).toBe(true); + + expect(log).toHaveBeenCalledWith("Delete convergence probe 1: status=1, state=absent"); + }); + it.each([ ["another sandbox", { status: 1, stderr: "sandbox beta not found" }], [ @@ -662,6 +761,22 @@ describe("rebuild destroy phase", () => { "a missing provider", { status: 1, stderr: 'status: NotFound, message: "provider alpha-mcp-github not found"' }, ], + [ + "a structured missing gateway", + { + status: 1, + stderr: + "Error: × code: 'Some requested entity was not found', message: \"gateway not found\"", + }, + ], + [ + "a structured missing provider", + { + status: 1, + stderr: + "Error: × code: 'Some requested entity was not found', message: \"provider not found\"", + }, + ], [ "mixed gateway and sandbox diagnostics", { @@ -757,7 +872,7 @@ describe("rebuild destroy phase", () => { expect(mocks.removeSandboxRegistryEntryWithReceipt).toHaveBeenCalledWith("alpha"); }); - it("preserves backup and registry state when transport failures prevent deletion confirmation", async () => { + it("marks accepted deletion as ambiguous when transport failures prevent confirmation", async () => { mocks.runOpenshell.mockReturnValue({ status: 0, stdout: "deleted", stderr: "" }); mocks.captureOpenshell.mockReturnValue({ status: 1, @@ -765,6 +880,8 @@ describe("rebuild destroy phase", () => { stderr: "tcp connect error: Connection refused", }); const onDeleted = vi.fn(); + const onDeleteStateAmbiguous = vi.fn(); + const relockShieldsIfNeeded = vi.fn(() => true); await expect( runRebuildDestroyPhase({ @@ -776,12 +893,15 @@ describe("rebuild destroy phase", () => { bail: vi.fn((message: string): never => { throw new Error(message); }), - relockShieldsIfNeeded: vi.fn(() => true), + relockShieldsIfNeeded, onDeleted, + onDeleteStateAmbiguous, }), ).rejects.toThrow("Sandbox deletion could not be confirmed."); expect(onDeleted).not.toHaveBeenCalled(); + expect(onDeleteStateAmbiguous).toHaveBeenCalledOnce(); + expect(relockShieldsIfNeeded).not.toHaveBeenCalled(); expect(mocks.runOpenshell).toHaveBeenCalledTimes(1); expect(mocks.captureOpenshell).toHaveBeenCalledTimes(3); expect(mocks.waitUntil).toHaveBeenCalledWith( diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.ts index f96d74e5819..9226de56d55 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.ts @@ -7,9 +7,10 @@ import { G, R } from "../../cli/terminal-style"; import { waitUntil } from "../../core/wait"; import { getSandboxDeleteOutcome } from "../../domain/sandbox/destroy"; import * as nim from "../../inference/nim"; -import { resolveSandboxGatewayName } from "../../onboard/gateway-binding"; +import { resolveGatewayName, resolveSandboxGatewayName } from "../../onboard/gateway-binding"; import { redactFull } from "../../security/redact"; import { parseSandboxPhase } from "../../state/gateway"; +import { registryEntryGatewayPort } from "../../state/gateway-registry"; import * as registry from "../../state/registry"; import { removeSandboxRegistryEntryWithReceipt } from "./destroy"; import { isExplicitMissingSandboxGatewayOutput } from "./gateway-state"; @@ -51,6 +52,12 @@ type PostDeleteReconciliation = | { state: "intact"; phase: "Ready" | "Running"; status: 0 } | { state: "ambiguous"; phase: string | null; status: number | null }; +interface RebuildDeleteTarget { + gatewayName: string; + gatewayPort: number; + sandboxName: string; +} + interface RebuildDeleteAbsenceDeps { captureSandboxGet?: ( sandboxName: string, @@ -71,6 +78,38 @@ const REBUILD_DELETE_ABSENCE_MAX_ATTEMPTS = 20; const REBUILD_DELETE_ABSENCE_INITIAL_INTERVAL_MS = 250; const REBUILD_DELETE_ABSENCE_MAX_INTERVAL_MS = 1_000; +function resolveRebuildDeleteTarget( + sandboxName: string, + sandboxEntry: RebuildSandboxEntry, +): RebuildDeleteTarget { + if (sandboxEntry.name !== sandboxName) { + throw new Error("Rebuild sandbox entry does not match the requested delete target."); + } + const gatewayPort = registryEntryGatewayPort({ + name: sandboxEntry.name, + gatewayName: sandboxEntry.gatewayName, + gatewayPort: sandboxEntry.gatewayPort, + }); + return { + gatewayName: resolveGatewayName(gatewayPort), + gatewayPort, + sandboxName, + }; +} + +function rebuildDeleteTargetMatchesRegistry(expected: RebuildDeleteTarget): boolean { + const currentEntry = registry.getSandbox(expected.sandboxName); + if (!currentEntry) return false; + try { + const current = resolveRebuildDeleteTarget(expected.sandboxName, currentEntry); + return ( + current.gatewayName === expected.gatewayName && current.gatewayPort === expected.gatewayPort + ); + } catch { + return false; + } +} + /** Wait for explicit absence from the same `sandbox get` boundary used by inner onboard. */ export function waitForRebuildDeleteAbsence( sandboxName: string, @@ -192,7 +231,8 @@ export async function runRebuildDestroyPhase( validateAfterMcpPreparation, onDeleted, } = input; - const gatewayName = resolveSandboxGatewayName(input.sandboxEntry); + const deleteTarget = resolveRebuildDeleteTarget(sandboxName, input.sandboxEntry); + const { gatewayName } = deleteTarget; if (blockRebuildOnPendingBaselineTransition(input.sandboxEntry, sandboxName, bail)) return null; @@ -298,6 +338,23 @@ export async function runRebuildDestroyPhase( } } + // MCP preparation can await external systems. Re-read the registry at the + // synchronous delete edge so those checks and deletion use one target. + if (!rebuildDeleteTargetMatchesRegistry(deleteTarget)) { + const mcpRecoveryFailure = await reattachMcpAfterDeleteFailure( + sandboxName, + rebuildDetachedMcpProviderEntries, + rebuildScrubbedMcpAdapterEntries, + ); + relockShieldsIfNeeded(true); + bail( + mcpRecoveryFailure + ? `Sandbox delete target changed during rebuild preparation; MCP provider recovery also failed: ${mcpRecoveryFailure}` + : "Sandbox delete target changed during rebuild preparation.", + ); + return null; + } + log(`Running: openshell sandbox delete -g ${gatewayName} ${sandboxName}`); const deleteResult = runOpenshell(["sandbox", "delete", "-g", gatewayName, sandboxName], { ignoreError: true, @@ -364,6 +421,7 @@ export async function runRebuildDestroyPhase( if (backupManifest) { console.error(" State backup is preserved at: " + backupManifest.backupPath); } + input.onDeleteStateAmbiguous?.(); bail("Sandbox deletion could not be confirmed."); return null; } diff --git a/src/lib/actions/sandbox/rebuild-shields-finally.test.ts b/src/lib/actions/sandbox/rebuild-shields-finally.test.ts index 85957000082..000da2e2ce2 100644 --- a/src/lib/actions/sandbox/rebuild-shields-finally.test.ts +++ b/src/lib/actions/sandbox/rebuild-shields-finally.test.ts @@ -32,6 +32,7 @@ describe("rebuild shields relock guard", () => { const rebuildWindow = { relocked: false, wasLocked: true }; const cleanupDcodePreflight = vi.fn(); const releaseOnboardLock = vi.fn(); + const revalidateDcodeBeforeDelete = vi.fn(async () => true); const relockShields = vi.fn(() => { rebuildWindow.relocked = true; return true; @@ -46,7 +47,10 @@ describe("rebuild shields relock guard", () => { recreateOptions: { observabilityEnabled: false }, liveState: { staleRecovery: false, staleRegistrySnapshot: null }, recoveryManifest: null, - dcodePreflight: { cleanup: cleanupDcodePreflight }, + dcodePreflight: { + cleanup: cleanupDcodePreflight, + revalidateBeforeDelete: revalidateDcodeBeforeDelete, + }, preparedImage: null, releaseOnboardLock, log: vi.fn(), @@ -72,6 +76,24 @@ describe("rebuild shields relock guard", () => { expect(rebuildWindow.relocked).toBe(true); }); + it("does not relock shields when sandbox deletion remains ambiguous (#7062)", async () => { + phaseMocks.runBackup.mockReturnValue({ backupManifest: null }); + phaseMocks.runDestroy.mockImplementation( + ({ onDeleteStateAmbiguous }: { onDeleteStateAmbiguous?: () => void }) => { + onDeleteStateAmbiguous?.(); + throw new Error("Sandbox deletion could not be confirmed."); + }, + ); + + await expect(rebuildSandbox("alpha", ["--yes"], { throwOnError: true })).rejects.toThrow( + "Sandbox deletion could not be confirmed.", + ); + + expect(phaseMocks.runDestroy).toHaveBeenCalledOnce(); + expect(relockShields).not.toHaveBeenCalled(); + expect(rebuildWindow.relocked).toBe(false); + }); + it("blocks a pending baseline transition before shields, backup, or destroy phases begin (#7194)", async () => { const bail = vi.fn(); phaseMocks.runPreflight.mockResolvedValue({