Repository navigation
v5.7.4 - Retry stale connection resets in client - #47
Conversation
Update `Parse::Client#request` to handle `Faraday::ConnectionFailed` by distinguishing transient reset-style failures from hard failures like refused or DNS errors. Reset causes (`ECONNRESET`, `EPIPE`, `ECONNABORTED`, `EOFError`, plus message fallback) now retry under existing idempotency rules and raise `Parse::Error::ConnectionError` when retries are exhausted, while non-reset failures still fail fast as raw `Faraday::ConnectionFailed`. Also bumps the gem to 5.7.4, adds changelog notes, and expands retry tests to cover reset, EOF, message-only reset detection, and POST idempotency behavior.
8b87188 to
8158ad7
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The behavior change is scoped, aligns with the PR description, and is backed by targeted regression tests; remaining feedback is non-blocking maintainability/documentation polish.
Pull request overview
This PR updates Parse::Client#request retry behavior to treat stale keep-alive “reset” failures as transient and retryable (under existing idempotency rules), while keeping non-transient connection failures (e.g., refused/DNS-style) fail-fast. It also bumps the gem version to 5.7.4 and documents the change.
Changes:
- Add a dedicated
Faraday::ConnectionFailedrescue path that retries only reset-style failures for idempotent requests and raisesParse::Error::ConnectionErrorwhen the retry budget is exhausted. - Add/reset-specific test coverage for GET/POST idempotency behavior (including EOF and message-only reset detection).
- Bump version to 5.7.4 and add changelog notes.
File summaries
| File | Description |
|---|---|
lib/parse/client.rb |
Implements reset-vs-non-reset handling for Faraday::ConnectionFailed and adds reset detection helper/constants. |
test/lib/parse/client/retry_test.rb |
Expands retry tests to cover reset-style connection failures and idempotency gating. |
lib/parse/stack/version.rb |
Bumps library version to 5.7.4. |
Gemfile.lock |
Updates locked gem version to 5.7.4. |
CHANGELOG.md |
Documents the new retry behavior and error-surfacing semantics for reset vs refused/DNS failures. |
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The retry behavior change is narrowly scoped, preserves fail-fast semantics for non-reset failures, and is backed by targeted regression tests and clear release notes.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Extracted duplicated retry logging and jittered backoff logic into `consume_retry_with_backoff`, then reused it in both the connection-reset and timeout rescue paths so retry behavior stays consistent. Also clarified the retry comments around stale keep-alive reset errors and documented why the 429/503 path keeps separate inline backoff handling for `Retry-After`.
09a1163 to
0c1304a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It introduces new internal helper methods/constants as part of the public Parse::Client API surface, which should be explicitly privatized to avoid unintended API expansion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Lite
Marks `consume_retry_with_backoff` and `connection_reset_error?` as private methods, and `CONNECTION_RESET_CAUSES` plus `CONNECTION_RESET_MESSAGE` as private constants in `Parse::Client`. This narrows the public API surface and keeps retry/connection reset plumbing internal.
There was a problem hiding this comment.
🟢 Approval recommended
The retry behavior change is narrowly scoped, matches the stated intent, and is backed by targeted tests covering both reset and fail-fast scenarios.
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Update
Parse::Client#requestto handleFaraday::ConnectionFailedby distinguishing transient reset-style failures from hard failures like refused or DNS errors. Reset causes (ECONNRESET,EPIPE,ECONNABORTED,EOFError, plus message fallback) now retry under existing idempotency rules and raiseParse::Error::ConnectionErrorwhen retries are exhausted, while non-reset failures still fail fast as rawFaraday::ConnectionFailed. Also bumps the gem to 5.7.4, adds changelog notes, and expands retry tests to cover reset, EOF, message-only reset detection, and POST idempotency behavior.