OPS-5449: One async client per event loop — fix cross-loop 'bound to a different event loop' (3.8.7)

[doctorcorral]

Fixes OPS-5449

Description

Workers hit Error processing workflow QUEUE_REPLENISHMENT_FOR_CSV_REPORT: <asyncio.locks.Event ...> is bound to a different event loop. Root cause: 3.8.6 (#80) started caching one httpx.AsyncClient per GraphQLClient — but the client's pool primitives bind to the loop that first awaits them, so any consumer running on more than one loop breaks. The concrete trigger is valuechainos-queues' trigger_by_subscription: its callback thread runs asyncio.run(...) per event (fresh loop each time) while the same shared client also serves the worker's main loop. The error message matched no retry predicate, so it propagated instead of self-healing; it's intermittent because a fresh loop only trips when the pool still holds another loop's connections.

• The cache becomes WeakKeyDictionary[event loop → client]: each loop gets and reuses its own client (3.8.6's no-churn goal is preserved within a loop; the two goals were never in conflict).
• _drop_async_client / async_cleanup act on the running loop's client; _close() (sync, __del__ path) schedules aclose() on each client's own loop when it's still open — same GC fallback otherwise.
• "is bound to a different event loop" joins _should_retry_on_fresh_connection as a defensive self-heal (can't arise with per-loop clients; cheap insurance if stale state ever surfaces).
• New test_async_client_per_loop.py pins the contract (per-loop isolation, same-loop reuse, and the field failure end-to-end through async_execute across two asyncio.run loops); the lifecycle tests are migrated to the per-loop registry with intents unchanged.
• The pre-existing suite errors (pytest-asyncio setup assertions on env-dependent tests) are unchanged: base was 2 failed/28 errors, this branch is 0 failed/25 errors.

Follow-up in valuechainos-queues (separate PR): trigger_by_subscription will reuse one loop per listener instead of asyncio.run per callback.

🤖 Generated with Claude Code

[linear-code]

OPS-5449

[palantir-valiot]

Overall Assessment

This PR fixes a real cross-event-loop bug introduced in 3.8.6 by replacing the single shared httpx.AsyncClient with a WeakKeyDictionary[event loop → client], so each loop gets (and reuses) its own client. The design is sound: WeakKeyDictionary keys on loop objects (which are weak-referenceable), per-loop reuse preserves 3.8.6's no-churn goal, and the _close()/_drop_async_client()/async_cleanup paths correctly target each client's own loop. The defensive retry predicate addition is reasonable insurance. No blocking bugs found.

Findings

No actionable findings.

Notes

• The new test_async_client_per_loop.py pins the three core contracts well: per-loop isolation across two asyncio.run calls, same-loop reuse (constructor called once), and the end-to-end field failure through async_execute. The lifecycle tests were correctly migrated to seed the running loop's slot via the _seed/_current helpers.
• _close() correctly snapshots list(self._async_clients.items()) before clearing, and the call_soon_threadsafe lambda uses default-arg capture (l=loop, c=client) to avoid the late-binding closure bug — good attention to detail.
• Minor doc drift (not a bug): the comment block in async_execute around the retry path says "it's the only RuntimeError the predicate admits", but after this PR _should_retry_on_fresh_connection admits two RuntimeErrors ("Event loop is closed" and the newly added "is bound to a different event loop"), plus "client has been closed" was already a RuntimeError. The code behavior (drop-and-rebuild) is correct for all of them; only the comment is now inaccurate. Worth a touch-up for the next reader.
• Thread-safety of WeakKeyDictionary under concurrent loops was considered: concurrent writers on different keys is the expected case here and is no worse than the prior shared-attribute race. Not flagged.
• async_cleanup only closes the running loop's client, so clients for other (possibly closed) loops linger until the loop object is GC'd and the weak ref drops. This matches the documented "GC fallback" behavior and is inherent to the per-loop design (you can only aclose() on the loop you're running on). Not flagged.

---

🤖 PR Reviewer · GLM 5.2 (Fireworks)

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR fixes cross-event-loop failures by ensuring httpx.AsyncClient instances are cached per running event loop (instead of per GraphQLClient), preventing “bound to a different event loop” errors in multi-loop consumers.

Changes:

• Replace single cached async client with a per-event-loop registry (WeakKeyDictionary) and adjust cleanup logic to close clients on their owning loops.
• Expand retry predicate to treat cross-loop binding errors as retryable (defensive self-heal).
• Add/adjust tests to pin per-loop caching behavior and update lifecycle tests to seed/assert against the per-loop registry.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pygqlc/GraphQLClient.py	Cache async clients per event loop; schedule closure on owning loop; broaden retry predicate.
tests/pygqlc/gql_client/test_async_client_per_loop.py	New regression tests covering per-loop isolation and same-loop reuse.
tests/pygqlc/gql_client/test_async_client_lifecycle.py	Update lifecycle tests to work with per-loop async client registry.
pygqlc/__version__.py	Bump version to 3.8.7.
CHANGELOG.md	Document the per-loop async client change and its motivation.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.