Skip to content

fix(mem_wal): prevent cross-generation durability acknowledgements#7759

Open
u70b3 wants to merge 5 commits into
lance-format:mainfrom
u70b3:fix/memwal-cross-generation-durability
Open

fix(mem_wal): prevent cross-generation durability acknowledgements#7759
u70b3 wants to merge 5 commits into
lance-format:mainfrom
u70b3:fix/memwal-cross-generation-durability

Conversation

@u70b3

@u70b3 u70b3 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #7760.

  • Prevent durable MemTable writes from being acknowledged by a flush from an older MemTable generation.
  • Replace the MemTable write path's writer-global watermark acknowledgement with a request-scoped completion tied to the exact BatchStore range that accepted the write.
  • Preserve public method signatures, generation-local WriteResult positions, WAL-only behavior, bindings, and persistent formats.

Root cause

MemTable batch positions restart at zero after generation rotation, while the previous durability watcher compared those positions against a writer-global watermark. A successful flush from generation N could therefore satisfy a durable write at the same local position in generation N+1 before that write's WAL append completed.

Failure sequence

sequenceDiagram
    autonumber
    participant C as Client
    participant G0 as Writer A / generation N
    participant M as Shared durability watermark
    participant G1 as Writer A / generation N+1
    participant B as Writer B
    participant W as WAL

    C->>G0: durable put, local range 0..1
    G0->>W: flush generation N
    W-->>G0: success
    G0->>M: publish watermark 1
    Note over G0,G1: MemTable rotates and local positions restart at 0
    B->>W: claim a higher writer epoch
    C->>G1: durable put, local range 0..1
    G1->>M: wait for watermark >= 1
    M-->>G1: already satisfied by generation N
    G1-->>C: incorrect success
    G1->>W: flush generation N+1
    W-->>G1: PeerClaimedEpoch
Loading

Fix

  • Capture the accepting BatchStore and indexes while holding the writer lock and before a size-triggered freeze can replace the active MemTable.
  • Attach a request-specific completion cell to the flush and back the returned BatchDurableWatcher with that same cell.
  • Preserve typed peer-fence and persistence-failure errors, and report a closed flush handler as an I/O error.
  • Cover the already-durable/coalesced empty-flush completion path.

Validation

  • cargo test -p lance --test integration_tests mem_wal::durable_put_does_not_alias_across_memtable_generations -- --exact — passed (1/1)
  • cargo test -p lance dataset::mem_wal::wal::tests --lib — passed (20/20)
  • cargo test -p lance dataset::mem_wal::write::tests --lib — passed (53/53)
  • cargo fmt --all -- --check — passed
  • pre-commit run --files rust/lance/src/dataset/mem_wal/wal.rs rust/lance/src/dataset/mem_wal/write.rs rust/lance/tests/integration_tests.rs rust/lance/tests/mem_wal.rs — passed
  • cargo clippy --all --tests --benches -- -D warnings — passed
  • cargo clippy --all-features --tests --benches -- -D warnings — passed

Compatibility

  • No public API or binding changes.
  • No WAL, manifest, MemTable generation, or other persistent-format changes.
  • No dependency or lockfile changes.
  • WAL-only mode is unchanged.

Maintainer note

Please apply the critical-fix label: the bug could report a durable write as successful before its own WAL flush completed.

Summary by CodeRabbit

  • Bug Fixes
    • Improved durable write confirmations to use the specific per-request flush completion, preventing cross-request and cross-generation mix-ups.
    • Ensured durable write failures preserve the correct fencing reason, and added a clear I/O error when flush completion is missing or the flush handler exits early.
  • Tests
    • Added regression tests for coalesced/empty flush handling.
    • Added durable watcher isolation tests across memtable generations, including persistence-failure behavior.
  • Chores
    • Expanded the MemWAL integration test coverage.

@github-actions github-actions Bot added the bug Something isn't working label Jul 13, 2026
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Durable MemTable writes now use request-scoped WAL flush completion cells, preventing acknowledgements from older generations. Watchers propagate successful completion, typed failures, and handler-exit errors, with unit and integration coverage.

Changes

WAL durability watcher flow

Layer / File(s) Summary
Dual-mode durability watcher
rust/lance/src/dataset/mem_wal/wal.rs
BatchDurableWatcher supports watermark-based and request-scoped flush-completion states, including typed failures and missing-completion I/O errors. Unit tests cover success, failure propagation, handler closure, and poisoned watermark errors.
Per-put flush completion wiring
rust/lance/src/dataset/mem_wal/write.rs
Durable MemTable puts capture request-specific state, create a fresh completion cell and watcher, pass the cell to WalFlusher, and test coalesced flushes and watcher isolation.
Cross-generation fencing coverage
rust/lance/tests/integration_tests.rs, rust/lance/tests/mem_wal.rs
Integration coverage verifies durable writes are fenced after a later writer claims a newer epoch across memtable generations.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

  • lance-format/lance#7769: Both changes wire per-flush completion cells into WAL flush handling and propagate typed or missing-completion failures.

Suggested reviewers: hamersaw, touch-of-grey, wjones127

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant put_memtable_no_wait
  participant WalFlusher
  participant WatchableOnceCell
  participant BatchDurableWatcher
  Client->>put_memtable_no_wait: durable put
  put_memtable_no_wait->>WatchableOnceCell: create request completion
  put_memtable_no_wait->>WalFlusher: trigger_flush with captured batch range
  WalFlusher->>WatchableOnceCell: write flush success or typed failure
  BatchDurableWatcher->>WatchableOnceCell: await request completion
  WatchableOnceCell-->>BatchDurableWatcher: return durability result
  BatchDurableWatcher-->>Client: resolve durability or failure
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the core fix to MemWAL durability acknowledgements.
Linked Issues check ✅ Passed The changes implement request-scoped flush completion, preserve typed errors and APIs, and add the requested regression coverage for #7760.
Out of Scope Changes check ✅ Passed The added test wiring and integration coverage are directly related to the MemWAL durability fix and do not appear out of scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.09091% with 24 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance/src/dataset/mem_wal/wal.rs 80.70% 20 Missing and 2 partials ⚠️
rust/lance/src/dataset/mem_wal/write.rs 98.11% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@u70b3 u70b3 force-pushed the fix/memwal-cross-generation-durability branch from 6e30389 to 43a408f Compare July 14, 2026 02:42

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
rust/lance/src/dataset/mem_wal/wal.rs (1)

91-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Design is sound; doc comments lack examples and enum-variant semantics.

The request-scoped FlushCompletion backend correctly resolves the cross-generation aliasing bug (completion is tied to the specific triggering BatchStore/request rather than a shared watermark), and wait()/is_durable() correctly branch per backend. Two doc gaps per the coding guideline:

  • BatchDurabilityState::Watermark/FlushCompletion (Lines 92-101) have no doc comments explaining what each variant represents.
  • The public BatchDurableWatcher struct/wait()/is_durable() docs (Lines 103-110, 137-144, 179-180) describe the concept but include no runnable example and don't link to sibling methods (e.g. [Self::wait]/[Self::is_durable]).
📝 Example doc additions
 enum BatchDurabilityState {
+    /// Legacy watermark-based tracking: durable once the shared watermark
+    /// reaches `target_batch_position`.
     Watermark {
         rx: watch::Receiver<usize>,
         target_batch_position: usize,
         terminal_error: Arc<StdMutex<Option<WalFlushFailure>>>,
     },
+    /// Request-scoped tracking: durable only when the specific triggering
+    /// flush (tied to one `BatchStore` generation) completes.
     FlushCompletion {
         reader: WatchableOnceCellReader<std::result::Result<WalFlushResult, WalFlushFailure>>,
     },
 }

As per coding guidelines, "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods" and "Document enum variants with behavioral semantics, not just labels."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/dataset/mem_wal/wal.rs` around lines 91 - 221, Add
documentation for both BatchDurabilityState variants describing their behavioral
semantics, including watermark tracking versus request-scoped flush completion.
Expand BatchDurableWatcher, wait, and is_durable documentation with runnable
Rust examples and cross-links to related methods such as [Self::wait] and
[Self::is_durable], while preserving the existing API behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/wal.rs`:
- Around line 91-221: Add documentation for both BatchDurabilityState variants
describing their behavioral semantics, including watermark tracking versus
request-scoped flush completion. Expand BatchDurableWatcher, wait, and
is_durable documentation with runnable Rust examples and cross-links to related
methods such as [Self::wait] and [Self::is_durable], while preserving the
existing API behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8a880222-4c62-4ea8-b5c8-60418127b0a6

📥 Commits

Reviewing files that changed from the base of the PR and between 6e30389 and 43a408f.

📒 Files selected for processing (4)
  • rust/lance/src/dataset/mem_wal/wal.rs
  • rust/lance/src/dataset/mem_wal/write.rs
  • rust/lance/tests/integration_tests.rs
  • rust/lance/tests/mem_wal.rs

@u70b3 u70b3 force-pushed the fix/memwal-cross-generation-durability branch from 43a408f to 9ae0702 Compare July 14, 2026 14:24

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rust/lance/src/dataset/mem_wal/wal.rs`:
- Around line 103-107: Expand the public watcher documentation around the
watcher type and its methods to include a runnable usage example showing
construction, awaiting durability, and checking status. Add intra-doc links to
Self::new, Self::wait, Self::is_durable, and the relevant ShardWriter method,
preserving the existing compatibility and request-scoped durability behavior.

In `@rust/lance/tests/mem_wal.rs`:
- Around line 12-14: Standardize all affected memory WAL tests on the canonical
in-memory URI: in rust/lance/tests/mem_wal.rs lines 12-14, replace the temporary
directory and filesystem URI setup with ObjectStore::from_uri("memory://"); in
rust/lance/src/dataset/mem_wal/write.rs line 3705, replace create_local_store()
with an in-memory object store; and at line 4890, change memory:/// to
memory://. Do not add counters or unique URI suffixes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2f78ee21-5656-40a8-bccf-0b9b9f22ac32

📥 Commits

Reviewing files that changed from the base of the PR and between 43a408f and 9ae0702.

📒 Files selected for processing (4)
  • rust/lance/src/dataset/mem_wal/wal.rs
  • rust/lance/src/dataset/mem_wal/write.rs
  • rust/lance/tests/integration_tests.rs
  • rust/lance/tests/mem_wal.rs

Comment on lines +103 to +107
/// Watcher for batch durability.
///
/// Uses a shared watch channel that broadcasts the durable watermark.
/// The watcher waits until the watermark reaches or exceeds its target batch ID.
/// Publicly constructed watchers retain watermark-based tracking for
/// compatibility. MemTable writes use a request-scoped flush completion so a
/// watermark from another generation cannot acknowledge their data.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add an example and intra-doc links for the public watcher API.

Document how callers obtain and await this watcher, and link [Self::new], [Self::wait], [Self::is_durable], and the relevant ShardWriter method.

As per coding guidelines: “All public APIs must have documentation with examples, and documentation should link to relevant structs and methods.”

Also applies to: 114-127, 137-191

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/dataset/mem_wal/wal.rs` around lines 103 - 107, Expand the
public watcher documentation around the watcher type and its methods to include
a runnable usage example showing construction, awaiting durability, and checking
status. Add intra-doc links to Self::new, Self::wait, Self::is_durable, and the
relevant ShardWriter method, preserving the existing compatibility and
request-scoped durability behavior.

Source: Coding guidelines

Comment on lines +12 to +14
let temp_dir = tempfile::tempdir().unwrap();
let base_uri = format!("file://{}", temp_dir.path().display());
let (store, base_path) = ObjectStore::from_uri(&base_uri).await.unwrap();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Standardize the new tests on the canonical memory:// URI.

  • rust/lance/tests/mem_wal.rs#L12-L14: replace the temporary filesystem store with ObjectStore::from_uri("memory://").
  • rust/lance/src/dataset/mem_wal/write.rs#L3705-L3705: replace create_local_store() with an in-memory object store.
  • rust/lance/src/dataset/mem_wal/write.rs#L4890-L4890: change memory:/// to memory://.

As per coding guidelines: “Use plain "memory://" URIs in tests; no atomic counters or unique suffixes are needed.”

📍 Affects 2 files
  • rust/lance/tests/mem_wal.rs#L12-L14 (this comment)
  • rust/lance/src/dataset/mem_wal/write.rs#L3705-L3705
  • rust/lance/src/dataset/mem_wal/write.rs#L4890-L4890
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/tests/mem_wal.rs` around lines 12 - 14, Standardize all affected
memory WAL tests on the canonical in-memory URI: in rust/lance/tests/mem_wal.rs
lines 12-14, replace the temporary directory and filesystem URI setup with
ObjectStore::from_uri("memory://"); in rust/lance/src/dataset/mem_wal/write.rs
line 3705, replace create_local_store() with an in-memory object store; and at
line 4890, change memory:/// to memory://. Do not add counters or unique URI
suffixes.

Source: Coding guidelines

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
rust/lance/src/dataset/mem_wal/wal.rs (1)

144-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Watcher docs still lack an example and full cross-links.

The new [Self::is_durable]/[Self::wait] cross-links are a good partial fix, but the public BatchDurableWatcher API (new, wait, is_durable) still has no runnable usage example, and neither method links back to Self::new or the relevant ShardWriter method that constructs/consumes it. This is the same gap flagged in the prior review round.

As per coding guidelines: "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/dataset/mem_wal/wal.rs` around lines 144 - 203, The public
BatchDurableWatcher API documentation needs a runnable usage example and
complete cross-links. Update the docs for BatchDurableWatcher::new,
BatchDurableWatcher::wait, and BatchDurableWatcher::is_durable to include an
appropriate example and links to Self::new plus the relevant ShardWriter method
that creates or consumes the watcher; retain the existing wait/is_durable
relationship links.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/wal.rs`:
- Around line 144-203: The public BatchDurableWatcher API documentation needs a
runnable usage example and complete cross-links. Update the docs for
BatchDurableWatcher::new, BatchDurableWatcher::wait, and
BatchDurableWatcher::is_durable to include an appropriate example and links to
Self::new plus the relevant ShardWriter method that creates or consumes the
watcher; retain the existing wait/is_durable relationship links.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0f69c551-d4b2-407c-9033-c5f42bdeaabb

📥 Commits

Reviewing files that changed from the base of the PR and between 9ae0702 and eb53323.

📒 Files selected for processing (2)
  • rust/lance/src/dataset/mem_wal/wal.rs
  • rust/lance/src/dataset/mem_wal/write.rs

@u70b3 u70b3 force-pushed the fix/memwal-cross-generation-durability branch from eb53323 to ba772ed Compare July 14, 2026 16:57

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (1)
rust/lance/tests/mem_wal.rs (1)

12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use memory:// instead of a temp-dir file:// store.

Per repo convention this new integration test should use an in-memory object store rather than a temporary filesystem directory.

As per coding guidelines: "Use plain \"memory://\" URIs in tests; no atomic counters or unique suffixes are needed."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/tests/mem_wal.rs` around lines 12 - 14, Update the test setup
around ObjectStore::from_uri to use the plain memory:// URI instead of creating
a tempfile directory and constructing a file:// URI. Remove the unnecessary
temp_dir and base_uri setup while preserving the existing store and base_path
initialization.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rust/lance/src/dataset/mem_wal/wal.rs`:
- Around line 144-203: Add runnable Rust doc examples to the public
BatchDurableWatcher methods wait and is_durable, showing a caller with an
obtained BatchDurableWatcher checking is_durable and awaiting wait when needed.
Keep the example compilable using the existing lance::Result and watcher types,
and retain links to the related methods.

In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 4897-4900: Update the base_uri value in
test_durable_watcher_does_not_alias_across_memtable_generations from the
noncanonical "memory:///" form to the plain "memory://" form, preserving the
rest of the test unchanged.
- Around line 3706-3715: The new test
test_coalesced_empty_flush_completes_request_watcher should use an in-memory
store with the plain memory:// URI instead of create_local_store() and its
temporary file-backed setup. Remove the temp-directory dependency while
preserving the existing WAL appender/flusher test behavior.

---

Duplicate comments:
In `@rust/lance/tests/mem_wal.rs`:
- Around line 12-14: Update the test setup around ObjectStore::from_uri to use
the plain memory:// URI instead of creating a tempfile directory and
constructing a file:// URI. Remove the unnecessary temp_dir and base_uri setup
while preserving the existing store and base_path initialization.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e455bee4-9e08-450b-a161-8237397f096f

📥 Commits

Reviewing files that changed from the base of the PR and between eb53323 and ba772ed.

📒 Files selected for processing (4)
  • rust/lance/src/dataset/mem_wal/wal.rs
  • rust/lance/src/dataset/mem_wal/write.rs
  • rust/lance/tests/integration_tests.rs
  • rust/lance/tests/mem_wal.rs

Comment on lines +144 to 203
/// Wait until the watched batch is durable.
///
/// Watermark-backed watchers return success when the durable watermark
/// reaches the target, and return an error for a terminal flush failure or
/// closed tracking channel. Request-scoped watchers return success only
/// after their flush completes, preserve typed flush failures, and return an
/// I/O error if the flush handler exits without reporting completion.
///
/// Returns Ok(()) when `durable_watermark >= target_batch_position`, or
/// Err if a terminal flush failure (e.g. a fence) means the watermark can
/// never reach the target.
/// For a non-blocking check see [`Self::is_durable`].
pub async fn wait(&mut self) -> Result<()> {
loop {
if let Some(failure) = self.terminal_error.lock().unwrap().clone() {
return Err(failure.into_error());
}
let current = *self.rx.borrow();
if current >= self.target_batch_position {
return Ok(());
}
self.rx
.changed()
.await
.map_err(|_| Error::io("Durable watermark channel closed"))?;
match &mut self.state {
BatchDurabilityState::Watermark {
rx,
target_batch_position,
terminal_error,
} => loop {
let terminal_failure = terminal_error
.lock()
.map_err(|_| {
Error::internal(
"Batch durability terminal error mutex was poisoned".to_string(),
)
})?
.clone();
if let Some(failure) = terminal_failure {
return Err(failure.into_error());
}
if *rx.borrow() >= *target_batch_position {
return Ok(());
}
rx.changed()
.await
.map_err(|_| Error::io("Durable watermark channel closed"))?;
},
BatchDurabilityState::FlushCompletion { reader } => match reader.await_value().await {
Some(Ok(_)) => Ok(()),
Some(Err(failure)) => Err(failure.into_error()),
None => Err(Error::io(
"WAL flush handler exited before reporting durability",
)),
},
}
}

/// Check if the batch is already durable (non-blocking).
///
/// Cheaper, non-awaiting counterpart to [`Self::wait`]: returns the
/// current state without blocking for the flush to complete.
pub fn is_durable(&self) -> bool {
*self.rx.borrow() >= self.target_batch_position
match &self.state {
BatchDurabilityState::Watermark {
rx,
target_batch_position,
..
} => *rx.borrow() >= *target_batch_position,
BatchDurabilityState::FlushCompletion { reader } => {
matches!(reader.read(), Some(Ok(_)))
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Doc examples still missing for wait()/is_durable().

Intra-doc links to [Self::wait]/[Self::is_durable] were added since the last review, but neither method has a runnable doc example showing how a caller constructs/obtains a watcher, awaits durability, and checks status.

As per coding guidelines: "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods."

📚 Suggested example
/// # Example
/// ```
/// # use lance::dataset::mem_wal::BatchDurableWatcher;
/// # async fn doc(mut watcher: BatchDurableWatcher) -> lance::Result<()> {
/// if !watcher.is_durable() {
///     watcher.wait().await?;
/// }
/// # Ok(())
/// # }
/// ```
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/dataset/mem_wal/wal.rs` around lines 144 - 203, Add runnable
Rust doc examples to the public BatchDurableWatcher methods wait and is_durable,
showing a caller with an obtained BatchDurableWatcher checking is_durable and
awaiting wait when needed. Keep the example compilable using the existing
lance::Result and watcher types, and retain links to the related methods.

Source: Coding guidelines

Comment on lines +3706 to +3715
#[tokio::test]
async fn test_coalesced_empty_flush_completes_request_watcher() {
let (store, base_path, _base_uri, _temp_dir) = create_local_store().await;
let shard_id = Uuid::new_v4();
let wal_appender = Arc::new(
WalAppender::open(store, base_path, shard_id, 0)
.await
.unwrap(),
);
let wal_flusher = Arc::new(WalFlusher::new(wal_appender));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use memory:// instead of a temp-dir file:// store for this new test.

create_local_store() backs this test with a real filesystem temp dir; per repo convention new tests should use an in-memory store instead.

As per coding guidelines: "Use plain \"memory://\" URIs in tests; no atomic counters or unique suffixes are needed."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/dataset/mem_wal/write.rs` around lines 3706 - 3715, The new
test test_coalesced_empty_flush_completes_request_watcher should use an
in-memory store with the plain memory:// URI instead of create_local_store() and
its temporary file-backed setup. Remove the temp-directory dependency while
preserving the existing WAL appender/flusher test behavior.

Source: Coding guidelines

Comment on lines +4897 to +4900
async fn test_durable_watcher_does_not_alias_across_memtable_generations() {
let (store, base_path, controls) = failing_memory_store().await;
let base_uri = "memory:///";
let schema = create_test_schema();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

"memory:///" should be "memory://".

Extra trailing slash deviates from the canonical form specified by the guideline.

As per coding guidelines: "Use plain \"memory://\" URIs in tests; no atomic counters or unique suffixes are needed."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/dataset/mem_wal/write.rs` around lines 4897 - 4900, Update the
base_uri value in
test_durable_watcher_does_not_alias_across_memtable_generations from the
noncanonical "memory:///" form to the plain "memory://" form, preserving the
rest of the test unchanged.

Source: Coding guidelines

u70b3 and others added 5 commits July 15, 2026 13:06
Doc-comment-only follow-up to the request-scoped durability fix:

- State the ordering invariant at the `ShardWriter::put` lock block:
  the accepting BatchStore/indexes must be captured BEFORE the
  `maybe_trigger_*` calls that can freeze the active MemTable, or the
  request-scoped flush completion binds to the wrong (new) generation.
- Document the `BatchDurabilityState` variants' behavioral semantics,
  noting that `Watermark` is cross-generation and unsafe for durable
  MemTable writes (lance-format#7760), while `FlushCompletion` is request-scoped.
- Cross-link `BatchDurableWatcher::wait` and `is_durable`.
- Document the lib regression test's failure mode so the assertion
  ties back to the cross-generation aliasing bug.

No behavioral change. cargo fmt, cargo clippy -p lance --tests -D
warnings, and the full mem_wal lib suite (515 tests) pass.

Co-Authored-By: Claude <noreply@anthropic.com>
@u70b3 u70b3 force-pushed the fix/memwal-cross-generation-durability branch from ba772ed to e6ce2d5 Compare July 15, 2026 05:06
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.

bug: durable MemTable writes can be acknowledged by an older generation

1 participant