fix(storage): cold-tier correctness + reliability (DEL/FLUSH resurrection, expired-read leak, crash durability, orphan sweep, liveness)

[pilotspacex-byte]

Summary

Five fixes for the disk-offload (cold tier) path, from the offload architecture review (tmp/OFFLOAD-COMPRESSION-REVIEW.md). Each fix was red/green TDD-proven — every new test failed before its fix.

D1 — DEL/UNLINK/FLUSH cold resurrection (P1 bug)

Database::remove/clear() never touched the ColdIndex, so deleting a spilled key left its index entry alive and the next GET resurrected the deleted value from the .mpf heap file (DEL even returned 0 for cold-only keys). remove()/clear() now drop cold entries (queueing file unlinks via the existing refcount/pending-unlink machinery), and DEL/UNLINK use a new Database::remove_counting_cold() so cold-only keys count as removed.

R1 — expired cold reads reclaim their index entry

The cold read path returned a bare Option, so an expired-on-disk entry left its index entry + file refcount leaked forever (nothing else reclaims them — the orphan sweep only checks hot-shadowing). New ColdReadOutcome{Hit, Expired, Miss} lets Database::get remove the index entry on Expired only; transient I/O errors (Miss) never drop a key.

D3 — directory fsync after spill publication

Spill writes fsynced the file but never data/, so a power loss could vanish the directory entry of a file the (dir-fsynced) manifest references. Both the batch (tmp+rename) and single-file spill paths now fsync_directory(data/) after publishing.

R3 — startup sweep of crash-orphaned heap files

A crash between spill write and manifest commit leaked unregistered heap-*.mpf/.tmp files forever. Recovery now unlinks heap files not registered in the manifest — gated on the manifest opening successfully, so a corrupt-manifest signal can never trigger deletion.

R5 — spill-thread liveness metrics

A silently-dead spill thread was observable only as unbounded eviction backlog. INFO persistence now exposes spill_batches_flushed, spill_completions_dropped, spill_last_heartbeat_ms (0 = never ran).

Testing

• 5 new unit tests (3 in cold_read, 1 in kv_spill, 1 in spill_thread), all confirmed RED before the fixes
• macOS: full lib suite 3658 passed
• OrbStack Linux CI-parity matrix green: cargo fmt --check, clippy -D warnings on default and runtime-tokio,jemalloc, cargo test --release (monoio), cargo test --no-default-features --features runtime-tokio,jemalloc (26 suites, 0 failures)
• Transient cluster-test failures during the first VM run were the pre-existing 100ms-sleep harness flake under load (pass 3/3 in isolation and in the clean re-run)

Not in this PR (documented follow-ups in tmp/OFFLOAD-COMPRESSION-REVIEW.md)

• D2 BGREWRITEAOF-drops-cold-keys invariant (decision record + regression test)
• R2 defer-remove on promote, R4 sparse-file compaction
• EXISTS/TYPE/MOVE cold-transparency gaps (siblings of D1)
• A/C performance items (batch read-through, compression upgrades)

[qodo-code-review]

PR Summary by Qodo

Fix cold-tier correctness: DEL/FLUSH, expiry cleanup, fsync, sweep, metrics

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Make DEL/UNLINK/FLUSH clear cold-tier index entries to prevent value resurrection.
• Reclaim expired-on-disk entries by distinguishing Expired vs Miss in cold reads.
• Improve crash-safety and operability: directory fsync, orphan-file sweep, spill liveness metrics.

Diagram

graph TD
  C["Client commands (GET/DEL/UNLINK/FLUSH/INFO)"] --> DB["Database (hot + read-through)"] --> CI["ColdIndex (refs + pending unlink)"]
  DB --> CR["cold_read (Hit/Expired/Miss)"] --> HF["Heap files (heap-*.mpf)"]
  ST["Spill thread"] --> KS["kv_spill (write + dir fsync)"] --> HF
  RC["Recovery (startup)"] --> MF["Shard manifest"] --> SW["Orphan sweep"] --> HF

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make cold_read mutate ColdIndex directly

• ➕ Localizes expiry reclamation logic to cold_read implementation
• ➕ Avoids adding an outcome enum to propagate state upward
• ➖ Requires mutable access to ColdIndex in read path, increasing borrow/locking complexity
• ➖ Harder to keep Database as the single point deciding when deletions are safe

2. Periodic background heap GC instead of startup sweep

• ➕ Cleans up orphans without waiting for restart
• ➕ Can amortize filesystem work over time
• ➖ Adds continuous runtime overhead and new scheduling/failure modes
• ➖ Still needs manifest-safety gating to avoid destructive deletion on corruption

Recommendation: Current approach is appropriate: (1) returning ColdReadOutcome keeps cold_read side-effect free while still enabling Database to reclaim only confidently-expired entries; (2) directory fsync after publication matches the manifest’s durability model; and (3) startup-only orphan sweeping is a low-risk, well-scoped cleanup gated on successful manifest open.

Files changed (9) +478 / -39

Enhancement (2) +74 / -1

connection.rsExpose spill-thread liveness counters via INFO persistence +7/-1

Expose spill-thread liveness counters via INFO persistence

• Extends the INFO persistence section to include spill batch flush count, dropped completion count, and last heartbeat timestamp sourced from spill_thread statics.

src/command/connection.rs

spill_thread.rsAdd spill-thread heartbeat and batch counters with unit test +67/-0

Add spill-thread heartbeat and batch counters with unit test

• Introduces global liveness/throughput metrics (last heartbeat timestamp and batches flushed) and updates the spill loop and flush path to publish them. Adds a unit test asserting heartbeat publication and batch counter increment after a successful spill.

src/storage/tiered/spill_thread.rs

Bug fix (6) +373 / -38

key.rsCount cold-only keys as removed for DEL and UNLINK +9/-2

Count cold-only keys as removed for DEL and UNLINK

• Switches DEL/UNLINK to a new Database removal API that treats cold-only (spilled) keys as logically present. Preserves UNLINK’s async-drop decision by still returning the hot entry when present.

src/command/key.rs

recovery.rsSweep crash-orphaned heap files after successful manifest open +11/-0

Sweep crash-orphaned heap files after successful manifest open

• Invokes tiered storage orphan sweeping during shard recovery to delete heap files not referenced by the manifest. The sweep is gated on manifest open success and only logs failures to avoid aborting recovery.

src/persistence/recovery.rs

db.rsFix cold-tier deletion/flush semantics and reclaim expired disk entries +60/-15

Fix cold-tier deletion/flush semantics and reclaim expired disk entries

• Updates read-through to consume a ColdReadOutcome and remove only truly-expired cold index entries. Ensures clear()/flush clears cold-tier index state, and ensures remove() also drops any cold copy; adds remove_counting_cold for Redis DEL/UNLINK semantics.

src/storage/db.rs

cold_index.rsReturn presence on remove and add clear_all for FLUSH semantics +18/-2

Return presence on remove and add clear_all for FLUSH semantics

• Changes ColdIndex::remove to return whether the key existed and continues to refcount/queue pending unlinks. Adds clear_all() to drop all keys and queue all backing files for unlink during FLUSHDB/FLUSHALL.

src/storage/tiered/cold_index.rs

cold_read.rsIntroduce ColdReadOutcome and tests for delete/flush and expiry reclamation +174/-19

Introduce ColdReadOutcome and tests for delete/flush and expiry reclamation

• Adds ColdReadOutcome {Hit, Expired, Miss} and an outcome-aware cold_read_through API so callers can reclaim index entries only when expiry is confirmed. Hardens error handling to treat I/O/corruption as Miss, and adds unit tests covering cold resurrection prevention, cold-tier flush, and expired-entry reclamation.

src/storage/tiered/cold_read.rs

kv_spill.rsFsync spill directory entries and sweep crash-orphaned heap files +101/-0

Fsync spill directory entries and sweep crash-orphaned heap files

• Adds fsync_directory(data/) after spill file publication and after tmp->final rename to ensure directory entry durability. Introduces sweep_orphan_heap_files() to remove unregistered heap-*.mpf and heap-*.tmp leftovers at startup, with a unit test validating behavior and safety for unrelated files.

src/storage/tiered/kv_spill.rs

Documentation (1) +31 / -0

CHANGELOG.mdDocument cold-tier correctness fixes and new spill liveness metrics +31/-0

Document cold-tier correctness fixes and new spill liveness metrics

• Adds release notes covering cold-tier deletion/flush correctness, expired-read reclamation, directory fsync durability, and startup orphan sweeping. Documents new INFO persistence spill-thread liveness counters.

CHANGELOG.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (2) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 40 rules

1. unwrap() missing allow comment 📘 Rule violation ✧ Quality

Description

New .unwrap() calls were added without the required adjacent // ... justification comment and
#[allow(clippy::unwrap_used)] attribute in scope. This violates the project’s unwrap audit policy
and can cause lint/audit gate failures.

Code

src/storage/tiered/spill_thread.rs[R589-607]

+    fn test_spill_thread_liveness_metrics() {
+        let tmp = tempfile::tempdir().unwrap();
+        let batches_before = spill_batches_flushed_total();
+
+        let st = SpillThread::new(9);
+        let sender = st.sender();
+        sender
+            .send(SpillRequest {
+                key: Bytes::from_static(b"liveness_key"),
+                db_index: 0,
+                value_bytes: Bytes::from_static(b"liveness_value"),
+                value_type: ValueType::String,
+                flags: 0,
+                ttl_ms: None,
+                file_id: 1,
+                shard_dir: tmp.path().to_path_buf(),
+            })
+            .unwrap();
+        drop(sender);

Relevance

⭐⭐ Medium

Unwrap annotation enforced in PR #71, but similar test unwrap-annotation suggestion rejected in PR
#211.

PR-#71
PR-#211

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 302083 requires each .unwrap() to have an immediately preceding justification
comment and an attached #[allow(clippy::unwrap_used)] attribute; the cited new/changed code
contains .unwrap() calls without those annotations.

Rule 302083: Annotate safe unwrap calls with allow and justification
src/storage/tiered/spill_thread.rs[589-607]
src/storage/tiered/cold_read.rs[205-223]
src/storage/tiered/kv_spill.rs[463-480]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New `.unwrap()` calls were introduced without the required `#[allow(clippy::unwrap_used)]` attribute and a one-line justification comment immediately above it.

## Issue Context
PR Compliance requires every `.unwrap()` to be paired with a local allow attribute and an adjacent justification comment.

## Fix Focus Areas
- src/storage/tiered/spill_thread.rs[589-607]
- src/storage/tiered/cold_read.rs[205-223]
- src/storage/tiered/kv_spill.rs[463-480]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unlink lacks dir fsync 🐞 Bug ☼ Reliability

Description

sweep_orphan_heap_files() removes crash-orphaned heap-* files but does not fsync the data/
directory afterward, so a crash can lose the unlink metadata and leave the orphaned files behind
(undermining the sweep’s durability goal). The repo’s own fsync_directory helper explicitly
documents directory fsync as required for unlink metadata durability.

Code

src/storage/tiered/kv_spill.rs[R409-417]

+        if orphan {
+            match std::fs::remove_file(&path) {
+                Ok(()) => {
+                    removed += 1;
+                    tracing::info!("cold-tier sweep: removed crash-orphaned {}", name);
+                }
+                Err(e) => warn!("cold-tier sweep: failed to remove {}: {}", name, e),
+            }
+        }

Relevance

⭐⭐ Medium

Dir-fsync durability fixes accepted (rename) in PR #154; no clear unlink+fsync precedent.

PR-#154

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new startup sweep deletes files via std::fs::remove_file but never calls fsync_directory on
the containing directory. fsync_directory’s doc comment states it is required for unlink metadata
durability, so omitting it means deletions may not persist across a crash.

src/storage/tiered/kv_spill.rs[381-420]
src/persistence/fsync.rs[8-13]
PR-#154

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`sweep_orphan_heap_files()` unlinks files from `{shard_dir}/data` but never fsyncs the directory. On filesystems that require an explicit directory fsync for metadata durability, the unlink can be lost after a crash, leaving the crash-orphaned files behind.

### Issue Context
The project’s own helper documents directory fsync as required for **rename/unlink metadata durability**.

### Fix Focus Areas
- src/storage/tiered/kv_spill.rs[381-420]

### Suggested fix
After the sweep loop, if `removed > 0`, do a best-effort `fsync_directory(&data_dir)`.
- Keep the sweep’s “never abort recovery” contract by logging fsync errors (warn) instead of returning early.
- Optionally, only fsync when at least one unlink succeeded (to avoid extra syscalls on no-op sweeps).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Tests added outside mod.rs 📘 Rule violation ▣ Testability

Description

New unit tests were added inside leaf submodule files under a split module (src/storage/tiered/),
rather than being centralized in the directory’s mod.rs. This violates the project convention for
split-module test placement.

Code

src/storage/tiered/cold_read.rs[R233-304]

+    #[test]
+    fn test_del_removes_cold_entry_no_resurrection() {
+        let tmp = tempfile::tempdir().unwrap();
+        let mut db = db_with_spilled_key(tmp.path(), b"doomed", b"value-on-disk", None);
+
+        // Sanity: the key is reachable via cold read-through before DEL.
+        assert!(
+            db.cold_index.as_ref().unwrap().lookup(b"doomed").is_some(),
+            "precondition: key is cold-indexed"
+        );
+
+        let frame = crate::command::key::del(
+            &mut db,
+            &[crate::protocol::Frame::BulkString(Bytes::from_static(
+                b"doomed",
+            ))],
+        );
+        assert_eq!(
+            frame,
+            crate::protocol::Frame::Integer(1),
+            "DEL of a cold-only key must count it as removed"
+        );
+        assert!(
+            db.get(b"doomed").is_none(),
+            "GET after DEL must NOT resurrect the cold value"
+        );
+        assert!(
+            db.cold_index.as_ref().unwrap().lookup(b"doomed").is_none(),
+            "cold index entry must be gone after DEL"
+        );
+        assert!(
+            db.cold_index.as_ref().unwrap().has_pending_unlink(),
+            "last referrer removed: file must be queued for unlink"
+        );
+    }
+
+    /// D1: FLUSHDB/FLUSHALL (`Database::clear`) must clear the cold tier too —
+    /// flushed keys must not remain readable from disk.
+    #[test]
+    fn test_clear_flushes_cold_tier() {
+        let tmp = tempfile::tempdir().unwrap();
+        let mut db = db_with_spilled_key(tmp.path(), b"flushed", b"value-on-disk", None);
+
+        db.clear();
+
+        assert!(
+            db.get(b"flushed").is_none(),
+            "GET after FLUSH must NOT read the cold value back from disk"
+        );
+        assert!(
+            db.cold_index.as_ref().unwrap().has_pending_unlink(),
+            "cold files must be queued for unlink after clear"
+        );
+    }
+
+    /// R1: a cold read that finds the entry EXPIRED must reclaim the index
+    /// entry (and thereby the file refcount) instead of leaking it forever.
+    #[test]
+    fn test_expired_cold_read_reclaims_index_entry() {
+        let tmp = tempfile::tempdir().unwrap();
+        // TTL 1ms in the past relative to the read below.
+        let mut db = db_with_spilled_key(tmp.path(), b"stale", b"old", Some(1));
+
+        assert!(
+            db.get(b"stale").is_none(),
+            "expired cold entry reads as nil"
+        );
+        assert!(
+            db.cold_index.as_ref().unwrap().lookup(b"stale").is_none(),
+            "expired cold entry must be reclaimed from the index on read"
+        );
+    }

Relevance

⭐ Low

Similar “move tests to mod.rs” suggestion was definitely rejected in PR #211.

PR-#211

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 302093 requires split-module unit tests to be located in the module directory’s
mod.rs; the tiered module is split via src/storage/tiered/mod.rs, but the PR adds tests in leaf
files under that directory.

Rule 302093: Keep test code for split Rust modules in mod.rs
src/storage/tiered/mod.rs[1-10]
src/storage/tiered/cold_read.rs[233-304]
src/storage/tiered/kv_spill.rs[462-502]
src/storage/tiered/spill_thread.rs[547-622]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tests were added in non-`mod.rs` files within a split Rust module directory.

## Issue Context
The module `src/storage/tiered/` is split via `mod.rs` and submodules; compliance requires unit tests for split modules to live in the directory’s `mod.rs`, not in leaf files.

## Fix Focus Areas
- src/storage/tiered/mod.rs[1-10]
- src/storage/tiered/cold_read.rs[233-304]
- src/storage/tiered/kv_spill.rs[462-502]
- src/storage/tiered/spill_thread.rs[547-622]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

1. unwrap() missing allow comment 📘 Rule violation ✧ Quality

New .unwrap() calls were added without the required adjacent // ... justification comment and
#[allow(clippy::unwrap_used)] attribute in scope. This violates the project’s unwrap audit policy
and can cause lint/audit gate failures.

Agent Prompt

## Issue description
New `.unwrap()` calls were introduced without the required `#[allow(clippy::unwrap_used)]` attribute and a one-line justification comment immediately above it.

## Issue Context
PR Compliance requires every `.unwrap()` to be paired with a local allow attribute and an adjacent justification comment.

## Fix Focus Areas
- src/storage/tiered/spill_thread.rs[589-607]
- src/storage/tiered/cold_read.rs[205-223]
- src/storage/tiered/kv_spill.rs[463-480]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

3. Unlink lacks dir fsync 🐞 Bug ☼ Reliability

sweep_orphan_heap_files() removes crash-orphaned heap-* files but does not fsync the data/
directory afterward, so a crash can lose the unlink metadata and leave the orphaned files behind
(undermining the sweep’s durability goal). The repo’s own fsync_directory helper explicitly
documents directory fsync as required for unlink metadata durability.

Agent Prompt

### Issue description
`sweep_orphan_heap_files()` unlinks files from `{shard_dir}/data` but never fsyncs the directory. On filesystems that require an explicit directory fsync for metadata durability, the unlink can be lost after a crash, leaving the crash-orphaned files behind.

### Issue Context
The project’s own helper documents directory fsync as required for **rename/unlink metadata durability**.

### Fix Focus Areas
- src/storage/tiered/kv_spill.rs[381-420]

### Suggested fix
After the sweep loop, if `removed > 0`, do a best-effort `fsync_directory(&data_dir)`.
- Keep the sweep’s “never abort recovery” contract by logging fsync errors (warn) instead of returning early.
- Optionally, only fsync when at least one unlink succeeded (to avoid extra syscalls on no-op sweeps).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[coderabbitai]

Warning

Review limit reached

@pilotspacex-byte, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 30 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1c1388b1-46ad-4cb5-9e45-50fccdf70898

📥 Commits

Reviewing files that changed from the base of the PR and between eadc8b8 and 92390a7.

📒 Files selected for processing (9)

• CHANGELOG.md
• src/command/connection.rs
• src/command/key.rs
• src/persistence/recovery.rs
• src/storage/db.rs
• src/storage/tiered/cold_index.rs
• src/storage/tiered/cold_read.rs
• src/storage/tiered/kv_spill.rs
• src/storage/tiered/spill_thread.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/cold-tier-correctness

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}