fix(desktop): cascade persona deletes and restart agents on global config save#1766
Conversation
…ith screenshot spec The delete-persona cascade and global-config restart feedback introduced two new UI states that were untested and unscreenshotted: the confirm dialog showing an agent-instance count, and the save bar showing a restart count vs. plain "Saved.". Three tests with real assertions pin these states and produce screenshots for PR #1766. Extended the set_global_agent_config mock case to honor a configurable globalConfigRestartedCount so specs can drive the restart-count text path without wiring a real agent lifecycle.
wesbillman
left a comment
There was a problem hiding this comment.
Blocking on making the cascade failure-atomic/recoverable.
delete_persona saves the persona removal and queues its tombstone before loading/locking the linked-agent cascade. It then deletes linked keys and queues agent tombstones before the final filtered save_managed_agents. If any later load, process-lock, or agent-store save fails, the command returns an error after irreversible partial deletion: linked records can remain without their persona/key, and retrying through the normal persona-delete path is no longer possible because the persona is already gone.
Please stage and validate all cascade state before committing destructive changes, then order disk-authoritative writes and side effects so an error remains safely retryable. Please also cover an injected failure after persona discovery / before the final agent save; the current helper tests only verify pubkey filtering, not the partial-failure behavior.
|
🤖 hey @wesbillman, thanks for the careful review — the cascade ordering was genuinely unsafe. I reworked
If the agent-store save fails, nothing was destroyed and a retry re-runs the full cascade; if the persona save fails, the persona survives and a retry finds an empty cascade and completes cleanly. Keys and tombstones are only touched after their records leave disk. On the injected-failure coverage: phase 3 goes through an injectable I think this covers both points, but happy to iterate if you'd order things differently. |
349bb33 to
814ff1e
Compare
…nfig save Deleting a custom persona left its managed-agent instances orphaned with persona_id=None, appearing under "Custom agents" instead of disappearing — only one UI surface papered over it with a frontend pre-cascade. Global-config saves only respawned setup-listener agents whose readiness flipped NotReady→Ready; running agents with a changed provider/model/env kept stale baked-at-spawn env silently. The save now restarts all local agents whose effective env changed and the card reports "Saved. Restarted N agent(s)." Review-round hardening: delete_persona is restructured into three phases (stage → stop → commit) so disk writes are ordered agent-store save → persona save → keychain delete → tombstones — a failure in any write leaves state intact and the command retryable, with an injectable commit_cascade_agents seam unit-testing an injected post-discovery save failure. A runtime pre-flight (collect_remote_deployed) refuses the cascade when a linked agent is provider-deployed, since create_managed_agent does not forbid that combination. failed_restart_count on GlobalAgentConfigSaveResult surfaces "M failed to restart — check the Agents tab."; restart candidates are collected via a private predicate with the personas snapshot threaded through (one load per restart instead of three). The card's handleSave snapshots the submitted config and applies the backend response only when no newer edit arrived mid-flight. E2E specs pin the cascade confirm copy (count, singular/plural, zero-instance), restart-count and failed-restart save-bar variants, and the mid-save edit race via a configurable mock delay.
814ff1e to
fbdc125
Compare
* origin/main: fix(desktop): correct provider and model handling in agent config dialogs (#1764) fix(desktop): cascade persona deletes and restart agents on global config save (#1766) fix(desktop): make doctor installs retryable with per-runtime progress and auth status (#1765) fix(codex): swap to @agentclientprotocol/codex-acp 1.x + detect outdated adapter (#1750) Fix oversized mobile Manage channel sheet (#1774) fix(desktop): show thread replies loader (#1773) fix(build): allow vendored opus to configure under CMake 4 (#1763) fix(desktop): restore multi-image mosaic galleries (#1769) Group channel membership events (#1713) Fix desktop launch motion and reaction spacing (#1717) fix(desktop): align sidebar search across themes (#1712) fix(desktop): Buzz theme flicker, white bar & accent-picker motion (#1681) fix(mobile): highlight full multi-word mentions (#1762) [BOT-1264] fix(mobile): #channel tags not tappable (#1695) feat(desktop): surface Team Instructions as distinct observer section (#1759) # Conflicts: # desktop/playwright.config.ts
* origin/main: fix(desktop): correct provider and model handling in agent config dialogs (#1764) fix(desktop): cascade persona deletes and restart agents on global config save (#1766) fix(desktop): make doctor installs retryable with per-runtime progress and auth status (#1765) fix(codex): swap to @agentclientprotocol/codex-acp 1.x + detect outdated adapter (#1750) Fix oversized mobile Manage channel sheet (#1774) fix(desktop): show thread replies loader (#1773) fix(build): allow vendored opus to configure under CMake 4 (#1763) fix(desktop): restore multi-image mosaic galleries (#1769) Group channel membership events (#1713) Fix desktop launch motion and reaction spacing (#1717) fix(desktop): align sidebar search across themes (#1712) fix(desktop): Buzz theme flicker, white bar & accent-picker motion (#1681) fix(mobile): highlight full multi-word mentions (#1762) [BOT-1264] fix(mobile): #channel tags not tappable (#1695) feat(desktop): surface Team Instructions as distinct observer section (#1759) Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com> # Conflicts: # desktop/playwright.config.ts





Fixes two agent-lifecycle issues from fresh-install feedback:
delete_personapreviously nullifiedpersona_idon linked managed agents, so deleted agents reappeared under "Custom agents" (one surface —UserProfilePanel— papered over this with a frontend-side cascade; the nav path didn't). The backend now cascades in three phases under the store lock: stage (every fallible load/lock — persona, teams, agents, process sync — happens up front with zero destructive writes, plus a pre-flight that refuses the cascade while any target is a provider-deployed agent, mirroringdelete_managed_agent'sforce_remote_deleteguard), stop (best-effort stop of running instances, with the process lock taken per agent so it is not held across the stop poll cycle), then commit (agent-store save → persona save → keychain deletes → kind:30177 tombstones — disk-authoritative writes strictly before side effects). A failure at any disk write leaves state intact and the command retryable; the injectablecommit_cascade_agentsseam is unit-tested with a failing save to prove it. The confirm dialog says "Also deletes N agent instance(s)." and the redundant frontend cascade is removed. The builtin-deactivation path keeps its own instance cleanup — builtins are deactivated rather than deleted, so the backend cascade doesn't apply there.set_global_agent_confignow restarts every running local agent whose effective env changed under the new config — including Ready→NotReady (credential removed), which restarts into setup-listener mode via the normal spawn path. The command returnsGlobalAgentConfigSaveResult { config, restarted_count, failed_restart_count }; the settings card reports "Saved. Restarted N agent(s)." and appends "M failed to restart — check the Agents tab." when a stop succeeded but the respawn failed. The card also snapshots the submitted config so an edit made during the save round-trip is never clobbered by the resolving response — the newer value survives and the card stays dirty for a follow-up save.Both behaviors are covered by a Playwright screenshot spec (
agent-lifecycle-feedback.spec.ts) driving the cascade confirm dialog (plural, singular, and zero-instance copy), all three save outcomes, and the mid-save edit race through configurable bridge mocks.