Skip to content

fix(mem_wal): keep sealed memtables queryable until flush commits#6814

Merged
jackye1995 merged 2 commits into
lance-format:mainfrom
hamersaw:bug/wal-read-sealed-memtable
May 17, 2026
Merged

fix(mem_wal): keep sealed memtables queryable until flush commits#6814
jackye1995 merged 2 commits into
lance-format:mainfrom
hamersaw:bug/wal-read-sealed-memtable

Conversation

@hamersaw
Copy link
Copy Markdown
Contributor

@hamersaw hamersaw commented May 17, 2026

Problem

A concurrent reader could miss rows in the window between freeze_memtable and the flush task's manifest commit. During that window the frozen generation is no longer the active memtable, but is not yet recorded as a flushed generation in the manifest — so it falls out of the read union entirely.

Fix

  • Keep frozen-awaiting-flush memtables in WriterState.frozen_memtables so they stay in the read union (Arc refcount, not a copy — the flush task holds them alive for the drain anyway).
  • Drop them by generation only on flush-commit success. On failure they are retained until a later flush or WAL replay on reopen, so a transient flush error doesn't reopen the hole. Keyed by generation, so non-FIFO completion is fine.
  • Add ShardWriter::in_memory_memtable_refs and LsmScanner::with_in_memory_memtables as the read-path entry point — captures active + frozen atomically under one state.read() (no torn freeze).
  • Rename ActiveMemTableRefInMemoryMemTableRef (frozen is just the immutable case; same shape). Back-compat alias and with_active_memtable test/back-compat entry point retained.
  • LsmDataSourceCollector emits in-memory sources in ascending generation order (see below).

Source-ordering correctness fix

Adding the scan-level test surfaced a latent bug in the first cut: collect() pushed active (newest generation) before frozen (older). The planner relies on sources being in ascending-generation order — it reverses them to generation-DESC so the newest row gets the lowest stream index and wins the dedup tiebreaker. With active emitted first, a pk that was written, sealed, then re-written into the new active memtable before the sealed one flushed would resolve to the stale frozen value. The collector now sorts in-memory sources by generation, restoring active-wins across the active/frozen seam.

Tests

  • collector: collect()/collect_for_shards() emit active + every frozen memtable, in ascending generation order (frozen deliberately registered out of order).
  • planner: end-to-end scan over base + flushed + frozen + active asserting frozen rows are in the read union, frozen beats older flushed generations, and active beats frozen for the same pk.
  • write: frozen handle dropped from the queryable set on flush-commit success; retained with rows intact on a fenced/failed flush (hole cannot reopen).
  • Migrated the planner/vector_search test harnesses and the WAL-only accessor test onto the new frozen-aware entry point.

All 265 mem_wal tests pass; cargo clippy -p lance --tests -D warnings clean.

🤖 Generated with Claude Code

A concurrent reader could miss rows in the window between
`freeze_memtable` and the flush task's manifest commit: the frozen
generation was no longer the active memtable but not yet recorded as
flushed.

Keep frozen-awaiting-flush memtables in `WriterState.frozen_memtables`
so they stay in the read union, and drop them by generation only on
flush-commit success (retained on failure until a later flush or WAL
replay). Add `ShardWriter::in_memory_memtable_refs` and
`LsmScanner::with_in_memory_memtables` as the read-path entry point,
capturing active + frozen atomically under one state lock. Rename
`ActiveMemTableRef` -> `InMemoryMemTableRef` (back-compat alias kept).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the bug Something isn't working label May 17, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 17, 2026

Codecov Report

❌ Patch coverage is 90.23747% with 37 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance/src/dataset/mem_wal/scanner/builder.rs 40.00% 18 Missing ⚠️
...ust/lance/src/dataset/mem_wal/scanner/collector.rs 83.13% 14 Missing ⚠️
rust/lance/src/dataset/mem_wal/write.rs 95.53% 3 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

Add tests for the sealed-memtable read fix and migrate the in-repo
scanner readers onto the frozen-aware entry point:

- collector: assert collect()/collect_for_shards() emit active + every
  frozen memtable, in ascending generation order.
- planner: end-to-end scan spanning base/flushed/frozen/active asserting
  frozen rows are in the read union and dedup by generation across the
  active/frozen seam.
- write: frozen handle dropped on flush-commit success; retained (rows
  intact) on a fenced/failed flush so the hole cannot reopen.
- Migrate planner/vector_search test harnesses and the write.rs
  WAL-only accessor test from with_active_memtable/active_memtable_ref
  to with_in_memory_memtables/in_memory_memtable_refs.

The planner scan test surfaced a correctness bug in the original fix:
collect() pushed `active` (newest generation) before `frozen` (older),
but the planner relies on sources being in ascending-generation order
(it reverses to gen-DESC for the dedup tiebreaker). A pk written, sealed,
then re-written into the new active memtable before the sealed one
flushed would resolve to the stale frozen value. LsmDataSourceCollector
now sorts in-memory sources by generation, restoring active-wins.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@hamersaw hamersaw marked this pull request as ready for review May 17, 2026 15:53
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

Copy link
Copy Markdown
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@jackye1995 jackye1995 merged commit 7dd5c1a into lance-format:main May 17, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants