Skip to content

Commit 58b17d7

Browse files
authored
fix(uninstall): settle portable cleanup in one invocation (#9513)
<!-- markdownlint-disable MD041 --> ## Summary Make receipt-owned Portable uninstall settle in one supported invocation when a transient rootless Podman observation returns status 125 or OpenShell deletion has already converged to strict gateway-scoped absence. Failed retirement now keeps the installed NemoClaw CLI and source available for a version-pinned retry. ## Related Issue Fixes #9499 Parent Epic: #9200 ## Changes - Retry the exact read-only, receipt-label Podman discovery once only after status 125, with socket-authority reassertion before re-observation. - Never retry a container or OpenShell delete mutation. - After one nonzero OpenShell delete, accept only strict structured absence from the connected recorded gateway; ambiguous, unreachable, live, or identity-drift states remain fatal. - Complete Portable retirement before removing the CLI/source; preserve both when retirement cannot be proven. - Add production-shaped regressions for 125-to-success, repeated 125, non-125, spawn error, socket-authority drift, strict OpenShell absence settlement, and retry-path preservation. - Document the bounded settlement and fail-closed retry contract. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — independent prepublication docs/security review of commit `deb4209a8a0c8f62d16e2c748e10c3826055bf10` found no remaining findings and passed all nine security categories. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; no DGX Station preparation changes. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed. The growth guard used its supported `NEMOCLAW_GROWTH_BASE_REF=upstream/main` override after refreshing upstream because `origin/main` was stale. - [x] Targeted behavior tests pass — Portable run-plan 31/31; all five #9499 runtime cleanup cases pass. - [x] Applicable broad gates passed — CLI build/typecheck, repository checks, source-shape, growth guard 32/32, focused Oxlint, diff checks, and docs build. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings — PASS with Fern 0 errors and 2 pre-existing unprinted warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ### Local baseline disclosure Two unchanged #9189 child-process lock tests fail on this controller. A clean current-main worktree reproduces both with identical assertions. The #9499 cases, the other focused tests, repository checks, and normal hooks pass. ### Live validation A protected L40S Portable install-to-chat-to-uninstall run will validate the published revision before this PR is marked ready. No Brev result is claimed yet. --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved portable uninstall reliability when sandbox deletion fails or sandboxes are already absent. * Added safe retries for temporary sandbox discovery failures. * Prevented cleanup from proceeding when sandbox absence cannot be verified. * Preserved the installed CLI and retry information when cleanup is incomplete. * Added an alternative uninstall path when service disabling fails. * Protected retained and externally managed services during cleanup. * Ensured uninstall failures are reported cleanly without leaving cleanup steps unfinished. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
1 parent b2d1ce5 commit 58b17d7

6 files changed

Lines changed: 255 additions & 112 deletions

File tree

docs/manage-sandboxes/uninstall-nemoclaw.mdx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,11 +175,13 @@ NemoClaw uses the pinned local Podman socket to remove only the exact receipt-ow
175175
Before every exact receipt-name OpenShell sandbox deletion, NemoClaw runs `openshell status -g <gateway>` to prove that the gateway name recorded by the matching registry row is connected.
176176
It then runs `openshell sandbox delete -g <gateway> <sandbox>` for that exact gateway and sandbox name.
177177
After deletion, bounded `openshell sandbox get -g <gateway> <sandbox>` probes must verify that the exact sandbox is absent.
178+
If the single delete command returns a nonzero status, NemoClaw still performs those read-only probes.
179+
Cleanup continues only when the connected recorded gateway returns the strict structured absence result for that exact sandbox; it never repeats the delete mutation.
178180
It never relies on or switches the globally selected gateway, and it never falls back to implicit deletion.
179181
A same-name sandbox on another gateway remains untouched.
180182
It never uses `openshell sandbox delete --all` during portable OpenShell cleanup.
181183
If explicit deletion reports an exact receipt sandbox as absent, cleanup continues only because the gateway-scoped status probe proved that exact gateway is connected and reachable.
182-
A missing gateway, missing provider, transport failure, verification timeout, generic or ambiguous `NotFound` result, or absence that the status probe cannot prove is fatal.
184+
A missing gateway, missing provider, transport failure, verification timeout, generic or ambiguous `NotFound` result during verification, or absence that the status probe cannot prove is fatal.
183185
Uninstall exits nonzero and preserves the portable registry row and container, selectors, receipts, `~/.config/nemoclaw/portable/containers.conf`, gateway state, shared OpenShell provider and gateway registrations, and unrelated OpenShell-only sandboxes.
184186
Portable cleanup retires only the exact receipt-owned portable resources.
185187
It preserves model stores and images, shared OpenShell provider and gateway registrations, the gateway service and process, shared OpenShell binaries and configuration, and unrelated OpenShell-registered sandboxes.
@@ -203,6 +205,7 @@ NemoClaw retains the record until a later completed onboarding durably supersede
203205
It contains a random local cleanup ID, receipt basenames derived from SHA-256 hashes of sandbox names, and length-framed, transaction-scoped, dictionary-testable pseudonymous fingerprints, but no raw sandbox or gateway names, absolute paths, environment values, configuration bytes, or secrets.
204206
A repeat invocation remains on portable cleanup and performs no Docker probe or mutation, implicit OpenShell cleanup, or model cleanup.
205207
If NemoClaw cannot prove the required portable authority or complete a cleanup step, uninstall exits nonzero.
208+
Before portable retirement completes, it also keeps the installed NemoClaw CLI and source so the same version-pinned command remains available for retry.
206209
It preserves any existing durable retirement record and any remaining receipts, `~/.config/nemoclaw/portable/containers.conf`, gateway state, and other retry evidence, even with `--destroy-user-data`.
207210
Resolve the reported authority or cleanup error, keep the preserved state intact, and rerun the same full uninstall command.
208211
This cleanup does not change Docker uninstall, externally supervised gateways, `--keep-openshell`, scoped sibling-gateway cleanup, or non-portable profiles.

src/lib/actions/uninstall/portable-runtime-cleanup.test.ts

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,25 @@ function completeCleanup(input: PortableRuntimeCleanupInput, deps: PortableRunti
329329
return runPortableRuntimeCleanupTransaction(input, () => true, deps);
330330
}
331331

332+
type PodmanObservation = ReturnType<NonNullable<PortableRuntimeCleanupDeps["podman"]>>;
333+
type PodmanObservationResponse = PodmanObservation | (() => PodmanObservation);
334+
335+
function interceptSandboxDiscoveries(
336+
test: ReturnType<typeof fixture>,
337+
responses: readonly PodmanObservationResponse[],
338+
): () => number {
339+
const originalPodman = test.deps.podman!;
340+
let observations = 0;
341+
test.deps.podman = vi.fn((args, env) => {
342+
const command = args[0] === "--url" ? args.slice(2) : args;
343+
const observesSandbox =
344+
command[0] === "ps" && command.includes("label=openshell.ai/sandbox-name=alpha");
345+
const response = observesSandbox ? responses[observations++] : undefined;
346+
return (typeof response === "function" ? response() : response) ?? originalPodman(args, env);
347+
});
348+
return () => observations;
349+
}
350+
332351
function addRetirementConfig(test: ReturnType<typeof fixture>): string {
333352
const target = path.join(test.homeDir, ".config/nemoclaw/portable/containers.conf");
334353
fs.mkdirSync(path.dirname(target), { mode: 0o700, recursive: true });
@@ -933,6 +952,81 @@ describe("portable runtime uninstall cleanup", () => {
933952
expect(fs.existsSync(`${test.registryFile}.lock`)).toBe(false);
934953
});
935954

955+
it("retries one failed read-only sandbox discovery under reasserted authority (#9499)", () => {
956+
const test = fixture();
957+
const observations = interceptSandboxDiscoveries(test, [
958+
{ status: 125, stdout: "", stderr: "transient local Podman observation failure" },
959+
]);
960+
961+
expect(completeCleanup(test.input, test.deps)).toEqual({
962+
registryRemoved: true,
963+
sandboxContainersRemoved: 1,
964+
selectorsRemoved: ["CONTAINERS_CONF", "NETAVARK_FW"],
965+
});
966+
expect(observations()).toBe(4);
967+
expect(test.containers.has(ALPHA_ID)).toBe(false);
968+
});
969+
970+
it.each([
971+
["non-125 status", { status: 126, stdout: "", stderr: "permission denied" }],
972+
["spawn error", { status: null, stdout: "", stderr: "", error: new Error("spawn EACCES") }],
973+
] as const)("does not retry a %s sandbox discovery failure (#9499)", (_label, failure) => {
974+
const test = fixture();
975+
const observations = interceptSandboxDiscoveries(test, [failure]);
976+
977+
expect(() => completeCleanup(test.input, test.deps)).toThrow(
978+
"Finding portable sandbox 'alpha' failed",
979+
);
980+
expect(observations()).toBe(1);
981+
expect(test.containers.has(ALPHA_ID)).toBe(true);
982+
expect(test.containers.has(REGISTRY_ID)).toBe(true);
983+
expect(test.podmanCalls.some((args) => args[0] === "rm")).toBe(false);
984+
});
985+
986+
it("stops a status-125 retry when socket authority changes before re-observation (#9499)", () => {
987+
const test = fixture();
988+
let authorityChanged = false;
989+
const authorityError = "Podman socket authority changed after it was qualified";
990+
const rejectChangedAuthority = () => {
991+
throw new Error(authorityError);
992+
};
993+
const assertSocketAuthority = vi.fn(() =>
994+
authorityChanged ? rejectChangedAuthority() : undefined,
995+
);
996+
test.deps.runtimeReadiness = { ...test.deps.runtimeReadiness, assertSocketAuthority };
997+
const observations = interceptSandboxDiscoveries(test, [
998+
() => {
999+
authorityChanged = true;
1000+
return { status: 125, stdout: "", stderr: "transient local Podman observation failure" };
1001+
},
1002+
]);
1003+
1004+
expect(() => completeCleanup(test.input, test.deps)).toThrow(authorityError);
1005+
expect(observations()).toBe(1);
1006+
expect(assertSocketAuthority).toHaveBeenCalled();
1007+
expect(test.containers.has(ALPHA_ID)).toBe(true);
1008+
expect(test.containers.has(REGISTRY_ID)).toBe(true);
1009+
expect(test.podmanCalls.some((args) => args[0] === "rm")).toBe(false);
1010+
});
1011+
1012+
it("stops without mutation after two failed read-only sandbox discoveries (#9499)", () => {
1013+
const test = fixture();
1014+
const failure = {
1015+
status: 125,
1016+
stdout: "",
1017+
stderr: "persistent local Podman observation failure",
1018+
};
1019+
const observations = interceptSandboxDiscoveries(test, [failure, failure]);
1020+
1021+
expect(() => completeCleanup(test.input, test.deps)).toThrow(
1022+
"Finding portable sandbox 'alpha' failed",
1023+
);
1024+
expect(observations()).toBe(2);
1025+
expect(test.containers.has(ALPHA_ID)).toBe(true);
1026+
expect(test.containers.has(REGISTRY_ID)).toBe(true);
1027+
expect(test.podmanCalls.some((args) => args[0] === "rm")).toBe(false);
1028+
});
1029+
9361030
it("preserves changed current-user manager selector values (#9189)", () => {
9371031
const test = fixture();
9381032
test.selectors.set("CONTAINERS_CONF", "/home/test/user-containers.conf");

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

Lines changed: 98 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ describe("uninstall OpenShell gateway user service", () => {
236236
? ok(`${String(process.getuid?.() ?? -1)}\n`)
237237
: command === "ps" && args.includes("args=")
238238
? ok("/usr/local/bin/openshell-gateway --name nemoclaw --port 8080\n")
239-
: ok(),
239+
: ok(),
240240
},
241241
[{ name: "nemoclaw" }, { name: "sibling" }],
242242
);
@@ -335,7 +335,9 @@ describe("uninstall OpenShell gateway user service", () => {
335335
(command === "systemctl" &&
336336
args.includes("--property=MainPID") &&
337337
ok(`${String(pid)}\n`)) ||
338-
(command === "ps" && args.includes("uid=") && ok(`${String(process.getuid?.() ?? -1)}\n`)) ||
338+
(command === "ps" &&
339+
args.includes("uid=") &&
340+
ok(`${String(process.getuid?.() ?? -1)}\n`)) ||
339341
(command === "ps" &&
340342
args.includes("args=") &&
341343
ok("/usr/local/bin/openshell-gateway --name nemoclaw --port 8080\n")) ||
@@ -445,82 +447,82 @@ describe("uninstall OpenShell gateway user service", () => {
445447
{ externallySupervised: false, keepOpenShell: false, mode: "managed cleanup" },
446448
{ externallySupervised: false, keepOpenShell: true, mode: "--keep-openshell" },
447449
{ externallySupervised: true, keepOpenShell: false, mode: "external supervision" },
448-
])("does not mutate scoped resources with an unproven sandbox namespace under $mode (#8663)", ({
449-
externallySupervised,
450-
keepOpenShell,
451-
}) => {
452-
const test = fixture(true);
453-
const servicePath = writeManagedService(test);
454-
const configPath = writeGatewayState(test);
455-
const registryPath = writeSelectedSandboxRegistry(test, "my-assistant");
456-
const registryBefore = fs.readFileSync(registryPath, "utf-8");
457-
fs.writeFileSync(configPath, '[openshell.drivers.docker]\nsandbox_namespace = "default"\n');
458-
const calls: string[][] = [];
459-
const errors: string[] = [];
460-
461-
const externalAuthority = externallySupervised
462-
? {
463-
resolveGatewayTeardownAuthority: ({
464-
gatewayName,
465-
gatewayPort,
466-
}: {
467-
gatewayName: string;
468-
gatewayPort: number;
469-
}) => ({
470-
gatewayName,
471-
gatewayPort,
472-
mode: "externally-supervised" as const,
473-
source: "declared" as const,
474-
endpoint: `http://127.0.0.1:${String(gatewayPort)}`,
475-
stateDir: path.dirname(configPath),
476-
supervisor: {
477-
kind: "systemd-user" as const,
478-
serviceName: "external-openshell.service",
479-
execPath: "/usr/local/bin/openshell-gateway",
480-
},
481-
requiredCapabilities: [],
482-
}),
483-
}
484-
: {};
485-
const deps: Partial<UninstallRunDeps> = {
486-
commandExists: (command) => command === "systemctl",
487-
run: (command, args) => {
488-
calls.push([command, ...args]);
489-
return ok();
490-
},
491-
error: (message) => errors.push(message),
492-
...externalAuthority,
493-
};
494-
495-
const result = uninstall(test, keepOpenShell, deps, [
496-
{ name: "nemoclaw" },
497-
{ name: "nemoclaw-8081" },
498-
]);
499-
500-
expect(result.exitCode).toBe(1);
501-
expect(fs.existsSync(servicePath)).toBe(true);
502-
expect(fs.existsSync(configPath)).toBe(true);
503-
expect(fs.readFileSync(registryPath, "utf-8")).toBe(registryBefore);
504-
expect(
505-
calls.some(([command, resource]) => command === "openshell" && resource === "sandbox"),
506-
).toBe(false);
507-
expect(
508-
calls.some(
509-
([command, resource, action]) =>
510-
command === "openshell" && resource === "gateway" && action === "remove",
511-
),
512-
).toBe(false);
513-
expect(
514-
calls.some(
515-
([command, ...args]) => command === "systemctl" && !args.includes("--property=MainPID"),
516-
),
517-
).toBe(false);
518-
expect(errors).toContain(
519-
externallySupervised
520-
? "Refusing scoped gateway cleanup because the externally supervised process's loaded sandbox namespace cannot be proven."
521-
: "Refusing scoped gateway cleanup because its sandbox namespace cannot be proven.",
522-
);
523-
});
450+
])(
451+
"does not mutate scoped resources with an unproven sandbox namespace under $mode (#8663)",
452+
({ externallySupervised, keepOpenShell }) => {
453+
const test = fixture(true);
454+
const servicePath = writeManagedService(test);
455+
const configPath = writeGatewayState(test);
456+
const registryPath = writeSelectedSandboxRegistry(test, "my-assistant");
457+
const registryBefore = fs.readFileSync(registryPath, "utf-8");
458+
fs.writeFileSync(configPath, '[openshell.drivers.docker]\nsandbox_namespace = "default"\n');
459+
const calls: string[][] = [];
460+
const errors: string[] = [];
461+
462+
const externalAuthority = externallySupervised
463+
? {
464+
resolveGatewayTeardownAuthority: ({
465+
gatewayName,
466+
gatewayPort,
467+
}: {
468+
gatewayName: string;
469+
gatewayPort: number;
470+
}) => ({
471+
gatewayName,
472+
gatewayPort,
473+
mode: "externally-supervised" as const,
474+
source: "declared" as const,
475+
endpoint: `http://127.0.0.1:${String(gatewayPort)}`,
476+
stateDir: path.dirname(configPath),
477+
supervisor: {
478+
kind: "systemd-user" as const,
479+
serviceName: "external-openshell.service",
480+
execPath: "/usr/local/bin/openshell-gateway",
481+
},
482+
requiredCapabilities: [],
483+
}),
484+
}
485+
: {};
486+
const deps: Partial<UninstallRunDeps> = {
487+
commandExists: (command) => command === "systemctl",
488+
run: (command, args) => {
489+
calls.push([command, ...args]);
490+
return ok();
491+
},
492+
error: (message) => errors.push(message),
493+
...externalAuthority,
494+
};
495+
496+
const result = uninstall(test, keepOpenShell, deps, [
497+
{ name: "nemoclaw" },
498+
{ name: "nemoclaw-8081" },
499+
]);
500+
501+
expect(result.exitCode).toBe(1);
502+
expect(fs.existsSync(servicePath)).toBe(true);
503+
expect(fs.existsSync(configPath)).toBe(true);
504+
expect(fs.readFileSync(registryPath, "utf-8")).toBe(registryBefore);
505+
expect(
506+
calls.some(([command, resource]) => command === "openshell" && resource === "sandbox"),
507+
).toBe(false);
508+
expect(
509+
calls.some(
510+
([command, resource, action]) =>
511+
command === "openshell" && resource === "gateway" && action === "remove",
512+
),
513+
).toBe(false);
514+
expect(
515+
calls.some(
516+
([command, ...args]) => command === "systemctl" && !args.includes("--property=MainPID"),
517+
),
518+
).toBe(false);
519+
expect(errors).toContain(
520+
externallySupervised
521+
? "Refusing scoped gateway cleanup because the externally supervised process's loaded sandbox namespace cannot be proven."
522+
: "Refusing scoped gateway cleanup because its sandbox namespace cannot be proven.",
523+
);
524+
},
525+
);
524526

525527
it("preserves the marked Linux unit when scoped sandbox deletion fails (#8220)", () => {
526528
const test = fixture(true);
@@ -698,18 +700,31 @@ describe("uninstall OpenShell gateway user service", () => {
698700
const test = fixture();
699701
const servicePath = writeManagedService(test);
700702
const errors: string[] = [];
703+
const run = vi.fn((command: string, args: string[]) =>
704+
command === "systemctl" && args.includes("disable")
705+
? { status: 1, stdout: "", stderr: "failed" }
706+
: ok(),
707+
);
701708

702709
const result = uninstall(test, false, {
703-
commandExists: (command) => command === "systemctl",
710+
commandExists: (command) => ["systemctl", "npm"].includes(command),
704711
error: (line) => errors.push(line),
705-
run: (command, args) =>
706-
command === "systemctl" && args.includes("disable")
707-
? { status: 1, stdout: "", stderr: "failed" }
708-
: ok(),
712+
run,
709713
});
710714

711715
expect(result.exitCode).toBe(1);
712716
expect(fs.existsSync(servicePath)).toBe(true);
717+
const failedDisableIndex = run.mock.calls.findIndex(
718+
([command, args]) => command === "systemctl" && args.includes("disable"),
719+
);
720+
const npmCleanupIndex = run.mock.calls.findIndex(
721+
([command, args], index) =>
722+
index > failedDisableIndex &&
723+
command === "npm" &&
724+
args.join(" ") === "uninstall -g --loglevel=error nemoclaw",
725+
);
726+
expect(failedDisableIndex).toBeGreaterThanOrEqual(0);
727+
expect(npmCleanupIndex).toBeGreaterThan(failedDisableIndex);
713728
expect(errors).toContain(
714729
"Uninstall completed with errors. Some state may remain on disk; see warnings above.",
715730
);

0 commit comments

Comments
 (0)