fix: verify ambiguous commit ownership by identity and intent - #8137
fix: verify ambiguous commit ownership by identity and intent#8137Xuanwo wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
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.
CreateIndexmetadata 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