Repository navigation
Conversation
Add the `maintain` and `reclaim` Cedar actions. `maintain` covers optimize, index builds, the full-text rebuild and the repair report and is judged on the branch maintained; `reclaim` covers cleanup and takes no branch scope. Maintenance has no actor-less public engine entry point any more: `optimize_as`, `cleanup_as`, `ensure_indices_as`, `ensure_indices_on_as`, `rebuild_full_text_indices_on_as` and the read-only `repair_report_as` each enforce their action before any effect. The full-text rebuild admits `maintain` or `change` through `PolicyChecker::check_any`. Repair loses its publishing mode, its options and its hard-coded actor strings. The CLI, DST, GQT and bench callers move to the `_as` forms; the direct CLI passes the actor it resolves for direct writes.
|
This draft was built on Phase A of RFC #1168, which was closed on 2026-10-27 in favor of focused RFCs that start with a bounded, manually requested remote |
Record the intent nonce and resolve by commit_id_answers; keep an in-doubt publication non-terminal; release the writer gates before the artifact scan; add a resume position, resource ceilings and per-table failure isolation; require maintain on every route; depend on ModernRelay#1175 and ModernRelay#1176.
…cy-actions The index build from main checked `change`; it now checks `maintain` like the other index writers, and `ensure_indices_on_as` returns main's `IndexBuildResult`. The actor-less `ensure_indices` and `ensure_indices_on` stay removed. Main's unreleased index-build note is updated to match, and the maintenance page and notes are brought within their caps.
* rfc: remote optimize through the serving owner A bounded, manually requested optimize of a served graph's main branch, executed by the process that serves and publishes it: today's gate shape and one publication, a gate-hold bound with an exact outcome per table, a durable operation record under a caller-chosen id before any effect, lookup by that id after a lost response, and restart resolution from the graph's history. Replaces the closed served-maintenance proposal in the form its review asked for. * rfc: link the remote optimize RFC to its discussion * rfc: revise remote optimize after review Record the intent nonce and resolve by commit_id_answers; keep an in-doubt publication non-terminal; release the writer gates before the artifact scan; add a resume position, resource ceilings and per-table failure isolation; require maintain on every route; depend on ModernRelay#1175 and ModernRelay#1176. * rfc: settle remote optimize resume, ceilings and resolution after second review * rfc: settle remote optimize resolution reads and the artifact bound after third review * rfc: resolve through the buffered commit lookup and count both offset sides after fourth review * rfc: state the history cost of a released commit and reuse the collector's path resolution after fifth review
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: not ready to merge. Resolve the merge conflict first. The design is sound and I found no code defect.
Why
No maintenance entry point checks policy today. optimize, cleanup, ensure_indices and repair take no actor. An embedded program with a policy checker can compact, rebuild indexes and delete history around the gate that invariant 10 requires. Served maintenance (RFC #1214) needs a grant to check before it can have a route.
What
- Two Cedar actions join the vocabulary.
maintainis branch scoped.reclaimis graph scoped and refuses branch scopes. - Every maintenance method is now an
_asform that enforces its action before any effect. The actor-less forms are removed. - Optimize and the index writer record the actor on the commit they publish.
- The full-text rebuild admits
maintainorchangethroughPolicyChecker::check_any. An evaluation failure refuses at once and does not fall through to the next action. repairbecomes the read-onlyrepair_report_as. The publishing mode (confirm,force) is gone.forbidden_apis.rs::maintenance_entry_points_take_an_actorpins the set of maintenance surfaces.
Blocking
- The branch conflicts with
mainincrates/omnigraph/src/db/omnigraph/repair.rs(checked withgit merge-treeagainstdd7a9f75). Main changed that file in #1228. The merge has to keep the report-only shape and themaintaingate. Please rebase or merge and run CI again. - The PR is a draft and its own comment says it needs re-justifying against the replacement RFC. #1214 now depends on this PR for
maintainandoptimize_as, and saysreclaim,cleanup_asand the repair report belong with cleanup and repair. That settles the question formaintainandoptimize_as. State in the PR whether thereclaimand repair parts stay here or move out, and take it out of draft when you decide.
Optional
cleanup_planand its helpers stay actor-less and public. They are read-only, but they list whatreclaimwould delete. Gate them withreclaim(ormaintain) when the cleanup route lands, as the PR text says. Add a test then, so the surface is not forgotten.- "Accepts
changefor one more release" has no mechanism. The note says the full-text rebuild stops acceptingchangeafter one release. Nothing fails at that point. Add a tracking issue, or a test that goes red at the next version bump. Otherwisecheck_anyand its two-action call stay for good. They are the only multi-action gate. - The PR joins two changes. One is the policy gate. The other removes
repair's publishing mode. The removal is correct: invariant 5 says foreign drift is reported and never adopted, and no internal caller used the mode. It also removes a mutating door instead of gating it, and it deletes state. It does enlarge the breaking API change for embedders. If review slows down, this part splits out cleanly. - The access report does not list the new actions. The PR defers this to the routes step, because a new enum value is a wire change under the 0.14 contract. That is right. Make the routes PR carry it, so the report cannot disagree with the policy language.
- Liability check. Five more maintenance operations would each add one
_asmethod and one registry row, and the pinned-set test would fail on a method without an actor. That is a good place for the invariant to live. The cost is two actions pluscheck_any. Both are small and have one owner each.
Validation
- Exact-head CI at
de602a32: all required checks pass, including Test Workspace, Clippy, rustfmt, DST pinned suite and the four GQT contexts. The PR text reports one flakycsv_sourcefailure on a rerun. It is unrelated. - Local run at
de602a32(cargo, reused build outputs):cargo test -p omnigraph-policy --locked --lib: 24 passed.cargo test -p omnigraph-engine --features failpoints --test policy_engine_chassis maintenance: 1 passed (maintenance_entry_points_enforce_maintain_on_their_branch_before_effects_issue_840).cargo test -p omnigraph-engine --test forbidden_apis maintenance_entry_points: 1 passed.
- Code inspection: every
enforcecall comes before the schema-state check and the export reservation, so a refused call writes nothing. The chassis test asserts the physical tree is unchanged after each refusal. - No before/after regression proof. This is a feature, not a bug fix, and the embedded policy API is not visible in GQT rows. The chassis and policy-crate tests are the cheapest owners. They are new tests, so they have no earlier head to fail against.
- Not run locally: the full workspace, the CLI tests and the S3 and Azure suites. I rely on CI for those.
- Not verified: the server and MCP behavior after the conflict is resolved.
Update, 2026-10-29. #1168 is closed. RFC #1190 (remote optimize) now depends on this PR for the
maintainaction andoptimize_as. The merge of main brings inomnigraph index buildfrom #1150, which checkedchange. It now checksmaintainlike the other index writers: thatchangegate never shipped in a release, so it needs no deprecation, unlike the full-text rebuild's.ensure_indices_on_asreturns main'sIndexBuildResult, and #1150's assertions on the built indexes and the commit's actor move intomaintenance_entry_points_enforce_maintain_on_their_branch_before_effects_issue_840. The release notes and the maintenance page are brought within their caps. After the merge I built and tested the engine locally: workspace Clippy with failpoints passes, as dopolicy_engine_chassis(27),forbidden_apis(37),schema_apply(41) and themaintenance::andbranching::integration modules (110).Phase A, step 1 ("Policy") of
docs/rfcs/2026-10-26-served-graph-maintenance.md(#1168). No maintenance entry point checks policy today:optimize,cleanup,ensure_indices,ensure_indices_onandrepairtake no actor, andrepairwithconfirmpublishes under hard-coded actor strings, so an embedded program with a policy checker installed can compact, rebuild indexes and delete history around the gate that invariant 10 requires of every mutating entry point (fact F7 in the RFC). Served maintenance, the later Phase A steps, needs a grant to check before it can have a route.Two Cedar actions join the vocabulary, following decisions O1 and O6.
maintaincovers optimize, index builds, the full-text rebuild and the repair report, and is judged on the branch the operation maintains (mainfor optimize and repair), so it takesbranch_scopelikechange.reclaimcovers cleanup, which spans every branch's history, so it is graph scoped and refuses branch scopes.Adminstays reserved. The engine's maintenance surface is now only_asforms, each enforcing its action before any effect:optimize_as,ensure_indices_as,ensure_indices_on_as,rebuild_full_text_indices_on_asand the read-onlyrepair_report_ascheckmaintain, andcleanup_aschecksreclaim. The actor-less forms are removed rather than made crate-private, since nothing inside the crate needs them; optimize and the index writer now record the actor on the commit they publish. For O5 the full-text rebuild admitsmaintainorchangethrough a new provided method,PolicyChecker::check_any, behindOmnigraph::enforce_any, of whichenforceis now the one-action case: the first allowed action admits, an evaluation failure refuses at once rather than falling through to the next action, and a refusal names every action. Repair is report-only:RepairOptions, thehealedandforcedactions,RepairStats::graph_manifest_versionand the publish withomnigraph:repairandomnigraph:repair:forceare gone. No internal path used the publishing mode, so nothing is kept crate-private for it; the coordinator'sPublishPrecondition::Anypublisher it called now compiles only for tests and failpoint fixtures. The guard isforbidden_apis.rs::maintenance_entry_points_take_an_actor: every surface registered under the index, optimize and cleanup protocols, plus the repair report, must be an_asmethod with anactor: Option<&str>parameter, and the set is pinned, so a new actor-less maintenance method fails the registry. The CLI, DST, GQT and bench callers move to the_asforms; the direct CLI passesresolve_actor, as the full-text rebuild and direct writes already do.policy_engine_chassis.rsgainsmaintenance_entry_points_enforce_maintain_on_their_branch_before_effects, which refuses every maintenance entry point to an actor holding onlychange,reclaimormaintainon unprotected branches, and to no actor, with the storage tree byte-for-byte unchanged, then admits each undermaintainand checks that optimize's commit carries the actor;cleanup_as_enforces_reclaim_before_effectsdoes the same for cleanup; and the full-text test now admits the rebuild under either grant, requires the refusal to name both, and checks each commit's actor. The policy crate's tests round-trip every action through its name and serde, compile a rule for every action against the Cedar schema, pin the scope rules of the two new actions, and covercheck_any, including that an evaluation failure on the first action is not passed over. The ignoredchanges_cost.rstest that built its multi-version interval with a forced repair is deleted, since no builder for that fixture remains, and the repair confirm and force tests inintegration/maintenance.rsfold into the two report tests they duplicated.For review: the decision to challenge is the scope of
maintain. Judging it per branch lets a policy keep automation off protectedmain, and lets the O5 overlap comparechangeandmaintainon the same scope; a graph-widemaintainwould be simpler for the future routes but could not express that. The access report (GET /graphs/{graph_id}/access, the MCPaccesstool andomnigraph access) does not name the new actions, because no served operation performs maintenance and a new value in that enum is a wire change under the 0.14 contract; the routes step should add them.cleanup_plan, the read-only preview, keeps no actor here; the RFC'sreclaimon the preview lands with its route. The CLI still refuses--ason optimize, repair and cleanup, so they act asoperator.actor; widening--asbelongs to the CLI step.repair --jsondrops its always-nullgraph_manifest_version. This is a source-breaking change for Rust embedders, with a breaking note. The engine was not built locally: the machine's free disk fell below the 20 GB floor during the first engine build, so the engine, CLI and workspace results are CI's. On the first CI runcsv_source::tests::cancelled_csv_producer_settles_and_releases_its_admissionfailed itspause.is_paused()assertion; it touches nothing here and passed on the re-run, so it looks like a race in that test.