Repository navigation
fix(rollout): bound periodic WAL cleanup pass with a timeout - #133
Merged
Merged
Conversation
The periodic per-shard WAL cleanup loop called `cleanup_own_shard()` under the store's write lock with no time bound. A hung object-store call could never return: `tokio::time::interval` fires serially (the next tick will not start until the current pass completes), and the pass holds the write lock the whole time — so one stuck I/O would permanently wedge both the cleanup timer and every read/write on that store, with no self-recovery. Wrap each pass in `tokio::time::timeout` (5x interval, floored at 30s). On timeout the pass is abandoned, the write lock is released, and the next tick retries — a merely-slow merge is not aborted, but a truly hung one no longer blocks the store forever. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
beinan
added a commit
that referenced
this pull request
Jul 14, 2026
#136) ## Why We want to partition rollouts by `experiment_name` — one physical Lance dataset per experiment. At the target scale (tens to hundreds of thousands of experiments) the current server runtime cannot cope: 1. `rollout_stores` is an **unbounded `HashMap`** of open handles → never evicted → OOM. 2. Each store spawns its **own tokio cleanup timer** (`spawn_periodic_cleanup`) → 100k+ background timers wedge the process. 3. `list_rollout_stores` only enumerates the **in-memory cache**, so with eviction it would be incomplete, and there is no durable directory to enumerate. This PR decouples resident memory / background tasks from the number of stores. ## What - **Durable registry** (`RolloutRegistry`, new `crates/lance-context-core/src/registry.rs`): a single small Lance table (`name`, `uri`, `created_at`) that is the source of truth for which stores exist. `create` / `list` / `delete` / existence checks now go through it. Existence is no longer inferred from cache membership, so eviction cannot cause a false 404, and `list` returns every store. - **Bounded LRU** replaces the unbounded `HashMap`; evicted stores are transparently reopened from storage on the next request (reusing `open_existing_with_options` from #135). - **Single global sweeper** (`AppState::spawn_global_sweeper`) replaces one-timer-per-store. It sweeps only the resident set each tick, with the same per-pass `tokio::time::timeout` hardening as #133. Cold (evicted) stores accrue no new generations and are swept again once re-opened. - `RolloutStoreInfo.version` → `Option<u64>` (omitted in `list` to avoid opening every dataset; populated for single get/create). Client updated (`unwrap_or(0)`). - New `--rollout-cache-capacity` / `ROLLOUT_CACHE_CAPACITY` (default 2000). ## Tests - `registry.rs`: upsert idempotency, contains/remove, list, survive-reopen. - Server: evicted-store-transparently-reopened (capacity-1 LRU), list-enumerates-all-registered, delete-unregisters, sweeper gating. Existing multi-replica regression tests (#135) still pass. - `cargo test --workspace`, `cargo clippy --workspace --all-targets`, `cargo fmt --check` all clean. ## Follow-ups (out of scope) - Startup reconciliation (scan `data_dir` to backfill registry rows if a crash left a dataset without its row). - Optional btree index on `registry.name` if the narrow scan becomes hot. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4 <noreply@anthropic.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
1 participant
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
spawn_periodic_cleanup) calledcleanup_own_shard()under the store write lock with no time bound.tokio::time::intervalfires serially and the pass holds the write lock the whole time — so one stuck I/O would permanently wedge both the cleanup timer and every read/write on that store, with no self-recovery.tokio::time::timeout(5× interval, floored at 30s). On timeout the pass is abandoned, the write lock is released, and the next tick retries. A merely-slow merge is not aborted; a truly hung one no longer blocks the store forever.Test plan
cargo test -p lance-context-core rollout(16 pass, 1 ignored bench)cargo fmt --all --check,cargo clippy -p lance-context-core --all-targetsspawn_periodic_cleanup_reclaims_on_a_timerstill covers the normal loop. A fault-injected hang test was skipped (needs a test-only seam into the merge path + 30s+ wall-clock).🤖 Generated with Claude Code