From e3793ee1541d08cd54bb0d9d7243be285469b4bf Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Sun, 6 Sep 2026 13:46:26 +0300 Subject: [PATCH] core, app: FunPack updates itself again Second off the port list, and the one that gets reached for most: a pack whose value is experimental work shipped continuously lives on switch-branch and update. git_update.py comes across from v4 as core/update.py, changed in three places and no more: where the repo root is, a `remote=False` mode, and a 20-second timeout on the one call that touches the network. It has been switching branches on real rentals for months and every refusal in it -- no git on PATH, not a checkout, a dirty tree, a branch that exists on neither side -- is a lesson that cost somebody an afternoon. Rewriting it to look more like v5 would trade proven plumbing for a fresh set of edge cases. It is core rather than a module because it is about the INSTALL: no module can announce the thing that updates every module. Four routes, and every one of them ends by relaunching ComfyUI -- so the answer is written first and the restart is a callback scheduled after it. `restart` is looked up on the module rather than captured, or running the suite would relaunch the test runner. The window reads the checkout TWICE: once without the network, so there is something to see instantly, then again with it for ahead and behind. A fetch takes seconds on a good day and blocks until it times out on a machine that cannot reach the remote, and neither is a reason to show an empty box. A dirty tree is said before either button is pressed rather than as a refusal after. Then it waits: an overlay that polls /api/health until the server answers, and says so when it does not come back rather than spinning forever. That last part needed the kit's blocking overlay to be able to change its message -- it could not, so my give-up branch would have failed silently, which is the exact pattern this project keeps finding. --- app/boot.js | 2 + app/composer/elements/floating.js | 11 +- app/shell/layout.js | 1 + app/shell/menubar.js | 9 +- app/shell/updates.js | 180 +++++++++++ app/tests-layout/updates.spec.js | 103 ++++++ core/restart.py | 43 +++ core/routes.py | 61 +++- core/tests/test_routes_update.py | 164 ++++++++++ core/update.py | 498 ++++++++++++++++++++++++++++++ 10 files changed, 1068 insertions(+), 4 deletions(-) create mode 100644 app/shell/updates.js create mode 100644 app/tests-layout/updates.spec.js create mode 100644 core/restart.py create mode 100644 core/tests/test_routes_update.py create mode 100644 core/update.py diff --git a/app/boot.js b/app/boot.js index cc4a7781..c61a1a72 100644 --- a/app/boot.js +++ b/app/boot.js @@ -21,6 +21,7 @@ import { createInspector } from "./shell/inspector.js"; import { createWheel } from "./shell/wheel.js"; import { offerAction } from "./shell/actions.js"; import { open as openWizard } from "./shell/wizard.js"; +import { open as openUpdates } from "./shell/updates.js"; const root = document.querySelector("#app"); @@ -106,6 +107,7 @@ async function start() { onGenerate: () => { ranFor = project.selectedId; session.generate(); }, onCancel: () => run.cancel(), onConstructor: () => page.constructor.open(), + onUpdates: () => openUpdates(), // What the Edit menu offers, and what the keyboard reaches. One list, so a // menu item and its shortcut cannot drift apart. edits: { diff --git a/app/composer/elements/floating.js b/app/composer/elements/floating.js index a8ce87ff..35fefc07 100644 --- a/app/composer/elements/floating.js +++ b/app/composer/elements/floating.js @@ -2,6 +2,7 @@ import { define } from "../internals/register.js"; import { el } from "../internals/el.js"; +import { setText } from "../internals/text.js"; import { uid } from "../internals/ids.js"; import { mount, unmount } from "../internals/portal.js"; import { claimFor } from "../internals/zlayer.js"; @@ -203,9 +204,10 @@ define("floating", "window", ({ /** A full-screen "please wait" that genuinely blocks. */ define("overlay", "blocking", ({ message, progress, cancel } = {}) => { + const words = el("p", { cls: "cx-t cx-t-sm", text: message }); const card = el("div", { cls: "cx-blocking-card", children: [ el("span", { cls: ["cx-spinner", "cx-spinner-md"], attrs: { "aria-hidden": "true" } }), - el("p", { cls: "cx-t cx-t-sm", text: message }), + words, progress && progress.node ? progress.node : null, cancel && cancel.node ? cancel.node : null, ].filter(Boolean) }); @@ -220,6 +222,13 @@ define("overlay", "blocking", ({ message, progress, cancel } = {}) => { let closed = false; const handle = { node, isOverlay: true, + // What it is waiting for can change while it waits -- "restarting" becomes + // "it has not come back", and a spinner that cannot say so is a spinner + // somebody watches for ten minutes. + setMessage(next) { + setText(words, next); + node.setAttribute("aria-label", next); + }, close() { if (closed) return; closed = true; release(); layer.release(); unmount(node); }, destroy() { handle.close(); }, }; diff --git a/app/shell/layout.js b/app/shell/layout.js index 0464a71a..de75b2dc 100644 --- a/app/shell/layout.js +++ b/app/shell/layout.js @@ -187,6 +187,7 @@ export function build(root, handlers = {}) { const menubar = createMenubar({ workspace, onPipeline: handlers.onPipeline, + onUpdates: handlers.onUpdates, onProject: handlers.onProject, edits: handlers.edits, projects: handlers.projects, diff --git a/app/shell/menubar.js b/app/shell/menubar.js index 772abc23..23a2b225 100644 --- a/app/shell/menubar.js +++ b/app/shell/menubar.js @@ -70,7 +70,7 @@ function menu(label, itemsOf, onPick) { } export function createMenubar({ workspace, onPipeline, onProject, projects = () => [], - current = () => null, edits = null, + current = () => null, edits = null, onUpdates, theme = services.theme } = {}) { const connection = createConnection(); @@ -117,7 +117,12 @@ export function createMenubar({ workspace, onPipeline, onProject, projects = () const settings = menu("Settings", () => [ { id: "pipeline", label: "Models and pipeline…" }, - ], (id) => { if (id === "pipeline" && onPipeline) onPipeline(); }); + { separator: true }, + { id: "updates", label: "Updates…" }, + ], (id) => { + if (id === "pipeline" && onPipeline) onPipeline(); + else if (id === "updates" && onUpdates) onUpdates(); + }); const bar = composer.toolbar.default({ label: "FunPack", diff --git a/app/shell/updates.js b/app/shell/updates.js new file mode 100644 index 00000000..7ffea647 --- /dev/null +++ b/app/shell/updates.js @@ -0,0 +1,180 @@ +// Updating FunPack, switching branch, and getting back afterwards. +// +// The most-used feature in a pack whose value is experimental work shipped +// continuously -- so it is a window in the app rather than a terminal command, +// and it is honest about the one thing that makes it frightening: every one of +// these restarts ComfyUI. The window says so before it does it, and then stays +// up, polling, until the server answers again. A restart with nothing on screen +// is indistinguishable from a crash. + +import { composer } from "../composer/composer.js"; + +const BASE = "/funpack/api/git"; + +async function ask(method, path, body) { + const res = await fetch(`${BASE}${path}`, { + method, + headers: body === undefined ? {} : { "Content-Type": "application/json" }, + body: body === undefined ? undefined : JSON.stringify(body), + }); + const payload = await res.json().catch(() => ({})); + if (!res.ok) throw new Error(payload.detail || `HTTP ${res.status}`); + return payload; +} + +export const status = ({ remote = true } = {}) => + ask("GET", `/status${remote ? "" : "?remote=0"}`); + +/** How far behind, in words. The number alone does not say whether to care. */ +function summarise(s) { + if (!s.ok) return s.detail || "FunPack cannot read its own git checkout."; + if (!s.checked_remote) return "Checking for updates…"; + if (!s.fetch_ok) return "Could not reach the remote, so this may be out of date."; + if (s.behind) return `${s.behind} update${s.behind === 1 ? "" : "s"} available.`; + if (s.ahead) return `Up to date, with ${s.ahead} local commit${s.ahead === 1 ? "" : "s"} of your own.`; + return "Up to date."; +} + +/** + * The overlay that stays up across a restart. + * + * Polls /api/health rather than guessing a duration: an update that installs + * requirements takes as long as it takes, and a fixed wait either lies or gives + * up early. Reloads once the server answers, because the code that just changed + * is the code the page is running. + */ +export function waitForRestart({ poll = 1500, give_up = 180000 } = {}) { + const overlay = composer.overlay.blocking({ + message: "Restarting ComfyUI. This page comes back on its own.", + }); + const started = Date.now(); + + const tick = async () => { + try { + const res = await fetch("/funpack/api/health", { cache: "no-store" }); + if (res.ok) { window.location.reload(); return; } + } catch { /* still down, which is the expected half of this */ } + if (Date.now() - started > give_up) { + overlay.setMessage("ComfyUI has not come back. Check the terminal it was started from."); + return; + } + setTimeout(tick, poll); + }; + setTimeout(tick, poll); + return overlay; +} + +/** + * open() -> the window. One at a time. + * + * Built from a live status every time it opens: what branch you are on and how + * far behind are exactly the facts that go stale while a window is closed. + */ +export function open({ onRestart = waitForRestart } = {}) { + let window_ = null; + const body = composer.region.stack({ gap: "sm", label: "Updates" }); + + const act = async (run) => { + body.set([composer.hint.default({ text: "Working. This can take a few minutes if " + + "requirements changed." })]); + try { + await run(); + if (window_) window_.close("restarting"); + onRestart(); + } catch (err) { + // Said in the window, where the button was pressed. Every refusal this can + // give names what to do: commit or stash, install git, pick another branch. + draw(null, err.message); + } + }; + + function draw(s, problem) { + if (problem) { + body.set([composer.banner.danger({ text: problem }), + composer.button.md({ label: "Try again", onClick: () => load() })]); + return; + } + if (!s) { body.set([composer.hint.default({ text: "Reading the checkout…" })]); return; } + + const rows = [ + composer.settingsRow.default({ label: "Version", hint: s.codename || undefined, + control: composer.text.sm({ text: s.version || "unknown" }) }), + composer.settingsRow.default({ label: "Branch", hint: summarise(s), + control: composer.text.sm({ text: s.branch || "?" }) }), + ]; + + if (s.dirty) { + // The one state that blocks both actions, said before either is pressed + // rather than as a refusal afterwards. + rows.push(composer.banner.warn({ + text: "This checkout has local changes. Commit or stash them before updating " + + "or switching branch.", + })); + } + + rows.push(composer.settingsRow.default({ + label: "Switch to", + hint: "Switching pulls that branch and restarts ComfyUI.", + control: composer.select.md({ + label: "Branch", + value: s.branch, + options: (s.branches || []).map((b) => ({ value: b, label: b })), + onChange: (branch) => { if (branch !== s.branch) act(() => ask("POST", "/checkout", { branch })); }, + disabled: Boolean(s.dirty), + }), + })); + + if (s.rollback_target) { + rows.push(composer.settingsRow.default({ + label: "Roll back", + hint: `Back to ${String(s.rollback_target.commit || "").slice(0, 8)} — the version before the last update.`, + control: composer.button.md({ label: "Roll back", tone: "danger", + onClick: () => act(() => ask("POST", "/rollback")) }), + })); + } + + body.set([composer.group.default({ rows })]); + if (window_) { + window_.setFooter({ + note: s.ok ? "" : "FunPack is not a git checkout, so it cannot update itself.", + actions: [ + composer.button.md({ label: "Close", tone: "ghost", + onClick: () => window_.close("done") }), + composer.button.md({ + label: s.behind ? `Update (${s.behind})` : "Update", tone: "primary", + disabled: !s.ok || Boolean(s.dirty), + onClick: () => act(() => ask("POST", "/update", {})), + }), + ], + }); + } + } + + async function load() { + draw(null); + // Twice: the checkout alone first, so there is something to read instantly, + // then again with the remote for ahead and behind. A `git fetch` takes + // seconds on a good day and blocks until it times out on a machine that + // cannot reach the network -- and neither is a reason to show nothing. + let local = null; + try { + local = await status({ remote: false }); + draw(local); + } catch (err) { + draw(null, err.message); + return; + } + try { draw(await status()); } + catch { /* the local answer stands, and says it has not checked */ } + } + + window_ = composer.modal.generic({ + title: "Updates", + subtitle: "FunPack updates itself from git, then restarts ComfyUI.", + size: "md", + body, + onClose: () => { window_ = null; }, + }); + load(); + return window_; +} diff --git a/app/tests-layout/updates.spec.js b/app/tests-layout/updates.spec.js new file mode 100644 index 00000000..426f6f12 --- /dev/null +++ b/app/tests-layout/updates.spec.js @@ -0,0 +1,103 @@ +// Updating, against the real routes. +// +// The dev server IS a git checkout of this repo, so the status route answers +// truthfully and the window can be read the way a person would read it. What is +// NOT exercised here is a real pull or a real restart: those change the checkout +// the tests are running from. Every path that would do that is stubbed at the +// fetch, and the stub is what proves the request was the right one. + +import { test, expect } from "@playwright/test"; + +const openUpdates = async (page) => { + await page.getByRole("button", { name: "Settings" }).click(); + await page.getByRole("menuitem", { name: /Updates/ }).click(); + await expect(page.locator(".cx-modal")).toBeVisible(); +}; + +test("the window says which branch you are on and whether there is anything new", async ({ page }) => { + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + await openUpdates(page); + + const modal = page.locator(".cx-modal"); + // Read from the real checkout, not from a fixture: this repo is on a branch + // and the route reports it. + const branch = await page.evaluate(async () => { + const r = await fetch("/funpack/api/git/status"); + return (await r.json()).branch; + }); + await expect(modal).toContainText(branch); + await expect(modal.getByText(/Up to date|update.? available|out of date/i)).toBeVisible(); +}); + +test("a checkout with local changes says so before anything is pressed", async ({ page }) => { + // The one state that blocks both actions. A refusal after the press would be + // a surprise; this is the same fact, said first. + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + await page.route("**/api/git/status", (route) => route.fulfill({ + json: { ok: true, version: "5.0", branch: "v5", branches: ["v5", "dev"], + dirty: true, ahead: 0, behind: 2, fetch_ok: true, repo: "/x" }, + })); + await openUpdates(page); + + const modal = page.locator(".cx-modal"); + await expect(modal).toContainText(/local changes/i); + await expect(modal.getByRole("button", { name: /Update/ })).toBeDisabled(); +}); + +test("update asks the server to update, and then waits for it to come back", async ({ page }) => { + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + await page.route("**/api/git/status", (route) => route.fulfill({ + json: { ok: true, version: "5.0", branch: "v5", branches: ["v5", "dev"], + dirty: false, ahead: 0, behind: 3, fetch_ok: true, repo: "/x" }, + })); + + const asked = []; + await page.route("**/api/git/update", (route) => { + asked.push(route.request().method()); + return route.fulfill({ json: { restarting: true, updated: true } }); + }); + // The page would reload on a healthy answer; keep the server "down" so the + // overlay stays up and can be read. + await page.route("**/api/health", (route) => route.abort()); + + await openUpdates(page); + await page.getByRole("button", { name: /Update \(3\)/ }).click(); + + await expect.poll(() => asked).toEqual(["POST"]); + await expect(page.locator(".cx-blocking")).toBeVisible(); + await expect(page.locator(".cx-blocking")).toContainText(/Restarting/i); +}); + +test("a refusal from git is shown in the window, not swallowed", async ({ page }) => { + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + await page.route("**/api/git/status", (route) => route.fulfill({ + json: { ok: true, version: "5.0", branch: "v5", branches: ["v5", "dev"], + dirty: false, ahead: 0, behind: 1, fetch_ok: true, repo: "/x" }, + })); + await page.route("**/api/git/update", (route) => route.fulfill({ + status: 400, json: { detail: "Working tree has local changes." }, + })); + + await openUpdates(page); + await page.getByRole("button", { name: /Update/ }).click(); + + await expect(page.locator(".cx-modal")).toContainText("Working tree has local changes."); + await expect(page.locator(".cx-blocking"), "it waited for a restart that was refused") + .toHaveCount(0); +}); + +test("an install that is not a git checkout says so instead of offering to update", async ({ page }) => { + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + await page.route("**/api/git/status", (route) => route.fulfill({ + json: { ok: false, version: "5.0", detail: "FunPack is not a git checkout (no .git directory)." }, + })); + + await openUpdates(page); + await expect(page.locator(".cx-modal")).toContainText(/not a git checkout/i); + await expect(page.locator(".cx-modal").getByRole("button", { name: /Update/ })).toBeDisabled(); +}); diff --git a/core/restart.py b/core/restart.py new file mode 100644 index 00000000..cf63b877 --- /dev/null +++ b/core/restart.py @@ -0,0 +1,43 @@ +"""Relaunching ComfyUI, and nothing else. + +Its own file because it is the one operation here that ends the process: every +route that calls it has already finished answering, and anything that imports it +by accident should be obvious in the import list. + +Carried across from v4. The three launch shapes it handles -- comfy-cli's reboot +file, `python -m comfy`, and a plain script -- are three ways somebody's install +would otherwise fail to come back, and each was found by an install failing to +come back. +""" + +from __future__ import annotations + +def restart() -> None: + """Relaunch the ComfyUI process in place. Mirrors ComfyUI-Manager's reboot so it + works the same whether launched directly, as a module, or via comfy-cli.""" + import os + import sys + try: + sys.stdout.close_log() # type: ignore[attr-defined] # Manager's tee logger, if present + except Exception: + pass + # comfy-cli watches for a .reboot file and relaunches us itself. + if "__COMFY_CLI_SESSION__" in os.environ: + try: + open(os.environ["__COMFY_CLI_SESSION__"] + ".reboot", "w").close() + except Exception: + pass + print("\n[FunPack] Restarting ComfyUI...\n", flush=True) + os._exit(0) + sys_argv = sys.argv.copy() + if "--windows-standalone-build" in sys_argv: + sys_argv.remove("--windows-standalone-build") + if sys_argv and sys_argv[0].endswith("__main__.py"): # python -m comfy + module_name = os.path.basename(os.path.dirname(sys_argv[0])) + cmds = [sys.executable, "-m", module_name] + sys_argv[1:] + elif sys.platform.startswith("win32"): + cmds = ['"' + sys.executable + '"', '"' + sys_argv[0] + '"'] + sys_argv[1:] + else: + cmds = [sys.executable] + sys_argv + print(f"\n[FunPack] Restarting ComfyUI... {cmds}\n", flush=True) + os.execv(sys.executable, cmds) diff --git a/core/routes.py b/core/routes.py index 6bb7b8b0..7bcd19ea 100644 --- a/core/routes.py +++ b/core/routes.py @@ -7,7 +7,7 @@ thin adapters over pure functions in `serve`. import json -from . import (config, graph as graph_mod, log, projects, +from . import (config, graph as graph_mod, log, projects, update as update_mod, registry as registry_mod, serve as static, widgets) from .contract import CONTRACT_VERSION from .relations import order @@ -236,6 +236,65 @@ def register(routes, prefix=None): "prompt": prompt if not (problems or incomplete) else None, }) + # --- keeping the install alive ------------------------------------------- + # + # Updating is the most-used feature in a pack whose value is experimental + # work shipped continuously, so it is in the app rather than in a terminal. + # + # Every one of these ends by relaunching ComfyUI, which means the answer has + # to be written BEFORE the process goes. They schedule the restart and return + # `restarting: true`; the app then polls /api/health until the server is back. + # + # `restart` is looked up on the module rather than captured, so a test can + # replace it -- otherwise running the suite would relaunch the test runner. + + async def _git(action, **kwargs): + """Run one git operation off the event loop, or say why it did not.""" + import asyncio + try: + result = await asyncio.to_thread(action, **kwargs) + except update_mod.GitUpdateError as exc: + # A refusal, not a crash: no git, not a checkout, a dirty tree, a + # branch that exists on neither side. Each names what to do. + return web.json_response({"detail": str(exc)}, status=400) + except Exception as exc: # noqa: BLE001 + log.broke("git", exc, doing=getattr(action, "__name__", "git")) + return web.json_response({"detail": f"{type(exc).__name__}: {exc}"}, status=500) + + import asyncio as _asyncio + from . import restart as restart_mod + # After the response is written, not before. + _asyncio.get_event_loop().call_later(0.7, restart_mod.restart) + return web.json_response({"restarting": True, **(result or {})}) + + @routes.get(P + "/api/git/status") + async def _git_status(req): + # `?remote=0` answers from the checkout alone. The fetch is the only part + # that touches the network and so the only part that can hang; the app + # asks without it first so it has something to draw. + import asyncio + remote = req.query.get("remote", "1") not in ("0", "false", "no") + return web.json_response(await asyncio.to_thread(update_mod.status, remote=remote)) + + @routes.post(P + "/api/git/update") + async def _git_pull(req): + body = await req.json() if req.can_read_body else {} + branch = str((body or {}).get("branch") or "").strip() or None + return await _git(update_mod.pull, branch=branch, install_deps=True) + + @routes.post(P + "/api/git/checkout") + async def _git_checkout(req): + body = await req.json() if req.can_read_body else {} + branch = str((body or {}).get("branch") or "").strip() + if not branch: + return web.json_response({"detail": "Which branch? Name one."}, status=400) + return await _git(update_mod.checkout, branch=branch, + pull_after=True, install_deps=True) + + @routes.post(P + "/api/git/rollback") + async def _git_rollback(_req): + return await _git(update_mod.rollback) + @routes.get(P + "/api/nodes") async def _nodes(req): """What the nodes in a pipeline look like to someone editing them. diff --git a/core/tests/test_routes_update.py b/core/tests/test_routes_update.py new file mode 100644 index 00000000..0afa4a4b --- /dev/null +++ b/core/tests/test_routes_update.py @@ -0,0 +1,164 @@ +"""Keeping the install alive, over HTTP. + +Every one of these routes ends by relaunching ComfyUI. The point of the tests is +that the ANSWER is written first and the restart is a scheduled callback -- so +they replace it, and a suite that did not would relaunch the test runner. +""" + +import json +import socket +import threading + +import pytest + +pytest.importorskip("aiohttp") + +from core import routes, update as update_mod # noqa: E402 + + +@pytest.fixture +def server(comfyui, monkeypatch): + from aiohttp import web as aioweb + + restarts = [] + from core import restart as restart_mod + monkeypatch.setattr(restart_mod, "restart", lambda: restarts.append(1)) + + app = aioweb.Application() + table = aioweb.RouteTableDef() + routes.register(table, prefix="/funpack") + app.add_routes(table) + + holder = {"ready": threading.Event(), "restarts": restarts} + + def run(): + import asyncio + loop = asyncio.new_event_loop() + asyncio.set_event_loop(loop) + runner = aioweb.AppRunner(app) + loop.run_until_complete(runner.setup()) + site = aioweb.TCPSite(runner, "127.0.0.1", 0) + loop.run_until_complete(site.start()) + holder["port"] = site._server.sockets[0].getsockname()[1] + holder["loop"] = loop + holder["ready"].set() + loop.run_forever() + + thread = threading.Thread(target=run, daemon=True) + thread.start() + assert holder["ready"].wait(20), "the test server did not start" + yield holder + holder["loop"].call_soon_threadsafe(holder["loop"].stop) + + +def _request(port, method, path, body=None): + payload = b"" if body is None else json.dumps(body).encode() + head = (f"{method} {path} HTTP/1.1\r\nHost: localhost\r\n" + f"Content-Type: application/json\r\n" + f"Content-Length: {len(payload)}\r\nConnection: close\r\n\r\n").encode() + with socket.create_connection(("127.0.0.1", port), timeout=60) as sock: + sock.sendall(head + payload) + chunks = [] + while True: + got = sock.recv(65536) + if not got: + break + chunks.append(got) + raw = b"".join(chunks) + header, _, rest = raw.partition(b"\r\n\r\n") + status = int(header.split()[1]) + try: + return status, json.loads(rest.decode() or "{}") + except ValueError: + return status, {"raw": rest.decode(errors="replace")} + + +def test_the_app_can_see_which_branch_it_is_on(server): + """The one question every update starts from.""" + status, body = _request(server["port"], "GET", "/funpack/api/git/status") + assert status == 200 + assert body["ok"] is True, body + assert body["branch"], "no branch reported" + assert isinstance(body["branches"], list) and body["branches"] + assert body["repo"].endswith("ComfyUI-FunPack-v5") + assert server["restarts"] == [], "reading the status restarted ComfyUI" + + +def test_switching_to_a_branch_that_is_not_there_is_refused_by_name(server): + """A refusal has to say which branch and that it exists nowhere -- otherwise + the user cannot tell it from a typo in their own request.""" + status, body = _request(server["port"], "POST", "/funpack/api/git/checkout", + {"branch": "no-such-branch-here"}) + assert status == 400 + assert "no-such-branch-here" in body["detail"] + assert server["restarts"] == [], "a refused switch still restarted ComfyUI" + + +def test_switching_to_nothing_is_refused_before_git_is_touched(server): + status, body = _request(server["port"], "POST", "/funpack/api/git/checkout", {}) + assert status == 400 + assert "branch" in body["detail"].lower() + assert server["restarts"] == [] + + +def test_a_refusal_from_git_is_reported_as_a_reason_not_a_crash(server, monkeypatch): + """Dirty tree, no git on PATH, not a checkout: the user has to be told which.""" + def refuse(*_a, **_k): + raise update_mod.GitUpdateError("Working tree has local changes.") + monkeypatch.setattr(update_mod, "pull", refuse) + + status, body = _request(server["port"], "POST", "/funpack/api/git/update", {}) + assert status == 400 + assert "local changes" in body["detail"] + assert server["restarts"] == [] + + +def test_an_update_that_worked_answers_before_it_restarts(server, monkeypatch): + """The answer has to be written first: the process is about to go.""" + monkeypatch.setattr(update_mod, "pull", + lambda **_k: {"updated": True, "before": "a1", "after": "b2"}) + + status, body = _request(server["port"], "POST", "/funpack/api/git/update", + {"branch": "dev"}) + assert status == 200, body + assert body["restarting"] is True + assert body["after"] == "b2" + + +def test_an_unexpected_failure_is_a_500_with_the_reason_not_a_hang(server, monkeypatch): + def explode(*_a, **_k): + raise OSError("disk went away") + monkeypatch.setattr(update_mod, "rollback", explode) + + status, body = _request(server["port"], "POST", "/funpack/api/git/rollback") + assert status == 500 + assert "disk went away" in body["detail"] + assert server["restarts"] == [], "a failed rollback restarted ComfyUI anyway" + + +def test_the_status_can_be_had_without_touching_the_network(server): + """The fetch is the only part that can hang. On a machine that cannot reach + the remote it must still be possible to see which branch you are on.""" + status, body = _request(server["port"], "GET", "/funpack/api/git/status?remote=0") + assert status == 200 + assert body["ok"] is True + assert body["branch"] + assert body["checked_remote"] is False + assert body["behind"] == 0, "behind was reported without asking the remote" + + +def test_a_remote_that_cannot_be_reached_is_said_rather_than_guessed(server, monkeypatch): + def no_network(*args, **kwargs): + class Failed: + returncode = 1 + stdout = "" + stderr = "could not resolve host" + return Failed() if args and args[0] == "fetch" else _real(*args, **kwargs) + _real = update_mod._run_git + monkeypatch.setattr(update_mod, "_run_git", no_network) + + status, body = _request(server["port"], "GET", "/funpack/api/git/status") + assert status == 200 + assert body["fetch_ok"] is False + assert body["checked_remote"] is True + assert body["behind"] == 0, "a failed fetch was read as 'up to date'" diff --git a/core/update.py b/core/update.py new file mode 100644 index 00000000..011ada62 --- /dev/null +++ b/core/update.py @@ -0,0 +1,498 @@ +"""Git pull / branch switch for the FunPack repo (ComfyUI custom node root). + +Carried across from v4 unchanged but for REPO_ROOT, and deliberately: this is +the most-used feature in an experimental pack, it has been switching branches on +real rentals for months, and every one of its refusals -- no git on PATH, not a +checkout, a dirty tree, a branch that exists on neither side -- is a lesson that +cost somebody an afternoon. Rewriting it to look more like v5 would be trading +proven plumbing for a fresh set of edge cases. + +It is core rather than a module because it is about the INSTALL, not about a +feature: no module can announce the thing that updates every module. +""" +from __future__ import annotations + +import re +import shutil +import subprocess +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] +_BRANCH_PRIORITY = ("dev", "cutting_edge", "main") + + +class GitUpdateError(RuntimeError): + pass + + +def _run_git(*args: str, timeout: int = 120) -> subprocess.CompletedProcess[str]: + if not shutil.which("git"): + raise GitUpdateError("git not found on PATH.") + if not (REPO_ROOT / ".git").exists(): + raise GitUpdateError("FunPack is not a git checkout (no .git directory).") + proc = subprocess.run( + ["git", *args], + cwd=REPO_ROOT, + capture_output=True, + text=True, + timeout=timeout, + ) + return proc + + +def funpack_version() -> str: + """Custom-node version from pyproject.toml (shown in Settings ▸ About).""" + try: + text = (REPO_ROOT / "pyproject.toml").read_text(encoding="utf-8") + except OSError: + return "" + m = re.search(r'^version\s*=\s*"([^"]+)"', text, re.MULTILINE) + return m.group(1) if m else "" + + +# Adjective + vegetable sharing an initial, advancing alphabetically. Keyed by MAJOR, so +# every 3.x release ships under one name. +CODENAMES = { + "3": "Auspicious Asparagus", + "4": "Blinding Blackout", +} + + +def funpack_codename(version: str = "") -> str: + """Codename for `version` (default: the installed one); "" when its major has none.""" + major = (version or funpack_version()).split(".", 1)[0].strip() + return CODENAMES.get(major, "") + + +def _current_branch() -> str: + proc = _run_git("rev-parse", "--abbrev-ref", "HEAD") + if proc.returncode != 0: + raise GitUpdateError((proc.stderr or proc.stdout or "git rev-parse failed").strip()) + return (proc.stdout or "").strip() + + +def _current_commit() -> str: + proc = _run_git("rev-parse", "--short", "HEAD") + if proc.returncode != 0: + raise GitUpdateError((proc.stderr or proc.stdout or "git rev-parse failed").strip()) + return (proc.stdout or "").strip() + + +def _is_dirty() -> bool: + proc = _run_git("status", "--porcelain") + if proc.returncode != 0: + raise GitUpdateError((proc.stderr or "git status failed").strip()) + return bool((proc.stdout or "").strip()) + + +def _ahead_behind(branch: str) -> tuple[int, int]: + proc = _run_git("rev-list", "--left-right", "--count", f"HEAD...origin/{branch}") + if proc.returncode != 0: + return 0, 0 + parts = (proc.stdout or "").strip().split() + if len(parts) != 2: + return 0, 0 + try: + return int(parts[0]), int(parts[1]) + except ValueError: + return 0, 0 + + +def _unique_local_commits(branch: str) -> list[str]: + """Local commits with no patch-equivalent upstream — i.e. real work that a reset + would destroy. `git cherry` marks those "+" and marks "-" the ones whose CONTENT is + already upstream under a different hash, which is exactly what a rewritten history + leaves behind.""" + proc = _run_git("cherry", f"origin/{branch}", branch) + if proc.returncode != 0: + return [""] # can't prove it's safe → treat as if there were local work + return [ln[2:].strip() for ln in (proc.stdout or "").splitlines() if ln.startswith("+")] + + +def _remote_names() -> set[str]: + proc = _run_git("remote") + if proc.returncode != 0: + return set() + return {ln.strip() for ln in (proc.stdout or "").splitlines() if ln.strip()} + + +def _list_branches() -> list[str]: + proc = _run_git("branch", "-a", "--format=%(refname:short)") + if proc.returncode != 0: + raise GitUpdateError((proc.stderr or "git branch failed").strip()) + # refs/remotes/origin/HEAD shortens to a bare "origin", which is not a branch and + # fails on checkout — drop anything that is just a remote's name. + remotes = _remote_names() + names: set[str] = set() + for raw in (proc.stdout or "").splitlines(): + line = raw.strip() + if not line or line == "HEAD" or line.endswith("/HEAD") or line in remotes: + continue + if line.startswith("origin/"): + names.add(line[7:]) + elif not line.startswith("remotes/"): + names.add(line) + ordered = [b for b in _BRANCH_PRIORITY if b in names] + rest = sorted(b for b in names if b not in _BRANCH_PRIORITY) + return ordered + rest + + +def rollback_target() -> dict | None: + """The commit HEAD pointed to before its last move (an Update or Switch branch — pull() + and checkout() are the only things in this app that move HEAD), read from git's own + reflog rather than a separate state file: git already records exactly this. None if + there is no prior entry (a fresh clone) or HEAD hasn't moved since.""" + proc = _run_git("rev-parse", "--verify", "HEAD@{1}") + if proc.returncode != 0: + return None + commit = proc.stdout.strip() + if not commit or commit == _current_commit(): + return None + subject = _run_git("log", "-1", "--format=%s", commit) + return {"commit": commit, "subject": (subject.stdout or "").strip()} + + +def rollback() -> dict: + """Hard-reset to HEAD@{1} -- undoes the most recent Update or Switch branch. Refuses on + a dirty tree, same as pull()/checkout(), so nothing local is silently discarded. Does + NOT reinstall requirements for the commit being rolled back to -- an update that changed + dependencies and turned out bad may need `pip install -r requirements.txt` run by hand + after rolling back the code; automatically reversing a pip install is its own can of + worms (downgrades can break OTHER already-installed packages) and out of scope here.""" + if _is_dirty(): + raise GitUpdateError("Working tree has local changes. Commit or stash them before rolling back.") + target = rollback_target() + if target is None: + raise GitUpdateError("Nothing to roll back to.") + before = _current_commit() + reset = _run_git("reset", "--hard", target["commit"]) + if reset.returncode != 0: + raise GitUpdateError((reset.stderr or reset.stdout or "git reset failed").strip()) + return {"branch": _current_branch(), "before": before, "after": _current_commit()} + + +def status(*, remote: bool = True) -> dict: + """Current branch, switchable branches, and update availability. + + `remote=False` answers from the checkout alone. Everything except ahead and + behind is local and instant; the fetch is the only part that touches the + network, and on a machine that cannot reach the remote it is the only part + that can hang. The app asks twice -- once without, to have something to draw, + then once with. + + The fetch gets a short timeout of its own for the same reason: the default + 120s is a spinner nobody waits out, and a failed fetch already has an honest + answer (`fetch_ok: false`, "this may be out of date"). + """ + try: + branch = _current_branch() + commit = _current_commit() + dirty = _is_dirty() + branches = _list_branches() + fetch_ok = False + if remote: + fetch = _run_git("fetch", "--prune", "origin", timeout=20) + fetch_ok = fetch.returncode == 0 + ahead, behind = _ahead_behind(branch) if fetch_ok else (0, 0) + return { + "ok": True, + "version": funpack_version(), + "codename": funpack_codename(), + "branch": branch, + "commit": commit, + "dirty": dirty, + "branches": branches, + "ahead": ahead, + "behind": behind, + "fetch_ok": fetch_ok, + "checked_remote": bool(remote), + "repo": str(REPO_ROOT), + "rollback_target": rollback_target(), + } + except GitUpdateError as e: + return {"ok": False, "version": funpack_version(), + "codename": funpack_codename(), "detail": str(e)} + + +REQUIREMENTS = "requirements.txt" + + +def requirements_changed(before: str, after: str) -> bool: + """Did this update touch requirements.txt? + + The only honest trigger for installing: running pip on every pull would be slow and + surprising, and running it on none leaves an update that added a dependency looking + like a broken build instead of an unfinished install. + """ + if not before or not after or before == after: + return False + proc = _run_git("diff", "--name-only", f"{before}..{after}", "--", REQUIREMENTS) + if proc.returncode != 0: + # Cannot tell — install rather than skip. A redundant pip run costs seconds; a + # skipped one costs a broken node pack and a confusing error. + return True + return bool((proc.stdout or "").strip()) + + +def parse_requirement(line: str): + """(name, specifier) for one requirements line, or None for a blank/comment. + + Deliberately small: FunPack's requirements are plain names with at most a version floor. + Anything with an environment marker or a URL is handed back name-only, which makes it + "install if absent" and never a reason to touch what is already there. + """ + text = (line or "").split("#", 1)[0].strip() + if not text or text.startswith("-"): + return None + text = text.split(";", 1)[0].strip() # drop environment markers + if not text: + return None + m = re.match(r"^([A-Za-z0-9._-]+)\s*(\[[^\]]*\])?\s*(.*)$", text) + if not m: + return None + return m.group(1), (m.group(3) or "").strip() + + +def _installed_version(name: str): + """Installed version of `name` in THIS interpreter, or None. FunPack runs inside ComfyUI, + so importlib.metadata already sees the right environment — no pip subprocess needed.""" + try: + from importlib import metadata + return metadata.version(name) + except Exception: # noqa: BLE001 + return None + + +def requirement_status() -> dict: + """{"missing": [name], "below_floor": [(name, have, spec)], "present": [name]}. + + `below_floor` is REPORTED, never acted on. Upgrading a package the rest of ComfyUI is + already built against is how a working install stops working: torch, numpy, transformers + and the compiled extensions on top of them do not survive being moved underneath. + Deciding to upgrade is the user's call, with the command in hand. + """ + req = REPO_ROOT / REQUIREMENTS + out = {"missing": [], "below_floor": [], "present": []} + if not req.is_file(): + return out + try: + lines = req.read_text(encoding="utf-8", errors="replace").splitlines() + except OSError: + return out + for line in lines: + parsed = parse_requirement(line) + if parsed is None: + continue + name, spec = parsed + have = _installed_version(name) + if have is None: + out["missing"].append(name) + continue + out["present"].append(name) + if spec and not _satisfies(have, spec): + out["below_floor"].append((name, have, spec)) + return out + + +def _satisfies(have: str, spec: str) -> bool: + """Whether `have` meets `spec`. Unknown/unparseable -> True, because the fallback of a + false "outdated" is a warning about a package that is fine.""" + try: + from packaging.specifiers import SpecifierSet + from packaging.version import Version + return Version(have) in SpecifierSet(spec) + except Exception: # noqa: BLE001 + return True + + +def install_requirements(timeout: int = 900) -> dict: + """Install ONLY the requirements that are absent. Never upgrades anything. + + `sys.executable`, never a bare `pip`: ComfyUI is usually in a venv, and the pip on PATH + belongs to whatever else is on it. Installing into the wrong environment succeeds loudly + and changes nothing. + + A package that is present but below a version floor is reported, not moved. `pip install + -r requirements.txt` would upgrade it — and on a rental full of compiled extensions + (torch, comfy-kitchen, comfy-aimdo, onnxruntime, opencv) a numpy or transformers bump + underneath them does not raise. It segfaults, or corrupts memory, hours later, with + nothing in the log connecting it to the update that caused it. + + Never raises. A failed install must not turn a completed update into an error — the code + IS updated by this point, and the remedy is a command the user can run. + """ + req = REPO_ROOT / REQUIREMENTS + if not req.is_file(): + return {"ran": False, "ok": True, "detail": "no requirements.txt in this checkout"} + status = requirement_status() + floor_note = "" + if status["below_floor"]: + floor_note = ("Present but older than FunPack asks for, and LEFT ALONE — upgrading a " + "package the rest of ComfyUI is built against is not something an " + "update should do behind you:\n" + + "\n".join(f" {n}: have {h}, wants {sp}" + for n, h, sp in status["below_floor"]) + + f"\n\nTo do it yourself:\n {sys.executable} -m pip install -r {req}") + print(f"[FunPack update] {floor_note}") + if not status["missing"]: + return {"ran": False, "ok": True, "changed": [], + "detail": ("Nothing to install — every requirement is already present." + + (f"\n\n{floor_note}" if floor_note else ""))} + + before = _pip_freeze() + # Only the absent ones, by name — never `-r requirements.txt`, which would upgrade + # anything below its floor. Dependencies are resolved normally: a new package with + # nothing under it is not installed, it is broken. The freeze diff below is what says + # whether anything already present moved as a result. + cmd = [sys.executable, "-m", "pip", "install", "--disable-pip-version-check", + *status["missing"]] + try: + proc = subprocess.run(cmd, cwd=REPO_ROOT, capture_output=True, text=True, + timeout=timeout) + except subprocess.TimeoutExpired: + return {"ran": True, "ok": False, + "detail": f"pip did not finish within {timeout}s — run it yourself: " + f"pip install {' '.join(status['missing'])}"} + except OSError as e: + return {"ran": True, "ok": False, "detail": f"could not run pip: {e}"} + if proc.returncode != 0: + tail = ((proc.stderr or proc.stdout or "").strip() or "pip failed")[-800:] + return {"ran": True, "ok": False, + "detail": f"pip install failed — run it yourself:\n" + f" {sys.executable} -m pip install " + f"{' '.join(status['missing'])}\n\n{tail}"} + changed = _pip_diff(before, _pip_freeze()) + if changed: + print("[FunPack update] pip changed these packages: " + ", ".join(changed)) + return {"ran": True, "ok": True, "changed": changed, + "detail": (f"Installed missing: {', '.join(status['missing'])}\n" + + (("Changed: " + ", ".join(changed) + "\n") if changed else "") + + (f"\n{floor_note}\n" if floor_note else "") + + (proc.stdout or "").strip()[-800:])} + + +def _pip_freeze() -> dict: + """{name: version} for the environment ComfyUI is running in. {} if pip cannot be read — + an unreadable freeze must never block the install it was only meant to describe.""" + try: + out = subprocess.run([sys.executable, "-m", "pip", "freeze", + "--disable-pip-version-check"], + capture_output=True, text=True, timeout=120) + if out.returncode != 0: + return {} + found = {} + for line in (out.stdout or "").splitlines(): + if "==" in line: + name, _, ver = line.partition("==") + found[name.strip().lower()] = ver.strip() + return found + except Exception: # noqa: BLE001 + return {} + + +def _pip_diff(before: dict, after: dict) -> list[str]: + """Human-readable list of what moved. Empty when nothing did, or when either side is + unknown — reporting every package as "new" because the freeze failed would be worse + than saying nothing.""" + if not before or not after: + return [] + out = [] + for name, ver in sorted(after.items()): + was = before.get(name) + if was is None: + out.append(f"{name} {ver} (new)") + elif was != ver: + out.append(f"{name} {was} -> {ver}") + for name in sorted(set(before) - set(after)): + out.append(f"{name} {before[name]} (removed)") + return out + + +def pull(branch: str | None = None, *, install_deps: bool = False) -> dict: + """Fast-forward pull from origin on the given branch (current if omitted). + + `install_deps` is OFF by default and turned on by the update route. Running pip is a + side effect no caller should acquire by accident just for asking git to move a branch — + it has to be asked for at the point that means "the user pressed Update". + """ + branch = (branch or _current_branch()).strip() + if not branch: + raise GitUpdateError("Could not determine current branch.") + if _is_dirty(): + raise GitUpdateError("Working tree has local changes. Commit or stash them before updating.") + before = _current_commit() + fetch = _run_git("fetch", "--prune", "origin") + if fetch.returncode != 0: + raise GitUpdateError((fetch.stderr or fetch.stdout or "git fetch failed").strip()) + if branch != _current_branch(): + co = _run_git("checkout", branch) + if co.returncode != 0: + raise GitUpdateError((co.stderr or co.stdout or "git checkout failed").strip()) + pull_proc = _run_git("pull", "--ff-only", "origin", branch) + realigned = False + if pull_proc.returncode != 0: + msg = (pull_proc.stderr or pull_proc.stdout or "git pull failed").strip() + # A fast-forward is impossible once upstream history has been rewritten: every + # local commit is a different hash, so git reports the branches as diverged even + # though the content matches. Only realign when nothing would actually be lost — + # a clean tree (checked above) and no local commit whose content is missing + # upstream. Anything else keeps the original error and the user's work. + unique = _unique_local_commits(branch) + if unique: + raise GitUpdateError( + f"{msg}\n\nThis checkout has {len(unique)} local commit(s) that are not on " + f"origin/{branch}. Push or move them first — updating would discard them.") + reset = _run_git("reset", "--hard", f"origin/{branch}") + if reset.returncode != 0: + raise GitUpdateError(msg) + realigned = True + after = _current_commit() + # Dependencies are part of the update. Done BEFORE the response, so the restart the + # caller schedules cannot race an install that is still running. + deps = None + if install_deps and before != after and requirements_changed(before, after): + deps = install_requirements() + return { + "branch": branch, + "before": before, + "after": after, + "updated": before != after, + "requirements": deps, + # Surfaced so the update reads as what it was, not a silent jump to another commit. + "realigned": realigned, + "output": ((pull_proc.stdout or "").strip() if not realigned + else f"Upstream history was rewritten — realigned to origin/{branch}. " + "No local commits were lost."), + } + + +def checkout(branch: str, *, pull_after: bool = True, install_deps: bool = False) -> dict: + """Switch branch, optionally pull, return combined result.""" + branch = (branch or "").strip() + if not branch: + raise GitUpdateError("Branch name is required.") + branches = _list_branches() + if branch not in branches: + raise GitUpdateError(f'Branch "{branch}" is not available locally or on origin.') + if _is_dirty(): + raise GitUpdateError("Working tree has local changes. Commit or stash them before switching branches.") + before_branch = _current_branch() + before_commit = _current_commit() + if branch != before_branch: + co = _run_git("checkout", branch) + if co.returncode != 0: + raise GitUpdateError((co.stderr or co.stdout or "git checkout failed").strip()) + result = {"branch": branch, "before_branch": before_branch, "before": before_commit} + if pull_after: + pulled = pull(branch, install_deps=install_deps) + result.update(pulled) + else: + result["after"] = _current_commit() + result["updated"] = result["before"] != result["after"] + # A branch switch alone can cross a requirements change just as a pull can — the + # checkout above already moved the working tree onto the other branch's files. + if install_deps and result["updated"] and requirements_changed(before_commit, + result["after"]): + result["requirements"] = install_requirements() + return result