From 827a5ee622bafbe92ce8775cb19ad869e84e392c Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Tue, 8 Sep 2026 18:27:51 -0400 Subject: [PATCH 01/24] Implement XACKDEL & Address Code Reviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Squash merged from PR#3466 to create combined PR with both XACKDEL & XDELEX. Summary of XACKDEL Below from the original PR: Adds a stream command XDELEX that deletes one or more stream messages with explicit control over how consumer groups' PEL references are handled. While implementing XACKDEL, 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 groups - DELREF: Deletes the stream entry and forcibly removes it from all consumer group PELs - ACKED: Only deletes the entry once every consumer group has acknowledged or passed it — safe for fan-out topologies The command returns a per-ID integer array: 1 for deleted, 2 for exists-but-not-yet-deletable (ACKED mode only), and -1 for not found. Signed-off-by: Nick Iaquinto --- src/commands.def | 37 +++ src/commands/xackdel.json | 97 ++++++ src/server.h | 1 + src/t_stream.c | 273 +++++++++++++++++ tests/unit/type/stream-cgroups.tcl | 98 ++++++ tests/unit/type/stream.tcl | 472 +++++++++++++++++++++++++++++ 6 files changed, 978 insertions(+) create mode 100644 src/commands/xackdel.json diff --git a/src/commands.def b/src/commands.def index e9a3eb56624..59aa67faad8 100644 --- a/src/commands.def +++ b/src/commands.def @@ -10452,6 +10452,42 @@ struct COMMAND_ARG XACK_Args[] = { {MAKE_ARG("id",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_MULTIPLE,0,NULL)}, }; +/********** XACKDEL ********************/ + +#ifndef SKIP_CMD_HISTORY_TABLE +/* XACKDEL history */ +#define XACKDEL_History NULL +#endif + +#ifndef SKIP_CMD_TIPS_TABLE +/* XACKDEL tips */ +#define XACKDEL_Tips NULL +#endif + +#ifndef SKIP_CMD_KEY_SPECS_TABLE +/* XACKDEL key specs */ +keySpec XACKDEL_Keyspecs[1] = { +{NULL,CMD_KEY_RW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}} +}; +#endif + +/* XACKDEL mode argument table */ +struct COMMAND_ARG XACKDEL_mode_Subargs[] = { +{MAKE_ARG("keepref",ARG_TYPE_PURE_TOKEN,-1,"KEEPREF",NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("delref",ARG_TYPE_PURE_TOKEN,-1,"DELREF",NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("acked",ARG_TYPE_PURE_TOKEN,-1,"ACKED",NULL,NULL,CMD_ARG_NONE,0,NULL)}, +}; + +/* XACKDEL argument table */ +struct COMMAND_ARG XACKDEL_Args[] = { +{MAKE_ARG("key",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("group",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("mode",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,3,NULL),.subargs=XACKDEL_mode_Subargs}, +{MAKE_ARG("ids",ARG_TYPE_PURE_TOKEN,-1,"IDS",NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("numids",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("id",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_MULTIPLE,0,NULL)}, +}; + /********** XADD ********************/ #ifndef SKIP_CMD_HISTORY_TABLE @@ -12214,6 +12250,7 @@ struct COMMAND_STRUCT serverCommandTable[] = { {MAKE_CMD("zunionstore","Stores the union of multiple sorted sets in a key.","O(N)+O(M log(M)) with N being the sum of the sizes of the input sorted sets, and M being the number of elements in the resulting sorted set.","2.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZUNIONSTORE_History,0,ZUNIONSTORE_Tips,0,zunionstoreCommand,-4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZUNIONSTORE_Keyspecs,2,zunionInterDiffStoreGetKeys,5),.args=ZUNIONSTORE_Args}, /* stream */ {MAKE_CMD("xack","Returns the number of messages that were successfully acknowledged by the consumer group member of a stream.","O(1) for each message ID processed.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XACK_History,0,XACK_Tips,0,xackCommand,-4,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XACK_Keyspecs,1,NULL,3),.args=XACK_Args}, +{MAKE_CMD("xackdel","Acknowledge and (if possible) delete stream message(s).","O(1)","9.2.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XACKDEL_History,0,XACKDEL_Tips,0,xackdelCommand,-6,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_WRITE|ACL_CATEGORY_STREAM,NULL,XACKDEL_Keyspecs,1,NULL,6),.args=XACKDEL_Args}, {MAKE_CMD("xadd","Appends a new message to a stream. Creates the key if it doesn't exist.","O(1) when adding a new entry, O(N) when trimming where N being the number of entries evicted.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XADD_History,2,XADD_Tips,1,xaddCommand,-5,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XADD_Keyspecs,1,NULL,5),.args=XADD_Args}, {MAKE_CMD("xautoclaim","Changes, or acquires, ownership of messages in a consumer group, as if the messages were delivered to a consumer group member.","O(1) if COUNT is small.","6.2.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XAUTOCLAIM_History,1,XAUTOCLAIM_Tips,1,xautoclaimCommand,-6,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XAUTOCLAIM_Keyspecs,1,NULL,7),.args=XAUTOCLAIM_Args}, {MAKE_CMD("xclaim","Changes, or acquires, ownership of a message in a consumer group, as if the message was delivered to a consumer group member.","O(log N) with N being the number of messages in the PEL of the consumer group.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XCLAIM_History,0,XCLAIM_Tips,1,xclaimCommand,-6,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XCLAIM_Keyspecs,1,NULL,11),.args=XCLAIM_Args}, diff --git a/src/commands/xackdel.json b/src/commands/xackdel.json new file mode 100644 index 00000000000..f4f1251b74c --- /dev/null +++ b/src/commands/xackdel.json @@ -0,0 +1,97 @@ +{ + "XACKDEL": { + "summary": "Acknowledge and (if possible) delete stream message(s).", + "complexity": "O(1)", + "group": "stream", + "since": "9.2.0", + "arity": -6, + "function": "xackdelCommand", + "command_flags": [ + "WRITE", + "FAST" + ], + "acl_categories": [ + "FAST", + "WRITE", + "STREAM" + ], + "key_specs": [ + { + "flags": [ + "RW", + "UPDATE" + ], + "begin_search": { + "index": { + "pos": 1 + } + }, + "find_keys": { + "range": { + "lastkey": 0, + "step": 1, + "limit": 0 + } + } + } + ], + "arguments": [ + { + "name": "key", + "type": "key", + "key_spec_index": 0 + }, + { + "name": "group", + "type": "string" + }, + { + "name": "mode", + "type": "oneof", + "optional": true, + "arguments": [ + { + "name": "keepref", + "type": "pure-token", + "token": "KEEPREF" + }, + { + "name": "delref", + "type": "pure-token", + "token": "DELREF" + }, + { + "name": "acked", + "type": "pure-token", + "token": "ACKED" + } + ] + }, + { + "name": "ids", + "type": "pure-token", + "token": "IDS" + }, + { + "name": "numids", + "type": "integer" + }, + { + "name": "id", + "type": "string", + "multiple": true + } + ], + "reply_schema": { + "description": "The command returns an integer for each stream message: -1=message not found, 1=acked and deleted, 2=acked but not deleted.", + "type": "array", + "minItems": 1, + "items": { + "description": "Status of the stream message, -1=message not found, 1=acked and deleted, 2=acked but not deleted.", + "type": "integer", + "minimum": -1, + "maximum": 2 + } + } + } +} diff --git a/src/server.h b/src/server.h index 9524e9f4741..4b3f66be991 100644 --- a/src/server.h +++ b/src/server.h @@ -4338,6 +4338,7 @@ void xclaimCommand(client *c); void xautoclaimCommand(client *c); void xinfoCommand(client *c); void xdelCommand(client *c); +void xackdelCommand(client *c); void xtrimCommand(client *c); void lolwutCommand(client *c); void aclCommand(client *c); diff --git a/src/t_stream.c b/src/t_stream.c index af92e6a0316..5488f7a7fbf 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -3613,6 +3613,279 @@ void xdelCommand(client *c) { if (ids != static_ids) zfree(ids); } +/* XACKDEL [KEEPREF | DELREF | ACKED] IDS num [ ... ] + * + * Acknowledge one or more messages and delete them if all consumer groups have + * ack'd. Returns an array of integers equal to the number of ids. For this int + * array, -1 means the message does not exist in the stream, 1 means the message + * was acknowledged and deleted, and 2 means the message was acknowledged but + * not deleted. */ +void xackdelCommand(client *c) { + streamCG *group = NULL; + robj *o = lookupKeyRead(c->db, c->argv[1]); + if (o) { + if (checkType(c, o, OBJ_STREAM)) return; /* Type error. */ + group = streamLookupCG(objectGetVal(o), objectGetVal(c->argv[2])); + } + + /* Check what mode is set, if any. + * ex. [KEEPREF | DELREF | ACKED] IDS n [ID ...] + */ + int argi = 3; + int mode = 0; /* 0=keepref, 1=delref, 2=acked */ + if (strcasecmp(objectGetVal(c->argv[argi]), "KEEPREF") == 0) { + argi += 1; + mode = 0; + } else if (strcasecmp(objectGetVal(c->argv[argi]), "DELREF") == 0) { + argi += 1; + mode = 1; + } else if (strcasecmp(objectGetVal(c->argv[argi]), "ACKED") == 0) { + argi += 1; + mode = 2; + } + + /* Expect IDS token. */ + if (strcasecmp(objectGetVal(c->argv[argi]), "IDS") != 0) { + addReplyErrorObject(c, shared.syntaxerr); + return; + } + argi++; /* past IDS */ + + /* Parse and validate numids: must be a positive integer. */ + long long id_count; + if (getLongLongFromObjectOrReply(c, c->argv[argi], &id_count, NULL) == C_ERR) { + return; + } + if (id_count <= 0) { + addReplyError(c, "The IDs argument must be a positive integer"); + return; + } + argi++; /* past numids */ + + /* Validate numids matches remaining arg count. */ + long long actual_ids = c->argc - argi; + if (id_count != actual_ids) { + addReplyErrorObject(c, shared.syntaxerr); + return; + } + + /* If missing stream or group, return -1 for each ID. */ + if (o == NULL || group == NULL) { + addReplyArrayLen(c, id_count); + for (long long i = 0; i < id_count; i++) { + addReplyLongLong(c, -1); + } + return; + } + + stream *s = objectGetVal(o); + + /* Start parsing the IDs, so that we abort ASAP if there is a syntax + * error: the return value of this command cannot be an error in case + * the client successfully acknowledged some messages, so it should be + * executed in a "all or nothing" fashion. */ + streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; + streamID *ids = static_ids; + int static_resps[STREAMID_STATIC_VECTOR_LEN]; + int *resps = static_resps; + if (id_count > STREAMID_STATIC_VECTOR_LEN) { + ids = zmalloc(sizeof(streamID) * id_count); + resps = zmalloc(sizeof(int) * id_count); + } + for (long long j = argi; j < c->argc; j++) { + if (streamParseStrictIDOrReply(c, c->argv[j], &ids[j - argi], 0, NULL) != C_OK) goto cleanup; + + /* Default to 1 for ack'd and deleted. If we discover the message doesn't exist or if it is + * not acked by all consumers (in ACKED mode), then we change the response code. */ + resps[j - argi] = 1; + } + + int acked = 0; + int deleted = 0; + int first_entry = 0; + + /* Fast path for KEEPREF. Since we only need to cleanup the PEL for the target + * group, we only need to loop over messages (and not consumers) and can set + * responses inline. Thus, there's a separate setup for KEEPREF vs. ACKED/DELREF*/ + if (mode == 0) { /* KEEPREF */ + for (long long j = 0; j < id_count; j++) { + int response = -1; + streamID *id = &ids[j]; + unsigned char buf[sizeof(streamID)]; + streamEncodeID(buf, id); + + /* ACK for the target group (but not others) */ + void *result; + if (raxFind(group->pel, buf, sizeof(buf), &result)) { + streamNACK *nack = result; + raxRemove(group->pel, buf, sizeof(buf), NULL); + raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); + streamFreeNACK(nack); + response = 1; + acked++; + + /* Delete the message */ + if (streamDeleteItem(s, id)) { + deleted++; + } + + /* We want to know if the first entry in the stream was deleted + * so we can later set the new one. */ + if (streamCompareID(id, &s->first_id) == 0) { + first_entry = 1; + } + + /* Update the stream's maximal tombstone if needed. */ + if (streamCompareID(id, &s->max_deleted_entry_id) > 0) { + s->max_deleted_entry_id = *id; + } + } + + resps[j] = response; + } + + goto sync; + } + + /* For ACKED & DELREF modes we use two phases. + * + * Phase 1: Check the target group. If a stream message isn't in the target + * group, we don't need to check other groups. + * + * Phase 2: Loops over other groups to do DELREF cleanup and ACKED blocking + * (but only for the messages that were present in the target group PEL + * from the checks in Phase 1). + * + * The target group decides eligibility first: in DELREF this prevents + * clearing a non-target's PEL entry before the target is confirmed to hold + * the message, and in ACKED it ensures non-targets are only consulted to + * block deletion of messages the target is acking. */ + for (long long j = 0; j < id_count; j++) { + streamID *id = &ids[j]; + unsigned char buf[sizeof(streamID)]; + streamEncodeID(buf, id); + + void *result; + if (raxFind(group->pel, buf, sizeof(buf), &result)) { + streamNACK *nack = result; + raxRemove(group->pel, buf, sizeof(buf), NULL); + raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); + streamFreeNACK(nack); + acked++; + /* resps[j] stays 1: eligible for deletion (ACKED may still block it). */ + } else { + resps[j] = -1; /* Never delivered / already acked / doesn't exist. */ + } + } + + if (s->cgroups != NULL) { + raxIterator ri_cgroups; + raxStart(&ri_cgroups, s->cgroups); + raxSeek(&ri_cgroups, "^", NULL, 0); + while (raxNext(&ri_cgroups)) { + streamCG *cg = ri_cgroups.data; + if (cg == group) { + /* Handled above in Phase 1. */ + continue; + } + + for (long long j = 0; j < id_count; j++) { + if (resps[j] != 1) { + /* Skip when message wasn't found in target group (-1) + * or when the message can't be deleted b/c of ACKED (2). */ + continue; + } + + streamID *id = &ids[j]; + unsigned char buf[sizeof(streamID)]; + streamEncodeID(buf, id); + + void *result; + if (raxFind(cg->pel, buf, sizeof(buf), &result)) { + if (mode == 1) { /* DELREF */ + streamNACK *nack = result; + raxRemove(cg->pel, buf, sizeof(buf), NULL); + raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); + streamFreeNACK(nack); + acked++; + } else { /* ACKED */ + /* Another group still has it pending. */ + resps[j] = 2; + } + } else if (mode == 2 && + streamCompareID(id, &cg->last_id) > 0) { /* ACKED */ + /* Non-target hasn't claimed it yet; may still need to + * deliver it, so block deletion. */ + resps[j] = 2; + } + } + } + raxStop(&ri_cgroups); + } + + /* Based on the response calculated above for each stream message, delete + * the messages if needed. + * + * This step doesn't enqueue the response yet, because we need to do stream + * metadata bookkeeping and send signals first to meet the module keyspace + * API contract (matching xdel, xtrim & other stream commands, see + * issue #3429). */ + for (long long j = 0; j < id_count; j++) { + /* Delete the message if needed. */ + if (resps[j] == 1) { + streamID *id = &ids[j]; + if (streamDeleteItem(s, id)) { + deleted++; + } + + /* We want to know if the first entry in the stream was deleted + * so we can later set the new one. */ + if (streamCompareID(id, &s->first_id) == 0) { + first_entry = 1; + } + + /* Update the stream's maximal tombstone if needed. */ + if (streamCompareID(id, &s->max_deleted_entry_id) > 0) { + s->max_deleted_entry_id = *id; + } + } + } + +sync: + /* Update the stream's first ID. */ + if (deleted) { + if (s->length == 0) { + s->first_id.ms = 0; + s->first_id.seq = 0; + } else if (first_entry) { + streamGetEdgeID(s, 1, 1, &s->first_id); + } + } + + /* Propagate the write if needed. */ + if (deleted) { + signalModifiedKey(c, c->db, c->argv[1]); + notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); + server.dirty += deleted; + } + + /* PEL entries were removed even without stream deletion; mark dirty so + * the command is propagated to replicas and written to AOF. */ + if (acked) { + server.dirty += acked; + } + + /* Emit the array of per-ID results after the mutation has been signaled. */ + addReplyArrayLen(c, id_count); + for (long long j = 0; j < id_count; j++) { + addReplyLongLong(c, resps[j]); + } + +cleanup: + if (ids != static_ids) zfree(ids); + if (resps != static_resps) zfree(resps); +} + /* General form: XTRIM [... options ...] * * List of options: diff --git a/tests/unit/type/stream-cgroups.tcl b/tests/unit/type/stream-cgroups.tcl index 047defecf74..fb6ef8717dd 100644 --- a/tests/unit/type/stream-cgroups.tcl +++ b/tests/unit/type/stream-cgroups.tcl @@ -1479,6 +1479,104 @@ start_server { } } + start_server {tags {"external:skip"}} { + set master [srv -1 client] + set master_host [srv -1 host] + set master_port [srv -1 port] + set replica [srv 0 client] + + test {XACKDEL replication: ack-only (no deletion) propagates PEL removal to replica} { + $replica replicaof $master_host $master_port + wait_for_condition 50 100 { + [s 0 master_link_status] eq {up} + } else { + fail "Replication not started." + } + + # Two groups both read the message so grp2 blocks deletion in ACKED mode + $master DEL stream + $master XADD stream 1-0 f v + $master XGROUP CREATE stream grp1 0 + $master XGROUP CREATE stream grp2 0 + $master XREADGROUP GROUP grp1 alice COUNT 1 STREAMS stream > + $master XREADGROUP GROUP grp2 bob COUNT 1 STREAMS stream > + + wait_for_ofs_sync $master $replica + + # Replica should have the pending entry in grp1 before ack + assert_equal [llength [$replica XPENDING stream grp1 - + 10]] 1 + + # ACKED mode: grp2 still has entry in PEL so deletion is suppressed + $master XACKDEL stream grp1 ACKED IDS 1 1-0 + + wait_for_ofs_sync $master $replica + + # grp1's PEL entry should be gone, stream entry still present on replica + assert_equal [llength [$replica XPENDING stream grp1 - + 10]] 0 + assert_equal [llength [$replica XRANGE stream - +]] 1 + } + + test {XACKDEL replication: ACKED mode deletion propagates stream removal to replica} { + $replica replicaof $master_host $master_port + wait_for_condition 50 100 { + [s 0 master_link_status] eq {up} + } else { + fail "Replication not started." + } + + $master DEL stream + $master XADD stream 1-0 f v + $master XGROUP CREATE stream grp 0 + $master XREADGROUP GROUP grp alice COUNT 1 STREAMS stream > + + wait_for_ofs_sync $master $replica + + # Replica should have entry in stream and PEL before ack + assert_equal [llength [$replica XRANGE stream - +]] 1 + assert_equal [llength [$replica XPENDING stream grp - + 10]] 1 + + # ACKED mode with only one group: triggers deletion + $master XACKDEL stream grp ACKED IDS 1 1-0 + + wait_for_ofs_sync $master $replica + + # Both PEL entry and stream entry should be gone on replica + assert_equal [llength [$replica XPENDING stream grp - + 10]] 0 + assert_equal [llength [$replica XRANGE stream - +]] 0 + } + + test {XACKDEL replication: DELREF clears other groups' PELs on replica} { + $replica replicaof $master_host $master_port + wait_for_condition 50 100 { + [s 0 master_link_status] eq {up} + } else { + fail "Replication not started." + } + + $master DEL stream + $master XADD stream 1-0 f v + $master XGROUP CREATE stream grp1 0 + $master XGROUP CREATE stream grp2 0 + $master XREADGROUP GROUP grp1 alice COUNT 1 STREAMS stream > + $master XREADGROUP GROUP grp2 bob COUNT 1 STREAMS stream > + + wait_for_ofs_sync $master $replica + + # Both groups should have the entry in their PEL on replica + assert_equal [llength [$replica XPENDING stream grp1 - + 10]] 1 + assert_equal [llength [$replica XPENDING stream grp2 - + 10]] 1 + + # DELREF: ack for grp1, force-delete from stream and clear all groups' PELs + $master XACKDEL stream grp1 DELREF IDS 1 1-0 + + wait_for_ofs_sync $master $replica + + # Stream entry and grp2's PEL entry should both be gone on replica + assert_equal [llength [$replica XRANGE stream - +]] 0 + assert_equal [llength [$replica XPENDING stream grp2 - + 10]] 0 + } + } + start_server {tags {"stream needs:debug"} overrides {appendonly yes aof-use-rdb-preamble no}} { test {Empty stream with no lastid can be rewrite into AOF correctly} { r XGROUP CREATE mystream group-name $ MKSTREAM diff --git a/tests/unit/type/stream.tcl b/tests/unit/type/stream.tcl index 1c8aa533f2f..49bed58a9bf 100644 --- a/tests/unit/type/stream.tcl +++ b/tests/unit/type/stream.tcl @@ -634,6 +634,478 @@ start_server { } } + test {XACKDEL returns syntax error when IDS token is missing after mode} { + r DEL teststream + r XADD teststream 1 msg hello + r XGROUP CREATE teststream testgrp 0 + assert_error "*syntax error*" {r XACKDEL teststream testgrp KEEPREF NOIDS 1 1} + assert_error "*syntax error*" {r XACKDEL teststream testgrp DELREF NOIDS 1 1} + assert_error "*syntax error*" {r XACKDEL teststream testgrp ACKED NOIDS 1 1} + } + + test {XACKDEL wrong number of args} { + assert_error {*wrong number of arguments*} {r XACKDEL s} + assert_error {*wrong number of arguments*} {r XACKDEL s grp} + } + + test {XACKDEL w/ KEEPREF keeps refs in other consumer groups' PEL} { + r DEL testxadstream + r XADD testxadstream 1 msg hello + r XGROUP CREATE testxadstream testxadgrp1 1 + r XGROUP CREATE testxadstream testxadgrp2 1 + r XADD testxadstream 2 msg2 hello2 + + # Setup consumer groups w/ message in both groups PEL + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + r XREADGROUP GROUP testxadgrp2 testxadcnsmr COUNT 1 STREAMS testxadstream > + + # Group 1 + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2-0 [lindex $pend 1] + set ids [r XACKDEL testxadstream testxadgrp1 KEEPREF IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal {} [r xrange testxadstream 2 2] + + # Group 2 still has ref in PEL + set pend [r XPENDING testxadstream testxadgrp2] + assert_equal 2-0 [lindex $pend 1] + } + + test {XACKDEL uses KEEPREF by default} { + r DEL testxadstream + r XADD testxadstream 1 msg hello + r XGROUP CREATE testxadstream testxadgrp1 1 + r XGROUP CREATE testxadstream testxadgrp2 1 + r XADD testxadstream 2 msg2 hello2 + + # Setup consumer groups w/ message in both groups PEL + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + r XREADGROUP GROUP testxadgrp2 testxadcnsmr COUNT 1 STREAMS testxadstream > + + # Group 1 + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2-0 [lindex $pend 1] + set ids [r XACKDEL testxadstream testxadgrp1 IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal {} [r xrange testxadstream 2 2] + + # Group 2 still has ref in PEL + set pend [r XPENDING testxadstream testxadgrp2] + assert_equal 2-0 [lindex $pend 1] + } + + test {XACKDEL w/ ACKED doesn't delete when 2nd consumer group has message in PEL} { + r DEL testxadstream + r XADD testxadstream 1 msg hello + r XGROUP CREATE testxadstream testxadgrp1 1 + r XGROUP CREATE testxadstream testxadgrp2 1 + r XADD testxadstream 2 msg2 hello2 + + # Setup consumer groups w/ message in both groups PEL + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + r XREADGROUP GROUP testxadgrp2 testxadcnsmr COUNT 1 STREAMS testxadstream > + + # Group 1 w/ ACKED only ack's, doesn't delete b/c group2 still hasn't gotten there + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2-0 [lindex $pend 1] + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal 2 [lindex $ids 0] + assert_equal {{2-0 {msg2 hello2}}} [r xrange testxadstream 2 2] + + # Group 2 w/ ACKED both ack's and deletes now that all groups have ACK'd + set pend [r XPENDING testxadstream testxadgrp2] + assert_equal 2-0 [lindex $pend 1] + set ids [r XACKDEL testxadstream testxadgrp2 ACKED IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal {} [r xrange testxadstream 2 2] + } + + # The claim checking logic uses `last_id`. So using XCLAIM to FORCE setting the LAST_ID + # would naturally affect this. + test {XACKDEL w/ ACKED doesn't delete when 2nd consumer group hasn't claimed message yet} { + r DEL testxadstream + r XADD testxadstream 1 msg hello + r XGROUP CREATE testxadstream testxadgrp1 1 + r XGROUP CREATE testxadstream testxadgrp2 1 + r XADD testxadstream 2 msg2 hello2 + + # Group 1 w/ ACKED only ack's, doesn't delete b/c group2 still hasn't gotten there + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2-0 [lindex $pend 1] + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal 2 [lindex $ids 0] + assert_equal {{2-0 {msg2 hello2}}} [r xrange testxadstream 2 2] + + # Group 2 w/ ACKED both ack's and deletes now that all groups have ACK'd + r XREADGROUP GROUP testxadgrp2 testxadcnsmr COUNT 1 STREAMS testxadstream > + set ids [r XACKDEL testxadstream testxadgrp2 ACKED IDS 1 2-0] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal {} [r xrange testxadstream 2 2] + } + + test {XACKDEL w/ DELREF deletes from stream and 2nd consumer group's PEL even if not ACK'd} { + r DEL testxadstream + r XADD testxadstream 1 msg hello + r XGROUP CREATE testxadstream testxadgrp1 1 + r XGROUP CREATE testxadstream testxadgrp2 1 + r XADD testxadstream 2 msg2 hello2 + + # Setup consumer groups w/ message in both groups PEL + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + r XREADGROUP GROUP testxadgrp2 testxadcnsmr COUNT 1 STREAMS testxadstream > + + # Group 1 w/ DELREF does ACK in group 1, removes from group 1's PEL, and deletes from stream + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2-0 [lindex $pend 1] + set ids [r XACKDEL testxadstream testxadgrp1 DELREF IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal {} [r xrange testxadstream 2 2] + + # And Group 2 still has the message in it's PEL + set pend [r XPENDING testxadstream testxadgrp2] + assert_equal {} [lindex $pend 1] + } + + test {XACKDEL w/ DELREF skips deleting refs when target group never received message} { + r DEL testxadstream + r XADD testxadstream 1-0 msg hello + # grp1 created first and reads the message; grp2 (target) never reads it. + # "grp1" < "grp2" so grp1 is iterated first. + r XGROUP CREATE testxadstream grp1 0 + r XGROUP CREATE testxadstream grp2 0 + r XREADGROUP GROUP grp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + + # Target grp2 never had 1-0 pending -> must reply -1 and not change any state. + set ids [r XACKDEL testxadstream grp2 DELREF IDS 1 1-0] + assert_equal 1 [llength $ids] + assert_equal -1 [lindex $ids 0] + + # Stream entry is still be present. + assert_equal {{1-0 {msg hello}}} [r XRANGE testxadstream 1-0 1-0] + + # grp1's PEL entry is untouched. + set pend [r XPENDING testxadstream grp1] + assert_equal 1-0 [lindex $pend 1] + } + + test {XACKDEL w/ ACKED is a no-op when target group already acked the message} { + r DEL testxadstream + r XADD testxadstream 1-0 msg hello + r XGROUP CREATE testxadstream grp1 0 + r XGROUP CREATE testxadstream grp2 0 + r XREADGROUP GROUP grp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + r XREADGROUP GROUP grp2 testxadcnsmr COUNT 1 STREAMS testxadstream > + # Target grp2 acks the message, dropping it from grp2's PEL. + r XACK testxadstream grp2 1-0 + + # grp2 no longer has 1-0 pending, so reply -1 and dont modify anything. + set ids [r XACKDEL testxadstream grp2 ACKED IDS 1 1-0] + assert_equal 1 [llength $ids] + assert_equal -1 [lindex $ids 0] + assert_equal {{1-0 {msg hello}}} [r XRANGE testxadstream 1-0 1-0] + + # grp1 still holds its PEL entry. + set pend [r XPENDING testxadstream grp1] + assert_equal 1-0 [lindex $pend 1] + } + + test {XACKDEL w/ mix of existing and non-existent messages} { + r DEL testxadstream + r XADD testxadstream 1 msg hello + r XGROUP CREATE testxadstream testxadgrp1 1 + r XGROUP CREATE testxadstream testxadgrp2 1 + r XADD testxadstream 2 msg2 hello2 + + # Setup consumer groups w/ message in both groups PEL + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + r XREADGROUP GROUP testxadgrp2 testxadcnsmr COUNT 1 STREAMS testxadstream > + + # Group 1 + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2-0 [lindex $pend 1] + set ids [r XACKDEL testxadstream testxadgrp1 IDS 4 2 10 99 234] + assert_equal 4 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal -1 [lindex $ids 1] + assert_equal -1 [lindex $ids 2] + assert_equal -1 [lindex $ids 3] + assert_equal {} [r xrange testxadstream 2 2] + + # Group 2 still has ref in PEL + set pend [r XPENDING testxadstream testxadgrp2] + assert_equal 2-0 [lindex $pend 1] + } + + test {XACKDEL multiple IDs some acked some not} { + r DEL testxadstream + r XADD testxadstream 1-0 f v1 + r XADD testxadstream 2-0 f v2 + r XADD testxadstream 3-0 f v3 + r XGROUP CREATE testxadstream testxadgrp1 0 + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 10 STREAMS testxadstream > + + # Delete 1-0 and 3-0; leave 2-0 in stream; 99-0 doesn't exist + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 3 1-0 3-0 99-0] + assert_equal 3 [llength $ids] + assert_equal 1 [lindex $ids 0] ;# 1-0 deleted + assert_equal 1 [lindex $ids 1] ;# 3-0 deleted + assert_equal -1 [lindex $ids 2] ;# 99-0 not found + assert_equal 1 [r XLEN testxadstream] + assert_equal 2-0 [lindex [lindex [r XRANGE testxadstream - +] 0] 0] + } + + test {XACKDEL w/ message not claimed does nothing} { + r DEL testxadstream + r XADD testxadstream 1 msg hello + r XGROUP CREATE testxadstream testxadgrp1 1 + r XGROUP CREATE testxadstream testxadgrp2 1 + r XADD testxadstream 2 msg2 hello2 + + # Group 1 + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 2 99 2] + assert_equal 2 [llength $ids] + assert_equal -1 [lindex $ids 0] + assert_equal -1 [lindex $ids 1] + assert_equal {{2-0 {msg2 hello2}}} [r xrange testxadstream 2 2] + + # Group 2 + set ids [r XACKDEL testxadstream testxadgrp2 ACKED IDS 2 2 99] + assert_equal 2 [llength $ids] + assert_equal -1 [lindex $ids 0] + assert_equal -1 [lindex $ids 1] + assert_equal {{2-0 {msg2 hello2}}} [r xrange testxadstream 2 2] + + # Check stream length + assert_equal 2 [r xlen testxadstream] + } + + test {XACKDEL run multiple times returns -1 after first time} { + r DEL testxadstream + r XADD testxadstream 1 msg hello + r XGROUP CREATE testxadstream testxadgrp1 1 + r XGROUP CREATE testxadstream testxadgrp2 1 + r XADD testxadstream 2 msg2 hello2 + + # Setup consumer groups w/ message in both groups PEL + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + r XREADGROUP GROUP testxadgrp2 testxadcnsmr COUNT 1 STREAMS testxadstream > + + # Group 1 w/ ACKED only ack's, doesn't delete b/c group2 still hasn't gotten there + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2-0 [lindex $pend 1] + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal 2 [lindex $ids 0] + assert_equal {{2-0 {msg2 hello2}}} [r xrange testxadstream 2 2] + + # Group 1 run again now returns -1 + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal -1 [lindex $ids 0] + assert_equal {{2-0 {msg2 hello2}}} [r xrange testxadstream 2 2] + + # Group 2 w/ ACKED both ack's and deletes now that all groups have ACK'd + set pend [r XPENDING testxadstream testxadgrp2] + assert_equal 2-0 [lindex $pend 1] + set ids [r XACKDEL testxadstream testxadgrp2 ACKED IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal {} [r xrange testxadstream 2 2] + } + + test {XACKDEL with non-existent stream and group} { + r DEL testxadstream + + # Missing stream and group + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal -1 [lindex $ids 0] + + # Missing Group + r XADD testxadstream 1 msg hello + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 2] + assert_equal 1 [llength $ids] + assert_equal -1 [lindex $ids 0] + } + + test {XACKDEL should fail if given an invalid stream ID} { + r DEL testxadstream + r XADD testxadstream 1 msg hello + r XGROUP CREATE testxadstream testxadgrp1 1 + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + assert_error "*Invalid stream ID*" {r XACKDEL testxadstream testxadgrp1 IDS 1 not-a-valid-id} + } + + test {XACKDEL should fail if called on a non-stream key} { + r DEL testxadstream + r SET testxadstream notastream + assert_error "*WRONGTYPE*" {r XACKDEL testxadstream testxadgrp1 IDS 1 1-0} + r DEL testxadstream + } + + test {XACKDEL should fail if given an unrecognized mode} { + r DEL testxadstream + r XADD testxadstream 1 msg hello + r XGROUP CREATE testxadstream testxadgrp1 1 + assert_error "*" {r XACKDEL testxadstream testxadgrp1 BADMODE IDS 1 1-0} + } + + test {XACKDEL IDS numids must be a positive integer} { + r DEL testxadstream + r XADD testxadstream 1-0 f v + r XGROUP CREATE testxadstream testxadgrp1 0 + assert_error {*value is not an integer or out of range*} {r XACKDEL testxadstream testxadgrp1 IDS abc 1-0} + assert_error {*The IDs argument must be a positive integer*} {r XACKDEL testxadstream testxadgrp1 IDS 0 1-0} + assert_error {*The IDs argument must be a positive integer*} {r XACKDEL testxadstream testxadgrp1 IDS -5 1-0} + } + + test {XACKDEL IDS numids must match argument count} { + r DEL testxadstream + r XADD testxadstream 1-0 f v + r XGROUP CREATE testxadstream testxadgrp1 0 + assert_error {*syntax error*} {r XACKDEL testxadstream testxadgrp1 IDS 3 1-0 2-0} + assert_error {*syntax error*} {r XACKDEL testxadstream testxadgrp1 IDS 1 1-0 2-0} + } + + test {XACKDEL with more than 8 IDs exercises dynamic allocation} { + r DEL teststream + # STREAMID_STATIC_VECTOR_LEN is 8, use 10 to force zmalloc path + for {set i 1} {$i <= 10} {incr i} { + r XADD teststream $i msg hello + } + r XGROUP CREATE teststream testgrp 0 + r XREADGROUP GROUP testgrp consumer1 COUNT 10 STREAMS teststream > + set ids [r XACKDEL teststream testgrp IDS 10 1 2 3 4 5 6 7 8 9 10] + assert_equal 10 [llength $ids] + foreach id $ids { + assert_equal 1 $id + } + assert_equal 0 [r XLEN teststream] + } + + test {XACKDEL returns -1 for deleted entry not in group PEL} { + r DEL teststream + r XADD teststream 1-0 msg hello + r XGROUP CREATE teststream testgrp 0 + # Message never claimed, so not in any PEL. Delete it from the stream. + r XDEL teststream 1-0 + set res [r XACKDEL teststream testgrp IDS 1 1-0] + assert_equal 1 [llength $res] + assert_equal -1 [lindex $res 0] + } + + test {XACKDEL updates first_id via streamGetEdgeID when first entry is deleted but stream is non-empty} { + r DEL testxadstream + r XADD testxadstream 1-0 msg hello + r XADD testxadstream 2-0 msg2 hello2 + r XGROUP CREATE testxadstream testxadgrp1 0 + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 10 STREAMS testxadstream > + + # Delete 1-0 (the first entry) while 2-0 remains; triggers the streamGetEdgeID path + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 1-0] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + + # 2-0 must still be present and must now be the first entry (first_id updated) + assert_equal 1 [r XLEN testxadstream] + set entries [r XRANGE testxadstream - +] + assert_equal 1 [llength $entries] + assert_equal 2-0 [lindex [lindex $entries 0] 0] + } + + test {XACKDEL DELREF deletes entry when non-target group has not yet claimed it} { + r DEL testxadstream + r XADD testxadstream 1-0 msg hello + r XGROUP CREATE testxadstream testxadgrp1 0 + r XGROUP CREATE testxadstream testxadgrp2 0 + + # Only grp1 claims the message; grp2 has never read it (beyond grp2's last_id) + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + + set ids [r XACKDEL testxadstream testxadgrp1 DELREF IDS 1 1-0] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + + # Entry must be deleted from the stream even though grp2 never had a PEL entry for it + assert_equal 0 [r XLEN testxadstream] + assert_equal {} [r XRANGE testxadstream - +] + set pend2 [r XPENDING testxadstream testxadgrp2] + assert_equal 0 [lindex $pend2 0] + } + + test {XACKDEL ACKED returns 1 when stream entry was already removed via XDEL} { + r DEL testxadstream + r XADD testxadstream 1-0 f v + r XGROUP CREATE testxadstream testxadgrp1 0 + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + + # XDEL removes the stream entry but leaves the PEL entry intact + r XDEL testxadstream 1-0 + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 1 [lindex $pend 0] + + # XACKDEL clears the PEL and still returns 1 even though already deleted + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 1-0] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + + # Check that XACKDEL ACKED removes the dangling PEL entry left after XDEL + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 0 [lindex $pend 0] + } + + test {XACKDEL ACKED with a single consumer group deletes the entry} { + r DEL testxadstream + r XADD testxadstream 1-0 f v + r XGROUP CREATE testxadstream testxadgrp1 0 + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 1-0] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal 0 [r XLEN testxadstream] + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 0 [lindex $pend 0] + } + + test {XACKDEL ACKED drains PEL despite XGROUP SETID moving last_id backward} { + r DEL testxadstream + r XADD testxadstream 1-0 f v1 + r XADD testxadstream 5-0 f v5 + r XGROUP CREATE testxadstream testxadgrp1 0 + + # Claim both entries: last_id advances to 5-0 and both land in the PEL. + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 10 STREAMS testxadstream > + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2 [lindex $pend 0] + assert_equal 5-0 [lindex $pend 2] + + # Move last_id back before the latest claimed entry. 5-0 stays in the + # PEL even though id > last_id now. + r XGROUP SETID testxadstream testxadgrp1 1-0 + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2 [lindex $pend 0] + + # ACKED must still drain the dangling 5-0 PEL entry and delete the msg. + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 5-0] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal {} [r xrange testxadstream 5-0 5-0] + + # 5-0 should be gone from the PEL; 1-0 remains pending. + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 1 [lindex $pend 0] + assert_equal 1-0 [lindex $pend 1] + } + test {XRANGE fuzzing} { set items [r XRANGE mystream{t} - +] set low_id [lindex $items 0 0] From f93c54c839f61012ad91fad695e3ce678c2a2c12 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Tue, 8 Sep 2026 18:41:16 -0400 Subject: [PATCH 02/24] Implement XDELEX & Address Code Reviews Squashed commit of the following: commit 8e5e20298766bc6e9232557b6142a87bf2052400 Author: Nick Iaquinto Date: Tue Sep 8 18:13:11 2026 -0400 Simplify & Standardize Error Handling (RE: Review Comments) Responding to [these review comments](https://github.com/valkey-io/valkey/pull/3467#pullrequestreview-5143209449), 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 commit 27cf7bc9425c2d36f24949549c3e2c1a3e1eb84b Author: Nick Iaquinto Date: Thu Jul 23 21:13:38 2026 -0400 Clang Format Fix (remove trailing newline) Signed-off-by: Nick Iaquinto commit 703506c652926d63dea0695e60ee146ff9cc1694 Author: Nick Iaquinto 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 commit 0fc54c5623106138fb4d8637a5bd8079403c13ed Author: Nick Iaquinto 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 #3429. Signed-off-by: Nick Iaquinto commit 20f2d99e89837ed1105c451ee354e34741b0830b Author: Nick Iaquinto 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 commit 66a85557b7f92576dd99ea248673ea9ce6f047db Author: Nick Iaquinto 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 Signed-off-by: Nick Iaquinto --- src/commands.def | 36 ++++ src/commands/xdelex.json | 94 ++++++++++ src/server.h | 2 + src/t_stream.c | 200 ++++++++++++++++++++ tests/unit/type/stream-cgroups.tcl | 102 ++++++++++ tests/unit/type/stream.tcl | 290 ++++++++++++++++++++++++++++- 6 files changed, 721 insertions(+), 3 deletions(-) create mode 100644 src/commands/xdelex.json diff --git a/src/commands.def b/src/commands.def index 59aa67faad8..446bbf5271f 100644 --- a/src/commands.def +++ b/src/commands.def @@ -10648,6 +10648,41 @@ struct COMMAND_ARG XDEL_Args[] = { {MAKE_ARG("id",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_MULTIPLE,0,NULL)}, }; +/********** XDELEX ********************/ + +#ifndef SKIP_CMD_HISTORY_TABLE +/* XDELEX history */ +#define XDELEX_History NULL +#endif + +#ifndef SKIP_CMD_TIPS_TABLE +/* XDELEX tips */ +#define XDELEX_Tips NULL +#endif + +#ifndef SKIP_CMD_KEY_SPECS_TABLE +/* XDELEX key specs */ +keySpec XDELEX_Keyspecs[1] = { +{NULL,CMD_KEY_RW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}} +}; +#endif + +/* XDELEX mode argument table */ +struct COMMAND_ARG XDELEX_mode_Subargs[] = { +{MAKE_ARG("keepref",ARG_TYPE_PURE_TOKEN,-1,"KEEPREF",NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("delref",ARG_TYPE_PURE_TOKEN,-1,"DELREF",NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("acked",ARG_TYPE_PURE_TOKEN,-1,"ACKED",NULL,NULL,CMD_ARG_NONE,0,NULL)}, +}; + +/* XDELEX argument table */ +struct COMMAND_ARG XDELEX_Args[] = { +{MAKE_ARG("key",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("mode",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,3,NULL),.subargs=XDELEX_mode_Subargs}, +{MAKE_ARG("ids",ARG_TYPE_PURE_TOKEN,-1,"IDS",NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("numids",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("id",ARG_TYPE_STRING,-1,"IDS",NULL,NULL,CMD_ARG_MULTIPLE,0,NULL)}, +}; + /********** XGROUP CREATE ********************/ #ifndef SKIP_CMD_HISTORY_TABLE @@ -12255,6 +12290,7 @@ struct COMMAND_STRUCT serverCommandTable[] = { {MAKE_CMD("xautoclaim","Changes, or acquires, ownership of messages in a consumer group, as if the messages were delivered to a consumer group member.","O(1) if COUNT is small.","6.2.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XAUTOCLAIM_History,1,XAUTOCLAIM_Tips,1,xautoclaimCommand,-6,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XAUTOCLAIM_Keyspecs,1,NULL,7),.args=XAUTOCLAIM_Args}, {MAKE_CMD("xclaim","Changes, or acquires, ownership of a message in a consumer group, as if the message was delivered to a consumer group member.","O(log N) with N being the number of messages in the PEL of the consumer group.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XCLAIM_History,0,XCLAIM_Tips,1,xclaimCommand,-6,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XCLAIM_Keyspecs,1,NULL,11),.args=XCLAIM_Args}, {MAKE_CMD("xdel","Returns the number of messages after removing them from a stream.","O(1) for each single item to delete in the stream, regardless of the stream size.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XDEL_History,0,XDEL_Tips,0,xdelCommand,-3,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XDEL_Keyspecs,1,NULL,2),.args=XDEL_Args}, +{MAKE_CMD("xdelex","Delete stream message(s) with extended options","O(1)","9.2.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XDELEX_History,0,XDELEX_Tips,0,xdelexCommand,-5,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_WRITE|ACL_CATEGORY_STREAM,NULL,XDELEX_Keyspecs,1,NULL,5),.args=XDELEX_Args}, {MAKE_CMD("xgroup","A container for consumer groups commands.","Depends on subcommand.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XGROUP_History,0,XGROUP_Tips,0,NULL,-2,0,ACL_CATEGORY_SLOW,NULL,XGROUP_Keyspecs,0,NULL,0),.subcommands=XGROUP_Subcommands}, {MAKE_CMD("xinfo","A container for stream introspection commands.","Depends on subcommand.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XINFO_History,0,XINFO_Tips,0,NULL,-2,0,ACL_CATEGORY_SLOW,NULL,XINFO_Keyspecs,0,NULL,0),.subcommands=XINFO_Subcommands}, {MAKE_CMD("xlen","Returns the number of messages in a stream.","O(1)","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XLEN_History,0,XLEN_Tips,0,xlenCommand,2,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_STREAM,NULL,XLEN_Keyspecs,1,NULL,1),.args=XLEN_Args}, diff --git a/src/commands/xdelex.json b/src/commands/xdelex.json new file mode 100644 index 00000000000..e82fba323b3 --- /dev/null +++ b/src/commands/xdelex.json @@ -0,0 +1,94 @@ +{ + "XDELEX": { + "summary": "Delete stream message(s) with extended options", + "complexity": "O(1)", + "group": "stream", + "since": "9.2.0", + "arity": -5, + "function": "xdelexCommand", + "command_flags": [ + "WRITE", + "FAST" + ], + "acl_categories": [ + "FAST", + "WRITE", + "STREAM" + ], + "key_specs": [ + { + "flags": [ + "RW", + "UPDATE" + ], + "begin_search": { + "index": { + "pos": 1 + } + }, + "find_keys": { + "range": { + "lastkey": 0, + "step": 1, + "limit": 0 + } + } + } + ], + "arguments": [ + { + "name": "key", + "type": "key", + "key_spec_index": 0 + }, + { + "name": "mode", + "type": "oneof", + "optional": true, + "arguments": [ + { + "name": "keepref", + "type": "pure-token", + "token": "KEEPREF" + }, + { + "name": "delref", + "type": "pure-token", + "token": "DELREF" + }, + { + "name": "acked", + "type": "pure-token", + "token": "ACKED" + } + ] + }, + { + "name": "ids", + "token": "IDS", + "type": "pure-token" + }, + { + "name": "numids", + "type": "integer" + }, + { + "name": "id", + "token": "IDS", + "type": "string", + "multiple": true + } + ], + "reply_schema": { + "description": "The command returns an integer for each stream message: -1=message not found, 1=message was deleted, 2=message was not deleted due to existing references (ACKED mode).", + "type": "array", + "minItems": 0, + "items": { + "description": "Status of the stream message, -1=message not found, 1=message was deleted, 2=message was not deleted due to existing references (ACKED mode).", + "type": "integer", + "minimum": -1, + "maximum": 2 + } + } + } +} diff --git a/src/server.h b/src/server.h index 4b3f66be991..a34b956f7af 100644 --- a/src/server.h +++ b/src/server.h @@ -4339,6 +4339,7 @@ void xautoclaimCommand(client *c); void xinfoCommand(client *c); void xdelCommand(client *c); void xackdelCommand(client *c); +void xdelexCommand(client *c); void xtrimCommand(client *c); void lolwutCommand(client *c); void aclCommand(client *c); @@ -4424,3 +4425,4 @@ int iAmPrimary(void); #define STRINGIFY(x) STRINGIFY_(x) #endif + diff --git a/src/t_stream.c b/src/t_stream.c index 5488f7a7fbf..54db40d4dde 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -3613,6 +3613,206 @@ void xdelCommand(client *c) { if (ids != static_ids) zfree(ids); } +/* XDELEX [KEEPREF | DELREF | ACKED] IDS num [ ... ] + */ +void xdelexCommand(client *c) { + robj *o = lookupKeyRead(c->db, c->argv[1]); + if (o) { + if (checkType(c, o, OBJ_STREAM)) return; /* Type error. */ + } + + /* Check what mode is set, if any. + * ex. [KEEPREF | DELREF | ACKED] IDS n [ID ...] + */ + int argi = 2; + int mode = 0; /* 0=keepref, 1=delref, 2=acked */ + if (strcasecmp(objectGetVal(c->argv[argi]), "KEEPREF") == 0) { + argi += 1; + mode = 0; + } else if (strcasecmp(objectGetVal(c->argv[argi]), "DELREF") == 0) { + argi += 1; + mode = 1; + } else if (strcasecmp(objectGetVal(c->argv[argi]), "ACKED") == 0) { + argi += 1; + mode = 2; + } + + /* Expect IDS token. */ + if (strcasecmp(objectGetVal(c->argv[argi]), "IDS") != 0) { + addReplyErrorObject(c, shared.syntaxerr); + return; + } + argi++; /* past IDS */ + + /* Parse and validate numids: must be a positive integer. */ + long long id_count; + if (getLongLongFromObject(c->argv[argi], &id_count) != C_OK || id_count <= 0) { + addReplyError(c, "Number of IDs must be a positive integer"); + return; + } + argi++; /* past numids */ + + /* Validate numids matches remaining arg count. */ + long long actual_ids = c->argc - argi; + if (id_count != actual_ids) { + addReplyErrorObject(c, shared.syntaxerr); + return; + } + + /* Start parsing the IDs, so that we abort ASAP if there is a syntax + * error: the return value of this command cannot be an error in case + * the client successfully acknowledged some messages, so it should be + * executed in a "all or nothing" fashion. */ + streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; + int static_resps[STREAMID_STATIC_VECTOR_LEN]; + streamID *ids = static_ids; + int *resps = static_resps; + if (id_count > STREAMID_STATIC_VECTOR_LEN) { + ids = zmalloc(sizeof(streamID) * id_count); + resps = zmalloc(sizeof(int) * id_count); + } + for (int j = argi; j < c->argc; j++) { + if (streamParseStrictIDOrReply(c, c->argv[j], &ids[j - argi], 0, NULL) != C_OK) goto cleanup; + + /* Default to 1 (will be deleted). Changed to 2 (can't delete yet) or -1 + * (not found) if we discover a blocking condition. */ + resps[j - argi] = 1; + } + + /* If missing stream, return -1 for each ID. */ + if (o == NULL) { + addReplyArrayLen(c, id_count); + for (int i = 0; i < id_count; i++) { + addReplyLongLong(c, -1); + } + goto cleanup; + } + + stream *s = objectGetVal(o); + + /* True if DELREF removed any PEL references. PEL-only changes still modify + * the stream metadata, so they should signal WATCH/tracking and emit a + * keyspace event just like a stream-entry deletion (see issue #3429). */ + bool pel_modified = 0; + + /* For ACKED and DELREF modes: loop over consumer groups (outer) then messages + * (inner). This opens the iterator once instead of once per message, and + * allows inner-loop skips via the resps array. */ + if ((mode == 1 || mode == 2) && s->cgroups != NULL) { + bool first_loop = 1; + raxIterator ri_cgroups; + raxStart(&ri_cgroups, s->cgroups); + raxSeek(&ri_cgroups, "^", NULL, 0); + while (raxNext(&ri_cgroups)) { + streamCG *cg = ri_cgroups.data; + + for (int j = 0; j < id_count; j++) { + /* Skip messages already finalized. For ACKED, 2 means another + * group already has a pending ref so deletion is blocked. */ + if (resps[j] == -1) continue; + if (mode == 2 && resps[j] == 2) continue; + + streamID *id = &ids[j]; + unsigned char buf[sizeof(streamID)]; + streamEncodeID(buf, id); + + /* Group hasn't claimed this message yet; it can't have a PEL + * entry for it either, so there's nothing to remove. */ + if (streamCompareID(id, &cg->last_id) > 0) { + if (mode == 2) { + /* ACKED: can't delete until this group has seen it. */ + resps[j] = streamEntryExists(s, id) ? 2 : -1; + } + continue; + } + + void *result; + if (raxFind(cg->pel, buf, sizeof(buf), &result)) { + if (mode == 1) { + /* DELREF: remove PEL entry from this group. */ + streamNACK *nack = result; + raxRemove(cg->pel, buf, sizeof(buf), NULL); + raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); + streamFreeNACK(nack); + server.dirty++; + pel_modified = 1; + } else { + /* ACKED: still pending in this group, cannot delete. */ + resps[j] = 2; + } + } else if (mode == 2 && first_loop && !streamEntryExists(s, id)) { + /* Message doesn't exist in the stream; check once and skip + * iterating the remaining groups. */ + resps[j] = -1; + } + } + first_loop = 0; + } + raxStop(&ri_cgroups); + } + + /* Based on the response calculated above for each stream message, delete + * the messages if needed (catching not found messages and adjusting the + * response). + * + * This step doesn't enqueue the response yet, because we need to do stream + * metadata bookkeeping and send signals first to meet the module keyspace + * API contract (matching xdel, xtrim & other stream commands, see + * issue #3429). */ + int deleted = 0; + bool first_entry = 0; + for (int j = 0; j < id_count; j++) { + if (resps[j] == 1) { + streamID *id = &ids[j]; + if (streamDeleteItem(s, id)) { + /* Track whether the first stream entry was removed so we can + * update s->first_id below. */ + if (streamCompareID(id, &s->first_id) == 0) { + first_entry = 1; + } + /* Update the stream's maximal tombstone if needed. */ + if (streamCompareID(id, &s->max_deleted_entry_id) > 0) { + s->max_deleted_entry_id = *id; + } + deleted++; + } else { + /* If the message does not exist, use -1 response code. + * Necessary here b/c in KEEPREF mode, we skip checking above. */ + resps[j] = -1; + } + } + } + + /* Update the stream's first ID. */ + if (deleted) { + if (s->length == 0) { + s->first_id.ms = 0; + s->first_id.seq = 0; + } else if (first_entry) { + streamGetEdgeID(s, 1, 1, &s->first_id); + } + } + + /* Either deleting entries or a PEL-only change mutate consumer-group state + * on this key, so we need to signal in either case to WATCH-ers & keyspace + * subscribers (see issue #3429). */ + if (deleted || pel_modified) { + signalModifiedKey(c, c->db, c->argv[1]); + notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); + server.dirty += deleted; + } + + /* Emit the array of per-ID results after the mutation has been signaled. */ + addReplyArrayLen(c, id_count); + for (int j = 0; j < id_count; j++) { + addReplyLongLong(c, resps[j]); + } + +cleanup: + if (ids != static_ids) zfree(ids); + if (resps != static_resps) zfree(resps); +} + /* XACKDEL [KEEPREF | DELREF | ACKED] IDS num [ ... ] * * Acknowledge one or more messages and delete them if all consumer groups have diff --git a/tests/unit/type/stream-cgroups.tcl b/tests/unit/type/stream-cgroups.tcl index fb6ef8717dd..7001a16f22e 100644 --- a/tests/unit/type/stream-cgroups.tcl +++ b/tests/unit/type/stream-cgroups.tcl @@ -1577,6 +1577,108 @@ start_server { } } + start_server {tags {"external:skip"}} { + set master [srv -1 client] + set master_host [srv -1 host] + set master_port [srv -1 port] + set replica [srv 0 client] + + test {XDELEX replication: KEEPREF deletes stream entry but keeps dangling PEL ref on replica} { + $replica replicaof $master_host $master_port + wait_for_condition 50 100 { + [s 0 master_link_status] eq {up} + } else { + fail "Replication not started." + } + + $master DEL stream + $master XADD stream 1-0 f v + $master XADD stream 2-0 f v + $master XGROUP CREATE stream grp 0 + $master XREADGROUP GROUP grp alice COUNT 1 STREAMS stream > + + wait_for_ofs_sync $master $replica + + # Replica has both entries and grp's PEL contains 1-0 + assert_equal 2 [$replica XLEN stream] + assert_equal 1 [llength [$replica XPENDING stream grp - + 10]] + + # KEEPREF: deletes entry from stream but leaves PEL reference intact + $master XDELEX stream KEEPREF IDS 1 1-0 + + wait_for_ofs_sync $master $replica + + # Stream entry gone on replica, PEL reference still present + assert_equal 1 [$replica XLEN stream] + assert_equal 1 [llength [$replica XPENDING stream grp - + 10]] + } + + test {XDELEX replication: DELREF deletes stream entry and clears PEL on replica} { + $replica replicaof $master_host $master_port + wait_for_condition 50 100 { + [s 0 master_link_status] eq {up} + } else { + fail "Replication not started." + } + + $master DEL stream + $master XADD stream 1-0 f v + $master XADD stream 2-0 f v + $master XGROUP CREATE stream grp 0 + $master XREADGROUP GROUP grp alice COUNT 1 STREAMS stream > + + wait_for_ofs_sync $master $replica + + # Replica has both entries and grp's PEL contains 1-0 + assert_equal 2 [$replica XLEN stream] + assert_equal 1 [llength [$replica XPENDING stream grp - + 10]] + + # DELREF: deletes entry from stream AND removes it from all PELs + $master XDELEX stream DELREF IDS 1 1-0 + + wait_for_ofs_sync $master $replica + + # Stream entry gone and PEL cleared on replica + assert_equal 1 [$replica XLEN stream] + assert_equal 0 [llength [$replica XPENDING stream grp - + 10]] + } + + test {XDELEX replication: ACKED skips pending entries, deletes only after all groups ack} { + $replica replicaof $master_host $master_port + wait_for_condition 50 100 { + [s 0 master_link_status] eq {up} + } else { + fail "Replication not started." + } + + $master DEL stream + $master XADD stream 1-0 f v + $master XGROUP CREATE stream grp 0 + $master XREADGROUP GROUP grp alice COUNT 1 STREAMS stream > + + wait_for_ofs_sync $master $replica + + # Entry is pending; ACKED mode should not delete it + $master XDELEX stream ACKED IDS 1 1-0 + + wait_for_ofs_sync $master $replica + + # Entry still present on replica since it was not acked + assert_equal 1 [$replica XLEN stream] + assert_equal 1 [llength [$replica XPENDING stream grp - + 10]] + + # Now ack the entry and retry XDELEX ACKED + $master XACK stream grp 1-0 + $master XDELEX stream ACKED IDS 1 1-0 + + wait_for_ofs_sync $master $replica + + # Entry deleted on replica after all groups have acked + assert_equal 0 [$replica XLEN stream] + assert_equal 0 [llength [$replica XPENDING stream grp - + 10]] + } + } + start_server {tags {"stream needs:debug"} overrides {appendonly yes aof-use-rdb-preamble no}} { test {Empty stream with no lastid can be rewrite into AOF correctly} { r XGROUP CREATE mystream group-name $ MKSTREAM diff --git a/tests/unit/type/stream.tcl b/tests/unit/type/stream.tcl index 49bed58a9bf..75e12d81670 100644 --- a/tests/unit/type/stream.tcl +++ b/tests/unit/type/stream.tcl @@ -962,9 +962,9 @@ start_server { r DEL testxadstream r XADD testxadstream 1-0 f v r XGROUP CREATE testxadstream testxadgrp1 0 - assert_error {*value is not an integer or out of range*} {r XACKDEL testxadstream testxadgrp1 IDS abc 1-0} - assert_error {*The IDs argument must be a positive integer*} {r XACKDEL testxadstream testxadgrp1 IDS 0 1-0} - assert_error {*The IDs argument must be a positive integer*} {r XACKDEL testxadstream testxadgrp1 IDS -5 1-0} + assert_error {*Number of IDs must be a positive integer*} {r XACKDEL testxadstream testxadgrp1 IDS abc 1-0} + assert_error {*Number of IDs must be a positive integer*} {r XACKDEL testxadstream testxadgrp1 IDS 0 1-0} + assert_error {*Number of IDs must be a positive integer*} {r XACKDEL testxadstream testxadgrp1 IDS -5 1-0} } test {XACKDEL IDS numids must match argument count} { @@ -1106,6 +1106,290 @@ start_server { assert_equal 1-0 [lindex $pend 1] } + test {XDELEX deletes items} { + r DEL teststream + r XADD teststream 1 msg helllo + r XADD teststream 2 msg helllo + r XADD teststream 3 msg helllo + set resp [ r XDELEX teststream IDS 1 1 ] + assert_equal 1 [llength $resp] + assert_equal 1 [lindex $resp 0] + + assert_equal 2 [ r XLEN teststream ] + } + + test {XDELEX on non-existent key returns -1 for each ID} { + r DEL nonexistent + set resp [r XDELEX nonexistent IDS 2 1 2] + assert_equal 2 [llength $resp] + assert_equal -1 [lindex $resp 0] + assert_equal -1 [lindex $resp 1] + } + + test {XDELEX on wrong key type returns error} { + r DEL testset + r SADD testset a b c + assert_error "*WRONGTYPE*" {r XDELEX testset IDS 1 1} + } + + test {XDELEX w/ ACKED only deletes acked items} { + r DEL teststream + r XADD teststream 1 msg helllo + r XADD teststream 2 msg helllo + r XADD teststream 3 msg helllo + + r XGROUP CREATE teststream testgrp1 0 + r XREADGROUP GROUP testgrp1 testconsumer COUNT 1 STREAMS teststream > + r XACK teststream testgrp1 1 + + set ids [ r XDELEX teststream ACKED IDS 3 1 2 99 ] + assert_equal 3 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal 2 [lindex $ids 1] + assert_equal -1 [lindex $ids 2] + + assert_equal 2 [ r XLEN teststream ] + } + + test {XDELEX w/ KEEPREF deletes all but keeps refs in consumer group PELs} { + r DEL teststream + r XADD teststream 1 msg helllo + r XADD teststream 2 msg helllo + r XADD teststream 3 msg helllo + + r XGROUP CREATE teststream testgrp1 0 + r XREADGROUP GROUP testgrp1 testconsumer COUNT 1 STREAMS teststream > + + set ids [ r XDELEX teststream KEEPREF IDS 3 1 2 99 ] + assert_equal 3 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal 1 [lindex $ids 1] + assert_equal -1 [lindex $ids 2] + + assert_equal {1 1-0 1-0 {{testconsumer 1}}} [r XPENDING teststream testgrp1] + + assert_equal 1 [ r XLEN teststream ] + } + + test {XDELEX w/ DELREF deletes entries and clears consumer group PEL refs} { + r DEL teststream + r XADD teststream 1 msg helllo + r XADD teststream 2 msg helllo + r XADD teststream 3 msg helllo + + r XGROUP CREATE teststream testgrp1 0 + r XREADGROUP GROUP testgrp1 testconsumer COUNT 1 STREAMS teststream > + + set ids [ r XDELEX teststream DELREF IDS 3 1 2 99 ] + assert_equal 3 [llength $ids] + assert_equal 1 [lindex $ids 0] + assert_equal 1 [lindex $ids 1] + assert_equal -1 [lindex $ids 2] + + assert_equal {0 {} {} {}} [r XPENDING teststream testgrp1] + + assert_equal 1 [ r XLEN teststream ] + } + + test {XDELEX DELREF signals WATCH when removing an orphaned PEL ref} { + r DEL teststream + r XADD teststream 1-0 msg hello + r XGROUP CREATE teststream grp1 0 + r XREADGROUP GROUP grp1 consumer COUNT 1 STREAMS teststream > + + # Delete the entry but keep its (now orphaned) PEL reference. + r XDELEX teststream KEEPREF IDS 1 1-0 + assert_equal 0 [r XLEN teststream] + assert_equal {1 1-0 1-0 {{consumer 1}}} [r XPENDING teststream grp1] + + # DELREF clearing the orphaned NACK modifies the key even though no + # stream entry is deleted, so the pending MULTI/EXEC must abort and + # return the empty reply. + r WATCH teststream + r XDELEX teststream DELREF IDS 1 1-0 + r MULTI + r ping + assert_equal {} [r EXEC] + + assert_equal {0 {} {} {}} [r XPENDING teststream grp1] + } + + test {XDELEX DELREF removing an orphaned PEL ref emits an xdel keyspace event} { + r DEL teststream + r XADD teststream 1-0 msg hello + r XGROUP CREATE teststream grp1 0 + r XREADGROUP GROUP grp1 consumer COUNT 1 STREAMS teststream > + r XDELEX teststream KEEPREF IDS 1 1-0 + assert_equal {1 1-0 1-0 {{consumer 1}}} [r XPENDING teststream grp1] + + # Enable keyspace notifications: K = keyspace events on __keyspace@:, + # t = stream events (so xdel/XPENDING PEL mutations surface as 'xdel'). + r config set notify-keyspace-events Kt + + # Subscribe with a glob so the channel matches regardless of which DB + # tests are running under. The keyspace channel is __keyspace@: + # so * matches all db numbers. + set rd1 [valkey_deferring_client] + set subpat __keyspace@*:teststream + assert_equal {1} [psubscribe $rd1 $subpat] + + # No stream entry is deleted here, but the PEL change still notifies. + r XDELEX teststream DELREF IDS 1 1-0 + + # The pmessage reply is: pmessage . + # Here, is the xdel event we're looking for. The subscribed + # pattern is the glob from above, and the channel is also wildcarded by + # assert_match to avoid failing depending on the DB number in use for + # tests. + assert_match "pmessage $subpat __keyspace@*:teststream xdel" [$rd1 read] + + $rd1 close + assert_equal {0 {} {} {}} [r XPENDING teststream grp1] + r config set notify-keyspace-events {} + } + + test {XDELEX on already-deleted ID returns -1} { + r DEL teststream + r XADD teststream 1 msg hello + r XADD teststream 2 msg hello + + # First delete succeeds + set resp [r XDELEX teststream IDS 1 1] + assert_equal 1 [lindex $resp 0] + + # Second delete on the same ID returns -1 + set resp [r XDELEX teststream IDS 1 1] + assert_equal -1 [lindex $resp 0] + } + + test {XDELEX should fail on invalid stream ID format} { + r DEL teststream + r XADD teststream 1 msg hello + assert_error "*Invalid stream ID specified*" {r XDELEX teststream IDS 1 not-a-valid-id} + } + + test {XDELEX w/ KEEPREF preserves second consumer group's PEL} { + r DEL teststream + r XADD teststream 1-0 msg hello + r XADD teststream 2-0 msg hello + r XGROUP CREATE teststream grp1 0 + r XGROUP CREATE teststream grp2 0 + r XREADGROUP GROUP grp1 consumer COUNT 1 STREAMS teststream > + r XREADGROUP GROUP grp2 consumer COUNT 1 STREAMS teststream > + + # KEEPREF: delete from stream but leave grp2's PEL reference intact + set ids [r XDELEX teststream KEEPREF IDS 1 1-0] + assert_equal 1 [lindex $ids 0] + + assert_equal {1 1-0 1-0 {{consumer 1}}} [r XPENDING teststream grp2] + assert_equal 1 [r XLEN teststream] + } + + test {XDELEX w/ DELREF clears all consumer groups' PELs} { + r DEL teststream + r XADD teststream 1-0 msg hello + r XADD teststream 2-0 msg hello + r XGROUP CREATE teststream grp1 0 + r XGROUP CREATE teststream grp2 0 + r XREADGROUP GROUP grp1 consumer COUNT 1 STREAMS teststream > + r XREADGROUP GROUP grp2 consumer COUNT 1 STREAMS teststream > + + # DELREF: delete from stream and wipe all groups' PEL references + set ids [r XDELEX teststream DELREF IDS 1 1-0] + assert_equal 1 [lindex $ids 0] + + assert_equal {0 {} {} {}} [r XPENDING teststream grp1] + assert_equal {0 {} {} {}} [r XPENDING teststream grp2] + assert_equal 1 [r XLEN teststream] + } + + test {XDELEX w/ ACKED waits for all consumer groups before deleting} { + r DEL teststream + r XADD teststream 1-0 msg hello + r XGROUP CREATE teststream grp1 0 + r XGROUP CREATE teststream grp2 0 + r XREADGROUP GROUP grp1 consumer COUNT 1 STREAMS teststream > + r XREADGROUP GROUP grp2 consumer COUNT 1 STREAMS teststream > + + # grp1 acks but grp2 still has it in PEL → no deletion + r XACK teststream grp1 1-0 + set ids [r XDELEX teststream ACKED IDS 1 1-0] + assert_equal 2 [lindex $ids 0] + assert_equal 1 [r XLEN teststream] + + # grp2 acks; all groups done → deletion occurs + r XACK teststream grp2 1-0 + set ids [r XDELEX teststream ACKED IDS 1 1-0] + assert_equal 1 [lindex $ids 0] + assert_equal 0 [r XLEN teststream] + } + + test {XDELEX returns syntax error when IDS token is missing after mode} { + r DEL teststream + r XADD teststream 1 msg hello + assert_error "*syntax error*" {r XDELEX teststream KEEPREF NOIDS 1 1} + assert_error "*syntax error*" {r XDELEX teststream DELREF NOIDS 1 1} + assert_error "*syntax error*" {r XDELEX teststream ACKED NOIDS 1 1} + } + + test {XDELEX returns error for unrecognized mode token} { + r DEL teststream + r XADD teststream 1 msg hello + assert_error "*syntax error*" {r XDELEX teststream BADMODE IDS 1 1} + } + + test {XDELEX returns error for invalid numids} { + r DEL teststream + r XADD teststream 1 msg hello + assert_error "*positive integer*" {r XDELEX teststream IDS 0 1} + assert_error "*positive integer*" {r XDELEX teststream IDS -1 1} + assert_error "*positive integer*" {r XDELEX teststream IDS abc 1} + } + + test {XDELEX returns error when numids does not match ID count} { + r DEL teststream + r XADD teststream 1 msg hello + r XADD teststream 2 msg hello + # Too few IDs provided for numids + assert_error "*syntax error*" {r XDELEX teststream IDS 3 1 2} + # Too many IDs provided for numids + assert_error "*syntax error*" {r XDELEX teststream IDS 1 1 2} + } + + test {XDELEX with more than 8 IDs exercises dynamic allocation} { + r DEL teststream + # STREAMID_STATIC_VECTOR_LEN is 8, use 10 to force zmalloc path + for {set i 1} {$i <= 10} {incr i} { + r XADD teststream $i msg hello + } + set ids [r XDELEX teststream IDS 10 1 2 3 4 5 6 7 8 9 10] + assert_equal 10 [llength $ids] + foreach id $ids { + assert_equal 1 $id + } + assert_equal 0 [r XLEN teststream] + } + + test {XDELEX with more than 8 IDs and ACKED mode exercises dynamic allocation} { + r DEL teststream + for {set i 1} {$i <= 10} {incr i} { + r XADD teststream $i msg hello + } + r XGROUP CREATE teststream grp 0 + # Read all entries into the group's PEL + r XREADGROUP GROUP grp consumer COUNT 10 STREAMS teststream > + # Ack all so ACKED mode can delete them + for {set i 1} {$i <= 10} {incr i} { + r XACK teststream grp $i + } + set ids [r XDELEX teststream ACKED IDS 10 1 2 3 4 5 6 7 8 9 10] + assert_equal 10 [llength $ids] + foreach id $ids { + assert_equal 1 $id + } + assert_equal 0 [r XLEN teststream] + } + test {XRANGE fuzzing} { set items [r XRANGE mystream{t} - +] set low_id [lindex $items 0 0] From 955738848771d8b4ebd8faf8c9cc0ddbf61ace49 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Tue, 8 Sep 2026 20:16:29 -0400 Subject: [PATCH 03/24] Shared helpers for XDEL/XDELEX/XACKDEL This extracts 4 helper functions to reduce duplicated code across XDEL, XDELEX and XACKDEL. The helpers are as follows: - `streamParseModeAndIDCountOrReply`: Parse `[ KEEPREF | DELREF | ACKED ]` into PEL MODE enum (shared across `XACKDEL` & `XDELEX`) - `streamParseDelIDsOrReply`: Parse the `IDS [ID...]` as int into array (shared across all 3) - `streamTrackFirstEntryAndPropagate`: Common state tracking across `XACKDEL` & `XDELEX`, track stream first id & send keyspace events - `streamDeleteItemAndTrackFirstLast`: Deletes entries, tracking first entry bool & stream max deleted entry (shared across `XDEL` & `XDELEX`) Signed-off-by: Nick Iaquinto --- src/server.h | 1 - src/t_stream.c | 277 ++++++++++++++++++++++--------------------------- 2 files changed, 123 insertions(+), 155 deletions(-) diff --git a/src/server.h b/src/server.h index a34b956f7af..1d7463f646e 100644 --- a/src/server.h +++ b/src/server.h @@ -4425,4 +4425,3 @@ int iAmPrimary(void); #define STRINGIFY(x) STRINGIFY_(x) #endif - diff --git a/src/t_stream.c b/src/t_stream.c index 54db40d4dde..9d2f96c28c2 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -3551,6 +3551,63 @@ void xautoclaimCommand(client *c) { preventCommandPropagation(c); } +/* Parse IDS into array of stream message ids, ensuring each is a valid + * stream message ID. Returns C_OK, or replies to the client on first invalid + * ID and returns C_ERR. */ +static int streamParseDelIDsOrReply(client *c, int argi, long long id_count, streamID *ids, int *resps) { + for (long long j = 0; j < id_count; j++) { + if (streamParseStrictIDOrReply(c, c->argv[argi + j], &ids[j], 0, NULL) != C_OK) return C_ERR; + if (resps != NULL) resps[j] = 1; + } + return C_OK; +} + +/* Maintain stream state by tracking the first entry & propagating keyspace + * events for deleted entries and/or PEL modifications. Used when deleting or + * acknowledging messages, as in XDEL, XDELEX, XACKDEL. + */ +void streamTrackFirstEntryAndPropagate(client *c, stream *s, int deleted, bool pel_modified, bool first_entry) { + /* Update the stream's first ID. */ + if (deleted) { + if (s->length == 0) { + s->first_id.ms = 0; + s->first_id.seq = 0; + } else if (first_entry) { + streamGetEdgeID(s, 1, 1, &s->first_id); + } + } + + /* Either deleting entries or a PEL-only change mutate consumer-group state + * on this key, so we need to signal in either case to WATCH-ers & keyspace + * subscribers (see issue #3429). */ + if (deleted || pel_modified) { + signalModifiedKey(c, c->db, c->argv[1]); + notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); + server.dirty += deleted; + } +} + +/* Delete a stream entry & do bookkeeping for first entry, last deleted entry, + * deleted count. Shared in XDEL & XDELEX. Returns 1 if entry was deleted, + * otherwise 0. */ +int streamDeleteItemAndTrackFirstLast(stream *s, streamID *id, bool *first_entry, int *deleted) { + if (streamDeleteItem(s, id)) { + /* We want to know if the first entry in the stream was deleted + * so we can later set the new one. */ + if (streamCompareID(id, &s->first_id) == 0) { + *first_entry = 1; + } + /* Update the stream's maximal tombstone if needed. */ + if (streamCompareID(id, &s->max_deleted_entry_id) > 0) { + s->max_deleted_entry_id = *id; + } + (*deleted)++; + return 1; + }; + + return 0; +} + /* XDEL [ ... ] * * Removes the specified entries from the stream. Returns the number @@ -3569,93 +3626,86 @@ void xdelCommand(client *c) { streamID *ids = static_ids; int id_count = c->argc - 2; if (id_count > STREAMID_STATIC_VECTOR_LEN) ids = zmalloc(sizeof(streamID) * id_count); - for (int j = 2; j < c->argc; j++) { - if (streamParseStrictIDOrReply(c, c->argv[j], &ids[j - 2], 0, NULL) != C_OK) goto cleanup; + if (streamParseDelIDsOrReply(c, 2, id_count, ids, NULL) != C_OK) { + goto cleanup; } /* Actually apply the command. */ int deleted = 0; - int first_entry = 0; + bool first_entry = 0; for (int j = 2; j < c->argc; j++) { streamID *id = &ids[j - 2]; - if (streamDeleteItem(s, id)) { - /* We want to know if the first entry in the stream was deleted - * so we can later set the new one. */ - if (streamCompareID(id, &s->first_id) == 0) { - first_entry = 1; - } - /* Update the stream's maximal tombstone if needed. */ - if (streamCompareID(id, &s->max_deleted_entry_id) > 0) { - s->max_deleted_entry_id = *id; - } - deleted++; - }; + streamDeleteItemAndTrackFirstLast(s, id, &first_entry, &deleted); } - /* Update the stream's first ID. */ - if (deleted) { - if (s->length == 0) { - s->first_id.ms = 0; - s->first_id.seq = 0; - } else if (first_entry) { - streamGetEdgeID(s, 1, 1, &s->first_id); - } - } + /* Stream bookkeeping. */ + streamTrackFirstEntryAndPropagate(c, s, deleted, false, first_entry); - /* Propagate the write if needed. */ - if (deleted) { - signalModifiedKey(c, c->db, c->argv[1]); - notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); - server.dirty += deleted; - } addReplyLongLong(c, deleted); cleanup: if (ids != static_ids) zfree(ids); } -/* XDELEX [KEEPREF | DELREF | ACKED] IDS num [ ... ] - */ -void xdelexCommand(client *c) { - robj *o = lookupKeyRead(c->db, c->argv[1]); - if (o) { - if (checkType(c, o, OBJ_STREAM)) return; /* Type error. */ - } +typedef enum { + PELMODE_KEEPREF = 0, + PELMODE_DELREF, + PELMODE_ACKED +} streamPELMode; +/* Parse the "[KEEPREF | DELREF | ACKED] IDS " arguments shared by + * XDELEX and XACKDEL. */ +static int streamParseModeAndIDCountOrReply(client *c, int argi, streamPELMode *mode, long long *id_count, int *ids_argi) { /* Check what mode is set, if any. * ex. [KEEPREF | DELREF | ACKED] IDS n [ID ...] */ - int argi = 2; - int mode = 0; /* 0=keepref, 1=delref, 2=acked */ + *mode = PELMODE_KEEPREF; if (strcasecmp(objectGetVal(c->argv[argi]), "KEEPREF") == 0) { argi += 1; - mode = 0; } else if (strcasecmp(objectGetVal(c->argv[argi]), "DELREF") == 0) { argi += 1; - mode = 1; + *mode = PELMODE_DELREF; } else if (strcasecmp(objectGetVal(c->argv[argi]), "ACKED") == 0) { argi += 1; - mode = 2; + *mode = PELMODE_ACKED; } /* Expect IDS token. */ if (strcasecmp(objectGetVal(c->argv[argi]), "IDS") != 0) { addReplyErrorObject(c, shared.syntaxerr); - return; + return C_ERR; } argi++; /* past IDS */ /* Parse and validate numids: must be a positive integer. */ - long long id_count; - if (getLongLongFromObject(c->argv[argi], &id_count) != C_OK || id_count <= 0) { + if (getLongLongFromObject(c->argv[argi], id_count) != C_OK || *id_count <= 0) { addReplyError(c, "Number of IDs must be a positive integer"); - return; + return C_ERR; } argi++; /* past numids */ /* Validate numids matches remaining arg count. */ - long long actual_ids = c->argc - argi; - if (id_count != actual_ids) { + if (*id_count != c->argc - argi) { addReplyErrorObject(c, shared.syntaxerr); + return C_ERR; + } + + *ids_argi = argi; + return C_OK; +} + +/* XDELEX [KEEPREF | DELREF | ACKED] IDS num [ ... ] + */ +void xdelexCommand(client *c) { + robj *o = lookupKeyRead(c->db, c->argv[1]); + if (o) { + if (checkType(c, o, OBJ_STREAM)) return; /* Type error. */ + } + + int argi = 2; + long long id_count = 0; + int id_argi = argi; + streamPELMode mode; + if (streamParseModeAndIDCountOrReply(c, argi, &mode, &id_count, &id_argi) != C_OK) { return; } @@ -3671,12 +3721,8 @@ void xdelexCommand(client *c) { ids = zmalloc(sizeof(streamID) * id_count); resps = zmalloc(sizeof(int) * id_count); } - for (int j = argi; j < c->argc; j++) { - if (streamParseStrictIDOrReply(c, c->argv[j], &ids[j - argi], 0, NULL) != C_OK) goto cleanup; - - /* Default to 1 (will be deleted). Changed to 2 (can't delete yet) or -1 - * (not found) if we discover a blocking condition. */ - resps[j - argi] = 1; + if (streamParseDelIDsOrReply(c, id_argi, id_count, ids, resps) != C_OK) { + goto cleanup; } /* If missing stream, return -1 for each ID. */ @@ -3695,10 +3741,10 @@ void xdelexCommand(client *c) { * keyspace event just like a stream-entry deletion (see issue #3429). */ bool pel_modified = 0; - /* For ACKED and DELREF modes: loop over consumer groups (outer) then messages + /* For DELREF and ACKED modes: loop over consumer groups (outer) then messages * (inner). This opens the iterator once instead of once per message, and * allows inner-loop skips via the resps array. */ - if ((mode == 1 || mode == 2) && s->cgroups != NULL) { + if ((mode == PELMODE_DELREF || mode == PELMODE_ACKED) && s->cgroups != NULL) { bool first_loop = 1; raxIterator ri_cgroups; raxStart(&ri_cgroups, s->cgroups); @@ -3710,7 +3756,7 @@ void xdelexCommand(client *c) { /* Skip messages already finalized. For ACKED, 2 means another * group already has a pending ref so deletion is blocked. */ if (resps[j] == -1) continue; - if (mode == 2 && resps[j] == 2) continue; + if (mode == PELMODE_ACKED && resps[j] == 2) continue; streamID *id = &ids[j]; unsigned char buf[sizeof(streamID)]; @@ -3719,7 +3765,7 @@ void xdelexCommand(client *c) { /* Group hasn't claimed this message yet; it can't have a PEL * entry for it either, so there's nothing to remove. */ if (streamCompareID(id, &cg->last_id) > 0) { - if (mode == 2) { + if (mode == PELMODE_ACKED) { /* ACKED: can't delete until this group has seen it. */ resps[j] = streamEntryExists(s, id) ? 2 : -1; } @@ -3728,7 +3774,7 @@ void xdelexCommand(client *c) { void *result; if (raxFind(cg->pel, buf, sizeof(buf), &result)) { - if (mode == 1) { + if (mode == PELMODE_DELREF) { /* DELREF: remove PEL entry from this group. */ streamNACK *nack = result; raxRemove(cg->pel, buf, sizeof(buf), NULL); @@ -3740,7 +3786,7 @@ void xdelexCommand(client *c) { /* ACKED: still pending in this group, cannot delete. */ resps[j] = 2; } - } else if (mode == 2 && first_loop && !streamEntryExists(s, id)) { + } else if (mode == PELMODE_ACKED && first_loop && !streamEntryExists(s, id)) { /* Message doesn't exist in the stream; check once and skip * iterating the remaining groups. */ resps[j] = -1; @@ -3764,18 +3810,7 @@ void xdelexCommand(client *c) { for (int j = 0; j < id_count; j++) { if (resps[j] == 1) { streamID *id = &ids[j]; - if (streamDeleteItem(s, id)) { - /* Track whether the first stream entry was removed so we can - * update s->first_id below. */ - if (streamCompareID(id, &s->first_id) == 0) { - first_entry = 1; - } - /* Update the stream's maximal tombstone if needed. */ - if (streamCompareID(id, &s->max_deleted_entry_id) > 0) { - s->max_deleted_entry_id = *id; - } - deleted++; - } else { + if (!streamDeleteItemAndTrackFirstLast(s, id, &first_entry, &deleted)) { /* If the message does not exist, use -1 response code. * Necessary here b/c in KEEPREF mode, we skip checking above. */ resps[j] = -1; @@ -3783,24 +3818,8 @@ void xdelexCommand(client *c) { } } - /* Update the stream's first ID. */ - if (deleted) { - if (s->length == 0) { - s->first_id.ms = 0; - s->first_id.seq = 0; - } else if (first_entry) { - streamGetEdgeID(s, 1, 1, &s->first_id); - } - } - - /* Either deleting entries or a PEL-only change mutate consumer-group state - * on this key, so we need to signal in either case to WATCH-ers & keyspace - * subscribers (see issue #3429). */ - if (deleted || pel_modified) { - signalModifiedKey(c, c->db, c->argv[1]); - notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); - server.dirty += deleted; - } + /* Stream bookkeeping. */ + streamTrackFirstEntryAndPropagate(c, s, deleted, pel_modified, first_entry); /* Emit the array of per-ID results after the mutation has been signaled. */ addReplyArrayLen(c, id_count); @@ -3828,44 +3847,11 @@ void xackdelCommand(client *c) { group = streamLookupCG(objectGetVal(o), objectGetVal(c->argv[2])); } - /* Check what mode is set, if any. - * ex. [KEEPREF | DELREF | ACKED] IDS n [ID ...] - */ int argi = 3; - int mode = 0; /* 0=keepref, 1=delref, 2=acked */ - if (strcasecmp(objectGetVal(c->argv[argi]), "KEEPREF") == 0) { - argi += 1; - mode = 0; - } else if (strcasecmp(objectGetVal(c->argv[argi]), "DELREF") == 0) { - argi += 1; - mode = 1; - } else if (strcasecmp(objectGetVal(c->argv[argi]), "ACKED") == 0) { - argi += 1; - mode = 2; - } - - /* Expect IDS token. */ - if (strcasecmp(objectGetVal(c->argv[argi]), "IDS") != 0) { - addReplyErrorObject(c, shared.syntaxerr); - return; - } - argi++; /* past IDS */ - - /* Parse and validate numids: must be a positive integer. */ - long long id_count; - if (getLongLongFromObjectOrReply(c, c->argv[argi], &id_count, NULL) == C_ERR) { - return; - } - if (id_count <= 0) { - addReplyError(c, "The IDs argument must be a positive integer"); - return; - } - argi++; /* past numids */ - - /* Validate numids matches remaining arg count. */ - long long actual_ids = c->argc - argi; - if (id_count != actual_ids) { - addReplyErrorObject(c, shared.syntaxerr); + long long id_count = 0; + int id_argi = argi; + streamPELMode mode; + if (streamParseModeAndIDCountOrReply(c, argi, &mode, &id_count, &id_argi) != C_OK) { return; } @@ -3892,12 +3878,8 @@ void xackdelCommand(client *c) { ids = zmalloc(sizeof(streamID) * id_count); resps = zmalloc(sizeof(int) * id_count); } - for (long long j = argi; j < c->argc; j++) { - if (streamParseStrictIDOrReply(c, c->argv[j], &ids[j - argi], 0, NULL) != C_OK) goto cleanup; - - /* Default to 1 for ack'd and deleted. If we discover the message doesn't exist or if it is - * not acked by all consumers (in ACKED mode), then we change the response code. */ - resps[j - argi] = 1; + if (streamParseDelIDsOrReply(c, id_argi, id_count, ids, resps) != C_OK) { + goto cleanup; } int acked = 0; @@ -3907,7 +3889,7 @@ void xackdelCommand(client *c) { /* Fast path for KEEPREF. Since we only need to cleanup the PEL for the target * group, we only need to loop over messages (and not consumers) and can set * responses inline. Thus, there's a separate setup for KEEPREF vs. ACKED/DELREF*/ - if (mode == 0) { /* KEEPREF */ + if (mode == PELMODE_KEEPREF) { for (long long j = 0; j < id_count; j++) { int response = -1; streamID *id = &ids[j]; @@ -4002,18 +3984,19 @@ void xackdelCommand(client *c) { void *result; if (raxFind(cg->pel, buf, sizeof(buf), &result)) { - if (mode == 1) { /* DELREF */ + if (mode == PELMODE_DELREF) { + /* DELREF: remove PEL entry from this group. */ streamNACK *nack = result; raxRemove(cg->pel, buf, sizeof(buf), NULL); raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); streamFreeNACK(nack); acked++; - } else { /* ACKED */ + } else { /* Another group still has it pending. */ resps[j] = 2; } - } else if (mode == 2 && - streamCompareID(id, &cg->last_id) > 0) { /* ACKED */ + } else if (mode == PELMODE_ACKED && + streamCompareID(id, &cg->last_id) > 0) { /* Non-target hasn't claimed it yet; may still need to * deliver it, so block deletion. */ resps[j] = 2; @@ -4052,22 +4035,8 @@ void xackdelCommand(client *c) { } sync: - /* Update the stream's first ID. */ - if (deleted) { - if (s->length == 0) { - s->first_id.ms = 0; - s->first_id.seq = 0; - } else if (first_entry) { - streamGetEdgeID(s, 1, 1, &s->first_id); - } - } - - /* Propagate the write if needed. */ - if (deleted) { - signalModifiedKey(c, c->db, c->argv[1]); - notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); - server.dirty += deleted; - } + /* Stream bookkeeping. */ + streamTrackFirstEntryAndPropagate(c, s, deleted, acked, first_entry); /* PEL entries were removed even without stream deletion; mark dirty so * the command is propagated to replicas and written to AOF. */ From 45effc1d72176c8f8ae1a8da4e5a579bad095451 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Tue, 8 Sep 2026 22:12:19 -0400 Subject: [PATCH 04/24] Address CodeRabbit comments - Use `lookupKeyWrite` instead of read - Apply fix in JSON `IDS` token defn from `XACKDEL` to `XDELEX` too - Correct `XDELEX` PEL & last ID checking to match `XACKDEL` so that it properly handles `XGROUP SETID` / `XCLAIM ... LASTID` Signed-off-by: Nick Iaquinto --- src/commands.def | 2 +- src/commands/xdelex.json | 1 - src/t_stream.c | 32 ++++++++-------- tests/unit/type/stream.tcl | 78 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 18 deletions(-) diff --git a/src/commands.def b/src/commands.def index 446bbf5271f..6d205bd71b1 100644 --- a/src/commands.def +++ b/src/commands.def @@ -10680,7 +10680,7 @@ struct COMMAND_ARG XDELEX_Args[] = { {MAKE_ARG("mode",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,3,NULL),.subargs=XDELEX_mode_Subargs}, {MAKE_ARG("ids",ARG_TYPE_PURE_TOKEN,-1,"IDS",NULL,NULL,CMD_ARG_NONE,0,NULL)}, {MAKE_ARG("numids",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, -{MAKE_ARG("id",ARG_TYPE_STRING,-1,"IDS",NULL,NULL,CMD_ARG_MULTIPLE,0,NULL)}, +{MAKE_ARG("id",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_MULTIPLE,0,NULL)}, }; /********** XGROUP CREATE ********************/ diff --git a/src/commands/xdelex.json b/src/commands/xdelex.json index e82fba323b3..7ae1f17dc47 100644 --- a/src/commands/xdelex.json +++ b/src/commands/xdelex.json @@ -74,7 +74,6 @@ }, { "name": "id", - "token": "IDS", "type": "string", "multiple": true } diff --git a/src/t_stream.c b/src/t_stream.c index 9d2f96c28c2..209d0fbe9e9 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -3696,7 +3696,7 @@ static int streamParseModeAndIDCountOrReply(client *c, int argi, streamPELMode * /* XDELEX [KEEPREF | DELREF | ACKED] IDS num [ ... ] */ void xdelexCommand(client *c) { - robj *o = lookupKeyRead(c->db, c->argv[1]); + robj *o = lookupKeyWrite(c->db, c->argv[1]); if (o) { if (checkType(c, o, OBJ_STREAM)) return; /* Type error. */ } @@ -3762,16 +3762,10 @@ void xdelexCommand(client *c) { unsigned char buf[sizeof(streamID)]; streamEncodeID(buf, id); - /* Group hasn't claimed this message yet; it can't have a PEL - * entry for it either, so there's nothing to remove. */ - if (streamCompareID(id, &cg->last_id) > 0) { - if (mode == PELMODE_ACKED) { - /* ACKED: can't delete until this group has seen it. */ - resps[j] = streamEntryExists(s, id) ? 2 : -1; - } - continue; - } - + /* Check the PEL before consulting cg->last_id: XGROUP SETID + * can move last_id backward below IDs that are still pending + * (or were pending and later acked), so last_id alone cannot + * prove this group never claimed the message. */ void *result; if (raxFind(cg->pel, buf, sizeof(buf), &result)) { if (mode == PELMODE_DELREF) { @@ -3786,10 +3780,16 @@ void xdelexCommand(client *c) { /* ACKED: still pending in this group, cannot delete. */ resps[j] = 2; } - } else if (mode == PELMODE_ACKED && first_loop && !streamEntryExists(s, id)) { - /* Message doesn't exist in the stream; check once and skip - * iterating the remaining groups. */ - resps[j] = -1; + } else if (mode == PELMODE_ACKED) { + if (first_loop && !streamEntryExists(s, id)) { + /* Message doesn't exist in the stream; check once and + * skip iterating the remaining groups. */ + resps[j] = -1; + } else if (streamCompareID(id, &cg->last_id) > 0) { + /* Message may still be delivered to this group, so + * block deletion (same as XACKDEL). */ + resps[j] = 2; + } } } first_loop = 0; @@ -3841,7 +3841,7 @@ void xdelexCommand(client *c) { * not deleted. */ void xackdelCommand(client *c) { streamCG *group = NULL; - robj *o = lookupKeyRead(c->db, c->argv[1]); + robj *o = lookupKeyWrite(c->db, c->argv[1]); if (o) { if (checkType(c, o, OBJ_STREAM)) return; /* Type error. */ group = streamLookupCG(objectGetVal(o), objectGetVal(c->argv[2])); diff --git a/tests/unit/type/stream.tcl b/tests/unit/type/stream.tcl index 75e12d81670..408efb30ab0 100644 --- a/tests/unit/type/stream.tcl +++ b/tests/unit/type/stream.tcl @@ -1151,6 +1151,84 @@ start_server { assert_equal 2 [ r XLEN teststream ] } + test {XDELEX ACKED blocks deletion for entries pending below a rewound last_id} { + r DEL testxadstream + r XADD testxadstream 1-0 f v1 + r XADD testxadstream 5-0 f v5 + r XGROUP CREATE testxadstream testxadgrp1 0 + + # Claim both entries: last_id advances to 5-0 and both land in the PEL. + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 10 STREAMS testxadstream > + + # Move last_id back before 5-0, which stays in the PEL (id > last_id). + r XGROUP SETID testxadstream testxadgrp1 1-0 + + # 5-0 is still pending in the group, so ACKED must not delete it. + set ids [r XDELEX testxadstream ACKED IDS 1 5-0] + assert_equal 2 [lindex $ids 0] + assert_equal 2 [r XLEN testxadstream] + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2 [lindex $pend 0] + + # After XACK drains the PEL entry, deletion stays blocked because the + # rewound delivery cursor means the group may still be served 5-0 by + # XREADGROUP "" (same fallback as XACKDEL ACKED). + r XACK testxadstream testxadgrp1 5-0 + set ids [r XDELEX testxadstream ACKED IDS 1 5-0] + assert_equal 2 [lindex $ids 0] + assert_equal 2 [r XLEN testxadstream] + + # Catching the cursor back up unblocks deletion. + r XGROUP SETID testxadstream testxadgrp1 5-0 + set ids [r XDELEX testxadstream ACKED IDS 1 5-0] + assert_equal 1 [lindex $ids 0] + assert_equal 1 [r XLEN testxadstream] + } + + test {XDELEX ACKED reports pending ref, not -1, when entry is deleted below a rewound last_id} { + r DEL testxadstream + r XADD testxadstream 1-0 f v1 + r XADD testxadstream 5-0 f v5 + r XGROUP CREATE testxadstream testxadgrp1 0 + + # Claim both entries, then rewind last_id below the still-pending 5-0. + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 10 STREAMS testxadstream > + r XGROUP SETID testxadstream testxadgrp1 1-0 + + # Remove the stream entry, leaving 5-0 dangling in the PEL. + r XDEL testxadstream 5-0 + + # The dangling pending ref must block ACKED (2), like when the group + # cursor is ahead of the ID, instead of reporting -1 (not found). + set ids [r XDELEX testxadstream ACKED IDS 1 5-0] + assert_equal 2 [lindex $ids 0] + assert_equal 1 [r XLEN testxadstream] + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 2 [lindex $pend 0] + } + + test {XDELEX DELREF clears PEL ref even when last_id was rewound below it} { + r DEL testxadstream + r XADD testxadstream 1-0 f v1 + r XADD testxadstream 5-0 f v5 + r XGROUP CREATE testxadstream testxadgrp1 0 + + # Claim both entries, then rewind last_id below the still-pending 5-0. + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 10 STREAMS testxadstream > + r XGROUP SETID testxadstream testxadgrp1 1-0 + + # DELREF must remove the pending ref even though 5-0 > last_id. + set ids [r XDELEX testxadstream DELREF IDS 1 5-0] + assert_equal 1 [lindex $ids 0] + assert_equal 1 [r XLEN testxadstream] + + # Only 1-0 remains pending; the 5-0 ref must be gone. + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 1 [lindex $pend 0] + assert_equal 1-0 [lindex $pend 1] + assert_equal 1-0 [lindex $pend 2] + } + test {XDELEX w/ KEEPREF deletes all but keeps refs in consumer group PELs} { r DEL teststream r XADD teststream 1 msg helllo From 71e9dbd2ba996808c39591945f0cf40db4c3392e Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Tue, 8 Sep 2026 23:38:53 -0400 Subject: [PATCH 05/24] Address CodeRabbit Comments on XDELEX ACKED mode gap Checks existance separately from groups' PELs to ensure proper logic for entries with dangling PEL references after deletion. Signed-off-by: Nick Iaquinto --- src/t_stream.c | 45 ++++++++++++++++++++++++-------------- tests/unit/type/stream.tcl | 38 ++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 16 deletions(-) diff --git a/src/t_stream.c b/src/t_stream.c index 209d0fbe9e9..aa0e0da83f8 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -3715,11 +3715,14 @@ void xdelexCommand(client *c) { * executed in a "all or nothing" fashion. */ streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; int static_resps[STREAMID_STATIC_VECTOR_LEN]; + unsigned char static_exists[STREAMID_STATIC_VECTOR_LEN]; streamID *ids = static_ids; int *resps = static_resps; + unsigned char *exists = static_exists; if (id_count > STREAMID_STATIC_VECTOR_LEN) { ids = zmalloc(sizeof(streamID) * id_count); resps = zmalloc(sizeof(int) * id_count); + exists = zmalloc(sizeof(unsigned char) * id_count); } if (streamParseDelIDsOrReply(c, id_argi, id_count, ids, resps) != C_OK) { goto cleanup; @@ -3745,7 +3748,14 @@ void xdelexCommand(client *c) { * (inner). This opens the iterator once instead of once per message, and * allows inner-loop skips via the resps array. */ if ((mode == PELMODE_DELREF || mode == PELMODE_ACKED) && s->cgroups != NULL) { - bool first_loop = 1; + /* Determine stream message existance upfront to ensure we mark entry as + * "not found" for ACKED only after checking all groups' PELs. */ + if (mode == PELMODE_ACKED) { + for (int j = 0; j < id_count; j++) { + exists[j] = streamEntryExists(s, &ids[j]); + } + } + raxIterator ri_cgroups; raxStart(&ri_cgroups, s->cgroups); raxSeek(&ri_cgroups, "^", NULL, 0); @@ -3753,9 +3763,7 @@ void xdelexCommand(client *c) { streamCG *cg = ri_cgroups.data; for (int j = 0; j < id_count; j++) { - /* Skip messages already finalized. For ACKED, 2 means another - * group already has a pending ref so deletion is blocked. */ - if (resps[j] == -1) continue; + /* Skip messages already finalized. */ if (mode == PELMODE_ACKED && resps[j] == 2) continue; streamID *id = &ids[j]; @@ -3780,21 +3788,24 @@ void xdelexCommand(client *c) { /* ACKED: still pending in this group, cannot delete. */ resps[j] = 2; } - } else if (mode == PELMODE_ACKED) { - if (first_loop && !streamEntryExists(s, id)) { - /* Message doesn't exist in the stream; check once and - * skip iterating the remaining groups. */ - resps[j] = -1; - } else if (streamCompareID(id, &cg->last_id) > 0) { - /* Message may still be delivered to this group, so - * block deletion (same as XACKDEL). */ - resps[j] = 2; - } + } else if (mode == PELMODE_ACKED && exists[j] && + streamCompareID(id, &cg->last_id) > 0) { + /* Message exists and may still be delivered to this + * group, so block deletion (same as XACKDEL). Entries + * that no longer exist can't be re-delivered. */ + resps[j] = 2; } } - first_loop = 0; } raxStop(&ri_cgroups); + + /* ACKED: Entries that don't exist and that no group references return + * status "not found". */ + if (mode == PELMODE_ACKED) { + for (int j = 0; j < id_count; j++) { + if (resps[j] == 1 && !exists[j]) resps[j] = -1; + } + } } /* Based on the response calculated above for each stream message, delete @@ -3812,7 +3823,8 @@ void xdelexCommand(client *c) { streamID *id = &ids[j]; if (!streamDeleteItemAndTrackFirstLast(s, id, &first_entry, &deleted)) { /* If the message does not exist, use -1 response code. - * Necessary here b/c in KEEPREF mode, we skip checking above. */ + * Necessary here b/c in KEEPREF and DELREF modes, we don't + * check entry existence above. */ resps[j] = -1; } } @@ -3830,6 +3842,7 @@ void xdelexCommand(client *c) { cleanup: if (ids != static_ids) zfree(ids); if (resps != static_resps) zfree(resps); + if (exists != static_exists) zfree(exists); } /* XACKDEL [KEEPREF | DELREF | ACKED] IDS num [ ... ] diff --git a/tests/unit/type/stream.tcl b/tests/unit/type/stream.tcl index 408efb30ab0..c441190d288 100644 --- a/tests/unit/type/stream.tcl +++ b/tests/unit/type/stream.tcl @@ -1185,6 +1185,44 @@ start_server { assert_equal 1 [r XLEN testxadstream] } + test {XDELEX ACKED reports pending ref even when a clean group is iterated first} { + r DEL testxadstream + r XADD testxadstream 1-0 f v1 + r XADD testxadstream 5-0 f v5 + # agrp sorts before zgrp, so it is iterated first; it never claims 5-0. + r XGROUP CREATE testxadstream agrp 0 + r XGROUP CREATE testxadstream zgrp 0 + r XREADGROUP GROUP zgrp zcnsmr COUNT 10 STREAMS testxadstream > + + # Remove the stream entry, leaving 5-0 dangling only in zgrp's PEL. + r XDEL testxadstream 5-0 + + # The dangling pending ref must block ACKED with 2; the reply cannot + # depend on which consumer group the iteration reaches first. + set ids [r XDELEX testxadstream ACKED IDS 1 5-0] + assert_equal 2 [lindex $ids 0] + assert_equal 1 [r XLEN testxadstream] + set pend [r XPENDING testxadstream zgrp] + assert_equal 2 [lindex $pend 0] + } + + test {XDELEX ACKED returns -1 for deleted entry not referenced by any group} { + r DEL testxadstream + r XADD testxadstream 1-0 f v1 + r XADD testxadstream 5-0 f v5 + r XGROUP CREATE testxadstream testxadgrp1 0 + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 10 STREAMS testxadstream > + r XGROUP SETID testxadstream testxadgrp1 1-0 + r XACK testxadstream testxadgrp1 5-0 + + # Remove the entry; nothing references it anymore. The rewound last_id + # must not produce 2, since a deleted entry can never be re-delivered. + r XDEL testxadstream 5-0 + set ids [r XDELEX testxadstream ACKED IDS 1 5-0] + assert_equal -1 [lindex $ids 0] + assert_equal 1 [r XLEN testxadstream] + } + test {XDELEX ACKED reports pending ref, not -1, when entry is deleted below a rewound last_id} { r DEL testxadstream r XADD testxadstream 1-0 f v1 From 295a4e1ef95a732062e7f21bd7f1c2914f4f7c07 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Wed, 9 Sep 2026 09:54:05 -0400 Subject: [PATCH 06/24] Propagate XACKDEL/XDELEX manually with pre-9.2 commands Since these commands are being introduced in 9.2, propagating the new commands to replicas running older version would cause them to crash. Thus, we need to emit the equivalent behavior using XACK/XDEL instead. Now, all 3 categories of underlying changes to stream/consumer group state get propagated as v5+ compatible commands: - The target group's PEL changes (ie. `XACK`) - (In DELREF mode) Each other group that is affected (ie. `XACK`) - Stream message deletions (ie. `XDEL`) Propagate XDELEX/XACKDEL effects as XACK/XDEL XDELEX and XACKDEL are new in 9.2, so replicas running older versions would reject them as unknown commands. Both new commands now suppress implicit propagation and instead emit their effects as v5+ compatible commands: - Target group PEL updates as `XACK ` - Cleared PEL refs in other groups (`DELREF`) as one `XACK` per group - Deletions as a single `XDEL ` Stream replication tests now assert via replica commandstats that XACK/XDEL are received and XDELEX/XACKDEL never are. Signed-off-by: Nick Iaquinto --- src/server.c | 2 + src/server.h | 2 +- src/t_stream.c | 179 +++++++++++++++++++++++++++-- tests/unit/type/stream-cgroups.tcl | 151 ++++++++++++++++++++++++ 4 files changed, 321 insertions(+), 13 deletions(-) diff --git a/src/server.c b/src/server.c index f205fe7b753..675d7252536 100644 --- a/src/server.c +++ b/src/server.c @@ -2301,6 +2301,8 @@ void createSharedObjects(void) { shared.srem = createSharedString("SREM"); shared.xgroup = createSharedString("XGROUP"); shared.xclaim = createSharedString("XCLAIM"); + shared.xdel = createSharedString("XDEL"); + shared.xack = createSharedString("XACK"); shared.script = createSharedString("SCRIPT"); shared.replconf = createSharedString("REPLCONF"); shared.pexpireat = createSharedString("PEXPIREAT"); diff --git a/src/server.h b/src/server.h index 1d7463f646e..db43aadf57c 100644 --- a/src/server.h +++ b/src/server.h @@ -1529,7 +1529,7 @@ struct sharedObjectsStruct { *execaborterr, *noautherr, *noreplicaserr, *busykeyerr, *oomerr, *plus, *messagebulk, *pmessagebulk, *subscribebulk, *unsubscribebulk, *psubscribebulk, *punsubscribebulk, *del, *unlink, *rpop, *lpop, *lpush, *zadd, *rpoplpush, *lmove, *blmove, *zpopmin, *zpopmax, *emptyscan, *multi, *exec, *left, *right, *hset, *hsetex, *hdel, *hpexpireat, *hpersist, *srem, - *xgroup, *xclaim, *script, *replconf, *eval, *cluster, *syncslots, *persist, *set, *pexpireat, *pexpire, *time, *pxat, *absttl, + *xgroup, *xclaim, *xdel, *xack, *script, *replconf, *eval, *cluster, *syncslots, *persist, *set, *pexpireat, *pexpire, *time, *pxat, *absttl, *retrycount, *force, *justid, *entriesread, *lastid, *ping, *setid, *keepttl, *load, *createconsumer, *getack, *special_asterisk, *special_equals, *default_username, *redacted, *ssubscribebulk, *sunsubscribebulk, *fields, *finish, *state, *success, *failed, *name, *message, diff --git a/src/t_stream.c b/src/t_stream.c index aa0e0da83f8..c6c2653d770 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -1625,6 +1625,47 @@ void streamPropagateConsumerCreation(client *c, robj *key, robj *groupname, sds decrRefCount(argv[4]); } +/* Propagate the deletion of stream entries as + * + * XDEL ... + * + * XDELEX & XACKDEL propagate their effects manually this way to ensure + * compatibility with any pre-9.2 replicas. */ +static void streamPropagateDelIDs(client *c, robj *key, streamID *ids, int count) { + if (count == 0) return; + + robj **argv = zmalloc(sizeof(robj *) * (2 + count)); + argv[0] = shared.xdel; + argv[1] = key; + for (int j = 0; j < count; j++) argv[2 + j] = createObjectFromStreamID(&ids[j]); + + alsoPropagate(c->db->id, argv, 2 + count, PROPAGATE_AOF | PROPAGATE_REPL, c->slot); + + for (int j = 0; j < count; j++) decrRefCount(argv[2 + j]); + zfree(argv); +} + +/* Propagate acknowledgement of 'count' ids for 'groupname' as + * + * XACK ... + * + * XDELEX & XACKDEL propagate their effects manually this way to ensure + * compatibility with any pre-9.2 replicas. */ +static void streamPropagateAckIDs(client *c, robj *key, robj *groupname, streamID *ids, int count) { + if (count == 0) return; + + robj **argv = zmalloc(sizeof(robj *) * (3 + count)); + argv[0] = shared.xack; + argv[1] = key; + argv[2] = groupname; + for (int j = 0; j < count; j++) argv[3 + j] = createObjectFromStreamID(&ids[j]); + + alsoPropagate(c->db->id, argv, 3 + count, PROPAGATE_AOF | PROPAGATE_REPL, c->slot); + + for (int j = 0; j < count; j++) decrRefCount(argv[3 + j]); + zfree(argv); +} + /* Send the stream items in the specified range to the client 'c'. The range * the client will receive is between start and end inclusive, if 'count' is * non zero, no more than 'count' elements are sent. @@ -3709,21 +3750,37 @@ void xdelexCommand(client *c) { return; } - /* Start parsing the IDs, so that we abort ASAP if there is a syntax - * error: the return value of this command cannot be an error in case - * the client successfully acknowledged some messages, so it should be - * executed in a "all or nothing" fashion. */ + /* Space for tracking changes to make/propagated & response status codes */ streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; - int static_resps[STREAMID_STATIC_VECTOR_LEN]; - unsigned char static_exists[STREAMID_STATIC_VECTOR_LEN]; streamID *ids = static_ids; + + int static_resps[STREAMID_STATIC_VECTOR_LEN]; int *resps = static_resps; + + streamID static_ack_ids[STREAMID_STATIC_VECTOR_LEN]; + streamID *ack_ids = static_ack_ids; + + streamID static_del_ids[STREAMID_STATIC_VECTOR_LEN]; + streamID *del_ids = static_del_ids; + int del_count = 0; + + unsigned char static_exists[STREAMID_STATIC_VECTOR_LEN]; unsigned char *exists = static_exists; + + unsigned char static_cleared[STREAMID_STATIC_VECTOR_LEN]; + unsigned char *cleared = static_cleared; + if (id_count > STREAMID_STATIC_VECTOR_LEN) { ids = zmalloc(sizeof(streamID) * id_count); resps = zmalloc(sizeof(int) * id_count); + ack_ids = zmalloc(sizeof(streamID) * id_count); + del_ids = zmalloc(sizeof(streamID) * id_count); exists = zmalloc(sizeof(unsigned char) * id_count); + cleared = zmalloc(sizeof(unsigned char) * id_count); } + + /* Start parsing the IDs, so that we abort ASAP if there is a syntax + * error giving "all or nothing" semantics. */ if (streamParseDelIDsOrReply(c, id_argi, id_count, ids, resps) != C_OK) { goto cleanup; } @@ -3748,6 +3805,10 @@ void xdelexCommand(client *c) { * (inner). This opens the iterator once instead of once per message, and * allows inner-loop skips via the resps array. */ if ((mode == PELMODE_DELREF || mode == PELMODE_ACKED) && s->cgroups != NULL) { + /* Tracks which PEL entries were cleared for this group so we can + * propagate XACK's. Reset in loop after propagating each group. */ + memset(cleared, 0, id_count); + /* Determine stream message existance upfront to ensure we mark entry as * "not found" for ACKED only after checking all groups' PELs. */ if (mode == PELMODE_ACKED) { @@ -3784,6 +3845,7 @@ void xdelexCommand(client *c) { streamFreeNACK(nack); server.dirty++; pel_modified = 1; + cleared[j] = 1; } else { /* ACKED: still pending in this group, cannot delete. */ resps[j] = 2; @@ -3796,6 +3858,24 @@ void xdelexCommand(client *c) { resps[j] = 2; } } + + if (mode == PELMODE_DELREF) { + /* Propagate the PEL entries cleared for this group as + * XACK (see streamPropagateDelIDs for + * why effects are propagated as primitive commands). */ + int ack_count = 0; + for (int j = 0; j < id_count; j++) { + if (cleared[j]) { + ack_ids[ack_count++] = ids[j]; + cleared[j] = 0; + } + } + if (ack_count) { + robj *groupname = createStringObject((char *)ri_cgroups.key, ri_cgroups.key_len); + streamPropagateAckIDs(c, c->argv[1], groupname, ack_ids, ack_count); + decrRefCount(groupname); + } + } } raxStop(&ri_cgroups); @@ -3826,6 +3906,8 @@ void xdelexCommand(client *c) { * Necessary here b/c in KEEPREF and DELREF modes, we don't * check entry existence above. */ resps[j] = -1; + } else { + del_ids[del_count++] = *id; } } } @@ -3833,6 +3915,11 @@ void xdelexCommand(client *c) { /* Stream bookkeeping. */ streamTrackFirstEntryAndPropagate(c, s, deleted, pel_modified, first_entry); + /* Propagate the effects as XACK/XDEL commands instead of XDELEX itself to + * ensure compatibility with pre-9.2 replica. */ + preventCommandPropagation(c); + streamPropagateDelIDs(c, c->argv[1], del_ids, del_count); + /* Emit the array of per-ID results after the mutation has been signaled. */ addReplyArrayLen(c, id_count); for (int j = 0; j < id_count; j++) { @@ -3843,6 +3930,9 @@ void xdelexCommand(client *c) { if (ids != static_ids) zfree(ids); if (resps != static_resps) zfree(resps); if (exists != static_exists) zfree(exists); + if (ack_ids != static_ack_ids) zfree(ack_ids); + if (del_ids != static_del_ids) zfree(del_ids); + if (cleared != static_cleared) zfree(cleared); } /* XACKDEL [KEEPREF | DELREF | ACKED] IDS num [ ... ] @@ -3879,18 +3969,39 @@ void xackdelCommand(client *c) { stream *s = objectGetVal(o); - /* Start parsing the IDs, so that we abort ASAP if there is a syntax - * error: the return value of this command cannot be an error in case - * the client successfully acknowledged some messages, so it should be - * executed in a "all or nothing" fashion. */ + /* Space for tracking changes to make/propagated & response status codes */ streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; streamID *ids = static_ids; + int static_resps[STREAMID_STATIC_VECTOR_LEN]; int *resps = static_resps; + + unsigned char static_acked_flags[STREAMID_STATIC_VECTOR_LEN]; + unsigned char *acked_flags = static_acked_flags; + + unsigned char static_cleared[STREAMID_STATIC_VECTOR_LEN]; + unsigned char *cleared = static_cleared; + + streamID static_ack_ids[STREAMID_STATIC_VECTOR_LEN]; + streamID *ack_ids = static_ack_ids; + + streamID static_del_ids[STREAMID_STATIC_VECTOR_LEN]; + streamID *del_ids = static_del_ids; + int del_count = 0; + if (id_count > STREAMID_STATIC_VECTOR_LEN) { ids = zmalloc(sizeof(streamID) * id_count); resps = zmalloc(sizeof(int) * id_count); + acked_flags = zmalloc(sizeof(unsigned char) * id_count); + cleared = zmalloc(sizeof(unsigned char) * id_count); + ack_ids = zmalloc(sizeof(streamID) * id_count); + del_ids = zmalloc(sizeof(streamID) * id_count); } + memset(acked_flags, 0, id_count); + memset(cleared, 0, id_count); + + /* Start parsing the IDs, so that we abort ASAP if there is a syntax + * error giving "all or nothing" semantics. */ if (streamParseDelIDsOrReply(c, id_argi, id_count, ids, resps) != C_OK) { goto cleanup; } @@ -3918,10 +4029,12 @@ void xackdelCommand(client *c) { streamFreeNACK(nack); response = 1; acked++; + acked_flags[j] = 1; /* Delete the message */ if (streamDeleteItem(s, id)) { deleted++; + del_ids[del_count++] = *id; } /* We want to know if the first entry in the stream was deleted @@ -3967,6 +4080,7 @@ void xackdelCommand(client *c) { raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); streamFreeNACK(nack); acked++; + acked_flags[j] = 1; /* resps[j] stays 1: eligible for deletion (ACKED may still block it). */ } else { resps[j] = -1; /* Never delivered / already acked / doesn't exist. */ @@ -3974,6 +4088,10 @@ void xackdelCommand(client *c) { } if (s->cgroups != NULL) { + /* Tracks which PEL entries were cleared for this group so we can + * propagate XACK's. Reset in loop after propagating each group. */ + memset(cleared, 0, id_count); + raxIterator ri_cgroups; raxStart(&ri_cgroups, s->cgroups); raxSeek(&ri_cgroups, "^", NULL, 0); @@ -4004,6 +4122,7 @@ void xackdelCommand(client *c) { raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); streamFreeNACK(nack); acked++; + cleared[j] = 1; } else { /* Another group still has it pending. */ resps[j] = 2; @@ -4015,6 +4134,24 @@ void xackdelCommand(client *c) { resps[j] = 2; } } + + if (mode == PELMODE_DELREF) { + /* Propagate the PEL entries cleared for this group as + * `XACK ` to ensure compatibility with + * pre-9.2 replicas. */ + int ack_count = 0; + for (long long j = 0; j < id_count; j++) { + if (cleared[j]) { + ack_ids[ack_count++] = ids[j]; + cleared[j] = 0; + } + } + if (ack_count) { + robj *groupname = createStringObject((char *)ri_cgroups.key, ri_cgroups.key_len); + streamPropagateAckIDs(c, c->argv[1], groupname, ack_ids, ack_count); + decrRefCount(groupname); + } + } } raxStop(&ri_cgroups); } @@ -4032,6 +4169,7 @@ void xackdelCommand(client *c) { streamID *id = &ids[j]; if (streamDeleteItem(s, id)) { deleted++; + del_ids[del_count++] = *id; } /* We want to know if the first entry in the stream was deleted @@ -4051,12 +4189,25 @@ void xackdelCommand(client *c) { /* Stream bookkeeping. */ streamTrackFirstEntryAndPropagate(c, s, deleted, acked, first_entry); - /* PEL entries were removed even without stream deletion; mark dirty so - * the command is propagated to replicas and written to AOF. */ + /* PEL entries can be removed without any stream deletion; keep the dirty + * increment so save-point accounting reflects the mutation. */ if (acked) { server.dirty += acked; } + /* Propagate the effects as XACK/XDEL commands instead of XACKDEL itself so + * that pre-9.2 replica's don't crash. + * + * Target-group acknowledgements first, then the deletions. */ + preventCommandPropagation(c); + + int ack_count = 0; + for (long long j = 0; j < id_count; j++) { + if (acked_flags[j]) ack_ids[ack_count++] = ids[j]; + } + streamPropagateAckIDs(c, c->argv[1], c->argv[2], ack_ids, ack_count); + streamPropagateDelIDs(c, c->argv[1], del_ids, del_count); + /* Emit the array of per-ID results after the mutation has been signaled. */ addReplyArrayLen(c, id_count); for (long long j = 0; j < id_count; j++) { @@ -4066,6 +4217,10 @@ void xackdelCommand(client *c) { cleanup: if (ids != static_ids) zfree(ids); if (resps != static_resps) zfree(resps); + if (acked_flags != static_acked_flags) zfree(acked_flags); + if (cleared != static_cleared) zfree(cleared); + if (ack_ids != static_ack_ids) zfree(ack_ids); + if (del_ids != static_del_ids) zfree(del_ids); } /* General form: XTRIM [... options ...] diff --git a/tests/unit/type/stream-cgroups.tcl b/tests/unit/type/stream-cgroups.tcl index 7001a16f22e..0cf4a0a307d 100644 --- a/tests/unit/type/stream-cgroups.tcl +++ b/tests/unit/type/stream-cgroups.tcl @@ -1679,6 +1679,157 @@ start_server { } } + start_server {tags {"external:skip"}} { + set master [srv -1 client] + set master_host [srv -1 host] + set master_port [srv -1 port] + set replica [srv 0 client] + + # Number of times 'cmd' was executed on the replica, or 0 if never + # called (INFO omits zero counters). + proc get_replica_calls {client cmd} { + set info [$client INFO commandstats] + foreach line [split $info "\n"] { + if {[string match "cmdstat_$cmd:*" $line]} { + regexp {calls=(\d+)} $line -> count + return $count + } + } + return 0 + } + + $replica replicaof $master_host $master_port + wait_for_condition 50 100 { + [s 0 master_link_status] eq {up} + } else { + fail "Replication not started." + } + + test {XACKDEL ack-only propagates XACK but never XACKDEL or XDEL} { + $master DEL stream + $master XADD stream 1-0 f v + $master XGROUP CREATE stream grp1 0 + $master XGROUP CREATE stream grp2 0 + $master XREADGROUP GROUP grp1 alice COUNT 1 STREAMS stream > + $master XREADGROUP GROUP grp2 bob COUNT 1 STREAMS stream > + wait_for_ofs_sync $master $replica + + set xack_before [get_replica_calls $replica xack] + set xdel_before [get_replica_calls $replica xdel] + + # grp2 still holds the message pending, so nothing is deleted + $master XACKDEL stream grp1 ACKED IDS 1 1-0 + wait_for_ofs_sync $master $replica + + assert_equal 1 [expr {[get_replica_calls $replica xack] - $xack_before}] + assert_equal 0 [expr {[get_replica_calls $replica xdel] - $xdel_before}] + assert_equal 0 [get_replica_calls $replica xackdel] + assert_equal 1 [$replica XLEN stream] + assert_equal 0 [llength [$replica XPENDING stream grp1 - + 10]] + } + + test {XACKDEL KEEPREF propagates XACK + XDEL but never XACKDEL} { + $master DEL stream + $master XADD stream 1-0 f v + $master XGROUP CREATE stream grp 0 + $master XREADGROUP GROUP grp alice COUNT 1 STREAMS stream > + wait_for_ofs_sync $master $replica + + set xack_before [get_replica_calls $replica xack] + set xdel_before [get_replica_calls $replica xdel] + + $master XACKDEL stream grp KEEPREF IDS 1 1-0 + wait_for_ofs_sync $master $replica + + assert_equal 1 [expr {[get_replica_calls $replica xack] - $xack_before}] + assert_equal 1 [expr {[get_replica_calls $replica xdel] - $xdel_before}] + assert_equal 0 [get_replica_calls $replica xackdel] + assert_equal 0 [$replica XLEN stream] + assert_equal 0 [llength [$replica XPENDING stream grp - + 10]] + } + + test {XACKDEL DELREF propagates per-group XACK + XDEL but never XACKDEL} { + $master DEL stream + $master XADD stream 1-0 f v + $master XGROUP CREATE stream grp1 0 + $master XGROUP CREATE stream grp2 0 + $master XREADGROUP GROUP grp1 alice COUNT 1 STREAMS stream > + $master XREADGROUP GROUP grp2 bob COUNT 1 STREAMS stream > + wait_for_ofs_sync $master $replica + + set xack_before [get_replica_calls $replica xack] + set xdel_before [get_replica_calls $replica xdel] + + $master XACKDEL stream grp1 DELREF IDS 1 1-0 + wait_for_ofs_sync $master $replica + + # One XACK for the target group + one for grp2's cleared PEL ref + assert_equal 2 [expr {[get_replica_calls $replica xack] - $xack_before}] + assert_equal 1 [expr {[get_replica_calls $replica xdel] - $xdel_before}] + assert_equal 0 [get_replica_calls $replica xackdel] + assert_equal 0 [$replica XLEN stream] + assert_equal 0 [llength [$replica XPENDING stream grp2 - + 10]] + } + + test {XDELEX KEEPREF propagates XDEL only but never XDELEX} { + $master DEL stream + $master XADD stream 1-0 f v + wait_for_ofs_sync $master $replica + + set xack_before [get_replica_calls $replica xack] + set xdel_before [get_replica_calls $replica xdel] + + $master XDELEX stream KEEPREF IDS 1 1-0 + wait_for_ofs_sync $master $replica + + assert_equal 1 [expr {[get_replica_calls $replica xdel] - $xdel_before}] + assert_equal 0 [expr {[get_replica_calls $replica xack] - $xack_before}] + assert_equal 0 [get_replica_calls $replica xdelex] + assert_equal 0 [$replica XLEN stream] + } + + test {XDELEX DELREF propagates XDEL + XACK but never XDELEX} { + $master DEL stream + $master XADD stream 1-0 f v + $master XGROUP CREATE stream grp 0 + $master XREADGROUP GROUP grp alice COUNT 1 STREAMS stream > + wait_for_ofs_sync $master $replica + + set xack_before [get_replica_calls $replica xack] + set xdel_before [get_replica_calls $replica xdel] + + $master XDELEX stream DELREF IDS 1 1-0 + wait_for_ofs_sync $master $replica + + assert_equal 1 [expr {[get_replica_calls $replica xdel] - $xdel_before}] + assert_equal 1 [expr {[get_replica_calls $replica xack] - $xack_before}] + assert_equal 0 [get_replica_calls $replica xdelex] + assert_equal 0 [$replica XLEN stream] + assert_equal 0 [llength [$replica XPENDING stream grp - + 10]] + } + + test {XDELEX ACKED with nothing deleted propagates nothing} { + $master DEL stream + $master XADD stream 1-0 f v + $master XGROUP CREATE stream grp 0 + $master XREADGROUP GROUP grp alice COUNT 1 STREAMS stream > + wait_for_ofs_sync $master $replica + + set xack_before [get_replica_calls $replica xack] + set xdel_before [get_replica_calls $replica xdel] + + # Entry still pending in grp, so ACKED neither deletes nor clears + $master XDELEX stream ACKED IDS 1 1-0 + wait_for_ofs_sync $master $replica + + assert_equal 0 [expr {[get_replica_calls $replica xdel] - $xdel_before}] + assert_equal 0 [expr {[get_replica_calls $replica xack] - $xack_before}] + assert_equal 0 [get_replica_calls $replica xdelex] + assert_equal 1 [$replica XLEN stream] + assert_equal 1 [llength [$replica XPENDING stream grp - + 10]] + } + } + start_server {tags {"stream needs:debug"} overrides {appendonly yes aof-use-rdb-preamble no}} { test {Empty stream with no lastid can be rewrite into AOF correctly} { r XGROUP CREATE mystream group-name $ MKSTREAM From 5d2bd88277b1d1112323a2e36549d4bb2ca73caa Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Wed, 9 Sep 2026 12:34:24 -0400 Subject: [PATCH 07/24] Create common parser for `XDEL`-like commands Covers `XDEL`, `XDELEX`, `XACKDEL` to parse: - Stream Name - Group Name (optionally, for `XACKDEL`) - PEL Mode (optionally, for `XACKDEL`, `XDELEX`) - IDS List (optionally w/ `IDS ` for `XACKDEL`, `XDELEX`) There are out params for the above. I've stopped short of having the parse helper also parse the list of IDs for 2 reasons: 1. Some commands need to reply (with varying response shapes) in between the stream/group step and the IDs parsing step 2. This division of responsibility makes the memory management simpler, so all allocations are in the same spot in the commands, instead of having to track whether the helper allocated or not. Also renamed `streamParseDelIDsOrReply` to `streamParseStrictIDsOrReply` to align with `streamParseStrictIDOrReply` and move those helpers closer together. Putting those next to each other surfaced that different parts of the stream code refer to `id` vs `seq`. I can align `streamParseStrictIDsOrReply` to the arg naming from `streamParseStrictIDOrReply`, but `XADD` refers to `id` throughout as well. So that maybe isn't a worthwile naming standardization in this PR. Signed-off-by: Nick Iaquinto --- src/server.c | 4 +- src/t_stream.c | 191 +++++++++++++++++++++++---------------- tests/support/server.tcl | 6 +- 3 files changed, 121 insertions(+), 80 deletions(-) diff --git a/src/server.c b/src/server.c index 675d7252536..1baa1e8e983 100644 --- a/src/server.c +++ b/src/server.c @@ -7494,8 +7494,8 @@ void dismissMemoryInChild(void) { /* madvise(MADV_DONTNEED) may not work if Transparent Huge Pages is enabled. */ if (server.thp_enabled) return; - /* Currently we use zmadvise_dontneed only when we use jemalloc with Linux. - * so we avoid these pointless loops when they're not going to do anything. */ + /* Currently we use zmadvise_dontneed only when we use jemalloc with Linux. + * so we avoid these pointless loops when they're not going to do anything. */ #if defined(USE_JEMALLOC) && defined(__linux__) listIter li; listNode *ln; diff --git a/src/t_stream.c b/src/t_stream.c index c6c2653d770..79dca342e4e 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -63,6 +63,7 @@ size_t streamReplyWithRangeFromConsumerPEL(client *c, size_t count, streamConsumer *consumer); int streamParseStrictIDOrReply(client *c, robj *o, streamID *id, uint64_t missing_seq, int *seq_given); +int streamParseStrictIDsOrReply(client *c, int argi, long long id_count, streamID *ids, int *resps); int streamParseIDOrReply(client *c, robj *o, streamID *id, uint64_t missing_seq); /* ----------------------------------------------------------------------- @@ -2012,6 +2013,17 @@ int streamParseStrictIDOrReply(client *c, robj *o, streamID *id, uint64_t missin return streamGenericParseIDOrReply(c, o, id, missing_seq, 1, seq_given); } +/* Parse IDS into array of stream message ids, ensuring each is a valid + * stream message ID. Returns C_OK, or replies to the client on first invalid + * ID and returns C_ERR. */ +int streamParseStrictIDsOrReply(client *c, int argi, long long id_count, streamID *ids, int *resps) { + for (long long j = 0; j < id_count; j++) { + if (streamParseStrictIDOrReply(c, c->argv[argi + j], &ids[j], 0, NULL) != C_OK) return C_ERR; + if (resps != NULL) resps[j] = 1; + } + return C_OK; +} + /* Helper for parsing a stream ID that is a range query interval. When the * exclude argument is NULL, streamParseIDOrReply() is called and the interval * is treated as close (inclusive). Otherwise, the exclude argument is set if @@ -3592,17 +3604,6 @@ void xautoclaimCommand(client *c) { preventCommandPropagation(c); } -/* Parse IDS into array of stream message ids, ensuring each is a valid - * stream message ID. Returns C_OK, or replies to the client on first invalid - * ID and returns C_ERR. */ -static int streamParseDelIDsOrReply(client *c, int argi, long long id_count, streamID *ids, int *resps) { - for (long long j = 0; j < id_count; j++) { - if (streamParseStrictIDOrReply(c, c->argv[argi + j], &ids[j], 0, NULL) != C_OK) return C_ERR; - if (resps != NULL) resps[j] = 1; - } - return C_OK; -} - /* Maintain stream state by tracking the first entry & propagating keyspace * events for deleted entries and/or PEL modifications. Used when deleting or * acknowledging messages, as in XDEL, XDELEX, XACKDEL. @@ -3649,56 +3650,56 @@ int streamDeleteItemAndTrackFirstLast(stream *s, streamID *id, bool *first_entry return 0; } -/* XDEL [ ... ] - * - * Removes the specified entries from the stream. Returns the number - * of items actually deleted, that may be different from the number - * of IDs passed in case certain IDs do not exist. */ -void xdelCommand(client *c) { - robj *o; - - if ((o = lookupKeyWriteOrReply(c, c->argv[1], shared.czero)) == NULL || checkType(c, o, OBJ_STREAM)) return; - stream *s = objectGetVal(o); - - /* We need to sanity check the IDs passed to start. Even if not - * a big issue, it is not great that the command is only partially - * executed because at some point an invalid ID is parsed. */ - streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; - streamID *ids = static_ids; - int id_count = c->argc - 2; - if (id_count > STREAMID_STATIC_VECTOR_LEN) ids = zmalloc(sizeof(streamID) * id_count); - if (streamParseDelIDsOrReply(c, 2, id_count, ids, NULL) != C_OK) { - goto cleanup; - } - - /* Actually apply the command. */ - int deleted = 0; - bool first_entry = 0; - for (int j = 2; j < c->argc; j++) { - streamID *id = &ids[j - 2]; - streamDeleteItemAndTrackFirstLast(s, id, &first_entry, &deleted); - } - - /* Stream bookkeeping. */ - streamTrackFirstEntryAndPropagate(c, s, deleted, false, first_entry); - - addReplyLongLong(c, deleted); -cleanup: - if (ids != static_ids) zfree(ids); -} - +/* PEL handling modes shared by XDELEX & XACKDEL. */ typedef enum { PELMODE_KEEPREF = 0, PELMODE_DELREF, PELMODE_ACKED } streamPELMode; -/* Parse the "[KEEPREF | DELREF | ACKED] IDS " arguments shared by - * XDELEX and XACKDEL. */ -static int streamParseModeAndIDCountOrReply(client *c, int argi, streamPELMode *mode, long long *id_count, int *ids_argi) { +/* Shared argument parsing of the XDEL-like commands: + * + * XDEL key ... + * XDELEX key [KEEPREF | DELREF | ACKED] IDS ... + * XACKDEL key [KEEPREF | DELREF | ACKED] IDS ... + * + * Parsing options include 1) if the command has a group arg (ie. XACKDEL) and + * 2) if the command has PEL mode (XACKDEL/XDELEX). + * + * This only parses the number of arguments, not the actual array for 2 reasons: + * 1) different commands have different reply shapes in some circumstances and + * 2) this simplifies memory management of the allocated ids array. + * + * On success, 'ids_argi' points at the first ID argument and 'id_count' + * holds the number of IDs. The IDs themselves are parsed by the callers + * so each command keeps its own ID-array allocation strategy. + * + * Returns C_OK, or C_ERR with an error already replied to the client. */ +static int streamParseXDelArgsOrReply(client *c, bool has_group_arg, bool has_pelmode_arg, robj **o, streamCG **group, streamPELMode *mode, int *ids_argi, long long *id_count) { + *group = NULL; + *o = lookupKeyWrite(c->db, c->argv[1]); + if (*o && checkType(c, *o, OBJ_STREAM)) return C_ERR; /* Type error. */ + + int argi = 2; + if (has_group_arg) { + /* The group name is a positional argument: always consume it, even + * when the key is missing (the lookup simply yields a NULL group). */ + if (*o) { + *group = streamLookupCG(objectGetVal(*o), objectGetVal(c->argv[argi])); + } + argi++; /* past group */ + } + + if (!has_pelmode_arg) { + /* XDEL has no IDS token, so the remaining args is the id count. */ + *mode = PELMODE_KEEPREF; + *id_count = c->argc - argi; + *ids_argi = argi; + return C_OK; + } + /* Check what mode is set, if any. - * ex. [KEEPREF | DELREF | ACKED] IDS n [ID ...] - */ + * ex. [KEEPREF | DELREF | ACKED] IDS n [ID ...] */ *mode = PELMODE_KEEPREF; if (strcasecmp(objectGetVal(c->argv[argi]), "KEEPREF") == 0) { argi += 1; @@ -3734,19 +3735,61 @@ static int streamParseModeAndIDCountOrReply(client *c, int argi, streamPELMode * return C_OK; } +/* XDEL [ ... ] + * + * Removes the specified entries from the stream. Returns the number + * of items actually deleted, that may be different from the number + * of IDs passed in case certain IDs do not exist. */ +void xdelCommand(client *c) { + robj *o; + int ids_argi; + long long id_count; + streamCG *group; /* Unused: XDEL has no group argument. */ + streamPELMode mode; /* Unused: XDEL has no PEL mode argument. */ + if (streamParseXDelArgsOrReply(c, false, false, &o, &group, &mode, &ids_argi, &id_count) != C_OK) return; + + /* Missing key: reply as if zero entries were deleted. */ + if (o == NULL) { + addReply(c, shared.czero); + return; + } + stream *s = objectGetVal(o); + + /* We need to sanity check the IDs passed to start. Even if not + * a big issue, it is not great that the command is only partially + * executed because at some point an invalid ID is parsed. */ + streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; + streamID *ids = static_ids; + if (id_count > STREAMID_STATIC_VECTOR_LEN) ids = zmalloc(sizeof(streamID) * id_count); + if (streamParseStrictIDsOrReply(c, ids_argi, id_count, ids, NULL) != C_OK) { + goto cleanup; + } + + /* Actually apply the command. */ + int deleted = 0; + bool first_entry = 0; + for (long long j = 0; j < id_count; j++) { + streamID *id = &ids[j]; + streamDeleteItemAndTrackFirstLast(s, id, &first_entry, &deleted); + } + + /* Stream bookkeeping. */ + streamTrackFirstEntryAndPropagate(c, s, deleted, false, first_entry); + + addReplyLongLong(c, deleted); +cleanup: + if (ids != static_ids) zfree(ids); +} + /* XDELEX [KEEPREF | DELREF | ACKED] IDS num [ ... ] */ void xdelexCommand(client *c) { - robj *o = lookupKeyWrite(c->db, c->argv[1]); - if (o) { - if (checkType(c, o, OBJ_STREAM)) return; /* Type error. */ - } - - int argi = 2; - long long id_count = 0; - int id_argi = argi; + robj *o; + int ids_argi; + long long id_count; + streamCG *group; /* Unused: XDELEX has no group argument. */ streamPELMode mode; - if (streamParseModeAndIDCountOrReply(c, argi, &mode, &id_count, &id_argi) != C_OK) { + if (streamParseXDelArgsOrReply(c, false, true, &o, &group, &mode, &ids_argi, &id_count) != C_OK) { return; } @@ -3781,7 +3824,7 @@ void xdelexCommand(client *c) { /* Start parsing the IDs, so that we abort ASAP if there is a syntax * error giving "all or nothing" semantics. */ - if (streamParseDelIDsOrReply(c, id_argi, id_count, ids, resps) != C_OK) { + if (streamParseStrictIDsOrReply(c, ids_argi, id_count, ids, resps) != C_OK) { goto cleanup; } @@ -3809,7 +3852,7 @@ void xdelexCommand(client *c) { * propagate XACK's. Reset in loop after propagating each group. */ memset(cleared, 0, id_count); - /* Determine stream message existance upfront to ensure we mark entry as + /* Determine stream message existence upfront to ensure we mark entry as * "not found" for ACKED only after checking all groups' PELs. */ if (mode == PELMODE_ACKED) { for (int j = 0; j < id_count; j++) { @@ -3943,18 +3986,12 @@ void xdelexCommand(client *c) { * was acknowledged and deleted, and 2 means the message was acknowledged but * not deleted. */ void xackdelCommand(client *c) { - streamCG *group = NULL; - robj *o = lookupKeyWrite(c->db, c->argv[1]); - if (o) { - if (checkType(c, o, OBJ_STREAM)) return; /* Type error. */ - group = streamLookupCG(objectGetVal(o), objectGetVal(c->argv[2])); - } - - int argi = 3; - long long id_count = 0; - int id_argi = argi; + robj *o; + int ids_argi; + long long id_count; + streamCG *group; streamPELMode mode; - if (streamParseModeAndIDCountOrReply(c, argi, &mode, &id_count, &id_argi) != C_OK) { + if (streamParseXDelArgsOrReply(c, true, true, &o, &group, &mode, &ids_argi, &id_count) != C_OK) { return; } @@ -4002,7 +4039,7 @@ void xackdelCommand(client *c) { /* Start parsing the IDs, so that we abort ASAP if there is a syntax * error giving "all or nothing" semantics. */ - if (streamParseDelIDsOrReply(c, id_argi, id_count, ids, resps) != C_OK) { + if (streamParseStrictIDsOrReply(c, ids_argi, id_count, ids, resps) != C_OK) { goto cleanup; } diff --git a/tests/support/server.tcl b/tests/support/server.tcl index f6243e67838..d8bf888860b 100644 --- a/tests/support/server.tcl +++ b/tests/support/server.tcl @@ -618,7 +618,11 @@ proc start_server {options {code undefined}} { } set unixsocket [file normalize [format "%s/%s" [dict get $config "dir"] "socket"]] - dict set config "unixsocket" $unixsocket + # Skip unix socket if path is too long. macOS/BSD sun_path limit is 104 + # & linux sun_path limit is 108. + if {[string length $unixsocket] < 104} { + dict set config "unixsocket" $unixsocket + } # apply overrides from global space and arguments foreach {directive arguments} [concat $::global_overrides $overrides] { From 9ea19ed9c0690625048d3247939e67f3b09b7b22 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Wed, 9 Sep 2026 12:48:44 -0400 Subject: [PATCH 08/24] Align XDEL-like parse helper to others Renames to align naming with `genericZrangebyscoreCommand`, `genericZpopCommand`, `genericHgetallCommand`, `genericGetKeys`, etc. Also some Clang Format fixes. Signed-off-by: Nick Iaquinto --- src/server.c | 5 +++-- src/t_stream.c | 9 +++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/server.c b/src/server.c index 1baa1e8e983..0610ac2ea4e 100644 --- a/src/server.c +++ b/src/server.c @@ -7494,8 +7494,8 @@ void dismissMemoryInChild(void) { /* madvise(MADV_DONTNEED) may not work if Transparent Huge Pages is enabled. */ if (server.thp_enabled) return; - /* Currently we use zmadvise_dontneed only when we use jemalloc with Linux. - * so we avoid these pointless loops when they're not going to do anything. */ +/* Currently we use zmadvise_dontneed only when we use jemalloc with Linux. + * so we avoid these pointless loops when they're not going to do anything. */ #if defined(USE_JEMALLOC) && defined(__linux__) listIter li; listNode *ln; @@ -8247,3 +8247,4 @@ int parseExtendedCommandArgumentsOrReply(client *c, int command_type, int start_ } /* The End */ + diff --git a/src/t_stream.c b/src/t_stream.c index 79dca342e4e..65ff5b7dd54 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -3675,7 +3675,7 @@ typedef enum { * so each command keeps its own ID-array allocation strategy. * * Returns C_OK, or C_ERR with an error already replied to the client. */ -static int streamParseXDelArgsOrReply(client *c, bool has_group_arg, bool has_pelmode_arg, robj **o, streamCG **group, streamPELMode *mode, int *ids_argi, long long *id_count) { +static int genericXDelCommand(client *c, bool has_group_arg, bool has_pelmode_arg, robj **o, streamCG **group, streamPELMode *mode, int *ids_argi, long long *id_count) { *group = NULL; *o = lookupKeyWrite(c->db, c->argv[1]); if (*o && checkType(c, *o, OBJ_STREAM)) return C_ERR; /* Type error. */ @@ -3746,7 +3746,7 @@ void xdelCommand(client *c) { long long id_count; streamCG *group; /* Unused: XDEL has no group argument. */ streamPELMode mode; /* Unused: XDEL has no PEL mode argument. */ - if (streamParseXDelArgsOrReply(c, false, false, &o, &group, &mode, &ids_argi, &id_count) != C_OK) return; + if (genericXDelCommand(c, false, false, &o, &group, &mode, &ids_argi, &id_count) != C_OK) return; /* Missing key: reply as if zero entries were deleted. */ if (o == NULL) { @@ -3789,7 +3789,7 @@ void xdelexCommand(client *c) { long long id_count; streamCG *group; /* Unused: XDELEX has no group argument. */ streamPELMode mode; - if (streamParseXDelArgsOrReply(c, false, true, &o, &group, &mode, &ids_argi, &id_count) != C_OK) { + if (genericXDelCommand(c, false, true, &o, &group, &mode, &ids_argi, &id_count) != C_OK) { return; } @@ -3991,7 +3991,7 @@ void xackdelCommand(client *c) { long long id_count; streamCG *group; streamPELMode mode; - if (streamParseXDelArgsOrReply(c, true, true, &o, &group, &mode, &ids_argi, &id_count) != C_OK) { + if (genericXDelCommand(c, true, true, &o, &group, &mode, &ids_argi, &id_count) != C_OK) { return; } @@ -4751,3 +4751,4 @@ int streamValidateListpackIntegrity(unsigned char *lp, size_t size, uint64_t *va return 1; } + From 9fdb69aecb984b7712c44e865ad0d43c5e701256 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Wed, 9 Sep 2026 12:59:25 -0400 Subject: [PATCH 09/24] Revert unrelated testing change Signed-off-by: Nick Iaquinto --- tests/support/server.tcl | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/support/server.tcl b/tests/support/server.tcl index d8bf888860b..f6243e67838 100644 --- a/tests/support/server.tcl +++ b/tests/support/server.tcl @@ -618,11 +618,7 @@ proc start_server {options {code undefined}} { } set unixsocket [file normalize [format "%s/%s" [dict get $config "dir"] "socket"]] - # Skip unix socket if path is too long. macOS/BSD sun_path limit is 104 - # & linux sun_path limit is 108. - if {[string length $unixsocket] < 104} { - dict set config "unixsocket" $unixsocket - } + dict set config "unixsocket" $unixsocket # apply overrides from global space and arguments foreach {directive arguments} [concat $::global_overrides $overrides] { From 98c4c30ac9f9cdb168163cd362e527313df6955e Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Wed, 9 Sep 2026 13:07:41 -0400 Subject: [PATCH 10/24] Remove clunky XDELEX/XACKDEL helpers Signed-off-by: Nick Iaquinto --- src/t_stream.c | 127 +++++++++++++++++++++++++++---------------------- 1 file changed, 70 insertions(+), 57 deletions(-) diff --git a/src/t_stream.c b/src/t_stream.c index 65ff5b7dd54..b590d265ded 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -3604,52 +3604,6 @@ void xautoclaimCommand(client *c) { preventCommandPropagation(c); } -/* Maintain stream state by tracking the first entry & propagating keyspace - * events for deleted entries and/or PEL modifications. Used when deleting or - * acknowledging messages, as in XDEL, XDELEX, XACKDEL. - */ -void streamTrackFirstEntryAndPropagate(client *c, stream *s, int deleted, bool pel_modified, bool first_entry) { - /* Update the stream's first ID. */ - if (deleted) { - if (s->length == 0) { - s->first_id.ms = 0; - s->first_id.seq = 0; - } else if (first_entry) { - streamGetEdgeID(s, 1, 1, &s->first_id); - } - } - - /* Either deleting entries or a PEL-only change mutate consumer-group state - * on this key, so we need to signal in either case to WATCH-ers & keyspace - * subscribers (see issue #3429). */ - if (deleted || pel_modified) { - signalModifiedKey(c, c->db, c->argv[1]); - notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); - server.dirty += deleted; - } -} - -/* Delete a stream entry & do bookkeeping for first entry, last deleted entry, - * deleted count. Shared in XDEL & XDELEX. Returns 1 if entry was deleted, - * otherwise 0. */ -int streamDeleteItemAndTrackFirstLast(stream *s, streamID *id, bool *first_entry, int *deleted) { - if (streamDeleteItem(s, id)) { - /* We want to know if the first entry in the stream was deleted - * so we can later set the new one. */ - if (streamCompareID(id, &s->first_id) == 0) { - *first_entry = 1; - } - /* Update the stream's maximal tombstone if needed. */ - if (streamCompareID(id, &s->max_deleted_entry_id) > 0) { - s->max_deleted_entry_id = *id; - } - (*deleted)++; - return 1; - }; - - return 0; -} - /* PEL handling modes shared by XDELEX & XACKDEL. */ typedef enum { PELMODE_KEEPREF = 0, @@ -3770,11 +3724,33 @@ void xdelCommand(client *c) { bool first_entry = 0; for (long long j = 0; j < id_count; j++) { streamID *id = &ids[j]; - streamDeleteItemAndTrackFirstLast(s, id, &first_entry, &deleted); + if (streamDeleteItem(s, id)) { + deleted++; + /* We want to know if the first entry in the stream was deleted + * so we can later set the new one. */ + if (streamCompareID(id, &s->first_id) == 0) first_entry = 1; + /* Update the stream's maximal tombstone if needed. */ + if (streamCompareID(id, &s->max_deleted_entry_id) > 0) s->max_deleted_entry_id = *id; + } + } + + /* Update the stream's first ID. */ + if (deleted) { + if (s->length == 0) { + s->first_id.ms = 0; + s->first_id.seq = 0; + } else if (first_entry) { + streamGetEdgeID(s, 1, 1, &s->first_id); + } } - /* Stream bookkeeping. */ - streamTrackFirstEntryAndPropagate(c, s, deleted, false, first_entry); + /* Deleting entries mutates consumer-group state on this key, so we need + * to signal to WATCH-ers & keyspace subscribers (see issue #3429). */ + if (deleted) { + signalModifiedKey(c, c->db, c->argv[1]); + notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); + server.dirty += deleted; + } addReplyLongLong(c, deleted); cleanup: @@ -3944,19 +3920,41 @@ void xdelexCommand(client *c) { for (int j = 0; j < id_count; j++) { if (resps[j] == 1) { streamID *id = &ids[j]; - if (!streamDeleteItemAndTrackFirstLast(s, id, &first_entry, &deleted)) { + if (streamDeleteItem(s, id)) { + deleted++; + del_ids[del_count++] = *id; + /* We want to know if the first entry in the stream was deleted + * so we can later set the new one. */ + if (streamCompareID(id, &s->first_id) == 0) first_entry = 1; + /* Update the stream's maximal tombstone if needed. */ + if (streamCompareID(id, &s->max_deleted_entry_id) > 0) s->max_deleted_entry_id = *id; + } else { /* If the message does not exist, use -1 response code. * Necessary here b/c in KEEPREF and DELREF modes, we don't * check entry existence above. */ resps[j] = -1; - } else { - del_ids[del_count++] = *id; } } } - /* Stream bookkeeping. */ - streamTrackFirstEntryAndPropagate(c, s, deleted, pel_modified, first_entry); + /* Stream bookkeeping: update the stream's first ID, and signal WATCHed + * keys & emit the keyspace event before replying. Either deleting entries + * or a PEL-only change mutates consumer-group state on this key, so we + * need to signal in either case to WATCH-ers & keyspace subscribers (see + * issue #3429). */ + if (deleted) { + if (s->length == 0) { + s->first_id.ms = 0; + s->first_id.seq = 0; + } else if (first_entry) { + streamGetEdgeID(s, 1, 1, &s->first_id); + } + } + if (deleted || pel_modified) { + signalModifiedKey(c, c->db, c->argv[1]); + notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); + server.dirty += deleted; + } /* Propagate the effects as XACK/XDEL commands instead of XDELEX itself to * ensure compatibility with pre-9.2 replica. */ @@ -4223,8 +4221,24 @@ void xackdelCommand(client *c) { } sync: - /* Stream bookkeeping. */ - streamTrackFirstEntryAndPropagate(c, s, deleted, acked, first_entry); + /* Stream bookkeeping: update the stream's first ID, and signal WATCHed + * keys & emit the keyspace event before replying. Either deleting entries + * or removing PEL references mutates consumer-group state on this key, so + * we need to signal in either case to WATCH-ers & keyspace subscribers + * (see issue #3429). */ + if (deleted) { + if (s->length == 0) { + s->first_id.ms = 0; + s->first_id.seq = 0; + } else if (first_entry) { + streamGetEdgeID(s, 1, 1, &s->first_id); + } + } + if (deleted || acked) { + signalModifiedKey(c, c->db, c->argv[1]); + notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); + server.dirty += deleted; + } /* PEL entries can be removed without any stream deletion; keep the dirty * increment so save-point accounting reflects the mutation. */ @@ -4751,4 +4765,3 @@ int streamValidateListpackIntegrity(unsigned char *lp, size_t size, uint64_t *va return 1; } - From 71669bb86324544d2a0fd3416657d6e7b2ccd992 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Wed, 9 Sep 2026 13:20:32 -0400 Subject: [PATCH 11/24] Clang Format Signed-off-by: Nick Iaquinto --- src/server.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/server.c b/src/server.c index 0610ac2ea4e..0846a2d96aa 100644 --- a/src/server.c +++ b/src/server.c @@ -8247,4 +8247,3 @@ int parseExtendedCommandArgumentsOrReply(client *c, int command_type, int start_ } /* The End */ - From b814bcfccc74758dbbb466a7d4bac653d3343815 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Wed, 9 Sep 2026 13:57:25 -0400 Subject: [PATCH 12/24] Add streamDeletePELEntry helper across XACKDEL, XDELEX, XACK Signed-off-by: Nick Iaquinto --- src/t_stream.c | 112 +++++++++++++++++++++++-------------------------- 1 file changed, 52 insertions(+), 60 deletions(-) diff --git a/src/t_stream.c b/src/t_stream.c index b590d265ded..77c77c19620 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -2573,6 +2573,21 @@ void streamFreeNACK(streamNACK *na) { zfree(na); } +/* Delete a pending entry from the group PEL and from the PEL of the consumer + * owning it, freeing the NACK. Returns 1 if entry was pending and was deleted, + * 0 otherwise leaving both group & individual consumer PEL untouched. */ +static int streamDeletePELEntry(rax *pel, streamID *id) { + unsigned char buf[sizeof(streamID)]; + streamEncodeID(buf, id); + void *result; + if (!raxFind(pel, buf, sizeof(buf), &result)) return 0; + streamNACK *nack = result; + raxRemove(pel, buf, sizeof(buf), NULL); + raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); + streamFreeNACK(nack); + return 1; +} + /* Free a consumer and associated data structures. Note that this function * will not reassign the pending messages associated with this consumer * nor will delete them from the stream, so when this function is called @@ -2957,12 +2972,7 @@ void xackCommand(client *c) { /* Lookup the ID in the group PEL: it will have a reference to the * NACK structure that will have a reference to the consumer, so that * we are able to remove the entry from both PELs. */ - void *result; - if (raxFind(group->pel, buf, sizeof(buf), &result)) { - streamNACK *nack = result; - raxRemove(group->pel, buf, sizeof(buf), NULL); - raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); - streamFreeNACK(nack); + if (streamDeletePELEntry(group->pel, &ids[j - 3])) { acknowledged++; server.dirty++; } @@ -3847,34 +3857,33 @@ void xdelexCommand(client *c) { if (mode == PELMODE_ACKED && resps[j] == 2) continue; streamID *id = &ids[j]; - unsigned char buf[sizeof(streamID)]; - streamEncodeID(buf, id); - /* Check the PEL before consulting cg->last_id: XGROUP SETID - * can move last_id backward below IDs that are still pending - * (or were pending and later acked), so last_id alone cannot - * prove this group never claimed the message. */ - void *result; - if (raxFind(cg->pel, buf, sizeof(buf), &result)) { - if (mode == PELMODE_DELREF) { - /* DELREF: remove PEL entry from this group. */ - streamNACK *nack = result; - raxRemove(cg->pel, buf, sizeof(buf), NULL); - raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); - streamFreeNACK(nack); + if (mode == PELMODE_DELREF) { + /* DELREF: remove the PEL entry from this group. */ + if (streamDeletePELEntry(cg->pel, id)) { server.dirty++; pel_modified = 1; cleared[j] = 1; - } else { - /* ACKED: still pending in this group, cannot delete. */ + } + } else { + /* ACKED: check the PEL before consulting cg->last_id: + * XGROUP SETID can move last_id backward below IDs that + * are still pending (or were pending and later acked), + * so last_id alone cannot prove this group never claimed + * the message. */ + unsigned char buf[sizeof(streamID)]; + streamEncodeID(buf, id); + void *result; + if (raxFind(cg->pel, buf, sizeof(buf), &result)) { + /* Still pending in this group, cannot delete. */ + resps[j] = 2; + } else if (exists[j] && + streamCompareID(id, &cg->last_id) > 0) { + /* Message exists and may still be delivered to this + * group, so block deletion (same as XACKDEL). Entries + * that no longer exist can't be re-delivered. */ resps[j] = 2; } - } else if (mode == PELMODE_ACKED && exists[j] && - streamCompareID(id, &cg->last_id) > 0) { - /* Message exists and may still be delivered to this - * group, so block deletion (same as XACKDEL). Entries - * that no longer exist can't be re-delivered. */ - resps[j] = 2; } } @@ -4052,16 +4061,9 @@ void xackdelCommand(client *c) { for (long long j = 0; j < id_count; j++) { int response = -1; streamID *id = &ids[j]; - unsigned char buf[sizeof(streamID)]; - streamEncodeID(buf, id); /* ACK for the target group (but not others) */ - void *result; - if (raxFind(group->pel, buf, sizeof(buf), &result)) { - streamNACK *nack = result; - raxRemove(group->pel, buf, sizeof(buf), NULL); - raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); - streamFreeNACK(nack); + if (streamDeletePELEntry(group->pel, id)) { response = 1; acked++; acked_flags[j] = 1; @@ -4105,15 +4107,8 @@ void xackdelCommand(client *c) { * block deletion of messages the target is acking. */ for (long long j = 0; j < id_count; j++) { streamID *id = &ids[j]; - unsigned char buf[sizeof(streamID)]; - streamEncodeID(buf, id); - void *result; - if (raxFind(group->pel, buf, sizeof(buf), &result)) { - streamNACK *nack = result; - raxRemove(group->pel, buf, sizeof(buf), NULL); - raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); - streamFreeNACK(nack); + if (streamDeletePELEntry(group->pel, id)) { acked++; acked_flags[j] = 1; /* resps[j] stays 1: eligible for deletion (ACKED may still block it). */ @@ -4145,28 +4140,25 @@ void xackdelCommand(client *c) { } streamID *id = &ids[j]; - unsigned char buf[sizeof(streamID)]; - streamEncodeID(buf, id); - void *result; - if (raxFind(cg->pel, buf, sizeof(buf), &result)) { - if (mode == PELMODE_DELREF) { - /* DELREF: remove PEL entry from this group. */ - streamNACK *nack = result; - raxRemove(cg->pel, buf, sizeof(buf), NULL); - raxRemove(nack->consumer->pel, buf, sizeof(buf), NULL); - streamFreeNACK(nack); + if (mode == PELMODE_DELREF) { + /* DELREF: remove the PEL entry from this group. */ + if (streamDeletePELEntry(cg->pel, id)) { acked++; cleared[j] = 1; - } else { + } + } else { + unsigned char buf[sizeof(streamID)]; + streamEncodeID(buf, id); + void *result; + if (raxFind(cg->pel, buf, sizeof(buf), &result)) { /* Another group still has it pending. */ resps[j] = 2; + } else if (streamCompareID(id, &cg->last_id) > 0) { + /* Non-target hasn't claimed it yet; may still need to + * deliver it, so block deletion. */ + resps[j] = 2; } - } else if (mode == PELMODE_ACKED && - streamCompareID(id, &cg->last_id) > 0) { - /* Non-target hasn't claimed it yet; may still need to - * deliver it, so block deletion. */ - resps[j] = 2; } } From 37e669025e8e46e3b88f7a1dded2e90862baad46 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Wed, 9 Sep 2026 15:36:49 -0400 Subject: [PATCH 13/24] Mirror fix for XDELEX ACKED mode gap in XACKDEL as well Signed-off-by: Nick Iaquinto --- src/t_stream.c | 26 +++++++++++++++++--- tests/unit/type/stream.tcl | 50 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/src/t_stream.c b/src/t_stream.c index 77c77c19620..fe6aa03aa2e 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -4023,6 +4023,9 @@ void xackdelCommand(client *c) { unsigned char static_acked_flags[STREAMID_STATIC_VECTOR_LEN]; unsigned char *acked_flags = static_acked_flags; + unsigned char static_exists[STREAMID_STATIC_VECTOR_LEN]; + unsigned char *exists = static_exists; + unsigned char static_cleared[STREAMID_STATIC_VECTOR_LEN]; unsigned char *cleared = static_cleared; @@ -4037,6 +4040,7 @@ void xackdelCommand(client *c) { ids = zmalloc(sizeof(streamID) * id_count); resps = zmalloc(sizeof(int) * id_count); acked_flags = zmalloc(sizeof(unsigned char) * id_count); + exists = zmalloc(sizeof(unsigned char) * id_count); cleared = zmalloc(sizeof(unsigned char) * id_count); ack_ids = zmalloc(sizeof(streamID) * id_count); del_ids = zmalloc(sizeof(streamID) * id_count); @@ -4122,6 +4126,14 @@ void xackdelCommand(client *c) { * propagate XACK's. Reset in loop after propagating each group. */ memset(cleared, 0, id_count); + /* Determine stream message existence upfront to ensure we mark entry as + * "not found" for ACKED only after checking all groups' PELs. */ + if (mode == PELMODE_ACKED) { + for (long long j = 0; j < id_count; j++) { + exists[j] = streamEntryExists(s, &ids[j]); + } + } + raxIterator ri_cgroups; raxStart(&ri_cgroups, s->cgroups); raxSeek(&ri_cgroups, "^", NULL, 0); @@ -4148,15 +4160,22 @@ void xackdelCommand(client *c) { cleared[j] = 1; } } else { + /* ACKED: check the PEL before consulting cg->last_id: + * XGROUP SETID can move last_id backward below IDs that + * are still pending (or were pending and later acked), + * so last_id alone cannot prove this group never claimed + * the message. */ unsigned char buf[sizeof(streamID)]; streamEncodeID(buf, id); void *result; if (raxFind(cg->pel, buf, sizeof(buf), &result)) { /* Another group still has it pending. */ resps[j] = 2; - } else if (streamCompareID(id, &cg->last_id) > 0) { - /* Non-target hasn't claimed it yet; may still need to - * deliver it, so block deletion. */ + } else if (exists[j] && + streamCompareID(id, &cg->last_id) > 0) { + /* Message exists and may still be delivered to this + * group, so block deletion (same as XACKDEL). Entries + * that no longer exist can't be re-delivered. */ resps[j] = 2; } } @@ -4261,6 +4280,7 @@ void xackdelCommand(client *c) { if (ids != static_ids) zfree(ids); if (resps != static_resps) zfree(resps); if (acked_flags != static_acked_flags) zfree(acked_flags); + if (exists != static_exists) zfree(exists); if (cleared != static_cleared) zfree(cleared); if (ack_ids != static_ack_ids) zfree(ack_ids); if (del_ids != static_del_ids) zfree(del_ids); diff --git a/tests/unit/type/stream.tcl b/tests/unit/type/stream.tcl index c441190d288..4440279b18e 100644 --- a/tests/unit/type/stream.tcl +++ b/tests/unit/type/stream.tcl @@ -750,6 +750,56 @@ start_server { assert_equal {} [r xrange testxadstream 2 2] } + test {XACKDEL w/ ACKED acks dangling PEL reference after plain XDEL} { + r DEL testxadstream + r XADD testxadstream 1-0 msg hello + r XGROUP CREATE testxadstream testxadgrp1 0 + r XGROUP CREATE testxadstream testxadgrp2 0 + + # Only group 1 delivers the message; group 2's last_id stays at 0-0. + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + set pend [r XPENDING testxadstream testxadgrp1] + assert_equal 1-0 [lindex $pend 1] + + # Plain XDEL removes the entry but leaves group 1's PEL reference dangling. + assert_equal 1 [r XDEL testxadstream 1-0] + + # Acking the dangling reference replies 1 (acked, nothing left to + # delete), not 2 (blocked by group 2): the entry no longer exists and + # can never be delivered to group 2. + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 1-0] + assert_equal 1 [llength $ids] + assert_equal 1 [lindex $ids 0] + + # The dangling reference is gone from group 1's PEL and the stream is + # still empty. + assert_equal 0 [r XLEN testxadstream] + assert_equal {} [lindex [r XPENDING testxadstream testxadgrp1] 1] + assert_equal {} [lindex [r XPENDING testxadstream testxadgrp2] 1] + } + + test {XACKDEL w/ ACKED doesn't delete when 2nd group has message pending after plain XDEL} { + r DEL testxadstream + r XADD testxadstream 1-0 msg hello + r XGROUP CREATE testxadstream testxadgrp1 0 + r XGROUP CREATE testxadstream testxadgrp2 0 + + # Both groups deliver the message, then plain XDEL removes the entry, + # leaving both PEL references dangling. + r XREADGROUP GROUP testxadgrp1 testxadcnsmr COUNT 1 STREAMS testxadstream > + r XREADGROUP GROUP testxadgrp2 testxadcnsmr COUNT 1 STREAMS testxadstream > + assert_equal 1 [r XDEL testxadstream 1-0] + + # Group 2 still has the message pending, so deletion stays blocked (2). + set ids [r XACKDEL testxadstream testxadgrp1 ACKED IDS 1 1-0] + assert_equal 1 [llength $ids] + assert_equal 2 [lindex $ids 0] + + # Group 1 was acked, group 2's reference is untouched. + assert_equal {} [lindex [r XPENDING testxadstream testxadgrp1] 1] + assert_equal 1-0 [lindex [r XPENDING testxadstream testxadgrp2] 1] + } + test {XACKDEL w/ DELREF deletes from stream and 2nd consumer group's PEL even if not ACK'd} { r DEL testxadstream r XADD testxadstream 1 msg hello From c02928126e65256c11f596337248ad78156b953f Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Wed, 9 Sep 2026 16:18:31 -0400 Subject: [PATCH 14/24] Use size_t for id count to clarify constraints The ID count is parsed as `long long`, but is checked manually to avoid negative input or overflow. Adding the cast avoids a GCC 8 `-Wstringop-overflow` false positive. Signed-off-by: Nick Iaquinto --- src/t_stream.c | 58 ++++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/src/t_stream.c b/src/t_stream.c index fe6aa03aa2e..e7fc62f801c 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -63,7 +63,7 @@ size_t streamReplyWithRangeFromConsumerPEL(client *c, size_t count, streamConsumer *consumer); int streamParseStrictIDOrReply(client *c, robj *o, streamID *id, uint64_t missing_seq, int *seq_given); -int streamParseStrictIDsOrReply(client *c, int argi, long long id_count, streamID *ids, int *resps); +int streamParseStrictIDsOrReply(client *c, int argi, size_t id_count, streamID *ids, int *resps); int streamParseIDOrReply(client *c, robj *o, streamID *id, uint64_t missing_seq); /* ----------------------------------------------------------------------- @@ -2016,8 +2016,8 @@ int streamParseStrictIDOrReply(client *c, robj *o, streamID *id, uint64_t missin /* Parse IDS into array of stream message ids, ensuring each is a valid * stream message ID. Returns C_OK, or replies to the client on first invalid * ID and returns C_ERR. */ -int streamParseStrictIDsOrReply(client *c, int argi, long long id_count, streamID *ids, int *resps) { - for (long long j = 0; j < id_count; j++) { +int streamParseStrictIDsOrReply(client *c, int argi, size_t id_count, streamID *ids, int *resps) { + for (size_t j = 0; j < id_count; j++) { if (streamParseStrictIDOrReply(c, c->argv[argi + j], &ids[j], 0, NULL) != C_OK) return C_ERR; if (resps != NULL) resps[j] = 1; } @@ -3639,7 +3639,7 @@ typedef enum { * so each command keeps its own ID-array allocation strategy. * * Returns C_OK, or C_ERR with an error already replied to the client. */ -static int genericXDelCommand(client *c, bool has_group_arg, bool has_pelmode_arg, robj **o, streamCG **group, streamPELMode *mode, int *ids_argi, long long *id_count) { +static int genericXDelCommand(client *c, bool has_group_arg, bool has_pelmode_arg, robj **o, streamCG **group, streamPELMode *mode, int *ids_argi, size_t *id_count) { *group = NULL; *o = lookupKeyWrite(c->db, c->argv[1]); if (*o && checkType(c, *o, OBJ_STREAM)) return C_ERR; /* Type error. */ @@ -3683,18 +3683,23 @@ static int genericXDelCommand(client *c, bool has_group_arg, bool has_pelmode_ar argi++; /* past IDS */ /* Parse and validate numids: must be a positive integer. */ - if (getLongLongFromObject(c->argv[argi], id_count) != C_OK || *id_count <= 0) { + long long ll; + if (getLongLongFromObject(c->argv[argi], &ll) != C_OK || ll <= 0) { addReplyError(c, "Number of IDs must be a positive integer"); return C_ERR; } argi++; /* past numids */ /* Validate numids matches remaining arg count. */ - if (*id_count != c->argc - argi) { + if (ll != c->argc - argi) { addReplyErrorObject(c, shared.syntaxerr); return C_ERR; } + /* Cast is safe here b/c confirmed above for + * 0 <= id_count <= c->argc <= INT_MAX, so it always fits size_t. */ + *id_count = (size_t)ll; + *ids_argi = argi; return C_OK; } @@ -3707,7 +3712,7 @@ static int genericXDelCommand(client *c, bool has_group_arg, bool has_pelmode_ar void xdelCommand(client *c) { robj *o; int ids_argi; - long long id_count; + size_t id_count; streamCG *group; /* Unused: XDEL has no group argument. */ streamPELMode mode; /* Unused: XDEL has no PEL mode argument. */ if (genericXDelCommand(c, false, false, &o, &group, &mode, &ids_argi, &id_count) != C_OK) return; @@ -3732,7 +3737,7 @@ void xdelCommand(client *c) { /* Actually apply the command. */ int deleted = 0; bool first_entry = 0; - for (long long j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { streamID *id = &ids[j]; if (streamDeleteItem(s, id)) { deleted++; @@ -3772,7 +3777,7 @@ void xdelCommand(client *c) { void xdelexCommand(client *c) { robj *o; int ids_argi; - long long id_count; + size_t id_count; streamCG *group; /* Unused: XDELEX has no group argument. */ streamPELMode mode; if (genericXDelCommand(c, false, true, &o, &group, &mode, &ids_argi, &id_count) != C_OK) { @@ -3817,7 +3822,7 @@ void xdelexCommand(client *c) { /* If missing stream, return -1 for each ID. */ if (o == NULL) { addReplyArrayLen(c, id_count); - for (int i = 0; i < id_count; i++) { + for (size_t i = 0; i < id_count; i++) { addReplyLongLong(c, -1); } goto cleanup; @@ -3841,7 +3846,7 @@ void xdelexCommand(client *c) { /* Determine stream message existence upfront to ensure we mark entry as * "not found" for ACKED only after checking all groups' PELs. */ if (mode == PELMODE_ACKED) { - for (int j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { exists[j] = streamEntryExists(s, &ids[j]); } } @@ -3852,7 +3857,7 @@ void xdelexCommand(client *c) { while (raxNext(&ri_cgroups)) { streamCG *cg = ri_cgroups.data; - for (int j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { /* Skip messages already finalized. */ if (mode == PELMODE_ACKED && resps[j] == 2) continue; @@ -3892,7 +3897,7 @@ void xdelexCommand(client *c) { * XACK (see streamPropagateDelIDs for * why effects are propagated as primitive commands). */ int ack_count = 0; - for (int j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { if (cleared[j]) { ack_ids[ack_count++] = ids[j]; cleared[j] = 0; @@ -3910,7 +3915,7 @@ void xdelexCommand(client *c) { /* ACKED: Entries that don't exist and that no group references return * status "not found". */ if (mode == PELMODE_ACKED) { - for (int j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { if (resps[j] == 1 && !exists[j]) resps[j] = -1; } } @@ -3926,7 +3931,7 @@ void xdelexCommand(client *c) { * issue #3429). */ int deleted = 0; bool first_entry = 0; - for (int j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { if (resps[j] == 1) { streamID *id = &ids[j]; if (streamDeleteItem(s, id)) { @@ -3972,7 +3977,7 @@ void xdelexCommand(client *c) { /* Emit the array of per-ID results after the mutation has been signaled. */ addReplyArrayLen(c, id_count); - for (int j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { addReplyLongLong(c, resps[j]); } @@ -3995,7 +4000,7 @@ void xdelexCommand(client *c) { void xackdelCommand(client *c) { robj *o; int ids_argi; - long long id_count; + size_t id_count; streamCG *group; streamPELMode mode; if (genericXDelCommand(c, true, true, &o, &group, &mode, &ids_argi, &id_count) != C_OK) { @@ -4005,7 +4010,7 @@ void xackdelCommand(client *c) { /* If missing stream or group, return -1 for each ID. */ if (o == NULL || group == NULL) { addReplyArrayLen(c, id_count); - for (long long i = 0; i < id_count; i++) { + for (size_t i = 0; i < id_count; i++) { addReplyLongLong(c, -1); } return; @@ -4045,6 +4050,7 @@ void xackdelCommand(client *c) { ack_ids = zmalloc(sizeof(streamID) * id_count); del_ids = zmalloc(sizeof(streamID) * id_count); } + memset(acked_flags, 0, id_count); memset(cleared, 0, id_count); @@ -4062,7 +4068,7 @@ void xackdelCommand(client *c) { * group, we only need to loop over messages (and not consumers) and can set * responses inline. Thus, there's a separate setup for KEEPREF vs. ACKED/DELREF*/ if (mode == PELMODE_KEEPREF) { - for (long long j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { int response = -1; streamID *id = &ids[j]; @@ -4109,7 +4115,7 @@ void xackdelCommand(client *c) { * clearing a non-target's PEL entry before the target is confirmed to hold * the message, and in ACKED it ensures non-targets are only consulted to * block deletion of messages the target is acking. */ - for (long long j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { streamID *id = &ids[j]; if (streamDeletePELEntry(group->pel, id)) { @@ -4129,7 +4135,7 @@ void xackdelCommand(client *c) { /* Determine stream message existence upfront to ensure we mark entry as * "not found" for ACKED only after checking all groups' PELs. */ if (mode == PELMODE_ACKED) { - for (long long j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { exists[j] = streamEntryExists(s, &ids[j]); } } @@ -4144,7 +4150,7 @@ void xackdelCommand(client *c) { continue; } - for (long long j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { if (resps[j] != 1) { /* Skip when message wasn't found in target group (-1) * or when the message can't be deleted b/c of ACKED (2). */ @@ -4186,7 +4192,7 @@ void xackdelCommand(client *c) { * `XACK ` to ensure compatibility with * pre-9.2 replicas. */ int ack_count = 0; - for (long long j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { if (cleared[j]) { ack_ids[ack_count++] = ids[j]; cleared[j] = 0; @@ -4209,7 +4215,7 @@ void xackdelCommand(client *c) { * metadata bookkeeping and send signals first to meet the module keyspace * API contract (matching xdel, xtrim & other stream commands, see * issue #3429). */ - for (long long j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { /* Delete the message if needed. */ if (resps[j] == 1) { streamID *id = &ids[j]; @@ -4264,7 +4270,7 @@ void xackdelCommand(client *c) { preventCommandPropagation(c); int ack_count = 0; - for (long long j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { if (acked_flags[j]) ack_ids[ack_count++] = ids[j]; } streamPropagateAckIDs(c, c->argv[1], c->argv[2], ack_ids, ack_count); @@ -4272,7 +4278,7 @@ void xackdelCommand(client *c) { /* Emit the array of per-ID results after the mutation has been signaled. */ addReplyArrayLen(c, id_count); - for (long long j = 0; j < id_count; j++) { + for (size_t j = 0; j < id_count; j++) { addReplyLongLong(c, resps[j]); } From 96128f8de9fabaceef842629b016e59141982ce5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Thu, 10 Sep 2026 12:59:46 +0200 Subject: [PATCH 15/24] Unify XDEL, XDELEX and XACKDEL into a single xdelGenericCommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the separate xdelCommand, xdelexCommand and xackdelCommand implementations and the genericXDelCommand parse helper with a single xdelGenericCommand driven by an xdelVariant enum. The three public command functions become one-line wrappers. This halves the diff footprint of the XDELEX/XACKDEL feature in t_stream.c (~350 net lines vs ~670) by recognising that XDEL is a special case of XDELEX (KEEPREF without PEL awareness) and XDELEX is a special case of XACKDEL (no target consumer group). Signed-off-by: Viktor Söderqvist --- src/t_stream.c | 667 +++++++++++++------------------------------------ 1 file changed, 170 insertions(+), 497 deletions(-) diff --git a/src/t_stream.c b/src/t_stream.c index e7fc62f801c..bfae29db522 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -3621,404 +3621,97 @@ typedef enum { PELMODE_ACKED } streamPELMode; -/* Shared argument parsing of the XDEL-like commands: - * - * XDEL key ... - * XDELEX key [KEEPREF | DELREF | ACKED] IDS ... - * XACKDEL key [KEEPREF | DELREF | ACKED] IDS ... - * - * Parsing options include 1) if the command has a group arg (ie. XACKDEL) and - * 2) if the command has PEL mode (XACKDEL/XDELEX). - * - * This only parses the number of arguments, not the actual array for 2 reasons: - * 1) different commands have different reply shapes in some circumstances and - * 2) this simplifies memory management of the allocated ids array. +/* Command variant for xdelGenericCommand. */ +typedef enum { + XDEL_CMD, /* XDEL ... */ + XDELEX_CMD, /* XDELEX [KEEPREF|DELREF|ACKED] IDS ... */ + XACKDEL_CMD, /* XACKDEL [KEEPREF|DELREF|ACKED] IDS ... */ +} xdelVariant; + +/* XDEL [ ... ] + * XDELEX [KEEPREF | DELREF | ACKED] IDS num [ ... ] + * XACKDEL [KEEPREF | DELREF | ACKED] IDS num [ ... ] * - * On success, 'ids_argi' points at the first ID argument and 'id_count' - * holds the number of IDs. The IDs themselves are parsed by the callers - * so each command keeps its own ID-array allocation strategy. + * Unified implementation of XDEL, XDELEX and XACKDEL. * - * Returns C_OK, or C_ERR with an error already replied to the client. */ -static int genericXDelCommand(client *c, bool has_group_arg, bool has_pelmode_arg, robj **o, streamCG **group, streamPELMode *mode, int *ids_argi, size_t *id_count) { - *group = NULL; - *o = lookupKeyWrite(c->db, c->argv[1]); - if (*o && checkType(c, *o, OBJ_STREAM)) return C_ERR; /* Type error. */ - + * XDEL removes stream entries unconditionally. + * XDELEX is XDEL with PEL-awareness across all consumer groups. + * XACKDEL is XDELEX scoped to a target consumer group: it acknowledges + * entries in the target group first, then consults remaining groups. */ +static void xdelGenericCommand(client *c, xdelVariant variant) { + bool has_group = (variant == XACKDEL_CMD); + bool has_pelmode = (variant != XDEL_CMD); + bool array_reply = (variant != XDEL_CMD); + + /* --- Argument parsing ------------------------------------------------ */ + streamCG *group = NULL; + streamPELMode mode = PELMODE_KEEPREF; + robj *o = lookupKeyWrite(c->db, c->argv[1]); int argi = 2; - if (has_group_arg) { + + if (o && checkType(c, o, OBJ_STREAM)) return; /* Type error. */ + + if (has_group) { /* The group name is a positional argument: always consume it, even * when the key is missing (the lookup simply yields a NULL group). */ - if (*o) { - *group = streamLookupCG(objectGetVal(*o), objectGetVal(c->argv[argi])); + if (o) { + group = streamLookupCG(objectGetVal(o), objectGetVal(c->argv[argi])); } argi++; /* past group */ } - if (!has_pelmode_arg) { - /* XDEL has no IDS token, so the remaining args is the id count. */ - *mode = PELMODE_KEEPREF; - *id_count = c->argc - argi; - *ids_argi = argi; - return C_OK; - } - - /* Check what mode is set, if any. - * ex. [KEEPREF | DELREF | ACKED] IDS n [ID ...] */ - *mode = PELMODE_KEEPREF; - if (strcasecmp(objectGetVal(c->argv[argi]), "KEEPREF") == 0) { - argi += 1; - } else if (strcasecmp(objectGetVal(c->argv[argi]), "DELREF") == 0) { - argi += 1; - *mode = PELMODE_DELREF; - } else if (strcasecmp(objectGetVal(c->argv[argi]), "ACKED") == 0) { - argi += 1; - *mode = PELMODE_ACKED; - } - - /* Expect IDS token. */ - if (strcasecmp(objectGetVal(c->argv[argi]), "IDS") != 0) { - addReplyErrorObject(c, shared.syntaxerr); - return C_ERR; - } - argi++; /* past IDS */ - - /* Parse and validate numids: must be a positive integer. */ - long long ll; - if (getLongLongFromObject(c->argv[argi], &ll) != C_OK || ll <= 0) { - addReplyError(c, "Number of IDs must be a positive integer"); - return C_ERR; - } - argi++; /* past numids */ - - /* Validate numids matches remaining arg count. */ - if (ll != c->argc - argi) { - addReplyErrorObject(c, shared.syntaxerr); - return C_ERR; - } - - /* Cast is safe here b/c confirmed above for - * 0 <= id_count <= c->argc <= INT_MAX, so it always fits size_t. */ - *id_count = (size_t)ll; - - *ids_argi = argi; - return C_OK; -} - -/* XDEL [ ... ] - * - * Removes the specified entries from the stream. Returns the number - * of items actually deleted, that may be different from the number - * of IDs passed in case certain IDs do not exist. */ -void xdelCommand(client *c) { - robj *o; - int ids_argi; size_t id_count; - streamCG *group; /* Unused: XDEL has no group argument. */ - streamPELMode mode; /* Unused: XDEL has no PEL mode argument. */ - if (genericXDelCommand(c, false, false, &o, &group, &mode, &ids_argi, &id_count) != C_OK) return; - - /* Missing key: reply as if zero entries were deleted. */ - if (o == NULL) { - addReply(c, shared.czero); - return; - } - stream *s = objectGetVal(o); - - /* We need to sanity check the IDs passed to start. Even if not - * a big issue, it is not great that the command is only partially - * executed because at some point an invalid ID is parsed. */ - streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; - streamID *ids = static_ids; - if (id_count > STREAMID_STATIC_VECTOR_LEN) ids = zmalloc(sizeof(streamID) * id_count); - if (streamParseStrictIDsOrReply(c, ids_argi, id_count, ids, NULL) != C_OK) { - goto cleanup; - } - - /* Actually apply the command. */ - int deleted = 0; - bool first_entry = 0; - for (size_t j = 0; j < id_count; j++) { - streamID *id = &ids[j]; - if (streamDeleteItem(s, id)) { - deleted++; - /* We want to know if the first entry in the stream was deleted - * so we can later set the new one. */ - if (streamCompareID(id, &s->first_id) == 0) first_entry = 1; - /* Update the stream's maximal tombstone if needed. */ - if (streamCompareID(id, &s->max_deleted_entry_id) > 0) s->max_deleted_entry_id = *id; - } - } - - /* Update the stream's first ID. */ - if (deleted) { - if (s->length == 0) { - s->first_id.ms = 0; - s->first_id.seq = 0; - } else if (first_entry) { - streamGetEdgeID(s, 1, 1, &s->first_id); - } - } - - /* Deleting entries mutates consumer-group state on this key, so we need - * to signal to WATCH-ers & keyspace subscribers (see issue #3429). */ - if (deleted) { - signalModifiedKey(c, c->db, c->argv[1]); - notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); - server.dirty += deleted; - } - - addReplyLongLong(c, deleted); -cleanup: - if (ids != static_ids) zfree(ids); -} - -/* XDELEX [KEEPREF | DELREF | ACKED] IDS num [ ... ] - */ -void xdelexCommand(client *c) { - robj *o; - int ids_argi; - size_t id_count; - streamCG *group; /* Unused: XDELEX has no group argument. */ - streamPELMode mode; - if (genericXDelCommand(c, false, true, &o, &group, &mode, &ids_argi, &id_count) != C_OK) { - return; - } - - /* Space for tracking changes to make/propagated & response status codes */ - streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; - streamID *ids = static_ids; - - int static_resps[STREAMID_STATIC_VECTOR_LEN]; - int *resps = static_resps; - - streamID static_ack_ids[STREAMID_STATIC_VECTOR_LEN]; - streamID *ack_ids = static_ack_ids; - - streamID static_del_ids[STREAMID_STATIC_VECTOR_LEN]; - streamID *del_ids = static_del_ids; - int del_count = 0; - - unsigned char static_exists[STREAMID_STATIC_VECTOR_LEN]; - unsigned char *exists = static_exists; - - unsigned char static_cleared[STREAMID_STATIC_VECTOR_LEN]; - unsigned char *cleared = static_cleared; - - if (id_count > STREAMID_STATIC_VECTOR_LEN) { - ids = zmalloc(sizeof(streamID) * id_count); - resps = zmalloc(sizeof(int) * id_count); - ack_ids = zmalloc(sizeof(streamID) * id_count); - del_ids = zmalloc(sizeof(streamID) * id_count); - exists = zmalloc(sizeof(unsigned char) * id_count); - cleared = zmalloc(sizeof(unsigned char) * id_count); - } - - /* Start parsing the IDs, so that we abort ASAP if there is a syntax - * error giving "all or nothing" semantics. */ - if (streamParseStrictIDsOrReply(c, ids_argi, id_count, ids, resps) != C_OK) { - goto cleanup; - } - - /* If missing stream, return -1 for each ID. */ - if (o == NULL) { - addReplyArrayLen(c, id_count); - for (size_t i = 0; i < id_count; i++) { - addReplyLongLong(c, -1); - } - goto cleanup; - } - - stream *s = objectGetVal(o); - - /* True if DELREF removed any PEL references. PEL-only changes still modify - * the stream metadata, so they should signal WATCH/tracking and emit a - * keyspace event just like a stream-entry deletion (see issue #3429). */ - bool pel_modified = 0; - - /* For DELREF and ACKED modes: loop over consumer groups (outer) then messages - * (inner). This opens the iterator once instead of once per message, and - * allows inner-loop skips via the resps array. */ - if ((mode == PELMODE_DELREF || mode == PELMODE_ACKED) && s->cgroups != NULL) { - /* Tracks which PEL entries were cleared for this group so we can - * propagate XACK's. Reset in loop after propagating each group. */ - memset(cleared, 0, id_count); - - /* Determine stream message existence upfront to ensure we mark entry as - * "not found" for ACKED only after checking all groups' PELs. */ - if (mode == PELMODE_ACKED) { - for (size_t j = 0; j < id_count; j++) { - exists[j] = streamEntryExists(s, &ids[j]); - } - } - - raxIterator ri_cgroups; - raxStart(&ri_cgroups, s->cgroups); - raxSeek(&ri_cgroups, "^", NULL, 0); - while (raxNext(&ri_cgroups)) { - streamCG *cg = ri_cgroups.data; - - for (size_t j = 0; j < id_count; j++) { - /* Skip messages already finalized. */ - if (mode == PELMODE_ACKED && resps[j] == 2) continue; - - streamID *id = &ids[j]; - - if (mode == PELMODE_DELREF) { - /* DELREF: remove the PEL entry from this group. */ - if (streamDeletePELEntry(cg->pel, id)) { - server.dirty++; - pel_modified = 1; - cleared[j] = 1; - } - } else { - /* ACKED: check the PEL before consulting cg->last_id: - * XGROUP SETID can move last_id backward below IDs that - * are still pending (or were pending and later acked), - * so last_id alone cannot prove this group never claimed - * the message. */ - unsigned char buf[sizeof(streamID)]; - streamEncodeID(buf, id); - void *result; - if (raxFind(cg->pel, buf, sizeof(buf), &result)) { - /* Still pending in this group, cannot delete. */ - resps[j] = 2; - } else if (exists[j] && - streamCompareID(id, &cg->last_id) > 0) { - /* Message exists and may still be delivered to this - * group, so block deletion (same as XACKDEL). Entries - * that no longer exist can't be re-delivered. */ - resps[j] = 2; - } - } - } - - if (mode == PELMODE_DELREF) { - /* Propagate the PEL entries cleared for this group as - * XACK (see streamPropagateDelIDs for - * why effects are propagated as primitive commands). */ - int ack_count = 0; - for (size_t j = 0; j < id_count; j++) { - if (cleared[j]) { - ack_ids[ack_count++] = ids[j]; - cleared[j] = 0; - } - } - if (ack_count) { - robj *groupname = createStringObject((char *)ri_cgroups.key, ri_cgroups.key_len); - streamPropagateAckIDs(c, c->argv[1], groupname, ack_ids, ack_count); - decrRefCount(groupname); - } - } + if (!has_pelmode) { + /* XDEL has no IDS token, so the remaining args is the id count. */ + id_count = c->argc - argi; + } else { + /* Parse optional PEL mode: [KEEPREF | DELREF | ACKED] */ + if (strcasecmp(objectGetVal(c->argv[argi]), "KEEPREF") == 0) { + argi++; + } else if (strcasecmp(objectGetVal(c->argv[argi]), "DELREF") == 0) { + argi++; + mode = PELMODE_DELREF; + } else if (strcasecmp(objectGetVal(c->argv[argi]), "ACKED") == 0) { + argi++; + mode = PELMODE_ACKED; } - raxStop(&ri_cgroups); - /* ACKED: Entries that don't exist and that no group references return - * status "not found". */ - if (mode == PELMODE_ACKED) { - for (size_t j = 0; j < id_count; j++) { - if (resps[j] == 1 && !exists[j]) resps[j] = -1; - } + /* Expect IDS token. */ + if (strcasecmp(objectGetVal(c->argv[argi]), "IDS") != 0) { + addReplyErrorObject(c, shared.syntaxerr); + return; } - } + argi++; /* past IDS */ - /* Based on the response calculated above for each stream message, delete - * the messages if needed (catching not found messages and adjusting the - * response). - * - * This step doesn't enqueue the response yet, because we need to do stream - * metadata bookkeeping and send signals first to meet the module keyspace - * API contract (matching xdel, xtrim & other stream commands, see - * issue #3429). */ - int deleted = 0; - bool first_entry = 0; - for (size_t j = 0; j < id_count; j++) { - if (resps[j] == 1) { - streamID *id = &ids[j]; - if (streamDeleteItem(s, id)) { - deleted++; - del_ids[del_count++] = *id; - /* We want to know if the first entry in the stream was deleted - * so we can later set the new one. */ - if (streamCompareID(id, &s->first_id) == 0) first_entry = 1; - /* Update the stream's maximal tombstone if needed. */ - if (streamCompareID(id, &s->max_deleted_entry_id) > 0) s->max_deleted_entry_id = *id; - } else { - /* If the message does not exist, use -1 response code. - * Necessary here b/c in KEEPREF and DELREF modes, we don't - * check entry existence above. */ - resps[j] = -1; - } + /* Parse and validate numids: must be a positive integer. */ + long long ll; + if (getLongLongFromObject(c->argv[argi], &ll) != C_OK || ll <= 0) { + addReplyError(c, "Number of IDs must be a positive integer"); + return; } - } + argi++; /* past numids */ - /* Stream bookkeeping: update the stream's first ID, and signal WATCHed - * keys & emit the keyspace event before replying. Either deleting entries - * or a PEL-only change mutates consumer-group state on this key, so we - * need to signal in either case to WATCH-ers & keyspace subscribers (see - * issue #3429). */ - if (deleted) { - if (s->length == 0) { - s->first_id.ms = 0; - s->first_id.seq = 0; - } else if (first_entry) { - streamGetEdgeID(s, 1, 1, &s->first_id); + /* Validate numids matches remaining arg count. */ + if (ll != c->argc - argi) { + addReplyErrorObject(c, shared.syntaxerr); + return; } - } - if (deleted || pel_modified) { - signalModifiedKey(c, c->db, c->argv[1]); - notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); - server.dirty += deleted; - } - - /* Propagate the effects as XACK/XDEL commands instead of XDELEX itself to - * ensure compatibility with pre-9.2 replica. */ - preventCommandPropagation(c); - streamPropagateDelIDs(c, c->argv[1], del_ids, del_count); - - /* Emit the array of per-ID results after the mutation has been signaled. */ - addReplyArrayLen(c, id_count); - for (size_t j = 0; j < id_count; j++) { - addReplyLongLong(c, resps[j]); - } - -cleanup: - if (ids != static_ids) zfree(ids); - if (resps != static_resps) zfree(resps); - if (exists != static_exists) zfree(exists); - if (ack_ids != static_ack_ids) zfree(ack_ids); - if (del_ids != static_del_ids) zfree(del_ids); - if (cleared != static_cleared) zfree(cleared); -} - -/* XACKDEL [KEEPREF | DELREF | ACKED] IDS num [ ... ] - * - * Acknowledge one or more messages and delete them if all consumer groups have - * ack'd. Returns an array of integers equal to the number of ids. For this int - * array, -1 means the message does not exist in the stream, 1 means the message - * was acknowledged and deleted, and 2 means the message was acknowledged but - * not deleted. */ -void xackdelCommand(client *c) { - robj *o; - int ids_argi; - size_t id_count; - streamCG *group; - streamPELMode mode; - if (genericXDelCommand(c, true, true, &o, &group, &mode, &ids_argi, &id_count) != C_OK) { - return; + id_count = (size_t)ll; } - /* If missing stream or group, return -1 for each ID. */ - if (o == NULL || group == NULL) { - addReplyArrayLen(c, id_count); - for (size_t i = 0; i < id_count; i++) { - addReplyLongLong(c, -1); + /* --- Missing key / group early exit ---------------------------------- */ + if (o == NULL || (has_group && group == NULL)) { + if (array_reply) { + addReplyArrayLen(c, id_count); + for (size_t i = 0; i < id_count; i++) addReplyLongLong(c, -1); + } else { + addReply(c, shared.czero); } return; } - stream *s = objectGetVal(o); - /* Space for tracking changes to make/propagated & response status codes */ + /* --- Allocate working arrays ----------------------------------------- */ streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; streamID *ids = static_ids; @@ -4051,89 +3744,72 @@ void xackdelCommand(client *c) { del_ids = zmalloc(sizeof(streamID) * id_count); } - memset(acked_flags, 0, id_count); - memset(cleared, 0, id_count); - - /* Start parsing the IDs, so that we abort ASAP if there is a syntax - * error giving "all or nothing" semantics. */ - if (streamParseStrictIDsOrReply(c, ids_argi, id_count, ids, resps) != C_OK) { + /* We need to sanity check the IDs passed to start. Even if not + * a big issue, it is not great that the command is only partially + * executed because at some point an invalid ID is parsed. */ + if (streamParseStrictIDsOrReply(c, argi, id_count, ids, array_reply ? resps : NULL) != C_OK) { goto cleanup; } int acked = 0; int deleted = 0; - int first_entry = 0; + bool first_entry = 0; + memset(acked_flags, 0, id_count); - /* Fast path for KEEPREF. Since we only need to cleanup the PEL for the target - * group, we only need to loop over messages (and not consumers) and can set - * responses inline. Thus, there's a separate setup for KEEPREF vs. ACKED/DELREF*/ + /* --- KEEPREF fast path ----------------------------------------------- * + * When the mode is KEEPREF: with a target group we gate deletion on the + * entry being pending in that group's PEL; without a group we delete + * unconditionally (the original XDEL / XDELEX KEEPREF behaviour). */ if (mode == PELMODE_KEEPREF) { for (size_t j = 0; j < id_count; j++) { - int response = -1; streamID *id = &ids[j]; - /* ACK for the target group (but not others) */ - if (streamDeletePELEntry(group->pel, id)) { - response = 1; + if (group) { + /* XACKDEL KEEPREF: only delete if pending in target group. */ + if (!streamDeletePELEntry(group->pel, id)) { + if (array_reply) resps[j] = -1; + continue; + } acked++; acked_flags[j] = 1; - - /* Delete the message */ - if (streamDeleteItem(s, id)) { - deleted++; - del_ids[del_count++] = *id; - } - - /* We want to know if the first entry in the stream was deleted - * so we can later set the new one. */ - if (streamCompareID(id, &s->first_id) == 0) { - first_entry = 1; - } - - /* Update the stream's maximal tombstone if needed. */ - if (streamCompareID(id, &s->max_deleted_entry_id) > 0) { - s->max_deleted_entry_id = *id; - } } - resps[j] = response; + if (streamDeleteItem(s, id)) { + deleted++; + del_ids[del_count++] = *id; + if (streamCompareID(id, &s->first_id) == 0) first_entry = 1; + if (streamCompareID(id, &s->max_deleted_entry_id) > 0) s->max_deleted_entry_id = *id; + } else if (array_reply) { + /* Entry does not exist in the stream. */ + resps[j] = -1; + } } - goto sync; } - /* For ACKED & DELREF modes we use two phases. - * - * Phase 1: Check the target group. If a stream message isn't in the target - * group, we don't need to check other groups. - * - * Phase 2: Loops over other groups to do DELREF cleanup and ACKED blocking - * (but only for the messages that were present in the target group PEL - * from the checks in Phase 1). - * - * The target group decides eligibility first: in DELREF this prevents - * clearing a non-target's PEL entry before the target is confirmed to hold - * the message, and in ACKED it ensures non-targets are only consulted to - * block deletion of messages the target is acking. */ - for (size_t j = 0; j < id_count; j++) { - streamID *id = &ids[j]; - - if (streamDeletePELEntry(group->pel, id)) { - acked++; - acked_flags[j] = 1; - /* resps[j] stays 1: eligible for deletion (ACKED may still block it). */ - } else { - resps[j] = -1; /* Never delivered / already acked / doesn't exist. */ + /* --- Phase 1 (XACKDEL only): target-group PEL scan ------------------- * + * If the entry isn't pending in the target group we mark it -1 and skip + * it in Phase 2. XDELEX has no target group so this phase is skipped. */ + if (group) { + for (size_t j = 0; j < id_count; j++) { + if (streamDeletePELEntry(group->pel, &ids[j])) { + acked++; + acked_flags[j] = 1; + /* resps[j] stays 1: eligible for deletion. */ + } else { + resps[j] = -1; + } } } - if (s->cgroups != NULL) { - /* Tracks which PEL entries were cleared for this group so we can - * propagate XACK's. Reset in loop after propagating each group. */ + /* --- Phase 2: iterate consumer groups -------------------------------- * + * XDELEX iterates all groups uniformly; XACKDEL skips the target group + * (handled in Phase 1). The `if (cg == group) continue` naturally + * never fires when group is NULL (XDELEX). */ + if ((mode == PELMODE_DELREF || mode == PELMODE_ACKED) && s->cgroups != NULL) { memset(cleared, 0, id_count); - /* Determine stream message existence upfront to ensure we mark entry as - * "not found" for ACKED only after checking all groups' PELs. */ + /* Determine stream message existence upfront for ACKED mode. */ if (mode == PELMODE_ACKED) { for (size_t j = 0; j < id_count; j++) { exists[j] = streamEntryExists(s, &ids[j]); @@ -4145,52 +3821,34 @@ void xackdelCommand(client *c) { raxSeek(&ri_cgroups, "^", NULL, 0); while (raxNext(&ri_cgroups)) { streamCG *cg = ri_cgroups.data; - if (cg == group) { - /* Handled above in Phase 1. */ - continue; - } + if (cg == group) continue; /* Target group handled in Phase 1. */ for (size_t j = 0; j < id_count; j++) { - if (resps[j] != 1) { - /* Skip when message wasn't found in target group (-1) - * or when the message can't be deleted b/c of ACKED (2). */ - continue; - } + if (resps[j] != 1) continue; /* Already finalized. */ + if (mode == PELMODE_ACKED && resps[j] == 2) continue; streamID *id = &ids[j]; if (mode == PELMODE_DELREF) { - /* DELREF: remove the PEL entry from this group. */ if (streamDeletePELEntry(cg->pel, id)) { acked++; cleared[j] = 1; } } else { - /* ACKED: check the PEL before consulting cg->last_id: - * XGROUP SETID can move last_id backward below IDs that - * are still pending (or were pending and later acked), - * so last_id alone cannot prove this group never claimed - * the message. */ + /* ACKED: check PEL before consulting cg->last_id. */ unsigned char buf[sizeof(streamID)]; streamEncodeID(buf, id); void *result; if (raxFind(cg->pel, buf, sizeof(buf), &result)) { - /* Another group still has it pending. */ resps[j] = 2; } else if (exists[j] && streamCompareID(id, &cg->last_id) > 0) { - /* Message exists and may still be delivered to this - * group, so block deletion (same as XACKDEL). Entries - * that no longer exist can't be re-delivered. */ resps[j] = 2; } } } if (mode == PELMODE_DELREF) { - /* Propagate the PEL entries cleared for this group as - * `XACK ` to ensure compatibility with - * pre-9.2 replicas. */ int ack_count = 0; for (size_t j = 0; j < id_count; j++) { if (cleared[j]) { @@ -4206,43 +3864,39 @@ void xackdelCommand(client *c) { } } raxStop(&ri_cgroups); + + /* ACKED without a target group: entries that don't exist and that no + * group references return "not found". When a target group exists + * Phase 1 already marked non-pending entries as -1. */ + if (mode == PELMODE_ACKED && !group) { + for (size_t j = 0; j < id_count; j++) { + if (resps[j] == 1 && !exists[j]) resps[j] = -1; + } + } } - /* Based on the response calculated above for each stream message, delete - * the messages if needed. - * - * This step doesn't enqueue the response yet, because we need to do stream - * metadata bookkeeping and send signals first to meet the module keyspace - * API contract (matching xdel, xtrim & other stream commands, see - * issue #3429). */ + /* --- Deletion phase -------------------------------------------------- * + * Delete entries whose status is still 1 (eligible). */ for (size_t j = 0; j < id_count; j++) { - /* Delete the message if needed. */ if (resps[j] == 1) { streamID *id = &ids[j]; if (streamDeleteItem(s, id)) { deleted++; del_ids[del_count++] = *id; - } - - /* We want to know if the first entry in the stream was deleted - * so we can later set the new one. */ - if (streamCompareID(id, &s->first_id) == 0) { - first_entry = 1; - } - - /* Update the stream's maximal tombstone if needed. */ - if (streamCompareID(id, &s->max_deleted_entry_id) > 0) { - s->max_deleted_entry_id = *id; + if (streamCompareID(id, &s->first_id) == 0) first_entry = 1; + if (streamCompareID(id, &s->max_deleted_entry_id) > 0) s->max_deleted_entry_id = *id; + } else if (!acked_flags[j]) { + /* Entry doesn't exist and was never pending in the target + * group — genuinely not found. When acked_flags[j] is set + * the target-group PEL was successfully cleared in Phase 1, + * so the entry being already gone is fine (status stays 1). */ + resps[j] = -1; } } } sync: - /* Stream bookkeeping: update the stream's first ID, and signal WATCHed - * keys & emit the keyspace event before replying. Either deleting entries - * or removing PEL references mutates consumer-group state on this key, so - * we need to signal in either case to WATCH-ers & keyspace subscribers - * (see issue #3429). */ + /* --- Stream bookkeeping & signalling --------------------------------- */ if (deleted) { if (s->length == 0) { s->first_id.ms = 0; @@ -4256,30 +3910,34 @@ void xackdelCommand(client *c) { notifyKeyspaceEvent(NOTIFY_STREAM, "xdel", c->argv[1], c->db->id); server.dirty += deleted; } - - /* PEL entries can be removed without any stream deletion; keep the dirty - * increment so save-point accounting reflects the mutation. */ if (acked) { server.dirty += acked; } - /* Propagate the effects as XACK/XDEL commands instead of XACKDEL itself so - * that pre-9.2 replica's don't crash. - * - * Target-group acknowledgements first, then the deletions. */ - preventCommandPropagation(c); + /* --- Propagation ----------------------------------------------------- * + * XDELEX/XACKDEL are rewritten as XACK + XDEL primitives so that + * pre-9.2 replicas can apply them. XDEL propagates as itself. */ + if (has_pelmode) { + preventCommandPropagation(c); - int ack_count = 0; - for (size_t j = 0; j < id_count; j++) { - if (acked_flags[j]) ack_ids[ack_count++] = ids[j]; + /* Target-group acknowledgements (XACKDEL only). */ + if (group) { + int ack_count = 0; + for (size_t j = 0; j < id_count; j++) { + if (acked_flags[j]) ack_ids[ack_count++] = ids[j]; + } + streamPropagateAckIDs(c, c->argv[1], c->argv[2], ack_ids, ack_count); + } + + streamPropagateDelIDs(c, c->argv[1], del_ids, del_count); } - streamPropagateAckIDs(c, c->argv[1], c->argv[2], ack_ids, ack_count); - streamPropagateDelIDs(c, c->argv[1], del_ids, del_count); - /* Emit the array of per-ID results after the mutation has been signaled. */ - addReplyArrayLen(c, id_count); - for (size_t j = 0; j < id_count; j++) { - addReplyLongLong(c, resps[j]); + /* --- Reply ----------------------------------------------------------- */ + if (array_reply) { + addReplyArrayLen(c, id_count); + for (size_t j = 0; j < id_count; j++) addReplyLongLong(c, resps[j]); + } else { + addReplyLongLong(c, deleted); } cleanup: @@ -4292,6 +3950,21 @@ void xackdelCommand(client *c) { if (del_ids != static_del_ids) zfree(del_ids); } +/* XDEL [ ... ] */ +void xdelCommand(client *c) { + xdelGenericCommand(c, XDEL_CMD); +} + +/* XDELEX [KEEPREF | DELREF | ACKED] IDS num [ ... ] */ +void xdelexCommand(client *c) { + xdelGenericCommand(c, XDELEX_CMD); +} + +/* XACKDEL [KEEPREF | DELREF | ACKED] IDS num [ ... ] */ +void xackdelCommand(client *c) { + xdelGenericCommand(c, XACKDEL_CMD); +} + /* General form: XTRIM [... options ...] * * List of options: From 3e67a035b22fb10469ac4c56319aa8c5a963e239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Thu, 10 Sep 2026 13:08:54 +0200 Subject: [PATCH 16/24] Inline single-caller helper streamParseStrictIDsOrReply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit streamParseStrictIDsOrReply had exactly one call site after the xdelGenericCommand unification. Inline it to reduce indirection and shrink the diff from the merge-base. Signed-off-by: Viktor Söderqvist --- src/t_stream.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/t_stream.c b/src/t_stream.c index bfae29db522..d5f9983d598 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -63,7 +63,6 @@ size_t streamReplyWithRangeFromConsumerPEL(client *c, size_t count, streamConsumer *consumer); int streamParseStrictIDOrReply(client *c, robj *o, streamID *id, uint64_t missing_seq, int *seq_given); -int streamParseStrictIDsOrReply(client *c, int argi, size_t id_count, streamID *ids, int *resps); int streamParseIDOrReply(client *c, robj *o, streamID *id, uint64_t missing_seq); /* ----------------------------------------------------------------------- @@ -1626,6 +1625,7 @@ void streamPropagateConsumerCreation(client *c, robj *key, robj *groupname, sds decrRefCount(argv[4]); } + /* Propagate the deletion of stream entries as * * XDEL ... @@ -2013,16 +2013,6 @@ int streamParseStrictIDOrReply(client *c, robj *o, streamID *id, uint64_t missin return streamGenericParseIDOrReply(c, o, id, missing_seq, 1, seq_given); } -/* Parse IDS into array of stream message ids, ensuring each is a valid - * stream message ID. Returns C_OK, or replies to the client on first invalid - * ID and returns C_ERR. */ -int streamParseStrictIDsOrReply(client *c, int argi, size_t id_count, streamID *ids, int *resps) { - for (size_t j = 0; j < id_count; j++) { - if (streamParseStrictIDOrReply(c, c->argv[argi + j], &ids[j], 0, NULL) != C_OK) return C_ERR; - if (resps != NULL) resps[j] = 1; - } - return C_OK; -} /* Helper for parsing a stream ID that is a range query interval. When the * exclude argument is NULL, streamParseIDOrReply() is called and the interval @@ -3747,8 +3737,9 @@ static void xdelGenericCommand(client *c, xdelVariant variant) { /* We need to sanity check the IDs passed to start. Even if not * a big issue, it is not great that the command is only partially * executed because at some point an invalid ID is parsed. */ - if (streamParseStrictIDsOrReply(c, argi, id_count, ids, array_reply ? resps : NULL) != C_OK) { - goto cleanup; + for (size_t j = 0; j < id_count; j++) { + if (streamParseStrictIDOrReply(c, c->argv[argi + j], &ids[j], 0, NULL) != C_OK) goto cleanup; + if (array_reply) resps[j] = 1; } int acked = 0; From 03ff02d50d65e541d5b74a67d2f889fcb03b6538 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Thu, 10 Sep 2026 13:27:13 +0200 Subject: [PATCH 17/24] Clang format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Viktor Söderqvist --- src/t_stream.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/t_stream.c b/src/t_stream.c index d5f9983d598..a40c8531729 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -1625,7 +1625,6 @@ void streamPropagateConsumerCreation(client *c, robj *key, robj *groupname, sds decrRefCount(argv[4]); } - /* Propagate the deletion of stream entries as * * XDEL ... @@ -2013,7 +2012,6 @@ int streamParseStrictIDOrReply(client *c, robj *o, streamID *id, uint64_t missin return streamGenericParseIDOrReply(c, o, id, missing_seq, 1, seq_given); } - /* Helper for parsing a stream ID that is a range query interval. When the * exclude argument is NULL, streamParseIDOrReply() is called and the interval * is treated as close (inclusive). Otherwise, the exclude argument is set if @@ -3629,7 +3627,7 @@ typedef enum { * XACKDEL is XDELEX scoped to a target consumer group: it acknowledges * entries in the target group first, then consults remaining groups. */ static void xdelGenericCommand(client *c, xdelVariant variant) { - bool has_group = (variant == XACKDEL_CMD); + bool has_group = (variant == XACKDEL_CMD); bool has_pelmode = (variant != XDEL_CMD); bool array_reply = (variant != XDEL_CMD); From 3882fdc3d92c27c34ba3892e7da5a52d28d84535 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Thu, 10 Sep 2026 13:51:42 +0200 Subject: [PATCH 18/24] Only allocate working arrays each xdelGenericCommand variant needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Set unused array pointers to NULL so accidental access crashes immediately. For large id_count the heap path also skips allocations the variant does not need. Signed-off-by: Viktor Söderqvist --- src/t_stream.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/src/t_stream.c b/src/t_stream.c index a40c8531729..b9f53998bf3 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -3699,37 +3699,41 @@ static void xdelGenericCommand(client *c, xdelVariant variant) { } stream *s = objectGetVal(o); - /* --- Allocate working arrays ----------------------------------------- */ + /* --- Allocate working arrays ----------------------------------------- * + * Each variant only declares static buffers for the arrays it actually + * uses. Unused pointers are NULL so accidental access crashes rather + * than silently touching an unrelated stack buffer. For large id_count + * the heap path also skips allocations the variant does not need. */ streamID static_ids[STREAMID_STATIC_VECTOR_LEN]; streamID *ids = static_ids; + streamID static_del_ids[STREAMID_STATIC_VECTOR_LEN]; + streamID *del_ids = static_del_ids; + int del_count = 0; + int static_resps[STREAMID_STATIC_VECTOR_LEN]; - int *resps = static_resps; + int *resps = array_reply ? static_resps : NULL; unsigned char static_acked_flags[STREAMID_STATIC_VECTOR_LEN]; - unsigned char *acked_flags = static_acked_flags; + unsigned char *acked_flags = has_group ? static_acked_flags : NULL; unsigned char static_exists[STREAMID_STATIC_VECTOR_LEN]; - unsigned char *exists = static_exists; + unsigned char *exists = (mode == PELMODE_ACKED) ? static_exists : NULL; unsigned char static_cleared[STREAMID_STATIC_VECTOR_LEN]; - unsigned char *cleared = static_cleared; + unsigned char *cleared = (mode == PELMODE_DELREF || mode == PELMODE_ACKED) ? static_cleared : NULL; streamID static_ack_ids[STREAMID_STATIC_VECTOR_LEN]; - streamID *ack_ids = static_ack_ids; - - streamID static_del_ids[STREAMID_STATIC_VECTOR_LEN]; - streamID *del_ids = static_del_ids; - int del_count = 0; + streamID *ack_ids = (has_group || mode == PELMODE_DELREF || mode == PELMODE_ACKED) ? static_ack_ids : NULL; if (id_count > STREAMID_STATIC_VECTOR_LEN) { ids = zmalloc(sizeof(streamID) * id_count); - resps = zmalloc(sizeof(int) * id_count); - acked_flags = zmalloc(sizeof(unsigned char) * id_count); - exists = zmalloc(sizeof(unsigned char) * id_count); - cleared = zmalloc(sizeof(unsigned char) * id_count); - ack_ids = zmalloc(sizeof(streamID) * id_count); del_ids = zmalloc(sizeof(streamID) * id_count); + if (resps) resps = zmalloc(sizeof(int) * id_count); + if (acked_flags) acked_flags = zmalloc(sizeof(unsigned char) * id_count); + if (exists) exists = zmalloc(sizeof(unsigned char) * id_count); + if (cleared) cleared = zmalloc(sizeof(unsigned char) * id_count); + if (ack_ids) ack_ids = zmalloc(sizeof(streamID) * id_count); } /* We need to sanity check the IDs passed to start. Even if not @@ -3743,7 +3747,7 @@ static void xdelGenericCommand(client *c, xdelVariant variant) { int acked = 0; int deleted = 0; bool first_entry = 0; - memset(acked_flags, 0, id_count); + if (acked_flags) memset(acked_flags, 0, id_count); /* --- KEEPREF fast path ----------------------------------------------- * * When the mode is KEEPREF: with a target group we gate deletion on the @@ -3874,7 +3878,7 @@ static void xdelGenericCommand(client *c, xdelVariant variant) { del_ids[del_count++] = *id; if (streamCompareID(id, &s->first_id) == 0) first_entry = 1; if (streamCompareID(id, &s->max_deleted_entry_id) > 0) s->max_deleted_entry_id = *id; - } else if (!acked_flags[j]) { + } else if (!acked_flags || !acked_flags[j]) { /* Entry doesn't exist and was never pending in the target * group — genuinely not found. When acked_flags[j] is set * the target-group PEL was successfully cleared in Phase 1, From 11ed5b7ee00067bc4682d207005a79686ec3afcc Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Thu, 10 Sep 2026 09:30:23 -0400 Subject: [PATCH 19/24] Clarify Complexity in XDELEX JSON Defn Co-authored-by: Harkrishn Patro Signed-off-by: Nick Iaquinto --- src/commands/xdelex.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/xdelex.json b/src/commands/xdelex.json index 7ae1f17dc47..11fa5a18ae8 100644 --- a/src/commands/xdelex.json +++ b/src/commands/xdelex.json @@ -1,7 +1,7 @@ { "XDELEX": { "summary": "Delete stream message(s) with extended options", - "complexity": "O(1)", + "complexity": "O(1) for each single item to delete in the stream, regardless of the stream size", "group": "stream", "since": "9.2.0", "arity": -5, From 07661382e9cb0a0964aa71e5f4e2fea753e50c96 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Thu, 10 Sep 2026 09:30:49 -0400 Subject: [PATCH 20/24] Clarify Complexity in XACKDEL JSON Defn Co-authored-by: Harkrishn Patro Signed-off-by: Nick Iaquinto --- src/commands/xackdel.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/xackdel.json b/src/commands/xackdel.json index f4f1251b74c..62d0e7ab3e9 100644 --- a/src/commands/xackdel.json +++ b/src/commands/xackdel.json @@ -1,7 +1,7 @@ { "XACKDEL": { "summary": "Acknowledge and (if possible) delete stream message(s).", - "complexity": "O(1)", + "complexity": "O(1) for each single item to delete in the stream, regardless of the stream size", "group": "stream", "since": "9.2.0", "arity": -6, From 75b1be9c21795202f127a905f0347e64090263c1 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Thu, 10 Sep 2026 09:58:23 -0400 Subject: [PATCH 21/24] Revert formatting change to unrelated line Signed-off-by: Nick Iaquinto --- src/server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server.c b/src/server.c index 0846a2d96aa..1baa1e8e983 100644 --- a/src/server.c +++ b/src/server.c @@ -7494,8 +7494,8 @@ void dismissMemoryInChild(void) { /* madvise(MADV_DONTNEED) may not work if Transparent Huge Pages is enabled. */ if (server.thp_enabled) return; -/* Currently we use zmadvise_dontneed only when we use jemalloc with Linux. - * so we avoid these pointless loops when they're not going to do anything. */ + /* Currently we use zmadvise_dontneed only when we use jemalloc with Linux. + * so we avoid these pointless loops when they're not going to do anything. */ #if defined(USE_JEMALLOC) && defined(__linux__) listIter li; listNode *ln; From 7a0addb26cdfdc500b8a08ea27175580efb61bed Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Thu, 10 Sep 2026 10:06:04 -0400 Subject: [PATCH 22/24] Revert formatting change to unrelated line Signed-off-by: Nick Iaquinto --- src/server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server.c b/src/server.c index 1baa1e8e983..675d7252536 100644 --- a/src/server.c +++ b/src/server.c @@ -7494,8 +7494,8 @@ void dismissMemoryInChild(void) { /* madvise(MADV_DONTNEED) may not work if Transparent Huge Pages is enabled. */ if (server.thp_enabled) return; - /* Currently we use zmadvise_dontneed only when we use jemalloc with Linux. - * so we avoid these pointless loops when they're not going to do anything. */ + /* Currently we use zmadvise_dontneed only when we use jemalloc with Linux. + * so we avoid these pointless loops when they're not going to do anything. */ #if defined(USE_JEMALLOC) && defined(__linux__) listIter li; listNode *ln; From 51d6e795339587f10993b6ee89cfdbd482f58618 Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Thu, 10 Sep 2026 10:28:09 -0400 Subject: [PATCH 23/24] Regenerate commands.def after accepting JSON changes in GitHub Signed-off-by: Nick Iaquinto --- src/commands.def | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/commands.def b/src/commands.def index 6d205bd71b1..3fadac8f357 100644 --- a/src/commands.def +++ b/src/commands.def @@ -12285,12 +12285,12 @@ struct COMMAND_STRUCT serverCommandTable[] = { {MAKE_CMD("zunionstore","Stores the union of multiple sorted sets in a key.","O(N)+O(M log(M)) with N being the sum of the sizes of the input sorted sets, and M being the number of elements in the resulting sorted set.","2.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZUNIONSTORE_History,0,ZUNIONSTORE_Tips,0,zunionstoreCommand,-4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZUNIONSTORE_Keyspecs,2,zunionInterDiffStoreGetKeys,5),.args=ZUNIONSTORE_Args}, /* stream */ {MAKE_CMD("xack","Returns the number of messages that were successfully acknowledged by the consumer group member of a stream.","O(1) for each message ID processed.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XACK_History,0,XACK_Tips,0,xackCommand,-4,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XACK_Keyspecs,1,NULL,3),.args=XACK_Args}, -{MAKE_CMD("xackdel","Acknowledge and (if possible) delete stream message(s).","O(1)","9.2.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XACKDEL_History,0,XACKDEL_Tips,0,xackdelCommand,-6,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_WRITE|ACL_CATEGORY_STREAM,NULL,XACKDEL_Keyspecs,1,NULL,6),.args=XACKDEL_Args}, +{MAKE_CMD("xackdel","Acknowledge and (if possible) delete stream message(s).","O(1) for each single item to delete in the stream, regardless of the stream size","9.2.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XACKDEL_History,0,XACKDEL_Tips,0,xackdelCommand,-6,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_WRITE|ACL_CATEGORY_STREAM,NULL,XACKDEL_Keyspecs,1,NULL,6),.args=XACKDEL_Args}, {MAKE_CMD("xadd","Appends a new message to a stream. Creates the key if it doesn't exist.","O(1) when adding a new entry, O(N) when trimming where N being the number of entries evicted.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XADD_History,2,XADD_Tips,1,xaddCommand,-5,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XADD_Keyspecs,1,NULL,5),.args=XADD_Args}, {MAKE_CMD("xautoclaim","Changes, or acquires, ownership of messages in a consumer group, as if the messages were delivered to a consumer group member.","O(1) if COUNT is small.","6.2.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XAUTOCLAIM_History,1,XAUTOCLAIM_Tips,1,xautoclaimCommand,-6,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XAUTOCLAIM_Keyspecs,1,NULL,7),.args=XAUTOCLAIM_Args}, {MAKE_CMD("xclaim","Changes, or acquires, ownership of a message in a consumer group, as if the message was delivered to a consumer group member.","O(log N) with N being the number of messages in the PEL of the consumer group.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XCLAIM_History,0,XCLAIM_Tips,1,xclaimCommand,-6,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XCLAIM_Keyspecs,1,NULL,11),.args=XCLAIM_Args}, {MAKE_CMD("xdel","Returns the number of messages after removing them from a stream.","O(1) for each single item to delete in the stream, regardless of the stream size.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XDEL_History,0,XDEL_Tips,0,xdelCommand,-3,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_STREAM|ACL_CATEGORY_WRITE,NULL,XDEL_Keyspecs,1,NULL,2),.args=XDEL_Args}, -{MAKE_CMD("xdelex","Delete stream message(s) with extended options","O(1)","9.2.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XDELEX_History,0,XDELEX_Tips,0,xdelexCommand,-5,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_WRITE|ACL_CATEGORY_STREAM,NULL,XDELEX_Keyspecs,1,NULL,5),.args=XDELEX_Args}, +{MAKE_CMD("xdelex","Delete stream message(s) with extended options","O(1) for each single item to delete in the stream, regardless of the stream size","9.2.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XDELEX_History,0,XDELEX_Tips,0,xdelexCommand,-5,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_WRITE|ACL_CATEGORY_STREAM,NULL,XDELEX_Keyspecs,1,NULL,5),.args=XDELEX_Args}, {MAKE_CMD("xgroup","A container for consumer groups commands.","Depends on subcommand.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XGROUP_History,0,XGROUP_Tips,0,NULL,-2,0,ACL_CATEGORY_SLOW,NULL,XGROUP_Keyspecs,0,NULL,0),.subcommands=XGROUP_Subcommands}, {MAKE_CMD("xinfo","A container for stream introspection commands.","Depends on subcommand.","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XINFO_History,0,XINFO_Tips,0,NULL,-2,0,ACL_CATEGORY_SLOW,NULL,XINFO_Keyspecs,0,NULL,0),.subcommands=XINFO_Subcommands}, {MAKE_CMD("xlen","Returns the number of messages in a stream.","O(1)","5.0.0",CMD_DOC_NONE,NULL,NULL,"stream",COMMAND_GROUP_STREAM,XLEN_History,0,XLEN_Tips,0,xlenCommand,2,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_STREAM,NULL,XLEN_Keyspecs,1,NULL,1),.args=XLEN_Args}, From 704fca79d6c2ece8256ec9df6239d35bbdb646de Mon Sep 17 00:00:00 2001 From: Nick Iaquinto Date: Thu, 10 Sep 2026 10:29:23 -0400 Subject: [PATCH 24/24] Add Cross Version (pre-9.2.0) Replication Tests Tested with... ``` make -j $(nproc) ./runtest \ --verbose --dump-logs \ --single tests/integration/cross-version-replication.tcl \ --other-server-path /path/to/9.1.2/checkout/src/valkey-server ``` Signed-off-by: Nick Iaquinto --- .../integration/cross-version-replication.tcl | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/integration/cross-version-replication.tcl b/tests/integration/cross-version-replication.tcl index 0e3de03de3f..48ee1d89c49 100644 --- a/tests/integration/cross-version-replication.tcl +++ b/tests/integration/cross-version-replication.tcl @@ -87,4 +87,48 @@ start_server {tags {"repl needs:other-server external:skip"}} { assert_equal value1 [$old_replica hget hfe field1] } } + + test "XACKDEL replicates as equivalent pre-9.2 commands XACK/XDEL for backwards compatibility" { + if {[version_greater_or_equal $old_replica_version 9.2.0]} { + skip "Replica $old_replica_version must be before 9.2.0 for this test" + } + + r FLUSHALL + r XADD mystream 1-0 hello world + r XGROUP CREATE mystream grp1 0 + r XGROUP CREATE mystream grp2 0 + r XREADGROUP GROUP grp1 alice COUNT 1 STREAMS mystream > + r XREADGROUP GROUP grp2 bob COUNT 1 STREAMS mystream > + r XACKDEL mystream grp1 DELREF IDS 1 1-0 + start_server {start-other-server 1 config "minimal.conf"} { + set old_replica [srv 0 client] + $old_replica replicaof $primary_host $primary_port + wait_for_sync $old_replica 500 100 + assert_equal [llength [$old_replica XRANGE mystream - +]] 0 + assert_equal [llength [$old_replica XPENDING mystream grp1 - + 10]] 0 + assert_equal [llength [$old_replica XPENDING mystream grp2 - + 10]] 0 + } + } + + test "XDELEX replicates as equivalent pre-9.2 commands XACK/XDEL for backwards compatibility" { + if {[version_greater_or_equal $old_replica_version 9.2.0]} { + skip "Replica $old_replica_version must be before 9.2.0 for this test" + } + + r FLUSHALL + r XADD mystream 1-0 hello world + r XGROUP CREATE mystream grp1 0 + r XGROUP CREATE mystream grp2 0 + r XREADGROUP GROUP grp1 alice COUNT 1 STREAMS mystream > + r XREADGROUP GROUP grp2 bob COUNT 1 STREAMS mystream > + r XDELEX mystream DELREF IDS 1 1-0 + start_server {start-other-server 1 config "minimal.conf"} { + set old_replica [srv 0 client] + $old_replica replicaof $primary_host $primary_port + wait_for_sync $old_replica 500 100 + assert_equal [llength [$old_replica XRANGE mystream - +]] 0 + assert_equal [llength [$old_replica XPENDING mystream grp1 - + 10]] 0 + assert_equal [llength [$old_replica XPENDING mystream grp2 - + 10]] 0 + } + } }