Harden branch recovery and cleanup retention (#344)

* Harden branch recovery and cleanup retention

* Fail closed on blocked partial rollback
This commit is contained in:
Andrew Altshuler 2026-07-11 16:56:40 +03:00 committed by GitHub
parent f758ff0d17
commit bd4c614e42
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
25 changed files with 2227 additions and 213 deletions

View file

@ -198,6 +198,20 @@ findings:
the substrate enforcing what omnigraph's entry-owner resolution always did.
Production opens by owner-resolved location (unaffected); the lazy-fork
namespace test pins both the error and the owner-branch open.
- **Native branch control remains two-phase at beta.15.**
`Dataset::create_branch` commits the shallow-cloned `tree/{branch}` dataset
before validating/writing authoritative `BranchContents`; its random
`BranchIdentifier` element is minted only in that second phase and cannot be
pre-minted through the public API. OmniGraph therefore validates first and
classifies an ambiguous result from exact parent metadata rather than
inventing an identifier. Delete removes `BranchContents` before tree cleanup
and exposes no compare-and-delete primitive. Slash-separated names share
nested physical directories; `force_delete_branch` deliberately leaves an
ancestor tree while a live path-child exists, so live graph names are
path-prefix-disjoint. The public format page does not currently spell out the
identifier field, so the pinned Rust shape remains load-bearing. Guard 9 in
`lance_surface_guards.rs` pins clone-only raw-create failure plus force reclaim;
`src/branch_control.rs` pins delete classification and JSON identity fencing.
- **Native DirectoryNamespace churn** (#7222 removed
`table_version_storage_enabled` + the `__manifest` version-storage
experiment; #7176/#7191/#7234 rewrote manifest handling): the decoupling
@ -287,7 +301,7 @@ Migration from Lance 4.0.0 → 6.0.1 landed in this cycle (DataFusion 52 → 53,
- **Lance #6658 closed** (2026-05-14) but `DeleteBuilder::execute_uncommitted` did **not** ship in v6.0.1 — binary search across the release stream shows it first appears in `v7.0.0-beta.10` (the closing commits landed on main but didn't backport to the 6.x line). Tracked as MR-A: migrate `delete_where` to staged, retire the parse-time D2 mutation rule, extend recovery sidecar coverage. **Gated on the Lance v7.x bump**, not this PR. v7.0.0-rc.1 dropped 2026-05-21.
- **Lance #6666 still open** (`build_index_metadata_from_segments` public): vector-index two-phase blocked; inline `create_vector_index` residual retained.
- **Lance #6877 still open** (`MergeInsertBuilder` dup-rowid): PR #109's `SourceDedupeBehavior::FirstSeen` + `check_batch_unique_by_keys` precondition stay load-bearing.
- **`Dataset::force_delete_branch`** (`branches().delete(name, force=true)`, dataset.rs:524) tolerates a missing branch-*contents* ref (vs plain `delete_branch`'s `RefNotFound`), but on the local store still errors `NotFound` if the branch `tree/` directory is fully absent (`remove_dir_all`'s NotFound is not caught for Lance's native error variant, refs.rs:526-549). Both variants still refuse a branch with referencing descendants (`RefConflict`). `TableStore::force_delete_branch` wraps this to be fully idempotent (tolerates already-absent). The single-authority branch-delete redesign uses it for orphan reclamation (eager best-effort reclaim + cleanup reconciler). Pinned by `lance_surface_guards.rs::force_delete_branch_semantics`. Branch delete is "flip the ref atomically, then `remove_dir_all(tree/{branch})`"; branch-exclusive data lives under `tree/{branch}/` so a drop reclaims it immediately without touching `main`.
- **`Dataset::force_delete_branch`** (`branches().delete(name, force=true)`, dataset.rs:524) tolerates a missing branch-*contents* ref (vs plain `delete_branch`'s `RefNotFound`), but on the local store still errors `NotFound` if the branch `tree/` directory is fully absent (`remove_dir_all`'s NotFound is not caught for Lance's native error variant, refs.rs:526-549). Both delete variants still refuse a branch with referencing descendants (`RefConflict`). The current OmniGraph disposition for these still-present substrate behaviors is recorded in the beta.15 audit above.
- **Lance blob-v2 `compact_files` bug** (no public issue found as of 2026-06): `compact_files` disables binary-copy for blob datasets and forces `BlobHandling::AllBinary` on the read side; the v2.1+ structural decoder then mis-counts column infos for the blob-v2 struct and fails with `Invalid user input: there were more fields in the schema than provided column indices / infos` (`lance-encoding/src/decoder.rs::ColumnInfoIter::expect_next`). This fails even a pristine uniform-V2_2 multi-fragment blob table; vector/list/scalar/ragged columns and mixed file versions all compact fine. Reads/queries use descriptor handling (`BlobHandling::default()`) and are unaffected. `optimize` skips blob-bearing tables behind `LANCE_SUPPORTS_BLOB_COMPACTION = false` (`db/omnigraph/optimize.rs`), reporting `SkipReason::BlobColumnsUnsupportedByLance`. Pinned by `lance_surface_guards.rs::compact_files_still_fails_on_blob_columns`, which turns red when the bug is fixed → flip the gate, remove the skip branch + the `maintenance.rs::optimize_skips_blob_table_and_reports_skip` skip assertions.
Surface guards added: `crates/omnigraph/tests/lance_surface_guards.rs` (10 named guards; 5 runtime + 5 compile-only; plus the index-coverage work's `_compile_optimize_indices_signature` and `optimize_indices_extends_fragment_coverage`). Future Lance bumps re-run this file first as the smoke check. Two additional guards from the original plan deferred to follow-up (`manifest_cas_returns_row_level_contention_variant` needs full publisher-race harness; `table_version_metadata_byte_compatible_with_v4` needs `pub(crate)` reach extension).