fix(onboard): retry terminated forward listeners

[cjagwani]

Summary

OpenShell can terminate a dashboard-forward attempt after its local listener fails to open, while NemoClaw continues polling the dead attempt for three minutes. This change recognizes that definitive failure, verifies the port is still closed, and performs bounded sandbox-scoped cleanup and retries while preserving the existing live ControlMaster exception.

Related Issue

Fixes #7266

Changes

• Return a distinct terminal result for OpenShell's ssh exited before local forward listener opened and local forward listener did not open diagnostics instead of polling an attempt that cannot recover.
• Extend the existing forward-start retry boundary used by dashboard and fixed agent forwards to clean and retry only port conflicts or definitive listener-start failures.
• Probe before each attempt and reject a pre-existing listener or another sandbox's forward row, so retry cannot turn unrelated ownership into success.
• Cover recovery, persistent failure, authentication failure, foreign ownership, arbitrary listeners, and the existing ControlMaster live-listener exception.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Quality Gates

• Tests added or updated for changed behavior
• Existing tests cover changed behavior — justification:
• Tests not applicable — justification:
• Docs updated for user-facing behavior changes
• Docs not applicable — justification: this changes internal recovery for an existing onboarding forward and does not add or change a documented command or supported contract.
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
• Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification:
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

DGX Station Hardware Evidence

• Tested on DGX Station
• Tested commit:
• Station profile/scenario:
• Result:
• Supporting evidence:

Verification

• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above — forward-start.test.ts: 35 passed; dashboard/messaging caller suite: 86 passed; dashboard integration suite: 46 passed; npm run typecheck:cli passed.
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

---

Signed-off-by: Charan Jagwani cjagwani@nvidia.com

Summary by CodeRabbit

• Bug Fixes
  • Improved dashboard and fixed-agent port-forward startup by detecting and classifying a new “listener-start-failure” condition.
  • Added bounded retry behavior for recoverable startup failures, with cleaner recovery between attempts.
  • Refined polling/probing and improved cleanup so retries and stops only occur for the correct sandbox/port.
• Tests
  • Expanded onboarding coverage for listener-start recovery scenarios, including verification of forward-stop behavior and retry/sleep timing.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0c2cf0d6-688a-4af8-a00b-d46b278725b9

📥 Commits

Reviewing files that changed from the base of the PR and between 1bde00d and 0f6fba2.

📒 Files selected for processing (1)

• test/onboard-dashboard.test.ts

---

📝 Walkthrough

Walkthrough

The forward-start flow recognizes definitive listener-start failures, checks sandbox ownership and port liveness, and retries eligible failures with bounded attempts. Agent and dashboard onboarding use the new retry helper, with sandbox-scoped cleanup and expanded tests.

Changes

Forward startup retry handling

Layer / File(s)	Summary
Listener failure detection and polling src/lib/onboard/forward-start.ts, src/lib/onboard/forward-start.test.ts	Forward startup adds the listener-start-failure outcome, classifies definitive listener diagnostics, validates sandbox ownership or port liveness, and avoids extended polling after terminated listener attempts.
Bounded retry policy src/lib/onboard/forward-start.ts, src/lib/onboard/forward-start.test.ts	runDetachedForwardStartWithRetries retries port conflicts and listener-start failures, preserves retry callbacks, and stops after bounded attempts while excluding authentication failures and arbitrary listeners.
Agent and dashboard integration src/lib/onboard/agent-fixed-forward.ts, src/lib/onboard/dashboard.ts, test/onboard-dashboard.test.ts	Agent and dashboard forwarding flows use the replacement retry helper; dashboard retries create fresh sandbox-scoped stoppers, with integration tests covering target and foreign ports.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Onboarding
  participant ForwardStart
  participant OpenShell
  participant PortProbe
  Onboarding->>ForwardStart: start detached forward with retries
  ForwardStart->>OpenShell: start SSH forward and fetch forward list
  OpenShell-->>ForwardStart: listener diagnostic and ownership rows
  ForwardStart->>PortProbe: check local port after listener failure
  PortProbe-->>ForwardStart: live or closed
  ForwardStart-->>Onboarding: success or bounded retry outcome

Loading

Suggested reviewers: prekshivyas, cv, cv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately summarizes the main onboarding change: retrying terminated forward listeners.
Linked Issues check	✅ Passed	The changes add listener-start-failure detection, bounded retries, port verification, sandbox-scoped cleanup, and preserve the fail-closed exceptions.
Out of Scope Changes check	✅ Passed	The refactor and test updates stay focused on onboarding forward recovery and retry behavior described in the issue.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-forward-listener-retry-7266

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

PR Review Advisor — Informational

Advisor assessment: Informational / high confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: No actionable findings remain in the canonical review ledger.

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings match; normalized E2E selections differ; severity counts match.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: onboard-repair, onboard-resume, cloud-onboard

1 optional E2E recommendation

• tunnel-lifecycle

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 0f6fba2 in the codex/fix-forward-li... branch remains at 96%, unchanged from commit cef1fd0 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 0f6fba2 in the codex/fix-forward-li... branch remains at 80%, unchanged from commit 361ceac in the main branch.

Show a code coverage summary of the most impacted files.

File	main 361ceac	codex/fix-forward-li... 0f6fba2	+/-
src/lib/platform.ts	89%	84%	-5%
src/lib/state/m...-acquisition.ts	89%	87%	-2%
src/lib/onboard/dashboard.ts	72%	72%	0%
src/lib/messagi...nnels/policy.ts	100%	100%	0%
src/lib/sandbox...rce-identity.ts	87%	87%	0%
src/lib/securit...ntial-filter.ts	93%	93%	0%
src/lib/trace.ts	94%	94%	0%
src/lib/onboard...shboard-port.ts	90%	93%	+3%
src/lib/onboard...orward-start.ts	89%	98%	+9%
src/lib/onboard...ixed-forward.ts	38%	77%	+39%

---

_{Updated July 24, 2026 19:33 UTC}

[coderabbitai]

🧹 Nitpick comments (1)

src/lib/onboard/forward-start.ts (1)

326-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Verified against tests — logic is correct.

Traced the listedOwner/listedForAnotherSandbox computation and the port-conflict → listener-start-failure → untracked-forward branch ordering against every new test case (foreign ownership rejection, ControlMaster live-port exception, bounded retries). All outcomes match. One minor readability note: this block plus the retry predicate in runDetachedForwardStartWithRetries (Lines 393-424) are both flagged as high-complexity by the diff tooling, and the coding guidelines ask to keep function complexity low. Consider extracting the classification (spawn-conflict / listener-start-failure / ok-port-live decision) into a small named helper to keep runDetachedForwardStartWithDiagnostics's loop body flatter, though the current early-return control flow makes this non-trivial to extract cleanly.

As per coding guidelines, "Keep function complexity low and avoid introducing unnecessary complexity hotspots."

🤖 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/onboard/forward-start.ts` around lines 326 - 346, Reduce complexity
in runDetachedForwardStartWithDiagnostics by extracting the
listedOwner/port-conflict/listener-start-failure classification into a small
named helper that preserves the existing spawn-conflict, listener-start-failure,
ok-port-live, and normal-success outcomes. Keep retry behavior in
runDetachedForwardStartWithRetries unchanged and retain the current early-return
semantics.

Source: Coding guidelines

🤖 Prompt for all review comments with 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.

Nitpick comments:
In `@src/lib/onboard/forward-start.ts`:
- Around line 326-346: Reduce complexity in
runDetachedForwardStartWithDiagnostics by extracting the
listedOwner/port-conflict/listener-start-failure classification into a small
named helper that preserves the existing spawn-conflict, listener-start-failure,
ok-port-live, and normal-success outcomes. Keep retry behavior in
runDetachedForwardStartWithRetries unchanged and retain the current early-return
semantics.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5256a50c-8531-44c3-adf6-7df259831eba

📥 Commits

Reviewing files that changed from the base of the PR and between 8331dcd and f26d5c9.

📒 Files selected for processing (4)

• src/lib/onboard/agent-fixed-forward.ts
• src/lib/onboard/dashboard.ts
• src/lib/onboard/forward-start.test.ts
• src/lib/onboard/forward-start.ts

[dfernandez365-rgb]

Security review — changes required before merge.

The new success and retry paths do not prove listener ownership:

• A port that is closed before spawn and TCP-live afterward is temporal correlation, not ownership. A foreign process can bind in that interval and be accepted as ok-port-live.
• fetchForwardList() errors are collapsed to an empty list. A gateway authentication/authorization/access failure can therefore combine with listener diagnostics and a live foreign port to return success.
• Retry cleanup waits and then stops by current sandboxName + port, not immutable attempt identity. It can stop a newer concurrent forward after OpenShell has already terminated the failed child.

Please require structured attempt/listener identity or an authenticated application-level challenge, fail closed whenever ownership enumeration fails, and avoid cleanup unless it is bound to an immutable attempt ID/PID/process identity. Add tests for access-denied plus live port, a foreign bind in the pre/post window, concurrent same-target replacement, and the positive-PID termination path. The current false-to-true mock encodes the race as proof rather than distinguishing it.