fix(mem-wal): propagate final flush failures from ShardWriter::close#7769
fix(mem-wal): propagate final flush failures from ShardWriter::close#7769u70b3 wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesShardWriter close reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ShardWriter
participant WALFlushHandler
participant FrozenMemtableWatchers
participant BackgroundTasks
ShardWriter->>WALFlushHandler: Trigger and await final WAL flush
WALFlushHandler-->>ShardWriter: Return completion or failure
ShardWriter->>FrozenMemtableWatchers: Await frozen memtable flushes
FrozenMemtableWatchers-->>ShardWriter: Return completion or durability failure
ShardWriter->>BackgroundTasks: Shutdown and join tasks
BackgroundTasks-->>ShardWriter: Return shutdown result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/write.rs (1)
2310-2330: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate "trigger + await_final_wal_flush" boilerplate between MemTable and WalOnly close paths.
Both blocks are structurally identical (build
WatchableOnceCell,.send(TriggerWalFlush{...}).is_ok(),await_final_wal_flushon success, else the sameError::io("WAL flush channel closed during close")), differing only in the channel handle andWalFlushSourcevariant. Sinceawait_final_wal_flushwas already extracted for the reader-await half, extending the same treatment to the send+trigger half removes the remaining duplication.♻️ Suggested extraction
async fn send_final_wal_flush( wal_flush_tx: &mpsc::UnboundedSender<TriggerWalFlush>, source: WalFlushSource, end_batch_position: usize, ) -> Result<()> { let done = WatchableOnceCell::new(); let reader = done.reader(); if wal_flush_tx .send(TriggerWalFlush { source, end_batch_position, done: Some(done) }) .is_ok() { Self::await_final_wal_flush(reader).await } else { Err(Error::io("WAL flush channel closed during close")) } }Both call sites then collapse to a single
Self::send_final_wal_flush(&tx, source, end_position).await.Also applies to: 2379-2397
🤖 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 2310 - 2330, Extract the duplicated final WAL flush trigger-and-wait logic from the close paths into a shared async helper near await_final_wal_flush, accepting the WAL flush sender, WalFlushSource, and end batch position. Have it create the WatchableOnceCell, send TriggerWalFlush, await completion on success, and return the existing channel-closed Error otherwise; replace both MemTable and WalOnly close blocks with calls to this helper.
🤖 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/write.rs`:
- Line 2295: Update the close-stage error merging around close_result and
shutdown_result so secondary failures are logged before being discarded. Add a
shared merge_close_stage helper that logs the stage name and secondary error
when both results fail, then replace each close_result.and(stage_result) and
shutdown_result merge with that helper while preserving the first error as the
returned result.
- Around line 4330-4375: Combine this close-failure test with the equivalent
close-failure test into a single #[rstest] case set. Parameterize the writer
mode/config differences, while keeping the shared setup, close invocation,
fence-reason and error assertions, and background-task cleanup assertion in one
test body.
---
Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 2310-2330: Extract the duplicated final WAL flush trigger-and-wait
logic from the close paths into a shared async helper near
await_final_wal_flush, accepting the WAL flush sender, WalFlushSource, and end
batch position. Have it create the WatchableOnceCell, send TriggerWalFlush,
await completion on success, and return the existing channel-closed Error
otherwise; replace both MemTable and WalOnly close blocks with calls to this
helper.
🪄 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: fe621e8b-b147-4084-827b-527eb2c2d81a
📒 Files selected for processing (1)
rust/lance/src/dataset/mem_wal/write.rs
|
Addressed the CodeRabbit outside-diff duplication finding in 3f9093c. The two writer modes now call one private flush_final_wal helper for completion-cell creation, TriggerWalFlush delivery, channel-close handling, and durability-result conversion; mode-specific source and end position remain at the call sites. Follow-up verification: dataset::mem_wal 502 passed / 1 ignored, cargo fmt passed, and workspace clippy passed with warnings denied. |
u70b3
force-pushed
the
fix/mem-wal-close-flush-errors
branch
from
e940cbe to
ebb2bca
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/lance/src/dataset/mem_wal/write.rs (2)
2309-2317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing doc example on
close().The
# Errorssection was updated to describe the new failure surface, but the doc comment still has no usage example, unlikedelete()/put_no_wait()elsewhere in this file. Sinceclose(self)consumes the writer, an example would need an owned-writer signature similar to the pattern used for other consuming methods.📝 Suggested example
/// ``` /// # use lance::Result; /// # use lance::dataset::mem_wal::ShardWriter; /// # async fn doc(writer: ShardWriter) -> Result<()> { /// writer.close().await?; /// # Ok(()) /// # } /// ```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/write.rs` around lines 2309 - 2317, Add a Rustdoc usage example to the public `close` method documentation, using an owned `ShardWriter`, an async example function returning `lance::Result`, and awaiting `writer.close()` with error propagation. Keep the existing error documentation and behavior unchanged.Source: Coding guidelines
5678-5725: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve typed fence reasons on the frozen-MemTable close path.
close()still drops this failure throughDurabilityResult::Failed(String) -> Error::io, so a peer fence here losesFenceReasonanderror.fence_reason()returnsNone(rust/lance/src/dataset/mem_wal/write.rs:2383-2385, 2759-2763). If typed fence reasons are part of the close contract, this path needs to carry the typed failure through the MemTable watcher too.🤖 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 5678 - 5725, The frozen-MemTable close flow must preserve the typed FenceReason when a peer fence causes flush failure instead of converting it to a string-based IO error. Update the MemTable watcher and close handling around DurabilityResult::Failed, including the relevant failure propagation near ShardWriter::close, so the original typed fence error reaches the returned Error and error.fence_reason() remains available; retain existing behavior for non-fence failures.
🤖 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/write.rs`:
- Around line 2309-2317: Add a Rustdoc usage example to the public `close`
method documentation, using an owned `ShardWriter`, an async example function
returning `lance::Result`, and awaiting `writer.close()` with error propagation.
Keep the existing error documentation and behavior unchanged.
- Around line 5678-5725: The frozen-MemTable close flow must preserve the typed
FenceReason when a peer fence causes flush failure instead of converting it to a
string-based IO error. Update the MemTable watcher and close handling around
DurabilityResult::Failed, including the relevant failure propagation near
ShardWriter::close, so the original typed fence error reaches the returned Error
and error.fence_reason() remains available; retain existing behavior for
non-fence failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 31f63890-11ec-46b5-b0e2-43edd20804ec
📒 Files selected for processing (1)
rust/lance/src/dataset/mem_wal/write.rs
u70b3
force-pushed
the
fix/mem-wal-close-flush-errors
branch
2 times, most recently
from
f1eb4ad to
568703c
Compare
There was a problem hiding this comment.
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/write.rs`:
- Around line 2309-2321: Add a concise # Examples section to the public
ShardWriter::close method documentation, showing a caller awaiting close() and
handling its Result. Link the example to the relevant ShardWriter API and
preserve the existing multi-stage # Errors documentation.
- Around line 2367-2394: The frozen MemTable watcher handling in close()
currently converts DurabilityResult::Failed into Error::io and loses FenceReason
metadata. Update the watcher result mapping around await_value() to use the
typed WalFlushFailure::into_error() conversion, preserving fence causes while
retaining the existing missing-watcher completion error and merge_close_stage
flow.
🪄 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: d76b3e23-a452-4089-9c47-23759cf28548
📒 Files selected for processing (1)
rust/lance/src/dataset/mem_wal/write.rs
u70b3
force-pushed
the
fix/mem-wal-close-flush-errors
branch
from
36f39b0 to
1158476
Compare
`ShardWriter::close()` awaited three close-time completion channels but discarded their values, and ignored failures to send the final WAL flush requests. As a result, close could return `Ok(())` after WAL persistence failure, peer fencing, L0 flush failure, handler shutdown, or completion-channel closure. The WAL-only case was especially unsafe: an unsuccessful append remained only in the in-memory pending queue, which was dropped when the consuming close returned success. The close path now maintains a first-error accumulator and executes the complete shutdown sequence in operational order: 1. request and await the final WAL flush; 2. freeze the active MemTable; 3. await every frozen MemTable flush; 4. shut down all background tasks; 5. return the first error encountered. `WalFlushFailure::into_error()` preserves typed fence reasons. Frozen MemTable results continue to use the existing `DurabilityResult` conversion. A private close-specific helper (`flush_final_wal`) centralizes WAL completion handling for both writer modes, sending directly on the channel rather than via `WalFlusher::trigger_flush` (which silently returns `Ok` when `flush_tx` is unset, masking a close-path send failure). A second helper (`merge_close_stage`) preserves the first causal error while logging secondary stage failures, and the final close error is logged at WARN so a single-stage failure is observable. Tests cover: WAL-only and MemTable final WAL persistence failure with first-error preservation and background task join; frozen MemTable/L0 fencing failure; and `merge_close_stage` first-error semantics. The stricter close behavior also exposed three indexed-flush fixtures using `memory://` even though generation flushing reopens the dataset through an independent object-store registry; those fixtures now use isolated `shared-memory://` authorities so the reopened generation observes the same backing bytes. Closes lance-format#7770
u70b3
force-pushed
the
fix/mem-wal-close-flush-errors
branch
from
1158476 to
d54662b
Compare
1 participant
Closes #7770
Summary
ShardWriter::close()error contract.Root cause
ShardWriter::close()awaited three close-time completion channels but discardedtheir values. It also ignored failures to send the final WAL flush requests.
As a result, close could return
Ok(())after WAL persistence failure, peerfencing, L0 flush failure, handler shutdown, or completion-channel closure.
The WAL-only case was especially unsafe: an unsuccessful append remained only in
the in-memory pending queue, which was dropped when the consuming close returned
success.
Control-flow comparison
flowchart TD M["MemTable final WAL"] --> C["Close-time completion"] L["Frozen MemTable / L0 flush"] --> C W["WAL-only final WAL"] --> C C --> OLD["Before: discard completion value"] OLD --> OLD_SHUTDOWN["Shutdown tasks"] OLD_SHUTDOWN --> FALSE_OK["Return Ok even after persistence failure"] C --> NEW["After: convert outcome to Result"] NEW --> FIRST["Preserve the first causal error"] FIRST --> DRAIN["Continue draining remaining watchers"] DRAIN --> NEW_SHUTDOWN["Always shut down tasks"] NEW_SHUTDOWN --> RETURN["Return first error, otherwise Ok"]Fix
The close path now maintains a first-error accumulator and executes the complete
shutdown sequence in operational order:
WalFlushFailure::into_error()preserves typed fence reasons. Frozen MemTableresults continue to use the existing
DurabilityResultconversion.A private close-specific helper centralizes WAL completion handling for both
writer modes.
Tests
Added failure-focused coverage for:
The stricter close behavior also exposed three indexed-flush fixtures using
memory://even though generation flushing reopens the dataset through anindependent object-store registry. Those fixtures now use isolated
shared-memory://authorities so the reopened generation observes the samebacking bytes.
Validation
cargo test -p lance --lib dataset::mem_wal::write::testscargo test -p lance --lib dataset::mem_walcargo fmt --all -- --checkpre-commit run --files rust/lance/src/dataset/mem_wal/write.rscargo clippy --all --tests --benches -- -D warningscargo clippy --all-features --tests --benches -- -D warningsCompatibility
Implementation notes
Beyond propagating the three close-time failures, the close path also:
successor failure is logged without replacing the first causal error (the
accompanying comment documents this first-error-preserving drain behavior);
WalFlusher::trigger_flush, which silently returnsOkwhen the flusher'sflush_txis unset and would let close acknowledge durability it neverachieved — a closed send channel must surface as an error here;
(
merge_close_stageonly logs the secondary error when both stages fail).Maintainers: please consider applying the
critical-fixlabel because the oldbehavior could acknowledge a graceful close after final persistence failed.
Summary by CodeRabbit