Steady state replication throttling POC - #3
Open
harrylin98 wants to merge 37 commits into
Open
Conversation
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
…ths (valkey-io#3600) The `pending_command` flag indicates that a client has a fully parsed command ready for execution. This update ensures that the flag is set/cleared consistently across different execution paths. --------- Signed-off-by: harrylin98 <harrylin980107@gmail.com>
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
harrylin98
force-pushed
the
rate_limit_poc
branch
4 times, most recently
from
June 24, 2026 04:54
430a560 to
72ebc75
Compare
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
harrylin98
force-pushed
the
rate_limit_poc
branch
from
June 24, 2026 05:29
72ebc75 to
5904359
Compare
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
JimB123
suggested changes
Jun 24, 2026
JimB123
left a comment
JimB123
left a comment
There was a problem hiding this comment.
I focused on the APIs. Big comments:
- APIs (.h files) need solid documentation
- Avoid pulling in Amazon overdesign
- Consider each thing in the .h file. Ask does this need to be part of the API? Why?
| long oldest_client_delay_us; | ||
| } throttleMetrics; | ||
|
|
||
| /* Public API */ |
There was a problem hiding this comment.
The public API should be fully documented.
There was a problem hiding this comment.
+1, This is applicable to all the .h files we have included here.
|
|
||
| void tokenBucket_capDebt(tokenBucket *bucket, double max_debt); | ||
| double tokenBucket_add(tokenBucket *bucket, double tokens); | ||
| double tokenBucket_replenish(tokenBucket *bucket); |
There was a problem hiding this comment.
No need for this to be an API. This can happen intrinsically.
dubey02
reviewed
Jul 6, 2026
| long oldest_client_delay_us; | ||
| } throttleMetrics; | ||
|
|
||
| /* Public API */ |
There was a problem hiding this comment.
+1, This is applicable to all the .h files we have included here.
| tokenBucket *bucket; | ||
| list *client_queue; | ||
| listNode *ln; /* my node in throttlerList */ | ||
| monotime rate_below_guardrail_since; |
There was a problem hiding this comment.
Isn't this field more related to repl-throttle specifically? I think a generic throttle framework shouldn't care about guardrails. Can we please track it in throttle-repl.c somewhere ?
| static list *throttlerList = NULL; | ||
| static hashtable *metricsTable = NULL; | ||
|
|
||
| typedef struct throttler { |
There was a problem hiding this comment.
One liner Documentation on structures and its members would help the readability.
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
harrylin98
force-pushed
the
rate_limit_poc
branch
from
July 10, 2026 19:11
041c81d to
ec331ac
Compare
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
harrylin98
force-pushed
the
rate_limit_poc
branch
8 times, most recently
from
July 14, 2026 08:44
802f610 to
9a1ca67
Compare
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
harrylin98
force-pushed
the
rate_limit_poc
branch
2 times, most recently
from
July 21, 2026 00:16
e99ce3d to
53b098f
Compare
harrylin98
force-pushed
the
rate_limit_poc
branch
2 times, most recently
from
July 21, 2026 23:30
81ade19 to
c2a2485
Compare
harrylin98
force-pushed
the
rate_limit_poc
branch
5 times, most recently
from
August 11, 2026 23:55
43b5a9d to
6555d4a
Compare
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
harrylin98
force-pushed
the
rate_limit_poc
branch
2 times, most recently
from
August 12, 2026 00:52
c1d0346 to
35244c2
Compare
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
harrylin98
force-pushed
the
rate_limit_poc
branch
from
August 12, 2026 00:59
35244c2 to
8f30657
Compare
…lkey-io#4380) Fixes paths which used a client's cached slot id for keys not directly associated with the client. --------- Signed-off-by: Nitai Caro <caronita@amazon.com> Co-authored-by: Nitai Caro <caronita@amazon.com>
…io#4323) In an earlier commit we didn't properly reset the redaction bitmap during exec, and used the wrong one for lua scripts. This fixes that to properly redact commands. Added new regression tests and all existing tests still pass. --------- Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
…d Tcl tests (valkey-io#2243) Fix spelling and grammar issues across 17 files This is a subset of valkey-io#2183. --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
…-io#3922) ## Problem Two crafted-`RESTORE` crashes in stream loading. In both, the payload passes the existing structural validation but violates an invariant downstream code relies on. **Any client with `RESTORE` access can remotely crash the server.** ### 1. Length vs. tombstones A stream can claim a positive `length` while every listpack entry is a tombstone (`STREAM_ITEM_FLAG_DELETED`). The length is loaded directly from the payload and only checked against the rax being non-empty. `streamLastValidID()` then finds no non-tombstone entry while `s->length` is non-zero and aborts: ``` serverPanic("Corrupt stream, length is %llu, but no max id", ...) // t_stream.c ``` Triggered by `XSETID` / `XADD` / `XREADGROUP`. Confirmed: a 2-entry stream with both entries flagged `DELETED` and `length=1` loads OK, then `XSETID` panics. ### 2. Negative field counts A master entry (or a per-entry field count for non-`SAMEFIELDS` entries) can declare a **negative** number of fields. The validator only checked `lpGetIntegerIfValid()`'s success flag, not the sign. The negative count drives listpack traversal in `streamIteratorGetID()`, walking past the listpack and asserting (`lpAssertValidEntry`) on `XRANGE` and similar reads. Confirmed: crafted payload loads OK, then `XRANGE` aborts at `listpack.c`. ## Fix 1. `streamValidateListpackIntegrity()` already parses each listpack's master entry count (live entries). Sum it across listpacks via a new out-parameter and reject the payload if it does not match the loaded length. This reuses the assertion-safe parsing rather than iterating the stream with `streamIteratorGetID()`, which can itself hit entry-level assertions on *other* malformed payloads (an earlier iterate-based version regressed three existing corrupt-dump tests). 2. Reject negative `primary_fields` and per-entry `fields` counts during validation. ## Testing - Two `RESTORE`-path integration tests in `tests/integration/corrupt-dump.tcl`. - Both verified to **fail pre-fix** (panic / assert) and **pass post-fix**. - Confirmed legitimate streams — including ones with real tombstones (5 entries, 2 deleted) and multi-field entries — still load and read correctly. - Full `integration/corrupt-dump` suite: 75 passed, 0 failed (including the three stream consumer-group tests an earlier iterate-based approach broke). > [!NOTE] > Found via structure-aware fuzzing + code review of the RESTORE path. This issue was generated by AI but verified, with love, by a human. --------- Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Grammatical errors are fixed in documentation Signed-off-by: takahashi shun <mokopoi44@gmail.com> Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
Fixes grammatical and spelling errors --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
…valkey-io#4368) In `generateStringArgValue`, the `argName` dispatch chain contains two identical `else if (strcmp(argName, "command") == 0)` branches. The earlier branch already handles every `argName == "command"` case, so the second one, whose body is identical, is unreachable dead code. This removes the duplicate branch; behavior is unchanged. Signed-off-by: latent-9 <296084221+latent-9@users.noreply.github.com>
Improves spelling and grammar across codebase --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Grammar corrections in comments --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
…ey-io#4381) Stream listpack master entries store separate live and deleted record counts, but integrity validation only verified their sum. A corrupted payload could therefore preserve the total while understating the live count, causing XLEN to disagree with the stored records and allowing XDEL to discard records not accounted for by the header. Count live and deleted records independently while validating stream listpacks and reject payloads when either count differs from its declared value. Signed-off-by: Roshan Khatri <roshanvkhatri@gmail.com>
Consolidating nonexistent spelling Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
…alkey-io#4402) `src/commands/config-info.json` and `src/commands/move.json` still carry `"since": "10.0.0"`. *This was generated by AI but verified, with love, by a human.* Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
…load (valkey-io#4360) moduleUnregisterCleanup did not remove the module's cluster message receivers. The stale entries kept pointing at the freed ValkeyModule, so a later cluster message of that type dereferenced r->module in moduleCallClusterReceivers, causing a use-after-free. Added a MODULE UNLOAD test to verify the fix, and also added type=254 to allow us to verify the correctness of the loop logic. --------- Signed-off-by: Binbin <binloveplay1314@qq.com>
…ndlers (valkey-io#4401) Follow-up of valkey-io#3611. Fixes a regression introduced by that PR. The May 27 On-Demand run on this PR (SET/GET, 96B, io-threads 2/10, pipeline 1/10) showed no significant RPS change. After `17ec23f` removed `post_read_done_postpone_mask`, `processClientIOReadsDone()` started postponing READ and returning `needs_post_read_update = 1` for every non-ACCEPTING completed read. That second phase (`lookupClientByID` + `processPendingCommandAndInputBuffer` + `connUpdateState`) is only required when `update_state` may synchronously invoke handlers. For other transports it is per-completion overhead. This matches the post-merge dashboard drop: small payloads, io-threads, **P1 worse than P10**. This PR restores the original gate without bringing `struct client` into the connection driver (the review concern that led to `17ec23f`): - `ConnectionType.sync_handlers_in_update_state` (0 by default) - Set only where `update_state` can sync-call handlers - Mask is still computed in `networking.c` from IO state - `connUpdateState()` still runs immediately, including ACCEPTING --------- Signed-off-by: quanyeyang <quanyemostima@gmail.com>
Log EXEC in commandlog. Catches cases where there's no individually slow command. --------- Signed-off-by: Michelle Lee <michellee.3104@gmail.com>
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
harrylin98
force-pushed
the
rate_limit_poc
branch
from
August 13, 2026 21:54
7aaa28a to
2d69b73
Compare
…lkey-io#4404) `unit/commandlog` fails on unstable at 917de6a: ``` *** [err]: COMMANDLOG slow - Redaction does not leak to later commands in a MULTI in tests/unit/commandlog.tcl Expected 'set foo bar' to be equal to 'exec' (context: type eval line 12 cmd {assert_equal {set foo bar} [lindex [lindex $slowlog_resp 0] 3]} proc ::test) ``` valkey-io#4267 dropped `SKIP_COMMANDLOG` from EXEC (`src/commands/exec.json:12`), so EXEC is now logged after the commands it ran, which puts it at entry 0 of the newest-first `COMMANDLOG GET`. The redaction test at `tests/unit/commandlog.tcl:212`, added by valkey-io#4323, still reads entry 0 and gets the EXEC instead of the SET. Neither PR was rebased on the other, so this only broke on merge and not in either PR's CI. The log after `MULTI; ACL SETUSER commandlog-test-user +get; SET foo bar; EXEC`: | Entry | Command | |---|---| | 0 | `exec` | | 1 | `set foo bar` | | 2 | `acl setuser (redacted) (redacted)` | Read the SET from entry 1 instead. The other tests valkey-io#4267 touched already assert entry 0 is `exec` and entry 1 is the inner command, so this matches. Also assert entries 0 and 2 rather than only the SET. The point of the test is that the ACL SETUSER redaction stops at the ACL SETUSER, and asserting entry 2 is redacted is what keeps it from passing if redaction breaks entirely. Asserting entry 0 is `exec` makes the index arithmetic fail loudly next time the entry order changes, instead of silently comparing against the wrong entry the way it just did. ## Testing `unit/commandlog` goes from 29 passed, 1 failed to 30 passed. `unit/slowlog` and `unit/multi` are unchanged at 100 passed. Reverting the `src/` hunks of 917de6a confirms that commit is the trigger. The new entry-0 assert fails, along with the two EXEC tests 917de6a added: ``` *** [err]: COMMANDLOG slow - Redaction does not leak to later commands in a MULTI in tests/unit/commandlog.tcl Expected 'exec' to be equal to 'set foo bar' (context: type eval line 13 cmd {assert_equal {exec} [lindex [lindex $slowlog_resp 0] 3]} proc ::test) *** [err]: COMMANDLOG slow - EXEC is logged alongside slow inner commands in tests/unit/commandlog.tcl Expected '1' to be equal to '2' (context: type eval line 8 cmd {assert_equal [r commandlog len slow] 2} proc ::test) *** [err]: COMMANDLOG slow - EXEC records total transaction time when inner commands are individually fast in tests/unit/commandlog.tcl Expected '0' to be equal to '1' (context: type eval line 9 cmd {assert_equal [r commandlog len slow] 1} proc ::test) ``` *This was generated by AI but verified, with love, by a human.* Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
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
12 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.