Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
49 changes: 49 additions & 0 deletions src/lib/onboard/agent-fixed-forward.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// 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("runs no `forward stop` when the capture runner reports a failed forward list", () => {
const deps = makeDeps(() => null);

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

expect(started).toBe(true);
expect(deps.runOpenshell).not.toHaveBeenCalled();
});

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

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

expect(started).toBe(true);
expect(deps.runOpenshell).toHaveBeenCalledWith(["forward", "stop", "18789", "my-sandbox"], {
ignoreError: true,
suppressOutput: true,
});
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
});
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
10 changes: 10 additions & 0 deletions src/lib/onboard/forward-cleanup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,16 @@ describe("bestEffortForwardStopForSandbox", () => {
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(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
18 changes: 11 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,23 @@ 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. Neither may reach getOccupiedPorts, which parses
// an empty string into an empty map, so the "no-entry" branch below would
// still run the stop — exactly the collateral-damage case this helper
// exists to avoid. Runners must not pass ignoreError: true, which collapses
// a failure into an indistinguishable empty output.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
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