Skip to content

feat(server): task-exit parking for idle connections — 19.8 → 3.25 KB/conn (c1M P1) - #422

Merged
TinDang97 merged 1 commit into
mainfrom
feat/c1m-parked-idle
Jul 29, 2026
Merged

feat(server): task-exit parking for idle connections — 19.8 → 3.25 KB/conn (c1M P1)#422
TinDang97 merged 1 commit into
mainfrom
feat/c1m-parked-idle

Conversation

@TinDang97

@TinDang97 TinDang97 commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

c1M campaign round 5 — Proposal 1 task-exit parking, the c1M lever from .planning/rfcs/c1m-connection-plane.md. An idle monoio plain-TCP connection past --conn-park-secs (default 60 s, 0 = off) has its handler task exit instead of sitting parked inside the read future; only a tiny boxed watcher future survives, awaiting readiness or shutdown.

No vendor patch needed: monoio's public TcpStream::readable(relaxed=false) is a race-free standalone readiness await on both drivers (io_uring PollAdd, cancel-on-drop; legacy readiness + verifying poll), and task memory frees on future return.

Mechanism

  • W11 sweep gains a stage 2: a slot parked ≥ conn-park-secs gets its read cancelled with a stage2 marker; the handler returns MonoioHandlerResult::ParkIdle { Box<MigratedConnectionState>, RegistryGuard }.
  • spawn_parked_idle_watcher owns {stream, state, guard, ctx}; select!s readable(false) vs shutdown. On wake it drops the registry guard, then respawns the full handler via the migration-restore path (can_migrate=false, can_park=true → re-parks indefinitely).
  • Parkable requires: can_park && SUPPORTS_TASK_PARK, empty read/write buffers, !in_multi, empty command queue, no cross-shard txn. Subscriber/tracking/timeout N conns structurally never reach the park arm. can_park is opt-in per call site so a ParkIdle can never be silently dropped (TLS/fail-open/migrated-spawn pass false).
  • CLIENT LIST sees parked conns; CLIENT KILL's shutdown(2) wakes the watcher and the resumed handler reads EOF. record_connection_closed fires exactly once per connection on every path.

Measured (E10, moon-dev VM, 10 k idle conns, shards=2, same-binary flag A/B)

Leg PARK_OFF (W11) PARK_ON
Idle 19.8 KB/conn 3.25 KB/conn (−84 %)
Wake sweep 10 k PINGs 0.10 s, bad=0 0.25 s, bad=0 (~15 µs/conn)
Idle again after sweep 20.0 3.75 (all re-park)
After close RSS returns RSS returns

Campaign total: 56.5 → 3.3 KB/idle-conn (−94 %); 1 M idle ≈ 3.3 GB.

Gates

  • New tests/parked_idle_parity.rs (3 tests): multi-cycle park/wake parity (probe, 100-deep pipeline, 4 KiB value), CLIENT LIST/KILL on a parked conn, active-sibling isolation — green on kqueue and io_uring (+ tokio degenerate).
  • idle_park unit tests 10/10; VM monoio lib 4466 pass; tokio lib 3609 pass; W11 + TLS parity suites still green.
  • fmt, clippy --all-targets on both matrices (0 lints), unsafe/unwrap audits pass.
  • Independent lifecycle review: clean except one Low (documented at the drop site): on data-wake the deregister→re-register window spans a task-scheduling boundary, so a racing CLIENT LIST briefly misses the waking conn — same observable as a reconnect race.

Follow-ups (documented, unscheduled)

  • TLS task-parking (readable() passthrough in the vendored wrapper; TLS keeps SUPPORTS_TASK_PARK=false).
  • Migrated-spawn site routes ParkIdle (currently can_park=false).
  • Registration handoff to close the Low-severity wake-window race.

refs: .planning/rfcs/c1m-connection-plane.md (Proposal 1, sequencing item 6), tmp/C10K-REVIEW.md round-5 appendix

Summary by CodeRabbit

  • New Features
    • Added task-exit parking for idle plain-TCP connections to reduce resource usage.
    • Parked connections remain visible in CLIENT LIST and continue supporting CLIENT KILL.
    • Connections automatically resume when new data arrives or the server shuts down.
    • Added the --conn-park-secs option, defaulting to 60 seconds; set it to 0 to disable parking.
  • Compatibility
    • Feature applies to monoio plain-TCP connections and does not affect tokio runtime connections or unsupported connection states.

…/conn (c1M P1)

A monoio plain-TCP connection idle past --conn-park-secs (default 60 s,
0 = off) now has its handler task EXIT instead of sitting parked inside
the read future. The task returns MonoioHandlerResult::ParkIdle carrying
{Box<MigratedConnectionState>, RegistryGuard}; conn_accept spawns a tiny
boxed watcher future owning {TcpStream, state, guard, ConnectionContext}
that awaits stream.readable(false) (race-free on both io_uring and
kqueue/epoll — no vendor patch needed) or shard shutdown. On wake it
drops the registry guard synchronously and respawns the full handler
through the migration-restore path (can_migrate=false, can_park=true),
so a resumed connection can park again indefinitely.

Mechanism details:
- Stage-2 arm in handler_monoio/mod.rs splits: parkable requires
  can_park && S::SUPPORTS_TASK_PARK && park_after_ms() > 0, empty
  read/write buffers, !in_multi, empty command_queue, and no active
  cross-shard txn. Subscribers, tracking conns, and `timeout N` conns
  are structurally excluded (their select arms precede the park arm).
- idle_park sweep gains a stage2 flag per slot: stage-1 cancels at
  IDLE_DOWNSHIFT_MS (1 s, probe-buffer downshift), stage-2 cancels at
  conn_park_after_ms (task exit). TLS keeps SUPPORTS_TASK_PARK=false
  (follow-up: readable() passthrough in the vendored wrapper).
- RegistryGuard hoisted to module scope and moved through ParkIdle so
  deregister-before-fd-close holds on every path; CLIENT KILL's
  shutdown(2) makes the parked fd read-ready, the watcher wakes, and
  the resumed handler reads EOF. record_connection_closed fires exactly
  once per connection (original task skips it on park; the watcher or
  resumed task owns it).
- can_park is opt-in per call site: only sites that route ParkIdle pass
  true (plain-TCP accept + resumed helper); TLS, fail-open, and
  migrated-spawn sites pass false so a ParkIdle can never be silently
  dropped (= closed). Follow-up noted to wire the migrated-spawn site.
- Known limitation (independent review, Low): on data-wake the
  deregister→re-register window spans a task-scheduling boundary, so a
  racing CLIENT LIST briefly misses the waking conn and CLIENT KILL ID
  returns 0 (same observable as a reconnect race). Closing it needs
  registration handoff into the handler; documented at the drop site.
- New flag plumbed via moon::runtime atomic (set_conn_park_secs);
  tokio runtime warns and ignores it.

Measured (E10, moon-dev VM, 10 k idle conns, shards=2, same-binary
flag A/B, tmp/c10k/e10_park_rss.sh):
- PARK_OFF (W11 baseline): 19.8 KB/conn idle
- PARK_ON: 3.25 KB/conn idle; re-parks at 3.75 after a full wake sweep
- Wake sweep wire-correct: bad=0; 0.25 s vs 0.10 s for 10 k PINGs
  (~15 µs/conn extra wake cost, off the hot path)
- Campaign total: 56.5 → 3.3 KB/idle-conn (−94 %); 1 M idle ≈ 3.3 GB

Gates:
- tests/parked_idle_parity.rs (new, 3 tests): multi-cycle park/wake
  parity (probe, 100-deep pipeline, 4 KiB value), CLIENT LIST/KILL on
  a parked conn, active-sibling isolation — green on kqueue (macOS)
  AND io_uring (VM), plus tokio (degenerate parity)
- idle_park unit tests 10/10 incl. new stage2_uses_park_after_threshold
- VM monoio lib 4466 pass; tokio lib 3609 pass; W11 + TLS idle parity
  suites still green
- fmt, clippy --all-targets (both matrices, 0 lints), unsafe/unwrap
  audits pass

refs: .planning/rfcs/c1m-connection-plane.md (sequencing item 6, P1)
author: Tin Dang
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds configurable stage-2 task-exit parking for eligible monoio plain-TCP connections. Parked connections retain registry visibility, wake on readability or shutdown, and resume through a fresh handler task. Integration tests cover protocol continuity, client management, and sibling activity.

Changes

Connection task parking

Layer / File(s) Summary
Parking configuration and runtime state
src/config.rs, src/runtime/mod.rs, src/main.rs, CHANGELOG.md
Adds --conn-park-secs, runtime atomic storage, startup wiring, runtime-specific logging, and changelog documentation.
Stage-2 idle threshold handling
src/server/conn/handler_monoio/idle_park.rs
Tracks stage-2 slots, applies the configured threshold, and enables task parking for monoio TCP streams.
Handler parking result and registry lifetime
src/server/conn/handler_monoio/mod.rs
Returns parked migrated state with a registry guard when eligible stage-2 reads are cancelled.
Watcher wakeup and accept-path routing
src/shard/conn_accept.rs
Routes parked plain TCP connections to readiness watchers, resumes them on wakeup, and preserves non-parking TLS and migration paths.
Integration coverage and test configuration
tests/parked_idle_parity.rs, tests/mq_integration.rs, tests/txn_kv_wiring.rs, tests/workspace_integration.rs
Tests repeated wakeups, protocol traffic, CLIENT LIST/KILL, and sibling throughput; existing test servers disable parking.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MonoioHandler
  participant ParkedIdleWatcher
  participant TcpStream
  participant ClientRegistry
  MonoioHandler->>TcpStream: Cancel idle stage-2 read
  MonoioHandler->>ClientRegistry: Preserve registry entry
  MonoioHandler->>ParkedIdleWatcher: Pass parked connection state
  ParkedIdleWatcher->>TcpStream: Wait for readability or shutdown
  TcpStream-->>ParkedIdleWatcher: Report readiness
  ParkedIdleWatcher->>MonoioHandler: Resume connection handler
  ParkedIdleWatcher->>ClientRegistry: Deregister on final close
Loading

Possibly related PRs

  • pilotspace/moon#421: Modifies the monoio idle-connection parking and sweep behavior extended here for stage-2 task-exit parking.

Suggested reviewers: pilotspacex-byte

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: task-exit parking for idle connections.
Description check ✅ Passed The description follows the template and includes summary, checklist, performance impact, and notes with substantive details.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/c1m-parked-idle

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Task-exit parking for idle monoio TCP connections via --conn-park-secs

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Add --conn-park-secs to let idle monoio TCP handlers exit after downshift.
• Route a new ParkIdle outcome to a minimal readiness watcher that respawns handlers.
• Add parity tests for multi-cycle park/wake plus CLIENT LIST/KILL behavior.
Diagram

graph TD
  A["main.rs / CLI"] --> B["runtime park threshold"] --> C["idle_park sweep"] --> D["monoio handler"]
  D -->|"ParkIdle"| E["conn_accept"] --> F["parked watcher"]
  F -->|"readable/shutdown"| D
  D --> G["client registry"]
  F --> G
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Keep handler task alive; further shrink parked working set
  • ➕ Avoids introducing a second parked state machine (watcher→resume) and its edge cases
  • ➕ No transient deregister/re-register window for CLIENT LIST/KILL visibility
  • ➖ Cannot reclaim the handler future/state-machine frame; memory savings likely capped well above this PR’s goal
  • ➖ Still keeps larger per-task runtime bookkeeping vs. exiting the task entirely
2. Pass RegistryGuard through to resumed handler (true handoff)
  • ➕ Eliminates the documented transient invisibility window between drop and re-register
  • ➕ Stronger CLIENT LIST/KILL semantics during wake scheduling delays
  • ➖ More invasive API/ownership changes across handler entry points and registry usage
  • ➖ Bigger correctness surface; likely follow-up-sized change rather than needed for P1 savings

Recommendation: The PR’s approach (exit handler task and keep only a boxed readiness watcher) is the most direct way to reclaim task-frame memory without vendor patching monoio, and it composes cleanly with the existing migration-restore shape. Consider the registry-guard handoff alternative only if the documented transient CLIENT LIST/KILL invisibility becomes operationally problematic; otherwise the current tradeoff seems appropriate for the memory win.

Files changed (11) +608 / -19

Enhancement (5) +350 / -19
main.rsPublish conn-park threshold before shard spawn and log runtime support +15/-0

Publish conn-park threshold before shard spawn and log runtime support

• Calls 'moon::runtime::set_conn_park_secs' before shard threads start. Logs an info message under monoio when enabled, and warns under tokio when the flag is set away from its default.

src/main.rs

mod.rsAdd global atomic conn_park_after_ms runtime setting +19/-0

Add global atomic conn_park_after_ms runtime setting

• Adds a process-wide 'CONN_PARK_AFTER_MS' atomic with setter/getter, mirroring the existing before-shard-spawn configuration contract used for io_uring entries. This value is consumed by the monoio idle-park sweep and handler stage-2 logic.

src/runtime/mod.rs

idle_park.rsExtend idle-park to support stage-2 task-exit threshold +57/-1

Extend idle-park to support stage-2 task-exit threshold

• Adds a stage-2 marker per idle slot and applies 'park_after_ms()' as the sweep threshold for stage-2 parked reads. Introduces 'IdleParkRead::SUPPORTS_TASK_PARK' (enabled for TcpStream) and adds unit coverage for stage-2 threshold behavior.

src/server/conn/handler_monoio/idle_park.rs

mod.rsIntroduce ParkIdle outcome + portable RegistryGuard +100/-18

Introduce ParkIdle outcome + portable RegistryGuard

• Promotes the client-registry RAII guard to a public 'RegistryGuard' so it can outlive the handler task. Adds 'MonoioHandlerResult::ParkIdle' carrying migrated session state and the guard, and gates task parking behind an explicit 'can_park' argument plus a parkability predicate (buffers empty, not in MULTI, no queued commands, no cross-shard txn).

src/server/conn/handler_monoio/mod.rs

conn_accept.rsRoute ParkIdle into a boxed readiness watcher and resume handler on wake +159/-0

Route ParkIdle into a boxed readiness watcher and resume handler on wake

• Updates monoio connection spawn sites to pass 'can_park' only where ParkIdle is handled (plain TCP) and disables it for TLS/migration fallbacks. Adds 'spawn_parked_idle_watcher' (awaits 'TcpStream::readable(false)' or shutdown) and 'spawn_resumed_parked_conn' to rehydrate via the migration-restore path and allow re-parking cycles; ensures close metrics are recorded exactly once by the owning task/watcher chain.

src/shard/conn_accept.rs

Tests (4) +234 / -0
mq_integration.rsDisable conn-park in MQ integration server config +1/-0

Disable conn-park in MQ integration server config

• Sets 'conn_park_secs: 0' in test server config to keep existing integration behavior deterministic and avoid timing-dependent parking during unrelated tests.

tests/mq_integration.rs

parked_idle_parity.rsAdd integration parity tests for parked idle connections +230/-0

Add integration parity tests for parked idle connections

• Adds three tests validating that task-exit parking is wire-invisible across multiple park/wake cycles, that parked conns remain visible/killable via CLIENT LIST/KILL, and that an active sibling connection is unaffected while another parks and wakes.

tests/parked_idle_parity.rs

txn_kv_wiring.rsDisable conn-park in txn wiring integration server config +1/-0

Disable conn-park in txn wiring integration server config

• Sets 'conn_park_secs: 0' for the transaction KV wiring server used in tests, preventing idle parking from influencing transaction-focused scenarios.

tests/txn_kv_wiring.rs

workspace_integration.rsDisable conn-park in workspace integration server configs +2/-0

Disable conn-park in workspace integration server configs

• Sets 'conn_park_secs: 0' in workspace integration server configs (including the auth variant) to avoid introducing timing-based parking into workspace/auth test flows.

tests/workspace_integration.rs

Documentation (1) +17 / -0
CHANGELOG.mdDocument idle task-exit parking and operational semantics +17/-0

Document idle task-exit parking and operational semantics

• Adds an Unreleased changelog entry describing '--conn-park-secs', the watcher-based parking mechanism, and key exclusions (TLS/tokio, subscribers/tracking/timeout, MULTI/cross-txn).

CHANGELOG.md

Other (1) +7 / -0
config.rsAdd '--conn-park-secs' server flag (default 60s) +7/-0

Add '--conn-park-secs' server flag (default 60s)

• Introduces a new CLI/config field 'conn_park_secs' controlling the stage-2 task-exit parking threshold. Documents that the feature applies to monoio plain TCP only and that 0 disables it.

src/config.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Around line 9-25: Remove the duplicate ### Added heading around the task-exit
parking entry and fold that entry into the existing ### Added section within
[Unreleased], preserving the existing ### Fixed section and changelog ordering.
🪄 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: e84fe1f9-c9cf-4e73-a53d-b09e6f96fcfd

📥 Commits

Reviewing files that changed from the base of the PR and between 287a63f and 68031b4.

📒 Files selected for processing (11)
  • CHANGELOG.md
  • src/config.rs
  • src/main.rs
  • src/runtime/mod.rs
  • src/server/conn/handler_monoio/idle_park.rs
  • src/server/conn/handler_monoio/mod.rs
  • src/shard/conn_accept.rs
  • tests/mq_integration.rs
  • tests/parked_idle_parity.rs
  • tests/txn_kv_wiring.rs
  • tests/workspace_integration.rs

Comment thread CHANGELOG.md
Comment on lines +9 to +25
### Added
- **Task-exit parking for idle connections (c1M P1, `--conn-park-secs`,
default 60 s).** A plain-TCP monoio connection that stays idle past the
W11 downshift now has its handler task exit entirely: only a tiny
readiness watcher (boxed future holding the stream, ~100 B of session
state, and the client-registry guard) remains, reclaiming the ~6 KB task
state machine plus the remaining per-task buffers. The watcher wakes on
read-readiness (`readable(false)`, race-free on io_uring and
epoll/kqueue) or server shutdown and rehydrates a fresh handler through
the migration-restore path — wire-invisible across repeated park/wake
cycles. Parked connections stay in CLIENT LIST, keep their maxclients
slot, and CLIENT KILL still works (its `shutdown(2)` wakes the watcher;
the resumed handler sees EOF). Exclusions: subscriber/tracking/timeout
connections (structurally never reach the park arm), MULTI/EXEC or
cross-store-txn sessions, partial frames, TLS (keeps W11+P4b buffer
downshift), and the tokio runtime. `--conn-park-secs 0` disables.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Duplicate ### Added heading inside [Unreleased].

Line 53 already opens an ### Added section in the same release block (with ### Fixed in between). Fold this entry into the existing ### Added list so changelog tooling and readers see one section per change type.

🤖 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 `@CHANGELOG.md` around lines 9 - 25, Remove the duplicate ### Added heading
around the task-exit parking entry and fold that entry into the existing ###
Added section within [Unreleased], preserving the existing ### Fixed section and
changelog ordering.

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 55 rules

Grey Divider


Remediation recommended

1. Unannotated unwrap() in tests 📘 Rule violation ✧ Quality
Description
tests/parked_idle_parity.rs introduces multiple .unwrap() calls without the required // ...
justification line and adjacent #[allow(clippy::unwrap_used)] attribute in scope. This violates
the unwrap-audit policy and makes it harder to distinguish intentional invariant-based unwraps from
accidental panics.
Code

tests/parked_idle_parity.rs[R77-85]

+        if buf.starts_with(b"$") {
+            if let Some(pos) = buf.iter().position(|&b| b == b'\n') {
+                let len: usize = std::str::from_utf8(&buf[1..pos - 1])
+                    .unwrap()
+                    .trim()
+                    .parse()
+                    .unwrap();
+                if buf.len() >= pos + 1 + len + 2 {
+                    break;
Relevance

●● Moderate

Unwrap-annotation enforcement is mixed: accepted in some PRs, but rejected similar test
unwrap-annotation requests elsewhere.

PR-#71
PR-#217

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 302083 requires every .unwrap() in diffs to be covered by an immediately-adjacent
#[allow(clippy::unwrap_used)] and a one-line justification comment. The new test contains several
.unwrap() calls (e.g., RESP length parsing and string conversions) with no such annotation or
justification.

Rule 302083: Annotate safe unwrap calls with allow and justification
tests/parked_idle_parity.rs[21-34]
tests/parked_idle_parity.rs[77-84]
tests/parked_idle_parity.rs[134-136]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New `.unwrap()` calls were added without the required adjacent `#[allow(clippy::unwrap_used)]` and a one-line justification comment.

## Issue Context
The policy applies to unwraps in both application code and tests. This file has several unwraps (path conversion, RESP parsing, UTF-8 conversions) that need either removal (use `expect`/`match`) or explicit justification + allow.

## Fix Focus Areas
- tests/parked_idle_parity.rs[21-34]
- tests/parked_idle_parity.rs[77-84]
- tests/parked_idle_parity.rs[134-136]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. CLIENT LIST age resets 🐞 Bug ≡ Correctness
Description
On wake from task-exit parking, the watcher deregisters the connection and the resumed handler
re-registers the same client_id, which recreates ClientLiveState timestamps so CLIENT LIST/INFO
age/idle reset on every park/wake cycle despite the TCP connection never reconnecting.
Code

src/shard/conn_accept.rs[R1033-1044]

+        // Wake: the handler re-registers this client_id at entry, so drop
+        // the parked entry first. The deregistered window lasts until the
+        // executor first polls the resumed task (a task-scheduling boundary,
+        // not just a few instructions): a racing CLIENT LIST misses the
+        // conn and CLIENT KILL ID returns 0 — same observable as a
+        // reconnect race, and a kill_flag set in that window is superseded
+        // by the shutdown(2) the killer already issued, which is what woke
+        // us. Closing the gap needs registration handoff into the handler
+        // (pass the guard through instead of drop/re-register); not worth
+        // it for a transient-invisibility race on an actively-waking conn.
+        drop(registry_guard);
+        spawn_resumed_parked_conn(stream, state, conn_ctx, shutdown, client_id, kill_fd);
Relevance

●● Moderate

Semantics change acknowledged in PR comments; no historical precedent found on preserving age/idle
across re-register.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The watcher explicitly deregisters on wake, and the resumed handler path always calls
client_registry::register(), which sets a fresh connected_at. CLIENT LIST derives age/idle
from that timestamp, so the values reset after each park/wake.

src/shard/conn_accept.rs[1033-1045]
src/server/conn/handler_monoio/mod.rs[251-260]
src/client_registry.rs[195-224]
src/client_registry.rs[435-451]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Parked connections are deregistered on wake (`drop(registry_guard)`) and then re-registered by the resumed handler. Because `client_registry::register()` sets `connected_at = Instant::now()` and `connected_at_epoch_ms = current_time_ms()`, the same TCP connection appears “new” after each wake, resetting `CLIENT LIST`/`CLIENT INFO` `age` and distorting `idle`.

### Issue Context
This is user-visible Redis-compat output and can break monitoring/tooling that interprets `age`/`idle` as connection lifetime.

### Fix Focus Areas
- src/shard/conn_accept.rs[1016-1045]
- src/server/conn/handler_monoio/mod.rs[251-260]
- src/client_registry.rs[195-224]
- src/client_registry.rs[435-451]

### Suggested fix direction
Implement a “registry handoff” so the resumed handler does not create a fresh registry entry:
1. Pass the existing registry ownership into the resumed handler instead of `drop(registry_guard)` on wake. (This also eliminates the transient invisibility window.)
2. Update `handle_connection_sharded_monoio` to accept an optional pre-existing registry/live state (or an optional `RegistryGuard` + `Arc<ClientLiveState>`), and when provided, skip `client_registry::register()` and reuse the existing `ClientLiveState` so `connected_at`/`connected_at_epoch_ms` remain stable.

Alternative (if full handoff is too invasive): add a `client_registry::register_with_timestamps(...)` used only by parked-resume to preserve the original `connected_at`/`connected_at_epoch_ms` carried in `MigratedConnectionState`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Guard/FD drop order not enforced 🐞 Bug ☼ Reliability
Description
CLIENT KILL’s raw-fd shutdown safety relies on deregistration happening strictly before the socket
fd closes; the new parked-idle watcher only enforces guard→stream drop order on the explicit
shutdown branch, leaving other watcher teardown paths dependent on implicit drop behavior rather
than an explicit invariant-preserving wrapper.
Code

src/shard/conn_accept.rs[R1016-1031]

+    let fut: std::pin::Pin<Box<dyn std::future::Future<Output = ()>>> = Box::pin(async move {
+        let woke = monoio::select! {
+            res = stream.readable(false) => {
+                // Err (fd error) also resumes: the handler's first read
+                // surfaces the real error and tears down cleanly.
+                let _ = res;
+                true
+            }
+            _ = shutdown.cancelled() => false,
+        };
+        if !woke {
+            // Server shutdown: this watcher owns the close accounting.
+            drop(registry_guard);
+            drop(stream);
+            crate::admin::metrics_setup::record_connection_closed();
+            return;
Relevance

●● Moderate

Invariant seems important, but no close precedent on enforcing drop order structurally for this
watcher teardown.

PR-#95
PR-#72

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The registry code explicitly states it depends on RegistryGuard dropping before the socket closes to
keep kill_fd live while an entry exists. The new watcher takes ownership of both, and only the
shutdown leg explicitly sequences drop(registry_guard) before drop(stream) rather than enforcing
that ordering structurally for all watcher teardown scenarios.

src/client_registry.rs[308-316]
src/shard/conn_accept.rs[1016-1031]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`client_registry::kill_clients` documents a lock-ordering + fd-liveness invariant: while a registry entry exists, its `kill_fd` must still refer to the live socket, which is ensured by dropping the registry guard (deregister) before closing the stream.

With task-exit parking, the stream + RegistryGuard live inside a spawned watcher future. The shutdown branch explicitly drops `registry_guard` before `stream`, but the code does not structurally enforce this ordering for *all* watcher teardown paths (e.g., early drops/unwinds), making the invariant easier to accidentally break.

### Issue Context
If the stream were ever dropped before the registry entry is removed, the registry can temporarily hold a `kill_fd` that has been closed and potentially reused by the OS, which is exactly what the documented invariant is avoiding.

### Fix Focus Areas
- src/shard/conn_accept.rs[1016-1047]
- src/client_registry.rs[308-316]

### Suggested fix direction
Make the guard→stream ordering unconditional by construction:
1. Introduce a small wrapper struct owned by the watcher, e.g.
  ```rust
  struct ParkedOwned {
      guard: RegistryGuard,
      stream: monoio::net::TcpStream,
  }
  ```
  and keep it as a single value inside the watcher future.
2. Ensure the wrapper’s drop order (or a custom `Drop` impl) always deregisters before closing the stream.
3. In the wake path, destructure the wrapper and explicitly `drop(guard)` before handing `stream` to `spawn_resumed_parked_conn`.

This preserves the `kill_fd` liveness contract regardless of how the watcher future terminates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

4. MOON_BIN not enforced 📘 Rule violation ▣ Testability
Description
tests/parked_idle_parity.rs spawns the server via common::find_moon_binary() instead of
requiring MOON_BIN to be set, which violates the integration-test contract for explicitly
selecting the server binary. This can make tests accidentally run against
target/{release,debug}/moon and break reproducibility in CI and local runs.
Code

tests/parked_idle_parity.rs[R21-38]

+fn spawn_moon(dir: &std::path::Path, port: u16) -> std::process::Child {
+    Command::new(common::find_moon_binary())
+        .args([
+            "--port",
+            &port.to_string(),
+            "--shards",
+            "1",
+            "--dir",
+            dir.to_str().unwrap(),
+            "--disk-free-min-pct",
+            "0",
+            "--conn-park-secs",
+            "2",
+        ])
+        .stdout(Stdio::null())
+        .stderr(Stdio::null())
+        .spawn()
+        .expect("spawn moon")
Relevance

● Weak

Explicit MOON_BIN enforcement in tests has a clear rejection precedent; repo keeps
find_moon_binary/env! fallbacks.

PR-#421
PR-#216

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 992389 requires integration tests to set MOON_BIN explicitly and to avoid helpers
that guess a default path. The new test spawns the server using common::find_moon_binary(), and
that helper explicitly falls back to CARGO_BIN_EXE_moon and target/{release,debug}/moon when
MOON_BIN is unset.

Rule 992389: Integration tests must set MOON_BIN explicitly for server binaries
tests/parked_idle_parity.rs[21-38]
tests/common/mod.rs[133-166]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Integration tests must set and use `MOON_BIN` explicitly when spawning the server binary; the new test uses a helper that falls back to `CARGO_BIN_EXE_moon` and `target/{release,debug}/moon`.

## Issue Context
`tests/parked_idle_parity.rs` calls `common::find_moon_binary()` which has non-`MOON_BIN` fallback behavior.

## Fix Focus Areas
- tests/parked_idle_parity.rs[21-38]
- tests/common/mod.rs[133-166]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment on lines +77 to +85
if buf.starts_with(b"$") {
if let Some(pos) = buf.iter().position(|&b| b == b'\n') {
let len: usize = std::str::from_utf8(&buf[1..pos - 1])
.unwrap()
.trim()
.parse()
.unwrap();
if buf.len() >= pos + 1 + len + 2 {
break;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Unannotated unwrap() in tests 📘 Rule violation ✧ Quality

tests/parked_idle_parity.rs introduces multiple .unwrap() calls without the required // ...
justification line and adjacent #[allow(clippy::unwrap_used)] attribute in scope. This violates
the unwrap-audit policy and makes it harder to distinguish intentional invariant-based unwraps from
accidental panics.
Agent Prompt
## Issue description
New `.unwrap()` calls were added without the required adjacent `#[allow(clippy::unwrap_used)]` and a one-line justification comment.

## Issue Context
The policy applies to unwraps in both application code and tests. This file has several unwraps (path conversion, RESP parsing, UTF-8 conversions) that need either removal (use `expect`/`match`) or explicit justification + allow.

## Fix Focus Areas
- tests/parked_idle_parity.rs[21-34]
- tests/parked_idle_parity.rs[77-84]
- tests/parked_idle_parity.rs[134-136]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread src/shard/conn_accept.rs
Comment on lines +1033 to +1044
// Wake: the handler re-registers this client_id at entry, so drop
// the parked entry first. The deregistered window lasts until the
// executor first polls the resumed task (a task-scheduling boundary,
// not just a few instructions): a racing CLIENT LIST misses the
// conn and CLIENT KILL ID returns 0 — same observable as a
// reconnect race, and a kill_flag set in that window is superseded
// by the shutdown(2) the killer already issued, which is what woke
// us. Closing the gap needs registration handoff into the handler
// (pass the guard through instead of drop/re-register); not worth
// it for a transient-invisibility race on an actively-waking conn.
drop(registry_guard);
spawn_resumed_parked_conn(stream, state, conn_ctx, shutdown, client_id, kill_fd);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

3. Client list age resets 🐞 Bug ≡ Correctness

On wake from task-exit parking, the watcher deregisters the connection and the resumed handler
re-registers the same client_id, which recreates ClientLiveState timestamps so CLIENT LIST/INFO
age/idle reset on every park/wake cycle despite the TCP connection never reconnecting.
Agent Prompt
### Issue description
Parked connections are deregistered on wake (`drop(registry_guard)`) and then re-registered by the resumed handler. Because `client_registry::register()` sets `connected_at = Instant::now()` and `connected_at_epoch_ms = current_time_ms()`, the same TCP connection appears “new” after each wake, resetting `CLIENT LIST`/`CLIENT INFO` `age` and distorting `idle`.

### Issue Context
This is user-visible Redis-compat output and can break monitoring/tooling that interprets `age`/`idle` as connection lifetime.

### Fix Focus Areas
- src/shard/conn_accept.rs[1016-1045]
- src/server/conn/handler_monoio/mod.rs[251-260]
- src/client_registry.rs[195-224]
- src/client_registry.rs[435-451]

### Suggested fix direction
Implement a “registry handoff” so the resumed handler does not create a fresh registry entry:
1. Pass the existing registry ownership into the resumed handler instead of `drop(registry_guard)` on wake. (This also eliminates the transient invisibility window.)
2. Update `handle_connection_sharded_monoio` to accept an optional pre-existing registry/live state (or an optional `RegistryGuard` + `Arc<ClientLiveState>`), and when provided, skip `client_registry::register()` and reuse the existing `ClientLiveState` so `connected_at`/`connected_at_epoch_ms` remain stable.

Alternative (if full handoff is too invasive): add a `client_registry::register_with_timestamps(...)` used only by parked-resume to preserve the original `connected_at`/`connected_at_epoch_ms` carried in `MigratedConnectionState`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread src/shard/conn_accept.rs
Comment on lines +1016 to +1031
let fut: std::pin::Pin<Box<dyn std::future::Future<Output = ()>>> = Box::pin(async move {
let woke = monoio::select! {
res = stream.readable(false) => {
// Err (fd error) also resumes: the handler's first read
// surfaces the real error and tears down cleanly.
let _ = res;
true
}
_ = shutdown.cancelled() => false,
};
if !woke {
// Server shutdown: this watcher owns the close accounting.
drop(registry_guard);
drop(stream);
crate::admin::metrics_setup::record_connection_closed();
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

4. Guard/fd drop order not enforced 🐞 Bug ☼ Reliability

CLIENT KILL’s raw-fd shutdown safety relies on deregistration happening strictly before the socket
fd closes; the new parked-idle watcher only enforces guard→stream drop order on the explicit
shutdown branch, leaving other watcher teardown paths dependent on implicit drop behavior rather
than an explicit invariant-preserving wrapper.
Agent Prompt
### Issue description
`client_registry::kill_clients` documents a lock-ordering + fd-liveness invariant: while a registry entry exists, its `kill_fd` must still refer to the live socket, which is ensured by dropping the registry guard (deregister) before closing the stream.

With task-exit parking, the stream + RegistryGuard live inside a spawned watcher future. The shutdown branch explicitly drops `registry_guard` before `stream`, but the code does not structurally enforce this ordering for *all* watcher teardown paths (e.g., early drops/unwinds), making the invariant easier to accidentally break.

### Issue Context
If the stream were ever dropped before the registry entry is removed, the registry can temporarily hold a `kill_fd` that has been closed and potentially reused by the OS, which is exactly what the documented invariant is avoiding.

### Fix Focus Areas
- src/shard/conn_accept.rs[1016-1047]
- src/client_registry.rs[308-316]

### Suggested fix direction
Make the guard→stream ordering unconditional by construction:
1. Introduce a small wrapper struct owned by the watcher, e.g.
   ```rust
   struct ParkedOwned {
       guard: RegistryGuard,
       stream: monoio::net::TcpStream,
   }
   ```
   and keep it as a single value inside the watcher future.
2. Ensure the wrapper’s drop order (or a custom `Drop` impl) always deregisters before closing the stream.
3. In the wake path, destructure the wrapper and explicitly `drop(guard)` before handing `stream` to `spawn_resumed_parked_conn`.

This preserves the `kill_fd` liveness contract regardless of how the watcher future terminates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@TinDang97
TinDang97 merged commit d832452 into main Jul 29, 2026
14 checks passed
@TinDang97
TinDang97 deleted the feat/c1m-parked-idle branch July 29, 2026 14:39
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
TinDang97 added a commit that referenced this pull request Jul 29, 2026
…ration handoff, ECANCELED spin fix (#424)

* fix(server): registration handoff across park/wake + migrated-conn parking (c1M P1 follow-ups)

Two follow-ups from the round-5 task-exit-parking review:

1. Registration handoff (closes the review's Low finding, and a worse
   bug found while fixing it): waking a parked connection deregistered
   then re-registered the client across a task-scheduling boundary.
   Beyond the transient CLIENT LIST invisibility / KILL-returns-0
   window, the fresh registration silently RESET the connection's
   CLIENT SETNAME and age (register() inserts name: None; only the
   SETNAME dispatch path ever sets it). The watcher now passes the held
   RegistryGuard through spawn_resumed_parked_conn into the handler
   (ParkArgs::kept_registration); the handler reuses the live entry via
   the new client_registry::live_handle() instead of re-registering.
   The entry — name, connected_at, kill state, TOTAL_CLIENTS/shard
   counters — persists unbroken across any number of park/wake cycles;
   there is no deregistered window at all.
   The can_park bool param becomes ParkArgs {can_park,
   kept_registration} with NO_PARK/PARK consts at the six monoio call
   sites.

2. Migrated-spawn site routes ParkIdle: a connection that migrated
   shards (Linux) and then idled past --conn-park-secs now parks like a
   primary-accept connection instead of holding its handler task
   forever. Mechanical mirror of the reviewed plain-TCP routing (same
   watcher, same close-accounting transfer). No dedicated migration
   integration harness exists (migration is affinity-driven,
   Linux-only); covered by compile gates + the shared watcher paths.

Red/green: new tests/parked_idle_parity.rs test
resumed_connection_keeps_registry_identity (SETNAME → 2× park/data-wake
cycles → name+id survive, CLIENT LIST holds exactly victim+control) was
RED before (name= empty, age=0 after wake), GREEN after.

Gates: parked parity 4/4 on kqueue AND io_uring (VM); TLS parity green;
VM monoio lib 4466 pass; VM tokio lib 3628 pass; fmt; clippy
--all-targets both matrices 0 lints.

refs: PR #422 review Low, .planning/rfcs/c1m-connection-plane.md
author: Tin Dang

* feat(server): TLS task-exit parking — idle TLS 47.0 → 26.0 KB/conn (c1M P1-TLS)

Extends round-5 task-exit parking to TLS connections, closing the last
c1M follow-up. No new unsafe; vendored additions are moon-patch style.

Vendored (monoio-rustls + monoio-io-wrapper):
- Stream::io_ref() — raw-transport reference so the parked watcher can
  await readable(false) on the underlying fd through the TLS wrapper.
- Stream::task_park_safe() — park-safety veto: refuses while the TLS
  stack holds ANYTHING the raw fd's readability cannot signal (wrapper
  buffer bytes or pending EOF/error status via the new
  ReadBuffer/WriteBuffer::is_drained(), decrypted plaintext, a received
  close_notify, or pending session output). A partial record in the
  deframer is park-safe: completing it requires more socket bytes.
- SafeRead/SafeWrite::is_drained() with moon-patch unit tests (7/7).

moon:
- IdleParkRead: TLS sets SUPPORTS_TASK_PARK=true; new per-park
  task_park_safe() hook (plain TCP: always true) appended LAST in the
  parkable predicate.
- conn_accept: ParkWatchable trait (park_readable()) — TcpStream awaits
  its own readable(false), TLS awaits io_ref().readable(false); watcher +
  resume helpers genericized over the stream; the monoio-TLS accept site
  routes ParkIdle like the plain site.

Spin bug found by E11 and fixed (affects plain TCP too):
- The idle-park arms treated EVERY read error as the sweep cancel. With
  task parking, a dead connection whose error leaves the fd permanently
  readable (TLS client FIN without close_notify => read ERROR; plain RST)
  spun park→wake→park at 100% CPU forever — E11's first ON leg ended
  with 3000 CLOSE_WAIT conns and a pinned shard. The arms now match
  monoio's exact cancel error (raw os 125 on BOTH drivers — uring kernel
  -ECANCELED, legacy hardcodes 125): only the sweep cancel
  downshifts/parks, real errors tear down promptly (idle_park::
  is_sweep_cancel). Small-N repro: fds released, 0 CLOSE_WAIT, 0% CPU.

Measured (E11, moon-dev VM, 3000 idle TLS conns, shards=2, same-binary
flag A/B, tmp/c10k/e11_tls_park_rss.sh):
- TLSPARK_OFF (P4b downshift only): 47.0 KB/conn idle
- TLSPARK_ON: 26.0 KB/conn idle (−45%); re-parks at 31.4 after a full
  wake sweep; sweeps bad=0 both legs; RSS + fds fully return on close
- Remaining TLS floor ≈ rustls session (~15-20 KB, not shrinkable via
  public API) + registry entry + kernel socket

Tests:
- tls_idle_downshift_parity: +tls_parked_connection_serves_traffic_and_
  stays_killable (3 park/wake cycles incl. 100-pipeline on ONE session;
  CLIENT LIST shows the parked conn WITH its name via registration
  handoff; CLIENT KILL closes it), +tls_fin_while_parked_tears_down_
  promptly (the spin regression, TLS leg)
- parked_idle_parity: +rst_while_parked_tears_down_promptly (spin
  regression, plain-TCP RST leg)
- All parity suites green on kqueue AND io_uring (plain 5/5, TLS 3/3)

Gates: VM monoio+tokio lib suites pass; E10 plain-park sanity re-run
green; fmt; clippy --all-targets both matrices 0 lints; vendor wrapper
moon-patch tests 7/7.

refs: .planning/rfcs/c1m-connection-plane.md (P1-TLS follow-up)
author: Tin Dang

* fix(server): PR #424 review fixes — REPLCONF park exclusion, SAFETY comment, expect-free is_drained

CodeRabbit review of PR #424, all three findings addressed:

1. (Major) PSYNC-after-park closed the replica connection: connections
   that have issued REPLCONF (replica mid-handshake, PSYNC next) are now
   permanently excluded from task-parking via a sticky
   ConnectionState::saw_replconf flag checked in the parkable predicate —
   the unsupported HijackForPsync warn+close arm on the resumed path is
   now unreachable for real replicas. (A manual PSYNC with no prior
   REPLCONF after 60s of idle still lands on the loud warn+close and the
   replica's reconnect loop recovers — documented.)
2. (Minor) SO_LINGER unsafe block in rst_while_parked test gained its
   required // SAFETY: comment.
3. (Minor) Vendored SafeRead/SafeWrite::is_drained() are expect-free:
   a transiently-absent buffer answers false ("don't park" is always the
   safe verdict) instead of panicking.

Gates: parity suites green kqueue (plain 5/5, TLS 3/3); vendor wrapper
7/7; fmt; clippy --all-targets both matrices 0 lints.

refs: PR #424 review threads
author: Tin Dang
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant