From 1216a1cc143f6587409c61cee38b1808907dfa17 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 09:24:54 +0000 Subject: [PATCH 1/3] test(skills): stop leaking TREQ_APP_DATA_DIR into parallel tests Skills tests set the process-wide TREQ_APP_DATA_DIR to a tempdir and removed it on drop. Tests running in parallel that resolve the app database (e.g. submodule sync) then opened treq.db under a directory that was being deleted, failing intermittently with 'unable to open database file'. Tests now set a per-thread override that resolve_app_db_path and app_skills_root read instead. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MxqTt1WtVRKkjNXQpgkJS9 --- src-tauri/src/core/mod.rs | 28 +++++++++++++++++---- src-tauri/src/core/skills.rs | 47 +++++++++++++++--------------------- 2 files changed, 42 insertions(+), 33 deletions(-) diff --git a/src-tauri/src/core/mod.rs b/src-tauri/src/core/mod.rs index 0db008c35..7d23dd49b 100644 --- a/src-tauri/src/core/mod.rs +++ b/src-tauri/src/core/mod.rs @@ -78,16 +78,34 @@ pub fn resolve_app_db_path(repo_path: &str) -> PathBuf { } } - if let Ok(app_data_dir) = std::env::var("TREQ_APP_DATA_DIR") { - let trimmed = app_data_dir.trim(); - if !trimmed.is_empty() { - return Path::new(trimmed).join("treq.db"); - } + if let Some(app_data_dir) = app_data_dir() { + return app_data_dir.join("treq.db"); } Path::new(repo_path).join(".treq").join("treq.db") } +#[cfg(test)] +thread_local! { + /// Per-thread app data dir for tests. Setting `TREQ_APP_DATA_DIR` would leak + /// into every test running in parallel. + pub(crate) static TEST_APP_DATA_DIR: std::cell::RefCell> = + const { std::cell::RefCell::new(None) }; +} + +/// The app data dir from `TREQ_APP_DATA_DIR`, or a test's per-thread override. +pub(crate) fn app_data_dir() -> Option { + #[cfg(test)] + if let Some(dir) = TEST_APP_DATA_DIR.with(|dir| dir.borrow().clone()) { + return Some(dir); + } + std::env::var("TREQ_APP_DATA_DIR") + .ok() + .map(|dir| dir.trim().to_string()) + .filter(|dir| !dir.is_empty()) + .map(PathBuf::from) +} + #[cfg(test)] mod tests { use super::resolve_app_db_path; diff --git a/src-tauri/src/core/skills.rs b/src-tauri/src/core/skills.rs index e4ca521a2..791952f64 100644 --- a/src-tauri/src/core/skills.rs +++ b/src-tauri/src/core/skills.rs @@ -133,12 +133,9 @@ pub fn read_bytes_from_locator(locator: &str) -> Result, String> { } pub fn app_skills_root() -> Result { - let dir = std::env::var("TREQ_APP_DATA_DIR") - .ok() - .map(|s| s.trim().to_string()) - .filter(|s| !s.is_empty()) - .ok_or_else(|| "TREQ_APP_DATA_DIR is not set".to_string())?; - Ok(PathBuf::from(dir).join("skills")) + let dir = + crate::core::app_data_dir().ok_or_else(|| "TREQ_APP_DATA_DIR is not set".to_string())?; + Ok(dir.join("skills")) } pub fn repo_skills_root(repo_path: &str) -> PathBuf { @@ -551,25 +548,19 @@ mod tests { } } - struct EnvGuard { - key: &'static str, - previous: Option, - } + /// Points this test thread's app data dir at `dir` until dropped. + struct AppDataDirGuard; - impl EnvGuard { - fn set(key: &'static str, value: &str) -> Self { - let previous = std::env::var(key).ok(); - std::env::set_var(key, value); - Self { key, previous } + impl AppDataDirGuard { + fn set(dir: &Path) -> Self { + crate::core::TEST_APP_DATA_DIR.with(|d| *d.borrow_mut() = Some(dir.to_path_buf())); + Self } } - impl Drop for EnvGuard { + impl Drop for AppDataDirGuard { fn drop(&mut self) { - match &self.previous { - Some(value) => std::env::set_var(self.key, value), - None => std::env::remove_var(self.key), - } + crate::core::TEST_APP_DATA_DIR.with(|d| *d.borrow_mut() = None); } } @@ -592,7 +583,7 @@ mod tests { #[test] fn install_skill_files_rejects_checksum_mismatch() { let app = TempDir::new().unwrap(); - let _guard = EnvGuard::set("TREQ_APP_DATA_DIR", app.path().to_str().unwrap()); + let _guard = AppDataDirGuard::set(app.path()); let files = sample_files(); let entry = sample_entry("deadbeef".to_string()); let err = install_skill_files(&entry, &files, SkillInstallScope::Application, None) @@ -603,7 +594,7 @@ mod tests { #[test] fn install_skill_files_records_computed_checksum_when_catalog_omits_it() { let app = TempDir::new().unwrap(); - let _guard = EnvGuard::set("TREQ_APP_DATA_DIR", app.path().to_str().unwrap()); + let _guard = AppDataDirGuard::set(app.path()); let files = sample_files(); let mut entry = sample_entry(skill_checksum(&files)); entry.checksum = None; @@ -615,7 +606,7 @@ mod tests { #[test] fn install_skill_files_writes_application_pack() { let app = TempDir::new().unwrap(); - let _guard = EnvGuard::set("TREQ_APP_DATA_DIR", app.path().to_str().unwrap()); + let _guard = AppDataDirGuard::set(app.path()); let files = sample_files(); let checksum = skill_checksum(&files); let entry = sample_entry(checksum.clone()); @@ -637,7 +628,7 @@ mod tests { fn install_skill_files_writes_repository_pack() { let app = TempDir::new().unwrap(); let repo = TempDir::new().unwrap(); - let _guard = EnvGuard::set("TREQ_APP_DATA_DIR", app.path().to_str().unwrap()); + let _guard = AppDataDirGuard::set(app.path()); let files = sample_files(); let entry = sample_entry(skill_checksum(&files)); install_skill_files( @@ -664,7 +655,7 @@ mod tests { fn set_skill_install_scope_moves_pack() { let app = TempDir::new().unwrap(); let repo = TempDir::new().unwrap(); - let _guard = EnvGuard::set("TREQ_APP_DATA_DIR", app.path().to_str().unwrap()); + let _guard = AppDataDirGuard::set(app.path()); let files = sample_files(); let entry = sample_entry(skill_checksum(&files)); install_skill_files(&entry, &files, SkillInstallScope::Application, None).expect("install"); @@ -687,7 +678,7 @@ mod tests { let app = TempDir::new().unwrap(); let repo = TempDir::new().unwrap(); let workspace = TempDir::new().unwrap(); - let _guard = EnvGuard::set("TREQ_APP_DATA_DIR", app.path().to_str().unwrap()); + let _guard = AppDataDirGuard::set(app.path()); let files = sample_files(); let entry = sample_entry(skill_checksum(&files)); install_skill_files(&entry, &files, SkillInstallScope::Application, None).expect("install"); @@ -715,7 +706,7 @@ mod tests { let app = TempDir::new().unwrap(); let repo = TempDir::new().unwrap(); let workspace = TempDir::new().unwrap(); - let _guard = EnvGuard::set("TREQ_APP_DATA_DIR", app.path().to_str().unwrap()); + let _guard = AppDataDirGuard::set(app.path()); let files = sample_files(); let entry = sample_entry(skill_checksum(&files)); install_skill_files(&entry, &files, SkillInstallScope::Application, None).expect("install"); @@ -747,7 +738,7 @@ mod tests { #[test] fn merge_catalog_marks_installed_skills() { let app = TempDir::new().unwrap(); - let _guard = EnvGuard::set("TREQ_APP_DATA_DIR", app.path().to_str().unwrap()); + let _guard = AppDataDirGuard::set(app.path()); let files = sample_files(); let entry = sample_entry(skill_checksum(&files)); install_skill_files(&entry, &files, SkillInstallScope::Application, None).expect("install"); From a40f701de27b4b663730f2a3def379df789733a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 09:26:06 +0000 Subject: [PATCH 2/3] test(skills): keep the test override comment on one line Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MxqTt1WtVRKkjNXQpgkJS9 --- src-tauri/src/core/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src-tauri/src/core/mod.rs b/src-tauri/src/core/mod.rs index 7d23dd49b..d642205ef 100644 --- a/src-tauri/src/core/mod.rs +++ b/src-tauri/src/core/mod.rs @@ -87,8 +87,7 @@ pub fn resolve_app_db_path(repo_path: &str) -> PathBuf { #[cfg(test)] thread_local! { - /// Per-thread app data dir for tests. Setting `TREQ_APP_DATA_DIR` would leak - /// into every test running in parallel. + /// Per-thread app data dir for tests; the env var would leak across parallel tests. pub(crate) static TEST_APP_DATA_DIR: std::cell::RefCell> = const { std::cell::RefCell::new(None) }; } From 89dbd898aa26308c2b9f5b663d199e887973c727 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 00:36:28 +0000 Subject: [PATCH 3/3] test(core): resolve app db path tests without mutating process env The resolve_app_db_path tests set TREQ_APP_DATA_DIR and TREQ_APP_DB_PATH process-wide, so parallel lib tests could read them. Route the env lookup through an injected closure and have the tests pass their own. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01E1poGWXMSFcPsJ8V5gujWG --- src-tauri/src/core/mod.rs | 49 ++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/src-tauri/src/core/mod.rs b/src-tauri/src/core/mod.rs index d642205ef..554f650d0 100644 --- a/src-tauri/src/core/mod.rs +++ b/src-tauri/src/core/mod.rs @@ -71,14 +71,19 @@ pub fn resolve_conflict_marker_style(db: &std::sync::Mutex) } pub fn resolve_app_db_path(repo_path: &str) -> PathBuf { - if let Ok(explicit_db_path) = std::env::var("TREQ_APP_DB_PATH") { + resolve_app_db_path_with(repo_path, |key| std::env::var(key).ok()) +} + +/// `resolve_app_db_path` reading env vars through `env`, so tests need not mutate the process env. +fn resolve_app_db_path_with(repo_path: &str, env: impl Fn(&str) -> Option) -> PathBuf { + if let Some(explicit_db_path) = env("TREQ_APP_DB_PATH") { let trimmed = explicit_db_path.trim(); if !trimmed.is_empty() { return PathBuf::from(trimmed); } } - if let Some(app_data_dir) = app_data_dir() { + if let Some(app_data_dir) = app_data_dir_with(env) { return app_data_dir.join("treq.db"); } @@ -94,12 +99,15 @@ thread_local! { /// The app data dir from `TREQ_APP_DATA_DIR`, or a test's per-thread override. pub(crate) fn app_data_dir() -> Option { + app_data_dir_with(|key| std::env::var(key).ok()) +} + +fn app_data_dir_with(env: impl Fn(&str) -> Option) -> Option { #[cfg(test)] if let Some(dir) = TEST_APP_DATA_DIR.with(|dir| dir.borrow().clone()) { return Some(dir); } - std::env::var("TREQ_APP_DATA_DIR") - .ok() + env("TREQ_APP_DATA_DIR") .map(|dir| dir.trim().to_string()) .filter(|dir| !dir.is_empty()) .map(PathBuf::from) @@ -107,37 +115,26 @@ pub(crate) fn app_data_dir() -> Option { #[cfg(test)] mod tests { - use super::resolve_app_db_path; - use std::sync::{Mutex, OnceLock}; - - fn env_lock() -> &'static Mutex<()> { - static LOCK: OnceLock> = OnceLock::new(); - LOCK.get_or_init(|| Mutex::new(())) - } + use super::resolve_app_db_path_with; + use std::path::Path; #[test] fn resolve_app_db_path_prefers_explicit_db_path() { - let _guard = env_lock().lock().unwrap(); - std::env::set_var("TREQ_APP_DB_PATH", "/tmp/explicit-treq.db"); - std::env::set_var("TREQ_APP_DATA_DIR", "/tmp/ignored-dir"); + let env = |key: &str| match key { + "TREQ_APP_DB_PATH" => Some("/tmp/explicit-treq.db".to_string()), + "TREQ_APP_DATA_DIR" => Some("/tmp/ignored-dir".to_string()), + _ => None, + }; - let resolved = resolve_app_db_path("/repo/path"); + let resolved = resolve_app_db_path_with("/repo/path", env); assert_eq!(resolved.to_string_lossy(), "/tmp/explicit-treq.db"); - - std::env::remove_var("TREQ_APP_DB_PATH"); - std::env::remove_var("TREQ_APP_DATA_DIR"); } #[test] fn resolve_app_db_path_falls_back_to_app_data_dir() { - let _guard = env_lock().lock().unwrap(); - std::env::remove_var("TREQ_APP_DB_PATH"); - std::env::set_var("TREQ_APP_DATA_DIR", "/tmp/app-data"); - - let resolved = resolve_app_db_path("/repo/path"); - let expected = std::path::Path::new("/tmp/app-data").join("treq.db"); - assert_eq!(resolved, expected); + let env = |key: &str| (key == "TREQ_APP_DATA_DIR").then(|| "/tmp/app-data".to_string()); - std::env::remove_var("TREQ_APP_DATA_DIR"); + let resolved = resolve_app_db_path_with("/repo/path", env); + assert_eq!(resolved, Path::new("/tmp/app-data").join("treq.db")); } }