fix(storage): #541 review follow-ups — GETEX overflow guard, stale-pair discipline, latch-at-promotion - #550
Conversation
…ir discipline, latch-at-promotion Review findings on the merged expiry-index PR (#549), each verified against the code before fixing: - GETEX EX/EXAT: the seconds->ms `* 1000` could overflow u64 (panic in debug, silent wrap to a bogus TTL in release). checked_mul now routes overflow to the existing range error; EX's absolute-time addition saturates. New test getex_rejects_overflowing_seconds (red pre-fix). - Sweep 1 drops an index pair ONLY when it is provably stale (entry gone or TTL != ts). Previously any failed expiry re-check dropped the pair, which would discard a VALID pair if the wall clock stepped backwards between the cycle-start peek and re-verification (the key would then never actively expire). A valid-but-not-due head pair now ends the sweep (ordered index: nothing later is due either), so loop progress is preserved on every path. peek_due_expiry's doc records the is_expired_at alignment and why re-check failure is not proof of staleness. New test sweep_drops_only_provably_stale_pairs pins that a stale pair is dropped without touching the live entry. - Budget clock read batched: start.elapsed() every 64 pops instead of per key; at least one key still processes before the first check. - hash_field_ttl_latch arms at PROMOTION, not at HEXPIRE success: the NX/XX/GT/LT gate can return -2 after the value already became HashWithTtl (e.g. GT on a non-volatile field), leaving a HashWithTtl with the latch down — breaking the latch's conservativeness invariant the oracle checks. New test hash_field_ttl_latch_arms_even_when_condition_fails (red pre-fix). - Sweep 2 lowers the latch from its own reap outcomes (FieldsRemoved/ NoOp = still eligible; Downgraded/KeyDeleted = not) instead of a second O(N) hashes_with_field_expiry() rescan per tick. - reserve() clears expiry_index + hash latch alongside the table swap, matching clear(); recalculate_memory() assigns the hash latch authoritatively from its scan (a restore WITHOUT HashWithTtl entries now clears a previously raised latch), matching its index rebuild. - debug_expiry_index_consistent is #[cfg(test)] pub(crate) — an oracle, not storage API. - expire_cycle_removes_all_due_keys_among_many_live_ones made deterministic under CI preemption: bounded cycles (<=20), cumulative count. Still red on the sampling sweep (~2 of 50 expected in 20 cycles). Validation: lib 4687/4687 (3 new tests), clippy clean default + tokio+jemalloc legs, fmt clean. author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 6 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
1 participant
Summary
Review follow-ups on the merged expiry-index PR (#549). Every finding was verified against the code before fixing; all nine were valid.
Fixes
× 1000could overflow u64 (debug panic, release silent wrap to a bogus TTL).checked_mulnow routes overflow to the existing range error; EX's absolute-time addition saturates. New testgetex_rejects_overflowing_seconds(red pre-fix).peek_due_expiry's doc records theis_expired_atalignment. New pinning testsweep_drops_only_provably_stale_pairs.start.elapsed()every 64 pops instead of per key; at least one key always processes first.hash_ttl.rs): the NX/XX/GT/LT gate can return-2AFTER the value becameHashWithTtl(e.g. GT on a non-volatile field), leaving aHashWithTtlwith the latch down — breaking the conservativeness invariant the oracle checks. The latch now arms immediately after promotion. New testhash_field_ttl_latch_arms_even_when_condition_fails(red pre-fix).FieldsRemoved/NoOp= still eligible) to lower the latch — the second per-tick O(N) rescan is gone.reserveclearsexpiry_index+ hash latch alongside the table swap, matchingclear;recalculate_memoryassigns the hash latch authoritatively from its scan, so a restore withoutHashWithTtlentries clears a stale latch.debug_expiry_index_consistentis#[cfg(test)] pub(crate)— an oracle, not storage API.Evidence
Lib 4687/4687 (3 new tests), clippy clean both legs, fmt clean; VM monoio suite 5482/5482; VM tokio suite green (rerun on the recreated moon-dev VM — the original run's "failure" was the OrbStack VM vanishing mid-compile, not code). Full dispatch matrix in the checks below.
Follow-up to #549 (#541).