fix(mem_wal): harden shard open and fail reads on a poisoned writer#7872
Conversation
…ndex `insert_batches_parallel` spawned one OS thread per index on every call via `std::thread::scope`. That is sized for flush-time batches; for a handful of rows the spawn costs more than the indexing itself. Merge it into `insert_batches`, which now picks the path by size: inline on the calling thread when there is a single index or the batch is at or below `PARALLEL_INDEX_MIN_ROWS` (64) rows, and threaded above it. Atomicity is unchanged — both paths run every task and keep only the first error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`check_poisoned()` guarded the three write paths but no read path, so a writer that had been fenced by a peer or had latched a persistence failure kept serving scans. Those scans can hand out rows that are not durable and that replay will not reproduce — a divergent snapshot from a shard that is already known to be dead. Recovery is evict and reopen; until then the shard should fail closed. Guard `scan()`, `active_memtable_ref()`, and `in_memory_memtable_refs()`, mirroring SlateDB's `check_closed()` at the top of every read. `memtable_stats()` is deliberately left unguarded: a poisoned writer is exactly when an operator — and the eviction path — most needs to read its state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hard open An in-memory index configured on a column that cannot support it fails deterministically on every insert — including inserts replayed from the WAL. So once a row is durable the shard can never reopen: replay re-reads the same rows, hits the same error, and `open()` propagates it. That is a permanently down shard, and it is the failure mode that makes poison-and-replay non-terminating. Validate once at open, before a single row is accepted: FTS columns must be Utf8/LargeUtf8/Utf8View, HNSW columns must be FixedSizeList<Float32> with a non-zero dimension, every index column must exist, and composite primary-key columns must have an order-preserving key encoding. BTree needs only existence — its backend falls through to per-row `ScalarValue` extraction and accepts any type the schema can hold. Also close two related gaps: - FTS silently appended an empty batch when its column was missing from the batch, so a misconfigured index stayed empty forever while the shard reported healthy. It now errors, matching BTree and HNSW. - HNSW's runtime capacity-exhaustion errors were labelled `invalid_input`, which blames the caller for a well-formed batch. They mean the index was sized below what the memtable holds, so they are `internal`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughMemWAL now validates index configurations during shard opening, consolidates inline and threaded index insertion, updates flush and replay callers, fences poisoned read paths, and classifies HNSW capacity exhaustion as internal errors. ChangesMemWAL index correctness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Writer
participant WalFlusher
participant IndexStore
participant MemWALIndexes
Writer->>Writer: validate_index_configs(schema, pk_columns)
Writer->>WalFlusher: open memtable mode
WalFlusher->>IndexStore: insert_batches(stored_batches)
IndexStore->>MemWALIndexes: run inline or threaded index tasks
MemWALIndexes-->>IndexStore: return index results and durations
IndexStore-->>WalFlusher: advance visibility watermark
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The public `insert_batches` doc used an intra-doc link to the private `PARALLEL_INDEX_MIN_ROWS`, which fails the rustdoc CI job under `RUSTDOCFLAGS="-D warnings"` (rustdoc::private-intra-doc-links). Keep it as plain code formatting instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance/src/dataset/mem_wal/hnsw/storage.rs-292-298 (1)
292-298: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude the exhausted batch index in the diagnostic.
The condition is
batch_idx >= self.max_batches, but the message reports onlymax_batches; failures at different batch positions become indistinguishable. Includebatch_idx(and, if useful, the current row range) in the error.As per coding guidelines, include full error context, including variable names, values, sizes, types, and indices.
🤖 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/hnsw/storage.rs` around lines 292 - 298, Update the exhaustion error in the batch-capacity check around committed_batches and max_batches to include the exhausted batch index batch_idx alongside max_batches. Preserve the existing error behavior while providing complete diagnostic context, including relevant variable names and values; include the current row range if it is already available in this scope.Source: Coding guidelines
🧹 Nitpick comments (3)
rust/lance/src/dataset/mem_wal/index.rs (3)
891-899: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
use std::time::Instant;to the top of the file.This
useis declared inline insideinsert_batches's body rather than at the top of the file.As per coding guidelines, "Place `use` imports at the top of the file, not inline within function bodies."♻️ Proposed fix
+use std::time::Instant; + ... pub fn insert_batches( &self, batches: &[StoredBatch], ) -> Result<std::collections::HashMap<String, std::time::Duration>> { - use std::time::Instant; - if batches.is_empty() {🤖 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/index.rs` around lines 891 - 899, Move the std::time::Instant import from inside insert_batches to the file-level use declarations, leaving insert_batches behavior unchanged and reusing the top-level import for its timing logic.Source: Coding guidelines
1646-1692: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for the HNSW
dim <= 0validation branch.
test_validate_index_configscovershnsw_not_a_vector,hnsw_wrong_item_type, andhnsw_missing_column, but there's no case for aFixedSizeListcolumn withdim == 0— the new branch at Lines 167-174 that this PR specifically added.As per coding guidelines, "Every bugfix and feature must have corresponding tests."✅ Proposed additional case
+ #[case::hnsw_zero_dim(MemIndexConfig::Hnsw(Box::new(HnswIndexConfig::new( + "idx".into(), 2, "zero_dim_vector".into(), DistanceType::L2, + ))), Some("requires a vector dimension > 0"))](with a corresponding zero-width
FixedSizeListcolumn added tovector_schema())🤖 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/index.rs` around lines 1646 - 1692, Add a zero-dimension FixedSizeList column to vector_schema and extend test_validate_index_configs with an HNSW case targeting that column, using DistanceType::L2 and asserting the validation error fragment for the dim <= 0 branch. Preserve the existing cases and test structure.Source: Coding guidelines
1758-1791: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for
insert_batches's error/atomicity path.The PR's stated goal is to "preserve atomicity across inline and threaded indexing paths," but this test only exercises the success path. Nothing verifies that when one index task fails,
insert_batchesreturnsErrand leaves the composite-PK update andmax_visible_batch_positionunadvanced (Lines 1004-1022). This is now easy to trigger deterministically: configure an FTS index and insert a batch missing that column —fts.rs'sinsert_batch(this same PR) now returnsError::invalid_inputfor that case instead of silently no-op'ing.🤖 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/index.rs` around lines 1758 - 1791, Add an error-path test alongside test_insert_batches_indexes_every_row_once that configures a composite-PK index and an FTS index, then calls insert_batches with a batch missing the FTS column. Assert it returns Err and verify the composite-PK state and max_visible_batch_position remain unchanged, using the deterministic fts.rs invalid-input 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.
Inline comments:
In `@rust/lance/src/dataset/mem_wal/hnsw/graph.rs`:
- Around line 597-604: The HNSW capacity-error paths lack regression coverage.
In rust/lance/src/dataset/mem_wal/hnsw/graph.rs:597-604, add a test asserting
graph-capacity exhaustion returns the exact internal error variant and includes
required length and graph capacity; in
rust/lance/src/dataset/mem_wal/hnsw/storage.rs:272-298, add tests for
row-capacity and max_batches exhaustion asserting the exact error variant and
diagnostic fields start, batch_len, and batch_idx.
---
Other comments:
In `@rust/lance/src/dataset/mem_wal/hnsw/storage.rs`:
- Around line 292-298: Update the exhaustion error in the batch-capacity check
around committed_batches and max_batches to include the exhausted batch index
batch_idx alongside max_batches. Preserve the existing error behavior while
providing complete diagnostic context, including relevant variable names and
values; include the current row range if it is already available in this scope.
---
Nitpick comments:
In `@rust/lance/src/dataset/mem_wal/index.rs`:
- Around line 891-899: Move the std::time::Instant import from inside
insert_batches to the file-level use declarations, leaving insert_batches
behavior unchanged and reusing the top-level import for its timing logic.
- Around line 1646-1692: Add a zero-dimension FixedSizeList column to
vector_schema and extend test_validate_index_configs with an HNSW case targeting
that column, using DistanceType::L2 and asserting the validation error fragment
for the dim <= 0 branch. Preserve the existing cases and test structure.
- Around line 1758-1791: Add an error-path test alongside
test_insert_batches_indexes_every_row_once that configures a composite-PK index
and an FTS index, then calls insert_batches with a batch missing the FTS column.
Assert it returns Err and verify the composite-PK state and
max_visible_batch_position remain unchanged, using the deterministic fts.rs
invalid-input behavior.
🪄 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: 14781107-fc17-47a2-b067-a823ab915b8f
📒 Files selected for processing (7)
rust/lance/benches/mem_wal/vector/mem_wal_index_micro.rsrust/lance/src/dataset/mem_wal/hnsw/graph.rsrust/lance/src/dataset/mem_wal/hnsw/storage.rsrust/lance/src/dataset/mem_wal/index.rsrust/lance/src/dataset/mem_wal/index/fts.rsrust/lance/src/dataset/mem_wal/wal.rsrust/lance/src/dataset/mem_wal/write.rs
| // Not caller input: the graph was sized below what the memtable holds. | ||
| // See the matching note in `storage.rs::append_batch`. | ||
| if needed_len > self.nodes.len() { | ||
| return Err(Error::invalid_input(format!( | ||
| "graph capacity {} exhausted: need {needed_len}", | ||
| return Err(Error::internal(format!( | ||
| "HNSW graph capacity {} exhausted: need {needed_len}; \ | ||
| the graph is sized below the memtable's row capacity", | ||
| self.nodes.len() | ||
| ))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add regression tests for HNSW capacity-error classification.
Both paths changed observable error variants and messages; cover each exhaustion mode and assert the exact error variant plus key diagnostic fields.
rust/lance/src/dataset/mem_wal/hnsw/graph.rs#L597-L604: test graph capacity exhaustion with required length and graph capacity in the message.rust/lance/src/dataset/mem_wal/hnsw/storage.rs#L272-L298: test row-capacity andmax_batchesexhaustion, includingstart,batch_len, andbatch_idx.
📍 Affects 2 files
rust/lance/src/dataset/mem_wal/hnsw/graph.rs#L597-L604(this comment)rust/lance/src/dataset/mem_wal/hnsw/storage.rs#L272-L298
🤖 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/hnsw/graph.rs` around lines 597 - 604, The
HNSW capacity-error paths lack regression coverage. In
rust/lance/src/dataset/mem_wal/hnsw/graph.rs:597-604, add a test asserting
graph-capacity exhaustion returns the exact internal error variant and includes
required length and graph capacity; in
rust/lance/src/dataset/mem_wal/hnsw/storage.rs:272-298, add tests for
row-capacity and max_batches exhaustion asserting the exact error variant and
diagnostic fields start, batch_len, and batch_idx.
Source: Coding guidelines
| // Exhaustion is not a caller-input problem — the batch is well-formed and | ||
| // has already passed the memtable's schema gate. It means the HNSW store | ||
| // was sized below what the memtable will hold before it flushes, which is | ||
| // a shard-construction bug. `invalid_input` mislabelled it as the writer's | ||
| // fault and hid that. |
There was a problem hiding this comment.
nitpick: this makes sense, but this comment is not really necessary. Though I understand it's hard to get the AIs from adding this kind of comment.
There was a problem hiding this comment.
Agreed, I actually have a prompt I usually saying "dude, remove some comments, this is outrageous" (paraphrasing of course) ... missed it on this one.
There was a problem hiding this comment.
I need to add that to my /polish-pr skill...
| ))); | ||
| } | ||
| } | ||
| MemIndexConfig::Hnsw(_) => match field.data_type() { |
There was a problem hiding this comment.
question: do we only support HNSW and not IVF_PQ/IVF_RQ?
There was a problem hiding this comment.
So on the active memtable (completely in-memory we are currently buidling HSNW regardless of which type is defined. It's my understanding, and certainly could be wrong, that this is the best fit for in-memory, small, frequent writes. Whereas it's kind of an anti-pattern for IVF_PQ/IVF_RQ. On flushed generations and the base Lance table we will still maintain the configured IVF_PQ/IVF_RQ index.
| let handles: Vec<_> = tasks | ||
| .iter() | ||
| .map(|(name, task)| { | ||
| let handle = scope.spawn(move || { |
There was a problem hiding this comment.
note: in a multi-tenant system it might not be desire-able to spawn this many threads for each write. Might want to eventually have some shared threadpool and ability to constraint the max amount of threads a given table mem wal can take up.
There was a problem hiding this comment.
100% agreed. I have a ticket on updating this to use Rayon (already imported elsewhere) so we're not spawning a ton of OS threads. There is some configuration on a record count threshold where we choose to either spawn threads or do it inline. The hope is that >95% of inserts are done inline without spawning threads.
Address PR review: report batch_idx and the row range in the HNSW max_batches exhaustion error to match the sibling row-capacity error, trim the over-long exhaustion rationale comment, and move the inline `use std::time::Instant` to the file-level imports. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance/src/dataset/mem_wal/hnsw/storage.rs-292-295 (1)
292-295: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
batch_idxin the total-capacity diagnostic.The
max_batcheserror now reportsbatch_idx, but the precedingend > self.capacitypath still omits it. Loadbatch_idxbefore both checks and include it in the total-capacity message as well, so every capacity-exhaustion error contains the batch index and inserted row range.Proposed fix
let start = self.committed_len.load(Ordering::Relaxed); +let batch_idx = self.committed_batches.load(Ordering::Relaxed); if end > self.capacity { return Err(Error::internal(format!( - "HNSW vector store capacity {} exhausted: inserting rows [{}..{}); \ + "HNSW vector store capacity {} exhausted at batch_idx {}: \ + inserting rows [{}..{}); \ the store is sized below the memtable's row capacity", - self.capacity, start, end + self.capacity, batch_idx, start, end ))); } -let batch_idx = self.committed_batches.load(Ordering::Relaxed);🤖 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/hnsw/storage.rs` around lines 292 - 295, Update the capacity-exhaustion handling in the HNSW vector store insertion flow to load batch_idx before both the end > self.capacity and max_batches checks. Include batch_idx and the inserted row range in the total-capacity diagnostic, while preserving the existing max_batches message and behavior.
🤖 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.
Other comments:
In `@rust/lance/src/dataset/mem_wal/hnsw/storage.rs`:
- Around line 292-295: Update the capacity-exhaustion handling in the HNSW
vector store insertion flow to load batch_idx before both the end >
self.capacity and max_batches checks. Include batch_idx and the inserted row
range in the total-capacity diagnostic, while preserving the existing
max_batches message and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: fbd23ee0-689d-4c26-90b9-8765638403fc
📒 Files selected for processing (2)
rust/lance/src/dataset/mem_wal/hnsw/storage.rsrust/lance/src/dataset/mem_wal/index.rs
…ity cursors (#7888) ## WAL read-visibility redesign The core of the former #7791. Rows become visible to readers only once they are **both** indexed **and** WAL-durable, resolved through two independent cursors instead of a single conflated watermark. Index application runs on its own task rather than as an arm of the WAL flush, giving read-your-writes in every mode. Commits (bottom → top): - **mark replayed batches durable so the next flush does not re-append** — replayed rows are already WAL-durable; stamp the cursor so the first post-reopen flush doesn't re-append or re-index them. - **delete dead WAL-tracking state from MemTable** - **make the WAL durability cursor writer-global** / **make the visibility cursor an exclusive count** — the two-cursor model: an `indexed` cursor and a writer-global durability cursor; readers snapshot `visible_count` derived from both. - **publish rows only once they are indexed and durable** - **run the index apply on its own task, not as an arm of the WAL flush** - **rotate memtables during replay instead of overflowing** - **validate single-column PKs and restore index-apply stats** - **keep the latched failure across a poisoned terminal_error lock** - **harden shard-open validation and freeze failure handling** — move PK/index/interval validation before `claim_epoch` (a doomed open must not fence the incumbent); reject a config whose `field_id` names a different column than its `column`; retain the outgoing memtable and poison on a failed freeze dispatch. - **reject a batch position past committed_len instead of skipping it** **Stack** (the former #7791, split for reviewability) 1. #7872 — foundational hardening → **merged** 2. **this PR** — read-visibility redesign → `main` 3. flush-interval ticker → follows this PR 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## WAL flush-interval ticker The feature the original #7791 was titled for, now small because it sits on the read-visibility redesign (#7888) that landed ahead of it. `flush_interval_ms` was inert: it routed to a timer only ever evaluated on the write path, so it could add a redundant trigger but never delay or batch one. - **append the WAL on a background ticker, resolved by cursor** — give the WAL flusher a real ticker (`MessageHandler::tickers`) and take the append off the put path. The append is the only thing on that schedule — it's an S3 PUT, billed per call, and bounding that cost is the whole reason a flush interval exists. The index apply stays per-put on its own task. `WalFlushSource::NextPending` is resolved when the message is handled, by walking live stores oldest-first and taking the first that still owes an append. - **remove the inert `sync_indexed_write` config family** — `sync_indexed_write`, `async_index_buffer_rows`, and `async_index_interval` had no behavioral consumers after the index-apply-task split (#7888) made every write read-your-writes. Removed across Rust core, benches, and Python/Java bindings. **Stack** — this is the last of three; the first two have merged into `main`: 1. #7872 — foundational shard-open hardening ✅ merged 2. #7888 — read-your-writes / two-cursor visibility redesign ✅ merged 3. **this PR** — flush-interval ticker → `main` Closes the original monolithic #7791. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Stack: 1/3 — foundational hardening
This is the first of three stacked PRs that split the former #7791 (WAL poison / read-visibility / flush-interval) into reviewable pieces. It carries the changes that stand on their own, independent of the visibility redesign:
open(), before any row is accepted. A config that fails deterministically on every insert (including WAL replay) makes poison-and-replay non-terminating; reject it up front. Also errors (instead of silently indexing nothing) when an FTS column is missing from a batch, and relabels HNSW capacity-exhaustion asinternalrather thaninvalid_input.Reviewable independently of the two PRs stacked on top.
Stack
mainSplits the former #7791 into three reviewable pieces; #7791 can be closed once this stack lands.
🤖 Generated with Claude Code