Fix module commandresult event cleanup during unsubscribe and module unload#3545
Merged
hpatro merged 6 commits intoApr 24, 2026
Merged
Conversation
Signed-off-by: Sarthak Aggarwal <25262500+sarthakaggarwal97@users.noreply.github.com>
9460812 to
95f6e0f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #3545 +/- ##
=========================================
Coverage 76.37% 76.37%
=========================================
Files 159 159
Lines 80045 80054 +9
=========================================
+ Hits 61133 61142 +9
Misses 18912 18912
🚀 New features to boost your workflow:
|
Contributor
Author
|
@hpatro if you can help review this PR |
Contributor
Author
|
@martinrvisser it would be really helpful if you can take a look! |
Contributor
|
@sarthakaggarwal97 thanks for fixing the stale counters and NULL callback guard. The change for |
hpatro
reviewed
Apr 23, 2026
Contributor
|
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
hpatro
approved these changes
Apr 23, 2026
Contributor
Author
|
@lucasyonge here is the 100 passing runs for reply-schema-validator test: https://github.com/sarthakaggarwal97/valkey/actions/runs/24863141290/job/72793132621#step:9:4006 |
sarthakaggarwal97
added a commit
to sarthakaggarwal97/valkey
that referenced
this pull request
Apr 24, 2026
…unload (valkey-io#3545) This follows up on the commandresult API work and fixes cleanup around unsubscribe and module unload. The main issue was that command-result event listeners could leave stale state behind. On unload, we removed the listeners themselves but didn’t fully update the fast-path listener counters. Separately, unsubscribing with a NULL callback could behave badly if the listener wasn’t present anymore. In practice, that meant later commands could still walk into command-result event handling after the module was supposed to be cleaned up. Failed in Daily as well yesterday: https://github.com/valkey-io/valkey/actions/runs/24753491944/job/72421581610#step:10:852 Related Failures: valkey-io#2936 (comment) --------- Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
madolson
pushed a commit
that referenced
this pull request
Apr 27, 2026
…unload (#3545) This follows up on the commandresult API work and fixes cleanup around unsubscribe and module unload. The main issue was that command-result event listeners could leave stale state behind. On unload, we removed the listeners themselves but didn’t fully update the fast-path listener counters. Separately, unsubscribing with a NULL callback could behave badly if the listener wasn’t present anymore. In practice, that meant later commands could still walk into command-result event handling after the module was supposed to be cleaned up. Failed in Daily as well yesterday: https://github.com/valkey-io/valkey/actions/runs/24753491944/job/72421581610#step:10:852 Related Failures: #2936 (comment) --------- Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
sarthakaggarwal97
added a commit
to sarthakaggarwal97/valkey
that referenced
this pull request
Apr 27, 2026
…sers) Removed valkey-io#3504, valkey-io#3545, valkey-io#3503 - these fix bugs introduced by valkey-io#3324, valkey-io#2936, Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com> valkey-io#3392 respectively, all new in RC2. Users never experienced these bugs.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
This follows up on the commandresult API work and fixes cleanup around unsubscribe and module unload.
The main issue was that command-result event listeners could leave stale state behind. On unload, we removed the listeners themselves but didn’t fully update the fast-path listener counters. Separately, unsubscribing with a NULL callback could behave badly if the listener wasn’t present anymore. In practice, that meant later commands could still walk into command-result event handling after the module was supposed to be cleaned up.
Failed in Daily as well yesterday: https://github.com/valkey-io/valkey/actions/runs/24753491944/job/72421581610#step:10:852
Related Failures: #2936 (comment)