From 3f22012729f68c735c20988afb750391fce36961 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Fri, 14 Aug 2026 22:49:37 +0000 Subject: [PATCH] fix: disable ANSI log colors when stdout is not a terminal Motivation: four sync integration tests failed deterministically in some environments: proactive_sync_plus:: missing_relay_list_uses_bounded_fallback_coverage, maintainer_reprocessing:: unresolved_repositories_share_one_dependency_poll_per_relay, metrics::test_live_sync_event_count, and reconnect_backoff:: flapping_relay_handshakes_do_not_reset_exponential_backoff. Each waits for a plain `field=value` substring (e.g. `consecutive_failures=1`) in the relay subprocess log written by the TestRelay fixture. The tracing fmt layer colorizes by default, so the redirected log contained `consecutive_failures^[[0m^[[2m=^[[0m1`, which never matches. tracing-subscriber honours NO_COLOR, so the same tests passed or failed depending on the invoking environment: runs with NO_COLOR set produced clean logs, interactive-launched runs produced ANSI logs and deterministic failures. Approach: initialize the fmt layer with `.with_ansi(std::io::stdout().is_terminal())`. Colors are emitted only when a human is watching a terminal; redirected output (test fixtures, journald, pipelines) stays plain. This is a logging hygiene fix in the binary, not a test accommodation: the defect was writing terminal control sequences to a non-terminal stream. Correctness assumptions: the relay binary is the only process whose logs the fixtures scrape, and tests always redirect its stdout to a file, so is_terminal() is deterministically false there and the log format no longer varies with the parent environment. Excluded scope: several sync tests remain flaky under full-suite parallel load but pass repeatedly in isolation (req_concurrency::startup_historic_sync_stays_within_relay_req_concurrency_limit, live_sync::live_sync_regroups_after_filter_count_refusal, tag_variations::test_layer3_sync_with_lowercase_e_tag). They assert on metrics or proxy behaviour, not log text; their bounded deadlines are exceeded when the whole suite competes for CPU. Left undiagnosed rather than papered over with wider timeouts. Validation: the four log-scraping tests pass individually and in full runs after the fix; cargo fmt makes no changes; cargo clippy --workspace --all-targets -- -D warnings is clean; full cargo test is green except the pre-existing load flakes above, each of which passes 3/3 when run in isolation. --- src/main.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 314d970..9c44ba3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,3 +1,5 @@ +use std::io::IsTerminal; + use anyhow::Result; use clap::Parser; use tokio::signal; @@ -70,8 +72,13 @@ async fn main() -> Result<()> { async fn run_relay(config: Config) -> Result<()> { // Initialize tracing with configured log level let filter = EnvFilter::new(&config.log_level).and(SuppressRoutineDisconnects); - let subscriber = - tracing_subscriber::registry().with(tracing_subscriber::fmt::layer().with_filter(filter)); + // Only colorize when stdout is a terminal: ANSI escape codes in redirected + // logs corrupt journald/file output and break log-scraping consumers. + let subscriber = tracing_subscriber::registry().with( + tracing_subscriber::fmt::layer() + .with_ansi(std::io::stdout().is_terminal()) + .with_filter(filter), + ); tracing::subscriber::set_global_default(subscriber)?; info!("Starting ngit-grasp with log level: {}", config.log_level);