mirror of https://github.com/ModernRelay/omnigraph.git synced 2026-06-09 01:35:18 +02:00

Ragnor Comerford 353c0c876a

fix(branch): make branch delete correct under partial failure (#137 )

* test(lance): pin force_delete_branch surface guard

Pin the Lance 6.0.1 force_delete_branch behavior the branch-delete
single-authority redesign relies on: plain delete_branch errors on a
missing ref, force_delete_branch removes an existing forked branch, and
the local-store quirk where force_delete on a fully-absent branch still
errors (worked around by the upcoming TableStore::force_delete_branch).

Re-pin the docs/dev/lance.md alignment stanza (9 guards; 4 runtime).

* feat(storage): add force branch-delete to TableStore + CommitGraph

Add TableStore::force_delete_branch and CommitGraph::force_delete_branch
(idempotent: tolerate an already-absent branch via Lance RefNotFound /
NotFound), plus CommitGraph::list_branches for the cleanup reconciler to
diff against the manifest authority. RefConflict (referencing
descendants) is still surfaced. Unused until the branch-delete rewire.

* test(maintenance): red — cleanup reconciles orphaned branch forks

Forge a Lance branch on the Person table that the manifest never
references (a zombie fork from an incomplete prior delete) and assert
cleanup reclaims it while leaving main intact. Fails today: cleanup does
not yet reconcile orphaned forks. Goes green with the next commit.

* fix(maintenance): reconcile orphaned branch forks in cleanup

Add reconcile_orphaned_branches: force_delete_branch every per-table and
commit-graph Lance branch absent from the manifest branch set (the
authority), children-before-parents. Folded into cleanup_all_tables,
runs before version GC. Idempotent and authority-derived; no-ops once
nothing is orphaned, and would harmlessly find nothing if a future Lance
atomic multi-dataset branch op prevented orphans. Adds TableStore::list_branches
and exposes graph_commits_uri(pub crate). Turns the maintenance red test green.

* test(failpoints): red — branch_delete partial failure converges

Add the branch_delete.before_table_cleanup failpoint hook (inert without
the feature) and a regression test: a cleanup-step failure after the
manifest authority flip must leave branch_delete returning Ok, the branch
gone, the orphan stranded, then reclaimed by cleanup, and the name
reusable. Fails today: cleanup_deleted_branch_tables propagates the error
as a hard failure. Goes green with the next commit.

* fix(branch): best-effort fork reclaim after the manifest flip

Make branch_delete treat per-table forks and the commit-graph branch as
derived state reclaimed best-effort with force_delete_branch after the
manifest authority flip. A reclaim failure (transient error, or the
branch_delete.before_table_cleanup failpoint) is logged via tracing::warn
and swallowed: the branch is already gone and the cleanup reconciler
converges the orphan. cleanup_deleted_branch_tables no longer returns an
error or blocks the call. Turns the partial-failure recovery test green.

* test(failpoints): red — recreate over orphaned fork is actionable

After a partial-failure delete leaves a fork orphaned, recreating the
branch name and writing to the previously-forked table before cleanup
runs currently surfaces the opaque ExpectedVersionMismatch ("stale view
... expected manifest table version N"). Assert instead a clear error
pointing the user at cleanup. Goes green with the next commit.

* fix(branch): actionable orphan-collision error in fork_branch_from_state

When a fork's create_branch collides with an existing target ref, reuse
it only if its head matches source_version (a legitimate concurrent
first-write). A version mismatch means a zombie fork from an incomplete
prior delete: return a manifest_conflict pointing the user at
`omnigraph cleanup`, instead of the opaque ExpectedVersionMismatch.
Turns the recreate-over-orphan red test green.

* docs(invariants): single-authority branch-lifecycle + Lance forward-compat

Record branch delete in the Current Truth Matrix: manifest is the single
authority flipped atomically first, per-table forks + commit-graph branch
are derived state reclaimed best-effort with the cleanup reconciler as
backstop, and reusing a name whose reclaim failed surfaces an actionable
error. Note the reconciler is authority-derived and degrades to a no-op
under a future Lance atomic multi-dataset branch op, the same shape as
invariant 7.

* test(failpoints): red — cleanup isolates a single-table failure

Add the cleanup.table_gc failpoint hook (inert without the feature) and
an error: Option<String> field on TableCleanupStats (mechanical, always
None for now). Regression test: a one-shot version-GC failure for one
table must not abort the whole cleanup — assert cleanup still succeeds,
surfaces the failure per-table in stats, and the independent reconcile
pass still reclaimed an orphan. Fails today: the version-GC collect
aborts on the first table error. Goes green with the next commit.

* fix(maintenance): fault-isolate cleanup per table

Make the cleanup sweep do as much as it can and converge on re-run
instead of aborting wholesale on one table's transient error
(invariant 13). The version-GC loop now records a per-table failure on
its stats row (error: Some) and logs it rather than collecting into a
Result that aborts; reconcile_orphaned_branches isolates per-table and
commit-graph failures into BranchReconcileStats.failures. The CLI reports
any failed tables and tells the user to rerun cleanup. Addresses the
Devin review finding. Turns the single-table-failure test green.

* test(failpoints): red — branch_create heals commit-graph zombie + is atomic

Add the branch_delete.before_commit_graph_reclaim failpoint hook and two
regression tests: (a) recreating a name whose delete left a commit-graph
zombie must succeed (today it dies on Lance's internal Clone error), and
(b) branch_create must roll back the manifest branch when the derived
commit-graph branch fails (today it leaves the manifest branch created
while returning Err). Both fail now; green with the next commit. The
existing branch_create_failpoint_triggers test still passes.

* fix(branch): make branch_create atomic + heal commit-graph zombie

branch_create now flips the manifest authority first, then creates the
derived commit-graph branch in create_commit_graph_branch, force-dropping
any orphaned commit-graph ref left by an incomplete prior delete (the
manifest branch is fresh, so a same-named commit-graph branch is provably
a zombie). If commit-graph creation fails, the manifest branch is rolled
back so the name never half-exists. Addresses the Codex review finding.
Turns the two branch_create red tests green; existing tests unaffected.

* test(failpoints): red — fork collision misclassifies live concurrent fork

Add the fork.before_classify failpoint hook and a concurrency test: when
a concurrent first-write legitimately wins the fork race, the loser must
get a retryable refresh-and-retry, not the misleading run-cleanup orphan
error. Today the version-comparison misclassifies the live fork as an
orphan (the Cursor finding). Goes green with the next commit.

* fix(branch): manifest-arbitrated fork-collision classification

Classify a fork collision by the manifest authority instead of comparing
Lance branch versions. Before forking, open_owned_dataset_for_branch_write
re-reads the live manifest: if the table is already forked on the active
branch, a concurrent first-write won and the loser gets a retryable
refresh-and-retry (not a misleading orphan error). fork_branch_from_state
no longer guesses from versions — a create collision past that check is
an orphan, so it returns the actionable cleanup error. Addresses the
Cursor finding; turns the live-concurrent-fork test green, zombie path
unchanged.

* test(failpoints): close branch-lifecycle test gaps

Three coverage additions for the branch-delete work (behavior already
correct; these lock it in and catch regressions):

- cleanup_isolates_reconcile_failure: inject a force-delete failure into
  the reconcile loop (new cleanup.reconcile_fork hook) and assert the
  sweep continues + converges on re-run. Directly covers the reconcile
  loop the Devin finding was about (previously only version-GC was).
- cleanup_reclaims_orphaned_commit_graph_branch: forge a commit-graph
  orphan via the delete reclaim failpoint and assert cleanup's
  reconcile_commit_graph_orphans drops it (previously untested).
- fork_collision_with_live_concurrent_fork_is_retryable: replace the
  fixed 300ms sleep with a deterministic readiness signal (cfg_callback +
  compare_exchange atomics) so the two-writer ordering can't flake.

Full failpoints suite 31/0.

2026-06-01 13:28:38 +02:00

13 KiB

Raw Blame History

Architectural Invariants

Type: standing review checklist Status: living document Audience: anyone proposing, reviewing, or implementing an OmniGraph change

This file is intentionally short. It records the rules that should be in working memory for every non-trivial change. Detailed mechanics live in the area docs linked below.

Use it this way:

Review the change against Hard Invariants and the Deny-list.
If code and docs disagree, either fix the code or add/update a Known Gap.
Keep implementation ledgers, roadmap detail, and historical MR notes in the per-area docs. This file is the filter, not the encyclopedia.

Hard Invariants

Respect the substrate. Lance owns columnar storage, per-dataset versioning, fragments, branches, compaction, cleanup, and index primitives. DataFusion should own relational execution where it fits. Do not add custom WALs, transaction managers, buffer pools, page formats, or local clones of substrate behavior. Read lance.md before guessing.
Graph visibility is manifest-atomic. Lance commits are per dataset. OmniGraph's graph-level atomicity comes from publishing one manifest update for the whole graph, guarded by expected table versions and sidecar recovery. No write path may make a subset of touched node/edge tables visible as a graph commit.
A query reads one snapshot. Query execution captures a manifest snapshot for its lifetime. Do not re-read branch head mid-query to discover newer table versions.
Mutations publish at one boundary. A mutate_as or load operation accumulates constructive writes, commits each touched table at the end, then publishes one manifest update. Do not commit per statement. Delete-only queries are the documented inline residual; the parse-time D2 rule prevents mixing deletes with insert/update until Lance exposes two-phase delete. Read writes.md and execution.md.
Recovery is part of the commit protocol. Writers that can advance Lance HEAD before manifest publish must write __recovery/{ulid}.json sidecars. Omnigraph::open in read-write mode runs the all-or-nothing sweep, and refresh runs roll-forward-only recovery for long-lived processes. Do not add a new writer kind without sidecar coverage or an explicit proof that no Lance HEAD can move before manifest publish.
Strong consistency is the default. Reads are snapshot-isolated, writes are durable before acknowledgement, and branch reads observe the current committed graph state. Any eventual-consistency mode must be explicit, read-only, auditable, and non-default.
Indexes are derived state. Reads must see the correct result for the branch they read even when index coverage is partial. Expensive index work should converge from manifest state instead of extending the critical write path. Scalar staged index builds and vector inline residuals are documented in writes.md and indexes.md.
Schema identity survives renames. Accepted schema identity must remain stable across type and property renames. Rename support belongs in migration planning, not in "drop and recreate" behavior. See the known gap below.
Schema/data integrity failures are loud. Type errors, required-field misses, invalid edge endpoints, cardinality violations, and unsupported mixed mutation modes fail before a graph commit is published. The system must not invent placeholder nodes or silently weaken integrity.
Query semantics are first-class IR concepts. Search modes, mutations, polymorphism, traversal, retrieval scores, imports, and policy predicates belong in typed AST/IR/planner structures. Do not smuggle semantics through strings, side tables, global state, or transport-specific flags.
Transport/auth stay at the boundary. Kernel crates should not depend on HTTP, OpenAPI, bearer-token parsing, or future transport protocols. The server resolves bearer tokens to actors; clients cannot set actor identity directly.
Bearer-token plaintext is not retained. Server startup hashes bearer tokens, authentication uses constant-time comparison, and request handling carries only the resolved actor identity and hash-derived match state.
Operational failures are bounded and observable. Timeout, memory, OOM, partial result, recovery, and conflict paths must fail loudly or degrade in a documented way. If a metric affects plan choice or operator behavior, it must be exposed through the relevant trait or observability surface.
Tests match the boundary being changed. Prefer extending the existing test that owns the area. Planner changes need planner-level coverage, storage changes need storage/recovery coverage, and end-to-end tests are not a substitute for missing lower-level assertions. Read testing.md before adding tests.

Current Truth Matrix

Area	Current state	Source
Multi-table commit	Manifest CAS plus recovery sidecars; not a single Lance primitive	writes.md, architecture.md
Constructive mutations	In-memory `MutationStaging`, one end-of-query table commit per touched table, then one manifest publish	writes.md, execution.md
Deletes	Inline-commit residual; delete-only queries allowed, mixed insert/update/delete rejected by D2	query-language.md, writes.md
Branch delete	Manifest is the single authority, flipped atomically first; per-table forks + commit-graph branch are derived state, reclaimed best-effort (`force_delete_branch`) with the `cleanup` reconciler as the guaranteed backstop. Reusing a name whose reclaim failed before `cleanup` surfaces an actionable error	branches-commits.md, maintenance.md
Schema validation	Type checks, required fields, defaults, edge endpoint checks, and edge cardinality are enforced on write paths	schema-language.md, execution.md
Unique constraints	Intra-batch and write-path checks exist; full cross-version uniqueness is still a gap	schema-language.md
Storage trait	`TableStorage` exists as the sealed staged-write surface; full call-site migration and capability/stat surfaces are incomplete	writes.md, architecture.md
Index lifecycle	`ensure_indices` is explicit today; reconciler-based convergence is roadmap	indexes.md, maintenance.md
Traversal IDs	Runtime still builds `TypeIndex`; Lance stable row-id based graph IDs are roadmap	architecture.md, query-language.md
Auth	Bearer token hashing and server-side actor resolution are implemented at the HTTP boundary	server.md, policy.md
Tests	Tempdir-backed Lance tests are the current substrate; there is no `MemStorage` test backend	testing.md

The branch-delete reconciler is authority-derived: it reclaims orphaned forks today and degrades to a no-op if Lance ships an atomic multi-dataset branch operation, so the design composes with that future rather than blocking it. This is the same shape as invariant 7 (indexes are derived state); prefer it over a recovery-sidecar-style approach for any new multi-dataset metadata operation, since the sidecar would be scaffolding to remove once the substrate closes the gap.

Known Gaps

Do not hide these behind invariant wording. Either move them forward or keep them explicit.

Rename-stable schema identity: the invariant is that accepted IDs survive renames. The current compiler still derives type IDs from kind:name; this must be fixed before relying on renamed IDs across accepted schemas.
Storage abstraction: TableStorage is present, sealed, and canonical for staged writes, but older inherent TableStore call sites and inline residuals remain. New write paths should use the staged shape unless a documented Lance blocker applies.
Deletes and vector indexes: delete_where and vector index creation still advance Lance HEAD inline because the required public Lance APIs are missing. Keep D2 and recovery coverage in place until those residuals are removed.
Planner capability/stat surfaces: cost-aware planning, complete capability advertisement, and explain-with-cost are roadmap. Do not describe them as implemented.
Traversal execution: current multi-hop execution still uses TypeIndex, ad-hoc ID filtering, and eager materialization in places. Stable row IDs, SIP, and factorization are target patterns, not current fact.
Retrieval ranks: hybrid search works, but rank/score are not yet carried everywhere as ordinary columns through the plan.
Policy pushdown and Source: Cedar enforcement is at the HTTP boundary today, and imports are still loader-shaped. Planner predicates and a unified Source operator are roadmap.
Resource bounds: some operations still lack enforced per-query memory or time budgets. New long-running work should add explicit bounds rather than widening the gap.

Deny-list

If a proposal fits one of these, the burden is on the proposer to prove why the case is exceptional.

Custom WAL, transaction manager, buffer pool, page format, or storage engine.
Per-table graph publishing outside the manifest publisher.
Re-reading current branch head during a query instead of using the captured snapshot.
New write paths that can advance Lance HEAD before manifest publish without a recovery sidecar.
Cross-query BEGIN/COMMIT transactions in the OSS engine. Use branches and merges for multi-query workflows.
Acknowledging writes before durable Lance and manifest persistence.
Silent fallback to eventual consistency, partial results, or dropped rows.
State that drifts from Lance or the manifest when it can be derived.
Job queues for manifest-derivable state where a reconciler is the right shape.
Synchronous inline vector/FTS index rebuilds on the query commit path, except for documented Lance API residuals.
Side-channels for query semantics: hidden globals, magic strings, transport flags, or out-of-band metadata.
Cost-blind plan choice when statistics are available or required.
Hidden statistics for behavior that affects planning or operator choice.
Hash-map iteration order in result ordering, plan choice, or migration output.
String-flattened SQL/filter generation when a structured pushdown API is available.
Eager multi-hop cross-product materialization when factorization fits.
Ad-hoc IN-list filtering where SIP or another structured selectivity path fits.
Discarding retrieval score/rank before fusion or projection decisions.
Auto-creating placeholder nodes for orphan edges.
Wire-protocol-specific code in compiler or engine crates.
Cloud-only correctness fixes or forks of the OSS engine for correctness.
Mutating immutable substrate state in place, including Lance fragments or index segments.
Shipping observable behavior as if it were not part of the contract. Output ordering, error text, timestamp precision, defaults, and latency profiles all become dependencies once exposed.

Review Checklist

Use this as yes/no/NA for any non-trivial design or PR:

Does it respect Lance/DataFusion instead of rebuilding them?
Does it preserve manifest-atomic graph visibility?
Does every query keep one snapshot for its lifetime?
Do mutations publish once at the commit boundary?
Can every Lance-HEAD-before-manifest gap recover all-or-nothing?
Are schema and edge integrity checks strict by default?
Are query semantics represented in AST/IR/planner structures?
Are transport, auth, and policy boundaries preserved?
Are failures bounded, typed, and observable?
Are result ordering and plan choices deterministic within a snapshot?
Are stats/capabilities exposed when behavior depends on them?
Are existing known gaps left no worse and documented if touched?
Does the test live at the same boundary as the change?
Does the change avoid every deny-list pattern, or justify the exception?

Maintenance Policy

Update this file when an invariant changes, a known gap opens or closes, or a new review anti-pattern deserves deny-list treatment. Prefer stable headings over numbered sections so other docs can link here without churn.

Removing or relaxing a hard invariant requires the same review process as code. Adding a known gap is acceptable when it makes reality explicit; leaving stale claims is not.

13 KiB Raw Blame History