fix(sandbox): treat an already-active port forward as recovery success

src/lib/actions/sandbox/forward-recovery.ts

@@ -196,8 +196,12 @@ export function ensureSandboxPortForwardForPort(
   // before the old SSH listener is guaranteed to release its host port. A
   // blind stop -> start can therefore collide with the just-stopped process.
   // Preserve authoritative owner metadata while waiting: accept a target-
-  // owned forward that recovered on its own, reject another sandbox, and only
-  // start after an otherwise-unowned local listener has actually quiesced.
+  // owned forward that recovered on its own, reject another sandbox, and
+  // prefer starting only after an otherwise-unowned local listener has
+  // quiesced. If an authoritative list remains ownerless while the listener
+  // stays reachable, `forward start` is still the reconciliation operation:
+  // its result is accepted below only after the list reports the exact target
+  // owner. An unavailable list and forced bind replacement remain fail-closed.
   // NemoClaw must compensate while the supported OpenShell 0.0.85
   // contract remains supported; test/process-recovery.test.ts locks both the
   // delayed-release and fail-closed cases. Remove this wait only after every
@@ -209,7 +213,7 @@ export function ensureSandboxPortForwardForPort(
       health: forwardHealth,
       portReleased: false,
     };
-    const stopSettled = waitUntil(
+    waitUntil(
       () => {
         stopState.health = isSandboxPortForwardHealthy(sandboxName, port, expectedBind);
         stopState.portReleased = !isLocalForwardReachable(port);
@@ -227,7 +231,10 @@ export function ensureSandboxPortForwardForPort(
       },
     );
     if (stopState.health === true && !forceRestart) return acceptSuccessfulForward();
-    if (stopState.health === "occupied" || !stopSettled || !stopState.portReleased) return false;
+    if (stopState.health === "occupied") return false;
+    if (!stopState.portReleased && (forceRestart || stopState.health === null)) {
+      return false;
+    }
   }
 
   if (!beforeStart()) return false;
@@ -242,7 +249,13 @@ export function ensureSandboxPortForwardForPort(
       stdio: "ignore",
     },
   );
-  if (startResult.status !== 0) return false;
+  // OpenShell 0.0.85 returns an error when start preflight finds a validated
+  // live forward for the requested port. Recovery cannot change that upstream
+  // CLI contract, so a reachable listener settles against the authoritative
+  // forward list below; an absent listener still fails fast. Remove this
+  // tolerance once every supported OpenShell release makes `forward start`
+  // idempotent for an already-tracked live forward.
+  if (startResult.status !== 0 && !isLocalForwardReachable(port)) return false;
 
   // `forward start --background` can return before its authoritative list
   // entry becomes visible. Poll for the exact live sandbox+port owner instead

test/process-recovery-forward-failure.test.ts

@@ -7,6 +7,9 @@ import os from "node:os";
 import path from "node:path";
 
 import { afterEach, describe, expect, it, vi } from "vitest";
+import * as forwardHealth from "../src/lib/actions/sandbox/forward-health.js";
+import { ensureSandboxPortForwardForPort } from "../src/lib/actions/sandbox/forward-recovery.js";
+import * as openshellRuntime from "../src/lib/adapters/openshell/runtime.js";
 
 const requireSource = createRequire(import.meta.url);
 const { checkAndRecoverSandboxProcesses: checkAndRecoverSandboxProcessesImpl } = requireSource(
@@ -190,3 +193,190 @@ beta  127.0.0.1  18789  12345  dead`,
     );
   });
 });
+
+describe("ensureSandboxPortForwardForPort already-forwarded idempotency (#7085)", () => {
+  it("reconciles a reachable ownerless listener with a nonzero recovery wait", () => {
+    vi.stubEnv("NEMOCLAW_FORWARD_RECOVERY_WAIT_MS", "25");
+    let started = false;
+
+    // The pre-start list remains ownerless for the full stop-settle window,
+    // while OpenShell's idempotent start refreshes the authoritative owner row.
+    vi.spyOn(forwardHealth, "isLocalForwardReachable").mockReturnValue(true);
+    vi.spyOn(openshellRuntime, "captureOpenshell").mockImplementation(() => ({
+      status: 0,
+      output: started
+        ? `SANDBOX  BIND  PORT  PID  STATUS
+beta  127.0.0.1  18791  12345  running`
+        : "SANDBOX  BIND  PORT  PID  STATUS",
+    }));
+    const runOpenshell = vi
+      .spyOn(openshellRuntime, "runOpenshell")
+      .mockImplementation((rawArgs: unknown) => {
+        const args = Array.isArray(rawArgs) ? rawArgs.map(String) : [];
+        const isForwardStart = args[0] === "forward" && args[1] === "start";
+        started ||= isForwardStart;
+        return { status: Number(isForwardStart) } as never;
+      });
+
+    expect(
+      withFakeOpenshellBinary(() =>
+        ensureSandboxPortForwardForPort("beta", 18791, { expectedBind: "127.0.0.1" }),
+      ),
+    ).toBe(true);
+    expect(runOpenshell).toHaveBeenCalledWith(
+      ["forward", "start", "--background", "18791", "beta"],
+      expect.objectContaining({ ignoreError: true }),
+    );
+  });
+
+  it("accepts an already-active target-owned forward when `forward start` exits non-zero", () => {
+    // Forward visibility is fixed by mocks, so the production settle window is unnecessary.
+    vi.stubEnv("NEMOCLAW_FORWARD_RECOVERY_WAIT_MS", "0");
+
+    // The port is listening throughout; OpenShell's forward list only shows the
+    // live owner after the (idempotent) start, modelling the stale-list drift
+    // that makes recovery attempt a stop -> start on an already-active forward.
+    let started = false;
+    vi.spyOn(forwardHealth, "isLocalForwardReachable").mockReturnValue(true);
+    vi.spyOn(openshellRuntime, "captureOpenshell").mockImplementation(() => ({
+      status: 0,
+      output: started
+        ? `SANDBOX  BIND  PORT  PID  STATUS
+beta  127.0.0.1  18791  12345  running`
+        : `SANDBOX  BIND  PORT  PID  STATUS`,
+    }));
+    const runOpenshell = vi
+      .spyOn(openshellRuntime, "runOpenshell")
+      .mockImplementation((rawArgs: unknown) => {
+        const args = Array.isArray(rawArgs) ? rawArgs.map(String) : [];
+        const isForwardStart = args[0] === "forward" && args[1] === "start";
+        started ||= isForwardStart;
+        // OpenShell exits non-zero because the port is already forwarded.
+        return { status: Number(isForwardStart) } as never;
+      });
+
+    expect(
+      withFakeOpenshellBinary(() =>
+        ensureSandboxPortForwardForPort("beta", 18791, { expectedBind: "127.0.0.1" }),
+      ),
+    ).toBe(true);
+    expect(runOpenshell).toHaveBeenCalledWith(
+      ["forward", "start", "--background", "18791", "beta"],
+      expect.objectContaining({ ignoreError: true }),
+    );
+  });
+
+  it("still fails when `forward start` exits non-zero and no target-owned forward is active", () => {
+    vi.stubEnv("NEMOCLAW_FORWARD_RECOVERY_WAIT_MS", "0");
+
+    // No live owner row ever appears: a genuine start failure must not be
+    // masked by the idempotency re-probe.
+    vi.spyOn(forwardHealth, "isLocalForwardReachable").mockReturnValue(false);
+    vi.spyOn(openshellRuntime, "captureOpenshell").mockReturnValue({
+      status: 0,
+      output: "SANDBOX  BIND  PORT  PID  STATUS",
+    });
+    vi.spyOn(openshellRuntime, "runOpenshell").mockImplementation((rawArgs: unknown) => {
+      const args = Array.isArray(rawArgs) ? rawArgs.map(String) : [];
+      return { status: args[0] === "forward" && args[1] === "start" ? 1 : 0 } as never;
+    });
+
+    expect(
+      withFakeOpenshellBinary(() =>
+        ensureSandboxPortForwardForPort("beta", 18791, { expectedBind: "127.0.0.1" }),
+      ),
+    ).toBe(false);
+  });
+
+  it("rejects a reachable listener that never gains authoritative ownership", () => {
+    vi.stubEnv("NEMOCLAW_FORWARD_RECOVERY_WAIT_MS", "25");
+
+    vi.spyOn(forwardHealth, "isLocalForwardReachable").mockReturnValue(true);
+    const captureOpenshell = vi.spyOn(openshellRuntime, "captureOpenshell").mockReturnValue({
+      status: 0,
+      output: "SANDBOX  BIND  PORT  PID  STATUS",
+    });
+    const runOpenshell = vi
+      .spyOn(openshellRuntime, "runOpenshell")
+      .mockImplementation((rawArgs: unknown) => {
+        const args = Array.isArray(rawArgs) ? rawArgs.map(String) : [];
+        const isForwardStart = args[0] === "forward" && args[1] === "start";
+        return { status: Number(isForwardStart) } as never;
+      });
+
+    expect(
+      withFakeOpenshellBinary(() =>
+        ensureSandboxPortForwardForPort("beta", 18791, { expectedBind: "127.0.0.1" }),
+      ),
+    ).toBe(false);
+    expect(captureOpenshell.mock.calls.length).toBeGreaterThanOrEqual(3);
+    expect(runOpenshell).toHaveBeenCalledWith(
+      ["forward", "start", "--background", "18791", "beta"],
+      expect.objectContaining({ ignoreError: true }),
+    );
+  });
+
+  it("waits for delayed target ownership after a non-zero `forward start`", () => {
+    vi.stubEnv("NEMOCLAW_FORWARD_RECOVERY_WAIT_MS", "250");
+    let started = false;
+    let postStartProbes = 0;
+
+    vi.spyOn(forwardHealth, "isLocalForwardReachable").mockImplementation(() => started);
+    vi.spyOn(openshellRuntime, "captureOpenshell").mockImplementation(() => {
+      postStartProbes += Number(started);
+      return {
+        status: 0,
+        output:
+          postStartProbes >= 2
+            ? `SANDBOX  BIND  PORT  PID  STATUS
+beta  127.0.0.1  18791  12345  running`
+            : "SANDBOX  BIND  PORT  PID  STATUS",
+      };
+    });
+    vi.spyOn(openshellRuntime, "runOpenshell").mockImplementation((rawArgs: unknown) => {
+      const args = Array.isArray(rawArgs) ? rawArgs.map(String) : [];
+      const isForwardStart = args[0] === "forward" && args[1] === "start";
+      started ||= isForwardStart;
+      return { status: Number(isForwardStart) } as never;
+    });
+
+    expect(
+      withFakeOpenshellBinary(() =>
+        ensureSandboxPortForwardForPort("beta", 18791, { expectedBind: "127.0.0.1" }),
+      ),
+    ).toBe(true);
+    expect(postStartProbes).toBe(2);
+  });
+
+  it("rejects delayed ownership by another sandbox after a non-zero `forward start`", () => {
+    vi.stubEnv("NEMOCLAW_FORWARD_RECOVERY_WAIT_MS", "250");
+    let started = false;
+    let postStartProbes = 0;
+
+    vi.spyOn(forwardHealth, "isLocalForwardReachable").mockImplementation(() => started);
+    vi.spyOn(openshellRuntime, "captureOpenshell").mockImplementation(() => {
+      postStartProbes += Number(started);
+      return {
+        status: 0,
+        output:
+          postStartProbes >= 2
+            ? `SANDBOX  BIND  PORT  PID  STATUS
+gamma  127.0.0.1  18791  12345  running`
+            : "SANDBOX  BIND  PORT  PID  STATUS",
+      };
+    });
+    vi.spyOn(openshellRuntime, "runOpenshell").mockImplementation((rawArgs: unknown) => {
+      const args = Array.isArray(rawArgs) ? rawArgs.map(String) : [];
+      const isForwardStart = args[0] === "forward" && args[1] === "start";
+      started ||= isForwardStart;
+      return { status: Number(isForwardStart) } as never;
+    });
+
+    expect(
+      withFakeOpenshellBinary(() =>
+        ensureSandboxPortForwardForPort("beta", 18791, { expectedBind: "127.0.0.1" }),
+      ),
+    ).toBe(false);
+    expect(postStartProbes).toBe(2);
+  });
+});

test/process-recovery.test.ts

@@ -282,23 +282,25 @@ beta  127.0.0.1  18789  12345  running`;
     );
   });
 
-  it("fails closed without starting when an unowned stopped-forward listener never releases", () => {
+  it("fails closed after start reconciliation when a listener remains unowned", () => {
     const openshellRuntime = requireSource("../src/lib/adapters/openshell/runtime.js");
     const forwardHealth = requireSource("../src/lib/actions/sandbox/forward-health.js");
 
-    vi.stubEnv("NEMOCLAW_FORWARD_RECOVERY_WAIT_MS", "150");
+    vi.stubEnv("NEMOCLAW_FORWARD_RECOVERY_WAIT_MS", "25");
     vi.spyOn(openshellRuntime, "captureOpenshell").mockReturnValue({ status: 0, output: "" });
     vi.spyOn(forwardHealth, "isLocalForwardReachable").mockReturnValue(true);
     const runOpenshell = vi
       .spyOn(openshellRuntime, "runOpenshell")
-      .mockReturnValue({ status: 0 } as never);
+      .mockImplementation((rawArgs: unknown) => {
+        const args = Array.isArray(rawArgs) ? rawArgs.map(String) : [];
+        return { status: Number(args[0] === "forward" && args[1] === "start") } as never;
+      });
 
     expect(ensureSandboxPortForwardForPort("beta", 8642)).toBe(false);
-    expect(
-      runOpenshell.mock.calls.some(
-        ([rawArgs]) => Array.isArray(rawArgs) && rawArgs[0] === "forward" && rawArgs[1] === "start",
-      ),
-    ).toBe(false);
+    expect(runOpenshell).toHaveBeenCalledWith(
+      ["forward", "start", "--background", "8642", "beta"],
+      { ignoreError: true, stdio: "ignore" },
+    );
   });
 
   it("checkAndRecoverSandboxProcesses re-establishes an active Teams messaging host forward from a compact plan when the dashboard forward is healthy", () => {