Compare commits

...
Author SHA1 Message Date
atlas
af3a9e5433 test(hive-c0re): one crate-wide lock for env-mutating tests
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.
2026-08-19 01:38:54 +02:00
atlas
76f6b7c3b7 fix(otel): let the SDK resolve hive-c0re's OTLP endpoint
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
2026-08-19 01:38:54 +02:00
5 changed files with 143 additions and 12 deletions

View file

@ -36,6 +36,8 @@ mod socket_server;
mod stats;
mod stores;
mod swarm_status;
#[cfg(test)]
mod test_env;
mod webhook_secret;
mod workers;

View file

@ -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",

View file

@ -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<Arc<Mutex<Vec<ContainerResource>>>> = OnceLock::new();
static PROVIDER: OnceLock<SdkMeterProvider> = 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<Mutex<Vec<ContainerResource>>>,
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<String> {
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,57 @@ 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");
}
}
}

39
hive-c0re/src/test_env.rs Normal file
View file

@ -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)
}

View file

@ -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