Skip to content

fix: evict pooled connection when a request does not complete (7.1.x) - #538

Closed
gjtorikian wants to merge 2 commits into
v7.1.xfrom
security/evict-connection-v7.1.x
Closed

fix: evict pooled connection when a request does not complete (7.1.x)#538
gjtorikian wants to merge 2 commits into
v7.1.xfrom
security/evict-connection-v7.1.x

Conversation

@gjtorikian

Copy link
Copy Markdown
Contributor

Backport to the 7.1 series (targets a v7.1.3 patch release via release-please on this branch).

What changed

  • lib/workos/base_client.rb: wrap Net::HTTP#request in begin/ensure and 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-level Timeout.timeout or Thread#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 a teardown that clears the fiber-local connection cache between tests (this branch predates the Thread.current[] storage change in v9.1.0, so the teardown clears Fiber[:workos_connections]).
  • .github/workflows/release-please.yml: trigger release-please on pushes to v7.1.x with target-branch: v7.1.x, so merging this PR opens the 7.1.3 release PR automatically.

Verification

  • Full test/workos/test_base_client.rb suite passes across multiple seeds (11 runs, 27 assertions).
  • Without the lib change, the new regression test fails with the second request reading the first request's abandoned response.
  • standardrb clean on both changed files.

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.
@gjtorikian
gjtorikian requested review from a team as code ownersAugust 19, 2026 18:40
@gjtorikian
gjtorikian requested review from dandorman and removed request for a teamAugust 19, 2026 18:40
@greptile-apps

Copy link
Copy Markdown
Contributor

Greptile Summary

This 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.

  • Evicts and closes a pooled connection whenever Net::HTTP#request exits without returning a response.
  • Adds unit and real-socket regression coverage for aborted, failed, and completed requests.
  • Clears cached test connections during teardown.
  • Retargets release-please to produce v7.1.x patch releases.

Confidence Score: 5/5

The 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

FilenameOverview
lib/workos/base_client.rbAdds ensure-based eviction for requests that do not return, preventing reuse of potentially desynchronized sockets without changing successful connection reuse.
test/workos/test_base_client.rbAdds focused pooled-connection tests, an end-to-end aborted-request regression test, and deterministic cache cleanup between tests.
.github/workflows/release-please.ymlRetargets this maintenance branch's release automation from main to v7.1.x.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Acquire pooled connection] --> B[Issue Net::HTTP request]
B -->|Response returned| C[Mark request completed]
C --> D[Keep connection pooled]
B -->|Exception or non-local exit| E[Ensure block]
E --> F[Evict and close connection]
F --> G{Handled connection error?}
G -->|Yes, retries remain| A
G -->|Yes, exhausted| H[Raise APIConnectionError]
G -->|No| I[Propagate original exit]
Loading

Reviews (1): Last reviewed commit: "ci: run release-please against the v7.1...." | Re-trigger Greptile

@gjtorikian
gjtorikian deleted the security/evict-connection-v7.1.x branch August 19, 2026 18:53
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@gjtorikian