Fix CI-MACOS flakiness in connection-timeout tests by giving login phase adequate headroom - #2953
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2953 +/- ##
=========================================
Coverage 60.81% 60.81%
+ Complexity 4974 4969 -5
=========================================
Files 151 151
Lines 35223 35223
Branches 5900 5900
=========================================
+ Hits 21421 21422 +1
+ Misses 10934 10931 -3
- Partials 2868 2870 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts several connection-timeout-related tests to be less flaky on slower CI agents (notably macOS hosted runners) by increasing loginTimeout headroom (and related timing windows) so login-phase socket deadlines don’t expire before expected server responses arrive.
Changes:
- Increased
loginTimeoutin multiple tests to reduce macOS CI “Read timed out” failures during the login phase. - Updated timing bounds / delays in tests to align with longer login headroom and preserve the original assertions’ intent.
- Added clarifying comments explaining why longer login headroom is needed for CI.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/test/java/com/microsoft/sqlserver/jdbc/resiliency/ReflectiveTests.java | Raises loginTimeout and adjusts the expected timing bound in the default retry resiliency test. |
| src/test/java/com/microsoft/sqlserver/jdbc/connection/TimeoutTest.java | Increases loginTimeout and WAITFOR DELAY to validate post-login unlimited socket timeout on slower agents. |
| src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java | Adds explicit loginTimeout=30 to retry connection strings to avoid login-phase read timeouts during 4060 round-trips. |
Divang Sharma (divang)
approved these changes
May 18, 2026
Muskan Gupta (muskan124947)
approved these changes
May 18, 2026
Mahendra Chavan (machavan)
approved these changes
May 19, 2026
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
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.
Fix CI-MACOS flakiness in connection-timeout tests: Three tests were consistently failing on the MacOS CI pipeline with "Read timed out" errors while passing on Linux and Windows:
Root cause
All three tests assumed that prelogin + TLS handshake + LOGIN7 could complete within a short loginTimeout (5s, or the default 15s). Hosted MacOS runners are noticeably slower than Linux/Windows agents, so the JDBC socket-read deadline expired before the server's clean response arrived. The thrown SocketTimeoutException either bypassed the inner
assertion (ReflectiveTests) or produced an unexpected error message (ConfigurableRetryLogicTest), and in TimeoutTest the login phase itself timed out before the post-login behavior could be verified.
This was not caused by a driver regression. PR #2927 (IOBuffer login socket timeout fix) correctly tightened login-phase socket deadlines; the tests just had insufficient headroom to ride out slow CI agents.
Fixes
ReflectiveTests.testDefaultRetry
loginTimeout: 5 -> 15 (allow initial getConnection on slow agents)
bound: 6000 -> 32000 ms
Bound now matches the actual retry math: (retryCount + 1) * loginTimeout + slack = 0 + 15s + 5s = 20s. Tight enough to catch an accidental extra retry.
The previous 6000ms bound was inconsistent with retry semantics and only passed before #2927 because the login socket lacked a deadline.
ConfigurableRetryLogicTest.connectionTimingTest
Added loginTimeout=20 to all three retry connection strings so the TDS 4060 "cannot open database" round-trip always completes before the JDBC socket read deadline, even on slow CI agents.
TimeoutTest.testDefaultSocketTimeoutUnlimitedAfterLogin
loginTimeout: 5s -> 15s
WAITFOR DELAY: 8s -> 20s
The test verifies post-login socket behavior; raising both keeps the WAITFOR safely above loginTimeout while giving the login phase enough headroom to complete on any CI agent.
These changes are platform-agnostic - no @DisabledOnOs gates, no OS-specific branches, no relaxed error-message matching. The under-test assertions remain intact and will still fail fast on real regressions.