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
32 changes: 30 additions & 2 deletions src/lib/actions/sandbox/destroy-flow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type DestroyHarness = {
removeSandboxSpy: MockInstance;
runOpenshellSpy: MockInstance;
selectGatewaySpy: MockInstance;
stopAllSpy: MockInstance;
stopNimByNameSpy: MockInstance;
unloadOllamaModelsSpy: MockInstance;
shieldsUpSpy: MockInstance;
Expand All @@ -29,6 +30,7 @@ type DestroyHarnessOptions = {
activeTimer?: boolean;
deleteStatus?: number;
deleteOutput?: string;
registeredSandboxCount?: number;
shieldsUpError?: Error;
};

Expand Down Expand Up @@ -69,7 +71,11 @@ function createDestroyHarness(options: DestroyHarnessOptions = {}): DestroyHarne
sessions: [{ pid: 1 }],
});
vi.spyOn(registry, "getSandbox").mockReturnValue(sandboxEntry);
vi.spyOn(registry, "listSandboxes").mockReturnValue({ sandboxes: [] });
vi.spyOn(registry, "listSandboxes").mockReturnValue({
sandboxes: Array.from({ length: options.registeredSandboxCount ?? 0 }, (_, i) => ({
name: `sb-${i}`,
})),
});
const removeSandboxSpy = vi.spyOn(registry, "removeSandbox").mockReturnValue(true);
vi.spyOn(onboardSession, "loadSession").mockReturnValue({ sandboxName: "alpha" });
vi.spyOn(onboardSession, "updateSession").mockImplementation((mutator: unknown) => {
Expand Down Expand Up @@ -117,7 +123,7 @@ function createDestroyHarness(options: DestroyHarnessOptions = {}): DestroyHarne
const unloadOllamaModelsSpy = vi
.spyOn(ollamaProxy, "unloadOllamaModels")
.mockImplementation(() => undefined);
vi.spyOn(tunnelServices, "stopAll").mockImplementation(() => undefined);
const stopAllSpy = vi.spyOn(tunnelServices, "stopAll").mockImplementation(() => undefined);
vi.spyOn(timerControl, "readTimerMarker").mockReturnValue(
options.activeTimer
? {
Expand Down Expand Up @@ -156,6 +162,7 @@ function createDestroyHarness(options: DestroyHarnessOptions = {}): DestroyHarne
removeSandboxSpy,
runOpenshellSpy,
selectGatewaySpy,
stopAllSpy,
stopNimByNameSpy,
unloadOllamaModelsSpy,
shieldsUpSpy,
Expand Down Expand Up @@ -220,6 +227,27 @@ describe("destroySandbox flow", () => {
expect(exitSpy).toHaveBeenCalledWith(7);
});

it("does not stop shared host services when --force cleans up the last sandbox with the gateway down (#6046)", async () => {
// Gateway-unreachable delete failure + --force triggers forcedLocalCleanup:
// the local record is removed but the gateway-side delete was never
// confirmed, so the sandbox may still exist. Even as the only registered
// sandbox, that must not tear down shared host services (CodeRabbit #6050).
const harness = createDestroyHarness({
deleteStatus: 1,
deleteOutput: "error trying to connect: connection refused",
registeredSandboxCount: 1,
});

await expect(harness.destroySandbox("alpha", { force: true })).resolves.toBeUndefined();

// Local cleanup still proceeds...
expect(harness.removeSandboxSpy).toHaveBeenCalledWith("alpha");
// ...but shared host services are preserved on the unconfirmed delete.
expect(harness.stopAllSpy).not.toHaveBeenCalled();
expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled();
expect(exitSpy).not.toHaveBeenCalled();
});

it("wipes while mutable, hardens an active timer window, then deletes and clears it", async () => {
const harness = createDestroyHarness({ activeTimer: true });

Expand Down
62 changes: 54 additions & 8 deletions src/lib/actions/sandbox/destroy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -409,29 +409,48 @@ export async function destroySandbox(
ignoreError: true,
stdio: ["ignore", "pipe", "pipe"],
});
const { output: deleteOutput, alreadyGone: lockedAlreadyGone } =
getSandboxDeleteOutcome(lockedDeleteResult);

if (lockedDeleteResult.status !== 0 && !lockedAlreadyGone) {
const {
output: deleteOutput,
alreadyGone: lockedAlreadyGone,
gatewayUnreachable,
} = getSandboxDeleteOutcome(lockedDeleteResult);

// When the OpenShell gateway is down, every gateway call (including the
// final delete) gets a connection-refused/transport error. That used to
// abort destroy with no bypass, leaving no supported way to remove the
// sandbox record (#6046). Under --force, fall back to local cleanup;
// otherwise keep failing but point at the recovery paths.
const forcedLocalCleanup =
lockedDeleteResult.status !== 0 &&
!lockedAlreadyGone &&
gatewayUnreachable &&
normalized.force === true;

if (lockedDeleteResult.status !== 0 && !lockedAlreadyGone && !forcedLocalCleanup) {
// Any active timer was cleared only after shieldsUp verified the live
// sandbox was hardened. Preserve that locked state on delete failure;
// do not remove its local shields record as if deletion had succeeded.
return {
ok: false as const,
deleteOutput,
gatewayUnreachable,
exitCode: lockedDeleteResult.status || 1,
};
}

// The live sandbox is now gone while this name remains serialized.
// Revoke the timer and local shields state before releasing the lock so
// neither can target a subsequently created sandbox with the same name.
// Either the live sandbox is confirmed gone, or --force is discarding the
// local record for an unreachable gateway. In both cases the sandbox is
// no longer tracked locally, so revoke the timer and local shields state
// before releasing the lock so neither can target a subsequently created
// sandbox with the same name.
cleanupShieldsDestroyArtifacts(sandboxName);
return {
ok: true as const,
detachOutcome: lockedDetachOutcome,
deleteResult: lockedDeleteResult,
alreadyGone: lockedAlreadyGone,
forcedLocalCleanup,
deleteOutput,
};
},
);
Expand All @@ -440,10 +459,37 @@ export async function destroySandbox(
console.error(` ${destructiveResult.deleteOutput}`);
}
console.error(` Failed to destroy sandbox '${sandboxName}'.`);
if (destructiveResult.gatewayUnreachable) {
console.error(
` The OpenShell gateway is unreachable. Start it (run '${CLI_NAME} ${sandboxName} status'),`,
);
console.error(
` or re-run with --force to remove the local sandbox record without the gateway.`,
);
}
process.exit(destructiveResult.exitCode);
}
const { detachOutcome, deleteResult, alreadyGone } = destructiveResult;
const { detachOutcome, deleteResult, alreadyGone, forcedLocalCleanup, deleteOutput } =
destructiveResult;

if (forcedLocalCleanup) {
if (deleteOutput) {
console.error(` ${deleteOutput}`);
}
console.warn(
` ${YW}⚠${R} OpenShell gateway unreachable; removing the local record for '${sandboxName}' (--force).`,
);
console.warn(
` ${YW}⚠${R} If the gateway comes back, the sandbox may still exist — re-run destroy or remove it via openshell.`,
);
}

// Forced local cleanup removes the registry entry/local artifacts but cannot
// confirm the gateway-side delete, so it must not trigger shared host-service
// or gateway teardown: the sandbox may still exist on the (unreachable)
// gateway. Gate that teardown on the *confirmed* delete state only — never on
// forcedLocalCleanup — so a forced cleanup of the last registered sandbox does
// not shut down services for a sandbox we never confirmed deleted (#6046).
const deleteSucceededOrAlreadyGone = deleteResult.status === 0 || alreadyGone;
const shouldStopHostServices = shouldStopHostServicesAfterDestroy({
deleteSucceededOrAlreadyGone,
Expand Down
2 changes: 1 addition & 1 deletion src/lib/actions/sandbox/snapshot.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ vi.mock("../../credentials/store", () => ({
}));

vi.mock("../../domain/sandbox/destroy", () => ({
getSandboxDeleteOutcome: vi.fn(() => ({ alreadyGone: false })),
getSandboxDeleteOutcome: vi.fn(() => ({ alreadyGone: false, gatewayUnreachable: false })),
}));

vi.mock("../../inference/nim", () => ({
Expand Down
21 changes: 21 additions & 0 deletions src/lib/domain/sandbox/destroy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { describe, expect, it } from "vitest";

import {
getSandboxDeleteOutcome,
isGatewayUnreachableDeleteOutput,
isMissingSandboxDeleteOutput,
shouldCleanupGatewayAfterDestroy,
shouldStopHostServicesAfterDestroy,
Expand All @@ -17,20 +18,40 @@ describe("sandbox destroy helpers", () => {
expect(isMissingSandboxDeleteOutput("permission denied")).toBe(false);
});

it("detects gateway transport errors vs real failures (#6046)", () => {
expect(isGatewayUnreachableDeleteOutput("Connection refused (os error 61)")).toBe(true);
expect(isGatewayUnreachableDeleteOutput("tcp connect error: Connection refused")).toBe(true);
expect(isGatewayUnreachableDeleteOutput("error trying to connect to 127.0.0.1:8080")).toBe(
true,
);
expect(isGatewayUnreachableDeleteOutput("permission denied")).toBe(false);
expect(isGatewayUnreachableDeleteOutput("sandbox alpha not found")).toBe(false);
});

it("classifies delete outcomes", () => {
expect(
getSandboxDeleteOutcome({ status: 1, stderr: "Error: sandbox alpha not found" }),
).toEqual({
output: "Error: sandbox alpha not found",
alreadyGone: true,
gatewayUnreachable: false,
});
expect(getSandboxDeleteOutcome({ status: 1, stdout: "boom" })).toEqual({
output: "boom",
alreadyGone: false,
gatewayUnreachable: false,
});
expect(getSandboxDeleteOutcome({ status: 0, stdout: "deleted" })).toEqual({
output: "deleted",
alreadyGone: false,
gatewayUnreachable: false,
});
expect(
getSandboxDeleteOutcome({ status: 1, stderr: "tcp connect error: Connection refused" }),
).toEqual({
output: "tcp connect error: Connection refused",
alreadyGone: false,
gatewayUnreachable: true,
});
});

Expand Down
19 changes: 18 additions & 1 deletion src/lib/domain/sandbox/destroy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,31 @@ export function isMissingSandboxDeleteOutput(output = ""): boolean {
);
}

/**
* True when a `sandbox delete` failure is a gateway transport error (the
* OpenShell gateway at 127.0.0.1:8080 is not listening) rather than a real
* delete rejection. When the gateway process is down every gateway call gets a
* connection-refused/transport error, which used to make `destroy` fatal with
* no bypass (#6046).
*/
export function isGatewayUnreachableDeleteOutput(output = ""): boolean {
return /connection refused|os error (?:61|111)|tcp connect error|error trying to connect|transport error|failed to connect to|connect(?:ion)? timed out|deadline has elapsed|connection reset/i.test(
stripAnsi(output),
);
}

export function getSandboxDeleteOutcome(deleteResult: SpawnLikeResult): {
output: string;
alreadyGone: boolean;
gatewayUnreachable: boolean;
} {
const output = `${deleteResult.stdout || ""}${deleteResult.stderr || ""}`.trim();
const failed = deleteResult.status !== 0;
const alreadyGone = failed && isMissingSandboxDeleteOutput(output);
return {
output,
alreadyGone: deleteResult.status !== 0 && isMissingSandboxDeleteOutput(output),
alreadyGone,
gatewayUnreachable: failed && !alreadyGone && isGatewayUnreachableDeleteOutput(output),
};
}

Expand Down
101 changes: 101 additions & 0 deletions test/cli/destroy-gateway-unreachable.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

/**
* #6046: when the OpenShell gateway is down, `sandbox delete` fails with a
* connection-refused transport error. `destroy` used to abort fatally with no
* bypass. `--force` must now fall back to local cleanup; without it, destroy
* still fails but points at the recovery paths.
*/

import fs from "node:fs";
import os from "node:os";
import path from "node:path";

import { describe, expect, it } from "vitest";

import { runWithEnv, testTimeoutOptions } from "./helpers";

// Fake openshell whose `sandbox delete` fails as if the gateway is down; every
// other call succeeds so the destroy flow reaches the delete.
const GATEWAY_DOWN_OPENSHELL = [
"#!/bin/sh",
'if [ "$1" = "sandbox" ] && [ "$2" = "list" ]; then exit 0; fi',
'if [ "$1" = "sandbox" ] && [ "$2" = "delete" ]; then',
' printf "tcp connect error: Connection refused (os error 61)\\n" >&2',
" exit 1",
"fi",
"exit 0",
].join("\n");

function fixture(): { home: string; registryPath: string; localBin: string } {
const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-destroy-gwdown-"));
const localBin = path.join(home, "bin");
const registryDir = path.join(home, ".nemoclaw");
fs.mkdirSync(localBin, { recursive: true });
fs.mkdirSync(registryDir, { recursive: true });
fs.writeFileSync(
path.join(registryDir, "sandboxes.json"),
JSON.stringify({
sandboxes: {
alpha: {
name: "alpha",
model: "test-model",
provider: "nvidia-prod",
gpuEnabled: false,
policies: [],
},
},
defaultSandbox: "alpha",
}),
{ mode: 0o600 },
);
fs.writeFileSync(path.join(localBin, "openshell"), GATEWAY_DOWN_OPENSHELL, { mode: 0o755 });
fs.writeFileSync(path.join(localBin, "docker"), ["#!/bin/sh", "exit 0"].join("\n"), {
mode: 0o755,
});
return { home, registryPath: path.join(registryDir, "sandboxes.json"), localBin };
}

function registryHasAlpha(registryPath: string): boolean {
const reg = JSON.parse(fs.readFileSync(registryPath, "utf8"));
return Boolean(reg.sandboxes?.alpha);
}

describe("CLI destroy when the gateway is unreachable (#6046)", () => {
it("removes the local sandbox record with --force", testTimeoutOptions(30_000), () => {
const { home, registryPath, localBin } = fixture();
try {
const r = runWithEnv("alpha destroy --force", {
HOME: home,
PATH: `${localBin}:${process.env.PATH || ""}`,
});

// --force succeeds (exit 0); the gateway-unreachable warning goes to
// stderr (not captured on success), so assert the behavioral outcome:
// the local record is removed and destroy reports success on stdout.
expect(r.code, r.out).toBe(0);
expect(r.out).toContain("Sandbox 'alpha' destroyed");
expect(registryHasAlpha(registryPath)).toBe(false);
} finally {
fs.rmSync(home, { recursive: true, force: true });
}
});

it("fails with a recovery hint when --force is absent", testTimeoutOptions(30_000), () => {
const { home, registryPath, localBin } = fixture();
try {
const r = runWithEnv("alpha destroy -y", {
HOME: home,
PATH: `${localBin}:${process.env.PATH || ""}`,
});

expect(r.code).not.toBe(0);
expect(r.out).toContain("The OpenShell gateway is unreachable");
expect(r.out).toContain("--force");
expect(registryHasAlpha(registryPath)).toBe(true);
} finally {
fs.rmSync(home, { recursive: true, force: true });
}
});
});
1 change: 1 addition & 0 deletions test/image-cleanup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ describe("image cleanup: sandbox destroy removes Docker image (#2086)", () => {
).toEqual({
output: "Error: sandbox alpha not found",
alreadyGone: true,
gatewayUnreachable: false,
});
});

Expand Down
Loading