Repository navigation
test: remove TLS handshake-limit counter race from ConnectionAdmissionTests - #234
Conversation
…nTests The client observing EOF on the rejected connection is not a sync point for the server accounting: the connection lease is released only after terminal cleanup completes (#162 / PR #209 design), so ActiveConnections may transiently still be 2 when the assertion runs. Replace the immediate assertion with YieldUntilAsync convergence on ActiveConnections == 1, keeping the immediate-close verification via ReadUntilClosedAsync and the stable ActiveHandshakes == 1 check. Baseline reproduction (dev @ 25f7cc4, macOS arm64): 114/150 iterations failed with the same failure as PR #227 CI (run 31996872168). With the fix the local single-test loop passes 150/150. Closes #229
SunSi12138
left a comment
There was a problem hiding this comment.
Reviewed against current dev and issue #229. No blocking findings.
The change correctly separates the two assertions: client-side EOF proves the over-handshake-limit connection is rejected promptly, while ActiveConnections == 1 is now awaited as a server-side post-terminal-cleanup invariant. This matches the connection-admission lease lifecycle introduced in #209, where the connection lease is released only after transport terminal cleanup completes. The existing deterministic ConnectionSlotStaysHeldUntilTerminalCleanupCompletes test already covers the release ordering directly, so an additional TLS-specific test seam is not required for this fix.
PR Quick and CodeQL both pass on head 2d31d9ac3b0cbd7d883d309c563b3a7283e6b026.
Summary
Fixes the
TlsHandshakeLimitRejectsTheSecondConnectionWhileTheFirstStallscounter race from #229 (first hit on PR #227 CI run31996872168).The client observing EOF on the rejected connection is not a sync point for the server accounting: per the #162 / PR #209 design the connection lease is released only after terminal cleanup completes, so
ActiveConnectionscan legitimately still be2when the client-side close is observed. The immediateIsEqualTo(1)assertion therefore asserted on a transient state.Change
test/SharpLink.UnitTests/Server/ConnectionAdmissionTests.cs(test-only, +6/−1):ReadUntilClosedAsync+Ensure), and (2) terminal cleanup converges the connection accounting to 1.Task.Delaymasking.Why not the optional test seam
The issue lists the deterministic test seam as optional ("若能加"). The existing
ConnectionSlotStaysHeldUntilTerminalCleanupCompletesalready proves release-after-terminal-cleanup deterministically via a dispose gate on the scripted transport path; adding a gate to the real TLS socket listener would mean product test hooks for a semantic already covered.Validation
25f7cc4, macOS arm64, Release): single-test loop 150 iterations → 114 failed with the exact CI failure (Expected to be 1 but found 2, line 520).ConnectionAdmissionTestsclass loop 20/20 pass.ProtocolHandshakeTimeoutReleasesBothSlots, a pre-existing, unrelated machine-speed-dependent test that fails identically on baseline dev (10/10 on the same host; passes on the Mac 20/20 and on GitHub's 4-core runners). Root cause: its wait caps at 4000Task.Yieldpolls (~190 ms wall time on the 32-core host), which can outrun its own 200 ms handshake-timeout timer. Suggested as a separate follow-up.quickgates this PR as usual.Closes #229