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 owners August 19, 2026 18:40
@gjtorikian
gjtorikian requested review from dandorman and removed request for a team August 19, 2026 18:40
@gjtorikian gjtorikian closed this Aug 19, 2026
@greptile-apps

greptile-apps Bot commented Aug 19, 2026

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

Filename Overview
lib/workos/base_client.rb Adds 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.rb Adds focused pooled-connection tests, an end-to-end aborted-request regression test, and deterministic cache cleanup between tests.
.github/workflows/release-please.yml Retargets 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 free to 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