fix(shard): multi-shard SWAPDB — durable + replica-visible before +OK (#133)#385
Conversation
…#133) coordinate_swapdb had two defects. Defect 1 (the issue's report): no fsync rendezvous anywhere — the local leg fired its record into the generic wal_append_txs channel and remote legs acked right after enqueueing, so under --appendfsync always the client saw +OK before any shard had fsynced. Defect 2 (found during scoping, empirically confirmed): the local leg's channel drains into WAL v3 ONLY, but for --shards >= 2 --appendonly yes recovery wipes every shard and replays ONLY the per-shard AOF manifest — the coordinator shard's own SWAPDB record never reached the plane recovery reads. A kill-9 after +OK permanently lost the LOCAL shard's half of the swap while remote shards' halves survived: cross-shard keyspace divergence, strictly worse than a fsync-timing gap. The local leg also never reached the replication plane (no backlog append, no offset advance, no live fan-out), unlike remote legs. Fix: - Local leg: durable AOF append via send_append_group (the same v3-5 group-commit primitive persist_local_leg uses) + one fsync_barrier under Always, BEFORE the swap — any failure aborts with no local mutation, mirroring the single-shard handler_single contract. The WAL v3 write is kept (other record types share the channel; it is a documented non-authority for this deployment shape). Replication: record_local_swapdb_repl appends to the per-shard backlog and advances the offset synchronously, then defers the live-replica send via ShardMessage::ReplicaLiveFanout — the same backlog-then-defer contract wal_append_and_fanout and record_local_write_db use — with a SELECT 0 prefix (R2 multi-shard framing; SWAPDB has no single-db context, task #35 precedent). - Remote legs: the SwapDb SPSC arm already enqueues to that shard's AOF writer strictly before acking; the coordinator now issues one fsync_barrier(target) after observing each ack (H1-BARRIER happens-before ordering: barrier queues behind the append in the same FIFO channel, so an acked barrier proves the record durable). Barriers no-op under everysec/no — no cost off the Always policy. - Failure semantics: local durability failure aborts before any mutation; a remote barrier failure lands AFTER that shard's swap (SWAPDB has no rollback) and is reported truthfully as durability-unconfirmed instead of a false +OK. All remote legs are drained even after one leg errors (coordinate_mset pattern). Red/green: crash_133_swapdb_multishard_durability_after_sigkill (2-shard, appendfsync always, distinct db0/db1 content on both shards via hash tags, SWAPDB 0 1, +OK, zero-delay SIGKILL, restart) — RED on unfixed code with the local shard's entire swap half lost (db0 keys kept db0 values), GREEN 8/8 with the fix. Unit tests prove exactly-once backlog append + offset advance with SELECT-0 framing, no-op without repl_state, and inactive-fanout gating. Fixes #133 author: Tin Dang <tindang.ht97@gmail.com>
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? |
📝 WalkthroughWalkthroughChangesMulti-shard SWAPDB durability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SWAPDBDispatcher
participant coordinate_swapdb
participant AofWriterPool
participant ShardDatabases
SWAPDBDispatcher->>coordinate_swapdb: pass AOF pool and replication state
coordinate_swapdb->>AofWriterPool: append SWAPDB and confirm local fsync
AofWriterPool-->>coordinate_swapdb: local durability confirmed
coordinate_swapdb->>ShardDatabases: apply local database swap
coordinate_swapdb->>AofWriterPool: confirm remote shard fsync
AofWriterPool-->>coordinate_swapdb: remote durability result
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/shard/coordinator.rs (2)
2762-3031: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftExtract the new SWAPDB coordination into a dedicated module.
This file now exceeds 3500 lines. Move the SWAPDB helpers, coordinator, and focused tests into a directory module.
As per coding guidelines, “No single Rust file should exceed 1500 lines” and large command groups should use the directory-module convention.
🤖 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/coordinator.rs` around lines 2762 - 3031, Extract swapdb_repl_active, record_local_swapdb_repl, coordinate_swapdb, and their focused tests from coordinator.rs into a dedicated swapdb directory module using Rust’s mod.rs/module convention. Update coordinator.rs and any existing module declarations/imports to reference the extracted symbols while preserving their visibility, behavior, and call sites; leave unrelated coordinator logic in place.Source: Coding guidelines
2912-2921: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftEstablish local durability before exposing SWAPDB to remotes or replication.
Remote swaps are dispatched before the local WAL/AOF operations that can fail. Additionally, replication backlog/offset updates happen before AOF success and are skipped entirely when
aof_poolisNone. A local enqueue/barrier failure can therefore leave remote shards or replicas swapped while the coordinator remains unchanged; AOF-disabled replication also loses the local leg.Commit local durability first, then record replication independently of
aof_pool, dispatch remote legs, and drain every dispatched receiver on all exits. This also makes the abort-with-no-mutation claim inCHANGELOG.mdaccurate.Also applies to: 2954-2984
🤖 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/coordinator.rs` around lines 2912 - 2921, Reorder the SWAPDB coordinator flow so local WAL/AOF enqueue and durability barriers complete successfully before constructing or dispatching remote ShardMessage::SwapDb requests through spsc_send. Record replication backlog and offset updates independently of aof_pool, including the AOF-disabled path, and only then dispatch remote legs. Ensure every receiver collected in receivers is drained on both success and all failure/abort exits, preserving the no-mutation-on-abort behavior.
🤖 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/shard/coordinator.rs`:
- Around line 3148-3168: Make the assertion in
test_swapdb_repl_active_false_before_any_replica_attaches unconditional by
removing the fanout_hint_active guard, so swapdb_repl_active is always verified
as false for the fresh ReplicationState regardless of the process-global hint.
---
Outside diff comments:
In `@src/shard/coordinator.rs`:
- Around line 2762-3031: Extract swapdb_repl_active, record_local_swapdb_repl,
coordinate_swapdb, and their focused tests from coordinator.rs into a dedicated
swapdb directory module using Rust’s mod.rs/module convention. Update
coordinator.rs and any existing module declarations/imports to reference the
extracted symbols while preserving their visibility, behavior, and call sites;
leave unrelated coordinator logic in place.
- Around line 2912-2921: Reorder the SWAPDB coordinator flow so local WAL/AOF
enqueue and durability barriers complete successfully before constructing or
dispatching remote ShardMessage::SwapDb requests through spsc_send. Record
replication backlog and offset updates independently of aof_pool, including the
AOF-disabled path, and only then dispatch remote legs. Ensure every receiver
collected in receivers is drained on both success and all failure/abort exits,
preserving the no-mutation-on-abort behavior.
🪄 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: e800522c-b8f9-4c09-92b7-e2ec6e9ea5cb
📒 Files selected for processing (5)
CHANGELOG.mdsrc/server/conn/handler_monoio/dispatch.rssrc/server/conn/handler_sharded/dispatch.rssrc/shard/coordinator.rstests/crash_matrix_per_shard_aof.rs
… drop repl-plane recording (#133 review round) Adversarial review of the previous commit refuted two parts of it while confirming the core durability fix sound: - Vector 2 (new hazard, confirmed): record_local_swapdb_repl committed backlog + offset + a queued live fan-out BEFORE the AOF durability gate; a send_append_group/fsync_barrier failure then aborted the local swap but could not retract the already-queued replication record -> the replica applies a swap the master never performed. - Vector 5 (pre-existing, falsifies the "replica-visible" claim): replicas cannot execute streamed SWAPDB at all - replication::apply::apply_local falls through to cmd_dispatch, which hard-errors, and the record silently no-ops with no resync. The new plumbing faithfully delivered bytes the replica cannot apply. - Vector 3 (pre-existing, widened): remote SwapDb messages were dispatched before the local durability gate, so every local abort point left N-1 shards swapped and the coordinator shard not. This commit: - Removes swapdb_repl_active / record_local_swapdb_repl and their unit tests. The local leg no longer touches the replication plane; wiring replica-side SWAPDB application properly (including the remote legs' pre-existing no-op fan-out) is issue #386, with the ordering constraints from the review recorded there. - Reorders coordinate_swapdb: the local durable-append + fsync barrier + local swap now run BEFORE any remote dispatch. Every abort point (WAL backpressure, AOF enqueue failure, fsync failure) fires while the whole cluster is still unmutated - a local abort now leaves all shards consistent instead of N-1 swapped. The confirmed-sound parts are unchanged: durable AOF append on the recovery-authority plane for the local leg (defect 2), post-ack fsync_barrier per remote shard (defect 1, H1-BARRIER ordering), truthful durability-unconfirmed error on post-swap remote barrier failure, lsn=0 PerShard sentinel parity with the remote legs (review Vector 1), no double-replay across WAL v3 + AOF (Vector 4). Refs #133 #386 author: Tin Dang <tindang.ht97@gmail.com>
|
Adversarial review outcome: REFUTED first round — fixed in the follow-up commit. The review confirmed the core durability fix sound (lsn=0 PerShard sentinel parity with remote legs; no double-replay across WAL v3 + AOF under any shards≥2 config; self-queue thread affinity holds on both runtimes) but found: (V2, new) the replication-plane recording committed backlog+offset+fanout BEFORE the durability gate — an AOF/fsync abort could not retract it, diverging master/replica; (V5, pre-existing) replicas cannot execute streamed SWAPDB at all — Follow-up commit: removes the replication-plane recording (tracked properly as #386 with the review's ordering constraints), and reorders the local durable-append+swap BEFORE any remote dispatch so every abort now fires with the cluster fully unmutated. Gates re-run green (lib 4392, both clippys --all-targets, fmt); VM SWAPDB acceptance re-run to follow before merge. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shard/coordinator.rs (1)
2882-2915: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMove the WAL append after the AOF leg
src/shard/coordinator.rs:2882-2915CDC.READconsumes WAL files directly, so enqueuingSWAPDBbefore the AOF append/barrier can still expose a WAL-visible command even if the AOF path aborts and the swap never happens. Reorder the WAL write or make this record provisional.🤖 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/coordinator.rs` around lines 2882 - 2915, Move the try_wal_append_required call and its failure return in the SWAPDB persistence flow to after the AOF send_append_group and any required fsync_barrier complete successfully. Preserve the existing WAL enqueue error response, and ensure AOF failure returns before the command becomes visible through WAL.
🤖 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.
Outside diff comments:
In `@src/shard/coordinator.rs`:
- Around line 2882-2915: Move the try_wal_append_required call and its failure
return in the SWAPDB persistence flow to after the AOF send_append_group and any
required fsync_barrier complete successfully. Preserve the existing WAL enqueue
error response, and ensure AOF failure returns before the command becomes
visible through WAL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee72f558-c591-4a43-9c6f-3162971732a5
📒 Files selected for processing (2)
CHANGELOG.mdsrc/shard/coordinator.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
2 participants
Summary
Closes #133. Two defects in
coordinate_swapdb(the multi-shard SWAPDB path):wal_append_txschannel and remote legs acked right after enqueue —+OKreturned before any shard fsynced under--appendfsync always.--shards ≥ 2 --appendonly yesrecovery replays ONLY the per-shard AOF manifest. The coordinator shard's SWAPDB record never reached the plane recovery reads — a kill-9 after+OKpermanently lost the LOCAL shard's half of the swap while remote halves survived (cross-shard keyspace divergence). The local leg also never reached the replication plane.Fix
send_append_group+ onefsync_barrierunderAlways, BEFORE the swap (abort with no mutation on failure — the single-shardhandler_singlecontract). Replication parity viarecord_local_swapdb_repl(backlog + offset synchronously, deferred live fan-out,SELECT 0framing per R2/task Sync commits Feat/vector engine and fix critical issues #35 precedent).fsync_barrier(target)per remote shard (H1-BARRIER happens-before ordering). Barriers no-op undereverysec/no.coordinate_msetpattern).Verification
crash_133_swapdb_multishard_durability_after_sigkill(2-shard,appendfsync always, distinct db0/db1 content on both shards, SWAPDB, zero-delay SIGKILL, restart) — RED against unfixed code (local shard's swap half lost entirely), GREEN 8/8 with the fix.--all-targets, fmt; VM battery:crash_matrix_per_shard_aof4/4,replication_planes11/11,crash_recovery_cold_multidb1/1 — all on the fixed Linux binary.Out-of-scope finding (separate issue to be filed)
persist_local_leg(MSET/BITOP/COPY/DEL coordinator local legs) appears to have the same missing replica-fanout gap fixed here for SWAPDB — flagged for its own issue rather than folded in (5 shipped call sites, no dedicated test coverage for that bug yet).Summary by CodeRabbit
SWAPDBdurability and replication visibility.SWAPDBreports success.SWAPDBdurability afterSIGKILL.