Add Redis readiness verification (#3555)

[ManelCoutinhoSensei]

Pull Request check-list

Please make sure to review and check all of these items:

• Do tests and lints pass with this change?
• Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Is there an example added to the examples folder (if applicable)?
• Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Fixes #3555 (specially the comment on that issue)

Please check if you'd rather run the tests with check_ready=True by default (I also tested that) and if you think that any other tests deserve a second version with this flag set.

[petyaslavova]

Hi @ManelCoutinhoSensei, thank you for your contribution! We’ll review your change soon.

[ManelCoutinhoSensei]

@petyaslavova you need to rerun the failed tests due to the test suit flakiness

[ManelCoutinhoSensei]

Hi @petyaslavova , any news about this one?

[petyaslavova]

Hi @ManelCoutinhoSensei, I have started the review, but couldn't complete it.
We have some urgent changes we need to complete, and the PR reviews will be delayed by a week or two.
Sorry for the delay!

[ManelCoutinhoSensei]

Should I solve the conflicts because the PR reviews are going to be done this week or should I postpone it?

[petyaslavova]

Should I solve the conflicts because the PR reviews are going to be done this week or should I postpone it?

Hi @ManelCoutinhoSensei, please wait for a while with the conflict solving. We first need to address one more planned change related to the async cluster, and then I will be able to progress on this PR.
Thank you for your patience and understanding!

[ManelCoutinhoSensei]

Hey @petyaslavova! 🙌 Just checking in to see if there are any updates or an estimated timeline for moving forward with this PR. Totally understand things can get busy — just wanted to make sure it’s still on your radar.
Thanks again for your time and support! 😊

[ManelCoutinhoSensei]

[petyaslavova]

Hi @ManelCoutinhoSensei,
Apologies for the delay in reviewing the PR. I've added some comments and suggestions for changes that I believe will help make the code more maintainable in the long term. While the comments are placed in the sync client/connection code, they apply to the async parts as well.

[petyaslavova]

I think it would be better to replace this block with a call to self._send_ping(), and refactor that method to accept an additional argument—something like accept_no_auth_response=False by default. This way, you can reuse the existing logic without duplicating the socket write/read operations, making the code cleaner and easier to maintain.

[ManelCoutinhoSensei]

I understand the desired but in order to use the read_response part of the _send_ping, the parsers should already be initialized (which only happens on on_connect_check_health that is called after the _connect_check_server_ready).
Either way I refactored the _connection method so that i could use the send_command command part, let me know if this suffices or if we should consider refactoring the connection flow to accommodate an earlier parser initialization

[ManelCoutinhoSensei]

Hey @petyaslavova, just updated this PR after the merge of #3601
One of the failing test-suits is the same problem as before and the other seems to be flaky (since it passed in my run).
Let me know if there are any follow ups needed to your comments or any new changes that you'd like to see made

[ManelCoutinhoSensei]

Hey @petyaslavova, have you had the chance of reviewing the changes you asked?

[petyaslavova]

Hey @petyaslavova, have you had the chance of reviewing the changes you asked?

Hi @ManelCoutinhoSensei, I was in vacation for a while and I didn't have a chance to look at this. I'm trying to catch up with the PRs this and next week.

[ManelCoutinhoSensei]

Hey @petyaslavova, I updated it and it is passing all the tests on my side

[petyaslavova]

Hey @petyaslavova, I updated it and it is passing all the tests on my side

Hi @ManelCoutinhoSensei, there are some details in this PR that can't be merged into the current master state - there is a new functionality in the client that handles specific push notifications, which can be received with the command responses. If you read directly from the socket, you won't be able to use the automatic handling of those messages, which happens in the parser.
Can't it be transformed to use read_response function(the parser is already initialized when those requests are sent)?

[ManelCoutinhoSensei]

Hey @petyaslavova,

(...) Can't it be transformed to use read_response function(the parser is already initialized when those requests are sent)?

does this comment from July 1st address what you are asking? Or is it no longer up to date?

[petyaslavova]

Hey @petyaslavova,

(...) Can't it be transformed to use read_response function(the parser is already initialized when those requests are sent)?

does this comment from July 1st address what you are asking? Or is it no longer up to date?

I have to admit I’d forgotten about this one, but yes — this is exactly the right solution here.

[ManelCoutinhoSensei]

(...) Can't it be transformed to use read_response function(the parser is already initialized when those requests are sent)?

does this comment from July 1st address what you are asking? Or is it no longer up to date?

I have to admit I’d forgotten about this one, but yes — this is exactly the right solution here.

Which part is the right solution? Are you saying that using the send_command is sufficient or that we should refactor the connection flow to initialize the parser earlier?

[petyaslavova]

(...) Can't it be transformed to use read_response function(the parser is already initialized when those requests are sent)?

does this comment from July 1st address what you are asking? Or is it no longer up to date?

I have to admit I’d forgotten about this one, but yes — this is exactly the right solution here.

Which part is the right solution? Are you saying that using the send_command is sufficient or that we should refactor the connection flow to initialize the parser earlier?

The parser is initialized in the init method when resp3 is used - https://github.com/redis/redis-py/blob/8403ddcfcf16c95baab330076b4905483e44fbb7/redis/connection.py#L429C25-L429C37
This logic can be extended to set the parser when protocol is 2 as well, and then you will be able to use directly the _send_ping command to validate server is already ready to respond to requests.

[ManelCoutinhoSensei]

The parser is initialized in the init method when resp3 is used - https://github.com/redis/redis-py/blob/8403ddcfcf16c95baab330076b4905483e44fbb7/redis/connection.py#L429C25-L429C37
This logic can be extended to set the parser when protocol is 2 as well,

I might be wrong but even when using resp3, the parser is only initialized (meaning only the socket_read_size is set). For the read_response method to work, we need to call the Parser.on_connect method (that initializes the _buffer and the enconder) which is only called afterwards on on_connect_check_health (like previously explained)

[petyaslavova]

The parser is initialized in the init method when resp3 is used - https://github.com/redis/redis-py/blob/8403ddcfcf16c95baab330076b4905483e44fbb7/redis/connection.py#L429C25-L429C37
This logic can be extended to set the parser when protocol is 2 as well,

I might be wrong but even when using resp3, the parser is only initialized (meaning only the socket_read_size is set). For the read_response method to work, we need to call the Parser.on_connect method (that initializes the _buffer and the enconder) which is only called afterwards on on_connect_check_health (like previously explained)

You’re absolutely right. 🙂

I finally had time to carefully review (and debug) the code related to the reported issue and this PR. My conclusion is that this change would introduce additional complexity without fully resolving the core problem.

With the current approach, we’d force a PING on all connections, even though we already have health checks available. When health checks are enabled, a PING is executed just before the first real command as part of the initial client setup.
The issue is that some clients of the library may prefer not to enable health checks because they add extra command executions.

In my opinion, the correct solution is to move the initial health-check logic (executed after the socket connect) into the retry block. That way, we can retry the entire connection-establishment flow—not just the socket connect.

Something like the following will handle the retry of the whole connection establishment logic:
def connect(self): "Connects to the Redis server if not already connected" self.retry.call_with_retry( lambda: self.connect_check_health( check_health=True, retry_socket_connect=False ), lambda error: self.disconnect(error), )
This will try once the socket connect and run the commands in the connect_check_health method, in case of failure of any of those two - this whole flow will be retried after disconnect is called as error handler.

@ManelCoutinhoSensei, what do you think about this change in direction?

[ManelCoutinhoSensei]

Hey @petyaslavova,

Thanks a lot for taking a close look at this issue! 🙏

The problem that I originally flagged still exists (_connect doesn't guarantee connectedness in cases of idle containers). That said, the issue seems largely solved by fixing the recursion bug - at least in my toy examples. It might reappear in more complex scenarios.

What I was proposing was essentially the current approach (sending a PING after attempting to connect to confirm the connection). So both your suggestion and mine introduce a bit of extra complexity, without much additional benefit.
I think that as long as we keep in mind that _connect doesn't guarantee a fully working connection, we're fine to close this branch (and related issue) with one caveat: authenticated requests.

Authenticated requests

If you look at the authentication-related code (650-652, 661 and 670) we're hardcoding check_health=False to avoid error when PING fails. I think that we could/should change this to use the same pattern as the other commands (check_health=check_health) and then change _send_ping so it accepts AuthenticationError as valid outcome (since this method is only used to check server health and this does, indeed, mean that the server is alive and healthy).

That would require setting disconnect_on_error=False for read_response, but those cases are already handled properly. In the end, something like this should be close to what we need:

def _send_ping(self):
    """Send PING, expect PONG in return"""
    self.send_command("PING", check_health=False)
    try:
        if str_if_bytes(self.read_response(disconnect_on_error=False)) != "PONG":
            raise ConnectionError("Bad response from PING health check")
    except AuthenticationError:
        # That's fine -> server is healthy
        # Reset _parser due to the rewind on error mechanic that would make it read the same answer twice
        # The rational behind rewind on error when disconnect_on_error is deactivated should be rethought perhaps
        self._parser.on_disconnect()
        self._parser.on_connect(self)

Let me know what you think! 😊

[jit-ci]

❌ The following Jit checks failed to run:

• secret-detection-trufflehog
• static-code-analysis-semgrep-pro

#jit_bypass_commit in this PR to bypass, Jit Admin privileges required.

More info in the Jit platform.

[ManelCoutinhoSensei]

Hi again @petyaslavova 👋,

I’ve been experimenting with this other issue (better explained in this comment) and realized that my previous explanation wasn’t entirely accurate.

Here’s the situation:

• If the connection is successfully established but the container later goes idle, we attempt to reconnect.
• However, since the last check_health may have been performed very recently, that step will be skipped.

This was something I didn’t account for in my previous comment.

To address this, I updated the synchronous version with a proposal where the PING command is moved unconditionally outside of send_commands. In other words, regardless of the last ping or the health_check_interval, we always check the connection when starting a new one.

I’ve also removed the previous flag since, as mentioned earlier, the library’s default behavior already includes sending this PING. This change keeps that same behavior (though I can re-add the flag if you prefer).

On top of that, this approach also resolves the #3203 (mentioned before) - at least in the way I’ve been reproducing it.

Eager to hear your thoughts! Should we move forward with this solution and implement it for the async version as well? 🚀