Make live recovery refresh atomic, cheap, and off the write path - #488
Make live recovery refresh atomic, cheap, and off the write path#488ragnorc wants to merge 2 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9c09e5e. Configure here.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c09e5e281
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| drop(schema_guard); | ||
| self.reload_schema_if_source_changed().await?; |
There was a problem hiding this comment.
Do not release schema_guard before reloading the schema view. If another handle completes apply_schema after this drop but before reload_schema_if_source_changed reacquires the gate, this handle combines the newly accepted schema with the coordinator captured before that migration; an added-table migration therefore makes refresh() fail with a schema/manifest conflict, while compatible metadata-only changes can install a catalog from a different accepted view than the adopted coordinator. Reload under the already-held, non-reentrant gate (for example through a helper that assumes the gate is held), then release it after both coordinator and schema view are published.
AGENTS.md reference: AGENTS.md:L165-L167
Useful? React with 👍 / 👎.
The live Full-recovery refresh landed with three problems this commit closes. **It could leave the handle torn.** `refresh` published the recovered coordinator, released the schema gate, and only then reloaded the schema view. A failure in that reload — or a schema apply interleaving in the gap — left the handle carrying a new coordinator beside a stale catalog: data on one contract, catalog on another. Restructure into prepare-then-publish. Recovery, the branch re-open, and the prospective schema view are all built against local state; then one publish takes the coordinator write lock, drops derived caches, and swaps coordinator and view together with no fallible work after the first replacement. A failure anywhere is a clean no-op. This needs the schema gate held across recovery *and* view preparation, so the reload splits into an acquiring wrapper and a gate-held builder — the write queue is not reentrant. That builder takes the snapshot as a parameter and never reads `self.coordinator`, which is what keeps the publish path clear of the non-reentrant `RwLock` deadlock already pinned by `composite_flow_schema_apply_then_branch_ops_no_deadlock_in_refresh`. **It cost two or three full manifest scans.** The shared helper always re-read the coordinator, because read-write *open* builds its coordinator before awaiting the schema gate and a live handle may have published meanwhile. `refresh` builds its coordinator after acquiring the gate, so that re-read was a second full `__manifest` open and scan for no new information — on a path the supervisor calls, over a journal whose scan cost grows with commit depth. Make the freshness explicit at the call site, and skip the trailing re-read when the sweep changed nothing (the common deferred-sidecar outcome), using the signal `process_sidecar` already returns. A steady-state refresh is now one open, one scan, pinned by a cost budget. **It silently upgraded ordinary writes to destructive recovery.** `mutate` and `load` call `refresh()` to reprepare when a pre-effect read set changes. Neither file was touched when `refresh` gained Full-recovery semantics, so both quietly began running rollback-capable recovery on the contended write path — up to 32 times in the loader's reprepare loop. A reprepare needs current authority and nothing else: it now uses `refresh_coordinator_only`. An unresolved rollback-eligible sidecar is already surfaced to these callers by the write-entry roll-forward barrier as `RecoveryRequired`, which the server resolves by scheduling an explicit refresh. A source guard pins it, because the regression was invisible in the diff that caused it. Also: a handle opened read-only no longer performs recovery in `refresh()`. A caller that asked to skip the open-time sweep did not consent to restores, sidecar deletion, or schema-staging promotion later, and its credentials may not permit them — the cluster crate opens read-only handles purely to probe graphs.
9c09e5e to
3c06689
Compare
ragnorc
changed the title
Maintainer review: changes requestedThe prepare-then-publish direction is sound, but two correctness blockers remain:
Please rebase after #491 is corrected and run canonical workspace/failpoint tests, Clippy, format, focused recovery tests, and RustFS coverage. The production delta is separable from #491, so restacking directly on main is also viable if this needs to land independently. |
|
Closing this implementation PR in favor of RFC-034: #496 The requirement remains—recover one unavailable graph without restarting unrelated graphs—but the replacement design uses a safer boundary:
This closes the in-place-refresh implementation direction, not the problem. Useful failure schedules and test ideas from #488 are carried into RFC-034's acceptance matrix; the #488 commits should not be merged as-is. |
2 participants
Summary
Stacked on #491. Makes
Omnigraph::refresh()the served-graph recovery primitive — running the same Full sweep as read-write open, through one shared helper — so a long-running server heals a rollback-eligible sidecar without a restart.The first commit introduces that. The second commit closes three problems it landed with:
It could leave the handle torn.
refreshpublished the recovered coordinator, released the schema gate, and only then reloaded the schema view. A failure in that reload — or a schema apply interleaving in the gap — left the handle carrying a new coordinator beside a stale catalog.Restructured into prepare-then-publish: recovery, the branch re-open, and the prospective schema view are all built against local state; then one publish takes the coordinator write lock, drops derived caches, and swaps coordinator and view together with no fallible work after the first replacement. This requires holding the schema gate across recovery and view preparation, so the reload splits into an acquiring wrapper plus a gate-held builder (the write queue is not reentrant). That builder takes the snapshot as a parameter and never reads
self.coordinator— which keeps the publish path clear of the non-reentrantRwLockdeadlock already pinned bycomposite_flow_schema_apply_then_branch_ops_no_deadlock_in_refresh.It cost two or three full manifest scans. The shared helper always re-read the coordinator, because read-write open builds its coordinator before awaiting the schema gate.
refreshbuilds its coordinator after acquiring the gate, so that re-read was a second full__manifestopen and scan for no new information — on a path the supervisor calls, over a journal whose scan cost grows with commit depth. Freshness is now explicit at the call site, and the trailing re-read is skipped when the sweep changed nothing (using the signalprocess_sidecaralready returns).It silently upgraded ordinary writes to destructive recovery.
mutateandloadcallrefresh()to reprepare when a pre-effect read set changes. Neither file was touched whenrefreshgained Full-recovery semantics, so both quietly began running rollback-capable recovery on the contended write path — up to 32 times in the loader's reprepare loop. They now userefresh_coordinator_only. An unresolved rollback-eligible sidecar is already surfaced to these callers by the write-entry roll-forward barrier asRecoveryRequired.Additionally: a handle opened read-only no longer performs recovery in
refresh(). A caller that asked to skip the open-time sweep did not consent to restores, sidecar deletion, or schema-staging promotion later, and its credentials may not permit them — the cluster crate opens read-only handles purely to probe graphs.The safety boundary is unchanged: handles in one process share the root gates; foreign writer processes remain unsupported.
Verification
refresh_without_sidecars_uses_one_manifest_open— steady-state refresh is exactly one manifest open and one scan, at commit-history depth.failed_refresh_publishes_nothing_and_still_converges— a failure in preparation leaves the schema projection untouched and the handle converges on retry. (Scope is stated in the test: the coordinator half is not black-box observable because reads self-refresh.)read_only_refresh_never_runs_recovery— a read-only handle neither consumes nor rewrites a sidecar; a read-write handle still does.write_reprepare_paths_do_not_run_full_recovery— source guard, non-vacuous (asserts the reprepare site is still seen). Source-level because the regression was invisible in the diff that caused it.cargo test --workspace --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints— 75 suites, zero failures.cargo fmt --all --check;scripts/check-agents-md.sh.No storage-format migration.
Greptile Summary
The PR makes explicit refresh on writable live handles run the same process-gated Full recovery sweep as read-write open while preserving a coordinator-only path for read-only handles and contended write retries.
Confidence Score: 5/5
The PR appears safe to merge because the previously reported read-only recovery write path is now blocked and no blocking failure remains.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller participant Handle as Omnigraph handle participant Gate as Root schema gate participant Recovery as Full recovery participant Live as Live coordinator/schema view Caller->>Handle: refresh() alt Read-only handle Handle->>Live: refresh coordinator and invalidate caches Live-->>Caller: refreshed read view else Read-write handle Handle->>Gate: acquire process-global schema gate Handle->>Recovery: open temporary coordinator Recovery->>Recovery: reconcile schema staging and sidecars Recovery-->>Handle: recovered coordinator Handle->>Handle: prepare coherent schema view Handle->>Live: invalidate caches and atomically adopt state Handle->>Gate: release gate Live-->>Caller: recovered live handle endReviews (2): Last reviewed commit: "fix(recovery): make live refresh atomic ..." | Re-trigger Greptile
Context used: