fix: 2026-08 deep-review wave — durability, long-uptime arithmetic, cluster election, silent-failure hardening - #453
Merged
Merged
Conversation
…iew A1/A3/A4/A2) The packed 32-bit entry metadata stored last_access in 16 bits with three mutually inconsistent wraparound treatments, and the WATCH/EXEC version in 8 bits: - lfu_decay subtracted the u16-truncated last_access from the full u32 epoch clock and truncated the quotient with `as u8`, so the decay value cycled 0->255 every ~4.5h of wall clock — LFU victim selection was effectively pseudo-random from day one. - lru_is_older's i16 circular compare inverted verdicts once two keys' access times differed by more than ~9.1h, so allkeys-lru evicted hot keys and retained cold ones under diurnal idle patterns. - OBJECT IDLETIME reported idle time modulo 65536s (18.2h wrap). - The 8-bit version wrapped after 256 writes between WATCH and EXEC and reported 0 for both "absent" and "freshly created", so EXEC committed over concurrent modifications (ABA) and never detected key creation. Fix, at zero size cost (CompactEntry stays 32 bytes): repurpose the unused _pad u32 as a full-width last_access_secs field, and use the freed 16 metadata bits to widen version to 24 bits ([version:24|counter:8]). Versions start at INITIAL_VERSION=1 and the wrap skips 0, so version 0 is now a reliable "key absent" sentinel for WATCH creation detection, and the ABA window moves from 256 to 16.7M intervening writes. lfu_decay and IDLETIME use saturating_sub so a lagging cached clock yields zero elapsed rather than a near-2^32 value; lru_is_older uses i32 circular distance (valid to +/-68 years, correct across the year-2106 wrap). Red/green: new regression tests for >9.1h LRU ordering, 2h/11h LFU decay, >18.2h idle survival, 24-bit wrap-skip-0, and version-1-on-create; stale 8-bit/16-bit assertions updated to the new contract. author: Tin Dang
…tch (deep-review F2/F3) Both tokio everysec deadline-fsync sites dropped flush/sync_data errors with `let _ =` and then unconditionally recorded the fsync as successful (last_fsync reset, pending cleared) — zero log lines, and the PerShard variant additionally lacked the `!write_error` torn-stream gate its three siblings have. Because the kernel marks pages clean after a failed fsync (fsyncgate), the next attempt succeeds trivially: the failed window's records are permanently absent from the AOF while the server keeps claiming the <=1s everysec loss bound. - Tokio TopLevel + PerShard now mirror the monoio sites: log error!, record the fsync metric only on success, keep last_fsync unadvanced on failure, and gate on !write_error. - New AOF_LAST_FSYNC_OK / AOF_FSYNC_FAILURES statics (record_everysec_fsync_result), wired into all four everysec sites across both runtimes and surfaced in INFO # Persistence as aof_last_fsync_status:ok|err + aof_fsync_failures — Moon's analogue of Redis's aof_last_write_status, closing the "no operator surface" inconsistency with the WalSyncAgent poison latch and PR #211 policy. Red/green: everysec_fsync_failure_latches_err_status covers the latch contract (err latches + counts; success restores status, never the count). author: Tin Dang
… (deep-review F1) evict_one_async_spill removes the hot entry as soon as the spill request is queued, relying on the completion to land the key in the cold index. When the background pwrite failed (ENOSPC/EIO), the failure completion carried no payload and the handler warned and continued — the key existed in NEITHER plane, so GET returned nil for an acked write until an AOF-replay restart (and permanently on the no-AOF sync paths' async sibling). - SpillCompletion now carries the original SpillRequest back on failure (Box'd; Bytes fields are refcounted, so the clone is cheap). - apply_completion_vec rehydrates the payload (rehydrate_spill_payload, the exact inverse of build_spill_payload) and re-inserts into the hot table via with_shard_db — fail-closed, mirroring the enqueue-failure path which retains the hot value. A newer write that recreated the key while the spill was in flight wins (version!=0 check); the stale payload is dropped. - Loud operator surface: error! log + new spill_failed_reinserted counter in INFO # Persistence (nonzero means the spill volume is failing writes). Red/green: spill_payload_round_trips_through_rehydrate (hash + string, TTL fidelity) and failed_spill_write_carries_request_back (unwritable shard_dir → failure completion carries key/payload/ttl back). author: Tin Dang
…-review G1) TrackingTable documented "bounded: max_keys (default 1_000_000), evict oldest with fake invalidation" but the field was #[allow(dead_code)] — a long-lived CLIENT TRACKING client reading many distinct never-written keys grew key_clients without limit (writes and disconnects clean up, but a read-mostly keyspace never triggers either). track_key now enforces the cap: tracking a NEW key at capacity evicts an arbitrary existing entry and returns its (key, senders) so track_read_keys pushes the RESP3 invalidation for the evicted key — clients drop their cached copy instead of holding it stale forever (Redis's tracking-table "fake invalidation" semantics). Re-tracking an existing key never evicts. Known follow-up (documented, out of scope here): expiry/eviction of the underlying KEY still doesn't invalidate trackers — only writes do. Red/green: test_track_key_enforces_max_keys_bound (cap holds, evicted key's tracker notified, existing-key re-track exempt). author: Tin Dang
…eep-review R6) The monoio inline dispatch loop executes plain GET/SET against the local DashTable before the generic dispatch loop runs, and had no cluster gate: can_inline_writes checked ACL/multi/tracking/replica/spill/fanout but not cluster_enabled(), and GET inlining consulted no gate at all. In cluster mode the two hottest commands therefore bypassed try_handle_cluster_routing entirely — writes for slots owned elsewhere were silently misplaced locally, reads returned stale/nil instead of MOVED, and non-ASKING writes to migrating slots broke the ASK handoff (the known "three dispatch paths" bug class). try_inline_dispatch_loop now takes cluster_enabled (passed from crate::cluster::cluster_enabled() at the call site; a param for unit-testability) and inlines nothing when set — every command falls through to the generic loop where MOVED/ASK routing lives. Non-cluster deployments are unaffected (one relaxed atomic load per batch). Red/green: test_inline_loop_disabled_in_cluster_mode. author: Tin Dang
…urable dirent (deep-review D1/D4) Two crash windows around the rewrite's old-generation prune: D1 — The task-#59 deferred ShardManifest commit is justified by "AOF replay + orphan sweep reconstruct anything a lost manifest commit would have recorded". The #433 auto-rewrite deletes exactly those AOF records (the fold snapshots only db.data(), so cold-spilled keys exist ONLY in the old incr), and the rewrite's own multi-GB write+fsync is what stalls the manifest-sync thread, widening the deferred window across the rewrite. A kill-9 after the prune but before the deferred root persists loses every spilled key in the un-persisted batch — and the boot orphan sweep then deletes the .mpf holding the only surviving copy. D4 — write_manifest's rename fsyncs its parent dir best-effort (warn only). Pruning after a FAILED dir fsync can crash into "old manifest resolves, old files deleted": shard replay treats that as a fresh init and cleanup_orphans deletes the NEW generation too — silent total loss instead of a fail-stop. Fix: both prune sites (TopLevel AofManifest::advance step 4; PerShard RewriteCoordinator::shard_done) now delete the old generation only after (a) manifest_sync::flush_all_agents() — a new ack-only barrier that blocks until every live agent's pending deferred snapshot is durable (bounded 30s; registry of Weak agent handles, registered at spawn, deregistered at shutdown) — and (b) an explicit propagating fsync_directory on the AOF manifest's parent. On either failure the old generation is retained: costs disk space, never data. Red/green: flush_all_agents_forces_pending_deferred_commit_durable (injected sync delay; barrier alone must make the snapshot durable with no shutdown flush) + flush_all_agents_is_noop_when_idle. author: Tin Dang
…ck rules (deep-review C1-C4/R8/A5)
Five defects in the v0.9 W0/C-1 control plane, each small and independently
verifiable (the structural C-2/C-3 gaps — slot-map propagation, demotion,
plane wiring, node persistence — are tracked separately, not half-fixed
here):
- C1: election votes were structurally unreceivable. The replica sent
FailoverAuthRequest on a fire-and-forget connection and dropped it before
the master's reply, which was written back on that same dying stream; no
master ever dials the replica's bus. Any election with votes_needed >= 2
timed out unconditionally (masked in small test clusters where a FAILed
master deflates quorum to 1 and the self-vote wins alone). The requester
now holds each stream open and reads the length-prefixed ack back
(4s bound, frame-length guard); the bus inbound ack arm stays as a
secondary path.
- C2: acks carried no election binding and voters no identity check — a
stale ack from a previous timed-out election, or repeated acks from one
voter, counted toward quorum. Acks now ECHO the request epoch; the vote
loop discards epoch mismatches and dedups voters via HashSet (self-id
pre-seeded so a reflected self-ack can never double-count).
- C4: on win, check_and_initiate_failover bumped state.epoch a SECOND time,
so the promoted master claimed an epoch nobody voted for — defeating
last_vote_epoch's single-vote-per-epoch guarantee and enabling epoch ties
gossip's strict-> merge cannot resolve. Election wins now promote via
promote_self_to_master at the voted epoch; FORCE/TAKEOVER keep their
explicit bumps.
- R8: graceful CLUSTER FAILOVER (no args) parked failover_state at
WaitingDelay{0} forever — nothing consumed it and the ticker's election
gate requires FailoverState::None, so one admin command permanently
disabled automatic failover on the node. It now returns an explicit
"not yet supported" error (state untouched) until the coordinated
offset-sync protocol lands.
- C3: ClusterState was std::sync::RwLock with ~25 .read()/.write().unwrap()
sites including the per-command route_slot path — violating the repo lock
rules and turning any panic under a write guard into poisoning that
panics every shard thread. Migrated to parking_lot::RwLock (no poisoning,
no unwraps) across src/cluster, main.rs, shard wiring, and conn handlers.
- A5: a peer announcing a client port > 55535 got a silently wrapped bus
port (our own port is already refused at startup); the wrap now warns
loudly that gossip to that peer will fail.
Red/green: test_promote_self_keeps_voted_epoch,
test_failover_normal_errors_without_wedging_state; existing 55 cluster
tests green.
author: Tin Dang
…temporal registry (deep-review F4/G3/G4) F4 — vector background compaction failed silently at every layer: a dead worker (panic/pool teardown) cleared inflight with no log, a compaction Err (e.g. B2 disk persist on a full volume) was dropped with no log, and the resubmit swallow made it an invisible infinite retry loop while the mutable segment grew past COMPACT_THRESHOLD and FT.SEARCH degraded toward brute force. The merge path learned this lesson in PR #353; the compact path now matches: error! on worker death, warn! on compaction failure, debug trail on deferred submits. G3 — CDC subscribers whose consumer disconnected were only reaped via a failed try_send driven by a NEW WAL record; write-idle shards accumulated dead subscribers (each holding a tail reader) indefinitely. fanout_tick now checks tx.is_disconnected() every tick. G4 — TemporalRegistry grew one binding per TEMPORAL.SNAPSHOT_AT for the life of the process. Bounded at 262,144 bindings/shard (~4 MB); overflow evicts the oldest, bounding AS_OF history depth instead of memory. Red/green: test_cdc_fanout_reaps_disconnected_subscriber_when_idle, test_registry_bounded_evicts_oldest. author: Tin Dang
Formatting-only pass over files touched by the 2026-08 deep-review fixes (cluster, tracking, AOF, conn tests). No behavior change. author: Tin Dang
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR applies correctness and resilience fixes across storage metadata, spill recovery, AOF durability, cluster failover and routing, tracking limits, CDC cleanup, temporal bounds, compaction logging, and persistence reporting. ChangesStorage metadata and spill recovery
AOF telemetry and rewrite durability
Cluster state and failover correctness
Cluster routing and connection wiring
Bounded tracking and background cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Replica
participant ClusterPeer
participant Election
participant ClusterState
Replica->>ClusterPeer: Send failover request
ClusterPeer-->>Replica: Return voter ID and acknowledgement epoch
Replica->>Election: Submit vote and epoch
Election->>ClusterState: Validate epoch and deduplicate voter
Election->>ClusterState: Promote replica at voted epoch
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoDeep-review fixes: long-uptime storage metadata, AOF fsync visibility, cluster election/routing
AI Description
Diagram
High-Level Assessment
Files changed (32)
|
…ng_lot The C3 lock migration moved `Shard::run`'s `cluster_state` parameter to `parking_lot::RwLock`, but the integration-test cluster harness still wrapped ClusterState in `std::sync::RwLock`, so `cargo test` (all targets) failed to compile on CI while the local `--lib`-only gate stayed green. Lesson applied: verify with `cargo check --all-targets` on both feature sets, both now clean. author: Tin Dang
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+261
to
+263
| #[test] | ||
| fn test_cdc_fanout_reaps_disconnected_subscriber_when_idle() { | ||
| let tmp = tempfile::tempdir().unwrap(); |
There was a problem hiding this comment.
1. fanout.rs test uses unwrap() 📘 Rule violation ✧ Quality
A newly added test uses .unwrap() without the required #[allow(clippy::unwrap_used)] plus justification comment, and it is placed in a split-module subfile instead of mod.rs. This violates the unwrap-annotation and split-module test placement requirements.
Agent Prompt
## Issue description
A new unit test was added in a split-module subfile (`src/cdc/fanout.rs`) and it uses `.unwrap()` without the required allow+justification pattern.
## Issue Context
Compliance requires (1) unit tests for split modules live in the module `mod.rs`, and (2) any remaining `.unwrap()` usage in diffs must be covered by `#[allow(clippy::unwrap_used)]` with a justification comment directly above the attribute.
## Fix Focus Areas
- src/cdc/fanout.rs[256-283]
- src/cdc/mod.rs[26-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+748
to
+751
| let tmp = tempfile::tempdir().unwrap(); | ||
| // Make shard_dir an existing FILE so the spill write cannot succeed. | ||
| let bogus_dir = tmp.path().join("not-a-dir"); | ||
| std::fs::write(&bogus_dir, b"occupied").unwrap(); |
There was a problem hiding this comment.
2. spill_thread.rs test uses unwrap() 📘 Rule violation ✧ Quality
A newly added test uses .unwrap() without the required #[allow(clippy::unwrap_used)] plus justification comment, and it is placed in a split-module subfile instead of mod.rs. This violates the unwrap-annotation and split-module test placement requirements.
Agent Prompt
## Issue description
A new unit test was added in a split-module subfile (`src/storage/tiered/spill_thread.rs`) and it introduces `.unwrap()` calls without the required allow+justification.
## Issue Context
Compliance requires unit tests for split modules to live in the corresponding `mod.rs`, and any `.unwrap()` remaining in diffs must be annotated with `#[allow(clippy::unwrap_used)]` plus a justification comment directly above the attribute.
## Fix Focus Areas
- src/storage/tiered/spill_thread.rs[744-772]
- src/storage/tiered/mod.rs[1-10]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+338
to
+339
| #[allow(clippy::unwrap_used)] // test-only; poisoning would already be a failed test | ||
| let _knob = super::super::manifest::TEST_SYNC_KNOB_LOCK.lock().unwrap(); |
There was a problem hiding this comment.
3. Knob-lock unwrap lacks comment 📘 Rule violation ✧ Quality
An added #[allow(clippy::unwrap_used)] does not have the required justification comment on the line directly above it. This violates the unwrap annotation rule and reduces auditability of why the unwrap is safe.
Agent Prompt
## Issue description
The unwrap allow attribute is missing the required preceding justification comment line.
## Issue Context
Compliance requires a single-line justification comment immediately above `#[allow(clippy::unwrap_used)]` (no blank line), explaining the invariant that makes the unwrap safe.
## Fix Focus Areas
- src/persistence/manifest_sync.rs[336-341]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+152
to
+153
| #[allow(clippy::unwrap_used)] // len >= 1 guaranteed by the branch | ||
| let victim = self.key_clients.keys().next().unwrap().clone(); |
There was a problem hiding this comment.
4. victim unwrap comment missing 📘 Rule violation ✧ Quality
The added #[allow(clippy::unwrap_used)] for self.key_clients.keys().next().unwrap() does not have the required justification comment on the line directly above it. This violates the unwrap annotation rule and makes the safety invariant easy to miss during review.
Agent Prompt
## Issue description
A production `.unwrap()` was added with `#[allow(clippy::unwrap_used)]`, but the required justification comment is not on the line directly above the attribute.
## Issue Context
Compliance requires a single-line comment immediately preceding the `#[allow(clippy::unwrap_used)]` attribute explaining why the unwrap cannot panic (the invariant established by the prior branch).
## Fix Focus Areas
- src/tracking/mod.rs[149-154]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main.rs (1)
1011-1031: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit
src/main.rsbefore adding more bootstrap logic.
src/main.rsreaches at least line 2151. It exceeds the 1500-line file limit. Extract cluster bootstrap and control-plane setup into dedicated modules, and keep only top-level orchestration inmain.As per coding guidelines, “No single Rust file should exceed 1500 lines.”
🤖 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/main.rs` around lines 1011 - 1031, Extract the cluster bootstrap and control-plane setup currently embedded in main into dedicated Rust modules, including the logic around ClusterState initialization and cluster-enabled validation. Keep main limited to top-level orchestration and call the new module APIs from it, ensuring no Rust source file exceeds 1500 lines.Source: Coding guidelines
src/cluster/bus.rs (1)
197-207: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject untrusted failover ACKs before forwarding them.
Line 206 forwards an ACK from any TCP peer with its claimed voter ID.
run_election_taskonly checks the epoch and de-duplicates that supplied ID. A peer that knows the active epoch can send ACKs with distinct forged IDs and satisfy quorum without master votes.Before
tx.send, verify thatsender_ididentifies a current master and thatpeer_addr.ip()matches that node's registered address. Recheck master membership in the election loop as defense in depth.Proposed authorization check
debug!( "Received failover ACK from {} (epoch {})", sender_id, msg.config_epoch ); + let authorized_voter = { + let cs = cluster_state.read(); + cs.nodes.get(&sender_id).is_some_and(|node| { + matches!(node.flags, crate::cluster::NodeFlags::Master) + && node.addr.ip() == peer_addr.ip() + }) + }; + if !authorized_voter { + warn!("Ignoring failover ACK from unauthorized peer {}", peer_addr); + continue; + } if let Some(tx) = vote_tx.lock().as_ref() { let _ = tx.send((sender_id, msg.config_epoch)); }🤖 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/cluster/bus.rs` around lines 197 - 207, Authorize failover ACKs in the GossipMsgType::FailoverAuthAck handler before tx.send: resolve sender_id against the current master membership and require peer_addr.ip() to match that master’s registered address, dropping unauthorized ACKs. Also revalidate master membership in run_election_task before counting received votes, while preserving epoch and deduplication checks.
🧹 Nitpick comments (5)
src/storage/eviction.rs (1)
1168-1181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning the wraparound claim.
The doc comment on
lru_is_olderstates that verdicts stay correct across the year-2106 clock wrap. No assertion covers that case. Add one so the documented guarantee is verified.💚 Proposed assertion
// Still correct at multi-day idle. let idle_3d = now - 259_200; assert!(lru_is_older(idle_3d, idle_10h)); + // Across the u32 (year-2106) wrap: `before` precedes `after` in time. + let before = u32::MAX - 100; + let after = 100u32; + assert!(lru_is_older(before, after)); + assert!(!lru_is_older(after, before)); }🤖 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/storage/eviction.rs` around lines 1168 - 1181, Add a regression assertion in lru_is_older_valid_beyond_nine_hours that exercises timestamps across the u32 year-2106 clock wrap, verifying lru_is_older preserves the documented ordering in both argument directions. Keep the existing long-idle assertions unchanged.src/persistence/aof/mod.rs (1)
429-462: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider releasing the manifest lock before the durability barrier.
flush_all_agents()waits up to 30 seconds per registered agent (seesrc/persistence/manifest_sync.rslines 116-121).fsync_directoryis also a blocking syscall. Both run while theAofManifestmutex guardmis held;drop(m)is at line 462.At this call site the terminal writer is the only manifest user, so contention is low today. The pattern still couples an unbounded-latency wait to a shared lock. The barrier needs no manifest state, so it can run before the guard is taken for the prune.
No deadlock exists: the manifest-sync agents persist
ShardManifeststate and never take thisAofManifestmutex.🤖 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/persistence/aof/mod.rs` around lines 429 - 462, Move the manifest-sync durability barrier and directory fsync out of the scope holding the AofManifest guard in the rewrite flow. Run flush_all_agents() and fsync_directory using the manifest path before acquiring the guard needed for prune_shard_files, then retain the existing safe_to_prune behavior and only lock the manifest for pruning.src/storage/entry.rs (1)
485-618: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared metadata initializer.
Thirteen constructors now repeat the same two lines. A future constructor can be added without the version stamp, which would reintroduce a version-0 entry and break WATCH creation detection.
Extract a private helper and call it from each constructor.
♻️ Proposed helper
+/// Fresh metadata for a newly constructed entry: version [`INITIAL_VERSION`], +/// LFU counter at [`LFU_INIT_VAL`], last access at the current clock. +#[inline] +fn fresh_metadata() -> (u32, u32) { + ( + pack_metadata_u32(INITIAL_VERSION, LFU_INIT_VAL), + current_secs(), + ) +}Each constructor then becomes:
let (metadata, last_access_secs) = fresh_metadata(); CompactEntry { value: CompactValue::from_redis_value(RedisValue::String(value)), ttl_ms: 0, metadata, last_access_secs, }🤖 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/storage/entry.rs` around lines 485 - 618, Extract a private fresh_metadata helper near the CompactEntry constructors that returns the initialized metadata and current access timestamp. Update all thirteen constructors, including new_string_with_expiry, to obtain these fields from the helper instead of calling pack_metadata_u32 and current_secs directly, while preserving each constructor’s value and TTL behavior.src/persistence/aof/writer_task.rs (1)
1359-1385: 🩺 Stability & Availability | 🔵 TrivialTorn-write state is not visible in
aof_last_fsync_status.The new
!write_errorguard at line 1360 makes this site consistent with the other three everysec sites. That is the right change.It also means a writer that has latched
write_errorstops callingrecord_everysec_fsync_resultentirely.AOF_LAST_FSYNC_OKthen keeps its last value, which istrueif the tear followed a clean fsync. Undereverysecthere are noAppendSyncwaiters, so no client receivesAOF_FSYNC_ERReither. An operator reading INFO seesaof_last_fsync_status:okfor a writer that has permanently stopped appending.This gap is shared by all four everysec sites and is not introduced here. Consider exposing the latch as its own INFO field, for example
aof_write_error:0|1, alongside the newaof_fsync_failurescounter.🤖 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/persistence/aof/writer_task.rs` around lines 1359 - 1385, Expose the latched write-error state in INFO as a dedicated field such as aof_write_error:0|1, alongside aof_last_fsync_status and aof_fsync_failures. Reuse the existing write_error latch from the everysec writer paths, including the flow around the PerShard sync block, and ensure the field reports 1 after a torn write and 0 otherwise without changing existing fsync status behavior.src/persistence/aof_manifest/mod.rs (1)
1049-1068: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared prune-durability barrier. Both sites implement the same rule before deleting an old AOF generation: make pending deferred
ShardManifestcommits durable viaflush_all_agents(), then fsync the manifest directory entry, and skip the prune if either step fails. The rule is duplicated, and the copies have already diverged on log severity.
src/persistence/aof_manifest/mod.rs#L1049-L1068: extract the barrier into a helper onAofManifest, for examplefn prune_is_safe(&self, old_seq: u64) -> bool, and call it here in place of the inlineflush_all_agents()andfsync_directoryblock.src/persistence/aof/mod.rs#L429-L462: replace the inlinesafe_to_prunematch with a call to the same helper, so theerror!severity used here and thewarn!severity used inadvanceresolve to one level.🤖 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/persistence/aof_manifest/mod.rs` around lines 1049 - 1068, Extract the shared prune-durability check into an AofManifest helper such as prune_is_safe(&self, old_seq: u64) -> bool, preserving the flush_all_agents and fsync_directory ordering and skip-on-failure behavior. In src/persistence/aof_manifest/mod.rs lines 1049-1068, replace the inline barrier with the helper call; in src/persistence/aof/mod.rs lines 429-462, replace the safe_to_prune match with the same helper so both paths share the helper’s warning severity and 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/cluster/failover.rs`:
- Around line 265-290: Update the tokio::spawn failover task around
TcpStream::connect, the length/data write_all calls, and read_ack so a single
four-second timeout covers the entire connection, request, and ACK exchange.
Move the timeout boundary to encompass all three phases, while preserving the
existing early return behavior for connection, write, timeout, or invalid ACK
failures.
In `@src/persistence/aof/mod.rs`:
- Around line 99-107: Update the doc comment for AOF_LAST_FSYNC_OK to describe
it as the status of the most recent everysec fsync attempt, noting that a later
successful fsync restores it to “ok” without requiring a successful write batch.
Clarify that AOF_FSYNC_FAILURES preserves failure history, and remove the claim
that the boolean latches “err”.
In `@src/server/conn/handler_monoio/dispatch.rs`:
- Around line 263-265: Remove the obsolete #[allow(clippy::unwrap_used)]
attribute and the accompanying std RwLock poison comment above the route lookup
in the dispatch handler, leaving the parking_lot::RwLock read flow through
cs.read().route_slot(slot, was_asking) unchanged.
In `@src/tracking/mod.rs`:
- Around line 149-164: Remove the suppressed unwrap in the eviction branch of
track_key. Replace the keys().next().unwrap() and subsequent remove sequence
with an Option-based remove_entry flow, handling the absent-entry case without
unwrap or expect while preserving victim notification behavior.
---
Outside diff comments:
In `@src/cluster/bus.rs`:
- Around line 197-207: Authorize failover ACKs in the
GossipMsgType::FailoverAuthAck handler before tx.send: resolve sender_id against
the current master membership and require peer_addr.ip() to match that master’s
registered address, dropping unauthorized ACKs. Also revalidate master
membership in run_election_task before counting received votes, while preserving
epoch and deduplication checks.
In `@src/main.rs`:
- Around line 1011-1031: Extract the cluster bootstrap and control-plane setup
currently embedded in main into dedicated Rust modules, including the logic
around ClusterState initialization and cluster-enabled validation. Keep main
limited to top-level orchestration and call the new module APIs from it,
ensuring no Rust source file exceeds 1500 lines.
---
Nitpick comments:
In `@src/persistence/aof_manifest/mod.rs`:
- Around line 1049-1068: Extract the shared prune-durability check into an
AofManifest helper such as prune_is_safe(&self, old_seq: u64) -> bool,
preserving the flush_all_agents and fsync_directory ordering and skip-on-failure
behavior. In src/persistence/aof_manifest/mod.rs lines 1049-1068, replace the
inline barrier with the helper call; in src/persistence/aof/mod.rs lines
429-462, replace the safe_to_prune match with the same helper so both paths
share the helper’s warning severity and behavior.
In `@src/persistence/aof/mod.rs`:
- Around line 429-462: Move the manifest-sync durability barrier and directory
fsync out of the scope holding the AofManifest guard in the rewrite flow. Run
flush_all_agents() and fsync_directory using the manifest path before acquiring
the guard needed for prune_shard_files, then retain the existing safe_to_prune
behavior and only lock the manifest for pruning.
In `@src/persistence/aof/writer_task.rs`:
- Around line 1359-1385: Expose the latched write-error state in INFO as a
dedicated field such as aof_write_error:0|1, alongside aof_last_fsync_status and
aof_fsync_failures. Reuse the existing write_error latch from the everysec
writer paths, including the flow around the PerShard sync block, and ensure the
field reports 1 after a torn write and 0 otherwise without changing existing
fsync status behavior.
In `@src/storage/entry.rs`:
- Around line 485-618: Extract a private fresh_metadata helper near the
CompactEntry constructors that returns the initialized metadata and current
access timestamp. Update all thirteen constructors, including
new_string_with_expiry, to obtain these fields from the helper instead of
calling pack_metadata_u32 and current_secs directly, while preserving each
constructor’s value and TTL behavior.
In `@src/storage/eviction.rs`:
- Around line 1168-1181: Add a regression assertion in
lru_is_older_valid_beyond_nine_hours that exercises timestamps across the u32
year-2106 clock wrap, verifying lru_is_older preserves the documented ordering
in both argument directions. Keep the existing long-idle assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a39a811f-e487-4853-9b83-74ffa1286832
📒 Files selected for processing (32)
CHANGELOG.mdsrc/cdc/fanout.rssrc/cluster/bus.rssrc/cluster/command.rssrc/cluster/failover.rssrc/cluster/gossip.rssrc/cluster/mod.rssrc/command/connection.rssrc/command/key.rssrc/main.rssrc/persistence/aof/mod.rssrc/persistence/aof/writer_task.rssrc/persistence/aof_manifest/mod.rssrc/persistence/manifest_sync.rssrc/server/conn/blocking.rssrc/server/conn/core.rssrc/server/conn/handler_monoio/dispatch.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/tests.rssrc/shard/conn_accept.rssrc/shard/event_loop.rssrc/shard/persistence_tick.rssrc/storage/db/kv_ops.rssrc/storage/db/mod.rssrc/storage/entry.rssrc/storage/eviction.rssrc/storage/tiered/spill_thread.rssrc/temporal/mod.rssrc/tracking/invalidation.rssrc/tracking/mod.rssrc/vector/store.rs
Comment on lines
265
to
+290
| tokio::spawn(async move { | ||
| if let Ok(mut stream) = TcpStream::connect(addr).await { | ||
| let len = (data.len() as u32).to_be_bytes(); | ||
| let _ = stream.write_all(&len).await; | ||
| let _ = stream.write_all(&data).await; | ||
| let Ok(mut stream) = TcpStream::connect(addr).await else { | ||
| return; | ||
| }; | ||
| let len = (data.len() as u32).to_be_bytes(); | ||
| if stream.write_all(&len).await.is_err() || stream.write_all(&data).await.is_err() { | ||
| return; | ||
| } | ||
| let read_ack = async { | ||
| let mut len_buf = [0u8; 4]; | ||
| stream.read_exact(&mut len_buf).await.ok()?; | ||
| let body_len = u32::from_be_bytes(len_buf) as usize; | ||
| if body_len == 0 || body_len > crate::cluster::bus::MAX_GOSSIP_FRAME_LEN { | ||
| return None; | ||
| } | ||
| let mut buf = vec![0u8; body_len]; | ||
| stream.read_exact(&mut buf).await.ok()?; | ||
| deserialize_gossip(&buf).ok() | ||
| }; | ||
| // Bounded: a master that voted NO sends nothing and we must not | ||
| // hold the task past the election deadline. | ||
| let Ok(Some(msg)) = | ||
| tokio::time::timeout(std::time::Duration::from_secs(4), read_ack).await | ||
| else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound connection setup and request writes.
The four-second timeout starts only after TcpStream::connect and both write_all calls finish. A blackholed peer can keep those operations pending beyond the election. Each later election then spawns more pending tasks.
Wrap connection setup, framing writes, and ACK reads in one deadline.
Proposed deadline scope
- tokio::spawn(async move {
- let Ok(mut stream) = TcpStream::connect(addr).await else {
- return;
- };
+ tokio::spawn(async move {
+ let request = async {
+ let mut stream = TcpStream::connect(addr).await.ok()?;
let len = (data.len() as u32).to_be_bytes();
- if stream.write_all(&len).await.is_err() || stream.write_all(&data).await.is_err() {
- return;
- }
+ stream.write_all(&len).await.ok()?;
+ stream.write_all(&data).await.ok()?;
let read_ack = async {
// existing ACK read logic
};
- let Ok(Some(msg)) =
- tokio::time::timeout(std::time::Duration::from_secs(4), read_ack).await
- else {
+ let msg = read_ack.await?;
+ Some(msg)
+ };
+ let Ok(Some(msg)) =
+ tokio::time::timeout(std::time::Duration::from_secs(4), request).await
+ else {
return;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tokio::spawn(async move { | |
| if let Ok(mut stream) = TcpStream::connect(addr).await { | |
| let len = (data.len() as u32).to_be_bytes(); | |
| let _ = stream.write_all(&len).await; | |
| let _ = stream.write_all(&data).await; | |
| let Ok(mut stream) = TcpStream::connect(addr).await else { | |
| return; | |
| }; | |
| let len = (data.len() as u32).to_be_bytes(); | |
| if stream.write_all(&len).await.is_err() || stream.write_all(&data).await.is_err() { | |
| return; | |
| } | |
| let read_ack = async { | |
| let mut len_buf = [0u8; 4]; | |
| stream.read_exact(&mut len_buf).await.ok()?; | |
| let body_len = u32::from_be_bytes(len_buf) as usize; | |
| if body_len == 0 || body_len > crate::cluster::bus::MAX_GOSSIP_FRAME_LEN { | |
| return None; | |
| } | |
| let mut buf = vec![0u8; body_len]; | |
| stream.read_exact(&mut buf).await.ok()?; | |
| deserialize_gossip(&buf).ok() | |
| }; | |
| // Bounded: a master that voted NO sends nothing and we must not | |
| // hold the task past the election deadline. | |
| let Ok(Some(msg)) = | |
| tokio::time::timeout(std::time::Duration::from_secs(4), read_ack).await | |
| else { | |
| return; | |
| }; | |
| tokio::spawn(async move { | |
| let request = async { | |
| let mut stream = TcpStream::connect(addr).await.ok()?; | |
| let len = (data.len() as u32).to_be_bytes(); | |
| stream.write_all(&len).await.ok()?; | |
| stream.write_all(&data).await.ok()?; | |
| let read_ack = async { | |
| let mut len_buf = [0u8; 4]; | |
| stream.read_exact(&mut len_buf).await.ok()?; | |
| let body_len = u32::from_be_bytes(len_buf) as usize; | |
| if body_len == 0 || body_len > crate::cluster::bus::MAX_GOSSIP_FRAME_LEN { | |
| return None; | |
| } | |
| let mut buf = vec![0u8; body_len]; | |
| stream.read_exact(&mut buf).await.ok()?; | |
| deserialize_gossip(&buf).ok() | |
| }; | |
| let msg = read_ack.await?; | |
| Some(msg) | |
| }; | |
| let Ok(Some(msg)) = | |
| tokio::time::timeout(std::time::Duration::from_secs(4), request).await | |
| else { | |
| return; | |
| }; |
🤖 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/cluster/failover.rs` around lines 265 - 290, Update the tokio::spawn
failover task around TcpStream::connect, the length/data write_all calls, and
read_ack so a single four-second timeout covers the entire connection, request,
and ACK exchange. Move the timeout boundary to encompass all three phases, while
preserving the existing early return behavior for connection, write, timeout, or
invalid ACK failures.
Comment on lines
+149
to
+164
| let evicted = if self.key_clients.len() >= self.max_keys.max(1) { | ||
| // Evict an arbitrary entry (HashMap has no age order; correctness | ||
| // needs only that the evicted key's trackers are told to drop it). | ||
| #[allow(clippy::unwrap_used)] // len >= 1 guaranteed by the branch | ||
| let victim = self.key_clients.keys().next().unwrap().clone(); | ||
| let clients = self.key_clients.remove(&victim).unwrap_or_default(); | ||
| let mut senders = Vec::new(); | ||
| for (cid, _noloop) in clients { | ||
| // No noloop skip: cap eviction is not a self-write — every | ||
| // tracker of the victim key must drop its cached copy. | ||
| let target_id = self.redirects.get(&cid).copied().unwrap_or(cid); | ||
| if let Some(tx) = self.client_channels.get(&target_id) { | ||
| senders.push(tx.clone()); | ||
| } | ||
| } | ||
| Some((victim, senders)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the prohibited unwrap.
track_key is library code. Line 152 suppresses Clippy for the unwrap on Line 153. Replace the lookup and removal sequence with an Option-based remove_entry flow.
As per coding guidelines, library code must avoid unwrap and expect.
🤖 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/tracking/mod.rs` around lines 149 - 164, Remove the suppressed unwrap in
the eviction branch of track_key. Replace the keys().next().unwrap() and
subsequent remove sequence with an Option-based remove_entry flow, handling the
absent-entry case without unwrap or expect while preserving victim notification
behavior.
Source: Coding guidelines
…iew wave Pre-merge adversarial review of this branch confirmed four defects in the new code; all four are fixed here with red/green unit coverage. 1. P1 — flush_all_agents false-Ok after a failed persist: a failed deferred manifest commit consumed the root, latched nothing, and the next empty-slot barrier round acked Ok — letting the rewrite prune the old incr while the on-disk ShardManifest was missing spill placements (permanent cold-plane loss on a later crash). SyncShared now latches last_persist_failed; empty-slot rounds ack Err until a newer snapshot persists successfully. 2. P2 — AGENT_REGISTRY zombie: an agent handle dropped without shutdown() (panic unwind, early teardown) left the registry's wake-sender clone keeping the sync thread parked forever holding the manifest fd, and strong_count pruning could never collect it (the thread holds its own strong Arc). ManifestSyncAgent now deregisters in Drop, so the thread sees the disconnect, flushes, and exits. 3. P2 — AOF_LAST_FSYNC_OK cross-writer masking: one global bool meant a healthy shard's everysec success cleared a failing shard's err status within ~1s. Replaced with a per-writer bitmask (AOF_FSYNC_ERR_WRITERS); INFO reports err while ANY writer's latest fsync failed. 4. P2 — failed-spill re-insert stale shadow: a failed pwrite for key K could re-insert its stale payload into the hot table after K was re-created and re-evicted (newer spill in flight), shadowing the newer cold value and re-spilling the stale copy as authoritative. Databases now track the newest in-flight spill request id per key (requests already carry a unique monotonic file_id); a superseded request's failure arm skips the re-insert, and completions retire their own record. Also removes two stale std-RwLock clippy annotations left over from the parking_lot migration (route_slot call sites). Refs the deep-review wave PR; found by pre-merge adversarial review. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 8, 2026
… writes (#456) Patch release rolling up the 2026-08 deep-review wave (#453) and durability wave 1 (#452/#54, PR #454), both merged after two-round adversarial review. Headline: AOF rewrites no longer drop acked writes under sustained pipelined load. A per-writer RewriteOverflow spill buffer (256 MiB cap, strict ordering, all six fold arms on both runtimes) buffers appends while the writer is mid-fold, with an exactly-once snapshot cut so a committed fold discards pre-snapshot spills (effects live in the new base) and an aborted fold writes everything. Merge-base A/B: main lost ~5.6k acked writes per hit; fixed is exact across SIGKILL + recovery (251k appends through the overflow in the release-gate e2e). The re-verify round closed an abort-treated-as-commit P0, a same-key replay-inversion window, an ungated ordered-append leg, and taken-batch loss accounting; residual architectural findings tracked in #455. Also: WAL v3 mid-chain tears abort boot (exit 70; MOON_WAL_SALVAGE=1 override) instead of silently replaying past a hole; sticky aof_last_append_status + per-writer aof_last_fsync_status latches; reason-DEL escalated backpressure with one shared bound per eviction sweep; manifest-sync failure latch; failed-spill victim re-insert; eviction metadata widened (LFU decay, LRU inversion, OBJECT IDLETIME wrap, WATCH ABA); cluster election acks received + inline fast path disabled in cluster mode; CLIENT TRACKING max_keys enforced. Validation: fmt + clippy x2 feature sets; macOS monoio 4541 + tokio 3705 and VM Linux 4563 lib tests; rewrite-under-pipelined-load e2e; PR CI green on both PRs pre-merge; crash-matrix nightly + ITERS=20 soak dispatched on the RC (b346910), green before tag. Rolls CHANGELOG [Unreleased] into [0.8.5], bumps Cargo.toml/lock, adds the RELEASES.md row, updates the README milestone table. author: Tin Dang
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
1 participant
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
Self-directed deep review across six dimensions chosen for long-uptime, large-scale deployments — durability/crash-recovery ordering, long-uptime resource growth, concurrency/unsafe, replication+cluster correctness, long-horizon arithmetic, and silent degradation. 32 findings total; this PR fixes the 19 bounded, unambiguous defects. The remaining structural items are filed as #451 (cluster control-plane gaps, for the v0.9 C-2/C-3 track) and #452 (durability follow-ups).
Fixes by commit
fix(storage)— full-width last_access + 24-bit version (A1/A3/A4/A2):CompactEntry's unused_pad: u32becomes a fulllast_access_secs: u32(the old 16-bit field wrapped every ~18h, corrupting LRU ordering and OBJECT IDLETIME on any server up longer than that). Version is now 24-bit (wraps skipping 0, starts at 1) soget_version() == 0reliably means "absent".lru_is_olderuses a wrapping signed compare valid for ±68 years. OBJECT IDLETIME drops its& 0xFFFFtruncation.fix(persistence)— fail-loud everysec fsync (F2/F3): all four writer-task deadline-fsync sites (tokio TopLevel/PerShard + monoio) now log on error and only advancelast_fsyncon success (fsyncgate: a failed fsync must not be treated as durable). New INFO fields:aof_last_fsync_status,aof_fsync_failures.fix(storage)— spill-failure re-insert (F1): a failed background spill write silently dropped the evicted key.SpillCompletionnow carries the failed request back; the shard rehydrates and re-inserts it (version-checked so a newer write wins), with a counter exposed in INFO.fix(tracking)— enforce max_keys (G1): the documented 1M tracking-table cap was dead code; a long-lived tracking client reading distinct keys grew the table without bound.track_keynow evicts at the cap and pushes a fake invalidation to the evicted key's trackers (Redis semantics).fix(cluster)— inline fast path gated off in cluster mode (R6): the inline GET/SET dispatch bypassed MOVED/slot ownership checks entirely, silently serving keys the node doesn't own.fix(persistence)— rewrite prune safety (D1/D4): AOF rewrite now flushes all off-loop manifest-sync agents (new ack-only flush barrier) and fsyncs the manifest directory entry before pruning the old generation; on failure the old files are kept. Closes the crash window where the new manifest wasn't durable but the old generation was already deleted.fix(cluster)— election correctness + lock rules (C1-C4/R8/A5): failover auth acks are now read back on the request stream with an epoch echo; stale-epoch acks are discarded; voters are deduplicated (a double-ack could previously fake a quorum); promotion happens at the voted epoch instead of double-bumping.CLUSTER FAILOVERwithout FORCE/TAKEOVER returns an error instead of wedging inWaitingDelay. Remainingstd::sync::RwLockusers migrated toparking_lotper repo rules.fix(storage)— fail-loud + bounds (F4/G3/G4): vector compaction install errors are logged instead of swallowed; CDC fanout reaps disconnected subscribers even when idle; temporal registry bounded at 262,144 bindings.Testing
cargo fmt --check✅cargo clippy -- -D warnings✅ (default features)cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings✅cargo test --lib: 4518 passed (monoio default) ✅cargo test --lib --no-default-features --features runtime-tokio,jemalloc: 3682 passed ✅Follow-ups (filed, out of scope here)
author: Tin Dang
Summary by CodeRabbit