From 606a02667af430cc14577671d2c98bbcda304a14 Mon Sep 17 00:00:00 2001 From: David Abram Date: Thu, 1 Oct 2026 01:31:42 +0200 Subject: [PATCH 1/2] runtime: Share mutation-scope advisory locks across adapters Centralize OS advisory-lock acquisition behind typed state and boundary lock identities so the Codex, OpenCode, and Pi adapters use one implementation while retaining distinct lock files and diagnostics. Update adapter tests and lifecycle documentation to cover process-safe contention, timeout behavior, and the shared lock ownership model. Co-authored-by: SCE --- .../codex_mutation_scope/boundary_lock.rs | 195 --------- .../hooks/codex_mutation_scope/lifecycle.rs | 6 +- .../hooks/codex_mutation_scope/mod.rs | 2 - .../hooks/codex_mutation_scope/os_lock.rs | 95 ----- .../hooks/codex_mutation_scope/state.rs | 82 +--- .../hooks/codex_mutation_scope/tests.rs | 24 +- cli/src/services/hooks/mod.rs | 1 + cli/src/services/hooks/mutation_scope_lock.rs | 396 ++++++++++++++++++ .../opencode_mutation_scope/boundary_lock.rs | 109 ----- .../opencode_mutation_scope/lifecycle.rs | 6 +- .../hooks/opencode_mutation_scope/mod.rs | 2 - .../hooks/opencode_mutation_scope/os_lock.rs | 95 ----- .../hooks/opencode_mutation_scope/state.rs | 41 +- .../hooks/pi_mutation_scope/boundary_lock.rs | 109 ----- .../hooks/pi_mutation_scope/lifecycle.rs | 6 +- .../services/hooks/pi_mutation_scope/mod.rs | 2 - .../hooks/pi_mutation_scope/os_lock.rs | 95 ----- .../services/hooks/pi_mutation_scope/state.rs | 40 +- .../cli/codex-mutation-scope-integration.md | 5 +- context/cli/doctor-recovery-safety-model.md | 4 +- ...encode-mutation-scope-adapter-lifecycle.md | 4 +- context/context-map.md | 2 +- 22 files changed, 461 insertions(+), 860 deletions(-) delete mode 100644 cli/src/services/hooks/codex_mutation_scope/boundary_lock.rs delete mode 100644 cli/src/services/hooks/codex_mutation_scope/os_lock.rs create mode 100644 cli/src/services/hooks/mutation_scope_lock.rs delete mode 100644 cli/src/services/hooks/opencode_mutation_scope/boundary_lock.rs delete mode 100644 cli/src/services/hooks/opencode_mutation_scope/os_lock.rs delete mode 100644 cli/src/services/hooks/pi_mutation_scope/boundary_lock.rs delete mode 100644 cli/src/services/hooks/pi_mutation_scope/os_lock.rs diff --git a/cli/src/services/hooks/codex_mutation_scope/boundary_lock.rs b/cli/src/services/hooks/codex_mutation_scope/boundary_lock.rs deleted file mode 100644 index eb41286d3..000000000 --- a/cli/src/services/hooks/codex_mutation_scope/boundary_lock.rs +++ /dev/null @@ -1,195 +0,0 @@ -use std::path::{Path, PathBuf}; -use std::time::Duration; - -use super::os_lock::{AdvisoryLockError, OsAdvisoryLock}; -use super::state::adapter_state_dir; - -const ADAPTER_BOUNDARY_LOCK_FILE: &str = "codex-mutation-scope-boundary.lock"; -const BOUNDARY_LOCK_WHAT: &str = "adapter-boundary"; - -pub(crate) const DEFAULT_BOUNDARY_LOCK_TIMEOUT: Duration = Duration::from_secs(10); - -pub(crate) fn boundary_lock_path(git_dir: &Path) -> PathBuf { - adapter_state_dir(git_dir).join(ADAPTER_BOUNDARY_LOCK_FILE) -} - -pub(crate) struct AdapterBoundaryLock { - _inner: OsAdvisoryLock, -} - -impl AdapterBoundaryLock { - pub(crate) fn acquire( - git_dir: &Path, - timeout: Duration, - ) -> Result { - let inner = OsAdvisoryLock::acquire( - &adapter_state_dir(git_dir), - boundary_lock_path(git_dir), - timeout, - BOUNDARY_LOCK_WHAT, - )?; - Ok(AdapterBoundaryLock { _inner: inner }) - } -} - -#[cfg(test)] -mod tests { - use std::io::Write as _; - use std::process::Command; - use std::sync::atomic::{AtomicU64, Ordering}; - use std::sync::mpsc; - use std::thread; - - use super::*; - - static NEXT_ID: AtomicU64 = AtomicU64::new(0); - - fn unique_git_dir(label: &str) -> PathBuf { - let id = NEXT_ID.fetch_add(1, Ordering::Relaxed); - std::env::temp_dir().join(format!( - "sce-codex-mutation-scope-boundary-{label}-{}-{id}", - std::process::id() - )) - } - - const CHILD_ENV_GIT_DIR: &str = "SCE_BOUNDARY_LOCK_CHILD_GIT_DIR"; - const CHILD_ENV_TIMEOUT_MS: &str = "SCE_BOUNDARY_LOCK_CHILD_TIMEOUT_MS"; - const CHILD_HELPER_PATH: &str = - "services::hooks::codex_mutation_scope::boundary_lock::tests::boundary_lock_child_helper"; - - #[test] - #[ignore = "subprocess helper, driven by process_death_releases_the_boundary_lock"] - fn boundary_lock_child_helper() { - let git_dir = PathBuf::from( - std::env::var(CHILD_ENV_GIT_DIR).expect("child helper needs a git dir in the env"), - ); - let timeout = Duration::from_millis( - std::env::var(CHILD_ENV_TIMEOUT_MS) - .expect("child helper needs a timeout in the env") - .parse() - .expect("timeout must parse"), - ); - match AdapterBoundaryLock::acquire(&git_dir, timeout) { - Ok(lock) => { - print!("ACQUIRED"); - std::io::stdout().flush().expect("flush stdout"); - std::mem::forget(lock); - } - Err(AdvisoryLockError::TimedOut { .. }) => { - print!("TIMEOUT"); - std::io::stdout().flush().expect("flush stdout"); - } - Err(other) => panic!("unexpected child lock error: {other}"), - } - } - - fn run_child(git_dir: &Path, timeout: Duration) -> String { - let exe = std::env::current_exe().expect("test executable path should resolve"); - let output = Command::new(exe) - .args(["--exact", "--ignored", "--nocapture", CHILD_HELPER_PATH]) - .env(CHILD_ENV_GIT_DIR, git_dir) - .env(CHILD_ENV_TIMEOUT_MS, timeout.as_millis().to_string()) - .output() - .expect("the child test process should spawn"); - assert!( - output.status.success(), - "child process failed: stdout={:?} stderr={:?}", - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr), - ); - let stdout = String::from_utf8_lossy(&output.stdout); - if stdout.contains("ACQUIRED") { - "ACQUIRED".to_string() - } else if stdout.contains("TIMEOUT") { - "TIMEOUT".to_string() - } else { - panic!("child produced no lock verdict: {stdout}"); - } - } - - #[test] - fn process_death_releases_the_boundary_lock() { - let git_dir = unique_git_dir("process-death"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - let held = AdapterBoundaryLock::acquire(&git_dir, Duration::from_secs(5)) - .expect("the parent should acquire the boundary lock immediately"); - - assert_eq!( - run_child(&git_dir, Duration::from_millis(300)), - "TIMEOUT", - "a second OS process must not acquire the boundary lock while the parent holds it", - ); - - drop(held); - - assert_eq!( - run_child(&git_dir, Duration::from_millis(300)), - "ACQUIRED", - "once the parent releases, a child process acquires it", - ); - - let reacquired = AdapterBoundaryLock::acquire(&git_dir, Duration::from_secs(2)) - .expect("the child's process exit must have released the boundary lock"); - drop(reacquired); - - let _ = std::fs::remove_dir_all(&git_dir); - } - - #[test] - fn a_leftover_boundary_lock_file_alone_does_not_block_acquisition() { - let git_dir = unique_git_dir("leftover-file"); - std::fs::create_dir_all(adapter_state_dir(&git_dir)).expect("state dir should be created"); - std::fs::write(boundary_lock_path(&git_dir), b"leftover") - .expect("leftover lock file should be writable"); - - AdapterBoundaryLock::acquire(&git_dir, Duration::from_millis(200)) - .expect("a lock file with no live OS owner must not block a new acquirer"); - - let _ = std::fs::remove_dir_all(&git_dir); - } - - #[test] - fn a_second_in_process_acquirer_blocks_until_the_first_releases() { - let git_dir = unique_git_dir("in-process-contention"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - let holder = AdapterBoundaryLock::acquire(&git_dir, Duration::from_secs(5)) - .expect("first acquirer should succeed immediately"); - - let (tx, rx) = mpsc::channel(); - let git_dir_clone = git_dir.clone(); - let handle = thread::spawn(move || { - let result = AdapterBoundaryLock::acquire(&git_dir_clone, Duration::from_secs(5)); - let _ = tx.send(()); - result.is_ok() - }); - - assert!( - rx.recv_timeout(Duration::from_millis(300)).is_err(), - "the second acquirer must not proceed while the first holds the boundary lock", - ); - - drop(holder); - - rx.recv_timeout(Duration::from_secs(5)) - .expect("the second acquirer should complete once the first releases"); - assert!(handle - .join() - .expect("second acquirer thread should not panic")); - - let _ = std::fs::remove_dir_all(&git_dir); - } - - #[test] - fn the_boundary_lock_path_is_distinct_from_the_state_lock_and_lives_under_sce() { - let git_dir = unique_git_dir("path-shape"); - let path = boundary_lock_path(&git_dir); - assert!(path.starts_with(adapter_state_dir(&git_dir))); - assert!(path.ends_with(ADAPTER_BOUNDARY_LOCK_FILE)); - assert_ne!( - path.file_name(), - Path::new("codex-mutation-scope-state.lock").file_name(), - ); - } -} diff --git a/cli/src/services/hooks/codex_mutation_scope/lifecycle.rs b/cli/src/services/hooks/codex_mutation_scope/lifecycle.rs index 5eb417bd1..abef09050 100644 --- a/cli/src/services/hooks/codex_mutation_scope/lifecycle.rs +++ b/cli/src/services/hooks/codex_mutation_scope/lifecycle.rs @@ -1,6 +1,6 @@ use std::path::{Path, PathBuf}; -use anyhow::{anyhow, Result}; +use anyhow::Result; use crate::services::hooks; use crate::services::hooks::codex::bash_policy::{ @@ -12,7 +12,6 @@ use crate::services::hooks::{ use crate::services::mutation_trace::runtime::resolve_git_dir; use crate::services::observability::traits::Logger; -use super::boundary_lock::{AdapterBoundaryLock, DEFAULT_BOUNDARY_LOCK_TIMEOUT}; use super::events::CODEX_TRACKED_TOOL_BASH; use super::state; use super::{ @@ -250,8 +249,7 @@ pub(super) fn with_boundary_lock( git_dir: &Path, operation: impl FnOnce() -> Result, ) -> Result { - let _boundary = AdapterBoundaryLock::acquire(git_dir, DEFAULT_BOUNDARY_LOCK_TIMEOUT) - .map_err(|error| anyhow!("Failed to acquire adapter boundary lock: {error}"))?; + let _boundary = state::BOUNDARY_LOCK.acquire(&state::adapter_state_dir(git_dir))?; operation() } diff --git a/cli/src/services/hooks/codex_mutation_scope/mod.rs b/cli/src/services/hooks/codex_mutation_scope/mod.rs index fc904317c..365535907 100644 --- a/cli/src/services/hooks/codex_mutation_scope/mod.rs +++ b/cli/src/services/hooks/codex_mutation_scope/mod.rs @@ -1,10 +1,8 @@ #![allow(dead_code)] -mod boundary_lock; mod events; pub(crate) mod health; mod lifecycle; -mod os_lock; mod payload; pub(crate) mod state; diff --git a/cli/src/services/hooks/codex_mutation_scope/os_lock.rs b/cli/src/services/hooks/codex_mutation_scope/os_lock.rs deleted file mode 100644 index dc567c1b7..000000000 --- a/cli/src/services/hooks/codex_mutation_scope/os_lock.rs +++ /dev/null @@ -1,95 +0,0 @@ -use std::fs::{File, OpenOptions, TryLockError}; -use std::path::{Path, PathBuf}; -use std::time::{Duration, Instant}; - -use anyhow::Context; - -const LOCK_POLL_INTERVAL: Duration = Duration::from_millis(20); - -#[derive(Debug)] -pub(crate) enum AdvisoryLockError { - TimedOut { - path: PathBuf, - timeout: Duration, - what: &'static str, - }, - Io(anyhow::Error), -} - -impl std::fmt::Display for AdvisoryLockError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - AdvisoryLockError::TimedOut { - path, - timeout, - what, - } => write!( - f, - "Timed out after {timeout:?} waiting for the {what} lock '{}'", - path.display() - ), - AdvisoryLockError::Io(source) => write!(f, "{source}"), - } - } -} - -impl std::error::Error for AdvisoryLockError {} - -pub(crate) struct OsAdvisoryLock { - file: File, -} - -impl OsAdvisoryLock { - pub(crate) fn acquire( - parent_dir: &Path, - lock_path: PathBuf, - timeout: Duration, - what: &'static str, - ) -> Result { - std::fs::create_dir_all(parent_dir) - .with_context(|| { - format!( - "Failed to create {what} lock directory '{}'", - parent_dir.display() - ) - }) - .map_err(AdvisoryLockError::Io)?; - - let file = OpenOptions::new() - .write(true) - .create(true) - .truncate(false) - .open(&lock_path) - .with_context(|| format!("Failed to open {what} lock file '{}'", lock_path.display())) - .map_err(AdvisoryLockError::Io)?; - - let deadline = Instant::now() + timeout; - loop { - match file.try_lock() { - Ok(()) => return Ok(OsAdvisoryLock { file }), - Err(TryLockError::WouldBlock) => { - let now = Instant::now(); - if now >= deadline { - return Err(AdvisoryLockError::TimedOut { - path: lock_path, - timeout, - what, - }); - } - std::thread::sleep(LOCK_POLL_INTERVAL.min(deadline - now)); - } - Err(TryLockError::Error(source)) => { - return Err(AdvisoryLockError::Io(anyhow::Error::new(source).context( - format!("Failed to acquire {what} lock '{}'", lock_path.display()), - ))); - } - } - } - } -} - -impl Drop for OsAdvisoryLock { - fn drop(&mut self) { - let _ = self.file.unlock(); - } -} diff --git a/cli/src/services/hooks/codex_mutation_scope/state.rs b/cli/src/services/hooks/codex_mutation_scope/state.rs index e7ab576e5..833be77dd 100644 --- a/cli/src/services/hooks/codex_mutation_scope/state.rs +++ b/cli/src/services/hooks/codex_mutation_scope/state.rs @@ -1,20 +1,21 @@ use std::fs::OpenOptions; use std::io::Write as _; use std::path::{Path, PathBuf}; -use std::time::Duration; use anyhow::{anyhow, Context, Result}; use serde::{Deserialize, Serialize}; -use super::os_lock::{AdvisoryLockError, OsAdvisoryLock}; +use crate::services::hooks::mutation_scope_lock::{AdapterLockSpec, OsAdvisoryLock}; + use super::{format_codex_scope_id, AttemptKey}; const SCE_STATE_DIR: &str = "sce"; const ADAPTER_STATE_FILE: &str = "codex-mutation-scope-state.json"; -const ADAPTER_STATE_LOCK_FILE: &str = "codex-mutation-scope-state.lock"; -const STATE_LOCK_WHAT: &str = "adapter-state"; -const DEFAULT_LOCK_TIMEOUT: Duration = Duration::from_secs(10); +pub(crate) const STATE_LOCK: AdapterLockSpec = + AdapterLockSpec::state("codex-mutation-scope-state.lock"); +pub(crate) const BOUNDARY_LOCK: AdapterLockSpec = + AdapterLockSpec::boundary("codex-mutation-scope-boundary.lock"); const ADAPTER_STATE_VERSION: u32 = 3; @@ -124,26 +125,6 @@ pub(crate) fn state_path(git_dir: &Path) -> PathBuf { adapter_state_dir(git_dir).join(ADAPTER_STATE_FILE) } -fn lock_path(git_dir: &Path) -> PathBuf { - adapter_state_dir(git_dir).join(ADAPTER_STATE_LOCK_FILE) -} - -struct AdapterStateLock { - _inner: OsAdvisoryLock, -} - -impl AdapterStateLock { - fn acquire(git_dir: &Path, timeout: Duration) -> Result { - let inner = OsAdvisoryLock::acquire( - &adapter_state_dir(git_dir), - lock_path(git_dir), - timeout, - STATE_LOCK_WHAT, - )?; - Ok(AdapterStateLock { _inner: inner }) - } -} - pub(crate) fn read_state(git_dir: &Path) -> Result { let path = state_path(git_dir); if !path.exists() { @@ -240,9 +221,8 @@ where Ok(()) } -fn acquire_lock(git_dir: &Path) -> Result { - AdapterStateLock::acquire(git_dir, DEFAULT_LOCK_TIMEOUT) - .map_err(|err| anyhow!("Failed to acquire adapter-state lock: {err}")) +fn acquire_lock(git_dir: &Path) -> Result { + STATE_LOCK.acquire(&adapter_state_dir(git_dir)) } fn allocate_pending_start( @@ -456,6 +436,7 @@ pub(crate) fn arm_mark_active_failure_for_tests() { mod tests { use std::sync::atomic::{AtomicU64, Ordering}; use std::thread; + use std::time::Duration; use super::*; @@ -1299,7 +1280,7 @@ mod tests { let git_dir = unique_test_git_dir("leftover-lock-file"); let dir = adapter_state_dir(&git_dir); std::fs::create_dir_all(&dir).expect("state dir should be created"); - std::fs::write(lock_path(&git_dir), b"leftover") + std::fs::write(STATE_LOCK.path(&dir), b"leftover") .expect("leftover lock file should be writable"); let decision = admit_tracked_attempt( @@ -1374,42 +1355,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn a_second_acquirer_blocks_until_the_first_releases() { - use std::sync::mpsc; - - let git_dir = unique_test_git_dir("lock-contention"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - let holder = AdapterStateLock::acquire(&git_dir, Duration::from_secs(5)) - .expect("first acquirer should succeed immediately"); - - let (result_tx, result_rx) = mpsc::channel(); - let git_dir_clone = git_dir.clone(); - let handle = thread::spawn(move || { - let result = AdapterStateLock::acquire(&git_dir_clone, Duration::from_secs(5)); - let _ = result_tx.send(()); - result - }); - - assert!( - result_rx.recv_timeout(Duration::from_millis(300)).is_err(), - "second acquirer should not succeed while the first still holds the lock" - ); - - drop(holder); - - result_rx - .recv_timeout(Duration::from_secs(5)) - .expect("second acquirer should complete once the first releases the lock"); - assert!(handle - .join() - .expect("second acquirer thread should not panic") - .is_ok()); - - remove_test_git_dir(&git_dir); - } - #[test] fn every_locked_helper_releases_the_state_lock_before_returning() { let git_dir = unique_test_git_dir("lock-released-between-helpers"); @@ -1430,7 +1375,8 @@ mod tests { .expect("relinquish_recovery_flush must acquire the lock"); drop( - AdapterStateLock::acquire(&git_dir, Duration::from_millis(200)) + STATE_LOCK + .acquire_with_timeout(&adapter_state_dir(&git_dir), Duration::from_millis(200)) .expect("the state lock must be free once every helper has returned"), ); @@ -1446,7 +1392,9 @@ mod tests { let sce_dir = git_dir.join(SCE_STATE_DIR); assert!(state_path(&git_dir).starts_with(&sce_dir)); - assert!(lock_path(&git_dir).starts_with(&sce_dir)); + assert!(STATE_LOCK + .path(&adapter_state_dir(&git_dir)) + .starts_with(&sce_dir)); let mut found_state_file = false; for entry in std::fs::read_dir(&sce_dir).expect("sce dir should be readable") { diff --git a/cli/src/services/hooks/codex_mutation_scope/tests.rs b/cli/src/services/hooks/codex_mutation_scope/tests.rs index 5aa72d25e..2418d4953 100644 --- a/cli/src/services/hooks/codex_mutation_scope/tests.rs +++ b/cli/src/services/hooks/codex_mutation_scope/tests.rs @@ -2249,10 +2249,9 @@ mod driver { } assert!( - !crate::services::hooks::codex_mutation_scope::boundary_lock::boundary_lock_path( - &git_dir - ) - .exists(), + !state::BOUNDARY_LOCK + .path(&state::adapter_state_dir(&git_dir)) + .exists(), "Test M: no untracked tool may create the adapter boundary lock", ); assert!( @@ -2299,10 +2298,9 @@ mod driver { "an untracked PostToolUse resolves no git dir and creates no adapter state directory", ); assert!( - !crate::services::hooks::codex_mutation_scope::boundary_lock::boundary_lock_path( - &git_dir - ) - .exists(), + !state::BOUNDARY_LOCK + .path(&state::adapter_state_dir(&git_dir)) + .exists(), "an untracked PostToolUse must not create the adapter boundary lock", ); @@ -2359,10 +2357,9 @@ mod driver { state lock, or boundary lock", ); assert!( - !crate::services::hooks::codex_mutation_scope::boundary_lock::boundary_lock_path( - &git_dir - ) - .exists(), + !state::BOUNDARY_LOCK + .path(&state::adapter_state_dir(&git_dir)) + .exists(), "a complete successful MCP lifecycle creates no boundary lock", ); @@ -2485,7 +2482,8 @@ mod driver { } fn boundary_lock_exists(git_dir: &Path) -> bool { - crate::services::hooks::codex_mutation_scope::boundary_lock::boundary_lock_path(git_dir) + state::BOUNDARY_LOCK + .path(&state::adapter_state_dir(git_dir)) .exists() } diff --git a/cli/src/services/hooks/mod.rs b/cli/src/services/hooks/mod.rs index a42fad7b2..e8b00793f 100644 --- a/cli/src/services/hooks/mod.rs +++ b/cli/src/services/hooks/mod.rs @@ -29,6 +29,7 @@ mod claude_transforms; mod commit_hooks; mod conversation_trace; mod diff_trace; +mod mutation_scope_lock; mod runtime; pub mod claude_bridge_session; diff --git a/cli/src/services/hooks/mutation_scope_lock.rs b/cli/src/services/hooks/mutation_scope_lock.rs new file mode 100644 index 000000000..78d6b20eb --- /dev/null +++ b/cli/src/services/hooks/mutation_scope_lock.rs @@ -0,0 +1,396 @@ +use std::fs::{File, OpenOptions, TryLockError}; +use std::path::{Path, PathBuf}; +use std::time::{Duration, Instant}; + +use anyhow::{anyhow, Context}; + +const LOCK_POLL_INTERVAL: Duration = Duration::from_millis(20); + +pub(crate) const DEFAULT_STATE_LOCK_TIMEOUT: Duration = Duration::from_secs(10); +pub(crate) const DEFAULT_BOUNDARY_LOCK_TIMEOUT: Duration = Duration::from_secs(10); + +const STATE_LOCK_WHAT: &str = "adapter-state"; +const BOUNDARY_LOCK_WHAT: &str = "adapter-boundary"; + +const STATE_LOCK_FAILURE_CONTEXT: &str = "Failed to acquire adapter-state lock"; +const BOUNDARY_LOCK_FAILURE_CONTEXT: &str = "Failed to acquire adapter boundary lock"; + +#[derive(Debug)] +pub(crate) enum AdvisoryLockError { + TimedOut { + path: PathBuf, + timeout: Duration, + what: &'static str, + }, + Io(anyhow::Error), +} + +impl std::fmt::Display for AdvisoryLockError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + AdvisoryLockError::TimedOut { + path, + timeout, + what, + } => write!( + f, + "Timed out after {timeout:?} waiting for the {what} lock '{}'", + path.display() + ), + AdvisoryLockError::Io(source) => write!(f, "{source}"), + } + } +} + +impl std::error::Error for AdvisoryLockError {} + +pub(crate) struct OsAdvisoryLock { + file: File, +} + +impl OsAdvisoryLock { + fn acquire( + parent_dir: &Path, + lock_path: PathBuf, + timeout: Duration, + what: &'static str, + ) -> Result { + std::fs::create_dir_all(parent_dir) + .with_context(|| { + format!( + "Failed to create {what} lock directory '{}'", + parent_dir.display() + ) + }) + .map_err(AdvisoryLockError::Io)?; + + let file = OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .open(&lock_path) + .with_context(|| format!("Failed to open {what} lock file '{}'", lock_path.display())) + .map_err(AdvisoryLockError::Io)?; + + let deadline = Instant::now() + timeout; + loop { + match file.try_lock() { + Ok(()) => return Ok(OsAdvisoryLock { file }), + Err(TryLockError::WouldBlock) => { + let now = Instant::now(); + if now >= deadline { + return Err(AdvisoryLockError::TimedOut { + path: lock_path, + timeout, + what, + }); + } + std::thread::sleep(LOCK_POLL_INTERVAL.min(deadline - now)); + } + Err(TryLockError::Error(source)) => { + return Err(AdvisoryLockError::Io(anyhow::Error::new(source).context( + format!("Failed to acquire {what} lock '{}'", lock_path.display()), + ))); + } + } + } + } +} + +impl Drop for OsAdvisoryLock { + fn drop(&mut self) { + let _ = self.file.unlock(); + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) struct AdapterLockSpec { + file_name: &'static str, + what: &'static str, + default_timeout: Duration, + failure_context: &'static str, +} + +impl AdapterLockSpec { + pub(crate) const fn state(file_name: &'static str) -> Self { + AdapterLockSpec { + file_name, + what: STATE_LOCK_WHAT, + default_timeout: DEFAULT_STATE_LOCK_TIMEOUT, + failure_context: STATE_LOCK_FAILURE_CONTEXT, + } + } + + pub(crate) const fn boundary(file_name: &'static str) -> Self { + AdapterLockSpec { + file_name, + what: BOUNDARY_LOCK_WHAT, + default_timeout: DEFAULT_BOUNDARY_LOCK_TIMEOUT, + failure_context: BOUNDARY_LOCK_FAILURE_CONTEXT, + } + } + + pub(crate) fn path(&self, adapter_state_dir: &Path) -> PathBuf { + adapter_state_dir.join(self.file_name) + } + + pub(crate) fn acquire(&self, adapter_state_dir: &Path) -> anyhow::Result { + self.acquire_with_timeout(adapter_state_dir, self.default_timeout) + .map_err(|error| anyhow!("{}: {error}", self.failure_context)) + } + + pub(crate) fn acquire_with_timeout( + &self, + adapter_state_dir: &Path, + timeout: Duration, + ) -> Result { + OsAdvisoryLock::acquire( + adapter_state_dir, + self.path(adapter_state_dir), + timeout, + self.what, + ) + } +} + +#[cfg(test)] +mod tests { + use std::io::Write as _; + use std::process::Command; + use std::sync::atomic::{AtomicU64, Ordering}; + use std::sync::mpsc; + use std::thread; + + use super::*; + use crate::services::hooks::{ + codex_mutation_scope, opencode_mutation_scope, pi_mutation_scope, + }; + + const TEST_LOCK: AdapterLockSpec = AdapterLockSpec::boundary("test-mutation-scope.lock"); + + static NEXT_ID: AtomicU64 = AtomicU64::new(0); + + fn unique_lock_dir(label: &str) -> PathBuf { + let id = NEXT_ID.fetch_add(1, Ordering::Relaxed); + std::env::temp_dir().join(format!( + "sce-mutation-scope-lock-{label}-{}-{id}", + std::process::id() + )) + } + + const CHILD_ENV_LOCK_DIR: &str = "SCE_MUTATION_SCOPE_LOCK_CHILD_DIR"; + const CHILD_ENV_TIMEOUT_MS: &str = "SCE_MUTATION_SCOPE_LOCK_CHILD_TIMEOUT_MS"; + const CHILD_HELPER_PATH: &str = + "services::hooks::mutation_scope_lock::tests::mutation_scope_lock_child_helper"; + + #[test] + #[ignore = "subprocess helper, driven by process_death_releases_the_lock"] + fn mutation_scope_lock_child_helper() { + let lock_dir = PathBuf::from( + std::env::var(CHILD_ENV_LOCK_DIR).expect("child helper needs a lock dir in the env"), + ); + let timeout = Duration::from_millis( + std::env::var(CHILD_ENV_TIMEOUT_MS) + .expect("child helper needs a timeout in the env") + .parse() + .expect("timeout must parse"), + ); + match TEST_LOCK.acquire_with_timeout(&lock_dir, timeout) { + Ok(lock) => { + print!("ACQUIRED"); + std::io::stdout().flush().expect("flush stdout"); + std::mem::forget(lock); + } + Err(AdvisoryLockError::TimedOut { .. }) => { + print!("TIMEOUT"); + std::io::stdout().flush().expect("flush stdout"); + } + Err(other) => panic!("unexpected child lock error: {other}"), + } + } + + fn run_child(lock_dir: &Path, timeout: Duration) -> String { + let exe = std::env::current_exe().expect("test executable path should resolve"); + let output = Command::new(exe) + .args(["--exact", "--ignored", "--nocapture", CHILD_HELPER_PATH]) + .env(CHILD_ENV_LOCK_DIR, lock_dir) + .env(CHILD_ENV_TIMEOUT_MS, timeout.as_millis().to_string()) + .output() + .expect("the child test process should spawn"); + assert!( + output.status.success(), + "child process failed: stdout={:?} stderr={:?}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + let stdout = String::from_utf8_lossy(&output.stdout); + if stdout.contains("ACQUIRED") { + "ACQUIRED".to_string() + } else if stdout.contains("TIMEOUT") { + "TIMEOUT".to_string() + } else { + panic!("child produced no lock verdict: {stdout}"); + } + } + + #[test] + fn process_death_releases_the_lock() { + let lock_dir = unique_lock_dir("process-death"); + + let held = TEST_LOCK + .acquire_with_timeout(&lock_dir, Duration::from_secs(5)) + .expect("the parent should acquire the lock immediately"); + + assert_eq!( + run_child(&lock_dir, Duration::from_millis(300)), + "TIMEOUT", + "a second OS process must not acquire the lock while the parent holds it", + ); + + drop(held); + + assert_eq!( + run_child(&lock_dir, Duration::from_millis(300)), + "ACQUIRED", + "once the parent releases, a child process acquires it", + ); + + let reacquired = TEST_LOCK + .acquire_with_timeout(&lock_dir, Duration::from_secs(2)) + .expect("the child's process exit must have released the lock"); + drop(reacquired); + + let _ = std::fs::remove_dir_all(&lock_dir); + } + + #[test] + fn a_leftover_lock_file_alone_does_not_block_acquisition_and_is_never_truncated_or_removed() { + let lock_dir = unique_lock_dir("leftover-file"); + std::fs::create_dir_all(&lock_dir).expect("lock dir should be created"); + let lock_path = TEST_LOCK.path(&lock_dir); + std::fs::write(&lock_path, b"leftover").expect("leftover lock file should be writable"); + + let lock = TEST_LOCK + .acquire_with_timeout(&lock_dir, Duration::from_millis(200)) + .expect("a lock file with no live OS owner must not block a new acquirer"); + drop(lock); + + assert_eq!( + std::fs::read(&lock_path).expect("the lock file must survive release"), + b"leftover", + "ownership must never be expressed by truncating or removing the lock file", + ); + + let _ = std::fs::remove_dir_all(&lock_dir); + } + + #[test] + fn a_second_in_process_acquirer_blocks_until_the_first_releases() { + let lock_dir = unique_lock_dir("in-process-contention"); + + let holder = TEST_LOCK + .acquire_with_timeout(&lock_dir, Duration::from_secs(5)) + .expect("first acquirer should succeed immediately"); + + let (tx, rx) = mpsc::channel(); + let lock_dir_clone = lock_dir.clone(); + let handle = thread::spawn(move || { + let result = TEST_LOCK.acquire_with_timeout(&lock_dir_clone, Duration::from_secs(5)); + let _ = tx.send(()); + result.is_ok() + }); + + assert!( + rx.recv_timeout(Duration::from_millis(300)).is_err(), + "the second acquirer must not proceed while the first holds the lock", + ); + + drop(holder); + + rx.recv_timeout(Duration::from_secs(5)) + .expect("the second acquirer should complete once the first releases"); + assert!(handle + .join() + .expect("second acquirer thread should not panic")); + + let _ = std::fs::remove_dir_all(&lock_dir); + } + + #[test] + fn a_contended_acquire_times_out_with_a_role_specific_diagnostic() { + let lock_dir = unique_lock_dir("timeout"); + let state_lock = AdapterLockSpec::state("test-mutation-scope-state.lock"); + + let _boundary_holder = TEST_LOCK + .acquire_with_timeout(&lock_dir, Duration::from_secs(5)) + .expect("boundary holder should acquire immediately"); + let _state_holder = state_lock + .acquire_with_timeout(&lock_dir, Duration::from_secs(5)) + .expect("state holder should acquire immediately"); + + for (spec, what) in [ + (TEST_LOCK, "adapter-boundary"), + (state_lock, "adapter-state"), + ] { + let timeout = Duration::from_millis(100); + let started = Instant::now(); + let Err(error) = spec.acquire_with_timeout(&lock_dir, timeout) else { + panic!("a held {what} lock must not be acquired a second time"); + }; + assert!( + started.elapsed() < Duration::from_secs(5), + "a contended acquire must give up at its timeout", + ); + let expected = format!( + "Timed out after {timeout:?} waiting for the {what} lock '{}'", + spec.path(&lock_dir).display() + ); + assert_eq!(error.to_string(), expected); + assert!(matches!( + &error, + AdvisoryLockError::TimedOut { path, timeout: t, what: w } + if *path == spec.path(&lock_dir) && *t == timeout && *w == what + )); + } + + let _ = std::fs::remove_dir_all(&lock_dir); + } + + #[test] + fn adapter_lock_identities_are_exact_distinct_and_below_the_adapter_state_dir() { + let git_dir = unique_lock_dir("adapter-identities"); + let rows = [ + ( + codex_mutation_scope::state::adapter_state_dir(&git_dir), + codex_mutation_scope::state::STATE_LOCK, + codex_mutation_scope::state::BOUNDARY_LOCK, + "codex-mutation-scope-state.lock", + "codex-mutation-scope-boundary.lock", + ), + ( + opencode_mutation_scope::state::adapter_state_dir(&git_dir), + opencode_mutation_scope::state::STATE_LOCK, + opencode_mutation_scope::state::BOUNDARY_LOCK, + "opencode-mutation-scope-state.lock", + "opencode-mutation-scope-boundary.lock", + ), + ( + pi_mutation_scope::state::adapter_state_dir(&git_dir), + pi_mutation_scope::state::STATE_LOCK, + pi_mutation_scope::state::BOUNDARY_LOCK, + "pi-mutation-scope-state.lock", + "pi-mutation-scope-boundary.lock", + ), + ]; + + let sce_dir = git_dir.join("sce"); + for (state_dir, state_lock, boundary_lock, state_file, boundary_file) in rows { + assert_eq!(state_dir, sce_dir); + assert_eq!(state_lock, AdapterLockSpec::state(state_file)); + assert_eq!(boundary_lock, AdapterLockSpec::boundary(boundary_file)); + assert_eq!(state_lock.path(&state_dir), sce_dir.join(state_file)); + assert_eq!(boundary_lock.path(&state_dir), sce_dir.join(boundary_file)); + assert_ne!(state_lock.path(&state_dir), boundary_lock.path(&state_dir)); + } + } +} diff --git a/cli/src/services/hooks/opencode_mutation_scope/boundary_lock.rs b/cli/src/services/hooks/opencode_mutation_scope/boundary_lock.rs deleted file mode 100644 index 863343a12..000000000 --- a/cli/src/services/hooks/opencode_mutation_scope/boundary_lock.rs +++ /dev/null @@ -1,109 +0,0 @@ -use std::path::{Path, PathBuf}; -use std::time::Duration; - -use super::os_lock::{AdvisoryLockError, OsAdvisoryLock}; -use super::state::adapter_state_dir; - -const ADAPTER_BOUNDARY_LOCK_FILE: &str = "opencode-mutation-scope-boundary.lock"; -const BOUNDARY_LOCK_WHAT: &str = "adapter-boundary"; - -pub(crate) const DEFAULT_BOUNDARY_LOCK_TIMEOUT: Duration = Duration::from_secs(10); - -pub(crate) fn boundary_lock_path(git_dir: &Path) -> PathBuf { - adapter_state_dir(git_dir).join(ADAPTER_BOUNDARY_LOCK_FILE) -} - -pub(crate) struct AdapterBoundaryLock { - _inner: OsAdvisoryLock, -} - -impl AdapterBoundaryLock { - pub(crate) fn acquire( - git_dir: &Path, - timeout: Duration, - ) -> Result { - let inner = OsAdvisoryLock::acquire( - &adapter_state_dir(git_dir), - boundary_lock_path(git_dir), - timeout, - BOUNDARY_LOCK_WHAT, - )?; - Ok(AdapterBoundaryLock { _inner: inner }) - } -} - -#[cfg(test)] -mod tests { - use std::sync::atomic::{AtomicU64, Ordering}; - use std::sync::mpsc; - use std::thread; - - use super::*; - - static NEXT_ID: AtomicU64 = AtomicU64::new(0); - - fn unique_git_dir(label: &str) -> PathBuf { - let id = NEXT_ID.fetch_add(1, Ordering::Relaxed); - std::env::temp_dir().join(format!( - "sce-opencode-mutation-scope-boundary-{label}-{}-{id}", - std::process::id() - )) - } - - #[test] - fn a_leftover_boundary_lock_file_alone_does_not_block_acquisition() { - let git_dir = unique_git_dir("leftover-file"); - std::fs::create_dir_all(adapter_state_dir(&git_dir)).expect("state dir should be created"); - std::fs::write(boundary_lock_path(&git_dir), b"leftover") - .expect("leftover lock file should be writable"); - - AdapterBoundaryLock::acquire(&git_dir, Duration::from_millis(200)) - .expect("a lock file with no live OS owner must not block a new acquirer"); - - let _ = std::fs::remove_dir_all(&git_dir); - } - - #[test] - fn a_second_in_process_acquirer_blocks_until_the_first_releases() { - let git_dir = unique_git_dir("in-process-contention"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - let holder = AdapterBoundaryLock::acquire(&git_dir, Duration::from_secs(5)) - .expect("first acquirer should succeed immediately"); - - let (tx, rx) = mpsc::channel(); - let git_dir_clone = git_dir.clone(); - let handle = thread::spawn(move || { - let result = AdapterBoundaryLock::acquire(&git_dir_clone, Duration::from_secs(5)); - let _ = tx.send(()); - result.is_ok() - }); - - assert!( - rx.recv_timeout(Duration::from_millis(300)).is_err(), - "the second acquirer must not proceed while the first holds the boundary lock", - ); - - drop(holder); - - rx.recv_timeout(Duration::from_secs(5)) - .expect("the second acquirer should complete once the first releases"); - assert!(handle - .join() - .expect("second acquirer thread should not panic")); - - let _ = std::fs::remove_dir_all(&git_dir); - } - - #[test] - fn the_boundary_lock_path_is_distinct_from_the_state_lock_and_lives_under_sce() { - let git_dir = unique_git_dir("path-shape"); - let path = boundary_lock_path(&git_dir); - assert!(path.starts_with(adapter_state_dir(&git_dir))); - assert!(path.ends_with(ADAPTER_BOUNDARY_LOCK_FILE)); - assert_ne!( - path.file_name(), - Path::new("opencode-mutation-scope-state.lock").file_name(), - ); - } -} diff --git a/cli/src/services/hooks/opencode_mutation_scope/lifecycle.rs b/cli/src/services/hooks/opencode_mutation_scope/lifecycle.rs index 8f8a7492c..a1d5215a7 100644 --- a/cli/src/services/hooks/opencode_mutation_scope/lifecycle.rs +++ b/cli/src/services/hooks/opencode_mutation_scope/lifecycle.rs @@ -1,12 +1,11 @@ use std::path::{Path, PathBuf}; -use anyhow::{anyhow, bail, Result}; +use anyhow::{bail, Result}; use crate::services::hooks; use crate::services::mutation_trace::runtime::resolve_git_dir; use crate::services::observability::traits::Logger; -use super::boundary_lock::{AdapterBoundaryLock, DEFAULT_BOUNDARY_LOCK_TIMEOUT}; use super::events::{ opencode_scope_close_event_id, opencode_scope_provenance, opencode_scope_start_event_id, parse_opencode_hook_event, AttemptKey, OpenCodeHookEvent, OpenCodeScopeProvenance, @@ -202,8 +201,7 @@ pub(super) fn with_boundary_lock( git_dir: &Path, operation: impl FnOnce() -> Result, ) -> Result { - let _boundary = AdapterBoundaryLock::acquire(git_dir, DEFAULT_BOUNDARY_LOCK_TIMEOUT) - .map_err(|error| anyhow!("Failed to acquire adapter boundary lock: {error}"))?; + let _boundary = state::BOUNDARY_LOCK.acquire(&state::adapter_state_dir(git_dir))?; operation() } diff --git a/cli/src/services/hooks/opencode_mutation_scope/mod.rs b/cli/src/services/hooks/opencode_mutation_scope/mod.rs index bae7d60bd..cca8c28b0 100644 --- a/cli/src/services/hooks/opencode_mutation_scope/mod.rs +++ b/cli/src/services/hooks/opencode_mutation_scope/mod.rs @@ -1,10 +1,8 @@ #![allow(dead_code)] -mod boundary_lock; mod events; pub(crate) mod health; mod lifecycle; -mod os_lock; mod payload; pub(crate) mod state; diff --git a/cli/src/services/hooks/opencode_mutation_scope/os_lock.rs b/cli/src/services/hooks/opencode_mutation_scope/os_lock.rs deleted file mode 100644 index dc567c1b7..000000000 --- a/cli/src/services/hooks/opencode_mutation_scope/os_lock.rs +++ /dev/null @@ -1,95 +0,0 @@ -use std::fs::{File, OpenOptions, TryLockError}; -use std::path::{Path, PathBuf}; -use std::time::{Duration, Instant}; - -use anyhow::Context; - -const LOCK_POLL_INTERVAL: Duration = Duration::from_millis(20); - -#[derive(Debug)] -pub(crate) enum AdvisoryLockError { - TimedOut { - path: PathBuf, - timeout: Duration, - what: &'static str, - }, - Io(anyhow::Error), -} - -impl std::fmt::Display for AdvisoryLockError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - AdvisoryLockError::TimedOut { - path, - timeout, - what, - } => write!( - f, - "Timed out after {timeout:?} waiting for the {what} lock '{}'", - path.display() - ), - AdvisoryLockError::Io(source) => write!(f, "{source}"), - } - } -} - -impl std::error::Error for AdvisoryLockError {} - -pub(crate) struct OsAdvisoryLock { - file: File, -} - -impl OsAdvisoryLock { - pub(crate) fn acquire( - parent_dir: &Path, - lock_path: PathBuf, - timeout: Duration, - what: &'static str, - ) -> Result { - std::fs::create_dir_all(parent_dir) - .with_context(|| { - format!( - "Failed to create {what} lock directory '{}'", - parent_dir.display() - ) - }) - .map_err(AdvisoryLockError::Io)?; - - let file = OpenOptions::new() - .write(true) - .create(true) - .truncate(false) - .open(&lock_path) - .with_context(|| format!("Failed to open {what} lock file '{}'", lock_path.display())) - .map_err(AdvisoryLockError::Io)?; - - let deadline = Instant::now() + timeout; - loop { - match file.try_lock() { - Ok(()) => return Ok(OsAdvisoryLock { file }), - Err(TryLockError::WouldBlock) => { - let now = Instant::now(); - if now >= deadline { - return Err(AdvisoryLockError::TimedOut { - path: lock_path, - timeout, - what, - }); - } - std::thread::sleep(LOCK_POLL_INTERVAL.min(deadline - now)); - } - Err(TryLockError::Error(source)) => { - return Err(AdvisoryLockError::Io(anyhow::Error::new(source).context( - format!("Failed to acquire {what} lock '{}'", lock_path.display()), - ))); - } - } - } - } -} - -impl Drop for OsAdvisoryLock { - fn drop(&mut self) { - let _ = self.file.unlock(); - } -} diff --git a/cli/src/services/hooks/opencode_mutation_scope/state.rs b/cli/src/services/hooks/opencode_mutation_scope/state.rs index a8c0a0d85..61011eadb 100644 --- a/cli/src/services/hooks/opencode_mutation_scope/state.rs +++ b/cli/src/services/hooks/opencode_mutation_scope/state.rs @@ -1,12 +1,12 @@ use std::fs::OpenOptions; use std::io::Write as _; use std::path::{Path, PathBuf}; -use std::time::Duration; use anyhow::{anyhow, Context, Result}; use serde::{Deserialize, Serialize}; -use super::os_lock::{AdvisoryLockError, OsAdvisoryLock}; +use crate::services::hooks::mutation_scope_lock::{AdapterLockSpec, OsAdvisoryLock}; + use super::{format_opencode_scope_id, AttemptKey}; use crate::services::hooks::mutation_scope_owner::{ current_process_owner, is_definitely_dead, ProcessOwner, @@ -14,10 +14,11 @@ use crate::services::hooks::mutation_scope_owner::{ const SCE_STATE_DIR: &str = "sce"; const ADAPTER_STATE_FILE: &str = "opencode-mutation-scope-state.json"; -const ADAPTER_STATE_LOCK_FILE: &str = "opencode-mutation-scope-state.lock"; -const STATE_LOCK_WHAT: &str = "adapter-state"; -const DEFAULT_LOCK_TIMEOUT: Duration = Duration::from_secs(10); +pub(crate) const STATE_LOCK: AdapterLockSpec = + AdapterLockSpec::state("opencode-mutation-scope-state.lock"); +pub(crate) const BOUNDARY_LOCK: AdapterLockSpec = + AdapterLockSpec::boundary("opencode-mutation-scope-boundary.lock"); const ADAPTER_STATE_VERSION: u32 = 1; @@ -119,26 +120,6 @@ pub(crate) fn state_path(git_dir: &Path) -> PathBuf { adapter_state_dir(git_dir).join(ADAPTER_STATE_FILE) } -fn lock_path(git_dir: &Path) -> PathBuf { - adapter_state_dir(git_dir).join(ADAPTER_STATE_LOCK_FILE) -} - -struct AdapterStateLock { - _inner: OsAdvisoryLock, -} - -impl AdapterStateLock { - fn acquire(git_dir: &Path, timeout: Duration) -> Result { - let inner = OsAdvisoryLock::acquire( - &adapter_state_dir(git_dir), - lock_path(git_dir), - timeout, - STATE_LOCK_WHAT, - )?; - Ok(AdapterStateLock { _inner: inner }) - } -} - pub(crate) fn read_state(git_dir: &Path) -> Result { let path = state_path(git_dir); if !path.exists() { @@ -235,9 +216,8 @@ where Ok(()) } -fn acquire_lock(git_dir: &Path) -> Result { - AdapterStateLock::acquire(git_dir, DEFAULT_LOCK_TIMEOUT) - .map_err(|err| anyhow!("Failed to acquire adapter-state lock: {err}")) +fn acquire_lock(git_dir: &Path) -> Result { + STATE_LOCK.acquire(&adapter_state_dir(git_dir)) } fn allocate_pending_start( @@ -520,6 +500,7 @@ pub(crate) fn set_attempt_owner_for_tests( mod tests { use std::sync::atomic::{AtomicU64, Ordering}; use std::thread; + use std::time::Duration; use super::*; @@ -1136,7 +1117,9 @@ mod tests { let sce_dir = git_dir.join(SCE_STATE_DIR); assert!(state_path(&git_dir).starts_with(&sce_dir)); - assert!(lock_path(&git_dir).starts_with(&sce_dir)); + assert!(STATE_LOCK + .path(&adapter_state_dir(&git_dir)) + .starts_with(&sce_dir)); remove_test_git_dir(&git_dir); } diff --git a/cli/src/services/hooks/pi_mutation_scope/boundary_lock.rs b/cli/src/services/hooks/pi_mutation_scope/boundary_lock.rs deleted file mode 100644 index b8665bc6e..000000000 --- a/cli/src/services/hooks/pi_mutation_scope/boundary_lock.rs +++ /dev/null @@ -1,109 +0,0 @@ -use std::path::{Path, PathBuf}; -use std::time::Duration; - -use super::os_lock::{AdvisoryLockError, OsAdvisoryLock}; -use super::state::adapter_state_dir; - -const ADAPTER_BOUNDARY_LOCK_FILE: &str = "pi-mutation-scope-boundary.lock"; -const BOUNDARY_LOCK_WHAT: &str = "adapter-boundary"; - -pub(crate) const DEFAULT_BOUNDARY_LOCK_TIMEOUT: Duration = Duration::from_secs(10); - -pub(crate) fn boundary_lock_path(git_dir: &Path) -> PathBuf { - adapter_state_dir(git_dir).join(ADAPTER_BOUNDARY_LOCK_FILE) -} - -pub(crate) struct AdapterBoundaryLock { - _inner: OsAdvisoryLock, -} - -impl AdapterBoundaryLock { - pub(crate) fn acquire( - git_dir: &Path, - timeout: Duration, - ) -> Result { - let inner = OsAdvisoryLock::acquire( - &adapter_state_dir(git_dir), - boundary_lock_path(git_dir), - timeout, - BOUNDARY_LOCK_WHAT, - )?; - Ok(AdapterBoundaryLock { _inner: inner }) - } -} - -#[cfg(test)] -mod tests { - use std::sync::atomic::{AtomicU64, Ordering}; - use std::sync::mpsc; - use std::thread; - - use super::*; - - static NEXT_ID: AtomicU64 = AtomicU64::new(0); - - fn unique_git_dir(label: &str) -> PathBuf { - let id = NEXT_ID.fetch_add(1, Ordering::Relaxed); - std::env::temp_dir().join(format!( - "sce-pi-mutation-scope-boundary-{label}-{}-{id}", - std::process::id() - )) - } - - #[test] - fn a_leftover_boundary_lock_file_alone_does_not_block_acquisition() { - let git_dir = unique_git_dir("leftover-file"); - std::fs::create_dir_all(adapter_state_dir(&git_dir)).expect("state dir should be created"); - std::fs::write(boundary_lock_path(&git_dir), b"leftover") - .expect("leftover lock file should be writable"); - - AdapterBoundaryLock::acquire(&git_dir, Duration::from_millis(200)) - .expect("a lock file with no live OS owner must not block a new acquirer"); - - let _ = std::fs::remove_dir_all(&git_dir); - } - - #[test] - fn a_second_in_process_acquirer_blocks_until_the_first_releases() { - let git_dir = unique_git_dir("in-process-contention"); - std::fs::create_dir_all(&git_dir).expect("git dir should be created"); - - let holder = AdapterBoundaryLock::acquire(&git_dir, Duration::from_secs(5)) - .expect("first acquirer should succeed immediately"); - - let (tx, rx) = mpsc::channel(); - let git_dir_clone = git_dir.clone(); - let handle = thread::spawn(move || { - let result = AdapterBoundaryLock::acquire(&git_dir_clone, Duration::from_secs(5)); - let _ = tx.send(()); - result.is_ok() - }); - - assert!( - rx.recv_timeout(Duration::from_millis(300)).is_err(), - "the second acquirer must not proceed while the first holds the boundary lock", - ); - - drop(holder); - - rx.recv_timeout(Duration::from_secs(5)) - .expect("the second acquirer should complete once the first releases"); - assert!(handle - .join() - .expect("second acquirer thread should not panic")); - - let _ = std::fs::remove_dir_all(&git_dir); - } - - #[test] - fn the_boundary_lock_path_is_distinct_from_the_state_lock_and_lives_under_sce() { - let git_dir = unique_git_dir("path-shape"); - let path = boundary_lock_path(&git_dir); - assert!(path.starts_with(adapter_state_dir(&git_dir))); - assert!(path.ends_with(ADAPTER_BOUNDARY_LOCK_FILE)); - assert_ne!( - path.file_name(), - Path::new("pi-mutation-scope-state.lock").file_name(), - ); - } -} diff --git a/cli/src/services/hooks/pi_mutation_scope/lifecycle.rs b/cli/src/services/hooks/pi_mutation_scope/lifecycle.rs index 4d6202ebf..d02dc258f 100644 --- a/cli/src/services/hooks/pi_mutation_scope/lifecycle.rs +++ b/cli/src/services/hooks/pi_mutation_scope/lifecycle.rs @@ -1,12 +1,11 @@ use std::path::{Path, PathBuf}; -use anyhow::{anyhow, bail, Result}; +use anyhow::{bail, Result}; use crate::services::hooks; use crate::services::mutation_trace::runtime::resolve_git_dir; use crate::services::observability::traits::Logger; -use super::boundary_lock::{AdapterBoundaryLock, DEFAULT_BOUNDARY_LOCK_TIMEOUT}; use super::state::{self, AdmitDecision, RecoveryFlushCompletion}; use super::{ abandon_payload, flush_payload, parse_pi_hook_event, pi_scope_close_event_id, @@ -162,8 +161,7 @@ pub(super) fn with_boundary_lock( git_dir: &Path, operation: impl FnOnce() -> Result, ) -> Result { - let _boundary = AdapterBoundaryLock::acquire(git_dir, DEFAULT_BOUNDARY_LOCK_TIMEOUT) - .map_err(|error| anyhow!("Failed to acquire adapter boundary lock: {error}"))?; + let _boundary = state::BOUNDARY_LOCK.acquire(&state::adapter_state_dir(git_dir))?; operation() } diff --git a/cli/src/services/hooks/pi_mutation_scope/mod.rs b/cli/src/services/hooks/pi_mutation_scope/mod.rs index 017e6bcf4..cc35fafa2 100644 --- a/cli/src/services/hooks/pi_mutation_scope/mod.rs +++ b/cli/src/services/hooks/pi_mutation_scope/mod.rs @@ -1,10 +1,8 @@ #![allow(dead_code)] -mod boundary_lock; mod events; pub(crate) mod health; mod lifecycle; -mod os_lock; mod payload; pub(crate) mod state; diff --git a/cli/src/services/hooks/pi_mutation_scope/os_lock.rs b/cli/src/services/hooks/pi_mutation_scope/os_lock.rs deleted file mode 100644 index dc567c1b7..000000000 --- a/cli/src/services/hooks/pi_mutation_scope/os_lock.rs +++ /dev/null @@ -1,95 +0,0 @@ -use std::fs::{File, OpenOptions, TryLockError}; -use std::path::{Path, PathBuf}; -use std::time::{Duration, Instant}; - -use anyhow::Context; - -const LOCK_POLL_INTERVAL: Duration = Duration::from_millis(20); - -#[derive(Debug)] -pub(crate) enum AdvisoryLockError { - TimedOut { - path: PathBuf, - timeout: Duration, - what: &'static str, - }, - Io(anyhow::Error), -} - -impl std::fmt::Display for AdvisoryLockError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - AdvisoryLockError::TimedOut { - path, - timeout, - what, - } => write!( - f, - "Timed out after {timeout:?} waiting for the {what} lock '{}'", - path.display() - ), - AdvisoryLockError::Io(source) => write!(f, "{source}"), - } - } -} - -impl std::error::Error for AdvisoryLockError {} - -pub(crate) struct OsAdvisoryLock { - file: File, -} - -impl OsAdvisoryLock { - pub(crate) fn acquire( - parent_dir: &Path, - lock_path: PathBuf, - timeout: Duration, - what: &'static str, - ) -> Result { - std::fs::create_dir_all(parent_dir) - .with_context(|| { - format!( - "Failed to create {what} lock directory '{}'", - parent_dir.display() - ) - }) - .map_err(AdvisoryLockError::Io)?; - - let file = OpenOptions::new() - .write(true) - .create(true) - .truncate(false) - .open(&lock_path) - .with_context(|| format!("Failed to open {what} lock file '{}'", lock_path.display())) - .map_err(AdvisoryLockError::Io)?; - - let deadline = Instant::now() + timeout; - loop { - match file.try_lock() { - Ok(()) => return Ok(OsAdvisoryLock { file }), - Err(TryLockError::WouldBlock) => { - let now = Instant::now(); - if now >= deadline { - return Err(AdvisoryLockError::TimedOut { - path: lock_path, - timeout, - what, - }); - } - std::thread::sleep(LOCK_POLL_INTERVAL.min(deadline - now)); - } - Err(TryLockError::Error(source)) => { - return Err(AdvisoryLockError::Io(anyhow::Error::new(source).context( - format!("Failed to acquire {what} lock '{}'", lock_path.display()), - ))); - } - } - } - } -} - -impl Drop for OsAdvisoryLock { - fn drop(&mut self) { - let _ = self.file.unlock(); - } -} diff --git a/cli/src/services/hooks/pi_mutation_scope/state.rs b/cli/src/services/hooks/pi_mutation_scope/state.rs index 049786361..adde96d90 100644 --- a/cli/src/services/hooks/pi_mutation_scope/state.rs +++ b/cli/src/services/hooks/pi_mutation_scope/state.rs @@ -1,12 +1,12 @@ use std::fs::OpenOptions; use std::io::Write as _; use std::path::{Path, PathBuf}; -use std::time::Duration; use anyhow::{anyhow, Context, Result}; use serde::{Deserialize, Serialize}; -use super::os_lock::{AdvisoryLockError, OsAdvisoryLock}; +use crate::services::hooks::mutation_scope_lock::{AdapterLockSpec, OsAdvisoryLock}; + use super::{format_pi_scope_id, AttemptKey}; use crate::services::hooks::mutation_scope_owner::{ current_process_owner, is_definitely_dead, ProcessOwner, @@ -14,10 +14,11 @@ use crate::services::hooks::mutation_scope_owner::{ const SCE_STATE_DIR: &str = "sce"; const ADAPTER_STATE_FILE: &str = "pi-mutation-scope-state.json"; -const ADAPTER_STATE_LOCK_FILE: &str = "pi-mutation-scope-state.lock"; -const STATE_LOCK_WHAT: &str = "adapter-state"; -const DEFAULT_LOCK_TIMEOUT: Duration = Duration::from_secs(10); +pub(crate) const STATE_LOCK: AdapterLockSpec = + AdapterLockSpec::state("pi-mutation-scope-state.lock"); +pub(crate) const BOUNDARY_LOCK: AdapterLockSpec = + AdapterLockSpec::boundary("pi-mutation-scope-boundary.lock"); const ADAPTER_STATE_VERSION: u32 = 2; @@ -126,26 +127,6 @@ pub(crate) fn state_path(git_dir: &Path) -> PathBuf { adapter_state_dir(git_dir).join(ADAPTER_STATE_FILE) } -fn lock_path(git_dir: &Path) -> PathBuf { - adapter_state_dir(git_dir).join(ADAPTER_STATE_LOCK_FILE) -} - -struct AdapterStateLock { - _inner: OsAdvisoryLock, -} - -impl AdapterStateLock { - fn acquire(git_dir: &Path, timeout: Duration) -> Result { - let inner = OsAdvisoryLock::acquire( - &adapter_state_dir(git_dir), - lock_path(git_dir), - timeout, - STATE_LOCK_WHAT, - )?; - Ok(AdapterStateLock { _inner: inner }) - } -} - pub(crate) fn read_state(git_dir: &Path) -> Result { let path = state_path(git_dir); if !path.exists() { @@ -242,9 +223,8 @@ where Ok(()) } -fn acquire_lock(git_dir: &Path) -> Result { - AdapterStateLock::acquire(git_dir, DEFAULT_LOCK_TIMEOUT) - .map_err(|err| anyhow!("Failed to acquire adapter-state lock: {err}")) +fn acquire_lock(git_dir: &Path) -> Result { + STATE_LOCK.acquire(&adapter_state_dir(git_dir)) } fn allocate_pending_start( @@ -965,7 +945,9 @@ mod tests { let sce_dir = git_dir.join(SCE_STATE_DIR); assert!(state_path(&git_dir).starts_with(&sce_dir)); - assert!(lock_path(&git_dir).starts_with(&sce_dir)); + assert!(STATE_LOCK + .path(&adapter_state_dir(&git_dir)) + .starts_with(&sce_dir)); remove_test_git_dir(&git_dir); } diff --git a/context/cli/codex-mutation-scope-integration.md b/context/cli/codex-mutation-scope-integration.md index 96bb7fd60..98753bf23 100644 --- a/context/cli/codex-mutation-scope-integration.md +++ b/context/cli/codex-mutation-scope-integration.md @@ -142,7 +142,10 @@ Version-3 JSON stores `next_attempt_seq`, recovery generation, tool, lane turn, and `pending_start`/`active` phase. Writes stage, `sync_data`, rename, and sync the directory where supported. The boundary lock serializes state-to-ingress-to-state across hook processes (boundary lock then state lock); -file existence alone is never ownership. +file existence alone is never ownership. Both locks are `AdapterLockSpec` +identities (`state::STATE_LOCK`, `state::BOUNDARY_LOCK`) over the single +OS advisory-lock implementation in `cli/src/services/hooks/mutation_scope_lock.rs`, +shared with the OpenCode and Pi adapters. Positive cleanup arms `recovery_pending`, abandons matching tracked scopes via generic `abandon`, and removes settled attempts. Known attempts keep tracked diff --git a/context/cli/doctor-recovery-safety-model.md b/context/cli/doctor-recovery-safety-model.md index f68551503..9185e9684 100644 --- a/context/cli/doctor-recovery-safety-model.md +++ b/context/cli/doctor-recovery-safety-model.md @@ -25,7 +25,7 @@ each real adapter releases before every mutation-scope seam call. | `Health = Healthy \| Recovering \| Blocked \| Invalid` via `adapterHealth(phase, recovery)` | each adapter's own `classify_health`, computed from every attempt's phase plus the one shared recovery fact — never from a single attempt in isolation | | `FixState = NotAttempted \| RepairAttempted \| RepairCompleted \| ReportedFixed \| ReportedManual` (adapter-wide) | doctor's one fix-result lifecycle per adapter target (`assess_repairability` -> `repair_blocked` -> re-diagnose -> one `DoctorFixResultRecord`) | | `OwnerReading = SeenAlive \| SeenDead \| SeenUnknown` | `mutation_scope_owner::is_definitely_dead`'s `Alive`/`Dead`/`Unknown` result (PID + `/proc` start-time liveness, never time-based) | -| `stateTxn: StateTxnHolder` | OpenCode's `AdapterStateLock` transaction (and Claude's equivalent short state-file lock) around one re-read/re-prove/durable-transition step — **not** OpenCode's coarser `AdapterBoundaryLock`, which this model has no dedicated variable for (see below) | +| `stateTxn: StateTxnHolder` | OpenCode's adapter-state lock (`state::STATE_LOCK`) transaction (and Claude's equivalent short state-file lock) around one re-read/re-prove/durable-transition step — **not** OpenCode's coarser adapter-boundary lock (`state::BOUNDARY_LOCK`), which this model has no dedicated variable for (see below) | `RecoveryState`/`FixState` are single adapter-wide variables, not per-attempt: both real adapters report one health value and one fix result @@ -36,7 +36,7 @@ remains `PendingAbandon`, so a second obligation can join an in-flight recovery pipeline without disturbing it. **Scope boundary:** `stateTxn` verifies only the short per-transaction lock. -OpenCode's `AdapterBoundaryLock` — which serializes a repair's whole +OpenCode's adapter-boundary lock — which serializes a repair's whole lifecycle, seam calls included — is a separate, coarser concern outside this model's variables; T03 is responsible for acquiring it around the whole `repair_blocked` call, wrapping (not replacing) the `stateTxn`-shaped diff --git a/context/cli/opencode-mutation-scope-adapter-lifecycle.md b/context/cli/opencode-mutation-scope-adapter-lifecycle.md index 8fd03e27e..5c30c3ca3 100644 --- a/context/cli/opencode-mutation-scope-adapter-lifecycle.md +++ b/context/cli/opencode-mutation-scope-adapter-lifecycle.md @@ -132,8 +132,8 @@ it changes none of the ordinary hook lifecycle above and none of `mutation_scope_owner::is_definitely_dead` proves dead; otherwise `ManualOnly` (no owner recorded, a live owner, or unprovable liveness). - `repair_blocked(git_dir, repository_root, logger, seam)` acquires the - `AdapterBoundaryLock`, normalizes an orphaned `Flushing`, then — in one - `AdapterStateLock` transaction — re-reads state fresh and re-evaluates + adapter-boundary lock (`state::BOUNDARY_LOCK`), normalizes an orphaned `Flushing`, then — in one + adapter-state lock (`state::STATE_LOCK`) transaction — re-reads state fresh and re-evaluates liveness against the *current* state rather than trusting any earlier read. Only attempts still `PendingStart` with a still-dead owner at that moment transition to `PendingAbandon`, using the same durable diff --git a/context/context-map.md b/context/context-map.md index 3b65cc836..4287d2f3f 100644 --- a/context/context-map.md +++ b/context/context-map.md @@ -114,7 +114,7 @@ Additional mutation-scope integration context: - `context/cli/codex-mutation-scope-health.md` (the `doctor-mutation-scope-health` plan's T03: `classify_health` in `cli/src/services/hooks/codex_mutation_scope/health.rs` maps the adapter's `RecoveryState`/attempt-state combinations onto `healthy | recovering | blocked | invalid`, proven by driving the real dispatch/recovery functions rather than reading enum names — `Pending` with attempts empty, `Pending` with unresolved attempts, and an orphaned `Flushing` are all `recovering`: the production `PreToolUse` order runs `sweep_stale_lane_predecessors` before the global `RecoveryBlocked` gate, so a same-`(session_id, turn_id)`-lane successor can retry and clear a stuck attempt and self-admit through a flush, even though an unrelated session/lane still denies fail-closed in the meantime; `Flushing` with unresolved attempts and any read/parse failure are `invalid`) - `context/sce/codex-apply-patch-diff-runtime.md` (the complementary Codex `PostToolUse(apply_patch)` parsing, path containment, normalization, and `diff_traces` evidence contract) - `context/cli/opencode-mutation-scope-adapter-lifecycle.md` (the T04 OpenCode adapter runtime detail split out of the integration doc: attempt phases `PendingStart` → `Active` → `PendingAbandon`, per-`git-dir` boundary lock, durable `/sce/` state, write-ahead fail-closed `Start`, `Close` on successful `ToolExecuteAfter`, exact-`ToolError`-only `Abandon` with durable terminal intent + ambiguity-consuming `flush` + generation-tracked recovery barrier that retries transient cleanup failures, non-authoritative broad async events, no same-session sweep, no TTL, no protocol/Quint change) -- `context/cli/opencode-mutation-scope-integration.md` (the third concrete harness producer, reachable by a real OpenCode session as of the `opencode-mutation-scope-integration` plan's T05 and now proven end-to-end through real Git/DB production-path regressions as of T06: OpenCode tool lifecycle frozen (T01) against `opencode-ai@1.15.4` / `@opencode-ai/plugin@1.15.4` (upstream `v1.15.4`), with the probe fixtures/report under `cli/src/services/hooks/opencode_mutation_scope/fixtures/`; `(sessionID, callID)` scope identity, `bash`/`write`/`edit`/`apply_patch` tracked with the `gpt-`-model patch gate making `edit`/`write` vs `apply_patch` mutually exclusive per session, `task` delegation, MCP/plugin/unknown tools untracked by explicit allowlist; `bash` Start on `shell.env` (post-permission, pre-spawn), file-tool Start write-ahead on `tool.execute.before`, Close on successful `tool.execute.after` (fires for non-zero exit / 127 / timeout, not for rejection / interrupt / validation failure); OpenCode scopes confirmation-required like Codex (shipped in T02 as the generic `requires_boundary_confirmation` predicate); `chat.params` model provenance keyed by `sessionID` else `NULL`; sequential plugin dispatch in explicit-`plugin`-array order with fail-closed `tool.execute.before` / `shell.env` barriers (Probes A/B/C PROVEN); SIGINT/SIGKILL leave no terminal hook and orphan child processes so no TTL is safe; OpenCode persistence is global-user-scoped not checkout-local; T03 added `cli/src/services/hooks/opencode_mutation_scope/mod.rs` (strict wire-event parsing, `classify_tool`, `AttemptKey`, frozen `oc-tool-v1|s=:|c=:` `ScopeId` with no attempt-seq, `oc_` provenance) and the hidden `sce hooks opencode-mutation-scope` command; T04 added the full lifecycle (`state.rs` durable checkout-local attempt state under `/sce/` with attempt phases `PendingStart` → `Active` → `PendingAbandon`, `os_lock.rs`/`boundary_lock.rs`, generation-tracked recovery barrier, write-ahead fail-closed `Start`, `Close`, `Abandon` on exact `ToolError` `(session_id, call_id)` evidence only — broad async `SessionIdle`/`SessionError`/`SessionDeleted`/`ServerDisposed` events retire nothing (D10/D11); `ToolError` first persists the doomed attempt as `PendingAbandon` **and** the recovery generation in one write before any seam call (`begin_terminal_cleanup`), then `resolve_recovery` drives an ineligible `flush` (while doomed + sibling scopes still resolve it `IneligibleUnscoped`), then the generic `abandon`, then removes the attempt only on `abandon` success, then a second `flush` for the rebaseline; a transient failure of any step relinquishes recovery to `Pending` and leaves the attempt `PendingAbandon` so the next recovery-capable boundary (a new tracked `Start` claims the flush; a duplicate `ToolError` retries) replays the whole idempotent sequence rather than forgetting the scope; a replayed `Start` for a `PendingAbandon` identity fails closed and never returns it to `Active`; so a concurrent `Abandon(A)`→`Close(B)` can never yield `AiExclusive(B)` over A's interval while B keeps its later intervals; the ambiguity flush runs alongside live siblings, not deferred to `attempts.is_empty()`; `ToolError` carries `tool_name` so untracked/delegation errors are zero-footprint; no same-session sweep, no TTL; no protocol/Quint change — the generic `flush`/`abandon`/confirmation-required semantics already suffice; the full T04 recovery detail now lives in `context/cli/opencode-mutation-scope-adapter-lifecycle.md`) driving the generic in-process ingress seam; T05 added the generated `sce-mutation-scope.ts` transport plugin (`config/lib/mutation-scope-plugin/`, emitted by `config/pkl/generate.pkl`, registered last in `config/pkl/renderers/common.pkl`), installed as the final OpenCode plugin by the `config_merge` append after arbitrary user plugins, with `sce doctor`'s `inspect_opencode_plugin_ordering_health` flagging a non-last position; the plugin maps `write`/`edit`/`apply_patch` `tool.execute.before` → fail-closed `ToolExecuteBefore`, `shell.env` → fail-closed `ShellEnv` bash Start, `tool.execute.after` → best-effort Close, tool-part `error` → best-effort `ToolError`, observes the model per turn from `chat.params` (`providerID/api.id`, ignoring the `title` agent, replacing rather than merging the cached per-session model so a later turn with no valid model clears it instead of reusing stale evidence), throws on any failure to establish a tracked Start (non-zero adapter exit, spawn failure, timeout, or missing `sce` CLI) so every transport failure fails closed; the broad async `SessionIdle`/`SessionError`/`ServerDisposed` events are not forwarded at all (T04's dispatch is a no-op for them) and `session.deleted` only clears the plugin's local model cache — full plugin-transport/model-provenance detail split into [`opencode-mutation-scope-plugin-transport.md`](cli/opencode-mutation-scope-plugin-transport.md); T06 added real Git/DB production-path regressions in `cli/src/services/hooks/mod.rs` (`services::hooks::tests::mutation_provenance_e2e`) proving tracked-tool success, model-present/model-missing provenance, task/unknown-tool zero-footprint, concurrent reject-and-confirm, and OpenCode+Codex/Claude overlap down to `mutation_ai_patch` and Agent Trace output; a live `apply_patch` fixture against a real credentialed OpenCode CLI session remains outstanding for `/validate` (a credential gap, not a soundness gap)) +- `context/cli/opencode-mutation-scope-integration.md` (the third concrete harness producer, reachable by a real OpenCode session as of the `opencode-mutation-scope-integration` plan's T05 and now proven end-to-end through real Git/DB production-path regressions as of T06: OpenCode tool lifecycle frozen (T01) against `opencode-ai@1.15.4` / `@opencode-ai/plugin@1.15.4` (upstream `v1.15.4`), with the probe fixtures/report under `cli/src/services/hooks/opencode_mutation_scope/fixtures/`; `(sessionID, callID)` scope identity, `bash`/`write`/`edit`/`apply_patch` tracked with the `gpt-`-model patch gate making `edit`/`write` vs `apply_patch` mutually exclusive per session, `task` delegation, MCP/plugin/unknown tools untracked by explicit allowlist; `bash` Start on `shell.env` (post-permission, pre-spawn), file-tool Start write-ahead on `tool.execute.before`, Close on successful `tool.execute.after` (fires for non-zero exit / 127 / timeout, not for rejection / interrupt / validation failure); OpenCode scopes confirmation-required like Codex (shipped in T02 as the generic `requires_boundary_confirmation` predicate); `chat.params` model provenance keyed by `sessionID` else `NULL`; sequential plugin dispatch in explicit-`plugin`-array order with fail-closed `tool.execute.before` / `shell.env` barriers (Probes A/B/C PROVEN); SIGINT/SIGKILL leave no terminal hook and orphan child processes so no TTL is safe; OpenCode persistence is global-user-scoped not checkout-local; T03 added `cli/src/services/hooks/opencode_mutation_scope/mod.rs` (strict wire-event parsing, `classify_tool`, `AttemptKey`, frozen `oc-tool-v1|s=:|c=:` `ScopeId` with no attempt-seq, `oc_` provenance) and the hidden `sce hooks opencode-mutation-scope` command; T04 added the full lifecycle (`state.rs` durable checkout-local attempt state under `/sce/` with attempt phases `PendingStart` → `Active` → `PendingAbandon`, adapter state/boundary locks (now `AdapterLockSpec` identities over the shared `hooks/mutation_scope_lock.rs` OS advisory lock), generation-tracked recovery barrier, write-ahead fail-closed `Start`, `Close`, `Abandon` on exact `ToolError` `(session_id, call_id)` evidence only — broad async `SessionIdle`/`SessionError`/`SessionDeleted`/`ServerDisposed` events retire nothing (D10/D11); `ToolError` first persists the doomed attempt as `PendingAbandon` **and** the recovery generation in one write before any seam call (`begin_terminal_cleanup`), then `resolve_recovery` drives an ineligible `flush` (while doomed + sibling scopes still resolve it `IneligibleUnscoped`), then the generic `abandon`, then removes the attempt only on `abandon` success, then a second `flush` for the rebaseline; a transient failure of any step relinquishes recovery to `Pending` and leaves the attempt `PendingAbandon` so the next recovery-capable boundary (a new tracked `Start` claims the flush; a duplicate `ToolError` retries) replays the whole idempotent sequence rather than forgetting the scope; a replayed `Start` for a `PendingAbandon` identity fails closed and never returns it to `Active`; so a concurrent `Abandon(A)`→`Close(B)` can never yield `AiExclusive(B)` over A's interval while B keeps its later intervals; the ambiguity flush runs alongside live siblings, not deferred to `attempts.is_empty()`; `ToolError` carries `tool_name` so untracked/delegation errors are zero-footprint; no same-session sweep, no TTL; no protocol/Quint change — the generic `flush`/`abandon`/confirmation-required semantics already suffice; the full T04 recovery detail now lives in `context/cli/opencode-mutation-scope-adapter-lifecycle.md`) driving the generic in-process ingress seam; T05 added the generated `sce-mutation-scope.ts` transport plugin (`config/lib/mutation-scope-plugin/`, emitted by `config/pkl/generate.pkl`, registered last in `config/pkl/renderers/common.pkl`), installed as the final OpenCode plugin by the `config_merge` append after arbitrary user plugins, with `sce doctor`'s `inspect_opencode_plugin_ordering_health` flagging a non-last position; the plugin maps `write`/`edit`/`apply_patch` `tool.execute.before` → fail-closed `ToolExecuteBefore`, `shell.env` → fail-closed `ShellEnv` bash Start, `tool.execute.after` → best-effort Close, tool-part `error` → best-effort `ToolError`, observes the model per turn from `chat.params` (`providerID/api.id`, ignoring the `title` agent, replacing rather than merging the cached per-session model so a later turn with no valid model clears it instead of reusing stale evidence), throws on any failure to establish a tracked Start (non-zero adapter exit, spawn failure, timeout, or missing `sce` CLI) so every transport failure fails closed; the broad async `SessionIdle`/`SessionError`/`ServerDisposed` events are not forwarded at all (T04's dispatch is a no-op for them) and `session.deleted` only clears the plugin's local model cache — full plugin-transport/model-provenance detail split into [`opencode-mutation-scope-plugin-transport.md`](cli/opencode-mutation-scope-plugin-transport.md); T06 added real Git/DB production-path regressions in `cli/src/services/hooks/mod.rs` (`services::hooks::tests::mutation_provenance_e2e`) proving tracked-tool success, model-present/model-missing provenance, task/unknown-tool zero-footprint, concurrent reject-and-confirm, and OpenCode+Codex/Claude overlap down to `mutation_ai_patch` and Agent Trace output; a live `apply_patch` fixture against a real credentialed OpenCode CLI session remains outstanding for `/validate` (a credential gap, not a soundness gap)) - `context/cli/opencode-mutation-scope-plugin-transport.md` (the T05 generated `sce-mutation-scope.ts` plugin's model-provenance observation from per-session `chat.params` and its sequential, last-registered position in OpenCode's plugin ordering — detail split out of `opencode-mutation-scope-integration.md` for the repository's per-file line budget) - `context/cli/opencode-mutation-scope-health.md` (the `doctor-mutation-scope-health` plan's T04: `classify_health` in `cli/src/services/hooks/opencode_mutation_scope/health.rs` maps the adapter's `RecoveryState`/`AttemptPhase` combinations onto `healthy | recovering | blocked | invalid`, proven by driving the real dispatch/recovery functions — `Clear` + `PendingAbandon` is `invalid`; any state containing a `PendingStart` attempt is `blocked` (except that `Clear` + `PendingAbandon` retains `invalid` precedence), because no boundary sweeps a stale `PendingStart` on behalf of an unrelated call, unlike Pi's dead-owner sweep or Codex's same-lane retry, so a crashed owning process leaves it a permanent wedge for new admissions even while a `Pending`/`Flushing` recovery for another attempt is also outstanding, since `resolve_recovery` never retires `PendingStart`; `Pending`/`Flushing` with no `PendingStart` outstanding is `recovering`; `Clear` with ordinary `Active`/no attempts is `healthy`; any read/parse failure is `invalid`) - `context/cli/pi-mutation-scope-integration.md` (the fourth concrete harness adapter, `cli/src/services/hooks/pi_mutation_scope/`, hidden command `sce hooks pi-mutation-scope`, driven by the canonical generated extension in ordinary Pi sessions: Pi's `bash`/`edit`/`write` tracked-tool allowlist with `tool_call` as the single universal pre-execution gate for every tool including `bash`; the `pi-tool-v1|n=|s=:|c=:` `ScopeId` whose checkout-local monotonic attempt sequence stops a reused `toolCallId` from ever reactivating a terminal scope; the `PendingStart`→`Executed`→`Closed`/`PendingAbandon` attempt-phase machine with no `Active` phase, because `PendingStart` is Pi's normal resting state for an entire in-flight execution (`tool_execution_start` fires unconditionally before the fail-closed `tool_call` gate and proves nothing; `tool_result` is the sole `Executed` evidence, and `tool_execution_end` with no preceding `tool_result` abandons rather than closes); why admission's fail-closed "uncertain attempt" check covers only a lingering `PendingAbandon` or non-`Clear` recovery state and never a sibling's `PendingStart`, unlike the other three adapters, so concurrent Pi tool calls stay distinct live scopes; a positive-process-death-only stale-`PendingStart`/`Executed` recovery (`getppid()`-captured owner plus Linux `/proc`-start-time PID-reuse proofing, no TTL/sweep), run on every tracked Start admission by independently inspecting persisted attempts rather than only ones matching the incoming key, and adapter reconciliation with the external-mutation guard's forced recovery, both added by a later task in this same plan; and the harness-neutral `external-mutation-guard` mechanism this task also built for Pi's `user_bash`, detailed separately in `context/cli/mutation-trace-external-mutation-guard.md`) From 11327815f6932a82781e90da57dc633b7de24b8c Mon Sep 17 00:00:00 2001 From: David Abram Date: Thu, 1 Oct 2026 01:44:53 +0200 Subject: [PATCH 2/2] tests: Remove obsolete mutation-scope lock assertions Update mutation-scope tests to match the current state layout and lock behavior by removing stale lock-path checks and the obsolete leftover-lock scenario. --- .../hooks/codex_mutation_scope/state.rs | 25 ------------------- .../hooks/opencode_mutation_scope/state.rs | 3 --- .../services/hooks/pi_mutation_scope/state.rs | 3 --- 3 files changed, 31 deletions(-) diff --git a/cli/src/services/hooks/codex_mutation_scope/state.rs b/cli/src/services/hooks/codex_mutation_scope/state.rs index 833be77dd..8d81a8e0a 100644 --- a/cli/src/services/hooks/codex_mutation_scope/state.rs +++ b/cli/src/services/hooks/codex_mutation_scope/state.rs @@ -1275,28 +1275,6 @@ mod tests { remove_test_git_dir(&git_dir); } - #[test] - fn a_leftover_lock_file_with_no_active_os_lock_does_not_block_a_new_acquirer() { - let git_dir = unique_test_git_dir("leftover-lock-file"); - let dir = adapter_state_dir(&git_dir); - std::fs::create_dir_all(&dir).expect("state dir should be created"); - std::fs::write(STATE_LOCK.path(&dir), b"leftover") - .expect("leftover lock file should be writable"); - - let decision = admit_tracked_attempt( - &git_dir, - &key("session-1", None, "exec-1"), - "turn-1", - "Bash", - ); - assert!( - matches!(decision, Ok(AdmitDecision::Admitted(_))), - "a lock file with no active OS lock held against it must not block a new acquirer" - ); - - remove_test_git_dir(&git_dir); - } - const PARALLEL_ADMISSION_COUNT: u64 = 6; #[test] @@ -1392,9 +1370,6 @@ mod tests { let sce_dir = git_dir.join(SCE_STATE_DIR); assert!(state_path(&git_dir).starts_with(&sce_dir)); - assert!(STATE_LOCK - .path(&adapter_state_dir(&git_dir)) - .starts_with(&sce_dir)); let mut found_state_file = false; for entry in std::fs::read_dir(&sce_dir).expect("sce dir should be readable") { diff --git a/cli/src/services/hooks/opencode_mutation_scope/state.rs b/cli/src/services/hooks/opencode_mutation_scope/state.rs index 61011eadb..fc9bbab6e 100644 --- a/cli/src/services/hooks/opencode_mutation_scope/state.rs +++ b/cli/src/services/hooks/opencode_mutation_scope/state.rs @@ -1117,9 +1117,6 @@ mod tests { let sce_dir = git_dir.join(SCE_STATE_DIR); assert!(state_path(&git_dir).starts_with(&sce_dir)); - assert!(STATE_LOCK - .path(&adapter_state_dir(&git_dir)) - .starts_with(&sce_dir)); remove_test_git_dir(&git_dir); } diff --git a/cli/src/services/hooks/pi_mutation_scope/state.rs b/cli/src/services/hooks/pi_mutation_scope/state.rs index adde96d90..a64c37aff 100644 --- a/cli/src/services/hooks/pi_mutation_scope/state.rs +++ b/cli/src/services/hooks/pi_mutation_scope/state.rs @@ -945,9 +945,6 @@ mod tests { let sce_dir = git_dir.join(SCE_STATE_DIR); assert!(state_path(&git_dir).starts_with(&sce_dir)); - assert!(STATE_LOCK - .path(&adapter_state_dir(&git_dir)) - .starts_with(&sce_dir)); remove_test_git_dir(&git_dir); }