From 5687cd2ee57fe2dc54a29cad022bc988d73898d1 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Mon, 14 Sep 2026 23:11:07 -0400 Subject: [PATCH] Stop the proxy test hanging Windows CI for six hours MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `build (windows-2022)` has been failing on a six-hour job timeout since #5, and the last thing its log says is mine: test an_https_proxy_in_the_environment_is_used has been running for over 60 seconds … 5.95 hours … ##[error]The operation was canceled. It went unnoticed because every run on `main` during the 0.2.0 merges was cancelled by the next push long before six hours elapsed. #12 — a one-line CODEOWNERS file from a bot — is simply the first run left alone long enough to time out. **Two faults, and the second is the one that turned a failure into a wedge.** The test cleared the child's environment and set `PATH` back. On Windows that takes `SystemRoot` with it, and without `SystemRoot` the socket and TLS stacks cannot initialise — so the CLI failed before it could reach any proxy. `tests/update_check.rs` spawns the binary too and uses `env_remove` for the handful of variables that would interfere; this now does the same, and also clears the proxy variables themselves so a developer's own `HTTPS_PROXY` cannot change the result. And it blocked on `accept()` with no deadline, on a thread it then joined. So "the CLI never connected" was indistinguishable from "the CLI has not connected yet", and the job sat there until GitHub killed it. `accept_within` gives it a 30-second bound: nothing arriving is now a *result*, reported as a failure that names the variable and quotes what the CLI said. Verified the bound does what it claims rather than assuming: with the CLI made to bypass the proxy, the test fails in **3.01 seconds** with nothing reached the fake proxy within 3s, so HTTPS_PROXY was not used. The CLI said: … The environment fix is the diagnosis; the deadline is the guarantee. I cannot run Windows here, so if `SystemRoot` was not the whole story the test will now say so in half a minute instead of burning a runner for six hours. Also spawns the child rather than running it to completion, since the connection only arrives while it is running — which removes the thread entirely. 588 tests, fmt and clippy clean. --- tests/proxy.rs | 107 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 83 insertions(+), 24 deletions(-) diff --git a/tests/proxy.rs b/tests/proxy.rs index 03a180d..dc08670 100644 --- a/tests/proxy.rs +++ b/tests/proxy.rs @@ -23,7 +23,8 @@ use std::io::{Read, Write}; use std::net::{TcpListener, TcpStream}; -use std::process::Command; +use std::process::{Command, Stdio}; +use std::time::{Duration, Instant}; /// A read that returns what arrived rather than blocking for a full buffer. fn read_some(stream: &mut TcpStream) -> Vec { @@ -51,26 +52,20 @@ fn what_the_proxy_saw( let listener = TcpListener::bind("127.0.0.1:0").expect("a loopback port"); let port = listener.local_addr().expect("the bound address").port(); - let accepted = std::thread::spawn(move || { - listener - .set_nonblocking(false) - .expect("a blocking listener"); - match listener.incoming().next() { - Some(Ok(mut stream)) => handler(&mut stream), - _ => String::new(), - } - }); + listener + .set_nonblocking(true) + .expect("a non-blocking listener"); - let output = Command::new(env!("CARGO_BIN_EXE_mapbox")) - .args(["styles", "list", "--username", "someone", "-o", "json"]) - .env_clear() - .env("PATH", std::env::var("PATH").unwrap_or_default()) - .env("MAPBOX_ACCESS_TOKEN", "pk.a-fake-token-for-a-proxy-test") - .env("MAPBOX_NO_UPDATE_CHECK", "1") + // Spawned rather than run to completion: the connection this is waiting + // for only arrives while the child is running. + let child = mapbox() .env(variable, format!("{scheme}://127.0.0.1:{port}")) - .output() + .spawn() .expect("the binary runs"); + let seen = accept_within(&listener, ACCEPT_TIMEOUT, handler); + let output = child.wait_with_output().expect("the binary exits"); + // The request cannot succeed — the proxy refuses it — so a success here // would mean the proxy was bypassed entirely. assert!( @@ -79,7 +74,76 @@ fn what_the_proxy_saw( String::from_utf8_lossy(&output.stderr) ); - accepted.join().expect("the fake proxy thread") + seen.unwrap_or_else(|| { + panic!( + "nothing reached the fake proxy within {ACCEPT_TIMEOUT:?}, so {variable} was not \ + used. The CLI said: {}", + String::from_utf8_lossy(&output.stderr) + ) + }) +} + +/// How long a connection may take to arrive before the variable is judged +/// ignored. +/// +/// Generous, because a cold spawn on a loaded runner is not fast — and +/// **bounded**, which is the point. This blocked on `accept()` with no +/// deadline once, so "never connected" was indistinguishable from "has not +/// connected yet": on Windows it wedged the job for its full six-hour limit +/// instead of failing in seconds. +const ACCEPT_TIMEOUT: Duration = Duration::from_secs(30); + +/// Waits for one connection, up to `timeout`, and runs `handler` on it. +/// `None` means nothing arrived — a result, not a reason to keep waiting. +fn accept_within( + listener: &TcpListener, + timeout: Duration, + handler: fn(&mut TcpStream) -> String, +) -> Option { + let deadline = Instant::now() + timeout; + loop { + match listener.accept() { + Ok((mut stream, _)) => { + stream + .set_nonblocking(false) + .expect("a blocking stream to talk on"); + return Some(handler(&mut stream)); + } + Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => { + if Instant::now() >= deadline { + return None; + } + std::thread::sleep(Duration::from_millis(25)); + } + Err(_) => return None, + } + } +} + +/// The binary, with the environment these tests need. +/// +/// `env_remove` rather than `env_clear`, matching `tests/update_check.rs`. +/// Clearing takes `SystemRoot` with it on Windows, and without that the +/// socket and TLS stacks cannot initialise — so the CLI failed before it +/// could reach any proxy, which is what left the earlier version of this +/// test waiting for a connection that was never going to come. +fn mapbox() -> Command { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_mapbox")); + cmd.args(["styles", "list", "--username", "someone", "-o", "json"]) + .env_remove("HTTP_PROXY") + .env_remove("HTTPS_PROXY") + .env_remove("ALL_PROXY") + .env_remove("NO_PROXY") + .env_remove("http_proxy") + .env_remove("https_proxy") + .env_remove("all_proxy") + .env_remove("no_proxy") + .env_remove("MAPBOX_OUTPUT") + .env("MAPBOX_ACCESS_TOKEN", "pk.a-fake-token-for-a-proxy-test") + .env("MAPBOX_NO_UPDATE_CHECK", "1") + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + cmd } /// An HTTP proxy is asked to tunnel an https request with `CONNECT`. @@ -117,12 +181,7 @@ fn an_https_proxy_in_the_environment_is_used() { /// rather than a break: delete the test and document the support. #[test] fn a_socks_proxy_is_refused_with_a_reason() { - let output = Command::new(env!("CARGO_BIN_EXE_mapbox")) - .args(["styles", "list", "--username", "someone", "-o", "json"]) - .env_clear() - .env("PATH", std::env::var("PATH").unwrap_or_default()) - .env("MAPBOX_ACCESS_TOKEN", "pk.a-fake-token-for-a-proxy-test") - .env("MAPBOX_NO_UPDATE_CHECK", "1") + let output = mapbox() // Port 9 (discard) rather than a live one: the scheme is rejected // before anything is dialled, so nothing needs to be listening. .env("ALL_PROXY", "socks5://127.0.0.1:9")