Repository navigation
fix(messaging): reserve SSH forward ports until handoff and recover lost candidates - #7893
Conversation
…tdown Keep intrinsic I/O and pre-cleanup exit failure provenance on each helper. Join diagnostic output and pair cleanup before classifying readiness failures, so cleanup-induced sibling rejection cannot mask an explicit lost forward port. Independent failures, cancellation and incomplete cleanup remain terminal. Add deterministic L/R delayed-stderr recovery and exhaustion regressions plus terminal-failure controls. Keep reservation handoff, setup deadline and the three-attempt ceiling unchanged. Validation: regression 11 pass/4 fail before, 15 pass/0 fail after; requested eight focused files 85 pass/15 skip/0 fail; test:changed 769 pass/112 skip/0 fail; typecheck, structure:check, privacy:scan and git diff --check passed.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Port reservation and SSH readiness src/messaging/remote-ports.ts, src/messaging/remote-forward-failure.ts, src/messaging/remote-process.ts, tests/codex-integration/messaging-remote-port-reservation.test.ts, tests/codex-integration/messaging-remote-forward-failure.test.ts |
Local port candidates are reserved exclusively until handoff. SSH helper stderr is checked for forwarding failure and direction-specific readiness receipts without returning diagnostic text through helper output. Tests cover reservation, receipt parsing, and failure detection. |
Forward-pair setup and recovery src/messaging/remote-tunnels.ts, tests/codex-integration/messaging-remote-port-reservation.test.ts, tests/codex-integration/messaging-remote-forward-failure.test.ts, structure/remote-messaging.md |
Pair setup waits for both readiness receipts before loopback inspection. It retries only forward_port_lost, after closing the failed pair, and stops after three attempts. Tests and documentation cover retry, cleanup, cancellation, and terminal failures. |
Remote setup integration src/messaging/remote-tunnels.ts, docs-site/src/content/docs/reference/cli/messaging.md, tests/codex-integration/messaging-remote-enrollment-gaps.test.ts, tests/codex-integration/messaging-remote-lifecycle.test.ts, scripts/test-layout/layout-codex-collision.json, tests/fixtures/test-layout-expected.json |
Remote tunnel startup delegates forward setup to the pair flow. The CLI documentation describes lease renewal and bounded recovery. Existing helper mocks and test-layout mappings are updated for the readiness contract and new tests. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant RemoteMessaging
participant startRemoteForwardPair
participant SSHHelpers
participant LoopbackInspection
RemoteMessaging->>startRemoteForwardPair: request forward setup
startRemoteForwardPair->>SSHHelpers: start local and return forwards
SSHHelpers->>startRemoteForwardPair: provide readiness receipts or forward failure
startRemoteForwardPair->>LoopbackInspection: inspect after both receipts
LoopbackInspection->>startRemoteForwardPair: return reachability result
startRemoteForwardPair->>RemoteMessaging: return pair or setup failure
Possibly related issues
lidge-jun/opencodexissue 7845: The changes reserve local candidates through SSH handoff and reselect ports after recognized lost-port failures, matching the issue’s reported release-before-bind race.
Suggested reviewers: phillipod
Merge Risk: 
1ef95
-
No actionable issue remains from this review; the change is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main changes: reserving SSH forward ports until handoff and recovering lost candidates. Linked Issues check Issue #7845requires the product paths to retain a local SSH forward candidate until handoff, or detect loss and reselect within a bound, without weakening authenticated readiness. `src/messaging/remo…Out of Scope Changes check The changes stay within issue #7845. The source changes implement port reservation, SSH bind-failure detection, bounded recovery, pair cleanup, and readiness ordering. The new tests directly verify th…Full details: Linked Issues check
-
Explanation
Issue
#7845requires the product paths to retain a local SSH forward candidate until handoff, or detect loss and reselect within a bound, without weakening authenticated readiness.src/messaging/remote-ports.tsadds exclusive socket reservations and process-local pending-port tracking.src/messaging/remote-tunnels.tshands off the local reservation immediately before SSH spawn, waits for both forward receipts, retries onlyforward_port_lost, and stops after three attempts.src/messaging/remote-process.tsandsrc/messaging/remote-forward-failure.tsclassify only recognized SSH bind failures and do not retain stderr. The added tests cover exclusive reservation, duplicate pending selections,-Land-Rfailure detection, delayed diagnostics, retry exhaustion, terminal failures, cancellation, cleanup, and readiness ordering. The changes preserve the existing authenticated tunnel setup path.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#7845. The source changes implement port reservation, SSH bind-failure detection, bounded recovery, pair cleanup, and readiness ordering. The new tests directly verify those behaviors. The documentation indocs-site/.../messaging.mdandstructure/remote-messaging.md, test-layout updates, and mock updates support the same implementation and test contract. No unrelated product behavior or cleanup appears in the reviewed changes.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 7 functions across 8 files. (4 skipped: 4 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Summary
Remote messaging setup picked its local SSH forward ports with
remotePortCandidate(), which bound port 0, read the number and released the listener before SSH bound that port. Another process, or a second selection in the same process, could take the port in between. Bun also binds a second listener on an occupied port withoutEADDRINUSEunlessexclusiveis set, so the collision was not always visible as a bind error. Setup then failed, or forwarded to a port shared with another listener. Authenticated readiness already prevented talking to the wrong peer, so the impact was availability.This change:
-L/-Rbind failures through its fixed debug1 messages (identical in OpenSSH 7.9, current portable OpenSSH and Microsoft's Windows OpenSSH). Only that explicit signal counts asforward_port_lost. stderr is scanned in memory and never printed or retained. If the message is missing or worded differently, setup fails closed: readiness stays pending until the existing deadline.forward_port_lost. Authentication failures, cancellation, independent helper failures and incomplete cleanup stay terminal. Owner authentication, native initialization and return-route registration are still required before the tunnel is used.Closes #7845
Verification
Pinned Bun 1.4.4 on Windows.
tests/codex-integration/messaging-remote-forward-failure.test.ts, red → green:tests/codex-integration/messaging-remote-port-reservation.test.tscovers exclusive reservation and duplicate pending selections.tests/codex-integration/messaging-remote-*.test.ts,tests/test-layout.test.ts,tests/test-layout-tooling.test.ts): 85 pass / 15 existing Windows skips / 0 fail.bun run test:changed: 769 pass / 112 skip / 0 fail.bun run typecheck(2,228 files),bun run structure:check,bun run privacy:scan,git diff --check: pass. Docs site build passed on the first commit (593 pages).An independent review of the first commit found the sibling-shutdown race, which the second commit fixes. A fresh independent review of this head (
1ef955759) passed with no blockers. It re-ran every ordering: initiator stderr before and after its exit, sibling exit first, both ports lost, and cancellation during shutdown. Retryable cases stopped at three attempts and terminal cases after one; unresponsive helpers settled ascleanup_incompletewithin the cleanup ceiling.Hosted CI: all 30 checks green on
1ef955759. The firsttest 2/4attempt failed intests/ci-workflows/ci-gui-typecheck-gate.test.tswith "Bun's postinstall script was not run". A deadline-killedbun installattempt had leftnode_modules/bunwithout its binary, and the retry reported success (written up on #7870). Only that job was rerun, on the same head.Checklist
docs-site/.../reference/cli/messaging.md,structure/remote-messaging.md.)Summary by CodeRabbit