From 1e369e9ac28456fb1c97a88442d96d348ce57a41 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 17 Aug 2026 16:45:35 -0400 Subject: [PATCH] =?UTF-8?q?feat(managed-agents):=20add=20=C2=A72.5=20recov?= =?UTF-8?q?ery=20reap=20for=20unreferenced=20orphans?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mint orchestrator leaves a durable orphan row whenever a crash lands before the atomic step-4 commit; nothing reclaimed the dangling keyring secret. Add reap_unreferenced_orphans: for each orphan with no live binding, delete its keyring entry THEN drop its journal row, persisting the trimmed document once. Delete-before-drop and drop-only-on-success make the sweep idempotent across recovery points, and a backend delete failure keeps that row for a later retry. store_all only merges, so a new KeyStore::delete seam (delegating to SecretStore::delete, absent-entry = Ok) is required; the fakes mirror that contract. Also folds in Paul's read-back-miss mint crash point: load returning None after a verified write yields Err, no binding, and a surviving orphan row for the reap. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src-tauri/src/managed_agents/library.rs | 57 +++++- .../managed_agents/library/binding_tests.rs | 191 +++++++++++++++++- .../src-tauri/src/managed_agents/storage.rs | 8 + .../src/managed_agents/storage_tests.rs | 8 + 4 files changed, 260 insertions(+), 4 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/library.rs b/desktop/src-tauri/src/managed_agents/library.rs index 38ef46f9a..592ee8493 100644 --- a/desktop/src-tauri/src/managed_agents/library.rs +++ b/desktop/src-tauri/src/managed_agents/library.rs @@ -818,8 +818,8 @@ impl LibraryDocument { /// once reaped, the caller drops the row via [`remove_orphan_pubkey`]. Scans /// RAW entries for a live binding to the pubkey (binding only — a /// `deferred_archives` marker is not a live binding and must not keep an - /// uncommitted orphan alive). Pure selection; the keyring delete is the IO - /// half performed by the recovery point (Phase 4b). + /// uncommitted orphan alive). Pure selection; [`reap_unreferenced_orphans`] + /// performs the keyring delete + journal drop at the recovery point. pub fn unreferenced_orphans(&self) -> Vec<&str> { self.orphan_keys .iter() @@ -829,6 +829,59 @@ impl LibraryDocument { } } +/// Reap every unreferenced orphan at a §4 recovery point (§2.5 step 3): for each +/// [`LibraryDocument::unreferenced_orphans`] pubkey, delete its keyring entry +/// THEN drop its journal row, and durably `persist` the trimmed document once. +/// +/// Ordering is the crash-safety guarantee: the keyring `delete` precedes the +/// journal-row drop, and the row is only dropped for a pubkey whose delete +/// SUCCEEDED. A crash after a delete but before `persist` leaves the row intact, +/// so the next recovery point re-deletes (a no-op on the already-absent entry — +/// [`KeyStore::delete`] treats absent as success) and drops the row then; the +/// reap is idempotent. A backend `delete` failure keeps that pubkey's row for a +/// later retry and surfaces as `Err` AFTER the successful drops are persisted, +/// so partial progress is never lost. Referenced (committed) orphans are never +/// touched — their key backs a live binding. +/// +/// `persist` and [`KeyStore`] are injected so the ordering is unit-testable at +/// each crash point without real IO; production passes the live secret store and +/// `|doc| save_library_document(base_dir, doc)`. +pub(crate) fn reap_unreferenced_orphans( + document: &mut LibraryDocument, + store: &impl KeyStore, + persist: impl FnOnce(&LibraryDocument) -> Result<(), String>, +) -> Result<(), String> { + let targets: Vec = document + .unreferenced_orphans() + .into_iter() + .map(str::to_string) + .collect(); + + let mut failures = Vec::new(); + for pubkey in &targets { + match store.delete(&agent_keyring_name(pubkey)) { + // Key gone (or already absent) — safe to drop the coordinate. + Ok(()) => document.remove_orphan_pubkey(pubkey), + // Backend failure: keep the row so a later recovery point retries. + Err(e) => failures.push(format!("{pubkey}: {e}")), + } + } + + // Durably record the successful drops before reporting any failure, so a + // partial reap never repeats work it already completed. + persist(document)?; + + if failures.is_empty() { + Ok(()) + } else { + Err(format!( + "reap left {} orphan(s) for retry: {}", + failures.len(), + failures.join("; ") + )) + } +} + #[cfg(test)] mod tests; diff --git a/desktop/src-tauri/src/managed_agents/library/binding_tests.rs b/desktop/src-tauri/src/managed_agents/library/binding_tests.rs index 9153202c2..4526ab566 100644 --- a/desktop/src-tauri/src/managed_agents/library/binding_tests.rs +++ b/desktop/src-tauri/src/managed_agents/library/binding_tests.rs @@ -277,7 +277,13 @@ fn test_unreferenced_orphans_ignores_deferred_only_reference() { struct FakeKeyStore { stored: RefCell>, fail_write: bool, + fail_delete: bool, + /// `load` returns `Ok(None)` even after a verified write — models a keyring + /// where the minted key vanishes between write-back and read-back (§2.5 + /// step 4's "minted key absent immediately after verified write" arm). + load_misses: bool, writes: RefCell, + deletes: RefCell, } impl FakeKeyStore { @@ -285,16 +291,36 @@ impl FakeKeyStore { Self { stored: RefCell::new(HashMap::new()), fail_write: false, + fail_delete: false, + load_misses: false, writes: RefCell::new(0), + deletes: RefCell::new(0), } } fn write_fails() -> Self { Self { - stored: RefCell::new(HashMap::new()), fail_write: true, - writes: RefCell::new(0), + ..Self::ok() } } + fn delete_fails() -> Self { + Self { + fail_delete: true, + ..Self::ok() + } + } + fn load_misses() -> Self { + Self { + load_misses: true, + ..Self::ok() + } + } + fn with_key(self, name: &str, value: &str) -> Self { + self.stored + .borrow_mut() + .insert(name.to_string(), value.to_string()); + self + } } impl crate::managed_agents::storage::KeyStore for FakeKeyStore { @@ -302,6 +328,9 @@ impl crate::managed_agents::storage::KeyStore for FakeKeyStore { KeyringProbe::ReachableButEmpty } fn load(&self, name: &str) -> Result, String> { + if self.load_misses { + return Ok(None); + } Ok(self.stored.borrow().get(name).cloned()) } fn load_all_readonly(&self) -> Result>, String> { @@ -324,6 +353,14 @@ impl crate::managed_agents::storage::KeyStore for FakeKeyStore { .extend(entries.iter().map(|(k, v)| (k.clone(), v.clone()))); Ok(()) } + fn delete(&self, name: &str) -> Result<(), String> { + *self.deletes.borrow_mut() += 1; + if self.fail_delete { + return Err("keyring backend unreachable".to_string()); + } + self.stored.borrow_mut().remove(name); + Ok(()) + } } #[test] @@ -453,3 +490,153 @@ fn test_mint_crash_at_keyring_write_leaves_durable_orphan_for_reap() { .collect::>() ); } + +#[test] +fn test_mint_crash_at_read_back_leaves_durable_orphan_no_binding() { + // Crash point in step 4: the keyring write verified, but the immediate + // read-back misses (key vanished between write and load). No binding is + // produced, the error surfaces, and the durable orphan row (step 2) survives + // so the dangling key is reaped at the next recovery point. + let owner = Keys::generate(); + let store = FakeKeyStore::load_misses(); + let mut doc = library(vec![]); + let mut persisted: Option = None; + + let err = mint_bound_identity(&mut doc, &owner, &store, |d| { + persisted = Some(d.clone()); + Ok(()) + }) + .expect_err("read-back miss propagates"); + + assert!(err.contains("absent from keyring")); + assert_eq!( + *store.writes.borrow(), + 1, + "keyring write verified before load" + ); + // The durable snapshot carries the orphan coordinate, unreferenced — the reap + // selects it exactly as at the keyring-write crash point. + let durable = persisted.expect("journal was persisted before the keyring write"); + assert_eq!(durable.orphan_keys.len(), 1); + assert_eq!( + durable.unreferenced_orphans(), + durable + .orphan_keys + .iter() + .map(String::as_str) + .collect::>() + ); +} + +// ── reap_unreferenced_orphans: §4 recovery sweep (§2.5 step 3) ─────────────────── + +#[test] +fn test_reap_deletes_uncommitted_keys_and_drops_rows_leaving_committed() { + // Two orphans journaled: one committed (a live binding references it), one + // dangling. The sweep deletes only the dangling key and drops only its row; + // the committed key and row survive untouched. + let committed = "aa".repeat(32); + let dangling = "bb".repeat(32); + let store = FakeKeyStore::ok() + .with_key(&format!("agent:{committed}"), "nsec-committed") + .with_key(&format!("agent:{dangling}"), "nsec-dangling"); + let mut doc = library(vec![bound_entry(&committed, false)]); + doc.journal_orphan_pubkey(&committed); + doc.journal_orphan_pubkey(&dangling); + let saves: RefCell = RefCell::new(0); + + reap_unreferenced_orphans(&mut doc, &store, |_| { + *saves.borrow_mut() += 1; + Ok(()) + }) + .expect("reap"); + + assert_eq!( + *saves.borrow(), + 1, + "one durable write for the trimmed journal" + ); + assert_eq!(*store.deletes.borrow(), 1, "only the dangling key deleted"); + // Committed row + key survive; dangling row + key gone. + assert_eq!(doc.orphan_keys, vec![committed.clone()]); + assert!(store + .stored + .borrow() + .contains_key(&format!("agent:{committed}"))); + assert!(!store + .stored + .borrow() + .contains_key(&format!("agent:{dangling}"))); +} + +#[test] +fn test_reap_is_idempotent_across_recovery_points() { + // A second sweep after the first has nothing to reap: no deletes, the + // journal is already clean. Proves re-running a recovery point is safe. + let dangling = "cc".repeat(32); + let store = FakeKeyStore::ok().with_key(&format!("agent:{dangling}"), "nsec"); + let mut doc = library(vec![]); + doc.journal_orphan_pubkey(&dangling); + + reap_unreferenced_orphans(&mut doc, &store, |_| Ok(())).expect("first reap"); + assert!(doc.orphan_keys.is_empty()); + assert_eq!(*store.deletes.borrow(), 1); + + reap_unreferenced_orphans(&mut doc, &store, |_| Ok(())).expect("second reap"); + assert_eq!( + *store.deletes.borrow(), + 1, + "nothing left to delete on re-sweep" + ); +} + +#[test] +fn test_reap_delete_failure_keeps_rows_persists_and_errors() { + // Every keyring delete backend-fails: no row is dropped, yet persist still + // runs once (recording zero progress durably), and the sweep returns Err + // naming the retained orphans so a later recovery point retries them. + let ok_pubkey = "dd".repeat(32); + let fail_pubkey = "ee".repeat(32); + let store = FakeKeyStore::delete_fails(); + let mut doc = library(vec![]); + doc.journal_orphan_pubkey(&ok_pubkey); + doc.journal_orphan_pubkey(&fail_pubkey); + let mut persisted: Option = None; + + let err = reap_unreferenced_orphans(&mut doc, &store, |d| { + persisted = Some(d.clone()); + Ok(()) + }) + .expect_err("backend delete failure surfaces"); + + assert!(err.contains("retry")); + assert_eq!(*store.deletes.borrow(), 2, "both deletes attempted"); + // Persist ran once even though every delete failed — no row was dropped. + let durable = persisted.expect("persist ran"); + assert_eq!( + durable.orphan_keys.len(), + 2, + "failed deletes keep their rows" + ); +} + +#[test] +fn test_reap_persist_failure_propagates() { + // A durable-write failure after the keyring deletes propagates — the caller + // must know the trimmed journal did not reach disk (the next recovery point + // re-deletes harmlessly and retries the drop). + let dangling = "ff".repeat(32); + let store = FakeKeyStore::ok().with_key(&format!("agent:{dangling}"), "nsec"); + let mut doc = library(vec![]); + doc.journal_orphan_pubkey(&dangling); + + let err = reap_unreferenced_orphans(&mut doc, &store, |_| Err("disk full".to_string())) + .expect_err("persist failure propagates"); + + assert!(err.contains("disk full")); + assert_eq!( + *store.deletes.borrow(), + 1, + "delete ran before the failed persist" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index b8334d273..9ab361fb2 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -164,6 +164,11 @@ pub(crate) trait KeyStore { fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String>; /// Insert all entries from `entries` in a single blob mutation. fn store_all(&self, entries: &HashMap) -> Result<(), String>; + /// Delete the entry for `name`. Deleting an absent entry is `Ok(())` (not a + /// backend failure) — so the §2.5 recovery reap is idempotent across repeated + /// recovery points until the journal row is also dropped. `Err` only on a + /// backend failure that leaves the entry possibly still present. + fn delete(&self, name: &str) -> Result<(), String>; } impl KeyStore for SecretStore { @@ -186,6 +191,9 @@ impl KeyStore for SecretStore { fn store_all(&self, entries: &HashMap) -> Result<(), String> { SecretStore::store_all(self, entries) } + fn delete(&self, name: &str) -> Result<(), String> { + SecretStore::delete(self, name) + } } /// Outcome of attempting to lift a record's inline key into the keyring. diff --git a/desktop/src-tauri/src/managed_agents/storage_tests.rs b/desktop/src-tauri/src/managed_agents/storage_tests.rs index 631fe0dc6..62855b24b 100644 --- a/desktop/src-tauri/src/managed_agents/storage_tests.rs +++ b/desktop/src-tauri/src/managed_agents/storage_tests.rs @@ -118,6 +118,14 @@ impl KeyStore for FakeKeyStore { } Ok(()) } + fn delete(&self, name: &str) -> Result<(), String> { + if !self.reachable { + return Err("keyring backend unreachable".to_string()); + } + // Deleting an absent entry is a no-op success, matching SecretStore. + self.stored.borrow_mut().remove(name); + Ok(()) + } } fn record_with_key(nsec: &str) -> ManagedAgentRecord {