Comfy-Org / Comfy-Org/Comfy-Desktop

Install pipeline hardening: ComfyBuilder updates lose user data (urgent) plus cross-route follow-ups

Open
#1,408 2 comments 1 reaction 0 assignees View on GitHub
comfy-builder
Dominant language
TypeScript
Stars
458
Forks
59
Avg merge
22h 18m
Merged PRs (30d)
45

Description

Cross-route comparison of the two install pipelines (ComfyBuilder distributions vs standalone) during the PR #1404 review cycle surfaced hardening work that each route already solved and the other lacks. Recording all of it here so it can be scheduled. One item is urgent because it silently loses user data.

## Urgent: ComfyBuilder version updates discard saved workflows, settings, and custom nodes

The ComfyBuilder update transaction (`src/main/sources/comfybuilder/index.ts`) replaces the whole executable tree and preserves only `ComfyUI/models`:

- `environmentPaths()` tracks `venv.previous`, `ComfyUI.previous`, and `.comfybuilder-models-preserved` only (index.ts ~93-105).
- `installEnvironmentLocked()` renames `venv`, `ComfyUI/models`, and `ComfyUI` aside, extracts the new archive, moves models back, and on commit deletes both `*.previous` trees (index.ts ~370-433).
- `buildLaunchSpec()` (`src/main/comfybuilder/launch.ts`) passes no `--user-directory`, `--base-directory`, or `--database-url`, so ComfyUI defaults put all user state inside the replaced tree.

What is lost on every successful version update:

- `ComfyUI/user/` - saved workflows, frontend settings, `comfyui.db`, Manager v4 per-install config (`user/__manager/config.ini`)
- `ComfyUI/custom_nodes/` - node packs the user installed through Manager (reachable: the default launch args are `--enable-manager`)

Input/output are safe by default only because the generic launch path injects shared `--input-directory`/`--output-directory` args (`src/main/lib/ipc/sessionActions/launch.ts` ~165-196). A user who switches an install to per-install input/output without setting explicit paths falls back to the in-tree defaults and loses those too.

Repro: install a distribution, save a workflow (or install a node pack via Manager), run the "Update" action to another version. After the update commits, the workflow/node pack is gone with no warning.

This should be fixed before the ComfyBuilder feature ships. Fix plan is being worked out on the PR #1404 branch; acceptance criteria:

- [ ] A version update (and a failed/interrupted update after recovery) preserves saved workflows, frontend settings, the SQLite DB, Manager config, and installed custom node packs
- [ ] Startup recovery handles every crash point of the extended transaction without losing the preserved directories
- [ ] Tests cover update-preserves-user-data, rollback-restores-user-data, and crash-recovery interleavings

## Follow-ups: standalone route should adopt ComfyBuilder's integrity guarantees

- [ ] Verify SHA-256 on standalone bundle downloads. `downloadAndExtract` / `downloadAndExtractMulti` (`src/main/lib/installer.ts`) and the R2 catalog (`src/main/sources/standalone/r2Catalog.ts`) have no checksum verification for the Python standalone bundles or torch-stack bundles; the bytes are extracted and executed on HTTPS trust alone. ComfyBuilder fails closed without a hash for both artifacts and models; the same reasoning (TLS does not protect against storage/control-plane substitution) applies to the R2 catalog. Needs hashes published in the catalog manifest plus verification before extraction.
- [ ] Audit that torch-stack and opMarker startup recovery are sequenced before any window/reentry can launch an install, the way ComfyBuilder recovery is awaited behind IPC registration.

## Follow-ups: ComfyBuilder route should adopt standalone's field-hardening

- [ ] Per-phase install telemetry. Standalone emits `comfy.desktop.install.phase` (phase, duration, error fields); ComfyBuilder install/update/recovery has no equivalent, so field failures will not be attributable to resolve/download/extract/models/recovery.
- [ ] Post-commit rollback. ComfyBuilder rollback only covers failed/interrupted transactions; once the ready marker commits, going back means a full re-download. The record already captures the previous artifact identity during an update - expose it as a "reinstall previous version" action instead of discarding it on commit.

## Follow-up: unify the transactional directory-swap primitives

Three independent implementations of "atomic rename double-buffering + durable intent + startup recovery" now exist:

- `src/main/sources/standalone/torchStackTransaction.ts` (journal file, `.venv.torch-backup`, `.torch-gc` commit rename)
- ComfyBuilder's `*.previous` + ready marker + record rollback field (`src/main/sources/comfybuilder/index.ts`)
- the generic opMarker (`src/main/lib/opMarker.ts`) used by the standalone update orchestrator

- [ ] Extract one shared primitive so there is a single recovery function to test exhaustively instead of three.

Refs #1404.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reading src/main/sources/comfybuilder/index.ts, src/main/comfybuilder/launch.ts, and src/main/lib/ipc/sessionActions/launch.ts to trace update preservation and launch paths. Then inspect src/main/lib/installer.ts, src/main/sources/standalone/r2Catalog.ts, src/main/sources/standalone/torchStackTransaction.ts, and src/main/lib/opMarker.ts for the follow-ups. Done requires preserved user data, rollback and crash-recovery coverage, checksum verification, recovery sequencing, telemetry, previous-version reinstall support, and a shared transaction primitive.

Written by the indexing model from the issue text.

Assessment

Tech stack
sqlite, typescript
Domain
desktop, devops, release, testing
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.