Skip to content

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

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

fix: evict pooled connection when a request does not complete (8.0.x)#539
gjtorikian wants to merge 2 commits into
v8.0.xfrom
security/evict-connection-v8.0.x

Conversation

@gjtorikian

Copy link
Copy Markdown
Contributor

Backport to the 8.0 series (targets a v8.0.2 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 v8.0.x with target-branch: v8.0.x, so merging this PR opens the 8.0.2 release PR automatically.

Verification

  • Full test/workos/test_base_client.rb suite passes across multiple seeds (20 runs, 37 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 nicknisi and removed request for a teamAugust 19, 2026 18:40
@greptile-apps

Copy link
Copy Markdown
Contributor

Greptile Summary

This backport makes incomplete HTTP requests evict their pooled sockets, adds regression coverage for exceptional and asynchronous abort paths, and retargets release automation to the v8.0.x maintenance branch.

  • Wraps Net::HTTP#request with failure-path connection cleanup.
  • Adds unit and real-socket regression coverage plus test cache teardown.
  • Configures release-please to create v8.0.x release PRs.

Confidence Score: 4/5

The PR appears safe to merge, with a non-blocking opportunity to limit failure cleanup to the specific timeout-keyed connection that failed.

The incomplete-request socket is reliably removed and the release/test changes align with the backport, but endpoint-wide eviction can unnecessarily discard other healthy pooled connections.

Files Needing Attention: lib/workos/base_client.rb

Important Files Changed

FilenameOverview
lib/workos/base_client.rbCorrectly evicts potentially desynchronized sockets on all incomplete exits, but cleanup also closes healthy timeout-specific sibling connections for the endpoint.
test/workos/test_base_client.rbAdds focused stub and real-socket coverage for eviction and connection-retention behavior, with cache cleanup between tests.
.github/workflows/release-please.ymlRetargets release-please triggers and PR creation to the v8.0.x maintenance branch.

Sequence Diagram

sequenceDiagram
participant App
participant Client as BaseClient
participant Pool as Connection pool
participant API
App->>Client: execute_request
Client->>Pool: connection_for(base, timeout)
Pool-->>Client: cached Net::HTTP
Client->>API: request
API--xClient: request aborted before response returns
Client->>Pool: evict_connection(base)
App->>Client: next request
Client->>Pool: connection_for(base, timeout)
Pool-->>Client: fresh connection
Loading
Prompt To Fix All With AI
### Issue 1
lib/workos/base_client.rb:153
**Eviction drops healthy sibling connections**
When the same fiber has connections to this endpoint cached with different timeout values, an incomplete request calls `evict_connection(base)`, which closes every timeout-specific entry rather than only the failed socket. Subsequent requests using those healthy entries must reconnect, adding connection and TLS setup latency.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

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

# socket handed back to the pool desyncs the *next* request on this
# thread, so drop it here rather than in the rescue.
evict_connection(base) unless request_completed
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2Eviction drops healthy sibling connections

When the same fiber has connections to this endpoint cached with different timeout values, an incomplete request calls evict_connection(base), which closes every timeout-specific entry rather than only the failed socket. Subsequent requests using those healthy entries must reconnect, adding connection and TLS setup latency.

Knowledge Base Used:Core client infrastructure

Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/workos/base_client.rb
Line: 153
Comment:
**Eviction drops healthy sibling connections**
When the same fiber has connections to this endpoint cached with different timeout values, an incomplete request calls `evict_connection(base)`, which closes every timeout-specific entry rather than only the failed socket. Subsequent requests using those healthy entries must reconnect, adding connection and TLS setup latency.
**Knowledge Base Used:**[Core client infrastructure](https://app.greptile.com/workos/-/custom-context/knowledge-base/workos/workos-ruby/-/docs/core-client.md)---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@gjtorikian
gjtorikian deleted the security/evict-connection-v8.0.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