fix(master): write _stats as one snapshot per scan, push sweep filters down - #207
Merged
Merged
Conversation
…s down
`_stats` is a ~50-row control-plane cache, but `StatsStore::upsert` wrote each
row delete-then-append: two Lance commits per experiment per round. At the
default 300s interval that is ~100 new versions per round and ~28,800 per day,
unbounded. Production reached version 246,000+ with the table fragmented into
90+ fragments, and `GET /experiments` degraded from milliseconds to 30-43s
because every open traverses the manifest chain.
Three changes.
1. One commit per round instead of two per experiment.
A scan already computes the complete set of live experiments, so it writes that
set as a single `Overwrite` commit. Measured over 5 rounds x 50 experiments:
before: version=501 fragments=50
after: version=6 fragments=1 (84x fewer versions, 50x fewer fragments)
This also subsumes the reconcile-remove pass: an experiment absent from the
registry is simply absent from the snapshot.
Two hazards handled explicitly. An empty snapshot is a no-op rather than a wipe
-- a round where every `observe` failed would otherwise blank the table, which
does not just empty the UI, it silently stops both auto-sweeps, since they read
this table to decide what to enqueue. And an experiment that failed to observe
this round keeps its previous row instead of vanishing.
Note `Dataset::append` hardcodes `WriteMode::Append` and silently ignores
`params.mode`; `Dataset::write` honours it. Using `append` here would duplicate
every row each round rather than replace the table. Called out in a comment
because it fails silently.
2. Sweep predicates pushed into the scan, and capped.
Both auto-sweeps called `list(None, usize::MAX, 0)` and filtered in a Rust loop,
materialising the whole table twice per interval. They now use
`list_above_fragment_count` / `list_above_pending_wal`, which push the predicate
into the scanner. Each sweep is also capped at 256 enqueues per tick; it was
previously unbounded, so at tens of thousands of experiments one tick could
flood the queue. Anything still over threshold is picked up next tick.
3. The first maintenance pass runs without a timeout.
`MAINTENANCE_TIMEOUT` (300s) is right for steady state but guaranteed failure
against an already-bloated table: a 246k-version chain cannot be compacted and
pruned inside the window, so every pass timed out, rolled back, and left the
table exactly as bloated -- the bound ensured it could never recover. The first
pass per process is now unbounded; later passes take the bound, by which point
the backlog is gone and exceeding it is a genuine fault.
On pruning: `cleanup_old_versions` already physically deletes old manifests and
their data files (verified: 12 manifests on disk -> 2 after one pass, 49,887
bytes reclaimed, rows intact). It was not broken, it was outrun -- 100 versions
per round against a pass every twelfth round. At 1 version per round it keeps
up easily: over 120 rounds the version *number* reaches 121 while the table
holds 2 manifests and 5.4 KB. The monotonic version number is just a u64
counter and costs nothing; only the manifest and data files do.
Co-Authored-By: Claude <noreply@anthropic.com>
Kazimierzsier
approved these changes
Jul 27, 2026
This was referenced Jul 27, 2026
beinan
added a commit
that referenced
this pull request
Jul 27, 2026
Follow-up to #207. A failing maintenance pass left **no signal at all**. `master_stats_versions_removed_total` only moves on success, so a pass failing for days — object-store outage, permissions, a genuinely stuck compaction — looked **identical to a pass with nothing to reclaim**, while old manifests piled back up and the table walked back toward exactly the state that path exists to prevent. ## New metrics ``` master_stats_maintenance_failures_total counter master_stats_maintenance_consecutive_failures gauge, 0 after any success master_stats_unreclaimed_versions gauge ``` **`unreclaimed_versions` is the signal to alert on** — the version gap since the last successful pass. `master_stats_version` is deliberately *not* the signal: it climbs by design and says nothing about disk. This distinction caused real confusion on the original report ("version 246,000+ and climbing") — the number itself is a harmless `u64` counter. Only manifests still sitting on storage cost anything, and the gap is what measures those. The failure path also logs at `warn` with both numbers, so the reason is visible without a metrics backend. ## Descriptions Also describes every `_stats` metric, **including the three from #194 that had no HELP or TYPE**. Datadog's OpenMetrics check infers type from these, and the description text states explicitly which metric is the alerting signal so it isn't rediscovered the hard way. ## Testing Two tests on the bookkeeping: - consecutive failures accumulate and reset on success - **a failure before any successful pass reports the full backlog** rather than a zero gap — the case a freshly-upgraded, already-bloated deployment hits first, and the one most likely to be got wrong `MasterState` needs etcd to construct, so the counters are tested directly rather than through an `#[ignore]`d integration test that CI would skip. 21 master + 1 metrics tests pass; `clippy --workspace --all-targets -D warnings` and `cargo fmt` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
beinan
added a commit
that referenced
this pull request
Jul 27, 2026
…t moved (#209) First of three steps toward surviving tens of thousands of experiments. #207 fixed how `_stats` is *written*; this fixes how much work a scan *does*. ## The problem A scan round fully observes **every** experiment, every 300s. `observe` is not cheap: | step | cost | |---|---| | `wal_shard_snapshots()` | list every MemWAL shard + read each manifest | | `pending_wal_rows()` | open every flushed generation | | `count_rows()` | scan the base table | | `manifest.version` | **free — already in memory** | At 30k experiments with concurrency 8: **~19 min** at an optimistic 0.3s each, **~90 min** at object-storage latencies — against a **5 minute** interval. The next round starts before the previous ends, forever. No choice of stats storage fixes this; the scan itself is the wall. ## The fix `RolloutObservation` **already returned** the base dataset version — `StatRow` just never persisted it. The cheapest possible change signal was being thrown away. Opening the dataset reads its manifest, which is where the version comes from, so the comparison is **free**. Unchanged version ⇒ no write landed ⇒ every derived metric is necessarily unchanged ⇒ reuse the previous row and skip everything above. Cold experiments now cost **one open** per round instead of a full observation. Also removes a per-experiment `stats.get()` that took the stats lock **once per experiment per round**; the previous snapshot is now read once up front and passed down. `observe_one` no longer needs `MasterState` at all. ## Two edges, both tested **1. Existing tables have no `version` column.** Reads tolerate the absence and record `UNKNOWN_VERSION`, forcing one full observation which then persists the column. Erroring instead would break **every read on upgrade** — and reads are what the UI *and both auto-sweeps* depend on, so the master would come up blind until the table happened to be rewritten. There's a test that builds a legacy-schema table and asserts it reads, keeps its data, and migrates on first snapshot. **2. `refresh_one` must not skip.** Its callers use it precisely when they believe something just changed (post-compaction, explicit refresh) — the version check would short-circuit exactly the observation they asked for. It passes `None`. ## Observability `master_scan_experiments_skipped` / `_observed`. A ratio near 1 is the healthy state at scale; trending to 0 means the incremental path stopped engaging (usually a stats table losing its rows). ## Testing 4 new scanner tests + 1 migration test. **Verified non-vacuous:** disabling the skip makes 2 of the 4 fail. Full workspace: 178 core + 24 master + 51 server + 5 concurrency + 1 merge-cleanup, all pass. clippy `-D warnings` and fmt clean. ## Next - **C2** — retire experiments cold for 7 days: merge WAL → compact → verify `pending_wal_generations == 0` → drop from the hot table. The ordering matters: `compact_files` does not touch WAL generations, so retiring before draining them would strand those generations forever (the WAL sweep only reads this table) and leak `_mem_wal/` directories. - **C3** — UI becomes "recently active N + search", with cold experiments observed on demand via the registry. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
beinan
added a commit
that referenced
this pull request
Jul 27, 2026
#211)⚠️ **Stacked on #209 → #210.** Both of those commits are included here. Merge order: **#209 → #210 → this**. Once they land, this diff reduces to the UI commit alone. ## Why this exists #210 removes cold experiments from the stats table — that's what keeps a scan round proportional to *active* work. But on its own it also removes them from `GET /experiments`, which is only acceptable once they stay findable another way. **This PR is what makes retirement safe to turn on.** | | behaviour | |---|---| | **default list** | the stats table — experiments written recently enough not to have been retired | | **search** | stats table **plus the registry** for names it no longer holds; cold matches observed on demand | | **detail** | on a stats miss, resolve via registry and observe on demand rather than returning 404 | Retirement therefore removes an experiment from the *default view* and **never makes it unfindable**. The default is also the right one at scale on its own merits: a flat list of every experiment ever created is neither renderable nor interesting once there are tens of thousands. ## Two deliberate decisions **1. `observe_cold` does not write a stats row.** Reading about a retired experiment must not make it hot again — otherwise **browsing the UI would silently undo retirement**, and the table would creep back toward holding everything, which is exactly the state retirement exists to prevent. It also takes no `MasterState`, so it *structurally* cannot write one. A refactor that hands it one has to justify itself. There's a test asserting the no-rehydration property. **2. Search bounds its on-demand observations by page size.** A query matching thousands of cold experiments must not open thousands of datasets to render one page. ## Known gap — stating it rather than hiding it The two handlers changed here live in `routes.rs`, whose tests **all require etcd and are `#[ignore]`d**, so CI does not cover them. `observe_cold` is tested in `scanner.rs`, which CI does run. So the registry-fallback logic in the handlers is verified locally but not by CI. That's a pre-existing gap in the test structure, worth fixing as its own change rather than as a fourth concern here — and it's the kind of blind spot that only became visible after #202 made CI actually run integration tests. ## Testing Full workspace: 178 core + 30 master + 51 server + 5 concurrency + 1 merge-cleanup, all pass. clippy `-D warnings` and fmt clean. ## The complete picture This finishes the line of work from the original report (`/experiments` at 30–43s): | | problem | fix | |---|---|---| | **#207** | write amplification — 100 versions/round | one snapshot commit; **84× fewer versions** | | **#209** | scan cost — full observe of everything, every round | skip unchanged; cold = one open | | **#210** | table size — held every experiment ever created | retire after 7 days idle, drained first | | **this** | retirement made cold experiments invisible | active-N default + registry-backed search | | **#208** | all of the above failing silently | maintenance failure metrics | 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <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
2 participants
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.
_statsis a ~50-row control-plane cache, butStatsStore::upsertwrote each row delete-then-append: two Lance commits per experiment per round. At the default 300s interval that is ~100 versions/round, ~28,800/day, unbounded. Production reached version 246,000+ with 90+ fragments, andGET /experimentsdegraded from milliseconds to 30–43s.1. One commit per round instead of two per experiment
A scan already computes the complete set of live experiments, so it writes that set as a single
Overwritecommit. Measured, 5 rounds × 50 experiments:This also subsumes the reconcile-remove pass — an experiment absent from the registry is simply absent from the snapshot.
Two hazards handled explicitly:
observefailed would otherwise blank the table — which doesn't just empty the UI, it silently stops both auto-sweeps, since they read this table to decide what to enqueue.Dataset::appendhardcodesWriteMode::Appendand silently ignoresparams.mode;Dataset::writehonours it. Usingappendhere duplicates every row each round instead of replacing the table. My first attempt hit exactly this — row count went to 100 instead of 50. Called out in a comment since it fails silently.2. Sweep predicates pushed into the scan, and capped
Both auto-sweeps called
list(None, usize::MAX, 0)and filtered in a Rust loop — materialising the whole table twice per interval. Nowlist_above_fragment_count/list_above_pending_walpush the predicate into the scanner.Each sweep is also capped at 256 enqueues/tick; previously unbounded, so at tens of thousands of experiments one tick could flood the queue. Anything still over threshold is picked up next tick.
3. The first maintenance pass runs without a timeout
MAINTENANCE_TIMEOUT(300s) is right for steady state but guaranteed failure against an already-bloated table: a 246k-version chain cannot be compacted and pruned inside the window, so every pass timed out, rolled back, and left the table exactly as bloated. The bound ensured it could never recover. The first pass per process is now unbounded; later passes take the bound.On pruning — it wasn't broken, it was outrun
Worth stating since "246k versions" sounds like pruning was absent.
cleanup_old_versionsalready physically deletes old manifests and their data files. Verified:It was simply outrun — 100 versions/round against a pass every twelfth round. At 1 version/round it keeps up easily. Over 120 rounds: version number reaches 121 while the table holds 2 manifests and 5.4 KB.
Three things get conflated here and shouldn't be:
The production "246,000+" is the number, which is harmless on its own. What degraded
/experimentswas the un-reclaimed manifest chain.Testing
5 new tests asserting cost, not just correctness: version growth per round, snapshot-replaces-not-appends, empty-snapshot-is-no-op, threshold queries return only matching rows and respect the cap, deterministic pagination.
Full workspace: 167 core + 20 master + 51 server + 5 concurrency + 1 merge-cleanup, all pass.
clippy --workspace --all-targets -D warningsandcargo fmtclean.Not in this PR
This is the stopgap — it fixes write amplification but keeps the full-scan-every-round model. At the tens-of-thousands scale discussed, the scan itself is the next bottleneck (
observe()per experiment per round, ~19–94 min for 30k at concurrency 8 against a 5-minute interval). Follow-ups: retire cold experiments from the hot table after merge+compact, serve the UI as "recently active N + search", and have workers recordlast_write_atso scans touch only what changed.🤖 Generated with Claude Code