Skip to content

fix: verify ambiguous commit ownership by identity and intent - #8137

Open
Xuanwo wants to merge 4 commits into
mainfrom
xuanwo/commit-protocol-blackbox
Open

fix: verify ambiguous commit ownership by identity and intent#8137
Xuanwo wants to merge 4 commits into
mainfrom
xuanwo/commit-protocol-blackbox

Conversation

@Xuanwo

@Xuanwo Xuanwo commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Conditional manifest writes may land durably while the caller receives a transient error. A retry then observes a conflict, and treating that conflict as a definite failure can let cleanup delete fragments still referenced by the live manifest.

Commit ownership previously depended on decoded transaction equality. CreateIndex metadata is normalized by its protobuf round trip, which can make the same commit appear foreign; identity-only matching would instead allow a reused UUID to claim a different intent. This requires a non-empty transaction UUID together with canonical persisted intent before classifying a landed commit as ours, while retaining artifacts when the outcome remains unknown.

The accompanying black-box conformance harness exercises conditional object-store and DynamoDB commits under lost responses, unavailable verification reads, foreign writers, recovery, process exit, and GC. It validates the complete visible history and the correspondence between successful results, persisted transactions, and effects instead of trusting handler return values, and retains replayable fault traces for diagnosis.

Closes #8123

@github-actions github-actions Bot added A-python Python bindings A-deps Dependency updates A-ci CI / build workflows bug Something isn't working labels Aug 1, 2026
@codecov

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.94595% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance/src/io/commit.rs 95.94% 0 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

@Xuanwo
Xuanwo marked this pull request as ready for review August 1, 2026 16:47

@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: request changes. The ownership proof must bind an attempt identity to every manifest-affecting part of the intended commit. Prefer a lossless persisted intent, or a canonical digest over that full intent, so representation normalization cannot let a reused identity report a different durable result as success.

fn is_same_commit_attempt(committed: &Transaction, attempted: &Transaction) -> bool {
!attempted.uuid.is_empty()
&& committed.uuid == attempted.uuid
&& pb::Transaction::from(committed) == pb::Transaction::from(attempted)

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.

pb::Transaction equality omits manifest-affecting intent, so a reused UUID can make a foreign manifest look like this attempt and return success for the wrong durable result. Overwrite and Merge drop schema metadata in their protobuf conversions, although build_manifest uses the full schema; Rewrite.frag_reuse_index is also omitted while it changes the index section. Persist all such fields or compare a canonical digest of the complete manifest-affecting intent.

Reproducer run against this head

Added this regression in the existing test module:

#[test]
fn reproducer_reused_uuid_must_include_schema_metadata_intent() {
    use crate::dataset::transaction::TransactionBuilder;

    let mut committed_schema = Schema::default();
    committed_schema.metadata.insert("owner".into(), "committed".into());
    let mut attempted_schema = committed_schema.clone();
    attempted_schema.metadata.insert("owner".into(), "attempted".into());

    let make = |schema| TransactionBuilder::new(
        7,
        Operation::Overwrite {
            fragments: vec![],
            schema,
            config_upsert_values: None,
            initial_bases: None,
        },
    ).uuid("shared-attempt-id".into()).build();

    assert!(!is_same_commit_attempt(
        &make(committed_schema),
        &make(attempted_schema),
    ));
}

cargo test -p lance io::commit::tests::reproducer_reused_uuid_must_include_schema_metadata_intent -- --exact

Observed: the assertion failed because is_same_commit_attempt returned true.

@Xuanwo
Xuanwo requested a review from lance-community August 1, 2026 18:52
@lance-gatekeeper
lance-gatekeeper Bot removed the request for review from lance-community August 1, 2026 18:52
@Xuanwo Xuanwo closed this Aug 3, 2026
@Xuanwo Xuanwo reopened this Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ci CI / build workflows A-deps Dependency updates A-python Python bindings bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

commit_compaction can delete live data after an ambiguously successful manifest PUT

1 participant