diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index e937724..d4ecfa5 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -8,9 +8,9 @@ # CI fails if this file drifts from its source, and rejects PRs that # edit this file directly without also editing the yml. -* @ragnorc @aaltshuler +* @ragnorc -crates/** @ragnorc @aaltshuler +crates/** @ragnorc docs/** @ragnorc README.md @ragnorc AGENTS.md @ragnorc diff --git a/.github/DISCUSSION_TEMPLATE/rfc.yml b/.github/DISCUSSION_TEMPLATE/rfc.yml deleted file mode 100644 index 2a63525..0000000 --- a/.github/DISCUSSION_TEMPLATE/rfc.yml +++ /dev/null @@ -1,34 +0,0 @@ -labels: ["rfc"] -body: - - type: markdown - attributes: - value: | - Use this to **incubate an RFC** β€” socialize a design and reach rough - consensus before writing the formal document. When it's ready, graduate - it into a pull request that adds `docs/rfcs/NNNN-title.md` - (see [docs/rfcs/README.md](../blob/main/docs/rfcs/README.md)); a - maintainer merging that PR is acceptance. - - For a plain feature request or open-ended idea, use the **Ideas** - category instead. For bugs, open an [Issue](../../issues/new/choose). - - type: textarea - id: problem - attributes: - label: Problem / motivation - description: What needs solving, and why is it worth the long-run cost? - validations: - required: true - - type: textarea - id: sketch - attributes: - label: Proposed direction (sketch) - description: A rough shape of the design. Detail comes later in the RFC document. - validations: - required: true - - type: textarea - id: invariants - attributes: - label: Invariants touched - description: Which items in docs/dev/invariants.md does this affect or risk? Any deny-list brush? - validations: - required: false diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml deleted file mode 100644 index 8e19465..0000000 --- a/.github/ISSUE_TEMPLATE/bug_report.yml +++ /dev/null @@ -1,55 +0,0 @@ -name: Bug report -description: Report a reproducible problem or wrong behavior in OmniGraph. -title: "bug: " -labels: ["bug", "needs-triage"] -body: - - type: markdown - attributes: - value: | - Issues are for **reporting problems** β€” concrete, reproducible bugs. - For ideas, feature requests, or questions, please use - [Discussions](../../discussions) instead. - For a security vulnerability, follow [SECURITY.md](../../blob/main/SECURITY.md) β€” do **not** file it here. - - A maintainer will triage this; once labelled **`accepted`** it's open for a pull request - (see [GOVERNANCE.md](../../blob/main/GOVERNANCE.md)). - - type: textarea - id: what-happened - attributes: - label: What happened - description: What went wrong, and what you expected instead. - validations: - required: true - - type: textarea - id: repro - attributes: - label: Steps to reproduce - description: Minimal steps, commands, schema/query, or a failing snippet. - placeholder: | - 1. omnigraph init ... - 2. omnigraph ... - 3. observed: ... / expected: ... - validations: - required: true - - type: input - id: version - attributes: - label: Version - description: Output of `omnigraph --version` (or the engine/crate version) and how you installed it. - validations: - required: true - - type: input - id: environment - attributes: - label: Environment - description: OS, architecture, and storage backend (local FS / S3 / RustFS / MinIO). - validations: - required: false - - type: textarea - id: logs - attributes: - label: Logs / output - description: Relevant error text or logs. Will be rendered as code. - render: shell - validations: - required: false diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml deleted file mode 100644 index 50720b8..0000000 --- a/.github/ISSUE_TEMPLATE/config.yml +++ /dev/null @@ -1,13 +0,0 @@ -# Issues are for problem reports only. Disable blank issues so everything is -# routed: bugs through the form, everything else to Discussions / SECURITY.md. -blank_issues_enabled: false -contact_links: - - name: πŸ’‘ Idea, feature request, or RFC - url: https://github.com/ModernRelay/omnigraph/discussions - about: Propose features and designs in Discussions. RFCs graduate from there into a docs/rfcs/ pull request. - - name: ❓ Question or help - url: https://github.com/ModernRelay/omnigraph/discussions - about: Ask in Discussions β€” questions are not tracked as Issues. - - name: πŸ”’ Security vulnerability - url: https://github.com/ModernRelay/omnigraph/blob/main/SECURITY.md - about: Report security issues privately per SECURITY.md β€” never as a public Issue. diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md deleted file mode 100644 index 2a548c7..0000000 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ /dev/null @@ -1,29 +0,0 @@ - - -## What & why - - - -## Backing issue / RFC - - - -- [ ] Fixes an **accepted** issue: Closes # -- [ ] Implements / is an **accepted** RFC: -- [ ] **Trivial fast-lane** (typo / docs / dependency bump / comment / one-line CI) β€” no issue/RFC required - -## Checklist - -- [ ] Change is focused (one logical change) -- [ ] Tests added/updated for behavior changes (or N/A) -- [ ] Public docs updated if user-facing surface changed (or N/A) -- [ ] Reviewed against [docs/dev/invariants.md](../blob/main/docs/dev/invariants.md) β€” no Hard Invariant weakened, no deny-list item hit (or justified) - -## Notes for reviewers - - diff --git a/.github/branch-protection.json b/.github/branch-protection.json index c039e32..61b7d33 100644 --- a/.github/branch-protection.json +++ b/.github/branch-protection.json @@ -1,5 +1,5 @@ { - "_comment": "Branch protection policy for main. Applied via scripts/apply-branch-protection.sh. See docs/branch-protection.md for rationale. NOTE: bypass_pull_request_allowances.users must mirror the engineering owners in .github/codeowners-roles.yml β€” code owners merge their own PRs without a second review; non-owners still need a code-owner approval. (render-codeowners.py does NOT generate this list; keep it in sync by hand.)", + "_comment": "Branch protection policy for main. Applied via scripts/apply-branch-protection.sh. See docs/branch-protection.md for rationale.", "required_status_checks": { "strict": true, "contexts": [ @@ -7,8 +7,8 @@ "Check AGENTS.md Links", "Test Workspace", "Test omnigraph-server --features aws", - "CODEOWNERS matches source", - "CODEOWNERS not hand-edited" + "CODEOWNERS / drift", + "CODEOWNERS / noedit" ] }, "enforce_admins": false, @@ -17,12 +17,7 @@ "dismiss_stale_reviews": true, "require_code_owner_reviews": true, "required_approving_review_count": 1, - "require_last_push_approval": false, - "bypass_pull_request_allowances": { - "users": ["ragnorc", "aaltshuler"], - "teams": [], - "apps": [] - } + "require_last_push_approval": false }, "restrictions": null, "required_linear_history": true, diff --git a/.github/codeowners-roles.yml b/.github/codeowners-roles.yml index ce4014d..c5e36a9 100644 --- a/.github/codeowners-roles.yml +++ b/.github/codeowners-roles.yml @@ -22,7 +22,6 @@ roles: compiler. members: - ragnorc - - aaltshuler docs: description: > diff --git a/.github/scripts/render-codeowners.py b/.github/scripts/render-codeowners.py index 5e96545..f243d0c 100755 --- a/.github/scripts/render-codeowners.py +++ b/.github/scripts/render-codeowners.py @@ -1,14 +1,10 @@ #!/usr/bin/env python3 -"""Render .github/CODEOWNERS and the ownership tables in -docs/dev/codeowners.md from .github/codeowners-roles.yml. +"""Render .github/CODEOWNERS from .github/codeowners-roles.yml. -The yml is the source of truth. This script expands the role-based yml -into (1) the flat pathβ†’owners format GitHub expects in -`.github/CODEOWNERS`, and (2) the "who owns what" markdown tables spliced -between the generated-region markers in `docs/dev/codeowners.md`. Both are -derived artifacts; CI re-renders them on every PR (see -.github/workflows/codeowners.yml) and auto-commits the result on same-repo -PRs, so the source of truth and the human-readable view never drift. +The yml is the source of truth β€” editing CODEOWNERS directly is +rejected by CI (see .github/workflows/codeowners.yml). This script +expands the role-based yml into the flat pathβ†’owners format GitHub +expects. Usage: python3 .github/scripts/render-codeowners.py @@ -20,7 +16,6 @@ Exits non-zero on: one owner; otherwise CODEOWNERS would assign nobody and GitHub would silently fall back to "no required reviewer", which defeats the purpose). - - Missing generated-region markers in docs/dev/codeowners.md. """ from __future__ import annotations @@ -39,13 +34,6 @@ except ImportError: REPO_ROOT = Path(__file__).resolve().parents[2] SOURCE = REPO_ROOT / ".github" / "codeowners-roles.yml" OUTPUT = REPO_ROOT / ".github" / "CODEOWNERS" -DOCS = REPO_ROOT / "docs" / "dev" / "codeowners.md" - -# The "who owns what" tables in docs/dev/codeowners.md are spliced between -# these markers so the human-readable view never drifts from the source of -# truth. Edit codeowners-roles.yml and re-render β€” never the table by hand. -DOCS_BEGIN = "" -DOCS_END = "" BANNER = """\ # AUTOGENERATED from .github/codeowners-roles.yml. Do not edit by hand. @@ -87,62 +75,6 @@ def owners_for(role_names: list[str], roles: dict) -> list[str]: return seen -def _oneline(text: str) -> str: - """Collapse a folded/multi-line YAML description into one cell of text.""" - return " ".join((text or "").split()) - - -def ownership_tables(spec: dict, roles: dict) -> str: - """Render the human-readable "who owns what" markdown β€” a pathβ†’owners - table (the operative view at PR time, in last-match-wins order with the - catch-all first) plus a roleβ†’members table. Spliced into the docs between - the markers so it is always current with the source of truth.""" - out: list[str] = [] - - out.append("**Path β†’ owners** (GitHub applies *last match wins*; the `*` " - "catch-all is listed first and is overridden by the specific " - "patterns below it):") - out.append("") - out.append("| Path | Owners | Role(s) |") - out.append("|---|---|---|") - if "default" in spec: - owners = " ".join(owners_for(spec["default"], roles)) - out.append(f"| `*` | {owners} | {', '.join(spec['default'])} |") - for pattern, role_names in (spec.get("paths") or {}).items(): - owners = " ".join(owners_for(role_names, roles)) - out.append(f"| `{pattern}` | {owners} | {', '.join(role_names)} |") - out.append("") - - out.append("**Roles**:") - out.append("") - out.append("| Role | Members | Description |") - out.append("|---|---|---|") - for name, role in roles.items(): - members = " ".join(f"@{m}" for m in (role.get("members") or [])) - out.append(f"| `{name}` | {members} | {_oneline(role.get('description', ''))} |") - out.append("") - - return "\n".join(out) - - -def splice_docs(table_md: str) -> None: - """Replace the region between DOCS_BEGIN/DOCS_END in the docs file with the - freshly generated tables, leaving surrounding prose untouched.""" - if not DOCS.exists(): - sys.exit(f"error: docs file not found: {DOCS}") - text = DOCS.read_text() - if DOCS_BEGIN not in text or DOCS_END not in text: - sys.exit( - f"error: ownership markers not found in {DOCS.relative_to(REPO_ROOT)}. " - f"Add the lines:\n {DOCS_BEGIN}\n {DOCS_END}\n" - f"around the generated table region." - ) - head, rest = text.split(DOCS_BEGIN, 1) - _, tail = rest.split(DOCS_END, 1) - new = f"{head}{DOCS_BEGIN}\n\n{table_md}\n{DOCS_END}{tail}" - DOCS.write_text(new) - - def main() -> int: if not SOURCE.exists(): sys.exit(f"error: source file not found: {SOURCE}") @@ -195,9 +127,6 @@ def main() -> int: OUTPUT.write_text(rendered) print(f"wrote {OUTPUT.relative_to(REPO_ROOT)}") - - splice_docs(ownership_tables(spec, roles)) - print(f"updated {DOCS.relative_to(REPO_ROOT)}") return 0 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bbe5893..5b7b7b2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -261,6 +261,63 @@ jobs: if: needs.classify_changes.outputs.run_full_ci == 'true' run: cargo test --locked -p omnigraph-server --features aws + test_windows_binaries: + name: Test Windows release binaries + needs: classify_changes + runs-on: windows-latest + timeout-minutes: 75 + permissions: + contents: read + env: + CARGO_TERM_COLOR: always + steps: + - name: Skip for text-only changes + if: needs.classify_changes.outputs.run_full_ci != 'true' + run: Write-Host "Text-only change detected; skipping Windows binary build." + + - name: Checkout source + if: needs.classify_changes.outputs.run_full_ci == 'true' + uses: actions/checkout@v5.0.1 + + - name: Install system dependencies + if: needs.classify_changes.outputs.run_full_ci == 'true' + run: choco install protoc -y + + - name: Install Rust stable + if: needs.classify_changes.outputs.run_full_ci == 'true' + uses: dtolnay/rust-toolchain@stable + with: + toolchain: stable + + - name: Cache Rust build data + if: needs.classify_changes.outputs.run_full_ci == 'true' + uses: Swatinem/rust-cache@v2 + with: + workspaces: | + . -> target + key: windows-release-binaries + + - name: Build Windows binaries + if: needs.classify_changes.outputs.run_full_ci == 'true' + run: cargo build --release --locked -p omnigraph-cli -p omnigraph-server + + - name: Smoke test Windows binaries + if: needs.classify_changes.outputs.run_full_ci == 'true' + run: | + & ./target/release/omnigraph.exe version + & ./target/release/omnigraph-server.exe --help + + - name: Check PowerShell installer syntax + if: needs.classify_changes.outputs.run_full_ci == 'true' + run: | + $tokens = $null + $errors = $null + [System.Management.Automation.Language.Parser]::ParseFile("scripts/install.ps1", [ref]$tokens, [ref]$errors) | Out-Null + if ($errors.Count -gt 0) { + $errors | Format-List + exit 1 + } + rustfs_integration: name: RustFS S3 Integration needs: diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners.yml index 75b3515..19d5835 100644 --- a/.github/workflows/codeowners.yml +++ b/.github/workflows/codeowners.yml @@ -1,24 +1,19 @@ name: CODEOWNERS -# Runs on EVERY pull request (no paths filter). The two jobs below are -# required status checks on `main`; a path-filtered required check never -# reports for PRs outside the filter and leaves them permanently "pending" -# (the trap that forced admin-override merges). Always-run + cheap -# short-circuit is what keeps them honest. on: pull_request: + paths: + - '.github/codeowners-roles.yml' + - '.github/CODEOWNERS' + - '.github/scripts/render-codeowners.py' + - '.github/workflows/codeowners.yml' workflow_dispatch: -# `drift` auto-commits the regenerated artifacts back to same-repo PR -# branches, so it needs write access. +# Read-only; we never push from this workflow. permissions: - contents: write + contents: read jobs: - # NOTE: the job `name:` values below ("CODEOWNERS matches source" / - # "CODEOWNERS not hand-edited") ARE the status-check contexts that - # .github/branch-protection.json must list verbatim. Renaming a job here - # is a branch-protection change β€” update the JSON and re-apply. drift: name: CODEOWNERS matches source runs-on: ubuntu-latest @@ -33,56 +28,19 @@ jobs: - name: Install PyYAML run: pip install pyyaml - - name: Re-render CODEOWNERS + ownership docs + - name: Re-render CODEOWNERS run: python3 .github/scripts/render-codeowners.py - # Same-repo PR: push the regenerated artifacts back so contributors - # never have to run the script locally. Mirrors the openapi.json - # auto-commit in ci.yml (separate shallow clone of the head branch so - # the pushed commit carries only the regenerated files). - - name: Commit regenerated artifacts to PR branch - if: | - github.event_name == 'pull_request' && - github.event.pull_request.head.repo.full_name == github.repository - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Reject drift run: | - if git diff --quiet -- .github/CODEOWNERS docs/dev/codeowners.md; then - echo "CODEOWNERS and ownership docs already in sync." - exit 0 - fi - tmp=$(mktemp -d) - git clone --depth 1 --branch "${{ github.head_ref }}" \ - "https://x-access-token:${GITHUB_TOKEN}@github.com/${{ github.repository }}.git" \ - "$tmp" - cp .github/CODEOWNERS "$tmp/.github/CODEOWNERS" - cp docs/dev/codeowners.md "$tmp/docs/dev/codeowners.md" - cd "$tmp" - if git diff --quiet -- .github/CODEOWNERS docs/dev/codeowners.md; then - echo "Head branch already matches; nothing to push." - exit 0 - fi - git config user.name "github-actions[bot]" - git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - git add .github/CODEOWNERS docs/dev/codeowners.md - git commit -m "chore: regenerate CODEOWNERS + ownership docs" - git push - - # Fork PR / workflow_dispatch: cannot push back, so enforce drift - # strictly. The contributor runs the script and commits the result. - - name: Verify in sync (forks / manual runs) - if: | - !(github.event_name == 'pull_request' && - github.event.pull_request.head.repo.full_name == github.repository) - run: | - if ! git diff --quiet -- .github/CODEOWNERS docs/dev/codeowners.md; then - echo "::error::Generated CODEOWNERS / ownership docs are out of sync with .github/codeowners-roles.yml." - echo "::error::Run \`python3 .github/scripts/render-codeowners.py\` and commit the result." + if ! git diff --quiet .github/CODEOWNERS; then + echo "::error::.github/CODEOWNERS is out of sync with .github/codeowners-roles.yml." + echo "::error::Run \`python3 .github/scripts/render-codeowners.py\` locally and commit the result." echo "--- diff ---" - git --no-pager diff -- .github/CODEOWNERS docs/dev/codeowners.md + git --no-pager diff .github/CODEOWNERS exit 1 fi - echo "Generated artifacts are in sync with their source." + echo "CODEOWNERS is in sync with its source." noedit: name: CODEOWNERS not hand-edited @@ -94,8 +52,6 @@ jobs: fetch-depth: 0 - name: Reject hand-edits to generated file - # Only meaningful for PRs (needs a base to diff against). - if: github.event_name == 'pull_request' run: | base="origin/${{ github.base_ref }}" git fetch origin "${{ github.base_ref }}" --quiet diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a265c40..3a66ff2 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -121,30 +121,16 @@ jobs: run: | ./scripts/update-homebrew-formula.sh "${GITHUB_REF_NAME}" homebrew-tap/Formula/omnigraph.rb - # Diagnostic only: brew is not on PATH on the ubuntu runner by default, so - # set it up explicitly. Both this setup and the audit below are best-effort - # canaries, not gates β€” continue-on-error on each keeps a failed/flaky brew - # (the action is pinned to a moving @master ref) from skipping the actual - # tap publish below. The formula is correct by construction - # (update-homebrew-formula.sh), so brew tooling must never block the push. - - name: Set up Homebrew - if: env.HOMEBREW_TAP_SKIP != '1' - continue-on-error: true - uses: Homebrew/actions/setup-homebrew@master - - name: Audit generated formula if: env.HOMEBREW_TAP_SKIP != '1' - continue-on-error: true run: | # Audit the checked-out tap by name (brew audit rejects bare paths # and needs tap context). Symlink the checkout into Homebrew's Taps - # tree so `modernrelay/tap/omnigraph` resolves to it. Offline audit - # (no --online) keeps it deterministic; it still catches the - # ComponentsOrder/structure class of problems. + # tree so `modernrelay/tap/omnigraph` resolves to it. tap_dir="$(brew --repository)/Library/Taps/modernrelay/homebrew-tap" mkdir -p "$(dirname "$tap_dir")" ln -sfn "$PWD/homebrew-tap" "$tap_dir" - brew audit --strict modernrelay/tap/omnigraph + brew audit --strict --online modernrelay/tap/omnigraph - name: Commit and push formula update if: env.HOMEBREW_TAP_SKIP != '1' diff --git a/AGENTS.md b/AGENTS.md index 3f5b711..b876749 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -236,8 +236,8 @@ omnigraph policy explain --actor act-alice --action change --branch main | Columnar storage on object store | βœ… Arrow/Lance | URI normalization, S3 env-var plumbing | | Per-dataset versioning + time travel | βœ… | `snapshot_at_version`, `entity_at`, snapshot-pinned reads across many tables | | Per-dataset branches | βœ… | **Graph-level** branches (atomic across all sub-tables), lazy fork, system branch filtering | -| Atomic single-dataset commits | βœ… | **Multi-table publish via three layers**, NOT a single Lance primitive: (1) per-table Lance `commit_staged` for the data write, (2) `__manifest` row-level CAS via `ManifestBatchPublisher` for cross-table ordering, (3) the open-time recovery sweep for the residual gap between (1) and (2). All three layers ship; the five migrated writers (`MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`, `optimize_all_tables`) write a `__recovery/{ulid}.json` sidecar before Phase B and delete it after Phase C. The next `Omnigraph::open` (gated on `OpenMode::ReadWrite`) runs the sweep in `db/manifest/recovery.rs`: classify, decide all-or-nothing per sidecar, roll forward via single `ManifestBatchPublisher::publish` or roll back via `Dataset::restore` followed by a manifest publish of the restored version (so both directions converge to `manifest == HEAD` β€” no residual drift), and record an audit row in `_graph_commit_recoveries.lance` (queryable via `omnigraph commit list --filter actor=omnigraph:recovery`). Continuous in-process recovery (no restart needed between Phase B failure and recovery) is the goal of a future background reconciler. Engine writes route through a sealed `TableStorage` trait exposing `stage_*` + `commit_staged` as the canonical staged-write surface; documented inline-commit residuals (`delete_where`, `create_vector_index`, plus legacy `append_batch` / `merge_insert_batches` / `overwrite_batch` / `create_*_index`) remain on the trait until upstream Lance ships a public two-phase API ([#6658](https://github.com/lance-format/lance/issues/6658), [#6666](https://github.com/lance-format/lance/issues/6666)) and the migration of every call site completes. | -| Compaction (`compact_files`) | βœ… | `omnigraph optimize` orchestrates over all node/edge tables, bounded concurrency; **publishes each compacted table's new version to `__manifest`** (so the manifest tracks the Lance HEAD β€” required for reads to observe compaction and for schema apply / strict writes to pass their HEAD-vs-manifest precondition), under the per-`(table, main)` write queue with `SidecarKind::Optimize` recovery coverage; **refuses on an unrecovered graph** (errors if a `__recovery` sidecar is pending β€” recovery may roll back a partial write, so optimize requires `manifest == HEAD` going in); **skips blob-bearing tables** (reported via `TableOptimizeStats.skipped`, not silent), gated on `LANCE_SUPPORTS_BLOB_COMPACTION` until the upstream blob-v2 compaction-decode bug is fixed (see [docs/dev/invariants.md](docs/dev/invariants.md) Known Gaps) | +| Atomic single-dataset commits | βœ… | **Multi-table publish via three layers**, NOT a single Lance primitive: (1) per-table Lance `commit_staged` for the data write, (2) `__manifest` row-level CAS via `ManifestBatchPublisher` for cross-table ordering, (3) the open-time recovery sweep for the residual gap between (1) and (2). All three layers ship; the four migrated writers (`MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`) write a `__recovery/{ulid}.json` sidecar before Phase B and delete it after Phase C. The next `Omnigraph::open` (gated on `OpenMode::ReadWrite`) runs the sweep in `db/manifest/recovery.rs`: classify, decide all-or-nothing per sidecar, roll forward via single `ManifestBatchPublisher::publish` or roll back via `Dataset::restore`, and record an audit row in `_graph_commit_recoveries.lance` (queryable via `omnigraph commit list --filter actor=omnigraph:recovery`). Continuous in-process recovery (no restart needed between Phase B failure and recovery) is the goal of a future background reconciler. Engine writes route through a sealed `TableStorage` trait exposing `stage_*` + `commit_staged` as the canonical staged-write surface; documented inline-commit residuals (`delete_where`, `create_vector_index`, plus legacy `append_batch` / `merge_insert_batches` / `overwrite_batch` / `create_*_index`) remain on the trait until upstream Lance ships a public two-phase API ([#6658](https://github.com/lance-format/lance/issues/6658), [#6666](https://github.com/lance-format/lance/issues/6666)) and the migration of every call site completes. | +| Compaction (`compact_files`) | βœ… | `omnigraph optimize` orchestrates over all node/edge tables, bounded concurrency; **skips blob-bearing tables** (reported via `TableOptimizeStats.skipped`, not silent), gated on `LANCE_SUPPORTS_BLOB_COMPACTION` until the upstream blob-v2 compaction-decode bug is fixed (see [docs/dev/invariants.md](docs/dev/invariants.md) Known Gaps) | | Cleanup (`cleanup_old_versions`) | βœ… | `omnigraph cleanup` with `--keep` / `--older-than` policy | | BTREE / inverted (FTS) / vector indexes | βœ… | `ensure_indices` builds them on every relevant column; idempotent; lazy across branches | | `merge_insert` upsert | βœ… | `LoadMode::Merge`, mutation `update`/`insert`/`delete` lowering | diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2d77ef0..8d9c687 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,29 +1,10 @@ # Contributing -Thanks for your interest in OmniGraph. This page is the practical how-to; the -rules and decision authority behind it live in [GOVERNANCE.md](GOVERNANCE.md). +Small bug fixes and documentation improvements are welcome directly through pull +requests. -## Start in the right place - -| I want to… | Go to | Notes | -|---|---|---| -| **Report a bug** or wrong behavior | **[Open an Issue](../../issues/new/choose)** | Concrete and reproducible. A maintainer triages it; once labelled **`accepted`** it's open for a PR. | -| **Suggest a feature / share an idea / ask** | **[Start a Discussion](../../discussions)** | Ideas and questions live here, not in Issues. | -| **Propose a design / RFC** | **An RFC pull request** | Anyone can author one β€” see [docs/rfcs/README.md](docs/rfcs/README.md). A maintainer merging it is acceptance. | -| **Fix something / implement a change** | **A pull request** | Must link an `accepted` issue or an accepted RFC β€” unless it's trivial (below). | -| **Report a security vulnerability** | **[SECURITY.md](SECURITY.md)** | Do **not** open a public Issue. | - -### When can I just open a PR? -The **trivial fast-lane** β€” open directly, no prior issue/RFC needed: typo and -wording fixes, doc corrections, dependency bumps, comment fixes, obvious -one-line CI tweaks. Anything more substantial needs a backing `accepted` issue -or accepted RFC first, so the *why* is agreed before the *how* is reviewed. A PR -that turns out to be non-trivial will be redirected β€” that's about process, not -the merit of the change. - -> **Maintainers (ModernRelay team)** follow a separate internal process and are -> not bound by the intake rules above. Everyone is bound by review, CODEOWNERS, -> branch protection, and CI. +For larger changes, please open an issue or design discussion first so the +proposed direction is clear before implementation starts. ## Development @@ -68,11 +49,6 @@ CI runs both. ## Pull Requests -- **Link the backing issue or RFC** (`Closes #123`, or reference the RFC) β€” or - mark the PR as trivial per the fast-lane. -- Keep changes focused; one logical change per PR. -- Include tests for behavior changes when practical. -- Update public docs when the user-facing surface changes. - -New to the codebase? Read [AGENTS.md](AGENTS.md) β€” the architecture map and the -always-on invariants every change is reviewed against. +- keep changes focused +- include tests for behavior changes when practical +- update public docs when the user-facing surface changes diff --git a/GOVERNANCE.md b/GOVERNANCE.md deleted file mode 100644 index 5878f1f..0000000 --- a/GOVERNANCE.md +++ /dev/null @@ -1,106 +0,0 @@ -# Governance - -This document describes how **external contributions** to OmniGraph are -proposed, accepted, and merged. It exists so an outside contributor can answer, -without asking: *where does my report/idea/change go, who decides, and what has -to happen before code lands?* - -> **Scope.** This governs the public contribution surface β€” Issues, -> Discussions, RFCs, and pull requests from people outside the ModernRelay -> team. **Maintainers operate under a separate internal process** and are not -> bound by the intake gates below. Everyone, maintainer or not, is still bound -> by the universal gates: branch protection on `main` and CODEOWNERS review -> (see [docs/dev/branch-protection.md](docs/dev/branch-protection.md) and -> [docs/dev/codeowners.md](docs/dev/codeowners.md)). - -## Roles - -| Role | Who | Authority | -|---|---|---| -| **Maintainer** | The code owners in [`.github/CODEOWNERS`](.github/CODEOWNERS) (generated from [`.github/codeowners-roles.yml`](.github/codeowners-roles.yml)) | Validate issues, accept/reject RFCs, review and merge PRs, set direction. Final decision authority. | -| **Contributor** | Anyone else | Report problems (Issues), propose ideas (Discussions), author RFCs, and open pull requests. | - -Decision authority rests with the maintainers. CODEOWNERS is the single source -of truth for who that is; this document does not duplicate the list. - -## The three channels - -Each channel has one job. Using the right one is the first thing we ask of a -contribution. - -| Channel | Purpose | Not for | -|---|---|---| -| **[Issues](../../issues)** | **Report a problem** β€” a bug, a regression, a documented behavior that's wrong. Something concrete and reproducible. | Feature requests, ideas, questions, or design proposals (β†’ Discussions). | -| **[Discussions](../../discussions)** | **Propose and explore** β€” new ideas, feature requests, questions, and the incubation of RFCs. | Bug reports (β†’ Issues). | -| **Pull requests** | **Land a sanctioned change** β€” a fix for a *validated* issue, an *accepted* RFC, or a trivial change (see fast-lane). | Substantive change with no backing issue/RFC β€” it will be redirected. | - -## How a change becomes mergeable - -``` - β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€ bug ───────────┐ β”Œβ”€β”€β”€β”€β”€β”€β”€β”€ idea / feature ────────┐ - β–Ό β”‚ β–Ό β”‚ - Issue (problem report) β”‚ Discussion (idea / RFC incubation) β”‚ - β”‚ β”‚ β”‚ β”‚ - maintainer triage β”‚ rough consensus β”‚ - β”‚ β”‚ β”‚ graduate β”‚ - β–Ό β”‚ β–Ό β”‚ - label: accepted ──────────┐ β”‚ RFC PR (docs/rfcs/NNNN-*.md) β”‚ - β”‚ β”‚ β”‚ β”‚ β”‚ - β”‚ β”‚ β”‚ maintainer review β”‚ - β–Ό β–Ό β”‚ β–Ό β”‚ - Pull request ◀──────────┴──────────│── merged == accepted β”‚ - (links the issue or the accepted RFC) β—€β”€β”€β”€β”€β”€β”€β”€β”˜ (implementation PRs reference it) β”‚ - β”‚ - review + CODEOWNERS + branch protection - β–Ό - merged -``` - -### Issues β†’ validated -A new issue starts unlabeled. A maintainer triages it and, if it's a real, -in-scope problem, applies the **`accepted`** label. **Only `accepted` issues are -open for a contributor PR.** This prevents the "I fixed an issue you hadn't -agreed was a problem" rejection. Want to fix something? Get the issue accepted -first, or pick one already labelled `accepted` / `help wanted`. - -### Discussions β†’ RFCs β†’ accepted -Ideas and feature requests start in **Discussions**. Anyone β€” including external -contributors β€” may then **author an RFC** by opening a pull request that adds -`docs/rfcs/NNNN-title.md` (see [docs/rfcs/README.md](docs/rfcs/README.md)). The -RFC is reviewed as code; **a maintainer merging it is the act of acceptance** -(it becomes the durable decision record). Implementation PRs then reference the -accepted RFC. - -Authoring an RFC is open to everyone; **accepting one is a maintainer -decision.** Maintainers may also decline an RFC, with rationale, by closing it. - -### Pull requests β†’ sanctioned -A contributor PR must do one of: -1. link a maintainer-**`accepted`** issue it fixes, or -2. be (or reference) an **accepted RFC**, or -3. qualify for the **trivial fast-lane**. - -**Trivial fast-lane** β€” these may be opened directly, no prior issue/RFC: -typo and wording fixes, documentation corrections, dependency bumps, comment -fixes, and obviously-correct one-line CI tweaks. When in doubt, open an Issue or -Discussion first; a PR that turns out to be non-trivial will be asked to. - -A substantive PR with no backing issue/RFC will be closed with a pointer to the -right channel β€” not as a judgment of the idea, but to keep design discussion -where it's reviewable. - -## What maintainers do *not* gate -Maintainers' own changes do not pass through the intake gates above β€” the team -runs a separate internal process. The universal gates (review, CODEOWNERS, -branch protection, CI) apply to everyone. Enforcement of the intake rules is, to -start, **by convention and review** (PR template + labels); an automated check -keyed to author association may be added later if volume warrants. - -## Code of conduct & security -- Conduct: [CODE_OF_CONDUCT.md](CODE_OF_CONDUCT.md). -- Security issues are **not** public Issues β€” see [SECURITY.md](SECURITY.md). - -## Changing this document -Governance changes the same way code does: a pull request, reviewed by -maintainers. This file describes the external surface; the internal maintainer -process is intentionally out of scope here. diff --git a/crates/omnigraph/src/db/manifest.rs b/crates/omnigraph/src/db/manifest.rs index 5bf1f87..7fcf7de 100644 --- a/crates/omnigraph/src/db/manifest.rs +++ b/crates/omnigraph/src/db/manifest.rs @@ -36,7 +36,7 @@ use publisher::{GraphNamespacePublisher, ManifestBatchPublisher}; pub(crate) use recovery::{ RecoveryMode, RecoverySidecar, RecoverySidecarHandle, SidecarKind, SidecarTablePin, SidecarTableRegistration, SidecarTombstone, delete_sidecar, has_schema_apply_sidecar, - list_sidecars, new_sidecar, recover_manifest_drift, write_sidecar, + new_sidecar, recover_manifest_drift, write_sidecar, }; pub use state::SubTableEntry; #[cfg(test)] @@ -48,22 +48,6 @@ const OBJECT_TYPE_TABLE_VERSION: &str = "table_version"; const OBJECT_TYPE_TABLE_TOMBSTONE: &str = "table_tombstone"; const TABLE_VERSION_MANAGEMENT_KEY: &str = "table_version_management"; -/// Apply pending internal-schema migrations against `__manifest` on the -/// open-for-write path, independent of a publish. -/// -/// `Omnigraph::open(ReadWrite)` calls this before the coordinator reads branch -/// state, so branch-observing code (`branch_list`, the schema-apply -/// blocking-branch checks) sees the post-migration graph. In particular the -/// v2β†’v3 step sweeps legacy `__run__*` staging branches off `__manifest` -/// (MR-770); running it here closes the window where those branches would -/// otherwise block schema apply before the first publish runs the migration. -/// -/// Idempotent: a no-op stamp read when the on-disk version already matches. -pub(crate) async fn migrate_on_open(root_uri: &str) -> Result<()> { - let mut dataset = open_manifest_dataset(root_uri, None).await?; - migrations::migrate_internal_schema(&mut dataset).await -} - /// Immutable point-in-time view of the database. /// /// Cheap to create (no storage I/O). All reads within a query go through one diff --git a/crates/omnigraph/src/db/manifest/migrations.rs b/crates/omnigraph/src/db/manifest/migrations.rs index e2801fe..bbb7995 100644 --- a/crates/omnigraph/src/db/manifest/migrations.rs +++ b/crates/omnigraph/src/db/manifest/migrations.rs @@ -46,11 +46,7 @@ use crate::error::{OmniError, Result}; /// - v2 β€” `__manifest.object_id` carries the unenforced-PK annotation, /// engaging Lance's bloom-filter conflict resolver at commit time. Added /// alongside `expected_table_versions` OCC on `ManifestBatchPublisher::publish`. -/// - v3 β€” one-time sweep of legacy `__run__` staging branches left on the -/// `__manifest` dataset by the pre-v0.4.0 Run state machine (removed in -/// MR-771). Once swept, the `is_internal_run_branch` defense-in-depth guard -/// is no longer needed (MR-770). -pub(super) const INTERNAL_MANIFEST_SCHEMA_VERSION: u32 = 3; +pub(super) const INTERNAL_MANIFEST_SCHEMA_VERSION: u32 = 2; const INTERNAL_SCHEMA_VERSION_KEY: &str = "omnigraph:internal_schema_version"; const OBJECT_ID_PK_KEY: &str = "lance-schema:unenforced-primary-key"; @@ -93,10 +89,6 @@ pub(super) async fn migrate_internal_schema(dataset: &mut Dataset) -> Result<()> migrate_v1_to_v2(dataset).await?; current = 2; } - 2 => { - migrate_v2_to_v3(dataset).await?; - current = 3; - } other => { return Err(OmniError::manifest_internal(format!( "no internal-schema migration registered for v{} β†’ v{}", @@ -130,51 +122,6 @@ async fn migrate_v1_to_v2(dataset: &mut Dataset) -> Result<()> { set_stamp(dataset, 2).await } -/// v2 β†’ v3: sweep legacy `__run__` staging branches off the `__manifest` -/// dataset, then bump the stamp. -/// -/// The pre-v0.4.0 Run state machine (removed in MR-771) created graph-level -/// staging branches named `__run__` on `__manifest`. MR-771 stopped -/// creating them but left any pre-existing ones in place; Lance's -/// `list_branches` still enumerates them, so they leak into `branch_list()` -/// and count as blocking branches at schema-apply time. This one-time sweep -/// removes them so the `is_internal_run_branch` guard can retire (MR-770). -/// -/// The `"__run__"` prefix is inlined here on purpose: this migration must keep -/// working after the `run_registry` module (the guard) is deleted, so it does -/// not depend on it. -/// -/// Idempotent under both sequential retry and concurrent runners: each run -/// re-enumerates `list_branches` fresh, and `force_delete_branch` tolerates a -/// branch that is already gone β€” so a crash before the stamp bump, or a second -/// process opening the same legacy graph at the same time, never errors out. -async fn migrate_v2_to_v3(dataset: &mut Dataset) -> Result<()> { - const LEGACY_RUN_BRANCH_PREFIX: &str = "__run__"; - let branches = dataset - .list_branches() - .await - .map_err(|e| OmniError::Lance(e.to_string()))?; - let run_branches: Vec = branches - .into_keys() - .filter(|name| { - name.trim_start_matches('/') - .starts_with(LEGACY_RUN_BRANCH_PREFIX) - }) - .collect(); - for name in run_branches { - // `force_delete_branch` deletes even when the `BranchContents` is - // already gone. Plain `delete_branch` errors "BranchContents not - // found", which would fail a second concurrent open (or a retry that - // raced another runner) after the first one swept the branch. Force is - // exactly Lance's documented path for cleaning up zombie branches. - dataset - .force_delete_branch(&name) - .await - .map_err(|e| OmniError::Lance(e.to_string()))?; - } - set_stamp(dataset, 3).await -} - async fn set_stamp(dataset: &mut Dataset, version: u32) -> Result<()> { dataset .update_schema_metadata([(INTERNAL_SCHEMA_VERSION_KEY.to_string(), version.to_string())]) diff --git a/crates/omnigraph/src/db/manifest/recovery.rs b/crates/omnigraph/src/db/manifest/recovery.rs index 3119531..4c1b987 100644 --- a/crates/omnigraph/src/db/manifest/recovery.rs +++ b/crates/omnigraph/src/db/manifest/recovery.rs @@ -106,12 +106,6 @@ pub(crate) enum SidecarKind { BranchMerge, /// `ensure_indices_for_branch` β€” index lifecycle commits. EnsureIndices, - /// `optimize_all_tables` β€” Lance `compact_files` (reserve-fragments + - /// rewrite commits) followed by a manifest publish of the compacted - /// version. Loose-match like the other multi-commit writers; roll-forward - /// is always safe because compaction is content-preserving (Lance - /// `Operation::Rewrite` "reorganizes data without semantic modification"). - Optimize, } /// One table's contribution to a sidecar's intended commit. The classifier @@ -418,13 +412,11 @@ pub(crate) fn parse_sidecar(sidecar_uri: &str, body: &str) -> Result manifest_pinned` as `RolledPastExpected` when /// `pin.expected_version == manifest_pinned` (the writer's CAS /// target matches what the manifest currently shows). The risk this @@ -502,12 +494,9 @@ pub(crate) fn decide(classifications: &[TableClassification]) -> SidecarDecision /// Skipping the restore in those cases would leave Lance HEAD ahead of /// the manifest with no recovery artifact left. /// -/// Cost: a successful roll-back appends one restore commit and then publishes -/// the manifest to match (`roll_back_sidecar`), so the table converges -/// (`manifest == HEAD`) in one pass. Only repeated crashes *between* the restore -/// and that publish (rare) accumulate extra restore commits; each re-classified -/// roll-back restores again and `omnigraph cleanup` reclaims the surplus. -/// Bounded by the number of interrupted recovery iterations β€” typically 0. +/// Cost: under repeated mid-rollback crashes (rare), Lance HEAD +/// accumulates extra restore commits that `omnigraph cleanup` reclaims. +/// Bounded by the number of recovery iterations β€” typically 1. pub(crate) async fn restore_table_to_version( table_path: &str, branch: Option<&str>, @@ -812,24 +801,13 @@ async fn roll_back_sidecar( sidecar: &RecoverySidecar, states: &[ClassifiedTable], ) -> Result<()> { - // Restore every drifted table (RolledPastExpected / UnexpectedAtP1 / - // UnexpectedMultistep) to its manifest-pinned content, then PUBLISH so - // `manifest == Lance HEAD` for each β€” symmetric with roll-forward. The - // restore commit's content equals the manifest-pinned version, so re-pinning - // the manifest to the new (restored) HEAD is content-correct and closes the - // orphaned-drift class (`HEAD > manifest` with no covering sidecar). This is - // what makes a failed-then-retried schema_apply converge: after one - // roll-back `manifest == HEAD`, so the retry's precondition passes instead of - // failing one version higher each iteration. - // - // NoMovement tables are already at the pin β€” excluded from both the restore - // and the publish. The audit `to_version` stays the *logical* rolled-back-to - // version (`manifest_pinned`), while the manifest is published at - // `manifest_pinned + 1` (the restore commit, same content) β€” keep that - // asymmetry so the audit records the drift (`from_version > to_version`). + // Restore every table whose Lance HEAD has drifted from the + // manifest pin (RolledPastExpected, UnexpectedAtP1, + // UnexpectedMultistep). NoMovement tables are already at the + // manifest pin β€” no action. Restore is unconditional; repeated + // mid-rollback crashes accumulate a few extra Lance commits that + // `omnigraph cleanup` reclaims. let mut outcomes = Vec::with_capacity(sidecar.tables.len()); - let mut updates: Vec = Vec::with_capacity(sidecar.tables.len()); - let mut expected: HashMap = HashMap::with_capacity(sidecar.tables.len()); for (pin, state) in sidecar.tables.iter().zip(states.iter()) { if matches!( state.classification, @@ -843,20 +821,10 @@ async fn roll_back_sidecar( state.manifest_pinned, ) .await?; - // Publish the post-restore HEAD, CAS against the current (unmoved) - // manifest pin β€” the same helper roll-forward uses. - push_table_update_at_head( - root_uri, - &pin.table_key, - &pin.table_path, - pin.table_branch.as_deref(), - state.manifest_pinned, - &mut updates, - &mut expected, - ) - .await?; - // `from_version` records the Lance HEAD observed BEFORE the restore - // (the actual drift); `to_version` the logical pin we rolled back to. + // `from_version` records the Lance HEAD observed BEFORE the + // restore (the actual drift), not the manifest pin. Operators + // reading `_graph_commit_recoveries.lance` see "rolled back + // from v7 to v5" rather than "v5 β†’ v5". outcomes.push(TableOutcome { table_key: pin.table_key.clone(), from_version: state.lance_head, @@ -864,23 +832,13 @@ async fn roll_back_sidecar( }); } } - // Publish the restored HEADs so manifest == HEAD. A degenerate all-NoMovement - // roll-back restores nothing β€” there's nothing to publish, and the audit - // records the unchanged snapshot version. - let manifest_version = if updates.is_empty() { - snapshot.version() - } else { - let publisher = GraphNamespacePublisher::new(root_uri, sidecar.branch.as_deref()); - publisher - .publish(&updates, &expected) - .await? - .version() - .version - }; + // Manifest pin doesn't move on rollback; record an audit-only + // commit at the existing version so operators can correlate via + // `omnigraph commit list --filter actor=omnigraph:recovery`. record_audit( root_uri, sidecar, - manifest_version, + snapshot.version(), RecoveryKind::RolledBack, outcomes, ) @@ -961,20 +919,44 @@ async fn roll_forward_all( HashMap::with_capacity(sidecar.tables.len() + sidecar.additional_registrations.len()); for pin in &sidecar.tables { - // Publish to the table's CURRENT Lance HEAD on the pin's branch (not the - // sidecar's `post_commit_pin`, a lower bound for loose-match writers that - // run multiple commit_staged calls per table). CAS against the pin's - // pre-write `expected_version`. - let head_version = push_table_update_at_head( + // Open the dataset at its CURRENT Lance HEAD on the pin's branch + // (not at the sidecar's post_commit_pin). For strict-match writers + // (Mutation/Load) HEAD == post_commit_pin by construction. For + // loose-match writers (SchemaApply/EnsureIndices/BranchMerge) HEAD + // may be higher than post_commit_pin (multiple commit_staged + // calls per table); we want to publish to the actual current HEAD. + let head_ds = Dataset::open(&pin.table_path) + .await + .map_err(|e| OmniError::Lance(e.to_string()))?; + let head_ds = match pin.table_branch.as_deref() { + Some(b) if b != "main" => head_ds + .checkout_branch(b) + .await + .map_err(|e| OmniError::Lance(e.to_string()))?, + _ => head_ds, + }; + let head_version = head_ds.version().version; + + let row_count = head_ds + .count_rows(None) + .await + .map_err(|e| OmniError::Lance(e.to_string()))? as u64; + + let table_relative_path = super::table_path_for_table_key(&pin.table_key)?; + let version_metadata = super::metadata::TableVersionMetadata::from_dataset( root_uri, - &pin.table_key, - &pin.table_path, - pin.table_branch.as_deref(), - pin.expected_version, - &mut updates, - &mut expected, - ) - .await?; + &table_relative_path, + &head_ds, + )?; + + updates.push(ManifestChange::Update(SubTableUpdate { + table_key: pin.table_key.clone(), + table_version: head_version, + table_branch: pin.table_branch.clone(), + row_count, + version_metadata, + })); + expected.insert(pin.table_key.clone(), pin.expected_version); published_versions.insert(pin.table_key.clone(), head_version); } @@ -1065,57 +1047,6 @@ async fn roll_forward_all( Ok((new_dataset.version().version, published_versions)) } -/// Open `table_path` at its branch HEAD, read the current Lance HEAD version, -/// row count, and version metadata, and push a `ManifestChange::Update` (plus -/// its CAS `expected` entry) that re-pins the manifest to that HEAD. Returns the -/// published HEAD version. -/// -/// Shared by `roll_forward_all` (where `expected_version` is the sidecar's -/// pre-write pin) and `roll_back_sidecar` (where it is the manifest-pinned -/// version the table was just restored to). The HEAD is read AFTER any restore -/// in the same single-threaded sweep, so no concurrent writer can have advanced -/// it. -async fn push_table_update_at_head( - root_uri: &str, - table_key: &str, - table_path: &str, - branch: Option<&str>, - expected_version: u64, - updates: &mut Vec, - expected: &mut HashMap, -) -> Result { - let head_ds = Dataset::open(table_path) - .await - .map_err(|e| OmniError::Lance(e.to_string()))?; - let head_ds = match branch { - Some(b) if b != "main" => head_ds - .checkout_branch(b) - .await - .map_err(|e| OmniError::Lance(e.to_string()))?, - _ => head_ds, - }; - let head_version = head_ds.version().version; - let row_count = head_ds - .count_rows(None) - .await - .map_err(|e| OmniError::Lance(e.to_string()))? as u64; - let table_relative_path = super::table_path_for_table_key(table_key)?; - let version_metadata = super::metadata::TableVersionMetadata::from_dataset( - root_uri, - &table_relative_path, - &head_ds, - )?; - updates.push(ManifestChange::Update(SubTableUpdate { - table_key: table_key.to_string(), - table_version: head_version, - table_branch: branch.map(str::to_string), - row_count, - version_metadata, - })); - expected.insert(table_key.to_string(), expected_version); - Ok(head_version) -} - /// Append the audit row describing this recovery action. /// /// Two-part write: (a) `_graph_commits.lance` row anchored on the recovery diff --git a/crates/omnigraph/src/db/manifest/tests.rs b/crates/omnigraph/src/db/manifest/tests.rs index 885a2a8..effa0b5 100644 --- a/crates/omnigraph/src/db/manifest/tests.rs +++ b/crates/omnigraph/src/db/manifest/tests.rs @@ -1461,80 +1461,6 @@ async fn test_publish_migrates_pre_stamp_manifest_to_current_version() { assert!(reopened.snapshot().entry("node:Person").is_some()); } -#[tokio::test] -async fn test_v2_to_v3_sweeps_legacy_run_branches_on_write_open() { - let dir = tempfile::tempdir().unwrap(); - let uri = dir.path().to_str().unwrap(); - let catalog = build_test_catalog(); - let mut mc = ManifestCoordinator::init(uri, &catalog).await.unwrap(); - - // Synthesize a pre-MR-770 graph: several stale `__run__` staging branches - // left on `__manifest` (a real legacy graph accumulates one per run), plus - // a real user branch that must survive the sweep. Multiple run branches - // exercise the migration's delete loop on a single reused dataset handle. - mc.create_branch("__run__01J9LEGACY").await.unwrap(); - mc.create_branch("__run__01J9SECOND").await.unwrap(); - mc.create_branch("__run__01J9THIRD").await.unwrap(); - mc.create_branch("feature").await.unwrap(); - let before = mc.list_branches().await.unwrap(); - assert_eq!( - before.iter().filter(|b| b.starts_with("__run__")).count(), - 3, - "precondition: three legacy run branches exist on __manifest; got {before:?}", - ); - - // Rewind the internal-schema stamp to v2 so the next write-open runs the - // v2 β†’ v3 sweep arm (init stamps at the current version, which is past it). - { - let mut ds = open_manifest_dataset(uri, None).await.unwrap(); - ds.update_schema_metadata([( - "omnigraph:internal_schema_version".to_string(), - Some("2".to_string()), - )]) - .await - .unwrap(); - let post = open_manifest_dataset(uri, None).await.unwrap(); - assert_eq!(super::migrations::read_stamp(&post), 2, "stamp rewound to v2"); - } - - // A no-op publish forces the open-for-write path, which runs the migration. - let mut expected = HashMap::new(); - expected.insert("node:Person".to_string(), 1); - GraphNamespacePublisher::new(uri, None) - .publish(&[], &expected) - .await - .unwrap(); - - // Stamp advanced to current; the legacy run branch is physically gone from - // `__manifest` (checked via the raw, unfiltered manifest list β€” not the - // guard-filtered `branch_list`), and the real branch + `main` survive. - let post = open_manifest_dataset(uri, None).await.unwrap(); - assert_eq!( - super::migrations::read_stamp(&post), - super::migrations::INTERNAL_MANIFEST_SCHEMA_VERSION, - ); - let reopened = ManifestCoordinator::open(uri).await.unwrap(); - let after = reopened.list_branches().await.unwrap(); - assert!( - !after.iter().any(|b| b.starts_with("__run__")), - "legacy run branch must be swept; got {after:?}", - ); - assert!(after.iter().any(|b| b == "feature"), "user branch must survive"); - assert!(after.iter().any(|b| b == "main"), "main must survive"); - - // Idempotent: a second write-open finds the stamp at current and does not - // re-run the sweep or error. - GraphNamespacePublisher::new(uri, None) - .publish(&[], &expected) - .await - .unwrap(); - let final_ds = open_manifest_dataset(uri, None).await.unwrap(); - assert_eq!( - super::migrations::read_stamp(&final_ds), - super::migrations::INTERNAL_MANIFEST_SCHEMA_VERSION, - ); -} - #[tokio::test] async fn test_publish_rejects_manifest_stamped_at_future_version() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/omnigraph/src/db/mod.rs b/crates/omnigraph/src/db/mod.rs index 13e1c74..8702f88 100644 --- a/crates/omnigraph/src/db/mod.rs +++ b/crates/omnigraph/src/db/mod.rs @@ -3,6 +3,7 @@ pub mod graph_coordinator; pub mod manifest; mod omnigraph; mod recovery_audit; +mod run_registry; mod schema_state; pub(crate) mod write_queue; @@ -14,6 +15,7 @@ pub use omnigraph::{ CleanupPolicyOptions, InitOptions, MergeOutcome, Omnigraph, OpenMode, SchemaApplyOptions, SchemaApplyResult, SkipReason, TableCleanupStats, TableOptimizeStats, }; +pub(crate) use run_registry::is_internal_run_branch; pub(crate) const SCHEMA_APPLY_LOCK_BRANCH: &str = "__schema_apply_lock__"; @@ -67,8 +69,5 @@ pub(crate) fn is_schema_apply_lock_branch(name: &str) -> bool { } pub(crate) fn is_internal_system_branch(name: &str) -> bool { - // Legacy `__run__*` staging branches (Run state machine, removed MR-771) - // are swept off `__manifest` by the v2β†’v3 internal-schema migration, so the - // only internal branch the engine still creates is the schema-apply lock. - is_schema_apply_lock_branch(name) + is_internal_run_branch(name) || is_schema_apply_lock_branch(name) } diff --git a/crates/omnigraph/src/db/omnigraph.rs b/crates/omnigraph/src/db/omnigraph.rs index ba2b70e..7b8a3f6 100644 --- a/crates/omnigraph/src/db/omnigraph.rs +++ b/crates/omnigraph/src/db/omnigraph.rs @@ -346,16 +346,6 @@ impl Omnigraph { mode: OpenMode, ) -> Result { let root = normalize_root_uri(uri)?; - // Apply pending internal-schema migrations before the coordinator reads - // branch state, so `branch_list` and the schema-apply blocking-branch - // checks observe the post-migration graph β€” notably the v2β†’v3 sweep of - // legacy `__run__*` staging branches (MR-770). ReadWrite only: a - // read-only open must not trigger object-store writes, so a read-only - // open of an unmigrated legacy graph still lists `__run__*` until its - // first read-write open (an accepted, documented limitation). - if matches!(mode, OpenMode::ReadWrite) { - crate::db::manifest::migrate_on_open(&root).await?; - } // Open the coordinator first so the schema-staging recovery sweep can // compare its snapshot against any leftover staging files. let mut coordinator = GraphCoordinator::open(&root, Arc::clone(&storage)).await?; @@ -1501,6 +1491,12 @@ pub(crate) fn normalize_branch_name(branch: &str) -> Result> { } pub(crate) fn ensure_public_branch_ref(branch: &str, operation: &str) -> Result<()> { + if super::is_internal_run_branch(branch) { + return Err(OmniError::manifest(format!( + "{} does not allow internal run ref '{}'", + operation, branch + ))); + } if is_internal_system_branch(branch) { return Err(OmniError::manifest(format!( "{} does not allow internal system ref '{}'", @@ -1904,6 +1900,7 @@ fn json_value_from_array(array: &dyn Array, row: usize) -> Result Company #[tokio::test] async fn test_apply_schema_succeeds_after_load() { // Historical: schema apply used to be blocked by leftover - // `__run__` branches. The Run state machine was removed in - // MR-771, so a fresh graph never creates a `__run__` branch; - // legacy ones are swept by the v2β†’v3 manifest migration. This - // asserts the invariant a current graph upholds: publish leaves - // no `__run__` branch behind, so schema apply proceeds. + // `__run__` branches. A defense-in-depth filter now skips + // internal system branches, and run branches were made + // ephemeral on every terminal state β€” so in practice no + // `__run__` branch survives publish. The filter still guards + // the invariant. let dir = tempfile::tempdir().unwrap(); let uri = dir.path().to_str().unwrap(); let mut db = Omnigraph::init(uri, TEST_SCHEMA).await.unwrap(); @@ -2260,8 +2257,8 @@ edge WorksAt: Person -> Company let all_branches = db.coordinator.read().await.all_branches().await.unwrap(); assert!( - !all_branches.iter().any(|b| b.starts_with("__run__")), - "no __run__ branch should exist after publish, got: {:?}", + !all_branches.iter().any(|b| is_internal_run_branch(b)), + "run branch should be deleted after publish, got: {:?}", all_branches ); @@ -2273,56 +2270,6 @@ edge WorksAt: Person -> Company assert!(result.applied, "schema apply should have applied"); } - /// Regression (MR-770): a pre-v0.4.0 graph that still carries a stale - /// `__run__*` branch on `__manifest` must not block schema apply. The - /// v2β†’v3 sweep runs in `Omnigraph::open(ReadWrite)` β€” before the - /// schema-apply blocking-branch check β€” so apply succeeds with no - /// intervening publish. - /// - /// Confirmed to fail before the open-time migration landed: the reopened - /// graph still listed `__run__legacy`, and `apply_schema` returned - /// "found non-main branches: __run__legacy". - #[tokio::test] - async fn legacy_run_branch_is_swept_on_open_and_does_not_block_schema_apply() { - let dir = tempfile::tempdir().unwrap(); - let uri = dir.path().to_str().unwrap(); - let mut db = Omnigraph::init(uri, TEST_SCHEMA).await.unwrap(); - - // Synthesize a legacy graph: a stale `__run__` branch on `__manifest` - // plus the manifest stamp rewound to v2 (pre-sweep). - db.branch_create("__run__legacy").await.unwrap(); - drop(db); - { - let mut ds = lance::Dataset::open(&format!("{}/__manifest", uri)) - .await - .unwrap(); - ds.update_schema_metadata([( - "omnigraph:internal_schema_version".to_string(), - Some("2".to_string()), - )]) - .await - .unwrap(); - } - - // Reopen (ReadWrite): the open-time migration must sweep `__run__legacy` - // before any branch-observing code runs. - let db = Omnigraph::open(uri).await.unwrap(); - let branches = db.branch_list().await.unwrap(); - assert!( - !branches.iter().any(|b| b.starts_with("__run__")), - "open-time migration must sweep legacy __run__ branches; got {branches:?}", - ); - - // Schema apply must proceed with no intervening publish β€” the - // blocking-branch check no longer sees `__run__legacy`. - let desired = TEST_SCHEMA.replace( - " age: I32?\n}", - " age: I32?\n nickname: String?\n}", - ); - let result = db.apply_schema(&desired).await.unwrap(); - assert!(result.applied, "schema apply should have applied"); - } - #[tokio::test] async fn test_apply_schema_adds_index_for_existing_property() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/omnigraph/src/db/omnigraph/optimize.rs b/crates/omnigraph/src/db/omnigraph/optimize.rs index ee39323..fff3f54 100644 --- a/crates/omnigraph/src/db/omnigraph/optimize.rs +++ b/crates/omnigraph/src/db/omnigraph/optimize.rs @@ -8,14 +8,8 @@ //! Two dials: //! //! * `optimize_all_tables` β€” Lance `compact_files` on every table. Rewrites -//! small fragments into fewer large ones, then **publishes the compacted -//! version to the `__manifest`** so the manifest's `table_version` tracks the -//! compacted Lance HEAD (reads pin the manifest version, so without the -//! publish compaction would be invisible to readers and would break the -//! HEAD-vs-manifest precondition of schema apply / strict writes). Compaction -//! is content-preserving (Lance `Operation::Rewrite` "reorganizes data -//! without semantic modification"), so old fragments remain reachable via -//! older manifest versions until `cleanup` runs. +//! small fragments into fewer large ones. Non-destructive (creates a new +//! version; old fragments remain reachable via older manifest versions). //! * `cleanup_all_tables` β€” Lance `cleanup_old_versions` on every table. //! Removes manifests (and their unique fragments) older than the configured //! retention. Destructive to version history β€” callers should gate this @@ -29,9 +23,7 @@ use std::time::Duration; use chrono::Utc; use futures::stream::StreamExt; use lance::dataset::cleanup::{CleanupPolicy, RemovalStats}; -use lance::dataset::optimize::{ - CompactionMetrics, CompactionOptions, compact_files, plan_compaction, -}; +use lance::dataset::optimize::{CompactionMetrics, CompactionOptions, compact_files}; use super::*; @@ -119,8 +111,7 @@ pub struct TableOptimizeStats { pub fragments_removed: usize, /// Number of new, larger fragments Lance produced. pub fragments_added: usize, - /// Did this table get a new manifest version from the compaction? True when - /// compaction ran and its compacted version was published to `__manifest`. + /// Did this table get a new Lance manifest version from the compaction? pub committed: bool, /// `Some(reason)` if this table was deliberately not compacted. When set, /// `fragments_removed == 0`, `fragments_added == 0`, and `!committed`. @@ -162,29 +153,12 @@ pub struct TableCleanupStats { pub error: Option, } -/// Run Lance `compact_files` on every node + edge table on `main`, publishing -/// each compacted table's new version to the `__manifest`. Tables run in -/// parallel (bounded concurrency); each is fault-isolated only at the Lance -/// level β€” a publish error is propagated (the recovery sidecar covers it). +/// Run Lance `compact_files` on every node + edge table on `main`. +/// Tables run in parallel (bounded concurrency). pub async fn optimize_all_tables(db: &Omnigraph) -> Result> { db.ensure_schema_state_valid().await?; db.ensure_schema_apply_idle("optimize").await?; - // Refuse on an unrecovered graph. A pending recovery sidecar means a failed - // write left partial state that the open-time sweep must resolve (roll - // forward/back) first; compacting + publishing a table covered by such a - // sidecar could commit a partial write the sweep would roll back. Reopen the - // graph to run recovery, then re-run optimize. - if !crate::db::manifest::list_sidecars(db.root_uri(), db.storage_adapter()) - .await? - .is_empty() - { - return Err(OmniError::manifest_conflict( - "optimize requires a clean recovery state; reopen the graph to run the \ - recovery sweep before optimizing", - )); - } - let resolved = db.resolved_branch_target(None).await?; let snapshot = resolved.snapshot; @@ -209,179 +183,49 @@ pub async fn optimize_all_tables(db: &Omnigraph) -> Result> = futures::stream::iter(table_tasks.into_iter()) - .map(move |(table_key, full_path, has_blob)| async move { - optimize_one_table(db, table_key, full_path, has_blob).await + .map(|(table_key, full_path, has_blob)| async move { + // Lance `compact_files` mis-decodes blob-v2 columns under the forced + // `BlobHandling::AllBinary` read (see LANCE_SUPPORTS_BLOB_COMPACTION). + // Skip blob-bearing tables and report it rather than aborting the + // whole sweep β€” the other tables still compact. + if has_blob && !LANCE_SUPPORTS_BLOB_COMPACTION { + tracing::warn!( + target: "omnigraph::optimize", + table = %table_key, + "skipping compaction: table has blob columns the current Lance \ + cannot rewrite (blob-v2 AllBinary decode bug); other tables \ + unaffected β€” rerun after the Lance fix", + ); + return Ok(TableOptimizeStats::skipped( + table_key, + SkipReason::BlobColumnsUnsupportedByLance, + )); + } + let mut ds = table_store + .open_dataset_head_for_write(&table_key, &full_path, None) + .await?; + let version_before = ds.version().version; + let metrics: CompactionMetrics = + compact_files(&mut ds, CompactionOptions::default(), None) + .await + .map_err(|e| OmniError::Lance(e.to_string()))?; + let version_after = ds.version().version; + Ok(TableOptimizeStats::compacted( + table_key, + &metrics, + version_after != version_before, + )) }) .buffer_unordered(concurrency) .collect() .await; - // Invalidate caches for any table that published a compaction β€” done BEFORE - // propagating a sibling table's error, since the published versions are - // durable and reads must observe the new fragment layout (Lance invalidates - // the original row addresses on rewrite). The CSR/CSC graph topology index - // is rebuilt only when an edge table moved. Mirrors schema_apply's - // post-publish invalidation. - let any_committed = stats - .iter() - .any(|s| matches!(s, Ok(st) if st.committed)); - let edge_committed = stats - .iter() - .any(|s| matches!(s, Ok(st) if st.committed && st.table_key.starts_with("edge:"))); - if any_committed { - db.runtime_cache.invalidate_all().await; - if edge_committed { - db.invalidate_graph_index().await; - } - } - stats.into_iter().collect() } -/// Compact one table and publish the compacted version to the `__manifest`. -/// -/// Compaction (`compact_files`) advances the *dataset's* Lance HEAD via a -/// reserve-fragments + rewrite commit, but Lance knows nothing about the -/// `__manifest`. To keep the manifest the single authority for each table's -/// visible version (invariant 2), optimize must publish the compacted version. -/// The Lance-HEAD-before-manifest-publish gap is unavoidable (Lance has no -/// staged/uncommitted compaction), so it is covered by a recovery sidecar like -/// the other multi-commit writers; roll-forward is always safe because -/// compaction is content-preserving. -async fn optimize_one_table( - db: &Omnigraph, - table_key: String, - full_path: String, - has_blob: bool, -) -> Result { - // Lance `compact_files` mis-decodes blob-v2 columns under the forced - // `BlobHandling::AllBinary` read (see LANCE_SUPPORTS_BLOB_COMPACTION). Skip - // blob-bearing tables and report it rather than aborting the whole sweep. - if has_blob && !LANCE_SUPPORTS_BLOB_COMPACTION { - tracing::warn!( - target: "omnigraph::optimize", - table = %table_key, - "skipping compaction: table has blob columns the current Lance \ - cannot rewrite (blob-v2 AllBinary decode bug); other tables \ - unaffected β€” rerun after the Lance fix", - ); - return Ok(TableOptimizeStats::skipped( - table_key, - SkipReason::BlobColumnsUnsupportedByLance, - )); - } - - // Serialize the whole compactβ†’publish against concurrent mutations on this - // (table, main): compaction is a Rewrite op that retryable-conflicts with a - // concurrent Merge/Update/Delete on overlapping fragments, and an - // interleaved write would also move the manifest version out from under the - // CAS below. Holding the queue makes the CAS baseline read under it exact. - let _guard = db - .write_queue() - .acquire_many(&[(table_key.clone(), None)]) - .await; - - let mut ds = db - .table_store - .open_dataset_head_for_write(&table_key, &full_path, None) - .await?; - - // CAS baseline: the table's current manifest version, read under the queue - // (in-memory coordinator snapshot, no storage I/O β€” stable for this section). - let expected_version = db - .snapshot() - .await - .entry(&table_key) - .map(|e| e.table_version) - .ok_or_else(|| OmniError::manifest(format!("no manifest entry for {}", table_key)))?; - - // Precise "will it compact?" check β€” `plan_compaction` also accounts for - // deletion materialization (which can rewrite even a single fragment). A - // steady-state already-compacted table yields an empty plan and is never - // pinned in a sidecar (a zero-commit pin would classify NoMovement on - // recovery and force an all-or-nothing rollback). There is no drift to - // reconcile here: optimize runs only on a recovered graph (the pending- - // sidecar guard above), and recovery roll-back now publishes, so - // `HEAD == manifest` holds going in. - let options = CompactionOptions::default(); - let plan = plan_compaction(&ds, &options) - .await - .map_err(|e| OmniError::Lance(e.to_string()))?; - if plan.num_tasks() == 0 { - return Ok(TableOptimizeStats::compacted( - table_key, - &CompactionMetrics::default(), - false, - )); - } - - // Phase A: recovery sidecar BEFORE compaction advances the Lance HEAD, so a - // crash before the manifest publish rolls forward on next open. - let sidecar = crate::db::manifest::new_sidecar( - crate::db::manifest::SidecarKind::Optimize, - None, - // optimize is system-attributed (no `optimize_as` actor API today). - None, - vec![crate::db::manifest::SidecarTablePin { - table_key: table_key.clone(), - table_path: full_path.clone(), - expected_version, - // Lower bound β€” compaction commits Nβ‰₯1 versions (reserve + rewrite); - // the classifier loose-matches SidecarKind::Optimize. - post_commit_pin: expected_version + 1, - table_branch: None, - }], - ); - let handle = - crate::db::manifest::write_sidecar(db.root_uri(), db.storage_adapter(), &sidecar).await?; - - // Phase B: compaction (reserve-fragments + rewrite commits advance HEAD). - let version_before = ds.version().version; - let metrics: CompactionMetrics = compact_files(&mut ds, options, None) - .await - .map_err(|e| OmniError::Lance(e.to_string()))?; - let version_after = ds.version().version; - let committed = version_after != version_before; - - // Pin the per-writer Phase B β†’ Phase C residual for optimize: Lance HEAD has - // advanced but the manifest publish below hasn't run. - crate::failpoints::maybe_fail("optimize.post_phase_b_pre_manifest_commit")?; - - // Phase C: publish the compacted version to the manifest (one CAS commit, - // expected = the version observed under the queue). On failure the sidecar - // is intentionally left for the open-time recovery sweep to roll forward. - if committed { - let state = db.table_store.table_state(&full_path, &ds).await?; - let update = crate::db::SubTableUpdate { - table_key: table_key.clone(), - table_version: state.version, - table_branch: None, - row_count: state.row_count, - version_metadata: state.version_metadata, - }; - let mut expected = std::collections::HashMap::new(); - expected.insert(table_key.clone(), expected_version); - db.coordinator - .write() - .await - .commit_updates_with_actor_with_expected(&[update], &expected, None) - .await?; - } - - // Phase D: delete the sidecar (best-effort; recovery resolves a leftover). - if let Err(err) = crate::db::manifest::delete_sidecar(&handle, db.storage_adapter()).await { - tracing::warn!( - error = %err, - operation_id = handle.operation_id.as_str(), - "optimize recovery sidecar cleanup failed; next open's recovery sweep will resolve it" - ); - } - - Ok(TableOptimizeStats::compacted(table_key, &metrics, committed)) -} - /// Run Lance `cleanup_old_versions` on every node + edge table on `main`, /// using [`CleanupPolicyOptions`]. The latest manifest is always preserved /// regardless (Lance invariant). diff --git a/crates/omnigraph/src/db/omnigraph/schema_apply.rs b/crates/omnigraph/src/db/omnigraph/schema_apply.rs index 7cb3193..35fe161 100644 --- a/crates/omnigraph/src/db/omnigraph/schema_apply.rs +++ b/crates/omnigraph/src/db/omnigraph/schema_apply.rs @@ -61,11 +61,11 @@ async fn plan_schema_for_apply( ) -> Result { db.ensure_schema_state_valid().await?; let branches = db.coordinator.read().await.all_branches().await?; - // Skip `main` and internal system branches (the schema-apply lock branch, - // the cluster-wide schema-apply serializer). Legacy `__run__*` staging - // branches were swept off `__manifest` by the v2β†’v3 migration that runs in - // `Omnigraph::open(ReadWrite)` before this check (MR-770), so they no - // longer appear here. + // Skip `main` and internal system branches. The schema-apply lock branch + // is excluded because it is the cluster-wide schema-apply serializer. + // `__run__*` branches are no longer created; the filter remains as + // defense-in-depth for legacy graphs with leftover staging branches. + // A future production sweep will let this guard go. let blocking_branches = branches .into_iter() .filter(|branch| branch != "main" && !is_internal_system_branch(branch)) diff --git a/crates/omnigraph/src/db/run_registry.rs b/crates/omnigraph/src/db/run_registry.rs new file mode 100644 index 0000000..ee3d336 --- /dev/null +++ b/crates/omnigraph/src/db/run_registry.rs @@ -0,0 +1,16 @@ +// The Run state machine has been removed. Mutations now write directly +// to target tables and use the publisher's `expected_table_versions` +// CAS for cross-table OCC; `__run__` staging branches and the +// `_graph_runs.lance` state machine no longer exist. +// +// What remains is the branch-name predicate, kept as a defense-in-depth +// guard against users naming a public branch `__run__*`. A future +// production sweep of legacy `_graph_runs.lance` rows and stale +// `__run__*` branches will let this predicate (and this file) go too. + +pub(crate) const INTERNAL_RUN_BRANCH_PREFIX: &str = "__run__"; + +pub(crate) fn is_internal_run_branch(name: &str) -> bool { + name.trim_start_matches('/') + .starts_with(INTERNAL_RUN_BRANCH_PREFIX) +} diff --git a/crates/omnigraph/src/exec/merge.rs b/crates/omnigraph/src/exec/merge.rs index eb6c4a3..2e5f32e 100644 --- a/crates/omnigraph/src/exec/merge.rs +++ b/crates/omnigraph/src/exec/merge.rs @@ -1087,9 +1087,9 @@ impl Omnigraph { target: &str, actor_id: Option<&str>, ) -> Result { - if is_internal_system_branch(source) || is_internal_system_branch(target) { + if is_internal_run_branch(source) || is_internal_run_branch(target) { return Err(OmniError::manifest(format!( - "branch_merge does not allow internal system refs ('{}' -> '{}')", + "branch_merge does not allow internal run refs ('{}' -> '{}')", source, target ))); } diff --git a/crates/omnigraph/src/exec/mod.rs b/crates/omnigraph/src/exec/mod.rs index ce72d42..33a7e41 100644 --- a/crates/omnigraph/src/exec/mod.rs +++ b/crates/omnigraph/src/exec/mod.rs @@ -35,7 +35,7 @@ use time::format_description::well_known::Rfc3339; use crate::db::commit_graph::CommitGraph; use crate::db::manifest::ManifestCoordinator; -use crate::db::{MergeOutcome, Omnigraph, is_internal_system_branch}; +use crate::db::{MergeOutcome, Omnigraph, is_internal_run_branch}; use crate::db::{ReadTarget, Snapshot}; use crate::embedding::EmbeddingClient; use crate::error::{MergeConflict, MergeConflictKind, OmniError, Result}; diff --git a/crates/omnigraph/src/loader/mod.rs b/crates/omnigraph/src/loader/mod.rs index d5d74c0..46a46e2 100644 --- a/crates/omnigraph/src/loader/mod.rs +++ b/crates/omnigraph/src/loader/mod.rs @@ -288,24 +288,21 @@ async fn load_jsonl_reader( let mut node_rows: HashMap> = HashMap::new(); let mut edge_rows: HashMap> = HashMap::new(); - // Parse a stream of JSON values. Accepts both compact JSONL (one object - // per line) and pretty-printed JSON where a single object spans multiple - // lines β€” serde's streaming deserializer treats any whitespace (including - // newlines) between top-level values as a separator. - for (idx, parsed) in serde_json::Deserializer::from_reader(reader) - .into_iter::() - .enumerate() - { - let record_num = idx + 1; - let value: JsonValue = parsed.map_err(|e| { - OmniError::manifest(format!("invalid JSON at record {}: {}", record_num, e)) + for (line_num, line) in reader.lines().enumerate() { + let line = line?; + let line = line.trim(); + if line.is_empty() { + continue; + } + let value: JsonValue = serde_json::from_str(line).map_err(|e| { + OmniError::manifest(format!("invalid JSON on line {}: {}", line_num + 1, e)) })?; if let Some(type_name) = value.get("type").and_then(|v| v.as_str()) { if !catalog.node_types.contains_key(type_name) { return Err(OmniError::manifest(format!( - "record {}: unknown node type '{}'", - record_num, + "line {}: unknown node type '{}'", + line_num + 1, type_name ))); } @@ -320,8 +317,8 @@ async fn load_jsonl_reader( } else if let Some(edge_name) = value.get("edge").and_then(|v| v.as_str()) { if catalog.lookup_edge_by_name(edge_name).is_none() { return Err(OmniError::manifest(format!( - "record {}: unknown edge type '{}'", - record_num, + "line {}: unknown edge type '{}'", + line_num + 1, edge_name ))); } @@ -329,14 +326,14 @@ async fn load_jsonl_reader( .get("from") .and_then(|v| v.as_str()) .ok_or_else(|| { - OmniError::manifest(format!("record {}: edge missing 'from'", record_num)) + OmniError::manifest(format!("line {}: edge missing 'from'", line_num + 1)) })? .to_string(); let to = value .get("to") .and_then(|v| v.as_str()) .ok_or_else(|| { - OmniError::manifest(format!("record {}: edge missing 'to'", record_num)) + OmniError::manifest(format!("line {}: edge missing 'to'", line_num + 1)) })? .to_string(); let data = value @@ -350,8 +347,8 @@ async fn load_jsonl_reader( .push((from, to, data)); } else { return Err(OmniError::manifest(format!( - "record {}: expected 'type' or 'edge' field", - record_num + "line {}: expected 'type' or 'edge' field", + line_num + 1 ))); } } diff --git a/crates/omnigraph/tests/composite_flow.rs b/crates/omnigraph/tests/composite_flow.rs index dd41310..6c720da 100644 --- a/crates/omnigraph/tests/composite_flow.rs +++ b/crates/omnigraph/tests/composite_flow.rs @@ -294,19 +294,21 @@ async fn composite_flow_canonical_lifecycle() { ); // ───────────────────────────────────────────────────────────────── - // Step 10: optimize the post-merge graph β€” verify compaction is - // published to the manifest (so the manifest pin tracks the compacted - // Lance HEAD), indices stay valid and queryable, and a post-optimize - // strict write commits. + // Step 10: optimize the post-merge graph β€” verify indices stay + // valid and queryable. // - // This step used to carry a "Known limitation": `optimize_all_tables` - // ran Lance `compact_files` without publishing the new version to - // `__manifest`, so the manifest pin lagged the Lance HEAD and the next - // strict write / schema apply failed with `ExpectedVersionMismatch` - // ("stale view … refresh and retry") β€” so post-optimize mutations were - // deliberately omitted here. optimize now publishes the compacted - // version, and this flow exercises exactly that previously-failing - // write below. + // **Known limitation**: `optimize_all_tables` calls Lance + // `compact_files` directly β€” it advances per-table Lance HEAD + // without updating the omnigraph `__manifest` pin. After optimize, + // the next writer's expected_table_versions captures the + // pre-optimize manifest pin, but the publisher's pre-check reads + // a higher version from the manifest dataset (because some other + // path β€” possibly schema-state recovery on reopen β€” wrote a newer + // __manifest row). The `ExpectedVersionMismatch` is benign + // (re-issuing the mutation after a snapshot refresh succeeds), but + // a composite test cannot reliably exercise post-optimize mutations + // until that path is investigated. Coverage of post-optimize + // mutations is left to a focused optimize+cleanup integration test. // ───────────────────────────────────────────────────────────────── let optimize_stats = db.optimize().await.unwrap(); assert!( @@ -329,28 +331,6 @@ async fn composite_flow_canonical_lifecycle() { "row counts unchanged by optimize" ); - // A strict update on a compacted table is exactly the write that - // failed with "stale view" before optimize published its compaction. - // It must now commit (Alice is one of the seed Persons; an update - // leaves the row count at 6). - let post_optimize_update = mutate_main( - &mut db, - MUTATION_QUERIES, - "set_age", - &mixed_params(&[("$name", "Alice")], &[("$age", 41)]), - ) - .await - .expect("post-optimize strict update must commit β€” optimize published the manifest"); - assert_eq!( - post_optimize_update.affected_nodes, 1, - "post-optimize update must affect exactly Alice" - ); - assert_eq!( - count_rows(&db, "node:Person").await, - 6, - "an update must not change the Person row count" - ); - // ───────────────────────────────────────────────────────────────── // Step 11: cleanup β€” keep last 10 versions, only purge versions // older than 1 hour. With this small test, we have well under 10 @@ -393,27 +373,14 @@ async fn composite_flow_canonical_lifecycle() { branches, ); - // Final exercise β€” full read AND write path works post-reopen, - // post-cleanup. (The post-cleanup mutation was previously omitted - // pending resolution of the optimize-vs-manifest-pin interaction in - // Step 10; that is now fixed, so a strict write here must commit.) + // Final query exercise β€” full read path works post-reopen, + // post-cleanup. Post-cleanup mutation is omitted here pending + // resolution of the optimize-vs-manifest-pin interaction documented + // in Step 10. let final_total = query_main(&mut db, TEST_QUERIES, "total_people", &ParamMap::default()) .await .unwrap(); assert!(!final_total.batches().is_empty()); - - let post_reopen_update = mutate_main( - &mut db, - MUTATION_QUERIES, - "set_age", - &mixed_params(&[("$name", "Alice")], &[("$age", 42)]), - ) - .await - .expect("post-reopen, post-cleanup strict update must commit"); - assert_eq!( - post_reopen_update.affected_nodes, 1, - "post-reopen update must affect exactly Alice" - ); } /// Cross-handle sequence that exercises operations after a schema_apply diff --git a/crates/omnigraph/tests/end_to_end.rs b/crates/omnigraph/tests/end_to_end.rs index ea11d0e..a0fdb0e 100644 --- a/crates/omnigraph/tests/end_to_end.rs +++ b/crates/omnigraph/tests/end_to_end.rs @@ -1933,87 +1933,3 @@ query docs_with_tag($tag: String) { "contains-pushdown should return exactly the rows whose tags list contains 'red'" ); } - -// ─── Maintenance in the full lifecycle: optimize (compaction) ──────────────── - -/// `optimize` (Lance compaction) is part of a realistic graph lifecycle: it -/// advances the Lance HEAD and publishes the compacted version to the manifest. -/// The rest of the flow must keep working across that boundary β€” reads observe -/// the compacted data, strict updates (which check Lance HEAD == manifest -/// version) still commit, inserts still commit, and the state survives a reopen -/// (the open-time recovery sweep finds no leftover drift). Before optimize -/// published its compaction, the manifest lagged the Lance HEAD here and the -/// post-optimize update below failed with "stale view ... refresh and retry". -#[tokio::test] -async fn full_flow_optimize_then_query_update_and_reopen() { - let dir = tempfile::tempdir().unwrap(); - let uri = dir.path().to_str().unwrap().to_string(); - let mut db = init_and_load(&dir).await; - - // Build several Person fragments so compaction has something to merge. - for (name, age) in [("Eve", 40), ("Frank", 41), ("Grace", 42)] { - mutate_main( - &mut db, - MUTATION_QUERIES, - "insert_person", - &mixed_params(&[("$name", name)], &[("$age", age)]), - ) - .await - .unwrap(); - } - - let stats = db.optimize().await.unwrap(); - assert!( - stats.iter().any(|s| s.committed), - "a multi-fragment table should have compacted in this flow" - ); - - // Reads observe the compacted data. - let qr = query_main( - &mut db, - TEST_QUERIES, - "get_person", - ¶ms(&[("$name", "Alice")]), - ) - .await - .unwrap(); - assert_eq!(qr.num_rows(), 1); - - // Strict update after optimize commits (previously failed with "stale view" - // because the manifest lagged the compacted Lance HEAD). - let upd = mutate_main( - &mut db, - MUTATION_QUERIES, - "set_age", - &mixed_params(&[("$name", "Alice")], &[("$age", 31)]), - ) - .await - .unwrap(); - assert_eq!(upd.affected_nodes, 1); - - // Insert after optimize also commits. - mutate_main( - &mut db, - MUTATION_QUERIES, - "insert_person", - &mixed_params(&[("$name", "Ivan")], &[("$age", 50)]), - ) - .await - .unwrap(); - assert_eq!(count_rows(&db, "node:Person").await, 8); // 4 seed + Eve/Frank/Grace + Ivan - - // State survives a reopen β€” the recovery sweep runs and finds no drift. - drop(db); - let reopened = Omnigraph::open(&uri).await.unwrap(); - assert_eq!(count_rows(&reopened, "node:Person").await, 8); - let alice = reopened - .entity_at_target(ReadTarget::branch("main"), "node:Person", "Alice") - .await - .unwrap() - .unwrap(); - assert_eq!( - alice["age"], - serde_json::json!(31), - "Alice's post-optimize age update must persist across reopen" - ); -} diff --git a/crates/omnigraph/tests/failpoints.rs b/crates/omnigraph/tests/failpoints.rs index d240108..149c63a 100644 --- a/crates/omnigraph/tests/failpoints.rs +++ b/crates/omnigraph/tests/failpoints.rs @@ -1245,7 +1245,7 @@ async fn refresh_defers_rollback_eligible_sidecar_to_next_open() { // the rollback (will use Dataset::restore safely; no concurrent // writers at open time). drop(db); - let db = Omnigraph::open(&uri).await.unwrap(); + let _db = Omnigraph::open(&uri).await.unwrap(); // After full-sweep recovery, the sidecar should be processed // (deleted). Sidecar's tables are eligible for rollback (UnexpectedAtP1): // restore happens on Person (HEAD advances by 1). @@ -1268,19 +1268,6 @@ async fn refresh_defers_rollback_eligible_sidecar_to_next_open() { "full sweep must run Dataset::restore (head advances); \ post_head={post_head}, final_head={final_head}", ); - // Convergence: roll-back published the restored HEAD, so the manifest pin - // tracks Lance HEAD afterward (no residual drift). - let entry_version = db - .snapshot_of(omnigraph::db::ReadTarget::branch("main")) - .await - .unwrap() - .entry("node:Person") - .unwrap() - .table_version; - assert_eq!( - entry_version, final_head, - "full-sweep roll-back must publish so manifest pin ({entry_version}) == Lance HEAD ({final_head})", - ); } /// Companion to the above β€” confirms that a finalizeβ†’publisher failure @@ -1474,15 +1461,10 @@ edge WorksAt: Person -> Company } let db = Omnigraph::open(&uri).await.unwrap(); - // Roll-back now publishes the restored version, so the manifest version - // advances β€” but to the OLD-schema content: the migration never applied - // (asserted by count_rows + the `_schema.pg` checks below), and the sweep - // converges (`manifest == Lance HEAD`, asserted by - // assert_post_recovery_invariants's RolledBack arm). - assert!( - version_main(&db).await.unwrap() > pre_failure_version, - "roll-back publishes the restored (old-schema) version, advancing the manifest; \ - pre={pre_failure_version}", + assert_eq!( + version_main(&db).await.unwrap(), + pre_failure_version, + "manifest must remain on the old schema when no schema staging files existed" ); assert_eq!( helpers::count_rows(&db, "node:Person").await, @@ -1655,100 +1637,6 @@ edge WorksAt: Person -> Company ); } -/// `optimize` Phase B β†’ Phase C residual: `compact_files` advanced the Lance -/// HEAD but the manifest publish hasn't run. The `Optimize` recovery sidecar -/// (loose-match, like SchemaApply/EnsureIndices) must roll the compacted version -/// forward on next open so the manifest tracks the Lance HEAD β€” and the healed -/// table must then accept a schema apply (the original bug's victim). -#[tokio::test] -async fn optimize_phase_b_failure_recovered_on_next_open() { - let _scenario = FailScenario::setup(); - let dir = tempfile::tempdir().unwrap(); - let uri = dir.path().to_str().unwrap().to_string(); - let operation_id; - - // Seed: several separate Person inserts β†’ multiple fragments, so compaction - // has real work and advances the Lance HEAD. - { - let db = Omnigraph::init(&uri, helpers::TEST_SCHEMA).await.unwrap(); - for (name, age) in [("alice", 30), ("bob", 31), ("carol", 32), ("dave", 33)] { - db.mutate( - "main", - MUTATION_QUERIES, - "insert_person", - &mixed_params(&[("$name", name)], &[("$age", age)]), - ) - .await - .unwrap(); - } - } - - let pre_failure_version = { - let db = Omnigraph::open(&uri).await.unwrap(); - version_main(&db).await.unwrap() - }; - - // Failpoint fires AFTER compact_files advanced the Lance HEAD but BEFORE the - // manifest publish. The Optimize sidecar persists (only node:Person has - // compactable fragments, so exactly one sidecar is written). - { - let db = Omnigraph::open(&uri).await.unwrap(); - let _failpoint = - ScopedFailPoint::new("optimize.post_phase_b_pre_manifest_commit", "return"); - let err = db.optimize().await.unwrap_err(); - assert!( - err.to_string() - .contains("injected failpoint triggered: optimize.post_phase_b_pre_manifest_commit"), - "unexpected error: {err}" - ); - - let recovery_dir = dir.path().join("__recovery"); - let sidecars: Vec<_> = std::fs::read_dir(&recovery_dir) - .unwrap() - .filter_map(|e| e.ok()) - .collect(); - assert_eq!( - sidecars.len(), - 1, - "exactly one Optimize sidecar must persist after optimize failure" - ); - operation_id = single_sidecar_operation_id(dir.path()); - } - - // Recovery: reopen runs the sweep. The Optimize sidecar classifies - // RolledPastExpected (loose-match) β†’ RollForward β†’ manifest extends to the - // compacted Lance HEAD. - let db = Omnigraph::open(&uri).await.unwrap(); - let post_recovery_version = version_main(&db).await.unwrap(); - assert!( - post_recovery_version > pre_failure_version, - "manifest version must advance post-recovery (compaction rolled forward); \ - pre={pre_failure_version}, post={post_recovery_version}", - ); - drop(db); - - assert_post_recovery_invariants( - dir.path(), - &operation_id, - RecoveryExpectation::RolledForward { - tables: vec![TableExpectation::main("node:Person")], - }, - ) - .await - .unwrap(); - - // The healed table accepts an additive schema apply β€” its HEAD-vs-manifest - // precondition is satisfied because recovery published the compacted version. - let db = Omnigraph::open(&uri).await.unwrap(); - let desired = helpers::TEST_SCHEMA.replace( - " age: I32?\n}", - " age: I32?\n nickname: String?\n}", - ); - db.apply_schema(&desired) - .await - .expect("schema apply after optimize recovery must succeed"); -} - #[tokio::test] async fn branch_merge_phase_b_failure_recovered_on_next_open() { use omnigraph::loader::{LoadMode, load_jsonl}; diff --git a/crates/omnigraph/tests/helpers/recovery.rs b/crates/omnigraph/tests/helpers/recovery.rs index 90d9a25..c76009e 100644 --- a/crates/omnigraph/tests/helpers/recovery.rs +++ b/crates/omnigraph/tests/helpers/recovery.rs @@ -181,9 +181,6 @@ pub async fn assert_post_recovery_invariants( "audit row for {operation_id} recorded the wrong recovery_kind", ); assert_rollback_outcomes_record_drift(&audit); - // Roll-back now publishes the restored HEAD, so manifest == Lance - // HEAD afterward (symmetric with roll-forward) β€” no residual drift. - assert_manifest_pins_match_lance_heads(graph_root, &tables).await?; assert_recovery_commit_shape(graph_root, &audit, &tables).await?; assert_non_main_did_not_move_main(graph_root, &tables).await?; assert_idempotent_reopen(graph_root, operation_id).await?; diff --git a/crates/omnigraph/tests/maintenance.rs b/crates/omnigraph/tests/maintenance.rs index 2a5a659..3e61677 100644 --- a/crates/omnigraph/tests/maintenance.rs +++ b/crates/omnigraph/tests/maintenance.rs @@ -8,12 +8,10 @@ mod helpers; use std::time::Duration; use lance::Dataset; -use omnigraph::db::{CleanupPolicyOptions, Omnigraph, ReadTarget, SkipReason}; +use omnigraph::db::{CleanupPolicyOptions, Omnigraph, SkipReason}; use omnigraph::loader::{LoadMode, load_jsonl}; -use helpers::{ - MUTATION_QUERIES, TEST_DATA, TEST_SCHEMA, count_rows, init_and_load, mixed_params, mutate_main, -}; +use helpers::{TEST_DATA, TEST_SCHEMA, count_rows, init_and_load}; /// Filesystem URI of a node sub-table, mirroring the engine's layout /// (FNV-1a of the type name under `nodes/`). Matches the helper in @@ -165,124 +163,6 @@ node Tag {\n slug: String @key\n}\n"; assert_eq!(tag.skipped, None, "non-blob table must not be skipped"); } -// Regression: `optimize` must publish its compaction to the `__manifest` so the -// manifest's recorded `table_version` tracks the compacted Lance HEAD. -// -// Lance `compact_files` advances the *dataset's* version (reserve-fragments + -// rewrite commits) but knows nothing about OmniGraph's `__manifest`. If optimize -// does not publish a manifest update, the manifest's `table_version` lags the -// Lance HEAD: reads stay pinned to the pre-compaction version (compaction is -// invisible to them) and any subsequent schema apply / strict update/delete -// fails its HEAD-vs-manifest precondition with -// "stale view of '': expected manifest table version X but current is Y". -// This pins the fix β€” optimize publishes the compacted version, so manifest == -// HEAD and migrations after a compaction succeed. -#[tokio::test] -async fn optimize_publishes_compaction_to_manifest_so_schema_apply_succeeds() { - let dir = tempfile::tempdir().unwrap(); - let root = dir.path().to_str().unwrap().trim_end_matches('/').to_string(); - let mut db = init_and_load(&dir).await; - - // Several separate inserts β†’ multiple Person fragments, so `compact_files` - // actually merges and moves the Lance HEAD (a single fragment is a no-op). - for (name, age) in [("Eve", 40), ("Frank", 41), ("Grace", 42), ("Heidi", 43)] { - mutate_main( - &mut db, - MUTATION_QUERIES, - "insert_person", - &mixed_params(&[("$name", name)], &[("$age", age as i64)]), - ) - .await - .expect("insert"); - } - - let stats = db.optimize().await.unwrap(); - let person = stats - .iter() - .find(|s| s.table_key == "node:Person") - .expect("Person stat present"); - assert!( - person.committed, - "Person is multi-fragment, so optimize must have compacted it" - ); - - // After optimize, the manifest's recorded table_version must equal the actual - // Lance HEAD β€” optimize published its compaction, so there is no drift. - let snap = db.snapshot_of(ReadTarget::branch("main")).await.unwrap(); - let entry = snap.entry("node:Person").unwrap(); - let manifest_version = entry.table_version; - let full = format!("{}/{}", root, entry.table_path); - let lance_head = Dataset::open(&full).await.unwrap().version().version; - assert_eq!( - manifest_version, lance_head, - "after optimize, manifest table_version ({manifest_version}) must equal Lance HEAD ({lance_head})", - ); - - // Reads observe the compacted version with rows preserved (4 seed + 4 inserts). - assert_eq!(count_rows(&db, "node:Person").await, 8); - - // The headline: an additive (nullable property) migration touching the - // just-compacted table succeeds, where it previously failed with "stale view". - let desired = TEST_SCHEMA.replace( - " age: I32?\n}", - " age: I32?\n nickname: String?\n}", - ); - let result = db - .apply_schema(&desired) - .await - .expect("additive schema apply after optimize must succeed"); - assert!(result.applied, "schema apply should report applied=true"); -} - -// Regression: `optimize` must REFUSE when an unresolved recovery sidecar is -// pending. Operating on an unrecovered graph could publish a partial write that -// the all-or-nothing recovery sweep would roll back; the operator must reopen -// (run the recovery sweep) first. -#[tokio::test] -async fn optimize_defers_when_recovery_sidecar_is_pending() { - let dir = tempfile::tempdir().unwrap(); - let uri = dir.path().to_str().unwrap(); - let db = init_and_load(&dir).await; - - // Simulate an in-process failed write that left a recovery sidecar on disk. - let recovery_dir = dir.path().join("__recovery"); - std::fs::create_dir_all(&recovery_dir).unwrap(); - let person_path = node_table_uri(uri, "Person"); - let sidecar_json = format!( - r#"{{ - "schema_version": 1, - "operation_id": "01H000000000000000000DEFR", - "started_at": "0", - "branch": null, - "actor_id": "act-test", - "writer_kind": "Mutation", - "tables": [ - {{ - "table_key": "node:Person", - "table_path": "{}", - "expected_version": 1, - "post_commit_pin": 2 - }} - ] - }}"#, - person_path - ); - std::fs::write( - recovery_dir.join("01H000000000000000000DEFR.json"), - sidecar_json, - ) - .unwrap(); - - let err = db - .optimize() - .await - .expect_err("optimize must defer (error) while a recovery sidecar is pending"); - assert!( - err.to_string().to_lowercase().contains("recovery"), - "optimize defer error should mention recovery; got: {err}", - ); -} - #[tokio::test] async fn cleanup_without_any_policy_option_errors() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/omnigraph/tests/recovery.rs b/crates/omnigraph/tests/recovery.rs index f6b19e8..a090178 100644 --- a/crates/omnigraph/tests/recovery.rs +++ b/crates/omnigraph/tests/recovery.rs @@ -278,97 +278,6 @@ async fn recovery_rolls_back_synthetic_drift_on_open() { ); } -/// Regression: recovery roll-back must PUBLISH the restored version so -/// `manifest == Lance HEAD` afterward (no residual "orphaned drift"). Before the -/// fix, roll-back restored via `Dataset::restore` but left the manifest pin -/// behind HEAD, so a subsequent strict write / schema apply failed its -/// HEAD-vs-manifest precondition ("stale view … refresh and retry") β€” and a -/// failed schema apply's own roll-back leaked +1 each retry (the original bug's -/// loop). With convergence, one roll-back leaves `manifest == HEAD` and the -/// follow-up succeeds. -#[tokio::test] -async fn recovery_rollback_converges_manifest_so_schema_apply_succeeds() { - use omnigraph::db::ReadTarget; - use omnigraph::loader::{LoadMode, load_jsonl}; - use omnigraph::table_store::TableStore; - - let dir = tempfile::tempdir().unwrap(); - let uri = dir.path().to_str().unwrap(); - - let mut db = Omnigraph::init(uri, TEST_SCHEMA).await.unwrap(); - load_jsonl( - &mut db, - r#"{"type":"Person","data":{"name":"alice","age":30}} -{"type":"Person","data":{"name":"bob","age":25}} -"#, - LoadMode::Append, - ) - .await - .unwrap(); - drop(db); - - // Forge a Phase-B residual: advance Person's Lance HEAD without publishing to - // the manifest (the manifest pin stays at the load's committed version). - let person_uri = node_table_uri(uri, "Person"); - let store = TableStore::new(uri); - let mut ds = Dataset::open(&person_uri).await.unwrap(); - let manifest_pin = ds.version().version; - let _ = store - .delete_where(&person_uri, &mut ds, "1 = 2") - .await - .unwrap(); - drop(ds); - - // Roll-back-classified sidecar (post_commit_pin != observed head β‡’ - // UnexpectedAtP1 β‡’ RollBack). - let sidecar_json = format!( - r#"{{ - "schema_version": 1, - "operation_id": "01H0000000000000000000CVG", - "started_at": "0", - "branch": null, - "actor_id": "act-test", - "writer_kind": "Mutation", - "tables": [ - {{ - "table_key": "node:Person", - "table_path": "{}", - "expected_version": {}, - "post_commit_pin": {} - }} - ] - }}"#, - person_uri, manifest_pin, manifest_pin - ); - write_sidecar_file(dir.path(), "01H0000000000000000000CVG", &sidecar_json); - - // Reopen runs the sweep: restore Person to manifest_pin, then PUBLISH so the - // manifest tracks the restored Lance HEAD. - let db = Omnigraph::open(uri).await.unwrap(); - - // Convergence: manifest pin == Lance HEAD. Fails before the fix β€” the - // manifest stays at manifest_pin while HEAD advanced past it. - let snap = db.snapshot_of(ReadTarget::branch("main")).await.unwrap(); - let entry = snap.entry("node:Person").unwrap(); - let lance_head = Dataset::open(&person_uri).await.unwrap().version().version; - assert_eq!( - entry.table_version, lance_head, - "roll-back must publish so manifest pin ({}) == Lance HEAD ({})", - entry.table_version, lance_head, - ); - - // The +1-loop victim: an additive schema apply must now succeed (its - // HEAD-vs-manifest precondition is satisfied). Before the fix this failed - // with "stale view … refresh and retry". - let desired = TEST_SCHEMA.replace( - " age: I32?\n}", - " age: I32?\n nickname: String?\n}", - ); - db.apply_schema(&desired) - .await - .expect("schema apply after a converging roll-back must succeed"); -} - // ===================================================================== // Phase 4 β€” roll-forward path + audit row recording // ===================================================================== diff --git a/crates/omnigraph/tests/writes.rs b/crates/omnigraph/tests/writes.rs index 0a309c9..13cb10f 100644 --- a/crates/omnigraph/tests/writes.rs +++ b/crates/omnigraph/tests/writes.rs @@ -371,10 +371,11 @@ async fn cancelled_mutation_future_leaves_no_state() { // Cancel-safety property: no graph-level run/staging state remains. // - // No `__run__` branches can ever be created: the Run state machine - // (`begin_run` etc.) was deleted in MR-771 β€” verified by the build itself, - // those symbols no longer exist. Any legacy `__run__*` branch on an - // upgraded graph is swept by the v2β†’v3 manifest migration. + // Note: `branch_list()` already filters `__run__*` via + // `is_internal_system_branch`, so a runtime "no `__run__` branches" check + // would be vacuous. The structural property that no `__run__` branches + // can ever be created is enforced by deletion of `begin_run` etc. in + // (verified by the build itself β€” those symbols no longer exist). // // (1) The branch list is unchanged: cancellation/completion cannot // synthesize new public branches. @@ -441,40 +442,34 @@ async fn repeated_loads_do_not_accumulate_branches() { assert_eq!(db.branch_list().await.unwrap(), vec!["main".to_string()]); } -/// After MR-770, `__run__*` is an ordinary branch name β€” the Run state machine -/// and its `is_internal_run_branch` guard are gone. The surviving internal-ref -/// guard still rejects the active `__schema_apply_lock__` branch on the public -/// create/merge APIs. +/// User code must not be able to write to internal `__run__*` names. +/// The branch-name guard predicate is kept as defense-in-depth; it +/// will be removed once a future production sweep retires the legacy +/// branches. #[tokio::test] -async fn public_branch_apis_reject_internal_system_refs() { +async fn public_branch_apis_reject_internal_run_refs() { let dir = tempfile::tempdir().unwrap(); let mut db = init_and_load(&dir).await; - // `__run__*` is no longer reserved β€” creating it now succeeds. - db.branch_create("__run__formerly_reserved") - .await - .expect("__run__ prefix is a normal branch name post-MR-770"); - - // The schema-apply lock branch is still rejected on public branch APIs. - let create_err = db.branch_create("__schema_apply_lock__").await.unwrap_err(); + let create_err = db.branch_create("__run__synthetic").await.unwrap_err(); let OmniError::Manifest(err) = create_err else { panic!("expected Manifest error"); }; assert!( - err.message.contains("internal system ref"), + err.message.contains("internal run ref"), "unexpected error: {}", err.message ); let merge_err = db - .branch_merge("__schema_apply_lock__", "main") + .branch_merge("__run__synthetic", "main") .await .unwrap_err(); let OmniError::Manifest(err) = merge_err else { panic!("expected Manifest error"); }; assert!( - err.message.contains("internal system refs"), + err.message.contains("internal run refs"), "unexpected error: {}", err.message ); diff --git a/docs/dev/branch-protection.md b/docs/dev/branch-protection.md index 2b6cc37..9b2fa78 100644 --- a/docs/dev/branch-protection.md +++ b/docs/dev/branch-protection.md @@ -8,7 +8,7 @@ This page explains what the policy says and how to change it. | Setting | Value | Why | |---|---|---| -| **Required status checks (strict)** | `Classify Changes`, `Check AGENTS.md Links`, `Test Workspace`, `Test omnigraph-server --features aws`, `CODEOWNERS matches source`, `CODEOWNERS not hand-edited` | Every PR must pass workspace tests, AGENTS.md link integrity, and the CODEOWNERS hygiene checks. The two CODEOWNERS contexts must equal the job `name:` values in `.github/workflows/codeowners.yml` **verbatim** β€” a context naming a job that never reports (the old `CODEOWNERS / drift` used the job *id*, and the job was path-filtered) leaves every PR permanently pending and forces admin overrides. `strict: true` requires the branch to be up-to-date with `main` before merge. | +| **Required status checks (strict)** | `Classify Changes`, `Check AGENTS.md Links`, `Test Workspace`, `Test omnigraph-server --features aws`, `CODEOWNERS / drift`, `CODEOWNERS / noedit` | Every PR must pass workspace tests, AGENTS.md link integrity, and the CODEOWNERS hygiene checks. `strict: true` requires the branch to be up-to-date with `main` before merge. | | **Required approving reviews** | `1` | At least one reviewer. With a 2-person team, going higher would block all merges when one person is unavailable. | | **Require code-owner reviews** | `true` | The reviewer must be a code owner per `.github/CODEOWNERS`. This is what makes the codeowners chassis enforced. | | **Dismiss stale reviews on new commits** | `true` | A push after approval invalidates the prior review. Prevents the "approve, then sneak in unreviewed changes" pattern. | @@ -16,7 +16,7 @@ This page explains what the policy says and how to change it. | **Disallow force pushes** | `true` | No history rewrites on `main`. | | **Disallow branch deletions** | `true` | `main` cannot be deleted. | | **Required conversation resolution** | `true` | All review comment threads must be resolved before merge. | -| **Enforce on admins** | `false` | Admins can override the gates (`enforce_admins: false` in the JSON). This is the intended escape hatch for the 2-person team; tightening to `true` is tracked under hardening below. | +| **Enforce on admins** | `true` | Even repository admins go through the gates. The point is no bypasses. | | **Required signed commits** | not yet | Not enabled. Would lock out maintainers until everyone enrolls GPG/SSH commit signing. Tracked as a follow-up. | ## How to apply diff --git a/docs/dev/codeowners.md b/docs/dev/codeowners.md index 50c4dc7..9a7fb50 100644 --- a/docs/dev/codeowners.md +++ b/docs/dev/codeowners.md @@ -4,45 +4,24 @@ This setup gives every role change a reviewable PR and a permanent in-repository audit trail (`git log .github/codeowners-roles.yml`). -## Who owns what +## Current roles -The tables below are **generated** from `.github/codeowners-roles.yml` by `.github/scripts/render-codeowners.py` (the same render that produces `.github/CODEOWNERS`). They are the always-current "who owns what at this commit" view β€” don't edit them by hand; edit the yml and re-render. - - - -**Path β†’ owners** (GitHub applies *last match wins*; the `*` catch-all is listed first and is overridden by the specific patterns below it): - -| Path | Owners | Role(s) | +| Role | Members | Scope | |---|---|---| -| `*` | @ragnorc @aaltshuler | engineering | -| `crates/**` | @ragnorc @aaltshuler | engineering | -| `docs/**` | @ragnorc | docs | -| `README.md` | @ragnorc | docs | -| `AGENTS.md` | @ragnorc | docs | -| `CLAUDE.md` | @ragnorc | docs | -| `SECURITY.md` | @ragnorc | docs | +| `engineering` | `@ragnorc` | All code under `crates/**`, repository infrastructure, default for unmapped paths | +| `docs` | `@ragnorc` | `docs/**`, README.md, AGENTS.md, CLAUDE.md, SECURITY.md | -**Roles**: - -| Role | Members | Description | -|---|---|---| -| `engineering` | @ragnorc @aaltshuler | All production code under crates/**. Engine, CLI, server, compiler. | -| `docs` | @ragnorc | Documentation under docs/**, plus repo-level docs (README.md, AGENTS.md, CLAUDE.md symlink, SECURITY.md). | - - - -GitHub treats multiple owners on a CODEOWNERS line as **"any one of them satisfies the review requirement"**. To require N distinct approvers on a specific path, layer a CI check on top (not currently configured). +GitHub treats multiple owners in a CODEOWNERS line as **"any one of them satisfies the review requirement"**. To require N distinct approvers on a specific path, layer a CI check on top (not currently configured). ## How to change role membership or path mappings 1. Edit `.github/codeowners-roles.yml`. -2. Open a PR. **CI re-renders for you**: the `CODEOWNERS` workflow regenerates `.github/CODEOWNERS` and the ownership tables above and auto-commits them back to your PR branch on same-repository PRs β€” you don't have to run the script locally (though you can: `python3 .github/scripts/render-codeowners.py`, requires PyYAML). - -On a fork (where CI can't push back), the workflow instead fails with the diff so you can run the script and commit it yourself. +2. Run `python3 .github/scripts/render-codeowners.py` (requires PyYAML; `pip install pyyaml`). +3. Commit both files in the same PR. CI fails the PR if: -- a fork PR left a generated artifact out of sync, or -- `CODEOWNERS` was edited without a corresponding yml change (the `CODEOWNERS not hand-edited` check). +- `CODEOWNERS` was edited without a corresponding yml change, or +- The yml was changed but the rendered `CODEOWNERS` doesn't match. ## How to add a new role diff --git a/docs/dev/index.md b/docs/dev/index.md index 1e41342..600c969 100644 --- a/docs/dev/index.md +++ b/docs/dev/index.md @@ -51,18 +51,6 @@ constraints. User-facing behavior should still be documented through | Install and deployment packaging | [install.md](../user/install.md), [deployment.md](../user/deployment.md) | | Release history | [releases/](../releases/) | -## Contribution & Governance - -| Area | Read | -|---|---| -| How to contribute (external) | [CONTRIBUTING.md](../../CONTRIBUTING.md) | -| Governance model, roles, decision authority | [GOVERNANCE.md](../../GOVERNANCE.md) | -| Public contribution RFC track | [rfcs/](../rfcs/) | - -The `docs/rfcs/` track is the **public, externally-authorable** RFC process. The -maintainer/internal RFCs below (`rfc-00N-*.md`) are a separate, team-owned -track; don't conflate the two. - ## Active Implementation Plans Working documents for in-flight feature work. Removed when the work lands. diff --git a/docs/dev/testing.md b/docs/dev/testing.md index 8974a9f..425fcee 100644 --- a/docs/dev/testing.md +++ b/docs/dev/testing.md @@ -34,10 +34,10 @@ The engine's `tests/` is the principal coverage surface; most graph-shaped behav | `s3_storage.rs` | S3-backed graph (skipped unless `OMNIGRAPH_S3_TEST_BUCKET` is set) | | `lance_version_columns.rs` | Per-row `_row_last_updated_at_version` behavior | | `validators.rs` | Schema constraint enforcement (enum, range, unique, cardinality) across JSONL, insert, update paths | -| `maintenance.rs` | `optimize` (compaction) + `cleanup` (version GC): empty/idempotent/no-op edges, policy validation, head preservation; `optimize` publishes the compacted version so the manifest tracks the Lance HEAD and a subsequent schema apply succeeds (`optimize_publishes_compaction_to_manifest_so_schema_apply_succeeds`), and refuses to run while a `__recovery` sidecar is pending so optimize only ever operates on a recovered graph (`optimize_defers_when_recovery_sidecar_is_pending`) | -| `failpoints.rs` | Failure-injection coverage (gated on `failpoints` feature). Includes the five per-writer Phase B β†’ recovery integration tests (`recovery_rolls_forward_after_finalize_publisher_failure`, `schema_apply_phase_b_failure_recovered_on_next_open`, `branch_merge_phase_b_failure_recovered_on_next_open`, `ensure_indices_phase_b_failure_recovered_on_next_open`, `optimize_phase_b_failure_recovered_on_next_open`). | +| `maintenance.rs` | `optimize` (compaction) + `cleanup` (version GC): empty/idempotent/no-op edges, policy validation, head preservation | +| `failpoints.rs` | Failure-injection coverage (gated on `failpoints` feature). Includes the four per-writer Phase B β†’ recovery integration tests (`recovery_rolls_forward_after_finalize_publisher_failure`, `schema_apply_phase_b_failure_recovered_on_next_open`, `branch_merge_phase_b_failure_recovered_on_next_open`, `ensure_indices_phase_b_failure_recovered_on_next_open`). | | `recovery.rs` | Open-time recovery sweep β€” sidecar I/O, classifier dispatch (NoMovement / RolledPastExpected / UnexpectedAtP1 / UnexpectedMultistep / InvariantViolation), all-or-nothing decision, roll-forward via `ManifestBatchPublisher::publish`, roll-back via `Dataset::restore`, audit row in `_graph_commit_recoveries.lance`, `OpenMode::ReadOnly` skip path | -| `composite_flow.rs` | Compositional/narrative end-to-end stories β€” multi-step flows that compose mechanics covered by other test files. Catches integration regressions where individual operations all pass their unit tests but their composition breaks (sequential merges, post-merge main writes, time-travel through merge DAG, reopen consistency over multi-merge histories, post-optimize and post-cleanup strict writes). | +| `composite_flow.rs` | Compositional/narrative end-to-end stories β€” multi-step flows that compose mechanics covered by other test files. Catches integration regressions where individual operations all pass their unit tests but their composition breaks (sequential merges, post-merge main writes, time-travel through merge DAG, reopen consistency over multi-merge histories). | ## Fixtures diff --git a/docs/dev/writes.md b/docs/dev/writes.md index d2c7c7e..974f7a6 100644 --- a/docs/dev/writes.md +++ b/docs/dev/writes.md @@ -14,11 +14,8 @@ publisher's row-level CAS on `__manifest` is the single fence. - No `RunRecord`, no `_graph_runs.lance`, no `_graph_run_actors.lance`. - No `omnigraph run *` CLI subcommands and no `/runs/*` HTTP endpoints. -- No `__run__` staging branches; `__run__*` is no longer a reserved - name. The branch-name guard was removed in MR-770, and any stale - `__run__*` branch on an upgraded graph is swept off `__manifest` by the - v2β†’v3 internal-schema migration on first read-write open. (The inert - `_graph_runs.lance` bytes remain until a `delete_prefix` primitive lands.) +- No `__run__` staging branches. (Legacy on-disk artifacts from + pre-MR-771 repos are inert; MR-770 sweeps them in production.) - Cancelled mutation futures leave **no graph-level state** β€” only orphaned Lance fragments, which the existing `omnigraph cleanup` pipe reclaims. @@ -157,14 +154,10 @@ are left at `Lance HEAD = manifest_pinned + 1`. **Recovery protocol** (lifecycle of every staged-write writer β€” `MutationStaging::finalize`, `schema_apply::apply_schema_with_lock`, -`branch_merge_on_current_target`, `ensure_indices_for_branch`, -`optimize_all_tables`): +`branch_merge_on_current_target`, `ensure_indices_for_branch`): 1. **Phase A**: writer writes a sidecar JSON to - `__recovery/{ulid}.json` BEFORE its first HEAD-advancing commit - (`commit_staged`, or `compact_files` for `optimize_all_tables`, - which advances the Lance HEAD via a reserve-fragments + rewrite - commit rather than a staged write). The + `__recovery/{ulid}.json` BEFORE its first `commit_staged`. The sidecar names every `(table_key, table_path, expected_version, post_commit_pin)` it intends to commit + the writer kind + actor_id. @@ -199,13 +192,8 @@ recovery sweep in `crates/omnigraph/src/db/manifest/recovery.rs`: otherwise full open-time recovery rolls them back and refresh-time recovery leaves them for the next read-write open. - Otherwise **roll back**: per-table `Dataset::restore` to the - manifest-pinned table version, then a single `ManifestBatchPublisher::publish` - of the restored HEAD β€” symmetric with roll-forward, so `manifest == HEAD` - after recovery (no residual drift). This convergence is what lets a - failed-then-retried schema apply succeed instead of failing one version higher - each iteration. The audit row's `to_version` records the logical - rolled-back-to version (`manifest_pinned`); the manifest is published at the - restore commit (`manifest_pinned + 1`, same content). + manifest-pinned table version for that branch. Rollback records the + actual restore target in the audit row's `to_version`. - After a successful roll-forward or roll-back, an audit row is recorded β€” `_graph_commits.lance` carries a commit tagged `actor_id = "omnigraph:recovery"`, and a sibling @@ -257,14 +245,9 @@ list`. ## Migration code -`db/manifest/migrations.rs` carries the v2β†’v3 internal-schema step (MR-770): -a one-time sweep that deletes legacy `__run__*` staging branches off -`__manifest`. It runs in `Omnigraph::open(ReadWrite)` (via -`manifest::migrate_on_open`, before the coordinator reads branch state) and -again on the publisher's write path; both are idempotent once the stamp is at -v3. Deleting the inert `_graph_runs.lance` / `_graph_run_actors.lance` dataset -*bytes* is still deferred β€” it needs a `StorageAdapter::delete_prefix` -primitive β€” but those bytes are invisible to graph-level state. +`db/manifest/migrations.rs` does not change. Active deletion of +`_graph_runs.lance` belongs in MR-770 (the production sweep) β€” this PR +stops *creating* run state but does not destroy legacy bytes on disk. ## Mid-query partial failure: closed by MR-794 diff --git a/docs/releases/v0.6.1.md b/docs/releases/v0.6.1.md index 0acc34b..aafe1af 100644 --- a/docs/releases/v0.6.1.md +++ b/docs/releases/v0.6.1.md @@ -7,7 +7,6 @@ v0.6.1 focuses on operational polish after v0.6.0: stored-query registries, safe - **Stored-query registries.** `omnigraph.yaml` can declare curated `queries:` blocks per graph. Servers load and type-check them at startup, `omnigraph queries validate` checks them offline, `omnigraph queries list` shows exposed queries and typed params, `GET /queries` exposes a typed catalog, and `POST /queries/{name}` invokes a stored query without accepting ad hoc `.gq` source from the client. - **Stored-query policy gate.** New Cedar action `invoke_query` gates the stored-query invocation surface. Stored mutations are double-gated: `invoke_query` to reach the stored query and `change` for the actual write. - **Safer branch deletion.** `branch_delete` now treats the manifest as the authority, flips branch visibility atomically, and reclaims per-table/commit-graph forks as derived state. If best-effort reclaim is interrupted, `cleanup` reconciles orphaned forks; reusing a branch name before cleanup reports an actionable error. -- **Legacy `__run__` cleanup (MR-770).** Removed the last functional remnant of the Run state machine (retired in v0.4.0): the `__run__` branch-name guard. A new v2β†’v3 `__manifest` internal-schema migration sweeps any stale `__run__*` staging branches on the first read-write open, so `__run__*` is no longer a reserved branch name. This closes the "unpromoted `__run__` branches block reads" condition behind the zombie-run cascade incident; the inert `_graph_runs.lance` row cleanup is tracked separately (it needs a `delete_prefix` primitive). - **Blob-safe optimize.** `omnigraph optimize` skips tables with `Blob` properties instead of failing the whole sweep on Lance's blob-v2 compaction decode bug. Skips are visible in human output, `--json` as `skipped`, `TableOptimizeStats.skipped`, and logs; non-blob tables still compact normally. - **Deployment improvements.** The container entrypoint now composes `OMNIGRAPH_TARGET_URI` with `OMNIGRAPH_CONFIG`, so operators can keep the graph URI in env while loading policy/query config from a mounted file. The local RustFS bootstrap pins RustFS beta.3 and allows the current insecure local-dev default credentials. - **Windows release support.** Tagged and edge releases now publish Windows x86_64 archives containing `omnigraph.exe` and `omnigraph-server.exe`, with a PowerShell installer and Windows install docs. @@ -18,7 +17,6 @@ v0.6.1 focuses on operational polish after v0.6.0: stored-query registries, safe - A graph selected by name (`--target` or `server.graph`) now uses `graphs..policy` and `graphs..queries`. Top-level `policy` / `queries` blocks are only for anonymous bare-URI single-graph mode; using them with a named graph now fails loudly with migration guidance. - `mcp.expose` defaults to `true` for stored-query registry entries. Set `mcp: { expose: false }` for service-only queries that should not appear in the catalog. - `invoke_query` is graph-scoped, not branch-scoped. Branch/snapshot access remains enforced by the inner `read` / `change` gate. -- **Legacy `__run__` migration.** Graphs created before v0.4.0 are migrated automatically on the first **read-write** open by a v0.6.1 binary (one-time `__manifest` stamp v2β†’v3 sweep of stale `__run__*` branches). No action required. Two caveats: (1) a graph opened **read-only** still lists any stale `__run__*` branch until its first read-write open, since the migration is write-path-only like all manifest migrations β€” long-lived read-only deployments should be opened read-write once after upgrading; (2) the inert `_graph_runs.lance` / `_graph_run_actors.lance` dataset bytes are left in place until a future `delete_prefix` primitive (they are invisible to graph-level state). - Blob tables are not compacted until the upstream Lance fix lands, so fragment count and deleted-row space on blob tables are not reclaimed by `optimize`. Reads, writes, and query results are unaffected; no on-disk migration is required. - `TableOptimizeStats` is now `#[non_exhaustive]` and gains a `skipped: Option` field (so does the new `SkipReason` enum). This is a source-level change only for downstream code that built this returned result struct by literal β€” rare, since it is produced by `optimize` and consumed by reading its fields; field access is unaffected, and `#[non_exhaustive]` keeps future additions non-breaking. diff --git a/docs/rfcs/0000-template.md b/docs/rfcs/0000-template.md deleted file mode 100644 index 48f4bda..0000000 --- a/docs/rfcs/0000-template.md +++ /dev/null @@ -1,54 +0,0 @@ -# RFC NNNN: - -| | | -|---|---| -| **Status** | Proposed | -| **Author(s)** | <your name / handle> | -| **Discussion** | <link to the originating Discussion, if any> | -| **Implementation** | <issue/PR links, filled in as work lands> | - -> Status is maintained by maintainers: `Proposed` while the PR is open, -> `Accepted` on merge, `Declined` on close, `Superseded by NNNN` later. - -## Summary - -One paragraph: what this changes, in plain terms. - -## Motivation - -What problem does this solve, and why is it worth the ongoing cost? Tie it to a -concrete need (a Discussion, a recurring issue, a user request). Per the -project's first principle, argue the *long-run liability*, not just the -short-term convenience. - -## Guide-level explanation - -Explain the change as you'd teach it to a user or contributor: new commands, -syntax, API shapes, behavior. Examples first. - -## Reference-level design - -The precise design: data structures, IR/AST/planner changes, storage/format -impact, migration path, error behavior. Enough that a reviewer can find the -holes. - -## Invariants & deny-list check - -Which Hard Invariants in [../dev/invariants.md](../dev/invariants.md) does this -touch? Does it brush against any deny-list item β€” and if so, why is this the -justified exception? State explicitly that no invariant is weakened, or which -Known Gap moves. - -## Drawbacks & alternatives - -What does this cost, what did you reject, and why. "Do nothing" is a valid -alternative to weigh. - -## Reversibility - -Is this reversible? On-disk/wire/format and substrate choices are near-permanent -and demand more evidence; a CLI flag or doc is cheap to undo. Say which this is. - -## Unresolved questions - -What's deliberately left open for review to settle. diff --git a/docs/rfcs/README.md b/docs/rfcs/README.md deleted file mode 100644 index 99cdd76..0000000 --- a/docs/rfcs/README.md +++ /dev/null @@ -1,66 +0,0 @@ -# RFCs - -Substantial changes to OmniGraph β€” new user-facing surface, format or protocol -changes, anything irreversible or cross-cutting β€” go through a lightweight RFC -so the design is agreed *as reviewable code* before implementation starts. This -is the public RFC track, open to **anyone, including external contributors**. - -This complements the always-on review bar in -[../dev/invariants.md](../dev/invariants.md): the invariants say *what every -change must respect*; an RFC says *why this particular change is worth making and -how*. - -> **Two tracks, don't conflate them.** This `docs/rfcs/` directory is the -> **public contribution** track (anyone authors; maintainers accept). The -> maintainer-internal RFCs under `docs/dev/rfc-00N-*.md` are a separate, -> team-owned track for in-flight internal work. If you're an outside -> contributor, you're in the right place here. - -## When you need one - -- **RFC required:** new query/schema/CLI/HTTP surface; on-disk or wire-format - changes; a new substrate dependency; anything the deny-list in - [../dev/invariants.md](../dev/invariants.md) flags; anything irreversible - ("reversibility shapes evidence demand"). -- **RFC not required:** bug fixes for an `accepted` issue, and the trivial - fast-lane (typos, docs, deps) β€” see [../../CONTRIBUTING.md](../../CONTRIBUTING.md). - -If you're unsure, start a [Discussion](../../../discussions); a maintainer will -tell you whether it needs an RFC. - -## Lifecycle - -``` -Discussion (incubate, get rough consensus) - β”‚ graduate - β–Ό -RFC pull request β†’ adds docs/rfcs/NNNN-title.md (Status: Proposed) - β”‚ -maintainer review ──▢ changes requested / declined (PR closed, with rationale) - β”‚ - β–Ό -merged == Accepted (the merged file is the durable decision record) - β”‚ - β–Ό -Implementation PR(s) reference the accepted RFC -``` - -- **Author:** anyone. **Acceptance:** a maintainer decision, performed by - merging the RFC PR. Declining is closing it with rationale. -- The merged RFC *is* the accepted record β€” there is no separate sign-off step. -- Later reversals don't edit history: supersede with a new RFC that links back - and flip the old one's `Status` to `Superseded`. - -## Numbering & naming - -- File: `docs/rfcs/NNNN-kebab-title.md`, where `NNNN` is the next free - zero-padded integer (`0001`, `0002`, …). `0000-template.md` is reserved. -- Pick the number when you open the PR; if it collides with another in-flight - RFC, the second to merge bumps theirs. - -## Status values - -`Proposed` (open PR) Β· `Accepted` (merged) Β· `Declined` (closed) Β· -`Superseded by NNNN` Β· `Implemented` (set once the work lands, optional). - -Copy [0000-template.md](0000-template.md) to start. diff --git a/docs/user/audit.md b/docs/user/audit.md index ab028ac..e8abe5b 100644 --- a/docs/user/audit.md +++ b/docs/user/audit.md @@ -4,4 +4,4 @@ - `_as` variants of every write API let callers override the actor: `mutate_as`, `ingest_as`, `branch_merge_as`, `apply_schema_as`, etc. - Actor IDs are persisted on `GraphCommit.actor_id` with split storage in `_graph_commit_actors.lance` (the commit graph is split into `_graph_commits.lance` for the linkage and `_graph_commit_actors.lance` for the actor map). - HTTP server uses the bearer-token actor automatically; CLI uses the local user / explicit env (no implicit actor). -- Pre-v0.4.0 graphs also stored actor IDs on `RunRecord.actor_id` in `_graph_runs.lance` / `_graph_run_actors.lance`. The Run state machine was removed in MR-771; those files are inert post-v0.4.0. The v2β†’v3 manifest migration sweeps any stale `__run__*` branches on first write-open (MR-770); the inert dataset bytes remain until a `delete_prefix` primitive lands. +- Pre-v0.4.0 graphs also stored actor IDs on `RunRecord.actor_id` in `_graph_runs.lance` / `_graph_run_actors.lance`. The Run state machine was removed in MR-771; those files are inert post-v0.4.0 and reclaimed by MR-770's production sweep. diff --git a/docs/user/branches-commits.md b/docs/user/branches-commits.md index a4044cb..c1894f9 100644 --- a/docs/user/branches-commits.md +++ b/docs/user/branches-commits.md @@ -9,8 +9,8 @@ Lance supports branching at the dataset level: a branch is a named lineage of ve OmniGraph builds *graph branches* on top by branching every sub-table coherently: - `branch_create(name)` / `branch_create_from(target, name)` β€” disallowed name `main`; fails if branch exists; ensures the schema-apply lock is idle. Atomic and authority-first like `branch_delete`: it flips the `__manifest` branch (authority), then creates the derived commit-graph branch, force-dropping any orphaned commit-graph ref left by an incomplete prior delete (the manifest branch is fresh, so a same-named commit-graph branch is provably a zombie). If commit-graph creation fails, the manifest branch is rolled back so the name never half-exists. -- `branch_list()` β€” returns public branches, **filters the internal** `__schema_apply_lock__` branch. -- `branch_delete(name)` β€” refuses if there are descendants on the branch, or if it is the current branch. The manifest is the single authority for branch existence: deletion flips the `__manifest` branch ref first (one atomic op), after which the branch is gone from every snapshot. The owned per-table forks and the commit-graph branch are derived state, reclaimed best-effort with `force_delete_branch` after the flip. A failure during that reclaim (transient object-store error) does not fail the call or block the authority flip; the leftover forks are unreachable orphans that the [`cleanup`](maintenance.md) reconciler converges. One consequence: if a delete's best-effort reclaim fails, reusing that branch name before the next `cleanup` surfaces a clear error pointing at `cleanup` (the stale fork would otherwise collide on first write). +- `branch_list()` β€” returns public branches, **filters internal** `__run__…` and `__schema_apply_lock__` prefixes. +- `branch_delete(name)` β€” refuses if there are descendants or active runs on the branch. The manifest is the single authority for branch existence: deletion flips the `__manifest` branch ref first (one atomic op), after which the branch is gone from every snapshot. The owned per-table forks and the commit-graph branch are derived state, reclaimed best-effort with `force_delete_branch` after the flip. A failure during that reclaim (transient object-store error) does not fail the call or block the authority flip; the leftover forks are unreachable orphans that the [`cleanup`](maintenance.md) reconciler converges. One consequence: if a delete's best-effort reclaim fails, reusing that branch name before the next `cleanup` surfaces a clear error pointing at `cleanup` (the stale fork would otherwise collide on first write). - **Lazy forking**: a branch only forks a sub-table when that sub-table is first mutated on it. Pure-read branches share fragments with their source. A fork collision is classified by the manifest authority, not by Lance branch versions: if the live manifest already records the fork on the active branch, a concurrent first-write won and the caller gets a retryable "refresh and retry"; if the manifest does not, a physical branch there is an orphan and the caller is pointed at `cleanup`. - `sync_branch(branch)` β€” re-binds the in-memory handle to the latest head of the branch. @@ -51,13 +51,13 @@ Notes: ## L2 β€” Internal system branches -Internal or legacy branch refs: +Filtered from `branch_list()` but visible to internals: -- `__schema_apply_lock__` β€” serializes schema migrations; filtered from `branch_list()` but visible to internals. -- `__run__<run-id>` β€” legacy from the pre-v0.4.0 Run state machine (removed in MR-771). These are swept off `__manifest` on the first read-write open by the v2β†’v3 internal-schema migration (MR-770), and `__run__*` is no longer a reserved name. Known limitation: a pre-v0.4.0 graph opened **read-only** still surfaces any stale `__run__*` branch in `branch_list()` until its first read-write open (the migration is write-path-only, like all manifest migrations). +- `__schema_apply_lock__` β€” serializes schema migrations. +- `__run__<run-id>` β€” legacy from the pre-v0.4.0 Run state machine (removed in MR-771). The branch-name guard predicate `is_internal_run_branch` is kept as defense-in-depth so users cannot create a branch matching the legacy prefix; the filter will be removed once production legacy branches are swept (MR-770). ## L2 β€” Recovery audit trail -The five migrated writers (`MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`, `optimize_all_tables`) protect their multi-table commits with a sidecar at `__recovery/{ulid}.json` written before Phase B and deleted after Phase C. The next `Omnigraph::open` (gated on `OpenMode::ReadWrite`) runs the recovery sweep in `crates/omnigraph/src/db/manifest/recovery.rs`: classify per-table state, decide all-or-nothing per sidecar, roll forward / back, record an audit row. +The four migrated writers (`MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`) protect their multi-table commits with a sidecar at `__recovery/{ulid}.json` written before Phase B and deleted after Phase C. The next `Omnigraph::open` (gated on `OpenMode::ReadWrite`) runs the recovery sweep in `crates/omnigraph/src/db/manifest/recovery.rs`: classify per-table state, decide all-or-nothing per sidecar, roll forward / back, record an audit row. Audit rows live in `_graph_commit_recoveries.lance` (sibling to `_graph_commits.lance`) and reference the commit graph by `graph_commit_id`. The linked recovery commit is identified by that same `graph_commit_id`, and `actor_id="omnigraph:recovery"` is stored in `_graph_commit_actors.lance` (joined by `graph_commit_id`) β€” `_graph_commits.lance` itself does not carry the `actor_id` column. To find recoveries for a specific original actor: `omnigraph commit list --filter actor=omnigraph:recovery`, then join to `_graph_commit_recoveries.lance` by `graph_commit_id` to read `recovery_for_actor`. Schema: see `crates/omnigraph/src/db/recovery_audit.rs`. diff --git a/docs/user/constants.md b/docs/user/constants.md index 210155e..8f13555 100644 --- a/docs/user/constants.md +++ b/docs/user/constants.md @@ -4,11 +4,11 @@ |---|---|---| | `MANIFEST_DIR` | `__manifest` | `db/manifest/layout.rs` | | Commit graph dir | `_graph_commits.lance` | `db/commit_graph.rs` | -| Run registry dir (legacy, removed MR-771) | `_graph_runs.lance` | inert post-v0.4.0; bytes remain until a `delete_prefix` primitive lands | -| Run branch prefix (legacy, removed MR-771/MR-770) | `__run__` | swept off `__manifest` by the v2β†’v3 migration; no longer a reserved name | +| Run registry dir (legacy, removed MR-771) | `_graph_runs.lance` | inert post-v0.4.0; reclaimed by MR-770 | +| Run branch prefix (legacy, removed MR-771) | `__run__` | filtered by `is_internal_run_branch` defense-in-depth | | Schema apply lock | `__schema_apply_lock__` | `db/mod.rs` | | Manifest publisher retry budget | `PUBLISHER_RETRY_BUDGET = 5` | `db/manifest/publisher.rs` | -| Internal manifest schema version | `INTERNAL_MANIFEST_SCHEMA_VERSION = 3` | `db/manifest/migrations.rs` | +| Internal manifest schema version | `INTERNAL_MANIFEST_SCHEMA_VERSION = 2` | `db/manifest/migrations.rs` | | Merge stage batch | `MERGE_STAGE_BATCH_ROWS = 8192` | `exec/merge.rs` | | Maintenance concurrency | `OMNIGRAPH_MAINTENANCE_CONCURRENCY=8` | `db/omnigraph/optimize.rs` | | Lance blob compaction support | `LANCE_SUPPORTS_BLOB_COMPACTION = false` | `db/omnigraph/optimize.rs` | diff --git a/docs/user/maintenance.md b/docs/user/maintenance.md index a835799..3628fa0 100644 --- a/docs/user/maintenance.md +++ b/docs/user/maintenance.md @@ -4,10 +4,8 @@ ## `optimize_all_tables(db)` β€” non-destructive -- Lance `compact_files()` on every node + edge table on `main`, then **publishes the compacted version to the `__manifest`** so the manifest's `table_version` tracks the compacted Lance HEAD. Reads pin the manifest version, so without this publish compaction would be invisible to readers *and* would break the HEAD-vs-manifest precondition of the next schema apply / strict update/delete ("stale view … refresh and retry"). The publish advances the graph version (a system-attributed commit) only for tables that actually compacted. -- Rewrites small fragments into fewer large ones; old fragments remain reachable via older manifests until `cleanup` runs. -- Each table's compactβ†’publish runs under its per-`(table, main)` write queue (serializing with concurrent mutations β€” compaction is a Lance `Rewrite` op that retryable-conflicts with a concurrent merge/update/delete on overlapping fragments). The Lance-HEAD-before-manifest-publish gap is covered by a `SidecarKind::Optimize` recovery sidecar (loose-match): a crash in that window rolls the compacted version forward on the next `Omnigraph::open` (compaction is content-preserving, so roll-forward is always safe). -- **Requires a recovered graph.** `optimize` refuses (errors) when an unresolved recovery sidecar is present under `__recovery` β€” operating on an unrecovered graph could publish a partial write the open-time recovery sweep would roll back. Reopen the graph to run the recovery sweep, then re-run `optimize`. (Recovery roll-back now publishes its restored version, so a recovered graph always satisfies `manifest == Lance HEAD` going in; there is no leftover drift for `optimize` to interpret.) +- Lance `compact_files()` on every node + edge table on `main`. +- Rewrites small fragments into fewer large ones; old fragments remain reachable via older manifests. - Bounded by `OMNIGRAPH_MAINTENANCE_CONCURRENCY` (default 8). - Returns `[TableOptimizeStats { table_key, fragments_removed, fragments_added, committed, skipped }]`. - **Blob tables are skipped.** A table that declares any `Blob` property is not compacted: it is reported with `skipped: Some(BlobColumnsUnsupportedByLance)` (and logged via `tracing::warn`) instead of compacted, and the rest of the sweep proceeds normally. The current Lance `compact_files` mis-decodes blob-v2 columns under its forced `BlobHandling::AllBinary` read; **reads and writes are unaffected** β€” only compaction is. This is gated by `LANCE_SUPPORTS_BLOB_COMPACTION` (`db/omnigraph/optimize.rs`) and removed when the upstream Lance fix lands (see [docs/dev/lance.md](../dev/lance.md)). Consequence: fragment count and deleted-row space on blob tables are not reclaimed until then; query results are never affected. diff --git a/docs/user/storage.md b/docs/user/storage.md index 2c57a92..c22d4d6 100644 --- a/docs/user/storage.md +++ b/docs/user/storage.md @@ -22,7 +22,7 @@ OmniGraph is **not** a single Lance dataset; it is a *graph* of datasets coordin - `edges/{fnv1a64-hex(edge_type_name)}` β€” one Lance dataset per edge type - `__manifest/` β€” the catalog of all sub-tables and their published versions - `_graph_commits.lance` / `_graph_commit_actors.lance` β€” the commit graph and its actor map - - (legacy `_graph_runs.lance` / `_graph_run_actors.lance` from pre-v0.4.0 graphs are inert; the run state machine was removed in MR-771. The v2β†’v3 manifest migration sweeps stale `__run__*` branches on first write-open; the inert dataset bytes themselves remain until a `delete_prefix` storage primitive lands) + - (legacy `_graph_runs.lance` / `_graph_run_actors.lance` from pre-v0.4.0 graphs are inert; the run state machine was removed in MR-771 and these files are cleaned up via MR-770's production sweep) - **Manifest row schema** (`object_id, object_type, location, metadata, base_objects, table_key, table_version, table_branch, row_count`): - `object_type` ∈ `table | table_version | table_tombstone` - `table_key` ∈ `node:<TypeName> | edge:<EdgeName>` @@ -47,7 +47,6 @@ Adding a new on-disk shape change is one constant bump (`INTERNAL_MANIFEST_SCHEM |---|---| | v1 (implicit, pre-stamp) | `__manifest.object_id` had no PK annotation; publisher had no row-level CAS protection. | | v2 | `__manifest.object_id` carries `lance-schema:unenforced-primary-key=true`; row-level CAS engaged. Stamped as `omnigraph:internal_schema_version=2`. | -| v3 | One-time sweep of legacy `__run__*` staging branches (pre-v0.4.0 Run state machine, removed MR-771) off `__manifest`. Runs at `Omnigraph::open(ReadWrite)` and on publish. Stamped as `omnigraph:internal_schema_version=3`. | ## On-disk layout @@ -92,9 +91,9 @@ flowchart TB - **Graph root** is one directory (or S3 prefix). Everything below is part of one OmniGraph graph. - **`__manifest/`** is a Lance dataset whose rows describe which sub-table version is published at which graph-branch. Reading a snapshot starts here. - **`nodes/`** and **`edges/`** are sibling directories holding one Lance dataset per declared type. Names are `fnv1a64-hex` of the type name to keep paths fixed-length and case-safe. -- **`_graph_commits.lance`** is an L2 dataset that records the graph-level commit DAG, with a paired `_graph_commit_actors.lance` for the actor map. (Pre-v0.4.0 graphs also have inert `_graph_runs.lance` / `_graph_run_actors.lance` from the removed Run state machine; the v2β†’v3 migration sweeps their stale `__run__*` branches, and the dataset bytes are reclaimed once `delete_prefix` lands.) +- **`_graph_commits.lance`** is an L2 dataset that records the graph-level commit DAG, with a paired `_graph_commit_actors.lance` for the actor map. (Pre-v0.4.0 graphs also have inert `_graph_runs.lance` / `_graph_run_actors.lance` from the removed Run state machine; MR-770 sweeps these in production.) - **`_graph_commit_recoveries.lance`** β€” one row per recovery sweep action. Joined to `_graph_commits.lance` by `graph_commit_id`; the linked commit row carries `actor_id=omnigraph:recovery`. Operators correlate recoveries with the original mutations they rolled forward / back via this join. See `crates/omnigraph/src/db/recovery_audit.rs`. -- **`__recovery/{ulid}.json`** β€” transient sidecar files written by the five migrated writers (`MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`, `optimize_all_tables`) before Phase B begins, deleted after Phase C succeeds. A sidecar persisting after process exit means the writer crashed in the Phase B β†’ Phase C window; the next `Omnigraph::open` recovery sweep processes it. Steady-state directory is empty. See `crates/omnigraph/src/db/manifest/recovery.rs`. +- **`__recovery/{ulid}.json`** β€” transient sidecar files written by the four migrated writers (`MutationStaging::finalize`, `schema_apply`, `branch_merge`, `ensure_indices`) before Phase B begins, deleted after Phase C succeeds. A sidecar persisting after process exit means the writer crashed in the Phase B β†’ Phase C window; the next `Omnigraph::open` recovery sweep processes it. Steady-state directory is empty. See `crates/omnigraph/src/db/manifest/recovery.rs`. - **`_refs/branches/{name}.json`** is graph-level branch metadata β€” pointers from a branch name to the manifest version it heads. - **Inside each Lance dataset** (orange): the standard Lance directory layout. `_versions/{n}.manifest` records every commit; `data/` holds the actual Arrow fragments; `_indices/{uuid}/` holds index segments with their own `fragment_bitmap` for partial coverage; `_refs/` holds Lance-native per-dataset branches and tags. diff --git a/scripts/check-agents-md.sh b/scripts/check-agents-md.sh index 02a177a..abc6469 100755 --- a/scripts/check-agents-md.sh +++ b/scripts/check-agents-md.sh @@ -34,15 +34,10 @@ PY canonical=() while IFS= read -r line; do canonical+=("$line") -done < <(find docs -type f -name '*.md' ! -path 'docs/releases/*' ! -path 'docs/internal/*' ! -path 'docs/rfcs/*' | sort) +done < <(find docs -type f -name '*.md' ! -path 'docs/releases/*' ! -path 'docs/internal/*' | sort) if [[ -d docs/releases ]]; then canonical+=("docs/releases/") fi -# RFCs are a growing collection (like releases): represent the directory, not -# every per-RFC file. The dir must be linked from an audience index. -if [[ -d docs/rfcs ]]; then - canonical+=("docs/rfcs/") -fi linked=() for index_file in "${index_files[@]}"; do