Skip to content

fix: avoid panic when rebasing a commit whose read version was cleaned up - #8935

Merged
zhangyue19921010 merged 1 commit into
lance-format:mainfrom
qiuyuhang:fix/rebase-checkout-version-gc
Sep 2, 2026
Merged

fix: avoid panic when rebasing a commit whose read version was cleaned up#8935
zhangyue19921010 merged 1 commit into
lance-format:mainfrom
qiuyuhang:fix/rebase-checkout-version-gc

Conversation

@qiuyuhang

Copy link
Copy Markdown
Contributor

Problem

When two transactions commit concurrently and one of them needs to be rebased,
initial_fragments_for_rebase calls checkout_version(transaction.read_version)
and .unwrap()s the result:

dataset
    .checkout_version(transaction.read_version)
    .await
    .unwrap(),

If a concurrent cleanup_old_versions (e.g. triggered by VACUUM) removes that
version's manifest between the conflicting commit and the rebase, checkout_version
returns DatasetNotFound and the unwrap() panics. In builds compiled with
panic = "abort" (common when Lance is embedded via FFI) this aborts the entire
host process instead of failing just the commit.

Observed in production as:

thread '<unnamed>' panicked at .../lance/src/io/commit/conflict_resolver.rs:
called `Result::unwrap()` on an `Err` value: DatasetNotFound {
    path: ".../_versions/85.manifest", ...
}

Fix

Return a Result from initial_fragments_for_rebase and propagate the error
instead of unwrapping. A commit whose read version has been garbage-collected now
fails gracefully with DatasetNotFound, allowing the caller to retry, rather than
panicking. All 5 call sites are updated to use ?.

This is a behavior change only for the previously-panicking path; the success path
is unchanged.

Test

Added test_rebase_errors_when_read_version_was_cleaned_up, which:

  1. writes two versions of a dataset,
  2. builds a transaction pinned to version 1,
  3. deletes version 1's manifest to simulate concurrent cleanup,
  4. asserts TransactionRebase::try_new returns DatasetNotFound instead of panicking.

Verified the test fails (panics) without the fix and passes with it.

Verification

  • cargo check -p lance — clean
  • cargo test -p lance --lib io::commit::conflict_resolver — 59 passed (58 existing + 1 new)

…d up

initial_fragments_for_rebase unwrapped checkout_version(read_version). If a concurrent cleanup_old_versions removed that version between the conflicting commit and the rebase, the unwrap panicked on DatasetNotFound, aborting the whole process when built with panic="abort". Propagate the error instead so the commit fails gracefully and can be retried. Adds a regression test that deletes the read version's manifest and asserts the rebase returns DatasetNotFound instead of panicking.
@github-actions github-actions Bot added the bug Something isn't working label Sep 2, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

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.

Gate recommendation: approve.

The rebase now propagates the existing DatasetNotFound contract from the authoritative historical checkout instead of panicking, with no durable write performed before the failure. The regression coverage exercises the cleaned-up-manifest path while preserving the surrounding conflict-resolution behavior.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 2, 2026

@zhangyue19921010 zhangyue19921010 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

@zhangyue19921010
zhangyue19921010 merged commit d57d0fb into lance-format:main Sep 2, 2026
37 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 K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants