Skip to content

Commit e6e0e1d

Browse files
cjagwaniCharjagscv
authored
fix(onboard,uninstall): replace misleading recovery messages (NVIDIA#3456) (NVIDIA#3520)
> **Draft for visibility.** Issue-autopilot Stages 4-5 of NVIDIA#3456. Will mark ready once batch self-review + CI complete. ## Summary Closes the two remaining output threads in NVIDIA#3456 after the core dead-loop fix already landed on `main` (via NVIDIA#3459, NVIDIA#3434, NVIDIA#3483). Full sub-bug mapping in the [NVIDIA#3456 status comment](NVIDIA#3456 (comment)). - **Sub-bug NVIDIA#3** — `nemoclaw <name> destroy --yes` recovery hint replaced with a registry-aware helper. - **Sub-bug NVIDIA#4** — `Destroyed gateway 'nemoclaw' skipped` self-contradictory wording replaced with `Gateway 'nemoclaw' already removed or unreachable`. ## Acceptance criteria mapping | Sub-bug | Resolution | Evidence | |---|---|---| | #1 dead loop | Already fixed on main (NVIDIA#3459) | out of scope | | NVIDIA#2 firewall diagnostic | Already fixed on main (NVIDIA#3459) | out of scope | | **NVIDIA#3** literal `<name>` placeholder | **This PR** | `src/lib/onboard/gpu-recovery.ts` + `onboard.ts:10387-10405` | | **NVIDIA#4** misleading "skipped" wording | **This PR** | `src/lib/actions/uninstall/run-plan.ts:210-228, 407-414` | | NVIDIA#5 uninstall residuals | Already fixed on main (NVIDIA#3483) | out of scope | ## Behavior matrix `gpuPassthroughRecoveryLines(names)`: | Input | Suggestion | |---|---| | `null` / `[]` | `nemoclaw uninstall && nemoclaw onboard --gpu` | | one sandbox | `nemoclaw <name> destroy --yes --cleanup-gateway && nemoclaw onboard --gpu` | | many sandboxes | each `destroy --yes`, only the last gets `--cleanup-gateway` | ## Test plan ``` npm run typecheck:cli npx vitest run src/lib/onboard/gpu-recovery.test.ts src/lib/actions/uninstall/run-plan.test.ts ``` 22 tests pass (6 new + 16 existing). ## Notes for reviewers - This is the work [NVIDIA#3464 attempted](NVIDIA#3464); that PR was closed without merging after CodeRabbit asked for the `<name>` placeholder to be forbidden in tests via negative assertion. This PR adopts that refinement. - `runOptional` extension is backwards-compatible — existing callers without `onSkip` get the original wording. Closes NVIDIA#3456 once merged. --------- Signed-off-by: Charan Jagwani <charjags100@gmail.com> Co-authored-by: Charan Jagwani <charjags100@gmail.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
1 parent 2bed2c6 commit e6e0e1d

5 files changed

Lines changed: 247 additions & 12 deletions

File tree

src/lib/actions/uninstall/run-plan.test.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -582,4 +582,41 @@ describe("uninstall run plan", () => {
582582
expect(warnings).toContain("Failed to disable /swapfile; skipping swap cleanup.");
583583
expect(logs).not.toContain("Swap file removed");
584584
});
585+
586+
it("#3456 sub-bug #4: gateway destroy no-op uses the 'already removed' wording, not 'Destroyed ... skipped'", () => {
587+
// When `openshell gateway destroy -g nemoclaw` returns non-zero (gateway
588+
// already gone), the previous code printed `Destroyed gateway 'nemoclaw'
589+
// skipped` — self-contradictory. The fix routes this branch to an onSkip
590+
// message that describes the actual state.
591+
const warnings: string[] = [];
592+
const logs: string[] = [];
593+
const result = runUninstallPlan(
594+
{ assumeYes: true, deleteModels: false, keepOpenShell: true },
595+
{
596+
commandExists: (command) => command !== "docker" && command !== "pgrep",
597+
env: { HOME: "/home/test", TMPDIR: "/tmp/test" } as NodeJS.ProcessEnv,
598+
error: (line) => warnings.push(line),
599+
existsSync: () => false,
600+
isTty: false,
601+
log: (line) => logs.push(line),
602+
rmSync: vi.fn(),
603+
run: (command, args) => {
604+
// The openshell gateway destroy command no-ops when the gateway is
605+
// already gone — return non-zero to exercise the onSkip branch.
606+
if (command === "openshell" && args[0] === "gateway" && args[1] === "destroy") {
607+
return notFound();
608+
}
609+
if (args[0] === "-c") return ok("/fake/bin/tool\n");
610+
return ok();
611+
},
612+
runDocker: () => ok(""),
613+
},
614+
);
615+
616+
expect(result.exitCode).toBe(0);
617+
expect(warnings.join("\n")).toContain("Gateway 'nemoclaw' already removed or unreachable");
618+
expect(`${warnings.join("\n")}\n${logs.join("\n")}`).not.toContain(
619+
"Destroyed gateway 'nemoclaw' skipped",
620+
);
621+
});
585622
});

src/lib/actions/uninstall/run-plan.ts

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -207,10 +207,24 @@ function confirm(options: UninstallRunOptions, runtime: UninstallRuntime): boole
207207
return false;
208208
}
209209

210-
function runOptional(runtime: UninstallRuntime, description: string, command: string, args: string[]): void {
210+
function runOptional(
211+
runtime: UninstallRuntime,
212+
description: string,
213+
command: string,
214+
args: string[],
215+
opts: { onSkip?: string } = {},
216+
): void {
211217
const result = runtime.run(command, args, { env: runtime.env, stdio: "ignore" });
212-
if (result.status === 0) runtime.log(description);
213-
else runtime.warn(`${description} skipped`);
218+
if (result.status === 0) {
219+
runtime.log(description);
220+
return;
221+
}
222+
// #3456 sub-bug #4: when the destroy/delete call no-ops (target already
223+
// gone), printing `<description> skipped` was self-contradictory — e.g.
224+
// "Destroyed gateway 'nemoclaw' skipped" suggested the gateway was both
225+
// destroyed AND skipped. Callers that care can pass a `onSkip` message
226+
// describing the actual state (target absent or unreachable).
227+
runtime.warn(opts.onSkip ?? `${description} skipped`);
214228
}
215229

216230
function stopHelperServices(paths: UninstallPaths, runtime: UninstallRuntime): void {
@@ -390,12 +404,14 @@ function removeOpenShellResources(options: UninstallRunOptions, runtime: Uninsta
390404
for (const provider of NEMOCLAW_PROVIDERS) {
391405
runOptional(runtime, `Deleted provider '${provider}'`, "openshell", ["provider", "delete", provider]);
392406
}
393-
runOptional(runtime, `Destroyed gateway '${options.gatewayName || "nemoclaw"}'`, "openshell", [
394-
"gateway",
395-
"destroy",
396-
"-g",
397-
options.gatewayName || "nemoclaw",
398-
]);
407+
const gatewayLabel = options.gatewayName || "nemoclaw";
408+
runOptional(
409+
runtime,
410+
`Destroyed gateway '${gatewayLabel}'`,
411+
"openshell",
412+
["gateway", "destroy", "-g", gatewayLabel],
413+
{ onSkip: `Gateway '${gatewayLabel}' already removed or unreachable` },
414+
);
399415
}
400416

401417
function removeAliases(paths: UninstallPaths, runtime: UninstallRuntime): void {

src/lib/onboard.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,7 @@ import type {
351351
} from "./onboard/types";
352352
import { listChannels } from "./sandbox/channels";
353353
import { streamGatewayStart } from "./onboard/gateway";
354+
import { reportGpuPassthroughRecovery } from "./onboard/gpu-recovery";
354355
import type { StreamSandboxCreateResult } from "./sandbox/create-stream";
355356
import type { SandboxEntry } from "./state/registry";
356357
import type { BackupResult } from "./state/sandbox";
@@ -10316,9 +10317,7 @@ async function onboard(opts: OnboardOptions = {}): Promise<void> {
1031610317
const gpuOutput = String(gpuCheck.stdout || "").trim();
1031710318
const gatewayHasGpu = gpuCheck.status === 0 && gpuOutput !== "null" && gpuOutput !== "[]";
1031810319
if (!gatewayHasGpu) {
10319-
console.error(" Existing gateway was started without GPU passthrough.");
10320-
console.error(" To enable GPU, destroy the existing sandbox and gateway, then re-onboard:");
10321-
console.error(` nemoclaw <name> destroy --yes && nemoclaw onboard --gpu`);
10320+
reportGpuPassthroughRecovery(console.error);
1032210321
process.exit(1);
1032310322
}
1032410323
}
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
/**
5+
* Tests for the GPU-passthrough mismatch recovery hint (#3456 sub-bug #3).
6+
*
7+
* The hint replaces a hard-coded line that printed a literal `<name>`
8+
* placeholder and assumed at least one sandbox was registered — which broke
9+
* the install-loop recovery flow when the registry was empty (the State A /
10+
* State B dead loop the reporter hit on six Linux hosts).
11+
*/
12+
13+
import { describe, expect, it, vi } from "vitest";
14+
import { gpuPassthroughRecoveryLines, reportGpuPassthroughRecovery } from "./gpu-recovery";
15+
16+
describe("gpuPassthroughRecoveryLines", () => {
17+
it("never emits a literal `<name>` placeholder for any input", () => {
18+
for (const names of [null, [], ["alpha"], ["alpha", "beta"], ["alpha", "beta", "gamma"]]) {
19+
const lines = gpuPassthroughRecoveryLines(names);
20+
expect(lines.join("\n")).not.toMatch(/<name>/);
21+
}
22+
});
23+
24+
it("suggests `nemoclaw uninstall` when no sandboxes are registered (null input)", () => {
25+
const lines = gpuPassthroughRecoveryLines(null);
26+
const joined = lines.join("\n");
27+
expect(joined).toContain("Existing gateway was started without GPU passthrough");
28+
expect(joined).toContain("nemoclaw uninstall");
29+
expect(joined).toContain("nemoclaw onboard --gpu");
30+
// Must NOT suggest the destroy form — there is nothing to destroy.
31+
expect(joined).not.toMatch(/nemoclaw [a-z-]+ destroy/);
32+
});
33+
34+
it("suggests `nemoclaw uninstall` when no sandboxes are registered (empty array)", () => {
35+
const lines = gpuPassthroughRecoveryLines([]);
36+
expect(lines.join("\n")).toContain("nemoclaw uninstall");
37+
expect(lines.join("\n")).not.toMatch(/nemoclaw [a-z-]+ destroy/);
38+
});
39+
40+
it("suggests destroy for a single registered sandbox with --cleanup-gateway", () => {
41+
const lines = gpuPassthroughRecoveryLines(["my-assistant"]);
42+
const joined = lines.join("\n");
43+
expect(joined).toContain("nemoclaw my-assistant destroy --yes --cleanup-gateway");
44+
expect(joined).toContain("nemoclaw onboard --gpu");
45+
// The single-sandbox form must not suggest uninstall — destroy is enough.
46+
expect(joined).not.toContain("nemoclaw uninstall");
47+
});
48+
49+
it("lists every registered sandbox and only appends --cleanup-gateway to the last", () => {
50+
const lines = gpuPassthroughRecoveryLines(["alpha", "beta", "gamma"]);
51+
const joined = lines.join("\n");
52+
expect(joined).toContain("nemoclaw alpha destroy --yes");
53+
expect(joined).toContain("nemoclaw beta destroy --yes");
54+
expect(joined).toContain("nemoclaw gamma destroy --yes --cleanup-gateway");
55+
// Only one --cleanup-gateway across all rows.
56+
expect(joined.match(/--cleanup-gateway/g) ?? []).toHaveLength(1);
57+
// alpha/beta lines must NOT have --cleanup-gateway.
58+
const alphaLine = lines.find((line) => line.includes("nemoclaw alpha destroy"));
59+
const betaLine = lines.find((line) => line.includes("nemoclaw beta destroy"));
60+
expect(alphaLine).not.toContain("--cleanup-gateway");
61+
expect(betaLine).not.toContain("--cleanup-gateway");
62+
});
63+
64+
it("filters out empty/whitespace names defensively", () => {
65+
// Belt-and-suspenders: if registry.listSandboxes() ever returns a row with
66+
// an empty name, we shouldn't render `nemoclaw destroy --yes` (the very
67+
// bug shape this fix exists to prevent).
68+
const lines = gpuPassthroughRecoveryLines(["", " ", "real"]);
69+
const joined = lines.join("\n");
70+
expect(joined).toContain("nemoclaw real destroy --yes --cleanup-gateway");
71+
// No double-spaced "nemoclaw destroy" rendering.
72+
expect(joined).not.toMatch(/nemoclaw\s{2,}destroy/);
73+
});
74+
});
75+
76+
describe("reportGpuPassthroughRecovery", () => {
77+
it("emits the empty-registry path when loadNames returns no names", () => {
78+
const emit = vi.fn();
79+
reportGpuPassthroughRecovery(emit, () => []);
80+
const joined = emit.mock.calls.map((c) => c[0]).join("\n");
81+
expect(joined).toContain("nemoclaw uninstall");
82+
expect(joined).not.toMatch(/<name>/);
83+
});
84+
85+
it("emits the multi-sandbox path when loadNames returns several names", () => {
86+
const emit = vi.fn();
87+
reportGpuPassthroughRecovery(emit, () => ["alpha", "beta"]);
88+
const joined = emit.mock.calls.map((c) => c[0]).join("\n");
89+
expect(joined).toContain("nemoclaw alpha destroy --yes");
90+
expect(joined).toContain("nemoclaw beta destroy --yes --cleanup-gateway");
91+
});
92+
});

src/lib/onboard/gpu-recovery.ts

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
/**
5+
* Recovery hint emitted when an onboard run finds the reusable gateway was
6+
* started without GPU passthrough but the current run requested it.
7+
*
8+
* Before #3456 this was a hard-coded `nemoclaw <name> destroy --yes` line
9+
* with a literal `<name>` placeholder — not actionable when the registry was
10+
* empty (the State A / State B dead loop the reporter hit on six Linux
11+
* hosts). This helper renders the right shape based on what's actually
12+
* registered AND owns the registry lookup, so the onboard.ts callsite stays
13+
* a single call (also keeps onboard.ts inside its size budget).
14+
*/
15+
16+
import * as registry from "../state/registry";
17+
18+
/**
19+
* Returns the multi-line recovery hint for the GPU-passthrough mismatch
20+
* branch in onboard. Caller is expected to emit each line on its own line
21+
* via `console.error` / `runtime.log`.
22+
*
23+
* Empty / null input means no sandboxes are registered locally; we suggest
24+
* `nemoclaw uninstall` because there is nothing for `nemoclaw <name>
25+
* destroy` to act on. A single registered sandbox gets one destroy line
26+
* with `--cleanup-gateway` so the gateway also goes away (otherwise destroy
27+
* preserves the shared gateway by default — see v0.0.39 release notes).
28+
* Multiple sandboxes get one destroy line each; only the last carries
29+
* `--cleanup-gateway` so the gateway lives until every sandbox is gone.
30+
*/
31+
export function gpuPassthroughRecoveryLines(names: readonly string[] | null): string[] {
32+
const cleanNames = (names ?? []).map((n) => n.trim()).filter((n) => n.length > 0);
33+
34+
if (cleanNames.length === 0) {
35+
return [
36+
" Existing gateway was started without GPU passthrough.",
37+
" No sandboxes are registered, so there is nothing for `nemoclaw destroy` to act on.",
38+
" Clear the stale gateway state and re-onboard with GPU enabled:",
39+
" nemoclaw uninstall && nemoclaw onboard --gpu",
40+
];
41+
}
42+
43+
if (cleanNames.length === 1) {
44+
return [
45+
" Existing gateway was started without GPU passthrough.",
46+
" To enable GPU, destroy the existing sandbox and gateway, then re-onboard:",
47+
` nemoclaw ${cleanNames[0]} destroy --yes --cleanup-gateway && nemoclaw onboard --gpu`,
48+
];
49+
}
50+
51+
const lastIdx = cleanNames.length - 1;
52+
const destroyLines = cleanNames.map((name, idx) =>
53+
idx === lastIdx
54+
? ` nemoclaw ${name} destroy --yes --cleanup-gateway && nemoclaw onboard --gpu`
55+
: ` nemoclaw ${name} destroy --yes`,
56+
);
57+
58+
return [
59+
" Existing gateway was started without GPU passthrough.",
60+
" To enable GPU, destroy each registered sandbox and the gateway, then re-onboard:",
61+
...destroyLines,
62+
];
63+
}
64+
65+
/**
66+
* Read registered sandbox names with a graceful empty-list fallback when the
67+
* registry can't be opened. Extracted so the onboard callsite stays a single
68+
* line and so unit tests can inject their own list.
69+
*/
70+
export function getRegisteredSandboxNamesForGpuRecovery(): string[] {
71+
try {
72+
return registry
73+
.listSandboxes()
74+
.sandboxes.map((s) => s.name)
75+
.filter(Boolean);
76+
} catch {
77+
return [];
78+
}
79+
}
80+
81+
/**
82+
* Emit the GPU-passthrough mismatch recovery hint to `emit` (typically
83+
* `console.error`). `loadNames` is injectable for tests; the production
84+
* default reads the on-disk sandbox registry.
85+
*/
86+
export function reportGpuPassthroughRecovery(
87+
emit: (line: string) => void,
88+
loadNames: () => string[] = getRegisteredSandboxNamesForGpuRecovery,
89+
): void {
90+
for (const line of gpuPassthroughRecoveryLines(loadNames())) emit(line);
91+
}

0 commit comments

Comments
 (0)