fix(incremental): failed fragments should never be re-added to the graph

src/execution/IncrementalGraph.ts

@@ -132,14 +132,8 @@ export class IncrementalGraph {
     return { newRootNodes, successfulExecutionGroups };
   }
 
-  removeDeferredFragment(
-    deferredFragmentRecord: DeferredFragmentRecord,
-  ): boolean {
-    if (!this._rootNodes.has(deferredFragmentRecord)) {
-      return false;
-    }
+  removeDeferredFragment(deferredFragmentRecord: DeferredFragmentRecord): void {
     this._rootNodes.delete(deferredFragmentRecord);
-    return true;
   }
 
   removeStream(streamRecord: StreamRecord): void {
@@ -219,7 +213,10 @@ export class IncrementalGraph {
     deferredFragmentRecord: DeferredFragmentRecord,
     initialResultChildren: Set<DeliveryGroup> | undefined,
   ): void {
-    if (this._rootNodes.has(deferredFragmentRecord)) {
+    if (
+      deferredFragmentRecord.failed ||
+      this._rootNodes.has(deferredFragmentRecord)
+    ) {
       return;
     }
     const parent = deferredFragmentRecord.parent;

src/execution/IncrementalPublisher.ts

@@ -228,13 +228,13 @@ class IncrementalPublisher {
     if (isFailedExecutionGroup(completedExecutionGroup)) {
       for (const deferredFragmentRecord of completedExecutionGroup
         .pendingExecutionGroup.deferredFragmentRecords) {
-        const id = deferredFragmentRecord.id;
-        if (
-          !this._incrementalGraph.removeDeferredFragment(deferredFragmentRecord)
-        ) {
-          // This can occur if multiple deferred grouped field sets error for a fragment.
+        const failed = deferredFragmentRecord.failed;
+        if (failed) {
           continue;
         }
+        deferredFragmentRecord.failed = true;
+        const id = deferredFragmentRecord.id;
+        this._incrementalGraph.removeDeferredFragment(deferredFragmentRecord);
         invariant(id !== undefined);
         context.completed.push({
           id,

src/execution/__tests__/defer-test.ts

@@ -99,6 +99,7 @@ const a = new GraphQLObjectType({
   fields: {
     b: { type: b },
     someField: { type: GraphQLString },
+    nonNullErrorField: { type: new GraphQLNonNull(GraphQLString) },
   },
   name: 'a',
 });
@@ -1688,6 +1689,83 @@ describe('Execute: defer directive', () => {
     ]);
   });
 
+  it('Nulls cross defer boundaries, failed fragment with slower shared child execution groups', async () => {
+    const document = parse(`
+      query {
+        ... @defer {
+          a {
+            someField
+            nonNullErrorField
+            b {
+              c {
+                d
+              }
+            }
+          }
+        }
+        a {
+          ... @defer {
+            someField
+            b {
+              e {
+                f
+              }
+            }
+          }
+        }
+      }
+    `);
+    const result = await complete(document, {
+      a: {
+        b: { c: { d: 'd' }, e: { f: 'f' } },
+        someField: Promise.resolve('someField'),
+      },
+    });
+    expectJSON(result).toDeepEqual([
+      {
+        data: {
+          a: {},
+        },
+        pending: [
+          { id: '0', path: [] },
+          { id: '1', path: ['a'] },
+        ],
+        hasNext: true,
+      },
+      {
+        completed: [
+          {
+            id: '0',
+            errors: [
+              {
+                message:
+                  'Cannot return null for non-nullable field a.nonNullErrorField.',
+                locations: [{ line: 6, column: 13 }],
+                path: ['a', 'nonNullErrorField'],
+              },
+            ],
+          },
+        ],
+        hasNext: true,
+      },
+      {
+        incremental: [
+          {
+            data: { b: {}, someField: 'someField' },
+            id: '1',
+          },
+          {
+            data: { e: { f: 'f' } },
+            id: '1',
+            subPath: ['b'],
+          },
+        ],
+        completed: [{ id: '1' }],
+        hasNext: false,
+      },
+    ]);
+  });
+
   it('Handles multiple erroring deferred grouped field sets', async () => {
     const document = parse(`
       query {

src/execution/types.ts

@@ -224,6 +224,7 @@ export class DeferredFragmentRecord {
   pendingExecutionGroups: Set<PendingExecutionGroup>;
   successfulExecutionGroups: Set<SuccessfulExecutionGroup>;
   children: Set<DeliveryGroup>;
+  failed: true | undefined;
 
   constructor(
     path: Path | undefined,