fix(storage): skip caching splits larger than the split cache budget - #6635
fix(storage): skip caching splits larger than the split cache budget#6635zchataiwala-abstract wants to merge 6 commits into
Conversation
The searcher on-disk split cache had no per-split size guard: it would download a split even when the split was larger than the whole cache budget (`split_cache.max_num_bytes`). A single oversized split therefore transiently wrote its full size to the searcher disk and evicted the entire small-split working set to (fail to) make room. Add a size-based guard to the split cache: - The download loop now skips any candidate whose file size exceeds `max_num_bytes` and advances to the next-best fitting candidate, rather than stalling on a permanently-hot oversized split (which would starve the whole cache). Eviction also accounts for the incoming split's size, so on-disk usage no longer overshoots the budget by a whole split. - The split file size is plumbed to the cache two ways: a new `ReportSplit.num_bytes` field populated by the indexer, and the search open path (`open_split_bundle`), so pre-existing splits that were never reported with a size are guarded the first time they are queried. - An unknown size (0) is treated as "fits", so behavior is byte-for-byte unchanged when the size is not known. - Add a counter for downloads skipped because the split exceeds the budget. Oversized splits are served by the existing cold-storage range warmup, so search results are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4192615fcb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ndidates Address review feedback on the split-cache size guard: - Reserve the reported size of in-flight downloads against the budget, so with `num_concurrent_downloads > 1` two sub-budget splits can no longer both start and overshoot the cache once they land. `Status::Downloading` now carries the split size and `would_exceed_limits_with` counts it. - When the hottest fitting candidate cannot make room without evicting a fresher on-disk split, advance to the next-best fitting candidate instead of returning early, so smaller cacheable splits are not starved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Gate the per-split size guard behind a new `SplitCacheLimits::skip_oversized_splits` flag (default false), so the default searcher behavior is unchanged. When disabled (the default), the download loop and eviction use the original on-disk-only logic and the search-path size registration is a no-op. When enabled, the cache skips splits larger than `max_num_bytes`, reserves the bytes of in-flight downloads against the budget, and advances past candidates it cannot make room for. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
https://github.com/quickwit-oss/quickwit/blob/864c00fbc5465f32cb3c1a966228c547f171ab78/quickwit-storage/src/split_cache/split_table.rs#L430-L436
When a download fails, perform_eviction_and_download drops the candidate's last strong token without removing its Downloading entry, but this calculation still reserves that entry's full reported size. Because dead entries are garbage-collected only after the downloading set grows beyond num_concurrent_downloads + 10, a single failed 600 MB download in a 1 GB cache can indefinitely prevent an unrelated 500 MB candidate from starting. Ignore entries whose alive_token has no strong references, or eagerly remove them when the download task fails.
https://github.com/quickwit-oss/quickwit/blob/864c00fbc5465f32cb3c1a966228c547f171ab78/quickwit-storage/src/split_cache/split_table.rs#L468-L470
With skip_oversized_splits enabled, this condition always reserves one incoming split, but SearchSplitCache::open calls make_room_for_split_if_necessary(..., 0) solely to trim existing files and has no incoming split. Consequently, restarting with exactly max_num_splits valid cached files evicts the oldest one even though the cache is already within its configured limit. The startup trim needs a way to omit the + 1 reservation.
https://github.com/quickwit-oss/quickwit/blob/864c00fbc5465f32cb3c1a966228c547f171ab78/quickwit-storage/src/split_cache/split_table.rs#L358-L361
When the guard is enabled, every open_split_bundle calls this method even for a split that is already OnDisk. Delegating to report runs the entry through mutate_split, whose remove path increments evict_num_items and evict_num_bytes; reinsertion restores the current cache gauges but cannot undo those cumulative eviction counters. Normal searches of cached splits therefore appear as evictions, corrupting the cache telemetry. Return without mutating non-candidate entries, or update candidate sizes without removing on-disk entries.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Two follow-up fixes to the split-cache size guard (only active when `skip_oversized_splits` is enabled): - `downloading_bytes` now ignores dead `Downloading` entries (whose living token has no strong references). A failed download otherwise kept its reported size reserved until garbage collection, which could block an unrelated candidate from starting. - `report_split_size_from_search` only attaches a size to candidate or not-yet-known splits. Routing an already-on-disk split through `report` (remove + reinsert) spuriously incremented the cache eviction counters, making normal searches of cached splits look like evictions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the follow-up review — addressed the findings in 9cea95b: Exclude failed downloads from reserved bytes — Fixed. Avoid recording cache hits as evictions — Fixed. Avoid reserving a phantom split during startup — This one is not a regression. With the guard enabled the count check is All three paths are behind the opt-in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60158ff5f3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if self.on_disk_splits.len() + self.downloading_splits.len() + 1 | ||
| > self.limits.max_num_splits.get() as usize |
There was a problem hiding this comment.
When skip_oversized_splits is enabled, this split-count check includes every entry in downloading_splits, even entries whose alive_token is already dead after a failed download. For example, with max_num_splits = 1, a failed download leaves one dead Downloading entry; a different candidate then always sees 0 + 1 + 1 > 1, there is no on-disk split to evict, and find_download_opportunity returns None forever because GC only runs after many downloading entries accumulate. The byte accounting already ignores dead downloads, so the count path should either GC first or count only live downloads.
Useful? React with 👍 / 👎.
1 participant
What
The searcher on-disk split cache (
split_cache) has no per-split size guard: it will download a split even when that split is larger than the entire cache budget (split_cache.max_num_bytes). A single oversized split therefore transiently writes its full size to the searcher disk (riskingENOSPC) and evicts the entire small-split working set while (failing to) make room for it.This PR adds a size-based guard so the cache refuses to download a split larger than its budget; such queries fall back to the normal cold-storage range warmup.
How
max_num_bytesand advances to the next one, instead of stalling on a permanently-hot oversized split (which would otherwise starve the whole cache).on_disk_bytes + incoming <= max_num_bytes), so on-disk usage no longer overshoots the budget by a whole split.ReportSplit.num_bytesfield populated by the indexer at publish time (from the split footer offset), andopen_split_bundle), which registers the size from the split's footer offsets — so splits that were never reported with a size are still guarded the first time they are queried.0) is treated as "fits", so behavior is byte-for-byte unchanged when the size is unknown.Search results are unaffected — skipped splits are served by the existing cold-storage range warmup.
Tests
Unit tests in
split_table.rscover: an oversized candidate is skipped while a smaller one behind it is selected; oversized-only → nothing is downloaded; eviction accounts for the incoming split size; unknown size (0) behaves as before; and a size attached to a candidate discovered via the search path causes the guard to skip it.Before merge
cargo build -p quickwit-proto(requiresprotoc) sosearch_descriptor.binmatches the newReportSplitfield.🤖 Generated with Claude Code