Fix GC bug causing Fast Refresh to break on cycles with async deps

Summary:
Fix a bug in Metro's delta traversal, where a garbage cycle with an async or weak dependency would throw and fail the traversal.

This was due to the incorrect assumption that each of a `Module`'s `dependencies` was a reference to another node in the graph. In fact, weak and async+lazy dependencies are not counted references for GC purposes ("children" in [Bacon & Rajan](https://pages.cs.wisc.edu/~cymen/misc/interests/Bacon01Concurrent.pdf)), and have no corresponding entry in `inverseDependencies`.

This would throw on `nullthrows(this.dependencies.get(dependency.absolutePath))`, which is caught upstream as a traversal error. This would leave the Fast Refresh in a broken state, with the delta effectively lost, and also leave partially collected cycles in graph's internal state.

Instead, we iterate over only those dependencies with a corresponding inverse.

Changelog:
```
**[Fix]:** Fast Refresh breaks on encountering a garbage cycle with async/weak dependencies.
```

Reviewed By: motiz88

Differential Revision: D51667736

fbshipit-source-id: ab6191c1ed6cc5f454200a5eb12a4514ce4e6b70

packages/metro/src/DeltaBundler/Graph.js

@@ -202,7 +202,7 @@ export class Graph<T = MixedOutput> {
       }
     }
 
-    this._collectCycles(delta);
+    this._collectCycles(delta, internalOptions);
 
     const added = new Map<string, Module<T>>();
     for (const path of delta.added) {
@@ -709,6 +709,21 @@ export class Graph<T = MixedOutput> {
     this.#gc.color.set(module.path, 'black');
   }
 
+  // Iterate "children" of the given module - i.e. non-weak / async
+  // dependencies having a corresponding inverse dependency.
+  *_children(
+    module: Module<T>,
+    options: InternalOptions<T>,
+  ): Iterator<Module<T>> {
+    for (const dependency of module.dependencies.values()) {
+      const asyncType = dependency.data.data.asyncType;
+      if (asyncType === 'weak' || (options.lazy && asyncType != null)) {
+        continue;
+      }
+      yield nullthrows(this.dependencies.get(dependency.absolutePath));
+    }
+  }
+
   // Delete an unreachable module (and its outbound edges) from the graph
   // immediately.
   // Called when the reference count of a module has reached 0.
@@ -749,13 +764,13 @@ export class Graph<T = MixedOutput> {
   }
 
   // Collect any unreachable cycles in the graph.
-  _collectCycles(delta: Delta) {
+  _collectCycles(delta: Delta, options: InternalOptions<T>) {
     // Mark recursively from roots (trial deletion)
     for (const path of this.#gc.possibleCycleRoots) {
       const module = nullthrows(this.dependencies.get(path));
       const color = nullthrows(this.#gc.color.get(path));
       if (color === 'purple') {
-        this._markGray(module);
+        this._markGray(module, options);
       } else {
         this.#gc.possibleCycleRoots.delete(path);
         if (
@@ -770,7 +785,7 @@ export class Graph<T = MixedOutput> {
     // Scan recursively from roots (undo unsuccessful trial deletions)
     for (const path of this.#gc.possibleCycleRoots) {
       const module = nullthrows(this.dependencies.get(path));
-      this._scan(module);
+      this._scan(module, options);
     }
     // Collect recursively from roots (free unreachable cycles)
     for (const path of this.#gc.possibleCycleRoots) {
@@ -780,52 +795,43 @@ export class Graph<T = MixedOutput> {
     }
   }
 
-  _markGray(module: Module<T>) {
+  _markGray(module: Module<T>, options: InternalOptions<T>) {
     const color = nullthrows(this.#gc.color.get(module.path));
     if (color !== 'gray') {
       this.#gc.color.set(module.path, 'gray');
-      for (const dependency of module.dependencies.values()) {
-        const childModule = nullthrows(
-          this.dependencies.get(dependency.absolutePath),
-        );
+      for (const childModule of this._children(module, options)) {
         // The inverse dependency will be restored during the scan phase if this module remains live.
         childModule.inverseDependencies.delete(module.path);
-        this._markGray(childModule);
+        this._markGray(childModule, options);
       }
     }
   }
 
-  _scan(module: Module<T>) {
+  _scan(module: Module<T>, options: InternalOptions<T>) {
     const color = nullthrows(this.#gc.color.get(module.path));
     if (color === 'gray') {
       if (
         module.inverseDependencies.size > 0 ||
         this.entryPoints.has(module.path)
       ) {
-        this._scanBlack(module);
+        this._scanBlack(module, options);
       } else {
         this.#gc.color.set(module.path, 'white');
-        for (const dependency of module.dependencies.values()) {
-          const childModule = nullthrows(
-            this.dependencies.get(dependency.absolutePath),
-          );
-          this._scan(childModule);
+        for (const childModule of this._children(module, options)) {
+          this._scan(childModule, options);
         }
       }
     }
   }
 
-  _scanBlack(module: Module<T>) {
+  _scanBlack(module: Module<T>, options: InternalOptions<T>) {
     this.#gc.color.set(module.path, 'black');
-    for (const dependency of module.dependencies.values()) {
-      const childModule = nullthrows(
-        this.dependencies.get(dependency.absolutePath),
-      );
+    for (const childModule of this._children(module, options)) {
       // The inverse dependency must have been deleted during the mark phase.
       childModule.inverseDependencies.add(module.path);
       const childColor = nullthrows(this.#gc.color.get(childModule.path));
       if (childColor !== 'black') {
-        this._scanBlack(childModule);
+        this._scanBlack(childModule, options);
       }
     }
   }

packages/metro/src/DeltaBundler/__tests__/Graph-test.js

@@ -9,6 +9,29 @@
  * @oncall react_native
  */
 
+/*
+ * Diagrams in this file are created with the help of
+ * https://dot-to-ascii.ggerganov.com/ with rankdir=lr and "Boxart" mode, e.g:
+ *
+ * digraph {
+ *   rankdir = lr
+ *   "/bundle" -> "/foo"
+ *   "/foo" -> "/bar"
+ *   "/foo" -> "/baz"
+ *   "/baz" -> "/foo"
+ *   "/baz" -> "/async" [style=dotted,label="async"]
+ * }
+ *
+ * - Represent the delta visually: use thicker lines for added modules/edges,
+ *   cross out deleted modules/edges (this needs to be done manually)
+ * - Put a comment above each initialTraverseDependencies or
+ *   traverseDependencies call with the state of the graph which that call will
+ *   observe.
+ * - Ideally, keep the same graph layout from comment to comment (e.g. first
+ *   render out the most complete version of the graph and manually remove
+ *   boxes/lines as needed).
+ */
+
 import type {
   Dependency,
   MixedOutput,
@@ -845,6 +868,108 @@ describe('edge cases', () => {
     });
   });
 
+  it('removes a cycle with a weak dependency', async () => {
+    Actions.addDependency('/baz', '/foo');
+    Actions.addDependency('/baz', '/weak', {data: {asyncType: 'weak'}});
+    files.clear();
+
+    /*
+    Initial state contains a /foo-/baz cycle with a weak leaf.
+
+                      ┌────────────┐
+                      ▼            │
+    ┌─────────┐     ┌──────┐     ┌──────┐  weak    ┌───────┐
+    │ /bundle │ ──▶ │ /foo │ ──▶ │ /baz │ ·······▶ │ /weak │
+    └─────────┘     └──────┘     └──────┘          └───────┘
+                      │
+                      │
+                      ▼
+                    ┌──────┐
+                    │ /bar │
+                    └──────┘
+    */
+    await graph.initialTraverseDependencies(options);
+
+    /*
+    Remove /bundle -> /foo to cause the cycle to be collected.
+
+                        ┌────────────┐
+                        ▼            │
+    ┌─────────┐   /  ┌──────┐     ┌──────┐  weak    ┌───────┐
+    │ /bundle │ ─/─▶ │ /foo │ ──▶ │ /baz │ ·······▶ │ /weak │
+    └─────────┘ /    └──────┘     └──────┘          └───────┘
+                        │
+                        │
+                        ▼
+                      ┌──────┐
+                      │ /bar │
+                      └──────┘
+    */
+    Actions.removeDependency('/bundle', '/foo');
+
+    expect(
+      getPaths(await graph.traverseDependencies([...files], options)),
+    ).toEqual({
+      added: new Set(),
+      modified: new Set(['/bundle']),
+      deleted: new Set(['/foo', '/bar', '/baz']),
+    });
+  });
+
+  it.each([true, false])(
+    'removes a cycle with an async dependency when lazy: %s',
+    async lazy => {
+      Actions.addDependency('/baz', '/foo');
+      Actions.addDependency('/baz', '/async', {data: {asyncType: 'async'}});
+      files.clear();
+
+      /*
+      Initial state contains a /foo-/baz cycle with an async leaf.
+
+                        ┌────────────┐
+                        ▼            │
+      ┌─────────┐     ┌──────┐     ┌──────┐  async   ┌────────┐
+      │ /bundle │ ──▶ │ /foo │ ──▶ │ /baz │ ·······▶ │ /async │
+      └─────────┘     └──────┘     └──────┘          └────────┘
+                        │
+                        │
+                        ▼
+                      ┌──────┐
+                      │ /bar │
+                      └──────┘
+      */
+
+      const localOptions = {...options, lazy};
+      await graph.initialTraverseDependencies(localOptions);
+
+      /*
+      Remove /bundle -> /foo to cause the cycle to be collected.
+
+                          ┌────────────┐
+                          ▼            │
+      ┌─────────┐   /  ┌──────┐     ┌──────┐  async   ┌────────┐
+      │ /bundle │ ─/─▶ │ /foo │ ──▶ │ /baz │ ·······▶ │ /async │
+      └─────────┘ /    └──────┘     └──────┘          └────────┘
+                          │
+                          │
+                          ▼
+                        ┌──────┐
+                        │ /bar │
+                        └──────┘
+      */
+      Actions.removeDependency('/bundle', '/foo');
+
+      expect(
+        getPaths(await graph.traverseDependencies([...files], localOptions)),
+      ).toEqual({
+        added: new Set(),
+        modified: new Set(['/bundle']),
+        // The /async node was never in the graph in lazy mode.
+        deleted: new Set(['/foo', '/bar', '/baz', ...(lazy ? [] : ['/async'])]),
+      });
+    },
+  );
+
   it('removes a cyclic dependency which is both inverse dependency and direct dependency', async () => {
     Actions.addDependency('/foo', '/bundle');