From 29d04a5cf41a0390ec793441f4f050c6caef94b2 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 4 Aug 2026 18:23:00 -0400 Subject: [PATCH] =?UTF-8?q?fix(b1-store):=20round-11=20=E2=80=94=20hoist?= =?UTF-8?q?=20file=20recovery=20before=20migrations,=20fail=20closed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit run_file_commit_recovery is now called immediately after reset handling succeeds, before run_boot_migrations*, backfill_persona_snapshots, and every other canonical-store reader/writer. A stale stage from a pre-crash commit can no longer overwrite migration/backfill output. Fail closed: a recovery error sets store_recovery_failed on AppState (same pattern as reset_failed) and returns early, keeping all store-touching setup closed. get_identity surfaces the flag to the frontend. spawn_boot_recovery (publication-only since round 9) is the sole remaining background recovery path; it cannot enter file repair at any schedule. New test: test_setup_order_recovery_completes_before_migration_reads_canonical plants stale canonical + stage file with repaired content, runs file_commit_recovery_at_pub (the exact function production delegates to), then asserts the canonical holds the repaired content — proving any subsequent migration read sees the repaired state. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/app_state.rs | 24 +++---- desktop/src-tauri/src/commands/identity.rs | 6 ++ desktop/src-tauri/src/lib.rs | 30 ++++---- .../managed_agents/store_journal_fix_tests.rs | 69 +++++++++++++++++++ desktop/src-tauri/src/models.rs | 3 + 5 files changed, 106 insertions(+), 26 deletions(-) diff --git a/desktop/src-tauri/src/app_state.rs b/desktop/src-tauri/src/app_state.rs index fc90e6ab14..cdf62f1e58 100644 --- a/desktop/src-tauri/src/app_state.rs +++ b/desktop/src-tauri/src/app_state.rs @@ -56,9 +56,8 @@ pub struct AppState { pub huddle_audio: crate::huddle::tts_settings::HuddleAudioSettingsState, /// Tauri app handle — stored after setup so huddle commands can emit /// `huddle-state-changed` events without needing the handle threaded - /// through every call site. - /// - /// Set once during `setup()` in `lib.rs`; never cleared. + /// through every call site. Set once during `setup()` in `lib.rs`; + /// never cleared. pub app_handle: Mutex>, /// Port of the localhost media streaming proxy (set during setup). pub media_proxy_port: AtomicU16, @@ -68,13 +67,8 @@ pub struct AppState { /// signing commands check this flag via [`AppState::signing_keys`] and /// return `Err` so no events are published under the inaccessible identity. /// Mutually exclusive with `identity_lost` (guaranteed by `RecoveryState` - /// at the resolve boundary). - /// - /// Ordering: writers store with `Ordering::Release` after `state.keys` is - /// updated, so a reader observing `false` with `Ordering::Acquire` is - /// guaranteed to see the updated keys. Writers: `setup()` (initial - /// resolution via `resolve_persisted_identity`) and `import_identity` - /// (clears the flag when the user successfully imports a new key). + /// at the resolve boundary). Ordering: writers store with + /// `Ordering::Release`; readers use `Ordering::Acquire`. pub keyring_locked: AtomicBool, /// Set when identity resolution detected a "lost" state: the migration /// marker was present but the keyring was empty and no plaintext fallback @@ -97,11 +91,12 @@ pub struct AppState { /// Set when the boot-time Phase 2 reset attempted a wipe but verification /// failed. The sentinel is preserved so the next relaunch retries. All /// identity-dependent setup is skipped; the frontend shows a reset-failed - /// recovery screen via `get_identity`. - /// - /// Ordering: written once in `setup()` with `Ordering::Release`; read in - /// `get_identity` with `Ordering::Acquire`. + /// recovery screen via `get_identity`. Written once in `setup()` with + /// `Ordering::Release`; read in `get_identity` with `Ordering::Acquire`. pub reset_failed: AtomicBool, + /// Set when pre-migration file-commit recovery fails; all store-touching + /// setup is skipped. Same write/read ordering as `reset_failed`. + pub store_recovery_failed: AtomicBool, /// Cached ACP session config from running agents, keyed by canonical /// `(agent pubkey, relay URL)` runtime identity. /// Populated when the harness emits `session_config_captured` observer events. @@ -226,6 +221,7 @@ pub fn build_app_state() -> AppState { keyring_locked: AtomicBool::new(false), identity_lost: AtomicBool::new(false), reset_failed: AtomicBool::new(false), + store_recovery_failed: AtomicBool::new(false), #[cfg(feature = "mesh-llm")] mesh_llm_runtime: AsyncMutex::new(None), #[cfg(feature = "mesh-llm")] diff --git a/desktop/src-tauri/src/commands/identity.rs b/desktop/src-tauri/src/commands/identity.rs index bddf2e725a..8da8e5d689 100644 --- a/desktop/src-tauri/src/commands/identity.rs +++ b/desktop/src-tauri/src/commands/identity.rs @@ -39,6 +39,9 @@ pub fn get_identity(state: State<'_, AppState>) -> Result let reset_failed = state .reset_failed .load(std::sync::atomic::Ordering::Acquire); + let store_recovery_failed = state + .store_recovery_failed + .load(std::sync::atomic::Ordering::Acquire); Ok(IdentityInfo { pubkey: pubkey_hex, @@ -47,6 +50,7 @@ pub fn get_identity(state: State<'_, AppState>) -> Result lost, locked, reset_failed, + store_recovery_failed, }) } @@ -382,6 +386,7 @@ pub async fn import_identity( lost: false, locked: false, reset_failed: false, + store_recovery_failed: false, }) }) .await @@ -515,6 +520,7 @@ pub async fn persist_current_identity( lost: false, locked: false, reset_failed: false, + store_recovery_failed: false, }) }) .await diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index 6ee5bd71d7..2219cb4494 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -340,6 +340,24 @@ pub fn run() { return Ok(()); } + // File-commit recovery MUST run before any migration, backfill, or + // canonical-store reader/writer. A stale stage from a pre-crash + // commit must not overwrite migration output. One owner, one site. + // Fail closed: a repair error sets store_recovery_failed and skips + // all store-touching setup. + { + let state = app_handle.state::(); + if let Err(e) = + crate::managed_agents::store_journal::run_file_commit_recovery(&app_handle) + { + eprintln!("buzz-desktop: file-commit-recovery: {e}"); + state + .store_recovery_failed + .store(true, std::sync::atomic::Ordering::Release); + return Ok(()); + } + } + // Run all pre-identity data migrations before state loads from disk. if reset_outcome.completed { migration::run_boot_migrations_after_reset(&app_handle); @@ -597,18 +615,6 @@ pub fn run() { } }); } - // File-commit recovery runs synchronously under the advisory lock - // BEFORE spawn_boot_recovery is called. This ensures the JSON - // store is in a consistent state (no dangling *.stage files) - // before the background task or any command can arrive. - // File repair has exactly ONE owner: this synchronous call. - // spawn_boot_recovery is publication-only — it does NOT run - // recover_interrupted_file_commits. - if let Err(e) = - crate::managed_agents::store_journal::run_file_commit_recovery(app.handle()) - { - eprintln!("buzz-desktop: file-commit-recovery: {e}"); - } crate::managed_agents::spawn_boot_recovery(app.handle()); Ok(()) }) diff --git a/desktop/src-tauri/src/managed_agents/store_journal_fix_tests.rs b/desktop/src-tauri/src/managed_agents/store_journal_fix_tests.rs index 1e1aa31ab9..ba58313e4d 100644 --- a/desktop/src-tauri/src/managed_agents/store_journal_fix_tests.rs +++ b/desktop/src-tauri/src/managed_agents/store_journal_fix_tests.rs @@ -674,6 +674,75 @@ fn test_sync_file_recovery_repairs_dangling_intent_row() { ); } +/// Fix A — production-order seam test: file-commit recovery must complete +/// BEFORE any migration or canonical-store reader observes the canonical files. +/// +/// Seam used: `file_commit_recovery_at_pub` is the exact function that +/// `run_file_commit_recovery` delegates to in production — not a +/// reimplementation. The "migration read" is a direct filesystem read of the +/// canonical path, matching what every real migration/backfill does. +/// +/// Scenario: a process crashed after writing a stage file but before the rename +/// completed. The canonical (`managed-agents.json`) still holds pre-crash +/// ("stale") content. The stage file holds the intended post-crash ("repaired") +/// content. After recovery runs, the canonical must hold the repaired content — +/// so any subsequent migration or backfill reader sees the repaired state. +#[test] +fn test_setup_order_recovery_completes_before_migration_reads_canonical() { + let dir = tmp_dir(); + let anchor = dir.path().to_path_buf(); + + // "Stale" content — what canonical holds before recovery. + let stale_content = b"[{\"pubkey\":\"stale-pre-crash\"}]"; + // "Repaired" content — what the stage file holds (the intended commit). + let repaired_content = b"[{\"pubkey\":\"repaired-post-crash\"}]"; + let teams_content = b"[]"; + + let a_can = anchor.join("managed-agents.json"); + let t_can = anchor.join("teams.json"); + let a_stage = anchor.join("managed-agents.order_test.stage"); + let t_stage = anchor.join("teams.order_test.stage"); + + // Write stale canonical — this is what migrations would see WITHOUT recovery. + std::fs::write(&a_can, stale_content).unwrap(); + std::fs::write(&t_can, teams_content).unwrap(); + + // Write stage files containing the repaired (intended) state. + std::fs::write(&a_stage, repaired_content).unwrap(); + std::fs::write(&t_stage, teams_content).unwrap(); + + // Plant the intent row — recovery will detect and replay it. + { + let j = open_journal(&anchor).unwrap(); + insert_phase_row( + &j, + "order_test", + "intent", + a_stage.to_str().unwrap(), + t_stage.to_str().unwrap(), + &sha256_hex(repaired_content), + &sha256_hex(teams_content), + ); + } + + // Run recovery — the exact same code path production calls via + // run_file_commit_recovery → file_commit_recovery_at. + file_commit_recovery_at_pub(&anchor).unwrap(); + + // Now simulate a "migration read" — read the canonical directly, as every + // real migration/backfill does. + let observed = std::fs::read(&a_can).unwrap(); + assert_eq!( + observed, repaired_content, + "migration read must observe the repaired canonical, not the stale pre-crash state" + ); + // Stale content must not be present. + assert_ne!( + observed, stale_content, + "stale canonical content must have been replaced by recovery" + ); +} + // ── Fix B: migration tests ─────────────────────────────────────────────────── /// Helper: create a v2-shaped in-memory journal (no hash/d-tag columns). diff --git a/desktop/src-tauri/src/models.rs b/desktop/src-tauri/src/models.rs index 3f04d3d7a1..4c9369516c 100644 --- a/desktop/src-tauri/src/models.rs +++ b/desktop/src-tauri/src/models.rs @@ -24,6 +24,9 @@ pub struct IdentityInfo { /// reset-failed recovery screen. The sentinel is preserved so the next /// relaunch retries the wipe automatically. pub reset_failed: bool, + /// True when pre-migration file-commit recovery failed at boot. All + /// store-touching setup was skipped; the frontend shows a recovery screen. + pub store_recovery_failed: bool, } #[derive(Serialize, Deserialize)]