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)