fix(replication): seed shard-0 offset alongside master_repl_offset on AOF recovery (task #67) - #329
Merged
Conversation
… AOF recovery (task #67) After a multi-shard master (--shards >= 2) was kill -9'd and restarted with prior write history, the surviving replica kept streaming and applying writes correctly, but WAIT 1 <timeout> on the restarted master timed out for minutes — REPLCONF ACK kept arriving every second but never registered as "caught up". Reproduced 2x by the v0.7.0 replication soak (scripts/soak-replication-24h.sh, PR #327). Root cause: ReplicationState::seed_master_offset (AOF recovery, RFC §2 Rule 3) seeded ONLY the process-wide master_repl_offset from the recovered max LSN, leaving every per-shard shard_offsets[i] at the fresh-boot 0. handle_psync_inline_multi_shard's full-resync handshake advertises Σ shard_offset(i) — not total_offset() — as a reconnecting replica's new baseline, because each shard captures its own offset atomically with its RDB body (the invariant the exactly-once live-fanout `cut` gate depends on). A replica reconnecting post-restart therefore adopted a near-zero baseline while wait_for_replicas kept comparing ACKs against the correctly-seeded (large) total_offset() — a gap the replica could never close, since both axes only ever grow. The data plane was unaffected: the per-shard `cut`/end_offset filtering that guarantees exactly-once delivery never references total_offset(), only each shard's own counter. Fix: seed_master_offset now also seeds shard 0 to the same recovered value, restoring the Σ shard_offsets == total_offset() invariant every write already maintains going forward (increment_shard_offset/issue_lsn bump both axes by the same delta in lockstep). The exact per-shard split of the seed doesn't affect correctness — each shard's counter is only ever compared against itself — so concentrating it on shard 0 is sufficient and avoids a real double-count hazard (per-shard AOF replay only tracks each shard's max *global* LSN tag, not its own cumulative byte length, so seeding every shard from that tag would over-count the sum by roughly Nx). Added tests/replication_hardening.rs::master_kill_restart_wait_acks (RED on unmodified code — WAIT returned 0 with replica ACK lag stuck at ~2050 bytes behind a freshly-seeded ~2134-byte master offset; GREEN 5x consecutively after the fix) plus two state.rs unit tests pinning the seed invariant and its fetch_max never-regress semantics. Local gates green: fmt, clippy (default + runtime-tokio,jemalloc), full replication_hardening + replication_multishard suites, replication:: unit tests under both runtimes. author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
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
Fixes the P0 found by the soak smoke (PR #327): after a master kill-9 + restart, the surviving replica kept replicating but
WAITtimed out for the rest of the window.Root cause:
ReplicationState::seed_master_offset(AOF recovery) seeded onlymaster_repl_offset, leavingshard_offsets[]at 0. The multi-shard PSYNC handshake advertisesΣ shard_offset(i)as the reconnecting replica's baseline, whilewait_for_replicascompares ACKs againsttotal_offset()— with the axes divergent post-restart, the replica's ACK offset could never reach the target.Fix: seed shard 0 to the same recovered value (
fetch_maxon both axes), restoring theΣ shard_offsets == total_offset()invariant at boot. The per-shard split doesn't affect correctness — each shard's offset is only compared against itself (cut gate + backlog are shard-scoped); splitting proportionally isn't even possible from what AOF replay retains, and seeding all shards near the global max would genuinely double-count. Comment-only clarification inmaster.rs.Verification
tests/replication_hardening.rs::master_kill_restart_wait_acksreproduced the wedge on unmodified code (WAIT=0, ACK stuck ~2050 vs master ~2134).replication_hardening(6/6, re-verified post-rebase ×2) +replication_multishard(9/9); unit tests both runtimes; fmt + both clippy matrices clean.fetch_maxnever regresses either axis.SOAK-PASS cycles=3 acked=1798 inflight=1 master_kills=2 replica_kills=1— vsacked=493 inflight=429pre-fix. The wedge is gone; zero acked-write loss.Unblocks the 24h soak (task #62) gating the v0.7.0 tag.