Conversation
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>
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>
…on burst The test fired an unbounded 5000-command burst on a deferring client to grow the frozen replica's COB. Once the throttler activates it stops reading that client's socket, so the remainder of the burst blocks in write(). The replica is SIGSTOPped, so its COB never drains and the throttle rate decays to zero, leaving the writer parked until repl-timeout (1800s) drops the replica, which tears the throttler down and makes the following poll report 'Throttler did not activate.' Write the burst in ~64KB chunks and re-check activation between them, the pattern already used elsewhere in this file. Signed-off-by: Madelyn Olson <matolson@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Client disconnect while throttlinggrows the frozen replica's output buffer with an unbounded 5000-command burst on a deferring client. As soon as the throttler activates it stops reading that client's socket, so the rest of the burst blocks inwrite(), and because the only replica is SIGSTOPped its buffer never drains and the throttle rate decays to zero, so the writer is never released. Nothing moves untilrepl-timeout(1800s) drops the replica, which emptiesserver.replicasand uninstalls the throttler, so the poll that follows reportsThrottler did not activate.This writes the burst in ~64KB chunks and re-checks activation between them, the pattern already used attests/integration/throttle-repl.tcl:141-152.Details
Problem
tests/integration/throttle-repl.tcl:316-319(before this change) sends 5000SET key:$i <1000 bytes>on a deferring client with the replica SIGSTOPped:The replica COB target is
soft_limit / 2= 512KB and the throttle threshold is half of that (src/throttle_repl.c:100-102), so the 1mb soft limit set attests/integration/throttle-repl.tcl:36is crossed after roughly 260 of those 5000 commands. TwoserverCronticks later (src/server.c:1772runs the evaluator every 100ms; the first tick installs the throttler at unlimited rate, the second sets a finite one) the writer is queued andconnSetReadHandler(c->conn, NULL)is called atsrc/throttle.c:174. The primary stops reading the writer's socket while ~4.7MB of the burst is still unsent, so the Tclwrite()blocks.The writer is only released when the token bucket yields a token (
src/throttle.c:143-155), and the rate goes the other way: the frozen replica's COB can only grow, soevaluateSteadyStateThrottlekeeps asking to reduce (src/throttle_repl.c:96-112) and the rate is multiplied byRATE_DECREASE_MULTIPLIER = 0.95on every tick (src/throttle_repl.c:19,src/throttle_repl.c:71). It reaches zero long before the writer drains. The deadlock breaks only atrepl-timeout, whensrc/replication.c:5847-5852drops the replica,listLength(server.replicas) == 0uninstalls the throttler (src/throttle_repl.c:147-152) andrepl_throttle_rategoes back to-1(src/throttle_repl.c:191-196). Thewait_for_conditionattests/integration/throttle-repl.tcl:320-325then fails.Reproduction
The CI failure needs valgrind only because a slow server guarantees the throttler activates early in the burst. Locally the same thing is forced with two knobs, no valgrind: raise the burst payload from 1000 to 100000 bytes so the burst cannot outrun the 100ms evaluator, and lower
repl-timeoutfrom 1800 to 20 so the stall is observable in seconds instead of half an hour. Extract the single test intotests/integration/throttle-repl-repro35.tclwith those two values and run./runtest --single integration/throttle-repl-repro35 --loops 5.Before, 5/5 loops fail with the CI assertion verbatim, ~32s each:
The primary log shows the mechanism exactly as in the CI job. Key count is frozen for 21 seconds while the writer sits in
write(), thenrepl-timeoutfires and the burst floods through with the throttler gone:After, with the same two knobs, 5/5 loops pass in ~1.4s each:
Decisions
wait_for_conditionafter the loop is kept. The chunked loop can finish all 5000 commands before the 100ms evaluator has run at all, so removing the grace poll makes the test fail for the opposite reason.Testing
The reproduction above is the proof: the test fails 5/5 before and passes 5/5 after under the same knobs.
./runtest --single integration/throttle-repl --loops 5still shows two pre-existing flakes in neighbouring tests, present on unmodifiedupstream/unstabletoo.Throttling protects a replica above the soft COB limitfailed 2/5 loops without this change, andThrottling not protect a replica above the hard COB limitfailed 1/5 with it. Both are separate reports and neither touches the code this change edits.This branch is based on
upstream/unstable, not this fork'sunstable, which predates the throttling feature entirely, so the PR carries upstream commits alongside the one test commit.Fixes #35
This was generated by AI but verified, with love, by a human.