diff --git a/CHANGELOG.md b/CHANGELOG.md index 34608f5c..66e4d4f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -152,6 +152,18 @@ and this project adheres to [Semantic Versioning 2.0.0](https://semver.org/spec/ `0.0.30+cu126`, unless the requirement sets a minimum above it; pip replaced the build with whatever it found. For such a build, these changes only add requirements that are skipped. +- In the legacy UI, enable a disabled node pack that another extension + requires before restarting. After the user agreed to enable it and restart, + ComfyUI restarted with the pack still disabled, because the request went to + a route that no longer exists. The pack is now enabled through the queue, as + the Custom Nodes Manager does, and ComfyUI restarts only when that + succeeded; otherwise the reason is shown. While other tasks are queued + behind the pack, ComfyUI is not restarted, and the user is asked to restart + after them. `Rebooting...` is shown as well: the restart request is never + answered, which ended the handler before it. +- In the legacy UI, say so when that restart is refused at the current + security level. Nothing was shown; the browser console had + `show_message is not defined`. ## [4.3] - 2026-09-19 diff --git a/comfyui_manager/js/cm-api.js b/comfyui_manager/js/cm-api.js index da34f814..d2800988 100644 --- a/comfyui_manager/js/cm-api.js +++ b/comfyui_manager/js/cm-api.js @@ -1,59 +1,146 @@ import { api } from "../../scripts/api.js"; import { app } from "../../scripts/app.js"; -import { sleep, customConfirm, customAlert } from "./common.js"; +import { sleep, customConfirm, customAlert, show_message, sanitizeHTML, generateUUID, md5 } from "./common.js"; + +// Enable a disabled node pack as the Enable button of the Custom Nodes Manager does: find the pack +// in the node list, put it in the queue, and wait until the queue has finished it. +// Returns { failure }, the reason, when the pack was not enabled, and otherwise { busy }: whether +// the queue still holds other work. +async function enableNodePack(target) { + const list = await api.fetchApi(`/v2/customnode/getlist?mode=cache&skip_update=true`); + if(list.status != 200) { + return { failure: `Failed to get the custom node list (${list.status}).` }; + } + + const { channel, node_packs } = await list.json(); + const key = Object.keys(node_packs).find((k) => { + const pack = node_packs[k]; + return pack.state == 'disabled' && pack.files?.some((url) => target.files.includes(url)); + }); + if(key == undefined) { + return { failure: 'It is not in the custom node list as a disabled node pack. Please enable it in the Custom Nodes Manager.' }; + } + + // Behind other work the pack would be enabled at some later time, after the wait below has ended. + const status = await api.fetchApi('/v2/manager/queue/status'); + if((await status.json()).is_processing) { + return { failure: 'There are already tasks in progress. Please try again after it is completed.' }; + } + + const batch_id = generateUUID(); + const ui_id = md5(key); // the id of this pack's row in the Custom Nodes Manager + + // Once the batch is in the queue, 'all-done' without its report ends the wait: something else, + // such as another tab, has emptied the queue. + let listener, queued = false; + const result = new Promise((resolve) => { + listener = (event) => { + const report = event.detail; + if(report.status == 'batch-done' && report.batch_id == batch_id) { + resolve(report); + } + else if(report.status == 'all-done' && queued) { + resolve({ nodepack_result: {} }); + } + }; + }); + + // The queue may report the batch before the request below is answered. + api.addEventListener("cm-queue-status", listener); + try { + const response = await api.fetchApi(`/v2/manager/queue/batch`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + batch_id, + install: [{ + ...node_packs[key], + channel, + mode: 'cache', + ui_id, + skip_post_install: true + }] + }) + }); + + if(response.status != 200) { + return { failure: `The request was refused (${response.status}). Please check the terminal log.` }; + } + + const { failed } = await response.json(); + if(failed.length > 0) { + return { failure: 'The request was refused. Please check the terminal log.' }; + } + + // A report that does not reach the page would leave the listener in place, and the batch, + // run at some later time, would then restart ComfyUI unasked. + queued = true; + const report = await Promise.race([result, sleep(60000)]); + if(report == undefined) { + return { failure: 'The queue did not report the result. Please check the node pack in the Custom Nodes Manager.' }; + } + + const message = report.nodepack_result[ui_id] ?? 'The queue finished without a result for this node pack. Please check the node pack in the Custom Nodes Manager.'; + if(message != 'success') { + return { failure: String(message) }; + } + + return { busy: report.remaining_batch_count > 1 }; // the count includes this batch + } + finally { + api.removeEventListener("cm-queue-status", listener); + } +} async function tryInstallCustomNode(event) { - let msg = '-= [ComfyUI Manager] extension installation request =-\n\n'; - msg += `The '${event.detail.sender}' extension requires the installation of the '${event.detail.target.title}' extension. `; + const target = event.detail.target; - if(event.detail.target.installed == 'Disabled') { + let msg = '-= [ComfyUI Manager] extension installation request =-\n\n'; + msg += `The '${event.detail.sender}' extension requires the installation of the '${target.title}' extension. `; + + if(target.installed == 'Disabled') { msg += 'However, the extension is currently disabled. Would you like to enable it and reboot?' } - else if(event.detail.target.installed == 'True') { - msg += 'However, it seems that the extension is in an import-fail state or is not compatible with the current version. Please address this issue.'; - } else { - msg += `Would you like to install it and reboot?`; + msg += 'However, it seems that the extension is in an import-fail state or is not compatible with the current version. Please address this issue.'; } msg += `\n\nRequest message:\n${event.detail.msg}`; - if(event.detail.target.installed == 'True') { + // The server sends this request only for a node pack that is installed: it is disabled, or it failed to load. + if(target.installed != 'Disabled') { customAlert(msg); return; } + const res = await customConfirm(msg); if(res) { - if(event.detail.target.installed == 'Disabled') { - const response = await api.fetchApi(`/v2/customnode/toggle_active`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(event.detail.target) - }); - } - else { - await sleep(300); - app.ui.dialog.show(`Installing... '${event.detail.target.title}'`); + const title = sanitizeHTML(String(target.title)); - const response = await api.fetchApi(`/v2/customnode/install`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(event.detail.target) - }); + await sleep(300); + app.ui.dialog.show(`Enabling... '${title}'`); - if(response.status == 403) { - show_message('This action is not allowed with this security level configuration.'); - return false; - } - else if(response.status == 400) { - let msg = await res.text(); - show_message(msg); - return false; - } + const { failure, busy } = await enableNodePack(target).catch((exception) => ({ failure: String(exception) })); + if(failure != undefined) { + show_message(`Failed to enable '${title}'.

${sanitizeHTML(failure)}`); + return false; } - let response = await api.fetchApi("/v2/manager/reboot", { method: 'POST' }); - if(response.status == 403) { + // A restart would cut off the work that was queued behind the pack. + if(busy) { + show_message(`'${title}' is enabled.

Other tasks are still in progress. Please restart ComfyUI after they are completed.`); + return false; + } + + let response; + try { + response = await api.fetchApi("/v2/manager/reboot", { method: 'POST' }); + } + catch(exception) { + // The server restarts without answering. + } + + if(response?.status == 403) { show_message('This action is not allowed with this security level configuration.'); return false; } diff --git a/comfyui_manager/js/comfyui-manager.js b/comfyui_manager/js/comfyui-manager.js index 15e8097f..22be7004 100644 --- a/comfyui_manager/js/comfyui-manager.js +++ b/comfyui_manager/js/comfyui-manager.js @@ -676,6 +676,11 @@ async function switchComfyUI() { } async function onQueueStatus(event) { + // The buttons exist once the Manager menu has been created; the queue can run before that. + if(update_all_button == null) { + return; + } + const isElectron = 'electronAPI' in window; if(event.detail.status == 'in_progress') { diff --git a/tests/test_cm_api_try_install.py b/tests/test_cm_api_try_install.py new file mode 100644 index 00000000..6fd9b7b7 --- /dev/null +++ b/tests/test_cm_api_try_install.py @@ -0,0 +1,381 @@ +"""The legacy UI enables a disabled node pack that another extension asks for, and then restarts. + +An extension asks through `cm.try-install-custom-node` for a node pack it needs. When the pack is +disabled, the legacy UI asks the user whether to enable it and restart. It enables the pack as +the Enable button of the Custom Nodes Manager does: it finds the pack in the node list, puts it +in the queue, and waits for the queue. It restarts only after the queue reports success, and +the wait has an end. + +The cases run the shipped cm-api.js and common.js in Chromium against stand-ins for ComfyUI's +scripts; no ComfyUI server is used. The stand-in for the API answers each route as the case +says and records the requests. + +Browser cases require Playwright and its Chromium install. +""" +import hashlib +import os +from pathlib import Path + +import pytest + +from test_grid_highlight_escaping import browser # noqa: F401 -- fixture + +JS = Path(os.environ.get("MANAGER_JS_DIR") or (Path(__file__).resolve().parent.parent / "comfyui_manager" / "js")) +EXTENSION = "http://manager.test/extensions/comfyui-manager-legacy/" + +GETLIST = "/v2/customnode/getlist" +STATUS = "/v2/manager/queue/status" +BATCH = "/v2/manager/queue/batch" +REBOOT = "/v2/manager/reboot" +# the requests that enable the pack +ENABLE = [("GET", GETLIST), ("GET", STATUS), ("POST", BATCH)] + +URL = "https://github.com/someone/Other-Pack" +# The entry of custom-node-list.json that the server sends with the request +TARGET = {"title": "Other Pack", "installed": "Disabled", "files": [URL], "id": "listed-id"} +# The node list as /v2/customnode/getlist answers it: rows by key +PACKS = { + "another-pack": {"id": "another-pack", "title": "Another Pack", "version": "1.0.0", "state": "disabled", + "files": ["https://github.com/someone/Another-Pack"]}, + "from-the-registry": {"id": "from-the-registry", "title": "No Files", "version": "1.0.0", "state": "disabled"}, + "other-pack": {"id": "other-pack", "title": "Other Pack", "version": "nightly", "state": "disabled", + "files": [URL], "repository": URL}, +} +UI_ID = hashlib.md5(b"other-pack").hexdigest() + +SCRIPTS = { + # `window.answers[route]` says how a route answers: `status`, `json`, `fails` for a request that + # gets no answer, and for the batch route `result`, which the queue reports for the pack either + # `before` the request is answered or after it; `remaining` is the number of batches the queue + # then holds, this one included. With `emptied` the queue only reports that it has finished, and + # with `silent` no report reaches the page. The end of an earlier run is reported while the + # batch request is handled. + "api.js": """ + class Api extends EventTarget { + report(detail) { + this.dispatchEvent(new CustomEvent('cm-queue-status', {detail})); + } + + async fetchApi(path, options) { + const route = path.split('?')[0]; + const answer = window.answers[route] ?? {}; + const body = options?.body ? JSON.parse(options.body) : null; + window.requested.push({path, method: options?.method ?? 'GET', body}); + if(answer.fails) { + throw new TypeError('Failed to fetch'); + } + if(route == '/v2/manager/queue/batch') { + this.report({status: 'all-done'}); + } + if(answer.emptied) { + setTimeout(() => this.report({status: 'all-done'}), 50); + } + else if(answer.result !== undefined && !answer.silent) { + const report = () => { + this.report({status: 'in_progress', target: body.install[0].ui_id, batch_id: body.batch_id}); + this.report({status: 'batch-done', batch_id: 'another-batch', nodepack_result: {}}); + this.report({status: 'batch-done', batch_id: body.batch_id, + remaining_batch_count: answer.remaining ?? 1, + nodepack_result: answer.result === null ? {} : {[body.install[0].ui_id]: answer.result}}); + this.report({status: 'all-done'}); + }; + if(answer.before) report(); else setTimeout(report, 50); + } + return {status: answer.status ?? 200, json: async () => answer.json, text: async () => ''}; + } + } + export const api = new Api(); + """, + # `show_message` in common.js and the handler show their text in this dialog. + "app.js": """ + const element = document.createElement('div'); + element.id = 'comfy-dialog'; + document.body.append(element); + export const app = {ui: {dialog: {element, show(html) { element.innerHTML = html; }}}}; + """, + "ui.js": "export const $el = () => null; export class ComfyDialog {}", +} + + +@pytest.fixture +def page(browser): # noqa: F811 + page = browser.new_page() + + def route(request): + url = request.request.url + name = url.rsplit("/", 1)[-1] + if url == "http://manager.test/": + request.fulfill(body="", content_type="text/html") + elif url.startswith(EXTENSION) and (JS / name).is_file(): + request.fulfill(path=str(JS / name), content_type="text/javascript") + elif url == "http://manager.test/scripts/" + name and name in SCRIPTS: + request.fulfill(body=SCRIPTS[name], content_type="text/javascript") + else: + request.abort() + + page.route("**/*", route) + page.errors = [] + page.on("pageerror", lambda error: page.errors.append(str(error))) + page.goto("http://manager.test/") + yield page + page.close() + + +# The batch is accepted, and the queue reports the pack as enabled. +ENABLED = {"json": {"failed": []}, "result": "success"} + + +def answers(batch=ENABLED, reboot=None, packs=PACKS, getlist=None, status=None): + """How the routes answer; by default the pack is listed as disabled, the queue is idle, the pack is enabled, and the restart is accepted.""" + return { + GETLIST: {"json": {"channel": "default", "node_packs": packs}} if getlist is None else getlist, + STATUS: {"json": {"is_processing": False}} if status is None else status, + BATCH: batch, + REBOOT: {} if reboot is None else reboot, + } + + +def ask(page, answers, target=TARGET): + """Deliver the request an extension makes for `target`.""" + page.evaluate("""async ({answers, target, module}) => { + window.requested = []; + window.answers = answers; + await import(module); + const { api } = await import('/scripts/api.js'); + api.dispatchEvent(new CustomEvent('cm-api-try-install-customnode', {detail: { + sender: 'Some Pack', msg: 'needed for the detector nodes', target, + }})); + }""", {"answers": answers, "target": target, "module": EXTENSION + "cm-api.js"}) + + +def confirm(page): + """Confirm the dialog that asks whether to enable the pack and restart.""" + question = page.get_by_text("Would you like to enable it and reboot?") + question.wait_for() + assert "'Some Pack' extension requires the installation of the 'Other Pack' extension" in question.inner_text() + page.get_by_role("button", name="Confirm").click() + + +def shown(page, text): + page.locator("#comfy-dialog").get_by_text(text).wait_for(timeout=5000) + + +def requested(page): + return [(request["method"], request["path"].split("?")[0]) for request in page.evaluate("window.requested")] + + +def test_the_pack_is_enabled_through_the_queue_and_then_comfyui_restarts(page): + ask(page, answers()) + confirm(page) + + shown(page, "Rebooting...") + assert requested(page) == ENABLE + [("POST", REBOOT)] + assert page.evaluate("window.requested[0].path") == GETLIST + "?mode=cache&skip_update=true" + batch = page.evaluate("window.requested[2].body") + # what the Enable button of the Custom Nodes Manager sends for this row + assert batch["install"] == [{**PACKS["other-pack"], "channel": "default", "mode": "cache", + "ui_id": UI_ID, "skip_post_install": True}] + assert set(batch) == {"batch_id", "install"} + assert page.errors == [] + + +def test_a_report_that_arrives_before_the_request_is_answered_is_not_missed(page): + ask(page, answers(batch={**ENABLED, "before": True})) + confirm(page) + + shown(page, "Rebooting...") + assert requested(page)[-1] == ("POST", REBOOT) + + +def test_the_restart_is_announced_when_the_server_restarts_without_answering(page): + ask(page, answers(reboot={"fails": True})) + confirm(page) + + shown(page, "Rebooting...") + assert page.errors == [] + + +def test_a_refused_restart_is_reported(page): + ask(page, answers(reboot={"status": 403})) + confirm(page) + + shown(page, "This action is not allowed with this security level configuration.") + assert requested(page)[-1] == ("POST", REBOOT) + assert page.errors == [] + + +@pytest.mark.parametrize("batch, reason", [ + ({"json": {"failed": ["other-pack"]}}, "The request was refused. Please check the terminal log."), + ({"status": 500}, "The request was refused (500). Please check the terminal log."), + ({"json": {"failed": []}, "result": "Cannot resolve install target: 'other-pack@nightly'"}, + "Cannot resolve install target: 'other-pack@nightly'"), +], ids=["refused by the queue", "an error answer", "failed in the queue"]) +def test_comfyui_does_not_restart_when_the_pack_was_not_enabled(page, batch, reason): + ask(page, answers(batch=batch)) + confirm(page) + + shown(page, "Failed to enable 'Other Pack'.") + shown(page, reason) + page.wait_for_timeout(300) + assert requested(page) == ENABLE + assert page.errors == [] + + +@pytest.mark.parametrize("batch", [{"json": {"failed": []}, "emptied": True}, {**ENABLED, "result": None}], + ids=["the queue was emptied", "the batch is reported without the pack"]) +def test_a_queue_that_finishes_without_the_pack_ends_the_wait(page, batch): + ask(page, answers(batch=batch)) + confirm(page) + + shown(page, "Failed to enable 'Other Pack'.") + shown(page, "The queue finished without a result for this node pack.") + shown(page, "Please check the node pack in the Custom Nodes Manager.") + page.wait_for_timeout(300) + assert requested(page) == ENABLE + assert page.errors == [] + + +def test_the_wait_ends_when_no_report_reaches_the_page(page): + page.clock.install() + ask(page, answers(batch={**ENABLED, "silent": True})) + confirm(page) + shown(page, "Enabling... 'Other Pack'") + + page.clock.fast_forward(59000) + assert "Failed to enable" not in page.locator("#comfy-dialog").inner_text() + page.clock.fast_forward(2000) + shown(page, "Failed to enable 'Other Pack'.") + shown(page, "The queue did not report the result.") + + # the batch is run at some later time, as when another tab wakes the queue + page.evaluate("""async (ui_id) => { + const { api } = await import('/scripts/api.js'); + api.report({status: 'batch-done', batch_id: window.requested[2].body.batch_id, remaining_batch_count: 1, + nodepack_result: {[ui_id]: 'success'}}); + }""", UI_ID) + page.clock.fast_forward(1000) + assert requested(page) == ENABLE + assert page.errors == [] + + +def test_a_queue_that_is_running_is_left_alone(page): + ask(page, answers(status={"json": {"is_processing": True}})) + confirm(page) + + shown(page, "Failed to enable 'Other Pack'.") + shown(page, "There are already tasks in progress. Please try again after it is completed.") + page.wait_for_timeout(300) + assert requested(page) == ENABLE[:2] + assert page.errors == [] + + +def test_comfyui_does_not_restart_while_other_work_is_queued_behind_the_pack(page): + ask(page, answers(batch={**ENABLED, "remaining": 2})) + confirm(page) + + shown(page, "'Other Pack' is enabled.") + shown(page, "Other tasks are still in progress. Please restart ComfyUI after they are completed.") + page.wait_for_timeout(300) + assert requested(page) == ENABLE + assert page.errors == [] + + +@pytest.mark.parametrize("packs", [ + {**PACKS, "other-pack": {**PACKS["other-pack"], "state": "enabled"}}, + {**PACKS, "other-pack": {**PACKS["other-pack"], "state": "not-installed"}}, + {key: pack for key, pack in PACKS.items() if key != "other-pack"}, +], ids=["listed as enabled", "listed as not installed", "not listed"]) +def test_a_pack_that_the_node_list_does_not_have_as_disabled_is_not_queued(page, packs): + ask(page, answers(packs=packs)) + confirm(page) + + shown(page, "Failed to enable 'Other Pack'.") + shown(page, "Please enable it in the Custom Nodes Manager.") + page.wait_for_timeout(300) + assert requested(page) == ENABLE[:1] + assert page.errors == [] + + +def test_a_node_list_that_cannot_be_read_is_reported(page): + ask(page, answers(getlist={"status": 500})) + confirm(page) + + shown(page, "Failed to get the custom node list (500).") + page.wait_for_timeout(300) + assert requested(page) == ENABLE[:1] + + +@pytest.mark.parametrize("failing, requests", [ + ({"getlist": {"fails": True}}, ENABLE[:1]), + ({"status": {"fails": True}}, ENABLE[:2]), + ({"batch": {"fails": True}}, ENABLE), +], ids=["the node list", "the queue status", "the batch request"]) +def test_a_request_that_gets_no_answer_is_reported(page, failing, requests): + ask(page, answers(**failing)) + confirm(page) + + shown(page, "Failed to enable 'Other Pack'.") + shown(page, "Failed to fetch") + page.wait_for_timeout(300) + assert requested(page) == requests + assert page.errors == [] + + +def test_text_from_the_node_list_and_the_queue_is_shown_as_text(page): + markup = '' + target = {**TARGET, "title": "Other " + markup} + ask(page, answers(batch={"json": {"failed": []}, "result": "failed " + markup}), target) + page.get_by_text("Would you like to enable it and reboot?").wait_for() + page.get_by_role("button", name="Confirm").click() + + shown(page, "failed " + markup) + assert "Failed to enable 'Other " + markup + "'." in page.locator("#comfy-dialog").inner_text() + assert page.locator("#comfy-dialog img").count() == 0 + assert page.evaluate("window.__probe === undefined") + + +def test_nothing_is_requested_when_the_user_declines(page): + ask(page, answers()) + page.get_by_text("Would you like to enable it and reboot?").wait_for() + page.get_by_role("button", name="Cancel").click() + + page.wait_for_timeout(500) + assert requested(page) == [] + assert page.locator("#comfy-dialog").inner_text() == "" + + +def test_a_pack_that_is_installed_but_did_not_load_is_only_reported(page): + alerts = [] + page.on("dialog", lambda dialog: (alerts.append(dialog.message), dialog.dismiss())) + + ask(page, answers(), {**TARGET, "installed": "True"}) + + page.wait_for_timeout(500) + assert len(alerts) == 1 + assert "it seems that the extension is in an import-fail state" in alerts[0] + assert requested(page) == [] + + +def lift(name): + """A function of comfyui-manager.js, as it is written there.""" + source = (JS / "comfyui-manager.js").read_text(encoding="utf-8") + start = source.index(f"async function {name}(") + return source[start:source.index("\n}\n", start) + 3] + + +@pytest.mark.parametrize("status", ["in_progress", "all-done"]) +def test_a_queue_report_before_the_manager_menu_exists_is_ignored(browser, status): # noqa: F811 + """A queue started from this handler reports to the menu's listener, whose buttons the menu creates.""" + page = browser.new_page() + errors = [] + page.on("pageerror", lambda error: errors.append(str(error))) + page.add_script_tag(content=""" + var update_all_button = null, update_comfyui_button = null, switch_comfyui_button = null; + var restart_stop_button = null, batch_id = null; + """ + lift("reset_action_buttons") + lift("onQueueStatus")) + + page.evaluate("event => onQueueStatus(event)", {"detail": {"status": status, "done_count": 0, "total_count": 1}}) + page.wait_for_timeout(100) + + assert errors == [] + page.close()