jerry/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-10-31 14:31:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(desktop): harden keyring secret projection fail-closed + cross-process txn lock

Browse Source 
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:<coord> 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 <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
Duncan
2026-08-11 15:02:33 -04:00
parent fe2b5dfc75
commit 06f2ce9a15
9 changed files with 671 additions and 135 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
desktop/src-tauri/src/commands/personas/update.rs
View File

@@ -198,8 +198,9 @@ pub(super) async fn update_persona_with<R: Send + 'static>(
// `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,

290
desktop/src-tauri/src/managed_agents/dev_service_migration.rs
View File

@@ -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<String, String>,
dest_map: &std::collections::HashMap<String, String>,
global_refs: &std::collections::HashSet<String>,
live_coords: Option<&std::collections::HashSet<String>>,
) -> 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<String> = 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<std::collections::HashSet<String>> {
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<String> {
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<String> = ["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<String> = [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<String> = 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:<coord>` 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<HashMap<String, String>>);
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<Option<String>, String> {
Ok(self.0.borrow().get(key).cloned())
}
fn load_all(&self) -> Result<Option<HashMap<String, String>>, String> {
Ok(Some(self.0.borrow().clone()))
}
fn store_batch(&self, entries: &HashMap<String, String>) -> 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:<coord>` 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"
);
}
}

8
desktop/src-tauri/src/managed_agents/global_config/mod.rs
View File

@@ -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 {

36
desktop/src-tauri/src/managed_agents/secret_projection.rs
View File

@@ -258,16 +258,32 @@ pub fn load_secret<S: ProjectionStore>(
};
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)),

61
desktop/src-tauri/src/managed_agents/secret_projection_tests.rs
View File

@@ -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<Option<String>, 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<Option<HashMap<String, String>>, String> {
unreachable!("load_secret never calls load_all")
}
fn store_batch(&self, _entries: &HashMap<String, String>) -> 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 {

36
desktop/src-tauri/src/managed_agents/storage.rs
View File

@@ -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<Option<crate::secret_store::SecretTxnGuard>, 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.

4
desktop/src-tauri/src/migration/migration_secrets.rs
View File

@@ -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!(

213
desktop/src-tauri/src/secret_store.rs
View File

@@ -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<SecretStore> = 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<SecretTxnGuard, String> {
#[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<SecretTxnGuard, String> {
#[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<u16> = 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::<SECURITY_ATTRIBUTES>(),
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<SecretTxnGuard, String> {
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

153
desktop/src-tauri/src/secret_store_tests.rs
View File

@@ -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)"]