Skip to content

fix(web): serialize provider client initialization - #83786

Open
embwl0x wants to merge 2 commits into
NousResearch:mainfrom
embwl0x:agent/web-provider-client-init-locks
Open

embwl0x wants to merge 2 commits into
NousResearch:mainfrom
embwl0x:agent/web-provider-client-init-locks

Conversation

@embwl0x

@embwl0x embwl0x commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Prevents concurrent first-use calls from constructing and leaking duplicate synchronous Parallel and Exa SDK clients.

Addresses the sync Parallel and Exa portions of #24736. The loop-affine AsyncParallel lifecycle is intentionally excluded and handled by #87168.

Root cause

The two synchronous lazy getters read their canonical cache slots on tools.web_tools, constructed a client without synchronization, and then published it. A barrier reproduction on current main made simultaneous callers construct duplicate clients on both paths.

The provider migration moved these getters out of tools/web_tools.py, so synchronization belongs in the canonical modules under plugins/web/parallel and plugins/web/exa.

Changes

  • add independent first-initialization locks for sync Parallel and Exa clients
  • double-check each shared compatibility cache after acquiring its lock
  • keep credential lookup, lazy SDK setup, client construction, and Exa header setup inside the serialized initialization boundary
  • acquire the same locks in test reset helpers so reset cannot race publication
  • retain lock-free post-publication reads, documented at the fast path
  • add deterministic eight-caller regressions for both constructors; each proves one construction and one returned object identity without timing sleeps

Async ownership

This PR does not change _get_async_client() relative to current main and no longer asserts that AsyncParallel should be a process-wide singleton. AsyncParallel owns an HTTPX transport with event-loop affinity, so #87168 gives each extraction its own client and closes it on the loop that used it.

Validation

  • provider/config matrix: 108 passed
  • adjacent extraction and session-hygiene suites: 17 passed
  • two constructor-contention regressions repeated 25 times: 50 parameterized cases passed
  • ruff check on all changed Python files
  • ruff format --check on the new test
  • uv lock --check
  • git diff --check
  • contribution publish gate and gitleaks: passed

Overlap and attribution

This is the current-main refresh invited by closed PR #24741 from @wesleysimplicio. That PR identified the original races before the provider migration. The current implementation preserves the valid sync-client direction at the canonical provider owners. Thanks to @goslingmanagment for identifying the distinct async loop-ownership invariant and implementing it in #87168.

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins tool/web Web search and extraction P3 Low — cosmetic, nice to have labels Aug 11, 2026
@embwl0x

embwl0x commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

CI classification: slice 5 has the same current-main baseline failure seen on other PRs at base c0106e50e7ec: test_profile_route_and_nonmultiplexed_resolution_preserve_boundaries expects the research route to resolve, but the fixture does not mark that profile as served, so the route is rejected and the default mode is returned. This branch only changes plugins/web/{parallel,exa}/provider.py plus provider tests and does not touch gateway routing. The exact baseline correction is #83745, whose full required CI is green. All other #83786 lanes, including the other 11 Python slices, lints, OS-specific tests, scans, attribution, lock check, and both Docker architectures, passed. I am leaving the unrelated test correction out of this focused PR; once #83745 lands, rebasing will pick it up.

@embwl0x
embwl0x force-pushed the agent/web-provider-client-init-locks branch from 1fc104c to 160279b Compare August 12, 2026 13:20
@embwl0x

embwl0x commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Replacement CI is feature-clean; the sole Python failure is the unrelated wall-clock assertion in tests/gateway/test_session_hygiene.py::test_session_hygiene_timeout_continues_to_agent_and_sets_cooldown (4.445s < 2.0). This PR changes only the Parallel/Exa provider client constructors and their concurrency regression.

Fresh-process verification:

  • PR head 160279bbc: tests/gateway/test_session_hygiene.py passed 15/15 in 0.8s.
  • untouched exact base 222465d84: the same file passed 15/15 in 1.8s.
  • provider/web matrix plus the former shared-main regression: 185/185 passed.

I attempted to rerun failed jobs through gh, but GitHub rejected it because fork contributors do not have repository admin rights. Please rerun slice 8; no no-op commit or unrelated test relaxation has been added.

@embwl0x
embwl0x force-pushed the agent/web-provider-client-init-locks branch from 160279b to 347ba9d Compare August 15, 2026 10:09
@embwl0x

embwl0x commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Current-main refresh complete at 347ba9de2.

  • Rebased the Parallel/Exa singleton-initialization fix onto upstream main 7a16840ad.
  • Revalidated the 13-file provider/web matrix together with the formerly flaky session-hygiene suite: 185 passed.
  • Repeated the three constructor-contention regressions 25 times and the complete session-hygiene file 25 times without failure.
  • ruff, regression-file formatting, uv lock --check, git diff --check, public-identity/privacy checks, and gitleaks passed.
  • Replacement GitHub CI is fully green, including all 12 Python slices and both Docker architectures.

The old timing-only failure is gone on the fresh run; this PR is mergeable and ready for review.

@goslingmanagment

Copy link
Copy Markdown

The sync Parallel and Exa locks here look right, but I think the async Parallel portion should not be a locked singleton.

AsyncParallel owns an HTTPX async transport whose connections are event-loop-affine, while concurrent Hermes tool workers each run on their own thread-local event loop (model_tools._get_worker_loop). Locking initialization does remove the orphaned constructor losers, but it guarantees the remaining condition: one loop-affine client shared across worker loops. The TOCTOU race and the loop-ownership bug have the same symptom here but different fixes.

Reproduced on 165c889 with a three-worker constructor barrier: three clients constructed, two lose publication, and two later raise RuntimeError: Event loop is closed when finalized on prompt_toolkit's loop while their transports belong to worker loops that are gone. The fetches themselves succeed — the crash lands afterwards, at finalization, which is why it shows up as Unhandled exception in event loop / Press ENTER to continue... after a completed turn.

On the test in tests/plugins/web/test_provider_client_concurrency.py: the parallel-async case asserts len(constructed) == 1 and shared identity, but never uses the client — there's no await, no event loop, and beta.extract() is never called. So it pins single-instance identity, which for the async client is the invariant that produces the crash rather than the one that prevents it. The sync and Exa parameterizations aren't affected by this.

I opened #87168 with the async-side fix: make AsyncParallel extraction-scoped and close it in finally on the loop that used it, leaving the sync Parallel cache in place. With that, all three clients close on their owner loops and there are zero closed-loop errors. It also guards the close so a teardown failure can't discard an already-materialized response (logged at warning level, CancelledError still propagates).

Suggestion: keep the sync Parallel and Exa singleton locks from this PR, and drop the async singleton/cache/lock in favour of per-extraction ownership. Happy to rebase #87168 onto this branch instead if you'd prefer it all land as one change.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(web): serialize provider client initialization

The double-checked locking is correct: the unlocked fast path reads the cache slot, and the slow path re-checks under the lock before constructing and publishing, so concurrent first-use builds exactly one client. A few observations:

  1. plugins/web/exa/provider.py / plugins/web/parallel/provider.py_reset_client_for_tests() / _reset_clients_for_tests() mutate the cache slots (_wt._exa_client = None, _wt._parallel_client = None) without acquiring the corresponding lock. If a test reset races an in-flight first-use from another thread, the freshly constructed client can be published after the reset, leaving a stale client cached. Test-only, so low risk, but wrapping the reset in the same lock would make the invariant airtight.
  2. The unlocked fast path (cached = getattr(_wt, ..., None); if cached is not None: return cached) relies on CPython's GIL for a coherent reference read. That is fine in practice, but a short comment noting the fast path is intentionally lock-free (so future editors don't "fix" it into a double-lock) would help.
  3. tests/plugins/web/test_provider_client_concurrency.py — the boundary condition lock_attempts[0] == workers or len(constructed) == workers is satisfied by lock attempts alone; the assertion len(constructed) == 1 is what really guards the single-construction invariant, which is good. Minor: if reached_boundary times out, release_constructor.set() is still called and future.result(timeout=5) may still succeed — the final assert reached_boundary correctly catches that, so no change needed.
  4. Minor: the three locks are independent and never nested, so no deadlock risk; worth keeping it that way if a shared helper is ever extracted.

@embwl0x
embwl0x force-pushed the agent/web-provider-client-init-locks branch from 347ba9d to a442a77 Compare August 15, 2026 23:30
@embwl0x

embwl0x commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

@goslingmanagment Updated in a442a7704 as suggested. This PR now has no async-client diff relative to current main and no async singleton regression; it retains only the sync Parallel and Exa initialization locks. I also made the sync reset helpers acquire their corresponding locks and documented the intentionally lock-free post-publication fast paths.

The PR description now scopes this to the sync portions of #24736 and points the loop-affine async lifecycle to #87168. Verification on current main 9c58a78a7: 108 provider/config tests, 17 adjacent tests, and 25 repeated runs of both contention cases passed; Ruff, formatting, lockfile, diff, publish-gate, and gitleaks checks passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have tool/web Web search and extraction type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants