fix(sdk/python): harden cross-loop teardown - #907
Conversation
|
|
Performance
✓ No regressions detected |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
Verified at d110f9a in a clean clone. The anti-tautology check is the convincing part: reverting just the two source files to main (tests kept) reproduces both #901 items exactly — the idempotency test fails with is bound to a different event loop (the double-close race) and the propagation test fails with DID-NOT-RAISE (the blanket RuntimeError swallow). Gates: ruff 0.15.22 clean, full run_pytest.sh green on 3.10 and 3.12, websockets-compat 12.0/15.0.1, and 12 consecutive repeats of the connection/deadlock suites with zero flakes. Edge probes: stopped owner loop → close() returns immediately, no hang (session leak in that case exists on main too); closed loop → graceful; restart after cross-loop close → correctly rejected. No collision with #904 (no file overlap).
Two hardening notes inline on the same code this PR touches — the first is one line and I'd fold it in here; the second is fine as a follow-up. Minor test-coverage note: the strengthened test_connection_manager_cross_loop_close_no_deadlock also passes on main, and _close_requested/the get_session() capture ordering have no direct test references — the real regression guards are the two new tests, which do cover both #901 items.
…k teardown
cancel_and_await_if_same_loop only suppressed the asyncio/tasks.py wording
("attached to a different loop"). CPython raises the same error class with a
second wording from asyncio/mixins.py ("is bound to a different event loop")
when a cancellation cleanup awaits a Lock/Event/Condition bound to a foreign
loop, so that case re-raised out of teardown instead of being absorbed.
Match both wordings. The regression test binds an asyncio.Event to a
background thread's loop and awaits it from a cancelled task's cleanup on
another loop, reproducing the mixins wording genuinely rather than raising a
synthetic RuntimeError; the existing test asserting unrelated RuntimeErrors
still propagate is unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
Re-approving at 0e1f663 (my push dismissed the earlier stamp). The wording fix is in with a regression test; independent verification reproduced both directions live — a cleanup awaiting a foreign-loop-bound primitive is now absorbed, an unrelated RuntimeError still propagates — and confirmed the test fails against d110f9a's helper. Full Python gates green (ruff 0.15.22, full suite, deadlock suite 5x). The atomicity follow-up is #909.
3 participants
Fixes #901
Summary
ConnectionManagerteardown on the owning event loop and its async lock, including task/resource state updates.close()calls return safely while teardown is pending, and keepget_session()from yielding a torn-down session.RuntimeError; propagate unrelated task-cleanup failures.Tests
uv run --frozen --extra dev pytest tests/test_async_lifecycle_deadlock.py tests/test_http_connection_manager.py -q(21 passed)pyproject.tomlreads and has nobashexecutable.