Comfy-Org / Comfy-Org/Comfy-Desktop
Cancel during copy-update / release-update post-install phase doesn't roll back the new install
- Dominant language
- TypeScript
- Stars
- 458
- Forks
- 59
- Avg merge
- 22h 18m
- Merged PRs (30d)
- 45
Description
## Summary
Cancelling a `copy-update` (or `release-update` during its post-install migrate phase) after the initial copy/install phase has finished does **not** roll back the new installation. The cancel is silently swallowed inside the update step, the copy is kept on disk, the new installation is registered, and the operation reports `ok: true` to the caller (the picker shows "Copy complete" instead of "Cancelled").
## Affected paths
- [`handleCopyUpdate` in `src/main/lib/ipc/sessionActions/copy.ts`](https://github.com/Comfy-Org/ComfyUI-Desktop-2.0-Beta/blob/main/src/main/lib/ipc/sessionActions/copy.ts) (lines ~78–98) — inner try/catch around `source.handleAction('update-comfyui', …)` catches any thrown error from the update step (including `Cancelled`), writes a warning to the output stream, then falls through to `return { ok: true, navigate: 'list', newInstallationId: entry.id }`.
- [`handleReleaseUpdate`](https://github.com/Comfy-Org/ComfyUI-Desktop-2.0-Beta/blob/main/src/main/lib/ipc/sessionActions/copy.ts) — similar shape during the post-install `source.handleAction('migrate-from', …)` step. The catch block writes a warning, attempts a partial cleanup if `installComplete === false`, but after install completion the cancel just degrades to a "Migration failed" message and keeps the copy.
## Per-op cleanup table (current behaviour)
| Op | Cancel mid-flight | Disk cleanup |
|---|---|---|
| `copy` | ✅ removes dest dir (via `performCopy`'s catch) |
| `copy-update` during copy | ✅ removes dest dir |
| `copy-update` during update | ❌ keeps the copy + registers it, reports success |
| `release-update` during install | ✅ removes entry + dest dir |
| `release-update` during migrate | ❌ same shape — keeps copy, reports as migration failure |
## Reproduce
1. From the picker, run **Copy and Update** on a stable install (so the update step has real work to do — a pull + deps refresh).
2. Wait for the file-copy phase to reach 100%.
3. Click **Cancel** during the "Updating…" phase.
Expected: cancel banner ("Cancelled"), new install removed, source install untouched.
Actual: progress overlay reads "Copy complete", a new install is registered with the partially-updated state, no cancel feedback.
## Suggested fix
1. In `handleCopyUpdate`, when `abort.signal.aborted` is true after the update step throws (or any time during it), treat it as a cancellation: delete `entry` from installations, `rm -rf destPath`, return `{ ok: false, cancelled: true, message: 'Cancelled' }` instead of `{ ok: true, … }`.
2. Mirror the same logic in `handleReleaseUpdate`'s migrate-phase catch (the install-phase catch already handles it via the `!installComplete` branch — extend that to cover post-install-but-pre-migration-success aborts).
3. Audit `update-comfyui` / `migrate-from` source handlers to confirm they actually honour the AbortSignal — if they don't, the cancel will sit until the current shell-out finishes regardless of how the wrapping logic is written.
## Context
Surfaced during the `feature/picker-copy-update` PR (#668) review. Out of scope for that PR — opening this issue so it can be picked up afterwards. The cancel path itself was fixed in #668 (the picker IPC handler was silently swallowing the abort via a dynamic import); this issue is about what the operation handlers do **once** the abort actually reaches them.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.