From b98583068ab765035427580cd0b2780bdce7e083 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 9 Oct 2026 08:26:56 +0000 Subject: [PATCH 1/2] test(vllm): race the probe-state-path uniqueness test The test for `probe_state_path` called it 64 times in a row on one thread. Every other test for this bug class releases 16 threads together on a Barrier, because that is how parallel tests actually hit these helpers. Use the same shape here so the vllm check matches the rest of the workspace and does not depend on how fast one thread loops. Signed-off-by: Roman Inflianskas --- engines/vllm/src/state.rs | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/engines/vllm/src/state.rs b/engines/vllm/src/state.rs index 454c8a402..fbf647dd0 100644 --- a/engines/vllm/src/state.rs +++ b/engines/vllm/src/state.rs @@ -345,19 +345,42 @@ mod tests { NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed) )) } - #[test] - fn probe_state_path_gives_each_call_its_own_file_even_for_one_tag() { - // Many calls rather than two: without a counter the names differ only - // when the clock ticks between calls, so a pair can pass by luck. - let paths: Vec = (0..64).map(|_| probe_state_path("same-tag")).collect(); + /// Call `make_path` from many threads at once and assert no two calls + /// returned the same path. + /// + /// Concurrent rather than sequential, because that is how tests call these + /// helpers: calls made one after another let the clock tick between them + /// and can pass by luck, while threads released together all read the same + /// millisecond, so a name keyed on pid and time alone repeats. + fn assert_each_call_gets_its_own_path(make_path: impl Fn() -> PathBuf + Sync) { + const CALLERS: usize = 16; + let start = std::sync::Barrier::new(CALLERS); + let paths: Vec = std::thread::scope(|scope| { + let callers: Vec<_> = (0..CALLERS) + .map(|_| { + scope.spawn(|| { + start.wait(); + make_path() + }) + }) + .collect(); + callers + .into_iter() + .map(|caller| caller.join().expect("a caller panicked")) + .collect() + }); let distinct: std::collections::HashSet<_> = paths.iter().collect(); assert_eq!( distinct.len(), - paths.len(), - "two calls with one tag shared a path" + CALLERS, + "two calls with one tag must not share a path" ); } #[test] + fn probe_state_path_gives_each_call_its_own_file_even_for_one_tag() { + assert_each_call_gets_its_own_path(|| probe_state_path("same-tag")); + } + #[test] fn inference_verification_latches_into_the_state_file() -> Result<()> { // First check probes and records the verdict; the second reads the latch // and leaves the model alone. From 14ac166f62c992aae4f244e072745e37c3d6c2c4 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 9 Oct 2026 08:26:56 +0000 Subject: [PATCH 2/2] test(lemonade): give each scratch dir a per-process unique name Four test modules each carried an identical `scratch_dir(tag)` that built `target/lemonade-fs-test-`, with no pid, time or counter. Every caller passes a distinct tag today, so nothing collides yet, but the helper wipes the directory before handing it out: a reused tag, or two `cargo test` processes against one checkout, would have one run delete the other's files. Replace the four copies with one shared `test_support::scratch_dir` that names the directory `---`, as #603 did for the other crates, keeping the remove-then-create semantics, and add a Barrier-concurrent test that 16 calls with one tag get 16 distinct directories. Signed-off-by: Roman Inflianskas --- engines/lemonade/src/backend_alignment.rs | 13 +--- engines/lemonade/src/direct_llama.rs | 13 +--- engines/lemonade/src/install.rs | 13 +--- engines/lemonade/src/lib.rs | 2 + engines/lemonade/src/state.rs | 13 +--- engines/lemonade/src/test_support.rs | 72 +++++++++++++++++++++++ 6 files changed, 78 insertions(+), 48 deletions(-) create mode 100644 engines/lemonade/src/test_support.rs diff --git a/engines/lemonade/src/backend_alignment.rs b/engines/lemonade/src/backend_alignment.rs index 16f2d63ea..ca053da37 100644 --- a/engines/lemonade/src/backend_alignment.rs +++ b/engines/lemonade/src/backend_alignment.rs @@ -939,22 +939,11 @@ fn resolve_rocm_llamacpp_backend_dirs_aside( mod tests { use super::*; use crate::install::platform_binary_name; + use crate::test_support::scratch_dir; use anyhow::anyhow; use serde_json::json; use std::fs; - /// A fresh scratch directory under the crate's `target/`. The base is - /// `CARGO_MANIFEST_DIR`, a compile-time constant, so the path never derives from a - /// runtime environment read. - fn scratch_dir(tag: &str) -> PathBuf { - let dir = Path::new(env!("CARGO_MANIFEST_DIR")) - .join("target") - .join(format!("lemonade-fs-test-{tag}")); - let _ = fs::remove_dir_all(&dir); - fs::create_dir_all(&dir).unwrap(); - dir - } - #[test] fn backend_install_succeeds_without_retry() { let mut attempts = 0; diff --git a/engines/lemonade/src/direct_llama.rs b/engines/lemonade/src/direct_llama.rs index 79721f529..44bc6a482 100644 --- a/engines/lemonade/src/direct_llama.rs +++ b/engines/lemonade/src/direct_llama.rs @@ -484,20 +484,9 @@ fn filter_ggufs_by_variant(mut ggufs: Vec, variant: Option<&str>) -> Ve mod tests { use super::*; use crate::LLAMACPP_RECIPE; + use crate::test_support::scratch_dir; use std::fs; - /// A fresh scratch directory under the crate's `target/`. The base is - /// `CARGO_MANIFEST_DIR`, a compile-time constant, so the path never derives from a - /// runtime environment read. - fn scratch_dir(tag: &str) -> PathBuf { - let dir = Path::new(env!("CARGO_MANIFEST_DIR")) - .join("target") - .join(format!("lemonade-fs-test-{tag}")); - let _ = fs::remove_dir_all(&dir); - fs::create_dir_all(&dir).unwrap(); - dir - } - #[test] fn parses_canonical_hugging_face_checkpoints() { assert_eq!( diff --git a/engines/lemonade/src/install.rs b/engines/lemonade/src/install.rs index e03b7e8cd..34f6f30e2 100644 --- a/engines/lemonade/src/install.rs +++ b/engines/lemonade/src/install.rs @@ -952,22 +952,11 @@ fn copy_tree_entries( #[cfg(test)] mod tests { use super::*; + use crate::test_support::scratch_dir; use std::io::Write; use std::process::{Command as ProcessCommand, Stdio}; use std::time::Duration; - /// A fresh scratch directory under the crate's `target/`. The base is - /// `CARGO_MANIFEST_DIR`, a compile-time constant, so the path never derives from a - /// runtime environment read. - fn scratch_dir(tag: &str) -> PathBuf { - let dir = Path::new(env!("CARGO_MANIFEST_DIR")) - .join("target") - .join(format!("lemonade-fs-test-{tag}")); - let _ = fs::remove_dir_all(&dir); - fs::create_dir_all(&dir).unwrap(); - dir - } - #[test] fn embeddable_package_matches_runtime_os() { let version = rocm_deps::LEMONADE_VERSION; diff --git a/engines/lemonade/src/lib.rs b/engines/lemonade/src/lib.rs index a51711143..20a74b447 100644 --- a/engines/lemonade/src/lib.rs +++ b/engines/lemonade/src/lib.rs @@ -24,6 +24,8 @@ mod install; mod process; mod runtime_dir; mod state; +#[cfg(test)] +mod test_support; // Re-exported so this stays reachable at its pre-split crate-root path — // `backend_alignment` is a private module, but this constant was `pub` at diff --git a/engines/lemonade/src/state.rs b/engines/lemonade/src/state.rs index 55c49cab5..e6ae73cbc 100644 --- a/engines/lemonade/src/state.rs +++ b/engines/lemonade/src/state.rs @@ -542,21 +542,10 @@ pub(crate) fn manifest_lock_hash(manifest: &LemonadeInstallManifest) -> String { #[cfg(test)] mod tests { use super::*; + use crate::test_support::scratch_dir; use std::io::{Read, Write}; use std::time::Duration; - /// A fresh scratch directory under the crate's `target/`. The base is - /// `CARGO_MANIFEST_DIR`, a compile-time constant, so the path never derives from a - /// runtime environment read. - fn scratch_dir(tag: &str) -> PathBuf { - let dir = Path::new(env!("CARGO_MANIFEST_DIR")) - .join("target") - .join(format!("lemonade-fs-test-{tag}")); - let _ = fs::remove_dir_all(&dir); - fs::create_dir_all(&dir).unwrap(); - dir - } - #[test] fn identity_from_state_prefers_server_pid_and_its_start_ticks() { let state = json!({ diff --git a/engines/lemonade/src/test_support.rs b/engines/lemonade/src/test_support.rs new file mode 100644 index 000000000..e51555bf7 --- /dev/null +++ b/engines/lemonade/src/test_support.rs @@ -0,0 +1,72 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Shared test-only fixtures for `rocm-engine-lemonade`'s module test suites. + +use std::fs; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +/// A fresh, empty scratch directory under the crate's `target/`, named +/// `lemonade-fs-test----`. +/// +/// The base is `CARGO_MANIFEST_DIR`, a compile-time constant, so the path never +/// derives from a runtime environment read. The tag keeps the name readable; +/// it does not keep it unique. A tag reused by two tests running on parallel +/// threads, or one test running in two `cargo test` processes against the same +/// checkout, would otherwise share the directory, and each call wipes it +/// first, so one run would delete the other's files. The pid separates +/// processes and the per-process counter separates calls within one, so no two +/// calls agree whatever the tag. +pub(crate) fn scratch_dir(tag: &str) -> PathBuf { + static NEXT: AtomicU64 = AtomicU64::new(0); + let dir = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("target") + .join(format!( + "lemonade-fs-test-{tag}-{}-{}-{}", + std::process::id(), + rocm_core::unix_time_millis(), + NEXT.fetch_add(1, Ordering::Relaxed) + )); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + dir +} + +mod tests { + use super::*; + + /// Concurrent rather than sequential, because that is how tests call this + /// helper: calls made one after another can let the clock tick between + /// them and pass by luck, while threads released together all read the + /// same millisecond, so a name keyed on pid and time alone repeats. + #[test] + fn scratch_dir_gives_each_call_its_own_directory_even_for_one_tag() { + const CALLERS: usize = 16; + let start = std::sync::Barrier::new(CALLERS); + let dirs: Vec = std::thread::scope(|scope| { + let callers: Vec<_> = (0..CALLERS) + .map(|_| { + scope.spawn(|| { + start.wait(); + scratch_dir("same-tag") + }) + }) + .collect(); + callers + .into_iter() + .map(|caller| caller.join().expect("a caller panicked")) + .collect() + }); + for dir in &dirs { + let _ = fs::remove_dir_all(dir); + } + let distinct: std::collections::HashSet<_> = dirs.iter().collect(); + assert_eq!( + distinct.len(), + CALLERS, + "two calls with one tag must not share a directory" + ); + } +}