Skip to content

fix(rollout): stop swallowing failures in RolloutStore's detached close - #190

Merged
beinan merged 1 commit into
mainfrom
fix/rollout-drop-close-observability
Jul 25, 2026
Merged

fix(rollout): stop swallowing failures in RolloutStore's detached close#190
beinan merged 1 commit into
mainfrom
fix/rollout-drop-close-observability

Conversation

@beinan

@beinan beinan commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Closes #185 — though not in the way the issue described. See below.

The reported bug does not exist

#185 claimed the flush sweeper's LRU-only coverage could leave an evicted store's rows permanently invisible. That is wrong, and I should have verified before filing it.

ShardWriter::close() explicitly freezes the active memtable and waits for the flush (lance-7.0.0 dataset/mem_wal/write.rs:1911-1921), and RolloutStore::Drop spawns a detached close. So dropping an evicted handle does seal its rows. Confirmed by probe and now by a test in this PR:

dropping_a_store_seals_its_unflushed_rows ... ok

The real (smaller) problem

That safety net is entirely silent:

if let Ok(writer) = Arc::try_unwrap(writer) {
    let _ = writer.close().await;
}

Three distinct ways it can fail to seal, none observable:

  1. close() returns Err — discarded by let _ =
  2. Arc::try_unwrap fails because an in-flight append still shares the writer — the if let Ok silently skips
  3. No Tokio runtime is available — the outer if let Ok(handle) silently skips, nothing is spawned

In each case rows stay durable in the WAL but invisible until the next restart replays it. Debugging that from the outside means staring at a store that accepted writes and returns no rows, with nothing in the logs.

Change

Log all three (warn / debug / warn respectively). No behavior change — best-effort stays best-effort.

Also documents the eviction guarantee on AppState::rollout_stores: the sweeper's resident-only scan looks like a gap on inspection, and the reason it isn't was not written down.

Testing

New dropping_a_store_seals_its_unflushed_rows adds a row, drops the handle without flush or close, and polls a fresh reader until the row appears. This is the invariant #185 doubted; now it is enforced.

cargo test -p lance-context-core --lib → 162 passed, 0 failed. fmt + clippy clean.

🤖 Generated with Claude Code

`RolloutStore::Drop` spawns a detached `ShardWriter::close` and discards its
result with `let _ =`. Because `close` seals the active memtable, this path
is what keeps an LRU-evicted store's unflushed rows from being stranded — so
every way it can fail to run is a case where rows stay durable in the WAL but
invisible to reads until the next process restart, with no signal anywhere.

Log all three: a failing close, a writer still shared by an in-flight append
(so the close no-ops), and a drop with no Tokio runtime (so nothing is
spawned at all). Behavior is unchanged — this is purely observability on a
path that was silent by construction.

Also documents the eviction guarantee on `AppState::rollout_stores`, since
the flush sweeper only visits resident stores and the reason eviction is
nonetheless safe was not written down anywhere.

Adds a test asserting end to end that dropping a store with an unsealed
memtable makes its rows visible, so the eviction path cannot regress.

Closes #185

Co-Authored-By: Claude <noreply@anthropic.com>
@beinan
beinan force-pushed the fix/rollout-drop-close-observability branch from 420dea3 to 59f8f1a Compare July 25, 2026 07:20
@beinan
beinan merged commit 76d34d5 into main Jul 25, 2026
9 checks passed
@beinan
beinan deleted the fix/rollout-drop-close-observability branch July 25, 2026 07:30
beinan added a commit that referenced this pull request Jul 26, 2026
…erate drained generations (#200)

Three fixes the rollout store received but `DatagenStore` never did.
Each is latent today and becomes reachable as soon as datagen sees
concurrent or long-lived use.

## 1. Merge used the compile-time schema, not the live one

`merge_own_shard` built its append batch from `datagen_log_schema()`. A
base table written by an older binary can lack columns the current
schema has, and merging a batch built from the compile-time schema into
it **fails outright**. Now aligned via `align_batch_to_schema` — the
rollout store hit exactly this and was fixed in #175.

## 2. `Drop` swallowed the writer's close error

```rust
let _ = writer.close().await;   // before
```

A failed close leaks the writer's background tasks and, if the memtable
was still buffered, **strands rows that are durable in the WAL but never
sealed** — with no signal anywhere. Now logged, and the no-runtime path
says so too. (Rollout equivalent: #190.)

## 3. A concurrently-drained generation failed the whole lookup

`get_blob` propagated not-found when a merge drained and deleted a
generation between snapshot and open. Those rows are already in the base
table, so it now skips and falls through — as the rollout store does.

## Sharing

`align_batch_to_schema` and `is_not_found_error` become `pub(crate)` so
both stores share one implementation instead of drifting again. This
drift is the actual pattern here: all three bugs are cases where rollout
was fixed and datagen was not.

## Scope note — worth reading

I expected datagen to have the write-lock stall fixed in #199. **It does
not**, and I verified rather than assumed:

- `DatagenStore::append` is already `&mut self` (its writer is a bare
`Option<ShardWriter>`, not behind a mutex), so appends need the
exclusive lock **regardless of merge** — the read/write lock split from
#199 would buy nothing.
- `write_with_resident_writer` already retries on fence, so
`claim_epoch` fencing its own writer is handled. Restoring `claim_epoch`
leaves the new test **green**, confirming this empirically.
- Datagen is not yet wired into any server route or sweeper.

So the epoch change from #199 is deliberately **not** ported. These
three divergences are the real defects.

## Testing

New test asserts a merge does not fence the store's own resident writer,
so an append immediately after a merge still succeeds and both rows stay
readable, and a second merge still converges.

9 datagen tests pass; `clippy --all-targets -D warnings` and `cargo fmt`
clean.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(rollout): flush sweeper only covers LRU-resident stores; evicted stores may never flush

1 participant