fix(relay): crate-wide EnvGuard ends order-dependent env race in config tests - #6265
Open
j-goodz wants to merge 1 commit into
Open
fix(relay): crate-wide EnvGuard ends order-dependent env race in config tests#6265j-goodz wants to merge 1 commit into
j-goodz wants to merge 1 commit into
Conversation
… config test writers and unguarded from_env readers Signed-off-by: j-goodz <22462858+j-goodz@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR: fix(relay): crate-wide EnvGuard ends order-dependent env race between config test writers and unguarded from_env readers
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— nonefound. No existing PR/issue covers this race; the closest prior art is the
two module-private mutexes (
ENV_MUTEXinconfig.rs,ENV_LOCKintelemetry.rs) that this PR replaces.Summary
Rust's test runner executes
buzz-relay's tests on parallel threads in oneprocess, and
std::env::set_var/remove_varmutate process-global state.The
config.rsandtelemetry.rstest modules each had a private mutexserializing their own env-mutating tests — but the test helpers in roughly a
dozen other modules call
Config::from_env(), which reads the very varsthose tests mutate (
BUZZ_S3_ADDRESSING_STYLE,BUZZ_REDIS_POOL_SIZE,BUZZ_REPLICA_READ_MAX_AGE_MS, …), while holding no lock at all. Areader interleaving with a writer mid-mutation observes an invalid value and
from_env()errors — an order-dependent flake observed at roughly a 1/30panic rate (e.g.
handlers::event::tests::fanout_accessfailing withInvalidValue("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
EnvGuardand routes everycfg(test)envreader 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.rsreaders, and the nextfrom_envhelper in
handlers/,api/, ormesh_bootis the one that panics. Thetest helper surface is crate-wide, so the lock has to be too.
What changed
crates/buzz-relay/src/test_env.rs(#[cfg(test)]module, wiredin
lib.rs): a single crate-wide mutex plus anEnvGuardthat(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.rstests now mutate through the guard(
set_now/remove_now, or builder-styleset/remove), replacing thetwo module-private mutexes and every manual save/set/restore block.
cfg(test)Config::from_env()helper across the crate nowtakes 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 inapi/git/store, which readsBUZZ_S3_ADDRESSING_STYLEand would panicon the config writer's transient values.
Out of scope
Production code is deliberately untouched: every edit is inside
#[cfg(test)]code or thecfg(test)module wiring inlib.rs. Noruntime 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 notpart of any writer/reader race.
Test evidence
Commands and results from a Windows machine without local Postgres:
The failures in the full suite are all pre-existing on upstream main,
proven by reproducing each on pristine
2693e0db1via a detached worktree:api::git::policy::tests::bash_hmac_*— Windowsbashtooling HMACmismatch (fails identically on pristine main).
api::admin::tests::*(500 vs 404) and 6api::media::tests::*(
Sqlx(PoolTimedOut)) — require a local Postgres (fails identically onpristine main).
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):
For contrast, full-suite runs of the base tree intermittently show the
race this PR fixes:
handlers::event::tests::fanout_access::*panicking withdefault config loads: InvalidValue("BUZZ_REPLICA_READ_MAX_AGE_MS must be a non-negative integer")whileconfig::tests::replica_read_max_age_*runs —the exact unguarded-reader/writer interleaving the
EnvGuardeliminates.No UI change — no screenshots.