Repository navigation
[backport] Backport sweep for 9.2 - #5308
valkeyrie-ops[bot] wants to merge 22 commits into
Conversation
![valkeyrie-bot[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Read-only list commands were fixed in #4782 and no longer need a special case. Signed-off-by: Jim Brunner <brunnerj@amazon.com> Backport-Source-PR: 5283
Replace uses of `lsort -stride` in the path hash tests with a helper compatible with Tcl 8.5. macOS ships Tcl 8.5, which does not support the `lsort -stride` option. [Failed CI](https://github.com/valkey-io/valkey/actions/runs/41744961340/job/126295526487) Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com> Backport-Source-PR: 5276
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
The test references `$quorum` without defining it locally. It can
therefore fail deterministically when the test file is run independently
instead of after another test that happens to initialize the variable.
This change derive the majority quorum from the configured Sentinel
instance count:
[llength $::sentinel_instances] / 2 + 1
This avoids a hard-coded quorum and keeps the test valid if the number
of Sentinel instances changes.
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Backport-Source-PR: 5275
This fixes the `SENTINEL SIMULATE-FAILURE valkey-cli hint` test by freeing the generated hint before exit, eliminating the deterministic 186-byte LeakSanitizer report. [Failed CI](https://github.com/valkey-io/valkey/actions/runs/41744961340/job/126295525902) Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com> Backport-Source-PR: 5274
With io-threads enabled, the test's first 'r ping' is read by an I/O thread in the same batch as the infinite eval_ro script. processClientsCommandsBatch executes the script first, which enters busy mode and loops in processEventsWhileBlocked; postponeClientRead stops offloading reads while blocked, so nothing ever re-drains the batch, the parsed PING is never executed, the Tcl client blocks forever waiting for its reply, and SCRIPT KILL is never sent. The replica log in the failed job shows busy mode entered and then total silence until the primary drops the link at repl-timeout. This is a timing assumption in a recently added test, not a server regression: the test assumes a command sent concurrently with a busy script always gets a -BUSY reply, which io-threads batching does not guarantee. The repository's own stable busy-script test (functions.tcl) waits for the 'Slow script detected' log message before contacting the server; once busy mode is active, reads are processed inline on the main thread and answered with -BUSY. Applying the same wait here removes the race without weakening any assertion or changing product behavior, which fits the backport-branch policy of a minimal mechanical fix.
|
Automatic follow-up on this backport's CI. Pushed What failed: [TIMEOUT]: Busy script ending while the replica's postponed primary is paused in tests/unit/pause.tcl (job test-ubuntu-tls-io-threads) Root cause: With io-threads enabled, the test's first 'r ping' is read by an I/O thread in the same batch as the infinite eval_ro script. processClientsCommandsBatch executes the script first, which enters busy mode and loops in processEventsWhileBlocked; postponeClientRead stops offloading reads while blocked, so nothing ever re-drains the batch, the parsed PING is never executed, the Tcl client blocks forever waiting for its reply, and SCRIPT KILL is never sent. The replica log in the failed job shows busy mode entered and then total silence until the primary drops the link at repl-timeout. The failure is flaky. The fix: This is a timing assumption in a recently added test, not a server regression: the test assumes a command sent concurrently with a busy script always gets a -BUSY reply, which io-threads batching does not guarantee. The repository's own stable busy-script test (functions.tcl) waits for the 'Slow script detected' log message before contacting the server; once busy mode is active, reads are processed inline on the main thread and answered with -BUSY. Applying the same wait here removes the race without weakening any assertion or changing product behavior, which fits the backport-branch policy of a minimal mechanical fix. What the bot checked
The agent's own notesThe wait uses helpers that exist on this branch (count_log_lines, wait_for_log_messages in tests/support/util.tcl) with the same 1000x10ms bounds as functions.tcl:1428. The subsequent wait_for_condition on 'r ping' failing is kept, so the test still asserts the -BUSY behavior before SCRIPT KILL. This PR's own CI decides whether the fix holds, and the bot never merges. |
Fixes #4705. ### Summary `XADD` can leave a stream's radix tree holding a **dangling pointer to a freed listpack**. When the entry is later read (`XRANGE`, `XREVRANGE`, `XINFO STREAM ... FULL`), we hit UB. ### Root cause In `streamAppendItem()`, when a `XADD` cannot append to the current tail node, the tail node is shrunk to fit, that in turn might relocate it to a different location in memory, while `ri.data` is kept pointing to the old dangling pointer. ``` if (new_node) { lp = lpShrinkToFit(lp); <------ lp is moved to a new location in memory if (ri.data != lp) raxInsert(s->rax, ri.key, ri.key_len, lp, NULL); lp = NULL; <----- ri.data is still pointing to the old dangling pointer } ``` After which a new node is created & is inserted. ``` lp = lpNew(prealloc); .... raxInsert(s->rax, (unsigned char *)&rax_key, sizeof(rax_key), lp, NULL); <---- lp is now pointing to a new location different from ri.data (will later dangle) ``` Following that multiple appends happen on that node, that operation might reallocate it & it's possible that the final allocation might just happen to reallocate to the same location as the original node `ri.data`, I.E. `ri.data == lp` Due to this unfortunate allocation, the radix tree will never be updated with the latest values leaving it with a dangling pointer ``` if (ri.data != lp) raxInsert(s->rax, (unsigned char *)&rax_key, sizeof(rax_key), lp, NULL); <---- This check fails as ri.data == lp ``` ### Fix - A new `rax_lp` variable is now used track the need to update the radix tree along side the origin `lp` - remove `raxInsert` when a new node is created to avoid sending a pointer to the radix tree that might later get invalidated ### Testing Added an integration test in `tests/unit/type/stream.tcl` that reliably fails with jemalloc without the fix --------- Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com> Backport-Source-PR: 4711
A cluster bus peer can send a header claiming a packet of up to 4GB, and the node keeps the link open and grows its receive buffer until the whole body arrives, because the header's length is only checked against a minimum. This rejects packets over 16MB as soon as the header is read, on both the main-thread and IO-thread read paths, for every type except PUBLISH, PUBLISHSHARD and MODULE, whose client-supplied payloads can legitimately be larger. Reported by Petr Khartskhaev. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Backport-Source-PR: 5065
`n` (new key events) is not part of `A`, but `keyspaceEventsFlagsToString()` only prints `n` when `A` is not set. So: ``` CONFIG SET notify-keyspace-events KEAn CONFIG GET notify-keyspace-events -> "AKE" ``` New key events still fire at runtime, but `CONFIG REWRITE` saves `"AKE"`, so after a restart they silently stop. The fix prints `n` outside the `A` branch, the same way `m` is handled. `KEAn` now reads back as `AnKE`. Added the case to the existing CONFIG GET/SET flags test. Signed-off-by: Aman Kumar <amankumar020003@gmail.com> Backport-Source-PR: 5199
#4983) Fixes [#4979 ](#4979) `XINFO GROUPS` / `XINFO STREAM ... FULL` reported a `lag` that was too high after `XTRIM` (or `XADD` with a trimming option) removed entries the group had not read yet. `streamReplyWithCGLag` trusts the group's `entries-read` counter when no tombstone sits after the group's last ID. `XTRIM` does not update `max_deleted_entry_id`, so after a trim Valkey believed nothing was deleted and computed `entries_added - entries_read`, which counts the trimmed entries as still pending. If the group's last ID is before the stream's first entry and no tombstone follows it, everything left in the stream is unread, so the lag is now the stream length. Example (6 entries, group has read 0-1, then `XTRIM s MAXLEN 2`): | | Before | After | |---|---|---| | `lag` | 5 | 2 | Tests: - Added "Consumer group lag after XTRIM removes unread entries" to `tests/unit/type/stream-cgroups.tcl`. - Corrected the expected lag in "Consumer group lag with XTRIM" (2 to 1) and "Consumer group lag with XADD trimming" (3 to 2). Both trim an unread entry and asserted the old, incorrect value. --------- Signed-off-by: Sasidharan Gopal <sasidharan.gopal94@gmail.com> Backport-Source-PR: 4983
Ensure bgiteration queue doesn't adjust downwards in the presence of replication. Signed-off-by: Muhammad Taha <76549451+taham0@users.noreply.github.com> Backport-Source-PR: 5298
### Problem If the previous "GEOSEARCH repeated BYPOLYGON uses the last polygon" test failed, it would have left `points` with some data, which might cause a cascading failure. ### Fix Clear `points` before using it Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com> Backport-Source-PR: 5293
…#4854) ### Problem Currently the "client evicted due to client tracking prefixes" test is very slow, & can take +14 minutes under valgrind, in some cases I've observed it taking +20 minutes (1230689ms) by itself. This happens due the checking done in `checkPrefixCollisionsOrReply` which checks every new prefix against every existing prefix, this in practice costs us to hit a quadratic time complexity. Reducing the `maxmemory-clients` isn't a viable option, as it has can't go below 128KB (getClientEvictionLimit) ### Fix The maximum memory is shrunk down by padding the client with various pubsub channel names that are accounted for in the max memory checks This lowers the number of needed prefixes to hit the eviction limit to a couple of thousands rather than 100s of thousands This in turn reduces the time it takes the test to complete to under 10s under valgrind. --------- Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com> Backport-Source-PR: 4854
Correct the `CLUSTER GETSLOTMIGRATIONS` reply schema to match the fields already returned by the server. This change fixes two inconsistencies in `src/commands/cluster-getslotmigrations.json`: - Renames `link_name` to `name`, matching the response generated by `clusterCommandGetSlotMigrations()`. - Adds the missing `remaining_repl_size` property, which was introduced in #3135. Signed-off-by: SHUVA JYOTI KAR <shuva.jyoti.kar.87@gmail.com> Backport-Source-PR: 5251
The module API reference is generated from the doc comments in `src/module.c` by `utils/generate-module-api-doc.rb`, so constants defined only in `valkeymodule.h` never appear in it. When regenerating from 9.2.0-rc2, `VALKEYMODULE_KEYTYPE_PATH_HASH` and `VALKEYMODULE_NOTIFY_PATH_HASH` are missing. This PR updates the doc comments only, with no code changes: - `VM_KeyType`: list all possible return values, including `VALKEYMODULE_KEYTYPE_PATH_HASH`. - `VM_SubscribeToKeyspaceEvents`: - Add `VALKEYMODULE_NOTIFY_PATH_HASH`. - Also add `VALKEYMODULE_NOTIFY_NEW`, which was missing from the same list. - Fix `VALKEYMODULE_NOTIFY_KEYMISS` to the actual macro name `VALKEYMODULE_NOTIFY_KEY_MISS`. - Note that `VALKEYMODULE_NOTIFY_ALL` also excludes `VALKEYMODULE_NOTIFY_NEW`, matching `valkey.conf`. Refs : valkey-io/valkey-doc#531 Signed-off-by: Su Ko <rhtn1128@gmail.com> Backport-Source-PR: 5289
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
`XX` was added in #2972, which first shipped in 9.2.0-rc1. --------- Signed-off-by: 李凝炜 <951448078@qq.com> Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Backport-Source-PR: 5310
Ensures that bgIteration isn't short-circuited by hasPostExecutionUnitPendingWork Signed-off-by: harrylin98 <harrylin980107@gmail.com> Backport-Source-PR: 5316
Stop background write generators before comparing replication offsets and database digests. --------- Signed-off-by: Nitai Caro <caronita@amazon.com> Co-authored-by: Nitai Caro <caronita@amazon.com> Backport-Source-PR: 5307
|
Automatic follow-up on this backport's CI. No fix for What failed: tests/unit/cluster/slot-migration.tcl: 'Empty-shard migration target is auto-updated after failover in target shard' and 'Empty-shard migration source is auto-updated after failover in source shard' in job test-ubuntu-tls-no-tls Root cause: A rare cluster-state propagation race: after failback of R6 in the empty target shard, observer replica R3 never updated migrating_slots_to from R7 back to R6 (clusterUpdateSlotsConfigWith / role-switch paths in src/cluster_legacy.c), and the stale entry then made the second failover test fail when R3 became primary. The exact interleaving is not recoverable from the CI log, which lacks the server logs. The briefing's reproduction instead fails deterministically because --only skips the prerequisite test that opens the migration, leaving R0 with an empty slot state, which is an artifact of isolated selection, not the CI race. What the bot checked
|
The test asserts the literal errno string 'Connection refused', but on the macOS arm64 runner the libvalkey non-blocking connect race makes valkey-cli report the refused connection as 'setsockopt(TCP_NODELAY): Invalid argument' (deps/libvalkey/src/net.c blocking path sets TCP_NODELAY after SO_ERROR reads 0), so the assert_match at tests/unit/cluster/cli.tcl:649 fails. A secondary test bug: the final cluster_known_nodes==1 checks depend on the previous del-node test having run, so the test also fails when run alone with --only. The server and CLI behaved correctly; the test encodes an OS- and timing-specific error string the platform does not guarantee, so the fix belongs in the test. Matching the stable 'Could not connect to Valkey at' prefix keeps the intent (CLI warned it could not reach the dead node) without depending on errno text, and checking the node ID is gone from remaining members keeps the intent of the state check while making the test order-independent per the repo's own test standards. Test-only, minimal, and backport-friendly; no product behavior changes on this release branch, and no merged upstream commit addressing this failure was found to port.
|
Automatic follow-up on this backport's CI. Pushed What failed: del-node: Delete unreachable node without slots in tests/unit/cluster/cli.tcl (job test-macos-latest) Root cause: The test asserts the literal errno string 'Connection refused', but on the macOS arm64 runner the libvalkey non-blocking connect race makes valkey-cli report the refused connection as 'setsockopt(TCP_NODELAY): Invalid argument' (deps/libvalkey/src/net.c blocking path sets TCP_NODELAY after SO_ERROR reads 0), so the assert_match at tests/unit/cluster/cli.tcl:649 fails. A secondary test bug: the final cluster_known_nodes==1 checks depend on the previous del-node test having run, so the test also fails when run alone with --only. The failure is flaky. The fix: The server and CLI behaved correctly; the test encodes an OS- and timing-specific error string the platform does not guarantee, so the fix belongs in the test. Matching the stable 'Could not connect to Valkey at' prefix keeps the intent (CLI warned it could not reach the dead node) without depending on errno text, and checking the node ID is gone from remaining members keeps the intent of the state check while making the test order-independent per the repo's own test standards. Test-only, minimal, and backport-friendly; no product behavior changes on this release branch, and no merged upstream commit addressing this failure was found to port. What the bot checked
The agent's own notesNote for the reviewer: the bot's reproduction uses --only, which skips the two earlier del-node tests; the original exact known-nodes counts can never be satisfied in that mode, which is why reproduction failed with 'Nodes don't have expected cluster state' rather than the CI assertion. Both the CI failure (errno string) and the standalone failure (count assumption) are addressed. This PR's own CI decides whether the fix holds, and the bot never merges. |
…nel test (#5235) Fixes #4483 ### Problem The previous test, "ODOWN is not possible without N (quorum) Sentinels reports", kills the primary and restarts it. On a slow CI machine this can take a few seconds, so the Sentinels get close to `down-after-milliseconds` (2 s) before the primary is back. As such when the "Failover is not possible without majority agreement" test starts, every Sentinel still shows the primary as `master,disconnected`. The test lowers the quorum to 3 immediately. A Sentinel then crosses `down-after`, reaches ODOWN and starts a failover while all five Sentinels are still alive to vote. If the promotion finishes before the test's read, the surviving Sentinel reports the new primary and the assertion fails. ### Out of Scope A server bug makes the failover instant: since #2763, peer `master_down` reports are never expired while a Sentinel sees the primary as up (#4912, fixed by #4913). # Fix Before lowering the quorum, wait until every Sentinel sees the primary as plain `master` (connected) with a PONG in the last second. This is the same idea as `ensure_master_up` in `07-down-conditions.tcl`. The wait is on state, not time, so a slower runner just waits longer. Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com> Backport-Source-PR: 5235
Multiple error paths in the geo commands are not covered, adding coverage for the following: - longitudes & latitudes ranges - Too many input arguments --------- Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com> Backport-Source-PR: 4793
A node only refreshes migrating/importing slot state when it sees a slots-bitmap change or a replica-to-primary role edge (gate at src/cluster_legacy.c:4708). An empty shard's bitmap never changes, and failback issued the manual failover as soon as FAIL flags cleared, which can happen via a stale pre-demotion PONG that still claims the primary role. A node that missed the demotion sees no role edge on the re-promotion and keeps its migrating destination pointed at the interim primary forever, exactly the stuck [609->-R7] state in the job log. This is a test synchronization bug, not a server regression: the server's state converged everywhere except for the edge-triggered open-slot update, and the already-ported deflake b55889b (#4795) fixed the FAIL-flag half of this same race in the same helper. Extending failback with a wait on the demoted primary's role view uses the repository's polling-helper convention, changes no product behavior, and removes the race deterministically. The --only guard makes the reproduction command meaningful without affecting full-file runs.
|
Automatic follow-up on this backport's CI. Pushed What failed: tests/unit/cluster/slot-migration.tcl: 'Empty-shard migration target is auto-updated after failover in target shard' and 'Empty-shard migration source is auto-updated after failover in source shard' in job test-almalinux9-jemalloc Root cause: A node only refreshes migrating/importing slot state when it sees a slots-bitmap change or a replica-to-primary role edge (gate at src/cluster_legacy.c:4708). An empty shard's bitmap never changes, and failback issued the manual failover as soon as FAIL flags cleared, which can happen via a stale pre-demotion PONG that still claims the primary role. A node that missed the demotion sees no role edge on the re-promotion and keeps its migrating destination pointed at the interim primary forever, exactly the stuck [609->-R7] state in the job log. The failure is flaky. The fix: This is a test synchronization bug, not a server regression: the server's state converged everywhere except for the edge-triggered open-slot update, and the already-ported deflake b55889b (#4795) fixed the FAIL-flag half of this same race in the same helper. Extending failback with a wait on the demoted primary's role view uses the repository's polling-helper convention, changes no product behavior, and removes the race deterministically. The --only guard makes the reproduction command meaningful without affecting full-file runs. What the bot checked
The agent's own notesThe bot's reproduction command uses --only for the two tests, which skips the preceding test that opens slot 609; without the added guard it fails trivially with an empty open-slots string (see reproduction.log line 43), which differs from the CI race. With the guard plus the failback wait, both the --only reproduction and the full-file ordering are covered. Note node_seen_as_replica_by_all uses the 'slave' flag as emitted by CLUSTER NODES. This PR's own CI decides whether the fix holds, and the bot never merges. |
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
GitHub retired the macOS 14 runner image, causing `build-old-macos-versions` to be cancelled before a runner is allocated: ```text The configuration 'macos-14-arm64-us-default' is not supported ``` Update the job to use `macos-15`, preserving older-version coverage without duplicating the existing `macos-latest` jobs. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com> Backport-Source-PR: 5315
|
Automatic follow-up on this backport's CI. No fix for |
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
Backport sweep for 9.2
Automated cherry-picks from PRs marked "To be backported".
Applied
sinceversion on SISMEMBER XX to 9.2.0Skipped
These candidates were evaluated but contribute no change to this branch, e.g. the fix targets code that does not exist here. No backport commit was created for them.
Generated by valkey-ci-agent using Claude Code.