test: rewrite stale-keepalive retry test without real socket dependence#611
Merged
Conversation
The original integration-style test spun up a real uvicorn server with timeout_keep_alive=0, established a connection, slept 100 ms to give the server time to close the socket, monkeypatched urllib3's is_connected property to lie once, then asserted the resulting ConnectionError caused HTTPClient to retry. The assertion was flaky on Linux + Python 3.14 + latest deps, where either the FIN didn't arrive in 100 ms reliably or newer urllib3 transparently refreshed the stale connection — in both cases the first request succeeded without raising, so HTTPClient's retry never fired and mock.call_count came in at 1 instead of 2. That brittleness is testing real-socket timing and urllib3 internals, not HTTPClient's contract. The contract is just: "if session.request raises ConnectionError, retry once". Inject the ConnectionError at the session layer directly — same assertion, no environment dependence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jpbrodrick89
requested review from
apaleyes,
dionhaefner and
xalelax
as code owners
May 27, 2026 09:53
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #611 +/- ##
==========================================
- Coverage 77.08% 67.11% -9.97%
==========================================
Files 32 32
Lines 4495 4495
Branches 739 739
==========================================
- Hits 3465 3017 -448
- Misses 728 1237 +509
+ Partials 302 241 -61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@dionhaefner I appreciate this changes the test's behaviour a lot, there's a chance it was just flaky and a rerun might have allowed it to pass, let me know your preference how best you'd like this handled. |
dionhaefner
approved these changes
May 27, 2026
dionhaefner
left a comment
dionhaefner
left a comment
Contributor
There was a problem hiding this comment.
Fine with me, thanks!
dionhaefner
enabled auto-merge (squash)
May 27, 2026 12:59
dionhaefner
disabled auto-merge
May 27, 2026 13:00
dionhaefner
changed the title
dionhaefner
enabled auto-merge (squash)
May 27, 2026 13:00
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
`test_stale_keepalive_connection_is_handled` was failing intermittently — specifically on `tests-base (ubuntu-latest, 3.14, latest)` — with `assert 1 == 2` (only one call to `session.request`, not the expected two-call retry sequence).
The original test was integration-style: it spun up a real uvicorn server with `timeout_keep_alive=0`, established a connection, slept 100 ms to give the server time to FIN, monkeypatched urllib3's `is_connected` property to lie once, then asserted that the resulting `ConnectionError` caused `HTTPClient` to retry.
On Linux + Python 3.14 + latest deps, the first request succeeded instead of raising — either uvicorn's FIN didn't arrive in the 100 ms window on that platform, or newer urllib3 transparently refreshed the stale connection before `requests.Session.request` returned. Either way, the retry path never fired and `mock.call_count` came in at 1 rather than 2. (macOS 3.14 latest and Linux 3.10 latest both still pass.)
That brittleness is a symptom of the test exercising real-socket timing and urllib3 internals, not `HTTPClient`'s actual contract. The contract is simply: "if `session.request` raises `ConnectionError`, retry once". Inject the `ConnectionError` at the session layer directly — same assertion (`call_count == 2`, correct result returned), no environmental dependence.
Test plan
🤖 Generated with Claude Code