fix(desktop): delete provider-backed instances before persona cascade - #3808
Open
jbforge wants to merge 3 commits into
Open
fix(desktop): delete provider-backed instances before persona cascade#3808jbforge wants to merge 3 commits into
jbforge wants to merge 3 commits into
Conversation
jbforge
force-pushed
the
fix/persona-delete-provider-instances
branch
from
July 30, 2026 21:23
2e23513 to
dd45b88
Compare
Deleting a persona whose instances are provider-deployed always failed. `delete_persona` has no force parameter and hard-rejects the cascade when any target is remote-deployed, while `delete_managed_agent` does have the `force_remote_delete` escape hatch. Both persona-delete call sites went straight to `deletePersona` without touching instances first, so the cascade hit that rejection every time. The confirm dialog also promised the cascade unconditionally, so the user saw a promise followed by a refusal. Delete the persona's managed-agent instances first, through `deleteManagedAgentWithRules`, so the cascade finds nothing left to refuse. That routes each provider deployment through the existing orphan-warning confirm rather than force-deleting it silently. The cascade stops at the first cancelled or failed instance delete, so the persona is never removed while instances remain; instances already deleted when it stops stay deleted. The new `deleteManagedAgentsForPersonaWithRules` sits beside the per-agent function it wraps. `features/profile` keeps its own loop because its channel cleanup is interleaved per instance; both loops now carry a cross-reference comment naming the shared abort contract. The backend guard is deliberately left alone — its invariant is that a compromised IPC caller must not be able to orphan a live remote deployment. The delete dialog now calls out the provider-hosted instances in the cascade as its own sentence rather than folding them into the relay-archival one. Deleting removes the local management record and its key, tombstones the agent's relay record, and archives its relay identity; it does not tear down the provider deployment, because the protocol has no undeploy operation (deferred to v2, `commands/agents.rs:455`), so the remote infrastructure keeps running until it is removed at the provider. Fixes block#3771 Co-authored-by: jbforge <270093744+jbforge@users.noreply.github.com> Signed-off-by: jbforge <270093744+jbforge@users.noreply.github.com>
Review of the first commit caught a false claim in the copy it added. The delete dialog told the user that provider-hosted instances "will also be removed from that provider". Nothing on the delete path contacts a provider: `delete_managed_agent` stops the local process, drops the record, deletes the keyring key, then tombstones and publishes a NIP-IA archive request, and `force_remote_delete` only bypasses the backend's orphan guard. The protocol has no undeploy operation at all — `commands/agents.rs:455` defers it to v2 — so the remote deployment keeps running and keeps costing money. The claim also contradicted the orphan-warning confirm shown moments later in the same flow, which says the deployment "will keep running". A user who read the dialog, confirmed, and read past the second warning would believe their billed infrastructure was gone when it was not. The dialog now says the deployment is not torn down and keeps running until it is removed at the provider, and a regression test asserts the copy never claims removal again. Two smaller corrections from the same review: The cascade's abort contract was overstated on both loops as "never leaves a half-torn persona". Aborting stops further deletes; it cannot undo the ones that already ran. Only provider-deployed instances prompt, so a local instance visited earlier is already gone when the user declines. The comments now say that, `deleteManagedAgentsForPersonaWithRules` reports `deletedCount`, and the Agents view surfaces "Deleted N agent instance(s); kept <persona> because you cancelled" instead of leaving that destruction silent. A new test puts the aborting instance second so the partial teardown is pinned rather than hidden by test ordering. `handleDeleteInstancesForPersona` deliberately does not remove instances from their channels, unlike the profile variant; that preserves the pre-existing behaviour of the persona cascade rather than widening what this fix changes. The reason is now in the doc comment. Co-authored-by: jbforge <270093744+jbforge@users.noreply.github.com> Signed-off-by: jbforge <270093744+jbforge@users.noreply.github.com>
jbforge
force-pushed
the
fix/persona-delete-provider-instances
branch
from
July 30, 2026 21:25
dd45b88 to
a3a4b91
Compare
Review found the PR could not go green: the first commit pushed `UserProfilePanel.tsx` from 984 to 1004 lines, and the desktop file-size ratchet caps `src/features` at 1000. CI runs it — `Justfile:118-119` (`desktop-check`) invokes `pnpm check`, whose `check` script in `desktop/package.json` chains `check:file-sizes`. Repo convention is to split the file rather than raise the limit or add an override. Move the persona-delete cluster into `UserProfilePanelPersonaDelete.ts`, beside the module that already owns this component's deletion logic: the pending persona, the two instance counts the confirm dialog discloses, and the confirmed-delete handler. Only that cluster moves — a thousand-line component has other seams, but widening the split would put an unrelated refactor inside a fix PR. `UserProfilePanel.tsx` lands at 964 lines. The room that frees also closes a gap the same review found. The agents-side cascade reports a declined confirm to the user; the profile side did not, so an instance destroyed before the user cancelled vanished with no feedback on a path that toasts every other outcome. `deleteProfileManagedAgentsForPersona` now returns `deletedCount` like its sibling, and the profile call site raises "Deleted N agent instance(s); kept <persona> because you cancelled." Co-authored-by: jbforge <270093744+jbforge@users.noreply.github.com> Signed-off-by: jbforge <270093744+jbforge@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3771.
Deleting a persona whose managed-agent instances are provider-deployed always
failed.
delete_persona(
desktop/src-tauri/src/commands/personas/mod.rs:113) takes only(id, app)—there is no force parameter — and it hard-rejects the cascade when any target
is remote-deployed, via
collect_remote_deployed(
mod.rs:81, matchingbackend != Local && backend_agent_id.is_some()):Both persona-delete call sites went straight to
deletePersonawithouttouching instances first, so they hit that rejection every time:
desktop/src/features/agents/ui/usePersonaActions.ts—handleDeletedesktop/src/features/profile/ui/UserProfilePanel.tsx—handleConfirmDeletePersonaThe confirm dialog made it worse by promising the cascade unconditionally, so
the user saw a promise immediately followed by a refusal.
The fix
Delete the persona's instances first, so the cascade finds nothing left to
refuse.
delete_managed_agentalready has the escape hatch this needs —force_remote_delete: Option<bool>(
desktop/src-tauri/src/commands/agents.rs:1276) — and the frontend alreadywraps it in the right ritual:
deleteManagedAgentWithRulessends!shutdown,presents the orphan-warning confirm, and only then passes
forceRemoteDelete: true.New
deleteManagedAgentsForPersonaWithRulesinfeatures/agents/lib/managedAgentControlActions.tsloops a persona's instancesthrough that per-agent function. Contract: stop the persona cascade at the
first cancelled or failed instance delete, so the persona itself is never
removed while instances remain. Instances already deleted when the abort fires
stay deleted — the cascade bounds the damage rather than unwinding it, and it
reports
deletedCountso the caller can tell the user about that partialstate. Instances are deduplicated by normalized pubkey so a list carrying the
same instance twice cannot delete it twice.
Provider deployments are never force-deleted silently — every one goes through
the existing orphan-warning confirm, and declining it stops the whole cascade.
The delete dialog now calls out the provider-hosted instances in the cascade as
their own sentence rather than folding them into the relay-archival one. The
count is scoped to instances that are actually deployed
(
backend.type === "provider" && backendAgentId), matching both the conditionthe backend rejects on and the condition under which
deleteManagedAgentWithRulesforces. Deleting removes the local managementrecord and its key, tombstones the agent's relay record, and archives its relay
identity — it does not tear down the provider deployment. There is no undeploy
operation today (
commands/agents.rs:455), so the remote infrastructure keepsrunning until it is removed at the provider, and the dialog now says so,
matching the orphan-warning confirm the same flow shows.
The backend guard is deliberately left alone. Its comment states the
invariant: the rejection is a runtime guard, and a compromised IPC caller must
not be able to orphan a live remote deployment. Adding a force flag to
delete_personawould weaken that for no gain, since the frontend can do theinstance deletion explicitly, behind the orphan-warning confirm, rather than
have the backend bypass its own invariant.
On the two loops
features/profile/ui/UserProfilePanelDeletion.tshas its ownpersona-instance loop (
deleteProfileManagedAgentsForPersona) and this PRleaves it in place rather than sharing one implementation. Its extra work is
interleaved per instance — it removes each agent from its channels between
deletes — so sharing the loop would mean giving the generic helper a per-agent
callback parameter, and parameterizing a working delete path to save ~20 lines
seemed the worse trade. Both loops now carry a cross-reference comment naming
the other and the shared abort contract, so drift is visible to a reader.
Happy to consolidate them if maintainers would prefer that.
One deliberate asymmetry: the agents-side cascade does not remove instances
from their channels, while the profile variant does. That preserves the
pre-existing behaviour of the persona cascade —
delete_personaonly evertombstoned and archived — so this fix does not quietly widen what deleting a
persona does beyond making it succeed.
A note on the call signature
usePersonaActions.handleDeletenow takes the instance-teardown function as arequired parameter, supplied by
AgentsView, which already holds both thepersona and managed-agent hooks. That hook is also mounted app-wide by
RequestedAgentCreateDialogs, which never deletes anything, so wiring four newqueries into it to serve one button seemed wrong. Making the parameter required
rather than optional is deliberate: TypeScript now refuses a call that skips
instance teardown, instead of silently degrading back to the bug this PR fixes.
Tests
11 new, all passing.
Cascade (
managedAgentControlActions.test.mjs):is deliberately not first, so the partial teardown is pinned rather than
hidden by test ordering
The abort tests assert that instances after the stopping point are never
touched, and that
deletedCountreports the ones already destroyed.Dialog copy (
PersonaDeleteDialog.test.mjs): singular and plural providerdisclosure, no provider claim when the count is zero, and parity with the
previous output for callers that don't pass the new argument.
Verified locally at the pushed commit: full desktop suite 3846 passed, 0
failed;
just desktop-checkexits 0 (biome, the file-size ratchet,check:px-text,check:pubkey-truncation);just desktop-build(
tsc && vite build) succeeds; anddesktop-tauri-check,desktop-tauri-testand
desktop-tauri-test-compiled-flagsall pass.desktop-tauri-clippyfailslocally on macOS with three
dead_codeerrors insrc-tauri/src/linux_media.rs— a file this PR does not touch, in a modulewhose code is live on the Linux runner CI uses; this PR changes no Rust.
Not covered: there is no E2E test driving the rendered dialog against a
live provider deployment — the confirm path is exercised at the unit level with
a stubbed
window.confirm, not through the real dialog. I also could notverify against an actual provider-backed agent, so the claim that the cascade
now succeeds rests on the traced code path rather than on a reproduction. A
maintainer with a provider deployment to hand could confirm that end to end
quickly.
Related
terms are fix(persona): pack validate errors on skills the runtime would silently skip #3532 (persona pack skill validation) and fix(relay): retire the kind:30620 event on workflow delete (#2879) #3392 (relay workflow
delete); neither touches this path.