Skip to content

fix(relay): crate-wide EnvGuard ends order-dependent env race in config tests - #6265

Open
j-goodz wants to merge 1 commit into
block:mainfrom
j-goodz:pr/relay-env-guard
Open

fix(relay): crate-wide EnvGuard ends order-dependent env race in config tests#6265
j-goodz wants to merge 1 commit into
block:mainfrom
j-goodz:pr/relay-env-guard

Conversation

@j-goodz

@j-goodz j-goodz commented Aug 18, 2026

Copy link
Copy Markdown

PR: fix(relay): crate-wide EnvGuard ends order-dependent env race between config test writers and unguarded from_env readers

Ready-to-submit PR body. Single commit on upstream main, touching only
crates/buzz-relay/**.


Duplicate search

Searched upstream history for prior art: git log --all --grep=EnvGuard,
git log --oneline upstream/main -- crates/buzz-relay/src/test_env.rs — none
found. No existing PR/issue covers this race; the closest prior art is the
two module-private mutexes (ENV_MUTEX in config.rs, ENV_LOCK in
telemetry.rs) that this PR replaces.


Summary

Rust's test runner executes buzz-relay's tests on parallel threads in one
process, and std::env::set_var / remove_var mutate process-global state.
The config.rs and telemetry.rs test modules each had a private mutex
serializing their own env-mutating tests — but the test helpers in roughly a
dozen other modules call Config::from_env(), which reads the very vars
those tests mutate (BUZZ_S3_ADDRESSING_STYLE, BUZZ_REDIS_POOL_SIZE,
BUZZ_REPLICA_READ_MAX_AGE_MS, …), while holding no lock at all. A
reader interleaving with a writer mid-mutation observes an invalid value and
from_env() errors — an order-dependent flake observed at roughly a 1/30
panic rate (e.g. handlers::event::tests::fanout_access failing with
InvalidValue("BUZZ_REPLICA_READ_MAX_AGE_MS must be a non-negative integer")
while config::tests::replica_read_max_age_* runs).

This PR adds one crate-wide EnvGuard and routes every cfg(test) env
reader and writer through it. Zero production changes.

Why a module-private mutex wasn't enough

The two module mutexes serialized only the writers inside their own
module
. Any reader elsewhere in the crate still sees the mutation window,
so the flake simply moves: fix config.rs readers, and the next from_env
helper in handlers/, api/, or mesh_boot is the one that panics. The
test helper surface is crate-wide, so the lock has to be too.

What changed

  • New crates/buzz-relay/src/test_env.rs (#[cfg(test)] module, wired
    in lib.rs): a single crate-wide mutex plus an EnvGuard that
    (a) serializes every env-mutating test AND every env-reading helper, and
    (b) records each touched key's prior value and restores it on drop —
    including on panic or early return.
  • config.rs / telemetry.rs tests now mutate through the guard
    (set_now/remove_now, or builder-style set/remove), replacing the
    two module-private mutexes and every manual save/set/restore block.
  • Every cfg(test) Config::from_env() helper across the crate now
    takes the guard: api/admin, api/bridge, api/git/policy,
    api/git/transport, api/invites, api/media, api/operator,
    handlers/event, handlers/identity_archive, handlers/relay_admin,
    mesh_boot, state, workflow_sink — plus the S3-probe helper in
    api/git/store, which reads BUZZ_S3_ADDRESSING_STYLE and would panic
    on the config writer's transient values.

Out of scope

Production code is deliberately untouched: every edit is inside
#[cfg(test)] code or the cfg(test) module wiring in lib.rs. No
runtime behavior, no HTTP/WS surface, no configuration semantics change.
Raw env reads in test helpers of vars no test ever writes (REDIS_URL,
DATABASE_URL, PATH, BUZZ_GIT_S3_PROBE) are left as-is — they are not
part of any writer/reader race.

Test evidence

Commands and results from a Windows machine without local Postgres:

cargo fmt --all -- --check
# clean (exit 0)

cargo check -p buzz-relay --all-targets
# Finished, no warnings

cargo clippy -p buzz-relay --all-targets -- -D warnings
# Finished, no warnings

cargo test -p buzz-relay
# 864–865 passed, 43 ignored, 10–11 failed

The failures in the full suite are all pre-existing on upstream main,
proven by reproducing each on pristine 2693e0db1 via a detached worktree:

  • 2 api::git::policy::tests::bash_hmac_* — Windows bash tooling HMAC
    mismatch (fails identically on pristine main).
  • 2 api::admin::tests::* (500 vs 404) and 6 api::media::tests::*
    (Sqlx(PoolTimedOut)) — require a local Postgres (fails identically on
    pristine main).
  • 1 telemetry::tests::trace_context_lookup_does_not_enable_callsites
    order-dependent flake unrelated to env (reproduced intermittently on
    pristine main, passes in isolation).

Race proof (the point of the PR):

# 20 iterations x 4 filter groups:
cargo test -p buzz-relay --lib config::tests                 # 29 tests
cargo test -p buzz-relay --lib telemetry::tests::test_service # 3 tests
cargo test -p buzz-relay --lib test_try_init_tracer_disabled_when_endpoint_unset
cargo test -p buzz-relay --lib mesh_boot::tests              # incl. mesh_off_boots_nothing
# Result: 80/80 groups green, 0 failures.

# 15 further full-suite runs on the patched tree:
# 0 config/telemetry/mesh_boot/storage_sweep env-test failures in any run.

For contrast, full-suite runs of the base tree intermittently show the
race this PR fixes: handlers::event::tests::fanout_access::* panicking with
default config loads: InvalidValue("BUZZ_REPLICA_READ_MAX_AGE_MS must be a non-negative integer") while config::tests::replica_read_max_age_* runs —
the exact unguarded-reader/writer interleaving the EnvGuard eliminates.

No UI change — no screenshots.

… config test writers and unguarded from_env readers

Signed-off-by: j-goodz <22462858+j-goodz@users.noreply.github.com>
@j-goodz
j-goodz requested a review from a team as a code owner August 18, 2026 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant