diff --git a/hive-c0re/src/main.rs b/hive-c0re/src/main.rs index 9efe1d51..9515f47c 100644 --- a/hive-c0re/src/main.rs +++ b/hive-c0re/src/main.rs @@ -36,8 +36,6 @@ 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 b3f220a6..a17d0186 100644 --- a/hive-c0re/src/meta.rs +++ b/hive-c0re/src/meta.rs @@ -2222,12 +2222,8 @@ mod tests { // no endpoint signal, no hyperhive.otel lines are emitted (agents // keep the the harness modules disabled default). // - // 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. + // SAFETY: single-threaded mutation of process env vars no other + // test asserts on; 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 24f012f1..0883f04e 100644 --- a/hive-c0re/src/stats/otel_metrics.rs +++ b/hive-c0re/src/stats/otel_metrics.rs @@ -1,9 +1,8 @@ //! 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, 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 +//! 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 //! 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. @@ -46,8 +45,8 @@ static SNAPSHOT: OnceLock>>> = OnceLock::new(); static PROVIDER: OnceLock = OnceLock::new(); /// Spawn the container-resource OTEL exporter if OTEL is configured -/// (`OTEL_EXPORTER_OTLP_ENDPOINT` non-empty). No-op otherwise. Call once at -/// startup. +/// (`HYPERHIVE_OTEL_ENDPOINT` non-empty — the same enable signal +/// `meta::otel_config` uses). 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"); @@ -70,7 +69,7 @@ pub fn spawn_exporter(hyperhive_flake: &str) { } }); - match build_provider(interval, snapshot, hyperhive_flake) { + match build_provider(&endpoint, interval, snapshot, hyperhive_flake) { Ok(provider) => { let _ = PROVIDER.set(provider); tracing::info!(%endpoint, ?interval, "otel container-metrics: exporter enabled"); @@ -82,6 +81,7 @@ pub fn spawn_exporter(hyperhive_flake: &str) { } fn build_provider( + endpoint: &str, interval: Duration, snapshot: Arc>>, hyperhive_flake: &str, @@ -90,24 +90,12 @@ 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")?; @@ -280,12 +268,9 @@ fn host_arch() -> &'static str { } } -/// `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`). +/// `HYPERHIVE_OTEL_ENDPOINT`, non-empty. The enable signal. fn endpoint() -> Option { - std::env::var("OTEL_EXPORTER_OTLP_ENDPOINT") + std::env::var("HYPERHIVE_OTEL_ENDPOINT") .ok() .map(|s| s.trim().to_owned()) .filter(|s| !s.is_empty()) @@ -366,57 +351,4 @@ 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() { - // 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"); - } - 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/hive-c0re/src/test_env.rs b/hive-c0re/src/test_env.rs deleted file mode 100644 index 3e5a95bb..00000000 --- a/hive-c0re/src/test_env.rs +++ /dev/null @@ -1,39 +0,0 @@ -//! 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) -} diff --git a/nix/host-modules/hive-c0re/environment.nix b/nix/host-modules/hive-c0re/environment.nix index 92b612e8..64975b30 100644 --- a/nix/host-modules/hive-c0re/environment.nix +++ b/nix/host-modules/hive-c0re/environment.nix @@ -81,11 +81,6 @@ 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 @@ -93,20 +88,7 @@ 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 = 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; + HYPERHIVE_OTEL_ENDPOINT = "http://${config.services.hyperhive.network.bridgeIp}:${toString otel.collector.port}"; # 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