fix(metrics): connection counter double-decrement on monoio path#72
Merged
Conversation
handle_connection_sharded_monoio called record_connection_closed() at its exit, while conn_accept.rs (the caller) ALSO calls it in the non- migration branch (line 627). The AtomicU64 counter wrapped from 0 to u64::MAX on the second fetch_sub, causing every subsequent try_accept_connection to reject against maxclients. Symptom: first connection succeeds, all subsequent connections rejected with "maxclients reached" — even though CONFIG GET maxclients returns 10000 and the original value was never reached. Repro (before fix): ./target/release/moon --port 6400 --shards 1 --appendonly no & redis-cli -p 6400 SET foo bar # ✓ OK redis-cli -p 6400 SET foo bar # ✗ Connection reset by peer Fix: remove the handler-level decrement. The comment at line 84 already documents that the caller owns the increment via try_accept_connection; by symmetry the caller owns the decrement (conn_accept.rs:547 for TLS, conn_accept.rs:627 for plain TCP non-migrated path). Migration path counter accounting is a separate concern (already imbalanced) and is not addressed here. Verified on aarch64 Linux (OrbStack moon-dev): - 10 sequential SETs all succeed - INFO clients reports connected_clients:1 (just the probe) - redis-benchmark SET p=16 c=50 n=10000 → 1.25M req/s (real number) Blocks: PR #71 perf recovery — cannot measure real throughput without this fix. Once merged, PR #71 can be validated with bench-compare.sh.
Review Summary by QodoFix connection counter double-decrement on monoio path
WalkthroughsDescription• Remove duplicate record_connection_closed() call in monoio handler • Fixes counter wraparound causing all connections rejected after first • Restores symmetry: caller owns both increment and decrement • Verified: sequential SETs succeed, real throughput restored (1.25M req/s) Diagramflowchart LR
A["try_accept_connection<br/>increments counter"] -->|caller owns| B["conn_accept.rs<br/>decrements on close"]
C["handle_connection_sharded_monoio<br/>REMOVED duplicate decrement"] -.->|was breaking| D["AtomicU64 wraps<br/>to u64::MAX"]
D -->|causes| E["maxclients check<br/>always rejects"]
B -->|fix restores| F["Counter stays valid<br/>connections accepted"]
File Changes1. src/server/conn/handler_monoio.rs
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughA bug fix addressing connection counter underflow: removed double-decrementing of the shared Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Accept as conn_accept.rs (caller)
participant Handler as handler_monoio.rs
participant Target as spawn_migrated_monoio_connection
participant Metrics as record_connection_closed()
Client->>Accept: connect / try_accept_connection
Accept->>Handler: spawn handler (monoio)
alt non-migration path
Handler-->>Accept: handler completes
Accept->>Metrics: record_connection_closed() %% caller decrements on normal close
else migration path
Handler-->>Target: migrate connection (spawn on target shard)
Target->>Handler: run migrated handler future
Target-->>Metrics: record_connection_closed() %% target decrements after migrated handler finishes
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+2330
to
2336
| // NOTE: connection close is recorded by the caller (conn_accept.rs) to | ||
| // preserve symmetry with `try_accept_connection`, which owns the | ||
| // increment. Decrementing here too produces a double-decrement on the | ||
| // AtomicU64 counter — it wraps to u64::MAX on the second subtraction | ||
| // and all subsequent `try_accept_connection` comparisons against | ||
| // `maxclients` reject new connections. | ||
| (MonoioHandlerResult::Done, None) |
There was a problem hiding this comment.
1. Migrated clients never decrement 🐞 Bug ≡ Correctness
After this PR, monoio connections that migrate to another shard will no longer call record_connection_closed() when they finally disconnect on the target shard, so CONNECTED_CLIENTS leaks upward. Over time this will incorrectly trip try_accept_connection(maxclients) and reject new connections even when no clients are actually connected.
Agent Prompt
## Issue description
`handle_connection_sharded_monoio()` no longer calls `record_connection_closed()`. For migrated monoio connections, the source shard correctly skips decrementing (connection stays alive), but the target shard’s migrated handler spawn also does not decrement after the handler finishes. This leaks `CONNECTED_CLIENTS`, eventually causing erroneous `maxclients` rejections.
## Issue Context
- Source shard: increments via `try_accept_connection(maxclients)`.
- Source shard: skips decrement when `_migrated == true`.
- Target shard: `spawn_migrated_monoio_connection()` spawns `handle_connection_sharded_monoio(..., Some(&state))` but does not call `record_connection_closed()` afterward.
- After this PR, there is no remaining place that decrements for migrated connections on final disconnect.
## Fix Focus Areas
- src/shard/conn_accept.rs[640-778]
- src/server/conn/handler_monoio.rs[2289-2337]
### Suggested implementation direction
Add `crate::admin::metrics_setup::record_connection_closed();` after awaiting `handle_connection_sharded_monoio()` in the `monoio::spawn` block inside `spawn_migrated_monoio_connection()`.
Optionally, update the comment in `handler_monoio.rs` to clarify that the caller owns close accounting for fresh accepts, but migrated-connection spawns must still decrement on final close.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
10 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/conn/handler_monoio.rs`:
- Around line 2330-2335: In the migration/resumption branch where
handle_connection_sharded_monoio(...).await is invoked with can_migrate: false,
add a call to record_connection_closed() immediately after the await completes
so the connection-count decrement mirrors the plain-TCP path (see the existing
pattern around the plain TCP handler). This prevents leaked connected_clients
for resumed/migrated connections by ensuring the counter is decremented when the
handler returns Done.
🪄 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
Run ID: 3bc90056-84a7-4420-be78-9ec447786152
📒 Files selected for processing (2)
CHANGELOG.mdsrc/server/conn/handler_monoio.rs
TinDang97
added a commit
that referenced
this pull request
Apr 12, 2026
The initial inline SET used Bytes::copy_from_slice for key and value,
which triggers MALLOC+memcpy twice per SET. The Frame-based path
achieves zero-copy via Bytes::slice on a frozen BytesMut, which is
just an Arc refcount bump. This disparity caused a ~4-7% SET p=1
regression because the inline-path savings were outweighed by the
new allocations at low pipeline depth.
Fix: call read_buf.split_to(consumed).freeze() once, then slice() the
frozen Bytes for key, value, and AOF. All three are now Arc refcount
bumps over the same underlying allocation — zero malloc, zero memcpy.
Measured impact on aarch64 Linux (OrbStack, 1 shard, 50 clients):
before nocopy after nocopy delta
SET p=16: 2.94M rps 3.11M rps +5.4% (peak 3.60M)
SET p=1: 237K rps 235K rps -1% (within noise)
GET p=16: 3.32M rps 3.33M rps +0.3%
PR #71 totals vs origin/main (both with PR #72 maxclients fix):
SET p=16: 2.43M → 3.11M = +28% (peak +48%)
SET p=1: 241K → 235K = -2.5% (was -4%, noise-level now)
GET p=16: 3.36M → 3.33M = ±0%
All 11 inline-dispatch tests pass unchanged.
When a connection migrates to another shard, the source wrapper skips the decrement (connection stays alive) and the target shard's spawn_migrated_monoio_connection() now owns the balancing decrement on final close. Without this, every migration leaked CONNECTED_CLIENTS upward and eventually tripped maxclients on long-running clusters. Addresses review feedback from qodo-code-review and coderabbit on PR #72 — both bots flagged this exact leak.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shard/conn_accept.rs`:
- Around line 768-773: The migrated-connection decrement is only done after
handle_connection_sharded_monoio() returns, so if target-side setup fails
earlier the source-side skipped decrement (because _migrated == true) and
CONNECTED_CLIENTS leaks; fix by ensuring record_connection_closed() (or the
CONNECTED_CLIENTS decrement) is invoked on all early error/return paths when
_migrated == true — e.g., add a short-lived guard in the conn_accept setup code
(or explicit calls) so that any early return before
handle_connection_sharded_monoio() will call
crate::admin::metrics_setup::record_connection_closed(); keep transfer semantics
so that when ownership is successfully transferred the guard is disarmed and no
double-decrement occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
pilotspacex-byte
enabled auto-merge
April 12, 2026 07:13
coderabbit flagged two leak paths in spawn_migrated_monoio_connection: 1. set_nonblocking(true) failure on the reconstructed std stream 2. monoio::net::TcpStream::from_std(...) Err branch Both return without running the handler. The source shard already skipped its wrapper decrement on successful hand-off (_migrated == true), so the counter was +1 with no balancing decrement. Add record_connection_closed() on each early-error return. The happy path's decrement after handler.await still works unchanged.
Contributor
Author
Review feedback addressedqodo-code-review (1 bug): ✅ Fixed in 4b5e1e9 — coderabbitai (2 leak paths on migration errors): ✅ Fixed in 320b780 — All three counter-leak exit paths now balanced. Source shard owns increment via |
Contributor
Author
CI flake — re-running
Evidence this is not our change: this PR only touches
|
TinDang97
added a commit
that referenced
this pull request
Apr 12, 2026
The initial inline SET used Bytes::copy_from_slice for key and value,
which triggers MALLOC+memcpy twice per SET. The Frame-based path
achieves zero-copy via Bytes::slice on a frozen BytesMut, which is
just an Arc refcount bump. This disparity caused a ~4-7% SET p=1
regression because the inline-path savings were outweighed by the
new allocations at low pipeline depth.
Fix: call read_buf.split_to(consumed).freeze() once, then slice() the
frozen Bytes for key, value, and AOF. All three are now Arc refcount
bumps over the same underlying allocation — zero malloc, zero memcpy.
Measured impact on aarch64 Linux (OrbStack, 1 shard, 50 clients):
before nocopy after nocopy delta
SET p=16: 2.94M rps 3.11M rps +5.4% (peak 3.60M)
SET p=1: 237K rps 235K rps -1% (within noise)
GET p=16: 3.32M rps 3.33M rps +0.3%
PR #71 totals vs origin/main (both with PR #72 maxclients fix):
SET p=16: 2.43M → 3.11M = +28% (peak +48%)
SET p=1: 241K → 235K = -2.5% (was -4%, noise-level now)
GET p=16: 3.36M → 3.33M = ±0%
All 11 inline-dispatch tests pass unchanged.
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 14, 2026
…NOUT_HINT probe fix + dead PSYNC path deletion (tasks #70, #72) (#332) * perf(replication): parking_lot fanout-gate migration + FANOUT_HINT fix; chore(replication): delete dead master PSYNC path (tasks #70, #72) Two pre-soak defects blocking the v0.7.0 tag, fixed together since both touch ReplicationState locking on the per-write hot path. Task #70 — per-write replication fanout gate perf debt (4 sub-fixes): 1. Migrated ReplicationState's outer lock from std::sync::RwLock to parking_lot::RwLock across 27 call sites (admin/metrics_setup.rs, cluster/gossip.rs, command/server_admin.rs, main.rs, persistence/aof/pool.rs, replication/{reason_del,replica,master, state}.rs, scripting/bridge.rs, server/conn/*, shard/*). The inner per_shard_backlogs Mutex was already parking_lot; the outer lock was the odd one out, forcing .read().unwrap() / if-let-Ok poisoning dances on the write path. parking_lot doesn't poison: read()/write() return the guard directly, try_read()/try_write() return Option not Result (fixed 3 sites that assumed Result). 2. ensure_backlogs_allocated() — called from try_handle_replconf on ANY bare REPLCONF, including probes and failed handshakes — used to call mark_fanout_active() unconditionally. FANOUT_HINT is a sticky, never-cleared process-global AtomicBool, so one stray REPLCONF permanently taxed every write with the fanout-active gate check even on a server that never completes PSYNC. Split allocation (still load-bearing, kept in ensure_backlogs_allocated) from hint activation (moved to try_handle_psync, on an actual PSYNC/replica registration). 3. record_local_write / record_local_write_db collapsed from up to 3-4 separate repl_state.read() acquisitions per write down to ONE guard, taken at the top of record_local_write_db and threaded through. replication_fanout_active stays a separate short-lived gate lock by design: some external call sites (handler_monoio/mod.rs MOVE/COPY) reach an .await between the gate check and the next repl-state touch, and Rust drops guards at end of lexical scope (not last-use), so folding it into one long-lived guard would hold a lock across .await. Net: at most 2 acquisitions per write (1 gate + 1 write), down from 4-5. 4. Added #[inline] to record_local_write, record_local_write_db, and replication_fanout_active, matching the try_handle_* convention. Task #72 — delete dead master PSYNC path: handle_psync_on_master (both tokio and monoio variants) and register_replica_with_shards in src/replication/master.rs were unreachable — every live PSYNC handler goes through handle_psync_inline_single_shard / handle_psync_inline_multi_shard from shard/conn_accept.rs instead. The dead pair also had a latent broken WAIT implementation: it initialized ack_offsets but never spawned ack_read_loop to drain replica ACKs into them, so wait_for_replicas against that path would block forever. Deleted both variants plus evaluate_psync_shared (only reachable from the deleted code); kept backlog_bytes_from, still used by the live send_backlog_range. Testing: - Two new unit tests in replication::state::tests cover the FANOUT_HINT fix with delta-based assertions (robust against shared-binary test-order contamination): test_ensure_backlogs_allocated_does_not_activate_fanout_hint (RED on the pre-fix code, verified by temporarily reintroducing the bare mark_fanout_active() call) and test_mark_fanout_active_sets_hint. - Lock-across-await audit: every touched call site was checked by reading the actual code, not just grepping. The one risk found (handler_monoio/mod.rs MOVE/COPY awaiting pool.send_append_group() after the fanout gate check) is why replication_fanout_active was kept as its own short-lived guard instead of being folded into the record_local_write_db guard. No parking_lot guard is held across an .await point anywhere in the touched code. - fmt clean; clippy -D warnings clean on both default (monoio) and --no-default-features --features runtime-tokio,jemalloc. - Full cargo test --lib green on both feature sets (4303 passed / 1 ignored monoio; 3484 passed / 1 ignored tokio). - replication_hardening (6/6), replication_multishard (9/9), replication_planes (11/11) integration suites green, MOON_BIN pinned to a fresh release-fast monoio binary, --ignored --test-threads=1. replication_test has 0 tests (empty suite, pre-existing). replication:: unit-test module green under both runtimes (67/67 monoio-filtered, full set included in the --lib totals above) — master-side PSYNC (and thus the suites above) is monoio-only by pre-existing, unmodified design (try_handle_psync_unsupported under tokio); the "both runtimes" gate is satisfied at the unit-test level. - Rebased onto origin/main (108a01e) to pick up PR #329's seed_master_offset shard-0 seeding fix, which touched the same two files (master.rs, state.rs); auto-merged cleanly with no conflict markers, re-verified by re-running every gate above post-rebase. No wire-protocol or observable behavior change beyond the hint gating: REPLCONF -> PSYNC handshake sequence is unchanged. author: Tin Dang * fix(replication): realign skewed backlogs at activation (review-caught regression on task #70) Orchestrator review of eb4d47b3 found a correctness regression in the FANOUT_HINT fix (task #70.2). Splitting backlog allocation from hint activation opened a REPLCONF -> PSYNC window during which a bare REPLCONF could allocate + seed a shard's ReplicationBacklog at its offset at that instant, then -- with FANOUT_HINT still false -- every local write in the window advanced the shard's real offset counter via the bare-issue_lsn branch with NO matching backlog append (the KV wal_append_and_fanout path is unaffected; only the handler-inline issue_append_lsn branch skews). The backlog's end_offset silently drifted stale relative to the real counter for as long as the window stayed open -- an AOF-enabled master under continuous write load with periodic replica kill-9/reconnect, exactly the v0.7.0 24h soak's shape (shards=4, appendfsync always, reconnects every 12min). Any snapshot/cut/push-offset captured from that backlog after activation would be range-inconsistent with it: FULLRESYNC catch-up and partial resync could then read wrong bytes, or silently skip catch-up (bytes_from returning None for an offset the backlog didn't yet know about) -- acked-write loss / replica corruption. Fix: restore the invariant "backlog byte positions are trustworthy from the last activation realign onward" by realigning already-allocated backlogs at every activation site, before any snapshot/cut/push offset is captured from them. 1. ReplicationBacklog::realign_to(offset) (src/replication/backlog.rs): resets the buffer to empty, reseeded at offset -- the same state as new_at(capacity, offset). No-op if already aligned. A skewed buffer's contents are already useless (they cover positions the real counter never confirmed), so dropping them is correct: any partial-resync request for an offset below the new start_offset falls through bytes_from returning None, and the caller's existing fail-safe -- full resync -- kicks in. Never serves wrong bytes. 2. ReplicationState::realign_backlog(shard_id) (src/replication/state.rs): reads the shard's current real offset and calls realign_to on that shard's backlog if allocated. Race-free only when called from the shard thread that owns shard_id's own offset advances -- true for every call site below. 3. Wired into try_handle_psync (src/server/conn/handler_monoio/dispatch.rs), right after mark_fanout_active(): realigns ctx.shard_id's backlog. Race-free at shards=1, where the connection task IS the owning shard's own event-loop thread (documented in the code -- this call does NOT cover other shards on a multi-shard master; that's handled by point 4 below, which runs per-shard on each shard's own thread). 4. Wired into the RegisterReplica and PrepareReplicaSync arms (src/shard/spsc_handler.rs): both arms already had an `if guard.is_none() { allocate }` pattern; changed to `match guard.as_mut() { None => allocate, Some(backlog) => realign }` so an already-allocated backlog is realigned instead of silently left skewed, before cut/push_offset/shard_offset are captured further down in the same arm. Testing (RED/GREEN): - backlog.rs: test_realign_to_resets_skewed_backlog, test_realign_to_noop_when_already_aligned -- low-level buffer-reset behavior of realign_to itself. - state.rs: test_realign_backlog_fixes_skew_after_hint_false_window -- reproduces the exact bug: ensure_backlogs_allocated (REPLCONF), then issue_lsn(shard, 100/150/50) with no append (the hint-false window), asserts the skew is real (backlog end_offset stuck at 0 while the real offset is 300, and bytes_from(300) returns None on the unrealigned backlog), then calls realign_backlog and asserts end_offset now matches the real offset and bytes_from(300) returns Some(empty). test_realign_backlog_then_append_reads_back_exact_record is the integration-shaped companion: allocate, issue_lsn, activate, append one record via the normal backlog.append + increment_shard_offset pairing, assert bytes_from(pre-append offset) returns exactly that record's bytes. - RED verified by temporarily neutering realign_backlog's body (#[cfg(any())] stub) and confirming both new state.rs tests fail (end_offset stuck at 0 instead of 300; the post-append record unreadable at its own pre-append offset), then reverted. - GREEN: all 4 new tests pass, plus the full replication:: unit module (71/71) and full cargo test --lib (4307/4307 monoio, 3490/3490 tokio, both 0 failed). Gates: - fmt clean. - clippy -D warnings clean, both default (monoio) and --no-default-features --features runtime-tokio,jemalloc. - cargo test --lib green both feature sets (4307 passed / 1 ignored monoio; 3490 passed / 1 ignored tokio; 0 failed either). - replication_hardening (6/6), replication_multishard (9/9), replication_planes (11/11) integration suites green against a fresh MOON_BIN built from this fix, --ignored --test-threads=1. No VM A/B re-run: the realign call sites are activation-time-only (PSYNC arrival, replica registration), off the per-write hot path the prior A/B measured -- perf surface is unchanged by this fix. author: Tin Dang --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
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
2 participants
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
handle_connection_sharded_monoioandconn_accept.rsboth callrecord_connection_closed()on disconnect, producing a doublefetch_subon theAtomicU64counter. On the first disconnect the counter wraps0 → u64::MAX, which exceedsmaxclientson every subsequenttry_accept_connection— all new connections are rejected until server restart.Reproduction (before fix)
Server log floods with:
Even though
CONFIG GET maxclientsreturns10000.Root cause
src/server/conn/handler_monoio.rs:2330:While
src/shard/conn_accept.rs:624-628also decrements (correctly guarded for the non-migration case):The comment at
handler_monoio.rs:84already documents the intended ownership:By symmetry, the caller should also own the decrement. Removing the handler-level call restores that symmetry.
Fix
Single-line removal:
record_connection_closed()call athandler_monoio.rs:2330.Verification (aarch64 Linux / OrbStack moon-dev)
Before the fix, the bench produced
SET: rps=0.0+Error: Server closed the connection.Scope
Fixes only the monoio path. The Tokio TLS path (
conn_accept.rs:240+handler_sharded.rs:176) has an analogous double-decrement — follow-up PR. The migration path's counter accounting is imbalanced in the opposite direction and is also a separate fix.Blocks
PR #71 (perf recovery) — any throughput claim on monoio aarch64 needs this fix to produce real benchmark numbers.
Test plan
cargo clippy -- -D warningscleanscripts/audit-unsafe.shpasses (178/178 SAFETY comments)redis-benchmark SET p=16 c=50 n=10000returns real throughput (1.25M rps on OrbStack aarch64)INFO clientsshows correctconnected_clients(no wraparound)Summary by CodeRabbit
--maxclients.