From 762a3eeb7a4d13296e641fa4831df8811dcee1d4 Mon Sep 17 00:00:00 2001 From: Duncan Date: Thu, 13 Aug 2026 01:55:23 -0400 Subject: [PATCH] fix(desktop): serialize identity persist against agent saves on the txn lock Identity and agent secrets share one SecretStore blob, but the identity persist path took no transaction lock, so an identity keyring write could interleave with a concurrent agent save/GC projection transaction on the same blob. Acquire the secret txn lock at the AppHandle-bearing entry points (import_identity, persist_current_identity, pairing recover), held across the persist span only. Lock order is identity_mutation -> txn -> blob on identity paths and txn -> blob on save/GC paths, so no path acquires them in opposing order. The boot path is left to single-instance serialization (lib.rs single-instance plugin). Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/commands/identity.rs | 21 +++- .../src/commands/identity_txn_lock_tests.rs | 98 +++++++++++++++++++ desktop/src-tauri/src/commands/pairing.rs | 6 ++ 3 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 desktop/src-tauri/src/commands/identity_txn_lock_tests.rs diff --git a/desktop/src-tauri/src/commands/identity.rs b/desktop/src-tauri/src/commands/identity.rs index ec2357b85e..cf02c05d1a 100644 --- a/desktop/src-tauri/src/commands/identity.rs +++ b/desktop/src-tauri/src/commands/identity.rs @@ -362,6 +362,14 @@ pub async fn import_identity( let key_path = data_dir.join("identity.key"); let (pubkey, storage) = commit_imported_identity(&state, &data_dir, keys, |keys| { + // Serialize the shared-keyring blob write against a concurrent + // agent save/GC, which hold the same transaction lock across their + // full span: identity and agent secrets share one SecretStore, so + // an unserialized identity persist could interleave with a + // projection transaction on the same blob. Held across the persist + // span only — the in-memory key swap that follows this closure + // touches no keyring blob. + let _txn = crate::managed_agents::storage::acquire_secret_txn_lock(&app_handle)?; // Persist into the OS keyring first (store → read-back verify → // marker → delete file). Falls back to the 0o600 file when the // keyring is unavailable; returns Err only when both backends fail. @@ -494,8 +502,13 @@ pub async fn persist_current_identity( let key_path = data_dir.join("identity.key"); let store = crate::secret_store::SecretStore::shared(crate::app_state::keyring_service()); - let storage = - crate::app_state::persist_imported_identity(store, &keys, &key_path, &data_dir)?; + let storage = { + // Serialize the shared-keyring blob write against a concurrent agent + // save/GC on the same SecretStore. Scoped to the persist span only — + // the identity_lost clear that follows touches no keyring blob. + let _txn = crate::managed_agents::storage::acquire_secret_txn_lock(&app_handle)?; + crate::app_state::persist_imported_identity(store, &keys, &key_path, &data_dir)? + }; // Keys are already the live identity. Record where the durable write // landed before clearing identity_lost. @@ -788,3 +801,7 @@ mod nostr_identity_binding_tests { #[cfg(test)] #[path = "identity_key_backup_tests.rs"] mod identity_key_backup_tests; + +#[cfg(test)] +#[path = "identity_txn_lock_tests.rs"] +mod identity_txn_lock_tests; diff --git a/desktop/src-tauri/src/commands/identity_txn_lock_tests.rs b/desktop/src-tauri/src/commands/identity_txn_lock_tests.rs new file mode 100644 index 0000000000..a691cfda6d --- /dev/null +++ b/desktop/src-tauri/src/commands/identity_txn_lock_tests.rs @@ -0,0 +1,98 @@ +//! W6-A interleave coverage. +//! +//! The identity-persist entry points (`import_identity`, +//! `persist_current_identity`, and the pairing recover path) now hold the same +//! cross-process secret transaction lock that a concurrent agent save/GC holds, +//! so an identity keyring write can no longer interleave with a projection +//! transaction on the shared `SecretStore` blob. +//! +//! Both real callers reach the lock through +//! `acquire_secret_txn_lock`, which resolves it as +//! `transaction_lock_at(store_txn_lock_dir(store_file))`. Driving the Tauri +//! commands themselves needs a live `AppHandle` (integration-only); the unit +//! level below proves the property that matters — that the identity-persist +//! span and an agent-save span derive one lock target from the shared store +//! file and mutually exclude on it. + +/// While the identity-persist span holds the transaction lock, a concurrent +/// agent-save projection span (an independent open file description, exactly +/// what a second process or a parallel save has) must be excluded, and may +/// acquire only after the persist span releases. +/// +/// Deterministic by handshake: the persist span holds the lock until the save +/// span has probed exclusion and explicitly hands back — no timing sleeps. The +/// `released` flag is set before the guard drops, so whenever the blocking +/// acquire returns it is guaranteed to observe it, proving the save span could +/// only proceed after release. +#[cfg(all(unix, feature = "system-keyring"))] +#[test] +fn test_identity_persist_span_excludes_concurrent_agent_save_on_shared_txn_lock() { + use crate::secret_store::{store_txn_lock_dir, transaction_lock_at}; + use std::os::unix::io::AsRawFd; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::mpsc; + use std::sync::Arc; + + // A store directory shared by identity and agent secrets — the same file + // `managed_agents_store_path` yields and `acquire_secret_txn_lock` resolves. + let dir = std::env::temp_dir().join(format!("buzz-w6a-interleave-{}", 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"); + + // The crux of W6-A: both spans derive the lock target from the store FILE + // via `store_txn_lock_dir`, so identity persist and agent save contend on + // one directory inode instead of racing on the shared keyring blob. + let lock_dir = store_txn_lock_dir(&store_file); + + let released = Arc::new(AtomicBool::new(false)); + let (held_tx, held_rx) = mpsc::channel(); + let (go_tx, go_rx) = mpsc::channel(); + + // Identity-persist span: hold the txn lock across its keyring write, then + // release only once the save span has verified exclusion. + let persist_dir = lock_dir.clone(); + let persist_released = Arc::clone(&released); + let persist = std::thread::spawn(move || { + let guard = transaction_lock_at(&persist_dir).expect("identity persist acquires txn lock"); + held_tx.send(()).expect("signal lock held"); + go_rx.recv().expect("save span verified exclusion"); + // Set BEFORE releasing: the waiter can only unblock once this drops, so + // it is guaranteed to observe `true`. + persist_released.store(true, Ordering::SeqCst); + drop(guard); + }); + + held_rx + .recv() + .expect("identity persist reported the lock held"); + + // Agent-save projection span: a non-blocking acquire on an independent OFD + // must fail while identity persist holds the lock. + let probe = std::fs::File::open(&lock_dir).expect("agent save opens store dir"); + let rc = unsafe { libc::flock(probe.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; + assert_eq!( + rc, -1, + "an agent save must NOT acquire the txn lock while identity persist holds it" + ); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EWOULDBLOCK), + "exclusion must report EWOULDBLOCK" + ); + drop(probe); + + // Hand back: persist releases, and the save span acquires through the real + // blocking API. It returns only after release — proven by `released`. + go_tx.send(()).expect("release identity persist span"); + let save_guard = + transaction_lock_at(&lock_dir).expect("agent save acquires txn lock after release"); + assert!( + released.load(Ordering::SeqCst), + "agent save acquired the lock only after the identity persist span released it" + ); + drop(save_guard); + + persist.join().expect("identity persist thread"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/desktop/src-tauri/src/commands/pairing.rs b/desktop/src-tauri/src/commands/pairing.rs index aedd67854c..6feb885eb5 100644 --- a/desktop/src-tauri/src/commands/pairing.rs +++ b/desktop/src-tauri/src/commands/pairing.rs @@ -482,6 +482,12 @@ async fn import_recovered_identity( std::fs::create_dir_all(&data_dir).map_err(|e| format!("create app data dir: {e}"))?; let key_path = data_dir.join("identity.key"); crate::commands::identity::commit_imported_identity(&state, &data_dir, keys, |keys| { + // Serialize the shared-keyring blob write against a concurrent + // agent save/GC on the same SecretStore, held across the persist + // span only. Lock order on this path is identity_mutation → + // pairing generation_fence → txn; no save/GC path touches either + // of the first two, so there is no inversion. + let _txn = crate::managed_agents::storage::acquire_secret_txn_lock(&app)?; let store = crate::secret_store::SecretStore::shared(crate::app_state::keyring_service()); crate::app_state::persist_imported_identity(store, keys, &key_path, &data_dir)