From c172addb62d44452c1c10eb4f37bede299748da2 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Fri, 14 Aug 2026 17:03:05 +0800 Subject: [PATCH 1/7] fix(sandbox): stop the model router when the last routed sandbox is destroyed The Model Router proxy is a detached host process whose PID is recorded only in the onboarding session (routerPid). Uninstall stops it (#5169) and an agent change stops it, but destroy never did, so the orphaned proxy kept port 4000 and the next routed onboard failed with "Port 4000 already has a healthy router endpoint; refusing to start a second router" with no user-facing recovery. Destroy now stops the router after the registry entry of a routed sandbox is removed and no registered routed sandbox remains, using the existing ownership-checked SIGTERM primitive. The recorded PID is preferred; a /proc scan recovers an orphan whose PID a fresh session no longer records. A stop failure warns and keeps routerPid so uninstall and reconcile can still find the process, because the sandbox delete already succeeded and a stuck session-global proxy must not fail the destroy. The teardown lives in destroy-preflight.ts beside the other inference teardown and receives session access from destroy.ts, keeping the source-architecture fan-in and file-count budgets unchanged. Fixes #9098 Signed-off-by: Dongni Yang --- docs/inference/set-up-model-router.mdx | 2 + .../sandbox/destroy-model-router-flow.test.ts | 120 ++++++++++++++ .../sandbox/destroy-model-router.test.ts | 151 ++++++++++++++++++ src/lib/actions/sandbox/destroy-preflight.ts | 96 +++++++++++ src/lib/actions/sandbox/destroy.ts | 13 +- test/helpers/destroy-flow-test-harness.ts | 4 + 6 files changed, 385 insertions(+), 1 deletion(-) create mode 100644 src/lib/actions/sandbox/destroy-model-router-flow.test.ts create mode 100644 src/lib/actions/sandbox/destroy-model-router.test.ts diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index 85a4b34d64..ac4134a390 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -32,6 +32,8 @@ When the log contains readable output from this run, the error includes a redact The owner-only log contains unredacted router output. Review the log before you share it. NemoClaw does not provide log-only cleanup. +When a destroy removes the last registered Model Router sandbox, it also stops the Model Router process and frees its port. +A destroy of one Model Router sandbox keeps the router running while another registered Model Router sandbox remains. A successful [uninstall](../../manage-sandboxes/operate-sandboxes/uninstall-nemoclaw) stops the selected Model Router and removes its log with the selected gateway's operational state. Review the uninstall scope before you use it for log removal. diff --git a/src/lib/actions/sandbox/destroy-model-router-flow.test.ts b/src/lib/actions/sandbox/destroy-model-router-flow.test.ts new file mode 100644 index 0000000000..80af136879 --- /dev/null +++ b/src/lib/actions/sandbox/destroy-model-router-flow.test.ts @@ -0,0 +1,120 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { type ChildProcess, spawn } from "node:child_process"; +import fs from "node:fs"; +import net from "node:net"; +import os from "node:os"; +import path from "node:path"; + +import { afterEach, beforeEach, describe, expect, it, type MockInstance, vi } from "vitest"; + +import { + createDestroyHarness, + resetDestroyModuleCache, +} from "../../../../test/helpers/destroy-flow-test-harness"; + +// A real detached HTTP server whose command line matches the model-router +// proxy shape (venv-style interposition: args[0]=node, args[1]=.../model-router). +const STUB_SOURCE = [ + 'const http = require("node:http");', + 'const port = Number(process.argv[process.argv.indexOf("--port") + 1]);', + "http", + ' .createServer((_req, res) => { res.statusCode = 200; res.end("{}"); })', + ' .listen(port, "127.0.0.1");', +].join("\n"); + +async function reserveLoopbackPort(): Promise { + return new Promise((resolve, reject) => { + const server = net.createServer(); + server.once("error", reject); + server.listen(0, "127.0.0.1", () => { + const address = server.address() as net.AddressInfo; + server.close(() => resolve(address.port)); + }); + }); +} + +async function probeHealthy(port: number): Promise { + try { + const response = await fetch(`http://127.0.0.1:${port}/health`, { + signal: AbortSignal.timeout(1000), + }); + return response.ok; + } catch { + return false; + } +} + +describe("destroySandbox model-router teardown (#9098)", () => { + let exitSpy: MockInstance; + let originalGatewayEnv: string | undefined; + let stubDir: string; + let stub: ChildProcess | null = null; + + beforeEach(() => { + originalGatewayEnv = process.env.OPENSHELL_GATEWAY; + exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number | string | null) => { + throw new Error(`process.exit(${code ?? 0})`); + }) as never); + stubDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-router-stub-")); + }); + + afterEach(() => { + try { + stub?.kill("SIGKILL"); + } catch { + // Already exited. + } + stub = null; + fs.rmSync(stubDir, { recursive: true, force: true }); + originalGatewayEnv === undefined + ? delete process.env.OPENSHELL_GATEWAY + : (process.env.OPENSHELL_GATEWAY = originalGatewayEnv); + vi.restoreAllMocks(); + vi.unstubAllEnvs(); + resetDestroyModuleCache(); + }); + + it( + "destroying the last routed sandbox stops the tracked router proxy and frees its port (#9098)", + { timeout: 30_000 }, + async () => { + const port = await reserveLoopbackPort(); + const stubPath = path.join(stubDir, "model-router"); + fs.writeFileSync(stubPath, STUB_SOURCE); + stub = spawn(process.execPath, [stubPath, "proxy", "--port", String(port)], { + stdio: "ignore", + }); + let stubExited = false; + stub.on("exit", () => { + stubExited = true; + }); + await vi.waitFor(async () => expect(await probeHealthy(port)).toBe(true), { + timeout: 10_000, + interval: 100, + }); + + const harness = createDestroyHarness({ + provider: "nvidia-router", + endpointUrl: `http://host.openshell.internal:${port}/v1`, + sessionRouterPid: stub.pid, + }); + + await expect( + harness.destroySandbox("alpha", { yes: true, cleanupGateway: true }), + ).resolves.toBeUndefined(); + + await vi.waitFor(() => expect(stubExited).toBe(true), { timeout: 8_000, interval: 100 }); + expect(await probeHealthy(port)).toBe(false); + expect(exitSpy).not.toHaveBeenCalled(); + + const sessionsWithRouterPidCleared = harness.updateSessionSpy.mock.results + .map((result) => result.value as { routerPid?: number | null }) + .filter((session) => "routerPid" in session); + expect(sessionsWithRouterPidCleared).toEqual([ + expect.objectContaining({ routerPid: null, routerCredentialHash: null }), + ]); + }, + ); +}); diff --git a/src/lib/actions/sandbox/destroy-model-router.test.ts b/src/lib/actions/sandbox/destroy-model-router.test.ts new file mode 100644 index 0000000000..6b01ba9f8e --- /dev/null +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -0,0 +1,151 @@ +// 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 type { Session } from "../../state/onboard-session"; +import type { SandboxEntry } from "../../state/registry"; +import { + resolveDestroyedSandboxRouterPort, + stopModelRouterForDestroyedSandbox, + type StopModelRouterForDestroyedSandboxDeps, +} from "./destroy-preflight"; + +const routedSandbox = { + name: "alpha", + provider: "nvidia-router", + endpointUrl: "http://host.openshell.internal:4100/v1", +} as SandboxEntry; + +function createDeps(overrides: Partial = {}) { + const session = { routerPid: 4242, routerCredentialHash: "hash" } as Session; + const deps = { + findPidForPort: vi.fn(() => null), + isRoutedProvider: vi.fn((provider: string | null | undefined) => provider === "nvidia-router"), + listSandboxes: vi.fn(() => ({ sandboxes: [] as SandboxEntry[], defaultSandbox: null })), + loadSession: vi.fn(() => session), + log: vi.fn(), + ownsPort: vi.fn(() => true), + stopProcess: vi.fn(async () => undefined), + updateSession: vi.fn((mutator: (current: Session) => Session | void) => { + mutator(session); + return session; + }), + warn: vi.fn(), + ...overrides, + }; + return { deps, session }; +} + +describe("resolveDestroyedSandboxRouterPort", () => { + it("parses the router port from the sandbox endpoint URL", () => { + expect(resolveDestroyedSandboxRouterPort("http://host.openshell.internal:4100/v1")).toBe(4100); + }); + + it("falls back to port 4000 for a missing or unparseable endpoint", () => { + expect(resolveDestroyedSandboxRouterPort(null)).toBe(4000); + expect(resolveDestroyedSandboxRouterPort("not a url")).toBe(4000); + expect(resolveDestroyedSandboxRouterPort("http://host.openshell.internal/v1")).toBe(4000); + }); +}); + +describe("stopModelRouterForDestroyedSandbox", () => { + it("stops the tracked router and clears its session identity for the last routed sandbox (#9098)", async () => { + const { deps, session } = createDeps(); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.stopProcess).toHaveBeenCalledWith(4242, 4100); + expect(session.routerPid).toBeNull(); + expect(session.routerCredentialHash).toBeNull(); + expect(deps.warn).not.toHaveBeenCalled(); + }); + + it("does nothing for a sandbox without a routed provider", async () => { + const { deps } = createDeps(); + + await stopModelRouterForDestroyedSandbox( + { name: "alpha", provider: "ollama-local" } as SandboxEntry, + deps, + ); + + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(deps.updateSession).not.toHaveBeenCalled(); + }); + + it("does nothing when the registry entry is missing", async () => { + const { deps } = createDeps(); + + await stopModelRouterForDestroyedSandbox(null, deps); + + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(deps.updateSession).not.toHaveBeenCalled(); + }); + + it("keeps the router while another registered routed sandbox remains", async () => { + const { deps } = createDeps({ + listSandboxes: vi.fn(() => ({ + sandboxes: [{ name: "beta", provider: "nvidia-router" } as SandboxEntry], + defaultSandbox: null, + })), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(deps.updateSession).not.toHaveBeenCalled(); + }); + + it("recovers an orphaned router by port scan when the recorded PID does not own the port", async () => { + const { deps, session } = createDeps({ + ownsPort: vi.fn(() => false), + findPidForPort: vi.fn(() => 5151), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.findPidForPort).toHaveBeenCalledWith(4100); + expect(deps.stopProcess).toHaveBeenCalledWith(5151, 4100); + expect(session.routerPid).toBeNull(); + }); + + it("clears a stale recorded PID when no router process is found", async () => { + const { deps, session } = createDeps({ + ownsPort: vi.fn(() => false), + findPidForPort: vi.fn(() => null), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(session.routerPid).toBeNull(); + expect(session.routerCredentialHash).toBeNull(); + }); + + it("leaves the session untouched when it records no router PID and no orphan exists", async () => { + const { deps } = createDeps({ + loadSession: vi.fn(() => ({ routerPid: null }) as Session), + ownsPort: vi.fn(() => false), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(deps.updateSession).not.toHaveBeenCalled(); + }); + + it("warns and keeps the recorded PID when the stop fails, so uninstall can still find it", async () => { + const { deps, session } = createDeps({ + stopProcess: vi.fn(async () => { + throw new Error("shutdown did not converge"); + }), + }); + + await expect(stopModelRouterForDestroyedSandbox(routedSandbox, deps)).resolves.toBeUndefined(); + + expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("shutdown did not converge")); + expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("kill 4242")); + expect(deps.updateSession).not.toHaveBeenCalled(); + expect(session.routerPid).toBe(4242); + }); +}); diff --git a/src/lib/actions/sandbox/destroy-preflight.ts b/src/lib/actions/sandbox/destroy-preflight.ts index dc2b688d72..b9d3df09cf 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -2,6 +2,13 @@ // SPDX-License-Identifier: Apache-2.0 import { OPENSHELL_PROBE_TIMEOUT_MS } from "../../adapters/openshell/timeouts"; +import { isRoutedInferenceProvider } from "../../onboard/model-router"; +import { + doesModelRouterProcessOwnPort, + findModelRouterPidForPort, + stopModelRouterProcess, +} from "../../onboard/model-router-process"; +import type { Session } from "../../state/onboard-session"; import type { SandboxEntry } from "../../state/registry"; import * as registry from "../../state/registry"; import { type DestroyRunOpenshell, selectGatewayForSandboxDestroy } from "./destroy-gateway"; @@ -42,6 +49,95 @@ export function stopSandboxInferenceResources( } } +// Routed onboard profiles use blueprint port 4000 by default; matches the +// uninstall teardown default in src/lib/actions/uninstall/run-plan.ts. +const DEFAULT_MODEL_ROUTER_PORT = 4000; + +export type StopModelRouterForDestroyedSandboxDeps = { + loadSession: () => Session | null; + updateSession: (mutator: (session: Session) => Session | void) => Session; + findPidForPort?: typeof findModelRouterPidForPort; + isRoutedProvider?: typeof isRoutedInferenceProvider; + listSandboxes?: typeof registry.listSandboxes; + log?: (message: string) => void; + ownsPort?: typeof doesModelRouterProcessOwnPort; + stopProcess?: (pid: number, port: number) => Promise; + warn?: (message: string) => void; +}; + +export function resolveDestroyedSandboxRouterPort(endpointUrl: string | null | undefined): number { + try { + const port = Number(new URL(endpointUrl ?? "").port); + return Number.isInteger(port) && port > 0 ? port : DEFAULT_MODEL_ROUTER_PORT; + } catch { + return DEFAULT_MODEL_ROUTER_PORT; + } +} + +/** + * Stop the host Model Router proxy after the last routed sandbox is destroyed. + * + * The router is a detached host process whose PID is recorded only in the + * onboarding session (routerPid). Destroy never stopped it, so the orphan kept + * its port and the next routed onboard failed with "Port 4000 already has a + * healthy router endpoint" (#9098). This mirrors the uninstall teardown + * (#5169) but stays scoped: it acts only when the destroyed sandbox was routed + * and no registered routed sandbox remains, so a routed peer keeps its router. + * + * The recorded PID is preferred; when a fresh session no longer records it, + * the /proc scan recovers the orphan by verified command line, exactly like + * reconcileModelRouter's recovery path. A stop failure is a warning, not an + * error: the sandbox delete already succeeded, and a stuck session-global host + * proxy must not fail the destroy. The session keeps routerPid on failure so + * uninstall and reconcile can still find the process. + */ +export async function stopModelRouterForDestroyedSandbox( + sandbox: SandboxEntry | null, + deps: StopModelRouterForDestroyedSandboxDeps, +): Promise { + const isRoutedProvider = deps.isRoutedProvider ?? isRoutedInferenceProvider; + if (!isRoutedProvider(sandbox?.provider)) return; + const listSandboxes = deps.listSandboxes ?? registry.listSandboxes; + // Called after registry removal, so every remaining entry is a peer. + const routedPeerRemains = listSandboxes().sandboxes.some((entry) => + isRoutedProvider(entry.provider), + ); + if (routedPeerRemains) return; + + const port = resolveDestroyedSandboxRouterPort(sandbox?.endpointUrl); + const ownsPort = deps.ownsPort ?? doesModelRouterProcessOwnPort; + const findPidForPort = deps.findPidForPort ?? findModelRouterPidForPort; + const recordedPid = deps.loadSession()?.routerPid ?? null; + const pid = ownsPort(recordedPid, port) ? (recordedPid as number) : findPidForPort(port); + + if (pid !== null) { + const log = deps.log ?? console.log; + const warn = deps.warn ?? console.warn; + log(` Stopping model router (PID ${pid})...`); + try { + await (deps.stopProcess ?? stopModelRouterProcess)(pid, port); + } catch (error) { + warn( + `Failed to stop the model router (PID ${pid}) on port ${port}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + warn( + `Stop it manually (kill ${pid}) before the next Model Router onboard, or the onboard fails with "Port ${port} already has a healthy router endpoint".`, + ); + return; + } + } + + if (recordedPid !== null) { + deps.updateSession((current: Session) => { + current.routerPid = null; + current.routerCredentialHash = null; + return current; + }); + } +} + export function prepareSandboxDestroy(sandboxName: string): SandboxDestroyPreflight { const sandbox = registry.getSandbox(sandboxName); console.log(` Deleting sandbox '${sandboxName}'...`); diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 710dc66988..97786802de 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -53,7 +53,11 @@ import { classifyDestroySandboxPresence, isSameDestroyContainerIdentityProof, } from "./destroy-presence"; -import { prepareSandboxDestroy, stopSandboxInferenceResources } from "./destroy-preflight"; +import { + prepareSandboxDestroy, + stopModelRouterForDestroyedSandbox, + stopSandboxInferenceResources, +} from "./destroy-preflight"; import { type WipeSandboxStateDeps, wipeSandboxState } from "./wipe-state"; export { assertUnambiguousDestroyContainerIdentity, classifyDestroySandboxPresence }; @@ -661,6 +665,13 @@ async function destroySandboxUnlocked( if (deleteSucceededOrAlreadyGone && removed && priorHttpsPinRouteId) { await revokeDestroyedSandboxHttpsPinRoute(cleanupGatewayName, priorHttpsPinRouteId); } + if (deleteSucceededOrAlreadyGone && removed) { + await stopModelRouterForDestroyedSandbox(sandbox, { + loadSession: onboardSession.loadSession, + updateSession: onboardSession.updateSession, + warn: defaultDestroyWarn, + }); + } const session = onboardSession.loadSession(); if (session && session.sandboxName === sandboxName) { onboardSession.updateSession((s: Session) => { diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index 7ca7a02330..deb5335842 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -69,9 +69,11 @@ type DestroyHarnessOptions = { openshellDriver?: string; prepareMcpBridgeError?: string; promptResponses?: string[]; + provider?: string; registeredSandboxCount?: number; restoreMcpError?: string; sandboxPresent?: boolean; + sessionRouterPid?: number; shieldsDown?: boolean; shieldsUpError?: Error; stopInferenceError?: string; @@ -178,6 +180,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr ...sandboxEntry, imageTag: options.imageTag === undefined ? sandboxEntry.imageTag : options.imageTag, agent: options.agent ?? sandboxEntry.agent, + ...(options.provider ? { provider: options.provider } : {}), ...(options.openshellDriver ? { openshellDriver: options.openshellDriver } : {}), ...(options.endpointUrl ? { endpointUrl: options.endpointUrl } : {}), ...(options.workload ? { workload: options.workload } : {}), @@ -215,6 +218,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr .mockResolvedValue(true); vi.spyOn(onboardSession, "loadSession").mockReturnValue({ sandboxName: "alpha", + ...(options.sessionRouterPid ? { routerPid: options.sessionRouterPid } : {}), }); const updateSessionSpy = vi .spyOn(onboardSession, "updateSession") From 5013dd61b07999831ad6a3532e27641724bf35e7 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Fri, 14 Aug 2026 17:13:15 +0800 Subject: [PATCH 2/7] docs(sandbox): state the Model Router stop failure result and align terms The documentation writer review found two gaps: the Model Router page claimed an unconditional stop without its failure result, and the destroy command reference omitted the new teardown beside the existing Ollama one. Also align the new CLI strings with the controlled words: "Model Router" in prose and "onboarding" as the noun. Refs #9098 Signed-off-by: Dongni Yang --- docs/inference/set-up-model-router.mdx | 5 +++-- docs/reference/commands.mdx | 1 + src/lib/actions/sandbox/destroy-preflight.ts | 6 +++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index ac4134a390..a0f817dd17 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -32,8 +32,9 @@ When the log contains readable output from this run, the error includes a redact The owner-only log contains unredacted router output. Review the log before you share it. NemoClaw does not provide log-only cleanup. -When a destroy removes the last registered Model Router sandbox, it also stops the Model Router process and frees its port. -A destroy of one Model Router sandbox keeps the router running while another registered Model Router sandbox remains. +When `destroy` removes the last registered Model Router sandbox, it also stops the Model Router process and frees its port. +If the stop fails, destroy still completes and prints a warning with the manual stop command. +While another registered Model Router sandbox remains, destroying one Model Router sandbox keeps the Model Router process running. A successful [uninstall](../../manage-sandboxes/operate-sandboxes/uninstall-nemoclaw) stops the selected Model Router and removes its log with the selected gateway's operational state. Review the uninstall scope before you use it for log removal. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 8df4f09583..88ddaa6463 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2260,6 +2260,7 @@ Model traffic uses the OpenShell-managed `inference.local` route configured by N Stop managed local inference resources, remove the host-side Docker image built during onboard, and delete the sandbox. This removes the sandbox from the registry. For Ollama-backed sandboxes, `destroy` also asks Ollama to unload currently loaded models and clears stale auth proxy state on a best-effort basis. +When `destroy` removes the last registered Model Router sandbox, it also stops the host Model Router process on a best-effort basis and frees its port. This command attempts to wipe the manifest-defined agent state while its persistent volume is mounted, then removes the sandbox. diff --git a/src/lib/actions/sandbox/destroy-preflight.ts b/src/lib/actions/sandbox/destroy-preflight.ts index b9d3df09cf..4b3e0ce664 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -113,17 +113,17 @@ export async function stopModelRouterForDestroyedSandbox( if (pid !== null) { const log = deps.log ?? console.log; const warn = deps.warn ?? console.warn; - log(` Stopping model router (PID ${pid})...`); + log(` Stopping Model Router (PID ${pid})...`); try { await (deps.stopProcess ?? stopModelRouterProcess)(pid, port); } catch (error) { warn( - `Failed to stop the model router (PID ${pid}) on port ${port}: ${ + `Failed to stop the Model Router (PID ${pid}) on port ${port}: ${ error instanceof Error ? error.message : String(error) }`, ); warn( - `Stop it manually (kill ${pid}) before the next Model Router onboard, or the onboard fails with "Port ${port} already has a healthy router endpoint".`, + `Stop it manually (kill ${pid}) before the next Model Router onboarding, or onboarding fails with "Port ${port} already has a healthy router endpoint".`, ); return; } From a273bf732e12e9831811771bc89ae1a8dd4441a7 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Fri, 14 Aug 2026 18:02:56 +0800 Subject: [PATCH 3/7] fix(sandbox): serialize router teardown and clear stale credential hash Two review findings on the Model Router destroy teardown. The routed-peer scan and router stop now run under the gateway route lock, the same lock routed onboarding holds while it registers its route, so a concurrent onboard cannot register a routed sandbox after the scan and then lose its shared router; a lock failure warns and does not fail the destroy. The session clear now also fires when only routerCredentialHash is set, so a session without a recorded PID does not keep stale router identity. Refs #9098 Signed-off-by: Dongni Yang --- .../sandbox/destroy-model-router-flow.test.ts | 8 ++++++- .../sandbox/destroy-model-router.test.ts | 18 ++++++++++++++ src/lib/actions/sandbox/destroy-preflight.ts | 8 +++++-- src/lib/actions/sandbox/destroy.ts | 24 +++++++++++++++---- test/helpers/destroy-flow-test-harness.ts | 10 ++++++++ 5 files changed, 60 insertions(+), 8 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-model-router-flow.test.ts b/src/lib/actions/sandbox/destroy-model-router-flow.test.ts index 80af136879..d5509f5e6c 100644 --- a/src/lib/actions/sandbox/destroy-model-router-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router-flow.test.ts @@ -100,11 +100,17 @@ describe("destroySandbox model-router teardown (#9098)", () => { endpointUrl: `http://host.openshell.internal:${port}/v1`, sessionRouterPid: stub.pid, }); - await expect( harness.destroySandbox("alpha", { yes: true, cleanupGateway: true }), ).resolves.toBeUndefined(); + // The teardown must run under the gateway route lock so routed + // onboarding cannot register a peer between the scan and the stop. + expect(harness.withGatewayRouteMutationLockSpy).toHaveBeenCalledWith( + "nemoclaw-19080", + expect.any(Function), + ); + await vi.waitFor(() => expect(stubExited).toBe(true), { timeout: 8_000, interval: 100 }); expect(await probeHealthy(port)).toBe(false); expect(exitSpy).not.toHaveBeenCalled(); diff --git a/src/lib/actions/sandbox/destroy-model-router.test.ts b/src/lib/actions/sandbox/destroy-model-router.test.ts index 6b01ba9f8e..f07abfe6bf 100644 --- a/src/lib/actions/sandbox/destroy-model-router.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -122,6 +122,24 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(session.routerCredentialHash).toBeNull(); }); + it("clears a stale credential hash when the session records no router PID (#9098)", async () => { + const session = { routerPid: null, routerCredentialHash: "stale" } as Session; + const { deps } = createDeps({ + loadSession: vi.fn(() => session), + ownsPort: vi.fn(() => false), + updateSession: vi.fn((mutator: (current: Session) => Session | void) => { + mutator(session); + return session; + }), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(session.routerPid).toBeNull(); + expect(session.routerCredentialHash).toBeNull(); + }); + it("leaves the session untouched when it records no router PID and no orphan exists", async () => { const { deps } = createDeps({ loadSession: vi.fn(() => ({ routerPid: null }) as Session), diff --git a/src/lib/actions/sandbox/destroy-preflight.ts b/src/lib/actions/sandbox/destroy-preflight.ts index 4b3e0ce664..1923bfe58e 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -107,7 +107,9 @@ export async function stopModelRouterForDestroyedSandbox( const port = resolveDestroyedSandboxRouterPort(sandbox?.endpointUrl); const ownsPort = deps.ownsPort ?? doesModelRouterProcessOwnPort; const findPidForPort = deps.findPidForPort ?? findModelRouterPidForPort; - const recordedPid = deps.loadSession()?.routerPid ?? null; + const session = deps.loadSession(); + const recordedPid = session?.routerPid ?? null; + const recordedCredentialHash = session?.routerCredentialHash ?? null; const pid = ownsPort(recordedPid, port) ? (recordedPid as number) : findPidForPort(port); if (pid !== null) { @@ -129,7 +131,9 @@ export async function stopModelRouterForDestroyedSandbox( } } - if (recordedPid !== null) { + // Clear when either field is set: a session with only a credential hash + // still carries stale router identity after the last routed sandbox is gone. + if (recordedPid !== null || recordedCredentialHash !== null) { deps.updateSession((current: Session) => { current.routerPid = null; current.routerCredentialHash = null; diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 97786802de..03139f86e2 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -666,11 +666,25 @@ async function destroySandboxUnlocked( await revokeDestroyedSandboxHttpsPinRoute(cleanupGatewayName, priorHttpsPinRouteId); } if (deleteSucceededOrAlreadyGone && removed) { - await stopModelRouterForDestroyedSandbox(sandbox, { - loadSession: onboardSession.loadSession, - updateSession: onboardSession.updateSession, - warn: defaultDestroyWarn, - }); + try { + // The routed-peer scan and router stop are one critical section with + // routed onboarding's route registration, which runs under the same + // gateway route lock. Otherwise a concurrent onboard can register a + // routed sandbox after the scan and then lose its shared router. + await withGatewayRouteMutationLock(cleanupGatewayName, () => + stopModelRouterForDestroyedSandbox(sandbox, { + loadSession: onboardSession.loadSession, + updateSession: onboardSession.updateSession, + warn: defaultDestroyWarn, + }), + ); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + defaultDestroyWarn( + `Sandbox deletion succeeded, but the Model Router teardown could not run under the gateway route lock: ${detail}. ` + + `Stop the Model Router process manually before the next Model Router onboarding.`, + ); + } } const session = onboardSession.loadSession(); if (session && session.sandboxName === sandboxName) { diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index deb5335842..b745f6e9d2 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -43,6 +43,7 @@ export type DestroyHarness = { unloadOllamaModelsSpy: MockInstance; updateSessionSpy: MockInstance; warnSpy: MockInstance; + withGatewayRouteMutationLockSpy: MockInstance; }; type DestroyHarnessOptions = { @@ -156,6 +157,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr const sandboxProviderCleanup = requireDist("../../onboard/sandbox-provider-cleanup.js"); const nim = requireDist("../../inference/nim.js"); const ollamaProxy = requireDist("../../inference/ollama/proxy.js"); + const gatewayRouteMutationLock = requireDist("../../inference/gateway-route-mutation-lock.js"); const httpsPinRuntimeAdapter = requireDist("../../inference/https-pin-runtime-adapter.js"); const tunnelServices = requireDist("../../tunnel/services.js"); const onboardSession = requireDist("../../state/onboard-session.js"); @@ -216,6 +218,13 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr const revokeHttpsPinRuntimeAdapterRouteSpy = vi .spyOn(httpsPinRuntimeAdapter, "revokeHttpsPinRuntimeAdapterRoute") .mockResolvedValue(true); + // Pass-through: run the critical section without the cross-process lease so + // flow tests stay hermetic while asserting lock scope. + const withGatewayRouteMutationLockSpy = vi + .spyOn(gatewayRouteMutationLock, "withGatewayRouteMutationLock") + .mockImplementation(async (_gatewayName: unknown, operation: unknown) => + (operation as () => Promise)(), + ); vi.spyOn(onboardSession, "loadSession").mockReturnValue({ sandboxName: "alpha", ...(options.sessionRouterPid ? { routerPid: options.sessionRouterPid } : {}), @@ -427,5 +436,6 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr unloadOllamaModelsSpy, updateSessionSpy, warnSpy, + withGatewayRouteMutationLockSpy, }; } From 60e16b5f0ec9c05914552f5890b01f0b91eed6ec Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Fri, 14 Aug 2026 18:17:14 +0800 Subject: [PATCH 4/7] fix(sandbox): state the teardown failure without naming the lock The catch also receives failures from inside the teardown, not only lock-acquisition failures, so the warning now states that the teardown did not complete and includes the underlying detail. Also use the controlled noun "onboarding" in the serialization comment. Refs #9098 Signed-off-by: Dongni Yang --- src/lib/actions/sandbox/destroy.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 03139f86e2..dc912ddb1f 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -669,7 +669,7 @@ async function destroySandboxUnlocked( try { // The routed-peer scan and router stop are one critical section with // routed onboarding's route registration, which runs under the same - // gateway route lock. Otherwise a concurrent onboard can register a + // gateway route lock. Otherwise concurrent onboarding can register a // routed sandbox after the scan and then lose its shared router. await withGatewayRouteMutationLock(cleanupGatewayName, () => stopModelRouterForDestroyedSandbox(sandbox, { @@ -681,7 +681,7 @@ async function destroySandboxUnlocked( } catch (error) { const detail = error instanceof Error ? error.message : String(error); defaultDestroyWarn( - `Sandbox deletion succeeded, but the Model Router teardown could not run under the gateway route lock: ${detail}. ` + + `Sandbox deletion succeeded, but the Model Router teardown did not complete: ${detail}. ` + `Stop the Model Router process manually before the next Model Router onboarding.`, ); } From de4be75e98e570bd2dffcf38016308253d4ea495 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Fri, 14 Aug 2026 11:37:52 -0700 Subject: [PATCH 5/7] fix(sandbox): scope Model Router teardown by port Signed-off-by: Carlos Villela --- docs/inference/set-up-model-router.mdx | 4 +- docs/reference/commands.mdx | 2 +- .../sandbox/destroy-model-router.test.ts | 78 ++++++++++++++++++- src/lib/actions/sandbox/destroy-preflight.ts | 27 +++++-- .../destroy-model-router-flow.test.ts | 2 +- 5 files changed, 100 insertions(+), 13 deletions(-) rename {src/lib/actions/sandbox => test/package-contract}/destroy-model-router-flow.test.ts (98%) diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index a0f817dd17..aa7694f33c 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -32,9 +32,9 @@ When the log contains readable output from this run, the error includes a redact The owner-only log contains unredacted router output. Review the log before you share it. NemoClaw does not provide log-only cleanup. -When `destroy` removes the last registered Model Router sandbox, it also stops the Model Router process and frees its port. +When `destroy` removes the last registered Model Router sandbox that uses a host port, it also stops the Model Router process and frees that port. If the stop fails, destroy still completes and prints a warning with the manual stop command. -While another registered Model Router sandbox remains, destroying one Model Router sandbox keeps the Model Router process running. +While another registered Model Router sandbox uses the same host port, destroying one Model Router sandbox keeps that Model Router process running. A successful [uninstall](../../manage-sandboxes/operate-sandboxes/uninstall-nemoclaw) stops the selected Model Router and removes its log with the selected gateway's operational state. Review the uninstall scope before you use it for log removal. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 88ddaa6463..e729b9d76d 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2260,7 +2260,7 @@ Model traffic uses the OpenShell-managed `inference.local` route configured by N Stop managed local inference resources, remove the host-side Docker image built during onboard, and delete the sandbox. This removes the sandbox from the registry. For Ollama-backed sandboxes, `destroy` also asks Ollama to unload currently loaded models and clears stale auth proxy state on a best-effort basis. -When `destroy` removes the last registered Model Router sandbox, it also stops the host Model Router process on a best-effort basis and frees its port. +When `destroy` removes the last registered Model Router sandbox that uses a host port, it also stops the host Model Router process on a best-effort basis and frees that port. This command attempts to wipe the manifest-defined agent state while its persistent volume is mounted, then removes the sandbox. diff --git a/src/lib/actions/sandbox/destroy-model-router.test.ts b/src/lib/actions/sandbox/destroy-model-router.test.ts index f07abfe6bf..a7bea07fb8 100644 --- a/src/lib/actions/sandbox/destroy-model-router.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -18,7 +18,13 @@ const routedSandbox = { } as SandboxEntry; function createDeps(overrides: Partial = {}) { - const session = { routerPid: 4242, routerCredentialHash: "hash" } as Session; + const session = { + sessionId: "session-alpha", + sandboxName: "alpha", + endpointUrl: "http://host.openshell.internal:4100/v1", + routerPid: 4242, + routerCredentialHash: "hash", + } as Session; const deps = { findPidForPort: vi.fn(() => null), isRoutedProvider: vi.fn((provider: string | null | undefined) => provider === "nvidia-router"), @@ -85,7 +91,13 @@ describe("stopModelRouterForDestroyedSandbox", () => { it("keeps the router while another registered routed sandbox remains", async () => { const { deps } = createDeps({ listSandboxes: vi.fn(() => ({ - sandboxes: [{ name: "beta", provider: "nvidia-router" } as SandboxEntry], + sandboxes: [ + { + name: "beta", + provider: "nvidia-router", + endpointUrl: "http://host.openshell.internal:4100/v1", + } as SandboxEntry, + ], defaultSandbox: null, })), }); @@ -96,6 +108,25 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(deps.updateSession).not.toHaveBeenCalled(); }); + it("stops the target router when a routed peer uses a different port", async () => { + const { deps } = createDeps({ + listSandboxes: vi.fn(() => ({ + sandboxes: [ + { + name: "beta", + provider: "nvidia-router", + endpointUrl: "http://host.openshell.internal:4200/v1", + } as SandboxEntry, + ], + defaultSandbox: null, + })), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.stopProcess).toHaveBeenCalledWith(4242, 4100); + }); + it("recovers an orphaned router by port scan when the recorded PID does not own the port", async () => { const { deps, session } = createDeps({ ownsPort: vi.fn(() => false), @@ -123,7 +154,13 @@ describe("stopModelRouterForDestroyedSandbox", () => { }); it("clears a stale credential hash when the session records no router PID (#9098)", async () => { - const session = { routerPid: null, routerCredentialHash: "stale" } as Session; + const session = { + sessionId: "session-alpha", + sandboxName: "alpha", + endpointUrl: "http://host.openshell.internal:4100/v1", + routerPid: null, + routerCredentialHash: "stale", + } as Session; const { deps } = createDeps({ loadSession: vi.fn(() => session), ownsPort: vi.fn(() => false), @@ -140,9 +177,42 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(session.routerCredentialHash).toBeNull(); }); + it("does not clear session identity for a different routed sandbox", async () => { + const session = { + sessionId: "session-beta", + sandboxName: "beta", + endpointUrl: "http://host.openshell.internal:4200/v1", + routerPid: 5252, + routerCredentialHash: "beta-hash", + } as Session; + const { deps } = createDeps({ + loadSession: vi.fn(() => session), + ownsPort: vi.fn(() => false), + findPidForPort: vi.fn(() => 5151), + updateSession: vi.fn((mutator: (current: Session) => Session | void) => { + mutator(session); + return session; + }), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.stopProcess).toHaveBeenCalledWith(5151, 4100); + expect(deps.updateSession).not.toHaveBeenCalled(); + expect(session).toMatchObject({ routerPid: 5252, routerCredentialHash: "beta-hash" }); + }); + it("leaves the session untouched when it records no router PID and no orphan exists", async () => { const { deps } = createDeps({ - loadSession: vi.fn(() => ({ routerPid: null }) as Session), + loadSession: vi.fn( + () => + ({ + sessionId: "session-beta", + sandboxName: "beta", + endpointUrl: "http://host.openshell.internal:4200/v1", + routerPid: null, + }) as Session, + ), ownsPort: vi.fn(() => false), }); diff --git a/src/lib/actions/sandbox/destroy-preflight.ts b/src/lib/actions/sandbox/destroy-preflight.ts index 1923bfe58e..a27b64c62b 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -97,20 +97,28 @@ export async function stopModelRouterForDestroyedSandbox( ): Promise { const isRoutedProvider = deps.isRoutedProvider ?? isRoutedInferenceProvider; if (!isRoutedProvider(sandbox?.provider)) return; + const port = resolveDestroyedSandboxRouterPort(sandbox?.endpointUrl); const listSandboxes = deps.listSandboxes ?? registry.listSandboxes; // Called after registry removal, so every remaining entry is a peer. - const routedPeerRemains = listSandboxes().sandboxes.some((entry) => - isRoutedProvider(entry.provider), + const routedPeerRemains = listSandboxes().sandboxes.some( + (entry) => + isRoutedProvider(entry.provider) && + resolveDestroyedSandboxRouterPort(entry.endpointUrl) === port, ); if (routedPeerRemains) return; - const port = resolveDestroyedSandboxRouterPort(sandbox?.endpointUrl); const ownsPort = deps.ownsPort ?? doesModelRouterProcessOwnPort; const findPidForPort = deps.findPidForPort ?? findModelRouterPidForPort; const session = deps.loadSession(); const recordedPid = session?.routerPid ?? null; const recordedCredentialHash = session?.routerCredentialHash ?? null; - const pid = ownsPort(recordedPid, port) ? (recordedPid as number) : findPidForPort(port); + const recordedPidOwnsPort = ownsPort(recordedPid, port); + const sessionMatchesDestroyedSandbox = + session !== null && + session.sandboxName === sandbox?.name && + resolveDestroyedSandboxRouterPort(session.endpointUrl) === port; + const sessionOwnsTargetRouter = recordedPidOwnsPort || sessionMatchesDestroyedSandbox; + const pid = recordedPidOwnsPort ? (recordedPid as number) : findPidForPort(port); if (pid !== null) { const log = deps.log ?? console.log; @@ -133,8 +141,17 @@ export async function stopModelRouterForDestroyedSandbox( // Clear when either field is set: a session with only a credential hash // still carries stale router identity after the last routed sandbox is gone. - if (recordedPid !== null || recordedCredentialHash !== null) { + if (sessionOwnsTargetRouter && (recordedPid !== null || recordedCredentialHash !== null)) { deps.updateSession((current: Session) => { + if ( + current.sessionId !== session?.sessionId || + current.sandboxName !== session?.sandboxName || + current.endpointUrl !== session?.endpointUrl || + current.routerPid !== recordedPid || + current.routerCredentialHash !== recordedCredentialHash + ) { + return current; + } current.routerPid = null; current.routerCredentialHash = null; return current; diff --git a/src/lib/actions/sandbox/destroy-model-router-flow.test.ts b/test/package-contract/destroy-model-router-flow.test.ts similarity index 98% rename from src/lib/actions/sandbox/destroy-model-router-flow.test.ts rename to test/package-contract/destroy-model-router-flow.test.ts index d5509f5e6c..ee3a2450f8 100644 --- a/src/lib/actions/sandbox/destroy-model-router-flow.test.ts +++ b/test/package-contract/destroy-model-router-flow.test.ts @@ -12,7 +12,7 @@ import { afterEach, beforeEach, describe, expect, it, type MockInstance, vi } fr import { createDestroyHarness, resetDestroyModuleCache, -} from "../../../../test/helpers/destroy-flow-test-harness"; +} from "../helpers/destroy-flow-test-harness"; // A real detached HTTP server whose command line matches the model-router // proxy shape (venv-style interposition: args[0]=node, args[1]=.../model-router). From 028b96890931b94275e5d9b75b60ba3f127eb847 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Fri, 14 Aug 2026 12:34:16 -0700 Subject: [PATCH 6/7] test(destroy): keep router contract on compiled artifacts --- .../destroy-model-router-flow.test.ts | 61 +++++++++---------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/test/package-contract/destroy-model-router-flow.test.ts b/test/package-contract/destroy-model-router-flow.test.ts index ee3a2450f8..db3a913d50 100644 --- a/test/package-contract/destroy-model-router-flow.test.ts +++ b/test/package-contract/destroy-model-router-flow.test.ts @@ -7,12 +7,10 @@ import net from "node:net"; import os from "node:os"; import path from "node:path"; -import { afterEach, beforeEach, describe, expect, it, type MockInstance, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { - createDestroyHarness, - resetDestroyModuleCache, -} from "../helpers/destroy-flow-test-harness"; +import { stopModelRouterForDestroyedSandbox } from "../../dist/lib/actions/sandbox/destroy-preflight"; +import type { Session } from "../../dist/lib/state/onboard-session"; // A real detached HTTP server whose command line matches the model-router // proxy shape (venv-style interposition: args[0]=node, args[1]=.../model-router). @@ -47,16 +45,10 @@ async function probeHealthy(port: number): Promise { } describe("destroySandbox model-router teardown (#9098)", () => { - let exitSpy: MockInstance; - let originalGatewayEnv: string | undefined; let stubDir: string; let stub: ChildProcess | null = null; beforeEach(() => { - originalGatewayEnv = process.env.OPENSHELL_GATEWAY; - exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number | string | null) => { - throw new Error(`process.exit(${code ?? 0})`); - }) as never); stubDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-router-stub-")); }); @@ -68,12 +60,7 @@ describe("destroySandbox model-router teardown (#9098)", () => { } stub = null; fs.rmSync(stubDir, { recursive: true, force: true }); - originalGatewayEnv === undefined - ? delete process.env.OPENSHELL_GATEWAY - : (process.env.OPENSHELL_GATEWAY = originalGatewayEnv); vi.restoreAllMocks(); - vi.unstubAllEnvs(); - resetDestroyModuleCache(); }); it( @@ -95,32 +82,40 @@ describe("destroySandbox model-router teardown (#9098)", () => { interval: 100, }); - const harness = createDestroyHarness({ + const session = { + sessionId: "router-session", + sandboxName: "alpha", provider: "nvidia-router", endpointUrl: `http://host.openshell.internal:${port}/v1`, - sessionRouterPid: stub.pid, + routerPid: stub.pid, + routerCredentialHash: "router-credential-hash", + } as Session; + const updateSession = vi.fn((mutator: (current: Session) => Session | void) => { + mutator(session); + return session; }); + await expect( - harness.destroySandbox("alpha", { yes: true, cleanupGateway: true }), + stopModelRouterForDestroyedSandbox( + { + name: "alpha", + provider: "nvidia-router", + endpointUrl: session.endpointUrl, + }, + { + listSandboxes: () => ({ sandboxes: [], defaultSandbox: null }), + loadSession: () => session, + updateSession, + }, + ), ).resolves.toBeUndefined(); - // The teardown must run under the gateway route lock so routed - // onboarding cannot register a peer between the scan and the stop. - expect(harness.withGatewayRouteMutationLockSpy).toHaveBeenCalledWith( - "nemoclaw-19080", - expect.any(Function), - ); - await vi.waitFor(() => expect(stubExited).toBe(true), { timeout: 8_000, interval: 100 }); expect(await probeHealthy(port)).toBe(false); - expect(exitSpy).not.toHaveBeenCalled(); - - const sessionsWithRouterPidCleared = harness.updateSessionSpy.mock.results - .map((result) => result.value as { routerPid?: number | null }) - .filter((session) => "routerPid" in session); - expect(sessionsWithRouterPidCleared).toEqual([ + expect(updateSession).toHaveBeenCalledOnce(); + expect(session).toEqual( expect.objectContaining({ routerPid: null, routerCredentialHash: null }), - ]); + ); }, ); }); From 881b1ff2a659ff5ee1c3cd5dee19ffcce650c6cf Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Fri, 14 Aug 2026 12:47:31 -0700 Subject: [PATCH 7/7] test(destroy): cover router teardown guard --- src/lib/actions/sandbox/destroy-flow.test.ts | 37 ++++++++++++++++++++ test/helpers/destroy-flow-test-harness.ts | 8 +++++ 2 files changed, 45 insertions(+) diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 40eac50465..899d2ae984 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -78,6 +78,41 @@ describe("destroySandbox flow", () => { ); }); + it("stops the routed sandbox proxy after registry removal under the gateway route lock (#9098)", async () => { + const harness = createDestroyHarness({ + provider: "nvidia-router", + endpointUrl: "http://host.openshell.internal:4000/v1", + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); + + expect(harness.stopModelRouterForDestroyedSandboxSpy).toHaveBeenCalledOnce(); + expect(harness.withGatewayRouteMutationLockSpy).toHaveBeenCalledWith( + "nemoclaw-19080", + expect.any(Function), + ); + expect(harness.removeSandboxSpy.mock.invocationCallOrder[0]).toBeLessThan( + harness.withGatewayRouteMutationLockSpy.mock.invocationCallOrder[0], + ); + expect(harness.withGatewayRouteMutationLockSpy.mock.invocationCallOrder[0]).toBeLessThan( + harness.stopModelRouterForDestroyedSandboxSpy.mock.invocationCallOrder[0], + ); + }); + + it("does not stop the routed sandbox proxy when registry removal does not complete (#9098)", async () => { + const harness = createDestroyHarness({ + provider: "nvidia-router", + endpointUrl: "http://host.openshell.internal:4000/v1", + removeSandboxResult: false, + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); + + expect(harness.removeSandboxSpy).toHaveBeenCalledWith("alpha"); + expect(harness.withGatewayRouteMutationLockSpy).not.toHaveBeenCalled(); + expect(harness.stopModelRouterForDestroyedSandboxSpy).not.toHaveBeenCalled(); + }); + it.each([ ["--yes", "darwin", { yes: true }, "", true], ["NEMOCLAW_NON_INTERACTIVE=1", "darwin", {}, "1", true], @@ -115,6 +150,8 @@ describe("destroySandbox flow", () => { expectFailedDeletePreservesHostState(harness, exitSpy); expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + expect(harness.withGatewayRouteMutationLockSpy).not.toHaveBeenCalled(); + expect(harness.stopModelRouterForDestroyedSandboxSpy).not.toHaveBeenCalled(); }); it("refuses before destructive work when Docker identity cannot be inspected", async () => { diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index b745f6e9d2..fb09a0647e 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -39,6 +39,7 @@ export type DestroyHarness = { setSandboxPresent: (present: boolean) => void; shieldsDownSpy: MockInstance; stopAllSpy: MockInstance; + stopModelRouterForDestroyedSandboxSpy: MockInstance; stopNimByNameSpy: MockInstance; unloadOllamaModelsSpy: MockInstance; updateSessionSpy: MockInstance; @@ -72,6 +73,7 @@ type DestroyHarnessOptions = { promptResponses?: string[]; provider?: string; registeredSandboxCount?: number; + removeSandboxResult?: boolean; restoreMcpError?: string; sandboxPresent?: boolean; sessionRouterPid?: number; @@ -163,6 +165,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr const onboardSession = requireDist("../../state/onboard-session.js"); const registry = requireDist("../../state/registry.js"); const destroyExecution = requireDist("./destroy-execution.js"); + const destroyPreflight = requireDist("./destroy-preflight.js"); const sandboxSession = requireDist("../../state/sandbox-session.js"); const shields = requireDist("../../shields/index.js"); const timerControl = requireDist("../../shields/timer-control.js"); @@ -209,9 +212,13 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr })), })); const removeSandboxSpy = vi.spyOn(registry, "removeSandbox").mockImplementation(() => { + if (options.removeSandboxResult === false) return false; registeredSandboxCount = Math.max(0, registeredSandboxCount - 1); return true; }); + const stopModelRouterForDestroyedSandboxSpy = vi + .spyOn(destroyPreflight, "stopModelRouterForDestroyedSandbox") + .mockResolvedValue(undefined); const retirePortableLifecycleReceiptSpy = vi .spyOn(destroyExecution, "retirePortableLifecycleAuthority") .mockImplementation(() => undefined); @@ -432,6 +439,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr }, shieldsDownSpy, stopAllSpy, + stopModelRouterForDestroyedSandboxSpy, stopNimByNameSpy, unloadOllamaModelsSpy, updateSessionSpy,