fix(storage): TTL millisecond fidelity — entry expiry stored as ms, fossil base_ts plumbing removed (W3)#398
Merged
Merged
Conversation
|
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 (21)
✨ 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 |
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? |
This was referenced Jul 20, 2026
TinDang97
force-pushed
the
refactor/w3-ttl-ms
branch
from
July 20, 2026 05:33
6fc7600 to
edbc9e6
Compare
TinDang97
force-pushed
the
refactor/w2-spill-pipeline
branch
from
July 20, 2026 05:33
4f4af29 to
6716c4d
Compare
pilotspacex-byte
changed the base branch from
refactor/w2-spill-pipeline
to
main
July 20, 2026 05:34
…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
TinDang97
force-pushed
the
refactor/w3-ttl-ms
branch
from
July 20, 2026 05:36
edbc9e6 to
5b9efaa
Compare
TinDang97
added a commit
that referenced
this pull request
Jul 20, 2026
…ionRun replace the 13-name try_evict_if_needed* family (W4) The eviction API had grown 13 public variants that encoded every optional parameter combination in the function name: try_evict_if_needed try_evict_if_needed_budget[_reporting] try_evict_if_needed_with_spill[_and_total[_budget[_reporting]]] try_evict_if_needed_async_spill[_with_total][_budget][_reporting] try_evict_if_needed_async_spill_with_total_budget[_reporting] Five of them already had zero external callers (chain-internal only), and the two "cores" at the bottom of each forwarding chain duplicated the same reclaim-loop skeleton five times (sync plain, sync batch, async plain-fallback, async durable-batch, async AOF-fast-path). This PR replaces the family with one entry point: evict_to_budget(db, config, EvictionRun) -> Result<(), Frame> where EvictionRun is an options struct built from three constructors (plain / sync_spill / async_spill = the EvictionSink choice) plus chainable knobs .total(n) (aggregate accounting), .budget(n) (elastic per-shard override, capped at instance maxmemory), and .report(f) (plain-drop DEL emission, task #34). The reclaim loop now exists once with a per-iteration sink dispatch; the --appendonly durability- ordering branches (AOF-backstopped fast path vs durable-batch vs plain-drop fallback) are byte-for-byte the same decisions, now documented in one place on EvictionSink::AsyncSpill. Pure refactor — no behavior change: - Red-first: three new entry-point behavior pins (plain sink honors noeviction-OOM + reports every plain-dropped victim; sync-spill sink batches 40 victims into shared .mpf files, all cold-readable; elastic override respected and clamped to maxmemory) written RED against the missing API, then GREEN. - All ~27 call sites across 9 files migrated (connection handlers, SPSC cross-shard gate, Lua bridge, blocking path, memory-pressure cascade, eviction tick, db_quota docs); every historical eviction pin test migrated and green. - Stale doc references to the old names scrubbed repo-wide. Gates: lib suite 4425 green (4422 + 3 new pins) on macOS + Linux VM release-fast, monoio + tokio,jemalloc feature sets; crash matrix + kill-9 offload suites on Linux VM; clippy -D warnings on both feature sets; fmt. Stacked on refactor/w3-ttl-ms (PR #398). author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 20, 2026
…ionRun replace the 13-name try_evict_if_needed* family (W4) (#399) The eviction API had grown 13 public variants that encoded every optional parameter combination in the function name: try_evict_if_needed try_evict_if_needed_budget[_reporting] try_evict_if_needed_with_spill[_and_total[_budget[_reporting]]] try_evict_if_needed_async_spill[_with_total][_budget][_reporting] try_evict_if_needed_async_spill_with_total_budget[_reporting] Five of them already had zero external callers (chain-internal only), and the two "cores" at the bottom of each forwarding chain duplicated the same reclaim-loop skeleton five times (sync plain, sync batch, async plain-fallback, async durable-batch, async AOF-fast-path). This PR replaces the family with one entry point: evict_to_budget(db, config, EvictionRun) -> Result<(), Frame> where EvictionRun is an options struct built from three constructors (plain / sync_spill / async_spill = the EvictionSink choice) plus chainable knobs .total(n) (aggregate accounting), .budget(n) (elastic per-shard override, capped at instance maxmemory), and .report(f) (plain-drop DEL emission, task #34). The reclaim loop now exists once with a per-iteration sink dispatch; the --appendonly durability- ordering branches (AOF-backstopped fast path vs durable-batch vs plain-drop fallback) are byte-for-byte the same decisions, now documented in one place on EvictionSink::AsyncSpill. Pure refactor — no behavior change: - Red-first: three new entry-point behavior pins (plain sink honors noeviction-OOM + reports every plain-dropped victim; sync-spill sink batches 40 victims into shared .mpf files, all cold-readable; elastic override respected and clamped to maxmemory) written RED against the missing API, then GREEN. - All ~27 call sites across 9 files migrated (connection handlers, SPSC cross-shard gate, Lua bridge, blocking path, memory-pressure cascade, eviction tick, db_quota docs); every historical eviction pin test migrated and green. - Stale doc references to the old names scrubbed repo-wide. Gates: lib suite 4425 green (4422 + 3 new pins) on macOS + Linux VM release-fast, monoio + tokio,jemalloc feature sets; crash matrix + kill-9 offload suites on Linux VM; clippy -D warnings on both feature sets; fmt. Stacked on refactor/w3-ttl-ms (PR #398). 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
Third PR of the storage-unification campaign (stacked on #397 ← #396). Fixes finding F5 (TTL truncation) and finishes F6's fossil-parameter cleanup.
The bug (F5)
CompactEntrystored key expiry as seconds in itsu64TTL field (ms / 1000floor on everyset_expires_at_ms):PEXPIRE key 1500expired at the 1000 ms boundary — every millisecond-granularity TTL fired up to 999 ms early.PTTLreported the second-truncated deadline, not the one the client set.The fix
Store absolute Unix milliseconds directly (the
u64was already paid for; 32-byte entry layout and const assert unchanged;0stays the no-expiry sentinel).is_expired_atcompares in ms;PTTLreads back the exact deadline. Red-firstttl_millisecond_fidelitytest pins exact readback + boundary behavior (alive at deadline−1 ms, dead at deadline); the RDB round-trip test now asserts exact ms equality. The oldsaturating_mul/.max(1)guards became obsolete (no arithmetic on the stored value remains).On-disk formats untouched — RDB/spill/AOF always carried absolute ms; the truncation happened only at the in-memory
Entryboundary.Fossil
base_tsplumbing removed (F6)The expiry API carried an ignored
base_ts: u32parameter since expiry became absolute. Gone:is_expired_at/expires_at_ms/set_expires_at_ms/new_string_with_expiryparams + ~60 dead call-site args + deadlet base_ts = …bindings across command/persistence/storage/shardDatabase::check_expired/is_expired,rdb::write_entry,redis_rdb::write_rdb_entryfossil paramsSnapshotState.base_timestampsfield (+ ctor params + the per-tick Vecpersistence_tickbuilt to fill it)u32in the AOF-rewrite/BGSAVE snapshot tuple end-to-end:AofFoldSnapshot.dbs, the shard-thread builder,save_from_snapshot/save_snapshot_to_bytes, both BGSAVE builders, the rewrite temp-db loopmerge_shard_snapshots(zero callers)Net −128 lines.
Gates
ttl_millisecond_fidelityconfirmed RED pre-fix, GREEN postcrash_matrix_cross_plane46/46,spill_batch_kill92/2,disk_offload_no_aof1/1 on Linux VM with pinned fresh-ELFMOON_BIN-D warnings×2 feature sets · fmt · CHANGELOG entry