fix(mem-wal): expose typed writer fence detection#7896
Open
beinan wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughMemWAL peer-epoch fencing now uses source-aware typed errors across manifest conflict handling, error cloning, memtable flush completion, writer shutdown, and regression tests. Ambiguous manifest write outcomes remain non-fencing when the write may have committed. ChangesMemWAL fencing
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ObjectStore
participant Manifest
participant MemTableFlushHandler
participant ShardWriter
ObjectStore-->>Manifest: return sourced manifest write error
Manifest->>Manifest: detect confirmed version conflict
Manifest-->>ShardWriter: fenced_by_peer or may-have-committed error
MemTableFlushHandler->>ShardWriter: signal typed WalFlushFailure
ShardWriter->>ShardWriter: drain typed completion watcher
ShardWriter-->>ShardWriter: return is_mem_wal_fenced() result
🚥 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: 4
🤖 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-core/src/error.rs`:
- Around line 987-992: Preserve nested fence reasons in both propagation paths:
in rust/lance-core/src/error.rs lines 987-992, update the Error::Fenced cloning
logic to recover FenceReason from the wrapped source before falling back to
string-only cloning; in rust/lance/src/dataset/mem_wal/write.rs lines 630-644,
make WalFlushFailure extraction inspect error sources and add round-trip tests
for wrapped peer-claimed-epoch and persistence-failure fences.
- Around line 489-501: Add a Rustdoc # Examples section to the public
Error::is_mem_wal_fenced method, using a compiling example that demonstrates
true for peer epoch takeovers and false for persistence failures. Include
documentation links to Error::fenced_by_peer, Error::writer_poisoned, and
Error::fence_reason, keeping the example aligned with the documented
peer/persistence distinction.
In `@rust/lance/src/dataset/mem_wal/manifest.rs`:
- Around line 485-488: Update the shard-claim error handling around the
conditional manifest write so a reread showing latest_epoch equal to next_epoch
is not treated as peer fencing after an ambiguous or lost response. Distinguish
confirmed conditional conflicts from ambiguous I/O, or validate ownership with a
unique claim token before returning Error::fenced_by_peer; add a regression test
covering a committed write whose response is lost.
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 611-614: Preserve the existing public `DurabilityResult::Failed`
payload type so downstream constructors and pattern matches remain compatible.
Revert the variant change from `String` to `WalFlushFailure`, or introduce an
explicit backward-compatible migration path before exposing the typed failure
carrier.
🪄 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: QUIET
Plan: Pro Plus
Run ID: d761c4ac-2873-4755-a047-641e5f3b0d06
📒 Files selected for processing (4)
rust/lance-core/src/error.rsrust/lance/src/dataset/mem_wal/manifest.rsrust/lance/src/dataset/mem_wal/wal.rsrust/lance/src/dataset/mem_wal/write.rs
There was a problem hiding this comment.
Actionable comments posted: 1
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/memtable.rs (1)
343-368: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMake completion-cell lifecycle failures explicit.
Line 348 panics when the cell is absent, while Lines 364-368 silently drop a duplicate/missing completion. Return a descriptive
Resultfrom reader creation and at leastdebug_assert!on an unexpected second signal.As per coding guidelines, “Never use
.unwrap(),.expect(),panic!(), orassert!()in library code for fallible operations” and “Do not silently guard against impossible conditions.”🤖 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/memtable.rs` around lines 343 - 368, Update create_typed_memtable_flush_completion to return a descriptive Result instead of unwrapping and expecting the completion cell, propagating lock or missing-cell failures without panicking. In signal_memtable_flush_complete and signal_typed_memtable_flush_complete, retain one-shot completion behavior but add debug_assert! checks for an absent cell so duplicate or missing signals are no longer silently ignored.Source: Coding guidelines
🧹 Nitpick comments (1)
rust/lance-core/src/error.rs (1)
1062-1077: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
rstestcases for these parameterized fence tests
rust/lance-core/src/error.rs#L1062-L1077rust/lance/src/dataset/mem_wal/wal.rs#L1737-L1757Replace the manual
forloops with named#[case::...]tests so these fence cases follow the repo’s parameterized-test convention.🤖 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-core/src/error.rs` around lines 1062 - 1077, Replace the manual parameter loop in cloneable_error_preserves_nested_fence_reason in rust/lance-core/src/error.rs:1062-1077 with an rstest parameterized test using descriptive named cases for each fence reason. Apply the same conversion to the corresponding fence test in rust/lance/src/dataset/mem_wal/wal.rs:1737-1757, preserving each case’s inputs and assertions.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/test_util.rs`:
- Around line 60-65: Document the public methods fail_manifest_puts and
set_manifest_lost_ack with Rustdoc describing their manifest-failure controls,
including examples showing configuration usage. Add links to
failing_memory_store and FailControls, following the repository’s public API
documentation conventions.
---
Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/memtable.rs`:
- Around line 343-368: Update create_typed_memtable_flush_completion to return a
descriptive Result instead of unwrapping and expecting the completion cell,
propagating lock or missing-cell failures without panicking. In
signal_memtable_flush_complete and signal_typed_memtable_flush_complete, retain
one-shot completion behavior but add debug_assert! checks for an absent cell so
duplicate or missing signals are no longer silently ignored.
---
Nitpick comments:
In `@rust/lance-core/src/error.rs`:
- Around line 1062-1077: Replace the manual parameter loop in
cloneable_error_preserves_nested_fence_reason in
rust/lance-core/src/error.rs:1062-1077 with an rstest parameterized test using
descriptive named cases for each fence reason. Apply the same conversion to the
corresponding fence test in rust/lance/src/dataset/mem_wal/wal.rs:1737-1757,
preserving each case’s inputs and assertions.
🪄 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: QUIET
Plan: Pro Plus
Run ID: e9546022-6755-4c7e-828b-e8b5e0bdef1a
📒 Files selected for processing (6)
rust/lance-core/src/error.rsrust/lance/src/dataset/mem_wal/manifest.rsrust/lance/src/dataset/mem_wal/memtable.rsrust/lance/src/dataset/mem_wal/test_util.rsrust/lance/src/dataset/mem_wal/wal.rsrust/lance/src/dataset/mem_wal/write.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
hamersaw
self-requested a review
July 22, 2026 00:59
Contributor
Author
|
Addressed the follow-up review and patch coverage report in
Validated with targeted unit tests, fence regressions, close/flush-drain regressions, Rustdoc, fmt, and |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/memtable.rs`:
- Around line 359-376: Update signal_memtable_flush_complete and
signal_typed_memtable_flush_complete to recover poisoned completion mutex guards
via PoisonError::into_inner() instead of unwrap, then always publish the
terminal result so drain watchers and close do not remain pending. Add a
regression test that creates the watcher before poisoning the mutex and verifies
flush completion is still signaled.
🪄 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: QUIET
Plan: Pro Plus
Run ID: 86649667-2083-4a7a-999e-6910bc47fd4c
📒 Files selected for processing (6)
rust/lance-core/src/error.rsrust/lance/src/dataset/mem_wal/manifest.rsrust/lance/src/dataset/mem_wal/memtable.rsrust/lance/src/dataset/mem_wal/test_util.rsrust/lance/src/dataset/mem_wal/wal.rsrust/lance/src/dataset/mem_wal/write.rs
Comment on lines
+359
to
376
| let cell = self.memtable_flush_completion.lock().unwrap().take(); | ||
| if let Some(cell) = cell { | ||
| cell.write(result); | ||
| } else { | ||
| debug_assert!(false, "memtable flush completion signaled more than once"); | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn signal_typed_memtable_flush_complete(&self, result: MemTableFlushResult) { | ||
| let cell = self.typed_memtable_flush_completion.lock().unwrap().take(); | ||
| if let Some(cell) = cell { | ||
| cell.write(result); | ||
| } else { | ||
| debug_assert!( | ||
| false, | ||
| "typed memtable flush completion signaled more than once" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Recover poisoned completion mutexes before signaling.
Both .lock().unwrap() calls panic on a poisoned mutex. Since MemTableFlushHandler::flush_memtable publishes these results before removing drain watchers, that panic leaves wait_for_flush_drain() and close() awaiting an unresolved watcher indefinitely. Recover the guard with PoisonError::into_inner() and publish a terminal result; add a watcher-created-before-poisoning regression test.
As per coding guidelines, “Never use .unwrap(), .expect(), panic!(), or assert!() in library code for fallible operations.”
🤖 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/memtable.rs` around lines 359 - 376, Update
signal_memtable_flush_complete and signal_typed_memtable_flush_complete to
recover poisoned completion mutex guards via PoisonError::into_inner() instead
of unwrap, then always publish the terminal result so drain watchers and close
do not remain pending. Add a regression test that creates the watcher before
poisoning the mutex and verifies flush completion is still signaled.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Error::is_mem_wal_fenced()for detecting peer epoch takeovers without parsing error textDurabilityResult::Failed(String)APIput, replay/open,force_seal_active,wait_for_flush_drain, andcloseTesting
cargo test -p lance-core --doc is_mem_wal_fencedcargo test -p lance-core fencecargo test -p lance fencecargo test -p lance dataset::mem_wal::manifest::tests::test_claim_epoch_lost_ack_is_not_peer_fence -- --exactcargo test -p lance dataset::mem_wal::write::tests::test_close_propagates_frozen_memtable_flush_failure -- --exactcargo test -p lance dataset::mem_wal::write::tests::test_frozen_retained_after_failed_flush -- --exactcargo fmt --all -- --checkcargo clippy --all --tests --benches -- -D warnings