Fix infinite loop with self-referential data (#1240)

Self-referential data causes issues. Verified that the creation itself is fine.

Changed QueryResultProxy in two ways to improve situation:
- It now uses a cache to return the exact same object when recursing, so any code that already detects cycles acts correctly
- throw an error after 100 recursions to at least error more sanely.

Author: seefeldb

packages/runner/src/query-result-proxy.ts

@@ -6,6 +6,14 @@ import { type ReactivityLog } from "./scheduler.ts";
 import { diffAndUpdate, setNestedValue } from "./data-updating.ts";
 import { resolveLinkToValue } from "./link-resolution.ts";
 
+// Maximum recursion depth to prevent infinite loops
+const MAX_RECURSION_DEPTH = 100;
+
+// Cache of target objects to their proxies, scoped by ReactivityLog
+const proxyCacheByLog = new WeakMap<ReactivityLog, WeakMap<object, any>>();
+// Use this to index cache if there is no log provided.
+const fallbackLog: ReactivityLog = { reads: [], writes: [] };
+
 // Array.prototype's entries, and whether they modify the array
 enum ArrayMethodType {
   ReadOnly,
@@ -51,7 +59,15 @@ export function createQueryResultProxy<T>(
   valueCell: DocImpl<T>,
   valuePath: PropertyKey[],
   log?: ReactivityLog,
+  depth: number = 0,
 ): T {
+  // Check recursion depth
+  if (depth > MAX_RECURSION_DEPTH) {
+    throw new Error(
+      `Maximum recursion depth of ${MAX_RECURSION_DEPTH} exceeded`,
+    );
+  }
+
   // Resolve path and follow links to actual value.
   ({ cell: valueCell, path: valuePath } = resolveLinkToValue(
     valueCell,
@@ -64,7 +80,19 @@ export function createQueryResultProxy<T>(
 
   if (!isRecord(target)) return target;
 
-  return new Proxy(target as object, {
+  // Get the appropriate cache index by log
+  const cacheIndex = log ?? fallbackLog;
+  let logCache = proxyCacheByLog.get(cacheIndex);
+  if (!logCache) {
+    logCache = new WeakMap<object, any>();
+    proxyCacheByLog.set(cacheIndex, logCache);
+  }
+
+  // Check if we already have a proxy for this target in the cache
+  const existingProxy = logCache?.get(target);
+  if (existingProxy) return existingProxy;
+
+  const proxy = new Proxy(target as object, {
     get: (target, prop, receiver) => {
       if (typeof prop === "symbol") {
         if (prop === getCellLink) {
@@ -88,7 +116,12 @@ export function createQueryResultProxy<T>(
             // methods implicitly read all elements. TODO: Deal with
             // exceptions like at().
             const copy = target.map((_, index) =>
-              createQueryResultProxy(valueCell, [...valuePath, index], log)
+              createQueryResultProxy(
+                valueCell,
+                [...valuePath, index],
+                log,
+                depth + 1,
+              )
             );
 
             return method.apply(copy, args);
@@ -179,7 +212,12 @@ export function createQueryResultProxy<T>(
           };
       }
 
-      return createQueryResultProxy(valueCell, [...valuePath, prop], log);
+      return createQueryResultProxy(
+        valueCell,
+        [...valuePath, prop],
+        log,
+        depth + 1,
+      );
     },
     set: (target, prop, value) => {
       if (isQueryResult(value)) value = value[getCellLink];
@@ -221,6 +259,10 @@ export function createQueryResultProxy<T>(
       return true;
     },
   }) as T;
+
+  // Cache the proxy in the appropriate cache before returning
+  logCache.set(target, proxy);
+  return proxy;
 }
 
 // Wraps a value on an array so that it can be read as literal or object,

packages/runner/test/recipes.test.ts

@@ -1081,4 +1081,69 @@ describe("Recipe Runner", () => {
     // That same value was updated, which shows that the id was stable
     expect(ref.cell.get()).toBe(10);
   });
+
+  it("should handle pushing objects that reference their containing array", async () => {
+    const addItemHandler = handler<
+      { detail: { message: string } },
+      { items: Array<{ title: string; items: any[] }> }
+    >((event, { items }) => {
+      const title = event.detail?.message?.trim();
+      if (title) {
+        items.push({ title, items });
+      }
+    });
+
+    const itemsRecipe = recipe<
+      { items: Array<{ title: string; items: any[] }> }
+    >(
+      "Items with self-reference",
+      ({ items }) => {
+        return { items, stream: addItemHandler({ items }) };
+      },
+    );
+
+    const result = runtime.run(
+      itemsRecipe,
+      { items: [] },
+      runtime.documentMap.getDoc(
+        undefined,
+        "should handle pushing objects that reference their containing array",
+        "test",
+      ),
+    );
+
+    await runtime.idle();
+
+    // Add first item
+    result.asCell(["stream"]).send({ detail: { message: "First Item" } });
+    await runtime.idle();
+
+    const firstState = result.getAsQueryResult();
+    expect(firstState.items).toHaveLength(1);
+    expect(firstState.items[0].title).toBe("First Item");
+
+    // Test reuse of proxy for array items
+    expect(firstState.items[0].items).toBe(firstState.items);
+
+    // Add second item
+    result.asCell(["stream"]).send({ detail: { message: "Second Item" } });
+    await runtime.idle();
+
+    const secondState = result.getAsQueryResult();
+    expect(secondState.items).toHaveLength(2);
+    expect(secondState.items[1].title).toBe("Second Item");
+
+    // All three should point to the same array
+    expect(secondState.items[0].items).toBe(secondState.items);
+    expect(secondState.items[1].items).toBe(secondState.items);
+
+    // And triple check that it actually refers to the same underlying array
+    expect(firstState.items[0].items[1].title).toBe("Second Item");
+
+    const recurse = ({ items }: { items: { items: any[] }[] }): any =>
+      items.map((item) => recurse(item));
+
+    // Now test that we catch infinite recursion
+    expect(() => recurse(firstState)).toThrow();
+  });
 });