From 9b8958b7b1ae58d0646031e4de85fe1386adfc7b Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 12:18:32 -0700 Subject: [PATCH 01/16] fix(sandbox): serialize model router teardown across gateways Signed-off-by: Prekshi Vyas --- ci/onboard-entry-composition-budget.json | 2 +- docs/inference/set-up-model-router.mdx | 14 +- docs/reference/commands.mdx | 11 +- src/lib/actions/sandbox/destroy-flow.test.ts | 14 ++ .../sandbox/destroy-model-router.test.ts | 144 +++++++++++--- src/lib/actions/sandbox/destroy-preflight.ts | 179 +++++++++++------- src/lib/actions/sandbox/destroy.ts | 11 +- .../gateway-route-mutation-lock.test.ts | 59 +++++- .../inference/gateway-route-mutation-lock.ts | 25 +++ src/lib/onboard.ts | 2 + src/lib/onboard/model-router-process.test.ts | 38 ++-- src/lib/onboard/model-router-process.ts | 33 ++-- src/lib/onboard/model-router.ts | 20 +- .../setup-inference-route-containment.test.ts | 19 +- src/lib/onboard/setup-inference.ts | 21 +- test/helpers/destroy-flow-test-harness.ts | 33 ++-- test/onboard-entry-composition.test.ts | 2 +- .../destroy-model-router-flow.test.ts | 3 +- 18 files changed, 475 insertions(+), 155 deletions(-) diff --git a/ci/onboard-entry-composition-budget.json b/ci/onboard-entry-composition-budget.json index 65c3c4f583e..81dee89018b 100644 --- a/ci/onboard-entry-composition-budget.json +++ b/ci/onboard-entry-composition-budget.json @@ -12,7 +12,7 @@ "provider": { "createSandboxWithBaseImageResolution": 15, "handleNimLocalSelection": 32, - "handleRemoteProviderSelection": 80, + "handleRemoteProviderSelection": 76, "handleRoutedSelection": 15, "runOnboard": 8, "selectAndValidateOllamaModel": 18 diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index aa7694f33c9..bca4271d842 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -32,9 +32,17 @@ 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 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 uses the same host port, destroying one Model Router sandbox keeps that Model Router process running. +Routed onboarding holds a host-wide lock for the selected Model Router port through router setup and sandbox registry publication. +Model Router destruction holds the same port lock through its peer check, process stop, and session update. +When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. +If no same-port peer remains, NemoClaw stops only a process whose command line still identifies the Model Router on that port. +It clears the matching session recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. +If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destroy still completes and keeps the session recovery identity. +The warning tells you to inspect the current listener process immediately before you stop anything. +Stop it only if its command line still identifies the Model Router on the named port. +Do not stop a previously reported process ID when its command line no longer matches. +While another registered Model Router sandbox in any host gateway registry uses the same port, destroying one Model Router sandbox keeps the process running. +A Model Router sandbox on another port does not keep the 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 165466115e5..ff4f969408a 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2293,7 +2293,16 @@ 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 that uses a host port, it also stops the host Model Router process on a best-effort basis and frees that port. +Routed onboarding and Model Router destruction use the same host-wide lifecycle lock for each Model Router port. +Onboarding holds the lock through sandbox registry publication. +Destruction holds the lock through the peer check, process stop, and session update. +When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. +If no same-port peer remains, it stops only a process whose command line still identifies the Model Router on that port. +It clears matching session recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. +If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destruction still completes and keeps the recovery identity. +Inspect the current listener process immediately before you stop anything. +Stop it only if its command line still identifies the Model Router on the named port. +Do not stop a previously reported process ID when its command line no longer matches. 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-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 899d2ae9841..3c6de4a00a5 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -64,6 +64,20 @@ describe("destroySandbox flow", () => { ); }); + it("runs routed teardown under the gateway and host router-port locks (#9098)", async () => { + const harness = createDestroyHarness({ provider: "nvidia-router" }); + + await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); + + expect(harness.withGatewayRouteMutationLockSpy).toHaveBeenCalledWith( + "nemoclaw-19080", + expect.any(Function), + ); + expect(harness.withModelRouterPortLifecycleLockSpy).toHaveBeenCalledWith( + 4000, + expect.any(Function), + ); + }); it("revokes the prior HTTPS-pin route only after confirmed deletion and registry removal", async () => { const routeId = "a".repeat(64); const harness = createDestroyHarness({ diff --git a/src/lib/actions/sandbox/destroy-model-router.test.ts b/src/lib/actions/sandbox/destroy-model-router.test.ts index a7bea07fb8b..685efbbdff2 100644 --- a/src/lib/actions/sandbox/destroy-model-router.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it, vi } from "vitest"; +import type { GatewayRegistryEntry } from "../../state/gateway-registry"; import type { Session } from "../../state/onboard-session"; import type { SandboxEntry } from "../../state/registry"; import { @@ -25,10 +26,11 @@ function createDeps(overrides: Partial = routerPid: 4242, routerCredentialHash: "hash", } as Session; - const deps = { - findPidForPort: vi.fn(() => null), + const deps: StopModelRouterForDestroyedSandboxDeps = { + inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), + isHealthy: vi.fn(async () => false), isRoutedProvider: vi.fn((provider: string | null | undefined) => provider === "nvidia-router"), - listSandboxes: vi.fn(() => ({ sandboxes: [] as SandboxEntry[], defaultSandbox: null })), + listHostRegistryEntries: vi.fn(() => []), loadSession: vi.fn(() => session), log: vi.fn(), ownsPort: vi.fn(() => true), @@ -38,6 +40,8 @@ function createDeps(overrides: Partial = return session; }), warn: vi.fn(), + withModelRouterPortLifecycleLock: async (_port: number, operation: () => Promise | T) => + await operation(), ...overrides, }; return { deps, session }; @@ -88,18 +92,20 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(deps.updateSession).not.toHaveBeenCalled(); }); - it("keeps the router while another registered routed sandbox remains", async () => { + it("keeps the router while a sandbox in another gateway state root uses the same port", async () => { const { deps } = createDeps({ - listSandboxes: vi.fn(() => ({ - sandboxes: [ - { + listHostRegistryEntries: vi.fn(() => [ + { + entry: { name: "beta", provider: "nvidia-router", - endpointUrl: "http://host.openshell.internal:4100/v1", - } as SandboxEntry, - ], - defaultSandbox: null, - })), + endpointUrl: routedSandbox.endpointUrl, + } as GatewayRegistryEntry, + gatewayPort: 9090, + registryFile: "/tmp/gateways/9090/sandboxes.json", + stateRoot: "/tmp/gateways/9090", + }, + ]), }); await stopModelRouterForDestroyedSandbox(routedSandbox, deps); @@ -110,16 +116,18 @@ describe("stopModelRouterForDestroyedSandbox", () => { it("stops the target router when a routed peer uses a different port", async () => { const { deps } = createDeps({ - listSandboxes: vi.fn(() => ({ - sandboxes: [ - { + listHostRegistryEntries: vi.fn(() => [ + { + entry: { name: "beta", provider: "nvidia-router", endpointUrl: "http://host.openshell.internal:4200/v1", - } as SandboxEntry, - ], - defaultSandbox: null, - })), + } as GatewayRegistryEntry, + gatewayPort: 9090, + registryFile: "/tmp/gateways/9090/sandboxes.json", + stateRoot: "/tmp/gateways/9090", + }, + ]), }); await stopModelRouterForDestroyedSandbox(routedSandbox, deps); @@ -130,20 +138,60 @@ describe("stopModelRouterForDestroyedSandbox", () => { 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), + inspectProcessForPort: vi.fn(() => ({ status: "found" as const, pid: 5151 })), }); await stopModelRouterForDestroyedSandbox(routedSandbox, deps); - expect(deps.findPidForPort).toHaveBeenCalledWith(4100); + expect(deps.inspectProcessForPort).toHaveBeenCalledWith(4100); expect(deps.stopProcess).toHaveBeenCalledWith(5151, 4100); expect(session.routerPid).toBeNull(); }); + it("stops a target-port orphan without clearing another sandbox session", async () => { + const unrelatedSession = { + sandboxName: "beta", + endpointUrl: "http://host.openshell.internal:4200/v1", + routerPid: 6262, + routerCredentialHash: "beta-hash", + } as Session; + const { deps } = createDeps({ + loadSession: vi.fn(() => unrelatedSession), + ownsPort: vi.fn(() => false), + inspectProcessForPort: vi.fn(() => ({ status: "found" as const, pid: 5151 })), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.stopProcess).toHaveBeenCalledWith(5151, 4100); + expect(deps.updateSession).not.toHaveBeenCalled(); + expect(unrelatedSession.routerPid).toBe(6262); + expect(unrelatedSession.routerCredentialHash).toBe("beta-hash"); + }); + + it("preserves a reused sandbox-name session for another router port", async () => { + const reusedNameSession = { + sandboxName: "alpha", + endpointUrl: "http://host.openshell.internal:4200/v1", + routerPid: 6262, + routerCredentialHash: "new-hash", + } as Session; + const { deps } = createDeps({ + loadSession: vi.fn(() => reusedNameSession), + ownsPort: vi.fn(() => false), + inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.updateSession).not.toHaveBeenCalled(); + expect(reusedNameSession.routerPid).toBe(6262); + expect(reusedNameSession.routerCredentialHash).toBe("new-hash"); + }); 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), + inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), }); await stopModelRouterForDestroyedSandbox(routedSandbox, deps); @@ -153,6 +201,37 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(session.routerCredentialHash).toBeNull(); }); + it("keeps session identity when process inventory is unavailable and the port is healthy", async () => { + const { deps, session } = createDeps({ + ownsPort: vi.fn(() => false), + inspectProcessForPort: vi.fn(() => ({ status: "unavailable" as const })), + isHealthy: vi.fn(async () => true), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(deps.updateSession).not.toHaveBeenCalled(); + expect(session.routerPid).toBe(4242); + expect(session.routerCredentialHash).toBe("hash"); + expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("process inventory")); + }); + + it("keeps session identity when no process is visible but the router port stays healthy", async () => { + const { deps, session } = createDeps({ + ownsPort: vi.fn(() => false), + inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), + isHealthy: vi.fn(async () => true), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.updateSession).not.toHaveBeenCalled(); + expect(session.routerPid).toBe(4242); + expect(session.routerCredentialHash).toBe("hash"); + expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("healthy port 4100")); + }); + it("clears a stale credential hash when the session records no router PID (#9098)", async () => { const session = { sessionId: "session-alpha", @@ -232,8 +311,27 @@ describe("stopModelRouterForDestroyedSandbox", () => { 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.warn).toHaveBeenCalledWith(expect.stringContaining("Inspect PID 4242")); + expect(deps.warn).not.toHaveBeenCalledWith(expect.stringContaining("kill 4242")); expect(deps.updateSession).not.toHaveBeenCalled(); expect(session.routerPid).toBe(4242); }); + + it("does not recommend a PID stop when ownership changes after a failed shutdown", async () => { + let ownershipChecks = 0; + const { deps } = createDeps({ + ownsPort: vi.fn(() => { + ownershipChecks += 1; + return ownershipChecks === 1; + }), + stopProcess: vi.fn(async () => { + throw new Error("ownership changed during shutdown"); + }), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("Do not stop it by PID")); + expect(deps.warn).not.toHaveBeenCalledWith(expect.stringContaining("kill 4242")); + }); }); diff --git a/src/lib/actions/sandbox/destroy-preflight.ts b/src/lib/actions/sandbox/destroy-preflight.ts index a27b64c62b1..0cb7e43159a 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -1,13 +1,18 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import os from "node:os"; + import { OPENSHELL_PROBE_TIMEOUT_MS } from "../../adapters/openshell/timeouts"; +import { withModelRouterPortLifecycleLock } from "../../inference/gateway-route-mutation-lock"; import { isRoutedInferenceProvider } from "../../onboard/model-router"; import { doesModelRouterProcessOwnPort, - findModelRouterPidForPort, + inspectModelRouterProcessForPort, + isRouterHealthy, stopModelRouterProcess, } from "../../onboard/model-router-process"; +import { listHostGatewayRegistryEntries } from "../../state/gateway-registry"; import type { Session } from "../../state/onboard-session"; import type { SandboxEntry } from "../../state/registry"; import * as registry from "../../state/registry"; @@ -56,13 +61,16 @@ const DEFAULT_MODEL_ROUTER_PORT = 4000; export type StopModelRouterForDestroyedSandboxDeps = { loadSession: () => Session | null; updateSession: (mutator: (session: Session) => Session | void) => Session; - findPidForPort?: typeof findModelRouterPidForPort; + inspectProcessForPort?: typeof inspectModelRouterProcessForPort; + isHealthy?: typeof isRouterHealthy; isRoutedProvider?: typeof isRoutedInferenceProvider; - listSandboxes?: typeof registry.listSandboxes; + listHostRegistryEntries?: typeof listHostGatewayRegistryEntries; log?: (message: string) => void; ownsPort?: typeof doesModelRouterProcessOwnPort; + resolveHomeDir?: () => string; stopProcess?: (pid: number, port: number) => Promise; warn?: (message: string) => void; + withModelRouterPortLifecycleLock?: typeof withModelRouterPortLifecycleLock; }; export function resolveDestroyedSandboxRouterPort(endpointUrl: string | null | undefined): number { @@ -82,81 +90,118 @@ export function resolveDestroyedSandboxRouterPort(endpointUrl: string | null | u * 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. + * and no registered routed sandbox in any gateway state root uses the same + * router port, so a same-port 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. + * The recorded PID is preferred only when the session sandbox and router port + * match the destroyed sandbox. Otherwise, the /proc scan recovers the orphan + * by verified command line, exactly like reconcileModelRouter's recovery path. + * The host-global port lock covers the peer scan, stop, and session update so + * routed onboarding on another gateway cannot publish a peer between them. A + * stop failure or inconclusive process inventory is a warning, not an error: + * the sandbox delete already succeeded. The matching session keeps routerPid + * and credential identity so uninstall and reconcile retain recovery evidence. */ export async function stopModelRouterForDestroyedSandbox( sandbox: SandboxEntry | null, deps: StopModelRouterForDestroyedSandboxDeps, ): 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) && - resolveDestroyedSandboxRouterPort(entry.endpointUrl) === port, - ); - if (routedPeerRemains) return; - - 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 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; + if (!sandbox || !isRoutedProvider(sandbox.provider)) return; + const port = resolveDestroyedSandboxRouterPort(sandbox.endpointUrl); + const withPortLock = deps.withModelRouterPortLifecycleLock ?? withModelRouterPortLifecycleLock; + await withPortLock(port, async () => { + const listHostRegistryEntries = deps.listHostRegistryEntries ?? listHostGatewayRegistryEntries; + const home = (deps.resolveHomeDir ?? (() => process.env.HOME || os.homedir()))(); + // Called after selected-registry removal, so every remaining host entry is + // a peer, including entries owned by a different gateway state root. + const routedPeerRemains = listHostRegistryEntries(home).some(({ entry }) => { + const provider = typeof entry.provider === "string" ? entry.provider : null; + const endpointUrl = typeof entry.endpointUrl === "string" ? entry.endpointUrl : null; + return isRoutedProvider(provider) && resolveDestroyedSandboxRouterPort(endpointUrl) === port; + }); + if (routedPeerRemains) return; + + const ownsPort = deps.ownsPort ?? doesModelRouterProcessOwnPort; + const inspectProcessForPort = deps.inspectProcessForPort ?? inspectModelRouterProcessForPort; + const isHealthy = deps.isHealthy ?? isRouterHealthy; 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 onboarding, or onboarding fails with "Port ${port} already has a healthy router endpoint".`, - ); - return; + const session = deps.loadSession(); + const sessionMatchesSandbox = + session?.sandboxName === sandbox.name && + resolveDestroyedSandboxRouterPort(session.endpointUrl) === port; + const recordedPid = sessionMatchesSandbox ? (session.routerPid ?? null) : null; + const recordedCredentialHash = sessionMatchesSandbox + ? (session.routerCredentialHash ?? null) + : null; + let pid: number | null = null; + if (ownsPort(recordedPid, port)) { + pid = recordedPid as number; + } else { + const lookup = inspectProcessForPort(port); + if (lookup.status === "unavailable") { + warn( + `Could not inspect the host process inventory for the Model Router on port ${port}. ` + + "Keeping its session recovery identity; inspect the port listener before the next Model Router onboarding.", + ); + return; + } + if (lookup.status === "found") { + pid = lookup.pid; + } else if (await isHealthy(port, 1000)) { + warn( + `No Model Router process could be confirmed for healthy port ${port}. ` + + "Keeping its session recovery identity; inspect the port listener before the next Model Router onboarding.", + ); + return; + } } - } - // 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 (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; + if (pid !== null) { + const log = deps.log ?? console.log; + 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) + }`, + ); + if (ownsPort(pid, port)) { + warn( + `Inspect PID ${pid} and the listener on port ${port}; stop the process only after confirming it is still the matching model-router proxy.`, + ); + } else { + warn( + `PID ${pid} no longer identifies the matching model-router proxy. Do not stop it by PID; inspect the listener on port ${port}.`, + ); + } + return; } - current.routerPid = null; - current.routerCredentialHash = null; - return current; - }); - } + } + + // Clear when either field is set: a matching session with only a + // credential hash still carries stale router identity after its sandbox + // is gone. A completed process scan plus an unhealthy port confirms that + // no router remains when no PID was found. + if (sessionMatchesSandbox && (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; + }); + } + }); } export function prepareSandboxDestroy(sandboxName: string): SandboxDestroyPreflight { diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 6ee874182cb..921dff7f491 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -690,10 +690,10 @@ async function destroySandboxUnlocked( } if (deleteSucceededOrAlreadyGone && removed) { 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 concurrent onboarding can register a - // routed sandbox after the scan and then lose its shared router. + // The gateway route lock nests the host-global router-port lock inside + // stopModelRouterForDestroyedSandbox. Routed onboarding takes the same + // lock order and holds both through registry publication, including + // when the competing sandbox belongs to another gateway. await withGatewayRouteMutationLock(cleanupGatewayName, () => stopModelRouterForDestroyedSandbox(sandbox, { loadSession: onboardSession.loadSession, @@ -705,7 +705,8 @@ async function destroySandboxUnlocked( const detail = error instanceof Error ? error.message : String(error); defaultDestroyWarn( `Sandbox deletion succeeded, but the Model Router teardown did not complete: ${detail}. ` + - `Stop the Model Router process manually before the next Model Router onboarding.`, + `Inspect the listener before the next Model Router onboarding and stop only a process ` + + `that still owns the matching port and model-router command line.`, ); } } diff --git a/src/lib/inference/gateway-route-mutation-lock.test.ts b/src/lib/inference/gateway-route-mutation-lock.test.ts index 4185e35cc7c..ba7cb9f97af 100644 --- a/src/lib/inference/gateway-route-mutation-lock.test.ts +++ b/src/lib/inference/gateway-route-mutation-lock.test.ts @@ -4,8 +4,11 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { describe, expect, it } from "vitest"; -import { withGatewayRouteMutationLock } from "./gateway-route-mutation-lock"; +import { describe, expect, it, vi } from "vitest"; +import { + withGatewayRouteMutationLock, + withModelRouterPortLifecycleLock, +} from "./gateway-route-mutation-lock"; describe("gateway route mutation lock", () => { it("serializes separate operations for the same gateway", async () => { @@ -81,4 +84,56 @@ describe("gateway route mutation lock", () => { await fs.rm(stateDir, { recursive: true, force: true }); } }); + + it("keeps cross-gateway router onboarding publication ahead of teardown", async () => { + const homeDir = await fs.mkdtemp(path.join(os.tmpdir(), "nemoclaw-router-port-lock-")); + const homedirSpy = vi.spyOn(os, "homedir").mockReturnValue(homeDir); + let publishPeer!: () => void; + const peerPublished = new Promise((resolve) => { + publishPeer = resolve; + }); + let reportOnboardEntered!: () => void; + const onboardEntered = new Promise((resolve) => { + reportOnboardEntered = resolve; + }); + const events: string[] = []; + const options = { pollIntervalMs: 1, timeoutMs: 5_000 }; + try { + vi.stubEnv("NEMOCLAW_GATEWAY_PORT", "18080"); + vi.resetModules(); + const firstGateway = await import("./gateway-route-mutation-lock"); + const onboarding = firstGateway.withModelRouterPortLifecycleLock( + 4000, + async () => { + events.push("onboard-enter"); + reportOnboardEntered(); + await peerPublished; + events.push("peer-published"); + }, + options, + ); + await onboardEntered; + vi.stubEnv("NEMOCLAW_GATEWAY_PORT", "18081"); + vi.resetModules(); + const secondGateway = await import("./gateway-route-mutation-lock"); + const teardown = secondGateway.withModelRouterPortLifecycleLock( + 4000, + () => { + events.push("teardown-peer-scan"); + }, + options, + ); + + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(events).toEqual(["onboard-enter"]); + publishPeer(); + await Promise.all([onboarding, teardown]); + expect(events).toEqual(["onboard-enter", "peer-published", "teardown-peer-scan"]); + } finally { + publishPeer(); + vi.unstubAllEnvs(); + homedirSpy.mockRestore(); + await fs.rm(homeDir, { recursive: true, force: true }); + } + }); }); diff --git a/src/lib/inference/gateway-route-mutation-lock.ts b/src/lib/inference/gateway-route-mutation-lock.ts index 1e64c3ed06a..46749163c10 100644 --- a/src/lib/inference/gateway-route-mutation-lock.ts +++ b/src/lib/inference/gateway-route-mutation-lock.ts @@ -1,9 +1,18 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import os from "node:os"; +import path from "node:path"; + import { type McpLifecycleLockOptions, withMcpLifecycleLock } from "../state/mcp-lifecycle-lock"; +import { resolveSharedLocalAdapterStateRoot } from "./local-adapter-lifecycle"; const GATEWAY_ROUTE_LOCK_PREFIX = "gateway-route:"; +const MODEL_ROUTER_PORT_LOCK_PREFIX = "model-router-port:"; + +export function resolveHostGlobalModelRouterLockStateDir(homeDir: string = os.homedir()): string { + return path.join(resolveSharedLocalAdapterStateRoot(homeDir), "state"); +} /** * Serializes host-side reads and writes of OpenShell's one-route-per-gateway @@ -23,3 +32,19 @@ export function withGatewayRouteMutationLock( options, ); } + +/** Serialize lifecycle changes for the host-global Model Router port. */ +export function withModelRouterPortLifecycleLock( + port: number, + operation: () => Promise | T, + options: McpLifecycleLockOptions = {}, +): Promise { + if (!Number.isInteger(port) || port < 1 || port > 65535) { + throw new Error("Model Router port must be an integer from 1 to 65535."); + } + const stateDir = options.stateDir ?? resolveHostGlobalModelRouterLockStateDir(); + return withMcpLifecycleLock(`${MODEL_ROUTER_PORT_LOCK_PREFIX}${String(port)}`, operation, { + ...options, + stateDir, + }); +} diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index b37ad45c184..18309c004d4 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -2781,6 +2781,8 @@ function getSetupInferenceDeps(): SetupInferenceDeps { return { checkGatewayRouteCompatibility, withGatewayRouteMutationLock: gatewayRouteMutationLock.withGatewayRouteMutationLock, + withModelRouterPortLifecycleLock: gatewayRouteMutationLock.withModelRouterPortLifecycleLock, + getModelRouterPort: modelRouter.resolveModelRouterPort, withSandboxMutationLock: sandboxMutationLock.withSandboxMutationLock, step, getGatewayName: () => GATEWAY_NAME, diff --git a/src/lib/onboard/model-router-process.test.ts b/src/lib/onboard/model-router-process.test.ts index 732156ab1c8..a6ee036de04 100644 --- a/src/lib/onboard/model-router-process.test.ts +++ b/src/lib/onboard/model-router-process.test.ts @@ -7,8 +7,8 @@ import type { AddressInfo } from "node:net"; import { describe, expect, it, vi } from "vitest"; import { - findModelRouterPidForPort, getRouterHealthSnapshot, + inspectModelRouterProcessForPort, stopModelRouterProcess, } from "./model-router-process"; @@ -103,20 +103,20 @@ describe("getRouterHealthSnapshot (#8962)", () => { const ROUTER_ARGS = ["/opt/model-router", "proxy", "--port", "4000"]; -describe("findModelRouterPidForPort", () => { +describe("inspectModelRouterProcessForPort", () => { it("returns the PID when a model-router proxy is found via direct proc scan (#5169)", () => { - const pid = findModelRouterPidForPort(4000, { + const result = inspectModelRouterProcessForPort(4000, { readProcCommandLine: (p) => p === 12345 ? ["/home/user/.nemoclaw/model-router-venv/bin/model-router", "proxy", "--port", "4000"] : null, listProcPids: () => [1, 100, 12345, 99999], }); - expect(pid).toBe(12345); + expect(result).toEqual({ status: "found", pid: 12345 }); }); it("returns the PID when model-router is Python-interpreted through args[1] (#5169)", () => { - const pid = findModelRouterPidForPort(4000, { + const result = inspectModelRouterProcessForPort(4000, { readProcCommandLine: (p) => p === 12345 ? [ @@ -129,30 +129,40 @@ describe("findModelRouterPidForPort", () => { : null, listProcPids: () => [1, 100, 12345, 99999], }); - expect(pid).toBe(12345); + expect(result).toEqual({ status: "found", pid: 12345 }); }); - it("returns null when no model-router is found on that port", () => { - const pid = findModelRouterPidForPort(4000, { + it("reports absence when no model-router is found on that port", () => { + const result = inspectModelRouterProcessForPort(4000, { readProcCommandLine: (p) => p === 12345 ? ["/home/user/.nemoclaw/model-router-venv/bin/model-router", "proxy", "--port", "9999"] : null, listProcPids: () => [12345], }); - expect(pid).toBe(null); + expect(result).toEqual({ status: "absent" }); }); - it("returns null when listProcPids returns an empty list", () => { - const pid = findModelRouterPidForPort(4000, { + it("reports absence when the process inventory is empty", () => { + const result = inspectModelRouterProcessForPort(4000, { readProcCommandLine: () => null, listProcPids: () => [], }); - expect(pid).toBe(null); + expect(result).toEqual({ status: "absent" }); + }); + + it("reports an unavailable process inventory separately from absence", () => { + const result = inspectModelRouterProcessForPort(4000, { + listProcPids: () => { + throw new Error("process inventory unavailable"); + }, + }); + + expect(result).toEqual({ status: "unavailable" }); }); it("returns the first matching PID when multiple model-routers are present", () => { - const pid = findModelRouterPidForPort(4000, { + const result = inspectModelRouterProcessForPort(4000, { readProcCommandLine: (p) => { if (p === 100) return ["/opt/model-router", "proxy", "--port", "4000"]; if (p === 200) return ["/opt/model-router", "proxy", "--port", "4000"]; @@ -160,7 +170,7 @@ describe("findModelRouterPidForPort", () => { }, listProcPids: () => [50, 100, 200], }); - expect(pid).toBe(100); + expect(result).toEqual({ status: "found", pid: 100 }); }); }); diff --git a/src/lib/onboard/model-router-process.ts b/src/lib/onboard/model-router-process.ts index 882f65021ae..ed3a09aa735 100644 --- a/src/lib/onboard/model-router-process.ts +++ b/src/lib/onboard/model-router-process.ts @@ -27,6 +27,11 @@ type ModelRouterCommandLineReaderDeps = { listProcPids?: () => number[]; }; +export type ModelRouterProcessLookup = + | { status: "found"; pid: number } + | { status: "absent" } + | { status: "unavailable" }; + export type RouterHealthSnapshot = { healthy: boolean; body: string | null; @@ -255,34 +260,34 @@ export async function stopModelRouterProcess( /** * Scan /proc for a model-router process bound to `port`. * - * Used by reconcileModelRouter to auto-recover orphaned routers whose PID - * was not recorded in the current session (e.g. after a failed install left a - * running router and the next session starts fresh). Returns null when /proc - * is unavailable (macOS) or no matching process is found. + * Used by reconcileModelRouter and destroy to recover orphaned routers whose + * PID was not recorded in the matching session. The result distinguishes a + * completed scan with no match from an unavailable process inventory so + * teardown does not erase recovery identity on inconclusive evidence. */ -export function findModelRouterPidForPort( +export function inspectModelRouterProcessForPort( port: number, deps: ModelRouterCommandLineReaderDeps = {}, -): number | null { +): ModelRouterProcessLookup { let pids: number[]; - if (deps.listProcPids) { - pids = deps.listProcPids(); - } else { - try { + try { + if (deps.listProcPids) { + pids = deps.listProcPids(); + } else { pids = fs .readdirSync("/proc") .map(Number) .filter((n) => Number.isFinite(n) && n > 0); - } catch { - return null; } + } catch { + return { status: "unavailable" }; } const readCmdLine = deps.readProcCommandLine ?? readProcCommandLine; for (const pid of pids) { const args = readCmdLine(pid); - if (args && isModelRouterCommandLineForPort(args, port)) return pid; + if (args && isModelRouterCommandLineForPort(args, port)) return { status: "found", pid }; } - return null; + return { status: "absent" }; } export async function stopTrackedModelRouterForAgentChange( diff --git a/src/lib/onboard/model-router.ts b/src/lib/onboard/model-router.ts index 6dc2fff3455..adb426067f7 100644 --- a/src/lib/onboard/model-router.ts +++ b/src/lib/onboard/model-router.ts @@ -37,8 +37,8 @@ import { } from "./model-router-command"; import { doesModelRouterProcessOwnPort, - findModelRouterPidForPort, getRouterHealthSnapshot, + inspectModelRouterProcessForPort, isRouterHealthy, ROUTER_HEALTH_TIMEOUT_MS as ROUTER_HEALTH_REQUEST_TIMEOUT_MS, type RouterHealthSnapshot, @@ -577,6 +577,10 @@ function getRoutedProfile(): BlueprintInferenceProfile { return bp; } +export function resolveModelRouterPort(): number { + return getRoutedProfile().router?.port || 4000; +} + export function isRoutedInferenceProvider(provider: string | null | undefined): boolean { if (!provider) return false; if (provider === "nvidia-router") return true; @@ -615,7 +619,7 @@ async function verifyModelRouterSandboxReachability(routerPort: number): Promise export async function reconcileModelRouter(): Promise { const bp = getRoutedProfile(); - const routerPort = bp.router.port || 4000; + const routerPort = resolveModelRouterPort(); const routerCredentialEnv = bp.router.credential_env || bp.credential_env || DEFAULT_MODEL_ROUTER_CREDENTIAL_ENV; const routerCredential = @@ -653,13 +657,15 @@ export async function reconcileModelRouter(): Promise { // requiring a manual stop-and-retry. Only stop it if the cmdline // confirms it is actually model-router proxy — never kill an unrelated // service that happens to occupy the port. See issue #5169. - const orphanPid = findModelRouterPidForPort(routerPort); - if (orphanPid !== null) { - console.log(` Stopping orphaned model router (PID ${orphanPid})...`); - await stopModelRouterProcess(orphanPid, routerPort); + const orphan = inspectModelRouterProcessForPort(routerPort); + if (orphan.status === "found") { + console.log(` Stopping orphaned model router (PID ${orphan.pid})...`); + await stopModelRouterProcess(orphan.pid, routerPort); } else { + const inventoryDetail = + orphan.status === "unavailable" ? " The host process inventory is unavailable." : ""; throw new Error( - `Port ${routerPort} already has a healthy router endpoint, but its credential state is unknown. Stop the existing model-router process and rerun onboarding.`, + `Port ${routerPort} already has a healthy router endpoint, but its credential state is unknown.${inventoryDetail} Stop the existing model-router process and rerun onboarding.`, ); } } diff --git a/src/lib/onboard/setup-inference-route-containment.test.ts b/src/lib/onboard/setup-inference-route-containment.test.ts index 30703c9da72..a837c36941c 100644 --- a/src/lib/onboard/setup-inference-route-containment.test.ts +++ b/src/lib/onboard/setup-inference-route-containment.test.ts @@ -123,7 +123,10 @@ describe("onboard shared gateway route containment", () => { events.push("openshell"); return { status: 0 }; }); - const updateSandbox = vi.fn(() => true); + const updateSandbox = vi.fn(() => { + events.push("registry-published"); + return true; + }); const upsertProvider = vi.fn(() => ({ ok: true })); const verifyInferenceRoute = vi.fn(); const verifyOnboardInferenceSmoke = vi.fn(); @@ -161,6 +164,14 @@ describe("onboard shared gateway route containment", () => { events.push("lock"); return await operation(); }, + withModelRouterPortLifecycleLock: async ( + port: number, + operation: () => Promise | T, + ) => { + events.push(`router-port-lock:${String(port)}`); + return await operation(); + }, + getModelRouterPort: () => 4100, step: () => events.push("step"), getGatewayName, runOpenshell, @@ -191,8 +202,9 @@ describe("onboard shared gateway route containment", () => { setupInference("new-sandbox", "model-b", "router-b", "http://router-b.test/v1", "ROUTER_KEY"), ).resolves.toEqual({ ok: true }); - expect(events.slice(0, 4)).toEqual([ + expect(events.slice(0, 5)).toEqual([ "lock", + "router-port-lock:4100", "guard", expect.stringContaining("error: Warning: Onboarding 'new-sandbox' will re-point"), "step", @@ -208,6 +220,9 @@ describe("onboard shared gateway route containment", () => { expect(verifyInferenceRoute).toHaveBeenCalledWith("nemoclaw-9090", "router-b", "model-b"); expect(verifyOnboardInferenceSmoke).toHaveBeenCalledOnce(); expect(updateSandbox).toHaveBeenCalledOnce(); + expect(events.indexOf("router-port-lock:4100")).toBeLessThan( + events.indexOf("registry-published"), + ); expect(error).toHaveBeenCalledWith(expect.stringContaining("stopped-sandbox")); expect(exitProcess).not.toHaveBeenCalled(); }); diff --git a/src/lib/onboard/setup-inference.ts b/src/lib/onboard/setup-inference.ts index 3f489f5d64f..4e0d12520a1 100644 --- a/src/lib/onboard/setup-inference.ts +++ b/src/lib/onboard/setup-inference.ts @@ -14,7 +14,10 @@ import { formatGatewayRouteImpactWarning, isAdvisoryGatewayRouteConflict, } from "../inference/gateway-route-compatibility"; -import { withGatewayRouteMutationLock } from "../inference/gateway-route-mutation-lock"; +import { + withGatewayRouteMutationLock, + withModelRouterPortLifecycleLock, +} from "../inference/gateway-route-mutation-lock"; import { getManagedVllmProviderBinding } from "../inference/local"; import { type OllamaModelHolder, @@ -35,6 +38,7 @@ import { withSandboxMutationLock } from "../state/mcp-lifecycle-lock"; import type { Session } from "../state/onboard-session"; import { createSandboxHostLocalInferenceProvenance } from "../state/registry/host-local-inference"; import { shouldFrontOllamaWithProxy } from "./local-inference-topology"; +import { resolveModelRouterPort } from "./model-router"; export { assertNoOpenShellGatewayEndpointOverride }; @@ -185,6 +189,8 @@ export type SetupInferenceDeps = ProviderBranchDeps & { trustedPrivateEndpointHosts?: readonly string[]; checkGatewayRouteCompatibility: CurrentGatewayRouteCompatibilityCheck; withGatewayRouteMutationLock: typeof withGatewayRouteMutationLock; + withModelRouterPortLifecycleLock?: typeof withModelRouterPortLifecycleLock; + getModelRouterPort?: () => number; withSandboxMutationLock: typeof withSandboxMutationLock; step: (current: number, total: number, label: string) => void; getGatewayName: () => string; @@ -509,9 +515,18 @@ export function createSetupInference( const gatewayName = options.gatewayName ?? deps.getGatewayName(); const endpointSource = options.endpointSource === undefined ? "onboard" : options.endpointSource; + const routedProvider = deps.isRoutedInferenceProvider?.(provider) === true; + const withInferenceMutationLocks = (operation: () => Promise | T): Promise => + deps.withGatewayRouteMutationLock(gatewayName, () => { + if (!routedProvider) return operation(); + const withRouterPortLock = + deps.withModelRouterPortLifecycleLock ?? withModelRouterPortLifecycleLock; + const port = (deps.getModelRouterPort ?? resolveModelRouterPort)(); + return withRouterPortLock(port, operation); + }); const mutateGatewayRoute = (): Promise => // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: provider onboarding centralizes route and two-phase transaction ordering. - deps.withGatewayRouteMutationLock(gatewayName, async () => { + withInferenceMutationLocks(async () => { if ( options.isRecordedProviderRecoveryAuthorized && !options.isRecordedProviderRecoveryAuthorized() @@ -883,7 +898,7 @@ export function createSetupInference( } return outcome.result; } - } else if (deps.isRoutedInferenceProvider(provider)) { + } else if (routedProvider) { await inferenceProviders.setupRoutedInference( { model, provider, endpointUrl, credentialEnv }, { diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index fb09a0647e0..2b949f45526 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -45,6 +45,7 @@ export type DestroyHarness = { updateSessionSpy: MockInstance; warnSpy: MockInstance; withGatewayRouteMutationLockSpy: MockInstance; + withModelRouterPortLifecycleLockSpy: MockInstance; }; type DestroyHarnessOptions = { @@ -160,9 +161,11 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr const nim = requireDist("../../inference/nim.js"); const ollamaProxy = requireDist("../../inference/ollama/proxy.js"); const gatewayRouteMutationLock = requireDist("../../inference/gateway-route-mutation-lock.js"); + const modelRouterProcess = requireDist("../../onboard/model-router-process.js"); const httpsPinRuntimeAdapter = requireDist("../../inference/https-pin-runtime-adapter.js"); const tunnelServices = requireDist("../../tunnel/services.js"); const onboardSession = requireDist("../../state/onboard-session.js"); + const gatewayRegistry = requireDist("../../state/gateway-registry.js"); const registry = requireDist("../../state/registry.js"); const destroyExecution = requireDist("./destroy-execution.js"); const destroyPreflight = requireDist("./destroy-preflight.js"); @@ -232,6 +235,17 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr .mockImplementation(async (_gatewayName: unknown, operation: unknown) => (operation as () => Promise)(), ); + const withModelRouterPortLifecycleLockSpy = vi + .spyOn(gatewayRouteMutationLock, "withModelRouterPortLifecycleLock") + .mockImplementation(async (_port: unknown, operation: unknown) => + (operation as () => Promise)(), + ); + vi.spyOn(gatewayRegistry, "listHostGatewayRegistryEntries").mockReturnValue([]); + vi.spyOn(modelRouterProcess, "doesModelRouterProcessOwnPort").mockReturnValue(false); + vi.spyOn(modelRouterProcess, "inspectModelRouterProcessForPort").mockReturnValue({ + status: "absent", + }); + vi.spyOn(modelRouterProcess, "isRouterHealthy").mockResolvedValue(false); vi.spyOn(onboardSession, "loadSession").mockReturnValue({ sandboxName: "alpha", ...(options.sessionRouterPid ? { routerPid: options.sessionRouterPid } : {}), @@ -302,10 +316,8 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr } identityProbeCall += 1; options.onDockerRun?.(identityProbeCall); - const result = - options.dockerRunResultSequence?.[identityProbeCall - 1] ?? - options.dockerRunResult ?? - { status: 0 }; + const result = options.dockerRunResultSequence?.[identityProbeCall - 1] ?? + options.dockerRunResult ?? { status: 0 }; return result as ReturnType; }); const selectGatewaySpy = vi @@ -321,13 +333,11 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr vi.spyOn(sandboxProviderCleanup, "emitProviderDetachResidualHint").mockImplementation( () => undefined, ); - const stopNimByNameSpy = vi - .spyOn(nim, "stopNimContainerByName") - .mockImplementation(() => { - if (options.stopInferenceError !== undefined) { - throw new Error(options.stopInferenceError); - } - }); + const stopNimByNameSpy = vi.spyOn(nim, "stopNimContainerByName").mockImplementation(() => { + if (options.stopInferenceError !== undefined) { + throw new Error(options.stopInferenceError); + } + }); vi.spyOn(nim, "stopNimContainer").mockImplementation(() => undefined); const killStaleProxySpy = vi .spyOn(ollamaProxy, "killStaleProxy") @@ -445,5 +455,6 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr updateSessionSpy, warnSpy, withGatewayRouteMutationLockSpy, + withModelRouterPortLifecycleLockSpy, }; } diff --git a/test/onboard-entry-composition.test.ts b/test/onboard-entry-composition.test.ts index 54a82c5dad8..ba824fae7fd 100644 --- a/test/onboard-entry-composition.test.ts +++ b/test/onboard-entry-composition.test.ts @@ -36,7 +36,7 @@ describe("onboarding entry composition boundary", () => { provider: { createSandboxWithBaseImageResolution: 15, handleNimLocalSelection: 32, - handleRemoteProviderSelection: 80, + handleRemoteProviderSelection: 76, handleRoutedSelection: 15, runOnboard: 8, selectAndValidateOllamaModel: 18, diff --git a/test/package-contract/destroy-model-router-flow.test.ts b/test/package-contract/destroy-model-router-flow.test.ts index db3a913d503..66aea4d35e4 100644 --- a/test/package-contract/destroy-model-router-flow.test.ts +++ b/test/package-contract/destroy-model-router-flow.test.ts @@ -103,9 +103,10 @@ describe("destroySandbox model-router teardown (#9098)", () => { endpointUrl: session.endpointUrl, }, { - listSandboxes: () => ({ sandboxes: [], defaultSandbox: null }), + listHostRegistryEntries: () => [], loadSession: () => session, updateSession, + withModelRouterPortLifecycleLock: async (_port, operation) => await operation(), }, ), ).resolves.toBeUndefined(); From 8888d2fdb3cf53d4f72b6a364abc612bc2e38961 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 12:38:50 -0700 Subject: [PATCH 02/16] fix(onboard): lock routed resume through route publication Signed-off-by: Prekshi Vyas --- src/lib/onboard.ts | 3 + .../onboard/machine/core-flow-phases.test.ts | 5 ++ ...ovider-inference-route-containment.test.ts | 67 ++++++++++++++ .../provider-inference.test-support.ts | 3 + .../machine/handlers/provider-inference.ts | 89 ++++++++++--------- 5 files changed, 126 insertions(+), 41 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 18309c004d4..839ba224d0f 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -3439,6 +3439,9 @@ async function runOnboard(opts: OnboardOptions = {}): Promise { preflightGatewayRouteDiscovery, getSandboxRecoveryAuthority: providerRecovery.getSandboxRecoveryAuthority, withGatewayRouteMutationLock: gatewayRouteMutationLock.withGatewayRouteMutationLock, + withModelRouterPortLifecycleLock: + gatewayRouteMutationLock.withModelRouterPortLifecycleLock, + getModelRouterPort: modelRouter.resolveModelRouterPort, normalizeHermesAuthMethod, setupNim: (g, s, a, recover, gateway, assertRouteCompatible, canProbeRoute, recoverySessionId) => setupNim(g, s, a, recover, opts.rebuildRegistryInferenceRoute, gateway, assertRouteCompatible, canProbeRoute, recoverySessionId), setupInference, resolveHostLocalInferenceStartupSelection: () => null, diff --git a/src/lib/onboard/machine/core-flow-phases.test.ts b/src/lib/onboard/machine/core-flow-phases.test.ts index bce24f35ab7..deaa29b0af0 100644 --- a/src/lib/onboard/machine/core-flow-phases.test.ts +++ b/src/lib/onboard/machine/core-flow-phases.test.ts @@ -127,6 +127,11 @@ function createPhases( _gatewayName: string, operation: () => Promise | T, ) => await operation(), + withModelRouterPortLifecycleLock: async ( + _port: number, + operation: () => Promise | T, + ) => await operation(), + getModelRouterPort: () => 4000, normalizeHermesAuthMethod: (value) => value === "oauth" || value === "api_key" ? value : null, setupNim: vi.fn(async () => ({ diff --git a/src/lib/onboard/machine/handlers/provider-inference-route-containment.test.ts b/src/lib/onboard/machine/handlers/provider-inference-route-containment.test.ts index 8f0a3567f09..89a1680bb69 100644 --- a/src/lib/onboard/machine/handlers/provider-inference-route-containment.test.ts +++ b/src/lib/onboard/machine/handlers/provider-inference-route-containment.test.ts @@ -88,6 +88,8 @@ function createDeps() { preflightGatewayRouteDiscovery: calls.preflightGatewayRouteDiscovery, getSandboxRecoveryAuthority: (): "missing" => "missing", withGatewayRouteMutationLock: async (_gatewayName, operation) => await operation(), + withModelRouterPortLifecycleLock: async (_port, operation) => await operation(), + getModelRouterPort: () => 4000, normalizeHermesAuthMethod: () => null, setupNim: calls.setupNim, setupInference: calls.setupInference, @@ -413,6 +415,71 @@ describe("provider route containment", () => { expect(calls.updateSandbox).not.toHaveBeenCalled(); }); + it("waits for a cross-gateway teardown port lock before publishing routed resume repair (#9098)", async () => { + const session = createSession({ provider: "nvidia-router", model: "router/model" }); + session.steps.provider_selection.status = "complete"; + const { calls, deps } = createDeps(); + let insideGatewayLock = false; + let insidePortLock = false; + let reportPortLockRequested!: () => void; + const portLockRequested = new Promise((resolve) => { + reportPortLockRequested = resolve; + }); + let releaseTeardownPortLock!: () => void; + const teardownPortLockReleased = new Promise((resolve) => { + releaseTeardownPortLock = resolve; + }); + deps.withGatewayRouteMutationLock = async (gatewayName, operation) => { + expect(gatewayName).toBe("nemoclaw-9090"); + insideGatewayLock = true; + try { + return await operation(); + } finally { + insideGatewayLock = false; + } + }; + deps.withModelRouterPortLifecycleLock = async (port, operation) => { + expect(insideGatewayLock).toBe(true); + expect(port).toBe(4000); + reportPortLockRequested(); + await teardownPortLockReleased; + insidePortLock = true; + try { + return await operation(); + } finally { + insidePortLock = false; + } + }; + calls.reconcileRouter.mockImplementation(async () => { + expect(insideGatewayLock).toBe(true); + expect(insidePortLock).toBe(true); + }); + calls.reupsertRoutedProvider.mockImplementation(() => { + expect(insideGatewayLock).toBe(true); + expect(insidePortLock).toBe(true); + return { ok: true, endpointUrl: "http://host.openshell.internal:4000/v1" }; + }); + calls.reserveRoute.mockImplementation(() => { + expect(insideGatewayLock).toBe(true); + expect(insidePortLock).toBe(true); + return true; + }); + + const repair = handleProviderInferenceState(resumeOptions(deps, session)); + await portLockRequested; + + expect(calls.reconcileRouter).not.toHaveBeenCalled(); + expect(calls.reupsertRoutedProvider).not.toHaveBeenCalled(); + expect(calls.reserveRoute).not.toHaveBeenCalled(); + + releaseTeardownPortLock(); + await expect(repair).resolves.toMatchObject({ provider: "nvidia-router" }); + + expect(calls.reconcileRouter).toHaveBeenCalledOnce(); + expect(calls.reupsertRoutedProvider).toHaveBeenCalledOnce(); + expect(calls.reserveRoute).toHaveBeenCalledOnce(); + }); + it("allows compatible-endpoint refresh to reach the final setup boundary (#6315)", async () => { const session = createSession({ provider: "compatible-endpoint", diff --git a/src/lib/onboard/machine/handlers/provider-inference.test-support.ts b/src/lib/onboard/machine/handlers/provider-inference.test-support.ts index 5aa7a4b1e05..f0fe5a7f1b6 100644 --- a/src/lib/onboard/machine/handlers/provider-inference.test-support.ts +++ b/src/lib/onboard/machine/handlers/provider-inference.test-support.ts @@ -160,6 +160,9 @@ export function createDeps( _gatewayName: string, operation: () => Promise | T, ) => await operation(), + withModelRouterPortLifecycleLock: async (_port: number, operation: () => Promise | T) => + await operation(), + getModelRouterPort: () => 4000, normalizeHermesAuthMethod: (value: string | null | undefined) => value === "oauth" || value === "api_key" ? value : null, setupNim: calls.setupNim, diff --git a/src/lib/onboard/machine/handlers/provider-inference.ts b/src/lib/onboard/machine/handlers/provider-inference.ts index 38629ae7c2c..441cc58c36c 100644 --- a/src/lib/onboard/machine/handlers/provider-inference.ts +++ b/src/lib/onboard/machine/handlers/provider-inference.ts @@ -165,6 +165,11 @@ export interface ProviderInferenceStateOptions { gatewayName: string, operation: () => Promise | T, ): Promise; + withModelRouterPortLifecycleLock( + port: number, + operation: () => Promise | T, + ): Promise; + getModelRouterPort(): number; normalizeHermesAuthMethod(value: string | null | undefined): HermesAuthMethod | null; setupNim( gpu: Gpu, @@ -1456,48 +1461,50 @@ export async function handleProviderInferenceState({ // #4564: re-upsert the gateway provider with the sandbox-facing // endpoint so a stale localhost base URL recorded by an earlier run is // repaired on resume instead of surviving and breaking inference.local. - const routedRepair = await deps.withGatewayRouteMutationLock(gatewayName, async () => { - assertProviderInferenceRouteCompatible(deps, gatewayName, sandboxName, { - provider: selectedProvider, - model: selectedModel, - endpointUrl, - credentialEnv, - preferredInferenceApi, - }); - try { - await deps.reconcileModelRouter(); - } catch (err) { - deps.error( - ` ✗ Failed to reconcile model router: ${err instanceof Error ? err.message : String(err)}`, + const routedRepair = await deps.withGatewayRouteMutationLock(gatewayName, () => + deps.withModelRouterPortLifecycleLock(deps.getModelRouterPort(), async () => { + assertProviderInferenceRouteCompatible(deps, gatewayName, sandboxName, { + provider: selectedProvider, + model: selectedModel, + endpointUrl, + credentialEnv, + preferredInferenceApi, + }); + try { + await deps.reconcileModelRouter(); + } catch (err) { + deps.error( + ` ✗ Failed to reconcile model router: ${err instanceof Error ? err.message : String(err)}`, + ); + deps.exitProcess(1); + } + const reupserted = deps.reupsertRoutedProvider( + gatewayName, + selectedProvider, + endpointUrl, + credentialEnv, ); - deps.exitProcess(1); - } - const reupserted = deps.reupsertRoutedProvider( - gatewayName, - selectedProvider, - endpointUrl, - credentialEnv, - ); - const reservationEndpointSource = endpointSourceForCurrentUrl( - endpointSource, - reupserted.endpointUrl, - onboardEndpointUrl, - ); - const reserved = - reupserted.ok && resumeReservationName - ? deps.reserveSandboxInferenceRoute(resumeReservationName, { - provider: selectedProvider, - model: selectedModel, - endpointUrl: reupserted.endpointUrl, - endpointSource: reservationEndpointSource, - credentialEnv, - preferredInferenceApi, - gatewayName, - reservationSessionId: session?.sessionId, - }) - : null; - return { reupserted, reservationEndpointSource, reserved }; - }); + const reservationEndpointSource = endpointSourceForCurrentUrl( + endpointSource, + reupserted.endpointUrl, + onboardEndpointUrl, + ); + const reserved = + reupserted.ok && resumeReservationName + ? deps.reserveSandboxInferenceRoute(resumeReservationName, { + provider: selectedProvider, + model: selectedModel, + endpointUrl: reupserted.endpointUrl, + endpointSource: reservationEndpointSource, + credentialEnv, + preferredInferenceApi, + gatewayName, + reservationSessionId: session?.sessionId, + }) + : null; + return { reupserted, reservationEndpointSource, reserved }; + }), + ); const { reupserted, reservationEndpointSource, reserved } = routedRepair; if (!reupserted.ok) { deps.error( From 25324cdb44011b5ef77c717ef7f41ff62f9c2103 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 12:54:23 -0700 Subject: [PATCH 03/16] test(destroy): preserve router teardown execution Signed-off-by: Prekshi Vyas --- test/helpers/destroy-flow-test-harness.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index 2b949f45526..682c718487d 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -219,9 +219,10 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr registeredSandboxCount = Math.max(0, registeredSandboxCount - 1); return true; }); - const stopModelRouterForDestroyedSandboxSpy = vi - .spyOn(destroyPreflight, "stopModelRouterForDestroyedSandbox") - .mockResolvedValue(undefined); + const stopModelRouterForDestroyedSandboxSpy = vi.spyOn( + destroyPreflight, + "stopModelRouterForDestroyedSandbox", + ); const retirePortableLifecycleReceiptSpy = vi .spyOn(destroyExecution, "retirePortableLifecycleAuthority") .mockImplementation(() => undefined); From 2b56e814e7c08bfa17f4ecb3c3552a3db64bd89b Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 13:04:05 -0700 Subject: [PATCH 04/16] docs(inference): state model router lock order Signed-off-by: Prekshi Vyas --- docs/inference/set-up-model-router.mdx | 5 +++-- docs/reference/commands.mdx | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index bca4271d842..6be91883164 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. -Routed onboarding holds a host-wide lock for the selected Model Router port through router setup and sandbox registry publication. -Model Router destruction holds the same port lock through its peer check, process stop, and session update. +Routed onboarding acquires the selected gateway route lock first, then the host-wide lock for the selected Model Router port. +It holds both locks through router setup and sandbox registry publication. +Model Router destruction uses the same lock order and holds both locks through its peer check, process stop, and session update. When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. If no same-port peer remains, NemoClaw stops only a process whose command line still identifies the Model Router on that port. It clears the matching session recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index ff4f969408a..9b4d63f035f 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2293,9 +2293,9 @@ 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. -Routed onboarding and Model Router destruction use the same host-wide lifecycle lock for each Model Router port. -Onboarding holds the lock through sandbox registry publication. -Destruction holds the lock through the peer check, process stop, and session update. +Routed onboarding and Model Router destruction acquire the selected gateway route lock first, then the host-wide lifecycle lock for each Model Router port. +Onboarding holds both locks through sandbox registry publication. +Destruction holds both locks through the peer check, process stop, and session update. When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. If no same-port peer remains, it stops only a process whose command line still identifies the Model Router on that port. It clears matching session recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. From 89b4ded314ae2538d97e3f5ab476172df9ad7c79 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 14:09:19 -0700 Subject: [PATCH 05/16] test(destroy): preserve router session replacement coverage Signed-off-by: Prekshi Vyas --- src/lib/actions/sandbox/destroy-flow.test.ts | 1 + .../sandbox/destroy-model-router.test.ts | 61 +++++++++---------- 2 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 3c6de4a00a5..c23bc0587ef 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -78,6 +78,7 @@ describe("destroySandbox flow", () => { expect.any(Function), ); }); + it("revokes the prior HTTPS-pin route only after confirmed deletion and registry removal", async () => { const routeId = "a".repeat(64); const harness = createDestroyHarness({ diff --git a/src/lib/actions/sandbox/destroy-model-router.test.ts b/src/lib/actions/sandbox/destroy-model-router.test.ts index 685efbbdff2..7b30d21d579 100644 --- a/src/lib/actions/sandbox/destroy-model-router.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -22,7 +22,7 @@ function createDeps(overrides: Partial = const session = { sessionId: "session-alpha", sandboxName: "alpha", - endpointUrl: "http://host.openshell.internal:4100/v1", + endpointUrl: routedSandbox.endpointUrl, routerPid: 4242, routerCredentialHash: "hash", } as Session; @@ -114,7 +114,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(deps.updateSession).not.toHaveBeenCalled(); }); - it("stops the target router when a routed peer uses a different port", async () => { + it("stops the router when a routed peer uses another port", async () => { const { deps } = createDeps({ listHostRegistryEntries: vi.fn(() => [ { @@ -188,6 +188,30 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(reusedNameSession.routerPid).toBe(6262); expect(reusedNameSession.routerCredentialHash).toBe("new-hash"); }); + + it("does not clear router identity after the session changes", async () => { + const replacementSession = { + sessionId: "session-beta", + sandboxName: "beta", + endpointUrl: "http://host.openshell.internal:4200/v1", + routerPid: 6262, + routerCredentialHash: "new-hash", + } as Session; + const { deps } = createDeps({ + updateSession: vi.fn((mutator: (current: Session) => Session | void) => { + mutator(replacementSession); + return replacementSession; + }), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(replacementSession).toMatchObject({ + routerPid: 6262, + routerCredentialHash: "new-hash", + }); + }); + it("clears a stale recorded PID when no router process is found", async () => { const { deps, session } = createDeps({ ownsPort: vi.fn(() => false), @@ -234,9 +258,8 @@ describe("stopModelRouterForDestroyedSandbox", () => { it("clears a stale credential hash when the session records no router PID (#9098)", async () => { const session = { - sessionId: "session-alpha", sandboxName: "alpha", - endpointUrl: "http://host.openshell.internal:4100/v1", + endpointUrl: routedSandbox.endpointUrl, routerPid: null, routerCredentialHash: "stale", } as Session; @@ -256,39 +279,13 @@ 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( () => ({ - sessionId: "session-beta", - sandboxName: "beta", - endpointUrl: "http://host.openshell.internal:4200/v1", + sandboxName: "alpha", + endpointUrl: routedSandbox.endpointUrl, routerPid: null, }) as Session, ), From a60217f42ed34d04fb781224d7d03d78c55c61fc Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 14:12:17 -0700 Subject: [PATCH 06/16] docs(inference): preserve replacement router identity Signed-off-by: Prekshi Vyas --- docs/inference/set-up-model-router.mdx | 5 +++-- docs/reference/commands.mdx | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index 6be91883164..0730d746914 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -37,8 +37,9 @@ It holds both locks through router setup and sandbox registry publication. Model Router destruction uses the same lock order and holds both locks through its peer check, process stop, and session update. When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. If no same-port peer remains, NemoClaw stops only a process whose command line still identifies the Model Router on that port. -It clears the matching session recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. -If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destroy still completes and keeps the session recovery identity. +NemoClaw clears the matching Model Router process and credential recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. +If the onboarding session changes during teardown, NemoClaw does not clear the replacement session's Model Router identity. +If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destroy still completes and keeps the recovery identity. The warning tells you to inspect the current listener process immediately before you stop anything. Stop it only if its command line still identifies the Model Router on the named port. Do not stop a previously reported process ID when its command line no longer matches. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 9b4d63f035f..e41802b3f27 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2298,7 +2298,8 @@ Onboarding holds both locks through sandbox registry publication. Destruction holds both locks through the peer check, process stop, and session update. When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. If no same-port peer remains, it stops only a process whose command line still identifies the Model Router on that port. -It clears matching session recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. +NemoClaw clears the matching Model Router process and credential recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. +If the onboarding session changes during teardown, NemoClaw does not clear the replacement session's Model Router identity. If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destruction still completes and keeps the recovery identity. Inspect the current listener process immediately before you stop anything. Stop it only if its command line still identifies the Model Router on the named port. From de19e53f57c1c4775c6c40190308124686021467 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 14:36:25 -0700 Subject: [PATCH 07/16] fix(sandbox): preserve replacement router sessions Signed-off-by: Prekshi Vyas --- docs/inference/set-up-model-router.mdx | 4 +- docs/reference/commands.mdx | 4 +- .../sandbox/destroy-model-router.test.ts | 50 +++++++++----- src/lib/actions/sandbox/destroy-preflight.ts | 35 +++++----- src/lib/actions/sandbox/destroy.ts | 2 +- ...onboard-session-cross-process-lock.test.ts | 67 +++++++++++++++++++ src/lib/state/onboard-session.ts | 27 ++++++++ test/helpers/destroy-flow-test-harness.ts | 15 +++++ .../destroy-model-router-flow.test.ts | 18 +++-- 9 files changed, 181 insertions(+), 41 deletions(-) diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index 0730d746914..51dec96208c 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -38,7 +38,9 @@ Model Router destruction uses the same lock order and holds both locks through i When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. If no same-port peer remains, NemoClaw stops only a process whose command line still identifies the Model Router on that port. NemoClaw clears the matching Model Router process and credential recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. -If the onboarding session changes during teardown, NemoClaw does not clear the replacement session's Model Router identity. +Before clearing this identity, destroy acquires the non-blocking onboarding session lock and rechecks the exact session identity. +If another onboarding run owns the lock, destroy keeps the recovery identity and warns instead of waiting. +If the session identity no longer matches, destroy leaves the replacement session unchanged. If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destroy still completes and keeps the recovery identity. The warning tells you to inspect the current listener process immediately before you stop anything. Stop it only if its command line still identifies the Model Router on the named port. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index e41802b3f27..d315ded42ea 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2299,7 +2299,9 @@ Destruction holds both locks through the peer check, process stop, and session u When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. If no same-port peer remains, it stops only a process whose command line still identifies the Model Router on that port. NemoClaw clears the matching Model Router process and credential recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. -If the onboarding session changes during teardown, NemoClaw does not clear the replacement session's Model Router identity. +Before clearing this identity, destroy acquires the non-blocking onboarding session lock and rechecks the exact session identity. +If another onboarding run owns the lock, destroy keeps the recovery identity and warns instead of waiting. +If the session identity no longer matches, destroy leaves the replacement session unchanged. If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destruction still completes and keeps the recovery identity. Inspect the current listener process immediately before you stop anything. Stop it only if its command line still identifies the Model Router on the named port. diff --git a/src/lib/actions/sandbox/destroy-model-router.test.ts b/src/lib/actions/sandbox/destroy-model-router.test.ts index 7b30d21d579..3f029e1f892 100644 --- a/src/lib/actions/sandbox/destroy-model-router.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -27,6 +27,11 @@ function createDeps(overrides: Partial = routerCredentialHash: "hash", } as Session; const deps: StopModelRouterForDestroyedSandboxDeps = { + compareAndSwapSession: vi.fn((matches, mutator) => { + if (!matches(session)) return "mismatch"; + mutator(session); + return "updated"; + }), inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), isHealthy: vi.fn(async () => false), isRoutedProvider: vi.fn((provider: string | null | undefined) => provider === "nvidia-router"), @@ -35,10 +40,6 @@ function createDeps(overrides: Partial = 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(), withModelRouterPortLifecycleLock: async (_port: number, operation: () => Promise | T) => await operation(), @@ -80,7 +81,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { ); expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); }); it("does nothing when the registry entry is missing", async () => { @@ -89,7 +90,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(null, deps); expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); }); it("keeps the router while a sandbox in another gateway state root uses the same port", async () => { @@ -111,7 +112,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); }); it("stops the router when a routed peer uses another port", async () => { @@ -164,7 +165,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); expect(deps.stopProcess).toHaveBeenCalledWith(5151, 4100); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); expect(unrelatedSession.routerPid).toBe(6262); expect(unrelatedSession.routerCredentialHash).toBe("beta-hash"); }); @@ -184,7 +185,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); expect(reusedNameSession.routerPid).toBe(6262); expect(reusedNameSession.routerCredentialHash).toBe("new-hash"); }); @@ -198,9 +199,10 @@ describe("stopModelRouterForDestroyedSandbox", () => { routerCredentialHash: "new-hash", } as Session; const { deps } = createDeps({ - updateSession: vi.fn((mutator: (current: Session) => Session | void) => { + compareAndSwapSession: vi.fn((matches, mutator) => { + if (!matches(replacementSession)) return "mismatch"; mutator(replacementSession); - return replacementSession; + return "updated"; }), }); @@ -212,6 +214,18 @@ describe("stopModelRouterForDestroyedSandbox", () => { }); }); + it("keeps session identity while another onboarding run owns the session lock", async () => { + const { deps, session } = createDeps({ + compareAndSwapSession: vi.fn(() => "busy" as const), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(session.routerPid).toBe(4242); + expect(session.routerCredentialHash).toBe("hash"); + expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("owns the session lock")); + }); + it("clears a stale recorded PID when no router process is found", async () => { const { deps, session } = createDeps({ ownsPort: vi.fn(() => false), @@ -235,7 +249,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); expect(session.routerPid).toBe(4242); expect(session.routerCredentialHash).toBe("hash"); expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("process inventory")); @@ -250,7 +264,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); expect(session.routerPid).toBe(4242); expect(session.routerCredentialHash).toBe("hash"); expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("healthy port 4100")); @@ -258,6 +272,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { it("clears a stale credential hash when the session records no router PID (#9098)", async () => { const session = { + sessionId: "session-alpha", sandboxName: "alpha", endpointUrl: routedSandbox.endpointUrl, routerPid: null, @@ -266,9 +281,10 @@ describe("stopModelRouterForDestroyedSandbox", () => { const { deps } = createDeps({ loadSession: vi.fn(() => session), ownsPort: vi.fn(() => false), - updateSession: vi.fn((mutator: (current: Session) => Session | void) => { + compareAndSwapSession: vi.fn((matches, mutator) => { + if (!matches(session)) return "mismatch"; mutator(session); - return session; + return "updated"; }), }); @@ -295,7 +311,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); }); it("warns and keeps the recorded PID when the stop fails, so uninstall can still find it", async () => { @@ -310,7 +326,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("shutdown did not converge")); expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("Inspect PID 4242")); expect(deps.warn).not.toHaveBeenCalledWith(expect.stringContaining("kill 4242")); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).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 0cb7e43159a..73e9c6095e1 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -13,7 +13,7 @@ import { stopModelRouterProcess, } from "../../onboard/model-router-process"; import { listHostGatewayRegistryEntries } from "../../state/gateway-registry"; -import type { Session } from "../../state/onboard-session"; +import type { compareAndSwapSession, Session } from "../../state/onboard-session"; import type { SandboxEntry } from "../../state/registry"; import * as registry from "../../state/registry"; import { type DestroyRunOpenshell, selectGatewayForSandboxDestroy } from "./destroy-gateway"; @@ -59,8 +59,8 @@ export function stopSandboxInferenceResources( const DEFAULT_MODEL_ROUTER_PORT = 4000; export type StopModelRouterForDestroyedSandboxDeps = { + compareAndSwapSession: typeof compareAndSwapSession; loadSession: () => Session | null; - updateSession: (mutator: (session: Session) => Session | void) => Session; inspectProcessForPort?: typeof inspectModelRouterProcessForPort; isHealthy?: typeof isRouterHealthy; isRoutedProvider?: typeof isRoutedInferenceProvider; @@ -186,20 +186,25 @@ export async function stopModelRouterForDestroyedSandbox( // is gone. A completed process scan plus an unhealthy port confirms that // no router remains when no PID was found. if (sessionMatchesSandbox && (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 - ) { + const result = deps.compareAndSwapSession( + (current) => + current.sessionId === session.sessionId && + current.sandboxName === session.sandboxName && + current.endpointUrl === session.endpointUrl && + current.routerPid === recordedPid && + current.routerCredentialHash === recordedCredentialHash, + (current) => { + current.routerPid = null; + current.routerCredentialHash = null; return current; - } - current.routerPid = null; - current.routerCredentialHash = null; - return current; - }); + }, + "nemoclaw destroy Model Router session cleanup", + ); + if (result === "busy") { + warn( + "Another onboarding run owns the session lock. Keeping its Model Router recovery identity.", + ); + } } }); } diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 921dff7f491..79273e3043a 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -696,8 +696,8 @@ async function destroySandboxUnlocked( // when the competing sandbox belongs to another gateway. await withGatewayRouteMutationLock(cleanupGatewayName, () => stopModelRouterForDestroyedSandbox(sandbox, { + compareAndSwapSession: onboardSession.compareAndSwapSession, loadSession: onboardSession.loadSession, - updateSession: onboardSession.updateSession, warn: defaultDestroyWarn, }), ); diff --git a/src/lib/state/onboard-session-cross-process-lock.test.ts b/src/lib/state/onboard-session-cross-process-lock.test.ts index 52c36e169d4..7d91c94ad9d 100644 --- a/src/lib/state/onboard-session-cross-process-lock.test.ts +++ b/src/lib/state/onboard-session-cross-process-lock.test.ts @@ -70,4 +70,71 @@ describe("cross-process onboard lock", () => { await exited; } }); + + it("does not replace a session written by the process that owns the onboard lock", async () => { + session.saveSession( + session.createSession({ + sessionId: "destroyed-sandbox-session", + sandboxName: "alpha", + endpointUrl: "http://host.openshell.internal:4000/v1", + routerPid: 4242, + routerCredentialHash: "old-hash", + }), + ); + const childScript = ` + const fs = require("node:fs"); + const path = require("node:path"); + const lockFile = process.argv[1]; + const sessionFile = process.argv[2]; + fs.mkdirSync(path.dirname(lockFile), { recursive: true }); + const fd = fs.openSync(lockFile, "wx", 0o600); + fs.writeSync(fd, JSON.stringify({ + pid: process.pid, + startedAt: new Date().toISOString(), + command: "replacement nemoclaw onboard process", + })); + const replacement = JSON.parse(fs.readFileSync(sessionFile, "utf8")); + replacement.sessionId = "replacement-session"; + replacement.sandboxName = "beta"; + replacement.endpointUrl = "http://host.openshell.internal:4200/v1"; + replacement.routerPid = 6262; + replacement.routerCredentialHash = "replacement-hash"; + const tempFile = sessionFile + ".replacement"; + fs.writeFileSync(tempFile, JSON.stringify(replacement), { mode: 0o600 }); + fs.renameSync(tempFile, sessionFile); + process.stdout.write("replacement-written\\n"); + setInterval(() => {}, 1000); + `; + const child = spawn( + process.execPath, + ["-e", childScript, session.LOCK_FILE, session.SESSION_FILE], + { stdio: ["ignore", "pipe", "inherit"] }, + ); + await once(child.stdout, "data"); + + try { + const result = session.compareAndSwapSession( + (current) => current.sessionId === "destroyed-sandbox-session", + (current) => { + current.routerPid = null; + current.routerCredentialHash = null; + return current; + }, + "nemoclaw destroy Model Router session cleanup", + ); + + expect(result).toBe("busy"); + expect(session.loadSession()).toMatchObject({ + sessionId: "replacement-session", + sandboxName: "beta", + endpointUrl: "http://host.openshell.internal:4200/v1", + routerPid: 6262, + routerCredentialHash: "replacement-hash", + }); + } finally { + const exited = once(child, "exit"); + child.kill(); + await exited; + } + }); }); diff --git a/src/lib/state/onboard-session.ts b/src/lib/state/onboard-session.ts index 9a87e9f3442..2cfe75f4773 100644 --- a/src/lib/state/onboard-session.ts +++ b/src/lib/state/onboard-session.ts @@ -1489,6 +1489,33 @@ export function updateSession(mutator: (session: Session) => Session | void): Se return saveSession(next); } +export type CompareAndSwapSessionResult = "updated" | "busy" | "mismatch"; + +/** + * Mutate the current session only while no onboarding writer owns its lock. + * + * Production onboarding holds `LOCK_FILE` across its session writes. Reusing + * that boundary closes the load-before-rename race for short mutations from a + * different command without waiting while an onboarding run is active. + */ +export function compareAndSwapSession( + matches: (session: Session) => boolean, + mutator: (session: Session) => Session | void, + command = "nemoclaw session compare-and-swap", +): CompareAndSwapSessionResult { + const lock = acquireOnboardLock(command); + if (!lock.acquired) return "busy"; + try { + const current = loadSession(); + if (!current || !matches(current)) return "mismatch"; + const next = mutator(current) || current; + saveSession(next); + return "updated"; + } finally { + releaseOnboardLock(); + } +} + export function markStepStarted(stepName: string): Session { const updatedSession = updateSession((session) => { const step = session.steps[stepName]; diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index 682c718487d..4c874f5ec79 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -16,6 +16,7 @@ const destroyModulePath = "./destroy.js"; export type DestroyHarness = { cleanupGatewaySpy: MockInstance; captureOpenshellSpy: MockInstance; + compareAndSwapSessionSpy: MockInstance; destroySandbox: DestroySandbox; dockerCaptureSpy: MockInstance; dockerRunSpy: MockInstance; @@ -251,6 +252,19 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr sandboxName: "alpha", ...(options.sessionRouterPid ? { routerPid: options.sessionRouterPid } : {}), }); + const compareAndSwapSessionSpy = vi + .spyOn(onboardSession, "compareAndSwapSession") + .mockImplementation((matches: unknown, mutator: unknown) => { + const session = { + sandboxName: "alpha", + ...(options.sessionRouterPid ? { routerPid: options.sessionRouterPid } : {}), + }; + expect(typeof matches).toBe("function"); + expect(typeof mutator).toBe("function"); + if (!(matches as (value: typeof session) => boolean)(session)) return "mismatch"; + (mutator as (value: typeof session) => void)(session); + return "updated"; + }); const updateSessionSpy = vi .spyOn(onboardSession, "updateSession") .mockImplementation((mutator: unknown) => { @@ -425,6 +439,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr return { cleanupGatewaySpy, captureOpenshellSpy, + compareAndSwapSessionSpy, dockerCaptureSpy, dockerRunSpy, destroySandbox: requireDist(destroyModulePath).destroySandbox, diff --git a/test/package-contract/destroy-model-router-flow.test.ts b/test/package-contract/destroy-model-router-flow.test.ts index 66aea4d35e4..643afcc50d3 100644 --- a/test/package-contract/destroy-model-router-flow.test.ts +++ b/test/package-contract/destroy-model-router-flow.test.ts @@ -90,10 +90,16 @@ describe("destroySandbox model-router teardown (#9098)", () => { routerPid: stub.pid, routerCredentialHash: "router-credential-hash", } as Session; - const updateSession = vi.fn((mutator: (current: Session) => Session | void) => { - mutator(session); - return session; - }); + const compareAndSwapSession = vi.fn( + ( + matches: (current: Session) => boolean, + mutator: (current: Session) => Session | void, + ) => { + if (!matches(session)) return "mismatch" as const; + mutator(session); + return "updated" as const; + }, + ); await expect( stopModelRouterForDestroyedSandbox( @@ -104,8 +110,8 @@ describe("destroySandbox model-router teardown (#9098)", () => { }, { listHostRegistryEntries: () => [], + compareAndSwapSession, loadSession: () => session, - updateSession, withModelRouterPortLifecycleLock: async (_port, operation) => await operation(), }, ), @@ -113,7 +119,7 @@ describe("destroySandbox model-router teardown (#9098)", () => { await vi.waitFor(() => expect(stubExited).toBe(true), { timeout: 8_000, interval: 100 }); expect(await probeHealthy(port)).toBe(false); - expect(updateSession).toHaveBeenCalledOnce(); + expect(compareAndSwapSession).toHaveBeenCalledOnce(); expect(session).toEqual( expect.objectContaining({ routerPid: null, routerCredentialHash: null }), ); From aa71cf6ca76d7dfbac2e35a9d3907bb429850d67 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 15:08:13 -0700 Subject: [PATCH 08/16] fix(sandbox): serialize full destroy session cleanup Signed-off-by: Prekshi Vyas --- docs/inference/set-up-model-router.mdx | 12 +- docs/reference/commands.mdx | 14 +- src/lib/actions/sandbox/destroy-flow.test.ts | 29 ++- .../sandbox/destroy-model-router.test.ts | 78 ++++-- src/lib/actions/sandbox/destroy-preflight.ts | 227 +++++++++++------- src/lib/actions/sandbox/destroy.ts | 34 ++- ...onboard-session-cross-process-lock.test.ts | 30 ++- src/lib/state/onboard-session.ts | 21 +- test/helpers/destroy-flow-test-harness.ts | 49 +++- .../destroy-model-router-flow.test.ts | 22 +- 10 files changed, 356 insertions(+), 160 deletions(-) diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index 51dec96208c..41f281c67b4 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -34,13 +34,17 @@ Review the log before you share it. NemoClaw does not provide log-only cleanup. Routed onboarding acquires the selected gateway route lock first, then the host-wide lock for the selected Model Router port. It holds both locks through router setup and sandbox registry publication. -Model Router destruction uses the same lock order and holds both locks through its peer check, process stop, and session update. +Before sandbox deletion, `destroy` captures the current onboarding session identity. +Model Router destruction takes the gateway route lock, then the host-wide Model Router port lock, and then tries the onboarding session lock without waiting. +After acquiring the session lock, `destroy` rechecks the captured identity before the peer check or process stop. +If another onboarding run owns the session lock, or the identity changed, `destroy` skips Model Router teardown and warns. +It leaves the Model Router process and replacement session unchanged. When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. If no same-port peer remains, NemoClaw stops only a process whose command line still identifies the Model Router on that port. NemoClaw clears the matching Model Router process and credential recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. -Before clearing this identity, destroy acquires the non-blocking onboarding session lock and rechecks the exact session identity. -If another onboarding run owns the lock, destroy keeps the recovery identity and warns instead of waiting. -If the session identity no longer matches, destroy leaves the replacement session unchanged. +The final sandbox-name cleanup also checks the captured session identity. +Non-Model Router cleanup uses a non-blocking session update. +If onboarding owns the lock or a same-name replacement has a different identity, `destroy` leaves the replacement session unchanged. If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destroy still completes and keeps the recovery identity. The warning tells you to inspect the current listener process immediately before you stop anything. Stop it only if its command line still identifies the Model Router on the named port. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index d315ded42ea..a0af5ce70ae 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2293,15 +2293,19 @@ 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. -Routed onboarding and Model Router destruction acquire the selected gateway route lock first, then the host-wide lifecycle lock for each Model Router port. +Routed onboarding acquires the selected gateway route lock first, then the host-wide lifecycle lock for each Model Router port. Onboarding holds both locks through sandbox registry publication. -Destruction holds both locks through the peer check, process stop, and session update. +Before sandbox deletion, `destroy` captures the current onboarding session identity. +Model Router destruction takes the gateway route lock, then the host-wide Model Router port lock, and then tries the onboarding session lock without waiting. +After acquiring the session lock, `destroy` rechecks the captured identity before the peer check or process stop. +If another onboarding run owns the session lock, or the identity changed, `destroy` skips Model Router teardown and warns. +It leaves the Model Router process and replacement session unchanged. When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. If no same-port peer remains, it stops only a process whose command line still identifies the Model Router on that port. NemoClaw clears the matching Model Router process and credential recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. -Before clearing this identity, destroy acquires the non-blocking onboarding session lock and rechecks the exact session identity. -If another onboarding run owns the lock, destroy keeps the recovery identity and warns instead of waiting. -If the session identity no longer matches, destroy leaves the replacement session unchanged. +The final sandbox-name cleanup also checks the captured session identity. +Non-Model Router cleanup uses a non-blocking session update. +If onboarding owns the lock or a same-name replacement has a different identity, `destroy` leaves the replacement session unchanged. If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destruction still completes and keeps the recovery identity. Inspect the current listener process immediately before you stop anything. Stop it only if its command line still identifies the Model Router on the named port. diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index c23bc0587ef..f730d285e84 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -79,6 +79,29 @@ describe("destroySandbox flow", () => { ); }); + it("leaves an active same-name replacement onboarding session unchanged", async () => { + const harness = createDestroyHarness({ + provider: "nvidia-router", + endpointUrl: "http://host.openshell.internal:4000/v1", + replaceSessionAfterRegistryRemoval: true, + sessionRouterPid: 4242, + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); + + expect(harness.stopModelRouterForDestroyedSandboxSpy).toHaveBeenCalledOnce(); + expect(harness.compareAndSwapSessionSpy).not.toHaveBeenCalled(); + expect(harness.updateSessionSpy).not.toHaveBeenCalled(); + expect(harness.sessionState).toMatchObject({ + sessionId: "replacement-session", + sandboxName: "alpha", + endpointUrl: "http://host.openshell.internal:4000/v1", + routerPid: 6262, + routerCredentialHash: "replacement-hash", + }); + expect(harness.warnSpy).toHaveBeenCalledWith(expect.stringContaining("owns the session lock")); + }); + it("revokes the prior HTTPS-pin route only after confirmed deletion and registry removal", async () => { const routeId = "a".repeat(64); const harness = createDestroyHarness({ @@ -491,7 +514,8 @@ describe("destroySandbox flow", () => { timeout: 30_000, }); expect(harness.removeSandboxSpy).toHaveBeenCalledWith("alpha"); - expect(harness.updateSessionSpy).toHaveBeenCalledOnce(); + expect(harness.compareAndSwapSessionSpy).toHaveBeenCalledOnce(); + expect(harness.updateSessionSpy).not.toHaveBeenCalled(); expect(harness.logSpy.mock.calls.map((call) => String(call[0])).join("\n")).toContain( "Sandbox 'alpha' destroyed", ); @@ -740,7 +764,8 @@ describe("destroySandbox flow", () => { }); expect(harness.finalizeMcpBridgesAfterSandboxDeleteSpy).toHaveBeenCalledTimes(2); expect(harness.removeSandboxSpy).toHaveBeenCalledWith("alpha"); - expect(harness.updateSessionSpy).toHaveBeenCalledOnce(); + expect(harness.compareAndSwapSessionSpy).toHaveBeenCalledOnce(); + expect(harness.updateSessionSpy).not.toHaveBeenCalled(); expect(harness.cleanupGatewaySpy).toHaveBeenCalledWith( "nemoclaw-19080", harness.runOpenshellSpy, diff --git a/src/lib/actions/sandbox/destroy-model-router.test.ts b/src/lib/actions/sandbox/destroy-model-router.test.ts index 3f029e1f892..6e52d0380bd 100644 --- a/src/lib/actions/sandbox/destroy-model-router.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -27,11 +27,17 @@ function createDeps(overrides: Partial = routerCredentialHash: "hash", } as Session; const deps: StopModelRouterForDestroyedSandboxDeps = { + acquireOnboardLock: vi.fn(() => ({ + acquired: true, + lockFile: "/tmp/onboard.lock", + stale: false, + })), compareAndSwapSession: vi.fn((matches, mutator) => { if (!matches(session)) return "mismatch"; mutator(session); return "updated"; }), + expectedSession: session, inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), isHealthy: vi.fn(async () => false), isRoutedProvider: vi.fn((provider: string | null | undefined) => provider === "nvidia-router"), @@ -39,6 +45,7 @@ function createDeps(overrides: Partial = loadSession: vi.fn(() => session), log: vi.fn(), ownsPort: vi.fn(() => true), + releaseOnboardLock: vi.fn(), stopProcess: vi.fn(async () => undefined), warn: vi.fn(), withModelRouterPortLifecycleLock: async (_port: number, operation: () => Promise | T) => @@ -69,6 +76,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(deps.stopProcess).toHaveBeenCalledWith(4242, 4100); expect(session.routerPid).toBeNull(); expect(session.routerCredentialHash).toBeNull(); + expect(session.sandboxName).toBeNull(); expect(deps.warn).not.toHaveBeenCalled(); }); @@ -94,7 +102,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { }); it("keeps the router while a sandbox in another gateway state root uses the same port", async () => { - const { deps } = createDeps({ + const { deps, session } = createDeps({ listHostRegistryEntries: vi.fn(() => [ { entry: { @@ -112,7 +120,8 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).toHaveBeenCalledOnce(); + expect(session.sandboxName).toBeNull(); }); it("stops the router when a routed peer uses another port", async () => { @@ -157,6 +166,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { routerCredentialHash: "beta-hash", } as Session; const { deps } = createDeps({ + expectedSession: unrelatedSession, loadSession: vi.fn(() => unrelatedSession), ownsPort: vi.fn(() => false), inspectProcessForPort: vi.fn(() => ({ status: "found" as const, pid: 5151 })), @@ -186,6 +196,8 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(reusedNameSession.sandboxName).toBe("alpha"); expect(reusedNameSession.routerPid).toBe(6262); expect(reusedNameSession.routerCredentialHash).toBe("new-hash"); }); @@ -198,31 +210,38 @@ describe("stopModelRouterForDestroyedSandbox", () => { routerPid: 6262, routerCredentialHash: "new-hash", } as Session; - const { deps } = createDeps({ - compareAndSwapSession: vi.fn((matches, mutator) => { - if (!matches(replacementSession)) return "mismatch"; - mutator(replacementSession); - return "updated"; - }), - }); + const { deps } = createDeps({ loadSession: vi.fn(() => replacementSession) }); await stopModelRouterForDestroyedSandbox(routedSandbox, deps); expect(replacementSession).toMatchObject({ + sandboxName: "beta", routerPid: 6262, routerCredentialHash: "new-hash", }); + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); }); it("keeps session identity while another onboarding run owns the session lock", async () => { const { deps, session } = createDeps({ - compareAndSwapSession: vi.fn(() => "busy" as const), + acquireOnboardLock: vi.fn(() => ({ + acquired: false, + lockFile: "/tmp/onboard.lock", + stale: false, + holderPid: 6262, + holderStartedAt: "2026-08-14T00:00:00.000Z", + holderCommand: "replacement nemoclaw onboard process", + })), }); await stopModelRouterForDestroyedSandbox(routedSandbox, deps); expect(session.routerPid).toBe(4242); expect(session.routerCredentialHash).toBe("hash"); + expect(session.sandboxName).toBe("alpha"); + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("owns the session lock")); }); @@ -249,9 +268,10 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).toHaveBeenCalledOnce(); expect(session.routerPid).toBe(4242); expect(session.routerCredentialHash).toBe("hash"); + expect(session.sandboxName).toBeNull(); expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("process inventory")); }); @@ -264,9 +284,10 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); - expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).toHaveBeenCalledOnce(); expect(session.routerPid).toBe(4242); expect(session.routerCredentialHash).toBe("hash"); + expect(session.sandboxName).toBeNull(); expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("healthy port 4100")); }); @@ -279,6 +300,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { routerCredentialHash: "stale", } as Session; const { deps } = createDeps({ + expectedSession: session, loadSession: vi.fn(() => session), ownsPort: vi.fn(() => false), compareAndSwapSession: vi.fn((matches, mutator) => { @@ -295,23 +317,30 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(session.routerCredentialHash).toBeNull(); }); - it("leaves the session untouched when it records no router PID and no orphan exists", async () => { + it("clears the destroyed sandbox association when no router identity remains", async () => { + const session = { + sessionId: "session-alpha", + sandboxName: "alpha", + endpointUrl: routedSandbox.endpointUrl, + routerPid: null, + routerCredentialHash: null, + } as Session; const { deps } = createDeps({ - loadSession: vi.fn( - () => - ({ - sandboxName: "alpha", - endpointUrl: routedSandbox.endpointUrl, - routerPid: null, - }) as Session, - ), + expectedSession: session, + loadSession: vi.fn(() => session), ownsPort: vi.fn(() => false), + compareAndSwapSession: vi.fn((matches, mutator) => { + if (!matches(session)) return "mismatch"; + mutator(session); + return "updated"; + }), }); await stopModelRouterForDestroyedSandbox(routedSandbox, deps); expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).toHaveBeenCalledOnce(); + expect(session.sandboxName).toBeNull(); }); it("warns and keeps the recorded PID when the stop fails, so uninstall can still find it", async () => { @@ -321,13 +350,14 @@ describe("stopModelRouterForDestroyedSandbox", () => { }), }); - await expect(stopModelRouterForDestroyedSandbox(routedSandbox, deps)).resolves.toBeUndefined(); + await expect(stopModelRouterForDestroyedSandbox(routedSandbox, deps)).resolves.toBe(true); expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("shutdown did not converge")); expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("Inspect PID 4242")); expect(deps.warn).not.toHaveBeenCalledWith(expect.stringContaining("kill 4242")); - expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).toHaveBeenCalledOnce(); expect(session.routerPid).toBe(4242); + expect(session.sandboxName).toBeNull(); }); it("does not recommend a PID stop when ownership changes after a failed shutdown", async () => { diff --git a/src/lib/actions/sandbox/destroy-preflight.ts b/src/lib/actions/sandbox/destroy-preflight.ts index 73e9c6095e1..52edc3cf427 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -13,7 +13,12 @@ import { stopModelRouterProcess, } from "../../onboard/model-router-process"; import { listHostGatewayRegistryEntries } from "../../state/gateway-registry"; -import type { compareAndSwapSession, Session } from "../../state/onboard-session"; +import type { + acquireOnboardLock, + compareAndSwapSession, + releaseOnboardLock, + Session, +} from "../../state/onboard-session"; import type { SandboxEntry } from "../../state/registry"; import * as registry from "../../state/registry"; import { type DestroyRunOpenshell, selectGatewayForSandboxDestroy } from "./destroy-gateway"; @@ -59,8 +64,11 @@ export function stopSandboxInferenceResources( const DEFAULT_MODEL_ROUTER_PORT = 4000; export type StopModelRouterForDestroyedSandboxDeps = { + acquireOnboardLock: typeof acquireOnboardLock; compareAndSwapSession: typeof compareAndSwapSession; + expectedSession: Session | null; loadSession: () => Session | null; + releaseOnboardLock: typeof releaseOnboardLock; inspectProcessForPort?: typeof inspectModelRouterProcessForPort; isHealthy?: typeof isRouterHealthy; isRoutedProvider?: typeof isRoutedInferenceProvider; @@ -73,6 +81,18 @@ export type StopModelRouterForDestroyedSandboxDeps = { withModelRouterPortLifecycleLock?: typeof withModelRouterPortLifecycleLock; }; +function sessionMatchesDestroySnapshot(current: Session | null, expected: Session | null): boolean { + if (current === null || expected === null) return current === expected; + return ( + current.sessionId === expected.sessionId && + current.updatedAt === expected.updatedAt && + current.sandboxName === expected.sandboxName && + current.endpointUrl === expected.endpointUrl && + current.routerPid === expected.routerPid && + current.routerCredentialHash === expected.routerCredentialHash + ); +} + export function resolveDestroyedSandboxRouterPort(endpointUrl: string | null | undefined): number { try { const port = Number(new URL(endpointUrl ?? "").port); @@ -96,117 +116,152 @@ export function resolveDestroyedSandboxRouterPort(endpointUrl: string | null | u * The recorded PID is preferred only when the session sandbox and router port * match the destroyed sandbox. Otherwise, the /proc scan recovers the orphan * by verified command line, exactly like reconcileModelRouter's recovery path. - * The host-global port lock covers the peer scan, stop, and session update so - * routed onboarding on another gateway cannot publish a peer between them. A - * stop failure or inconclusive process inventory is a warning, not an error: - * the sandbox delete already succeeded. The matching session keeps routerPid - * and credential identity so uninstall and reconcile retain recovery evidence. + * The host-global port lock and non-blocking onboarding session lock cover the + * peer scan, stop, and session update. Destroy skips teardown when onboarding + * owns the session lock or the pre-delete session snapshot changed. A stop + * failure or inconclusive process inventory is a warning, not an error: the + * sandbox delete already succeeded. The matching session keeps routerPid and + * credential identity so uninstall and reconcile retain recovery evidence. */ export async function stopModelRouterForDestroyedSandbox( sandbox: SandboxEntry | null, deps: StopModelRouterForDestroyedSandboxDeps, -): Promise { +): Promise { const isRoutedProvider = deps.isRoutedProvider ?? isRoutedInferenceProvider; - if (!sandbox || !isRoutedProvider(sandbox.provider)) return; + if (!sandbox || !isRoutedProvider(sandbox.provider)) return false; const port = resolveDestroyedSandboxRouterPort(sandbox.endpointUrl); const withPortLock = deps.withModelRouterPortLifecycleLock ?? withModelRouterPortLifecycleLock; await withPortLock(port, async () => { - const listHostRegistryEntries = deps.listHostRegistryEntries ?? listHostGatewayRegistryEntries; - const home = (deps.resolveHomeDir ?? (() => process.env.HOME || os.homedir()))(); - // Called after selected-registry removal, so every remaining host entry is - // a peer, including entries owned by a different gateway state root. - const routedPeerRemains = listHostRegistryEntries(home).some(({ entry }) => { - const provider = typeof entry.provider === "string" ? entry.provider : null; - const endpointUrl = typeof entry.endpointUrl === "string" ? entry.endpointUrl : null; - return isRoutedProvider(provider) && resolveDestroyedSandboxRouterPort(endpointUrl) === port; - }); - if (routedPeerRemains) return; - - const ownsPort = deps.ownsPort ?? doesModelRouterProcessOwnPort; - const inspectProcessForPort = deps.inspectProcessForPort ?? inspectModelRouterProcessForPort; - const isHealthy = deps.isHealthy ?? isRouterHealthy; const warn = deps.warn ?? console.warn; - const session = deps.loadSession(); - const sessionMatchesSandbox = - session?.sandboxName === sandbox.name && - resolveDestroyedSandboxRouterPort(session.endpointUrl) === port; - const recordedPid = sessionMatchesSandbox ? (session.routerPid ?? null) : null; - const recordedCredentialHash = sessionMatchesSandbox - ? (session.routerCredentialHash ?? null) - : null; - let pid: number | null = null; - if (ownsPort(recordedPid, port)) { - pid = recordedPid as number; - } else { - const lookup = inspectProcessForPort(port); - if (lookup.status === "unavailable") { - warn( - `Could not inspect the host process inventory for the Model Router on port ${port}. ` + - "Keeping its session recovery identity; inspect the port listener before the next Model Router onboarding.", - ); - return; - } - if (lookup.status === "found") { - pid = lookup.pid; - } else if (await isHealthy(port, 1000)) { + const sessionLock = deps.acquireOnboardLock("nemoclaw destroy Model Router teardown"); + if (!sessionLock.acquired) { + warn( + "Another onboarding run owns the session lock. Keeping the Model Router process and recovery identity.", + ); + return; + } + + let destroyedSessionId: string | null = null; + try { + const session = deps.loadSession(); + if (!sessionMatchesDestroySnapshot(session, deps.expectedSession)) { warn( - `No Model Router process could be confirmed for healthy port ${port}. ` + - "Keeping its session recovery identity; inspect the port listener before the next Model Router onboarding.", + "The onboarding session changed during destroy. Keeping the Model Router process and replacement session unchanged.", ); return; } - } + const sessionMatchesSandbox = + session?.sandboxName === sandbox.name && + resolveDestroyedSandboxRouterPort(session.endpointUrl) === port; + destroyedSessionId = session?.sandboxName === sandbox.name ? session.sessionId : null; - if (pid !== null) { - const log = deps.log ?? console.log; - 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) - }`, + const listHostRegistryEntries = + deps.listHostRegistryEntries ?? listHostGatewayRegistryEntries; + const home = (deps.resolveHomeDir ?? (() => process.env.HOME || os.homedir()))(); + // Called after selected-registry removal, so every remaining host entry is + // a peer, including entries owned by a different gateway state root. + const routedPeerRemains = listHostRegistryEntries(home).some(({ entry }) => { + const provider = typeof entry.provider === "string" ? entry.provider : null; + const endpointUrl = typeof entry.endpointUrl === "string" ? entry.endpointUrl : null; + return ( + isRoutedProvider(provider) && resolveDestroyedSandboxRouterPort(endpointUrl) === port ); - if (ownsPort(pid, port)) { + }); + if (routedPeerRemains) return; + + const ownsPort = deps.ownsPort ?? doesModelRouterProcessOwnPort; + const inspectProcessForPort = deps.inspectProcessForPort ?? inspectModelRouterProcessForPort; + const isHealthy = deps.isHealthy ?? isRouterHealthy; + const recordedPid = sessionMatchesSandbox ? (session.routerPid ?? null) : null; + const recordedCredentialHash = sessionMatchesSandbox + ? (session.routerCredentialHash ?? null) + : null; + let pid: number | null = null; + if (ownsPort(recordedPid, port)) { + pid = recordedPid as number; + } else { + const lookup = inspectProcessForPort(port); + if (lookup.status === "unavailable") { warn( - `Inspect PID ${pid} and the listener on port ${port}; stop the process only after confirming it is still the matching model-router proxy.`, + `Could not inspect the host process inventory for the Model Router on port ${port}. ` + + "Keeping its session recovery identity; inspect the port listener before the next Model Router onboarding.", ); - } else { + return; + } + if (lookup.status === "found") { + pid = lookup.pid; + } else if (await isHealthy(port, 1000)) { warn( - `PID ${pid} no longer identifies the matching model-router proxy. Do not stop it by PID; inspect the listener on port ${port}.`, + `No Model Router process could be confirmed for healthy port ${port}. ` + + "Keeping its session recovery identity; inspect the port listener before the next Model Router onboarding.", ); + return; } - return; } - } - // Clear when either field is set: a matching session with only a - // credential hash still carries stale router identity after its sandbox - // is gone. A completed process scan plus an unhealthy port confirms that - // no router remains when no PID was found. - if (sessionMatchesSandbox && (recordedPid !== null || recordedCredentialHash !== null)) { - const result = deps.compareAndSwapSession( - (current) => - current.sessionId === session.sessionId && - current.sandboxName === session.sandboxName && - current.endpointUrl === session.endpointUrl && - current.routerPid === recordedPid && - current.routerCredentialHash === recordedCredentialHash, - (current) => { - current.routerPid = null; - current.routerCredentialHash = null; - return current; - }, - "nemoclaw destroy Model Router session cleanup", - ); - if (result === "busy") { - warn( - "Another onboarding run owns the session lock. Keeping its Model Router recovery identity.", + if (pid !== null) { + const log = deps.log ?? console.log; + 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) + }`, + ); + if (ownsPort(pid, port)) { + warn( + `Inspect PID ${pid} and the listener on port ${port}; stop the process only after confirming it is still the matching model-router proxy.`, + ); + } else { + warn( + `PID ${pid} no longer identifies the matching model-router proxy. Do not stop it by PID; inspect the listener on port ${port}.`, + ); + } + return; + } + } + + // Clear when either field is set: a matching session with only a + // credential hash still carries stale router identity after its sandbox + // is gone. A completed process scan plus an unhealthy port confirms that + // no router remains when no PID was found. + if (sessionMatchesSandbox && (recordedPid !== null || recordedCredentialHash !== null)) { + deps.compareAndSwapSession( + (current) => + current.sessionId === session.sessionId && + current.sandboxName === session.sandboxName && + current.endpointUrl === session.endpointUrl && + current.routerPid === recordedPid && + current.routerCredentialHash === recordedCredentialHash, + (current) => { + current.routerPid = null; + current.routerCredentialHash = null; + return current; + }, + "nemoclaw destroy Model Router session cleanup", ); } + } finally { + try { + if (destroyedSessionId !== null) { + deps.compareAndSwapSession( + (current) => + current.sessionId === destroyedSessionId && current.sandboxName === sandbox.name, + (current) => { + current.sandboxName = null; + return current; + }, + "nemoclaw destroy sandbox session cleanup", + ); + } + } finally { + deps.releaseOnboardLock(); + } } }); + return true; } export function prepareSandboxDestroy(sandboxName: string): SandboxDestroyPreflight { diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 79273e3043a..77f53d3f0c4 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -36,7 +36,6 @@ import { import { validateName } from "../../runner"; import { killTimer as defaultKillShieldsTimer } from "../../shields/timer-control"; import { withMcpLifecycleLock } from "../../state/mcp-lifecycle-lock"; -import type { Session } from "../../state/onboard-session"; import * as onboardSession from "../../state/onboard-session"; import { resolveNemoclawStateDir } from "../../state/paths"; import * as registry from "../../state/registry"; @@ -473,6 +472,7 @@ async function destroySandboxUnlocked( ): Promise { const normalized = normalizeDestroySandboxOptions(options); if (!(await confirmSandboxDestroy(sandboxName, normalized))) return; + const destroySession = onboardSession.loadSession(); const inspectContainerIdentity = () => assertUnambiguousDestroyContainerIdentity(sandboxName, { @@ -688,16 +688,20 @@ async function destroySandboxUnlocked( if (deleteSucceededOrAlreadyGone && removed && priorHttpsPinRouteId) { await revokeDestroyedSandboxHttpsPinRoute(cleanupGatewayName, priorHttpsPinRouteId); } + let routedSessionCleanupHandled = false; if (deleteSucceededOrAlreadyGone && removed) { try { // The gateway route lock nests the host-global router-port lock inside // stopModelRouterForDestroyedSandbox. Routed onboarding takes the same // lock order and holds both through registry publication, including // when the competing sandbox belongs to another gateway. - await withGatewayRouteMutationLock(cleanupGatewayName, () => + routedSessionCleanupHandled = await withGatewayRouteMutationLock(cleanupGatewayName, () => stopModelRouterForDestroyedSandbox(sandbox, { + acquireOnboardLock: onboardSession.acquireOnboardLock, compareAndSwapSession: onboardSession.compareAndSwapSession, + expectedSession: destroySession, loadSession: onboardSession.loadSession, + releaseOnboardLock: onboardSession.releaseOnboardLock, warn: defaultDestroyWarn, }), ); @@ -710,12 +714,26 @@ async function destroySandboxUnlocked( ); } } - const session = onboardSession.loadSession(); - if (session && session.sandboxName === sandboxName) { - onboardSession.updateSession((s: Session) => { - s.sandboxName = null; - return s; - }); + if (!routedSessionCleanupHandled && destroySession?.sandboxName === sandboxName) { + const cleanupResult = onboardSession.compareAndSwapSession( + (current) => + current.sessionId === destroySession.sessionId && + current.updatedAt === destroySession.updatedAt && + current.sandboxName === destroySession.sandboxName && + current.endpointUrl === destroySession.endpointUrl && + current.routerPid === destroySession.routerPid && + current.routerCredentialHash === destroySession.routerCredentialHash, + (current) => { + current.sandboxName = null; + return current; + }, + "nemoclaw destroy sandbox session cleanup", + ); + if (cleanupResult === "busy") { + defaultDestroyWarn( + "Another onboarding run owns the session lock. Keeping its replacement session unchanged.", + ); + } } if ( shouldCleanupGatewayAfterConfirmedFinalDestroy({ diff --git a/src/lib/state/onboard-session-cross-process-lock.test.ts b/src/lib/state/onboard-session-cross-process-lock.test.ts index 7d91c94ad9d..d8f501a1c47 100644 --- a/src/lib/state/onboard-session-cross-process-lock.test.ts +++ b/src/lib/state/onboard-session-cross-process-lock.test.ts @@ -39,6 +39,28 @@ afterEach(() => { }); describe("cross-process onboard lock", () => { + it("updates under a caller-owned onboard lock without releasing it", () => { + session.saveSession( + session.createSession({ + sessionId: "destroy-session", + sandboxName: "alpha", + }), + ); + expect(session.acquireOnboardLock("nemoclaw destroy").acquired).toBe(true); + + const result = session.compareAndSwapSession( + (current) => current.sessionId === "destroy-session", + (current) => { + current.sandboxName = null; + return current; + }, + ); + + expect(result).toBe("updated"); + expect(session.loadSession()?.sandboxName).toBeNull(); + expect(fs.existsSync(session.LOCK_FILE)).toBe(true); + }); + it("reports the holder without acquiring a competing lock", async () => { const childScript = ` const fs = require("node:fs"); @@ -95,8 +117,8 @@ describe("cross-process onboard lock", () => { })); const replacement = JSON.parse(fs.readFileSync(sessionFile, "utf8")); replacement.sessionId = "replacement-session"; - replacement.sandboxName = "beta"; - replacement.endpointUrl = "http://host.openshell.internal:4200/v1"; + replacement.sandboxName = "alpha"; + replacement.endpointUrl = "http://host.openshell.internal:4000/v1"; replacement.routerPid = 6262; replacement.routerCredentialHash = "replacement-hash"; const tempFile = sessionFile + ".replacement"; @@ -126,8 +148,8 @@ describe("cross-process onboard lock", () => { expect(result).toBe("busy"); expect(session.loadSession()).toMatchObject({ sessionId: "replacement-session", - sandboxName: "beta", - endpointUrl: "http://host.openshell.internal:4200/v1", + sandboxName: "alpha", + endpointUrl: "http://host.openshell.internal:4000/v1", routerPid: 6262, routerCredentialHash: "replacement-hash", }); diff --git a/src/lib/state/onboard-session.ts b/src/lib/state/onboard-session.ts index 2cfe75f4773..dbe4cc4c065 100644 --- a/src/lib/state/onboard-session.ts +++ b/src/lib/state/onboard-session.ts @@ -520,13 +520,11 @@ function parseSessionMetadata(value: SessionJsonValue | undefined): SessionMetad !hasUnsafeHostMountTerminalText(candidate.target) && candidate.readOnly === true, ) - ? value.hostMounts.map( - (candidate): SandboxHostMount => ({ - source: (candidate as { source: string }).source, - target: (candidate as { target: string }).target, - readOnly: true, - }), - ) + ? value.hostMounts.map((candidate): SandboxHostMount => ({ + source: (candidate as { source: string }).source, + target: (candidate as { target: string }).target, + readOnly: true, + })) : []; return { gatewayName: readString(value.gatewayName) ?? "nemoclaw", @@ -1503,8 +1501,11 @@ export function compareAndSwapSession( mutator: (session: Session) => Session | void, command = "nemoclaw session compare-and-swap", ): CompareAndSwapSessionResult { - const lock = acquireOnboardLock(command); - if (!lock.acquired) return "busy"; + const ownsOnboardLock = heldLockFd === null; + if (ownsOnboardLock) { + const lock = acquireOnboardLock(command); + if (!lock.acquired) return "busy"; + } try { const current = loadSession(); if (!current || !matches(current)) return "mismatch"; @@ -1512,7 +1513,7 @@ export function compareAndSwapSession( saveSession(next); return "updated"; } finally { - releaseOnboardLock(); + if (ownsOnboardLock) releaseOnboardLock(); } } diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index 4c874f5ec79..7b0b78e4c2e 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -4,6 +4,7 @@ import { createRequire } from "node:module"; import { expect, type MockInstance, vi } from "vitest"; +import type { Session } from "../../src/lib/state/onboard-session"; import type { SandboxWorkloadReceipt } from "../../src/lib/state/registry"; type DestroySandbox = (typeof import("../../src/lib/actions/sandbox/destroy"))["destroySandbox"]; @@ -37,6 +38,7 @@ export type DestroyHarness = { restoreMcpBridgesAfterDestroyAbortSpy: MockInstance; runOpenshellSpy: MockInstance; selectGatewaySpy: MockInstance; + sessionState: Session; setSandboxPresent: (present: boolean) => void; shieldsDownSpy: MockInstance; stopAllSpy: MockInstance; @@ -75,6 +77,7 @@ type DestroyHarnessOptions = { promptResponses?: string[]; provider?: string; registeredSandboxCount?: number; + replaceSessionAfterRegistryRemoval?: boolean; removeSandboxResult?: boolean; restoreMcpError?: string; sandboxPresent?: boolean; @@ -149,6 +152,15 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr resetDestroyModuleCache(); const events: string[] = []; let sandboxPresent = options.sandboxPresent !== false; + let sessionLockBusy = false; + const sessionState = { + sessionId: "session-alpha", + updatedAt: "2026-08-14T00:00:00.000Z", + sandboxName: "alpha", + endpointUrl: options.endpointUrl ?? null, + routerPid: options.sessionRouterPid ?? null, + routerCredentialHash: options.sessionRouterPid ? "router-hash" : null, + } as Session; const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); @@ -218,6 +230,15 @@ 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); + if (options.replaceSessionAfterRegistryRemoval) { + sessionState.sessionId = "replacement-session"; + sessionState.updatedAt = "2026-08-14T00:01:00.000Z"; + sessionState.sandboxName = "alpha"; + sessionState.endpointUrl = "http://host.openshell.internal:4000/v1"; + sessionState.routerPid = 6262; + sessionState.routerCredentialHash = "replacement-hash"; + sessionLockBusy = true; + } return true; }); const stopModelRouterForDestroyedSandboxSpy = vi.spyOn( @@ -248,21 +269,28 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr status: "absent", }); vi.spyOn(modelRouterProcess, "isRouterHealthy").mockResolvedValue(false); - vi.spyOn(onboardSession, "loadSession").mockReturnValue({ - sandboxName: "alpha", - ...(options.sessionRouterPid ? { routerPid: options.sessionRouterPid } : {}), - }); + vi.spyOn(onboardSession, "loadSession").mockImplementation(() => ({ ...sessionState })); + vi.spyOn(onboardSession, "acquireOnboardLock").mockImplementation(() => + sessionLockBusy + ? { + acquired: false, + lockFile: "/tmp/onboard.lock", + stale: false, + holderPid: 6262, + holderStartedAt: "2026-08-14T00:01:00.000Z", + holderCommand: "replacement nemoclaw onboard process", + } + : { acquired: true, lockFile: "/tmp/onboard.lock", stale: false }, + ); + vi.spyOn(onboardSession, "releaseOnboardLock").mockImplementation(() => undefined); const compareAndSwapSessionSpy = vi .spyOn(onboardSession, "compareAndSwapSession") .mockImplementation((matches: unknown, mutator: unknown) => { - const session = { - sandboxName: "alpha", - ...(options.sessionRouterPid ? { routerPid: options.sessionRouterPid } : {}), - }; expect(typeof matches).toBe("function"); expect(typeof mutator).toBe("function"); - if (!(matches as (value: typeof session) => boolean)(session)) return "mismatch"; - (mutator as (value: typeof session) => void)(session); + if (sessionLockBusy) return "busy"; + if (!(matches as (value: Session) => boolean)(sessionState)) return "mismatch"; + (mutator as (value: Session) => void)(sessionState); return "updated"; }); const updateSessionSpy = vi @@ -460,6 +488,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr restoreMcpBridgesAfterDestroyAbortSpy, runOpenshellSpy, selectGatewaySpy, + sessionState, setSandboxPresent: (present: boolean) => { sandboxPresent = present; }, diff --git a/test/package-contract/destroy-model-router-flow.test.ts b/test/package-contract/destroy-model-router-flow.test.ts index 643afcc50d3..517ffe37794 100644 --- a/test/package-contract/destroy-model-router-flow.test.ts +++ b/test/package-contract/destroy-model-router-flow.test.ts @@ -91,10 +91,7 @@ describe("destroySandbox model-router teardown (#9098)", () => { routerCredentialHash: "router-credential-hash", } as Session; const compareAndSwapSession = vi.fn( - ( - matches: (current: Session) => boolean, - mutator: (current: Session) => Session | void, - ) => { + (matches: (current: Session) => boolean, mutator: (current: Session) => Session | void) => { if (!matches(session)) return "mismatch" as const; mutator(session); return "updated" as const; @@ -109,19 +106,30 @@ describe("destroySandbox model-router teardown (#9098)", () => { endpointUrl: session.endpointUrl, }, { + acquireOnboardLock: () => ({ + acquired: true, + lockFile: "/tmp/onboard.lock", + stale: false, + }), listHostRegistryEntries: () => [], compareAndSwapSession, + expectedSession: session, loadSession: () => session, + releaseOnboardLock: () => undefined, withModelRouterPortLifecycleLock: async (_port, operation) => await operation(), }, ), - ).resolves.toBeUndefined(); + ).resolves.toBe(true); await vi.waitFor(() => expect(stubExited).toBe(true), { timeout: 8_000, interval: 100 }); expect(await probeHealthy(port)).toBe(false); - expect(compareAndSwapSession).toHaveBeenCalledOnce(); + expect(compareAndSwapSession).toHaveBeenCalledTimes(2); expect(session).toEqual( - expect.objectContaining({ routerPid: null, routerCredentialHash: null }), + expect.objectContaining({ + sandboxName: null, + routerPid: null, + routerCredentialHash: null, + }), ); }, ); From 1b5e8961598ce906e331b8f1c66dcf74ded01af7 Mon Sep 17 00:00:00 2001 From: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Date: Fri, 14 Aug 2026 15:43:39 -0700 Subject: [PATCH 09/16] test(model-router): exercise reused session cleanup Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> --- .../sandbox/destroy-model-router.test.ts | 17 ++++++++++++++--- .../gateway-route-mutation-lock.test.ts | 5 +---- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-model-router.test.ts b/src/lib/actions/sandbox/destroy-model-router.test.ts index 6e52d0380bd..93168b09384 100644 --- a/src/lib/actions/sandbox/destroy-model-router.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -180,14 +180,25 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(unrelatedSession.routerCredentialHash).toBe("beta-hash"); }); - it("preserves a reused sandbox-name session for another router port", async () => { + it("clears the destroyed sandbox association without clearing another router port", async () => { const reusedNameSession = { + sessionId: "replacement-session", + updatedAt: "2026-08-14T00:01:00.000Z", sandboxName: "alpha", endpointUrl: "http://host.openshell.internal:4200/v1", routerPid: 6262, routerCredentialHash: "new-hash", } as Session; + const compareAndSwapSession = vi.fn( + (matches: (current: Session) => boolean, mutator: (current: Session) => Session | void) => { + if (!matches(reusedNameSession)) return "mismatch"; + mutator(reusedNameSession); + return "updated"; + }, + ); const { deps } = createDeps({ + compareAndSwapSession, + expectedSession: reusedNameSession, loadSession: vi.fn(() => reusedNameSession), ownsPort: vi.fn(() => false), inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), @@ -195,9 +206,9 @@ describe("stopModelRouterForDestroyedSandbox", () => { await stopModelRouterForDestroyedSandbox(routedSandbox, deps); - expect(deps.compareAndSwapSession).not.toHaveBeenCalled(); + expect(compareAndSwapSession).toHaveBeenCalledOnce(); expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(reusedNameSession.sandboxName).toBe("alpha"); + expect(reusedNameSession.sandboxName).toBeNull(); expect(reusedNameSession.routerPid).toBe(6262); expect(reusedNameSession.routerCredentialHash).toBe("new-hash"); }); diff --git a/src/lib/inference/gateway-route-mutation-lock.test.ts b/src/lib/inference/gateway-route-mutation-lock.test.ts index ba7cb9f97af..f93280c0676 100644 --- a/src/lib/inference/gateway-route-mutation-lock.test.ts +++ b/src/lib/inference/gateway-route-mutation-lock.test.ts @@ -5,10 +5,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { describe, expect, it, vi } from "vitest"; -import { - withGatewayRouteMutationLock, - withModelRouterPortLifecycleLock, -} from "./gateway-route-mutation-lock"; +import { withGatewayRouteMutationLock } from "./gateway-route-mutation-lock"; describe("gateway route mutation lock", () => { it("serializes separate operations for the same gateway", async () => { From a98070cd1aee86bc16994ca668f0535ccec4f558 Mon Sep 17 00:00:00 2001 From: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Date: Fri, 14 Aug 2026 15:51:47 -0700 Subject: [PATCH 10/16] refactor(onboard): default router lifecycle dependencies Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> --- src/lib/onboard.ts | 5 ----- .../onboard/machine/handlers/provider-inference.ts | 11 ++++++++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 839ba224d0f..b37ad45c184 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -2781,8 +2781,6 @@ function getSetupInferenceDeps(): SetupInferenceDeps { return { checkGatewayRouteCompatibility, withGatewayRouteMutationLock: gatewayRouteMutationLock.withGatewayRouteMutationLock, - withModelRouterPortLifecycleLock: gatewayRouteMutationLock.withModelRouterPortLifecycleLock, - getModelRouterPort: modelRouter.resolveModelRouterPort, withSandboxMutationLock: sandboxMutationLock.withSandboxMutationLock, step, getGatewayName: () => GATEWAY_NAME, @@ -3439,9 +3437,6 @@ async function runOnboard(opts: OnboardOptions = {}): Promise { preflightGatewayRouteDiscovery, getSandboxRecoveryAuthority: providerRecovery.getSandboxRecoveryAuthority, withGatewayRouteMutationLock: gatewayRouteMutationLock.withGatewayRouteMutationLock, - withModelRouterPortLifecycleLock: - gatewayRouteMutationLock.withModelRouterPortLifecycleLock, - getModelRouterPort: modelRouter.resolveModelRouterPort, normalizeHermesAuthMethod, setupNim: (g, s, a, recover, gateway, assertRouteCompatible, canProbeRoute, recoverySessionId) => setupNim(g, s, a, recover, opts.rebuildRegistryInferenceRoute, gateway, assertRouteCompatible, canProbeRoute, recoverySessionId), setupInference, resolveHostLocalInferenceStartupSelection: () => null, diff --git a/src/lib/onboard/machine/handlers/provider-inference.ts b/src/lib/onboard/machine/handlers/provider-inference.ts index 441cc58c36c..dd39d539cfa 100644 --- a/src/lib/onboard/machine/handlers/provider-inference.ts +++ b/src/lib/onboard/machine/handlers/provider-inference.ts @@ -10,6 +10,7 @@ import { type GatewayRouteDiscoveryConstraints, isAdvisoryGatewayRouteConflict, } from "../../../inference/gateway-route-compatibility"; +import { withModelRouterPortLifecycleLock } from "../../../inference/gateway-route-mutation-lock"; import { getOllamaContextWindowFloorForAgent } from "../../../inference/ollama-runtime-context"; import type { InferenceEndpointSource } from "../../../inference/selection"; import type { ServingProfileProvenance } from "../../../inference/serving/types"; @@ -18,6 +19,7 @@ import type { HermesAuthMethod, Session, SessionUpdates } from "../../../state/o import { checkpointSandboxIdentityMatches } from "../../checkpoint-replay"; import type { OnboardInferenceCapabilityCache } from "../../inference-capability-cache"; import type { RepairLocalInferenceSystemdOverrideOptions } from "../../local-inference-topology"; +import { resolveModelRouterPort } from "../../model-router"; import { promptOnboardConfigurationReview } from "../../prompt-helpers"; import { describeIgnoredReasoningEffortEnv, @@ -165,11 +167,11 @@ export interface ProviderInferenceStateOptions { gatewayName: string, operation: () => Promise | T, ): Promise; - withModelRouterPortLifecycleLock( + withModelRouterPortLifecycleLock?( port: number, operation: () => Promise | T, ): Promise; - getModelRouterPort(): number; + getModelRouterPort?(): number; normalizeHermesAuthMethod(value: string | null | undefined): HermesAuthMethod | null; setupNim( gpu: Gpu, @@ -1461,8 +1463,11 @@ export async function handleProviderInferenceState({ // #4564: re-upsert the gateway provider with the sandbox-facing // endpoint so a stale localhost base URL recorded by an earlier run is // repaired on resume instead of surviving and breaking inference.local. + const withRouterPortLifecycleLock = + deps.withModelRouterPortLifecycleLock ?? withModelRouterPortLifecycleLock; + const getRouterPort = deps.getModelRouterPort ?? resolveModelRouterPort; const routedRepair = await deps.withGatewayRouteMutationLock(gatewayName, () => - deps.withModelRouterPortLifecycleLock(deps.getModelRouterPort(), async () => { + withRouterPortLifecycleLock(getRouterPort(), async () => { assertProviderInferenceRouteCompatible(deps, gatewayName, sandboxName, { provider: selectedProvider, model: selectedModel, From a50cc1752f5ae8188e5f44eec99b6c4123d8ce9f Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Fri, 14 Aug 2026 15:55:36 -0700 Subject: [PATCH 11/16] refactor(model-router): clarify lifecycle locking Signed-off-by: Carlos Villela --- docs/inference/set-up-model-router.mdx | 12 +++++++++--- docs/reference/commands.mdx | 26 +++++++++----------------- src/lib/state/onboard-session.ts | 14 +++++++------- 3 files changed, 25 insertions(+), 27 deletions(-) diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index 41f281c67b4..7c357f99aff 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -32,6 +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. + +## Router and Sandbox Lifecycle Locks + Routed onboarding acquires the selected gateway route lock first, then the host-wide lock for the selected Model Router port. It holds both locks through router setup and sandbox registry publication. Before sandbox deletion, `destroy` captures the current onboarding session identity. @@ -46,9 +49,12 @@ The final sandbox-name cleanup also checks the captured session identity. Non-Model Router cleanup uses a non-blocking session update. If onboarding owns the lock or a same-name replacement has a different identity, `destroy` leaves the replacement session unchanged. If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destroy still completes and keeps the recovery identity. -The warning tells you to inspect the current listener process immediately before you stop anything. -Stop it only if its command line still identifies the Model Router on the named port. -Do not stop a previously reported process ID when its command line no longer matches. +If `destroy` keeps the recovery identity, follow these steps: + +1. Inspect the current listener process immediately before you stop anything. +2. Stop it only if its command line identifies the Model Router on the named port. +3. Do not stop a previously reported process ID if its command line no longer matches. + While another registered Model Router sandbox in any host gateway registry uses the same port, destroying one Model Router sandbox keeps the process running. A Model Router sandbox on another port does not keep the 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. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index a0af5ce70ae..dbb890df541 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2293,23 +2293,15 @@ 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. -Routed onboarding acquires the selected gateway route lock first, then the host-wide lifecycle lock for each Model Router port. -Onboarding holds both locks through sandbox registry publication. -Before sandbox deletion, `destroy` captures the current onboarding session identity. -Model Router destruction takes the gateway route lock, then the host-wide Model Router port lock, and then tries the onboarding session lock without waiting. -After acquiring the session lock, `destroy` rechecks the captured identity before the peer check or process stop. -If another onboarding run owns the session lock, or the identity changed, `destroy` skips Model Router teardown and warns. -It leaves the Model Router process and replacement session unchanged. -When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. -If no same-port peer remains, it stops only a process whose command line still identifies the Model Router on that port. -NemoClaw clears the matching Model Router process and credential recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. -The final sandbox-name cleanup also checks the captured session identity. -Non-Model Router cleanup uses a non-blocking session update. -If onboarding owns the lock or a same-name replacement has a different identity, `destroy` leaves the replacement session unchanged. -If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destruction still completes and keeps the recovery identity. -Inspect the current listener process immediately before you stop anything. -Stop it only if its command line still identifies the Model Router on the named port. -Do not stop a previously reported process ID when its command line no longer matches. +For Model Router sandboxes, `destroy` keeps the process and recovery identity when it cannot prove that teardown is safe. +It also preserves a replacement onboarding session when the captured session identity changed. +For lock order, same-port peer handling, and cleanup checks, refer to [Set Up Model Router](../inference/hosted-inference/set-up-model-router#router-and-sandbox-lifecycle-locks). + +If `destroy` keeps the Model Router recovery identity: + +1. Inspect the current listener process immediately before you stop anything. +2. Stop it only if its command line identifies the Model Router on the named port. +3. Do not stop a previously reported process ID if its command line no longer matches. 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/state/onboard-session.ts b/src/lib/state/onboard-session.ts index dbe4cc4c065..ce008bc03ea 100644 --- a/src/lib/state/onboard-session.ts +++ b/src/lib/state/onboard-session.ts @@ -1490,19 +1490,19 @@ export function updateSession(mutator: (session: Session) => Session | void): Se export type CompareAndSwapSessionResult = "updated" | "busy" | "mismatch"; /** - * Mutate the current session only while no onboarding writer owns its lock. + * Mutate the current session while this process owns the onboarding lock. * - * Production onboarding holds `LOCK_FILE` across its session writes. Reusing - * that boundary closes the load-before-rename race for short mutations from a - * different command without waiting while an onboarding run is active. + * Reuse the process-local `LOCK_FILE` lock when the caller already holds it. + * Otherwise, acquire the lock without waiting and return `busy` when another + * onboarding writer owns it. */ export function compareAndSwapSession( matches: (session: Session) => boolean, mutator: (session: Session) => Session | void, command = "nemoclaw session compare-and-swap", ): CompareAndSwapSessionResult { - const ownsOnboardLock = heldLockFd === null; - if (ownsOnboardLock) { + const managesOnboardLock = heldLockFd === null; + if (managesOnboardLock) { const lock = acquireOnboardLock(command); if (!lock.acquired) return "busy"; } @@ -1513,7 +1513,7 @@ export function compareAndSwapSession( saveSession(next); return "updated"; } finally { - if (ownsOnboardLock) releaseOnboardLock(); + if (managesOnboardLock) releaseOnboardLock(); } } From 731d7d8e733655eaaf8cd192e834b290c5821497 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Fri, 14 Aug 2026 16:01:41 -0700 Subject: [PATCH 12/16] test(model-router): keep session fixtures linear Signed-off-by: Carlos Villela --- .../sandbox/destroy-model-router.test.ts | 18 ++++++------------ .../destroy-model-router-flow.test.ts | 6 +++--- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-model-router.test.ts b/src/lib/actions/sandbox/destroy-model-router.test.ts index 93168b09384..c355d348df4 100644 --- a/src/lib/actions/sandbox/destroy-model-router.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -33,9 +33,7 @@ function createDeps(overrides: Partial = stale: false, })), compareAndSwapSession: vi.fn((matches, mutator) => { - if (!matches(session)) return "mismatch"; - mutator(session); - return "updated"; + return matches(session) ? (mutator(session), "updated") : "mismatch"; }), expectedSession: session, inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), @@ -191,9 +189,9 @@ describe("stopModelRouterForDestroyedSandbox", () => { } as Session; const compareAndSwapSession = vi.fn( (matches: (current: Session) => boolean, mutator: (current: Session) => Session | void) => { - if (!matches(reusedNameSession)) return "mismatch"; - mutator(reusedNameSession); - return "updated"; + return matches(reusedNameSession) + ? (mutator(reusedNameSession), "updated") + : "mismatch"; }, ); const { deps } = createDeps({ @@ -315,9 +313,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { loadSession: vi.fn(() => session), ownsPort: vi.fn(() => false), compareAndSwapSession: vi.fn((matches, mutator) => { - if (!matches(session)) return "mismatch"; - mutator(session); - return "updated"; + return matches(session) ? (mutator(session), "updated") : "mismatch"; }), }); @@ -341,9 +337,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { loadSession: vi.fn(() => session), ownsPort: vi.fn(() => false), compareAndSwapSession: vi.fn((matches, mutator) => { - if (!matches(session)) return "mismatch"; - mutator(session); - return "updated"; + return matches(session) ? (mutator(session), "updated") : "mismatch"; }), }); diff --git a/test/package-contract/destroy-model-router-flow.test.ts b/test/package-contract/destroy-model-router-flow.test.ts index 517ffe37794..d5277d2ad4f 100644 --- a/test/package-contract/destroy-model-router-flow.test.ts +++ b/test/package-contract/destroy-model-router-flow.test.ts @@ -92,9 +92,9 @@ describe("destroySandbox model-router teardown (#9098)", () => { } as Session; const compareAndSwapSession = vi.fn( (matches: (current: Session) => boolean, mutator: (current: Session) => Session | void) => { - if (!matches(session)) return "mismatch" as const; - mutator(session); - return "updated" as const; + return matches(session) + ? (mutator(session), "updated" as const) + : ("mismatch" as const); }, ); From 68def061a2f9759464c287f65e358268ebc2f3a1 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 16:07:38 -0700 Subject: [PATCH 13/16] fix(sandbox): preserve replacement router sessions Signed-off-by: Prekshi Vyas --- docs/inference/set-up-model-router.mdx | 20 ++++++++++++------- docs/reference/commands.mdx | 20 ++++--------------- .../sandbox/destroy-model-router.test.ts | 2 ++ src/lib/actions/sandbox/destroy-preflight.ts | 11 +++++----- src/lib/onboard.ts | 5 ----- .../machine/handlers/provider-inference.ts | 17 ++++++++++------ src/lib/onboard/model-router.ts | 4 +++- .../setup-inference-route-containment.test.ts | 10 ++++++++++ src/lib/state/onboard-session.ts | 16 ++++++++------- 9 files changed, 57 insertions(+), 48 deletions(-) diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index 41f281c67b4..c62d0852ca7 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -32,23 +32,29 @@ 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. + +## Router and Sandbox Lifecycle Locks + Routed onboarding acquires the selected gateway route lock first, then the host-wide lock for the selected Model Router port. It holds both locks through router setup and sandbox registry publication. Before sandbox deletion, `destroy` captures the current onboarding session identity. Model Router destruction takes the gateway route lock, then the host-wide Model Router port lock, and then tries the onboarding session lock without waiting. After acquiring the session lock, `destroy` rechecks the captured identity before the peer check or process stop. If another onboarding run owns the session lock, or the identity changed, `destroy` skips Model Router teardown and warns. -It leaves the Model Router process and replacement session unchanged. +It leaves the Model Router process and current onboarding session unchanged. When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. If no same-port peer remains, NemoClaw stops only a process whose command line still identifies the Model Router on that port. NemoClaw clears the matching Model Router process and credential recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. The final sandbox-name cleanup also checks the captured session identity. -Non-Model Router cleanup uses a non-blocking session update. -If onboarding owns the lock or a same-name replacement has a different identity, `destroy` leaves the replacement session unchanged. -If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destroy still completes and keeps the recovery identity. -The warning tells you to inspect the current listener process immediately before you stop anything. -Stop it only if its command line still identifies the Model Router on the named port. -Do not stop a previously reported process ID when its command line no longer matches. +Other sandbox destroy paths use a non-blocking session update. +If onboarding owns the lock, the session identity changed, or a same-name session uses another router port, `destroy` leaves the current session unchanged. +If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, `destroy` still completes and keeps the recovery identity. +When `destroy` warns about the remaining listener: + +- Inspect the current listener process immediately before you stop anything. +- Stop it only if its command line still identifies the Model Router on the named port. +- Do not stop a previously reported process ID when its command line no longer matches. + While another registered Model Router sandbox in any host gateway registry uses the same port, destroying one Model Router sandbox keeps the process running. A Model Router sandbox on another port does not keep the 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. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index a0af5ce70ae..ff4c1d8b1e1 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2293,23 +2293,11 @@ 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. -Routed onboarding acquires the selected gateway route lock first, then the host-wide lifecycle lock for each Model Router port. -Onboarding holds both locks through sandbox registry publication. Before sandbox deletion, `destroy` captures the current onboarding session identity. -Model Router destruction takes the gateway route lock, then the host-wide Model Router port lock, and then tries the onboarding session lock without waiting. -After acquiring the session lock, `destroy` rechecks the captured identity before the peer check or process stop. -If another onboarding run owns the session lock, or the identity changed, `destroy` skips Model Router teardown and warns. -It leaves the Model Router process and replacement session unchanged. -When `destroy` removes a Model Router sandbox, it checks the bounded set of NemoClaw gateway registries under the host state directory for a same-port peer. -If no same-port peer remains, it stops only a process whose command line still identifies the Model Router on that port. -NemoClaw clears the matching Model Router process and credential recovery identity only after the stop succeeds or a complete process scan and health probe confirm that the router is absent. -The final sandbox-name cleanup also checks the captured session identity. -Non-Model Router cleanup uses a non-blocking session update. -If onboarding owns the lock or a same-name replacement has a different identity, `destroy` leaves the replacement session unchanged. -If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, destruction still completes and keeps the recovery identity. -Inspect the current listener process immediately before you stop anything. -Stop it only if its command line still identifies the Model Router on the named port. -Do not stop a previously reported process ID when its command line no longer matches. +For a Model Router sandbox, it stops the matching process only when no same-port peer remains and the captured session identity still matches the current session. +If the session lock is busy, the session identity changed, process inspection is inconclusive, or process stop fails, sandbox destruction still completes and keeps the recovery identity. +Inspect the current listener and confirm its Model Router command line before you stop it. +Refer to [Set Up Model Router](../inference/hosted-inference/set-up-model-router#router-and-sandbox-lifecycle-locks) for the lock order, replacement-session protection, and recovery behavior. 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 6e52d0380bd..57ab4199ba6 100644 --- a/src/lib/actions/sandbox/destroy-model-router.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -182,12 +182,14 @@ describe("stopModelRouterForDestroyedSandbox", () => { it("preserves a reused sandbox-name session for another router port", async () => { const reusedNameSession = { + sessionId: "session-replacement", sandboxName: "alpha", endpointUrl: "http://host.openshell.internal:4200/v1", routerPid: 6262, routerCredentialHash: "new-hash", } as Session; const { deps } = createDeps({ + expectedSession: reusedNameSession, loadSession: vi.fn(() => reusedNameSession), ownsPort: vi.fn(() => false), inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), diff --git a/src/lib/actions/sandbox/destroy-preflight.ts b/src/lib/actions/sandbox/destroy-preflight.ts index 52edc3cf427..eca329c9685 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -5,7 +5,10 @@ import os from "node:os"; import { OPENSHELL_PROBE_TIMEOUT_MS } from "../../adapters/openshell/timeouts"; import { withModelRouterPortLifecycleLock } from "../../inference/gateway-route-mutation-lock"; -import { isRoutedInferenceProvider } from "../../onboard/model-router"; +import { + DEFAULT_MODEL_ROUTER_PORT, + isRoutedInferenceProvider, +} from "../../onboard/model-router"; import { doesModelRouterProcessOwnPort, inspectModelRouterProcessForPort, @@ -59,10 +62,6 @@ 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 = { acquireOnboardLock: typeof acquireOnboardLock; compareAndSwapSession: typeof compareAndSwapSession; @@ -153,7 +152,7 @@ export async function stopModelRouterForDestroyedSandbox( const sessionMatchesSandbox = session?.sandboxName === sandbox.name && resolveDestroyedSandboxRouterPort(session.endpointUrl) === port; - destroyedSessionId = session?.sandboxName === sandbox.name ? session.sessionId : null; + destroyedSessionId = sessionMatchesSandbox ? session.sessionId : null; const listHostRegistryEntries = deps.listHostRegistryEntries ?? listHostGatewayRegistryEntries; diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 839ba224d0f..b37ad45c184 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -2781,8 +2781,6 @@ function getSetupInferenceDeps(): SetupInferenceDeps { return { checkGatewayRouteCompatibility, withGatewayRouteMutationLock: gatewayRouteMutationLock.withGatewayRouteMutationLock, - withModelRouterPortLifecycleLock: gatewayRouteMutationLock.withModelRouterPortLifecycleLock, - getModelRouterPort: modelRouter.resolveModelRouterPort, withSandboxMutationLock: sandboxMutationLock.withSandboxMutationLock, step, getGatewayName: () => GATEWAY_NAME, @@ -3439,9 +3437,6 @@ async function runOnboard(opts: OnboardOptions = {}): Promise { preflightGatewayRouteDiscovery, getSandboxRecoveryAuthority: providerRecovery.getSandboxRecoveryAuthority, withGatewayRouteMutationLock: gatewayRouteMutationLock.withGatewayRouteMutationLock, - withModelRouterPortLifecycleLock: - gatewayRouteMutationLock.withModelRouterPortLifecycleLock, - getModelRouterPort: modelRouter.resolveModelRouterPort, normalizeHermesAuthMethod, setupNim: (g, s, a, recover, gateway, assertRouteCompatible, canProbeRoute, recoverySessionId) => setupNim(g, s, a, recover, opts.rebuildRegistryInferenceRoute, gateway, assertRouteCompatible, canProbeRoute, recoverySessionId), setupInference, resolveHostLocalInferenceStartupSelection: () => null, diff --git a/src/lib/onboard/machine/handlers/provider-inference.ts b/src/lib/onboard/machine/handlers/provider-inference.ts index 441cc58c36c..700a6146038 100644 --- a/src/lib/onboard/machine/handlers/provider-inference.ts +++ b/src/lib/onboard/machine/handlers/provider-inference.ts @@ -10,6 +10,7 @@ import { type GatewayRouteDiscoveryConstraints, isAdvisoryGatewayRouteConflict, } from "../../../inference/gateway-route-compatibility"; +import { withModelRouterPortLifecycleLock } from "../../../inference/gateway-route-mutation-lock"; import { getOllamaContextWindowFloorForAgent } from "../../../inference/ollama-runtime-context"; import type { InferenceEndpointSource } from "../../../inference/selection"; import type { ServingProfileProvenance } from "../../../inference/serving/types"; @@ -18,6 +19,7 @@ import type { HermesAuthMethod, Session, SessionUpdates } from "../../../state/o import { checkpointSandboxIdentityMatches } from "../../checkpoint-replay"; import type { OnboardInferenceCapabilityCache } from "../../inference-capability-cache"; import type { RepairLocalInferenceSystemdOverrideOptions } from "../../local-inference-topology"; +import { resolveModelRouterPort } from "../../model-router"; import { promptOnboardConfigurationReview } from "../../prompt-helpers"; import { describeIgnoredReasoningEffortEnv, @@ -165,11 +167,11 @@ export interface ProviderInferenceStateOptions { gatewayName: string, operation: () => Promise | T, ): Promise; - withModelRouterPortLifecycleLock( + withModelRouterPortLifecycleLock?( port: number, operation: () => Promise | T, ): Promise; - getModelRouterPort(): number; + getModelRouterPort?(): number; normalizeHermesAuthMethod(value: string | null | undefined): HermesAuthMethod | null; setupNim( gpu: Gpu, @@ -1461,8 +1463,11 @@ export async function handleProviderInferenceState({ // #4564: re-upsert the gateway provider with the sandbox-facing // endpoint so a stale localhost base URL recorded by an earlier run is // repaired on resume instead of surviving and breaking inference.local. - const routedRepair = await deps.withGatewayRouteMutationLock(gatewayName, () => - deps.withModelRouterPortLifecycleLock(deps.getModelRouterPort(), async () => { + const routedRepair = await deps.withGatewayRouteMutationLock(gatewayName, () => { + const withRouterPortLock = + deps.withModelRouterPortLifecycleLock ?? withModelRouterPortLifecycleLock; + const port = (deps.getModelRouterPort ?? resolveModelRouterPort)(); + return withRouterPortLock(port, async () => { assertProviderInferenceRouteCompatible(deps, gatewayName, sandboxName, { provider: selectedProvider, model: selectedModel, @@ -1503,8 +1508,8 @@ export async function handleProviderInferenceState({ }) : null; return { reupserted, reservationEndpointSource, reserved }; - }), - ); + }); + }); const { reupserted, reservationEndpointSource, reserved } = routedRepair; if (!reupserted.ok) { deps.error( diff --git a/src/lib/onboard/model-router.ts b/src/lib/onboard/model-router.ts index adb426067f7..ac6249fed9d 100644 --- a/src/lib/onboard/model-router.ts +++ b/src/lib/onboard/model-router.ts @@ -577,8 +577,10 @@ function getRoutedProfile(): BlueprintInferenceProfile { return bp; } +export const DEFAULT_MODEL_ROUTER_PORT = 4000; + export function resolveModelRouterPort(): number { - return getRoutedProfile().router?.port || 4000; + return getRoutedProfile().router?.port || DEFAULT_MODEL_ROUTER_PORT; } export function isRoutedInferenceProvider(provider: string | null | undefined): boolean { diff --git a/src/lib/onboard/setup-inference-route-containment.test.ts b/src/lib/onboard/setup-inference-route-containment.test.ts index a837c36941c..973c59d2261 100644 --- a/src/lib/onboard/setup-inference-route-containment.test.ts +++ b/src/lib/onboard/setup-inference-route-containment.test.ts @@ -378,6 +378,11 @@ describe("onboard shared gateway route containment", () => { withSandboxMutationLock: async (_sandboxName: string, operation: () => Promise | T) => await operation(), withGatewayRouteMutationLock, + withModelRouterPortLifecycleLock: async ( + _port: number, + operation: () => Promise | T, + ) => await operation(), + getModelRouterPort: () => 4000, step: vi.fn(), getGatewayName: () => "nemoclaw", runOpenshell, @@ -484,6 +489,11 @@ describe("onboard shared gateway route containment", () => { await operation(), withGatewayRouteMutationLock: async (_name: string, operation: () => Promise | T) => await operation(), + withModelRouterPortLifecycleLock: async ( + _port: number, + operation: () => Promise | T, + ) => await operation(), + getModelRouterPort: () => 4000, step: vi.fn(), getGatewayName: () => "nemoclaw", runOpenshell: vi.fn(() => ({ status: 0 })), diff --git a/src/lib/state/onboard-session.ts b/src/lib/state/onboard-session.ts index dbe4cc4c065..6a722e22b0b 100644 --- a/src/lib/state/onboard-session.ts +++ b/src/lib/state/onboard-session.ts @@ -1490,19 +1490,21 @@ export function updateSession(mutator: (session: Session) => Session | void): Se export type CompareAndSwapSessionResult = "updated" | "busy" | "mismatch"; /** - * Mutate the current session only while no onboarding writer owns its lock. + * Mutate the current session while this process owns the onboarding lock. * - * Production onboarding holds `LOCK_FILE` across its session writes. Reusing - * that boundary closes the load-before-rename race for short mutations from a - * different command without waiting while an onboarding run is active. + * Production onboarding holds `LOCK_FILE` across its session writes. When + * this process already owns that lock, the mutation reuses it. Otherwise, the + * mutation acquires the lock only when no onboarding writer owns it and + * returns `busy` without waiting. Reusing that boundary closes the + * load-before-rename race for short mutations from a different command. */ export function compareAndSwapSession( matches: (session: Session) => boolean, mutator: (session: Session) => Session | void, command = "nemoclaw session compare-and-swap", ): CompareAndSwapSessionResult { - const ownsOnboardLock = heldLockFd === null; - if (ownsOnboardLock) { + const acquiredLockHere = heldLockFd === null; + if (acquiredLockHere) { const lock = acquireOnboardLock(command); if (!lock.acquired) return "busy"; } @@ -1513,7 +1515,7 @@ export function compareAndSwapSession( saveSession(next); return "updated"; } finally { - if (ownsOnboardLock) releaseOnboardLock(); + if (acquiredLockHere) releaseOnboardLock(); } } From b1a7298c7017b8988dad3d32d17f70333338561b Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 16:30:42 -0700 Subject: [PATCH 14/16] docs(model-router): scope manual recovery steps Signed-off-by: Prekshi Vyas --- docs/inference/set-up-model-router.mdx | 5 +++-- docs/reference/commands.mdx | 5 +++-- src/lib/actions/sandbox/destroy-model-router.test.ts | 4 ++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index a713e49cfbc..4024d51100a 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -50,11 +50,12 @@ Other sandbox destroy paths use a non-blocking session update. If onboarding owns the lock or the captured session identity changed, `destroy` leaves the current session unchanged. If the captured session still names the destroyed sandbox but uses another router port, `destroy` clears only the sandbox association and preserves the router process and credential recovery identity. If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, `destroy` still completes and keeps the recovery identity. -If `destroy` keeps the recovery identity, follow these steps: +If `destroy` warns that it could not identify or stop a listener for the deleted sandbox, follow these steps: 1. Inspect the current listener process immediately before you stop anything. 2. Stop it only if its command line identifies the Model Router on the named port. -3. Do not stop a previously reported process ID if its command line no longer matches. +3. Do not stop the router recorded by a preserved session for another port. +4. Do not stop a previously reported process ID if its command line no longer matches. While another registered Model Router sandbox in any host gateway registry uses the same port, destroying one Model Router sandbox keeps the process running. A Model Router sandbox on another port does not keep the process running. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 4272631342e..fefd8daad31 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2306,11 +2306,12 @@ It also preserves a replacement onboarding session when the captured session ide If the captured session uses the destroyed sandbox name with another router port, `destroy` clears only the sandbox association and preserves that router's recovery identity. For lock order, same-port peer handling, and cleanup checks, refer to [Set Up Model Router](../inference/hosted-inference/set-up-model-router#router-and-sandbox-lifecycle-locks). -If `destroy` keeps the Model Router recovery identity: +If `destroy` warns that it could not identify or stop a listener for the deleted sandbox: 1. Inspect the current listener process immediately before you stop anything. 2. Stop it only if its command line identifies the Model Router on the named port. -3. Do not stop a previously reported process ID if its command line no longer matches. +3. Do not stop the router recorded by a preserved session for another port. +4. Do not stop a previously reported process ID if its command line no longer matches. 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 c355d348df4..9eaef8b4bd3 100644 --- a/src/lib/actions/sandbox/destroy-model-router.test.ts +++ b/src/lib/actions/sandbox/destroy-model-router.test.ts @@ -267,7 +267,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(session.routerCredentialHash).toBeNull(); }); - it("keeps session identity when process inventory is unavailable and the port is healthy", async () => { + it("keeps router recovery identity when process inventory is unavailable", async () => { const { deps, session } = createDeps({ ownsPort: vi.fn(() => false), inspectProcessForPort: vi.fn(() => ({ status: "unavailable" as const })), @@ -284,7 +284,7 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("process inventory")); }); - it("keeps session identity when no process is visible but the router port stays healthy", async () => { + it("keeps router recovery identity when no process is visible but the router port stays healthy", async () => { const { deps, session } = createDeps({ ownsPort: vi.fn(() => false), inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), From 28ef3cb6bc77c1c26fcbdedf39bef48f00f4cb87 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 14 Aug 2026 16:46:47 -0700 Subject: [PATCH 15/16] test(sandbox): keep Ollama stop checks within timeout Signed-off-by: Prekshi Vyas --- src/lib/actions/sandbox/stop.test.ts | 2 ++ src/lib/actions/sandbox/stop.ts | 13 +++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/lib/actions/sandbox/stop.test.ts b/src/lib/actions/sandbox/stop.test.ts index fcc04e13a7e..9561018cd53 100644 --- a/src/lib/actions/sandbox/stop.test.ts +++ b/src/lib/actions/sandbox/stop.test.ts @@ -9,6 +9,7 @@ import { type DockerRuntimeProviderDependencies, } from "../../onboard/runtime-provider/docker"; import { createRuntimeProviderBundleRegistry } from "../../onboard/runtime-provider/registry"; +import { exclusivelyHeldOllamaModel } from "../../inference/ollama/model-ownership"; import type { SandboxEntry } from "../../state/registry"; import { teardownSandboxDashboardForward } from "./forward-recovery"; import { type SandboxStopDeps, stopSandbox } from "./stop"; @@ -72,6 +73,7 @@ function harness(overrides: StopHarnessOverrides = {}) { teardownSandboxDashboardForward, log, warn, + exclusivelyHeldOllamaModel, withOllamaModelOwnershipLock: (operation) => operation(), ...actionOverrides, }; diff --git a/src/lib/actions/sandbox/stop.ts b/src/lib/actions/sandbox/stop.ts index 628da8b989f..f14065bb328 100644 --- a/src/lib/actions/sandbox/stop.ts +++ b/src/lib/actions/sandbox/stop.ts @@ -48,13 +48,17 @@ function unloadOllamaModelsBestEffort( ): void { if (!sandbox.provider?.includes("ollama")) return; try { - const ownership = require("../../inference/ollama/model-ownership") as typeof import("../../inference/ollama/model-ownership"); - const proxy = require("../../inference/ollama/proxy") as typeof import("../../inference/ollama/proxy"); const withOwnershipLock = - deps.withOllamaModelOwnershipLock ?? proxy.withOllamaModelOwnershipLock; + deps.withOllamaModelOwnershipLock ?? + (require("../../inference/ollama/proxy") as typeof import("../../inference/ollama/proxy")) + .withOllamaModelOwnershipLock; + const exclusivelyHeldOllamaModel = + deps.exclusivelyHeldOllamaModel ?? + (require("../../inference/ollama/model-ownership") as typeof import("../../inference/ollama/model-ownership")) + .exclusivelyHeldOllamaModel; withOwnershipLock(() => { const { sandboxes } = (deps.listSandboxes ?? registry.listSandboxes)(); - const model = ownership.exclusivelyHeldOllamaModel(sandbox, sandboxes); + const model = exclusivelyHeldOllamaModel(sandbox, sandboxes); if (!model) return; (deps.unloadOllamaModels ?? defaultUnloadOllamaModels)([model]); }); @@ -73,6 +77,7 @@ export interface SandboxStopDeps { teardownSandboxDashboardForward?: typeof teardownSandboxDashboardForward; listSandboxes?: typeof registry.listSandboxes; unloadOllamaModels?: (onlyModels: readonly string[]) => void; + exclusivelyHeldOllamaModel?: typeof import("../../inference/ollama/model-ownership").exclusivelyHeldOllamaModel; withOllamaModelOwnershipLock?: typeof import("../../inference/ollama/proxy").withOllamaModelOwnershipLock; log?: (message: string) => void; warn?: (message: string) => void; From ea4def461192227a28d2655fdb8c8aaf452f2c0c Mon Sep 17 00:00:00 2001 From: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:18:15 -0700 Subject: [PATCH 16/16] docs(router): clarify lifecycle lock scope Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> --- docs/inference/set-up-model-router.mdx | 9 +++++---- docs/reference/commands.mdx | 2 +- src/lib/actions/sandbox/destroy-preflight.ts | 7 ++----- src/lib/actions/sandbox/destroy.ts | 2 +- src/lib/inference/gateway-route-mutation-lock.ts | 6 +++--- 5 files changed, 12 insertions(+), 14 deletions(-) diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index 4024d51100a..55668a6a6c1 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -35,10 +35,11 @@ NemoClaw does not provide log-only cleanup. ## Router and Sandbox Lifecycle Locks -Routed onboarding acquires the selected gateway route lock first, then the host-wide lock for the selected Model Router port. -It holds both locks through router setup and sandbox registry publication. +Routed onboarding already holds the onboarding session lock when it acquires the selected gateway route lock, then the current-user lock for the selected Model Router port. +The port lock is shared across the current user's NemoClaw gateways. +Onboarding holds all three locks through router setup and sandbox registry publication. Before sandbox deletion, `destroy` captures the current onboarding session identity. -Model Router destruction takes the gateway route lock, then the host-wide Model Router port lock, and then tries the onboarding session lock without waiting. +Model Router destruction takes the gateway route lock, then the current-user Model Router port lock, and then tries the onboarding session lock without waiting. After acquiring the session lock, `destroy` rechecks the captured identity before the peer check or process stop. If another onboarding run owns the session lock, or the identity changed, `destroy` skips Model Router teardown and warns. It leaves the Model Router process and current onboarding session unchanged. @@ -49,7 +50,7 @@ The final sandbox-name cleanup also checks the captured session identity. Other sandbox destroy paths use a non-blocking session update. If onboarding owns the lock or the captured session identity changed, `destroy` leaves the current session unchanged. If the captured session still names the destroyed sandbox but uses another router port, `destroy` clears only the sandbox association and preserves the router process and credential recovery identity. -If process inspection is unavailable, the port remains healthy without a verified owner, or the stop fails, `destroy` still completes and keeps the recovery identity. +If the process inventory is unavailable, the completed scan finds no matching process while the port remains healthy, or the stop fails, `destroy` still completes and keeps the recovery identity. If `destroy` warns that it could not identify or stop a listener for the deleted sandbox, follow these steps: 1. Inspect the current listener process immediately before you stop anything. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index fefd8daad31..04b99d63c97 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2301,7 +2301,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. -For Model Router sandboxes, `destroy` keeps the process and recovery identity when it cannot prove that teardown is safe. +For Model Router sandboxes, `destroy` keeps the process and recovery identity when another sandbox uses the port or when session, process, or absence checks are inconclusive. It also preserves a replacement onboarding session when the captured session identity changed. If the captured session uses the destroyed sandbox name with another router port, `destroy` clears only the sandbox association and preserves that router's recovery identity. For lock order, same-port peer handling, and cleanup checks, refer to [Set Up Model Router](../inference/hosted-inference/set-up-model-router#router-and-sandbox-lifecycle-locks). diff --git a/src/lib/actions/sandbox/destroy-preflight.ts b/src/lib/actions/sandbox/destroy-preflight.ts index f903c38ee4d..496edb895b1 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -5,10 +5,7 @@ import os from "node:os"; import { OPENSHELL_PROBE_TIMEOUT_MS } from "../../adapters/openshell/timeouts"; import { withModelRouterPortLifecycleLock } from "../../inference/gateway-route-mutation-lock"; -import { - DEFAULT_MODEL_ROUTER_PORT, - isRoutedInferenceProvider, -} from "../../onboard/model-router"; +import { DEFAULT_MODEL_ROUTER_PORT, isRoutedInferenceProvider } from "../../onboard/model-router"; import { doesModelRouterProcessOwnPort, inspectModelRouterProcessForPort, @@ -115,7 +112,7 @@ export function resolveDestroyedSandboxRouterPort(endpointUrl: string | null | u * The recorded PID is preferred only when the session sandbox and router port * match the destroyed sandbox. Otherwise, the /proc scan recovers the orphan * by verified command line, exactly like reconcileModelRouter's recovery path. - * The host-global port lock and non-blocking onboarding session lock cover the + * The current-user port lock and non-blocking onboarding session lock cover the * peer scan, stop, and session update. Destroy skips teardown when onboarding * owns the session lock or the pre-delete session snapshot changed. A stop * failure or inconclusive process inventory is a warning, not an error: the diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 77f53d3f0c4..3b1ae1597ed 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -691,7 +691,7 @@ async function destroySandboxUnlocked( let routedSessionCleanupHandled = false; if (deleteSucceededOrAlreadyGone && removed) { try { - // The gateway route lock nests the host-global router-port lock inside + // The gateway route lock nests the current-user router-port lock inside // stopModelRouterForDestroyedSandbox. Routed onboarding takes the same // lock order and holds both through registry publication, including // when the competing sandbox belongs to another gateway. diff --git a/src/lib/inference/gateway-route-mutation-lock.ts b/src/lib/inference/gateway-route-mutation-lock.ts index 46749163c10..697fc5dd734 100644 --- a/src/lib/inference/gateway-route-mutation-lock.ts +++ b/src/lib/inference/gateway-route-mutation-lock.ts @@ -10,7 +10,7 @@ import { resolveSharedLocalAdapterStateRoot } from "./local-adapter-lifecycle"; const GATEWAY_ROUTE_LOCK_PREFIX = "gateway-route:"; const MODEL_ROUTER_PORT_LOCK_PREFIX = "model-router-port:"; -export function resolveHostGlobalModelRouterLockStateDir(homeDir: string = os.homedir()): string { +export function resolveCurrentUserModelRouterLockStateDir(homeDir: string = os.homedir()): string { return path.join(resolveSharedLocalAdapterStateRoot(homeDir), "state"); } @@ -33,7 +33,7 @@ export function withGatewayRouteMutationLock( ); } -/** Serialize lifecycle changes for the host-global Model Router port. */ +/** Serialize current-user lifecycle changes for one Model Router port across gateways. */ export function withModelRouterPortLifecycleLock( port: number, operation: () => Promise | T, @@ -42,7 +42,7 @@ export function withModelRouterPortLifecycleLock( if (!Number.isInteger(port) || port < 1 || port > 65535) { throw new Error("Model Router port must be an integer from 1 to 65535."); } - const stateDir = options.stateDir ?? resolveHostGlobalModelRouterLockStateDir(); + const stateDir = options.stateDir ?? resolveCurrentUserModelRouterLockStateDir(); return withMcpLifecycleLock(`${MODEL_ROUTER_PORT_LOCK_PREFIX}${String(port)}`, operation, { ...options, stateDir,