v1.0 tag-cut: fs2 advisory lock against concurrent analyze (STO-01, Day 2)#19
Closed
tachyon-beep wants to merge 1 commit into
Closed
v1.0 tag-cut: fs2 advisory lock against concurrent analyze (STO-01, Day 2)#19tachyon-beep wants to merge 1 commit into
tachyon-beep wants to merge 1 commit into
Conversation
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<ProjectLock>`. 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) <noreply@anthropic.com>
Closed
11 tasks
There was a problem hiding this comment.
Pull request overview
Adds a cross-process, non-blocking advisory lock to enforce ADR-011’s single-writer invariant for a Clarion project, preventing concurrent clarion analyze (and writer-mode serve) processes from interleaving SQLite writes against the same .clarion/clarion.db.
Changes:
- Introduces
ProjectLockRAII guard +acquire_project_lock()usingfs2::FileExt::try_lock_exclusiveon.clarion/clarion.lock. - Wires the lock into
clarion analyze(always) andclarion serve(only when an LLM-provider-backed writer is spawned). - Adds integration/unit tests for lock contention behavior and adds
fs2as a workspace dependency (plus rustfmt-only whitespace changes inhttp_read.rs).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/clarion-cli/src/project_lock.rs | New lock module providing RAII guard and acquisition helper with operator-facing errors. |
| crates/clarion-cli/src/analyze.rs | Acquires the project lock before spawning the writer actor. |
| crates/clarion-cli/src/serve.rs | Acquires the project lock only when spawning the MCP LLM summary-cache writer. |
| crates/clarion-cli/src/main.rs | Registers the new project_lock module. |
| crates/clarion-cli/tests/project_lock.rs | New integration tests validating cross-process locking behavior. |
| crates/clarion-cli/src/http_read.rs | Rustfmt whitespace-only changes. |
| crates/clarion-cli/Cargo.toml | Adds fs2 dependency/dev-dependency for CLI + tests. |
| Cargo.toml | Adds workspace dependency on fs2 = "0.4". |
| Cargo.lock | Locks new fs2 (and transitive) packages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+124
to
+133
| // Compute the project root from the db_path (db_path is | ||
| // `<root>/.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()))?; |
Comment on lines
+199
to
+205
| 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, |
Collaborator
Author
|
Superseded by the Day-2 consolidation onto |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes gap-register STO-01 (Critical, deep-dive-db's highest-priority finding) — see
docs/implementation/v1.0-tag-cut/gap-register.mdlines 380-395.ADR-011 names the writer-actor as the sole SQLite write connection per Clarion project. That invariant was enforced inside a process (single mpsc channel into one actor) but had no cross-process guard. A second concurrent
clarion analyzeagainst the same project root could:status='running'row before the first writer's CommitRun,This PR adds the missing
fs2::FileExt::try_lock_exclusiveadvisory lock on.clarion/clarion.lock. Acquisition is non-blocking; a second analyze fails fast with a clear operator-facing error.Filigree:
clarion-c0caf61ab6(v1.0 blocker, P1).Implementation
fs2 = "0.4"added as a workspace dependency and reused inclarion-cli.crates/clarion-cli/src/project_lock.rsexposesacquire_project_lock(project_root) -> Result<ProjectLock>and theProjectLockRAII guard. POSIX semantics: the held file descriptor closes on Drop, releasing theflock.analyze::run_with_optionsacquires the lock immediately after the.clarion/existence check and beforeWriter::spawn. The guard is bound to a local for the function's stack frame, so every early-return path holds it throughhandle.await.serve::run_mcp_stdioacquires the lock only inside theif let Some(provider) = llm_providerbranch — read-onlyserveinvocations are intentionally not gated. The HTTP read API and MCP read-only tools may run concurrently with each other and with an analyze; only writer-mode operations contend. The asymmetry is documented in the module doc-comment ofproject_lock.rs.PROJECT_LOCK_FILENAME,PROJECT_STATE_SUBDIR) carries the ADR-035 four-field declaration block (stated basis / override surface / retune trigger / coupling). ADR-035 itself is in-flight at v1.0 tag-cut (the discipline already shows up inclarion-storage::pragma); aligning new constants now keeps the post-tag retrofit small.Notable scope deviations from the brief
1. The
recover_preexisting_running_runssweep does not exist. The gap-register text citesrun_lifecycle.rs:19-25for an unconditionalUPDATE runs SET status='failed' WHERE status='running'. That code is not in the tree — the actualrun_lifecycle.rsis a 20-linebegin_runhelper. The closest real code is the per-actorcleanup_after_channel_closeatcrates/clarion-storage/src/writer.rs:262-281, which is process-local and unaffected by the cross-process lock. STO-05 (gap-register lines 439+) explicitly defers the schema-additiveruns.owner_pid/heartbeat_atcolumns to v1.1, so a real recovery-sweep change was out of scope here.2. ADR-035 file is in-flight. It is referenced in docs (and by branch
v1.0/07-storage-identity-integrity's09f9a88commit) but not yet committed tomain. This PR cites it as in-flight and bakes the four-field annotations into the new constants per the task brief.3. Drive-by rustfmt on
http_read.rs. The file is not rustfmt-clean at the branch tip; this commit includes the formatter's whitespace fixes socargo fmt --all -- --checkpasses. Branchv1.0/07-storage-identity-integrityindependently rolled the same fix into its STO-02 commit, so the two branches will collide harmlessly at the rebase point.Test plan
project_lock.rs::tests(4 tests): clean acquire, contention error, drop releases, missing-.clarion/diagnostic.tests/project_lock.rs::analyze_refuses_when_lock_already_held— the test process holds the lock viafs2directly, spawns aclarion analyzesubprocess, asserts non-zero exit + lock-conflict stderr + lockfile path mentioned. Releases the lock and asserts a freshclarion analyzesucceeds. Deterministic — no race window.tests/project_lock.rs::concurrent_analyze_processes_exactly_one_wins— spawns two realclarion analyzesubprocesses; retries up to 8 attempts to observe a contended schedule. Fails loudly if any process exits non-zero for a non-lock reason.cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo build --workspace --binscargo nextest run --workspace --all-features— 511 tests, 6 new, all passRUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --all-featurescargo deny checkbash tests/e2e/sprint_1_walking_skeleton.sh— walking-skeleton happy path unaffected.References
docs/implementation/v1.0-tag-cut/gap-register.mdSTO-01clarion-c0caf61ab6Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com