Repository navigation
[backport] Backport sweep for 9.1 - #5074
valkeyrie-ops[bot] wants to merge 23 commits into
Conversation
![valkeyrie-bot[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…rag (#4968) Clear `defrag_later` (and reset its cursor) at the start of every DbKeys defrag stage Signed-off-by: Muhammad Taha <76549451+taham0@users.noreply.github.com> Backport-Source-PR: 4968
moduleBlockClient() treated every subscribed client as unblockable because getClientType() returns CLIENT_TYPE_PUBSUB. A RESP3 client can still run commands while subscribed, so ValkeyModule_BlockClient and ValkeyModule_BlockClientOnAuth returned NULL with ENOTSUP and queued no reply. Allow blocking those clients again, matching the processCommand rule that only RESP2 Pub/Sub clients are restricted. Keyspace notifications still cannot block them. Introduced by PR 1819 (8.1.1). Restores the pre-8.1.1 behavior for RESP3 subscribed clients; no change for RESP2 clients or keyspace-notification contexts. Fixes #4862 --------- Signed-off-by: quanyeyang <quanyemostima@gmail.com> Signed-off-by: Rain Valentine <rsg000@gmail.com> Co-authored-by: Rain Valentine <rsg000@gmail.com> Co-authored-by: Roshan Khatri <117414976+roshkhatri@users.noreply.github.com> Backport-Source-PR: 4863
valkeyrie-ops
Bot
added
ai-resolved-conflicts
AI backport resolution: source PR #4968Fix crash when a database's keyspace is replaced mid-stage during defrag 1 AI-edited file Claude Summary
AI-edited files requiring review
Full backport commit diff: commit b779efc567fe.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 9.1 #5074 +/- ##
==========================================
+ Coverage 76.98% 77.39% +0.40%
==========================================
Files 165 166 +1
Lines 82092 82730 +638
==========================================
+ Hits 63201 64029 +828
+ Misses 18891 18701 -190
🚀 New features to boost your workflow:
|
|
Automatic follow-up for the current backport head. I did not push a fix: This is a flaky, allocator/heap-state-dependent failure, not a deterministic breakage attributable to the backport: it failed only in one of 30+ jobs, including passing in the identical clang variant of the same sanitizer job. No default-branch candidate commit touches tests/unit/hashexpire.tcl or the hash TTL/RESTORE memory path, so there is nothing to port, and the only 'author' option would be to weaken or remove the memory-equality assertion the test exists to verify, which is prohibited. Looked at the failure from this run. |
Limits time evicting from tracking table. Signed-off-by: Tanmay <tanmaymohan12@gmail.com> Backport-Source-PR: 4775
AI backport resolution: source PR #4190Reclaim dead client IDs from the tracking table (#4143) 1 AI-edited file Claude Summary
AI-edited files requiring review
Full backport commit diff: commit c619f8eb1361.
|
|
Automatic follow-up on this backport's CI. The fix attempt for |
1 similar comment
|
Automatic follow-up on this backport's CI. The fix attempt for |
…gtest (#3683) release.c is compiled into valkey-cli, valkey-benchmark, and the valkeylib-gtest static library, but only valkey-server declared a dependency on the release_header custom target that runs mkreleasehdr.sh to generate src/release.h. On a clean build (no stale release.h on disk) or under high parallelism, those targets could attempt to compile release.c before the header was generated and fail with "release.h: No such file or directory". Add the missing add_dependencies(... release_header) for all three. Signed-off-by: Vadym Khoptynets <1099644+poiuj@users.noreply.github.com> Backport-Source-PR: 3683
Repeated `BYPOLYGON` options in `GEOSEARCH` and `GEOSEARCHSTORE` overwrite the previous vertex array without freeing it. Only the final array is released, leaking memory even when the source key does not exist. Free the previous vertices before allocating a replacement, preserving the existing behavior of using the last polygon. Add regression tests for both commands, invalid `COUNT` cleanup, and retained memory across repeated requests. is merged first, I'll rebase this PR and use the new name. Validation: - GEO suite: 75 passed. - New tests with 4 I/O threads and `io-threads-always-active yes`: 2 passed. - The leak regression fails against the parent commit and passes with this fix. Signed-off-by: DaeMyung Kang <charsyam@gmail.com> Backport-Source-PR: 4995
AI backport resolution: source PR #4995Fix GEOSEARCH repeated BYPOLYGON memory leak 1 AI-edited file Claude Summary
AI-edited files requiring review
Full backport commit diff: commit 1b7c5ccda267.
|
|
Automatic follow-up on this backport's CI. The fix attempt for |
|
Automatic follow-up on this backport's CI. The fix attempt for |
|
Automatic follow-up on this backport's CI. The fix attempt for |
For large key lists, `ValkeyModule_GetCommandKeysWithFlags` allocates temporary storage in `getKeysResult` and returns without freeing it. This adds the missing cleanup after copying the positions and flags into the returned arrays. Fixes #5042 Signed-off-by: Christopher Lim <41635847+christopherlim98@users.noreply.github.com> Backport-Source-PR: 5046
`objectComputeSize()` samples a hashtable-encoded hash with a validating iterator, which skips fields whose TTL has passed but that have not been reaped yet. Those fields still occupy memory, and `hashtableSize()`, which the sampled average is multiplied by, still counts them. The result is under-reported whenever expired fields are larger than live ones. **Example:** one small live field plus 99 expired 1 KB fields, with active expiry disabled. `MEMORY USAGE` reports ~4 KB for a hash that holds ~100 KB. **Fix:** iterate with `HASHTABLE_ITER_SKIP_VALIDATION`. This matches the rest of `objectComputeSize()`, where the listpack branch uses `zmalloc_size()` and sets and sorted sets have no validator, and it matches `rdbSaveObject()`, which already skips validation when walking the hash. It also removes a dependency on expiry evaluation, which reads server state a size estimate should not need. Once expired fields are reaped the reported value is unchanged. Existing `hashexpire.tcl` tests that check memory drops after expiry wait for reaping first, so they are unaffected. Adds a regression test to `tests/unit/hashexpire.tcl`. Signed-off-by: Sushil Paneru <sushil.paneru1@gmail.com> Backport-Source-PR: 5092
AI backport resolution: source PR #5092Count expired but unreaped hash fields in objectComputeSize 1 AI-edited file Claude Summary
AI-edited files requiring review
Full backport commit diff: commit 2a9680219c6f.
|
|
Automatic follow-up on this backport's CI. The fix attempt for |
|
Automatic follow-up on this backport's CI. The fix attempt for |
|
Automatic follow-up on this backport's CI. The fix attempt for |
Calling `ValkeyModule_Call` with `S` but without `E` crashes on validation errors because cleanup calls `sdslen()` on a NULL error message. Use a generic ERR message for error accounting when no reply message was requested. --------- Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com> Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Backport-Source-PR: 5091
dictSdsKeyCompare() and evictionPoolPopulate() stored sdslen() in an int. At 2^31 bytes that is negative. dictSdsKeyCompare() then passes a sign-extended length to memcmp(), which reads past the end of both strings and can report two equal strings as different, so a set or hash stores the same member twice, or crash. evictionPoolPopulate() takes the short-key branch and memcpy()s a huge length into the 255 byte cached sds, crashing the server. dictSdsKeyCompare() became the comparator for every data hashtable in fb655db (#3366), replacing hashtableSdsKeyCompare(), which compared size_t lengths. The eviction pool narrowing is older. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Backport-Source-PR: 5133
|
Automatic follow-up on this backport's CI. The fix attempt for |
|
Automatic follow-up on this backport's CI. The fix attempt for |
|
Automatic follow-up on this backport's CI. The fix attempt for |
The command metadata describes several sorted-set numeric arguments as integers even though the command implementations accept floating-point values: - The `ZINCRBY` increment. - `ZINTER` and `ZINTERSTORE` weights. - `ZUNION` and `ZUNIONSTORE` weights. This prevents metadata consumers from accurately representing valid command arguments. Credits to @madolson for finding this issue. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com> Backport-Source-PR: 5210
PFADD with several elements applies them to the HLL in place, one at a time. On a malformed sparse HLL, an early element can modify the key and a later one can then fail with INVALIDOBJ. When that happened, PFADD returned before `signalModifiedKey()`, the keyspace event and the `server.dirty` update. The modified value stayed on the primary and never reached replicas or the AOF, and it did not trip WATCH or client tracking. This change finishes the bookkeeping for the elements that were applied, and propagates a PFADD that carries only those elements. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Backport-Source-PR: 5128
|
Automatic follow-up on this backport's CI. The fix attempt for |
|
Automatic follow-up on this backport's CI. The fix attempt for |
|
Automatic follow-up on this backport's CI. No fix for |
|
Automatic follow-up on this backport's CI. The fix attempt for |
In cluster mode, one script that calls a cross-slot command (for example `MGET` on keys in two slots) makes every later `server.call()` from any script or function fail with "Attempted to access a non local key in a cluster node" until the server restarts. That includes keyless commands like `PING`. `clusterSlotByCommand()` only ORs `READ_FLAGS_CROSSSLOT` into the flags it is given, and the pooled client that `ValkeyModule_Call()` uses never resets them between calls. The bit stays set, and `getNodeByQuery()` rejects everything sent through that client. The fix clears both result flags at the start of `clusterSlotByCommand()`. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Backport-Source-PR: 5191
… in SYNC mode (#5127) `readReadCommandInternal()` in `src/scripting_engine.c` asserts `*argc == 0` on entry (`scripting_engine.c:749`). When a SCRIPT DEBUG SYNC command spans more than one read, the function parses the `*<n>\r\n` header, writes `*argc = n`, and then exits through `keep_reading` because the rest of the command has not arrived yet. The caller (`scriptingEngineDebuggerProcessCommands()`) reads at most 1024 bytes per `connRead()` call, appends to `ds.cbuf`, and retries — but `argc` still holds `n` from the first call, so the assert fires on the second. In SYNC mode the assert runs in the server process, which exits with SIGABRT. Any debugger command longer than 1024 bytes triggers this; TCP framing can split shorter commands too. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Backport-Source-PR: 5127
|
Automatic follow-up on this backport's CI. The fix attempt for |
|
Automatic follow-up on this backport's CI. The fix attempt for |
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
The `numfields` argument in several hash-field commands incorrectly has `key_spec_index: 0`. `numfields` is a count, not a key. Associating it with a key specification produces incorrect command introspection data and can mislead clients that consume the command metadata. Credits to @madolson for finding this issue. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com> Backport-Source-PR: 5211
) The command metadata currently places `NX`, `XX`, `GT`, and `LT` in a flat `oneof`. That incorrectly describes all four options as mutually exclusive. While `NX` is exclusive, `XX` can validly be combined with either `GT` or `LT`. The inaccurate structure affects `COMMAND DOCS` and clients generated from the command metadata. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com> Backport-Source-PR: 5213
AI backport resolution: source PR #5211Remove key specs from hash NUMFIELDS arguments 1 AI-edited file Claude Summary
AI-edited files requiring review
Full backport commit diff: commit 4a5544688451.
|
|
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
|
A script that returns or raises an error table whose metatable has a raising `__index` makes the server exit with `PANIC: unprotected error in call to Lua API`. `luaExtractErrorInformation()` reads the table's fields with `lua_getfield()`, which runs `__index`, and it does so after the script's `lua_pcall()` has returned, so the error has nowhere to unwind to. This switches those four reads to `lua_rawget()`, the same fix redis/redis#11032 applied to `luaReplyToServerReply()`. Every Valkey release is affected. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Backport-Source-PR: 5118
A module that calls `ValkeyModule_ReplyWithArray(ctx, VALKEYMODULE_POSTPONED_LEN)` (or Map, Set, Attribute) and never sets the length crashes the server with SIGSEGV. `moduleFreeContext()` already detects this and logs an API misuse warning, but it leaves the NULL placeholder node in the reply list, and the write path dereferences it. The fix fills each leftover placeholder with an empty array header, the same call `ValkeyModule_ReplySetArrayLength()` makes. For a thread safe context on a blocked client, this happens on the main thread when the client is unblocked, since the context may only be freed later. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Backport-Source-PR: 5159
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
Backport-Source-PR: 3510
…4091) When loading nodes.conf, if a replica entry references a primary that has no separate entry in the file, the primary is implicitly created with flags=0 (noflags). ``` /* Get primary if any. Set the primary and populate primary's * replica list. */ if (argv[3][0] != '-') { ... primary = clusterLookupNode(argv[3], sdslen(argv[3])); if (!primary) { primary = createClusterNode(argv[3], 0); clusterAddNode(primary); } ``` Once created this way, the node's role can never be recovered: the role-switch logic only triggers transitions between known roles (replica->primary or primary->replica), so a noflags node is stuck indefinitely. This leads to: - serverAssert(nodeIsPrimary(sender)) crash when the noflags node claims to be a primary and the slot-update path is reached. It should have been working correctly before #2337, because we always call `clusterSetNodeAsPrimary`. - Inconsistent local view where a replica has replicaof set but no REPLICA flag, breaking failover candidate selection and CLUSTER NODES output. This also happens when a noflags entry is directly present in nodes.conf (e.g. manual edit or corruption). The node role will not be able to recover correctly from noflags. Fix by adding fallback branches: if the sender has no role locally, adopt its advertised role (primary or replica) on the first heartbeat. Signed-off-by: Binbin <binloveplay1314@qq.com> Backport-Source-PR: 4091
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
The test introduced by fbfe087 asserts '*Connection refused*' in valkey-cli stderr, but on macOS hiredis reports the connection failure to the shut-down node as 'setsockopt(TCP_NODELAY): Invalid argument', so the assertion fails on macOS runners. Additionally, the test's final wait asserted cluster_known_nodes == 1 on nodes 0 and 1, which depends on the preceding 'Delete reachable node' test having removed node 1 and therefore fails when the test is run in isolation with --only. Both failures are test bugs, not server misbehavior: an OS-dependent error string and an inter-test state dependency. The fix keeps all the meaningful assertions (command reports success, CLUSTER RESET warning is printed, a connect error occurred, and the node is actually forgotten by every live cluster member via the existing cluster_get_node_by_id helper inside wait_for_condition), so nothing the test guarantees is weakened, and it uses polling helpers already defined on this branch. No merged upstream commit fixes this assertion, so a minimal test-only patch is appropriate for the release branch.
|
Automatic follow-up on this backport's CI. Pushed What failed: del-node: Delete unreachable node without slots in tests/unit/cluster/cli.tcl (test-macos-latest) Root cause: The test introduced by fbfe087 asserts 'Connection refused' in valkey-cli stderr, but on macOS hiredis reports the connection failure to the shut-down node as 'setsockopt(TCP_NODELAY): Invalid argument', so the assertion fails on macOS runners. Additionally, the test's final wait asserted cluster_known_nodes == 1 on nodes 0 and 1, which depends on the preceding 'Delete reachable node' test having removed node 1 and therefore fails when the test is run in isolation with --only. The fix: Both failures are test bugs, not server misbehavior: an OS-dependent error string and an inter-test state dependency. The fix keeps all the meaningful assertions (command reports success, CLUSTER RESET warning is printed, a connect error occurred, and the node is actually forgotten by every live cluster member via the existing cluster_get_node_by_id helper inside wait_for_condition), so nothing the test guarantees is weakened, and it uses polling helpers already defined on this branch. No merged upstream commit fixes this assertion, so a minimal test-only patch is appropriate for the release branch. What the bot checked
The agent's own notesThe previous iteration already proved the relaxed connect-error match works on macOS (the run got past the assertion and failed only at the cluster-state wait). The new final check passes trivially in isolation because valkey-cli sends CLUSTER FORGET to all reachable nodes before printing OK, and in a full-file run node 1 is standalone after CLUSTER RESET SOFT and never re-learns the deleted ID thanks to the forget blacklist. 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
|
…ed (#4351) Fixes #4349 When every time event in an event loop is pending deletion (`id == AE_DELETED_EVENT_ID`, a "zombie-only" list), the rescan in `usUntilEarliestTimer()` finds no valid timer, `earliest` stays `NULL`, and the function dereferences it. The empty-list case is guarded; the all-zombies case is not. The state is reachable whenever the last timer of a loop fires `AE_NOMORE` (marked deleted in `processTimeEvents`, reaped only on the next pass) or is deleted between `processTimeEvents` runs, while the lookup runs before the reaping on every iteration. Redis fixed the identical bug in redis/redis#15391; this applies the same guard. Signed-off-by: Haoze Xie <royenheart@outlook.com> Signed-off-by: Yuan Tan <yuantan098@gmail.com> Backport-Source-PR: 4351
The broken-link detection in clusterCron only reconnects a link when no traffic is seen for more than half the node timeout (data_delay > cluster_node_timeout/2). This leaves ping_sent unable to advance in this case: while ping_sent is set we never send another PING (clusterCron skips nodes with an outstanding ping), and ping_sent is only cleared by a PONG. If the peer keeps the link alive with its own PINGs (refreshing the data_received) while our outgoing PING was lost, the link is never reconnected, so ping_sent stays set forever and we can not send PING. Force a reconnect when our PING has been outstanding for a full node timeout without a PONG, independent of data_received. The reconnect itself sends a fresh PING, letting us receive a PONG and clear the stale state. A practical example is a two-node cluster: with no third node to confirm or correct the failure, a lost PING combined with the peer's own PINGs leaves the node permanently stuck in PFAIL state. Signed-off-by: Binbin <binloveplay1314@qq.com> Backport-Source-PR: 4171
|
Automatic follow-up on this backport's CI. No fix for What the bot checked
|
Backport sweep for 9.1
Automated cherry-picks from PRs marked "To be backported".
Applied
AI resolution details are posted as comments on this PR when available.
Needs attention
These candidates could not be applied automatically and need a maintainer to follow up.
1 candidate(s)
Generated by valkey-ci-agent using Claude Code.