Skip to content

Fix pooled connections raising errors on requests that never failed - #11

Open
oshboul wants to merge 5 commits into
ahoshaiyan:mainfrom
oshboul:fix-connection-pool-error-handling
Open

oshboul wants to merge 5 commits into
ahoshaiyan:mainfrom
oshboul:fix-connection-pool-error-handling

Conversation

@oshboul

@oshboul oshboul commented Sep 15, 2026

Copy link
Copy Markdown

Running this gem in production behind a payment gateway, we saw a steady stream
of ConnectionResetError and ReadTimeoutError that didn't correspond to
anything wrong upstream. Tracking them down surfaced four defects in connection
pooling. One commit each, reviewable independently.

1. A connection probe could raise out of the pool

PoolEntry#healthy? asks the socket whether the peer is still there. When a peer
resets a connection, eof? raises Errno::ECONNRESET rather than returning
true, and nothing caught it. That escaped out of acquire (as an error on a
request not yet sent), out of release (replacing an already successful response
from inside ensure), and out of clean_pool_unsafe! — which reaps via
to_clean?, so one dead connection broke cleanup for the whole pool and failed
requests to unrelated healthy hosts
.

The rescue is scoped to what a socket probe can raise. Timeout::Error is
excluded on purpose: Rack::Timeout raises it asynchronously and swallowing it
would defeat the caller's own timeout.

2. Dead connections weren't reaped while any connection was in flight

Cleanup ran only when every entry was reapable (all?), so one in-flight
connection blocked removal of every dead one beside it and a full pool could
never make room. Separately, release! never updated @last_used, so idle time
was measured from when a request started — any request slower than :idle_ttl
left its connection already expired on return.

3. Stale keep-alive connections had no recovery path

keep_alive_timeout was pinned at 30 days, disabling Net::HTTP's own staleness
check, and max_retries was 0. It's now derived from the pool's :idle_ttl so
both layers agree.

max_retries stays 0 deliberately — Net::HTTP's retry list includes
Net::ReadTimeout, which would double the worst case latency of a slow request.
Instead with_client replays once, guarded: only connections that came
started out of the pool, only once, only idempotent methods, and never when
the request carries files
(Net::HTTP re-encodes multipart bodies from source
IOs that are at EOF after the first attempt — replaying would ship a truncated
body with a matching Content-Length and the server would accept it with a 200).
Every error in the retry list fails immediately, so the replay costs ~1ms.

4. Seven network faults escaped as raw Ruby exceptions

rescue MintHttp::Error never caught them:

raw now
Errno::ECONNABORTED ConnectionAbortedError — declared but never raised until now
Errno::ENETRESET ConnectionResetError
Net::HTTPBadResponse ConnectionIoError — how a desynced keep-alive connection presents
ETIMEDOUT, EHOSTDOWN, ENETDOWN, EADDRNOTAVAIL ConnectionError

Pool exhaustion raised a bare RuntimeError, bypassing every rescue MintHttp::.
Now PoolTimeoutError, placed under Error rather than ConnectionError — a
saturated pool isn't a network fault and shouldn't be retried as one.

Tests

13 new tests across 4 files, in the existing local-TCPServer style. Every one
was checked against 77118f2 to confirm it fails without the fix. Three assert
that something doesn't happen (fresh connections aren't replayed, requests with
files are never replayed, probe bugs aren't swallowed) — those pass on the old
code too and guard the guards rather than the fixes.

Suite: 25 runs, 73 assertions, 0 failures (was 12/39).

New fake servers bind to port 0 instead of hardcoded ports — nine across four
files would otherwise collide. Happy to match the existing convention if you'd
rather.

Note for reviewers

PoolTimeoutError is new public API, and code rescuing those Errno classes
directly around a MintHttp call will stop matching them — probably worth a minor
bump. No version bump included; that looked like your call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant