fix(blocking): reap clients that disconnect while blocked (c10k A1) - #428
Merged
Conversation
|
Warning Review limit reached
Next review available in: 56 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 (7)
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 QodoReap disconnected blocked clients by watching peer socket (c10k hardening A1)
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Flaky INFO response parsing
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+68
to
+83
| fn read_some(stream: &mut TcpStream) -> std::io::Result<String> { | ||
| let mut buf = [0u8; 8192]; | ||
| let n = stream.read(&mut buf)?; | ||
| Ok(String::from_utf8_lossy(&buf[..n]).into_owned()) | ||
| } | ||
|
|
||
| fn connected_clients(port: u16) -> u64 { | ||
| let mut c = connect(port); | ||
| send(&mut c, &["INFO", "clients"]); | ||
| let body = read_some(&mut c).expect("INFO reply"); | ||
| for line in body.lines() { | ||
| if let Some(rest) = line.strip_prefix("connected_clients:") { | ||
| return rest.trim().parse().unwrap_or(0); | ||
| } | ||
| } | ||
| panic!("INFO clients has no connected_clients; got:\n{body}"); |
There was a problem hiding this comment.
2. Flaky info response parsing 🐞 Bug ☼ Reliability
tests/blocking_peer_eof.rs reads the INFO reply with a single TcpStream::read() and then searches for "connected_clients:", so a short/partial read can make the test panic even when the server is healthy. This can introduce intermittent CI failures for the new regression suite.
Agent Prompt
## Issue description
`connected_clients()` assumes the full RESP reply to `INFO clients` arrives in a single `read()` via `read_some()`. TCP is a byte stream and partial reads are allowed, so this can miss the `connected_clients:` line and panic, making the test flaky.
## Issue Context
`INFO` replies are RESP Bulk Strings (`$<len>\r\n...payload...\r\n`). The test should read/parse the complete frame (or at least read until the full bulk payload is received) before scanning for `connected_clients:`.
## Fix Focus Areas
- tests/blocking_peer_eof.rs[68-83]
## Suggested fix
Replace `read_some()`/`connected_clients()` response handling with RESP-aware reading:
- read the first line to determine reply type
- if it is `$<len>`: read exactly `<len> + 2` bytes (payload + trailing CRLF)
- then scan the complete payload for `connected_clients:`
Alternatively, reuse an existing RESP test helper/parser if the repo already has one.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
TinDang97
changed the base branch from
fix/c10k-hardening-blocking-registry
to
main
August 6, 2026 14:33
The infinite-wait select! behind BLPOP/BRPOP/BLMOVE/BZPOPMIN/BZPOPMAX/
BLMPOP/BZMPOP/BRPOPLPUSH had exactly two arms, reply_rx and shutdown.
Nothing watched the socket, so `BLPOP key 0` followed by a disconnect
leaked the handler task, the WaitEntry, the client-registry entry and the
maxclients slot forever: infinite waiters carry `deadline: None` so the W6
deadline sweep skips them, and `timeout` exempts blocked clients by design
(Redis clientsCronHandleTimeout parity). A few thousand throwaway
connections wedged the server until restart — one unauthenticated command,
not reapable by disconnect, CLIENT KILL, or timeout.
The wait now watches the peer. Two mechanisms, because the runtimes have
different cancellation semantics:
* tokio — AsyncReadExt::read_buf is documented cancel-safe (a losing
select! branch reads nothing), so the watch is a plain read arm.
* monoio — a read owns its buffer, so a read arm losing the race could
take client bytes down with the cancelled op. The watch is instead a
two-step: a non-consuming readiness poll inside the select!, then the
drain OUTSIDE it, where nothing can cancel it. Level-triggered
readiness guarantees that read completes at once. Racing the reply is
harmless — reply_tx is a flume bounded(1), so a wake delivered during
the drain waits in the channel for the next loop turn.
A vanished client tears down every registration it held, local and remote,
through the same cleanup the timeout path uses. CLIENT KILL of a blocked
client flows through this too, since it closes the fd with shutdown(2).
Only a real error is death: EWOULDBLOCK (the legacy driver can hand back a
readiness that yields nothing), EINTR and ECANCELED all re-arm. Misreading
any of them as EOF would drop a healthy blocked client — the exact inverse
of the bug being fixed.
Bytes a client legally pipelines behind its blocking command are carried
into the parse stream instead of being consumed and dropped. That carry
needed a matching fix in both read loops: pre-A1 those bytes sat in the
kernel and the next read returned them at once, but now they are already
in read_buf, so parking in read() first would hang the pipelined command.
Both handlers skip exactly one read after a blocking command leaves
unparsed input; a carry that is only a partial frame parses to nothing and
the loop comes straight back with the flag cleared.
Known gap: TLS keeps the pre-A1 behaviour. The vendored monoio-rustls
read_inner loops on read_io until rustls yields plaintext, so a
post-readiness read can park inside a partial record — the one thing the
cancel-free design must never do. TLS at least requires a completed
handshake, so it is not the unauthenticated one-command DoS this fixes.
IdleParkRead::peer_readable defaults to a never-resolving future, which is
what keeps those streams on exactly the old path.
The carry buffer gets its own 64 MiB ceiling rather than inheriting the
read loop's (which has none — that is finding C2, a separate fix).
Verification (Linux VM, both runtimes):
* RED proven independently per runtime by disabling only that runtime's
peer watch: 24 disconnected single-key clients left connected_clients
at 25, 16 multi-key at 17. The two positive-control tests stayed green
in both RED runs, so the failures are the defect, not the harness.
* tests/blocking_peer_eof.rs — 5 e2e, green on monoio and tokio:
single-key reap, multi-key reap across 4 shards (exercises the remote
BlockCancel unwind and asserts no ghost waiter swallows a later push),
pipelining-while-blocked, partial-frame carry, and idle-blocked-client
stays blocked and wakeable.
* fmt; clippy -D warnings on default and runtime-tokio,jemalloc; lib
tests 4471 monoio / 3633 tokio; blocking_ghost_waiter,
blocking_list_timeout (incl. --ignored), parked_idle_parity,
idle_downshift_parity, tls_idle_downshift_parity all still green.
Refs: tmp/C10K-HARDENING-REVIEW.md finding A1. Stacked on #426 (A2-A6) —
same select! blocks.
author: Tin Dang
TinDang97
force-pushed
the
fix/c10k-blocked-client-eof
branch
from
August 6, 2026 14:34
7421a0d to
42b7e36
Compare
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 A1)
The infinite-wait
select!behindBLPOP/BRPOP/BLMOVE/BZPOPMIN/BZPOPMAX/BLMPOP/BZMPOP/BRPOPLPUSHhad exactly two arms —reply_rxandshutdown. Nothing watched the socket.BLPOP key 0followed by a disconnect therefore leaked the handler task, theWaitEntry, the client-registry entry and the maxclients slot forever:deadline: None, so the W6 deadline sweep skips them;timeoutexempts blocked clients by design (RedisclientsCronHandleTimeoutparity);CLIENT KILLcouldn't reach them either.A few thousand throwaway connections wedge the server until restart. One unauthenticated command, no auth required.
Fix
The wait now watches the peer. Two mechanisms, because the runtimes have different cancellation semantics — this is the substance of the change:
tokio —
AsyncReadExt::read_bufis documented cancel-safe (a losingselect!branch reads nothing), so the watch is a plain read arm.monoio — a read owns its buffer, so a read arm losing the race could take client bytes down with the cancelled op. The watch is instead a two-step: a non-consuming readiness poll inside the
select!, then the drain outside it, where nothing can cancel it. Level-triggered readiness guarantees that drain completes at once (data, EOF, or error). Racing the reply is harmless —reply_txis aflumebounded(1), so a wake delivered mid-drain waits in the channel for the next loop turn.A vanished client tears down every registration it held, local and remote, through the same cleanup the timeout path uses.
CLIENT KILLflows through this too, since it closes the fd withshutdown(2).Only a real error is death.
EWOULDBLOCK(the legacy driver can hand back a readiness that yields nothing),EINTRandECANCELEDall re-arm. Misreading any of them as EOF would drop a healthy blocked client — the exact inverse of the bug being fixed.Byte carry, and the regression it required fixing
Bytes a client legally pipelines behind its blocking command are carried into the parse stream instead of being consumed and dropped. That needed a matching fix in both read loops: pre-A1 those bytes sat in the kernel and the next
read()returned them at once, but now they are already inread_buf, so parking inread()first would hang the pipelined command. Both handlers skip exactly one read after a blocking command leaves unparsed input; a carry that is only a partial frame parses to nothing and the loop comes straight back with the flag cleared.Known gap — TLS
TLS keeps the pre-A1 behaviour. The vendored
monoio-rustlsread_innerloops onread_iountil rustls yields plaintext, so a post-readiness read can park inside a partial record — the one thing the cancel-free design must never do.IdleParkRead::peer_readabledefaults to a never-resolving future, which is what keeps those streams on exactly the old path. TLS at least requires a completed handshake, so it is not the unauthenticated one-command DoS this fixes.The carry buffer gets its own 64 MiB ceiling rather than inheriting the read loop's (which has none — that is finding C2, a separate fix).
Verification
All on the Linux VM (macOS runs were contended; ELF magic asserted on the binary).
RED proven independently per runtime, by disabling only that runtime's peer watch:
connected_clients is still 25connected_clients is still 25connected_clients is still 17connected_clients is still 17In both RED runs the two positive-control tests stayed green — so the failures are the defect, not the harness.
tests/blocking_peer_eof.rs(5 e2e, green on both runtimes):disconnected_single_key_blocked_clients_are_reaped— the A1 regression testdisconnected_multi_key_blocked_clients_are_reaped— 4 shards, exercises the remoteBlockCancelunwind and asserts no ghost waiter swallows a later pushpipelining_behind_a_blocking_command_still_works— the live-client inverse failure modepartial_frame_carried_while_blocked_completes_later— the read-skip guardsilent_blocked_client_stays_blocked— no spin, no self-triggerGates. fmt; clippy
-D warningson default andruntime-tokio,jemalloc; lib tests 4471 monoio / 3633 tokio;blocking_ghost_waiter,blocking_list_timeout(incl.--ignored),parked_idle_parity,idle_downshift_parity,tls_idle_downshift_parityall still green.Notes
break. Separate from A1; flagged for follow-up.tmp/C10K-HARDENING-REVIEW.md, finding A1. Completes Cluster A with fix(blocking): stop cross-shard BLPOP from destroying the next push (c10k A2/A3/A4/A5/A6) #426 (A2–A6).