diff --git a/src/lib/actions/uninstall/run-plan.test.ts b/src/lib/actions/uninstall/run-plan.test.ts index 72b3d2fd92f..03091864b16 100644 --- a/src/lib/actions/uninstall/run-plan.test.ts +++ b/src/lib/actions/uninstall/run-plan.test.ts @@ -582,4 +582,41 @@ describe("uninstall run plan", () => { expect(warnings).toContain("Failed to disable /swapfile; skipping swap cleanup."); expect(logs).not.toContain("Swap file removed"); }); + + it("#3456 sub-bug #4: gateway destroy no-op uses the 'already removed' wording, not 'Destroyed ... skipped'", () => { + // When `openshell gateway destroy -g nemoclaw` returns non-zero (gateway + // already gone), the previous code printed `Destroyed gateway 'nemoclaw' + // skipped` — self-contradictory. The fix routes this branch to an onSkip + // message that describes the actual state. + const warnings: string[] = []; + const logs: string[] = []; + const result = runUninstallPlan( + { assumeYes: true, deleteModels: false, keepOpenShell: true }, + { + commandExists: (command) => command !== "docker" && command !== "pgrep", + env: { HOME: "/home/test", TMPDIR: "/tmp/test" } as NodeJS.ProcessEnv, + error: (line) => warnings.push(line), + existsSync: () => false, + isTty: false, + log: (line) => logs.push(line), + rmSync: vi.fn(), + run: (command, args) => { + // The openshell gateway destroy command no-ops when the gateway is + // already gone — return non-zero to exercise the onSkip branch. + if (command === "openshell" && args[0] === "gateway" && args[1] === "destroy") { + return notFound(); + } + if (args[0] === "-c") return ok("/fake/bin/tool\n"); + return ok(); + }, + runDocker: () => ok(""), + }, + ); + + expect(result.exitCode).toBe(0); + expect(warnings.join("\n")).toContain("Gateway 'nemoclaw' already removed or unreachable"); + expect(`${warnings.join("\n")}\n${logs.join("\n")}`).not.toContain( + "Destroyed gateway 'nemoclaw' skipped", + ); + }); }); diff --git a/src/lib/actions/uninstall/run-plan.ts b/src/lib/actions/uninstall/run-plan.ts index b3d71ea1fc3..c99289bbebe 100644 --- a/src/lib/actions/uninstall/run-plan.ts +++ b/src/lib/actions/uninstall/run-plan.ts @@ -207,10 +207,24 @@ function confirm(options: UninstallRunOptions, runtime: UninstallRuntime): boole return false; } -function runOptional(runtime: UninstallRuntime, description: string, command: string, args: string[]): void { +function runOptional( + runtime: UninstallRuntime, + description: string, + command: string, + args: string[], + opts: { onSkip?: string } = {}, +): void { const result = runtime.run(command, args, { env: runtime.env, stdio: "ignore" }); - if (result.status === 0) runtime.log(description); - else runtime.warn(`${description} skipped`); + if (result.status === 0) { + runtime.log(description); + return; + } + // #3456 sub-bug #4: when the destroy/delete call no-ops (target already + // gone), printing ` skipped` was self-contradictory — e.g. + // "Destroyed gateway 'nemoclaw' skipped" suggested the gateway was both + // destroyed AND skipped. Callers that care can pass a `onSkip` message + // describing the actual state (target absent or unreachable). + runtime.warn(opts.onSkip ?? `${description} skipped`); } function stopHelperServices(paths: UninstallPaths, runtime: UninstallRuntime): void { @@ -390,12 +404,14 @@ function removeOpenShellResources(options: UninstallRunOptions, runtime: Uninsta for (const provider of NEMOCLAW_PROVIDERS) { runOptional(runtime, `Deleted provider '${provider}'`, "openshell", ["provider", "delete", provider]); } - runOptional(runtime, `Destroyed gateway '${options.gatewayName || "nemoclaw"}'`, "openshell", [ - "gateway", - "destroy", - "-g", - options.gatewayName || "nemoclaw", - ]); + const gatewayLabel = options.gatewayName || "nemoclaw"; + runOptional( + runtime, + `Destroyed gateway '${gatewayLabel}'`, + "openshell", + ["gateway", "destroy", "-g", gatewayLabel], + { onSkip: `Gateway '${gatewayLabel}' already removed or unreachable` }, + ); } function removeAliases(paths: UninstallPaths, runtime: UninstallRuntime): void { diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 104359340fb..f33a1cdf3ba 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -343,6 +343,7 @@ import type { } from "./onboard/types"; import { listChannels } from "./sandbox/channels"; import { streamGatewayStart } from "./onboard/gateway"; +import { reportGpuPassthroughRecovery } from "./onboard/gpu-recovery"; import type { StreamSandboxCreateResult } from "./sandbox/create-stream"; import type { SandboxEntry } from "./state/registry"; import type { BackupResult } from "./state/sandbox"; @@ -10343,9 +10344,7 @@ async function onboard(opts: OnboardOptions = {}): Promise { const gpuOutput = String(gpuCheck.stdout || "").trim(); const gatewayHasGpu = gpuCheck.status === 0 && gpuOutput !== "null" && gpuOutput !== "[]"; if (!gatewayHasGpu) { - console.error(" Existing gateway was started without GPU passthrough."); - console.error(" To enable GPU, destroy the existing sandbox and gateway, then re-onboard:"); - console.error(` nemoclaw destroy --yes && nemoclaw onboard --gpu`); + reportGpuPassthroughRecovery(console.error); process.exit(1); } } diff --git a/src/lib/onboard/gpu-recovery.test.ts b/src/lib/onboard/gpu-recovery.test.ts new file mode 100644 index 00000000000..172501e63cc --- /dev/null +++ b/src/lib/onboard/gpu-recovery.test.ts @@ -0,0 +1,92 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Tests for the GPU-passthrough mismatch recovery hint (#3456 sub-bug #3). + * + * The hint replaces a hard-coded line that printed a literal `` + * placeholder and assumed at least one sandbox was registered — which broke + * the install-loop recovery flow when the registry was empty (the State A / + * State B dead loop the reporter hit on six Linux hosts). + */ + +import { describe, expect, it, vi } from "vitest"; +import { gpuPassthroughRecoveryLines, reportGpuPassthroughRecovery } from "./gpu-recovery"; + +describe("gpuPassthroughRecoveryLines", () => { + it("never emits a literal `` placeholder for any input", () => { + for (const names of [null, [], ["alpha"], ["alpha", "beta"], ["alpha", "beta", "gamma"]]) { + const lines = gpuPassthroughRecoveryLines(names); + expect(lines.join("\n")).not.toMatch(//); + } + }); + + it("suggests `nemoclaw uninstall` when no sandboxes are registered (null input)", () => { + const lines = gpuPassthroughRecoveryLines(null); + const joined = lines.join("\n"); + expect(joined).toContain("Existing gateway was started without GPU passthrough"); + expect(joined).toContain("nemoclaw uninstall"); + expect(joined).toContain("nemoclaw onboard --gpu"); + // Must NOT suggest the destroy form — there is nothing to destroy. + expect(joined).not.toMatch(/nemoclaw [a-z-]+ destroy/); + }); + + it("suggests `nemoclaw uninstall` when no sandboxes are registered (empty array)", () => { + const lines = gpuPassthroughRecoveryLines([]); + expect(lines.join("\n")).toContain("nemoclaw uninstall"); + expect(lines.join("\n")).not.toMatch(/nemoclaw [a-z-]+ destroy/); + }); + + it("suggests destroy for a single registered sandbox with --cleanup-gateway", () => { + const lines = gpuPassthroughRecoveryLines(["my-assistant"]); + const joined = lines.join("\n"); + expect(joined).toContain("nemoclaw my-assistant destroy --yes --cleanup-gateway"); + expect(joined).toContain("nemoclaw onboard --gpu"); + // The single-sandbox form must not suggest uninstall — destroy is enough. + expect(joined).not.toContain("nemoclaw uninstall"); + }); + + it("lists every registered sandbox and only appends --cleanup-gateway to the last", () => { + const lines = gpuPassthroughRecoveryLines(["alpha", "beta", "gamma"]); + const joined = lines.join("\n"); + expect(joined).toContain("nemoclaw alpha destroy --yes"); + expect(joined).toContain("nemoclaw beta destroy --yes"); + expect(joined).toContain("nemoclaw gamma destroy --yes --cleanup-gateway"); + // Only one --cleanup-gateway across all rows. + expect(joined.match(/--cleanup-gateway/g) ?? []).toHaveLength(1); + // alpha/beta lines must NOT have --cleanup-gateway. + const alphaLine = lines.find((line) => line.includes("nemoclaw alpha destroy")); + const betaLine = lines.find((line) => line.includes("nemoclaw beta destroy")); + expect(alphaLine).not.toContain("--cleanup-gateway"); + expect(betaLine).not.toContain("--cleanup-gateway"); + }); + + it("filters out empty/whitespace names defensively", () => { + // Belt-and-suspenders: if registry.listSandboxes() ever returns a row with + // an empty name, we shouldn't render `nemoclaw destroy --yes` (the very + // bug shape this fix exists to prevent). + const lines = gpuPassthroughRecoveryLines(["", " ", "real"]); + const joined = lines.join("\n"); + expect(joined).toContain("nemoclaw real destroy --yes --cleanup-gateway"); + // No double-spaced "nemoclaw destroy" rendering. + expect(joined).not.toMatch(/nemoclaw\s{2,}destroy/); + }); +}); + +describe("reportGpuPassthroughRecovery", () => { + it("emits the empty-registry path when loadNames returns no names", () => { + const emit = vi.fn(); + reportGpuPassthroughRecovery(emit, () => []); + const joined = emit.mock.calls.map((c) => c[0]).join("\n"); + expect(joined).toContain("nemoclaw uninstall"); + expect(joined).not.toMatch(//); + }); + + it("emits the multi-sandbox path when loadNames returns several names", () => { + const emit = vi.fn(); + reportGpuPassthroughRecovery(emit, () => ["alpha", "beta"]); + const joined = emit.mock.calls.map((c) => c[0]).join("\n"); + expect(joined).toContain("nemoclaw alpha destroy --yes"); + expect(joined).toContain("nemoclaw beta destroy --yes --cleanup-gateway"); + }); +}); diff --git a/src/lib/onboard/gpu-recovery.ts b/src/lib/onboard/gpu-recovery.ts new file mode 100644 index 00000000000..51c770adf4b --- /dev/null +++ b/src/lib/onboard/gpu-recovery.ts @@ -0,0 +1,91 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Recovery hint emitted when an onboard run finds the reusable gateway was + * started without GPU passthrough but the current run requested it. + * + * Before #3456 this was a hard-coded `nemoclaw destroy --yes` line + * with a literal `` placeholder — not actionable when the registry was + * empty (the State A / State B dead loop the reporter hit on six Linux + * hosts). This helper renders the right shape based on what's actually + * registered AND owns the registry lookup, so the onboard.ts callsite stays + * a single call (also keeps onboard.ts inside its size budget). + */ + +import * as registry from "../state/registry"; + +/** + * Returns the multi-line recovery hint for the GPU-passthrough mismatch + * branch in onboard. Caller is expected to emit each line on its own line + * via `console.error` / `runtime.log`. + * + * Empty / null input means no sandboxes are registered locally; we suggest + * `nemoclaw uninstall` because there is nothing for `nemoclaw + * destroy` to act on. A single registered sandbox gets one destroy line + * with `--cleanup-gateway` so the gateway also goes away (otherwise destroy + * preserves the shared gateway by default — see v0.0.39 release notes). + * Multiple sandboxes get one destroy line each; only the last carries + * `--cleanup-gateway` so the gateway lives until every sandbox is gone. + */ +export function gpuPassthroughRecoveryLines(names: readonly string[] | null): string[] { + const cleanNames = (names ?? []).map((n) => n.trim()).filter((n) => n.length > 0); + + if (cleanNames.length === 0) { + return [ + " Existing gateway was started without GPU passthrough.", + " No sandboxes are registered, so there is nothing for `nemoclaw destroy` to act on.", + " Clear the stale gateway state and re-onboard with GPU enabled:", + " nemoclaw uninstall && nemoclaw onboard --gpu", + ]; + } + + if (cleanNames.length === 1) { + return [ + " Existing gateway was started without GPU passthrough.", + " To enable GPU, destroy the existing sandbox and gateway, then re-onboard:", + ` nemoclaw ${cleanNames[0]} destroy --yes --cleanup-gateway && nemoclaw onboard --gpu`, + ]; + } + + const lastIdx = cleanNames.length - 1; + const destroyLines = cleanNames.map((name, idx) => + idx === lastIdx + ? ` nemoclaw ${name} destroy --yes --cleanup-gateway && nemoclaw onboard --gpu` + : ` nemoclaw ${name} destroy --yes`, + ); + + return [ + " Existing gateway was started without GPU passthrough.", + " To enable GPU, destroy each registered sandbox and the gateway, then re-onboard:", + ...destroyLines, + ]; +} + +/** + * Read registered sandbox names with a graceful empty-list fallback when the + * registry can't be opened. Extracted so the onboard callsite stays a single + * line and so unit tests can inject their own list. + */ +export function getRegisteredSandboxNamesForGpuRecovery(): string[] { + try { + return registry + .listSandboxes() + .sandboxes.map((s) => s.name) + .filter(Boolean); + } catch { + return []; + } +} + +/** + * Emit the GPU-passthrough mismatch recovery hint to `emit` (typically + * `console.error`). `loadNames` is injectable for tests; the production + * default reads the on-disk sandbox registry. + */ +export function reportGpuPassthroughRecovery( + emit: (line: string) => void, + loadNames: () => string[] = getRegisteredSandboxNamesForGpuRecovery, +): void { + for (const line of gpuPassthroughRecoveryLines(loadNames())) emit(line); +}