Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/lib/actions/sandbox/gateway-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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})`;
Expand Down
128 changes: 124 additions & 4 deletions src/lib/actions/sandbox/rebuild-destroy-phase.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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: [],
Expand Down Expand Up @@ -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",
Expand All @@ -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({
Expand Down Expand Up @@ -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" }],
[
Expand All @@ -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",
{
Expand Down Expand Up @@ -757,14 +872,16 @@ 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,
stdout: "",
stderr: "tcp connect error: Connection refused",
});
const onDeleted = vi.fn();
const onDeleteStateAmbiguous = vi.fn();
const relockShieldsIfNeeded = vi.fn(() => true);

await expect(
runRebuildDestroyPhase({
Expand All @@ -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(
Expand Down
62 changes: 60 additions & 2 deletions src/lib/actions/sandbox/rebuild-destroy-phase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down
24 changes: 23 additions & 1 deletion src/lib/actions/sandbox/rebuild-shields-finally.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(),
Expand All @@ -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({
Expand Down
Loading