Skip to content

Deflake "Throttling protects a replica above the soft COB limit" - #61

Open
madolson wants to merge 67 commits into
unstablefrom
ai/issue-30
Open

madolson wants to merge 67 commits into
unstablefrom
ai/issue-30

Conversation

@madolson

Copy link
Copy Markdown
Owner

The test freezes the replica, pipelines 30 x 1MB SETs, then asserts the replica is still connected. That assertion only holds while the throttler's soft-limit exemption is in force, and the exemption expires 120 seconds after the COB crosses the soft limit (src/throttle_repl.c:136-141). The throttler rate-limits ops per second and, with the replica frozen, drives that rate toward zero with no floor, so a write phase sized by a fixed op count has no time bound and outlasts the exemption on valgrind and Fedora TLS. This bounds the write phase by the COB reaching the state under test instead, then holds there for three seconds so the soft-limit second boundary is still crossed.

Fixes #30

Details

Problem

tests/integration/throttle-repl.tcl:143 sets client-output-buffer-limit "replica 1073741824 1048576 0". With soft_limit_seconds == 0, checkClientOutputBufferLimits() kills the replica as soon as one server.unixtime second ticks past the first over-limit observation (src/networking.c:6719-6729). The only thing that keeps it alive is src/networking.c:6738:

    if (soft && !hard && throttleRepl_isClientExemptFromCobLimits(c)) return 0;

and that exemption is bounded at src/throttle_repl.c:138:

    time_t elapsed = server.unixtime - c->obuf_soft_limit_reached_time;
    if (elapsed > 4 * STEADY_STATE_CONVERGENCE_SECS) return false;

STEADY_STATE_CONVERGENCE_SECS = 30 (src/throttle_repl.c:20), so 120 seconds. The replica is SIGSTOPped for the whole test, so the throttler can never converge and the exemption always expires eventually. The old write phase at tests/integration/throttle-repl.tcl:158-162 had no time bound, so whether the assertion held depended on machine speed.

Reproduction

The forcing knob is the op count of the write phase, not its byte count. throttle_adjustRate() limits ops per second (src/throttle.c:266-274), so raising the byte count changes nothing: 300 x 1MB completes in 3 seconds locally. Raising the op count for the same 30MB starves the phase.

Applied to the unmodified test at d6415e766:

-            set value_size [expr {1 * 1024 * 1024}]
-            set num_writes 30
+            set num_writes $::repro_ops
+            set value_size [expr {30 * 1024 * 1024 / $num_writes}]
$ REPRO_OPS=100000 ./runtest --single integration/throttle-repl
[ok]: Steady-state throttle happy case (1225 ms)
REPRO: write phase start 1789533866 100000 x 314B
REPRO: write phase end   1789533987
[err]: Throttling protects a replica above the soft COB limit in tests/integration/throttle-repl.tcl
replica was disconnected while above soft but below hard COB limit

The write phase took 121 seconds, one second past the cap, and the replica was dropped by design. A 125 second pause_process of the primary inserted at the same point reproduces the identical message, confirming wall-clock time in the window is the whole mechanism.

Testing

Before, 5 loops, unmodified test plus the REPRO_OPS knob:

$ REPRO_OPS=100000 ./runtest --single integration/throttle-repl --loops 5
[err]: Throttling protects a replica above the soft COB limit in tests/integration/throttle-repl.tcl
replica was disconnected while above soft but below hard COB limit
... x5 ...
Test Summary: 30 passed, 5 failed

After, 5 loops, same knob:

$ REPRO_OPS=100000 ./runtest --single integration/throttle-repl --loops 5
Test Summary: 35 passed, 0 failed
\o/ All tests passed without errors!

The test is still not vacuous. Adding $primary config set repl-throttling-enabled no immediately before the new dwell loop makes it fail, so the dwell really does depend on the exemption:

REPRO: write phase end   1789535034 above_soft 1
[err]: Throttling protects a replica above the soft COB limit in tests/integration/throttle-repl.tcl
replica was disconnected while above soft but below hard COB limit

clientsCron re-runs the limit check every cycle (src/server.c:1315), which is why a passive dwell is enough to catch the disconnect.

Rejected alternative

Restoring a large soft_limit_seconds (the pre-#4612 value was 10000) also makes the test pass, but it makes it vacuous: the base soft-limit kill can then never fire inside a test, so the test would pass whether or not the exemption exists.

Out of scope

While one replica is frozen the throttler drives the primary's write rate to ~0 for up to 120 seconds and then drops the replica anyway, because throttle_adjustRate() applies no floor on decrease (src/throttle.c:266-274). That is a design question for the product, not a test issue.

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

ranshid and others added 30 commits August 31, 2026 14:55
HGETEX mutates the key: `EX`/`PX`/`EXAT`/`PXAT` change a field's TTL,
and an `EXAT`/`PXAT` in the past deletes the field outright. It is a
write command (`@write` category, `CMD_WRITE` flag), but its key spec
only declared `["RW","ACCESS"]`. `ACCESS` maps to read permission alone,
so the key spec required only read access on the key.

As a result, a user granted read-only access to a key pattern (`%R~`)
was allowed to run HGETEX and change field TTLs or delete fields it was
only permitted to read.

Add `"UPDATE"` to the key spec so HGETEX requires both read and write
permission, matching the command's write semantics and the key specs of
the other hash-mutating commands (`HEXPIRE`/`HPEXPIRE`/`HGETDEL`).
Regenerate `commands.def` and add ACL regression tests covering
read-only, write-only, and read+write grants.

## Testing
- Regenerated `commands.def` from `hgetex.json`; the only change is the
HGETEX keyspec line.
- New ACL regression tests in `tests/unit/hashexpire.tcl` pass
(read-only denied, write-only denied, read+write permitted).
- Verified live via `ACL DRYRUN`: `%R~` and `%W~` grants are denied,
`%RW~` is permitted.

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Fix reentrancy problem with rdbSaveLzfStringObject

Signed-off-by: Abhishek Kumar <abhishranchi@gmail.com>
`tlsGetPeerUser()` derived the ACL username from the certificate CN with
`strlen()`, so an embedded NUL truncated the identity before the lookup.
A certificate whose CA vouched subject is `CN=admin\0attacker`
authenticated as the ACL user `admin` and got its privileges. The
sibling SAN URI path already passed an explicit length to
`ACLGetUserByName()`. The CN path did not.

No explicit NUL check is needed. `ACL SETUSER` refuses a username
containing one, so comparing the full length is sufficient. Dropping the
buffer also removes the silent truncation of a CN longer than 255 bytes.

The CN is handed back whether or not it matches, so a failed
authentication now produces an `ACL LOG` entry and bumps
`acl_access_denied_tls_cert` instead of leaving no trace at the default
loglevel.

Affects 9.0 and 9.1, CN path only. The feature arrived in #1920.

Not fixed here: `X509_NAME_get_index_by_NID()` takes the first CN and
ignores the rest, so `CN=admin, CN=attacker` authenticates as `admin`.
Refusing that would break certificates that authenticate today and
contradicts `valkey.conf:275-277`, which documents first match wins for
URI mode. Its own PR.

The openssl CLI strips a NUL from a name, which is why #3078 could not
cover this when it added the URI path, so the fixture is issued with a
placeholder byte and re-signed. The new test fails on unstable and
passes with the fix. `unit/tls` and `unit/acl` pass under `--tls` and
`--tls-module`, and `unit/tls` is clean under `SANITIZER=address`.

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Expose two new CLUSTER INFO counters to track cluster link:
- total_cluster_links_established_inbound: inbound links
  successfully established with peers via accept
- total_cluster_links_established_outbound: outbound links
  successfully established to peers via connect

Unlike the existing topology-derived cluster_connections field,
these are precise cumulative counters incremented when a link
is actually established (on accept, and on successful connect).
They help surface network flapping / frequent re-connections
between nodes.

Signed-off-by: Binbin <binloveplay1314@qq.com>
…ior best-effort mode (#2555)

When the cluster changes, we need to persist the cluster configuration.
Currently, nodes.conf is saved synchronously in the main thread during
clusterBeforeSleep, if I/O is delayed or blocked, possibly by disk
contention, this may result in large latencies on the main thread that
affect client requests.

We should avoid synchronous I/O from the main thread.

In this commit, we will try to use bio to save the config file when
cluster-config-save-behavior is set to best-effort. We add a new bio job
and send a sds version of the config file, which does the synchronous save,
so there is some eventually consistent version consistently stored on disk.

For shutdown and cluster saveconfig, we will wait for the bio job to get
drained and trigger a new save in a sync way.

New cluster info fields:
- cluster_config_save_status: ok or err.
- cluster_config_last_save_time: the unix time of the last successful save.

New DEBUG BIO-DRAIN subcommand to drain the bio jobs.

Closes #2424.

Signed-off-by: Binbin <binloveplay1314@qq.com>
… (#3833)

In the current failover protocol, a replica sends one AUTH_REQUEST per epoch and
each voter casts at most one vote per epoch. Despite the various delay heuristics
in `clusterHandleReplicaFailover` that try to stagger the replicas, concurrent
elections can still collide on the same epoch. When the vote is split and nobody
reaches the quorum, the losing replica cannot learn this in time: it must first
wait for the election to be declared expired after auth_timeout (2 ×
cluster-node-timeout), and then wait another auth_retry_time (2 × auth_timeout)
before it is allowed to start the next election with a higher epoch.

This PR introduces a new message type, FAILOVER_AUTH_NACK, that a voter replies
with in every rejection branch (NOT_SAFE, REQ_EPOCH_OLD, ALREADY_VOTED,
REQ_IS_PRIMARY, NO_PRIMARY, PRIMARY_UP, STALE_CONFIG). The replica counts the
NACKs it receives; since a voter never answers twice in the same epoch, the most
votes it could still gather is bounded by the voters that have not NACKed. Once
that bound drops below the quorum, the election is declared unwinnable and a new
one is started immediately with a higher epoch, shrinking recovery from the
auth_timeout + auth_retry_time window down to a few cron ticks.

A voting primary that is itself FAIL can never reply (it neither ACKs nor NACKs)
yet is still counted in cluster->size, so it is excluded from this bound: the
achievable votes are `size - size_fail - nack_count`, where size_fail is the
number of FAIL voting primaries maintained in clusterUpdateState(). Without this,
a split vote among the replicas of several primaries that went down together
could never push nack_count high enough, and the replica would fall back to the
slow auth_timeout path. The quorum itself is still computed from the full size,
since a failover must be authorized by a majority of the whole configuration.

Wire compatibility is preserved by gating NACK sending behind a new capability
flag, CLUSTER_NODE_FAILOVER_AUTH_NACK_SUPPORTED, advertised in the PING/PONG
flags via `clusterUpdateMyselfFlags`. Nodes that do not advertise this capability
will never see the new message type and fall back to the legacy auth_timeout path.

Granted (ACK) and rejected (NACK) votes are logged symmetrically per epoch, so
the vote tally of an election can be reconstructed from the logs.

Two DEBUG hooks are added for testing:
- DEBUG CLUSTER-FAILOVER-DELAY <ms>: override the failover delay computed in
  clusterHandleReplicaFailover.
- DEBUG CLUSTER-FAILOVER-EPOCH <epoch>: force the next election started by this
  replica to run in the given epoch (consumed once, then back to currentEpoch+1),
  so several replicas can be made to contend in the same epoch and deterministically
  reproduce a split vote.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
`utils/reply-schema-linter` pins fast-uri 3.1.0 transitively via ajv,
flagged by
[CVE-2026-6321](https://nvd.nist.gov/vuln/detail/CVE-2026-6321),
[CVE-2026-6322](https://nvd.nist.gov/vuln/detail/CVE-2026-6322),
[CVE-2026-13676](https://nvd.nist.gov/vuln/detail/CVE-2026-13676),
[CVE-2026-16221](https://nvd.nist.gov/vuln/detail/CVE-2026-16221) and
[CVE-2026-18446](https://nvd.nist.gov/vuln/detail/CVE-2026-18446).

Bumped to 3.1.6, the newest release still inside ajv's `^3.0.1` range,
so `package.json` is unchanged.

The lockfile was regenerated with `npm update fast-uri
--package-lock-only` rather than hand-edited.

Verified locally: `npm ci --ignore-scripts` on an empty cache installs
with no `EINTEGRITY`, `npm audit` reports 0 vulnerabilities, the linter
exits 0 over `src/commands` (398 of 427 command entries carry a
`reply_schema` and are compiled), and a deliberately corrupted schema
still exits 1.

Signed-off-by: Stav Ben Shahar <stavbs@amazon.com>
A replica could keep the `CLUSTER_NODE_MY_PRIMARY_FAIL` flag after it was
moved from a failed primary to a healthy new primary. Because this flag is
gossiped to other replicas, a later unrelated failover could incorrectly look
like every replica had already observed the primary failure, allowing the
best-ranked replica to skip the normal election delay even though replication
offsets had not been re-exchanged.

This change clears the local `CLUSTER_NODE_MY_PRIMARY_FAIL` flag when the old
primary recovers, when the local node is promoted, and when a replica is
reconfigured under a new primary.

Introduced in #2227.

Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
Adds `IFNE` option to `SET`, allowing a conditional set when the value
differs. Includes tests.

---------

Signed-off-by: arshidkv12 <arshidkv12@gmail.com>
Signed-off-by: Arshid <arshidkv12@gmail.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
> **Builds on prior work:** This PR is a continuation of
[#2965](valkey-io/valkey#2965) by @li-benson,
which introduced the original hot key detection feature based on
Count-Min Sketch with operator-tuned QPS thresholds. That PR established
the command surface (`HOTKEYS GET` / `RESET`), the per-access sampling
hook in `lookupKey`, the cluster integration points, and most of the
operator-facing scaffolding. Many thanks to @li-benson for the
foundation.
>
> **What changed vs #2965:**
> * **Algorithm**: Count-Min Sketch + min-heap → **Space-Saving**. A
bounded set of K `(key, count, error)` counters whose natural
replace-min eviction *is* the top-K selector. Each entry carries an
error bound, so the true count lies in `[count - error, count]` — no
sketch-wide multi-hashing, no width/depth tuning, small fixed footprint.
The algorithm and its frozen-window manager live in their own
`src/space_saving.{c,h}` module, unit-tested independently of the
server.
> * **No thresholds**: Operators no longer configure
`hotkey-read-threshold` / `hotkey-write-threshold`. The algorithm's
natural eviction is the filter — only the K hottest keys ever appear.
> * **Single combined summary**: One global read+write summary (no
per-slot summaries, and no read/write split in v1); `db` is per-entry
metadata.
> * **QPS**: A **frozen-window** model. Counts accumulate during a fixed
window; `HOTKEYS GET` reports the last *completed* window, never a
partial one.
> * **Polling-only API**: Removed the pub/sub `__hotkey_notify__`
channel in favor of stateless `HOTKEYS GET`.
> * **Enable via top-K**: No separate on/off switch — `hotkeys-top-k`
doubles as one, since tracking zero keys is the same as not tracking.
`0` (the default) disables detection; any positive value enables it and
sets the Space-Saving capacity.

## Summary

This PR adds built-in server-side hot key detection to Valkey using the
**Space-Saving** algorithm, a lightweight, memory-efficient heavy-hitter
detector that identifies the top-K hottest keys without requiring QPS
thresholds.

The detector maintains a fixed-size summary of K counters and guarantees
that any key whose frequency exceeds **N/K** within a detection window
is tracked. Each counter additionally carries an `error` bound, so the
true count is always within `[count - error, count]`. The implementation
uses a single global summary (a few KB), supports configurable sampling,
and exposes results through a simple polling API. It is **disabled by
default**.

## Design highlights

### Space-Saving module

The algorithm and its frozen-window manager live in a standalone
`src/space_saving.{c,h}` module, which depends only on the allocator and
`sds` — no server internals, no global clock (the caller passes a
monotonic timestamp in). The tracked item is concretely a `(key name, db
id)` pair stored directly in the slot, so comparison, hashing and
copying are inlined rather than dispatched through callbacks, and there
is no per-entry item allocation. Anything derivable from the key —
notably the cluster hash slot — is computed on demand instead of stored.

`src/hotkeys.{c,h}` holds the policy around it: what counts as a
recordable access, the sampling and enable configuration, the
invalidation predicates, and the `HOTKEYS` commands. The data path in
`db.c` makes a single call — `hotkeysRecordLookup()` or
`hotkeysRecordDelete()` — and needs no hot-key knowledge of its own.

An earlier revision made this layer fully generic (a caller-supplied
`cmp`/`dup`/`free`/`hash` vtable and an opaque per-window context
pointer). With exactly one consumer that abstraction only bought
indirection, so it was collapsed into the concrete form above; the two
genuinely useful pieces of that shape — predicate-based invalidation and
on-demand slot derivation — were kept.

### API

Three subcommands are introduced:

* `HOTKEYS GET` – returns the current hottest keys sorted by estimated
QPS (descending). No arguments.
* `HOTKEYS RESET` – clears all collected state.
* `HOTKEYS HELP` – the usual container help text.

While detection is disabled, `HOTKEYS GET` returns an empty array and
`HOTKEYS RESET` succeeds as a no-op rather than erroring, matching
`SLOWLOG GET` and `LATENCY HISTORY`: a polling client then has a single
reply shape and never has to match on an error string to tell "disabled"
from "nothing is hot".

A polling model was chosen over pub/sub because it is simpler,
stateless, resilient to client disconnects, and integrates naturally
with monitoring systems.

### Global summary

The implementation uses a single global summary instead of per-slot ×
per-db summaries that explode combinatorially. This keeps memory usage
fixed regardless of cluster size while still reporting the database for
each detected key. Reads and writes share one summary in v1; a
read/write split can be added later without changing the wire format.

### Frozen-window QPS

Counts are accumulated in **exact integer counters** over a fixed window
of `hotkeys-window-seconds` (default: 1 second). At each window boundary
the live summary is *frozen* into an immutable snapshot and a fresh
window starts empty. `HOTKEYS GET` reports **only the last completed
window**, and estimates QPS from the midpoint of the Space-Saving error
band:

```
qps = (count - error/2) * (100 / sampling_percentage) / measured_window_seconds
```

computed entirely in integer arithmetic. Reporting the last *completed*
window (rather than the partial, in-progress one) means a query never
divides a half-filled window by a full window length, which would
understate the rate. Window rotation is driven from `serverCron` (and
re-checked on `HOTKEYS GET`), anchored to the monotonic clock, so a
completed window is frozen on schedule even with no traffic. The sampled
hot path performs no clock read at all — just a single integer
increment.

Because rotation runs on a cron tick, a window is closed at or *after*
its nominal boundary and so holds slightly more than
`hotkeys-window-seconds` of traffic. Each window records the interval it
really accumulated over, and that measured duration is the rate
denominator; dividing by the configured length would over-report by the
rotation lag (~+10% at the default `hz` with a 1s window) and always in
the same direction. Boundaries are measured from when a window actually
started, so a late rotation cannot shorten the following one.

If the server stalls long enough that the open window covers more than
**twice** the configured length, its counts describe too coarse an
interval to publish as "the last window" and are dropped instead —
`HOTKEYS GET` returns empty until the next window completes. That bounds
how stale a report can be (a frozen window always spans `[length, 2 ×
length)`), at the cost of discarding the accesses seen during the stall.

We considered two alternatives:

* **Cumulative counters (no reset).** Simple, but stale keys accumulate
forever and dominate the top-K, so the "what's hot right now" semantic
breaks down.
* **Exponential decay.** Smoothly ages out old observations, but
requires floating-point math and a clock read on every sampled access,
and forces operators to reason about a half-life parameter; on a partial
window it over-extrapolates.

The frozen window keeps the hot path to a single integer increment,
gives an exact count over a known interval, and the "hottest keys in the
last N seconds" semantic is immediately intuitive.

### Sampling

Detection is turned on by setting `hotkeys-top-k` above 0 (it is 0, i.e.
off, by default). While enabled, `hotkeys-sampling-percentage` (1–100,
default 1) controls what fraction of key accesses are sampled —
primarily a trade-off between overhead and QPS-estimation accuracy;
genuine hot keys remain detectable even at low sampling rates.

### What counts as an access

Only genuine, direct client activity is recorded, gated on a real client
actively executing a command:

* **Excluded**: the replication stream, the AOF client, RDB/AOF loading,
passive **expiry** and **eviction**, bulk slot deletion
(`delKeysInSlot`, i.e. `CLUSTER FLUSHSLOT` / migration away, whose keys
are also purged afterwards), and introspection lookups flagged
`LOOKUP_NOHOTKEYS` (`OBJECT`, `DEBUG`, and the cluster-redirect lookup
in `getNodeByQuery`).
* **Included**: client-issued `DEL`/`UNLINK` (counted as a write — they
signify activity on the key), and **importing (slot-migration)
clients**.
* **Lookups that merely skip the LRU/LFU touch still count.** The
exclusion tests a dedicated `LOOKUP_NOHOTKEYS` flag rather than the
composite `LOOKUP_NOEFFECTS` mask, so `EXISTS`/`TYPE`/`TTL` (which pass
`LOOKUP_NOTOUCH`) are reported, and a `CLIENT NO-TOUCH on` client is
accounted for identically to any other — previously such a client had
its hits dropped while its misses were still counted.
* **`RENAME`/`MOVE`/`SWAPDB`** are not re-attributed: an entry is keyed
by `(key, db)`, so a briefly-stale entry may appear under the old
identity until it ages out within one window (documented in
`valkey.conf`).
* **Misses are counted by design.** The sampling hook in `lookupKey`
runs after the hit/miss branch, so a lookup of a missing key is recorded
too — a repeatedly-probed missing key is genuine load worth surfacing.
`HOTKEYS GET` can therefore list a key that does not currently exist.

## Configuration

| Parameter                     | Default | Range  |
| ----------------------------- | ------- | ------ |
| `hotkeys-top-k`               | 0 (off) | 0–1000 |
| `hotkeys-sampling-percentage` | 1       | 1–100  |
| `hotkeys-window-seconds`      | 1       | 1–300  |

Detection is enabled by setting `hotkeys-top-k` to a positive value. All
parameters are runtime configurable.

## Commands and reply format

| Command | Purpose |
|---|---|
| `HOTKEYS GET` | Top-K hot keys, sorted by QPS descending |
| `HOTKEYS RESET` | Clear all detection state |
| `HOTKEYS HELP` | Subcommand help text |

`HOTKEYS` is a pure container command; `GET` and `RESET` are tagged
`ADMIN | DANGEROUS`. Slot/db invalidation on `clusterDelSlot` and FLUSH
is handled internally — no user-facing purge command.

`HOTKEYS GET` returns an array of entries, each a **map** with the
fields `key`, `db`, and `qps`, ordered by QPS descending and capped at
`hotkeys-top-k`. (In RESP2 the map serializes as the equivalent flat
field/value array.)

Example entry (RESP2):
```
1) "key"  2) "user:session:abc"
3) "db"   4) (integer) 0
5) "qps"  6) (integer) 15200
```

### Observability

`INFO hotkeys` exposes two fields:

```
# Hotkeys
hotkeys_last_window_samples:30124
hotkeys_last_window_duration_ms:1004
```

`hotkeys_last_window_samples` is **N** for the last completed window —
the number of sampled observations it was built from. The Space-Saving
guarantee is stated relative to N (only keys above `N/K` are guaranteed
tracked), so it tells an operator the detection floor of the report they
are looking at, and how much to trust a low-ranked entry.

`hotkeys_last_window_duration_ms` is the interval that window really
spanned, which is also the QPS denominator. It lets an operator tell a
report measured over an unusually short or long window from a normal
one. It is `0` when there is no completed window — detection just
enabled or reset, or the last window dropped for being too coarse.

## Testing

* **Unit tests** (`src/unit/test_space_saving.cpp`) cover the
algorithmic properties that integration tests cannot pin down
deterministically, since the clock is injected: the `[count - error,
count]` band across repeated evictions, the "frequency above N/K is
tracked" guarantee under eviction pressure, the rotation policy (a late
rotation keeps its counts with a lag-inclusive duration, a window past
twice the configured length is dropped even when it saw traffic, and a
partial window is never reported), that the lag does not shorten the
following window, top-K selection when the capacity shrinks, `RemoveIf`
across both windows, and that the per-window sampling percentage
survives both a config change and a dropped window.
* **Integration tests** (`tests/unit/hotkeys.tcl`) cover the command
surface and server-side behavior: detection through repeated access,
ordering by QPS, the top-K cap, the reported `db`, `HOTKEYS RESET`,
`HOTKEYS HELP`, enable/disable via `hotkeys-top-k` (including that the
disabled state returns an empty array and an `OK`), `FLUSHALL`
invalidation, cold keys aging out after an idle window, and the
accounting rules above — that `EXISTS`/`TYPE`/`TTL` are counted, that a
`CLIENT NO-TOUCH` client's hits and misses are accounted identically,
and that `OBJECT ENCODING` in a tight loop is not counted while a real
access in the same window is.

## Performance

Throughput impact of running detection at various sampling percentages
(detection is off by default, so baseline overhead is zero):

| Sampling percentage   | Throughput | Degradation (%) |
| --------------------: | ---------: | --------------: |
| Baseline (disabled)   |     210.6K |       **0.00%** |
|                     1 |     209.5K |       **0.52%** |
|                     5 |     209.2K |       **0.66%** |
|                    10 |     209.0K |       **0.76%** |
|                    25 |     208.3K |       **1.09%** |
|                    50 |     207.2K |       **1.61%** |
|                    75 |     206.1K |       **2.14%** |
|                   100 |     205.2K |       **2.56%** |

Even measuring every access (100%) costs under 3%.

**Benchmark setting:**
- Cluster configuration: CME, no replicas
- Client/server instance type: `c7g.16xlarge`
- Connections: 800
- SET/GET ratio: 20/80 (16 `valkey-benchmark` GET processes with 40
clients each, 16 SET processes with 10 clients each)
- TLS: disabled
- Key space: 3M keys; key size 18 bytes; value size 512 bytes
- `io-threads`: 1, `io-threads-do-reads`: no
- `hotkeys-top-k`: 16, `hotkeys-window-seconds`: 1

## Additional implementation details

* Single combined read+write summary. Each summary keeps a **live**
array (the open window) and a **frozen** snapshot (the last completed
window, read by `HOTKEYS GET`); rotation swaps the two in O(K) and
restarts the live window empty, so idle keys age out with no carry-over.
* Counters are exact `uint64_t` integers — no floating point and no
per-access clock-driven decay.
* Membership uses a linear scan over K entries with a cached 32-bit hash
(fast-reject before the full compare); K is small (tens), so the scan is
a handful of cache lines and beats the bookkeeping of a heap at this
size.
* Automatic state invalidation on `FLUSHDB`, `FLUSHALL`, and cluster
slot migration (both live and frozen arrays are purged).
* All `HOTKEYS` commands are restricted to the `ADMIN` and `DANGEROUS`
ACL categories.

## Known limitations (v1)

* **No read/write separation** — a single combined summary; a split is a
straightforward follow-up.
* **Counts real key lookups, not commands** — multi-key commands
contribute one access per key actually touched; a command that errors
mid-execution counts only the keys reached before the error; ACL-denied
commands are rejected before `lookupKey` and count nothing;
`MULTI`/`EXEC` and scripts count each underlying lookup.
* **Possible multi-counting** for commands that both look up and delete
a key (e.g. `GETDEL`, collection-emptying ops) and for blocking commands
re-processed after unblocking. Since detection is sampled/approximate,
this is within the noise.
* **Hot-key state is not reset on replica failover** — detection state
is node-local and is not carried in the replication stream. A replica
accumulates its own counts from reads served locally (the replication
stream from the primary is never counted), and on promotion it keeps
that state rather than starting from a clean window; likewise a demoted
primary retains its counters. Because the reporting window rolls over
continuously, any pre-failover attribution ages out within one
`hotkeys-window-seconds`.

---------

Signed-off-by: Alon Arenberg <alonare@amazon.com>
The module load log lines only reported the module name (and the path
it was loaded from). Also log the module version, so it is easy to tell
which version of a module was loaded.

---------

Signed-off-by: Binbin <binloveplay1314@qq.com>
Correct the condition introduced in #1379

In the original commit 0a987b2d7b5379d2f8e4a98e633614bfef48a05e, the fix
was to avoid using `/tmp` directory when it is detected as tmpfs

However in later refactoring (7892bf8)
the intention of the check was reversed
- If tmpfs test will run & fail
- If not tmpfs test will be skipped 

fixes #897

Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com>
…ucture (#4356)

Token bucket client throttling infrastructure and steady-state replication throttling.

Signed-off-by: harrylin98 <harrylin980107@gmail.com>
Signed-off-by: Harry Lin <49881386+harrylin98@users.noreply.github.com>
Co-authored-by: Jim Brunner <brunnerj@amazon.com>
## Summary

Improve diagnostics for the sporadic RDMA CI failures reported in #4579.

The observed failure only reports:

```text
RDMA: accept failed
RDMA_CM_EVENT_REJECTED
```

This does not expose enough information to distinguish an RXE/kernel
issue from an RDMA CM or Valkey resource setup failure.

---------

Signed-off-by: quanyeyang <quanyemostima@gmail.com>
The broken-link detection in clusterCron only reconnects a link when
no traffic is seen for more than half the node timeout (data_delay >
cluster_node_timeout/2).

This leaves ping_sent unable to advance in this case: while ping_sent
is set we never send another PING (clusterCron skips nodes with an
outstanding ping), and ping_sent is only cleared by a PONG. If the peer
keeps the link alive with its own PINGs (refreshing the data_received)
while our outgoing PING was lost, the link is never reconnected, so
ping_sent stays set forever and we can not send PING.

Force a reconnect when our PING has been outstanding for a full node
timeout without a PONG, independent of data_received. The reconnect
itself
sends a fresh PING, letting us receive a PONG and clear the stale state.

A practical example is a two-node cluster: with no third node to confirm
or correct the failure, a lost PING combined with the peer's own PINGs
leaves the node permanently stuck in PFAIL state.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Throttling test is found to be flaky:
```
52023:M 01 Sep 2026 20:09:16.495 # Disconnecting timedout replica (streaming sync): 127.0.0.1:21192
52023:M 01 Sep 2026 20:09:16.495 * Connection with replica 127.0.0.1:21192 lost.
```

The throttling tests deliberately freeze the replica so that its COB on
the primary grows while the replica stops sending REPLCONF ACK. The
default 60s repl-timeout then disconnects the replica out from under the
throttler before the test can observe throttling. Raising repl-timeout
fixes this flakiness.

Signed-off-by: harrylin98 <harrylin980107@gmail.com>
…(#4017)

In large-dataset random-read workloads, multi-field/member lookup
commands such as `HMGET`, `SMISMEMBER`, and `ZMSCORE` perform repeated
hashtable point lookups. Accessing hashtable buckets and entries in this
workload is often cache-miss-heavy, so command latency can become
dominated by memory access latency.

This PR reuses Valkey's incremental hashtable lookup APIs to add
`hashtableFindBatch`, a simple batched lookup helper for the
single-hashtable case used by `HMGET`, `SMISMEMBER`, and `ZMSCORE`,
improving large-dataset random-read performance through memory-level
parallelism.

The batched lookup path is used only for multi-field/member requests
on hashtable-backed encodings. Other encodings and single-field/member
requests continue to use the existing lookup paths.

---------

Signed-off-by: chzhoo <czawyx@163.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
The LRU list stores a duplicated SDS copy of each EVAL script SHA,
but its allocation was not included in scripts_mem. Account for the
duplicated SHA when adding an LRU node and subtract it when the node
is removed.

Also see #1310 for more details.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Repl throttle disabled with no replicas.  Test fixed.

Signed-off-by: harrylin98 <harrylin980107@gmail.com>
Forkless infrastructure & Forkless implementation of saving RDB snapshots to disk.
Fixes valkey-io/valkey#4219.

---------

Signed-off-by: Jim Brunner <brunnerj@amazon.com>
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
Signed-off-by: Alina Liu <liusalisa6363@gmail.com>
Signed-off-by: Nitai Caro <caronita@amazon.com>
Signed-off-by: nitaicaro <42576749+nitaicaro@users.noreply.github.com>
Co-authored-by: harrylin98 <harrylin980107@gmail.com>
Co-authored-by: Alina Liu <liusalisa6363@gmail.com>
Co-authored-by: Harry Lin <49881386+harrylin98@users.noreply.github.com>
Co-authored-by: nitaicaro <42576749+nitaicaro@users.noreply.github.com>
Co-authored-by: Nitai Caro <caronita@amazon.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: valkey-review-bot[bot] <282604435+valkey-review-bot[bot]@users.noreply.github.com>
Signed-off-by: Jim Brunner <brunnerj@amazon.com>
`tests/unit/type/stream.tcl:947` reads the entire incr AOF and asserts
`LIMIT` appears nowhere. In external server mode the AOF is shared with
every preceding test, so it also matches `LIMIT` tokens propagated by
unrelated commands (`ZRANGESTORE ... BYSCORE LIMIT` is one). Added in
#4063; External Server Tests have been failing on unrelated PRs since:

```
*** [err]: XADD/XTRIM strip redundant LIMIT when rewriting for propagation in tests/unit/type/stream.tcl
Expected '-1' to be equal to '5144546' (context: type eval line 22 cmd {assert_equal -1 [string first "LIMIT" $blob]} proc ::test)
```

Seek past the bytes that were already there, so only this test's
propagation is inspected.

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
CLUSTER SYNCSLOTS FINISH is an internal message sent by a target primary
to its replicas within the slot migration replication stream, but had no guard
rejecting clients that are not part of that stream. A normal client could
therefore abuse it to drive the import state machine to a terminal state.

Reject any client that is not a primary, AOF or slot migration client via
mustObeyClient(). Note that mustObeyClient() is used here rather than the
c->slot_migration_job check used by the other subcommands, since FINISH is
propagated to replicas and the AOF rather than being received on the import
job connection.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Int assigned into bit flag incorrectly (debug code).

Signed-off-by: Jim Brunner <brunnerj@amazon.com>
… (#4610)

Changed flush parsing for more consistent naming.  Avoid double error response with forkless.

Signed-off-by: Jim Brunner <brunnerj@amazon.com>
…XEC test (#4599)

The test was added in #4380 and sometimes it fails:
```
*** [err]: WATCHed key in another slot that expired aborts EXEC in tests/unit/cluster/misc.tcl
Expected '' to be equal to 'OK' (context: type eval line 21 cmd {assert_equal {} $reply} proc ::test)
```

The delay between `SET ... PX` and `WATCH` can be long enough for the
watched key to expire before `WATCH` runs. In that case, WATCH marks the
key as already expired, and `EXEC` ignores it when checking for expired
watched keys, so the transaction commits instead of returning a null reply.

Apparently we need to increase the expiration time so that the key can not
expire logically then the WATCH is called. 100ms should be fine, as we use
it this way elsewhere too. Also added retries to make sure it doesn't fail.

Signed-off-by: Binbin <binloveplay1314@qq.com>
…4332)

- Key generation uses a per-thread splitmix64 stream, seeded per thread
from the (`--seed`-seedable) global generator. Uniformity spot-check:
500K random SETs over a 100K keyspace populated 99.3% of keys (matches
expectation).
- Each benchmark thread records latency into its own histograms. The
final report folds them exactly after `pthread_join`; the
once-per-second live display line is folded by thread 0.
- `requests_finished` accumulates per thread and publishes in batches of
256; the per-command termination check reads a (typically shared-state
cached) global instead of taking exclusive ownership of the line.
Residues are flushed from each thread's timer and at event-loop exit.
`requests_issued` is unchanged (already amortized per pipeline batch,
and it is the gate that keeps `-n` exact).
- Single-threaded mode (`--threads 0`) is unchanged on all three paths.

## Background

valkey-benchmark in multi-threaded mode has a per-process throughput
ceiling that adding `--threads` does not move. Profiling the generator
at saturation (Graviton 4, 16 threads, 1200 connections, pipelined GET)
showed why: ~40% of its CPU is spent in three places that all threads
share, so per-command work serializes process-wide regardless of thread
count:

1. **Key generation takes a process-global lock.** `rand62()` calls
glibc `random()` twice per command, and glibc `random()` serializes
every caller on one internal lock. Measured at ~21% of generator cycles
-- and, worse, it is a lock convoy that all threads pass through once
per command.
2. **Latency recording contends on one shared histogram.** Every
response does `hdr_record_value_atomic` into the same HDR histogram: an
atomic RMW on shared counter cache lines from all threads (~16% of
cycles).
3. **`requests_finished` is a shared atomic incremented per command** --
the same contended-cache-line pattern one layer down, which becomes the
next ceiling once 1 and 2 are fixed.

## Caveats

- With `--seed`, per-thread key streams are reproducible only up to the
thread scheduling order of first use.
- Count-mode termination detection can lag by up to 256 x threads
commands; the issued-request gate still bounds total requests exactly,
and the final "N requests completed" is exact (verified in both
threading modes).
- The per-second live display fold can lose a concurrently recorded
sample; this affects only the transient display line, never the final
report.

---------

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Debian 11 Bullseye has reached end of life, and package rotations are
causing apt installs in the build-debian-old job to fail with 404 errors.

Switch the job to Debian 12 Bookworm so it continues testing against the
oldest supported Debian toolchain without relying on EOL package mirrors.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Extends the memory prefetch batch to the inner hashtable of hash, sorted
set and set values, so member lookups get the same treatment top-level
keys already get.

When io-threads are enabled, commands are collected into a batch before
execution. For each key the prefetcher walks a phased state machine,
yielding to the next key at every step so the cache misses of different
commands overlap:

1. `PREFETCH_VALUE`: (when the key has been prefetched) prefetch
   `objectGetVal(val)`, the data structure header
2. `NESTED_PREFETCH_INIT`: resolve the inner hashtable and start an
   incremental find for the member
3. `NESTED_PREFETCH_STEP`: walk the inner hashtable buckets
4. `NESTED_PREFETCH_VALUE`: prefetch the entry value, only for hash
   entries whose value is a separate allocation. Embedded values and set
   or sorted set entries skip this phase.

Only one member is prefetched per key, so the work stays bounded by
`prefetch-batch-max-size` instead of being multiplied by the field
count. Keys that are overwritten without being read are skipped, since
there is nothing worth warming.

Commands opt in by typing the member argument as `field` or `member` in
the command JSON. The code generator derives the argv position from the
argument layout if the position of the first field/member is fixed, so
commands with optional arguments before their members
cannot be annotated. 30 commands are covered.

Sorted set lookup keys are marked with `zsetMarkLookupKey` around the
hash and compare callbacks, since the sorted set hashtable stores packed
`[score][element]` items and an unmarked key would be read as a packed
item.

Batched lookup within a single multi-field command is covered by #4017,
which is complementary: it does not help single-member commands, and
this does not help later fields.

---------

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
The `Migration not cancelled when snapshot takes more time than
repl-timeout` test intentionally makes the slot snapshot take about five
seconds while setting `repl-timeout` to two seconds.

The test currently uses 1 KiB values. These writes accumulate in rio's
16 KiB file-descriptor buffer before being sent. On slower macOS
runners, the interval between flushes can exceed `repl-timeout`, causing
the target to close the import connection. The later 120-second wait for
the slot to move is only the secondary symptom.

Use 32 KiB values so each value is written directly instead of being
buffered. This keeps data flowing during the snapshot, while the 100 ms
delay across 50 keys still ensures that the overall snapshot takes
longer than `repl-timeout`.

This failure started after #4409 moved the save delay to the actual
source node, making the test exercise the intended slow-snapshot path.

Closes #4552.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
roshkhatri and others added 28 commits September 11, 2026 09:00
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
### Problem

When `tls-ca-cert-dir` is configured to a directory that exists but
contains no certificates, the server starts successfully and logs no
warning. This is because `SSL_CTX_load_verify_locations` with a
directory argument is lazy, OpenSSL registers the path without scanning
it, returning success even for an empty directory.

The failure is only discovered at handshake time, when every connecting
client sees `Server closed the connection` and the server logs `Error
accepting a client connection: error:0A000086:SSL routines::certificate
verify failed`.

This is a silent misconfiguration that gives no actionable signal at
startup or `CONFIG SET` time.

### Fix

`loadCaCertDir` (introduced in #2999 for validity checking) already
eagerly scans the directory to load certs. This PR extends it to count
the certs loaded and fail immediately if zero certs were found.

### Behavior change

Scenario 1: Startup with empty `tls-ca-cert-dir`.
Before: Server starts, clients fail at handshake.
After: Server refuses to start with a clear log message.

Scenario 2: `CONFIG SET tls-ca-cert-dir <empty-dir>`
Before: Returns OK, clients fail at handshake.
After: Returns error, previous config preserved.

Signed-off-by: Yang Zhao <zymy701@gmail.com>
New config options to enable using two certificates with different
algorithm types at once.

    tls-alt-cert-file
    tls-alt-key-file
    tls-alt-key-file-pass

E.g. one can be a PQC mldsa certificate and another can be RSA for
backwards compatibility with older clients that don't support PQC yet.

New INFO fields under the TLS section (matching existing
`tls_server_cert_serial` and `tls_server_cert_expires_in_seconds`):

    tls_server_alt_cert_serial
    tls_server_alt_cert_expires_in_seconds

Resolves #3403

---------

Signed-off-by: Petr Khartskhaev <pkhartsk@redhat.com>
When trying to compile unit tests using clang-23 the following
compilation error was hit

```
test_util.cpp:198:13: error: implicit conversion increases floating-point precision: 'double' to 'long double' [-Werror,-Wdouble-promotion]
  198 |     v = 0.0 / 0.0;
      |       ~ ~~~~^~~~~
1 error generated.  
```

Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com>
In this code path the `!c->flag.blocked` is always true as it's already
checked on the parent `if` statement. This check is unneeded & redundant
(Compiler was already removing it Implicitly)

Signed-off-by: Bara' Hasheesh <bara.hasheesh@gmail.com>
…listpack (#4669)

Loading a `HASH_2` payload can build a hash whose field-expiry metadata
is inconsistent with its volatile set. The loader appends each field's
expiry to the listpack as it goes but installs the aggregate
volatile-count header only after the loop, and
`hashTypeConvertListpack()` decides whether to register the converted
entries in the volatile set by peeking at exactly that header. A payload
whose later field crosses `hash-max-listpack-value` therefore converts a
header-less listpack, and the resulting hashtable entries carry an
expiry while belonging to no volatile set: the fields never expire,
their TTLs are dropped by the next RDB save, and `HDEL` of one of them
dereferences a NULL set. This derives the decision from the expiries of
the entries the conversion actually carried over, so it no longer
depends on the header being installed.

<details>
<summary>Details</summary>

## Problem

`rdbLoadObject()` builds a `RDB_TYPE_HASH_2` hash incrementally. Each
field's expiry goes in as a trailing tagged metadata entry inside the
loop (`src/rdb.c:2428-2436`), but the leading aggregate volatile-count
header is installed once, after the loop (`src/rdb.c:2449-2454`), to
avoid rewriting it per field.

Inside the loop, a field or value over `hash-max-listpack-value`
converts the partially built object (`src/rdb.c:2404-2409`). That call
site assumes conversion carries the already-loaded TTLs across on its
own:

```c
/* hashTypeConvert carries the TTLs of the pairs already in the
 * listpack into the volatile set; no header is needed for that. */
hashTypeConvert(o, OBJ_ENCODING_HASHTABLE);
```

It did not. `hashTypeConvertListpack()` gated its volatile-set
registration loop on `hashTypeHasVolatileFields(o)`
(`src/t_hash.c:998`), which for a listpack is an O(1) peek at the
leading header (`src/t_hash.c:128`). Mid-load, the header does not exist
yet, so the gate was false. The conversion loop still copied each
field's expiry into the new entry (`src/t_hash.c:1010-1011`), so the
hashtable ended up holding entries with an expiry that no `vset` knew
about.

Three consequences, all confirmed on a build of unstable at `da91ccd12`:

`keys_with_volatile_items` stays 0 and the fields are never reaped or
lazily hidden. `dbTrackKeyWithVolatileItems()` asks the same
`hashTypeHasVolatileFields()` (`src/db.c:549`), which for a hashtable is
`set && !vsetIsEmpty(set)`, so the key is not tracked; and because
`hashTypeTrackEntry()` never ran, `hashTypeIgnoreTTL(o, false)` never
swapped in `hashWithVolatileItemsHashtableType`, so reads have no
validate callback either.

The field TTLs are silently dropped by the next RDB save.
`rdbObjectType()` selects `RDB_TYPE_HASH_2` only when
`hashTypeHasVolatileFields(o)` (`src/rdb.c:775-783`), so a desynced hash
is written as plain `RDB_TYPE_HASH`:

```
httl before reload: 1000
OK
httl after reload:  -1
```

`HDEL` of a volatile field crashes. `hashTypeUntrackEntry()` takes
`hashTypeGetVolatileSet()`, which returns NULL for uninitialized
metadata (`src/t_hash.c:67-71`); the `debugServerAssert(set)` at
`src/t_hash.c:201` is compiled out of a release build and
`vsetRemoveEntryWithExpiry()` dereferences it one line before its own
`assert(bucket)` (`src/vset.c:1839`):

```
EIP:
0   valkey-server                       0x0000000104c7f5f4 vsetRemoveEntryWithExpiry + 40

Backtrace:
0   libsystem_platform.dylib            0x0000000181c81744 _sigtramp + 56
1   valkey-server                       0x0000000104c29a1c hashTypeUntrackEntry + 188
2   valkey-server                       0x0000000104c296d8 hashTypeDelete + 168
3   valkey-server                       0x0000000104c2bf28 hdelCommand + 376
4   valkey-server                       0x0000000104c10230 call + 1048
```

Reproduction, with no crafted payload; the source hash is one the server
itself produced, and the threshold change stands in for any load whose
`hash-max-listpack-value` is smaller than the one in effect when the
payload was written:

```
config set hash-max-listpack-value 64
hset myhash a b cc dd
hexpire myhash 1000 FIELDS 1 a          # listpack, 'a' first and volatile
dump myhash
config set hash-max-listpack-value 1    # 'a'/'b' still fit, field 'cc' does not
restore myhash 0 <payload>              # converts after 'a' landed in the listpack
hdel myhash a                           # SIGSEGV
```

Both halves of the mismatch, the deferred header install and the
header-based gate, came from #3212. No tag contains that commit.

## Fix

Set `has_volatile` from the expiry of each entry the conversion carries
over, in the same statement that hands that expiry to `entryCreate()`.
The flag can no longer disagree with what landed in the hashtable, and
it no longer reads the header at all, so the loader's deferred install
stops being load-bearing. `hashTypeCurrentExpiry()` was already called
on that line, so this adds no pass over the entries, and the
registration pass is still skipped entirely when nothing carries an
expiry.

`hashTypeHasVolatileFields()` documented its listpack fast path as "the
aggregate header exists iff at least one field carries an expiry", which
is false for the duration of the loader's loop. That comment now names
the exception, so the next header consumer added on a path reachable
from the loop does not reintroduce this.

Behavior of `RDB_TYPE_HASH_2` loads, per input:

| payload | before | after |
| --- | --- | --- |
| `len > hash-max-listpack-entries` | correct: converts an empty
listpack, every field tracked by the hashtable loop at `rdb.c:2526-2528`
| unchanged |
| fits the listpack, no field over `hash-max-listpack-value` | correct:
header installed after the loop | unchanged |
| first field over the value threshold | correct: converts before any
expiry is appended, nothing to register | unchanged |
| a volatile field appended, then a field over the value threshold |
entry keeps its expiry, `vset` empty | entry registered in the `vset` |

## Alternative considered

Install the aggregate header in `rdb.c` before the mid-load
`hashTypeConvert()` call. It is the same line count and it fixes this
caller, but it keeps `hashTypeConvertListpack()` depending on a header
that every caller has to remember to maintain, which is the thing that
broke. It also spends a `lpInsertMetadata()` realloc writing a header
into a listpack the very next call frees. The conversion is already
reading every field's expiry; asking it to consult a summary of data it
holds in its hand buys nothing.

Making `hashTypeUntrackEntry()` return an error on a NULL set is the
other tempting fix. Not doing that here: with the invariant repaired, a
NULL set at that point means a fresh desync, and converting it into a
returned error would hide it. Promoting `debugServerAssert(set)` to
`serverAssert(set)` so release builds fail the same way the test suite
does is defensible, but it is a separate change and it belongs at
`vset.c:1839`, where the dereference precedes the existing assert and
defeats it.

## Testing

Two tests, because the missing registration has two independent
observable consequences and the second one is not a crash. Both fail
with the `t_hash.c` hunk reverted:

```
[err]: RESTORE tracks field TTLs when the load converts mid-listpack
Expected '1' to be equal to '0' (context: type eval line 6 cmd {assert_equal 1 [get_keys_with_volatile_items r]} proc ::test)
[err]: Field TTLs survive a save after a mid-listpack conversion
Expected '-1' to be between to '1' and '1000' (context: type eval line 7 cmd {assert_range [lindex [r httl myhash FIELDS 1 a] 0] 1 1000} proc ::test)
Test Summary: 449 passed, 2 failed
```

The discriminating assertion in the first test is
`keys_with_volatile_items`, not the `HTTL` line above it: `HTTL` reads
the entry's own expiry and is independent of the vset, so it reports
1000 either way. The second test is tagged `needs:debug` for `debug
reload` and is separate rather than folded into the first so the crash
and vset assertions still run when that tag is denied.

`hashexpire.tcl` already covered DUMP/RESTORE of a listpack hash with
field TTLs (`tests/unit/hashexpire.tcl:5044`) and loads that go straight
to a hashtable on the entry count. Neither reaches a mid-load
conversion: the first keeps the listpack encoding end to end, and the
second converts an empty listpack before reading a single field. Nothing
exercised a load that converts after a volatile field had already landed
in the listpack.

</details>

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

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
…4670)

CLUSTER SYNCSLOTS ESTABLISH had no guard on clients that already own a slot
migration job. Such a client is treated as a replicated client
(mustObeyClient()), so a second ESTABLISH on the same connection skips
validation and creates a second import job that overwrites
c->slot_migration_job, leaving the first job holding a stale job->client
pointer. When the connection then closes, freeClient() only notifies the
current job, and the orphaned job later dereferences the freed client in
clusterSlotMigrationCron(); if it survives until the migration times out,
resetSlotMigrationJob() writes to the freed client and frees it a second
time. Sending two ESTABLISH commands with disjoint slot ranges and then
disconnecting crashes the server (confirmed with an ASAN build:
use-after-free read in clusterSlotMigrationCron).

Every other SYNCSLOTS subcommand already rejects clients it does not belong
to. Reject ESTABLISH on a client that already has a slot migration job, so a
connection can only establish a single import job.

Signed-off-by: sunliqiang <sunliqiang@kylinos.cn>
Add test case for #4610. #4610 replaces this fix, but does not
pick the test case. Adding a test case would be harmless and can
provide extra code coverage.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Improve performance post-command by only calling functions if there's possibly something to do.
---------

Signed-off-by: ahmetalicc <ahmetalicswe@gmail.com>
Co-authored-by: ahmetalicc <ahmetalicswe@gmail.com>
…ion streams, and slot migration) (#4076)

An anti-starvation mechanism for AE.  Prioritizes cluster bus connections.
---------

Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Co-authored-by: Satheesha Gowda <satheeshagowda@google.com>
## Summary

This PR adds module APIs for accounting memory that is not tracked by
zmalloc, and includes that memory in `used_memory` / `maxmemory`
bookkeeping.

  New APIs:

  - `ValkeyModule_IncrExternalMemory(size_t bytes)`
  - `ValkeyModule_DecrExternalMemory(size_t bytes)`

These APIs are intended to be used by modules from a command callback or
from a locked thread-safe context.

  ## Solution

This change introduces an explicit external-memory accounting path for
modules.

  Internally:

  - the core keeps a separate external-memory counter
- `zmalloc_used_memory()` adds that counter to the allocator-tracked
total
- as a result, maxmemory/OOM checks, `used_memory`, and
`used_memory_peak` all include module external memory

`Incr` and `Decr` describe the APIs’ actual behavior: they adjust
accounting only; they do not allocate or free memory themselves.

  Closes: #3339

---------

Signed-off-by: Su Ko <rhtn1128@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
… tcl tests (#4612)

Steady state throttler hardening.

Signed-off-by: harrylin98 <harrylin980107@gmail.com>
Co-authored-by: Jim Brunner <brunnerj@amazon.com>
Follow-on to #4076 which defines priority sources for anti-starvation.
---------

Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Co-authored-by: Jim Brunner <brunnerj@amazon.com>
This provides an optimistic-locking workflow for multi-key updates on a
single shard without relying on `WATCH`, which is awkward for pipelined
clients and clients that reuse a
  single connection across lightweight threads.

  Closes #2713.

  ## Motivation

Today, non-trivial optimistic locking is usually implemented with
either:

  - `WATCH` / `MULTI` / `EXEC`
  - Lua

`WATCH` requires an extra round trip between `WATCH` and `MULTI`, which
does not fit well with pipelined clients. Single-command conditional
writes such as `SET ... IFEQ` help
for simple cases, but they do not cover multi-key transactional updates.

Conditional `EXEC` addresses that gap without introducing a new
transaction-starting command. Clients use ordinary `MULTI`, queue their
commands, and attach the preconditions to
  `EXEC`.

  ## Syntax

  ```text
EXEC [IFEQ <key> <value> | IFNE <key> <value> | NX <key> | XX <key>]...

Multiple conditions are allowed and use implicit AND semantics. The
transaction is executed only when every condition matches at EXEC time.

Condition Matches when
━━━━━━━━━━━━━━━━
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
   IFEQ key value    key exists as a string and equals value
────────────────
──────────────────────────────────────────────────────────────────
IFNE key value key does not exist, or exists as a string and differs
from value
────────────────
──────────────────────────────────────────────────────────────────
   NX key            key does not exist
────────────────
──────────────────────────────────────────────────────────────────
   XX key            key exists
```

  String comparisons do not match non-string keys. All condition keys require read permission and, in cluster mode, must share a hash slot with every key used by the transaction.

  If any condition does not match, EXEC discards the queued transaction and returns a null reply, just like an EXEC aborted by WATCH.


  ## Example

  Instead of:
```
  WATCH ver{foo}
  GET ver{foo}
  MULTI
  SET ver{foo} <new>
  SET mykey{foo}1 111
  SET mykey{foo}2 222
  EXEC
```
  clients can do:
```
  GET ver{foo}
  MULTI
  SET ver{foo} <new>
  SET mykey{foo}1 111
  SET mykey{foo}2 222
  EXEC IFEQ ver{foo} <old>
```

  The transaction is executed only if ver{foo} still equals <old> at EXEC time.

  ## Scope

  This proposal intentionally supports only a flat list of conditions. It does not introduce AND, OR, parentheses, or nested expressions; users needing complex expressions can use
  Lua or another scripting interface.

  ## Supported conditions

  - IFEQ
  - IFNE
  - NX
  - XX

---------

Signed-off-by: Su Ko <rhtn1128@gmail.com>
Signed-off-by: Jacob Murphy <jkmurphy@google.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Jacob Murphy <jkmurphy@google.com>
…4497)

The lex range sentinels are identity values: `zslParseLexRangeItem`
returns the shared `minstring`/`maxstring` objects for the `-` and `+`
bounds, and their exclusivity flag is set arbitrarily (every element
orders strictly between them under the skiplist's pointer-identity
comparison, so inclusive versus exclusive admitted the same set). The
ordered index translates bounds into packed byte keys, where both
properties stop being harmless. Two fixes:

**1. The min sentinel excluded the empty-string member.** The sentinel
packs to the bare score prefix, which is byte-identical to the packed
empty-string element, so the pass-through exclusivity flag wrongly
excluded it in the count and delete-range paths:

```
> ZADD key 0 "" 0 a 0 b
> ZRANGEBYLEX key - [a      → "", "a"     (correct)
> ZLEXCOUNT key - [a        → 1           (should be 2)
> ZREMRANGEBYLEX key - [a   → removes only "a", leaves "" behind
```

This violates the documented behavior: `-` means the negatively infinite
string ([ZRANGEBYLEX](https://valkey.io/commands/zrangebylex/)),
[ZREMRANGEBYLEX](https://valkey.io/commands/zremrangebylex/) removes the
same elements ZRANGEBYLEX would return for the same bounds, and
[ZLEXCOUNT](https://valkey.io/commands/zlexcount/)'s bounds carry the
same meaning as ZRANGEBYLEX's. Fix: the sentinel bound is always
inclusive in the count and delete-range paths, matching the seek path.

**2. Crossed sentinels packed the sentinel's literal bytes as an
element.** The seek path handles the natural sentinel for each direction
but a crossed sentinel (`+` as min, or `-` as max) fell through to
`packLexBound`, which packed the sentinel sds content
("maxstring"/"minstring") as element bytes:

```
> ZADD key 0 min 0 mzz 0 z
> ZRANGEBYLEX key + [zz     → min, mzz, z   (members > the string "maxstring"; should be empty)
> ZREVRANGEBYLEX key - [b   → returns elements (should be empty)
```

Fix: a crossed sentinel bound admits no elements; park the iterator
where the first step in the iteration direction yields nothing, matching
the existing guards in the count and delete-range paths.

**Test coverage:** the lex fuzzy tests generated only `[str`/`(str`
bounds, so sentinels and the empty-string member were structurally
unreachable — which is how both bugs survived the suite. Besides
targeted regression tests for both fixes, the fuzzy tests now include
the empty string in the member pool and sometimes substitute infinite
sentinels for generated bounds (natural, double and crossed positions),
with the ZRANGEBYLEX fuzzy model gaining explicit sentinel tracking.
With the fixes reverted locally, the extended fuzzy tests catch both
bugs.

**Testing:** all regression and fuzzy tests run under both listpack and
btree encodings; full `unit/type/zset` suite passes locally (repeated
runs with the randomized sentinel coverage). Behavior additionally
cross-validated against the listpack implementation over all bound-pair
combinations of `-`, `+`, `[`/`(` with empty and boundary members.

---------

Signed-off-by: Rain Valentine <rsg000@gmail.com>
…#4554)

Two bugs in the fbtree sorted-set backend, both only reachable once a
zset spans multiple btree leaves. There hasn't been a release since the
ZSET work was merged, so neither affects any released version. The first
causes silent data loss.

**1. Wrong results and silent data loss for score ranges**

`resolveStartIdx()`/`resolveEndIdx()` applied the inclusive/exclusive
"advance past equals" adjustment within a single leaf, clamped at
`leaf->num_items`, so they could not express "the boundary lies in a
later leaf". `descendSubPath()` compounded this by clamping a
past-the-end child index to `num_items - 1` instead of advancing to the
next sibling. The max boundary was also located with a lower-bound child
search, which cannot represent a run of equal scores extending into
later children.

Whenever members sharing a boundary score spanned more than one leaf,
ZCOUNT returned wrong counts and ZREMRANGEBYSCORE deleted the wrong set,
both under-deleting and over-deleting. The tree stayed structurally
valid, so `fbtreeDebugValidate()` never flagged it. With 300 members all
at score 5, an exclusive bound on that score should match nothing:

```
ZCOUNT k (5 +inf            -> 239   (correct: 0)
ZREMRANGEBYSCORE k (5 +inf  -> 239   (correct: 0; only 61 of 300 members survive)
```

Score ranges are now reformulated as a half-open rank range whose two
edges are resolved by the same whole-tree lower-bound seek, then handed
to the existing rank-based delete. Both edges therefore stay correct
regardless of how many leaves an equal-score run occupies. An exclusive
bound is rewritten as an inclusive bound on the next 8-byte score
prefix, which is exact byte ordering rather than an assumption about how
scores are normalized. The whole-tree short-circuit is retained so
counting everything still avoids the boundary descents.

**2. Stale prefix on an inner node reduced to a single child**

`updateCommonPrefix()` returned early for `num_items < 2`, so a range
delete that stripped every sibling from an inner node left it holding
the prefix it derived when it still had two or more anchors.
`innerNodeRefreshChildMeta()` had meanwhile rewritten the surviving
child-0 anchor, and child 0's key range extends below its own high key,
so the refreshed anchor could fall below the retained prefix, giving
`inner at depth 1 child 0: anchor missing node prefix`. Such nodes now
drop prefix compression, which is trivially valid. This one is latent:
it produces no wrong answers on its own and the node self-heals on the
next insert.

Bug 2 was reported by Khalilov moe (3ntr0py1337) at leetprotect.com.

**Tests**

The existing range tests are single-leaf with distinct scores, so
neither bug was reachable by them, and there was no ZREMRANGEBYSCORE
fuzzy test at all. Added a gtest that builds a three-level tree and
starves an inner node to a single child, plus TCL cases for equal-score
runs spanning leaves, exclusive bounds landing on a duplicated boundary,
and dense duplicate-score fuzz for ZCOUNT/ZRANGEBYSCORE and
ZREMRANGEBYSCORE. The dense fuzz runs in both encoding arms so listpack
acts as the reference. Six of the seven new tests fail before this
change; the seventh is a deep-tree integrity guard.

---------

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Adding support for INCREX -

```
INCREX key [BYFLOAT increment | BYINT increment] [EX seconds | PX milliseconds | EXAT unix-time-seconds| PXAT unix-time-milliseconds] [NX | XX]
```

---------

Signed-off-by: Gavin D'mello <dmellogavin5000@gmail.com>
Signed-off-by: Gavin D'Mello <dmellogavin5000@gmail.com>
Signed-off-by: Jacob Murphy <jkmurphy@google.com>
Co-authored-by: Jacob Murphy <jkmurphy@google.com>
… replicas (#4425)

In cluster mode, when a primary fails, its replicas compete in an election to
take over the shard. A replica whose replication offset is 0 has never received
any data from its primary, for example when it was just added and has not
finished the initial synchronization yet.

Newly added replicas already get an extra election delay to make them less
likely to win. However, this is only a best-effort mitigation: the replica can
still be elected and get promoted with an empty dataset, and thereby drop all
the data of the shard. This is especially likely when it is the only surviving
replica, where the delay alone cannot prevent it.

What makes this particularly dangerous is that the failure is silent:

* After the failover the shard is back online and fully available.
* The data loss is usually discovered much later, only after it has already
  caused damage and someone starts to investigate.

In other words, an availability problem is easy to notice, while a data loss
problem tends to surface only after the damage is done.

This commit adds a third value to the existing 'cluster-replica-no-failover'
option, turning it from a boolean into an enum with the values 'no', 'yes' and
the new 'if-empty'. With 'if-empty', a replica refuses to start an automatic
failover while it is still empty, i.e. while its replication offset is 0
because it has never completed an initial sync with its primary. Note that
"empty" refers to the data received from the primary, not to the number of
keys: a replica that is fully synced with an empty primary has a non-zero
offset, so it is not considered empty. The default remains 'no', which
preserves the current availability-first behavior, and both 'no' and 'yes'
keep their existing meaning, so existing configurations are unaffected.
Whenever the failover is refused, the replica logs the reason, so the situation
is visible instead of silent.

The goal is to give administrators one more choice in the trade-off between
availability and data safety. In some deployments, users can tolerate losing
part of the data, but cannot accept losing the entire shard. For them it is
often better to sacrifice availability for a while and wait for the original
primary to be brought back up, rather than promote an empty replica and lose
everything. Manual failovers, being an explicit user action, are still allowed.

This issue was previously described in #885.

Signed-off-by: Binbin <binloveplay1314@qq.com>
When the server works out which keys a `SORT` touches, it re-examines
the `STORE` destination as if that key name were an option keyword. A
destination named `by`, `get` or `limit` therefore swallows the
arguments after it and hides a later `STORE` clause, and one named
`store` is read as another `STORE` clause and reports whatever follows
it. Because `SORT` writes to the last `STORE`, the server ends up
checking permissions against one key and writing to a different one, so
a user can write to a key outside their allowed patterns. The fix
advances the scan past the destination so a key name is never re-read as
an option.

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Fixes #4579 

## Summary

The diagnostics added in #4586 showed that sporadic RDMA CI failures
were using the Azure `mana_0` provider instead of the test-created RXE
device.

Both providers shared the host `eth0` address, so selecting an IP from
`rxe_eth0` did not guarantee that RDMA CM would select RXE. This caused
MANA QP transitions to fail with `EPROTO`.

Create a dedicated dummy netdev with a test-only IP and attach RXE to
it. This gives the test an isolated GID/address mapping and prevents
RDMA CM from selecting host RDMA providers.

Cleanup is also limited to test-owned devices.

## Test plan

- Run repeated RXE setup and cleanup cycles.
- Run `sudo ./runtest-rdma --install-rxe` four times.
- Verify both `rdma-test` and `valkey-benchmark --rdma` pass.
- Verify Python syntax and `git diff --check`.

Signed-off-by: quanyeyang <quanyemostima@gmail.com>
…972)

This [issue ](valkey-io/valkey#2751 been
implemented. Please check if it is generic and aligns with Valkey's
design principles.

An optional parameter ``XX`` is added to the ``SISMEMBER`` command to
distinguish between two scenarios: the specified key does not exist, and
the key exists but the target member is not present in the set.
- Return -1 if the key does not exist;
- Return 0 if the key exists but the target member is not present.

---------

Signed-off-by: li-benson <1260437731@qq.com>
Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
Fix missing `signalModifiedKey()` calls for stream commands that mutate
stream metadata rather than stream entries themselves.

Fixes #3429

A few stream commands already mutate consumer-group or PEL state,
increment `server.dirty`, and propagate changes to AOF / replicas, but
they do not always mark the key as modified for `WATCH` and client
tracking invalidation.

Affected command paths:
- `XGROUP CREATE/SETID/DESTROY/CREATECONSUMER/DELCONSUMER`
- `XSETID`
- `XREADGROUP`
- `XACK`
- `XCLAIM`
- `XAUTOCLAIM`

Changes:
- Added `signalModifiedKey()` to `XGROUP
  CREATE/SETID/DESTROY/CREATECONSUMER/DELCONSUMER`
- Added `signalModifiedKey()` to `XSETID`
- Updated `XREADGROUP` to signal key modification when synchronous group
  reads or consumer creation mutate stream metadata
- Updated `XACK`, `XCLAIM`, and `XAUTOCLAIM` to signal key modification
  once per command when they actually mutate PEL or consumer-group state

Additional change:
- Don't emit xdel event for XDELEX DELREF, when removing an orphaned PEL
  ref without deleting the actual stream entry.

---------

Signed-off-by: Tarte <emprimula@gmail.com>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
## Summary

Closes #3726

Implements ACL roles as described in the issue. A role is a named,
reusable set of ACL selectors that can be assigned to multiple users.
This allows operators to define permission policies once and apply them
to many users, avoiding per-user rule duplication.

### Core design

- A role holds its own list of selectors (same structure as user
selectors). Internally a role is a `user` struct with `USER_FLAG_ROLE`
set, stored in a separate `Roles` radix tree, so it reuses the existing
selector machinery.
- Users hold pointers to role objects. Every permission check evaluates
the user's own selectors first, then each role's selectors (OR logic) —
this covers command, key, channel, and unrestricted-key-access checks.
- Role updates modify selectors in-place, immediately visible to all
members (zero-cost propagation).
- Users can only add permissions on top of roles, not restrict them
(consistent with existing multi-selector OR semantics).
- Roles cannot be nested, cannot have passwords, and cannot be
enabled/disabled — those modifiers are rejected.
- Role names must not collide with an existing command or ACL category
name.

### New commands

All new in 9.2.0.

| Command | Description |
|---------|-------------|
| `ACL SETROLE <name> <rules...>` | Create or update a role |
| `ACL DELROLE <name> [name ...]` | Delete one or more roles; fails if
any role still has members, and returns the number actually deleted
(mirrors `ACL DELUSER`) |
| `ACL GETROLE <name>` | Show role permissions and members |
| `ACL ROLES` | List all role names |
| `ACL SETUSER <user> +@ROLE:<name>` | Assign a role to a user |
| `ACL SETUSER <user> -@ROLE:<name>` | Remove a role from a user |

### ACL file and config support

- Roles can be defined in the ACL file with the `role` keyword or inline
in `valkey.conf`.
- ACL file loading uses a two-pass approach (roles first, then users) so
definition order doesn't matter. Duplicate role definitions are
rejected, as duplicate users already were.
- Role rules in `valkey.conf` may reference commands and categories that
a module registers later — roles are loaded after modules, same as
users.
- `ACL SAVE` writes roles before users.
- `ACL LIST` outputs roles before users.
- `CONFIG REWRITE` persists the in-memory roles, so roles created or
deleted at runtime survive a restart.

## Tests

### 1. ACL commands (runtime)

- Role CRUD (`SETROLE`, `DELROLE`, `GETROLE`, `ROLES`), including
deleting multiple roles at once
- Role rule validation: rejects passwords, on/off, nested roles,
unmatched parenthesis, names with spaces, and names colliding with a
command or category
- Role assignment and removal (`+@ROLE:<name>`, `-@ROLE:<name>`),
including empty and non-existent role names
- `ACL DRYRUN` respects role selectors; role changes are immediately
visible to members
- Multiple roles with OR logic, roles with multiple selectors, user
permissions adding on top of a role, user cannot restrict role
permissions
- Channel patterns via roles, and pubsub clients disconnected when
`SETROLE` or `SETUSER` revokes channel access
- `ACL LIST` includes roles; user reset clears role memberships
- Role and user names are case-sensitive on both sides of the membership
- `SORT` `BY`/`GET` honour full key access granted only through a role

### 2. ACL file (`aclfile` option)

- Roles and users loaded from a dedicated ACL file; definition order
doesn't matter
- User-level permissions add on top of a role from the ACL file
- `ACL SAVE` and `ACL LOAD` preserve roles
- The default user keeps its role membership across `ACL LOAD`, and a
role it holds cannot be deleted
- Error paths: invalid role rules, `role` line without a name, duplicate
role definitions

### 3. Inline directives in `valkey.conf`

- Both `role` and `user` directives loaded from the main config; role
permissions effective for users defined in the same config
- Startup fails on duplicate roles, invalid role rules, and role names
colliding with a command or category
- `CONFIG REWRITE` persists roles created at runtime and drops roles
deleted at runtime

### 4. Module API

- `VM_ACLCheckKeyPermissions` and `VM_ACLCheckChannelPermissions` honour
grants that reach the user only through a role
- Roles pick up module commands in a granted category when the module
loads
- Module unload is blocked while a role references one of its commands
- A role declared in `valkey.conf` can reference a module command

## Backwards compatibility

- Existing ACL files without roles load correctly with no changes
required.
- Existing `valkey.conf` files without `role` directives work as before.
- `ACL GETUSER` output adds a new `roles` field but all other fields
remain unchanged.
- `ACL LIST` prepends role entries before user entries; existing user
entry format is unchanged.

---------

Signed-off-by: Yang Zhao <zymy701@gmail.com>
Signed-off-by: Harkrishn Patro <h_patro@apple.com>
Co-authored-by: Harkrishn Patro <h_patro@apple.com>
Co-authored-by: Harkrishn Patro <bunty.hari@gmail.com>
…M flag to SCRIPT LOAD (#866)

By default, Valkey caches up to 500 scripts loaded via EVAL using count-based
LRU eviction. Scripts loaded via SCRIPT LOAD are exempt from this count limit.
However, this can lead to memory abuse if users load a large number of scripts
via SCRIPT LOAD, or if a small number of EVAL scripts are very large.

To address this memory abuse, we introduce two complementary changes:
maxmemory-scripts provides a memory limit for cached EVAL scripts, while SCRIPT
LOAD is marked with DENYOOM and handled by the normal global maxmemory path.

The maxmemory-scripts option provides memory-based eviction control for EVAL
scripts. When the configured memory limit is exceeded, existing EVAL scripts
are evicted from the EVAL LRU list until memory usage drops below the threshold.
Scripts loaded via SCRIPT LOAD are not evicted by maxmemory-scripts.
Note: The 500-script limit is independent of the maxmemory-scripts memory limit.

The SCRIPT LOAD command is now marked with DENYOOM and is handled by the normal
global maxmemory path, which may evict keys or reject the command with an OOM error.

Signed-off-by: Binbin <binloveplay1314@qq.com>
…#4403)

Add a new module API function `ValkeyModule_ScanKeyRawBorrowed` to allow
scanning keys without allocating.

---------

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>
Signed-off-by: Karthik Subbarao <karsubba@amazon.com>
Co-authored-by: Karthik Subbarao <karsubba@amazon.com>
#### Summary

Adds **per-replica streaming compression** for replication, with LZ4 as the first codec. This PR extends the negotiated stream into steady-state replication.

Compression is disabled by default with `repl-compression no`. A replica with replication compression enabled advertises the existing codec-specific `REPLCONF capa lz4`, and a primary compresses only when its own configuration enables LZ4 and the replica advertised support. Older or opted-out replicas continue using plaintext, so mixed deployments fall back safely.

The configuration applies as follows:

- **Diskless full sync:** whole-stream LZ4 is controlled by `repl-compression`; when it is not selected, `rdbcompression` may still apply per-string LZF
- **Disk-based full sync:** controlled by `rdbcompression`, because the generated RDB may become the persisted snapshot
- **Steady-state replication:** controlled by `repl-compression`

Compression runs on the existing write path, on an IO thread or the main-thread fallback, with a bounded 1 MB raw-input batch. Replication-buffer cursor updates remain on the main thread. On the replica, `streamPushReader` is installed when LZ4 was advertised, detects the `VCS_STREAM_REPL` envelope, and decodes directly into the query buffer; plaintext streams pass through unchanged.

**Headline results** (BlockMesh tweets, 3M keys × ~315 B):

| | Bandwidth savings | Compression CPU | Throughput overhead |
|---|---:|---:|---:|
| LZ4 level 0 (default) | 52% | 2.5s | <1% |
| ZSTD level 9 (future, [#3798](valkey-io/valkey#3798)) | 75% | 29.7s | <3% |

**Configuration added:**

- `repl-compression`: `no` (default), `yes` (currently LZ4), or `lz4`

**Capability used:**

- `lz4`: the replica accepts LZ4 streaming-compressed replication payloads


Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
…s (#4676)

`ValkeyModule_FreeModuleUser`, `ValkeyModule_ACLAddLogEntry` and
`ValkeyModule_ACLAddLogEntryByUserName` are declared in valkeymodule.h
as returning void, but the implementations (`VM_FreeModuleUser`,
`VM_ACLAddLogEntry`, `VM_ACLAddLogEntryByUserName`) return int, and the
latter two document `VALKEYMODULE_OK` / `VALKEYMODULE_ERR`. Modules
calling them through the API table therefore call an int function via a
void function pointer, which is undefined behavior in C. On x86-64 and
arm64 the discarded return register makes it harmless in practice; on a
target with strict indirect-call signature checking (WebAssembly) the
call traps.

Declare them as int to match the implementation. Existing modules that
ignore the return value are unaffected; modules can now check the result
of the ACL log calls as documented. Also document that `FreeModuleUser`
returns `VALKEYMODULE_OK`.

Found by compiling the server with Emscripten, where
`redis.acl_check_cmd` in a Lua script (which calls `FreeModuleUser`
through the API table) trapped with 'null function or function signature
mismatch'.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
The test froze the replica and pipelined 30 x 1MB SETs before asserting the
replica was still connected. The throttler's soft-limit exemption only holds for
4 * STEADY_STATE_CONVERGENCE_SECS = 120s after the COB crosses the soft limit
(src/throttle_repl.c:136-141), and with soft_limit_seconds 0 nothing else keeps
the replica alive. The throttler rate-limits ops/sec and drives the rate toward
zero while the replica is frozen, so a fixed-op-count write phase has no time
bound and outlasts the exemption on slow builds.

Bound the write phase by the COB reaching the state under test, then hold there
for 3s so the soft-limit second boundary is still crossed.

Signed-off-by: Madelyn Olson <matolson@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[daily-ci] FLAKY-TEST: Throttling protects a replica above the soft COB limit races the 120s throttler exemption cap