From 76f6b7c3b7907c1764b16fbf5889d15b6bb4df38 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 19 Aug 2026 00:16:34 +0200 Subject: [PATCH 1/2] fix(otel): let the SDK resolve hive-c0re's OTLP endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hive-c0re's container-resource exporter has POSTed to a 404 for as long as it has existed, silently: it passed the collector's base address to `with_endpoint`, which the SDK takes verbatim, so every export went to `/` instead of `/v1/metrics`. Nothing reported it — OTLP export failures go to an error handler no binary here installs — so the daemon logged "exporter enabled" and delivered nothing. VictoriaMetrics has never held a sample under `service.name=hyperhive-c0re`. Fix the way the rest of the repo already resolves an endpoint: an endpoint option names a BASE, and the layer that knows the signal appends to it. `hive-metric` — same SDK, same collector — never calls `with_endpoint`, and `docs/observability.md` documents the append as system behaviour; the one place a full path is spelled out is the VictoriaMetrics exporter, because its far end is not a standard OTLP path. So drop the call. The builder is now byte-identical to hive-metric's, and hive-c0re's unit carries the standard `OTEL_EXPORTER_OTLP_ENDPOINT` for the SDK to read. The address is bound once in nix and consumed twice, so what a hive hands its agents and what it exports to itself cannot drift. The enable signal moves to that same standard variable: "configured" and "where it actually goes" become one string rather than two that agree by convention. `HYPERHIVE_OTEL_*` keeps its own job, the agent-config transport meta.rs reads — a name the SDK has never known, which is the bug. The test changes shape with the fix. The old one asserted a URL this module built; the new one pins that the exporter is gated on the variable the SDK itself reads, because the fix is now an absence and an absence is what a later "the endpoint is right there, just pass it" edit puts back. Refs #3402 --- hive-c0re/src/stats/otel_metrics.rs | 80 +++++++++++++++++++--- nix/host-modules/hive-c0re/environment.nix | 20 +++++- 2 files changed, 90 insertions(+), 10 deletions(-) diff --git a/hive-c0re/src/stats/otel_metrics.rs b/hive-c0re/src/stats/otel_metrics.rs index 0883f04e..0adf3f6a 100644 --- a/hive-c0re/src/stats/otel_metrics.rs +++ b/hive-c0re/src/stats/otel_metrics.rs @@ -1,8 +1,9 @@ //! Per-agent container-resource OTEL export. hive-c0re already samples each //! agent container's cgroup load for the dashboard //! ([`super::container_stats`]); this rides those same gauges out to the -//! hive's own collector — the same first hop the agents use, arriving as -//! `HYPERHIVE_OTEL_ENDPOINT` (see `nix/host-modules/hive-c0re`). No toggle of +//! hive's own collector — the same first hop the agents use, named by the +//! standard `OTEL_EXPORTER_OTLP_ENDPOINT` (see `nix/host-modules/hive-c0re`, +//! which sets it from the same binding it hands agents). No toggle of //! its own, and no credential: a hive's collector takes unauthenticated OTLP //! on the bridge, and the only hop that presents anything is the swarm tier's, //! which is the one that leaves the swarm. @@ -45,8 +46,8 @@ static SNAPSHOT: OnceLock>>> = OnceLock::new(); static PROVIDER: OnceLock = OnceLock::new(); /// Spawn the container-resource OTEL exporter if OTEL is configured -/// (`HYPERHIVE_OTEL_ENDPOINT` non-empty — the same enable signal -/// `meta::otel_config` uses). No-op otherwise. Call once at startup. +/// (`OTEL_EXPORTER_OTLP_ENDPOINT` non-empty). No-op otherwise. Call once at +/// startup. pub fn spawn_exporter(hyperhive_flake: &str) { let Some(endpoint) = endpoint() else { tracing::debug!("otel container-metrics: no endpoint configured, exporter disabled"); @@ -69,7 +70,7 @@ pub fn spawn_exporter(hyperhive_flake: &str) { } }); - match build_provider(&endpoint, interval, snapshot, hyperhive_flake) { + match build_provider(interval, snapshot, hyperhive_flake) { Ok(provider) => { let _ = PROVIDER.set(provider); tracing::info!(%endpoint, ?interval, "otel container-metrics: exporter enabled"); @@ -81,7 +82,6 @@ pub fn spawn_exporter(hyperhive_flake: &str) { } fn build_provider( - endpoint: &str, interval: Duration, snapshot: Arc>>, hyperhive_flake: &str, @@ -90,12 +90,24 @@ fn build_provider( // hive-metric). The Claude SDK path honours `HYPERHIVE_OTEL_PROTOCOL` for // its own export; this exporter is always http/json. // + // The address is deliberately NOT passed here. The SDK reads + // `OTEL_EXPORTER_OTLP_ENDPOINT` itself and appends the signal path + // (`/v1/metrics`); `with_endpoint` is taken **verbatim**, so handing it a + // collector's base address POSTs to `/` and 404s on every export — with + // nothing logged, because OTLP export failures go to an error handler no + // binary here installs. That is not hypothetical: it was this module's + // behaviour for its whole existence, and no sample ever reached the store. + // An endpoint names a BASE throughout hyperhive and the + // layer that knows the signal appends to it (the one exception is the + // VictoriaMetrics exporter, whose far end is not a standard OTLP path). + // Construction is identical to `hive-metric`'s on purpose: two producers, + // one collector, one way to resolve the address. + // // No auth headers: the destination is this hive's own collector, which // takes unauthenticated OTLP on the bridge. Adding one here would put the // upstream credential on a hop that never uses it. let exporter = MetricExporter::builder() .with_http() - .with_endpoint(endpoint) .with_protocol(Protocol::HttpJson) .build() .context("build OTLP metric exporter")?; @@ -268,9 +280,12 @@ fn host_arch() -> &'static str { } } -/// `HYPERHIVE_OTEL_ENDPOINT`, non-empty. The enable signal. +/// `OTEL_EXPORTER_OTLP_ENDPOINT`, non-empty. The enable signal — and the very +/// variable the SDK reads to build the exporter's URL, so "configured" and +/// "where it goes" cannot disagree. Read here only to decide whether to start +/// at all; the value is never handed to the builder (see `build_provider`). fn endpoint() -> Option { - std::env::var("HYPERHIVE_OTEL_ENDPOINT") + std::env::var("OTEL_EXPORTER_OTLP_ENDPOINT") .ok() .map(|s| s.trim().to_owned()) .filter(|s| !s.is_empty()) @@ -351,4 +366,51 @@ mod tests { #[cfg(target_arch = "aarch64")] assert_eq!(host_arch(), "arm64"); } + + /// The exporter's destination must come from the standard OTLP variable, + /// because that is the only spelling the SDK appends the signal path to. + /// + /// Asserted rather than left to review because the fix here is an + /// *absence* — no `with_endpoint` call — and an absence is exactly what + /// a later "the endpoint is right there, just pass it" edit restores. If + /// this exporter is ever gated on a variable the SDK does not itself read, + /// it resumes posting to a base URL that 404s in silence. + #[test] + fn endpoint_is_the_standard_otlp_var() { + // SAFETY: single-threaded mutation of a process env var no other test + // in this module asserts on; both names are cleared before returning. + unsafe { + std::env::remove_var("OTEL_EXPORTER_OTLP_ENDPOINT"); + std::env::set_var("HYPERHIVE_OTEL_ENDPOINT", "http://hyperhive.invalid:4318"); + } + assert_eq!( + endpoint(), + None, + "the hive-wide agent-config variable must NOT enable this exporter — \ + the SDK does not read it, so its address would never reach the builder" + ); + + unsafe { + std::env::set_var("OTEL_EXPORTER_OTLP_ENDPOINT", " http://10.42.0.1:4318 "); + } + assert_eq!( + endpoint(), + Some("http://10.42.0.1:4318".to_owned()), + "the standard variable enables the exporter, trimmed" + ); + + unsafe { + std::env::set_var("OTEL_EXPORTER_OTLP_ENDPOINT", " "); + } + assert_eq!( + endpoint(), + None, + "whitespace-only is not a configured endpoint" + ); + + unsafe { + std::env::remove_var("OTEL_EXPORTER_OTLP_ENDPOINT"); + std::env::remove_var("HYPERHIVE_OTEL_ENDPOINT"); + } + } } diff --git a/nix/host-modules/hive-c0re/environment.nix b/nix/host-modules/hive-c0re/environment.nix index 64975b30..92b612e8 100644 --- a/nix/host-modules/hive-c0re/environment.nix +++ b/nix/host-modules/hive-c0re/environment.nix @@ -81,6 +81,11 @@ in # don't render no-op env lines. let otel = config.services.hyperhive.otel; + # The first hop, bound once and consumed twice below: what agents are + # handed, and where hive-c0re's own exporter sends. One binding so the + # address a hive tells its agents about and the one it uses itself + # cannot drift apart. + firstHop = "http://${config.services.hyperhive.network.bridgeIp}:${toString otel.collector.port}"; in { # `otel.endpoint` means "where telemetry ultimately goes" and keeps @@ -88,7 +93,20 @@ in # always this hive's own collector. Deriving it rather than # redefining `endpoint` is what lets every existing deployment keep # its configured value untouched. - HYPERHIVE_OTEL_ENDPOINT = "http://${config.services.hyperhive.network.bridgeIp}:${toString otel.collector.port}"; + HYPERHIVE_OTEL_ENDPOINT = firstHop; + # hive-c0re's OWN container-resource exporter (stats/otel_metrics.rs) + # reads the STANDARD OTLP variable — the same one hive-metric and every + # agent read — and lets the SDK resolve the URL, which appends the + # signal path (`/v1/metrics`). Handing the SDK an address + # programmatically instead takes it verbatim: it POSTs to the + # collector's root, gets a 404 on every export, and says nothing, + # because OTLP export failures go to an error handler no binary here + # installs — the daemon logged "exporter enabled" and never delivered a + # sample, for as long as it had that exporter. The variable above cannot + # replace this one: + # it is the agent-config transport meta.rs reads, and the SDK does not + # know that name. + OTEL_EXPORTER_OTLP_ENDPOINT = firstHop; # The first hop is the collector's OTLP/HTTP receiver, which speaks # protobuf regardless of what the upstream wants — `otel.protocol` # describes the *upstream* link, and the collector's own exporter is From af3a9e5433eb8e80737cfa21be5ea5f1da791956 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 19 Aug 2026 00:42:26 +0200 Subject: [PATCH 2/2] test(hive-c0re): one crate-wide lock for env-mutating tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding from argus. The new endpoint test carried a SAFETY comment claiming no other test in its module asserts on the variables it perturbs — the wrong boundary. The module is not the unit that shares the environment, the process is: meta.rs's render_flake_injects_otel_when_signalled mutates the same HYPERHIVE_OTEL_ENDPOINT, both land in the one hive-c0re test binary, and cargo runs it at default parallelism with no serialisation anywhere in the crate. Each test independently claimed exclusive ownership of shared global state, which is the instrument-that-looks-solid class the endpoint change's own gate reasoning warns about. Adds test_env with a single ENV_LOCK, taken by both. No new dependency: this is the pattern hive-bash-mcp and hive-agent already use, and hive-bash-mcp's helper records why it has to be crate-wide rather than per-module — two per-module mutexes serialise nothing against each other, which produced a CI-only flake there. The asymmetry that makes this hard to see locally is worth stating: an agent container has the hyperhive variables ambient-set, so a losing race still finds a plausible value and the test passes; the nix sandbox strips them, so only there can one thread delete a variable out from under another. Verified in that shape with `env -u HYPERHIVE_OTEL_ENDPOINT -u OTEL_EXPORTER_OTLP_ENDPOINT`, five consecutive runs green — a sanity check, not a proof, since a race cannot be shown absent by running. What makes it correct is structural: both tests take the same lock. Deliberately scoped to the pair that overlaps. meta.rs has three further env-mutating tests (HIVE_FORGE_URL twice, the TLS CA pair) that race with each other, untouched here and tracked separately, because the fix is not the mechanical one it looks like: std::sync::Mutex is not reentrant, so adding a lock to a test whose helpers also lock deadlocks. That needs reading per test rather than a sweep. --- hive-c0re/src/main.rs | 2 ++ hive-c0re/src/meta.rs | 8 ++++-- hive-c0re/src/stats/otel_metrics.rs | 10 ++++++-- hive-c0re/src/test_env.rs | 39 +++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 4 deletions(-) create mode 100644 hive-c0re/src/test_env.rs diff --git a/hive-c0re/src/main.rs b/hive-c0re/src/main.rs index 9515f47c..9efe1d51 100644 --- a/hive-c0re/src/main.rs +++ b/hive-c0re/src/main.rs @@ -36,6 +36,8 @@ mod socket_server; mod stats; mod stores; mod swarm_status; +#[cfg(test)] +mod test_env; mod webhook_secret; mod workers; diff --git a/hive-c0re/src/meta.rs b/hive-c0re/src/meta.rs index a17d0186..b3f220a6 100644 --- a/hive-c0re/src/meta.rs +++ b/hive-c0re/src/meta.rs @@ -2222,8 +2222,12 @@ mod tests { // no endpoint signal, no hyperhive.otel lines are emitted (agents // keep the the harness modules disabled default). // - // SAFETY: single-threaded mutation of process env vars no other - // test asserts on; restored before returning. + // Serialised against every other env-mutating test in the crate. + // `stats::otel_metrics::tests` perturbs HYPERHIVE_OTEL_ENDPOINT too, + // and lands in this same test binary — "no other test asserts on + // these" was true of this module and false of the process. + let _env = crate::test_env::lock(); + // SAFETY: serialised by the guard above; restored before returning. let render = || { render_flake( "github:example/hyperhive", diff --git a/hive-c0re/src/stats/otel_metrics.rs b/hive-c0re/src/stats/otel_metrics.rs index 0adf3f6a..24f012f1 100644 --- a/hive-c0re/src/stats/otel_metrics.rs +++ b/hive-c0re/src/stats/otel_metrics.rs @@ -377,8 +377,14 @@ mod tests { /// it resumes posting to a base URL that 404s in silence. #[test] fn endpoint_is_the_standard_otlp_var() { - // SAFETY: single-threaded mutation of a process env var no other test - // in this module asserts on; both names are cleared before returning. + // Serialised against every other env-mutating test in the crate. Both + // variables below are also read by `meta::tests` in this same test + // binary, so "no other test in this module" would be the wrong + // boundary — the module is not the unit that shares the environment, + // the process is. + let _env = crate::test_env::lock(); + // SAFETY: serialised by the guard above; both names are restored (to + // absent) before it drops at the end of this test. unsafe { std::env::remove_var("OTEL_EXPORTER_OTLP_ENDPOINT"); std::env::set_var("HYPERHIVE_OTEL_ENDPOINT", "http://hyperhive.invalid:4318"); diff --git a/hive-c0re/src/test_env.rs b/hive-c0re/src/test_env.rs new file mode 100644 index 00000000..3e5a95bb --- /dev/null +++ b/hive-c0re/src/test_env.rs @@ -0,0 +1,39 @@ +//! Test-only: the crate's single lock for tests that mutate process +//! environment variables. +//! +//! Environment variables are one process-global, and every `#[test]` in this +//! crate lands in the *same* test binary, run on parallel threads. A test that +//! sets a variable, asserts, then restores it is only safe against a *concurrent* +//! test if both take the same lock — so any test here that touches the +//! environment must route through [`lock`] rather than rolling its own +//! save/restore. +//! +//! ⚠️ **The lock has to be crate-wide, not per-module.** `hive-bash-mcp`'s +//! sibling helper records why: two separate per-module mutexes serialise +//! nothing against each other, and that produced a CI-only flake. The pair that +//! motivated this one is `meta::tests` (which renders a flake from +//! `HYPERHIVE_OTEL_*`) and `stats::otel_metrics::tests` (which asserts which +//! variable enables the exporter) — different modules, same variable, same +//! binary. +//! +//! ⚠️ **A green local run is weak evidence for this class.** An agent container +//! has the hyperhive variables ambient-set, so a losing race still finds a +//! plausible value; the nix sandbox strips them, so there the race can delete a +//! variable out from under another thread. Reproduce the sandbox shape with +//! `env -u HYPERHIVE_OTEL_ENDPOINT cargo test -p hive-c0re`. + +use std::sync::{Mutex, MutexGuard, PoisonError}; + +static ENV_LOCK: Mutex<()> = Mutex::new(()); + +/// Serialise this test against every other environment-mutating test in the +/// crate. Hold the returned guard for as long as the variables are perturbed — +/// bind it (`let _env = lock();`), never discard it with `let _ = lock();`, +/// which drops the guard immediately and serialises nothing. +/// +/// Recovers from poisoning: a test that panicked mid-mutation has already +/// failed and reported, and refusing to run every later test on top of that +/// turns one failure into a cascade that hides which test actually broke. +pub fn lock() -> MutexGuard<'static, ()> { + ENV_LOCK.lock().unwrap_or_else(PoisonError::into_inner) +}