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>
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
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