From 55e37d138077b7c8e1ea60469fd9d5fbdd4f6229 Mon Sep 17 00:00:00 2001 From: Ken Simpson Date: Thu, 23 Apr 2026 14:47:43 -0700 Subject: [PATCH] fix: recover from stale modal handles after app deletion --- README.md | 1 + docs/LEARNINGS.md | 1 + remote/modal_app.py | 75 +++++++++++-- tests/test_modal_executor_node.py | 180 ++++++++++++++++++++++++++++++ 4 files changed, 248 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 6ebb65c..6e31557 100644 --- a/README.md +++ b/README.md @@ -149,6 +149,7 @@ Remote mode requires: The normal remote path is deployed-app lookup by name. On first remote use, the extension can auto-deploy the stable cloud entrypoint from [`comfyui_modal_sync_cloud.py`](comfyui_modal_sync_cloud.py) if the configured app does not exist yet. If that deployed Modal app is later deleted manually, the next lookup failure now invalidates the stale in-process deploy cache and auto-deploys a fresh copy instead of surfacing a hard "app not found" error. +If a previously looked-up `RemoteEngine` handle later fails during `hydrate()` or the first `remote()` call because the app was deleted out-of-band, the invoke path now treats that as the same missing-app condition, auto-deploys, and retries instead of first surfacing a spurious prompt failure. If a previous attempt imported that cloud entrypoint only partially before failing, the loader now discards the stale cached module and retries the import cleanly before auto-deploy. The local and remote subgraph runners also canonicalize malformed singleton-list wrappers in rewritten prompts, including socket indexes like `["node", [0]]`, scalar inputs like `[4]`, and wrapped execute node ids like `["12"]`, before handing the prompt to ComfyUI. When the remote PromptExecutor still fails, the Modal worker now logs the normalized execute-node ids, boundary outputs, suspicious remaining wrapped prompt inputs, and the underlying ComfyUI `execution_error` payload to make the bad field visible. diff --git a/docs/LEARNINGS.md b/docs/LEARNINGS.md index fd9af8a..a7cac75 100644 --- a/docs/LEARNINGS.md +++ b/docs/LEARNINGS.md @@ -208,4 +208,5 @@ - Frontend mapped-progress lanes cannot be keyed only by `laneId` at prompt scope. Different remote components legitimately reuse local worker lane numbers like `0` and `1`, so the UI has to namespace lane ownership by component representative or concurrent components will clear each other's purple bars and appear to flicker between active and ready states. - Streamed remote progress should suppress native proxy `executing` churn, but not the proxy's terminal completion. If the frontend drops every native phase update after streamed progress starts, a single-node remote component can stay stuck purple because it only ever receives streamed `finalizing` and never consumes the completion event that should turn it green. - The in-process "already auto-deployed" cache cannot be treated as authoritative once operators can delete the Modal app out-of-band. A later `app not found` lookup has to invalidate that cache entry and redeploy instead of assuming the earlier successful deploy still exists remotely. +- With deployed `modal.Cls` handles, "app missing" is not only a lookup-time failure. A stale handle can also survive lookup and then fail later during `hydrate()` or the first `remote()` call. The invoke path has to treat that the same way as a missing-app lookup, auto-deploy, and retry before surfacing an error locally. - Combined first-run and second-run performance logs are now clear enough to prioritize work. In this workflow, uploads and volume reload were already cheap, while the recurring non-sampling bottleneck was static-phase model loading (`UNETLoader` + `CLIPLoader` at roughly `24-25s` total per run). The second run also showed that local proxy caching can skip the static proxy dispatch while still leaving a large replay cost inside the mapped phase, which makes two no-brainer optimizations obvious: reuse heavy loader outputs on the same worker, and avoid replaying the whole static producer branch just to reconstruct session-backed bridge values. diff --git a/remote/modal_app.py b/remote/modal_app.py index b56fc9b..66fdbfc 100644 --- a/remote/modal_app.py +++ b/remote/modal_app.py @@ -3809,7 +3809,7 @@ async def _invoke_bound_remote_engine_async( ) cancellation_event = threading.Event() future = _REMOTE_MODAL_CALL_EXECUTOR.submit( - _invoke_remote_engine_payload, + _invoke_remote_engine_payload_with_recovery, remote_engine, dict(payload), kwargs_payload, @@ -4203,6 +4203,27 @@ def _invoke_remote_engine_warmup(remote_engine: Any, warmup_request: dict[str, A return warmup_method(warmup_request) +def _invoke_remote_engine_warmup_with_recovery( + remote_engine: Any, + warmup_request: dict[str, Any], +) -> Any: + """Retry one warmup call after auto-deploy when a stale deployed handle vanishes.""" + lookup_error_types = _modal_lookup_error_types() + settings = get_settings() + try: + return _invoke_remote_engine_warmup(remote_engine, warmup_request) + except lookup_error_types as exc: + if not settings.auto_deploy or not _is_missing_modal_deployment_error(exc): + raise + logger.warning( + "Modal warmup invocation failed for component=%s because the deployed app was missing at call time: %s. Recreating the app and retrying.", + warmup_request.get("component_id"), + exc, + ) + recovered_remote_engine = _auto_deploy_modal_app(warmup_request, exc) + return _invoke_remote_engine_warmup(recovered_remote_engine, warmup_request) + + def _invoke_modal_warmup_blocking(warmup_request: dict[str, Any]) -> Any: """Warm one Modal container slot using deployed or ephemeral app state.""" if modal is None: @@ -4213,12 +4234,15 @@ def _invoke_modal_warmup_blocking(warmup_request: dict[str, Any]) -> Any: if lookup_error_types: try: remote_engine = _lookup_deployed_remote_engine(warmup_request) - return _invoke_remote_engine_warmup(remote_engine, warmup_request) + return _invoke_remote_engine_warmup_with_recovery(remote_engine, warmup_request) except lookup_error_types as exc: if settings.auto_deploy: remote_engine = _auto_deploy_modal_app(warmup_request, exc) try: - return _invoke_remote_engine_warmup(remote_engine, warmup_request) + return _invoke_remote_engine_warmup_with_recovery( + remote_engine, + warmup_request, + ) except lookup_error_types as retry_exc: exc = retry_exc if not settings.allow_ephemeral_fallback: @@ -4228,7 +4252,7 @@ def _invoke_modal_warmup_blocking(warmup_request: dict[str, Any]) -> Any: ) from exc else: remote_engine = _lookup_deployed_remote_engine(warmup_request) - return _invoke_remote_engine_warmup(remote_engine, warmup_request) + return _invoke_remote_engine_warmup_with_recovery(remote_engine, warmup_request) cloud_module = _load_modal_cloud_module() cloud_app = getattr(cloud_module, "app", None) @@ -4240,7 +4264,7 @@ def _invoke_modal_warmup_blocking(warmup_request: dict[str, Any]) -> Any: run_context = cloud_app.run() if hasattr(cloud_app, "run") else nullcontext() with run_context: remote_engine = cloud_remote_engine() - return _invoke_remote_engine_warmup(remote_engine, warmup_request) + return _invoke_remote_engine_warmup_with_recovery(remote_engine, warmup_request) def _run_prompt_warmup_slot( @@ -4566,6 +4590,39 @@ def _invoke_remote_engine_payload( ) +def _invoke_remote_engine_payload_with_recovery( + remote_engine: Any, + payload: dict[str, Any], + kwargs_payload: bytes, + cancellation_event: threading.Event | None, +) -> bytes: + """Retry one payload call after auto-deploy when a stale deployed handle vanishes.""" + lookup_error_types = _modal_lookup_error_types() + settings = get_settings() + try: + return _invoke_remote_engine_payload( + remote_engine, + payload, + kwargs_payload, + cancellation_event, + ) + except lookup_error_types as exc: + if not settings.auto_deploy or not _is_missing_modal_deployment_error(exc): + raise + logger.warning( + "Modal payload invocation failed for component=%s because the deployed app was missing at call time: %s. Recreating the app and retrying.", + payload.get("component_id"), + exc, + ) + recovered_remote_engine = _auto_deploy_modal_app(payload, exc) + return _invoke_remote_engine_payload( + recovered_remote_engine, + payload, + kwargs_payload, + cancellation_event, + ) + + def _invoke_modal_payload_blocking( payload: dict[str, Any], kwargs_payload: bytes, @@ -4585,7 +4642,7 @@ def _invoke_modal_payload_blocking( settings.app_name, payload.get("component_id"), ) - return _invoke_remote_engine_payload( + return _invoke_remote_engine_payload_with_recovery( remote_engine, payload, kwargs_payload, @@ -4600,7 +4657,7 @@ def _invoke_modal_payload_blocking( settings.app_name, payload.get("component_id"), ) - return _invoke_remote_engine_payload( + return _invoke_remote_engine_payload_with_recovery( remote_engine, payload, kwargs_payload, @@ -4628,7 +4685,7 @@ def _invoke_modal_payload_blocking( settings.app_name, payload.get("component_id"), ) - return _invoke_remote_engine_payload( + return _invoke_remote_engine_payload_with_recovery( remote_engine, payload, kwargs_payload, @@ -4652,7 +4709,7 @@ def _invoke_modal_payload_blocking( run_context = cloud_app.run() if hasattr(cloud_app, "run") else nullcontext() with run_context: remote_engine = cloud_remote_engine() - result = _invoke_remote_engine_payload( + result = _invoke_remote_engine_payload_with_recovery( remote_engine, payload, kwargs_payload, diff --git a/tests/test_modal_executor_node.py b/tests/test_modal_executor_node.py index 42af5c8..b0b2eac 100644 --- a/tests/test_modal_executor_node.py +++ b/tests/test_modal_executor_node.py @@ -2812,6 +2812,186 @@ def test_remote_modal_redeploys_when_cached_app_was_deleted( assert deploy_calls == [("comfy-modal-sync", "main")] +def test_remote_modal_redeploys_when_deployed_handle_disappears_during_payload_invoke( + remote_modal_app_module: Any, + monkeypatch: Any, +) -> None: + """A stale deployed engine handle should auto-redeploy when invocation hydrate fails.""" + + class FakeLookupError(Exception): + """Stand-in for Modal deployed lookup failures.""" + + deploy_calls: list[tuple[str | None, str | None]] = [] + + class FakeRemoteEngine: + """Minimal deployed remote engine instance with stale-handle behavior.""" + + def __init__(self, *, stale: bool) -> None: + """Record whether this engine should fail once as a stale handle.""" + self._stale = stale + self.execute_payload = types.SimpleNamespace(remote=self._remote) + + def _remote(self, payload: dict[str, Any], kwargs_payload: bytes) -> bytes: + """Raise the missing-app error for stale handles and otherwise succeed.""" + del payload, kwargs_payload + if self._stale: + raise FakeLookupError( + "Lookup failed for Cls 'RemoteEngine' from the 'comfy-modal-sync' app: " + "App 'comfy-modal-sync' not found in environment 'main'." + ) + return b"remote-response" + + class FakeApp: + """Minimal deployable cloud app double.""" + + def deploy( + self, + *, + name: str | None = None, + environment_name: str | None = None, + **_: Any, + ) -> "FakeApp": + """Record the redeploy request and mark future handles as fresh.""" + deploy_calls.append((name, environment_name)) + FakeModal.redeployed = True + return self + + class FakeModal: + """Minimal modal SDK double with stale-handle invocation failures.""" + + redeployed = False + 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 stale handles before redeploy and fresh handles after.""" + del app_name, class_name + return lambda **kwargs: FakeRemoteEngine(stale=not FakeModal.redeployed) + + @staticmethod + def enable_output() -> Any: + """Provide a no-op output context manager.""" + return nullcontext() + + monkeypatch.setattr(remote_modal_app_module, "modal", FakeModal) + monkeypatch.setattr( + remote_modal_app_module, + "_load_modal_cloud_module", + lambda: types.SimpleNamespace(app=FakeApp()), + ) + monkeypatch.setenv("COMFY_MODAL_AUTO_DEPLOY", "true") + monkeypatch.setenv("MODAL_ENVIRONMENT", "main") + remote_modal_app_module.get_settings.cache_clear() + remote_modal_app_module._MODAL_AUTO_DEPLOY_STATES.clear() + try: + response = remote_modal_app_module._invoke_modal_payload_blocking( + {"component_id": "component-1"}, + b"{}", + ) + finally: + remote_modal_app_module.get_settings.cache_clear() + remote_modal_app_module._MODAL_AUTO_DEPLOY_STATES.clear() + + assert response == b"remote-response" + assert deploy_calls == [("comfy-modal-sync", "main")] + + +def test_remote_modal_redeploys_when_deployed_handle_disappears_during_warmup( + remote_modal_app_module: Any, + monkeypatch: Any, +) -> None: + """A stale deployed warmup handle should auto-redeploy instead of surfacing a warmup error.""" + + class FakeLookupError(Exception): + """Stand-in for Modal deployed lookup failures.""" + + deploy_calls: list[tuple[str | None, str | None]] = [] + + class FakeRemoteEngine: + """Minimal deployed remote engine instance with stale-handle warmup behavior.""" + + def __init__(self, *, stale: bool) -> None: + """Expose a warmup method that fails for stale handles.""" + self._stale = stale + self.warmup_for_request = types.SimpleNamespace(remote=self._remote) + + def _remote(self, payload: dict[str, Any]) -> dict[str, Any]: + """Raise the missing-app error for stale handles and otherwise succeed.""" + if self._stale: + raise FakeLookupError( + "Lookup failed for Cls 'RemoteEngine' from the 'comfy-modal-sync' app: " + "App 'comfy-modal-sync' not found in environment 'main'." + ) + return {"component_id": str(payload.get("component_id"))} + + class FakeApp: + """Minimal deployable cloud app double.""" + + def deploy( + self, + *, + name: str | None = None, + environment_name: str | None = None, + **_: Any, + ) -> "FakeApp": + """Record the redeploy request and mark future handles as fresh.""" + deploy_calls.append((name, environment_name)) + FakeModal.redeployed = True + return self + + class FakeModal: + """Minimal modal SDK double with stale-handle warmup failures.""" + + redeployed = False + 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 stale handles before redeploy and fresh handles after.""" + del app_name, class_name + return lambda **kwargs: FakeRemoteEngine(stale=not FakeModal.redeployed) + + @staticmethod + def enable_output() -> Any: + """Provide a no-op output context manager.""" + return nullcontext() + + monkeypatch.setattr(remote_modal_app_module, "modal", FakeModal) + monkeypatch.setattr( + remote_modal_app_module, + "_load_modal_cloud_module", + lambda: types.SimpleNamespace(app=FakeApp()), + ) + monkeypatch.setenv("COMFY_MODAL_AUTO_DEPLOY", "true") + monkeypatch.setenv("MODAL_ENVIRONMENT", "main") + remote_modal_app_module.get_settings.cache_clear() + remote_modal_app_module._MODAL_AUTO_DEPLOY_STATES.clear() + try: + response = remote_modal_app_module._invoke_modal_warmup_blocking( + {"component_id": "component-1::warmup:0"}, + ) + finally: + remote_modal_app_module.get_settings.cache_clear() + remote_modal_app_module._MODAL_AUTO_DEPLOY_STATES.clear() + + assert response == {"component_id": "component-1::warmup:0"} + assert deploy_calls == [("comfy-modal-sync", "main")] + + def test_remote_modal_auto_deploy_is_shared_across_concurrent_first_run_callers( remote_modal_app_module: Any, monkeypatch: Any,