fix(server): accept-loop HOL block, migration fd ownership, park auth gates (#438 F3–F6 + conn-secondary) - #446
Conversation
… gates (#438 F3-F6 + conn-secondary) F3 (routing F10) — the central accept loop delivered every connection with a blocking send on the routed shard's bounded (4096) conn channel: one wedged shard froze accepts for ALL shards. The monoio legs were worse than reviewed — `flume::Sender::send` there is synchronous, so a full channel stalled the entire listener THREAD (plain + TLS legs alike). All five delivery sites (tokio plain/TLS, monoio plain/TLS x2 select shapes) now `try_send` and rotate to the next shard with room (`try_route_conn`); the affinity hint is best-effort and any live shard serves the client. Only when EVERY channel refuses does the loop fall back to the pre-F3 blocking send — server-wide saturation, where back-pressure is the correct behaviour — with a warn. Unit-tested (rotation, disconnected-skip, all-full payload return, target-first). F4 (routing F12) — MigrateConnectionPayload carried the migrated socket as a raw i32 with no Drop: a migration message still queued in an SPSC ring at shutdown, or drained into pending_migrations but never spawned, leaked the fd and stranded the client on a connection no task would ever serve (silent, until the fd table filled). The payload now owns the socket as an OwnedFd end to end — producer (tokio into_std; monoio dup wrapped at birth) → ring → pending_migrations → spawn_migrated_* — so every undelivered path closes the socket and the client gets a FIN. This also discharges the F1-drain interaction flagged on PR #445: queued migrations dropped by a shutting-down shard now close deterministically. Three `unsafe from_raw_fd` blocks became safe ownership conversions (net -3 unsafe). Unit test pins the drop semantics; a revert to a raw fd no longer compiles. Resumed-parked conns stay deliberately can_migrate:false — the resume wrapper has no MigrateConnection arm, and migration sampling exists for HOT conns while this path is only reachable by idling past --conn-park-secs; rationale documented at the spawn site. F5 (sec L3) — both migrated-connection spawn sites built the target shard's ConnectionContext with requirepass: None. Session auth was unaffected (it travels in MigratedConnectionState), but AUTH issued on a migrated conn wrongly answered "no password is set", and any future auth-re-derivation from the context would have failed open. The real requirepass is now read from runtime config at spawn. F6 (sec L2) — unauthenticated connections could task-park: on an auth-enabled server an attacker could open maxclients silent sockets, never AUTH, and hold each at the parked ~3.3 KB watcher footprint indefinitely and invisibly. The stage-2 parkable predicate now requires conn.authenticated — pre-AUTH conns keep their full handler task (costly enough to surface in monitoring, still reaped by `timeout N`); no-auth servers see no change (authenticated=true from accept). Red/green on macOS monoio: parked_clients=2 pre-fix, =1 post-fix (authed idle sibling doubles as the positive control that the park machinery stayed live). conn-secondary — * is_sweep_cancel matched any bare errno 125, so an ECANCELED from a non-sweep source read as "sweep cancel -> re-park": for a dead fd that is a permanent park->wake->park spin at 100% CPU (the exact E11 failure class the errno check was built to prevent). The idle-park slot now records provenance (`swept`, set by sweep/drain before firing, re-armed false at every park) and the park arms require it via consume-once `was_swept_cancel`; blocking.rs keeps the plain errno check where a stray 125 only re-arms a level-triggered poll (no spin possible). Unit-tested (provenance required, consume-once, stale-flag cleared on re-park, drain marks too). * client_registry::register double-counted TOTAL_CLIENTS and the shard gauges when inserting over a still-present id (the kept_registration miss-arm fail-safe racing an entry back into existence): permanent +1 skew feeding shard_overloaded routing. register() now balances against the replaced entry, and the miss arm (an invariant violation, not a code path) warns loudly. Unit-tested. * `timeout` read-once: already fixed by the D1 chore-sweep rework — both shard chores re-read runtime_config.timeout every second and CONFIG SET timeout is live (verified, comment at the monoio chore says exactly this). No change. Gates: fmt + clippy -D warnings (default and tokio,jemalloc), new unit tests green under both feature sets, parked_idle_parity 7/7 + shutdown_drain 4/4 + tls_park_keyupdate on macOS monoio AND tokio (F6 leg degenerates to parked=0 under tokio as documented), full suites on macOS monoio (28 suites; one cross_shard_consistency_red load flake, solo-green — same documented family as cdg6e), Linux VM monoio (lib 4519/4519; client_tracking_invalidation push-delivery flake fires on UNMODIFIED main too — 3x-vs-3x merge-base A/B, 1/3 main vs 2/3 branch, same missing-second-key signature; pre-existing, queued for the test-hygiene sweep) and Linux VM tokio CI-parity (exit 0, 190 suites). Bench gates waived — no hot-path cost: try_route_conn replaces a blocking send with try_send attempts on the accept path (not the command path); the OwnedFd is layout-identical to i32 with a Drop that only runs on the cold migration/teardown path; the park-arm provenance check is two Cell ops on the already-cold cancelled-read path; the parkable predicate gains one bool test evaluated once per park decision. Refs #438 (F3 = routing F10, F4 = routing F12, F5 = sec L3, F6 = sec L2, conn-secondary) author: Tin Dang
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix accept-loop HOL blocking, migration fd ownership, and auth-gated task parking
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/client_registry.rs (1)
1311-1327: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the global count after deregistration.
The test checks
t2 == t1before cleanup and checks onlylive_handle(ID).is_none()afterward. It never verifies thatderegister(ID)restoresTOTAL_CLIENTStot1. Add a post-deregistration count assertion under the same test serialization.🤖 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/client_registry.rs` around lines 1311 - 1327, Extend replacing_register_does_not_double_count after deregister(ID) to load TOTAL_CLIENTS with Ordering::Relaxed and assert it equals the pre-replacement baseline t1, while retaining the existing live_handle assertion and test serialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client_registry.rs`:
- Around line 1315-1322: Make the registry test containing the TOTAL_CLIENTS
assertion run under the existing test serializer so reads of the process-global
counter cannot race with other registry-mutating tests. If no suitable
serializer exists, add a test-only parking_lot::Mutex and acquire its guard for
the test’s full registration, assertion, and deregistration scope, ensuring
cleanup still occurs before releasing the guard.
In `@src/server/listener.rs`:
- Around line 43-56: The try_route_conn function must distinguish full live
channels from disconnected channels: track a full channel index while iterating,
continue probing other shards, and return that index with the payload only when
all channels are unavailable. Update its callers to use the returned live full
shard for fallback blocking, and add coverage for a disconnected target with
another live full shard.
In `@src/shard/conn_accept.rs`:
- Around line 1021-1026: The new unsafe code requires explicit approval: approve
the OwnedFd::from_raw_fd transfer in src/shard/conn_accept.rs lines 1021-1026,
and approve the libc::fcntl(F_GETFD) assertions in src/shard/dispatch.rs lines
1669-1679, or move each operation behind an already-approved isolated
module/test utility.
In `@src/shard/event_loop.rs`:
- Around line 1002-1008: The Rust sources exceed the 1500-line limit; split the
responsibilities into focused modules while preserving behavior. In
src/shard/event_loop.rs lines 1002-1008, extract migration queue draining and
handoff; in src/server/conn/handler_sharded/mod.rs lines 195-205, separate
connection lifecycle from dispatch; in src/shard/conn_accept.rs lines 410-427,
separate Tokio and Monoio acceptance and migration setup; and in
src/shard/dispatch.rs lines 366-386, extract migration payload definitions and
related tests. Ensure all resulting files remain within the size limit and
existing symbols and behavior continue to work.
In `@tests/parked_idle_parity.rs`:
- Around line 454-463: Update the assertion following the parked_clients check
to close the authenticated connection (`authed`), then poll `INFO clients` until
`parked_clients` reaches 0 while `silent` remains open. Assert this post-close
state so the test specifically detects parking of the unauthenticated connection
rather than relying only on the aggregate gauge.
---
Nitpick comments:
In `@src/client_registry.rs`:
- Around line 1311-1327: Extend replacing_register_does_not_double_count after
deregister(ID) to load TOTAL_CLIENTS with Ordering::Relaxed and assert it equals
the pre-replacement baseline t1, while retaining the existing live_handle
assertion and test serialization.
🪄 Autofix
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: 5c644b11-b013-469f-be6f-83edee2cefa1
📒 Files selected for processing (11)
CHANGELOG.mdsrc/client_registry.rssrc/server/conn/handler_monoio/idle_park.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/server/listener.rssrc/shard/conn_accept.rssrc/shard/dispatch.rssrc/shard/event_loop.rssrc/shard/spsc_handler.rstests/parked_idle_parity.rs
| // F4 (#438): the fd is an OwnedFd — entries still queued when the event | ||
| // loop exits (shutdown) drop here and CLOSE their sockets, giving the | ||
| // client a FIN instead of a permanently stranded silent connection. | ||
| let mut pending_migrations: Vec<( | ||
| crate::shard::dispatch::RawSocketFd, | ||
| crate::shard::dispatch::MigrateFd, | ||
| crate::server::conn::affinity::MigratedConnectionState, | ||
| )> = Vec::new(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split the oversized Rust source files.
These changed files exceed the 1500-line limit.
src/shard/event_loop.rs#L1002-L1008: Extract migration queue draining and handoff into a focused module.src/server/conn/handler_sharded/mod.rs#L195-L205: Split connection lifecycle and dispatch responsibilities into modules.src/shard/conn_accept.rs#L410-L427: Split Tokio and Monoio connection acceptance and migration setup.src/shard/dispatch.rs#L366-L386: Extract migration payload definitions and related tests into focused modules.
As per coding guidelines, “No single Rust file should exceed 1500 lines.”
📍 Affects 4 files
src/shard/event_loop.rs#L1002-L1008(this comment)src/server/conn/handler_sharded/mod.rs#L195-L205src/shard/conn_accept.rs#L410-L427src/shard/dispatch.rs#L366-L386
🤖 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/shard/event_loop.rs` around lines 1002 - 1008, The Rust sources exceed
the 1500-line limit; split the responsibilities into focused modules while
preserving behavior. In src/shard/event_loop.rs lines 1002-1008, extract
migration queue draining and handoff; in src/server/conn/handler_sharded/mod.rs
lines 195-205, separate connection lifecycle from dispatch; in
src/shard/conn_accept.rs lines 410-427, separate Tokio and Monoio acceptance and
migration setup; and in src/shard/dispatch.rs lines 366-386, extract migration
payload definitions and related tests. Ensure all resulting files remain within
the size limit and existing symbols and behavior continue to work.
Source: Coding guidelines
There was a problem hiding this comment.
Skipping in this PR: all three files exceeded the 1500-line limit before this change (this PR adds ~30 lines to each), and a module split mid-hardening-campaign would bury the behavioral diff under a mechanical one. Noting the split (event_loop migration-drain extraction, handler_sharded lifecycle/dispatch split) as a follow-up refactor candidate after the #438 tail lands.
There was a problem hiding this comment.
@TinDang97, acknowledged. The file-size finding remains valid, but deferring the mechanical module split keeps this hardening PR focused.
Would you like me to create a GitHub follow-up issue for the event_loop migration-drain extraction and handler_sharded lifecycle/dispatch split after #438 lands?
You are interacting with an AI system.
There was a problem hiding this comment.
Yes, please create the follow-up issue for the module split (event_loop migration-drain extraction + handler_sharded lifecycle/dispatch split), referencing this PR and the 1500-line guideline.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="pilotspace/moon"
body=$(cat <<'EOF'
## Summary
Refactor the oversized Rust modules that remain after the Cluster F hardening work.
This follow-up is intentionally separate from PR `#446`. The current PR changes connection lifecycle behavior. A mechanical module split in the same change would make that behavior harder to review.
## Required changes
- Extract the pending migration queue drain and connection handoff from `src/shard/event_loop.rs` into a focused module.
- Split connection lifecycle responsibilities from dispatch responsibilities in `src/server/conn/handler_sharded/mod.rs`.
- Preserve the current connection migration, shutdown, registration, and dispatch behavior.
## Rationale
The repository coding guideline requires that no Rust source file exceeds 1500 lines. These files already exceeded that limit before PR `#446`. The hardening PR adds only limited changes, so this refactor is deferred to keep the behavioral diff focused.
## Affected areas
- `src/shard/event_loop.rs`
- `src/server/conn/handler_sharded/mod.rs`
## Backlinks
- PR: https://github.com/pilotspace/moon/pull/446
- Review discussion: https://github.com/pilotspace/moon/pull/446#discussion_r3736083965
- Requested by: `@TinDang97`
## Acceptance criteria
- The migration-drain and handoff logic is in a focused module.
- Connection lifecycle and dispatch responsibilities are separated into focused modules.
- Each resulting Rust source file complies with the 1500-line guideline.
- Existing public and internal behavior remains unchanged.
- The relevant unit and consistency tests pass.
- Formatting and Clippy complete without new warnings for both runtime feature configurations.
EOF
)
gh issue create \
--repo "$repo" \
--title "Refactor oversized shard connection modules after `#446`" \
--body "$body" \
--assignee "TinDang97"Length of output: 2056
@TinDang97, created follow-up issue #447.
It covers the event_loop migration-drain extraction and the handler_sharded lifecycle/dispatch split. It references PR #446, the review discussion, and the 1500-line Rust file guideline.
You are interacting with an AI system.
…ic tests, F6 identity check Review findings (Qodo + CodeRabbit, converging on one real defect): 1. Fallback could drop a conn a live shard would take (both bots, CONFIRMED): when try_route_conn exhausted the rotation, the blocking fallback targeted the ORIGINALLY routed shard — if that receiver was disconnected while another shard was merely full, the fallback errored instantly and the accepted conn was dropped. The helper now reports the first LIVE-but-full shard from the rotation (Err((Option<usize>, payload))); callers back-pressure on THAT shard, and only drop (with an error log) when every receiver is gone — i.e. shutdown, where dropping is correct. New unit tests: disconnected-target-with-live-full-shard, all-disconnected. 2. Flaky global-state unit tests (both bots): the TOTAL_CLIENTS bracket could false-fail if an unrelated test registered between its two loads, and a failed assert skipped cleanup; it now retries with a fresh id (bug reads +1 on EVERY attempt; fix needs one undisturbed attempt) and deregisters before the verdict. The F4 fd-drop test probed the raw fd number, which the parallel runner could reuse between close and probe; it now observes the close through the PEER's read() -> EOF — no fd-number reasoning at all. 3. F6 test identified WHICH conn parked (CodeRabbit): parked_clients=1 also holds if the silent conn parked and the authed one didn't. The test now closes the authed conn and requires the gauge to drain to 0 while the silent conn stays open. Skipped with rationale (replied on-thread): - "Split oversized files" — pre-existing sizes, out of scope for a hardening PR; noted for a future refactor wave. - New-unsafe flag on the monoio dup wrap — consolidates two prior unsafe sites at the same location; net unsafe count for the PR is -3, SAFETY comment present. Gates: fmt + clippy -D warnings (both feature sets), listener/registry/ migrate-fd unit tests green both feature sets, parked_idle_parity monoio 3/4 full-green runs (the one bad run failed on connect/read setup races in OTHER tests — the documented harness flake class queued for the test-hygiene sweep; the new assertions never tripped) and tokio 7/7. Refs #438, #446 author: Tin Dang
…iene) (#449) Pid-only temp-dir names (`temp_dir().join(format!("...-{}", pid))`) at eight spawn sites across six suites resurrected STALE data dirs: a crashed run leaves its dir behind, the pid is eventually reused, and the next run's server silently reloads the leftover persistence state (the documented CWD/stale-reload trap) — failures then point at whatever assertion tripped over the ghost data, not at the cause. - info_memory_allocator_pagecache, memory_doctor_response, memory_prometheus_kinds: the shared Moon harness now owns a tempfile::TempDir (random unique name; removed on drop AFTER the child is killed — hand-written Drop body runs before field drops). - vector_exact_rerank (3 sites): RAII tempdir; also removes the manual pre/post remove_dir_all pairs, so cleanup now happens even when an assert panics mid-test. - vector_db_isolation (2 sites): unique random dirs via tempfile::Builder + keep(). Deliberately NOT RAII: the restart test shares one dir between two Moon values (kill_keep_dir → same-port respawn), so the existing PathBuf ownership + manual cleanup are kept and only the collision-prone NAME is fixed. - tls_park_keyupdate: RAII tempdir; early-return and end-of-test manual removes deleted. parked_idle_parity deadline-poll fixes (the flakes deferred from the #444/#445 rounds, all environmental classes observed in CI or today's gate runs): - CLIENT KILL registry-removal assert now POLLS with a 10s deadline — the client-side close arrives instantly via shutdown(2) but the registry entry is released only when the killed handler task gets scheduled; the read-once assert fired 3/3 on the starved 2-vCPU runner and passed solo (documented assert-too-soon race). - All 13 TcpStream::connect sites use a connect_retry helper (10s deadline, 50ms backoff): under full-suite load a freshly listening server can still refuse the first attempts, which failed tests at the connect line (seen live today at the F6 test's connect). - Read deadlines 10s → 30s in the two reply helpers; deadline-bound, so green runs spend no extra time. The client_tracking_invalidation multikey second-key push flake is PRODUCT-side (a real RESP3 client would miss the same invalidation) — filed as #448 with the merge-base A/B evidence instead of being papered over here. Gates: fmt + clippy -D warnings (default and tokio,jemalloc); all six converted suites + parked_idle_parity green on macOS monoio (31 tests); tokio leg green on the four suites that run there. Bench gates waived — tests-only change, no src/ code touched. Refs #444 #445 #446, closes nothing (#448 filed for the product flake) author: Tin Dang
Continues the #438 hardening tail (sequential campaign, after F1/F2 in #445). Four findings from the c10k review's Cluster F plus the conn-secondary triple.
F3 — central accept loop head-of-line block (routing F10)
Every central-listener delivery blocked on the routed shard's bounded (4096) conn channel; one wedged shard froze accepts for all shards. The monoio legs were worse than reviewed:
flume::Sender::sendthere is synchronous, so a full channel stalled the whole listener thread, TLS accepts included. All five delivery sites nowtry_sendand rotate to the next shard with room (try_route_conn, unit-tested); only when every channel refuses does the loop fall back to the pre-F3 blocking send (server-wide saturation ⇒ back-pressure is correct) with a warn.F4 — migration fd lifecycle (routing F12)
MigrateConnectionPayloadcarried the socket as a rawi32with no Drop: a migration message still queued at shutdown, or drained but never spawned, leaked the fd and stranded the client. The payload now owns the socket as anOwnedFdend to end (producer → SPSC → pending-migrations → target-shard spawn), so every undelivered path closes the socket and the client sees a FIN. This also discharges the F1-drain interaction flagged by CodeRabbit on #445: queued migrations dropped by a shutting-down shard now close deterministically. Net −3unsafeblocks (rawfrom_raw_fdconversions became safeFromimpls). Resumed-parked conns stay deliberatelycan_migrate:false— rationale documented at the spawn site (no MigrateConnection arm in the resume wrapper; migration sampling exists for hot conns, this path is only reachable by idling past--conn-park-secs).F5 — migrated
ConnectionContextrequirepass (sec L3)Both migrated-spawn sites built the context with
requirepass: None. Session auth was unaffected (travels inMigratedConnectionState), butAUTHon a migrated conn wrongly answered "no password is set" and any future auth re-derivation would have failed open. Now reads the real value from runtime config.F6 — unauthenticated conns could task-park (sec L2)
On an auth-enabled server, silent pre-AUTH sockets downshifted and task-parked into the ~3.3 KB watcher state — a maxclients-worth of connections holdable indefinitely at near-zero, invisible cost. The stage-2 parkable predicate now requires
conn.authenticated. Red/green on macOS monoio:parked_clients=2pre-fix →=1post-fix (the authed idle sibling is the in-test positive control that parking machinery stayed live). No-auth servers unaffected.conn-secondary
is_sweep_cancelbare-125: anyECANCELEDread as "sweep cancel → re-park"; for a dead fd that's a permanent park→wake→park spin at 100% CPU (the E11 class). Park arms now require sweep/drain provenance via consume-onceIdleSlot::was_swept_cancel(flag set bysweep/cancel_all_parkedbefore firing, re-armed false at every park).blocking.rskeeps the plain errno check where a stray 125 only re-arms a level-triggered poll (no spin possible). Unit-tested.register()over a still-present id double-countedTOTAL_CLIENTS/shard gauges permanently (skewsshard_overloadedrouting). Now balances against the replaced entry; thekept_registrationmiss arm (invariant violation, not a code path) warns loudly. Unit-tested.timeoutread-once: already fixed by the D1 chore-sweep rework — both shard chores re-readruntime_config.timeoutevery second;CONFIG SET timeoutis live. No change; verified.Gates
-D warnings, default andruntime-tokio,jemallocparked_idle_parity7/7 +shutdown_drain4/4 +tls_park_keyupdateon macOS monoio and tokiocross_shard_consistency_redload flake, solo-green, documented family) · Linux VM monoio (lib 4519/4519) · Linux VM tokio CI-parity (exit 0, 190 suites)client_tracking_invalidationflake proven pre-existing: 3×-vs-3× merge-base A/B on the VM (1/3 failing on unmodified main vs 2/3 on branch, same missing-second-key push signature) — queued for the test-hygiene sweepCellops on the already-cold cancelled-read path; predicate adds one bool test per park decision)Refs #438 (F3 = routing F10, F4 = routing F12, F5 = sec L3, F6 = sec L2, conn-secondary)
Summary by CodeRabbit