mirrors/valkey
0
0 Fork 0
mirror of https://github.com/valkey-io/valkey.git synced 2026-10-28 08:24:42 +08:00
Code Activity
14,221 Commits 45 Branches 61 Tags
unstable
Commit Graph
14221 Commits
This Branch
This Branch
All Branches
Author SHA1 Message Date
Quanye YangRain ValentineRoshan Khatri
 		255018c000 Fix module blocking of RESP3 Pub/Sub clients (#4863) 
moduleBlockClient() treated every subscribed client as unblockable because getClientType() returns CLIENT_TYPE_PUBSUB. A RESP3 client can still run commands while subscribed, so ValkeyModule_BlockClient and ValkeyModule_BlockClientOnAuth returned NULL with ENOTSUP and queued no reply. Allow blocking those clients again, matching the processCommand rule that only RESP2 Pub/Sub clients are restricted. Keyspace notifications still cannot block them.

Introduced by PR 1819 (8.1.1). Restores the pre-8.1.1 behavior for RESP3 subscribed clients; no change for RESP2 clients or keyspace-notification contexts.

Fixes #4862

---------

Signed-off-by: quanyeyang <quanyemostima@gmail.com>
Signed-off-by: Rain Valentine <rsg000@gmail.com>
Co-authored-by: Rain Valentine <rsg000@gmail.com>
Co-authored-by: Roshan Khatri <117414976+roshkhatri@users.noreply.github.com>
 2026-10-27 12:26:02 -07:00
Dante Knowles ee247dbd4b [Bug Fix] Fix fast-fail double-counting a NACK voter that later fails (#4655) 
The election bound subtracted a voter twice if it NACKed and was then
marked FAIL, resetting elections that were still winnable. Track
ACK/NACK per voter per election instead and add unit tests.

Fixes #4626

---------

Signed-off-by: Dante Knowles <xdk@amazon.com>
 2026-10-27 15:09:45 +01:00
Jim BrunnerRain Valentinevalkey-review-bot[bot]
 		692e57e6f7 strengthen forkless PEXPIRE test (#5049) 
Missed this test improvement when submitting
https://github.com/valkey-io/valkey/pull/4996

---------

Signed-off-by: Jim Brunner <brunnerj@amazon.com>
Signed-off-by: Rain Valentine <rsg000@gmail.com>
Co-authored-by: Rain Valentine <rsg000@gmail.com>
Co-authored-by: valkey-review-bot[bot] <282604435+valkey-review-bot[bot]@users.noreply.github.com>
 2026-10-26 23:14:08 -07:00
pirate329 25d3b7d9e7 Match listpack for lex range commands on mixed-score btree zsets (#4954) 
Fixes #4950

On a btree zset with more than one distinct score, the lex range
functions in `ordered_index.c` packed the bounds with the score of the
smallest member, so they only looked at the lowest score bucket.
`ZLEXCOUNT`, `ZREMRANGEBYLEX`, `ZRANGEBYLEX`, `ZREVRANGEBYLEX`, `ZRANGE
... BYLEX` and `ZRANGESTORE ... BYLEX` could all return something
different from listpack. `ZREMRANGEBYLEX` and `ZRANGESTORE` are
replicated as is, so a primary and replica holding the key in different
encodings could diverge.

For keys with more than one score, these now follow listpack:
- count and delete use the run in score order starting at the first
element >= min, up to the first element after it > max (delete removes
it by rank)
- forward range seeks start at the first element >= min, reverse at the
last element <= max scanning from the end (like `zzlLastInLexRange`),
then apply the LIMIT offset; the caller already stops at the far bound

Single-score keys keep the existing O(log N) paths.

I matched listpack rather than the 9.1 skiplist where they differ: 9.1's
skiplist can disagree with itself, e.g. `ZADD z 0 b 0 d 1 a 1 c 2 e`
with `[a [c` gives `ZLEXCOUNT` 4 but removes 1, while listpack gives 1
for both.

Known trade-off: on mixed-score keys, finding the start of a lex range
is a linear walk in score order, so it costs O(N) in the worst case,
while the 9.1 skiplist and unstable did an O(log N) descent (with
inconsistent results on mixed scores). It shows up most when members
that are next to each other in lex order share a score, e.g. @charsyam
measured `ZRANGEBYLEX ... LIMIT 0 10` from the middle of 20k/30k-member
keys going from about 0.7 µs to 59/88 µs. Stepping one score at a time
instead of one member at a time would cut this down and can be a
follow-up.

Tests: fixed mixed-score cases for count/delete and for forward/reverse
ranges with LIMIT and ZRANGESTORE on both encodings, plus a fuzz test
checking listpack and btree agree on random mixed-score sets for all of
these. They fail without the change. I also ran 5000 random cases
locally with sets up to 600 members.

---------

Signed-off-by: Aman Kumar <amankumar020003@gmail.com>
 2026-10-26 23:12:44 -07:00
Madelyn Olson 938007fd98 Only warn about a command line password when -a or -u set it (#4886) 
Since #3402, `valkey-cli` reads environment variables before it parses
command line options, so options can override them. The "Using a
password with '-a' or '-u' option on the command line interface may not
be safe" warning only checks that a password is set, so a password from
`VALKEYCLI_AUTH` or `REDISCLI_AUTH` alone now prints it, even though
nothing was on the command line. The fix warns only when `-a` or `-u`
replaced the password the environment provided. `-a` still warns and
still overrides the environment variable.

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
 2026-10-27 10:48:41 +08:00
Roshan Khatri b4c125082a Fix cluster Pub/Sub length truncation (#5030) 
**Summary**

Fixes
[GHSA-f4q9-j22m-xqq9](https://github.com/valkey-io/valkey/security/advisories/GHSA-f4q9-j22m-xqq9),
a heap buffer overflow in cluster Pub/Sub message construction reported
through ZDI

`clusterCreatePublishMsgBlock()` calculated message lengths using
`size_t` but passed the result through a 32-bit length field. An
oversized channel and message could therefore cause a much smaller
buffer to be allocated before copying the full payload.

**Changes**
- Validate the complete serialized message length before allocation or
32-bit conversion.
- Reject oversized `PUBLISH` and `SPUBLISH` requests before publishing
locally or propagating to the cluster.
- Adds focused tests for valid and oversized payloads.

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
 2026-10-26 12:10:45 -07:00
charsyam e50b62d0bb [correctness] Fix LSET modifying elements for out-of-range 64-bit indexes (#4823) 
## Problem

  On 64-bit builds, `LSET` parses its index as `long`, but
  `listTypeReplaceAtIndex()` narrowed it to `int`. An out-of-range index
such as `4294967296` became `0`, causing `LSET` to return `OK` and
modify
  the first element instead of returning `ERR index out of range`.

This is a correctness issue affecting both listpack and quicklist lists.

  ## Fix

  Preserve the `long` index through `listTypeReplaceAtIndex()` so an
  out-of-range index returns an error without modifying the list. Add
regression tests for large positive and negative indexes in both
encodings.

  ## Tests

  - `./runtest --single unit/type/list --quiet`: 289 passed, 0 failed.
- Confirmed the command returns `ERR index out of range` and leaves the
    list unchanged after the fix.

  On 32-bit builds, these values are rejected when parsing the `long`
  index; the narrowing bug occurs on 64-bit builds.

---------

Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
 2026-10-26 09:32:10 -07:00
charsyam e23cab1355 Optimize B+tree ZSCAN with item references and prefetch (#4856) 
For B+tree encoded sorted sets, ZSCAN now collects OrderedIndexItem *
pointers while scanning the hashtable and writes members and scores to
the reply directly from those items. This removes intermediate SDS
allocations, copies, and frees for returned members and scores. The
pointers stay valid because the reply is built within the same command
and hashtable rehashing does not move items.

Reply shape and cursor semantics are unchanged; the listpack path is
untouched.

When no pattern filter is active (MATCH absent or MATCH *), the scan
callback only prefetches each item instead of reading its SDS length,
and the reply reads the item once. With a pattern, the callback must
read the member to filter it, so prefetch is skipped; the item-reference
savings still apply, including with NOSCORES. Builds without prefetch
support simply skip prefetching.

Tests cover binary members, MATCH, NOSCORES, and MATCH with NOSCORES
over complete cursor traversals at COUNT 10, 100, and 1000.

Performance

Common setup: AMD Ryzen 7 8845HS, gcc 13.3, default build (-O3 -flto),
baseline 1db8edcbe (unstable), and a fixed hash-seed. Times are
server-side ZSCAN time from INFO commandstats per complete cursor
traversal; they exclude client parsing and network time. Results are
the median of five runs with 10 traversals per run, server restarts
between runs, and rotated version order. Lower is better. Change is the
time difference; "faster" is baseline / patch - 1, the equivalent
throughput gain, computed from unrounded medians.

General ZSCAN: 100k members, member = "member:<i>", score = i * 1.5.
The server was pinned to CPU 2 and the client to CPU 4.

| Case                        | Baseline | Patch   | Change                |
| --------------------------- | -------: | ------: | --------------------- |
| COUNT 10                    | 35.1 ms  | 23.7 ms | -32.7% (48.5% faster) |
| COUNT 100                   | 28.1 ms  | 20.0 ms | -28.9% (40.6% faster) |
| COUNT 1000                  | 29.4 ms  | 20.7 ms | -29.6% (42.0% faster) |
| COUNT 100 MATCH member:*    | 29.2 ms  | 23.2 ms | -20.4% (25.6% faster) |

Compared with an item-reference build without prefetch, prefetch
further reduced traversal time by 5.3%, 4.6%, and 3.3% at COUNT 10,
100, and 1000. At COUNT 100 MATCH member:*, which bypasses prefetch,
the 0.5% difference was within noise. COUNT 1000 MATCH member:1* varied
by 12-17% between runs and is omitted.

MATCH with NOSCORES: 100k members, member = f000000..f099999,
score = i; f00* matches 10k members and f* matches all 100k. The server
was pinned to CPU 0 and the client ran on CPUs 2-5. Member names are
shorter and scores are omitted, so compare absolute times within this
table only.

| Case                             | Baseline | Patch   | Change                |
| -------------------------------- | -------: | ------: | --------------------- |
| COUNT 100 MATCH f* NOSCORES      | 10.33 ms | 6.66 ms | -35.5% (55.1% faster) |
| COUNT 1000 MATCH f* NOSCORES     | 10.08 ms | 6.56 ms | -34.9% (53.5% faster) |
| COUNT 1000 MATCH f00* NOSCORES   | 2.51 ms  | 2.15 ms | -14.4% (16.8% faster) |

Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
 2026-10-26 09:29:24 -07:00
Bara' Hasheesh 7c950b1ed7 Fix CI failures on clang-format due to OS changes on the runner Image (#5029) 
Github actions is moving `ubuntu-latest` from 24.04 to 26.04 

`apt.llvm.org` repo has no `llvm-18` build for 26.04, this in turn is
causing failures when it comes to
[installing](https://github.com/valkey-io/valkey/actions/runs/40344093709)
`clang-format-18`

This PR updates the CI job to be compatible with the new `ubuntu-26.04`

Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com>
 2026-10-26 09:23:14 -07:00
yzc-yzcandRain Valentine de4b2c198a Add static specifier to the internal functions of HLL (#2496) 
Related: #2343

Signed-off-by: yzc-yzc <96833212+yzc-yzc@users.noreply.github.com>
Signed-off-by: Rain Valentine <rsg000@gmail.com>
Co-authored-by: Rain Valentine <rsg000@gmail.com>
 2026-10-26 09:15:27 -07:00
Omer Rubinstein 2c11a10f49 Fix false sharing in IO thread active-time and job-finished counters (#4976) 
Follow-up to #4107, from the false-sharing survey in [this
comment](https://github.com/valkey-io/valkey/pull/4107#issuecomment-6311748165).

### Problem

Two counters in `io_threads.c` that IO threads write on every busy loop
iteration are packed with other threads' data:

- `used_active_time_io_thread[]` stores one `_Atomic long long` per IO
thread, so eight slots share a 64-byte line (sixteen on 128-byte-line
targets), and each IO thread adds to its own slot after every iteration
that did work.
- `io_jobs_finished` is updated by every IO thread after each busy
iteration, and the line it's on also holds main-thread state. On a macOS
clang build its neighbour is `io_jobs_submitted`, which the main thread
increments for every job. On Linux aarch64 with GCC 13 and LTO, its
neighbours are `cur_epoll_thread`, `cluster_io_pending_responses` and
the first `shared_querybuf_registry` slots.

### Fix

Following #4107, each `used_active_time_io_thread` slot and
`io_jobs_finished` are wrapped in a struct aligned to `CACHE_LINE_SIZE`,
and a `static_assert` checks each wrapper is exactly one line. Only
`INFO` reads `used_active_time_io_thread`, so padding it costs nothing
on the read side. It's now `static`; nothing outside `io_threads.c`
referenced it.

This doesn't address the true sharing on `io_jobs_finished` itself,
where all IO threads update the same counter. Fixing that would need
per-thread counters summed by the main thread.

Memory cost: `used_active_time_io_thread` grows from 2 KiB to 16 KiB
with 64-byte lines (32 KiB with 128-byte lines), in `.bss`.

### Testing

- Builds with `-Werror`; `clang-format` 18.1.8 is clean.
- Symbol layout (macOS arm64, 128-byte lines): `io_jobs_finished` is
alone on its line, and the array is 256 × 128 B.
- Removing the `_Alignas` makes both `static_assert`s fail.
- New gtests in `src/unit/test_io_threads.cpp` check, through `testOnly`
accessors, that every `used_active_time_io_thread` slot is cache-line
aligned and one line apart, and that `io_jobs_finished` shares no line
with `io_jobs_submitted` or any slot. They pass on Linux aarch64 (GCC
13, `-Werror`) alongside the existing `ClusterIOOffload` tests, and all
three fail against the unpadded layout.
- `unit/io-threads`, `unit/info` and `unit/introspection` pass. `stats:
client input and output buffer limit disconnections` fails under
`--config io-threads 4` on macOS both with and without this change.

### Performance

Measured by Rain Valentine
([review](https://github.com/valkey-io/valkey/pull/4976#pullrequestreview-5631041699)),
cachecannon GET 16B, 400 connections, 5 × 30s:

- clang 15 + LTO, Sapphire Rapids, where
`io_jobs_submitted`/`io_jobs_finished` share a line with
`cur_epoll_thread` and `cluster_io_pending_responses`: +2.6% throughput
(CI95 ±1.4%) at pipeline 1 / io-threads 16, with main-thread L1d misses
per request down 5%. Pipeline 10 / io-threads 7 unchanged.
- GCC 11 + LTO, Sapphire Rapids and Graviton 4, where the two counters
already sit on separate lines: within ±2% in every cell (io-threads
7/16, pipeline 1/10).

---------

Signed-off-by: Omer Rubinstein <omerrubi@amazon.com>
 2026-10-26 08:28:00 -07:00
Jim Brunner aaeee8d5b1 bgiteration: remove from early-iterate on key-delete 
Signed-off-by: Jim Brunner <brunnerj@amazon.com>
 2026-10-26 08:13:03 -07:00
Avi FeneshAvi FeneshRoshan KhatriSarthak Aggarwal
 		668d875ca4 Link macOS modules with the compiler driver (#4927) 
macOS module builds can fail when raw `ld` reads a libSystem stub from a
different SDK/toolchain:

```
ld: tapi error: malformed file
.../CommandLineTools/SDKs/MacOSX27.0.sdk/usr/lib/libSystem.B.tbd:4:20: error: unknown architecture
arm64e.x1-macos, arm64e.x1-maccatalyst
```

Set `LD = $(CC)` on Darwin in the example and test module Makefiles. The
compiler driver selects the SDK and libSystem. This matches the [8.1 fix
in
#4687](https://github.com/valkey-io/valkey/commit/d3ad04a5d3c759a817a096613d483814883ef977).
Remove the explicit SDK library flags from the Lua Makefile, which
already links through `$(CC)`.

Validation at `b45e0c43b26d026ee87df73dfe69369b43f66b52`:

- [Native macOS latest and macOS 14 builds
pass](https://github.com/avifenesh/valkey/actions/runs/39731006790).
Each builds all 8 example modules, all 50 test modules with `-Werror`,
and the loadable Lua engine. The workflow checks out and asserts the
exact PR commit. `file` and `otool -L` confirm native bundles linked to
libSystem.
- Linux builds pass for the same three module groups, using normal flags
for examples and Lua and `-Werror` for test modules. Linux dry-run
commands are unchanged by the patch.
- Darwin dry runs confirm all module links use the compiler driver
without explicit SDK paths. A command-line `LD` override still works.

The broader upstream CI suite is running. The original macOS failure is
recorded
[here](https://github.com/avifenesh/valkey/actions/runs/39150878403/job/117744099060).

---------

Signed-off-by: Avi Fenesh <aviarchi1994@gmail.com>
Signed-off-by: Avi Fenesh <avifenesh@gmail.com>
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Co-authored-by: Avi Fenesh <avifenesh@gmail.com>
Co-authored-by: Roshan Khatri <117414976+roshkhatri@users.noreply.github.com>
Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
 2026-10-24 10:20:31 -07:00
pirate329 989889a9b2 Send SELECT at the end of the slot migration snapshot (#4952) 2026-10-24 10:04:50 -07:00
Leo MurilloandClaude Opus 5.5 5c6a0cdcf3 Fix ZINTER/ZINTERSTORE score when a weighted term is 0*inf (#4997) 
ZINTER and ZINTERSTORE replaced a NaN weighted score (0inf) with 0 for the first input only. For every later input the product reached zunionInterAggregate() as NaN: with AGGREGATE SUM the isnan guard reset the whole running sum to 0, and with MIN/MAX the comparison is false so the term was dropped. The result therefore depended on input order (inputs are sorted by cardinality) and disagreed with ZUNION, which treats every 0inf product as 0.

Apply the same NaN-to-0 cleanup to the weighted score of every later input in the intersection loop, both the repeated-key branch and the lookup branch.

Behaviour change only for ZINTER/ZINTERSTORE where a weight/score product is NaN on a non-first input: the term now counts as 0, as for the first input and as in ZUNION. During a rolling upgrade an old replica recomputes ZINTERSTORE itself and may store a different score for such input.

Signed-off-by: Leo Murillo <leonardo.murillo@gmail.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
 2026-10-23 15:04:52 -07:00
Jacob Murphy 6902fdc965 Make quicklist read-only iteration non-mutating on compressed nodes (#4782) 
Previously, iterating over an LZF-compressed quicklistNode via
quicklistNext() or quicklistGetIteratorEntryAtIdx() decompressed
node->entry in place, freed the compressed buffer, set node->recompress
= 1, and recompressed node->entry when advancing to the next node or
releasing the iterator.

This caused three issues:
1. Every read-only traversal (such as LRANGE or LPOS) paid the CPU cost
of repeatedly decompressing and re-running LZF compression on every
visited interior node (as well as re-attempting compression on
incompressible RAW interior nodes via quicklistCompress).
2. When bgiteration was active, all readonly quicklist iterations need
to block while the key is being serialized (in use).
3. (not present in our code, but possible) Two interleaved iterators on
the same compressed quicklist hit a use-after-free: iterator A
decompresses node X in place, iterator B lands on X and holds iter->zi
pointing into X->entry, and then A leaves X, recompresses it, and frees
the uncompressed buffer out from under B.


Changes:
- Add decompressed_buf to quicklistIter. When inspecting an
LZF-compressed node we don't touch node->entry, node->encoding, and
node->recompress unless they become modified.
- If a mutating iterator operation (quicklistDelEntry,
quicklistReplaceEntry, _quicklistInsert) is called, promote
iter->decompressed_buf to node->entry with node->recompress = 1. Also
ensure the large-element early return in _quicklistInsert() resets the
iterator consistently with all other insert paths.
- Use quicklistRecompressOnly(iter->current) in quicklistNext() and
quicklistReleaseIterator() so leaving a node only recompresses when the
iterator actually promoted the node for mutation (note: read traversals
no longer opportunistically run __quicklistCompress() as a side effect
on RAW interior nodes that had recompress == 0).

## Benchmark Results

Dataset: `mylist` with 500 elements x 256B (~128 KB),
`list-max-listpack-size -1` (4 KB max per listpack node -> ~34 nodes
total, 32 interior nodes), `valkey-benchmark -c 50`.

### 1. Compressible Quicklist (`list-compress-depth 1`, 32 LZF interior
nodes)

| Operation | `unstable` (Baseline) | `patched` (Non-Mutating Iter) |
Throughput Change | p99 Latency (`unstable` -> `patched`) |
| :--- | ---: | ---: | ---: | ---: |
| `LPOS mylist __nonexistent__` (full 500-elem scan) | 10,248.0 RPS |
28,885.0 RPS | +181.9% (2.82x) | 4.975 ms -> 1.791 ms (2.8x lower) |
| `LRANGE mylist 100 200` (mid-list 101-elem slice) | 29,256.9 RPS |
54,525.6 RPS | +86.4% (1.86x) | 1.959 ms -> 0.759 ms (2.6x lower) |
| `LRANGE mylist 0 -1` (full 500-elem fetch) | 7,054.7 RPS | 11,236.0
RPS | +59.3% (1.59x) | 6.751 ms -> 2.903 ms (2.3x lower) |
| `LINDEX mylist 250` (mid-list single lookup) | 154,321.0 RPS |
172,612.2 RPS | +11.9% (1.12x) | 0.391 ms -> 0.191 ms (2.0x lower) |
| `LSET mylist 250 <256B>` (mid-list write via promote) | 143,678.2 RPS
| 140,845.1 RPS | -2.0% (0.98x, noise) | 0.407 ms -> 0.415 ms
(unchanged) |

### 2. Incompressible Quicklist (`list-compress-depth 1`, 500 x 256B
high-entropy elements, RAW interior nodes)

| Operation | `unstable` (Baseline) | `patched` (Non-Mutating Iter) |
Throughput Change | p99 Latency (`unstable` -> `patched`) |
| :--- | ---: | ---: | ---: | ---: |
| `LPOS mylist __nonexistent__` (full 500-elem scan) | 749.7 RPS |
78,616.4 RPS | +10,386% (104.9x) | 67.455 ms -> 0.695 ms (97x lower) |
| `LRANGE mylist 0 -1` (full 500-elem fetch) | 721.5 RPS | 12,330.5 RPS
| +1,609% (17.1x) | 68.607 ms -> 1.767 ms (38.8x lower) |
| `LRANGE mylist 100 200` (mid-list 101-elem slice) | 3,408.3 RPS |
54,171.2 RPS | +1,489% (15.9x) | 15.647 ms -> 0.535 ms (29.2x lower) |
| `LINDEX mylist 250` (mid-list single lookup) | 52,029.1 RPS |
179,425.8 RPS | +244.9% (3.45x) | 1.023 ms -> 0.175 ms (5.8x lower) |

### 3. Uncompressed Control (`list-compress-depth 0`, 500 x 256B
elements)

| Operation | `unstable` (Baseline) | `patched` (Non-Mutating Iter) |
Throughput Change | p99 Latency (`unstable` -> `patched`) |
| :--- | ---: | ---: | ---: | ---: |
| `LPOS mylist __nonexistent__` | 81,300.8 RPS | 79,617.8 RPS | -2.1%
(0.98x, noise) | 0.679 ms -> 0.695 ms |
| `LRANGE mylist 0 -1` | 12,187.7 RPS | 12,158.0 RPS | -0.2% (1.00x,
noise) | 1.783 ms -> 1.799 ms |
| `LRANGE mylist 100 200` | 53,705.7 RPS | 55,126.8 RPS | +2.6% (1.03x,
noise) | 0.535 ms -> 0.527 ms |
| `LINDEX mylist 250` | 177,514.8 RPS | 180,288.5 RPS | +1.6% (1.02x,
noise) | 0.183 ms -> 0.183 ms |

Signed-off-by: Jacob Murphy <jkmurphy@google.com>
 2026-10-23 14:36:08 -07:00
Muhammad Taha 8b9019c955 Fix crash when a database's keyspace is replaced mid-stage during defrag (#4968) 
Clear `defrag_later` (and reset its cursor) at the start of every DbKeys defrag stage

Signed-off-by: Muhammad Taha <76549451+taham0@users.noreply.github.com>
 2026-10-22 13:53:04 -07:00
cxljs a4e5b695ad chore: update stale skiplist comments to the ordered index(#4911) 
The zset skiplist encoding was replaced by the ordered index, but several comments still referenced it — a stale filename in fbtree.c and "skiplist" wording in the module iterator and the RDB/listpack NAN-score guards. Update them to name ordered_index.c and the actual mechanism (scores packed into a sortable 8-byte key, asserted non-NAN).

Comment-only, no behavioral change.

Signed-off-by: Xiaolong Chen <fukua95@gmail.com>
 2026-10-21 16:05:44 -07:00
Himangshu Deka e283addef5 Hold no descriptor on nodes.conf when rename() publishes it (#4489) 
clusterSaveConfig() calls rename() while the process holds descriptors on both files involved: the temp file it just wrote (the rename source) and the startup lock on nodes.conf (the rename destination, held open for the life of the process by design). Three problems follow. The temp file is closed after the rename and the result discarded, so a deferred write error reported only by close() publishes a truncated nodes.conf and the node panics in clusterLoadConfig() on restart. flock() applies to the inode, not the name, and rename() puts a new inode under nodes.conf on every save, so after the first save the lock guards an unnamed orphan and a second node can start on a config already in use. On SMB, which refuses to replace a file that has an open handle, every save fails and clusterSaveConfigOrDie() exits the process, producing a crash loop on Azure Files.

The lock's own comment stated its precondition: it works because nodes.conf is always rewritten in place. Making the save atomic via temp file plus rename (redis PR 10924, 2022) removed that precondition, and the lock was never revisited.

Fix: hold no descriptor on nodes.conf when the rename happens. Close the temp file and check the result before renaming, so a failed write aborts the save instead of publishing it, as ACLSaveToFile() and rdbSave() already do. Take the startup lock on a sibling nodes.conf.lock instead of on nodes.conf; it is only ever created, never replaced, so its inode is stable and the guard survives any number of saves. The save path no longer touches the lock at all, so there is no window where nodes.conf is unguarded. The shutdown unlock now operates on a stable inode rather than the orphan. Fork children and Solaris (which still does not lock) are unchanged.

Compatibility: on-disk format of nodes.conf is unchanged. nodes.conf.lock is a new, empty, never-removed file in the data directory, documented in valkey.conf. During a rolling upgrade an old node locks nodes.conf and a new node locks nodes.conf.lock, so the two do not exclude each other; little is lost, since the old node's lock is already orphaned after its first save.

Tests: a new test in tests/unit/cluster/misc.tcl forces a save, asserts the inode under nodes.conf changed, starts a second server on the same config file, and asserts it exits with "already used by a different Cluster node"; it fails without this change and is skipped on Solaris. The existing cluster-config-save-behavior test now asserts no temp files are left behind. A failing close() cannot be forced from Tcl and is not covered.

Fixes #2884
 2026-10-21 15:56:08 -07:00
Josh Soref 5f38f9e897 Spelling 4 (#2241) 
- Subset of #2183

---------

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
 2026-10-21 15:06:17 -07:00
Josh Soref 9a029ba836 Fix typos in comments and rename TimedOut/Sparkline functions and tests (#2252) 
- Subset of #2183

---------

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
 2026-10-21 13:11:12 -07:00
Josh Soref 6b8dfd83c6 spelling: cannot (#2242) 
- Subset of #2183

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
 2026-10-21 13:06:44 -07:00
Bara' Hasheesh 7c7436bfc9 Deflake Dual-channel load converges after primary repl-compression test (#4966) 
Fixes #4930

### Timeline

| Event | Timestamp | Elapsed from sync start |

|----------------------------------------------------|----------------|-------------------------|
| Dual-channel full sync starts | `02:03:17.554` | 0.00s |
| Background RDB transfer terminated with success | `02:03:30.259`|
12.70s |
| Synchronization with replica succeeded | `02:03:30.636`| 13.08s |
| Cron fires reconcile, disconnects replica | `02:03:31.509` | 13.96s |
| Partial resync accepted, plaintext — converged | `02:03:31.510` |
13.96s |

The throttled full sync alone consumed ~12.7s, and total convergence was
~14.0s, well past the 10s `wait_for_condition` budget.

### Fix

Increased the `wait_for_condition` budget to 20s (200 100).

Adjusted other tests that may hit the same issue to have similar wait
budget
- Dual-channel full sync with compression delivers writes made during
load
- Dual-channel ZSTD stream did not preserve buffered writes

---------

Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com>
 2026-10-21 09:51:24 -07:00
jjuleslasarteandSarthak Aggarwal 32e3e81faf Speed up HGETALL, HKEYS and HVALS with exact-length headers and fused bulk replies (#4417) 
# Speed up HGETALL, HKEYS and HVALS with exact-length headers and fused
bulk replies

Perf improvement for workloads where many clients fetch moderately-sized
hashes (#3451). This is a rework of #4390, addressing the review
findings raised there.


## Changes

### 1. Exact-length reply headers

RESP requires the element count *before* the elements (`%200\r\n` for a
200-pair map). Previously, `genericHgetallCommand()` did not know the
count up front, so it reserved a placeholder with
`addReplyDeferredLen()` and patched it at the end. Once that placeholder
is there, every subsequent reply byte bypasses the static output buffer
and goes through heap-allocated reply blocks.

#### before

```
  c->reply list:  [placeholder node] --> forces ALL reply bytes into
                        |                heap-allocated clientReplyBlocks
                        v
                  [16KB block] -> [16KB block] -> ...   (malloc'd + freed
                                                         per command)
  c->buf (static 16KB): unused
```

#### after
```
  c->buf (static 16KB): [hdr|f1|v1|f2|v2|...]  <- first ~16KB, zero allocations
  c->reply list:        [16KB block]           <- only the overflow, if any
```

We don't know the count only if fields can expire mid-iteration. When a
hash has no volatile (TTL-bearing) fields, `hashTypeLength()` is exact,
so the real header is emitted immediately. This applies to
listpack-encoded hashes (which cannot hold volatile fields at all) as
well as hashtable-encoded ones without volatile fields, so the default
hash encoding benefits too. A `serverAssert` verifies the emitted count
matches the header. Hashes with volatile fields keep old path.

For the hashtable encoding, entries are additionally iterated in batches
of 16 with prefetching.

#### before

```
BEFORE - serial pointer chase, one stall per entry:
  entry1 --(miss, ~100ns stall)--> emit --> entry2 --(miss)--> emit --> ...

```

#### after

``` 
  Phase 1: collect 16 entry ptrs, prefetch entry structs   -+ misses in
  Phase 2: prefetch their field/value sds buffers           -| flight
  Phase 3: emit all 16 against cache-warm data              -+ concurrently
```

Listpack skips the batching since its entries are in one contiguous
allocation.

### 2. merged bulk string writes (`networking.c`)

`addWritePreparedReplyBulkCBuffer()` now attempts to emit the complete
bulk string (`$<len>\r\n<data>\r\n`) into the client's static reply
buffer with a single bounds check and two memcpy calls, instead of three
separate appends that each re-walk the buffer-or-list routing logic:

#### before

```
  addWritePreparedReplyBulkCBuffer("valuedata", 9)
    +- append "$9\r\n"      --> [checks: replica? deferred? push? list? fits?]
    +- append "valuedata"   --> [same checks again]
    +- append "\r\n"        --> [same checks again]
                                 x 400 bulk strings per HGETALL = 1200 walks

```

#### after

```
  tryAddBulkCBufferFast("valuedata", 9)
    +- bail-outs (replica/deferred/push/encoded/full) -> generic path
    +- reqresSaveClientReplyOffset()        <- before bufpos moves
    +- c->buf: ...[$9\r\nvaluedata\r\n]     <- one fused write
```

It falls back to the generic path for encoded (copy-avoidance) buffers,
deferred replies, push messages, replica clients and when the reply does
not fit in the static buffer.


## Benchmarks

Environment: aarch64/64 cores. I pinned valkey to one core (`taskset -c
2`), `valkey-benchmark` to others to cores 16-47 (`-c 50 --threads 8`).

Baseline is unstable at `917de6ac1`.

### synthetic hashes, TCP

`hth` = hashtable-encoded, 200 fields x 128B values (~27KB reply).
`hlp` = listpack-encoded, 100 fields, small values.

```
=== RUN 1: hgetall hth (hashtable 200fx128B), TCP, -n 200000 ===
unstable:
  throughput summary: 21006.20 requests per second
          avg       min       p50       p95       p99       max
        2.287     0.456     2.095     3.455     3.807     7.247
clean:
  throughput summary: 28502.21 requests per second
          avg       min       p50       p95       p99       max
        1.652     0.304     1.543     2.383     2.551     4.167

=== RUN 2: hgetall hlp (listpack 100f), TCP, -n 200000 ===
unstable:
  throughput summary: 31928.48 requests per second
          avg       min       p50       p95       p99       max
        1.497     0.288     1.455     2.103     2.239     3.223
clean:
  throughput summary: 42069.84 requests per second
          avg       min       p50       p95       p99       max
        1.100     0.112     1.087     1.375     1.551     5.351

=== RUN 3: hgetall hth pipelined, TCP, -P 10 -n 400000 ===
unstable:
  throughput summary: 27517.89 requests per second
          avg       min       p50       p95       p99       max
        6.470     0.504     6.575     8.311    11.743    19.807
clean:
  throughput summary: 33252.97 requests per second
          avg       min       p50       p95       p99       max
        6.481     0.512     6.495     7.359     7.975    16.591
```

`perf stat` on the server process (8s window during RUN 1):

```
unstable:
    12,181,864,694      cycles:u
    56,922,768,381      instructions:u            #    4.67  insn per cycle
       205,615,849      cache-misses:u
        32,479,163      branch-misses:u

clean:
     8,039,421,649      cycles:u
    36,808,357,246      instructions:u            #    4.58  insn per cycle
       224,631,859      cache-misses:u
        11,828,499      branch-misses:u
```

Per request that is roughly 344k -> 163k user instructions (-53%) and
74.5k -> 35.6k user cycles (-52%).

### op's workload

A single client serially HGETALLing every key (a SCAN loop). Hashes with
one `chunk` blob field (10-150KB) plus 99 `metricN` fields (~100B)

```
=== serial scan loop: -c 1 --threads 1 --sequential -r 1000 -n 20000 ===
unstable (run A):
  throughput summary: 13651.88 requests per second
          avg       min       p50       p95       p99       max
        0.038     0.024     0.039     0.047     0.047     0.287
  wall time: 1.47 s; server CPU time consumed: 0.84 s
unstable (run B):
  throughput summary: 13633.26 requests per second
          avg       min       p50       p95       p99       max
        0.037     0.024     0.039     0.047     0.047     0.831
  wall time: 1.47 s; server CPU time consumed: 0.86 s
clean (run A):
  throughput summary: 14947.68 requests per second
          avg       min       p50       p95       p99       max
        0.032     0.024     0.031     0.039     0.039     0.359
  wall time: 1.35 s; server CPU time consumed: 0.73 s
clean (run B):
  throughput summary: 15003.75 requests per second
          avg       min       p50       p95       p99       max
        0.031     0.024     0.031     0.039     0.039     0.311
  wall time: 1.34 s; server CPU time consumed: 0.71 s
```


### Reading the results

| Workload | unstable | this pr | Delta |
|---|---|---|---|
| hashtable hash, 200f x 128B, TCP | 21.0k rps | 28.5k rps | +36% |
| listpack hash, 100f, TCP | 31.9k rps | 42.1k rps | +32% |
| hashtable hash, TCP, -P 10 | 27.5k rps | 33.3k rps | +21% |
| issue shape, serial scan loop (1 client) | 13.6k rps | 15.0k rps |
+10% wall, -15% CPU |


## Interaction with #3212 (listpack TTL support)

#3212 lets listpack-encoded hashes carry TTL-bearing fields, which
touches the assumption behind the listpack inclusion here. The two
changes should work fine together:

- This PR's fast path does not gate on encoding; it gates on
`hashTypeHasVolatileFields(o)`. #3212 updates that same function to
detect
listpack TTLs with an O(1) header peek (`lpIsMetadata(lpStart(...))`),
so after both merge, a listpack hash without TTLs still takes the
exact-header fast path, and one with TTLs automatically falls back to
the deferred-header path (correct, since its fields can expire
mid-iteration).
- If the gate were ever bypassed incorrectly, the `serverAssert` on the
emitted count would fire rather than emitting a malformed reply.
- (AI tells me) the test merge of the two branches auto-merges cleanly
in `t_hash.c` and`networking.c`


## Testing

- All existing hash tests pass (90/90 including the new ones).
- New Tcl tests in `tests/unit/type/hash.tcl` cover:
  - the exact-header path for both encodings in RESP2 and RESP3,
- a reply large enough to overflow the static buffer and spill into the
reply list (exercising the fast-path bail-out mid-command),
  - a hash with volatile fields taking the deferred-header path.
- `clang-format-18` clean.

Fixes #3451

---------

Signed-off-by: jjuleslasarte <jules.lasarte@gmail.com>
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
 2026-10-21 09:48:05 -07:00
Omer Rubinstein 8b73b97349 Fix false sharing in per-thread memory usage counters (#4107) 
Fixes #4111

used_memory_thread[] in zmalloc.c is an array of plain size_t. #1179 padded only the start of the array, so the main thread's counter (index 0, &padded[7]) sits alone at the end of the first cache line, but every later counter is packed eight to a 64-byte line. The bio and I/O threads' counters therefore share lines: each zmalloc/zfree updates its own counter and invalidates the line for the other threads on it. The main thread pays on the read side, since it sums all active counters in zmalloc_used_memory() after every command.

Give each per-thread counter its own cache line by wrapping it in a struct aligned to CACHE_LINE_SIZE. Cost: ~16KB static memory, zero per-op cost. This reverses the layout chosen in #1179; the tradeoff is one cache line fetched per active thread in the aggregation loop (which iterates total_active_threads, not MAX_THREADS_NUM) versus eliminating cross-thread invalidations on every allocation. A static_assert pins sizeof(used_memory_entry) to CACHE_LINE_SIZE, so dropping the alignment attribute fails the build.

With io-threads 16 on dedicated hardware, XADD throughput improves 17.5% and GET 9.8% on x86 c7i (SMT); on Graviton3 (no SMT) XADD is neutral and GET +3.0%. The #1179 regression did not reproduce on either arch. zmalloc_used_memory drops from 5-9% of main-thread self-time to about half that, and cache misses fall 25-45% under the io=16 XADD load. Full tables are on the PR.

The _Atomic fallback for non-x86/ARM/POWER only changes layout, not the memory model; its static_assert was checked with a riscv64 cross-compile.
 2026-10-21 09:46:26 -07:00
Bara' Hasheesh 9d21c19f69 [Test] Add automatic per-test cleanup to the Tcl test framework (#4871) 
Tests that change a config or pause a process have to undo it themselves at the end of the body. When an assertion fails partway, that cleanup is skipped: configs keep the wrong value and paused processes stay paused, so following tests fail or time out and the original failure cascades.

The test harness now undoes this state itself when a test ends. test_config_set sets a config for the duration of one test and restores the old value afterward, pinned to the server's run_id so the restore is skipped if the client no longer reaches that same server. test_defer and test_defer_fail register cleanup scripts that run (always, or only on failure) in reverse order when the test ends. pause_process is now resumed automatically at test end; pause_process_persistent keeps the old cross-test behavior for tests with inter-test dependencies. A cleanup failure fails an otherwise-passing test. cluster-migrateslots.tcl is converted to the new helpers as the first user.

Compatible: existing tests are unaffected; the new helpers are opt-in and pause_process callers that relied on persistence were moved to pause_process_persistent.

---------

Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com>
 2026-10-21 09:17:45 -07:00
Shun Takahashi bc7ceb79a2 Fix grammatical typo "This functions" in code comments (#3728) 
Fix subject-verb agreement in 7 comment lines across 5 files.

In 6 places, the determiner "this" was followed by the plural noun
"functions" while the surrounding verb (`may`, `increases`, `attempts`)
is in third-person singular -- so the noun should be "function".

In `src/rand.c` the verb is plural (`are used`), so the fix there is
"This functions" -> "These functions".

No behavioral changes.

Signed-off-by: takahashi shun <mokopoi44@gmail.com>
 2026-10-21 08:41:04 -07:00
Josh Soref c1ed73bfc0 Fix typos in comments and rename VALKEY_SANITAIZER_* CMake variables (#2250) 
---------

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
 2026-10-21 07:56:04 -07:00
Josh Soref 4a698691cf spelling: ; otherwise, (#2238) 
- Subset of #2183

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
 2026-10-21 07:51:37 -07:00
Sebastian Turner 45ec67c307 Fix HSETEX and HGETEX when the key is named "fields" (#4945) 
HSETEX and HGETEX look for the `FIELDS` token starting at argv[0], so a
key named `fields` (any case) is mistaken for the token and the next
argument is parsed as numfields:

```
> HSETEX fields FIELDS 1 f v
(error) ERR value is not an integer or out of range
```

Start the scan at argv[2], after the command name and the key, the same
way the HEXPIRE family starts after its fixed arguments.

Added tests in tests/unit/hashexpire.tcl for both commands, with and
without expiration options.

Fixes #4944

Signed-off-by: Sebastian Turner <sebastiant3484@gmail.com>
 2026-10-21 07:35:14 -07:00
Shubham S TapleandViktor Söderqvist b936001714 CLI hints: Fix unbalanced ']' in bitops (BITFIELD_RO) command (#3974) 
Fixed the following issue. For the below hint extra/unbalanced ']' was
being suggested in valkey-cli in interactive mode:

```
"BITFIELD_RO k GET " "encoding offset [GET encoding offset ...]]"
"BITFIELD_RO k GET xyz " "offset [GET encoding offset ...]]"
"BITFIELD_RO k GET xyz 12 " "[GET encoding offset ...]]"
"BITFIELD_RO k GET xyz 12 GET " "encoding offset [GET encoding offset ...]]"
"BITFIELD_RO k GET enc1 12 GET enc2 " "offset [GET encoding offset ...]]"
"BITFIELD_RO k GET enc1 12 GET enc2 34 " "[GET encoding offset ...]]"
```

Fix: `arg->matched` flag is being reset inside
`addHintForRepeatedArgument()` function (therefore `if` condition to add
']' evaluated to true), so now in the fix we are evaluating once the
conditions used to add '[' and reusing it to check while adding ']'.

---------

Signed-off-by: ShubhamTaple <subbutaple@gmail.com>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
 2026-10-21 09:12:49 +02:00
Bara' Hasheesh 1074b3cada PERF: remove redundant checks in HINCRBYFLOAT command (#4690) 
### First Enhancement

Currently `HINCRBYFLOAT` uses the standard
`getLongDoubleFromObjectOrReply` utility function to parse input values

This function internally eventually either:
- calls `string2ld` which returns an error in case `NaN` is encountered 
- does a casting chain of `(long double)(long)` which will never produce
a `NaN`

Using these facts we can remove any existing `isnan` check that is done
on the results obtained from this function call, this allows us to
remove the `isnan(incr)` check in `hincrbyfloatCommand`

### Second Enhancement 


After which given that we check `isinf(incr)` in `hincrbyfloatCommand`,
the result of `value += incr` will never be `NaN`

- `incr` is not `NaN` or `±INF`
- `value` is not `NaN`

Hence we can remove the `isnan(value)` value check

---------

Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com>
 2026-10-20 20:15:29 -07:00
Sarthak Aggarwal 7db2df1f79 CI: avoid installing LTTng kernel modules in UST job (#4967) 
The `test-ubuntu-lttng` job fails while installing `lttng-modules-dkms`.
Ubuntu Noble's LTTng 2.14.0 kernel modules cannot
compile against the GitHub runner's new 7.0 kernel, causing `apt` to
fail before Valkey is built or tested.

This PR removes `lttng-modules-dkms` from the job's dependencies.

Valkey uses LTTng-UST userspace tracepoints, so the kernel tracing
modules are not required. The job continues to install `lttng-tools` and
`liblttng-ust-dev`.

- [PR #4417
failure](https://github.com/valkey-io/valkey/actions/runs/39483879798/job/118839179823)
- [Backport sweep for
9.2](https://github.com/valkey-io/valkey/actions/runs/39477196768/job/118816113535)
- [Backport sweep for
9.1](https://github.com/valkey-io/valkey/actions/runs/39450349822/job/118723199243)
- [Dirty-key tracking
PR](https://github.com/valkey-io/valkey/actions/runs/39453195017/job/118733148841)

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
 2026-10-20 14:20:02 -07:00
Roshan KhatriandSarthak Aggarwal 16d3c5d24f Fix Valgrind fork-child IO thread buffer report (#4915) 
Fixes #4932 

## Summary

- keep process-wide references to each I/O thread's reusable query
buffer so forked children retain discoverable roots for inherited
allocations
- set the worker thread ID before registering its buffer so each pointer
is stored in the correct registry slot
- verify each worker clears its registry slot after a successful
shutdown

## Background

After `fork()`, only the calling thread remains in the child. The heap
allocations for the I/O workers' reusable query buffers are inherited,
but their thread-local pointers disappear, causing Valgrind to report
the buffers as definitely lost even though the parent still owns and
eventually frees them.

The registry is non-owning and does not retain additional allocations or
change query-buffer ownership. It only preserves a process-wide pointer
to each current buffer across the fork snapshot.

The registry intentionally does not synchronize allocation with
`fork()`. This leaves a narrow diagnostic race if a fork occurs between
allocating a replacement buffer and publishing it, while avoiding a
production fork-latency dependency on I/O thread scheduling.

This is the focused production-code portion of #4814, with Sarthak
Aggarwal credited as co-author. It intentionally excludes unrelated
Valgrind suppressions and test skips, preserving the existing regression
coverage.

## Testing

- `make`
- `./runtest --single unit/socket-prioritization --clients 1 --verbose
--timeout 2400 --dump-logs` — 12 passed
- `./runtest --single integration/shutdown --clients 1 --verbose
--timeout 2400 --dump-logs` — 7 passed
- `make distclean && make CFLAGS="-DNO_MALLOC_USABLE_SIZE"`
- repeated both targeted suites with `NO_MALLOC_USABLE_SIZE` — 12 passed
and 7 passed

---------

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
 2026-10-20 11:38:51 -07:00
皇心andViktor Söderqvist bc5aae2479 Cache earliest timer for O(1) usUntilEarliestTimer lookup (#4354) 
`usUntilEarliestTimer()` scans the whole unsorted `aeTimeEvent` list on
every event-loop iteration, before each `aeApiPoll()`. This is O(N) per
iteration even when the timer set did not change. With RDMA enabled,
every accepted connection registers a keepalive time event on the main
loop (`src/rdma.c:804`), so N scales with connection count — a few
thousand RDMA connections means a few thousand nodes walked per
iteration.

This PR caches the earliest valid time event in `aeEventLoop` and makes
the lookup O(1), keeping the list itself and all timer semantics unchanged.

Implementation:

- New field `aeEventLoop->earliestTimer` (per-event-loop state; no
  globals, so the multi-loop/multi-thread usage model of ae is unchanged).
- `aeCreateTimeEvent`: replace the cache when the new timer fires sooner
  (also when the cached entry was lazily deleted).
- `aeDeleteTimeEvent` / `processTimeEvents`: invalidate the cache when
  the cached timer is deleted or fires (including the re-scheduled case);
  the next lookup falls back to a single O(N) rescan and re-caches.
- No new configuration (per DEVELOPMENT_GUIDE: controlled entirely by
  heuristics); `AE_DELETED_EVENT_ID` and empty-list semantics preserved.
- A pre-existing NULL-deref found while testing this change (zombie-only
  timer list crashes the rescan) is split into a companion PR — see
  "Related work" below.

Fixes #4352

Signed-off-by: Haoze Xie <royenheart@outlook.com>
Signed-off-by: Yuan Tan <yuantan098@gmail.com>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
 2026-10-20 09:51:11 +02:00
Jacob Murphy f870ab590c Skip INCREX long double edge-case tests under valgrind (#4955) 2026-10-19 21:53:37 -07:00
shanezx-sc b9e22c2715 Fix eviction replication and replica DB selection during atomic slot migration (#4920) 
Fixes #4921

## Problem

With atomic slot migration, an import target that is at `maxmemory`
evicts keys of the slots it owns to make room for the imported ones.
**Those evictions are never replicated**:

1. Before executing each command of the slot import client,
`processCommand()` calls `performEvictions()`. At that point
`server.current_client` is already the import client.
2. Each eviction goes through `propagateDeletion()` → `alsoPropagate()`.
There, the slot migration guard drops everything propagated while
`current_client->slot_migration_job` is set.
3. The guard exists because the import client's stream is proxied to the
replicas verbatim. Evictions are a local decision of the target, so they
are not part of that stream.
4. Replicas don't evict (`replica-ignore-maxmemory yes`). So they keep
every key their primary evicted, and grow by about the imported volume
while the primary stays at `maxmemory`.

When the importing shards are full, as in a scale-in, replicas can grow
well past `maxmemory` within a minute and get OOM-killed. Their resyncs
then cause further pressure. A failed import (`Ran out of memory (OOM)
during slot import`) leaks the same way: the primary evicted everything
it owned before failing, so it ends with no keys while its replicas keep
all of them.

The bug was introduced with atomic slot migration (#1949) and is present
in `unstable` and 9.0.

Local reproduction: ASM into a 64 MB `allkeys-lru` primary with one
replica, repeated 3 times.

| | evictions on target | target keys | replica keys | keys only on
replica |
|---|---|---|---|---|
| ASM | ~35,000 | 47,483 | 82,305 | **34,822**, all in the target's own
slots |
| legacy `MIGRATE`, same fill | ~34,300 | 48,203 | 48,203 | 0 |
| ASM, import fails with OOM | ~48,900 | 0 | 48,937 | **48,937** |

## Fix

### 1. Replicate evictions done while a slot import client is current

Only suppress the propagation while a command of an **import** client is
executing:

```c
if (cc != NULL && cc->slot_migration_job && isImportSlotMigrationJob(cc->slot_migration_job) &&
    server.executing_client != NULL) {
```

`call()` sets `executing_client` for the whole call tree, around its own
`alsoPropagate()`, so the guard still covers exactly what the proxied
stream contains. Anything propagated while the import client is current
but no command is executing, such as `performEvictions()` in
`processCommand()`, is now replicated like on any other primary.
`_addReplyToBufferOrList()` already uses `executing_client` in the same
way to tell evictions apart from command execution.
`c->flag.executing_command` can't be used instead, because it is cleared
before `call()` propagates the command.

Checked:
- **No double propagation.** I went through every `propagateDeletion()`
caller:
  - lazy expiry is skipped for import clients;
- active expiry, `delKeysInSlot()` and the beforeSleep cleanup all run
without a current client.
- **SYNCSLOTS ESTABLISH/FINISH** are still suppressed. Their
`forceCommandPropagation()` runs inside `call()`.
- **Ordering.** Each eviction is flushed as its own execution unit
before the command's bytes are proxied.
- **The pause assertion in `propagateNow()`** can't be reached, because
evictions are paused whenever replica writes are.
- **AOF** behaviour is unchanged.

### 2. Keep replicas in the right DB around a proxied slot import stream

The target feeds its replicas two streams: the proxied import stream,
and its own commands via `replicationFeedReplicas()`. The latter now
includes evictions. `replicationFeedReplicas()` tracks the replicas'
selected db in `server.replicas_eldb`, but the proxied stream can select
a db itself, since the snapshot selects each db that has keys, and
nothing accounts for that. With `cluster-databases > 1`, this sends
commands to the wrong db on the replicas:
- a write or eviction in db 1 is applied to db 2 if the proxied stream
last selected db 2;
- the next proxied command for db 2 is applied to db 0 if the target
last selected db 0 for its own writes.

The fix:
- track on the import client (`repl_applied_dbid`) the db the import
stream is in;
- before proxying what the client applied, select that db for the
replicas if needed (`replicationFeedStreamFromSlotImport()`);
- after proxying, set `replicas_eldb` to the db the stream leaves them
in.

`replicationFeedStreamFromSlotImport()` asserts that we are a primary:
in cluster mode a primary can only be demoted by `clusterSetPrimary()`,
which unlinks the import client from its job
(`clusterUpdateSlotImportsOnOwnershipChange()`) right after
`replicationSetPrimary()`.

Without this, the first commit would make multi-db clusters worse:
evictions in another db would switch the replicas' db in the middle of
an import. It also fixes the existing case of normal client writes to
other dbs during an import.

### 3. Replicate evictions done while a slot export client is current

The old guard matched any slot migration client, so it also covered the
source's export client. While processing its commands (e.g. `SYNCSLOTS
ACK`), `processCommand()` can evict, and the guard then dropped
`PROPAGATE_REPL`, which `propagateNow()` uses both for
`replicationFeedReplicas()` and `clusterFeedSlotExportJobs()`. Those
evictions reached neither the source's replicas nor the target, so a key
evicted in a migrating slot could survive on the target. The export
client no longer matches the guard (commit 1). Its own commands
propagate nothing, so nothing else changes for it.

## Compatibility

- **With a single db:** at most an extra `SELECT 0` in the replication
stream, e.g. after a full sync resets `replicas_eldb`.
- No config or protocol change.

## Tests

New tests in `tests/unit/cluster/cluster-migrateslots.tcl`. Each checks
that the primary and replica have matching `DBSIZE` per db and matching
`DEBUG DIGEST` once their offsets meet, and that no resync happened. A
primary at maxmemory can evict on any command, including the ones
checking it, so the target's own keys have a TTL and are evicted with
`volatile-random` (the imported keys are never evicted), and the primary
doesn't evict while it is compared with its replica.

| test | `unstable` | commit 1 only | this PR |
|---|---|---|---|
| Evictions on a full import target are replicated | ❌ replica 547 vs
primary 285 keys | ✅ | ✅ |
| Evictions on an import target that runs out of memory are replicated |
❌ replica 457 vs primary 0 keys | ✅ | ✅ |
| Writes in other DBs during a slot import are replicated to the right
DB | ❌ db 0: replica 2 vs primary 1 | ❌ db 0: replica 2 vs primary 1 | ✅
|
| Evictions in another DB than the imported keys are replicated to the
right DB | ❌ db 1: replica 484 vs primary 221 | ❌ db 1: replica 398 vs
primary 222 | ✅ |

With this PR, the 4 tests also passed 20 times in a row with 4 test
clients pinned to 2 CPUs, and 20 times with 8 unpinned test clients.

Regression runs, Linux x86_64, `BUILD_TLS=yes`:

| suite | result |
|---|---|
|
`unit/cluster/{cluster-migrateslots,multidb,slot-stats,cluster-flush-slot,slot-migration,many-slot-migration}`
| 228 ok (includes the 4 new tests) |
| `unit/{maxmemory,expire,hashexpire,multi}` | 673 ok |
|
`integration/{replication,replication-2,replication-3,replication-4,replication-psync,replication-buffer,dual-channel-replication,block-repl}`
| 269 ok |
| `unit/moduleapi/{propagate,postnotifications,hooks}` | 65 ok |

With only the first commit, the full `cluster-migrateslots` suite also
passes (117 ok). `clang-format` makes no changes.

This should be backported to 9.0, 9.1 and 9.2, which have the same
guard.

---------

Signed-off-by: Shane Zhang <xzhang14@snapchat.com>
 2026-10-19 15:44:43 -07:00
Sarthak Aggarwal 319db007f3 CI: update FreeBSD daily test image (#4922) 
The FreeBSD daily job currently uses FreeBSD 13.2, whose package
repository is no longer available. This causes the job to fail during
package installation, before Valkey is built or tested.

This updates `cross-platform-actions/action` to v1.6.0 and uses FreeBSD
14.5.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
 2026-10-19 15:09:15 -07:00
Bara' Hasheesh 2a23bcda07 Derive the ZRANGEBYLEX LIMIT offset test value from arch_bits (#4948) 
Fixes #4931

### Problem 

The `ZRANGEBYLEX with LIMIT offset across leaves` test passed a
hardcoded `9223372036854775807` offset, which exceeds `LONG_MAX` on
32-bit builds and is correctly rejected with `ERR value is out of
range`, failing the daily `test-ubuntu-32bit` job.

### Fix

Compute the offset from `[s arch_bits]` so it stays within `LONG_MAX` on
both word sizes.

Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com>
 2026-10-19 13:08:25 -07:00
zackcamSarthak Aggarwalvalkey-review-bot[bot]
 		8b362e17c5 Redacting when hide_user_data_from_log is set for after error reply (#4815) 
When a replica, primary, AOF loading client or slot migration link sends
an error reply, afterErrorReply() logs the full error text. For example,
the "unknown command" error copies up to 128 bytes of arguments, so keys
and values can end up in the server log even when
hide-user-data-from-log is enabled. This change logs "*redacted*" in
place of the error text when hide-user-data-from-log is on. The command
name is still logged, and the error is still counted in errorstats.
Nothing changes when the config is off, and the reply sent over the
connection is the same either way.

Testing: added a test in tests/integration/replication.tcl. A fake
replica sends an unknown command with an argument, and the test checks
the primary's log. With the config off the argument appears. With it on
the log shows "*redacted*" and the argument does not appear.

<img width="661" height="184" alt="image"
src="https://github.com/user-attachments/assets/bc42c56b-7b5e-4bb5-ad4a-f706e0cf0ae8"
/>

---------

Signed-off-by: Cameron Zack <zackcam@amazon.com>
Signed-off-by: Sarthak Aggarwal <sarthakaggarwal97@gmail.com>
Co-authored-by: Sarthak Aggarwal <sarthakaggarwal97@gmail.com>
Co-authored-by: valkey-review-bot[bot] <282604435+valkey-review-bot[bot]@users.noreply.github.com>
 2026-10-19 13:02:20 -07:00
xiaocj bce52d39fa Return empty result for a negative LIMIT offset in zset range commands (#4637) 
Part of #4619 (item 4).

**Problem**

`zrangeGenericCommand` accepted any `long` as the LIMIT offset. On
btree-encoded keys the offset flows into `orderedIndexSeekToScoreRange`
/ `orderedIndexSeekToLexRange` as `reverse ? -offset - 1 : offset`,
where a negative value *is* the reverse-direction encoding. The cursor
ended up positioned for `prev()` while the caller stepped with `next()`
(or vice versa), so exactly one element outside the window was returned;
listpack returned an empty array.

```
ZADD z 1 a 2 b 3 c 4 d 5 e
ZRANGEBYSCORE z 1 5 LIMIT -2 10      -> btree: e     listpack: (empty array)
ZREVRANGEBYSCORE z 5 1 LIMIT -2 10   -> btree: a     listpack: (empty array)
ZADD l 0 a 0 b 0 c 0 d 0 e
ZRANGEBYLEX l - + LIMIT -2 10        -> btree: e     listpack: (empty array)
```

**Fix**

Keep `getLongFromObjectOrReply()` when parsing the LIMIT offset so
negative offsets remain accepted without error (matching Redis and
listpack behaviour), and check `offset < 0 || offset >=
(long)zsetLength(zobj)` at the start of both
`genericZrangebyscoreCommand()` and `genericZrangebylexCommand()` to
emit an empty result (`0` for `ZRANGESTORE`) and return directly.


**Tests**

`ZRANGEBYSCORE/ZRANGEBYLEX with a negative or past-the-end LIMIT offset`
in `tests/unit/type/zset.tcl`, both encodings: empty array for negative
offsets across `ZRANGEBYSCORE`, `ZREVRANGEBYSCORE`, `ZRANGEBYLEX`,
`ZREVRANGEBYLEX`, and the `ZRANGE BYSCORE/BYLEX/REV` forms; the last
element, the first past-the-end offset, and a far past-the-end offset
for both range types; `ZRANGESTORE` with a negative or past-the-end
offset returning `0` without creating the destination, using hash-tagged
keys (`dst{zset}`) so the suite runs in cluster mode. `unit/type/zset`
passes locally in standalone and cluster mode.

---------

Signed-off-by: Xiao Chuanjiao <xiaocj@gmail.com>
 2026-10-19 11:11:08 -07:00
Quanye Yang f2e5af9f17 Restore hex float parsing for sorted set scores (#4875) 
Fix #4825

accept C99 hex-floats like 0x10. Write commands propagate the client
argv unchanged, so a 9.0 primary can commit a hex score that a 9.1
replica rejects.

Fall back to strtod when a token starts with 0x/0X so rolling upgrades
and 9.0 AOF replay keep the same accept set.

---------

Signed-off-by: quanyeyang <quanyemostima@gmail.com>
 2026-10-19 10:23:44 -07:00
xiaocjandvalkey-review-bot[bot] e8ddf8d0bc Fix module zset range steps re-returning the current element on btree keys (#4645) 
Part of #4619 (item 1). 

**Problem**

The module range API treats `current` as an element and steps to its
neighbour, while the ordered index iterator keeps its cursor *between*
items (`orderedIndexNext` returns `values[current_index++]`,
`orderedIndexPrev` returns `values[--current_index]`). The btree
branches of `VM_ZsetRangeNext` / `VM_ZsetRangePrev` call
`orderedIndexNext` / `orderedIndexPrev` directly, so a change of
direction re-returns `current`. With `ZADD k 1 a 2 b 3 c 4 d 5 e` and
`VM_ZsetFirstInScoreRange(key, 2, 4, 0, 0)`:

| Calls | btree | listpack |
|---|---|---|
| Next, Next, Prev, then CurrentElement | `b`, `c`, then `c` again |
`b`, `c`, `b` |
| Prev right after First: return / EndReached / current | `1` / `0` /
`b` | `0` / `1` / `b` |
| Next right after `VM_ZsetLastInScoreRange` | returns `d` again | steps
past `d` |

Lex ranges go through the same code (`zsetInitLexRange`) and behave the
same. A module walking with the documented `while
(!VM_ZsetRangeEndReached(key))` loop sees duplicates or one phantom
element whenever it changes direction, and the outcome depends on the
key's encoding.

**Fix**

`src/module.c`: the zset iterator state records which side of `current`
the ordered index cursor sits on (`fwd`, set by First/Last). Next/Prev
step over `current` first when the direction changes, and put the cursor
back next to `current` when the neighbour is outside the range, so any
later step stays correct.

VM_ZsetRangeEndReached now reflects the most recent step on both encodings: set when a step finds nothing, cleared by a step that moves (zsetInitLexRange also defers clearing er until zsetParseLexRange succeeds). Previously the flag stayed set once reached, which is invisible to a one-way walk but makes the documented while (!EndReached) loop stop immediately after any reversal. Compatibility: a module that reverses direction after hitting an end and re-checks the flag now sees 0 after a successful step. The old behaviour was never documented, so this is treated as a bug fix (TSC, 2026-10-19). Not backported.

The iterator model itself is unchanged: every command path scans one way
and relies on the cursor-between-items semantics, so the adaptation
lives in the module layer, which is the only element-centred consumer.

**Tests**

- `tests/modules/zset.c`: `ZSET.WALK` accepts `e` in its pattern to
report `VM_ZsetRangeEndReached`.
- `tests/unit/moduleapi/zset.tcl`, for both encodings: walking to the
end and back (`nnnppp`, and `nnnnnpppppn` reaching both edges of the
key), reversing without revisiting (`npnp`), stepping back from the
first element and continuing (`pnn`), walking backwards from the last
element (`ppp`, `np`), and the end-reached flag after each step
(`ennnepeppene`, plus an empty range).

---------

Signed-off-by: Xiao Chuanjiao <xiaocj@gmail.com>
Signed-off-by: xiaocj <xiaocj@gmail.com>
Co-authored-by: valkey-review-bot[bot] <282604435+valkey-review-bot[bot]@users.noreply.github.com>
 2026-10-19 09:42:54 -07:00
Alisina Karimi e401d52d72 Reject bare "(" and empty string in zset score ranges (#4054) 
zslParseRange accepted a bare `(` as a score, silently treating it as
`(0`, and an empty string as `0`. The `(` branch parses a zero-length
number, which returns 0.0 with the end pointer already sitting on the
terminator, so the existing `eptr[0] != '\0'` check passes.

Guard both branches with a length check so `(` with no number, and an
empty string, return "not a float" like any other invalid score. Affects
ZRANGEBYSCORE/ZREVRANGEBYSCORE/ZCOUNT/ZRANGESTORE.

Closes #3483.

---------

Signed-off-by: AlisinaDevelo <Alisinakarimi.2003@gmail.com>
 2026-10-19 17:16:36 +02:00
pirate329 fbb5ab63bb Copy expired-but-unreaped fields in hashTypeDup (#4939) 
For a hashtable encoded hash, hashTypeDup() iterates with validation on,
so COPY drops fields whose TTL has passed but that active expiry has not
reaped yet. A replica applies the same COPY with field expiry ignored
and keeps them. The primary's copy never had them, so it never reaps
them or propagates their deletion, and the replica keeps them for as
long as the link stays up.

Ignore TTL while copying, the same way #4704 does for listpack
conversion. The listpack branch already copies everything. The reaper
then deletes the expired fields from the copy and propagates the
deletion.

---------

Signed-off-by: Aman Kumar <amankumar020003@gmail.com>
 2026-10-19 18:00:39 +03:00
Daniel Frankcom 9fdda3152e Fix broken link in ValkeyModule_ReplyWithAttribute docs (#4845) 
The comment links to `https://valkey.io/topics/protocol#attribute-type`,
but the protocol page has no attribute type section. This change links
to the page itself, like the other existing RESP3 links in `module.c`.

Signed-off-by: Daniel Frankcom <frankcom@amazon.com>
 2026-10-19 15:15:38 +02:00
WooSeung KimandRan Shidlansik 7f72724dc2 Fix untracked expired hash fields after listpack-to-hashtable conversion (#4894) 
Follow-up to #4704.

Converting a listpack hash to a hashtable tracks its fields in the
volatile set with a validating iterator. The first
`hashTypeTrackEntry()` switches the hashtable to the TTL-validating
type, so the iterator then skips the remaining expired-but-unreaped
fields that #4704 now carries over. A hash with two such fields, or one
plus another field with a TTL, ends up with an untracked field.

That field is never reaped. It either shows up again on the primary
while the replica hides it, or stays hidden but counted by HLEN. Writing
to it crashes the server (HSET fails `vsetUpdateEntry(...) == 1`, HDEL
can hit a freed volatile set). Any converting write, such as a value
over 64 bytes, before active expiry reaches the key triggers it.

Iterate with `HASHTABLE_ITER_SKIP_VALIDATION`, as `rdbSaveObject()`
does. #4704 is on the 9.2 branch but not in a release yet.

---------

Signed-off-by: WooSeung Kim <wooseung.dev@gmail.com>
Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
 2026-10-19 12:50:35 +03:00
Saurabh K 1519066403 Drop WRITE flag from pausetest timer commands to shorten test sleeps (#4817) 
Follow-up to #3645. Speeds up test cases.

Signed-off-by: Saurabh Kher <saurabh@amazon.com>
 2026-10-19 09:21:44 +02:00
Jacob MurphyandRan Shidlansik 741b0159f5 Support LBOUND, UBOUND, SATURATE, ENX, and PERSIST options for INCREX (#4679) 
Extend INCREX command with:
- LBOUND <lowerbound>: lower bound constraint. Out-of-bounds results are
rejected and reply [current_value, 0] without modifying the key.
- UBOUND <upperbound>: upper bound constraint. Out-of-bounds results are
rejected and reply [current_value, 0] without modifying the key.
- SATURATE: clamp results to LBOUND/UBOUND when out-of-bounds instead of
rejecting.
- ENX: set the expiration only if the key currently has no TTL.
- PERSIST: clear the TTL of the key when applying the increment.

### Replication

Make `INCREX` always replicate as `SET`. This converges double and
integer `INCREX` paths and keeps the replication stream simple.

### Error Handling & Replies
#### 1. Option Syntax & Exclusivity Errors
| Scenario | Return Value / Error Reply | Notes |
|---|---|---|
| `ENX` without an expiration option | `-ERR ENX flag requires an
expiration` | `ENX` must be paired with `EX`, `PX`, `EXAT`, or `PXAT`. |
| `PERSIST` combined with `EX`, `PX`, `EXAT`, or `PXAT` | `-ERR syntax
error` | Mutually exclusive TTL operations. |
| `PERSIST` combined with `ENX` | `-ERR syntax error` | Mutually
exclusive. |
| Missing argument value (`LBOUND`, `UBOUND`, `BYINT`, etc.) | `-ERR
syntax error` | E.g. `INCREX k LBOUND`. |
| Unknown option token | `-ERR syntax error` | |
#### 2. Bound Argument Validation Errors
| Scenario | Return Value / Error Reply | Notes |
|---|---|---|
| `LBOUND > UBOUND` | `-ERR LBOUND can't be greater than UBOUND` |
Checked in both integer and float modes before execution. |
| `UBOUND -inf` without `LBOUND` | `-ERR LBOUND can't be greater than
UBOUND` | Default `LBOUND` is `-LDBL_MAX`, which is greater than `-inf`.
|
| `LBOUND inf` without `UBOUND` | `-ERR LBOUND can't be greater than
UBOUND` | Default `UBOUND` is `LDBL_MAX`, which is less than `+inf`. |
| `LBOUND inf UBOUND -inf` | `-ERR LBOUND can't be greater than UBOUND`
| `+inf > -inf`. |
| `LBOUND` not an integer (integer mode) | `-ERR LBOUND is not an
integer or out of range` | E.g. `LBOUND abc`, `LBOUND 1.5`, `LBOUND
inf`. |
| `UBOUND` not an integer (integer mode) | `-ERR UBOUND is not an
integer or out of range` | E.g. `UBOUND abc`, `UBOUND 1.5`, `UBOUND
inf`. |
| `LBOUND` not a valid float (float mode) | `-ERR LBOUND is not a valid
float` | E.g. `LBOUND abc`, `LBOUND nan`, float overflow (`2e4932`). |
| `UBOUND` not a valid float (float mode) | `-ERR UBOUND is not a valid
float` | E.g. `UBOUND abc`, `UBOUND nan`, float overflow (`2e4932`). |
*Note: In `BYFLOAT` mode, `"inf"`, `"+inf"`, and `"-inf"` are valid IEEE
754 floats and are accepted by the parser (e.g. `UBOUND inf` or `LBOUND
-inf`).*
#### 3. Runtime Overflow Errors (under `SATURATE`)
When `SATURATE` clamps a value to a bound, the applied delta (`value -
oldvalue`) must be representable. If computing the applied increment
would overflow the type, an error is returned and the key/TTL are left
untouched:
| Scenario | Return Value / Error Reply | Notes |
|---|---|---|
| Integer applied delta overflow | `-ERR applied increment would
overflow` | Occurs if `value - oldvalue` overflows signed 64-bit int
(e.g. `k = LLONG_MAX`, `INCREX k BYINT -5 UBOUND LLONG_MIN SATURATE`). |
| Float applied delta or saturated value is Infinity | `-ERR applied
increment would be Infinity` | Occurs if `value - oldvalue` is
`Infinity` or clamping reaches `Infinity` (e.g. `k = -1e4932`, `INCREX k
BYFLOAT 1e4932 LBOUND 1e4932 SATURATE`, or `UBOUND inf SATURATE`). |
#### 4. Non-Error Responses for Declined Operations (Semantic Replies)
Unlike the error cases above, out-of-bounds or unsatisfied conditions do
not return a RESP error; they return a two-element array indicating the
operation was skipped:
| Scenario | Reply | Key & TTL Side Effects |
|---|---|---|
| Exceeds `UBOUND` (without `SATURATE`) | `[current_value, 0]` | Key
untouched, existing TTL preserved, no keyspace events. |
| Falls below `LBOUND` (without `SATURATE`) | `[current_value, 0]` | Key
untouched, existing TTL preserved, no keyspace events. |
| Integer/float overflow (without `SATURATE`) | `[current_value, 0]` |
Key untouched, existing TTL preserved, no keyspace events. |
| Out-of-bounds on missing key (without `SATURATE`) | `[0, 0]` | Key is
**not** created, TTL not set. |
| Out-of-bounds on missing key (with `SATURATE`) | `[bound, bound]` |
Key **is** created with the clamped bound, expiration applied. |
| `NX` failed (key exists) | `[current_value, 0]` | Key untouched, TTL
untouched. |
| `XX` failed (key absent) | `[0, 0]` | Key is not created. |

#### 5. Infinite Bounds & `SATURATE` Interaction
In `BYFLOAT` mode, `"inf"`, `"+inf"`, and `"-inf"` are valid IEEE 754
floats and are accepted as bounds by the parser (`UBOUND inf`, `UBOUND
+inf`, `LBOUND -inf`). However, Valkey string objects cannot store
`Infinity`:
- **Without `SATURATE`**:
- Increments that remain finite succeed normally within the infinite
bound (e.g., `SET k 10; INCREX k BYFLOAT 1.0 UBOUND +inf` -> `[11, 1]`).
- Increments that would overflow to `Infinity` without `SATURATE` are
safely declined: `[current_value, 0]` (leaving key and TTL untouched).
- **With `SATURATE`**:
- If clamping would reach an infinite bound (e.g., `INCREX k BYFLOAT
1e4932 UBOUND +inf SATURATE` or `INCREX k BYFLOAT -1e4932 LBOUND -inf
SATURATE`), the resulting target value and applied delta would be
`Infinity`.
- In this case, the command safely returns an error and does not mutate
the key or TTL:
    ```text
    -ERR applied increment would be Infinity
    ```

Note: Some error messages may change when compared to what was returned
in `INCREX` for RC1. Since it is unreleased, aligning the error messages
now is safe and not a breaking change.

---------

Signed-off-by: Jacob Murphy <jkmurphy@google.com>
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
 2026-10-18 08:49:18 +03:00
Madelyn OlsonandSarthak Aggarwal e1b4ec8455 Fail the RDB transcode on bytes after the source frame (#4890) 
During a disk-based full sync that transcodes the RDB (wire codec
differs from the replica's `rdbcompression`), any bytes after the end of
the primary's VCS frame are left pending in the push reader, and the
next transcoder write trips `serverAssert(len == 0 || !resuming)` and
crashes the replica. A well-formed payload never has such bytes, so this
is reachable only with a malformed or corrupted payload, but it should
fail the sync, not the process. This fails the transcode when the frame
ends with input left over, so the replica aborts the sync and retries.

<details>
<summary>AI generated details</summary>

## Problem

`rdbTranscoderRun()` breaks out of its feed loop on
`STREAM_PUSH_READER_FRAME_DONE` (`src/rdb_transcoder.c:141`), on the
assumption stated in its comment that the caller already excluded
trailing bytes. `streamPushReaderFeed()` stashes whatever follows the
frame end in `pending_input` (`src/compression_stream.c:616-618`). After
the frame ends the decompressor consumes nothing
(`src/compression.c:161`), so later input is stashed too. The next feed
with input then asserts at `src/compression_stream.c:593`.

LZ4 source, plaintext target:

| Bytes after the frame | Before | After |
|---|---|---|
| none | OK | OK |
| in the same write as the frame end | silently dropped, transcode
succeeds | transcode fails |
| in one later write | stashed, transcode succeeds | transcode fails |
| in two or more later writes | `serverAssert` crash | transcode fails |

`replicaReceiveRDBFromPrimaryToDisk()` calls `rdbTranscoderWrite()` once
per read for size-framed transfers and up to twice per read for
`$EOF`-marked ones (`src/replication.c:3199-3210`), so trailing bytes
that span a read boundary reach the crash. The verbatim route (source
codec equals target) never decodes; it still writes trailing bytes to
disk, where the loader stops at the frame end and ignores them.

Introduced in #4749 (`7fe7ca45f` on unstable, `4216cddd7` on 9.2). Not
in any release tag.

## Can a well-behaved primary send trailing bytes?

No. The receiver holds back the trailing `RDB_EOF_MARK_SIZE` bytes, so
the `$EOF` mark is never fed (`src/replication.c:3193-3214`). The
primary writes exactly one codec frame per RDB (`rdbSaveRio()`,
`src/rdb.c:1675`), and the diskless `$EOF` trailer is written after it
returns and the writer is detached (`src/rdb.c:1631`).

## Alternative considered

Ignore the trailing bytes instead of failing. The on-disk loader stops
at the frame end
(`CompressionTest.streamReaderStopsBeforeTrailingBytes`), so the
verbatim route already tolerates them. That loses: nothing legitimate
produces them, the transcoder's own contract says there are none, and it
needs a frame-done flag plus discarding pending input on every later
write rather than one check. Failing the sync costs a retry on a payload
that is already wrong.

## Behavior change

Trailing bytes that arrive with the frame end used to be dropped and the
sync succeeded. They now fail the sync. Only malformed payloads are
affected.

## Testing

`CompressionTest.rdbTranscoderRejectsBytesAfterSourceFrame` in
`src/unit/test_compression.cpp` drives the transcoder directly for every
source/target pair that converts, with the trailer in the frame's write
and split across two later writes. There were no unit tests for the
transcoder before. With the `src/rdb_transcoder.c` hunk reverted:

```
[ RUN      ] CompressionTest.rdbTranscoderRejectsBytesAfterSourceFrame
test_compression.cpp:1830: Failure
Expected equality of these values:
  rdbTranscoderWrite(&t, with_trailer, sdslen(with_trailer))
    Which is: 0
  -1
Google Test trace:
test_compression.cpp:1824: src=2 target=0 same_write=true
...
test_compression.cpp:1835: Failure
Expected equality of these values:
  rdbTranscoderWrite(&t, trailer, 3)
    Which is: 0
  -1
Google Test trace:
test_compression.cpp:1824: src=2 target=0 same_write=false
```

The second trailer write then dies in the assert:

```
frame #3: _serverAssert(estr="len == 0 || !resuming", file="compression_stream.c", line=593) at debug.c:1180:22
frame #4: streamPushReaderFeed(..., len=4, ...) at compression_stream.c:593:5
frame #5: rdbTranscoderRun + 180
frame #6: rdbTranscoderWrite + 392
frame #7: CompressionTest_rdbTranscoderRejectsBytesAfterSourceFrame_Test::TestBody() at test_compression.cpp:1836:21
```

</details>

This was generated by AI but verified, with love, by a human.

---------

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
 2026-10-16 19:03:51 -07:00