Repository navigation
fix(connection): opt-in fast-fail on missing Unix socket (ENOENT) - #4558
Conversation
connect() wrapped the whole connect+handshake in the retry policy and re-wrapped every OSError into a generic ConnectionError, so definitively unrecoverable failures such as a missing Unix socket path (ENOENT) were retried DEFAULT_RETRY_COUNT times with full exponential backoff. Pass an is_retryable predicate from connect() that returns False for a conservative set of unrecoverable errnos (currently ENOENT), and preserve the original OSError as the exception cause (raise ConnectionError(...) from e) so the retry policy can classify it. Transient errors like ECONNREFUSED during server startup remain retryable, so the intentional handshake-retry behavior is unchanged. Mirrored across the sync and async connection stacks, with tests in both. (cherry picked from commit 3459a64)
Add an optional is_retryable callable to AbstractRetry and both Retry implementations. When set, an error is retried only if its type is in supported_errors AND the checker returns True, matching the existing per-call checker semantics. The default (None) leaves behavior unchanged. Ship is_retryable_connect_error as a reusable checker that returns False only for a ConnectionError chained from an ENOENT OSError, so callers can opt in to failing fast on a missing Unix socket path. Preserve the checker when converting a sync Retry to the asyncio one and when deep-copying a policy.
Chain the original OSError with `raise ... from e` at every retryable wrap site in the sync and async connection stacks (connect, socket write, socket read and can_read), so a cause-inspecting retry checker can reach the root errno. Drop the always-on ENOENT classification from the connection classes: the reusable checker now lives on the Retry object, so callers opt in and the default retry behavior is unchanged. Regression tests cover a real missing Unix socket path (POSIX-only) with and without the checker, the default retries for ENOENT and ECONNREFUSED, checker composition, and cause preservation at the read, write and can_read wrap sites.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5da3fa6029
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3be360b to
8fd61c2
Compare
8fd61c2 to
ce7bd76
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12dd43d040
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e883bfbcdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @BlueX888, thank you for your contribution!
This follows the takeover checklist closely, and the design matches what I asked for in #4166. Two small test fixes are needed before merge:
- In
tests/test_asyncio/test_retry.py, the new tests were inserted in the middle oftest_pool_converts_sync_retry, so itsset_retryhalf now runs insidetest_client_conversion_preserves_checker. Please restore the original test unchanged and keep the new test self-contained. test_lazy_reconnect_uses_retry_checker(sync and async) makes_connectraise a chainedConnectionError, which the real_connectnever does. On the real path, the rawOSErroris not insupported_errors, so it passes through the inner retry.connect_check_healththen wraps it, and the command-level retry stops it. Please replace this test with a client-level one, for exampleRedis(unix_socket_path=<missing path>, retry=Retry(NoBackoff(), 3, is_retryable=is_retryable_connect_error)).ping()asserting a single_connectcall, or drop it.
Once those are in, this should be ready to merge.
petyaslavova
added
maintenance
Restore test_pool_converts_sync_retry unchanged and keep the new conversion test self-contained. Replace the lazy-reconnect tests, whose mock raised a chained ConnectionError that _connect never raises, with a client-level test whose _connect raises the raw OSError the real path produces; the client must still stop after a single attempt.
|
Thanks for the precise review — both points are fixed in
Verification: both new tests pass (sync and async); |
|
CI note on |
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks for the quick update @BlueX888, both test fixes look good.
One more change is needed, from the Codex WATCH finding. When the Retry-level checker rejects an error, call_with_retry raises before fail runs. A watching pipeline relies on that callback (_disconnect_reset_raise_on_watching) to reset itself and raise WatchError. So with a checker that rejects, for example, TimeoutError, the pipeline stays watching=True. A caller that catches the timeout can then run MULTI/EXEC on a reconnected socket with no WATCH. Please make a rejection by the Retry-level checker call fail before raising, the same as an exhausted retry budget, and leave the per-call predicate unchanged. Please mirror this in the async Retry and add a watching-pipeline regression test on both sides.
I'm fine leaving the deepcopy finding as is, because backoff objects are already deep-copied the same way. Please reply on that thread so it can be resolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b5201c301
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks for the update @BlueX888, the fail call on a checker rejection and the watching-pipeline tests on both stacks are exactly what I asked for.
About the latest Codex PubSub finding: I agree with your reply. Running out of retries already calls fail (and so PubSub._reconnect()) on its final failure, so a rejection now matches that path. Splitting cleanup from reconnect is a separate design question and not needed for this PR. Please resolve that thread.
LGTM.
petyaslavova
added
feature
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f029ee8f3c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 948701f9c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9f7be652f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Takes over #4166 per @petyaslavova's open-for-takeover note. Builds on @sean-kim05's commits (author preserved —
97b25374is acherry-pick -xof3459a64) and applies the review checklist. Links #4026 (kept open per the original PR's request; this covers the ENOENT case only).Change
Retry:AbstractRetrygains an optionalis_retryablechecker (defaultNone= unchanged behavior), AND-composed withsupported_errorsand the per-call predicate in both the sync and asynccall_with_retry; preserved through_to_async_retryanddeepcopy, and included in equality/hash.is_retryable_connect_errorreturnsFalseonly when__cause__is anOSErrorwitherrno == ENOENT— shipped as a helper for the checker, not as always-on behavior (the original PR's hardcoded default is removed).raise ConnectionError(...) from eat theConnectionError-from-OSErrorwrap sites inconnection.pyandasyncio/connection.py(connect / read / write / can_read). The asyncsendpath already usedfrom None, andTimeoutErrorwraps are unchanged — both pre-existing on master.Behavior
is_retryable=is_retryable_connect_error: a missing Unix socket fails after exactly one attempt.__cause__only, so a rawOSErrorraised straight into the lazy-reconnect inner retry is still treated as retryable (conservativeTrue, as discussed in the review). The default opt-in configuration is unaffected.Tests
tests/test_retry.py/tests/test_asyncio/test_retry.py: checker composition with supported-errors and the per-call predicate, conversion through_to_async_retry, deepcopy, and default-behavior-unchanged; 19 new test cases.tests/test_connection.py/tests/test_asyncio/test_connection.py: a real missing-Unix-socket fast-fail with the checker (exactly one attempt) versus without it (multiple attempts), plus__cause__preservation. These are POSIX-only and skip on Windows — their first real run is Linux CI on this PR.ruff check,ruff format --check, andvultureare clean.Red evidence: against
ac8f8c1ethe new tests fail collection (helper/checker absent); against the original commit alone, the default ENOENT behavior is the always-on fast-fail that the review asked to make opt-in.Note
Medium Risk
Touches core connection retry and exception-wrapping paths in sync and async stacks; default behavior is unchanged but misconfigured checkers or reliance on old exception shapes could affect callers.
Overview
Adds an opt-in retry filter on
Retry/AbstractRetry: an optionalis_retryablecallable runs after the usualsupported_errorsmatch and is AND-composed with any per-callis_retryableincall_with_retry. Default isNone, so existing clients behave the same.Ships
is_retryable_connect_error, which returnsFalsewhen the raisedConnectionErrorchains anOSErrorwithENOENT(missing Unix socket path), so callers can fail after one attempt instead of burning the full retry/backoff budget. Transient errors likeECONNREFUSEDstay retryable.To support that classification, sync and async connection code now raises
ConnectionErrorfrom the underlyingOSErroron connect/read/write/can_readpaths (with a connect-path cleanup to avoid traceback reference cycles). When the retry-level checker rejects an error,call_with_retrystill invokes the failure handler before re-raising so pipeline WATCH cleanup and similar hooks run.Documentation in
docs/retry.rstdescribes the checker;_to_async_retry, equality, and deepcopy preserveis_retryable. Broad test coverage covers checker composition, ENOENT fast-fail vs default retries, cause chaining, and pipeline reset on checker rejection.Reviewed by Cursor Bugbot for commit a424ff3. Bugbot is set up for automated code reviews on this repo. Configure here.