Comfy-Org / Comfy-Org/ComfyUI_frontend

LGraph.configure(data, keep_old=true) leaves _nodes_by_id stale and renumbers the configured node

Open
#15,627 0 comments 0 reactions 1 assignee Claimed by @christian-byrne View on GitHub
agent-ok Potential Bug Public API
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Problem

`LGraph.configure(data, keep_old = true)` leaves the graph internally inconsistent:
`getNodeById(id)` returns a **detached** node that is no longer in `_nodes`, and the node that
*is* in `_nodes` has been silently renumbered.

`configure()` resets `this._nodes = []` but never resets `this._nodes_by_id`. The stale id map is
therefore still populated when the replacement nodes go through `add()`, whose duplicate-id guard
sees a live collision, logs `LiteGraph: there is already a node with this ID, changing it`, and
mints a new id for the *correct* node. `_nodes_by_id[originalId]` keeps pointing at the discarded
object.

One line. Reproduced and fixed by execution at `origin/main` `a08a7598aa`; the full litegraph suite
stays green.

## Provenance

* **Found by:** graph-identity probes written while reviewing an unrelated refactor; re-derived
against `origin/main` `a08a7598aa` for this issue on 2026-08-23
* **How:** read `LGraph.configure` and `LGraph.add` in full at `origin/main`, wrote a throwaway
vitest probe, confirmed it fails, applied the one-line fix, confirmed it passes, then ran the whole
`src/lib/litegraph` suite before and after
* **Why now:** a documented public parameter of a public API does not do what its docstring says,
and the fix is one line with a proven regression test
* **Confidence:** verified by execution

## Evidence

All anchors opened at `origin/main` `a08a7598aa`, in `src/lib/litegraph/src/LGraph.ts`.

The docstring, `:2470-2472`:

```ts
/**
* @param keep_old If `true`, the graph will not be cleared prior to
* adding the configuration.
*/
```

`clear()` is what normally resets both structures, and `keep_old` skips it — `:2492`:

```ts
if (options.clearGraph) this.clear() // options.clearGraph === !keep_old
```

`clear()` resets both, `:426-427`:

```ts
this._nodes = []
this._nodes_by_id = {}
```

But `configure` then unconditionally resets only **one** of them, `:2613-2614`:

```ts
// create nodes
this._nodes = []
```

There is no matching `this._nodes_by_id = {}`. So with `keep_old = true`, `_nodes` is empty and
`_nodes_by_id` still holds every node from before the call.

`add()` then hits its duplicate-id guard, `:984-989`:

```ts
if (node.id !== UNASSIGNED_NODE_ID && this._nodes_by_id[node.id] != null) {
console.warn('LiteGraph: there is already a node with this ID, changing it')
node.id = nextNodeId(state)
}
```

and finally overwrites the map at `:1019` — `this._nodes_by_id[node.id] = node` — under the **new**
id, leaving the original key pointing at the stale object. `getNodeById` reads that map (`:1164`).

### Executed repro at `origin/main` `a08a7598aa`

```ts
import { LGraph, LGraphNode } from '@/lib/litegraph/src/litegraph'
import { toNodeId } from '@/types/nodeId'
import { createUuidv4 } from '@/utils/uuid'

const g = new LGraph()
g.id = createUuidv4()
const n = new LGraphNode('probe')
n.id = toNodeId(1)
g.add(n)

g.configure(g.serialize(), true)

const looked = g.getNodeById(toNodeId(1))
expect(g._nodes.includes(looked)).toBe(true)
```

Result before the fix:

```
stderr: LiteGraph: there is already a node with this ID, changing it
AssertionError: expected false to be true
```

`getNodeById(1)` returns the **old, detached** node; `_nodes` contains exactly one node, and it is
the newly configured one under a freshly minted id.

The fix, one line at `:2614`, immediately after the existing `this._nodes = []`:

```ts
// create nodes
this._nodes = []
this._nodes_by_id = {}
```

After it the probe passes and the `configure`-time warning stops firing.

**Regression check, both arms executed** with `vitest run src/lib/litegraph`:

| Arm | Result |
| --- | ------ |
| baseline, no change | `Test Files 71 passed (71)` · `Tests 1048 passed \| 3 expected fail \| 5 skipped` |
| with the one-line fix + the probe | `Test Files 71 passed (71)` · `Tests 1049 passed \| 3 expected fail \| 5 skipped` |

The delta is exactly the added probe. Nothing else moved.

### Note on scope of the behaviour

`keep_old = true` has no in-repo caller other than `Subgraph.configure` forwarding its own parameter
(`LGraph.ts:2940-2942`). This is a public LiteGraph API that appears to have no current user — which
is a reason to fix it cheaply or to delete the parameter, not a reason to leave a documented flag
broken. Treat it as low priority and low risk.

`git blame` on the `configure` body lands on a bulk import commit rather than on authorship, so do
not assign this from blame.

## Acceptance criteria

- [ ] `LGraph.configure()` resets `this._nodes_by_id` wherever it resets `this._nodes`, so the two
structures cannot disagree after the call
- [ ] A regression test that runs `configure(serialised, true)` on a graph with at least one existing
node and asserts **all three** of: `getNodeById(id)` returns a node that is in `_nodes`; the
node keeps its original id (no renumbering); `_nodes.length` is what the serialised data
describes. Asserting only the first would let a future change satisfy it by renumbering
- [ ] The test also covers the default `keep_old = false` path staying correct, so the fix cannot
regress the common case
- [ ] The test is mutation-verified: remove the added line, confirm the new test goes red, restore
it, confirm green. Report the before/after `vitest run src/lib/litegraph` counts in the PR
- [ ] `vitest run src/lib/litegraph` is green, with the same `expected fail` and `skipped` counts as
before your change

## Out of scope

* **Do not remove or deprecate the `keep_old` parameter.** Whether a parameter with no callers should
exist at all is a maintainer's call, not this issue's.
* **Do not touch `LGraph.clear()`, `LGraph.add()`'s renumbering behaviour, or `nextNodeId`.** The
duplicate-id guard on `:984` is behaving correctly given the state it is handed; the bug is the
state, not the guard.
* **Do not touch `Subgraph.configure` (`:2940`) or `deduplicateSubgraphNodeIds`.** Subgraph id
deduplication is a separate mechanism with its own tests.
* **Anything on the `feature/ecs-migration` branch.** This issue is about `main`. That branch has a
different node-registration mechanism and different line numbers; do not read it, target it, or
try to reconcile the two.

## Working notes for whoever picks this up

Traps in this repo that will otherwise cost you a cycle:

* **Import litegraph classes from the barrel**, `@/lib/litegraph/src/litegraph`, never from
`'./LGraphNode'` or `'./LGraph'` directly. A deep import makes the suite die at collection with
`Class extends value undefined` and report **zero tests run**, which is easy to misread as green.
`toNodeId` is the exception — it lives at `@/types/nodeId`, not on the barrel.
* **This is a pnpm workspace with eight `node_modules` directories** — the root plus
`apps/desktop-ui`, `apps/website`, and `packages/{design-system,object-info-parser,tailwind-utils,shared-frontend-utils,ingest-types}`.
If you work in a worktree with only the root linked, `vitest` dies at
`Failed to resolve import "clsx"` **at every commit**, which makes any comparison meaningless.
* **Run a positive control before trusting any red result.**
`vitest run src/lib/litegraph/src/LLink.test.ts` should report `Tests 3 passed (3)`.
* **Vitest 4 has no `basic` reporter.** `--reporter=basic` exits 1 with
`Failed to load custom Reporter from basic` before running anything. Omit the flag.
* **There is no root `vitest.config.ts`.** The config is `vite.config.mts`, read by default.
`vitest run --config vitest.config.ts` dies with `UNRESOLVED_ENTRY`.
* **`vue-tsc --noEmit` OOMs at Node's 2GB default heap and exits 134**, and a grep filtered for
`error TS` then returns zero — byte-identical to a clean run. Run it as
`NODE_OPTIONS=--max-old-space-size=8192 vue-tsc --noEmit` and always report the exit code.
* **The suite has 3 `it.fails` tests in this tree that are expected to fail.** A run reporting
`3 expected fail` is green. Do not "fix" them.

---

Original issue text, preserved verbatim (this body was narrowed on 2026-08-23 to be finishable from the issue alone)

`LGraph.configure(data, keep_old = true)` leaves the graph in a state where `getNodeById()` returns a detached node that is not in `_nodes`.

Repro, verified by execution:

```ts
const g = new LGraph()
g.id = createUuidv4()
const n = new LGraphNode('probe')
n.id = toNodeId(1)
g.add(n)
g.configure(g.serialize(), true)

g.getNodeById(toNodeId(1)) === n // true — the OLD, detached node
g._nodes.includes(g.getNodeById(toNodeId(1))) // false
g._nodes.map(x => x.id) // ["1"] — renumbered to a string id
```

Root cause: `configure` resets `this._nodes = []` (`LGraph.ts:2720`) but not `this._nodes_by_id`. The old node is therefore still registered when the replacement with the same id goes through `add()`, where `while (!registerNodeState(this, node)) node.id = mintNodeId(state)` (`LGraph.ts:1171`) sees a live collision and renumbers it. `_nodes_by_id[1]` keeps pointing at the stale object. LiteGraph logs its own `"there is already a node with this ID, changing it"` while this happens.

**Pre-existing, not an ECS regression.** I ran the merge-base arm in a separate worktree at `6532665db947acb61ed044fe91a1d4fe1fb84c8b` and got the identical shape: `sameObject: true`, `inNodes: false`, `ids: ["1"]`, same warning.

What #14246 adds on top is that the surviving renumbered node also has no `layoutStore` entry (`_layoutRegistered === false`), because `configure`'s `else detachGraphLayouts([this])` branch (`LGraph.ts:2602`) deletes the entries with `removeLayouts: true` and only the renumbered id gets re-created.

**Reachability: none found.** `keep_old = true` has zero in-repo callers other than `Subgraph.configure` forwarding its own parameter (`LGraph.ts:2998`). Across a local corpus of 29 custom-node packs / 157 frontend files, `.configure(x, true)` matches **0 files, 0 sites, 0 packs** — control arm `/registerExtension/` live at 84 of 157 files. It is a public LiteGraph API with no user I can find.

So: low priority. Filing it because it is a documented public parameter that does not work, and because "nobody calls it" is a reason to either fix it cheaply or delete it, not a reason to leave it.

`git blame` on `LGraph.ts:2720` lands on `aff7f2a296` (`#8070`), a 2,238-file / +612,812-line bulk import — not authorship. Leaving this unassigned rather than naming the wrong person.

Related: #15620, #15618, #15594, #15577. Found reviewing #14246 (slice C3, layout/geometry).

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.