From ade79bf89a545671a90db6cf297dbbfc4fb3351f Mon Sep 17 00:00:00 2001 From: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@sprout-oss.stage.blox.sqprod.co> Date: Sat, 18 Jul 2026 19:58:42 -0400 Subject: [PATCH] fix(desktop): durable session-scope migration and rollback convergence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings from the PR #1981 session-scope setting: Legacy materialization (Mari, MAJOR 1): hydration read the legacy acp_top_level_sessions override but never persisted the translation, so removing the compatibility reader would silently flip explicit channel users to the thread default. hydrate_scope now atomically materializes an explicit legacy value into acp_session_scope during hydration — false→channel, true→thread — while the bare no-field default stays unwritten (unset remains distinguishable from an override) and an explicit new-field value is never rewritten. DesktopSettings gains a #[serde(flatten)] passthrough so any save preserves JSON fields owned by other features. A failed materialization write keeps the translated scope in memory and the legacy field on disk for retry — no divergence. Rollback convergence (Mari, MAJOR 2): on apply failure with a failed rollback write, the old path restarted processes and forced the UI to 'previous' while the authoritative backend still held the new value — false convergence. applyAcpSessionScopeSetting now establishes the authoritative scope first (confirmed rollback write, else re-read via getBackend), reconciles processes and UI to that actual value, and if the authority is unreadable fires onUnrecoverable — the card surfaces a hard recovery state and disables the toggle instead of claiming a scope. Tests: 7 new Tauri hydration/materialization tests (incl. read-only-dir write-failure injection) and 2 new rollback-matrix tests (authority re-read reconciliation; double-failure hard recovery). Co-authored-by: Tyler Longwell Signed-off-by: Tyler Longwell --- desktop/src-tauri/src/commands/experiments.rs | 144 +++++++++++++++++- .../ui/AcpSessionScopeSettingsCard.tsx | 14 +- .../ui/acpSessionScopeSetting.test.mjs | 82 ++++++++++ .../settings/ui/acpSessionScopeSetting.ts | 32 +++- 4 files changed, 263 insertions(+), 9 deletions(-) diff --git a/desktop/src-tauri/src/commands/experiments.rs b/desktop/src-tauri/src/commands/experiments.rs index e7c971d3f..5094087a4 100644 --- a/desktop/src-tauri/src/commands/experiments.rs +++ b/desktop/src-tauri/src/commands/experiments.rs @@ -43,6 +43,9 @@ struct DesktopSettings { // have had a release cycle to persist `acp_session_scope`. #[serde(skip_serializing)] acp_top_level_sessions: Option, + // Fields owned by other features must survive our writes untouched. + #[serde(flatten)] + extra: serde_json::Map, } impl DesktopSettings { @@ -97,10 +100,8 @@ fn save_settings(path: &Path, settings: &DesktopSettings) -> Result<(), String> pub(crate) fn acp_session_scope(app: &AppHandle) -> AcpSessionScope { HYDRATE.call_once( - || match settings_path(app).and_then(|path| load_settings(&path)) { - Ok(settings) => { - ACP_SESSION_SCOPE.store(settings.session_scope().atomic_value(), Ordering::Release) - } + || match settings_path(app).and_then(|path| hydrate_scope(&path)) { + Ok(scope) => ACP_SESSION_SCOPE.store(scope.atomic_value(), Ordering::Release), Err(error) => { eprintln!("buzz-desktop: failed to hydrate desktop settings: {error}"); } @@ -112,6 +113,29 @@ pub(crate) fn acp_session_scope(app: &AppHandle) -> AcpSessionScope { } } +/// Load the persisted scope, materializing an explicit legacy override into +/// the durable `acp_session_scope` field so the compatibility reader can be +/// removed without flipping untouched legacy installs to the default. +/// +/// The ordinary no-field default is deliberately NOT written: unset must stay +/// distinguishable from a user/legacy override. A failed materialization +/// write keeps the translated scope in memory and leaves the legacy field on +/// disk, so the next launch retries — no divergence, no corruption. +fn hydrate_scope(path: &Path) -> Result { + let mut settings = load_settings(path)?; + let scope = settings.session_scope(); + if settings.acp_session_scope.is_none() && settings.acp_top_level_sessions.is_some() { + settings.acp_session_scope = Some(scope); + if let Err(error) = save_settings(path, &settings) { + eprintln!( + "buzz-desktop: failed to materialize legacy ACP session scope \ + (kept in memory; legacy field retained on disk for retry): {error}" + ); + } + } + Ok(scope) +} + #[tauri::command] pub fn get_acp_session_scope(app: AppHandle) -> AcpSessionScope { acp_session_scope(&app) @@ -130,7 +154,7 @@ pub fn set_acp_session_scope(scope: AcpSessionScope, app: AppHandle) -> Result<( #[cfg(test)] mod tests { - use super::{load_settings, save_settings, AcpSessionScope, DesktopSettings}; + use super::{hydrate_scope, load_settings, save_settings, AcpSessionScope, DesktopSettings}; #[test] fn missing_store_defaults_to_thread_scope() { @@ -148,6 +172,7 @@ mod tests { &DesktopSettings { acp_session_scope: Some(AcpSessionScope::Channel), acp_top_level_sessions: None, + extra: Default::default(), }, ) .unwrap(); @@ -175,4 +200,113 @@ mod tests { std::fs::write(&path, b"not json").unwrap(); assert!(load_settings(&path).is_err()); } + + #[test] + fn hydration_materializes_legacy_opt_out_as_channel() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("desktop-experiments.json"); + std::fs::write(&path, br#"{"acp_top_level_sessions":false}"#).unwrap(); + assert_eq!(hydrate_scope(&path).unwrap(), AcpSessionScope::Channel); + let persisted: serde_json::Value = + serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); + assert_eq!(persisted["acp_session_scope"], "channel"); + assert!(persisted.get("acp_top_level_sessions").is_none()); + // Restart: the durable field alone must reproduce the override. + assert_eq!(hydrate_scope(&path).unwrap(), AcpSessionScope::Channel); + } + + #[test] + fn hydration_materializes_legacy_opt_in_as_thread() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("desktop-experiments.json"); + std::fs::write(&path, br#"{"acp_top_level_sessions":true}"#).unwrap(); + assert_eq!(hydrate_scope(&path).unwrap(), AcpSessionScope::Thread); + let persisted: serde_json::Value = + serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); + assert_eq!(persisted["acp_session_scope"], "thread"); + assert!(persisted.get("acp_top_level_sessions").is_none()); + } + + #[test] + fn hydration_never_materializes_the_bare_default() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("desktop-experiments.json"); + std::fs::write(&path, br#"{"other_feature":1}"#).unwrap(); + let before = std::fs::read(&path).unwrap(); + assert_eq!(hydrate_scope(&path).unwrap(), AcpSessionScope::Thread); + // Unset must remain distinguishable from an override: no write. + assert_eq!(std::fs::read(&path).unwrap(), before); + // Nor is a missing store created. + let missing = dir.path().join("missing.json"); + assert_eq!(hydrate_scope(&missing).unwrap(), AcpSessionScope::Thread); + assert!(!missing.exists()); + } + + #[test] + fn explicit_new_field_wins_over_legacy_and_is_not_rewritten() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("desktop-experiments.json"); + std::fs::write( + &path, + br#"{"acp_session_scope":"channel","acp_top_level_sessions":true}"#, + ) + .unwrap(); + let before = std::fs::read(&path).unwrap(); + assert_eq!(hydrate_scope(&path).unwrap(), AcpSessionScope::Channel); + assert_eq!(std::fs::read(&path).unwrap(), before); + } + + #[test] + fn materialization_preserves_unrelated_fields() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("desktop-experiments.json"); + std::fs::write( + &path, + br#"{"acp_top_level_sessions":false,"other_feature":{"nested":true},"count":3}"#, + ) + .unwrap(); + assert_eq!(hydrate_scope(&path).unwrap(), AcpSessionScope::Channel); + let persisted: serde_json::Value = + serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); + assert_eq!(persisted["acp_session_scope"], "channel"); + assert_eq!(persisted["other_feature"]["nested"], true); + assert_eq!(persisted["count"], 3); + } + + #[test] + fn save_preserves_unrelated_fields_round_trip() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("desktop-experiments.json"); + std::fs::write( + &path, + br#"{"acp_session_scope":"thread","other_feature":"keep-me"}"#, + ) + .unwrap(); + let mut settings = load_settings(&path).unwrap(); + settings.acp_session_scope = Some(AcpSessionScope::Channel); + save_settings(&path, &settings).unwrap(); + let persisted: serde_json::Value = + serde_json::from_slice(&std::fs::read(&path).unwrap()).unwrap(); + assert_eq!(persisted["acp_session_scope"], "channel"); + assert_eq!(persisted["other_feature"], "keep-me"); + } + + #[cfg(unix)] + #[test] + fn failed_materialization_keeps_translated_scope_and_legacy_field() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("desktop-experiments.json"); + let legacy = br#"{"acp_top_level_sessions":false}"#; + std::fs::write(&path, legacy).unwrap(); + // Read-only directory: the atomic temp-file write must fail. + let readonly = std::fs::Permissions::from_mode(0o555); + std::fs::set_permissions(dir.path(), readonly).unwrap(); + // In memory: translated override. On disk: legacy field untouched, + // so the next launch retries the materialization. No divergence. + assert_eq!(hydrate_scope(&path).unwrap(), AcpSessionScope::Channel); + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o755)).unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), legacy); + assert_eq!(hydrate_scope(&path).unwrap(), AcpSessionScope::Channel); + } } diff --git a/desktop/src/features/settings/ui/AcpSessionScopeSettingsCard.tsx b/desktop/src/features/settings/ui/AcpSessionScopeSettingsCard.tsx index 3579533a2..1ffbe9199 100644 --- a/desktop/src/features/settings/ui/AcpSessionScopeSettingsCard.tsx +++ b/desktop/src/features/settings/ui/AcpSessionScopeSettingsCard.tsx @@ -13,6 +13,7 @@ type SessionScope = "thread" | "channel"; export function AcpSessionScopeSettingsCard() { const [scope, setScope] = useState("thread"); const [pending, setPending] = useState(true); + const [unrecoverable, setUnrecoverable] = useState(false); useEffect(() => { let cancelled = false; @@ -37,10 +38,12 @@ export function AcpSessionScopeSettingsCard() { await applyAcpSessionScopeSetting(scope === "thread", threadScoped, { setBackend: (next) => invokeTauri("set_acp_session_scope", { scope: next }), + getBackend: () => invokeTauri("get_acp_session_scope"), listAgents: listManagedAgents, stopAgent: stopManagedAgent, startAgent: startManagedAgent, setUi: (enabled) => setScope(enabled ? "thread" : "channel"), + onUnrecoverable: () => setUnrecoverable(true), }); } catch (error) { console.error("Failed to apply ACP session scope", error); @@ -64,12 +67,21 @@ export function AcpSessionScopeSettingsCard() { Run separate threads concurrently. Turn this off for one legacy session per channel.

+ {unrecoverable && ( +

+ The session scope could not be applied or restored. Restart the + app to recover a consistent state. +

+ )} void setThreadScoped(value)} /> diff --git a/desktop/src/features/settings/ui/acpSessionScopeSetting.test.mjs b/desktop/src/features/settings/ui/acpSessionScopeSetting.test.mjs index f020f64bb..992b3d1ea 100644 --- a/desktop/src/features/settings/ui/acpSessionScopeSetting.test.mjs +++ b/desktop/src/features/settings/ui/acpSessionScopeSetting.test.mjs @@ -24,10 +24,15 @@ function harness(overrides = {}) { calls, deps: { setBackend: async (scope) => calls.push(["backend", scope]), + getBackend: async () => { + calls.push(["read-backend"]); + return "thread"; + }, listAgents: async () => [localRunning, remoteRunning, localStopped], stopAgent: async (pubkey) => calls.push(["stop", pubkey]), startAgent: async (pubkey) => calls.push(["start", pubkey]), setUi: (enabled) => calls.push(["ui", enabled]), + onUnrecoverable: () => calls.push(["unrecoverable"]), ...overrides, }, }; @@ -130,4 +135,81 @@ describe("ACP session scope setting", () => { ]); assert.equal(secondStarts, 2); }); + + it("reconciles UI and processes to the re-read authoritative scope when rollback persistence fails", async () => { + let backendCalls = 0; + const { calls, deps } = harness({ + setBackend: async (scope) => { + calls.push(["backend", scope]); + backendCalls += 1; + if (backendCalls === 1) return; // apply write succeeds (thread persisted) + throw new Error("rollback persist failed"); + }, + startAgent: async (pubkey) => { + calls.push(["start", pubkey]); + if (calls.filter((c) => c[0] === "start").length === 1) + throw new Error("restart failed"); + }, + // Authoritative backend remains the NEW value (thread): the apply + // write landed and the rollback write failed. + getBackend: async () => { + calls.push(["read-backend"]); + return "thread"; + }, + }); + await assert.rejects( + applyAcpSessionScopeSetting(false, true, deps), + /restart failed/, + ); + // UI must land on the actual persisted scope (thread), never the + // assumed previous (channel), and processes reconcile after the read. + const readIndex = calls.findIndex((c) => c[0] === "read-backend"); + const uiIndex = calls.findIndex((c) => c[0] === "ui"); + assert.notEqual(readIndex, -1); + assert.deepEqual(calls[uiIndex], ["ui", true]); + assert.ok(readIndex < uiIndex, "authority read must precede UI commit"); + const restartsAfterRead = calls + .slice(readIndex) + .filter((c) => c[0] === "stop" || c[0] === "start"); + assert.deepEqual(restartsAfterRead, [ + ["stop", "local"], + ["start", "local"], + ]); + assert.ok(!calls.some((c) => c[0] === "unrecoverable")); + }); + + it("surfaces hard recovery and touches nothing when rollback and authority read both fail", async () => { + let backendCalls = 0; + const { calls, deps } = harness({ + setBackend: async (scope) => { + calls.push(["backend", scope]); + backendCalls += 1; + if (backendCalls === 1) return; + throw new Error("rollback persist failed"); + }, + startAgent: async (pubkey) => { + calls.push(["start", pubkey]); + if (calls.filter((c) => c[0] === "start").length === 1) + throw new Error("restart failed"); + }, + getBackend: async () => { + calls.push(["read-backend"]); + throw new Error("authority unreadable"); + }, + }); + await assert.rejects( + applyAcpSessionScopeSetting(false, true, deps), + /restart failed/, + ); + assert.ok(calls.some((c) => c[0] === "unrecoverable")); + // No UI claim and no process restarts under an unknown scope. + const readIndex = calls.findIndex((c) => c[0] === "read-backend"); + assert.ok(!calls.some((c) => c[0] === "ui")); + assert.deepEqual( + calls + .slice(readIndex + 1) + .filter((c) => c[0] === "stop" || c[0] === "start"), + [], + ); + }); }); diff --git a/desktop/src/features/settings/ui/acpSessionScopeSetting.ts b/desktop/src/features/settings/ui/acpSessionScopeSetting.ts index 57a004402..0e91e8ab9 100644 --- a/desktop/src/features/settings/ui/acpSessionScopeSetting.ts +++ b/desktop/src/features/settings/ui/acpSessionScopeSetting.ts @@ -6,10 +6,14 @@ export type SessionScopeAgent = { export type SessionScopeDependencies = { setBackend: (scope: "thread" | "channel") => Promise; + getBackend: () => Promise<"thread" | "channel">; listAgents: () => Promise; stopAgent: (pubkey: string) => Promise; startAgent: (pubkey: string) => Promise; setUi: (threadScoped: boolean) => void; + /** The authoritative backend scope could not be restored or read: the UI + * must surface a hard recovery state instead of claiming any scope. */ + onUnrecoverable: () => void; }; async function restartRunningLocalAgents( @@ -25,8 +29,13 @@ async function restartRunningLocalAgents( /** * Apply the Rust-owned session-scope setting and restart affected processes. The UI is - * committed only after every restart succeeds. On failure, both persisted - * backend state and already-restarted agents are restored best-effort. + * committed only after every restart succeeds. + * + * Failure invariant: persisted setting, Rust in-memory value, UI, and affected + * processes must converge on one authoritative scope. Rollback is only claimed + * after the rollback write is confirmed; if that write fails, the authoritative + * value is re-read and UI/processes reconcile to it. If the authority cannot be + * read either, `onUnrecoverable` fires and nothing pretends to know the scope. */ export async function applyAcpSessionScopeSetting( previous: boolean, @@ -39,14 +48,31 @@ export async function applyAcpSessionScopeSetting( await restartRunningLocalAgents(agents, deps); deps.setUi(next); } catch (error) { + // Establish the authoritative backend scope before touching UI or + // processes: preferably by restoring `previous`, otherwise by reading + // what actually persisted. + let authoritative: boolean; try { await deps.setBackend(previous ? "thread" : "channel"); + authoritative = previous; } catch (rollbackError) { console.error( "Failed to roll back ACP session-scope backend state", rollbackError, ); + try { + authoritative = (await deps.getBackend()) === "thread"; + } catch (readError) { + console.error( + "Failed to read authoritative ACP session scope after rollback failure", + readError, + ); + deps.onUnrecoverable(); + throw error; + } } + // Reconcile every affected process under the confirmed authoritative + // scope — never under an assumed one. for (const agent of agents) { if (agent.status !== "running" || agent.backend.type !== "local") continue; @@ -60,7 +86,7 @@ export async function applyAcpSessionScopeSetting( ); } } - deps.setUi(previous); + deps.setUi(authoritative); throw error; } }