From 3459a64fe9262b3b96a48fff4deda3945ec52b9e Mon Sep 17 00:00:00 2001 From: Sean Kim Date: Fri, 3 Jul 2026 20:01:03 -0700 Subject: [PATCH] fix(connection): fail fast on unrecoverable connect errors (#4026) 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. --- redis/asyncio/connection.py | 29 ++++++++++++++++++++--- redis/connection.py | 29 ++++++++++++++++++++--- tests/test_asyncio/test_connection.py | 33 ++++++++++++++++++++++++++- tests/test_connection.py | 31 ++++++++++++++++++++++++- 4 files changed, 114 insertions(+), 8 deletions(-) diff --git a/redis/asyncio/connection.py b/redis/asyncio/connection.py index 781239df9c..aad20846fe 100644 --- a/redis/asyncio/connection.py +++ b/redis/asyncio/connection.py @@ -1,5 +1,6 @@ import asyncio import copy +import errno import inspect import math import socket @@ -344,6 +345,25 @@ def set_parser(self, parser_class: Type[BaseParser]) -> None: """ self._parser = parser_class(socket_read_size=self._socket_read_size) + # OS-level errno values for which reconnecting can never succeed, so + # there is no point spending the retry/backoff budget on them. + _UNRETRYABLE_CONNECT_ERRNOS = frozenset({errno.ENOENT}) + + @classmethod + def _is_retryable_connect_error(cls, error: BaseException) -> bool: + """Whether a failed connection attempt is worth retrying. + + Returns ``False`` for definitively unrecoverable socket errors (e.g. a + Unix socket path that does not exist, ``ENOENT``) so that ``connect()`` + fails fast instead of backing off for every retry. Transient errors + such as ``ECONNREFUSED`` during server startup stay retryable. + """ + cause = error.__cause__ + return not ( + isinstance(cause, OSError) + and cause.errno in cls._UNRETRYABLE_CONNECT_ERRNOS + ) + async def connect(self): """Connects to the Redis server if not already connected""" # try once the socket connect with the handshake, retry the whole @@ -355,6 +375,7 @@ async def connect(self): lambda error, failure_count: self.disconnect( error=error, failure_count=failure_count ), + is_retryable=self._is_retryable_connect_error, with_failure_count=True, ) @@ -395,17 +416,19 @@ def failure_callback(error, failure_count): ) raise e except OSError as e: - e = ConnectionError(self._error_message(e)) + conn_error = ConnectionError(self._error_message(e)) await record_error_count( server_address=getattr(self, "host", None), server_port=getattr(self, "port", None), network_peer_address=getattr(self, "host", None), network_peer_port=getattr(self, "port", None), - error_type=e, + error_type=conn_error, retry_attempts=actual_retry_attempts, is_internal=False, ) - raise e + # Preserve the original OSError (with its errno) as the cause so + # the retry policy can tell unrecoverable failures apart. + raise conn_error from e except Exception as exc: raise ConnectionError(exc) from exc diff --git a/redis/connection.py b/redis/connection.py index c481ec15dc..33f526c3b2 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -1,4 +1,5 @@ import copy +import errno import os import socket import sys @@ -1000,6 +1001,25 @@ def set_parser(self, parser_class): def _get_parser(self) -> Union[_HiredisParser, _RESP3Parser, _RESP2Parser]: return self._parser + # OS-level errno values for which reconnecting can never succeed, so + # there is no point spending the retry/backoff budget on them. + _UNRETRYABLE_CONNECT_ERRNOS = frozenset({errno.ENOENT}) + + @classmethod + def _is_retryable_connect_error(cls, error: BaseException) -> bool: + """Whether a failed connection attempt is worth retrying. + + Returns ``False`` for definitively unrecoverable socket errors (e.g. a + Unix socket path that does not exist, ``ENOENT``) so that ``connect()`` + fails fast instead of backing off for every retry. Transient errors + such as ``ECONNREFUSED`` during server startup stay retryable. + """ + cause = error.__cause__ + return not ( + isinstance(cause, OSError) + and cause.errno in cls._UNRETRYABLE_CONNECT_ERRNOS + ) + def connect(self): "Connects to the Redis server if not already connected" # try once the socket connect with the handshake, retry the whole @@ -1009,6 +1029,7 @@ def connect(self): check_health=True, retry_socket_connect=False ), lambda error: self.disconnect(error), + is_retryable=self._is_retryable_connect_error, ) def connect_check_health( @@ -1044,16 +1065,18 @@ def failure_callback(error, failure_count): ) raise e except OSError as e: - e = ConnectionError(self._error_message(e)) + conn_error = ConnectionError(self._error_message(e)) record_error_count( server_address=getattr(self, "host", None), server_port=getattr(self, "port", None), network_peer_address=getattr(self, "host", None), network_peer_port=getattr(self, "port", None), - error_type=e, + error_type=conn_error, retry_attempts=actual_retry_attempts[0], ) - raise e + # Preserve the original OSError (with its errno) as the cause so + # the retry policy can tell unrecoverable failures apart. + raise conn_error from e self._sock = sock try: diff --git a/tests/test_asyncio/test_connection.py b/tests/test_asyncio/test_connection.py index 2d5557b1d9..f5abdf402d 100644 --- a/tests/test_asyncio/test_connection.py +++ b/tests/test_asyncio/test_connection.py @@ -3,7 +3,7 @@ import ssl import types from unittest import mock -from errno import ECONNREFUSED +from errno import ECONNREFUSED, ENOENT from unittest.mock import patch import pytest @@ -411,6 +411,37 @@ async def test_connect_timeout_error_without_retry(): assert conn._connect.call_count == 1 +@pytest.mark.fixed_client +async def test_connect_without_retry_on_unrecoverable_oserror(): + """A connect error that can never succeed on retry (e.g. a missing Unix + socket path, ENOENT) fails fast instead of consuming the retry budget.""" + conn = Connection(retry=Retry(NoBackoff(), 3)) + conn._connect = mock.AsyncMock() + conn._connect.side_effect = OSError(ENOENT, "No such file or directory") + + with pytest.raises(ConnectionError) as excinfo: + await conn.connect() + # no retries: _connect is called exactly once + assert conn._connect.call_count == 1 + # the original OSError is preserved as the cause for classification + assert isinstance(excinfo.value.__cause__, OSError) + assert excinfo.value.__cause__.errno == ENOENT + + +@pytest.mark.fixed_client +async def test_connect_retries_on_transient_oserror(): + """A transient socket error such as ECONNREFUSED (server not up yet) + stays retryable, so the whole connect flow is retried as before.""" + conn = Connection(retry=Retry(NoBackoff(), 2)) + conn._connect = mock.AsyncMock() + conn._connect.side_effect = OSError(ECONNREFUSED, "Connection refused") + + with pytest.raises(ConnectionError): + await conn.connect() + # 2 retries --> 3 attempts + assert conn._connect.call_count == 3 + + @pytest.mark.onlynoncluster async def test_connection_parse_response_resume(r: redis.Redis): """ diff --git a/tests/test_connection.py b/tests/test_connection.py index 46884bdf86..55b3151eee 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -7,7 +7,7 @@ import ssl import threading import types -from errno import ECONNREFUSED, EWOULDBLOCK +from errno import ECONNREFUSED, ENOENT, EWOULDBLOCK from typing import Any from unittest import mock from unittest.mock import call, patch, MagicMock, Mock @@ -502,6 +502,35 @@ def test_connect_timeout_error_without_retry(self): assert conn._connect.call_count == 1 self.clear(conn) + def test_connect_without_retry_on_unrecoverable_oserror(self): + """A connect error that can never succeed on retry (e.g. a missing Unix + socket path, ENOENT) fails fast instead of consuming the retry budget.""" + conn = Connection(retry=Retry(NoBackoff(), 3)) + conn._connect = mock.Mock() + conn._connect.side_effect = OSError(ENOENT, "No such file or directory") + + with pytest.raises(ConnectionError) as excinfo: + conn.connect() + # no retries: _connect is called exactly once + assert conn._connect.call_count == 1 + # the original OSError is preserved as the cause for classification + assert isinstance(excinfo.value.__cause__, OSError) + assert excinfo.value.__cause__.errno == ENOENT + self.clear(conn) + + def test_connect_retries_on_transient_oserror(self): + """A transient socket error such as ECONNREFUSED (server not up yet) + stays retryable, so the whole connect flow is retried as before.""" + conn = Connection(retry=Retry(NoBackoff(), 2)) + conn._connect = mock.Mock() + conn._connect.side_effect = OSError(ECONNREFUSED, "Connection refused") + + with pytest.raises(ConnectionError): + conn.connect() + # 2 retries --> 3 attempts + assert conn._connect.call_count == 3 + self.clear(conn) + @pytest.mark.onlynoncluster @pytest.mark.parametrize(