Uh oh!
There was an error while loading. Please reload this page.
fix(desktop): keep a remove from destroying a restored task - #3056
Merged
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review. 📝 WalkthroughSummary
Review-relevant risks
WalkthroughSession removal now requires an archived-state precondition when requested. The desktop stack returns ChangesArchived session removal
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk:⚪ Minimal · up to The desktop deletion flow now preserves tasks restored during removal and reports bulk-delete outcomes correctly; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer
participant PreloadBridge
participant IPCHandler
participant RuntimeHostClient
User->>Renderer: purge archived sessions
Renderer->>PreloadBridge: remove(sessionId, requireArchived)
PreloadBridge->>IPCHandler: invoke session removal
IPCHandler->>RuntimeHostClient: removeSession(sessionId, requireArchived)
RuntimeHostClient-->>IPCHandler: removed or restored
IPCHandler-->>PreloadBridge: removal disposition
PreloadBridge-->>Renderer: removal disposition
Renderer-->>User: deletion or restored-session notification
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Astro-Han
marked this pull request as ready for review
August 15, 2026 09:22
4 tasks
This was referenced Aug 15, 2026
Astro-Hanforce-pushed
the
fix/desktop-remove-requires-archived
branch
from
August 16, 2026 12:00
9c1ee30 to
c15faabCompareNote GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Deleting a task is a read of the catalog followed by a remove, and the Host already refuses the remove when the record changed underneath: the retirement coordinator compares the revision inside its admission lock, and the metadata store re-checks the version in the same transaction as the DELETE. Unarchiving writes through that same header, so a concurrent restore reliably rejects the delete. The Desktop Client threw that rejection away. `removeSession` treated a `revision_conflict` as a stale read and replayed the delete at the fresh revision, up to eight times. Replay is right for a rename or a configuration patch, where the write means the same thing either way. It is wrong for a remove: the conflict is the signal that the task was touched after the caller decided to destroy it, so the replay executed a destruction whose premise was gone — and the retry widened the race from the gap between two calls to the whole loop. The bulk purge in 设置 › 活动 › 已归档任务 made it easy to hit, because a serial sweep holds that window open for every task in it. `removeSession` now takes the premise the caller decided on. With `requireArchived`, each fresh read re-asserts that the task is still archived and answers `restored` instead of replaying. That is enough to hold the premise through the commit: the version check and the DELETE share one transaction, so a remove committing at the revision just read is a remove of the record that was read as archived. `restored` is not a failure and is reported apart from one. A sweep does not count it as removed, does not check it back against the catalog, and names it in the toast rather than quietly deleting fewer tasks than the confirm agreed to. Single-row delete carries the premise of the row it was raised from, so deleting an active task from the rail is unaffected. No Runtime Host or protocol change: the compare-and-set that makes this work is already there. Fixes#3050 Generated-by: Claude Code
The sweep decided an id's fate in two places. It skipped anything its own catalog snapshot no longer showed as archived, and separately reported what the Host answered `restored`. The renderer's list is written synchronously as the sweep runs, so the skip is the path a restore usually takes — and it dropped the id with no outcome at all: not removed, not restored, not remaining. A person who confirmed five and got "deleted two" was told nothing about the other three. The reporting added for that only covered the narrow window where a restore lands after the snapshot read and before the Host commits. Deciding here was never sound. The snapshot is one observer of state the Host owns, a second window can outdate it in either direction, and it cannot tell a restored task from one already deleted elsewhere. So the filter is gone rather than taught to classify: every id goes to the delete, which already distinguishes all four outcomes. Still archived removes; restored answers `restored`; already gone rejects and settles as removed against the catalog, which is what the rejected-id check has always done; anything else is an error with a reason. One path, one outcome, one source of truth. The purge toast stopped choosing which single fact to report. Failures and kept tasks are independent, so a sweep that hit both said only the first and silently dropped the second — and an unverified sweep claimed "the tasks were deleted" over tasks it had deliberately kept. Kept tasks are now a fragment that reads after either outcome. Also corrects the reasoning in `removeSession`'s contract. Re-asserting the premise holds through the commit because the Host serializes `session.remove` and `session.lifecycle.set` through the same per-Session admission queue, not because the client's revision reaches the DELETE transaction — the coordinator refreshes family records inside that lock and commits with those versions. The conclusion was right, the mechanism named was not, and the version check alone would not have carried it. The remove options now refuse a shape they cannot read instead of falling through to "no premise stated", which is the destructive answer, and no longer depend on a sibling validator running first. Refs #3050 Generated-by: Claude Code
M4n5terforce-pushed
the
fix/desktop-remove-requires-archived
branch
from
August 16, 2026 12:40
c15faab to
f0fa5feCompareM4n5ter
approved these changes
Aug 16, 2026
M4n5ter
left a comment
Member
There was a problem hiding this comment.
LGTM. Re-reviewed after rebasing onto the latest main; the conflict resolution preserves the host-aware session routing and the restored-task deletion guard. Targeted tests, Desktop typecheck, and Biome checks pass.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Deleting a task that was concurrently restored deleted it permanently anyway. The Host-side atomicity that prevents this already exists —
session.removeandsession.lifecycle.setboth enter#withStableFamily, which queues per Session id through the admission gate, and the remove compares the revision on the way in. A restore therefore either lands before that comparison, bumpingmetadataVersionand rejecting the remove, or waits until the retirement has finished. It cannot land between the check and the delete.The Desktop Client threw that rejection away.
removeSessiontreated the conflict as a stale read and replayed the delete at the fresh revision, up to eight times. Replay is right for a rename or a configuration patch — the write means the same thing either way. It is wrong for a remove: the conflict is the signal that the task was touched after the caller decided to destroy it, so the replay executed a destruction whose premise no longer held, and the retry widened the race from the gap between two calls to the whole loop.removeSessionnow takes the premise the caller decided on. WithrequireArchived, each fresh read re-asserts that the task is still archived and answersrestoredinstead of replaying. Single-row delete carries the premise of the row it was raised from, so deleting an active task from the rail is unaffected, as is copy cleanup, which states no premise.The bulk sweep in 设置 › 活动 › 已归档任务 used to decide an id's fate in two places: it skipped anything its own catalog snapshot no longer showed as archived, and separately reported what the Host answered
restored. That snapshot is one observer of state the Host owns, a second window can outdate it in either direction, and it cannot tell a restored task from one already deleted elsewhere — so the filter is gone rather than taught to classify. Every id now goes to the delete, which already distinguishes all four outcomes: still archived removes, restored answersrestored, already gone rejects and settles as removed against the catalog, anything else is an error with a reason. The toast reports failures and kept tasks independently instead of choosing which single fact to say.No Runtime Host or protocol change: the serialization and compare-and-set this relies on are already there. The issue's original "Why this belongs to the Host" and "Proposed direction" sections proposed an expected-lifecycle field on
session.remove; that is over-built while Desktop is the only consumer, and is being corrected separately. It becomes the right call when a second client can delete — see the follow-up note below.Fixes#3050
Verification
Reproduced first on unmodified code, then re-run after the fix:
apps/desktop/src/main/__tests__/runtime-host-client-uds.test.ts— the whole main-process chain over real UDS framing. A restore lands between the Client's read and its remove; onmainthe sweep deleted the task (actual: undefined, expected: 'restored', and the catalog came back empty), and now it answersrestored, the task is still listed as active, and nodeletedretirement is emitted for it.apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts— the retry loop itself: abandons on a restore, still retries through churn that left the task archived, still removes a task that was never archived.apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts— every id reaches the delete rather than being filtered against the renderer's snapshot; restored tasks are reported apart from removed and failed and keep their renderer state; single-row delete carries the premise of its own row. Mutation-checked: restoring the snapshot filter turns three of these red.Ran locally: those suites plus
app-shell-session-row-actions-revisions(35 tests, all passing),npm --workspace apps/desktop run typecheck, and Biome lint overapps/desktop/srcandapps/desktop/stories. Notenpm run formatis a no-op here — the formatter excludesapps/desktop/**(biome.jsonc); lint does cover these files. Not run: the repository-wide suite, E2E, and Storybook visual smoke — left to CI. The two-window GUI repro described in the issue was not performed by hand; the UDS test above stands in for it at the seam where the defect lives.Review focus
Data deletion — this needs independent human review, no self-merge fast path.
Two judgement calls worth checking:
requireArchivedis the caller's to state rather than somethingremoveSessionassumes. An active task deleted from the rail, and revision-copy cleanup, have no archived premise to lose; requiring one would break both.Known gaps
SessionRetirementCoordinatorracing a realsession.lifecycle.sethas no integration coverage. That belongs inpackages/runtime-hostand is out of scope here.session.removeconsumer (CLI, bots) could reintroduce Removing a session should require it to still be archived #3050 with no test failing. Worth revisiting under Removing a session should require it to still be archived #3050 when that consumer appears.Checklist
Does this PR entail a change in behavior?