From 0b893426e7ca9fd8289835d7c67647a68674ca81 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Wed, 27 May 2026 13:03:59 +0200 Subject: [PATCH] mr-668: cfg(test)-gate GraphRegistry::insert and its mutex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `insert` and the `mutate: Mutex<()>` that serializes it had no runtime consumer in v0.7.0 — the only insertion path at startup is `from_handles`, and runtime add/remove is deferred until a managed cluster catalog ships. Leaving both `pub` and live made them a "looks like API, isn't" footgun: a future change could build on `insert` without re-establishing the concurrency contract with an actual consumer in scope. Gate both together (`#[cfg(test)]` on the method, the field, and the `tokio::sync::Mutex` import) so the race-pinning tests still compile but production cannot reach them. When a real consumer ships, ungate both — they're a unit. Closes the "public API with no runtime consumer drifts toward incorrect" class. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/omnigraph-server/src/registry.rs | 26 +++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/crates/omnigraph-server/src/registry.rs b/crates/omnigraph-server/src/registry.rs index 918d033..1bc21a1 100644 --- a/crates/omnigraph-server/src/registry.rs +++ b/crates/omnigraph-server/src/registry.rs @@ -23,6 +23,7 @@ use std::sync::Arc; use arc_swap::ArcSwap; use omnigraph::db::Omnigraph; +#[cfg(test)] use tokio::sync::Mutex; use crate::identity::GraphKey; @@ -107,6 +108,11 @@ pub enum InsertError { pub struct GraphRegistry { snapshot: ArcSwap, + /// Serializes runtime mutations through [`GraphRegistry::insert`]. + /// Gated with `insert` because they share a single contract — if + /// the consumer goes away, so does the lock. Re-introducing one + /// requires re-introducing the other. + #[cfg(test)] mutate: Mutex<()>, } @@ -115,6 +121,7 @@ impl GraphRegistry { pub fn new() -> Self { Self { snapshot: ArcSwap::from_pointee(RegistrySnapshot::default()), + #[cfg(test)] mutate: Mutex::new(()), } } @@ -136,6 +143,7 @@ impl GraphRegistry { } Ok(Self { snapshot: ArcSwap::from_pointee(RegistrySnapshot::new(graphs)), + #[cfg(test)] mutate: Mutex::new(()), }) } @@ -179,13 +187,19 @@ impl GraphRegistry { /// Add a new handle. Async because the mutex is `tokio::sync::Mutex` /// (a future managed-catalog flow may hold it across `.await` points /// during atomic registry mutations). Rejects duplicate `GraphKey` - /// and duplicate `uri`. Currently unused at runtime — only construction - /// via `from_handles` runs at startup — but kept for the tests that - /// pin its concurrency contract. + /// and duplicate `uri`. /// - /// Race semantics (pinned by `concurrent_insert_same_key_one_succeeds_one_errors`): - /// under two concurrent calls with the same key, exactly one returns - /// `Ok(())` and the other returns `Err(InsertError::DuplicateKey(_))`. + /// **Test-only surface.** No production code reaches this — startup + /// uses `from_handles`, and runtime add/remove is deferred. The + /// race-contract tests below pin the mutex linearization point so + /// that when a real consumer ships (managed cluster catalog), the + /// concurrency contract is already proven. Ungate by removing + /// `#[cfg(test)]` once that consumer is in scope. + /// + /// Race semantics (pinned by `concurrent_insert_same_key_exactly_one_succeeds`): + /// under N concurrent calls with the same key, exactly one returns + /// `Ok(())` and the rest return `Err(InsertError::DuplicateKey(_))`. + #[cfg(test)] pub async fn insert(&self, handle: Arc) -> Result<(), InsertError> { let _guard = self.mutate.lock().await; let current = self.snapshot.load();