From 333a9b1168e478bbb824f06c9f2ddbad25481292 Mon Sep 17 00:00:00 2001 From: kirito632 Date: Fri, 22 May 2026 15:52:22 +0800 Subject: [PATCH 1/4] fix(stream): filter consumers by group name in GetConsumerInfo to prevent cross-group leakage --- src/types/redis_stream.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/types/redis_stream.cc b/src/types/redis_stream.cc index 580629c9fb1..0c6aa8b08c1 100644 --- a/src/types/redis_stream.cc +++ b/src/types/redis_stream.cc @@ -1389,6 +1389,11 @@ rocksdb::Status Stream::GetConsumerInfo( continue; } + // Iterate bounds may include consumers from other groups; verify group name explicitly. + if (groupNameFromInternalKey(iter->key()) != group_name) { + continue; + } + std::string consumer_name = consumerNameFromInternalKey(iter->key()); StreamConsumerMetadata c_metadata = decodeStreamConsumerMetadataValue(iter->value().ToString()); std::pair tmp_item(consumer_name, c_metadata); From 18e8c869df4bc14a1a3a22b5586c0bbd6f3aa57d Mon Sep 17 00:00:00 2001 From: kirito632 Date: Tue, 26 May 2026 11:21:04 +0800 Subject: [PATCH 2/4] feat(stream): support XACKDEL command --- src/commands/cmd_stream.cc | 74 +++ src/storage/batch_extractor.cc | 82 ++- src/storage/batch_extractor.h | 2 + src/types/redis_stream.cc | 420 +++++++++++++ src/types/redis_stream.h | 23 + src/types/redis_stream_base.h | 12 + tests/gocase/unit/server/poll_updates_test.go | 1 + tests/gocase/unit/type/stream/stream_test.go | 577 ++++++++++++++++++ 8 files changed, 1190 insertions(+), 1 deletion(-) diff --git a/src/commands/cmd_stream.cc b/src/commands/cmd_stream.cc index 73a5279c8eb..9b480b469a6 100644 --- a/src/commands/cmd_stream.cc +++ b/src/commands/cmd_stream.cc @@ -268,6 +268,79 @@ class CommandXDel : public Commander { std::vector ids_; }; +class CommandXAckDel : public Commander { + public: + Status Parse(const std::vector &args) override { + CommandParser parser(args, 1); + stream_name_ = GET_OR_RET(parser.TakeStr()); + group_name_ = GET_OR_RET(parser.TakeStr()); + + option_ = redis::StreamDeleteOption::KeepRef; + + while (parser.Good() && !util::EqualICase(parser.RawPeek(), "IDS")) { + if (parser.EatEqICase("KEEPREF")) { + option_ = redis::StreamDeleteOption::KeepRef; + } else if (parser.EatEqICase("DELREF")) { + option_ = redis::StreamDeleteOption::DelRef; + } else if (parser.EatEqICase("ACKED")) { + option_ = redis::StreamDeleteOption::Acked; + } else { + return parser.InvalidSyntax(); + } + } + + if (!parser.EatEqICase("IDS")) { + return {Status::RedisParseErr, "syntax error, expected IDS keyword"}; + } + + auto numids_result = parser.TakeInt(); + if (!numids_result.IsOK()) { + return {Status::RedisParseErr, errValueNotInteger}; + } + int64_t numids = numids_result.GetValue(); + if (numids <= 0) { + return {Status::RedisParseErr, "numids must be positive"}; + } + + for (int64_t i = 0; i < numids; i++) { + auto id_str = GET_OR_RET(parser.TakeStr()); + redis::StreamEntryID id; + auto s = ParseStreamEntryID(id_str, &id); + if (!s.IsOK()) return s; + entry_ids_.emplace_back(id); + } + + if (parser.Good()) { + return {Status::RedisParseErr, "syntax error, unexpected trailing arguments after IDs"}; + } + + return Status::OK(); + } + + Status Execute(engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override { + redis::Stream stream_db(srv->storage, conn->GetNamespace()); + std::vector results; + + auto s = stream_db.DeleteEntriesAndAck(ctx, stream_name_, group_name_, entry_ids_, option_, &results); + if (!s.ok()) { + return {Status::RedisExecErr, s.ToString()}; + } + + output->append(redis::MultiLen(results.size())); + for (int r : results) { + output->append(redis::Integer(r)); + } + + return Status::OK(); + } + + private: + std::string stream_name_; + std::string group_name_; + redis::StreamDeleteOption option_ = redis::StreamDeleteOption::KeepRef; + std::vector entry_ids_; +}; + class CommandXClaim : public Commander { public: Status Parse(const std::vector &args) override { @@ -1907,6 +1980,7 @@ class CommandXSetId : public Commander { REDIS_REGISTER_COMMANDS(Stream, MakeCmdAttr("xack", -4, "write no-dbsize-check", 1, 1, 1), MakeCmdAttr("xadd", -5, "write", 1, 1, 1), MakeCmdAttr("xdel", -3, "write no-dbsize-check", 1, 1, 1), + MakeCmdAttr("xackdel", -6, "write no-dbsize-check", 1, 1, 1), MakeCmdAttr("xclaim", -6, "write", 1, 1, 1), MakeCmdAttr("xautoclaim", -6, "write", 1, 1, 1), MakeCmdAttr("xgroup", -4, "write", 2, 2, 1), diff --git a/src/storage/batch_extractor.cc b/src/storage/batch_extractor.cc index 8dfd4258e7e..670fb2bb7bc 100644 --- a/src/storage/batch_extractor.cc +++ b/src/storage/batch_extractor.cc @@ -26,6 +26,7 @@ #include "server/redis_reply.h" #include "server/server.h" #include "types/redis_bitmap.h" +#include "types/redis_stream_base.h" void WriteBatchExtractor::LogData(const rocksdb::Slice &blob) { // Currently, we only have two kinds of log data @@ -38,6 +39,8 @@ void WriteBatchExtractor::LogData(const rocksdb::Slice &blob) { // Redis type log data if (auto s = log_data_.Decode(blob); !s.IsOK()) { WARN("Failed to decode Redis type log: {}", s.Msg()); + } else { + seen_xackdel_entry_keys_.clear(); } } } @@ -266,6 +269,21 @@ rocksdb::Status WriteBatchExtractor::PutCF(uint32_t column_family_id, const Slic break; } } else if (column_family_id == static_cast(ColumnFamilyID::Stream)) { + InternalKey ikey(key, is_slot_id_encoded_); + Slice entry_id_check = ikey.GetSubKey(); + uint64_t delimiter = 0; + GetFixed64(&entry_id_check, &delimiter); + if (delimiter == UINT64_MAX) { + return rocksdb::Status::OK(); + } + + user_key = ikey.GetKey().ToString(); + auto key_slot_id = GetSlotIdFromKey(user_key); + if (slot_range_.IsValid() && !slot_range_.Contains(key_slot_id)) { + return rocksdb::Status::OK(); + } + ns = ikey.GetNamespace().ToString(); + auto s = ExtractStreamAddCommand(is_slot_id_encoded_, key, value, &command_args); if (!s.IsOK()) { ERROR("Failed to parse write_batch in PutCF. Type=Stream: {}", s.Msg()); @@ -397,8 +415,70 @@ rocksdb::Status WriteBatchExtractor::DeleteCF(uint32_t column_family_id, const S Slice encoded_id = ikey.GetSubKey(); redis::StreamEntryID entry_id; GetFixed64(&encoded_id, &entry_id.ms); + + if (entry_id.ms == UINT64_MAX) { + // PEL / group / consumer metadata sub-key. Only PEL deletions + // produce XACKDEL commands for replication. + auto args = log_data_.GetArguments(); + if (!args->empty() && (*args)[0] == "XACKDEL" && args->size() >= 3) { + uint8_t type_delimiter = 0; + if (!GetFixed8(&encoded_id, &type_delimiter)) { + return rocksdb::Status::OK(); + } + if (type_delimiter == static_cast(redis::StreamSubkeyType::StreamPelEntry)) { + uint64_t group_name_len = 0; + if (!GetFixed64(&encoded_id, &group_name_len)) { + return rocksdb::Status::OK(); + } + if (group_name_len > encoded_id.size() || encoded_id.size() - group_name_len < 16) { + return rocksdb::Status::OK(); + } + encoded_id.remove_prefix(group_name_len); + + if (!GetFixed64(&encoded_id, &entry_id.ms) || !GetFixed64(&encoded_id, &entry_id.seq)) { + return rocksdb::Status::OK(); + } + std::string entry_id_str = entry_id.ToString(); + + std::string user_key = ikey.GetKey().ToString(); + auto key_slot_id = GetSlotIdFromKey(user_key); + if (slot_range_.IsValid() && !slot_range_.Contains(key_slot_id)) { + return rocksdb::Status::OK(); + } + ns = ikey.GetNamespace().ToString(); + std::string dedup_key = ns + '\0' + user_key + '\0' + (*args)[1] + '\0' + entry_id_str; + if (seen_xackdel_entry_keys_.insert(std::move(dedup_key)).second) { + command_args = {(*args)[0], user_key, (*args)[1], (*args)[2], "IDS", "1", entry_id_str}; + resp_commands_[ns].emplace_back(redis::ArrayOfBulkStrings(command_args)); + } + } + } + return rocksdb::Status::OK(); + } + GetFixed64(&encoded_id, &entry_id.seq); - command_args = {"XDEL", ikey.GetKey().ToString(), entry_id.ToString()}; + std::string entry_id_str = entry_id.ToString(); + std::string user_key = ikey.GetKey().ToString(); + + auto key_slot_id = GetSlotIdFromKey(user_key); + if (slot_range_.IsValid() && !slot_range_.Contains(key_slot_id)) { + return rocksdb::Status::OK(); + } + ns = ikey.GetNamespace().ToString(); + + auto args = log_data_.GetArguments(); + if (!args->empty()) { + if ((*args)[0] == "XACKDEL" && args->size() >= 3) { + std::string dedup_key = ns + '\0' + user_key + '\0' + (*args)[1] + '\0' + entry_id_str; + if (seen_xackdel_entry_keys_.insert(std::move(dedup_key)).second) { + command_args = {(*args)[0], user_key, (*args)[1], (*args)[2], "IDS", "1", entry_id_str}; + } + } else { + command_args = {"XDEL", user_key, entry_id_str}; + } + } else { + command_args = {"XDEL", user_key, entry_id_str}; + } } if (!command_args.empty()) { diff --git a/src/storage/batch_extractor.h b/src/storage/batch_extractor.h index 4f5775046cf..fef55a1c08a 100644 --- a/src/storage/batch_extractor.h +++ b/src/storage/batch_extractor.h @@ -22,6 +22,7 @@ #include #include +#include #include #include "cluster/cluster_defs.h" @@ -54,4 +55,5 @@ class WriteBatchExtractor : public rocksdb::WriteBatch::Handler { bool is_slot_id_encoded_ = false; SlotRange slot_range_; bool to_redis_; + std::unordered_set seen_xackdel_entry_keys_; }; diff --git a/src/types/redis_stream.cc b/src/types/redis_stream.cc index 0c6aa8b08c1..d76a15beb14 100644 --- a/src/types/redis_stream.cc +++ b/src/types/redis_stream.cc @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -394,6 +395,425 @@ rocksdb::Status Stream::DeletePelEntries(engine::Context &ctx, const Slice &stre return storage_->Write(ctx, storage_->DefaultWriteOptions(), batch->GetWriteBatch()); } +rocksdb::Status Stream::getGroupNames(engine::Context &ctx, const std::string &ns_key, const StreamMetadata &metadata, + std::vector *group_names) { + group_names->clear(); + + std::string subkey_type_delimiter; + PutFixed64(&subkey_type_delimiter, UINT64_MAX); + PutFixed8(&subkey_type_delimiter, static_cast(StreamSubkeyType::StreamConsumerGroupMetadata)); + + std::string next_version_prefix_key = + InternalKey(ns_key, subkey_type_delimiter, metadata.version + 1, storage_->IsSlotIdEncoded()).Encode(); + std::string prefix_key = + InternalKey(ns_key, subkey_type_delimiter, metadata.version, storage_->IsSlotIdEncoded()).Encode(); + + rocksdb::ReadOptions read_options = ctx.DefaultScanOptions(); + rocksdb::Slice upper_bound(next_version_prefix_key); + read_options.iterate_upper_bound = &upper_bound; + rocksdb::Slice lower_bound(prefix_key); + read_options.iterate_lower_bound = &lower_bound; + + auto iter = util::UniqueIterator(ctx, read_options, stream_cf_handle_); + for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { + if (identifySubkeyType(iter->key()) != StreamSubkeyType::StreamConsumerGroupMetadata) { + continue; + } + group_names->push_back(groupNameFromInternalKey(iter->key())); + } + return iter->status(); +} + +rocksdb::Status Stream::deleteEntryAndUpdateMeta(rocksdb::WriteBatchBase *batch, const std::string &entry_key, + const StreamEntryID &id, StreamMetadata *metadata, + uint64_t *deleted_cnt) { + rocksdb::Status s = batch->Delete(stream_cf_handle_, entry_key); + if (!s.ok()) return s; + (*deleted_cnt)++; + + if (metadata->max_deleted_entry_id < id) { + metadata->max_deleted_entry_id = id; + } + + return rocksdb::Status::OK(); +} + +rocksdb::Status Stream::cleanPelFromAllGroups( + engine::Context &ctx, const std::string &ns_key, const StreamMetadata &metadata, const StreamEntryID &id, + rocksdb::WriteBatchBase *batch, bool *batch_modified, const std::vector &group_names, + std::map *group_pending_decrements, + std::map> *consumer_pending_decrements) { + for (const auto &group_name : group_names) { + std::string pel_key = internalPelKeyFromGroupAndEntryId(ns_key, metadata, group_name, id); + std::string pel_value; + auto pel_s = storage_->Get(ctx, ctx.GetReadOptions(), stream_cf_handle_, pel_key, &pel_value); + if (pel_s.ok()) { + rocksdb::Status s = batch->Delete(stream_cf_handle_, pel_key); + if (!s.ok()) return s; + *batch_modified = true; + + auto pel_entry = decodeStreamPelEntryValue(pel_value); + (*group_pending_decrements)[group_name]++; + (*consumer_pending_decrements)[group_name][pel_entry.consumer_name]++; + } else if (!pel_s.IsNotFound()) { + return pel_s; + } + } + return rocksdb::Status::OK(); +} + +rocksdb::Status Stream::flushPendingNumberUpdates( + engine::Context &ctx, const std::string &ns_key, const StreamMetadata &metadata, rocksdb::WriteBatchBase *batch, + const std::map &group_pending_decrements, + const std::map> &consumer_pending_decrements) { + for (const auto &[group_name, decrement] : group_pending_decrements) { + auto group_key = internalKeyFromGroupName(ns_key, metadata, group_name); + std::string group_value; + auto s = storage_->Get(ctx, ctx.GetReadOptions(), stream_cf_handle_, group_key, &group_value); + if (!s.ok() && !s.IsNotFound()) { + return s; + } + if (s.ok()) { + auto group_meta = decodeStreamConsumerGroupMetadataValue(group_value); + group_meta.pending_number -= decrement; + s = batch->Put(stream_cf_handle_, group_key, encodeStreamConsumerGroupMetadataValue(group_meta)); + if (!s.ok()) return s; + } + } + + for (const auto &[group_name, consumers] : consumer_pending_decrements) { + for (const auto &[consumer_name, decrement] : consumers) { + auto consumer_key = internalKeyFromConsumerName(ns_key, metadata, group_name, consumer_name); + std::string consumer_value; + auto s = storage_->Get(ctx, ctx.GetReadOptions(), stream_cf_handle_, consumer_key, &consumer_value); + if (!s.ok() && !s.IsNotFound()) { + return s; + } + if (s.ok()) { + auto consumer_meta = decodeStreamConsumerMetadataValue(consumer_value); + consumer_meta.pending_number -= decrement; + s = batch->Put(stream_cf_handle_, consumer_key, encodeStreamConsumerMetadataValue(consumer_meta)); + if (!s.ok()) return s; + } + } + } + + return rocksdb::Status::OK(); +} + +rocksdb::Status Stream::isAckedByAllGroups( + engine::Context &ctx, const std::string &ns_key, const StreamMetadata &metadata, const StreamEntryID &id, + const std::vector &group_names, + const std::unordered_map &last_delivered_ids_by_group, bool *all_acked) { + *all_acked = true; + for (const auto &group_name : group_names) { + // A group with last_delivered_id < id has never delivered this entry, + // so it cannot be considered acknowledged. + auto it = last_delivered_ids_by_group.find(group_name); + if (it != last_delivered_ids_by_group.end() && id > it->second) { + *all_acked = false; + return rocksdb::Status::OK(); + } + + std::string pel_key = internalPelKeyFromGroupAndEntryId(ns_key, metadata, group_name, id); + std::string pel_value; + auto pel_s = storage_->Get(ctx, ctx.GetReadOptions(), stream_cf_handle_, pel_key, &pel_value); + if (pel_s.ok()) { + *all_acked = false; + return rocksdb::Status::OK(); + } else if (!pel_s.IsNotFound()) { + return pel_s; + } + } + return rocksdb::Status::OK(); +} + +rocksdb::Status Stream::DeleteEntriesAndAck(engine::Context &ctx, const Slice &stream_name, + const std::string &group_name, const std::vector &ids, + StreamDeleteOption option, std::vector *results) { + results->assign(ids.size(), static_cast(StreamEntryDeleteResult::kEntryNotFound)); + + std::string ns_key = AppendNamespacePrefix(stream_name); + + StreamMetadata metadata(false); + rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); + if (!s.ok()) { + if (s.IsNotFound()) { + // Missing keys return per-ID not-found results, not a command error. + return rocksdb::Status::OK(); + } + return s; + } + + if (ids.empty()) { + return rocksdb::Status::OK(); + } + + std::string group_key = internalKeyFromGroupName(ns_key, metadata, group_name); + std::string get_group_value; + s = storage_->Get(ctx, ctx.GetReadOptions(), stream_cf_handle_, group_key, &get_group_value); + if (!s.ok()) { + if (s.IsNotFound()) { + return rocksdb::Status::OK(); + } + return s; + } + + auto batch = storage_->GetWriteBatchBase(); + std::string option_str; + switch (option) { + case StreamDeleteOption::DelRef: + option_str = "DELREF"; + break; + case StreamDeleteOption::Acked: + option_str = "ACKED"; + break; + default: + option_str = "KEEPREF"; + break; + } + WriteBatchLogData log_data(kRedisStream, {"XACKDEL", group_name, option_str}); + s = batch->PutLogData(log_data.Encode()); + if (!s.ok()) return s; + + std::string next_version_prefix_key = + InternalKey(ns_key, "", metadata.version + 1, storage_->IsSlotIdEncoded()).Encode(); + std::string prefix_key = InternalKey(ns_key, "", metadata.version, storage_->IsSlotIdEncoded()).Encode(); + + rocksdb::ReadOptions read_options = ctx.DefaultScanOptions(); + rocksdb::Slice upper_bound(next_version_prefix_key); + read_options.iterate_upper_bound = &upper_bound; + rocksdb::Slice lower_bound(prefix_key); + read_options.iterate_lower_bound = &lower_bound; + + auto iter = util::UniqueIterator(ctx, read_options, stream_cf_handle_); + + std::vector all_groups; + bool need_groups = (option == StreamDeleteOption::DelRef || option == StreamDeleteOption::Acked); + if (need_groups) { + s = getGroupNames(ctx, ns_key, metadata, &all_groups); + if (!s.ok()) return s; + } + + // Prefetch last-delivered IDs so ACKED can distinguish acked entries + // from entries that were never delivered. + std::unordered_map last_delivered_ids_by_group; + if (option == StreamDeleteOption::Acked) { + for (const auto &candidate_group_name : all_groups) { + std::string group_metadata_key = internalKeyFromGroupName(ns_key, metadata, candidate_group_name); + std::string group_metadata_value; + s = storage_->Get(ctx, ctx.GetReadOptions(), stream_cf_handle_, group_metadata_key, &group_metadata_value); + if (s.ok()) { + auto group_metadata = decodeStreamConsumerGroupMetadataValue(group_metadata_value); + last_delivered_ids_by_group[candidate_group_name] = group_metadata.last_delivered_id; + } else if (!s.IsNotFound()) { + return s; + } + } + } + + std::map consumer_acknowledges; + std::map other_group_pending_decrements; + std::map> other_consumer_pending_decrements; + uint64_t deleted_cnt = 0; + uint64_t acknowledged_cnt = 0; + bool batch_modified = false; + + std::unordered_set seen_entry_keys; + seen_entry_keys.reserve(ids.size()); + std::unordered_set deleted_entry_keys; + deleted_entry_keys.reserve(ids.size()); + StreamEntryID original_first_entry_id = metadata.first_entry_id; + StreamEntryID original_last_entry_id = metadata.last_entry_id; + + for (size_t i = 0; i < ids.size(); i++) { + const auto &id = ids[i]; + + std::string entry_key = internalKeyFromEntryID(ns_key, metadata, id); + + if (!seen_entry_keys.insert(entry_key).second) { + (*results)[i] = static_cast(StreamEntryDeleteResult::kEntryNotFound); + continue; + } + + // Look up the current group's PEL entry first. + std::string pel_key = internalPelKeyFromGroupAndEntryId(ns_key, metadata, group_name, id); + std::string pel_value; + s = storage_->Get(ctx, ctx.GetReadOptions(), stream_cf_handle_, pel_key, &pel_value); + if (!s.ok() && !s.IsNotFound()) { + return s; + } + if (s.IsNotFound()) { + (*results)[i] = static_cast(StreamEntryDeleteResult::kEntryNotFound); + continue; + } + + s = batch->Delete(stream_cf_handle_, pel_key); + if (!s.ok()) return s; + acknowledged_cnt++; + batch_modified = true; + + auto pel_entry = decodeStreamPelEntryValue(pel_value); + consumer_acknowledges[pel_entry.consumer_name]++; + + std::string value; + s = storage_->Get(ctx, read_options, stream_cf_handle_, entry_key, &value); + if (!s.ok() && !s.IsNotFound()) { + return s; + } + bool stream_entry_exists = s.ok(); + + std::vector other_groups; + if (need_groups) { + for (const auto &candidate_group_name : all_groups) { + if (candidate_group_name != group_name) other_groups.push_back(candidate_group_name); + } + } + + if (!stream_entry_exists) { + if (option == StreamDeleteOption::DelRef && !other_groups.empty()) { + s = cleanPelFromAllGroups(ctx, ns_key, metadata, id, batch.Get(), &batch_modified, other_groups, + &other_group_pending_decrements, &other_consumer_pending_decrements); + if (!s.ok()) return s; + } + + if (option == StreamDeleteOption::Acked) { + bool all_other_acked = true; + if (!other_groups.empty()) { + s = isAckedByAllGroups(ctx, ns_key, metadata, id, other_groups, last_delivered_ids_by_group, + &all_other_acked); + if (!s.ok()) return s; + } + (*results)[i] = static_cast(all_other_acked ? StreamEntryDeleteResult::kEntryDeleted + : StreamEntryDeleteResult::kEntrySkipped); + } else { + (*results)[i] = static_cast(StreamEntryDeleteResult::kEntryDeleted); + } + continue; + } + + if (option == StreamDeleteOption::KeepRef) { + s = deleteEntryAndUpdateMeta(batch.Get(), entry_key, id, &metadata, &deleted_cnt); + if (!s.ok()) return s; + batch_modified = true; + deleted_entry_keys.insert(entry_key); + (*results)[i] = static_cast(StreamEntryDeleteResult::kEntryDeleted); + } else if (option == StreamDeleteOption::DelRef) { + s = deleteEntryAndUpdateMeta(batch.Get(), entry_key, id, &metadata, &deleted_cnt); + if (!s.ok()) return s; + batch_modified = true; + deleted_entry_keys.insert(entry_key); + + if (!other_groups.empty()) { + s = cleanPelFromAllGroups(ctx, ns_key, metadata, id, batch.Get(), &batch_modified, other_groups, + &other_group_pending_decrements, &other_consumer_pending_decrements); + if (!s.ok()) return s; + } + (*results)[i] = static_cast(StreamEntryDeleteResult::kEntryDeleted); + } else { // StreamDeleteOption::Acked + bool all_other_acked = true; + if (!other_groups.empty()) { + s = isAckedByAllGroups(ctx, ns_key, metadata, id, other_groups, last_delivered_ids_by_group, &all_other_acked); + if (!s.ok()) return s; + } + + if (all_other_acked) { + s = deleteEntryAndUpdateMeta(batch.Get(), entry_key, id, &metadata, &deleted_cnt); + if (!s.ok()) return s; + batch_modified = true; + deleted_entry_keys.insert(entry_key); + (*results)[i] = static_cast(StreamEntryDeleteResult::kEntryDeleted); + } else { + (*results)[i] = static_cast(StreamEntryDeleteResult::kEntrySkipped); + } + } + } + + if (deleted_cnt > 0 || acknowledged_cnt > 0 || !other_group_pending_decrements.empty()) { + if (deleted_cnt > 0) { + metadata.size -= deleted_cnt; + + if (metadata.size == 0) { + metadata.first_entry_id.Clear(); + metadata.last_entry_id.Clear(); + metadata.recorded_first_entry_id.Clear(); + } else { + bool first_deleted = + deleted_entry_keys.count(internalKeyFromEntryID(ns_key, metadata, original_first_entry_id)) > 0; + bool last_deleted = + deleted_entry_keys.count(internalKeyFromEntryID(ns_key, metadata, original_last_entry_id)) > 0; + + if (first_deleted) { + iter->SeekToFirst(); + while (iter->Valid() && (identifySubkeyType(iter->key()) != StreamSubkeyType::StreamEntry || + deleted_entry_keys.count(iter->key().ToString()) > 0)) { + iter->Next(); + } + if (iter->Valid()) { + metadata.first_entry_id = entryIDFromInternalKey(iter->key()); + metadata.recorded_first_entry_id = metadata.first_entry_id; + } else { + metadata.first_entry_id.Clear(); + metadata.recorded_first_entry_id.Clear(); + } + } + if (last_deleted) { + iter->SeekToLast(); + while (iter->Valid() && (identifySubkeyType(iter->key()) != StreamSubkeyType::StreamEntry || + deleted_entry_keys.count(iter->key().ToString()) > 0)) { + iter->Prev(); + } + if (iter->Valid()) { + metadata.last_entry_id = entryIDFromInternalKey(iter->key()); + } else { + metadata.last_entry_id.Clear(); + } + } + } + + std::string bytes; + metadata.Encode(&bytes); + s = batch->Put(metadata_cf_handle_, ns_key, bytes); + if (!s.ok()) return s; + } + + if (acknowledged_cnt > 0) { + StreamConsumerGroupMetadata group_metadata = decodeStreamConsumerGroupMetadataValue(get_group_value); + group_metadata.pending_number -= acknowledged_cnt; + std::string group_value = encodeStreamConsumerGroupMetadataValue(group_metadata); + s = batch->Put(stream_cf_handle_, group_key, group_value); + if (!s.ok()) return s; + + for (const auto &[consumer_name, ack_count] : consumer_acknowledges) { + auto consumer_meta_key = internalKeyFromConsumerName(ns_key, metadata, group_name, consumer_name); + std::string consumer_meta_original; + s = storage_->Get(ctx, ctx.GetReadOptions(), stream_cf_handle_, consumer_meta_key, &consumer_meta_original); + if (!s.ok() && !s.IsNotFound()) { + return s; + } + if (s.ok()) { + auto consumer_metadata = decodeStreamConsumerMetadataValue(consumer_meta_original); + consumer_metadata.pending_number -= ack_count; + s = batch->Put(stream_cf_handle_, consumer_meta_key, encodeStreamConsumerMetadataValue(consumer_metadata)); + if (!s.ok()) return s; + } + } + } + + if (!other_group_pending_decrements.empty()) { + s = flushPendingNumberUpdates(ctx, ns_key, metadata, batch.Get(), other_group_pending_decrements, + other_consumer_pending_decrements); + if (!s.ok()) return s; + } + } + + if (!batch_modified) { + return rocksdb::Status::OK(); + } + + return storage_->Write(ctx, storage_->DefaultWriteOptions(), batch->GetWriteBatch()); +} + rocksdb::Status Stream::ClaimPelEntries(engine::Context &ctx, const Slice &stream_name, const std::string &group_name, const std::string &consumer_name, const uint64_t min_idle_time_ms, const std::vector &entry_ids, const StreamClaimOptions &options, diff --git a/src/types/redis_stream.h b/src/types/redis_stream.h index 08cae0b1d48..6a1442c7030 100644 --- a/src/types/redis_stream.h +++ b/src/types/redis_stream.h @@ -22,8 +22,10 @@ #include +#include #include #include +#include #include #include "storage/redis_db.h" @@ -53,6 +55,9 @@ class Stream : public SubKeyScanner { uint64_t *deleted_cnt); rocksdb::Status DeletePelEntries(engine::Context &ctx, const Slice &stream_name, const std::string &group_name, const std::vector &entry_ids, uint64_t *acknowledged); + rocksdb::Status DeleteEntriesAndAck(engine::Context &ctx, const Slice &stream_name, const std::string &group_name, + const std::vector &ids, StreamDeleteOption option, + std::vector *results); rocksdb::Status ClaimPelEntries(engine::Context &ctx, const Slice &stream_name, const std::string &group_name, const std::string &consumer_name, uint64_t min_idle_time_ms, const std::vector &entry_ids, const StreamClaimOptions &options, @@ -100,6 +105,8 @@ class Stream : public SubKeyScanner { static StreamConsumerGroupMetadata decodeStreamConsumerGroupMetadataValue(const std::string &value); std::string internalKeyFromConsumerName(const std::string &ns_key, const StreamMetadata &metadata, const std::string &group_name, const std::string &consumer_name) const; + rocksdb::Status getGroupNames(engine::Context &ctx, const std::string &ns_key, const StreamMetadata &metadata, + std::vector *group_names); std::string consumerNameFromInternalKey(rocksdb::Slice key) const; static std::string encodeStreamConsumerMetadataValue(const StreamConsumerMetadata &consumer_metadata); static StreamConsumerMetadata decodeStreamConsumerMetadataValue(const std::string &value); @@ -109,6 +116,22 @@ class Stream : public SubKeyScanner { std::string internalPelKeyFromGroupAndEntryId(const std::string &ns_key, const StreamMetadata &metadata, const std::string &group_name, const StreamEntryID &id); StreamEntryID groupAndEntryIdFromPelInternalKey(rocksdb::Slice key, std::string &group_name); + + rocksdb::Status deleteEntryAndUpdateMeta(rocksdb::WriteBatchBase *batch, const std::string &entry_key, + const StreamEntryID &id, StreamMetadata *metadata, uint64_t *deleted_cnt); + rocksdb::Status cleanPelFromAllGroups( + engine::Context &ctx, const std::string &ns_key, const StreamMetadata &metadata, const StreamEntryID &id, + rocksdb::WriteBatchBase *batch, bool *batch_modified, const std::vector &group_names, + std::map *group_pending_decrements, + std::map> *consumer_pending_decrements); + rocksdb::Status isAckedByAllGroups(engine::Context &ctx, const std::string &ns_key, const StreamMetadata &metadata, + const StreamEntryID &id, const std::vector &group_names, + const std::unordered_map &last_delivered_ids_by_group, + bool *all_acked); + rocksdb::Status flushPendingNumberUpdates( + engine::Context &ctx, const std::string &ns_key, const StreamMetadata &metadata, rocksdb::WriteBatchBase *batch, + const std::map &group_pending_decrements, + const std::map> &consumer_pending_decrements); static std::string encodeStreamPelEntryValue(const StreamPelEntry &pel_entry); static StreamPelEntry decodeStreamPelEntryValue(const std::string &value); StreamSubkeyType identifySubkeyType(const rocksdb::Slice &key) const; diff --git a/src/types/redis_stream_base.h b/src/types/redis_stream_base.h index f7991f573e3..1e9830249b1 100644 --- a/src/types/redis_stream_base.h +++ b/src/types/redis_stream_base.h @@ -203,6 +203,18 @@ enum class StreamSubkeyType { StreamPelEntry = 3, }; +enum class StreamDeleteOption { + KeepRef = 0, + DelRef = 1, + Acked = 2, +}; + +enum class StreamEntryDeleteResult : int { + kEntryNotFound = -1, + kEntryDeleted = 1, + kEntrySkipped = 2, +}; + struct StreamPelEntry { uint64_t last_delivery_time_ms; uint64_t last_delivery_count; diff --git a/tests/gocase/unit/server/poll_updates_test.go b/tests/gocase/unit/server/poll_updates_test.go index 4d76499f9cc..d000cd8868a 100644 --- a/tests/gocase/unit/server/poll_updates_test.go +++ b/tests/gocase/unit/server/poll_updates_test.go @@ -283,6 +283,7 @@ func TestPollUpdates_WithRESPFormat(t *testing.T) { pollUpdates = parsePollUpdatesResult(t, result.(map[any]any), true) require.Len(t, pollUpdates.Updates, 1) require.EqualValues(t, []any{RESPFormat{ + Namespace: "default", Commands: [][]string{ {"XADD", "stream", id, "field", "value"}, {"XDEL", "stream", id}, diff --git a/tests/gocase/unit/type/stream/stream_test.go b/tests/gocase/unit/type/stream/stream_test.go index 00354848ca9..d3070fb6ddc 100644 --- a/tests/gocase/unit/type/stream/stream_test.go +++ b/tests/gocase/unit/type/stream/stream_test.go @@ -2643,6 +2643,583 @@ func TestStreamOffset(t *testing.T) { require.NoError(t, rdb.Del(ctx, streamKey).Err()) }) + + t.Run("XACKDEL DELREF cross-group PEL cleanup", func(t *testing.T) { + streamName := "xackdel_delref_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + otherGroup := "otherGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + _, err = rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: otherGroup, + Consumer: "c2", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "DELREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + + require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) + + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(0), pending.Count) + + otherPending, err := rdb.XPending(ctx, streamName, otherGroup).Result() + require.NoError(t, err) + require.Equal(t, int64(0), otherPending.Count) + + infoGroups := rdb.XInfoGroups(ctx, streamName).Val() + require.Len(t, infoGroups, 2) + require.Equal(t, int64(0), infoGroups[0].Pending) + require.Equal(t, int64(0), infoGroups[1].Pending) + + consumers1 := rdb.XInfoConsumers(ctx, streamName, groupName).Val() + require.Len(t, consumers1, 1) + require.Equal(t, "c1", consumers1[0].Name) + require.Equal(t, int64(0), consumers1[0].Pending) + + consumers2 := rdb.XInfoConsumers(ctx, streamName, otherGroup).Val() + require.Len(t, consumers2, 1) + require.Equal(t, "c2", consumers2[0].Name) + require.Equal(t, int64(0), consumers2[0].Pending) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL ACKED all groups have acked deletes entry", func(t *testing.T) { + streamName := "xackdel_acked_all_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + otherGroup := "otherGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + _, err = rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: otherGroup, + Consumer: "c2", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + require.NoError(t, rdb.XAck(ctx, streamName, otherGroup, "1-0").Err()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + + require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL ACKED with other group not acked returns 2", func(t *testing.T) { + streamName := "xackdel_acked_notall_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + otherGroup := "otherGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + _, err = rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: otherGroup, + Consumer: "c2", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + // Both groups still have pending entries; XACKDEL acks current group but + // other group still references the entry, so it cannot be deleted. + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(2)}, r) + + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL non-existent key returns array of -1", func(t *testing.T) { + streamName := "xackdel_nokey_" + strconv.Itoa(rand.Int()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, "nonexistent", "KEEPREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(-1)}, r) + }) + + t.Run("XACKDEL non-existent group on existing stream returns array of -1", func(t *testing.T) { + streamName := "xackdel_nogroup_" + strconv.Itoa(rand.Int()) + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, "nonexistent", "KEEPREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(-1)}, r) + + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL DELREF duplicate IDs are idempotent", func(t *testing.T) { + streamName := "xackdel_dedup_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "DELREF", "IDS", "2", "1-0", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1), int64(-1)}, r) + + require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) + + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(0), pending.Count) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL KEEPREF deletes entry", func(t *testing.T) { + streamName := "xackdel_keepref_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + // Create a pending entry so the ID is in the group's PEL. + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + + require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL KEEPREF ID not pending in group does not delete entry", func(t *testing.T) { + streamName := "xackdel_keepref_notpending_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(-1)}, r) + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL DELREF dangling PEL after stream entry deleted by XDEL", func(t *testing.T) { + streamName := "xackdel_delref_dangling_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + otherGroup := "otherGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + + // Both groups read the entry so both have PEL entries. + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + _, err = rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: otherGroup, + Consumer: "c2", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + // Delete the stream entry directly, leaving dangling PEL entries. + require.NoError(t, rdb.XDel(ctx, streamName, "1-0").Err()) + + // XACKDEL DELREF from the first group should clean up all dangling PEL entries. + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "DELREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(0), pending.Count) + + otherPending, err := rdb.XPending(ctx, streamName, otherGroup).Result() + require.NoError(t, err) + require.Equal(t, int64(0), otherPending.Count) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL KEEPREF acks current group PEL", func(t *testing.T) { + streamName := "xackdel_keepref_ack_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + // Create a pending entry by reading. + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + + require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) + + // KEEPREF must still ACK the current group's pending entry. + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(0), pending.Count) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL KEEPREF non-existent entry returns -1", func(t *testing.T) { + streamName := "xackdel_keepref_notfound_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "999-999").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(-1)}, r) + + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL KEEPREF delete first entry recalculates boundary", func(t *testing.T) { + streamName := "xackdel_boundary_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, ID: "1-0", Values: []string{"f", "v"}, + }).Err()) + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, ID: "2-0", Values: []string{"f", "v"}, + }).Err()) + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, ID: "3-0", Values: []string{"f", "v"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + // Create a pending entry so the ID is in the group's PEL. + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 3, + }).Result() + require.NoError(t, err) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + require.Equal(t, int64(2), rdb.XLen(ctx, streamName).Val()) + + msgs, err := rdb.XRange(ctx, streamName, "-", "+").Result() + require.NoError(t, err) + require.Equal(t, "2-0", msgs[0].ID) + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL DELREF single group PEL cleanup", func(t *testing.T) { + streamName := "xackdel_delref_single_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "DELREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) + + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(0), pending.Count) + + consumers := rdb.XInfoConsumers(ctx, streamName, groupName).Val() + require.Len(t, consumers, 1) + require.Equal(t, "c1", consumers[0].Name) + require.Equal(t, int64(0), consumers[0].Pending) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL ACKED already acked in current group returns -1", func(t *testing.T) { + streamName := "xackdel_acked_single_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + // Ack the entry first; the PEL entry for the current group is now gone. + require.NoError(t, rdb.XAck(ctx, streamName, groupName, "1-0").Err()) + + // XACKDEL must find the ID in the current group's PEL; since it is already + // acked, it returns -1 and does not delete the stream entry. + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(-1)}, r) + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL ACKED single group pending acknowledges and deletes entry", func(t *testing.T) { + streamName := "xackdel_acked_pending_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + pendingBefore, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(1), pendingBefore.Count) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) + + pendingAfter, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(0), pendingAfter.Count) + + consumers := rdb.XInfoConsumers(ctx, streamName, groupName).Val() + require.Len(t, consumers, 1) + require.Equal(t, int64(0), consumers[0].Pending) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL with invalid option returns error", func(t *testing.T) { + streamName := "xackdel_badopt_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "INVALID", "IDS", "1", "1-0").Result() + require.Error(t, err) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL missing IDS returns error", func(t *testing.T) { + streamName := "xackdel_noids_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF").Result() + require.Error(t, err) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL with non-integer numids returns error", func(t *testing.T) { + streamName := "xackdel_badnumids_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "abc").Result() + require.Error(t, err) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL with zero numids returns error", func(t *testing.T) { + streamName := "xackdel_zeronumids_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "0").Result() + require.Error(t, err) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL with negative numids returns error", func(t *testing.T) { + streamName := "xackdel_negnumids_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "-1").Result() + require.Error(t, err) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("XACKDEL with invalid entry ID returns error", func(t *testing.T) { + streamName := "xackdel_badid_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, + }).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + + _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "bad-id").Result() + require.Error(t, err) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) } func parseStreamEntryID(id string) (ts int64, seqNum int64) { From f55dbb378ca7024ca242dc4565857cdcf7e7af0f Mon Sep 17 00:00:00 2001 From: kirito632 Date: Thu, 11 Jun 2026 21:39:32 +0800 Subject: [PATCH 3/4] fix(stream): harden XACKDEL parsing and replication --- src/cluster/batch_sender.cc | 4 + src/cluster/batch_sender.h | 5 +- src/commands/cmd_stream.cc | 102 +++- src/storage/batch_extractor.cc | 25 +- src/storage/batch_extractor.h | 3 +- src/types/redis_stream.cc | 40 +- src/types/redis_stream_base.h | 2 + tests/cppunit/writebatch_indexer_test.cc | 135 ++++++ tests/gocase/unit/server/poll_updates_test.go | 183 ++++++- tests/gocase/unit/type/stream/stream_test.go | 450 ++++++++++++++---- 10 files changed, 819 insertions(+), 130 deletions(-) diff --git a/src/cluster/batch_sender.cc b/src/cluster/batch_sender.cc index e92221ee6f3..7ed64370f2c 100644 --- a/src/cluster/batch_sender.cc +++ b/src/cluster/batch_sender.cc @@ -38,6 +38,7 @@ Status BatchSender::Put(rocksdb::ColumnFamilyHandle *cf, const rocksdb::Slice &k return {Status::NotOK, fmt::format("failed to put key value to migration batch, {}", s.ToString())}; } + pending_logdata_only_ = false; pending_entries_++; entries_num_++; return Status::OK(); @@ -48,6 +49,7 @@ Status BatchSender::Delete(rocksdb::ColumnFamilyHandle *cf, const rocksdb::Slice if (!s.ok()) { return {Status::NotOK, fmt::format("failed to delete key from migration batch, {}", s.ToString())}; } + pending_logdata_only_ = false; pending_entries_++; entries_num_++; return Status::OK(); @@ -58,6 +60,7 @@ Status BatchSender::PutLogData(const rocksdb::Slice &blob) { if (!s.ok()) { return {Status::NotOK, fmt::format("failed to put log data to migration batch, {}", s.ToString())}; } + pending_logdata_only_ = true; pending_entries_++; entries_num_++; return Status::OK(); @@ -89,6 +92,7 @@ Status BatchSender::Send() { sent_bytes_ += write_batch_.GetDataSize(); sent_batches_num_++; pending_entries_ = 0; + pending_logdata_only_ = false; write_batch_.Clear(); return Status::OK(); } diff --git a/src/cluster/batch_sender.h b/src/cluster/batch_sender.h index 07c4e9fdbbe..338af8a2bf9 100644 --- a/src/cluster/batch_sender.h +++ b/src/cluster/batch_sender.h @@ -46,7 +46,9 @@ class BatchSender { void SetMaxBytes(size_t max_bytes) { if (max_bytes_ != max_bytes) max_bytes_ = max_bytes; } - bool IsFull() const { return write_batch_.GetDataSize() >= max_bytes_; } + // A log-data-only batch must wait for the first data record; + // otherwise the receiver would see metadata without matching writes. + bool IsFull() const { return !pending_logdata_only_ && write_batch_.GetDataSize() >= max_bytes_; } uint64_t GetSentBytes() const { return sent_bytes_; } uint32_t GetSentBatchesNum() const { return sent_batches_num_; } uint32_t GetEntriesNum() const { return entries_num_; } @@ -62,6 +64,7 @@ class BatchSender { uint32_t sent_batches_num_ = 0; uint32_t entries_num_ = 0; uint32_t pending_entries_ = 0; + bool pending_logdata_only_ = false; int dst_fd_; size_t max_bytes_; diff --git a/src/commands/cmd_stream.cc b/src/commands/cmd_stream.cc index 9b480b469a6..ee8756d562d 100644 --- a/src/commands/cmd_stream.cc +++ b/src/commands/cmd_stream.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include "command_parser.h" #include "commander.h" @@ -49,6 +50,48 @@ CommandKeyRange ParseStreamReadRange(const std::vector &args, uint3 range.last_key = range.first_key + stream_size - 1; return range; } + +// Redis accepts only canonical positive decimal numids here; +// reject forms like +1 or 01 before integer parsing. +bool IsXAckDelNumIDs(std::string_view input) { + if (input.empty() || input[0] < '1' || input[0] > '9') return false; + + return std::all_of(input.begin() + 1, input.end(), [](char c) { return c >= '0' && c <= '9'; }); +} + +StatusOr ParseXAckDelStreamEntryIDComponent(std::string_view input, bool allow_negative_zero) { + if (input.empty()) return {Status::RedisParseErr, redis::kErrInvalidEntryIdSpecified}; + + if (input[0] == '+') { + input.remove_prefix(1); + } else if (input[0] == '-') { + if (!allow_negative_zero) return {Status::RedisParseErr, redis::kErrInvalidEntryIdSpecified}; + input.remove_prefix(1); + if (input.empty() || !std::all_of(input.begin(), input.end(), [](char c) { return c == '0'; })) { + return {Status::RedisParseErr, redis::kErrInvalidEntryIdSpecified}; + } + return 0; + } + + auto parsed = ParseInt(input, 10); + if (!parsed) return {Status::RedisParseErr, redis::kErrInvalidEntryIdSpecified}; + return *parsed; +} + +Status ParseXAckDelStreamEntryID(const std::string &input, redis::StreamEntryID *id) { + auto pos = input.find('-'); + if (pos != std::string::npos) { + auto ms = GET_OR_RET(ParseXAckDelStreamEntryIDComponent(std::string_view(input).substr(0, pos), false)); + auto seq = GET_OR_RET(ParseXAckDelStreamEntryIDComponent(std::string_view(input).substr(pos + 1), true)); + id->ms = ms; + id->seq = seq; + } else { + auto ms = GET_OR_RET(ParseXAckDelStreamEntryIDComponent(input, false)); + id->ms = ms; + id->seq = 0; + } + return Status::OK(); +} } // namespace void AddStreamEntriesToResponse(std::string *output, const std::vector &entries) { @@ -276,44 +319,57 @@ class CommandXAckDel : public Commander { group_name_ = GET_OR_RET(parser.TakeStr()); option_ = redis::StreamDeleteOption::KeepRef; + bool has_option = false; + bool has_ids = false; - while (parser.Good() && !util::EqualICase(parser.RawPeek(), "IDS")) { + while (parser.Good()) { if (parser.EatEqICase("KEEPREF")) { + if (has_option) return parser.InvalidSyntax(); option_ = redis::StreamDeleteOption::KeepRef; + has_option = true; } else if (parser.EatEqICase("DELREF")) { + if (has_option) return parser.InvalidSyntax(); option_ = redis::StreamDeleteOption::DelRef; + has_option = true; } else if (parser.EatEqICase("ACKED")) { + if (has_option) return parser.InvalidSyntax(); option_ = redis::StreamDeleteOption::Acked; + has_option = true; + } else if (parser.EatEqICase("IDS")) { + has_ids = true; + + if (!parser.Good() || !IsXAckDelNumIDs(parser.RawPeek())) { + return {Status::RedisParseErr, "Number of IDs must be a positive integer"}; + } + auto numids_result = parser.TakeInt(); + if (!numids_result.IsOK()) { + return {Status::RedisParseErr, "Number of IDs must be a positive integer"}; + } + int64_t numids = numids_result.GetValue(); + + if (parser.Remains() < static_cast(numids)) { + return {Status::RedisParseErr, "The `numids` parameter must match the number of arguments"}; + } + + std::vector entry_ids; + entry_ids.reserve(static_cast(numids)); + for (int64_t i = 0; i < numids; i++) { + auto id_str = GET_OR_RET(parser.TakeStr()); + redis::StreamEntryID id; + auto s = ParseXAckDelStreamEntryID(id_str, &id); + if (!s.IsOK()) return s; + entry_ids.emplace_back(id); + } + entry_ids_ = std::move(entry_ids); } else { return parser.InvalidSyntax(); } } - if (!parser.EatEqICase("IDS")) { + if (!has_ids) { return {Status::RedisParseErr, "syntax error, expected IDS keyword"}; } - auto numids_result = parser.TakeInt(); - if (!numids_result.IsOK()) { - return {Status::RedisParseErr, errValueNotInteger}; - } - int64_t numids = numids_result.GetValue(); - if (numids <= 0) { - return {Status::RedisParseErr, "numids must be positive"}; - } - - for (int64_t i = 0; i < numids; i++) { - auto id_str = GET_OR_RET(parser.TakeStr()); - redis::StreamEntryID id; - auto s = ParseStreamEntryID(id_str, &id); - if (!s.IsOK()) return s; - entry_ids_.emplace_back(id); - } - - if (parser.Good()) { - return {Status::RedisParseErr, "syntax error, unexpected trailing arguments after IDs"}; - } - return Status::OK(); } diff --git a/src/storage/batch_extractor.cc b/src/storage/batch_extractor.cc index 670fb2bb7bc..c7e17c6c905 100644 --- a/src/storage/batch_extractor.cc +++ b/src/storage/batch_extractor.cc @@ -37,10 +37,13 @@ void WriteBatchExtractor::LogData(const rocksdb::Slice &blob) { } } else { // Redis type log data + // Reset state first so malformed log data cannot reuse XACKDEL dedup keys. + log_data_ = redis::WriteBatchLogData(); + first_seen_ = true; + seen_xackdel_xack_keys_.clear(); + seen_xackdel_xdel_keys_.clear(); if (auto s = log_data_.Decode(blob); !s.IsOK()) { WARN("Failed to decode Redis type log: {}", s.Msg()); - } else { - seen_xackdel_entry_keys_.clear(); } } } @@ -417,10 +420,11 @@ rocksdb::Status WriteBatchExtractor::DeleteCF(uint32_t column_family_id, const S GetFixed64(&encoded_id, &entry_id.ms); if (entry_id.ms == UINT64_MAX) { - // PEL / group / consumer metadata sub-key. Only PEL deletions - // produce XACKDEL commands for replication. + // XACKDEL physical replay emits XACK for each PEL deletion, + // using the actual group encoded in the PEL key. auto args = log_data_.GetArguments(); if (!args->empty() && (*args)[0] == "XACKDEL" && args->size() >= 3) { + // Skip malformed internal stream keys instead of emitting partial replay commands. uint8_t type_delimiter = 0; if (!GetFixed8(&encoded_id, &type_delimiter)) { return rocksdb::Status::OK(); @@ -433,6 +437,7 @@ rocksdb::Status WriteBatchExtractor::DeleteCF(uint32_t column_family_id, const S if (group_name_len > encoded_id.size() || encoded_id.size() - group_name_len < 16) { return rocksdb::Status::OK(); } + std::string group_name = encoded_id.ToString().substr(0, group_name_len); encoded_id.remove_prefix(group_name_len); if (!GetFixed64(&encoded_id, &entry_id.ms) || !GetFixed64(&encoded_id, &entry_id.seq)) { @@ -446,9 +451,9 @@ rocksdb::Status WriteBatchExtractor::DeleteCF(uint32_t column_family_id, const S return rocksdb::Status::OK(); } ns = ikey.GetNamespace().ToString(); - std::string dedup_key = ns + '\0' + user_key + '\0' + (*args)[1] + '\0' + entry_id_str; - if (seen_xackdel_entry_keys_.insert(std::move(dedup_key)).second) { - command_args = {(*args)[0], user_key, (*args)[1], (*args)[2], "IDS", "1", entry_id_str}; + std::string dedup_key = ns + '\0' + user_key + '\0' + group_name + '\0' + entry_id_str; + if (seen_xackdel_xack_keys_.insert(std::move(dedup_key)).second) { + command_args = {"XACK", user_key, group_name, entry_id_str}; resp_commands_[ns].emplace_back(redis::ArrayOfBulkStrings(command_args)); } } @@ -469,9 +474,9 @@ rocksdb::Status WriteBatchExtractor::DeleteCF(uint32_t column_family_id, const S auto args = log_data_.GetArguments(); if (!args->empty()) { if ((*args)[0] == "XACKDEL" && args->size() >= 3) { - std::string dedup_key = ns + '\0' + user_key + '\0' + (*args)[1] + '\0' + entry_id_str; - if (seen_xackdel_entry_keys_.insert(std::move(dedup_key)).second) { - command_args = {(*args)[0], user_key, (*args)[1], (*args)[2], "IDS", "1", entry_id_str}; + std::string dedup_key = ns + '\0' + user_key + '\0' + entry_id_str; + if (seen_xackdel_xdel_keys_.insert(std::move(dedup_key)).second) { + command_args = {"XDEL", user_key, entry_id_str}; } } else { command_args = {"XDEL", user_key, entry_id_str}; diff --git a/src/storage/batch_extractor.h b/src/storage/batch_extractor.h index fef55a1c08a..763907617da 100644 --- a/src/storage/batch_extractor.h +++ b/src/storage/batch_extractor.h @@ -55,5 +55,6 @@ class WriteBatchExtractor : public rocksdb::WriteBatch::Handler { bool is_slot_id_encoded_ = false; SlotRange slot_range_; bool to_redis_; - std::unordered_set seen_xackdel_entry_keys_; + std::unordered_set seen_xackdel_xack_keys_; + std::unordered_set seen_xackdel_xdel_keys_; }; diff --git a/src/types/redis_stream.cc b/src/types/redis_stream.cc index d76a15beb14..a0fedfec7df 100644 --- a/src/types/redis_stream.cc +++ b/src/types/redis_stream.cc @@ -28,6 +28,7 @@ #include #include "db_util.h" +#include "scope_exit.h" #include "string_util.h" #include "time_util.h" @@ -466,6 +467,7 @@ rocksdb::Status Stream::flushPendingNumberUpdates( engine::Context &ctx, const std::string &ns_key, const StreamMetadata &metadata, rocksdb::WriteBatchBase *batch, const std::map &group_pending_decrements, const std::map> &consumer_pending_decrements) { + // Saturate pending counters to tolerate stale metadata without uint64_t underflow. for (const auto &[group_name, decrement] : group_pending_decrements) { auto group_key = internalKeyFromGroupName(ns_key, metadata, group_name); std::string group_value; @@ -475,7 +477,7 @@ rocksdb::Status Stream::flushPendingNumberUpdates( } if (s.ok()) { auto group_meta = decodeStreamConsumerGroupMetadataValue(group_value); - group_meta.pending_number -= decrement; + group_meta.pending_number = group_meta.pending_number > decrement ? group_meta.pending_number - decrement : 0; s = batch->Put(stream_cf_handle_, group_key, encodeStreamConsumerGroupMetadataValue(group_meta)); if (!s.ok()) return s; } @@ -491,7 +493,8 @@ rocksdb::Status Stream::flushPendingNumberUpdates( } if (s.ok()) { auto consumer_meta = decodeStreamConsumerMetadataValue(consumer_value); - consumer_meta.pending_number -= decrement; + consumer_meta.pending_number = + consumer_meta.pending_number > decrement ? consumer_meta.pending_number - decrement : 0; s = batch->Put(stream_cf_handle_, consumer_key, encodeStreamConsumerMetadataValue(consumer_meta)); if (!s.ok()) return s; } @@ -560,6 +563,11 @@ rocksdb::Status Stream::DeleteEntriesAndAck(engine::Context &ctx, const Slice &s } auto batch = storage_->GetWriteBatchBase(); + // PutLogData is added before knowing whether any ID mutates data; + // roll it back on no-op or error to avoid phantom replay. + batch->SetSavePoint(); + auto rollback_batch = MakeScopeExit([&batch] { batch->RollbackToSavePoint().PermitUncheckedError(); }); + std::string option_str; switch (option) { case StreamDeleteOption::DelRef: @@ -626,11 +634,21 @@ rocksdb::Status Stream::DeleteEntriesAndAck(engine::Context &ctx, const Slice &s StreamEntryID original_first_entry_id = metadata.first_entry_id; StreamEntryID original_last_entry_id = metadata.last_entry_id; + std::vector other_groups; + if (need_groups) { + other_groups.reserve(all_groups.empty() ? 0 : all_groups.size() - 1); + for (const auto &candidate_group_name : all_groups) { + if (candidate_group_name != group_name) other_groups.push_back(candidate_group_name); + } + } + for (size_t i = 0; i < ids.size(); i++) { const auto &id = ids[i]; std::string entry_key = internalKeyFromEntryID(ns_key, metadata, id); + // Each ID can mutate the stream at most once per command; + // duplicates report not-found after the first attempt. if (!seen_entry_keys.insert(entry_key).second) { (*results)[i] = static_cast(StreamEntryDeleteResult::kEntryNotFound); continue; @@ -663,13 +681,6 @@ rocksdb::Status Stream::DeleteEntriesAndAck(engine::Context &ctx, const Slice &s } bool stream_entry_exists = s.ok(); - std::vector other_groups; - if (need_groups) { - for (const auto &candidate_group_name : all_groups) { - if (candidate_group_name != group_name) other_groups.push_back(candidate_group_name); - } - } - if (!stream_entry_exists) { if (option == StreamDeleteOption::DelRef && !other_groups.empty()) { s = cleanPelFromAllGroups(ctx, ns_key, metadata, id, batch.Get(), &batch_modified, other_groups, @@ -777,9 +788,11 @@ rocksdb::Status Stream::DeleteEntriesAndAck(engine::Context &ctx, const Slice &s if (!s.ok()) return s; } + // Saturate pending counters to tolerate stale metadata without uint64_t underflow. if (acknowledged_cnt > 0) { StreamConsumerGroupMetadata group_metadata = decodeStreamConsumerGroupMetadataValue(get_group_value); - group_metadata.pending_number -= acknowledged_cnt; + group_metadata.pending_number = + group_metadata.pending_number > acknowledged_cnt ? group_metadata.pending_number - acknowledged_cnt : 0; std::string group_value = encodeStreamConsumerGroupMetadataValue(group_metadata); s = batch->Put(stream_cf_handle_, group_key, group_value); if (!s.ok()) return s; @@ -793,7 +806,8 @@ rocksdb::Status Stream::DeleteEntriesAndAck(engine::Context &ctx, const Slice &s } if (s.ok()) { auto consumer_metadata = decodeStreamConsumerMetadataValue(consumer_meta_original); - consumer_metadata.pending_number -= ack_count; + consumer_metadata.pending_number = + consumer_metadata.pending_number > ack_count ? consumer_metadata.pending_number - ack_count : 0; s = batch->Put(stream_cf_handle_, consumer_meta_key, encodeStreamConsumerMetadataValue(consumer_metadata)); if (!s.ok()) return s; } @@ -811,6 +825,10 @@ rocksdb::Status Stream::DeleteEntriesAndAck(engine::Context &ctx, const Slice &s return rocksdb::Status::OK(); } + s = batch->PopSavePoint(); + if (!s.ok()) return s; + rollback_batch.Disable(); + return storage_->Write(ctx, storage_->DefaultWriteOptions(), batch->GetWriteBatch()); } diff --git a/src/types/redis_stream_base.h b/src/types/redis_stream_base.h index 1e9830249b1..5a323eef6ce 100644 --- a/src/types/redis_stream_base.h +++ b/src/types/redis_stream_base.h @@ -32,6 +32,8 @@ namespace redis { +extern const char *kErrInvalidEntryIdSpecified; + struct StreamEntryID { uint64_t ms = 0; uint64_t seq = 0; diff --git a/tests/cppunit/writebatch_indexer_test.cc b/tests/cppunit/writebatch_indexer_test.cc index 51a5d4203ea..1539edcb794 100644 --- a/tests/cppunit/writebatch_indexer_test.cc +++ b/tests/cppunit/writebatch_indexer_test.cc @@ -20,10 +20,13 @@ #include #include +#include #include +#include "storage/batch_extractor.h" #include "storage/batch_indexer.h" #include "test_base.h" +#include "types/redis_stream.h" class WriteBatchIndexerTest : public TestBase { protected: @@ -104,3 +107,135 @@ TEST_F(WriteBatchIndexerTest, SingleDelete) { s = ctx_->batch->GetFromBatchAndDB(storage_->GetDB(), rocksdb::ReadOptions(), "key", &value); EXPECT_TRUE(s.IsNotFound()); } + +namespace { + +std::unique_ptr GetOnlyWalBatchAfter(engine::Storage *storage, rocksdb::SequenceNumber seq) { + std::unique_ptr iter; + auto s = storage->GetWALIter(seq + 1, &iter); + if (!s.IsOK() || !iter || !iter->Valid()) { + return nullptr; + } + + auto batch = iter->GetBatch(); + auto copy = std::make_unique(batch.writeBatchPtr->Data()); + + iter->Next(); + EXPECT_FALSE(iter->Valid()); + return copy; +} + +struct DeleteOp { + uint32_t column_family_id = 0; + std::string key; +}; + +class XAckDelBatchCollector : public rocksdb::WriteBatch::Handler { + public: + rocksdb::Status PutCF(uint32_t, const rocksdb::Slice &, const rocksdb::Slice &) override { + return rocksdb::Status::OK(); + } + + void LogData(const rocksdb::Slice &blob) override { + redis::WriteBatchLogData log_data; + if (log_data.Decode(blob).IsOK()) { + auto *args = log_data.GetArguments(); + if (!args->empty() && (*args)[0] == "XACKDEL") { + log_data_ = blob.ToString(); + } + } + } + + rocksdb::Status DeleteCF(uint32_t column_family_id, const rocksdb::Slice &key) override { + deletes_.push_back({column_family_id, key.ToString()}); + return rocksdb::Status::OK(); + } + + const std::string &LogDataBlob() const { return log_data_; } + const std::vector &Deletes() const { return deletes_; } + + private: + std::string log_data_; + std::vector deletes_; +}; + +void AddPendingStreamEntry(redis::Stream *stream, engine::Context *ctx, const std::string &stream_name, + const std::string &group_name) { + redis::StreamAddOptions add_options; + add_options.next_id_strategy = *redis::ParseNextStreamEntryIDStrategy("1-0"); + redis::StreamEntryID id; + auto s = stream->Add(*ctx, stream_name, add_options, {"field", "value"}, &id); + ASSERT_TRUE(s.ok()) << s.ToString(); + + redis::StreamXGroupCreateOptions create_options = {false, 0, "0-0"}; + s = stream->CreateGroup(*ctx, stream_name, create_options, group_name); + ASSERT_TRUE(s.ok()) << s.ToString(); + + redis::StreamRangeOptions range_options; + range_options.start = redis::StreamEntryID::Minimum(); + range_options.end = redis::StreamEntryID::Maximum(); + range_options.count = 1; + range_options.with_count = true; + range_options.exclude_start = true; + std::vector entries; + std::string pending_group_name = group_name; + std::string consumer_name = "c1"; + s = stream->RangeWithPending(*ctx, stream_name, range_options, &entries, pending_group_name, consumer_name, false, + true); + ASSERT_TRUE(s.ok()) << s.ToString(); + ASSERT_EQ(entries.size(), 1); +} + +} // namespace + +TEST_F(WriteBatchIndexerTest, MalformedLogDataResetsXAckDelExtractorState) { + const std::string stream_name = "stream"; + const std::string group_name = "group"; + redis::Stream stream(storage_.get(), "test_ns"); + + AddPendingStreamEntry(&stream, ctx_.get(), stream_name, group_name); + auto start_seq = storage_->GetDB()->GetLatestSequenceNumber(); + std::vector results; + auto s = stream.DeleteEntriesAndAck(*ctx_, stream_name, group_name, {redis::StreamEntryID{1, 0}}, + redis::StreamDeleteOption::KeepRef, &results); + ASSERT_TRUE(s.ok()) << s.ToString(); + ASSERT_EQ(results, std::vector({1})); + auto first_batch = GetOnlyWalBatchAfter(storage_.get(), start_seq); + ASSERT_NE(first_batch, nullptr); + + XAckDelBatchCollector collector; + auto collect_status = first_batch->Iterate(&collector); + ASSERT_TRUE(collect_status.ok()) << collect_status.ToString(); + ASSERT_FALSE(collector.LogDataBlob().empty()); + ASSERT_GE(collector.Deletes().size(), 2); + + rocksdb::WriteBatch combined; + combined.PutLogData(collector.LogDataBlob()); + for (const auto &op : collector.Deletes()) { + auto status = combined.Delete(storage_->GetCFHandle(static_cast(op.column_family_id)), op.key); + ASSERT_TRUE(status.ok()) << status.ToString(); + } + combined.PutLogData("malformed-log-data"); + for (const auto &op : collector.Deletes()) { + auto status = combined.Delete(storage_->GetCFHandle(static_cast(op.column_family_id)), op.key); + ASSERT_TRUE(status.ok()) << status.ToString(); + } + + WriteBatchExtractor extractor(storage_->IsSlotIdEncoded()); + auto iterate_status = combined.Iterate(&extractor); + ASSERT_TRUE(iterate_status.ok()) << iterate_status.ToString(); + + auto *resp_commands = extractor.GetRESPCommands(); + auto it = resp_commands->find("test_ns"); + ASSERT_NE(it, resp_commands->end()); + + int xack_count = 0; + int xdel_count = 0; + for (const auto &encoded : it->second) { + if (encoded.find("$4\r\nXACK\r\n") != std::string::npos) xack_count++; + if (encoded.find("$4\r\nXDEL\r\n") != std::string::npos) xdel_count++; + } + + EXPECT_EQ(xack_count, 1); + EXPECT_EQ(xdel_count, 2); +} diff --git a/tests/gocase/unit/server/poll_updates_test.go b/tests/gocase/unit/server/poll_updates_test.go index d000cd8868a..b46442865c4 100644 --- a/tests/gocase/unit/server/poll_updates_test.go +++ b/tests/gocase/unit/server/poll_updates_test.go @@ -72,6 +72,53 @@ func parseRESPCommands(t *testing.T, values []any) []any { return updates } +func collectRESPCommands(updates []any, name string) [][]string { + commands := make([][]string, 0) + for _, update := range updates { + resp, ok := update.(RESPFormat) + if !ok { + continue + } + for _, command := range resp.Commands { + if len(command) > 0 && command[0] == name { + commands = append(commands, command) + } + } + } + return commands +} + +func applyRESPCommands(t *testing.T, rdb *redis.Client, updates []any, names ...string) { + t.Helper() + // Simulate a downstream instance applying POLLUPDATES RESP commands. + // The optional names filter keeps each case focused on expected effects. + allowed := make(map[string]struct{}, len(names)) + for _, name := range names { + allowed[name] = struct{}{} + } + for _, update := range updates { + resp, ok := update.(RESPFormat) + if !ok { + continue + } + for _, command := range resp.Commands { + if len(command) == 0 { + continue + } + if len(allowed) > 0 { + if _, ok := allowed[command[0]]; !ok { + continue + } + } + args := make([]interface{}, 0, len(command)) + for _, token := range command { + args = append(args, token) + } + require.NoError(t, rdb.Do(context.Background(), args...).Err()) + } + } +} + func parsePollUpdatesResult(t *testing.T, m map[any]any, isRESP bool) *PollUpdatesResult { itemCount := 3 require.Len(t, m, itemCount) @@ -115,7 +162,6 @@ func TestPollUpdates_Basic(t *testing.T) { defer srv0.Close() rdb0 := srv0.NewClient() defer func() { require.NoError(t, rdb0.Close()) }() - srv1 := util.StartServer(t, map[string]string{}) defer srv1.Close() rdb1 := srv1.NewClient() @@ -175,6 +221,12 @@ func TestPollUpdates_WithRESPFormat(t *testing.T) { rdb0 := srv0.NewClient() defer func() { require.NoError(t, rdb0.Close()) }() + // rdb1 simulates a downstream instance replaying RESP commands from rdb0. + srv1 := util.StartServer(t, map[string]string{}) + defer srv1.Close() + rdb1 := srv1.NewClient() + defer func() { require.NoError(t, rdb1.Close()) }() + var pollUpdates *PollUpdatesResult t.Run("String type", func(t *testing.T) { require.NoError(t, rdb0.Set(ctx, "k0", "v0", 0).Err()) @@ -291,6 +343,135 @@ func TestPollUpdates_WithRESPFormat(t *testing.T) { }, pollUpdates.Updates) }) + t.Run("Stream XACKDEL KEEPREF replay", func(t *testing.T) { + streamName := "poll_xackdel_keepref" + groupName := "myGroup" + require.NoError(t, rdb0.XAdd(ctx, &redis.XAddArgs{Stream: streamName, ID: "1-0", Values: []string{"field", "value"}}).Err()) + require.NoError(t, rdb0.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + _, err := rdb0.XReadGroup(ctx, &redis.XReadGroupArgs{Group: groupName, Consumer: "c1", Streams: []string{streamName, ">"}, Count: 1}).Result() + require.NoError(t, err) + r, err := rdb0.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + + result, err := rdb0.Do(ctx, "POLLUPDATES", pollUpdates.NextSeq, "MAX", 20, "FORMAT", "RESP").Result() + require.NoError(t, err) + pollUpdates = parsePollUpdatesResult(t, result.(map[any]any), true) + xackCommands := collectRESPCommands(pollUpdates.Updates, "XACK") + require.Equal(t, [][]string{{"XACK", streamName, groupName, "1-0"}}, xackCommands) + xdelCommands := collectRESPCommands(pollUpdates.Updates, "XDEL") + require.Equal(t, [][]string{{"XDEL", streamName, "1-0"}}, xdelCommands) + require.Empty(t, collectRESPCommands(pollUpdates.Updates, "XACKDEL")) + + require.NoError(t, rdb1.FlushDB(ctx).Err()) + require.NoError(t, rdb1.XAdd(ctx, &redis.XAddArgs{Stream: streamName, ID: "1-0", Values: []string{"field", "value"}}).Err()) + require.NoError(t, rdb1.XGroupCreateMkStream(ctx, streamName, groupName, "$").Err()) + applyRESPCommands(t, rdb1, pollUpdates.Updates, "XACK", "XDEL") + require.Equal(t, int64(0), rdb1.XLen(ctx, streamName).Val()) + }) + + t.Run("Stream XACKDEL ACKED replay", func(t *testing.T) { + streamName := "poll_xackdel_acked" + groupName := "myGroup" + require.NoError(t, rdb0.XAdd(ctx, &redis.XAddArgs{Stream: streamName, ID: "1-0", Values: []string{"field", "value"}}).Err()) + require.NoError(t, rdb0.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + _, err := rdb0.XReadGroup(ctx, &redis.XReadGroupArgs{Group: groupName, Consumer: "c1", Streams: []string{streamName, ">"}, Count: 1}).Result() + require.NoError(t, err) + r, err := rdb0.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + + result, err := rdb0.Do(ctx, "POLLUPDATES", pollUpdates.NextSeq, "MAX", 20, "FORMAT", "RESP").Result() + require.NoError(t, err) + pollUpdates = parsePollUpdatesResult(t, result.(map[any]any), true) + xackCommands := collectRESPCommands(pollUpdates.Updates, "XACK") + require.Equal(t, [][]string{{"XACK", streamName, groupName, "1-0"}}, xackCommands) + xdelCommands := collectRESPCommands(pollUpdates.Updates, "XDEL") + require.Equal(t, [][]string{{"XDEL", streamName, "1-0"}}, xdelCommands) + require.Empty(t, collectRESPCommands(pollUpdates.Updates, "XACKDEL")) + + require.NoError(t, rdb1.FlushDB(ctx).Err()) + require.NoError(t, rdb1.XAdd(ctx, &redis.XAddArgs{Stream: streamName, ID: "1-0", Values: []string{"field", "value"}}).Err()) + require.NoError(t, rdb1.XGroupCreateMkStream(ctx, streamName, groupName, "$").Err()) + applyRESPCommands(t, rdb1, pollUpdates.Updates, "XACK", "XDEL") + require.Equal(t, int64(0), rdb1.XLen(ctx, streamName).Val()) + }) + + t.Run("Stream XACKDEL DELREF replay deduplicates group PEL cleanup", func(t *testing.T) { + streamName := "poll_xackdel_delref" + groupName := "myGroup" + otherGroup := "otherGroup" + require.NoError(t, rdb0.XAdd(ctx, &redis.XAddArgs{Stream: streamName, ID: "1-0", Values: []string{"field", "value"}}).Err()) + require.NoError(t, rdb0.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + require.NoError(t, rdb0.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + _, err := rdb0.XReadGroup(ctx, &redis.XReadGroupArgs{Group: groupName, Consumer: "c1", Streams: []string{streamName, ">"}, Count: 1}).Result() + require.NoError(t, err) + _, err = rdb0.XReadGroup(ctx, &redis.XReadGroupArgs{Group: otherGroup, Consumer: "c2", Streams: []string{streamName, ">"}, Count: 1}).Result() + require.NoError(t, err) + r, err := rdb0.Do(ctx, "XACKDEL", streamName, groupName, "DELREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + + result, err := rdb0.Do(ctx, "POLLUPDATES", pollUpdates.NextSeq, "MAX", 20, "FORMAT", "RESP").Result() + require.NoError(t, err) + pollUpdates = parsePollUpdatesResult(t, result.(map[any]any), true) + xackCommands := collectRESPCommands(pollUpdates.Updates, "XACK") + require.ElementsMatch(t, [][]string{ + {"XACK", streamName, groupName, "1-0"}, + {"XACK", streamName, otherGroup, "1-0"}, + }, xackCommands) + xdelCommands := collectRESPCommands(pollUpdates.Updates, "XDEL") + require.Equal(t, [][]string{{"XDEL", streamName, "1-0"}}, xdelCommands) + require.Empty(t, collectRESPCommands(pollUpdates.Updates, "XACKDEL")) + + require.NoError(t, rdb1.FlushDB(ctx).Err()) + require.NoError(t, rdb1.XAdd(ctx, &redis.XAddArgs{Stream: streamName, ID: "1-0", Values: []string{"field", "value"}}).Err()) + require.NoError(t, rdb1.XGroupCreateMkStream(ctx, streamName, groupName, "$").Err()) + require.NoError(t, rdb1.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + _, err = rdb1.XReadGroup(ctx, &redis.XReadGroupArgs{Group: otherGroup, Consumer: "c2", Streams: []string{streamName, ">"}, Count: 1}).Result() + require.NoError(t, err) + applyRESPCommands(t, rdb1, pollUpdates.Updates, "XACK", "XDEL") + require.Equal(t, int64(0), rdb1.XLen(ctx, streamName).Val()) + otherPending, err := rdb1.XPending(ctx, streamName, otherGroup).Result() + require.NoError(t, err) + require.Equal(t, int64(0), otherPending.Count) + }) + + t.Run("Stream XACKDEL ACKED skipped replays ack only", func(t *testing.T) { + streamName := "poll_xackdel_acked_skipped" + groupName := "myGroup" + otherGroup := "otherGroup" + require.NoError(t, rdb0.XAdd(ctx, &redis.XAddArgs{Stream: streamName, ID: "1-0", Values: []string{"field", "value"}}).Err()) + require.NoError(t, rdb0.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + require.NoError(t, rdb0.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + _, err := rdb0.XReadGroup(ctx, &redis.XReadGroupArgs{Group: groupName, Consumer: "c1", Streams: []string{streamName, ">"}, Count: 1}).Result() + require.NoError(t, err) + _, err = rdb0.XReadGroup(ctx, &redis.XReadGroupArgs{Group: otherGroup, Consumer: "c2", Streams: []string{streamName, ">"}, Count: 1}).Result() + require.NoError(t, err) + r, err := rdb0.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(2)}, r) + + result, err := rdb0.Do(ctx, "POLLUPDATES", pollUpdates.NextSeq, "MAX", 20, "FORMAT", "RESP").Result() + require.NoError(t, err) + pollUpdates = parsePollUpdatesResult(t, result.(map[any]any), true) + xackCommands := collectRESPCommands(pollUpdates.Updates, "XACK") + require.Equal(t, [][]string{{"XACK", streamName, groupName, "1-0"}}, xackCommands) + require.Empty(t, collectRESPCommands(pollUpdates.Updates, "XDEL")) + require.Empty(t, collectRESPCommands(pollUpdates.Updates, "XACKDEL")) + + require.NoError(t, rdb1.FlushDB(ctx).Err()) + require.NoError(t, rdb1.XAdd(ctx, &redis.XAddArgs{Stream: streamName, ID: "1-0", Values: []string{"field", "value"}}).Err()) + require.NoError(t, rdb1.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + _, err = rdb1.XReadGroup(ctx, &redis.XReadGroupArgs{Group: groupName, Consumer: "c1", Streams: []string{streamName, ">"}, Count: 1}).Result() + require.NoError(t, err) + applyRESPCommands(t, rdb1, pollUpdates.Updates, "XACK") + require.Equal(t, int64(1), rdb1.XLen(ctx, streamName).Val()) + pending, err := rdb1.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(0), pending.Count) + }) + t.Run("JSON type", func(t *testing.T) { require.NoError(t, rdb0.JSONSet(ctx, "json", "$", `{"field": "value"}`).Err()) require.NoError(t, rdb0.JSONDel(ctx, "json", "$.field").Err()) diff --git a/tests/gocase/unit/type/stream/stream_test.go b/tests/gocase/unit/type/stream/stream_test.go index d3070fb6ddc..16b68fd5711 100644 --- a/tests/gocase/unit/type/stream/stream_test.go +++ b/tests/gocase/unit/type/stream/stream_test.go @@ -2644,6 +2644,37 @@ func TestStreamOffset(t *testing.T) { require.NoError(t, rdb.Del(ctx, streamKey).Err()) }) + setupXAckDelPending := func(t *testing.T, streamName, groupName string, ids ...string) { + t.Helper() + for i, id := range ids { + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ + Stream: streamName, + ID: id, + Values: []string{"field", strconv.Itoa(i)}, + }).Err()) + } + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: int64(len(ids)), + }).Result() + require.NoError(t, err) + } + + assertXAckDelNoMutation := func(t *testing.T, streamName, groupName string, args ...interface{}) { + t.Helper() + + cmd := append([]interface{}{"XACKDEL", streamName, groupName}, args...) + _, err := rdb.Do(ctx, cmd...).Result() + require.Error(t, err) + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(1), pending.Count) + } + t.Run("XACKDEL DELREF cross-group PEL cleanup", func(t *testing.T) { streamName := "xackdel_delref_" + strconv.Itoa(rand.Int()) groupName := "myGroup" @@ -2865,6 +2896,14 @@ func TestStreamOffset(t *testing.T) { require.Equal(t, []interface{}{int64(1)}, r) require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) + + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(0), pending.Count) + + consumers := rdb.XInfoConsumers(ctx, streamName, groupName).Val() + require.Len(t, consumers, 1) + require.Equal(t, int64(0), consumers[0].Pending) require.NoError(t, rdb.Del(ctx, streamName).Err()) }) @@ -2936,40 +2975,6 @@ func TestStreamOffset(t *testing.T) { require.NoError(t, rdb.Del(ctx, streamName).Err()) }) - t.Run("XACKDEL KEEPREF acks current group PEL", func(t *testing.T) { - streamName := "xackdel_keepref_ack_" + strconv.Itoa(rand.Int()) - groupName := "myGroup" - - require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ - Stream: streamName, - ID: "1-0", - Values: []string{"field", "value"}, - }).Err()) - require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) - - // Create a pending entry by reading. - _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ - Group: groupName, - Consumer: "c1", - Streams: []string{streamName, ">"}, - Count: 1, - }).Result() - require.NoError(t, err) - - r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "1-0").Result() - require.NoError(t, err) - require.Equal(t, []interface{}{int64(1)}, r) - - require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) - - // KEEPREF must still ACK the current group's pending entry. - pending, err := rdb.XPending(ctx, streamName, groupName).Result() - require.NoError(t, err) - require.Equal(t, int64(0), pending.Count) - - require.NoError(t, rdb.Del(ctx, streamName).Err()) - }) - t.Run("XACKDEL KEEPREF non-existent entry returns -1", func(t *testing.T) { streamName := "xackdel_keepref_notfound_" + strconv.Itoa(rand.Int()) groupName := "myGroup" @@ -3131,95 +3136,374 @@ func TestStreamOffset(t *testing.T) { require.NoError(t, rdb.Del(ctx, streamName).Err()) }) - t.Run("XACKDEL with invalid option returns error", func(t *testing.T) { - streamName := "xackdel_badopt_" + strconv.Itoa(rand.Int()) + t.Run("XACKDEL argument count boundaries", func(t *testing.T) { + streamName := "xackdel_arg_count_" + strconv.Itoa(rand.Int()) groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0") + + for _, args := range [][]interface{}{ + {"XACKDEL"}, + {"XACKDEL", streamName}, + {"XACKDEL", streamName, groupName}, + {"XACKDEL", streamName, groupName, "KEEPREF"}, + {"XACKDEL", streamName, groupName, "KEEPREF", "IDS"}, + } { + err := rdb.Do(ctx, args...).Err() + require.ErrorContains(t, err, "wrong number of arguments") + } + + for _, tc := range []struct { + name string + args []interface{} + }{ + {name: "zero numids", args: []interface{}{"KEEPREF", "IDS", "0"}}, + {name: "negative numids", args: []interface{}{"KEEPREF", "IDS", "-1"}}, + {name: "numids without id", args: []interface{}{"KEEPREF", "IDS", "1"}}, + {name: "fewer ids than numids", args: []interface{}{"KEEPREF", "IDS", "2", "1-0"}}, + {name: "more ids than numids", args: []interface{}{"KEEPREF", "IDS", "1", "1-0", "2-0"}}, + } { + t.Run(tc.name, func(t *testing.T) { + assertXAckDelNoMutation(t, streamName, groupName, tc.args...) + }) + } - require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ - Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, - }).Err()) - require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) - _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "INVALID", "IDS", "1", "1-0").Result() - require.Error(t, err) + t.Run("XACKDEL argument value boundaries", func(t *testing.T) { + streamName := "xackdel_arg_value_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0") + + for _, tc := range []struct { + name string + args []interface{} + }{ + {name: "invalid id", args: []interface{}{"KEEPREF", "IDS", "1", "abc"}}, + {name: "invalid option before ids", args: []interface{}{"GARBAGE", "IDS", "1", "1-0"}}, + {name: "invalid option after ids", args: []interface{}{"IDS", "1", "1-0", "GARBAGE"}}, + {name: "acked id ms overflow", args: []interface{}{"ACKED", "IDS", "1", "18446744073709551616-0"}}, + {name: "acked id seq overflow", args: []interface{}{"ACKED", "IDS", "1", "1-18446744073709551616"}}, + {name: "invalid id sequence", args: []interface{}{"KEEPREF", "IDS", "1", "1--1"}}, + {name: "negative id", args: []interface{}{"DELREF", "IDS", "1", "-1-0"}}, + } { + t.Run(tc.name, func(t *testing.T) { + assertXAckDelNoMutation(t, streamName, groupName, tc.args...) + }) + } require.NoError(t, rdb.Del(ctx, streamName).Err()) }) - t.Run("XACKDEL missing IDS returns error", func(t *testing.T) { - streamName := "xackdel_noids_" + strconv.Itoa(rand.Int()) + t.Run("XACKDEL accepts option after IDS", func(t *testing.T) { + streamName := "xackdel_option_after_ids_" + strconv.Itoa(rand.Int()) groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0") - require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ - Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, - }).Err()) - require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) - - _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF").Result() - require.Error(t, err) + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "IDS", "1", "1-0", "DELREF").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) require.NoError(t, rdb.Del(ctx, streamName).Err()) }) - t.Run("XACKDEL with non-integer numids returns error", func(t *testing.T) { - streamName := "xackdel_badnumids_" + strconv.Itoa(rand.Int()) - groupName := "myGroup" + t.Run("XACKDEL rejects duplicate or conflicting options without mutation", func(t *testing.T) { + for _, tc := range []struct { + name string + args []interface{} + }{ + {name: "duplicate", args: []interface{}{"KEEPREF", "KEEPREF", "IDS", "1", "1-0"}}, + {name: "conflict", args: []interface{}{"KEEPREF", "IDS", "1", "1-0", "DELREF"}}, + } { + t.Run(tc.name, func(t *testing.T) { + streamName := "xackdel_option_error_" + tc.name + "_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0") + + cmd := append([]interface{}{"XACKDEL", streamName, groupName}, tc.args...) + _, err := rdb.Do(ctx, cmd...).Result() + require.Error(t, err) + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(1), pending.Count) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + } + }) - require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ - Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, - }).Err()) - require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + t.Run("XACKDEL repeated IDS uses last block", func(t *testing.T) { + streamName := "xackdel_repeated_ids_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0", "2-0") - _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "abc").Result() - require.Error(t, err) + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "IDS", "1", "1-0", "IDS", "1", "2-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + entries, err := rdb.XRange(ctx, streamName, "-", "+").Result() + require.NoError(t, err) + require.Len(t, entries, 1) + require.Equal(t, "1-0", entries[0].ID) require.NoError(t, rdb.Del(ctx, streamName).Err()) }) - t.Run("XACKDEL with zero numids returns error", func(t *testing.T) { - streamName := "xackdel_zeronumids_" + strconv.Itoa(rand.Int()) + t.Run("XACKDEL invalid final IDS block does not mutate", func(t *testing.T) { + streamName := "xackdel_repeated_ids_error_" + strconv.Itoa(rand.Int()) groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0", "2-0") - require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ - Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, - }).Err()) - require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) - - _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "0").Result() + _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "IDS", "1", "1-0", "IDS", "bad", "2-0").Result() require.Error(t, err) + require.Equal(t, int64(2), rdb.XLen(ctx, streamName).Val()) + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(2), pending.Count) require.NoError(t, rdb.Del(ctx, streamName).Err()) }) - t.Run("XACKDEL with negative numids returns error", func(t *testing.T) { - streamName := "xackdel_negnumids_" + strconv.Itoa(rand.Int()) - groupName := "myGroup" + t.Run("XACKDEL rejects Redis-incompatible numids formats", func(t *testing.T) { + for _, numids := range []string{"01", "+1", "1.0", "18446744073709551616"} { + t.Run(numids, func(t *testing.T) { + streamName := "xackdel_bad_numids_format_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0") - require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ - Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, - }).Err()) - require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "IDS", numids, "1-0").Result() + require.Error(t, err) + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(1), pending.Count) - _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "-1").Result() - require.Error(t, err) + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + } + }) - require.NoError(t, rdb.Del(ctx, streamName).Err()) + t.Run("XACKDEL accepts Redis-compatible signed stream ID components", func(t *testing.T) { + for _, id := range []string{"1", "+1-0", "1-+0", "+1-+0", "1--0"} { + t.Run(id, func(t *testing.T) { + streamName := "xackdel_signed_id_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0") + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "IDS", "1", id).Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + } + }) + + t.Run("XACKDEL rejects invalid signed stream ID components", func(t *testing.T) { + for _, id := range []string{"-0-0", "-1-0", "1--1", "18446744073709551616-0", "1-18446744073709551616"} { + t.Run(id, func(t *testing.T) { + streamName := "xackdel_invalid_signed_id_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0") + + _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "IDS", "1", id).Result() + require.Error(t, err) + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(1), pending.Count) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + } }) - t.Run("XACKDEL with invalid entry ID returns error", func(t *testing.T) { - streamName := "xackdel_badid_" + strconv.Itoa(rand.Int()) + t.Run("XACKDEL ACKED duplicate skipped returns minus one for duplicate", func(t *testing.T) { + streamName := "xackdel_acked_duplicate_skipped_" + strconv.Itoa(rand.Int()) groupName := "myGroup" + otherGroup := "otherGroup" require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{ - Stream: streamName, ID: "1-0", Values: []string{"field", "value"}, + Stream: streamName, + ID: "1-0", + Values: []string{"field", "value"}, }).Err()) require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) - _, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "bad-id").Result() - require.Error(t, err) + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: groupName, + Consumer: "c1", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + _, err = rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: otherGroup, + Consumer: "c2", + Streams: []string{streamName, ">"}, + Count: 1, + }).Result() + require.NoError(t, err) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "2", "1-0", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(2), int64(-1)}, r) + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) require.NoError(t, rdb.Del(ctx, streamName).Err()) }) + + t.Run("XACKDEL ACKED truth table edge cases", func(t *testing.T) { + t.Run("other group created at dollar does not block", func(t *testing.T) { + streamName := "xackdel_acked_dollar_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + otherGroup := "otherGroup" + + setupXAckDelPending(t, streamName, groupName, "1-0") + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, otherGroup, "$").Err()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("other group created at zero but not delivered blocks", func(t *testing.T) { + streamName := "xackdel_acked_undelivered_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + otherGroup := "otherGroup" + + setupXAckDelPending(t, streamName, groupName, "1-0") + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(2)}, r) + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("past last delivered without PEL does not block", func(t *testing.T) { + streamName := "xackdel_acked_past_clear_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + otherGroup := "otherGroup" + + setupXAckDelPending(t, streamName, groupName, "1-0", "2-0") + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: otherGroup, Consumer: "c2", Streams: []string{streamName, ">"}, Count: 2, + }).Result() + require.NoError(t, err) + require.NoError(t, rdb.XAck(ctx, streamName, otherGroup, "1-0").Err()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("multiple IDs are evaluated independently", func(t *testing.T) { + streamName := "xackdel_acked_mixed_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + otherGroup := "otherGroup" + + setupXAckDelPending(t, streamName, groupName, "1-0", "2-0") + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{ + Group: otherGroup, Consumer: "c2", Streams: []string{streamName, ">"}, Count: 2, + }).Result() + require.NoError(t, err) + require.NoError(t, rdb.XAck(ctx, streamName, otherGroup, "1-0").Err()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "2", "1-0", "2-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1), int64(2)}, r) + require.Equal(t, int64(1), rdb.XLen(ctx, streamName).Val()) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + }) + + t.Run("XACKDEL dangling current-group PEL", func(t *testing.T) { + t.Run("KEEPREF acknowledges dangling PEL", func(t *testing.T) { + streamName := "xackdel_keepref_dangling_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + + setupXAckDelPending(t, streamName, groupName, "1-0") + require.NoError(t, rdb.XDel(ctx, streamName, "1-0").Err()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + pending, err := rdb.XPending(ctx, streamName, groupName).Result() + require.NoError(t, err) + require.Equal(t, int64(0), pending.Count) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("ACKED dangling PEL checks other groups", func(t *testing.T) { + streamName := "xackdel_acked_dangling_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + otherGroup := "otherGroup" + + require.NoError(t, rdb.XAdd(ctx, &redis.XAddArgs{Stream: streamName, ID: "1-0", Values: []string{"field", "value"}}).Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, groupName, "0").Err()) + require.NoError(t, rdb.XGroupCreateMkStream(ctx, streamName, otherGroup, "0").Err()) + _, err := rdb.XReadGroup(ctx, &redis.XReadGroupArgs{Group: groupName, Consumer: "c1", Streams: []string{streamName, ">"}, Count: 1}).Result() + require.NoError(t, err) + _, err = rdb.XReadGroup(ctx, &redis.XReadGroupArgs{Group: otherGroup, Consumer: "c2", Streams: []string{streamName, ">"}, Count: 1}).Result() + require.NoError(t, err) + require.NoError(t, rdb.XDel(ctx, streamName, "1-0").Err()) + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "ACKED", "IDS", "1", "1-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(2)}, r) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + }) + + t.Run("XACKDEL boundary recalculation after deleting last and all entries", func(t *testing.T) { + t.Run("delete last entry", func(t *testing.T) { + streamName := "xackdel_boundary_last_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0", "2-0", "3-0") + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "1", "3-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1)}, r) + msgs, err := rdb.XRange(ctx, streamName, "-", "+").Result() + require.NoError(t, err) + require.Len(t, msgs, 2) + require.Equal(t, "2-0", msgs[1].ID) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + + t.Run("delete all entries", func(t *testing.T) { + streamName := "xackdel_boundary_all_" + strconv.Itoa(rand.Int()) + groupName := "myGroup" + setupXAckDelPending(t, streamName, groupName, "1-0", "2-0") + + r, err := rdb.Do(ctx, "XACKDEL", streamName, groupName, "KEEPREF", "IDS", "2", "1-0", "2-0").Result() + require.NoError(t, err) + require.Equal(t, []interface{}{int64(1), int64(1)}, r) + require.Equal(t, int64(0), rdb.XLen(ctx, streamName).Val()) + msgs, err := rdb.XRange(ctx, streamName, "-", "+").Result() + require.NoError(t, err) + require.Empty(t, msgs) + + require.NoError(t, rdb.Del(ctx, streamName).Err()) + }) + }) + } func parseStreamEntryID(id string) (ts int64, seqNum int64) { From 2865e0e4c02be67be999ee07fed91d628c556a20 Mon Sep 17 00:00:00 2001 From: kirito632 Date: Sat, 20 Jun 2026 09:50:58 +0800 Subject: [PATCH 4/4] fix(stream): validate max stream ID and batch sender state --- src/cluster/batch_sender.cc | 2 +- src/commands/cmd_stream.cc | 3 +++ tests/gocase/unit/type/stream/stream_test.go | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/cluster/batch_sender.cc b/src/cluster/batch_sender.cc index 7ed64370f2c..c0d6c9dca4d 100644 --- a/src/cluster/batch_sender.cc +++ b/src/cluster/batch_sender.cc @@ -60,7 +60,7 @@ Status BatchSender::PutLogData(const rocksdb::Slice &blob) { if (!s.ok()) { return {Status::NotOK, fmt::format("failed to put log data to migration batch, {}", s.ToString())}; } - pending_logdata_only_ = true; + pending_logdata_only_ = pending_logdata_only_ || pending_entries_ == 0; pending_entries_++; entries_num_++; return Status::OK(); diff --git a/src/commands/cmd_stream.cc b/src/commands/cmd_stream.cc index ee8756d562d..9bb6f15f05b 100644 --- a/src/commands/cmd_stream.cc +++ b/src/commands/cmd_stream.cc @@ -90,6 +90,9 @@ Status ParseXAckDelStreamEntryID(const std::string &input, redis::StreamEntryID id->ms = ms; id->seq = 0; } + if (id->ms > redis::StreamEntryID::Maximum().ms) { + return {Status::RedisParseErr, redis::kErrInvalidEntryIdSpecified}; + } return Status::OK(); } } // namespace diff --git a/tests/gocase/unit/type/stream/stream_test.go b/tests/gocase/unit/type/stream/stream_test.go index 16b68fd5711..220829754f9 100644 --- a/tests/gocase/unit/type/stream/stream_test.go +++ b/tests/gocase/unit/type/stream/stream_test.go @@ -3301,7 +3301,7 @@ func TestStreamOffset(t *testing.T) { }) t.Run("XACKDEL rejects invalid signed stream ID components", func(t *testing.T) { - for _, id := range []string{"-0-0", "-1-0", "1--1", "18446744073709551616-0", "1-18446744073709551616"} { + for _, id := range []string{"-0-0", "-1-0", "1--1", "18446744073709551615-0", "18446744073709551616-0", "1-18446744073709551616"} { t.Run(id, func(t *testing.T) { streamName := "xackdel_invalid_signed_id_" + strconv.Itoa(rand.Int()) groupName := "myGroup"