write-protocol lifetime ownership and recovery barriers#378
Conversation
…n barriers Proposes three coordinated, independently landable changes closing the class behind the interrupted-mutation availability incident: engine-owned write-protocol lifetime (cancellation shielding, staged server-first), full recovery escalation at the write-entry heal under exclusive gates (restart barrier -> gate barrier), and server boot supervision that makes quarantine an observable, converging state. The RFC-022 commit protocol, Armed/EffectsConfirmed classification, fail-closed ambiguity, and the single-writer-process boundary are explicitly unchanged. Every motivating claim is validated against current code and Lance upstream source at the pinned rev, including decoding the incident's _versions/18446744073709551611.manifest as an ordinary V2-named commit of table version 4 and locating Lance's verbatim orphaned-transaction- file warning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb55888a10
ℹ️ 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".
| let task = tokio::spawn(async move { | ||
| engine.mutate_as(&branch, &query, &name, ¶ms, actor_id.as_deref()).await | ||
| }); |
There was a problem hiding this comment.
Move admission guard into the spawned task
In the current run_mutate path the per-actor permit is acquired before this call (handlers.rs:686-689). If Stage 1 follows this snippet, the spawned task owns only the engine and inputs; _admission stays in the handler and is dropped when the client disconnects, releasing the slot while the write continues. That violates this RFC's admission invariant and lets disconnecting clients bypass per-actor concurrency limits, so the spawned future needs to own the guard or the whole admitted operation must be spawned.
Useful? React with 👍 / 👎.
| 1. **PR-1 (W1 Stage 1):** server-boundary shield + the two W1 tests + stale | ||
| comment fix. Smallest change, kills the incident's trigger. |
There was a problem hiding this comment.
Move engine-cancellation test to Stage 2
PR-1 is scoped to the HTTP server-boundary shield, but requiring both W1 tests here also includes the failpoint test above that drops the engine caller future directly. That case remains cancellable until PR-4's engine-level Arc/inner-handle restructure, so PR-1 cannot turn its required red test green. Split the plan so PR-1 only tests a dropped HTTP client and reserve the direct future-drop test for Stage 2.
Useful? React with 👍 / 👎.
…tations-graph-drift-njvits
Feature-gated failpoint test that pins the three behaviors RFC-029's design rests on, at the engine boundary: (1) dropping a mutation future between sidecar arm and confirmation leaks the Armed recovery sidecar (no Drop guard compensates); (2) a subsequent write on the still-live handle returns RecoveryRequired because the write-entry heal is roll-forward-only; (3) an in-process read-write Omnigraph::open runs the Full sweep under the shared root-scoped write queue, resolves the residual, and the previously wedged handle becomes writable again without a process restart. These are assertions of current behavior (green today), recorded as the baseline the RFC's W1/W2/W3 changes will deliberately move: after W1, cell (1) inverts — cancellation can no longer create the residual. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
| // effect is committed, the sidecar is armed, confirmation has not been | ||
| // durably recorded. Cancelling here manufactures the exact Armed residual | ||
| // a client disconnect produces. | ||
| let rv = Rendezvous::park_first(omnigraph::failpoints::names::RECOVERY_SIDECAR_CONFIRM); |
There was a problem hiding this comment.
Missing FailScenario setup
Medium Severity
The RFC-029 probe registers a Rendezvous on RECOVERY_SIDECAR_CONFIRM but never calls FailScenario::setup(), unlike other failpoint integration tests. With the fail crate, injected failpoints stay disabled in tests without that setup, so the mutation likely never parks and the probe cannot reliably reproduce cancellation mid-confirmation.
Reviewed by Cursor Bugbot for commit 0793712. Configure here.
Code investigation strengthened both bug claims and upgraded the design:
- W1 has an exact in-engine precedent: the RFC-026 B1 stream-fold writer
already detaches its whole protocol via Arc<Self> +
spawn_with_query_io_probes, explicitly so cancellation cannot abandon
it, and that helper already solves task-local probe propagation across
the spawn. Stage 2 is rescoped to bringing the established writers up
to that existing standard.
- W2 gains a zero-engine-change implementation, W2(b): a supervised
in-process reopen + registry swap. The open-time Full sweep runs under
the same root-scoped write queue live handles share and takes
exclusive recovery admission, so it serializes against live traffic by
construction; the open path's own comment ('only rollback-eligible
sidecars wait for this open-time sweep') names it as the designed
resolution. W2(b) and W3 unify into one supervision mechanism with two
triggers; the heal escalation W2(a) becomes an optional end state and
the only part of the RFC that relaxes a stated rule.
- Evidence: reference the checked-in tests/rfc029_probe.rs baseline pin
and rework the rollout into PR-1 (shield), PR-2 (unified supervision),
PR-3 (engine shield), PR-4 (optional heal escalation).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
…idual class The first probe run refuted the naive Bug-2 reproduction: a mutation cancelled AT the confirmation failpoint on local FS still produced a durably-confirmed sidecar, because storage puts run on spawn_blocking and an already-spawned blocking write completes after the awaiting future is dropped — the residual self-healed on the next write's roll-forward-only heal. On a network object store the in-flight PUT dies with the future, leaving the Armed class that wedges; consistent with the motivating incident occurring on S3. RFC-029 now records this: the cancellation residual's class is a storage-backend-dependent race, and W1 removes the roulette rather than tuning it. The probe is reworked into three deterministic phases, all green: (1) cancellation leaks the sidecar (class-agnostic assert); (2) a failed confirmation write — the failpoint's documented crash model — manufactures Armed deterministically, and the live handle then returns RecoveryRequired with the documented reopen guidance; (3) an in-process read-write open clears __recovery/ and the wedged handle writes again without a process restart, proving the W2(b) supervised-reopen mechanism end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
RFC-029 PR-1 prep: expose the engine's failpoint registry to omnigraph-server's feature-gated integration tests, mirroring the omnigraph-cluster passthrough but without dep:fail since the server defines no failpoint hooks of its own. No behavior change; the feature is never enabled in production builds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
…idual (RFC-029 W1, red) HTTP-boundary cancellation cell: park the engine at the sidecar-confirmation failpoint, abort the task driving the oneshot request (the exact disconnect model — tower drives the handler future inline, so the abort drops it as hyper does on client disconnect), then poll ONLY the filesystem: __recovery/ must empty with no further requests and no reopen. Sidecar deletion happens strictly after the manifest publish, so an empty __recovery/ proves the whole protocol completed under engine ownership. The poll deliberately sends nothing: a follow-up write would run the write-entry heal and mask an unshielded server. RED as predicted: 'dropped mutation abandoned its armed protocol: __recovery/ still holds [<ulid>.json] after the deadline with no further requests'. Green arrives with the next commit's shield. Also: CI runs the new suite in the failpoints step, and the failpoint-names source guard now walks omnigraph-server so the compile-checked-catalog rule covers the new call sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
…FC-029 W1 Stage 1) One shielded_write helper + six site conversions (mutate, load, schema apply, branch create/delete/merge, and the merge's follow-up source delete). Cedar authorization, request validation, and per-actor admission stay in the handler in their existing order; the admission guard and owned inputs then move into a spawned task that runs the engine protocol to its own terminal state regardless of the request future's fate. The handler still awaits the result, so the HTTP contract is unchanged — only abandonment semantics change: a client disconnect no longer cancels the commit protocol mid-flight, and the admission slot releases at protocol completion rather than at disconnect. Panic in the protocol surfaces as JoinError -> 500. Turns the previous commit's red HTTP-boundary cancellation cell green; full server suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
…ation claims - Correct writes.rs::cancelled_mutation_future_leaves_no_state's stale doc comment: 'only orphaned Lance fragments can remain' predates the RFC-022 sidecar protocol, and the test's final open runs the Full sweep before its assertions, so it cannot observe post-arm residue; point at rfc029_probe.rs as the residue-lifecycle pin. - Qualify writes.md's cancellation paragraph the same way: the no-Lance-write guarantee holds during op execution, while post-arm drops leave recovery-covered residue; the HTTP boundary now shields cancellation entirely (RFC-029 W1 Stage 1). - server.md: document disconnect semantics (writes complete server-side; outcome ambiguous to the disconnected client — verify by read) and admission-slot-until-completion. - testing.md: register the new server failpoints suite and its passthrough feature; names-guard coverage note. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
| .await | ||
| .map_err(ApiError::from_omni) | ||
| }) | ||
| .await? |
There was a problem hiding this comment.
Merge delete drops admission early
Medium Severity
For delete_branch: true, the per-actor admission guard now lives only inside the merge shielded_write task and is released when the merge finishes. The follow-on branch_delete_as runs with no admission slot, so one merge request can briefly run two armed write protocols while consuming one slot, and other writes can admit during the post-merge delete.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bd2a043. Configure here.
…W3 prep) QuarantineInfo + a parallel quarantined map on RegistrySnapshot (a key is never in both maps; disjointness debug-asserted), a RegistryLookup::Quarantined variant, and two production writer methods serialized on the now-ungated mutate mutex — its original doc comment named this exact consumer as the ungating condition: - publish(handle): RCU install for the handle's key, clearing any quarantine entry and replacing a prior serving handle (the supervised-reopen swap; duplicate-URI check skips the replaced key). - set_quarantined(key, info): records/updates a quarantine entry, no-op while the key is serving so a failed supervised reopen of a healthy graph cannot take it down. A quarantined graph whose config declares a per-graph policy keeps any_per_graph_policy true, so bearer auth cannot flap off while the graph heals (strictly safer than today, where the graph vanishes). Nothing populates quarantine state yet: from_handles delegates to from_boot with no entries, and the routing middleware temporarily maps Quarantined to the existing 404 until the lookup-semantics change lands behind its red test. Zero behavior change; unit tests cover publish-replaces-and-clears, noop-while-serving, the auth-flap closure, and concurrent-publish serialization. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
bd2a043 to
d7ae7b4
Compare
…r 503 (RFC-029 W3, red) Flip cluster_boot_quarantines_graph_open_failures' lenient-mode assertions to the RFC-029 contract: a failed boot open is observable, configured state, not silent absence. GET /graphs must list both graphs with a status discriminator (serving|quarantined) and quarantine detail (last_error, attempts); routes under the quarantined graph answer 503 (configured-but-healing, retry later); a never-configured id stays 404, pinning Quarantined-vs-Gone. Strict-mode assertions unchanged. RED as predicted: GET /graphs lists only 'good' and the broken graph routes 404. Green arrives with the boot-wiring commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
| self.snapshot.store(Arc::new(RegistrySnapshot::with_quarantined( | ||
| new_graphs, | ||
| current.quarantined.clone(), | ||
| ))); |
There was a problem hiding this comment.
Test insert breaks quarantine invariant
Low Severity
The test-only insert path copies the existing quarantined map unchanged when adding a serving handle. If the key exists only under quarantine, the snapshot can hold the same key in both graphs and quarantined, unlike publish, which clears quarantine for that key.
Reviewed by Cursor Bugbot for commit 57fb5d0. Configure here.
…on GET /graphs (RFC-029 W3) Turns the previous commit's red test green: - open_multi_graph_state keeps each failed graph's startup config, builds a QuarantineInfo entry (attempts=1, the open error verbatim, policy_configured from the config), and retains clones of EVERY configured graph's startup config on GraphRouting.startup_configs for the upcoming supervision loop (healthy graphs included — a supervised reopen needs them too). Strict --require-all-graphs still bails before any quarantine state exists; the no-healthy-graphs bail is unchanged. - New AppState::new_multi_with_boot carries the supervision state; new_multi delegates with empty state so its six existing callers are untouched. - resolve_graph_handle maps Quarantined to a new ApiError::service_unavailable (503, no closed ErrorCode — mirroring the recovery-required 503 shape): configured-but-healing is 'retry later', not 'no such resource'. Never-configured ids stay 404. - GET /graphs merges quarantined graphs into the listing with an additive status discriminator (serde-defaulted to serving for wire compat) and a quarantine detail object (Unix-second timestamps, no new dependency); openapi.json regenerated. Full server suite (failpoints included) and api-types suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
…int (RFC-029 prep) Behavior-free skeleton so the upcoming red convergence tests compile: - src/supervisor.rs: SupervisorConfig (production 5s/x2/10min/10% jitter; fast_for_tests at ms scale) and GraphSupervisor with a drain-only run() (receives and discards reopen requests; no reopen yet). - AppState::spawn_supervision(config): idempotent one-shot channel setup + task spawn — the entry point both serve() and in-process tests use. Shutdown is free: the only sender lives in AppState, so when the state drops, recv() yields None and the loop exits. - shielded_write gains the (state, key) chokepoint: a shielded writer whose result carries recovery_required notifies request_reopen, arming the W2(b) supervised reopen for every write surface at one site. The merge's follow-up source delete threads state through. Full server suite green (scaffolding drains; nothing converges yet). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
…pervised reopen (RFC-029 W3+W2(b), red) Two bounded-poll convergence cells against the drain-only supervision skeleton: - multi_graph::boot_quarantined_graph_converges_to_serving_without_restart: a graph whose root is missing at boot quarantines (visible in GET /graphs with the verbatim open error), the root is initialized while the server runs (the deterministic transient-fault-clears model), and the listing must converge to serving without a restart, after which the graph's routes answer 200. RED: 'quarantined graph never converged to serving without a restart' at the 10s deadline. - failpoints::recovery_required_write_triggers_supervised_reopen_and_heals: the HTTP twin of rfc029_probe phases 2-3. A confirm-failure-injected mutation leaves an Armed sidecar; the next write surfaces 503 recovery_required (arming trigger B through the shielded-write chokepoint); polling the same write must converge to 200 and __recovery/ must empty. RED: 'RecoveryRequired never healed without a restart; last status: 503' at the 15s deadline. Green arrives with the next commit's supervision loop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 36c900d. Configure here.
| { | ||
| state.request_reopen(key); | ||
| } | ||
| result |
There was a problem hiding this comment.
Reopen signal dropped on disconnect
Medium Severity
request_reopen runs only after the handler awaits the spawned write task. If the client disconnects first, the detached protocol may still finish with RecoveryRequired, but the handler never reaches that hook, so W2(b) reopen is not triggered for that outcome unless another client writes later.
Reviewed by Cursor Bugbot for commit 36c900d. Configure here.
… W3+W2(b)) The real supervision loop replaces the drain-only skeleton, turning both red convergence cells green: - Seeds per-graph retry state from the registry's boot quarantine entries (trigger A) and selects over reopen notifications from the shielded-write chokepoint (trigger B — retried immediately, deduped while pending) and the earliest scheduled retry. - A due graph gets one full open_single_graph; success RCU-publishes the healed handle (clearing quarantine, or swapping a serving handle after a W2(b) reopen — in-flight requests finish on their own Arc); failure reschedules with capped exponential backoff plus jitter and refreshes the quarantine record, which no-ops while the graph still serves so a failed reopen never takes a healthy graph down. - One loop, not one task per graph: dedups notification storms and guarantees at most one in-flight open per root (two concurrent read-write opens would run redundant Full sweeps). Deliberate refinement of RFC-029 §5.1's per-graph wording; observable contract identical. - serve() starts supervision with production pacing before binding the listener; shutdown is free (the sender drops with the state). Full server suite green, failpoints included. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
- server.md: quarantine as a converging observable state — supervision backoff constants, GET /graphs status schema, 503-vs-404 semantics, and the RecoveryRequired self-heal on the write path. - errors.md: RecoveryRequired guidance rewritten — against the HTTP server the residual now heals automatically via the supervised reopen; embedded SDK callers keep the read-write reopen remedy. - invariants.md: note under the in-process recovery boundary gap that RFC-029 narrows its operational cost without relaxing any recovery rule (a reopen IS the documented resolution barrier). - testing.md: register the new W3/W2(b) coverage cells; AGENTS.md GET /graphs bullet mentions per-graph status. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5


What & why
This RFC proposes three coordinated changes to close structural defects in write-protocol cancellation safety and recovery residual handling:
W1 — Engine-owned write-protocol lifetime: Shield armed write protocols from caller cancellation by spawning protocol execution as a detached task. Caller disconnection means "stopped waiting for the result", never "abandoned the protocol".
W2 — Full recovery at the write-entry barrier: Escalate rollback-class recovery residuals to
RecoveryMode::Fullunder exclusive admission at the write-entry heal, instead of parking them for process restart. ResolvesRecoveryRequiredas a transient condition.W3 — Boot supervision: Replace silent graph exclusion on open failure with an observable
quarantinedregistry state and a capped-backoff re-open loop, making recovery from transient substrate errors automatic.Together these establish the invariant: Caller fate is not a protocol participant. An armed graph-write protocol completes or compensates under engine ownership regardless of caller cancellation, and resolving any recovery residual requires only exclusive gate acquisition — never process restart.
Backing issue / RFC
docs/rfcs/0029-write-lifetime-and-recovery-barriers.mdMotivation
A validated production incident showed the chain of failures: client timeout interrupted a multi-statement delete mid-protocol, leaving an
Armedsidecar; the live-handle heal is roll-forward-only and parks rollback-class residuals; the next write hit a stale manifest version; boot recovery hit a transient S3 error and silently quarantined the graph until redeploy. Every link was validated against current code (Lance 9.0.0-rc.1 at pinned rev).The RFC documents this incident in detail (§1) and validates the mechanism at each step. The long-run liability: cancel-safety is maintained by audit today, and every write-path change must re-reason about cancellation at every await. W1 replaces that with a structural property.
Design summary
W1 (Stage 1 — server boundary): Apply the existing spawn-and-clone idiom from
server_exportto every_aswriter handler. The HTTP response contract is unchanged; on client disconnect, the spawned task runs the protocol to terminal state (success or error) while the request future drops.W1 (Stage 2 — engine level): Move the shield into the engine via
Arc<Self>receivers or anArc<OmnigraphInner>split (shape TBD in review), so the property holds for embedded SDK callers.W2: When the write-entry heal encounters a rollback-class sidecar, escalate it to
RecoveryMode::Fullunder exclusive admission + existing schema → branch → table gates, instead of parking it. The heal re-reads the sidecar under those gates (existing behavior), dispatches withFullmode (new), and the triggering write proceeds from a fresh capture.W3: A graph whose open fails enters
Quarantined { since, attempts, last_error, retry_at }in the registry. One supervision task per quarantined graph retries the fullopen_single_graphwith capped exponential backoff (proposed: 5 s initial, ×2, 10 min cap).GET /graphsgains a per-graphstatusfield (serving|quarantined) with quarantine detail.Safety argument
https://claude.ai/code/session_01EqR7rPVfAT7UMiy85pG9b5
Note
High Risk
Changes multi-graph boot, all mutating HTTP paths, recovery/reopen behavior, and public
GET /graphscontract; incorrect supervision or registry swaps could affect availability or write semantics.Overview
Implements RFC-029 at the HTTP server: armed writes survive client disconnect, failed graph opens become visible and retryable, and
RecoveryRequiredcan trigger an in-process reopen instead of leaving the graph wedged until restart.W1 (cancellation shield): Mutating handlers (
mutate,load, schema apply, branch ops) run engine work throughshielded_write— atokio::spawnthat keeps the RFC-022 commit protocol running if the request future is dropped; admission slots release when the protocol finishes, not on disconnect.W3 + W2(b) (supervision): Boot retains per-graph
GraphStartupConfig, records open failures as quarantined registry entries (not silent omission), and starts aGraphSupervisorloop beforeserve()with capped backoff. The loop re-runsopen_single_graphand **publish**es healed handles;shielded_writecallsrequest_reopenwhen a write returnsRecoveryRequired. Routing returns 503 for quarantined graphs;GET /graphsaddsstatus(serving|quarantined) and optionalquarantinedetail (OpenAPI updated).Supporting changes:
GraphRegistry::publish/set_quarantined, engine proberfc029_probe.rs, serverfailpointsfeature + integration tests, docs/RFC-029, and CI runningomnigraph-serverfailpoint tests. Engine-level W1 Stage 2 and write-entry W2(a) remain future work per the RFC.Reviewed by Cursor Bugbot for commit dad1cf4. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR strengthens write recovery and graph availability at the server boundary. The main changes are:
Confidence Score: 5/5
No additional blocking issue qualifies for this follow-up.
Important Files Changed
Reviews (15): Last reviewed commit: "docs: quarantine supervision, transient ..." | Re-trigger Greptile
Context used: