Skip to content

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

Description

@Kosinkadink

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    comfy-builderComfyBuilder distribution install route

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions