Skip to content

tests: fix TlsPreferTest flap on net481#1108

Merged
mtmk merged 2 commits into
mainfrom
fix-tls-prefer-test-flap-net481
Apr 14, 2026
Merged

tests: fix TlsPreferTest flap on net481#1108
mtmk merged 2 commits into
mainfrom
fix-tls-prefer-test-flap-net481

Conversation

@mtmk

@mtmk mtmk commented Apr 14, 2026

Copy link
Copy Markdown
Member

The server task in Prefer_mode_attempts_tls_upgrade_when_server_advertises_tls_available raced the client's failed TLS-handshake socket teardown: a FIN surfaced as ReadLineAsync returning null, while an RST threw IOException. On net481/Windows the reset path is common, producing the intermittent failure. Detect TLS positively from the first byte (0x16 Handshake record) instead of inferring from how the socket closed.

The server task's ReadLineAsync races the client's failed TLS-handshake
socket teardown: a FIN surfaces as null, an RST throws IOException. On
net481/Windows the reset path is common. Detect TLS positively from the
first byte (0x16 = Handshake record) instead of inferring from how the
socket closed.
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 2m 15s —— View job


PR Review

  • Gather context and read changed files
  • Analyze the fix for correctness
  • Check for async/await issues
  • Post review

Approach is sound. Detecting TLS positively from the first byte (0x16 = TLS record ContentType Handshake) is protocol-correct for TLS 1.0, 1.2, and 1.3 (all use 0x16 0x03 0x01 for the outer ClientHello record), and cleanly avoids the race between a FIN (ReadLineAsync → null) and an RST (IOException) that caused the net481 flap.

One issue worth fixing:

catch (IOException) at line 230 does not catch OperationCanceledException. If the 10-second CTS fires while stream.ReadAsync(…, cts.Token) is in-flight (slow CI agent, etc.), the OCE propagates uncaught from the lambda, serverTask faults, and await serverTask (line 275) re-throws — producing a confusing failure rather than a clean timeout. An inline suggestion has been left on the exact line.

Minor observation:

using var sr = new StreamReader(stream, encoding) in the 'C' (plaintext) branch (line 243) will dispose the underlying NetworkStream when sr goes out of scope, before the outer using var tcp does. This is harmless in practice (double-dispose of NetworkStream is a no-op and tcp closes immediately after), but new StreamReader(stream, encoding, leaveOpen: true) would be more precise if this branch is ever expanded.

@Jarema Jarema left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT!

int read;
try
{
read = await stream.ReadAsync(firstByte, 0, 1, cts.Token);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only IOException is caught here, but stream.ReadAsync(…, cts.Token) can also throw OperationCanceledException when the 10-second CTS fires. If the test is slow (e.g. on a loaded CI agent) and the read hasn't completed before the timeout, the OCE propagates uncaught from the lambda, serverTask faults, and await serverTask on line 275 re-throws — producing a confusing test-failure message instead of a clean timeout.

Consider catching both, or catching the base type:

Suggested change
read = await stream.ReadAsync(firstByte, 0, 1, cts.Token);
try
{
read = await stream.ReadAsync(firstByte, 0, 1, cts.Token);
}
catch (Exception ex) when (ex is IOException or OperationCanceledException)
{
output.WriteLine($"[SERVER] RCV failed: {ex.Message}");
read = 0;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would still be confusing. not worth it.

Default ConnectTimeout is 2s, which is too tight for the fake-server
handshake on slow CI runners (net481/Windows). Raise it to 10s to
match the surrounding CancellationTokenSource.
@mtmk
mtmk merged commit 33f69a4 into main Apr 14, 2026
22 checks passed
@mtmk
mtmk deleted the fix-tls-prefer-test-flap-net481 branch April 14, 2026 13:41
This was referenced Apr 16, 2026
@mtmk mtmk mentioned this pull request May 13, 2026
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.

3 participants