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

CHANGELOG.md

@@ -132,6 +132,127 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   iterations clean (`10 passed; 0 failed`, 40.3s) — these are the same cells that, pre-fix,
   failed at iteration 7/20 and 11/20 in the kernel M3 stage 2 investigation.
 
+### Fixed
+- **`used_memory` truthfulness under disk-offload (task #56).** The G2
+  acceptance run (4 shards, `--maxmemory 256MB`, 2.6GB dataset,
+  disk-offload enabled) reported `INFO` `used_memory` at 406-762MB against
+  the 256MB cap, and worse after a kill-9 restart. Three independent bugs,
+  found by instrumenting a 2-shard/8MB-cap/40k-key macOS repro
+  (`tests/used_memory_offload_truthful.rs`) at load, steady state, and
+  post-restart:
+  - **`used_memory` was literally process RSS** (`src/command/connection.rs`),
+    not the logical ledger `--maxmemory` eviction actually gates on. RSS
+    always carries allocator arena overhead, mmap'd cold-read page-cache
+    frames, thread stacks, the binary image, the (intentionally unbounded)
+    Lua script cache, and the replication backlog ring — none of which the
+    eviction system charges against the cap, so `used_memory` was
+    guaranteed to read high under any disk-offload workload even when
+    eviction was correctly holding the real ledger under budget. `INFO`'s
+    `used_memory` now reports the same KV(+ColdIndex)+vector+text+graph
+    "used-term" `ShardDatabases::recompute_elastic_budget` already gates on
+    (`admin::metrics_setup::logical_used_memory_bytes`); `used_memory_rss`
+    / `used_memory_peak` still report the true OS footprint alongside it, a
+    new `moon_used_memory_bytes` Prometheus gauge mirrors the ledger figure,
+    and `MEMORY DOCTOR` gained an explicit "gated vs RSS vs outside-the-cap"
+    header so the two numbers are never conflated again.
+  - **Restart double-loaded every spilled `String` key into hot RAM**
+    (`src/persistence/recovery.rs`): v3 recovery Phase 3 re-inserted each
+    previously-spilled `ValueType::String` entry directly into the hot
+    DashTable (`#79-04`, predating cold read-through) AND — two days later,
+    unrelated to the first change — rebuilt a `ColdIndex` stub for the same
+    key pointing at the same on-disk location (`#80-02`); nobody removed
+    the first loop when the second, correct mechanism landed. Every value
+    type now recovers as a cold-index-only stub, exactly like
+    Hash/List/Set/ZSet/Stream already did; the first `GET` lazily promotes
+    a key into hot RAM (and only then charges `used_memory`) via the same
+    `Database::promote_cold_if_present` path ordinary (non-restart) cold
+    read-through uses.
+  - **AOF replay re-hydrated already-cold keys back into hot RAM on every
+    restart** (`src/main.rs`, `src/storage/db.rs`): fixing the bug above
+    exposed a second, independent restart-time re-charge path that only
+    surfaces when `--appendonly yes` *and* disk-offload are both active.
+    An evicted-and-spilled key gets no AOF `DEL` record — a spilled entry
+    stays cold-readable, not deleted, so `record_reason_del` never fires
+    for it — so the AOF's incr log still holds that key's original `SET`.
+    AOF replay (`main.rs`'s per-shard and single-shard multi-part branches)
+    has no cold-tier awareness and blindly reapplies every historical `SET`
+    into the hot DashTable, so a restart transiently re-inflated
+    `used_memory` to ~6x the steady-state figure (self-healing over
+    several eviction ticks, but proportionally slower to catch up the
+    larger the disk-offloaded dataset — the "got WORSE after restart"
+    symptom on the real 2.6GB G2 dataset). `Database::demote_replayed_cold_shadows`
+    reconciles the two right after AOF replay finishes and before the
+    server accepts connections: any key still present in the (already
+    crash-consistent) `ColdIndex` at that point is provably redundant with
+    whatever AOF replay just wrote hot for it (replay cannot re-cold a key,
+    so the last AOF command touching a crash-time-cold key must be exactly
+    the SET the eviction path spilled), so the redundant hot copy is
+    dropped and `used_memory` no longer spikes at all post-restart.
+  `tests/used_memory_offload_truthful.rs` drives all three mechanisms
+  end-to-end (spill past `--maxmemory`, confirm real disk spill, SIGKILL,
+  restart) and asserts `used_memory` stays within 1.75x `--maxmemory` at
+  steady state AND immediately post-restart. See `docs/guides/monitoring.md`
+  for the updated operator story on what counts against `--maxmemory` and
+  what doesn't.
+- **Adversarial-review follow-up on task #56 (three findings, all fixed on
+  the same branch):**
+  - **CRITICAL — stale cold-shadow resurrection on live overwrite.** The
+    `demote_replayed_cold_shadows` reconciliation above assumed any key
+    still present in the recovered `ColdIndex` after AOF replay must be
+    crash-time-cold and untouched. False whenever a key is spilled at v1
+    and then *live-overwritten* to v2 with no further eviction before a
+    crash: the manifest still shows it spilled at v1, so a restart
+    rebuilds `ColdIndex[key] = v1`, AOF replay reconstructs `hot[key] =
+    v2`, and the (pre-fix) demote pass wrongly dropped the hot v2,
+    resurrecting stale v1 on the next `GET`. `Database::set`
+    (`src/storage/db.rs`) now clears the key's `ColdIndex` shadow the
+    instant a SECOND write to that key is observed
+    (`InsertOrUpdate::Updated`, not `Inserted`) — provably safe because AOF
+    replay always starts from an empty DashTable (`db.clear()` runs before
+    every replay branch), so the first replayed write to a key is always
+    `Inserted` (left alone; it may be exactly the write that later got
+    spilled) and any subsequent write to the same key during that same
+    replay can only happen if the AOF recorded a write *after* the one
+    that got spilled — proving the cold copy stale. No new record type or
+    spill-file format change needed. Covered by the new unit test
+    `storage::db::tests::test_second_write_invalidates_cold_shadow` and
+    the new end-to-end kill-9 test
+    `tests/cold_shadow_overwrite_resurrection.rs`.
+  - **HIGH — demotion never wired into the no-manifest (tokio +
+    `--shards 1`) recovery branch.** `demote_replayed_cold_shadows` was
+    only invoked from the manifest-based AOF replay branches in
+    `src/main.rs`; under `runtime-tokio` + `--shards 1` no manifest is
+    ever created (`AofManifest::initialize` is monoio-only there), so the
+    only KV replay for that runtime/shard combination — the Phase 4b
+    `appendonly.aof` fallback inside `recover_shard_v3_pitr`
+    (`src/persistence/recovery.rs`) — never demoted anything, leaving that
+    config just as exposed to the AOF-replay-rehydration bug as the
+    manifest-based paths were before task #56. The demote call is now
+    also invoked at the end of Phase 4b. Covered by the new
+    tokio+jemalloc-feature integration test
+    `tests/cold_shadow_single_shard_tokio.rs`
+    (`single_shard_overwritten_cold_key_returns_new_value_after_crash`),
+    using `SETEX` rather than a bare `SET` per the documented
+    monoio-write-gate/inline-SET-path gotcha.
+  - **MEDIUM — `used_memory` parity gap: Lua script cache and replication
+    backlog.** Real Redis's `used_memory` is "total allocator-attributed
+    memory", not "memory eviction can reclaim" — it counts the Lua script
+    cache and replication backlog even though neither is evictable.
+    Decision: include both in the reported figure rather than document an
+    exclusion list, since both were already tracked as separate
+    `moon_memory_bytes{kind="lua_scripts"}` /
+    `{kind="replication_backlog"}` gauges — folding them into
+    `logical_used_memory_bytes()` (`src/admin/metrics_setup.rs`) and the
+    `moon_used_memory_bytes` gauge cost no new instrumentation. The actual
+    `--maxmemory` eviction gate (`ShardDatabases::recompute_elastic_budget`)
+    is deliberately left unchanged and narrower — eviction has no
+    mechanism to reclaim Lua bytecode or backlog bytes, so gating on them
+    would free nothing. `MEMORY DOCTOR` (`src/command/server_admin.rs`)
+    now prints three distinct figures (elastic budget / used_memory
+    reported / RSS) instead of conflating the first two; see the expanded
+    `docs/guides/monitoring.md` "`used_memory` vs RSS under disk-offload"
+    section.
+
 ### Documentation
 - **Roadmap Rev 2 (task #68, doc half).** `docs/roadmap/ROADMAP.md` updated to
   post-v0.7.1 actuals: v0.6.1/v0.7.0 marked shipped (with the R5/R6 slips and the

docs/guides/monitoring.md

@@ -62,13 +62,79 @@ scrape_configs:
 Moon exposes standard Redis-compatible INFO metrics through the Prometheus endpoint. Key metrics to monitor include:
 
 - **`moon_connected_clients`** -- current number of connected clients
-- **`moon_used_memory_bytes`** -- total memory used by the server
+- **`moon_used_memory_bytes`** -- the logical memory ledger reported as
+  `INFO`'s `used_memory` field, matching real Redis's `used_memory`
+  semantics: KV (+ its ColdIndex bookkeeping) + vector/text/graph resident
+  bytes, **plus** the Lua script cache and the replication backlog ring.
+  This is NOT quite the same figure `--maxmemory` eviction gates on (see
+  the elastic-budget note below) -- it is deliberately wider, because real
+  Redis's `used_memory` counts Lua scripts and the replication backlog too
+  even though neither is data eviction can reclaim.
+- **Elastic budget (eviction gate)** -- `ShardDatabases::recompute_elastic_budget`,
+  not directly exposed as its own gauge, is the NARROWER figure
+  `--maxmemory` eviction actually acts on: KV+ColdIndex+vector+text+graph
+  only, with NO Lua/replication-backlog terms. In the overwhelming majority
+  of deployments (small script cache, small/no replication backlog) this is
+  indistinguishable from `moon_used_memory_bytes`; it only diverges when
+  either of those two subsystems grows large, in which case
+  `moon_used_memory_bytes` can legitimately read above what eviction is
+  bounding -- `MEMORY DOCTOR` prints both figures side by side for exactly
+  this reason.
+- **`moon_rss_bytes`** -- the process's true OS-level resident set size.
+  Under disk-offload this is expected to run noticeably ABOVE
+  `moon_used_memory_bytes` -- the remaining gap (RSS minus `used_memory`,
+  which now already includes Lua + replication backlog) is allocator arena
+  fragmentation, mmap'd page-cache frames serving cold-tier reads, and the
+  binary image and thread stacks. **Do not alert on `moon_rss_bytes /
+  <maxmemory>` and expect it to track eviction health** -- use
+  `moon_used_memory_bytes` for that; use `moon_rss_bytes` (and
+  `moon_memory_bytes{kind="allocator_overhead"}` /
+  `{kind="pagecache"}`) to watch total OS footprint / capacity planning.
+- **`moon_memory_bytes{kind="..."}`** -- the same breakdown `MEMORY DOCTOR`
+  prints, per subsystem: `dashtable`, `hnsw` (vector), `text`, `csr`
+  (graph), `wal`, `sealed`, `replication_backlog`, `lua_scripts`,
+  `pagecache`, `allocator_overhead`. The first six (dashtable/hnsw/text/csr
+  + replication_backlog + lua_scripts) sum to `moon_used_memory_bytes`;
+  `pagecache` and `allocator_overhead` are the remaining components that
+  explain the `moon_rss_bytes` gap.
 - **`moon_commands_processed_total`** -- total commands processed (rate = ops/sec)
 - **`moon_keyspace_hits_total`** -- successful key lookups
 - **`moon_keyspace_misses_total`** -- failed key lookups (cache miss rate)
 - **`moon_evicted_keys_total`** -- keys evicted due to maxmemory
 - **`moon_expired_keys_total`** -- keys removed by expiration
 
+### `used_memory` vs RSS under disk-offload
+
+A disk-offloaded deployment intentionally keeps most of its dataset off the
+hot ledger (on disk, in `heap-*.mpf` files), read back on demand. Three
+figures that are easy to conflate but answer different questions:
+
+| Field | Answers | Gated by `--maxmemory`? |
+|---|---|---|
+| Elastic budget (`recompute_elastic_budget`, shown in `MEMORY DOCTOR` only) | "How much of my logical dataset does the eviction system currently think is resident?" | Yes -- this is the exact number eviction reads |
+| `used_memory` (INFO) / `moon_used_memory_bytes` | "How much allocator-attributed memory does Redis-parity `used_memory` report?" (elastic budget + Lua scripts + replication backlog) | Mostly -- diverges from the elastic budget only when the script cache or replication backlog is large |
+| `used_memory_rss` (INFO) / `moon_rss_bytes` | "How much physical RAM does this process actually hold?" | No -- always some amount above `used_memory` |
+
+Before task #56, `used_memory` was implemented as raw process RSS, so all
+three rows above were identical and a healthy disk-offload deployment
+permanently looked 1.5-3x over its configured cap. If you see this gap in an
+older build, it is a reporting bug, not a memory leak -- upgrade rather than
+raise `--maxmemory` to compensate.
+
+**Why `used_memory` includes Lua scripts and the replication backlog but
+`--maxmemory` eviction does not (adversarial-review finding, task #56):**
+real Redis's `used_memory` is "total allocator-attributed memory", not
+"memory eviction can reclaim" -- it counts the Lua script cache and the
+replication backlog even though eviction never touches either. Moon matches
+that semantics for the reported figure (both terms were already tracked as
+separate `moon_memory_bytes{kind=...}` gauges, so folding them into
+`used_memory` cost nothing new), while keeping the actual eviction gate
+(`recompute_elastic_budget`) unchanged and narrower -- eviction has no
+mechanism to reclaim Lua bytecode or replication-backlog bytes, so gating on
+them would not free anything. If your script cache or replication backlog
+is large enough for the two figures to diverge meaningfully, `SCRIPT FLUSH`
+and reviewing `repl-backlog-size` are the levers, not raising `--maxmemory`.
+
 ## Grafana dashboard
 
 Import the metrics into Grafana for visualization. A minimal dashboard should include:
@@ -132,7 +198,12 @@ groups:
           summary: "Moon instance {{ $labels.instance }} is down"
 
       - alert: MoonHighMemory
-        expr: moon_used_memory_bytes / moon_maxmemory_bytes > 0.9
+        # Moon does not (yet) publish a `--maxmemory` gauge -- substitute
+        # your instance's configured byte value here. Use
+        # `moon_used_memory_bytes` (the gated logical ledger), NOT
+        # `moon_rss_bytes` (always higher under disk-offload by design --
+        # see "used_memory vs RSS under disk-offload" above).
+        expr: moon_used_memory_bytes / <your-maxmemory-bytes> > 0.9
         for: 5m
         labels:
           severity: warning