Supervise configured graph availability, with typed failures - #489
Open
ragnorc wants to merge 4 commits into
Open
Supervise configured graph availability, with typed failures#489ragnorc wants to merge 4 commits into
ragnorc wants to merge 4 commits into
Conversation
![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: d53d883da3
ℹ️ 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".
| OmniError::Compiler(error) => { | ||
| GraphFailure::new(FailureClass::InvalidConfiguration, error.to_string()) | ||
| } | ||
| error => GraphFailure::new(FailureClass::Unknown, error.to_string()), |
There was a problem hiding this comment.
When an S3/object-store outage makes DatasetBuilder::load fail during the initial graph open, the engine maps that failure to OmniError::Lance (crates/omnigraph/src/instrumentation.rs:551-554), but this catch-all classifies it as Unknown, which FailureClass::retryable rejects. The entry therefore becomes permanently unavailable, so the new supervisor never retries after storage recovers and operators must restart the process; preserve enough typed Lance/object-store error information to recognize transient failures.
Useful? React with 👍 / 👎.
| /// Block new writes and coalesce a Full-recovery wake-up for a serving | ||
| /// graph. The prior blocking operation remains the public diagnostic until | ||
| /// recovery succeeds; later requests are never treated as replays of it. | ||
| pub async fn mark_recovering( |
There was a problem hiding this comment.
When a served write leaves a durable sidecar and returns OmniError::RecoveryRequired, the handlers only convert it with ApiError::from_omni; a repository-wide search for mark_recovering finds no production caller of this method. Consequently the entry remains ready with write_ready=true, no notification reaches attempt_recovery, and the advertised background recovery path never runs; transition the graph at the request/error boundary and gate subsequent writes while recovery converges.
AGENTS.md reference: AGENTS.md:L166-L167
Useful? React with 👍 / 👎.
![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 d53d883. Configure here.
ragnorc
force-pushed
the
rfc-031-live-full-refresh
branch
from
August 12, 2026 20:55
9c09e5e to
3c06689
Compare
Four defects, all in the same layer: the supervisor could not tell a retryable
failure from a permanent one, and could not tell its own conclusion from a
newer request.
**The retry classifier could never return "retryable."** `FailureClass::Io` and
`Timeout` came only from `OmniError::Io`, which the engine has no construction
site for. Every Lance and object-store failure arrived as an opaque string and
landed in `Unknown`, which is not retried — so a transient S3 error pinned a
graph permanently, and `full_jitter_backoff` plus the whole `Opening{next_retry}`
arm were unreachable code. The unit test passed only because it hand-built
`OmniError::Io` values nothing emits. Consume the classification the engine now
derives at the substrate boundary, and delete the arm that sniffed error text
for "internal schema v" — a wire-visible field must not depend on internal
prose.
**One failed refresh could permanently kill reads.** The non-retryable branch
dropped the live handle and stored `Unavailable`, a terminal state with no way
back — and because of the above, *every* recovery failure took it. Recovery is
a write concern: a failed `refresh()` leaves the engine's live view exactly as
it was, `Dataset::restore` appends a version rather than removing one, reads pin
an exact version, `cleanup` refuses while a sidecar is pending, and the
byte-destructive paths are guarded to objects absent from the live manifest. So
`Serving` is now a one-way door with respect to recovery: a permanent failure is
`Blocked { retry_at: None }` — writes blocked, reads intact. `Unavailable`
remains reachable only from `Opening`.
**A newer recovery request could be clobbered.** Attempts read state, refresh
without the entry lock, then store a result derived from that read; the success
path re-checked only that the graph was still `Serving`, which every write state
satisfies. A `mark_recovering` landing mid-attempt was silently marked `Ready`.
Entries now carry a generation, bumped on each trigger and captured after the
permit is acquired, and an attempt only publishes if it still holds.
**Backoff never escalated.** Coalescing reset `attempts` to zero, so sustained
write traffic against a broken graph held the supervisor at its shortest delay
indefinitely — each retry a full recovery sweep whose cost grows with commit
depth. Coalescing now preserves both the attempt count and any scheduled
deadline; a new trigger invalidates an in-flight attempt without pulling the
schedule earlier.
Also in this pass:
- `GraphAvailability.state` is a typed enum with one conversion to the wire
form, replacing a `&'static str` re-parsed by two independent matches that
each silently reported "unavailable" for anything unrecognised.
- Public 503s carry the graph id and a class, never backend text. That response
is built in middleware, before any per-graph policy check, so it reaches
principals a Cedar policy would deny — and storage errors carry bucket names
and filesystem paths. The class summary stays on the policy-gated
`GET /graphs`; the full diagnostic stays in structured logs.
- `Retry-After` is emitted only when a retry is actually scheduled.
- A graph-local invalid URI becomes an unavailable configured entry instead of
aborting the process. Aborting made a typo *more* fatal than a graph that
cannot open at all. Invalid ids and duplicate URIs stay fatal, now with the
real reason recorded: the registry is keyed by `GraphId`, so an unparseable id
has no key, and a duplicate URI is genuinely ambiguous ownership.
- One process-global attempt-permit pool. Two `Semaphore::new` calls meant the
documented four-attempt bound was per-set, and the test asserting a
process-wide bound was asserting a property the code did not have.
Neither `Omnigraph::open` nor `refresh()` was bounded in time, and both were awaited while holding one of only four attempt permits. Four graphs stalled on an unresponsive object store starved open and recovery for every other configured graph, with no diagnostic — they simply sat in `Opening` or `Recovering` forever. The supervisor loop also awaited each attempt inline, so while one ran it observed neither shutdown nor new notifications, and `SupervisorSet::shutdown` blocked on it. The obvious fix — wrap the attempt in `tokio::time::timeout` — is wrong here. A recovery attempt mutates durable state under an armed sidecar: it may restore table A, then table B, then publish, then delete the sidecar. Dropping that future at an arbitrary await leaves a partially compensated multi-table state. That is precisely the failure mode the tracked-write executor exists to prevent for HTTP writes; introducing it for recovery would be worse, not better. So the deadline bounds the *wait*, not the work. Each attempt is spawned into a `TaskTracker` the supervisor set owns. After 240 seconds the loop stops waiting, logs, and moves on; the task keeps running to a terminal result and publishes under its own generation check. The loop retains the handle and starts no second attempt for that graph until the first finishes, so two refreshes never contend on the engine's gates for the same graph. Shutdown takes the same path: the loop can exit while an attempt it owns is still settling, and `drain_attempts` waits for them under a caller-supplied bound. `tokio-util` moves to the workspace dependency table, where the other shared async crates already live.
ragnorc
force-pushed
the
rfc-031-graph-supervision
branch
from
August 12, 2026 23:32
d53d883 to
953b208
Compare
`AppState::open*` and `new_multi` paired a live write executor with `SupervisorSet::idle()` — a set with no per-graph tasks. Those constructors are public and used by embedders and the whole server test suite. A single `RecoveryRequired` result, or a panic in a tracked write, marks the graph recovering; with no supervisor there is no consumer for that wake-up, so the graph stays write-blocked with `write_ready: false` until the process restarts. The state was reachable only because the type system allowed it. Removing `idle()` makes the combination unconstructible: every `AppState` supervises its own registry. A set over an empty or already-serving registry costs one parked task per graph, which is what the loop does at rest anyway.
ragnorc
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #488. The registry retains every configured graph — including ones that fail to open — and a per-graph supervisor drives open and recovery. The first commit introduces that; the rest close what it landed with.
The retry classifier could never return "retryable."
FailureClass::Io/Timeoutcame only fromOmniError::Io, which the engine has no construction site for. Every Lance and object-store failure arrived as an opaque string and landed inUnknown, which is not retried — so a transient S3 error pinned a graph permanently andfull_jitter_backoffplus the wholeOpening{next_retry}arm were unreachable code. The existing unit test passed only because it hand-builtOmniError::Iovalues nothing emits. Now consumes the classification #491 derives at the substrate boundary, and the arm that sniffed error text for"internal schema v"is gone — a wire-visible field must not depend on internal prose.One failed refresh could permanently kill reads. The non-retryable branch dropped the live handle and stored
Unavailable, a terminal state with no way back — and because of the above, every recovery failure took it. Recovery is a write concern: a failedrefresh()leaves the engine's live view intact,Dataset::restoreappends rather than removes, reads pin an exact version,cleanuprefuses while a sidecar is pending, and byte-destructive paths are guarded to objects absent from the live manifest.Servingis now a one-way door: a permanent failure isBlocked { retry_at: None }— writes blocked, reads intact.Unavailableis reachable only fromOpening.A newer recovery request could be clobbered. Attempts read state, refresh without the entry lock, then store a result derived from that read; the success path re-checked only that the graph was still
Serving, which every write state satisfies. Entries now carry a generation, bumped per trigger and captured after the permit, and an attempt publishes only if it still holds.Backoff never escalated. Coalescing reset
attemptsto zero, so sustained write traffic held the supervisor at its shortest delay indefinitely — each retry a full recovery sweep whose cost grows with commit depth. Coalescing now preserves the attempt count and any scheduled deadline.Attempts were unbounded in time and awaited inline, holding one of four permits. Four graphs stalled on an unresponsive store starved every other graph, and the loop saw neither shutdown nor new notifications while one ran. Wrapping the attempt in a timeout would be wrong — a recovery mutates durable state under an armed sidecar, and dropping that future mid-restore is exactly what the tracked-write executor exists to prevent. So the 240s deadline bounds the wait, not the work: attempts are spawned into a
TaskTrackerthe set owns, run to a terminal result, and publish under their own generation check. The loop starts no second attempt for a graph while one is in flight.Also: typed
GraphStatewith one conversion to the wire form (replacing a&'static strre-parsed by two matches that each silently reported "unavailable" for anything unrecognised); public 503s carry graph id and class only, never backend text, since that response is built in middleware ahead of any per-graph policy check;Retry-Afteronly when a retry is actually scheduled; a graph-local invalid URI becomes an unavailable entry instead of aborting the process; one process-global attempt-permit pool; andSupervisorSet::idle()is removed so a live write executor can never be paired with a set that has no consumer for its wake-ups.Verification
a_trigger_arriving_mid_attempt_is_not_clobbered— drives the race through the attempt seam.coalescing_preserves_the_backoff_ladder;a_permanent_recovery_failure_keeps_reads_available.cargo test --workspace --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints— 75 suites, zero failures.cargo fmt --all --check.Known, deliberate
mark_recoveringstill has no production caller at this commit — the caller is the tracked-write executor in #490. Reviewers checking this PR in isolation will correctly observe that the recovery half of the state machine has no producer yet; that is a property of the stack split, not a defect.Cluster-quarantined graphs (recovery sidecars) are still filtered before settings are built, so they 404 rather than 503. Converting them to configured entries is follow-up work.
No storage-format migration.
Greptile Summary
The PR retains all configured graphs in the registry and supervises opening and recovery independently while exposing bounded availability metadata.
Confidence Score: 5/5
The PR appears safe to merge because the previously reported backend-diagnostic disclosure has been removed and no blocking failure remains.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Auth participant Registry participant Supervisor participant Engine Client->>Auth: "Request /graphs/{id}/..." Auth->>Registry: Resolve configured graph alt Graph is serving Registry-->>Client: Dispatch with live handle else Graph is opening or unavailable Registry-->>Client: 503 graph_unavailable end Supervisor->>Engine: Open or refresh alt Attempt succeeds Engine-->>Supervisor: Healthy handle Supervisor->>Registry: Mark ready else Retryable failure Engine-->>Supervisor: Classified failure Supervisor->>Registry: Publish retry metadata Supervisor->>Supervisor: Full-jitter backoff else Permanent failure Engine-->>Supervisor: Non-retryable failure Supervisor->>Registry: Mark unavailable/degraded endReviews (3): Last reviewed commit: "fix(server): give every AppState a live ..." | Re-trigger Greptile
Context used: