From ca474e23b10904bd1158b5ed146a2b930dad099c Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Wed, 27 May 2026 12:02:51 +0200 Subject: [PATCH] mr-668: derive any_per_graph_policy on RegistrySnapshot; simplify dup check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AppState::requires_bearer_auth` walked the entire registry per request (cloning Arcs into a `Vec`, then `.iter().any(|h| h.policy .is_some())`) to decide whether the auth middleware should challenge. The walk is unnecessary — the answer only changes when the registry mutates, which is exactly the moment a new snapshot is constructed. Move the flag onto the snapshot itself: * `RegistrySnapshot { graphs, any_per_graph_policy: bool }`. * `RegistrySnapshot::new(graphs)` is the only construction path — it derives the flag from `graphs.values().any(|h| h.policy .is_some())` so the cached value can't drift from the source data. * `Default` delegates to `new(HashMap::new())`. * `GraphRegistry::from_handles` and `insert` build snapshots via `RegistrySnapshot::new(...)`. * `GraphRegistry::snapshot_ref()` exposes the current snapshot through an `arc_swap::Guard`; callers that need cached derived state go through this accessor (callers that only want `graphs` still use `list` / `get`). `requires_bearer_auth` becomes one `ArcSwap::load` + bool read. Also (drive-by, same file, same hunk): replace the dead `if let Some(other) = seen_uris.get(...)` + `let _ = other;` pattern in `from_handles` with a plain `seen_uris.contains_key(...)`. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/omnigraph-server/src/lib.rs | 6 ++-- crates/omnigraph-server/src/registry.rs | 46 ++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index 5e64b07..0b005e3 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -483,8 +483,10 @@ impl AppState { return true; } // Any per-graph policy also requires auth — otherwise the - // policy gate would receive unauthenticated requests. - self.registry.list().iter().any(|h| h.policy.is_some()) + // policy gate would receive unauthenticated requests. The flag + // is derived at snapshot construction (`RegistrySnapshot::new`) + // so this is an O(1) ArcSwap load — not a per-request walk. + self.registry.snapshot_ref().any_per_graph_policy } fn authenticate_bearer_token(&self, provided_token: &str) -> Option { diff --git a/crates/omnigraph-server/src/registry.rs b/crates/omnigraph-server/src/registry.rs index b1c879e..918d033 100644 --- a/crates/omnigraph-server/src/registry.rs +++ b/crates/omnigraph-server/src/registry.rs @@ -49,9 +49,38 @@ pub struct GraphHandle { /// Immutable snapshot of the registry's current state. Replaced atomically /// via `ArcSwap`; readers see a consistent view of all graphs without locking. -#[derive(Default)] +/// +/// Derived state (`any_per_graph_policy`) is computed at snapshot +/// construction so request-time middleware doesn't have to walk the +/// graph map every call. Construct only via [`RegistrySnapshot::new`] +/// (or `Default`) so the field stays in sync with `graphs`. pub struct RegistrySnapshot { pub graphs: HashMap>, + /// `true` iff any registered graph has a per-graph policy installed. + /// Used by `AppState::requires_bearer_auth` to decide whether the + /// auth middleware should challenge a request — a per-graph policy + /// implies bearer auth is required even when no server-level tokens + /// or policy are configured. + pub any_per_graph_policy: bool, +} + +impl RegistrySnapshot { + /// Build a snapshot from a graph map, deriving cached fields. + /// The only construction path — direct struct-literal use elsewhere + /// would let derived state drift from `graphs`. + pub fn new(graphs: HashMap>) -> Self { + let any_per_graph_policy = graphs.values().any(|h| h.policy.is_some()); + Self { + graphs, + any_per_graph_policy, + } + } +} + +impl Default for RegistrySnapshot { + fn default() -> Self { + Self::new(HashMap::new()) + } } /// Result of a registry lookup. Two-valued — `Tombstoned` deferred with DELETE. @@ -99,19 +128,26 @@ impl GraphRegistry { if graphs.contains_key(&handle.key) { return Err(InsertError::DuplicateKey(handle.key.clone())); } - if let Some(other) = seen_uris.get(&handle.uri) { - let _ = other; // existing key shown in the error message via uri + if seen_uris.contains_key(&handle.uri) { return Err(InsertError::DuplicateUri(handle.uri.clone())); } seen_uris.insert(handle.uri.clone(), handle.key.clone()); graphs.insert(handle.key.clone(), handle); } Ok(Self { - snapshot: ArcSwap::from_pointee(RegistrySnapshot { graphs }), + snapshot: ArcSwap::from_pointee(RegistrySnapshot::new(graphs)), mutate: Mutex::new(()), }) } + /// Lock-free snapshot read. Callers that need derived state cached + /// on the snapshot (e.g. `any_per_graph_policy`) go through here; + /// callers that only need values of `graphs` should use [`list`] + /// or [`get`]. + pub fn snapshot_ref(&self) -> arc_swap::Guard> { + self.snapshot.load() + } + /// Lock-free read. Returns `Ready` if the graph is in the current snapshot, /// `Gone` otherwise. pub fn get(&self, key: &GraphKey) -> RegistryLookup { @@ -164,7 +200,7 @@ impl GraphRegistry { let mut new_graphs = current.graphs.clone(); new_graphs.insert(handle.key.clone(), handle); self.snapshot - .store(Arc::new(RegistrySnapshot { graphs: new_graphs })); + .store(Arc::new(RegistrySnapshot::new(new_graphs))); Ok(()) } }