From 06f2ce9a156fb4f4afd47ca1e67e8085d2d2d0f9 Mon Sep 17 00:00:00 2001 From: Duncan Date: Tue, 11 Aug 2026 15:02:33 -0400 Subject: [PATCH] fix(desktop): harden keyring secret projection fail-closed + cross-process txn lock Close the r4 blocking findings on the keyring secret-projection PR: - Fail-closed load: a conflict-marker read that returns Err is now treated as unavailable, not fall-through. load_blob caches successful reads but never errors, so a transient marker-read failure could be followed by a cached-success value read that hydrates a known-conflicted credential. - Conflict-marker cleanup only on proven resolution: the dev-migration planner clears a conflict: marker only when source and destination converge OR the coordinate is proven no longer live in canonical JSON. A disappearing source while the destination value is still live retains the marker (fail closed). - Cross-process transaction lock (Race 1): the lock now spans the other-half read through generation writes to the JSON commit in both save paths, so two Desktop processes cannot interleave a cross-half overwrite. Keyed by the symlink-resolved canonical store directory inode, not the keyring service, so processes sharing one JSON file always contend on one lock. - Transaction lockfile out of /tmp: the lock target is the store directory inode in the owner's app-data tree, immune to the tmp-cleaner unlink/recreate split that let two processes both "hold" a /tmp lockfile. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src-tauri/src/commands/personas/update.rs | 5 +- .../managed_agents/dev_service_migration.rs | 290 +++++++++++++++++- .../src/managed_agents/global_config/mod.rs | 8 +- .../src/managed_agents/secret_projection.rs | 36 ++- .../managed_agents/secret_projection_tests.rs | 61 ++++ .../src-tauri/src/managed_agents/storage.rs | 36 +-- .../src/migration/migration_secrets.rs | 4 +- desktop/src-tauri/src/secret_store.rs | 213 ++++++++++--- desktop/src-tauri/src/secret_store_tests.rs | 153 ++++++--- 9 files changed, 671 insertions(+), 135 deletions(-) diff --git a/desktop/src-tauri/src/commands/personas/update.rs b/desktop/src-tauri/src/commands/personas/update.rs index 74dd6caae4..9f84c877ca 100644 --- a/desktop/src-tauri/src/commands/personas/update.rs +++ b/desktop/src-tauri/src/commands/personas/update.rs @@ -198,8 +198,9 @@ pub(super) async fn update_persona_with( // `auth_tag` empty, so signing would strip the NIP-OA // tag. Skip only this instance — one stale record must // not block the rest of the persona rename. The local - // record edits above still persist so a later reachable - // boot reconciles the profile. + // record edits above still persist, so this instance's + // profile converges on its next successful start/reconcile + // (once its secrets hydrate), not merely on a reachable boot. let secrets_ok = crate::managed_agents::effective_config::require_effective_secrets_available( record, diff --git a/desktop/src-tauri/src/managed_agents/dev_service_migration.rs b/desktop/src-tauri/src/managed_agents/dev_service_migration.rs index 00121c725c..b15e7a696b 100644 --- a/desktop/src-tauri/src/managed_agents/dev_service_migration.rs +++ b/desktop/src-tauri/src/managed_agents/dev_service_migration.rs @@ -92,12 +92,18 @@ pub fn migrate_agent_secrets_to_dev_service(app: &tauri::AppHandle) { // references it (global-agent-config.json is not a shared file). let global_refs = collect_global_env_refs(app); + // Full blob coordinates referenced by canonical JSON. Used to prove a + // conflict marker is safe to clear when its coordinate is no longer live. + // `None` when either store is unreadable/malformed — in that case liveness + // is unknown and only value-convergence can clear a marker (fail closed). + let live_coords = collect_live_projection_coords(app); + let DevMigrationPlan { to_write, conflict_keys, write_marker, conflict_markers_to_clear, - } = plan_dev_secrets_migration(&src_map, &dest_map, &global_refs); + } = plan_dev_secrets_migration(&src_map, &dest_map, &global_refs, live_coords.as_ref()); for key in &conflict_keys { eprintln!( @@ -208,6 +214,7 @@ fn plan_dev_secrets_migration( src_map: &std::collections::HashMap, dest_map: &std::collections::HashMap, global_refs: &std::collections::HashSet, + live_coords: Option<&std::collections::HashSet>, ) -> DevMigrationPlan { use crate::managed_agents::secret_projection::{conflict_marker_key, is_projection_key}; @@ -241,17 +248,38 @@ fn plan_dev_secrets_migration( } } - // Clear conflict markers for coordinates that no longer conflict. A marker - // present in dest whose underlying coordinate is NOT in this run's - // conflict set has resolved (values now agree, or the coordinate is gone), - // so lift the availability block. - let still_conflicting: std::collections::HashSet<&String> = conflict_keys.iter().collect(); + // Clear a conflict marker ONLY when the conflict is provably resolved. + // Two proofs qualify: + // 1. Source and destination both hold the coordinate AND agree — the + // values converged, so the destination value can be trusted again. + // 2. The coordinate is proven no longer live in canonical JSON — no + // record references it, so nothing will ever hydrate it and the + // two-cycle GC retires the orphaned generation. + // Every other state RETAINS the marker and keeps the coordinate + // unavailable (fail closed). In particular, source-absent while the + // destination still holds the (conflicting) value and canonical JSON still + // references the coordinate is NOT a resolution — a disappearing source is + // not evidence that the destination value won. When liveness cannot be + // proven (`live_coords` is `None` because a store was unreadable), only + // proof #1 can clear a marker. + let still_conflicting: std::collections::HashSet<&str> = + conflict_keys.iter().map(String::as_str).collect(); let conflict_markers_to_clear: Vec = dest_map .keys() .filter(|k| k.starts_with(NS_CONFLICT_PREFIX)) .filter(|marker| { let coord = marker.trim_start_matches(NS_CONFLICT_PREFIX); - !still_conflicting.contains(&coord.to_string()) + // A coordinate re-flagged as conflicting this run is also being + // (re)written as a marker above — never clear it in the same pass. + if still_conflicting.contains(coord) { + return false; + } + let converged = matches!( + (src_map.get(coord), dest_map.get(coord)), + (Some(s), Some(d)) if s == d + ); + let not_live = live_coords.is_some_and(|live| !live.contains(coord)); + converged || not_live }) .cloned() .collect(); @@ -299,6 +327,49 @@ fn collect_global_env_refs(app: &tauri::AppHandle) -> std::collections::HashSet< refs } +/// Collect the full blob coordinates referenced by the canonical JSON stores +/// (`managed-agents.json` + `global-agent-config.json`), reusing the same +/// [`collect_live_refs`] the GC uses so the liveness notion is identical. +/// +/// Returns `None` when either store is unreadable or the JSON is in a state +/// `collect_live_refs` refuses to reason about (malformed, duplicate, or +/// inline+ref ambiguity). A `None` result must be treated as "liveness +/// unknown" — the marker-cleanup caller then declines to clear a marker on +/// liveness grounds and keeps the coordinate unavailable (fail closed). +#[cfg(debug_assertions)] +fn collect_live_projection_coords( + app: &tauri::AppHandle, +) -> Option> { + use crate::managed_agents::secret_projection::collect_live_refs; + + let agents_path = crate::managed_agents::storage::managed_agents_store_path(app).ok()?; + let global_path = app + .path() + .app_data_dir() + .ok()? + .join("agents/global-agent-config.json"); + + // An absent file is a *known* empty liveness set, so default it to the + // store's empty JSON shape (`[]` for the agents array, `{}` for the global + // object). Only an unreadable *existing* file is genuinely unknown → `None`. + let agents_json = read_json_or_default(&agents_path, "[]")?; + let global_json = read_json_or_default(&global_path, "{}")?; + + collect_live_refs(&agents_json, &global_json).map(|live| live.coords) +} + +/// Read a JSON store to a string, substituting `default_empty` for an absent +/// file. Returns `None` only when an existing file cannot be read — a genuine +/// "unknown" state the caller must fail closed on. +#[cfg(debug_assertions)] +fn read_json_or_default(path: &std::path::Path, default_empty: &str) -> Option { + match std::fs::read_to_string(path) { + Ok(s) => Some(s), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Some(default_empty.to_string()), + Err(_) => None, + } +} + #[cfg(all(test, debug_assertions))] mod tests { use super::*; @@ -318,7 +389,7 @@ mod tests { // write the marker (migration is complete). let key = agent_env_key("abc", "gen1"); let src = map(&[(&key, "val")]); - let plan = plan_dev_secrets_migration(&src, &HashMap::new(), &HashSet::new()); + let plan = plan_dev_secrets_migration(&src, &HashMap::new(), &HashSet::new(), None); assert!(plan.conflict_keys.is_empty()); assert!(plan.write_marker, "clean migration must write the marker"); @@ -336,7 +407,7 @@ mod tests { let src = map(&[(&conflict, "src-val"), (&fresh, "fresh-val")]); let dest = map(&[(&conflict, "dest-val")]); - let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new()); + let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new(), None); assert_eq!(plan.conflict_keys, vec![conflict.clone()]); assert!( @@ -366,7 +437,7 @@ mod tests { let src = map(&[(&key, "same")]); let dest = map(&[(&key, "same")]); - let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new()); + let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new(), None); assert!(plan.conflict_keys.is_empty()); assert!(plan.write_marker); @@ -381,7 +452,8 @@ mod tests { #[test] fn test_empty_source_writes_only_marker() { // Nothing to copy, no conflict → clean: the batch is just the marker. - let plan = plan_dev_secrets_migration(&HashMap::new(), &HashMap::new(), &HashSet::new()); + let plan = + plan_dev_secrets_migration(&HashMap::new(), &HashMap::new(), &HashSet::new(), None); assert!(plan.write_marker); assert_eq!(plan.to_write.len(), 1); assert!(plan.to_write.contains_key(DEV_SECRETS_MIGRATION_MARKER)); @@ -393,7 +465,7 @@ mod tests { // copied across services (global-agent-config.json is not shared). let key = global_env_key("gen-unref"); let src = map(&[(&key, "val")]); - let plan = plan_dev_secrets_migration(&src, &HashMap::new(), &HashSet::new()); + let plan = plan_dev_secrets_migration(&src, &HashMap::new(), &HashSet::new(), None); assert!( !plan.to_write.contains_key(&key), @@ -411,7 +483,7 @@ mod tests { let key = global_env_key("gen-ref"); let src = map(&[(&key, "val")]); let refs: HashSet = ["gen-ref".to_string()].into_iter().collect(); - let plan = plan_dev_secrets_migration(&src, &HashMap::new(), &refs); + let plan = plan_dev_secrets_migration(&src, &HashMap::new(), &refs, None); assert_eq!(plan.to_write.get(&key).map(String::as_str), Some("val")); } @@ -421,7 +493,7 @@ mod tests { // A non-projection key (e.g. an nsec or identity marker) present only // in src must never be copied by this migration. let src = map(&[("some-agent-nsec-key", "secret")]); - let plan = plan_dev_secrets_migration(&src, &HashMap::new(), &HashSet::new()); + let plan = plan_dev_secrets_migration(&src, &HashMap::new(), &HashSet::new(), None); assert!(!plan.to_write.contains_key("some-agent-nsec-key")); assert!(plan.write_marker); // no projection conflict @@ -438,7 +510,7 @@ mod tests { let src = map(&[(&conflict, "src-val")]); let dest = map(&[(&conflict, "dest-val")]); - let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new()); + let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new(), None); assert_eq!(plan.conflict_keys, vec![conflict.clone()]); assert!( @@ -464,7 +536,7 @@ mod tests { let src = map(&[(&coord, "agreed")]); let dest = map(&[(&coord, "agreed"), (&marker, "1")]); - let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new()); + let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new(), None); assert!( plan.conflict_keys.is_empty(), @@ -486,7 +558,7 @@ mod tests { let src = map(&[(&coord, "src-val")]); let dest = map(&[(&coord, "dest-val"), (&marker, "1")]); - let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new()); + let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new(), None); assert_eq!(plan.conflict_keys, vec![coord.clone()]); assert!( @@ -498,4 +570,188 @@ mod tests { "the (re)written marker keeps the coordinate unavailable" ); } + + #[test] + fn test_marker_retained_when_source_gone_but_dest_coord_still_live() { + // F4: a disappearing SOURCE coordinate is not evidence the destination + // won the conflict. If dest still holds the (conflicting) value AND + // canonical JSON still references the coordinate, the marker must be + // RETAINED so `load_secret` keeps failing closed. This is the fail-open + // arm Thufir flagged: the old code cleared any marker whose coordinate + // left the conflict set (source disappearance counted as resolution). + use crate::managed_agents::secret_projection::conflict_marker_key; + let coord = agent_env_key("abc", "gen1"); + let marker = conflict_marker_key(&coord); + // Source no longer holds the coordinate; dest still holds the old value + // plus the marker. + let src = map(&[]); + let dest = map(&[(&coord, "dest-val"), (&marker, "1")]); + // Canonical JSON STILL references the coordinate's generation → live. + let live: HashSet = [coord.clone()].into_iter().collect(); + + let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new(), Some(&live)); + + assert!( + plan.conflict_keys.is_empty(), + "source is gone, so this run detects no fresh conflict" + ); + assert!( + !plan.conflict_markers_to_clear.contains(&marker), + "marker must be RETAINED: source disappearance is not resolution while \ + the destination coordinate is still live" + ); + } + + #[test] + fn test_marker_cleared_when_coord_no_longer_live() { + // F4 positive bound: once the coordinate is proven no longer referenced + // by canonical JSON, nothing can hydrate it, so the stale marker is + // safe to clear (the two-cycle GC retires the orphaned generation). + use crate::managed_agents::secret_projection::conflict_marker_key; + let coord = agent_env_key("abc", "gen1"); + let marker = conflict_marker_key(&coord); + let src = map(&[]); + let dest = map(&[(&coord, "dest-val"), (&marker, "1")]); + // Canonical JSON references NO coordinates → coord is not live. + let live: HashSet = HashSet::new(); + + let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new(), Some(&live)); + + assert!( + plan.conflict_markers_to_clear.contains(&marker), + "a marker whose coordinate is proven no longer live must be cleared" + ); + } + + #[test] + fn test_marker_retained_when_liveness_unknown_and_values_disagree() { + // F4: when liveness cannot be proven (`live_coords` is None because a + // store was unreadable) and the values still disagree, only value + // convergence (proof #1) may clear a marker — so it is RETAINED. + use crate::managed_agents::secret_projection::conflict_marker_key; + let coord = agent_env_key("abc", "gen1"); + let marker = conflict_marker_key(&coord); + let src = map(&[(&coord, "src-val")]); + let dest = map(&[(&coord, "dest-val"), (&marker, "1")]); + + // liveness unknown → None. src/dest disagree AND this coord conflicts + // again this run, so it is re-marked and never cleared. + let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new(), None); + + assert!( + !plan.conflict_markers_to_clear.contains(&marker), + "with liveness unknown and values disagreeing, the marker must be retained" + ); + } + + // ── F4 end-to-end: a PLANNER-PRODUCED marker drives hydration refusal ──── + // + // The other F4 tests hand-seed `conflict:` and prove `load_secret` + // fails closed on it. This one closes the loop from the ORIGIN: run the + // real planner on a conflicting src/dest, apply its `to_write` batch + // verbatim (marker included) into a store, then hydrate a record pointing + // at the conflicted generation. It proves the whole chain — + // planner → keyring batch → hydration → spawn refusal — with no + // hand-placed marker anywhere in the test. + #[test] + fn test_planner_conflict_marker_drives_hydration_refusal() { + use crate::managed_agents::secret_projection::ProjectionStore; + use crate::managed_agents::secret_seam::hydrate_all_secrets_for_records; + use crate::managed_agents::storage::spawn_key_refusal; + use crate::managed_agents::ManagedAgentRecord; + use std::cell::RefCell; + + // Minimal in-memory store; hydration only reads via `load_key`. + struct FakeStore(RefCell>); + impl ProjectionStore for FakeStore { + fn write_and_verify(&self, key: &str, value: &str) -> Result<(), String> { + self.0.borrow_mut().insert(key.into(), value.into()); + Ok(()) + } + fn load_key(&self, key: &str) -> Result, String> { + Ok(self.0.borrow().get(key).cloned()) + } + fn load_all(&self) -> Result>, String> { + Ok(Some(self.0.borrow().clone())) + } + fn store_batch(&self, entries: &HashMap) -> Result<(), String> { + for (k, v) in entries { + self.0.borrow_mut().insert(k.clone(), v.clone()); + } + Ok(()) + } + fn remove_batch(&self, keys: &[&str]) -> Result<(), String> { + for k in keys { + self.0.borrow_mut().remove(*k); + } + Ok(()) + } + } + + let pubkey = "abc"; + let coord = agent_env_key(pubkey, "gen_live"); + // Source and destination disagree on the same coordinate → conflict. + let src = map(&[(&coord, r#"{"ANTHROPIC_API_KEY":"src-value"}"#)]); + let dest = map(&[(&coord, r#"{"ANTHROPIC_API_KEY":"dest-value"}"#)]); + + // Run the REAL planner. It flags the conflict, withholds the completion + // marker, and emits the `conflict:` marker in its write batch. + let plan = plan_dev_secrets_migration(&src, &dest, &HashSet::new(), None); + assert_eq!(plan.conflict_keys, vec![coord.clone()]); + assert!( + !plan.write_marker, + "a conflict withholds the completion marker" + ); + + // Apply the planner's batch verbatim (the conflict marker rides along), + // plus the destination value the migration left untouched — the exact + // post-migration keyring state for an unresolved conflict. + let store = FakeStore(RefCell::new(HashMap::new())); + store.store_batch(&plan.to_write).unwrap(); + store + .write_and_verify(&coord, r#"{"ANTHROPIC_API_KEY":"dest-value"}"#) + .unwrap(); + + // Hydrate a record that references the conflicted generation. + let mut records = vec![{ + let mut r: ManagedAgentRecord = serde_json::from_str(&format!( + r#"{{ + "pubkey": "{pubkey}", + "name": "test-agent", + "private_key_nsec": "nsec1realkey", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }}"# + )) + .expect("instance record"); + r.env_vars_ref = Some("gen_live".to_string()); + r + }]; + + let unavailable = hydrate_all_secrets_for_records(&store, &mut records); + + assert_eq!( + unavailable, + vec![pubkey.to_string()], + "the planner's marker must surface the instance as unavailable" + ); + assert!( + records[0].secrets_unavailable, + "a planner-produced conflict marker must set secrets_unavailable" + ); + assert!( + records[0].env_vars.is_empty(), + "the untrusted destination value must NOT hydrate" + ); + assert!( + spawn_key_refusal(&records[0]).is_some(), + "spawn must refuse the instance while the planner's conflict stands" + ); + } } diff --git a/desktop/src-tauri/src/managed_agents/global_config/mod.rs b/desktop/src-tauri/src/managed_agents/global_config/mod.rs index d92082eebd..d7a7551cc5 100644 --- a/desktop/src-tauri/src/managed_agents/global_config/mod.rs +++ b/desktop/src-tauri/src/managed_agents/global_config/mod.rs @@ -274,8 +274,12 @@ pub fn save_global_agent_config(app: &AppHandle, config: &GlobalAgentConfig) -> // delete the just-written generation before its ref lands in JSON. The // lock releases when `_txn` drops at end of function. Callers never hold // it already (boot migration releases its agent-store span before - // calling here), so this does not nest. - let _txn = store.transaction_lock()?; + // calling here), so this does not nest. Keyed by the SAME canonical + // store directory as the agent-store saves and GC (via + // `acquire_secret_txn_lock`) so every mutator of the shared keyring + // blob serializes on one inode — global-agent-config.json is not itself + // shared, but it writes the same blob the agent store does. + let _txn = crate::managed_agents::storage::acquire_secret_txn_lock(app)?; let inline_env = if !config.env_vars.is_empty() { serialize_env_map(&config.env_vars).ok() } else { diff --git a/desktop/src-tauri/src/managed_agents/secret_projection.rs b/desktop/src-tauri/src/managed_agents/secret_projection.rs index ece532eeb9..f0314f9158 100644 --- a/desktop/src-tauri/src/managed_agents/secret_projection.rs +++ b/desktop/src-tauri/src/managed_agents/secret_projection.rs @@ -258,16 +258,32 @@ pub fn load_secret( }; let key = coord_key_fn(gen); // Fail closed on an unresolved dev-migration conflict for this coordinate. - // No marker (`Ok(None)`), or a marker store-read error (`Err(_)`), falls - // through to the normal load below: a marker-read error must not mask a - // genuinely available secret, and the normal load fails closed on its own - // errors. - if let Ok(Some(_)) = store.load_key(&conflict_marker_key(&key)) { - return Err(format!( - "secret unavailable: {context} ref {gen} has an unresolved \ - dev-migration conflict at {key}; refusing to hydrate a \ - potentially-wrong value until the conflict is resolved" - )); + // Only a definitive `Ok(None)` (marker absent) proceeds to the value read. + // + // A marker-read `Err(_)` MUST also fail closed: `SecretStore::load_blob` + // caches a successful read but never caches an error, so a transient + // marker-read failure could be followed immediately by a *successful* + // value read that hydrates a known-conflicted credential. Falling through + // on `Err` would therefore re-open exactly the window the marker exists to + // close, so a marker read we cannot complete is treated as "conflict + // status unknown" → unavailable. + match store.load_key(&conflict_marker_key(&key)) { + Ok(Some(_)) => { + return Err(format!( + "secret unavailable: {context} ref {gen} has an unresolved \ + dev-migration conflict at {key}; refusing to hydrate a \ + potentially-wrong value until the conflict is resolved" + )) + } + Err(e) => { + return Err(format!( + "secret unavailable: {context} ref {gen} conflict-marker read \ + at {key} failed ({e}); refusing to hydrate until the marker \ + can be checked (a later cached-success value read must not \ + bypass an unresolved conflict)" + )) + } + Ok(None) => {} // no marker — proceed to the normal load } match store.load_key(&key) { Ok(Some(v)) => Ok(Some(v)), diff --git a/desktop/src-tauri/src/managed_agents/secret_projection_tests.rs b/desktop/src-tauri/src/managed_agents/secret_projection_tests.rs index 205de9a4e2..fd3ccd058f 100644 --- a/desktop/src-tauri/src/managed_agents/secret_projection_tests.rs +++ b/desktop/src-tauri/src/managed_agents/secret_projection_tests.rs @@ -196,6 +196,67 @@ fn test_load_secret_err_when_keyring_unreachable() { assert!(result.is_err()); } +/// A [`ProjectionStore`] whose conflict-marker read fails transiently while +/// the value read succeeds — the exact production shape Thufir flagged: the +/// real `SecretStore::load_blob` caches a successful read but never caches an +/// error, so a first (marker) read can error and an immediately-following +/// (value) read can succeed against a warm cache. The value is present and +/// KNOWN-conflicted; `load_secret` must fail closed on the marker-read `Err` +/// rather than fall through and hydrate it. +struct MarkerReadErrStore { + value_key: String, + value: String, +} + +impl ProjectionStore for MarkerReadErrStore { + fn write_and_verify(&self, _key: &str, _value: &str) -> Result<(), String> { + unreachable!("load_secret never writes") + } + fn load_key(&self, key: &str) -> Result, String> { + if key.starts_with("conflict:") { + // Transient backend failure on the marker read only. + return Err("transient marker read failure".to_string()); + } + if key == self.value_key { + // The value read succeeds — a known-conflicted credential. + return Ok(Some(self.value.clone())); + } + Ok(None) + } + fn load_all(&self) -> Result>, String> { + unreachable!("load_secret never calls load_all") + } + fn store_batch(&self, _entries: &HashMap) -> Result<(), String> { + unreachable!("load_secret never writes") + } + fn remove_batch(&self, _keys: &[&str]) -> Result<(), String> { + unreachable!("load_secret never deletes") + } +} + +#[test] +fn test_load_secret_fails_closed_when_marker_read_errors() { + // F4: a transient conflict-marker read `Err` must be treated as "conflict + // status unknown" → unavailable, even though the value read would succeed. + // Falling through would hydrate a known-conflicted value the moment the + // marker check flaked. + let value_key = global_env_key("gen1"); + let store = MarkerReadErrStore { + value_key, + value: "sk-known-conflicted".to_string(), + }; + let result = load_secret(&store, Some("gen1"), global_env_key, "global:env"); + assert!( + result.is_err(), + "a marker-read Err must fail closed, not fall through to the value read" + ); + let msg = result.unwrap_err(); + assert!( + msg.contains("conflict-marker read") && msg.contains("failed"), + "error must name the marker-read failure as the refusal cause, got: {msg}" + ); +} + // ── Two-cycle GC tests ──────────────────────────────────────────────── fn make_agents_json(env_ref: Option<&str>) -> String { diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 2565aa9b15..d48923a689 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -388,10 +388,13 @@ fn hydrate_keys_with(store: &impl KeyStore, records: &mut [ManagedAgentRecord]) /// before the wholesale rewrite so a definition is never dropped by an /// instance-side save (and vice versa via [`save_agent_definitions`]). pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> Result<(), String> { + // Lock FIRST — the definition half re-read below must be under the lock + // (Race 1); see `acquire_secret_txn_lock` for span + residual. + let _txn = acquire_secret_txn_lock(app)?; let definitions = load_agent_definitions(app).unwrap_or_default(); let mut sorted = records.to_vec(); // A caller-supplied key-less record would collide with the definition - // half re-read below; instances always carry a pubkey. + // half re-read above; instances always carry a pubkey. sorted.retain(|record| !record.pubkey.is_empty()); sorted.sort_by(|left, right| { left.name @@ -403,11 +406,6 @@ pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> R // Persist each nsec to the keyring; on success blank the inline copy. persist_agent_keys(&mut sorted); - // Cross-process transaction lock across the generation writes - // (`strip_and_persist_all_for_records`) and the JSON commit - // (`write_agent_store`) — see `acquire_secret_txn_lock`. Leaf-level. - let _txn = acquire_secret_txn_lock()?; - // Persist env/auth_tag/provider_config to the keyring; on success blank // inline values and set *_ref. On failure keep inline and clear *_ref. if let Some(store) = agent_secret_store() { @@ -423,15 +421,14 @@ pub(crate) fn save_agent_definitions( app: &AppHandle, definitions: &[ManagedAgentRecord], ) -> Result<(), String> { + // Lock FIRST — the instance half re-read below must be under the lock + // (Race 1); mirror of `save_managed_agents`. Leaf-level. + let _txn = acquire_secret_txn_lock(app)?; let mut instances = load_agent_store(app)?; instances.retain(|record| !record.pubkey.is_empty()); let mut definitions = definitions.to_vec(); definitions.retain(|record| record.pubkey.is_empty()); - // Cross-process txn lock across gen-writes + JSON commit (see - // `save_managed_agents`). Leaf-level, never nested. - let _txn = acquire_secret_txn_lock()?; - // Persist definition env_vars to the keyring before writing JSON. if let Some(store) = agent_secret_store() { strip_and_persist_all_for_records(store, &mut definitions); @@ -672,17 +669,20 @@ pub(crate) fn agent_secret_store_pub() -> Option<&'static crate::secret_store::S agent_secret_store() } -/// Acquire the cross-process secret transaction lock for the agent secret -/// store, or `Ok(None)` when the build has no keyring backend. A save -/// (gen-write → JSON commit) and GC (live-ref read → blob remove) hold it for -/// their full span so two Desktop processes cannot interleave. Never nested; -/// the per-op `mutate_blob` lock is a different file taken after this one. +/// Acquire the cross-process secret transaction lock for the agent store, or +/// `Ok(None)` on a keyless build. A save and GC hold it for their full span so +/// two Desktop processes cannot interleave. Keyed by the resolved store dir +/// ([`crate::secret_store::store_txn_lock_dir`]). Residual (deferred, PR body): +/// the lock makes each `save_*` atomic cross-process but cannot make a caller's +/// pre-lock snapshot transactional — caller races stay last-writer-wins (pre-existing). pub(crate) fn acquire_secret_txn_lock( + app: &AppHandle, ) -> Result, String> { - match agent_secret_store() { - Some(store) => store.transaction_lock().map(Some), - None => Ok(None), + if agent_secret_store().is_none() { + return Ok(None); } + let dir = crate::secret_store::store_txn_lock_dir(&managed_agents_store_path(app)?); + crate::secret_store::transaction_lock_at(&dir).map(Some) } /// Atomic, symlink-preserving JSON write. diff --git a/desktop/src-tauri/src/migration/migration_secrets.rs b/desktop/src-tauri/src/migration/migration_secrets.rs index 85ed22b8a1..bcc51cd890 100644 --- a/desktop/src-tauri/src/migration/migration_secrets.rs +++ b/desktop/src-tauri/src/migration/migration_secrets.rs @@ -35,7 +35,7 @@ pub(super) fn migrate_inline_secrets_to_keyring(app: &tauri::AppHandle) { // just-written generation in the window before its ref lands in JSON. // The in-process `managed_agents_store_lock` above only serializes THIS // process; the file lock closes the cross-process interleave. - match crate::managed_agents::storage::acquire_secret_txn_lock() { + match crate::managed_agents::storage::acquire_secret_txn_lock(app) { Ok(_txn) => { let changed = migrate_inline_secrets_in_records(app, &mut records); if changed { @@ -226,7 +226,7 @@ pub(crate) fn run_secret_gc(app: &tauri::AppHandle) { // guard releases when this function returns. Leaf-level: GC is never // called while another secret transaction lock is held (boot migration // releases its extraction and global-save spans before calling here). - let _txn = match crate::managed_agents::storage::acquire_secret_txn_lock() { + let _txn = match crate::managed_agents::storage::acquire_secret_txn_lock(app) { Ok(guard) => guard, Err(e) => { eprintln!( diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs index bbf8849309..df13eb144e 100644 --- a/desktop/src-tauri/src/secret_store.rs +++ b/desktop/src-tauri/src/secret_store.rs @@ -86,31 +86,45 @@ fn blob_lockfile_path(service: &str) -> PathBuf { } } -/// Return the path of the *transaction* advisory lockfile for `service`. +/// Resolve the directory whose inode is the cross-process *transaction* lock +/// target for the store physically held by `store_file` +/// (`managed-agents.json`). /// -/// Distinct from [`blob_lockfile_path`] (the per-operation `mutate_blob` -/// lock): a save's generation-write → JSON-commit and GC's live-ref read → -/// blob-remove each hold THIS lock end-to-end, so two Desktop processes -/// sharing the store cannot interleave a GC delete against an in-flight -/// generation. Using a separate file keeps the coarse transaction lock and -/// the fine per-operation lock orthogonal: a `mutate_blob` inside a -/// transaction takes the op-lock on a *different* file, so there is no -/// same-process self-deadlock (the two locks are always acquired in the same -/// order — transaction first, operation second — so no cycle is possible). +/// The transaction lock is deliberately NOT keyed like the per-operation blob +/// lock. That lock is a service-keyed lockfile under `/tmp`; this one is the +/// store's own **directory inode**. Two properties fall out of that choice, +/// and both are load-bearing: /// -/// On Windows the same name feeds a distinct named kernel mutex (the `.txn` -/// file stem yields a different mutex name than the per-op lock). -fn blob_txn_lockfile_path(service: &str) -> PathBuf { - #[cfg(unix)] - { - // SAFETY: getuid() is always safe on Unix — it never fails. - let uid = unsafe { libc::getuid() }; - PathBuf::from(format!("/tmp/buzz-keychain-{uid}-{service}.txn.lock")) - } - #[cfg(not(unix))] - { - std::env::temp_dir().join(format!("buzz-keychain-{service}.txn.lock")) +/// - **Stable shared identity, independent of keyring service.** +/// `managed-agents.json` is symlinked *per file* across dev worktrees while +/// its parent directory is not (see `migration::SHARED_AGENT_FILES`), so +/// resolving the file through its symlink and taking the real parent yields +/// the one canonical directory every process sharing the store contends on — +/// even when `keyring_service()` hands those processes different scoped +/// services. Keying by service would let two processes share one JSON inode +/// while taking different locks; keying by the resolved directory cannot. +/// +/// - **Immunity to the unlink/recreate split.** A `/tmp` lockfile can be +/// unlinked by a temp cleaner while a process holds its `flock`; a second +/// process then recreates the pathname, locks a fresh inode, and both +/// "hold" the lock. A directory that holds the store files is non-empty and +/// lives in the owner's app-data tree: no tmp-cleaner unlinks it and +/// `rmdir` refuses a non-empty directory, so the inode a held lock refers to +/// cannot be swapped out underneath a second participant. +/// +/// Falls back to the file's own parent when the file does not exist yet (first +/// boot, before anything is written or shared) — there is no committed record +/// to lose in that window. +pub fn store_txn_lock_dir(store_file: &std::path::Path) -> PathBuf { + if let Ok(real) = std::fs::canonicalize(store_file) { + if let Some(parent) = real.parent() { + return parent.to_path_buf(); + } } + store_file + .parent() + .map(std::path::Path::to_path_buf) + .unwrap_or_else(|| store_file.to_path_buf()) } /// Acquire an exclusive advisory file lock for the blob identified by `service`. @@ -271,42 +285,143 @@ impl SecretStore { static INSTANCE: OnceLock = OnceLock::new(); INSTANCE.get_or_init(|| SecretStore::keyring(service)) } +} - /// Acquire the cross-process transaction lock for this store's service and - /// hold it for the returned guard's lifetime. - /// - /// This is the coarse lock every multi-step secret transaction must hold - /// so two Desktop processes sharing the store cannot interleave: a save - /// holds it from generation-write through the atomic JSON commit; GC holds - /// it from the live-ref read through the blob `remove_batch`. Without it, - /// process B's GC could delete a generation that process A wrote but has - /// not yet committed to JSON, leaving A's committed ref dangling. - /// - /// The guard is `#[must_use]` — dropping it early releases the lock. On a - /// build without the keyring feature this is a no-op guard. - #[must_use = "the transaction lock is released when the guard is dropped"] - pub fn transaction_lock(&self) -> Result { - #[cfg(feature = "system-keyring")] - { - let path = blob_txn_lockfile_path(&self.service); - Ok(SecretTxnGuard { - _inner: BlobLockGuard::acquire(&path)?, - }) +/// Acquire the cross-process secret **transaction** lock on the store +/// directory identified by `store_dir` (the resolved canonical directory from +/// [`store_txn_lock_dir`]) and hold it for the returned guard's lifetime. +/// +/// This is the coarse lock every multi-step secret transaction must hold so +/// two Desktop processes sharing the store cannot interleave: a save holds it +/// from the other-half read through generation writes to the atomic JSON +/// commit; GC holds it from the live-ref read through the blob `remove_batch`. +/// Without it, process B's GC could delete a generation that process A wrote +/// but has not yet committed to JSON, leaving A's committed ref dangling; or A +/// could commit a stale pre-lock snapshot over B's committed record. +/// +/// The lock target is the store **directory inode**, not a `/tmp` lockfile and +/// not the keyring service — see [`store_txn_lock_dir`] for why (stable shared +/// identity across worktrees + immunity to the unlink/recreate split). +/// +/// Orthogonal to the per-operation `mutate_blob` lock, which flocks a separate +/// `/tmp` lockfile ([`blob_lockfile_path`]): a `mutate_blob` inside a +/// transaction takes a lock on a *different* object, so the two never +/// self-deadlock. Transaction callers must not nest this lock within +/// themselves (a second acquire in the same process blocks on its own held +/// exclusive lock); the save/GC/global-config entry points are all leaf-level. +/// +/// The guard is `#[must_use]` — dropping it early releases the lock. On a +/// build without the keyring feature this is a no-op guard. +#[cfg(feature = "system-keyring")] +#[must_use = "the transaction lock is released when the guard is dropped"] +pub fn transaction_lock_at(store_dir: &std::path::Path) -> Result { + #[cfg(unix)] + { + use std::os::unix::io::AsRawFd; + // Open the directory read-only (never create/truncate — the store dir + // already exists, created by `managed_agents_base_dir`). `flock` on the + // directory inode blocks until the lock is acquired; no file inside can + // split it because a non-empty directory cannot be `rmdir`'d and its + // inode is fixed for the directory's lifetime. + let dir = std::fs::File::open(store_dir) + .map_err(|e| format!("txn lock open dir {}: {e}", store_dir.display()))?; + let ret = unsafe { libc::flock(dir.as_raw_fd(), libc::LOCK_EX) }; + if ret != 0 { + let err = std::io::Error::last_os_error(); + return Err(format!("txn lock flock {}: {err}", store_dir.display())); } - #[cfg(not(feature = "system-keyring"))] - { - Ok(SecretTxnGuard {}) + Ok(SecretTxnGuard { _dir: dir }) + } + #[cfg(windows)] + { + // Windows cannot `flock` a directory handle the same way, so use a + // named kernel mutex whose name is a deterministic hash of the + // resolved directory path — the same stable-identity property as the + // Unix directory inode, and cross-build stable so a signed build and a + // dev build sharing the store contend on one mutex. + let name_str = format!("Local\\BuzzSecretTxn-{:016x}", fnv1a64(store_dir)); + let name_wide: Vec = name_str + .encode_utf16() + .chain(std::iter::once(0u16)) + .collect(); + use windows_sys::Win32::Foundation::WAIT_OBJECT_0; + use windows_sys::Win32::Security::SECURITY_ATTRIBUTES; + use windows_sys::Win32::System::Threading::{CreateMutexW, WaitForSingleObject, INFINITE}; + let handle = unsafe { + CreateMutexW( + std::ptr::null::(), + 0, + name_wide.as_ptr(), + ) + }; + if handle.is_null() { + let err = std::io::Error::last_os_error(); + return Err(format!("txn lock CreateMutexW: {err}")); } + let wait_result = unsafe { WaitForSingleObject(handle, INFINITE) }; + if wait_result != WAIT_OBJECT_0 + && wait_result != windows_sys::Win32::Foundation::WAIT_ABANDONED + { + let err = std::io::Error::last_os_error(); + unsafe { windows_sys::Win32::Foundation::CloseHandle(handle) }; + return Err(format!( + "txn lock WaitForSingleObject: {wait_result} / {err}" + )); + } + Ok(SecretTxnGuard { + mutex_handle: handle, + }) + } + #[cfg(not(any(unix, windows)))] + { + let _ = store_dir; + Err("txn lock: unsupported platform".to_string()) } } +/// No-op transaction lock when the keyring feature is disabled: there are no +/// generation writes to serialize, so the guard holds nothing. +#[cfg(not(feature = "system-keyring"))] +#[must_use = "the transaction lock is released when the guard is dropped"] +pub fn transaction_lock_at(_store_dir: &std::path::Path) -> Result { + Ok(SecretTxnGuard {}) +} + +/// Deterministic 64-bit FNV-1a over a path's bytes, used only to name the +/// Windows transaction mutex. Stable across builds (no random seed), so two +/// Desktop builds sharing a store derive the same mutex name. +#[cfg(all(feature = "system-keyring", windows))] +fn fnv1a64(path: &std::path::Path) -> u64 { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + for byte in path.to_string_lossy().as_bytes() { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(0x0000_0100_0000_01b3); + } + hash +} + /// RAII guard for the cross-process secret transaction lock. Held for the full -/// duration of a save (generation-write → JSON commit) or a GC pass (live-ref -/// read → blob remove). See [`SecretStore::transaction_lock`]. +/// duration of a save (other-half read → generation writes → JSON commit) or a +/// GC pass (live-ref read → blob remove). See [`transaction_lock_at`]. #[must_use = "the transaction lock is released when the guard is dropped"] pub struct SecretTxnGuard { - #[cfg(feature = "system-keyring")] - _inner: BlobLockGuard, + /// The open directory fd. Never read — held purely for RAII: closing it + /// releases the `flock(LOCK_EX)` on the directory inode. + #[cfg(all(feature = "system-keyring", unix))] + #[allow(dead_code)] + _dir: std::fs::File, + #[cfg(all(feature = "system-keyring", windows))] + mutex_handle: windows_sys::Win32::Foundation::HANDLE, +} + +#[cfg(all(feature = "system-keyring", windows))] +impl Drop for SecretTxnGuard { + fn drop(&mut self) { + unsafe { + windows_sys::Win32::System::Threading::ReleaseMutex(self.mutex_handle); + windows_sys::Win32::Foundation::CloseHandle(self.mutex_handle); + } + } } /// Whether a keyring error string indicates the backend itself is unavailable diff --git a/desktop/src-tauri/src/secret_store_tests.rs b/desktop/src-tauri/src/secret_store_tests.rs index 81ac4ae1f6..b3de9cae8b 100644 --- a/desktop/src-tauri/src/secret_store_tests.rs +++ b/desktop/src-tauri/src/secret_store_tests.rs @@ -186,53 +186,90 @@ fn test_blob_lock_acquire_and_release() { ); } -// ── F5a: cross-process transaction lock ─────────────────────────────── +// ── F5a: cross-process transaction lock (directory-inode based) ─────── #[test] -fn test_txn_lockfile_path_is_distinct_from_op_lockfile() { - // The transaction lock and the per-operation `mutate_blob` lock MUST be - // separate files. A save/GC transaction holds the txn lock while its - // inner `mutate_blob` calls take the op lock; if they were the same - // file, the second flock in the same process would self-deadlock. - let op = blob_lockfile_path("buzz-desktop"); - let txn = blob_txn_lockfile_path("buzz-desktop"); - assert_ne!(op, txn, "txn and op lockfiles must be distinct paths"); - assert!( - txn.file_name() - .and_then(|n| n.to_str()) - .is_some_and(|n| n.contains("txn")), - "txn lockfile {txn:?} must be identifiable as the transaction lock" +fn test_store_txn_lock_dir_resolves_symlinked_file_to_canonical_parent() { + // Two dev worktrees each symlink `managed-agents.json` to one canonical + // file (the parent dir is NOT symlinked — see `SHARED_AGENT_FILES`). The + // transaction lock must key by the RESOLVED canonical directory so both + // worktrees contend on the same inode; keying by the worktree's own path + // would give them different locks over the same store. + let root = std::env::temp_dir().join(format!("buzz-txn-key-{}", std::process::id())); + let canonical = root.join("canonical/agents"); + std::fs::create_dir_all(&canonical).expect("create canonical agents dir"); + let real_file = canonical.join("managed-agents.json"); + std::fs::write(&real_file, b"[]").expect("write canonical store"); + + let mut resolved = Vec::new(); + for wt in ["wtA", "wtB"] { + let wt_dir = root.join(wt).join("agents"); + std::fs::create_dir_all(&wt_dir).expect("create worktree agents dir"); + let link = wt_dir.join("managed-agents.json"); + let _ = std::fs::remove_file(&link); + #[cfg(unix)] + std::os::unix::fs::symlink(&real_file, &link).expect("symlink store file"); + resolved.push(store_txn_lock_dir(&link)); + } + + #[cfg(unix)] + { + let want = std::fs::canonicalize(&canonical).expect("canonicalize canonical dir"); + assert_eq!( + resolved[0], want, + "worktree A must resolve to canonical dir" + ); + assert_eq!( + resolved[1], want, + "worktree B must resolve to canonical dir" + ); + assert_eq!( + resolved[0], resolved[1], + "both worktrees must key the transaction lock by ONE canonical dir inode" + ); + } + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn test_store_txn_lock_dir_falls_back_to_parent_when_file_absent() { + // First boot: the store file does not exist yet. The lock dir must still + // resolve to the file's parent so the very first save serializes (there is + // no committed record to lose in that window, but the API must not panic + // or hand back a nonsense path). + let dir = std::env::temp_dir().join(format!("buzz-txn-absent-{}/agents", std::process::id())); + let absent = dir.join("managed-agents.json"); + assert_eq!( + store_txn_lock_dir(&absent), + dir, + "an absent store file must key by its parent directory" ); } -/// Two INDEPENDENT participants (distinct `BlobLockGuard`s on the same -/// service — the analog of two Desktop processes contending on the same -/// on-disk lockfile) must mutually exclude: while one holds the +/// Two INDEPENDENT participants (distinct guards on the same resolved store +/// directory — the analog of two Desktop processes contending on one shared +/// canonical `agents/` dir) must mutually exclude: while one holds the /// transaction lock, a non-blocking acquire by the other must fail. /// -/// Uses the real `flock`/named-mutex path over a unique temp service so it -/// needs no OS keychain and does not touch the shared store. -#[cfg(unix)] +/// Uses the real `flock` path over a unique temp directory so it needs no OS +/// keychain and does not touch the shared store. +#[cfg(all(unix, feature = "system-keyring"))] #[test] fn test_txn_lock_excludes_a_second_participant() { use std::os::unix::io::AsRawFd; - let path = blob_txn_lockfile_path("buzz-test-txn-exclusion"); - let _ = std::fs::remove_file(&path); + let dir = std::env::temp_dir().join(format!("buzz-txn-excl-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create store dir"); + std::fs::write(dir.join("managed-agents.json"), b"[]").expect("seed store file"); - // Participant A takes the transaction lock and holds it. - let guard_a = BlobLockGuard::acquire(&path).expect("participant A acquires txn lock"); + // Participant A takes the transaction lock on the directory inode. + let guard_a = transaction_lock_at(&dir).expect("participant A acquires txn lock"); // Participant B is an independent open file description on the SAME - // lockfile (what a second process would have). A non-blocking exclusive + // directory (what a second process would have). A non-blocking exclusive // flock must fail with EWOULDBLOCK while A holds the lock. - let file_b = std::fs::OpenOptions::new() - .create(true) - .truncate(false) - .write(true) - .open(&path) - .expect("participant B opens lockfile"); - let rc = unsafe { libc::flock(file_b.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; + let dir_b = std::fs::File::open(&dir).expect("participant B opens dir"); + let rc = unsafe { libc::flock(dir_b.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; assert_eq!( rc, -1, "a second participant must NOT acquire the txn lock while A holds it" @@ -245,10 +282,56 @@ fn test_txn_lock_excludes_a_second_participant() { // After A releases, B's non-blocking acquire succeeds. drop(guard_a); - let rc2 = unsafe { libc::flock(file_b.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; + let rc2 = unsafe { libc::flock(dir_b.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; assert_eq!(rc2, 0, "second participant must acquire once A releases"); - let _ = unsafe { libc::flock(file_b.as_raw_fd(), libc::LOCK_UN) }; - let _ = std::fs::remove_file(&path); + let _ = unsafe { libc::flock(dir_b.as_raw_fd(), libc::LOCK_UN) }; + let _ = std::fs::remove_dir_all(&dir); +} + +/// Finding 3 adversarial regression: the lock must survive a tmp-cleaner-style +/// unlink+recreate of a FILE inside the store directory. The old `/tmp` +/// lockfile split here — A held an flock on an inode, the pathname was +/// unlinked and recreated, and B locked a fresh inode with both "holding" the +/// lock. Because the transaction lock is on the DIRECTORY inode (not a file +/// inside it), churning files within cannot swap the locked inode: B must +/// still be excluded while A holds. +#[cfg(all(unix, feature = "system-keyring"))] +#[test] +fn test_txn_lock_survives_unlink_recreate_of_file_inside() { + use std::os::unix::io::AsRawFd; + + let dir = std::env::temp_dir().join(format!("buzz-txn-unlink-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create store dir"); + let store_file = dir.join("managed-agents.json"); + std::fs::write(&store_file, b"[]").expect("seed store file"); + + // A holds the directory-inode transaction lock. + let guard_a = transaction_lock_at(&dir).expect("participant A acquires txn lock"); + + // A tmp-cleaner unlinks and recreates the store file (fresh file inode). + // The DIRECTORY inode is unchanged — it is non-empty and cannot be rmdir'd. + std::fs::remove_file(&store_file).expect("unlink store file"); + std::fs::write(&store_file, b"[]").expect("recreate store file"); + + // B (independent fd on the same directory) must STILL be excluded — the + // exact failure the /tmp-lockfile design could not prevent. + let dir_b = std::fs::File::open(&dir).expect("participant B opens dir"); + let rc = unsafe { libc::flock(dir_b.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; + assert_eq!( + rc, -1, + "B must NOT acquire after an unlink/recreate of a file inside the locked dir" + ); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EWOULDBLOCK), + "exclusion must hold across the file churn" + ); + + drop(guard_a); + let rc2 = unsafe { libc::flock(dir_b.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; + assert_eq!(rc2, 0, "B acquires once A releases"); + let _ = unsafe { libc::flock(dir_b.as_raw_fd(), libc::LOCK_UN) }; + let _ = std::fs::remove_dir_all(&dir); } #[ignore = "requires real OS keychain (run locally)"]