Clean up unique browser partitions on delete + drive-aware data defaults#1016
Open
Kosinkadink wants to merge 5 commits into
Open
Clean up unique browser partitions on delete + drive-aware data defaults#1016Kosinkadink wants to merge 5 commits into
Kosinkadink wants to merge 5 commits into
Conversation
Deleting an install never removed its browser partition. Unique-partition installs each own a persist:<id> bucket under userData/Partitions/<id> that nothing else reuses, so every delete leaked one forever. handleDelete now removes the install record first, then best-effort clears the session (timeout-bounded) and deletes the partition dir (force + retries) so it can never hang or lock up the delete. Shared partitions are left untouched. Also make the large data dirs follow the drive the user picks in the Windows installer: when the app is installed to a non-system drive, installDir, the shared models/input/output root, and the download cache default to that drive instead of always landing on the system drive. Non-Windows and same-drive installs keep the existing home/userData defaults. Amp-Thread-ID: https://ampcode.com/threads/T-019eac5b-d0b9-7090-a4dd-491a74ab1fbe Co-authored-by: Amp <amp@ampcode.com>
When the app is installed to a non-system drive, installs, shared models/input/output, and the download cache now live under one <drive>\\Comfy-Desktop parent instead of three separate folders at the drive root. Home-dir and non-Windows defaults are unchanged. Amp-Thread-ID: https://ampcode.com/threads/T-019eac5b-d0b9-7090-a4dd-491a74ab1fbe Co-authored-by: Amp <amp@ampcode.com>
Member
Author
|
Follow up - see if we can get the electron-manager userData moved as well. |
…-cleanup-and-drive-aware-defaults Amp-Thread-ID: https://ampcode.com/threads/T-019eac5b-d0b9-7090-a4dd-491a74ab1fbe Co-authored-by: Amp <amp@ampcode.com> # Conflicts: # src/main/lib/ipc/sessionActions/delete.ts
…system drive On a system-drive Windows install, the large data dirs (installs, shared models/input/output, download cache) previously defaulted to the user home root and the cache to roaming AppData. New installs now group under %LOCALAPPDATA%\Comfy-Desktop instead, while existing installs keep their home-root layout. The choice is classified from a home-root footprint and pinned with a one-time marker so it never flips between launches. Amp-Thread-ID: https://ampcode.com/threads/T-019eac3f-cda6-74ea-ba76-49da53cadea2 Co-authored-by: Amp <amp@ampcode.com>
…-cleanup-and-drive-aware-defaults
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.
Closes #1017
Summary
Two related disk/storage fixes for the launcher.
1. Browser partitions no longer leak on delete
Deleting an install previously only removed the install dir and the
installations.jsonrecord. Unique-partition installs (browserPartition: 'unique') each own apersist:<id>bucket underuserData/Partitions/<id>(cookies, IndexedDB, localStorage, service-worker cache, GPUCache). The id is unique per-install and never reused, so every create-unique -> delete cycle stranded one of these folders forever. (Copies were already safe: each copy gets a fresh id and its own deep-copied partition.)handleDeletenow, on both delete paths:clearStorageData, raced against a 5s timeout so it can never hang the operation or hold its lock) and removesPartitions/<id>(force+ transient-lock retries).Shared-partition installs are a no-op (they all share
persist:shared, which must never be deleted). Worst case degrades to the old behavior (one folder left behind) - never a lock-up.2. Large data dirs default to a sensible, grouped location
Previously the multi-GB consumers defaulted to the system drive even when the app was installed elsewhere, and on the common system-drive install they landed in poor spots: the install root and shared models/input/output under the home root, and the download cache under roaming AppData (which bloats roaming/AD profiles on login sync). These are still only defaults - users can override each in Settings.
Non-system-drive installs (e.g. app installed to
D:) now group everything under a single<drive>\Comfy-Desktopparent instead of scattering at the drive root:<drive>\Comfy-Desktop\ComfyUI-Installs<drive>\Comfy-Desktop\ComfyUI-Shared(models / input / output)<drive>\Comfy-Desktop\ComfyUI-Cache\download-cacheSystem-drive Windows installs now distinguish new vs existing users:
%LOCALAPPDATA%\Comfy-Desktop(the standard per-user, non-roaming spot for large app data — and writable without admin, unlike a drive root):%LOCALAPPDATA%\Comfy-Desktop\ComfyUI-Installs%LOCALAPPDATA%\Comfy-Desktop\ComfyUI-Shared(models / input / output)%LOCALAPPDATA%\Comfy-Desktop\ComfyUI-Cache\download-cacheNew vs existing is classified from a home-root footprint (
~/ComfyUI-Installs/~/ComfyUI-Shared) and then pinned with a one-time marker (userData/data-location.json) so a user's chosen location can never flip on a later launch (e.g. if the legacy folders appear externally). The marker is written once at startup, never at module import.settings.jsonexistence is deliberately not used as the signal (a new user creates it on first run).Non-Windows behavior and the small Electron-managed
userDatastate are unchanged (the latter intentionally stays on the system drive).Changes
src/main/lib/ipc/shared.ts- adddeleteBrowserPartition(timeout-bounded session clear + dir removal).src/main/lib/ipc/sessionActions/delete.ts- call it on both delete paths, after record removal.src/main/lib/paths.ts- addselectedInstallDrive/defaultDataRoot/defaultDownloadCacheDir; system-drive Windows classifier (%LOCALAPPDATA%\Comfy-Desktopfor new installs, home for existing) with a pinned marker viapersistWinDataRootChoice;builtinDefaultInstallDiruses the data root.src/main/index.ts- callpersistWinDataRootChoice()once at startup.src/main/settings.ts- defaultinstallDir, shared root, and download cache to the data root.Tests
delete.integration.test.ts- unique partition removed + session cleared; shared partition untouched; delete still completes whenclearStorageDatarejects; cleanup works when the install dir is already gone.paths.test.ts- drive redirect to<drive>\Comfy-Desktopon win32 non-home drive; system-drive new install ->%LOCALAPPDATA%\Comfy-Desktop; existing install (home footprint) -> home root + roaming cache; marker overrides later footprint changes in both directions;persistWinDataRootChoicewrites the classified mode once and never overwrites; non-Windows unchanged.