Fix crash during SLAVEOF when clients are blocked on lazyfree #13853
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
Author
|
CI with smoking test: https://github.com/sundb/redis/actions/runs/13790169463 |
8d641af to
8d32fe8
Compare
59c8a73 to
481e9cc
Compare
oranagra
approved these changes
Mar 11, 2025
Member
oranagra
left a comment
There was a problem hiding this comment.
LGTM, but i'll take this opportunity to note something else i recently noticed which might be related.
in unblockClient we're calling resetClient, but for some flows (notably flushallSyncBgDone, we're calling both unblockClient and commandProcessed (which calls resetClient too).
currently, there's no harm for calling resetClient twice, but there could be (in my POC, there was).
i ended up adding an exception in unblockClient (like we have for BLOCKED_SHUTDOWN), but this is probably wrong, and we should better define who's responsible of clearing both of these (resetClient, and updateStatsOnUnblock in each flow, avoid calling them twice, or not calling them at all).
oranagra
reviewed
Mar 11, 2025
Co-authored-by: oranagra <oran@redislabs.com>
Member
|
changes LGTM, from my perspective, unless you want to proceed with the cleanup around |
Collaborator
Author
@oranagra since this PR needs to backport to 7.4, let's put some other PR to handle it? |
sundb
added a commit
that referenced
this pull request
Mar 19, 2025
Fix #13853 (review) This PR ensures that the client's current command is not reset by unblockClient(), while still needing to be handled after `unblockclient()`. The FLUSH command still requires reprocessing (update the replication offset) after unblockClient(). Therefore, we mark such blocked clients with the CLIENT_PENDING_COMMAND flag to prevent the command from being reset during unblockClient().
sundb
added a commit
that referenced
this pull request
Apr 13, 2025
This test was introduced by #13853 We determine if the client is in blocked status, but if async flushdb is completed before checking the blocked status, the test will fail. So modify the test to only determine if `lazyfree_pending_objects` is correct to ensure that flushdb is async, that is, the client must be blocked.
YaacovHazan
pushed a commit
to YaacovHazan/redis
that referenced
this pull request
Apr 22, 2025
…13853) After redis#13167, when a client calls `FLUSHDB` command, we still async empty database, and the client was blocked until the lazyfree completes. 1) If another client calls `SLAVEOF` command during this time, the server will unblock all blocked clients, including those blocked by the lazyfree. However, when unblocking a lazyfree blocked client, we forgot to call `updateStatsOnUnblock()`, which ultimately triggered the following assertion. 2) If a client blocked by Lazyfree is unblocked midway, and at this point the `bio_comp_list` has already received the completion notification for the bio, we might end up processing a client that has already been unblocked in `flushallSyncBgDone()`. Therefore, we need to filter it out. --------- Co-authored-by: oranagra <oran@redislabs.com>
funny-dog
pushed a commit
to funny-dog/redis
that referenced
this pull request
Sep 17, 2025
…13853) After redis#13167, when a client calls `FLUSHDB` command, we still async empty database, and the client was blocked until the lazyfree completes. 1) If another client calls `SLAVEOF` command during this time, the server will unblock all blocked clients, including those blocked by the lazyfree. However, when unblocking a lazyfree blocked client, we forgot to call `updateStatsOnUnblock()`, which ultimately triggered the following assertion. 2) If a client blocked by Lazyfree is unblocked midway, and at this point the `bio_comp_list` has already received the completion notification for the bio, we might end up processing a client that has already been unblocked in `flushallSyncBgDone()`. Therefore, we need to filter it out. --------- Co-authored-by: oranagra <oran@redislabs.com>
funny-dog
pushed a commit
to funny-dog/redis
that referenced
this pull request
Sep 17, 2025
Fix redis#13853 (review) This PR ensures that the client's current command is not reset by unblockClient(), while still needing to be handled after `unblockclient()`. The FLUSH command still requires reprocessing (update the replication offset) after unblockClient(). Therefore, we mark such blocked clients with the CLIENT_PENDING_COMMAND flag to prevent the command from being reset during unblockClient().
funny-dog
pushed a commit
to funny-dog/redis
that referenced
this pull request
Sep 17, 2025
This test was introduced by redis#13853 We determine if the client is in blocked status, but if async flushdb is completed before checking the blocked status, the test will fail. So modify the test to only determine if `lazyfree_pending_objects` is correct to ensure that flushdb is async, that is, the client must be blocked.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After #13167, when a client calls
FLUSHDBcomand, we still async empty database, and the client was blocked until the lazyfree completes.SLAVEOFcommand during this time, the server will unblock all blocked clients, including those blocked by the lazyfree. However, when unblocking a lazyfree blocked client, we forgot to callupdateStatsOnUnblock(), which ultimately triggered the following assertion.bio_comp_listhas already received the completion notification for the bio, we might end up processing a client that has already been unblocked influshallSyncBgDone(). Therefore, we need to filter it out.failed CI: https://github.com/sundb/redis/actions/runs/13802290567/job/38606761833