[GH-ISSUE #3301] Preserve installed nodepack state when deletion is deferred #1138

New issue

Open

opened 2026-10-27 21:26:09 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-27 21:26:09 +00:00

Owner

Originally created by @ltdrdata on GitHub (Sep 18, 2026).
Original GitHub issue: https://github.com/Comfy-Org/ComfyUI-Manager/issues/3301

Problem

When legacy Manager cannot delete an installed nodepack directory or rename it into the trash directory, it schedules deletion of the original path for restart. unified_uninstall nevertheless reports success and removes the active-node entry. Reinstallation then fails because the original installation path still exists, leaving disk contents and Manager state inconsistent.

This is a valid pre-existing defect, tracked separately from the Flagged installation policy in #3299 by agreement with its reviewer and the project owner. Review finding and scope discussion.

Reproduction evidence

Executed the production removal and installation functions extracted from manager-v4 base a0d89e9a and PR head 0ceeec62, using isolated temporary directories. Only deletion/rename failures and retry delays were controlled. The production reservation writer recorded the actual deferred paths. This is a deterministic filesystem-failure reproduction, not a real Windows file-lock E2E test.

Both revisions produce the same results:

Filesystem outcome	Original path	Uninstall result	Manager cache	Reservation / subsequent installation
Deletion succeeds	Absent	Success	Removed	No deferred deletion
Rename succeeds, trash cleanup fails	Absent	Success	Removed	Trash path reserved
Deletion and rename both fail	Still present	Incorrectly reports success	Incorrectly removed	Original path reserved; installation rejects the existing path

The original file contents survive the third case, but the Manager cache no longer represents them. The removal functions are unchanged between the compared revisions.

Required behavior

Distinguish the original installation path becoming available from deletion merely being scheduled.
If the original path remains, retain state for the surviving installation and report that removal has not completed. Reinstallation must stop before downloading or running installation scripts.
Preserve the restart-deletion fallback as an explicit pending operation. Do not remove the existing ability to uninstall locked nodepacks after restart or silently describe a queued deletion as completed removal.
If rename succeeds, treat the original path as available. Deferred cleanup must target the unique trash path and must never delete a replacement installed at the original path.
When multiple installed versions/paths are involved, retain entries for surviving paths and remove entries only for paths actually removed. Do not claim rollback of paths already deleted.

Validation for the separate fix

Red/Green coverage for the controlled delete-plus-rename failure above, including result, cache, existing contents, reservation and absence of download/postinstall.
Successful deletion and successful rename controls must retain their existing uninstall/reinstall behavior; allowed reinstall must run its installation script.
Exercise restart processing so the old/trash path is removed as intended and a replacement installation survives.
Add a real supported-Windows lock/retry check where feasible, identifying any platform-dependent skips explicitly.
Audit corresponding v3/glob callers for the same removal-contract assumption and document the actual affected scope.

Scope boundary

The Flagged policy must reject before removal and honor an explicit uninstall failure. Its current regression test establishes that boundary by returning a failed uninstall result; it does not establish correct reporting of deferred deletion. Fixing this issue must preserve the common Active/Flagged gates and denial-state preservation.

Originally created by @ltdrdata on GitHub (Sep 18, 2026). Original GitHub issue: https://github.com/Comfy-Org/ComfyUI-Manager/issues/3301 ## Problem When legacy Manager cannot delete an installed nodepack directory or rename it into the trash directory, it schedules deletion of the original path for restart. unified_uninstall nevertheless reports success and removes the active-node entry. Reinstallation then fails because the original installation path still exists, leaving disk contents and Manager state inconsistent. This is a valid pre-existing defect, tracked separately from the Flagged installation policy in #3299 by agreement with its reviewer and the project owner. [Review finding and scope discussion](https://github.com/Comfy-Org/ComfyUI-Manager/pull/3299#discussion_r4051158926). ## Reproduction evidence Executed the production removal and installation functions extracted from manager-v4 base a0d89e9a and PR head 0ceeec62, using isolated temporary directories. Only deletion/rename failures and retry delays were controlled. The production reservation writer recorded the actual deferred paths. This is a deterministic filesystem-failure reproduction, not a real Windows file-lock E2E test. Both revisions produce the same results: | Filesystem outcome | Original path | Uninstall result | Manager cache | Reservation / subsequent installation | |---|---|---|---|---| | Deletion succeeds | Absent | Success | Removed | No deferred deletion | | Rename succeeds, trash cleanup fails | Absent | Success | Removed | Trash path reserved | | Deletion and rename both fail | Still present | Incorrectly reports success | Incorrectly removed | Original path reserved; installation rejects the existing path | The original file contents survive the third case, but the Manager cache no longer represents them. The removal functions are unchanged between the compared revisions. ## Required behavior - Distinguish the original installation path becoming available from deletion merely being scheduled. - If the original path remains, retain state for the surviving installation and report that removal has not completed. Reinstallation must stop before downloading or running installation scripts. - Preserve the restart-deletion fallback as an explicit pending operation. Do not remove the existing ability to uninstall locked nodepacks after restart or silently describe a queued deletion as completed removal. - If rename succeeds, treat the original path as available. Deferred cleanup must target the unique trash path and must never delete a replacement installed at the original path. - When multiple installed versions/paths are involved, retain entries for surviving paths and remove entries only for paths actually removed. Do not claim rollback of paths already deleted. ## Validation for the separate fix - Red/Green coverage for the controlled delete-plus-rename failure above, including result, cache, existing contents, reservation and absence of download/postinstall. - Successful deletion and successful rename controls must retain their existing uninstall/reinstall behavior; allowed reinstall must run its installation script. - Exercise restart processing so the old/trash path is removed as intended and a replacement installation survives. - Add a real supported-Windows lock/retry check where feasible, identifying any platform-dependent skips explicitly. - Audit corresponding v3/glob callers for the same removal-contract assumption and document the actual affected scope. ## Scope boundary The Flagged policy must reject before removal and honor an explicit uninstall failure. Its current regression test establishes that boundary by returning a failed uninstall result; it does not establish correct reporting of deferred deletion. Fixing this issue must preserve the common Active/Flagged gates and denial-state preservation.

bsakaguchi referenced this issue

2026-10-28 21:03:40 +00:00

[PR #1138] [MERGED] Export Workflow With Cyuai Api Available Nodes #1726