diff --git a/crates/hypercolor-daemon/src/app_state.rs b/crates/hypercolor-daemon/src/app_state.rs index 17f0a9031..5538ce1a5 100644 --- a/crates/hypercolor-daemon/src/app_state.rs +++ b/crates/hypercolor-daemon/src/app_state.rs @@ -3,8 +3,6 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Mutex as StdMutex; -#[cfg(test)] -use std::sync::atomic::Ordering; use std::sync::atomic::{AtomicBool, AtomicU64}; use std::sync::{Arc, OnceLock}; use std::time::Instant; @@ -66,9 +64,6 @@ use crate::zone_layout_preview::ZoneLayoutPreviewStore; // ── AppState ───────────────────────────────────────────────────────────── -#[cfg(test)] -static APP_STATE_TEST_DATA_DIR_COUNTER: AtomicU64 = AtomicU64::new(0); - fn test_constructor_task_spawner() -> tokio::runtime::Handle { if let Ok(handle) = tokio::runtime::Handle::try_current() { return handle; @@ -240,6 +235,9 @@ pub struct AppState { /// Shared API auth and rate-limiting state for HTTP and WS command dispatch. pub security_state: crate::api::security::SecurityState, + + // Drop the fixture directory after its state-owned stores. + _temporary_directory: Option, } struct AppStateLibrary { @@ -260,6 +258,7 @@ pub struct AppStateBuilder { runtime_state_path: Option, library: Option, input_manager: Option, + temporary_directory: Option, } impl AppStateBuilder { @@ -273,6 +272,7 @@ impl AppStateBuilder { runtime_state_path: None, library: None, input_manager: None, + temporary_directory: None, } } @@ -331,11 +331,12 @@ fn default_state_dir(data_dir: &Path) -> PathBuf { } impl AppState { - /// Create a new `AppState` with default empty subsystems. + /// Create an isolated `AppState` with empty subsystems and temporary storage. /// /// Primarily useful for testing. In production, prefer /// [`from_daemon_state`](Self::from_daemon_state) to share subsystems - /// with the daemon lifecycle. + /// with the daemon lifecycle. Isolation also applies when this crate is + /// compiled as a dependency; it does not depend on `cfg(test)`. pub fn new() -> Self { Self::builder().build() } @@ -343,20 +344,13 @@ impl AppState { #[doc(hidden)] #[must_use] pub fn builder() -> AppStateBuilder { - #[cfg(test)] - { - let id = APP_STATE_TEST_DATA_DIR_COUNTER.fetch_add(1, Ordering::Relaxed); - AppStateBuilder::new( - std::env::temp_dir() - .join("hypercolor-app-state-tests") - .join(format!("{}-{id}", std::process::id())), - ) - } - - #[cfg(not(test))] - { - AppStateBuilder::new(ConfigManager::data_dir()) - } + let directory = tempfile::Builder::new() + .prefix("hypercolor-app-state-") + .tempdir() + .expect("isolated AppState storage should initialize"); + let mut builder = AppStateBuilder::new(directory.path().to_path_buf()); + builder.temporary_directory = Some(directory); + builder } #[doc(hidden)] @@ -372,6 +366,7 @@ impl AppState { use hypercolor_types::spatial::{EdgeBehavior, SamplingMode, SpatialLayout}; let AppStateBuilder { + temporary_directory, data_dir, state_dir, config_manager, @@ -697,6 +692,7 @@ impl AppState { }, server_session_id: None, security_state: crate::api::security::SecurityState::unserved(), + _temporary_directory: temporary_directory, } } @@ -796,6 +792,7 @@ impl AppState { server_identity: daemon.server_identity.clone(), server_session_id: None, security_state: crate::api::security::SecurityState::unserved(), + _temporary_directory: None, } } diff --git a/crates/hypercolor-daemon/tests/app_state_isolation_tests.rs b/crates/hypercolor-daemon/tests/app_state_isolation_tests.rs new file mode 100644 index 000000000..1cc2c766f --- /dev/null +++ b/crates/hypercolor-daemon/tests/app_state_isolation_tests.rs @@ -0,0 +1,45 @@ +//! The daemon library is built without `cfg(test)` for this integration test. + +use hypercolor_core::config::ConfigManager; +use hypercolor_daemon::app_state::AppState; + +#[test] +fn empty_state_never_uses_ambient_user_storage() { + // Keep even the regressed implementation away from the developer's files. + // This binary has one test, so these process-wide overrides have one owner. + let ambient = tempfile::tempdir().expect("ambient storage fixture creates"); + let data = ambient.path().join("data"); + let state = ambient.path().join("state"); + ConfigManager::set_data_dir_override(Some(data.clone())); + ConfigManager::set_state_dir_override(Some(state.clone())); + ConfigManager::set_config_dir_override(Some(ambient.path().join("config"))); + + let first = AppState::new(); + let second = AppState::default(); + let third = AppState::builder().build(); + for fixture in [&first, &second, &third] { + assert_ne!(fixture.data_dir, data); + assert_ne!(fixture.state_dir, state); + assert!(fixture.state_dir.starts_with(&fixture.data_dir)); + assert!(fixture.data_dir.is_dir()); + } + assert_ne!(first.data_dir, second.data_dir); + assert_ne!(second.data_dir, third.data_dir); + assert!(!data.exists()); + assert!(!state.exists()); + + let temporary = first.data_dir.clone(); + drop(first); + assert!(!temporary.exists()); + + let explicit = ambient.path().join("explicit"); + let fixture = AppState::new_with_data_dir(explicit.clone()); + assert_eq!(fixture.data_dir, explicit); + assert_eq!(fixture.state_dir, explicit.join("state")); + drop(fixture); + assert!(explicit.exists()); + + ConfigManager::set_data_dir_override(None); + ConfigManager::set_state_dir_override(None); + ConfigManager::set_config_dir_override(None); +} diff --git a/crates/hypercolor-daemon/tests/attachment_api_tests.rs b/crates/hypercolor-daemon/tests/attachment_api_tests.rs index 4ce6b3821..dd49700b7 100644 --- a/crates/hypercolor-daemon/tests/attachment_api_tests.rs +++ b/crates/hypercolor-daemon/tests/attachment_api_tests.rs @@ -380,7 +380,7 @@ async fn attachment_template_collection_lists_builtin_metadata() { #[tokio::test] async fn user_template_create_persists_to_overridden_data_dir() { let guard = TestDataDirGuard::new().await; - let state = Arc::new(AppState::new()); + let state = Arc::new(AppState::new_with_data_dir(guard.data_dir.clone())); let app = test_app_with_state(state); let template_id = "test-custom-strip"; let template_path = guard.attachments_dir().join(format!("{template_id}.toml")); @@ -442,7 +442,7 @@ async fn attachment_template_item_and_facet_routes_are_absent() { )] async fn device_attachment_profile_flow_persists_and_clears() { let guard = TestDataDirGuard::new().await; - let state = Arc::new(AppState::new()); + let state = Arc::new(AppState::new_with_data_dir(guard.data_dir.clone())); let app = test_app_with_state(Arc::clone(&state)); let device_id = insert_test_device(&state, "Desk Strip").await; let template_id = "profile-test-strip"; @@ -777,7 +777,7 @@ async fn nollie32_channel_slots_accept_fan_profiles() { #[tokio::test] async fn nollie32_attachment_slots_support_cable_profiles() { let guard = TestDataDirGuard::new().await; - let state = Arc::new(AppState::new()); + let state = Arc::new(AppState::new_with_data_dir(guard.data_dir.clone())); let app = test_app_with_state(Arc::clone(&state)); let device_id = insert_nollie32_test_device(&state).await; diff --git a/crates/hypercolor-daemon/tests/effect_registry_invalidation_tests.rs b/crates/hypercolor-daemon/tests/effect_registry_invalidation_tests.rs index 9b6909b77..a360da68b 100644 --- a/crates/hypercolor-daemon/tests/effect_registry_invalidation_tests.rs +++ b/crates/hypercolor-daemon/tests/effect_registry_invalidation_tests.rs @@ -1,10 +1,9 @@ use std::collections::HashMap; use std::path::PathBuf; -use std::sync::{Arc, LazyLock, Mutex}; +use std::sync::Arc; use axum::body::Body; use http::{Request, StatusCode}; -use hypercolor_core::config::ConfigManager; use hypercolor_daemon::api; use hypercolor_daemon::app_state::AppState; use hypercolor_types::effect::{EffectCategory, EffectId, EffectMetadata, EffectSource}; @@ -13,18 +12,11 @@ use hypercolor_types::spatial::{EdgeBehavior, SamplingMode, SpatialLayout}; use tower::ServiceExt; use uuid::Uuid; -static DATA_DIR_LOCK: LazyLock> = LazyLock::new(|| Mutex::new(())); - fn isolated_state_with_tempdir() -> (AppState, tempfile::TempDir) { - let _lock = DATA_DIR_LOCK - .lock() - .expect("data dir lock should not be poisoned"); let tempdir = tempfile::tempdir().expect("tempdir should be created"); let data_dir = tempdir.path().join("data"); std::fs::create_dir_all(&data_dir).expect("temp data dir should be created"); - ConfigManager::set_data_dir_override(Some(data_dir)); - let state = AppState::new(); - ConfigManager::set_data_dir_override(None); + let state = AppState::new_with_data_dir(data_dir); (state, tempdir) }