fix(test): remove two process-global races causing flaky CI panics#61
Merged
Conversation
`install_debug_transport_logger_tracks_the_debug_pattern` asserted on the real process-wide transport debug logger, which `Cli::run` republishes on every call (including from the many unrelated tests that call `cli.run(...)` with no `--debug` flag). Any of those running concurrently could flip the global between this test's install and assert, tripping the panic seen in CI. Extract the pure decision logic into `debug_transport_logger_for` so the test asserts on a locally-built logger instead of the shared global, and drop the now-dead `TRANSPORT_LOGGER_TEST_LOCK`/`RestoreDefaultTransportLogger`, which only ever guarded this test against itself. `persist_active_without_app_id_errors_clearly` resolves "prod" internally, reading the same `PROD_*` env vars its sibling tests mutate under `ENV_LOCK` — except this test never took that lock, so a concurrent sibling could inject a value (e.g. an invalid `PROD_MIN_STAGE`) and fail resolution for an unrelated reason. Take `ENV_LOCK` here too.
There was a problem hiding this comment.
Pull request overview
This PR reduces flaky CI panics by removing two process-global test races: one around the default transport logger being republished by Cli::run, and another around environment resolution reading PROD_* env vars while sibling tests mutate them.
Changes:
- Extracted
debug_transport_logger_foras pure decision logic and refactored transport logger installation to use it. - Updated the transport logger unit test to assert on the pure builder rather than the process-wide global.
- Serialized the
persist_active_without_app_id_errors_clearlyenvironment test withENV_LOCK, and removed now-unused transport-logger test locking/RAII cleanup helpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/cli.rs | Adds a pure transport-logger builder, refactors installation to use it, and updates the unit test to avoid touching a process-global. |
| src/transport/client.rs | Removes unused test-only lock/RAII for restoring the default transport logger. |
| src/environments.rs | Takes ENV_LOCK in a test that indirectly reads PROD_* env vars to prevent concurrent env mutation races. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
qcai-godaddy
approved these changes
Jul 23, 2026
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.
Summary
cli::user_agent_tests::install_debug_transport_logger_tracks_the_debug_patternasserted on the process-wide default transport logger, butCli::runrepublishes that same global on every call — including from the many unrelated tests that callcli.run(...)with no--debugflag. Any of those running concurrently could flip the global between this test's install and assert. Fixed by extracting the pure decision logic (debug_transport_logger_for) so the test no longer touches the shared global, and removing the now-deadTRANSPORT_LOGGER_TEST_LOCK/RestoreDefaultTransportLogger.environments::tests::persist_active_without_app_id_errors_clearlyresolves"prod"internally, reading the samePROD_*env vars its sibling tests mutate underENV_LOCK— but it never took that lock itself. Fixed by takingENV_LOCKthere too.Test plan
cargo test --features pkce-auth --libx30 in a loop (previously failed within ~3 runs; now clean)cargo test --all-targetsandcargo test --all-targets --features pkce-authcargo test --docandcargo test --doc --features pkce-authcargo clippy --all-targets -- -D warnings(default andpkce-auth)🤖 Generated with Claude Code