fix(canvas): A2ATopologyOverlay re-fetch storm hammering /activity → 429

Selector instability caused fetchAndUpdate to recreate on every Zustand
nodes[] mutation (status flips, position drags, peer-discovery writes,
heartbeats — typically ~5/sec). Each recreation invalidated the
useEffect deps so the 60s polling fan-out fired on every update,
hammering /workspaces/<id>/activity?type=delegation 5×N requests/sec
until the edge rate-limit returned 429. User-reported via browser
console showing infinite uE→ux→uE→ux render loop and 429s repeating
across every visible workspace ID.

Root cause:
  const nodes = useCanvasStore((s) => s.nodes);
  const visibleIds = useMemo(() => nodes.filter(...).map(...), [nodes]);
  // useMemo dep recreates on every store update, even when ID set unchanged

Fix: select a STABLE STRING KEY (sorted CSV of visible IDs) from
Zustand. The selector's shallow-equal short-circuit prevents re-renders
when the actual visible-ID set is unchanged, so visibleIds reference
stays stable, fetchAndUpdate keeps its identity, and the useEffect
only re-fires when the visible-ID-set genuinely changes.

Tests:
- New regression test "does not re-fetch when nodes[] reference
  changes but visible IDs are the same"
- Discipline-verified: pre-fix code emits 4 fetches (2 mount + 2
  re-fetch storm), post-fix emits exactly 2
- Companion test "re-fetches when the visible ID set actually changes"
  pins the desired behavior so future "stabilization" doesn't suppress
  legitimate updates

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang 2026-05-03 23:39:36 -07:00
parent 3934325e23
commit 32fc77bad4
2 changed files with 100 additions and 6 deletions

View File

@ -138,14 +138,37 @@ export function A2ATopologyOverlay() {
// Stable Zustand action reference — safe to call inside effects
const setA2AEdges = useCanvasStore((s) => s.setA2AEdges);
// Read the nodes array as a primitive ref; derive visible IDs outside the selector
const nodes = useCanvasStore((s) => s.nodes);
// Subscribe to a STABLE STRING KEY of visible workspace IDs, not the
// nodes array itself. Zustand returns a new array reference on every
// store update (status flips, position drags, peer-discovery writes,
// workspace-tab opens, etc.) — even when the set of visible IDs is
// unchanged. Selecting a sorted-CSV string makes Zustand's default
// shallow-equal short-circuit the re-render unless the actual ID set
// changes.
//
// Why this matters: previously visibleIds was useMemo'd on `nodes`, so
// the array reference recreated on every store mutation. fetchAndUpdate
// (useCallback'd on visibleIds) then recreated, the useEffect re-fired,
// it tore down the 60s setInterval and immediately re-ran the fan-out.
// With ~5 store updates/second from heartbeats + polling, the canvas
// hammered /workspaces/<id>/activity?type=delegation 5×N requests/sec
// until edge rate-limit kicked in with HTTP 429. The recursive React
// render trace in the original bug report (uE → ux → uE → ux ...) is
// the symptom of this re-render storm.
//
// The fix is purely the dependency-stability change here; the fetch
// logic is unchanged.
const visibleIdsKey = useCanvasStore((s) =>
s.nodes
.filter((n) => !n.hidden)
.map((n) => n.id)
.sort()
.join(",")
);
// IDs of visible (non-nested, non-hidden) workspace nodes.
// Recomputed only when the nodes array reference changes.
const visibleIds = useMemo(
() => nodes.filter((n) => !n.hidden).map((n) => n.id),
[nodes]
() => (visibleIdsKey ? visibleIdsKey.split(",") : []),
[visibleIdsKey]
);
// Fetch delegation activity for all visible workspaces and rebuild overlay edges.

View File

@ -296,4 +296,75 @@ describe("A2ATopologyOverlay component", () => {
// setA2AEdges should still be called with an empty array
expect(mockStoreState.setA2AEdges).toHaveBeenCalled();
});
// Regression for the 2026-05-04 render-loop incident:
// tenant heartbeats / status flips / peer-discovery writes mutated
// canvas store .nodes ~5x/sec. Previously visibleIds was useMemo'd on
// [nodes] so the array reference recreated on every store mutation,
// causing fetchAndUpdate to recreate, the useEffect to re-fire, and
// the 60-second polling fan-out to fire on EVERY store update. With
// 5 visible workspaces and 5 store updates/sec, the canvas hammered
// /workspaces/<id>/activity?type=delegation 25×/sec until edge rate
// -limit returned 429 (per browser console captured by user).
//
// Fix: select a stable string key (sorted CSV of IDs) from Zustand
// so the selector's shallow-equal short-circuit prevents re-renders
// when the actual ID set hasn't changed.
//
// This test verifies the fetch fires ONCE on mount + only re-fires
// when the visible ID set actually changes, NOT on every nodes[]
// reference change.
it("does not re-fetch when nodes[] reference changes but visible IDs are the same", async () => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
mockGet.mockResolvedValue([] as any);
const { rerender } = render(<A2ATopologyOverlay />);
await act(async () => { await Promise.resolve(); await Promise.resolve(); });
const callsAfterMount = mockGet.mock.calls.length;
// Sanity: 2 visible nodes (ws-a, ws-b) → 2 fan-out requests on mount
expect(callsAfterMount).toBe(2);
// Simulate a store mutation that changes the nodes array reference
// (e.g. status flip on a node) WITHOUT changing the set of visible
// IDs. Pre-fix: this triggered a re-fetch storm. Post-fix: the
// sorted-CSV selector returns the same key, Zustand's shallow-equal
// short-circuits, useMemo keeps the same visibleIds, fetchAndUpdate
// keeps the same identity, useEffect does NOT re-fire.
mockStoreState.nodes = [
{ id: "ws-a", hidden: false, data: { newStatus: "online" } }, // mutated
{ id: "ws-b", hidden: false, data: {} },
{ id: "ws-hidden", hidden: true, data: {} },
];
rerender(<A2ATopologyOverlay />);
await act(async () => { await Promise.resolve(); await Promise.resolve(); });
// No additional fetches should have fired.
expect(mockGet.mock.calls.length).toBe(callsAfterMount);
});
it("re-fetches when the visible ID set actually changes", async () => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
mockGet.mockResolvedValue([] as any);
const { rerender } = render(<A2ATopologyOverlay />);
await act(async () => { await Promise.resolve(); await Promise.resolve(); });
const callsAfterMount = mockGet.mock.calls.length;
expect(callsAfterMount).toBe(2);
// Add a new visible workspace — the visible-ID-set actually changed.
mockStoreState.nodes = [
{ id: "ws-a", hidden: false, data: {} },
{ id: "ws-b", hidden: false, data: {} },
{ id: "ws-c", hidden: false, data: {} }, // NEW
{ id: "ws-hidden", hidden: true, data: {} },
];
rerender(<A2ATopologyOverlay />);
await act(async () => { await Promise.resolve(); await Promise.resolve(); });
// Should have fetched the additional workspace + the existing two
// (the effect re-fires once with the new ID set). Total: 2 + 3 = 5.
expect(mockGet.mock.calls.length).toBe(callsAfterMount + 3);
const allPaths = mockGet.mock.calls.map(([p]) => p as string);
expect(allPaths.some((p) => p.includes("ws-c"))).toBe(true);
});
});