fix(memory): tick-path eviction spills collections instead of plain-dropping (task #45)

[pilotspacex-byte]

Summary

Task #45 — root cause of the recurring Linux-CI stable-GHOST flake (cold_collection_visibility: EXISTS=1 but LRANGE/ZRANGE empty). Three compounding defects:

1. evict_one_with_spill (src/storage/eviction.rs) gated spilling behind a stale is_string check even though spill_to_datafile had full collection support — collection victims under a live SpillContext were silently plain-dropped.
2. Even spilled string victims never got a ColdIndex entry — durable file existed but was unreachable.
3. The periodic tick (shard::timers::run_eviction) never received a SpillContext at all — every tick-evicted collection was plain-dropped even with disk-offload + appendonly enabled. persistence_tick.rs now builds a real SpillContext from the shard manifest when disk-offload is enabled, fail-close otherwise (policy semantics unchanged).

Verification

• Deterministic red→green unit tests for both the sync-spill path and the tick path (no timing race).
• cold_collection_visibility: 10/10 green on monoio + tokio locally, 5/5 on Linux VM.
• Full lib 4254 green, 42/42 crash matrix (pre-P1-3: streaming cold-tier vector decode (avoid bulk Vec allocation) #46/P2-1: Cargo feature gate for disk-offload (~50 KLOC always compiles in) #50 fork), fmt + clippy both matrices (agent + independent VM re-run).
• Rebased onto main post-fix(persistence): adopt atomic_write_durable at 8 remaining bare-write sites (task #49) #304; hot-path waiver: tick/eviction path only, inline KV path untouched.

Fixes the flake class that has been rerolled manually on multiple PRs (incl. #299/main runs 29189049422, 29186983648). Task #44 (crash_recovery_disk_offload_no_aof pre-existing failure) may be affected — will recheck after merge.

Refs: task #45, #41/#8 precedents.

Summary by CodeRabbit

• Bug Fixes
  • Collection values evicted during routine maintenance are now spilled to durable storage instead of being silently dropped when disk offload is configured.
  • Successfully spilled values are registered for later retrieval.
  • Failed spill operations now retain the active value rather than deleting it.
  • Eviction continues to safely fall back to dropping values when no durability backstop is available.
• Tests
  • Added regression coverage for collection spilling and periodic eviction behavior.

[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]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 629c76a9-9811-4336-9614-11e2045f34e9

📥 Commits

Reviewing files that changed from the base of the PR and between 7fe6884 and f4fc924.

📒 Files selected for processing (4)

• CHANGELOG.md
• src/shard/persistence_tick.rs
• src/shard/timers.rs
• src/storage/eviction.rs

---

📝 Walkthrough

Walkthrough

Eviction now durably spills collection victims when a SpillContext is available, registers successful spills in ColdIndex, and retains hot values when spill I/O fails. Periodic eviction ticks construct and pass spill context when disk offload and a manifest are available.

Changes

Eviction spill behavior

Layer / File(s)	Summary
Collection spill and cold-index registration src/storage/eviction.rs	evict_one_with_spill spills all value types, fails closed on spill errors, registers successful spills in ColdIndex, and updates regression tests.
Periodic eviction spill context src/shard/timers.rs, src/shard/persistence_tick.rs, CHANGELOG.md	Eviction APIs accept and forward optional spill context; run_eviction_tick constructs it for configured disk offload and updated tests cover tick-path spilling.

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

Sequence Diagram(s)

sequenceDiagram
  participant run_eviction_tick
  participant run_eviction
  participant evict_one_with_spill
  participant ColdIndex
  run_eviction_tick->>run_eviction: pass SpillContext when configured
  run_eviction->>evict_one_with_spill: forward SpillContext
  evict_one_with_spill->>ColdIndex: register successful spill

Loading

Possibly related PRs

• pilotspace/moon#270: Related durable spill and fail-closed eviction changes.
• pilotspace/moon#285: Related collection spill behavior and eviction call-chain updates.
• pilotspace/moon#287: Related ColdIndex handling for evicted collections.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly reflects the main change: tick-path eviction now spills collections instead of plain-dropping them.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/tick-eviction-spill

Warning

Tools execution failed with the following error:

Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error)

---

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