fix(core): bound flushed generations folded per merge pass - #238
Merged
Conversation
Worker OOM: prepare_merge materialized an entire shard's flushed generations into memory at once. Dataset::append takes a *synchronous* RecordBatchReader, so the batches cannot be streamed lazily off object storage without pushing that IO into the writer's commit window (the stop-the-world append stall that wal_merge_concurrency.rs pins). Peak merge memory was therefore proportional to the whole shard -- and rollout rows carry inline binary_payload blobs, because blob-v2 offload reads back as None through the MemWAL LSM scanner. In production this OOMKilled 8 of 20 workers, with worker-2 at 23.3 GiB RSS. Cap one merge pass at merge_max_generations (default 8, env ROLLOUT_MERGE_MAX_GENERATIONS); leftovers stay pending for the next pass. A subset merge is safe because commit_merge's drain is already surgical -- it filters out exactly the generations that were merged rather than clearing the list -- so a partial merge is a smaller version of a full one with the same crash-safety argument. Generations are the granularity because each is a self-contained Lance dataset and the manifest tracks them individually. The default binds even where the count trigger does not: the time-triggered cleanup path merges at a hardcoded threshold of 1, so it never consults ROLLOUT_MERGE_AFTER_GENERATIONS (50 in the deployment that OOMed). 0 opts out, restoring the unbounded behavior. Tests: merge_pass_is_bounded_and_leftovers_survive (10 generations, cap 3 -- asserts exactly 3 reclaimed per pass, leftovers stay pending, >=4 passes to drain, all 10 rows survive, 0 leaked generation dirs) and zero_max_generations_merges_everything_in_one_pass. Verified negatively: removing the bound fails the first with left: 10, right: 3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
beinan
added a commit
that referenced
this pull request
Sep 8, 2026
## Motivation Inline `binary_payload` size drives memory pressure on both the worker (WAL merge buffers, resident manifests) and the master (compaction rewrite buffer) — but there was no way to see it. The only blob metric was `rollout_blob_budget_rejections_total`, which fires solely when `ROLLOUT_MAX_INFLIGHT_BLOB_BYTES` is set (default `0` = disabled). Capacity problems stayed invisible until a pod died. This is the missing observability behind #229 (compaction memory) and #238 (worker WAL-merge memory): both bounded memory as a function of blob-carrying rows, but neither surfaced how big those blobs actually are. ## Changes - **`rollout_blob_bytes` histogram** — recorded per inline blob on the add path. - **Oversized-blob warn log** — when a single blob crosses `ROLLOUT_LARGE_BLOB_LOG_BYTES` (new, default 16 MiB, `0` disables), logged with `experiment` and `record_id`. ## One deviation from the issue The issue suggests instrumenting inside `parse_multipart_rollouts`' blob loop, with a parenthetical to "also cover the inline-JSON path." I placed the instrumentation **where the multipart and JSON parse paths converge** instead, because a JSON body populates `binary_payload` directly and **never enters that loop** — instrumenting there would silently miss inline-JSON uploads entirely. The converged site also already has the experiment name in scope, so nothing needs threading through the parser. ## Cardinality The histogram is deliberately **unlabelled**, following the issue's own nice-to-have reasoning: a deployment can carry thousands of experiments and each label value is its own time series. The warn log supplies the identifying detail for the outliers that actually matter. ## Tests Both **verified to fail when the behavior is reverted**: - `blob_sizes_are_recorded_for_records_with_payloads` — uses a `DebuggingRecorder` to assert the emitted samples are exactly `[512, 2048]`, proving a payload-less record contributes nothing. It exercises the inline-JSON record shape, i.e. precisely what the issue's suggested location would have missed. *(Deleting the `histogram!` call → fails.)* - `large_blob_log_threshold_is_inclusive_and_zero_disables` — pins the inclusive boundary and the `0` disable switch. *(Changing `>=` to `>` → fails.)* Full workspace suite green: server 68 → 70, core 222, `fmt` + `clippy -D warnings` clean. `metrics-util` is added as a dev-dependency, matching the existing usage in `lance-context-core`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 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.
Problem
prepare_mergematerialized an entire shard's flushed generations into memory at once. In production this OOMKilled 8 of 20 workers; worker-2 was at 23.3 GiB RSS / 23.2 GiB anonymous with only 39 resident stores.Two things make the buffer unavoidable today, and both are why the streaming fix (option 1 in the issue) is off the table:
Dataset::appendtakes a synchronousRecordBatchReader(lance-7.0.0/src/dataset.rs:919), and there is no async→sync bridge in this repo.wal_merge_concurrency.rspins (~17s stop-the-world appends observed on abfss).Option 3 (projection) is also infeasible — merge must write back all columns.
The payload is large because blob-v2 offload reads back as
Nonethrough the MemWAL LSM scanner, sobinary_payloadis stored inline asLargeBinary.Fix
Cap how many flushed generations one merge pass folds in:
merge_max_generations, default 8, envROLLOUT_MERGE_MAX_GENERATIONS. Leftovers stay pending for the next pass, so a backlog drains incrementally at bounded peak memory.0opts out.Why a subset merge is safe:
commit_merge's drain is already surgical — it filters out exactly the generations that were merged rather than clearing the list:So a partial merge is just a smaller version of a full one, with the same crash-safety argument (immutable rows, read-time dedup by key). Generations are the right granularity because each is a self-contained Lance dataset that the manifest tracks individually.
Why the default actually binds: the time-triggered path (
cleanup_own_shard) callsmerge_own_shard_if_ready(1)— a hardcoded threshold — so it never consultsROLLOUT_MERGE_AFTER_GENERATIONS(50 in the deployment that OOMed). Lowering that env var alone would not have helped; the cap here applies to both trigger paths.Tests
merge_pass_is_bounded_and_leftovers_survive— 10 generations, cap 3: asserts exactly 3 reclaimed per pass, leftovers stay pending, ≥4 passes to fully drain, all 10 rows survive, and 0 leaked generation directories.zero_max_generations_merges_everything_in_one_pass— the opt-out escape hatch.Negative verification: removing
.take(budget)fails the first test withleft: 10, right: 3; restoring it passes.Full workspace suite green (222 core, up from 217).
wal_merge_concurrency5/5 — the prepare/commit invariants are intact.🤖 Generated with Claude Code