feat(server): c1M P1 follow-ups — TLS task-parking (47→26 KB), registration handoff, ECANCELED spin fix - #424
Conversation
…rking (c1M P1 follow-ups)
Two follow-ups from the round-5 task-exit-parking review:
1. Registration handoff (closes the review's Low finding, and a worse
bug found while fixing it): waking a parked connection deregistered
then re-registered the client across a task-scheduling boundary.
Beyond the transient CLIENT LIST invisibility / KILL-returns-0
window, the fresh registration silently RESET the connection's
CLIENT SETNAME and age (register() inserts name: None; only the
SETNAME dispatch path ever sets it). The watcher now passes the held
RegistryGuard through spawn_resumed_parked_conn into the handler
(ParkArgs::kept_registration); the handler reuses the live entry via
the new client_registry::live_handle() instead of re-registering.
The entry — name, connected_at, kill state, TOTAL_CLIENTS/shard
counters — persists unbroken across any number of park/wake cycles;
there is no deregistered window at all.
The can_park bool param becomes ParkArgs {can_park,
kept_registration} with NO_PARK/PARK consts at the six monoio call
sites.
2. Migrated-spawn site routes ParkIdle: a connection that migrated
shards (Linux) and then idled past --conn-park-secs now parks like a
primary-accept connection instead of holding its handler task
forever. Mechanical mirror of the reviewed plain-TCP routing (same
watcher, same close-accounting transfer). No dedicated migration
integration harness exists (migration is affinity-driven,
Linux-only); covered by compile gates + the shared watcher paths.
Red/green: new tests/parked_idle_parity.rs test
resumed_connection_keeps_registry_identity (SETNAME → 2× park/data-wake
cycles → name+id survive, CLIENT LIST holds exactly victim+control) was
RED before (name= empty, age=0 after wake), GREEN after.
Gates: parked parity 4/4 on kqueue AND io_uring (VM); TLS parity green;
VM monoio lib 4466 pass; VM tokio lib 3628 pass; fmt; clippy
--all-targets both matrices 0 lints.
refs: PR #422 review Low, .planning/rfcs/c1m-connection-plane.md
author: Tin Dang
…1M P1-TLS) Extends round-5 task-exit parking to TLS connections, closing the last c1M follow-up. No new unsafe; vendored additions are moon-patch style. Vendored (monoio-rustls + monoio-io-wrapper): - Stream::io_ref() — raw-transport reference so the parked watcher can await readable(false) on the underlying fd through the TLS wrapper. - Stream::task_park_safe() — park-safety veto: refuses while the TLS stack holds ANYTHING the raw fd's readability cannot signal (wrapper buffer bytes or pending EOF/error status via the new ReadBuffer/WriteBuffer::is_drained(), decrypted plaintext, a received close_notify, or pending session output). A partial record in the deframer is park-safe: completing it requires more socket bytes. - SafeRead/SafeWrite::is_drained() with moon-patch unit tests (7/7). moon: - IdleParkRead: TLS sets SUPPORTS_TASK_PARK=true; new per-park task_park_safe() hook (plain TCP: always true) appended LAST in the parkable predicate. - conn_accept: ParkWatchable trait (park_readable()) — TcpStream awaits its own readable(false), TLS awaits io_ref().readable(false); watcher + resume helpers genericized over the stream; the monoio-TLS accept site routes ParkIdle like the plain site. Spin bug found by E11 and fixed (affects plain TCP too): - The idle-park arms treated EVERY read error as the sweep cancel. With task parking, a dead connection whose error leaves the fd permanently readable (TLS client FIN without close_notify => read ERROR; plain RST) spun park→wake→park at 100% CPU forever — E11's first ON leg ended with 3000 CLOSE_WAIT conns and a pinned shard. The arms now match monoio's exact cancel error (raw os 125 on BOTH drivers — uring kernel -ECANCELED, legacy hardcodes 125): only the sweep cancel downshifts/parks, real errors tear down promptly (idle_park:: is_sweep_cancel). Small-N repro: fds released, 0 CLOSE_WAIT, 0% CPU. Measured (E11, moon-dev VM, 3000 idle TLS conns, shards=2, same-binary flag A/B, tmp/c10k/e11_tls_park_rss.sh): - TLSPARK_OFF (P4b downshift only): 47.0 KB/conn idle - TLSPARK_ON: 26.0 KB/conn idle (−45%); re-parks at 31.4 after a full wake sweep; sweeps bad=0 both legs; RSS + fds fully return on close - Remaining TLS floor ≈ rustls session (~15-20 KB, not shrinkable via public API) + registry entry + kernel socket Tests: - tls_idle_downshift_parity: +tls_parked_connection_serves_traffic_and_ stays_killable (3 park/wake cycles incl. 100-pipeline on ONE session; CLIENT LIST shows the parked conn WITH its name via registration handoff; CLIENT KILL closes it), +tls_fin_while_parked_tears_down_ promptly (the spin regression, TLS leg) - parked_idle_parity: +rst_while_parked_tears_down_promptly (spin regression, plain-TCP RST leg) - All parity suites green on kqueue AND io_uring (plain 5/5, TLS 3/3) Gates: VM monoio+tokio lib suites pass; E10 plain-park sanity re-run green; fmt; clippy --all-targets both matrices 0 lints; vendor wrapper moon-patch tests 7/7. refs: .planning/rfcs/c1m-connection-plane.md (P1-TLS follow-up) author: Tin Dang
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (4)
📝 WalkthroughWalkthroughIdle task-exit parking now supports TLS and migrated monoio connections, preserves client registry identity across wake cycles, distinguishes cancellation from real I/O errors, and adds readiness checks plus integration coverage for wake, kill, RST, and FIN behavior. ChangesIdle task-exit parking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MonoioHandler
participant ParkedIdleWatcher
participant ParkWatchable
participant ResumedHandler
participant ClientRegistry
MonoioHandler->>ParkedIdleWatcher: return ParkIdle
ParkedIdleWatcher->>ParkWatchable: await park_readable()
ParkWatchable-->>ParkedIdleWatcher: stream readable
ParkedIdleWatcher->>ResumedHandler: resume with RegistryGuard
ResumedHandler->>ClientRegistry: reuse live registration
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/tls_idle_downshift_parity.rs (1)
141-165: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
--conn-park-secshelp text is now stale.
src/config.rslines 307-312 documents the flag as applying to "plain-TCP monoio connections only — TLS and the tokio runtime keep the buffer-downshift behavior". These tests (and the changelog) now rely on TLS parking, so the user-facing help text contradicts actual behavior. Please update that doc comment in the same PR.🤖 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/tls_idle_downshift_parity.rs` around lines 141 - 165, Update the `--conn-park-secs` documentation in the configuration definition in `src/config.rs` to describe its current behavior for TLS connections as well as plain-TCP connections, removing the stale claim that TLS and tokio retain the old behavior. Keep the help text consistent with the TLS parking exercised by `spawn_moon_tls_park`.
🧹 Nitpick comments (4)
tests/parked_idle_parity.rs (2)
203-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpawned
moonchildren are only killed on the success path. All four new tests end withchild.kill(); child.wait();as plain statements, so any earlier assertion failure leaks the server process and its reserved port for the remainder of the test binary. Wrapping the child in aDropguard (astests/txn_kv_wiring.rslines 972-975 does) fixes every site.
tests/parked_idle_parity.rs#L203-L257: hold the child in a kill-on-drop guard inresumed_connection_keeps_registry_identity, and do the same inrst_while_parked_tears_down_promptly.tests/tls_idle_downshift_parity.rs#L244-L352: same guard intls_parked_connection_serves_traffic_and_stays_killableand intls_fin_while_parked_tears_down_promptly.🤖 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/parked_idle_parity.rs` around lines 203 - 257, The four new tests leak spawned moon processes when assertions fail because cleanup only runs on the success path. In tests/parked_idle_parity.rs:203-257, update resumed_connection_keeps_registry_identity and rst_while_parked_tears_down_promptly; in tests/tls_idle_downshift_parity.rs:244-352, update tls_parked_connection_serves_traffic_and_stays_killable and tls_fin_while_parked_tears_down_promptly to store each child in the existing kill-on-drop guard pattern, ensuring cleanup occurs on both success and failure.
302-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPoll instead of a single 2 s sleep.
Teardown timing depends on the shard sweep cadence and CI load; a fixed sleep makes this a latent flake (and a slow test when it passes). Retrying
CLIENT LISTuntilname=rstvictimdisappears, with a deadline, is both faster and stable. Same pattern applies to the TLS FIN test.🤖 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/parked_idle_parity.rs` around lines 302 - 310, Replace the fixed 2-second sleep in the parked-connection teardown test around command_reply and the rstvictim assertion with deadline-based polling of CLIENT LIST, retrying until name=rstvictim disappears and failing when the deadline expires. Apply the same polling pattern to the TLS FIN test, preserving the existing assertion diagnostics and connection cleanup behavior.tests/tls_idle_downshift_parity.rs (2)
339-348: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Ok(n)panic arm can trip on residual TLS records.The read is on the raw socket, so any bytes still in flight from the server side (a TLS alert, or a record the test's
read_exact_deadlineleft buffered in the kernel) fail the test with "expected close" even though the close did happen. Draining untilOk(0)/Errwith a deadline would assert the intended property — that the fd closes promptly — without depending on nothing being pending.🤖 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/tls_idle_downshift_parity.rs` around lines 339 - 348, Update the raw-socket read assertion in the parked TLS victim test to drain repeatedly until it observes EOF or an error, using a bounded deadline or the existing 10-second timeout. Remove the panic on successful nonzero reads so residual TLS records are consumed, while still failing if the socket does not close promptly.
299-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the bulk-reply reader.
This exact read-until-complete-bulk loop is repeated verbatim at lines 398-413, and
tests/parked_idle_parity.rsalready has an equivalentcommand_replyhelper. A single shared helper intests/common/mod.rswould remove three copies.🤖 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/tls_idle_downshift_parity.rs` around lines 299 - 316, Extract the repeated bulk-reply reading loop into a shared helper in tests/common/mod.rs, following the existing command_reply pattern from tests/parked_idle_parity.rs. Update the current reader and the duplicate at the later CLIENT LIST call in tls_idle_downshift_parity.rs to use the helper, preserving the existing completion and error 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/shard/conn_accept.rs`:
- Around line 1122-1127: Update the parked-wake handling around ParkArgs and the
HijackForPsync arm so resumed plain-TCP streams enter the same inline PSYNC flow
used by initial connections instead of logging unsupported and closing. Preserve
the parked registration, and make any TLS limitation explicit while keeping
valid plain replicas connected.
In `@tests/parked_idle_parity.rs`:
- Around line 283-299: Add an accurate // SAFETY: comment immediately above the
libc::setsockopt call within the existing unsafe block, documenting why the raw
file descriptor, linger pointer, and socket option arguments are valid. Keep the
SO_LINGER(0) setup and assertion unchanged.
In `@vendor/monoio-io-wrapper/src/safe_io.rs`:
- Around line 224-234: The library safety predicates SafeRead::is_drained and
SafeWrite::is_drained must not panic when their buffers are absent. Replace the
buffer expect-based access with logic that returns false for a missing buffer
while preserving the existing drained-state checks for present buffers.
---
Outside diff comments:
In `@tests/tls_idle_downshift_parity.rs`:
- Around line 141-165: Update the `--conn-park-secs` documentation in the
configuration definition in `src/config.rs` to describe its current behavior for
TLS connections as well as plain-TCP connections, removing the stale claim that
TLS and tokio retain the old behavior. Keep the help text consistent with the
TLS parking exercised by `spawn_moon_tls_park`.
---
Nitpick comments:
In `@tests/parked_idle_parity.rs`:
- Around line 203-257: The four new tests leak spawned moon processes when
assertions fail because cleanup only runs on the success path. In
tests/parked_idle_parity.rs:203-257, update
resumed_connection_keeps_registry_identity and
rst_while_parked_tears_down_promptly; in
tests/tls_idle_downshift_parity.rs:244-352, update
tls_parked_connection_serves_traffic_and_stays_killable and
tls_fin_while_parked_tears_down_promptly to store each child in the existing
kill-on-drop guard pattern, ensuring cleanup occurs on both success and failure.
- Around line 302-310: Replace the fixed 2-second sleep in the parked-connection
teardown test around command_reply and the rstvictim assertion with
deadline-based polling of CLIENT LIST, retrying until name=rstvictim disappears
and failing when the deadline expires. Apply the same polling pattern to the TLS
FIN test, preserving the existing assertion diagnostics and connection cleanup
behavior.
In `@tests/tls_idle_downshift_parity.rs`:
- Around line 339-348: Update the raw-socket read assertion in the parked TLS
victim test to drain repeatedly until it observes EOF or an error, using a
bounded deadline or the existing 10-second timeout. Remove the panic on
successful nonzero reads so residual TLS records are consumed, while still
failing if the socket does not close promptly.
- Around line 299-316: Extract the repeated bulk-reply reading loop into a
shared helper in tests/common/mod.rs, following the existing command_reply
pattern from tests/parked_idle_parity.rs. Update the current reader and the
duplicate at the later CLIENT LIST call in tls_idle_downshift_parity.rs to use
the helper, preserving the existing completion and error 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: 1c68a072-e5a5-46ba-8c52-c154ca6774b2
📒 Files selected for processing (10)
CHANGELOG.mdsrc/client_registry.rssrc/server/conn/handler_monoio/idle_park.rssrc/server/conn/handler_monoio/mod.rssrc/shard/conn_accept.rstests/parked_idle_parity.rstests/tls_idle_downshift_parity.rsvendor/monoio-io-wrapper/src/lib.rsvendor/monoio-io-wrapper/src/safe_io.rsvendor/monoio-rustls/src/stream.rs
…omment, expect-free is_drained CodeRabbit review of PR #424, all three findings addressed: 1. (Major) PSYNC-after-park closed the replica connection: connections that have issued REPLCONF (replica mid-handshake, PSYNC next) are now permanently excluded from task-parking via a sticky ConnectionState::saw_replconf flag checked in the parkable predicate — the unsupported HijackForPsync warn+close arm on the resumed path is now unreachable for real replicas. (A manual PSYNC with no prior REPLCONF after 60s of idle still lands on the loud warn+close and the replica's reconnect loop recovers — documented.) 2. (Minor) SO_LINGER unsafe block in rst_while_parked test gained its required // SAFETY: comment. 3. (Minor) Vendored SafeRead/SafeWrite::is_drained() are expect-free: a transiently-absent buffer answers false ("don't park" is always the safe verdict) instead of panicking. Gates: parity suites green kqueue (plain 5/5, TLS 3/3); vendor wrapper 7/7; fmt; clippy --all-targets both matrices 0 lints. refs: PR #424 review threads author: Tin Dang
PR Summary by QodoMonoio: TLS task parking, registry handoff on wake, and ECANCELED spin fix
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1.
|
| pub fn task_park_safe(&mut self) -> bool { | ||
| if !self.r_buffer.is_drained() || !self.w_buffer.is_drained() || self.session.wants_write() | ||
| { | ||
| return false; | ||
| } | ||
| match self.session.process_new_packets() { | ||
| Ok(state) => state.plaintext_bytes_to_read() == 0 && !state.peer_has_closed(), | ||
| // Corrupt/unexpected TLS state: don't park; the next read | ||
| // surfaces the error and tears down cleanly. | ||
| Err(_) => false, | ||
| } |
There was a problem hiding this comment.
3. Tls park ignores new writes 🐞 Bug ☼ Reliability
Stream::task_park_safe() checks session.wants_write() only before process_new_packets(), but packet processing can make wants_write() become true; the function can still return true and allow task-exit parking with pending TLS output. Because the parked watcher only awaits raw-fd readability, the connection can stall indefinitely waiting for a write that no task will perform.
Agent Prompt
## Issue description
`vendor/monoio-rustls::Stream::task_park_safe()` can return `true` (allowing task-exit parking) even if `process_new_packets()` causes `self.session.wants_write()` to become `true`. This can leave the TLS connection parked behind a read-readiness watcher while the TLS state machine is waiting to write, stalling the session.
## Issue Context
The new task-exit parking feature relies on raw-fd readability as the only wake signal while parked. Any pending TLS output after deciding to park is unsafe because there is no handler task left to flush it.
## Fix Focus Areas
- vendor/monoio-rustls/src/stream.rs[92-114]
## Suggested change
After calling `process_new_packets()`, include a second `!self.session.wants_write()` check in the success path, e.g.:
- `Ok(state) => state.plaintext_bytes_to_read() == 0 && !state.peer_has_closed() && !self.session.wants_write()`
(Optionally keep the early `wants_write()` check as the fast path.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…ve v0.8.3 hazard) (#425) Patch release rolling up PR #424 (c1M round 6). Ships the fix for a bug LIVE in v0.8.3: task-exit parking defaults on, and the idle-park arms treated every read error as the sweep cancel — a client dying with an RST while parked (crashed client, NAT reset, LB probe) left the fd permanently readable and spun the connection through park→wake→park at 100% shard CPU, socket stuck in CLOSE_WAIT. The arms now match monoio's exact ECANCELED (raw os 125, both drivers); real errors tear down promptly; FIN/RST-while-parked regression-tested. Also: TLS task-exit parking (idle TLS 47.0 → 26.0 KB/conn, −45%, no new unsafe — vendored io_ref() readiness passthrough + task_park_safe() veto); registration handoff across park/wake (no deregistered window; fixes the wake silently resetting CLIENT SETNAME/age); migrated conns park; REPLCONF'd conns excluded from parking (PSYNC-after-park unreachable for real replicas). Validation: parity suites green kqueue + io_uring (plain 5/5, TLS 3/3); VM monoio 4466 + tokio 3628 lib tests; E10 plain-park sanity unchanged (3.28 KB/conn); spin repro post-fix clean. Release gate: crash-matrix nightly + ITERS=20 soak dispatched on the RC (ritual held), green before tag. Rolls CHANGELOG [Unreleased] into [0.8.4], bumps Cargo.toml/lock, adds the RELEASES.md row, updates the README milestone table. author: Tin Dang
Summary
c1M campaign round 6 — the three follow-ups documented in round 5 (PR #422) and the v0.8.3 release, plus a serious latent bug each of them flushed out. Two commits:
1.
fix(server): registration handoff across park/wake + migrated-conn parkingCLIENT SETNAMEandageon every wake (register()insertsname: None; only the SETNAME dispatch path sets it). The watcher now hands itsRegistryGuardthrough to the resumed handler (ParkArgs::kept_registration+ newclient_registry::live_handle()); the entry — name, connected-at, kill state, counters — persists unbroken. Red/green:resumed_connection_keeps_registry_identity(RED before:name=empty,age=0after wake).ParkIdlelike the primary accept site.2.
feat(server): TLS task-exit parking — idle TLS 47.0 → 26.0 KB/conn (−45 %)Stream::io_ref()(raw-transport readiness passthrough) +Stream::task_park_safe()(park veto while ANY TLS-stack state can't be signalled by fd readability: wrapper bytes or pending EOF/error status via newis_drained(), decrypted plaintext, received close_notify, pending output; a partial deframer record is park-safe). Wrapper moon-patch tests 7/7.SUPPORTS_TASK_PARK=true;ParkWatchabletrait lets the watcher awaitreadable(false)on plain TCP orio_ref().readable(false)for TLS; watcher/resume helpers genericized; TLS accept site routesParkIdle.The spin bug (found by E11's first ON leg; affects plain TCP too)
The idle-park arms treated every read error as the sweep's cancel. With task parking, a dead connection whose error leaves the fd permanently readable — a TLS client FIN without close_notify surfaces as a read error (not
Ok(0)), same for a plain-TCP RST — spun park→wake→park at 100 % CPU forever: the first E11 run ended with 3 000 CLOSE_WAIT connections and a pinned shard. The arms now match monoio's exact cancel error (raw_os_error == 125on both drivers: io_uring surfaces kernel-ECANCELED, the legacy driver hardcodes 125): only a sweep cancel downshifts/parks; real errors tear down promptly. Small-N repro post-fix: fds released, 0 CLOSE_WAIT, 0 % CPU. Regression tests:tls_fin_while_parked_tears_down_promptly,rst_while_parked_tears_down_promptly.Measured (E11, moon-dev VM, 3 000 idle TLS conns, shards=2, same-binary flag A/B)
Plain-park E10 sanity re-run: unchanged at 3.28 KB/conn. Remaining TLS floor ≈ rustls session (~15–20 KB, not shrinkable via public API).
Gates
clippy --all-targetsboth matrices 0 lints; vendor wrapper tests 7/7refs:
.planning/rfcs/c1m-connection-plane.md(round-6 section),tmp/C10K-REVIEW.mdround-6 appendix, PR #422 follow-upsSummary by CodeRabbit