Skip to content
Open
Show file tree
Hide file tree
Changes from 5 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
4 changes: 3 additions & 1 deletion src/lib/actions/sandbox/messaging-host-forward-lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ import { parseForwardList } from "../../state/sandbox-session";
import { classifyForwardHealthWithReachability, isLocalForwardReachable } from "./forward-health";

function captureOpenShellOutput(args: string[], opts: Record<string, unknown> = {}): string | null {
const result = captureOpenshell(args, opts as Parameters<typeof captureOpenshell>[1]);
const result = captureOpenshell(args, { ...opts, ignoreError: true } as Parameters<
typeof captureOpenshell
>[1]);
return result.status === 0 ? result.output : null;
}

Expand Down
57 changes: 57 additions & 0 deletions src/lib/onboard/agent-fixed-forward.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

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

import { ensureAgentFixedForward } from "./agent-fixed-forward";

vi.mock("./forward-start", () => ({
buildDetachedForwardStartSpawn: vi.fn(() => vi.fn()),
buildForwardStartProgressLogger: vi.fn(() => vi.fn()),
runDetachedForwardStartWithRetries: vi.fn(() => ({ ok: true, diagnostic: "" })),
}));

function makeDeps(runCaptureOpenshell: () => string | null) {
return {
runOpenshell: vi.fn(() => ({ status: 0 })),
runCaptureOpenshell: vi.fn(runCaptureOpenshell),
openshellArgv: (args: string[]) => ["openshell", ...args],
cliName: () => "nemoclaw",
sleep: vi.fn(),
};
}

describe("ensureAgentFixedForward", () => {
beforeEach(() => {
vi.clearAllMocks();
});

it("skips the stop when the capture runner reports a failed `forward list` (#8522)", () => {
const deps = makeDeps(() => null);

const started = ensureAgentFixedForward(deps, "my-sandbox", 18789, "messaging webhook");

expect(started).toBe(true);
expect(deps.runCaptureOpenshell).toHaveBeenCalledWith(
["forward", "list"],
expect.objectContaining({ timeout: expect.any(Number) }),
);
expect(deps.runOpenshell).not.toHaveBeenCalled();
});

it("runs a sandbox-scoped `forward stop` when the `forward list` is genuinely empty (#8522)", () => {
const deps = makeDeps(() => "SANDBOX BIND PORT PID STATUS");

const started = ensureAgentFixedForward(deps, "my-sandbox", 18789, "messaging webhook");

expect(started).toBe(true);
expect(deps.runCaptureOpenshell).toHaveBeenCalledWith(
["forward", "list"],
expect.objectContaining({ timeout: expect.any(Number) }),
);
expect(deps.runOpenshell).toHaveBeenCalledWith(["forward", "stop", "18789", "my-sandbox"], {
ignoreError: true,
suppressOutput: true,
});
});
});
2 changes: 1 addition & 1 deletion src/lib/onboard/agent-fixed-forward.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export function ensureAgentFixedForward(
const stopForwardForSandbox = (portToStop: string | number) =>
bestEffortForwardStopForSandbox(
deps.runOpenshell,
(args, opts) => (deps.runCaptureOpenshell(args, opts) ?? "") as string,
(args, opts) => deps.runCaptureOpenshell(args, opts),
portToStop,
sandboxName,
);
Expand Down
8 changes: 1 addition & 7 deletions src/lib/onboard/dashboard-forward-control.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,7 @@ export function createSandboxForwardStopper(deps: {
if (stoppedPorts.has(portKey)) return null;
const result = bestEffortForwardStopForSandbox(
deps.runOpenshell,
(args, opts) => {
const output = deps.runCaptureOpenshell(args, opts);
if (output === null) {
throw new Error("Failed to list OpenShell forwards before stopping dashboard forward");
}
return output;
},
(args, opts) => deps.runCaptureOpenshell(args, opts),
port,
deps.sandboxName,
);
Expand Down
12 changes: 12 additions & 0 deletions src/lib/onboard/forward-cleanup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,25 @@ describe("bestEffortForwardStopForSandbox", () => {

const outcome = bestEffortForwardStopForSandbox(run, fetch, 18789, "my-sandbox");

expect(fetch).toHaveBeenCalledWith(["forward", "list"], expect.anything());
expect(outcome).toBe("list-failed");
// Without ownership data, a port-only stop could kill another
// sandbox's forward — better to leave the port alone and let the
// helper's retry / next poll observe the real state.
expect(run).not.toHaveBeenCalled();
});

it("skips the stop entirely when `forward list` reports failure as null (owner unknown)", () => {
const run = vi.fn();
const fetch = vi.fn().mockReturnValue(null);

const outcome = bestEffortForwardStopForSandbox(run, fetch, 18789, "my-sandbox");

expect(fetch).toHaveBeenCalledWith(["forward", "list"], expect.anything());
expect(outcome).toBe("list-failed");
expect(run).not.toHaveBeenCalled();
});

it("ignores forwards with non-live status when deciding ownership", () => {
// `getOccupiedPorts` filters by `isLiveForwardStatus`, so a "stopped"
// entry on the requested port should be treated as no-entry (not as a
Expand Down
19 changes: 12 additions & 7 deletions src/lib/onboard/forward-cleanup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export type ForwardStopRunner = (
export type ForwardListRunner = (
args: string[],
opts: { ignoreError?: boolean; timeout?: number },
) => string;
) => string | null;

/**
* `openshell forward stop <port>` — port-scoped, kills whatever forward is
Expand Down Expand Up @@ -64,19 +64,24 @@ export function bestEffortForwardStopForSandbox(
port: string | number,
sandboxName: string,
): "stopped" | "owned-other" | "no-entry" | "list-failed" {
// Let runCaptureOpenshell throw on failure/timeout so the catch branch
// returns "list-failed". With ignoreError: true the runner would swallow
// the error and return "", which getOccupiedPorts parses as an empty map
// and the "no-entry" branch below would still run the stop — exactly the
// collateral-damage case this helper exists to avoid.
let listOutput = "";
// A runner reports failure either by throwing or by returning null; both
// mean "list-failed" here. Do not pass either result to getOccupiedPorts,
// which parses an empty string into an empty map, so the "no-entry" branch
// below would still run the stop against this sandbox's own live forward
// without any ownership evidence. A runner that ignores the command failure
// itself must convert it to null, never to an empty string, which is
// indistinguishable from a genuinely empty forward list.
let listOutput: string | null = null;
try {
listOutput = runCaptureOpenshell(["forward", "list"], {
timeout: OPENSHELL_PROBE_TIMEOUT_MS,
});
} catch {
return "list-failed";
}
if (listOutput === null) {
return "list-failed";
}
const owner = getOccupiedPorts(listOutput).get(String(port)) ?? null;
if (owner && owner !== sandboxName) {
return "owned-other";
Expand Down
Loading