Merge remote-tracking branch 'upstream/unstable' into forkless - #4422
Open
nitaicaro wants to merge 285 commits into
Open
Merge remote-tracking branch 'upstream/unstable' into forkless#4422nitaicaro wants to merge 285 commits into
nitaicaro wants to merge 285 commits into
Conversation
Remove eval script cache entries that belong to a scripting engine when that engine is unregistered. This prevents the eval cache from retaining dangling engine pointers and keeps the tracked script memory in sync after engine shutdown. The scripting engine unregister path now invokes a new eval cleanup helper, which scans the cached scripts, drops matching entries from the LRU list and dictionary, and adjusts cache memory accounting accordingly. * scripting engine * eval cache Signed-off-by: Eran Ifrah <eifrah@amazon.com>
…egacy files (valkey-io#2297) (valkey-io#3382) Migrated the remaining cluster tests to tests/unit/cluster/ to use the same framework for all cluster tests. Cleaned up the obsolete cluster test framework files and updated the CI workflows to use the new unified test runner. Changes: Moved and mapped 6 test files: - 03-failover-loop.tcl → Merged into existing failover.tcl - 04-resharding.tcl → resharding.tcl - 12-replica-migration-2.tcl + 12.1-replica-migration-3.tcl → replica-migration-slow.tcl - 07-replica-migration.tcl → Merged into existing replica-migration.tcl - 28-cluster-shards.tcl → Merged into existing cluster-shards.tcl Other changes: - Converted old framework APIs (e.g., K, RI) to new framework APIs (e.g., R, srv) - Added process_is_alive check in cluster_util.tcl to fix an exception in failover tests caused by executing ps on dead processes - Heavy tests (resharding, replica-migration-slow) marked with slow tag and wrapped in run_solo to prevent resource contention in sanitizer environments - replica-migration-slow marked with valgrind:skip tag since it is very slow - Removed the entire tests/cluster/ directory including run.tcl, cluster.tcl, includes/, and helpers/ - Kept runtest-cluster as a wrapper script (exec ./runtest --cluster "$@") - Removed ./runtest-cluster calls from .github/workflows/daily.yml as cluster tests are now included in ./runtest Closes valkey-io#2297. Signed-off-by: Jun Yeong Kim <junyeonggim5@gmail.com> Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Binbin <binloveplay1314@qq.com>
) The argument `count /= 2` modifies `count` as a side effect, and the following `count /= 2` divides it again unnecessarily. Since `count` is not used after this point, fix it by using `count / 2` without the side effect and remove the redundant second assignment. Signed-off-by: djk1027 <djk9510271@gmail.com>
This change was introduced in valkey-io#3382. This test is already very slow on its own. Under valgrind it gets slow enough that the per-node restart step lets primaries be marked FAIL and triggers failovers, after which "Verify slaves consistency" no longer holds since it assumes the original topology. It was never run under valgrind before and exercises nothing valgrind meaningfully covers, so just tag it valgrind:skip. Signed-off-by: Binbin <binloveplay1314@qq.com>
I noticed this LTO compile warning in the eval code. Looks like it's
getting confused about an sds length, even though checked above. Just
added an assert to clarify.
The warning:
```c
LINK valkey-server
eval.c: In function ‘evalExtractShebangFlags’:
eval.c:263:27: warning: argument 1 value ‘18446744073709551615’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
*out_engine = zcalloc(engine_name_len + 1);
^
zmalloc.c:324:7: note: in a call to allocation function ‘valkey_calloc’ declared here
void *zcalloc(size_t size) {
^
```
Signed-off-by: Jim Brunner <brunnerj@amazon.com>
…y-io#2823) ## Background Add structured datasets loading capability. Support CSV and TSV file formats. Use `__field:fieldname__` placeholders to replace the corresponding fields from the dataset file. Support natural content size of varying length. Allow mixed placeholder usage combining dataset fields with random generators. Enable automatic field discovery from CSV/TSV headers. Use `--maxdocs` to limit the dataset loading. Rather than modifying the existing placeholder system, we detect field placeholders and switch to a separate code path that builds commands from scratch using `valkeyFormatCommandArgv()`. This ensures: - Zero impact on existing functionality - Full support for variable-size content - Thread-safe atomic record iteration - Compatible with pipelining and threading modes __Usage examples__ ```sh # Strings - Simple key-value with dataset fields ./valkey-benchmark --dataset products.csv -n 10000 SET product:__rand_int__ "__field:name__" # Sets - Unique collections from dataset ./valkey-benchmark --dataset categories.csv -n 10000 SADD tags:__rand_int__ "__field:category__" # CSV dataset with document limit ./valkey-benchmark --dataset wiki.csv --maxdocs 100000 -n 50000 HSET doc:__rand_int__ title "__field:title__" body "__field:abstract__" # Mixed placeholders (dataset + random) ./valkey-benchmark --dataset terms.csv -r 5000000 -n 50000 HSET search:__rand_int__ term "__field:term__" score __rand_1st__ ``` __Full-Text Search Benchmarking__ ```sh # Search hit scenarios (existing terms) ./valkey-benchmark --dataset search_terms.csv -n 50000 FT.SEARCH rd0 "__field:term__" # Search miss scenarios (non-existent terms) ./valkey-benchmark --dataset miss_terms.csv -n 50000 FT.SEARCH rd0 "__field:term__" # Query variations ./valkey-benchmark --dataset search_terms.csv -n 50000 FT.SEARCH rd0 "@title:__field:term__" ./valkey-benchmark --dataset search_terms.csv -n 50000 FT.SEARCH rd0 "__field:term__*" ``` __Benchmark Results__ Test environment: __Instance:__ AWS c7i.16xlarge, 64 vCPU Test Dataset: 5M+ Wikipedia XML documents, 5.8GB memory | Configuration | Throughput | CPU Usage | Wall Time | Memory Peak | |---------------|------------|-----------|-----------|-------------| | Single-threaded, P1 | 93,295 RPS | 99% | 71.4s | 5.8GB | | Multi-threaded (10), P1 | 93,332 RPS | 137% | 71.5s | 5.8GB | | Single-threaded, P10 | 274,499 RPS | 96% | 36.1s | 5.8GB | | Multi-threaded (4), P10 | 344,589 RPS | 161% | 32.4s | 5.8GB | --------- Signed-off-by: Ram Prasad Voleti <ramvolet@amazon.com> Co-authored-by: Ram Prasad Voleti <ramvolet@amazon.com>
Address this compile warning:
```c
CC util.o
util.c:638:1: warning: ‘no_sanitize’ attribute directive ignored [-Wattributes]
__attribute__((no_sanitize_address, no_sanitize("thread"), used)) static int (*string2ll_resolver(void))(const char *, size_t, long long *) {
^~~~~~~~~~~~~
```
Addresses portability concerns around these attributes.
---------
Signed-off-by: Jim Brunner <brunnerj@amazon.com>
Free BYPOLYGON points before returning from invalid COUNT parsing paths in GEOSEARCH/GEOSEARCHSTORE. Closes valkey-io#3567 --------- Signed-off-by: Su Ko <rhtn1128@gmail.com> Co-authored-by: Binbin <binloveplay1314@qq.com>
When read() returns 0 (EOF/connection closed) in syncRead(), errno is not set by POSIX, so it retains a stale value (typically 0). This causes callers using connGetLastError() to log strerror(0) which is the misleading string "Success". Set errno = ECONNRESET on EOF in syncRead(), matching the existing pattern used for the timeout case (errno = ETIMEDOUT). Also set conn->last_errno = errno in connSocketSyncWrite, connSocketSyncRead, and connSocketSyncReadLine wrappers, matching the pattern used by their async counterparts connSocketWrite and connSocketRead. After this fix, replica logs will show: "I/O error reading bulk count from PRIMARY: Connection reset by peer" instead of the misleading: "I/O error reading bulk count from PRIMARY: Success" --------- Signed-off-by: Abhishek Mathur <matshek@amazon.com> Signed-off-by: djk1027 <djk9510271@gmail.com> Co-authored-by: Abhishek Mathur <matshek@amazon.com> Co-authored-by: Daejun Kim <djk9510271@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
valkey-io#3586) clusterManagerNodePrimaryRandom() called srand(time(NULL)) on every invocation, then immediately rand() % primary_count. When called in a tight loop for uncovered slots, all calls within the same wall-clock second produce the identical seed, causing every uncovered slot to be assigned to the same primary node. Remove the srand() call since the PRNG is already seeded at startup (srand(time(NULL) ^ getpid()) at line 9838). This allows rand() to advance its state across calls, distributing uncovered slots randomly across available primaries. --------- Signed-off-by: Abhishek Mathur <matshek@amazon.com> Co-authored-by: Abhishek Mathur <matshek@amazon.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
…o#3598) In `getKeysUsingKeySpecs`, when extracting keys based on the `KSPEC_FK_KEYNUM `spec (like in the `EVAL` command), the server read the number of keys from the arguments and calculated the expected end index. However, it called `getKeysPrepareResult` to allocate memory for the result array before validating whether last was within the bounds of the actual arguments provided. If a client sent a command with a huge declared number of keys (e.g., `COMMAND GETKEYS EVAL "return 1" 2147483647 key1`), the server would allocate a massive amount of memory. Since `vm.overcommit_memory` is recommended, this allocation would NOT normally have triggered OOM (we never wrote to it so there is no physical memory allocated), but if you disable overcommit, this could trigger an OOM. You can reproduce it with: ``` $ prlimit --as=1073741824 src/valkey-server --save "" ... 384270:M 30 Apr 2026 04:27:24.456 * Ready to accept connections tcp ... <in valkey-cli> 127.0.0.1:6379> command getkeys eval "return 1" 2147483647 key1 ... <in server log> 384270:M 30 Apr 2026 04:29:26.950 # Out Of Memory allocating 17179869176 bytes! ``` ## Solution * Moved the bounds check `if (last >= argc || last < first || first >= argc)` to execute before the call to `getKeysPrepareResult`, preventing the large allocation on invalid input. * To further catch issues like this, protected against integer overflow during the calculation of last by using a long long temporary variable. If it exceeds INT_MAX or falls below INT_MIN, the spec is marked invalid immediately. Signed-off-by: Jacob Murphy <jkmurphy@google.com>
…alkey-io#3592) Fixes valkey-io#1905 ## Summary The direct use of `je_calloc` in `src/allocator_defrag.c` causes compilation failures on systems (e.g., Arch Linux with GCC 14.2.1) where `calloc` is marked as deprecated and `-Werror=deprecated` is enabled. ## Changes Replace the two `je_calloc` calls in `allocatorDefragInit()` with `zcalloc_num`, which is the proper Valkey allocation wrapper that provides the same semantics (num × size with zero-fill) without directly invoking the deprecated `calloc` symbol. ## Testing - Build compiles cleanly - Integration tests pass (unit/memefficiency, defrag, unit/other — 51 passed, 0 failed) Signed-off-by: jaduffy <jaduffy@amazon.com>
…et node disappears (valkey-io#3596) ### Summary This PR fixes a NULL pointer dereference (SIGSEGV) in `connectSlotExportJob()` (`src/cluster_migrateslots.c`) that can crash a Valkey cluster node, causing a denial-of-service condition. ### Root Cause When `CLUSTER MIGRATESLOTS` is issued, a migration job is created with state `SLOT_EXPORT_CONNECTING`. On the next `clusterCron()` tick, `proceedWithSlotMigration()` calls `connectSlotExportJob()`, which looks up the target node via `clusterLookupNode()`. `clusterLookupNode()` can legitimately return `NULL` — for example, if the target node is removed from the cluster (e.g. via `CLUSTER FORGET`) between the time the migration job is created and the time the cron fires. This is a realistic race condition in any cluster topology change scenario. The return value was **never checked**, so the subsequent call to `getNodeDefaultReplicationPort(n)` immediately dereferences the NULL pointer, crashing the process: ```c // Before fix — vulnerable clusterNode *n = clusterLookupNode(job->target_node_name, CLUSTER_NAMELEN); int port = getNodeDefaultReplicationPort(n); // SIGSEGV if n == NULL serverLog(..., n->ip, port); // second dereference Signed-off-by: chenshi5012 <chenshi5012@163.com>
…io#3591) When XTRIM marks the last entry in a listpack node as deleted, lpNext() returns NULL after the lp-count field (EOF). The delta calculation (p - lp) on a NULL pointer is undefined behavior and produces a garbage pointer, corrupting the listpack. A subsequent XREAD hitting the corrupted node triggers the lpValidateNext assertion failure and crashes the server. Guard the delta calculation with a NULL check so the while(p) loop terminates naturally when the last entry is reached. Fixes valkey-io#3569 Signed-off-by: Saurabh Kher <saurabh@amazon.com> Co-authored-by: Saurabh Kher <saurabh@amazon.com>
…3601) The function lpEncodeBacklen() uses `<= 127` for the 1-byte case but `< 16383`, `< 2097151`, and `< 268435455` for the subsequent cases. This means the exact values 16383, 2097151, and 268435455 (i.e. 2^14-1, 2^21-1, 2^28-1) unnecessarily use one extra byte than needed: - `l < 16383` → `16383` (2^14-1) uses 3 bytes instead of 2 - `l < 2097151` → `2097151` (2^21-1) uses 4 bytes instead of 3 - `l < 268435455` → `268435455` (2^28-1) uses 5 bytes instead of 4 The decoding side (`lpDecodeBacklen`) is unaffected since it parses continuation bits continuously without discrete range checks. This is a correctness issue and has no impact on data integrity since encoding and decoding use the same function boundaries, but it wastes up to 1 byte per affected entry. Signed-off-by: fanpei91 <fanpei91@gmail.com>
Big endian support on Valkey is "best effort" and not guaranteed, but we haven't been doing any regular testing at all afaik. This PR adds a job to the daily workflow to run UTs on an emulated big endian platform. Integration tests failed excessively because of how slow emulation is. I fixed several problems with tests and improved UT coverage of key points where endian byte order matters - and fwiw I didn't find any bugs. I think the main coverage gap remaining after this is RDB serialization (maybe little endian <-> big endian round trips?) There are couple lines of endian-specific code for valkey-io#3166 and this change can test it. Signed-off-by: Rain Valentine <rsg000@gmail.com>
It's important to enabled ASAN on run-extra-tests label so we can catch some of the bugs in the PRs before they are merged into unstable. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
…3610) ## Fix SIGSEGV in VM_GetLRU, VM_SetLRU, VM_GetLFU, VM_SetLFU on NULL key ### Description `VM_GetLRU`, `VM_SetLRU`, `VM_GetLFU`, and `VM_SetLFU` crash with SIGSEGV when passed a NULL `ValkeyModuleKey` pointer. This happens because all four functions dereference `key->value` without first checking if `key` itself is NULL. When a module opens a nonexistent key in `VALKEYMODULE_READ` mode, `VM_OpenKey` returns NULL. If a module passes that NULL pointer into any of these functions, the server crashes. ### Reproduction ``` valkey-server --loadmodule tests/modules/misc.so valkey-cli test.getlru nonexistent_key # Server crashes: SIGSEGV (signal 11) ``` ### Fix **`src/module.c`** — Add a `!key` guard before dereferencing `key->value` in all four functions: ```c // Before: if (!key->value) return VALKEYMODULE_ERR; // After: if (!key || !key->value) return VALKEYMODULE_ERR; ``` **`tests/modules/misc.c`** — Add early return after `open_key_or_reply()` in `test_getlru`, `test_setlru`, `test_getlfu`, and `test_setlfu`. The helper already sends the error reply to the client when the key is not found, so the command handler just needs to stop processing: ```c ValkeyModuleKey *key = open_key_or_reply(ctx, argv[1], VALKEYMODULE_READ|VALKEYMODULE_OPEN_KEY_NOTOUCH); if (!key) return VALKEYMODULE_OK; ``` ### After fix ``` valkey-cli test.getlru nonexistent_key (error) key not found # Server stays up ``` Signed-off-by: Yaron Sananes <yaron.sananes@gmail.com>
… (CVE-2026-23631) (valkey-io#3625) During a full sync, the functions/scripting engine is freed right before loading the RDB from the primary. If a Lua script is still running and yielding via the long-command mechanism at that moment, the freed engine can be accessed when the script resumes, causing a use-after-free. Add a guard at the top of replicaReceiveRDBFromPrimaryToMemory() to check isInsideYieldingLongCommand() and return early, deferring the sync processing until the script completes. No validating test was added because the vulnerability is a race condition between a yielding Lua script and a replication event handler, which cannot be reliably triggered in a deterministic Tcl test. Signed-off-by: ikolomi <ikolomin@amazon.com> Co-authored-by: ikolomi <ikolomin@amazon.com>
…25243) (valkey-io#3619) Root cause: zipmapValidateIntegrity() and zipmapNext() use different methods to calculate pointer advancement for length-encoded fields. Validation reads the actual encoded size via zipmapGetEncodedLengthSize() (which returns 5 for the 0xFE prefix), but zipmapRawKeyLength() (used by zipmapNext during hash conversion) recalculates via zipmapEncodeLength() which returns 1 for decoded lengths < 254. A crafted zipmap with an overlong 5-byte encoding for a small length passes validation but causes a 4-byte pointer mismatch in zipmapNext(), leading to heap buffer over-reads during the zipmap-to-listpack conversion. Fix: add sanity checks in zipmapValidateIntegrity() to reject entries where the decoded length < ZIPMAP_BIGLEN (254) but the encoding uses more than 1 byte. This is applied to both field-name and value lengths. Test: added a regression test in tests/unit/dump.tcl that crafts a RESTORE payload with a 2-entry zipmap where the first field uses an overlong 5-byte length encoding for value 3. Post-patch, this is cleanly rejected by zipmapValidateIntegrity(). Pre-patch, the misaligned zipmapNext() reads garbage (confirmed via server log: "Hash zipmap with dup elements, or big length (0)") which also produces an error, so the test serves as a defense-in-depth regression anchor rather than a strict pass/fail differentiator. The actual heap over-read is detectable with AddressSanitizer builds. Signed-off-by: ikolomi <ikolomin@amazon.com> Co-authored-by: ikolomi <ikolomin@amazon.com>
…alkey-io#3492) Add createFileEvent(): for non-RDMA, it is just aeCreateFileEvent. For RDMA, when registering AE_WRITABLE, register the event and call the handler once (same as the old direct writeHandler kick). Do not speculatively call readHandler after registering AE_READABLE (my previous approach): that interacts badly with libvalkey’s valkey-io/libvalkey#301 wakeup path and can stall the benchmark after the first request. Problem fixed: `valkey-benchmark --rdma` could hang or stall (e.g. GET-heavy workloads). Part of that was lost EPOLLIN when libvalkey drained the CQ and processed inbound data on the write path: the outer poll never saw a “new” edge. That belongs in libvalkey and is addressed by valkey-io/libvalkey#301 (eventfd re-arm / nested epoll integration). Separately, RDMA is not driven by kernel POLLOUT on the benchmark’s fd the way TCP is, so AE_WRITABLE may never fire unless we explicitly run the write path once after registration. The code previously did that with ad-hoc writeHandler calls (issueFirstRequestForClients, resetClient). Benchmark RDMA runs should use a libvalkey tree that includes valkey-io/libvalkey#301 (or equivalent fix). --------- Signed-off-by: Ada-Church-Closure <nunotabashinobu066@gmail.com>
…#3578) Ensure deferred-length reply placeholders are created and resolved in the same reply list that subsequent nested replies use when the deferred reply buffer is active. This prevents malformed responses when module callbacks build postponed-length arrays while a client is already deferring output. Add a regression test module and unit test that reproduce the issue through keyspace notifications and verify the nested array reply is serialized correctly. Register the new module with the module test build. Here is a small illustration of what happens with and without the fix: ## With the fix — placeholder goes to `c->deferred_reply`: ``` c->buf: (empty) c->reply: (empty) c->deferred_reply: [+OK\r\n] [*2\r\n] [+first\r\n] [*2\r\n] [+a\r\n] [+b\r\n] ^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SET reply reply callback (outer array, inner array via POSTPONED_LEN) → commitDeferredReplyBuffer joins into c->reply → Wire: +OK\r\n *2\r\n +first\r\n *2\r\n +a\r\n +b\r\n ✓ OK [ "first", ["a", "b"] ] ``` ## Without the fix — placeholder goes to `c->reply`: ``` c->buf: (empty) c->reply: [*2\r\n] ← placeholder filled by setDeferredArrayLen (WRONG LIST!) c->deferred_reply: [+OK\r\n] [*2\r\n] [+first\r\n] [+a\r\n] [+b\r\n] ^^^^^^^^ outer array "first" "a" "b" → commitDeferredReplyBuffer appends deferred_reply AFTER reply → Wire: *2\r\n +OK\r\n *2\r\n +first\r\n +a\r\n +b\r\n ✗ [ OK, ["first", "a"] ] "b" ← orphaned ``` * networking * tests/modules * tests/unit/moduleapi **Generated by CodeLite** --------- Signed-off-by: Eran Ifrah <eifrah@amazon.com>
…alkey-io#3481) Incrementally release memory to the OS using `madvice(MADV_DONTNEED)` when rehashing. This reduces the latency of the `free()` call at the end of the rehashing. ### Problem Upon rehashing completion, `rehashingCompleted()` frees the old table via `zfree()`. For large tables (e.g., tens of millions of keys), this operation can take tens of milliseconds, causing noticeable latency spikes in the server. ### Solution After each bucket migration, if 16 pages worth of buckets have been processed, we immediately release those pages to the OS via `madvise(MADV_DONTNEED)`. This incremental approach ensures that by the time `rehashingCompleted()` is called, most physical pages have already been returned to the OS, making the final cleanup fast and predictable. --------- Signed-off-by: chzhoo <czawyx@163.com>
Fixes valkey-io#3607 Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
…3448) Fixes server crash when RDMA benchmark clients disconnect (part of valkey-io#3345). This PR focuses on the server-side crash fix. The benchmark client-side fix will be submitted in a separate PR. ## Server Crash on Client Disconnect When interrupting valkey-benchmark with Ctrl+C, the server would crash with a segmentation fault in handleReadResult, accessing address 0x8 (NULL pointer dereference). ### Root Cause The same RDMA connection could be added to pending_list multiple times, leading to use-after-free: 1. When a client disconnects, rdmaHandleDisconnect() adds the connection to pending_list and sets conn->state = CONN_STATE_CLOSED 2. Before rdmaProcessPendingData() processes it, event-driven callbacks (e.g., connRdmaSetWriteHandler) could add the same connection again to pending_list 3. In rdmaProcessPendingData(), the first iteration processes and may free the connection, but the second iteration accesses the already freed connection → SIGSEGV at address 0x8 (NULL + offset) The problem was exacerbated because: - valkey-benchmark creates multiple concurrent connections - On Ctrl+C, these connections disconnect nearly simultaneously - Event callbacks can re-add connections before processing completes **Stack trace from crash:** handleReadResult+0x100 readQueryFromClient+0x63 rdmaProcessPendingData (0x1b1eb9) beforeSleep+0x82 ### Solution Four targeted fixes in `src/rdma.c`: 1. `rdmaHandleDisconnect (line 535)`: Check if pending_list_node is NULL before adding to pending_list to prevent duplicates ```C /* we can't close connection now, let's mark this connection as closed state */ if (rdma_conn->pending_list_node == NULL) { listAddNodeTail(pending_list, conn); rdma_conn->pending_list_node = listLast(pending_list); } ``` 2. `connRdmaSetWriteHandler (line 956)`: Add same check before adding to pending_list to prevent duplicates during write handler updates ```C /* does this connection has pending write data? */ if (func) { if (rdma_conn->pending_list_node == NULL) { listAddNodeTail(pending_list, conn); rdma_conn->pending_list_node = listLast(pending_list); } } else if (rdma_conn->pending_list_node) { listDelNode(pending_list, rdma_conn->pending_list_node); rdma_conn->pending_list_node = NULL; } ``` 3. `rdmaProcessPendingData (line 1786)`: Use iterator node 'ln' instead of rdma_conn->pending_list_node when deleting, as the latter may have been overwritten if the connection was re-added ```C listDelNode(pending_list, ln); // Use ln, not rdma_conn->pending_list_node rdma_conn->pending_list_node = NULL; ``` 4. `rdmaProcessPendingData (line 1782-1787)`: Reorder operations to call handlers before deleting from list, ensuring connection structure remains valid during handler execution ```C if (conn->state == CONN_STATE_ERROR || conn->state == CONN_STATE_CLOSED) { /* Invoke handlers first, then remove from list */ if (callHandler(conn, conn->read_handler)) { callHandler(conn, conn->write_handler); } listDelNode(pending_list, ln); rdma_conn->pending_list_node = NULL; ++processed; continue; } ``` The fix uses **pending_list_node** as both a list pointer and a "**already in list**" flag: - NULL = not in list, safe to add - non-NULL = already in list, skip adding This ensures each connection appears in pending_list at most once, preventing use-after-free crashes. Signed-off-by: Ada-Church-Closure <nunotabashinobu066@gmail.com>
…#3391) Instead of scanning one slot at a time `CLUSTERSCAN` now scans the entire contiguous range of slots owned by the current node. Implementation details * Re-sharding safe as the hash slot is updated based on the local cursor position. * Fingerprint remains stable across the entire contiguous slot range instead of being reset per slot. * Parsing/validation of parameters for the SCAN commands is refactored and moved to a separate function. ``` > CLUSTERSCAN 0 "0-{06S}-0" # start at slot 0 > CLUSTERSCAN 0-{06S}-0 "aBcDeF-{06S}-48" # scanning slot 0... > CLUSTERSCAN aBcDeF-{06S}-48 "aBcDeF-{1Y7}-16" # slot 0 done, continues to slot 6 (same node hence FP is unchanged) > CLUSTERSCAN aBcDeF-{1Y7}-16 "aBcDeF-{0or}-32" # slot 6 done, continues to slot 100 (same node hence FP is unchanged) ... > CLUSTERSCAN aBcDeF-{...}-64 "0-{8YG}-0" # Current continuous slot boundary reached hence cross-node transition ``` Follow-up of valkey-io#2934 --------- Signed-off-by: nmvk <r@nmvk.com> Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
CI caught ip and name SDS allocations being leaked in fetchClusterConfiguration. The ip SDS was copied again via sdsnew() before being passed to createClusterNode(), leaking the original. The name SDS was leaked when the node already existed in the dict. Free ip and name on all exit paths in fetchClusterConfiguration. Remove stale guard in freeClusterNode, no longer needed since valkey-io#1392 CI Error - ``` Direct leak of 33 byte(s) in 3 object(s) allocated from: #0 0x7f4c3a0fd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69 #1 0x5564620c124a in ztrymalloc_usable_internal /home/runner/work/valkey/valkey/src/zmalloc.c:172 #2 0x5564620c124a in zmalloc_usable /home/runner/work/valkey/valkey/src/zmalloc.c:268 valkey-io#3 0x5564620dfbe6 in _sdsnewlen.constprop.0 /home/runner/work/valkey/valkey/src/sds.c:102 valkey-io#4 0x556462050996 in sdsnewlen /home/runner/work/valkey/valkey/src/sds.c:169 valkey-io#5 0x556462050996 in sdsnew /home/runner/work/valkey/valkey/src/sds.c:185 valkey-io#6 0x556462050996 in fetchClusterConfiguration /home/runner/work/valkey/valkey/src/valkey-benchmark.c:1477 ``` Issue was reproduceable locally using `leaks --atExit` Signed-off-by: nmvk <r@nmvk.com>
Fix various typos related to setup and set up. Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Fix various typos around otherwise with a comma Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
…lkey-io#4380) Fixes paths which used a client's cached slot id for keys not directly associated with the client. --------- Signed-off-by: Nitai Caro <caronita@amazon.com> Co-authored-by: Nitai Caro <caronita@amazon.com>
…io#4323) In an earlier commit we didn't properly reset the redaction bitmap during exec, and used the wrong one for lua scripts. This fixes that to properly redact commands. Added new regression tests and all existing tests still pass. --------- Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
…d Tcl tests (valkey-io#2243) Fix spelling and grammar issues across 17 files This is a subset of valkey-io#2183. --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
…-io#3922) ## Problem Two crafted-`RESTORE` crashes in stream loading. In both, the payload passes the existing structural validation but violates an invariant downstream code relies on. **Any client with `RESTORE` access can remotely crash the server.** ### 1. Length vs. tombstones A stream can claim a positive `length` while every listpack entry is a tombstone (`STREAM_ITEM_FLAG_DELETED`). The length is loaded directly from the payload and only checked against the rax being non-empty. `streamLastValidID()` then finds no non-tombstone entry while `s->length` is non-zero and aborts: ``` serverPanic("Corrupt stream, length is %llu, but no max id", ...) // t_stream.c ``` Triggered by `XSETID` / `XADD` / `XREADGROUP`. Confirmed: a 2-entry stream with both entries flagged `DELETED` and `length=1` loads OK, then `XSETID` panics. ### 2. Negative field counts A master entry (or a per-entry field count for non-`SAMEFIELDS` entries) can declare a **negative** number of fields. The validator only checked `lpGetIntegerIfValid()`'s success flag, not the sign. The negative count drives listpack traversal in `streamIteratorGetID()`, walking past the listpack and asserting (`lpAssertValidEntry`) on `XRANGE` and similar reads. Confirmed: crafted payload loads OK, then `XRANGE` aborts at `listpack.c`. ## Fix 1. `streamValidateListpackIntegrity()` already parses each listpack's master entry count (live entries). Sum it across listpacks via a new out-parameter and reject the payload if it does not match the loaded length. This reuses the assertion-safe parsing rather than iterating the stream with `streamIteratorGetID()`, which can itself hit entry-level assertions on *other* malformed payloads (an earlier iterate-based version regressed three existing corrupt-dump tests). 2. Reject negative `primary_fields` and per-entry `fields` counts during validation. ## Testing - Two `RESTORE`-path integration tests in `tests/integration/corrupt-dump.tcl`. - Both verified to **fail pre-fix** (panic / assert) and **pass post-fix**. - Confirmed legitimate streams — including ones with real tombstones (5 entries, 2 deleted) and multi-field entries — still load and read correctly. - Full `integration/corrupt-dump` suite: 75 passed, 0 failed (including the three stream consumer-group tests an earlier iterate-based approach broke). > [!NOTE] > Found via structure-aware fuzzing + code review of the RESTORE path. This issue was generated by AI but verified, with love, by a human. --------- Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Grammatical errors are fixed in documentation Signed-off-by: takahashi shun <mokopoi44@gmail.com> Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
Fixes grammatical and spelling errors --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
…valkey-io#4368) In `generateStringArgValue`, the `argName` dispatch chain contains two identical `else if (strcmp(argName, "command") == 0)` branches. The earlier branch already handles every `argName == "command"` case, so the second one, whose body is identical, is unreachable dead code. This removes the duplicate branch; behavior is unchanged. Signed-off-by: latent-9 <296084221+latent-9@users.noreply.github.com>
Improves spelling and grammar across codebase --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Grammar corrections in comments --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
…ey-io#4381) Stream listpack master entries store separate live and deleted record counts, but integrity validation only verified their sum. A corrupted payload could therefore preserve the total while understating the live count, causing XLEN to disagree with the stored records and allowing XDEL to discard records not accounted for by the header. Count live and deleted records independently while validating stream listpacks and reject payloads when either count differs from its declared value. Signed-off-by: Roshan Khatri <roshanvkhatri@gmail.com>
Consolidating nonexistent spelling Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
…alkey-io#4402) `src/commands/config-info.json` and `src/commands/move.json` still carry `"since": "10.0.0"`. *This was generated by AI but verified, with love, by a human.* Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
…load (valkey-io#4360) moduleUnregisterCleanup did not remove the module's cluster message receivers. The stale entries kept pointing at the freed ValkeyModule, so a later cluster message of that type dereferenced r->module in moduleCallClusterReceivers, causing a use-after-free. Added a MODULE UNLOAD test to verify the fix, and also added type=254 to allow us to verify the correctness of the loop logic. --------- Signed-off-by: Binbin <binloveplay1314@qq.com>
…ndlers (valkey-io#4401) Follow-up of valkey-io#3611. Fixes a regression introduced by that PR. The May 27 On-Demand run on this PR (SET/GET, 96B, io-threads 2/10, pipeline 1/10) showed no significant RPS change. After `17ec23f` removed `post_read_done_postpone_mask`, `processClientIOReadsDone()` started postponing READ and returning `needs_post_read_update = 1` for every non-ACCEPTING completed read. That second phase (`lookupClientByID` + `processPendingCommandAndInputBuffer` + `connUpdateState`) is only required when `update_state` may synchronously invoke handlers. For other transports it is per-completion overhead. This matches the post-merge dashboard drop: small payloads, io-threads, **P1 worse than P10**. This PR restores the original gate without bringing `struct client` into the connection driver (the review concern that led to `17ec23f`): - `ConnectionType.sync_handlers_in_update_state` (0 by default) - Set only where `update_state` can sync-call handlers - Mask is still computed in `networking.c` from IO state - `connUpdateState()` still runs immediately, including ACCEPTING --------- Signed-off-by: quanyeyang <quanyemostima@gmail.com>
Log EXEC in commandlog. Catches cases where there's no individually slow command. --------- Signed-off-by: Michelle Lee <michellee.3104@gmail.com>
…lkey-io#4404) `unit/commandlog` fails on unstable at 917de6a: ``` *** [err]: COMMANDLOG slow - Redaction does not leak to later commands in a MULTI in tests/unit/commandlog.tcl Expected 'set foo bar' to be equal to 'exec' (context: type eval line 12 cmd {assert_equal {set foo bar} [lindex [lindex $slowlog_resp 0] 3]} proc ::test) ``` valkey-io#4267 dropped `SKIP_COMMANDLOG` from EXEC (`src/commands/exec.json:12`), so EXEC is now logged after the commands it ran, which puts it at entry 0 of the newest-first `COMMANDLOG GET`. The redaction test at `tests/unit/commandlog.tcl:212`, added by valkey-io#4323, still reads entry 0 and gets the EXEC instead of the SET. Neither PR was rebased on the other, so this only broke on merge and not in either PR's CI. The log after `MULTI; ACL SETUSER commandlog-test-user +get; SET foo bar; EXEC`: | Entry | Command | |---|---| | 0 | `exec` | | 1 | `set foo bar` | | 2 | `acl setuser (redacted) (redacted)` | Read the SET from entry 1 instead. The other tests valkey-io#4267 touched already assert entry 0 is `exec` and entry 1 is the inner command, so this matches. Also assert entries 0 and 2 rather than only the SET. The point of the test is that the ACL SETUSER redaction stops at the ACL SETUSER, and asserting entry 2 is redacted is what keeps it from passing if redaction breaks entirely. Asserting entry 0 is `exec` makes the index arithmetic fail loudly next time the entry order changes, instead of silently comparing against the wrong entry the way it just did. ## Testing `unit/commandlog` goes from 29 passed, 1 failed to 30 passed. `unit/slowlog` and `unit/multi` are unchanged at 100 passed. Reverting the `src/` hunks of 917de6a confirms that commit is the trigger. The new entry-0 assert fails, along with the two EXEC tests 917de6a added: ``` *** [err]: COMMANDLOG slow - Redaction does not leak to later commands in a MULTI in tests/unit/commandlog.tcl Expected 'exec' to be equal to 'set foo bar' (context: type eval line 13 cmd {assert_equal {exec} [lindex [lindex $slowlog_resp 0] 3]} proc ::test) *** [err]: COMMANDLOG slow - EXEC is logged alongside slow inner commands in tests/unit/commandlog.tcl Expected '1' to be equal to '2' (context: type eval line 8 cmd {assert_equal [r commandlog len slow] 2} proc ::test) *** [err]: COMMANDLOG slow - EXEC records total transaction time when inner commands are individually fast in tests/unit/commandlog.tcl Expected '0' to be equal to '1' (context: type eval line 9 cmd {assert_equal [r commandlog len slow] 1} proc ::test) ``` *This was generated by AI but verified, with love, by a human.* Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
The cluster message receiver array was sized UINT8_MAX (255), so the valid index range was 0..254. Two issues existed: - VM_RegisterClusterMessageReceiver allowed type 255, an out-of-bounds WRITE past the end of the array. - moduleCallClusterReceivers would perform an out-of-bounds READ when a cluster bus packet carried type 255 (UINT8_MAX), which can be sent by any cluster member. Grow the array to MAX_CLUSTER_MESSAGE_TYPES (UINT8_MAX + 1) so the full uint8_t type range (0..255) is covered, and use the explicit constant in both dispatch and unregister paths. type 255 is now a valid, dispatchable message type. Signed-off-by: Binbin <binloveplay1314@qq.com>
The current approach of manually inspecting logs to determine how long a full sync took is highly cumbersome, hence introducing a new metric called `last_successful_sync_duration_ms `. The new metric `last_successful_sync_duration_ms `, which measures the total time (in milliseconds) taken to complete the last full synchronization, will be reported by replicas in the `INFO replication` output. ``` 127.0.0.1:6380> info replication # Replication role:slave master_host:localhost master_port:6379 master_link_status:up master_last_io_seconds_ago:1 last_successful_sync_duration_ms:15537 <<<<<<<<<-------------===== I AM HERE! master_sync_in_progress:0 slave_read_repl_offset:5345000149 slave_repl_offset:5345000149 replicas_repl_buffer_size:0 replicas_repl_buffer_peak:0 slave_priority:100 slave_read_only:1 replica_announced:1 connected_slaves:0 replicas_waiting_psync:0 master_failover_state:no-failover master_replid:51005812f8730e42095a20c1fe9397c08abd608e master_replid2:0000000000000000000000000000000000000000 master_repl_offset:5345000149 second_repl_offset:-1 repl_backlog_active:1 repl_backlog_size:10485760 repl_backlog_first_byte_offset:5345000122 repl_backlog_histlen:28 ``` --------- Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
The DCO check is red because it could not retrieve the full PR commit list (250+ commits), so it reports an unknown verdict rather than a missing sign-off. Please re-run that check after the review findings are addressed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## forkless #4422 +/- ##
============================================
+ Coverage 77.81% 79.17% +1.36%
============================================
Files 167 171 +4
Lines 84925 91807 +6882
============================================
+ Hits 66083 72691 +6608
- Misses 18842 19116 +274
🚀 New features to boost your workflow:
|
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed this as a merge: I walked the conflict resolutions (git show --cc flags .config/typos.toml, src/Makefile, src/bgiteration.c, src/replication.c, src/server.c, src/unit/test_bgiteration.cpp, src/unit/wrappers.h as the files whose content differs from both parents) and checked that upstream's changes survived elsewhere.
Most of it looks right: getKeySlot -> getKVStoreIndexForKey in bgiteration.c is the correct half of upstream's #4380 split (bgIteration passes keys that aren't necessarily the executing command's, so the non-cached variant is the safe one), OBJ_ENCODING_SKIPLIST -> OBJ_ENCODING_BTREE still matches zset->ht, #4401's sync_handlers_in_update_state plumbing is intact, and commands.def matches the merged JSON. make valkey-server builds clean.
One resolution is wrong: rdmaProcessPendingData ended up with two copies of the same listDelNode. That lines up with test-rdma being green on both parents and red on this merge.
Signed-off-by: Nitai Caro <caronita@amazon.com>
VinayakGhai
reviewed
Aug 15, 2026
VinayakGhai
left a comment
VinayakGhai
left a comment
There was a problem hiding this comment.
Woah, merging upstream directly into a feature branch like this can pull in unrelated regressions if we haven't verified stable CI tests. We should run a full test-suite run on this branch first to make sure nothing broke.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.