Comfy-Org / Comfy-Org/ComfyUI_frontend

ECS branch: LGraph.reroutes exposes the raw Map, so .set() loses chain state and .delete() permanently poisons the id

Open
#15,633 0 comments 0 reactions 1 assignee Claimed by @DrJKL View on GitHub
area:reroutes Public API
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

`LGraph.reroutes` returns the raw internal `Map`, so `.set()` / `.delete()` bypass the registration chokepoint that the code's own docstring calls "the single entry point". Both directions half-commit, in opposite ways.

Reviewed at `5002fae1b12d44831a21367afa7c0f798f7e7a2c` (PR #14246), merge base `6532665db947acb61ed044fe91a1d4fe1fb84c8b`.

## Mechanism

`src/lib/litegraph/src/LGraph.ts:463-467` returns `reroutesInternal` unwrapped, typed as a mutable `Map`.

`_addReroute` (`LGraph.ts:1659-1666`) carries this docstring (`:1653-1657`):

> The single entry point for populating `reroutes`; routing every add through here keeps the store from silently desyncing.

Nothing enforces it.

- `reroutes.set(id, r)` **half-commits**: the reroute becomes visible to `getReroute` and to rendering, but `registerRerouteChain` never runs (`src/lib/litegraph/src/Reroute.ts:843-853`), so `_graphScope` stays unset, `membership` falls back to `EMPTY_MEMBERSHIP` (`Reroute.ts:188-192`), `linkIds` / `totalLinks` read 0 forever, and `buildMembershipIndex` breaks the chain walk at that id (`src/stores/rerouteStore.ts:75-76`) — so links routed through it lose membership too.
- `reroutes.delete(id)` **half-commits the other way**: the chain stays in `rerouteStore`, and `registerReroute` will then **permanently refuse** any later reroute reusing that id (`rerouteStore.ts:130-135`).

Add loses store state; delete leaks it and poisons the id. That is the asymmetry.

## Behaviour change

Yes, in effect. The getter is textually identical on `main` (`origin/main:src/lib/litegraph/src/LGraph.ts:322-325`), but on `main` the map was self-sufficient: a `Reroute` carried its own `parentId` and `linkIds: Set` (`origin/main:src/lib/litegraph/src/Reroute.ts:62-71, 124-127`). Direct `Map` mutation was correct there. It is not correct now, and nothing in the type signals that.

## Reachability, measured

Corpus: 29 custom-node packs on one host, 157 frontend source files (`.js`/`.ts` under `*/web/*`, excluding minified bundles, `litegraph.d.ts` and `node_modules`). Control `/registerExtension/` matched **84 files**, so this is a real zero:

`/\.reroutes\.(set|delete|clear)/` — **0 files, 0 sites, 0 packs**.

No in-tree caller misuses it either. `LGraphCanvas.ts:4277` / `:4326` operate on a **local** remap `Map`, not `graph.reroutes`. `LGraph.ts:601`'s `reroutes.clear()` is safe because `teardownOwnedGraphs` -> `unregisterAllRerouteChains` runs first (`LGraph.ts:568`, `:232-250`).

**So this is an extension-facing hazard with no observed caller, not a live bug.** Filed at that weight: the fix is cheap and the current type actively invites the misuse.

## Suggested fix

Return a read-only view (or a `Map`-shaped wrapper that routes `set`/`delete` through `_addReroute` / `_removeReroute`, as `LinkMap` already does for `graph.links`). `graph.links` is the model to copy — `LinkMap` + `MapProxyHandler` makes every mutation form commit, and it is one of the accessors this audit classified as fully honoured.

Part of a branch-wide audit of compatibility-accessor asymmetry — see the umbrella issue. Related: #15620, #15618, #15577, #15594.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.