Fix "Invalid state while adding deferred fragment node" when a @defer'd field errors#273
Fix "Invalid state while adding deferred fragment node" when a @defer'd field errors#273ben-xo wants to merge 1 commit into
Conversation
`IncrementalGraph._add_deferred_fragment_node` raised an internal RuntimeError while executing a spec-valid query when two sibling top-level `@defer` fragments — each resolving a field that errors and each containing its own nested `@defer` — were executed with async resolvers. When a deferred field errors, that fragment's execution group fails and the (parentless, root) deferred fragment is removed from the root nodes. A sibling execution group completing afterwards then discovers its nested `@defer` and, in `_add_incremental_data_records`, recurses up to the now-removed parentless fragment, reaching the `# pragma: no cover` branch and raising. Return instead of raising when reaching a parentless fragment that is no longer a root node: its subtree is no longer being delivered, so there is nothing to attach it to. This mirrors the `future.cancelled()` guard added to `_enqueue` (a race with a stopping/removed consumer). Adds a regression test that reproduces the crash prior to this change. Fixes graphql-python#271
ben-xo
force-pushed
the
fix-defer-invalid-state-on-error
branch
from
a639d60 to
9be181c
Compare
|
Hi @ben-xo. Thanks a lot for the analysis and PR with regression test which all look solid to me at a first glance. Will have a deeper look later. This issue occurs in a part of the code which has since been rewritten using a WorkQueue/IncrementalExecutor approach. I did not fully port this since it is heavily based on promises and hard to translate to the world of asyncio. However, this makes me think if I should still try to port it to Python somehow so that we stay aligned with upstream and avoid such problems. |
|
I noticed the build failed on mypy - double checked - I don't think it's my work, as the failures are in pre-existing files. (Just wanted to be diligent). Seems like there was a new release of mypy yesterday. |
|
I've raised #274 for the mypy errors against mypy 2.3.0 (unrelated to this PR, but hope it helps) |
2 participants
Fixes #271 (which is an issue we were seeing in production).
Problem
IncrementalGraph._add_deferred_fragment_noderaisesRuntimeError: Invalid state while adding deferred fragment node.while executing a spec-valid query, when two sibling top-level@deferfragments — each resolving a field that errors and each containing its own nested@defer— are executed with async resolvers.We hit this in production (Strawberry over ASGI, streaming
@deferasmultipart/mixed) at ~5.7k occurrences / 48h.Cause
When a deferred field errors, that fragment's execution group fails and the (parentless, root) deferred fragment is removed from
_root_nodes. A sibling execution group completing afterwards then discovers its nested@deferand, inadd_completed_successful_execution_group → _add_incremental_data_records → _add_deferred_fragment_node, recurses up to the now-removed parentless fragment — reaching the# pragma: no coverbranch and raising.Fix
Return instead of raising when the recursion reaches a parentless fragment that is no longer a root node: its subtree is no longer being delivered, so there is nothing to attach the record to. This mirrors the
future.cancelled()guard in_enqueue("defensive guard against a race with a stopping consumer").Test
Adds a regression test in
tests/execution/test_defer.pythat reproduces the crash before this change (raising the exactRuntimeError) and passes after it. The fulltests/execution/suite passes.The crash and its minimal trigger were discovered and reduced via fuzzing of
@defer/@streamshapes with erroring async resolvers.