fix: evict pooled connection when a request does not complete (7.1.x) - #538
Closed
gjtorikian wants to merge 2 commits into
Closed
fix: evict pooled connection when a request does not complete (7.1.x)#538gjtorikian wants to merge 2 commits into
gjtorikian wants to merge 2 commits into
Conversation
Wrap Net::HTTP#request in begin/ensure so that any exit other than a returned response — including exceptions outside StandardError such as an application-level Timeout.timeout or Thread#kill — removes and closes the cached keep-alive connection instead of leaving it mid-stream for the next request on the same thread to pick up. Adds a real-socket regression test plus StubConnection-based coverage of the evict/keep paths, and a teardown that clears the fiber-local connection cache between tests.
Contributor
Greptile SummaryThis backport prevents incomplete HTTP requests from leaving desynchronized keep-alive sockets in the fiber-local connection pool and configures release automation for the v7.1.x maintenance branch.
Confidence Score: 5/5The PR appears safe to merge, with the incomplete-request cleanup, retry lifecycle, regression coverage, and maintenance-branch release targeting remaining internally consistent. Incomplete requests now remove their cached socket before control propagates or retries, completed requests retain their connection, repeated eviction is harmless, and the branch-local workflow change does not disrupt main's release automation. Important Files Changed
|
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.
Backport to the 7.1 series (targets a
v7.1.3patch release via release-please on this branch).What changed
lib/workos/base_client.rb: wrapNet::HTTP#requestinbegin/ensureand evict the cached keep-alive connection unless the request completed. Any exit other than a returned response — a connection error the rescue below knows about, one it doesn't, or a non-local exit such as an application-levelTimeout.timeoutorThread#kill— now removes and closes the pooled socket instead of leaving it mid-stream for the next request on the same thread.test/workos/test_base_client.rb: adds a real-socket regression test (test_aborted_request_does_not_leak_its_response_to_the_next_request),StubConnection-based coverage of the evict/keep paths, and ateardownthat clears the fiber-local connection cache between tests (this branch predates theThread.current[]storage change in v9.1.0, so the teardown clearsFiber[:workos_connections])..github/workflows/release-please.yml: trigger release-please on pushes tov7.1.xwithtarget-branch: v7.1.x, so merging this PR opens the 7.1.3 release PR automatically.Verification
test/workos/test_base_client.rbsuite passes across multiple seeds (11 runs, 27 assertions).standardrbclean on both changed files.