Conversation
…4943] Faraday/net_http_persistent reuses dead sockets and surfaces SSL EOF, RST, or ReadTimeout-on-closed-socket as TransportError. Match Go net/http by retrying unused idle connections once with no backoff, including POST. DNS failures and real read timeouts are not retried. Co-authored-by: Cursor <[email protected]>
SSLError and ConnectionFailed also cover cert failures and connection refused. Retry once only when the message says the pooled socket is already dead. Co-authored-by: Cursor <[email protected]>
itsmeadi
changed the title
| handle_response(response) | ||
| rescue Faraday::Error => e | ||
| error = TransportError.new("Request failed: #{e.message}", error_type: ErrorMapping.classify_faraday_error(e)) | ||
| if stale_retries.zero? && ErrorMapping.stale_keep_alive?(e) |
There was a problem hiding this comment.
This retries every HTTP method after errors that can arrive while reading the response. The server may already have applied a POST, PATCH, or DELETE, so the retry can repeat a write. I reproduced a POST whose complete body reached the server twice. Restrict automatic retries to safe methods, or require a server enforced idempotency key for writes.
| STALE_KEEP_ALIVE_PATTERN = / | ||
| connection\ reset\ by\ peer | ||
| |unexpected\ eof | ||
| |tcpsocket:\(closed\) |
There was a problem hiding this comment.
TCPSocket:(closed) is also the normal message after a real Net::ReadTimeout because Net::HTTP closes the socket before Faraday formats the exception. A local real timeout sent two requests through this branch. Remove this match or detect a stale pooled socket without using the closed socket suffix.
| error = TransportError.new("Request failed: #{e.message}", error_type: ErrorMapping.classify_faraday_error(e)) | ||
| if stale_retries.zero? && ErrorMapping.stale_keep_alive?(e) | ||
| log_retry_attempt(method, path, error, stale_retries, started) | ||
| stale_retries += 1 |
There was a problem hiding this comment.
stale_retries does not increment attempt, so this retry sits outside retry_config.max_attempts. With max_attempts: 3, I reproduced four requests. Count this retry in the same total attempt budget.
| Detected from the error message, not Faraday class (so cert failures and | ||
| connection refused are not retried). Applies to POST as well as GET; DNS | ||
| failures and real read timeouts are not retried. No backoff; the opt-in | ||
| `retry_config:` policy is unchanged. |
There was a problem hiding this comment.
README.md still says retries are opt in, disabled clients make exactly one attempt, and writes are never retried. This change makes one retry automatic and retries writes. Update the public retry section to describe the new default.
| |broken\ pipe | ||
| |connection\ is\ closed | ||
| |end\ of\ file\ reached | ||
| |tls_retry_write_records |
There was a problem hiding this comment.
Four accepted messages have no tests: broken pipe, connection is closed, end of file reached, and tls_retry_write_records. The wrapped exception message branch is also only tested for DNS. Add table cases for every accepted message, including one stale message from wrapped_exception.
2 participants
Ticket
Summary
Follow-up to #78 (
idle_timeout55→25). PrizePicks still hitGetStreamRuby::TransportErroron reused keep-alive sockets (SSL_readEOF,Connection reset by peer,Net::ReadTimeouton a closedTCPSocket). Faraday/net_http_persistentdoes not retry those; Gonet/httpdoes for unused idle connections.Retry the request once, with no backoff, when the failure looks like a dead pooled socket. Applies to POST as well as GET. DNS failures and real read timeouts are not retried. The opt-in
retry_config:policy is unchanged.Previous PR
Checklist
./docs) has been updatedNotes for review
Client#requestbefore the retry loop).Made with Cursor