From ca913a3dec3230989d49880d8ec7667d52ef711c Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Sun, 6 Sep 2026 14:05:03 +0300 Subject: [PATCH] core, app: it restarted when nothing had changed, and could not see an update Both findings from the adversarial pass, confirmed and fixed. RESTARTING FOR NOTHING. Every successful git call scheduled a restart, whether or not the checkout had moved. Pressing Update while already up to date is a normal thing to do -- there is no way to know until it has been asked -- and it cost a full boot. The route now restarts only when something actually moved: a commit changed, or a branch did. Two branches at the same commit still count, because they stop being the same the moment anything lands on either. And the worse half, which the pass rated first: nothing checked whether a generation was in flight. The restart takes the run with it, GPU time already spent, with no warning anywhere. The window now says a run is going and turns off the three buttons that would end it. CHECK FOR UPDATES COULD NOT REPORT ONE. check_updates() answers under `checked` and the app read `nodes`. The request succeeded, the JSON was valid, and every pack reported nothing to update however far behind it was -- a fetch per pack over the network for an answer that could never be shown. Neither side's tests could see it: the route test asserted the server's shape, the window test never pressed the button, and nothing compared the two. So the tests now name each other. The Python one asserts the key the app reads; the browser one stubs the server's real shape and reads the words off the row. --- app/boot.js | 4 ++- app/shell/packs.js | 8 +++-- app/shell/updates.js | 24 +++++++++++--- app/tests-layout/packs.spec.js | 45 +++++++++++++++++++++++++ app/tests-layout/updates.spec.js | 37 +++++++++++++++++++++ core/routes.py | 16 ++++++++- core/tests/test_routes_update.py | 56 ++++++++++++++++++++++++++++++++ 7 files changed, 181 insertions(+), 9 deletions(-) diff --git a/app/boot.js b/app/boot.js index 5184b68e..6f650a55 100644 --- a/app/boot.js +++ b/app/boot.js @@ -108,7 +108,9 @@ async function start() { onGenerate: () => { ranFor = project.selectedId; session.generate(); }, onCancel: () => run.cancel(), onConstructor: () => page.constructor.open(), - onUpdates: () => openUpdates(), + // The window asks whether a run is in flight, because the restart that + // follows an update would take it with it. + onUpdates: () => openUpdates({ running: () => ["queued", "running"].includes(run.state.phase) }), onPacks: () => openPacks(), // What the Edit menu offers, and what the keyboard reaches. One list, so a // menu item and its shortcut cannot drift apart. diff --git a/app/shell/packs.js b/app/shell/packs.js index 8e918001..4bc49bd0 100644 --- a/app/shell/packs.js +++ b/app/shell/packs.js @@ -68,7 +68,8 @@ export function open() { placeholder: "https://github.com/owner/pack" }); out.push(composer.settingsRow.default({ label: "Install a pack", - hint: "Anything git can clone. ComfyUI restarts itself afterwards if it must.", + hint: "Anything git can clone. ComfyUI has to be restarted before a new pack " + + "loads — Settings ▸ Updates can do that.", control: composer.button.md({ label: busy === "*" ? "Installing…" : "Install", disabled: Boolean(busy), @@ -141,7 +142,10 @@ export function open() { busy = "*check*"; draw(); problem = null; - try { checked = (await ask("POST", "/check")).nodes || null; } + // `checked`, which is the key the server sends. It said `nodes` here for a + // while: the request succeeded, the JSON was valid, and every pack reported + // nothing to update no matter how far behind it was. + try { checked = (await ask("POST", "/check")).checked || null; } catch (err) { problem = err.message; } finally { busy = null; draw(); } } diff --git a/app/shell/updates.js b/app/shell/updates.js index 7ffea647..70c9653d 100644 --- a/app/shell/updates.js +++ b/app/shell/updates.js @@ -70,7 +70,7 @@ export function waitForRestart({ poll = 1500, give_up = 180000 } = {}) { * 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 } = {}) { +export function open({ onRestart = waitForRestart, running = () => false } = {}) { let window_ = null; const body = composer.region.stack({ gap: "sm", label: "Updates" }); @@ -78,7 +78,11 @@ export function open({ onRestart = waitForRestart } = {}) { body.set([composer.hint.default({ text: "Working. This can take a few minutes if " + "requirements changed." })]); try { - await run(); + const result = await run(); + // The server only restarts when the checkout actually moved. Pressing + // Update while already up to date is a normal thing to do, and waiting for + // a restart that is not coming would hang on an overlay for three minutes. + if (!result || result.restarting === false) { load(); return; } if (window_) window_.close("restarting"); onRestart(); } catch (err) { @@ -103,6 +107,16 @@ export function open({ onRestart = waitForRestart } = {}) { control: composer.text.sm({ text: s.branch || "?" }) }), ]; + // A run in flight is killed by the restart, without warning and with the GPU + // time already spent. The window will not do that behind someone's back. + const busy = running(); + if (busy) { + rows.push(composer.banner.warn({ + text: "A generation is running. Updating or switching branch restarts ComfyUI " + + "and stops it. Wait for it to finish, or cancel it first.", + })); + } + if (s.dirty) { // The one state that blocks both actions, said before either is pressed // rather than as a refusal afterwards. @@ -120,7 +134,7 @@ export function open({ onRestart = waitForRestart } = {}) { 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), + disabled: Boolean(s.dirty) || busy, }), })); @@ -128,7 +142,7 @@ export function open({ onRestart = waitForRestart } = {}) { 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", + control: composer.button.md({ label: "Roll back", tone: "danger", disabled: busy, onClick: () => act(() => ask("POST", "/rollback")) }), })); } @@ -142,7 +156,7 @@ export function open({ onRestart = waitForRestart } = {}) { onClick: () => window_.close("done") }), composer.button.md({ label: s.behind ? `Update (${s.behind})` : "Update", tone: "primary", - disabled: !s.ok || Boolean(s.dirty), + disabled: !s.ok || Boolean(s.dirty) || busy, onClick: () => act(() => ask("POST", "/update", {})), }), ], diff --git a/app/tests-layout/packs.spec.js b/app/tests-layout/packs.spec.js index 0d167e72..b5d02bcc 100644 --- a/app/tests-layout/packs.spec.js +++ b/app/tests-layout/packs.spec.js @@ -96,3 +96,48 @@ test("a refusal from the server is shown in the window", async ({ page }) => { .getByRole("button", { name: "Remove" }).click(); await expect(page.locator(".cx-modal")).toContainText("That is FunPack itself."); }); + +test("checking for updates shows which packs are behind", async ({ page }) => { + // The client read `nodes` off a response whose key is `checked`: the request + // succeeded, the JSON was valid, and every pack reported nothing to update + // however far behind it was. The stub below is the server's real shape. + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + await page.route("**/api/packs", (route) => route.fulfill({ + json: { root: "/x/custom_nodes", nodes: [ + { name: "ComfyUI-Behind", is_funpack: false, git: true, branch: "main", commit: "aaa" }, + { name: "ComfyUI-Current", is_funpack: false, git: true, branch: "main", commit: "bbb" }, + ] }, + })); + await page.route("**/api/packs/check", (route) => route.fulfill({ + json: { checked: { + "ComfyUI-Behind": { checked: true, branch: "main", ahead: 0, behind: 3 }, + "ComfyUI-Current": { checked: true, branch: "main", ahead: 0, behind: 0 }, + } }, + })); + + await openPacks(page); + await page.getByRole("button", { name: "Check for updates" }).click(); + + await expect(page.locator(".cx-settings-row", { hasText: "ComfyUI-Behind" })) + .toContainText("3 updates available"); + await expect(page.locator(".cx-settings-row", { hasText: "ComfyUI-Current" })) + .toContainText("up to date"); +}); + +test("a pack that could not be checked says why, not 'up to date'", async ({ page }) => { + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + await page.route("**/api/packs", (route) => route.fulfill({ + json: { root: "/x/custom_nodes", nodes: [ + { name: "ComfyUI-Offline", is_funpack: false, git: true, branch: "main" }] }, + })); + await page.route("**/api/packs/check", (route) => route.fulfill({ + json: { checked: { "ComfyUI-Offline": { checked: false, reason: "could not reach origin" } } }, + })); + + await openPacks(page); + await page.getByRole("button", { name: "Check for updates" }).click(); + await expect(page.locator(".cx-settings-row", { hasText: "ComfyUI-Offline" })) + .toContainText("could not reach origin"); +}); diff --git a/app/tests-layout/updates.spec.js b/app/tests-layout/updates.spec.js index 426f6f12..56dc36c3 100644 --- a/app/tests-layout/updates.spec.js +++ b/app/tests-layout/updates.spec.js @@ -101,3 +101,40 @@ test("an install that is not a git checkout says so instead of offering to updat await expect(page.locator(".cx-modal")).toContainText(/not a git checkout/i); await expect(page.locator(".cx-modal").getByRole("button", { name: /Update/ })).toBeDisabled(); }); + +test("an update that changed nothing does not wait for a restart", async ({ page }) => { + // The server only restarts when the checkout moved. Waiting for one that is + // not coming is three minutes of an overlay. + 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"], dirty: false, + ahead: 0, behind: 0, fetch_ok: true, checked_remote: true, repo: "/x" }, + })); + await page.route("**/api/git/update", (route) => route.fulfill({ + json: { restarting: false, updated: false }, + })); + + await openUpdates(page); + await page.getByRole("button", { name: /Update/ }).click(); + + await expect(page.locator(".cx-blocking"), "it waited for a restart that never comes") + .toHaveCount(0); + await expect(page.locator(".cx-modal")).toContainText("Up to date"); +}); + +test("a generation in flight is said, and the buttons that would kill it are off", async ({ page }) => { + // The restart takes the run with it, with the GPU time already spent. + 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: 4, fetch_ok: true, checked_remote: true, repo: "/x" }, + })); + // A run this page believes is its own and in flight. + await page.evaluate(() => window.FunPack.run.adopt("pretend-prompt-id")); + + await openUpdates(page); + await expect(page.locator(".cx-modal")).toContainText(/generation is running/i); + await expect(page.getByRole("button", { name: /Update \(4\)/ })).toBeDisabled(); +}); diff --git a/core/routes.py b/core/routes.py index e639f696..bd0e390b 100644 --- a/core/routes.py +++ b/core/routes.py @@ -262,11 +262,25 @@ def register(routes, prefix=None): log.broke("git", exc, doing=getattr(action, "__name__", "git")) return web.json_response({"detail": f"{type(exc).__name__}: {exc}"}, status=500) + result = result or {} + # ONLY when the checkout actually moved. Pressing Update while already up + # to date is a normal thing to do -- there is no way to know until it has + # been asked -- and restarting for it costs a boot and, if a generation + # is running, the generation. + moved = bool(result.get("updated")) or ( + result.get("before") is not None and result.get("before") != result.get("after") + ) or ( + result.get("before_branch") is not None + and result.get("before_branch") != result.get("branch") + ) + if not moved: + return web.json_response({"restarting": False, **result}) + 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 {})}) + return web.json_response({"restarting": True, **result}) @routes.get(P + "/api/git/status") async def _git_status(req): diff --git a/core/tests/test_routes_update.py b/core/tests/test_routes_update.py index aa57d246..beab7999 100644 --- a/core/tests/test_routes_update.py +++ b/core/tests/test_routes_update.py @@ -8,6 +8,7 @@ they replace it, and a suite that did not would relaunch the test runner. import json import socket import threading +import time import pytest @@ -219,3 +220,58 @@ def test_an_install_outside_comfyui_says_it_cannot_tell_where_packs_live(server, assert status == 400, body assert "custom_nodes" in body["detail"] assert "/tmp/somewhere" in body["detail"], "it did not say where it thinks it is" + + +def test_an_update_that_changed_nothing_does_not_restart(server, monkeypatch): + """Pressing Update while already up to date is a normal thing to do -- there + is no way to know until it has been asked. Restarting for it costs a boot, + and a generation if one is running.""" + monkeypatch.setattr(update_mod, "pull", + lambda **_k: {"updated": False, "before": "a1", "after": "a1"}) + + status, body = _request(server["port"], "POST", "/funpack/api/git/update", {}) + assert status == 200, body + assert body["restarting"] is False + assert server["restarts"] == [], "restarted for a checkout that had not moved" + + +def test_a_branch_switch_restarts_even_when_the_commit_is_the_same(server, monkeypatch): + """Two branches can point at the same commit and still be different code + once anything is committed to either.""" + monkeypatch.setattr(update_mod, "checkout", + lambda **_k: {"updated": False, "before": "a1", "after": "a1", + "before_branch": "v5", "branch": "dev"}) + + status, body = _request(server["port"], "POST", "/funpack/api/git/checkout", + {"branch": "dev"}) + assert status == 200, body + assert body["restarting"] is True + # Scheduled 0.7s AFTER the answer is written, which is the whole point: the + # process is about to go and the client needs the reply first. + time.sleep(1.0) + assert server["restarts"] == [1] + + +def test_a_rollback_restarts_because_the_commit_moved(server, monkeypatch): + monkeypatch.setattr(update_mod, "rollback", + lambda: {"branch": "v5", "before": "b2", "after": "a1"}) + + status, body = _request(server["port"], "POST", "/funpack/api/git/rollback") + assert status == 200, body + assert body["restarting"] is True + time.sleep(1.0) + assert server["restarts"] == [1] + + +def test_the_pack_check_answers_under_the_key_the_app_reads(server): + """The app reads `checked`. It read `nodes` for a while: the request + succeeded, the JSON was valid, and every pack reported nothing to update + however far behind it was. Neither side's tests could see it alone.""" + from core import nodes_manager + shape = nodes_manager.check_updates.__doc__ + assert shape, "check_updates lost its docstring" + + status, body = _request(server["port"], "POST", "/funpack/api/packs/check") + assert status == 200, body + assert "checked" in body, f"the key the app reads is not in {sorted(body)}" + assert isinstance(body["checked"], dict)