From 8ed351b30bd5dba47675d8e7edc02aab03071316 Mon Sep 17 00:00:00 2001 From: Lev Velykoivanenko Date: Sun, 27 Sep 2026 22:03:24 +0200 Subject: [PATCH] fix(locks): free every root on release --all lockm release --all only freed the holder's rows under the root of the checkout it ran from. The write hook claims in whichever checkout a write lands in, so a session that wrote into a second worktree or a scratch directory kept those claims until the TTL or session end, though the using-devkit skill promised "drop everything you hold". Release matches on the exact holder alone. Holder ids are globally unique, so the root filter bought nothing. Sub-agent rows (holder/agent) still stay for SubagentStop, as locks.md already documented. The daemon ReleaseAll request keeps its root field for wire compatibility: the daemon ignores it, and an older daemon still parses the request and releases in the caller's root, instead of failing the handshake and refusing every lock write until it restarts. MCP locks.release with all=true gets the same scope through the shared function. Closes #197 Co-authored-by: Claude Opus 5.5 --- crates/devkit-locks/src/daemon/proto.rs | 21 ++++++++++ crates/devkit-locks/src/lib.rs | 25 +++++++----- crates/devkit-locks/src/model.rs | 32 ++++++++------- crates/devkit-locks/src/store.rs | 6 +-- crates/devkit-mcp/src/locks.rs | 2 +- plugin/skills/using-devkit/SKILL.md | 2 +- .../skills/using-devkit/references/locks.md | 2 +- src/bin/devkit/locks.rs | 3 +- src/bin/devkitd/lock_server.rs | 2 +- tests/locks.rs | 40 +++++++++++++++++++ 10 files changed, 101 insertions(+), 34 deletions(-) diff --git a/crates/devkit-locks/src/daemon/proto.rs b/crates/devkit-locks/src/daemon/proto.rs index e5239d56..2278a854 100644 --- a/crates/devkit-locks/src/daemon/proto.rs +++ b/crates/devkit-locks/src/daemon/proto.rs @@ -40,6 +40,8 @@ pub enum Request { force: bool, }, ReleaseAll { + /// The caller's checkout. Release ignores it and frees the holder in + /// every root; it stays so an older daemon still parses the request. root: String, holder: String, }, @@ -112,4 +114,23 @@ mod tests { _ => panic!("wrong variant"), } } + + /// A daemon one release behind parses this exact shape. + #[test] + fn release_all_keeps_its_wire_shape() { + let msg = Request::ReleaseAll { + root: "/repo".into(), + holder: "alice".into(), + }; + let wire = serde_json::to_value(&msg).unwrap(); + assert_eq!( + wire, + serde_json::json!({ "ReleaseAll": { "root": "/repo", "holder": "alice" } }) + ); + let back: Request = serde_json::from_value(wire).unwrap(); + assert!(matches!( + back, + Request::ReleaseAll { root, holder } if root == "/repo" && holder == "alice" + )); + } } diff --git a/crates/devkit-locks/src/lib.rs b/crates/devkit-locks/src/lib.rs index 975ec0e5..e78b995e 100644 --- a/crates/devkit-locks/src/lib.rs +++ b/crates/devkit-locks/src/lib.rs @@ -394,7 +394,10 @@ pub fn release_all(as_flag: Option<&str>) -> Result> { release_all_resolved(&c.root, &c.holder) } -/// Release every lock held by `holder` under `root` (pre-resolved). +/// Release every lock held by exactly `holder` (pre-resolved), in every root. +/// Holder ids are globally unique, and the write hook claims in whichever +/// checkout a write lands in, so a root filter would strand rows. `root` is +/// the caller's checkout; it only fills the daemon's `ReleaseAll` request. pub fn release_all_resolved(root: &str, holder: &str) -> Result> { #[cfg(feature = "daemon")] if let Some(resp) = daemon_request(daemon::proto::Request::ReleaseAll { @@ -407,7 +410,7 @@ pub fn release_all_resolved(root: &str, holder: &str) -> Result> { other => Err(anyhow::anyhow!("unexpected daemon response: {other:?}")), }; } - store::release_all_with(&store::FlockStore::new(), root, holder) + store::release_all_with(&store::FlockStore::new(), holder) } /// Live locks for the current project root, or every project when `all`. @@ -837,8 +840,8 @@ mod tests { #[test] fn resolved_fns_roundtrip_via_flock_path() { // No daemon runs in unit tests, so the `_resolved` fns fall through to - // the FlockStore path. A unique root namespaces these lock - // rows. + // the FlockStore path. A unique root and holder namespace these lock + // rows, since release_all spans every root. let root = tempfile::tempdir().unwrap(); devkit_git::Git::fixture(root.path()) .args(["init", "-q", "-b", "main"]) @@ -846,30 +849,31 @@ mod tests { .unwrap(); let r = root.path().to_string_lossy().into_owned(); let paths = vec!["a.rs".to_string()]; + let holder = format!("roundtrip-{}", std::process::id()); - let out = acquire_resolved(&r, "holder-a", &paths, None, None, 60).expect("acquire"); + let out = acquire_resolved(&r, &holder, &paths, None, None, 60).expect("acquire"); assert_eq!(out.acquired.len(), 1); assert_eq!(out.acquired[0].path, "a.rs"); assert!(out.conflicts.is_empty()); let conflicts = check_resolved(&r, "holder-b", &paths).expect("check"); assert_eq!(conflicts.len(), 1); - assert_eq!(conflicts[0].held_by, "holder-a"); + assert_eq!(conflicts[0].held_by, holder); let entries = status_resolved(&r, false).expect("status"); assert!( entries .iter() - .any(|e| e.path == "a.rs" && e.holder == "holder-a") + .any(|e| e.path == "a.rs" && e.holder == holder) ); - let (released, refused) = release_resolved(&r, "holder-a", &paths, false).expect("release"); + let (released, refused) = release_resolved(&r, &holder, &paths, false).expect("release"); assert_eq!(released, vec!["a.rs".to_string()]); assert!(refused.is_empty()); - // release_all on a now-empty root is a no-op but must succeed. + // release_all with nothing held is a no-op but must succeed. assert!( - release_all_resolved(&r, "holder-a") + release_all_resolved(&r, &holder) .expect("release_all") .is_empty() ); @@ -1082,6 +1086,5 @@ mod tests { assert!(matches!(b, model::WriteDecision::Acquired)); let _ = release_all_resolved(&repo_a.path().to_string_lossy(), &holder); - let _ = release_all_resolved(&repo_b.path().to_string_lossy(), &holder); } } diff --git a/crates/devkit-locks/src/model.rs b/crates/devkit-locks/src/model.rs index 0f20f466..678850cf 100644 --- a/crates/devkit-locks/src/model.rs +++ b/crates/devkit-locks/src/model.rs @@ -339,17 +339,17 @@ impl Data { .collect() } - /// Release every lock held by `holder` in `root`; returns the freed paths. - pub fn release_all(&mut self, root: &str, holder: &str) -> Vec { - let freed: Vec = self - .locks - .values() - .filter(|e| e.root == root && e.holder == holder) - .map(|e| e.path.clone()) - .collect(); - for p in &freed { - self.locks.remove(&key_for(root, p)); - } + /// Release every lock held by exactly `holder`, in every root; returns + /// the freed paths. Its sub-agents' rows (`holder/agent`) stay. + pub fn release_all(&mut self, holder: &str) -> Vec { + let mut freed = Vec::new(); + self.locks.retain(|_, e| { + let mine = e.holder == holder; + if mine { + freed.push(e.path.clone()); + } + !mine + }); freed } @@ -639,17 +639,19 @@ mod tests { } #[test] - fn release_all_clears_only_callers_locks_in_root() { + fn release_all_clears_only_callers_locks_in_every_root() { let mut d = Data::default(); d.locks.extend([ entry("/repo", "a", "alice", 1, 0, None), entry("/repo", "b", "bob", 1, 0, None), entry("/other", "c", "alice", 1, 0, None), + entry("/other", "d", "alice/agent", 1, 0, None), ]); - let rel = d.release_all("/repo", "alice"); - assert_eq!(rel, vec!["a".to_string()]); + let mut rel = d.release_all("alice"); + rel.sort(); + assert_eq!(rel, ["a", "c"]); assert!(d.locks.contains_key(&key_for("/repo", "b"))); - assert!(d.locks.contains_key(&key_for("/other", "c"))); + assert!(d.locks.contains_key(&key_for("/other", "d"))); } #[test] diff --git a/crates/devkit-locks/src/store.rs b/crates/devkit-locks/src/store.rs index 7a49b42c..4bfcde6a 100644 --- a/crates/devkit-locks/src/store.rs +++ b/crates/devkit-locks/src/store.rs @@ -230,9 +230,9 @@ pub fn release_with( s.commit(|d| Ok(d.do_release(root, paths, holder, force))) } -/// Release every lock held by `holder` in `root` (explicit mutation). -pub fn release_all_with(s: &impl Store, root: &str, holder: &str) -> Result> { - s.commit(|d| Ok(d.release_all(root, holder))) +/// Release every lock held by `holder`, in every root (explicit mutation). +pub fn release_all_with(s: &impl Store, holder: &str) -> Result> { + s.commit(|d| Ok(d.release_all(holder))) } /// Live locks (ungated read), best-effort prune. `all` ignores the root filter. diff --git a/crates/devkit-mcp/src/locks.rs b/crates/devkit-mcp/src/locks.rs index a1565edb..6bea09c7 100644 --- a/crates/devkit-mcp/src/locks.rs +++ b/crates/devkit-mcp/src/locks.rs @@ -163,7 +163,7 @@ fn release_schema() -> Value { "properties": { "root": { "type": "string", "description": "Absolute path to the project root." }, "paths": { "type": "array", "items": { "type": "string" }, "description": "Paths to release (required unless all=true)." }, - "all": { "type": "boolean", "description": "Release every lock held by this holder in the project." }, + "all": { "type": "boolean", "description": "Release every lock held by exactly this holder, in every project. Rows its sub-agents hold stay." }, "force": { "type": "boolean", "description": "Release even locks held by another holder." }, "holder": { "type": "string", "description": "Override the session holder id." } }, diff --git a/plugin/skills/using-devkit/SKILL.md b/plugin/skills/using-devkit/SKILL.md index 052918e7..1aacba4f 100644 --- a/plugin/skills/using-devkit/SKILL.md +++ b/plugin/skills/using-devkit/SKILL.md @@ -77,7 +77,7 @@ lockm release src/auth/session.rs src/auth/mod.rs lockm release --all # or: drop everything you hold ``` -`release --all` also drops the write hook's automatic claims for this session, so it belongs at the end of a work unit rather than between edits. +`release --all` drops every claim this session holds in every checkout, not only the one you run it from, and that includes the write hook's automatic claims. Claims your sub-agents hold stay until they stop. It belongs at the end of a work unit rather than between edits. ### When a claim conflicts diff --git a/plugin/skills/using-devkit/references/locks.md b/plugin/skills/using-devkit/references/locks.md index 9464de3f..5a67cd0a 100644 --- a/plugin/skills/using-devkit/references/locks.md +++ b/plugin/skills/using-devkit/references/locks.md @@ -31,7 +31,7 @@ A long-lived shell can outlive the session that seeded its environment: a `tmux` A claim made by hand inside a sub-agent is recorded at session granularity, because no harness exposes a sub-agent id to a subprocess. It blocks every other session and does not block sibling sub-agents of your own; that isolation comes from the write hook, which does have sub-agent ids. A path your own sub-agent's hook already holds is reported as `already held on this session line` and left as it is: yours to write, but released by the lifecycle hook rather than by a `release` of your own. -`lockm release --all` frees every row this project holds under the bare session id — the session's manual claims and its top-level write-hook claims alike — whether you run it from the top level or from a sub-agent. Rows recorded as `session/agent`, which is how the hook records a sub-agent's writes, are left for `SubagentStop`. +`lockm release --all` frees every row held under the bare session id in every checkout, not only the one you run it from. That covers the session's manual claims and its top-level write-hook claims alike, whether you run it from the top level or from a sub-agent. Rows recorded as `session/agent`, which is how the hook records a sub-agent's writes, are left for `SubagentStop`. ## TTL diff --git a/src/bin/devkit/locks.rs b/src/bin/devkit/locks.rs index 94fc1d42..f03aad29 100644 --- a/src/bin/devkit/locks.rs +++ b/src/bin/devkit/locks.rs @@ -54,7 +54,8 @@ pub(crate) enum Cmd { /// pid. #[arg(long = "as")] holder: Option, - /// Release every path this holder claims. + /// Release every path this holder claims, in every checkout, not only + /// this one. Rows its sub-agents hold (`holder/agent`) stay. #[arg(long)] all: bool, /// Release even a path held by another session. diff --git a/src/bin/devkitd/lock_server.rs b/src/bin/devkitd/lock_server.rs index 32ae1262..fde2432e 100644 --- a/src/bin/devkitd/lock_server.rs +++ b/src/bin/devkitd/lock_server.rs @@ -67,7 +67,7 @@ pub(crate) fn dispatch(daemon: &Arc, req: Request) -> Response { Ok((released, refused)) => Response::Released { released, refused }, Err(e) => Response::Err(format!("{e:#}")), }, - Request::ReleaseAll { root, holder } => match store::release_all_with(&s, &root, &holder) { + Request::ReleaseAll { holder, .. } => match store::release_all_with(&s, &holder) { Ok(v) => Response::Freed(v), Err(e) => Response::Err(format!("{e:#}")), }, diff --git a/tests/locks.rs b/tests/locks.rs index 037b3883..82f9a0a2 100644 --- a/tests/locks.rs +++ b/tests/locks.rs @@ -96,6 +96,46 @@ fn release_frees_for_other_holder() { assert!(b.status.success(), "bob can acquire after alice releases"); } +/// The write hook claims in whichever checkout a write lands in, so one +/// session's rows span several roots. +#[test] +fn release_all_frees_the_holder_in_every_root() { + let (_dir, link) = shimtest::linked("lockm"); + let (here, elsewhere) = (project(), project()); + let state = tempfile::tempdir().unwrap(); + for proj in [&here, &elsewhere] { + for holder in ["alice", "alice/agent", "bob"] { + let path = format!("{}.rs", holder.replace('/', "-")); + let a = run(&link, proj.path(), state.path(), &[ + "acquire", &path, "--as", holder, + ]); + assert!(a.status.success(), "{holder} acquires {path}"); + } + } + + let r = run(&link, here.path(), state.path(), &[ + "release", "--all", "--as", "alice", + ]); + assert!(r.status.success()); + assert_eq!( + String::from_utf8_lossy(&r.stdout).trim(), + "released 2 lock(s)" + ); + + let s = run(&link, here.path(), state.path(), &[ + "status", "--all", "--json", + ]); + let v: serde_json::Value = serde_json::from_slice(&s.stdout).expect("json on stdout"); + let mut left: Vec<&str> = v["locks"] + .as_array() + .unwrap() + .iter() + .map(|e| e["holder"].as_str().unwrap()) + .collect(); + left.sort(); + assert_eq!(left, ["alice/agent", "alice/agent", "bob", "bob"]); +} + #[test] fn same_holder_reacquire_is_ok() { let (_dir, link) = shimtest::linked("lockm");