diff --git a/AGENTS.md b/AGENTS.md index a841831d..69978ed7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -253,7 +253,7 @@ omnigraph policy explain --cluster ./company-brain --graph knowledge --actor act | Columnar storage on object store | ✅ Arrow/Lance | URI normalization, S3 env-var plumbing | | Per-dataset versioning + time travel | ✅ | `snapshot_at_version`, `entity_at`, snapshot-pinned reads across many tables | | Per-dataset branches | ✅ | **Graph-level** refs are logically atomic through authoritative `__manifest` `BranchContents`; native create/delete crash gaps are classified and reclaimed under a single-writer-process boundary; live names are path-prefix-disjoint; data-table forks are lazy; system branches are filtered | -| Atomic single-dataset commits | ✅ | **Multi-table publish via three layers**, NOT a single Lance primitive: (1) per-table Lance `commit_staged` for the data write, (2) `__manifest` row-level CAS via `ManifestBatchPublisher` for cross-table ordering, (3) the open-time recovery sweep for the residual gap between (1) and (2). All three layers ship; the five migrated writers (`MutationStaging::commit_all`, `schema_apply`, `branch_merge`, `ensure_indices`, `optimize_all_tables`) write a `__recovery/{ulid}.json` sidecar before Phase B and delete it after Phase C. The next `Omnigraph::open` (gated on `OpenMode::ReadWrite`) runs the sweep in `db/manifest/recovery.rs`: classify, decide all-or-nothing per sidecar, roll forward via single `ManifestBatchPublisher::publish` or roll back via `Dataset::restore` followed by a manifest publish of the restored version (so both directions converge to `manifest == HEAD` — no residual drift), and record an internal audit row in `_graph_commit_recoveries.lance`. A v3 roll-forward preserves the interrupted writer's fixed commit lineage and actor; rollback and legacy recovery commits use `omnigraph:recovery`. There is currently no public CLI query for the recovery-audit table, and ordinary commit history is not a complete recovery enumeration. The write entry points (`load_as`, `mutate_as`, `apply_schema_as`, `branch_merge_as`) and `refresh` additionally run an in-process roll-forward-only heal, serialized against same-process live writers through the shared root-scoped gate manager, so a long-lived server converges on its next write without restart; only rollback-eligible sidecars still defer to the next read-write open (a future background reconciler's goal). Engine writes route through a sealed `TableStorage` trait (`db.storage()`) exposing only `stage_*` + `commit_staged` + reads; the sole inline-commit residual (`create_vector_index`) is split onto a separate sealed `InlineCommitResidual` trait reached via `db.storage_inline_residual()` (MR-854), so the default surface cannot couple a write with a HEAD advance — §1 holds by construction. `delete` migrated to the staged path in MR-A (`stage_delete` via Lance 7.0 `DeleteBuilder::execute_uncommitted`, [#6658](https://github.com/lance-format/lance/issues/6658)); `create_vector_index` stays inline until upstream Lance ships a public two-phase API ([#6666](https://github.com/lance-format/lance/issues/6666)); `LoadMode::Overwrite` uses Lance `Overwrite` staged transactions. | +| Atomic single-dataset commits | ✅ | **Multi-table publish via three layers**, NOT a single Lance primitive: (1) per-table Lance `commit_staged` for the data write, (2) `__manifest` row-level CAS via `ManifestBatchPublisher` for cross-table ordering, (3) the open-time recovery sweep for the residual gap between (1) and (2). All three layers ship; the five migrated writers (`MutationStaging::commit_all`, `schema_apply`, `branch_merge`, `ensure_indices`, `optimize_all_tables`) write a `__recovery/{ulid}.json` sidecar before Phase B and delete it after Phase C. The next `Omnigraph::open` (gated on `OpenMode::ReadWrite`) runs the sweep in `db/manifest/recovery.rs`: classify, decide all-or-nothing per sidecar, roll forward via single `ManifestBatchPublisher::publish` or roll back via `Dataset::restore` followed by a manifest publish of the restored version (so both directions converge to `manifest == HEAD` — no residual drift), and record an internal audit row in `_graph_commit_recoveries.lance`. Schema-v3 Mutation/Load and schema-v4 BranchMerge roll-forward preserve the interrupted writer's fixed commit lineage and actor; rollback and legacy recovery commits use `omnigraph:recovery`. BranchMerge v4 also owns first-touch refs, pre-mints each table's exact ordered data-transaction chain with zero transparent conflict retries, carries pointer-only updates in its complete confirmed manifest delta, and recognizes an interrupted compensation restore on restart. There is currently no public CLI query for the recovery-audit table, and ordinary commit history is not a complete recovery enumeration. The write entry points (`load_as`, `mutate_as`, `apply_schema_as`, `branch_merge_as`) and `refresh` additionally run an in-process roll-forward-only heal, serialized against same-process live writers through the shared root-scoped gate manager, so a long-lived server converges on its next write without restart; only rollback-eligible sidecars still defer to the next read-write open (a future background reconciler's goal). Engine writes route through a sealed `TableStorage` trait (`db.storage()`) exposing only `stage_*` + `commit_staged` + reads; the sole inline-commit residual (`create_vector_index`) is split onto a separate sealed `InlineCommitResidual` trait reached via `db.storage_inline_residual()` (MR-854), so the default surface cannot couple a write with a HEAD advance — §1 holds by construction. `delete` migrated to the staged path in MR-A (`stage_delete` via Lance 7.0 `DeleteBuilder::execute_uncommitted`, [#6658](https://github.com/lance-format/lance/issues/6658)); `create_vector_index` stays inline until upstream Lance ships a public two-phase API ([#6666](https://github.com/lance-format/lance/issues/6666)); `LoadMode::Overwrite` uses Lance `Overwrite` staged transactions. | | Compaction (`compact_files`) + reindex (`optimize_indices`) | ✅ | `omnigraph optimize` orchestrates over all node/edge tables, bounded concurrency; per table runs `compact_files` **then Lance `optimize_indices`** (folds appended/rewritten fragments back into existing indexes — incremental merge, not retrain) and **publishes the resulting version to `__manifest`** (so the manifest tracks the Lance HEAD — required for reads to observe the work and for schema apply / strict writes to pass their HEAD-vs-manifest precondition), under the per-`(table, main)` write queue with `SidecarKind::Optimize` recovery coverage spanning both ops; **commits even with no compaction work if index coverage is stale**; **refuses on an unrecovered graph**; **skips uncovered HEAD > manifest drift** with `DriftNeedsRepair`; **compacts blob-bearing tables** (the pre-9 `LANCE_SUPPORTS_BLOB_COMPACTION` skip was removed once Lance 8.0.0+ shipped blob-v2 compaction — see [docs/dev/invariants.md](docs/dev/invariants.md) Known Gaps) | | Repair uncovered drift | — | `omnigraph repair` explicitly classifies uncovered table `HEAD > manifest` drift: verified maintenance drift (`ReserveFragments`/`Rewrite`) can be published with `--confirm`; suspicious or unverifiable drift requires `--force --confirm`. Sidecar-covered crash residuals still recover automatically on open. | | Cleanup (`cleanup_old_versions`) | ✅ | `omnigraph cleanup` derives requested `--keep` / `--older-than` cutoffs from each table's available versions; Lance refs plus OmniGraph's live-lazy-branch and recovery floors may retain additional versions. It fails closed on unopenable pins, recovery intent, or uncovered main-table HEAD drift | diff --git a/crates/omnigraph/src/db/commit_graph.rs b/crates/omnigraph/src/db/commit_graph.rs index 8a6814f1..7791863f 100644 --- a/crates/omnigraph/src/db/commit_graph.rs +++ b/crates/omnigraph/src/db/commit_graph.rs @@ -134,6 +134,45 @@ impl CommitGraph { None => return Ok(None), }; + Self::merge_base_from_open_graphs( + source, + target, + &source_head.graph_commit_id, + &target_head.graph_commit_id, + ) + .await + } + + /// Compute a merge base for two already-captured graph commits rather than + /// re-reading each branch's live head. Branch merge uses this after it has + /// captured immutable source/target authority: a later source advance must + /// not silently substitute a newer source commit, and a later target + /// advance is rejected by the target OCC token instead of changing the + /// classifier input. + pub(crate) async fn merge_base_between( + root_uri: &str, + source_branch: Option<&str>, + target_branch: Option<&str>, + source_commit_id: &str, + target_commit_id: &str, + ) -> Result> { + let source = open_for_branch(root_uri, source_branch).await?; + let target = open_for_branch(root_uri, target_branch).await?; + Self::merge_base_from_open_graphs( + source, + target, + source_commit_id, + target_commit_id, + ) + .await + } + + async fn merge_base_from_open_graphs( + source: Self, + target: Self, + source_commit_id: &str, + target_commit_id: &str, + ) -> Result> { let mut commits = HashMap::new(); for commit in source.load_commits().await? { commits.insert(commit.graph_commit_id.clone(), commit); @@ -142,8 +181,12 @@ impl CommitGraph { commits.insert(commit.graph_commit_id.clone(), commit); } - let source_distances = ancestor_distances(&source_head.graph_commit_id, &commits); - let target_distances = ancestor_distances(&target_head.graph_commit_id, &commits); + if !commits.contains_key(source_commit_id) || !commits.contains_key(target_commit_id) { + return Ok(None); + } + + let source_distances = ancestor_distances(source_commit_id, &commits); + let target_distances = ancestor_distances(target_commit_id, &commits); let best = source_distances .iter() diff --git a/crates/omnigraph/src/db/graph_coordinator.rs b/crates/omnigraph/src/db/graph_coordinator.rs index 5dbb1cbc..26772de5 100644 --- a/crates/omnigraph/src/db/graph_coordinator.rs +++ b/crates/omnigraph/src/db/graph_coordinator.rs @@ -447,30 +447,6 @@ impl GraphCoordinator { }) } - /// Publish a branch-merge: `updates` (the merged table versions) plus the - /// merge commit, in one manifest CAS (RFC-013 Phase 7). The merge commit's - /// merged-in parent is `merged_parent_commit_id` (the source head, stable); - /// its first parent is resolved by the publisher as the current target-branch - /// head — the live head, which is the post-merge correct parent even if the - /// target advanced since the merge began. - pub(crate) async fn commit_merge_with_actor( - &mut self, - updates: &[SubTableUpdate], - merged_parent_commit_id: &str, - actor_id: Option<&str>, - ) -> Result { - let intent = - self.new_lineage_intent(actor_id, Some(merged_parent_commit_id.to_string()))?; - failpoints::maybe_fail(crate::failpoints::names::GRAPH_PUBLISH_BEFORE_COMMIT_APPEND)?; - let changes = updates_to_changes(updates); - let outcome = self - .manifest - .commit_changes_with_lineage(&changes, &HashMap::new(), Some(&intent)) - .await?; - failpoints::maybe_fail(crate::failpoints::names::GRAPH_PUBLISH_AFTER_MANIFEST_COMMIT)?; - Ok(self.apply_lineage_to_cache(intent, &outcome)) - } - /// Mint a [`LineageIntent`] for the next commit on the current branch: a /// fresh ULID (stable across the publisher's CAS retries) and a timestamp. /// The parent is NOT chosen here — the publisher resolves it per attempt diff --git a/crates/omnigraph/src/db/manifest.rs b/crates/omnigraph/src/db/manifest.rs index c2803934..d6edf9bc 100644 --- a/crates/omnigraph/src/db/manifest.rs +++ b/crates/omnigraph/src/db/manifest.rs @@ -38,10 +38,12 @@ use namespace::{branch_manifest_namespace, staged_table_namespace}; pub(crate) use publisher::{GraphHeadExpectation, LineageIntent, PublishPrecondition}; use publisher::{GraphNamespacePublisher, ManifestBatchPublisher, PublishOutcome}; pub(crate) use recovery::{ - HealPendingOutcome, RecoveryAuthorityToken, RecoveryLineageIntent, RecoveryMode, - RecoverySidecar, RecoverySidecarHandle, SidecarKind, SidecarTablePin, SidecarTableRegistration, - SidecarTombstone, confirm_occ_sidecar_phase_b, confirm_sidecar_phase_b, delete_sidecar, - has_schema_apply_sidecar, heal_pending_sidecars_roll_forward, list_sidecars, new_occ_sidecar, + HealPendingOutcome, RecoveryAuthorityToken, RecoveryBranchMergeEffect, + RecoveryBranchMergeEffectKind, RecoveryLineageIntent, RecoveryManifestDelta, RecoveryMode, + RecoverySidecar, RecoverySidecarHandle, RecoveryTableUpdateSlot, SidecarKind, SidecarTablePin, + SidecarTableRegistration, SidecarTombstone, confirm_branch_merge_sidecar_phase_b, + confirm_occ_sidecar_phase_b, delete_sidecar, has_schema_apply_sidecar, + heal_pending_sidecars_roll_forward, list_sidecars, new_branch_merge_sidecar, new_occ_sidecar, new_sidecar, recover_manifest_drift, schema_apply_serial_queue_key, write_sidecar, }; pub use state::SubTableEntry; diff --git a/crates/omnigraph/src/db/manifest/recovery.rs b/crates/omnigraph/src/db/manifest/recovery.rs index 8977efa6..9090babb 100644 --- a/crates/omnigraph/src/db/manifest/recovery.rs +++ b/crates/omnigraph/src/db/manifest/recovery.rs @@ -55,23 +55,24 @@ use super::{ }; /// System actor identifier for recovery-owned lineage: legacy recovery, -/// v3 rollback, and orphan discard. A v3 roll-forward deliberately publishes -/// the original writer's fixed lineage and actor instead. The recovery audit -/// row is the authoritative attribution surface in both cases; the original -/// sidecar actor flows into its `recovery_for_actor` field. +/// exact-protocol rollback, and orphan discard. A v3/v4 roll-forward +/// deliberately publishes the original writer's fixed lineage and actor +/// instead. The recovery audit row is the authoritative attribution surface in +/// both cases; the original sidecar actor flows into its `recovery_for_actor` +/// field. pub(crate) const RECOVERY_ACTOR: &str = "omnigraph:recovery"; /// Publish a recovery action's manifest `updates` AND its lineage in one CAS -/// (RFC-013 Phase 7). Legacy recovery and v3 rollback publish a recovery commit; -/// v3 roll-forward publishes the original writer's fixed lineage intent. The -/// lineage (`graph_commit` + `graph_head`) rides the same merge-insert as the -/// table-version re-pin — there is no separate `_graph_commits.lance` write and -/// no manifest→commit-graph gap. +/// (RFC-013 Phase 7). Legacy recovery and v3/v4 rollback publish a recovery +/// commit; v3/v4 roll-forward publishes the original writer's fixed lineage +/// intent. The lineage (`graph_commit` + `graph_head`) rides the same +/// merge-insert as the table-version re-pin — there is no separate +/// `_graph_commits.lance` write and no manifest→commit-graph gap. /// `updates` is empty for the no-table-change recovery paths (all-NoMovement /// roll-back, stale-sidecar cleanup, orphaned-branch discard); the lineage rows /// still publish, so the recovery commit is always durable. /// -/// Legacy commits resolve their parent from the live branch head. A v3 +/// Legacy commits resolve their parent from the live branch head. An exact /// roll-forward instead carries an exact graph-head precondition, so it cannot /// be silently re-parented after authority changes. Returns the new manifest /// version and the stable commit id the audit row references. @@ -82,10 +83,30 @@ async fn publish_recovery_commit( updates: &[ManifestChange], expected: &HashMap, ) -> Result<(u64, String)> { - let intent = match (sidecar.protocol_v3.as_ref(), kind) { - (Some(protocol), RecoveryKind::RolledForward) => LineageIntent::from(&protocol.lineage), - (Some(protocol), RecoveryKind::RolledBack) => LineageIntent { - graph_commit_id: protocol.rollback_graph_commit_id.clone(), + let exact_lineage = sidecar + .protocol_v3 + .as_ref() + .map(|protocol| &protocol.lineage) + .or_else(|| { + sidecar + .protocol_v4 + .as_ref() + .map(|protocol| &protocol.lineage) + }); + let exact_rollback_id = sidecar + .protocol_v3 + .as_ref() + .map(|protocol| protocol.rollback_graph_commit_id.as_str()) + .or_else(|| { + sidecar + .protocol_v4 + .as_ref() + .map(|protocol| protocol.rollback_graph_commit_id.as_str()) + }); + let intent = match (exact_lineage, exact_rollback_id, kind) { + (Some(lineage), _, RecoveryKind::RolledForward) => LineageIntent::from(lineage), + (_, Some(rollback_graph_commit_id), RecoveryKind::RolledBack) => LineageIntent { + graph_commit_id: rollback_graph_commit_id.to_string(), branch: sidecar.branch.clone(), actor_id: Some(RECOVERY_ACTOR.to_string()), merged_parent_commit_id: None, @@ -94,12 +115,13 @@ async fn publish_recovery_commit( .parse::() .unwrap_or(crate::db::now_micros()?), }, - (Some(_), RecoveryKind::OrphanedBranchDiscarded) => { + (Some(_), _, RecoveryKind::OrphanedBranchDiscarded) + | (_, Some(_), RecoveryKind::OrphanedBranchDiscarded) => { return Err(OmniError::manifest_internal( - "orphaned-branch discard cannot publish through the v3 OCC recovery path", + "orphaned-branch discard cannot publish through an exact recovery protocol", )); } - (None, _) => { + (None, None, _) => { let merged_parent_commit_id = match (sidecar.writer_kind, kind) { (SidecarKind::BranchMerge, RecoveryKind::RolledForward) => { sidecar.merge_source_commit_id.clone() @@ -114,14 +136,29 @@ async fn publish_recovery_commit( created_at: crate::db::now_micros()?, } } + _ => { + return Err(OmniError::manifest_internal( + "exact recovery sidecar is missing either lineage or rollback identity", + )); + } }; let publisher = GraphNamespacePublisher::new(root_uri, sidecar.branch.as_deref()); - let precondition = match (sidecar.protocol_v3.as_ref(), kind) { - (Some(protocol), RecoveryKind::RolledForward) => { + let exact_authority = sidecar + .protocol_v3 + .as_ref() + .map(|protocol| &protocol.authority) + .or_else(|| { + sidecar + .protocol_v4 + .as_ref() + .map(|protocol| &protocol.authority) + }); + let precondition = match (exact_authority, kind) { + (Some(authority), RecoveryKind::RolledForward) => { PublishPrecondition::ExactGraphHead(GraphHeadExpectation::new( sidecar.branch.as_deref(), - protocol.authority.branch_identifier.clone(), - protocol.authority.graph_head.clone(), + authority.branch_identifier.clone(), + authority.graph_head.clone(), )) } _ => PublishPrecondition::Any, @@ -154,7 +191,15 @@ pub(crate) const RECOVERY_DIR_NAME: &str = "__recovery"; /// transaction identities, a canonical manifest delta, and an explicit /// Armed → EffectsConfirmed transition. Legacy writers deliberately continue /// producing v2 until their adapter opts into [`new_occ_sidecar`]. -pub(crate) const SIDECAR_SCHEMA_VERSION: u32 = 3; +/// +/// v3 → v4: RFC-022 branch-merge authority and exact confirmed output. A v4 +/// sidecar carries the captured target authority, fixed merge lineage and +/// rollback ids, the complete intended manifest delta (including pointer-only +/// updates), and an ordered exact transaction chain for each multi-commit HEAD +/// effect or an explicit ref-only first-touch fork. Mutation/Load remain fixed +/// at v3: raising the reader maximum must not silently enroll an existing writer +/// in new semantics. +pub(crate) const SIDECAR_SCHEMA_VERSION: u32 = 4; /// Schema version emitted by the legacy constructor. Fixed at v2 so merely /// teaching this binary to understand v3 does not silently enroll every writer @@ -171,10 +216,19 @@ pub(crate) const CONFIRMATION_SCHEMA_VERSION: u32 = 2; /// to the current maximum so future sidecar versions retain this floor. pub(crate) const EXACT_EFFECT_IDENTITY_SCHEMA_VERSION: u32 = 3; -/// Bound the cold-path transaction-history probe used to find a v3 sidecar's -/// UUID after unexpected drift. Normal recovery reads one version; a larger -/// gap indicates foreign activity and is surfaced as unverifiable rather than -/// turning open into an unbounded history walk. +/// Exact schema generation emitted by [`new_occ_sidecar`]. Keep this fixed when +/// the reader learns a newer sidecar generation; v3 Mutation/Load semantics are +/// not reinterpreted by the v4 BranchMerge adapter. +pub(crate) const MUTATION_LOAD_SIDECAR_SCHEMA_VERSION: u32 = 3; + +/// Exact schema generation emitted by [`new_branch_merge_sidecar`]. +pub(crate) const BRANCH_MERGE_SIDECAR_SCHEMA_VERSION: u32 = 4; + +/// Bound the cold-path transaction-history probes used by the v3/v4 exact +/// recovery protocols. Normal v3 recovery reads one version and a v4 logical +/// merge chain is currently at most three versions; a larger gap is derived +/// index work or foreign activity. Exceeding the bound is surfaced as +/// unverifiable rather than turning open into an unbounded history walk. const MAX_EFFECT_IDENTITY_SCAN_VERSIONS: u64 = 1024; /// Selects which recovery actions are allowed in a sweep. @@ -462,6 +516,88 @@ pub(crate) struct RecoveryProtocolV3 { pub intended_delta: RecoveryManifestDelta, } +/// One branch-merge physical effect. A merge can make several staged commits +/// to one table, or it can create only a native target ref and publish that +/// source state by pointer. The latter is independently durable even though no +/// table HEAD moves, so it must be represented explicitly rather than inferred +/// from a numeric version comparison. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub(crate) struct RecoveryBranchMergeEffect { + pub table_key: String, + pub kind: RecoveryBranchMergeEffectKind, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "kind", rename_all = "PascalCase")] +pub(crate) enum RecoveryBranchMergeEffectKind { + /// One or more logical data commits on the target table. The exact final + /// version is unknown while Armed and is bound atomically with the complete + /// manifest delta at EffectsConfirmed. `source_fork_version` is present + /// when the target table ref itself is first-touch and therefore must be + /// recovered before any HEAD movement is considered. + MultiCommitHead { + #[serde(default, skip_serializing_if = "Option::is_none")] + source_fork_version: Option, + /// Exact Lance transactions the writer pre-minted for the logical + /// data effect, in commit order. Recovery proves ownership by reading + /// these identities back at their expected versions; a numeric HEAD + /// advance alone is never sufficient evidence. + planned_transactions: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + confirmed_version: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + confirmed_branch_identifier: Option, + }, + /// A first-touch target ref whose HEAD remains exactly the captured source + /// fork version. Lance mints the target BranchIdentifier during create, so + /// it is necessarily an EffectsConfirmed output rather than an arm-time + /// input. + RefOnlyFork { + source_version: u64, + #[serde(default, skip_serializing_if = "Option::is_none")] + confirmed_branch_identifier: Option, + }, +} + +impl RecoveryBranchMergeEffectKind { + fn source_fork_version(&self) -> Option { + match self { + Self::MultiCommitHead { + source_fork_version, + .. + } => *source_fork_version, + Self::RefOnlyFork { source_version, .. } => Some(*source_version), + } + } + + fn confirmed_branch_identifier(&self) -> Option<&lance::dataset::refs::BranchIdentifier> { + match self { + Self::MultiCommitHead { + confirmed_branch_identifier, + .. + } + | Self::RefOnlyFork { + confirmed_branch_identifier, + .. + } => confirmed_branch_identifier.as_ref(), + } + } +} + +/// v4 BranchMerge payload. Kept separate from v3 so old Mutation/Load files +/// retain their one-transaction-per-table interpretation forever. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub(crate) struct RecoveryProtocolV4 { + pub authority: RecoveryAuthorityToken, + pub lineage: RecoveryLineageIntent, + pub rollback_graph_commit_id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub rollback_audit_outcomes: Option>, + pub effect_phase: RecoveryEffectPhase, + pub effects: Vec, + pub intended_delta: RecoveryManifestDelta, +} + /// In-memory representation of the on-disk JSON sidecar. #[derive(Debug, Clone, Serialize, Deserialize)] pub(crate) struct RecoverySidecar { @@ -499,6 +635,9 @@ pub(crate) struct RecoverySidecar { /// RFC-022 exact-effect protocol. Absent for every v1/v2 sidecar. #[serde(default, skip_serializing_if = "Option::is_none")] pub protocol_v3: Option, + /// RFC-022 BranchMerge protocol. Present only on schema-v4 sidecars. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub protocol_v4: Option, } /// Opaque handle returned by [`write_sidecar`] so the caller can delete @@ -670,6 +809,7 @@ pub(crate) async fn write_sidecar( /// commit, so the `BranchMerge` arm of `classify_table` could fold back into the /// strict single-commit path and `IncompletePhaseB` retire. Do NOT delete this /// with the row path — keep the sidecar; only simplify the classifier. +#[cfg(test)] pub(crate) async fn confirm_sidecar_phase_b( root_uri: &str, storage: &dyn StorageAdapter, @@ -847,14 +987,24 @@ fn validate_sidecar_shape(sidecar_uri: &str, sidecar: &RecoverySidecar) -> Resul }; if sidecar.schema_version < EXACT_EFFECT_IDENTITY_SCHEMA_VERSION { - if sidecar.protocol_v3.is_some() { + if sidecar.protocol_v3.is_some() || sidecar.protocol_v4.is_some() { return Err(malformed( - "protocol_v3 is present on a pre-v3 sidecar".to_string(), + "an exact-effect protocol is present on a pre-v3 sidecar".to_string(), )); } return Ok(()); } + if sidecar.schema_version == BRANCH_MERGE_SIDECAR_SCHEMA_VERSION { + return validate_branch_merge_v4_shape(sidecar_uri, sidecar); + } + + if sidecar.protocol_v4.is_some() { + return Err(malformed( + "protocol_v4 is present on a pre-v4 sidecar".to_string(), + )); + } + if !matches!( sidecar.writer_kind, SidecarKind::Mutation | SidecarKind::Load @@ -1022,6 +1172,361 @@ fn validate_sidecar_shape(sidecar_uri: &str, sidecar: &RecoverySidecar) -> Resul Ok(()) } +fn validate_branch_merge_v4_shape(sidecar_uri: &str, sidecar: &RecoverySidecar) -> Result<()> { + let malformed = |reason: String| { + OmniError::manifest_internal(format!( + "recovery sidecar at '{}' has an invalid schema-v{} shape: {}", + sidecar_uri, sidecar.schema_version, reason + )) + }; + + if sidecar.writer_kind != SidecarKind::BranchMerge { + return Err(malformed(format!( + "protocol_v4 is only defined for BranchMerge, found {:?}", + sidecar.writer_kind + ))); + } + if sidecar.protocol_v3.is_some() { + return Err(malformed( + "schema-v4 BranchMerge sidecar also carries protocol_v3".to_string(), + )); + } + if sidecar.merge_source_commit_id.is_some() + || !sidecar.additional_registrations.is_empty() + || !sidecar.tombstones.is_empty() + { + return Err(malformed( + "schema-v4 BranchMerge must carry lineage/delta only in protocol_v4".to_string(), + )); + } + let protocol = sidecar + .protocol_v4 + .as_ref() + .ok_or_else(|| malformed("missing required protocol_v4 payload".to_string()))?; + if !protocol.intended_delta.registrations.is_empty() + || !protocol.intended_delta.tombstones.is_empty() + { + return Err(malformed( + "BranchMerge cannot register or tombstone tables".to_string(), + )); + } + + let sidecar_branch = sidecar.branch.as_deref().filter(|branch| *branch != "main"); + let lineage_branch = protocol + .lineage + .branch + .as_deref() + .filter(|branch| *branch != "main"); + if sidecar_branch != lineage_branch { + return Err(malformed( + "sidecar branch does not match original merge lineage branch".to_string(), + )); + } + if sidecar.actor_id != protocol.lineage.actor_id { + return Err(malformed( + "sidecar actor does not match original merge lineage actor".to_string(), + )); + } + if protocol.lineage.merged_parent_commit_id.is_none() { + return Err(malformed( + "BranchMerge lineage must carry the captured source commit".to_string(), + )); + } + if protocol.rollback_graph_commit_id == protocol.lineage.graph_commit_id { + return Err(malformed( + "original and rollback graph commit ids must differ".to_string(), + )); + } + + let pin_keys: HashSet<&str> = sidecar + .tables + .iter() + .map(|pin| pin.table_key.as_str()) + .collect(); + let effect_keys: HashSet<&str> = protocol + .effects + .iter() + .map(|effect| effect.table_key.as_str()) + .collect(); + let delta_keys: HashSet<&str> = protocol + .intended_delta + .table_updates + .iter() + .map(|slot| slot.table_key.as_str()) + .collect(); + if sidecar.tables.is_empty() + || pin_keys.len() != sidecar.tables.len() + || effect_keys.len() != protocol.effects.len() + || delta_keys.len() != protocol.intended_delta.table_updates.len() + || effect_keys != pin_keys + || !effect_keys.is_subset(&delta_keys) + { + return Err(malformed( + "physical table pins/effects must be one-to-one and a subset of unique intended-delta slots" + .to_string(), + )); + } + if let Some(outcomes) = protocol.rollback_audit_outcomes.as_ref() { + let outcome_keys: HashSet<&str> = outcomes + .iter() + .map(|outcome| outcome.table_key.as_str()) + .collect(); + if outcome_keys.len() != outcomes.len() || !outcome_keys.is_subset(&pin_keys) { + return Err(malformed( + "rollback audit outcomes must name a unique subset of physical table pins" + .to_string(), + )); + } + } + + for slot in &protocol.intended_delta.table_updates { + match protocol.effect_phase { + RecoveryEffectPhase::Armed if slot.confirmed.is_some() => { + return Err(malformed(format!( + "Armed manifest slot '{}' already carries confirmation", + slot.table_key + ))); + } + RecoveryEffectPhase::EffectsConfirmed if slot.confirmed.is_none() => { + return Err(malformed(format!( + "EffectsConfirmed manifest slot '{}' lacks its exact output", + slot.table_key + ))); + } + _ => {} + } + if let Some(confirmed) = slot.confirmed.as_ref() + && confirmed.table_branch != slot.table_branch + { + return Err(malformed(format!( + "confirmed output branch for '{}' differs from its planned slot", + slot.table_key + ))); + } + } + + let mut planned_transaction_uuids = HashSet::new(); + for pin in &sidecar.tables { + let effect = protocol + .effects + .iter() + .find(|effect| effect.table_key == pin.table_key) + .expect("effect/pin key sets checked above"); + let slot = protocol + .intended_delta + .table_updates + .iter() + .find(|slot| slot.table_key == pin.table_key) + .expect("physical effects are a subset of delta slots"); + if slot.expected_version != pin.expected_version || slot.table_branch != pin.table_branch { + return Err(malformed(format!( + "intended-delta pre-state for '{}' does not match its physical table pin", + pin.table_key + ))); + } + if pin + .table_branch + .as_deref() + .is_none_or(|branch| branch == "main") + && effect.kind.source_fork_version().is_some() + { + return Err(malformed(format!( + "first-touch effect '{}' does not name a non-main target ref", + pin.table_key + ))); + } + if let Some(source_fork_version) = effect.kind.source_fork_version() + && matches!( + &effect.kind, + RecoveryBranchMergeEffectKind::MultiCommitHead { .. } + ) + && source_fork_version != pin.expected_version + { + return Err(malformed(format!( + "first-touch multi-commit effect '{}' forks from version {}, pin expects {}", + pin.table_key, source_fork_version, pin.expected_version + ))); + } + match (&effect.kind, protocol.effect_phase) { + ( + RecoveryBranchMergeEffectKind::MultiCommitHead { + planned_transactions, + confirmed_version, + confirmed_branch_identifier, + source_fork_version, + }, + RecoveryEffectPhase::Armed, + ) => { + validate_branch_merge_transaction_chain( + &malformed, + pin, + planned_transactions, + &mut planned_transaction_uuids, + )?; + if confirmed_version.is_some() + || confirmed_branch_identifier.is_some() + || pin.confirmed_version.is_some() + { + return Err(malformed(format!( + "Armed multi-commit effect '{}' already carries confirmation", + pin.table_key + ))); + } + if source_fork_version.is_none() && confirmed_branch_identifier.is_some() { + return Err(malformed(format!( + "owned-ref effect '{}' unexpectedly carries a branch identifier", + pin.table_key + ))); + } + } + ( + RecoveryBranchMergeEffectKind::MultiCommitHead { + planned_transactions, + confirmed_version, + confirmed_branch_identifier, + source_fork_version, + }, + RecoveryEffectPhase::EffectsConfirmed, + ) => { + let planned_final_version = validate_branch_merge_transaction_chain( + &malformed, + pin, + planned_transactions, + &mut planned_transaction_uuids, + )?; + let version = confirmed_version.ok_or_else(|| { + malformed(format!( + "EffectsConfirmed multi-commit effect '{}' lacks final version", + pin.table_key + )) + })?; + let confirmed = slot.confirmed.as_ref().expect("phase checked above"); + if pin.confirmed_version != Some(version) + || confirmed.table_version != version + || version < planned_final_version + { + return Err(malformed(format!( + "multi-commit effect '{}' confirmation does not match its pin/delta", + pin.table_key + ))); + } + if source_fork_version.is_some() != confirmed_branch_identifier.is_some() { + return Err(malformed(format!( + "first-touch multi-commit effect '{}' must confirm its target ref identity", + pin.table_key + ))); + } + } + ( + RecoveryBranchMergeEffectKind::RefOnlyFork { + source_version: _, + confirmed_branch_identifier, + }, + RecoveryEffectPhase::Armed, + ) => { + if confirmed_branch_identifier.is_some() || pin.confirmed_version.is_some() { + return Err(malformed(format!( + "Armed ref-only effect '{}' already carries confirmation", + pin.table_key + ))); + } + } + ( + RecoveryBranchMergeEffectKind::RefOnlyFork { + source_version, + confirmed_branch_identifier, + }, + RecoveryEffectPhase::EffectsConfirmed, + ) => { + let confirmed = slot.confirmed.as_ref().expect("phase checked above"); + if confirmed_branch_identifier.is_none() + || pin.confirmed_version != Some(*source_version) + || confirmed.table_version != *source_version + { + return Err(malformed(format!( + "ref-only effect '{}' confirmation does not match its exact fork version", + pin.table_key + ))); + } + } + } + } + Ok(()) +} + +fn validate_branch_merge_transaction_chain( + malformed: &impl Fn(String) -> OmniError, + pin: &SidecarTablePin, + planned_transactions: &[StagedTransactionIdentity], + planned_transaction_uuids: &mut HashSet, +) -> Result { + let first = planned_transactions.first().ok_or_else(|| { + malformed(format!( + "multi-commit effect '{}' has an empty planned transaction chain", + pin.table_key + )) + })?; + if first.read_version != pin.expected_version { + return Err(malformed(format!( + "planned transaction chain for '{}' begins at version {}, pin expects {}", + pin.table_key, first.read_version, pin.expected_version + ))); + } + let first_output = first.read_version.checked_add(1).ok_or_else(|| { + malformed(format!( + "planned transaction chain for '{}' overflows its first output version", + pin.table_key + )) + })?; + if pin.post_commit_pin != first_output { + return Err(malformed(format!( + "post-commit pin for '{}' is {}, first planned output is {}", + pin.table_key, pin.post_commit_pin, first_output + ))); + } + + for (index, planned) in planned_transactions.iter().enumerate() { + if planned.uuid.is_empty() || !planned_transaction_uuids.insert(planned.uuid.clone()) { + return Err(malformed(format!( + "planned transaction chain for '{}' contains an empty or duplicate identity at offset {}", + pin.table_key, index + ))); + } + let expected_read_version = pin + .expected_version + .checked_add(u64::try_from(index).map_err(|_| { + malformed(format!( + "planned transaction chain for '{}' exceeds u64", + pin.table_key + )) + })?) + .ok_or_else(|| { + malformed(format!( + "planned transaction chain for '{}' overflows at offset {}", + pin.table_key, index + )) + })?; + if planned.read_version != expected_read_version { + return Err(malformed(format!( + "planned transaction chain for '{}' reads version {} at offset {}, expected {}", + pin.table_key, planned.read_version, index, expected_read_version + ))); + } + } + + planned_transactions + .last() + .expect("non-empty chain checked above") + .read_version + .checked_add(1) + .ok_or_else(|| { + malformed(format!( + "planned transaction chain for '{}' overflows its final output version", + pin.table_key + )) + }) +} + /// Classify one table's observed state vs. the sidecar's intent. /// /// `kind` adjusts the precision of the `RolledPastExpected` predicate: @@ -1653,6 +2158,9 @@ async fn process_sidecar( // stale-sidecar audit recovery). `false` = the sidecar was deferred // untouched -- callers must not treat that as a completed heal (no // schema reload / cache invalidation is warranted). + if sidecar.protocol_v4.is_some() { + return process_branch_merge_sidecar_v4(root_uri, storage, snapshot, sidecar, mode).await; + } if sidecar.protocol_v3.is_some() { if let Some(outcome) = detect_visible_v3_outcome(root_uri, sidecar).await? { return finalize_visible_v3_outcome(root_uri, storage.as_ref(), sidecar, outcome).await; @@ -2138,6 +2646,542 @@ async fn process_sidecar( } } +struct BranchMergeRefObservation { + dataset: lance::Dataset, + version: u64, + branch_identifier: lance::dataset::refs::BranchIdentifier, + parent_version: Option, +} + +/// Observe one physical target ref without treating an absent first-touch ref +/// as a storage error. A named ref's BranchContents identity is separate from +/// the graph-manifest branch identity carried by `RecoveryAuthorityToken`; both +/// are required to close their respective delete/recreate ABA windows. +async fn observe_branch_merge_target_ref( + pin: &SidecarTablePin, +) -> Result> { + let dataset = crate::instrumentation::open_dataset( + &pin.table_path, + crate::instrumentation::VersionResolution::Latest, + None, + crate::instrumentation::table_wrapper(), + ) + .await?; + let Some(branch) = pin + .table_branch + .as_deref() + .filter(|branch| *branch != "main") + else { + let branch_identifier = dataset + .branch_identifier() + .await + .map_err(|error| OmniError::Lance(error.to_string()))?; + let version = dataset.version().version; + return Ok(Some(BranchMergeRefObservation { + dataset, + version, + branch_identifier, + parent_version: None, + })); + }; + let branches = dataset + .list_branches() + .await + .map_err(|error| OmniError::Lance(error.to_string()))?; + let Some(contents) = branches.get(branch) else { + return Ok(None); + }; + let target = dataset + .checkout_branch(branch) + .await + .map_err(|error| OmniError::Lance(error.to_string()))?; + let version = target.version().version; + Ok(Some(BranchMergeRefObservation { + dataset: target, + version, + branch_identifier: contents.identifier.clone(), + parent_version: Some(contents.parent_version), + })) +} + +struct BranchMergeMultiCommitProof { + effect_ownership: EffectOwnership, + /// Every planned logical-data transaction is present in exact order and + /// every later commit through the observed HEAD is derived CreateIndex + /// work. Confirmation still has to bind that exact HEAD before recovery + /// may roll it forward. + full_effect_at_head: bool, + unsafe_reason: Option, +} + +impl BranchMergeMultiCommitProof { + fn unverifiable(reason: String) -> Self { + Self { + effect_ownership: EffectOwnership::Unverifiable, + full_effect_at_head: false, + unsafe_reason: Some(reason), + } + } +} + +/// Prove ownership of a BranchMerge multi-commit effect from Lance's durable +/// transaction history. Numeric version movement is only a scan bound: each +/// logical data commit must match its pre-minted identity at its exact output +/// version. Once the full chain is present, inline index reconciliation may +/// contribute a derived `CreateIndex` tail. No other operation is attributable +/// to this merge. +/// +/// A final Restore to the still-manifest-pinned version is the compensation a +/// previous Full recovery created. Recognizing it makes the restore→manifest +/// publish crash window restartable without appending another Restore commit. +async fn prove_branch_merge_multi_commit_effect( + observation: &BranchMergeRefObservation, + planned_transactions: &[StagedTransactionIdentity], + manifest_pinned: u64, + table_key: &str, +) -> Result { + let first = planned_transactions + .first() + .expect("v4 sidecar validation requires a non-empty transaction chain"); + let base_version = first.read_version; + let lance_head = observation.version; + if lance_head < base_version { + return Ok(BranchMergeMultiCommitProof::unverifiable(format!( + "table '{table_key}' HEAD {lance_head} is behind its planned transaction base {base_version}" + ))); + } + if lance_head == base_version { + return Ok(BranchMergeMultiCommitProof { + effect_ownership: EffectOwnership::None, + full_effect_at_head: false, + unsafe_reason: None, + }); + } + if lance_head.saturating_sub(base_version) > MAX_EFFECT_IDENTITY_SCAN_VERSIONS { + return Ok(BranchMergeMultiCommitProof::unverifiable(format!( + "table '{table_key}' requires scanning {} transaction versions, above the recovery bound {}", + lance_head.saturating_sub(base_version), + MAX_EFFECT_IDENTITY_SCAN_VERSIONS + ))); + } + + let mut planned_index = 0usize; + for version in base_version + 1..=lance_head { + let transaction = if version == lance_head { + observation + .dataset + .read_transaction() + .await + .map_err(|error| OmniError::Lance(error.to_string()))? + } else { + observation + .dataset + .read_transaction_by_version(version) + .await + .map_err(|error| OmniError::Lance(error.to_string()))? + }; + let Some(transaction) = transaction else { + return Ok(BranchMergeMultiCommitProof::unverifiable(format!( + "table '{table_key}' has no readable transaction identity at version {version}" + ))); + }; + + // Rollback can interrupt a proper prefix of the logical chain, not + // only the complete chain. Its final Restore is attributable after at + // least one exact planned transaction has established ownership. + if version == lance_head + && planned_index > 0 + && matches!( + &transaction.operation, + lance::dataset::transaction::Operation::Restore { version } + if *version == manifest_pinned + ) + { + return Ok(BranchMergeMultiCommitProof { + effect_ownership: EffectOwnership::OwnCompensatedAtHead, + full_effect_at_head: false, + unsafe_reason: None, + }); + } + + let observed_identity = StagedTransactionIdentity::from(&transaction); + if let Some(planned) = planned_transactions.get(planned_index) { + let expected_output_version = planned.read_version.checked_add(1).ok_or_else(|| { + OmniError::manifest_internal(format!( + "BranchMerge transaction version overflow for table '{table_key}'" + )) + })?; + if version != expected_output_version || &observed_identity != planned { + return Ok(BranchMergeMultiCommitProof::unverifiable(format!( + "table '{table_key}' transaction at version {version} is {observed_identity:?}, expected exact planned identity {planned:?} at version {expected_output_version}" + ))); + } + planned_index += 1; + continue; + } + + if !matches!( + &transaction.operation, + lance::dataset::transaction::Operation::CreateIndex { .. } + ) { + return Ok(BranchMergeMultiCommitProof::unverifiable(format!( + "table '{table_key}' has non-derived operation {:?} at version {version} after its complete planned transaction chain", + transaction.operation + ))); + } + } + + let full_effect_at_head = planned_index == planned_transactions.len(); + Ok(BranchMergeMultiCommitProof { + effect_ownership: if planned_index > 0 { + EffectOwnership::OwnAtHead + } else { + EffectOwnership::None + }, + full_effect_at_head, + unsafe_reason: None, + }) +} + +async fn process_branch_merge_sidecar_v4( + root_uri: &str, + storage: &std::sync::Arc, + snapshot: &Snapshot, + sidecar: &RecoverySidecar, + mode: RecoveryMode, +) -> Result { + if let Some(outcome) = detect_visible_v4_outcome(root_uri, sidecar).await? { + return finalize_visible_v4_outcome(root_uri, storage.as_ref(), sidecar, outcome).await; + } + let protocol = sidecar + .protocol_v4 + .as_ref() + .expect("caller checked protocol_v4"); + let mut states = Vec::with_capacity(sidecar.tables.len()); + let mut any_physical_effect = false; + let mut any_head_movement = false; + let mut confirmed_mismatch: Option = None; + let mut unsafe_observation: Option = None; + let mut all_confirmed_effects_at_head = + protocol.effect_phase == RecoveryEffectPhase::EffectsConfirmed; + + for pin in &sidecar.tables { + let effect = protocol + .effects + .iter() + .find(|effect| effect.table_key == pin.table_key) + .expect("v4 sidecar shape validates effect/pin key sets"); + let manifest_pinned = snapshot + .entry(&pin.table_key) + .map(|entry| entry.table_version) + .unwrap_or(0); + let unpublished_fork = pin + .table_branch + .as_deref() + .is_some_and(|branch| branch != "main") + && snapshot + .entry(&pin.table_key) + .map(|entry| entry.table_branch != pin.table_branch) + .unwrap_or(true); + if manifest_pinned != pin.expected_version { + unsafe_observation.get_or_insert_with(|| { + format!( + "table '{}' manifest pin changed from {} to {} while its merge effect remained pending", + pin.table_key, pin.expected_version, manifest_pinned + ) + }); + } + + let observed = observe_branch_merge_target_ref(pin).await?; + let first_touch_version = effect.kind.source_fork_version(); + if observed.is_none() && first_touch_version.is_none() { + unsafe_observation.get_or_insert_with(|| { + format!( + "existing target ref for table '{}' disappeared while BranchMerge recovery was pending", + pin.table_key + ) + }); + } + if let (Some(observed), Some(source_version)) = (&observed, first_touch_version) { + any_physical_effect = true; + if observed.parent_version != Some(source_version) { + unsafe_observation.get_or_insert_with(|| { + format!( + "first-touch target ref for table '{}' was forked at {:?}, expected exact source version {}", + pin.table_key, observed.parent_version, source_version + ) + }); + } + if let Some(expected_identifier) = effect.kind.confirmed_branch_identifier() + && &observed.branch_identifier != expected_identifier + { + unsafe_observation.get_or_insert_with(|| { + format!( + "first-touch target ref identity for table '{}' differs from its confirmed merge effect", + pin.table_key + ) + }); + } + } + + let (classification, lance_head, effect_ownership) = match &effect.kind { + RecoveryBranchMergeEffectKind::MultiCommitHead { + planned_transactions, + confirmed_version, + .. + } => { + let lance_head = observed + .as_ref() + .map(|observation| observation.version) + .unwrap_or(manifest_pinned); + if lance_head < pin.expected_version { + unsafe_observation.get_or_insert_with(|| { + format!( + "table '{}' HEAD {} is behind prepared merge pin {}", + pin.table_key, lance_head, pin.expected_version + ) + }); + } + let moved = lance_head > pin.expected_version; + any_physical_effect |= moved; + any_head_movement |= moved; + let proof = if let Some(observation) = observed.as_ref() { + prove_branch_merge_multi_commit_effect( + observation, + planned_transactions, + manifest_pinned, + &pin.table_key, + ) + .await? + } else { + BranchMergeMultiCommitProof { + effect_ownership: EffectOwnership::None, + full_effect_at_head: false, + unsafe_reason: None, + } + }; + if let Some(reason) = proof.unsafe_reason.as_ref() { + unsafe_observation.get_or_insert_with(|| reason.clone()); + } + let complete = protocol.effect_phase == RecoveryEffectPhase::EffectsConfirmed + && proof.full_effect_at_head + && confirmed_version == &Some(lance_head) + && pin.confirmed_version == Some(lance_head); + all_confirmed_effects_at_head &= complete; + if protocol.effect_phase == RecoveryEffectPhase::EffectsConfirmed + && !complete + && proof.effect_ownership != EffectOwnership::OwnCompensatedAtHead + { + confirmed_mismatch.get_or_insert_with(|| { + format!( + "confirmed multi-commit effect '{}' expected HEAD {:?}, observed {}", + pin.table_key, confirmed_version, lance_head + ) + }); + } + let classification = if complete { + TableClassification::RolledPastExpected + } else if moved { + TableClassification::IncompletePhaseB + } else { + TableClassification::NoMovement + }; + (classification, lance_head, proof.effect_ownership) + } + RecoveryBranchMergeEffectKind::RefOnlyFork { source_version, .. } => { + let complete = protocol.effect_phase == RecoveryEffectPhase::EffectsConfirmed + && observed + .as_ref() + .is_some_and(|observation| observation.version == *source_version) + && pin.confirmed_version == Some(*source_version); + all_confirmed_effects_at_head &= complete; + if protocol.effect_phase == RecoveryEffectPhase::EffectsConfirmed && !complete { + confirmed_mismatch.get_or_insert_with(|| { + format!( + "confirmed ref-only effect '{}' expected exact fork version {}, observed {:?}", + pin.table_key, + source_version, + observed.as_ref().map(|observation| observation.version) + ) + }); + } + ( + if complete { + TableClassification::RolledPastExpected + } else if observed.is_some() { + TableClassification::IncompletePhaseB + } else { + TableClassification::NoMovement + }, + // A ref-only fork never represents a data-HEAD delta. Keep + // rollback on the explicit ref-cleanup path rather than + // presenting the source version as a restore candidate. + manifest_pinned, + EffectOwnership::None, + ) + } + }; + states.push(ClassifiedTable { + classification, + manifest_pinned, + lance_head, + effect_ownership, + unpublished_fork, + }); + } + + if let Some(reason) = unsafe_observation { + let message = format!( + "BranchMerge recovery sidecar '{}' observed foreign or unverifiable physical state: {}", + sidecar.operation_id, reason + ); + return match mode { + RecoveryMode::RollForwardOnly => { + warn!(operation_id = sidecar.operation_id.as_str(), "{message}"); + Ok(false) + } + RecoveryMode::Full => Err(OmniError::manifest_internal(message)), + }; + } + if let Some(reason) = confirmed_mismatch { + let message = format!( + "BranchMerge recovery sidecar '{}' no longer matches its exact confirmed effects: {}", + sidecar.operation_id, reason + ); + return match mode { + RecoveryMode::RollForwardOnly => { + warn!(operation_id = sidecar.operation_id.as_str(), "{message}"); + Ok(false) + } + RecoveryMode::Full => Err(OmniError::manifest_internal(message)), + }; + } + + let live_authority = + read_live_recovery_authority(root_uri, storage, sidecar.branch.as_deref()).await?; + let branch_recreated = live_authority.branch_identifier != protocol.authority.branch_identifier; + let authority_changed = live_authority != protocol.authority; + if branch_recreated && any_physical_effect { + let message = format!( + "BranchMerge recovery sidecar '{}' targets a branch incarnation that was deleted and recreated; refusing to act through the reused name", + sidecar.operation_id + ); + return match mode { + RecoveryMode::RollForwardOnly => { + warn!(operation_id = sidecar.operation_id.as_str(), "{message}"); + Ok(false) + } + RecoveryMode::Full => Err(OmniError::manifest_internal(message)), + }; + } + + if !authority_changed && all_confirmed_effects_at_head { + return roll_forward_branch_merge_v4(root_uri, storage, sidecar, mode).await; + } + + // A first-touch ref with no data commit is a recoverable physical artifact, + // but cleaning it restores the exact pre-attempt graph state. Do not + // manufacture a rollback graph commit: doing so would advance the target + // lineage and turn a later logical fast-forward into a three-way merge. + if !any_head_movement { + if matches!(mode, RecoveryMode::RollForwardOnly) { + warn!( + operation_id = sidecar.operation_id.as_str(), + "recovery: deferring armed BranchMerge intent with no proven physical effects" + ); + return Ok(false); + } + if let NoEffectForkCleanup::DeferredPathChild { .. } = + cleanup_unpublished_no_effect_forks(root_uri, storage.as_ref(), sidecar, &states) + .await? + { + return Ok(false); + } + delete_sidecar_by_operation_id(root_uri, storage.as_ref(), &sidecar.operation_id).await?; + return Ok(true); + } + + if matches!(mode, RecoveryMode::RollForwardOnly) { + warn!( + operation_id = sidecar.operation_id.as_str(), + authority_changed, "recovery: deferring rollback-eligible BranchMerge sidecar" + ); + return Ok(false); + } + roll_back_sidecar(root_uri, storage.as_ref(), sidecar, &states).await?; + Ok(true) +} + +async fn roll_forward_branch_merge_v4( + root_uri: &str, + storage: &std::sync::Arc, + sidecar: &RecoverySidecar, + mode: RecoveryMode, +) -> Result { + let protocol = sidecar + .protocol_v4 + .as_ref() + .expect("caller checked protocol_v4"); + let mut updates = Vec::with_capacity(protocol.intended_delta.table_updates.len()); + let mut expected = HashMap::with_capacity(protocol.intended_delta.table_updates.len()); + let mut outcomes = Vec::with_capacity(protocol.intended_delta.table_updates.len()); + for slot in &protocol.intended_delta.table_updates { + let confirmed = slot.confirmed.as_ref().ok_or_else(|| { + OmniError::manifest_internal(format!( + "BranchMerge recovery sidecar '{}' delta slot '{}' is not confirmed", + sidecar.operation_id, slot.table_key + )) + })?; + expected.insert(slot.table_key.clone(), slot.expected_version); + updates.push(ManifestChange::Update(SubTableUpdate { + table_key: slot.table_key.clone(), + table_version: confirmed.table_version, + table_branch: confirmed.table_branch.clone(), + row_count: confirmed.row_count, + version_metadata: confirmed.version_metadata.clone(), + })); + outcomes.push(TableOutcome { + table_key: slot.table_key.clone(), + from_version: slot.expected_version, + to_version: confirmed.table_version, + }); + } + + crate::failpoints::maybe_fail(crate::failpoints::names::RECOVERY_BEFORE_ROLL_FORWARD_PUBLISH)?; + let graph_commit_id = match publish_recovery_commit( + root_uri, + sidecar, + RecoveryKind::RolledForward, + &updates, + &expected, + ) + .await + { + Ok((_, graph_commit_id)) => graph_commit_id, + Err(error) if error.is_read_set_changed() => { + if let Some(outcome) = detect_visible_v4_outcome(root_uri, sidecar).await? { + return finalize_visible_v4_outcome(root_uri, storage.as_ref(), sidecar, outcome) + .await; + } + return match mode { + RecoveryMode::RollForwardOnly => Ok(false), + RecoveryMode::Full => Err(error), + }; + } + Err(error) => return Err(error), + }; + record_audit( + root_uri, + sidecar, + graph_commit_id, + RecoveryKind::RolledForward, + outcomes, + ) + .await?; + delete_sidecar_by_operation_id(root_uri, storage.as_ref(), &sidecar.operation_id).await?; + Ok(true) +} + /// True if `err` is the publisher's per-table CAS precondition failure /// (`ExpectedVersionMismatch`) — the signal that a concurrent writer advanced /// the manifest past what this caller expected. @@ -2399,8 +3443,30 @@ enum NoEffectForkCleanup { }, } -/// Remove first-touch named-branch refs created by an Armed v3 attempt that -/// never landed this table's planned transaction. +fn has_exact_protocol(sidecar: &RecoverySidecar) -> bool { + sidecar.protocol_v3.is_some() || sidecar.protocol_v4.is_some() +} + +fn v4_effect_for<'a>( + sidecar: &'a RecoverySidecar, + table_key: &str, +) -> Option<&'a RecoveryBranchMergeEffect> { + sidecar + .protocol_v4 + .as_ref()? + .effects + .iter() + .find(|effect| effect.table_key == table_key) +} + +fn first_touch_fork_version(sidecar: &RecoverySidecar, pin: &SidecarTablePin) -> u64 { + v4_effect_for(sidecar, &pin.table_key) + .and_then(|effect| effect.kind.source_fork_version()) + .unwrap_or(pin.expected_version) +} + +/// Remove first-touch named-branch refs created by an Armed exact-protocol +/// attempt that never completed this table's planned effect. /// /// The sidecar is durable before the ref is created, so it is the ownership /// record while the manifest still inherits the table from another branch. @@ -2417,7 +3483,7 @@ async fn cleanup_unpublished_no_effect_forks( sidecar: &RecoverySidecar, states: &[ClassifiedTable], ) -> Result { - if sidecar.protocol_v3.is_none() { + if !has_exact_protocol(sidecar) { return Ok(NoEffectForkCleanup::Complete); } @@ -2496,7 +3562,8 @@ async fn cleanup_unpublished_no_effect_forks( } continue; }; - if contents.parent_version != pin.expected_version { + let exact_fork_version = first_touch_fork_version(sidecar, pin); + if contents.parent_version != exact_fork_version { return Err(OmniError::manifest_internal(format!( "OCC recovery sidecar '{}' cannot discard unpublished fork '{}:{}': \ parent version is {}, expected exact fork point {}", @@ -2504,14 +3571,24 @@ async fn cleanup_unpublished_no_effect_forks( pin.table_path, target_branch, contents.parent_version, - pin.expected_version + exact_fork_version + ))); + } + if let Some(expected_identifier) = v4_effect_for(sidecar, &pin.table_key) + .and_then(|effect| effect.kind.confirmed_branch_identifier()) + && &contents.identifier != expected_identifier + { + return Err(OmniError::manifest_internal(format!( + "BranchMerge recovery sidecar '{}' cannot discard unpublished fork '{}:{}': \ + live target ref identity differs from the confirmed effect", + sidecar.operation_id, pin.table_path, target_branch ))); } let target = dataset .checkout_branch(target_branch) .await .map_err(|error| OmniError::Lance(error.to_string()))?; - if target.version().version != pin.expected_version { + if target.version().version != exact_fork_version { return Err(OmniError::manifest_internal(format!( "OCC recovery sidecar '{}' cannot discard unpublished fork '{}:{}': \ live HEAD is {}, expected untouched version {}", @@ -2519,7 +3596,7 @@ async fn cleanup_unpublished_no_effect_forks( pin.table_path, target_branch, target.version().version, - pin.expected_version + exact_fork_version ))); } dataset @@ -2541,7 +3618,7 @@ fn table_requires_rollback_effect(state: &ClassifiedTable) -> bool { ) } -fn v3_rollback_audit_outcomes( +fn exact_rollback_audit_outcomes( sidecar: &RecoverySidecar, states: &[ClassifiedTable], ) -> Vec { @@ -2563,32 +3640,48 @@ fn v3_rollback_audit_outcomes( .collect() } -/// Durably bind the exact audit payload for a v3 rollback before the first +/// Durably bind the exact audit payload for a v3/v4 rollback before the first /// restore or rollback publish. A retry must replay the original observation, /// not reconstruct it from post-restore HEADs. In particular, partial /// multi-table attempts omit untouched pins, and a preflight-rebased v3 effect /// records its actual `own_head -> current_manifest_pin` compensation. -async fn prepare_v3_rollback_audit_plan( +async fn prepare_exact_rollback_audit_plan( root_uri: &str, storage: &dyn StorageAdapter, sidecar: &RecoverySidecar, states: &[ClassifiedTable], ) -> Result { let mut prepared = sidecar.clone(); - let protocol = prepared + let already_prepared = prepared .protocol_v3 - .as_mut() - .expect("caller checked protocol_v3"); - if protocol.rollback_audit_outcomes.is_some() { + .as_ref() + .and_then(|protocol| protocol.rollback_audit_outcomes.as_ref()) + .or_else(|| { + prepared + .protocol_v4 + .as_ref() + .and_then(|protocol| protocol.rollback_audit_outcomes.as_ref()) + }) + .is_some(); + if already_prepared { return Ok(prepared); } - protocol.rollback_audit_outcomes = Some(v3_rollback_audit_outcomes(sidecar, states)); + let outcomes = exact_rollback_audit_outcomes(sidecar, states); + if let Some(protocol) = prepared.protocol_v3.as_mut() { + protocol.rollback_audit_outcomes = Some(outcomes); + } else if let Some(protocol) = prepared.protocol_v4.as_mut() { + protocol.rollback_audit_outcomes = Some(outcomes); + } else { + return Err(OmniError::manifest_internal( + "prepare_exact_rollback_audit_plan called for a legacy sidecar", + )); + } let uri = sidecar_uri(root_uri, &prepared.operation_id); validate_sidecar_shape(&uri, &prepared)?; let json = serde_json::to_string_pretty(&prepared).map_err(|error| { OmniError::manifest_internal(format!( - "failed to serialize v3 rollback audit plan for sidecar '{}': {}", + "failed to serialize exact rollback audit plan for sidecar '{}': {}", prepared.operation_id, error )) })?; @@ -2632,12 +3725,12 @@ async fn roll_back_sidecar( // longer has pre-restore table observations. Persist the exact audit plan // after fork cleanup and before the first restore so that path can replay it // without fabricating outcomes from pins. - let prepared_v3 = if sidecar.protocol_v3.is_some() { - Some(prepare_v3_rollback_audit_plan(root_uri, storage, sidecar, states).await?) + let prepared_exact = if has_exact_protocol(sidecar) { + Some(prepare_exact_rollback_audit_plan(root_uri, storage, sidecar, states).await?) } else { None }; - let sidecar = prepared_v3.as_ref().unwrap_or(sidecar); + let sidecar = prepared_exact.as_ref().unwrap_or(sidecar); // Restore every drifted table (RolledPastExpected / UnexpectedAtP1 / // UnexpectedMultistep) to its manifest-pinned content, then PUBLISH so @@ -2658,7 +3751,7 @@ async fn roll_back_sidecar( let mut updates: Vec = Vec::with_capacity(sidecar.tables.len()); let mut expected: HashMap = HashMap::with_capacity(sidecar.tables.len()); for (pin, state) in sidecar.tables.iter().zip(states.iter()) { - if sidecar.protocol_v3.is_some() + if has_exact_protocol(sidecar) && !matches!( state.effect_ownership, EffectOwnership::OwnAtHead | EffectOwnership::OwnCompensatedAtHead @@ -2677,6 +3770,9 @@ async fn roll_back_sidecar( state.manifest_pinned, ) .await?; + crate::failpoints::maybe_fail( + crate::failpoints::names::RECOVERY_POST_TABLE_RESTORE_PRE_PUBLISH, + )?; } // Publish the post-restore HEAD (the restore commit we just made), // CAS against the current (unmoved) manifest pin — the same helper @@ -2722,6 +3818,12 @@ async fn roll_back_sidecar( .protocol_v3 .as_ref() .and_then(|protocol| protocol.rollback_audit_outcomes.clone()) + .or_else(|| { + sidecar + .protocol_v4 + .as_ref() + .and_then(|protocol| protocol.rollback_audit_outcomes.clone()) + }) .unwrap_or(outcomes); record_audit( root_uri, @@ -2789,16 +3891,16 @@ async fn record_audit_recovery_rollforward( Ok(()) } -/// Finalize a v3 sidecar whose tables are already manifest-aligned. +/// Finalize an exact-protocol sidecar whose tables are already manifest-aligned. /// /// Numeric table alignment alone is not evidence of WHICH outcome happened: /// the original manifest CAS may have succeeded before sidecar deletion, or a /// prior full recovery may have rolled the effects back and then failed during -/// audit/delete. v3 gives both outcomes fixed graph-commit ids, so inspect the +/// audit/delete. v3/v4 give both outcomes fixed graph-commit ids, so inspect the /// authoritative lineage and finish exactly that outcome without publishing a /// second synthetic commit. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum VisibleV3Outcome { +enum VisibleExactOutcome { Original, RolledBack, } @@ -2806,7 +3908,7 @@ enum VisibleV3Outcome { async fn detect_visible_v3_outcome( root_uri: &str, sidecar: &RecoverySidecar, -) -> Result> { +) -> Result> { let protocol = sidecar .protocol_v3 .as_ref() @@ -2871,8 +3973,8 @@ async fn detect_visible_v3_outcome( }; match (original_visible, rollback_visible) { - (true, false) => Ok(Some(VisibleV3Outcome::Original)), - (false, true) => Ok(Some(VisibleV3Outcome::RolledBack)), + (true, false) => Ok(Some(VisibleExactOutcome::Original)), + (false, true) => Ok(Some(VisibleExactOutcome::RolledBack)), (false, false) => Ok(None), (true, true) => Err(OmniError::manifest_internal(format!( "OCC recovery sidecar '{}' has both original commit '{}' and rollback commit '{}' \ @@ -2888,7 +3990,7 @@ async fn finalize_visible_v3_outcome( root_uri: &str, storage: &dyn StorageAdapter, sidecar: &RecoverySidecar, - outcome: VisibleV3Outcome, + outcome: VisibleExactOutcome, ) -> Result { let protocol = sidecar .protocol_v3 @@ -2896,7 +3998,7 @@ async fn finalize_visible_v3_outcome( .expect("caller checked protocol_v3"); let (kind, graph_commit_id, outcomes) = match outcome { - VisibleV3Outcome::Original => { + VisibleExactOutcome::Original => { let outcomes = sidecar .tables .iter() @@ -2921,7 +4023,7 @@ async fn finalize_visible_v3_outcome( outcomes, ) } - VisibleV3Outcome::RolledBack => { + VisibleExactOutcome::RolledBack => { let outcomes = protocol.rollback_audit_outcomes.clone().ok_or_else(|| { OmniError::manifest_internal(format!( "OCC recovery sidecar '{}' has visible fixed rollback commit '{}' but no \ @@ -2963,6 +4065,158 @@ async fn finalize_visible_v3_outcome( Ok(true) } +async fn detect_visible_v4_outcome( + root_uri: &str, + sidecar: &RecoverySidecar, +) -> Result> { + let protocol = sidecar + .protocol_v4 + .as_ref() + .expect("caller checked protocol_v4"); + let (commits, _) = + ManifestCoordinator::read_graph_lineage_at(root_uri, sidecar.branch.as_deref()).await?; + let original_commit = commits + .iter() + .find(|commit| commit.graph_commit_id == protocol.lineage.graph_commit_id); + let rollback_visible = commits + .iter() + .any(|commit| commit.graph_commit_id == protocol.rollback_graph_commit_id); + let original_visible = if let Some(commit) = original_commit { + let expected_branch = protocol + .lineage + .branch + .as_deref() + .filter(|branch| *branch != "main"); + if commit.manifest_branch.as_deref() != expected_branch + || protocol + .authority + .graph_head + .as_ref() + .is_some_and(|head| commit.parent_commit_id.as_ref() != Some(head)) + || commit.merged_parent_commit_id != protocol.lineage.merged_parent_commit_id + || commit.actor_id != protocol.lineage.actor_id + || commit.created_at != protocol.lineage.created_at + { + return Err(OmniError::manifest_internal(format!( + "BranchMerge recovery sidecar '{}' found original commit id '{}' with mismatched lineage", + sidecar.operation_id, protocol.lineage.graph_commit_id + ))); + } + let committed_snapshot = ManifestCoordinator::snapshot_at( + root_uri, + sidecar.branch.as_deref(), + commit.manifest_version, + ) + .await?; + let delta_matches = protocol.intended_delta.table_updates.iter().all(|slot| { + let Some(confirmed) = slot.confirmed.as_ref() else { + return false; + }; + committed_snapshot + .entry(&slot.table_key) + .is_some_and(|entry| { + entry.table_version == confirmed.table_version + && entry.table_branch == confirmed.table_branch + && entry.row_count == confirmed.row_count + && entry.version_metadata == confirmed.version_metadata + }) + }); + if !delta_matches { + return Err(OmniError::manifest_internal(format!( + "BranchMerge recovery sidecar '{}' found original commit id '{}' but its exact manifest delta differs", + sidecar.operation_id, protocol.lineage.graph_commit_id + ))); + } + true + } else { + false + }; + + match (original_visible, rollback_visible) { + (true, false) => Ok(Some(VisibleExactOutcome::Original)), + (false, true) => Ok(Some(VisibleExactOutcome::RolledBack)), + (false, false) => Ok(None), + (true, true) => Err(OmniError::manifest_internal(format!( + "BranchMerge recovery sidecar '{}' has both original commit '{}' and rollback commit '{}' visible; refusing ambiguous finalization", + sidecar.operation_id, + protocol.lineage.graph_commit_id, + protocol.rollback_graph_commit_id + ))), + } +} + +async fn finalize_visible_v4_outcome( + root_uri: &str, + storage: &dyn StorageAdapter, + sidecar: &RecoverySidecar, + outcome: VisibleExactOutcome, +) -> Result { + let protocol = sidecar + .protocol_v4 + .as_ref() + .expect("caller checked protocol_v4"); + let (kind, graph_commit_id, outcomes) = match outcome { + VisibleExactOutcome::Original => { + let outcomes = protocol + .intended_delta + .table_updates + .iter() + .map(|slot| { + let confirmed = slot + .confirmed + .as_ref() + .expect("visible v4 original requires confirmed delta"); + TableOutcome { + table_key: slot.table_key.clone(), + from_version: slot.expected_version, + to_version: confirmed.table_version, + } + }) + .collect(); + ( + RecoveryKind::RolledForward, + protocol.lineage.graph_commit_id.clone(), + outcomes, + ) + } + VisibleExactOutcome::RolledBack => { + let outcomes = protocol.rollback_audit_outcomes.clone().ok_or_else(|| { + OmniError::manifest_internal(format!( + "BranchMerge recovery sidecar '{}' has visible fixed rollback commit '{}' but no durable rollback audit outcomes", + sidecar.operation_id, protocol.rollback_graph_commit_id + )) + })?; + ( + RecoveryKind::RolledBack, + protocol.rollback_graph_commit_id.clone(), + outcomes, + ) + } + }; + + let mut audit = RecoveryAudit::open(root_uri).await?; + let already_recorded = + audit.list().await?.iter().any(|record| { + record.operation_id == sidecar.operation_id && record.recovery_kind == kind + }); + if !already_recorded { + crate::failpoints::maybe_fail(crate::failpoints::names::RECOVERY_RECORD_AUDIT)?; + audit + .append(RecoveryAuditRecord { + graph_commit_id, + recovery_kind: kind, + recovery_for_actor: sidecar.actor_id.clone(), + operation_id: sidecar.operation_id.clone(), + sidecar_writer_kind: format!("{:?}", sidecar.writer_kind), + per_table_outcomes: outcomes, + created_at: crate::db::now_micros()?, + }) + .await?; + } + delete_sidecar_by_operation_id(root_uri, storage, &sidecar.operation_id).await?; + Ok(true) +} + /// Atomically extend every table's manifest pin from `expected_version` to /// `post_commit_pin` via a single `ManifestBatchPublisher::publish` call. /// Returns the new manifest version produced by the publish. @@ -3450,6 +4704,7 @@ pub(crate) fn new_sidecar( additional_registrations: Vec::new(), tombstones: Vec::new(), protocol_v3: None, + protocol_v4: None, } } @@ -3514,7 +4769,7 @@ pub(crate) fn new_occ_sidecar( }) .collect(); let sidecar = RecoverySidecar { - schema_version: SIDECAR_SCHEMA_VERSION, + schema_version: MUTATION_LOAD_SIDECAR_SCHEMA_VERSION, operation_id, started_at, branch, @@ -3537,6 +4792,7 @@ pub(crate) fn new_occ_sidecar( tombstones: Vec::new(), }, }), + protocol_v4: None, }; validate_sidecar_shape("", &sidecar)?; Ok(sidecar) @@ -3675,6 +4931,288 @@ pub(crate) async fn confirm_occ_sidecar_phase_b( Ok(()) } +/// Arm the RFC-022 BranchMerge recovery protocol. +/// +/// `tables` and `effects` describe only independently durable physical work; +/// `intended_delta.table_updates` is deliberately allowed to be a superset so +/// pointer-only manifest updates are recovered in the same exact graph commit. +/// Every physical output remains unconfirmed until +/// [`confirm_branch_merge_sidecar_phase_b`] atomically binds the full delta. +pub(crate) fn new_branch_merge_sidecar( + branch: Option, + actor_id: Option, + tables: Vec, + authority: RecoveryAuthorityToken, + lineage: RecoveryLineageIntent, + effects: Vec, + intended_delta: RecoveryManifestDelta, +) -> Result { + let operation_id = ulid::Ulid::new().to_string(); + let started_at = match std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH) { + Ok(duration) => duration.as_micros().to_string(), + Err(_) => "0".to_string(), + }; + let sidecar = RecoverySidecar { + schema_version: BRANCH_MERGE_SIDECAR_SCHEMA_VERSION, + operation_id, + started_at, + branch, + actor_id, + writer_kind: SidecarKind::BranchMerge, + tables, + merge_source_commit_id: None, + additional_registrations: Vec::new(), + tombstones: Vec::new(), + protocol_v3: None, + protocol_v4: Some(RecoveryProtocolV4 { + authority, + lineage, + rollback_graph_commit_id: ulid::Ulid::new().to_string(), + rollback_audit_outcomes: None, + effect_phase: RecoveryEffectPhase::Armed, + effects, + intended_delta, + }), + }; + validate_sidecar_shape("", &sidecar)?; + Ok(sidecar) +} + +/// Durably transition a v4 BranchMerge sidecar from Armed to +/// EffectsConfirmed. `updates` is the complete manifest output, including +/// pointer-only slots with no physical effect. First-touch physical effects +/// additionally supply the exact Lance target-ref identity minted during +/// creation; existing-ref effects must not appear in that map. +pub(crate) async fn confirm_branch_merge_sidecar_phase_b( + root_uri: &str, + storage: &dyn StorageAdapter, + sidecar: &mut RecoverySidecar, + updates: &[SubTableUpdate], + confirmed_ref_identifiers: &HashMap, +) -> Result<()> { + crate::failpoints::maybe_fail(crate::failpoints::names::RECOVERY_SIDECAR_CONFIRM)?; + let uri = sidecar_uri(root_uri, &sidecar.operation_id); + validate_sidecar_shape(&uri, sidecar)?; + let protocol = sidecar.protocol_v4.as_ref().ok_or_else(|| { + OmniError::manifest_internal( + "confirm_branch_merge_sidecar_phase_b requires a v4 BranchMerge sidecar", + ) + })?; + if protocol.effect_phase != RecoveryEffectPhase::Armed { + return Err(OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' is already EffectsConfirmed", + sidecar.operation_id + ))); + } + + let update_keys: HashSet<&str> = updates + .iter() + .map(|update| update.table_key.as_str()) + .collect(); + let delta_keys: HashSet<&str> = protocol + .intended_delta + .table_updates + .iter() + .map(|slot| slot.table_key.as_str()) + .collect(); + if update_keys.len() != updates.len() || update_keys != delta_keys { + return Err(OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' confirmation updates differ from its complete intended delta", + sidecar.operation_id + ))); + } + let first_touch_keys: HashSet<&str> = protocol + .effects + .iter() + .filter(|effect| effect.kind.source_fork_version().is_some()) + .map(|effect| effect.table_key.as_str()) + .collect(); + let confirmed_ref_keys: HashSet<&str> = confirmed_ref_identifiers + .keys() + .map(String::as_str) + .collect(); + if confirmed_ref_keys != first_touch_keys { + return Err(OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' confirmed target-ref set differs from its first-touch effects", + sidecar.operation_id + ))); + } + + for slot in &protocol.intended_delta.table_updates { + let update = updates + .iter() + .find(|update| update.table_key == slot.table_key) + .expect("update/delta key sets checked above"); + if update.table_branch != slot.table_branch { + return Err(OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' table '{}' achieved output branch {:?}, planned {:?}", + sidecar.operation_id, slot.table_key, update.table_branch, slot.table_branch + ))); + } + } + for effect in &protocol.effects { + let pin = sidecar + .tables + .iter() + .find(|pin| pin.table_key == effect.table_key) + .expect("v4 shape validated effect/pin key sets"); + let update = updates + .iter() + .find(|update| update.table_key == effect.table_key) + .expect("physical effect is a delta-slot subset"); + let observed = observe_branch_merge_target_ref(pin).await?.ok_or_else(|| { + OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' cannot confirm missing target ref for table '{}'", + sidecar.operation_id, effect.table_key + )) + })?; + if observed.version != update.table_version { + return Err(OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' table '{}' observed HEAD {}, achieved update says {}", + sidecar.operation_id, effect.table_key, observed.version, update.table_version + ))); + } + if let Some(source_fork_version) = effect.kind.source_fork_version() { + if observed.parent_version != Some(source_fork_version) { + return Err(OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' table '{}' target ref parent is {:?}, exact fork version is {}", + sidecar.operation_id, + effect.table_key, + observed.parent_version, + source_fork_version + ))); + } + let expected_identifier = confirmed_ref_identifiers + .get(&effect.table_key) + .expect("confirmed first-touch ref key set checked above"); + if &observed.branch_identifier != expected_identifier { + return Err(OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' table '{}' target ref identity differs from the writer's achieved identity", + sidecar.operation_id, effect.table_key + ))); + } + } + match &effect.kind { + RecoveryBranchMergeEffectKind::MultiCommitHead { + planned_transactions, + .. + } => { + let planned_final_version = planned_transactions + .last() + .expect("v4 shape validation requires a non-empty transaction chain") + .read_version + .checked_add(1) + .ok_or_else(|| { + OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' planned transaction chain for '{}' overflows its final output version", + sidecar.operation_id, effect.table_key + )) + })?; + if update.table_version < planned_final_version { + return Err(OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' multi-commit table '{}' stopped before its complete planned transaction chain ({} < {})", + sidecar.operation_id, + effect.table_key, + update.table_version, + planned_final_version + ))); + } + let proof = prove_branch_merge_multi_commit_effect( + &observed, + planned_transactions, + pin.expected_version, + &effect.table_key, + ) + .await?; + if proof.effect_ownership != EffectOwnership::OwnAtHead + || !proof.full_effect_at_head + || proof.unsafe_reason.is_some() + { + return Err(OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' cannot confirm unproven transaction chain for table '{}': {}", + sidecar.operation_id, + effect.table_key, + proof + .unsafe_reason + .as_deref() + .unwrap_or("the full exact effect is not at HEAD") + ))); + } + } + RecoveryBranchMergeEffectKind::RefOnlyFork { source_version, .. } => { + if update.table_version != *source_version { + return Err(OmniError::manifest_internal(format!( + "BranchMerge sidecar '{}' ref-only table '{}' achieved version {}, exact fork version {}", + sidecar.operation_id, + effect.table_key, + update.table_version, + source_version + ))); + } + } + } + } + + let mut confirmed = sidecar.clone(); + let confirmed_protocol = confirmed + .protocol_v4 + .as_mut() + .expect("validated v4 protocol"); + for slot in &mut confirmed_protocol.intended_delta.table_updates { + let update = updates + .iter() + .find(|update| update.table_key == slot.table_key) + .expect("update/delta key sets checked above"); + slot.confirmed = Some(RecoveryConfirmedTableUpdate { + table_version: update.table_version, + table_branch: update.table_branch.clone(), + row_count: update.row_count, + version_metadata: update.version_metadata.clone(), + }); + } + for effect in &mut confirmed_protocol.effects { + let update = updates + .iter() + .find(|update| update.table_key == effect.table_key) + .expect("physical effect is a delta-slot subset"); + let ref_identifier = confirmed_ref_identifiers.get(&effect.table_key).cloned(); + match &mut effect.kind { + RecoveryBranchMergeEffectKind::MultiCommitHead { + confirmed_version, + confirmed_branch_identifier, + .. + } => { + *confirmed_version = Some(update.table_version); + *confirmed_branch_identifier = ref_identifier; + } + RecoveryBranchMergeEffectKind::RefOnlyFork { + confirmed_branch_identifier, + .. + } => { + *confirmed_branch_identifier = ref_identifier; + } + } + } + for pin in &mut confirmed.tables { + let update = updates + .iter() + .find(|update| update.table_key == pin.table_key) + .expect("effect/pin keys are delta-slot subset"); + pin.confirmed_version = Some(update.table_version); + } + confirmed_protocol.effect_phase = RecoveryEffectPhase::EffectsConfirmed; + + validate_sidecar_shape(&uri, &confirmed)?; + let json = serde_json::to_string_pretty(&confirmed).map_err(|error| { + OmniError::manifest_internal(format!( + "failed to serialize confirmed BranchMerge recovery sidecar: {error}" + )) + })?; + storage.write_text(&uri, &json).await?; + *sidecar = confirmed; + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -3750,6 +5288,15 @@ mod tests { .unwrap() } + fn test_branch_identifier( + version: u64, + suffix: &str, + ) -> lance::dataset::refs::BranchIdentifier { + lance::dataset::refs::BranchIdentifier { + version_mapping: vec![(version, suffix.to_string())], + } + } + #[test] fn sidecar_round_trips_through_json() { let original = new_sidecar( @@ -3772,7 +5319,10 @@ mod tests { #[test] fn occ_sidecar_round_trips_armed_with_stable_ids_and_exact_effect() { let original = occ_sidecar(); - assert_eq!(original.schema_version, SIDECAR_SCHEMA_VERSION); + assert_eq!( + original.schema_version, + MUTATION_LOAD_SIDECAR_SCHEMA_VERSION + ); let protocol = original.protocol_v3.as_ref().unwrap(); assert_eq!(protocol.effect_phase, RecoveryEffectPhase::Armed); assert_eq!(protocol.effects.len(), 1); @@ -3809,6 +5359,130 @@ mod tests { ); } + #[tokio::test] + async fn branch_merge_v4_arms_multi_commit_ref_only_and_pointer_slots() { + let root = "mem-root"; + let storage = ObjectStorageAdapter::in_memory(); + let target_identifier = test_branch_identifier(3, "target-incarnation"); + let lineage = RecoveryLineageIntent { + graph_commit_id: "01H000000000000000000000M1".to_string(), + branch: Some("target".to_string()), + actor_id: Some("act-merge".to_string()), + merged_parent_commit_id: Some("01H000000000000000000000S1".to_string()), + created_at: 456, + }; + let mut multi_pin = make_pin("node:Person", "memory://person", 5, 6); + multi_pin.table_branch = Some("target".to_string()); + let mut ref_pin = make_pin("node:Company", "memory://company", 4, 8); + ref_pin.table_branch = Some("target".to_string()); + let effects = vec![ + RecoveryBranchMergeEffect { + table_key: "node:Person".to_string(), + kind: RecoveryBranchMergeEffectKind::MultiCommitHead { + source_fork_version: Some(5), + planned_transactions: vec![ + transaction(5, "merge-person-append"), + transaction(6, "merge-person-delete"), + ], + confirmed_version: None, + confirmed_branch_identifier: None, + }, + }, + RecoveryBranchMergeEffect { + table_key: "node:Company".to_string(), + kind: RecoveryBranchMergeEffectKind::RefOnlyFork { + // Deliberately differs from the target manifest CAS pin (4): + // ref cleanup must use this exact SOURCE fork point. + source_version: 8, + confirmed_branch_identifier: None, + }, + }, + ]; + let intended_delta = RecoveryManifestDelta { + table_updates: vec![ + RecoveryTableUpdateSlot { + table_key: "node:Person".to_string(), + expected_version: 5, + table_branch: Some("target".to_string()), + confirmed: None, + }, + RecoveryTableUpdateSlot { + table_key: "node:Company".to_string(), + expected_version: 4, + table_branch: Some("target".to_string()), + confirmed: None, + }, + // No physical effect/pin: recovery must still publish this + // pointer-only output in the same fixed merge commit. + RecoveryTableUpdateSlot { + table_key: "edge:WorksAt".to_string(), + expected_version: 2, + table_branch: None, + confirmed: None, + }, + ], + registrations: Vec::new(), + tombstones: Vec::new(), + }; + let sidecar = new_branch_merge_sidecar( + Some("target".to_string()), + Some("act-merge".to_string()), + vec![multi_pin, ref_pin], + RecoveryAuthorityToken { + branch_identifier: target_identifier, + graph_head: Some("01H000000000000000000000T1".to_string()), + schema_ir_hash: "schema-hash".to_string(), + schema_identity_version: 1, + }, + lineage, + effects, + intended_delta, + ) + .unwrap(); + assert_eq!(sidecar.schema_version, BRANCH_MERGE_SIDECAR_SCHEMA_VERSION); + assert!(sidecar.protocol_v3.is_none()); + assert_eq!( + sidecar.protocol_v4.as_ref().unwrap().effect_phase, + RecoveryEffectPhase::Armed + ); + + let mut non_sequential = sidecar.clone(); + let RecoveryBranchMergeEffectKind::MultiCommitHead { + planned_transactions, + .. + } = &mut non_sequential.protocol_v4.as_mut().unwrap().effects[0].kind + else { + panic!("first effect must be multi-commit") + }; + planned_transactions[1].read_version = 9; + let error = validate_sidecar_shape("", &non_sequential).unwrap_err(); + assert!(error.to_string().contains("expected 6")); + + let mut mismatched_slot = sidecar.clone(); + mismatched_slot + .protocol_v4 + .as_mut() + .unwrap() + .intended_delta + .table_updates[0] + .expected_version = 4; + let error = validate_sidecar_shape("", &mismatched_slot).unwrap_err(); + assert!( + error + .to_string() + .contains("does not match its physical table pin") + ); + + write_sidecar(root, &storage, &sidecar).await.unwrap(); + + let listed = list_sidecars(root, &storage).await.unwrap(); + assert_eq!(listed.len(), 1); + assert_eq!( + listed[0].schema_version, + BRANCH_MERGE_SIDECAR_SCHEMA_VERSION + ); + } + #[test] fn parse_sidecar_refuses_v3_without_protocol_payload() { let mut value = serde_json::to_value(occ_sidecar()).unwrap(); @@ -4362,6 +6036,115 @@ mod tests { ); } + #[tokio::test] + async fn branch_merge_v4_proves_exact_chain_and_interrupted_compensation() { + let dir = tempfile::tempdir().unwrap(); + let uri = format!("{}/people.lance", dir.path().to_str().unwrap()); + let store = TableStore::new( + dir.path().to_str().unwrap(), + Arc::new(lance::session::Session::default()), + ); + let ds = TableStore::write_dataset(&uri, person_batch(&[("alice", Some(30))])) + .await + .unwrap(); + + let first_staged = store + .stage_append(&ds, person_batch(&[("bob", Some(25))]), &[]) + .await + .unwrap(); + let first_planned = first_staged.transaction_identity(); + let (after_first, first_committed) = store + .commit_staged_exact(Arc::new(ds), first_staged) + .await + .unwrap(); + assert_eq!(first_committed, first_planned); + + let second_staged = store + .stage_append(&after_first, person_batch(&[("carol", Some(40))]), &[]) + .await + .unwrap(); + let second_planned = second_staged.transaction_identity(); + let (after_second, second_committed) = store + .commit_staged_exact(Arc::new(after_first), second_staged) + .await + .unwrap(); + assert_eq!(second_committed, second_planned); + assert_eq!(after_second.version().version, 3); + + let observation = BranchMergeRefObservation { + dataset: after_second.clone(), + version: after_second.version().version, + branch_identifier: after_second.branch_identifier().await.unwrap(), + parent_version: None, + }; + let planned = vec![first_planned.clone(), second_planned.clone()]; + let proof = + prove_branch_merge_multi_commit_effect(&observation, &planned, 1, "node:Person") + .await + .unwrap(); + assert_eq!(proof.effect_ownership, EffectOwnership::OwnAtHead); + assert!(proof.full_effect_at_head); + assert!(proof.unsafe_reason.is_none()); + drop(observation); + drop(after_second); + + // Model Full recovery crashing after it appended Restore(v1), before + // it could publish that compensated HEAD through the manifest. + restore_table_to_version(&uri, None, 1).await.unwrap(); + let compensated = Dataset::open(&uri).await.unwrap(); + let observation = BranchMergeRefObservation { + version: compensated.version().version, + branch_identifier: compensated.branch_identifier().await.unwrap(), + parent_version: None, + dataset: compensated, + }; + let proof = + prove_branch_merge_multi_commit_effect(&observation, &planned, 1, "node:Person") + .await + .unwrap(); + assert_eq!( + proof.effect_ownership, + EffectOwnership::OwnCompensatedAtHead + ); + assert!(!proof.full_effect_at_head); + assert!(proof.unsafe_reason.is_none()); + } + + #[tokio::test] + async fn branch_merge_v4_refuses_unplanned_head_movement() { + let dir = tempfile::tempdir().unwrap(); + let uri = format!("{}/people.lance", dir.path().to_str().unwrap()); + let store = TableStore::new( + dir.path().to_str().unwrap(), + Arc::new(lance::session::Session::default()), + ); + let mut ds = TableStore::write_dataset(&uri, person_batch(&[("alice", Some(30))])) + .await + .unwrap(); + store + .append_batch(&uri, &mut ds, person_batch(&[("foreign", Some(99))])) + .await + .unwrap(); + let observation = BranchMergeRefObservation { + version: ds.version().version, + branch_identifier: ds.branch_identifier().await.unwrap(), + parent_version: None, + dataset: ds, + }; + + let proof = prove_branch_merge_multi_commit_effect( + &observation, + &[transaction(1, "planned-but-not-committed")], + 1, + "node:Person", + ) + .await + .unwrap(); + assert_eq!(proof.effect_ownership, EffectOwnership::Unverifiable); + assert!(!proof.full_effect_at_head); + assert!(proof.unsafe_reason.is_some()); + } + #[tokio::test] async fn exact_preflight_rebase_recovery_preserves_published_winner() { // Lance may preflight-rebase an Append even when CommitBuilder's retry diff --git a/crates/omnigraph/src/db/omnigraph.rs b/crates/omnigraph/src/db/omnigraph.rs index dc18e019..132c7d8d 100644 --- a/crates/omnigraph/src/db/omnigraph.rs +++ b/crates/omnigraph/src/db/omnigraph.rs @@ -131,6 +131,11 @@ pub(crate) struct WriteTxn { pub(crate) base: Snapshot, /// Complete coarse authority token for this prepared attempt. pub(crate) authority: WriteAuthorityToken, + /// Effective lineage head of the captured branch snapshot. Unlike + /// `authority.graph_head`, this includes an inherited head on a freshly + /// forked named branch whose materialized `graph_head:` row is + /// intentionally absent. + pub(crate) effective_graph_head: Option, /// Catalog built from the exact accepted IR whose identity is recorded in /// `authority`. Mutation/load planning and validation must use this snapshot, /// never the handle-global catalog, which can lag a schema apply performed by @@ -894,7 +899,7 @@ impl Omnigraph { .await?; let (schema_ir, schema_state) = load_validated_schema_contract(self.uri(), Arc::clone(&self.storage)).await?; - let (branch_identifier, graph_head, snapshot) = self + let (branch_identifier, graph_head, effective_graph_head, snapshot) = self .write_authority_for_known_branch(branch.as_deref(), true) .await?; self.ensure_schema_apply_not_locked("write preparation") @@ -918,6 +923,7 @@ impl Omnigraph { schema_ir_hash: schema_state.schema_ir_hash, schema_identity_version: schema_state.schema_identity_version, }, + effective_graph_head, catalog: Arc::new(catalog), }); } @@ -972,6 +978,7 @@ impl Omnigraph { ) -> Result<( lance::dataset::refs::BranchIdentifier, Option, + Option, Snapshot, )> { { @@ -987,6 +994,10 @@ impl Omnigraph { return Ok(( coord.branch_identifier().await?, coord.exact_graph_head(), + coord + .head_commit_id() + .await? + .map(|head| head.as_str().to_string()), coord.snapshot(), )); } @@ -997,6 +1008,10 @@ impl Omnigraph { Ok(( coord.branch_identifier().await?, coord.exact_graph_head(), + coord + .head_commit_id() + .await? + .map(|head| head.as_str().to_string()), coord.snapshot(), )) } @@ -1008,7 +1023,7 @@ impl Omnigraph { // Recheck the durable sentinel inside that critical section so a schema // apply observed after preparation cannot be followed by a table effect. self.ensure_schema_apply_not_locked("write commit").await?; - let (branch_identifier, graph_head, snapshot) = self + let (branch_identifier, graph_head, _effective_graph_head, snapshot) = self .write_authority_for_known_branch(txn.branch.as_deref(), true) .await?; let schema_state = self.ensure_schema_state_valid().await?; @@ -1969,17 +1984,6 @@ impl Omnigraph { normalize_branch_name(branch) } - pub(crate) async fn head_commit_id_for_branch( - &self, - branch: Option<&str>, - ) -> Result> { - let coordinator = self.open_coordinator_for_branch(branch).await?; - coordinator - .head_commit_id() - .await - .map(|id| id.map(|snapshot_id| snapshot_id.as_str().to_string())) - } - pub async fn branch_create(&self, name: &str) -> Result<()> { self.branch_create_as(name, None).await } @@ -2357,19 +2361,6 @@ impl Omnigraph { table_ops::commit_updates(self, updates).await } - /// Publish a branch merge: the merged table `updates` and the merge commit - /// in one manifest CAS (RFC-013 Phase 7). The merge commit's merged-in parent - /// is `merged_parent_commit_id` (the source head); its first parent is the - /// live target-branch head, resolved by the publisher. - pub(crate) async fn commit_merge_with_actor( - &self, - updates: &[crate::db::SubTableUpdate], - merged_parent_commit_id: &str, - actor_id: Option<&str>, - ) -> Result { - table_ops::commit_merge_with_actor(self, updates, merged_parent_commit_id, actor_id).await - } - pub(crate) async fn commit_updates_on_branch_with_expected( &self, branch: Option<&str>, diff --git a/crates/omnigraph/src/db/omnigraph/table_ops.rs b/crates/omnigraph/src/db/omnigraph/table_ops.rs index 7fb85cd3..70cc9f1b 100644 --- a/crates/omnigraph/src/db/omnigraph/table_ops.rs +++ b/crates/omnigraph/src/db/omnigraph/table_ops.rs @@ -1460,20 +1460,6 @@ pub(super) async fn commit_updates( commit_prepared_updates(db, &prepared, None).await } -pub(super) async fn commit_merge_with_actor( - db: &Omnigraph, - updates: &[crate::db::SubTableUpdate], - merged_parent_commit_id: &str, - actor_id: Option<&str>, -) -> Result { - db.coordinator - .write() - .await - .commit_merge_with_actor(updates, merged_parent_commit_id, actor_id) - .await - .map(|snapshot_id| snapshot_id.as_str().to_string()) -} - /// Commit updates with a publisher-level OCC fence. The /// `expected_table_versions` map asserts the manifest's pre-write per-table /// versions; mismatches surface as `ManifestConflictDetails::ExpectedVersionMismatch`. diff --git a/crates/omnigraph/src/exec/merge.rs b/crates/omnigraph/src/exec/merge.rs index 338047aa..01a3dc92 100644 --- a/crates/omnigraph/src/exec/merge.rs +++ b/crates/omnigraph/src/exec/merge.rs @@ -918,14 +918,84 @@ async fn publish_adopted_source_state( } } +fn pre_minted_merge_transaction( + read_version: u64, +) -> crate::table_store::StagedTransactionIdentity { + crate::table_store::StagedTransactionIdentity { + read_version, + uuid: format!("omnigraph-merge-{}", ulid::Ulid::new()), + } +} + +fn plan_merge_transactions( + table_key: &str, + candidate: &CandidateTableState, + first_read_version: u64, +) -> Result> { + let transaction_count = match candidate { + CandidateTableState::RewriteMerged(staged) => { + usize::from(staged.delta_staged.is_some()) + usize::from(!staged.deleted_ids.is_empty()) + } + CandidateTableState::AdoptWithDelta(delta) => { + usize::from(delta.appends.is_some()) + + usize::from(delta.upserts.is_some()) + + usize::from(!delta.deleted_ids.is_empty()) + } + CandidateTableState::AdoptSourceState { .. } => 0, + }; + if transaction_count == 0 { + return Err(OmniError::manifest_internal(format!( + "HEAD-advancing branch merge candidate '{table_key}' has no logical data transaction" + ))); + } + + (0..transaction_count) + .map(|offset| { + let offset = u64::try_from(offset).map_err(|_| { + OmniError::manifest_internal(format!( + "branch merge transaction count for '{table_key}' exceeds u64" + )) + })?; + let read_version = first_read_version.checked_add(offset).ok_or_else(|| { + OmniError::manifest_internal(format!( + "branch merge transaction version overflow for '{table_key}'" + )) + })?; + Ok(pre_minted_merge_transaction(read_version)) + }) + .collect() +} + +async fn commit_exact_merge_stage( + target_db: &Omnigraph, + current: SnapshotHandle, + mut staged: crate::storage_layer::StagedHandle, + planned: &crate::table_store::StagedTransactionIdentity, +) -> Result { + staged.bind_transaction_identity(planned)?; + let outcome = target_db + .storage() + .commit_staged_exact(current, staged) + .await?; + if !outcome.is_exact() { + return Err(OmniError::manifest_read_set_changed( + "branch_merge_lance_transaction", + Some(format!("{:?}", outcome.planned_transaction())), + Some(format!("{:?}", outcome.committed_transaction())), + )); + } + Ok(outcome.into_snapshot()) +} + async fn publish_rewritten_merge_table( target_db: &Omnigraph, table_key: &str, staged: &StagedMergeResult, + planned_transactions: &[crate::table_store::StagedTransactionIdentity], ) -> Result { // Branch merge's source-rewrite path is Merge-shaped (upsert from // source onto target). The staged delete later in this function - // (`stage_delete` + `commit_staged`) operates on rows the rewrite chose + // (`stage_delete` + an exact commit) operates on rows the rewrite chose // to remove, not user-facing predicates, so Merge is the correct policy // here. // `open_for_mutation` is the no-txn entry, so collapse #1's non-strict @@ -939,12 +1009,11 @@ async fn publish_rewritten_merge_table( // Phase 1: merge_insert changed/new rows (preserves _row_created_at_version for // existing rows, bumps _row_last_updated_at_version only for actually-changed rows). // - // Routed through the staged primitive so a failure between writing - // fragments and committing leaves no Lance-HEAD drift. The - // commit_staged here is per-table per-call (Lance has no - // multi-dataset atomic commit); the residual sits at this single - // commit point, narrowed from the previous "merge_insert + delete + - // index" multi-step inline-commit chain. + // Routed through the staged primitive with the transaction identity armed + // in the v4 sidecar and transparent Lance conflict retries disabled. A + // failure between writing fragments and committing leaves no Lance-HEAD + // drift; a failure after the exact commit remains recovery-owned. + let mut planned_transactions = planned_transactions.iter(); if let Some(delta) = &staged.delta_staged { // The staged delta dataset is a temp-dir Lance dataset used only // to collect the rewrite batches; wrap it in a `SnapshotHandle` @@ -976,10 +1045,13 @@ async fn publish_rewritten_merge_table( lance::dataset::WhenNotMatched::InsertAll, ) .await?; - current_ds = target_db - .storage() - .commit_staged(current_ds, staged_merge) - .await?; + let planned = planned_transactions.next().ok_or_else(|| { + OmniError::manifest_internal(format!( + "branch merge table '{table_key}' has no planned merge transaction" + )) + })?; + current_ds = + commit_exact_merge_stage(target_db, current_ds, staged_merge, planned).await?; } } @@ -988,10 +1060,12 @@ async fn publish_rewritten_merge_table( // rows are on Lance HEAD but the delete has not committed and the // achieved-version intent has not been recorded, so recovery must roll BACK. // See tests/failpoints.rs::branch_merge_rewrite_partial_after_merge_rolls_back. - crate::failpoints::maybe_fail(crate::failpoints::names::BRANCH_MERGE_REWRITE_AFTER_MERGE_PRE_DELETE)?; + crate::failpoints::maybe_fail( + crate::failpoints::names::BRANCH_MERGE_REWRITE_AFTER_MERGE_PRE_DELETE, + )?; // Phase 2: delete removed rows via deletion vectors, staged through - // `stage_delete` + `commit_staged` (MR-A — Lance 7.0's + // `stage_delete` + an exact pre-minted commit (MR-A — Lance 7.0's // `DeleteBuilder::execute_uncommitted`, #6658, made delete a two-phase // staged write, so this no longer inline-commits). if !staged.deleted_ids.is_empty() { @@ -1001,27 +1075,45 @@ async fn publish_rewritten_merge_table( .map(|id| format!("'{}'", id.replace('\'', "''"))) .collect(); let filter = format!("id IN ({})", escaped.join(", ")); - if let Some(staged_delete) = target_db.storage().stage_delete(¤t_ds, &filter).await? { - current_ds = target_db - .storage() - .commit_staged(current_ds, staged_delete) - .await?; + if let Some(staged_delete) = target_db + .storage() + .stage_delete(¤t_ds, &filter) + .await? + { + let planned = planned_transactions.next().ok_or_else(|| { + OmniError::manifest_internal(format!( + "branch merge table '{table_key}' has no planned delete transaction" + )) + })?; + current_ds = + commit_exact_merge_stage(target_db, current_ds, staged_delete, planned).await?; } } + if let Some(unused) = planned_transactions.next() { + return Err(OmniError::manifest_internal(format!( + "branch merge table '{table_key}' did not apply planned transaction {unused:?}" + ))); + } + // Failpoint: crash after the Phase 2 delete commit, before the index build. // Models a partial Phase B on the three-way path — constructive rows + // deletes are on Lance HEAD but the achieved-version intent has not been // recorded, so recovery must roll BACK (the index is reconciler-owned derived // state, but the merge itself never reached its commit boundary). See // tests/failpoints.rs::branch_merge_rewrite_partial_after_delete_rolls_back. - crate::failpoints::maybe_fail(crate::failpoints::names::BRANCH_MERGE_REWRITE_AFTER_DELETE_PRE_INDEX)?; + crate::failpoints::maybe_fail( + crate::failpoints::names::BRANCH_MERGE_REWRITE_AFTER_DELETE_PRE_INDEX, + )?; // Phase 3: rebuild indices. // // `build_indices_on_dataset` uses `stage_create_btree_index` / - // `stage_create_inverted_index` + `commit_staged` for scalar - // indices. Vector indices remain inline-commit + // `stage_create_inverted_index` + `commit_staged` for scalar indices. + // These are rebuildable `CreateIndex` tail transactions, not part of the + // merge's logical pre-minted data chain; Armed v4 recovery accepts them + // only after that complete exact chain and discards them with a rollback. + // Vector indices remain inline-commit // (`build_index_metadata_from_segments` is `pub(crate)` in lance- // 6.0.1 — companion ticket to lance-format/lance#6666). let row_count = target_db @@ -1095,6 +1187,7 @@ async fn publish_adopted_delta( target_db: &Omnigraph, table_key: &str, delta: &AdoptDelta, + planned_transactions: &[crate::table_store::StagedTransactionIdentity], ) -> Result { // `open_for_mutation` is the no-txn entry, so collapse #1's non-strict // open-skip (gated on `txn.is_some()`) never fires here — the handle is @@ -1115,16 +1208,19 @@ async fn publish_adopted_delta( // through the staged primitive so a failure between writing fragments and // committing leaves no Lance-HEAD drift. `appends` is `Some` only when the // staged table is non-empty (`compute_adopt_delta`). + let mut planned_transactions = planned_transactions.iter(); if let Some(append_table) = &delta.appends { let source = SnapshotHandle::new(append_table.dataset.clone()); let staged = target_db .storage() .stage_append_stream(¤t_ds, &source, &[]) .await?; - current_ds = target_db - .storage() - .commit_staged(current_ds, staged) - .await?; + let planned = planned_transactions.next().ok_or_else(|| { + OmniError::manifest_internal(format!( + "branch merge table '{table_key}' has no planned append transaction" + )) + })?; + current_ds = commit_exact_merge_stage(target_db, current_ds, staged, planned).await?; } // Failpoint: crash after the Phase 1a append commit, before the upsert. @@ -1132,14 +1228,17 @@ async fn publish_adopted_delta( // have not committed and the achieved-version intent has not been recorded, so // recovery must roll BACK (not publish the appends-only state). See // tests/failpoints.rs::branch_merge_adopt_partial_after_append_rolls_back. - crate::failpoints::maybe_fail(crate::failpoints::names::BRANCH_MERGE_ADOPT_AFTER_APPEND_PRE_UPSERT)?; + crate::failpoints::maybe_fail( + crate::failpoints::names::BRANCH_MERGE_ADOPT_AFTER_APPEND_PRE_UPSERT, + )?; // Phase 1b: upsert the CHANGED rows. The merge_insert hash join is now // bounded to the genuinely-changed set, not the whole delta. It runs against // the committed view that already includes the appends; the changed ids are // disjoint from the appended ids (each id is classified into exactly one of // new / changed / deleted / unchanged in the single ordered walk), so the - // join never collides with an appended row. + // join never collides with an appended row. Every logical data step uses + // the next identity in the exact transaction chain armed before Phase B. if let Some(upsert_table) = &delta.upserts { if let Some(combined) = scan_staged_combined(target_db, upsert_table).await? { let staged_merge = target_db @@ -1152,10 +1251,13 @@ async fn publish_adopted_delta( lance::dataset::WhenNotMatched::InsertAll, ) .await?; - current_ds = target_db - .storage() - .commit_staged(current_ds, staged_merge) - .await?; + let planned = planned_transactions.next().ok_or_else(|| { + OmniError::manifest_internal(format!( + "branch merge table '{table_key}' has no planned upsert transaction" + )) + })?; + current_ds = + commit_exact_merge_stage(target_db, current_ds, staged_merge, planned).await?; } } @@ -1164,10 +1266,13 @@ async fn publish_adopted_delta( // has not committed and the achieved-version intent has not been recorded, so // recovery must roll BACK. See // tests/failpoints.rs::branch_merge_adopt_partial_after_upsert_rolls_back. - crate::failpoints::maybe_fail(crate::failpoints::names::BRANCH_MERGE_ADOPT_AFTER_UPSERT_PRE_DELETE)?; + crate::failpoints::maybe_fail( + crate::failpoints::names::BRANCH_MERGE_ADOPT_AFTER_UPSERT_PRE_DELETE, + )?; // Phase 2: delete removed rows via deletion vectors, staged through - // `stage_delete` + `commit_staged` (same as the three-way path; MR-A). + // `stage_delete` + an exact pre-minted commit (same as the three-way path; + // MR-A). if !delta.deleted_ids.is_empty() { let escaped: Vec = delta .deleted_ids @@ -1175,14 +1280,27 @@ async fn publish_adopted_delta( .map(|id| format!("'{}'", id.replace('\'', "''"))) .collect(); let filter = format!("id IN ({})", escaped.join(", ")); - if let Some(staged_delete) = target_db.storage().stage_delete(¤t_ds, &filter).await? { - current_ds = target_db - .storage() - .commit_staged(current_ds, staged_delete) - .await?; + if let Some(staged_delete) = target_db + .storage() + .stage_delete(¤t_ds, &filter) + .await? + { + let planned = planned_transactions.next().ok_or_else(|| { + OmniError::manifest_internal(format!( + "branch merge table '{table_key}' has no planned delete transaction" + )) + })?; + current_ds = + commit_exact_merge_stage(target_db, current_ds, staged_delete, planned).await?; } } + if let Some(unused) = planned_transactions.next() { + return Err(OmniError::manifest_internal(format!( + "branch merge table '{table_key}' did not apply planned transaction {unused:?}" + ))); + } + // Phase 4: index coverage is reconciler-owned on the adopt path. Unlike the // three-way `RewriteMerged` path, this does NOT build indices inline: the // appended/upserted rows are left uncovered (reads stay correct via @@ -1273,32 +1391,37 @@ impl Omnigraph { .write_queue() .acquire_branches(&[source_branch.clone(), target_branch.clone()]) .await; - self.ensure_no_pending_recovery_sidecars_under_gates( - &relevant_branches, - "branch_merge", - ) - .await?; + self.ensure_no_pending_recovery_sidecars_under_gates(&relevant_branches, "branch_merge") + .await?; self.refresh_coordinator_only().await?; self.ensure_schema_apply_not_locked("branch_merge").await?; - let merge_catalog = self - .load_accepted_catalog_with_schema_gate_held() - .await?; - - let source_head_commit_id = self - .head_commit_id_for_branch(source_branch.as_deref()) - .await? + // Capture each branch as one coherent RFC-022 authority token plus + // immutable snapshot. The target token is the coarse publish read set; + // the source token pins the exact merge input without requiring the + // source head to remain latest until the target CAS. + let source_txn = self.open_write_txn(source_branch.as_deref()).await?; + let target_txn = self.open_write_txn(target_branch.as_deref()).await?; + let source_head_commit_id = source_txn + .effective_graph_head + .clone() .ok_or_else(|| OmniError::manifest("source branch has no head commit".to_string()))?; - let target_head_commit_id = self - .head_commit_id_for_branch(target_branch.as_deref()) - .await? + let target_head_commit_id = target_txn + .effective_graph_head + .clone() .ok_or_else(|| OmniError::manifest("target branch has no head commit".to_string()))?; - let base_commit = CommitGraph::merge_base( + let base_commit = CommitGraph::merge_base_between( self.uri(), source_branch.as_deref(), target_branch.as_deref(), + &source_head_commit_id, + &target_head_commit_id, ) .await? - .ok_or_else(|| OmniError::manifest("branches have no common ancestor".to_string()))?; + .ok_or_else(|| { + OmniError::manifest( + "captured branch commits are unavailable or have no common ancestor".to_string(), + ) + })?; if source_head_commit_id == target_head_commit_id || base_commit.graph_commit_id == source_head_commit_id @@ -1313,18 +1436,6 @@ impl Omnigraph { base_commit.manifest_version, ) .await?; - let source_snapshot = self - .resolved_target(ReadTarget::Branch( - source_branch.clone().unwrap_or_else(|| "main".to_string()), - )) - .await? - .snapshot; - let target_snapshot = self - .resolved_target(ReadTarget::Branch( - target_branch.clone().unwrap_or_else(|| "main".to_string()), - )) - .await? - .snapshot; crate::failpoints::maybe_fail( crate::failpoints::names::BRANCH_MERGE_POST_AUTHORITY_CAPTURE, )?; @@ -1345,9 +1456,8 @@ impl Omnigraph { let merge_result = self .branch_merge_on_current_target( &base_snapshot, - &source_snapshot, - &target_snapshot, - merge_catalog.as_ref(), + &source_txn, + &target_txn, source_branch.as_deref(), target_branch.as_deref(), &target_head_commit_id, @@ -1377,7 +1487,7 @@ impl Omnigraph { // Use `refresh_coordinator_only` rather than `refresh` so the // recovery sweep doesn't race the merge's own in-flight // sidecar: when the merge body returns Err between Phase B - // (per-table `commit_staged` + sidecar write) and Phase C + // (per-table exact commits + sidecar confirmation) and Phase C // (manifest publish + sidecar delete), the sidecar is still on // disk. `refresh`'s `RollForwardOnly` sweep would observe it // and close it here — masking the failure from the next @@ -1411,9 +1521,8 @@ impl Omnigraph { async fn branch_merge_on_current_target( &self, base_snapshot: &Snapshot, - source_snapshot: &Snapshot, - target_snapshot: &Snapshot, - catalog: &Catalog, + source_txn: &WriteTxn, + target_txn: &WriteTxn, source_branch: Option<&str>, target_branch: Option<&str>, target_head_commit_id: &str, @@ -1421,6 +1530,9 @@ impl Omnigraph { is_fast_forward: bool, actor_id: Option<&str>, ) -> Result { + let source_snapshot = &source_txn.base; + let target_snapshot = &target_txn.base; + let catalog = target_txn.catalog.as_ref(); let mut table_keys = HashSet::new(); for entry in base_snapshot.entries() { table_keys.insert(entry.table_key.clone()); @@ -1486,25 +1598,14 @@ impl Omnigraph { let changeset = build_merge_changeset(self, catalog, &candidates).await?; validate_merge_candidates(catalog, target_snapshot, &changeset).await?; - // Recovery sidecar: protect the per-table commit_staged loop. - // Pin `RewriteMerged` and `AdoptWithDelta` candidates — both advance - // Lance HEAD before the manifest publish (RewriteMerged via - // publish_rewritten_merge_table; AdoptWithDelta via publish_adopted_delta: - // stage_append + stage_merge_insert + stage_delete + index — multiple - // commit_staged calls per table, which the loose classification handles - // as multi-step drift). - // - // `AdoptSourceState` candidates are NOT pinned: their publish - // (`publish_adopted_source_state`) is a pure pointer switch or a fork - // (`fork_dataset_from_entry_state` only adds a Lance branch ref), neither - // of which advances the data HEAD. Pinning them would classify as - // NoMovement and force an all-or-nothing rollback that destroys sibling - // tables' committed work. - // - // The former gap — adopt subcases that applied a non-empty delta advanced - // HEAD unpinned — is closed: `classify_adopt` pre-computes the delta, so a - // HEAD-advancing adopt is `AdoptWithDelta` (pinned here) and an empty-delta - // adopt stays `AdoptSourceState`. + // Recovery sidecar: protect the complete physical effect set. Every + // `RewriteMerged` / `AdoptWithDelta` logical data step receives a + // pre-minted, sequential Lance transaction identity and commits with + // transparent retries disabled. Ref-only `AdoptSourceState` candidates + // have no data transaction, but a first-touch native target ref is still + // an independently durable v4 effect. Pointer-only manifest updates need + // no physical pin; they remain in the complete intended delta so a + // sibling effect's recovery publishes the merge atomically. // // This bridge still coexists with legacy maintenance writers that take // only `(table, branch)` queues. Acquire the conservative all-catalog @@ -1525,101 +1626,228 @@ impl Omnigraph { "branch_merge after acquiring source/target table gates", ) .await?; - let fresh_source_snapshot = self - .fresh_snapshot_for_branch_unchecked(source_branch) - .await?; let fresh_target_snapshot = self .fresh_snapshot_for_branch_unchecked(target_branch) .await?; - for (member, prepared, current) in [ - ("source", source_snapshot, &fresh_source_snapshot), - ("target", target_snapshot, &fresh_target_snapshot), - ] { - if prepared.version() != current.version() { - return Err(OmniError::manifest_read_set_changed( - format!( - "branch_merge_{member}:{}", - if member == "source" { - source_branch.unwrap_or("main") - } else { - target_branch.unwrap_or("main") - } - ), - Some(prepared.version().to_string()), - Some(current.version().to_string()), - )); + if target_snapshot.version() != fresh_target_snapshot.version() { + return Err(OmniError::manifest_read_set_changed( + format!("branch_merge_target:{}", target_branch.unwrap_or("main")), + Some(target_snapshot.version().to_string()), + Some(fresh_target_snapshot.version().to_string()), + )); + } + // Revalidate the complete target authority (branch incarnation, exact + // graph head, and schema identity), not only its numeric manifest + // version. This is the same coarse token mutation/load publish under. + self.revalidate_write_txn(target_txn).await?; + + // Source is an immutable input, not part of the target CAS. A later + // source-head advance is therefore harmless, but delete/recreate ABA is + // not: the captured snapshot must still belong to the same native ref + // incarnation and accepted schema contract. + let live_source = self.open_write_txn(source_branch).await?; + if live_source.authority.branch_identifier != source_txn.authority.branch_identifier { + return Err(OmniError::manifest_read_set_changed( + format!( + "branch_merge_source_incarnation:{}", + source_branch.unwrap_or("main") + ), + Some( + serde_json::to_string(&source_txn.authority.branch_identifier).map_err( + |error| { + OmniError::manifest_internal(format!( + "serialize captured source branch identifier: {error}" + )) + }, + )?, + ), + Some( + serde_json::to_string(&live_source.authority.branch_identifier).map_err( + |error| { + OmniError::manifest_internal(format!( + "serialize live source branch identifier: {error}" + )) + }, + )?, + ), + )); + } + if live_source.authority.schema_ir_hash != source_txn.authority.schema_ir_hash + || live_source.authority.schema_identity_version + != source_txn.authority.schema_identity_version + { + return Err(OmniError::manifest_read_set_changed( + "branch_merge_source_schema".to_string(), + Some(format!( + "{}:{}", + source_txn.authority.schema_identity_version, + source_txn.authority.schema_ir_hash + )), + Some(format!( + "{}:{}", + live_source.authority.schema_identity_version, + live_source.authority.schema_ir_hash + )), + )); + } + + let expected_versions = ordered_table_keys + .iter() + .map(|table_key| { + ( + table_key.clone(), + target_snapshot + .entry(table_key) + .map(|entry| entry.table_version) + .unwrap_or(0), + ) + }) + .collect::>(); + let mut merge_lineage = self + .new_lineage_intent_for_branch(target_branch, actor_id) + .await?; + merge_lineage.merged_parent_commit_id = Some(source_head_commit_id.to_string()); + + // Build the v4 recovery envelope. Physical pins are a subset of the + // complete intended manifest delta: pointer-only candidates have no + // pre-authority effect, but must still be replayed if a sibling table's + // durable effect forces recovery to finish the merge atomically. + let mut recovery_pins = Vec::new(); + let mut recovery_effects = Vec::new(); + let mut delta_slots = Vec::new(); + let mut first_touch_effects = HashSet::new(); + let mut planned_transactions_by_table = HashMap::new(); + for table_key in &ordered_table_keys { + let Some(candidate) = candidates.get(table_key) else { + continue; + }; + let source_entry = source_snapshot.entry(table_key).ok_or_else(|| { + OmniError::manifest(format!( + "branch merge candidate '{}' has no captured source entry", + table_key + )) + })?; + let target_entry = target_snapshot.entry(table_key); + let expected_version = target_entry.map(|entry| entry.table_version).unwrap_or(0); + let planned_output_branch = match candidate { + CandidateTableState::RewriteMerged(_) | CandidateTableState::AdoptWithDelta(_) => { + active_branch_for_keys.clone() + } + CandidateTableState::AdoptSourceState { .. } => { + match (target_branch, source_entry.table_branch.as_deref()) { + (Some(target), Some(_)) => Some(target.to_string()), + _ => None, + } + } + }; + delta_slots.push(crate::db::manifest::RecoveryTableUpdateSlot { + table_key: table_key.clone(), + expected_version, + table_branch: planned_output_branch, + confirmed: None, + }); + + match candidate { + CandidateTableState::RewriteMerged(_) | CandidateTableState::AdoptWithDelta(_) => { + let entry = target_entry.ok_or_else(|| { + OmniError::manifest(format!( + "HEAD-advancing branch merge candidate '{}' has no target entry", + table_key + )) + })?; + let source_fork_version = target_branch + .filter(|target| entry.table_branch.as_deref() != Some(*target)) + .map(|_| entry.table_version); + if source_fork_version.is_some() { + first_touch_effects.insert(table_key.clone()); + } + let planned_transactions = + plan_merge_transactions(table_key, candidate, expected_version)?; + planned_transactions_by_table + .insert(table_key.clone(), planned_transactions.clone()); + recovery_pins.push(crate::db::manifest::SidecarTablePin { + table_key: table_key.clone(), + table_path: self.storage().dataset_uri(&entry.table_path), + expected_version, + post_commit_pin: expected_version + 1, + confirmed_version: None, + table_branch: active_branch_for_keys.clone(), + }); + recovery_effects.push(crate::db::manifest::RecoveryBranchMergeEffect { + table_key: table_key.clone(), + kind: crate::db::manifest::RecoveryBranchMergeEffectKind::MultiCommitHead { + source_fork_version, + planned_transactions, + confirmed_version: None, + confirmed_branch_identifier: None, + }, + }); + } + CandidateTableState::AdoptSourceState { .. } => { + let Some(target) = target_branch else { + continue; + }; + let creates_target_ref = source_entry.table_branch.is_some() + && target_entry.and_then(|entry| entry.table_branch.as_deref()) + != Some(target); + if !creates_target_ref { + continue; + } + first_touch_effects.insert(table_key.clone()); + recovery_pins.push(crate::db::manifest::SidecarTablePin { + table_key: table_key.clone(), + table_path: self.storage().dataset_uri(&source_entry.table_path), + expected_version, + post_commit_pin: source_entry.table_version, + confirmed_version: None, + table_branch: Some(target.to_string()), + }); + recovery_effects.push(crate::db::manifest::RecoveryBranchMergeEffect { + table_key: table_key.clone(), + kind: crate::db::manifest::RecoveryBranchMergeEffectKind::RefOnlyFork { + source_version: source_entry.table_version, + confirmed_branch_identifier: None, + }, + }); + } } } - let recovery_pins: Vec = ordered_table_keys - .iter() - .filter_map(|table_key| { - let candidate = candidates.get(table_key)?; - if !matches!( - candidate, - CandidateTableState::RewriteMerged(_) | CandidateTableState::AdoptWithDelta(_) - ) { - return None; - } - let entry = target_snapshot.entry(table_key)?; - Some(crate::db::manifest::SidecarTablePin { - table_key: table_key.clone(), - table_path: self.storage().dataset_uri(&entry.table_path), - expected_version: entry.table_version, - post_commit_pin: entry.table_version + 1, - // Stamped after the whole per-table publish completes - // (Phase-B confirmation, just before the manifest publish). - // Until then `None` marks an unfinished publish that - // recovery must roll back, not roll forward. - confirmed_version: None, - // Use the merge target branch (where commits actually - // land), NOT entry.table_branch (where the table - // currently lives). publish_rewritten_merge_table calls - // open_for_mutation, which forks an inherited-from-main - // table to active_branch on first write — the resulting - // Lance commit lands on active_branch. Recovery's - // open_lance_head must check the same branch, otherwise - // an inherited-table feature-to-feature merge classifies - // as NoMovement and the all-or-nothing rollback skips - // the orphaned post-Phase-B HEAD on the target ref. - // Same rationale as table_ops.rs:115-120 in - // ensure_indices_for_branch. - table_branch: active_branch_for_keys.clone(), - }) - }) - .collect(); - // Keep the sidecar alongside its handle: after the per-table publish - // loop completes (Phase B), we re-write it with each table's confirmed - // version before the manifest publish, so recovery can tell a finished - // publish (roll forward) from a partial one (roll back). + // Keep the sidecar alongside its handle: after the whole physical + // effect set completes, confirmation binds every output slot and every + // first-touch ref identity before the one manifest CAS. let mut recovery: Option<( crate::db::manifest::RecoverySidecar, crate::db::manifest::RecoverySidecarHandle, )> = if recovery_pins.is_empty() { None } else { - // Use the merge target branch directly, NOT a heuristic - // derived from `ordered_table_keys.first()`. The first - // sorted table key may not be in the target snapshot at all - // (its `entry()` returns None → branch becomes None == main), - // and the SubTableEntry's `table_branch` field isn't - // necessarily the merge target branch. The caller - // `branch_merge` calls `swap_coordinator_for_branch(target_branch)` - // before invoking this function, so `self.active_branch()` - // is the target. - let target_branch = active_branch_for_keys.clone(); - let mut sidecar = crate::db::manifest::new_sidecar( - crate::db::manifest::SidecarKind::BranchMerge, - target_branch, + let authority = crate::db::manifest::RecoveryAuthorityToken { + branch_identifier: target_txn.authority.branch_identifier.clone(), + graph_head: target_txn.authority.graph_head.clone(), + schema_ir_hash: target_txn.authority.schema_ir_hash.clone(), + schema_identity_version: target_txn.authority.schema_identity_version, + }; + let recovery_lineage = crate::db::manifest::RecoveryLineageIntent { + graph_commit_id: merge_lineage.graph_commit_id.clone(), + branch: merge_lineage.branch.clone(), + actor_id: merge_lineage.actor_id.clone(), + merged_parent_commit_id: merge_lineage.merged_parent_commit_id.clone(), + created_at: merge_lineage.created_at, + }; + let sidecar = crate::db::manifest::new_branch_merge_sidecar( + active_branch_for_keys.clone(), actor_id.map(str::to_string), recovery_pins, - ); - // Carry the source branch's HEAD commit id so the recovery - // sweep's audit step can record this as a MERGE commit - // (linked to the source) instead of a plain commit. Without - // this, future merges between the same pair lose - // already-up-to-date detection and merge-base correctness. - sidecar.merge_source_commit_id = Some(source_head_commit_id.to_string()); + authority, + recovery_lineage, + recovery_effects, + crate::db::manifest::RecoveryManifestDelta { + table_updates: delta_slots, + registrations: Vec::new(), + tombstones: Vec::new(), + }, + )?; let handle = crate::db::manifest::write_sidecar( self.root_uri(), self.storage_adapter(), @@ -1629,72 +1857,134 @@ impl Omnigraph { Some((sidecar, handle)) }; - let mut updates = Vec::new(); - let mut changed_edge_tables = false; - for table_key in &ordered_table_keys { - let Some(candidate_state) = candidates.get(table_key) else { - continue; - }; - let update = match candidate_state { - CandidateTableState::AdoptSourceState { .. } => { - publish_adopted_source_state(self, source_snapshot, target_snapshot, table_key) - .await? - } - CandidateTableState::AdoptWithDelta(delta) => { - publish_adopted_delta(self, table_key, delta).await? - } - CandidateTableState::RewriteMerged(staged) => { - publish_rewritten_merge_table(self, table_key, staged).await? - } - }; - if table_key.starts_with("edge:") { - changed_edge_tables = true; + let recovery_operation_id = recovery + .as_ref() + .map(|(_, handle)| handle.operation_id.clone()); + let post_arm_result = async { + if recovery.is_some() && !first_touch_effects.is_empty() { + crate::failpoints::maybe_fail( + crate::failpoints::names::BRANCH_MERGE_POST_SIDECAR_PRE_FORK, + )?; } - updates.push(update); - } - // Phase-B confirmation: every table's publish finished, so stamp the - // sidecar with each table's exact achieved version before the manifest - // publish. This is the commit point of the recovery WAL: a crash from - // here on rolls FORWARD to these versions, while a crash anywhere in the - // publish loop above left the sidecar unconfirmed and rolls BACK. The - // `updates` carry the real per-table final versions (multiple - // commit_staged calls per table, so not derivable from `post_commit_pin` - // alone). A failure here leaves the unconfirmed sidecar → roll back. - crate::failpoints::maybe_fail( - crate::failpoints::names::BRANCH_MERGE_POST_EFFECTS_PRE_CONFIRM, - )?; - if let Some((sidecar, _)) = recovery.as_mut() { - let confirmed_versions: std::collections::HashMap = updates - .iter() - .map(|u| (u.table_key.clone(), u.table_version)) - .collect(); - crate::db::manifest::confirm_sidecar_phase_b( - self.root_uri(), - self.storage_adapter(), - sidecar, - &confirmed_versions, + let mut updates = Vec::new(); + let mut changed_edge_tables = false; + let mut confirmed_ref_identifiers = HashMap::new(); + for table_key in &ordered_table_keys { + let Some(candidate_state) = candidates.get(table_key) else { + continue; + }; + let update = match candidate_state { + CandidateTableState::AdoptSourceState { .. } => { + publish_adopted_source_state( + self, + source_snapshot, + target_snapshot, + table_key, + ) + .await? + } + CandidateTableState::AdoptWithDelta(delta) => { + let planned = planned_transactions_by_table.get(table_key).ok_or_else(|| { + OmniError::manifest_internal(format!( + "branch merge table '{table_key}' lacks its armed transaction chain" + )) + })?; + publish_adopted_delta(self, table_key, delta, planned).await? + } + CandidateTableState::RewriteMerged(staged) => { + let planned = planned_transactions_by_table.get(table_key).ok_or_else(|| { + OmniError::manifest_internal(format!( + "branch merge table '{table_key}' lacks its armed transaction chain" + )) + })?; + publish_rewritten_merge_table(self, table_key, staged, planned).await? + } + }; + if first_touch_effects.contains(table_key) { + let target = target_branch.ok_or_else(|| { + OmniError::manifest_internal(format!( + "first-touch merge effect '{}' has no named target branch", + table_key + )) + })?; + let entry = target_snapshot + .entry(table_key) + .or_else(|| source_snapshot.entry(table_key)) + .ok_or_else(|| { + OmniError::manifest_internal(format!( + "first-touch merge effect '{}' has no table path", + table_key + )) + })?; + let full_path = self.storage().dataset_uri(&entry.table_path); + let target_head = self + .storage() + .open_dataset_head(&full_path, Some(target)) + .await?; + confirmed_ref_identifiers.insert( + table_key.clone(), + self.storage().branch_identifier(&target_head).await?, + ); + } + if table_key.starts_with("edge:") { + changed_edge_tables = true; + } + updates.push(update); + } + + // The Armed body remains rollback-only until every physical effect, + // every first-touch ref identity, and every logical output slot are + // durably confirmed together. + crate::failpoints::maybe_fail( + crate::failpoints::names::BRANCH_MERGE_POST_EFFECTS_PRE_CONFIRM, + )?; + if let Some((sidecar, _)) = recovery.as_mut() { + crate::db::manifest::confirm_branch_merge_sidecar_phase_b( + self.root_uri(), + self.storage_adapter(), + sidecar, + &updates, + &confirmed_ref_identifiers, + ) + .await?; + } + + crate::failpoints::maybe_fail( + crate::failpoints::names::BRANCH_MERGE_POST_PHASE_B_PRE_MANIFEST_COMMIT, + )?; + + // Publish the complete merge delta and its pre-minted lineage under + // the exact target authority captured before classification. + debug_assert_eq!( + target_txn.effective_graph_head.as_deref(), + Some(target_head_commit_id) + ); + self.commit_updates_on_branch_with_expected( + target_branch, + &updates, + &expected_versions, + actor_id, + target_txn, + merge_lineage, ) .await?; + + Ok::<_, OmniError>((updates, changed_edge_tables)) } - - // Failpoint: pin the per-writer Phase B → Phase C residual for - // branch_merge. Lance HEAD has advanced on every touched table - // (publish_*) AND the sidecar is confirmed, but the manifest publish - // below hasn't run — so recovery rolls FORWARD. Used by - // `tests/failpoints.rs::branch_merge_phase_b_failure_recovered_on_next_open`. - crate::failpoints::maybe_fail(crate::failpoints::names::BRANCH_MERGE_POST_PHASE_B_PRE_MANIFEST_COMMIT)?; - - // Publish the merged table versions AND the merge commit in one manifest - // CAS (RFC-013 Phase 7): `graph_commit` + `graph_head` rows ride the same - // merge-insert as the table-version rows. The merge commit's first parent - // is resolved by the publisher as the live target-branch head (the - // post-merge correct parent even if the target advanced); its merged-in - // parent is the source head. `target_head_commit_id` is no longer passed - // — it was the pre-merge target head, which the publisher reads live. - let _ = target_head_commit_id; - self.commit_merge_with_actor(&updates, source_head_commit_id, actor_id) - .await?; + .await; + let (_updates, changed_edge_tables) = match post_arm_result { + Ok(result) => result, + Err(error) => { + return match recovery_operation_id { + Some(operation_id) => Err(OmniError::recovery_required( + operation_id, + error.to_string(), + )), + None => Err(error), + }; + } + }; // Recovery sidecar lifecycle: delete after the manifest publish (Phase C). // Best-effort cleanup; the merge already landed durably so failing the diff --git a/crates/omnigraph/src/exec/mod.rs b/crates/omnigraph/src/exec/mod.rs index 40764140..62aa655a 100644 --- a/crates/omnigraph/src/exec/mod.rs +++ b/crates/omnigraph/src/exec/mod.rs @@ -35,7 +35,7 @@ use time::format_description::well_known::Rfc3339; use crate::db::commit_graph::CommitGraph; use crate::db::manifest::ManifestCoordinator; -use crate::db::{MergeOutcome, Omnigraph, is_internal_system_branch}; +use crate::db::{MergeOutcome, Omnigraph, WriteTxn, is_internal_system_branch}; use crate::db::{ReadTarget, Snapshot}; use crate::embedding::EmbeddingClient; use crate::error::{MergeConflict, MergeConflictKind, OmniError, Result}; diff --git a/crates/omnigraph/src/failpoints.rs b/crates/omnigraph/src/failpoints.rs index e0a3630d..f6557a80 100644 --- a/crates/omnigraph/src/failpoints.rs +++ b/crates/omnigraph/src/failpoints.rs @@ -64,6 +64,10 @@ pub mod names { /// any durable table effect. pub const BRANCH_MERGE_POST_AUTHORITY_CAPTURE: &str = "branch_merge.post_authority_capture"; + /// The v4 BranchMerge recovery intent is durable, before any first-touch + /// target table ref is created. + pub const BRANCH_MERGE_POST_SIDECAR_PRE_FORK: &str = + "branch_merge.post_sidecar_pre_fork"; pub const BRANCH_MERGE_POST_PHASE_B_PRE_MANIFEST_COMMIT: &str = "branch_merge.post_phase_b_pre_manifest_commit"; /// Every merge table effect is complete, but the sidecar is still in its @@ -133,6 +137,11 @@ pub mod names { /// its operator-facing audit row is appended. pub const RECOVERY_POST_ROLLBACK_PUBLISH_PRE_AUDIT: &str = "recovery.post_rollback_publish_pre_audit"; + /// After recovery restores one table to its prepared pre-effect content, + /// before the compensating manifest publish. A retry must recognize that + /// restore as this sidecar's owned compensation instead of wedging open. + pub const RECOVERY_POST_TABLE_RESTORE_PRE_PUBLISH: &str = + "recovery.post_table_restore_pre_publish"; pub const RECOVERY_RECORD_AUDIT: &str = "recovery.record_audit"; pub const RECOVERY_SIDECAR_CONFIRM: &str = "recovery.sidecar_confirm"; pub const RECOVERY_SIDECAR_DELETE: &str = "recovery.sidecar_delete"; diff --git a/crates/omnigraph/src/storage_layer.rs b/crates/omnigraph/src/storage_layer.rs index 6e629310..99742cbd 100644 --- a/crates/omnigraph/src/storage_layer.rs +++ b/crates/omnigraph/src/storage_layer.rs @@ -65,7 +65,7 @@ use lance::dataset::scanner::{ColumnOrdering, DatasetRecordBatchStream}; use lance::dataset::{WhenMatched, WhenNotMatched}; use crate::db::{Snapshot, SubTableEntry}; -use crate::error::Result; +use crate::error::{OmniError, Result}; use crate::table_store::{StagedTransactionIdentity, StagedWrite, TableState, TableStore}; // ─── sealed module ────────────────────────────────────────────────────────── @@ -286,6 +286,14 @@ pub trait TableStorage: sealed::Sealed + Send + Sync + Debug { branch: Option<&str>, ) -> Result; + /// Native identity of the branch backing an already-open snapshot. Used + /// by recovery-enrolled first-touch effects to confirm the exact ref they + /// created, closing delete/recreate ABA during later recovery. + async fn branch_identifier( + &self, + snapshot: &SnapshotHandle, + ) -> Result; + async fn fork_branch_from_state( &self, dataset_uri: &str, @@ -564,6 +572,17 @@ impl TableStorage for TableStore { .map(SnapshotHandle::new) } + async fn branch_identifier( + &self, + snapshot: &SnapshotHandle, + ) -> Result { + snapshot + .dataset() + .branch_identifier() + .await + .map_err(|error| OmniError::Lance(error.to_string())) + } + async fn fork_branch_from_state( &self, dataset_uri: &str, diff --git a/crates/omnigraph/tests/failpoints.rs b/crates/omnigraph/tests/failpoints.rs index c8c5d3f2..56297495 100644 --- a/crates/omnigraph/tests/failpoints.rs +++ b/crates/omnigraph/tests/failpoints.rs @@ -12,7 +12,7 @@ use serial_test::serial; use helpers::recovery::{ FollowUpMutation, RecoveryExpectation, TableExpectation, assert_post_recovery_invariants, - branch_head_commit_id, single_sidecar_operation_id, + branch_head_commit_id, recovery_audit_kinds, single_sidecar_operation_id, }; use helpers::{ MUTATION_QUERIES, collect_column_strings, count_rows, mixed_params, mutate_main, params, @@ -5176,11 +5176,9 @@ async fn branch_merge_phase_b_failure_recovered_on_next_open() { let uri = dir.path().to_str().unwrap().to_string(); // Seed main with a row, branch off, mutate BOTH sides so the merge - // produces at least one `RewriteMerged` candidate (target moved past - // base too — required for the recovery sidecar to pin anything; the - // sidecar only pins RewriteMerged candidates because they're the - // only path that always advances Lance HEAD via - // `publish_rewritten_merge_table`). + // produces a `RewriteMerged` candidate (target moved past base too). The + // v4 sidecar records its exact planned data transactions and any contiguous + // derived-index tail before publishing the complete logical delta. { let mut db = Omnigraph::init(&uri, helpers::TEST_SCHEMA).await.unwrap(); load_jsonl( @@ -5246,12 +5244,9 @@ async fn branch_merge_phase_b_failure_recovered_on_next_open() { ); } - // Recovery: reopen runs the sweep. BranchMerge uses LOOSE - // classification — `publish_rewritten_merge_table` runs multiple - // commit_staged calls per table (stage_merge_insert + stage_delete + - // index rebuilds), so post_commit_pin in the sidecar is a lower - // bound; the loose-match classifier accepts any HEAD > expected_version - // when expected_version == manifest_pinned. + // Recovery: reopen proves the v4 sidecar's exact planned transaction chain, + // accepts only its contiguous derived CreateIndex suffix, and publishes the + // confirmed final version plus fixed lineage and complete manifest delta. let db = Omnigraph::open(&uri).await.unwrap(); let recovery_dir = dir.path().join("__recovery"); @@ -5305,6 +5300,931 @@ async fn branch_merge_phase_b_failure_recovered_on_next_open() { drop(db); } +/// The v4 recovery delta is wider than its physical pin set. A mixed merge can +/// rewrite one table while another table needs only a manifest pointer switch; +/// recovery must publish both under the original merge lineage. +#[tokio::test] +#[serial(branch_merge_phase_b)] +async fn branch_merge_recovery_replays_pointer_slots_with_fixed_lineage() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap().to_string(); + let db = helpers::init_and_load(&dir).await; + db.branch_create("target").await.unwrap(); + + // Main is the source. Advance Person and Company after target forked; + // target independently advances Person. Person therefore needs a physical + // three-way rewrite, while Company is a source-on-main pointer adoption. + db.mutate( + "main", + MUTATION_QUERIES, + "insert_person", + &mixed_params(&[("$name", "source-main-person")], &[("$age", 51)]), + ) + .await + .unwrap(); + db.mutate( + "main", + OCC_DISJOINT_MUTATIONS, + "insert_company", + ¶ms(&[("$name", "source-main-company")]), + ) + .await + .unwrap(); + db.mutate( + "target", + MUTATION_QUERIES, + "insert_person", + &mixed_params(&[("$name", "target-person")], &[("$age", 52)]), + ) + .await + .unwrap(); + let source_head = branch_head_commit_id(dir.path(), "main").await.unwrap(); + let target_parent = branch_head_commit_id(dir.path(), "target").await.unwrap(); + + let error = { + let _failpoint = ScopedFailPoint::new( + names::BRANCH_MERGE_POST_PHASE_B_PRE_MANIFEST_COMMIT, + "return", + ); + db.branch_merge_as("main", "target", Some("merge-author")) + .await + .unwrap_err() + }; + let operation_id = match error { + OmniError::RecoveryRequired { operation_id, .. } => operation_id, + other => panic!("confirmed mixed merge must retain recovery ownership: {other}"), + }; + let sidecar_path = dir + .path() + .join("__recovery") + .join(format!("{operation_id}.json")); + let sidecar: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&sidecar_path).unwrap()).unwrap(); + let fixed_commit_id = sidecar["protocol_v4"]["lineage"]["graph_commit_id"] + .as_str() + .unwrap() + .to_string(); + let delta_keys = sidecar["protocol_v4"]["intended_delta"]["table_updates"] + .as_array() + .unwrap() + .iter() + .map(|slot| slot["table_key"].as_str().unwrap()) + .collect::>(); + let effect_keys = sidecar["protocol_v4"]["effects"] + .as_array() + .unwrap() + .iter() + .map(|effect| effect["table_key"].as_str().unwrap()) + .collect::>(); + assert_eq!( + delta_keys, + std::collections::HashSet::from(["node:Person", "node:Company"]) + ); + assert_eq!( + effect_keys, + std::collections::HashSet::from(["node:Person"]), + "pointer-only Company must be in the logical delta but not physical pins" + ); + drop(db); + + let recovered = Omnigraph::open(&uri).await.unwrap(); + assert!(!sidecar_path.exists()); + let people = collect_column_strings( + &helpers::read_table_branch(&recovered, "target", "node:Person").await, + "name", + ); + assert!(people.iter().any(|name| name == "source-main-person")); + assert!(people.iter().any(|name| name == "target-person")); + let companies = collect_column_strings( + &helpers::read_table_branch(&recovered, "target", "node:Company").await, + "name", + ); + assert!(companies.iter().any(|name| name == "source-main-company")); + + let recovered_head = branch_head_commit_id(dir.path(), "target").await.unwrap(); + assert_eq!(recovered_head, fixed_commit_id); + let commit = recovered.get_commit(&recovered_head).await.unwrap(); + assert_eq!( + commit.parent_commit_id.as_deref(), + Some(target_parent.as_str()) + ); + assert_eq!( + commit.merged_parent_commit_id.as_deref(), + Some(source_head.as_str()) + ); + assert_eq!(commit.actor_id.as_deref(), Some("merge-author")); +} + +/// Phase D is outside the logical commit boundary. If deleting a confirmed v4 +/// sidecar fails after the exact merge commit is already visible, the merge +/// still succeeds. The next open must recognize that fixed outcome, append only +/// its recovery audit, and retire the stale artifact without publishing a +/// second graph commit or changing any lineage field. +#[tokio::test] +#[serial] +#[serial(branch_merge_phase_b)] +async fn branch_merge_sidecar_delete_failure_finalizes_visible_fixed_lineage_once() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let (uri, _) = setup_diverged_merge_branches(&dir).await; + let db = Omnigraph::open(&uri).await.unwrap(); + let source_head = branch_head_commit_id(dir.path(), "source").await.unwrap(); + let target_parent = branch_head_commit_id(dir.path(), "target").await.unwrap(); + + let outcome = { + let _failpoint = ScopedFailPoint::new(names::RECOVERY_SIDECAR_DELETE, "return"); + db.branch_merge_as("source", "target", Some("phase-d-actor")) + .await + .expect("Phase-D sidecar deletion failure must not fail a visible merge") + }; + assert_eq!(outcome, omnigraph::db::MergeOutcome::Merged); + + let operation_id = single_sidecar_operation_id(dir.path()); + let sidecar_path = dir + .path() + .join("__recovery") + .join(format!("{operation_id}.json")); + let sidecar: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&sidecar_path).unwrap()).unwrap(); + let fixed_commit_id = sidecar["protocol_v4"]["lineage"]["graph_commit_id"] + .as_str() + .unwrap() + .to_string(); + let fixed_created_at = sidecar["protocol_v4"]["lineage"]["created_at"] + .as_i64() + .unwrap(); + assert_eq!( + branch_head_commit_id(dir.path(), "target").await.unwrap(), + fixed_commit_id, + "the writer must already have published the sidecar's fixed commit" + ); + let commits_before_reopen = db.list_commits(Some("target")).await.unwrap().len(); + assert!( + recovery_audit_kinds(dir.path()).await.is_empty(), + "the writer does not append a recovery audit before stale-sidecar finalization" + ); + drop(db); + + let recovered = Omnigraph::open(&uri).await.unwrap(); + assert!(!sidecar_path.exists()); + let recovered_head = branch_head_commit_id(dir.path(), "target").await.unwrap(); + assert_eq!(recovered_head, fixed_commit_id); + let recovered_commit = recovered.get_commit(&recovered_head).await.unwrap(); + assert_eq!( + recovered_commit.parent_commit_id.as_deref(), + Some(target_parent.as_str()) + ); + assert_eq!( + recovered_commit.merged_parent_commit_id.as_deref(), + Some(source_head.as_str()) + ); + assert_eq!(recovered_commit.actor_id.as_deref(), Some("phase-d-actor")); + assert_eq!(recovered_commit.created_at, fixed_created_at); + assert_eq!( + recovered.list_commits(Some("target")).await.unwrap().len(), + commits_before_reopen, + "stale-sidecar finalization must not publish a second graph commit" + ); + assert_eq!( + recovery_audit_kinds(dir.path()) + .await + .into_iter() + .filter(|kind| kind == "RolledForward") + .count(), + 1, + "the visible fixed outcome must have exactly one recovery audit row" + ); + + drop(recovered); + let reopened = Omnigraph::open(&uri).await.unwrap(); + assert_eq!( + reopened.list_commits(Some("target")).await.unwrap().len(), + commits_before_reopen, + "a second reopen must remain a lineage no-op" + ); + assert_eq!( + recovery_audit_kinds(dir.path()) + .await + .into_iter() + .filter(|kind| kind == "RolledForward") + .count(), + 1, + "a second reopen must not duplicate the audit row" + ); +} + +/// A merge plan is classified against one exact target graph head. Advancing +/// that head after every table effect is confirmed but before the manifest CAS +/// must not silently re-parent the stale merge. This uses the queue-bypassing +/// test seam to model a writer in another process: ordinary `Omnigraph` +/// handles share the root-scoped gates and therefore cannot exercise the +/// persistent-authority boundary. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +#[serial(branch_merge_phase_b)] +async fn branch_merge_post_effect_target_advance_requires_recovery_and_preserves_winner() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let (uri, main_rows) = setup_diverged_merge_branches(&dir).await; + { + let db = Omnigraph::open(&uri).await.unwrap(); + db.mutate( + "target", + OCC_DISJOINT_MUTATIONS, + "insert_company", + ¶ms(&[("$name", "target-winner-company")]), + ) + .await + .unwrap(); + } + let merge_db = std::sync::Arc::new(Omnigraph::open(&uri).await.unwrap()); + let mut target_winner = Omnigraph::open(&uri).await.unwrap(); + let source_head = branch_head_commit_id(dir.path(), "source").await.unwrap(); + + let merge_rv = helpers::failpoint::Rendezvous::park_first( + names::BRANCH_MERGE_POST_PHASE_B_PRE_MANIFEST_COMMIT, + ); + let merge_handle = std::sync::Arc::clone(&merge_db); + let merge_task = tokio::spawn(async move { + merge_handle.branch_merge("source", "target").await + }); + merge_rv.wait_until_reached().await; + + // Publish a logically redundant Company pin directly on the target. The + // table is disjoint from the merge's Person effect, but the graph lineage + // still advances, invalidating the merge's coarse target authority token. + // The test-only seam deliberately bypasses the process-local queues. + let company_uri = node_table_uri(&uri, "Company"); + let mut raw_company = lance::Dataset::open(&company_uri) + .await + .unwrap() + .checkout_branch("target") + .await + .unwrap(); + helpers::lance_delete_inline(&mut raw_company, "1 = 2").await; + target_winner + .failpoint_publish_table_head_without_index_rebuild_for_test( + "target", + "node:Company", + Some("target"), + ) + .await + .unwrap(); + let winner_head = branch_head_commit_id(dir.path(), "target").await.unwrap(); + merge_rv.release(); + + let error = merge_task + .await + .unwrap() + .expect_err("a post-effect target advance must reject the stale merge publish"); + let operation_id = match error { + OmniError::RecoveryRequired { operation_id, .. } => operation_id, + other => panic!("expected RecoveryRequired after merge effects, got {other}"), + }; + + let sidecar_path = dir + .path() + .join("__recovery") + .join(format!("{operation_id}.json")); + let sidecar: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&sidecar_path).unwrap()).unwrap(); + assert_eq!(sidecar["schema_version"], 4); + assert_eq!( + sidecar["protocol_v4"]["lineage"]["merged_parent_commit_id"], + source_head.as_str(), + "recovery must retain the captured source commit as the merge parent" + ); + + drop(merge_db); + drop(target_winner); + + // Full recovery sees that target authority changed. It must preserve the + // winning graph commit and compensate only the unpublished merge effects. + let recovered = Omnigraph::open(&uri).await.unwrap(); + assert!( + !sidecar_path.exists(), + "successful compensation must retire the merge recovery intent" + ); + assert_eq!( + helpers::count_rows_branch(&recovered, "target", "node:Person").await, + main_rows + 1, + "recovery must restore the pre-merge target image" + ); + let target_names = collect_column_strings( + &helpers::read_table_branch(&recovered, "target", "node:Person").await, + "name", + ); + assert!(target_names.iter().any(|name| name == "old-target-only")); + assert!(!target_names.iter().any(|name| name == "source-only")); + + let recovered_head = branch_head_commit_id(dir.path(), "target").await.unwrap(); + let recovered_commit = recovered.get_commit(&recovered_head).await.unwrap(); + assert_eq!( + recovered_commit.parent_commit_id.as_deref(), + Some(winner_head.as_str()), + "rollback lineage must descend from, never replace, the target winner" + ); +} + +/// If a foreign writer advances and publishes the SAME target table after the +/// merge's confirmed effect, that effect is buried under foreign state. Full +/// recovery must fail closed instead of restoring through the winner or +/// adopting the newer numeric HEAD as this merge's output. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +#[serial(branch_merge_phase_b)] +async fn branch_merge_post_effect_same_table_advance_fails_closed() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let (uri, _) = setup_diverged_merge_branches(&dir).await; + let merge_db = std::sync::Arc::new(Omnigraph::open(&uri).await.unwrap()); + let mut target_winner = Omnigraph::open(&uri).await.unwrap(); + let merge_rv = helpers::failpoint::Rendezvous::park_first( + names::BRANCH_MERGE_POST_PHASE_B_PRE_MANIFEST_COMMIT, + ); + + let merge_handle = std::sync::Arc::clone(&merge_db); + let merge_task = tokio::spawn(async move { + merge_handle.branch_merge("source", "target").await + }); + merge_rv.wait_until_reached().await; + + let person_uri = node_table_uri(&uri, "Person"); + let mut raw_target = lance::Dataset::open(&person_uri) + .await + .unwrap() + .checkout_branch("target") + .await + .unwrap(); + helpers::lance_delete_inline(&mut raw_target, "1 = 2").await; + let winner_lance_head = raw_target.version().version; + target_winner + .failpoint_publish_table_head_without_index_rebuild_for_test( + "target", + "node:Person", + Some("target"), + ) + .await + .unwrap(); + let winner_manifest_version = helpers::version_branch(&target_winner, "target") + .await + .unwrap(); + merge_rv.release(); + + let operation_id = match merge_task.await.unwrap().unwrap_err() { + OmniError::RecoveryRequired { operation_id, .. } => operation_id, + other => panic!("same-table post-effect contention must require recovery: {other}"), + }; + let sidecar_path = dir + .path() + .join("__recovery") + .join(format!("{operation_id}.json")); + assert!(sidecar_path.exists()); + drop(merge_db); + + let error = match Omnigraph::open(&uri).await { + Ok(_) => panic!("Full recovery must refuse to restore through a same-table winner"), + Err(error) => error, + }; + assert!( + error.to_string().contains("manifest pin changed") + || error.to_string().contains("foreign or unverifiable"), + "unexpected fail-closed error: {error}" + ); + assert!( + sidecar_path.exists(), + "operator-owned intent must remain durable" + ); + assert_eq!( + helpers::version_branch(&target_winner, "target") + .await + .unwrap(), + winner_manifest_version, + "failed recovery must not move the winning target manifest" + ); + let lance_after_failed_recovery = lance::Dataset::open(&person_uri) + .await + .unwrap() + .checkout_branch("target") + .await + .unwrap() + .version() + .version; + assert_eq!( + lance_after_failed_recovery, winner_lance_head, + "fail-closed recovery must not restore through the winning Lance HEAD" + ); +} + +/// A v4 rollback is itself a recoverable multi-step write. If open restores an +/// owned merge effect and then crashes before publishing that compensated HEAD, +/// the next open must recognize the restore transaction, reuse it, and finish +/// the fixed rollback once. Re-restoring forever would monotonically advance +/// Lance HEAD while every read-write open remained wedged. +#[tokio::test] +#[serial] +#[serial(branch_merge_phase_b)] +async fn branch_merge_rollback_restarts_after_restore_before_publish() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let (uri, main_rows) = setup_diverged_merge_branches(&dir).await; + + // Give the target a disjoint owned table. After the merge reaches its + // confirmed Phase B, a no-op commit on this table advances target graph + // authority without burying the merge-owned Person HEAD, making safe + // compensation both necessary and possible. + let db = Omnigraph::open(&uri).await.unwrap(); + db.mutate( + "target", + OCC_DISJOINT_MUTATIONS, + "insert_company", + ¶ms(&[("$name", "rollback-winner-company")]), + ) + .await + .unwrap(); + let mut target_winner = Omnigraph::open(&uri).await.unwrap(); + + let operation_id = { + let _failpoint = ScopedFailPoint::new( + names::BRANCH_MERGE_POST_PHASE_B_PRE_MANIFEST_COMMIT, + "return", + ); + match db.branch_merge("source", "target").await.unwrap_err() { + OmniError::RecoveryRequired { operation_id, .. } => operation_id, + other => panic!("confirmed merge must retain recovery ownership: {other}"), + } + }; + let sidecar_path = dir + .path() + .join("__recovery") + .join(format!("{operation_id}.json")); + assert!(sidecar_path.exists()); + + let company_uri = node_table_uri(&uri, "Company"); + let mut raw_company = lance::Dataset::open(&company_uri) + .await + .unwrap() + .checkout_branch("target") + .await + .unwrap(); + helpers::lance_delete_inline(&mut raw_company, "1 = 2").await; + target_winner + .failpoint_publish_table_head_without_index_rebuild_for_test( + "target", + "node:Company", + Some("target"), + ) + .await + .unwrap(); + let winner_head = branch_head_commit_id(dir.path(), "target").await.unwrap(); + + let person_uri = node_table_uri(&uri, "Person"); + let person_before_restore = lance::Dataset::open(&person_uri) + .await + .unwrap() + .checkout_branch("target") + .await + .unwrap() + .version() + .version; + drop(db); + drop(target_winner); + + { + let _failpoint = + ScopedFailPoint::new(names::RECOVERY_POST_TABLE_RESTORE_PRE_PUBLISH, "return"); + let error = match Omnigraph::open(&uri).await { + Ok(_) => panic!("recovery must stop after the injected table restore"), + Err(error) => error, + }; + assert!( + error + .to_string() + .contains("recovery.post_table_restore_pre_publish"), + "unexpected interrupted-rollback error: {error}" + ); + } + assert!( + sidecar_path.exists(), + "an interrupted rollback must retain its recovery intent" + ); + assert_eq!( + branch_head_commit_id(dir.path(), "target").await.unwrap(), + winner_head, + "the rollback manifest publish must not occur before the failpoint" + ); + assert!( + recovery_audit_kinds(dir.path()).await.is_empty(), + "an interrupted rollback must not claim a completed audit outcome" + ); + let person_after_interrupted_restore = lance::Dataset::open(&person_uri) + .await + .unwrap() + .checkout_branch("target") + .await + .unwrap() + .version() + .version; + assert!( + person_after_interrupted_restore > person_before_restore, + "the fixture must durably restore Person before interrupting the manifest publish" + ); + + let recovered = Omnigraph::open(&uri) + .await + .expect("the next open must recognize and finish the interrupted compensation"); + assert!(!sidecar_path.exists()); + let person_after_recovery = lance::Dataset::open(&person_uri) + .await + .unwrap() + .checkout_branch("target") + .await + .unwrap() + .version() + .version; + assert_eq!( + person_after_recovery, person_after_interrupted_restore, + "restartable rollback must reuse the owned restore instead of restoring again" + ); + assert_eq!( + helpers::count_rows_branch(&recovered, "target", "node:Person").await, + main_rows + 1 + ); + let target_names = collect_column_strings( + &helpers::read_table_branch(&recovered, "target", "node:Person").await, + "name", + ); + assert!(target_names.iter().any(|name| name == "old-target-only")); + assert!(!target_names.iter().any(|name| name == "source-only")); + let rollback_head = branch_head_commit_id(dir.path(), "target").await.unwrap(); + let rollback_commit = recovered.get_commit(&rollback_head).await.unwrap(); + assert_eq!( + rollback_commit.parent_commit_id.as_deref(), + Some(winner_head.as_str()) + ); + assert_eq!( + recovery_audit_kinds(dir.path()) + .await + .into_iter() + .filter(|kind| kind == "RolledBack") + .count(), + 1 + ); + + drop(recovered); + let _reopened = Omnigraph::open(&uri).await.unwrap(); + let person_after_second_open = lance::Dataset::open(&person_uri) + .await + .unwrap() + .checkout_branch("target") + .await + .unwrap() + .version() + .version; + assert_eq!(person_after_second_open, person_after_recovery); + assert_eq!( + recovery_audit_kinds(dir.path()) + .await + .into_iter() + .filter(|kind| kind == "RolledBack") + .count(), + 1, + "a second open must not repeat rollback or its audit" + ); +} + +/// A pure first-touch/ref-only merge can reach EffectsConfirmed without any +/// data HEAD movement. Recovery must validate the minted ref identity and roll +/// the exact pointer delta forward, not discard it as an empty intent. +#[tokio::test] +#[serial(branch_merge_first_touch)] +async fn branch_merge_confirmed_ref_only_effect_rolls_forward() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap().to_string(); + let db = helpers::init_and_load(&dir).await; + let main_rows = helpers::count_rows(&db, "node:Person").await; + db.branch_create("source").await.unwrap(); + db.branch_create("target").await.unwrap(); + db.mutate( + "source", + MUTATION_QUERIES, + "insert_person", + &mixed_params(&[("$name", "confirmed-ref-row")], &[("$age", 38)]), + ) + .await + .unwrap(); + + let operation_id = { + let _failpoint = ScopedFailPoint::new( + names::BRANCH_MERGE_POST_PHASE_B_PRE_MANIFEST_COMMIT, + "return", + ); + match db.branch_merge("source", "target").await.unwrap_err() { + OmniError::RecoveryRequired { operation_id, .. } => operation_id, + other => panic!("confirmed ref-only merge must retain recovery ownership: {other}"), + } + }; + let sidecar_path = dir + .path() + .join("__recovery") + .join(format!("{operation_id}.json")); + let sidecar: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&sidecar_path).unwrap()).unwrap(); + assert_eq!(sidecar["protocol_v4"]["effect_phase"], "EffectsConfirmed"); + assert!(!sidecar["protocol_v4"]["effects"][0]["kind"]["confirmed_branch_identifier"].is_null()); + drop(db); + + let recovered = Omnigraph::open(&uri).await.unwrap(); + assert!(!sidecar_path.exists()); + assert_eq!( + helpers::count_rows_branch(&recovered, "target", "node:Person").await, + main_rows + 1 + ); + let names = collect_column_strings( + &helpers::read_table_branch(&recovered, "target", "node:Person").await, + "name", + ); + assert!(names.iter().any(|name| name == "confirmed-ref-row")); +} + +/// RewriteMerged can append reconciler-owned CreateIndex commits after its +/// pre-minted logical data transactions. An Armed crash in that derived tail is +/// still rollback-only, but recovery must prove the exact planned transaction +/// prefix and accept only the contiguous CreateIndex suffix rather than treating +/// its larger numeric HEAD as foreign movement. +#[tokio::test] +#[serial] +#[serial(branch_merge_phase_b)] +async fn branch_merge_armed_index_tail_rolls_back_after_exact_transaction_prefix() { + use lance::index::DatasetIndexExt; + use omnigraph::table_store::TableStore; + + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let (uri, main_rows) = setup_diverged_merge_branches(&dir).await; + let mut db = Omnigraph::open(&uri).await.unwrap(); + let person_uri = node_table_uri(&uri, "Person"); + + // Make the rewrite rebuild `id_idx` after its logical data transaction. + // Publish the dropped-index HEAD first so the merge's expected version and + // planned transaction chain are anchored to a fully consistent target. + let store = TableStore::new(&uri, test_session()); + let mut target_person = store + .open_dataset_head(&person_uri, Some("target")) + .await + .unwrap(); + target_person.drop_index("id_idx").await.unwrap(); + let dropped_index_head = target_person.version().version; + db.failpoint_publish_table_head_without_index_rebuild_for_test( + "target", + "node:Person", + Some("target"), + ) + .await + .unwrap(); + assert_eq!( + db.snapshot_of(omnigraph::db::ReadTarget::branch("target")) + .await + .unwrap() + .entry("node:Person") + .unwrap() + .table_version, + dropped_index_head + ); + + let operation_id = { + let _failpoint = + ScopedFailPoint::new(names::BRANCH_MERGE_POST_EFFECTS_PRE_CONFIRM, "return"); + match db.branch_merge("source", "target").await.unwrap_err() { + OmniError::RecoveryRequired { operation_id, .. } => operation_id, + other => panic!("Armed derived-tail failure must retain recovery ownership: {other}"), + } + }; + let sidecar_path = dir + .path() + .join("__recovery") + .join(format!("{operation_id}.json")); + let sidecar: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&sidecar_path).unwrap()).unwrap(); + assert_eq!(sidecar["protocol_v4"]["effect_phase"], "Armed"); + let person_pin = sidecar["tables"] + .as_array() + .unwrap() + .iter() + .find(|pin| pin["table_key"] == "node:Person") + .unwrap(); + let expected_version = person_pin["expected_version"].as_u64().unwrap(); + let person_effect = sidecar["protocol_v4"]["effects"] + .as_array() + .unwrap() + .iter() + .find(|effect| effect["table_key"] == "node:Person") + .unwrap(); + let planned_transaction_count = person_effect["kind"]["planned_transactions"] + .as_array() + .unwrap() + .len(); + assert!( + planned_transaction_count > 0, + "fixture must plan at least one logical Person transaction" + ); + let raw_head_with_index_tail = store + .open_dataset_head(&person_uri, Some("target")) + .await + .unwrap() + .version() + .version; + assert!( + raw_head_with_index_tail > expected_version + planned_transaction_count as u64, + "raw HEAD {raw_head_with_index_tail} must include at least one CreateIndex tail after \ + expected {expected_version} + {planned_transaction_count} planned transactions" + ); + drop(db); + + let recovered = Omnigraph::open(&uri) + .await + .expect("Full recovery must roll an exact Armed transaction+index tail back"); + assert!(!sidecar_path.exists()); + assert_eq!( + helpers::count_rows_branch(&recovered, "target", "node:Person").await, + main_rows + 1 + ); + let names = collect_column_strings( + &helpers::read_table_branch(&recovered, "target", "node:Person").await, + "name", + ); + assert!(names.iter().any(|name| name == "old-target-only")); + assert!(!names.iter().any(|name| name == "source-only")); +} + +/// Phase-B confirmation is an ownership proof, not a numeric HEAD stamp. A +/// foreign logical Append can land after the merge's exact data transaction and +/// before its retrying CreateIndex tail; Lance may validly rebase that index +/// build over the Append, but the merge must not confirm or recover the foreign +/// row as its own output. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +#[serial] +#[serial(branch_merge_phase_b)] +async fn branch_merge_confirmation_rejects_foreign_append_before_index_tail() { + use futures::TryStreamExt; + use lance::index::DatasetIndexExt; + use omnigraph::table_store::TableStore; + + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let (uri, _) = setup_diverged_merge_branches(&dir).await; + let mut db = Omnigraph::open(&uri).await.unwrap(); + + // Prepare one schema-exact row on an unrelated branch. Its RecordBatch can + // then be appended directly to target Lance HEAD without invoking any + // target manifest publisher or process-local write gate. + db.branch_create("foreign-seed").await.unwrap(); + db.mutate( + "foreign-seed", + MUTATION_QUERIES, + "insert_person", + &mixed_params(&[("$name", "foreign-unpublished")], &[("$age", 61)]), + ) + .await + .unwrap(); + let foreign_batch = helpers::read_table_branch(&db, "foreign-seed", "node:Person") + .await + .into_iter() + .find_map(|batch| { + let names = batch + .column_by_name("name")? + .as_any() + .downcast_ref::()?; + (0..batch.num_rows()) + .find(|row| names.value(*row) == "foreign-unpublished") + .map(|row| batch.slice(row, 1)) + }) + .expect("foreign seed row must be readable as one append batch"); + + // Force RewriteMerged to execute a CreateIndex tail after its exact data + // transaction, while keeping the target manifest consistent before merge. + let person_uri = node_table_uri(&uri, "Person"); + let store = TableStore::new(&uri, test_session()); + let mut target_person = store + .open_dataset_head(&person_uri, Some("target")) + .await + .unwrap(); + target_person.drop_index("id_idx").await.unwrap(); + let dropped_index_head = target_person.version().version; + db.failpoint_publish_table_head_without_index_rebuild_for_test( + "target", + "node:Person", + Some("target"), + ) + .await + .unwrap(); + let target_head_before_merge = branch_head_commit_id(dir.path(), "target").await.unwrap(); + + let merge_db = std::sync::Arc::new(db); + let merge_rv = helpers::failpoint::Rendezvous::park_first( + names::BRANCH_MERGE_REWRITE_AFTER_DELETE_PRE_INDEX, + ); + let merge_handle = std::sync::Arc::clone(&merge_db); + let merge_task = tokio::spawn(async move { + merge_handle.branch_merge("source", "target").await + }); + merge_rv.wait_until_reached().await; + + // The merge's logical data transaction is now at HEAD, but its index build + // has not started. Append a real logical row without publishing target + // manifest authority, then let CreateIndex rebase over that foreign commit. + let mut raw_target = store + .open_dataset_head(&person_uri, Some("target")) + .await + .unwrap(); + helpers::lance_append_inline(&mut raw_target, foreign_batch).await; + let foreign_append_head = raw_target.version().version; + merge_rv.release(); + + let operation_id = match merge_task.await.unwrap().unwrap_err() { + OmniError::RecoveryRequired { operation_id, .. } => operation_id, + other => panic!("foreign confirmation tail must retain recovery ownership: {other}"), + }; + let sidecar_path = dir + .path() + .join("__recovery") + .join(format!("{operation_id}.json")); + let sidecar: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&sidecar_path).unwrap()).unwrap(); + assert_eq!( + sidecar["protocol_v4"]["effect_phase"], "Armed", + "confirmation must reject before persisting EffectsConfirmed" + ); + let raw_head_after_index_rebase = store + .open_dataset_head(&person_uri, Some("target")) + .await + .unwrap() + .version() + .version; + assert!( + raw_head_after_index_rebase > foreign_append_head, + "fixture must place at least one rebased CreateIndex commit after the foreign Append" + ); + assert_eq!( + branch_head_commit_id(dir.path(), "target").await.unwrap(), + target_head_before_merge, + "rejected confirmation must not advance target graph lineage" + ); + drop(merge_db); + + let open_error = match Omnigraph::open(&uri).await { + Ok(_) => panic!("Full recovery must fail closed on a non-CreateIndex foreign tail"), + Err(error) => error, + }; + assert!( + open_error.to_string().contains("foreign") + || open_error.to_string().contains("unverifiable") + || open_error.to_string().contains("CreateIndex"), + "unexpected fail-closed error: {open_error}" + ); + assert!(sidecar_path.exists()); + assert_eq!( + branch_head_commit_id(dir.path(), "target").await.unwrap(), + target_head_before_merge, + "failed recovery must not publish or re-parent the merge" + ); + let read_only = Omnigraph::open_read_only(&uri).await.unwrap(); + assert_eq!( + read_only + .snapshot_of(omnigraph::db::ReadTarget::branch("target")) + .await + .unwrap() + .entry("node:Person") + .unwrap() + .table_version, + dropped_index_head, + "target manifest must remain at its pre-merge Person pin" + ); + let raw_after_failed_recovery = store + .open_dataset_head(&person_uri, Some("target")) + .await + .unwrap(); + assert_eq!( + raw_after_failed_recovery.version().version, + raw_head_after_index_rebase, + "fail-closed recovery must not restore through the foreign Append" + ); + let raw_batches: Vec = raw_after_failed_recovery + .scan() + .try_into_stream() + .await + .unwrap() + .try_collect() + .await + .unwrap(); + assert!( + collect_column_strings(&raw_batches, "name") + .iter() + .any(|name| name == "foreign-unpublished"), + "the unpublished foreign row must survive fail-closed confirmation and recovery" + ); +} + /// AdoptWithDelta recovery (the gap closure): a fast-forward merge — main has /// NOT advanced since the branch forked, so the touched table is classified /// `AdoptWithDelta`, not `RewriteMerged` — that fails after Phase B must still @@ -5425,9 +6345,9 @@ async fn sorted_person_names(db: &Omnigraph) -> Vec { /// THE recovery-atomicity regression gate. A branch merge whose per-table publish /// is a multi-commit sequence (append → upsert → delete, or merge_insert → delete /// → index) advances Lance HEAD step by step before the manifest publish. If the -/// process dies *mid*-sequence — after some commits but before the achieved-version -/// intent is recorded — recovery must roll the whole merge **back**, not publish -/// the partial and record the merge as complete. +/// process dies *mid*-sequence — after some planned commits but while the v4 +/// sidecar remains Armed — recovery must roll the whole merge **back**, not +/// publish the partial and record the merge as complete. /// /// The delta is deliberately MIXED — a fresh id (`bob`, append), a modified base id /// (`carol`, upsert) and a removed base id (`dave`, delete) — so every partial @@ -5435,10 +6355,11 @@ async fn sorted_person_names(db: &Omnigraph) -> Vec { /// back at its base name-set, and a *re-run* of the merge re-applies the full delta /// (the partial was not silently recorded as "already merged"). /// -/// RED before the fix: the loose `BranchMerge` classification rolls any -/// `lance_head > manifest_pinned` forward, so the partial is published (e.g. `bob` -/// present, `dave` kept) and the merge recorded — the first assert (back at base) -/// fails. GREEN after: `achieved_version == None` → `IncompletePhaseB` → roll back. +/// RED before the fix: the legacy loose `BranchMerge` classifier rolled any +/// `lance_head > manifest_pinned` forward, so the partial was published (e.g. +/// `bob` present, `dave` kept) and the merge recorded. GREEN after: an Armed v4 +/// sidecar can prove only a proper prefix of its planned transaction chain, so +/// recovery compensates it rather than treating numeric movement as completion. async fn assert_partial_merge_rolls_back(scenario: MergeScenario, failpoint: &str) { use omnigraph::loader::load_jsonl; @@ -5642,7 +6563,10 @@ async fn pre_upgrade_v1_branch_merge_sidecar_rolls_forward_not_back() { .expect("a recovery sidecar must exist after the post-Phase-B crash"); let mut v: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); + v["merge_source_commit_id"] = + v["protocol_v4"]["lineage"]["merged_parent_commit_id"].clone(); v["schema_version"] = serde_json::json!(1); + v.as_object_mut().unwrap().remove("protocol_v4"); for table in v["tables"].as_array_mut().unwrap() { table.as_object_mut().unwrap().remove("confirmed_version"); } @@ -5765,7 +6689,7 @@ async fn branch_merge_phase_b_failure_recovered_on_non_main_target() { assert_post_recovery_invariants( dir.path(), &operation_id, - RecoveryExpectation::RolledForward { + RecoveryExpectation::RolledForwardOriginalLineage { tables: vec![ TableExpectation::branch("node:Person", "target_branch") .expected_main_manifest_pin(main_person_pin) @@ -6561,6 +7485,169 @@ async fn branch_merge_rejects_fresh_target_manifest_change_before_effects() { ); } +/// Source is a captured immutable input, not part of the target publisher's +/// atomic read set. A later commit on the same source incarnation must not make +/// the merge substitute the newer source head (or reject an otherwise-valid +/// captured snapshot). +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +#[serial] +async fn branch_merge_source_advance_keeps_captured_source_parent() { + let _scenario = FailScenario::setup(); + let dir = tempfile::tempdir().unwrap(); + let (uri, main_rows) = setup_diverged_merge_branches(&dir).await; + let merge_db = std::sync::Arc::new(Omnigraph::open(&uri).await.unwrap()); + let mut source_writer = Omnigraph::open(&uri).await.unwrap(); + let captured_source_head = branch_head_commit_id(dir.path(), "source").await.unwrap(); + let merge_rv = + helpers::failpoint::Rendezvous::park_first(names::BRANCH_MERGE_POST_AUTHORITY_CAPTURE); + + let merge_handle = std::sync::Arc::clone(&merge_db); + let merge_task = tokio::spawn(async move { + merge_handle.branch_merge("source", "target").await + }); + merge_rv.wait_until_reached().await; + + // Model a foreign source writer without changing row content: advance the + // source table HEAD with a no-op delete, then publish it through the + // queue-bypassing seam. The source branch incarnation remains unchanged. + let person_uri = node_table_uri(&uri, "Person"); + let mut raw_source = lance::Dataset::open(&person_uri) + .await + .unwrap() + .checkout_branch("source") + .await + .unwrap(); + helpers::lance_delete_inline(&mut raw_source, "1 = 2").await; + source_writer + .failpoint_publish_table_head_without_index_rebuild_for_test( + "source", + "node:Person", + Some("source"), + ) + .await + .unwrap(); + let advanced_source_head = branch_head_commit_id(dir.path(), "source").await.unwrap(); + assert_ne!(advanced_source_head, captured_source_head); + merge_rv.release(); + + assert_eq!( + merge_task.await.unwrap().unwrap(), + omnigraph::db::MergeOutcome::Merged + ); + let reopened = Omnigraph::open(&uri).await.unwrap(); + assert_eq!( + helpers::count_rows_branch(&reopened, "target", "node:Person").await, + main_rows + 2 + ); + let target_head = branch_head_commit_id(dir.path(), "target").await.unwrap(); + let merge_commit = reopened.get_commit(&target_head).await.unwrap(); + assert_eq!( + merge_commit.merged_parent_commit_id.as_deref(), + Some(captured_source_head.as_str()), + "merge lineage must name the source commit captured before planning" + ); + assert_ne!( + merge_commit.merged_parent_commit_id.as_deref(), + Some(advanced_source_head.as_str()), + "a later source head must never be substituted at publish time" + ); +} + +async fn assert_branch_merge_first_touch_ref_is_recovered( + failpoint: &str, + ref_exists_before_recovery: bool, +) { + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap().to_string(); + let db = helpers::init_and_load(&dir).await; + let main_rows = helpers::count_rows(&db, "node:Person").await; + db.branch_create("source").await.unwrap(); + db.branch_create("target").await.unwrap(); + db.mutate( + "source", + MUTATION_QUERIES, + "insert_person", + &mixed_params(&[("$name", "source-first-touch")], &[("$age", 37)]), + ) + .await + .unwrap(); + + let person_uri = node_table_uri(&uri, "Person"); + let person = lance::Dataset::open(&person_uri).await.unwrap(); + assert!( + !person.list_branches().await.unwrap().contains_key("target"), + "fixture requires the target table ref to be lazy" + ); + + let error = { + let _failpoint = ScopedFailPoint::new(failpoint, "return"); + db.branch_merge("source", "target").await.unwrap_err() + }; + let operation_id = match error { + OmniError::RecoveryRequired { operation_id, .. } => operation_id, + other => panic!("first-touch failure must retain recovery ownership: {other}"), + }; + let sidecar_path = dir + .path() + .join("__recovery") + .join(format!("{operation_id}.json")); + let sidecar: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&sidecar_path).unwrap()).unwrap(); + assert_eq!(sidecar["schema_version"], 4); + assert_eq!( + sidecar["protocol_v4"]["effects"][0]["kind"]["kind"], + "RefOnlyFork" + ); + + let person = lance::Dataset::open(&person_uri).await.unwrap(); + assert_eq!( + person.list_branches().await.unwrap().contains_key("target"), + ref_exists_before_recovery, + "fixture must stop at the intended sidecar/ref boundary" + ); + drop(db); + + let recovered = Omnigraph::open(&uri).await.unwrap(); + assert!(!sidecar_path.exists()); + let person = lance::Dataset::open(&person_uri).await.unwrap(); + assert!( + !person.list_branches().await.unwrap().contains_key("target"), + "Full recovery must reclaim an unpublished first-touch target ref" + ); + assert_eq!( + helpers::count_rows_branch(&recovered, "target", "node:Person").await, + main_rows, + "failed first-touch merge must leave target inheriting its old image" + ); + + assert_eq!( + recovered.branch_merge("source", "target").await.unwrap(), + omnigraph::db::MergeOutcome::FastForward + ); + assert_eq!( + helpers::count_rows_branch(&recovered, "target", "node:Person").await, + main_rows + 1 + ); +} + +#[tokio::test] +#[serial(branch_merge_first_touch)] +async fn branch_merge_sidecar_precedes_first_touch_target_ref() { + let _scenario = FailScenario::setup(); + assert_branch_merge_first_touch_ref_is_recovered( + names::BRANCH_MERGE_POST_SIDECAR_PRE_FORK, + false, + ) + .await; +} + +#[tokio::test] +#[serial(branch_merge_first_touch)] +async fn branch_merge_recovers_ambiguous_first_touch_ref_creation() { + let _scenario = FailScenario::setup(); + assert_branch_merge_first_touch_ref_is_recovered(names::FORK_POST_CREATE_PRE_OPEN, true).await; +} + /// A legacy writer can arm a relevant sidecar after merge's initial recovery /// barrier. Merge acquires the complete source/target table envelope and lists /// again before Phase A, so the late intent blocks it even when no table HEAD @@ -6751,6 +7838,26 @@ async fn full_recovery_rereads_sidecar_body_after_discovery() { for table in tables { table["confirmed_version"] = serde_json::Value::Null; } + let protocol = sidecar["protocol_v4"] + .as_object_mut() + .expect("branch-merge sidecar must carry protocol_v4"); + protocol.insert( + "effect_phase".to_string(), + serde_json::Value::String("Armed".to_string()), + ); + for effect in protocol["effects"] + .as_array_mut() + .expect("branch-merge effects must be an array") + { + effect["kind"]["confirmed_version"] = serde_json::Value::Null; + effect["kind"]["confirmed_branch_identifier"] = serde_json::Value::Null; + } + for slot in protocol["intended_delta"]["table_updates"] + .as_array_mut() + .expect("branch-merge delta slots must be an array") + { + slot["confirmed"] = serde_json::Value::Null; + } std::fs::write( &sidecar_path, serde_json::to_string_pretty(&sidecar).unwrap(), diff --git a/crates/omnigraph/tests/helpers/recovery.rs b/crates/omnigraph/tests/helpers/recovery.rs index 7db5efeb..f398f8b1 100644 --- a/crates/omnigraph/tests/helpers/recovery.rs +++ b/crates/omnigraph/tests/helpers/recovery.rs @@ -16,8 +16,9 @@ pub enum RecoveryExpectation { RolledForward { tables: Vec, }, - /// Protocol-v3 mutation/load recovery republishes the writer's fixed - /// lineage intent rather than minting a synthetic recovery commit. + /// RFC-022 v3 mutation/load and v4 branch-merge recovery republish the + /// writer's fixed lineage intent rather than minting a synthetic recovery + /// commit. RolledForwardOriginalLineage { tables: Vec, }, diff --git a/crates/omnigraph/tests/merge_cost.rs b/crates/omnigraph/tests/merge_cost.rs index 4457b3ec..4e5136ca 100644 --- a/crates/omnigraph/tests/merge_cost.rs +++ b/crates/omnigraph/tests/merge_cost.rs @@ -53,7 +53,9 @@ where /// one inserted Person must NOT open the untouched tables for validation — cost /// follows the delta, not the catalog. Pre-#5 this opened ~6 tables via a /// full-graph validation scan; the index-backed evaluator probes only the -/// committed Person table (for uniqueness) plus the delta. +/// committed Person table (for uniqueness) plus the delta, and RFC-022 v4 +/// re-opens the one physical effect to prove its exact transaction history +/// before confirmation. #[test] fn merge_validation_is_delta_scoped() { on_big_stack(|| async { @@ -102,12 +104,12 @@ fn merge_validation_is_delta_scoped() { ); // The proof: only Person changed, so the merge opens only Person-related - // tables (the delta + the committed-target index probe for uniqueness) — - // never the untouched Company / Knows / WorksAt. Pre-#5 this was ~6 - // (every catalog table, full-scanned). + // tables (the delta + committed-target index probe + exact confirmation + // re-observation) — never the untouched Company / Knows / WorksAt. + // Pre-#5 this was ~6 (every catalog table, full-scanned). assert!( - io.data_open_count <= 3, - "merge of a 1-Person delta opened {} data tables; expected <= 3 (Δ-scoped). \ + io.data_open_count <= 4, + "merge of a 1-Person delta opened {} data tables; expected <= 4 (Δ-scoped, including exact confirmation). \ Pre-#5 it opened every catalog table (~6) via a whole-graph validation scan.", io.data_open_count ); diff --git a/docs/dev/invariants.md b/docs/dev/invariants.md index f2e94791..3e602e7a 100644 --- a/docs/dev/invariants.md +++ b/docs/dev/invariants.md @@ -258,10 +258,15 @@ them explicit. schema-apply lock branch) — design it before promoting multi-process write topologies. - **Fork ownership is durable, but Lance ref deletion is not conditional:** a - first-touch Mutation/Load table no longer creates its target ref while - preparing. Under schema → branch → table gates it revalidates, durably arms a - schema-v3 sidecar naming the target, creates the ref, then stages branch-local - files and commits. Both `reclaim_orphaned_fork_and_refork` and + first-touch Mutation/Load or BranchMerge table never creates its target ref + before recovery ownership is durable. Under schema → branch → table gates it + revalidates and arms a schema-v3 mutation/load or schema-v4 merge sidecar + naming the target; branch merge additionally records the exact fork version + and a pre-minted ordered transaction chain for every logical data effect, + commits that chain without transparent conflict retries, and confirms the + minted target `BranchIdentifier`. Recovery proves the exact chain rather than + treating numeric HEAD movement as ownership. Both + `reclaim_orphaned_fork_and_refork` and `reconcile_orphaned_branches` consult pending sidecars before destruction; a foreign claim is conflict/indeterminate, never permission to delete. Full recovery accepts sidecar-before-ref crashes. When `BranchContents` is absent, diff --git a/docs/dev/rfc-004-cluster-graph-schema-apply.md b/docs/dev/rfc-004-cluster-graph-schema-apply.md index c6a6d925..83d9df0a 100644 --- a/docs/dev/rfc-004-cluster-graph-schema-apply.md +++ b/docs/dev/rfc-004-cluster-graph-schema-apply.md @@ -36,7 +36,7 @@ The implementation spec's hard gate for this phase — failpoint recovery tests What Phase 4 builds on (all shipped): -- **The engine's recovery discipline.** Writers that can advance Lance HEAD before manifest publish write `__recovery/{ulid}.json` sidecars carrying per-table pins (`expected_version`, `post_commit_pin`); `Omnigraph::open` in read-write mode classifies every pinned table (`NoMovement` / `RolledPastExpected` / `UnexpectedAtP1` / `UnexpectedMultistep` / `InvariantViolation`) and decides all-or-nothing: roll forward via one manifest publish, or roll back via `Dataset::restore`. Every completed action records an audit row with the interrupted writer's actor in `recovery_for_actor`. A schema-v3 roll-forward publishes the sidecar's pre-minted lineage and therefore preserves that writer's commit id and actor; rollback and legacy recovery commits use `omnigraph:recovery`. The cluster inherits the *vocabulary* of this design but not its mechanics — see the roll-forward-only argument below. +- **The engine's recovery discipline.** Writers that can advance Lance HEAD before manifest publish write `__recovery/{ulid}.json` sidecars carrying per-table pins (`expected_version`, `post_commit_pin`); `Omnigraph::open` in read-write mode classifies every pinned table (`NoMovement` / `RolledPastExpected` / `UnexpectedAtP1` / `UnexpectedMultistep` / `InvariantViolation`) and decides all-or-nothing: roll forward via one manifest publish, or roll back via `Dataset::restore`. Every completed action records an audit row with the interrupted writer's actor in `recovery_for_actor`. Schema-v3 Mutation/Load and schema-v4 BranchMerge roll-forward publish the sidecar's pre-minted lineage and therefore preserve that writer's commit id and actor; v4 additionally proves each multi-commit data effect from its ordered pre-minted Lance transaction chain. Rollback and legacy recovery commits use `omnigraph:recovery`. The cluster inherits the *vocabulary* of this design but not its mechanics — see the roll-forward-only argument below. - **The engine's schema-apply surface.** `apply_schema_as(desired_source, SchemaApplyOptions { allow_data_loss }, actor)` returns `SchemaApplyResult { supported, applied, manifest_version, steps }`; `preview_schema_apply_with_options` returns the migration plan plus desired catalog without applying; the `__schema_apply_lock__` branch serializes schema applies graph-wide and refuses to run while user branches exist. Policy enforcement (`enforce(SchemaApply, TargetBranch("main"), actor)`) happens before the lock. - **Graph init.** `Omnigraph::init(uri, schema_source)` with a strict preflight (errors if schema artifacts exist) and an atomic `_schema.pg` claim. A documented gap: a failed init does not clean up Lance datasets or `__manifest/` it already created. - **No engine graph-delete primitive.** Deleting a graph today means removing its object-store prefix. This RFC works with that fact rather than waiting on a primitive. diff --git a/docs/dev/rfc-022-027-architecture-review.md b/docs/dev/rfc-022-027-architecture-review.md index b14dd0f7..cdb07e87 100644 --- a/docs/dev/rfc-022-027-architecture-review.md +++ b/docs/dev/rfc-022-027-architecture-review.md @@ -432,8 +432,9 @@ retaining the public lifecycle status `Proposed`. **Affected:** [RFC-022 §6.2](../rfcs/rfc-022-unified-write-path.md#62-branch-merge) -**Status:** Closed in specification on 2026-07-11; full merge-adapter conversion -remains rollout work rather than an architecture ambiguity. +**Status:** Closed in specification and implementation on 2026-07-11. Branch +merge captures an immutable source commit and revalidates only its incarnation; +the target publishes and recovers under its own exact authority token. RFC-022 requires every `ReadSet` member to be arbitrated atomically by the publish CAS. A CAS on reserved main cannot arbitrate a row on a named source @@ -516,6 +517,14 @@ performance. > multi-process native-ref fencing. RFC-024 remains the false-contention > narrowing step. +> **Branch-merge implementation disposition (2026-07-11):** branch merge uses +> the same coarse target token without pretending the source belongs to that +> atomic read set. Schema-v4 recovery persists the captured source parent, +> fixed merge/rollback ids, pre-minted exact transaction chains for multi-commit +> data effects, ref-only physical effects, and the complete logical delta. A later source advance remains harmless; a target +> advance after effects becomes `RecoveryRequired` and is compensated rather +> than re-parented. + ### TIGHTENING-01 — symmetric Lance conflicts are not obviously an activation gate RFC-023 correctly identifies Lance's directional filtered/unfiltered behavior. diff --git a/docs/dev/testing.md b/docs/dev/testing.md index 591ed9a9..1e173222 100644 --- a/docs/dev/testing.md +++ b/docs/dev/testing.md @@ -52,9 +52,9 @@ The engine's `tests/` is the principal coverage surface; most graph-shaped behav | `merge_cost.rs` | Cost-budget tests for branch MERGE on the shared `helpers::cost` harness: `merge_validation_is_delta_scoped` (a 1-row-delta merge opens ≤3 data tables — Δ-scoped, not the whole catalog; was ~6 pre-#5) and `merge_manifest_cost_grows_with_history` (the cross-branch `__manifest` open amplification still grows with commit depth — a separate, not-yet-addressed term — while validation `data_open_count` stays flat) | | `policy_engine_chassis.rs` | Engine-layer Cedar enforcement (MR-722): allow + deny through every `_as` writer via the SDK directly — no HTTP — proving embedded and CLI callers hit the same gate as the server, with action × scope shapes matching `authorize_request` | | `maintenance.rs` | `optimize` (compaction), `repair` (explicit uncovered-drift publish), and `cleanup` (version GC): empty/idempotent/no-op edges, policy validation, head preservation; cleanup pins exact keep-count behavior (including keep larger than history), count/time retention of a live lazy branch, the oldest of multiple lazy pins, graph-wide fail-closed ordering on an unopenable pin, and refusal of uncovered main HEAD drift before any GC; `optimize` publishes its own compaction (`optimize_publishes_compaction_to_manifest_so_schema_apply_succeeds`), skips pre-existing uncovered drift (`optimize_skips_preexisting_manifest_head_drift`), refuses to run while a `__recovery` sidecar is pending (`optimize_defers_when_recovery_sidecar_is_pending`), and compacts blob-v2 tables through the normal path (`optimize_compacts_blob_table_alongside_plain_table`); `repair` previews/heals verified maintenance drift, refuses raw semantic drift without `--force`, and forced repair publishes only by explicit operator choice; the index reconciler (iss-848): `index_build_tolerates_null_vector_rows` (logical load succeeds without inline index work; an untrainable Vector column then defers during reconciliation) and `optimize_materializes_index_declared_but_unbuilt` (optimize creates a declared-but-deferred index) | -| `failpoints.rs` | Failure-injection coverage (gated on `failpoints` feature). RFC-022 includes deterministic post-stage/pre-effect races for mutation/load uniqueness and strict disjoint-head changes, plus the cross-handle post-effect `RecoveryRequired` → read-write-open rollback cell. Native controls are pinned by `native_branch_controls_reclassify_lost_acknowledgements` (matching create and absent-ref delete, with no version/lineage movement); `armed_first_touch_recovery_accepts_missing_target_ref` additionally forges and reclaims the clone-only/no-`BranchContents` table state. Legacy path overlap has both sides pinned: `armed_first_touch_recovery_defers_legacy_path_overlap_until_leaf_delete` permits open only for a proven no-effect intent, while `partial_first_touch_recovery_fails_closed_on_legacy_path_overlap` leaves one exact multi-table effect and verifies open fails closed until offline leaf cleanup lets rollback converge. Other control/recovery race cells include `first_touch_post_create_open_error_keeps_recovery_ownership`, `branch_delete_orphans_sidecar_armed_after_initial_barrier`, `branch_merge_fences_target_delete_recreate_aba`, `branch_merge_fences_concurrent_sync_on_same_handle`, `branch_merge_rejects_fresh_target_manifest_change_before_effects`, `branch_merge_rechecks_late_sidecar_after_table_gates`, `cleanup_rechecks_sidecars_under_gc_gates`, and `full_recovery_rereads_sidecar_body_after_discovery`. The suite also includes the five per-writer effect → manifest-CAS recovery tests, write-entry in-process heal contract, storage-fault matrix, S3 recovery twin, and convergence-idempotent roll-forward regression. | +| `failpoints.rs` | Failure-injection coverage (gated on `failpoints` feature). RFC-022 includes deterministic post-stage/pre-effect races for mutation/load uniqueness and strict disjoint-head changes, plus the cross-handle post-effect `RecoveryRequired` → read-write-open rollback cell. Branch merge adds the captured-source advance cell; post-confirm target-winner compensation; mixed physical + pointer-only delta recovery with fixed commit id/actor/parents; and both sidecar-before-first-ref and ambiguous-ref-create recovery. Native controls are pinned by `native_branch_controls_reclassify_lost_acknowledgements` (matching create and absent-ref delete, with no version/lineage movement); `armed_first_touch_recovery_accepts_missing_target_ref` additionally forges and reclaims the clone-only/no-`BranchContents` table state. Legacy path overlap has both sides pinned: `armed_first_touch_recovery_defers_legacy_path_overlap_until_leaf_delete` permits open only for a proven no-effect intent, while `partial_first_touch_recovery_fails_closed_on_legacy_path_overlap` leaves one exact multi-table effect and verifies open fails closed until offline leaf cleanup lets rollback converge. Other control/recovery race cells include `first_touch_post_create_open_error_keeps_recovery_ownership`, `branch_delete_orphans_sidecar_armed_after_initial_barrier`, `branch_merge_fences_target_delete_recreate_aba`, `branch_merge_fences_concurrent_sync_on_same_handle`, `branch_merge_rejects_fresh_target_manifest_change_before_effects`, `branch_merge_rechecks_late_sidecar_after_table_gates`, `cleanup_rechecks_sidecars_under_gc_gates`, and `full_recovery_rereads_sidecar_body_after_discovery`. The suite also includes the five per-writer effect → manifest-CAS recovery tests, write-entry in-process heal contract, storage-fault matrix, S3 recovery twin, and convergence-idempotent roll-forward regression. | | `failpoint_names_guard.rs` | Source-walk guard: every `maybe_fail(...)` call site (engine + cluster) must reference the compile-checked `failpoints::names` consts, never a bare string literal — a typo'd literal compiles but silently never fires; same defense-in-depth shape as `forbidden_apis.rs` | -| `recovery.rs` | Open-time recovery sweep — legacy sidecars plus schema-v3 Mutation/Load exact transaction identity, fixed logical/rollback outcome IDs, branch-token comparison, fresh under-gate sidecar reread/reparse, all-or-nothing roll-forward/rollback/refusal, audit row in `_graph_commit_recoveries.lance`, and `OpenMode::ReadOnly` skip path | +| `recovery.rs` | Open-time recovery sweep — legacy sidecars; schema-v3 Mutation/Load exact transaction identity; schema-v4 BranchMerge exact multi-commit chains/ref-only effects plus complete logical delta; restartable compensation recognition; fixed logical/rollback outcome IDs; branch-token comparison; fresh under-gate sidecar reread/reparse; all-or-nothing roll-forward/rollback/refusal; audit row in `_graph_commit_recoveries.lance`; and `OpenMode::ReadOnly` skip path | | `composite_flow.rs` | Compositional/narrative end-to-end stories — multi-step flows that compose mechanics covered by other test files. Catches integration regressions where individual operations all pass their unit tests but their composition breaks (sequential merges, post-merge main writes, time-travel through merge DAG, reopen consistency over multi-merge histories, post-optimize and post-cleanup strict writes). | ## Fixtures diff --git a/docs/dev/writes.md b/docs/dev/writes.md index 185fff1d..baf7bbcd 100644 --- a/docs/dev/writes.md +++ b/docs/dev/writes.md @@ -75,38 +75,55 @@ The native branch identifier detects delete/recreate ABA but is not a Lance conditional-ref fence, and destructive recovery remains unsafe beside a live foreign process. -### Branch-merge authority fence (adapter bridge) +### Branch-merge authority and recovery adapter (RFC-022 v4) -Branch merge still uses its writer-specific multi-commit table effects and -confirmation sidecar; it has not yet been converted to the RFC-022 exact-effect -adapter. It does, however, join the closed control boundary needed by this first -slice: after the strict recovery barrier it acquires the root-shared schema gate -and the sorted source/target branch gates, performs the final sidecar check, -loads one operation-local catalog from the accepted contract, captures both graph -heads plus the base/source/target snapshots, and holds those gates through table -effects and manifest publication. Planning stays outside table queues. Before -Phase A, merge acquires the conservative all-catalog table envelope for both -source and target, re-lists sidecars, and compares fresh source/target manifest -versions with the captured snapshots. A stale warm handle catalog or coordinator -snapshot is never accepted as that revalidation. +Branch merge retains its writer-specific row classifier and multi-commit table +algorithms, but its authority, recovery, and visibility boundary now use the +RFC-022 adapter contract: -The source snapshot is a captured merge input, not authority that the target -manifest CAS can arbitrate. The current process-local source gate is a stronger -same-process fence around that capture, including delete/recreate ABA, but the -semantic contract is still "merge the captured source commit." A later source -advance does not invalidate an otherwise prepared target publish. Claiming -"latest source at target publish" would instead require a cross-process source -fence held through the target CAS. +1. capture source and target as coherent `WriteTxn` snapshots. The target token + is `(BranchIdentifier, exact optional graph_head, accepted schema identity)`; + the effective lineage head is captured separately because a fresh named + branch can inherit a parent while its own `graph_head:` row is absent; +2. compute the merge base from those captured commit ids and classify against + the immutable base/source/target snapshots outside table gates; +3. acquire the conservative all-catalog source/target table envelope, re-list + recovery intent, revalidate the complete target token, and revalidate the + source incarnation. A target change returns typed `ReadSetChanged` before + effects. A later source-head advance is allowed: the contract is "merge the + captured source commit," never "substitute whatever source is latest"; +4. pre-mint the merge lineage and each table's ordered Lance data-transaction + chain, then arm a schema-v4 BranchMerge sidecar before the first HEAD advance + or first-touch table ref. Logical data steps commit with those exact + `(read_version, uuid)` identities and zero transparent conflict retries. Its + physical-effect set can be smaller than its intended manifest delta: + pointer-only table updates are still recorded so recovery publishes the + complete logical merge; +5. after every multi-commit table effect completes, confirm exact final table + versions, every logical `SubTableUpdate`, and every first-touch target + `BranchIdentifier`; then publish once with `ExactGraphHead` and the captured + table expectations. -That fence prevents a same-process target delete/recreate from reusing the branch -name underneath a merge plan. The race test deliberately recreates a target with -the same name and numeric Lance version but a different `BranchIdentifier`, so -version-only checking cannot accidentally satisfy it. This is a process-local -bridge, not a cross-process conditional-ref primitive and not a substitute for -the later full branch-merge read-set/reprepare adapter. `sync_branch` joins the -same root schema gate before replacing a handle's coordinator, so it cannot -overwrite merge's temporary target coordinator or change a native control's -active-branch authority mid-operation. +Publisher retries cannot re-parent the prepared merge onto a newer target. Any +failure after the v4 sidecar is durable returns `RecoveryRequired`. Full recovery +rolls confirmed effects forward only while the captured target authority still +matches; otherwise it compensates the owned effects while preserving the target +winner, or fails closed when foreign/interleaved table state makes compensation +unverifiable. An Armed first-touch ref with no data HEAD movement is reclaimed +without manufacturing rollback lineage. Armed recovery accepts only a +contiguous prefix of the pre-minted data chain. Rebuildable `CreateIndex` +transactions may follow only the complete chain and are rollback-discardable +derived state; any other, unreadable, or non-contiguous transaction fails +closed. A compensating Lance `Restore` is also recognized by its exact target so +a crash after restore but before the manifest publish resumes without restoring +again. + +The handle-local coordinator swap and `merge_exclusive` mutex remain an +implementation detail until target-context extraction lands; neither is treated +as persistent authority. Native ref create/delete still lack conditional CAS, so +first-touch destructive recovery retains the documented single-writer-process +boundary. `sync_branch` continues to join the schema gate and cannot replace the +temporary coordinator during a merge. ### Branch-delete orphaning exception @@ -334,16 +351,19 @@ are left at `Lance HEAD = manifest_pinned + 1`. post_commit_pin)` it intends to commit + the writer kind + actor_id. For a first-touch named-branch Mutation/Load table, Phase A is followed by - target-ref creation and branch-local `stage_*`; the sidecar already carries - its pre-minted transaction identity. + target-ref creation and branch-local `stage_*`; the schema-v3 sidecar already + carries its pre-minted transaction identity. Branch merge uses schema v4: + it distinguishes multi-commit HEAD effects from ref-only forks, records each + multi-commit effect's ordered exact transaction chain, and records the + complete intended manifest delta, including pointer-only slots. 2. **Phase B**: writer's per-table `commit_staged` loop runs. - - **Phase-B confirmation:** a `BranchMerge` writer - advances each table's HEAD by *several* commits (append → upsert → - delete), so a bare "HEAD moved" is ambiguous — it could be a complete - publish or one crashed mid-sequence. After the whole per-table loop - finishes, the writer re-writes the sidecar stamping each pin's - `confirmed_version` with the exact achieved version, then proceeds to - Phase C. Schema-v3 Mutation/Load sidecars also confirm: each table must + - **Phase-B confirmation:** a schema-v4 `BranchMerge` writer + advances each table's HEAD by *several* exact commits (append → upsert → + delete). Recovery proves a contiguous prefix of the pre-armed transaction + chain rather than inferring ownership from numeric HEAD movement. After the + whole per-table loop finishes, the writer atomically confirms each exact + achieved version, the complete logical manifest delta, and first-touch ref + identities, then proceeds to Phase C. Schema-v3 Mutation/Load sidecars also confirm: each table must match the staged Lance transaction's `(read_version, uuid)`, and the sidecar records the exact `SubTableUpdate` plus original lineage intent. This is the commit point of the recovery WAL: a crash *after* confirmation @@ -371,9 +391,16 @@ recovery sweep in `crates/omnigraph/src/db/manifest/recovery.rs`: Lance HEAD to the manifest pin. Classify per the all-or-nothing decision tree (RolledPastExpected / NoMovement / UnexpectedAtP1 / UnexpectedMultistep / IncompletePhaseB / InvariantViolation). For a - `BranchMerge` sidecar, a moved HEAD with no `confirmed_version` classifies - as `IncompletePhaseB` (a partial multi-commit publish) and forces roll-back; - with a `confirmed_version`, roll-forward targets exactly that version. + legacy `BranchMerge` sidecar, a moved HEAD with no `confirmed_version` + classifies as `IncompletePhaseB` (a partial multi-commit publish) and forces + roll-back; with a `confirmed_version`, roll-forward targets exactly that + version. Schema-v4 BranchMerge recovery additionally requires the captured + target token, fixed original/rollback lineage ids, the exact ordered data + transaction chains, exact confirmed physical effects, first-touch ref + identities, and the complete confirmed manifest delta. A changed target token + is rollback-only and can never re-parent the merge onto the winner. Recovery + refuses a foreign or non-contiguous transaction instead of restoring through + it, and recognizes an already-landed exact compensation restore on restart. Schema-v3 Mutation/Load additionally requires `EffectsConfirmed`, the exact Lance transaction identity at the confirmed version, the original immutable manifest delta, and a matching captured authority token. A changed token is @@ -411,9 +438,10 @@ recovery sweep in `crates/omnigraph/src/db/manifest/recovery.rs`: - After a successful roll-forward or roll-back, an internal `_graph_commit_recoveries.lance` row records `recovery_kind`, `recovery_for_actor` (the original sidecar's actor), `operation_id`, and - exact per-table outcomes. A v3 roll-forward publishes the interrupted - writer's fixed lineage intent, including its original actor; rollback and - legacy recovery commits use `actor_id = "omnigraph:recovery"`. Ordinary + exact per-table outcomes. Schema-v3 Mutation/Load and schema-v4 BranchMerge + roll-forward publish the interrupted writer's fixed lineage intent, + including its original actor; rollback and legacy recovery commits use + `actor_id = "omnigraph:recovery"`. Ordinary commit history is therefore not a complete recovery enumeration, and the CLI currently has no public query for the recovery-audit table. - Sidecar deleted as the final step. @@ -469,7 +497,7 @@ guard says so explicitly ("a pending recovery sidecar requires rollback — reopen the graph read-write") instead of pointing at `omnigraph repair`, which refuses while a sidecar is pending. `cleanup` refuses pending sidecars at entry as well, before orphan reconciliation -or version GC: v3 ownership and compensation recovery may need the retained +or version GC: v3/v4 ownership and compensation recovery may need the retained Lance transaction/version history, so garbage collection cannot outrun the recovery barrier. Continuous in-process recovery for the rollback path is the goal of a @@ -477,15 +505,16 @@ future background reconciler. `ensure_indices` does not heal at entry itself; it is an explicit maintenance/reconciliation call, separate from mutation, load, and schema apply, and its strict preconditions fail loudly on drift. -For enrolled mutation/load, the publisher rechecks the attempt's exact native -branch identity and `graph_head` as well as the touched-table versions. A +For enrolled mutation/load and branch merge, the publisher rechecks the +attempt's exact native branch identity and `graph_head` as well as table +expectations. A concurrent graph commit anywhere on the target branch therefore invalidates the prepared authority instead of silently reparenting it. Before effects, an insert-only mutation or Append/Merge load fully reprepares with a bounded retry; strict -Update/Delete/Overwrite returns `ReadSetChanged`; after any effect, any later -error returns `RecoveryRequired` and leaves the fixed v3 intent durable. Legacy -writers still arbitrate only their explicit touched-table expectations until -their adapters are enrolled. +Update/Delete/Overwrite and branch merge return `ReadSetChanged`; after any +effect, any later error returns `RecoveryRequired` and leaves the fixed v3/v4 +intent durable. Schema apply and optimize/index remain on their writer-specific +arbitration until their adapters are enrolled. **Sidecar I/O failure semantics** (all sidecar I/O goes through the backend-generic `StorageAdapter`; the contracts below are pinned by the diff --git a/docs/rfcs/rfc-022-unified-write-path.md b/docs/rfcs/rfc-022-unified-write-path.md index 3a84e8bd..d1031286 100644 --- a/docs/rfcs/rfc-022-unified-write-path.md +++ b/docs/rfcs/rfc-022-unified-write-path.md @@ -694,14 +694,19 @@ Implementation proceeds in this order: > **Implementation note (2026-07-11):** mutation/load now use this coarse > token, schema-v3 exact-effect sidecars, fixed lineage/rollback outcome ids, > zero transparent Lance commit retries, and bounded full reprepare before - > effects. Branch merge remains on its writer-specific multi-commit path, but - > now holds the root-shared schema plus source/target branch gates from its - > strict recovery barrier and authority capture through publication. It plans - > with an accepted-contract catalog captured under that schema gate, then - > acquires all catalog table gates for source and target, re-lists recovery, - > and compares fresh manifest versions before Phase A. This closes - > same-process delete/recreate ABA and legacy table-only-writer races while - > its full exact-effect adapter remains future work. Schema apply, + > effects. Branch merge now captures an immutable source commit/snapshot and + > the target coarse token, computes its merge base from those captured ids, + > and revalidates target authority plus source incarnation under the ordered + > gates. Its schema-v4 recovery envelope distinguishes multi-commit HEAD + > effects from first-touch ref-only forks, persists an ordered pre-minted + > Lance transaction chain for every logical data effect plus fixed + > merge/rollback lineage, and confirms the complete manifest delta (including + > pointer-only slots) before publishing with `ExactGraphHead`. Those data + > transactions commit with zero transparent conflict retries; recovery accepts + > only their contiguous exact prefix (plus a derived `CreateIndex` tail after + > the complete chain) and fails closed on foreign movement. A pre-effect target change + > returns `ReadSetChanged`; any post-arm failure returns `RecoveryRequired` + > and recovery never re-parents onto a target winner. Schema apply, > optimize/index, and MemWAL fold remain on their writer-specific paths until > their adapter slices land. diff --git a/docs/user/branching/transactions.md b/docs/user/branching/transactions.md index bd559010..246af07c 100644 --- a/docs/user/branching/transactions.md +++ b/docs/user/branching/transactions.md @@ -19,7 +19,7 @@ Two primitives, two scopes: | **One `.gq` query** (any number of statements inside) | The query itself — handled by the publisher's atomic manifest commit | Yes — all statements land together or none of them do | The publisher never publishes; target unchanged | | **Many queries that must succeed together** | Branches: `branch_create` → run N queries on the branch → `branch_merge` | Yes — the merge is a single atomic publish | Drop the branch (`branch_delete`); main is unaffected | -Snapshot isolation is per-query — every read inside one query sees one consistent manifest version. Two concurrent queries on the same branch see independent snapshots. Mutation/load capture the branch head as coarse OCC authority, so a prepared plan is never silently reparented after another graph commit. +Snapshot isolation is per-query — every read inside one query sees one consistent manifest version. Two concurrent queries on the same branch see independent snapshots. Mutation/load and branch merge capture the target branch head as coarse OCC authority, so a prepared plan is never silently reparented after another graph commit. Merge additionally pins the exact source commit it classified; a later source advance is not substituted into the prepared result. ## Comparison with `BEGIN` / `COMMIT` diff --git a/docs/user/reference/constants.md b/docs/user/reference/constants.md index f3324912..291e7c09 100644 --- a/docs/user/reference/constants.md +++ b/docs/user/reference/constants.md @@ -5,6 +5,7 @@ | `MANIFEST_DIR` | `__manifest` | manifest layout | | Commit graph dirs (retired) | `_graph_commits.lance` / `_graph_commit_actors.lance` | retired in Phase B; lineage lives in `__manifest` (`graph_commit` / `graph_head` rows) since RFC-013 Phase 7. A graph this binary creates has neither. | | Recovery audit dir | `_graph_commit_recoveries.lance` | internal exact record of completed crash-recovery actions; no public CLI query yet | +| Exact recovery history-scan ceiling | `MAX_EFFECT_IDENTITY_SCAN_VERSIONS = 1024` | v3/v4 recovery fails closed as unverifiable rather than scanning an unbounded Lance transaction history; a v4 logical merge chain is at most three commits today, with only derived `CreateIndex` commits allowed after it | | Run branch prefix (legacy, removed) | `__run__` | pre-v0.4.0 Run state machine; no longer a reserved name. A graph still carrying `__run__*` branches is sub-v4 and refused on open (rebuild via export/import). | | Schema apply lock | `__schema_apply_lock__` | schema apply | | Manifest publisher retry budget | `PUBLISHER_RETRY_BUDGET = 5` | manifest publish |