607 commits

This branch

This branch

All branches

Author	SHA1	Message	Date
Ragnor Comerford	e7e057e26d	perf(engine): scope CSR topology index to traversed edges, reuse it cross-branch (#312) ... * perf(engine): scope the CSR topology index to traversed edges, reuse it cross-branch The in-memory CSR graph index was built over every edge type in the catalog and cache-keyed by the resolved snapshot id, so a single-edge join (`$x identifiesPerson $p`) full-scanned every edge table in the graph (the 40-60s / 428s-first-traversal hang), and a lazy-fork branch cold-rebuilt main's index. Two cuts close that: - Scope (A2): build only the edge types the query traverses (`referenced_edge_types` over Expand/AntiJoin, exhaustive match), not the whole catalog. Threaded through GraphIndexHandle -> RuntimeCache; cache-keyed on the scoped set. - Cross-branch reuse (A1): key RuntimeCache by each edge table's physical identity (table_key, version, table_branch, e_tag) instead of the snapshot id, so a lazy-fork branch whose edge tables physically are main's reuses main's built index. Local-FS (e_tag None) falls back to refresh-invalidation. Adds graph_build_count/graph_edges_built probes for the cost tests. * test(engine): cost tests for scoped + cross-branch-reused topology index fresh_branch_traversal_reuses_main_graph_index (A1: a lazy-fork branch reuses main's cached CSR index, 0 rebuilds) and single_edge_query_builds_only_referenced_edge (A2: a one-edge query builds only that edge, not the whole catalog), via the graph_build_count/graph_edges_built probes. Forced CSR mode, #[serial]. Updates the recreated-branch incarnation test comment for the physical-identity key. * docs(engine): topology-index scoping + physical-identity cache key Document the scoped CSR build and the physical-identity (e_tag) graph-index cache key with its local-FS refresh-invalidation fallback across invariants, testing, execution, and architecture docs. * fix(test): move CSR-forced topology cost tests to the all-serial binary The two topology-build cost tests force OMNIGRAPH_TRAVERSAL_MODE via process- global env mutation, which query.rs reads. In warm_read_cost.rs (a mixed serial/non-serial binary) a concurrent non-serial traversal test could race the env write (UB under Rust 2024's unsafe set_var contract) and be forced onto CSR. Move them to traversal_indexed.rs — the dedicated all-serial binary with no non-serial env reader (its documented-safe home) — and add a ModeGuard RAII helper so a panic mid-test cannot leak the override. Addresses a PR review (P2). * fix(engine): include edge endpoints in the graph-index cache key The A1 physical-identity key omitted the edge's (from_type, to_type). GraphIndex keys its TypeIndexes by those endpoint names and execute_expand_csr looks them up by the current catalog's names, so a schema repoint of an edge type that leaves the edge table's physical identity unchanged would reuse a stale index built with the old endpoint namespace and fail with "no type index for <new type>". The old snapshot_id (carrying the manifest version) masked this; dropping it exposed it. Adding the endpoints to the key rebuilds on a repoint while preserving lazy-fork cross-branch reuse (same endpoints -> same key). Addresses a PR review (P1). * test(engine): scoped with_traversal_mode seam + e_tag graph-index coverage Replace the process-global OMNIGRAPH_TRAVERSAL_MODE env-mutation test hack (which forced #[serial] + dedicated all-serial binaries and was triplicated as ModeGuard + set_mode/clear_mode) with one general abstraction: a task-local `with_traversal_mode` seam mirroring `with_query_io_probes`. It is scope-bound (leak-free even on panic) and process-safe (never touches shared state), so a forced-mode test cannot affect a concurrent test in the same binary. `traversal_indexed_override` consults the seam first, then the env var (which stays the documented ops escape hatch). - Migrate traversal_indexed.rs, proptest_equivalence.rs, and the two topology cost tests (moved back to warm_read_cost.rs) to the seam; drop all ModeGuard / set_mode / clear_mode / #[serial] / per-file column0 helpers. - Consolidate the duplicated first-column extractors into one shared `helpers::first_column_sorted`. - Add `s3_storage.rs::s3_fresh_branch_traversal_reuses_main_graph_index_with_etags`: the CSR cache-key cross-branch reuse path on a REAL per-table e_tag (None on local FS, so local tests can't reach it). Confirmed empirically that RustFS — the CI S3 backend — surfaces ETags into version_metadata.e_tag(). CI path filter now triggers the rustfs job on runtime_cache/graph_index changes.	2026-06-28 20:03:06 +02:00
Andrew Altshuler	20e5fada8a	docs: state cluster apply is storage-direct, not server-routed (#306) ... * docs: state cluster apply is storage-direct, not server-routed `cluster apply` reaches the object store directly — the `__cluster/` ledger and each graph's Lance datasets — never through a running omnigraph-server, so the host that runs it needs storage credentials. The rationale (declarative control plane, not a runtime mutation API) was documented in cluster-axioms.md §3/§4, and the out-of-band/direct-storage fact was stated for the maintenance verbs and init/load, but never spelled out for apply itself. - docs/user/clusters/index.md: add a day-2 note making apply's storage-direct execution and credential requirement explicit, linking the why to axioms 3/4. - skills/omnigraph/SKILL.md: extend the "init/load write storage directly (bypassing the server)" line to include cluster apply, with the same reasoning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: disambiguate the §5 cross-reference in cluster apply note The trailing (§5) sat right after the cluster-axioms.md §3/§4 citation, so a reader could read §5 as referring to cluster-axioms.md (whose §5 covers locked state) rather than this guide's §5. Make it an explicit same-page forward reference. Addresses Greptile P2 on #306. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: don't claim the server is read-only against storage The "server only reads from it" wording was wrong: the data plane serves HTTP writes (mutate/load/branch) that go through the server to the graph datasets, so omnigraph-server is not read-only against object storage. The hazard is an operator granting the server read-only S3 creds and breaking runtime writes. Scope the read-only claim to cluster (control-plane) state at boot, and state that data-plane writes still need read-write storage access. Addresses Greptile P-level finding on #306. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Ragnor Comerford <ragnor.comerford@gmail.com>	2026-06-28 17:14:58 +02:00
Ragnor Comerford	7779b72446	feat(engine): retire commit-graph tables (#311) ... * docs(dev): write-latency roadmap (validated cost model + layered fix) Records the validated 6-LIST warm-write cost model, the two root causes (un-GC'd _versions/; re-resolving latest by listing), and the layered fix (GC + capture-once reuse), plus how commit-graph-table retirement feeds in. Linked from docs/dev/index.md next to the RFC-013 docs. * feat(engine)!: strand storage versioning — one internal-schema version, no in-place migration Set MIN_SUPPORTED == CURRENT == 4: this binary reads exactly one `__manifest` internal-schema version and refuses any older graph on open with a rebuild-via-export/import message, instead of migrating it in place. Storage format changes become a deliberate cutover, not a permanently-carried in-place migration — the pre-release "complexity must be earned" contract. Delete the entire in-place migration apparatus and everything that existed only to support it: the `migrate_vN` arms + dispatcher + stamp-bump helpers + the schema-version-floor tripwire; `migrate_on_open` (both open modes now refuse); the legacy `_graph_commits.lance` readers + the v3 test fixtures + migration tests + `migration.v3_to_v4.*` failpoints + the two surface guards that pinned Lance variants only the migration matched on; and `state::merge_lineage_rows`. Keep `read_stamp` / `stamp_current_version` / `set_stamp` / `refuse_if_stamp_unsupported` — the seam a future one-shot converter plugs into. `load_commit_cache_for_branch` now reads the `__manifest` projection unconditionally (sub-v4 graphs are refused at open). Adds `sub_current_graph_is_refused_on_open_with_rebuild_hint`. The commit-graph TABLES are still created/used as branch-ref ledgers — their retirement (CommitGraph -> pure `__manifest` projection) is the next commit. BREAKING CHANGE: a graph created by omnigraph <= 0.7.2 (internal schema v3) is refused on open. Rebuild it: `omnigraph export` with the old release, then `omnigraph init` + `omnigraph load` with this one. Data, vectors, and blobs are preserved; commit history and branches are not. * feat(engine)!: retire `_graph_commits.lance` / `_graph_commit_actors.lance` — CommitGraph is a pure `__manifest` projection Since RFC-013 Phase 7, graph lineage lives in `__manifest` (`graph_commit` / `graph_head` rows) and branch authority is `__manifest` (branch create forks it first). The two commit-graph datasets were vestigial: `_graph_commit_actors.lance` was never written or read; `_graph_commits.lance` carried zero commit rows and only mirrored the manifest's branch refs (a deny-list "parallel copy"). Retire both. - `CommitGraph` collapses to a pure projection: drops its Lance dataset handles (`dataset`/`actor_dataset`) and all branch methods; `open`/`open_at_branch`/ `refresh`/`init` open NO dataset, building the cache from `ManifestCoordinator::read_graph_lineage_at`. Removes ~1.4s of cold-open dataset opens. - `graph_coordinator`: `commit_graph` is now non-`Option` (always a valid projection). `branch_create`/`branch_delete` go through `ManifestCoordinator` only — a single atomic op, replacing the two-step manifest-fork + commit-graph-fork + rollback. Deleted `create_commit_graph_branch`, `reclaim_commit_graph_branch`, `ensure_commit_graph_initialized`, and every `storage.exists(_graph_commits.lance)` gate. - `optimize`: dropped `reconcile_commit_graph_orphans` and the two tables from the internal-table compaction set (now `__manifest` only). - `instrumentation`: `INTERNAL_TABLE_DIRS` no longer lists the two tables. - Fresh graphs create neither table; `lineage_projection.rs` now asserts both `.lance` dirs are absent. Deleted the obsolete commit-graph-branch-race failpoint tests + their failpoint names, and updated the `maintenance` optimize tests (one internal table, not three). Review-pass fixes folded in: - Removed two stale `omnigraph.rs` in-source tests the prior run missed (a disk-full link failure masked them): one asserting `open` probes `_graph_commits.lance` (the exists-gate this commit removes) — it was masked earlier by a disk-full link failure. - Corrected src comments referencing deleted code (`migrate_v3_to_v4`, `append_commit`/`append_merge_commit`, the three-internal-table list, the `_graph_commits` reconcile owner) in publisher/recovery/optimize/recovery_audit. - Narrowed `set_stamp_for_test` to `cfg(test)` (its only caller is the refusal test) — removes a dead-code warning in the failpoints build. Branch create/delete atomicity improves (single atomic `__manifest` op). No behavior change for reads or branches. Follow-up (separate commit): the now-always-0 `IoCounts::commit_graph_reads` test counter + its `IOTracker`, threaded through ~11 cost-test files. * feat: surface the internal-schema (storage-format) version to operators After stranding storage versioning (a sub-v4 graph is refused on open), operators could only discover the storage-format version by hitting a refusal. Surface it: - `omnigraph version` prints an `internal-schema <N>` line (the binary's CURRENT storage-format version). - `omnigraph snapshot` includes `internal_schema_version` — the GRAPH's per-branch on-disk stamp, read via the new `Omnigraph::internal_schema_version_of`. - `GET /healthz` includes `internal_schema_version` (server-scoped: the binary's CURRENT, alongside `version`/`source_version`). Wire: re-expose `INTERNAL_MANIFEST_SCHEMA_VERSION` as `pub` on `db::manifest`; add `internal_schema_version: u32` to `SnapshotOutput` + `HealthOutput`; `snapshot_payload` takes the per-graph version (the `Snapshot` does not carry it), threaded through the embedded CLI + server snapshot callers. `openapi.json` regenerated (two added int32 properties). Extends the existing healthz / snapshot / version tests. * docs(engine): gate internal-schema version at the graph level; record the per-branch read gap PR reviewers flagged that the open path validates only main's internal-schema stamp, so a branch read could decode a branch stamped outside this binary's range. The stamp is a graph-wide storage-format property (the upgrade path is a whole-graph export/import), so with one binary version every branch is always CURRENT; divergence needs concurrent multi-version writers, an unsupported topology already in one-winner-CAS territory. Gating per-branch would add a second __manifest open per non-main branch read to defend a state we do not support, unearned complexity that regresses the warm-read budget. Keep the graph-level gate, document it at the code site (refuse_if_internal_schema_unsupported), and record the read-only residual hole as a known gap in invariants.md to close only when multi-version write topologies become supported. Also clarify the sub-floor rebuild message to say "export with the older omnigraph binary that created it." No behavior change: HEAD already gated at the graph level. * test(cost): remove the dead commit_graph_reads IO counter Phase B retired _graph_commits.lance / _graph_commit_actors.lance, so no commit-graph dataset is opened and the commit_graph IOTracker term is structurally always 0. Remove IoCounts::commit_graph_reads, its total_reads() term, the commit_graph IOTracker in OpProbes, and the now-dead commit_graph_wrapper field on QueryIoProbes (it had no accessor — nothing ever attached it). Drop the 7 trivially-true assert_eq!(commit_graph_reads, 0) checks in warm_read_cost.rs and the debug-print refs in write_cost{,_s3}.rs. Lineage and actor rows now live in __manifest (RFC-013 Phase 7), so the internal_table_scans_are_flat_in_history gate folds into the single manifest_reads flat-assertion — the manifest scan already covers them. Harness-only; no production runtime impact. * docs: align with the commit-graph retirement + strand storage versioning Update the always-loaded and user-facing docs to match the landed state: graph lineage lives in __manifest, the _graph_commits.lance / _graph_commit_actors.lance tables are retired, and storage is strict-single-version (no in-place migration — a sub-CURRENT graph is refused with an export/import rebuild). Fixed stale claims in invariants.md (the migration/atomicity known-gap entry, the Truth Matrix branch-delete row, the read-path/optimize internal-table scope), lance.md (the migrate_v1_to_v2 PK bullet now reflects init-time set; removed the two deleted v3->v4 migration surface guards), testing.md (dropped the deleted migration failpoint tests; manifest-only internal-table term), writes.md (rewrote the Migration-code section to the strand model), storage.md / maintenance.md / constants.md (retired tables out of the layout, internal-table compaction scope, and the constants cheat-sheet), and AGENTS.md. Marked the retirement DONE in the RFC-013 handoff/roadmap and banner-noted the historical RFC analysis. Added docs/user/operations/upgrade.md (the export/import rebuild recipe) and docs/dev/versioning.md (the four-axis compatibility policy: release lockstep / wire additive / storage strict-single-version / Lance pinned), cross-linked from the audience indexes and the AGENTS.md topic map, and rewrote the in-progress v0.8.0 release note for the strand model + version surfacing. check-agents-md.sh passes (65 links, 62 docs). * test(manifest): cover the v3-refusal→export/import rebuild cycle and branch stamp inheritance Two coverage additions from PR review (P1): (a) sub_current_graph_is_refused_then_rebuilt_via_export_import — the full operator narrative in one flow: load → export → a sub-CURRENT graph (stamp rewound below CURRENT) is refused with the export nudge → fresh init + load(export) → data present and the rebuilt graph opens. The refusal is stamp-only (read before any data), so a stamp-rewound graph is a faithful stand-in for a real older-release graph without a second binary; vector/blob fidelity stays covered by tests/export.rs. (b) branch_inherits_main_internal_schema_stamp — proves a branch cannot diverge from main's stamp under single-binary operation (create_branch forks main's __manifest, the publisher does not re-stamp), which is why the graph-level (main-only) stamp gate is sufficient for supported inputs. A divergent branch stamp needs concurrent multi-version writers, the unsupported topology recorded as a known gap.	2026-06-28 16:49:49 +02:00
Ragnor Comerford	0dcdcf5a9d	feat(engine): Stage the delete path; retire the inline-delete residual (#308) ... * test(engine): pin zero-row cascade delete must not drift an edge table (red) A delete <Node> cascades a delete_where into every incident edge type. The inline delete_where (Dataset::delete) advances Lance HEAD even when zero edges match, but the cascade records the new version only if deleted_rows > 0 — so a node with no incident edges leaves edge:Knows HEAD>manifest drift, which trips the next strict write's ExpectedVersionMismatch and repair refuses it. Red today: edge:Knows manifest=v5, Lance HEAD=v6. Goes green when delete moves to the staged two-phase path (iss-950, Lance 7.0 DeleteBuilder::execute_uncommitted), where a 0-row delete commits no Lance version and the deleted_rows>0 gate becomes correct by construction. * fix(engine): a zero-row delete must not advance Lance HEAD Lance's Dataset::delete commits a new version even when the predicate matches nothing (build_transaction always emits Operation::Delete), so a node delete that cascades a delete_where into an incident edge type with no matching edges advanced that edge table's Lance HEAD while the cascade skipped record_inline (gated on deleted_rows > 0) — leaving HEAD>manifest drift that wedged the next strict write and that repair refused as suspicious/unverifiable. Use Lance 7.0's two-phase DeleteBuilder::execute_uncommitted to read num_deleted_rows before committing: a no-match delete now advances nothing (no version, no drift) and the existing deleted_rows>0 gate is correct by construction. Non-zero deletes commit the staged transaction with skip_auto_cleanup + affected_rows (parity with the prior inline path). First step of the staged-delete migration (iss-950); turns the node_delete_with_no_incident_edges_leaves_no_edge_table_drift regression green. * feat(engine): stage_delete two-phase primitive (MR-A step 0) Add TableStore::stage_delete (Lance 7.0 DeleteBuilder::execute_uncommitted), the two-phase analogue of stage_merge_insert: writes deletion files without advancing Lance HEAD, returns Option<StagedWrite> (None on 0 rows = true no-op), carrying the deletion-vector updated_fragments as new_fragments and the superseded originals as removed_fragment_ids so combine_committed_with_staged makes the deletion visible to in-query reads. No affected_rows is threaded: like stage_merge_insert's Operation::Update commit, the staged delete relies on OmniGraph's per-table write queue + manifest CAS, not Lance's per-dataset conflict resolver (commit_staged is a single attempt). Flip the two residual guards to the staged path: staged_writes.rs now asserts stage_delete does NOT advance HEAD and that a staged delete is read-your-writes visible (the deletion-vector RYW proof D2 retirement depends on); the lance_surface_guards delete guard pins execute_uncommitted's UncommittedDelete. No behavior change yet (callers still use delete_where); Step 1 wires them. * feat(engine): TableStorage::stage_delete + migrate merge delete path (MR-A step 1a) Add stage_delete/Option<StagedHandle> to the TableStorage trait (delegates to TableStore::stage_delete). Migrate the two branch_merge delete sites (three-way RewriteMerged + adopt delta) from the inline delete_where residual to stage_delete + commit_staged — identical in shape to the stage_merge_insert + commit_staged pair above each. HEAD still advances within the merge sequence (via commit_staged), under the unchanged SidecarKind::BranchMerge Phase-B confirmation; the _pre_delete/_pre_index failpoints fire by position, unchanged. merge_truth_table, branching, composite_flow green. * feat(engine): migrate all delete sites to staged path, retire inline delete (MR-A step 1b/1c) Routes every delete through the staged write path so delete never advances Lance HEAD inline — the last inline-commit residual on the mutation path is gone. `MutationStaging` now accumulates delete predicates (`record_delete`) alongside pending write batches; at end-of-query `stage_all` combines a table's predicates into one `(p1) OR (p2) …` `stage_delete` (a deletion-vector transaction, no HEAD advance) and `commit_all` commits it through the same `commit_staged` path as inserts/updates. Deletes are now ordinary staged entries: one sidecar pin at `expected + 1`, no inline special-casing. Migrated callers (all 5): the 3 mutation.rs sites (delete-node, cascade, delete-edge) and the 2 merge.rs sites (already on stage_delete in step 1a). `affected_edges`/`affected` move from post-inline-commit `deleted_rows` to a committed `count_rows` at record time — exact under D₂, bounded by the cascade working set. A predicate matching zero rows stages nothing (the staged equivalent of the old "skip record_inline on 0 deleted rows"), so the zero-row edge-table drift class stays closed by construction. Retired scaffolding now that no caller remains: - `MutationStaging.inline_committed` + `record_inline` → `delete_predicates` + `record_delete`; `StagedMutation.inline_committed`/`paths` fields and all the `commit_all` inline handling (queue keys, sidecar pins with the `record_inline` table_version special-case, the inline recheck loop). - `open_table_for_mutation`'s post-inline-commit reopen branch (deletes no longer advance HEAD mid-query, so a second touch reopens at the pinned version like any write). - `InlineCommitResidual::delete_where` + its `TableStore` impl, the orphaned `TableStore::delete_where`, and `DeleteState`. `InlineCommitResidual` now carries only `create_vector_index` (Lance #6666 still open). D₂ stays for now: staged-delete read-your-writes doesn't yet compose into the pending accumulator (insert-then-delete on one table), so mixed insert/update/delete in one query is still rejected at parse time. Retiring D₂ is step 2. Doc comments updated to match across exec/, storage_layer, db/. Tests (all green): writes, consistency, validators, end_to_end, composite_flow, merge_truth_table, maintenance, recovery, staged_writes, forbidden_apis, lance_surface_guards, changes, point_in_time (286), plus failpoints (63). * docs: delete is a staged write, not an inline-commit residual (MR-A step 1) Update the docs that described `delete` as the inline-commit residual now that MR-A routes it through `stage_delete`. Always-loaded surfaces (AGENTS.md rule 4 / capability matrix, invariants.md Invariant 4 / truth matrix / known gaps) plus the dev write-path docs (writes.md, execution.md incl. its mutation sequence diagram, architecture.md) now state: deletes accumulate as predicates and stage like inserts/updates, no inline HEAD advance; `InlineCommitResidual` carries only `create_vector_index` (Lance #6666). The parse-time D₂ rule is documented as retained — not because delete inline-commits, but because staged-delete read-your-writes is not yet wired into the pending accumulator (MR-A step 2). lance.md's 7.0 audit note marked MR-A as landed. * docs: D₂ is a deliberate boundary, not temporary scaffolding (MR-A close-out) After MR-A staged the delete path, D₂ (a mutation query is insert/update-only OR delete-only) was left framed as temporary — "until Lance ships two-phase delete" / "retire in step 2". Lance shipped that and we used it for the inline-commit fix; D₂'s original justification is gone. It now stands for a different, permanent reason: keeping a query to one kind keeps its read-your-writes unambiguous and each table to one version per query. Retiring it would buy single-commit mixed atomicity (cheap workaround: split, or a branch) at the cost of an in-query delete view, pending pruning, edge id-resolution, and two-commit-per-table ordering in the hot mutation path — complexity not worth earning. Decision: keep D₂ as a deliberate boundary. Reframes the now-stale wording everywhere, no logic change: - The D₂ parse-time error message no longer promises "this restriction lifts when Lance exposes a two-phase delete API"; it states the boundary and points to a branch+merge for one atomic commit. - `enforce_no_mixed_destructive_constructive` doc, AGENTS.md, invariants.md (Invariant 4 / truth matrix / removed from the known-gaps), writes.md, architecture.md, lance.md, and the user mutations doc (which wrongly said deletes "commit through a different path" — both stage now). - Swept remaining stale `delete_where` mentions left from the Step-1 migration: the merge.rs "swap when upstream ships" comments (already swapped), the forbidden_apis / table_ops residual notes, the staged_writes vector-index guard doc (was "same as stage_delete's absence" — stage_delete now exists), and test comments/assert messages in recovery/maintenance/writes/failpoints. Genuinely-historical records (dated Lance audit, rfc-013, bug-case-fix) left. Verified: engine builds warning-free; check-agents-md OK; writes/maintenance/ recovery/staged_writes/forbidden_apis all green. Closes MR-A. * test(engine): overlapping delete predicates must not double-count affected_* (red) Reproduces a reporting regression from the staged-delete migration flagged in PR #308 review. Because deletes now stage (instead of inline-committing), two delete statements in one query both scan the same unchanged committed snapshot; counting each predicate independently over-reports `affected_*` when they overlap. The old inline path committed each delete before the next ran, so it counted distinct. `delete Person where name = "Alice"` then `delete Person where age > 29` over the standard fixture (Alice 30, Charlie 35) removes 2 distinct nodes and 3 distinct edges, but the buggy per-statement counting returns 3 nodes / 6 edges. RED at this commit (asserts left=3, right=2). * fix(engine): dedup overlapping delete predicates when counting affected_* Count each delete statement against the committed snapshot MINUS the predicates a prior delete statement on the same table already recorded: `(pred) AND NOT ((prior1) OR (prior2) …)`. Summed over statements this is inclusion-exclusion — `Σ |pₙ \ (p₁ ∪ …)| = |p₁ ∪ p₂ ∪ …|` — exactly the distinct count the combined `(p1) OR (p2)` staged delete removes. Works for nodes and edges alike with no edge identity needed; the node ID scan uses the same exclusion so a later statement also doesn't re-cascade already-deleted nodes. The ORIGINAL predicate is still what gets recorded (the staged delete removes the union); only the count uses the exclusion. The common single-delete path is unchanged (`prior` empty → filter is just the base predicate). New helper `dedup_delete_filter` + `MutationStaging::recorded_delete_predicates`. Turns the red regression test green (2 nodes / 3 edges); writes (33), end_to_end, validators, maintenance, recovery, composite_flow, merge_truth_table, consistency, changes, and failpoints (63) all stay green. * test(engine): delete dedup must not drop NULL-column rows (red) Follow-up to the overlapping-delete fix flagged in PR #308 review (Greptile P1): the `(base) AND NOT (prior)` exclusion breaks under SQL three-valued logic. If a prior delete predicate references a NULLable column, a later statement's matching row whose column is NULL makes `prior` evaluate to UNKNOWN, `NOT UNKNOWN` is UNKNOWN, and the row is filtered out of the scan — even though the prior delete never matched it. That drops it from `deleted_ids`, skipping its cascade (orphaned edges) or, if it is the only match, leaving the node undeleted. A data bug, not just a miscount. Data: Charlie(age 35), Zoe(age NULL); Knows Zoe→Charlie. `delete Person where age > 30` then `delete Person where name = "Zoe"`. Under the buggy `NOT`, Zoe's scan `(name='Zoe') AND NOT (age>30)` is UNKNOWN → Zoe survives. RED at this commit (Person count left=1, right=0). * fix(engine): NULL-safe delete dedup — exclude only definitely-matched prior rows Change `dedup_delete_filter` from `(base) AND NOT (prior)` to `(base) AND ((prior) IS NOT TRUE)`. `IS NOT TRUE` keeps both FALSE and UNKNOWN rows, so a prior predicate that evaluates to SQL UNKNOWN (a NULL in a referenced column) no longer drops a row this statement legitimately matches — only rows a prior predicate matched as definitely TRUE are excluded from the count/scan. The distinct-count semantics are unchanged for non-NULL data. Turns the red NULL-dedup test green (Zoe deleted, her edge cascaded), and the overlapping-dedup + writes/end_to_end/validators/maintenance/recovery/ composite_flow/consistency suites stay green. * docs(engine): note dedup_delete_filter's load-bearing dependency on D₂ Self-review follow-up: the overlapping-delete dedup assumes the committed snapshot is invariant across a query's statements, which holds only because D₂ forbids mixing writes with deletes (so a delete-touched table has no pending writes). Make that dependency explicit at the function so a future D₂ relaxation is forced to revisit the dedup. Comment-only. * Preserve staged write commit metadata	2026-06-27 16:48:41 +02:00
Ragnor Comerford	a7d4cba53d	perf(engine): halve per-write __manifest scans (#307) ... * test(write_cost): served-regime __manifest scan tripwire Adds `internal_table_scans_grow_without_compaction`, the served-regime twin of `internal_table_scans_are_flat_in_history`. The flat gate `optimize()`s before every measured write, so it only proves the *compacted* invariant and stays green even when a served graph's per-write `__manifest` scan amplifies without bound. This tripwire measures the uncompacted regime and asserts the scan grows — green today, and it flips RED once the amplification is bounded (write-path warm-reuse + version-GC), at which point it inverts to a permanent `assert_flat` gate. RFC-013. * perf(engine): halve per-write __manifest scans (RFC-013 PR2) Cuts a same-branch write from ~4 to ~2 `__manifest` scans (measured 50->25 at depth 10, 410->205 at depth 100) with the OCC contract and snapshot isolation preserved: - #1a probe-gate the OCC re-capture in `commit_all` via `occ_snapshot_for_branch` (mirrors the read path's `resolve_target_inner`): reuse the warm coordinator when a cheap incarnation probe proves it current, fall through to a cold read on mismatch. - #1b fold the post-publish `known_state` in-memory from `existing_versions` plus the committed rows instead of an O(fragments) re-scan; extracted the shared `assemble_manifest_state` reduction so the fold is byte-identical to a scan, proven by the new `post_publish_fold_matches_fresh_reopen` test. - #1c project `read_manifest_scan` to the columns it reads (drop `base_objects` always, `object_id` on the table-state path). The two remaining publish scans (`load_publish_state` and the `use_index(false)` merge-insert join) stay O(fragments), bounded by compaction/version-GC (RFC-013 PR1, not in this change). * test(manifest): reproduce owner-branch handoff fold desync The PR #307 post-publish fold appends pending table_version rows after existing_versions, and assemble_manifest_state keeps the first equal-version entry. A same-version owner-branch handoff updates a table_version row in place at the same Lance version with a new table_branch (merge-insert UpdateAll on the deterministic version_object_id), so the warm coordinator keeps the stale fork while a fresh re-scan reflects the handoff. This test commits a handoff through the coordinator commit path (exercising the fold) and asserts the warm snapshot equals a fresh reopen. It is red against the current fold; the following commit turns it green. Flagged by Cursor Bugbot (High) and ChatGPT Codex (P2) on PR #307. * fix(engine): fold table_version rows by (table_key, version) identity fold_inputs now keys version entries by (table_key, table_version), the manifest row identity carried by the deterministic version_object_id that the merge-insert CAS uses. A pending row at the same identity replaces the pre-publish entry, mirroring merge-insert UpdateAll on disk. Previously the fold appended pending rows after existing_versions, so an owner-branch handoff left two equal-version entries and assemble_manifest_state retained the stale one. The fold input now carries the same one-row-per-(table_key, version) uniqueness a fresh scan produces, so both feed assemble_manifest_state equivalent inputs and the warm known_state stays byte-identical to read_manifest_state. This corrects the derivation's identity model structurally and applies to any same-version in-place update. Closes the PR #307 review finding. * test(cost): enable lance-io test-util for IO request diagnostics Gives IoStats.requests + assert_io_eq!, used by the cost harness to record the __manifest read log (method + path) for failure diagnostics. Dev-dependency only, so production builds (which exclude dev-deps) never compile it. * test(cost): rebuild IO harness on GraphIoMeter + incremental_stats Consolidate the per-op ProbeHandles into OpProbes plus a persistent GraphIoMeter, and read per-op deltas via lance's incremental_stats() (get-and-reset) instead of cumulative stats() -- the upstream per-request idiom (rust/lance/src/dataset/tests/dataset_io.rs). Add cost_harness(body): it installs one __manifest tracker for a whole test body, so the graph opens under it and every coordinator handle (init plus each post-publish reassignment) carries the same tracker. measure reuses that ambient tracker when present, making manifest_reads ground truth (warm probe plus cold scans, handle-age-irrelevant); outside cost_harness it falls back to a fresh per-op tracker (today's behavior). The body future is boxed so wrapping a whole test body does not overflow the test thread's stack. Also stash each op's __manifest read log on the meter for assert_io_eq!-style failure diagnostics (last_manifest_reads). Behavior-preserving: no test wraps its body in cost_harness yet, so measure takes the fallback path and every cost number is unchanged. write_cost and warm_read_cost stay green. * test(write_cost): ground-truth __manifest counting via cost_harness Wrap the three __manifest-asserting tests (flat, grow, ceiling) in cost_harness so manifest_reads is ground truth -- the warm-coordinator freshness probe rides a long-lived handle a per-op tracker installed at measure time cannot see. The flat/grow gates are depth-difference assertions, so the constant per-write probe offset cancels and they pass unchanged; the absolute ceiling is retightened from 34 to 24 (~18 measured = ~15 publish-path scans + ~3 probe RPCs) with the read log dumped on a breach. Add manifest_reads_capture_warm_probe: it measures the same warm write fresh-only and under cost_harness and asserts ground truth strictly exceeds fresh-only by the probe's RPCs (11 vs 14). Reverting the ground-truth wiring makes the two equal, so this guards that a write's warm-handle probe (3 object-store RPCs that were counted as a single version_probe) cannot silently escape manifest_reads again. * test(warm_read_cost): ground-truth __manifest counting via cost_harness Wrap the warm (== 0) manifest gates in cost_harness so manifest_reads is ground truth. A read's freshness probe is served from Lance's cached manifest at 0 object-store reads (unlike a write's probe, which re-reads after its commit), so the == 0 assertions hold with no re-baseline -- and now also catch any future warm-handle scan a per-op tracker would miss. The stale (> 0) tests are unaffected either way and stay on the fresh fallback. * docs(testing): document ground-truth cost harness (GraphIoMeter) The cost harness now reads incremental_stats() deltas and, under cost_harness, installs one __manifest tracker before the graph opens so manifest_reads is ground truth (handle-age-irrelevant). Note that version_probes is the probe call count and that ground truth reveals a write's probe does ~3 object-store RPCs. * docs(rfc-013): bring write-path handoff current (Thread B + Phase 7 landed) Prepend a current-state section (§A) for the __manifest scan-amplification / version-chain thread: the problem, what landed on main (step 2a, Phase 7 #299), what is in flight on this branch / PR #307 (PR2 scan-halving, the owner-branch handoff fold fix, the PR2.1 ground-truth cost harness), the accurate measurement (per-write __manifest ops ~50->410 pre-PR2 vs 28->208 ground truth; the hidden 3-RPC freshness probe), the remaining roadmap (PR1a manual cleanup, PR3-scoping, deferred PR1b/PR4), critical files, and gotchas. Staleness fixes: Phase 7 was listed as a future "step 4" but landed as #299, so mark it LANDED in the TL;DR landed list and in the remaining-steps section. * docs(rfc-013): refresh PR307 handoff state	2026-06-27 13:18:04 +02:00
Ragnor Comerford	1c5cb8741e	feat(engine): graph lineage in __manifest — single-source fold, v3→v4 migration, schema-version floor (#299) ... * docs(rfc-013): bank the #295 spec-review comments as step-5 constraints (§5.1) 3b shipped a minimal WriteTxn{branch,base} and deferred the full §4.1 opener unification (pinned-base opener, shared Session, write-local handle cache, strict-op conflict-timing move) to step 5. The greptile comments on the #295 spec were moot for #298 (none of those constructs were built) but are load-bearing for step 5: (1) the handle cache must be Send+Sync (Mutex, not RefCell); (2) the strict-op timing move needs an explicit retry contract — txn discarded after any commit, retry re-opens a fresh base — which is the SAME contract as the stale-view false-fail (§1d.2); (3) the opener-equivalence test must advance HEAD externally then assert pinned-base, not the trivial HEAD==base. * feat(engine): fold graph lineage into the __manifest publish CAS (RFC-013 Phase 7) Graph lineage no longer lives in a second write to _graph_commits.lance. Each commit's graph_commit + graph_head:<branch> rows now ride the SAME __manifest merge-insert as the table-version rows (one atomic version), and CommitGraph reads its cache from the manifest projection (read_graph_lineage). _graph_commits.lance is no longer written commit rows (it remains only as a Lance branch-ref carrier). Mechanism: a LineageIntent { graph_commit_id (ULID, minted once), branch, actor, merged_parent, created_at } threads through ManifestBatchPublisher::publish. Inside the publisher retry loop the parent is resolved per attempt from the just-loaded branch-scoped manifest (the should_replace_head winner over the visible graph_commit rows — branch-correct by Lance branch isolation; the graph_head row is written for forward-compat + the §7.1 contention point but is not the parent source, so a freshly-forked branch resolves the right fork-point parent). A CAS-conflict retry re-reads the advanced head → correct new parent; the commit_id is stable across retries. Closes two known gaps BY CONSTRUCTION (one write, no second step to fail/ race): - manifest→commit-graph atomicity (no crash window between manifest + lineage), - commit-graph parent under concurrency (no refresh→append TOCTOU; the per-write commit_graph.refresh() is gone). Recovery, branch-merge, and genesis route their lineage through the same CAS (merge: one commit_merge_with_actor; recovery: publish_recovery_commit folds the recovery commit, actor=omnigraph:recovery; genesis rides the init __manifest write). The dead _graph_commits write helpers (append_commit/_merge/_actor) are #[allow(dead_code)] (the actor sidecar table is still enumerated by optimize). Verified (sequential): build clean; the new lineage_projection gate (manifest-only — _graph_commits/_actors have 0 rows; full lineage reconstructs via the projection); branching/merge_truth_table (exhaustive, branch-aware)/composite_flow/point_in_time/ changes/consistency/recovery; failpoints (59, incl. recovery lifecycle + the now-closed atomicity gap); full --workspace. Cost tests REVERT to their pre-fold values (writes +1, write_cost ceiling 80) — the proof of true single-CAS (no extra write). invariants.md marks both gaps CLOSED. PENDING (next stages, this PR): the §7.1 concurrent graph_head one-winner gate (stage 5 — two concurrent same-branch commits, exactly one wins); the stamp bump v4 + migrate_v3_to_v4 backfill + read-only refuse for EXISTING graphs (stage 4); full doc-sync of storage.md/architecture.md/writes.md. * feat(engine): migrate existing v3 graphs to manifest lineage (RFC-013 Phase 7 stage 4) The Phase-7 fold made CommitGraph read lineage from the __manifest projection, so a pre-Phase-7 (internal-schema v3) graph — lineage in _graph_commits.lance, none in __manifest — would read an empty commit DAG. Stage 4 makes existing graphs upgrade seamlessly and not break reads. - Stamp 3 -> 4 + migrate_v3_to_v4: bumps INTERNAL_MANIFEST_SCHEMA_VERSION and adds the 3 => migrate_v3_to_v4 arm. The migration reads this branch's _graph_commits/_actors, emits one graph_commit row per commit + exactly one graph_head:<branch> for the head (should_replace_head winner, deterministic id-sort — no hash-map-order in migration output), merge-inserts into __manifest, then set_stamp(4) LAST. Idempotency guard first (read_graph_lineage non-empty -> just stamp); crash before set_stamp re-enters at v3 and the guard completes it. Does NOT touch the unenforced-PK metadata. Runs per branch: migrate_on_open backfills main; load_publish_state backfills each branch on its first write (root_uri/branch threaded through migrate_internal_schema). - v3-read fallback: CommitGraph version-gates the lineage source — stamp < 4 reads the (re-activated) _graph_commits.lance; >= 4 uses the manifest projection. So a READ-ONLY open of an un-migrated graph reads correct history with no write. Correctness catch: the legacy _graph_commit_actors.lance was never branched, so the fallback reads it FLAT (no branch checkout) while checking out the branch only on the commits dataset. - Read-only stamp-refuse: a ReadOnly open of a FUTURE-stamped graph now refuses with the same upgrade error (future-proofing the next format bump; the write path already refused via migrate_internal_schema). - Docs: storage/architecture/writes/invariants/constants updated to manifest-stored lineage; release note docs/releases/v0.8.0.md (format v4, old writers clean-break, data preserved, upgrade writers first). 6 new tests (v3 backfill, idempotent, v3 read-only fallback, future-stamp refuse in both modes, crash-before-stamp completes, legacy branch+flat-actor read). Full engine suite + failpoints (59) + cargo test --workspace --locked green; check-agents-md passes. * test(engine): graph_head concurrency gate — disjoint same-branch writers form a linear commit DAG (RFC-013 Phase 7) Two (or N) writers committing disjoint tables on one branch still share the mutable `graph_head:<branch>` manifest row, so the only row-level CAS contention is that row. The contract — exactly one writer wins each CAS round; the loser retries inside the publisher, re-resolves its parent off the freshly-advanced head, and re-commits, so every writer lands and the graph_commit DAG stays a single LINEAR chain (no fork) — had no acceptance test. This adds it. - concurrent_disjoint_writes_share_head_and_form_linear_chain: two disjoint writers + distinct LineageIntent, tokio::join!; both commit; the on-disk DAG is genesis -> c -> c' (asserted linear: exactly one genesis, no two commits share a parent, the head is the unique non-parent). - n_concurrent_disjoint_writers_converge_to_one_linear_chain: N=8 disjoint writers each with an app-level retry loop (the publisher's internal budget can be exhausted under contention); all converge to one linear chain of 8. - concurrent_disjoint_writes_form_linear_chain_on_s3: the same race on a real object store (true conditional-put CAS), bucket-gated. Cites both tests from the §7.1 contention note in invariants.md. Test-only; no production change. * perf(engine): fold the lineage parent scan into the publish path's single __manifest scan (RFC-013 P2) Each lineage publish scanned `__manifest` twice: `load_publish_state` read table state via one scan, then `resolve_lineage_rows` did a second full `read_graph_lineage` scan only to find the parent commit. Fold the `graph_commit` extraction into the existing scan. - `read_manifest_scan` gains a `collect_lineage` flag. The publish path (`read_publish_scan`) collects the `graph_commit` rows in the same pass; the table-state hot path leaves them in the forward-compat skip arm, so it never pays the O(commits) lineage JSON decode (it also skips reading the `object_id` column entirely). One shared `decode_graph_commit_row` serves both the folded path and the standalone `read_graph_lineage`, so the two cannot drift. - `resolve_lineage_rows` is now sync and takes the already-parsed rows; the per-attempt re-read is preserved because `load_publish_state` runs once per CAS attempt, so a retry still re-parents off the advanced head. - `load_publish_state` returns a named `LoadedPublishState` instead of a four-tuple; the thin `read_registered_table_locations` / `read_tombstone_versions` accessors fold away. `read_manifest_entries` becomes `#[cfg(test)]`: the fold removes its last production caller, leaving only the test-only namespace module (`db/manifest.rs`: `#[cfg(test)] mod namespace`), so gating it keeps it from becoming dead code in non-test builds. Measured at depth ~5: per-write `__manifest` reads drop 44 -> 26 (total reads 54 -> 36). write_cost.rs gains a `manifest_reads <= 34` sub-ceiling that trips if a publish-path scan is re-added, and its calibration comment is corrected. * test(engine): red — transient legacy-open failure silently completes the v3→v4 migration A pre-Phase-7 (internal schema v3) graph keeps its graph lineage in `_graph_commits.lance`; the v3→v4 internal-schema migration backfills it into `__manifest` and stamps v4. `read_legacy_commit_cache` currently maps EVERY `Dataset::open` error to "no legacy data" (`Err(_) => empty`), so a transient or corrupt open during the one-time migration backfills nothing and still stamps v4 — orphaning the real lineage permanently (the migration runs once; the v3 fallback is then disabled). Add a `migration.v3_to_v4.legacy_open` failpoint that injects a non-not-found Lance error at the legacy open, and a fault-injection regression test in the `failpoints` binary. Against the current swallow the migration completes anyway, so the test fails on its "migration must abort" assertion — the predicted symptom. The fix follows in the next commit. Test support reachable from the `failpoints` integration binary (it compiles the crate without `cfg(test)`): the v3-fixture helpers and a stamp/row-count reader are gated `cfg(any(test, feature = "failpoints"))`, still excluded from release builds. Failpoint tests stay in the integration binary because the fail registry is process-global. * fix(engine): propagate non-not-found legacy-open errors in the v3→v4 migration `read_legacy_commit_cache` mapped EVERY `Dataset::open` error to an empty cache (`Err(_) => empty`) on both the legacy commits dataset and its actor sidecar. The v3→v4 internal-schema migration reads this once before stamping internal-schema v4; a transient or corrupt open therefore backfilled nothing and stamped v4 anyway, orphaning the graph's real lineage permanently (the migration runs once, and the stamp-gated v3 fallback is disabled at v4). This is the "no silent failures" deny-list violation, and realistic on object storage. Both opens now match the not-found variants — Lance maps an object-store NotFound to `DatasetNotFound` — as the benign "no legacy data" / "no authors" signal, and propagate anything else as a loud error. The two arms share the variant contract but carry different rationale (commits-absent is the legitimate empty signal; actor-sidecar-absent is benign, but a corrupt actor open silently wiping authorship before stamping v4 is the same loss hole), commented at each site. Pinned by the `lance_surface_guards.rs::dataset_open_missing_returns_not_found_variant` guard (turns red if a Lance bump changes the absence variant) and greens the fault-injection regression test from the previous commit. * test(engine): cover the per-branch v3→v4 migration against a real Lance branch `seed_legacy_v3_lineage` writes every commit (including the "feature"-tagged one) to MAIN's `_graph_commits.lance` with `manifest_branch` as a mere field, so the production per-branch migration path — `read_legacy_commit_cache` checking out a real Lance branch, and a branch-scoped `__manifest` — was never exercised. Add `seed_legacy_v3_lineage_with_branch`, which forks a real `feature` Lance branch on BOTH `_graph_commits.lance` and `__manifest` (the branch inherits main's stripped v3 state), and a test that migrates the BRANCH and asserts the branch's lineage lands in the BRANCH's `__manifest` (genesis + A + branch commit, `graph_head:feature` → branch commit, parents + actors intact) with main's `__manifest` untouched. This empirically resolves the open question behind the merge robustness work: the fast-path `read_graph_lineage(dataset)` has no `manifest_branch` filter, but `__manifest` is Lance-branched per graph-branch, so a branch reads only its own lineage — the test confirms migrating one branch does not leak into another. No branch filter is needed. * refactor(engine): type the lineage-backfill merge conflict via the publisher classifier `state::merge_lineage_rows` (the v3→v4 lineage backfill's standalone `__manifest` merge-insert) stringified its `execute_reader` error, discarding the Lance variant. Route it through the publisher's `map_lance_publish_error` (now `pub(crate)`) so a concurrent first-open's row-level CAS loss surfaces as the SAME typed `OmniError::Manifest{ details: RowLevelCasContention }` the publisher's own retry consumes — one vocabulary, no raw-Lance matching in the migration. Deliberately NOT unified with `optimize::is_retryable_lance_conflict`: that classifier also matches `CommitConflict`/`RetryableCommitConflict` from the compaction commit path, which a row-level merge-insert never emits. Cross-linked with a comment at both sites. Behavior-preserving: the only path that changes is the error TYPE on a CAS loss (previously an opaque `Lance` string, now a typed conflict); no success/failure outcome changes. The bounded re-open retry that consumes the new type lands next. * test(engine): red — concurrent v3→v4 migrations error instead of converging `migrate_v2_to_v3` is concurrent-runner idempotent by design; v3→v4 regressed it. `merge_lineage_rows` uses `conflict_retries(0)` and `migrate_v3_to_v4` has no app-level retry, so when two processes open the same legacy graph at once the backfill's row-level CAS loser errors the whole open instead of converging. The test opens two `__manifest` handles at the same pre-migration (v3, empty-lineage) HEAD and runs both `migrate_internal_schema` calls under `tokio::join!`, forcing the `graph_head:main` CAS to fire every run. Against the current code the loser fails with `RowLevelCasContention` ("Attempted 0 retries.") — the predicted symptom — so the "both must converge" assertion panics. The bounded re-open retry that makes both converge lands next. * fix(engine): make the v3→v4 lineage backfill converge under concurrent runners `migrate_v2_to_v3` is concurrent-runner idempotent; v3→v4 was not. Two processes (or open-for-write handles) opening the same legacy graph at once both reach the backfill merge, and `merge_lineage_rows`'s `conflict_retries(0)` made the row-level CAS loser error the whole open instead of converging. Two contention points, both now handled all-or-nothing: 1. The backfill merge on `graph_head:<branch>`. Wrap (fast-path re-read → read legacy → merge) in a bounded re-open retry loop: a `RowLevelCasContention` loss re-opens the manifest past the winner's (atomic) commit and re-loops; the fast-path re-read then sees the winner's lineage and stamps. On budget exhaustion it returns a `RowLevelCasContention`-typed error so the publisher's OUTER retry loop completes it. The retry decision reuses the publisher's `is_retryable_publish_conflict` so the two stay in lockstep. 2. The terminal stamp bump. Making the merge loser converge newly lets BOTH runners reach `set_stamp(4)` — an `UpdateConfig` commit on the same key — so the loser gets `lance::Error::IncompatibleTransaction` (NOT a row-level CAS, so the merge loop doesn't catch it). This surfaced only under the concurrent full-suite run, not the isolated test. Both write the SAME value, so the conflict is benign: `commit_v4_stamp_idempotently` re-opens and, if the stamp already reached the target, succeeds; else re-applies (bounded). Greens the race test from the previous commit (3x isolated, 5x full-suite, no flake). The new `IncompatibleTransaction` match is pinned by `lance_surface_guards.rs::lance_error_incompatible_transaction_variant_exists`. * fix(engine): refuse a future internal-schema stamp on the branch read path `load_commit_cache_for_branch` dispatched on the branch's internal-schema stamp — `< CURRENT` to the v3 legacy fallback, `>= CURRENT` to the manifest projection — but never refused a `> CURRENT` branch stamp, so a newer-binary shape would be misread by the projection rather than rejected. Add `refuse_if_stamp_too_new(stamp)` (re-exported `pub(crate)` from `migrations`) right after the branch stamp is read, mirroring the main read path's `refuse_if_internal_schema_too_new`. This is defense-in-depth, not a live hole: migrations run main-first (main migrates on open; each branch on its first write), so main's stamp is always >= every branch's and the main path refuses first. The guard closes the gap if that ordering invariant is ever weakened. Tested by force-stamping a real branch past CURRENT and asserting the branch read refuses with the upgrade error (the test misreads via the projection — returns Ok — without the guard, confirmed by removing it). * docs(rfc-013): record the v3→v4 migration robustness fixes invariants.md Known Gaps: the `migrate_v3_to_v4` entry now states the migration is loud on non-not-found legacy-open errors and concurrent-runner idempotent (bounded re-open retry on the merge CAS + idempotent stamp bump), and that the branch read path refuses a `> CURRENT` stamp. lance.md: note the two new surface guards the migration depends on (`dataset_open_missing_returns_not_found_variant`, `lance_error_incompatible_transaction_variant_exists`). testing.md: note the migration fault-injection test in the failpoints row. * refactor: remove dead code and silence warnings across engine + cluster Dead-code sweep follow-up to the RFC-013 stack. No behavior change. - engine: delete the orphaned `validate_edge_cardinality` — the load path uses `validate_edge_cardinality_with_pending_loader` for every mode (including Overwrite, which it treats as the replacement table image), so the old standalone validator had no caller — and correct its sibling's now-stale doc reference. Gate `TableStore::append_batch` `#[cfg(test)]`: it is the inline- commit residual kept only for recovery test setup, with no non-test caller. - cluster: drop unused imports in `lib.rs`, delete the unused `ClusterStore::payload_display`, and raise `LiveGraphObservation` / `GraphObservationJson` / `PolicyTarget` to `pub(crate)` to match the functions that return them. Both lib crates now build warning-free. * fix(engine): match Lance's typed DatasetAlreadyExists, not the message string The internal create-or-open idempotency fallbacks in `db/commit_graph.rs` and `db/recovery_audit.rs` classified the "already exists" race by `err.to_string().contains("Dataset already exists")` — a Lance display string, not an API contract. A wording change upstream would silently break the fallback (a re-create would error instead of opening the existing table). Match the typed `lance::Error::DatasetAlreadyExists { .. }` variant instead — the same discipline as the v3→v4 migration's not-found classifier — pinned by the new `lance_surface_guards.rs::lance_error_dataset_already_exists_variant_exists` guard so a Lance rename turns red instead of silently regressing. * refactor(engine): consolidate now_micros into one crate::db helper Four `fn now_micros() -> Result<i64>` copies (commit_graph, recovery_audit, graph_coordinator, manifest/graph) had already drifted: three mapped the clock error to `OmniError::manifest("...UNIX_EPOCH...")` while recovery_audit used `OmniError::manifest_internal("...unix epoch...")`. Replace all four with one `pub(crate) fn now_micros()` in `db/mod.rs` (the majority `manifest` variant), and repoint the eight call sites at `crate::db::now_micros()`. No test asserts on the failure message, so unifying the variant is behavior-safe; the timestamp-mapping contract can no longer fork across the rows it stamps. * refactor(engine): drop the dead snapshot param from roll_back_sidecar `roll_back_sidecar` took `snapshot: &Snapshot` only to discard it with `let _ = snapshot;` — rollbacks now always publish (the restored HEAD plus a recovery-commit lineage row), so the snapshot is never read to decide whether to skip a publish. Remove the parameter, the two call-site arguments, and the suppressor. A signature must not advertise inputs it does not consume. The `Snapshot` import stays — `process_sidecar`, `roll_forward_all`, and `record_audit_recovery_rollforward` still take it. * test(engine): red — open_at_branch wedges a branch on a missing commit-graph ref A v4 graph keeps its graph lineage in `__manifest` (RFC-013 Phase 7); the `_graph_commits.lance` branch ref is a derived artifact. An interrupted fork-reclaim or a `cleanup` race can drop that derived ref while the manifest lineage stays intact. Per invariants 7 + 15 a missing derived ref must not fail a logical read of the lineage. This wedge builds a real v4 `feature` branch (its `graph_head:feature` row in `__manifest`), force-deletes ONLY the `_graph_commits.lance` `feature` ref, then asserts the branch reads (`open_at_branch` / list-commits / `merge_base`) succeed from `__manifest` while a write that needs the derived ref (`create_branch`) fails loudly with the typed actionable error. Red against current code: `open_at_branch`'s hard `checkout_branch(branch)?` on the missing ref errors `OmniError::Lance` (Lance "Not found: _graph_commits.lance/tree/feature/_versions"), wedging the logical read. * fix(engine): read manifest lineage independent of the derived _graph_commits ref `CommitGraph::open_at_branch` did a hard `checkout_branch(branch)?` on the `_graph_commits.lance` branch ref before reading lineage — so a missing derived ref (an interrupted fork-reclaim, or a `cleanup` race) wedged the branch's commit-list / merge-base / snapshot resolution even though the lineage is readable from the authoritative `__manifest` (RFC-013 Phase 7). That is a derived/physical artifact failing a logical read — invariants 7 and 15. Make the held commits handle `Option<Dataset>` (mirroring `actor_dataset`). `open_at_branch` and `refresh` check out the derived ref best-effort: a typed not-found (`RefNotFound`/`NotFound`) yields a `None` handle while the read re-syncs from `__manifest`; any other open error still propagates. The manifest existence gate is unchanged — `load_commit_cache_for_branch` keeps its hard `?`, so a truly absent branch still fails loudly at the manifest. `create_branch` (the only writer that forks a ref) and the folded-in version lookup return a loud, actionable error on `None`, deferring repair to `cleanup`'s existing orphan reconciler rather than inlining a write on a read-side refresh. Reads (`head_commit`/`load_commits`/`get_commit`/`merge_base`) never touch the handle. Greens the wedge regression from the preceding commit. * fix(engine): v3→v4 retry loops return retryable contention on exhaustion `commit_v4_stamp_idempotently`'s retry loop used `0..=STAMP_RETRY_BUDGET` (6 iterations) with an `attempt < STAMP_RETRY_BUDGET` guard, so the LAST iteration's `IncompatibleTransaction` fell through to `Err(e) => OmniError::Lance(...)` — stringified, non-retryable — instead of the intended `RowLevelCasContention`, and the post-loop contention return was dead code. The publisher's outer retry only re-runs `is_retryable_publish_conflict`, so under sustained concurrent v3→v4 migration the one-time stamp bump could fail instead of converging, defeating the idempotency the migration is supposed to add. Fix the loop to `0..BUDGET` with an UNGUARDED `IncompatibleTransaction` arm: the retryable variant is always handled inside the loop (re-open + same-value check + retry), so it can never reach the stringifying catch-all, and the post-loop is the SINGLE reachable exhaustion path — the typed `RowLevelCasContention`. The `Err(e)` arm now catches only genuine non-contention errors. Apply the same range alignment to the sibling merge loop in `migrate_v3_to_v4` (behaviorally correct today — its `Err(err)` returns the already-typed contention — but it carried the identical off-by-one structure the stamp loop was copied from; aligning both stops the next copy from re-introducing it). Test-first. The exhaustion path is otherwise near-unreachable — a real concurrent winner stamps the same value, so the re-read returns Ok on the first retry — so a new `migration.v4_stamp.force_incompatible` failpoint forces every stamp attempt to lose, driving exhaustion deterministically. Against the pre-fix loop the new `v4_stamp_exhaustion_returns_retryable_contention` test goes red with `Lance("Incompatible transaction: injected failpoint triggered…")`; with the fix it asserts the typed `RowLevelCasContention`. Found by automated review on #299. * feat(engine): minimum-supported internal-schema floor + retirement tripwire The internal-schema migration chain (`migrate_internal_schema`) had a too-new ceiling but no floor, so every old `migrate_vN_…` arm and the v3 legacy readers it needs stay forever — the pile grows by one migration + readers + tests every schema version. Add `MIN_SUPPORTED_INTERNAL_SCHEMA_VERSION` (1 today, a pure no-op: `read_stamp` floors an absent stamp at 1 and no real graph carries 0) as the oldest stamp this binary opens; raising it is how the chain sheds old code. Collapse the one-sided `refuse_if_stamp_too_new` into `refuse_if_stamp_unsupported` checking both bounds, so the floor lands at all three stamp-enforcement sites — the write-path migrate dispatcher, the read-only open guard, and the branch lineage-read path (`commit_graph.rs`) — via one compiler-enforced rename. A hand-wired floor twin would have had to touch each site, and the branch-read path is easy to miss; one combined guard cannot half-enforce. Rename the read-only wrapper `refuse_if_internal_schema_unsupported` to match. A compile-time tripwire (`const _: () = assert!(LOWEST_REGISTERED_MIGRATION_SOURCE == MIN_SUPPORTED…)`) fails the build if a future floor bump forgets to delete the now-dead migration arm (or vice versa) — stronger than a runtime test, impossible to skip, and it doubles as the use that keeps the mirror const live. Tests: a sub-floor graph is refused in both open modes (twin of `future_stamp_is_refused_in_both_open_modes`); the guard accepts exactly [MIN, CURRENT]. No behavior change for any real graph. The retirement runbook lives on the `MIN_SUPPORTED` doc-comment + invariants.md. * fix(engine): compose migration contention with publisher retry; precise recovery-converge audit commit Three review-surfaced fixes on the RFC-013 Phase 7 path. Publisher retry vs migration contention: `publish()` propagated a `load_publish_state` error fatally via `?`, so a `RowLevelCasContention` surfaced by the v3->v4 migration's exhausted merge/stamp budgets aborted the publish instead of being retried — only `merge_rows` conflicts hit the retry. This contradicted the migration's own design, which returns that typed error EXPECTING the publisher to re-run the load (by which point a concurrent winner has usually finished the migration, so the next scan is a no-op). Route a retryable load error through the same retry path as a retryable `merge_rows` conflict. Regression test (failpoints): a one-shot retryable contention injected into `load_publish_state` now commits via the retry; red without the fix (the write fails with the injected contention). Recovery-converge audit commit id: `converge_or_defer_roll_forward` recorded the branch HEAD as the audit row's `graph_commit_id`, but a concurrent user write can advance `graph_head` past the recovery commit between the winner's publish and this read — attributing the audit to a later, wrong commit. Use the latest `RECOVERY_ACTOR`-authored commit (what `publish_recovery_commit` mints), which is the recovery commit by construction. The audit's actor was already correct (it comes from `sidecar.actor_id`, not the commit). Dead param: drop the unused `snapshot` from `record_audit_recovery_rollforward` (removing the `let _ = snapshot;` suppressor). `storage` stays — it is used to delete the sidecar.	2026-06-25 13:55:34 +02:00
Ragnor Comerford	b6c19bfa5d	release: v0.7.2 (#301) ... Patch release over v0.7.1: write-path latency reductions (#288 direct table opener, #298 schema-once + open-each-table-once) and three correctness fixes on the maintenance and recovery paths (#297 optimize survives a cross-process write race, #291 optimize compacts the internal metadata tables + non-destructive auto_cleanup strip, #296 recovery converges under a concurrent manifest advance). No breaking changes, no on-disk format change, no migration. Version coherence: all 7 crate manifests + path-dep constraints, Cargo.lock, openapi.json, and the AGENTS.md surveyed version bumped 0.7.1 -> 0.7.2. Build green under --locked; OpenAPI drift check green.	2026-06-25 09:08:12 +02:00
Ragnor Comerford	4a5277b9c0	fix(recovery): converge roll-forward on concurrent manifest advance (#296) ... * refactor(storage): gate test-only TableStore::append_batch behind cfg(test) The inherent append_batch is used only by in-source recovery test setup, but the non-test lib build (cfg(test) off) cannot see those callers and emitted a dead_code warning. Gating the method #[cfg(test)] silences the false positive and enforces its own doc contract ("no new engine call sites") by construction — engine code physically cannot call a cfg(test) method. * test(failpoints): harden fault-injection harness + reproduce roll-forward CAS race Hardens the test infrastructure around the process-global `fail` registry, and adds a deterministic red repro for the open-time recovery sweep's roll-forward CAS race (iss-schema-apply-reopen-recovery-race). The fix lands in the next commit — this commit is intentionally red (rule 12: red→green visible in log). Harness: - One `ScopedFailPoint` (engine) gaining `with_callback`; the cluster duplicate is removed and cluster tests reuse the engine type via `omnigraph/failpoints`. - `#[serial]` on every failpoint test (the registry is process-global, so shared names interfere under parallelism); `serial_test` added to cluster dev-deps. - `helpers::failpoint::Rendezvous` (park-first / wait-until-reached / release) replaces fixed-`sleep` cross-thread coordination; the three concurrent tests now rendezvous deterministically. The reached flag doubles as a fired-assert. - Compile-checked `failpoints::names` catalog (engine + cluster); every call site references a const, and `failpoint_names_guard.rs` enforces "no string literal names" by source-walk, so a typo is a build error not a silent no-fire. Red repro: - New `recovery.before_roll_forward_publish` failpoint at the sweep's classify -> publish-CAS window (the only injection point there). - `open_sweep_roll_forward_converges_when_manifest_advances_concurrently`: two concurrent open-sweeps race one pending sidecar; the sweep parked at the failpoint loses its publish CAS to the other and fails the open with `ExpectedVersionMismatch`. FAILS at this commit by design. * fix(recovery): converge roll-forward when the manifest advances concurrently The open-time recovery sweep classified a pending sidecar as RolledPastExpected, then published a manifest CAS at the sidecar's pinned expected_version. Under a concurrent writer that advanced the manifest past expected during the classify -> publish window, the CAS failed with ExpectedVersionMismatch and `?`-propagated, failing the whole Omnigraph::open. iss-schema-apply-reopen-recovery-race. A roll-forward's postcondition is "the manifest reflects the sidecar's committed Lance state", not "this sweep won the CAS" (invariants 7 & 15). On an ExpectedVersionMismatch, re-read the live manifest and check whether the sidecar's intent is already satisfied (every pinned table at a version >= the one we observed and tried to publish; added tables registered; tombstones gone — sound under the heal-first invariant, documented at the check). If satisfied, this is convergence: record the RolledForward audit + delete the sidecar idempotently. If only partway, defer to the next pass. Either way the open no longer fails. Other errors still propagate; a genuine logical conflict resurfaces via the classifier's InvariantViolation. Turns the red repro from the previous commit green. The roll-BACK twin (iss-recovery-sweep-live-writer-rollback) is destructive (Lance Restore) and still needs a cross-process lease — the known-gap is updated accordingly. * Address PR review: harden failpoint name guard + dedupe converge audit Two issues surfaced in PR review of the failpoint hardening + recovery fix: 1. Name guard had a line-split blind spot. It scanned per line, so a call wrapped across lines (`park_first(\n "name",\n)`) put the literal on a different line than the call prefix and bypassed the "no string-literal failpoint names" check — and one such literal (`mutation.delete_node_pre_primary_delete`) had slipped through. Make the guard whitespace/newline-tolerant (skip past the open paren to the first argument token) so wrapping can't hide a literal, and convert the bypassed site to the `names::` const. 2. Convergence path could append a duplicate recovery audit. When a roll-forward publish loses its CAS but the manifest already reached the sidecar's goal, `converge_or_defer_roll_forward` recorded a RolledForward audit unconditionally. Under the heal-first invariant, whoever advanced the manifest already healed this sidecar (audit + delete), so a second row landed in `_graph_commit_recoveries` for one recovery event. Gate the audit+delete on the sidecar still being present: absent => the winner completed it, return success with no duplicate row. The convergence regression test now asserts exactly one audit row. * docs(dev): remove the schema-apply recovery-flake handoff (fixed by this PR) The handoff was a transient investigation note for `iss-schema-apply-reopen-recovery-race`, which this PR fixes (the converge helper + the red→green regression). Its rationale now lives durably in the dev-graph issue, the PR/commit history, and invariants.md, so the handoff is obsolete. Drop the doc, its dev-index row, and the dangling reference from the RFC-013 handoff; the doc cross-link check stays green. * fix(recovery): include added-table registrations in the converge audit The CAS-loss convergence audit built outcomes only from `sidecar.tables`, omitting the `additional_registrations` that the normal `roll_forward_all` audit includes. For a SchemaApply sidecar with added types, a converge-path audit row would be incomplete versus the normal roll-forward path for the same recovery kind. Mirror the roll-forward outcome construction (append a registration outcome per added table) so both paths emit the same audit shape.	2026-06-24 19:03:51 +02:00
Ragnor Comerford	7d3a52d674	feat(engine): WriteTxn - validate schema + open each data table once per write (#298) ... * docs(rfc-013): step-3b handoff + §4.1 corrections (validated) Add the RFC-013 write-path handoff doc, and correct §4.1's WriteTxn sketch from the 4-subagent validation against current code: - HandleCache → handle-threading (forward the commit-return handle; a version-keyed cache misses because HEAD walks N→N+1→N+2 across staging + index-build commits). - "re-resolution unrepresentable" softened to "pinned base for the pre-commit phase + named fresh re-reads at the commit/fork boundary" — three reads (commit-time OCC, the live-HEAD drift probe, fork authority) are irreducible correctness machinery. - WriteParams DOES carry a session field; the real constraint is "stage off an open Dataset," so attach the Session by opening read-style then staging off it. * test(engine): RED step-3b capture-once fitness asserts + open_count probe Two write-path cost gates, RED today, GREEN after the WriteTxn lands: - write_validates_schema_contract_once: a write must validate the schema contract once (3 read_text + 2 exists). Today re-validates at every resolve point — measured 12 read_text / 9 exists (~4 validations) via CountingStorageAdapter (zero production change; the write twin of the read-path schema-once test). - keyed_insert_opens_table_at_most_once: a keyed single-table write must open its table <=1x. Today measured 10 opens. Adds an exact open-CALL probe: open_count + record_open() on QueryIoProbes (mirroring probe_count/record_probe), called at both open chokepoints; surfaced as IoCounts.open_count. forbidden_apis guarantees every write open routes through them. * feat(engine): WriteTxn carrier + open_write_txn (3b scaffolding) The capture-once write transaction (RFC-013 step 3b): WriteTxn{branch, base: Snapshot, session} + Omnigraph::open_write_txn, which validates the schema contract once and pins the base snapshot + the shared per-graph Session. Landed as reviewed scaffolding (gated #[allow(dead_code)]); the next pass threads Option<&WriteTxn> through open_for_mutation_on_branch / staging on the non-strict bound-branch path — opening the base once from the pinned entry with the warm session (a session-aware pinned opener returning a SnapshotHandle) and skipping the per-table schema re-validation — to turn the two RED cost gates green. Strict ops / fork / the commit-time OCC re-read keep their fresh reads. * test(engine): scope write-path open_count to data tables (RFC-013 step 3b) The keyed_insert_opens_table_at_most_once gate asserted open_count <= 1, but open_count was a single unclassified counter: record_open() fires in both open chokepoints, and open_dataset_tracked also opens the internal/system tables (__manifest via layout.rs, _graph_commits/_graph_commit_actors via commit_graph.rs). So the count conflated data-table opens with the publisher CAS + commit-graph append opens — making the gate measure the wrong quantity and unreachable by threading alone (the manifest publish keeps it >1 regardless). Scope it by table class, mirroring the read-side counters (which already split by URI prefix via separate wrappers): record_open(uri) classifies the open's last path segment and feeds data_open_count vs internal_open_count. IoCounts exposes both; the gate now asserts data_open_count <= 1. Re-baselined: a single keyed insert is data_open_count=4 / internal_open_count=6 (sum 10, the old conflated value). The RED target for the WriteTxn threading is now the real data-table-open count (4 -> 1), with internal opens correctly out of scope. Pure test-harness/instrumentation; no production behavior change (classification runs only inside the probe closure, skipped when no probes are installed). Also marks #297 (optimize-vs-write race) as landed in the step-3b handoff — this branch is already stacked on origin/main after it merged. * feat(engine): validate the schema contract once per write (RFC-013 step 3b) A single mutate/load re-validated the schema contract ~4 times: at the entry (ensure_schema_state_valid), per-table in open_for_mutation_on_branch (resolved_branch_target), at the commit-time OCC re-read (fresh_snapshot_for_branch), and in the publisher's index-build snapshot (snapshot_for_branch). Each validation is 3 read_text + 2 exists on the storage adapter — O(touched resolve-points) of redundant contract I/O on every write. Thread the already-landed WriteTxn carrier through the write path: capture `txn = open_write_txn(branch)` once at the mutate/load entry (the single validation), then source the per-table entry and the commit/publish snapshots from `txn.base` instead of re-resolving. When `txn` is None (branch merge, schema apply, tests) every function is byte-identical to before. - mutate_with_current_actor / load_jsonl_reader capture txn once (replacing the entry-point ensure_schema_state_valid) and thread Some(&txn) through execute_*/open_table_for_mutation, commit_all, and commit_updates_on_branch_with_expected. - open_for_mutation_on_branch sources (snapshot, branch) from txn.base/txn.branch when present — skipping resolved_branch_target's re-validation. The OPEN itself is unchanged (still HEAD via open_dataset_head_for_write), and strict ops keep ensure_expected_version. Schema-once applies to strict and non-strict alike; the data-open collapse is a separate change. - commit_all uses fresh_snapshot_for_branch_unchecked (the OCC manifest re-read minus the schema re-validation) when txn is present; the drift guard is unchanged. - prepare_updates_for_commit uses txn.base for the publisher index-build snapshot. fresh_snapshot_for_branch{,_unchecked} now read the manifest directly via ManifestCoordinator instead of resolve_target. The OCC re-read consumes only the Snapshot (per-table location + version), which ManifestCoordinator::open().snapshot() produces identically — but resolve_target additionally opened the commit graph (a spurious _graph_commits.lance exists probe the OCC read never consults). Dropping that load is a pure read-cost reduction for every fresh-snapshot caller (commit_all's None arm, optimize, repair, fork reclaim); the returned Snapshot is unchanged and the read is a fresher cold manifest re-read, so the OCC freshness guarantee is preserved. Greens write_validates_schema_contract_once (3 read_text / 2 exists, was 12/9). keyed_insert_opens_table_at_most_once stays red (data_open_count=4) — the open collapse lands next. Full engine suite green otherwise. * feat(engine): open each data table once per write (RFC-013 step 3b) A single keyed-node mutate opened its data table 4 times: accumulation (to read .version()), staging (the real write base), the commit-time drift guard (to read live HEAD), and the publisher's index build (reopen at the just-committed version). Collapse three of the four — using the WriteTxn carrier threaded for schema-once — so a write opens each touched data table at most once. - #1 accumulation: open_for_mutation_on_branch now returns (Option<SnapshotHandle>, expected_version, full_path, table_branch). On the txn's own branch, a non-strict (Insert/Merge) op needs no open — the only thing the caller reads is .version() (the CAS fence), which is exactly the pinned base version (entry.table_version). So skip open_dataset_head_for_write and source the version from txn.base. The node insert path already discarded that handle; the edge path resolves a pinned read only when non-default cardinality needs it. STRICT ops and any write that must fork still open live HEAD + ensure_expected_version. - #3 commit drift guard: commit_all reads live HEAD via entry.dataset.dataset().latest_version_id() — a cheap manifest-pointer probe off the already-open staging handle (the same primitive ManifestCoordinator:: probe_latest_version uses) instead of a fresh open_dataset_head_for_write. The head<current / head>current drift classification is byte-identical. - #4 index build: commit_all now returns the per-table post-commit_staged SnapshotHandle map; commit_updates_on_branch_with_expected threads it into prepare_updates_for_commit, which builds indices on the threaded handle instead of reopening at the same just-committed version. Absent a handle (other writers, inline/delete tables) the reopen path is byte-identical. When txn is None (branch merge, schema apply, tests) every function opens and checks exactly as before. Greens keyed_insert_opens_table_at_most_once (data_open_count 4->1). Schema-once gate stays 3/2. Full engine suite + failpoints (recovery sidecar lifecycle) green. * refactor(engine): name the write-path open/commit returns (RFC-013 step 3b) The open collapse left two positional returns that are easy to mis-thread and carry an unwritten contract: open_for_mutation_on_branch's (Option<SnapshotHandle>, u64, String, Option<String>) and commit_all's 5-tuple (updates, expected_versions, sidecar_handle, guards, committed_handles). Replace both with named structs so each field reads at the call site and the Option's contract is documented, not folklore. - OpenedForMutation { handle, expected_version, full_path, table_branch } with a require_handle(ctx) helper for the callers that must have a handle (strict ops, the fork path, every no-txn caller — branch merge, the seed test). The handle is None only on the non-strict-txn open-skip path (collapse #1); require_handle panics with a named context if that contract is ever broken. - CommittedMutation { updates, expected_versions, sidecar_handle, guards, committed_handles } for commit_all; consumers destructure into the same local bindings they already used, so the publish/sidecar/guard-hold logic is unchanged. - A debug_assert in open_table_for_mutation pins the skip contract: a missing handle is legal only on the non-strict txn path, so a future strict arm returning None trips in debug builds instead of handing None to a require_handle consumer. Pure refactor — no behavior change. Both cost gates stay green (schema 3/2, data_open_count=1), full engine suite + lib (162) green. * refactor(engine): drop the unearned session field from WriteTxn (RFC-013 step 3b) The open collapse greens data_open_count<=1 by SKIPPING the accumulation open, PROBING live HEAD with latest_version_id, and REUSING the commit_staged handle — none of which consume a session. The captured WriteTxn.session was therefore dead (`#[allow(dead_code)]`): unearned surface a reviewer rightly flags. Remove it. The carrier is now {branch, base} — exactly what schema-once + the open collapse use. Step 5 (PublishPlan unification) makes WriteTxn the non-optional publish carrier and is the right home for session-aware base opens, where the warm-session benefit on the single remaining open — an object-store (S3) phenomenon, invisible on local FS — can be earned by its own cost gate rather than carried dead through this PR. No behavior change; both cost gates stay green (schema 3/2, data_open_count=1). * docs(rfc-013): mark step 3b DONE — schema-once + open-collapse shipped, session deferred to step 5 * docs(rfc-013): capture the write-base-staleness convergence (§1d) Three findings this cycle share one root — the write base is a stale, un-probed, un-classified pin (the read path probes; the write path returns the warm coordinator snapshot): - #298 edge-@card stale-read regression (cursor High / codex P1, VALID): collapse #1 made the cardinality scan read txn.base instead of live HEAD, so a concurrent edge is uncounted and a max can be exceeded. Fix on #298: restore the live-HEAD read + deterministic test + correct the single-writer doc comment. - The structural liability underneath: no unified write-validation read-set — endpoint/cardinality/uniqueness each pick freshness ad hoc (warm/pinned/live), the same cardinality check forks mutation-vs-loader, none re-validated at commit. - The served-strict-write stale-view false-fail (validated on prod + a #[ignore] repro): a strict update/delete false-fails ExpectedVersionMismatch after an external optimize advance — the write-side mirror of #297/§6.6. The naive blanket probe is proven wrong (breaks the cross-process lost-update OCC contract). All three converge on Design A (step 5): open_txn's warm probe makes the base fresh, the op-class-aware precondition (derive maintenance vs logical from Lance per-version transaction metadata — no parallel marker) fast-forwards maintenance and fails logical, and §7.1's read-set-in-CAS unifies + re-validates the validation read-set. §8 records the #298 follow-up, the widened §7.1 scope, and the step-5 two-test acceptance contract. * test(engine): RED — edge @card must scan live HEAD, not stale txn.base (#298) Regression guard for the cursor-High/codex-P1 finding on #298: 3b's collapse #1 made the non-strict edge-insert cardinality scan read the pinned txn.base instead of live HEAD (edge_cardinality_read_handle), so a concurrent edge committed after txn capture is uncounted and a @card max is silently exceeded (invariant 9). Deterministic two-handle test (no failpoint): handle A commits WorksAt(Alice->Acme) to the @card(0..1) max; stale handle B (never read since) inserts a second WorksAt for Alice. B's coordinator is stale by construction (the write path doesn't probe), so B scans txn.base (Alice has 0) and wrongly commits the 2nd edge. RED: the insert that must be rejected currently succeeds (panics at unwrap_err). Goes green when the scan reads live HEAD. * fix(engine): scan live HEAD for edge @card, not the pinned txn.base (#298) 3b's collapse #1 skips the non-strict edge accumulation open, so edge_cardinality_ read_handle reopened the edge table at the pinned txn.base for the @card scan. Since cardinality is validated once (never rechecked at commit), a concurrent edge committed after txn capture was uncounted and a @card max could be silently exceeded (invariant 9) — the cursor-High/codex-P1 regression on #298. Pre-3b the scan read live HEAD (the mutation's own open_dataset_head_for_write handle). Restore the live-HEAD read: take the table LOCATION from the pinned entry (stable across versions) and open the dataset at its current HEAD via open_dataset_head_for_ write. Gate-safe — the data_open_count / merge-insert-only gates are node inserts; the edge cardinality path (non-default @card only) is untouched by them, and the extra live-HEAD open is exactly the pre-3b shape. Also drops the dead None-fallback's schema re-validation (greptile P2, auto-resolved). The residual validate->commit TOCTOU is the pre-existing §7.1 gap (RFC-013 step 4), recorded in handoff §1d/§8. Turns cardinality_rejected_for_stale_handle_after_concurrent_edge_commit green; validators / write_cost / writes / consistency / end_to_end / branching all green. * docs(dev): link handoff docs from index * docs(engine): tighten 3b claims to match the code (#298 review) Review caught several comments/docs overclaiming what the code does (the session drop + the #298 cardinality fix left stale/too-strong wording). No logic change. - open_write_txn doc: drop the stale "shared per-graph Session" (WriteTxn no longer carries one); scope "once" to the table-touch hot path and note edge/load RI validation still re-resolves (→ step 4 §7.1) + the session-aware open is step 5. - edge cardinality call-site comment: it said the scan uses a "pinned txn.base" — it now opens LIVE HEAD (#298); corrected. - write_cost.rs: "opens the base once (with the shared Session)" → session-aware base open is deferred to step 5. - data_open_count completeness (instrumentation.rs + write_cost.rs): forbidden_apis only keeps engine code OUTSIDE the storage layer on the chokepoints; table_store.rs is allow-listed and holds direct Dataset::opens for branch-management ops (not the keyed-write hot path the gate measures). Narrowed the claim accordingly. - handoff §4: "schema once / open once" is the node hot path (the two gates); edge endpoint + loader RI/cardinality still re-validate and read warm — #298 un-regresses cardinality only, it does NOT close write-validation freshness (that's step 4 §1d/§7.1). build clean; write_cost / validators / forbidden_apis green.	2026-06-23 21:27:31 +02:00
Ragnor Comerford	6d4606a830	fix(engine): optimize survives a cross-process write race on the same table (#297) ... * test(engine): cross-process optimize-vs-write race — RED Two regression tests for the prod bug: a direct `optimize` process racing a served write on the same table fails, because the in-process write queue does not serialize across processes and the data-table optimize path has no retry. - optimize_survives_concurrent_insert_advancing_manifest: a concurrent insert advances the manifest while optimize is paused between compact and publish; optimize's equality-CAS publish then fails "expected X but current Y". - optimize_survives_concurrent_delete_before_compaction: a concurrent delete commits before optimize compacts; Lance rebases the compaction past it cleanly, so optimize again fails the publish CAS (the genuine Lance Rewrite-vs-Rewrite overlap is rarer and shares the internal path's retry). Both fail today with ExpectedVersionMismatch. Adds the `optimize.before_compact` failpoint seam + a wait_for_sidecar helper; serializes the optimize failpoint tests (shared failpoint name). The fix lands next. * fix(engine): optimize survives a cross-process write race on the same table The data-table optimize path trusted the in-process write queue and skipped a retry, so a CLI `optimize` racing a served write (separate processes = separate queues) failed: either the Lance Rewrite lost ("preempted by concurrent Update") or the manifest publish lost the strict equality CAS ("expected X but current Y"). Unify both compaction paths on the internal path's reopen+replan shape, with a two-level retry that matches the two failure points: - Outer loop (reopen+replan): a genuine Lance Rewrite-vs-Update/Delete same- fragment conflict means our compaction did not commit — reopen at the new HEAD and re-plan. Lance rebases the common disjoint case (a concurrent insert/delete on other fragments) for free, so this fires only on real overlap. - Inner loop (Phase C, monotonic publish): the manifest advanced between our compaction and our publish. The compaction is already committed at Lance HEAD N, so we must NOT reopen (that trips the HEAD>manifest drift guard on our own work). Re-read the current manifest version C: if C >= N the manifest already includes our compaction (versions are linear) — no-op; else fast-forward to N. Monotonic, not the strict equality CAS that manufactured the conflict. The Phase-A sidecar is written once and reused across reopen attempts (every Phase-B commit is content-preserving, so recovery rolls the observed HEAD forward or safely rolls the compaction back). The in-process queue is kept — it is now an in-process contention reducer, not the cross-process correctness guard. Shares the COMPACTION_RETRY_BUDGET constant + is_retryable_lance_conflict with the internal path; adds is_retryable_manifest_conflict for the publish loop. No writer_epoch. Turns the prior commit's two race tests green. * docs(rfc-013): two-op-class principle + the found+fixed optimize-vs-write race §6.6 records the maintenance vs logical op-class distinction (maintenance commutes → Lance rebase + reopen/replan + monotonic manifest fast-forward, no writer_epoch; logical → strict cross-process OCC + epoch) and the prod optimize-vs-served-write race that motivated it, now landed. Adds the matching mechanic row to §4.2. * fix(engine): retry must not misclassify optimize's own HEAD drift Review catch on the cross-process optimize fix: the outer retry loop re-ran the `lance_head > manifest` drift guard every iteration. After a partial Phase-B commit (the auto_cleanup strip or compaction commits, then a later op hits a retryable conflict), the reopened attempt saw HEAD ahead of the manifest — from OUR own sidecar-covered work, not an external writer — and deleted the sidecar + returned `skipped_for_drift`, stranding uncovered drift that then needs `repair`. Track `head_advanced` (did one of our Phase-B ops already commit). The drift guard now fires only when `!head_advanced` (genuine pre-existing external drift); once we have advanced HEAD, a reopened HEAD>manifest is our work that the monotonic publish fast-forwards. The no-op early-return likewise publishes prior committed work instead of dropping it when `head_advanced`. Regression test `optimize_retry_does_not_misclassify_own_head_drift` injects one retryable reindex conflict after the compaction commits (new `optimize.inject_ reindex_conflict` seam); red→green verified by negative control (reverting the gate reproduces `skipped_for_drift: Some(DriftNeedsRepair)`). Also de-flake `optimize_survives_concurrent_insert_advancing_manifest`: pause at `before_compact` (not post-compact) so the concurrent insert lands while HEAD== manifest — otherwise it could race optimize's committed-but-unpublished compaction and hit the write-path "HEAD ahead of manifest" guard. * fix(engine): optimize publish converges on retry-budget exhaustion Review catch (greptile): the monotonic Phase-C publish loop returned an error on its final iteration's retryable manifest conflict, even though that conflict can itself mean a concurrent writer published a version that already includes our (content- preserving) compaction — i.e. the postcondition ("the manifest reflects our compaction") is already met. Recovery covered it (no data loss), but the operator saw a spurious error and had to re-run. Restructure the loop to re-read `current` on every retryable conflict and, on budget exhaustion, do a final `current >= state.version` convergence check before surfacing the error — the §6.6 "postcondition is the state, not winning the CAS" principle. Factor the repeated current-version read into `current_manifest_version`.	2026-06-22 13:05:28 +02:00
Ragnor Comerford	5cfae9acc1	docs(rfc-013): latency = (serial_hops + ops/concurrency)·RTT — concurrency-cap correction + Lance-metadata comparison (#292) ... * feat(engine): compact the internal __manifest/_graph_commits tables in optimize `optimize` iterated node/edge catalog tables only, so the two internal system tables (`__manifest`, `_graph_commits`) accumulated one fragment per commit and were never compacted -- making every write's metadata scan O(fragments), which grows forever on a long-lived graph (RFC-013 step 2). `optimize_all_tables` now also compacts both internal tables via a new `compact_internal_table`. They are not catalog-tracked (readers open them at their latest Lance HEAD), so it is a much simpler path than `optimize_one_table`: compact in place, no manifest publish (nothing to publish to), no recovery sidecar (a single atomic Lance commit -- no HEAD-before-publish gap), and no optimize_indices (they carry no Lance index, only object_id's unenforced-PK metadata). No application lock: Lance's compact_files auto-retries its Rewrite against any concurrent writer (the canonical LanceDB pattern; Rewrite vs Append is compatible, vs Update a retryable same-fragment conflict Lance rebases), and a coordinator refresh afterwards makes the warm handle observe the compacted HEAD. Compacts both tables even though Phase 7 (iss-991) will later fold _graph_commits into __manifest -- a one-call throwaway for the full interim win; __manifest compaction is also the prerequisite for Phase 7's graph_head contention. Cleanup (version GC) of the internal tables is deliberately NOT included here: it needs the Q8 cleanup-resurrection watermark first (deferred). maintenance.rs: optimize now returns 6 stats (4 data + 2 internal); adds optimize_compacts_internal_tables (sheds fragments, leaks no recovery sidecar, graph coherent for reads + strict writes after). * test(engine): un-ignore the internal-table scan LOCK (step 2 acceptance) `internal_table_scans_are_flat_in_history` was the RED, #[ignore]'d acceptance gate staged in PR #288. With internal-table compaction landed, a write's __manifest/_graph_commits scan is flat in commit-history depth on a compacted graph (measured __manifest 4->2, _graph_commits 7->3 across depth 10->100, vs the pre-step-2 RED 34->214 / 29->207). The test now compacts at each depth before measuring and runs green every-PR. * docs: RFC-013 step 2 internal-table compaction landed - invariants.md: close the compaction half of the read-path-rederivation known gap (optimize now compacts the internal tables; cleanup half still deferred). - maintenance.md: optimize covers __manifest/_graph_commits (no publish, no sidecar); not yet in cleanup. - rfc-013 §9: split step 2 into 2a (compaction, landed) and 2b (cleanup + Q8 watermark, deferred — debated; MTT-overlap + hot-path liability). - testing.md: the internal-table LOCK is now green every-PR. * fix(engine): guard absent _graph_commits + always compact internal tables Addresses PR #291 review findings: - Greptile (P1): optimize unconditionally opened `_graph_commits` for compaction, but a graph can validly have none (the coordinator opens it as `Option`, gated on `storage.exists`, for graphs predating the commit graph). `Dataset::open` on the absent table errored and failed the whole optimize. Guard the `_graph_commits` compaction with the same `storage_adapter().exists()` check the coordinator uses; `__manifest` always exists so it stays unguarded. Regression test `optimize_tolerates_absent_graph_commits_table` (empty graph so no publish recreates the table before the guard). - Cursor (low): the `table_tasks.is_empty()` early return skipped internal-table compaction for a schema with no node/edge types. Removed it so the internal tables are compacted regardless of the data-table set. - Codex (auto-cleanup, P1): documented — `compact_files` commits with a default `CommitConfig` (no skip_auto_cleanup) and `CompactionOptions` exposes no override, so on a graph storing an *on* auto_cleanup config the commit would fire version GC. Both internal tables are created with `auto_cleanup: None`, so new graphs are safe; the only exposure is pre-fix upgraded graphs, identical to the existing data-table optimize path, with step 2b's watermark as the comprehensive guard. Added a comment in `compact_internal_table` recording this. * docs(rfc-013): serial-hop correction — wall-clock is the ~110-hop backbone, not op count Latency-slope measurement on the deployed edge binary (f6d2cc03, steps 1+3a landed; rustfs + per-op latency proxy, depth 1..85) shows wall-clock is set by a ~110-hop SERIAL backbone that is depth-invariant. Total ops grow +~7/depth but PARALLELIZE (parallelism 1->6), so the depth term adds little wall-clock. - New §0(c): the serial-hop vs total-op finding + branch-op backbones (create ~77, delete ~87, branch-write ~258/1777-ops/21s floor = fork-on-first-write). - §2.4: correct the '1720->198 ops => 258s->30s' op-count->wall-clock conversion. - §5.1: promote serial-hop/num_stages to the PRIMARY latency LOCK; op-count flatness demoted to a cost/compute-floor gate. - §9 step 2: reprioritized as Phase-7 prerequisite + compute-floor/space, NOT the wall-clock fix; step 3b (parallel capture-once WriteTxn) is the headline latency lever; branch-write moved under step 3b + fork seam. - Summary: serial-backbone correction up front. Vindicates the §3/§4.1 design; corrects the op-count latency framing. * docs(rfc-013): concurrency-cap correction + Lance-metadata comparison Fold in two measured findings from the deployed edge binary (f6d2cc03) on rustfs behind a latency+concurrency proxy: - §0(d): concurrency-cap A/B. Under unlimited concurrency the internal-table scan parallelizes (backbone ~110); under an R2-realistic cap (8) it serializes and an UNCOMPACTED graph runs away (per-write ops 1273->3505, wall 6->16s), while #291's internal compaction cuts it ~6x and bounds it (137->1 frag). The latency model is (serial_hops + ops/effective_concurrency)*RTT + compute. - Reframe step 2 across Summary/§2.4/§9: NOT de-ranked — on R2 (capped) it is a primary latency lever + the anti-runaway fix + Phase-7 prereq. The earlier 'step 2 is parallel, irrelevant to latency' was an unlimited-concurrency artifact. Deployed f6d2cc03 optimize is node/edge-only; #291 (undeployed) is the prod win. - §5.1: the cost-gate ThrottledStore must cap concurrency AND inject latency; assert serial_hops flat AND ops flat in history. - §2.3 + §8: Lance/LanceDB comparison from 7.0.0 source — Lance metadata is a single-file per-version manifest read O(1) (latest_version_hint), pruned by default; omnigraph's __manifest-as-Lance-dataset scan is self-inflicted by the cross-table-atomicity choice. Adds explicit defense of Lance-dataset __manifest (MTT seam) vs a flat-file CAS'd manifest (cheaper, off the MTT path). Design (§3/§4.1) unchanged and vindicated; corrections are measurement framing, step sizing, and one design-choice that was implicit.	2026-06-21 21:54:59 +02:00
Ragnor Comerford	f2b792e0ae	(feat): compact the internal manifest/commit-graph tables in optimize (#291) ... * feat(engine): compact the internal __manifest/_graph_commits tables in optimize `optimize` iterated node/edge catalog tables only, so the two internal system tables (`__manifest`, `_graph_commits`) accumulated one fragment per commit and were never compacted -- making every write's metadata scan O(fragments), which grows forever on a long-lived graph (RFC-013 step 2). `optimize_all_tables` now also compacts both internal tables via a new `compact_internal_table`. They are not catalog-tracked (readers open them at their latest Lance HEAD), so it is a much simpler path than `optimize_one_table`: compact in place, no manifest publish (nothing to publish to), no recovery sidecar (a single atomic Lance commit -- no HEAD-before-publish gap), and no optimize_indices (they carry no Lance index, only object_id's unenforced-PK metadata). No application lock: Lance's compact_files auto-retries its Rewrite against any concurrent writer (the canonical LanceDB pattern; Rewrite vs Append is compatible, vs Update a retryable same-fragment conflict Lance rebases), and a coordinator refresh afterwards makes the warm handle observe the compacted HEAD. Compacts both tables even though Phase 7 (iss-991) will later fold _graph_commits into __manifest -- a one-call throwaway for the full interim win; __manifest compaction is also the prerequisite for Phase 7's graph_head contention. Cleanup (version GC) of the internal tables is deliberately NOT included here: it needs the Q8 cleanup-resurrection watermark first (deferred). maintenance.rs: optimize now returns 6 stats (4 data + 2 internal); adds optimize_compacts_internal_tables (sheds fragments, leaks no recovery sidecar, graph coherent for reads + strict writes after). * test(engine): un-ignore the internal-table scan LOCK (step 2 acceptance) `internal_table_scans_are_flat_in_history` was the RED, #[ignore]'d acceptance gate staged in PR #288. With internal-table compaction landed, a write's __manifest/_graph_commits scan is flat in commit-history depth on a compacted graph (measured __manifest 4->2, _graph_commits 7->3 across depth 10->100, vs the pre-step-2 RED 34->214 / 29->207). The test now compacts at each depth before measuring and runs green every-PR. * docs: RFC-013 step 2 internal-table compaction landed - invariants.md: close the compaction half of the read-path-rederivation known gap (optimize now compacts the internal tables; cleanup half still deferred). - maintenance.md: optimize covers __manifest/_graph_commits (no publish, no sidecar); not yet in cleanup. - rfc-013 §9: split step 2 into 2a (compaction, landed) and 2b (cleanup + Q8 watermark, deferred — debated; MTT-overlap + hot-path liability). - testing.md: the internal-table LOCK is now green every-PR. * fix(engine): guard absent _graph_commits + always compact internal tables Addresses PR #291 review findings: - Greptile (P1): optimize unconditionally opened `_graph_commits` for compaction, but a graph can validly have none (the coordinator opens it as `Option`, gated on `storage.exists`, for graphs predating the commit graph). `Dataset::open` on the absent table errored and failed the whole optimize. Guard the `_graph_commits` compaction with the same `storage_adapter().exists()` check the coordinator uses; `__manifest` always exists so it stays unguarded. Regression test `optimize_tolerates_absent_graph_commits_table` (empty graph so no publish recreates the table before the guard). - Cursor (low): the `table_tasks.is_empty()` early return skipped internal-table compaction for a schema with no node/edge types. Removed it so the internal tables are compacted regardless of the data-table set. - Codex (auto-cleanup, P1): documented — `compact_files` commits with a default `CommitConfig` (no skip_auto_cleanup) and `CompactionOptions` exposes no override, so on a graph storing an *on* auto_cleanup config the commit would fire version GC. Both internal tables are created with `auto_cleanup: None`, so new graphs are safe; the only exposure is pre-fix upgraded graphs, identical to the existing data-table optimize path, with step 2b's watermark as the comprehensive guard. Added a comment in `compact_internal_table` recording this. * fix(engine): retry publish on RetryableCommitConflict (compaction vs publish) Step 2 compacts `__manifest` with no app-level lock (Lance OCC arbitrates, validated against LanceDB + the lance-7.0.0 conflict resolver). compact_files' `Operation::Rewrite` auto-retries 20x (CommitConfig default num_retries=20), so a live publish usually wins the race and the compaction rebases. But the publish runs its merge-insert with conflict_retries(0) = one rebase attempt; if the compaction commits first AND the merge touched a fragment the Rewrite rewrote, Lance preempts the publish with `Error::RetryableCommitConflict` — a DIFFERENT variant from the row-level `TooMuchWriteContention` the publisher already retries. Left unhandled, that surfaces a transient error to the caller, i.e. a maintenance compaction (physical op) failing a live write (logical op) — invariant 7. Map `LanceError::RetryableCommitConflict` to a new `ManifestConflictDetails::RetryableCommitConflict` and treat it as retryable in the publisher's outer loop (reload fresh state + re-merge), alongside RowLevelCasContention. `ExpectedVersionMismatch` still propagates (a genuine expectation break must not be blindly retried). This also hardens multi-process concurrent writers generally, not just compaction. Normal publishes are insert-only (new object_ids -> new fragments, disjoint from rewritten old ones), so the conflict is rare; the guard covers the same-fragment-update edge and multi-process writers. Unit tests in publisher.rs pin the mapping + the retry-predicate contract. * revert: publisher RetryableCommitConflict handling (it was the wrong side) Reverts d138902e. Validated against lance-7.0.0: the publisher's merge-insert runs with conflict_retries(0), and execute_with_retry converts an exhausted retryable commit conflict to TooMuchWriteContention before the caller sees it (write/retry.rs ~95-130). So map_lance_publish_error NEVER receives RetryableCommitConflict from merge_rows — it receives TooMuchWriteContention, which the publisher already maps to RowLevelCasContention and retries. The reverted mapping was therefore dead on the real path and its unit test was synthetic. The actual exposure is the *compaction* side: compact_files -> commit_compaction -> apply_commit directly (no execute_with_retry), so a Rewrite-vs-Merge check_txn conflict propagates raw and optimize can fail on a live graph. That is fixed app-side in compact_internal_table in the following commit. * fix(engine): make internal-table compaction correct by construction Address three findings from review of the step-2 internal-table compaction: - Non-destructive by construction: before compacting an internal table, strip any stored `lance.auto_cleanup.*` config off it. `compact_files` commits with a default `CommitConfig` (skip_auto_cleanup=false) and `CompactionOptions` exposes no override, so on a graph created by an older binary (on-by-default GC hook) the compaction commit would fire Lance's auto-cleanup and silently prune `__manifest`-pinned versions. Current binaries store no such config; the strip is the upgrade-path safety net so `optimize` can never GC versions. - App-level compaction retry: `compact_files` does NOT auto-retry a semantic conflict against a concurrent live writer (Rewrite vs Update/Merge/Delete propagates raw from apply_commit; Lance prescribes app-rerun). Wrap the internal-table compaction in a bounded retry loop that reopens fresh and replans on a retryable Lance conflict, so a maintenance compaction (a physical op) never fails a live write (a logical op) — invariant 7. - Compact all three internal tables, not two: `_graph_commit_actors` grows one fragment per commit on the authenticated write path, the same O(depth) scan as `__manifest`/`_graph_commits`. Drive the sweep from one source-of-truth list with per-table existence guards (the two commit-graph tables are optional). Make `graph_commit_actors_uri` pub(crate). Tests: the `internal_table_scans_are_flat_in_history` LOCK now runs the authenticated (actorful) write path so it covers `_graph_commit_actors` via the shared commit-graph IO wrapper (new `commit_many_as`/`measure_insert_as` helpers); `optimize_clears_stale_auto_cleanup_and_preserves_versions` pins the non-destructive guarantee (config cleared + no version GC); a unit test pins the retryable-conflict classifier; the empty-graph stats count is 7 (the actor table is created at init). * docs: internal-table compaction covers all 3 tables, non-destructive, retried Sync the RFC-013 step-2a section and the maintenance guide with the correctness-by-design refinements: - optimize compacts `__manifest`, `_graph_commits`, AND `_graph_commit_actors` (the actor table grows on the authenticated write path). - optimize is non-destructive by construction — it never GCs versions, and strips stale `lance.auto_cleanup.*` config so an upgraded graph's commit-time GC hook cannot fire during compaction. - internal-table compaction rebases and retries against concurrent live writers rather than failing the operator's optimize or the live write. - the cost LOCK is the authenticated-path acceptance test. * fix(engine): refresh coordinator after a config-strip with no compaction work `compact_internal_table` returns early when `plan_compaction` finds no work, but `clear_stale_auto_cleanup_config` may have already committed a config-strip that advanced Lance HEAD. The early return skipped the coordinator refresh that the successful-compaction path performs, leaving warm `__manifest`/commit-graph handles pinned to the pre-strip version until the next read's version probe healed them. No correctness bug (the probe self-heals, and a stale-handle write would retry via publisher CAS), but the refresh makes coherence deterministic rather than probe-dependent. Refresh iff the config-strip actually committed. * docs(engine): correct compact_internal_table doc — compact_files does not auto-retry The function doc claimed "Lance's compact_files auto-retries its Operation::Rewrite against any concurrent writer" — wrong, and contradicting the is_retryable_lance_conflict doc just below it and the explicit retry loop that exists precisely because compact_files does NOT auto-retry semantic conflicts (Rewrite vs Update/Merge/Delete propagates raw through apply_commit). Also move the orphaned description from above the retry-budget const onto the function, and include the third internal table. * test(engine): optimize must clear stale auto_cleanup on DATA tables too (red) Regression test for a destructive bug on the data-table optimize path: on an upgraded graph whose node/edge table still carries pre-v7 lance.auto_cleanup.* config, `optimize`'s compact_files/optimize_indices commits fire Lance's version GC and prune __manifest-pinned data-table versions. Mirrors the internal-table auto_cleanup test on a Person table (force-repair realigns the config-induced drift so optimize doesn't skip the table). Red against the current code: the data-table path does not strip the config. The fix lands in the next commit. * fix(engine): clear stale auto_cleanup on the data-table optimize path too The auto_cleanup scrub previously only protected the internal tables; the data-table path (optimize_one_table) ran compact_files/optimize_indices with a default CommitConfig (skip_auto_cleanup=false) and no override, so on an upgraded graph those commits could fire Lance's version-GC hook and prune __manifest-pinned node/edge versions — making the "non-destructive" contract false for data tables. Strip the config before the HEAD-advancing commits, capturing version_before first so the strip's own commit still triggers the Phase-C manifest publish (no uncovered drift). No retry loop needed: the data-table path holds the per-table write queue. Covered by the existing Optimize recovery sidecar. Turns the prior commit's test green. Also: switch clear_stale_auto_cleanup_config off the deprecated delete_config_keys to update_config(None values), and correct two now-inaccurate doc comments — compaction is "one or more content-preserving commits" (compact_files can emit a ReserveFragments before the Rewrite), not "a single atomic commit"; the sidecar-free property rests on content-preservation + read-at-HEAD, not single-commit atomicity. * docs: optimize is non-destructive on all tables; correct atomicity/retry claims - non-destructive guarantee now spans data + internal tables (the auto_cleanup strip runs on both paths), not just the internal ones. - "single atomic Lance commit" was inaccurate: compaction can emit a ReserveFragments commit before the Rewrite; the no-sidecar property rests on content-preservation + read-at-HEAD, not single-commit atomicity. - "retries rather than failing" softened to the truth: a *bounded* retry on the internal path; sustained contention surfaces a loud conflict error (bounded + observable, not an infinite loop). The data path holds the per-table queue and never contends.	2026-06-21 16:38:20 +02:00
Andrew Altshuler	fff441196c	docs(dev): update coherence ledger — cookbooks drift resolved, omnigraph-ts mechanism (#294) ... - omnigraph-cookbooks `bearer_token_env` chain: RESOLVED by cookbooks PR #26 (deleted docs/best-practices.md in the 0.7 restructure). - omnigraph-ts catalog `mcp.expose` description: documented why there is no hand-fix — the SDK syncs openapi.json from a *tagged* omnigraph release, and the fix landed on main after the v0.7.1 tag, so it flows in on the next SDK version bump (v0.7.2+) rather than an out-of-band patch. Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-21 00:11:48 +03:00
Andrew Altshuler	9c792649e2	docs(user): coherence cleanup aligned with 0.7.1 (#293) ... * docs(cli): fix cluster apply semantics — converges graphs+schema, not config-only `cluster apply` creates graphs, applies schema updates (soft drops), writes stored-query/policy catalog resources, and executes approved graph deletes in one ordered run. Both the user docs and the shipped CLI help text still described it as a "Stage 3A" config-only (query/policy) subset that defers graph/schema changes "to a later stage" — wrong since the graph/schema executor landed. - docs/user/cli/reference.md: rewrite the cluster paragraph to describe apply's actual converge behavior; keep deferred for the genuinely-unsupported case (standalone schema deletes); drop the stale "Stage 3A" / "reserved for later stages" framing. - crates/omnigraph-cli/src/cli.rs: fix the `cluster apply` help text to match. Part of the docs/user coherence cleanup (docs/dev/docs-issues.md, P1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 * docs(server): align stored-query exposure with cluster-only behavior server.md documented a per-query expose knob ("`mcp.expose` defaults to true; set `mcp: { expose: false }` to hide from the catalog") that does not exist in the only deployment mode. Cluster-only serving lists every stored query: the cluster registry has no expose field (`QueryConfig { file }`) and the boot bridge hardcodes `expose: true` for all cluster queries (omnigraph-server settings), and there is no GQ-level expose annotation. This contradicted clusters/config.md, which already states the correct behavior. Replace the knob bullet with the cluster truth (every applied query is listed; per-query exposure may become a Cedar-policy decision later) and drop the "`mcp.expose` stored queries" phrasing from the catalog description, the endpoint table, and the intro. The `mcp_expose` JSON catalog field is unchanged (still emitted, always true in cluster mode). Part of the docs/user coherence cleanup (docs/dev/docs-issues.md, P1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 * docs(schema): split direct/embedded vs cluster-managed schema apply schema/index.md claimed `allow_data_loss` is "honored uniformly across transports" and listed HTTP `POST /schema/apply` among them. But that route is 409-disabled for cluster-backed serving (already documented in server.md), and cluster-managed graphs evolve only through `cluster apply` with soft drops — there is no cluster HTTP data-loss path. Scope the data-loss flag to the direct/embedded path (`schema apply --store`, SDK), and add a paragraph: cluster-managed graphs use `cluster apply` (soft drops only); HTTP `POST /schema/apply` is 409 for cluster serving; direct apply against a cluster-managed path is refused. Cross-refs server + cluster docs. Part of the docs/user coherence cleanup (docs/dev/docs-issues.md, P2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 * docs(server): document /load as canonical in limits + admission prose The endpoint table already listed both `/load` (canonical) and `/ingest` (deprecated alias) at 32 MB, but the admission-control, body-limit, rate-limit, and manifest-conflict prose named only `/ingest` — and the constants page called the limit "Ingest body limit". Add `/load` alongside (or ahead of) `/ingest` everywhere, and rename the constant to "Load (bulk-write) body limit" noting the `/ingest` alias shares it. Part of the docs/user coherence cleanup (docs/dev/docs-issues.md, P2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 * docs(cli): drop stale bearer-token keys + fix version string The "Bearer token resolution (CLI)" section still listed removed omnigraph.yaml keys (`graphs.<name>.bearer_token_env`, `auth.env_file`) — config surfaces that no longer exist and that implied plaintext tokens in config. Replace it with a pointer to the keyed-credential model documented above (`OMNIGRAPH_TOKEN_<NAME>` → `~/.omnigraph/credentials` → `OMNIGRAPH_BEARER_TOKEN`). Also fix the `version` row: the CLI prints 0.7.x, not 0.3.x. Part of the docs/user coherence cleanup (docs/dev/docs-issues.md, P2 + smaller). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 * docs: route-spelling note + drop stale stage/deferred crumbs - server.md: add a one-line note that the per-graph subsections name routes in shorthand (`GET /queries`, `POST /query`, `POST /mutate`, `POST /queries/{name}`) but every one is served under `/graphs/{id}/…` — the endpoint table is already fully-qualified. - clusters/config.md: redefine the `deferred` plan disposition as an unsupported change (e.g. a standalone schema delete) instead of "graph/schema change, later phase" (graph creates and schema updates apply now); drop the "Stage 2C" label from the lock-recovery note. - search/indexes.md: `ingest --mode merge` → canonical `load --mode merge`. Part of the docs/user coherence cleanup (docs/dev/docs-issues.md, P2 + smaller). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 * docs(dev): track user-docs coherence ledger; mark 2026-06-20 findings resolved Convert the scratch review notes into a tracked living ledger and link it from the dev index. All ten findings from the 2026-06-20 docs/user sweep are validated and fixed in this branch (P1 cluster-apply semantics + stored-query exposure; P2 schema-apply paths, /load canonical, bearer-token keys, route shorthand; plus version/ingest/deferred/stage crumbs). The verification grep checklist is retained for future audits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 * docs(api): align GET /queries OpenAPI contract with cluster-only behavior Greptile P1 on #293: the prose fix in server.md left the OpenAPI surface stale. The utoipa annotations (handlers.rs, omnigraph-api-types QueriesCatalogOutput) still described the catalog as "the `mcp.expose == true` subset", and those drive the checked-in openapi.json — so SDK consumers read a contract the cluster-only server does not honor (it lists every stored query). Update the three Rust doc-comment/annotation strings to "every stored query" and regenerate openapi.json (OMNIGRAPH_UPDATE_OPENAPI=1; drift test green) in the same change, per AGENTS.md rule 4. Ledger updated: this finding resolved, plus the cross-repo drift it surfaced (omnigraph-ts generated spec/types and omnigraph-cookbooks best-practices bearer_token_env) tracked as open follow-ups. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-21 00:02:34 +03:00
Ragnor Comerford	f6d2cc03e3	write-path cost gate + opener bypass (#288) ... * docs(rfc): RFC-013 write-path latency design + index link * perf(engine): open write-path tables directly, bypassing the namespace builder Write opens routed through DatasetBuilder::from_namespace, whose describe_table opened the whole dataset just to return a location and then re-resolved the latest version — an O(commit-depth) double latest-resolution per table open that missed Lance's O(1) version-hint fast path. On an object store this dominated write latency (~70%, RFC-013 section 2.4). TableStore::open_dataset_head_for_write now delegates to the direct opener (open_dataset_head: Dataset::open by URI + checkout_branch, routed through the tracked opener so cost tests can count it; a no-op in production). The manifest already holds every sub-table's location, so the namespace catalog lookup was redundant; ensure_expected_version still validates head == pinned for strict ops. This completes PR #268's open-by-location migration on the write side. With both reads (PR #268) and now writes bypassing it, nothing in production routes through the per-table Lance namespace. The dead open chain (load_table_from_namespace, open_table_head_for_write) is deleted and the StagedTableNamespace contract apparatus is gated #[cfg(test)], mirroring the already-test-only read namespace; __manifest commit coordination (GraphNamespacePublisher) is a separate component and is unaffected. See docs/dev/rfc-013-write-path-latency.md sections 2.4 and 9 (step 3a). * test(engine): write-path cost-budget gate on a shared harness Adds tests/helpers/cost.rs, a store-agnostic cost harness (IoCounts/StagedCounts, measure/measure_with_staged, assert_flat, local_graph/s3_graph) that the read-side warm_read_cost.rs, write_cost.rs, and write_cost_s3.rs share, so the IOTracker / task-local plumbing lives in exactly one place instead of duplicated per test. write_cost.rs (local, every-PR) gates the internal-table scan term flat in commit-history depth (a RED #[ignore]'d LOCK, the acceptance for bringing the internal tables into compaction) plus green guards: a single insert's data writes are bounded, a per-write read-op ceiling fails the moment a round-trip is added, and a keyed insert routes through stage_merge_insert once with no stage_append or vector-index build. write_cost_s3.rs (bucket-gated, rustfs CI) gates the data-table opener term flat across depth — the object-store-RPC phenomenon local FS cannot reproduce, and the red->green proof of the opener bypass. Wired into the rustfs_integration CI job and its path filter. Guards the "hot-path cost is bounded by work, not history" invariant on writes. See docs/dev/rfc-013-write-path-latency.md section 5.1, docs/dev/testing.md. * docs(rfc): RFC-013 step 3a landed; write-skew coupling; cost-gate test map - Section 9: mark step 1 (gate + harness) and step 3a (opener bypass) landed; record the per-table namespace retirement to test-only and the corrected measurement note (the opener win is S3-only; the local data-table growth is the merge-insert/RI fragment scan, a compaction term, not the opener). - Sections 7.1/6/11/5.5/10: correct the cross-table write-skew analysis after a prototype proved the scoped expected-set fix is a no-op against the per-object_id manifest (disjoint writers never share a row, so Lance never conflicts, the publisher never retries, and the expected check is a non-atomic pre-check evaluated once against stale state). The fix needs a shared contention row (Phase-7 graph_head / a minimal head row / commit-time re-validation), so it is coupled to that row, not standalone; that contention is load-bearing for correctness, not a drawback. Split the concurrent face (read-set + head) from the sequential face (inbound-RI validation on node removal) -- two different fixes. - testing.md: add write_cost.rs / helpers/cost.rs / write_cost_s3.rs to the test map; document the local-vs-S3 backend split; extend the cost-budget checklist item to the write/open path and point at the shared harness. * test(engine): isolate the opener in the S3 cost gate; fail loud on S3 setup errors Addresses two PR review findings on the bucket-gated write_cost_s3 gate: - The data-table opener was not isolated: `data_reads` also counts the merge-insert/RI scan, which reads O(fragment-count) and so grows with history for a different reason (compaction's domain, not the opener) -- the same term that made the local data-table count grow. The flat assertion would false-RED or misattribute scan growth to the opener on rustfs. Fix: compact (db.optimize) before each measurement so the table holds ~1 fragment, bounding the scan and leaving the opener's latest-version resolution as the only history-varying term. Compaction preserves version history, so the opener still faces a deep _versions/ chain -- the thing under test. - s3_graph used `.ok()?`, so when OMNIGRAPH_S3_TEST_BUCKET was set but the store was down/misconfigured, init/seed failures collapsed to None and the gate skipped + passed vacuously. Fix: skip only when the bucket env var is absent; once it is set, init/seed failures panic (mirrors tests/s3_storage.rs). * test(engine): isolate the S3 opener with a per-prefix IO probe (correct-by-design) Replaces the fixture-bounded isolation (compact-before-measure) from the prior commit with the root fix: a path-classifying ObjectStore wrapper (PrefixCounter) that attributes each data-table read to the opener term (_versions/.manifest) vs the scan term (data/*.lance). IoCounts now exposes data_opener_reads / data_scan_reads, so write_cost_s3 asserts the opener flat *directly* -- no compaction or fixture massaging, and the assertion measures the opener, not the conflated total. Closes the "harness conflates two IO terms" class: any cost test (read or write) can now isolate the opener. PrefixCounter implements only the object_store 0.13 core ObjectStore methods; the convenience surface (get/put/head/...) routes through get_opts/put_opts via ObjectStoreExt's blanket impl, so every read/write is still counted. Validated locally (every-PR) by write_cost::data_table_reads_split_into_flat_opener_ and_growing_scan: opener stays flat (7 -> 3) while scan grows (11 -> 91) and opener + scan == data_total exactly -- proving the classifier and confirming the local data-table growth is the fragment scan, not the opener. warm_read_cost (12 tests) stays green under the shared-harness change. * refactor(tests): remove cost-harness duplication and namespace cfg(test) noise Branch self-review (no behavior change) — pay down three liabilities the write-path work left: - warm_read_cost.rs kept its own probes() (three IOTrackers + a QueryIoProbes + a probe counter) and read raw .stats().read_iops — duplicating the shared helpers::cost harness this branch introduced. Migrated all 12 tests onto measure()/IoCounts; deleted the local probes(). (This also makes IoCounts' version_probes field used rather than dead.) - insert_cost was copy-pasted verbatim into write_cost.rs and write_cost_s3.rs. Hoisted to helpers::cost::measure_insert so the measured write is defined once. - The per-table Lance namespace (namespace.rs) became entirely test-only after step 3a, but was gated with ~22 per-item #[cfg(test)] attributes. Collapsed to a single `#[cfg(test)] mod namespace;` and stripped the per-item attributes; merged the import groups the gating had split. Verified: lib in-source 162 passed; write_cost 4 + warm_read_cost 12 passed; forbidden_apis passed.	2026-06-20 13:31:15 +02:00
Andrew Altshuler	b38b36e48f	release: v0.7.1 (#290) ... Patch release over v0.7.0: three correctness fixes (#283 camelCase filters, #284 cluster-apply crash-loop, #277 branch-merge OOM on embedding tables), the #280 queries-list catalog-metadata improvement, and the #268 warm-read perf fix. No breaking changes, no on-disk format change, no migration. Version coherence: all 7 crate manifests + path-dep constraints, Cargo.lock, openapi.json, and AGENTS.md surveyed version bumped 0.7.0 -> 0.7.1. Full workspace gate green (1472 tests). Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-19 23:12:44 +03:00
Andrew Altshuler	2d34d7c432	test(engine): pin camelCase @index → scalar-index routing (#283 follow-up) (#286) ... Greptile P2 on #285: the #283 tests prove correctness (right rows, type-level coercion) but not that a camelCase @index equality actually reaches the scalar-index path — a result-only test passes on a silent full-scan fallback, exactly the gap testing.md warns about and the bug-case-fix.md validation checklist (step 5) promised to close. Add lance-surface Guard 20 (mirrors Guard 16): build a BTREE on a camelCase column and assert the scan plan contains `ScalarIndexQuery` under the fix's case-preserving `ident()` expr, and that the pre-fix `col()` expr fails to plan (it normalizes `repoName` → a nonexistent `reponame`). A regression that breaks camelCase index routing — or reverts to `col()` — turns this red instead of degrading to a full scan. The existing e2e (`camelcase_property_filter_executes`) already guards the engine call-site (a `col()` revert errors there). Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-19 21:06:13 +03:00
Andrew Altshuler	57348cf7fa	fix(engine): preserve identifier case in filter pushdown (#283) (#285) ... * test(engine): regression tests for #283 camelCase property filters Red against current code. A query (or chained mutation) that filters on a camelCase schema field lints and plans cleanly but fails at run time with "No field named reponame" because the identifier's case is destroyed at the engine->Lance boundary. Coverage added: - query.rs unit: ir_filter_to_expr on a camelCase property must emit an Expr::Column named `repoName`, not `reponame` (red); plus a green coercion guard that a camelCase int column still gets a coerced literal. - mutation.rs unit: predicate_to_sql must emit the column UNQUOTED and case-preserved (green guard documenting the committed-scan contract). - literal_filters.rs e2e: a camelCase @index field with an inline-binding pushdown filter returns the seeded row (red — read pushdown). - writes.rs e2e: an update+delete on a camelCase predicate, and a chained update that re-reads the pending side of scan_with_pending by the same camelCase predicate (red — pending MemTable scan). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 * fix(engine): preserve identifier case in filter pushdown (#283) Two engine->Lance boundaries lowercased camelCase column identifiers, breaking any filter on a camelCase schema field even though the IR, compiler, projection, and in-memory filtering all preserve case. Read pushdown (exec/query.rs, ir_expr_to_expr): build the column reference with datafusion::prelude::ident() instead of col(). col() routes through SQL identifier normalization and lowercases an unquoted identifier (`repoName` -> `reponame`); ident() builds an unqualified, case-preserved Column. Property refs here are always bare column names, so there is no qualified-name handling to lose. No-op for the lowercase columns that work today. Pending mutation scan (table_store.rs, scan_pending_batches): the committed-scan consumer (Lance Scanner::filter(&str)) preserves an unquoted identifier's case but treats a double-quoted "col" as a string literal, so predicate_to_sql must keep the column unquoted. The pending side splices that same unquoted predicate into a DataFusion `SELECT ... WHERE`, which would lowercase it. Make that path case-preserving by disabling sql_parser.enable_ident_normalization on its SessionContext rather than quoting (quoting would match zero committed rows). predicate_to_sql gains only a clarifying comment; its emitted string is unchanged. Full engine suite green (579 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 * docs(dev): case study for #283 camelCase filter bug Record the root cause, the two-boundary fix (read pushdown col→ident; pending mutation scan ident-normalization off), and why the obvious symmetric "quote the column" fix is wrong (Lance reads a double-quoted column as a string literal and silently matches zero committed rows). Linked from a new "Case Studies" section in the dev index so the link check passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-19 18:42:56 +03:00
Andrew Altshuler	3feb23af05	feat(cli): surface stored-query @description/@instruction in queries list (#280) ... * test: e2e coverage for @description/@instruction surfaces Add end-to-end tests pinning the two annotation surfaces as they exist today, at their real boundaries: - engine (lifecycle.rs): schema-level @description (node/edge/property) and @instruction (node/edge) persist verbatim into the on-disk _schema.ir.json through Omnigraph::init; property-level @instruction aborts init and writes no schema IR. - server (stored_queries.rs): query-level @description/@instruction on a stored query surface as typed QueryCatalogEntry fields over GET /queries, and a query declaring neither omits both fields. No behavior change — these document the current contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(cli): surface stored-query @description/@instruction in `queries list` A stored query's @description/@instruction are its catalog metadata — what it does and how to invoke it. The HTTP GET /queries catalog already carries them, but `omnigraph queries list` dropped both fields in human and --json output even though they were available on the registry entry. Carry description/instruction on QueriesListItem (Option, skipped when None) and copy them from the query decl. Human output prints an indented `description:` / `instruction:` line per query when present; --json includes the fields when present and omits them otherwise — matching the HTTP catalog shape documented in docs/user/operations/server.md. Tests (cli_queries.rs): a query with both annotations surfaces them in human + --json; a query with neither prints no annotation lines and omits both JSON fields. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(cli): document `queries list` output incl. description/instruction Per AGENTS.md maintenance Rule 1, document the user-visible `queries list` output alongside the field addition. The `queries` command family had no row in the CLI reference top-level table; add one covering `list` (human + --json shapes, with description/instruction shown only when declared, matching the HTTP GET /queries catalog) and `validate`. Addresses the Greptile P2 review finding on PR #280. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cli): indent multiline stored-query annotations in `queries list` A `@description`/`@instruction` value can be multiline (GQ string literals admit newlines), which made the human `queries list` output break back to the left margin on continuation lines. Indent continuation lines to align under the first via a `print_query_annotation` helper. Addresses review feedback from @martin-g on PR #280. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Ragnor Comerford <ragnor.comerford@gmail.com>	2026-06-19 14:26:50 +03:00
Andrew Altshuler	7fd23c54a3	fix(cluster): stop cluster-apply crash-loops from the recovery-sidecar trap (#284) ... * fix(cluster): stop cluster-apply crash-loops from the recovery-sidecar trap A `cluster apply` carrying a schema change against a graph that has non-main branches, or an unsupported "needs backfill" migration, armed a recovery sidecar *before* calling the engine, then left it behind when the engine rejected the apply pre-movement. The server refuses to boot while any sidecar is pending, and re-running apply re-armed a fresh sidecar — an unescapable crash loop. None of the engine rejections are bugs; the trap is in the apply/serve choreography. Three coordinated changes: 1. Preview before arming the sidecar. `cluster apply` now runs `preview_schema_apply_with_options` before `write_recovery_sidecar`, so parser/planner rejections (non-main branches, unsupported plan) fail loudly without leaving recovery work behind. The post-preview engine error path now deletes the sidecar when the live schema still matches the recorded digest (nothing moved), and keeps it only on real mid-movement failure — both branches covered by new engine-failpoint tests (cluster failpoints now enable omnigraph/failpoints). 2. Per-graph quarantine at serve time instead of whole-cluster refusal. A graph-attributed pending sidecar, an unopenable graph root, a query parse failure, or an unresolvable embedding provider now quarantines just that graph (logged loudly at every boot layer) while healthy graphs serve; `/graphs` lists only ready graphs and quarantined routes 404. Cluster-global problems (missing/unreadable state, malformed or unattributable sidecars, shared-catalog or cluster-policy errors, zero healthy graphs) stay fail-fast. `--require-all-graphs` / OMNIGRAPH_REQUIRE_ALL_GRAPHS=1 restores all-or-nothing boot. 3. Backfill embedding-provider profile metadata on apply. Mirrors the existing policy-binding backfill: a pre-5A ledger missing `embedding_profile` is now detected as a metadata-only change and backfilled by a no-op apply, instead of bricking serve with `embedding_provider_profile_missing` forever. Tests: trap (no sidecar after a rejected apply), both digest-cleanup branches, per-graph quarantine (cluster + server), embedding backfill. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: resilient cluster boot + recovery-sidecar trap fix Amend RFC-005 D4 readiness posture (cluster-global fail-fast vs graph-local quarantine; deviation #5 for --require-all-graphs), add the v0.7.0 release note, and update the user cluster/server/deployment docs and the OMNIGRAPH_REQUIRE_ALL_GRAPHS env var. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cluster): surface sidecar-cleanup failures; document severity promotion Address Greptile review on PR #284: - The pre-movement sidecar cleanup fast-path discarded `delete_object`'s result, so a transient delete failure left the graph quarantined with no signal. Add `try_delete_object` (Result-returning) and emit a `recovery_sidecar_cleanup_failed` warning diagnostic on failure; the fire-and-forget `delete_object` now delegates to it. - Document why the serve-time loop promotes every `list_recovery_sidecars` diagnostic to a cluster-fatal error (the listing only emits genuine read/parse/version failures, as warnings, whose blast radius serving cannot prove) and note the promote-by-code path if that ever changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-19 03:34:15 +03:00
Ragnor Comerford	7168ee0ed0	fix(engine): stop branch-merge fast-forward OOM on embedding tables (#277) ... * fix(engine): stop branch-merge fast-forward OOM on embedding tables A branch→main fast-forward merge of a forked, embedding-bearing table re-derived the whole branch row-by-row: it lumped new + changed rows into one Lance `merge_insert`, i.e. a full-outer hash join over the entire delta that exhausts the DataFusion memory pool (8k rows × 3072-dim → `Resources exhausted: 188MB HashJoinInput, 100MB pool`), so the merge hung/failed instead of completing. Fix the data path on existing, substrate-supported primitives: - Adopt-with-delta split: new rows → `stage_append` (a streaming `Operation::Append`, no hash join), only genuinely-changed rows → a bounded `stage_merge_insert`, deletes inline. New `AdoptDelta` / `compute_adopt_delta` / `publish_adopted_delta` replace the combined `compute_source_delta` path; the three-way merge path is untouched. - Stream the append via `stage_append_stream` → `execute_uncommitted_stream` (the substrate-blessed bulk-append path), removing the `Vec`+`concat` full-delta materialization. Blob-aware via `scan_stream_for_rewrite`. Exposed on the sealed `TableStorage` trait. - Lazy row-signature: stop stringifying every row's embedding eagerly; compute the signature only for the `(Some,Some)` changed-candidate arm. - Index coverage is reconciler-owned: the adopt path no longer rebuilds vector/FTS indexes inline; `optimize`/`ensure_indices` folds the new rows in (reads stay correct via brute-force tail). Post-merge index-coverage contract documented in docs/user/branching/merge.md. - Recovery pin: new `CandidateTableState::AdoptWithDelta` is classified and pinned so the append's HEAD advance is sidecar-covered (invariant 5); the `BranchMerge` sidecar's loose classification covers the two-commit shape. The regression gate is structural, not a brittle size threshold: task-local write probes assert an append-only fast-forward merge does 0 `stage_merge_insert` (the OOM hash join), appends via `stage_append`, and streams (0 whole-delta materialization). Plus functional correctness, blob round-trip, index-defer, and a Phase-B failpoint recovery test. Residual: the classify-time staging round-trip is still O(N) in memory (architecturally required for the all-or-nothing multi-table publish); bounding it fully is the fragment-adopt follow-up. * test(engine): partial branch-merge Phase B must roll back (RED regression) A branch-merge per-table publish is a multi-commit sequence — adopt: append → upsert → delete; three-way: merge_insert → delete → index — each step advancing Lance HEAD before the single manifest publish. Add four failpoint sites at those windows and four regression tests (mixed delta: a fresh id, a modified base id, a removed base id) asserting that a crash mid-sequence rolls the whole merge BACK on the next open and a re-run re-applies the full delta. RED against current code: the loose `BranchMerge` classification rolls any `lance_head > manifest_pinned` forward, so the partial is published and the merge recorded — the rolled-back-to-base assertion fails with the partial state visible (e.g. bob appended, dave not deleted). The fix lands next. The failpoint sites are no-ops unless the `failpoints` feature activates them. * fix(engine): roll back partial branch-merge Phase B (recovery WAL confirmation) A branch-merge publishes each table with several Lance commits (adopt: append → upsert → delete; three-way: merge_insert → delete → index), then one manifest publish makes them atomic. Recovery classified `BranchMerge` loosely: any `lance_head > manifest_pinned` with a matching CAS pin rolled *forward* to the observed HEAD. So a crash mid-sequence published a partial delta (e.g. the append without its sibling upsert/delete) and recorded the merge as complete — silent data loss; a re-merge sees "already up to date" and never repairs it. Fix: make the recovery sidecar a two-phase WAL for `BranchMerge`. After the whole per-table publish loop completes, stamp each pin's `confirmed_version` with its exact achieved Lance version (a second sidecar write), then publish the manifest. Recovery now: - rolls FORWARD only to a pin's `confirmed_version` (set ⇒ Phase B finished); - rolls BACK (`TableClassification::IncompletePhaseB`) when the HEAD moved but no confirmation was recorded ⇒ a partial publish ⇒ all-or-nothing restore to the manifest pin, so a re-run re-applies the full delta. Scope: `BranchMerge` only. Other loose writers (`SchemaApply`, `EnsureIndices`, `Optimize`) keep the loose roll-forward — their drift is derived state (index coverage, compaction) a partial roll-forward never corrupts, so confirmation would be cost without benefit. This is the write-ahead intent record + idempotent roll-forward that the fast-forward-main commit model requires to be crash-atomic across N tables; version-recorded (not phase-count-derived), so it survives later changes to the per-table commit sequence. Regression tests (failpoints): four partial-window crashes — adopt after-append / after-upsert, three-way after-merge / after-delete — each with a mixed delta (new id, modified id, removed id) now roll the whole merge back; the existing complete-Phase-B tests still roll forward. * fix(engine): scope merge index docs to fast-forward; record append probe after write Two PR-review fixes: - docs(merge): the "a merge does not build indexes inline" note only holds for the fast-forward / adopt path (deferred to the reconciler). The three-way `Merged` path still rebuilds indexes inline in its publish, so a Merged-outcome merge of an embedding table pays the build up front. Scope the doc so a Merged-outcome user isn't surprised or led to skip a post-merge optimize. - `stage_append` recorded its instrumentation probe before the fallible `execute_uncommitted`, so a failed staging write left the call/row counters inflated — and diverged from `stage_append_stream`, which records after the transaction is built. Record after the write succeeds. * fix(engine): record stage_merge_insert / vector-index probes after write too The prior commit moved `stage_append`'s instrumentation probe to after the write, but left the two sibling write primitives with the identical ordering bug: `stage_merge_insert` recorded before `execute_uncommitted`, and `create_vector_index` before the index build. A failed write on either would inflate the probe counter. Move both to record only after the write succeeds, so all write-primitive probes share one rule (record-after-success) — closing the class rather than the single instance the review flagged. * docs(engine): mark the fragment-adopt excision boundary in the merge code Comment the transitional row-level merge code so a future fragment-adopt implementation (Lance branch-merge/rebase #7263 + UUID branch paths #7185) knows exactly what it deletes and what it keeps: - `AdoptDelta` / `compute_adopt_delta` / `publish_adopted_delta` — the row-level re-derivation; removed wholesale when a fast-forward merge becomes a fragment graft (adopt the source table version's fragments + indexes by reference). - `stage_append_stream` — its only caller is that merge append; dead with it unless re-adopted as a general bulk-append path. - `confirm_sidecar_phase_b` — the boundary marker: this SURVIVES. The recovery sidecar is the cross-table WAL a fast-forward-main commit still needs; only the within-table multi-commit reason for `IncompletePhaseB` narrows once each table is a single graft commit. Keep the sidecar; only simplify the classifier. Comments only; no behavior change. * test(engine): pre-upgrade v1 branch-merge sidecar must roll forward (RED) Phase-B confirmation made the recovery classifier strict for every BranchMerge sidecar — including ones written by a binary that predates confirmation. A pre-upgrade crash in the Phase-B→C gap can leave such a sidecar over a COMPLETED merge; the new classifier reads its absent confirmed_version as a partial and rolls it back, silently discarding the finished merge (greptile P1 / Cursor High). This regression test synthesizes that sidecar realistically: crash after Phase B (real sidecar + advanced Lance HEAD), downgrade the on-disk JSON to the pre-confirmation v1 shape (schema_version=1, strip confirmed_version), reopen. RED: the merge rolls back, `bob` is discarded (left ["alice"], want ["alice","bob"]). The versioning fix lands next. * fix(engine): version the recovery sidecar; read pre-confirmation merges as loose Phase-B confirmation changed how a BranchMerge sidecar's absent confirmed_version is interpreted (roll forward → roll back) without versioning the artifact, so the new classifier silently discarded completed pre-upgrade merges (greptile P1 / Cursor High). A capability flag would not fix the symmetric direction — keeping schema_version=1, an OLD binary reading a NEW sidecar sails through its already-shipped strict gate, ignores the unknown flag, and applies loose semantics to a new partial → the same data loss on downgrade. Use the versioning system instead. - Bump SIDECAR_SCHEMA_VERSION 1 → 2; add a fixed CONFIRMATION_SCHEMA_VERSION = 2 (the generation at which confirmation shipped — pinned, so a later v3 keeps v2 confirmation-aware). - Make the read gate version-aware (`parse_sidecar`): refuse only versions NEWER than this binary; accept and interpret older ones with their original semantics — no operator toil draining pre-upgrade sidecars. Rename `SidecarSchemaError.supported_version` → `max_supported_version` and reword. - Dispatch classification by version: the strict BranchMerge confirmation path is gated on `schema_version >= CONFIRMATION_SCHEMA_VERSION`; a v1 BranchMerge sidecar falls through to the existing loose roll-forward. Thread `sidecar.schema_version` from `process_sidecar`. This is bidirectionally safe: a new binary interprets v1 (loose) and v2 (strict) and refuses the future; an old binary's `!= 1` gate already refuses v2, so it never misreads a new sidecar. The flag was an additive-field pattern misapplied to a semantics change; versioning is the correct mechanism. Honest residual (any approach): an old *partial* sidecar still rolls forward — v1 carries no confirmation, so partialness is undetectable in it. The fix stops us from interpreting old sidecars under new rules; it can't retrofit information they never had. * fix(engine): harden recovery — mode resolver, loud divergence check, publish classified version Three correct-by-design fixes from the holistic review of the recovery path, all in recovery.rs (each closes a class, not an instance): A. Resolve the classification mode from `(kind, schema_version)` once, instead of a kind×version match accreting fall-through guards in `classify_table`. New `ClassificationMode { Strict, Loose, Confirmed }` + an exhaustive `SidecarKind::classification_mode` — adding a writer kind or version floor is now one arm in the resolver (the compiler forces it), not a guard threaded through the classifier. No behavior change; existing classify/decide tests are the guard. B. `confirm_sidecar_phase_b` now errors loudly when a pinned table has no achieved version in the publish `updates`, instead of silently skipping it (which left the pin unconfirmed → `IncompletePhaseB` → a silent rollback of a COMPLETE merge). Guards the implicit `pins ⊆ updates` invariant against a future divergence between the two filters (invariants 9/13). + a unit test. C. Recovery roll-forward publishes the version classification OBSERVED (`state.lance_head`), not a fresh HEAD re-read at publish time. For a Confirmed pin classify already validated `lance_head == confirmed_version`, so this publishes the recorded WAL intent by construction and closes the classify→publish re-derivation/TOCTOU for every writer (invariant 15). `push_table_update_at_head` → `push_table_update(target_version: Option<u64>)`: roll-forward pins the classified version; roll-back keeps `None` (publishes the restore commit it just made). In-scope behavior is preserved, so the existing roll-forward integration tests are the guard; the drift-hardening is correct-by-construction (deterministic mid-sweep drift injection isn't feasible — a sync failpoint can't do an async Lance write).	2026-06-19 00:15:06 +02:00
aaltshuler	f2c512ae26	chore: remove CODEOWNERS chassis and the code-owner review gate ... The repo is a 2-person team where both maintainers own every path, so the CODEOWNERS machinery (generated CODEOWNERS, roles yml, render script, the two drift/hand-edit CI jobs) gated nothing real while adding friction: every PR showed "Review required" and own-PRs merged only via admin/bypass override. Remove the whole chassis and drop the review gate: - delete .github/CODEOWNERS, codeowners-roles.yml, render-codeowners.py, the CODEOWNERS workflow, and docs/dev/codeowners.md - branch-protection.json: drop the two CODEOWNERS required status checks, set require_code_owner_reviews=false and required_approving_review_count=0 (CI checks are the gate; maintainers merge their own PRs once green) - scrub CODEOWNERS references from AGENTS.md, docs indexes, branch-protection and ci docs, GOVERNANCE.md, and CONTRIBUTING.md The policy change is inert until an admin runs scripts/apply-branch-protection.sh. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-18 02:55:27 +03:00
Andrew Altshuler	a9a5453520	Merge pull request #278 from ModernRelay/codex/compiler-error-rename ... Rename compiler error and fix cluster config warnings	2026-06-18 02:17:29 +03:00
aaltshuler	f118b67402	address compiler error review comments	2026-06-18 00:02:21 +03:00
aaltshuler	4590c91f9d	rename compiler NanoError and fix cluster config warnings	2026-06-17 23:44:24 +03:00
Ragnor Comerford	5243c048aa	perf(engine): remove the per-query metadata re-derivation tax on warm reads (#268) ... * test(engine): add read-path IO instrumentation seam for warm-read cost tests Prerequisite seam for the query-latency fixes. Adds crates/omnigraph/src/instrumentation.rs: - CountingStorageAdapter: a StorageAdapter decorator counting per-method reads (read_text/exists/read_text_versioned/list_dir), for the schema-contract reads on the query path. - A per-query task-local (QueryIoProbes) carrying Lance WrappingObjectStore wrappers per open category plus a probe counter, delivered via with_query_io_probes. open_dataset_tracked attaches the wrapper so the open itself is counted (ObjectStoreParams.object_store_wrapper). Wires the wrappers into the manifest open (open_manifest_dataset) and the commit-graph opens (CommitGraph::open/open_at_branch). Production leaves the task-local unset, so nothing attaches. Makes Omnigraph::open_with_storage public so tests can inject the counting adapter. lance-io is a dev-dependency (IOTracker named only in tests). No runtime behavior change. * test(engine): warm same-branch read should reuse the coordinator (red) Cost-budget test using Lance IOTracker at the object-store boundary (the LanceDB IO-counted-test pattern). On a 20-commit-deep graph, a warm same-branch query re-opens a fresh coordinator, which opens both the commit graph and __manifest. Asserts the read opens the commit graph zero times and performs exactly one cheap version probe; today it does neither (it scans the commit graph on re-open and never probes). The freshness guard already passes. Adds the commit_many helper for history-depth fixtures. Red half of the Fix 1 red->green pair; turns green with the next commit. * perf(engine): same-branch reads reuse the warm coordinator (Fix 1) query()/resolved_target re-opened a fresh GraphCoordinator from storage on every read (full __manifest scan + two commit-graph scans), so a warm read's cost grew with commit history (invariant 15) though the data was unchanged. resolved_target now serves same-branch reads from the warm in-memory coordinator, gated by a cheap version probe (latest_version_id, one object-store op) instead of a full re-open: - fresh (probe == cached version): return the in-memory snapshot under the read lock, with a synthetic (branch, version) id and no commit-graph access (reads pin the snapshot by manifest version, not the commit DAG; invariant 2). - stale: take the write lock, re-probe (double-checked; tokio RwLock has no read->write upgrade), then refresh_manifest_only (no commit-graph scan), preserving strong consistency for external writers (invariant 6). Cross-branch and snapshot targets keep the existing cold-resolve path. Adds ManifestCoordinator/GraphCoordinator::probe_latest_version and GraphCoordinator::refresh_manifest_only. Nothing on the read path needs a real commit ULID (only RuntimeCache keys on the id, where synthetic is consistent), per a caller audit. A warm same-branch read on a 20-commit graph now does zero commit-graph opens and exactly one probe (down from a deep commit-graph scan) and still observes external commits. The residual per-table __manifest scans are removed later by Fix 2. * test(engine): warm query should validate the schema contract once (red) ensure_schema_state_valid runs twice per query (query()/run_query_at AND resolved_target/snapshot_at_version), each reading 3 contract files + 2 existence probes. A warm query thus does 6 read_text + 4 exists where one validation (3 + 2) suffices, measured via CountingStorageAdapter. Adds a drift guard (schema_source_drift_is_caught_on_read) that already passes. Red half of the finding-A red->green pair. * perf(engine): validate the schema contract once per query (finding A) ensure_schema_state_valid ran on every query AND again inside resolved_target / snapshot_at_version, so each query validated the schema contract twice (~10 storage ops). Removes the redundant query()/ run_query_at() calls; the validation inside resolved_target / snapshot_at_version still runs, so drift is detected exactly as before. A source-only fast path was rejected: a long-lived handle must detect external drift of the schema source, IR, OR state on its next operation (lifecycle::long_lived_handle_rejects_schema_*), which a source-only compare would miss. So the only safe latency win is not validating twice. A warm query now does one validation (3 read_text + 2 exists) instead of two (6 + 4). * test(engine): warm + multi-table reads should do zero manifest scans (red) After Fix 1 a warm same-branch read still scans __manifest ~44 times at 20-commit depth: not from resolution (Fix 1 removed that) but from the per-table open path, which routes through the Lance namespace and full-scans __manifest twice per touched table (describe_table + describe_table_version). Tightens the warm test to assert manifest read_iops == 0 and adds a multi-table (traversal) test asserting the same, pinning the "2 tables = 2x" tax. Red half of the Fix 2 red->green pair. * perf(engine): open touched tables by location+version, not via the namespace (Fix 2) SubTableEntry::open routed every read-path table open through DatasetBuilder::from_namespace(BranchManifestNamespace), whose describe_table full-scans __manifest and, with managed_versioning, makes Lance scan again (describe_table_version) -- two full __manifest scans per touched table. That was the residual that made warm-read manifest IO grow with history and the '2 tables = 2x' multi-table tax. The resolved Snapshot already holds each table's path/version/branch, so open directly: from_uri(table_uri_for_path(root, path, branch)).with_version(v). The branch-qualified location is the dataset that physically holds the version (main: {path}; branch: {path}/tree/{branch}, Lance native-branch storage), and with_version resolves it within THAT dataset's _versions. 0 namespace calls + 1 HEAD via the native ConditionalPutCommitHandler. The read namespace (BranchManifestNamespace) is now unused in production (writes use StagedTableNamespace), so it, its constructor, and the helpers only it used (to_namespace_version, publish_requests, their imports) are gated #[cfg(test)] -- retained to validate the namespace contract in unit tests. Removes the dead open_table_at_version_from_manifest. Warm same-branch + multi-table reads now scan __manifest zero times; branch + time-travel reads stay correct (branching.rs, point_in_time.rs, 2 lib regression tests); production-lib warnings unchanged (baseline). * test(engine): cost-budget coverage for branch-warm and stale-refresh reads (matrix) Extends the read-path cost-budget tests across more of the morphological matrix: - warm_branch_read_does_no_manifest_scans: a warm read on a non-main branch (handle synced to it) scans __manifest zero times, exercising Fix 2's branch-owned-table open (tree/{branch} + with_version) on Fix 1's warm path -- the cell that regressed when the open used with_branch against the base. - stale_read_refreshes_manifest_only: an external commit makes the next read take the stale path, which re-reads the manifest (read_iops > 0) but never scans the commit graph (refresh_manifest_only), pinning Fix 1's manifest-only refresh. Cold paths (cross-branch, time-travel) stay behavior-covered (branching.rs, point_in_time.rs) and are cold by design (Fix 1 warm-paths only same-branch), so there is no manifest==0 contract to assert there. * test(engine): same-branch write after external commit must not fork the commit DAG (red) * fix(engine): refresh commit-graph head before append to prevent same-branch DAG fork A same-branch write that follows an external commit committed a fresh manifest version (commit_all rebases the pin from a fresh coordinator) but appended off the coordinator's stale in-memory commit-graph head, forking the commit DAG (the new commit and the external commit shared a parent). Pre-existing for non-strict inserts; widened to strict ops by Fix 1's refresh_manifest_only freshening the read-time pin. record_graph_commit now refreshes the commit-graph head from storage before append_commit, so the parent is the true current head. record_merge_commit is unaffected (it passes explicit parents). * perf(engine): hold open Dataset handles + share one Session per graph (Fix 3) A warm same-branch read still re-opened every touched table per query (the "never warms up" residual after Fix 1+2). A per-graph held-handle cache keyed by (table_path, branch, version) now serves repeat reads with zero table opens, and one shared lance::Session per graph warms metadata/index caches across opens. Validated against LanceDB upstream (rust/lancedb/src/table/dataset.rs DatasetConsistencyWrapper): hold an Arc<Dataset> and reuse it for 0-IO warm reads; one Session per connection threaded into opens; writers never serve from the read cache; time-travel bypasses. One adaptation: omnigraph keys by version (snapshot-pins-version model) where LanceDB keys per-table+HEAD, reusing the in-repo GraphIndexCache LRU template. - ReadCaches (session + TableHandleCache) injected onto live-Branch-read snapshots in resolved_target; Snapshot::open serves from the cache or opens once with the session on a miss (via the instrumented open_table_dataset). - Writes (resolved_branch_target -> open HEAD) and time-travel / Snapshot-id reads bypass the cache. Version-in-key makes a write a new key (old handle ages out via LRU); invalidate_all at branch-switch/refresh is hygiene only. - Cost tests: a 2nd identical warm read does 0 table opens; a write re-opens only the changed table at its new version. Full engine suite green. * test(engine): forbid raw data opens in the read/exec layer (P2 guard) Extend the forbidden-API guard with Dataset::open / DatasetBuilder::from_uri / from_namespace so the read/exec layer (exec/, loader/, changes/, db/omnigraph/) cannot bypass Snapshot::open and the held-handle cache (Fix 3). The instrumented opener (instrumentation.rs) is allow-listed; two legitimate non-read opens (a test editing __manifest, Hard-drop version GC) carry sentinels. The storage/manifest layers stay allow-listed. Lean P2 scope, per LanceDB-upstream + minimize-liability: the data-read boundary already exists (SubTableEntry::open); this guard pins it so a future read cannot open around the cache. Centralizing all internal opens behind one opener is deferred. * docs(dev): invariant 15 (one source of truth, cheaply derived) + cost-budget testing Records the principle behind the query-latency work: Lance and the manifest are the source of truth, everything else a derived view held warm and refreshed by a cheap probe; the two failure modes (a drifting parallel copy, and cold re-derivation whose cost grows with history) are deny-listed. Adds the cost-budget testing discipline (assert a warm read's open/IO count is flat at commit-history depth, the LanceDB IO-counted pattern) and the warm_read_cost.rs row. Updates the read-path-re-derivation known gap to reflect what Fix 1/2/3 + finding A close, and adds the commit-graph-parent-under-concurrency gap. * fix(engine): branch-incarnation identity + unified invalidation + shared LruMap (PR #268 review) Phase 6 A-D, correct-by-design responses to the Codex/Greptile P2 review comments. A: warm-read freshness and the table-handle cache key use the manifest incarnation (e_tag, manifest-timestamp fallback, then version), so a deleted+recreated non-main branch reusing a version number cannot be served stale; main stays version-cheap, non-main loads latest_manifest; a detected stale refresh also invalidates read caches; two regression tests force the version collision. B: unify the two cache invalidations into Omnigraph::invalidate_read_caches() at the four sites. C: assert the stale path's probe count. D: shared LruMap behind both caches with unconditional eviction, plus a unit test. Full engine suite green; multi-process lineage fork and O(history) write refresh remain known gaps for Phase 6E/7.	2026-06-17 13:25:20 +02:00
Andrew Altshuler	d0e06a6ff6	docs: audit pass — drop pre-0.7.0 release notes; scrub RFC refs from user docs (#272) ... * docs: audit pass — drop pre-0.7.0 release notes; scrub RFC refs from user docs - Delete the pre-0.7.0 release-notes archive (v0.2.0 … v0.6.2); keep v0.7.0. - Rewrite every inline "RFC-0NN" citation in docs/user/** into durable plain language (the behavior is the contract, not the planning doc): cli/index.md, cli/reference.md, clusters/index.md, operations/{maintenance, policy,server}.md. Updated the in-page "Scopes & profiles" anchor to match the de-RFC'd heading. No sub-0.7.0 version caveats or stale Lance-version refs were present in docs/user/**. Dev docs, AGENTS.md, and instruction files are out of scope for this pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: second alignment pass — drop residual pre-cluster-only framing - cli/reference.md: rewrite the server-scope graph-resolution rule — an omnigraph-server is always cluster-backed, so GET /graphs always answers and --graph is required; the bare-URL path is only the fallback for an unavailable/non-omnigraph endpoint (was "a single-graph / flat server … uses its bare URL as before"). - embeddings.md: "Direct single-graph serving" → "Direct (--store) access" (there is no single-graph serving mode under cluster-only). - clusters/{config,index}.md: drop the removed --target flag from the "--cluster cannot combine with …" clauses. Verified: no Linear tickets, no RFC refs, no single-graph-as-current, no --target-as-combinable in docs/user/**. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-17 02:58:47 +03:00
Andrew Altshuler	b6131393b7	docs(readme): drop em-dashes, Cursor→Codex, rename agent section (#274) ... * docs(readme): drop em-dashes, Cursor→Codex, rename agent section - Replace all 20 em-dashes with context-appropriate punctuation (colons, semicolons, parens, commas) — removes the AI-slop tell. - Cursor → Codex (the agent-host examples and the MCP host list). - "Drive it with an AI agent" → "Set it up with an AI agent". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(readme): wordmark header + simplify query examples - Add the compact wordmark header (light/dark SVG, subtitle, nav row, restyled badges) from the header-redesign work; bring the wordmark assets with it. - Rewrite the Query and mutate examples to lead with the short, config-default form (no repeated --server/--graph) and aliases — showing how simple it is, not crazy-long lines. The verbose --server/--graph/--store form is demoted to a one-line "ad-hoc target" note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-17 02:36:14 +03:00
Andrew Altshuler	b55ca02131	docs(readme): one sentence per line in the intro (#271) ... Adjacent source lines collapse into run-on text when rendered. Add hard line breaks so the headline, subhead, and each intro sentence land on their own line. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-17 00:58:28 +03:00
Andrew Altshuler	ccd13eca7c	docs(readme): make Key capabilities a table (#270) ... Match the "What you can build" section's scannable two-column format. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-17 00:45:28 +03:00
Andrew Altshuler	8cb127d8fa	docs(readme): server-first rewrite — deploy, agents, on-prem RustFS (#269) ... * docs(readme): server-first rewrite — deploy, agents, on-prem RustFS Reframe the README around the actual product: a self-hostable, multigraph graph server for context assembly and multi-agent coordination, deployed Terraform-style on your own object storage (on-prem via RustFS, or S3/R2/GCS). - Lead with key capabilities and what you can build, not a local toy. - Promote "Drive it with an AI agent" (skill + a docs-first setup prompt) above the manual deploy walkthrough — agents are the primary operator. - "Deploy" is the hero: cluster.yaml → object store → validate/plan/apply → omnigraph-server, with RustFS as the on-prem path front and center. - "Query and mutate": stored queries by name + branch/review/merge. - Security & governance as scannable bullets; Clients & SDKs as a table. - Embedded local graph demoted to a clearly-labeled "quick test" (Signal → Indicates → Pattern), explicitly dev/experiment-only. - Drop the "serve/served/serving" vocabulary tic in favor of deploy/run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(readme): add the server boot command to Deploy §3 (Greptile P1) The "Converge and run" step showed only the converge half — the code block ended at `cluster apply` with no `omnigraph-server` command, leaving a linear reader without a way to actually start the server. Add the boot line. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(readme): simplify the server boot command Drop the inline OMNIGRAPH_SERVER_BEARER_TOKEN prefix from the Deploy hero — the example cluster declares a policy so the server boots without it, and bearer auth is covered in Security & governance. Leaves a single clean line. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(readme): boot the server from the cluster dir, not a raw s3:// URI Pointing --cluster at the bucket hardcodes the storage URI in the run command. Boot from the config directory instead; the storage URI lives once in cluster.yaml and the server resolves it — single source of truth, and consistent with the cluster apply commands above. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-17 00:41:10 +03:00
Andrew Altshuler	e510937a7e	docs(readme): remove CI status badge (#267) ... The CI badge read "failing" largely from concurrency-cancelled runs (rapid doc pushes cancel in-flight runs → cancelled conclusion) and a now-fixed S3-job break, not from a broken default branch. It's been more noise than signal; drop it. CI status is still visible on the Actions tab. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 14:47:35 +03:00
Ragnor Comerford	a68896c4a0	docs(readme): embedded quick-start run-through + trimmed Clients (#266) ... * docs(readme): add embedded quick-start run-through + trim Clients - Quick start: a copy-pasteable embedded (local file) run-through (schema → init → load → query → branch), followed by a "Storage backends" table that surfaces the one thing that changes across embedded / S3 object storage / RustFS-MinIO — the graph address — with a pointer to cluster mode for served multi-graph deployments. - Clients: collapse to a two-line pointer (npm packages + omnigraph-ts repo); Python SDK marked coming soon. * docs(readme,quickstart): fix init addressing + drop non-parsing schema commas Addresses PR review (both verified against source): - README "Storage backends": `init` takes the graph address as a positional argument, not `--store` (`Command::Init { uri: String }`) — Codex. Table now shows bare addresses and a note on which flag each verb uses. - docs/user/quickstart.md: drop trailing commas in the schema. The .pg grammar (`prop_decl = { ident ~ ":" ~ type_ref ~ annotation* }`, node body `(prop_decl | body_constraint)*`, comma not in WHITESPACE) has no comma rule, so `name: String,` fails to parse — Greptile.	2026-06-16 12:36:11 +02:00
Ragnor Comerford	ee4986e9a1	docs: onboarding-first README + in-repo agent skill + drop RustFS script (#257) ... * docs: optimize README for dev onboarding; fix 0.7.0 staleness The README's setup half drifted from the shipped 0.7.0 CLI and led with the heaviest path (Docker + RustFS). This reworks it for fast, correct onboarding: README.md - New zero-dependency "Your first graph in 60 seconds" hero: a fully copy-pasteable local file-backed loop (schema → init → load → query → branch). - Add a correct "Serve it" section (cluster apply + omnigraph-server --cluster); the server is cluster-only on main, so the old positional-URI boot is gone. - Demote the RustFS bootstrap to "rehearse the S3 path locally"; reframe the storage bullet as "filesystem or any S3-compatible store (AWS S3, R2, MinIO, RustFS)" — RustFS is a provider, not a storage class. - Fix crate/MCP descriptions (query/mutate/load, not read/change/ingest). docs/user/quickstart.md - Fix the query example: `read --name <q> … <uri>` is removed — the query name is positional and the graph is addressed with `--store` (`omnigraph query find_people --query queries.gq --store graph.omni`). scripts/local-rustfs-bootstrap.sh - Convert to cluster mode: write a cluster.yaml (storage: s3://…), then validate → import → apply, load the fixture into the derived root with the now-required --mode, and serve with `omnigraph-server --cluster`. The old flow (`load` without --mode, `omnigraph-server <URI>` positional boot) no longer works on a cluster-only server. * docs: move agent skill into the repo, add agent-setup snippet, drop rustfs script skills/omnigraph - The operational skill (formerly `omnigraph-best-practices` in the cookbooks repo) now lives with the engine it documents, co-versioned. Renamed to `omnigraph`; repository metadata repointed here. - Broadened the description to trigger on intent — storing/retrieving/querying knowledge, agent memory, building a knowledge graph, operating Omnigraph — as well as on CLI/artifact sightings (stays ≤1024 chars). - Install: `npx skills add ModernRelay/omnigraph@omnigraph`. README - New "Set it up with an AI agent" paste snippet: installs the skill, reads the docs (URL), browses the cookbooks, and asks the user about a use case before standing up a first graph. - "Agent skill & starter graphs" section points at skills/omnigraph + cookbooks. Drop scripts/local-rustfs-bootstrap.sh - Not CI-tested (so it rotted: it broke on the cluster-only migration — positional server boot, load without --mode), demoed the now-optional S3 path, and was the most fragile artifact in the repo. Replaced with a "Testing against S3 locally" guide in deployment.md (docker run RustFS/MinIO + AWS_* env + cluster-on-S3). README/AGENTS references updated.	2026-06-16 11:48:13 +02:00
Andrew Altshuler	05cb73eda6	test(cli): pass --yes in the S3 e2e overwrite load (RFC-011 Decision 9) (#265) ... The RustFS S3 integration job was red on `local_cli_s3_end_to_end_init_load_read_flow`: the test runs `load --mode overwrite` against an `s3://` target, which the RFC-011 Decision 9 destructive-write guard now refuses without `--yes` ("refusing destructive `load --mode overwrite` against non-local target …"). The guard is intended and already covered in cli_data.rs; this test slipped through because it only runs in the bucket-gated RustFS CI job, not the default local gate. Add `--yes` to match the established pattern used by the other overwrite-against-non-local tests in this file (lines ~1305, ~1331). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 12:24:14 +03:00
Andrew Altshuler	1493ea4ce6	docs(readme): widen --server to <name|url>; show --as on cluster apply (#264) ... Two accuracy nits from the #263 bot review, against the cookbook's authoritative addressing reference: `--server` accepts a name OR a literal `http(s)://` URL (prose had narrowed it to `<name>`), and `cluster apply` should show `--as <actor>` so the control-plane action's attribution is explicit. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 11:43:45 +03:00
Andrew Altshuler	df0b0eadd1	docs(readme): make Common Commands cluster-first, not store-first (#263) ... Follow-up to #262: the command examples led with single-file `--store ./graph.omni` for everything (init/load/query/mutate/branch), which reads as a single-graph-file product — the opposite of the cluster-first paradigm. Reframe so the everyday loop is the headline: declare a cluster → `cluster apply` → `omnigraph-server --cluster …` → work against the served graph with `--server <name> --graph <id>`, invoking stored queries by name. `--store` is demoted to a clearly-secondary "Local / ad-hoc" note for standalone-graph iteration. Folds the former separate "Serving" section into step 1. All served examples verified to resolve correctly (`→ http://…/graphs/<id> (served)`). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 11:40:11 +03:00
Andrew Altshuler	a83da2ccfd	docs(readme): align with cluster-first paradigm + RFC-011 CLI ergonomics (#262) ... * docs(readme): align with the cluster-first paradigm + RFC-011 CLI ergonomics The README's command examples and crate list predated 0.7.0. Update them: - Common Commands: capability/addressing model — positional/`--store` for direct storage, `--server` for served graphs (no positional `http(s)://`), `query`/ `mutate` invoke a stored query by name (positional is the query name), `load` requires `--mode`. Drop the false "same URI works for local/s3/http" claim and the deprecated `read`/`change` + `--name` forms; mention `~/.omnigraph/config.yaml`. - New "Serving (cluster-first)" section: a deployment is a cluster.yaml converged with `cluster apply` and served by `omnigraph-server --cluster <dir|s3://…>` (no single-graph mode; config-free boot from a bucket). - Fix the stale docs link (`docs/user/cli.md` → `cli/index.md` + the CLI reference) after the docs were restructured into topic sections. - Workspace Crates: list all seven (add omnigraph-policy, omnigraph-api-types, omnigraph-cluster) with cluster-first framing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(agents): fix the stale `read --name` quick-reference example AGENTS.md (always-loaded; symlinked to CLAUDE.md) still showed the deprecated `omnigraph read --query … --name … <s3-uri>` form. Update it to the RFC-011 shape: `omnigraph query --query … <name> … --store <uri>` (read→query, --name→ positional query name, positional URI→--store). Addresses the bot-review finding on #262. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 11:18:16 +03:00
Andrew Altshuler	e539aa1693	ci(release): single-writer release publish — fix the matrix finalize race (#261) ... The release matrix ran three jobs (linux/macos/windows) that each called `softprops/action-gh-release` to create-or-update the SAME release. Concurrent "Finalizing release" calls exhausted the action's retries, so whole platforms' assets were dropped (on v0.7.0: linux won; macOS and Windows failed and their binaries never attached). Split build from publish: - the matrix now only uploads workflow artifacts (`actions/upload-artifact`); - a single `publish_release` job downloads them all and makes one `action-gh-release` call — the sole writer, so no race. Also add a `tag` workflow_dispatch input (resolved as `inputs.tag || github.ref_name`) so a tag can be re-published without re-cutting it — used to finish v0.7.0. `update_homebrew_tap` and `smoke_windows_installer` now depend on `publish_release` and use the resolved tag (not `GITHUB_REF_NAME`, which is the branch on a dispatch). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 11:08:20 +03:00
Andrew Altshuler	7540c86fa0	ci(publish): add omnigraph-api-types + omnigraph-cluster to the crates.io publish order (#260) ... The publish-crates workflow's list predated two workspace crates: the shared wire-DTO crate `omnigraph-api-types` (RFC-009) and `omnigraph-cluster`. On the v0.7.0 tag it published compiler/policy/engine, then failed on `omnigraph-server` ("no matching package named `omnigraph-api-types`") because that dependency was never published. Insert both in dependency order (after omnigraph-engine, before omnigraph-server). The workflow is idempotent (per-crate version check), so a re-dispatch for v0.7.0 skips the three already-published crates and finishes api-types → cluster → server → cli. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 05:23:55 +03:00
Andrew Altshuler	80ef9964fe	docs(releases): finalize v0.7.0 notes (#259) ... Reconcile the v0.7.0 release notes with what 0.7.0 actually ships. The draft was mid-cycle; two facts changed and a late-cycle arc was missing. - omnigraph.yaml is REMOVED (not deprecated): drop the deprecation-window framing (config migrate, OMNIGRAPH_NO_LEGACY_CONFIG, OMNIGRAPH_SUPPRESS_YAML_ DEPRECATION); the two-surface config (cluster.yaml + ~/.omnigraph/config.yaml) is the only config. - Cluster-only server: the server boots only from --cluster; no single-graph flat-route / positional-URI / omnigraph.yaml-graphs boot. Deprecated-route Link headers are the sibling-relative form (<load>, not </load>). - Add the RFC-011 tail: defaults.store, profile list/show, schema-apply refusal (CLI signpost + server 409), read-only aliases, the any/served/direct/control/ local capability vocabulary, removed legacy data-plane addressing. - New "Engine & substrate" section: Lance 6→7, indexed traversal, scalar-index/ query-latency, index-materialization-deferred, recovery liveness, branch-fork self-heal, composite @unique. - New "Embeddings (RFC-012)" section + breaking bullet: provider-independent client (OpenRouter default), @embed same-space validation, the default-provider flip (OMNIGRAPH_EMBED_PROVIDER=gemini for Gemini-direct; OMNIGRAPH_GEMINI_BASE_URL dropped). - Upgrade notes: replace the false "omnigraph.yaml keeps working / config migrate" guidance with the manual cluster.yaml + operator-config path; add server --cluster and embeddings notes. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 05:04:49 +03:00
Andrew Altshuler	51852fdbed	Merge pull request #248 from ModernRelay/ragnorc/shaping-config-integration ... RFC-012: embedding provider independence + remove dead client	2026-06-16 04:38:50 +03:00
aaltshuler	e33041c8b7	Fix aws server startup config test	2026-06-16 04:23:07 +03:00
aaltshuler	4f8c71fa23	Merge remote-tracking branch 'origin/main' into ragnorc/shaping-config-integration ... # Conflicts: # crates/omnigraph-cluster/src/lib.rs # crates/omnigraph-cluster/src/serve.rs # crates/omnigraph-server/src/lib.rs # crates/omnigraph-server/src/settings.rs # docs/user/clusters/config.md	2026-06-16 04:13:00 +03:00
aaltshuler	16e4a833c0	Wire cluster embedding providers	2026-06-16 04:02:08 +03:00
Andrew Altshuler	b5658dc696	[codex] fix RFC-011 follow-up regressions (#258) ... * fix rfc-011 follow-up regressions * test(cli): remove served schema-apply tests obsoleted by the cluster 409 This PR disables server-side schema apply for cluster-backed serving (409 → `omnigraph cluster apply`). Two system_local tests still drove *served* schema apply against a spawned `--cluster` server and asserted the pre-409 behavior, so they failed under `cargo test --workspace`: - `local_cli_schema_apply_enforces_engine_layer_policy` — expected a per-actor policy `denied`/allow on the served route; the route now 409s for everyone before policy runs. - `local_cli_schema_apply_rejects_stored_query_breakage_before_publish` — expected a served apply to reject a stored-query breakage; the route now 409s before any apply. Both exercise a path the PR intentionally removed. Their surviving coverage: the 409 itself is pinned by `schema_routes::schema_apply_route_refuses_cluster_backed_server_mode` (asserts 409 + no mutation); stored-query-breakage-before-publish stays covered by `schema_routes::schema_apply_route_rejects_stored_query_breakage_before_publish` (single-mode); engine-layer schema_apply Cedar enforcement stays covered by `policy_engine_chassis`. Remove the obsolete served versions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(server): report the cluster-backed schema-apply 409 after the Cedar gate The 409 ("schema apply is disabled for cluster-backed serving") fired at the top of `server_schema_apply`, before `authorize_request`. An authenticated-but- unauthorized actor therefore learned the server is cluster-backed (409) instead of getting a normal 403 — leaking topology before authorization, against the same posture that keeps `GET /graphs` default-deny. Move the 409 below the Cedar gate so the route reports 401 → 403 → 409: an unauthorized actor gets 403, and only an actor authorized for `schema_apply` sees the actionable "use `omnigraph cluster apply`" 409. (An open/unauthenticated server still 409s, as it has no topology to protect.) Regression: `schema_apply_route_cluster_backed_denies_unauthorized_actor_before_409` (POLICY_YAML grants no schema_apply → act-ragnor gets 403, not 409). Addresses the bot-review finding on #258. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 03:11:43 +03:00
aaltshuler	8d2128438e	fix(cli): quote @embed annotation values in schema show so they round-trip ... `render_annotations` emitted `@embed` values unquoted — `@embed(title, model=openai/text-embedding-3-large)`. The parser stores values via `decode_string_literal` (quotes stripped) and `annotation_kwarg` requires a quoted `literal`, so the rendered output did not re-parse: a `model` containing `/` or `-` is not a valid bare token. `schema show` therefore produced schema text that `schema apply`/lint would reject. Re-quote the positional value and every kwarg value as string literals, so `schema show` reproduces `@embed("title", model="openai/text-embedding-3-large")` and round-trips. Regression: `render_annotations_quotes_values_so_embed_round_trips` parses the rendered form back through the schema grammar. Addresses the bot-review finding on #248. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 01:25:27 +03:00
aaltshuler	1498ed821e	fix(embedding): from_parts honors an explicit model for the mock provider ... `EmbeddingConfig::from_parts(Some("mock"), _, Some(model), _)` short-circuited to `Self::mock()` and silently dropped the provided `model`, falling back to `OMNIGRAPH_EMBED_MODEL`. A cluster `embeddings` profile that sets `provider: mock, model: X` (RFC-012 Phase 5) would therefore not resolve to X — the query-time same-space check (`exec/query.rs`) compares the recorded model against the resolved one, so a dropped model makes that check fire on the wrong value (a spurious mismatch, or env-dependent). Mock vectors are model-independent so this is not silent cross-space ranking, but it breaks the contract `from_parts` documents ("model defaults exactly as from_env does") and the cluster path that Phase 5 wiring will feed it. Honor an explicit `model` for mock; fall back to `mock()`'s env-based model only when none is supplied. Regression: `from_parts_mock_honors_an_explicit_model` (red before this change). Addresses the bot-review finding on #248. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-16 01:14:01 +03:00
Andrew Altshuler	9513b076d2	docs(cli): fix the --help capability legend (config removed, profile added) (#256) ... The `local` line of the `--help` "COMMANDS BY CAPABILITY" legend was stale: it still listed `config` (the `config migrate` group was removed with the omnigraph.yaml excision) and omitted `profile` (added by RFC-011 D8). Update the list to `embed, login, logout, profile, version`. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-15 23:44:23 +03:00
Andrew Altshuler	7c092d3206	feat(cli): add read-only profile list / profile show (RFC-011 D8) (#255) ... Inspect the per-operator `~/.omnigraph/config.yaml` scope profiles without running anything: - `profile list [--json]` — every profile with its binding (server/cluster/store) and default graph; marks the `$OMNIGRAPH_PROFILE`-active one. A malformed (zero/two-scope) profile is reported as `invalid: <reason>`, not a hard failure. - `profile show [<name>] [--json]` — one profile's resolved scope: binding kind + target, the resolved endpoint (a server's URL / a cluster's root / the store URI), default graph, and output format. With no name, shows the active (`$OMNIGRAPH_PROFILE`) profile, else the flat operator defaults. Both are `local` (Session plane) — they read operator config only, take no addressing flags. Display reads `OperatorProfile::binding()` + the same `servers`/`clusters` lookups the scope resolver uses (not `resolve_scope`, which is capability-gated and can't render all three binding kinds at once), so it is honest about what a profile binds. Also: RFC-011 bookkeeping (Status → Accepted; D8 shipped, D11 gated on RFC #219, D5 deferred) and drop the stale "legacy config actor (RFC-008 window)" comment in operator.rs (the legacy actor is gone). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-15 23:33:01 +03:00