From 5e910ef27264a6b4d5484dfd346fccec521450aa Mon Sep 17 00:00:00 2001 From: Ken Simpson Date: Wed, 19 Aug 2026 10:33:32 -0700 Subject: [PATCH] Prevent disabled LLM nodes from launching Modal --- README.md | 1 + api_intercept.py | 16 +++++++++-- docs/LEARNINGS.md | 1 + pyproject.toml | 2 +- tests/modal_toggle_frontend.test.mjs | 34 ++++++++++++++++++++++ tests/test_api_intercept.py | 40 ++++++++++++++++++++++++++ web/modal_toggle.js | 43 ++++++++++++++++++++++++++-- 7 files changed, 132 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 3abe9f4..9fec728 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,7 @@ For credential safety, the node accepts only HTTPS `modal.direct` origins, refus - Leave `Run on Modal` disabled to run the node locally on Apple Silicon through MLX-VLM. This is intended for MacBooks whose unified memory can hold an LLM while heavier image or video nodes run on Modal. - Enable `Run on Modal` to run the node inside the same Modal GPU worker as its surrounding remote component. +- The visible toggle is authoritative at queue time. A disabled LLM node bypasses Modal setup, deployment, asset sync, and remote dispatch entirely, even if an older saved workflow contains stale remote-marker metadata. The queue planner treats an unmarked Modal LLM as an ordinary local node, including when it sits between two remote regions. Inputs and outputs cross those region boundaries through the existing transport layer; the model itself stays on the Mac. diff --git a/api_intercept.py b/api_intercept.py index 47c9e87..16296ba 100644 --- a/api_intercept.py +++ b/api_intercept.py @@ -49,6 +49,7 @@ _MODAL_UI_EVENT_LIMIT_PER_CLIENT = 512 _MODAL_UI_EVENTS_LOCK = threading.Lock() _MODAL_UI_EVENTS_BY_CLIENT: dict[str, deque[dict[str, Any]]] = {} _MODAL_INTERRUPT_QUEUE_BRIDGE_ATTR = "__comfy_modal_interrupt_queue_bridge_installed" +_REMOTE_TOGGLE_WIDGET_NAME = "Run on Modal" _TRANSPORTABLE_OUTPUT_TYPES = frozenset( { "*", @@ -1144,8 +1145,7 @@ def extract_remote_node_ids( marker = (settings or get_settings()).marker_property remote_node_ids: set[str] = set() for node, ancestor_node_ids in _iter_workflow_nodes_with_ancestors(workflow): - properties = node.get("properties") or {} - if properties.get(marker): + if _workflow_node_remote_enabled(node, marker): node_id = str(node.get("id")) if prompt_node_ids is None: remote_node_ids.add(node_id) @@ -1167,6 +1167,18 @@ def extract_remote_node_ids( return remote_node_ids +def _workflow_node_remote_enabled(node: Mapping[str, Any], marker: str) -> bool: + """Return the visible remote toggle, falling back to the saved marker property.""" + named_widget_values = node.get("widgets_values_named") + if isinstance(named_widget_values, Mapping): + visible_toggle_value = named_widget_values.get(_REMOTE_TOGGLE_WIDGET_NAME) + if isinstance(visible_toggle_value, bool): + return visible_toggle_value + + properties = node.get("properties") + return isinstance(properties, Mapping) and bool(properties.get(marker)) + + def _normalize_output_metadata(node_class: type[Any]) -> tuple[tuple[str, ...], tuple[str, ...], tuple[bool, ...]]: """Return normalized output metadata for a node class.""" if hasattr(node_class, "GET_SCHEMA"): diff --git a/docs/LEARNINGS.md b/docs/LEARNINGS.md index 21b5152..d05bf42 100644 --- a/docs/LEARNINGS.md +++ b/docs/LEARNINGS.md @@ -72,6 +72,7 @@ - The Modal status pill should be driven by actual remote work, not by the custom node route being installed. Since the frontend patches every prompt through `/modal/queue_prompt`, the backend must fast-path prompts with zero `Run on Modal` nodes without emitting `modal_status`, and the frontend should ignore any empty Modal status event as a defensive guard. - A visibility/focus redraw is not enough if the browser missed Modal websocket events while backgrounded. The local ComfyUI side needs to retain recent client-scoped `modal_status` and `modal_progress` payloads, and the frontend should replay that buffer on refocus before redrawing the status pill and node overlays. - Programmatic context-menu changes to `properties.is_modal_remote` also have to update the live `Run on Modal` widget value. Otherwise the saved workflow state and node border can be correct while the visible toggle still appears disabled. +- ComfyUI restores saved widget values after `nodeCreated`, so the inverse mismatch is also possible: the visible `Run on Modal` widget can be disabled while an older `properties.is_modal_remote` value remains true. Reconcile the property in `loadedGraphNode`/`afterConfigureGraph`, treat `widgets_values_named["Run on Modal"]` as authoritative at queue time, and keep the backend guard so stale metadata cannot launch Modal. - Graph-wide Modal enablement should operate on live eligible nodes, not just serialized workflow JSON. Scanning the root graph plus nested subgraphs lets the action update both node properties and visible widgets in one pass. - Once Modal has several node context actions, putting them under one `Modal` submenu keeps labels short and prevents repeated prefixes from cluttering the main node menu. diff --git a/pyproject.toml b/pyproject.toml index dd7adce..7208c7a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "modal-sync" -version = "0.2.2" +version = "0.2.3" description = "ComfyUI nodes for Modal-backed workflow execution and hosted model endpoints." readme = "README.md" requires-python = ">=3.11" diff --git a/tests/modal_toggle_frontend.test.mjs b/tests/modal_toggle_frontend.test.mjs index 4df4941..e4649b7 100644 --- a/tests/modal_toggle_frontend.test.mjs +++ b/tests/modal_toggle_frontend.test.mjs @@ -49,6 +49,8 @@ const transformedSource = `${[ " markPromptTerminal,", " patchQueuePrompt,", " setRemoteFlag,", + " synchronizeRemoteFlagFromWidget,", + " extractRemoteNodeIds,", " setSandwichedLocalNodeIds,", " isSandwichedLocalNode,", " setAllEligibleWorkflowNodesRemote,", @@ -338,6 +340,38 @@ modalToggle.setRemoteFlag(toggleSyncNode, false); assert.equal(toggleSyncNode.properties.is_modal_remote, false); assert.equal(toggleSyncNode.__modalToggleWidget.value, false); +const restoredToggleNode = { + comfyClass: "ModalLLM", + properties: { is_modal_remote: true }, + __modalToggleWidget: { value: false }, +}; +modalToggle.synchronizeRemoteFlagFromWidget(restoredToggleNode); +assert.equal(restoredToggleNode.properties.is_modal_remote, false); +assert.deepEqual( + modalToggle.extractRemoteNodeIds({ + nodes: [ + { + id: 9, + properties: { is_modal_remote: true }, + widgets_values_named: { "Run on Modal": false }, + }, + ], + }), + [], +); +assert.deepEqual( + modalToggle.extractRemoteNodeIds({ + nodes: [ + { + id: 9, + properties: { is_modal_remote: false }, + widgets_values_named: { "Run on Modal": true }, + }, + ], + }), + ["9"], +); + const eligibleNode = { id: "eligible", comfyClass: "KSampler", diff --git a/tests/test_api_intercept.py b/tests/test_api_intercept.py index de336f6..b41abed 100644 --- a/tests/test_api_intercept.py +++ b/tests/test_api_intercept.py @@ -3084,6 +3084,46 @@ def test_extract_remote_node_ids_recurses_into_nested_subgraph_workflows( ) == {"100:11"} +def test_extract_remote_node_ids_prefers_visible_toggle_over_stale_property( + api_intercept_module: Any, + settings_module: Any, +) -> None: + """A restored disabled widget must prevent stale metadata from starting Modal.""" + settings = settings_module.ModalSyncSettings( + app_name="app", + auto_deploy=True, + allow_ephemeral_fallback=False, + enable_memory_snapshot=True, + enable_gpu_memory_snapshot=False, + execution_mode="local", + sync_custom_nodes=False, + volume_name="volume", + route_path="/modal/queue_prompt", + marker_property="is_modal_remote", + local_storage_root=Path("/tmp/storage"), + remote_storage_root="/storage", + custom_nodes_archive_name="custom_nodes_bundle.zip", + comfyui_root=None, + custom_nodes_dir=Path("/tmp/custom_nodes"), + ) + workflow = { + "nodes": [ + { + "id": 9, + "properties": {"is_modal_remote": True}, + "widgets_values_named": {"Run on Modal": False}, + }, + { + "id": 10, + "properties": {"is_modal_remote": False}, + "widgets_values_named": {"Run on Modal": True}, + }, + ] + } + + assert api_intercept_module.extract_remote_node_ids(workflow, settings) == {"10"} + + def test_extract_remote_node_ids_maps_subgraph_container_to_descendant_prompt_nodes( api_intercept_module: Any, settings_module: Any, diff --git a/web/modal_toggle.js b/web/modal_toggle.js index 3d395f2..99f88bd 100644 --- a/web/modal_toggle.js +++ b/web/modal_toggle.js @@ -2,6 +2,7 @@ import { app } from "../../scripts/app.js"; import { PromptExecutionError, api } from "../../scripts/api.js"; const REMOTE_PROPERTY = "is_modal_remote"; +const REMOTE_WIDGET_NAME = "Run on Modal"; const MODAL_ROUTE = "/modal/queue_prompt"; const MODAL_ANALYZE_ROUTE = MODAL_ROUTE.replace(/\/queue_prompt$/, "/analyze_remote_nodes"); const MODAL_PROGRESS_STATE_ROUTE = MODAL_ROUTE.replace(/\/queue_prompt$/, "/progress_state"); @@ -2692,13 +2693,26 @@ function laneOwnerKey(promptId, nodeIdValue, laneId) { function extractRemoteNodeIds(workflow) { const remoteNodeIds = []; for (const node of workflow?.nodes ?? []) { - if (node?.properties?.[REMOTE_PROPERTY]) { + if (serializedRemoteFlag(node)) { remoteNodeIds.push(String(node.id)); } } return remoteNodeIds; } +/** + * Return the user-visible Modal toggle value from a serialized workflow node. + * @param {object | undefined} node + * @returns {boolean} + */ +function serializedRemoteFlag(node) { + const namedWidgetValue = node?.widgets_values_named?.[REMOTE_WIDGET_NAME]; + if (typeof namedWidgetValue === "boolean") { + return namedWidgetValue; + } + return Boolean(node?.properties?.[REMOTE_PROPERTY]); +} + /** * Return the root workflow graph, including subgraphs when available. * @returns {LGraph | null} @@ -3208,6 +3222,24 @@ function setRemoteFlag(node, value) { refreshNodeDecorations(); } +/** + * Reconcile a restored node property with the toggle value displayed to the user. + * @param {LGraphNode} node + */ +function synchronizeRemoteFlagFromWidget(node) { + if (!isEligibleNode(node) || !node.__modalToggleWidget) { + return; + } + const enabled = Boolean(node.__modalToggleWidget.value); + node.properties ||= {}; + if (node.properties[REMOTE_PROPERTY] === enabled) { + return; + } + node.properties[REMOTE_PROPERTY] = enabled; + clearSandwichedLocalNodeWarnings(); + refreshNodeDecorations(); +} + /** * Return the current remote visual state for a node. * @param {LGraphNode} node @@ -3896,7 +3928,7 @@ function decorateNode(node) { const widget = node.addWidget( "toggle", - "Run on Modal", + REMOTE_WIDGET_NAME, node.properties[REMOTE_PROPERTY], (value) => setRemoteFlag(node, value), { @@ -4787,7 +4819,14 @@ app.registerExtension({ decorateNode(node); }, + async loadedGraphNode(node) { + synchronizeRemoteFlagFromWidget(node); + }, + async afterConfigureGraph() { + for (const node of allWorkflowNodes()) { + synchronizeRemoteFlagFromWidget(node); + } selectedModalGpu(); }, });