Skip to content

feat(mem_wal): return the sealed generation from force_seal_active - #8051

Merged
jackye1995 merged 3 commits into
mainfrom
feature/wal-checkpoint
Aug 6, 2026
Merged

feat(mem_wal): return the sealed generation from force_seal_active#8051
jackye1995 merged 3 commits into
mainfrom
feature/wal-checkpoint

Conversation

@hamersaw

@hamersaw hamersaw commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Why

wait_for_flush_drain loops until the frozen-watcher set is empty, re-collecting the set on each round — so it also waits on every memtable frozen while it waits, including ones the size/interval trigger and backpressure freeze concurrently. On a table under sustained write load that set may never empty, which makes any post-seal ack unbounded on exactly the workload that asks for one.

The contract a caller actually needs is narrower: everything written before my seal is in L0. That is a predicate on a fixed set of outstanding flushes, not on the queue as it evolves.

What

force_seal_active now returns a SealFence: the flush-completion watchers outstanding at seal time, captured under the same lock that performs the freeze. SealFence::wait() awaits exactly those — bounded by construction, and unaffected by concurrent freezes.

The set is deliberately wider than the generation the call froze. A size/interval trigger freezes generations asynchronously, so an empty active memtable does not mean every pre-call write reached L0; the fence covers those pending generations too. What this call sealed is reporting only (SealFence::sealed_generation(), None on a no-op seal) and is not the wait target.

An earlier revision returned that generation for the caller to compare against the manifest's current_generation. That predicate was unsound (thanks @lance-gatekeeper): update_manifest advances current_generation to generation + 1 on every committed flush without checking for a gap, and the dispatcher logs a failed flush and keeps draining — so a later generation's success moves the watermark past one that failed and never reached L0. Waiting on per-generation flush outcomes avoids the manifest entirely. The durability hole underneath it is #8293.

Also reports the in-memory index set via IndexStore::index_names. An absent HNSW entry on a vector column is the whole explanation for a brute-force fresh-tier search, and there was no way to see it from outside the store.

Compatibility

force_seal_active's return type changes from Result<()> to Result<SealFence>. Existing .await?; call sites compile unchanged (the value is dropped); only code that binds or asserts on the unit value needs touching. No format or on-disk change.

Consumer

Sophon's WAL uses this to make POST /v1/table/{name}/flush bounded — it currently calls wait_for_flush_drain and inherits the unbounded-latency problem above.

Tests

  • test_force_seal_active_and_wait_for_flush_drain — extended to assert the fence names the frozen generation and that wait() is a working flush fence.
  • test_force_seal_active_fences_pending_generation_when_active_is_empty — an empty active memtable with a generation still awaiting flush must not produce a satisfied fence.
  • test_force_seal_active_fence_ignores_manifest_generation_advance — advancing current_generation past the sealed generation, as a later success would, must not satisfy its fence.

🤖 Generated with Claude Code

`wait_for_flush_drain` loops until the frozen-watcher set is *empty*,
re-collecting the set on each round — so it also waits on every memtable
frozen while it waits, including ones the size/interval trigger and
backpressure freeze concurrently. On a table under sustained write load
that set may never empty, which makes any post-seal ack unbounded on
exactly the workload that asks for one.

The contract a caller actually needs is narrower: *everything written
before my seal is in L0*. That is a predicate on one generation, not on
the queue. So `force_seal_active` now returns the `SealedGeneration` it
froze (`None` when the active memtable was empty, i.e. a no-op seal), and
a caller can wait on the shard manifest's `current_generation` exceeding
it — bounded by construction and unaffected by concurrent freezes.

Also add `IndexStore::describe`, which reports the in-memory index set
(kind + column) for diagnostics. An absent HNSW entry on a vector column
is the whole explanation for a brute-force fresh-tier search, and there
was no way to see it from outside.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the enhancement New feature or request label Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

`IndexStore::describe` existed only to feed sophon's `get_lsm_stats`, and
carried two types — `MemIndexDescriptor` and `MemIndexKind` — that nothing
else constructed or matched on. Replace it with `index_names`, which
answers the same diagnostic question (an absent name is why a fresh-tier
search on that column is brute-force) from the maps' keys alone. Kind and
column are one `list_indices` lookup away for the rare caller that needs
them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hamersaw
hamersaw marked this pull request as ready for review August 3, 2026 21:35

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gate recommendation: request changes.

The point-in-time fence is the right direction, but it must cover every generation through the call boundary and preserve failures within that fixed cutoff. A viable revision would return an exact per-cutoff completion fence, or first make manifest advancement contiguous with sticky failure handling, so later writes never enlarge or falsely satisfy the wait.

Please mark this PR with the breaking-change label.

Comment thread rust/lance/src/dataset/mem_wal/write.rs Outdated
Comment thread rust/lance/src/dataset/mem_wal/write.rs Outdated
Review found the `current_generation > sealed.generation` predicate
unsound in both directions, so `force_seal_active` now returns a
`SealFence` over the flushes actually outstanding at seal time instead of
a generation number to compare against the manifest.

`current_generation` was never a success predicate. `update_manifest`
advances it to `generation + 1` on every committed flush without checking
for a gap, and the dispatcher logs a failed flush and keeps draining — so
a later generation's success moves the watermark past one that failed and
never reached L0. The watermark reported durability that did not exist.

Nor was `None` on an empty active memtable a "nothing to wait for"
signal. A size/interval trigger swaps generation N for an empty N+1
before N's flush commits, so the caller got no watermark at exactly the
moment N was still pending, and could acknowledge the flush early.

Capturing the watcher set under the same lock that freezes fixes both:
the set is fixed at seal time (bounded no matter how many memtables
freeze during the wait, which is what `wait_for_flush_drain` gets wrong),
it covers generations frozen before the call, and each entry reports its
own flush's outcome, so a failure surfaces as an error rather than being
papered over by a later success.

`SealedGeneration` goes with it — `rows` had no caller, leaving a struct
wrapping one field, so the generation is now a plain `Option<u64>` on the
fence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gate recommendation: request changes.

The fixed watcher-set design is the right cutoff mechanism, but terminal flush outcomes are still either forgotten or left unresolved. A complete fence must keep every pre-call L0 failure sticky and must resolve with an error when its handler cannot deliver; otherwise it can both acknowledge missing data and hang indefinitely.

A viable revision would poison the writer or retain failed outcomes until recovery, and complete queued watchers with errors on dispatch failure or shutdown.

// whatever remains here is exactly what is still owed.
Ok(SealFence {
sealed_generation,
watchers: state

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Capturing only the currently queued watchers forgets a flush that has already failed. flush_memtable signals Failed, unconditionally pops that watcher, retains the failed frozen memtable, and the dispatcher keeps running. A later seal with an empty active table therefore captures an empty set and returns Ok(()) even though pre-call rows never reached L0. Make the L0 failure sticky—by poisoning the writer until recovery or retaining a fence-visible failed outcome—rather than treating “not pending” as “durable”.

Reproducer

I added this regression to the existing write.rs test module:

#[tokio::test]
async fn test_force_seal_active_remembers_settled_predecessor_failure() {
    use crate::utils::test::FailingProxyStore;
    use lance_io::object_store::ObjectStoreRegistry;

    let temp_dir = tempfile::tempdir().unwrap();
    let base_uri = format!("file://{}", temp_dir.path().display());
    let failing = Arc::new(FailingProxyStore::new());
    failing.fail_when("put", "_gen_", "injected L0 flush failure");
    failing.fail_when("put_multipart", "_gen_", "injected L0 flush failure");
    let store_params = ObjectStoreParams {
        object_store_wrapper: Some(failing),
        ..Default::default()
    };
    let registry = Arc::new(ObjectStoreRegistry::default());
    let (store, base_path) = ObjectStore::from_uri_and_params(
        registry.clone(), &base_uri, &store_params,
    )
    .await
    .unwrap();

    let schema = create_test_schema();
    let mut config = seal_fence_test_config(Uuid::new_v4());
    config.store_params = Some(store_params);
    config.session = Some(Arc::new(Session::new(0, 0, registry)));
    let writer = ShardWriter::open(
        store, base_path, base_uri, config, schema.clone(), vec![],
    )
    .await
    .unwrap();
    writer.put(vec![create_test_batch(&schema, 0, 10)]).await.unwrap();

    writer
        .force_seal_active().await.unwrap().wait().await
        .expect_err("the injected generation flush must fail");

    tokio::time::timeout(Duration::from_secs(5), async {
        loop {
            let is_empty = match &writer.mode {
                WriterMode::MemTable { state, .. } =>
                    state.read().await.frozen_flush_watchers.is_empty(),
                WriterMode::WalOnly { .. } => unreachable!(),
            };
            if is_empty {
                break;
            }
            tokio::task::yield_now().await;
        }
    })
    .await
    .unwrap();

    let later_fence = writer.force_seal_active().await.unwrap();
    assert_eq!(later_fence.sealed_generation(), None);
    later_fence
        .wait().await
        .expect_err("an earlier failed generation is still absent from L0");
}

cargo test -p lance test_force_seal_active_remembers_settled_predecessor_failure -- --nocapture failed at the final expect_err: the later fence returned Ok(()).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is fixed in #8295.

/// flush handler exited without reporting.
pub async fn wait(self) -> Result<()> {
for mut watcher in self.watchers {
match watcher.await_value().await {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This branch does not make handler exit observable while the writer is alive. The retained frozen memtable still owns the watch sender, and freeze_memtable discards a closed memtable-flush send, so await_value() can remain pending forever instead of returning None. Resolve the completion with an error when dispatch fails and during handler cleanup/cancellation so a fence cannot outlive the component that can satisfy it.

Reproducer

I ran cargo test -p lance test_force_seal_active -- --nocapture. The added test_force_seal_active_fences_pending_generation_when_active_is_empty shuts all handlers down before freezing and passes specifically because timeout(Duration::from_millis(200), fence.wait()).await.is_err(): the observed result is a timeout after handler exit, not the documented Err.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also handled in #8295.

@jackye1995 jackye1995 left a comment

Copy link
Copy Markdown
Contributor

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 5cd8aea into main Aug 6, 2026
43 checks passed
@jackye1995
jackye1995 deleted the feature/wal-checkpoint branch August 6, 2026 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants