perf(server): c10k connection-plane wave — memory ratchet fix, loud maxclients, striped registry, deadline-heap blocking (W1-W8+T3) - #421
Merged
Conversation
…tate module (c10k W4) The pending_wakers waker relay (Rc<RefCell<Vec<Waker>>>) had ZERO registrants anywhere in the tree — the only waker push in the codebase is runtime/cancel.rs on a different structure. It was retired as the reply-wake mechanism when swf0 proved cross-thread oneshot wakes work under monoio's `sync` feature (M2), but the plumbing survived: allocated per shard, cloned per accepted connection (2 spawn paths + 2 migration fail-open paths), threaded through 4 call sites, and drained twice per event-loop iteration for nothing. Also deletes src/server/conn_state.rs (209 lines): a duplicate ConnectionState/ConnectionContext pair marked "Phase 44: Defined only", never adopted, zero references. Found by the 2026-07-29 c10k connection-plane review (tmp/C10K-REVIEW.md); this is workstream W4 of tmp/C10K-PLAN.md. Both runtimes compile clean (cargo check default + runtime-tokio,jemalloc). CLAUDE.md monoio-waker note updated to match. author: Tin Dang
Empirically confirmed (tmp/C10K-REVIEW.md E5): one 1024-deep pipeline permanently ratcheted a connection from 56.5 KB to ~217 KB RSS until disconnect — 5000 such conns pinned 1.09 GB, stable after 35s idle. ~147 KB of that is the `responses`/`frames` Vec<Frame> pair growing to the 1024-frame batch cap (72 B/Frame) and only ever `.clear()`ing. Fixes, monoio handler: - Clear + shrink both batch scratch vecs at END of the batch iteration (before parking in read()), via new `util::shrink_batch_vec` — shrink fires only above 256-frame capacity, so p99 small batches never pay a realloc; a burst-then-idle conn now parks at steady-state capacity. (Shrinking at the top-of-loop clear sites is too late: an idle conn parks in read() before ever reaching them.) - I/O buffer shrink floor lowered 64 KiB → 16 KiB (util:: IO_BUF_SHRINK_TRIGGER): 16–64 KiB high-waters previously ratcheted until disconnect. - Subscriber mode: rent the loop-level tmp_buf via mem::take instead of allocating + zeroing a fresh 8 KiB per select iteration (the `__memset_zva64` flame entry, 2.5% at c10k). The buffer is only lost when the pubsub arm wins (dropped read op owns it — io_uring cancel semantics); command traffic now allocates nothing. Fixes, tokio handler: - Same 16 KiB shrink floor for read/write buffers. - Bump arena capped: reset() retains the largest chunk, so one huge batch pinned its high-water until disconnect; >64 KiB arenas are now rebuilt at 4 KiB. Red/green: 3 new unit tests for shrink_batch_vec thresholds; conn suite 42/42 green; both runtimes compile clean. RSS A/B re-run of E5/E1 gates the branch before merge. Workstream W1 of tmp/C10K-PLAN.md. author: Tin Dang
…0k W2) CrossStoreTxn is ~2.2 KB of inline SmallVec capacity (kv_undo SmallVec<[UndoRecord;16]> alone is 1160 B). Stored unboxed as Option<CrossStoreTxn> in ConnectionState, it sat inside EVERY connection's ~21 KB task future — transacting or not — as part of the measured 56.5 KB/idle-conn footprint (tmp/C10K-REVIEW.md §2). Option<Box<CrossStoreTxn>> moves that to the heap for the rare transacting connection only; TXN.BEGIN (cold path, not in the no-alloc dispatch list) pays one allocation. Call-site churn is minimal: field access auto-derefs, Option<&CrossStoreTxn> consumers switch as_ref() → as_deref(), by-value abort/commit sites deref the box. New regression guard: connection_state_stays_small asserts size_of::<ConnectionState>() ≤ 768 B (was ~2.7 KB). txn (46) + transaction (39) suites green; both runtimes compile clean. Workstream W2 of tmp/C10K-PLAN.md. author: Tin Dang
…heck (c10k W3) Redis parity break found by the 2026-07-29 c10k review: the monoio accept paths rejected over-cap connections with a warn! and a silent close — a client at the cap (default: exactly 10000) saw an unexplained EOF. The tokio plain-TCP path already wrote the error; monoio (default runtime) and both TLS paths did not. - monoio: the maxclients CAS now runs BEFORE the per-connection clone-fest and ConnectionContext construction — a rejected connection previously paid 3 O(num_shards) Vec clones, ~25 Arc refcount bumps and a spawned handler task before the gate ran. On rejection moon now writes `-ERR max number of clients reached\r\n` best-effort, then closes. TLS keeps slot-before-handshake semantics (#17); the in-task gates are removed (slot taken pre-spawn, released at task end as before — rejected conns were never counted, so accounting is unchanged). - tokio TLS: same best-effort error write on rejection (plaintext write surfaces client-side as a loud handshake failure, matching Redis's raw-fd behavior). - startup (unix): getrlimit(RLIMIT_NOFILE) vs maxclients + reserved fds (rlimit_reserved_fds: 64 + 16/shard for listeners/WAL/AOF/spill/uring). Soft limit is raised toward the hard limit when short (logged); if the hard limit still cannot fit, a loud warning states the real client ceiling. Previously moon never looked at RLIMIT_NOFILE and a 10k target on a 1024-fd shell died in silent EMFILE accept-backoff loops. Red/green: new tests/maxclients_reject_parity.rs — rejected_connection_receives_err_max_clients (red before: silent EOF; green after) and slot_frees_on_disconnect (guards the accounting restructure). Unit test for rlimit_reserved_fds. Both runtimes compile. Workstream W3 of tmp/C10K-PLAN.md. author: Tin Dang
… W6) BlockingRegistry::expire_timed_out ran at 100 Hz per shard and walked EVERY queue of EVERY blocked waiter — two full passes (scan + retain) — even when nothing was due: ~2M entry visits/s/shard at 10k blocked BLPOP clients, quadratic when many waiters shared a key (tmp/C10K-REVIEW.md defect #3). There was no deadline-ordered structure at all. Now a min-heap of (deadline, db_index, key) indexes every waiter that HAS a deadline. The sweep pops due entries and scans only the queues they name; block-forever waiters (BLPOP key 0) never enter the heap and are never visited. Stale heap entries (waiter served or cancelled before its deadline) pop to a no-op at their original deadline — bounded by registration rate x timeout, the same envelope as the queues themselves. Steady state with nothing due is a single peek. expire_timed_out now returns the visited-entry count as the test/ observability surface. Red/green: 4 new unit tests — sweep_skips_undeadlined_waiters (100 forever-waiters + 1 due => exactly 1 visit), shared-key partial expiry preserves FIFO, served-waiter stale-entry no-op, multi-key waiter expires from all queues. Full blocking suite 8/8. Workstream W6 of tmp/C10K-PLAN.md. author: Tin Dang
…(c10k W7) drain_spsc_shared always began at consumers[0] with a shared 256-message budget per cycle. Whenever the budget ran out before the tail was reached — sustained cross-shard pressure from a hot peer — low-index peers monopolized every cycle and high-index peers starved indefinitely (R-5 from the 2026-07-06 conn-plane review, re-confirmed by the 2026-07-29 c10k review, tmp/C10K-REVIEW.md defect #4). The drain now starts at a per-thread rotating index (thread-local Cell, same pattern as the existing DRAIN_SCRATCH), so the budget cut-off truncates a DIFFERENT tail each cycle and every peer is head-most once per n cycles. SnapshotBegin early-exit semantics unchanged. The visit order is extracted as rotated_indices(start, n) with unit tests: full coverage per cycle, rotating head, n=0 safety. spsc suite 20/20, shard suite 191/191. Workstream W7 of tmp/C10K-PLAN.md. (The give-up error path was already clean — both slotted dispatch paths return `ERR cross-shard dispatch backpressure` to the client — so W7 is rotation only.) author: Tin Dang
The registry was one process-wide write-preferring RwLock<HashMap>: every accept and every close serialized on it across all shard threads (the only cross-shard serialization point on the connect path), and a CLIENT LIST formatted all N clients under the read lock — blocking every pending register/deregister on all shards for the duration (at 1M conns that is a ~128 MB format under a global lock). CLIENT KILL scanned O(N) even for KILL ID on an id-keyed map. (tmp/C10K-REVIEW.md defect #2.) Now 16 stripes keyed by id % 16: - register/deregister/update/is_killed: single-stripe locks (contention /16, and never blocked by a listing of the other 15 stripes). - CLIENT LIST: walks one stripe at a time, pre-sized from a lock-free total counter. Cross-stripe ordering is not insertion order — Redis makes no CLIENT LIST ordering guarantee. - CLIENT KILL ID: O(1) single-stripe lookup. ADDR/USER filters walk stripes one at a time; a conn registering into an already-visited stripe mid-walk is missed — the same inherent race the single-lock version had at command granularity. - The R-3 fd-shutdown liveness invariant is preserved per stripe: an entry visible under its stripe's READ lock blocks that entry's deregister (same stripe's WRITE lock), so kill_fd is still open. New cross-stripe test (list sees all stripes; KILL ID exactly one victim; KILL USER reaches every stripe). client_registry suite 12/12. Workstream W5 of tmp/C10K-PLAN.md. author: Tin Dang
The AffinityTracker pins ALL central-accepted connections from an IP to the shard where one of its connections migrated (key locality) or first subscribed (pub/sub locality) — with zero load feedback. A saturated shard was never removed from rotation, giving ~2x worst-case connection skew from few client IPs, and FD migration compounded it (tmp/C10K-REVIEW.md defect #5). - client_registry now maintains readable per-shard connection counters (OnceLock<Box<[AtomicUsize]>>, initialized from startup with the resolved shard count; inert when uninitialized — tests/embedded). The metrics gauge could not serve this: gauges are write-only and gated on METRICS_INITIALIZED. - All three listener affinity lookups filter the hint through shard_overloaded(): a shard above 2x the mean conn count (+16 floor, so tiny fleets never flap) falls back to round-robin instead of receiving further funneled connections. Affinity resumes once the shard drains below threshold. - maybe_evict's sort stays as-is — review found it amortized ~60 ops/insert past cap (fires once per 4096 inserts), not a real cost. 4 unit tests on the pure threshold predicate (balanced, at-threshold, funneled, small-fleet floor, zero-shard safety). Workstream W8 of tmp/C10K-PLAN.md. author: Tin Dang
…g (c10k T3) The experimental tokio->io_uring bridge accepts connections that bypass maxclients and the client registry (CLIENT LIST/KILL blind). Full accounting integration is deliberately unscheduled — the bridge is documented as broken under sustained load — but the bypass now announces itself at startup instead of being silent. c1M roadmap (parked-idle conns, task-future diet, uring-entries knob, TLS caps) captured in .planning/rfcs/c1m-connection-plane.md. author: Tin Dang
rlimit math kept in rlim_t domain (kills useless u64::from lints); batch-vec shrink helpers cfg-gated monoio-or-test (tokio-only build saw them as dead code); cargo fmt over the new test/impl code. author: Tin Dang
📝 WalkthroughWalkthroughThe PR updates client admission and registry accounting, deadline-based blocked-client expiration, boxed cross-store transactions, io_uring configuration, connection memory management, Monoio idle handling, vendored TLS I/O, SPSC drain fairness, and removal of pending-waker plumbing. ChangesRuntime connection updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant Listener
participant ClientRegistry
participant Shard
Client->>Listener: connect
Listener->>ClientRegistry: check shard load and capacity
ClientRegistry-->>Listener: accept or reject
Listener->>Shard: route accepted connection
Shard-->>Client: PONG or maxclients error
🚥 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 QodoScale the c10k connection plane and eliminate memory ratchets
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. rlimit SAFETY rationale incomplete
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+627
to
+630
| // SAFETY: getrlimit/setrlimit with a valid resource constant and a | ||
| // pointer to a properly initialized rlimit struct — plain libc FFI | ||
| // with no aliasing or lifetime concerns. | ||
| unsafe { |
There was a problem hiding this comment.
2. rlimit safety rationale incomplete 📘 Rule violation ≡ Correctness
The new unsafe block's SAFETY comment does not state where each FFI precondition is established or why violating it would cause undefined behavior, as required by project policy. The PR summary also omits the policy-required Unsafe added: N blocks disclosure.
Agent Prompt
## Issue description
The new libc FFI block does not include the complete SAFETY rationale required by `UNSAFE_POLICY.md`, and the PR summary does not disclose the added unsafe block.
## Issue Context
Document the exact preconditions for both `getrlimit` and `setrlimit`, identify how the local values and libc constants establish them, and explain the undefined behavior risk. Also add the required `Unsafe added: 1 blocks` summary entry with a one-line justification.
## Fix Focus Areas
- src/main.rs[627-642]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+188
to
+192
| let std::cmp::Reverse((_, db_index, key)) = self.deadlines.pop().unwrap(); | ||
| let queue_key = (db_index, key); | ||
| // A missing queue is a STALE heap entry (waiter served/cancelled | ||
| // before its deadline) — the lazy-invalidation no-op. | ||
| let Some(queue) = self.waiters.get_mut(&queue_key) else { |
There was a problem hiding this comment.
3. Shared-key expiry becomes quadratic 🐞 Bug ➹ Performance
Each due heap item identifies only a queue, so expire_timed_out rescans every remaining waiter for that key on successive staggered expirations. A hot key with many blocked clients can therefore accumulate O(n²) timer work and stall its shard despite the intended O(due) behavior.
Agent Prompt
## Issue description
Deadline heap entries identify only a queue, causing repeated full-queue scans as staggered waiters expire on the same key. Index each deadline by waiter identity or another direct-removal mechanism so expiration work scales with due waiters rather than queue length.
## Issue Context
Preserve FIFO ordering for non-expired waiters and lazy cancellation safety. Add a shared-key stress test that checks visit/removal work across staggered deadlines.
## Fix Focus Areas
- src/blocking/mod.rs[69-79]
- src/blocking/mod.rs[109-129]
- src/blocking/mod.rs[169-225]
- src/blocking/mod.rs[385-402]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+115
to
+117
| if let Some(deadline) = entry.deadline { | ||
| self.deadlines | ||
| .push(std::cmp::Reverse((deadline, db_index, key))); |
There was a problem hiding this comment.
4. Served deadlines retain memory 🐞 Bug ➹ Performance
Serving or cancelling a waiter removes it from the waiter maps but leaves its heap entry and cloned key until the original deadline. Promptly served requests with long timeouts therefore retain memory at registration-rate × timeout even when no clients remain blocked.
Agent Prompt
## Issue description
Timed waiters leave stale heap entries after being served or cancelled, retaining tuples and key storage until their original deadlines. Add eager removal, compact validity indexing, or bounded heap rebuilding so stale-entry memory tracks active waiters rather than the timeout horizon.
## Issue Context
The design must preserve efficient deadline lookup and safe cancellation. Add a test that repeatedly registers and serves long-timeout waiters and verifies stale storage is reclaimed or bounded.
## Fix Focus Areas
- src/blocking/mod.rs[69-79]
- src/blocking/mod.rs[109-159]
- src/blocking/mod.rs[182-193]
- src/blocking/wakeup.rs[19-102]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+642
to
+651
| if libc::setrlimit(libc::RLIMIT_NOFILE, &raised) == 0 { | ||
| tracing::info!( | ||
| "Raised RLIMIT_NOFILE soft limit {} -> {} to fit maxclients {} (+{} reserved fds)", | ||
| rl.rlim_cur, | ||
| want, | ||
| config.maxclients, | ||
| reserved | ||
| ); | ||
| } | ||
| if want < need { |
There was a problem hiding this comment.
5. Rlimit failures stay silent 🐞 Bug ◔ Observability
The startup check ignores failures from both getrlimit and setrlimit; when want == need but raising the soft limit fails, the hard-limit warning is also skipped. The server then starts with an insufficient effective descriptor ceiling and can still encounter unexplained EMFILE failures under load.
Agent Prompt
## Issue description
The new startup RLIMIT check silently ignores syscall failures and may report neither a successful raise nor an insufficient effective ceiling. Handle both failure paths and report the actual usable client ceiling.
## Issue Context
On `setrlimit` failure, retain the original soft limit when calculating the effective ceiling. Choose an explicit policy such as warning, reducing the effective limit, or failing startup, and include the OS error in diagnostics.
## Fix Focus Areas
- src/main.rs[620-665]
- src/main.rs[2096-2103]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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
🤖 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/blocking/mod.rs`:
- Around line 182-210: Update the deadline sweep around the `self.deadlines`
loop to collect due `(db_index, key)` pairs in a `HashSet`, while still draining
due heap entries and preserving timeout IDs. After collection, scan each
corresponding `self.waiters` queue once so later or block-forever waiters are
not repeatedly rescanned; retain stale-entry handling and queue removal
behavior. Add a regression test covering mixed deadlines for multiple waiters
sharing one key.
In `@src/main.rs`:
- Around line 638-663: Update the RLIMIT_NOFILE handling around the setrlimit
call to explicitly detect a nonzero return and emit a warning containing the
failure details and requested limit, including when want >= need. Preserve the
existing success log and hard-limit warning behavior, while ensuring every
failed setrlimit attempt is surfaced to operators.
🪄 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: 6dc668ff-3ea8-44fd-adcf-fef104ae43eb
📒 Files selected for processing (22)
CHANGELOG.mdCLAUDE.mdsrc/blocking/mod.rssrc/client_registry.rssrc/main.rssrc/server/conn/core.rssrc/server/conn/handler_monoio/ft.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_monoio/txn.rssrc/server/conn/handler_sharded/ft.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/handler_sharded/txn.rssrc/server/conn/tests.rssrc/server/conn/util.rssrc/server/conn_state.rssrc/server/listener.rssrc/server/mod.rssrc/shard/conn_accept.rssrc/shard/event_loop.rssrc/shard/spsc_handler.rssrc/shard/timers.rstests/maxclients_reject_parity.rs
💤 Files with no reviewable changes (2)
- src/server/conn_state.rs
- src/server/mod.rs
Comment on lines
+182
to
+210
| while self | ||
| .deadlines | ||
| .peek() | ||
| .is_some_and(|std::cmp::Reverse((d, _, _))| *d <= now) | ||
| { | ||
| #[allow(clippy::unwrap_used)] // peek above just proved non-empty | ||
| let std::cmp::Reverse((_, db_index, key)) = self.deadlines.pop().unwrap(); | ||
| let queue_key = (db_index, key); | ||
| // A missing queue is a STALE heap entry (waiter served/cancelled | ||
| // before its deadline) — the lazy-invalidation no-op. | ||
| let Some(queue) = self.waiters.get_mut(&queue_key) else { | ||
| continue; | ||
| }; | ||
| let mut i = 0; | ||
| while i < queue.len() { | ||
| visited += 1; | ||
| let is_expired = queue[i].deadline.map_or(false, |d| d <= now); | ||
| if is_expired { | ||
| #[allow(clippy::unwrap_used)] // i < queue.len() by loop guard | ||
| let entry = queue.remove(i).unwrap(); | ||
| timed_out_ids.push(entry.wait_id); | ||
| timed_out.push((entry.wait_id, entry.reply_tx)); | ||
| } else { | ||
| i += 1; | ||
| } | ||
| } | ||
| if queue.is_empty() { | ||
| self.waiters.remove(&queue_key); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Deduplicate due queue keys before scanning.
The heap has one entry per waiter, not per queue. If many waiters on one key time out while later/block-forever waiters remain, every due heap entry rescans those survivors. This can turn a sweep into O(due entries × remaining queue length), stalling the 100 Hz shard loop. Collect due (db_index, key) values in a HashSet and scan each queue once; add a mixed-deadline same-key regression test. This also makes the O(due + stale) claim in src/shard/timers.rs accurate.
🤖 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/blocking/mod.rs` around lines 182 - 210, Update the deadline sweep around
the `self.deadlines` loop to collect due `(db_index, key)` pairs in a `HashSet`,
while still draining due heap entries and preserving timeout IDs. After
collection, scan each corresponding `self.waiters` queue once so later or
block-forever waiters are not repeatedly rescanned; retain stale-entry handling
and queue removal behavior. Add a regression test covering mixed deadlines for
multiple waiters sharing one key.
Comment on lines
+638
to
+663
| if config.maxclients > 0 && rl.rlim_cur < need { | ||
| let want = need.min(rl.rlim_max); | ||
| let mut raised = rl; | ||
| raised.rlim_cur = want; | ||
| if libc::setrlimit(libc::RLIMIT_NOFILE, &raised) == 0 { | ||
| tracing::info!( | ||
| "Raised RLIMIT_NOFILE soft limit {} -> {} to fit maxclients {} (+{} reserved fds)", | ||
| rl.rlim_cur, | ||
| want, | ||
| config.maxclients, | ||
| reserved | ||
| ); | ||
| } | ||
| if want < need { | ||
| tracing::warn!( | ||
| "RLIMIT_NOFILE hard limit {} cannot fit maxclients {} (+{} reserved fds): \ | ||
| accepts beyond ~{} clients will fail with EMFILE. Raise `ulimit -n` \ | ||
| or lower --maxclients.", | ||
| rl.rlim_max, | ||
| config.maxclients, | ||
| reserved, | ||
| want.saturating_sub(reserved) | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Silent no-op when setrlimit itself fails.
If setrlimit fails but want >= need (i.e., the raise should have succeeded within the hard limit — e.g. blocked by a seccomp/LSM policy), neither the info! nor the warn! fires. The operator gets zero indication that the fd-limit safety net silently didn't take effect, and later hits opaque EMFILE accept failures.
🔧 Proposed fix to surface a setrlimit failure
let mut raised = rl;
raised.rlim_cur = want;
- if libc::setrlimit(libc::RLIMIT_NOFILE, &raised) == 0 {
+ if libc::setrlimit(libc::RLIMIT_NOFILE, &raised) == 0 {
tracing::info!(
"Raised RLIMIT_NOFILE soft limit {} -> {} to fit maxclients {} (+{} reserved fds)",
rl.rlim_cur,
want,
config.maxclients,
reserved
);
+ } else {
+ tracing::warn!(
+ "setrlimit(RLIMIT_NOFILE, {}) failed: {}; maxclients {} may hit EMFILE under load",
+ want,
+ std::io::Error::last_os_error(),
+ config.maxclients
+ );
}📝 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.
Suggested change
| if config.maxclients > 0 && rl.rlim_cur < need { | |
| let want = need.min(rl.rlim_max); | |
| let mut raised = rl; | |
| raised.rlim_cur = want; | |
| if libc::setrlimit(libc::RLIMIT_NOFILE, &raised) == 0 { | |
| tracing::info!( | |
| "Raised RLIMIT_NOFILE soft limit {} -> {} to fit maxclients {} (+{} reserved fds)", | |
| rl.rlim_cur, | |
| want, | |
| config.maxclients, | |
| reserved | |
| ); | |
| } | |
| if want < need { | |
| tracing::warn!( | |
| "RLIMIT_NOFILE hard limit {} cannot fit maxclients {} (+{} reserved fds): \ | |
| accepts beyond ~{} clients will fail with EMFILE. Raise `ulimit -n` \ | |
| or lower --maxclients.", | |
| rl.rlim_max, | |
| config.maxclients, | |
| reserved, | |
| want.saturating_sub(reserved) | |
| ); | |
| } | |
| } | |
| } | |
| if config.maxclients > 0 && rl.rlim_cur < need { | |
| let want = need.min(rl.rlim_max); | |
| let mut raised = rl; | |
| raised.rlim_cur = want; | |
| if libc::setrlimit(libc::RLIMIT_NOFILE, &raised) == 0 { | |
| tracing::info!( | |
| "Raised RLIMIT_NOFILE soft limit {} -> {} to fit maxclients {} (+{} reserved fds)", | |
| rl.rlim_cur, | |
| want, | |
| config.maxclients, | |
| reserved | |
| ); | |
| } else { | |
| tracing::warn!( | |
| "setrlimit(RLIMIT_NOFILE, {}) failed: {}; maxclients {} may hit EMFILE under load", | |
| want, | |
| std::io::Error::last_os_error(), | |
| config.maxclients | |
| ); | |
| } | |
| if want < need { | |
| tracing::warn!( | |
| "RLIMIT_NOFILE hard limit {} cannot fit maxclients {} (+{} reserved fds): \ | |
| accepts beyond ~{} clients will fail with EMFILE. Raise `ulimit -n` \ | |
| or lower --maxclients.", | |
| rl.rlim_max, | |
| config.maxclients, | |
| reserved, | |
| want.saturating_sub(reserved) | |
| ); | |
| } | |
| } | |
| } |
🤖 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/main.rs` around lines 638 - 663, Update the RLIMIT_NOFILE handling around
the setrlimit call to explicitly detect a nonzero return and emit a warning
containing the failure details and requested limit, including when want >= need.
Preserve the existing success log and hard-limit warning behavior, while
ensuring every failed setrlimit attempt is surfaced to operators.
Expose monoio's already-public RuntimeBuilder::with_entries through moon config: `--uring-entries N` sets the io_uring submission-queue size per shard ring (monoio default 1024; CQ = 2x by kernel), and sizes the legacy epoll/kqueue driver's mio event batch on that path. 1024 in-flight ops per shard is small for 10k+ conns/shard under bursty pipelines (.planning/rfcs/c1m-connection-plane.md Proposal 4). Plumbing mirrors the --io-driver epoll precedent: a process-global AtomicU32 in runtime/mod.rs set once in main BEFORE any shard thread spawns, consumed by all three monoio build paths (legacy, tuned io_uring, fusion fallback). Values below monoio's 256 floor clamp up with a warning; unset keeps monoio's default; no effect under the tokio runtime (loud warn). Zero vendored-monoio changes needed — with_entries() was already public and forwarded on every builder path. Tests: config parse (default None / value / reject non-numeric) + runtime static default/set/clamp. author: Tin Dang
…re (c10k P3) -Zprint-type-sizes on the 58-variant connection state machine showed its size is set entirely by two COLD suspend points: awaiting try_handle_txn_abort (5568 B awaitee -> 8319 B variant) and the disconnect-teardown abort_cross_store_txn_routed (5376 B -> 8127 B), with try_handle_ft_command third (2816 B -> 5567 B). Box::pin those three behind their existing name-check fast paths, in both the monoio and sharded handlers: - TXN.ABORT rollback + leaked-txn disconnect teardown: alloc only when an abort actually executes - FT.* body split into ft_command_inner and boxed after the "FT." prefix check: alloc only per executed FT command (noise next to the search itself), never on the per-command filter chain Measured (nightly -Zprint-type-sizes A/B, macOS aarch64 release-fast): per-conn monoio task allocation 9280 -> 5952 B plain TCP (-36%), 13248 -> 9920 B TLS, 9472 -> 6144 B migrated; handler future 8640 -> 5312 B. Meets the RFC Proposal 3 target (<=8 KB) without touching the hot KV dispatch path (the next-largest variant is the hot cross-shard dispatch await, deliberately left inline). author: Tin Dang
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/config.rs`:
- Around line 298-305: Add an explicit upper bound or memory-budget validation
for Config::uring_entries alongside the existing MIN_URING_ENTRIES clamp,
document the accepted maximum in the CLI help, and reject oversized values
during startup configuration validation. Ensure the validated value is used by
both Monoio and legacy driver setup so neither io_uring::Builder::build nor
mio::Events allocation can receive an unsafe size, and add coverage for
oversized values.
In `@src/runtime/mod.rs`:
- Around line 390-407: Update uring_entries_default_set_and_clamp to serialize
access to the process-global URING_ENTRIES state and restore it to 0 after
assertions using a test-only guard. Ensure cleanup runs even when an assertion
fails, while preserving the existing value and clamping checks.
- Around line 48-73: Align the zero-value behavior between the `URING_ENTRIES`
storage, `set_uring_entries`, and CLI/help/log handling: either reject/document
zero as invalid floor input, or preserve the documented unset contract by
skipping storage for zero. Update the boundary test to cover `--uring-entries 0`
and ensure it verifies the chosen behavior consistently.
🪄 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: 087e6634-1cda-40a3-9523-70563250d329
📒 Files selected for processing (11)
CHANGELOG.mdsrc/config.rssrc/main.rssrc/runtime/mod.rssrc/runtime/monoio_impl.rssrc/server/conn/handler_monoio/ft.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_monoio/txn.rssrc/server/conn/handler_sharded/ft.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/handler_sharded/txn.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/server/conn/handler_sharded/txn.rs
- src/server/conn/handler_sharded/mod.rs
- src/server/conn/handler_sharded/ft.rs
- src/server/conn/handler_monoio/mod.rs
- CHANGELOG.md
- src/server/conn/handler_monoio/txn.rs
- src/main.rs
Comment on lines
+298
to
+305
| /// io_uring submission-queue entries per shard ring (default: monoio's | ||
| /// 1024; CQ is sized 2x by the kernel). Raise for high-connection shards | ||
| /// under bursty pipelines — 1024 in-flight ops per shard is small at 10k+ | ||
| /// conns/shard. Values below 256 are raised to 256 (monoio's floor). | ||
| /// Also sizes the legacy (epoll/kqueue) driver's event batch. Applies to | ||
| /// the monoio runtime only. | ||
| #[arg(long = "uring-entries")] | ||
| pub uring_entries: Option<u32>, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)src/config\.rs$|monoio_impl\.rs$|Cargo\.toml$|Cargo\.lock$' || true
echo "== config relevant lines =="
if [ -f src/config.rs ]; then
nl -ba src/config.rs | sed -n '260,330p'
fi
echo "== monoio_impl relevant lines =="
if [ -f src/runtime/monoio_impl.rs ]; then
nl -ba src/runtime/monoio_impl.rs | sed -n '120,175p'
fi
echo "== uring_entries search =="
rg -n "uring_entries|set_uring_entries|io_uring|mio::Events|Builder::build|build\(entries\)" src . --glob '*.rs' --glob 'Cargo.toml' --glob 'Cargo.lock' || true
echo "== Cargo monoio refs =="
rg -n 'name = "monoio"|version = ' Cargo.toml Cargo.lock | sed -n '1,140p'Repository: pilotspace/moon
Length of output: 393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== config relevant lines =="
awk '{printf "%6d: %s\n", NR, $0}' src/config.rs | sed -n '260,330p'
echo "== monoio_impl relevant lines =="
awk '{printf "%6d: %s\n", NR, $0}' src/runtime/monoio_impl.rs | sed -n '120,175p'
echo "== uring_entries search =="
rg -n "uring_entries|set_uring_entries|RUNTIME|io_uring|mio::Events|Builder::build|build\\(entries\\)" src Cargo.toml Cargo.lock fuzz Cargo.lock fuzz/Cargo.toml vendor/monoio 2>/dev/null || true
echo "== Cargo monoio refs =="
rg -n 'monoio|version = ' Cargo.toml Cargo.lock vendor/monoio/Cargo.toml 2>/dev/null | sed -n '1,200p'Repository: pilotspace/moon
Length of output: 40401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/runtime/mod.rs uring_entries definitions =="
awk '{printf "%6d: %s\n", NR, $0}' src/runtime/mod.rs | sed -n '40,75p'
awk '{printf "%6d: %s\n", NR, $0}' src/runtime/mod.rs | sed -n '393,410p'
echo "== src/main.rs uring_entries callsite =="
awk '{printf "%6d: %s\n", NR, $0}' src/main.rs | sed -n '1068,1085p'
rg -n "configure_shards_from_config|async_shard_runner|spawn.*monoio|start.*monoio|set_uring_entries" src/main.rs src/runtime/mod.rs src | sed -n '1,240p'
echo "== monoio builder implementation =="
awk '{printf "%6d: %s\n", NR, $0}' vendor/monoio/src/builder.rs | sed -n '120,165p'
awk '{printf "%6d: %s\n", NR, $0}' vendor/monoio/src/runtime.rs | sed -n '190,235p'
awk '{printf "%6d: %s\n", NR, $0}' vendor/monoio/src/driver/uring/mod.rs | sed -n '96,145p'
awk '{printf "%6d: %s\n", NR, $0}' vendor/monoio/src/driver/poll.rs | sed -n '12,24p'
awk '{printf "%6d: %s\n", NR, $0}' vendor/monoio/src/driver/legacy/mod.rs | sed -n '215,235p'
echo "== all tests touching uring_entries in src =="
rg -n "uring_entries|with_entries|io_uring entries|entries" src --glob '*test*' --glob '*.rs' | sed -n '1,200p'Repository: pilotspace/moon
Length of output: 28859
🌐 Web query:
io-uring 0.6 Builder build entries parameter allocation Mio Events capacity
💡 Result:
In the io-uring Rust crate (version 0.6 and later), the Builder::build method is used to construct an IoUring instance with a specified number of entries [1]. When you call Builder::build(entries: u32), the entries parameter sets the size of the submission queue (SQ) and the completion queue (CQ) [1]. Unless you have previously called setup_cqsize(entries: u32) to explicitly configure the completion queue size, the entries value passed to build will be used for both [1]. The number of entries must be a power of two, and the value may be rounded up by the kernel if it is not already [1]. Regarding Mio events and capacity, the io-uring crate itself is a low-level interface to the Linux io_uring syscalls [2][3]. It does not directly manage Mio events or their capacity. Integration with event loops like Mio or the Tokio runtime occurs at a higher level [4][5]. In such integrations (like Tokio's io_uring driver), the io_uring file descriptor is registered with the Mio poll/event system to allow the runtime to wait for readiness notifications [4][5]. The capacity of the io_uring itself—the number of SQ and CQ entries—is determined during the initial construction of the IoUring instance via the Builder or IoUring::new methods, not by Mio's event capacity [1][6].
Citations:
- 1: https://docs.rs/io-uring/latest/io_uring/struct.Builder.html
- 2: https://docs.rs/crate/io-uring/0.6.4
- 3: https://docs.rs/io-uring/latest/io_uring/index.html
- 4: https://github.com/tokio-rs/tokio/blob/c637f6e7/tokio/src/runtime/io/driver/uring.rs
- 5: tokio: add io_uring support tokio-rs/tokio#4952
- 6: https://docs.rs/io-uring/latest/io_uring/struct.IoUring.html
Document and enforce an upper bound for --uring-entries.
This flag only clamps small values to MIN_URING_ENTRIES, so the process-wide setting can be u32::MAX and is passed to every Monoio runtime builder. That reaches io_uring::Builder::build(entries) and sizes the legacy driver’s mio::Events capacity via entries as usize, which can fail startup with a per-shard resource/allocation problem. Add a documented maximum or memory-budget validation, return a startup error instead of panicking on the fallback path, and cover oversized values.
🤖 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/config.rs` around lines 298 - 305, Add an explicit upper bound or
memory-budget validation for Config::uring_entries alongside the existing
MIN_URING_ENTRIES clamp, document the accepted maximum in the CLI help, and
reject oversized values during startup configuration validation. Ensure the
validated value is used by both Monoio and legacy driver setup so neither
io_uring::Builder::build nor mio::Events allocation can receive an unsafe size,
and add coverage for oversized values.
Comment on lines
+48
to
+73
| /// Process-wide io_uring SQ-entries override (`--uring-entries N`), set ONCE | ||
| /// from main BEFORE any shard thread spawns — same contract as | ||
| /// [`force_legacy_driver`]. 0 = unset (monoio's default 1024). Also sizes the | ||
| /// legacy driver's mio event batch. | ||
| static URING_ENTRIES: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0); | ||
|
|
||
| /// monoio's `RuntimeBuilder::with_entries` floor; values below are raised so | ||
| /// the operator-visible behavior matches what the ring actually gets. | ||
| pub const MIN_URING_ENTRIES: u32 = 256; | ||
|
|
||
| /// Set the per-shard ring size. Values below [`MIN_URING_ENTRIES`] are clamped | ||
| /// up (mirrors monoio's internal floor). Call before shard threads spawn. | ||
| pub fn set_uring_entries(entries: u32) { | ||
| URING_ENTRIES.store( | ||
| entries.max(MIN_URING_ENTRIES), | ||
| std::sync::atomic::Ordering::Release, | ||
| ); | ||
| } | ||
|
|
||
| /// The configured ring size, or `None` when the operator left the default. | ||
| pub fn uring_entries() -> Option<u32> { | ||
| match URING_ENTRIES.load(std::sync::atomic::Ordering::Acquire) { | ||
| 0 => None, | ||
| n => Some(n), | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)src/(runtime/mod|main)\.rs$|Cargo\.toml$' || true
echo "== runtime mod outline =="
ast-grep outline src/runtime/mod.rs --view compact || true
echo "== relevant src/runtime/mod.rs =="
sed -n '1,120p' src/runtime/mod.rs
echo "== main usages =="
rg -n "set_uring_entries|uring_entries|uring-entries|MIN_URING_ENTRIES|ring size|entries" src/main.rs src/runtime/mod.rs tests 2>/dev/null || true
echo "== target files if tests exist =="
fd -i '.*tests?.*' . -t f | sed -n '1,80p'Repository: pilotspace/moon
Length of output: 21273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/main.rs relevant sections =="
sed -n '1030,1105p' src/main.rs
echo "== config definitions/options around ureg_uring_entries =="
rg -n "uring_entries|--uring-entries|ring size|floor|clap|Clap|Arg" src/main.rs src -g '*.rs' | sed -n '1,120p'
echo "== runtime tests =="
sed -n '360,415p' src/runtime/mod.rs
echo "== read-only behavioral semantic verifier =="
python3 - <<'PY'
from dataclasses import dataclass
MIN = 256
class UringEntries:
def __init__(self):
self.value = 0
def set_uring_entries(self, entries):
self.value = entries if entries >= MIN else MIN
def uring_entries(self):
return None if self.value == 0 else self.value
c = UringEntries()
for input_val in [0, 1, 255, 256]:
c.value = 0
c.set_uring_entries(input_val)
print({"input": input_val, "stored": c.value, "read": c.uring_entries(), "log_effective": f"{input_val} is {'below floor' if input_val < MIN else 'at/above floor'}", "log_prints_warning": "yes" if input_val < MIN else "no"})
PYRepository: pilotspace/moon
Length of output: 17023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/config.rs uring_entries definitions =="
sed -n '280,330p' src/config.rs
echo "== src/config.rs test around uring_entries_flag =="
sed -n '2000,2035p' src/config.rs
echo "== zero-range/value_parser occurrences involving uring_entries =="
rg -n "uring-entries|uring_entries|value_parser.*u32|0|default_value_t" src/config.rs src/main.rs -C 3
echo "== behavioral parser/zero contract probe =="
python3 - <<'PY'
from pathlib import Path
src = Path("src/config.rs").read_text()
idx = src.index("pub struct ServerConfig")
try:
idx2 = src.index("pub struct ServerConfig", idx + 1)
except ValueError:
idx2 = src.index("pub mod", idx)
struct = src[idx:idx2]
for needle in ["#[arg(", "uring_entries: Option", "value_parser", "0..", "1.."]:
print(f"{needle!r}: {needle in struct}")
print("uring_entries_help_lines:")
for i,line in enumerate(src.splitlines(),1):
if "--uring-entries" in line or "uring_entries: Option<u32>" in line:
print(f"{i}: {line}")
print("tests:")
for i,line inenumerate(src.splitlines(),1):
if "--uring-entries" in line or "uring_entries" in line:
print(f"{i}: {line}")
PYRepository: pilotspace/moon
Length of output: 50372
Align the zero-value contract.
--uring-entries 0 is parsed as Some(0) and set_uring_entries(0) stores MIN_URING_ENTRIES, so the help/log path treats it as a sub-floor value instead of “unset”. Reject 0/document it as the floor input, or keep “unset” semantics by treating 0 as skipped input and make the boundary test cover --uring-entries 0.
🤖 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/runtime/mod.rs` around lines 48 - 73, Align the zero-value behavior
between the `URING_ENTRIES` storage, `set_uring_entries`, and CLI/help/log
handling: either reject/document zero as invalid floor input, or preserve the
documented unset contract by skipping storage for zero. Update the boundary test
to cover `--uring-entries 0` and ensure it verifies the chosen behavior
consistently.
Comment on lines
+390
to
+407
| #[cfg(test)] | ||
| mod uring_entries_tests { | ||
| // Shared process-global: run assertions in one test to avoid ordering | ||
| // races between parallel test threads. | ||
| #[test] | ||
| fn uring_entries_default_set_and_clamp() { | ||
| assert_eq!(super::uring_entries(), None, "unset must read as None"); | ||
| super::set_uring_entries(4096); | ||
| assert_eq!(super::uring_entries(), Some(4096)); | ||
| super::set_uring_entries(64); | ||
| assert_eq!( | ||
| super::uring_entries(), | ||
| Some(super::MIN_URING_ENTRIES), | ||
| "sub-floor values clamp up to monoio's minimum" | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the process-global test state.
This test leaves URING_ENTRIES set to 256. Any sibling test that checks the unset state or constructs a runtime afterward can observe the mutated value; keeping the assertions in one test does not isolate the static from the rest of the test harness. Serialize access and restore the value to 0 with a test-only guard.
🤖 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/runtime/mod.rs` around lines 390 - 407, Update
uring_entries_default_set_and_clamp to serialize access to the process-global
URING_ENTRIES state and restore it to 0 after assertions using a test-only
guard. Ensure cleanup runs even when an assertion fails, while preserving the
existing value and clamping checks.
…rking set (c10k W11) A connection parked in read() pinned its full working set forever: an 8 KiB rent buffer submitted to the kernel plus the empty 8 KiB read/write scratch — ~24 KiB of the measured 43.5 KB idle RSS/conn, and the floor the c1M RFC said only connection parking could reclaim. Two-stage park, no timers or allocs on the hot path: - Stage 1 (full buffers): cancel-capable streams read via monoio's cancelable_read, with the park timestamp (shard cached clock) in a shard-thread-local IdleSlot registry. One Rc alloc per CONNECTION at setup; per park just two Cell stores + a CancelHandle Rc clone. - The shard event loop's existing 1s chore sweeps the registry and fires the Canceller of any read parked >=1s. Cancel-and-await is loss-free on both drivers (io_uring AsyncCancel; legacy READ_CANCELED): a completion that raced the cancel still delivers its bytes. - Stage 2: the woken handler drops the rent buffer, releases the empty scratch buffers, and re-parks on a 512 B probe buffer with no further chore involvement — one cancellation per idle period, not per second. - Real data restores the full working set lazily (pre-park sizing) and re-arms stage 1. Bursts >512 B spill into the next full-size read. TLS streams opt out via IdleParkRead::SUPPORTS_IDLE_PARK (monoio_rustls has no cancelable read) and keep the pre-W11 path byte-for-byte; the tokio runtime is untouched. Measured (moon-dev VM, 2 shards, 10k idle conns): 43.5 -> 19.9 KB/conn after 30s idle (-54%; campaign total 56.5 -> 19.9, -65%); post-wake steady state 27.8 KB/conn (lazy regrowth also sheds the old always-8KiB read/write scratch); PING sweep over all 10k downshifted conns 0.07s, 0 failures; RSS returns on close. Parity suite (probe-size wake, >512B pipeline, 4 KiB value round trip, undisturbed active sibling) green on BOTH cancel paths: macOS kqueue legacy driver and Linux io_uring. author: Tin Dang
author: Tin Dang
… red tokio Check Round-2 fallout (--uring-entries, fdcd5d9): three tokio-only integration suites build ServerConfig struct literals and were never rebuilt by the lib-suite gates, so the missing `uring_entries` field only surfaced in CI's full-matrix Check job (E0063 in mq_integration, txn_kv_wiring, workspace_integration ×2). Adds `uring_entries: None` to all four literals. Also moves the round-2 `uring_entries_tests` module to the end of src/runtime/mod.rs (clippy `items_after_test_module`, visible only under --all-targets). Verified: all three suites compile and pass under --no-default-features --features runtime-tokio,jemalloc (VM; txn_kv_wiring's crash-recovery leg needs disk headroom — the macOS host root volume sits under the 5% diskfull floor). Lesson for the gate checklist: a config-struct field addition must be followed by `cargo clippy --all-targets` on BOTH matrices — lib suites do not compile integration tests. author: Tin Dang
…d 40 KB (round 4) Round 4 of the c10k campaign, per the c1M RFC sequencing: the TLS diet. Root cause (round 3): monoio-io-wrapper eagerly allocates two fixed 16 KiB Box<[u8]> buffers per TLS stream and never frees them, and monoio-rustls has no cancelable read, so W11's idle downshift could not cover TLS connections at all — idle TLS RSS sat flat at ~87.5 KB/conn. Changes (vendored crates follow the existing vendor/monoio "moon patch" comment style; wired via [patch.crates-io]): - vendor/monoio-io-wrapper: Buffer is lazy (allocated on first use) and releasable (release_if_empty, only while drained; reallocates transparently on next use). New do_io_cancelable whose errors — including ECANCELED — are returned directly and NOT stashed for status replay: a stashed cancel error would resurface on the next plain read and tear down a healthy connection (unit-tested in-crate, 5 tests). - vendor/monoio-rustls: Stream::release_idle_buffers() and a CancelableAsyncReadRent impl that forwards the cancel handle into the wrapper's socket read. Loss-free under cancel-and-await: bytes from a completion racing the cancel persist in the wrapper buffer + rustls deframer and are consumed by the next read. - src/server/conn/handler_monoio/idle_park.rs: TLS streams now set SUPPORTS_IDLE_PARK = true and implement the new on_idle_downshift hook (releases both wrapper buffers). Lazy-alloc alone would be worthless — the handshake itself fills both buffers, so the win requires the idle-park cancel to fire the release. - tests/tls_idle_downshift_parity.rs: wire parity on one continuous TLS session across three downshift/wake cycles (probe wake, 700 B pipeline, 4 KiB value round trip), rustls client with throwaway cert; green on kqueue (macOS) and io_uring (VM). E9 A/B (moon-dev VM, 3000 idle TLS conns, shards=2): pre-P4b 87.5 KB/conn flat forever → P4b 47.4 KB/conn after 30 s idle (−46%), re-downshifts to 48.7 after a wake-sweep; sweeps bad=0; RSS returns on close. Matches the RFC's −30–40 KB estimate. Remaining TLS floor (rustls session ~15–20 KB + 9.9 KB task future) is Proposal 1 territory. Gates: tokio lib suite 3609 pass; VM monoio lib suite pass; fmt + both clippy matrices clean; unsafe/unwrap audits pass. refs: tmp/C10K-REVIEW.md round 4, .planning/rfcs/c1m-connection-plane.md author: Tin Dang
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
vendor/monoio-io-wrapper/src/unsafe_io.rs (1)
162-191: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConfirm
unsafe_ioremains opt-in in CI, and document the vendor unsafe exemptionNo repo feature currently enables
monoio-rustls/unsafe_io, but this package depends on path overrides formonoio-rustlsandmonoio-io-wrapper, so ensure CI/build manifests don’t enable it. Also add a short comment/exemption note near the vendored source so the project’s unsafe-code audit doesn’t flagRawBuf’s upstreamunsafe impl IoBuf/IoBufMut.🤖 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 `@vendor/monoio-io-wrapper/src/unsafe_io.rs` around lines 162 - 191, Keep the monoio-rustls/unsafe_io feature disabled across CI and build manifests, including path-override configurations for monoio-rustls and monoio-io-wrapper. Add a concise vendor exemption comment near RawBuf and its unsafe IoBuf/IoBufMut implementations documenting that this upstream unsafe code is intentionally retained and audited as an exemption.Source: Coding guidelines
🤖 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 `@vendor/monoio-rustls/src/stream.rs`:
- Line 289: Format the cancelable_read and cancelable_readv function
declarations in the vendored crate so they comply with rustfmt’s default line
width. Run cargo fmt for the crate and verify cargo fmt --check passes.
- Around line 197-220: Remove the unconditional write_io call from
read_io_cancelable’s process_new_packets error path, leaving it to return the
InvalidData error directly. This must preserve split-stream safety by ensuring
the cancelable read half never mutates the session or write buffer; do not alter
the existing read_io behavior.
---
Nitpick comments:
In `@vendor/monoio-io-wrapper/src/unsafe_io.rs`:
- Around line 162-191: Keep the monoio-rustls/unsafe_io feature disabled across
CI and build manifests, including path-override configurations for monoio-rustls
and monoio-io-wrapper. Add a concise vendor exemption comment near RawBuf and
its unsafe IoBuf/IoBufMut implementations documenting that this upstream unsafe
code is intentionally retained and audited as an exemption.
🪄 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: 7839b4e6-3d78-4170-8ddf-912782852e40
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
CHANGELOG.mdCargo.tomlsrc/runtime/mod.rssrc/server/conn/handler_monoio/idle_park.rssrc/server/conn/handler_monoio/mod.rstests/mq_integration.rstests/tls_idle_downshift_parity.rstests/txn_kv_wiring.rstests/workspace_integration.rsvendor/monoio-io-wrapper/Cargo.tomlvendor/monoio-io-wrapper/Cargo.toml.origvendor/monoio-io-wrapper/README.adocvendor/monoio-io-wrapper/src/lib.rsvendor/monoio-io-wrapper/src/safe_io.rsvendor/monoio-io-wrapper/src/unsafe_io.rsvendor/monoio-rustls/Cargo.tomlvendor/monoio-rustls/Cargo.toml.origvendor/monoio-rustls/README.mdvendor/monoio-rustls/examples/connect.rsvendor/monoio-rustls/src/client.rsvendor/monoio-rustls/src/error.rsvendor/monoio-rustls/src/lib.rsvendor/monoio-rustls/src/server.rsvendor/monoio-rustls/src/stream.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/runtime/mod.rs
- CHANGELOG.md
- src/server/conn/handler_monoio/mod.rs
Comment on lines
+197
to
+220
| pub(crate) async fn read_io_cancelable(&mut self, c: CancelHandle) -> io::Result<usize> | ||
| where | ||
| IO: CancelableAsyncReadRent, | ||
| { | ||
| let n = loop { | ||
| match self.session.read_tls(&mut self.r_buffer) { | ||
| Ok(n) => { | ||
| break n; | ||
| } | ||
| Err(ref err) if err.kind() == io::ErrorKind::WouldBlock => { | ||
| self.r_buffer.do_io_cancelable(&mut self.io, c.clone()).await?; | ||
| continue; | ||
| } | ||
| Err(err) => return Err(err), | ||
| } | ||
| }; | ||
|
|
||
| let state = match self.session.process_new_packets() { | ||
| Ok(state) => state, | ||
| Err(err) => { | ||
| let _ = self.write_io().await; | ||
| return Err(io::Error::new(io::ErrorKind::InvalidData, err)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
read_io_cancelable unconditionally calls write_io(), but CancelableAsyncReadRent is implemented for split-capable streams.
read_io takes a splitted flag precisely to avoid touching the write side from the read half (see the upstream comment at Lines 110-113). read_io_cancelable has no such guard, yet Stream is Split (Line 36) and the CancelableAsyncReadRent impl at Line 284 is unconditional — so a read half can drive write_io(), mutating session and w_buffer concurrently with the write half. The doc comment says "Unsplit streams only", but nothing enforces it.
Since this is the error path and the upstream contract already requires the user to shut down manually, the simplest fix is to drop the write attempt (equivalent to splitted = true), or thread a splitted flag through.
🛡️ Proposed fix
let state = match self.session.process_new_packets() {
Ok(state) => state,
Err(err) => {
- let _ = self.write_io().await;
+ // Do not touch the write side here: this path is reachable
+ // from a split read half (see `read_io`'s `splitted` guard).
+ // Callers must shut the stream down on error.
return Err(io::Error::new(io::ErrorKind::InvalidData, err));
}
};📝 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.
Suggested change
| pub(crate) async fn read_io_cancelable(&mut self, c: CancelHandle) -> io::Result<usize> | |
| where | |
| IO: CancelableAsyncReadRent, | |
| { | |
| let n = loop { | |
| match self.session.read_tls(&mut self.r_buffer) { | |
| Ok(n) => { | |
| break n; | |
| } | |
| Err(ref err) if err.kind() == io::ErrorKind::WouldBlock => { | |
| self.r_buffer.do_io_cancelable(&mut self.io, c.clone()).await?; | |
| continue; | |
| } | |
| Err(err) => return Err(err), | |
| } | |
| }; | |
| let state = match self.session.process_new_packets() { | |
| Ok(state) => state, | |
| Err(err) => { | |
| let _ = self.write_io().await; | |
| return Err(io::Error::new(io::ErrorKind::InvalidData, err)); | |
| } | |
| }; | |
| pub(crate) async fn read_io_cancelable(&mut self, c: CancelHandle) -> io::Result<usize> | |
| where | |
| IO: CancelableAsyncReadRent, | |
| { | |
| let n = loop { | |
| match self.session.read_tls(&mut self.r_buffer) { | |
| Ok(n) => { | |
| break n; | |
| } | |
| Err(ref err) if err.kind() == io::ErrorKind::WouldBlock => { | |
| self.r_buffer.do_io_cancelable(&mut self.io, c.clone()).await?; | |
| continue; | |
| } | |
| Err(err) => return Err(err), | |
| } | |
| }; | |
| let state = match self.session.process_new_packets() { | |
| Ok(state) => state, | |
| Err(err) => { | |
| // Do not touch the write side here: this path is reachable | |
| // from a split read half. Callers must shut the stream down on error. | |
| return Err(io::Error::new(io::ErrorKind::InvalidData, err)); | |
| } | |
| }; |
🤖 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 `@vendor/monoio-rustls/src/stream.rs` around lines 197 - 220, Remove the
unconditional write_io call from read_io_cancelable’s process_new_packets error
path, leaving it to return the InvalidData error directly. This must preserve
split-stream safety by ensuring the cancelable read half never mutates the
session or write buffer; do not alter the existing read_io behavior.
| IO: AsyncReadRent + AsyncWriteRent + CancelableAsyncReadRent, | ||
| C: DerefMut + Deref<Target = ConnectionCommon<SD>>, | ||
| { | ||
| async fn cancelable_read<T: IoBufMut>(&mut self, mut buf: T, c: CancelHandle) -> BufResult<usize, T> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
These signatures exceed rustfmt's default max_width and will fail the formatting gate.
Both cancelable_read and cancelable_readv declarations run past 100 columns, so cargo fmt --check will reject them. Run cargo fmt over the vendored crate (or wrap manually as below).
Based on learnings: "Before pushing, run the documented CI matrix: formatting, both clippy configurations with -D warnings, release tests, MSRV build, and applicable safety, unwrap, fuzz, and CodeQL checks."
🎨 Proposed formatting
- async fn cancelable_read<T: IoBufMut>(&mut self, mut buf: T, c: CancelHandle) -> BufResult<usize, T> {
+ async fn cancelable_read<T: IoBufMut>(
+ &mut self,
+ mut buf: T,
+ c: CancelHandle,
+ ) -> BufResult<usize, T> {- async fn cancelable_readv<T: IoVecBufMut>(&mut self, mut buf: T, c: CancelHandle) -> BufResult<usize, T> {
+ async fn cancelable_readv<T: IoVecBufMut>(
+ &mut self,
+ mut buf: T,
+ c: CancelHandle,
+ ) -> BufResult<usize, T> {Also applies to: 312-312
🤖 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 `@vendor/monoio-rustls/src/stream.rs` at line 289, Format the cancelable_read
and cancelable_readv function declarations in the vendored crate so they comply
with rustfmt’s default line width. Run cargo fmt for the crate and verify cargo
fmt --check passes.
Source: Learnings
TinDang97
added a commit
that referenced
this pull request
Jul 29, 2026
…conns −94%) (#423) Patch release rolling up the connection-plane campaign: PR #421 (rounds 1-4: W1-W8+T3, P3 future diet, P4a --uring-entries, W11 idle downshift, P4b TLS diet) and PR #422 (round 5: P1 task-exit parking), plus the SDK reconnect fix. Headline: idle-connection memory 56.5 → 3.25 KB/conn (−94%); 1 M idle connections ≈ 3.3 GB. Long-idle plain-TCP conns exit their handler task entirely (--conn-park-secs, default 60 s), leaving a tiny readiness watcher; wake is wire-invisible and re-parks indefinitely. TLS idle 87.5 → 47.4 KB/conn via vendored monoio-rustls/io-wrapper lazy+releasable buffers. Pipeline memory ratchet fixed (~217 KB permanent → 47 KB). Operability: loud maxclients -ERR + RLIMIT_NOFILE check, 16-way striped client registry, deadline-heap blocking sweep, SPSC drain rotation, affinity-funnel load gate. Validation: same-binary flag A/Bs on the Linux VM at 10 k conns for every memory wave; GCE t2a hardware proof (idle −46%, pinned p=1/p=16 perf-neutral); wire-parity suites across park/wake cycles green on kqueue AND io_uring; parked conns visible in CLIENT LIST and killable. This release touches no crash/persistence path; the crash-matrix + soak gate is dispatched on the RC to hold the ritual (soak-first-then-tag). Rolls CHANGELOG [Unreleased] into [0.8.3], bumps Cargo.toml/lock, adds the RELEASES.md row, updates the README milestone table. author: Tin Dang
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.
Summary
Connection-plane scalability wave from the 2026-07-29 c10k/c10m investigation (
tmp/C10K-REVIEW.md). Eight workstreams (W1–W8) plus the Tier-3 subset: fixes the permanent per-connection pipeline memory ratchet, makes maxclients rejection loud and early, deletes dead waker plumbing, stripes the global client registry, replaces the blocking-registry full walk with a deadline heap, rotates SPSC drain start, and load-gates the IP-affinity funnel. The c1M redesign (idle-conn parking, uring buf_ring) is written up as an RFC in.planning/rfcs/c1m-connection-plane.md(submodule commit 356b0a1).A/B results (moon-dev VM, aarch64, shards=2, monoio release)
The ratchet fix beat its ≤75 KB/conn expectation: the 16 KiB I/O-buffer shrink floor + batch-vec governor put post-burst footprint below the old pure-idle baseline.
Workstreams
01491f09):responses/framesbatch vecs shrink back to 64-cap when they exceed 256; I/O buffer shrink floor 64 KiB → 16 KiB; shrink placed at end-of-iteration so idle conns (parked inread()) actually reach it.85d5c590):active_cross_txn: Option<Box<CrossStoreTxn>>removes 2.16 KB from every connection future;size_of::<ConnectionState>() <= 768enforced by test.50dfefe8): monoio path now writes-ERR max number of clients reachedbefore close (was a silent drop), gate hoisted before per-conn ctx construction; startupRLIMIT_NOFILEcheck vs maxclients + reserved fds. Integration testtests/maxclients_reject_parity.rs.030c6b1b): vestigialpending_wakersrelay (per-conn clone, zero registrants since the flume-oneshot reply design) deleted end-to-end.bed94a34): global client registry → 16RwLockstripes keyed byid % 16; CLIENT LIST walks one stripe at a time; per-stripe fd-shutdown liveness invariant preserved and documented;TOTAL_CLIENTSatomic.e3821e4c):BinaryHeap<Reverse<(Instant, usize, Bytes)>>lazy-invalidated index replaces the 100 Hz full-registry walk; only due queues are scanned.e59c93e6): thread-local rotation cursor so the drain loop doesn't always start at consumer 0 (peer-shard starvation under sustained load).112f8bf7): IP-affinity routing skips a shard whencount > 2*(total/n)+16(per-shard atomic conn counters), bounding the ~2× funnel skew.7a3f921a): loud startup warning thatMOON_URING=1tokio-bridge conns bypass maxclients + registry; CHANGELOG entry; c1M RFC.Round 2 (same branch): roadmap continuation — P4a + P3 shipped
--uring-entries N(fdcd5d90): per-shard io_uring SQ size (monoio default 1024; also the legacy driver's event batch). Zero vendored-monoio changes —RuntimeBuilder::with_entrieswas already public. Clamps at monoio's 256 floor with a warning; loud no-op under tokio. Smoke-tested live (4096 → startup log + PONG).39203484):-Zprint-type-sizesshowed the 58-variant connection future sized entirely by cold suspend points.Box::pinof TXN.ABORT rollback, leaked-txn teardown, and the FT.* body (behind their name-check fast paths, both handlers; never on the per-command filter chain) drops the per-conn monoio task allocation 9280 → 5952 B plain TCP / 13248 → 9920 B TLS. The hot cross-shard dispatch await is deliberately left inline.poll-iofeature lead).Round 3: W11 idle-buffer downshift
b80c2bf9): connections parked inread()≥1s are cancelled by the shard's existing 1s chore (thread-localCancellerregistry; per park = twoCellstores + anRcclone, zero hot-path allocs/timers) and re-park on a 512 B probe buffer, shedding the 8 KiB rent buffer + empty scratch. Cancel-and-await on the same pinned future is loss-free on both drivers (io_uringAsyncCancel, legacyREAD_CANCELED) — a completion racing the cancel still delivers its bytes. TLS + tokio keep prior behavior via theIdleParkReadcapability trait.moon-bench-arm, pinned cores, 3 alternating rounds): GET p=1 −0.4%, busy-poll GET p=1 −0.2%, p=16 flat. (An OrbStack A/B showed a phantom −30% p=1 — VM noise, refuted by the pinned GCE run.)tests/idle_downshift_parity.rs(probe wake / 700 B pipeline / 4 KiB value / undisturbed active sibling) green on both kqueue and io_uring cancel paths.Round 4: P4b TLS diet — vendored wrapper stack
d2d01449; moon-patch style likevendor/monoio, wired via[patch.crates-io]): the wrapper's two eagerly-allocated, never-freed 16 KiB per-stream buffers are now lazy + released on idle downshift, and the TLS stream implements a loss-freeCancelableAsyncReadRent— so W11's idle sweep now covers TLS connections end-to-end (SUPPORTS_IDLE_PARK = true+ anon_idle_downshifthook that sheds both wrapper buffers; they reallocate lazily on the next I/O).do_iostashes read errors for status replay; a stashed ECANCELED would resurface on the NEXT plain read and tear down a healthy connection. The cancelable path therefore returns errors WITHOUT stashing (unit-tested in the vendored crate, 5 tests).tests/tls_idle_downshift_parity.rs: one continuous TLS session across three downshift/wake cycles (probe wake, 700 B pipeline, 4 KiB value), rustls client with a throwaway self-signed cert — green on kqueue and io_uring.d2f3e7cf): round 2'suring_entriesconfig field broke three tokio-only integration suites (E0063) that no lib-suite gate compiles — this is what had the tokio Check job red. Fixed + theitems_after_test_moduleclippy nit; all three suites verified green on the VM tokio matrix.Test plan
cargo fmt --check; clippy default + tokio,jemalloc matrices: 0 warningstests/maxclients_reject_parity.rs(reject message + slot-frees-on-disconnect)tmp/c10k/)author: Tin Dang
Summary by CodeRabbit
--uring-entriesto tune io_uring queue capacity for Monoio deployments.maxclientsnow receive a clear error response.