diff --git a/desktop/src-tauri/src/commands/personas/sharing.rs b/desktop/src-tauri/src/commands/personas/sharing.rs index 5980e6f20c..4b2db1a900 100644 --- a/desktop/src-tauri/src/commands/personas/sharing.rs +++ b/desktop/src-tauri/src/commands/personas/sharing.rs @@ -12,6 +12,16 @@ use crate::{ use super::pending::{prepare_persona_publication, PreparedPersonaPublication}; +/// Test-only observer called immediately after the `managed_agents_store_lock` +/// is acquired in `publish_and_refresh_teams_at`'s refresh section. Tests use +/// this to assert `try_lock()` fails — proving the lock is held during the +/// synchronous refresh. Moving the lock acquisition to AFTER the refresh call +/// (recreating the TOCTOU race) causes `try_lock()` to succeed, turning the +/// probe test RED. +#[cfg(test)] +pub(crate) static REFRESH_LOCK_OBSERVER: std::sync::Mutex>> = + std::sync::Mutex::new(None); + #[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize)] #[serde(rename_all = "kebab-case")] pub enum PersonaSharePublicationStatus { @@ -116,9 +126,34 @@ pub(crate) async fn publish_and_refresh_teams_at( persona_id: &str, ) -> Result { let result = publish_prepared_persona(state, prepared).await?; - // F2: refresh any shared 30178 heads that include this persona, matching the - // contract of `update_persona_and_publish`. Best-effort — failures are logged. - let _ = crate::commands::teams::refresh_for_persona_at(base_dir, keys, db_path, persona_id); + // F2: refresh any shared 30178 heads that include this persona. The refresh + // reads the current team/persona definitions and may retain a new head — it + // must be serialized with team edit/unshare/delete operations that also hold + // `managed_agents_store_lock` and may retain/retract the same head. + // + // Without the lock a concurrent `set_team_shared(false)` can: + // 1. acquire the lock, retain unshared head T+1, release the lock; + // 2. refresh (unlocked) reads old shared head T, rebuilds, retains T+1; + // `retain_event` accepts equal timestamps — the refresh wins, undoing + // the explicit unshare. + // + // Acquire AFTER the network await so the lock is never held across I/O; + // the synchronous refresh completes entirely inside the critical section. + { + let _guard = state + .managed_agents_store_lock + .lock() + .map_err(|e| format!("managed_agents_store_lock poisoned: {e}"))?; + #[cfg(test)] + { + if let Ok(obs) = REFRESH_LOCK_OBSERVER.lock() { + if let Some(ref f) = *obs { + f(state); + } + } + } + let _ = crate::commands::teams::refresh_for_persona_at(base_dir, keys, db_path, persona_id); + } Ok(result) } @@ -421,4 +456,68 @@ mod tests { assert!(error.contains("failed to open retention db")); } + + /// Lock-scope regression for P1-2: the synchronous refresh inside + /// `publish_and_refresh_teams_at` must hold `managed_agents_store_lock` + /// for its entire read+retain sequence so it is serialized with concurrent + /// team edits, unshare, and delete operations. + /// + /// The observer fires after the lock is acquired and asserts `try_lock()` + /// fails — proving the lock is held. Moving the lock acquisition to AFTER + /// the refresh call (removing the lock scope) causes `try_lock()` to + /// succeed, turning this test RED. + #[tokio::test] + async fn test_retry_refresh_holds_store_lock_during_refresh() { + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::Arc; + + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("retention.db"); + let keys = nostr::Keys::generate(); + let relay_url = spawn_relay(true).await; + let prepared = prepared(&db_path, relay_url, keys.clone(), Some(true)); + let state = build_app_state(); + + // Write an empty teams.json so refresh_for_persona_at reads zero teams + // (no 30178 to retract). This keeps the test focused on the lock scope. + std::fs::write(dir.path().join("teams.json"), b"[]").unwrap(); + + let observer_fired = Arc::new(AtomicBool::new(false)); + let observer_fired_clone = observer_fired.clone(); + { + let mut slot = REFRESH_LOCK_OBSERVER.lock().expect("observer slot"); + *slot = Some(Box::new(move |state: &AppState| { + assert!( + state.managed_agents_store_lock.try_lock().is_err(), + "managed_agents_store_lock must be held during the refresh — \ + a successful try_lock means the refresh runs outside the lock, \ + recreating the concurrent-unshare race condition" + ); + observer_fired_clone.store(true, Ordering::SeqCst); + })); + } + + let persona_id = prepared.persona.id.clone(); + let result = publish_and_refresh_teams_at( + &state, + prepared, + dir.path(), + &keys, + &db_path, + &persona_id, + ) + .await; + + // Clear observer before any assertion that could panic. + { + let mut slot = REFRESH_LOCK_OBSERVER.lock().expect("observer slot"); + *slot = None; + } + + result.expect("publish_and_refresh_teams_at must succeed"); + assert!( + observer_fired.load(Ordering::SeqCst), + "the refresh lock observer must have fired — if not, the hook is not wired" + ); + } } diff --git a/desktop/src-tauri/src/commands/personas/update.rs b/desktop/src-tauri/src/commands/personas/update.rs index 8c31689986..6e653adfeb 100644 --- a/desktop/src-tauri/src/commands/personas/update.rs +++ b/desktop/src-tauri/src/commands/personas/update.rs @@ -209,7 +209,15 @@ pub(super) async fn update_persona_with std::env::remove_var("XDG_DATA_HOME"), } } + +/// P2 regression: linked managed-agent name and avatar propagation must +/// complete even when the retain/publish callback returns an error. +/// +/// Before the fix: `retain(&app, &state, &result)?` propagated the error +/// immediately, skipping the avatar/name propagation block and `save_managed_agents`. +/// A publish-only retry (`set_persona_shared`) then published the persona but +/// the linked instance still carried the old name/avatar. +/// +/// After the fix: `retain_result` is captured without `?`, all linked +/// persistence runs to completion, and THEN the retain error is propagated. +/// +/// Mutation acceptance: restoring `retain(&app, &state, &result)?` before +/// the avatar/name propagation block causes `save_managed_agents` to never run +/// and the assertion on the stored agent name turns RED. +#[tokio::test] +async fn linked_instance_rename_completes_before_retain_error_propagates() { + let temp = tempfile::tempdir().unwrap(); + let home = temp.path().join("home2"); + std::fs::create_dir_all(&home).unwrap(); + + let old_home = std::env::var_os("HOME"); + let old_xdg = std::env::var_os("XDG_DATA_HOME"); + { + let _path_guard = crate::managed_agents::lock_path_mutex(); + std::env::set_var("HOME", &home); + std::env::set_var("XDG_DATA_HOME", &home); + } + + let app = mock_app(); + + // Seed persona "Alice" at R1. + save_personas(app.handle(), &[persona("p1", "Alice", R1)]).expect("seed must succeed"); + + // Seed a linked agent whose name matches the persona's display_name. + let agent_record = crate::managed_agents::ManagedAgentRecord { + pubkey: "pk-alice-p2".to_string(), + name: "Alice".to_string(), + persona_id: Some("p1".to_string()), + private_key_nsec: String::new(), + auth_tag: None, + relay_url: String::new(), + avatar_url: None, + acp_command: String::new(), + agent_command: String::new(), + agent_command_override: None, + agent_args: vec![], + mcp_command: String::new(), + turn_timeout_seconds: 0, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: None, + model: None, + provider: None, + persona_source_version: None, + env_vars: std::collections::BTreeMap::new(), + start_on_app_launch: false, + auto_restart_on_config_change: false, + runtime_pid: None, + backend: Default::default(), + backend_agent_id: None, + provider_policy_pending: false, + provider_binary_path: None, + team_id: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: String::new(), + updated_at: String::new(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + last_error_code: None, + respond_to: Default::default(), + respond_to_allowlist: vec![], + display_name: Some("Alice".to_string()), + slug: None, + runtime: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + shared: false, + source_team: None, + source_team_persona_slug: None, + catalog_source: None, + team_catalog_source: None, + definition_respond_to: None, + definition_respond_to_allowlist: vec![], + definition_parallelism: None, + relay_mesh: None, + effort_level: None, + }; + crate::managed_agents::save_managed_agents(app.handle(), &[agent_record]) + .expect("agent seed must succeed"); + + // Submit a rename with a retain callback that always fails — simulates a + // strict publication/enqueue failure AFTER save_personas ran. + let result = update_persona_with( + UpdatePersonaRequest { + id: "p1".to_string(), + display_name: "Alice Renamed".to_string(), + avatar_url: None, + system_prompt: "Do the work.".to_string(), + runtime: None, + model: None, + provider: None, + name_pool: Vec::new(), + env_vars: None, + behavior: None, + expected_updated_at: Some(R1.to_string()), + }, + app.handle().clone(), + |_app, _state, _persona| -> Result<(), String> { + Err("simulated publish failure after persona persisted".to_string()) + }, + ) + .await; + + // The retain error must propagate so the coordinator sees publishFailed. + assert!( + result.is_err(), + "update_persona_with must return Err when retain fails; \ + if this is Ok the retain error was swallowed" + ); + + // The linked agent must have been renamed BEFORE the error propagated. + // If save_managed_agents was skipped (the pre-fix path), the name is still "Alice". + let agents = crate::managed_agents::load_managed_agents(app.handle()) + .expect("agent reload must succeed"); + let stored_agent = agents + .iter() + .find(|a| a.pubkey == "pk-alice-p2") + .expect("linked agent must still exist"); + + assert_eq!( + stored_agent.name, "Alice Renamed", + "linked instance name must be propagated before the retain error is returned; \ + restoring `retain()?` before the propagation block turns this RED" + ); + + // Cleanup + std::env::remove_var("HOME"); + std::env::remove_var("XDG_DATA_HOME"); + match old_home { + Some(v) => std::env::set_var("HOME", v), + None => std::env::remove_var("HOME"), + } + match old_xdg { + Some(v) => std::env::set_var("XDG_DATA_HOME", v), + None => std::env::remove_var("XDG_DATA_HOME"), + } +} diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs index 93abbed8ff..3393ecc030 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs @@ -1864,3 +1864,256 @@ test("test_no_publish_retry_seam_shows_terminal_warning_not_reopen", async () => cap.restore(); } }); + +// ── Test family 12: P1 — combined saves (D+I, D+L) bypass recovery ───────────── +// +// When `publishCatalogUpdates` is true and the command throws after persist, +// `publishFailed` is set and `firstError` suppresses I/L writes. A combined +// D+I or D+L save must settle publication independently — before advancing — +// so the full D+I or D+L path can complete when the retry succeeds, and must +// name the unpublished catalog alongside the unsaved I/L remainder when it +// fails. +// +// Mutation acceptance: restoring the original `!observedRemainder` gate must +// turn these tests RED because the retry is never called for combined saves. + +test("test_combined_di_save_publish_retry_succeeds_continues_instance_write", async () => { + // D+I combined save: publish throws after persist, retry succeeds — I write + // must proceed and the whole save must close as full success. + const cap = captureToasts(); + try { + const persistedDef = makeDefinition({ displayName: "Alice" }); + const persistedInst = makeInstance({ name: "Alice" }); + let retryCalls = 0; + let agentCalls = 0; + const opts = makeOpts({ + ctx: { + kind: "instance-with-definition", + definition: makeDefinition({ displayName: "Alice" }), + instance: makeInstance(), + }, + personaInput: makePersonaInput({ displayName: "Alice" }), + agentInput: makeAgentInput({ name: "Alice" }), + publishCatalogUpdates: true, + updatePersonaAndPublish: async () => { + throw new Error("failed to open retention db"); + }, + updateManagedAgent: async () => { + agentCalls++; + return { agent: persistedInst, profileSyncError: null }; + }, + refetchStores: async () => ({ + persona: persistedDef, + agent: persistedInst, + }), + publishRetry: async () => { + retryCalls++; + return { persona: persistedDef, publicationStatus: "published" }; + }, + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal( + result, + true, + "full success when retry + I write both succeed", + ); + assert.equal( + opts._calls.onDone, + 1, + "onDone must be called on full success", + ); + assert.equal( + retryCalls, + 1, + "publishRetry must be called once for the early retry", + ); + assert.equal( + agentCalls, + 1, + "instance write must proceed after retry success", + ); + const warnings = cap.captured.filter((c) => c.kind === "warning"); + assert.equal(warnings.length, 0, "no warning when everything succeeds"); + const successes = cap.captured.filter((c) => c.kind === "success"); + assert.equal(successes.length, 1, "success toast must fire"); + } finally { + cap.restore(); + } +}); + +test("test_combined_dl_save_publish_retry_succeeds_continues_policy_write", async () => { + // D+L combined save: publish throws after persist, retry succeeds — policy + // write must proceed and the whole save must close as full success. + const cap = captureToasts(); + try { + const persistedDef = makeDefinition({ displayName: "Alice" }); + const persistedInst = makeInstance({ autoRestartOnConfigChange: true }); + let retryCalls = 0; + let policyCalls = 0; + const opts = makeOpts({ + ctx: { + kind: "instance-with-definition", + definition: makeDefinition({ displayName: "Alice" }), + instance: makeInstance(), + }, + personaInput: makePersonaInput({ displayName: "Alice" }), + agentInput: null, + policySets: [{ type: "autoRestart", pubkey: "pk-abc", value: true }], + publishCatalogUpdates: true, + updatePersonaAndPublish: async () => { + throw new Error("failed to open retention db"); + }, + setAutoRestart: async () => { + policyCalls++; + }, + refetchStores: async () => ({ + persona: persistedDef, + agent: persistedInst, + }), + publishRetry: async () => { + retryCalls++; + return { persona: persistedDef, publicationStatus: "published" }; + }, + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal( + result, + true, + "full success when retry + policy write both succeed", + ); + assert.equal( + opts._calls.onDone, + 1, + "onDone must be called on full success", + ); + assert.equal(retryCalls, 1, "publishRetry must be called once"); + assert.equal( + policyCalls, + 1, + "policy write must proceed after retry success", + ); + const warnings = cap.captured.filter((c) => c.kind === "warning"); + assert.equal(warnings.length, 0, "no warning when everything succeeds"); + } finally { + cap.restore(); + } +}); + +test("test_combined_di_save_publish_retry_failure_names_both_catalog_and_remainder", async () => { + // D+I combined save: publish throws, retry also fails — toast must name + // the profile as saved, instance settings as not saved, and catalog failure + // as the reason. Never claim reopen can republish the catalog. + const cap = captureToasts(); + try { + const persistedDef = makeDefinition({ displayName: "Alice" }); + const opts = makeOpts({ + ctx: { + kind: "instance-with-definition", + definition: makeDefinition({ displayName: "Alice" }), + instance: makeInstance(), + }, + personaInput: makePersonaInput({ displayName: "Alice" }), + agentInput: makeAgentInput({ name: "Alice-renamed" }), + publishCatalogUpdates: true, + updatePersonaAndPublish: async () => { + throw new Error("relay unreachable"); + }, + updateManagedAgent: async () => { + throw new Error("should not be called"); + }, + refetchStores: async () => ({ persona: persistedDef, agent: null }), + publishRetry: async () => { + throw new Error("still unreachable"); + }, + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal(result, false, "combined failure returns false"); + assert.equal(opts._calls.onDone, 0, "onDone must not be called"); + // Instance write must NOT have been attempted. + assert.equal( + opts._calls.updateManagedAgent, + 0, + "instance write must be skipped", + ); + const warnings = cap.captured.filter((c) => c.kind === "warning"); + assert.equal(warnings.length, 1, "exactly one warning"); + // Must name D as saved and publication failure. + assert.match( + warnings[0].message, + /profile saved/i, + "warning must name profile as saved", + ); + assert.match( + warnings[0].message, + /catalog publication failed|could not be published/i, + "warning must name the publication failure", + ); + // Must name the I/L remainder as not saved. + assert.match( + warnings[0].message, + /instance settings/i, + "warning must name instance settings as the unsaved remainder", + ); + } finally { + cap.restore(); + } +}); + +test("test_combined_di_save_no_retry_seam_names_both_catalog_and_remainder", async () => { + // D+I combined save: publish throws, no retry seam provided — toast must + // still name catalog failure + unsaved I/L remainder without a publishRetry. + const cap = captureToasts(); + try { + const persistedDef = makeDefinition({ displayName: "Alice" }); + const opts = makeOpts({ + ctx: { + kind: "instance-with-definition", + definition: makeDefinition({ displayName: "Alice" }), + instance: makeInstance(), + }, + personaInput: makePersonaInput({ displayName: "Alice" }), + agentInput: makeAgentInput({ name: "Alice-renamed" }), + publishCatalogUpdates: true, + updatePersonaAndPublish: async () => { + throw new Error("relay unreachable"); + }, + refetchStores: async () => ({ persona: persistedDef, agent: null }), + // publishRetry intentionally omitted. + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal(result, false, "combined failure without seam returns false"); + assert.equal(opts._calls.onDone, 0, "onDone must not be called"); + assert.equal( + opts._calls.updateManagedAgent, + 0, + "instance write must be skipped", + ); + const warnings = cap.captured.filter((c) => c.kind === "warning"); + assert.equal(warnings.length, 1, "exactly one warning"); + assert.match( + warnings[0].message, + /profile saved/i, + "warning must name profile as saved", + ); + assert.match( + warnings[0].message, + /catalog publication failed|could not be published/i, + "warning must name the catalog failure", + ); + assert.match( + warnings[0].message, + /instance settings/i, + "warning must name instance settings as the unsaved remainder", + ); + } finally { + cap.restore(); + } +}); diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.ts b/desktop/src/features/agents/ui/agentSaveCoordinator.ts index c023fb223c..df129f5a1b 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.ts +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.ts @@ -291,7 +291,47 @@ export async function runAgentSaveCoordinator( // reached. Mark this so final settlement does not close as full success: // "save and publish" promised a relay outcome that was not delivered. publishFailed = true; - firstError = caughtError; + // For combined D+I or D+L saves, settle publication independently now — + // before advancing to I/L writes — so a transient preparation failure + // does not permanently bypass recovery. If retry succeeds, clear + // publishFailed/firstError and proceed normally. If retry fails (or no + // seam), set firstError to block I/L advancement: the partial-failure + // toast will name both the unsaved I/L remainder and the unpublished + // catalog, since a fresh reopen seeds persisted values with no + // personaInput, meaning publication is never re-attempted. + const hasPendingIL = agentInput !== null || policySets.length > 0; + if (hasPendingIL) { + // Combined D+I or D+L save: settle publication before advancing to I/L. + if (publishRetry && def) { + let earlyRetryError: string | null = null; + try { + const earlyRetryResult = await publishRetry(def.id); + publicationStatus = earlyRetryResult.publicationStatus; + // Retry succeeded — publication settled. Clear both flags so I/L + // writes can continue and the final success branch fires. + publishFailed = false; + firstError = null; + } catch (err) { + earlyRetryError = + err instanceof Error ? err.message : "unknown error"; + } + if (publishFailed) { + // Retry also failed — block I/L advancement and carry the reason + // as firstError. The partial-failure toast will name the profile as + // saved and the I/L remainder as not saved, with the catalog error + // as the failure reason — reopen/retry is accurate for the I/L + // remainder; the D change IS kept, catalog publication is not. + firstError = `catalog publication failed: ${earlyRetryError ?? caughtError ?? "unknown error"}`; + } + } else { + // No retry seam — block I/L advancement for the same reason. + firstError = `catalog publication failed: ${caughtError ?? "unknown error"}`; + } + } else { + // Definition-only save: the final `!observedRemainder && publishFailed` + // block handles publication retry after all settlement. + firstError = caughtError; + } } }