Skip to content

fix: persist frag reuse index external file on local filesystem#6163

Merged
wjones127 merged 2 commits intolance-format:mainfrom
wjones127:fix/frag-reuse-external-file-persist
Mar 11, 2026
Merged

fix: persist frag reuse index external file on local filesystem#6163
wjones127 merged 2 commits intolance-format:mainfrom
wjones127:fix/frag-reuse-external-file-persist

Conversation

@wjones127
Copy link
Copy Markdown
Contributor

Previously, when FragReuseIndexDetails exceeded 204800 bytes (triggered by large compactions with many fragments), the code wrote the details to an external file (details.binpb). On local filesystems, ObjectStore::create returns a LocalWriter that atomically renames a temp file to the final path in Writer::shutdown. However, frag_reuse.rs imported tokio::io::AsyncWriteExt but not lance_io::traits::Writer, so writer.shutdown() resolved to AsyncWriteExt::shutdown (flush/close only) — the temp file was deleted on drop without being persisted. Any subsequent load_indices call would fail with Not found: .../details.binpb.

Fixed by using UFCS Writer::shutdown(writer.as_mut()).await? to explicitly call the lance trait method, matching the existing pattern in ivf.rs and blob.rs.

Fixes #6161

When `FragReuseIndexDetails` exceeds 204800 bytes, it is written to an
external file (`details.binpb`). On local object stores, `ObjectStore::create`
returns a `LocalWriter` that writes to a temp file and calls `persist()` in
`Writer::shutdown` to rename it to the final path. The code imported
`tokio::io::AsyncWriteExt` but not `lance_io::traits::Writer`, so
`writer.shutdown()` resolved to `AsyncWriteExt::shutdown` (flush/close only),
and the temp file was silently deleted on drop. Use UFCS to call
`Writer::shutdown` explicitly, matching the existing pattern in `ivf.rs`.

Fixes lance-format#6161

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added the bug Something isn't working label Mar 11, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Review

Clean, well-targeted bugfix. The root cause analysis is spot-on: AsyncWriteExt::shutdown (flush/close) was being resolved instead of lance_io::traits::Writer::shutdown (which persists the temp file via atomic rename on local FS). The UFCS fix Writer::shutdown(writer.as_mut()).await? matches the established pattern used throughout the codebase (ivf.rs, blob.rs, writer.rs, etc.).

The regression test is well-constructed — it creates enough fragments to exceed the 204800-byte inline threshold and verifies the external file round-trips correctly.

No issues found. LGTM.

@wjones127 wjones127 marked this pull request as ready for review March 11, 2026 00:16
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 11, 2026

Codecov Report

❌ Patch coverage is 83.76068% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-io/src/object_writer.rs 73.61% 16 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you!

@westonpace
Copy link
Copy Markdown
Member

Yikes, I wonder if there is any way we can rework our code to avoid this? Maybe LocalWriter can have a customer Drop impl which logs a warning if shutdown isn't called? Though maybe there are legitimate reasons to drop a writer before we finish writing 😦

@westonpace
Copy link
Copy Markdown
Member

Or maybe we can put the rename inside of poll_shutdown? That actually seems like a more natural fit.

@wjones127
Copy link
Copy Markdown
Contributor Author

Or maybe we can put the rename inside of poll_shutdown? That actually seems like a more natural fit.

@westonpace You are right, we can remove this footgun. I've just pushed a fix that does that.

@wjones127 wjones127 merged commit be5f439 into lance-format:main Mar 11, 2026
28 checks passed
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.

Index file not found during FTS prewarm

3 participants