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");