disk-offload: failed cold-manifest commit can lose cold keys on crash (apply_spill_completions)

[pilotspacex-byte]

Summary

apply_spill_completions (src/shard/persistence_tick.rs ~407-414) populates the in-memory cold_index unconditionally, but if the preceding manifest.commit() returns Err it only logs a warning and continues. Cold read-through works in the live process (the index is populated), but if the process crashes between the failed commit and the next successful one, rebuild_from_manifest rebuilds from on-disk state that does not include the file's entry — those cold keys are silently lost (the .mpf exists on disk but nothing indexes it after restart).

Severity

LOW-MEDIUM. Pre-existing; surfaced in the PR #136 self-review. Window is narrow (failed commit → crash before next commit) but the failure is silent.

Suggested fix

On commit() failure either (a) retry/escalate before populating the index, or (b) record the pending entry in a recovery-visible side-log so a crash in the window can rebuild it. At minimum, surface a metric/counter for failed cold-manifest commits.

Context

Found during PR #136 review (multishard stability + disk-offload durability). Reviewer-reported, not independently reproduced.

[pilotspacex-byte]

CodeRabbit PR #136 review (4420584384) sharpens this — two distinct Critical triggers, both validated against merged code:

1. Inline flush wholesale failure (spill_thread.rs:222-234): the pre-screen only checks value_bytes.len(), so a large key or incompressible ~3.5KB value can make build_kv_spill_batch() return InvalidData; that turns ONE bad candidate into a failed completion for the ENTIRE inline buffer — whose keys are already evicted from RAM. Should retry the offender via build_kv_spill_pages() / split the batch instead of dropping the whole flush.
2. Completion drop (spill_thread.rs:407-419, send_one_completion): try_send(Full) drops the SpillCompletion, so the file is never added to the manifest and keys are never reinserted into cold_index — permanent loss after restart (keys already out of RAM). Needs blocking/spill-to-disk backpressure, not a drop.

Being fixed on branch fix/coderabbit-pr136-durability.

[pilotspacex-byte]

Closing — fixed by commit 6cac58fd ("block instead of dropping spill
completions (data loss)", explicitly fixing this issue) and deae60d6
("salvage inline-batch spill failures per-entry, not wholesale").
apply_spill_completions (src/shard/persistence_tick.rs) is now backed by
a blocking send instead of drop-on-full, closing the crash window that could
lose cold keys.

Author: Tin Dang tindang.ht97@gmail.com