XDELEX Command - #3467
Closed
nickiaq wants to merge 6 commits into
Closed
XDELEX Command#3467nickiaq wants to merge 6 commits into
nickiaq wants to merge 6 commits into
Conversation
6 tasks
Closed
madolson
added
major-decision-approved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #3467 +/- ##
============================================
+ Coverage 76.68% 79.94% +3.26%
============================================
Files 162 187 +25
Lines 81021 95520 +14499
============================================
+ Hits 62129 76363 +14234
- Misses 18892 19157 +265
🚀 New features to boost your workflow:
|
added 2 commits
June 24, 2026 20:15
XDELEX provides extended deletion options for stream entries with three modes for handling consumer group pending entry list (PEL) references: - KEEPREF (default): Delete stream message(s) but not PEL references - DELREF: Delete stream message(s) and all associated consumer group PEL entries - ACKED: Only delete entries acknowledged by all consumer groups The command returns an array of status codes for each requested ID: - 1: Entry was deleted - 2: Entry exists but has pending references (ACKED mode only) - -1: Entry not found in stream Includes test coverage for all modes, edge cases, replication behavior, and syntax. Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io>
Refactor the ACKED and DELREF modes to iterate over consumer groups first, then messages, instead of the previous approach of iterating over messages first with nested group loops. This provides performance improvements b/c it: - Opens consumer group iterators once instead of once per message - Uses a response tracking array to skip already-finalized messages - Reduces redundant raxStart/raxStop/raxSeek calls from O(ids * groups) to O(groups) From local benchmarking, the additional allocation (of the response array) is faster than multiple iterations of the consumer groups. The KEEPREF mode is handled separately because it doesn't need to loop over consumer groups at all. Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the XDELEX stream command, including command-spec wiring, KEEPREF/DELREF/ACKED execution, per-ID results, and unit and replication tests for deletion and validation behavior. ChangesXDELEX stream command
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant xdelexCommand
participant Stream
participant ConsumerGroups
participant Server
Client->>xdelexCommand: XDELEX key [MODE] IDS numids ids...
xdelexCommand->>Stream: Parse IDs and load stream
xdelexCommand->>ConsumerGroups: Process DELREF or ACKED PEL state
xdelexCommand->>Stream: Delete eligible entries
xdelexCommand->>Server: Signal modification and emit xdel event
xdelexCommand->>Client: Return per-ID status array
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
The force push a few seconds ago was just keeping this up with unstable. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/type/stream.tcl (1)
702-702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the DELREF test to match asserted behavior.
The test name says DELREF “keeps refs”, but the assertions verify PEL refs are removed. Please rename to avoid semantic confusion.
Suggested diff
- test {XDELEX w/ DELREF deletes all but keeps refs in consumer group PELs} { + test {XDELEX w/ DELREF deletes entries and removes consumer group PEL refs} {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/type/stream.tcl` at line 702, Rename the XDELEX DELREF test case in stream.tcl so its name matches the asserted behavior: the current title in the test block says DELREF “keeps refs,” but the assertions in that same test verify the refs are removed from consumer group PELs. Update the test description string in the test block identified by the XDELEX/DELREF scenario to reflect that refs are deleted, using the surrounding test name and assertions as the source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/t_stream.c`:
- Around line 3677-3741: DELREF can mutate the stream’s consumer-group state
without emitting WATCH/invalidation notifications because `signalModifiedKey()`
and `notifyKeyspaceEvent()` in the `stream` deletion flow are only tied to the
`deleted` counter. Update the logic around the DELREF/PEL-removal path in
`t_stream.c` so any successful removal of PEL/NACK state (not just
`streamDeleteItem`) sets a separate mutation flag and uses it to trigger the key
notifications and `server.dirty` handling; keep the existing `streamDeleteItem`,
`streamFreeNACK`, and `first_entry`/`max_deleted_entry_id` behavior intact.
- Around line 3633-3640: The missing-stream early return in the XDELEX path
leaks heap-allocated `ids` and `resps` when `id_count` exceeds
`STREAMID_STATIC_VECTOR_LEN`. Update the `o == NULL` branch in `t_stream.c` to
route through the existing `cleanup` path instead of returning directly, so the
allocations are always released before exiting. Keep the reply behavior the
same, but ensure the `cleanup:` label handles both stack and heap cases for
`ids`/`resps`.
---
Nitpick comments:
In `@tests/unit/type/stream.tcl`:
- Line 702: Rename the XDELEX DELREF test case in stream.tcl so its name matches
the asserted behavior: the current title in the test block says DELREF “keeps
refs,” but the assertions in that same test verify the refs are removed from
consumer group PELs. Update the test description string in the test block
identified by the XDELEX/DELREF scenario to reflect that refs are deleted, using
the surrounding test name and assertions as the source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a8ad017-59f1-446a-9af4-d11bf546ba5b
📒 Files selected for processing (6)
src/commands.defsrc/commands/xdelex.jsonsrc/server.hsrc/t_stream.ctests/unit/type/stream-cgroups.tcltests/unit/type/stream.tcl
- Use consistent exit cleanup to avoid leaking stream id's and resp array when stream not found. - Emit keyspace event when removing an orphaned PEL entry, which is consistant with keyspace events for other operations and avoids adding another case of inconsistent behavior as described in issue valkey-io#3429. Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io>
Contributor
|
@valkey-review-bot Please review this change. |
Contributor
|
I found two user-visible issues in the new XDELEX path: the command metadata advertises syntax the parser doesn't accept, and the executor currently queues the reply before the mutation hooks run. Both should be small to fix, but they change how clients discover and observe the command. |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
- Correct JSON DSL command definition for XDELEX to use the proper style for an optional one-of token: mode as `[KEEPREF | DELREF | ACKED ]` - Delete stream messages, mark dirty, and signal before enqueueing response array so that we meet the module keyspace API contract Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io>
Contributor
Author
|
Thank you for starting the review for this as well as XACKDEL! These were great comments from the bot. Added comments in thread on each. And, I'll add responses on the XACKDEL one hopefully shortly as well. |
Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io>
nickiaq
pushed a commit
to nickiaq/valkey
that referenced
this pull request
Jul 24, 2026
Mirroring the same concern with XDELEX, we need to defer response enqueue-ing until after other modifications to preserve the module keyspace event API contract. See issue valkey-io#3429 and this comment: valkey-io#3467 (comment) Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io>
1 task
zuiderkwast
reviewed
Sep 8, 2026
zuiderkwast
left a comment
zuiderkwast
left a comment
Contributor
There was a problem hiding this comment.
Looks good in general and very similar to the XACKDEL PR. The two commands looked identical to me, but finally I noticed that the group argument is absent in XDELEX.
Since it has much in common with the other PR and we prefer to avoid duplicated logic, consider putting both commands in one PR.
@coderabbitai Please check for duplicate code with XDEL and with the PR #3466 which adds XACKDEL. How can the code be de-duplicated, for example using a helper function?
Comment on lines
+3583
to
+3586
| } else if (strcasecmp(objectGetVal(c->argv[argi]), "IDS") != 0) { | ||
| addReplyError(c, "xdelex mode not recognized"); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Same comments as for the XACKDEL PR.
Suggested change
| } else if (strcasecmp(objectGetVal(c->argv[argi]), "IDS") != 0) { | |
| addReplyError(c, "xdelex mode not recognized"); | |
| return; | |
| } | |
| } |
Contributor
Author
There was a problem hiding this comment.
Indeed, thanks for simplifying. Fixed in 8e5e202.
|
|
||
| /* Expect IDS token. */ | ||
| if (strcasecmp(objectGetVal(c->argv[argi]), "IDS") != 0) { | ||
| addReplyError(c, "syntax error"); |
Contributor
There was a problem hiding this comment.
Suggested change
| addReplyError(c, "syntax error"); | |
| addReplyErrorObject(c, shared.syntaxerr); |
Comment on lines
+3605
to
+3609
| if (id_count > actual_ids) { | ||
| addReplyError(c, "numids parameter must match the number of IDs provided"); | ||
| return; | ||
| } else if (id_count < actual_ids) { | ||
| addReplyError(c, "syntax error"); |
Contributor
There was a problem hiding this comment.
Suggested change
| if (id_count > actual_ids) { | |
| addReplyError(c, "numids parameter must match the number of IDs provided"); | |
| return; | |
| } else if (id_count < actual_ids) { | |
| addReplyError(c, "syntax error"); | |
| if (id_count != actual_ids) { | |
| addReplyErrorObject(c, shared.syntaxerr); |
Contributor
Author
There was a problem hiding this comment.
Fixed in 8e5e202; addressed as recommended here, but also needed to change tests to match.
Responding to [these review comments](valkey-io#3467 (review)), this switches syntax errors to the standard shared helper and simplifies error handling for the mode check and number of ID's matching remaining args. Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io>
nickiaq
pushed a commit
to nickiaq/valkey
that referenced
this pull request
Sep 9, 2026
Squashed commit of the following: commit 8e5e202 Author: Nick Iaquinto <git+valkey@iaquinto.io> Date: Tue Sep 8 18:13:11 2026 -0400 Simplify & Standardize Error Handling (RE: Review Comments) Responding to [these review comments](valkey-io#3467 (review)), this switches syntax errors to the standard shared helper and simplifies error handling for the mode check and number of ID's matching remaining args. Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io> commit 27cf7bc Author: Nick Iaquinto <git+valkey@iaquinto.io> Date: Thu Jul 23 21:13:38 2026 -0400 Clang Format Fix (remove trailing newline) Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io> commit 703506c Author: Nick Iaquinto <git+valkey@iaquinto.io> Date: Thu Jul 23 20:42:40 2026 -0400 Fixes from Valkey Bot & hpatro Code Review - Correct JSON DSL command definition for XDELEX to use the proper style for an optional one-of token: mode as `[KEEPREF | DELREF | ACKED ]` - Delete stream messages, mark dirty, and signal before enqueueing response array so that we meet the module keyspace API contract Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io> commit 0fc54c5 Author: Nick Iaquinto <git+valkey@iaquinto.io> Date: Wed Jun 24 22:53:07 2026 -0400 Corrections from XDELEX AI Review - Use consistent exit cleanup to avoid leaking stream id's and resp array when stream not found. - Emit keyspace event when removing an orphaned PEL entry, which is consistant with keyspace events for other operations and avoids adding another case of inconsistent behavior as described in issue valkey-io#3429. Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io> commit 20f2d99 Author: Nick Iaquinto <git+valkey@iaquinto.io> Date: Thu Apr 9 00:21:37 2026 -0400 Faster XDELEX Refactor the ACKED and DELREF modes to iterate over consumer groups first, then messages, instead of the previous approach of iterating over messages first with nested group loops. This provides performance improvements b/c it: - Opens consumer group iterators once instead of once per message - Uses a response tracking array to skip already-finalized messages - Reduces redundant raxStart/raxStop/raxSeek calls from O(ids * groups) to O(groups) From local benchmarking, the additional allocation (of the response array) is faster than multiple iterations of the consumer groups. The KEEPREF mode is handled separately because it doesn't need to loop over consumer groups at all. Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io> commit 66a8555 Author: Nick Iaquinto <git+valkey@iaquinto.io> Date: Thu Apr 9 00:02:02 2026 -0400 XDELEX Command XDELEX provides extended deletion options for stream entries with three modes for handling consumer group pending entry list (PEL) references: - KEEPREF (default): Delete stream message(s) but not PEL references - DELREF: Delete stream message(s) and all associated consumer group PEL entries - ACKED: Only delete entries acknowledged by all consumer groups The command returns an array of status codes for each requested ID: - 1: Entry was deleted - 2: Entry exists but has pending references (ACKED mode only) - -1: Entry not found in stream Includes test coverage for all modes, edge cases, replication behavior, and syntax. Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io> Signed-off-by: Nick Iaquinto <git+valkey@iaquinto.io>
Contributor
Author
|
Same applies here as in this comment; added |
Contributor
Author
|
Closing this in favor of #4629! |
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 PR adds a stream command
XDELEXthat deletes one or more stream messages with explicit control over how consumer groups' PEL references are handled. While implementingXACKDEL, it made sense to also add this as a complimentary command.For more background on the problem space, see #2903. Similar to
XACKDEL, this is compatible with the equivalent command introduced in Redis 8.2.0.The command supports three deletion modes:
KEEPREF(default): Deletes the stream entry but leaves PEL references intact in all consumer groupsDELREF: Deletes the stream entry and forcibly removes it from all consumer group PELsACKED: Only deletes the entry once every consumer group has acknowledged or passed it — safe for fan-out topologiesThe command returns a per-ID integer array:
1for deleted,2for exists-but-not-yet-deletable (ACKED mode only), and-1for not found.I've included test coverage for all modes, multi-group scenarios, replication behavior, syntax, and edge cases.
This is my first PR (alongside
XACKDEL#3466). And I've tried to follow the style guide & contributing guidelines. Happy to make any corrections if I've missed something.It's possible to create a shared function for parsing the mode & id count between
XACKDELandXDELEX. So I can follow up with a PR to reduce that duplicated code. Or, I can merge these 2 PRs into one.