Skip to content

feat(server): own writes beyond request cancellation - #490

Open
ragnorc wants to merge 3 commits into
rfc-031-graph-supervisionfrom
rfc-031-tracked-writes
Open

feat(server): own writes beyond request cancellation#490
ragnorc wants to merge 3 commits into
rfc-031-graph-supervisionfrom
rfc-031-tracked-writes

Conversation

@ragnorc

@ragnorc ragnorc commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a TrackedWriteExecutor whose owned task retains admission, handle, actor, and inputs until terminal engine completion
  • make HTTP future cancellation drop only the result receiver, never the logical write
  • route mutate/change, stored mutations, schema apply, load/ingest, branch create/delete, and merge through the executor
  • keep merge plus optional source deletion in one task envelope while preserving separate deletion failure reporting
  • detect RecoveryRequired before HTTP conversion, mark the graph recovering, retain the blocking operation ID, and wake the supervisor without replaying the request
  • catch executor-boundary panics and schedule a conservative recovery scan
  • close write admission and drain tracked writes before stopping supervisors during graceful shutdown
  • update server/error docs, release notes, testing guidance, dependencies, and generated OpenAPI

Stacked 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

  • real HTTP/1.1 disconnect, HTTP/2 reset, and timeout-middleware tests after sidecar arm
  • response receiver abort, admission retention/release, panic recovery notification, shutdown drain, and no replay
  • full server unit/integration/OpenAPI suite
  • cargo test --workspace --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints
  • cargo test -p omnigraph-server --locked --features failpoints --test write_cancellation
  • default and failpoint-superset workspace Clippy with warnings denied
  • cargo fmt --all --check

No 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 TrackedWriteExecutor owns 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. RecoveryRequired and 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.

  • Routes all served mutation, schema, ingestion, branch, merge, and stored-mutation paths through TrackedWriteExecutor.
  • Marks graphs recovering and wakes supervision when tracked writes panic or return RecoveryRequired.
  • Closes write admission and drains tracked work before stopping graph supervisors during graceful shutdown.
  • Adds real HTTP/1.1 disconnect, HTTP/2 reset, timeout-middleware, recovery, admission-retention, and shutdown tests.
  • Documents cancellation semantics, uncertain client outcomes, recovery behavior, and the thirty-second shutdown drain.

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

Filename Overview
crates/omnigraph-server/src/tracked_write.rs Adds cancellation-independent task ownership, retained admission, panic containment, recovery notification, and bounded shutdown draining.
crates/omnigraph-server/src/handlers.rs Moves all HTTP-reachable graph writers into the tracked executor while preserving actor identity, policy checks, outputs, and non-fatal post-merge deletion reporting.
crates/omnigraph-server/src/lib.rs Adds the executor to application state and coordinates admission closure, tracked-write draining, and supervisor shutdown.
crates/omnigraph-server/tests/write_cancellation.rs Exercises real transport cancellation and verifies that the armed mutation reaches terminal publication without replay.
crates/omnigraph-server/src/supervisor.rs Exposes the existing supervisor shutdown operation for the new server lifecycle sequence.

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
    end
Loading

Reviews (1): Last reviewed commit: "feat(server): own writes beyond request ..." | Re-trigger Greptile

Context used (4)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +1437 to +1440
shutdown_lifecycle
.write_executor
.drain(std::time::Duration::from_secs(30))
.await;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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(&registry));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant