fix: don't disable TTL cleanup when a scan error interrupts CleanExpiredKeys - #485
fix: don't disable TTL cleanup when a scan error interrupts CleanExpiredKeys#485thweetkomputer wants to merge 1 commit into
Conversation
…redKeys
The expired-key collection loop was `do {...} while (iter.Next() ==
NoError)`, so a real IO error while paging the TTL tree (EIO, cloud fetch
failure) was treated the same as reaching the end: the loop stopped
having seen only a prefix of the expired keys, then committed the partial
deletion with next_expire_ts_ = 0. TriggerTTL early-returns on
next_expire_ts_ == 0 and is the only producer of TTL work, so the
partition's TTL cleanup stayed disabled until a later TTL upsert /
RootMeta reload / reopen / restart re-armed it -- the remaining expired
keys lingered on disk and stayed readable, silently.
Distinguish EndOfFile (genuine end -> next_expire_ts_ = 0 is correct) from
a real error. On a real error, abort without committing so next_expire_ts_
stays armed and the next TriggerTTL retries. The Seek() error above
already made this distinction; the Next() loop did not.
Happy-path TTL behavior is unchanged (delete.cpp [TTL] tests pass); the
error path has no clean deterministic hook (async cleanup + scan-path
fault injection), so it is not separately tested.
|
Warning Review limit reached
Next review available in: 35 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
1 participant
Problem
CleanExpiredKeyscollects expired keys withdo { ... } while (iter.Next() == KvError::NoError).ScanIterator::Next()returnsEndOfFileat the end of the TTL tree but a real error (IoFail,Corrupted, …) when paging it fails (EIO, a cloud fetch failure). The loop treats both the same: it stops having seen only a prefix of the expired keys, then commits the partial deletion and setsnext_expire_ts_ = 0.TriggerTTLearly-returns onnext_expire_ts_ == 0, and it is the only producer of TTL work (there is no periodic timer). So after one transient scan error the partition's TTL cleanup stays disabled until a later TTL upsert /RootMetareload / reopen / restart re-armsnext_expire_ts_. The unscanned expired keys linger on disk and — since reads don't filter expired keys server-side — stay readable, with no error surfaced (the cleanup request is fire-and-forget). Consistency is fine (both trees update atomically); it's a silent space/staleness leak, unbounded in a partition that stops receiving TTL writes.The
Seek()error just above the loop already distinguishesEndOfFilefrom a real error; theNext()loop didn't.Fix
Capture
Next()'s result. On a real error (notEndOfFile), abort without committing sonext_expire_ts_stays armed (still<= now) and the nextTriggerTTLretries the whole scan;EndOfFilestill commitsnext_expire_ts_ = 0as before (genuinely nothing left to expire — re-armed later by new TTL upserts viaUpdateTTL).Test
Happy-path TTL behavior is unchanged —
delete.cpp[TTL]tests pass. The error path itself has no clean deterministic hook (the cleanup runs as an async task and would need scan-path fault injection), so it isn't separately tested.