From 53e97923ee9af237a0fa4b51c7ecc2a8966464f9 Mon Sep 17 00:00:00 2001 From: gerchowl Date: Thu, 30 Apr 2026 17:49:53 +0200 Subject: [PATCH] bugfix(client/rust): isolate MAT_VIS_HF_BASE per-test to fix mock/live state leak (#241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Rust client suite mixes httpmock-based unit tests (which set MAT_VIS_HF_BASE to a 127.0.0.1 mock server) with @live tests that need to hit real HF. Even with `cargo test -- --test-threads=1` the env var leaked across tests in the same process — live tests then 404'd on http://127.0.0.1:/v2026.04.2/release-manifest.json. Fix: an in-tree `EnvGuard` RAII helper (Option C — no new dev dep). Each mock test stores the prior value of MAT_VIS_HF_BASE, sets the mock URL, and restores on drop. Live tests then see the original (unset) state regardless of test ordering. `ENV_LOCK` stays so the suite is race-safe under default (parallel) `cargo test` — `std::env::set_var` is `unsafe` in 2024 edition. The existing hand-rolled restore in `e2e_fetch_color_png` and `resolved_tag_falls_back_to_default` is replaced by `EnvGuard` / panic-safe restoration. Verified: `MAT_VIS_LIVE_TESTS=1 MAT_VIS_LIVE_TAG=v2026.04.2 cargo test -- --test-threads=1` passes all 21 tests (4 live) cleanly in a single process, no SIGABRT, no 127.0.0.1 leakage into live URLs. Also rolls back the #253 dagger-side workaround (`test_client_rust` dual cargo invocation): with proper test-side isolation, the single-invocation form is correct again. --- .dagger/src/mat_vis_ci/main.py | 37 +++------ clients/rust/src/main.rs | 145 ++++++++++++++++++++------------- 2 files changed, 97 insertions(+), 85 deletions(-) diff --git a/.dagger/src/mat_vis_ci/main.py b/.dagger/src/mat_vis_ci/main.py index b80f828e..533a8ff5 100644 --- a/.dagger/src/mat_vis_ci/main.py +++ b/.dagger/src/mat_vis_ci/main.py @@ -357,20 +357,11 @@ async def test_client_rust( Doc("Set MAT_VIS_LIVE_TESTS=1 + MAT_VIS_LIVE_TAG=tag to enable live tests (#248)"), ] = False, ) -> str: - """Run cargo test for the Rust reference client against a live release. - - #241: when ``live=True``, mock-based unit tests and the @live - tests share process state — httpmock leaves ``MAT_VIS_HF_BASE`` - pointing at a 127.0.0.1 server, so live tests then 404 on the - mock instead of hitting real HF. Workaround: run the two test - sets as separate cargo invocations (separate processes), so - env-var pollution from the mock tests can't bleed into live. - Proper fix tracked in #241. - """ + """Run cargo test for the Rust reference client against a live release.""" context = src or dag.host().directory(".") cargo_cache = dag.cache_volume("cargo-registry") target_cache = dag.cache_volume("cargo-target") - base = ( + ctr = ( dag.container() .from_("rust:1.89-slim") .with_exec(["apt-get", "update", "-qq"]) @@ -381,22 +372,14 @@ async def test_client_rust( .with_workdir("/app/clients/rust") .with_env_variable("MAT_VIS_TAG", tag) ) - # Always run the mock-based / unit suite (live=False). Filters - # OUT live_* tests so they don't see any leaked mock state. - unit_out = await base.with_exec( - ["cargo", "test", "--", "--test-threads=1", "--skip", "live_"] - ).stdout() - if not live: - return unit_out - # Live tests run in a fresh process so MAT_VIS_HF_BASE leakage - # from the mock-based unit tests can't bleed in (#241). - live_out = await ( - base.with_env_variable("MAT_VIS_LIVE_TESTS", "1") - .with_env_variable("MAT_VIS_LIVE_TAG", tag) - .with_exec(["cargo", "test", "live_", "--", "--test-threads=1"]) - .stdout() - ) - return f"{unit_out}\n--- live tests ---\n{live_out}" + if live: + ctr = ctr.with_env_variable("MAT_VIS_LIVE_TESTS", "1").with_env_variable( + "MAT_VIS_LIVE_TAG", tag + ) + # #241: mock + live tests now share a single cargo invocation + # again — `EnvGuard` in the test code restores `MAT_VIS_HF_BASE` + # on test exit, so env-var pollution can't bleed across tests. + return await ctr.with_exec(["cargo", "test", "--", "--test-threads=1"]).stdout() @function async def test_clients( diff --git a/clients/rust/src/main.rs b/clients/rust/src/main.rs index 26babafa..115beda4 100644 --- a/clients/rust/src/main.rs +++ b/clients/rust/src/main.rs @@ -376,15 +376,18 @@ mod tests { #[test] fn resolved_tag_falls_back_to_default() { - // Drop MAT_VIS_TAG for this assertion; restore after. + // Drop MAT_VIS_TAG for this assertion; restore on scope exit. + // SAFETY: covered by `ENV_LOCK` — keeps mutation race-free + // even under default (parallel) `cargo test`. + let _lock = ENV_LOCK.lock().unwrap(); let prev = std::env::var("MAT_VIS_TAG").ok(); - // SAFETY: setting/removing env vars is safe in single-threaded - // test context — Cargo runs tests in parallel by default but - // this test only reads its own scope and restores. unsafe { std::env::remove_var("MAT_VIS_TAG") }; let tag = resolved_tag(&None); - if let Some(p) = prev { - unsafe { std::env::set_var("MAT_VIS_TAG", p) }; + unsafe { + match prev { + Some(p) => std::env::set_var("MAT_VIS_TAG", p), + None => std::env::remove_var("MAT_VIS_TAG"), + } } assert_eq!(tag, DEFAULT_TAG); } @@ -520,26 +523,62 @@ mod tests { // the live tests, which are gated off by default. These four // tests close that gap without needing prod HF. // - // env-var serialization: every test sets `MAT_VIS_HF_BASE` to its - // mock server URL. Cargo runs tests in a single binary in parallel - // by default, so we serialize through a process-local Mutex. + // env-var isolation (#241): every mock test sets `MAT_VIS_HF_BASE` + // to its mock server URL. Cargo runs tests in a single binary — + // and even with `--test-threads=1` the env var leaks to later + // tests in the same process. `@live` tests then 404 on + // `http://127.0.0.1:/...` instead of huggingface.co. + // + // Fix: an in-tree `EnvGuard` RAII helper. Each mock test stores + // the prior value, sets the mock URL, and restores on drop — + // test-runner agnostic, no new dev dep. We keep `ENV_LOCK` so the + // suite is also safe under default (parallel) `cargo test`: env + // mutation is not thread-safe in Rust 2024 (`set_var` is unsafe). use httpmock::prelude::*; use std::sync::Mutex; static ENV_LOCK: Mutex<()> = Mutex::new(()); - fn set_hf_base(url: &str) { - // SAFETY: the mutex above is held by the caller for the - // duration of the test, so no concurrent reader/writer races. - unsafe { std::env::set_var("MAT_VIS_HF_BASE", url) }; + /// RAII guard that sets an env var on construction and restores + /// the prior value (or removes it if previously unset) on drop. + /// See module-level comment for rationale (#241). + struct EnvGuard { + key: &'static str, + prior: Option, + } + + impl EnvGuard { + fn set(key: &'static str, value: &str) -> Self { + let prior = std::env::var(key).ok(); + // SAFETY: callers hold `ENV_LOCK` for the test's duration, + // so no concurrent reader/writer race on this var. + unsafe { std::env::set_var(key, value) }; + Self { key, prior } + } + } + + impl Drop for EnvGuard { + fn drop(&mut self) { + // SAFETY: `ENV_LOCK` is held by the test that owns this + // guard until after the guard drops (guard is dropped + // before the lock guard in reverse declaration order). + unsafe { + match &self.prior { + Some(v) => std::env::set_var(self.key, v), + None => std::env::remove_var(self.key), + } + } + } } #[test] fn http_png_ktx2_fallback() { - let _guard = ENV_LOCK.lock().unwrap(); + let _lock = ENV_LOCK.lock().unwrap(); let server = MockServer::start(); - set_hf_base(&server.base_url()); + // Drop order: `_env` drops before `_lock` (reverse decl order), + // so env restoration happens while the lock is still held. + let _env = EnvGuard::set("MAT_VIS_HF_BASE", &server.base_url()); // .png 404, .ktx2 200 with valid magic. let png_mock = server.mock(|when, then| { @@ -566,9 +605,9 @@ mod tests { #[test] fn http_magic_byte_rejection() { - let _guard = ENV_LOCK.lock().unwrap(); + let _lock = ENV_LOCK.lock().unwrap(); let server = MockServer::start(); - set_hf_base(&server.base_url()); + let _env = EnvGuard::set("MAT_VIS_HF_BASE", &server.base_url()); // .png 200 but bytes are bogus (no PNG/KTX2 magic). let _png = server.mock(|when, then| { @@ -587,9 +626,9 @@ mod tests { // treated identically to a 404 — the loop continues to .ktx2. // This is "wrong" in a UX sense (a 503 retry would be ideal) // but is consistent across all three v0.6 clients. - let _guard = ENV_LOCK.lock().unwrap(); + let _lock = ENV_LOCK.lock().unwrap(); let server = MockServer::start(); - set_hf_base(&server.base_url()); + let _env = EnvGuard::set("MAT_VIS_HF_BASE", &server.base_url()); let _png = server.mock(|when, then| { when.method(GET).path("/vtest/ambientcg/1k/Rock064/color.png"); @@ -612,9 +651,9 @@ mod tests { #[test] fn http_sentinel_probed_then_texture_fetched() { - let _guard = ENV_LOCK.lock().unwrap(); + let _lock = ENV_LOCK.lock().unwrap(); let server = MockServer::start(); - set_hf_base(&server.base_url()); + let _env = EnvGuard::set("MAT_VIS_HF_BASE", &server.base_url()); let sentinel = server.mock(|when, then| { when.method(GET).path("/vtest/ambientcg/1k/.tier_complete"); @@ -648,9 +687,9 @@ mod tests { #[test] fn http_sentinel_missing_rejects() { - let _guard = ENV_LOCK.lock().unwrap(); + let _lock = ENV_LOCK.lock().unwrap(); let server = MockServer::start(); - set_hf_base(&server.base_url()); + let _env = EnvGuard::set("MAT_VIS_HF_BASE", &server.base_url()); let _miss = server.mock(|when, then| { when.method(GET).path("/vtest/ambientcg/1k/.tier_complete"); @@ -694,40 +733,30 @@ mod tests { eprintln!("(skipped: set MAT_VIS_E2E=1)"); return; } - let _guard = ENV_LOCK.lock().unwrap(); - // Point the client at the scratch dataset for the duration - // of this test only — ENV_LOCK serialises with the httpmock - // tests above so they don't see this URL. - let prev = std::env::var("MAT_VIS_HF_BASE").ok(); - set_hf_base("https://huggingface.co/datasets/gerchowl/mat-vis-tst/resolve"); - - let result = (|| -> Result<(), String> { - let tag = e2e_tag(); - let m = fetch_manifest(&tag); - assert_eq!(m.schema_version, 3); - let cat = fetch_catalog(&tag, "polyhaven", &m); - let mid = cat - .iter() - .find(|e| e.available_tiers.iter().any(|t| t == "1k")) - .map(|e| e.id.clone()) - .ok_or_else(|| { - "no 1k-staged polyhaven material in mat-vis-tst — did the Python E2E suite bake the tag?".to_string() - })?; - assert_tier_complete(&tag, "polyhaven", "1k")?; - let bytes = fetch_texture_bytes(&tag, "polyhaven", &mid, "color", "1k")?; - assert!(bytes.starts_with(PNG_MAGIC), "must be a real PNG"); - assert!(bytes.len() > 1000, "PNG too small: {}", bytes.len()); - Ok(()) - })(); - - // Restore env (best-effort) before propagating any failure. - // SAFETY: ENV_LOCK is held for the duration of the test. - unsafe { - match prev { - Some(v) => std::env::set_var("MAT_VIS_HF_BASE", v), - None => std::env::remove_var("MAT_VIS_HF_BASE"), - } - } - result.expect("e2e round-trip"); + let _lock = ENV_LOCK.lock().unwrap(); + // Point the client at the scratch dataset for the duration of + // this test only. `EnvGuard` restores on drop (panic-safe), so + // a later @live test sees the original (unset) state — #241. + let _env = EnvGuard::set( + "MAT_VIS_HF_BASE", + "https://huggingface.co/datasets/gerchowl/mat-vis-tst/resolve", + ); + + let tag = e2e_tag(); + let m = fetch_manifest(&tag); + assert_eq!(m.schema_version, 3); + let cat = fetch_catalog(&tag, "polyhaven", &m); + let mid = cat + .iter() + .find(|e| e.available_tiers.iter().any(|t| t == "1k")) + .map(|e| e.id.clone()) + .expect( + "no 1k-staged polyhaven material in mat-vis-tst — did the Python E2E suite bake the tag?", + ); + assert_tier_complete(&tag, "polyhaven", "1k").expect("sentinel must exist"); + let bytes = fetch_texture_bytes(&tag, "polyhaven", &mid, "color", "1k") + .expect("texture fetch must succeed"); + assert!(bytes.starts_with(PNG_MAGIC), "must be a real PNG"); + assert!(bytes.len() > 1000, "PNG too small: {}", bytes.len()); } }