Skip to content

Fix race condition in CloseAsync that kills reconnect connections#132

Merged
DavidObando merged 3 commits into
microsoft:mainfrom
PiDiBi:user/dabures/fix-reconnect-race
Mar 31, 2026
Merged

Fix race condition in CloseAsync that kills reconnect connections#132
DavidObando merged 3 commits into
microsoft:mainfrom
PiDiBi:user/dabures/fix-reconnect-race

Conversation

@PiDiBi

@PiDiBi PiDiBi commented Mar 30, 2026

Copy link
Copy Markdown
Member

Fix race condition in CloseAsync that kills reconnect connections

Problem

When a connection drops (e.g. TCP reset), SshSession.CloseAsync runs on the ProcessMessages threadpool thread and does two things in sequence:

  1. Sets IsConnected = false (inside a lock)
  2. Calls Protocol?.Disconnect() (outside the lock)

Meanwhile, a reconnect thread (application or test) observes IsConnected == false and immediately starts ReconnectAsyncConnectAsync, which replaces the Protocol property with a new SshProtocol wrapping the new TCP stream.

The race: If the old CloseAsync thread's Protocol?.Disconnect() executes after the reconnect thread has replaced Protocol, it disconnects the new protocol/stream, killing the reconnect connection mid-key-exchange.

This manifests as:

  • SshConnectionException: Session closed while encrypting. Reason: ConnectionLost
  • The server receives KeyExchangeDhInitMessage but the connection dies before the reply can be sent
  • Flaky failures in InteropWithSshServerTSLib(..., reconnect: True) CI tests

Root Cause

Thread A (ProcessMessages → CloseAsync)     Thread B (app → ReconnectAsync)
─────────────────────────────────────────   ─────────────────────────────────
lock { IsConnected = false; }
                                            sees IsConnected == false
                                            ConnectAsync → Protocol = new SshProtocol(newStream)
Protocol?.Disconnect();  ← disconnects
the NEW stream!

Fix

Capture the Protocol reference inside the lock, atomically with IsConnected = false. Then disconnect only the captured (old) reference outside the lock:

// Before (buggy):
lock (this.disposeCancellationSource)
{
    if (IsClosed || !IsConnected) return;
    IsConnected = false;
}
// ... later:
Protocol?.Disconnect();           // ← reads current Protocol, may be replaced

// After (fixed):
SshProtocol? protocolToDisconnect;
lock (this.disposeCancellationSource)
{
    if (IsClosed || !IsConnected) return;
    IsConnected = false;
    protocolToDisconnect = Protocol;   // ← captured under lock
}
// ... later:
protocolToDisconnect?.Disconnect();    // ← always disconnects the old protocol

Changes

File Change
src/cs/Ssh/SshSession.cs Capture Protocol reference inside lock before disconnecting
test/cs/Ssh.Test/ReconnectRaceTests.cs New tests for immediate reconnect after disconnect

Testing

  • All 21 reconnect tests pass (19 existing + 2 new)
  • New tests:
    • ReconnectImmediatelyAfterDisconnect_ShouldNotFail — disconnects client stream and immediately reconnects without waiting for server to detect disconnect
    • ReconnectMultipleTimes_ShouldAlwaysSucceed — stress test with 3 rapid disconnect/reconnect cycles, verifies session works after all reconnections

David Bures and others added 3 commits March 30, 2026 12:19
When a connection drops, CloseAsync sets IsConnected = false inside a
lock, then calls Protocol?.Disconnect() outside the lock. If a
concurrent reconnect thread observes IsConnected == false and calls
ConnectAsync (which replaces Protocol with a new stream), the old
CloseAsync thread disconnects the NEW protocol, killing the reconnect
mid-key-exchange.

Fix: Capture the Protocol reference inside the lock atomically with
IsConnected = false, then disconnect only the captured (old) reference.

This manifests as flaky "Session closed while encrypting" /
"ConnectionLost" failures in reconnect scenarios, especially with
slower transports (TCP, cross-process interop tests).
@DavidObando
DavidObando merged commit 1792db0 into microsoft:main Mar 31, 2026
1 of 3 checks passed
avanderhoorn pushed a commit to avanderhoorn/dev-tunnels-ssh that referenced this pull request Apr 9, 2026
Port the race condition fix from C# PR microsoft#132:
microsoft#132

In closeImpl(), s.protocol was accessed outside the mutex (after unlock
at the end of the guard block). A concurrent reconnect could replace
s.protocol between that unlock and the subsequent use at lines 495/512,
causing the close to disconnect/close the wrong (new) connection.

Fix by capturing proto := s.protocol inside the lock, then using that
captured reference for the disconnect send and protocol close, matching
the C# fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DavidObando added a commit that referenced this pull request Apr 9, 2026
Port the race condition fix from C# PR #132:
#132

In closeImpl(), s.protocol was accessed outside the mutex (after unlock
at the end of the guard block). A concurrent reconnect could replace
s.protocol between that unlock and the subsequent use at lines 495/512,
causing the close to disconnect/close the wrong (new) connection.

Fix by capturing proto := s.protocol inside the lock, then using that
captured reference for the disconnect send and protocol close, matching
the C# fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DavidObando added a commit that referenced this pull request May 1, 2026
* feat: add native Go SSH2 client/server implementation

Full-parity Go implementation of the Dev Tunnels SSH library, matching
the existing C# and TypeScript implementations. Includes:

- Core SSH2 protocol: session lifecycle, key exchange, algorithm
  negotiation, channel multiplexing, and flow control
- Authentication: none, password, public key, host-based, and
  keyboard-interactive methods
- Algorithms: ECDH/DH key exchange, RSA/ECDSA public keys,
  AES-256-GCM/CBC/CTR encryption, HMAC-SHA2-256/512 with ETM variants
- Key import/export: PKCS#1, PKCS#8, SEC1, OpenSSH, SSH2, JWK formats
- TCP client/server wrappers with port forwarding service
- Session reconnection and piping (relay) support
- Non-blocking per-channel request dispatch via goroutines
- Keep-alive, metrics, custom message handlers, service activation
- ~679 unit tests, ~158 integration tests, ~48 E2E tests
- Cross-implementation interop validated against C# and TypeScript

* feat: Cross-platform benchmark suite for C#, TypeScript, and Go

Add a comprehensive benchmark suite that measures SSH library performance
across all three platform implementations with unified JSON output and
automated cross-platform comparison reporting.

- Define common JSON result schema for consistent cross-platform output
- Add C# benchmarks: encryption, HMAC, KEX, keygen, signatures, serialization
- Add TypeScript benchmarks: matching C# coverage with Node.js harness
- Add Go benchmarks: native Go SSH implementation benchmarks
- Build orchestrator script to coordinate builds, runs, and reporting
- Build Markdown report generator with trimmed-mean statistics
- Integrate benchmarks into the build system
- Add bench/README.md documenting usage and architecture

* feat: Add verification tests, fix transport parity, and improve throughput benchmarks

- Add --verify flag across all platforms (C#, TS, Go) with correctness checks
  for every benchmark category (encryption, HMAC, KEX, keygen, signatures,
  serialization, session setup, throughput, port forward, reconnect)
- Fix Go benchmarks to use TCP loopback (matching C#/TS) instead of io.Pipe
- Fix C#/TS throughput benchmarks to reuse sessions across runs with safety
  timeouts, eliminating backpressure deadlocks on large encrypted messages
- Align throughput duration to 2s across all platforms
- Add report validation warnings for zero/negative metrics and >10x cross-
  platform divergence, with 200x threshold for algorithm categories
- Document expected Go performance advantages and verification coverage in README

* perf: Optimize Go SSH throughput, session setup, and dispatch-loop pipelining

Throughput (~1.9x encrypted small-message improvement, 23.7K → 45.2K msgs/s):

- Reuse GCM cipher buffers (aadBuf, sealBuf, tag) instead of allocating
  per-message; Sign() and SetTag() now avoid copies
- Coalesce packet + MAC into single Write() call by pre-allocating packet
  buffer with extra capacity for the MAC
- Reuse HMAC input buffers (sendHmacBuf/recvHmacBuf) across messages
- Add sync.Pool for packet buffers with security zeroing before return
- Add reusable SSHDataWriter for direct message serialization (eliminates
  ToBuffer() double-serialization)
- Use math/rand for unencrypted padding instead of crypto/rand
- Replace session.mu lock in SendMessage with atomic connected flag

Session setup (latency-sensitive setup reduced from ~1929ms to ~710ms):

- Fire-and-forget ServiceRequest in Authenticate() (skip ServiceAccept wait)
- Synchronize post-KEX goroutines (extensionInfo, reconnect) before auth
  to avoid sendMu contention that added ~200ms per round-trip
- Inline NewKeys, ExtensionInfo, and reconnect-enable sends from the
  dispatch loop instead of spawning goroutines, matching C#/TS behavior
- Pipeline version string + KexInit in Connect() (send KexInit before
  reading remote version per RFC 4253, saving one round-trip)
- Add 64KB bufio.Writer to reduce write syscall overhead

Reconnect reliability:

- Add reconnectInfoReady channel so Reconnect() waits for the remote's
  enable-reconnect to be processed before returning
- Explicitly cache the enable-reconnect message in enableReconnect() to
  solve the chicken-and-egg where the message that triggers caching wasn't
  itself cached

Benchmark verification:

- Add key size assertions to keygen and signature verification across all
  three platforms (Go, C#, TS) to catch parameter mismatches
- Fix Go keygen benchmark that was ignoring the key size parameter
- Fix data race on kexService.exchanging (bool → atomic int32)
- Rewrite latencyStream with queue-based single-drainer for non-blocking
  writes matching C#/TS behavior

Tests:

- Add unit tests for SSHDataWriter.Slice(), GenerateKeyPairWithSize(),
  sendMessageDirect(), buffer reuse with varying payload sizes, GCM/HMAC
  aliasing contracts
- Fix gcm_tampering_test to copy tag before mutating
- Fix asyncPipeRWC.Write() race condition (pre-check closeCh)

* refactor: Move tests around

* feat: Add e2e tests for signals, extended data, rekey, and keyboard-interactive auth

Add 4 new e2e test suites (12 tests) covering SSH features that previously
had only unit test coverage: exit-status/exit-signal/standalone signals,
SendExtendedData (stderr) bidirectional flow, key rotation with low threshold,
and multi-round-trip keyboard-interactive authentication.

* fix: address PR #123 review  security, correctness, robustnessfeedback

Address all actionable review comments from PR #123 code review:

Security & correctness:
- Upgrade Go from 1.17 to 1.21 for crypto security fixes
- Migrate ECDH from deprecated elliptic.ScalarMult to crypto/ecdh
  (constant-time, automatic IsOnCurve validation)
- Fix bcrypt magic string comments (OxychromaticBlowfishSwatDynamite)
- Add DefaultPbkdf2Iterations const alongside mutable var

Session lifecycle:
- Refactor close()/closeWithError() into unified closeImpl()
  fixing reconnect check, closeMetrics, and custom error bugs
- Move done/kexDone channel init under session mutex
- Restore currentAlgorithms in reconnect restoreState

Channel robustness:
- Derive request contexts from session done channel
- Log window adjust send failures
- Add accept queue bound (64) with resource shortage rejection
- Use SendMessage for channel-failure replies (disconnected-buffer)
- Add DroppedRequests counter to ChannelMetrics

Protocol:
- Fix reconnect cache memory leak with slice compaction

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: replace synchronous io.Pipe with async-buffered writes in TCP tests

The TCP e2e tests deadlocked because createDuplexStreams used raw
io.Pipe which has no write buffering. During SSH Connect, both sides
pipeline version string + kexInit writes in a goroutine, then wait for
the writes to finish before starting the dispatch loop. With unbuffered
pipes, both goroutines block on Flush (pipe Write) waiting for readers
that haven't started  classic deadlock.yet

Replace duplexStream with asyncPipeRWC (matching the ssh package tests)
which buffers writes through a channel + background goroutine, simulating
the kernel buffer behavior of real TCP sockets.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: capture protocol under lock in closeImpl (port C# PR #132)

Port the race condition fix from C# PR #132:
#132

In closeImpl(), s.protocol was accessed outside the mutex (after unlock
at the end of the guard block). A concurrent reconnect could replace
s.protocol between that unlock and the subsequent use at lines 495/512,
causing the close to disconnect/close the wrong (new) connection.

Fix by capturing proto := s.protocol inside the lock, then using that
captured reference for the disconnect send and protocol close, matching
the C# fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Reacting to feedback

* Help tests pass

* Remove binaries

* Fix flaky Go test deadlocks and latency measurement timeouts

- Fix deadlock in session.Connect(): close s.done channel via defer if
  Connect() returns before the dispatch loop starts, preventing closeImpl()
  from blocking forever on <-s.done.

- Fix TestMeasureSessionLatency: increase context timeout from 10s to 30s
  to accommodate -race overhead on slow CI machines (especially Windows).

- Fix TestRecordSessionContour: add WaitUntilReconnectEnabled() call before
  sending data, ensuring the reconnect extension is fully enabled on both
  sides so latency info is appended to messages.

- Fix TestClosedSessionHasNoLatency: increase context timeout from 10s to
  30s for consistency with other latency tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: David Obando <davidobando@github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: David Obando <daobando@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants