From 497497621bcc66f52e7e2232e101be378149b2e9 Mon Sep 17 00:00:00 2001 From: John Morrissey <544926+tachyon-beep@users.noreply.github.com> Date: Sat, 23 May 2026 10:54:11 +1000 Subject: [PATCH] fix(storage): fs2 advisory lock against concurrent analyze (STO-01) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes gap-register STO-01 (Critical, deep-dive-db's highest-priority finding): a second concurrent `clarion analyze` against the same project root could interleave with the first writer-actor's open transaction, flip another run's status row, and corrupt the per-run accounting. The ADR-011 single-writer-actor invariant was enforced inside a process but had no cross-process guard. Implementation: - Add `fs2 = "0.4"` to the workspace and `clarion-cli` crate (the surface we use — `FileExt::try_lock_exclusive` / `unlock` — has been stable since 0.4; crate is lightly maintained but the dependency footprint is minimal). - New module `crates/clarion-cli/src/project_lock.rs`. Exports `acquire_project_lock(project_root) -> Result`. The guard holds an open file descriptor on `.clarion/clarion.lock`; POSIX `flock` is released when the fd closes (struct Drop). Non-blocking acquisition; on `WouldBlock` the operator-facing error is "another clarion analyze or serve is in progress against this project (lockfile: …)". - Wire into `analyze::run_with_options` immediately after the `.clarion/` existence check and before `Writer::spawn`. The guard is bound to a local that lives for the writer-actor's whole lifetime. - Wire into `serve::run_mcp_stdio` only inside the `if let Some(provider) = llm_provider` branch — read-only serve invocations intentionally do NOT take the lock (HTTP read API and MCP read-only tools are allowed to run concurrently with each other and with an analyze; only writer- mode operations contend). Tests: - Module unit tests cover the contract surface: clean acquire, contention error, drop-releases, missing-`.clarion/` diagnostic. - `tests/project_lock.rs::analyze_refuses_when_lock_already_held` is the authoritative contract test: the test process holds the lock, spawns a `clarion analyze` subprocess, asserts non-zero exit + the expected stderr message + lockfile path, then releases and asserts a fresh analyze succeeds. - `concurrent_analyze_processes_exactly_one_wins` exercises the race against two real subprocesses; retries up to 8 times to find a contended schedule (the fixture-plugin analyze is fast enough that serialised completion is also a possible outcome). Asserts no non-lock failure modes. ADR-035-aligned annotations: every new constant (`PROJECT_LOCK_FILENAME`, `PROJECT_STATE_SUBDIR`) carries the four-field block (stated basis / override surface / retune trigger / coupling). ADR-035 itself is in-flight at v1.0 tag-cut; the discipline already appears in `clarion-storage::pragma` and in this commit it gates the new lockfile constant. Note on `recover_preexisting_running_runs`: the gap-register text references `run_lifecycle.rs:19-25` for an unconditional `UPDATE runs SET status='failed' WHERE status='running'` recovery sweep. That code path does not exist in the tree today — the closest is the per-actor `cleanup_after_channel_close` in `clarion-storage::writer`, which is already local to the calling process. STO-05 schedules the schema-additive PID/heartbeat columns for v1.1; no recovery-sweep change was needed here. Drive-by: rustfmt was not clean on `crates/clarion-cli/src/http_read.rs` at the branch tip; included the formatter's whitespace fixes so the `cargo fmt --all -- --check` gate passes on this branch. (Branch `v1.0/07-storage-identity-integrity` independently rolled the same fix into its STO-02 commit.) Verification (all green): - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets --all-features -- -D warnings` - `cargo build --workspace --bins` - `cargo nextest run --workspace --all-features` (511 tests; 6 new) - `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --all-features` - `cargo deny check` - `bash tests/e2e/sprint_1_walking_skeleton.sh` Refs: gap-register STO-01; filigree clarion-c0caf61ab6; ADR-011 (single-writer-actor); ADR-035 (operational-tuning discipline, in-flight at v1.0 tag-cut). Co-Authored-By: Claude Opus 4.7 (1M context) --- Cargo.lock | 33 +++ Cargo.toml | 7 + crates/clarion-cli/Cargo.toml | 2 + crates/clarion-cli/src/analyze.rs | 13 ++ crates/clarion-cli/src/http_read.rs | 17 +- crates/clarion-cli/src/main.rs | 1 + crates/clarion-cli/src/project_lock.rs | 266 +++++++++++++++++++++ crates/clarion-cli/src/serve.rs | 21 ++ crates/clarion-cli/tests/project_lock.rs | 283 +++++++++++++++++++++++ 9 files changed, 635 insertions(+), 8 deletions(-) create mode 100644 crates/clarion-cli/src/project_lock.rs create mode 100644 crates/clarion-cli/tests/project_lock.rs diff --git a/Cargo.lock b/Cargo.lock index dbc817e1..e21c81f2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -325,6 +325,7 @@ dependencies = [ "clarion-scanner", "clarion-storage", "dotenvy", + "fs2", "ignore", "rusqlite", "serde", @@ -648,6 +649,16 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fs2" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9564fc758e15025b46aa6643b1b77d047d1a56a1aea6e01002ac0c7026876213" +dependencies = [ + "libc", + "winapi", +] + [[package]] name = "futures-channel" version = "0.3.32" @@ -2457,6 +2468,22 @@ dependencies = [ "winsafe", ] +[[package]] +name = "winapi" +version = "0.3.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419" +dependencies = [ + "winapi-i686-pc-windows-gnu", + "winapi-x86_64-pc-windows-gnu", +] + +[[package]] +name = "winapi-i686-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" + [[package]] name = "winapi-util" version = "0.1.11" @@ -2466,6 +2493,12 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "winapi-x86_64-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" + [[package]] name = "windows-link" version = "0.2.1" diff --git a/Cargo.toml b/Cargo.toml index d4b7eaa5..6fff9e0e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,3 +60,10 @@ xgraph = "2.0.0" assert_cmd = "2" tempfile = "3" nix = { version = "0.28", default-features = false, features = ["resource"] } +# fs2 provides a portable advisory-lock wrapper around POSIX flock(2) +# (Linux/macOS) and Windows LockFileEx. Used by the cross-process +# project-lock (STO-01, gap-register entry; ADR-035 declaration discipline +# for the new lockfile constant). The 0.4 series has been API-stable for +# years; the crate is lightly maintained upstream but the surface we use +# (`FileExt::try_lock_exclusive` / `unlock`) is settled. +fs2 = "0.4" diff --git a/crates/clarion-cli/Cargo.toml b/crates/clarion-cli/Cargo.toml index 813c8e88..62b93d70 100644 --- a/crates/clarion-cli/Cargo.toml +++ b/crates/clarion-cli/Cargo.toml @@ -23,6 +23,7 @@ clarion-mcp = { path = "../clarion-mcp", version = "1.0.0" } clarion-scanner = { path = "../clarion-scanner", version = "1.0.0" } clarion-storage = { path = "../clarion-storage", version = "1.0.0" } dotenvy.workspace = true +fs2.workspace = true ignore.workspace = true rusqlite.workspace = true serde.workspace = true @@ -41,6 +42,7 @@ xgraph.workspace = true [dev-dependencies] assert_cmd.workspace = true clarion-plugin-fixture = { path = "../clarion-plugin-fixture", version = "1.0.0" } +fs2.workspace = true rusqlite.workspace = true serde_json.workspace = true sha1.workspace = true diff --git a/crates/clarion-cli/src/analyze.rs b/crates/clarion-cli/src/analyze.rs index 5656c791..e6562d32 100644 --- a/crates/clarion-cli/src/analyze.rs +++ b/crates/clarion-cli/src/analyze.rs @@ -84,6 +84,19 @@ pub(crate) async fn run_with_options(project_path: PathBuf, options: AnalyzeOpti let analyze_config = AnalyzeConfig::load(&project_root, options.config_path.as_deref())?; let analyze_config_json = analyze_config.to_json_string()?; + // ── Cross-process writer lock (gap-register STO-01) ─────────────────────── + // Acquire the project-wide advisory lock BEFORE spawning the writer-actor + // so two concurrent `clarion analyze` invocations against the same + // project root cannot interleave their write transactions. The guard + // must outlive `handle.await` at every exit path; binding to a local + // here lets it drop with this function's stack frame regardless of + // which `return Ok(())` / `bail!` path the run takes. + // + // The lock is taken AFTER the `.clarion/` directory existence check + // above so the "no install" error message remains the user-facing + // diagnostic for that case, rather than a lockfile-creation failure. + let _project_lock = crate::project_lock::acquire_project_lock(&project_root)?; + // ── Writer actor ────────────────────────────────────────────────────────── let (writer, handle) = Writer::spawn( db_path.clone(), diff --git a/crates/clarion-cli/src/http_read.rs b/crates/clarion-cli/src/http_read.rs index 8b2af458..8d8e855b 100644 --- a/crates/clarion-cli/src/http_read.rs +++ b/crates/clarion-cli/src/http_read.rs @@ -196,9 +196,8 @@ where // local request because both auth knobs are unset and the bind is // loopback. On a shared developer host or CI runner this means any // local process can read the (non-blocked) catalogue. - let warn_unauthenticated_loopback = config.is_loopback_bind() - && auth_token.is_none() - && identity_secret.is_none(); + let warn_unauthenticated_loopback = + config.is_loopback_bind() && auth_token.is_none() && identity_secret.is_none(); let (shutdown_tx, shutdown_rx) = oneshot::channel(); let (ready_tx, ready_rx) = std::sync::mpsc::channel::>(); let (failure_tx, failure_rx) = mpsc::channel(); @@ -1571,7 +1570,10 @@ mod tests { impl io::Write for CaptureWriter { fn write(&mut self, buf: &[u8]) -> io::Result { - self.buffer.lock().expect("capture lock").extend_from_slice(buf); + self.buffer + .lock() + .expect("capture lock") + .extend_from_slice(buf); Ok(buf.len()) } fn flush(&mut self) -> io::Result<()> { @@ -1616,10 +1618,9 @@ mod tests { token_env: "CLARION_LOOPBACK_NO_TOKEN_TEST_UNSET".to_owned(), identity_token_env: None, }; - let instance_id = crate::instance::parse_instance_id_for_test( - "00000000-0000-4000-8000-000000000002", - ) - .expect("parse synthetic instance id"); + let instance_id = + crate::instance::parse_instance_id_for_test("00000000-0000-4000-8000-000000000002") + .expect("parse synthetic instance id"); // Env lookup that returns None for every variable — emulates // the operator running `clarion serve` on loopback with no diff --git a/crates/clarion-cli/src/main.rs b/crates/clarion-cli/src/main.rs index 1f5999ec..65f0318d 100644 --- a/crates/clarion-cli/src/main.rs +++ b/crates/clarion-cli/src/main.rs @@ -5,6 +5,7 @@ mod config; mod http_read; mod install; mod instance; +mod project_lock; mod run_lifecycle; mod secret_scan; mod serve; diff --git a/crates/clarion-cli/src/project_lock.rs b/crates/clarion-cli/src/project_lock.rs new file mode 100644 index 00000000..ee54b336 --- /dev/null +++ b/crates/clarion-cli/src/project_lock.rs @@ -0,0 +1,266 @@ +//! Cross-process advisory lock for the writer-actor (gap-register STO-01). +//! +//! ## Why this exists +//! +//! ADR-011 names the writer-actor as the **sole** `SQLite` write connection for +//! a Clarion project. The actor model serialises writes inside a single +//! process, but it does nothing to prevent a *second* Clarion process from +//! opening its own writer-actor against the same `.clarion/clarion.db`. +//! A second concurrent `clarion analyze` can interleave with the first +//! actor's open transaction, flip another run's `status='running'` row, or +//! contend on the WAL in ways the per-run accounting never anticipated +//! (`status` flips, orphaned `runs` rows, partial-batch entity rows). +//! +//! The fix is a file-system advisory lock acquired before the writer-actor +//! is spawned. Any second writer-mode invocation against the same project +//! root fails fast with a clear error rather than corrupting state. +//! +//! ## Scope +//! +//! - **`clarion analyze`**: must hold the lock. The whole analyze pipeline +//! is a writer-mode operation. +//! - **`clarion serve`**: must hold the lock **only** when it spawns the +//! optional MCP LLM summary-cache writer. A serve invocation without an +//! LLM provider is read-only and is intentionally permitted to run +//! concurrently with another serve (or with an analyze — but see below; +//! the reader pool is unaffected by the writer lock). +//! - **Reader pools** (HTTP read API, MCP read-only tools): not gated by +//! this lock. `SQLite` WAL allows readers concurrent with a writer; the +//! single-writer constraint is the only thing this lock enforces. +//! +//! ## Mechanism +//! +//! [`acquire_project_lock`] opens `.clarion/clarion.lock` and asks +//! `fs2::FileExt::try_lock_exclusive` for an exclusive non-blocking lock. +//! On POSIX this is a per-fd `flock(2)`; on Windows it is `LockFileEx` +//! with `LOCKFILE_FAIL_IMMEDIATELY`. The lock is released when the +//! returned [`ProjectLock`] is dropped (POSIX semantics: closing the +//! file descriptor releases the flock). +//! +//! Callers must bind the returned guard to a local that lives at least as +//! long as the writer-actor. A `let _ = acquire_project_lock(...)?;` is a +//! bug (the temporary is dropped at the end of the statement); `let _lock +//! = acquire_project_lock(...)?;` is correct. + +use std::fs::{File, OpenOptions}; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; +use fs2::FileExt; + +/// Filename appended to `/.clarion/` for the project advisory lock. +/// +/// **Stated basis**: ADR-011 declares a single writer-actor per project DB. +/// STO-01 in `docs/implementation/v1.0-tag-cut/gap-register.md` identifies +/// the missing cross-process enforcement of that invariant. Co-locating +/// the lockfile under `.clarion/` keeps the on-disk surface uniform: all +/// project state — `clarion.db`, `instance.json`, the lock — lives in a +/// single operator-visible directory. +/// +/// **Override surface**: not configurable. The lockfile is an +/// implementation detail of the writer-actor invariant, not an operator +/// tuning knob. Operators who need to inspect lock ownership use `lsof` +/// or `fuser` against the path. +/// +/// **Retune trigger**: change only if `.clarion/` itself moves (which +/// would be a wider ADR-011 / project-layout decision). Renaming the +/// lockfile alone has no operator benefit. +/// +/// **Coupling**: `clarion-cli/src/analyze.rs` and the writer-spawning +/// branch of `clarion-cli/src/serve.rs` both acquire this lock before +/// `Writer::spawn`. A renamed file path would let a stale CLI (still +/// looking at the old path) coexist with a newer one — defeating the +/// lock. See `docs/clarion/adr/ADR-035-operational-tuning-discipline.md` +/// (in-flight at v1.0 tag-cut) for the declaration-discipline +/// requirement that produced this annotation block. +pub(crate) const PROJECT_LOCK_FILENAME: &str = "clarion.lock"; + +/// Subdirectory under the project root where Clarion writes its state. +/// +/// **Stated basis**: REQ-CORE-04 / ADR-011 — Clarion state is local to a +/// project under a single dotted directory. +/// +/// **Override surface**: not configurable; `clarion install` is the +/// authoritative creator of this directory. +/// +/// **Retune trigger**: would require an ADR change to the project +/// on-disk layout. +/// +/// **Coupling**: `analyze.rs` and `serve.rs` both derive the database +/// path from `/.clarion/clarion.db`. This constant exists +/// solely so the lockfile path stays in lockstep. +const PROJECT_STATE_SUBDIR: &str = ".clarion"; + +/// RAII guard for the project-wide writer advisory lock. +/// +/// The lock is released when this struct is dropped — either when the +/// guard goes out of scope (normal completion) or when the stack unwinds +/// past it (panic). +/// +/// Bind to a named local (`let _lock = acquire_project_lock(...)?;`) +/// rather than `let _ = ...` so the guard lives for the writer-actor's +/// lifetime instead of being dropped at the end of the let-statement. +#[derive(Debug)] +pub(crate) struct ProjectLock { + /// The held file handle. Closing the handle releases the POSIX + /// `flock`. We retain the handle in the struct rather than calling + /// `fs2::FileExt::unlock` explicitly so a panic between acquisition + /// and a hypothetical explicit unlock still releases the lock. + _file: File, + /// The lock-file path, kept for diagnostic logging in the Drop impl. + path: PathBuf, +} + +impl Drop for ProjectLock { + fn drop(&mut self) { + // We intentionally do not call `FileExt::unlock` here. POSIX + // releases the lock when the last fd referencing the inode is + // closed; the `File` field is closed as part of normal struct + // drop. Calling `unlock` explicitly would be redundant and would + // surface an additional I/O error path we cannot meaningfully + // recover from in `Drop`. + tracing::trace!( + lockfile = %self.path.display(), + "released Clarion project lock" + ); + } +} + +/// Acquire the exclusive project-writer lock at `/.clarion/clarion.lock`. +/// +/// The caller owns the returned [`ProjectLock`]; dropping it releases the +/// lock. The lock must be acquired **before** spawning the writer-actor +/// and held until after the writer's `JoinHandle` resolves. +/// +/// # Errors +/// +/// - The `.clarion/` directory does not exist (caller should have run +/// `clarion install` first). Returned as `anyhow::Error` with the +/// resolved path for the operator. +/// - The lock file cannot be created or opened (filesystem permission +/// error, full disk, etc.). +/// - Another `clarion` process already holds the lock. The error +/// message is operator-facing and names both the project root and the +/// lockfile path. +pub(crate) fn acquire_project_lock(project_root: &Path) -> Result { + let clarion_dir = project_root.join(PROJECT_STATE_SUBDIR); + if !clarion_dir.is_dir() { + anyhow::bail!( + "{} has no {PROJECT_STATE_SUBDIR}/ directory. Run `clarion install` first.", + project_root.display() + ); + } + let lock_path = clarion_dir.join(PROJECT_LOCK_FILENAME); + let file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(&lock_path) + .with_context(|| format!("open Clarion project lockfile {}", lock_path.display()))?; + + match FileExt::try_lock_exclusive(&file) { + Ok(()) => { + tracing::debug!( + lockfile = %lock_path.display(), + "acquired Clarion project lock" + ); + Ok(ProjectLock { + _file: file, + path: lock_path, + }) + } + Err(err) => { + // fs2 surfaces `ErrorKind::WouldBlock` for "already locked" + // and other variants for genuine I/O failures. We treat + // *every* failure as "another process holds the lock" only + // when the kernel says so; otherwise we surface the I/O + // error verbatim. + if err.kind() == std::io::ErrorKind::WouldBlock { + anyhow::bail!( + "another clarion analyze or serve is in progress against this \ + project (lockfile: {})", + lock_path.display() + ); + } + Err(anyhow::Error::from(err) + .context(format!("acquire exclusive lock on {}", lock_path.display()))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use tempfile::TempDir; + + fn make_project(tmp: &TempDir) -> PathBuf { + let root = tmp.path().to_path_buf(); + fs::create_dir_all(root.join(PROJECT_STATE_SUBDIR)).expect("create .clarion dir"); + root + } + + #[test] + fn acquire_succeeds_when_no_other_holder() { + let tmp = TempDir::new().expect("tempdir"); + let root = make_project(&tmp); + let lock = acquire_project_lock(&root).expect("first acquisition"); + // Lock-file exists after acquisition. + assert!( + root.join(PROJECT_STATE_SUBDIR) + .join(PROJECT_LOCK_FILENAME) + .exists() + ); + drop(lock); + } + + #[test] + fn second_acquisition_in_same_process_fails_fast() { + // Note: POSIX flock semantics allow the same process to upgrade + // its own lock, but fs2 uses `flock(LOCK_EX | LOCK_NB)` and we + // open a fresh `File` per call — distinct fds in the same + // process still contend. (This is the intended semantics; if it + // ever stopped holding we'd want to know.) + let tmp = TempDir::new().expect("tempdir"); + let root = make_project(&tmp); + let _first = acquire_project_lock(&root).expect("first acquisition"); + let err = acquire_project_lock(&root).expect_err("second must fail"); + let msg = format!("{err:#}"); + assert!( + msg.contains("another clarion analyze or serve is in progress"), + "unexpected error message: {msg}" + ); + assert!( + msg.contains(PROJECT_LOCK_FILENAME), + "error message must name the lockfile: {msg}" + ); + } + + #[test] + fn lock_released_on_drop() { + let tmp = TempDir::new().expect("tempdir"); + let root = make_project(&tmp); + { + let _first = acquire_project_lock(&root).expect("first acquisition"); + } + // After the guard drops, a fresh acquisition succeeds. + let _second = acquire_project_lock(&root).expect("re-acquire after drop"); + } + + #[test] + fn missing_clarion_dir_is_a_clear_error() { + let tmp = TempDir::new().expect("tempdir"); + // Note: we do NOT create `.clarion/`. + let err = acquire_project_lock(tmp.path()).expect_err("should fail"); + let msg = format!("{err:#}"); + assert!( + msg.contains(".clarion/"), + "error must mention the missing dir: {msg}" + ); + assert!( + msg.contains("clarion install"), + "error must suggest install: {msg}" + ); + } +} diff --git a/crates/clarion-cli/src/serve.rs b/crates/clarion-cli/src/serve.rs index d570eda3..ed78834c 100644 --- a/crates/clarion-cli/src/serve.rs +++ b/crates/clarion-cli/src/serve.rs @@ -121,10 +121,31 @@ fn run_mcp_stdio( .build() .context("create MCP runtime")?; let _runtime_guard = runtime.enter(); + // Compute the project root from the db_path (db_path is + // `/.clarion/clarion.db`; pop twice). `project_root` was + // moved into `ServerState::new` below, so we recover it from + // `db_path` rather than cloning the original argument and adding a + // borrow gymnastics. + let project_root_for_lock = db_path + .parent() + .and_then(Path::parent) + .map(Path::to_path_buf) + .ok_or_else(|| anyhow!("derive project root from db path {}", db_path.display()))?; let mut state = clarion_mcp::ServerState::new(project_root, readers); let mut llm_writer = None; let mut llm_writer_join = None; + // Cross-process writer lock (gap-register STO-01). Only acquired + // when the MCP server actually opens a writer-actor (the + // summary-cache writer for the LLM provider). A read-only `serve` + // (no LLM provider) intentionally does NOT hold the lock — the + // reader pool and HTTP read API may run concurrently with each + // other and with an `analyze` invocation; only writer-mode + // operations contend. + let mut _llm_writer_lock: Option = None; if let Some(provider) = llm_provider { + _llm_writer_lock = Some(crate::project_lock::acquire_project_lock( + &project_root_for_lock, + )?); let (writer, handle) = Writer::spawn( db_path.to_owned(), DEFAULT_BATCH_SIZE, diff --git a/crates/clarion-cli/tests/project_lock.rs b/crates/clarion-cli/tests/project_lock.rs new file mode 100644 index 00000000..3436fb06 --- /dev/null +++ b/crates/clarion-cli/tests/project_lock.rs @@ -0,0 +1,283 @@ +//! Integration tests for the gap-register STO-01 cross-process advisory lock. +//! +//! Two scenarios: +//! +//! 1. `clarion analyze` refuses to run when another holder already owns the +//! project lockfile. We simulate the "other holder" by acquiring the +//! `fs2` exclusive lock from the test process itself, then spawning a +//! `clarion analyze` subprocess against the same project root and +//! asserting the subprocess exits non-zero with the expected error. +//! This is deterministic — the test process retains the lock for the +//! full lifetime of the spawned subprocess, so there is no race +//! window. +//! +//! 2. Two concurrent `clarion analyze` subprocesses against the same +//! project root: exactly one succeeds and exactly one fails with the +//! "another clarion analyze or serve is in progress" message. This +//! test is best-effort — if both processes happen to serialise +//! completely (the first finishes before the second tries to +//! acquire), the test re-tries up to a small bounded number of times. +//! The first scenario is the authoritative contract test; this one is +//! the realistic-workload smoke check. + +use std::env; +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::PathBuf; +use std::process::{Command as StdCommand, Stdio}; +use std::thread; + +use assert_cmd::Command; +use fs2::FileExt; +use tempfile::TempDir; + +/// Locate the `clarion-plugin-fixture` binary. Mirrors `wp2_e2e.rs`. +fn fixture_binary_path() -> PathBuf { + if let Ok(path) = env::var("CARGO_BIN_EXE_clarion-plugin-fixture") { + return PathBuf::from(path); + } + let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let workspace_root = manifest_dir + .parent() + .and_then(|p| p.parent()) + .expect("workspace root must exist"); + let target_dir = + env::var("CARGO_TARGET_DIR").map_or_else(|_| workspace_root.join("target"), PathBuf::from); + for profile in &["debug", "release"] { + let candidate = target_dir.join(profile).join("clarion-plugin-fixture"); + if candidate.exists() { + return candidate; + } + } + panic!( + "clarion-plugin-fixture binary not found. \ + Run `cargo build --workspace` before running this test. \ + Searched: {}", + target_dir.display() + ); +} + +/// Set up a synthetic `$PATH` dir with the fixture plugin + manifest. +/// Mirrors `wp2_e2e.rs::setup_plugin_dir`. +fn setup_plugin_dir(fixture_bin: &PathBuf) -> TempDir { + let plugin_dir = TempDir::new().expect("create plugin tempdir"); + let dest = plugin_dir.path().join("clarion-plugin-fixture"); + std::os::unix::fs::symlink(fixture_bin, &dest).expect("symlink clarion-plugin-fixture"); + let meta = fs::metadata(fixture_bin).expect("stat fixture binary"); + assert!( + meta.permissions().mode() & 0o111 != 0, + "fixture binary must be executable" + ); + let toml_src = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .unwrap() + .join("clarion-core") + .join("tests") + .join("fixtures") + .join("plugin.toml"); + let toml_dest = plugin_dir.path().join("plugin.toml"); + fs::copy(&toml_src, &toml_dest).expect("copy plugin.toml"); + plugin_dir +} + +/// Initialise a project directory with `clarion install` and write a source +/// file the fixture plugin will claim. +fn make_installed_project(plugin_path: &std::path::Path) -> TempDir { + let project_dir = TempDir::new().expect("create project tempdir"); + Command::cargo_bin("clarion") + .expect("clarion binary") + .args(["install", "--path"]) + .arg(project_dir.path()) + .assert() + .success(); + fs::write( + project_dir.path().join("demo.mt"), + b"widget demo.sample {}\n", + ) + .expect("write demo.mt"); + let _ = plugin_path; // silence unused warning if test list changes + project_dir +} + +/// Scenario 1: while the test process holds the lock, `clarion analyze` +/// must fail fast with the expected error message and a non-zero exit. +#[test] +fn analyze_refuses_when_lock_already_held() { + let fixture_bin = fixture_binary_path(); + let plugin_dir = setup_plugin_dir(&fixture_bin); + let project_dir = make_installed_project(plugin_dir.path()); + + // Acquire the lock from the test process. + let lock_path = project_dir.path().join(".clarion").join("clarion.lock"); + let lock_file = fs::OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(&lock_path) + .expect("open lockfile"); + FileExt::try_lock_exclusive(&lock_file).expect("acquire lock in test process"); + + // While the lock is held, attempt `clarion analyze`. + let new_path = + env::join_paths(std::iter::once(plugin_dir.path().to_path_buf())).expect("join_paths"); + let output = Command::cargo_bin("clarion") + .expect("clarion binary") + .args(["analyze"]) + .arg(project_dir.path()) + .env("PATH", &new_path) + .output() + .expect("spawn clarion analyze"); + + assert!( + !output.status.success(), + "clarion analyze must fail when lock is held; status={:?} stdout={} stderr={}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("another clarion analyze or serve is in progress"), + "expected lock-conflict message in stderr; got: {stderr}" + ); + assert!( + stderr.contains("clarion.lock"), + "stderr must name the lockfile path: {stderr}" + ); + + // Drop the lock; a subsequent analyze succeeds. + FileExt::unlock(&lock_file).expect("release lock"); + drop(lock_file); + + Command::cargo_bin("clarion") + .expect("clarion binary") + .args(["analyze"]) + .arg(project_dir.path()) + .env("PATH", &new_path) + .assert() + .success(); +} + +/// Scenario 2: race two concurrent `clarion analyze` subprocesses; exactly +/// one wins. +/// +/// We use raw `std::process::Command` rather than `assert_cmd::Command` for +/// the spawn step because we need `spawn()` + `wait_with_output()` to run +/// them concurrently. The losing process MUST emit the lock-conflict +/// message on stderr; the winning process MUST exit 0. +#[test] +fn concurrent_analyze_processes_exactly_one_wins() { + const RACE_ATTEMPTS: usize = 8; + let fixture_bin = fixture_binary_path(); + let plugin_dir = setup_plugin_dir(&fixture_bin); + let project_dir = make_installed_project(plugin_dir.path()); + + let new_path = + env::join_paths(std::iter::once(plugin_dir.path().to_path_buf())).expect("join_paths"); + + // Resolve the clarion binary via assert_cmd so we use the same path + // the other tests use (avoids cargo-target-dir resolution drift). + let clarion_bin = assert_cmd::cargo::cargo_bin("clarion"); + assert!( + clarion_bin.exists(), + "clarion binary missing at {}", + clarion_bin.display() + ); + + // Spawn two processes back-to-back from worker threads. The + // back-to-back spawn ordering does not guarantee contention on its + // own — analyze could finish before the second starts. We retry the + // whole race up to RACE_ATTEMPTS times until at least one observed + // case has one winner + one lock-conflict loser. With the fixture + // plugin, the first analyze takes tens of milliseconds; the spawn + // overhead of the second process is comparable, so contention is + // typical on a warm cache. + let mut saw_contended_race = false; + let mut last_diagnostic = String::new(); + + for attempt in 0..RACE_ATTEMPTS { + // Clean up runs from any previous attempt so the test stays + // idempotent. We leave `.clarion/` itself in place (the lock + // path stays valid) and drop the database file. `clarion + // install` re-creates the DB on next analyze? No — install is + // separate. Instead, just leave the DB alone and let runs + // accumulate; the test only cares about exit codes + stderr, + // not run-row count. + let _ = attempt; + + let project_for_a = project_dir.path().to_path_buf(); + let project_for_b = project_dir.path().to_path_buf(); + let bin_a = clarion_bin.clone(); + let bin_b = clarion_bin.clone(); + let path_a = new_path.clone(); + let path_b = new_path.clone(); + + let handle_a = thread::spawn(move || { + StdCommand::new(&bin_a) + .args(["analyze"]) + .arg(&project_for_a) + .env("PATH", &path_a) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn analyze A") + .wait_with_output() + .expect("wait analyze A") + }); + // No deliberate delay between the two spawns; we want the + // contention window to be as tight as the OS schedules. + let handle_b = thread::spawn(move || { + StdCommand::new(&bin_b) + .args(["analyze"]) + .arg(&project_for_b) + .env("PATH", &path_b) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn analyze B") + .wait_with_output() + .expect("wait analyze B") + }); + let out_a = handle_a.join().expect("join A"); + let out_b = handle_b.join().expect("join B"); + + let stderr_a = String::from_utf8_lossy(&out_a.stderr).into_owned(); + let stderr_b = String::from_utf8_lossy(&out_b.stderr).into_owned(); + let conflict_msg = "another clarion analyze or serve is in progress"; + let a_lost = !out_a.status.success() && stderr_a.contains(conflict_msg); + let b_lost = !out_b.status.success() && stderr_b.contains(conflict_msg); + let a_won = out_a.status.success(); + let b_won = out_b.status.success(); + + last_diagnostic = format!( + "attempt={attempt} a_status={:?} b_status={:?} \n--- stderr A ---\n{stderr_a}\n--- stderr B ---\n{stderr_b}", + out_a.status, out_b.status + ); + + if (a_won && b_lost) || (b_won && a_lost) { + saw_contended_race = true; + break; + } + // Both succeeded (they fully serialised) — try again to get an + // actual contention window. Crucially we never want to see + // "both failed" or "one failed for a non-lock reason"; assert + // that now so misconfiguration surfaces immediately. + assert!( + a_won || a_lost, + "process A failed for a non-lock reason: {last_diagnostic}" + ); + assert!( + b_won || b_lost, + "process B failed for a non-lock reason: {last_diagnostic}" + ); + } + + assert!( + saw_contended_race, + "after {RACE_ATTEMPTS} attempts no contention was observed. \ + The lock is the load-bearing guarantee; if the race never \ + materialises here the test cannot prove the lock fired. \ + Last diagnostic: {last_diagnostic}" + ); +}