From f26d5c974cbc6e119259180bc84ba918bfa4ad16 Mon Sep 17 00:00:00 2001 From: Charan Jagwani Date: Mon, 20 Jul 2026 14:29:59 -0700 Subject: [PATCH 1/6] fix(onboard): retry terminated forward listeners --- src/lib/onboard/agent-fixed-forward.ts | 4 +- src/lib/onboard/dashboard.ts | 4 +- src/lib/onboard/forward-start.test.ts | 247 +++++++++++++++++++++++-- src/lib/onboard/forward-start.ts | 90 ++++++--- 4 files changed, 297 insertions(+), 48 deletions(-) diff --git a/src/lib/onboard/agent-fixed-forward.ts b/src/lib/onboard/agent-fixed-forward.ts index 70357412269..bdfc46a754e 100644 --- a/src/lib/onboard/agent-fixed-forward.ts +++ b/src/lib/onboard/agent-fixed-forward.ts @@ -6,7 +6,7 @@ import { bestEffortForwardStopForSandbox } from "./forward-cleanup"; import { buildDetachedForwardStartSpawn, buildForwardStartProgressLogger, - runDetachedForwardStartWithPortReleaseRetries, + runDetachedForwardStartWithRetries, } from "./forward-start"; type CommandResult = { status: number | null }; @@ -35,7 +35,7 @@ export function ensureAgentFixedForward( ); stopForwardForSandbox(port); - const { ok, diagnostic } = runDetachedForwardStartWithPortReleaseRetries( + const { ok, diagnostic } = runDetachedForwardStartWithRetries( buildDetachedForwardStartSpawn( deps.openshellArgv(["forward", "start", "--background", forwardTarget, sandboxName]), ), diff --git a/src/lib/onboard/dashboard.ts b/src/lib/onboard/dashboard.ts index 0fd817e7209..57988b0ef52 100644 --- a/src/lib/onboard/dashboard.ts +++ b/src/lib/onboard/dashboard.ts @@ -31,7 +31,7 @@ import { buildDetachedForwardStartSpawn, buildForwardStartProgressLogger, looksLikeForwardPortConflict, - runDetachedForwardStartWithPortReleaseRetries, + runDetachedForwardStartWithRetries, } from "./forward-start"; import { ensureMessagingHostForwardForSandbox, @@ -319,7 +319,7 @@ export function createOnboardDashboardHelpers(deps: OnboardDashboardDeps): Onboa parsedUrl.port = String(actualPort); const actualTarget = getDashboardForwardTarget(parsedUrl.toString()); stopForwardForSandbox(actualPort); - const { ok: fwdOk, diagnostic: fwdDiagnostic } = runDetachedForwardStartWithPortReleaseRetries( + const { ok: fwdOk, diagnostic: fwdDiagnostic } = runDetachedForwardStartWithRetries( buildDetachedForwardStartSpawn( deps.openshellArgv(["forward", "start", "--background", actualTarget, sandboxName]), ), diff --git a/src/lib/onboard/forward-start.test.ts b/src/lib/onboard/forward-start.test.ts index 128da36fe13..19496f1a7f1 100644 --- a/src/lib/onboard/forward-start.test.ts +++ b/src/lib/onboard/forward-start.test.ts @@ -7,10 +7,11 @@ import { describe, expect, it, vi } from "vitest"; import { buildDetachedForwardStartSpawn, + looksLikeForwardListenerStartFailure, looksLikeForwardPortConflict, looksLikeUntrackedForward, runDetachedForwardStartWithDiagnostics, - runDetachedForwardStartWithPortReleaseRetries, + runDetachedForwardStartWithRetries, } from "./forward-start"; // Build an `openshell forward list`-shaped output for the given live entries. @@ -421,13 +422,11 @@ describe("runDetachedForwardStartWithDiagnostics", () => { expect(isPortListening).toHaveBeenCalledWith(18789); }); - it("keeps waiting (then times out) when ssh exits under ControlMaster but the port is not live (#6099)", () => { - // A genuinely failed ssh (auth error, refused connection) produces the same - // exit diagnostic without a live listener — the fallback must not confirm. + it("returns immediately when ssh exits and no ControlMaster listener is live (#7266)", () => { const fetchList = vi.fn().mockReturnValue(forwardListWith([])); const spawn = vi.fn().mockImplementation(({ stderr }: { stderr: number }) => { fs.writeSync(stderr, "ssh exited before local forward listener opened on 127.0.0.1:18789\n"); - return { pid: 782 }; + return {}; }); const sleep = vi.fn(); const isPortListening = vi.fn().mockReturnValue(false); @@ -440,8 +439,61 @@ describe("runDetachedForwardStartWithDiagnostics", () => { ); expect(result.ok).toBe(false); - expect(result.reason).toBe("timeout"); - expect(isPortListening).toHaveBeenCalled(); + expect(result.reason).toBe("listener-start-failure"); + expect(isPortListening).toHaveBeenCalledOnce(); + expect(sleep).not.toHaveBeenCalled(); + }); + + it("returns immediately for openshell's listener timeout when the port remains closed (#7266)", () => { + const fetchList = vi.fn().mockReturnValue(forwardListWith([])); + const spawn = vi.fn().mockImplementation(({ stderr }: { stderr: number }) => { + fs.writeSync( + stderr, + "Error: ssh process started but local forward listener was not reachable\n" + + "local forward listener did not open on 127.0.0.1:18789 within 10000ms\n", + ); + return {}; + }); + const sleep = vi.fn(); + const isPortListening = vi.fn().mockReturnValue(false); + + const result = runDetachedForwardStartWithDiagnostics( + spawn, + fetchList, + { port: 18789, sandboxName: "my-sandbox" }, + { overallTimeoutMs: 180_000, pollIntervalMs: 500, sleepMs: sleep, isPortListening }, + ); + + expect(result.ok).toBe(false); + expect(result.reason).toBe("listener-start-failure"); + expect(result.diagnostic).not.toContain("forward did not appear in list within"); + expect(isPortListening).toHaveBeenCalledOnce(); + expect(sleep).not.toHaveBeenCalled(); + }); + + it("rejects another sandbox's live row during listener-start failure (#7266)", () => { + const fetchList = vi + .fn() + .mockReturnValue(forwardListWith([{ sandbox: "other-sandbox", port: 18789 }])); + const spawn = vi.fn().mockImplementation(({ stderr }: { stderr: number }) => { + fs.writeSync( + stderr, + "local forward listener did not open on 127.0.0.1:18789 within 10000ms\n", + ); + return {}; + }); + const isPortListening = vi.fn().mockReturnValue(true); + + const result = runDetachedForwardStartWithDiagnostics( + spawn, + fetchList, + { port: 18789, sandboxName: "my-sandbox" }, + { overallTimeoutMs: 180_000, sleepMs: vi.fn(), isPortListening }, + ); + + expect(result.ok).toBe(false); + expect(result.reason).toBe("listener-start-failure"); + expect(isPortListening).not.toHaveBeenCalled(); }); it("keeps waiting (then times out) when openshell reports untracked but the port is not live", () => { @@ -490,7 +542,7 @@ describe("runDetachedForwardStartWithDiagnostics", () => { }); }); -describe("runDetachedForwardStartWithPortReleaseRetries", () => { +describe("runDetachedForwardStartWithRetries", () => { it("retries after a port-conflict diagnostic, then succeeds", () => { const fetchList = vi .fn() @@ -506,12 +558,18 @@ describe("runDetachedForwardStartWithPortReleaseRetries", () => { .mockReturnValueOnce({ pid: 99 }); const sleep = vi.fn(); - const result = runDetachedForwardStartWithPortReleaseRetries( + const result = runDetachedForwardStartWithRetries( spawn, fetchList, { port: 18789, sandboxName: "my-sandbox" }, beforeRetry, - { overallTimeoutMs: 30, pollIntervalMs: 10, sleepMs: sleep, maxRetries: 3 }, + { + overallTimeoutMs: 30, + pollIntervalMs: 10, + sleepMs: sleep, + maxRetries: 3, + isPortListening: vi.fn().mockReturnValue(false), + }, ); expect(result.ok).toBe(true); @@ -525,12 +583,18 @@ describe("runDetachedForwardStartWithPortReleaseRetries", () => { const spawn = vi.fn().mockReturnValue({ pid: 42 }); const sleep = vi.fn(); - const result = runDetachedForwardStartWithPortReleaseRetries( + const result = runDetachedForwardStartWithRetries( spawn, fetchList, { port: 18789, sandboxName: "my-sandbox" }, beforeRetry, - { overallTimeoutMs: 20, pollIntervalMs: 10, sleepMs: sleep, maxRetries: 3 }, + { + overallTimeoutMs: 20, + pollIntervalMs: 10, + sleepMs: sleep, + maxRetries: 3, + isPortListening: vi.fn().mockReturnValue(false), + }, ); expect(result.ok).toBe(false); @@ -547,18 +611,156 @@ describe("runDetachedForwardStartWithPortReleaseRetries", () => { .mockReturnValue({ error: new Error("EADDRINUSE: address already in use") }); const sleep = vi.fn(); - const result = runDetachedForwardStartWithPortReleaseRetries( + const result = runDetachedForwardStartWithRetries( spawn, fetchList, { port: 18789, sandboxName: "my-sandbox" }, beforeRetry, - { overallTimeoutMs: 20, pollIntervalMs: 10, sleepMs: sleep, maxRetries: 2 }, + { + overallTimeoutMs: 20, + pollIntervalMs: 10, + sleepMs: sleep, + maxRetries: 2, + isPortListening: vi.fn().mockReturnValue(false), + }, ); expect(result.ok).toBe(false); expect(beforeRetry).toHaveBeenCalledTimes(2); expect(spawn).toHaveBeenCalledTimes(3); // initial + 2 retries }); + + it("cleans and retries a failed listener start, then succeeds (#7266)", () => { + const fetchList = vi + .fn() + .mockReturnValueOnce(forwardListWith([])) + .mockReturnValue(forwardListWith([{ sandbox: "my-sandbox", port: 18789 }])); + const spawn = vi + .fn() + .mockImplementationOnce(({ stderr }: { stderr: number }) => { + fs.writeSync( + stderr, + "local forward listener did not open on 127.0.0.1:18789 within 10000ms\n", + ); + return {}; + }) + .mockReturnValueOnce({ pid: 785 }); + const beforeRetry = vi.fn(); + + const result = runDetachedForwardStartWithRetries( + spawn, + fetchList, + { port: 18789, sandboxName: "my-sandbox" }, + beforeRetry, + { + overallTimeoutMs: 180_000, + pollIntervalMs: 500, + sleepMs: vi.fn(), + isPortListening: vi.fn().mockReturnValue(false), + }, + ); + + expect(result.ok).toBe(true); + expect(beforeRetry).toHaveBeenCalledOnce(); + expect(spawn).toHaveBeenCalledTimes(2); + }); + + it("preserves a ControlMaster listener created by the current attempt (#6099)", () => { + const fetchList = vi.fn().mockReturnValue(forwardListWith([])); + const spawn = vi.fn().mockImplementation(({ stderr }: { stderr: number }) => { + fs.writeSync(stderr, "ssh exited before local forward listener opened on 127.0.0.1:18789\n"); + return { pid: 785 }; + }); + const beforeRetry = vi.fn(); + const isPortListening = vi + .fn() + .mockReturnValueOnce(false) // free before this attempt starts + .mockReturnValueOnce(true); // mux owns the listener after ssh exits + + const result = runDetachedForwardStartWithRetries( + spawn, + fetchList, + { port: 18789, sandboxName: "my-sandbox" }, + beforeRetry, + { isPortListening }, + ); + + expect(result.ok).toBe(true); + expect(result.reason).toBe("ok-port-live"); + expect(beforeRetry).not.toHaveBeenCalled(); + expect(isPortListening).toHaveBeenCalledTimes(2); + }); + + it("stops after bounded retries when listener startup keeps failing (#7266)", () => { + const fetchList = vi.fn().mockReturnValue(forwardListWith([])); + const spawn = vi.fn().mockImplementation(({ stderr }: { stderr: number }) => { + fs.writeSync( + stderr, + "local forward listener did not open on 127.0.0.1:18789 within 10000ms\n", + ); + return {}; + }); + const beforeRetry = vi.fn(); + + const result = runDetachedForwardStartWithRetries( + spawn, + fetchList, + { port: 18789, sandboxName: "my-sandbox" }, + beforeRetry, + { + overallTimeoutMs: 180_000, + pollIntervalMs: 500, + sleepMs: vi.fn(), + isPortListening: vi.fn().mockReturnValue(false), + maxRetries: 2, + }, + ); + + expect(result.ok).toBe(false); + expect(result.reason).toBe("listener-start-failure"); + expect(beforeRetry).toHaveBeenCalledTimes(2); + expect(spawn).toHaveBeenCalledTimes(3); + }); + + it("does not retry unrelated authentication failures (#7266)", () => { + const fetchList = vi.fn(); + const spawn = vi.fn().mockReturnValue({ error: new Error("Permission denied (publickey)") }); + const beforeRetry = vi.fn(); + + const result = runDetachedForwardStartWithRetries( + spawn, + fetchList, + { port: 18789, sandboxName: "my-sandbox" }, + beforeRetry, + { maxRetries: 3, isPortListening: vi.fn().mockReturnValue(false) }, + ); + + expect(result.ok).toBe(false); + expect(result.reason).toBe("spawn-error"); + expect(beforeRetry).not.toHaveBeenCalled(); + expect(spawn).toHaveBeenCalledOnce(); + }); + + it("never spawns over an arbitrary listener that predates the attempt (#7266)", () => { + const fetchList = vi.fn(); + const spawn = vi.fn(); + const beforeRetry = vi.fn(); + const isPortListening = vi.fn().mockReturnValue(true); + + const result = runDetachedForwardStartWithRetries( + spawn, + fetchList, + { port: 18789, sandboxName: "my-sandbox" }, + beforeRetry, + { maxRetries: 2, isPortListening }, + ); + + expect(result.ok).toBe(false); + expect(result.reason).toBe("spawn-conflict"); + expect(beforeRetry).toHaveBeenCalledTimes(2); + expect(spawn).not.toHaveBeenCalled(); + expect(fetchList).not.toHaveBeenCalled(); + }); }); describe("looksLikeForwardPortConflict", () => { @@ -576,6 +778,23 @@ describe("looksLikeForwardPortConflict", () => { }); }); +describe("looksLikeForwardListenerStartFailure", () => { + it("matches only definitive listener termination diagnostics", () => { + expect( + looksLikeForwardListenerStartFailure( + "local forward listener did not open on 127.0.0.1:18789 within 10000ms", + ), + ).toBe(true); + expect( + looksLikeForwardListenerStartFailure( + "ssh exited before local forward listener opened on 127.0.0.1:18789", + ), + ).toBe(true); + expect(looksLikeForwardListenerStartFailure("Permission denied (publickey)")).toBe(false); + expect(looksLikeForwardListenerStartFailure("gateway transport unavailable")).toBe(false); + }); +}); + describe("looksLikeUntrackedForward", () => { it("matches openshell's untracked-forward notice", () => { expect( diff --git a/src/lib/onboard/forward-start.ts b/src/lib/onboard/forward-start.ts index 1b868a68d06..fe31e30a1f3 100644 --- a/src/lib/onboard/forward-start.ts +++ b/src/lib/onboard/forward-start.ts @@ -34,7 +34,13 @@ export interface DetachedForwardStartOutcome { ok: boolean; diagnostic: string; pid?: number; - reason: "ok" | "ok-port-live" | "spawn-error" | "timeout" | "spawn-conflict"; + reason: + | "ok" + | "ok-port-live" + | "spawn-error" + | "timeout" + | "spawn-conflict" + | "listener-start-failure"; } export interface DetachedForwardStartOptions { @@ -46,8 +52,8 @@ export interface DetachedForwardStartOptions { // no-op so the helper stays terminal-quiet in non-interactive contexts. onProgress?: (info: { elapsedMs: number; listSnapshot: string }) => void; progressIntervalMs?: number; - // Number of EADDRINUSE-style retries after the initial attempt. Honoured - // only by `runDetachedForwardStartWithPortReleaseRetries`. Defaults to 3. + // Number of retryable startup attempts after the initial attempt. Honoured + // only by `runDetachedForwardStartWithRetries`. Defaults to 3. maxRetries?: number; // Loopback port-liveness probe. Defaults to `probeLocalPortListening` (a // synchronous Node TCP connect to 127.0.0.1:port). Consulted only as a @@ -91,6 +97,19 @@ export function looksLikeUntrackedForward(diagnostic: string): boolean { ); } +/** + * True only after openshell reports that the SSH process has definitively + * stopped waiting for its local listener. Unlike the broader untracked- + * forward diagnostic above, this means another list poll cannot make the + * terminated attempt appear. A live-port probe must still run first because + * a ControlMaster mux can own the listener after the child exits (#6099). + */ +export function looksLikeForwardListenerStartFailure(diagnostic: string): boolean { + return /ssh exited before local forward listener opened|local forward listener did not open\b/i.test( + diagnostic, + ); +} + /** * Synchronous, dependency-free loopback port-liveness probe. forward-start * runs in a synchronous code path (see `blockingSleepMs`), so we spawn a @@ -179,13 +198,6 @@ export function buildDetachedForwardStartSpawn( }; } -function isForwardConfirmed( - forwardListOutput: string, - expect: { port: number; sandboxName: string }, -): boolean { - return getOccupiedPorts(forwardListOutput).get(String(expect.port)) === expect.sandboxName; -} - /** * Best-effort SIGTERM of the detached `openshell forward start --background` * process when the helper gives up. Without this, a slow gateway handshake @@ -311,14 +323,27 @@ export function runDetachedForwardStartWithDiagnostics( lastFetchError = err instanceof Error ? err.message : String(err); } lastListSnapshot = list; - if (isForwardConfirmed(list, expect)) { + const listedOwner = getOccupiedPorts(list).get(String(expect.port)); + if (listedOwner === expect.sandboxName) { return { ok: true, diagnostic: readDiag(), pid, reason: "ok" }; } + const listedForAnotherSandbox = Boolean(listedOwner); const diagSoFar = readDiag(); if (looksLikeForwardPortConflict(diagSoFar)) { terminateDetachedForwardChild(pid); return { ok: false, diagnostic: diagSoFar, pid, reason: "spawn-conflict" }; } + // A completed listener-start failure cannot recover through more list + // polling. Probe once for the ControlMaster exception from #6099; if the + // port is still closed, return immediately so the caller can clean the + // exact sandbox/port attempt and retry within its existing bound. + if (looksLikeForwardListenerStartFailure(diagSoFar)) { + if (!listedForAnotherSandbox && isPortListening(expect.port)) { + return { ok: true, diagnostic: readDiag(), pid, reason: "ok-port-live" }; + } + terminateDetachedForwardChild(pid); + return { ok: false, diagnostic: diagSoFar, pid, reason: "listener-start-failure" }; + } // Fallback for the "untracked forward" failure (GitHub #6099): openshell // established the SSH tunnel but could not register/track it, so it never // appears in `openshell forward list` even though the local port is @@ -327,7 +352,11 @@ export function runDetachedForwardStartWithDiagnostics( // as confirmed instead of letting onboard time out and roll back a // healthy sandbox. The EADDRINUSE conflict check above runs first, so a // port held by a *different* process is never mistaken for our forward. - if (looksLikeUntrackedForward(diagSoFar) && Date.now() >= nextPortProbeAt) { + if ( + !listedForAnotherSandbox && + looksLikeUntrackedForward(diagSoFar) && + Date.now() >= nextPortProbeAt + ) { nextPortProbeAt = Date.now() + portProbeIntervalMs; if (isPortListening(expect.port)) { return { ok: true, diagnostic: readDiag(), pid, reason: "ok-port-live" }; @@ -361,12 +390,11 @@ export function runDetachedForwardStartWithDiagnostics( } /** - * Retry the detached forward-start when the diagnostic looks like an - * EADDRINUSE-style port conflict. `beforeRetry` runs between attempts so - * the caller can drop any stale forward bound to the same port before - * trying again. + * Retry the detached forward-start after an EADDRINUSE-style port conflict or + * a definitive listener-start failure. `beforeRetry` runs between attempts so + * the caller can drop the exact sandbox/port attempt before trying again. */ -export function runDetachedForwardStartWithPortReleaseRetries( +export function runDetachedForwardStartWithRetries( runDetachedSpawn: DetachedForwardSpawnRunner, fetchForwardList: ForwardListFetcher, expect: { port: number; sandboxName: string }, @@ -374,24 +402,26 @@ export function runDetachedForwardStartWithPortReleaseRetries( options: DetachedForwardStartOptions = {}, ): DetachedForwardStartOutcome { const maxRetries = options.maxRetries ?? 3; - let attempt = runDetachedForwardStartWithDiagnostics( - runDetachedSpawn, - fetchForwardList, - expect, - options, - ); + const isPortListening = options.isPortListening ?? probeLocalPortListening; + const runAttempt = (): DetachedForwardStartOutcome => + isPortListening(expect.port) + ? { + ok: false, + diagnostic: `port ${expect.port} is already in use before forward start`, + reason: "spawn-conflict", + } + : runDetachedForwardStartWithDiagnostics(runDetachedSpawn, fetchForwardList, expect, options); + let attempt = runAttempt(); for ( let retries = 0; - !attempt.ok && looksLikeForwardPortConflict(attempt.diagnostic) && retries < maxRetries; + !attempt.ok && + (looksLikeForwardPortConflict(attempt.diagnostic) || + attempt.reason === "listener-start-failure") && + retries < maxRetries; retries++ ) { beforeRetry(); - attempt = runDetachedForwardStartWithDiagnostics( - runDetachedSpawn, - fetchForwardList, - expect, - options, - ); + attempt = runAttempt(); } return attempt; } From 3672a78a46fb5f72083aed91c618e357f7697be2 Mon Sep 17 00:00:00 2001 From: Charan Jagwani Date: Mon, 20 Jul 2026 14:46:18 -0700 Subject: [PATCH 2/6] fix(onboard): repeat dashboard cleanup on retry --- src/lib/onboard/dashboard.ts | 17 +++++--- src/lib/onboard/forward-start.ts | 7 +++ test/onboard-dashboard.test.ts | 73 ++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 6 deletions(-) diff --git a/src/lib/onboard/dashboard.ts b/src/lib/onboard/dashboard.ts index 57988b0ef52..83d4848efbd 100644 --- a/src/lib/onboard/dashboard.ts +++ b/src/lib/onboard/dashboard.ts @@ -262,11 +262,13 @@ export function createOnboardDashboardHelpers(deps: OnboardDashboardDeps): Onboa const messagingForward = resolveMessagingHostForwardForSandbox(sandboxName); if (messagingForward) preservedPorts.add(String(messagingForward.port)); const preferredPort = Number(getDashboardForwardPort(chatUiUrl)); - const stopForwardForSandbox = createSandboxForwardStopper({ - runOpenshell: deps.runOpenshell, - runCaptureOpenshell: deps.runCaptureOpenshell, - sandboxName, - }); + const makeStopForwardForSandbox = () => + createSandboxForwardStopper({ + runOpenshell: deps.runOpenshell, + runCaptureOpenshell: deps.runCaptureOpenshell, + sandboxName, + }); + const stopForwardForSandbox = makeStopForwardForSandbox(); let existingForwards = deps.runCaptureOpenshell(["forward", "list"], { ignoreError: true }); const preferredEntry = findForwardEntry(existingForwards, String(preferredPort)); if ( @@ -329,7 +331,10 @@ export function createOnboardDashboardHelpers(deps: OnboardDashboardDeps): Onboa { port: actualPort, sandboxName }, () => { deps.sleep(1); - stopForwardForSandbox(actualPort); + // The setup stopper intentionally de-duplicates ports. A retry needs + // a fresh sandbox-scoped stopper so it can clean the failed attempt + // without falling through that one-shot guard. + makeStopForwardForSandbox()(actualPort); }, { onProgress: buildForwardStartProgressLogger(actualPort) }, ); diff --git a/src/lib/onboard/forward-start.ts b/src/lib/onboard/forward-start.ts index fe31e30a1f3..5885f884d3c 100644 --- a/src/lib/onboard/forward-start.ts +++ b/src/lib/onboard/forward-start.ts @@ -103,6 +103,13 @@ export function looksLikeUntrackedForward(diagnostic: string): boolean { * forward diagnostic above, this means another list poll cannot make the * terminated attempt appear. A live-port probe must still run first because * a ControlMaster mux can own the listener after the child exits (#6099). + * + * Compatibility boundary: these exact diagnostics are emitted by the pinned + * OpenShell 0.0.85 forward-start path tracked in #7266. Reassess this matcher + * when NemoClaw's supported OpenShell range moves beyond 0.0.85, and remove it + * once OpenShell either keeps the attempt alive until the listener is ready or + * exposes a structured retryable outcome. Keep the fragments narrow so an + * unrelated SSH or gateway failure cannot enter the cleanup-and-retry path. */ export function looksLikeForwardListenerStartFailure(diagnostic: string): boolean { return /ssh exited before local forward listener opened|local forward listener did not open\b/i.test( diff --git a/test/onboard-dashboard.test.ts b/test/onboard-dashboard.test.ts index d0d827757e6..703e5a817bd 100644 --- a/test/onboard-dashboard.test.ts +++ b/test/onboard-dashboard.test.ts @@ -29,6 +29,51 @@ function createTokenDownloadRunOpenshell() { }); } +function createListenerFailureRecoveryHarness(targetPort: number) { + const sandboxName = "my-sandbox"; + const foreignPort = targetPort === 18789 ? 19000 : 18789; + let targetStopCount = 0; + const runOpenshell = vi.fn((args: string[], _opts?: Record) => { + if (args.join(" ") === `forward stop ${targetPort} ${sandboxName}`) { + targetStopCount += 1; + } + return { status: 0 }; + }); + const runCaptureOpenshell = vi.fn((args: string[], _opts?: Record) => { + if (args.join(" ") !== "forward list") return ""; + const forwards = [ + "SANDBOX BIND PORT PID STATUS", + `other-sandbox 127.0.0.1 ${foreignPort} 42000 running`, + ]; + if (targetStopCount >= 2) { + forwards.push(`${sandboxName} 127.0.0.1 ${targetPort} 42001 running`); + } + return forwards.join("\n"); + }); + const sleep = vi.fn(); + const diagnostic = `local forward listener did not open on 127.0.0.1:${targetPort} within 10000ms\n`; + const helpers = createOnboardDashboardHelpers({ + runOpenshell, + runCaptureOpenshell, + openshellArgv: () => [ + process.execPath, + "-e", + `require("node:fs").writeSync(2, ${JSON.stringify(diagnostic)})`, + ], + cliName: () => "nemoclaw", + agentProductName: () => "NemoClaw", + getProviderLabel: (provider: string) => provider, + note: vi.fn(), + isWsl: () => false, + redact: (value: unknown) => String(value), + sleep, + printAgentDashboardUi: vi.fn(), + listSandboxes: () => ({ sandboxes: [] }), + }); + + return { helpers, runOpenshell, sleep, sandboxName, foreignPort }; +} + describe("onboard dashboard helpers", () => { it("prints platform-appropriate service hints for port conflicts", () => { expect(getPortConflictServiceHints("darwin").join("\n")).toMatch(/launchctl unload/); @@ -116,6 +161,34 @@ describe("onboard dashboard helpers", () => { }); }); + it("retries a terminated dashboard listener with sandbox-scoped cleanup (#7266)", () => { + const { helpers, runOpenshell, sleep, sandboxName, foreignPort } = + createListenerFailureRecoveryHarness(18789); + + expect(helpers.ensureDashboardForward(sandboxName, "http://127.0.0.1:18789")).toBe(18789); + + const stopArgs = runOpenshell.mock.calls.map(([args]) => args); + expect( + stopArgs.filter((args) => args.join(" ") === `forward stop 18789 ${sandboxName}`), + ).toHaveLength(2); + expect(stopArgs).not.toContainEqual(["forward", "stop", String(foreignPort), "other-sandbox"]); + expect(sleep).toHaveBeenCalledTimes(1); + }); + + it("retries a terminated fixed-agent listener with sandbox-scoped cleanup (#7266)", () => { + const { helpers, runOpenshell, sleep, sandboxName, foreignPort } = + createListenerFailureRecoveryHarness(8642); + + expect(helpers.ensureAgentFixedForward(sandboxName, 8642, "agent UI")).toBe(true); + + const stopArgs = runOpenshell.mock.calls.map(([args]) => args); + expect( + stopArgs.filter((args) => args.join(" ") === `forward stop 8642 ${sandboxName}`), + ).toHaveLength(2); + expect(stopArgs).not.toContainEqual(["forward", "stop", String(foreignPort), "other-sandbox"]); + expect(sleep).toHaveBeenCalledTimes(1); + }); + it("starts declared non-dashboard agent port forwards without cleaning up the dashboard forward", () => { const forwardList = "SANDBOX BIND PORT PID STATUS\n" + From ac069ccc0487dd47bd0a074c2c282033796614d5 Mon Sep 17 00:00:00 2001 From: Charan Jagwani Date: Mon, 20 Jul 2026 14:50:28 -0700 Subject: [PATCH 3/6] test(onboard): keep retry setup linear --- test/onboard-dashboard.test.ts | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/test/onboard-dashboard.test.ts b/test/onboard-dashboard.test.ts index 703e5a817bd..f5ac49e8535 100644 --- a/test/onboard-dashboard.test.ts +++ b/test/onboard-dashboard.test.ts @@ -34,22 +34,18 @@ function createListenerFailureRecoveryHarness(targetPort: number) { const foreignPort = targetPort === 18789 ? 19000 : 18789; let targetStopCount = 0; const runOpenshell = vi.fn((args: string[], _opts?: Record) => { - if (args.join(" ") === `forward stop ${targetPort} ${sandboxName}`) { - targetStopCount += 1; - } + targetStopCount += Number(args.join(" ") === `forward stop ${targetPort} ${sandboxName}`); return { status: 0 }; }); - const runCaptureOpenshell = vi.fn((args: string[], _opts?: Record) => { - if (args.join(" ") !== "forward list") return ""; - const forwards = [ - "SANDBOX BIND PORT PID STATUS", - `other-sandbox 127.0.0.1 ${foreignPort} 42000 running`, - ]; - if (targetStopCount >= 2) { - forwards.push(`${sandboxName} 127.0.0.1 ${targetPort} 42001 running`); - } - return forwards.join("\n"); - }); + const runCaptureOpenshell = vi.fn((args: string[], _opts?: Record) => + args.join(" ") === "forward list" + ? [ + "SANDBOX BIND PORT PID STATUS", + `other-sandbox 127.0.0.1 ${foreignPort} 42000 running`, + ...(targetStopCount >= 2 ? [`${sandboxName} 127.0.0.1 ${targetPort} 42001 running`] : []), + ].join("\n") + : "", + ); const sleep = vi.fn(); const diagnostic = `local forward listener did not open on 127.0.0.1:${targetPort} within 10000ms\n`; const helpers = createOnboardDashboardHelpers({ From 5e286c442e6ebe861eb921a7959705c175ee6c2a Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 24 Jul 2026 12:12:06 -0700 Subject: [PATCH 4/6] fix(onboard): fail closed on forward retry ownership Signed-off-by: Prekshi Vyas --- src/lib/onboard/dashboard.ts | 6 +- src/lib/onboard/forward-start.test.ts | 76 +++++++++++++++++++++++-- src/lib/onboard/forward-start.ts | 81 +++++++++++++++++++++------ test/onboard-dashboard.test.ts | 38 +++++++------ 4 files changed, 160 insertions(+), 41 deletions(-) diff --git a/src/lib/onboard/dashboard.ts b/src/lib/onboard/dashboard.ts index 83d4848efbd..4976f64de1c 100644 --- a/src/lib/onboard/dashboard.ts +++ b/src/lib/onboard/dashboard.ts @@ -331,9 +331,9 @@ export function createOnboardDashboardHelpers(deps: OnboardDashboardDeps): Onboa { port: actualPort, sandboxName }, () => { deps.sleep(1); - // The setup stopper intentionally de-duplicates ports. A retry needs - // a fresh sandbox-scoped stopper so it can clean the failed attempt - // without falling through that one-shot guard. + // The setup stopper intentionally de-duplicates ports. A port-conflict + // retry needs a fresh sandbox-scoped stopper so it can preserve the + // established conflict-recovery behavior despite that one-shot guard. makeStopForwardForSandbox()(actualPort); }, { onProgress: buildForwardStartProgressLogger(actualPort) }, diff --git a/src/lib/onboard/forward-start.test.ts b/src/lib/onboard/forward-start.test.ts index 19496f1a7f1..7612cd2aeee 100644 --- a/src/lib/onboard/forward-start.test.ts +++ b/src/lib/onboard/forward-start.test.ts @@ -492,10 +492,72 @@ describe("runDetachedForwardStartWithDiagnostics", () => { ); expect(result.ok).toBe(false); - expect(result.reason).toBe("listener-start-failure"); + expect(result.reason).toBe("listener-ownership-conflict"); expect(isPortListening).not.toHaveBeenCalled(); }); + it("does not accept a live port after an ownership lookup failure (#7266)", () => { + const fetchList = vi.fn().mockImplementation(() => { + throw new Error("gateway transport: access denied"); + }); + const spawn = vi.fn().mockImplementation(({ stderr }: { stderr: number }) => { + fs.writeSync(stderr, "ssh exited before local forward listener opened on 127.0.0.1:18789\n"); + return { pid: 786 }; + }); + const isPortListening = vi.fn().mockReturnValue(true); + const realKill = process.kill; + const killSpy = vi.fn(); + (process as { kill: typeof process.kill }).kill = killSpy as unknown as typeof process.kill; + + try { + const result = runDetachedForwardStartWithDiagnostics( + spawn, + fetchList, + { port: 18789, sandboxName: "my-sandbox" }, + { overallTimeoutMs: 180_000, sleepMs: vi.fn(), isPortListening }, + ); + + expect(result.ok).toBe(false); + expect(result.reason).toBe("listener-start-failure"); + expect(result.diagnostic).toMatch(/openshell forward list failed:.*access denied/i); + expect(isPortListening).not.toHaveBeenCalled(); + expect(killSpy).toHaveBeenCalledWith(786, "SIGTERM"); + } finally { + (process as { kill: typeof process.kill }).kill = realKill; + } + }); + + it("rejects a live port without the established untracked-forward diagnostic (#7266)", () => { + const fetchList = vi.fn().mockReturnValue(forwardListWith([])); + const spawn = vi.fn().mockImplementation(({ stderr }: { stderr: number }) => { + fs.writeSync( + stderr, + "local forward listener did not open on 127.0.0.1:18789 within 10000ms\n", + ); + return { pid: 787 }; + }); + const isPortListening = vi.fn().mockReturnValue(true); + const realKill = process.kill; + const killSpy = vi.fn(); + (process as { kill: typeof process.kill }).kill = killSpy as unknown as typeof process.kill; + + try { + const result = runDetachedForwardStartWithDiagnostics( + spawn, + fetchList, + { port: 18789, sandboxName: "my-sandbox" }, + { overallTimeoutMs: 180_000, sleepMs: vi.fn(), isPortListening }, + ); + + expect(result.ok).toBe(false); + expect(result.reason).toBe("listener-ownership-conflict"); + expect(isPortListening).toHaveBeenCalledWith(18789); + expect(killSpy).toHaveBeenCalledWith(787, "SIGTERM"); + } finally { + (process as { kill: typeof process.kill }).kill = realKill; + } + }); + it("keeps waiting (then times out) when openshell reports untracked but the port is not live", () => { const fetchList = vi.fn().mockReturnValue(forwardListWith([])); const spawn = vi.fn().mockImplementation(({ stderr }: { stderr: number }) => { @@ -630,7 +692,7 @@ describe("runDetachedForwardStartWithRetries", () => { expect(spawn).toHaveBeenCalledTimes(3); // initial + 2 retries }); - it("cleans and retries a failed listener start, then succeeds (#7266)", () => { + it("preserves a concurrent same-target replacement while retrying (#7266)", () => { const fetchList = vi .fn() .mockReturnValueOnce(forwardListWith([])) @@ -661,7 +723,9 @@ describe("runDetachedForwardStartWithRetries", () => { ); expect(result.ok).toBe(true); - expect(beforeRetry).toHaveBeenCalledOnce(); + // The second forward-list row can belong to a concurrent replacement. + // Listener-failure retry must observe it without stopping by sandbox/port. + expect(beforeRetry).not.toHaveBeenCalled(); expect(spawn).toHaveBeenCalledTimes(2); }); @@ -718,7 +782,7 @@ describe("runDetachedForwardStartWithRetries", () => { expect(result.ok).toBe(false); expect(result.reason).toBe("listener-start-failure"); - expect(beforeRetry).toHaveBeenCalledTimes(2); + expect(beforeRetry).not.toHaveBeenCalled(); expect(spawn).toHaveBeenCalledTimes(3); }); @@ -756,8 +820,8 @@ describe("runDetachedForwardStartWithRetries", () => { ); expect(result.ok).toBe(false); - expect(result.reason).toBe("spawn-conflict"); - expect(beforeRetry).toHaveBeenCalledTimes(2); + expect(result.reason).toBe("listener-ownership-conflict"); + expect(beforeRetry).not.toHaveBeenCalled(); expect(spawn).not.toHaveBeenCalled(); expect(fetchList).not.toHaveBeenCalled(); }); diff --git a/src/lib/onboard/forward-start.ts b/src/lib/onboard/forward-start.ts index 5885f884d3c..4a5d5881946 100644 --- a/src/lib/onboard/forward-start.ts +++ b/src/lib/onboard/forward-start.ts @@ -40,6 +40,7 @@ export interface DetachedForwardStartOutcome { | "spawn-error" | "timeout" | "spawn-conflict" + | "listener-ownership-conflict" | "listener-start-failure"; } @@ -222,6 +223,45 @@ function terminateDetachedForwardChild(pid: number | undefined): void { } } +function classifyListenerStartDiagnostic(input: { + diagnostic: string; + pid: number | undefined; + port: number; + ownerLookupSucceeded: boolean; + listedForAnotherSandbox: boolean; + isPortListening: (port: number) => boolean; +}): DetachedForwardStartOutcome | null { + if (!looksLikeForwardListenerStartFailure(input.diagnostic)) return null; + + if (input.listedForAnotherSandbox) { + terminateDetachedForwardChild(input.pid); + return { + ok: false, + diagnostic: input.diagnostic, + pid: input.pid, + reason: "listener-ownership-conflict", + }; + } + + const portListening = input.ownerLookupSucceeded && input.isPortListening(input.port); + if (portListening && looksLikeUntrackedForward(input.diagnostic)) { + return { + ok: true, + diagnostic: input.diagnostic, + pid: input.pid, + reason: "ok-port-live", + }; + } + + terminateDetachedForwardChild(input.pid); + return { + ok: false, + diagnostic: input.diagnostic, + pid: input.pid, + reason: portListening ? "listener-ownership-conflict" : "listener-start-failure", + }; +} + /** * Default progress logger for the detached forward-start helper. Emits a * single line to stdout every `progressIntervalMs` while the helper is @@ -341,16 +381,19 @@ export function runDetachedForwardStartWithDiagnostics( return { ok: false, diagnostic: diagSoFar, pid, reason: "spawn-conflict" }; } // A completed listener-start failure cannot recover through more list - // polling. Probe once for the ControlMaster exception from #6099; if the - // port is still closed, return immediately so the caller can clean the - // exact sandbox/port attempt and retry within its existing bound. - if (looksLikeForwardListenerStartFailure(diagSoFar)) { - if (!listedForAnotherSandbox && isPortListening(expect.port)) { - return { ok: true, diagnostic: readDiag(), pid, reason: "ok-port-live" }; - } - terminateDetachedForwardChild(pid); - return { ok: false, diagnostic: diagSoFar, pid, reason: "listener-start-failure" }; - } + // polling. Preserve the established ControlMaster exception from #6099 + // only when forward-list ownership enumeration succeeded and openshell + // also emitted that narrower untracked-forward diagnostic. A live TCP + // port alone is not evidence that this attempt owns the listener. + const listenerOutcome = classifyListenerStartDiagnostic({ + diagnostic: diagSoFar, + pid, + port: expect.port, + ownerLookupSucceeded: lastFetchError === null, + listedForAnotherSandbox, + isPortListening, + }); + if (listenerOutcome) return listenerOutcome; // Fallback for the "untracked forward" failure (GitHub #6099): openshell // established the SSH tunnel but could not register/track it, so it never // appears in `openshell forward list` even though the local port is @@ -360,6 +403,7 @@ export function runDetachedForwardStartWithDiagnostics( // healthy sandbox. The EADDRINUSE conflict check above runs first, so a // port held by a *different* process is never mistaken for our forward. if ( + lastFetchError === null && !listedForAnotherSandbox && looksLikeUntrackedForward(diagSoFar) && Date.now() >= nextPortProbeAt @@ -398,14 +442,16 @@ export function runDetachedForwardStartWithDiagnostics( /** * Retry the detached forward-start after an EADDRINUSE-style port conflict or - * a definitive listener-start failure. `beforeRetry` runs between attempts so - * the caller can drop the exact sandbox/port attempt before trying again. + * a definitive listener-start failure. `beforePortConflictRetry` preserves the + * established conflict-recovery behavior. Listener-start failures retry + * without sandbox/port cleanup because OpenShell does not expose immutable + * attempt identity. */ export function runDetachedForwardStartWithRetries( runDetachedSpawn: DetachedForwardSpawnRunner, fetchForwardList: ForwardListFetcher, expect: { port: number; sandboxName: string }, - beforeRetry: () => void, + beforePortConflictRetry: () => void, options: DetachedForwardStartOptions = {}, ): DetachedForwardStartOutcome { const maxRetries = options.maxRetries ?? 3; @@ -415,19 +461,22 @@ export function runDetachedForwardStartWithRetries( ? { ok: false, diagnostic: `port ${expect.port} is already in use before forward start`, - reason: "spawn-conflict", + reason: "listener-ownership-conflict", } : runDetachedForwardStartWithDiagnostics(runDetachedSpawn, fetchForwardList, expect, options); let attempt = runAttempt(); for ( let retries = 0; !attempt.ok && - (looksLikeForwardPortConflict(attempt.diagnostic) || + ((attempt.reason !== "listener-ownership-conflict" && + looksLikeForwardPortConflict(attempt.diagnostic)) || attempt.reason === "listener-start-failure") && retries < maxRetries; retries++ ) { - beforeRetry(); + if (looksLikeForwardPortConflict(attempt.diagnostic)) { + beforePortConflictRetry(); + } attempt = runAttempt(); } return attempt; diff --git a/test/onboard-dashboard.test.ts b/test/onboard-dashboard.test.ts index f5ac49e8535..76aa168436c 100644 --- a/test/onboard-dashboard.test.ts +++ b/test/onboard-dashboard.test.ts @@ -33,19 +33,25 @@ function createListenerFailureRecoveryHarness(targetPort: number) { const sandboxName = "my-sandbox"; const foreignPort = targetPort === 18789 ? 19000 : 18789; let targetStopCount = 0; + let forwardListCallsAfterStop = 0; const runOpenshell = vi.fn((args: string[], _opts?: Record) => { - targetStopCount += Number(args.join(" ") === `forward stop ${targetPort} ${sandboxName}`); + if (args.join(" ") === `forward stop ${targetPort} ${sandboxName}`) { + targetStopCount += 1; + forwardListCallsAfterStop = 0; + } return { status: 0 }; }); - const runCaptureOpenshell = vi.fn((args: string[], _opts?: Record) => - args.join(" ") === "forward list" - ? [ - "SANDBOX BIND PORT PID STATUS", - `other-sandbox 127.0.0.1 ${foreignPort} 42000 running`, - ...(targetStopCount >= 2 ? [`${sandboxName} 127.0.0.1 ${targetPort} 42001 running`] : []), - ].join("\n") - : "", - ); + const runCaptureOpenshell = vi.fn((args: string[], _opts?: Record) => { + if (args.join(" ") !== "forward list") return ""; + if (targetStopCount > 0) forwardListCallsAfterStop += 1; + return [ + "SANDBOX BIND PORT PID STATUS", + `other-sandbox 127.0.0.1 ${foreignPort} 42000 running`, + ...(forwardListCallsAfterStop >= 2 + ? [`${sandboxName} 127.0.0.1 ${targetPort} 42001 running`] + : []), + ].join("\n"); + }); const sleep = vi.fn(); const diagnostic = `local forward listener did not open on 127.0.0.1:${targetPort} within 10000ms\n`; const helpers = createOnboardDashboardHelpers({ @@ -157,7 +163,7 @@ describe("onboard dashboard helpers", () => { }); }); - it("retries a terminated dashboard listener with sandbox-scoped cleanup (#7266)", () => { + it("retries a terminated dashboard listener without repeat cleanup (#7266)", () => { const { helpers, runOpenshell, sleep, sandboxName, foreignPort } = createListenerFailureRecoveryHarness(18789); @@ -166,12 +172,12 @@ describe("onboard dashboard helpers", () => { const stopArgs = runOpenshell.mock.calls.map(([args]) => args); expect( stopArgs.filter((args) => args.join(" ") === `forward stop 18789 ${sandboxName}`), - ).toHaveLength(2); + ).toHaveLength(1); expect(stopArgs).not.toContainEqual(["forward", "stop", String(foreignPort), "other-sandbox"]); - expect(sleep).toHaveBeenCalledTimes(1); + expect(sleep).not.toHaveBeenCalled(); }); - it("retries a terminated fixed-agent listener with sandbox-scoped cleanup (#7266)", () => { + it("retries a terminated fixed-agent listener without repeat cleanup (#7266)", () => { const { helpers, runOpenshell, sleep, sandboxName, foreignPort } = createListenerFailureRecoveryHarness(8642); @@ -180,9 +186,9 @@ describe("onboard dashboard helpers", () => { const stopArgs = runOpenshell.mock.calls.map(([args]) => args); expect( stopArgs.filter((args) => args.join(" ") === `forward stop 8642 ${sandboxName}`), - ).toHaveLength(2); + ).toHaveLength(1); expect(stopArgs).not.toContainEqual(["forward", "stop", String(foreignPort), "other-sandbox"]); - expect(sleep).toHaveBeenCalledTimes(1); + expect(sleep).not.toHaveBeenCalled(); }); it("starts declared non-dashboard agent port forwards without cleaning up the dashboard forward", () => { From 1bde00de0ad8a2ed2ecc4a9e342de5a0ca384406 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 24 Jul 2026 12:16:07 -0700 Subject: [PATCH 5/6] docs(onboard): align forward retry rationale Signed-off-by: Prekshi Vyas --- src/lib/onboard/forward-start.test.ts | 7 ++--- src/lib/onboard/forward-start.ts | 42 ++++++++++++++------------- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/src/lib/onboard/forward-start.test.ts b/src/lib/onboard/forward-start.test.ts index 7612cd2aeee..25e14c24fc1 100644 --- a/src/lib/onboard/forward-start.test.ts +++ b/src/lib/onboard/forward-start.test.ts @@ -582,10 +582,9 @@ describe("runDetachedForwardStartWithDiagnostics", () => { expect(isPortListening).toHaveBeenCalled(); }); - it("does not consult the port probe unless openshell reports an untracked forward", () => { - // A plain empty list (no "not tracked" notice) must NOT trigger the - // live-port fallback — otherwise an unrelated listener on the port could be - // mistaken for the forward. This guards the gate on looksLikeUntrackedForward. + it("does not run the post-spawn probe without a relevant diagnostic", () => { + // A plain empty list must not trigger the post-spawn live-port probe. The + // retry wrapper's separate pre-attempt probe is outside this helper. const fetchList = vi.fn().mockReturnValue(forwardListWith([])); const spawn = vi.fn().mockReturnValue({ pid: 42 }); const sleep = vi.fn(); diff --git a/src/lib/onboard/forward-start.ts b/src/lib/onboard/forward-start.ts index 4a5d5881946..7331c4a2f32 100644 --- a/src/lib/onboard/forward-start.ts +++ b/src/lib/onboard/forward-start.ts @@ -57,10 +57,10 @@ export interface DetachedForwardStartOptions { // only by `runDetachedForwardStartWithRetries`. Defaults to 3. maxRetries?: number; // Loopback port-liveness probe. Defaults to `probeLocalPortListening` (a - // synchronous Node TCP connect to 127.0.0.1:port). Consulted only as a - // fallback when openshell reports it could not track the backgrounded - // forward (see the poll loop). Injectable so unit tests need not open real - // sockets or spawn probe subprocesses. + // synchronous Node TCP connect to 127.0.0.1:port). The retry wrapper uses it + // to reject a listener that predates an attempt. The poll loop uses it only + // for listener-start and untracked-forward diagnostics. Injectable so unit + // tests need not open real sockets or spawn probe subprocesses. isPortListening?: (port: number) => boolean; } @@ -89,8 +89,9 @@ export function looksLikeForwardPortConflict(diagnostic: string): boolean { * ssh client delegates the -L forward to the ControlMaster mux daemon and * exits, which openshell 0.0.72+ reports as "ssh exited before local forward * listener opened" even though the mux daemon holds the listener and serves - * traffic. Confirmation still requires the live-port probe, so a genuinely - * failed ssh (closed port) keeps timing out as before. See GitHub #6099. + * traffic. Confirmation still requires the live-port probe. A definitive + * listener failure with a closed port returns for bounded retry; other closed- + * port failures keep polling until the deadline. See GitHub #6099 and #7266. */ export function looksLikeUntrackedForward(diagnostic: string): boolean { return /could not discover backgrounded ssh process|forward may be running but is not tracked|ssh exited before local forward listener opened|local forward listener was not reachable/i.test( @@ -102,15 +103,16 @@ export function looksLikeUntrackedForward(diagnostic: string): boolean { * True only after openshell reports that the SSH process has definitively * stopped waiting for its local listener. Unlike the broader untracked- * forward diagnostic above, this means another list poll cannot make the - * terminated attempt appear. A live-port probe must still run first because - * a ControlMaster mux can own the listener after the child exits (#6099). + * terminated attempt appear. Successful ownership enumeration plus a live-port + * probe preserves the ControlMaster compatibility path after the child exits + * (#6099). * * Compatibility boundary: these exact diagnostics are emitted by the pinned * OpenShell 0.0.85 forward-start path tracked in #7266. Reassess this matcher * when NemoClaw's supported OpenShell range moves beyond 0.0.85, and remove it * once OpenShell either keeps the attempt alive until the listener is ready or * exposes a structured retryable outcome. Keep the fragments narrow so an - * unrelated SSH or gateway failure cannot enter the cleanup-and-retry path. + * unrelated SSH or gateway failure cannot enter the listener-retry path. */ export function looksLikeForwardListenerStartFailure(diagnostic: string): boolean { return /ssh exited before local forward listener opened|local forward listener did not open\b/i.test( @@ -281,12 +283,15 @@ export function buildForwardStartProgressLogger( /** * Spawn `openshell forward start --background` as a detached child and wait * for the resulting forward to appear in `openshell forward list`. Returns - * `ok: true` as soon as the live entry is observed, regardless of whether - * the original spawn process has exited yet. Returns `ok: false` with a - * captured diagnostic when: + * `ok: true` when the expected list entry appears, or under the established + * #6099 compatibility path after successful list enumeration and a live-port + * probe. Returns `ok: false` with a captured diagnostic when: * - the spawn itself failed (ENOENT, permission denied, …); * - the parent process wrote an EADDRINUSE-style error to stderr before * the deadline (port conflict — retry path); + * - a definitive listener failure makes the attempt eligible for retry; + * - a foreign or otherwise unproven live listener creates an ownership + * conflict; * - the deadline expired without the forward appearing. * * The diagnostic file pair is removed before return, so the temp dir does @@ -394,14 +399,11 @@ export function runDetachedForwardStartWithDiagnostics( isPortListening, }); if (listenerOutcome) return listenerOutcome; - // Fallback for the "untracked forward" failure (GitHub #6099): openshell - // established the SSH tunnel but could not register/track it, so it never - // appears in `openshell forward list` even though the local port is - // already accepting connections and the dashboard is serving. When - // openshell says so AND the local forward port is live, treat the forward - // as confirmed instead of letting onboard time out and roll back a - // healthy sandbox. The EADDRINUSE conflict check above runs first, so a - // port held by a *different* process is never mistaken for our forward. + // Preserve the established "untracked forward" compatibility path + // (GitHub #6099). It requires openshell's narrow diagnostic, a successful + // list query with no foreign sandbox row, and a live local port. This is + // intentionally not widened to other diagnostics because the probe does + // not establish process identity. if ( lastFetchError === null && !listedForAnotherSandbox && From 0f6fba29260cb760fd12f4899dfa9558444bb24e Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Fri, 24 Jul 2026 12:21:32 -0700 Subject: [PATCH 6/6] test(onboard): keep retry recovery fixture linear Signed-off-by: Prekshi Vyas --- test/onboard-dashboard.test.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/onboard-dashboard.test.ts b/test/onboard-dashboard.test.ts index 76aa168436c..546a2680a92 100644 --- a/test/onboard-dashboard.test.ts +++ b/test/onboard-dashboard.test.ts @@ -35,22 +35,22 @@ function createListenerFailureRecoveryHarness(targetPort: number) { let targetStopCount = 0; let forwardListCallsAfterStop = 0; const runOpenshell = vi.fn((args: string[], _opts?: Record) => { - if (args.join(" ") === `forward stop ${targetPort} ${sandboxName}`) { - targetStopCount += 1; - forwardListCallsAfterStop = 0; - } + const stoppedTarget = args.join(" ") === `forward stop ${targetPort} ${sandboxName}`; + targetStopCount += Number(stoppedTarget); + forwardListCallsAfterStop = stoppedTarget ? 0 : forwardListCallsAfterStop; return { status: 0 }; }); const runCaptureOpenshell = vi.fn((args: string[], _opts?: Record) => { - if (args.join(" ") !== "forward list") return ""; - if (targetStopCount > 0) forwardListCallsAfterStop += 1; - return [ + const isForwardList = args.join(" ") === "forward list"; + forwardListCallsAfterStop += Number(isForwardList && targetStopCount > 0); + const output = [ "SANDBOX BIND PORT PID STATUS", `other-sandbox 127.0.0.1 ${foreignPort} 42000 running`, ...(forwardListCallsAfterStop >= 2 ? [`${sandboxName} 127.0.0.1 ${targetPort} 42001 running`] : []), ].join("\n"); + return isForwardList ? output : ""; }); const sleep = vi.fn(); const diagnostic = `local forward listener did not open on 127.0.0.1:${targetPort} within 10000ms\n`;