diff --git a/nerve/_anyio_patch.py b/nerve/_anyio_patch.py index 7ee5091..8183942 100644 --- a/nerve/_anyio_patch.py +++ b/nerve/_anyio_patch.py @@ -1,9 +1,10 @@ -"""Monkey-patch for anyio 4.13.0 _deliver_cancellation hot-loop bug. +"""Narrow monkey-patch for anyio 4.13.0 ``_deliver_cancellation`` zombie-scope +hot-loop. -Upstream bug (anyio 4.13.0, _backends/_asyncio.py:582-616): +Upstream bug (anyio 4.13.0, ``_backends/_asyncio.py`` ~line 572-616):: for task in self._tasks: - should_retry = True # ← set unconditionally + should_retry = True # ← set unconditionally if task._must_cancel: continue if task is not current and (task is self._host_task or _task_started(task)): @@ -17,49 +18,44 @@ self._deliver_cancellation, origin ) -``should_retry`` is set to True simply because there is *any* task in the -scope's ``_tasks`` set, regardless of whether cancellation could actually be -delivered. When every task in the scope is the *current* task (a scope that -contains only itself, or a TaskGroup whose only live member is running the -cancel) nothing gets ``task.cancel()``-ed, but the scheduler reschedules -``_deliver_cancellation`` via ``call_soon`` on every event-loop tick. Result: -one CPU core pinned at 100%, tens of thousands of ``epoll_pwait`` syscalls per -second, and no forward progress. - -We have seen this trigger at least three times (April 22, 23, and 24, 2026). -The SDK-side mitigation in ``nerve.agent.engine._safe_disconnect`` only runs -during ``client.disconnect()``, so spins originating elsewhere (telegram -polling, cron, an active SDK request hitting a broken pipe) are not covered. - -The fix below sets ``should_retry = True`` only when we *actually* called -``task.cancel()`` on a task that could still receive a cancellation. We -skip three categories of tasks that upstream blindly retries on: - -1. **Done tasks** (``task.done() is True``). Root cause of the April 24 - regression: if a task completes while the scope has ``cancel_called`` - and the scope never pops it out of ``_tasks``, every subsequent pass - sees ``_must_cancel=False`` (cleared when the task finished), - ``_task_started()=True``, ``_fut_waiter=None``. Our "waiter not done" - branch fires, ``task.cancel()`` is a no-op on a done task, and yet we - flagged ``should_retry=True`` anyway. Result: infinite ``call_soon`` - on a scope whose only inhabitant is already a corpse. We observed - three such zombie-scopes running simultaneously, all spinning at - ~55k epoll_pwait/sec combined (100% CPU, load 1.6, 60°C). - -2. **Tasks already flagged with ``_must_cancel``**. asyncio's - ``Task.__step`` checks the flag when the task next resumes and will - raise ``CancelledError`` without our help. Retrying in that case means - we re-queue ourselves on every event-loop tick while the task is - blocked (shielded, awaiting I/O). This was the second iteration of - the bug (April 23 evening). - -3. **The current task**, because a task cannot cancel itself from inside - a callback. (``current_task()`` is often ``None`` here because the - callback runs from ``call_soon`` outside any task context — the check - is a belt-and-suspenders guard for the other case.) - -Import this module once, before anyio is used (i.e. very early in the -process entry point — see ``nerve/__main__.py``). +When a task lingers in ``CancelScope._tasks`` but cannot be cancelled +(``task.done()`` is True — finished task whose ``task_done`` cleanup hasn't +run yet) the upstream loop nevertheless sets ``should_retry=True`` and the +scope re-arms ``call_soon(_deliver_cancellation)`` on every event-loop tick. +Result: one CPU core pinned at ~100% with tens of thousands of +``epoll_pwait`` syscalls per second and no forward progress. + +Observed live on 2026-04-24: three simultaneous zombie-scopes in one nerve +process, each holding a single ``done=True`` task, ~55k epoll_pwait/sec +combined (load 1.6, cpu-thermal 60°C). Diagnosed via ``py-spy dump`` and a +GC scan of ``CancelScope`` instances. + +This patch is intentionally **as narrow as possible**: it adds a single +``if task.done(): continue`` skip at the top of the loop body and is +otherwise byte-for-byte identical to upstream anyio 4.13.0. Earlier wider +patches that also skipped ``current_task()`` and ``_must_cancel`` were +reverted (see ``ClickHouse/nerve#128``) because they broke legitimate +anyio cancellation semantics: + +* **Deferred self-delivery**: ``with CancelScope() as s: s.cancel(); await + sleep(5)``. ``s.cancel()`` calls ``_deliver_cancellation`` synchronously + with ``current_task()`` pointing at the host task; anyio relies on the + ``call_soon`` reschedule to redeliver on the *next* tick (when + ``current_task()`` is ``None``) and actually cancel. Skipping the current + task without setting ``should_retry`` strands the cancel — the sleep + runs to completion. + +* **Re-delivery after swallowed CancelledError**: anyio's contract is to + keep redelivering until the scope exits. Skipping tasks with + ``_must_cancel=True`` without ``should_retry=True`` strands tasks that + catch the first ``CancelledError`` and loop. + +The done-task skip avoids both problems: ``should_retry`` is set +unconditionally for every live (non-done) task, exactly like upstream, so +deferred self-delivery and re-delivery still work. Only zombie tasks are +elided. + +Import this module once, before anyio is used — see ``nerve/__init__.py``. """ from __future__ import annotations @@ -79,61 +75,47 @@ def _patched_deliver_cancellation(self, origin): # type: ignore[no-untyped-def] """Drop-in replacement for ``CancelScope._deliver_cancellation``. - Semantics: ``should_retry`` is True **only when we actually called - ``task.cancel()`` on some task in this pass**. A task already flagged - with ``_must_cancel`` does not justify a retry — asyncio's - ``Task.__step`` checks the flag when the task next resumes and will - raise ``CancelledError`` without our help. Retrying in that case means - we re-queue ourselves on every event-loop tick while the task is - blocked (shielded, awaiting I/O, or — critically — is the *current - task* running this very callback), which is the hot loop we're trying - to kill. + Identical to anyio 4.13.0 except for one early ``continue`` on + ``task.done()`` — the zombie-scope skip. Every other branch matches + upstream so legitimate cancellation (deferred self-delivery, + re-delivery on swallowed CancelledError, task-group cancel, timer + cancel) keeps working unchanged. """ should_retry = False current = current_task() for task in self._tasks: - # Already finished. anyio doesn't always remove tasks from - # ``_tasks`` before the scope's cancel callback fires (done - # tasks can linger until the task group unwinds). ``task.cancel()`` - # is a no-op on a done task, so retrying is pure busy-loop. - # This is the root cause of the April 24, 2026 spin. + # ONLY deviation from upstream: a done task can never be + # cancelled (``task.cancel()`` is a no-op), so letting upstream + # set ``should_retry=True`` for it produces the zombie-scope + # hot-loop. Skip it before ``should_retry`` is touched. if task.done(): continue - # The current task is running this callback; it can't cancel - # itself from inside it. Whatever state it's in (including - # ``_must_cancel``) will be handled once we return and control - # flows back to ``Task.__step``. - if task is current: - continue - - # Already flagged for cancellation. asyncio will deliver the - # exception when the task resumes; no retry needed from us. - # (Upstream anyio 4.13.0 set should_retry=True here — that's the - # root-cause bug.) + should_retry = True if task._must_cancel: # type: ignore[attr-defined] continue # The task is eligible for cancellation if it has started. - if task is self._host_task or _anyio_asyncio._task_started(task): + if task is not current and ( + task is self._host_task + or _anyio_asyncio._task_started(task) + ): waiter = task._fut_waiter # type: ignore[attr-defined] if not isinstance(waiter, asyncio.Future) or not waiter.done(): task.cancel(origin._cancel_reason) - # We actually delivered a cancel — re-check next tick. - should_retry = True if ( task is origin._host_task and origin._pending_uncancellations is not None ): origin._pending_uncancellations += 1 - # Deliver cancellation to child scopes that aren't shielded or running - # their own cancellation callbacks. + # Deliver cancellation to child scopes that aren't shielded or + # running their own cancellation callbacks. for scope in self._child_scopes: if not scope._shield and not scope.cancel_called: should_retry = scope._deliver_cancellation(origin) or should_retry - # Schedule another callback only if we actually did work this pass. + # Schedule another callback if there are still tasks left. if origin is self: if should_retry: self._cancel_handle = get_running_loop().call_soon( @@ -179,8 +161,8 @@ def apply() -> bool: cancel_scope_cls._deliver_cancellation = _patched_deliver_cancellation _APPLIED = True logger.info( - "anyio patch applied: CancelScope._deliver_cancellation fixed " - "(prevents 100%% CPU spin on unrecoverable cancel)" + "anyio patch applied: CancelScope._deliver_cancellation now skips " + "done tasks (prevents zombie-scope CPU spin)" ) return True diff --git a/tests/test_anyio_patch.py b/tests/test_anyio_patch.py index 45da9d7..2738178 100644 --- a/tests/test_anyio_patch.py +++ b/tests/test_anyio_patch.py @@ -1,11 +1,30 @@ -"""Regression tests for the anyio _deliver_cancellation hot-loop patch. - -See nerve/_anyio_patch.py for the root-cause explanation. +"""Regression tests for the narrow anyio ``_deliver_cancellation`` patch. + +The patch only deviates from upstream anyio 4.13.0 in one place: it skips +tasks where ``task.done()`` is True. Every other branch (deferred +self-delivery, re-delivery on swallowed ``CancelledError``, task-group +cancel, timer cancel) must behave exactly like upstream. + +The bulk of these tests are *real* anyio behavior tests, not stub tests: +they exercise the patched ``_deliver_cancellation`` through normal anyio +APIs (``CancelScope``, ``create_task_group``, ``fail_after``, +``move_on_after``) and assert observable behavior — cancellation arrives +in <0.2s, ``CancelledError`` is redelivered after swallowing, no CPU +spin. The earlier wider patch (skipping current task + ``_must_cancel``) +broke three of these patterns, which is why it was reverted. + +The single stub-based test (``test_no_hot_loop_when_only_task_is_done``) +covers the zombie-scope shape that the patch is *for*. Done tasks are +hard to construct in real anyio without racing the task-group unwind, so +a focused stub keeps the regression reliable. + +See ``nerve/_anyio_patch.py`` for the root-cause writeup. """ from __future__ import annotations import asyncio +import time import anyio import pytest @@ -15,6 +34,11 @@ from nerve import _anyio_patch +# --------------------------------------------------------------------------- # +# Module wiring # +# --------------------------------------------------------------------------- # + + def test_patch_is_applied(): """The patch must be active after ``import nerve``.""" from anyio._backends import _asyncio as anyio_asyncio @@ -28,10 +52,14 @@ def test_patch_is_applied(): def test_apply_is_idempotent(): """Calling apply() twice must not raise and must return False the 2nd time.""" - # Already applied from module import, so this should be a no-op. assert _anyio_patch.apply() is False +# --------------------------------------------------------------------------- # +# Upstream behavior that must keep working # +# --------------------------------------------------------------------------- # + + @pytest.mark.asyncio async def test_fail_after_still_works(): """Cancellation via fail_after must still raise TimeoutError.""" @@ -69,67 +97,152 @@ async def child(index): assert sorted(cancelled) == [0, 1] +# --------------------------------------------------------------------------- # +# Regressions Artem caught on PR #128 — must pass on the narrow patch # +# --------------------------------------------------------------------------- # + + @pytest.mark.asyncio -async def test_no_hot_loop_when_only_task_is_current(): - """Regression: a CancelScope whose only task is the current task must not - re-schedule ``_deliver_cancellation`` forever. +async def test_self_cancel_then_await_sleep_cancels_immediately(): + """Regression: ``with CancelScope() as s: s.cancel(); await sleep(5)`` + must exit immediately, not sleep the full 5 seconds. + + ``s.cancel()`` calls ``_deliver_cancellation`` synchronously with + ``current_task()`` pointing at the host task. anyio can't cancel the + host task in place, so it reschedules ``_deliver_cancellation`` via + ``call_soon``; on the next tick ``current_task()`` is None and the + cancel actually lands. The earlier wider patch skipped the current + task without setting ``should_retry`` → the reschedule never + happened → the sleep ran to completion. + """ + start = time.monotonic() + cancelled = False + + try: + with anyio.CancelScope() as scope: + scope.cancel() + await anyio.sleep(5) + pytest.fail( + "Unreachable: ``sleep(5)`` must raise CancelledError " + "immediately after ``s.cancel()``." + ) + except BaseException as exc: # pragma: no cover — anyio handles internally + # anyio swallows the CancelledError at scope exit; we only get here + # if something escapes. Treat any escape as a failure. + raise AssertionError(f"unexpected exception escaped scope: {exc!r}") + + elapsed = time.monotonic() - start + cancelled = scope.cancel_called and scope.cancelled_caught + assert elapsed < 0.5, ( + f"Self-cancel + await must return in <0.5s, took {elapsed:.3f}s. " + "Likely regression: deferred self-delivery is broken." + ) + assert cancelled, "Scope must report cancelled_caught after self-cancel." - This is the exact shape of the bug: ``should_retry`` used to be set to - True simply because ``self._tasks`` was non-empty, even when no task - could actually be cancelled. The scheduler then re-queued itself via - ``call_soon()`` on every event-loop tick, pinning one CPU core. - After the patch, the first pass must leave ``_cancel_handle = None`` - because nothing was delivered and nothing is awaiting pickup. +@pytest.mark.asyncio +async def test_task_cancels_own_taskgroup_scope_then_awaits(): + """Regression: a task that cancels its own task-group's scope, then + awaits, must be cancelled immediately. + + Same shape as the previous test but routed through a TaskGroup, which + is the production pattern (e.g. SDK Query teardown). The earlier + wider patch made the inner ``await sleep(5)`` run to completion. """ - from anyio._backends._asyncio import CancelScope + start = time.monotonic() + completed_sleep = False - scope = CancelScope() - # Enter/exit manually so we have a host_task bound to the running task - # without letting anyio's normal finalization run cancellation for us. - current = asyncio.current_task() - scope._host_task = current - scope._cancel_called = True - scope._cancel_reason = "regression test" - # Force the "pathological" shape: only task in scope is the current task, - # so the upstream loop would spin forever without the patch. - scope._tasks = {current} + async def child(tg): + nonlocal completed_sleep + tg.cancel_scope.cancel() + try: + await anyio.sleep(5) + completed_sleep = True + except BaseException: + raise - loop = asyncio.get_running_loop() - # Drop any stray handle so we can assert cleanly against the post-state. - scope._cancel_handle = None + async with anyio.create_task_group() as tg: + tg.start_soon(child, tg) - should_retry = scope._deliver_cancellation(scope) + elapsed = time.monotonic() - start + assert elapsed < 0.5, ( + f"Self-cancel of own task-group scope must return in <0.5s, " + f"took {elapsed:.3f}s." + ) + assert not completed_sleep, ( + "``await sleep(5)`` after self-cancelling the task group must " + "raise, not run to completion." + ) - assert should_retry is False, ( - "Patched _deliver_cancellation must not retry when the only task " - "in the scope is the current task (nothing to cancel)." + +@pytest.mark.asyncio +async def test_swallowed_cancelled_error_is_redelivered(): + """Regression: a task that swallows the first ``CancelledError`` must + be re-cancelled on the next pass. + + anyio's contract is to keep redelivering until the scope exits. The + earlier wider patch skipped tasks with ``_must_cancel=True`` without + setting ``should_retry``, so a task that caught the first cancel and + looped never received a second one — the scope hung until its + timeout, which in production took 15+ seconds. + """ + deliveries = 0 + start = time.monotonic() + + try: + with anyio.fail_after(2.0): + with anyio.CancelScope() as scope: + scope.cancel() + for _ in range(20): + try: + await anyio.sleep(0.05) + except asyncio.CancelledError: + deliveries += 1 + if deliveries >= 3: + raise + # Swallow and loop — anyio must re-cancel. + continue + pytest.fail( + "Loop should have been re-cancelled before 20 iterations." + ) + except TimeoutError: # pragma: no cover — only if redelivery is broken + pytest.fail( + "fail_after(2.0) tripped — anyio stopped redelivering after the " + "first swallowed CancelledError." + ) + + elapsed = time.monotonic() - start + assert deliveries >= 3, ( + f"Expected >=3 deliveries of CancelledError, got {deliveries}." ) - assert scope._cancel_handle is None, ( - "Patched _deliver_cancellation must clear _cancel_handle when no " - "retry is needed — otherwise the hot loop returns." + assert elapsed < 1.5, ( + f"Re-delivery loop should complete quickly, took {elapsed:.3f}s." ) - # Sanity: event loop must not have a queued _deliver_cancellation - # callback parked in it. We can't easily introspect the ready queue, but - # we *can* confirm we didn't stash a handle on the scope ourselves. - assert not loop.is_closed() + +# --------------------------------------------------------------------------- # +# Zombie-scope regression (the reason this patch exists) # +# --------------------------------------------------------------------------- # @pytest.mark.asyncio -async def test_no_hot_loop_when_current_task_has_must_cancel(): - """Regression (April 23, 2026 evening): the first version of the patch - still spun when the *current task* already had ``_must_cancel=True``. - - In production this happens when a task that's about to be cancelled is - itself running the cancel callback (via a CancelScope it owns). The - previous patch unconditionally set ``should_retry = True`` whenever it - saw ``_must_cancel``, re-queuing ``_deliver_cancellation`` on every - event-loop tick. Result: ~20% CPU / ~61k epoll_pwait/sec, nerve kept - crowning the fan. - - The fix: skip the current task entirely, and only retry when we - actually called ``task.cancel()``. +async def test_no_hot_loop_when_only_task_is_done(): + """Regression (April 24, 2026 production observation): a CancelScope + whose only task is already ``done()==True`` must not re-queue + ``_deliver_cancellation`` forever. + + Three such zombie-scopes were found in a live process dump (scope IDs + 0x7ffec1774f50, 0x7ffec17ae090, 0x7ffec17ad6d0), each spinning at + ~18k epoll_pwait/sec. Upstream sets ``should_retry=True`` for any + task in ``_tasks`` regardless of ``done()``, then ``task.cancel()`` + is a no-op on a done task → infinite ``call_soon``. + + The patched version skips done tasks before ``should_retry`` is + touched. With no other tasks, ``should_retry`` stays False, no + reschedule, scope falls idle. + + Done tasks are hard to engineer reliably in real anyio without + racing task-group cleanup, so this test stubs the task shape. """ from anyio._backends._asyncio import CancelScope @@ -137,69 +250,50 @@ async def test_no_hot_loop_when_current_task_has_must_cancel(): current = asyncio.current_task() scope._host_task = current scope._cancel_called = True - scope._cancel_reason = "regression test (must_cancel)" - scope._tasks = {current} + scope._cancel_reason = "regression test (done task)" scope._cancel_handle = None - # Simulate the production state: a task already flagged as - # "must cancel" is sitting in the scope. asyncio will deliver the - # CancelledError when the task next resumes — we must NOT re-queue - # ourselves in the meantime. - # - # The real ``_asyncio.Task._must_cancel`` attribute is read-only from - # Python, so we use a stub that mimics the shape the patch reads. - class _FakeTask: + class _DoneTask: + """Mimics an asyncio.Task that has already completed.""" + def __init__(self): - self._must_cancel = True - self._fut_waiter = None + self._must_cancel = False # cleared when task finished + self._fut_waiter = None # done tasks have no waiter self._cancel_calls = 0 def done(self): - return False + return True def cancel(self, *_args, **_kwargs): self._cancel_calls += 1 - return True + return False # cancel() returns False on done tasks - fake = _FakeTask() - scope._tasks = {fake} + done_task = _DoneTask() + scope._tasks = {done_task} should_retry = scope._deliver_cancellation(scope) - assert fake._cancel_calls == 0, ( - "Must not re-cancel a task already flagged with _must_cancel — " - "asyncio will deliver on resume." - ) assert should_retry is False, ( - "Patched _deliver_cancellation must not retry on _must_cancel " - "alone. That was the April 23 regression: the callback re-queued " - "itself every tick while the task sat with _must_cancel=True, " - "burning ~20% CPU on 61k epoll_pwait/sec." + "Patched _deliver_cancellation must not retry when the only task " + "in the scope is done. Retrying on a done task is the April 24 " + "zombie-scope spin: cancel() is a no-op, but the callback keeps " + "re-queuing itself every tick." + ) + assert done_task._cancel_calls == 0, ( + "Must not call task.cancel() on a done task — it's a no-op and " + "only serves to flip should_retry=True." ) assert scope._cancel_handle is None, ( - "No retry → no pending handle." + "No retry → no pending handle. Otherwise the zombie spin " + "returns on the next rebuild." ) @pytest.mark.asyncio -async def test_no_hot_loop_when_only_task_is_done(): - """Regression (April 24, 2026): a scope whose only task has already - ``done()==True`` must not re-queue ``_deliver_cancellation`` forever. - - Observed in production: anyio 4.13.0 does not always remove tasks from - ``CancelScope._tasks`` the instant they finish — a done task can linger - until the task group unwinds. For a done task ``_must_cancel`` is False - (cleared during final step), ``_task_started()`` is True, and - ``_fut_waiter`` is None. The previous patch therefore fell into the - "waiter not done → cancel()" branch, called ``task.cancel()`` (which is - a no-op on a done task), and still set ``should_retry=True``. Result: - a zombie-scope spinning ~55k epoll_pwait/sec per scope, pinning one - CPU core until the process was restarted. - - Three such zombies were found in a live process dump (scope IDs - 0x7ffec1774f50, 0x7ffec17ae090, 0x7ffec17ad6d0) — the live fix was to - ``discard()`` the done task and ``handle.cancel()`` the pending - ``call_soon``. This test guarantees the code-level fix holds. +async def test_live_task_still_reschedules_alongside_done_task(): + """A scope with a mix of done + live tasks must still reschedule for + the live task. The done-task skip must not short-circuit re-delivery + for other tasks. """ from anyio._backends._asyncio import CancelScope @@ -207,39 +301,43 @@ async def test_no_hot_loop_when_only_task_is_done(): current = asyncio.current_task() scope._host_task = current scope._cancel_called = True - scope._cancel_reason = "regression test (done)" + scope._cancel_reason = "regression test (mixed)" scope._cancel_handle = None class _DoneTask: - """Mimics an asyncio.Task that has already completed.""" - - def __init__(self): - self._must_cancel = False # cleared when task finished - self._fut_waiter = None # done tasks have no waiter - self._cancel_calls = 0 + _must_cancel = False + _fut_waiter = None def done(self): return True def cancel(self, *_args, **_kwargs): - self._cancel_calls += 1 - return False # cancel() returns False on done tasks + return False - done_task = _DoneTask() - scope._tasks = {done_task} + class _LiveBlockedTask: + """Live task with ``_must_cancel=True`` (already flagged) — same + shape as upstream's re-delivery path. Must keep should_retry alive.""" + + _must_cancel = True + _fut_waiter = None + + def done(self): + return False + + def cancel(self, *_args, **_kwargs): + return True + + scope._tasks = {_DoneTask(), _LiveBlockedTask()} should_retry = scope._deliver_cancellation(scope) - assert should_retry is False, ( - "Patched _deliver_cancellation must not retry on done tasks. " - "Retrying on a done task is the April 24 hot-loop: cancel() is a " - "no-op, but the callback keeps re-queuing itself every tick." + assert should_retry is True, ( + "Live task with _must_cancel=True must keep should_retry=True so " + "anyio's re-delivery contract holds (this is the test that the " + "earlier wider patch failed)." ) - assert done_task._cancel_calls == 0, ( - "Must not call task.cancel() on a done task — it's a no-op that " - "only exists to flag should_retry=True." - ) - assert scope._cancel_handle is None, ( - "No retry → no pending handle. Otherwise the zombie-scope spin " - "returns on the next rebuild." + assert scope._cancel_handle is not None, ( + "should_retry=True → must arm _cancel_handle for the next tick." ) + scope._cancel_handle.cancel() + scope._cancel_handle = None