diff --git a/docs/inference/set-up-model-router.mdx b/docs/inference/set-up-model-router.mdx index aa7694f33c9..55668a6a6c1 100644 --- a/docs/inference/set-up-model-router.mdx +++ b/docs/inference/set-up-model-router.mdx @@ -32,9 +32,34 @@ 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. + +## Router and Sandbox Lifecycle Locks + +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 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. +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. +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 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. +2. Stop it only if its command line identifies the Model Router on the named port. +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. 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 e7021a7cc76..04b99d63c97 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2301,7 +2301,17 @@ 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. +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). + +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 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-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 899d2ae9841..f730d285e84 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -64,6 +64,44 @@ 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("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({ @@ -476,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", ); @@ -725,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 a7bea07fb8b..9eaef8b4bd3 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 { @@ -21,23 +22,32 @@ 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; - const deps = { - findPidForPort: vi.fn(() => null), + const deps: StopModelRouterForDestroyedSandboxDeps = { + acquireOnboardLock: vi.fn(() => ({ + acquired: true, + lockFile: "/tmp/onboard.lock", + stale: false, + })), + compareAndSwapSession: vi.fn((matches, mutator) => { + return matches(session) ? (mutator(session), "updated") : "mismatch"; + }), + expectedSession: session, + 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), + releaseOnboardLock: vi.fn(), 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(), ...overrides, }; return { deps, session }; @@ -64,6 +74,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(); }); @@ -76,7 +87,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 () => { @@ -85,41 +96,46 @@ 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 another registered routed sandbox remains", async () => { - const { deps } = createDeps({ - listSandboxes: vi.fn(() => ({ - sandboxes: [ - { + it("keeps the router while a sandbox in another gateway state root uses the same port", async () => { + const { deps, session } = createDeps({ + 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); expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.compareAndSwapSession).toHaveBeenCalledOnce(); + expect(session.sandboxName).toBeNull(); }); - 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({ - 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 +146,118 @@ 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({ + expectedSession: unrelatedSession, + 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.compareAndSwapSession).not.toHaveBeenCalled(); + expect(unrelatedSession.routerPid).toBe(6262); + expect(unrelatedSession.routerCredentialHash).toBe("beta-hash"); + }); + + 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) => { + return matches(reusedNameSession) + ? (mutator(reusedNameSession), "updated") + : "mismatch"; + }, + ); + const { deps } = createDeps({ + compareAndSwapSession, + expectedSession: reusedNameSession, + loadSession: vi.fn(() => reusedNameSession), + ownsPort: vi.fn(() => false), + inspectProcessForPort: vi.fn(() => ({ status: "absent" as const })), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(compareAndSwapSession).toHaveBeenCalledOnce(); + expect(deps.stopProcess).not.toHaveBeenCalled(); + expect(reusedNameSession.sandboxName).toBeNull(); + 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({ 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({ + 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")); + }); + 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,20 +267,53 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(session.routerCredentialHash).toBeNull(); }); + 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 })), + isHealthy: vi.fn(async () => true), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + expect(deps.stopProcess).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")); + }); + + 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 })), + isHealthy: vi.fn(async () => true), + }); + + await stopModelRouterForDestroyedSandbox(routedSandbox, deps); + + 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")); + }); + 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; const { deps } = createDeps({ + expectedSession: session, loadSession: vi.fn(() => session), ownsPort: vi.fn(() => false), - updateSession: vi.fn((mutator: (current: Session) => Session | void) => { - mutator(session); - return session; + compareAndSwapSession: vi.fn((matches, mutator) => { + return matches(session) ? (mutator(session), "updated") : "mismatch"; }), }); @@ -177,49 +324,28 @@ describe("stopModelRouterForDestroyedSandbox", () => { expect(session.routerCredentialHash).toBeNull(); }); - it("does not clear session identity for a different routed sandbox", async () => { + it("clears the destroyed sandbox association when no router identity remains", async () => { const session = { - sessionId: "session-beta", - sandboxName: "beta", - endpointUrl: "http://host.openshell.internal:4200/v1", - routerPid: 5252, - routerCredentialHash: "beta-hash", + sessionId: "session-alpha", + sandboxName: "alpha", + endpointUrl: routedSandbox.endpointUrl, + routerPid: null, + routerCredentialHash: null, } as Session; const { deps } = createDeps({ + expectedSession: session, loadSession: vi.fn(() => session), ownsPort: vi.fn(() => false), - findPidForPort: vi.fn(() => 5151), - updateSession: vi.fn((mutator: (current: Session) => Session | void) => { - mutator(session); - return session; + compareAndSwapSession: vi.fn((matches, mutator) => { + return matches(session) ? (mutator(session), "updated") : "mismatch"; }), }); 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", - routerPid: null, - }) as Session, - ), - ownsPort: vi.fn(() => false), - }); - - await stopModelRouterForDestroyedSandbox(routedSandbox, deps); - expect(deps.stopProcess).not.toHaveBeenCalled(); - expect(deps.updateSession).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 () => { @@ -229,11 +355,31 @@ 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("kill 4242")); - expect(deps.updateSession).not.toHaveBeenCalled(); + expect(deps.warn).toHaveBeenCalledWith(expect.stringContaining("Inspect PID 4242")); + expect(deps.warn).not.toHaveBeenCalledWith(expect.stringContaining("kill 4242")); + 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 () => { + 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..496edb895b1 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -1,14 +1,24 @@ // 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 { isRoutedInferenceProvider } from "../../onboard/model-router"; +import { withModelRouterPortLifecycleLock } from "../../inference/gateway-route-mutation-lock"; +import { DEFAULT_MODEL_ROUTER_PORT, isRoutedInferenceProvider } from "../../onboard/model-router"; import { doesModelRouterProcessOwnPort, - findModelRouterPidForPort, + inspectModelRouterProcessForPort, + isRouterHealthy, stopModelRouterProcess, } from "../../onboard/model-router-process"; -import type { Session } from "../../state/onboard-session"; +import { listHostGatewayRegistryEntries } from "../../state/gateway-registry"; +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"; @@ -49,22 +59,36 @@ 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; + expectedSession: Session | null; loadSession: () => Session | null; - updateSession: (mutator: (session: Session) => Session | void) => Session; - findPidForPort?: typeof findModelRouterPidForPort; + releaseOnboardLock: typeof releaseOnboardLock; + 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; }; +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); @@ -82,81 +106,158 @@ 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 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 + * 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 (!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 false; + const port = resolveDestroyedSandboxRouterPort(sandbox.endpointUrl); + const withPortLock = deps.withModelRouterPortLifecycleLock ?? withModelRouterPortLifecycleLock; + await withPortLock(port, async () => { 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) - }`, - ); + const sessionLock = deps.acquireOnboardLock("nemoclaw destroy Model Router teardown"); + if (!sessionLock.acquired) { warn( - `Stop it manually (kill ${pid}) before the next Model Router onboarding, or onboarding fails with "Port ${port} already has a healthy router endpoint".`, + "Another onboarding run owns the session lock. Keeping the Model Router process and recovery identity.", ); 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; + let destroyedSessionId: string | null = null; + try { + const session = deps.loadSession(); + if (!sessionMatchesDestroySnapshot(session, deps.expectedSession)) { + warn( + "The onboarding session changed during destroy. Keeping the Model Router process and replacement session unchanged.", + ); + return; } - current.routerPid = null; - current.routerCredentialHash = null; - return current; - }); - } + const sessionMatchesSandbox = + session?.sandboxName === sandbox.name && + resolveDestroyedSandboxRouterPort(session.endpointUrl) === port; + destroyedSessionId = session?.sandboxName === sandbox.name ? session.sessionId : null; + + 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 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; + } + } + + 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 6ee874182cb..3b1ae1597ed 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 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. - await withGatewayRouteMutationLock(cleanupGatewayName, () => + // 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. + routedSessionCleanupHandled = await withGatewayRouteMutationLock(cleanupGatewayName, () => stopModelRouterForDestroyedSandbox(sandbox, { + acquireOnboardLock: onboardSession.acquireOnboardLock, + compareAndSwapSession: onboardSession.compareAndSwapSession, + expectedSession: destroySession, loadSession: onboardSession.loadSession, - updateSession: onboardSession.updateSession, + releaseOnboardLock: onboardSession.releaseOnboardLock, warn: defaultDestroyWarn, }), ); @@ -705,16 +709,31 @@ 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.`, ); } } - 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/inference/gateway-route-mutation-lock.test.ts b/src/lib/inference/gateway-route-mutation-lock.test.ts index 4185e35cc7c..f93280c0676 100644 --- a/src/lib/inference/gateway-route-mutation-lock.test.ts +++ b/src/lib/inference/gateway-route-mutation-lock.test.ts @@ -4,7 +4,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { withGatewayRouteMutationLock } from "./gateway-route-mutation-lock"; describe("gateway route mutation lock", () => { @@ -81,4 +81,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..697fc5dd734 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 resolveCurrentUserModelRouterLockStateDir(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 current-user lifecycle changes for one Model Router port across gateways. */ +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 ?? resolveCurrentUserModelRouterLockStateDir(); + return withMcpLifecycleLock(`${MODEL_ROUTER_PORT_LOCK_PREFIX}${String(port)}`, operation, { + ...options, + stateDir, + }); +} 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..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,6 +167,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 +1463,53 @@ 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 withRouterPortLifecycleLock = + deps.withModelRouterPortLifecycleLock ?? withModelRouterPortLifecycleLock; + const getRouterPort = deps.getModelRouterPort ?? resolveModelRouterPort; + const routedRepair = await deps.withGatewayRouteMutationLock(gatewayName, () => + withRouterPortLifecycleLock(getRouterPort(), 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( 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..ac6249fed9d 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,12 @@ function getRoutedProfile(): BlueprintInferenceProfile { return bp; } +export const DEFAULT_MODEL_ROUTER_PORT = 4000; + +export function resolveModelRouterPort(): number { + return getRoutedProfile().router?.port || DEFAULT_MODEL_ROUTER_PORT; +} + export function isRoutedInferenceProvider(provider: string | null | undefined): boolean { if (!provider) return false; if (provider === "nvidia-router") return true; @@ -615,7 +621,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 +659,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..973c59d2261 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(); }); @@ -363,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, @@ -469,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/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/src/lib/state/onboard-session-cross-process-lock.test.ts b/src/lib/state/onboard-session-cross-process-lock.test.ts index 52c36e169d4..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"); @@ -70,4 +92,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 = "alpha"; + replacement.endpointUrl = "http://host.openshell.internal:4000/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: "alpha", + endpointUrl: "http://host.openshell.internal:4000/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..ce008bc03ea 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", @@ -1489,6 +1487,36 @@ export function updateSession(mutator: (session: Session) => Session | void): Se return saveSession(next); } +export type CompareAndSwapSessionResult = "updated" | "busy" | "mismatch"; + +/** + * Mutate the current session while this process owns the onboarding lock. + * + * 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 managesOnboardLock = heldLockFd === null; + if (managesOnboardLock) { + 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 { + if (managesOnboardLock) 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 fb09a0647e0..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"]; @@ -16,6 +17,7 @@ const destroyModulePath = "./destroy.js"; export type DestroyHarness = { cleanupGatewaySpy: MockInstance; captureOpenshellSpy: MockInstance; + compareAndSwapSessionSpy: MockInstance; destroySandbox: DestroySandbox; dockerCaptureSpy: MockInstance; dockerRunSpy: MockInstance; @@ -36,6 +38,7 @@ export type DestroyHarness = { restoreMcpBridgesAfterDestroyAbortSpy: MockInstance; runOpenshellSpy: MockInstance; selectGatewaySpy: MockInstance; + sessionState: Session; setSandboxPresent: (present: boolean) => void; shieldsDownSpy: MockInstance; stopAllSpy: MockInstance; @@ -45,6 +48,7 @@ export type DestroyHarness = { updateSessionSpy: MockInstance; warnSpy: MockInstance; withGatewayRouteMutationLockSpy: MockInstance; + withModelRouterPortLifecycleLockSpy: MockInstance; }; type DestroyHarnessOptions = { @@ -73,6 +77,7 @@ type DestroyHarnessOptions = { promptResponses?: string[]; provider?: string; registeredSandboxCount?: number; + replaceSessionAfterRegistryRemoval?: boolean; removeSandboxResult?: boolean; restoreMcpError?: string; sandboxPresent?: boolean; @@ -147,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); @@ -160,9 +174,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"); @@ -214,11 +230,21 @@ 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(destroyPreflight, "stopModelRouterForDestroyedSandbox") - .mockResolvedValue(undefined); + const stopModelRouterForDestroyedSandboxSpy = vi.spyOn( + destroyPreflight, + "stopModelRouterForDestroyedSandbox", + ); const retirePortableLifecycleReceiptSpy = vi .spyOn(destroyExecution, "retirePortableLifecycleAuthority") .mockImplementation(() => undefined); @@ -232,10 +258,41 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr .mockImplementation(async (_gatewayName: unknown, operation: unknown) => (operation as () => Promise)(), ); - vi.spyOn(onboardSession, "loadSession").mockReturnValue({ - sandboxName: "alpha", - ...(options.sessionRouterPid ? { routerPid: options.sessionRouterPid } : {}), + 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").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) => { + expect(typeof matches).toBe("function"); + expect(typeof mutator).toBe("function"); + if (sessionLockBusy) return "busy"; + if (!(matches as (value: Session) => boolean)(sessionState)) return "mismatch"; + (mutator as (value: Session) => void)(sessionState); + return "updated"; + }); const updateSessionSpy = vi .spyOn(onboardSession, "updateSession") .mockImplementation((mutator: unknown) => { @@ -302,10 +359,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 +376,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") @@ -414,6 +467,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr return { cleanupGatewaySpy, captureOpenshellSpy, + compareAndSwapSessionSpy, dockerCaptureSpy, dockerRunSpy, destroySandbox: requireDist(destroyModulePath).destroySandbox, @@ -434,6 +488,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr restoreMcpBridgesAfterDestroyAbortSpy, runOpenshellSpy, selectGatewaySpy, + sessionState, setSandboxPresent: (present: boolean) => { sandboxPresent = present; }, @@ -445,5 +500,6 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr updateSessionSpy, warnSpy, withGatewayRouteMutationLockSpy, + withModelRouterPortLifecycleLockSpy, }; } diff --git a/test/package-contract/destroy-model-router-flow.test.ts b/test/package-contract/destroy-model-router-flow.test.ts index db3a913d503..d5277d2ad4f 100644 --- a/test/package-contract/destroy-model-router-flow.test.ts +++ b/test/package-contract/destroy-model-router-flow.test.ts @@ -90,10 +90,13 @@ 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) => { + return matches(session) + ? (mutator(session), "updated" as const) + : ("mismatch" as const); + }, + ); await expect( stopModelRouterForDestroyedSandbox( @@ -103,18 +106,30 @@ describe("destroySandbox model-router teardown (#9098)", () => { endpointUrl: session.endpointUrl, }, { - listSandboxes: () => ({ sandboxes: [], defaultSandbox: null }), + acquireOnboardLock: () => ({ + acquired: true, + lockFile: "/tmp/onboard.lock", + stale: false, + }), + listHostRegistryEntries: () => [], + compareAndSwapSession, + expectedSession: session, loadSession: () => session, - updateSession, + 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(updateSession).toHaveBeenCalledOnce(); + expect(compareAndSwapSession).toHaveBeenCalledTimes(2); expect(session).toEqual( - expect.objectContaining({ routerPid: null, routerCredentialHash: null }), + expect.objectContaining({ + sandboxName: null, + routerPid: null, + routerCredentialHash: null, + }), ); }, );