fix(web): scope Parallel async clients to extraction loops - #87168
Open
goslingmanagment wants to merge 1 commit into
Open
goslingmanagment wants to merge 1 commit into
goslingmanagment wants to merge 1 commit into
Conversation
`AsyncParallel` owns an HTTPX async transport whose connections are event-loop-affine, but it was cached process-wide in `tools.web_tools._async_parallel_client` while concurrent tool workers each run on their own thread-local event loop (`model_tools._get_worker_loop`). On first concurrent use all workers observe an empty cache, construct separate clients, and race to publish one. The clients that lose the race are dropped with live keep-alive connections still bound to their worker loops. When they are later finalized, the SDK's `AsyncHttpxClientWrapper.__del__` schedules `aclose()` on whatever loop is running at that moment — prompt_toolkit's — while the transports belong to loops that are gone, raising `RuntimeError: Event loop is closed`. Three concurrent first-use extractions reproduce this deterministically: three clients constructed, two unpublished, two closed-loop errors. Serializing construction with a lock does not fix this: it removes the orphans but forces a single loop-affine client to be shared across worker loops, which is the condition that makes the transports unusable. Instead, make the async client extraction-scoped and close it in `finally` on the same loop that used it. The synchronous `Parallel` client keeps its cache — it has no loop affinity. A cleanup failure must not discard work that already succeeded: `close()` funnels into `httpx.aclose()` -> `transport.aclose()`, which can raise after the response is fully materialized. Such a failure is masked from the caller but logged at warning level, so a regression of this ownership fix stays visible. `except Exception` is deliberate — `CancelledError` is a `BaseException` and must keep propagating.
Contributor
fix(web): scope Parallel async clients to extraction loops Making the async client request-scoped and closing it on the owning loop is the right fix for loop-affine httpx transports. Observations:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Prevents Parallel web-extraction clients from crossing event-loop ownership boundaries during concurrent tool execution.
AsyncParallelowns an HTTPX async transport whose connections are event-loop-affine, but it was cached process-wide intools.web_tools._async_parallel_client. Concurrent tool workers each run on their own thread-local event loop (model_tools._get_worker_loop), so a single cached client ends up with sockets bound to a loop other workers don't own.AsyncParallelis now created per extraction and closed infinallyon the same loop that performed the request. The synchronousParallelclient keeps its cache — it has no loop affinity.Related Issue
Addresses the async Parallel portion of #24736.
This corrects only the async Parallel portion of #83786; that PR's sync
ParallelandExasingleton-lock changes remain valid and are not touched here.Type of Change
Root Cause
On first concurrent use, every worker observes an empty cache, constructs its own client, and races to publish one. The clients that lose the race are dropped while still holding live keep-alive connections bound to their worker loops. When they are finalized later, the SDK's
AsyncHttpxClientWrapper.__del__schedulesaclose()on whatever loop happens to be running — prompt_toolkit's — while the transports belong to loops that are gone:Three concurrent first-use extractions reproduce it deterministically: three clients constructed, two unpublished, two closed-loop errors. The fetches themselves succeed — the crash lands afterwards, at finalization, and prompt_toolkit surfaces it as
Unhandled exception in event loopfollowed byPress ENTER to continue....Note on the alternative: serializing construction with a lock removes the orphaned clients but forces one loop-affine client to be shared across worker loops, which is the condition that makes the transports unusable in the first place.
Changes Made
plugins/web/parallel/provider.py—_get_async_client()is now a factory rather than a cache;extract()owns its client and closes it infinallyon the owning loop.plugins/web/parallel/provider.py— a cleanup failure can no longer discard a completed extraction.close()funnels intohttpx.aclose()->transport.aclose(), which can raise after the response is fully materialized; that failure is masked from the caller but logged at warning level so a regression of this ownership fix stays visible.except Exceptionis deliberate:CancelledErroris aBaseExceptionand must keep propagating so cancellation semantics are preserved.plugins/web/parallel/provider.py—_reset_clients_for_tests()no longer clears an async slot.tools/web_tools.py— removed the obsolete_async_parallel_clientcache slot.tests/tools/test_parallel_async_client_lifecycle.py— new regression coverage.How to Test
165c889, run three first-use Parallel extractions concurrently (a constructor barrier makes the publication race deterministic). The old implementation constructs three clients, leaves two unpublished and unclosed, and produces twoRuntimeError: Event loop is closedfailures at finalization.All three new tests fail on
165c889and pass with this change:test_concurrent_extract_closes_each_client_on_its_owner_loop— one client per extraction, closed on the same loop that used it, no process-wide publication.test_close_failure_does_not_discard_a_successful_extraction— asserts the close was actually attempted and exactly one warning emitted, so deleting the cleanup cannot make it pass.test_extraction_failure_outranks_a_close_failure— a secondary cleanup failure must not overwrite the primary error.Local results: 69 passed for the provider/config suites, 25/25 repeated runs of the new file with no flakes,
ruff checkandgit diff --checkclean.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — module and function docstrings in the provider now state the ownership rulecli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A