Comfy-Org / Comfy-Org/ComfyUI_frontend

ECS branch: EMPTY_MEMBERSHIP is a shared mutable singleton, so one reroute.linkIds.add() poisons every empty reroute and is saved into the workflow

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

Description

`EMPTY_MEMBERSHIP` is a module-scoped singleton whose `Readonly<>` is type-level only. The same two `Set` objects are handed out to every reroute with no members, so one write corrupts unrelated reroutes and is then **persisted into the saved workflow**.

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

## Mechanism

`src/stores/rerouteStore.ts:22-25`:

```ts
export const EMPTY_MEMBERSHIP: Readonly = {
linkIds: new Set(),
floatingLinkIds: new Set()
} as const
```

`Readonly<>` and `as const` are erased at runtime. There is no `Object.freeze`, so both `Set`s are mutable.

It is returned from three paths:

- `getMembership` when there is no root bucket (`rerouteStore.ts:101-109`)
- `getMembership` via `?? EMPTY_MEMBERSHIP` for a registered reroute with no members (same lines)
- `Reroute.membership` when `_graphScope` is unset (`src/lib/litegraph/src/Reroute.ts:188-192`) — i.e. **any** reroute not yet registered through `registerRerouteChain` (`Reroute.ts:843-853`)

and surfaced as `Reroute.linkIds` / `Reroute.floatingLinkIds` (`Reroute.ts:198-205`).

## Why it bites

`ReadonlySet` has no `.add`, so TypeScript blocks first-party code. It does not block plain-JS extensions, and on `main` this was the normal idiom — `linkIds` and `floatingLinkIds` were plain mutable `Set` fields (`origin/main:src/lib/litegraph/src/Reroute.ts:124,127`, assigned at `:218-219`) and internal code called `.add()` directly at `origin/main:LGraph.ts:2247/2267/2287`, `LGraphNode.ts:3086`, `SubgraphInput.ts:126`.

A single `reroute.linkIds.add(id)` from any surviving caller makes **every empty and every unregistered reroute in the process** report that id. Downstream:

- `Reroute.totalLinks` (`Reroute.ts:181-183`) goes non-zero for all of them, so `LLink.disconnect` (`LLink.ts:564-568`) and `LGraph.removeFloatingLink` (`LGraph.ts:1601-1608`) stop garbage-collecting orphaned reroutes.
- `Reroute.asSerialisable()` (`Reroute.ts:669-678`) emits `linkIds: [...linkIds]`, so **the poison is written into the saved workflow** on every empty reroute.
- `findTargetInputs` (`Reroute.ts:382-383`) and `calculateAngle` (`:491-492`) iterate it.

## The populated case is a different failure

`buildMembershipIndex` (`rerouteStore.ts:64-88`) builds fresh `Set`s per evaluation and `graphMembership` (`:90-99`) memoises the `computed` per owning graph. So on a populated reroute:

- `.add()` mutates the cached computed value — reads back true until any link topology change invalidates it, then silently reverts. It never reaches the store, so no link is created.
- `.delete()` is worse than a no-op: it can drop `totalLinks` to 0 and make the **next** `LLink.disconnect` genuinely `_removeReroute` the reroute (`LLink.ts:565-566`) before the cache rebuilds.

## Reachability

`/\.reroutes\.(set|delete|clear)/` over 29 packs / 157 frontend files matched **0 files, 0 sites, 0 packs**, with control `/registerExtension/` at **84 files** — a real zero, not a dead harness. I did not find a shipped pack that calls `reroute.linkIds.add(...)` either.

So this is a latent hazard rather than an observed failure today. It is filed because the cost of the fix is one line and the failure mode reaches the persisted workflow, which puts it above the usual latent-hazard bar.

## Behaviour change

Yes. On `main` these were authoritative mutable fields. Every legacy `.add()` / `.delete()` call site that used to be correct is now either a stale-cache write or singleton poison.

## Suggested fix

`Object.freeze` both sets (writes then throw in strict mode, which every ESM module here is), or return a fresh object per call. Freezing is preferable — it converts a silent cross-object corruption into a loud, local `TypeError`.

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.