fix(storage): cold index rebuild reports every entry it cannot recover; indexed-but-unreadable is no longer a miss (#875)

[TinDang97]

STACKED on #996 (fix/983-coldindex-duplicate-resolution). Base branch is that branch, not main. #996 must merge first; then retarget this PR to main. Do not squash-merge this PR while #996 is open — in this repo a squash of a stacked child has landed its parent silently.

Summary

Fixes moon#875, both halves.

Rebuild side. ColdIndex::rebuild_from_manifest_per_db skipped, with no log line, no counter and no error: a heap file that failed to read (Err(_) => continue), a page that failed its magic/type/CRC check (from_bytes → None, next page), and a trailing partial page (chunks_exact discards the remainder). All three confirmed independently at the cited lines. Two more silent paths found on the way, both now covered: a slot inside a CRC-valid page that does not decode (KvLeafPage::get → None: an unknown ValueType, i.e. a downgrade), and a file truncated on a page boundary, invisible to every per-page check and detectable only against the manifest's byte_size. One framing correction to the issue: KvLeafPage::from_bytes also returns None for a perfectly valid KvOverflow page, so "warn on every None" would have reported every large value as corruption; the rebuild classifies the header first.

Read side — the part that decides whether this class is detectable. A cold read-through that found the key indexed but its bytes unreadable was folded into the plain miss: GET nil, INCR minted a counter from zero, HSET/LPUSH/APPEND/… fabricated a fresh value that shadowed the cold copy until the orphan sweep reclaimed the file for good. Now such a key answers -IOERR cold tier: key is indexed but its data could not be read (see server log) on both dispatch paths, writes refuse before mutating, the index entry is retained so the read heals when the bytes come back, and the fault is counted (reclamation_cold_read_unreadable_total) and logged with its exact location.

Decision per rebuild path (not one blanket rule)

path	now	why
file NotFound	warn + files_missing + the file_id queued on the rebuilt index's pending_unlink so the orphan sweep retires the manifest entry	Legitimately produced by the sweep's own unlink-before-manifest-commit crash window (nothing lost: every key in a zero-ref file has a newer copy or was deleted), or by external removal (nothing recoverable). Without the heal it re-warns on every boot forever — the moon#546a pattern.
any other read error (EACCES, EIO, …)	error + files_unreadable; file skipped, never tombstoned	The class an operator can usually fix; a restart after the fix recovers the keys (proved). Refusing to boot would be stronger, but a recovery Err today falls back to v2 recovery, discarding the whole v3 replay — strictly worse — and shard init has no refuse-boot path (#996 hit the same wall). Follow-up.
file shorter than manifest byte_size	error + files_short/short_file_bytes	Whole pages gone; the only detector. Gated on byte_size > 0 so a legacy zero-stamped entry cannot false-alarm.
bad header / foreign type / CRC mismatch	error + pages_rejected; page skipped	Correct decision, wrong volume. The bytes are gone; no boot refusal brings them back.
trailing partial page	error + partial_page_bytes	The writer cannot produce one (temp + fsync + rename): post-rename damage. Not a debug_assert! — a dev build must not panic on disk damage during recovery.
undecodable slot	error + entries_rejected	Realistic on downgrade.
manifest fails to open (Phase 3)	error stating the consequence	Phase 2 warned about the open; nothing said "no cold index at all, every spilled key of this shard reads as absent". Now it does.

Per-file/page lines are capped at 16 per cause per rebuild (with the file path); totals go into one per-shard summary (cold index rebuild clean / cold index rebuild DEGRADED) and into INFO: reclamation_cold_recovery_{files_missing,files_unreadable,files_short,pages_rejected,partial_page_bytes,entries_rejected}_total.

The read-side reply: what and why

ColdReadOutcome::Miss now means only "no index entry" — the one outcome that is absence. New Unreadable(ColdReadFault { location, reason }) covers file missing / unreadable, page rejected, slot undecodable, overflow chain broken, body undecodable.

Mechanism, kept off the accessor signatures (Database::get(&mut self) -> Option<&Entry> has no error channel and dozens of callers): Database::cold_fault (AtomicU8 — the Database lives in an RwLock slot and must stay Sync) is raised by the two read-through funnels (promote_cold_outcome, get_cold_value) and consumed by whoever answers:

• the six fabricating accessors (get_or_create* and the listpack/intset siblings) refuse with the error before insert_fresh;
• INCR*/INCRBYFLOAT, APPEND, SETRANGE, GETSET, SET … GET|KEEPTTL refuse before their set;
• command::cold_fault_gate on dispatch and dispatch_read turns any remaining reply into the error — the catch-all for every Database::get-shaped read. One relaxed load per command when nothing is pending. The third path, try_inline_dispatch, stands down on any cold location and cannot raise the flag.

Why -IOERR and not nil: "key not found" is a legitimate answer a client acts on (re-derive, overwrite, report missing), so a lost entry answering nil is undetectable above the storage layer; an error is not. Why not fail at EXISTS/TYPE/DBSIZE: they are index-driven and correct — the key is present. Escape hatches: plain SET still overwrites (it never reads the old value), DEL discards. Known imperfection, documented in the commit: on the tokio shared-read path another connection's dispatch_read can theoretically interleave between the async GET pre-warm's raise and this connection's read; the pre-warm's own re-read re-raises for the right connection and the other gets a spurious IOERR on a disk that is already failing.

Evidence

Pre-fix control: $HOME/ab856-target/release/moon (release build of a8eb2efc). Fixed: release-fast build of c347a7b9. macOS host; no benchmark numbers.

Integration — tests/cold_index_rebuild_silent_drops_875.rs, MOON_BIN pinned, --test-threads=1:

test	pre-fix a8eb2efc	fixed
rebuild_reports_every_drop_{1,4}_shard(s) — spill → BGREWRITEAOF (cold file becomes the only copy) → SIGKILL → one file removed, one chmod 000, one page CRC broken, one file cut 100 B into a page → restart	all four keys GET -> None; EXISTS -> false, then FAILED: INFO reclamation has no reclamation_cold_recovery_files_missing_total field	same four nils (bytes are gone), then files_missing=1 files_unreadable=1 pages_rejected=1 partial_page_bytes=100 files_short=1, every damaged file named in the log with its path, DEGRADED summary; after chmod 644 + restart the unreadable key reads back — ok
live_damage_answers_ioerr_not_nil_{1,4}_shard(s) — three keys tiered; one file removed and one chmod 000 under the running server	GET on the indexed key → $-1 — FAILED	GET → -IOERR …, APPEND → -IOERR, INCR → -IOERR, EXISTS 1, reclamation_cold_read_unreadable_total=7/6, control key served, PING unaffected, after chmod 644 the original value is served (the refused APPEND fabricated nothing), DEL → 1 then nil — ok

Totals: pre-fix 0 passed; 4 failed; fixed 4 passed; 0 failed (both shard counts).

The nil answers in the rebuild test are printed on purpose: the bytes are gone, no rebuild can conjure them; what changes is that the loss is now visible. The live test is where the reply itself changes.

Harness lessons: (a) under --appendonly yes without a rewrite the AOF's own SET rebuilds the key hot and MOON.SPILLED only drops it if the index still maps the key to that file — cold damage is masked until the first rewrite; (b) tracing_subscriber::fmt() writes to stdout, with ANSI codes even into a file (the parent harness sent stdout to /dev/null).

Unit — cold_index_rebuild_tests.rs (one test per loss class, corpus from the spill thread's own writers), cold_read.rs (Miss vs Unreadable, index retained, and dispatch-level: GET/STRLEN on both paths → IOERR, twelve refusing writes leave nothing hot and the index entry in place, PING after IOERR unaffected, SET/DEL escape hatches, the value returns once the file is back): 21 passed in cold_read::tests + cold_index_rebuild_tests; 72 passed across cold_index cold_read info_reclamation kv_spill::tests.

Attack (each mutation applied, compiled, targeted tests run, file restored from a pristine copy; git diff HEAD --stat → 0 lines afterwards; restored tree re-run green):

mutation	result
A1 NotFound no longer counted	RED — missing_file_is_counted_and_queued_for_manifest_retirement
A2 overflow pages counted as rejected	RED — overflow_pages_are_expected_not_rejected
A3 missing file not queued for manifest retirement	RED — same test, on pending_unlink_len
A4 partial trailing page ignored (tail = 0)	RED — trailing_partial_page_is_counted_in_bytes
A5 read-side fault folded back into Miss	RED — 3 tests incl. the page-cache premise check
W1 dispatch gate a no-op	RED — indexed_but_unreadable_key_answers_ioerr…
W2 shared-read funnel does not raise the flag	RED — same
W3 generic get_or_create<K> guard inverted	first run: NOT detected — HSET/LPUSH/SADD/ZADD all take the listpack/intset siblings, so the generic site was untested. Added XADD (streams have no compact sibling) to the refusing-writes list; re-attack: RED — same test (the accessor fabricated a stream while the flag was still pending)
W4 INCR mints a counter over unreadable bytes	RED — same

Gates (final tree c03956fb): cargo fmt --check exit 0 · cargo clippy --all-targets -- -D warnings exit 0 · cargo check --all-targets --no-default-features --features runtime-tokio,jemalloc exit 0. Not run: scripts/ci-local.sh (VM legs), hosted matrix dispatch, Linux/io_uring, redis-server oracle (the -IOERR reply has no Redis counterpart — Redis has no cold tier; the prefix is Redis's own for a failed disk read).

Out of scope, flagged

• Boot refusal on an unreadable heap file needs an error path out of shard init that does not fall back to v2 (same finding as fix(storage): cold index rebuild resolves a duplicated key by file_id, not manifest order (#983) #996's seed caveat).
• moon#997 (file_id seed fails open → rename over a live file) and moon#965 (stale value after SIGKILL, AOF tail loss) are adjacent and untouched; the files_short detector would fire on a Spill file_id seed fails open: a failed cold-dir scan restarts numbering at 1 and the next spill renames over a live heap file #997 overwrite whose manifest byte_size no longer matches — a useful side effect, not a fix.
• tracing_subscriber::fmt() emits ANSI escapes into a non-TTY stdout; an operator grepping a log file hits the same thing the harness did. Cheap to fix (with_ansi(is_terminal)), separate change.
• The tokio shared-read interleaving noted above; a per-connection fault slot would close it, at the cost of threading connection identity into Database.

Refs moon#875

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

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

[TinDang97]

Independent verification — my own build of c03956fb

Built the pushed commit into a clean target dir and ran the committed test against two
servers, swapping only MOON_BIN. md5 confirms the binaries differ.

PRE-FIX  (a8eb2efc)   test result: FAILED. 0 passed; 4 failed    real exit 101
POST-FIX (this PR)    test result: ok.     4 passed; 0 failed    real exit 0

The pre-fix failure message is the whole issue in one line:

GET k875:live:file-removed: indexed but unreadable must answer -IOERR, got "$-1\r\n"

$-1 is a nil — the server telling the client the key does not exist, when in fact it is
indexed and its data is unreadable. That is the conflation this PR removes, and it fails
at both --shards 1 and --shards 4.

I tried to build my own probe first and it was vacuous — which is worth recording

Before falling back to your test, I wrote my own: spill keys to cold, chmod 000 the heap
files, restart, read. It reported no difference between the two binaries. Two separate
reasons, both of which your harness already handles and mine did not:

1. The AOF masked the damage. My first dataset had a 23 MB appendonlydir; every
   SET replayed on restart, so the cold files were never consulted. Your report calls
   this out explicitly — the AOF's own SET masks cold damage until the first rewrite —
   and your test rewrites before crashing. Mine did not.
2. With --appendonly no, my control was already nil. Undamaged, both binaries
   returned nil for all 80 keys, because those keys never reached the one heap file that
   got written. A control that already sits at the failure value cannot distinguish
   anything, and had I not run the undamaged leg I would have reported "no difference" as
   a finding.

I am recording this because it is the same class as the --pipe trap I hit on #993 and
the single-%K route_probe that let moon#962 hide: an instrument that cannot express the
broken case. Your harness earning its complexity — the AOF rewrite, the specific key
placement — is the point, and it is worth a comment in the test saying why the rewrite is
there, so nobody later "simplifies" it away and silently disarms the whole file.

On the rest of the report

Three things I want to note as good practice rather than just agree with:

• You found two drop paths beyond the three in the issue (an undecodable slot inside a
  CRC-valid page, and a file truncated on a page boundary that only the manifest's
  byte_size can detect), and you corrected the issue text — KvLeafPage::from_bytes
  also returns None for a valid KvOverflow page, so the issue's implied "warn on every
  None" would have flagged every large value as corruption. That is a correction to the
  premise, which is more valuable than the fix.
• Deciding per path rather than one blanket rule is right, and the NotFound →
  pending_unlink heal in particular avoids the moon#546a pattern of warning forever about
  the same condition.
• W3 was not caught on your first attack run, and you said so. HSET/LPUSH/SADD/ZADD
  take the listpack/intset siblings, so the generic get_or_create<K> guard was untested;
  adding XADD (streams have no compact sibling) made it red. An attack that silently
  passes because it never reached the guard is exactly the failure mode attacks exist to
  prevent, and catching that in your own work is the hard part.

Scope

EXISTS/TYPE/DBSIZE still reporting the key as present is correct — it is present; only
its data is unreadable. And retaining the index entry so the read heals after a chmod 644
is the right call over evicting it.

The documented tokio shared-read interleave, and boot refusal on unreadable heap files
needing an error path out of shard init, are both correctly left as follow-ups — the latter
is the same wall #996 hit, which suggests it is worth solving once for both.

scripts/ci-local.sh has not been run and the hosted matrix has not been dispatched.