From 00522d6fc10749c759d2c257985657e40159cc6d Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Wed, 27 May 2026 13:50:25 +0200 Subject: [PATCH] mr-668: remove ServerMode, registry field, and the SINGLE_GRAPH sentinel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-1/C-2 introduced `GraphRouting` and pointed the middleware at it. This commit removes the legacy shape that's now dead: * `ServerMode` enum — deleted. Single mode's `uri` lives on `handle.uri`; multi mode's `config_path` lives on the `GraphRouting::Multi` arm. * `AppState.mode: ServerMode` field — deleted. * `AppState.registry: Arc` field — deleted. Multi mode's registry is on `GraphRouting::Multi { registry, .. }`; single mode has no registry at all. * `AppState::mode()`, `AppState::uri()`, `AppState::registry()` accessors — deleted. New `AppState::routing() -> &GraphRouting` is the single public entry point. * `SINGLE_GRAPH_KEY_ID` constant — deleted. `GraphHandle.key` is still required by the struct, but in single mode the key is now only a tracing label (`"default"`, inlined with a comment naming its sole remaining purpose). Single-mode flat routes never carry a `{graph_id}` parameter, so the key is never compared against user input, and there is no registry where it could be a map key. C-1/C-2 already removed the registry walk that the sentinel was named for. Callers migrated: * `build_app` (lib.rs:944) — matches on `state.routing()` instead of `state.mode()`. * `server_graphs_list` (lib.rs:1162) — destructures the Multi arm to get the registry; Single arm short-circuits to 405. * `server_openapi` (lib.rs:1217) — matches the Multi arm for the cluster-prefix rewrite. * `tests/server.rs:3735` — the B2 footgun regression test now matches on `state.routing()` to extract the single-mode handle (the test's earlier `state.registry().list().next()` shape was the closest pre-fix analog to "embedded consumer reaches the engine"; the new shape is more direct). Closes the entire "single mode forced through a multi-mode abstraction" class. After this commit: * No magic sentinel as a routing key. * No `single_mode_handle` walk-and-assert helper. * No 500-class "programmer error" branch in the middleware. * No two-field discriminant on `AppState` where one would do. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/omnigraph-server/src/lib.rs | 173 ++++++++---------------- crates/omnigraph-server/tests/server.rs | 12 +- docs/releases/v0.7.0.md | 2 +- 3 files changed, 68 insertions(+), 119 deletions(-) diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index 4b4dfd8..4aa20f9 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -179,63 +179,32 @@ pub struct GraphStartupConfig { pub policy_file: Option, } -/// Server runtime topology. Single mode = legacy `omnigraph-server ` -/// invocation, one graph, flat HTTP routes. Multi mode = `--config -/// omnigraph.yaml` with a `graphs:` map, N graphs, cluster routes -/// (`/graphs/{graph_id}/...`). Mode is determined at startup by -/// `load_server_settings`. +/// Runtime routing for the server. Single mode = legacy +/// `omnigraph-server ` invocation, one graph, flat HTTP routes. +/// Multi mode = `--config omnigraph.yaml` with a non-empty `graphs:` +/// map, N graphs, cluster routes (`/graphs/{graph_id}/...`). Mode is +/// determined at startup by `load_server_settings`. /// -/// Both modes share the same handler bodies — the routing middleware -/// (`resolve_graph_handle`) injects `Arc` as a request -/// extension so handlers never see the mode discriminator directly. -#[derive(Clone, Debug)] -pub enum ServerMode { - /// Single-graph invocation. The `uri` is the only graph's URI. - /// Backward compatible with v0.6.0 deployments. - Single { uri: String }, - /// Multi-graph invocation (MR-668). `config_path` is the - /// `omnigraph.yaml` the server reads at startup. The server treats - /// the file as operator-owned and never writes it; runtime - /// add/remove (deferred) is the only path that would touch it. - Multi { config_path: Option }, -} - -/// Sentinel `GraphId` for single-graph mode. The single graph is -/// registered under this key in the registry; the routing middleware -/// uses it to inject the handle on every flat-route request. The -/// value is operator-invisible — it does NOT need to be unique with -/// any user-facing graph id since single-mode never serves cluster -/// routes. -/// -/// **Deprecated in C-1, removed in C-3 of this PR sequence.** The -/// new `GraphRouting::Single` arm carries `Arc` directly, -/// so there is no longer a need to thread the single-mode handle -/// through a one-element registry under a magic key. -pub(crate) const SINGLE_GRAPH_KEY_ID: &str = "default"; - -/// Runtime routing for the server. Replaces the prior pairing of -/// `ServerMode` (discriminant + Single's `uri`) with `Arc` -/// (always populated, even when serving one graph through a sentinel -/// key). In single-graph mode there is no registry to traverse — -/// the single handle lives here directly. In multi-graph mode the +/// In single mode the handle lives here directly — there is no +/// registry, no sentinel key, no walk-and-assert. In multi mode the /// registry carries N handles and the middleware dispatches on the /// URL's `{graph_id}` segment. /// -/// Closes the "single mode is forced through a multi-mode abstraction" -/// class: `SINGLE_GRAPH_KEY_ID`, the registry-walk-and-assert helper -/// (`single_mode_handle`), and the 500-class "programmer error" -/// branch all go away in C-3. +/// Both modes share the same handler bodies — the routing middleware +/// (`resolve_graph_handle`) injects `Arc` as a request +/// extension so handlers never see the routing discriminator. #[derive(Clone)] pub enum GraphRouting { /// Single-graph deployment: one handle, flat routes (`/snapshot`, /// `/read`, …). The `handle.uri` field carries the URI the engine - /// was opened from (replaces `ServerMode::Single { uri }`). + /// was opened from. Backward compatible with v0.6.0 deployments. Single { handle: Arc }, /// Multi-graph deployment: many handles, cluster routes /// (`/graphs/{graph_id}/...`). `config_path` is the `omnigraph.yaml` /// the server reads at startup; preserved here so future runtime /// mutation (deferred) can find the source of truth without - /// re-parsing CLI args (replaces `ServerMode::Multi { config_path }`). + /// re-parsing CLI args. The server treats the file as + /// operator-owned and never writes it. Multi { registry: Arc, config_path: Option, @@ -244,24 +213,12 @@ pub enum GraphRouting { #[derive(Clone)] pub struct AppState { - /// Topology + (single mode only) the single graph's URI for - /// startup wiring. The registry below is the runtime source of - /// truth in multi mode; the routing field below is the new - /// source-of-truth for both modes. - /// - /// **C-1 transitional**: `mode` and `registry` are kept populated - /// in lockstep with `routing` so middleware that reads from either - /// shape continues to work. C-2 switches the middleware to - /// `routing`; C-3 removes the two legacy fields. - mode: ServerMode, - /// MR-686 + MR-668: legacy multi-graph registry. C-2 stops the - /// middleware from reading this in single mode; C-3 removes the - /// field entirely (multi mode reads via `routing`). - registry: Arc, - /// New runtime routing source of truth (C-1). Holds the single - /// handle directly in single mode (no registry-walk-and-assert) - /// and the registry + config_path in multi mode. Middleware - /// migration to read from here lands in C-2. + /// Runtime routing — the single source of truth for where each + /// request's graph lives. Single mode holds the handle directly; + /// multi mode holds the registry + config path. Both arms are + /// the same shape from a handler's perspective: middleware + /// extracts an `Arc` and injects it as a request + /// extension. routing: GraphRouting, /// Per-actor admission control. Process-wide (not per-graph) — /// see MR-668 decision Q6. @@ -431,9 +388,10 @@ impl AppState { } /// Single-mode shared construction: wraps the bare engine + per-graph - /// policy in a `GraphHandle` registered under the sentinel - /// `SINGLE_GRAPH_KEY_ID` key. Per-graph policy enforcement on the - /// engine (MR-722) is re-applied via `Omnigraph::with_policy`. + /// policy in a `GraphHandle` carried directly by `GraphRouting::Single`. + /// Per-graph policy enforcement on the engine (MR-722) is re-applied + /// via `Omnigraph::with_policy` so HTTP and engine layers can never + /// diverge. fn build_single_mode( uri: String, db: Omnigraph, @@ -451,27 +409,26 @@ impl AppState { } else { db }; + // `GraphHandle.key` is required by the struct, but in single + // mode it is never a registry key (there's no registry) and + // never compared against user input (routes are flat, no + // `{graph_id}` parameter). The label appears only in tracing + // output from `resolve_graph_handle`. The literal below is a + // log label, not a routing key — when the future cluster + // catalog ships, single mode may carry the catalog-assigned + // id here instead. let key = GraphKey::cluster( - GraphId::try_from(SINGLE_GRAPH_KEY_ID) - .expect("single-graph sentinel key must validate"), + GraphId::try_from("default") + .expect("'default' is a valid GraphId log label"), ); let handle = Arc::new(GraphHandle { key, - uri: uri.clone(), + uri, engine: Arc::new(db), policy: policy_engine, }); - let registry = Arc::new( - GraphRegistry::from_handles(vec![Arc::clone(&handle)]) - .expect("single-mode registry construction is infallible"), - ); - let routing = GraphRouting::Single { - handle: Arc::clone(&handle), - }; Self { - mode: ServerMode::Single { uri }, - registry, - routing, + routing: GraphRouting::Single { handle }, workload, bearer_tokens, server_policy: None, @@ -494,38 +451,24 @@ impl AppState { ) -> std::result::Result { let bearer_tokens = hash_bearer_tokens(bearer_tokens); let registry = Arc::new(GraphRegistry::from_handles(handles)?); - let routing = GraphRouting::Multi { - registry: Arc::clone(®istry), - config_path: config_path.clone(), - }; Ok(Self { - mode: ServerMode::Multi { config_path }, - registry, - routing, + routing: GraphRouting::Multi { + registry, + config_path, + }, workload: Arc::new(workload), bearer_tokens, server_policy: server_policy.map(Arc::new), }) } - /// Topology accessor. Handlers don't typically inspect this — they - /// extract `Arc` via the routing middleware — but - /// `build_app` does to decide flat vs nested route mounting. - pub fn mode(&self) -> &ServerMode { - &self.mode - } - - /// The configured URI in single mode. Returns `None` in multi mode — - /// each graph has its own URI on `handle.uri` instead. - pub fn uri(&self) -> Option<&str> { - match &self.mode { - ServerMode::Single { uri } => Some(uri.as_str()), - ServerMode::Multi { .. } => None, - } - } - - pub fn registry(&self) -> &Arc { - &self.registry + /// Runtime routing accessor. Handlers don't typically inspect this — + /// they extract `Arc` via the routing middleware — but + /// `build_app` matches on it to decide flat vs nested route + /// mounting, and a handful of management endpoints (`GET /graphs`, + /// the OpenAPI cluster rewrite) match on the discriminant. + pub fn routing(&self) -> &GraphRouting { + &self.routing } fn requires_bearer_auth(&self) -> bool { @@ -998,9 +941,9 @@ pub fn build_app(state: AppState) -> Router { // Mount the protected routes differently per mode: // * Single → flat routes (legacy: `/snapshot`, `/read`, etc.) // * Multi → nested under `/graphs/{graph_id}/...` - let protected: Router = match state.mode() { - ServerMode::Single { .. } => per_graph_protected.merge(management), - ServerMode::Multi { .. } => Router::new() + let protected: Router = match state.routing() { + GraphRouting::Single { .. } => per_graph_protected.merge(management), + GraphRouting::Multi { .. } => Router::new() .nest("/graphs/{graph_id}", per_graph_protected) .merge(management), }; @@ -1222,11 +1165,14 @@ async fn server_graphs_list( ) -> std::result::Result, ApiError> { // 405 in single mode — there's no registry to enumerate, and the // legacy URL surface didn't expose this endpoint. - if matches!(state.mode(), ServerMode::Single { .. }) { - return Err(ApiError::method_not_allowed( - "GET /graphs is only available in multi-graph mode", - )); - } + let registry = match state.routing() { + GraphRouting::Single { .. } => { + return Err(ApiError::method_not_allowed( + "GET /graphs is only available in multi-graph mode", + )); + } + GraphRouting::Multi { registry, .. } => registry, + }; // Server-level Cedar gate. `state.server_policy` is loaded from // `server.policy.file` in `omnigraph.yaml` at startup. When no @@ -1246,8 +1192,7 @@ async fn server_graphs_list( }, )?; - let mut graphs: Vec = state - .registry() + let mut graphs: Vec = registry .list() .into_iter() .map(|handle| GraphInfo { @@ -1268,7 +1213,7 @@ async fn server_openapi(State(state): State) -> Json` was reused without `with_policy`, so any -/// caller reaching through `state.registry().list()` could bypass Cedar. +/// caller reaching through `state.routing()` could bypass Cedar. /// /// This test reaches the engine the same way an embedded SDK consumer /// or future routing code path would, and asserts the policy still @@ -3707,6 +3707,7 @@ async fn ingest_per_actor_admission_cap_returns_429() { /// the policy's allowed group" — i.e., authenticated-but-unauthorised. #[tokio::test(flavor = "multi_thread")] async fn engine_layer_policy_fires_via_direct_arc_omnigraph_from_new_single() { + use omnigraph_server::GraphRouting; let temp = init_loaded_graph().await; let graph = graph_path(temp.path()); let db = Omnigraph::open(graph.to_str().unwrap()).await.unwrap(); @@ -3728,11 +3729,14 @@ async fn engine_layer_policy_fires_via_direct_arc_omnigraph_from_new_single() { workload, ); - // Reach into the registry and pull the engine the same way an + // Reach into the routing and pull the engine the same way an // embedded consumer holding `Arc` would. If `new_single` // failed to apply `with_policy` to the engine, this `mutate_as` // would succeed — the HTTP-layer is bypassed entirely. - let handle = state.registry().list().into_iter().next().expect("handle"); + let handle = match state.routing() { + GraphRouting::Single { handle } => Arc::clone(handle), + GraphRouting::Multi { .. } => panic!("expected single-mode routing"), + }; let engine = Arc::clone(&handle.engine); let mut params: omnigraph_compiler::ParamMap = Default::default(); diff --git a/docs/releases/v0.7.0.md b/docs/releases/v0.7.0.md index 1b619e6..e20a761 100644 --- a/docs/releases/v0.7.0.md +++ b/docs/releases/v0.7.0.md @@ -8,7 +8,7 @@ Runtime add/remove (`POST /graphs`, `DELETE /graphs/{id}`, `omnigraph graphs cre - **Multi-graph deployments lose flat routes.** Single-graph invocation (`omnigraph-server `) is unchanged — same flat `/snapshot`, `/read`, `/branches`, etc. Multi-graph deployments serve those routes under `/graphs/{graph_id}/...`; bare flat paths return 404 in multi mode. - **`ServerConfig` shape change** (programmatic embedders only): `ServerConfig { uri, policy_file }` is replaced by `ServerConfig { mode: ServerConfigMode }`, where `ServerConfigMode = Single { uri, policy_file } | Multi { graphs, config_path, server_policy_file }`. Callers that use `load_server_settings` are unaffected; callers that construct `ServerConfig` directly need to wrap their fields in `ServerConfigMode::Single`. -- **`AppState::uri()`** now returns `Option<&str>` (was `&str`). Returns `Some` in single mode, `None` in multi mode — per-graph URIs live on `GraphHandle.uri` instead. +- **`AppState`'s routing surface** is `AppState::routing() -> &GraphRouting`, where `GraphRouting = Single { handle } | Multi { registry, config_path }`. The previous `AppState::uri()`, `AppState::mode()`, `AppState::registry()` accessors and the `ServerMode` enum are gone — embedders read `state.routing()` and match on the arm they need. Per-graph URIs live on `handle.uri`. - **`AppState::new_multi`** is the new multi-graph constructor. Single-mode `new_*` / `open_*` constructors are unchanged. - **`AuthenticatedActor(Arc)` → `ResolvedActor { actor_id, tenant_id, scopes, source }`** (programmatic embedders only). The struct shape changes, but the HTTP contract — bearer auth, MR-731 spoof defense — is unchanged. Cluster-mode call sites construct with `tenant_id: None`, `scopes: vec![Scope::Full]`, `source: AuthSource::Static`. Forward-compat for Cloud mode (RFC 0003) and OAuth provider (RFC 0004).