Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions src/lib/actions/sandbox/gateway-state-hints.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

import { createRequire } from "node:module";

import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from "vitest";

type GatewayStateModule = typeof import("../../../../dist/lib/actions/sandbox/gateway-state");

const requireDist = createRequire(import.meta.url);

describe("printGatewayLifecycleHint multi-instance hints", () => {
let gatewayState: GatewayStateModule;
let getSandboxSpy: MockInstance;

beforeEach(async () => {
const registry = requireDist("../../../../dist/lib/state/registry.js");
getSandboxSpy = vi.spyOn(registry, "getSandbox").mockReturnValue({
name: "instance-a",
gatewayName: "nemoclaw-8080",
gatewayPort: 8080,
});
gatewayState = requireDist("../../../../dist/lib/actions/sandbox/gateway-state.js");
});

afterEach(() => {
vi.restoreAllMocks();
});

it("surfaces a switch-gateway hint when the underlying gRPC error is `sandbox has no spec`", () => {
const lines: string[] = [];
gatewayState.printGatewayLifecycleHint(
'status: Internal, message: "sandbox has no spec", details: []',
"instance-a",
(msg: string) => lines.push(msg),
);

const combined = lines.join("\n");
expect(combined).toContain("instance-a");
expect(combined).toContain("nemoclaw");
expect(combined).toContain("openshell gateway select");
expect(getSandboxSpy).toHaveBeenCalledWith("instance-a");
});

it("uses the sandbox's per-port gateway name in the hint for a non-default `NEMOCLAW_GATEWAY_PORT`", () => {
getSandboxSpy.mockReturnValue({
name: "instance-b",
gatewayName: "nemoclaw-8081",
gatewayPort: 8081,
});
const lines: string[] = [];
gatewayState.printGatewayLifecycleHint("sandbox has no spec", "instance-b", (msg: string) =>
lines.push(msg),
);

const combined = lines.join("\n");
expect(combined).toContain("nemoclaw-8081");
expect(combined).toContain("openshell gateway select nemoclaw-8081");
});

it("does not match the new clause on unrelated gateway lifecycle output", () => {
const lines: string[] = [];
gatewayState.printGatewayLifecycleHint("No gateway configured", "instance-a", (msg: string) =>
lines.push(msg),
);

const combined = lines.join("\n");
expect(combined).not.toContain("sandbox has no spec");
expect(combined).toContain("openshell gateway start");
});
});
19 changes: 19 additions & 0 deletions src/lib/actions/sandbox/gateway-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,25 @@ export function printGatewayLifecycleHint(
): void {
const cleanOutput = stripAnsi(output);
const targetGatewayName = getSandboxTargetGatewayName(sandboxName);
// The gateway-side gRPC reply `sandbox has no spec` is returned when the
// active OpenShell gateway does not know about the sandbox — which on a
// multi-instance host typically means a sibling NemoClaw gateway (the one
// the sandbox was actually onboarded against) is the owner, and the
// current selection has to be switched back before the sandbox is
// reachable. Surface a concrete switch-gateway hint rather than letting
// the raw gRPC string be the last word.
if (/sandbox has no spec/i.test(cleanOutput)) {
writer(
` Sandbox '${sandboxName}' is registered against the ${CLI_DISPLAY_NAME} gateway '${targetGatewayName}', but the currently active OpenShell gateway does not know about it.`,
);
writer(
" On a multi-instance host, this usually means another NemoClaw gateway is the owner of this sandbox.",
);
writer(
` Select the owning gateway and retry: \`openshell gateway select ${targetGatewayName}\`, then \`${CLI_NAME} ${sandboxName} connect\`.`,
);
return;
}
if (/No gateway configured/i.test(cleanOutput)) {
writer(
` The selected ${CLI_DISPLAY_NAME} gateway is no longer configured or its metadata/runtime has been lost.`,
Expand Down
106 changes: 106 additions & 0 deletions src/lib/onboard/dashboard-port.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { describe, it } from "vitest";
import {
findAvailableDashboardPort,
findDashboardForwardOwner,
getRegistryOccupiedDashboardPorts,
preflightDashboardPortRangeAvailability,
resolveCreateSandboxDashboardPort,
} from "../../../dist/lib/onboard/dashboard-port";
Expand Down Expand Up @@ -217,6 +218,111 @@ describe("resolveCreateSandboxDashboardPort", () => {
});
});

describe("findAvailableDashboardPort multi-gateway registry occupancy", () => {
const stubBound = (...bound: number[]) => {
const set = new Set(bound);
return (port: number) => set.has(port);
};

it("treats ports persisted to sibling sandboxes in the registry as occupied even when the active gateway's forward list does not see them", () => {
const registryOccupied = new Map<string, string>([["18789", "instance-a"]]);

assert.equal(
findAvailableDashboardPort("instance-b", 18789, "", stubBound(), registryOccupied),
18790,
);
});

it("does not block the current sandbox from reusing its own registry-persisted port", () => {
const registryOccupied = new Map<string, string>([["18789", "instance-a"]]);

assert.equal(
findAvailableDashboardPort("instance-a", 18789, "", stubBound(), registryOccupied),
18789,
);
});

it("ignores registry entries with null or invalid dashboard ports", () => {
const noPorts = new Map<string, string>();

assert.equal(findAvailableDashboardPort("instance-b", 18789, "", stubBound(), noPorts), 18789);
});

it("includes registry-owned ports in the exhaustion error so the operator can see who holds them", () => {
const lines = ["SANDBOX BIND PORT PID STATUS"];
for (let p = 18789; p <= 18798; p++) {
lines.push(`forwarded${p} 127.0.0.1 ${p} ${p} running`);
}
const registryOccupied = new Map<string, string>([["18799", "instance-z"]]);

assert.throws(
() =>
findAvailableDashboardPort(
"instance-y",
18789,
lines.join("\n"),
stubBound(),
registryOccupied,
),
/18799 → instance-z/,
);
});

it("lets the active gateway's forward-list entry win when both views see the same port", () => {
const forwardList = [
"SANDBOX BIND PORT PID STATUS",
"live 127.0.0.1 18789 111 running",
].join("\n");
const registryOccupied = new Map<string, string>([["18789", "stale"]]);

assert.throws(
() => findAvailableDashboardPort("fresh", 18789, forwardList, () => true, registryOccupied),
/18789 → live/,
);
});
});

describe("getRegistryOccupiedDashboardPorts", () => {
it("returns a port→sandbox map for every sibling sandbox with a persisted dashboard port", () => {
const occupied = getRegistryOccupiedDashboardPorts("current", () => ({
sandboxes: [
{ name: "alpha", dashboardPort: 18789 },
{ name: "beta", dashboardPort: 18790 },
{ name: "current", dashboardPort: 18791 },
],
}));

assert.equal(occupied.size, 2);
assert.equal(occupied.get("18789"), "alpha");
assert.equal(occupied.get("18790"), "beta");
assert.equal(occupied.has("18791"), false);
});

it("skips sandboxes with null, undefined, or non-numeric dashboardPort values", () => {
const occupied = getRegistryOccupiedDashboardPorts("current", () => ({
sandboxes: [
{ name: "alpha", dashboardPort: null },
{ name: "beta", dashboardPort: undefined },
{ name: "gamma" },
{ name: "delta", dashboardPort: 18790 },
],
}));

assert.equal(occupied.size, 1);
assert.equal(occupied.get("18790"), "delta");
});

it("propagates registry read errors so the allocator does not silently hand out a colliding port", () => {
assert.throws(
() =>
getRegistryOccupiedDashboardPorts("current", () => {
throw new Error("registry locked");
}),
/registry locked/,
);
});
});

describe("preflightDashboardPortRangeAvailability (#3953)", () => {
const allBound = (_p: number) => true;
const noneBound = (_p: number) => false;
Expand Down
92 changes: 91 additions & 1 deletion src/lib/onboard/dashboard-port.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ import {
const { runCapture } = require("../runner");
type RunCaptureFn = typeof import("../runner").runCapture;

type SandboxRegistryEntry = {
name: string;
dashboardPort?: number | null;
};

type ListSandboxesFn = () => { sandboxes: SandboxRegistryEntry[] };

// Match the broader pattern used by onboard.ts (covers CSI, OSC, and Fe escapes)
// so colorised `openshell forward list` output parses correctly.
const ANSI_RE = /\x1B(?:\[[0-?]*[ -/]*[@-~]|\][^\x07]*(?:\x07|\x1B\\)|[@-_])/g;
Expand Down Expand Up @@ -150,13 +157,82 @@ export function findDashboardForwardOwner(
return getOccupiedPorts(forwardListOutput ?? null).get(portToStop) ?? null;
}

/**
* Merge per-gateway forward-list occupancy with cross-gateway registry
* occupancy. `openshell forward list` only reports forwards owned by the
* currently selected gateway, so a second NemoClaw gateway on a different
* `NEMOCLAW_GATEWAY_PORT` cannot see the first gateway's dashboard forwards
* and would happily re-allocate the same dashboard port to a fresh sandbox.
* The host-level bind probe also misses Docker-mediated forwards on macOS,
* which is exactly the scenario reported on multi-instance hosts.
*
* The registry persists `dashboardPort` per sandbox and lives at host scope
* (one file under `~/.nemoclaw/sandboxes.json`), so consulting it during
* allocation closes the gap between gateway namespaces without enumerating
* forwards across every NemoClaw gateway. The forward-list value still wins
* for sandboxes whose forward exists on the currently selected gateway —
* the registry view is a supplementary signal for sandboxes whose owning
* gateway is not currently selected.
*/
function mergeOccupiedPorts(
forwardOccupied: Map<string, string>,
registryOccupied: ReadonlyMap<string, string> | undefined,
): Map<string, string> {
if (!registryOccupied) return forwardOccupied;
for (const [port, sandbox] of registryOccupied.entries()) {
if (!forwardOccupied.has(port)) {
forwardOccupied.set(port, sandbox);
}
}
return forwardOccupied;
}

/**
* Build a cross-gateway occupancy map (port → owning sandbox name) from the
* persisted sandbox registry, excluding the sandbox currently being allocated
* for. The registry is the single host-scope view of dashboard ports across
* every NemoClaw gateway — `openshell forward list` only knows about the
* currently selected gateway's forwards, so a fresh onboard against a second
* `NEMOCLAW_GATEWAY_PORT` gateway cannot see the first gateway's allocations
* without this view.
*
* `listSandboxes()` already degrades to an empty registry when
* `~/.nemoclaw/sandboxes.json` is missing or unparseable, so this helper does
* not need an extra catch-all. Any remaining error (e.g. an unreadable
* registry file with the wrong filesystem permissions) propagates so the
* allocator surfaces it instead of silently handing out a colliding port.
*
* `listSandboxesFn` is an injectable seam for tests; production callers
* leave it at the default that reads `~/.nemoclaw/sandboxes.json`.
*/
export function getRegistryOccupiedDashboardPorts(
currentSandboxName: string,
listSandboxesFn?: ListSandboxesFn,
): Map<string, string> {
const occupied = new Map<string, string>();
const list = listSandboxesFn ?? (require("../state/registry").listSandboxes as ListSandboxesFn);
for (const entry of list().sandboxes) {
if (entry.name === currentSandboxName) continue;
const port = entry.dashboardPort;
if (typeof port !== "number" || !Number.isInteger(port) || port <= 0) continue;
occupied.set(String(port), entry.name);
}
return occupied;
}

export function findAvailableDashboardPort(
sandboxName: string,
preferredPort: number,
forwardListOutput: string | null,
isPortBoundCheck: (port: number) => boolean = isPortBoundOnHost,
// Default to an empty map so unit tests of this allocator do not become
// dependent on whatever sandboxes happen to live in the caller's real
// `~/.nemoclaw/sandboxes.json`. Production wrappers
// (`resolveCreateSandboxDashboardPort`, `ensureDashboardForward`) pass an
// explicit `getRegistryOccupiedDashboardPorts(sandboxName)` result.
registryOccupiedPorts: ReadonlyMap<string, string> = new Map(),
): number {
const occupied = getOccupiedPorts(forwardListOutput);
const occupied = mergeOccupiedPorts(getOccupiedPorts(forwardListOutput), registryOccupiedPorts);
const hostBoundPorts: number[] = [];
// Try the preferred port first (it may be outside the dashboard range when
// a caller passes --control-ui-port), then the rest of the range. Each port
Expand Down Expand Up @@ -204,6 +280,11 @@ export interface CreateSandboxDashboardPortInput {
defaultPort?: number;
findAvailablePort?: typeof findAvailableDashboardPort;
warn?: (message: string) => void;
// Cross-gateway occupancy view derived from the sandbox registry. Lets the
// allocator avoid handing out a dashboard port that already belongs to a
// sandbox on a different `NEMOCLAW_GATEWAY_PORT`, which the per-gateway
// forward-list view cannot see.
registryOccupiedPorts?: ReadonlyMap<string, string>;
}

export interface CreateSandboxDashboardPortResult {
Expand Down Expand Up @@ -250,10 +331,19 @@ export function resolveCreateSandboxDashboardPort(
input.agentForwardPort ??
input.defaultPort ??
DASHBOARD_PORT;
// When a caller does not supply an explicit cross-gateway view, read the
// persisted registry here so the allocator never silently hands out a
// dashboard port that already belongs to a sibling sandbox on a different
// NemoClaw gateway. The allocator itself defaults to an empty map to keep
// its unit tests independent of the caller's real `~/.nemoclaw/` state.
const registryOccupiedPorts =
input.registryOccupiedPorts ?? getRegistryOccupiedDashboardPorts(input.sandboxName);
const effectivePort = (input.findAvailablePort ?? findAvailableDashboardPort)(
input.sandboxName,
preferredPort,
input.forwardListOutput,
undefined,
registryOccupiedPorts,
);
if (effectivePort !== preferredPort) {
input.warn?.(` ! Port ${preferredPort} is taken. Using port ${effectivePort} instead.`);
Expand Down
9 changes: 8 additions & 1 deletion src/lib/onboard/dashboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
import {
findAvailableDashboardPort,
getOccupiedPorts,
getRegistryOccupiedDashboardPorts,
isLiveForwardStatus,
} from "./dashboard-port";
import { bestEffortForwardStop } from "./forward-cleanup";
Expand Down Expand Up @@ -249,7 +250,13 @@ export function createOnboardDashboardHelpers(deps: OnboardDashboardDeps): Onboa
}
let actualPort: number;
try {
actualPort = findAvailableDashboardPort(sandboxName, preferredPort, existingForwards);
actualPort = findAvailableDashboardPort(
sandboxName,
preferredPort,
existingForwards,
undefined,
getRegistryOccupiedDashboardPorts(sandboxName),
);
} catch (err) {
if (!rollbackSandboxOnFailure) throw err;
rollbackSandboxAndExit(sandboxName, err);
Expand Down
Loading