refactor(storage): one spill pipeline — sync eviction uses the durable batch writer (W2)#397
Merged
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
This was referenced Jul 20, 2026
TinDang97
force-pushed
the
refactor/w1-value-codec
branch
from
July 20, 2026 05:33
81599ed to
3f4fe04
Compare
TinDang97
force-pushed
the
refactor/w2-spill-pipeline
branch
from
July 20, 2026 05:33
4f4af29 to
6716c4d
Compare
…the durable batch writer (W2) Moon had two spill families: the async/batch path (SpillThread flush_buffer: shared multi-entry files, one manifest commit per file) and a sync per-victim path (kv_spill::spill_to_datafile: ONE single-entry file + ONE shard-thread-blocking durable manifest commit PER KEY) used by the background eviction tick, the memory-pressure cascade's sync fallback, and db_quota. The two evolved independently and that seam already bit three times: the task #34 plain-drop misclassification, the task #45 is_string gate, and the #139 spill-test blindness were all divergence bugs between these copies. This PR makes the batch machinery the ONE spill implementation: - select_victim (policy dispatch) and build_spill_payload (victim serialization) exist once — previously copy-pasted at all three entry points. - evict_batch_durable_no_aof is renamed evict_batch_durable and now backs every SpillContext sync path: the sync wrapper reclaims its whole deficit as batches; evict_one_with_spill's spill arm is a deficit-1 delegation. Write-then-durable-then-drop ordering, D1 remove-before-cold-index, and fail-closed retention on I/O error are inherited, not re-implemented. - Red-first: sync-evicting 40 keys now produces 1 shared .mpf file (was 40 files + 40 blocking manifest fsync round-trips). - Fail-closed serialization (red-first): all three sites previously did serialize_collection(..).unwrap_or_default() — a corrupt value was "spilled" as an EMPTY body and evicted (silent durable data loss on reload). Unserializable victims are now retained hot, skipped, and loudly logged, on the sync, batch, and async paths. - evict_batch_durable now calls record_eviction() per removed key (the batch path previously under-counted the eviction metric). - kv_spill::spill_to_datafile is no longer a production eviction path; it remains the single-entry primitive test fixtures use (same page layout the batch salvage path emits). Gates: lib suite 4421 green (macOS + Linux VM release-fast); eviction pin tests (spill-I/O-failure OOM retention, non-string durable spill, db_index restamp) all green; offload/cold integration suites + crash matrix on Linux VM; clippy -D warnings on default and tokio,jemalloc feature sets. Stacked on refactor/w1-value-codec (PR #396). author: Tin Dang
TinDang97
force-pushed
the
refactor/w2-spill-pipeline
branch
from
July 20, 2026 05:35
6716c4d to
568e2d9
Compare
TinDang97
added a commit
that referenced
this pull request
Jul 20, 2026
…ossil base_ts plumbing removed (W3) CompactEntry stored key expiry as SECONDS in its u64 ttl field (`ms / 1000` floor on every set_expires_at_ms). Consequences: - PEXPIRE key 1500 expired at the 1000 ms boundary — every millisecond-granularity TTL fired up to 999 ms EARLY. - PTTL reported the second-truncated deadline, not the one the client set. - The RDB round-trip test needed a 5-second tolerance to pass. The u64 was already paid for; this stores absolute Unix milliseconds in it directly (0 = no expiry, unchanged sentinel; 32-byte entry layout and const assert unchanged). is_expired_at compares in ms, expires_at_ms/PTTL read back the exact deadline, and the RDB round-trip test now asserts exact millisecond equality (red-first: ttl_millisecond_fidelity pinned exact readback + boundary behavior at deadline-1 ms and deadline). Second commit-sized cleanup folded in: the expiry API carried a fossil base_ts: u32 parameter (ignored since expiry became absolute) on is_expired_at / expires_at_ms / set_expires_at_ms / new_string_with_expiry. The parameter is gone, along with ~60 dead call-site arguments across command/persistence/storage/shard, the dead `let base_ts = …` bindings that fed them, Database:: check_expired's and is_expired's fossil params, rdb::write_entry's and redis_rdb::write_rdb_entry's fossil params, and the never-read SnapshotState.base_timestamps field (plus its constructor params and the per-tick Vec that persistence_tick built to fill it). The AOF rewrite snapshot type shrank from Vec<(Vec<(Key, Entry)>, u32)> to Vec<Vec<(Key, Entry)>> end-to-end (AofFoldSnapshot.dbs, the shard- thread builder, save_from_snapshot / save_snapshot_to_bytes, both BGSAVE builders, and the rewrite temp-db loop) — every consumer ignored the u32. merge_shard_snapshots (zero callers) is deleted. On-disk formats are untouched: RDB/spill/AOF always carried absolute milliseconds — the truncation happened only at the in-memory Entry boundary on load/set. Gates: lib suite 4422 green (macOS + Linux VM release-fast, monoio + tokio,jemalloc feature sets); clippy -D warnings on both feature sets; fmt; crash-matrix + offload suites on Linux VM. Stacked on refactor/w2-spill-pipeline (PR #397). author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 20, 2026
…ossil base_ts plumbing removed (W3) (#398) CompactEntry stored key expiry as SECONDS in its u64 ttl field (`ms / 1000` floor on every set_expires_at_ms). Consequences: - PEXPIRE key 1500 expired at the 1000 ms boundary — every millisecond-granularity TTL fired up to 999 ms EARLY. - PTTL reported the second-truncated deadline, not the one the client set. - The RDB round-trip test needed a 5-second tolerance to pass. The u64 was already paid for; this stores absolute Unix milliseconds in it directly (0 = no expiry, unchanged sentinel; 32-byte entry layout and const assert unchanged). is_expired_at compares in ms, expires_at_ms/PTTL read back the exact deadline, and the RDB round-trip test now asserts exact millisecond equality (red-first: ttl_millisecond_fidelity pinned exact readback + boundary behavior at deadline-1 ms and deadline). Second commit-sized cleanup folded in: the expiry API carried a fossil base_ts: u32 parameter (ignored since expiry became absolute) on is_expired_at / expires_at_ms / set_expires_at_ms / new_string_with_expiry. The parameter is gone, along with ~60 dead call-site arguments across command/persistence/storage/shard, the dead `let base_ts = …` bindings that fed them, Database:: check_expired's and is_expired's fossil params, rdb::write_entry's and redis_rdb::write_rdb_entry's fossil params, and the never-read SnapshotState.base_timestamps field (plus its constructor params and the per-tick Vec that persistence_tick built to fill it). The AOF rewrite snapshot type shrank from Vec<(Vec<(Key, Entry)>, u32)> to Vec<Vec<(Key, Entry)>> end-to-end (AofFoldSnapshot.dbs, the shard- thread builder, save_from_snapshot / save_snapshot_to_bytes, both BGSAVE builders, and the rewrite temp-db loop) — every consumer ignored the u32. merge_shard_snapshots (zero callers) is deleted. On-disk formats are untouched: RDB/spill/AOF always carried absolute milliseconds — the truncation happened only at the in-memory Entry boundary on load/set. Gates: lib suite 4422 green (macOS + Linux VM release-fast, monoio + tokio,jemalloc feature sets); clippy -D warnings on both feature sets; fmt; crash-matrix + offload suites on Linux VM. Stacked on refactor/w2-spill-pipeline (PR #397). author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
W2 of the storage unification wave. Stacked on #396 (W1) — merge that first; this PR's base is
refactor/w1-value-codec.Moon had two spill families: the async/batch path (
flush_buffer: shared multi-entry files, one manifest commit per file) and a sync per-victim path (spill_to_datafile: one single-entry.mpffile + one shard-thread-blocking durable manifest commit per key) used by the eviction tick and the memory-pressure cascade. The seam between them caused three past bugs (task #34 defect 1, task #45is_stringgate, #139 test blindness). This PR makes the batch machinery the only spill implementation.select_victim(policy dispatch) andbuild_spill_payload(victim serialization) exist once — were copy-pasted ×3.evict_batch_durable_no_aof→evict_batch_durable, now backing everySpillContextsync path;evict_one_with_spill's spill arm is a deficit-1 delegation, inheriting (not re-implementing) write-then-durable-then-drop, D1 ordering, and fail-closed I/O-error retention.unwrap_or_default()spilled an EMPTY value body and evicted the key (silent durable data loss) at all three sites.spill_to_datafiledemoted to the single-entry test-fixture primitive.Gates (Linux VM, release-fast, MOON_BIN pinned to fresh ELF)
-- --ignored: 46/46-D warnings× both feature sets; all historical eviction pin tests green