💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2a105b8b91

Thanks @vonbai — the diagnosis here is right. The unshielded await self._session_state.session_task does propagate caller-cancellation into the session task itself, cutting short _session_runner's ability to process stop_event, unwind its AsyncExitStack, and close the transport cleanly. That's worth fixing.

On the asyncio-vs-anyio question: session_task being an asyncio.Task is intentional — there's an existing comment at client.py:561-563 explaining that it has to outlive individual context manager scopes, which anyio's structured concurrency won't allow. So asyncio.shield + asyncio.CancelledError on this specific task is the right family of primitives, not an oversight.

The blocker: Codex's P1 race is real

The shield prevents session_task from being cancelled when the caller is cancelled mid-await. Combined with catching CancelledError for the force path, that opens a window where:

1. Caller is cancelled during await asyncio.shield(session_task)
2. CancelledError is caught and swallowed (force + task not cancelled)
3. finally: runs — session_task.done() is False (still unwinding in the background), so the field isn't cleared
4. _disconnect returns, lock releases
5. Concurrent _connect() acquires the lock, sees session_task is not None and not session_task.done(), treats the dying session as live, increments nesting_counter, and doesn't start a fresh runner
6. Original session task finishes, cleanup runs, transport resets
7. The "reused" session is stale — subsequent calls fail

The pre-fix behavior avoided this because an unshielded cancelled await would cancel session_task too, making .done() True before the lock released. The shield breaks that invariant.

The cleaner fix: match the existing pattern in this same function

There's already a working shielded-wait-with-timeout pattern a few lines up at client.py:570-579 for the connection-startup cancellation case:

with anyio.CancelScope(shield=True):
    with anyio.move_on_after(3):
        try:
            await session_task
        except asyncio.CancelledError:
            pass

Unlike asyncio.shield, anyio.CancelScope(shield=True) actually waits for the inner await to complete rather than re-raising cancellation immediately. By the time you release the lock, session_task.done() is True and the concurrent-reconnect race is closed. It also keeps this function to a single cancellation pattern instead of two.

Applied to the disconnect path:

session_task = self._session_state.session_task
self._session_state.stop_event.set()
try:
    if force:
        with anyio.CancelScope(shield=True):
            with anyio.move_on_after(self._disconnect_timeout):
                try:
                    await session_task
                except asyncio.CancelledError:
                    pass
    else:
        await session_task
finally:
    self._session_state.session_task = None

Please also add a regression test covering the race directly: spawn a force-close, cancel it mid-await, then immediately attempt a reconnect, and assert the new session actually works (i.e., _connect started a fresh runner rather than reusing the dying one). The existing test_multi_client_force_close doesn't exercise the cancellation path.

Other notes

• Your Validation section mentions the focused test "still emitted the existing subprocess/pytest-timeout warning shape as a warning, not a failure." Could you characterize that warning? If it's pre-existing noise unrelated to this change, say so explicitly; if it's new or related, the fix should address it.
• CI isn't running yet (only label triage is green). I'll approve workflows once the above is addressed.

Thanks!

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8fcc26eb4b