fix(conn): stop timeout N from silently disabling the c1M connection park (c10k D1) - #427
Merged
Conversation
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughIdle timeout enforcement moves from per-connection read loops to shard-level registry sweeps for both runtimes. Blocked and replica states are tracked, parked connections are counted and exposed through ChangesIdle timeout lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant ShardEventLoop
participant ClientRegistry
participant ParkedWatcher
Client->>Handler: Send commands or remain idle
Handler->>ClientRegistry: Track activity and blocked state
ShardEventLoop->>ClientRegistry: Run kill_idle_clients on 1-second tick
ClientRegistry->>ParkedWatcher: Mark and force-close eligible idle connection
ParkedWatcher->>Client: Close killed parked connection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
PR Summary by QodoFix idle timeout so
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/idle_timeout_sweep.rs (1)
104-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication:
dirpath recomputed instead of reused.
server()builds the samemoon-{tag}-{port}path twice — once inside thespawn_listeningclosure (Line 111) and again afterward (Line 115) — relying on both computations staying in sync. Consider capturing the value once (e.g., via aRefCell/return from the closure or restructuringspawn_listeningto hand back the dir) to avoid the duplicateformat!+joinlogic drifting apart later.♻️ Illustrative approach
- let (child, port) = common::spawn_listening(|port| { - let dir = std::env::temp_dir().join(format!("moon-{tag}-{port}")); - let _ = std::fs::create_dir_all(&dir); - spawn(&dir, port, timeout_secs) - }); - let dir = std::env::temp_dir().join(format!("moon-{tag}-{port}")); + let dir_cell = std::cell::RefCell::new(std::path::PathBuf::new()); + let (child, port) = common::spawn_listening(|port| { + let dir = std::env::temp_dir().join(format!("moon-{tag}-{port}")); + let _ = std::fs::create_dir_all(&dir); + *dir_cell.borrow_mut() = dir.clone(); + spawn(&dir, port, timeout_secs) + }); + let dir = dir_cell.into_inner();🤖 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/idle_timeout_sweep.rs` around lines 104 - 117, Update server so the temporary directory path is computed once and reused by both the spawn_listening closure and the returned Server; restructure the closure or spawn_listening interaction as needed to retain that single dir value, while preserving the existing spawn and Server behavior.
🤖 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/client_registry.rs`:
- Around line 404-486: Update kill_idle_clients in
src/client_registry.rs:404-486 to use a shard-partitioned registry or per-shard
secondary index so each sweep iterates only that shard’s clients, rather than
scanning every stripe and filtering entry.shard. Preserve the existing timeout,
exemption, and close behavior. The calls in src/shard/event_loop.rs:1702-1710
and src/shard/event_loop.rs:2464-2472 require no direct change; they should
automatically become proportional to the invoking shard’s client set.
In `@src/server/conn/handler_sharded/mod.rs`:
- Around line 999-1006: Replace the manual blocked-state toggles with a shared
RAII guard that clears the flag on drop, adding the guard API near
ClientLiveState in src/client_registry.rs. Update both blocking
paths—src/server/conn/handler_sharded/mod.rs lines 999-1006 and
src/server/conn/handler_monoio/dispatch.rs lines 1656-1674—to use the guard
around their awaited blocking-command calls; both sites require the replacement.
---
Nitpick comments:
In `@tests/idle_timeout_sweep.rs`:
- Around line 104-117: Update server so the temporary directory path is computed
once and reused by both the spawn_listening closure and the returned Server;
restructure the closure or spawn_listening interaction as needed to retain that
single dir value, while preserving the existing spawn and Server 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 965b922a-614b-4312-9211-a9ff08836a8d
📒 Files selected for processing (11)
.gitignoreCHANGELOG.mdsrc/client_registry.rssrc/command/connection.rssrc/server/conn/handler_monoio/dispatch.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/dispatch.rssrc/server/conn/handler_sharded/mod.rssrc/shard/conn_accept.rssrc/shard/event_loop.rstests/idle_timeout_sweep.rs
TinDang97
added a commit
that referenced
this pull request
Jul 30, 2026
…427 review) Three findings from the automated reviewers on #427, all real. 1. The idle sweep scaled with shard count. The registry is striped by `id % STRIPES`, NOT by shard, so filtering by `entry.shard` after walking every stripe meant each shard scanned the WHOLE registry once per second: O(num_shards x total_clients) per second, 12x the necessary work at --shards 12, all of it holding stripe read locks that contend with the write locks register/deregister need on accept and close. Each shard now sweeps a disjoint subset of stripes (i % num_shards == shard) and does not filter by entry.shard at all — one full pass per second regardless of shard count, genuinely distributed. Killing is safe from any thread (kill_flag is atomic, force_close_fd is a shutdown(2)); this is exactly what CLIENT KILL already does cross-shard. Unit tests keep their per-test synthetic shard isolation through a test-only entry point that walks all stripes and filters by shard, since production no longer does. 2. `blocked` was set and cleared as two statements around the blocking await. A panic or a dropped future (migration, cancellation) skipped the clear, and a client stuck with `blocked` set is exempt from `timeout` FOREVER — the same immortal-connection shape D1 exists to prevent. Replaced with an RAII `BlockedGuard` on both runtimes. 3. A parked connection closed by the sweep or by CLIENT KILL rehydrated a full handler task purely so its first read could observe kill_flag and exit. A fleet-wide timeout expiry wakes them in a burst. The watcher now checks is_killed on wake and closes directly, keeping the close accounting it already owns. New tests: stripe_partition_is_disjoint_and_total (every stripe swept exactly once per round across 1/2/3/4/12/16/24 shards), idle_client_is_reaped_by_exactly_one_shard (never by all, never by none), blocked_guard_clears_on_drop. Gates (Linux VM): fmt; clippy -D warnings on default and runtime-tokio,jemalloc; lib 4477 monoio / 3639 tokio; idle_timeout_sweep 5/5; parked_idle_parity 5/5 and tls_idle_downshift_parity 3/3 — the latter two cover the parked-kill path changed in (3), including parked_connection_visible_and_killable. author: Tin Dang
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/client_registry.rs`:
- Around line 164-191: Add a Loom model test covering the cross-thread atomic
behavior of ClientLiveState, including set_blocked and kill_flag, alongside the
existing ResponseSlot Loom coverage. Exercise concurrent updates and reads under
loom::model so the blocked and killed states remain correctly synchronized
without changing BlockedGuard 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ad3b4a7-c4a5-4a8d-b933-80cc8e3a868f
📒 Files selected for processing (6)
CHANGELOG.mdsrc/client_registry.rssrc/server/conn/handler_monoio/dispatch.rssrc/server/conn/handler_sharded/mod.rssrc/shard/conn_accept.rssrc/shard/event_loop.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
…n park (c10k D1)
The idle-timeout was enforced by a per-connection `select!` arm racing the
read against `sleep(timeout)`, and that arm sat FIRST in the read loop's
if/else chain:
if let Some(dur) = idle_timeout { ... continue }
else if conn.tracking_rx.is_some() { ... }
else if downshifted { ...stage-2 park / task-exit park... }
else if let Some(reg) = idle_reg { ...stage-1 downshift... }
Setting `timeout` therefore made the stage-1 downshift, the stage-2 park and
task-exit parking all structurally unreachable. Every connection silently
reverted from the parked footprint to its full working set — in exactly the
deployments that use the only slowloris knob moon ships. The v0.8.3/v0.8.4
headline feature was off and nothing said so; the code comment at the park
arm even stated the exclusion as if it were intentional.
Two further defects in the same arm: it read `runtime_config.timeout` once at
connection setup, so `CONFIG SET timeout` never reached a live connection; and
it had no exemption for replication links, which Redis exempts.
Enforcement moves to `client_registry::kill_idle_clients`, run once a second
by each shard's chore over the connections that shard owns. The registry
already had every piece: `last_cmd_ms` (touched unconditionally at the end of
every batch on both runtimes), `ClientEntry.shard`, and `force_close_fd` —
the CLIENT KILL mechanism, already documented to tear down a connection parked
in `read()`. So one sweep now covers plain and TLS, parked and unparked, and
task-exit-parked connections that no longer have a task to hold a `select!` —
with no per-connection timer and no new allocation. Redis enforces `timeout`
from serverCron the same way, so enforcement being up to one sweep interval
late matches upstream rather than diverging from it.
Prerequisite fix, included: `ClientFlags::blocked` was hardcoded `false` at
every `touch` call site, so the bit was never set. It was dead code behind
CLIENT LIST's `b` flag, and — the reason it matters here — a sweep would have
seen a client parked in `BLPOP key 0` as idle and closed it. Redis exempts
blocked clients, and so did moon implicitly (a blocked client never reached
the old arm). `set_blocked` now brackets the blocking await on both runtimes.
A `replica` bit is added for the same reason; it is exemption-only and is
deliberately not surfaced by `to_flag_str`, so CLIENT LIST output is
unchanged.
Also adds `parked_clients` to `INFO clients` — a real operational signal (how
much of the fleet is actually parked) and, for tests, a direct assertion that
parking engaged instead of inferring it from process RSS. Counted via an RAII
guard so the gauge stays accurate even when the watcher future is dropped
outright at runtime teardown.
Tests (red/green, verified failing first):
- 8 unit tests for the sweep policy: idle-closed vs active-spared, timeout 0
disabled, blocked/subscriber/replica exemptions, unblocking restores
eligibility, set_blocked preserves other flag bits, per-shard isolation,
idempotence. Each test uses its own synthetic shard id because the
registry is process-global and cargo runs tests in parallel.
- tests/idle_timeout_sweep.rs, 5 e2e cases against a real server. The D1
case asserts `parked_clients >= 1` with `--timeout 60` set; restoring the
old arm fails it with `parked_clients=0`.
Gates: cargo fmt --check; clippy --all-targets on monoio and on
runtime-tokio,jemalloc (zero warnings); lib 4452 monoio / 3617 tokio; the new
e2e green on both runtimes; parked_idle_parity, idle_downshift_parity and
tls_idle_downshift_parity all still green.
author: Tin Dang
…427 review) Three findings from the automated reviewers on #427, all real. 1. The idle sweep scaled with shard count. The registry is striped by `id % STRIPES`, NOT by shard, so filtering by `entry.shard` after walking every stripe meant each shard scanned the WHOLE registry once per second: O(num_shards x total_clients) per second, 12x the necessary work at --shards 12, all of it holding stripe read locks that contend with the write locks register/deregister need on accept and close. Each shard now sweeps a disjoint subset of stripes (i % num_shards == shard) and does not filter by entry.shard at all — one full pass per second regardless of shard count, genuinely distributed. Killing is safe from any thread (kill_flag is atomic, force_close_fd is a shutdown(2)); this is exactly what CLIENT KILL already does cross-shard. Unit tests keep their per-test synthetic shard isolation through a test-only entry point that walks all stripes and filters by shard, since production no longer does. 2. `blocked` was set and cleared as two statements around the blocking await. A panic or a dropped future (migration, cancellation) skipped the clear, and a client stuck with `blocked` set is exempt from `timeout` FOREVER — the same immortal-connection shape D1 exists to prevent. Replaced with an RAII `BlockedGuard` on both runtimes. 3. A parked connection closed by the sweep or by CLIENT KILL rehydrated a full handler task purely so its first read could observe kill_flag and exit. A fleet-wide timeout expiry wakes them in a burst. The watcher now checks is_killed on wake and closes directly, keeping the close accounting it already owns. New tests: stripe_partition_is_disjoint_and_total (every stripe swept exactly once per round across 1/2/3/4/12/16/24 shards), idle_client_is_reaped_by_exactly_one_shard (never by all, never by none), blocked_guard_clears_on_drop. Gates (Linux VM): fmt; clippy -D warnings on default and runtime-tokio,jemalloc; lib 4477 monoio / 3639 tokio; idle_timeout_sweep 5/5; parked_idle_parity 5/5 and tls_idle_downshift_parity 3/3 — the latter two cover the parked-kill path changed in (3), including parked_connection_visible_and_killable. author: Tin Dang
TinDang97
force-pushed
the
fix/c10k-timeout-park-d1
branch
from
August 6, 2026 15:07
2fb7184 to
afbcf9c
Compare
This was referenced Aug 6, 2026
2 tasks
This was referenced Aug 7, 2026
This was referenced Aug 7, 2026
Merged
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
1 participant
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.
Problem (c10k hardening finding D1)
Setting
timeout N— the only slowloris knob moon ships — silently disabled the entire c1M connection park.timeoutwas enforced by aselect!arm racing the idle read againstsleep(timeout), and that arm sat first in the read loop's if/else chain (handler_monoio/mod.rs). Taking it made the stage-1 downshift, the stage-2 park and task-exit parking all structurally unreachable: every connection silently reverted from the parked footprint to its full working set. The regression was invisible — no error, no log, no failing test — and it fired in exactly the deployments that hardened against slowloris.Fix
Enforcement moves off the per-connection timer and onto the existing 1 Hz shard chore, via
client_registry::kill_idle_clients(shard, timeout_secs, now_ms). The registry already carriedlast_cmd_ms,kill_flagandkill_fd; the sweep reuses them and closes idle clients withshutdown(SHUT_RDWR), which works on parked connections precisely because it does not need the handler task to be alive. The read loop keeps no timeout arm at all, so the park engages unconditionally.Redis parity (
clientsCronHandleTimeout): blocked, subscriber and replica clients are exempt.Prerequisite bug, included here
ClientFlags::blockedwas hardcoded false at every call site. A naive sweep would therefore have closedBLPOP key 0clients as idle. This PR addsClientLiveState::set_blocked()and drives it around the blocking await on both runtimes.ClientFlagsalso gainsreplica(bit 3) for the exemption — deliberately not added toto_flag_str, soCLIENT LISToutput stays byte-identical.Observability
INFO clientsgains aparked_clientsgauge (RAII-counted in the park watcher). This is what makes the claim CI-assertable instead of RSS-inferred.Verification
RED/GREEN. Restoring the timeout arm fails the regression test:
Linux memory A/B (moon-dev VM, 500 idle conns,
--conn-park-secs 2, 12s settle,/proc/<pid>/statusVmRSS):timeoutparked_clientsA second run gave 49,201 / 12,443 / 10,772 B — same shape. 4–5× per-connection reduction, and the
timeout=60leg now sits inside the noise band of thetimeout=0control (it beat the control in one run, which reads as equivalent, not cheaper).Absolute numbers are scale-specific: 500 conns on one shard amortizes fixed overhead only 500 ways, hence ~10 kB/conn rather than the 3.25 kB c1M headline. Run-to-run allocator noise is ±1–2 kB/conn; the before→after gap is ~5× that.
Tests. New
tests/idle_timeout_sweep.rs(5 e2e):park_still_engages_with_timeout_set— the D1 regression testidle_connection_is_closed_at_timeout— the policy still firesactive_connection_is_never_closedblocked_client_is_exempt_from_timeout— covers the prerequisite bugsubscriber_is_exempt_from_timeoutPlus 8 unit tests in
client_registry, each on its own synthetic shard id (the registry is process-global and cargo runs tests in parallel; the sweep's existingentry.shardfilter provides the isolation).Gates.
fmt --check; clippy--all-targetszero warnings on default andruntime-tokio,jemalloc; lib tests 4452 monoio / 3617 tokio;idle_timeout_sweep5/5 on both runtimes; existing park suites unchanged —parked_idle_parity(5),idle_downshift_parity(2),tls_idle_downshift_parity(3).Notes
N..N+1s. This matches Redis, whoseserverCrondoes the same.tmp/C10K-HARDENING-REVIEW.md, finding D1.Summary by CodeRabbit
parked_clientstoINFO clientsto improve visibility into parked connections.timeout, including parked connections.CLIENT LIST/CLIENT INFOnow correctly reflect the blocked state.timeout Nentry in the Unreleased changelog with more operational details.