Skip to content

fix: make FTS metadata loading retry-safe#7838

Draft
Xuanwo wants to merge 2 commits into
mainfrom
agent/fts-index-snapshot-support
Draft

fix: make FTS metadata loading retry-safe#7838
Xuanwo wants to merge 2 commits into
mainfrom
agent/fts-index-snapshot-support

Conversation

@Xuanwo

@Xuanwo Xuanwo commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Distributed FTS global-stats planning needs one immutable set of index segments and parsed tokenizer parameters per table snapshot. Concurrent cold callers currently race while reading index metadata, and InvertedIndex::load_params treats every metadata.lance open failure as a legacy index. A transient object-store error can therefore become a successful default-tokenizer fallback and be retained by an upper-layer cache.

Make index-metadata cache misses use the existing fallible singleflight path and include the object-store identity in the cache key. Restrict the legacy tokens fallback to typed not-found errors, preserving that variant through shared-reader error cloning so timeouts and permission failures remain retryable errors.

This is the Lance-side prerequisite for the Sophon QN-owned FTS index snapshot cache in lancedb/sophon#6759.

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 75f06ffa-bd0a-4a1d-b7a2-b75d4ac4f575

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch agent/fts-index-snapshot-support

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added A-index Vector index, linalg, tokenizer bug Something isn't working labels Jul 18, 2026
@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.87755% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-core/src/error.rs 86.66% 0 Missing and 4 partials ⚠️
rust/lance-index/src/scalar/inverted/index.rs 75.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Ok(serde_json::from_str::<InvertedIndexParams>(params)?)
}
Err(_) => {
Err(error) if error.is_not_found() => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Legacy FTS indexes can still fail to open on S3 readers that have s3:GetObject but not s3:ListBucket. These indexes do not contain metadata.lance, and S3 returns 403 rather than 404 for a missing object when ListBucket is unavailable, so is_not_found() is false even though tokens.lance is readable. Could we probe the legacy tokens file when opening metadata fails, use the fallback only if that succeeds, and otherwise preserve the original metadata error? https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html

Comment thread rust/lance/src/index.rs
None => {
let mut indices = self
.index_cache
.get_or_insert_with_key(metadata_key, || async {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

get_or_insert_with_key does not preserve loader errors for coalesced waiters. Moka runs one initializer, but each caller owns a separate oneshot; when the loader fails, only the initializer receives the original timeout or permission error and the other callers return Internal("Failed to retrieve error from cache loader"). A five-caller repro produced one Timeout and four Internal errors. Could we fan out the same fallible result and add a concurrent failure test before using this path here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-index Vector index, linalg, tokenizer bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants