From 7ef4d75f20067f41827f4b70a95afc1d9353dab3 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Tue, 2 Jun 2026 11:02:31 +0800 Subject: [PATCH 1/6] fix(onboard): don't leave a default sandbox when cancelling at policy presets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pressing Ctrl+C at the [8/8] Policy presets step left a fully created OpenShell container registered as the default sandbox with no policies applied. Two things caused the orphan: 1. The sandbox was marked default during the "sandbox" step (in createSandbox and the sandbox state handler) — two steps before the policy prompt — so a cancel at the prompt left it as a valid default. 2. Nothing rolled the freshly-created sandbox back on cancel. Defer default-marking to the finalization step, which only runs once the policy presets are confirmed, and add a cancel-rollback guard that deletes the container and unregisters the sandbox when the operator cancels (Ctrl+C / SIGTERM) at the policy prompt. The guard is a two-key gate (armed after a brand-new createSandbox, fired only on an explicit cancel), so ordinary failure-path exits and recreate/rebuild of existing sandboxes are untouched. Fixes #4614 Signed-off-by: Dongni Yang --- src/lib/onboard.ts | 41 ++++- src/lib/onboard/cancel-rollback.test.ts | 146 ++++++++++++++++++ src/lib/onboard/cancel-rollback.ts | 94 +++++++++++ .../machine/handlers/finalization.test.ts | 10 ++ .../onboard/machine/handlers/finalization.ts | 10 ++ .../onboard/machine/handlers/sandbox.test.ts | 4 +- src/lib/onboard/machine/handlers/sandbox.ts | 4 +- 7 files changed, 302 insertions(+), 7 deletions(-) create mode 100644 src/lib/onboard/cancel-rollback.test.ts create mode 100644 src/lib/onboard/cancel-rollback.ts diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index d77f798837..047854c9b5 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -424,6 +424,7 @@ const { } = modelRouter; const routedInference: typeof import("./onboard/routed-inference") = require("./onboard/routed-inference"); const { OnboardRuntimeBoundary }: typeof import("./onboard/runtime-boundary") = require("./onboard/runtime-boundary"); +const { createSandboxCancelRollback }: typeof import("./onboard/cancel-rollback") = require("./onboard/cancel-rollback"); const { handleAgentSetupState }: typeof import("./onboard/machine/handlers/agent-setup") = require("./onboard/machine/handlers/agent-setup"); const { handleFinalizationState }: typeof import("./onboard/machine/handlers/finalization") = require("./onboard/machine/handlers/finalization"); const { handleGatewayState }: typeof import("./onboard/machine/handlers/gateway") = require("./onboard/machine/handlers/gateway"); @@ -3818,7 +3819,8 @@ async function createSandbox( ...onboardHermesDashboard.getHermesDashboardRegistryFields(finalHermesDashboardState), dashboardPort: actualDashboardPort, }); - registry.setDefault(sandboxName); + // Default is NOT set here — it is deferred to finalization so a cancel at the + // policy-preset step does not leave this sandbox registered as default (#4614). if (restoreBackupPath) { note( @@ -3863,6 +3865,18 @@ async function createSandbox( warnIfLandlockUnsupported({ dockerInfoFormat, runCapture }); + // Arm cancel-rollback: from here until policies are confirmed, an operator + // cancel (Ctrl+C / SIGTERM) at the policy-preset step rolls this sandbox back + // instead of leaving it registered as the default (#4614). + // + // Only arm for brand-new creations. A recreate/rebuild (`--recreate`, drift + // repair, `nemoclaw rebuild`) is replacing a sandbox the operator already + // had, so deleting it on cancel would destroy existing state — for those we + // only defer the default-marking, never auto-delete. + if (!isRecreateSandbox()) { + sandboxCancelRollback.arm(sandboxName); + } + return sandboxName; } @@ -5858,6 +5872,7 @@ async function selectPolicyTier(): Promise { const onSigterm = () => { cleanup(); + sandboxCancelRollback.markCancelled(); process.exit(1); }; process.once("SIGTERM", onSigterm); @@ -5872,6 +5887,7 @@ async function selectPolicyTier(): Promise { redraw(); } else if (key === "\x03") { cleanup(); + sandboxCancelRollback.markCancelled(); process.exit(1); } else if (key === "\x1b[A" || key === "k") { cursor = (cursor - 1 + n) % n; @@ -6199,6 +6215,7 @@ async function presetsCheckboxSelector( const onSigterm = () => { cleanup(); + sandboxCancelRollback.markCancelled(); process.exit(1); }; process.once("SIGTERM", onSigterm); @@ -6211,6 +6228,7 @@ async function presetsCheckboxSelector( } else if (key === "\x03") { // Ctrl+C cleanup(); + sandboxCancelRollback.markCancelled(); process.exit(1); } else if (key === "\x1b[A" || key === "k") { cursor = (cursor - 1 + n) % n; @@ -6302,6 +6320,21 @@ const onboardRuntimeBoundary = new OnboardRuntimeBoundary({ maybeForceE2eStepFailure, }); +// Rolls back a freshly-created sandbox if the operator cancels (Ctrl+C / SIGTERM) +// before the policy-preset step is confirmed. Armed after createSandbox succeeds, +// disarmed once policies are applied; only fires on an explicit cancel, so ordinary +// failure-path exits leave their sandbox untouched (#4614). +const sandboxCancelRollback = createSandboxCancelRollback({ + deleteSandboxContainer: (name) => + runOpenshell(["sandbox", "delete", name], { ignoreError: true }).status === 0, + removeSandboxFromRegistry: (name) => registry.removeSandbox(name), + log: (message) => console.error(message), +}); +// `process.exit()` (how the policy-step prompts terminate on Ctrl+C) synchronously +// emits 'exit', and runOpenshell/removeSandbox are synchronous, so the rollback +// completes inside the handler. No-op unless armed AND cancelled. +process.on("exit", () => sandboxCancelRollback.runIfArmed()); + const startRecordedStep = onboardRuntimeBoundary.startRecordedStep.bind(onboardRuntimeBoundary); const recordStepComplete = onboardRuntimeBoundary.recordStepComplete.bind(onboardRuntimeBoundary); const recordStepSkipped = onboardRuntimeBoundary.recordStepSkipped.bind(onboardRuntimeBoundary); @@ -6927,7 +6960,6 @@ async function onboard(opts: OnboardOptions = {}): Promise { listRegistrySandboxes: registry.listSandboxes, createSandbox, updateSandboxRegistry: (name, updates) => registry.updateSandbox(name, updates), - setDefaultSandbox: registry.setDefault, getSandboxAgentRegistryFields, recordStepComplete, toSessionUpdates: (updates) => toSessionUpdates(updates as Parameters[0]), @@ -7018,6 +7050,10 @@ async function onboard(opts: OnboardOptions = {}): Promise { }); session = policiesResult.session; + // Policies are confirmed — the sandbox is no longer in the cancellable + // window, so disarm the cancel-rollback before finalization (#4614). + sandboxCancelRollback.disarm(); + await handleFinalizationState({ sandboxName, model, @@ -7031,6 +7067,7 @@ async function onboard(opts: OnboardOptions = {}): Promise { webSearchEnabled: braveProviderProfile.shouldEnableBraveWebSearch(webSearchConfig), deps: { ensureAgentDashboardForward, + setDefaultSandbox: registry.setDefault, verifyWebSearchInsideSandbox, recordPostVerifyStarted, recordSessionComplete, diff --git a/src/lib/onboard/cancel-rollback.test.ts b/src/lib/onboard/cancel-rollback.test.ts new file mode 100644 index 0000000000..6230532d4b --- /dev/null +++ b/src/lib/onboard/cancel-rollback.test.ts @@ -0,0 +1,146 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it, vi } from "vitest"; + +import { + buildCancelRollbackMessage, + createSandboxCancelRollback, + type SandboxCancelRollbackDeps, +} from "./cancel-rollback"; + +function createDeps(overrides: Partial = {}) { + const calls = { + deleteContainer: vi.fn((_name: string) => true), + removeFromRegistry: vi.fn(), + log: vi.fn(), + }; + const deps: SandboxCancelRollbackDeps = { + deleteSandboxContainer: calls.deleteContainer, + removeSandboxFromRegistry: calls.removeFromRegistry, + log: calls.log, + ...overrides, + }; + return { calls, deps }; +} + +describe("createSandboxCancelRollback", () => { + it("rolls back (delete + unregister) when armed and cancelled", () => { + const { deps, calls } = createDeps(); + const rollback = createSandboxCancelRollback(deps); + + rollback.arm("new-sb"); + rollback.markCancelled(); + rollback.runIfArmed(); + + expect(calls.deleteContainer).toHaveBeenCalledWith("new-sb"); + expect(calls.removeFromRegistry).toHaveBeenCalledWith("new-sb"); + // delete is attempted before the registry entry is removed + expect(calls.deleteContainer.mock.invocationCallOrder[0]).toBeLessThan( + calls.removeFromRegistry.mock.invocationCallOrder[0], + ); + expect(calls.log).toHaveBeenCalledWith(expect.stringContaining("removed incomplete sandbox 'new-sb'")); + }); + + it("still unregisters and prints manual cleanup when container delete fails", () => { + const { deps, calls } = createDeps({ deleteSandboxContainer: vi.fn(() => false) }); + const rollback = createSandboxCancelRollback(deps); + + rollback.arm("new-sb"); + rollback.markCancelled(); + rollback.runIfArmed(); + + expect(calls.removeFromRegistry).toHaveBeenCalledWith("new-sb"); + expect(calls.log).toHaveBeenCalledWith(expect.stringContaining("unregistered incomplete sandbox 'new-sb'")); + expect(calls.log).toHaveBeenCalledWith(expect.stringContaining('openshell sandbox delete "new-sb"')); + }); + + it("does NOT roll back on a non-cancel exit (armed but not cancelled)", () => { + const { deps, calls } = createDeps(); + const rollback = createSandboxCancelRollback(deps); + + rollback.arm("new-sb"); + // no markCancelled() — this is an ordinary failure-path process.exit + rollback.runIfArmed(); + + expect(calls.deleteContainer).not.toHaveBeenCalled(); + expect(calls.removeFromRegistry).not.toHaveBeenCalled(); + expect(calls.log).not.toHaveBeenCalled(); + }); + + it("does NOT roll back when cancelled before any sandbox was armed", () => { + const { deps, calls } = createDeps(); + const rollback = createSandboxCancelRollback(deps); + + rollback.markCancelled(); + rollback.runIfArmed(); + + expect(calls.deleteContainer).not.toHaveBeenCalled(); + expect(calls.removeFromRegistry).not.toHaveBeenCalled(); + }); + + it("does NOT roll back after disarm (policies confirmed), even if later cancelled", () => { + const { deps, calls } = createDeps(); + const rollback = createSandboxCancelRollback(deps); + + rollback.arm("new-sb"); + rollback.disarm(); + rollback.markCancelled(); + rollback.runIfArmed(); + + expect(calls.deleteContainer).not.toHaveBeenCalled(); + expect(calls.removeFromRegistry).not.toHaveBeenCalled(); + }); + + it("is idempotent — runs the rollback at most once", () => { + const { deps, calls } = createDeps(); + const rollback = createSandboxCancelRollback(deps); + + rollback.arm("new-sb"); + rollback.markCancelled(); + rollback.runIfArmed(); + rollback.runIfArmed(); + rollback.runIfArmed(); + + expect(calls.deleteContainer).toHaveBeenCalledTimes(1); + expect(calls.removeFromRegistry).toHaveBeenCalledTimes(1); + }); + + it("reports armed state via isArmed()", () => { + const { deps } = createDeps(); + const rollback = createSandboxCancelRollback(deps); + + expect(rollback.isArmed()).toBe(false); + rollback.arm("new-sb"); + expect(rollback.isArmed()).toBe(true); + rollback.disarm(); + expect(rollback.isArmed()).toBe(false); + }); + + it("re-arming after a previous sandbox tracks the latest name", () => { + const { deps, calls } = createDeps(); + const rollback = createSandboxCancelRollback(deps); + + rollback.arm("first"); + rollback.arm("second"); + rollback.markCancelled(); + rollback.runIfArmed(); + + expect(calls.deleteContainer).toHaveBeenCalledWith("second"); + expect(calls.deleteContainer).not.toHaveBeenCalledWith("first"); + }); +}); + +describe("buildCancelRollbackMessage", () => { + it("reports a clean removal when the delete succeeded", () => { + const lines = buildCancelRollbackMessage("sb", true); + expect(lines.join("\n")).toContain("removed incomplete sandbox 'sb'"); + expect(lines.join("\n")).not.toContain("openshell sandbox delete"); + }); + + it("falls back to manual cleanup guidance when the delete failed", () => { + const lines = buildCancelRollbackMessage("sb", false); + expect(lines.join("\n")).toContain("unregistered incomplete sandbox 'sb'"); + expect(lines.join("\n")).toContain('openshell sandbox delete "sb"'); + }); +}); diff --git a/src/lib/onboard/cancel-rollback.ts b/src/lib/onboard/cancel-rollback.ts new file mode 100644 index 0000000000..505d4cee41 --- /dev/null +++ b/src/lib/onboard/cancel-rollback.ts @@ -0,0 +1,94 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Rollback guard for a sandbox that was created during onboarding but whose + * onboarding was cancelled before the policy-preset step was confirmed. + * + * Without this, pressing Ctrl+C at the `[8/8] Policy presets` screen leaves a + * fully created OpenShell container registered as the default sandbox even + * though no policies were ever applied (#4614). + * + * The guard is deliberately a two-key gate — it only fires when BOTH: + * - a freshly-created sandbox is `arm()`ed (set after createSandbox succeeds), AND + * - the operator actually cancelled via `markCancelled()` (the policy-step + * prompts call this on Ctrl+C / SIGTERM before exiting). + * + * This keeps every other `process.exit(1)` failure path untouched: a genuine + * build/verify failure exits without `markCancelled()`, so the sandbox it left + * behind is preserved exactly as before. Only an explicit cancel rolls back. + */ +export interface SandboxCancelRollbackDeps { + /** Delete the OpenShell sandbox container. Returns true when the delete succeeded. */ + deleteSandboxContainer(sandboxName: string): boolean; + /** Remove the sandbox entry from the NemoClaw registry (clears default). */ + removeSandboxFromRegistry(sandboxName: string): void; + /** Emit an operator-facing line (stderr). */ + log(message: string): void; +} + +export interface SandboxCancelRollback { + /** Arm rollback for a just-created sandbox. */ + arm(sandboxName: string): void; + /** Disarm once the sandbox is past the cancellable window (policies confirmed). */ + disarm(): void; + /** Record that the operator cancelled at a cancellable step. */ + markCancelled(): void; + /** Run the rollback iff armed AND cancelled. Idempotent. */ + runIfArmed(): void; + /** Test/introspection helper. */ + isArmed(): boolean; +} + +export function buildCancelRollbackMessage(sandboxName: string, deleteSucceeded: boolean): string[] { + if (deleteSucceeded) { + return [ + "", + ` Onboarding cancelled — removed incomplete sandbox '${sandboxName}' (no policy presets were applied).`, + ]; + } + return [ + "", + ` Onboarding cancelled — unregistered incomplete sandbox '${sandboxName}'.`, + " The sandbox container may still be running. Remove it with:", + ` openshell sandbox delete "${sandboxName}"`, + ]; +} + +export function createSandboxCancelRollback( + deps: SandboxCancelRollbackDeps, +): SandboxCancelRollback { + let armedSandboxName: string | null = null; + let cancelRequested = false; + let done = false; + + return { + arm(sandboxName: string): void { + armedSandboxName = sandboxName; + }, + disarm(): void { + armedSandboxName = null; + }, + markCancelled(): void { + cancelRequested = true; + }, + isArmed(): boolean { + return armedSandboxName !== null; + }, + runIfArmed(): void { + if (done || !cancelRequested || armedSandboxName === null) return; + done = true; + const sandboxName = armedSandboxName; + armedSandboxName = null; + + // Delete the container first, then unregister regardless of the delete + // result — leaving a registry entry pointing at a half-built sandbox is + // worse than an orphaned container the operator can clean up manually. + const deleteSucceeded = deps.deleteSandboxContainer(sandboxName); + deps.removeSandboxFromRegistry(sandboxName); + for (const line of buildCancelRollbackMessage(sandboxName, deleteSucceeded)) { + deps.log(line); + } + }, + }; +} diff --git a/src/lib/onboard/machine/handlers/finalization.test.ts b/src/lib/onboard/machine/handlers/finalization.test.ts index df6000b2e6..aa6e6b2ad8 100644 --- a/src/lib/onboard/machine/handlers/finalization.test.ts +++ b/src/lib/onboard/machine/handlers/finalization.test.ts @@ -12,6 +12,7 @@ type VerificationResult = { ok: boolean }; function createDeps(overrides: Partial["deps"]> = {}) { const calls = { + setDefaultSandbox: vi.fn(), ensureAgentDashboard: vi.fn(() => 18789), postVerify: vi.fn(async () => createSession({ machine: { version: 1, state: "post_verify", stateEnteredAt: null, revision: 1 } })), complete: vi.fn(async () => createSession({ status: "complete" })), @@ -31,6 +32,7 @@ function createDeps(overrides: Partial) => updates as SessionUpdates, @@ -74,6 +76,11 @@ describe("handleFinalizationState", () => { const result = await handleFinalizationState(baseOptions(deps)); + // Default is set at finalization (deferred from sandbox creation, #4614), before completion. + expect(calls.setDefaultSandbox).toHaveBeenCalledWith("my-assistant"); + expect(calls.setDefaultSandbox.mock.invocationCallOrder[0]).toBeLessThan( + calls.complete.mock.invocationCallOrder[0], + ); expect(calls.cleanupHost).toHaveBeenCalledOnce(); expect(calls.recoverProcesses).toHaveBeenCalledWith("my-assistant", { quiet: true }); expect(calls.buildChain).toHaveBeenCalledWith("http://127.0.0.1:18789"); @@ -117,6 +124,9 @@ describe("handleFinalizationState", () => { expect(calls.postVerify).toHaveBeenCalledOnce(); expect(calls.complete).not.toHaveBeenCalled(); expect(calls.dashboard).not.toHaveBeenCalled(); + // The sandbox reached finalization (policies confirmed), so it stays the default + // even when post-policy verification flakes — only a pre-policy cancel rolls back. + expect(calls.setDefaultSandbox).toHaveBeenCalledWith("my-assistant"); }); it("removes legacy credentials only when all staged values migrated", async () => { diff --git a/src/lib/onboard/machine/handlers/finalization.ts b/src/lib/onboard/machine/handlers/finalization.ts index 34e2dba224..760cdf1561 100644 --- a/src/lib/onboard/machine/handlers/finalization.ts +++ b/src/lib/onboard/machine/handlers/finalization.ts @@ -16,6 +16,12 @@ export interface FinalizationStateOptions): number; + /** + * Mark this sandbox as the default. Called here (not at sandbox creation) so + * a cancel at the policy-preset step never leaves an unconfigured sandbox + * registered as default (#4614). + */ + setDefaultSandbox(sandboxName: string): void; recordPostVerifyStarted(): Promise; recordSessionComplete(updates: SessionUpdates): Promise; toSessionUpdates(updates: Record): SessionUpdates; @@ -64,6 +70,10 @@ export async function handleFinalizationState): Promise { + // Reaching finalization means the policy-preset step was confirmed, so it is + // now safe to register this sandbox as the default (#4614). + deps.setDefaultSandbox(sandboxName); + if (agent) deps.ensureAgentDashboardForward(sandboxName, agent as NonNullable); const allStagedMigrated = diff --git a/src/lib/onboard/machine/handlers/sandbox.test.ts b/src/lib/onboard/machine/handlers/sandbox.test.ts index 52cf8a6db2..bd8ecade50 100644 --- a/src/lib/onboard/machine/handlers/sandbox.test.ts +++ b/src/lib/onboard/machine/handlers/sandbox.test.ts @@ -35,7 +35,6 @@ function createDeps(overrides: Partial "my-assistant"), updateSandbox: vi.fn(), - setDefault: vi.fn(), complete: vi.fn(async () => createSession()), skipped: vi.fn(), recordSkip: vi.fn(async () => createSession()), @@ -79,7 +78,6 @@ function createDeps(overrides: Partial ({ sandboxes: [{ name: "old" }] }), createSandbox: calls.createSandbox, updateSandboxRegistry: calls.updateSandbox, - setDefaultSandbox: calls.setDefault, getSandboxAgentRegistryFields: () => ({ agent: null }), recordStepComplete: calls.complete, toSessionUpdates: (updates: Record) => updates as SessionUpdates, @@ -150,7 +148,7 @@ describe("handleSandboxState", () => { [], ); expect(calls.updateSandbox).toHaveBeenCalledWith("my-assistant", expect.objectContaining({ model: "model", provider: "provider" })); - expect(calls.setDefault).toHaveBeenCalledWith("my-assistant"); + // Default-marking is deferred to finalization (#4614) — the sandbox step must not set it. expect(calls.complete).toHaveBeenCalledWith("sandbox", expect.objectContaining({ sandboxName: "my-assistant" })); expect(result).toMatchObject({ sandboxName: "my-assistant", selectedMessagingChannels: ["telegram"], webSearchSupported: true }); }); diff --git a/src/lib/onboard/machine/handlers/sandbox.ts b/src/lib/onboard/machine/handlers/sandbox.ts index efa5cf0adb..9a91ba6360 100644 --- a/src/lib/onboard/machine/handlers/sandbox.ts +++ b/src/lib/onboard/machine/handlers/sandbox.ts @@ -77,7 +77,6 @@ export interface SandboxStateOptions; updateSandboxRegistry(sandboxName: string, updates: Record): void; - setDefaultSandbox(sandboxName: string): void; getSandboxAgentRegistryFields(agent: Agent, agentVersionKnown: boolean): Record; recordStepComplete(stepName: string, updates: SessionUpdates): Promise; toSessionUpdates(updates: Record): SessionUpdates; @@ -307,7 +306,8 @@ export async function handleSandboxState Date: Tue, 2 Jun 2026 11:03:23 +0800 Subject: [PATCH 2/6] fix(onboard): preserve the default flag across a recreate/rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deferring default-marking to finalization (the #4614 fix) introduced a failure-path gap for recreate/rebuild: createSandbox removes the existing registry entry (clearing the default pointer) and re-registers it without a default, so a rebuild that fails before finalization left the operator's previously-default sandbox no longer marked default until a successful re-run. Snapshot whether the sandbox is the default at createSandbox entry — before the recreate tears it down — and restore that flag right after re-registration. A brand-new sandbox was never the default, so it stays deferred and the #4614 behaviour is unchanged; only a recreate of an already-default sandbox restores it eagerly. Refs #4614 Signed-off-by: Dongni Yang --- src/lib/onboard.ts | 13 +++++- src/lib/onboard/default-preservation.test.ts | 48 ++++++++++++++++++++ src/lib/onboard/default-preservation.ts | 36 +++++++++++++++ 3 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 src/lib/onboard/default-preservation.test.ts create mode 100644 src/lib/onboard/default-preservation.ts diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 047854c9b5..c49be8b45d 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -425,6 +425,7 @@ const { const routedInference: typeof import("./onboard/routed-inference") = require("./onboard/routed-inference"); const { OnboardRuntimeBoundary }: typeof import("./onboard/runtime-boundary") = require("./onboard/runtime-boundary"); const { createSandboxCancelRollback }: typeof import("./onboard/cancel-rollback") = require("./onboard/cancel-rollback"); +const { wasSandboxDefault, restoreDefaultAfterRecreate }: typeof import("./onboard/default-preservation") = require("./onboard/default-preservation"); const { handleAgentSetupState }: typeof import("./onboard/machine/handlers/agent-setup") = require("./onboard/machine/handlers/agent-setup"); const { handleFinalizationState }: typeof import("./onboard/machine/handlers/finalization") = require("./onboard/machine/handlers/finalization"); const { handleGatewayState }: typeof import("./onboard/machine/handlers/gateway") = require("./onboard/machine/handlers/gateway"); @@ -2783,6 +2784,10 @@ async function createSandbox( sandboxNameOverride ?? (await promptValidatedSandboxName(agent)), "sandbox name", ); + // Snapshot whether this sandbox is the default before a recreate tears down + // its registry entry, so a rebuild that fails before finalization does not + // silently drop the default flag (#4614). + const sandboxWasDefaultBeforeCreate = wasSandboxDefault(registry.getDefault(), sandboxName); enabledChannels = filterEnabledChannelsByAgent(enabledChannels, agent); const effectiveSandboxGpuConfig = sandboxGpuConfig ?? resolveSandboxGpuConfig(gpu, { flag: null, device: null }); @@ -3819,8 +3824,12 @@ async function createSandbox( ...onboardHermesDashboard.getHermesDashboardRegistryFields(finalHermesDashboardState), dashboardPort: actualDashboardPort, }); - // Default is NOT set here — it is deferred to finalization so a cancel at the - // policy-preset step does not leave this sandbox registered as default (#4614). + // Default is NOT set here for a brand-new sandbox — it is deferred to + // finalization so a cancel at the policy-preset step does not leave this + // sandbox registered as default (#4614). The one exception: a recreate/rebuild + // of a sandbox that was already the default restores that flag now, so a + // rebuild failure before finalization does not silently demote it. + restoreDefaultAfterRecreate(registry.setDefault, sandboxName, sandboxWasDefaultBeforeCreate); if (restoreBackupPath) { note( diff --git a/src/lib/onboard/default-preservation.test.ts b/src/lib/onboard/default-preservation.test.ts new file mode 100644 index 0000000000..75d870807e --- /dev/null +++ b/src/lib/onboard/default-preservation.test.ts @@ -0,0 +1,48 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it, vi } from "vitest"; + +import { restoreDefaultAfterRecreate, wasSandboxDefault } from "./default-preservation"; + +describe("wasSandboxDefault", () => { + it("is true when the sandbox is the current default", () => { + expect(wasSandboxDefault("my-sb", "my-sb")).toBe(true); + }); + + it("is false when a different sandbox is the default", () => { + expect(wasSandboxDefault("other-sb", "my-sb")).toBe(false); + }); + + it("is false when there is no default", () => { + expect(wasSandboxDefault(null, "my-sb")).toBe(false); + }); +}); + +describe("restoreDefaultAfterRecreate", () => { + it("re-applies the default when the sandbox held it before recreate", () => { + const setDefault = vi.fn(); + restoreDefaultAfterRecreate(setDefault, "my-sb", true); + expect(setDefault).toHaveBeenCalledWith("my-sb"); + }); + + it("does NOT touch the default when the sandbox was not default (e.g. a brand-new sandbox)", () => { + const setDefault = vi.fn(); + restoreDefaultAfterRecreate(setDefault, "new-sb", false); + expect(setDefault).not.toHaveBeenCalled(); + }); + + it("round-trips: a recreate of the prior default restores it", () => { + const setDefault = vi.fn(); + const wasDefault = wasSandboxDefault("my-sb", "my-sb"); + restoreDefaultAfterRecreate(setDefault, "my-sb", wasDefault); + expect(setDefault).toHaveBeenCalledWith("my-sb"); + }); + + it("round-trips: a recreate of a non-default sandbox leaves the default alone", () => { + const setDefault = vi.fn(); + const wasDefault = wasSandboxDefault("other-sb", "my-sb"); + restoreDefaultAfterRecreate(setDefault, "my-sb", wasDefault); + expect(setDefault).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/onboard/default-preservation.ts b/src/lib/onboard/default-preservation.ts new file mode 100644 index 0000000000..0b9b30a9f8 --- /dev/null +++ b/src/lib/onboard/default-preservation.ts @@ -0,0 +1,36 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Preserve the "default sandbox" flag across a destructive recreate/rebuild. + * + * Default-marking is normally deferred to the finalization step so a cancelled + * fresh onboard never leaves an unconfigured sandbox registered as default + * (#4614). But a recreate/rebuild first removes the existing registry entry + * (which clears the default pointer) and re-registers it without a default. + * If the rebuild then fails before finalization, the sandbox the operator was + * already using would silently stop being the default until a successful re-run. + * + * These helpers snapshot whether the sandbox was the default *before* it is torn + * down, and restore that flag immediately after it is re-registered — so only a + * genuinely new sandbox (which was never the default) stays deferred. + */ + +/** True iff `sandboxName` is the current default — capture this before recreate tears it down. */ +export function wasSandboxDefault( + currentDefault: string | null, + sandboxName: string, +): boolean { + return currentDefault === sandboxName; +} + +/** Re-apply the default flag after re-registration iff the sandbox held it beforehand. */ +export function restoreDefaultAfterRecreate( + setDefault: (sandboxName: string) => void, + sandboxName: string, + wasDefaultBeforeRecreate: boolean, +): void { + if (wasDefaultBeforeRecreate) { + setDefault(sandboxName); + } +} From 7b4a91a61a7db896a7edd45d6dfae8ab3cb6d3d8 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Tue, 2 Jun 2026 11:46:58 +0800 Subject: [PATCH 3/6] fix(onboard): arm cancel-rollback off prior existence; keep onboard.ts under budget Addresses CodeRabbit review on #4642: - isRecreateSandbox() only reflects the CLI/env flag, but createSandbox() also recreates existing sandboxes for drift, credential rotation, missing providers, and non-ready state while that flag is false. Arming the cancel-rollback there would delete the rebuilt sandbox on Ctrl+C and wipe an operator's existing sandbox. Arm only when the sandbox did not exist before this run (captureSandboxPriorState().existed), so a cancel can never delete a pre-existing sandbox regardless of why it was being recreated. - Move the process-exit hook and the shared cancel-exit handler behind focused helpers (installSandboxCancelRollback, makeOnboardCancelExit) and collapse the pre-recreate snapshot into captureSandboxPriorState, bringing src/lib/onboard.ts back to net-negative under the codebase-growth guardrail. test/onboard.test.ts: createSandbox no longer marks the sandbox default (that is deferred to finalization), so it records no setDefault call. Refs #4614 Signed-off-by: Dongni Yang --- src/lib/onboard.ts | 70 +++----------------- src/lib/onboard/cancel-rollback.test.ts | 58 ++++++++++++++++ src/lib/onboard/cancel-rollback.ts | 60 +++++++++++++++++ src/lib/onboard/default-preservation.test.ts | 23 ++++++- src/lib/onboard/default-preservation.ts | 22 ++++++ test/onboard.test.ts | 5 +- 6 files changed, 177 insertions(+), 61 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index c49be8b45d..e7de6e90c7 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -424,8 +424,7 @@ const { } = modelRouter; const routedInference: typeof import("./onboard/routed-inference") = require("./onboard/routed-inference"); const { OnboardRuntimeBoundary }: typeof import("./onboard/runtime-boundary") = require("./onboard/runtime-boundary"); -const { createSandboxCancelRollback }: typeof import("./onboard/cancel-rollback") = require("./onboard/cancel-rollback"); -const { wasSandboxDefault, restoreDefaultAfterRecreate }: typeof import("./onboard/default-preservation") = require("./onboard/default-preservation"); +const { installSandboxCancelRollback, makeOnboardCancelExit, captureSandboxPriorState, restoreDefaultAfterRecreate }: typeof import("./onboard/cancel-rollback") = require("./onboard/cancel-rollback"); const { handleAgentSetupState }: typeof import("./onboard/machine/handlers/agent-setup") = require("./onboard/machine/handlers/agent-setup"); const { handleFinalizationState }: typeof import("./onboard/machine/handlers/finalization") = require("./onboard/machine/handlers/finalization"); const { handleGatewayState }: typeof import("./onboard/machine/handlers/gateway") = require("./onboard/machine/handlers/gateway"); @@ -2784,10 +2783,7 @@ async function createSandbox( sandboxNameOverride ?? (await promptValidatedSandboxName(agent)), "sandbox name", ); - // Snapshot whether this sandbox is the default before a recreate tears down - // its registry entry, so a rebuild that fails before finalization does not - // silently drop the default flag (#4614). - const sandboxWasDefaultBeforeCreate = wasSandboxDefault(registry.getDefault(), sandboxName); + const sandboxPrior = captureSandboxPriorState(registry, sandboxName); // #4614: pre-recreate snapshot enabledChannels = filterEnabledChannelsByAgent(enabledChannels, agent); const effectiveSandboxGpuConfig = sandboxGpuConfig ?? resolveSandboxGpuConfig(gpu, { flag: null, device: null }); @@ -3824,12 +3820,7 @@ async function createSandbox( ...onboardHermesDashboard.getHermesDashboardRegistryFields(finalHermesDashboardState), dashboardPort: actualDashboardPort, }); - // Default is NOT set here for a brand-new sandbox — it is deferred to - // finalization so a cancel at the policy-preset step does not leave this - // sandbox registered as default (#4614). The one exception: a recreate/rebuild - // of a sandbox that was already the default restores that flag now, so a - // rebuild failure before finalization does not silently demote it. - restoreDefaultAfterRecreate(registry.setDefault, sandboxName, sandboxWasDefaultBeforeCreate); + restoreDefaultAfterRecreate(registry.setDefault, sandboxName, sandboxPrior.wasDefault); // #4614: default deferred to finalization if (restoreBackupPath) { note( @@ -3874,18 +3865,8 @@ async function createSandbox( warnIfLandlockUnsupported({ dockerInfoFormat, runCapture }); - // Arm cancel-rollback: from here until policies are confirmed, an operator - // cancel (Ctrl+C / SIGTERM) at the policy-preset step rolls this sandbox back - // instead of leaving it registered as the default (#4614). - // - // Only arm for brand-new creations. A recreate/rebuild (`--recreate`, drift - // repair, `nemoclaw rebuild`) is replacing a sandbox the operator already - // had, so deleting it on cancel would destroy existing state — for those we - // only defer the default-marking, never auto-delete. - if (!isRecreateSandbox()) { - sandboxCancelRollback.arm(sandboxName); - } - + // #4614: arm rollback only for a genuinely new sandbox (never a recreate/rebuild). + if (!sandboxPrior.existed) sandboxCancelRollback.arm(sandboxName); return sandboxName; } @@ -5879,11 +5860,7 @@ async function selectPolicyTier(): Promise { process.removeListener("SIGTERM", onSigterm); }; - const onSigterm = () => { - cleanup(); - sandboxCancelRollback.markCancelled(); - process.exit(1); - }; + const onSigterm = makeOnboardCancelExit(sandboxCancelRollback, cleanup); process.once("SIGTERM", onSigterm); const onData = (key: string) => { @@ -5895,9 +5872,7 @@ async function selectPolicyTier(): Promise { selectedIdx = cursor; redraw(); } else if (key === "\x03") { - cleanup(); - sandboxCancelRollback.markCancelled(); - process.exit(1); + makeOnboardCancelExit(sandboxCancelRollback, cleanup)(); } else if (key === "\x1b[A" || key === "k") { cursor = (cursor - 1 + n) % n; redraw(); @@ -6222,11 +6197,7 @@ async function presetsCheckboxSelector( process.removeListener("SIGTERM", onSigterm); }; - const onSigterm = () => { - cleanup(); - sandboxCancelRollback.markCancelled(); - process.exit(1); - }; + const onSigterm = makeOnboardCancelExit(sandboxCancelRollback, cleanup); process.once("SIGTERM", onSigterm); const onData = (key: string) => { @@ -6235,10 +6206,7 @@ async function presetsCheckboxSelector( process.stdout.write("\n"); resolve([...selected]); } else if (key === "\x03") { - // Ctrl+C - cleanup(); - sandboxCancelRollback.markCancelled(); - process.exit(1); + makeOnboardCancelExit(sandboxCancelRollback, cleanup)(); // Ctrl+C } else if (key === "\x1b[A" || key === "k") { cursor = (cursor - 1 + n) % n; redraw(); @@ -6329,20 +6297,7 @@ const onboardRuntimeBoundary = new OnboardRuntimeBoundary({ maybeForceE2eStepFailure, }); -// Rolls back a freshly-created sandbox if the operator cancels (Ctrl+C / SIGTERM) -// before the policy-preset step is confirmed. Armed after createSandbox succeeds, -// disarmed once policies are applied; only fires on an explicit cancel, so ordinary -// failure-path exits leave their sandbox untouched (#4614). -const sandboxCancelRollback = createSandboxCancelRollback({ - deleteSandboxContainer: (name) => - runOpenshell(["sandbox", "delete", name], { ignoreError: true }).status === 0, - removeSandboxFromRegistry: (name) => registry.removeSandbox(name), - log: (message) => console.error(message), -}); -// `process.exit()` (how the policy-step prompts terminate on Ctrl+C) synchronously -// emits 'exit', and runOpenshell/removeSandbox are synchronous, so the rollback -// completes inside the handler. No-op unless armed AND cancelled. -process.on("exit", () => sandboxCancelRollback.runIfArmed()); +const sandboxCancelRollback = installSandboxCancelRollback({ runOpenshell, registry }); // #4614 const startRecordedStep = onboardRuntimeBoundary.startRecordedStep.bind(onboardRuntimeBoundary); const recordStepComplete = onboardRuntimeBoundary.recordStepComplete.bind(onboardRuntimeBoundary); @@ -7058,10 +7013,7 @@ async function onboard(opts: OnboardOptions = {}): Promise { }, }); session = policiesResult.session; - - // Policies are confirmed — the sandbox is no longer in the cancellable - // window, so disarm the cancel-rollback before finalization (#4614). - sandboxCancelRollback.disarm(); + sandboxCancelRollback.disarm(); // #4614: policies confirmed, past the cancellable window await handleFinalizationState({ sandboxName, diff --git a/src/lib/onboard/cancel-rollback.test.ts b/src/lib/onboard/cancel-rollback.test.ts index 6230532d4b..1d1640461f 100644 --- a/src/lib/onboard/cancel-rollback.test.ts +++ b/src/lib/onboard/cancel-rollback.test.ts @@ -6,6 +6,8 @@ import { describe, expect, it, vi } from "vitest"; import { buildCancelRollbackMessage, createSandboxCancelRollback, + installSandboxCancelRollback, + makeOnboardCancelExit, type SandboxCancelRollbackDeps, } from "./cancel-rollback"; @@ -131,6 +133,62 @@ describe("createSandboxCancelRollback", () => { }); }); +describe("installSandboxCancelRollback", () => { + it("wires delete to openshell and unregister to the registry, and registers an exit hook", () => { + const runOpenshell = vi.fn(() => ({ status: 0 })); + const removeSandbox = vi.fn(); + const exitHandlers: Array<() => void> = []; + + const rollback = installSandboxCancelRollback({ + runOpenshell, + registry: { removeSandbox }, + registerExitHandler: (h) => exitHandlers.push(h), + }); + + expect(exitHandlers).toHaveLength(1); + + rollback.arm("new-sb"); + rollback.markCancelled(); + exitHandlers[0](); + + expect(runOpenshell).toHaveBeenCalledWith(["sandbox", "delete", "new-sb"], { ignoreError: true }); + expect(removeSandbox).toHaveBeenCalledWith("new-sb"); + }); + + it("does not fire the rollback on a non-cancel exit", () => { + const runOpenshell = vi.fn(() => ({ status: 0 })); + const removeSandbox = vi.fn(); + const exitHandlers: Array<() => void> = []; + + const rollback = installSandboxCancelRollback({ + runOpenshell, + registry: { removeSandbox }, + registerExitHandler: (h) => exitHandlers.push(h), + }); + rollback.arm("new-sb"); // armed, but never cancelled + exitHandlers[0](); + + expect(runOpenshell).not.toHaveBeenCalled(); + expect(removeSandbox).not.toHaveBeenCalled(); + }); +}); + +describe("makeOnboardCancelExit", () => { + it("cleans up, marks cancelled, then exits non-zero", () => { + const order: string[] = []; + const cleanup = vi.fn(() => order.push("cleanup")); + const rollback = { markCancelled: vi.fn(() => order.push("markCancelled")) }; + const exit = vi.fn((_code: number) => { + order.push("exit"); + }); + + makeOnboardCancelExit(rollback, cleanup, exit)(); + + expect(order).toEqual(["cleanup", "markCancelled", "exit"]); + expect(exit).toHaveBeenCalledWith(1); + }); +}); + describe("buildCancelRollbackMessage", () => { it("reports a clean removal when the delete succeeded", () => { const lines = buildCancelRollbackMessage("sb", true); diff --git a/src/lib/onboard/cancel-rollback.ts b/src/lib/onboard/cancel-rollback.ts index 505d4cee41..c137764057 100644 --- a/src/lib/onboard/cancel-rollback.ts +++ b/src/lib/onboard/cancel-rollback.ts @@ -1,6 +1,14 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +// Re-exported so the onboard entrypoint imports its sandbox default/cancel +// lifecycle helpers from a single module. +export { + captureSandboxPriorState, + restoreDefaultAfterRecreate, + wasSandboxDefault, +} from "./default-preservation"; + /** * Rollback guard for a sandbox that was created during onboarding but whose * onboarding was cancelled before the policy-preset step was confirmed. @@ -55,6 +63,58 @@ export function buildCancelRollbackMessage(sandboxName: string, deleteSucceeded: ]; } +export interface InstallSandboxCancelRollbackOptions { + runOpenshell: (args: string[], opts: { ignoreError: boolean }) => { status: number | null }; + registry: { removeSandbox(name: string): void }; + log?: (message: string) => void; + /** Override for tests; defaults to `process.on("exit", ...)`. */ + registerExitHandler?: (handler: () => void) => void; +} + +/** + * Wire a sandbox cancel-rollback to OpenShell + the registry and register the + * process-exit hook that fires it. Kept here (not in onboard.ts) so the + * orchestration lives in a focused module rather than the onboard entrypoint. + * + * `process.exit()` — how the policy-step prompts terminate on Ctrl+C — + * synchronously emits 'exit', and runOpenshell/removeSandbox are synchronous, + * so the rollback completes inside the handler. No-op unless armed AND cancelled. + */ +export function installSandboxCancelRollback( + opts: InstallSandboxCancelRollbackOptions, +): SandboxCancelRollback { + const rollback = createSandboxCancelRollback({ + deleteSandboxContainer: (name) => + opts.runOpenshell(["sandbox", "delete", name], { ignoreError: true }).status === 0, + removeSandboxFromRegistry: (name) => opts.registry.removeSandbox(name), + log: opts.log ?? ((message) => console.error(message)), + }); + const register = + opts.registerExitHandler ?? + ((handler: () => void) => { + process.on("exit", handler); + }); + register(() => rollback.runIfArmed()); + return rollback; +} + +/** + * Build the cancel handler the policy-step prompts run on Ctrl+C / SIGTERM: + * restore the terminal (`cleanup`), record the cancel, then exit non-zero. + * Shared so both the tier and preset selectors stay in sync. + */ +export function makeOnboardCancelExit( + rollback: Pick, + cleanup: () => void, + exit: (code: number) => void = (code) => process.exit(code), +): () => void { + return () => { + cleanup(); + rollback.markCancelled(); + exit(1); + }; +} + export function createSandboxCancelRollback( deps: SandboxCancelRollbackDeps, ): SandboxCancelRollback { diff --git a/src/lib/onboard/default-preservation.test.ts b/src/lib/onboard/default-preservation.test.ts index 75d870807e..e5128d7da8 100644 --- a/src/lib/onboard/default-preservation.test.ts +++ b/src/lib/onboard/default-preservation.test.ts @@ -3,7 +3,28 @@ import { describe, expect, it, vi } from "vitest"; -import { restoreDefaultAfterRecreate, wasSandboxDefault } from "./default-preservation"; +import { + captureSandboxPriorState, + restoreDefaultAfterRecreate, + wasSandboxDefault, +} from "./default-preservation"; + +describe("captureSandboxPriorState", () => { + it("reports a brand-new sandbox as neither existing nor default", () => { + const registry = { getSandbox: () => null, getDefault: () => "other" }; + expect(captureSandboxPriorState(registry, "new-sb")).toEqual({ existed: false, wasDefault: false }); + }); + + it("reports an existing default sandbox as existed + wasDefault", () => { + const registry = { getSandbox: () => ({ name: "sb" }), getDefault: () => "sb" }; + expect(captureSandboxPriorState(registry, "sb")).toEqual({ existed: true, wasDefault: true }); + }); + + it("reports an existing non-default sandbox as existed but not default", () => { + const registry = { getSandbox: () => ({ name: "sb" }), getDefault: () => "other" }; + expect(captureSandboxPriorState(registry, "sb")).toEqual({ existed: true, wasDefault: false }); + }); +}); describe("wasSandboxDefault", () => { it("is true when the sandbox is the current default", () => { diff --git a/src/lib/onboard/default-preservation.ts b/src/lib/onboard/default-preservation.ts index 0b9b30a9f8..7c0073103c 100644 --- a/src/lib/onboard/default-preservation.ts +++ b/src/lib/onboard/default-preservation.ts @@ -24,6 +24,28 @@ export function wasSandboxDefault( return currentDefault === sandboxName; } +export interface SandboxPriorState { + /** Whether the sandbox was already registered before this create run (any recreate path). */ + existed: boolean; + /** Whether it was the default before this create run. */ + wasDefault: boolean; +} + +/** + * Snapshot a sandbox's registry state at the start of createSandbox, before any + * recreate path tears its entry down. `existed` gates cancel-rollback arming (only + * arm genuinely new sandboxes); `wasDefault` drives {@link restoreDefaultAfterRecreate}. + */ +export function captureSandboxPriorState( + registry: { getSandbox(name: string): unknown; getDefault(): string | null }, + sandboxName: string, +): SandboxPriorState { + return { + existed: registry.getSandbox(sandboxName) != null, + wasDefault: wasSandboxDefault(registry.getDefault(), sandboxName), + }; +} + /** Re-apply the default flag after re-registration iff the sandbox held it beforehand. */ export function restoreDefaultAfterRecreate( setDefault: (sandboxName: string) => void, diff --git a/test/onboard.test.ts b/test/onboard.test.ts index 5cc6513fd0..3e2be27754 100644 --- a/test/onboard.test.ts +++ b/test/onboard.test.ts @@ -2357,7 +2357,10 @@ const { createSandbox } = require(${onboardPath}); assert.ok(payloadLine, `expected JSON payload in stdout:\n${result.stdout}`); const payload = JSON.parse(payloadLine); assert.equal(payload.sandboxName, "my-assistant"); - assert.deepEqual(payload.defaultCalls, ["my-assistant"]); + // createSandbox no longer marks the sandbox default — that is deferred to the + // finalization step so a cancel at policy presets can't leave an unconfigured + // sandbox as default (#4614). + assert.deepEqual(payload.defaultCalls, []); assert.ok( payload.registerCalls.some( (entry: Record) => From b489b887bb3bc414f10b36a0c82f812580c54c37 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Tue, 2 Jun 2026 13:51:51 +0800 Subject: [PATCH 4/6] fix(onboard): base rollback/default decisions on post-prune live existence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on #4642: captureSandboxPriorState() read registry state before pruneStaleSandboxEntry() ran, so a stale registry row was treated as an existing sandbox. That let a brand-new sandbox (with a leftover stale entry) be marked default immediately and skip rollback arming — reintroducing the bug for that case. Use the post-prune `liveExists` signal instead: arm cancel-rollback when the sandbox was not live before this run, and only preserve the default when it was a genuinely live default (`liveExists && wasSandboxDefault(...)`). Drop the pre-prune captureSandboxPriorState helper. Refs #4614 Signed-off-by: Dongni Yang --- src/lib/onboard.ts | 11 +++++----- src/lib/onboard/cancel-rollback.ts | 6 +---- src/lib/onboard/default-preservation.test.ts | 23 +------------------- src/lib/onboard/default-preservation.ts | 22 ------------------- 4 files changed, 8 insertions(+), 54 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index e7de6e90c7..662021d614 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -424,7 +424,7 @@ const { } = modelRouter; const routedInference: typeof import("./onboard/routed-inference") = require("./onboard/routed-inference"); const { OnboardRuntimeBoundary }: typeof import("./onboard/runtime-boundary") = require("./onboard/runtime-boundary"); -const { installSandboxCancelRollback, makeOnboardCancelExit, captureSandboxPriorState, restoreDefaultAfterRecreate }: typeof import("./onboard/cancel-rollback") = require("./onboard/cancel-rollback"); +const { installSandboxCancelRollback, makeOnboardCancelExit, wasSandboxDefault, restoreDefaultAfterRecreate }: typeof import("./onboard/cancel-rollback") = require("./onboard/cancel-rollback"); const { handleAgentSetupState }: typeof import("./onboard/machine/handlers/agent-setup") = require("./onboard/machine/handlers/agent-setup"); const { handleFinalizationState }: typeof import("./onboard/machine/handlers/finalization") = require("./onboard/machine/handlers/finalization"); const { handleGatewayState }: typeof import("./onboard/machine/handlers/gateway") = require("./onboard/machine/handlers/gateway"); @@ -2783,7 +2783,6 @@ async function createSandbox( sandboxNameOverride ?? (await promptValidatedSandboxName(agent)), "sandbox name", ); - const sandboxPrior = captureSandboxPriorState(registry, sandboxName); // #4614: pre-recreate snapshot enabledChannels = filterEnabledChannelsByAgent(enabledChannels, agent); const effectiveSandboxGpuConfig = sandboxGpuConfig ?? resolveSandboxGpuConfig(gpu, { flag: null, device: null }); @@ -2986,6 +2985,8 @@ async function createSandbox( // Reconcile local registry state with the live OpenShell gateway state. const liveExists = pruneStaleSandboxEntry(sandboxName); + // #4614: capture default AFTER prune so a stale registry row isn't read as a live sandbox. + const sandboxWasLiveDefault = liveExists && wasSandboxDefault(registry.getDefault(), sandboxName); // Declared outside the liveExists block so it is accessible during // post-creation restore (the sandbox create path runs after the block). @@ -3820,7 +3821,7 @@ async function createSandbox( ...onboardHermesDashboard.getHermesDashboardRegistryFields(finalHermesDashboardState), dashboardPort: actualDashboardPort, }); - restoreDefaultAfterRecreate(registry.setDefault, sandboxName, sandboxPrior.wasDefault); // #4614: default deferred to finalization + restoreDefaultAfterRecreate(registry.setDefault, sandboxName, sandboxWasLiveDefault); // #4614: default deferred to finalization if (restoreBackupPath) { note( @@ -3865,8 +3866,8 @@ async function createSandbox( warnIfLandlockUnsupported({ dockerInfoFormat, runCapture }); - // #4614: arm rollback only for a genuinely new sandbox (never a recreate/rebuild). - if (!sandboxPrior.existed) sandboxCancelRollback.arm(sandboxName); + // #4614: arm rollback only when the sandbox was not live before (never a recreate/rebuild). + if (!liveExists) sandboxCancelRollback.arm(sandboxName); return sandboxName; } diff --git a/src/lib/onboard/cancel-rollback.ts b/src/lib/onboard/cancel-rollback.ts index c137764057..dabb502e93 100644 --- a/src/lib/onboard/cancel-rollback.ts +++ b/src/lib/onboard/cancel-rollback.ts @@ -3,11 +3,7 @@ // Re-exported so the onboard entrypoint imports its sandbox default/cancel // lifecycle helpers from a single module. -export { - captureSandboxPriorState, - restoreDefaultAfterRecreate, - wasSandboxDefault, -} from "./default-preservation"; +export { restoreDefaultAfterRecreate, wasSandboxDefault } from "./default-preservation"; /** * Rollback guard for a sandbox that was created during onboarding but whose diff --git a/src/lib/onboard/default-preservation.test.ts b/src/lib/onboard/default-preservation.test.ts index e5128d7da8..75d870807e 100644 --- a/src/lib/onboard/default-preservation.test.ts +++ b/src/lib/onboard/default-preservation.test.ts @@ -3,28 +3,7 @@ import { describe, expect, it, vi } from "vitest"; -import { - captureSandboxPriorState, - restoreDefaultAfterRecreate, - wasSandboxDefault, -} from "./default-preservation"; - -describe("captureSandboxPriorState", () => { - it("reports a brand-new sandbox as neither existing nor default", () => { - const registry = { getSandbox: () => null, getDefault: () => "other" }; - expect(captureSandboxPriorState(registry, "new-sb")).toEqual({ existed: false, wasDefault: false }); - }); - - it("reports an existing default sandbox as existed + wasDefault", () => { - const registry = { getSandbox: () => ({ name: "sb" }), getDefault: () => "sb" }; - expect(captureSandboxPriorState(registry, "sb")).toEqual({ existed: true, wasDefault: true }); - }); - - it("reports an existing non-default sandbox as existed but not default", () => { - const registry = { getSandbox: () => ({ name: "sb" }), getDefault: () => "other" }; - expect(captureSandboxPriorState(registry, "sb")).toEqual({ existed: true, wasDefault: false }); - }); -}); +import { restoreDefaultAfterRecreate, wasSandboxDefault } from "./default-preservation"; describe("wasSandboxDefault", () => { it("is true when the sandbox is the current default", () => { diff --git a/src/lib/onboard/default-preservation.ts b/src/lib/onboard/default-preservation.ts index 7c0073103c..0b9b30a9f8 100644 --- a/src/lib/onboard/default-preservation.ts +++ b/src/lib/onboard/default-preservation.ts @@ -24,28 +24,6 @@ export function wasSandboxDefault( return currentDefault === sandboxName; } -export interface SandboxPriorState { - /** Whether the sandbox was already registered before this create run (any recreate path). */ - existed: boolean; - /** Whether it was the default before this create run. */ - wasDefault: boolean; -} - -/** - * Snapshot a sandbox's registry state at the start of createSandbox, before any - * recreate path tears its entry down. `existed` gates cancel-rollback arming (only - * arm genuinely new sandboxes); `wasDefault` drives {@link restoreDefaultAfterRecreate}. - */ -export function captureSandboxPriorState( - registry: { getSandbox(name: string): unknown; getDefault(): string | null }, - sandboxName: string, -): SandboxPriorState { - return { - existed: registry.getSandbox(sandboxName) != null, - wasDefault: wasSandboxDefault(registry.getDefault(), sandboxName), - }; -} - /** Re-apply the default flag after re-registration iff the sandbox held it beforehand. */ export function restoreDefaultAfterRecreate( setDefault: (sandboxName: string) => void, From b590dc7c65295d3bde77d5cd3afacea0e44f28dc Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Tue, 2 Jun 2026 14:05:00 +0800 Subject: [PATCH 5/6] fix(onboard): route preset/access selector cancel through rollback too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on #4642: selectTierPresetsAndAccess() — the preset/access screen reached via setupPoliciesWithSelection() — still exited directly on SIGTERM / Ctrl+C, bypassing the cancel-rollback. Cancelling there could still leave the freshly-created sandbox behind. Route its SIGTERM and Ctrl+C ("\x03") handlers through makeOnboardCancelExit(sandboxCancelRollback, cleanup), matching selectPolicyTier(). Refs #4614 Signed-off-by: Dongni Yang --- src/lib/onboard.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 662021d614..d0a2cabf90 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -6048,10 +6048,7 @@ async function selectTierPresetsAndAccess( process.removeListener("SIGTERM", onSigterm); }; - const onSigterm = () => { - cleanup(); - process.exit(1); - }; + const onSigterm = makeOnboardCancelExit(sandboxCancelRollback, cleanup); process.once("SIGTERM", onSigterm); const onData = (key: string) => { @@ -6064,8 +6061,7 @@ async function selectTierPresetsAndAccess( .map((p) => ({ name: p.name, access: accessModes[p.name] })), ); } else if (key === "\x03") { - cleanup(); - process.exit(1); + makeOnboardCancelExit(sandboxCancelRollback, cleanup)(); } else if (key === "\x1b[A" || key === "k") { cursor = (cursor - 1 + n) % n; redraw(); From 6c2909159bbbef5c684887076e512d188df6dcdf Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Tue, 2 Jun 2026 14:16:51 +0800 Subject: [PATCH 6/6] fix(onboard): discard the aborted session so cancel doesn't leave a phantom MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A live end-to-end repro (build sandbox -> Ctrl+C at policy tier) showed the cancel-rollback deleted the container and removed the registry entry, but `nemoclaw list` still showed the sandbox. Its session-recovery (inventory keys on session.steps.sandbox.status === "complete") resurrected the just-removed entry as a phantom pointing at a deleted sandbox. Have the rollback also clear the onboard session (clearOnboardSession -> onboardSession.clearSession), so an aborted+rolled-back onboard leaves no resumable/recoverable record. The unit suite couldn't catch this — it doesn't exercise the list session-recovery path; the live repro did. Refs #4614 Signed-off-by: Dongni Yang --- src/lib/onboard.ts | 2 +- src/lib/onboard/cancel-rollback.test.ts | 7 +++++++ src/lib/onboard/cancel-rollback.ts | 10 ++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index d0a2cabf90..d719aa9608 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -6294,7 +6294,7 @@ const onboardRuntimeBoundary = new OnboardRuntimeBoundary({ maybeForceE2eStepFailure, }); -const sandboxCancelRollback = installSandboxCancelRollback({ runOpenshell, registry }); // #4614 +const sandboxCancelRollback = installSandboxCancelRollback({ runOpenshell, registry, clearOnboardSession: onboardSession.clearSession }); // #4614 const startRecordedStep = onboardRuntimeBoundary.startRecordedStep.bind(onboardRuntimeBoundary); const recordStepComplete = onboardRuntimeBoundary.recordStepComplete.bind(onboardRuntimeBoundary); diff --git a/src/lib/onboard/cancel-rollback.test.ts b/src/lib/onboard/cancel-rollback.test.ts index 1d1640461f..4ff069e95a 100644 --- a/src/lib/onboard/cancel-rollback.test.ts +++ b/src/lib/onboard/cancel-rollback.test.ts @@ -15,11 +15,13 @@ function createDeps(overrides: Partial = {}) { const calls = { deleteContainer: vi.fn((_name: string) => true), removeFromRegistry: vi.fn(), + clearSession: vi.fn(), log: vi.fn(), }; const deps: SandboxCancelRollbackDeps = { deleteSandboxContainer: calls.deleteContainer, removeSandboxFromRegistry: calls.removeFromRegistry, + clearOnboardSession: calls.clearSession, log: calls.log, ...overrides, }; @@ -37,6 +39,8 @@ describe("createSandboxCancelRollback", () => { expect(calls.deleteContainer).toHaveBeenCalledWith("new-sb"); expect(calls.removeFromRegistry).toHaveBeenCalledWith("new-sb"); + // also discards the aborted session so `nemoclaw list` recovery can't resurrect it + expect(calls.clearSession).toHaveBeenCalledOnce(); // delete is attempted before the registry entry is removed expect(calls.deleteContainer.mock.invocationCallOrder[0]).toBeLessThan( calls.removeFromRegistry.mock.invocationCallOrder[0], @@ -67,6 +71,7 @@ describe("createSandboxCancelRollback", () => { expect(calls.deleteContainer).not.toHaveBeenCalled(); expect(calls.removeFromRegistry).not.toHaveBeenCalled(); + expect(calls.clearSession).not.toHaveBeenCalled(); expect(calls.log).not.toHaveBeenCalled(); }); @@ -142,6 +147,7 @@ describe("installSandboxCancelRollback", () => { const rollback = installSandboxCancelRollback({ runOpenshell, registry: { removeSandbox }, + clearOnboardSession: () => {}, registerExitHandler: (h) => exitHandlers.push(h), }); @@ -163,6 +169,7 @@ describe("installSandboxCancelRollback", () => { const rollback = installSandboxCancelRollback({ runOpenshell, registry: { removeSandbox }, + clearOnboardSession: () => {}, registerExitHandler: (h) => exitHandlers.push(h), }); rollback.arm("new-sb"); // armed, but never cancelled diff --git a/src/lib/onboard/cancel-rollback.ts b/src/lib/onboard/cancel-rollback.ts index dabb502e93..ac1bafcb51 100644 --- a/src/lib/onboard/cancel-rollback.ts +++ b/src/lib/onboard/cancel-rollback.ts @@ -27,6 +27,12 @@ export interface SandboxCancelRollbackDeps { deleteSandboxContainer(sandboxName: string): boolean; /** Remove the sandbox entry from the NemoClaw registry (clears default). */ removeSandboxFromRegistry(sandboxName: string): void; + /** + * Discard the onboard session for the aborted run. Without this, the session + * still records the sandbox step as "complete", and `nemoclaw list`'s + * session-recovery resurrects the just-removed sandbox as a phantom entry. + */ + clearOnboardSession(): void; /** Emit an operator-facing line (stderr). */ log(message: string): void; } @@ -62,6 +68,7 @@ export function buildCancelRollbackMessage(sandboxName: string, deleteSucceeded: export interface InstallSandboxCancelRollbackOptions { runOpenshell: (args: string[], opts: { ignoreError: boolean }) => { status: number | null }; registry: { removeSandbox(name: string): void }; + clearOnboardSession: () => void; log?: (message: string) => void; /** Override for tests; defaults to `process.on("exit", ...)`. */ registerExitHandler?: (handler: () => void) => void; @@ -83,6 +90,7 @@ export function installSandboxCancelRollback( deleteSandboxContainer: (name) => opts.runOpenshell(["sandbox", "delete", name], { ignoreError: true }).status === 0, removeSandboxFromRegistry: (name) => opts.registry.removeSandbox(name), + clearOnboardSession: opts.clearOnboardSession, log: opts.log ?? ((message) => console.error(message)), }); const register = @@ -142,6 +150,8 @@ export function createSandboxCancelRollback( // worse than an orphaned container the operator can clean up manually. const deleteSucceeded = deps.deleteSandboxContainer(sandboxName); deps.removeSandboxFromRegistry(sandboxName); + // Discard the aborted session so `nemoclaw list` recovery doesn't resurrect it. + deps.clearOnboardSession(); for (const line of buildCancelRollbackMessage(sandboxName, deleteSucceeded)) { deps.log(line); }