Fix StreamForwarder crashing host process on unhandled socket errors#138
Merged
DavidObando merged 1 commit intoMay 1, 2026
Merged
Conversation
StreamForwarder pipes localStream and remoteStream together in its constructor with no 'error' listener on either side. Node's pipe() does not propagate errors between streams, so when the underlying transport resets (SSH channel abort, TCP reset, keepalive timeout) the stream emits an unhandled 'error' event and Node crashes the host process. Attach 'error' listeners on both streams that trace the event and dispose the forwarder via its existing idempotent close path. Fixes microsoft#137 Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
Author
|
@microsoft-github-policy-service agree |
DavidObando
approved these changes
May 1, 2026
DavidObando
added a commit
that referenced
this pull request
May 5, 2026
* Improve port-forwarding resilience under bursty multi-port load Follow-up to #138. PR #138 prevented host-process crashes from unhandled 'error' events on duplex streams inside StreamForwarder, but several related issues remained that either (a) leaked resources, (b) still left a small unhandled-'error' window outside the forwarder, or (c) wasted work on doomed connections. This change addresses them. Changes: * StreamForwarder: accept an optional onDisposed callback so the PortForwardingService can remove disposed forwarders from its tracking collection. Add a clarifying comment about pipe()'s end-vs-error semantics. * PortForwardingService: change streamForwarders from an append-only array to a Set, with a removeStreamForwarder callback wired into every StreamForwarder. Previously disposed forwarders were retained for the lifetime of the session, holding references to their socket and SshStream — a real leak under bursty connect/disconnect workloads (the exact workload PR #138 was reacting to). * LocalPortForwarder.acceptConnection: attach a temporary 'error' handler on the accepted socket *before* awaiting openChannel and the forwardedPortConnecting event. Without this, a peer reset during that await window emits 'error' on a listener-less socket and crashes the host — the same crash class as #138, just earlier in the lifecycle. Also destroy the socket when the connecting event handler rejects the connection (was previously leaked). * RemotePortForwarder.forwardChannel: - Bug fix: when the local TCP connect fails, return early instead of falling through to construct a StreamForwarder around a destroyed socket. The forwardedStream is now also destroyed so the underlying SSH channel doesn't leak. - Attach a temporary 'error' handler on the SshStream during the forwardedPortConnecting + connect window for the same reason as the local side: a remote channel reset in that window would emit an unhandled 'error'. * SshStream.destroy: trace channel.close() failures instead of silently swallowing them with .catch(). Aids diagnosis of teardown issues that the new error path may now expose. Behavior is unchanged on the happy path. On the error path, forwarders self-dispose, are removed from the PFS collection, and traces include the side and reason. No public API changes. Verified locally: npm run compile, npm run eslint, and npm run test-ts all pass (one pre-existing unrelated failure in VersionTests). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review findings: listener leaks, channel leaks, dispose race, and tests Fix several edge cases identified during code review of PR #144: 1. Listener leak if forwardedPortConnecting throws: wrap the call in try/catch in both localPortForwarder and remotePortForwarder so the temporary error handler is always removed and streams are destroyed on unexpected exceptions. 2. SSH channel leak in localPortForwarder.acceptConnection: when the forwardedPortConnecting event handler rejects (returns null), the already-opened SSH channel was not being closed. Now destroy the SshStream so the channel is released. Also remove the temporary error listener in the openChannel catch path for consistency. 3. Synchronous-dispose race in StreamForwarder constructor: if an error fires during pipe() setup (before the caller can call streamForwarders.add()), the onDisposed callback runs on a forwarder not yet in the set, then the caller adds a dead forwarder. Fixed by adding a public isDisposed getter and guarding add() with it. 4. StreamForwarder tests: add streamForwarderTests.ts with 10 tests covering data forwarding, error-triggered dispose, callback invocation, idempotent dispose, the synchronous-dispose race, and onDisposed error swallowing. Export StreamForwarder from the package (constructor made public) to enable testing. 5. Ensure sshStream is destroyed when forwardedStream is user-substituted: in remotePortForwarder error paths, if the forwardedStream returned by the connecting event is not the original sshStream, explicitly destroy sshStream so the underlying SSH channel does not leak. Verified: npm run compile, npm run eslint, npm run test-ts all pass (357 passing, 1 pre-existing unrelated VersionTests failure). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
StreamForwarderpipes two Duplex streams together but never attaches'error'listeners on either side. When the underlying transport resets(SSH channel aborted by remote, TCP reset on the local socket, keepalive
timeout), the stream emits an
'error'event with zero listeners and Nodecrashes the entire host process with:
Reproduces with any Node app using
TunnelRelayTunnelClientwith multipleforwarded ports under bursty load - any channel reset during teardown is
fatal.
Root cause
services/streamForwarder.tsconstructor pipeslocalStream<->remoteStreamwith no error handlers. Node's
.pipe()does not propagate errors betweenstreams, so each side must be handled independently.
Fix
Attach
'error'listeners on both streams in the constructor. On error,trace the event and dispose the forwarder (which already has correct
idempotent teardown logic via
this.disposed).Changes
src/ts/ssh-tcp/services/streamForwarder.tsTesting
Verified locally with a Node app forwarding multiple ports through
TunnelRelayTunnelClient: before this change, triggering a channelreset kills the host process within seconds. After, the stream disposes
cleanly with a warning trace and the host process continues running.
Happy to add a unit test if requested - didn't see an existing test
pattern for StreamForwarder error propagation in
test/ts/ssh-tcp/,so wanted to sanity-check scope first.
Fixes #137