diff --git a/.github/workflows/daily.yml b/.github/workflows/daily.yml index b6a8e44ef54..0a92882f66b 100644 --- a/.github/workflows/daily.yml +++ b/.github/workflows/daily.yml @@ -1879,7 +1879,7 @@ jobs: strategy: fail-fast: false matrix: - os: [macos-14] + os: [macos-15] runs-on: ${{ matrix.os }} if: | (github.event_name == 'workflow_call' || github.event_name == 'workflow_dispatch' || diff --git a/src/bgiteration.c b/src/bgiteration.c index 5668a85d1da..e7df8892ff8 100644 --- a/src/bgiteration.c +++ b/src/bgiteration.c @@ -26,12 +26,9 @@ static bool isScriptCallWriteCmd(struct serverCommand *cmd) { * write commands, blocking them from modifying the format while the background thread might be * operating on the item. * - PFCOUNT - modifies the underlying string (and is replicated!) - * - LINDEX/LRANGE/LPOS - may modify quicklist by LZF compress/uncompress */ + * - SORT_RO may modify the encoding (#4698) */ static bool isFormatChangingCommand(struct serverCommand *cmd) { return ((cmd->proc == pfcountCommand) || - (cmd->proc == lindexCommand) || - (cmd->proc == lrangeCommand) || - (cmd->proc == lposCommand) || (cmd->proc == sortroCommand)); } @@ -406,6 +403,7 @@ struct bgIterator { * deleted key. keyDelete() removes the dbEntry from early_iterate_entries immediately (step 1) * to avoid a dangling pointer. This flag records if the entry was in early_iterate_entries. */ bool last_key_delete_was_early; // Used only in main thread during command processing + unsigned long added_since_feed; // Items queued outside of feedIterator since its last call // Variables maintaining runtime statistics unsigned long dbentries_queued; // Updated by main thread @@ -969,13 +967,16 @@ static sds createEntryString(int dbid, dbEntry *de) { static void feedIterator(bgIterator *it, monotime end_time_us) { - unsigned int initial_queue_len = mutexQueueLength(it->items_for_iterator); - /* The queue size dynamically adjusts using an AIMD approach. If we have left over stuff from * the prior call to feedIterator, reduce by half the remaining size. If the queue ran dry - * and we have time left (at the end of this function), additively increase the queue length. */ - if (initial_queue_len > 2 && it->item_count_target >= initial_queue_len) { - it->item_count_target -= initial_queue_len / 2; + * and we have time left (at the end of this function), additively increase the queue length. + * Items added since the prior call (expedited keys, replication) aren't left over, as the + * queue may have run dry before they were added. */ + unsigned long queue_len = mutexQueueLength(it->items_for_iterator); + unsigned long left_over = (queue_len > it->added_since_feed) ? queue_len - it->added_since_feed : 0; + it->added_since_feed = 0; + if (left_over > 2 && it->item_count_target >= left_over) { + it->item_count_target -= left_over / 2; } // Now do some feeding @@ -1076,7 +1077,7 @@ static void feedIterator(bgIterator *it, monotime end_time_us) { } // Smart logic to dynamically adjust the size of the queue - if (initial_queue_len == 0 && have_time && it->item_count_target < BGITER_QUEUE_MAX_LENGTH) { + if (left_over == 0 && have_time && it->item_count_target < BGITER_QUEUE_MAX_LENGTH) { it->item_count_target += BGITER_QUEUE_INCREASE_INCR; } } @@ -1095,6 +1096,7 @@ static bool addEarlyIterationKey(bgIterator *it, dbEntry *earlyEntry, int cur_db bgIteratorItem *item = makeDbEntryItem(isClonedEntry ? cloneEntry : earlyEntry, dbid, isClonedEntry); it->dbentries_queued++; + it->added_since_feed++; if (isClonedEntry) it->dbentry_clones_queued++; if (it->barrier_items == 0) { @@ -1804,6 +1806,7 @@ static void handleSwapdb(int db1, int db2) { item->dbid = db1; item->u.dbid2 = db2; it->swapdb_queued++; + it->added_since_feed++; it->barrier_items++; mutexQueueAdd(it->items_for_iterator, item); } @@ -1850,6 +1853,7 @@ static void handleFlushdb(int dbid) { item->type = BGITERATOR_ITEM_FLUSHDB; item->dbid = dbid; it->flushdb_queued++; + it->added_since_feed++; it->barrier_items++; mutexQueueAdd(it->items_for_iterator, item); } @@ -2036,6 +2040,7 @@ static bgIterator *bgIteratorCreate(const char *name, it->terminated = false; it->cur_cmd_may_replicate = false; it->last_key_delete_was_early = false; + it->added_since_feed = 0; it->dbentries_queued = 0; it->dbentries_processed = 0; @@ -2180,7 +2185,7 @@ bgIteratorItem *bgIteratorRead(bgIterator *it) { * Without this, a unit test could get stuck waiting on the completion event because * feed won't get invoked. For production, feed is called regularly from the main thread. * Note - this is checking that the exact same thread is used and shouldn't count modules. */ - if (onServerMainThread()) bgIteration_feedIterators_task(NULL, 0, NULL); + if (onServerMainThread()) bgIteration_feedIterators(); } else { it->client_is_active = true; } @@ -2644,6 +2649,7 @@ void bgIteration_handleCommandReplication(int dbid, item->u.repl.replication_size = replicationItemSize(item); bufferedReplicationBytes += item->u.repl.replication_size; it->replication_queued++; + it->added_since_feed++; mutexQueueAdd(it->items_for_iterator, item); } } // allIterators loop diff --git a/src/cluster_legacy.c b/src/cluster_legacy.c index 6873361c9a8..b5b85511fc1 100644 --- a/src/cluster_legacy.c +++ b/src/cluster_legacy.c @@ -253,6 +253,14 @@ static inline char *clusterLinkGetHumanNodeName(clusterLink *link) { static_assert(offsetof(clusterMsg, type) + sizeof(uint16_t) == RCVBUF_MIN_READ_LEN, "Incorrect length to read to identify type"); +/* Upper bound on the length of an inbound packet, checked from the header so + * a peer can't make us buffer a huge packet of a type that is never that big. + * A PING carries at most 65535 gossip sections (6.8MB) plus extensions that + * are small for any sane human nodename and availability zone. Unknown types + * are capped too. Pub/Sub and module messages are exempt: their payload comes + * from a client and can legitimately be much larger. */ +#define CLUSTERMSG_MAX_LEN (16 * 1024 * 1024) + /* Cluster nodes hash table, mapping nodes addresses 1.2.3.4:6379 to * clusterNode structures. */ dictType clusterNodesDictType = { @@ -5043,6 +5051,13 @@ void clusterLinkConnectHandler(connection *conn) { serverLog(LL_DEBUG, "Connecting with Node %.40s at %s:%d", node->name, node->ip, node->cport); } +static inline int isClusterMsgLengthTooLarge(uint16_t type, uint32_t totlen) { + type &= ~CLUSTERMSG_MODIFIER_MASK; + if (type == CLUSTERMSG_TYPE_PUBLISH || type == CLUSTERMSG_TYPE_PUBLISHSHARD || type == CLUSTERMSG_TYPE_MODULE) + return 0; + return totlen > CLUSTERMSG_MAX_LEN; +} + /* Performs sanity check on the message signature and length depending on the type. */ static inline int isClusterMsgSignatureAndLengthValid(clusterMsgHeader *hdr) { if (memcmp(hdr->sig, "RCmb", 4) != 0) return 0; @@ -5050,6 +5065,7 @@ static inline int isClusterMsgSignatureAndLengthValid(clusterMsgHeader *hdr) { uint32_t totlen = ntohl(hdr->totlen); uint32_t minlen = IS_LIGHT_MESSAGE(type) ? CLUSTERMSG_LIGHT_MIN_LEN : CLUSTERMSG_MIN_LEN; if (totlen < minlen) return 0; + if (isClusterMsgLengthTooLarge(type, totlen)) return 0; return 1; } @@ -5093,7 +5109,7 @@ void clusterFindCompletePackets(char *rcvbuf, uint16_t type = ntohs(hdr->type); uint32_t minlen = IS_LIGHT_MESSAGE(type) ? CLUSTERMSG_LIGHT_MIN_LEN : CLUSTERMSG_MIN_LEN; - if (totlen < minlen) { + if (totlen < minlen || isClusterMsgLengthTooLarge(type, totlen)) { *complete_bytes = offset; /* preserve any valid prefix already scanned */ *result = CLUSTER_IO_BAD_LENGTH; return; diff --git a/src/commands.def b/src/commands.def index 2091703f700..3f6dc4be4bc 100644 --- a/src/commands.def +++ b/src/commands.def @@ -9655,7 +9655,7 @@ struct COMMAND_ARG SINTERSTORE_Args[] = { #ifndef SKIP_CMD_HISTORY_TABLE /* SISMEMBER history */ commandHistory SISMEMBER_History[] = { -{"9.1.0","Added the `XX` options."}, +{"9.2.0","Added the `XX` option."}, }; #endif @@ -9675,7 +9675,7 @@ keySpec SISMEMBER_Keyspecs[1] = { struct COMMAND_ARG SISMEMBER_Args[] = { {MAKE_ARG("key",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, {MAKE_ARG("member",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, -{MAKE_ARG("xx",ARG_TYPE_PURE_TOKEN,-1,"XX",NULL,"9.1.0",CMD_ARG_OPTIONAL,0,NULL)}, +{MAKE_ARG("xx",ARG_TYPE_PURE_TOKEN,-1,"XX",NULL,"9.2.0",CMD_ARG_OPTIONAL,0,NULL)}, }; /********** SMEMBERS ********************/ diff --git a/src/commands/cluster-getslotmigrations.json b/src/commands/cluster-getslotmigrations.json index 159ae250408..306774437d2 100644 --- a/src/commands/cluster-getslotmigrations.json +++ b/src/commands/cluster-getslotmigrations.json @@ -19,7 +19,7 @@ "type": "object", "additionalProperties": false, "properties": { - "link_name": { + "name": { "type": "string", "description": "A 40-byte randomly generated hex string representing the migration.", "pattern": "^[0-9a-fA-F]{40}$" @@ -72,6 +72,11 @@ "cow_size": { "description": "Copy on write bytes during slot migration fork.", "type": "integer" + }, + "remaining_repl_size": { + "description": "Bytes pending in the source slot export client's reply buffer. Zero for imports or when no export client is attached.", + "type": "integer", + "minimum": 0 } } } diff --git a/src/commands/sismember.json b/src/commands/sismember.json index 4d9235bc628..c88dda8c945 100644 --- a/src/commands/sismember.json +++ b/src/commands/sismember.json @@ -8,8 +8,8 @@ "function": "sismemberCommand", "history": [ [ - "9.1.0", - "Added the `XX` options." + "9.2.0", + "Added the `XX` option." ] ], "command_flags": [ @@ -70,7 +70,7 @@ "name": "xx", "token": "XX", "type": "pure-token", - "since": "9.1.0", + "since": "9.2.0", "optional": true } ] diff --git a/src/module.c b/src/module.c index cb73d882087..c6a72bf6842 100644 --- a/src/module.c +++ b/src/module.c @@ -4463,8 +4463,19 @@ void VM_CloseKey(ValkeyModuleKey *key) { zfree(key); } -/* Return the type of the key. If the key pointer is NULL then - * VALKEYMODULE_KEYTYPE_EMPTY is returned. */ +/* Return the type of the key as one of the following: + * + * - VALKEYMODULE_KEYTYPE_EMPTY + * - VALKEYMODULE_KEYTYPE_STRING + * - VALKEYMODULE_KEYTYPE_LIST + * - VALKEYMODULE_KEYTYPE_HASH + * - VALKEYMODULE_KEYTYPE_SET + * - VALKEYMODULE_KEYTYPE_ZSET + * - VALKEYMODULE_KEYTYPE_MODULE + * - VALKEYMODULE_KEYTYPE_STREAM + * - VALKEYMODULE_KEYTYPE_PATH_HASH + * + * If the key pointer is NULL then VALKEYMODULE_KEYTYPE_EMPTY is returned. */ int VM_KeyType(ValkeyModuleKey *key) { if (key == NULL || key->value == NULL) return VALKEYMODULE_KEYTYPE_EMPTY; /* We map between defines so that we are free to change the internal @@ -9620,14 +9631,16 @@ void moduleReleaseGIL(void) { * - VALKEYMODULE_NOTIFY_EVICTED: Eviction events * - VALKEYMODULE_NOTIFY_STREAM: Stream events * - VALKEYMODULE_NOTIFY_MODULE: Module types events - * - VALKEYMODULE_NOTIFY_KEYMISS: Key-miss events + * - VALKEYMODULE_NOTIFY_PATH_HASH: Path hash events + * - VALKEYMODULE_NOTIFY_NEW: New key events + * - VALKEYMODULE_NOTIFY_KEY_MISS: Key-miss events * Notice, key-miss event is the only type * of event that is fired from within a read command. * Performing VM_Call with a write command from within * this notification is wrong and discourage. It will * cause the read command that trigger the event to be * replicated to the AOF/Replica. - * - VALKEYMODULE_NOTIFY_ALL: All events (Excluding VALKEYMODULE_NOTIFY_KEYMISS) + * - VALKEYMODULE_NOTIFY_ALL: All events (Excluding VALKEYMODULE_NOTIFY_KEY_MISS and VALKEYMODULE_NOTIFY_NEW) * - VALKEYMODULE_NOTIFY_LOADED: A special notification available only for modules, * indicates that the key was loaded from persistence. * Notice, when this event fires, the given key diff --git a/src/notify.c b/src/notify.c index b3028561148..510ca239fb3 100644 --- a/src/notify.c +++ b/src/notify.c @@ -88,8 +88,8 @@ sds keyspaceEventsFlagsToString(int flags) { if (flags & NOTIFY_STREAM) res = sdscatlen(res, "t", 1); if (flags & NOTIFY_MODULE) res = sdscatlen(res, "d", 1); if (flags & NOTIFY_PATH_HASH) res = sdscatlen(res, "p", 1); - if (flags & NOTIFY_NEW) res = sdscatlen(res, "n", 1); } + if (flags & NOTIFY_NEW) res = sdscatlen(res, "n", 1); if (flags & NOTIFY_KEYSPACE) res = sdscatlen(res, "K", 1); if (flags & NOTIFY_KEYEVENT) res = sdscatlen(res, "E", 1); if (flags & NOTIFY_KEY_MISS) res = sdscatlen(res, "m", 1); diff --git a/src/server.c b/src/server.c index 0fbbcf18b25..72f666977b5 100644 --- a/src/server.c +++ b/src/server.c @@ -4066,7 +4066,8 @@ static void propagatePendingCommands(void) { * Must stay an OR of every condition below - never drop one as an * optimization, since that would silently skip real pending work. */ static inline int hasPostExecutionUnitPendingWork(void) { - return moduleHasPostExecUnitJobs() || server.also_propagate.numops || server.busy_module_yield_flags; + return moduleHasPostExecUnitJobs() || server.also_propagate.numops || server.busy_module_yield_flags || + bgIteration_iterationActive(); } /* Performs operations that should be performed after an execution unit ends. diff --git a/src/t_stream.c b/src/t_stream.c index 9b77223f130..297604c8782 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -477,10 +477,14 @@ int streamAppendItem(stream *s, robj **argv, int64_t numfields, streamID *added_ size_t lp_bytes = 0; /* Total bytes in the tail listpack. */ unsigned char *lp = NULL; /* Tail listpack pointer. */ + /* What the rax holds for the key this call writes, NULL if it holds nothing. + * Only used to skip a raxInsert storing a pointer the rax already has. */ + unsigned char *rax_lp = NULL; if (!raxEOF(&ri)) { /* Get a reference to the tail node listpack. */ lp = ri.data; + rax_lp = ri.data; lp_bytes = lpBytes(lp); } raxStop(&ri); @@ -542,7 +546,8 @@ int streamAppendItem(stream *s, robj **argv, int64_t numfields, streamID *added_ if (new_node) { /* Shrink extra pre-allocated memory */ lp = lpShrinkToFit(lp); - if (ri.data != lp) raxInsert(s->rax, ri.key, ri.key_len, lp, NULL); + if (rax_lp != lp) raxInsert(s->rax, ri.key, ri.key_len, lp, NULL); + rax_lp = NULL; /* the rest of this call writes rax_key, a new key */ lp = NULL; } } @@ -570,7 +575,6 @@ int streamAppendItem(stream *s, robj **argv, int64_t numfields, streamID *added_ lp = lpAppend(lp, (unsigned char *)field, sdslen(field)); } lp = lpAppendInteger(lp, 0); /* primary entry zero terminator. */ - raxInsert(s->rax, (unsigned char *)&rax_key, sizeof(rax_key), lp, NULL); /* The first entry we insert, has obviously the same fields of the * primary entry. */ flags |= STREAM_ITEM_FLAG_SAMEFIELDS; @@ -651,7 +655,7 @@ int streamAppendItem(stream *s, robj **argv, int64_t numfields, streamID *added_ lp = lpAppendInteger(lp, lp_count); /* Insert back into the tree in order to update the listpack pointer. */ - if (ri.data != lp) raxInsert(s->rax, (unsigned char *)&rax_key, sizeof(rax_key), lp, NULL); + if (rax_lp != lp) raxInsert(s->rax, (unsigned char *)&rax_key, sizeof(rax_key), lp, NULL); s->length++; s->entries_added++; s->last_id = id; @@ -1464,7 +1468,14 @@ void streamReplyWithCGLag(client *c, stream *s, streamCG *cg) { } else if (cg->entries_read != SCG_INVALID_ENTRIES_READ && !streamRangeHasTombstones(s, &cg->last_id, NULL)) { /* No fragmentation ahead means that the group's logical reads counter * is valid for performing the lag calculation. */ - lag = (long long)s->entries_added - cg->entries_read; + if (!s->length || streamCompareID(&cg->last_id, &s->first_id) < 0) { + /* The group is positioned before the first entry, so entries it + * hasn't read may have been trimmed (XTRIM doesn't update + * max_deleted_entry_id). Everything left in the stream is unread. */ + lag = (long long)s->length; + } else { + lag = (long long)s->entries_added - cg->entries_read; + } valid = 1; } else { /* Attempt to retrieve the group's last ID logical read counter. */ diff --git a/src/unit/test_bgiteration.cpp b/src/unit/test_bgiteration.cpp index ec7d22a202a..8928532a3f5 100644 --- a/src/unit/test_bgiteration.cpp +++ b/src/unit/test_bgiteration.cpp @@ -3220,3 +3220,37 @@ TEST_F(BgIterationTest, checkNoKeysWriteIsReplicated) { expectReadKeySequence(it, 3, LAST_ITEM); expectReadComplete(it); } + + +// During a save, a write to an unsaved key queues that key ahead of the scan. Expedited keys +// waiting at a feed must not stop the queue target from growing. +TEST_F(BgIterationTest, queueTargetGrowsWithExpeditedKeysWaiting) { + // Start a consistent save, as BGSAVE does. Capture its feed timer so the test runs the feed. + aeTimeProc *feed = NULL; + EXPECT_CALL(mock, aeCreateTimeEvent(_, _, _, _, _)).WillOnce(DoAll(SaveArg<2>(&feed), Return(0))); + bgIterator *it = bgIteratorCreateFullScanIter("iter", BGITERATOR_CONSISTENCY_START, NULL, + iteratorCleanupFn, PRIVDATA); + ASSERT_NE(feed, nullptr); + + // Before anything is fed, write 3 keys. All are expedited into the queue. + for (int item = 0; item < 3; item++) { + client *writer = getWriteClient(item, "xxx"); + simulateBlockedWrite(writer); + freeTestClient(writer); + } + + // The client reads one of them. (The first read doesn't run a feed.) + bgIteratorRead(it); + + bgIteratorStatus status; + bgIteratorGetStatus(it, &status); + ASSERT_EQ(status.queue_length, 2u); + unsigned long target = status.queue_length_target; + + // Run one feed. Everything in the queue was added since the last feed, so the target must grow. + feed(NULL, 0, NULL); + bgIteratorGetStatus(it, &status); + EXPECT_GT(status.queue_length_target, target); + + expectAnythingCleanup(it); +} diff --git a/src/valkey-cli.c b/src/valkey-cli.c index 704c32e6274..78205a5729c 100644 --- a/src/valkey-cli.c +++ b/src/valkey-cli.c @@ -10002,6 +10002,7 @@ void testHint(const char *input) { sds hint = getHintForInput(input); printf("%s\n", hint); + sdsfree(hint); exit(0); } diff --git a/tests/integration/replication.tcl b/tests/integration/replication.tcl index d1c769f7f8a..cefd969f152 100644 --- a/tests/integration/replication.tcl +++ b/tests/integration/replication.tcl @@ -2491,6 +2491,11 @@ start_server {tags {"repl external:skip"} overrides {save {} forkless-infrastruc set load2 [start_write_load $primary_host $primary_port 10] set load3 [start_write_load $primary_host $primary_port 10] set load4 [start_write_load $primary_host $primary_port 10] + wait_for_condition 50 100 { + [regexp -all {name=LOAD_HANDLER} [$primary client list]] == 4 + } else { + fail "Write load handlers didn't connect" + } after 200 $replica replicaof $primary_host $primary_port wait_for_condition 500 100 { @@ -2505,6 +2510,7 @@ start_server {tags {"repl external:skip"} overrides {save {} forkless-infrastruc stop_write_load $load2 stop_write_load $load3 stop_write_load $load4 + wait_load_handlers_disconnected -1 wait_for_ofs_sync $primary $replica assert_equal [$primary dbsize] [$replica dbsize] assert_equal [$primary debug digest] [$replica debug digest] @@ -2541,6 +2547,11 @@ foreach primary_diskless {yes no} { test "[string toupper $cmd] during forkless sync replays correctly (primary diskless=$primary_diskless, replica load=$replica_load)" { set load1 [start_write_load $ph $pp 8] set load2 [start_write_load $ph $pp 8] + wait_for_condition 50 100 { + [regexp -all {name=LOAD_HANDLER} [$primary client list]] == 2 + } else { + fail "Write load handlers didn't connect" + } $primary config set rdb-key-save-delay 100 $replica replicaof $ph $pp wait_for_condition 50 100 { @@ -2561,6 +2572,7 @@ foreach primary_diskless {yes no} { after 800 stop_write_load $load1 stop_write_load $load2 + wait_load_handlers_disconnected -1 wait_for_ofs_sync $primary $replica assert_equal [$primary debug digest] [$replica debug digest] assert_equal 0 [s -1 total_forks] @@ -2759,6 +2771,11 @@ foreach load_mode {disabled on-empty-db} { set loglines [count_log_lines -1] set load1 [start_write_load $primary_host $primary_port 60] set load2 [start_write_load $primary_host $primary_port 60] + wait_for_condition 50 100 { + [regexp -all {name=LOAD_HANDLER} [$primary client list]] == 2 + } else { + fail "Write load handlers didn't connect" + } after 200 $replica replicaof $primary_host $primary_port @@ -2774,6 +2791,7 @@ foreach load_mode {disabled on-empty-db} { stop_write_load $load1 stop_write_load $load2 + wait_load_handlers_disconnected -1 wait_for_ofs_sync $primary $replica assert_equal [$primary dbsize] [$replica dbsize] assert_equal [$primary debug digest] [$replica debug digest] diff --git a/tests/sentinel/tests/00-base.tcl b/tests/sentinel/tests/00-base.tcl index 7ec6d8130c0..7bb55d0ae2a 100644 --- a/tests/sentinel/tests/00-base.tcl +++ b/tests/sentinel/tests/00-base.tcl @@ -172,6 +172,20 @@ test "ODOWN is not possible without N (quorum) Sentinels reports" { test "Failover is not possible without majority agreement" { set quorum [expr {$sentinels/2 + 1}] + + # The previous test just restarted the primary. Sentinels can still show it + # as disconnected, and cross down-after right after we lower the quorum, + # which starts a real failover with every Sentinel alive. Wait until each + # one is connected and has had a recent PONG. + foreach_sentinel_id id { + wait_for_condition 1000 50 { + [dict get [S $id SENTINEL PRIMARY mymaster] flags] eq "master" && + [dict get [S $id SENTINEL PRIMARY mymaster] last-ok-ping-reply] < 1000 + } else { + fail "Sentinel $id does not see the primary as healthy" + } + } + foreach_sentinel_id id { S $id SENTINEL SET mymaster quorum $quorum } diff --git a/tests/sentinel/tests/07-down-conditions.tcl b/tests/sentinel/tests/07-down-conditions.tcl index fbc19248163..16d6b87ab6c 100644 --- a/tests/sentinel/tests/07-down-conditions.tcl +++ b/tests/sentinel/tests/07-down-conditions.tcl @@ -38,6 +38,7 @@ proc ensure_master_down {} { } test "Crash the majority of Sentinels to prevent failovers for this unit" { + set quorum [expr {[llength $::sentinel_instances] / 2 + 1}] for {set id 0} {$id < $quorum} {incr id} { kill_instance sentinel $id } diff --git a/tests/unit/client-eviction.tcl b/tests/unit/client-eviction.tcl index 865c56acdfc..a9685f07ee3 100644 --- a/tests/unit/client-eviction.tcl +++ b/tests/unit/client-eviction.tcl @@ -251,25 +251,40 @@ start_server {} { test "client evicted due to client tracking prefixes" { r flushdb set rr [valkey_client] + test_defer { $rr close } + # RESP3 so the client can still run CLIENT TRACKING after subscribing below + $rr hello 3 + + # The tracking prefixes list is only charged ~12 bytes per rax node , and registering a prefix scans + # all existing ones, so the cost grows quadratically with the number of prefixes. Since maxmemory-clients + # can't go below 128kb (see getClientEvictionLimit), pad the client with pubsub channel names to get close + # to the limit, so only a couple thousand prefixes are needed to trigger eviction. + set temp_maxmemory_clients [kb 128] + test_config_set r maxmemory-clients $temp_maxmemory_clients + # The headroom must absorb the reply buffers being resized by clientsCron while we pad. + set headroom [kb 20] + set c 0 + while {[set gap [expr {$temp_maxmemory_clients - $headroom - [clients_sum tot-mem]}]] > 0} { + $rr subscribe [string repeat x [expr {min($gap, [kb 8])}]][incr c] + } - # Since tracking prefixes list is a small overhead this test uses a minimal maxmemory-clients config - set temp_maxmemory_clients 200000 - r config set maxmemory-clients $temp_maxmemory_clients - - # Append tracking prefixes until list maxes out maxmemory clients and causes client eviction - # Combine more prefixes in each command to speed up the test. Because we did not actually count - # the memory usage of all prefixes, see getClientMemoryUsage, so we cannot use larger prefixes - # to speed up the test here. + # Append tracking prefixes until list maxes out maxmemory clients and causes client eviction. + # Send many prefixes per command to reduce the number of round trips. + set prefixes_per_cmd 100 + set n 0 catch { - for {set j 0} {$j < $temp_maxmemory_clients} {incr j} { - $rr client tracking on prefix [format a%09s $j] prefix [format b%09s $j] prefix [format c%09s $j] bcast + for {set j 0} {$j < 100} {incr j} { + set cmd [list client tracking on bcast] + for {set k 0} {$k < $prefixes_per_cmd} {incr k} { + lappend cmd prefix [format %010d $n] + incr n + } + $rr {*}$cmd } } e assert_match {I/O error reading reply} $e - $rr close - - # Restore config for next tests - r config set maxmemory-clients $maxmemory_clients + # Eviction must have been driven by the prefix list, not by the padding + assert {$j > 0} } test "client evicted due to output buf" { diff --git a/tests/unit/cluster/cli.tcl b/tests/unit/cluster/cli.tcl index 22ad9a0177a..437599cb6df 100644 --- a/tests/unit/cluster/cli.tcl +++ b/tests/unit/cluster/cli.tcl @@ -646,20 +646,23 @@ start_multiple_servers 3 [list overrides $base_conf] { # Result should be 1 due to stderr output assert_equal $result 1 assert_match {*unable to send CLUSTER RESET*} $output - assert_match {*Connection refused*} $output + # The errno string is OS-specific (e.g. macOS can report the refused + # connection as "setsockopt(TCP_NODELAY): Invalid argument" instead of + # "Connection refused"), so match only the stable prefix. + assert_match {*Could not connect to Valkey at*} $output # Check for success message assert_match {*\[OK\] Node*removed from the cluster*} $output - # Verify cluster state after deletion: - # - Node0 (primary): only node left, knows only itself - # - Node1 (deleted in test 2): still standalone - # - Node2 (deleted in test 3): shutdown, can't check + # Verify the deleted node is forgotten by the remaining nodes. + # Don't assert exact cluster_known_nodes counts: they depend on + # the previous del-node test having run, which isn't the case + # when this test is run alone with --only. wait_for_condition 1000 50 { - [CI 0 cluster_known_nodes] == 1 && - [CI 1 cluster_known_nodes] == 1 + [cluster_get_node_by_id 0 $node3_id] eq {} && + [cluster_get_node_by_id 1 $node3_id] eq {} } else { - fail "Nodes don't have expected cluster state" + fail "Deleted node is still known by the cluster" } } } ;# stop servers diff --git a/tests/unit/cluster/packet.tcl b/tests/unit/cluster/packet.tcl index 23b8bcd3092..dd088e6b072 100644 --- a/tests/unit/cluster/packet.tcl +++ b/tests/unit/cluster/packet.tcl @@ -341,6 +341,25 @@ start_cluster 1 0 {tags {external:skip cluster tls:skip}} { } } +start_cluster 1 0 {tags {external:skip cluster tls:skip}} { + test "Oversized PING length is rejected before the body is buffered" { + set cluster_port [expr {[srv 0 port] + 10000}] + + # Only the 14-byte header: signature, totlen of 256MB, ver, port, type PING. + set sock [socket 127.0.0.1 $cluster_port] + fconfigure $sock -translation binary -buffering none -blocking 0 + puts -nonewline $sock "RCmb[binary format ISSS [expr {256 * 1024 * 1024}] 1 0 0]" + + # The node must drop the link rather than wait for 256MB of body. + wait_for_condition 100 50 { + [read $sock] eq "" && [eof $sock] + } else { + fail "Node kept the link open waiting for an oversized packet" + } + close $sock + } +} + start_cluster 10 0 {tags {external:skip cluster tls:skip}} { test "Gossip count scales with higher percentage of `cluster-message-gossip-perc`" { R 0 config set cluster-message-gossip-perc 80 diff --git a/tests/unit/cluster/pubsub.tcl b/tests/unit/cluster/pubsub.tcl index 3d42e7f8900..33ad46b5769 100644 --- a/tests/unit/cluster/pubsub.tcl +++ b/tests/unit/cluster/pubsub.tcl @@ -59,3 +59,20 @@ start_cluster 3 0 {tags {external:skip cluster}} { assert_equal $sender_pubsub_bytes $receiver_pubsub_bytes } } + +start_cluster 2 0 {tags {external:skip cluster}} { + test "PUBLISH larger than the cluster bus packet cap reaches other nodes" { + set rd [valkey_deferring_client -1] + subscribe $rd {bigchannel} + + set data [string repeat x [expr {20 * 1024 * 1024}]] + R 0 PUBLISH bigchannel $data + wait_for_condition 100 100 { + [CI 1 cluster_stats_messages_publish_received] eq 1 + } else { + fail "node 1 didn't receive the clusterbus publish packet" + } + assert_equal [string length $data] [string length [lindex [$rd read] 2]] + $rd close + } +} diff --git a/tests/unit/cluster/slot-migration.tcl b/tests/unit/cluster/slot-migration.tcl index 75b8641ea62..9e70c5c0806 100644 --- a/tests/unit/cluster/slot-migration.tcl +++ b/tests/unit/cluster/slot-migration.tcl @@ -111,6 +111,19 @@ proc node_seen_healthy_by_all {node_id} { return 1 } +# Returns 1 when every node in the cluster sees $node_id as a replica. +proc node_seen_as_replica_by_all {node_id} { + for {set j 0} {$j < [llength $::servers]} {incr j} { + set node [cluster_get_node_by_id $j $node_id] + # A node turned into an empty node no longer knows the whole cluster. + if {$node eq {}} continue + if {![cluster_has_flag $node slave]} { + return 0 + } + } + return 1 +} + # Give the shard back to $primary with a manual failover. proc failback {primary} { set primary_id [R $primary CLUSTER MYID] @@ -119,6 +132,18 @@ proc failback {primary} { } else { fail "node $primary_id is still flagged as failing by some node" } + # Every node must also have seen $primary turn into a replica before it + # takes the shard back. A node refreshes its migrating/importing states + # for a shard only when it sees a replica->primary role change or a slot + # bitmap change in that shard. An empty shard's bitmap never changes, so + # a node that missed the demotion (its last packet from $primary predates + # it) sees no role change on the promotion either and would keep its open + # slots pointed at the old primary forever. + wait_for_condition 100 100 { + [node_seen_as_replica_by_all $primary_id] == 1 + } else { + fail "node $primary_id is not yet seen as a replica by all nodes" + } assert_equal {OK} [R $primary cluster failover] wait_for_role $primary master } @@ -352,6 +377,11 @@ start_cluster 3 5 {tags {external:skip cluster} overrides {cluster-allow-replica test "Empty-shard migration target is auto-updated after failover in target shard" { test_defer_fail { normalize_cluster $shard_pairs } + # Open slot 609 if the previous test did not run (e.g. --only runs). + if {[get_open_slots 0] eq ""} { + assert_equal {OK} [R 0 CLUSTER SETSLOT 609 MIGRATING $R6_id] + assert_equal {OK} [R 6 CLUSTER SETSLOT 609 IMPORTING $R0_id] + } # Trigger an auto-failover from R6 to R7 fail_server 6 7 # Validate final states @@ -370,6 +400,11 @@ start_cluster 3 5 {tags {external:skip cluster} overrides {cluster-allow-replica test "Empty-shard migration source is auto-updated after failover in source shard" { test_defer_fail { normalize_cluster $shard_pairs } + # Open slot 609 if the previous tests did not run (e.g. --only runs). + if {[get_open_slots 0] eq ""} { + assert_equal {OK} [R 0 CLUSTER SETSLOT 609 MIGRATING $R6_id] + assert_equal {OK} [R 6 CLUSTER SETSLOT 609 IMPORTING $R0_id] + } # Trigger an auto-failover from R0 to R3 fail_server 0 3 # Validate final states diff --git a/tests/unit/geo.tcl b/tests/unit/geo.tcl index da4244971e0..b3c8899d131 100644 --- a/tests/unit/geo.tcl +++ b/tests/unit/geo.tcl @@ -223,6 +223,22 @@ start_server {tags {"geo"}} { set err } {*valid*} + test {GEOADD out of range coordinates} { + r del range_check + # Valid longitudes are limited to [-180,+180] + assert_error "*invalid longitude,latitude pair*" {r geoadd range_check -190 40.747533 "out of range"} + assert_error "*invalid longitude,latitude pair*" {r geoadd range_check 190 40.747533 "out of range"} + # Valid latitudes are limited to [-85.05112878,+85.05112878] + assert_error "*invalid longitude,latitude pair*" {r geoadd range_check -73.9454966 -86 "out of range"} + assert_error "*invalid longitude,latitude pair*" {r geoadd range_check -73.9454966 86 "out of range"} + # The failing pair is reported even when it is not the first one. + assert_error "*invalid longitude,latitude pair 181.000000,38.000000*" { + r geoadd range_check -73.9454966 40.747533 "in range" 181 38 "out of range" + } + # GEOADD is all-or-nothing + assert_equal 0 [r exists range_check] + } + test {GEOADD multi add} { r geoadd nyc -73.9733487 40.7648057 "central park n/q/r" -73.9903085 40.7362513 "union square" -74.0131604 40.7126674 "wtc one" -73.7858139 40.6428986 "jfk" -73.9375699 40.7498929 "q4" -73.9564142 40.7480973 4545 } {6} @@ -459,6 +475,20 @@ start_server {tags {"geo"}} { assert {$m eq {}} } + test {GEODIST invalid unit} { + r del points + r geoadd points 13.361389 38.115556 "Palermo" \ + 15.087269 37.502669 "Catania" + assert_error "*unsupported unit*" {r geodist points Palermo Catania yards} + } + + test {GEODIST too many arguments} { + r del points + r geoadd points 13.361389 38.115556 "Palermo" \ + 15.087269 37.502669 "Catania" + assert_error "*syntax error*" {r geodist points Palermo Catania km extra} + } + test {GEORADIUS STORE option: syntax error} { r del points{t} r geoadd points{t} 13.361389 38.115556 "Palermo" \ @@ -622,7 +652,31 @@ start_server {tags {"geo"}} { assert_lessthan [expr {[s used_memory] - $before}] 262144 } {} {external:skip} + test {GEORADIUS/GEOSEARCH out of range coordinates} { + r del points + r geoadd points 13.361389 38.115556 "Palermo" + + assert_error "*invalid longitude,latitude pair*" {r georadius points 181 38 100 km} + assert_error "*invalid longitude,latitude pair*" {r georadius_ro points 13 86 100 km} + assert_error "*invalid longitude,latitude pair*" {r geosearch points fromlonlat 181 38 byradius 100 km} + assert_error "*invalid longitude,latitude pair*" {r geosearch points fromlonlat 13 86 bybox 100 100 km} + # A bad vertex is reported wherever it appears in the polygon. + assert_error "*invalid longitude,latitude pair 181.000000,40.000000*" { + r geosearch points bypolygon 3 13 38 14 39 181 40 + } + } + + test {GEOSEARCHSTORE out of range coordinates} { + r del points{t} dest{t} + r geoadd points{t} 13.361389 38.115556 "Palermo" + assert_error "*invalid longitude,latitude pair*" { + r geosearchstore dest{t} points{t} fromlonlat 181 38 byradius 100 km + } + assert_equal 0 [r exists dest{t}] + } + test {GEOSEARCH BYPOLYGON standard operations} { + r del points r geoadd points 151.20932132005692 -33.877723137822436 "146, Elizabeth Street, Koreatown, Sydney, Sydney CBD, Sydney, Council of the City of Sydney, New South Wales, 2000, Australia" r geoadd points 151.2119820713997 -33.87697539508043 "Connaught Centre, 187, Liverpool Street, Koreatown, Sydney, Sydney CBD, Sydney, Council of the City of Sydney, New South Wales, 2000, Australia" r geoadd points 151.2119820713997 -33.87156123068408 "St James, Archibald Fountain Plaza/Area, Koreatown, Sydney, Sydney CBD, Sydney, Council of the City of Sydney, New South Wales, 2000, Australia" diff --git a/tests/unit/pause.tcl b/tests/unit/pause.tcl index 73617204d30..8fe626e4c31 100644 --- a/tests/unit/pause.tcl +++ b/tests/unit/pause.tcl @@ -421,8 +421,13 @@ start_server {tags {"pause network"}} { } r config set busy-reply-threshold 10 + set loglines [count_log_lines 0] set rd [valkey_deferring_client] $rd eval_ro {while true do end} 0 + # With io-threads, a command read in the same I/O batch as the + # script runs only after the script ends, so don't talk to the + # replica until the script has entered busy mode. + wait_for_log_messages 0 {"*Slow script detected*"} $loglines 1000 10 wait_for_condition 50 100 { [catch {r ping} e] == 1 } else { diff --git a/tests/unit/pubsub.tcl b/tests/unit/pubsub.tcl index 5bf7cc17e03..9e40ae88a8c 100644 --- a/tests/unit/pubsub.tcl +++ b/tests/unit/pubsub.tcl @@ -494,6 +494,10 @@ start_server {tags {"pubsub network"}} { assert_equal {AK} [lindex [r config get notify-keyspace-events] 1] r config set notify-keyspace-events EA assert_equal {AE} [lindex [r config get notify-keyspace-events] 1] + r config set notify-keyspace-events KEAn + assert_equal {AnKE} [lindex [r config get notify-keyspace-events] 1] + r config set notify-keyspace-events KEAnm + assert_equal {AnKEm} [lindex [r config get notify-keyspace-events] 1] } test "Keyspace notifications: new key test" { diff --git a/tests/unit/type/path_hash.tcl b/tests/unit/type/path_hash.tcl index 7e7b9ca68b9..6d828ad4b23 100644 --- a/tests/unit/type/path_hash.tcl +++ b/tests/unit/type/path_hash.tcl @@ -1,3 +1,12 @@ +# macOS ships Tcl 8.5, where lsort does not support -stride. +proc sort_path_hash_field_pairs {pairs} { + set pair_list {} + foreach {field value} $pairs { + lappend pair_list [list $field $value] + } + return [concat {*}[lsort -index 0 $pair_list]] +} + start_server {tags {pathhash}} { test {PHSET creates a native path hash object and exact reads are binary safe} { set path [binary format H* 0001ff] @@ -122,7 +131,7 @@ start_server {tags {pathhash}} { [r phprefixes tree abczzz fields 3 f1 missing f2 count 1] set match [lindex [r phprefixes tree abczzz withvalues count 1] 0] assert_equal abc [lindex $match 0] - assert_equal {f1 v1 f2 v2} [lsort -stride 2 [lindex $match 1]] + assert_equal {f1 v1 f2 v2} [sort_path_hash_field_pairs [lindex $match 1]] assert_equal [list {}] [r phprefixes tree zzz count 1] assert_equal {0} [r phprefixes tree zzz lengths count 1] assert_equal {} [r phprefixes missing anything count 1] @@ -402,7 +411,7 @@ start_server {tags {pathhash}} { if {$cursor eq "0"} break assert {[incr pages] < 1000} } - assert_equal {user:1 Alice user:2 Bob} [lsort -stride 2 $values] + assert_equal {user:1 Alice user:2 Bob} [sort_path_hash_field_pairs $values] assert_equal {0 {}} [r phscan scan-tree a 0 match Alice] assert_equal {0 {}} [r phscan scan-tree missing 0] assert_equal {0 {}} [r phscan missing a 0] @@ -453,7 +462,7 @@ start_server {tags {pathhash}} { # Compact payloads finish in one call, even with COUNT 1. lassign [r phscan scan-tree p 0 count 1] cursor entries assert_equal 0 $cursor - assert_equal {f0 v0 f1 v1} [lsort -stride 2 $entries] + assert_equal {f0 v0 f1 v1} [sort_path_hash_field_pairs $entries] for {set i 2} {$i < 100} {incr i} { r phset scan-tree p fields 1 f$i v$i } diff --git a/tests/unit/type/stream-cgroups.tcl b/tests/unit/type/stream-cgroups.tcl index 74fd66c9365..fa8d365fa23 100644 --- a/tests/unit/type/stream-cgroups.tcl +++ b/tests/unit/type/stream-cgroups.tcl @@ -1226,6 +1226,38 @@ start_server { assert_equal [dict get $group lag] 0 } + test {Consumer group lag after XTRIM removes unread entries} { + r DEL x + for {set i 1} {$i <= 6} {incr i} { + r XADD x $i-0 data $i + } + r XGROUP CREATE x g1 0 + r XREADGROUP GROUP g1 c1 COUNT 1 STREAMS x > + r XTRIM x MAXLEN 2 + set reply [r XINFO STREAM x FULL] + set group [lindex [dict get $reply groups] 0] + assert_equal [dict get $group lag] 2 + + r XREADGROUP GROUP g1 c1 STREAMS x > + set reply [r XINFO STREAM x FULL] + set group [lindex [dict get $reply groups] 0] + assert_equal [dict get $group entries-read] 6 + assert_equal [dict get $group lag] 0 + } + + test {Consumer group lag after XTRIM empties the stream} { + r DEL x + for {set i 1} {$i <= 6} {incr i} { + r XADD x $i-0 data $i + } + r XGROUP CREATE x g1 0 + r XREADGROUP GROUP g1 c1 COUNT 1 STREAMS x > + r XTRIM x MAXLEN 0 + set reply [r XINFO STREAM x FULL] + set group [lindex [dict get $reply groups] 0] + assert_equal [dict get $group lag] 0 + } + test {Consumer group lag with XDELs} { r DEL x r XADD x 1-0 data a @@ -1342,7 +1374,7 @@ start_server { set reply [r XINFO STREAM x FULL] set group [lindex [dict get $reply groups] 0] assert_equal [dict get $group entries-read] 5 - assert_equal [dict get $group lag] 2 + assert_equal [dict get $group lag] 1 set group [lindex [dict get $reply groups] 1] assert_equal [dict get $group entries-read] {} assert_equal [dict get $group lag] 1 @@ -1405,7 +1437,7 @@ start_server { set reply [r XINFO STREAM x FULL] set group [lindex [dict get $reply groups] 0] assert_equal [dict get $group entries-read] 5 - assert_equal [dict get $group lag] 3 + assert_equal [dict get $group lag] 2 set group [lindex [dict get $reply groups] 1] assert_equal [dict get $group entries-read] {} assert_equal [dict get $group lag] 2 diff --git a/tests/unit/type/stream.tcl b/tests/unit/type/stream.tcl index ae29249f874..b76866da7b9 100644 --- a/tests/unit/type/stream.tcl +++ b/tests/unit/type/stream.tcl @@ -2033,6 +2033,45 @@ start_server {tags {"stream"}} { r XADD x 1-18446744073709551615 f1 v1 assert_error {*The ID specified in XADD is equal or smaller*} {r XADD x 1-* f2 v2} } + + test {XADD across macro nodes persists every entry (stream-node-max-bytes)} { + # Regression test for a use-after-free in streamAppendItem() when a new + # macro node's listpack is reallocated to the address just freed by the + # previous node's lpShrinkToFit(). The specific field payload sizes + # below and the XADD sequence drive jemalloc into that address reuse; + # without this exact shape the aliasing does not occur and the test + # silently loses coverage. The bug is allocator-agnostic, but this + # shape only reliably reproduces it under jemalloc. + r DEL k + set a100 [string repeat a 100] + set a1200 [string repeat a 1200] + set fields [list f1 $a100 f2 $a1200 f3 aaaa f4 a f5 $a1200 f6 $a1200] + + r XADD k 2-2 f1 gd f2 bmljhyqvch + assert_equal {2-2} [lmap e [dict get [r XINFO STREAM k FULL] entries] {lindex $e 0}] + + set saved_max_bytes [config_get_set stream-node-max-bytes 1] + r XADD k 2-3 {*}$fields + assert_equal {2-2 2-3} [lmap e [dict get [r XINFO STREAM k FULL] entries] {lindex $e 0}] + assert_equal {2-2 2-3} [lmap e [r XRANGE k - +] {lindex $e 0}] + + r CONFIG SET stream-node-max-bytes 4096 + r XADD k 2-4 {*}$fields + assert_equal {2-2 2-3 2-4} [lmap e [dict get [r XINFO STREAM k FULL] entries] {lindex $e 0}] + assert_equal {2-2 2-3 2-4} [lmap e [r XRANGE k - +] {lindex $e 0}] + + r XADD k 2-5 {*}$fields + assert_equal {2-2 2-3 2-4 2-5} [lmap e [dict get [r XINFO STREAM k FULL] entries] {lindex $e 0}] + assert_equal {2-2 2-3 2-4 2-5} [lmap e [r XRANGE k - +] {lindex $e 0}] + assert_equal {2-5 2-4 2-3 2-2} [lmap e [r XREVRANGE k + -] {lindex $e 0}] + + assert_equal 4 [r XLEN k] + + assert_equal {f1 gd f2 bmljhyqvch} [lindex [lindex [r XRANGE k 2-2 2-2] 0] 1] + assert_equal $fields [lindex [lindex [r XRANGE k 2-5 2-5] 0] 1] + + config_set stream-node-max-bytes $saved_max_bytes + } } start_server {tags {"stream needs:debug"} overrides {appendonly yes aof-use-rdb-preamble no}} {