fix(sdk/python): replace bare asyncio.run() with loop-aware helpers (#620) - #899
Conversation
…gent-Field#620) Bare asyncio.run() raises RuntimeError when called from within an already-running event loop (e.g. a sync reasoner dispatched by FastAPI, a serverless handler wrapping an async framework, or a destructor on the loop thread). This is slice 3 of Agent-Field#620. New module agentfield/run_async.py provides two helpers: - run_coroutine(coro): blocks until result. If a loop is running, runs in a new thread with its own loop so the caller can safely block. - fire_and_forget(coro): non-blocking. If a loop is running, creates a task; otherwise spawns a daemon thread. Applied to all 6 asyncio.run() sites: - agent.py handle_serverless: run_coroutine() for async reasoners - agent_serverless.py: same pattern - agent_cli.py: run_coroutine() (safe for CLI, consistent API) - agent.py note(): fire_and_forget() replaces manual loop detection + threading (was 15 lines, now 1 line) - agent.py destructor: fire_and_forget() for cleanup Tests: 9 tests in test_run_async.py covering both loop-running and no-loop cases, exception propagation, and fire-and-forget semantics. Updated test_agent_core.py to match new dispatch pattern. Part of Agent-Field#620.
Performance
✓ No regressions detected |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
santoshkumarradha
left a comment
santoshkumarradha
left a comment
There was a problem hiding this comment.
I rechecked the latest revision locally in a clean worktree. The running-loop cases look fixed and the new targeted SDK tests pass for me with uv run --extra dev pytest tests/test_run_async.py tests/test_agent_core.py -q.
One thing still feels loose before I can approve this: the no-loop fire_and_forget() path starts asyncio.run(coro) in a daemon thread without wrapping exceptions. On Python 3.13 that shows up as an unhandled thread exception in tests/test_fire_and_forget_exception_does_not_propagate, so the behavior is technically "does not propagate" but it still leaks noisy background failures to the runtime. Since this helper is now the common path for note sending and destructor cleanup, I think it should catch/log exceptions inside the worker thread rather than relying on the thread bootstrap output.
If you tighten that path, I’m happy to take another pass quickly.
Wrap the no-loop fire_and_forget() worker thread with try/except so exceptions are logged at debug level rather than leaking as unhandled thread exception tracebacks. Addresses @santoshkumarradha's review.
|
@santoshkumarradha fixed the no-loop |
santoshkumarradha
left a comment
santoshkumarradha
left a comment
There was a problem hiding this comment.
Rechecked the latest revision in a clean worktree. The loop-aware helper approach is now consistent across the touched sync entrypoints, the no-loop fire-and-forget path catches and logs background exceptions, and
no tests ran in 0.00s passed locally for me.
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
Ran the latest revision through the CI-exact gates locally (full suite on 3.11: 1881 passed, 0 failed; ruff 0.15.22 clean) and probed the fire_and_forget paths directly rather than just reading them. The no-loop exception wrapping does what Santosh asked, and except Exception (not BaseException) correctly lets KeyboardInterrupt/SystemExit through — that part looks right.
One real regression to fix before merging (inline at agent.py), and a symmetry nit on the running-loop branch that's worth folding in while you're there. Non-blocking observation: the two *_inside_running_loop tests dispatch via run_in_executor, and the executor thread has no running loop — so they actually exercise the asyncio.run() path, and the running-loop branch of run_coroutine currently has no coverage at all. Worth fixing the test plumbing in a follow-up.
| # Best-effort cleanup: schedule on running loop if one | ||
| # exists, otherwise spawn a daemon thread. Never raises | ||
| # even if a loop is already running (#620). | ||
| fire_and_forget(self._cleanup_async_resources()) |
There was a problem hiding this comment.
This swap regresses the destructor's no-loop path — the one case the old code handled correctly. asyncio.run() here ran cleanup synchronously to completion; fire_and_forget hands it to a daemon thread, and when __del__ fires at interpreter shutdown that thread is killed before it does anything, so cleanup is silently dropped. I verified side by side: the old path prints its completion marker, the new one never runs.
The PR does fix the running-loop case (which previously just raised and got swallowed), so the right shape is to keep both:
try:
asyncio.get_running_loop()
except RuntimeError:
asyncio.run(self._cleanup_async_resources())
else:
fire_and_forget(self._cleanup_async_resources())| """ | ||
| try: | ||
| loop = asyncio.get_running_loop() | ||
| loop.create_task(coro) |
There was a problem hiding this comment.
Symmetry nit: the thread path below now logs failures, but this branch still leaves the task bare — a failing coro on a running loop prints the exact Task exception was never retrieved noise this PR set out to remove (repro'd it), and an un-retained task can be GC'd mid-flight. Something like:
task = loop.create_task(coro)
_background_tasks.add(task)
def _done(t: asyncio.Task) -> None:
_background_tasks.discard(t)
if not t.cancelled() and t.exception() is not None:
logger.debug("fire_and_forget background task failed", exc_info=t.exception())
task.add_done_callback(_done)
… running (#620 follow-up) (#902) * fix(sdk/python): retain and observe fire_and_forget tasks on a running loop fire_and_forget()'s running-loop branch did a bare loop.create_task(coro). asyncio only keeps a weak reference to a task, so the task could be garbage-collected mid-flight and silently never complete, and because nobody ever retrieved the result, a failing task printed the noisy "Task exception was never retrieved" traceback on collection — exactly what the thread branch of #899 fixed for the no-loop case. Hold the task in a module-level set and attach a done callback that drops the reference and logs any failure at debug level, matching the thread branch's message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sdk/python): run destructor cleanup synchronously when no loop is running #899 replaced the bare asyncio.run() in Agent.__del__ with fire_and_forget(). That fixed the running-loop case (previously it raised RuntimeError and the exception was swallowed), but regressed the common destructor case: with no running loop, fire_and_forget() hands the coroutine to a daemon thread, and at interpreter exit that thread is killed before it does any work. AsyncExecutionManager.stop(), the background-task gather and the notification dispatcher shutdown were all silently dropped. Dispatch on loop presence instead: asyncio.run() when there is no running loop so the cleanup actually completes before __del__ returns, and fire_and_forget() only when a loop is already running. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
3 participants
Summary
Replaces all bare
asyncio.run()calls with loop-aware helpers that don't raiseRuntimeError: asyncio.run() cannot be called from a running event loop. This is slice 3 of the work on #620.The problem: sync code paths (serverless handlers, CLI dispatch, destructors) call
asyncio.run(coro)to run async reasoners. When these sync paths are invoked from within a running loop (FastAPI, uvicorn, or any async framework wrapping a sync handler),asyncio.run()raises immediately.Type of change
What changed
New module
agentfield/run_async.pywith two helpers:run_coroutine(coro)asyncio.run(). If a loop IS running, runs in a new daemon thread with its own loop so the caller can safely block without deadlocking.fire_and_forget(coro)Applied to all 6
asyncio.run()sites:agent.py(serverless handler)asyncio.run(func(**input_data))run_coroutine(func(**input_data))agent_serverless.pyasyncio.run(func(**input_data))run_coroutine(func(**input_data))agent_cli.pyasyncio.run(func(**kwargs))run_coroutine(func(**kwargs))agent.py(note)fire_and_forget(_send_note())agent.py(destructor)asyncio.run(self._cleanup_async_resources())fire_and_forget(self._cleanup_async_resources())Test plan
cd sdk/python && python -m pytest tests/test_run_async.py -v(9 tests covering both loop states, exception propagation, fire-and-forget)cd sdk/python && python -m pytest tests/test_agent_core.py tests/test_client.py tests/test_client_execution_paths.py(40 passed)cd sdk/python && ruff check .cleanTest coverage
coverage-baseline.json— N/AChecklist
Related issues / PRs
Part of #620
Follows #812 (ruff ASYNC lint gate, merged) and #799 (loop-aware teardown, pending)