Skip to content
Closed
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
43 changes: 43 additions & 0 deletions src/lib/tunnel/services-sandbox.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,49 @@ describe("stopAll with sandbox channels", () => {
logSpy.mockRestore();
});

it("releases managed host forwards for the selected sandbox on deprecated full stop", () => {
const logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
spawnSyncSpy
.mockReturnValueOnce({ status: 1, stdout: "" })
.mockReturnValueOnce({ status: 1 })
.mockReturnValueOnce({
status: 0,
stdout: [
"SANDBOX BIND PORT PID STATUS",
"test-sb 127.0.0.1 8642 1234 running",
"other-sb 127.0.0.1 18789 1235 running",
"test-sb 127.0.0.1 18790 1236 active",
].join("\n"),
})
.mockReturnValue({ status: 0 });

stopAll({ pidDir, sandboxName: "test-sb", releaseGatewayPort: true });

expect(spawnSyncSpy).toHaveBeenCalledWith(
"/usr/local/bin/openshell",
["forward", "list"],
expect.objectContaining({ timeout: 15_000 }),
);
expect(spawnSyncSpy).toHaveBeenCalledWith(
"/usr/local/bin/openshell",
["forward", "stop", "8642", "test-sb"],
expect.objectContaining({ timeout: 30_000 }),
);
expect(spawnSyncSpy).toHaveBeenCalledWith(
"/usr/local/bin/openshell",
["forward", "stop", "18790", "test-sb"],
expect.objectContaining({ timeout: 30_000 }),
);
expect(spawnSyncSpy).not.toHaveBeenCalledWith(
"/usr/local/bin/openshell",
["forward", "stop", "18789", "test-sb"],
expect.any(Object),
);
const output = logSpy.mock.calls.map((c) => c[0]).join("\n");
expect(output).toContain("Released 2 managed host forward(s) for sandbox test-sb");
logSpy.mockRestore();
});

it("warns when no sandbox name is available", () => {
const logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
const savedNemoclaw = process.env.NEMOCLAW_SANDBOX;
Expand Down
43 changes: 42 additions & 1 deletion src/lib/tunnel/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@ import {
import { basename, join } from "node:path";
import { dockerSpawnSync } from "../adapters/docker";
import { resolveOpenshell } from "../adapters/openshell/resolve";
import {
OPENSHELL_OPERATION_TIMEOUT_MS,
OPENSHELL_PROBE_TIMEOUT_MS,
} from "../adapters/openshell/timeouts";
import { renderBox } from "../cli/banner";
import { AGENT_PRODUCT_NAME, CLI_DISPLAY_NAME, CLI_NAME } from "../cli/branding";
import { isRecord } from "../core/json-types";
import { DASHBOARD_PORT } from "../core/ports";
import { getOccupiedPorts } from "../onboard/dashboard-port";
import { buildSubprocessEnv } from "../subprocess-env";
import { registerTunnelOrigin } from "./allowed-origins";
import * as gatewayStop from "./gateway-stop";
Expand Down Expand Up @@ -459,7 +464,7 @@ export function showStatus(opts: ServiceOptions = {}): void {
* post-stop process scan is empty.
*/
export function stopSandboxChannels(sandboxName: string): void {
info(`Stopping in-sandbox OpenClaw gateway (sandbox: ${sandboxName})...`);
info(`Stopping in-sandbox ${AGENT_PRODUCT_NAME} gateway (sandbox: ${sandboxName})...`);

const privilegedResult = stopSandboxChannelsViaKubectl(sandboxName);
if (reportStopResult(privilegedResult)) return;
Expand Down Expand Up @@ -587,6 +592,39 @@ function reportStopResult(result: StopAttemptResult | null): boolean {
return true;
}

function stopSandboxForwards(sandboxName: string): void {
const openshell = resolveOpenshell();
if (!openshell) {
warn("openshell not found — cannot release managed host forwards.");
return;
}

const listResult = spawnSync(openshell, ["forward", "list"], {
encoding: "utf-8",
stdio: ["ignore", "pipe", "pipe"],
timeout: OPENSHELL_PROBE_TIMEOUT_MS,
});
if (listResult.status !== 0) {
warn("Could not list OpenShell forwards — managed host forwards may still be running.");
return;
}

const output = listResult.stdout ?? "";
const ports = [...getOccupiedPorts(output).entries()]
.filter(([, owner]) => owner === sandboxName)
.map(([port]) => port);
for (const port of ports) {
spawnSync(openshell, ["forward", "stop", port, sandboxName], {
encoding: "utf-8",
stdio: "ignore",
timeout: OPENSHELL_OPERATION_TIMEOUT_MS,
});
}
if (ports.length > 0) {
info(`Released ${String(ports.length)} managed host forward(s) for sandbox ${sandboxName}.`);
}
}

Comment on lines +595 to +627

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Success count doesn't verify actual forward stop outcome.

ports.length reflects how many ports were targeted, not how many stops actually succeeded — the spawnSync result for each forward stop call is discarded. If a stop fails, the log still reports Released N managed host forward(s), which is misleading precisely for the scenario this PR is fixing (confirming that a forward like port 8642 was actually released).

🐛 Proposed fix to track actual successes
-  for (const port of ports) {
-    spawnSync(openshell, ["forward", "stop", port, sandboxName], {
-      encoding: "utf-8",
-      stdio: "ignore",
-      timeout: OPENSHELL_OPERATION_TIMEOUT_MS,
-    });
-  }
-  if (ports.length > 0) {
-    info(`Released ${String(ports.length)} managed host forward(s) for sandbox ${sandboxName}.`);
-  }
+  let released = 0;
+  for (const port of ports) {
+    const stopResult = spawnSync(openshell, ["forward", "stop", port, sandboxName], {
+      encoding: "utf-8",
+      stdio: "ignore",
+      timeout: OPENSHELL_OPERATION_TIMEOUT_MS,
+    });
+    if (stopResult.status === 0) released += 1;
+  }
+  if (released > 0) {
+    info(`Released ${String(released)} managed host forward(s) for sandbox ${sandboxName}.`);
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function stopSandboxForwards(sandboxName: string): void {
const openshell = resolveOpenshell();
if (!openshell) {
warn("openshell not found — cannot release managed host forwards.");
return;
}
const listResult = spawnSync(openshell, ["forward", "list"], {
encoding: "utf-8",
stdio: ["ignore", "pipe", "pipe"],
timeout: OPENSHELL_PROBE_TIMEOUT_MS,
});
if (listResult.status !== 0) {
warn("Could not list OpenShell forwards — managed host forwards may still be running.");
return;
}
const output = listResult.stdout ?? "";
const ports = [...getOccupiedPorts(output).entries()]
.filter(([, owner]) => owner === sandboxName)
.map(([port]) => port);
for (const port of ports) {
spawnSync(openshell, ["forward", "stop", port, sandboxName], {
encoding: "utf-8",
stdio: "ignore",
timeout: OPENSHELL_OPERATION_TIMEOUT_MS,
});
}
if (ports.length > 0) {
info(`Released ${String(ports.length)} managed host forward(s) for sandbox ${sandboxName}.`);
}
}
function stopSandboxForwards(sandboxName: string): void {
const openshell = resolveOpenshell();
if (!openshell) {
warn("openshell not found — cannot release managed host forwards.");
return;
}
const listResult = spawnSync(openshell, ["forward", "list"], {
encoding: "utf-8",
stdio: ["ignore", "pipe", "pipe"],
timeout: OPENSHELL_PROBE_TIMEOUT_MS,
});
if (listResult.status !== 0) {
warn("Could not list OpenShell forwards — managed host forwards may still be running.");
return;
}
const output = listResult.stdout ?? "";
const ports = [...getOccupiedPorts(output).entries()]
.filter(([, owner]) => owner === sandboxName)
.map(([port]) => port);
let released = 0;
for (const port of ports) {
const stopResult = spawnSync(openshell, ["forward", "stop", port, sandboxName], {
encoding: "utf-8",
stdio: "ignore",
timeout: OPENSHELL_OPERATION_TIMEOUT_MS,
});
if (stopResult.status === 0) released += 1;
}
if (released > 0) {
info(`Released ${String(released)} managed host forward(s) for sandbox ${sandboxName}.`);
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/tunnel/services.ts` around lines 595 - 627, The success log in
stopSandboxForwards currently counts targeted ports rather than actual
successful stops, so it can report releases even when a forward stop fails.
Update stopSandboxForwards to inspect each spawnSync(openshell, ["forward",
"stop", ...]) result, count only successful stops, and log that count instead of
ports.length. Keep the existing behavior around resolveOpenshell,
getOccupiedPorts, and the final info message, but base the message on verified
stop outcomes.

export function stopAll(opts: ServiceOptions = {}): void {
// Stop the in-sandbox OpenClaw gateway (and its messaging channels).
const rawSandboxName =
Expand Down Expand Up @@ -625,6 +663,9 @@ export function stopAll(opts: ServiceOptions = {}): void {
stopService(pidDir, "cloudflared");

if (opts.releaseGatewayPort) {
if (sandboxName) {
stopSandboxForwards(sandboxName);
}
gatewayStop.releaseGatewayPortForStop(sandboxName, { info, warn });
}

Expand Down