Reclaim dead client IDs from the tracking table (#4143) - #4190
Reclaim dead client IDs from the tracking table (#4143)#4190rayjinghaolei wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughClient tracking maintenance now removes dead client IDs incrementally, enforces an optional aggregate item limit, and defragments tracking-table radix trees. Configuration, cron integration, active-defrag integration, unit tests, integration tests, and documentation are included. ChangesTracking table cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant serverCron
participant trackingSweepDeadClients
participant trackingLimitInnerItems
participant TrackingTable
participant trackingInvalidateKey
serverCron->>trackingSweepDeadClients: Run periodic dead-ID sweep
trackingSweepDeadClients->>TrackingTable: Remove disconnected client IDs
serverCron->>trackingLimitInnerItems: Enforce aggregate item limit
trackingLimitInnerItems->>TrackingTable: Select over-limit keys
trackingLimitInnerItems->>trackingInvalidateKey: Evict whole keys and deliver invalidations
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tracking.c`:
- Around line 685-711: The max-items enforcement loop around timeout_counter and
trackingInvalidateKey can perform unbounded work in one cron tick because effort
grows without a ceiling. Replace the timeout_counter-derived effort with a
fixed, bounded attempt or time budget, while retaining the existing early return
when TrackingTableTotalItems reaches max_items and allowing subsequent cron runs
to continue cleanup.
- Around line 616-629: Make tracking maintenance resumable within each inner
radix tree: in src/tracking.c lines 616-629, persist an inner-ID cursor, process
only bounded work per call, and remove the allocation sized to the entire tree;
in src/defrag.c lines 1108-1124, make inner-tree defragmentation retain its
cursor and honor deadline interruptions; in src/unit/test_tracking.cpp lines
207-242, add a one-key/many-ID regression asserting bounded per-call progress.
In `@tests/unit/memefficiency.tcl`:
- Around line 606-609: Re-select the intended test database after each HELLO
handshake in the tracking and validation client setup: at
tests/unit/memefficiency.tcl lines 606-609 and 643-646, after consuming the
HELLO reply, conditionally issue SELECT 9 and read its response when $::singledb
is false. Ensure both clients operate on the populated database while preserving
single-database behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0cdf042-d002-40b1-a863-ddefdfdee531
📒 Files selected for processing (10)
design-docs/tracking-table-cleanup.mdsrc/config.csrc/defrag.csrc/server.csrc/server.hsrc/tracking.csrc/unit/test_tracking.cpptests/unit/memefficiency.tcltests/unit/tracking.tclvalkey.conf
| set tc [valkey_deferring_client] | ||
| $tc hello 3 | ||
| $tc read | ||
| $tc client tracking on |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Re-select the test database after sending HELLO.
The HELLO command re-initializes the client connection state, which resets the selected database back to DB 0. Based on learnings, when using the test framework's valkey_deferring_client, you must explicitly select the intended DB (usually DB 9) if the auto-selection is bypassed or reset, unless $::singledb is set. Without this, the tracking clients will silently operate on DB 0 while the data is populated and modified in DB 9.
tests/unit/memefficiency.tcl#L606-L609: insertif {!$::singledb} { $tc select 9; $tc read }after theHELLO 3reply is consumed to ensure tracking clients read the populated keys.tests/unit/memefficiency.tcl#L643-L646: insert the same DB selection logic for the validation client.
✅ Proposed fix for the affected sites
For tests/unit/memefficiency.tcl#L606-L609:
- $tc hello 3
- $tc read
+ $tc hello 3
+ $tc read
+ if {!$::singledb} {
+ $tc select 9
+ $tc read
+ }For tests/unit/memefficiency.tcl#L643-L646:
- $tc hello 3
- $tc read
+ $tc hello 3
+ $tc read
+ if {!$::singledb} {
+ $tc select 9
+ $tc read
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set tc [valkey_deferring_client] | |
| $tc hello 3 | |
| $tc read | |
| $tc client tracking on | |
| set tc [valkey_deferring_client] | |
| $tc hello 3 | |
| $tc read | |
| if {!$::singledb} { | |
| $tc select 9 | |
| $tc read | |
| } | |
| $tc client tracking on |
📍 Affects 1 file
tests/unit/memefficiency.tcl#L606-L609(this comment)tests/unit/memefficiency.tcl#L643-L646
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/memefficiency.tcl` around lines 606 - 609, Re-select the intended
test database after each HELLO handshake in the tracking and validation client
setup: at tests/unit/memefficiency.tcl lines 606-609 and 643-646, after
consuming the HELLO reply, conditionally issue SELECT 9 and read its response
when $::singledb is false. Ensure both clients operate on the populated database
while preserving single-database behavior.
Source: Learnings
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| /* Defrag this key's inner radix tree (IDs only, no data values) and | ||
| * write back the possibly-relocated pointer to the outer entry. */ | ||
| rax *ids = ri.data; | ||
| defragRadixTree(&ids, 0, NULL, NULL); |
There was a problem hiding this comment.
defragRadixTree(&ids, 0, NULL, NULL) still walks the entire inner tracking rax in one shot (src/defrag.c:607-623), with no endtime check or active_defrag_max_scan_fields cutoff. A single tracked key can accumulate one entry per tracking client (src/tracking.c:246-259), so one hot key can keep this stage inside one outer entry for far longer than the time budget even though the outer loop is cursor-based. Other large rax defrag paths switch to deferred/resumable work once they cross active_defrag_max_scan_fields (src/defrag.c:678-683); this stage needs the same treatment or active defrag can still stall on a heavily tracked key.
| assert_equal [expr {$num_clients * $num_keys}] $items_before | ||
| # Give background maintenance ample time to run. A correct | ||
| # implementation must NOT remove IDs for connected clients. | ||
| after 1100 |
There was a problem hiding this comment.
after 1100 makes this test race the cron loop. The new sweeper only runs from serverCron once per second (src/server.c:1731-1733), so on a slow runner the first eligible tick can land later than 1.1s and this assertion fails even though live IDs were preserved. Use wait_for_condition here instead of a fixed sleep.
In non-BCAST client-side caching, a disconnecting client's ID is intentionally left in each tracked key's inner radix tree and is only reclaimed when the key is modified, deleted, evicted, or flushed. Under read-heavy, low-mutation workloads the dead IDs accumulate without bound: tracking-table-max-keys caps only the outer key count, and the tracking table did not participate in active defrag. Address the three expected behaviors from the issue: * Sweeper: trackingSweepDeadClients() runs from serverCron, visiting a bounded number of keys per call with a resume cursor. IDs whose client no longer exists (lookupClientByID(id) == NULL) are removed, TrackingTableTotalItems stays in sync, and keys whose inner radix tree becomes empty are removed. The disconnect path is unchanged and stays O(1); IDs of connected clients are never removed. * Aggregate bound: new tracking-table-max-items config (default 0 = no limit) caps the total item count. Enforcement evicts whole keys via the same random-walk + trackingInvalidateKey mechanism as tracking-table-max-keys, preserving invalidation-on-eviction semantics. * Defrag: new defragStageTrackingTable() active-defrag stage relocates the outer radix tree and each key's inner radix tree, cursor-based and deadline-aware so a large table never stalls the event loop. Fixes valkey-io#4143 Signed-off-by: Ray Lei <rayjinghaolei@gmail.com>
15bd31f to
b406abe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/unit/test_tracking.cpp`:
- Around line 124-129: Update the test setup and teardown around
getTrackingTable and TrackingTableTotalItems to preserve the pre-existing global
table and counter: save both before replacing them with a fresh table and zero
count, then restore them after clearing the sweeper cursor and free only the
temporary table. Keep the temporary tracking_table_max_items setup and existing
test isolation behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7944e0c7-cf40-4696-aa32-8e246ec95eb8
📒 Files selected for processing (10)
design-docs/tracking-table-cleanup.mdsrc/config.csrc/defrag.csrc/server.csrc/server.hsrc/tracking.csrc/unit/test_tracking.cpptests/unit/memefficiency.tcltests/unit/tracking.tclvalkey.conf
🚧 Files skipped from review as they are similar to previous changes (9)
- src/server.h
- valkey.conf
- src/config.c
- design-docs/tracking-table-cleanup.md
- src/tracking.c
- tests/unit/tracking.tcl
- src/server.c
- src/defrag.c
- tests/unit/memefficiency.tcl
| /* Start every test from a fresh, empty tracking table and counter. */ | ||
| rax **tt = getTrackingTable(); | ||
| *tt = raxNew(); | ||
| TrackingTableTotalItems = 0; | ||
| saved_max_items = server.tracking_table_max_items; | ||
| server.tracking_table_max_items = 0; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve pre-existing global tracking state.
Lines 124-129 replace the global table and counter, while teardown frees the replacement and leaves the original table/count lost. If another test populated tracking state, this leaks it and makes later tests order-dependent. Save and restore both values after clearing the sweeper cursor.
Proposed fix
class TrackingTest : public ::testing::Test {
protected:
rax *saved_clients_index;
+ rax *saved_tracking_table;
+ uint64_t saved_tracking_table_total_items;
size_t saved_max_items;
@@
/* Start every test from a fresh, empty tracking table and counter. */
rax **tt = getTrackingTable();
+ saved_tracking_table = *tt;
+ saved_tracking_table_total_items = TrackingTableTotalItems;
*tt = raxNew();
TrackingTableTotalItems = 0;
@@
trackingSweepDeadClients();
+ *tt = saved_tracking_table;
+ TrackingTableTotalItems = saved_tracking_table_total_items;
for (int i = 0; i < num_fake_clients; i++) zfree(fake_clients[i]);Also applies to: 132-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/unit/test_tracking.cpp` around lines 124 - 129, Update the test setup and
teardown around getTrackingTable and TrackingTableTotalItems to preserve the
pre-existing global table and counter: save both before replacing them with a
fresh table and zero count, then restore them after clearing the sweeper cursor
and free only the temporary table. Keep the temporary tracking_table_max_items
setup and existing test isolation behavior unchanged.
1 participant
Fixes #4143
In non-BCAST client-side caching, a disconnecting client's ID is intentionally left in
each tracked key's inner radix tree and is only reclaimed when the key is modified,
deleted, evicted, or flushed. Under read-heavy, low-mutation workloads these dead IDs
accumulate without bound. This PR addresses each of the expected behaviors from the issue:
There should be a limitation on the inner rax size (in aggregate)
tracking-table-max-itemscaps the total number of client-ID entries acrossall inner raxes (the value reported as
tracking_total_itemsinINFO).trackingLimitInnerItems()evicts whole keys fromserverCronusing the same random-walk +trackingInvalidateKeymechanism as theexisting
tracking-table-max-keysenforcement, so evicted keys still send invalidationmessages to their live tracking clients.
0(unlimited), consistent withtracking-table-max-keys: no behaviorchange unless opted in.
A mechanism must exist to clean up dead entries (sweeper)
trackingSweepDeadClients(), an incremental sweeper driven fromserverCron,on by default.
cursor, so it never performs a synchronous O(N) scan.
lookupClientByID(id) == NULL),keeps
TrackingTableTotalItemsin sync, and removes keys whose inner rax becomesempty.
disableTracking) is untouched and stays O(1); IDs ofstill-connected clients are never removed.
Defragmentation should be applied to the TrackingTable
defragStageTrackingTable(), registered alongside the otherglobal-structure stages.
struct/nodes and each key's inner rax across multiple invocations, so a large table
never stalls the event loop.
one shot.
Validation
tests/unit/tracking.tcl: regression test proving dead IDs are reclaimed afterdisconnect without any key modification; tests that live IDs are never touched and
delivery (redirection/NOLOOP/BCAST) is unchanged; end-to-end
tracking-table-max-itemsenforcement test.
src/unit/test_tracking.cpp: GTest unit tests for sweeper removal, cursorincrementality, and bound enforcement.
tests/unit/memefficiency.tcl: new "Active Defrag tracking table" test verifyingdefrag efficacy (frag ratio recovers below 1.1) on a fragmented tracking table.
design-docs/tracking-table-cleanup.md.