From 6c1bc36b2cf2a98a5170f79a4c9b27f0709d0fea Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Sat, 18 Jul 2026 17:27:22 +0100 Subject: [PATCH 1/2] Add branch-control cost gate for delete's per-branch dependency scans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit branch_delete must prove no surviving branch still references the deleted branch's per-table forks. That check needs one manifest-only snapshot (one __manifest open + state scan) per surviving branch, so the new branch_control_cost.rs gate bounds the SLOPE of the delete's __manifest reads/opens per surviving branch on the shared helpers::cost harness. Red at this commit by design (the rule-12 red half of the pair): the current implementation performs a full cold target resolve per foreign branch — manifest state scan PLUS commit-lineage scan plus a schema-contract re-read/re-validate — measuring exactly 2 reads and 2 internal opens per surviving branch against a budget of 1 (+2 slack). The following commit converges the dependency check on the manifest-only snapshot and turns this gate green. --- crates/omnigraph/tests/branch_control_cost.rs | 109 ++++++++++++++++++ docs/dev/testing.md | 1 + 2 files changed, 110 insertions(+) create mode 100644 crates/omnigraph/tests/branch_control_cost.rs diff --git a/crates/omnigraph/tests/branch_control_cost.rs b/crates/omnigraph/tests/branch_control_cost.rs new file mode 100644 index 00000000..c922a57b --- /dev/null +++ b/crates/omnigraph/tests/branch_control_cost.rs @@ -0,0 +1,109 @@ +//! Cost-budget tests for native branch CONTROL operations (create/delete) on the +//! shared `helpers::cost` harness — the branch-op sibling of `write_cost.rs` +//! (mutations) and `merge_cost.rs` (merge). +//! +//! The gated term: `branch_delete` must prove no surviving branch still +//! references the deleted branch's per-table forks. That dependency check reads +//! ONLY each foreign branch's manifest `table_branch` entries, so its per-branch +//! cost budget is ONE manifest-only snapshot read (`__manifest` open + state +//! scan). The pre-fix implementation instead performed a full cold target +//! resolve per foreign branch — manifest state scan PLUS commit-lineage scan +//! plus a schema-contract re-read/re-validate — doubling the `__manifest` read +//! slope and making deletion O(branches × history) on un-compacted graphs. +//! +//! Like the other cost tests, the body runs on a 64 MiB-stack thread: the +//! debug-build engine futures plus the `cost_harness`/`measure` task-local +//! layers overflow the default 2 MiB test stack. +#![recursion_limit = "512"] + +mod helpers; + +use std::future::Future; + +use helpers::cost::{IoCounts, cost_harness, local_graph, measure}; + +/// Run an async test body on a thread with a large stack (see module docs). +fn on_big_stack(body: impl FnOnce() -> F + Send + 'static) +where + F: Future, +{ + std::thread::Builder::new() + .stack_size(64 * 1024 * 1024) + .spawn(move || { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap() + .block_on(body()); + }) + .unwrap() + .join() + .unwrap(); +} + +/// Per-foreign-branch budget for the delete dependency check: ONE manifest-only +/// snapshot (one `__manifest` open + state scan) per surviving branch. On this +/// fixture that is exactly 1 read iop and 1 internal open per branch; the full +/// cold resolve (state scan + lineage scan per branch) measures exactly 2 of +/// each, so a regression re-trips this gate. `SLOPE_SLACK` absorbs incidental +/// constant-ish noise without admitting a second per-branch open. +const DELETE_PER_BRANCH_BUDGET: u64 = 1; +const SLOPE_SLACK: u64 = 2; + +/// `branch_delete`'s dependency check visits every surviving branch, so its +/// `__manifest` reads necessarily scale with branch count — this gate bounds the +/// SLOPE (reads per surviving branch), not the total. Sibling branches (created +/// straight off main, never written) keep every other delete precondition +/// identical across the sweep: no path-prefix children, no lineage descendants, +/// and constant `__manifest` content (native branch create/delete emits no +/// lineage rows), so the fixed per-delete overhead cancels in the delta and the +/// slope isolates the per-branch dependency-check cost. +#[test] +fn branch_delete_manifest_reads_bounded_per_surviving_branch() { + on_big_stack(|| { + cost_harness(async { + let dir = tempfile::tempdir().unwrap(); + let db = local_graph(&dir).await; + + let mut curve: Vec<(u64, IoCounts)> = Vec::new(); + let mut created = 0u64; + for n in [4u64, 12] { + while created < n { + db.branch_create(&format!("sib_{created:02}")) + .await + .unwrap(); + created += 1; + } + let victim = format!("victim_{n}"); + db.branch_create(&victim).await.unwrap(); + let (res, io) = measure(db.branch_delete(&victim)).await; + res.unwrap(); + eprintln!( + "surviving branches={} (+main): DELETE manifest_reads={} data_reads={} \ + internal_open_count={}", + n, io.manifest_reads, io.data_reads, io.internal_open_count + ); + curve.push((n, io)); + } + + let (n_lo, n_hi) = (curve[0].0, curve[1].0); + let added_branches = n_hi - n_lo; + let budget = added_branches * DELETE_PER_BRANCH_BUDGET + SLOPE_SLACK; + let read_delta = curve[1] + .1 + .manifest_reads + .saturating_sub(curve[0].1.manifest_reads); + let open_delta = curve[1] + .1 + .internal_open_count + .saturating_sub(curve[0].1.internal_open_count); + assert!( + read_delta <= budget && open_delta <= budget, + "branch_delete __manifest cost grew {read_delta} reads / {open_delta} opens \ + across {added_branches} added surviving branches (budget {budget}) — the \ + dependency check must read one manifest-only snapshot per surviving branch, \ + not a full cold resolve (state + lineage scans + schema contract) per branch", + ); + }) + }); +} diff --git a/docs/dev/testing.md b/docs/dev/testing.md index cec53b43..c15f5a7a 100644 --- a/docs/dev/testing.md +++ b/docs/dev/testing.md @@ -51,6 +51,7 @@ The engine's `tests/` is the principal coverage surface; most graph-shaped behav | `lance_version_columns.rs` | Per-row `_row_last_updated_at_version` behavior | | `validators.rs` | Schema constraint enforcement (enum, range, unique, cardinality) across JSONL load, mutation insert/update. ALL THREE write surfaces — mutation, bulk load, AND merge — route through the unified `crate::validate` evaluator (Δ-scoped, index-backed, reusing these leaf checks). Cross-version-uniqueness closure: `cross_version_unique_rejected_on_mutation_insert` + `reinsert_existing_key_is_upsert_not_unique_violation` (mutation path); `cross_version_unique_rejected_on_append_load` + `merge_load_reupsert_existing_key_is_not_unique_violation` (load path). Per-table `Overwrite`: `overwrite_load_validates_ri_against_new_image` (an edges-only overwrite still resolves RI against retained committed nodes) + `append_load_rejects_orphan_edge`. The evaluator's own unit tests live in `src/validate.rs` (`#[cfg(test)]`); its merge-conflict equivalence is pinned by `merge_truth_table.rs` (OrphanEdge) + `branching.rs` (Unique/Cardinality merge tests). Intra-batch duplicate-`@key` rejection on every load mode is pinned by `consistency.rs::loader_rejects_intra_batch_duplicate_keys`; the mutation-coalesce counterpart (insert+update / chained updates of one id are NOT a self-collision) by `writes.rs`. Non-String `@unique` columns probe committed state with a TYPED literal (not a stringified key): `cross_version_unique_rejected_on_date_column` + `noncolliding_write_to_date_unique_column_succeeds` (a `Date @unique` collision is a proper `@unique` violation, and a distinct value does not raise a Date32-vs-Utf8 coercion error). Cardinality is keyed by edge id, last-wins (matching commit's `dedupe_merge_batches_by_id`): `merge_load_edge_src_move_rechecks_vacated_src_cardinality` (a Merge-load moving an edge recounts the vacated src for `@card` min) + `merge_load_duplicate_edge_id_counts_once_per_card` (a dup edge id under two srcs in one batch counts once, no spurious max violation). Direct deletes capture the ids they remove (from the delete op's own scan) into the change-set's `deleted_ids`, so a delete emptying a src is validated: `mutation_delete_edge_below_card_min_rejected` (a `delete Edge` dropping a src below `@card` min is rejected, not silently committed). | | `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) | +| `branch_control_cost.rs` | Cost-budget tests for native branch CONTROL ops on the shared `helpers::cost` harness: `branch_delete_manifest_reads_bounded_per_surviving_branch` gates the SLOPE of `branch_delete`'s `__manifest` reads per surviving branch — the delete dependency check reads one manifest-only snapshot per foreign branch, never a full cold resolve (state + lineage scans + schema-contract re-read) per branch | | `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` | `ensure_indices`, `optimize` (compaction), `repair` (explicit uncovered-drift publish), and `cleanup` (version GC): empty/idempotent/no-op edges, policy validation, head preservation. EnsureIndices refuses uncovered drift before arming its identity-bearing v9 envelope and keeps untrainable Vector work pending. Cleanup pins exact keep-count behavior, lazy-branch retention, graph-wide fail-closed ordering, and refusal of uncovered main HEAD drift before GC. Optimize's bounded payload inside the v9 envelope publishes multiple productive data tables through one graph commit, emits no lineage/sidecar at steady state, skips uncovered drift, refuses pending recovery, and compacts blob-v2 tables. Repair previews/heals verified maintenance drift and requires `--force` for semantic drift | | `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; both sidecar-before-first-ref and ambiguous-ref-create recovery; and an 8,193-delete between-chunk crash proving an `Armed` exact-transaction prefix is rolled back before the successful retry. Identity-bearing v9 SchemaApply is pinned by `schema_apply_phase_b_failure_recovered_on_next_open` (exact confirmed roll-forward with fixed commit id + initiating actor), `schema_apply_partial_table_effect_rolls_back_exactly` (Armed proper-prefix compensation), `schema_apply_recovery_reclaims_owned_add_type_target_and_retry_succeeds` (strict owned first-touch cleanup), `schema_apply_first_touch_foreign_winner_is_preserved_not_adopted` (foreign unregistered winner preservation), `schema_apply_post_effect_disjoint_winner_is_preserved` (winner-preserving compensation), `schema_apply_post_effect_same_table_winner_fails_closed` (buried-effect refusal), `schema_apply_recovers_partial_schema_promotion_after_commit_crash` (read-only refusal for both valid and corrupt intents in the torn manifest/schema window, followed by fixed-outcome completion of a partial source/IR/state promotion), and `schema_apply_live_query_waits_for_coherent_schema_publication` (same-handle publication wait plus pre-apply-handle query/export/whole-graph-index capture from the operation-local accepted catalog). Metadata-only before/after-staging and rollback-retry cells keep the empty-effect v9 boundary pinned. EnsureIndices v9 recovery retains both boundaries in `recovery_rolls_forward_ensure_indices_on_feature_branch`: the first residual rolls forward on the next read-write open, and a second roll-forward-eligible `EffectsConfirmed` residual under an unchanged captured token is completed by a same-handle retry before new planning. `ensure_indices_complete_armed_effects_roll_back` keeps the authority-clean complete-effect Armed rollback rule isolated, while `ensure_indices_entry_barrier_refuses_partial_armed_before_staging` leaves one of two table effects pending and proves the original `RecoveryRequired` wins before the remaining index can reach the post-stage failpoint. Its remaining cells are `ensure_indices_stage_btree_failure_leaves_existing_tables_writable` (after a clean entry barrier, expensive mixed-index staging remains outside the final authority/gates), `ensure_indices_first_touch_crash_before_ref_recovers_cleanly` (sidecar-before-ref no-effect recovery), `ensure_indices_mixed_first_touch_rollback_does_not_delete_moved_ref` (owned-effect rollback and sibling first-touch cleanup), and the no-work/no-sidecar failpoint cell; the recovery module separately pins existing + first-touch payload round-trip and identity-less-input refusal. Optimize's graph-wide identity-bearing v9 envelope is pinned by `optimize_phase_b_failure_recovered_on_next_open` (two-table roll-forward), `optimize_multi_table_partial_effect_rolls_back_under_one_v2_sidecar` (one shared sidecar, no partial visibility, compensation), `optimize_post_manifest_failure_finalizes_multi_table_v2_sidecar` (lost publish acknowledgement), and `optimize_excludes_pending_only_vector_table_from_v2_sidecar` (pending status cannot poison sibling recovery), plus its late-sidecar/main-gate/retry cells. 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`, `optimize_rechecks_late_schema_apply_sidecar_after_main_gate` (late zero-pin graph-global intent), `optimize_rechecks_late_disjoint_main_sidecar_after_main_gate` (table-disjoint intent sharing `graph_head:main`), `optimize_holds_main_gate_through_disjoint_table_effects` (post-relist branch-gate lifetime), `cleanup_rechecks_sidecars_under_gc_gates`, `full_recovery_rereads_sidecar_body_after_discovery`, `recovery_discovery_skips_sidecar_deleted_after_list` (an unrelated write succeeds after a listed sidecar is published/deleted), and `read_only_recovery_discovery_skips_sidecar_deleted_after_list` (read-only open succeeds against that same concurrent completion). 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. | From ad6c609aeeb2d05867b225a7d27d89e6f92e0d2d Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Sat, 18 Jul 2026 17:28:46 +0100 Subject: [PATCH 2/2] Read manifest-only snapshots in branch delete's dependency check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The branch_delete dependency check visits every surviving branch to prove none still references the deleted branch's per-table forks, but it consumes only each branch's manifest table_branch entries. It previously performed a full cold target resolve per foreign branch — a manifest state scan PLUS a commit-lineage scan plus a schema-contract re-read/re-validate — making deletion O(branches x history) on un-compacted graphs and timing out on large ones. The per-foreign-branch schema validation could also wedge deletion of an unrelated branch behind another branch's schema drift, even though the deleting operation's own schema is already validated under the schema gate. Switch the loop to fresh_snapshot_for_branch_unchecked — the same manifest-only per-branch snapshot cleanup already uses — halving the per-surviving-branch __manifest cost from 2 reads/opens to 1 and dropping the per-branch contract reads and recompiles entirely. Turns the branch_control_cost.rs slope gate green (previous commit, red by design): 19->35 reads / 16->32 opens across 4->12 branches before, 15->23 / 13->21 after. --- crates/omnigraph/src/db/omnigraph.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/crates/omnigraph/src/db/omnigraph.rs b/crates/omnigraph/src/db/omnigraph.rs index 2cccf9c1..02f6a850 100644 --- a/crates/omnigraph/src/db/omnigraph.rs +++ b/crates/omnigraph/src/db/omnigraph.rs @@ -2366,12 +2366,23 @@ impl Omnigraph { ))); } + // Dependency detection reads ONLY each surviving branch's manifest + // `table_branch` entries, so take the manifest-only snapshot. A full + // `snapshot_of` resolve would additionally load the commit-lineage + // projection and re-read + re-validate the schema contract PER BRANCH — + // O(branches x history) I/O that made deletion time out on large + // graphs — and its per-foreign-branch schema validation could wedge + // deletion of an unrelated branch behind another branch's schema + // drift. This operation's own schema was already validated under the + // schema gate above. for other_branch in branches .iter() .filter(|candidate| candidate.as_str() != branch) { let snapshot = self - .snapshot_of(ReadTarget::branch(other_branch.as_str())) + .fresh_snapshot_for_branch_unchecked( + Self::normalize_branch_name(other_branch)?.as_deref(), + ) .await?; if snapshot .entries()