fix(master): run stats cleanup off the store lock; never spin on the stats-writer lock - #272
Merged
Conversation
…he stats-writer lock The first master to take the stats-writer lock after a restart ran the untimed first maintenance pass and held state.stats for its whole duration. With ~68k accumulated _stats versions that was 36 minutes of object-store deletes, during which every sweep and every post-compaction stats refresh on every master waited behind the lock. The control plane went silent until ADLS happened to return a 500 on a bulk delete. Split StatsStore::maintain into compact() (under the lock, bounded) and a detached StatsCleaner::cleanup() (lock released, unbounded: it deletes as it goes, so a timeout would only abandon the pass, and nothing waits on it any more). Reload the store afterwards so its handle never points at a pruned manifest. The first-pass-without-timeout special case is gone with it. update_stats_after_compaction now waits at most 10s for stats-writer and otherwise leaves the row to the next scan; the compaction it follows is already committed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
beinan
added a commit
that referenced
this pull request
Sep 29, 2026
… _stats (#274) ## Problem `_registry.rollout.lance` (and `_registry.generic.lance`) is the shared directory of stores every worker consults. Workers write it on every store create/delete as a delete plus an append (`upsert` is delete-then-append so a retried create is idempotent). Nothing ever compacts or version-prunes it. Lance keeps every manifest until cleaned, and each manifest lists every fragment, so the table grows quadratically. Production today: **version 34,504, ~840 KB per manifest, ~29 GB of manifests** for a 10k-row three-column table, with one fragment per row. Every `POST /rollouts` does `contains()` (a `checkout_latest` that reads that head manifest) then `upsert()` (two commits, each writing a new 840 KB manifest); every `GET /rollouts/{name}` does another `checkout_latest`. Measured on prod: creates average **47 s** (201), 83 s when the client has already timed out and retried (409), and fail with 500 when ADLS throttles mid-way. `GET /rollouts/{name}` averages 0.7 s for a row lookup. ## Fix `RolloutRegistry` gets the same `compact()` / detached `RegistryCleaner::cleanup()` / `reload()` split as `StatsStore` (#272), plus `maintain()` for callers without a shared lock. The master runs it for both registries in the scanner's existing maintenance round (every `STATS_MAINTENANCE_EVERY_N_SCANS` scans, under the `stats-writer` lock so only one master rewrites). Compaction is bounded by `MAINTENANCE_TIMEOUT`; the cleanup runs with the lock released. Workers keep appending throughout: Lance's commit retry carries their appends past the rewrite, and the grace window (`STATS_HISTORY_TTL_SECS`) keeps any version a worker may still hold open. The first pass on prod will delete ~34k manifests; with #272 that no longer blocks anything. ## Verification - `maintain_bounds_versions_and_preserves_rows`: 20 upserts (≥40 versions) → compaction folds fragments, cleanup removes versions, all 20 rows and further writes work. - `cleanup_runs_detached_from_the_registry`: writes land while the cleaner is outstanding and survive. - `maintenance_does_not_lose_concurrent_worker_upserts`: a second handle on the same URI (the worker) creates stores before, between and after the master's compact/cleanup; both handles see all 26 rows. - etcd-backed master suite `--include-ignored`: 72 passed. Clippy `-D warnings`, fmt clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <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.
Fixes #271.
Problem
After a routine master restart, the first master to take the
stats-writerlock ran the untimed firstmaintain_statspass and heldstate.statsfor its whole duration. The_statstable had ~68k accumulated versions, so that was 36 minutes of object-store deletes (~31k data files, ~8k deletion files, ~32k manifests at ~30/s). During that time, on every master:state.stats.lock()to read candidatesupdate_stats_after_compactiononcoordination_lock("stats-writer"), which spins forever, pinning aTASK_CONCURRENCYslot eachIt ended only because ADLS returned a 500 on a bulk delete. Since the pass did not complete, the next restart would have replayed it.
Fix
StatsStore::maintainis split intocompact()(needs exclusive access; runs under the lock, bounded byMAINTENANCE_TIMEOUT) and a detachedStatsCleaner::cleanup()that owns aDatasetclone and runs with the lock released. Cleanup only deletes objects no live version references, so concurrent readers and writers are unaffected. It is deliberately unbounded: Lance deletes as it goes, so a timeout would only abandon a pass mid-way, and with the lock released nothing waits on it.stats_maintenance_doneare removed: the untimed first pass existed because a bounded pass could never drain a big backlog, but that was only a problem while the lock was held.update_stats_after_compactionwaits at most 10 s forstats-writer, then leaves the row to the next scan round. The compaction it follows is already committed.Verification
cleanup_runs_detached_from_the_store: compacts, writes through the store while the cleaner is outstanding, runs cleanup, reloads, and checks every row (including the one written mid-cleanup) survives.maintain_*tests pass unchanged through the new split.cargo test -p lance-context-master, clippy-D warnings, fmt clean.🤖 Generated with Claude Code