feat(server): own writes beyond request cancellation - #490
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68cbf58b7d
ℹ️ 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".
| shutdown_lifecycle | ||
| .write_executor | ||
| .drain(std::time::Duration::from_secs(30)) | ||
| .await; |
There was a problem hiding this comment.
Enforce the 30-second cutoff on the Axum server
When a connected request's engine operation is still running after 30 seconds, drain only stops waiting for the TaskTracker; it neither terminates the tracked task nor releases the handler awaiting its oneshot result. Axum's graceful shutdown continues waiting for that in-flight handler, so axum::serve(...).await does not return and SIGTERM/deploy shutdown can hang indefinitely despite the documented 30-second limit. Apply the deadline to the overall connection/server drain or explicitly release pending handlers and tasks at the cutoff.
AGENTS.md reference: AGENTS.md:L191-L192
Useful? React with 👍 / 👎.
| GraphRegistry::from_handles(vec![handle]) | ||
| .expect("a single handle never collides on graph id"), | ||
| ); | ||
| let write_executor = tracked_write::TrackedWriteExecutor::new(Arc::clone(®istry)); |
There was a problem hiding this comment.
Start supervisors for every write executor
When an app is built through the public AppState::open* convenience path or AppState::new_multi, this executor can call mark_recovering, but those constructors pair it with SupervisorSet::idle(), which has no per-graph tasks. A RecoveryRequired result or panic therefore leaves the graph permanently in recovering: the wake has no consumer and every subsequent write returns 503 until restart. Start supervisors for these registries or make recovery scheduling independent of the constructor.
AGENTS.md reference: AGENTS.md:L166-L167
Useful? React with 👍 / 👎.
| } | ||
| _ => None, | ||
| }; | ||
| registry.mark_recovering(&graph_key, operation_id).await; |
There was a problem hiding this comment.
Preserve recovery wakes that arrive during refresh
With two already-owned writes on one graph, the first can trigger a supervisor refresh while the second waits behind the engine recovery gate. If the second returns RecoveryRequired after that refresh's sweep but before attempt_recovery stores its success state, this call marks the graph recovering, but the active attempt then unconditionally overwrites any Serving state with Ready; the second residual is left without another recovery attempt while write_ready is reported true. Associate recovery attempts with a generation and only transition to ready when no newer notification arrived.
AGENTS.md reference: AGENTS.md:L166-L167
Useful? React with 👍 / 👎.
d53d883 to
953b208
Compare
Summary
TrackedWriteExecutorwhose owned task retains admission, handle, actor, and inputs until terminal engine completionRecoveryRequiredbefore HTTP conversion, mark the graph recovering, retain the blocking operation ID, and wake the supervisor without replaying the requestStacked on the configured graph supervision PR. Shielding guarantees server-side terminal execution, not client certainty; durable request identity/outcome lookup remains a separate idempotency follow-up.
Verification
cargo test --workspace --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpointscargo test -p omnigraph-server --locked --features failpoints --test write_cancellationcargo fmt --all --checkNo storage-format migration and no manual repair endpoint.
Note
High Risk
Changes the durability and cancellation semantics of every served write path, plus admission retention, recovery scheduling, and shutdown drain. Bugs here can leave writes half-applied, leak capacity, or mis-schedule recovery.
Overview
Served writes now outlive HTTP cancellation. A new
TrackedWriteExecutorowns each mutation after auth/validation, retaining the admission guard and engine inputs until a terminal result. Disconnect, reset, or timeout drops only the response receiver—never the write itself.All write surfaces (
mutate/change, schema apply, load/ingest/graph-batch, branch create/delete/merge) route through the executor.RecoveryRequiredand panics mark the graph recovering and wake the supervisor without replaying the request. Merge-plus-optional-delete stays one owned task while still reporting delete failures separately.Graceful shutdown closes write admission first, drains owned writes for up to 30s, then stops supervisors. Docs and failpoint tests cover HTTP/1 disconnect, HTTP/2 reset, and timeout middleware.
Reviewed by Cursor Bugbot for commit 68cbf58. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
The PR introduces server-owned tracked tasks so HTTP cancellation drops only the response receiver while admitted graph writes continue to an engine-terminal result.
TrackedWriteExecutor.RecoveryRequired.Confidence Score: 5/5
The PR appears safe to merge with no concrete unacknowledged correctness or security defects identified.
All served write paths consistently transfer ownership to the tracked executor, cancellation retains the operation and workload admission, recovery signals reach graph supervision, and the documented bounded-shutdown behavior matches the implementation.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant HTTP as Axum handler participant Exec as TrackedWriteExecutor participant Engine participant Registry participant Supervisor Client->>HTTP: Submit authorized write HTTP->>Exec: execute(handle, admission, operation) Exec->>Engine: Spawn owned operation alt Client disconnects or response times out Client--xHTTP: Drop response future Note over Exec,Engine: Owned task and admission remain alive end alt Engine completes normally Engine-->>Exec: Terminal result Exec-->>HTTP: Send result if receiver remains else RecoveryRequired or panic Engine-->>Exec: Error or panic Exec->>Registry: mark_recovering(operation_id?) Registry->>Supervisor: Wake recovery loop Exec-->>HTTP: Structured error if receiver remains endReviews (1): Last reviewed commit: "feat(server): own writes beyond request ..." | Re-trigger Greptile
Context used (4)