Fix repl alsopropagate - #488
Open
sundb wants to merge 391 commits into
Open
Conversation
Validate HEXPIRE-family field counts without parser overflow keep flexible option order; only require fields fit in argv add tests for INT_MAX numfields across HEXPIRE/HPEXPIRE/HEXPIREAT/HPEXPIREAT
…STORE (redis#14892) ### Overview This PR adds a new `COUNT` aggregation mode to the `ZUNIONSTORE`, `ZINTERSTORE`, `ZUNION`, and `ZINTER` sorted set commands. When `AGGREGATE COUNT` is specified, the resulting score for each element reflects how many input sets contain it (optionally scaled by `WEIGHTS`), rather than combining the actual scores of the elements. This enables a common use case — counting set membership frequency — directly at the command level, without application-side workarounds. ### Problem Statement For developers who need to know **how many input sorted sets contain each element**, there is no single-command solution today. **Example:** given several game leaderboards, find how many leaderboards each player appears in. The existing aggregation modes (`SUM`, `MIN`, `MAX`) all operate on the elements' scores. To ignore scores and just count set membership, you'd currently need to copy each sorted set with all scores set to 1, then run `ZUNIONSTORE`/`ZINTERSTORE` with `SUM` — requiring multiple round trips, temporary keys, and application-level locking to avoid races. A `COUNT` aggregation mode solves this directly. ### Solution Introduces `AGGREGATE COUNT` as a fourth aggregation mode: - `ZINTER numkeys key [key ...] [WEIGHTS weight [weight ...]] [AGGREGATE <SUM | MIN | MAX | COUNT>] [WITHSCORES]` - `ZINTERSTORE destination numkeys key [key ...] [WEIGHTS weight [weight ...]] [AGGREGATE <SUM | MIN | MAX | COUNT>]` - `ZUNION numkeys key [key ...] [WEIGHTS weight [weight ...]] [AGGREGATE <SUM | MIN | MAX | COUNT>] [WITHSCORES]` - `ZUNIONSTORE destination numkeys key [key ...] [WEIGHTS weight [weight ...]] [AGGREGATE <SUM | MIN | MAX | COUNT>]` When `COUNT` is specified, **the scores in the input sets are ignored**. Note that `WEIGHTS` is **not** ignored — each set contributes its weight (default 1) per element, and the contributions are summed. **Implementation details:** A new helper function `zuiWeightedScore()` computes the per-set contribution: ```c inline static double zuiWeightedScore(double score, double weight, int aggregate) { return (aggregate == REDIS_AGGR_COUNT) ? weight : weight * score; } ``` The `zunionInterAggregate()` function treats `COUNT` identically to `SUM` — it adds the per-set contributions. All four call sites where `weight * score` was previously computed inline are updated to use `zuiWeightedScore()`. ### Examples ``` > ZADD s1 1 foo 1 bar > ZADD s2 2 foo 2 bar > ZADD s3 3 foo ``` **With `SUM` (existing behavior, for comparison):** ``` > ZINTERSTORE t1 3 s1 s2 s3 WEIGHTS 10 5 3 AGGREGATE SUM (integer) 1 > ZRANGE t1 0 -1 WITHSCORES 1) "foo" 2) "29" > ZUNIONSTORE t1 3 s1 s2 s3 WEIGHTS 10 5 3 AGGREGATE SUM (integer) 2 > ZRANGE t1 0 -1 WITHSCORES 1) "bar" 2) "20" 3) "foo" 4) "29" ``` **With `COUNT` and `WEIGHTS`:** ``` > ZINTERSTORE t1 3 s1 s2 s3 WEIGHTS 10 5 3 AGGREGATE COUNT (integer) 1 > ZRANGE t1 0 -1 WITHSCORES 1) "foo" 2) "18" > ZUNIONSTORE t1 3 s1 s2 s3 WEIGHTS 10 5 3 AGGREGATE COUNT (integer) 2 > ZRANGE t1 0 -1 WITHSCORES 1) "bar" 2) "15" 3) "foo" 4) "18" ``` **With `COUNT` and no specified `WEIGHTS`** — resulting score equals the number of input sorted sets containing the element: ``` > ZINTERSTORE t1 3 s1 s2 s3 AGGREGATE COUNT (integer) 1 > ZRANGE t1 0 -1 WITHSCORES 1) "foo" 2) "3" > ZUNIONSTORE t1 3 s1 s2 s3 AGGREGATE COUNT (integer) 2 > ZRANGE t1 0 -1 WITHSCORES 1) "bar" 2) "2" 3) "foo" 4) "3" ``` ### Backward Compatibility This is a fully additive change. The new `COUNT` keyword is only recognized after the `AGGREGATE` token in the four affected commands. Existing commands, arguments, and default behavior (`AGGREGATE SUM`) are completely unchanged. No new command is introduced, and no existing response format is modified.
…tracking (redis#15037) `xinfoReplyWithStreamInfo` passed the wrong key(c->argv[1]) instead of `c->argv[2]` to `updateSlotAllocSize` when updating per-slot memory tracking. Fix by passing the key explicitly to `xinfoReplyWithStreamInfo` instead of relying on a hardcoded argv index. Also, add the `-DDEBUG_ASSERTIONS` flag to the test-ubuntu-jemalloc CI to cover this debug assertion.
Refactoring work for follow-ups (e.g. subkey notifications redis#14958), splitting reusable infrastructure from feature logic. Optimized for stack allocation with optional growth to heap. Usage: Start on stack (grow to heap): vec v; void *vstack[8]; vecInit(&v, vstack, 8); Start embedded (grow to heap): typedef struct { vec v; void *vembedded[8]; } obj; vecInit(&obj.v, obj.vembedded, 8); Heap only (capacity 8 or 0): vecInit(&v, NULL, 8); vecInit(&v, NULL, 0); Reserve based on size: vecInit(&v, vstack, 8); vecReserve(&v, varsize); // <=8 uses stack, else heap
The fast_float dependency required C++ (libstdc++) to build Redis. This commit replaces the 3800-line C++ template library with a minimal pure C implementation (~360 lines) that provides the same functionality needed by Redis. This is **very important** because Redis build process would fail without g++ installed, a common situation in Linux distributions even after installing the basic build tools: we want the build process of Redis to be the simplest possible. Also Redis sometimes is compiled in embedded systems lacking the g++ toolchain. There is no reason to depend on C++ in a project written in C. ## The C implementation uses 1. Fast path (Clinger's algorithm) for numbers with mantissa <= 2^53 and exponent in [-22, 22], covering ~99% of real-world cases. 2. Fallback to strtod() for complex cases to ensure correctly-rounded results. ## Changes - Move new fast_float_strtod.c(C implementation) from deps into Redis core since it is now a single file and no longer needs a separate directory. - Remove all c++ dependencies The implementation was tested against both strtod and the original C++ implementation with 10,000+ test cases including edge cases, special values (inf/nan), and random inputs. --------- Co-authored-by: debing.sun <debing.sun@redis.com> Co-authored-by: Mincho Paskalev <minchopaskal@gmail.com> Co-authored-by: Moti Cohen <moti.cohen@redis.com>
During RDB saving and AOF rewriting, the fork child already dismisses (madvise(MADV_DONTNEED)) individual key-value objects after serializing them. However, the hash table bucket arrays of each dict were never dismissed, leaving large contiguous allocations subject to CoW when the parent modifies them. This PR extends the dismiss mechanism to cover dict bucket arrays, reducing CoW memory overhead. - **Expires kvstore** — dismissed upfront before saving starts, since the child never accesses expires directly, after embeding expire time in the key object. - **Slot dicts** (cluster mode) — dismissed per-slot as the iterator moves to the next slot during RDB saving or AOF rewriting. - **DB keys kvstore** (standalone mode) — dismissed per-DB after each DB is fully serialized during RDB saving or AOF rewriting.
Refactor command propagation code to reduce overhead on master Currently, the main bottleneck is `feedReplicationBuffer()`. It is called for each argument in the command and has bookkeeping overhead on every call (e.g. checking whether to attach replicas to the replication backlog). It is also not inlined by the compiler. These costs become more visible with pipelining and commands with many arguments (e.g. HSET with many fields). Changes: - Defer all bookkeeping to be done once per command instead of once per command argument. - Refactor the hot path so the compiler can inline `replBufWriterAppend()`. - Add `replBufWritterAppendBulkLen()` that uses shared RESP headers for small values, avoiding formatting overhead. These changes should not introduce any behavioral change. **TODO:** In a follow-up PR, explore forwarding the exact command from the client querybuf to avoid re-serialization. Many commands are propagated without modification and can benefit from this. -- | Benchmark | Before (ops/s) | After (ops/s) | Improvement | |---|---|---|---| | SET | 256,048 | 265,131 | **+3%** | | SET (pipeline) | 1,477,310 | 1,671,272 | **+13%** | | HSET 10 fields | 145,000 | 158,000 | **+9%** | | HSET 10 fields (pipeline) | 363,483 | 430,855 | **+18%** | | HSET 10 fields, 15B values (pipeline) | 387,443 | 487,135 | **+26%** | | ZADD 5 members | 180,700 | 193,519 | **+7%** | | ZADD 5 members (pipeline) | 466,453 | 564,872 | **+21%** | ------ Co-authored-by: Yuan Wang <yuan.wang@redis.com>
[PR ](redis#14826) introduced a new rate limiting command which stores its internal implementation-detail data into a string key. Since this will prevent a client from detecting type errors or accidental overwrites or value invalidations, f.e via SET or INCR this PR introduces a new data type - OBJ_GCRA specifically created for that new command. Furthermore, a new RATE_LIMIT KSN type was introduced for emitting "gcra" events on such keys. GCRASETTAT was renamed to GCRASETVALUE. --------- Co-authored-by: debing.sun <debing.sun@redis.com>
Log source/destination address on import/migrate start events for easier debugging.
…edis#14956) HSETEX crashed on assert() with a SIGABRT when the same field appeared more than once in the FIELDS list and an expiry time was given (EX/PX/EXAT/PXAT). Root cause: hfieldPersist() and the KEEP_TTL path in hashTypeSet() both asserted that dictExpireMeta->expireMeta.trash == 0, meaning the hash must be globally registered in the HFE DS. This is incorrect during HSETEX execution because hashTypeSetExDone(), which registers the hash globally and clears trash, called only at the end of flow. The private per-field ebuckets are fully valid regardless of the global registration state. Fix: Remove both incorrect assertions. The operations on the private ebuckets (ebRemove in hfieldPersist, ebAdd in the KEEP_TTL path) are correct and do not require the hash to be globally registered. Tests: Added two regression tests covering the crash scenarios: - HSETEX EX with a duplicate field (existing field, expiry given) - HSETEX FNX EX with a duplicate field (no prior field, FNX condition passes)
## Summary This PR fixes two issues when processing corrupt data in rdbLoadCheckModuleValue(): 1. When handling `RDB_MODULE_OPCODE_STRING` opcode, rdbGenericLoadStringObject() can return NULL on a corrupt payload. The code called decrRefCount(o) unconditionally without a NULL check, resulting in a NULL pointer dereference crash. 2. The while loop condition was `!= RDB_MODULE_OPCODE_EOF`, which means a truncated payload (causing rdbLoadLen to return RDB_LENERR) would never exit the loop, since `RDB_LENERR != RDB_MODULE_OPCODE_EOF` is always true, potentially causing an infinite hang.
## Motivation
Redis's existing keyspace notification system operates at the **key
level** only — when a hash field is modified via `HSET`, `HDEL`, or
`HEXPIRE`, the subscriber receives the key name and the event type, but
not **which fields** were affected, therefore, these notifications has
very little practical value.
This PR introduces a subkey notification system that extends keyspace
events to include field-level (subkey) details for hash operations,
through both Pub/Sub channels and the Module API.
## New Pub/Sub Notification Channels
Four new channels are added:
|Channel Format | Payload |
|---------------|---------|
| `__subkeyspace@<db>__:<key>` | `<event>\|<len>:<subkey>[,...]` |
|`__subkeyevent@<db>__:<event>` |
`<key_len>:<key>\|<len>:<subkey>[,...]` |
| `__subkeyspaceitem@<db>__:<key>\n<subkey>` | `<event>` |
|`__subkeyspaceevent@<db>__:<event>\|<key>` | `<len>:<subkey>[,...]` |
**Design rationale for 4 channels:**
- **Subkeyspace**: Subscribe to a specific key, receive all field
changes in a single message — efficient for key-centric consumers.
- **Subkeyevent**: Subscribe to a specific event type, receive
key+fields — efficient for event-centric consumers.
- **Subkeyspaceitem**: Subscribe to a specific key+field combination —
the most selective, one message per field, no parsing needed.
- **Subkeyspaceevent**: Subscribe to event+key combination, receiving
only the affected fields — server-side filtering on both dimensions.
Subkeys are encoded in a length-prefixed format (`<len>:<subkey>`) to
support binary-safe field names containing delimiters.
**Safety guards:**
- Events containing `|` are skipped for `__subkeyspace` and
`__subkeyspaceevent ` channels (to avoid parsing ambiguity).
- Keys containing `\n` are skipped for the `__subkeyspaceitem` channel
(newline is the key/subkey separator).
- Subkeys channels are only published when `subkeys != NULL && count >
0`.
## Hash Command Integration
The following hash operations now emit subkey level notifications with
the affected field names:
| Command | Event | Subkeys |
|---------|-------|---------|
| `HSET` / `HMSET` | `hset` | All fields being set |
| `HSETNX` | `hset` | The field (if set) |
| `HDEL` | `hdel` | All fields deleted |
| `HGETDEL` | `hdel` / `hexpired` | Deleted or lazily expired fields |
| `HGETEX` | `hexpire` / `hpersist` / `hdel` / `hexpired` | Affected
fields per event |
| `HINCRBY` | `hincrby` | The field |
| `HINCRBYFLOAT` | `hincrbyfloat` | The field |
| `HEXPIRE` / `HPEXPIRE` / `HEXPIREAT` / `HPEXPIREAT` | `hexpire` |
Updated fields |
| `HPERSIST` | `hpersist` | Persisted fields |
| `HSETEX` | `hset` / `hdel` / `hexpire` / `hexpired` | Affected fields
per event |
| Field expiration (active/lazy) | `hexpired` | All expired fields
(batched) |
For field expiration, expired fields are collected into a dynamic array
and sent as a single batched notification after the expiration loop,
rather than one notification per field.
## Module API
Three new APIs and one new callback type:
```c
/* Function pointer type for keyspace event notifications with subkeys from modules. */
typedef void (*RedisModuleNotificationWithSubkeysFunc)(
RedisModuleCtx *ctx, int type, const char *event,
RedisModuleString *key, RedisModuleString **subkeys, int count);
/* Subscribe to keyspace notifications with subkey information.
*
* This is the extended version of RM_SubscribeToKeyspaceEvents. When subkeys
* are available, the `subkeys` array and `count` are passed to the callback.
* `subkeys` contains only the names of affected subkeys (values are not included),
* and `count` is the number of elements. The array may contain duplicates when
* the same subkey appears more than once in a command (e.g. HSET key f1 v1 f1 v2
* produces subkeys=["f1","f1"], count=2). When no subkeys are present, `subkeys`
* will be NULL and `count` will be 0. Whether events without subkeys are delivered
* depends on the `flags` parameter (see below).
*
* `types` is a bit mask of event types the module is interested in
* (using the same REDISMODULE_NOTIFY_* flags as RM_SubscribeToKeyspaceEvents).
*
* `flags` controls delivery filtering:
* - REDISMODULE_NOTIFY_FLAG_NONE: The callback is invoked for all matching
* events regardless of whether subkeys are present, so a separate
* RM_SubscribeToKeyspaceEvents registration can be omitted.
* - REDISMODULE_NOTIFY_FLAG_SUBKEYS_REQUIRED: The callback is only invoked
* when subkeys are not empty. Events without subkey information (e.g. SET,
* EXPIRE, DEL) are skipped.
*
* The callback signature is:
* void callback(RedisModuleCtx *ctx, int type, const char *event,
* RedisModuleString *key, RedisModuleString **subkeys, int count);
*
* The subkeys array and its contents are only valid during the callback.
* The underlying objects may be stack-allocated or temporary, so
* RM_RetainString must NOT be used on them. To keep a subkey beyond
* the callback (e.g. in a RM_AddPostNotificationJob callback), use
* RM_HoldString (which handles static objects by copying) or
* RM_CreateStringFromString to make a deep copy before returning.
*/
int RM_SubscribeToKeyspaceEventsWithSubkeys(RedisModuleCtx *ctx, int types, int flags, RedisModuleNotificationWithSubkeysFunc callback);
/* Unregister a module's callback from keyspace notifications with subkeys
* for specific event types.
*
* This function removes a previously registered subscription identified by
* the event mask, delivery flags, and the callback function.
*
* Parameters:
* - ctx: The RedisModuleCtx associated with the calling module.
* - types: The event mask representing the notification types to unsubscribe from.
* - flags: The delivery flags that were used during registration.
* - callback: The callback function pointer that was originally registered.
*
* Returns:
* - REDISMODULE_OK on successful removal of the subscription.
* - REDISMODULE_ERR if no matching subscription was found. */
int RM_UnsubscribeFromKeyspaceEventsWithSubkeys(
RedisModuleCtx *ctx, int types, int flags,
RedisModuleNotificationWithSubkeysFunc cb);
/* Like RM_NotifyKeyspaceEvent, but also triggers subkey-level notifications
* when subkeys are provided. Both key-level (keyspace/keyevent) and
* subkey-level (subkeyspace/subkeyevent/subkeyspaceitem/subkeyspaceevent)
* channels are published to, depending on the server configuration.
*
* This is the extended version of RM_NotifyKeyspaceEvent and can actually
* replace it. When called with subkeys=NULL and count=0, it behaves
* identically to RM_NotifyKeyspaceEvent. */
int RM_NotifyKeyspaceEventWithSubkeys(
RedisModuleCtx *ctx, int type, const char *event,
RedisModuleString *key, RedisModuleString **subkeys, int count);
```
## Configuration
Subkey notifications are controlled via the existing
`notify-keyspace-events` configuration string with four new characters:
`notify-keyspace-events` "STIV"
**S** -> Subkeyspace events, published with `__subkeyspace@<db>__:<key>`
prefix.
**T** -> Subkeyevent events, published with
`__subkeyevent@<db>__:<event>` prefix.
**I** -> Subkeyspaceitem events, published per subkey with
`__subkeyspaceitem@<db>__:<key>\n<subkey>` prefix.
**V** -> Subkeyspaceevent events, published with
`__subkeyspaceevent@<db>__:<event>|<key>` prefix.
These flags are **independent** from the existing key-level flags (`K`,
`E`, etc.). Enabling subkey notifications does **not** implicitly enable
or depend on keyspace/keyevent notifications, and vice versa.
## Known Limitations
- **Duplicate fields in subkey notifications**: Subkey notification
payloads may contain duplicate field names when the same field is
affected more than once within a single command. Since duplicate fields
are not the common case and deduplication would introduce significant
overhead on every notification, we chose not to deduplicate at this
time.
- **Subkey is sds encoding object**: We assume the subkey is sds
encoding object, and access it by `subkey->ptr`, and there is an assert,
redis will crash if not.
### Problem While the new type `OBJ_GCRA` was added, several related code paths were not updated accordingly, leading to failures in the `reply-schemas-validator` CI job and `corrupt-dump-fuzzer.tcl` ##### reply-schemas-validator Failed CI: https://github.com/redis/redis/actions/runs/24485248057/job/71558533290#step:10:903 ```shell Traceback (most recent call last): File "/home/runner/work/redis/redis/./utils/req-res-log-validator.py", line 238, in process_file jsonschema.validate(instance=res.json, schema=req.schema, cls=schema_validator) File "/home/runner/.local/lib/python3.12/site-packages/jsonschema/validators.py", line 1121, in validate raise error jsonschema.exceptions.ValidationError: 'rate_limit' is not valid under any of the given schemas Failed validating 'oneOf' in schema['patternProperties']['^.*$']['properties']['group']: {'description': 'the functional group to which the command belongs', 'oneOf': [{'const': 'bitmap'}, {'const': 'cluster'}, {'const': 'connection'}, {'const': 'generic'}, {'const': 'geo'}, {'const': 'hash'}, {'const': 'hyperloglog'}, {'const': 'list'}, {'const': 'module'}, {'const': 'pubsub'}, {'const': 'scripting'}, {'const': 'sentinel'}, {'const': 'server'}, {'const': 'set'}, {'const': 'sorted-set'}, {'const': 'stream'}, {'const': 'string'}, {'const': 'transactions'}]} On instance['gcrasetvalue']['group']: 'rate_limit' ``` ##### `corrupt-dump-fuzzer.tcl` Also fixed `: Fuzzer corrupt restore payloads - sanitize_dump: yes in tests/integration/corrupt-dump-fuzzer.tcl` Failed daily test : https://github.com/redis/redis/actions/runs/24485248057/job/71558533312#step:6:8652 ```shell Server crashed (by signal: 0, err: key "gcra" not known in dictionary), with payload: "\x1C\x0A\x02\x5F\x37\xC0\x06\xC0\x00\x02\x5F\x39\xC0\x08\x02\x5F\x33\x02\x5F\x35\x02\x5F\x31\xC0\x02\xC0\x04\x0E\x00\xA9\x71\xBF\xEE\x6F\x46\xEF\xA6" violating commands: Done 1434 cycles in 600 seconds. RESTORE: successful: 601, rejected: 833 Total commands sent in traffic: 1194776, crashes during traffic: 1 (0 by signal). [: Fuzzer corrupt restore payloads - sanitize_dump: yes in tests/integration/corrupt-dump-fuzzer.tcl Expected '1' to be equal to '0' (context: type eval line 155 cmd {assert_equal $stat_terminated_in_traffic 0} proc ::test) [147/147 done]: integration/corrupt-dump-fuzzer (1201 seconds) ``` ### Changed This change completes the necessary updates across all relevant components to ensure consistent handling of the rate_limit group and restores CI stability.
…ctLongLatOrReply() (redis#14995) In addReplyErrorLength and addReplyErrorFormatInternal, `-ERR` is automatically prepended if the message doesn’t start with `-`, so the initial `-ERR` is unnecessary. Also, trailing `\r\n` will be trimmed, so it doesn’t need to be included. --------- Signed-off-by: charsyam <charsyam@naver.com> Signed-off-by: DaeMyung Kang <charsyam@gmail.com> Co-authored-by: debing.sun <debing.sun@redis.com>
Update RediSearch module version to 8.8 RC1 (v8.7.90) Made with [Cursor](https://cursor.com) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: a single version bump that changes which RediSearch git tag is cloned/built; main risk is build/runtime incompatibility from the upstream RC update. > > **Overview** > Updates the RediSearch module build configuration to fetch and build upstream `redisearch` tag `v8.7.90` (8.8 RC1) instead of `v8.5.90`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 21e121c. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…is#15061) ## Root cause Roughly 50% of random double scores generated by the ZADD listpack workload have 17-19 significant digits, which exceed `MAX_MANTISSA_FAST_PATH` (`2^53`). These inputs fall through to the `strtod()` fallback: ```c char static_buf[128]; memcpy(buf, nptr, len); /* memcpy back! */ buf[len] = '\0'; /* null-term */ double result = strtod(buf, ...); /* glibc strtod — ~10× slower on ARM */ ``` The original C++ `fast_float` library handled the same 17-19 digit inputs with Eisel-Lemire / bigint arithmetic without falling back to `strtod()`. That is what the pure-C replacement lost. ## Fix Compute `mantissa * 10^exponent` in 128-bit integer arithmetic using `__uint128_t`, then convert to double with a single IEEE round-to-nearest-even cast. Supported for `|exp| in [0, 19]` where `10^|exp|` fits in `uint64`; cases outside that range (or otherwise outside the fast path's preconditions) still fall through to `strtod()`. --------- Co-authored-by: debing.sun <debing.sun@redis.com>
Several `addReplyError` and `addReplyErrorFormat` calls in `xnackCommand` included a redundant `"ERR "` prefix in the message string. Since `addReplyErrorLength` already prepends `-ERR ` to the RESP reply, clients received `ERR ERR ...` for these error paths. This PR removes the redundant prefix from all five affected calls and tightens the corresponding test patterns to match from the beginning of the error message (`"ERR ..."` instead of `"*...*"`), so any future double-prefix regression will be caught.
RM_RegisterClusterMessageReceiver() unlinks a receiver node from the clusterReceivers[type] linked list when the callback is set to NULL, but when removing the head node (prev == NULL), the code updates clusterReceivers[type]->next instead of clusterReceivers[type] itself. This leaves clusterReceivers[type] pointing to the freed node, so any later traversal through clusterReceivers[type] dereferences a dangling pointer. Fix by updating clusterReceivers[type] directly when prev == NULL. Fixes redis#15057 --------- Co-authored-by: debing.sun <debing.sun@redis.com>
### Problem In `scanGenericCommand`, `maxiterations = count * 10` overflows when `count > LONG_MAX / 10`, causing undefined behavior. ### Changed 1. Use saturating arithmetic to prevent overflow. 2. Added a test to trigger the overflow path, detectable by UBSan.
) **Summary** Detects and rejects corrupt stream RDB payloads where the same NACK (pending entry) is referenced by more than one consumer, which violates a stream data-structure. **Changes** - **`rdbLoadObject` (stream consumer PEL loading)**: Added a guard that checks `nack->consumer != NULL` before assigning the consumer pointer. When a second consumer's PEL references a NACK that was already claimed by a prior consumer, the loader now reports a corrupt RDB error and aborts instead of silently overwriting the pointer. Without this check, two consumers share the same `streamNACK`, and freeing the first consumer's PEL leaves the second with a dangling pointer. - **`corrupt-dump.tcl`**: Added a regression test that crafts a stream with two consumers (`consumerA`, `consumerB`) whose PELs both reference the same entry (`1-0`). The `RESTORE` command is expected to fail with `"Bad data format"`, and the server must remain responsive (`PING` succeeds). **Benefits** - **Fail-fast on corrupt data**: The invariant violation is caught at load time with a clear diagnostic message rather than manifesting as a crash later during normal operation. - **Regression coverage**: The crafted payload in the test ensures this class of corruption is permanently guarded against.
) `raxRecursiveFree` and `raxRecursiveFreeWithCtx` used C call-stack recursion to walk the entire radix tree during `raxFree`. On trees with pathologically deep paths (long keys with no shared prefixes) this could overflow the thread stack and crash the process. This PR replaces both recursive functions with a single unified iterative helper (`raxFreeNodesWithCallback`) that maintains an explicit heap-allocated `raxStack` — the same stack structure already used elsewhere in the rax code (e.g. `raxIterator`). The helper accepts both callback variants (with and without a user-supplied context) so the two public entry points `raxFreeWithCallback` and `raxFreeWithCbAndContext` now both delegate to it. Child pointers are now enumerated forward from `raxNodeFirstChildPtr` instead of backward from `raxNodeLastChildPtr`, which is simpler and consistent with how the rest of the codebase traverses children. No functional change: every node is still visited exactly once, its optional data callback is still invoked before the node is freed, and `rax->numnodes` is decremented identically.
…is#15065) ## Motivation With the append-only pointer vector (`vec`) introduced in redis#15039, the SCAN keys collection path is a natural consumer: `scanCallback` pushes each key to a `list` via `listAddNodeTail`, which allocates a `listNode` (~48 bytes + jemalloc overhead) per key. `scanGenericCommand` then iterates the list once to emit replies and frees every node plus the list itself. For `SCAN COUNT 500`, that is ~500 node allocations + frees on the hot path of a single command. This PR replaces that with `vec` — a 256-element stack buffer covers typical `COUNT` values without any heap allocation, and larger scans grow to a single heap allocation via `vecPush`. ## Change Single-file diff in `src/db.c` — ~30 touched lines, net +6 LOC: - `scanData.keys`: `list *` → `vec *` - `scanCallback`: `listAddNodeTail(keys, key)` → `vecPush(keys, key)` - `scanGenericCommand`: - `listCreate()` → stack-backed `vec` with 256-element `keys_stack[]` - Reply loop: `listFirst / listNodeValue / listDelNode` → `vecGet` index loop - The old `listSetFreeMethod(keys, sdsfreegeneric)` was only active for ZSET (which allocates temporary sds for scores) and listpack paths; we track that via a `free_collected` flag and do an explicit `sdsfree` loop before `vecRelease`. The listpack early-return paths (OBJ_SET, listpack, listpack_ex) call `vecRelease(&keys)` directly since they never called the callback. - `#include "vector.h"` added No algorithmic changes — SCAN cursor iteration, pattern matching, expiry filtering, type filtering and reply formatting are unchanged. ## Benchmarks Run via `redis-benchmarks-specification` on `x86-aws-m7i.metal-24xl` (Intel Sapphire Rapids) and `arm-aws-m8g.metal-24xl` (Neoverse-V2 Graviton4). `unstable` baseline is `n=5`; PR is `n=2-3` on commit `56458ce42` (the first push of this branch — the rebased commit `6e4aff26f` is a no-op rebase over upstream, identical tree). ### x86-aws-m7i.metal-24xl | Test | unstable | PR | Δ | |------|---------:|---:|--:| | `memtier_benchmark-1Mkeys-generic-scan-count-10-incremental-iteration` | 176,929 (n=5) | 181,185 (n=3) | **+2.4%** | | `memtier_benchmark-1Mkeys-generic-scan-count-10-incremental-iteration-high-cursor-count` | 157,405 (n=5) | 164,025 (n=3) | **+4.2%** | | `memtier_benchmark-1Mkeys-generic-scan-count-50-incremental-iteration` | 99,770 (n=5) | 110,862 (n=2) | **+11.1%** | | `memtier_benchmark-1Mkeys-generic-scan-count-100-incremental-iteration` | 61,722 (n=5) | 71,445 (n=3) | **+15.8%** | | `memtier_benchmark-1Mkeys-generic-scan-count-500-incremental-iteration` | 18,994 (n=5) | 22,594 (n=2) | **+19.0%** | | `memtier_benchmark-1Mkeys-generic-scan-count-500-pipeline-10` | 25,677 (n=5) | 35,442 (n=2) | **+38.0%** | | `memtier_benchmark-1Mkeys-generic-scan-pipeline-10` | 824,033 (n=5) | 920,415 (n=2) | **+11.7%** | | `memtier_benchmark-1Mkeys-generic-scan-type-pipeline-10` | 764,420 (n=5) | 852,255 (n=2) | **+11.5%** | | `memtier_benchmark-1Mkeys-generic-scan-cursor-count-500-pipeline-10` | 15,264 (n=5) | 19,688 (n=2) | **+29.0%** | | `memtier_benchmark-1Mkeys-generic-scan-cursor-pipeline-10` | 491,250 (n=5) | 564,721 (n=2) | **+15.0%** | ### arm-aws-m8g.metal-24xl | Test | unstable | PR | Δ | |------|---------:|---:|--:| | `memtier_benchmark-1Mkeys-generic-scan-count-10-incremental-iteration` | 195,917 (n=5) | 204,520 (n=3) | **+4.4%** | | `memtier_benchmark-1Mkeys-generic-scan-count-10-incremental-iteration-high-cursor-count` | 177,644 (n=5) | 182,682 (n=3) | **+2.8%** | | `memtier_benchmark-1Mkeys-generic-scan-count-50-incremental-iteration` | 103,337 (n=5) | 118,119 (n=2) | **+14.3%** | | `memtier_benchmark-1Mkeys-generic-scan-count-100-incremental-iteration` | 66,199 (n=5) | 77,436 (n=3) | **+17.0%** | | `memtier_benchmark-1Mkeys-generic-scan-count-500-incremental-iteration` | 18,869 (n=5) | 21,790 (n=2) | **+15.5%** | | `memtier_benchmark-1Mkeys-generic-scan-count-500-pipeline-10` | 27,621 (n=5) | 38,585 (n=2) | **+39.7%** | | `memtier_benchmark-1Mkeys-generic-scan-pipeline-10` | 789,621 (n=5) | 893,041 (n=2) | **+13.1%** | | `memtier_benchmark-1Mkeys-generic-scan-type-pipeline-10` | 725,833 (n=5) | 878,881 (n=2) | **+21.1%** | | `memtier_benchmark-1Mkeys-generic-scan-cursor-count-500-pipeline-10` | 11,061 (n=5) | 13,996 (n=2) | **+26.5%** | | `memtier_benchmark-1Mkeys-generic-scan-cursor-pipeline-10` | 411,119 (n=5) | 483,889 (n=2) | **+17.7%** | Pattern is consistent across both architectures: gains scale with `COUNT` (more keys collected per call → more `listNode` allocations avoided). The ~+40% peak on `count-500-pipeline-10` is where the per-call allocator overhead dominated the previous implementation. No test regresses. Every delta is positive. ## Tests - `./runtest --single unit/scan` — 0 exceptions - `./runtest --single unit/type/hash` — 0 exceptions (exercises HSCAN path) ## References - **redis#15039** — @moticless introduced `vec` (append-only pointer vector with optional stack-backed storage). - **redis#14958** (Subkey notification for hash fields, @ShooterIT) is the first in-tree consumer of `vec`; this PR is a second, small consumer.
…ing (redis#15118) ## Summary Follow-up to redis#15065. The merged code calls `vecReserve(&keys, count)` where `count` is user-supplied. A client can pass a giant `COUNT` (e.g. `HSCAN k 0 COUNT 10000000000000`) and the server pre-allocates the corresponding pointer slots before any work happens — ~80 TB on a 64-bit build. Pre-reserve DoS surface flagged in code review. ## Fix Drop the pre-reserve entirely. The vec already starts on a 256-pointer stack buffer and grows-by-doubling driven by **actual cardinality** of the dictionary, not by user-supplied `COUNT`. ## Why drop the pre-reserve (vs cap it) The pre-reserve doesn't pay measurable performance — `vecPush()`'s grow-by-doubling path is amortized O(1) and the dominant cost on SCAN workloads is the per-entry callback work, not vector growth.
Made-with: Cursor <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only change; no code or runtime behavior is affected, but it changes the official intake channels for vulnerability reports. > > **Overview** > Updates `SECURITY.md` to redirect vulnerability reporters from emailing the core team to using the **Redis Vulnerability Disclosure Program** link, with GitHub’s *Report a Vulnerability* as an alternative. > > Adds a dedicated security contact email (`security@redis.com`) for questions and includes brief rationale for the new reporting path. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit eeaa8c4. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Fixes checkPrefixCollisionsOrReply() to return 0 (failure) on any provided-prefix self-overlap, instead of accidentally returning a non-zero loop index for overlaps found after the first prefix. Signed-off-by: Raj Danday <rajkripal.danday@gmail.com>
Optimize SET key value GET propagation rewriting in setGenericCommand() by removing GET arguments in-place with rewriteClientCommandArgument(). This avoids the overhead of allocating a new argv vector and incrementing reference counts for every retained argument. The optimization is scoped to the no-expire SET ... GET rewrite path. It also adds test coverage for cases with repeated GET tokens to ensure robust string semantics and consistent replication behavior. Changes: - Use rewriteClientCommandArgument(c, j, NULL) for in-place removal. - Eliminate redundant argv allocations and refcount increments. - Improve performance of SET GET in high-throughput write streams.
redis#15042) Following redis#14890 ## Problem RM_GetUserUsername() documents that the returned RedisModuleString can be freed via automatic memory management, but it always creates the string with ctx=NULL so it cannot be tracked by RedisModule_AutoMemory. Modules following the documentation may leak memory. ## Fix Fixes `RedisModule_GetUserUsername` to accept a `RedisModuleCtx *` and create the returned `RedisModuleString` with that context, allowing RedisModule auto-memory management to track/free it as documented.
We have released 8.8 and 8.10, but don't update the supported versions.
Followup redis#15374 The keysizes histogram rows were indexed by bare integer literals in keysizesHistRow(), with a static_assert guarding that the row count and the mapping stay in sync. Replace them with a KEYSIZES_ROW_* enum whose last member defines MAX_KEYSIZES_ROWS, so adding or removing a row updates the count automatically and the assert is no longer needed. Also drop OBJ_TYPE_BASIC_MAX, which existed only to derive MAX_KEYSIZES_ROWS.
Fixes redis#15472. ## Problem After a failover, a sibling replica of the old master can stay attached to it. Since the old master has itself just become a replica of the new one, the observer ends up as `replica -> old master -> new master`, and it does not recover on its own. Re-pointing a replica at the new master happens in `clusterUpdateSlotsConfigWith()`, which also holds the sub-replica safeguard added in redis#10489. That function is only reached when the sender advertises slots we currently attribute to someone else: ```c dirty_slots = memcmp(sender_master->slots, hdr->myslots) != 0; ... if (sender && clusterNodeIsMaster(sender) && dirty_slots) clusterUpdateSlotsConfigWith(sender, senderConfigEpoch, hdr->myslots); ``` An observer replica can learn about the failover from the **demoted master** first, before it ever sees the new master's slot claim. The demotion handling in `clusterProcessPacket()` calls `clusterMoveNodeSlots(sender, master)`, which hands the slots over immediately. By the time the new master's packet arrives, both views agree, `dirty_slots` is 0, and `clusterUpdateSlotsConfigWith()` is never called — so neither the reconfiguration branch nor the safeguard runs. It cannot be repaired from the outside either: `UPDATE` is only sent to a node that *advertises* slots the sender knows a higher-epoch owner for. Once the observer's master owns no slots, the observer advertises an empty slot map and drops out of the set anyone would correct. ## How the ordering happens in practice The new master broadcasts a PONG to every node the instant it wins the election, so its news should normally arrive first. Merging the logs of the three nodes from the production incident (all `02:09:17.xxx`): ``` .138 B Failover election won -> clusterBroadcastPong(ALL) fires .141 A Configuration change detected -> reconfigures as replica of B <- broadcast applied, +3ms .175 C A failover occurred ... lost 5461 slot(s) <- via A's packet; B's claim never arrived ``` The broadcast reached the old master in 3ms but never reached the observer in the window that mattered. `clusterBroadcastPong()` walks the node table once with three silent skip conditions (`!node->link`, handshake, not in the dict) and nothing retries or re-sends the promotion to a node that missed it, so a single lost or delayed packet on that one path is enough. Our monitoring showed no RST, no TCP errors, and no drop in cluster link counts — nothing beyond ordinary packet loss is required. ## Fix Run the same sub-replica safeguard after the role-switch handling in `clusterProcessPacket()`: ```c clusterNode *grandmaster = nodeIsSlave(myself) && myself->slaveof ? myself->slaveof->slaveof : NULL; if (grandmaster && clusterNodeIsMaster(grandmaster) && grandmaster != myself && !(server.cluster_module_flags & CLUSTER_MODULE_FLAG_NO_REDIRECTION)) { serverLog(LL_NOTICE, "I'm a sub-replica! ..."); clusterSetMaster(grandmaster); clusterDoBeforeSleep(...); } ``` Design notes: - It runs after the whole role-switch block rather than inside the demotion branch, so it also recovers a node that learned of the demotion before it knew the new master, and the case where the old master moved to another shard. Since it re-checks on every packet, a missed opportunity is retried on the next one instead of stranding the node. - `clusterNodeIsMaster(grandmaster)` makes sure we only follow a node we still believe is a master. A stale message the enclosing code chose to ignore (its epoch guard skips the slot move but still records `sender->slaveof`), or a chain that has not settled yet, can otherwise present a slot-less "grandmaster"; without this check we would tear down a working replication link to resync from it. - The `CLUSTER_MODULE_FLAG_NO_REDIRECTION` gate matches the early return in `clusterUpdateSlotsConfigWith()`, so modules keep the same contract. - In a healthy cluster the condition is unreachable: Redis Cluster does not allow configuring chained replication (`CLUSTER REPLICATE` rejects a replica target), so a master-flagged grandparent only exists in the broken state this repairs. ## Affected versions Reachable from 7.4 onwards, and on 7.2 from **7.2.8**, where redis#13055 was backported (`bdc78175b`, distinct from the original `28976a900`). Before that commit the demotion path called `clusterDelNodeSlots()`, leaving the slots unassigned; the new master's later packet then still produced `dirty_slots = 1` and the redis#10489 safeguard recovered the node. redis#13055 removed the transient slot loss of redis#13018, but in doing so also removed the signal the safeguard depended on. 8.x may be more exposed than 7.2.x: `clusterUpdateSlotsConfigWith()` sets `CLUSTER_TODO_BROADCAST_PONG` there (absent on 7.2.x), so the old master announces its demotion immediately rather than on the next gossip cycle, making its news more likely to win the race.
…edis#1391) `tlsProcessPendingData()` iterates `pending_list` using a `listIter`, which pre-caches the `next` node pointer on every `listNext()` call. This cached pointer can dangle and be dereferenced after the node it points to has been freed, causing a use-after-free and a server crash (SIGSEGV). The issue occurs because `tlsHandleEvent()` runs the connection's read handler, which can execute a command (e.g. `CLIENT KILL`) that closes a *different* pending TLS connection. That close path goes through `freeClient()` → `connClose()` → `connTLSClose()`, which calls `listDelNode()` and frees the victim connection's `pending_list` node. If the iterator's cached `next` pointer referenced that node, the following `listNext()` reads freed memory. The `listNext()` contract only permits removing the *current* node, not arbitrary other nodes. Replace the `listIter`-based iteration with a detach-from-head, bounded drain so that no list node pointer is ever held across a handler call: - Re-read `listFirst()` on each iteration instead of relying on a pre-cached `next` pointer - Detach the head via `tlsPendingRemove()` *before* calling `tlsHandleEvent()`, so the loop always makes forward progress - Semantics are preserved: in the common case each connection is handled exactly once per cycle, in order (cherry picked from commit 98ff29b2828bf3245167b416ee23e6797f551a37)
Redis has a memory corruption bug in the RDB loader’s SLOT_INFO handling. A malicious RDB can provide invalid slot metadata that makes Redis use a slot id outside the normal cluster slot range while preparing per-slot dictionaries during load. With the right heap layout, that out-of-bounds slot lookup can be made to land on attacker-controlled data that was also loaded from the RDB. Redis then treats that data as a real dictionary structure The attacker can uses this to build a fake dictionary and fake callback table in memory, then reaches code execution as the Redis process. The cluster-bus INTERNALSECRET issue is used to get an internal Redis connection and make the target replicate from an attacker-controlled fake master. The fake master then serves the malicious SLOT_INFO RDB. The actual RCE primitive is the RDB-loader bug. Source review shows the same SLOT_INFO loader pattern in Redis 7.4.x through 8.8-m03; I verified full RCE on 8.6.2. The issue is that Redis trusts SLOT_INFO metadata from the RDB too much. During RDB load, Redis reads slot_id, slot_size, and expires_slot_size from the file, then passes the slot id directly into kvstore dictionary expansion. Just check the slot_id is in range. (cherry picked from commit 7262f3c2d9b0adbc1c15eaa551e1ccaba37b29f6)
…erialized` (illegal `max-level` / resource risk) (redis#1464) Fixed missing node level validation when reading from RDB. Also fixed a bug in `random_level()` which caused it to generate a level with value equal to `HNSW_MAX_LEVEL`. From the code I infer `HNSW_MAX_LEVEL` is exclusive cap (although I think only @antirez can confirm) so I fixed random_level to generate at most `HNSW_MAX_LEVEL - 1` - otherwise we could have cause OOB read inside `InsertContext->level_queues`. (cherry picked from commit 10c0238643ec2049afd5e8303afed4a30f296399)
VREM_RedisCommand mutated the HNSW graph without first waiting for background VSIM threads to drain, allowing a use-after-free: VREM on the main thread could free a node while a VSIM background pthread was still holding a reference to it, crashing the server with SIGSEGV. Add the missing vectorSetWaitAllBackgroundClients() call, matching the pattern already used by vectorSetInsert (VADD update path) and VSETATTR_RedisCommand. Adds a concurrent VSIM/VREM regression test. See MOD-16035. (cherry picked from commit 268919540bcc22fde1b3ba2c4d81f2486f72c1b6)
hnsw_search() / hnsw_search_with_filter() / hnsw_ground_truth_with_filter() all return int and may return -1 on error (e.g. k == 0, allocation failure). The result was stored in an unsigned int, so -1 coerced to UINT_MAX and drove the result loop past the end of the neighbors/distances allocations. Make 'found' signed, clamp negative returns to 0, and update the loop iterator to match. The negative return path is not currently reachable from a healthy client, but the type signature should hold up against future callers and any non-VREM way of producing a degenerate search state. See MOD-16035. (cherry picked from commit 124d206b19021da572b22ad6837ab767987ffa20)
When `tls-auth-clients` is configured to derive the Redis ACL username from a client certificate field (`client_auth_user = CN`), `tlsGetPeerUsername()` extracts the certificate's Common Name via `getCertFieldByName()` and uses it to build the ACL username. `X509_NAME_get_text_by_NID()` copies the *entire* CN — including any embedded NUL bytes — into the output buffer, but the old code passed that buffer to `sdsnew()`, which computes length with `strlen()` and therefore stops at the first NUL. A CA-signed certificate whose CN is `admin\0innocent` was truncated to `admin`, so a client whose certificate was only meant to identify `innocent` could authenticate as the privileged `admin` ACL user. This is a certificate-based authentication bypass / user-impersonation issue. Build the username from the certificate field in a fully binary-safe way so the value can never be silently truncated at an interior NUL: - Refactor `getCertFieldByName()` to return a newly allocated, binary-safe `sds` (or `NULL` on failure) instead of writing into a caller-provided `char` buffer. - Capture the return value of `X509_NAME_get_text_by_NID()`, which is the true field length, and construct the string with `sdstrynewlen(buf, len)` rather than relying on `strlen`. The embedded NUL is preserved instead of dropped. - `tlsGetPeerUsername()` now uses this `sds` directly; on failure it logs and returns `NULL` so the connection fails authentication. As a result, a CN of `admin\0innocent` is passed to the ACL layer in full and never collapses to `admin`. Since no such ACL user exists, the connection does not gain `admin` privileges and the full binary-safe username is recorded verbatim in the ACL log (reason `tls-cert`). Adds a regression test in `tests/unit/tls.tcl` that mints a CA-signed certificate with an embedded NUL in the CN (`admin\0innocent`) by patching and re-signing the DER, then asserts that: - the connecting client is **not** authenticated as `admin` (`ACL WHOAMI` returns `default`), and - the ACL log contains a single `tls-cert` failure whose `username` is the full `admin\0innocent` string, proving it was never truncated to `admin`. (cherry picked from commit 10da07edeecda2ddcc844acf07f26c1decb66447)
…s#15623) ## The problem Whole-key active expiration wraps each expiry in an execution unit, so post-notification jobs a module queues from the resulting keyspace notification are drained before the cycle returns: ```c /* expire.c -- activeExpireCycleTryExpire() */ enterExecutionUnit(1, 0); deleteExpiredKeyAndPropagate(db, keyobj); exitExecutionUnit(); postExecutionUnitOperations(); /* drains queued jobs */ ``` The hash-field equivalent has no such drain. `activeSubexpires()` calls `estoreActiveExpire()` directly, and `hashTypeExpire()` fires `hexpired` (and `del` when the hash empties) from inside it. `propagateHashFieldDeletion()` does wrap each field deletion in its own execution unit and drain at the end of it, but those per-field drains run *before* the `hexpired`/`del` notifications fire, so they cannot pick up the jobs those notifications queue. A per-key job queued from those notifications therefore stays on the queue until the tail of the *next* command's `call()`. A client command issued in between is served while the module has not yet reacted to the expiry — and that command's own `call()` tail then drains the job, so the effect is self-correcting and visible for exactly one command. That last part makes it easy to miss: a retry always looks fine. This matters for the documented use case of `RM_AddPostNotificationJobForKey`, whose contract says the callback should maintain module-attached key metadata via `RM_SetKeyMeta` / `RM_GetKeyMeta`. A module keeping such metadata in sync with field expiry serves one stale read per expiry. Incidentally, `postExecutionUnitOperations()`'s own doc comment already uses active expiry as its worked example of an execution unit. ## Reproducing With the in-tree `tests/modules/postnotifications_perkey_metadata.so`: ``` HSET hk f v g w HPEXPIRE hk 100 FIELDS 1 f PKMETA.RESET <wait ~500ms, sending nothing> PKMETA.FIRECOUNT ``` `0` before this change, `1` after. Field `g` outlives `f` so the hash survives its field's expiry and the job can attach metadata. ## The fix A single `postExecutionUnitOperations()` after the `estoreActiveExpire()` walk in `activeSubexpires()`, with **no** execution unit wrapped around the walk: ```c estoreActiveExpire(db->subexpires, slot, &info); postExecutionUnitOperations(); ``` The drain on its own is sufficient, and wrapping the walk would be actively harmful: - By the time the walk returns, `propagateHashFieldDeletion()`'s per-field units have all opened and closed, so nesting is already zero and the queued jobs are drainable as-is. - An outer unit would instead keep nesting non-zero for the whole walk, turning each of those per-field drains into a no-op and batching the per-field `HDEL`s into one `MULTI`/`EXEC` — a propagation change well outside the scope of this fix. An earlier revision of this PR did wrap the walk; see the review thread on `src/expire.c`. Keeping the drain outside the walk also keeps it clear of the estore iteration: a regular (non per-key) job is allowed to write to the keyspace, which could otherwise mutate the `subexpires` structure while it is being walked. Per-field propagation is therefore unchanged from before. The only added work on the hot path is one `postExecutionUnitOperations()` per `activeSubexpires()` batch — per db/slot per cron tick, not per field — which early-outs when nothing is pending.
…dates (redis#15671) The legacy cluster implementation directly accessed the internal asmTask to mark a task as done after applying a slot configuration update. This change looks up the corresponding task ID and reports ASM_EVENT_DONE through the common clusterAsmProcess() API. This keeps ASM task internals encapsulated and aligns the legacy cluster implementation with the interface used by other cluster implementations.
`getKeysUsingKeySpecs()` computed `last = first + (numkeys-1) * step` before validating `numkeys`, so a huge client-supplied value (e.g. `LONG_MAX`) overflowed a signed `long`, causing undefined behavior. This PR fixes redis#15403 by rejecting `numkeys` larger than the remaining argument count before computing `last`.
## Summary - add a tls-groups configuration for TLS named group selection - apply the option when creating or reconfiguring the OpenSSL TLS context - add matching --tls-groups support for redis-cli and redis-benchmark when supported by the build ## Why Fixes [redis#15502](redis#15502) Redis already exposes TLS protocol, cipher, and ciphersuite configuration, but there is no equivalent option for the OpenSSL supported groups list. Some deployments need to control this list as part of their TLS policy. The implementation uses SSL_CTX_set1_groups_list when available and falls back to SSL_CTX_set1_curves_list for older OpenSSL headers. If neither API is available, Redis fails the TLS build by default so the option is not silently ignored. Defining TLS_NO_GROUPS compiles the feature out. For redis-cli and redis-benchmark, --tls-groups follows the --tls-ciphersuites behavior: the option is only shown and accepted when the build supports OpenSSL named group configuration. --------- Signed-off-by: AliasJeff <zhexunchen@gmail.com>
…latforms (redis#15689) ### Issue Failed daily CI : >*** [err]: TLS: Verify tls-groups with disjoint groups in tests/unit/tls.tcl Expected 'Could not connect to Redis at 127.0.0.1:27121: SSL_connect failed: ssl/tls alert handshake failure child process exited abnormally' to match '*sslv3 alert handshake failure*' (context: type eval line 10 cmd {assert_match {*sslv3 alert handshake failure*} $e} proc ::test) Cleanup: may take some time... OK Error: Process completed with exit code 1. CentOS reports `ssl/tls alert handshake failure`, so the test failed.
sundb
force-pushed
the
fix-repl-alsopropagate
branch
3 times, most recently
from
August 24, 2026 06:34
11af719 to
cf40b96
Compare
sundb
force-pushed
the
fix-repl-alsopropagate
branch
from
August 24, 2026 06:39
cf40b96 to
d832bbc
Compare
Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
This reverts commit 2753556.
…edis#15018) ## Summary On x86_64 Linux, Redis's hardware TSC clock path was gated behind a compile-time `USE_PROCESSOR_CLOCK` flag. Without it, Redis falls back to `clock_gettime(CLOCK_MONOTONIC)` — a syscall that costs ~50-100ns per invocation vs ~5-10ns for RDTSC. This is already the default on ARM (Generic Timer), but x86 users had to opt in manually. Additionally, the existing x86 calibration parsed the CPU "model name" field for a GHz string, which fails on CPUs that don't include a frequency in that field. **This change:** 1. Removes the `USE_PROCESSOR_CLOCK` compile-time gate on x86_64 Linux 2. Enables HW TSC by default when `constant_tsc` is present in `/proc/cpuinfo` flags 3. Replaces the fragile GHz regex parsing with runtime calibration: measures RDTSC ticks over a 10ms `clock_gettime` interval at startup 4. Falls back to POSIX clock if `constant_tsc` is absent **Interpretation.** The improvement concentrates exactly where the theory predicts: topologies where multiple io-threads contend on `clock_gettime` (8/12/16 io-threads) show consistent +6.4% to +8.9% gains with non-overlapping confidence intervals across 3 runs. Low-thread-count topologies (standalone, 2/4 io-threads) are flat — no regressions, but no measurable win either, since the syscall-per-command cost isn't the bottleneck at low concurrency. **ARM:** No change expected and none observed — ARM already uses the HW clock path (Generic Timer / `CNTVCT_EL0`) by default. This PR only affects the x86_64 Linux path. The underlying mechanism is the existing `call()` optimization at `server.c:3910-3935`, which skips `ustime()` / `gettimeofday()` when a HW monotonic clock is available. By flipping x86 TSC on by default (when `constant_tsc` is reported), that fast path is taken on Intel Sapphire Rapids and equivalents without the user having to rebuild.
When a replica is reconfigured to follow a master in another shard, its cached replication history belongs to the old shard. Before completing the first full sync with its new master, the replica may still participate in failover using that invalid history and can be promoted with stale or unrelated data. The same risk exists when a sub-replica is flattened after its master moves across shards. This change captures the cross-shard transition before updating the shard ID, discards the cached master, and resets repl_down_since to its initial value of zero. This treats the node like a newly configured replica that has never synchronized with its current master. As a result: Until the first synchronization with the new master succeeds, the replica reports offset zero. When cluster-replica-validity-factor is zero, the replica remains eligible for failover but is ranked behind every eligible replica with an established offset. This preserves availability while reducing its chance of being promoted. Resetting `repl_down_since` to zero treats the replica as having never connected to its current master. With a non-zero `cluster-replica-validity-factor`, the existing data-age check prevents it from starting an automatic failover until its first synchronization succeeds. The tests cover automatic replica migration, explicit CLUSTER REPLICATE, and sub-replica flattening with both zero and non-zero validity factors. This PR also follows up our comment in redis#15530 (comment). Valkey redis#885 and its follow-up Valkey redis#944 drop the cached master, and let a replica report zero offset to delay its election and reduce its chance of being promoted. Besides the tests are based on valkey's. --------- Co-authored-by: Binbin binloveplay1314@qq.com
## MULTI/EXEC keys ACL permissions were not checked on EXEC `execCommand()` rechecks the ACLs of every queued command before running it, so that permission changes made while the transaction was parked are honoured. Since 235e688 the key half of that check examined nothing: a client could queue commands while a key pattern was granted, and still read and write those keys after an operator revoked the pattern with `ACL SETUSER u resetkeys ...`. The same commands, issued fresh on the same connection, were correctly refused. The key set used by the check is cached per pending command, and during a transaction the client's current pending command is `EXEC`, not the queued command being executed. `EXEC` has no keys, but its cached key result is flagged valid, so `ACLCheckAllUserCommandPerm()` seeded its cache with an empty key set, `ACLSelectorCheckCmd()` skipped key extraction, and `ACLSelectorCheckKey()` was never reached. The check returned `ACL_OK` having examined no key at all, which also made the dedicated error in `execCommand()` unreachable. Command permissions were still rechecked correctly, and the scripting call site was unaffected since the engine's fake client has no pending command. `getClientCachedKeyResult()` and `ACLCheckAllPerm()` now take the `pendingCommand` whose keys are to be checked, instead of reading `c->current_pending_cmd`, and `execCommand()` passes the queued command. The cached result records key positions within a specific command's argv, so making it a caller obligation to hand over the command that `c->cmd` / `c->argv` were populated from keeps the two from drifting apart again. A `debugServerAssert()` in `ACLCheckAllPerm()` now checks that invariant. The lazy `preprocessCommand()` call is limited to the command currently being read from the client. Commands queued in a MULTI were already preprocessed when they were parsed, and re-running preprocessing on one of them at EXEC time would look the command up again and overwrite `pcmd->cmd` after `execCommand()` had already copied it into `c->cmd`. A queued command whose cached result was invalidated (by a module command filter, which clears the pending command flags) now falls back to extracting keys from `c->argv`. Note that `keys_result` records which argv positions hold keys, not an authorization decision, so the selectors are still evaluated as they stand at EXEC time. Added a test that revokes a key pattern between queueing and `EXEC`, asserting that both a queued read and a queued write are refused, that the written value is left untouched, and that the refusals are recorded in `ACL LOG` with `context=multi` and `reason=key`. The existing test covering this scenario revokes a command rather than a key pattern, which is the half that kept working, so it passed regardless. ## Fixing lazy preprocessing when a module changes command keys Since redis#14440 the command, keys, slot and read error are preprocessed at parse time, but a command filter may insert/delete/replace arguments afterwards. `moduleCallCommandFilters()` only reset part of that state, so what was derived from the pre-filter argv — most notably the slot — could still be used later. Today this is masked in practice: the only consumer, `getClientCachedKeyResult()`, re-preprocesses lazily whenever the cached result was invalidated. ## How it is fixed Instead of relying on that lazy path, track whether a filter actually changed argv, and if so run `preprocessCommand()` right away on the new argv and sync `c->lookedcmd` / `c->slot` / `c->read_error` from it. If no filter changed argv there is nothing to refresh and we return early. The state is now always consistent at the point it is produced, which also removes the lazy preprocessing from `getClientCachedKeyResult()` (it asserts instead). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: debing.sun <debing.sun@redis.com>
…5634) ## Issue The coverage CI can fail with a negative count when multiple IO threads update the same non-atomic gcov counter at the same time. It may also warn about an "unexecuted block on non-branch line with non-zero hit count" when gcov maps both executed and unexecuted blocks to the same source line. ## Change 1. Fixes an error: Added -fprofile-update=atomic to prevent negative gcov counts caused by IO thread race conditions. 2. Cleans up warnings: Enabled geninfo_unexecuted_blocks=1 to properly handle partially executed lines with multiple basic blocks. --------- Co-authored-by: debing.sun <debing.sun@redis.com>
Co-authored-by: Ozan Tezcan <ozan.tezcan@redis.com>
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.