From 69f356b02180c42b7ff888d3dc4c8f5cd8235ad2 Mon Sep 17 00:00:00 2001 From: Ken Simpson Date: Tue, 4 Aug 2026 14:42:59 -0700 Subject: [PATCH] fix: stop stale Modal apps noninteractively --- README.md | 2 +- docs/LEARNINGS.md | 1 + remote/modal_app.py | 49 +++++++++++--- tests/test_modal_executor_node.py | 106 ++++++++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 38f022b..c363bea 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,7 @@ Each ComfyUI environment receives its own Modal app name on first startup. Modal For repository development, `uv sync --extra remote --group test` installs the same pinned SDK. Remote mode uses the stable cloud entrypoint in [`comfyui_modal_sync_cloud.py`](comfyui_modal_sync_cloud.py). On first use, Modal-Sync can auto-deploy the configured Modal app if it does not exist. -The deployed image uses Python 3.11 plus an exact ComfyUI support and CUDA package set, including the ComfyUI-pinned `comfy-aimdo` and `comfy-kitchen` releases required by current memory-management and quantization imports. Remote prompt executors mirror both legacy scalar and current active/inactive RAM-pressure cache arguments from ComfyUI. The image's local build context is limited to the ComfyUI source packages, top-level Python modules, and runtime configuration needed by the headless worker; model directories, custom nodes, caches, tests, virtual environments, user data, and unknown top-level directories stay out of the image snapshot. Before every process's first remote invocation, Modal-Sync compares the deployed worker's runtime fingerprint with the local source, ComfyUI source, custom-node requirements, and runtime-shaping settings. A missing or mismatched fingerprint is treated as stale and replaced automatically when `COMFY_MODAL_AUTO_DEPLOY=true`. +The deployed image uses Python 3.11 plus an exact ComfyUI support and CUDA package set, including the ComfyUI-pinned `comfy-aimdo` and `comfy-kitchen` releases required by current memory-management and quantization imports. Remote prompt executors mirror both legacy scalar and current active/inactive RAM-pressure cache arguments from ComfyUI. The image's local build context is limited to the ComfyUI source packages, top-level Python modules, and runtime configuration needed by the headless worker; model directories, custom nodes, caches, tests, virtual environments, user data, and unknown top-level directories stay out of the image snapshot. Before every process's first remote invocation, Modal-Sync compares the deployed worker's runtime fingerprint with the local source, ComfyUI source, custom-node requirements, and runtime-shaping settings. A missing or mismatched fingerprint is treated as stale and replaced automatically when `COMFY_MODAL_AUTO_DEPLOY=true`; replacement uses Modal's non-interactive SDK stop API when available and an explicitly confirmed CLI fallback, so app shutdown cannot stall on a hidden terminal prompt. ## Using It In ComfyUI diff --git a/docs/LEARNINGS.md b/docs/LEARNINGS.md index 40c5fcd..b97a4ce 100644 --- a/docs/LEARNINGS.md +++ b/docs/LEARNINGS.md @@ -308,3 +308,4 @@ - A fixed default Modal app name makes independent ComfyUI installations collide inside one Modal workspace and also shares every app-derived Dict namespace. Persist 64 random bits in the effective ComfyUI user directory, publish the fully written identity atomically so concurrent first starts agree, encode it with unpadded URL-safe Base64 for the app suffix, preserve explicit name overrides, and inject the resolved app name into the remote image so workers never generate a second identity. - Current ComfyUI imports `comfy_aimdo` unconditionally from its core memory-management modules, so omitting it from the Modal image prevents the worker from reaching node initialization. Keep both `comfy-aimdo` and `comfy-kitchen` aligned with the exact pins in the supported ComfyUI checkout; adding only the newly missing package can leave a stale kitchen ABI or API behind. - ComfyUI changed `args.cache_ram` from a scalar opt-in threshold to a list of zero, one, or two active/inactive thresholds and made RAM-pressure caching the default. Remote `PromptExecutor` construction must distinguish the legacy scalar and current list shapes, reproduce ComfyUI's system-RAM-derived defaults, and pass `ram_inactive`; comparing the list directly with zero fails only after a healthy worker reaches graph execution. +- Modal SDK 1.4 exposes deployed-app stopping through `modal.experimental.stop_app`, not `App.stop`. Falling back to `modal app stop` without `--yes` blocks on an invisible confirmation prompt when ComfyUI has captured the subprocess streams, making the local 120-second subprocess guard look like an app-build timeout. Prefer the SDK API with the active Modal environment, pass `--yes` and `--env` to the CLI fallback, and translate a real CLI timeout into an intentional stop failure. diff --git a/remote/modal_app.py b/remote/modal_app.py index 5213cd6..befa469 100644 --- a/remote/modal_app.py +++ b/remote/modal_app.py @@ -75,6 +75,7 @@ _REMOTE_APP_PROTOCOL_VERSION = REMOTE_APP_PROTOCOL_VERSION _MODAL_AUTO_DEPLOY_LOCK = threading.Lock() _MODAL_AUTO_DEPLOY_STATES: dict[tuple[str, str | None], "_ModalAutoDeployState"] = {} _MODAL_REMOTE_APP_VERSION_OK: set[tuple[str, str | None, str]] = set() +_MODAL_APP_STOP_TIMEOUT_SECONDS = 120.0 _MODAL_INTERRUPT_DICTS_LOCK = threading.Lock() _MODAL_INTERRUPT_DICTS: dict[tuple[str, str | None], Any] = {} _MAPPED_PROGRESS_NODE_IDS_LOCK = threading.Lock() @@ -4889,6 +4890,24 @@ def _stop_modal_app_via_sdk(app_name: str) -> bool: """Try to stop a Modal app through the SDK if this SDK version exposes app stopping.""" if modal is None: return False + try: + experimental_namespace = importlib.import_module("modal.experimental") + except ModuleNotFoundError as exc: + if exc.name not in {"modal", "modal.experimental"}: + raise + else: + stop_app = getattr(experimental_namespace, "stop_app", None) + if callable(stop_app): + try: + _call_modal_method( + stop_app, + app_name, + environment_name=_modal_environment_name(), + ) + except _modal_lookup_error_types(): + return True + return True + app_namespace = getattr(modal, "App", None) app_lookup = getattr(app_namespace, "lookup", None) if not callable(app_lookup): @@ -4915,18 +4934,30 @@ def _stop_modal_app_via_sdk(app_name: str) -> bool: def _stop_modal_app_via_cli(app_name: str) -> bool: - """Try to stop a Modal app through the Modal CLI.""" + """Try to stop a Modal app non-interactively through the Modal CLI.""" modal_cli = shutil.which("modal") if modal_cli is None: return False - command = [modal_cli, "app", "stop", app_name] - completed = subprocess.run( - command, - check=False, - capture_output=True, - text=True, - timeout=120, - ) + command = [modal_cli, "app", "stop", app_name, "--yes"] + environment_name = _modal_environment_name() + if environment_name is not None: + command.extend(("--env", environment_name)) + try: + completed = subprocess.run( + command, + check=False, + capture_output=True, + text=True, + timeout=_MODAL_APP_STOP_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired: + logger.warning( + "Modal CLI app stop timed out for app=%s environment=%s after %.1fs.", + app_name, + environment_name or "", + _MODAL_APP_STOP_TIMEOUT_SECONDS, + ) + return False if completed.returncode != 0: logger.warning( "Modal CLI app stop failed for app=%s exit_code=%s stderr=%s", diff --git a/tests/test_modal_executor_node.py b/tests/test_modal_executor_node.py index b3e7448..5a80751 100644 --- a/tests/test_modal_executor_node.py +++ b/tests/test_modal_executor_node.py @@ -4761,6 +4761,112 @@ def test_remote_modal_replaces_out_of_date_deployed_app( assert deploy_calls == [("comfy-modal-sync", "main")] +def test_remote_modal_stops_app_through_experimental_sdk( + remote_modal_app_module: Any, + monkeypatch: Any, +) -> None: + """Modal 1.4 should use its non-interactive experimental stop API.""" + stop_calls: list[tuple[str, str | None]] = [] + + def fake_stop_app(app_name: str, *, environment_name: str | None = None) -> None: + """Record one SDK stop request.""" + stop_calls.append((app_name, environment_name)) + + fake_experimental_namespace = types.SimpleNamespace(stop_app=fake_stop_app) + original_import_module = remote_modal_app_module.importlib.import_module + + def fake_import_module(module_name: str) -> Any: + """Return the Modal experimental namespace and delegate other imports.""" + if module_name == "modal.experimental": + return fake_experimental_namespace + return original_import_module(module_name) + + monkeypatch.setattr( + remote_modal_app_module, + "modal", + types.SimpleNamespace(exception=types.SimpleNamespace()), + ) + monkeypatch.setattr( + remote_modal_app_module.importlib, + "import_module", + fake_import_module, + ) + monkeypatch.setenv("MODAL_ENVIRONMENT", "main") + + stopped = remote_modal_app_module._stop_modal_app_via_sdk("comfy-modal-sync-instance") + + assert stopped is True + assert stop_calls == [("comfy-modal-sync-instance", "main")] + + +def test_remote_modal_cli_stop_is_noninteractive_and_environment_scoped( + remote_modal_app_module: Any, + monkeypatch: Any, +) -> None: + """The CLI fallback should never block waiting for confirmation.""" + observed_calls: list[tuple[list[str], dict[str, Any]]] = [] + + def fake_run(command: list[str], **kwargs: Any) -> Any: + """Record the CLI command and return success.""" + observed_calls.append((command, kwargs)) + return types.SimpleNamespace(returncode=0, stderr="") + + monkeypatch.setattr(remote_modal_app_module.shutil, "which", lambda _: "/venv/bin/modal") + monkeypatch.setattr(remote_modal_app_module.subprocess, "run", fake_run) + monkeypatch.setenv("MODAL_ENVIRONMENT", "main") + + stopped = remote_modal_app_module._stop_modal_app_via_cli("comfy-modal-sync-instance") + + assert stopped is True + assert observed_calls == [ + ( + [ + "/venv/bin/modal", + "app", + "stop", + "comfy-modal-sync-instance", + "--yes", + "--env", + "main", + ], + { + "check": False, + "capture_output": True, + "text": True, + "timeout": remote_modal_app_module._MODAL_APP_STOP_TIMEOUT_SECONDS, + }, + ) + ] + + +def test_remote_modal_cli_stop_timeout_returns_controlled_failure( + remote_modal_app_module: Any, + monkeypatch: Any, + caplog: Any, +) -> None: + """A genuine CLI timeout should not leak TimeoutExpired into prompt execution.""" + command = ["/venv/bin/modal", "app", "stop", "comfy-modal-sync-instance", "--yes"] + + def fake_run(*_: Any, **__: Any) -> Any: + """Simulate an unresponsive Modal CLI.""" + raise subprocess.TimeoutExpired( + command, + remote_modal_app_module._MODAL_APP_STOP_TIMEOUT_SECONDS, + ) + + monkeypatch.setattr(remote_modal_app_module.shutil, "which", lambda _: "/venv/bin/modal") + monkeypatch.setattr(remote_modal_app_module.subprocess, "run", fake_run) + monkeypatch.delenv("MODAL_ENVIRONMENT", raising=False) + + with caplog.at_level(logging.WARNING): + stopped = remote_modal_app_module._stop_modal_app_via_cli( + "comfy-modal-sync-instance" + ) + + assert stopped is False + assert "Modal CLI app stop timed out" in caplog.text + + def test_remote_modal_rejects_fingerprint_mismatch_when_auto_deploy_is_disabled( remote_modal_app_module: Any, monkeypatch: Any,