Skip to content

fix(graph): release root schedulers and destroy layers removed by useLayer - #394

Open
draedful wants to merge 1 commit into
v2from
fix/v2-layer-lifecycle
Open

draedful wants to merge 1 commit into
v2from
fix/v2-layer-lifecycle

Conversation

@draedful

@draedful draedful commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Important

Merge order: merge this PR strictly after #390 (fix/v2-use-graph-strict-mode).
In React StrictMode, useGraph calls graph.unmount() on effect replay and then reuses the memoized graph. Before this PR, the unmounted GraphLayer kept running as a zombie, so React-rendered blocks still appeared by accident. With this PR the unmounted roots are really stopped. #390 defers disposal of the graph useGraph creates itself and cancels that disposal on replay, so the graph is never unmounted there. The two branches merge cleanly (git merge-tree).

Summary

This PR fixes two lifecycle bugs in v2, plus a third leak that the new GC test exposed.

1. Root schedulers were never released

Every root created by Component.mount, which includes every graph layer, owns a Scheduler registered in globalScheduler. CoreComponent.__unmount called performRender() after unmounting and never stopped that scheduler. As a result:

  • after graph.unmount() the next tick rebuilt the layer subtrees (GraphLayer went from 1 to 4 nodes);
  • the registrations stayed in globalScheduler;
  • 0 of 20 unmounted graphs were garbage collected.

Fix: a component with no parent stops its own scheduler on unmount and skips performRender(). Children share their root's scheduler, so unmounting a child leaves it running.

2. useLayer removed layers without destroying them

useLayer cleanup called graph.detachLayer(layer). That call is reversible by contract (DevTools and PortConnectionLayer reattach layers), and it only cleans up DOM and listeners. So Component.unmount never ran for the layer or its children, and graph.unmount() could no longer find it. GraphCanvas uses useLayer(ReactLayer), so every GraphCanvas unmount left a live ReactLayer behind.

Fix: a new terminal graph.removeLayer(layer), backed by Layers.removeLayer:

  • It deletes the layer from the live set and unmounts it together with its children.
  • A WeakSet limits it to layers created by this graph, so repeated calls, layers already destroyed by graph.unmount(), and layers of other graphs are no-ops.
  • destroyLayers reuses it.
  • useLayer cleanup now calls it, and so do GraphCanvas, GraphLayer and GraphPortal through useLayer.
  • React StrictMode replay is safe: each effect run owns a new layer, so the replayed layer is destroyed and the live one is returned.
  • Both cleanup orders work, whether graph.unmount() runs before or after removeLayer.
  • graph.detachLayer and Layer.detachLayer stay reversible.

3. KeyboardService leaked document listeners

A heap-snapshot retainer search after fix 1 found this. The unsubscribe re-read graph.layers.$root, but graph.stop(true) clears $root before the state becomes INIT. The keydown/keyup listeners were therefore never removed, and document kept every started graph alive. Fix: capture the document at subscribe time.

Docs

  • v2 migration guide: a checklist line and a new section "Layer removal and root unmount".
  • layers.md.
  • Cleanup examples in PortConnectionLayer.md and the devtools README.

Tests

All of these were written first, and each failed for the stated reason.

  • CoreComponent.test.ts: an unmounted root does not rebuild its child and releases its registration; unmounting a child keeps the shared scheduler running.
  • graph.test.ts: layers of an unmounted graph are not iterated, and the number of registrations does not grow.
  • graph.gc.test.ts: 20/20 unmounted graphs are collected, including graphs whose layer was removed with removeLayer. Before the fix: 0/20. The test uses v8.setFlagsFromString("--expose-gc") and vm.runInNewContext("gc").
  • Layer.test.ts: removeLayer unmounts the layer and its children, forgets the layer, and it is never iterated again. It also destroys a detached layer exactly once, ignores layers of another graph, and ignores layers already destroyed by graph.unmount().
  • useLayer.test.ts: cleanup unmounts the layer and its children; StrictMode replay destroys the replayed layer and keeps the live one; the layer is destroyed once when the graph unmounts first.
  • GraphCanvas.test.tsx: unmounting GraphCanvas with an external graph destroys its ReactLayer exactly once.
  • KeyboardService.test.ts: after graph.unmount(), a Shift keydown no longer reaches the service. On the original code the handler fired twice.

Checks

All of these pass on top of v2 with #388:

  • pnpm run typecheck
  • pnpm run test:type-configs
  • pnpm test: graph 44 suites / 342 tests, graph-react 18 suites / 145 tests, devtools 11 tests
  • pnpm run lint: 0 errors; 1 warning, already present, in a storybook story
  • node scripts/strict-diagnostics.mjs
  • pnpm run test:package-contract

Review

There were three independent reviewer rounds. The last one reported no unresolved findings of P2 or higher.

Round 1 (P2): the last unmounted graph stays alive until the next global frame. This existed before this PR and has two independent causes: removals that GlobalScheduler defers until its next frame, and store debounces that are still pending at unmount (for example PortsStore.notifyPortMapChanged).

  • This PR documents the behaviour, and the GC test supplies that frame explicitly.
  • A full fix is left as a follow-up: cancel pending store work on unmount, and stop graph.stop() from halting the shared global loop for every graph.

Round 2 (P2): the StrictMode interaction with useGraph. It is resolved by the merge order above.

P3 findings: these were fixed: doc accuracy, foreign layers, a behaviour-based keyboard test, return types, stale examples. Two were left on purpose:

  • There is no guard against calling attachLayer after removal. The docs say a removed layer cannot be reused.
  • Built-in layers are documented as not removable, but this is not enforced.

🤖 Generated with Claude Code

…Layer

- CoreComponent: a parentless root stops its own scheduler on unmount
  instead of scheduling another render, so unmounted layer subtrees are
  no longer rebuilt and unmounted graphs can be garbage collected.
- Add terminal graph.removeLayer(layer) / Layers.removeLayer: forgets the
  layer and unmounts it with its children; repeated calls, layers already
  destroyed by graph.unmount() and foreign layers are ignored.
  graph.detachLayer stays reversible.
- useLayer (and GraphCanvas, GraphLayer, GraphPortal) destroy their layer
  on cleanup with graph.removeLayer.
- KeyboardService: capture the document at subscribe time so its
  keydown/keyup listeners are removed after the root is released.
- Docs: v2 migration note, layers.md, cleanup examples.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@draedful
draedful requested a review from Antamansid as a code owner October 16, 2026 15:40
@gravity-ui-bot

Copy link
Copy Markdown
Contributor

Preview is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants