fix(storage): truthful used_memory under disk-offload — RSS→logical ledger, stub-only recovery, replay demote (task #56)

[pilotspacex-byte]

Summary

Task #56 (v0.8 item 2): make used_memory truthful under disk-offload. The G2 acceptance run reported 406–762MB against a 256MB cap, worse after kill-9 restart. Three independent bugs, diagnosed via an instrumented 2-shard/8MB/40k-key repro:

1. used_memory was process RSS, not the logical ledger --maxmemory eviction gates on. It now reports the same kv(+ColdIndex)+vector+text+graph used-term the elastic budget gates on, plus Lua script cache and replication backlog for Redis parity; used_memory_rss/used_memory_peak unchanged; new moon_used_memory_bytes Prometheus gauge; MEMORY DOCTOR explains gated-vs-RSS-vs-outside-the-cap.
2. Restart double-loaded spilled String keys hot (recovery.rs Phase 3): a pre-ColdIndex loop re-inserted every spilled String into the hot DashTable while the newer mechanism rebuilt the cold stub for the same key. All types now recover cold-stub-only with lazy promotion on first read.
3. AOF replay re-heated cold keys (spilled keys get no AOF DEL, so replay reapplies their historical SETs hot — the "worse after restart" ~6× transient). Database::demote_replayed_cold_shadows reconciles post-replay, pre-serving.

Adversarial review trail (DO-NOT-SHIP → SHIP)

An independent adversarial review found and the fix round closed:

• CRITICAL: overwrite-then-crash resurrected stale cold data (SET on a cold-shadowed key never cleared ColdIndex). Fixed crash-consistently: Database::set() clears the shadow on InsertOrUpdate::Updated; sound at replay because replay starts from an empty table — an Updated proves a later AOF record exists. Re-review verified coverage across SETRANGE/bit-ops/INCR/GETSET/collection-writes/RESTORE/COPY/Lua/MULTI.
• HIGH: demote was inert under runtime-tokio + --shards 1 (no manifest branch); now wired in recover_shard_v3_pitr Phase 4b.
• MEDIUM: parity gap (Lua cache + repl backlog) — included in the reported figure; eviction gate unchanged.

Verification

• tests/used_memory_offload_truthful.rs (12 tests): spill → SIGKILL → restart; used_memory ≤ 1.75× maxmemory at steady state AND post-restart
• tests/cold_shadow_overwrite_resurrection.rs: overwrite cold key → kill-9 → GET returns the NEW value (RED on pre-fix HEAD)
• tests/cold_shadow_single_shard_tokio.rs: same, against a real tokio binary
• All gates: fmt, clippy ×4 configs (default/tokio × lib/--tests), cargo test --lib 4332 passed, crash-matrix + cold-DEL regression suites green
• Operator docs: docs/guides/monitoring.md — what counts against --maxmemory and what doesn't

Summary by CodeRabbit

• Bug Fixes

  • Corrected used_memory reporting during disk offload to reflect logical memory usage rather than process RSS.
  • Prevented stale or duplicated cold data from returning after overwrites, AOF replay, or restart recovery.
  • Improved memory accounting to include Lua scripts and replication backlog.
  • Expanded MEMORY DOCTOR with clearer memory breakdowns.

• Documentation

  • Added guidance distinguishing logical memory, eviction budget, and process RSS.
  • Updated monitoring examples for accurate memory alerts.

• Tests

  • Added regression coverage for crash recovery, overwrites, replay, and memory reporting.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 40 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: ad7847ac-a4ce-4e80-9e75-aa3c71764141

📥 Commits

Reviewing files that changed from the base of the PR and between ae45e2f and fc56c7b.

📒 Files selected for processing (11)

• CHANGELOG.md
• docs/guides/monitoring.md
• src/admin/metrics_setup.rs
• src/command/connection.rs
• src/command/server_admin.rs
• src/main.rs
• src/persistence/recovery.rs
• src/storage/db.rs
• tests/cold_shadow_overwrite_resurrection.rs
• tests/cold_shadow_single_shard_tokio.rs
• tests/used_memory_offload_truthful.rs

📝 Walkthrough

Walkthrough

Disk-offload memory reporting now uses a logical ledger instead of RSS. Recovery invalidates stale cold shadows and demotes redundant replayed hot copies across replay paths. Monitoring documentation, changelog entries, unit tests, and ignored end-to-end crash-recovery tests cover the updated behavior.

Changes

Disk-offload correctness

Layer / File(s)	Summary
Logical memory accounting src/admin/metrics_setup.rs, src/command/connection.rs, src/command/server_admin.rs, docs/guides/monitoring.md	Adds logical used-memory accounting for storage, Lua cache, and replication backlog; exposes it through metrics, INFO, MEMORY DOCTOR, and monitoring guidance.
Cold-shadow invalidation and demotion src/storage/db.rs	Clears stale cold-index entries after updates and demotes redundant hot replay copies while preserving cold read-through.
Replay recovery demotion src/main.rs, src/persistence/recovery.rs	Runs cold-shadow reconciliation after multi-part AOF and legacy fallback replay paths.
Regression coverage and release documentation tests/cold_shadow_*.rs, tests/used_memory_offload_truthful.rs, CHANGELOG.md	Adds unit and ignored integration coverage for overwrite resurrection, replay recovery, and bounded logical memory usage, and documents the fixes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AOFReplay
  participant Recovery
  participant Database
  AOFReplay->>Recovery: complete replay
  Recovery->>Database: demote_replayed_cold_shadows()
  Database->>Database: remove redundant hot copies
  Database-->>Recovery: return demoted count

Loading

Possibly related PRs

• pilotspace/moon#67: Overlaps with the INFO and metrics memory-accounting paths.
• pilotspace/moon#236: Modifies the recovery replay path where post-replay demotion is added.
• pilotspace/moon#257: Addresses related disk-offload crash-recovery and cold-plane resurrection behavior.

Suggested reviewers: tindang97

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The summary is strong, but the required Checklist, Performance Impact, and Notes sections are missing.	Add the missing template sections, including test status in Checklist, a Performance Impact note, and a Notes section.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main storage and disk-offload memory-reporting changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/t56-used-memory-offload

---

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.}

[coderabbitai]

🧹 Nitpick comments (2)

src/persistence/recovery.rs (1)

703-726: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Iterate over all databases for consistency.

While disk offload currently targets databases[0], iterating over all databases matches the loop structure in main.rs and ensures future-proofing if disk offload is expanded to multiple databases.

♻️ Proposed refactor

-    if let Some(db0) = databases.first_mut() {
-        let demoted = db0.demote_replayed_cold_shadows();
-        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
-            );
-        }
-    }
+    let mut total_demoted = 0;
+    for db in databases.iter_mut() {
+        total_demoted += db.demote_replayed_cold_shadows();
+    }
+    if total_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, total_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 703 - 726, Update the Phase 4b
fallback reconciliation to iterate over every database in databases instead of
only databases.first_mut(). Call demote_replayed_cold_shadows on each database
and log each positive demotion count with the corresponding shard context,
preserving the existing behavior and message for each database.

src/storage/db.rs (1)

1290-1345: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

File exceeds the 1500-line cap; this PR adds further to an already-oversized file.

demote_replayed_cold_shadows and its doc comment (plus the two new tests later in the file) are correct, but they land in a src/storage/db.rs that's already several times past the documented 1500-line limit. Consider splitting cold-tier-specific Database methods (promotion/demotion/cold accessors) and their tests into a dedicated submodule.

As per coding guidelines, "No single Rust file should exceed 1500 lines. Split command-group files into directory modules when approaching the limit; split read and write implementations above 1000 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/storage/db.rs` around lines 1290 - 1345, Extract cold-tier-specific
Database methods, including demote_replayed_cold_shadows, promotion/demotion
logic, and cold accessors, into a dedicated storage submodule while preserving
their existing visibility and behavior. Move the associated cold-tier tests with
the implementation, and keep src/storage/db.rs under the 1500-line limit without
changing unrelated Database functionality.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@src/persistence/recovery.rs`:
- Around line 703-726: Update the Phase 4b fallback reconciliation to iterate
over every database in databases instead of only databases.first_mut(). Call
demote_replayed_cold_shadows on each database and log each positive demotion
count with the corresponding shard context, preserving the existing behavior and
message for each database.

In `@src/storage/db.rs`:
- Around line 1290-1345: Extract cold-tier-specific Database methods, including
demote_replayed_cold_shadows, promotion/demotion logic, and cold accessors, into
a dedicated storage submodule while preserving their existing visibility and
behavior. Move the associated cold-tier tests with the implementation, and keep
src/storage/db.rs under the 1500-line limit without changing unrelated Database
functionality.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f8d41d92-cccc-4b9a-8027-a9fcd757650c

📥 Commits

Reviewing files that changed from the base of the PR and between 8f5e0c9 and ae45e2f.

📒 Files selected for processing (11)

• CHANGELOG.md
• docs/guides/monitoring.md
• src/admin/metrics_setup.rs
• src/command/connection.rs
• src/command/server_admin.rs
• src/main.rs
• src/persistence/recovery.rs
• src/storage/db.rs
• tests/cold_shadow_overwrite_resurrection.rs
• tests/cold_shadow_single_shard_tokio.rs
• tests/used_memory_offload_truthful.rs