Conversation
…d tier
Recovery Phase 3 rebuilds the cold index from the shard manifest. Phase 4b's
`replay_aof` then loads the `MOON` preamble that `BGREWRITEAOF` writes at the
head of a rewritten single-file AOF, and `rdb::load_from_bytes` builds fresh
`Database::new()` temporaries and swaps them wholesale over the live ones:
for (live, mut temp) in databases.iter_mut().zip(temp_dbs.into_iter()) {
temp.recalculate_memory();
*live = temp; // cold_index and cold_shard_dir die here
}
`cold_index` and `cold_shard_dir` are live-tier TOPOLOGY; the hot snapshot
does not carry them and `Database::new()` leaves them `None`. `main.rs` then
installs a fresh empty `ColdIndex`, so the server serves with a wired-but-
empty cold plane and every spilled key reads as an ABSENT key — no error, and
`DBSIZE`/`INFO keyspace` agree with the loss because `logical_len` counts
`ci.len()`.
Measured at `--shards 1` after any `BGREWRITEAOF`: 28,868 cold keys gone, and
gone again on every subsequent boot. The damaged-file scenario that surfaced
this is a red herring — an undamaged run loses exactly as much.
Only tokio `--shards 1` reaches it. At `--shards >= 2` the PerShard manifest
routes recovery through `replay_multi_part`, and `aof_manifest::shard_replay`
already brackets the identical swap with `take_cold_wiring` /
`restore_cold_wiring` — its doc comment describes this exact hazard. The
legacy single-file path never got the bracket. monoio is exposed for one boot
when upgrading from a legacy AOF; it self-heals on the next boot, but during
that window ~29K keys answer nil and an `INCR`/`APPEND` against one of them
mints from zero and corrupts it permanently.
The preamble load is now bracketed the same way, restored BEFORE the RESP
tail so replayed `DEL`/`UNLINK`/`FLUSH*` still tombstone the cold plane — with
`cold_index == None` those paths are silent no-ops.
Fixed in `replay_aof` rather than in `rdb::load_from_bytes` deliberately.
`replay_aof` only ever replays THIS node's own log, so preserving is
unconditionally correct; the generic loader also serves replica full-sync and
`DEBUG RELOAD` with a FOREIGN dataset, where keeping this node's cold index
would surface stale reads. The one site also covers the other three
`replay_aof` callers (recovery.rs, shard/mod.rs, wal_v3/replay.rs);
`listener.rs`'s bare `rdb::load` is safe because its databases are freshly
constructed.
Proven red/green: with the bracket reverted and the test kept,
`rdb_preamble_replay_keeps_the_rebuilt_cold_index` fails 3/3 on "the RDB
preamble swap must not unwire the cold tier"; with it, passes.
Closes #1007
Unblocks #1004 — its 1-shard leg then reaches the same pre-fix red as the
4-shard leg (line 623, INFO counters absent) instead of losing the control key.
author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
This was referenced Sep 18, 2026
Open
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.
Closes #1007.
What breaks today
Recovery Phase 3 rebuilds the cold index correctly. Phase 4b then destroys it:
replay_aofloads theMOONpreamble thatBGREWRITEAOFwrites, andrdb::load_from_bytesswaps freshDatabasetemporaries over the live ones (*live = temp,src/persistence/rdb.rs:359), droppingcold_indexandcold_shard_dir. The server serves with a wired-but-empty cold plane: every spilled key reads as an absent key, no error, andDBSIZEagrees.28,868 cold keys lost at
--shards 1after aBGREWRITEAOF, and lost again on every later boot.Rows 3 and 4 differ only in whether anything was damaged. The damage is a red herring.
Why only
--shards 1AOF layout, not sharding logic.
--shards >= 2uses the PerShard manifest and recovers throughreplay_multi_part, which is bracketed —aof_manifest::shard_replayalready hastake_cold_wiring/restore_cold_wiring, and its doc comment describes this exact hazard. Single-shard multi-part is monoio-only (main.rs:1840: "tokio --shards 1 fresh: no manifest"), so that config rewrites the flat AOF with a preamble and recovers through the unbracketed path.monoio is exposed for one boot on a legacy-AOF upgrade; it self-heals next boot, but during that window ~29K keys answer nil and any
INCR/APPENDmints from zero and corrupts permanently.The fix
Bracket the preamble load, restoring before the RESP tail so replayed
DEL/FLUSH*still tombstone cold (withcold_index == Nonethose are silent no-ops).Deliberately not in
rdb::load_from_bytes: that also serves replica full-sync andDEBUG RELOADwith a foreign dataset, where preserving this node's index would surface stale reads.replay_aofonly replays this node's own log. The one site also covers the other threereplay_aofcallers.Verification
Red/green proven by reverting only the 8-line bracket and keeping the test:
assertion failed: the RDB preamble swap must not unwire the cold tierPlus 619
persistence::and 668storage::lib tests, clippy and fmt clean.Relationship to other issues
rebuild_from_manifest_per_dbis proven correct here; Cold-index rebuild drops entries silently on three paths, and a missing cold entry reads as an absent key #875's filed severity is accurate for Cold-index rebuild drops entries silently on three paths, and a missing cold entry reads as an absent key #875.MOON.COLDCUTwatermark vs. destroyed in-memory wiring). Complementary; neither fixes the other.Follow-ups worth their own issues
rdb::load*an explicitTieringPolicy { Preserve, Discard }so the compiler names every call site and no future one can forget.*live = tempconflates "hot snapshot" with "live tier topology"; it also silently dropsreplay_cold_gateandspill_inflight(both empty pre-accept today, so latent).Not verified
The
--appendonly no→CONFIG SET appendonly yesroute on monoio: the harness could not spill in that configuration at all. A harness limitation, not evidence of safety.