diff --git a/src/lib/actions/maintenance.test.ts b/src/lib/actions/maintenance.test.ts index 830e66ad5f6..5a7dfc99357 100644 --- a/src/lib/actions/maintenance.test.ts +++ b/src/lib/actions/maintenance.test.ts @@ -8,12 +8,14 @@ const mocks = vi.hoisted(() => ({ backupSandboxState: vi.fn(), captureSandboxListWithGatewayPreflightOrExit: vi.fn(), parseReadySandboxNames: vi.fn(), + parseLiveSandboxNames: vi.fn(), dockerListImagesFormat: vi.fn().mockReturnValue(""), dockerRmi: vi.fn(), prompt: vi.fn(), startStoppedSandboxContainerForBackup: vi.fn(), backupStartedSandboxState: vi.fn(), returnSandboxContainerToStopped: vi.fn(), + isSandboxContainerDefinitivelyAbsent: vi.fn(), })); vi.mock("../state/registry", () => ({ @@ -30,6 +32,15 @@ vi.mock("../openshell-sandbox-list", () => ({ })); vi.mock("../runtime-recovery", () => ({ parseReadySandboxNames: mocks.parseReadySandboxNames, + parseLiveSandboxNames: mocks.parseLiveSandboxNames, +})); +// GATEWAY_PORT is baked from NEMOCLAW_GATEWAY_PORT at module load. Pin it so +// the #6520 orphan-classification tests (which run the real gateway-binding +// resolvers against literal ports) don't invert on a shell that exports a +// non-default gateway port. +vi.mock("../core/ports", async (importOriginal) => ({ + ...(await importOriginal()), + GATEWAY_PORT: 8080, })); vi.mock("../adapters/docker", () => ({ dockerListImagesFormat: mocks.dockerListImagesFormat, @@ -45,6 +56,7 @@ vi.mock("./sandbox/stopped-sandbox-backup", () => ({ startStoppedSandboxContainerForBackup: mocks.startStoppedSandboxContainerForBackup, backupStartedSandboxState: mocks.backupStartedSandboxState, returnSandboxContainerToStopped: mocks.returnSandboxContainerToStopped, + isSandboxContainerDefinitivelyAbsent: mocks.isSandboxContainerDefinitivelyAbsent, })); vi.mock("../domain/lifecycle/options", () => ({ normalizeGarbageCollectImagesOptions: (o: unknown) => o || {}, @@ -70,6 +82,11 @@ describe("backupAll", () => { output: "sb-good\nsb-bad\n", }); mocks.parseReadySandboxNames.mockReturnValue(new Set(["sb-good", "sb-bad"])); + // Defaults keep every pre-#6520 case on its original path: no sandbox is + // gateway-observed (so orphan classification is decided by the absence + // gate alone) and no container is ever definitively absent. + mocks.parseLiveSandboxNames.mockReturnValue(new Set()); + mocks.isSandboxContainerDefinitivelyAbsent.mockReturnValue(false); mocks.startStoppedSandboxContainerForBackup.mockReturnValue(null); mocks.returnSandboxContainerToStopped.mockReturnValue(true); }); @@ -174,10 +191,17 @@ describe("backupAll", () => { await backupAll(); - expect(mocks.captureSandboxListWithGatewayPreflightOrExit).toHaveBeenCalledWith({ - action: "backing up registered sandboxes", - command: "nemoclaw backup-all", - }); + // The listing must be pinned to the selected gateway (#6114/#6520): + // OpenShell's mutable current selection may be a sibling gateway, and an + // unpinned list would let the orphan classifier make a fail-open + // stranded call from another gateway's sandboxes. + expect(mocks.captureSandboxListWithGatewayPreflightOrExit).toHaveBeenCalledWith( + { + action: "backing up registered sandboxes", + command: "nemoclaw backup-all", + }, + { gatewayName: "nemoclaw" }, + ); expect(mocks.backupSandboxState).toHaveBeenCalledWith("sb-good"); logSpy.mockRestore(); }); @@ -684,6 +708,184 @@ describe("backupAll", () => { errorSpy.mockRestore(); exitSpy.mockRestore(); }); + + it("skips a stranded orphan sandbox without failing strict backup (#6520)", async () => { + // Uninstall + reinstall strands a sandbox: gateway registration and + // container removed, sandboxes.json preserved. There is nothing left to + // back up, so strict backup-all must warn and move on instead of aborting + // before the installer's recovery phase can surface the orphan. + mocks.listSandboxes.mockReturnValue({ + sandboxes: [{ name: "sb-good" }, { name: "sb-stranded" }], + defaultSandbox: null, + }); + mocks.parseReadySandboxNames.mockReturnValue(new Set(["sb-good"])); + mocks.parseLiveSandboxNames.mockReturnValue(new Set(["sb-good"])); + mocks.isSandboxContainerDefinitivelyAbsent.mockImplementation( + (name: string) => name === "sb-stranded", + ); + mocks.backupSandboxState.mockReturnValue({ + success: true, + backedUpDirs: ["workspace"], + failedDirs: [], + backedUpFiles: [], + failedFiles: [], + manifest: { backupPath: "/backups/sb-good/timestamp" }, + }); + process.env.NEMOCLAW_REQUIRE_ALL_SANDBOX_BACKUPS = "1"; + const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`exit:${code}`); + }) as never); + + await backupAll(); + + expect(exitSpy).not.toHaveBeenCalled(); + expect(mocks.backupSandboxState).toHaveBeenCalledWith("sb-good"); + expect(mocks.backupStartedSandboxState).not.toHaveBeenCalled(); + // The exemption requires a confirming second pinned listing after the loop. + expect(mocks.captureSandboxListWithGatewayPreflightOrExit).toHaveBeenCalledTimes(2); + expect(mocks.captureSandboxListWithGatewayPreflightOrExit).toHaveBeenNthCalledWith( + 2, + { + action: "confirming stranded sandboxes remain absent from the selected gateway", + command: "nemoclaw backup-all", + }, + { gatewayName: "nemoclaw" }, + ); + const logOutput = logSpy.mock.calls.flat().join("\n"); + expect(logOutput).toContain( + "1 recorded sandbox(es) were not found on their recorded gateway: sb-stranded.", + ); + expect(logOutput).toContain("destroy` to clear a stranded record"); + expect(logOutput).toContain("onboard` to rebuild it"); + expect(logOutput).toContain("1 backed up, 0 failed, 0 skipped"); + expect(logOutput).not.toContain("Skipping 'sb-stranded'"); + }); + + it("keeps the strict abort for an absent sandbox bound to a different gateway (#6520)", async () => { + // A sandbox persisted against a sibling gateway may be healthy there; + // this gateway's backup-all must never claim it is stranded, even when + // its container is absent on this host. + mocks.listSandboxes.mockReturnValue({ + sandboxes: [{ name: "sb-other", gatewayPort: 9999 }], + defaultSandbox: null, + }); + mocks.parseReadySandboxNames.mockReturnValue(new Set()); + mocks.parseLiveSandboxNames.mockReturnValue(new Set()); + mocks.isSandboxContainerDefinitivelyAbsent.mockReturnValue(true); + process.env.NEMOCLAW_REQUIRE_ALL_SANDBOX_BACKUPS = "1"; + const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`exit:${code}`); + }) as never); + + await expect(backupAll()).rejects.toThrow("exit:1"); + + expect(exitSpy).toHaveBeenCalledWith(1); + const logOutput = logSpy.mock.calls.flat().join("\n"); + expect(logOutput).toContain("Skipping 'sb-other' (not running"); + expect(logOutput).not.toContain("were not found on their recorded gateway"); + expect(errorSpy.mock.calls.flat().join("\n")).toContain( + "requires every registered sandbox to be backed up", + ); + }); + + it("keeps the strict abort when an unobserved sandbox still has a container (#6520)", async () => { + // Orphan classification alone is race-prone: a sandbox mid-reconnect (or + // one whose gateway row is drifting) is unobserved on the gateway yet its + // container still exists. Only definitive container absence may downgrade + // the strict abort to a stranded-orphan warning. + mocks.listSandboxes.mockReturnValue({ + sandboxes: [{ name: "sb-reconnecting" }], + defaultSandbox: null, + }); + mocks.parseReadySandboxNames.mockReturnValue(new Set()); + mocks.parseLiveSandboxNames.mockReturnValue(new Set()); + mocks.isSandboxContainerDefinitivelyAbsent.mockReturnValue(false); + process.env.NEMOCLAW_REQUIRE_ALL_SANDBOX_BACKUPS = "1"; + const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); + vi.spyOn(console, "error").mockImplementation(() => undefined); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`exit:${code}`); + }) as never); + + await expect(backupAll()).rejects.toThrow("exit:1"); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(mocks.isSandboxContainerDefinitivelyAbsent).toHaveBeenCalledWith("sb-reconnecting"); + const logOutput = logSpy.mock.calls.flat().join("\n"); + expect(logOutput).toContain("Skipping 'sb-reconnecting' (not running"); + expect(logOutput).not.toContain("were not found on their recorded gateway"); + }); + + it("reverts a stranded candidate to a strict skip when the confirming listing observes it again (#6520)", async () => { + // The pre-loop listing can be minutes stale by the time the loop ends. A + // candidate the confirming second listing observes has reconnected — the + // exemption must not apply and strict mode must keep failing closed. + mocks.listSandboxes.mockReturnValue({ + sandboxes: [{ name: "sb-flapping" }], + defaultSandbox: null, + }); + mocks.parseReadySandboxNames.mockReturnValue(new Set()); + mocks.captureSandboxListWithGatewayPreflightOrExit + .mockResolvedValueOnce({ status: 0, output: "" }) + .mockResolvedValueOnce({ + status: 0, + output: "sb-flapping openshell 2026-07-21 10:00:00 Ready\n", + }); + mocks.parseLiveSandboxNames.mockImplementation((output: string) => + output.includes("sb-flapping") ? new Set(["sb-flapping"]) : new Set(), + ); + mocks.isSandboxContainerDefinitivelyAbsent.mockReturnValue(true); + process.env.NEMOCLAW_REQUIRE_ALL_SANDBOX_BACKUPS = "1"; + const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); + vi.spyOn(console, "error").mockImplementation(() => undefined); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`exit:${code}`); + }) as never); + + await expect(backupAll()).rejects.toThrow("exit:1"); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(mocks.captureSandboxListWithGatewayPreflightOrExit).toHaveBeenCalledTimes(2); + const logOutput = logSpy.mock.calls.flat().join("\n"); + expect(logOutput).toContain("Skipping 'sb-flapping' (not running"); + expect(logOutput).toContain("0 backed up, 0 failed, 1 skipped"); + expect(logOutput).not.toContain("were not found on their recorded gateway"); + }); + + it("reverts a stranded candidate to a strict skip when its container reappears (#6520)", async () => { + mocks.listSandboxes.mockReturnValue({ + sandboxes: [{ name: "sb-flapping" }], + defaultSandbox: null, + }); + mocks.parseReadySandboxNames.mockReturnValue(new Set()); + mocks.captureSandboxListWithGatewayPreflightOrExit.mockResolvedValue({ + status: 0, + output: "", + }); + mocks.parseLiveSandboxNames.mockReturnValue(new Set()); + mocks.isSandboxContainerDefinitivelyAbsent.mockReturnValueOnce(true).mockReturnValueOnce(false); + process.env.NEMOCLAW_REQUIRE_ALL_SANDBOX_BACKUPS = "1"; + const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); + vi.spyOn(console, "error").mockImplementation(() => undefined); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`exit:${code}`); + }) as never); + + await expect(backupAll()).rejects.toThrow("exit:1"); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(mocks.captureSandboxListWithGatewayPreflightOrExit).toHaveBeenCalledTimes(2); + expect(mocks.isSandboxContainerDefinitivelyAbsent).toHaveBeenCalledTimes(2); + expect(mocks.isSandboxContainerDefinitivelyAbsent).toHaveBeenNthCalledWith(1, "sb-flapping"); + expect(mocks.isSandboxContainerDefinitivelyAbsent).toHaveBeenNthCalledWith(2, "sb-flapping"); + const logOutput = logSpy.mock.calls.flat().join("\n"); + expect(logOutput).toContain("Skipping 'sb-flapping' (not running"); + expect(logOutput).toContain("0 backed up, 0 failed, 1 skipped"); + expect(logOutput).not.toContain("were not found on their recorded gateway"); + }); }); describe("shouldSkipUnreachableSandboxBackup", () => { diff --git a/src/lib/actions/maintenance.ts b/src/lib/actions/maintenance.ts index 2910f6d8447..42812478c27 100644 --- a/src/lib/actions/maintenance.ts +++ b/src/lib/actions/maintenance.ts @@ -13,14 +13,21 @@ import { normalizeGarbageCollectImagesOptions, } from "../domain/lifecycle/options"; import { findOrphanedSandboxImages, parseSandboxImageRows } from "../domain/maintenance/images"; +import { + classifyOrphanedRegistrySandboxes, + orphanedRegistryRemediation, + orphanedRegistrySummary, +} from "../domain/maintenance/orphan-detection"; import { SANDBOX_IMAGE_REPOS } from "../domain/sandbox/image-tag"; +import { resolveGatewayName, resolveSandboxGatewayName } from "../onboard/gateway-binding"; import { captureSandboxListWithGatewayPreflightOrExit } from "../openshell-sandbox-list"; -import { parseReadySandboxNames } from "../runtime-recovery"; +import { parseLiveSandboxNames, parseReadySandboxNames } from "../runtime-recovery"; import * as registry from "../state/registry"; import * as sandboxState from "../state/sandbox"; import { nemoclawStateRoot, resolveHome } from "../state/state-root"; import { backupStartedSandboxState, + isSandboxContainerDefinitivelyAbsent, returnSandboxContainerToStopped, type StartedForBackup, startStoppedSandboxContainerForBackup, @@ -56,11 +63,53 @@ export async function backupAll(): Promise { return; } - const liveList = await captureSandboxListWithGatewayPreflightOrExit({ - action: "backing up registered sandboxes", - command: `${CLI_NAME} backup-all`, - }); + // Pin the listing to the selected gateway (#6114/#6520): OpenShell's + // mutable current selection may be a sibling gateway, and an unpinned list + // would both misjudge readiness and let the orphan classifier below make a + // fail-open stranded call from another gateway's sandboxes. + const selectedGatewayName = resolveGatewayName(GATEWAY_PORT); + const liveList = await captureSandboxListWithGatewayPreflightOrExit( + { + action: "backing up registered sandboxes", + command: `${CLI_NAME} backup-all`, + }, + { gatewayName: selectedGatewayName }, + ); const readyNames = parseReadySandboxNames(liveList.output || ""); + // Source-of-truth review (#6520): + // + // - Invalid state: a sandbox the selected gateway does not observe, whose + // persisted binding resolves to that gateway, and whose OpenShell-labeled + // container is definitively absent is stranded. It has no state left to + // back up, so counting it as a strict-gate skip would abort the + // installer's pre-upgrade backup before its recovery phase + // (recover_preexisting_sandboxes_before_onboard in scripts/install.sh) + // that knows how to surface it ever runs. + // - Source boundary: the state is created by `nemoclaw uninstall`, which + // removes the gateway registration and containers but deliberately + // preserves sandboxes.json so a later reinstall can rebuild from it. + // - Source-fix constraint: backup-all must not reconcile the registry — + // clearing a stranded record is owned by the recovery phase's + // destroy/onboard guidance (and the user), and this gate runs before + // that phase. Deleting records inside a backup command would destroy the + // very evidence the recovery phase reports. + // - Removal condition: drop this exemption when install/uninstall + // reconciles sandboxes.json against the gateway (stranded records can no + // longer reach backup-all), or when the installer runs its recovery + // phase before the strict pre-upgrade backup. + // + // The container-absence gate (checked per candidate at skip time and again + // after the confirming listing) makes the exemption race-safe: a + // reconnecting or sibling-healthy sandbox still has a container, and a + // candidate the gateway observes again reverts to a genuine strict skip. + const orphanNames = new Set( + classifyOrphanedRegistrySandboxes(sandboxes, { + observedNames: parseLiveSandboxNames(liveList.output || ""), + reconnectedNames: new Set(), + selectedGatewayName, + resolveGatewayBinding: resolveSandboxGatewayName, + }).map((sandbox) => sandbox.name), + ); const skipUnreachable = shouldSkipUnreachableSandboxBackup(process.env); const requireAll = process.env.NEMOCLAW_REQUIRE_ALL_SANDBOX_BACKUPS === "1"; @@ -69,6 +118,7 @@ export async function backupAll(): Promise { let skipped = 0; let unreachableRunning = 0; let notRunningSkipped = 0; + const strandedOrphans: string[] = []; for (const sb of sandboxes) { // A registered docker-driver sandbox whose container is merely stopped is // backupable: start it for the duration of the backup and return it to @@ -78,6 +128,12 @@ export async function backupAll(): Promise { if (!readyNames.has(sb.name)) { startedForBackup = startStoppedSandboxContainerForBackup(sb.name); if (!startedForBackup) { + if (orphanNames.has(sb.name) && isSandboxContainerDefinitivelyAbsent(sb.name)) { + // Tracked separately from `skipped` so the strict gate stays + // untripped: there is nothing to back up and nothing to start. + strandedOrphans.push(sb.name); + continue; + } console.log(` ${D}${notRunningBackupSkipMessage(sb.name)}${R}`); skipped++; notRunningSkipped++; @@ -176,11 +232,41 @@ export async function backupAll(): Promise { failed++; } } + // The classification above is only as fresh as the pre-loop listing, and + // the backup loop can run for minutes. Confirm with a second pinned listing + // that every stranded candidate is still unobserved before accepting the + // exemption (same two-phase confirmation as upgrade-sandboxes, #6114); a + // candidate that reappeared reverts to the genuine strict skip it would + // otherwise have been. + let confirmedStranded = strandedOrphans; + if (strandedOrphans.length > 0) { + const confirmation = await captureSandboxListWithGatewayPreflightOrExit( + { + action: "confirming stranded sandboxes remain absent from the selected gateway", + command: `${CLI_NAME} backup-all`, + }, + { gatewayName: selectedGatewayName }, + ); + const observedOnRecheck = parseLiveSandboxNames(confirmation.output || ""); + confirmedStranded = strandedOrphans.filter( + (name) => !observedOnRecheck.has(name) && isSandboxContainerDefinitivelyAbsent(name), + ); + const confirmedNames = new Set(confirmedStranded); + for (const name of strandedOrphans.filter((entry) => !confirmedNames.has(entry))) { + console.log(` ${D}${notRunningBackupSkipMessage(name)}${R}`); + skipped++; + notRunningSkipped++; + } + } console.log(""); console.log(` Pre-upgrade backup: ${backed} backed up, ${failed} failed, ${skipped} skipped`); if (backed > 0) { console.log(` Backups stored in: ${rebuildBackupsDirectory(resolveHome(), GATEWAY_PORT)}`); } + if (confirmedStranded.length > 0) { + console.log(` ${YW}${orphanedRegistrySummary(confirmedStranded)}${R}`); + console.log(` ${D}${orphanedRegistryRemediation(CLI_NAME)}${R}`); + } if (failed > 0) { if (unreachableRunning > 0) { console.error(""); diff --git a/src/lib/actions/sandbox/stopped-sandbox-backup.test.ts b/src/lib/actions/sandbox/stopped-sandbox-backup.test.ts index b52442fd3ff..e13fc7264ff 100644 --- a/src/lib/actions/sandbox/stopped-sandbox-backup.test.ts +++ b/src/lib/actions/sandbox/stopped-sandbox-backup.test.ts @@ -3,6 +3,15 @@ import { describe, expect, it, vi } from "vitest"; +const adapterMocks = vi.hoisted(() => ({ + dockerRun: vi.fn(), + dockerCapture: vi.fn(), +})); + +vi.mock("../../adapters/docker/run", () => ({ + dockerRun: adapterMocks.dockerRun, + dockerCapture: adapterMocks.dockerCapture, +})); vi.mock("../../state/registry", () => ({ getSandbox: vi.fn(), listSandboxes: vi.fn(), @@ -11,8 +20,10 @@ vi.mock("../../state/sandbox", () => ({ backupSandboxState: vi.fn(), })); +import * as registry from "../../state/registry"; import { backupStartedSandboxState, + isSandboxContainerDefinitivelyAbsent, returnSandboxContainerToStopped, startStoppedSandboxContainerForBackup, } from "./stopped-sandbox-backup"; @@ -97,6 +108,74 @@ describe("startStoppedSandboxContainerForBackup", () => { }); }); +describe("isSandboxContainerDefinitivelyAbsent (#6520)", () => { + const deps = (over: Record = {}) => ({ + getSandboxDriver: vi.fn().mockReturnValue("docker"), + listLabeledContainerNames: vi.fn().mockReturnValue([]), + ...over, + }); + + it("reports absent when a successful labeled listing shows zero containers", () => { + expect(isSandboxContainerDefinitivelyAbsent("my-sb", deps())).toBe(true); + }); + + it("reports present when a labeled container still exists", () => { + const d = deps({ listLabeledContainerNames: vi.fn().mockReturnValue(["openshell-my-sb-abc"]) }); + expect(isSandboxContainerDefinitivelyAbsent("my-sb", d)).toBe(false); + }); + + it("fails closed for non-docker-driver sandboxes", () => { + const d = deps({ getSandboxDriver: vi.fn().mockReturnValue("kubernetes") }); + expect(isSandboxContainerDefinitivelyAbsent("my-sb", d)).toBe(false); + expect(d.listLabeledContainerNames).not.toHaveBeenCalled(); + }); + + it("fails closed when the labeled listing itself fails (a swallowed ps error is not absence)", () => { + const d = deps({ listLabeledContainerNames: vi.fn().mockReturnValue(null) }); + expect(isSandboxContainerDefinitivelyAbsent("my-sb", d)).toBe(false); + }); + + it("fails closed when the registry read behind the driver gate throws", () => { + vi.mocked(registry.getSandbox).mockImplementation(() => { + throw new Error("corrupt sandboxes.json"); + }); + expect(isSandboxContainerDefinitivelyAbsent("my-sb")).toBe(false); + expect(adapterMocks.dockerRun).not.toHaveBeenCalled(); + }); + + it("status-checks the default listing with ignoreError so a dead daemon fails closed, not the process", () => { + // runner.run() calls process.exit on a non-zero status unless ignoreError + // is set, and a swallowed listing error must never read as "absent": a + // failed `docker ps` has to surface as false, not as an exit and not as + // an empty listing. + vi.mocked(registry.getSandbox).mockReturnValue({ + openshellDriver: "docker", + } as unknown as ReturnType); + adapterMocks.dockerRun.mockReturnValue({ status: 1, stdout: "" }); + expect(isSandboxContainerDefinitivelyAbsent("my-sb")).toBe(false); + expect(adapterMocks.dockerRun).toHaveBeenCalledWith( + expect.arrayContaining(["ps", "-a", "--filter", "label=openshell.ai/sandbox-name=my-sb"]), + expect.objectContaining({ ignoreError: true }), + ); + }); + + it("reports absent through the default wiring when the listing succeeds empty", () => { + vi.mocked(registry.getSandbox).mockReturnValue({ + openshellDriver: "docker", + } as unknown as ReturnType); + adapterMocks.dockerRun.mockReturnValue({ status: 0, stdout: "\n" }); + expect(isSandboxContainerDefinitivelyAbsent("my-sb")).toBe(true); + }); + + it("reports present through the default wiring when the listing returns a container", () => { + vi.mocked(registry.getSandbox).mockReturnValue({ + openshellDriver: "docker", + } as unknown as ReturnType); + adapterMocks.dockerRun.mockReturnValue({ status: 0, stdout: "openshell-my-sb-abc\n" }); + expect(isSandboxContainerDefinitivelyAbsent("my-sb")).toBe(false); + }); +}); + describe("returnSandboxContainerToStopped", () => { it("reports success when docker stop echoes the name and inspect confirms exited", () => { const dockerStop = vi.fn().mockReturnValue("openshell-my-sb-abc123"); diff --git a/src/lib/actions/sandbox/stopped-sandbox-backup.ts b/src/lib/actions/sandbox/stopped-sandbox-backup.ts index 04fef0e7bcb..b1a9c9d6e7f 100644 --- a/src/lib/actions/sandbox/stopped-sandbox-backup.ts +++ b/src/lib/actions/sandbox/stopped-sandbox-backup.ts @@ -2,12 +2,29 @@ // SPDX-License-Identifier: Apache-2.0 import { dockerContainerInspectFormat } from "../../adapters/docker/inspect"; -import { dockerCapture } from "../../adapters/docker/run"; -import { findLabeledSandboxContainers } from "../../onboard/docker-driver-sandbox-recovery"; +import { dockerCapture, dockerRun } from "../../adapters/docker/run"; +import { + findLabeledSandboxContainers, + OPENSHELL_MANAGED_BY_LABEL, + OPENSHELL_MANAGED_BY_VALUE, + OPENSHELL_SANDBOX_NAME_LABEL, +} from "../../onboard/docker-driver-sandbox-recovery"; import * as registry from "../../state/registry"; import * as sandboxState from "../../state/sandbox"; import { resolveSandboxContainerOwner } from "./sandbox-container-owner"; +/** Read a registered sandbox's OpenShell driver, treating registry read + * failure as unknown so callers fail closed on driver-gated decisions. */ +function readSandboxDriver(name: string): string | null | undefined { + try { + return registry.getSandbox(name)?.openshellDriver; + } catch { + return undefined; + } +} + +const DOCKER_ABSENCE_PROBE_TIMEOUT_MS = 5_000; + /** * Backup support for registered docker-driver sandboxes whose container is * stopped. `backup-all` skips sandboxes the gateway does not report Ready, @@ -38,13 +55,7 @@ interface StartDeps { } const defaultStartDeps: StartDeps = { - getSandboxDriver: (name) => { - try { - return registry.getSandbox(name)?.openshellDriver; - } catch { - return undefined; - } - }, + getSandboxDriver: readSandboxDriver, listSandboxNames: () => registry.listSandboxes().sandboxes.map((entry) => entry.name), listLabeledContainerNames: (sandboxName) => findLabeledSandboxContainers(sandboxName).map((container) => container.name), @@ -81,6 +92,66 @@ export function startStoppedSandboxContainerForBackup( return { containerName }; } +interface ContainerAbsenceDeps { + getSandboxDriver: (name: string) => string | null | undefined; + /** Labeled container names for the sandbox, or null when the listing itself + * failed (dead daemon, timeout) and absence must not be concluded. */ + listLabeledContainerNames: (name: string) => string[] | null; +} + +const defaultContainerAbsenceDeps: ContainerAbsenceDeps = { + getSandboxDriver: readSandboxDriver, + // findLabeledSandboxContainers swallows docker errors (a dead daemon reads + // as "no containers"), which suits its recovery callers but not an absence + // proof. Run the same labeled listing status-checked instead: any spawn + // error, timeout, or non-zero exit yields null, never "absent". ignoreError + // prevents runner.run() from exiting the process when the listing fails. + listLabeledContainerNames: (name) => { + const result = dockerRun( + [ + "ps", + "-a", + "--filter", + `label=${OPENSHELL_MANAGED_BY_LABEL}=${OPENSHELL_MANAGED_BY_VALUE}`, + "--filter", + `label=${OPENSHELL_SANDBOX_NAME_LABEL}=${name}`, + "--format", + "{{.Names}}", + ], + { + encoding: "utf-8", + stdio: ["ignore", "pipe", "pipe"], + ignoreError: true, + suppressOutput: true, + timeout: DOCKER_ABSENCE_PROBE_TIMEOUT_MS, + }, + ); + if (result.error || result.status !== 0) return null; + return String(result.stdout || "") + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean); + }, +}; + +/** + * Returns true only when the registered sandbox uses Docker and a successful + * labeled `docker ps -a` returns no matching container. + * + * Returns false when the driver is not Docker, the registry read fails, or the + * Docker listing fails or times out. Callers must separately confirm gateway + * absence and same-gateway binding before classifying a sandbox as stranded. + */ +export function isSandboxContainerDefinitivelyAbsent( + sandboxName: string, + depsOverride: Partial = {}, +): boolean { + const deps: ContainerAbsenceDeps = { ...defaultContainerAbsenceDeps, ...depsOverride }; + if (deps.getSandboxDriver(sandboxName) !== "docker") return false; + const labeledContainerNames = deps.listLabeledContainerNames(sandboxName); + return labeledContainerNames !== null && labeledContainerNames.length === 0; +} + interface StopDeps { dockerStop: (containerName: string) => string; dockerInspectStatus: (containerName: string) => string; diff --git a/test/install-orphaned-sandbox-recovery.test.ts b/test/install-orphaned-sandbox-recovery.test.ts index 3163f1ae444..2740e7d480b 100644 --- a/test/install-orphaned-sandbox-recovery.test.ts +++ b/test/install-orphaned-sandbox-recovery.test.ts @@ -98,6 +98,64 @@ function runPrintDone(flags: { recoveryRan: string; orphaned: string }): { }; } +function runStrictBackupRecoveryFlow(): { output: string; cleanup: () => void } { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-install-orphan-flow-")); + const cliLog = path.join(tmp, "cli.log"); + const stubBin = path.join(tmp, "stub-cli"); + fs.writeFileSync( + stubBin, + `#!/usr/bin/env bash +printf 'command=%s require_all=%s\\n' "$*" "\${NEMOCLAW_REQUIRE_ALL_SANDBOX_BACKUPS:-}" >> ${JSON.stringify(cliLog)} +case "\${1:-}" in + backup-all) + printf ' Pre-upgrade backup: 0 backed up, 0 failed, 0 skipped\\n' + ;; + upgrade-sandboxes) + printf '%s\\n' ${JSON.stringify(ORPHAN_LINE)} + printf '%s\\n' ${JSON.stringify(NO_REBUILD_LINE)} + ;; +esac +`, + { mode: 0o755 }, + ); + + const snippet = ` + set -e + source "${INSTALLER_PAYLOAD}" >/dev/null 2>&1 || true + prepare_current_cli_for_preupgrade_backup() { return 0; } + resolve_prepared_cli_runner() { printf '%s' "${stubBin}"; } + sleep() { :; } + needs_shell_reload() { return 1; } + _PREEXISTING_SANDBOX_COUNT=1 + _PREEXISTING_SANDBOX_RECOVERY_RAN=false + _PREEXISTING_SANDBOX_ORPHANED=false + _UPGRADE_SANDBOXES_FAILED=false + _INSTALL_START=0 + _CLI_DISPLAY="NemoClaw" + _CLI_BIN="nemoclaw" + ONBOARD_RAN=false + backup_status=0 + run_preupgrade_backup || backup_status=$? + recovery_status=0 + recover_preexisting_sandboxes_before_onboard "${stubBin}" || recovery_status=$? + echo "backup_status=\${backup_status}" + echo "recovery_status=\${recovery_status}" + echo "recovery_ran=\${_PREEXISTING_SANDBOX_RECOVERY_RAN}" + echo "orphaned=\${_PREEXISTING_SANDBOX_ORPHANED}" + print_done 2>&1 + cat "${cliLog}" + `; + + const result = spawnSync("bash", ["-c", snippet], { + encoding: "utf-8", + env: installerTestEnv(tmp), + }); + return { + output: `${result.stdout}\n${result.stderr}`, + cleanup: () => fs.rmSync(tmp, { recursive: true, force: true }), + }; +} + describe("install.sh recovery outcome classification (#6520)", () => { it("marks the run orphaned when the CLI reports sandboxes not found on their recorded gateway", () => { const { output, cleanup } = runRecoveryClassification([ORPHAN_LINE, NO_REBUILD_LINE], 0); @@ -153,6 +211,26 @@ describe("install.sh recovery outcome classification (#6520)", () => { }); }); +describe("install.sh strict-backup recovery handoff", () => { + it("continues to orphan recovery and reports warnings after strict backup succeeds (#6520)", () => { + const { output, cleanup } = runStrictBackupRecoveryFlow(); + try { + expect(output).toContain("backup_status=0"); + expect(output).toContain("recovery_status=0"); + expect(output).toContain("recovery_ran=true"); + expect(output).toContain("orphaned=true"); + expect(output).toContain("=== Installation completed with warnings ==="); + expect(output).toContain("command=backup-all require_all=1"); + expect(output).toContain("command=upgrade-sandboxes --auto require_all="); + expect(output.indexOf("command=backup-all")).toBeLessThan( + output.indexOf("command=upgrade-sandboxes --auto"), + ); + } finally { + cleanup(); + } + }); +}); + describe("install.sh print_done honesty for orphaned sandboxes (#6520)", () => { it("does not claim sandboxes were recovered when recovery skipped them", () => { const { output, cleanup } = runPrintDone({ recoveryRan: "true", orphaned: "true" });