Skip to content

fix(rollout): stop WAL merge from blocking concurrent appends - #199

Merged
beinan merged 1 commit into
lance-format:mainfrom
beinan:worktree-fix-merge-write-lock-stall
Jul 26, 2026
Merged

fix(rollout): stop WAL merge from blocking concurrent appends#199
beinan merged 1 commit into
lance-format:mainfrom
beinan:worktree-fix-merge-write-lock-stall

Conversation

@beinan

@beinan beinan commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Fixes the ~17s stop-the-world stall on the rollout write path.

Root cause: claim_epoch, not the lock

A merge held the store's write lock for its whole duration, so every concurrent add blocked. But this is an LSM — a merge folds sealed generations into the base table while add writes the active memtable at the WAL tail. Disjoint data. They should never have serialized.

The lock was covering for something else. To drain merged generations from the shard manifest, the merge called claim_epoch, which bumps writer_epoch and fences every live writer of that shard — including our own. So the merge had to close() the resident writer first, and callers had to hold an exclusive lock to hide that window.

The claim is unnecessary:

  • commit_update(local_epoch, ..) only rejects a writer whose epoch is older than the stored one (stored > local ⇒ fenced) — it does not require that you just claimed.
  • Lance's own flush path reuses one epoch for every manifest commit a writer ever makes. The epoch is an ownership token, not a per-commit token.
  • Lance explicitly sanctions concurrent draining by an external compactor: recovery keys off replay_after_wal_entry_position and deliberately does not consult flushed_generations, "since an external compactor may legitimately drain that vector back to empty" — with a dedicated regression test upstream.

So the merge reuses the shard's current epoch and never closes the writer.

Plus a lock-scope split

phase receiver work
prepare &self seal memtable, read every flushed generation from object storage — the dominant cost (1.5–3.4s per generation on abfss)
commit &mut self append to base table, drain manifest, delete merged dirs

Callers run prepare under a read lock so appends keep flowing, and take the write lock only for the short commit. Both sweepers and the HTTP merge-wal handler updated.

The issue's suggested fix would not have worked

Option 1 in the report was "drop the count trigger from the flush sweeper and let the global sweeper drive merges." But the global sweeper takes the same write lock and calls cleanup_own_shard with threshold 1 — more aggressive, not less. The stall would simply have moved to a different interval.

A subtle ordering that had to be preserved

cleanup_own_shard seals before reading the manifest. That ordering is load-bearing: with ROLLOUT_FLUSH_INTERVAL_SECS=0 nothing else seals the memtable, so reading the manifest first leaves flushed_generations empty forever and rows durable but permanently invisible until a restart replays the WAL. Preserved as prepare_cleanup_merge. (My first attempt got this backwards.)

Tests

New suite, 5 tests. The existing one is explicitly "one store, one shard, fully serial — no concurrency, no fence", so none of this had coverage:

  • appends succeed and no row is lost while a merge runs
  • a generation sealed during a merge is not dropped by the drain
  • concurrent merges do not duplicate rows
  • an interrupted merge loses nothing; the next merge converges
  • an append is not blocked for the merge's duration — the regression itself

Each test drives merges through a helper reproducing the sweeper's exact lock discipline (read-lock prepare → write-lock commit). Merging under a single exclusive lock in the test would pass while the real stall persisted.

Verified non-vacuous: restoring claim_epoch makes 2 of the 5 fail. They also do not compile against the old &mut self signature — "merge concurrent with append" was inexpressible, which is the design flaw in one line.

Caught during development

The first attempt made merge fully &self (clone-based append). The tests immediately caught data loss: all 8 merged rows vanished, because a &self merge cannot advance its own handle past the append, so the rows were neither in its base snapshot nor in the manifest. That is why the commit phase stays &mut self.

Follow-ups (filed, not folded in)

Verification

166 core + 5 new concurrency + 51 server + 14 master tests pass; cargo fmt and clippy --workspace --all-targets -D warnings clean.

🤖 Generated with Claude Code

A count- or time-triggered WAL self-merge held the store's write lock for its
entire duration, so every concurrent `add` to that store blocked until it
finished — ~17s once per flush interval on a 20-writer abfss deployment, with
appends otherwise at p50 ~13ms. This is an LSM: a merge folds *sealed*
generations into the base table while `add` writes the active memtable at the
WAL tail. They touch disjoint data and should never have serialized.

Root cause was `claim_epoch`, not the lock. To drain merged generations from the
shard manifest the merge claimed the shard epoch, which bumps `writer_epoch` and
fences *every* live writer of that shard — including our own. So the merge had
to `close()` the resident writer first (rollout_store.rs), and callers had to
hold an exclusive lock across the whole merge to hide that window. The lock was
covering for the epoch claim.

The claim is unnecessary. `ShardManifestStore::commit_update(local_epoch, ..)`
only rejects a writer whose epoch is *older* than the stored one
(`stored > local` ⇒ fenced, lance manifest.rs), and Lance's own flush path
reuses a single epoch for every manifest commit a writer ever makes
(lance flush.rs) — the epoch is an ownership token, not a per-commit token.
Lance also explicitly sanctions concurrent draining by an external compactor:
recovery keys off `replay_after_wal_entry_position` and deliberately does not
consult `flushed_generations`, "since an external compactor may legitimately
drain that vector back to empty" (lance write.rs, wal.rs).

So the merge now reuses the shard's current epoch and never closes the writer.
On top of that it is split by lock scope:

  prepare (&self)  — seal the memtable, read every flushed generation from
                     object storage (the dominant cost: 1.5–3.4s per generation
                     on abfss)
  commit (&mut self) — append to the base table, drain the manifest, delete the
                     merged generation dirs

Callers run `prepare` under a read lock so appends keep flowing, and take the
write lock only for the short commit. Both sweepers and the HTTP merge-wal
handler were updated.

Note the issue's suggested fix (drop the count trigger from the flush sweeper)
would not have worked: the *global* sweeper takes the same write lock and calls
`cleanup_own_shard` with threshold 1, so the stall would simply have moved to a
different interval.

`prepare_cleanup_merge` keeps the seal-before-manifest-read ordering that
`cleanup_own_shard` relied on. That ordering is load-bearing: with
ROLLOUT_FLUSH_INTERVAL_SECS=0 nothing else seals the memtable, so reading the
manifest first would leave `flushed_generations` empty forever and rows durable
but permanently invisible until a restart replayed the WAL.

New integration suite (tests/wal_merge_concurrency.rs, 5 tests). The existing
suite is explicitly "one store, one shard, fully serial — no concurrency, no
fence", so none of this had coverage:

  - appends succeed and no row is lost while a merge runs
  - a generation sealed *during* a merge is not dropped by the drain
  - concurrent merges do not duplicate rows
  - an interrupted merge loses nothing and the next merge converges
  - an append is not blocked for the merge's duration (the regression itself)

Each test drives merges through a helper that reproduces the sweeper's exact
lock discipline (read-lock prepare, write-lock commit); merging under a single
exclusive lock in the test would pass while the real stall persisted.

Verified the tests are not vacuous: restoring `claim_epoch` makes
`appends_succeed_and_are_not_lost_while_merge_runs` and
`append_is_not_blocked_for_the_duration_of_a_merge` fail. They also do not
compile against the old `&mut self` signature at all — "merge concurrent with
append" was inexpressible, which is the design flaw in one line.

Follow-ups filed rather than folded in:
- lance-format#198: make `dataset` interior-mutable so merge/compact need no exclusive lock
  at all (~52 call sites; deserves its own review).
- `datagen_store.rs` has the identical `claim_epoch`-then-close pattern.

Co-Authored-By: Claude <noreply@anthropic.com>
@beinan
beinan merged commit 9e99ab1 into lance-format:main Jul 26, 2026
9 checks passed
beinan added a commit that referenced this pull request Jul 26, 2026
…erate drained generations (#200)

Three fixes the rollout store received but `DatagenStore` never did.
Each is latent today and becomes reachable as soon as datagen sees
concurrent or long-lived use.

## 1. Merge used the compile-time schema, not the live one

`merge_own_shard` built its append batch from `datagen_log_schema()`. A
base table written by an older binary can lack columns the current
schema has, and merging a batch built from the compile-time schema into
it **fails outright**. Now aligned via `align_batch_to_schema` — the
rollout store hit exactly this and was fixed in #175.

## 2. `Drop` swallowed the writer's close error

```rust
let _ = writer.close().await;   // before
```

A failed close leaks the writer's background tasks and, if the memtable
was still buffered, **strands rows that are durable in the WAL but never
sealed** — with no signal anywhere. Now logged, and the no-runtime path
says so too. (Rollout equivalent: #190.)

## 3. A concurrently-drained generation failed the whole lookup

`get_blob` propagated not-found when a merge drained and deleted a
generation between snapshot and open. Those rows are already in the base
table, so it now skips and falls through — as the rollout store does.

## Sharing

`align_batch_to_schema` and `is_not_found_error` become `pub(crate)` so
both stores share one implementation instead of drifting again. This
drift is the actual pattern here: all three bugs are cases where rollout
was fixed and datagen was not.

## Scope note — worth reading

I expected datagen to have the write-lock stall fixed in #199. **It does
not**, and I verified rather than assumed:

- `DatagenStore::append` is already `&mut self` (its writer is a bare
`Option<ShardWriter>`, not behind a mutex), so appends need the
exclusive lock **regardless of merge** — the read/write lock split from
#199 would buy nothing.
- `write_with_resident_writer` already retries on fence, so
`claim_epoch` fencing its own writer is handled. Restoring `claim_epoch`
leaves the new test **green**, confirming this empirically.
- Datagen is not yet wired into any server route or sweeper.

So the epoch change from #199 is deliberately **not** ported. These
three divergences are the real defects.

## Testing

New test asserts a merge does not fence the store's own resident writer,
so an append immediately after a merge still succeeds and both rows stay
readable, and a second merge still converges.

9 datagen tests pass; `clippy --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 26, 2026
```diff
-  cargo test -p lance-context-core -p lance-context-master --lib
+  cargo test --workspace --all-targets
```

`--lib` runs **unit tests only**. The preceding `--no-run` step compiled
the `crates/lance-context-core/tests/*.rs` integration tests and then
**discarded them** — they never executed.

## Evidence

The CI log from **#199**:

```
test result: ok. 167 passed   ← core unit tests
test result: ok. 14 passed    ← master unit tests
test result: ok. 1 passed     ← the one etcd test named explicitly
```

No line for the **5 WAL-merge concurrency tests that PR added**. They
merged into `main` having never run in CI — as had
`wal_merge_generation_cleanup.rs` before them.

`--workspace` additionally covers `lance-context-api`, `-server`,
`-client`, `-metrics` and the facade crate, **none of which were tested
at all**.

## Impact

**250 tests pass** under the new command, against **182** under `--lib`.
Runtime ~7 min against the existing 30 min timeout.

Same class of failure as #195 (Python CI collecting the wrong
directory), in a different mechanism: tests that exist, compile, and are
silently not run. I flagged `--lib` in an earlier review and then failed
to re-check it when adding integration tests in #199 — a test that never
executes is the same as no test.

## Note

This PR is **only** the CI change, deliberately. It was originally
bundled with a `ContextStore` concurrency fix in #201; on review that
fix addresses a scenario that is not currently reachable in
single-process deployments, so it should be argued on its own merits
rather than riding along with an unambiguous CI repair. #201 will be
rescoped.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant