Conversation
|
@duckki is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel. A member of the Team first needs to authorize it. |
duckki
marked this pull request as draft
September 24, 2026 22:58
Author
|
This is a real bug. But, the current proposed fix may not be the right fix. I'm validating a new fix.
|
Author
|
Updated the fix to retain failures and buffered values until their defer groups are announced, and to cancel children discovered after their parent fails. |
duckki
marked this pull request as ready for review
September 28, 2026 14:04
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
{ ... @defer(label: "R") { bad } ... @defer(label: "P") { slow ... @defer(label: "C") { bad } } }With
bad: String!returningnullandslow: Stringreturning"ok",badis shared by R and C. Initially, only R and P are pending. Upstream completes both R and C when the shared task fails, before P releases C, so the response contains acompletedID for C, which never appeared inpending.The draft spec's Incremental Completion Notice requires the associated pending notice to appear in an earlier response or the same response. C needs to delay its failure notice until P succeeds and announces it. If P instead fails, C is cancelled with P and doesn't need to be announced.
The earlier guard in this PR suppressed the invalid completion but discarded C's failure. Further tests expose the same retention problem for successful values:
{ ... @defer(label: "R") { bad x } ... @defer(label: "P") { slow ... @defer(label: "C") { x } } }All three resolvers start. Settle
bad = null, thenx = "X", thenslow = "ok", allowing each settlement to reach the queue.pruneEmptyGroupssees C's pending count reach zero and deletes it, even though C still owns the unpublished value. The response terminates without deliveringx. Only if the last two settlements are reversed,xis delivered successfully. Both schedules are tested with early execution enabled and disabled.This conflicts with the draft's pending notice semantics: when no notice is returned, the corresponding data should already be available in the initial or prior results. Here the successful value is absent everywhere.
Finally, a shared producer can reveal a nested defer group after its parent has failed and been removed. Without remembering failed parents, the late child can remain an owner of shared work and affect later failure reporting. A regression covers this with and without an intervening empty defer group.
Proposed solution
Treat deferred outcomes as retained state until their groups are released or cancelled:
These changes belong together: retaining values without draining can stall a group, while suppressing early failure notices without retention loses the group's error.
The follow-up commits first characterize the previous candidate with passing tests, then implement the correction and update the expectations. The tests include validated public operations and direct queue regressions for retained child streams, cancelled late outcomes, shared-value deduplication, and multiple accepted failures.
Verification
npm teststops at the Deno runtime check because Deno is not installed locally. The separately invoked integration suite is blocked by missing Docker.(Follow-up PR: Error collection before a task's non-null failure is handled separately in #4861.)