From 96dbe9dec00b41b68907708d7535437677d3fde7 Mon Sep 17 00:00:00 2001 From: Andrew Altshuler Date: Sat, 6 Jun 2026 00:44:48 +0300 Subject: [PATCH 01/10] fix(release): make Homebrew audit non-blocking + set up brew on runner (#140) The v0.6.1 Release shipped binaries but the Homebrew tap update job died at the audit step (brew not on the ubuntu runner; exit 127), skipping the formula push so the tap stayed at 0.6.0. - Install Homebrew via Homebrew/actions/setup-homebrew so brew is available. - Make both the setup and audit steps continue-on-error: they are best-effort diagnostics (the formula is correct by construction via update-homebrew-formula.sh), so neither can skip the actual tap publish. - Drop --online from brew audit for deterministic, network-independent linting. --- .github/workflows/release.yml | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 3a66ff2..a265c40 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -121,16 +121,30 @@ 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. + # tree so `modernrelay/tap/omnigraph` resolves to it. Offline audit + # (no --online) keeps it deterministic; it still catches the + # ComponentsOrder/structure class of problems. tap_dir="$(brew --repository)/Library/Taps/modernrelay/homebrew-tap" mkdir -p "$(dirname "$tap_dir")" ln -sfn "$PWD/homebrew-tap" "$tap_dir" - brew audit --strict --online modernrelay/tap/omnigraph + brew audit --strict modernrelay/tap/omnigraph - name: Commit and push formula update if: env.HOMEBREW_TAP_SKIP != '1' From c7365bf8efd4500d6af16b00eec34d4c2202ca2b Mon Sep 17 00:00:00 2001 From: Andrew Altshuler Date: Sat, 6 Jun 2026 18:09:47 +0300 Subject: [PATCH 02/10] ci(codeowners): un-trap required checks, auto-render, generate owner tables (#142) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CODEOWNERS required checks blocked every PR — the real root cause was a name mismatch, compounded by a path filter: - branch-protection.json required the contexts `CODEOWNERS / drift` and `CODEOWNERS / noedit` (the GitHub UI "workflow / job-id" display form), but the jobs report check-run names from their `name:` fields — "CODEOWNERS matches source" / "CODEOWNERS not hand-edited". The required contexts therefore never matched any reported check and sat permanently pending. - The workflow was also path-filtered to CODEOWNERS files, so it didn't even run for most PRs. Net effect: with both required checks unsatisfiable, every PR could only land via admin override (e.g. #140). Fixes: - A: drop the `paths:` filter so the workflow runs on every PR and both required contexts always report. - name fix: point branch-protection.json at the actual job names verbatim, and add a doc note that the contexts must equal the job `name:` values. - B: the `drift` job now re-renders and, on same-repo PRs, auto-commits the regenerated artifacts back to the branch (mirrors the openapi.json job in ci.yml); forks / manual runs strict-check instead. Contributors no longer run the script by hand. - D: render-codeowners.py also generates a "who owns what" path->owners + roles table spliced into docs/dev/codeowners.md between markers, so the human-readable view never drifts. Idempotent; CODEOWNERS output unchanged. - docs: correct the stale `enforce_admins: true` line (JSON and live are false). NOTE: the branch-protection.json change only takes effect after an admin runs `./scripts/apply-branch-protection.sh` (deliberate manual step, per docs/dev/branch-protection.md). Until then `main` still requires the old mismatched contexts, so this PR itself needs an admin-override merge — the last one that should be necessary. Co-authored-by: Claude Opus 4.8 (1M context) --- .github/branch-protection.json | 4 +- .github/scripts/render-codeowners.py | 81 ++++++++++++++++++++++++++-- .github/workflows/codeowners.yml | 72 ++++++++++++++++++++----- docs/dev/branch-protection.md | 4 +- docs/dev/codeowners.md | 39 ++++++++++---- 5 files changed, 168 insertions(+), 32 deletions(-) diff --git a/.github/branch-protection.json b/.github/branch-protection.json index 61b7d33..7ca46b9 100644 --- a/.github/branch-protection.json +++ b/.github/branch-protection.json @@ -7,8 +7,8 @@ "Check AGENTS.md Links", "Test Workspace", "Test omnigraph-server --features aws", - "CODEOWNERS / drift", - "CODEOWNERS / noedit" + "CODEOWNERS matches source", + "CODEOWNERS not hand-edited" ] }, "enforce_admins": false, diff --git a/.github/scripts/render-codeowners.py b/.github/scripts/render-codeowners.py index f243d0c..5e96545 100755 --- a/.github/scripts/render-codeowners.py +++ b/.github/scripts/render-codeowners.py @@ -1,10 +1,14 @@ #!/usr/bin/env python3 -"""Render .github/CODEOWNERS from .github/codeowners-roles.yml. +"""Render .github/CODEOWNERS and the ownership tables in +docs/dev/codeowners.md from .github/codeowners-roles.yml. -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. +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. Usage: python3 .github/scripts/render-codeowners.py @@ -16,6 +20,7 @@ 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 @@ -34,6 +39,13 @@ 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. @@ -75,6 +87,62 @@ 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}") @@ -127,6 +195,9 @@ 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/codeowners.yml b/.github/workflows/codeowners.yml index 19d5835..75b3515 100644 --- a/.github/workflows/codeowners.yml +++ b/.github/workflows/codeowners.yml @@ -1,19 +1,24 @@ 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: -# Read-only; we never push from this workflow. +# `drift` auto-commits the regenerated artifacts back to same-repo PR +# branches, so it needs write access. permissions: - contents: read + contents: write 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 @@ -28,19 +33,56 @@ jobs: - name: Install PyYAML run: pip install pyyaml - - name: Re-render CODEOWNERS + - name: Re-render CODEOWNERS + ownership docs run: python3 .github/scripts/render-codeowners.py - - name: Reject drift + # 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 }} run: | - 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." + 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." echo "--- diff ---" - git --no-pager diff .github/CODEOWNERS + git --no-pager diff -- .github/CODEOWNERS docs/dev/codeowners.md exit 1 fi - echo "CODEOWNERS is in sync with its source." + echo "Generated artifacts are in sync with their source." noedit: name: CODEOWNERS not hand-edited @@ -52,6 +94,8 @@ 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/docs/dev/branch-protection.md b/docs/dev/branch-protection.md index 9b2fa78..2b6cc37 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 / 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 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 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** | `true` | Even repository admins go through the gates. The point is no bypasses. | +| **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. | | **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 9a7fb50..14bba0b 100644 --- a/docs/dev/codeowners.md +++ b/docs/dev/codeowners.md @@ -4,24 +4,45 @@ This setup gives every role change a reviewable PR and a permanent in-repository audit trail (`git log .github/codeowners-roles.yml`). -## Current roles +## Who owns what -| Role | Members | Scope | +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) | |---|---|---| -| `engineering` | `@ragnorc` | All code under `crates/**`, repository infrastructure, default for unmapped paths | -| `docs` | `@ragnorc` | `docs/**`, README.md, AGENTS.md, CLAUDE.md, SECURITY.md | +| `*` | @ragnorc | engineering | +| `crates/**` | @ragnorc | engineering | +| `docs/**` | @ragnorc | docs | +| `README.md` | @ragnorc | docs | +| `AGENTS.md` | @ragnorc | docs | +| `CLAUDE.md` | @ragnorc | docs | +| `SECURITY.md` | @ragnorc | docs | -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). +**Roles**: + +| Role | Members | Description | +|---|---|---| +| `engineering` | @ragnorc | 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). ## How to change role membership or path mappings 1. Edit `.github/codeowners-roles.yml`. -2. Run `python3 .github/scripts/render-codeowners.py` (requires PyYAML; `pip install pyyaml`). -3. Commit both files in the same PR. +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. CI fails the PR if: -- `CODEOWNERS` was edited without a corresponding yml change, or -- The yml was changed but the rendered `CODEOWNERS` doesn't match. +- 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). ## How to add a new role From 343f1f17ed8e86032aef6d9a466a778a9c39b6bd Mon Sep 17 00:00:00 2001 From: Andrew Altshuler Date: Sat, 6 Jun 2026 23:58:08 +0300 Subject: [PATCH 03/10] governance: external contribution model (issues/discussions/RFCs/PRs) (#143) Formalize the public contribution surface. Maintainers keep a separate internal process and are exempt from the intake gates; everyone stays bound by review, CODEOWNERS, and branch protection. Model: - Issues = problem reports only (bug form + config.yml redirects ideas to Discussions and disables blank issues). - Discussions = ideas + RFC incubation. - RFCs = anyone (incl. external) authors docs/rfcs/NNNN-*.md; a maintainer merging it is acceptance. Distinct from the maintainer-internal docs/dev/rfc-00N-* track. - PRs = link an `accepted` issue or accepted RFC, or use the trivial fast-lane (typos/docs/deps). Enforced softly to start (template + review). Adds GOVERNANCE.md, rewrites CONTRIBUTING.md, adds docs/rfcs/ (README + template), .github issue/PR/discussion templates. Wires docs/rfcs/ into the doc-link checker (excluded like releases; linked from docs/dev/index.md). Co-authored-by: Claude Opus 4.8 (1M context) --- .github/DISCUSSION_TEMPLATE/rfc.yml | 34 +++++++++ .github/ISSUE_TEMPLATE/bug_report.yml | 55 +++++++++++++ .github/ISSUE_TEMPLATE/config.yml | 13 ++++ .github/PULL_REQUEST_TEMPLATE.md | 29 +++++++ CONTRIBUTING.md | 38 +++++++-- GOVERNANCE.md | 106 ++++++++++++++++++++++++++ docs/dev/index.md | 12 +++ docs/rfcs/0000-template.md | 54 +++++++++++++ docs/rfcs/README.md | 66 ++++++++++++++++ scripts/check-agents-md.sh | 7 +- 10 files changed, 406 insertions(+), 8 deletions(-) create mode 100644 .github/DISCUSSION_TEMPLATE/rfc.yml create mode 100644 .github/ISSUE_TEMPLATE/bug_report.yml create mode 100644 .github/ISSUE_TEMPLATE/config.yml create mode 100644 .github/PULL_REQUEST_TEMPLATE.md create mode 100644 GOVERNANCE.md create mode 100644 docs/rfcs/0000-template.md create mode 100644 docs/rfcs/README.md diff --git a/.github/DISCUSSION_TEMPLATE/rfc.yml b/.github/DISCUSSION_TEMPLATE/rfc.yml new file mode 100644 index 0000000..2a63525 --- /dev/null +++ b/.github/DISCUSSION_TEMPLATE/rfc.yml @@ -0,0 +1,34 @@ +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 new file mode 100644 index 0000000..8e19465 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -0,0 +1,55 @@ +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 new file mode 100644 index 0000000..50720b8 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1,13 @@ +# 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 new file mode 100644 index 0000000..2a548c7 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,29 @@ + + +## 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/CONTRIBUTING.md b/CONTRIBUTING.md index 8d9c687..2d77ef0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,10 +1,29 @@ # Contributing -Small bug fixes and documentation improvements are welcome directly through pull -requests. +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). -For larger changes, please open an issue or design discussion first so the -proposed direction is clear before implementation starts. +## 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. ## Development @@ -49,6 +68,11 @@ CI runs both. ## Pull Requests -- keep changes focused -- include tests for behavior changes when practical -- update public docs when the user-facing surface changes +- **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. diff --git a/GOVERNANCE.md b/GOVERNANCE.md new file mode 100644 index 0000000..5878f1f --- /dev/null +++ b/GOVERNANCE.md @@ -0,0 +1,106 @@ +# 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/docs/dev/index.md b/docs/dev/index.md index 600c969..1e41342 100644 --- a/docs/dev/index.md +++ b/docs/dev/index.md @@ -51,6 +51,18 @@ 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/rfcs/0000-template.md b/docs/rfcs/0000-template.md new file mode 100644 index 0000000..48f4bda --- /dev/null +++ b/docs/rfcs/0000-template.md @@ -0,0 +1,54 @@ +# 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 new file mode 100644 index 0000000..99cdd76 --- /dev/null +++ b/docs/rfcs/README.md @@ -0,0 +1,66 @@ +# 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/scripts/check-agents-md.sh b/scripts/check-agents-md.sh index abc6469..02a177a 100755 --- a/scripts/check-agents-md.sh +++ b/scripts/check-agents-md.sh @@ -34,10 +34,15 @@ PY canonical=() while IFS= read -r line; do canonical+=("$line") -done < <(find docs -type f -name '*.md' ! -path 'docs/releases/*' ! -path 'docs/internal/*' | sort) +done < <(find docs -type f -name '*.md' ! -path 'docs/releases/*' ! -path 'docs/internal/*' ! -path 'docs/rfcs/*' | 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 From fd8e078a77fcce8be31b3ec3c18614427555b6fe Mon Sep 17 00:00:00 2001 From: Andrew Altshuler <andrew@collectivelab.io> Date: Sun, 7 Jun 2026 18:05:01 +0300 Subject: [PATCH 04/10] ci(codeowners): add aaltshuler to engineering role (#147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restores aaltshuler as an `engineering` code-owner (removed in #142), so `crates/**` and repo-infra PRs have a second reviewer besides the sole owner ragnorc — unblocking review of author-ragnorc PRs (e.g. #132) that ragnorc cannot self-approve. Edited the source of truth (.github/codeowners-roles.yml) and re-rendered .github/CODEOWNERS + the docs/dev/codeowners.md tables via .github/scripts/render-codeowners.py, per the documented flow. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- .github/CODEOWNERS | 4 ++-- .github/codeowners-roles.yml | 1 + docs/dev/codeowners.md | 6 +++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index d4ecfa5..e937724 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 +* @ragnorc @aaltshuler -crates/** @ragnorc +crates/** @ragnorc @aaltshuler docs/** @ragnorc README.md @ragnorc AGENTS.md @ragnorc diff --git a/.github/codeowners-roles.yml b/.github/codeowners-roles.yml index c5e36a9..ce4014d 100644 --- a/.github/codeowners-roles.yml +++ b/.github/codeowners-roles.yml @@ -22,6 +22,7 @@ roles: compiler. members: - ragnorc + - aaltshuler docs: description: > diff --git a/docs/dev/codeowners.md b/docs/dev/codeowners.md index 14bba0b..50c4dc7 100644 --- a/docs/dev/codeowners.md +++ b/docs/dev/codeowners.md @@ -14,8 +14,8 @@ The tables below are **generated** from `.github/codeowners-roles.yml` by `.gith | Path | Owners | Role(s) | |---|---|---| -| `*` | @ragnorc | engineering | -| `crates/**` | @ragnorc | engineering | +| `*` | @ragnorc @aaltshuler | engineering | +| `crates/**` | @ragnorc @aaltshuler | engineering | | `docs/**` | @ragnorc | docs | | `README.md` | @ragnorc | docs | | `AGENTS.md` | @ragnorc | docs | @@ -26,7 +26,7 @@ The tables below are **generated** from `.github/codeowners-roles.yml` by `.gith | Role | Members | Description | |---|---|---| -| `engineering` | @ragnorc | All production code under crates/**. Engine, CLI, server, compiler. | +| `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). | <!-- END GENERATED OWNERSHIP --> From 54842808dbd981e61e0a4be2cf987fc0a52b2584 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford <ragnor.comerford@gmail.com> Date: Sun, 7 Jun 2026 17:33:14 +0200 Subject: [PATCH 05/10] feat(engine): sweep & remove legacy __run__ branch guard (MR-770) (#132) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(engine): sweep legacy __run__ branches via v2→v3 manifest migration Pre-v0.4.0 graphs can carry stale `__run__<id>` staging branches on the `__manifest` dataset, left by the Run state machine removed in MR-771. Lance's `list_branches` still enumerates them, so they leak into `branch_list()` and count as blocking branches at schema-apply time. Add a one-time `migrate_v2_to_v3` arm to the internal-schema dispatcher: on the first read-write open it enumerates `__manifest` branches, deletes every `__run__*` ref, and bumps the stamp to 3. Idempotent under retry (re-enumerates fresh each run). The `"__run__"` prefix is inlined so the migration does not depend on the run_registry guard that MR-770 removes next. This is the prerequisite sweep; the guard removal follows in the next commit. * refactor(engine): remove the legacy __run__ branch guard (MR-770) With the v2→v3 migration sweeping stale `__run__*` branches off `__manifest` on first read-write open, the defense-in-depth `is_internal_run_branch` guard is no longer needed. - delete `db/run_registry.rs`; drop the module + re-export from `db/mod.rs` - collapse `is_internal_system_branch` to the schema-apply-lock check only - `ensure_public_branch_ref`: drop the run-ref rejection; `__run__*` is now an ordinary branch name - `branch_merge`: reject `is_internal_system_branch` (was run-only) so the schema-apply lock is rejected consistently with create/delete — a small, deliberate tightening - update the inline schema-apply test + the writes integration tests (`public_branch_apis_reject_internal_run_refs` → `public_branch_apis_reject_internal_system_refs`, which also asserts `__run__*` now creates successfully) - docs: flip the "pending production sweep / defense-in-depth" notes to "auto-swept by the v2→v3 migration"; document the read-only-open limitation Known residual: the inert `_graph_runs.lance` / `_graph_run_actors.lance` bytes remain until a `StorageAdapter::delete_prefix` primitive lands. * fix(engine): run __run__ sweep at Omnigraph::open, not only on publish Review (PR #132) caught a regression: removing __run__ from `is_internal_system_branch` exposed legacy `__run__*` branches to the schema-apply blocking-branch checks (schema_apply.rs:104 and :778) and to `branch_list()`, but the v2→v3 sweep ran only inside the publisher's `load_publish_state`. On a pre-v0.4.0 graph whose first write is a schema apply, the blocking-branch check fires before any publish, so apply failed with "found non-main branches: __run__…". The same lazy timing also created a reverse hazard: a user-created `__run__*` branch on a still-v2 graph could be deleted by the first publish's sweep. Fix: run the internal-schema migration in `Omnigraph::open(ReadWrite)` (new `manifest::migrate_on_open`), before the coordinator reads branch state. The sweep now lands before any branch-observing code, and a graph is stamped v3 at open — so the one-time sweep can never catch a legitimately-created branch. Both checks and `branch_list` see the swept graph; correct by construction for every write path. Accepted residual: a read-only open of an unmigrated legacy graph still lists `__run__*` (read-only opens must not write, so they can't sweep). Documented. Regression test `legacy_run_branch_is_swept_on_open_and_does_not_block_schema_apply` confirmed RED before the fix (panicked on the branch_list leak assertion) and GREEN after. Also updates the stale schema_apply.rs comment, the writes.md "Migration code" section, and adds the v3 row to storage.md's migration table. * test(engine): sweep multiple legacy __run__ branches; doc nit Strengthen the v2→v3 migration test to synthesize three `__run__*` branches (a real legacy graph accumulates one per run) so the migration's delete loop is exercised on a single reused dataset handle, not just a single branch. Confirms multi-branch deletion is safe. Also drop a stale "active runs" reference from the branch_delete doc line. * fix(engine): force-delete in __run__ sweep for concurrency safety `migrate_v2_to_v3` ran `Dataset::delete_branch` (= `branches().delete(.., false)`), which errors "BranchContents not found" if the branch is already gone. Since the sweep now runs in `Omnigraph::open(ReadWrite)`, two processes opening the same legacy v2 graph concurrently would race: one wins each delete, the other's open fails. The migration only claimed idempotency under *sequential* retry. Switch to `Dataset::force_delete_branch` (= `delete(.., true)`), Lance's documented path for cleaning up zombie branches, which tolerates an already-absent branch. The sweep is now idempotent under concurrent runners and robust to partial/zombie state. Found in self-review; no behavior change for the common single-open path. * docs(release): note MR-770 __run__ cleanup in v0.6.1 * docs(branches): reconcile branch cleanup semantics --- crates/omnigraph/src/db/manifest.rs | 16 ++++ .../omnigraph/src/db/manifest/migrations.rs | 55 ++++++++++++- crates/omnigraph/src/db/manifest/tests.rs | 74 +++++++++++++++++ crates/omnigraph/src/db/mod.rs | 7 +- crates/omnigraph/src/db/omnigraph.rs | 81 +++++++++++++++---- .../src/db/omnigraph/schema_apply.rs | 10 +-- crates/omnigraph/src/db/run_registry.rs | 16 ---- crates/omnigraph/src/exec/merge.rs | 4 +- crates/omnigraph/src/exec/mod.rs | 2 +- crates/omnigraph/tests/writes.rs | 33 ++++---- docs/dev/writes.md | 18 +++-- docs/releases/v0.6.1.md | 2 + docs/user/audit.md | 2 +- docs/user/branches-commits.md | 10 +-- docs/user/constants.md | 6 +- docs/user/storage.md | 5 +- 16 files changed, 269 insertions(+), 72 deletions(-) delete mode 100644 crates/omnigraph/src/db/run_registry.rs diff --git a/crates/omnigraph/src/db/manifest.rs b/crates/omnigraph/src/db/manifest.rs index 7fcf7de..3b2886f 100644 --- a/crates/omnigraph/src/db/manifest.rs +++ b/crates/omnigraph/src/db/manifest.rs @@ -48,6 +48,22 @@ 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 bbb7995..e2801fe 100644 --- a/crates/omnigraph/src/db/manifest/migrations.rs +++ b/crates/omnigraph/src/db/manifest/migrations.rs @@ -46,7 +46,11 @@ 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`. -pub(super) const INTERNAL_MANIFEST_SCHEMA_VERSION: u32 = 2; +/// - v3 — one-time sweep of legacy `__run__<id>` 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; const INTERNAL_SCHEMA_VERSION_KEY: &str = "omnigraph:internal_schema_version"; const OBJECT_ID_PK_KEY: &str = "lance-schema:unenforced-primary-key"; @@ -89,6 +93,10 @@ 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{}", @@ -122,6 +130,51 @@ async fn migrate_v1_to_v2(dataset: &mut Dataset) -> Result<()> { set_stamp(dataset, 2).await } +/// v2 → v3: sweep legacy `__run__<id>` 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__<ulid>` 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<String> = 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/tests.rs b/crates/omnigraph/src/db/manifest/tests.rs index effa0b5..885a2a8 100644 --- a/crates/omnigraph/src/db/manifest/tests.rs +++ b/crates/omnigraph/src/db/manifest/tests.rs @@ -1461,6 +1461,80 @@ 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 8702f88..13e1c74 100644 --- a/crates/omnigraph/src/db/mod.rs +++ b/crates/omnigraph/src/db/mod.rs @@ -3,7 +3,6 @@ pub mod graph_coordinator; pub mod manifest; mod omnigraph; mod recovery_audit; -mod run_registry; mod schema_state; pub(crate) mod write_queue; @@ -15,7 +14,6 @@ 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__"; @@ -69,5 +67,8 @@ pub(crate) fn is_schema_apply_lock_branch(name: &str) -> bool { } pub(crate) fn is_internal_system_branch(name: &str) -> bool { - is_internal_run_branch(name) || is_schema_apply_lock_branch(name) + // 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) } diff --git a/crates/omnigraph/src/db/omnigraph.rs b/crates/omnigraph/src/db/omnigraph.rs index 7b8a3f6..ba2b70e 100644 --- a/crates/omnigraph/src/db/omnigraph.rs +++ b/crates/omnigraph/src/db/omnigraph.rs @@ -346,6 +346,16 @@ impl Omnigraph { mode: OpenMode, ) -> Result<Self> { 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?; @@ -1491,12 +1501,6 @@ pub(crate) fn normalize_branch_name(branch: &str) -> Result<Option<String>> { } 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 '{}'", @@ -1900,7 +1904,6 @@ fn json_value_from_array(array: &dyn Array, row: usize) -> Result<serde_json::Va #[cfg(test)] mod tests { use super::*; - use crate::db::is_internal_run_branch; use crate::db::manifest::ManifestCoordinator; use async_trait::async_trait; use serde_json::Value; @@ -2238,11 +2241,11 @@ edge WorksAt: Person -> Company #[tokio::test] async fn test_apply_schema_succeeds_after_load() { // Historical: schema apply used to be blocked by leftover - // `__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. + // `__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. let dir = tempfile::tempdir().unwrap(); let uri = dir.path().to_str().unwrap(); let mut db = Omnigraph::init(uri, TEST_SCHEMA).await.unwrap(); @@ -2257,8 +2260,8 @@ edge WorksAt: Person -> Company let all_branches = db.coordinator.read().await.all_branches().await.unwrap(); assert!( - !all_branches.iter().any(|b| is_internal_run_branch(b)), - "run branch should be deleted after publish, got: {:?}", + !all_branches.iter().any(|b| b.starts_with("__run__")), + "no __run__ branch should exist after publish, got: {:?}", all_branches ); @@ -2270,6 +2273,56 @@ 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/schema_apply.rs b/crates/omnigraph/src/db/omnigraph/schema_apply.rs index 35fe161..7cb3193 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<PlannedSchemaApply> { 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 - // 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. + // 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. 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 deleted file mode 100644 index ee3d336..0000000 --- a/crates/omnigraph/src/db/run_registry.rs +++ /dev/null @@ -1,16 +0,0 @@ -// 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__<id>` 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 2e5f32e..eb6c4a3 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<MergeOutcome> { - if is_internal_run_branch(source) || is_internal_run_branch(target) { + if is_internal_system_branch(source) || is_internal_system_branch(target) { return Err(OmniError::manifest(format!( - "branch_merge does not allow internal run refs ('{}' -> '{}')", + "branch_merge does not allow internal system refs ('{}' -> '{}')", source, target ))); } diff --git a/crates/omnigraph/src/exec/mod.rs b/crates/omnigraph/src/exec/mod.rs index 33a7e41..ce72d42 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_run_branch}; +use crate::db::{MergeOutcome, Omnigraph, is_internal_system_branch}; use crate::db::{ReadTarget, Snapshot}; use crate::embedding::EmbeddingClient; use crate::error::{MergeConflict, MergeConflictKind, OmniError, Result}; diff --git a/crates/omnigraph/tests/writes.rs b/crates/omnigraph/tests/writes.rs index 13cb10f..0a309c9 100644 --- a/crates/omnigraph/tests/writes.rs +++ b/crates/omnigraph/tests/writes.rs @@ -371,11 +371,10 @@ async fn cancelled_mutation_future_leaves_no_state() { // Cancel-safety property: no graph-level run/staging state remains. // - // 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). + // 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. // // (1) The branch list is unchanged: cancellation/completion cannot // synthesize new public branches. @@ -442,34 +441,40 @@ async fn repeated_loads_do_not_accumulate_branches() { assert_eq!(db.branch_list().await.unwrap(), vec!["main".to_string()]); } -/// 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. +/// 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. #[tokio::test] -async fn public_branch_apis_reject_internal_run_refs() { +async fn public_branch_apis_reject_internal_system_refs() { let dir = tempfile::tempdir().unwrap(); let mut db = init_and_load(&dir).await; - let create_err = db.branch_create("__run__synthetic").await.unwrap_err(); + // `__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 OmniError::Manifest(err) = create_err else { panic!("expected Manifest error"); }; assert!( - err.message.contains("internal run ref"), + err.message.contains("internal system ref"), "unexpected error: {}", err.message ); let merge_err = db - .branch_merge("__run__synthetic", "main") + .branch_merge("__schema_apply_lock__", "main") .await .unwrap_err(); let OmniError::Manifest(err) = merge_err else { panic!("expected Manifest error"); }; assert!( - err.message.contains("internal run refs"), + err.message.contains("internal system refs"), "unexpected error: {}", err.message ); diff --git a/docs/dev/writes.md b/docs/dev/writes.md index 974f7a6..8b692b4 100644 --- a/docs/dev/writes.md +++ b/docs/dev/writes.md @@ -14,8 +14,11 @@ 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__<id>` staging branches. (Legacy on-disk artifacts from - pre-MR-771 repos are inert; MR-770 sweeps them in production.) +- No `__run__<id>` 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.) - Cancelled mutation futures leave **no graph-level state** — only orphaned Lance fragments, which the existing `omnigraph cleanup` pipe reclaims. @@ -245,9 +248,14 @@ list`. ## Migration code -`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. +`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. ## 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 aafe1af..0acc34b 100644 --- a/docs/releases/v0.6.1.md +++ b/docs/releases/v0.6.1.md @@ -7,6 +7,7 @@ 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. @@ -17,6 +18,7 @@ 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.<name>.policy` and `graphs.<name>.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<SkipReason>` 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/user/audit.md b/docs/user/audit.md index e8abe5b..ab028ac 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 and reclaimed by MR-770's production sweep. +- 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. diff --git a/docs/user/branches-commits.md b/docs/user/branches-commits.md index c1894f9..0565186 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 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). +- `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). - **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,10 +51,10 @@ Notes: ## L2 — Internal system branches -Filtered from `branch_list()` but visible to internals: +Internal or legacy branch refs: -- `__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). +- `__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). ## L2 — Recovery audit trail diff --git a/docs/user/constants.md b/docs/user/constants.md index 8f13555..210155e 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; reclaimed by MR-770 | -| Run branch prefix (legacy, removed MR-771) | `__run__` | filtered by `is_internal_run_branch` defense-in-depth | +| 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 | | 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 = 2` | `db/manifest/migrations.rs` | +| Internal manifest schema version | `INTERNAL_MANIFEST_SCHEMA_VERSION = 3` | `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/storage.md b/docs/user/storage.md index c22d4d6..d1c52b5 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 and these files are cleaned up via MR-770's production sweep) + - (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) - **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,6 +47,7 @@ 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 @@ -91,7 +92,7 @@ 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; MR-770 sweeps these in production.) +- **`_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_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 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. From 4a66d6e071ce95eabe49cd496356fba0617901be Mon Sep 17 00:00:00 2001 From: Aaron Goh <aaronwgoh5@gmail.com> Date: Sun, 7 Jun 2026 20:37:37 +0200 Subject: [PATCH 06/10] fix(loader): accept multi-line (pretty-printed) JSON in load (#146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The loader read input line-by-line (reader.lines() + serde_json::from_str per line), so any delta where a JSON object spanned multiple lines failed with 'invalid JSON on line 1: EOF while parsing an object'. Compact JSONL worked; pretty-printed JSON never did. Switch to a streaming value deserializer (Deserializer::from_reader().into_iter::<Value>()), which treats any whitespace (including newlines inside objects) as a separator — so both compact JSONL and pretty-printed JSON load. Error labels switch from line numbers to record numbers (line numbers are meaningless once objects span lines). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Ragnor Comerford <ragnor.comerford@gmail.com> --- crates/omnigraph/src/loader/mod.rs | 35 ++++++++++++++++-------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/crates/omnigraph/src/loader/mod.rs b/crates/omnigraph/src/loader/mod.rs index 46a46e2..d5d74c0 100644 --- a/crates/omnigraph/src/loader/mod.rs +++ b/crates/omnigraph/src/loader/mod.rs @@ -288,21 +288,24 @@ async fn load_jsonl_reader<R: BufRead>( let mut node_rows: HashMap<String, Vec<JsonValue>> = HashMap::new(); let mut edge_rows: HashMap<String, Vec<(String, String, JsonValue)>> = HashMap::new(); - 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)) + // 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::<JsonValue>() + .enumerate() + { + let record_num = idx + 1; + let value: JsonValue = parsed.map_err(|e| { + OmniError::manifest(format!("invalid JSON at record {}: {}", record_num, 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!( - "line {}: unknown node type '{}'", - line_num + 1, + "record {}: unknown node type '{}'", + record_num, type_name ))); } @@ -317,8 +320,8 @@ async fn load_jsonl_reader<R: BufRead>( } 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!( - "line {}: unknown edge type '{}'", - line_num + 1, + "record {}: unknown edge type '{}'", + record_num, edge_name ))); } @@ -326,14 +329,14 @@ async fn load_jsonl_reader<R: BufRead>( .get("from") .and_then(|v| v.as_str()) .ok_or_else(|| { - OmniError::manifest(format!("line {}: edge missing 'from'", line_num + 1)) + OmniError::manifest(format!("record {}: edge missing 'from'", record_num)) })? .to_string(); let to = value .get("to") .and_then(|v| v.as_str()) .ok_or_else(|| { - OmniError::manifest(format!("line {}: edge missing 'to'", line_num + 1)) + OmniError::manifest(format!("record {}: edge missing 'to'", record_num)) })? .to_string(); let data = value @@ -347,8 +350,8 @@ async fn load_jsonl_reader<R: BufRead>( .push((from, to, data)); } else { return Err(OmniError::manifest(format!( - "line {}: expected 'type' or 'edge' field", - line_num + 1 + "record {}: expected 'type' or 'edge' field", + record_num ))); } } From e62d9166fb39d0b309d1c345928f93748b5ea176 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford <ragnor.comerford@gmail.com> Date: Mon, 8 Jun 2026 01:50:12 +0200 Subject: [PATCH 07/10] fix: optimize publishes compaction; recovery roll-back converges manifest (#141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(optimize): cover manifest publish + HEAD-drift reconcile Red against the pre-fix optimize, which ran compact_files without publishing the compacted version to __manifest: - maintenance: optimize must publish so the manifest table_version tracks the compacted Lance HEAD and a later schema apply succeeds; and must reconcile a pre-existing manifest-behind-HEAD drift (forged via raw Lance compaction) so strict writes commit again. - end_to_end + composite_flow: post-optimize query / strict update / reopen in the full lifecycle (the canonical flow previously omitted post-optimize writes as a documented "known limitation"). - failpoints: a crash between compaction and the manifest publish rolls forward on next open. * fix(optimize): publish compaction to manifest and reconcile HEAD drift optimize ran Lance compact_files without publishing the new version to __manifest, so the manifest table_version lagged the Lance HEAD: reads stayed pinned to the pre-compaction version, and the next schema apply or strict update/delete failed its HEAD-vs-manifest precondition with "stale view ... refresh and retry" (open-time recovery rollback inflated the gap on retry). optimize now publishes each compacted table's version under the per-(table, main) write queue, guarded by a manifest CAS and a SidecarKind::Optimize recovery sidecar (loose-match; roll-forward is safe because compaction is content-preserving). When a table has nothing left to compact but its Lance HEAD is already ahead of the manifest pin (pre-fix drift, or a recovery restore commit), optimize reconciles the manifest forward to HEAD (metadata-only, no sidecar). Caches and the CSR/CSC graph index are invalidated after a publish. Docs updated (maintenance, storage, branches-commits, writes, testing). * test(recovery): rollback convergence + optimize-defer regressions Red against the current code, landed before the fix: - recovery: after the open-time sweep rolls a sidecar back, the manifest must track Lance HEAD (no residual drift) so a follow-up schema apply succeeds — the original "+1 per retry" loop. Today roll-back restores without publishing, so the manifest lags HEAD and the apply fails its HEAD-vs-manifest precondition. - maintenance: optimize must refuse while a recovery sidecar is pending — operating on an unrecovered graph could publish a partial write the sweep would roll back. Also removes optimize_reconciles_preexisting_manifest_head_drift: the ad-hoc drift reconcile it covered is replaced by recovery-side convergence. * fix(recovery): converge manifest on roll-back; optimize defers on pending recovery Root of PR #141's review findings and the original "+1 per retry" loop: a Lance HEAD ahead of the manifest was ambiguous (benign content-preserving drift vs. a partial write a sidecar will roll back), and optimize's reconcile guessed it benign. Close the class instead of guessing: - Recovery roll-back now PUBLISHES the restored version (via a push_table_update_at_head helper shared with roll-forward), so the manifest tracks the Lance HEAD after recovery — symmetric with roll-forward. This fixes the +1 loop (after one roll-back the retry's HEAD-vs-manifest precondition passes) and removes the only remaining source of orphaned drift. The audit still records the logical rolled-back-to version; the manifest is published at the restore commit (identical content). - optimize drops the ad-hoc drift reconcile and instead REFUSES when a __recovery sidecar is pending, so it only ever operates on a recovered graph (manifest == HEAD); its compaction publish can no longer commit a partial write. With the reconcile gone, the blob-skip-vs-reconcile gap is moot. Updates the rollback recovery-test helper (manifest == HEAD after roll-back), the failpoints assertions, and the user/dev docs. * test(recovery): fix rollback assertion for manifest convergence The roll-back-publishes change makes the manifest version advance after a SchemaApply roll-back (to the old-schema content), so the schema_apply_without_schema_staging_rolls_back_on_next_open assertion must be `version > pre`, not `version == pre`. This update was dropped during the commit churn and surfaced as a CI Test Workspace failure; the old-schema-preserved intent stays covered by count_rows + _schema.pg + the RolledBack convergence invariant. --- AGENTS.md | 4 +- crates/omnigraph/src/db/manifest.rs | 2 +- crates/omnigraph/src/db/manifest/recovery.rs | 187 +++++++++----- crates/omnigraph/src/db/omnigraph/optimize.rs | 234 +++++++++++++++--- crates/omnigraph/tests/composite_flow.rs | 69 ++++-- crates/omnigraph/tests/end_to_end.rs | 84 +++++++ crates/omnigraph/tests/failpoints.rs | 122 ++++++++- crates/omnigraph/tests/helpers/recovery.rs | 3 + crates/omnigraph/tests/maintenance.rs | 124 +++++++++- crates/omnigraph/tests/recovery.rs | 91 +++++++ docs/dev/testing.md | 6 +- docs/dev/writes.md | 17 +- docs/user/branches-commits.md | 2 +- docs/user/maintenance.md | 6 +- docs/user/storage.md | 2 +- 15 files changed, 816 insertions(+), 137 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b876749..3f5b711 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 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) | +| 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) | | 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/crates/omnigraph/src/db/manifest.rs b/crates/omnigraph/src/db/manifest.rs index 3b2886f..5bf1f87 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, - new_sidecar, recover_manifest_drift, write_sidecar, + list_sidecars, new_sidecar, recover_manifest_drift, write_sidecar, }; pub use state::SubTableEntry; #[cfg(test)] diff --git a/crates/omnigraph/src/db/manifest/recovery.rs b/crates/omnigraph/src/db/manifest/recovery.rs index 4c1b987..3119531 100644 --- a/crates/omnigraph/src/db/manifest/recovery.rs +++ b/crates/omnigraph/src/db/manifest/recovery.rs @@ -106,6 +106,12 @@ 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 @@ -412,11 +418,13 @@ pub(crate) fn parse_sidecar(sidecar_uri: &str, body: &str) -> Result<RecoverySid /// - **Strict** (`Mutation`, `Load`): exactly one `commit_staged` per /// table, so `lance_head == manifest_pinned + 1` AND /// `post_commit_pin == lance_head` is required. -/// - **Loose** (`SchemaApply`, `EnsureIndices`, `BranchMerge`): the -/// writer may run N ≥ 1 `commit_staged` calls per table (one per -/// index built + one for the overwrite, etc.; merge tables run -/// merge_insert + delete_where + index rebuilds) and the exact N -/// is hard to compute at sidecar-write time. The loose match accepts +/// - **Loose** (`SchemaApply`, `EnsureIndices`, `BranchMerge`, +/// `Optimize`): the writer advances the Lance HEAD by N ≥ 1 commits +/// per table (one per index built + one for the overwrite, etc.; +/// merge tables run merge_insert + delete_where + index rebuilds; +/// `Optimize` runs `compact_files`, which commits reserve-fragments + +/// rewrite) and the exact N is hard to compute at sidecar-write time. +/// The loose match accepts /// any `lance_head > manifest_pinned` as `RolledPastExpected` when /// `pin.expected_version == manifest_pinned` (the writer's CAS /// target matches what the manifest currently shows). The risk this @@ -494,9 +502,12 @@ 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: 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. +/// 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. pub(crate) async fn restore_table_to_version( table_path: &str, branch: Option<&str>, @@ -801,13 +812,24 @@ async fn roll_back_sidecar( sidecar: &RecoverySidecar, states: &[ClassifiedTable], ) -> Result<()> { - // 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. + // 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`). let mut outcomes = Vec::with_capacity(sidecar.tables.len()); + let mut updates: Vec<ManifestChange> = Vec::with_capacity(sidecar.tables.len()); + let mut expected: HashMap<String, u64> = HashMap::with_capacity(sidecar.tables.len()); for (pin, state) in sidecar.tables.iter().zip(states.iter()) { if matches!( state.classification, @@ -821,10 +843,20 @@ async fn roll_back_sidecar( state.manifest_pinned, ) .await?; - // `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". + // 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. outcomes.push(TableOutcome { table_key: pin.table_key.clone(), from_version: state.lance_head, @@ -832,13 +864,23 @@ async fn roll_back_sidecar( }); } } - // 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`. + // 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 + }; record_audit( root_uri, sidecar, - snapshot.version(), + manifest_version, RecoveryKind::RolledBack, outcomes, ) @@ -919,44 +961,20 @@ async fn roll_forward_all( HashMap::with_capacity(sidecar.tables.len() + sidecar.additional_registrations.len()); for pin in &sidecar.tables { - // 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( + // 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( root_uri, - &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); + &pin.table_key, + &pin.table_path, + pin.table_branch.as_deref(), + pin.expected_version, + &mut updates, + &mut expected, + ) + .await?; published_versions.insert(pin.table_key.clone(), head_version); } @@ -1047,6 +1065,57 @@ 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<ManifestChange>, + expected: &mut HashMap<String, u64>, +) -> Result<u64> { + 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/omnigraph/optimize.rs b/crates/omnigraph/src/db/omnigraph/optimize.rs index fff3f54..ee39323 100644 --- a/crates/omnigraph/src/db/omnigraph/optimize.rs +++ b/crates/omnigraph/src/db/omnigraph/optimize.rs @@ -8,8 +8,14 @@ //! Two dials: //! //! * `optimize_all_tables` — Lance `compact_files` on every table. Rewrites -//! small fragments into fewer large ones. Non-destructive (creates a new -//! version; old fragments remain reachable via older manifest versions). +//! 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. //! * `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 @@ -23,7 +29,9 @@ 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}; +use lance::dataset::optimize::{ + CompactionMetrics, CompactionOptions, compact_files, plan_compaction, +}; use super::*; @@ -111,7 +119,8 @@ pub struct TableOptimizeStats { pub fragments_removed: usize, /// Number of new, larger fragments Lance produced. pub fragments_added: usize, - /// Did this table get a new Lance manifest version from the compaction? + /// Did this table get a new manifest version from the compaction? True when + /// compaction ran and its compacted version was published to `__manifest`. pub committed: bool, /// `Some(reason)` if this table was deliberately not compacted. When set, /// `fragments_removed == 0`, `fragments_added == 0`, and `!committed`. @@ -153,12 +162,29 @@ pub struct TableCleanupStats { pub error: Option<String>, } -/// Run Lance `compact_files` on every node + edge table on `main`. -/// Tables run in parallel (bounded concurrency). +/// 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). pub async fn optimize_all_tables(db: &Omnigraph) -> Result<Vec<TableOptimizeStats>> { 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; @@ -183,49 +209,179 @@ pub async fn optimize_all_tables(db: &Omnigraph) -> Result<Vec<TableOptimizeStat } let concurrency = maint_concurrency().min(table_tasks.len()).max(1); - let table_store = &db.table_store; let stats: Vec<Result<TableOptimizeStats>> = futures::stream::iter(table_tasks.into_iter()) - .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, - )) + .map(move |(table_key, full_path, has_blob)| async move { + optimize_one_table(db, table_key, full_path, has_blob).await }) .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<TableOptimizeStats> { + // 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/tests/composite_flow.rs b/crates/omnigraph/tests/composite_flow.rs index 6c720da..dd41310 100644 --- a/crates/omnigraph/tests/composite_flow.rs +++ b/crates/omnigraph/tests/composite_flow.rs @@ -294,21 +294,19 @@ async fn composite_flow_canonical_lifecycle() { ); // ───────────────────────────────────────────────────────────────── - // Step 10: optimize the post-merge graph — verify indices stay - // valid and queryable. + // 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. // - // **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. + // 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. // ───────────────────────────────────────────────────────────────── let optimize_stats = db.optimize().await.unwrap(); assert!( @@ -331,6 +329,28 @@ 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 @@ -373,14 +393,27 @@ async fn composite_flow_canonical_lifecycle() { branches, ); - // 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. + // 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.) 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 a0fdb0e..ea11d0e 100644 --- a/crates/omnigraph/tests/end_to_end.rs +++ b/crates/omnigraph/tests/end_to_end.rs @@ -1933,3 +1933,87 @@ 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 149c63a..d240108 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,6 +1268,19 @@ 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 @@ -1461,10 +1474,15 @@ edge WorksAt: Person -> Company } let db = Omnigraph::open(&uri).await.unwrap(); - assert_eq!( - version_main(&db).await.unwrap(), - pre_failure_version, - "manifest must remain on the old schema when no schema staging files existed" + // 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!( helpers::count_rows(&db, "node:Person").await, @@ -1637,6 +1655,100 @@ 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 c76009e..90d9a25 100644 --- a/crates/omnigraph/tests/helpers/recovery.rs +++ b/crates/omnigraph/tests/helpers/recovery.rs @@ -181,6 +181,9 @@ 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 3e61677..2a5a659 100644 --- a/crates/omnigraph/tests/maintenance.rs +++ b/crates/omnigraph/tests/maintenance.rs @@ -8,10 +8,12 @@ mod helpers; use std::time::Duration; use lance::Dataset; -use omnigraph::db::{CleanupPolicyOptions, Omnigraph, SkipReason}; +use omnigraph::db::{CleanupPolicyOptions, Omnigraph, ReadTarget, SkipReason}; use omnigraph::loader::{LoadMode, load_jsonl}; -use helpers::{TEST_DATA, TEST_SCHEMA, count_rows, init_and_load}; +use helpers::{ + MUTATION_QUERIES, TEST_DATA, TEST_SCHEMA, count_rows, init_and_load, mixed_params, mutate_main, +}; /// Filesystem URI of a node sub-table, mirroring the engine's layout /// (FNV-1a of the type name under `nodes/`). Matches the helper in @@ -163,6 +165,124 @@ 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 '<table>': 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 a090178..f6b19e8 100644 --- a/crates/omnigraph/tests/recovery.rs +++ b/crates/omnigraph/tests/recovery.rs @@ -278,6 +278,97 @@ 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/docs/dev/testing.md b/docs/dev/testing.md index 425fcee..f18600b 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 | -| `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`). | +| `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 reconciles a pre-existing manifest-behind-HEAD drift forged via raw Lance compaction (`optimize_reconciles_preexisting_manifest_head_drift`) | +| `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`). | | `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). | +| `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). | ## Fixtures diff --git a/docs/dev/writes.md b/docs/dev/writes.md index 8b692b4..d2c7c7e 100644 --- a/docs/dev/writes.md +++ b/docs/dev/writes.md @@ -157,10 +157,14 @@ 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`): +`branch_merge_on_current_target`, `ensure_indices_for_branch`, +`optimize_all_tables`): 1. **Phase A**: writer writes a sidecar JSON to - `__recovery/{ulid}.json` BEFORE its first `commit_staged`. The + `__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 sidecar names every `(table_key, table_path, expected_version, post_commit_pin)` it intends to commit + the writer kind + actor_id. @@ -195,8 +199,13 @@ 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 for that branch. Rollback records the - actual restore target in the audit row's `to_version`. + 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). - 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 diff --git a/docs/user/branches-commits.md b/docs/user/branches-commits.md index 0565186..a4044cb 100644 --- a/docs/user/branches-commits.md +++ b/docs/user/branches-commits.md @@ -58,6 +58,6 @@ Internal or legacy branch refs: ## L2 — Recovery audit trail -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. +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. 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/maintenance.md b/docs/user/maintenance.md index 3628fa0..a835799 100644 --- a/docs/user/maintenance.md +++ b/docs/user/maintenance.md @@ -4,8 +4,10 @@ ## `optimize_all_tables(db)` — non-destructive -- Lance `compact_files()` on every node + edge table on `main`. -- Rewrites small fragments into fewer large ones; old fragments remain reachable via older manifests. +- 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.) - 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 d1c52b5..2c57a92 100644 --- a/docs/user/storage.md +++ b/docs/user/storage.md @@ -94,7 +94,7 @@ flowchart TB - **`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_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 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`. +- **`__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`. - **`_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. From ce150fb0ca903296cd7f26512293d1b63a4fceec Mon Sep 17 00:00:00 2001 From: Andrew Altshuler <andrew@collectivelab.io> Date: Mon, 8 Jun 2026 22:19:21 +0300 Subject: [PATCH 08/10] docs(testing): fix stale optimize test name in maintenance.rs row (#148) The maintenance.rs row referenced `optimize_reconciles_preexisting_manifest_head_drift`, which never existed (leftover from the reconcile-drift heuristic removed in #141). The actual second test is `optimize_defers_when_recovery_sidecar_is_pending`. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- docs/dev/testing.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/dev/testing.md b/docs/dev/testing.md index f18600b..8974a9f 100644 --- a/docs/dev/testing.md +++ b/docs/dev/testing.md @@ -34,7 +34,7 @@ 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 reconciles a pre-existing manifest-behind-HEAD drift forged via raw Lance compaction (`optimize_reconciles_preexisting_manifest_head_drift`) | +| `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`). | | `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). | From c2a97f4559b1e2c6e048be72630844a64d60d9aa Mon Sep 17 00:00:00 2001 From: Andrew Altshuler <andrew@collectivelab.io> Date: Mon, 8 Jun 2026 22:25:33 +0300 Subject: [PATCH 09/10] ci: drop per-PR Windows release build; bind to release tags (#155) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `test_windows_binaries` job ran a full Windows --release build + smoke test on every code PR. It was a non-required (non-blocking) check, so it never gated a merge — it only burned the slowest/most expensive runner (windows-latest, --release, 75-min ceiling) on every code change. Windows binary validation is already covered (better) on release tags: release.yml's `smoke_windows_installer` (on v* tags) builds the release binaries, installs via scripts/install.ps1, and smoke-runs `omnigraph.exe version` + `omnigraph-server.exe --help` — the same smoke test plus the real installer path. Nothing `needs:` the removed job. Trade-off (accepted): a PR that breaks the Windows build or install.ps1 syntax is now caught at release-cut rather than at PR time. install.ps1 and platform-specific code change rarely; the cost savings on every PR outweigh the earlier signal. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- .github/workflows/ci.yml | 57 ---------------------------------------- 1 file changed, 57 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5b7b7b2..bbe5893 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -261,63 +261,6 @@ 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: From 5eead8d29eb6a4e7dfb453603aa0efd8e6851c47 Mon Sep 17 00:00:00 2001 From: Andrew Altshuler <andrew@collectivelab.io> Date: Mon, 8 Jun 2026 22:26:04 +0300 Subject: [PATCH 10/10] ci(branch-protection): let code owners bypass required PR review (#154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit require_code_owner_reviews + count=1 with no bypass meant EVERY PR needed a code-owner approval — including code owners' own PRs, which can't be self-approved, so an owner's PR deadlocked on the other owner (forcing admin overrides). Intended behavior: review is required only for non-owners. Add bypass_pull_request_allowances for the two engineering owners (ragnorc, aaltshuler): they merge their own PRs after CI without a second review; non-owners still require a code-owner approval. CI status checks remain required for everyone. Applied live via scripts/apply-branch-protection.sh. Note: the bypass list mirrors codeowners-roles.yml engineering members by hand (render-codeowners.py doesn't generate it) — keep in sync on owner changes. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- .github/branch-protection.json | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/branch-protection.json b/.github/branch-protection.json index 7ca46b9..c039e32 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.", + "_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.)", "required_status_checks": { "strict": true, "contexts": [ @@ -17,7 +17,12 @@ "dismiss_stale_reviews": true, "require_code_owner_reviews": true, "required_approving_review_count": 1, - "require_last_push_approval": false + "require_last_push_approval": false, + "bypass_pull_request_allowances": { + "users": ["ragnorc", "aaltshuler"], + "teams": [], + "apps": [] + } }, "restrictions": null, "required_linear_history": true,