From 3bd857f0abdce152356a7ed78e34a30660717fcd Mon Sep 17 00:00:00 2001
From: James Veitch <1722315+darth-veitcher@users.noreply.github.com>
Date: Sun, 12 Jul 2026 09:36:21 +0100
Subject: [PATCH 1/2] fix: chat_structured() crashes with real asyncio.run()
 error in live ComfyUI
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Found by running a genuine end-to-end test — real docker-compose
ComfyUI harness, real Ollama, real llama-server (router mode),
actual workflow graphs submitted via POST /prompt and polled via
/history, not direct Python calls to provider classes. Every
structured_output=True ChatCompletion call failed with:

  RuntimeError: asyncio.run() cannot be called from a running event loop

Root cause: _run_async() tried asyncio.get_running_loop() first and
only spun up an isolated worker thread if that succeeded; otherwise
it called asyncio.run(coro) directly on the current thread. Under
pytest or a standalone script, get_running_loop() never spuriously
raises, so this worked and the worker-thread path was rarely
exercised for real. Under ComfyUI's actual async execution engine
(Python 3.13, node functions invoked synchronously from inside an
already-running event loop), get_running_loop() sometimes raised
anyway — routing straight into asyncio.run(coro) on the one thread
guaranteed to already have a loop running, reproducing the crash
every time. Confirmed via a live-instrumented debug run against the
real container: the coroutine executed correctly whenever the
worker-thread path was taken (get_running_loop() succeeding), and
crashed only via the direct-call fallback.

Fixed by removing the conditional entirely: always run the coroutine
in a freshly spawned worker thread. A new thread never has an
ambient loop, so asyncio.run() is safe there unconditionally,
regardless of what the calling thread's loop state actually is.

Also traced down a second symptom from the same end-to-end run:
non-structured chat() calls sometimes returned an empty response
with no error. Verified via direct curl calls to Ollama and
llama-server (bypassing comfydv entirely) that this was a real,
transient GPU/Metal memory-pressure issue on the test machine
(`ggml_metal_synchronize: error: Insufficient Memory`, Ollama's own
backend left in a wedged state returning HTTP 200 with an empty
zero-value body) — not a comfydv bug. Restarting the backend
processes resolved it; no code change was needed for that part.

Added tests/test_ollama_provider.py coverage for the specific shape
that broke: _run_async called synchronously from within code that is
itself already executing inside asyncio.run() — the actual pattern
ComfyUI's execution engine uses, which no prior test exercised (every
existing call site invoked _run_async from a plain synchronous pytest
function with no ambient loop).

Re-verified end-to-end after the fix: 5/5 real workflows passing
through actual ComfyUI execution — Ollama basic chat, Ollama full
lifecycle (Load->Chat->Unload), Ollama structured output, llama.cpp
basic chat, llama.cpp structured output — all with real generated
content, not mocks.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
---
 src/comfydv/_llm/ollama_provider.py | 32 ++++++++++-----
 tests/test_ollama_provider.py       | 60 +++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/src/comfydv/_llm/ollama_provider.py b/src/comfydv/_llm/ollama_provider.py
index 205d052..aaa5efd 100644
--- a/src/comfydv/_llm/ollama_provider.py
+++ b/src/comfydv/_llm/ollama_provider.py
@@ -88,17 +88,29 @@ _CHAT_RESPONSE_CACHE = _TTLLRUCache(maxsize=64, ttl_seconds=None)
 
 
 def _run_async(coro):
-    """Run an async coroutine synchronously, safe inside a running event loop."""
-    try:
-        asyncio.get_running_loop()
-        # Called from within a running loop (e.g. ComfyUI's async executor).
-        # Spin up a worker thread with its own loop to avoid "loop already running".
-        import concurrent.futures
+    """Run an async coroutine synchronously in an isolated worker thread.
 
-        with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool:
-            return pool.submit(asyncio.run, coro).result()
-    except RuntimeError:
-        return asyncio.run(coro)
+    Always spins up a fresh thread rather than conditionally checking
+    asyncio.get_running_loop() first: live-verified against a real running
+    ComfyUI instance (its execution engine runs its own event loop, in
+    Python 3.13, on the same process) that the conditional version — try
+    get_running_loop(), spin up a thread only if it succeeds, otherwise
+    call asyncio.run(coro) directly — is unreliable there. Under real
+    ComfyUI, get_running_loop() sometimes raised inside that try block
+    (unlike under pytest or a standalone script, where it never does),
+    which routed straight into `asyncio.run(coro)` on the *current* thread
+    — the one thread guaranteed to already have ComfyUI's own loop running
+    — reproducing exactly the "asyncio.run() cannot be called from a
+    running event loop" crash this function exists to prevent. Always
+    using a dedicated thread sidesteps the detection entirely: a freshly
+    spawned thread never has an ambient loop, so asyncio.run() is safe
+    there unconditionally, regardless of what the calling thread's loop
+    state actually is.
+    """
+    import concurrent.futures
+
+    with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool:
+        return pool.submit(asyncio.run, coro).result()
 
 
 async def _post_json(
diff --git a/tests/test_ollama_provider.py b/tests/test_ollama_provider.py
index 28d7c39..e97e84f 100644
--- a/tests/test_ollama_provider.py
+++ b/tests/test_ollama_provider.py
@@ -12,6 +12,8 @@ BDD coverage:
   ../specs/007-llm-provider-abstraction/features/us3_model_lifecycle.feature
 """
 
+import asyncio
+
 import pytest
 
 import comfydv._llm.ollama_provider as provider_mod
@@ -326,3 +328,61 @@ def test_chat_structured_caches_after_successful_validation(monkeypatch):
 
     assert r1 == r2 == Widget(name="cached")
     assert calls["n"] == 1
+
+
+# ---------------------------------------------------------------------------
+# _run_async — live-verified critical bug (found running the real
+# docker-compose ComfyUI harness, not caught by any prior test): ComfyUI's
+# actual async execution engine runs node functions synchronously *inside*
+# an already-running event loop. The old implementation tried
+# `asyncio.get_running_loop()` first and only spun up an isolated worker
+# thread if that succeeded — under real ComfyUI (Python 3.13), that check
+# sometimes raised anyway, falling through to a direct `asyncio.run(coro)`
+# call on the *current* thread — the one thread guaranteed to already have
+# a loop running — reproducing "asyncio.run() cannot be called from a
+# running event loop" exactly, every time chat_structured() was called.
+# Every existing test called _run_async from a plain synchronous pytest
+# function (no ambient loop), which never exercised this path — pytest
+# alone could not have caught this.
+# ---------------------------------------------------------------------------
+
+
+def test_run_async_works_with_no_ambient_loop():
+    async def coro():
+        return "ok"
+
+    assert _run_async(coro()) == "ok"
+
+
+def test_run_async_works_when_called_from_within_a_running_loop():
+    """The actual shape of a real ComfyUI node call: a synchronous function
+    invoked from code that is itself already executing inside
+    asyncio.run(). Must not raise "asyncio.run() cannot be called from a
+    running event loop"."""
+
+    async def inner_coro():
+        return "from inside a running loop"
+
+    def sync_node_function():
+        # This is exactly what comfydv.ollama's chat() does: a plain sync
+        # function calling _run_async(some_coroutine()).
+        return _run_async(inner_coro())
+
+    async def outer():
+        # Mirrors ComfyUI's execution.py calling `result = f(**inputs)`
+        # synchronously from within its own already-running event loop.
+        return sync_node_function()
+
+    result = asyncio.run(outer())
+    assert result == "from inside a running loop"
+
+
+def test_run_async_propagates_exceptions_from_within_a_running_loop():
+    async def failing_coro():
+        raise ValueError("boom")
+
+    async def outer():
+        return _run_async(failing_coro())
+
+    with pytest.raises(ValueError, match="boom"):
+        asyncio.run(outer())

From 1fe3bef6630b04f14b347e5d5ebb317e0c5be71d Mon Sep 17 00:00:00 2001
From: James Veitch <1722315+darth-veitcher@users.noreply.github.com>
Date: Sun, 12 Jul 2026 09:39:55 +0100
Subject: [PATCH 2/2] docs(tests): stop overclaiming what the _run_async
 regression tests prove
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

beacon-reviewer caught it: reconstructing the old buggy _run_async and
running it against test_run_async_works_when_called_from_within_a_running_loop
and test_run_async_propagates_exceptions_from_within_a_running_loop shows
both pass against the old code too. asyncio.get_running_loop() never
spuriously raises under plain CPython/pytest, so the old conditional also
takes the safe worker-thread branch there — the actual reported bug is not
reproducible outside real ComfyUI's execution engine, and only the live
end-to-end run against it proved the fix.

Corrects the block comment to say what the tests actually do: lock in the
documented contract so a regression to a no-thread asyncio.run(coro) call
is caught immediately, without claiming they reproduce the specific
environment-dependent failure.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
---
 tests/test_ollama_provider.py | 36 ++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/tests/test_ollama_provider.py b/tests/test_ollama_provider.py
index e97e84f..f14ee46 100644
--- a/tests/test_ollama_provider.py
+++ b/tests/test_ollama_provider.py
@@ -331,19 +331,29 @@ def test_chat_structured_caches_after_successful_validation(monkeypatch):
 
 
 # ---------------------------------------------------------------------------
-# _run_async — live-verified critical bug (found running the real
-# docker-compose ComfyUI harness, not caught by any prior test): ComfyUI's
-# actual async execution engine runs node functions synchronously *inside*
-# an already-running event loop. The old implementation tried
-# `asyncio.get_running_loop()` first and only spun up an isolated worker
-# thread if that succeeded — under real ComfyUI (Python 3.13), that check
-# sometimes raised anyway, falling through to a direct `asyncio.run(coro)`
-# call on the *current* thread — the one thread guaranteed to already have
-# a loop running — reproducing "asyncio.run() cannot be called from a
-# running event loop" exactly, every time chat_structured() was called.
-# Every existing test called _run_async from a plain synchronous pytest
-# function (no ambient loop), which never exercised this path — pytest
-# alone could not have caught this.
+# _run_async — regression coverage for a bug found running the real
+# docker-compose ComfyUI harness (not caught by any prior test, and not
+# reproducible under pytest — see below). ComfyUI's actual async execution
+# engine runs node functions synchronously *inside* an already-running event
+# loop. The old implementation tried `asyncio.get_running_loop()` first and
+# only spun up an isolated worker thread if that succeeded — under real
+# ComfyUI (Python 3.13), that check sometimes raised anyway, falling through
+# to a direct `asyncio.run(coro)` call on the *current* thread — the one
+# thread guaranteed to already have a loop running — reproducing "asyncio.run()
+# cannot be called from a running event loop" exactly, every time
+# chat_structured() was called.
+#
+# Honest caveat (raised by beacon-reviewer, confirmed by reconstructing the
+# old code and running it against these tests): under CPython/pytest,
+# asyncio.get_running_loop() inside asyncio.run(outer()) never spuriously
+# raises, so the *old, buggy* implementation also takes the safe
+# worker-thread branch here and these tests pass against it too. They do not
+# reproduce the actual reported bug — only the live end-to-end run against
+# real ComfyUI did that. What these tests do lock in: the documented
+# contract ("calling _run_async from within a running loop must work and
+# must propagate exceptions"), so a regression to a naive
+# `asyncio.run(coro)`-with-no-thread implementation (which *does* fail here)
+# gets caught immediately.
 # ---------------------------------------------------------------------------