fix(model-tools): reuse persistent async bridge loop - #16573
Conversation
382152c to
ea06a63
Compare
c26200d to
d5f448c
Compare
d5f448c to
41e0889
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the thorough lifecycle and timeout coverage. The underlying issue remains present on current main: model_tools.py:113-169 still creates a per-call worker loop for a running event loop.
Problems
- The new shared-loop submission at
model_tools.py:373bypasses current main's context and approval propagation wrapper atmodel_tools.py:146-151, added by09af0a8c1d2ae55eaf78b9976619023fac41d041.tools/thread_context.py:4-14documents that a bare worker loses profile ContextVars and approval/sudo callbacks;tests/test_profile_isolation_runtime.py:188-207guards this exact_run_async()boundary. - The new bridge tests do not cover that current-main profile/approval contract, including concurrent bridge submissions.
Suggested changes
- Salvage the persistent-loop design while preserving the later task-context and approval-routing guarantees, then add profile-isolation and concurrent approval-context regressions for the bridge path.
This is an automated hermes-sweeper review.
| pool = concurrent.futures.ThreadPoolExecutor(max_workers=1) | ||
| future = pool.submit(_run_in_worker) | ||
| bridge_loop = _get_async_bridge_loop() | ||
| future = asyncio.run_coroutine_threadsafe(coro, bridge_loop) |
There was a problem hiding this comment.
This schedules directly onto a shared bridge thread and drops the current-main propagate_context_to_thread contract added in 09af0a8c1d2ae55eaf78b9976619023fac41d041. That wrapper preserves the active profile and approval/sudo callbacks for async tool dispatch; please retain an equivalent task-safe mechanism and cover it with the current profile-isolation regression.
There was a problem hiding this comment.
Addressed in 5f359bed3 after rebasing onto current main (55624e10b).
- Shared-loop submissions now preserve the caller's existing
ContextVarstate and expose the current thread-local approval/sudo callbacks through a task-local override duringrun_coroutine_threadsafe()submission. TLS remains the CLI/ACP fallback, so concurrent bridge tasks cannot share callback state. - The nested one-off fallback now uses the existing
propagate_context_to_thread()contract. - Added a concurrent two-profile/two-callback regression proving both submissions use the same bridge loop/thread while retaining isolated profile, approval, and sudo state before and after an
await; also retained and ran the current profile-isolation regression.
41e0889 to
5f359be
Compare
|
I ran a direct before/after check against today's The context-propagation concern from the review is covered as well: concurrent callers keep separate profile and approval/sudo state, and the nested fallback uses the existing thread-propagation path. I also linked #8043 as related, scoped to the running-loop/gateway part rather than claiming to close the whole issue. This should be ready for another look. |
What and why
Fixes #16570.
When
model_tools._run_async()is called from code that already owns an event loop, Hermes currently starts a new worker thread and a new event loop for every call. Cached async clients such asAsyncOpenAIand httpx can outlive those short-lived loops, leaving clients and transports tied to closed loops while the gateway keeps creating more threads and loops.This PR changes only that running-loop path. Calls are submitted to a lazily started bridge loop with
asyncio.run_coroutine_threadsafe(), while the existing main-thread and worker-thread paths keep their current behavior.Details
The nested-call fallback still uses the existing
propagate_context_to_thread()contract, so it does not lose profile or approval state when it has to move work to a one-off thread.Related
Refs #8043. This PR addresses the running-loop/gateway part of that issue, including timeout and shutdown lifecycle. Cleanup for the persistent main-thread and per-worker loops remains separate.
Testing
Before/after probe
I ran the same no-mock probe against current
main(10dc1571bc) and this branch (5f359bed3b). It calls the real_run_async()path 20 times from inside an active asyncio loop and retains the returned loop objects.Regression suites
Also checked:
python -m py_compileon the bridge and context-propagation pathsgit diff --check upstream/main...HEADscripts/check-windows-footguns.py --diff upstream/mainTested on macOS with Python 3.13.12.
Checklist
pytest tests/ -qsuite