Skip to content

bugfix(client/rust): isolate MAT_VIS_HF_BASE per-test (#241) #254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gerchowl merged 1 commit into from
Apr 30, 2026
Merged

bugfix(client/rust): isolate MAT_VIS_HF_BASE per-test (#241) #254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 10 additions & 27 deletions .dagger/src/mat_vis_ci/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
Expand All @@ -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(
Expand Down
145 changes: 87 additions & 58 deletions clients/rust/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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:<port>/...` 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<String>,
}

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| {
Expand All @@ -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| {
Expand All @@ -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");
Expand All @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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());
}
}
Loading