fix(persistence): cold recovery re-attaches spilled keys to their own database (#139)#383
Conversation
… database (#139) Under disk-offload with SELECT N (N>0), the v3 recovery rebuild merged every spilled key into databases[0]'s cold index. A key spilled from db 1 was unreachable after restart (nil — and under appendonly=no the cold tier is its ONLY durable copy), and because rebuild inserts are keyed by name alone, a db0 key sharing a name with a db1 key could recover the WRONG database's value into db 0. The live spill path was already db-aware end to end (SpillRequest.db_index routes completions to the right per-DB cold index); only the on-disk attribution and the recovery attach were db-blind. Fix, three layers: - Manifest: FileEntry gains db_index — a byte-compatible repurpose of the former min_key_hash slot, which every writer in the codebase's history serialized as 0. Manifests written before this field read back as db 0, exactly matching their actual provenance (the old spill buffer was db-blind and recovery attached everything to db 0). No version bump, no migration, v1/v2 record layouts unchanged. - Spill thread: flush_buffer's sub-batch chunking additionally never crosses a logical-DB boundary, so every spill file is single-db and can be attributed wholesale by its manifest entry. Interleaved multi-DB eviction costs extra files, never correctness. The per-entry fallback path stamps the request's own db_index. - Recovery: rebuild_from_manifest_per_db returns one cold index per database; recover_shard_v3 attaches each to ITS database (plus cold_shard_dir) BEFORE Phase 4 tail replay, preserving the replayed DEL/UNLINK/FLUSH*/EXPIRE-past cold-plane tombstone contract per db. A manifest referencing a db beyond the configured --databases count is skipped with a loud warning — never attached to a wrong database. The merged rebuild_from_manifest stays as a thin wrapper for single-db callers. Red/green coverage: flush_buffer db-boundary chunk cut (interleaved dbs -> one file per contiguous same-db run, per-file db matches every entry); rebuild_from_manifest_per_db attribution (two files tagged db 0 / db 3 -> separate indexes, no cross-db keys, merged wrapper unchanged); new real-server kill-9 integration suite crash_recovery_cold_multidb (offload WITHOUT AOF, same key names in db 0 + db 1 with db-distinct values, LRU-spill, SIGKILL, restart: every readable probe must carry its own db's value, db 1 must recover >= half its probes — pre-fix db 1 recovers zero). Fixes #139 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? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDisk-offload manifests now record each spill file’s logical database. Spill batching preserves database boundaries, cold indexes rebuild per database, and recovery attaches them before WAL/AOF replay with compatibility handling for old or out-of-range entries. ChangesCold recovery database attribution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SpillPipeline
participant Manifest
participant ColdIndex
participant Recovery
SpillPipeline->>Manifest: write spill file with db_index
Recovery->>Manifest: read active KvLeaf entries
Recovery->>ColdIndex: rebuild indexes per db_index
ColdIndex->>Recovery: return database-indexed cold indexes
Recovery->>Recovery: attach indexes before WAL/AOF replay
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 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/persistence/recovery.rs`:
- Around line 329-342: Update the recovery flow around
demote_replayed_cold_shadows so it runs for every recovered database, not only
databases.first_mut(). Iterate through all entries in databases and invoke the
reconciliation for each database after Phase 4b AOF replay, preserving the
existing behavior for database zero.
In `@src/storage/tiered/kv_spill.rs`:
- Around line 193-199: Update spill_to_datafile and its synchronous eviction
caller to accept and propagate the caller’s logical db_index, then assign that
value to the spilled file metadata instead of hardcoding 0. Ensure every call
site supplies the appropriate database index so per-db recovery remains
correctly attributed.
🪄 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: a667dcef-db15-42b3-82eb-6e550097d4ee
📒 Files selected for processing (13)
CHANGELOG.mdsrc/persistence/manifest.rssrc/persistence/manifest_sync.rssrc/persistence/recovery.rssrc/shard/mod.rssrc/storage/tiered/cold_index.rssrc/storage/tiered/kv_spill.rssrc/storage/tiered/spill_thread.rssrc/storage/tiered/warm_tier.rstests/crash_recovery_cold_multidb.rstests/crash_recovery_spill_batch_kill9.rstests/crash_recovery_vacuum.rstests/manifest_gc_tombstones.rs
| for (db_index, cold_idx) in per_db { | ||
| info!( | ||
| "Shard {}: rebuilt cold index with {} entries", | ||
| "Shard {}: rebuilt cold index for db {} with {} entries", | ||
| shard_id, | ||
| db_index, | ||
| cold_idx.len() | ||
| ); | ||
| // Attach to the database BEFORE Phase 4 replay. Replayed | ||
| // DEL/UNLINK/FLUSH*/EXPIRE-past must tombstone the cold plane: | ||
| // with `cold_index == None`, `remove_counting_cold()`/`clear()` | ||
| // are silent no-ops (`as_mut()` on None), so a key deleted in | ||
| // the WAL tail resurrects via cold read-through after restart | ||
| // whenever the crash lands inside the pre-orphan-sweep window | ||
| // (the manifest entry is still Active until the sweep). | ||
| if let Some(db0) = databases.first_mut() { | ||
| db0.cold_shard_dir = Some(shard_dir.to_path_buf()); | ||
| match db0.cold_index.as_mut() { | ||
| Some(existing) => existing.merge(cold_idx), | ||
| None => db0.cold_index = Some(cold_idx), | ||
| match databases.get_mut(db_index) { | ||
| Some(db) => { | ||
| db.cold_shard_dir = Some(shard_dir.to_path_buf()); | ||
| match db.cold_index.as_mut() { | ||
| Some(existing) => existing.merge(cold_idx), | ||
| None => db.cold_index = Some(cold_idx), | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Reconcile replayed cold shadows for every recovered database.
This now attaches cold indexes to DBs above zero, but Line 737 still calls demote_replayed_cold_shadows() only on databases.first_mut(). After Phase 4b AOF replay, SELECT N keys can therefore remain duplicated in hot and cold storage, restoring the restart memory-accounting regression for N > 0.
Proposed fix
- if let Some(db0) = databases.first_mut() {
- let demoted = db0.demote_replayed_cold_shadows();
+ let demoted: usize = databases
+ .iter_mut()
+ .map(crate::storage::Database::demote_replayed_cold_shadows)
+ .sum();
- if demoted > 0 {
+ if demoted > 0 {
info!(
"Shard {}: demoted {} AOF-replay hot shadow(s) back to cold-only \
stubs (Phase 4b fallback path, used_memory truthful after restart, task `#56`)",
shard_id, demoted
);
- }
}🤖 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/recovery.rs` around lines 329 - 342, Update the recovery flow
around demote_replayed_cold_shadows so it runs for every recovered database, not
only databases.first_mut(). Iterate through all entries in databases and invoke
the reconciliation for each database after Phase 4b AOF replay, preserving the
existing behavior for database zero.
…ex (#139) Adversarial review of the #139 fix found it incomplete: the SYNC spill path — persistence tick -> run_eviction (non-cascade branch) -> evict_one_with_spill -> kv_spill::spill_to_datafile — still hardcoded db_index: 0 in the manifest FileEntry. An ordinary maxmemory eviction picking a db>0 victim (below the 0.85x cascade threshold, so the db-aware async batch path never engages) produced a spill file that rebuild_from_manifest_per_db re-attached to db 0 on restart: the exact wrong-database recovery corruption #139 describes, reintroduced through a path the integration test's filler burst (which always crosses the cascade threshold) did not exercise. Fix: spill_to_datafile takes an explicit db_index parameter and stamps it into the FileEntry; SpillContext gains a db_index field which run_eviction's per-database loop restamps each iteration (the context is shared across the loop) and the memory-pressure sync fallback sets to its loop index. The async batch path already carried the index on SpillRequest and is unchanged. Red/green: sync_spill_stamps_victims_database_into_manifest evicts a victim through try_evict_if_needed_with_spill with db_index 2 and asserts the manifest entry says 2 — verified failing (entry said 0) against the pre-fix stamp, passing after. Drive-by: collapse a pre-existing clippy `collapsible_if` in tests/crash_matrix_cross_plane/harness.rs (only surfaces under `clippy --all-targets`, which CI's plain clippy gate skips). Refs #139 (PR #383 follow-up) author: Tin Dang <tindang.ht97@gmail.com>
|
Adversarial review outcome: the review REFUTED the original commit as incomplete — one confirmed defect: Fixed in bb11151: Gates re-run post-fix: lib 4392 ✓, both clippys |
2 participants
Summary
Under disk-offload with
SELECT N(N>0), the v3 recovery rebuild merged every spilled key intodatabases[0]'s cold index: SELECT>0 keys answered nil after restart (underappendonly nothe cold tier is their ONLY durable copy), and a db0 key sharing a name with a db1 key could recover the wrong database's value into db0. The live spill path was already db-aware end to end; only the on-disk attribution and the recovery attach were db-blind.Three layers:
FileEntrygainsdb_index— a byte-compatible repurpose of the formermin_key_hashslot, which every writer in the codebase's history serialized as 0. Old manifests read as db 0 (exactly their real provenance). No version bump, no migration.flush_buffer's sub-batch chunking never crosses a logical-DB boundary, so every spill file is single-db and attributable wholesale. Interleaved multi-DB eviction costs extra files, never correctness.rebuild_from_manifest_per_dbreturns one cold index per database;recover_shard_v3attaches each to ITS database (+cold_shard_dir) before Phase 4 tail replay, preserving the DEL/FLUSH cold-tombstone contract per db. A manifest referencing a db beyond--databasesis skipped with a loud warning, never attached to a wrong db.Test plan
"db0 probe:0000: recovered value belongs to the wrong database"(the corruption mode, not just the miss); with the fix it passes.flush_buffer_cuts_chunks_at_db_boundaries(interleaved dbs → one file per contiguous same-db run, per-file db matches every entry);test_rebuild_per_db_attributes_files_to_their_databases(files tagged db0/db3 → separate indexes, no cross-db keys, merged wrapper intact)crash_recovery_cold_multidb(--ignored): offload WITHOUT AOF, same key names in db0+db1 with db-distinct values, LRU spill, SIGKILL, restart — every readable probe carries its own db's value; db1 recovers ≥half its probes (pre-fix: zero)-D warningsboth feature setsFixes #139
Summary by CodeRabbit