fix(ssh): release port-forward listener on transport death - #763
Conversation
Local port forwards closed their listener only on ctx cancellation or idle timeout, never when the SSH transport died. On a network drop the accept loop stayed blocked on the dead client and the bound host port leaked, so reconnects failed with 'bind: address already in use'. Watch client.Wait() and cancel the forward context when the transport closes, releasing the listener and returning ErrTransportClosed so the port can be rebound on reconnect. Refs #759
✅ Deploy Preview for images-devsy-sh canceled.
|
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe SSH forwarder now distinguishes transport termination from idle timeout, propagates ChangesSSH transport shutdown
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SSHClient
participant PortForwarding
participant LocalListener
SSHClient->>PortForwarding: Transport closes
PortForwarding->>PortForwarding: Cancel with ErrTransportClosed
PortForwarding->>LocalListener: Release listener
PortForwarding-->>SSHClient: Return ErrTransportClosed
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
✅ Deploy Preview for devsydev canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/ssh/forward.go (1)
119-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect watcher design; consider logging the transport-close cause.
The watcher correctly converts
client.Wait()into a signal that cancelsfwdCtxwithErrTransportClosed, and thedone-guarded goroutine avoids leaking past normal shutdown. One easy improvement:client.Wait()'s returned error is discarded (_ = client.Wait()), losing the underlying reason the transport died (useful for diagnosing dropped tunnels/reconnects in production).♻️ Optional: capture and log the Wait() error
go func() { - _ = client.Wait() + if werr := client.Wait(); werr != nil { + log.Debugf("ssh transport closed on %s: %v", srcAddr, werr) + } close(transportClosed) }()🤖 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 `@pkg/ssh/forward.go` around lines 119 - 133, Update the client.Wait watcher in the client != nil block to capture its returned error and log the transport-close cause before closing transportClosed, while preserving the existing done-guarded cancellation flow and ErrTransportClosed behavior.
🤖 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.
Inline comments:
In `@pkg/ssh/forward_test.go`:
- Around line 111-143: Register cleanup for ln immediately after net.Listen
succeeds, before starting the Accept goroutine or calling ssh.Dial, including
the existing listener-close and server termination cleanup via the test’s
cleanup mechanism. Ensure cleanup runs when Dial fails and preserves the current
cleanup behavior for successful connections.
---
Nitpick comments:
In `@pkg/ssh/forward.go`:
- Around line 119-133: Update the client.Wait watcher in the client != nil block
to capture its returned error and log the transport-close cause before closing
transportClosed, while preserving the existing done-guarded cancellation flow
and ErrTransportClosed behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c209811-34c8-44c5-9506-860e5fe81e7e
📒 Files selected for processing (2)
pkg/ssh/forward.gopkg/ssh/forward_test.go
1 participant
Refs #759 (secondary bug)
Summary
Local port forwards (
portForwardinginpkg/ssh/forward.go) closed their listener only onctxcancellation or idle timeout — never when the SSH transport died. On a network drop the accept loop stayed blocked on the dead client and the bound host port leaked, so each reconnect failed withbind: address already in use(the reporter saw this once per reconnect on port 7777).This adds a watcher that cancels the forward context when
client.Wait()returns, so the listener is released promptly andportForwardingreturns the newErrTransportClosed, letting the port be rebound on reconnect.Verification
TestPortForwarding_ReleasesListenerOnTransportDeathstands up a real SSH client/server, severs the transport, and asserts the forward returnsErrTransportClosedand the host port rebinds.listener leaked) and passes with it../pkg/ssh/...suite (34 tests) green;go vetand full build clean.Relationship to the keep-alive fix
Separate from #760 (which loosens the server keep-alive so brief stalls don't tear the tunnel down in the first place). This one addresses the port-leak that surfaced on the subsequent reconnect.
Summary by CodeRabbit