From 8be50662ed9ee71041ccbb359ffac82886151e97 Mon Sep 17 00:00:00 2001 From: Ken Simpson Date: Mon, 22 Jun 2026 19:54:23 -0700 Subject: [PATCH] Surface remote Modal runtime failures directly --- docs/LEARNINGS.md | 2 +- remote/modal_app.py | 8 +++- tests/test_modal_executor_node.py | 67 +++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 2 deletions(-) diff --git a/docs/LEARNINGS.md b/docs/LEARNINGS.md index 4555128..b83ad9e 100644 --- a/docs/LEARNINGS.md +++ b/docs/LEARNINGS.md @@ -8,7 +8,7 @@ - Absorbing a preview tap means closing over its non-transportable boundary in both directions. A local `VAEDecode` preview branch cannot be moved remote while leaving its `VAELoader` local, and once that loader is remote, any remaining local `VAEEncode` or `VAEDecode` consumers of the same `VAE` must join the component too; otherwise the planner just moves the illegal `VAE` boundary from remote input to remote output. - Preview taps should suppress split proxies only when the final component has no local re-entry dependency. If a component output feeds a local node such as `BetterGrok` and that local node feeds back into the same remote component, a single proxy creates a synthetic `proxy -> local -> same proxy` dependency cycle, so the component still has to split into ordered phases. - Durable session bridge replay is acceptable for cheap loader/encoder recovery paths, but replaying a sampler bridge is usually a planning or affinity failure. When a downstream phase loses the live or warm bridge value for a sampler-producing component, the runtime now fails loudly instead of silently rerunning that sampler phase. -- Modal `ExecutionError` is used for both lookup/deployment failures and real remote runtime failures. Auto-deploy recovery must not key only on broad strings like `not found`; if the message contains a remote traceback or `RemoteSubgraphExecutionError`, surface the remote failure directly instead of importing the cloud deploy module and replacing the real error with an existing-app guard. +- Modal `ExecutionError` is used for both lookup/deployment failures and real remote runtime failures. Auto-deploy recovery must not key only on broad strings like `not found`, and both the stale-handle retry layer and the outer deployed-invocation catch need the same check; if the message contains a remote traceback or `RemoteSubgraphExecutionError`, surface the remote failure directly instead of importing the cloud deploy module and replacing the real error with an existing-app guard. - Refocus replay cannot depend only on buffered Modal websocket events. Native ComfyUI completion events such as `execution_success` may be missed while the browser is backgrounded, so the frontend now reconciles temporary Modal UI state against ComfyUI's `/queue` and `/history/{prompt_id}` endpoints before leaving old node colors or the global pill visible. ## 2026-05-20 diff --git a/remote/modal_app.py b/remote/modal_app.py index c8b8c32..9bc9110 100644 --- a/remote/modal_app.py +++ b/remote/modal_app.py @@ -2063,6 +2063,8 @@ def _is_missing_modal_deployment_error(exc: BaseException) -> bool: ) ): return False + if "not deployed" in message: + return True if "not found" not in message: return False return any( @@ -5069,7 +5071,8 @@ def _invoke_modal_payload_blocking( cancellation_event, ) except lookup_error_types as exc: - if settings.auto_deploy: + missing_deployment = _is_missing_modal_deployment_error(exc) + if settings.auto_deploy and missing_deployment: remote_engine = _auto_deploy_modal_app(payload, exc) try: logger.info( @@ -5085,6 +5088,9 @@ def _invoke_modal_payload_blocking( ) except lookup_error_types as retry_exc: exc = retry_exc + missing_deployment = _is_missing_modal_deployment_error(exc) + if not missing_deployment: + raise if not settings.allow_ephemeral_fallback: raise ModalRemoteInvocationError( "Remote execution requires a deployed Modal app or a successful first-run auto-deploy. " diff --git a/tests/test_modal_executor_node.py b/tests/test_modal_executor_node.py index 79cf831..97a21c7 100644 --- a/tests/test_modal_executor_node.py +++ b/tests/test_modal_executor_node.py @@ -3347,6 +3347,73 @@ def test_remote_modal_does_not_classify_remote_execution_error_as_missing_app( assert remote_modal_app_module._is_missing_modal_deployment_error(missing_lookup_error) +def test_remote_modal_does_not_redeploy_after_remote_execution_error( + remote_modal_app_module: Any, + monkeypatch: Any, +) -> None: + """The deployed invocation path should not auto-deploy after a remote runtime failure.""" + + class FakeLookupError(Exception): + """Stand-in for Modal errors that share the lookup error type.""" + + class FakeExecuteMethod: + """Minimal Modal method handle that raises a remote execution failure.""" + + def remote(self, payload: dict[str, Any], kwargs_payload: bytes) -> bytes: + """Raise the wrapped remote traceback from the deployed call.""" + del payload, kwargs_payload + raise FakeLookupError( + "Could not deserialize remote exception due to local error:\n" + "Here is the remote traceback:\n" + "comfyui_modal_sync_cloud.RemoteSubgraphExecutionError: " + "Object of type CLIP is not JSON serializable\n" + "App 'comfy-modal-sync' not found in environment 'main'." + ) + + class FakeRemoteEngine: + """Minimal deployed remote engine instance.""" + + execute_payload = FakeExecuteMethod() + runtime_version = types.SimpleNamespace( + remote=lambda: {"protocol_version": remote_modal_app_module._REMOTE_APP_PROTOCOL_VERSION} + ) + + class FakeModal: + """Minimal Modal SDK double with shared lookup/runtime error types.""" + + exception = types.SimpleNamespace( + NotFoundError=FakeLookupError, + ExecutionError=FakeLookupError, + InvalidError=FakeLookupError, + ) + + class Cls: + """Namespace for deployed class lookups.""" + + @staticmethod + def from_name(app_name: str, class_name: str) -> Any: + """Return a deployed class successfully.""" + del app_name, class_name + return lambda **kwargs: FakeRemoteEngine() + + def fail_load_cloud_module() -> Any: + """Fail if runtime errors attempt auto-deploy.""" + raise AssertionError("remote execution errors must not trigger auto-deploy") + + monkeypatch.setattr(remote_modal_app_module, "modal", FakeModal) + monkeypatch.setattr(remote_modal_app_module, "_load_modal_cloud_module", fail_load_cloud_module) + monkeypatch.setenv("COMFY_MODAL_AUTO_DEPLOY", "true") + remote_modal_app_module.get_settings.cache_clear() + try: + with pytest.raises(FakeLookupError, match="Object of type CLIP"): + remote_modal_app_module._invoke_modal_payload_blocking( + {"component_id": "component-1"}, + b"{}", + ) + finally: + remote_modal_app_module.get_settings.cache_clear() + + def test_remote_modal_redeploys_when_cached_app_was_deleted( remote_modal_app_module: Any, monkeypatch: Any,