From f91d156f855d760564889e9d5312490717674fc9 Mon Sep 17 00:00:00 2001 From: Yeachan-Heo Date: Sun, 12 Apr 2026 13:52:41 +0000 Subject: [PATCH] Keep poisoned test locks from cascading across unrelated regressions The repo-local backlog was effectively exhausted, so this sweep promoted the newly observed test-lock poisoning pain point into ROADMAP #74 and fixed it in place. Test-only env/cwd lock acquisition now recovers poisoned mutexes in the remaining strict call sites, and each affected surface has a regression that proves a panic no longer permanently poisons later tests. Constraint: Keep the fix test-only and avoid widening runtime behavior changes Rejected: Refactor shared helper signatures across broader call paths | unnecessary churn beyond the remaining strict test sites Confidence: high Scope-risk: narrow Reversibility: clean Directive: These guards only recover the mutex; tests that mutate env or cwd still must restore process-global state explicitly Tested: cargo fmt --all --check Tested: cargo clippy --workspace --all-targets -- -D warnings Tested: cargo test --workspace Tested: Architect review (APPROVE) Not-tested: Additional fault-injection around partially restored env/cwd state after panic Related: ROADMAP #74 --- ROADMAP.md | 4 +- rust/crates/commands/src/lib.rs | 20 ++++++- rust/crates/plugins/src/lib.rs | 70 +++++++++++++++--------- rust/crates/rusty-claude-cli/src/main.rs | 24 +++++++- rust/crates/tools/src/lib.rs | 32 ++++++++--- 5 files changed, 112 insertions(+), 38 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index ae33b44..3e8766a 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -496,7 +496,7 @@ Model name prefix now wins unconditionally over env-var presence. Regression tes 62. **Worker state file surface not implemented** — **done (verified 2026-04-12):** current `main` already wires `emit_state_file(worker)` into the worker transition path in `rust/crates/runtime/src/worker_boot.rs`, atomically writes `.claw/worker-state.json`, and exposes the documented reader surface through `claw state` / `claw state --output-format json` in `rust/crates/rusty-claude-cli/src/main.rs`. Fresh proof exists in `runtime` regression `emit_state_file_writes_worker_status_on_transition`, the end-to-end `tools` regression `recovery_loop_state_file_reflects_transitions`, and direct CLI parsing coverage for `state` / `state --output-format json`. Source: Jobdori dogfood. -**Scope note (verified 2026-04-12):** ROADMAP #31, #43, and #63-#68 currently appear to describe acpx/droid or upstream OMX/server orchestration behavior, not claw-code source already present in this repository. Repo-local searches for `acpx`, `use-droid`, `run-acpx`, `commit-wrapper`, `ultraclaw`, `roadmap-nudge-10min`, `OMX_TMUX_INJECT`, `/hooks/health`, and `/hooks/status` found no implementation hits outside `ROADMAP.md`, and the earlier state-surface note already records that the HTTP server is not owned by claw-code. With #45, #65, #67, and #69 now fixed, the remaining unresolved items in this section look like external tracking notes rather than confirmed repo-local backlog; re-check if new repo-local evidence appears. +**Scope note (verified 2026-04-12):** ROADMAP #31, #43, and #63-#68 currently appear to describe acpx/droid or upstream OMX/server orchestration behavior, not claw-code source already present in this repository. Repo-local searches for `acpx`, `use-droid`, `run-acpx`, `commit-wrapper`, `ultraclaw`, `roadmap-nudge-10min`, `OMX_TMUX_INJECT`, `/hooks/health`, and `/hooks/status` found no implementation hits outside `ROADMAP.md`, and the earlier state-surface note already records that the HTTP server is not owned by claw-code. With #45, #64-#67, and #69 now fixed, the remaining unresolved items in this section still look like external tracking notes rather than confirmed repo-local backlog; re-check if new repo-local evidence appears. 63. **Droid session completion semantics broken: code arrives after "status: completed"** — dogfooded 2026-04-12. Ultraclaw droid sessions (use-droid via acpx) report `session.status: completed` before file writes are fully flushed/synced to the working tree. Discovered +410 lines of "late-arriving" droid output that appeared after I had already assessed 8 sessions as "no code produced." This creates false-negative assessments and duplicate work. **Fix shape:** (a) droid agent should only report completion after explicit file-write confirmation (fsync or existence check); (b) or, claw-code should expose a `pending_writes` status that indicates "agent responded, disk flush pending"; (c) lane orchestrators should poll for file changes for N seconds after completion before final assessment. **Blocker:** none. Source: Jobdori ultraclaw dogfood 2026-04-12. @@ -518,3 +518,5 @@ Model name prefix now wins unconditionally over env-var presence. Regression tes 72. **`latest` managed-session selection depends on filesystem mtime before semantic session recency** — **done (verified 2026-04-12):** managed-session summaries now carry `updated_at_ms`, `SessionStore::list_sessions()` sorts by semantic recency before filesystem mtime, and regression coverage locks the case where `latest` must prefer the newer session payload even when file mtimes point the other way. The CLI session-summary wrapper now stays in sync with the runtime field so `latest` resolution uses the same ordering signal everywhere. **Original filing below.** 73. **Session timestamps are not monotonic enough for latest-session ordering under tight loops** — **done (verified 2026-04-12):** runtime session timestamps now use a process-local monotonic millisecond source, so back-to-back saves still produce increasing `updated_at_ms` even when the wall clock does not advance. The temporary sleep hack was removed from the resume-latest regression, and fresh workspace verification stayed green with the semantic-recency ordering path from #72. **Original filing below.** + +74. **Poisoned test locks cascade into unrelated Rust regressions** — **done (verified 2026-04-12):** test-only env/cwd lock acquisition in `rust/crates/tools/src/lib.rs`, `rust/crates/plugins/src/lib.rs`, `rust/crates/commands/src/lib.rs`, and `rust/crates/rusty-claude-cli/src/main.rs` now recovers poisoned mutexes via `PoisonError::into_inner`, and new regressions lock that behavior so one panic no longer causes later tests to fail just by touching the shared env/cwd locks. Source: Jobdori dogfood 2026-04-12. diff --git a/rust/crates/commands/src/lib.rs b/rust/crates/commands/src/lib.rs index 0724b16..bf6d793 100644 --- a/rust/crates/commands/src/lib.rs +++ b/rust/crates/commands/src/lib.rs @@ -4152,6 +4152,24 @@ mod tests { LOCK.get_or_init(|| Mutex::new(())) } + fn env_guard() -> std::sync::MutexGuard<'static, ()> { + env_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } + + #[test] + fn env_guard_recovers_after_poisoning() { + let poisoned = std::thread::spawn(|| { + let _guard = env_guard(); + panic!("poison env lock"); + }) + .join(); + assert!(poisoned.is_err(), "poisoning thread should panic"); + + let _guard = env_guard(); + } + fn restore_env_var(key: &str, original: Option) { match original { Some(value) => std::env::set_var(key, value), @@ -5214,7 +5232,7 @@ mod tests { #[test] fn discovers_omc_skills_from_project_and_user_compatibility_roots() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let workspace = temp_dir("skills-omc-workspace"); let user_home = temp_dir("skills-omc-home"); let claude_config_dir = temp_dir("skills-omc-claude-config"); diff --git a/rust/crates/plugins/src/lib.rs b/rust/crates/plugins/src/lib.rs index 3252d91..765c0ac 100644 --- a/rust/crates/plugins/src/lib.rs +++ b/rust/crates/plugins/src/lib.rs @@ -2294,6 +2294,12 @@ fn env_lock() -> &'static std::sync::Mutex<()> { mod tests { use super::*; + fn env_guard() -> std::sync::MutexGuard<'static, ()> { + env_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } + fn temp_dir(label: &str) -> PathBuf { let nanos = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) @@ -2302,6 +2308,18 @@ mod tests { std::env::temp_dir().join(format!("plugins-{label}-{nanos}")) } + #[test] + fn env_guard_recovers_after_poisoning() { + let poisoned = std::thread::spawn(|| { + let _guard = env_guard(); + panic!("poison env lock"); + }) + .join(); + assert!(poisoned.is_err(), "poisoning thread should panic"); + + let _guard = env_guard(); + } + fn write_file(path: &Path, contents: &str) { if let Some(parent) = path.parent() { fs::create_dir_all(parent).expect("parent dir"); @@ -2485,7 +2503,7 @@ mod tests { #[test] fn load_plugin_from_directory_validates_required_fields() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let root = temp_dir("manifest-required"); write_file( root.join(MANIFEST_FILE_NAME).as_path(), @@ -2500,7 +2518,7 @@ mod tests { #[test] fn load_plugin_from_directory_reads_root_manifest_and_validates_entries() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let root = temp_dir("manifest-root"); write_loader_plugin(&root); @@ -2530,7 +2548,7 @@ mod tests { #[test] fn load_plugin_from_directory_supports_packaged_manifest_path() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let root = temp_dir("manifest-packaged"); write_external_plugin(&root, "packaged-demo", "1.0.0"); @@ -2544,7 +2562,7 @@ mod tests { #[test] fn load_plugin_from_directory_defaults_optional_fields() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let root = temp_dir("manifest-defaults"); write_file( root.join(MANIFEST_FILE_NAME).as_path(), @@ -2566,7 +2584,7 @@ mod tests { #[test] fn load_plugin_from_directory_rejects_duplicate_permissions_and_commands() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let root = temp_dir("manifest-duplicates"); write_file( root.join("commands").join("sync.sh").as_path(), @@ -2862,7 +2880,7 @@ mod tests { #[test] fn discovers_builtin_and_bundled_plugins() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let manager = PluginManager::new(PluginManagerConfig::new(temp_dir("discover"))); let plugins = manager.list_plugins().expect("plugins should list"); assert!(plugins @@ -2875,7 +2893,7 @@ mod tests { #[test] fn installs_enables_updates_and_uninstalls_external_plugins() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("home"); let source_root = temp_dir("source"); write_external_plugin(&source_root, "demo", "1.0.0"); @@ -2924,7 +2942,7 @@ mod tests { #[test] fn auto_installs_bundled_plugins_into_the_registry() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("bundled-home"); let bundled_root = temp_dir("bundled-root"); write_bundled_plugin(&bundled_root.join("starter"), "starter", "0.1.0", false); @@ -2956,7 +2974,7 @@ mod tests { #[test] fn default_bundled_root_loads_repo_bundles_as_installed_plugins() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("default-bundled-home"); let manager = PluginManager::new(PluginManagerConfig::new(&config_home)); @@ -2975,7 +2993,7 @@ mod tests { #[test] fn bundled_sync_prunes_removed_bundled_registry_entries() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("bundled-prune-home"); let bundled_root = temp_dir("bundled-prune-root"); let stale_install_path = config_home @@ -3039,7 +3057,7 @@ mod tests { #[test] fn installed_plugin_discovery_keeps_registry_entries_outside_install_root() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("registry-fallback-home"); let bundled_root = temp_dir("registry-fallback-bundled"); let install_root = config_home.join("plugins").join("installed"); @@ -3094,7 +3112,7 @@ mod tests { #[test] fn installed_plugin_discovery_prunes_stale_registry_entries() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("registry-prune-home"); let bundled_root = temp_dir("registry-prune-bundled"); let install_root = config_home.join("plugins").join("installed"); @@ -3140,7 +3158,7 @@ mod tests { #[test] fn persists_bundled_plugin_enable_state_across_reloads() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("bundled-state-home"); let bundled_root = temp_dir("bundled-state-root"); write_bundled_plugin(&bundled_root.join("starter"), "starter", "0.1.0", false); @@ -3174,7 +3192,7 @@ mod tests { #[test] fn persists_bundled_plugin_disable_state_across_reloads() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("bundled-disabled-home"); let bundled_root = temp_dir("bundled-disabled-root"); write_bundled_plugin(&bundled_root.join("starter"), "starter", "0.1.0", true); @@ -3208,7 +3226,7 @@ mod tests { #[test] fn validates_plugin_source_before_install() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("validate-home"); let source_root = temp_dir("validate-source"); write_external_plugin(&source_root, "validator", "1.0.0"); @@ -3223,7 +3241,7 @@ mod tests { #[test] fn plugin_registry_tracks_enabled_state_and_lookup() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("registry-home"); let source_root = temp_dir("registry-source"); write_external_plugin(&source_root, "registry-demo", "1.0.0"); @@ -3251,7 +3269,7 @@ mod tests { #[test] fn plugin_registry_report_collects_load_failures_without_dropping_valid_plugins() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); // given let config_home = temp_dir("report-home"); let external_root = temp_dir("report-external"); @@ -3296,7 +3314,7 @@ mod tests { #[test] fn installed_plugin_registry_report_collects_load_failures_from_install_root() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); // given let config_home = temp_dir("installed-report-home"); let bundled_root = temp_dir("installed-report-bundled"); @@ -3327,7 +3345,7 @@ mod tests { #[test] fn rejects_plugin_sources_with_missing_hook_paths() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); // given let config_home = temp_dir("broken-home"); let source_root = temp_dir("broken-source"); @@ -3355,7 +3373,7 @@ mod tests { #[test] fn rejects_plugin_sources_with_missing_failure_hook_paths() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); // given let config_home = temp_dir("broken-failure-home"); let source_root = temp_dir("broken-failure-source"); @@ -3383,7 +3401,7 @@ mod tests { #[test] fn plugin_registry_runs_initialize_and_shutdown_for_enabled_plugins() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("lifecycle-home"); let source_root = temp_dir("lifecycle-source"); let _ = write_lifecycle_plugin(&source_root, "lifecycle-demo", "1.0.0"); @@ -3407,7 +3425,7 @@ mod tests { #[test] fn aggregates_and_executes_plugin_tools() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("tool-home"); let source_root = temp_dir("tool-source"); write_tool_plugin(&source_root, "tool-demo", "1.0.0"); @@ -3436,7 +3454,7 @@ mod tests { #[test] fn list_installed_plugins_scans_install_root_without_registry_entries() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("installed-scan-home"); let bundled_root = temp_dir("installed-scan-bundled"); let install_root = config_home.join("plugins").join("installed"); @@ -3468,7 +3486,7 @@ mod tests { #[test] fn list_installed_plugins_scans_packaged_manifests_in_install_root() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); let config_home = temp_dir("installed-packaged-scan-home"); let bundled_root = temp_dir("installed-packaged-scan-bundled"); let install_root = config_home.join("plugins").join("installed"); @@ -3502,7 +3520,7 @@ mod tests { /// host `~/.claw/plugins/` from bleeding into test runs. #[test] fn claw_config_home_isolation_prevents_host_plugin_leakage() { - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); // Create a temp directory to act as our isolated CLAW_CONFIG_HOME let config_home = temp_dir("isolated-home"); @@ -3556,7 +3574,7 @@ mod tests { use std::sync::Arc; use std::thread; - let _guard = env_lock().lock().expect("env lock"); + let _guard = env_guard(); // Shared base directory for all threads let base_dir = temp_dir("parallel-base"); diff --git a/rust/crates/rusty-claude-cli/src/main.rs b/rust/crates/rusty-claude-cli/src/main.rs index 32d8177..6e0b307 100644 --- a/rust/crates/rusty-claude-cli/src/main.rs +++ b/rust/crates/rusty-claude-cli/src/main.rs @@ -10534,7 +10534,7 @@ UU conflicted.rs", #[test] fn managed_sessions_default_to_jsonl_and_resolve_legacy_json() { - let _guard = cwd_lock().lock().expect("cwd lock"); + let _guard = cwd_guard(); let workspace = temp_workspace("session-resolution"); std::fs::create_dir_all(&workspace).expect("workspace should create"); let previous = std::env::current_dir().expect("cwd"); @@ -10573,7 +10573,7 @@ UU conflicted.rs", #[test] fn latest_session_alias_resolves_most_recent_managed_session() { - let _guard = cwd_lock().lock().expect("cwd lock"); + let _guard = cwd_guard(); let workspace = temp_workspace("latest-session-alias"); std::fs::create_dir_all(&workspace).expect("workspace should create"); let previous = std::env::current_dir().expect("cwd"); @@ -10606,7 +10606,7 @@ UU conflicted.rs", #[test] fn load_session_reference_rejects_workspace_mismatch() { - let _guard = cwd_lock().lock().expect("cwd lock"); + let _guard = cwd_guard(); let workspace_a = temp_workspace("session-mismatch-a"); let workspace_b = temp_workspace("session-mismatch-b"); std::fs::create_dir_all(&workspace_a).expect("workspace a should create"); @@ -10680,6 +10680,24 @@ UU conflicted.rs", LOCK.get_or_init(|| Mutex::new(())) } + fn cwd_guard() -> MutexGuard<'static, ()> { + cwd_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } + + #[test] + fn cwd_guard_recovers_after_poisoning() { + let poisoned = std::thread::spawn(|| { + let _guard = cwd_guard(); + panic!("poison cwd lock"); + }) + .join(); + assert!(poisoned.is_err(), "poisoning thread should panic"); + + let _guard = cwd_guard(); + } + fn temp_workspace(label: &str) -> PathBuf { let nanos = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) diff --git a/rust/crates/tools/src/lib.rs b/rust/crates/tools/src/lib.rs index dd8bf8d..0786eb4 100644 --- a/rust/crates/tools/src/lib.rs +++ b/rust/crates/tools/src/lib.rs @@ -6047,6 +6047,24 @@ mod tests { LOCK.get_or_init(|| Mutex::new(())) } + fn env_guard() -> std::sync::MutexGuard<'static, ()> { + env_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } + + #[test] + fn env_guard_recovers_after_poisoning() { + let poisoned = std::thread::spawn(|| { + let _guard = env_guard(); + panic!("poison env lock"); + }) + .join(); + assert!(poisoned.is_err(), "poisoning thread should panic"); + + let _guard = env_guard(); + } + fn temp_path(name: &str) -> PathBuf { let unique = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) @@ -7167,7 +7185,7 @@ mod tests { #[test] fn skill_loads_local_skill_prompt() { - let _guard = env_lock().lock().expect("env lock should acquire"); + let _guard = env_guard(); let home = temp_path("skills-home"); let skill_dir = home.join(".agents").join("skills").join("help"); fs::create_dir_all(&skill_dir).expect("skill dir should exist"); @@ -7224,7 +7242,7 @@ mod tests { #[test] fn skill_resolves_project_local_skills_and_legacy_commands() { - let _guard = env_lock().lock().expect("env lock should acquire"); + let _guard = env_guard(); let root = temp_path("project-skills"); let skill_dir = root.join(".claw").join("skills").join("plan"); let command_dir = root.join(".claw").join("commands"); @@ -7268,7 +7286,7 @@ mod tests { #[test] fn skill_loads_project_local_claude_skill_prompt() { - let _guard = env_lock().lock().expect("env lock should acquire"); + let _guard = env_guard(); let root = temp_path("project-skills"); let home = root.join("home"); let workspace = root.join("workspace"); @@ -7319,7 +7337,7 @@ mod tests { #[test] fn skill_loads_project_local_omc_and_agents_skill_prompts() { - let _guard = env_lock().lock().expect("env lock should acquire"); + let _guard = env_guard(); let root = temp_path("project-omc-skills"); let home = root.join("home"); let workspace = root.join("workspace"); @@ -7389,7 +7407,7 @@ mod tests { #[test] fn skill_loads_learned_skill_from_claude_config_dir() { - let _guard = env_lock().lock().expect("env lock should acquire"); + let _guard = env_guard(); let root = temp_path("claude-config-learned-skill"); let home = root.join("home"); let claude_config_dir = root.join("claude-config"); @@ -7444,7 +7462,7 @@ mod tests { #[test] fn skill_loads_direct_skill_and_legacy_command_from_claude_config_dir() { - let _guard = env_lock().lock().expect("env lock should acquire"); + let _guard = env_guard(); let root = temp_path("claude-config-direct-skill"); let home = root.join("home"); let claude_config_dir = root.join("claude-config"); @@ -7516,7 +7534,7 @@ mod tests { #[test] fn skill_loads_project_local_legacy_command_markdown() { - let _guard = env_lock().lock().expect("env lock should acquire"); + let _guard = env_guard(); let root = temp_path("project-legacy-command"); let home = root.join("home"); let workspace = root.join("workspace");