Retry the second cross-loop RuntimeError wording ('attached to a different loop') (3.8.8) - #83
Retry the second cross-loop RuntimeError wording ('attached to a different loop') (3.8.8)#83Aast12 wants to merge 1 commit into
Conversation
3.8.7 made the async client per event loop, which is what actually stops cross-loop reuse, and added 'is bound to a different event loop' to the retry-on-fresh-connection predicate as a defensive belt. That belt only covered one of the two wordings CPython emits: asyncio/mixins.py phrases it '<asyncio.locks.Event ...> is bound to a different event loop' when a pool primitive is touched from the wrong loop, but asyncio/tasks.py raises 'Task ... got Future ... attached to a different loop' when a Task awaits another loop's Future. The second string matched no predicate, so it propagated out of async_execute and async_query/async_mutate folded it into a GraphQL error list, where it reads as an unexplained failure rather than a loop problem. Observed against 3.8.6 in a Temporal worker that runs each activity through its own asyncio.run — a fresh loop per run against a process-shared client. Both wordings now self-heal via drop-and-rebuild. Predicate cases cover both strings, an end-to-end test drives async_execute through the tasks.py wording, and a per-loop test pins that the dead loop's client is released without aclose() ever being awaited on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![palantir-valiot[bot]](https://avatars.githubusercontent.com/in/3492679?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
One-line widening of _should_retry_on_fresh_connection to also admit CPython's second cross-loop RuntimeError wording (Task ... got Future ... attached to a different loop, asyncio/tasks.py), plus predicate cases and two regression tests. I verified the message verbatim against the live CPython source — the substring is exact, and it is disjoint from 3.8.7's is bound to a different event loop wording, so both predicates are genuinely needed. The retry path handles the new case correctly: a matching RuntimeError triggers _drop_async_client() (which swallows the aclose() failure a dead-loop client would produce) followed by a rebuild on the current loop. No blocking bugs found.
Findings
No actionable findings.
Notes
- Verified externally: the exact
asyncio/tasks.pywording isTask {task!r} got Future {future!r} attached to a different loop(confirmed against the CPython source), so"attached to a different loop" in msgmatches the real failure. False-positive risk is minimal given the phrase's specificity, and the worst case of a spurious match is one retry on a rebuilt client before the error propagates. - Test quality:
test_async_execute_rebuilds_client_on_cross_loop_futureexactly consumes the two-entry_get_async_clientside_effect list (initial fetch + post-drop rebuild) and asserts the drop happened — a faithful mirror of the existing closed-loop test. The parametrized predicate cases use the real message strings for both wordings. test_stale_loops_client_is_released_without_awaiting_on_its_dead_loopis a design pin rather than a regression test against current behavior — nothing in the present code path would ever awaitaclose()on the first loop's client, so the assertion passes trivially today. It still has value as a guard against a future "proactively clean up stale loops' clients" change that would reintroduce awaiting on a dead loop; the docstring makes that intent clear.- Considered flagging the deliberately untouched
uv.lock(AGENTS.md asks foruv lock --upgradeon every PR), but the PR body justifies the deviation with precedent from the last four fix PRs and an already-redpython-check-outdatedjob failing for unrelated reasons — a reasonable, explicitly stated trade-off, so noted rather than flagged. - The formatting-only hunks in
test_async_client_per_loop.pyare justified: that file landed unformatted in #82 and this PR already touches it, keepingruff format --checkgreen.
🤖 PR Reviewer · Kimi K3 (Modal)
There was a problem hiding this comment.
Pull request overview
Warning
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.
Expands the transient retry predicate to handle CPython’s second cross-event-loop RuntimeError wording (“attached to a different loop”), preventing it from surfacing to callers and ensuring client rebuild/self-heal behavior.
Changes:
- Widened
_should_retry_on_fresh_connectionto also match"attached to a different loop". - Added/updated tests covering both cross-loop error wordings and stale-loop client release behavior.
- Bumped version to 3.8.8 and documented the fix in the changelog.
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 |
|---|---|
| tests/pygqlc/gql_client/test_transient_transport_retry.py | Adds predicate cases + an end-to-end async_execute test for the “attached to a different loop” wording. |
| tests/pygqlc/gql_client/test_async_client_per_loop.py | Formats existing tests and adds a regression test asserting stale-loop clients aren’t awaited/used. |
| pygqlc/version.py | Bumps package version to 3.8.8. |
| pygqlc/GraphQLClient.py | Extends retry predicate to include the second cross-loop RuntimeError message substring. |
| CHANGELOG.md | Documents the 3.8.8 fix and context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| created[0].aclose.assert_not_awaited() | ||
| assert created[0].post.await_count == 1, "dead loop's client must not be reused" |
| response = MagicMock(status_code=200, content=b'{"data": {"ok": true}}') | ||
| created = [] | ||
|
|
||
| def make_client(**_): | ||
| mock = AsyncMock(is_closed=False) | ||
| mock.post.return_value = response | ||
| created.append(mock) |
Description
Follow-up to #82 (3.8.7). Per-loop clients are what actually stop cross-loop reuse and that fix stands — this only finishes the defensive belt it added.
3.8.7 put
"is bound to a different event loop"into_should_retry_on_fresh_connection. CPython emits two wordings for cross-loop state, and only that one was covered:asyncio/mixins.py→<asyncio.locks.Event ...> is bound to a different event loop(a pool primitive touched from the wrong loop) — already handled.asyncio/tasks.py→Task <...> got Future <...> attached to a different loop(a Task awaiting another loop's Future) — matched no predicate, so it propagated out ofasync_execute, andasync_query/async_mutatefolded it into a GraphQL error list where it reads as an unexplained failure rather than a loop problem.Observed against 3.8.6 in a Temporal worker: each activity runs through its own
asyncio.run— a fresh loop per run against the process-shared client — so a later run awaits pool primitives bound to the previous, now-closed loop. Deployments still on 3.8.6 hit this; 3.8.7 already prevents it structurally, and after this change either wording self-heals via drop-and-rebuild instead of escaping.test_async_execute_rebuilds_client_on_cross_loop_futuredrivesasync_executeend-to-end through thetasks.pywording and asserts the client is dropped and rebuilt.test_stale_loops_client_is_released_without_awaiting_on_its_dead_looppins that the first loop's client is released withoutaclose()ever being awaited on its closed loop (awaiting there would itself raise).How Has This Been Tested?
uv run pytest: base59 passed, 25 errors→ this branch63 passed, 25 errors. The 25 errors are the pre-existing env-dependentgqlsession fixture (needsAPI/WSS/TOKEN), unchanged.RuntimeError: Task ... attached to a different loopescapingasync_execute); theis bound tocase passed before and after, as expected for 3.8.7.uvx ruff format --check: clean on all tracked files. This also formatstest_async_client_per_loop.py, which landed unformatted in OPS-5449: One async client per event loop — fix cross-loop 'bound to a different event loop' (3.8.7) #82 — it's a file this PR already touches.uvx bandit -r . -s B101 -ll: 0 medium/high, unchanged.uv.lockis deliberately untouched, matching the last four fix PRs (#77, #79, #80, #82). Thepython-check-outdatedjob is already red onmainfor unrelated reasons (setuptools83,twine7 majors, and the privatevaliotloggingextra) — worth its own PR, not bundled into a one-line predicate fix.Type of change
🤖 Generated with Claude Code