Fixed child task not cancelling its task group scope when a parent scope is already cancelled - #1093
Conversation
…ope is already cancelled (agronholm#787) Made-with: Cursor
|
We're hitting this in production: Stack: anyio 4.12.1, CPython 3.12, LangGraph Platform ( LangGraph uses anyio task groups internally for streaming. When an outer scope gets cancelled (HTTP disconnect/timeout), child tasks that fail don't cancel the group's scope because The host task never learns the child failed, and cleanup hits: The minimal fix from @gschaffner's review comment on #774, removing the @agronholm No, shielding is not required for this to manifest. We're hitting it in production with no explicit shielding, just nested task groups where an outer scope gets cancelled (HTTP timeout/disconnect). See PR #1093 for the fix and details. Langraph Workaround: We monkey-patch This one-line fix PR in |
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("anyio_backend", asyncio_params) |
There was a problem hiding this comment.
We should run this on the Trio backend too:
| @pytest.mark.parametrize("anyio_backend", asyncio_params) |
| tg.start_soon(taskfunc) | ||
| with CancelScope(shield=True): | ||
| await wait_all_tasks_blocked() | ||
| await sleep(0.1) |
There was a problem hiding this comment.
This sleep is the difference between the weak and the strong case. The reason that the sleep is here is just to work around bug (2) in #787. It might be good to have a comment here about how this test intentionally waits an extra event loop cycle for the task_done callback to run on the asyncio backend.
|
Disclosure about the review above: it is not an independent review—I was the author of most of this patch, so I may have some blind spots here. |
| outer_scope.cancel() | ||
| tg.start_soon(taskfunc) | ||
| with CancelScope(shield=True): | ||
| await wait_all_tasks_blocked() |
There was a problem hiding this comment.
Should we use the version of this test that uses an event ("taskfunc_exited") instead of the version that uses wait_all_tasks_blocked? It would give me a bit more confidence that this test is testing what it's supposed to. The asyncio version of wait_all_tasks_blocked is a bit less precise than the Trio version, and the point of these awaits is to control scheduling order.
I suppose that this point matters more for the strong case than the weak case. For the weak case, as long as we wait long enough, the test is fine and will not pass when it should fail. For the strong case, the test needs to wait until a particular event loop cycle and not longer than that.
There was a problem hiding this comment.
I guess I am foreseeing that we may remove the sleep from this test in the future in order to change it from being the weak test into the strong test. Per #787: the weak case only tests for bug (1) in #787, but the strong case tests for bug (2) also. If/when we fix bug (2), I don't see a reason to retain a separate test that still has the sleep (the weak form), because bug (1) will clearly be completely covered by the strong test already, making the weak test redundant (a waste of 0.1 s every run).
In other words: if we remove task_done to fix bug (2) (i.e. we change task_done from a callback to a finally), then the line
# Wait at least one more scheduling round to ensure that taskfunc's
# done callback (task_done) on asyncio has finished. This is
# workaround for the delay that is currently present between a task
# failing and cancelling its task group on asyncio (#787, bug (2)).
await sleep(0.1)in the weak test would not make sense anymore, because bug (2) and the callback task_done would no longer exist :)
|
@danieldorado-cmyk Is this PR blocking anything for you downstream? I ask because: what I would find simplest here is to
|
|
@gschaffner I initially worked on this PR assuming it was our problem, it may help in some cases, but it doesn’t address our specific problem. Happy to mark this as draft if there are prior PRs to review. Thanks. |
Changes
On the asyncio backend, the
task_donecallback inTaskGroup._spawnskippedcalling
cancel()on the task group's cancel scope when_effectively_cancelledreturned
True(i.e. any ancestor scope was already cancelled). This meant a childtask could finish with an unhandled exception without cancelling the group's scope,
leaving the host task unaware of the failure.
This is the minimal fix suggested by @gschaffner during the review of #774
(#774 (comment)), which was
deferred at the time.
Fixes #787.
Checklist
tests/) which would fail without your patchdocs/), in case of behavior changes or new featuresdocs/versionhistory.rst).