Comfy-Org / Comfy-Org/ComfyUI_frontend

Assert registration-identity invariants in registerNodeState/nodeDataStore (feature/ecs-migration)

Open
#15,706 1 comment 0 reactions 1 assignee Claimed by @christian-byrne View on GitHub
area:nodes claimed
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

Repo: Comfy-Org/ComfyUI_frontend. **Base branch:** `feature/ecs-migration` — NOT main. The code this ticket touches (`src/stores/nodeDataStore.ts`, `registerNodeState`/`unregisterNodeState` at the end of `src/lib/litegraph/src/LGraphNode.ts`) was merged there by #13963 (merge commit `6d1cd8e`) and does not exist on main. Branch from `feature/ecs-migration` and open the PR with base `feature/ecs-migration`.

**Step 0 — dedupe check**: Alexander Brown (DrJKL) said on #13963 they'd add these asserts as TODOs on their stack. Before writing code, grep the current `feature/ecs-migration` head for `assert(` in `LGraphNode.ts` and `nodeDataStore.ts` and check DrJKL's open PRs based on that branch. If the asserts below already exist, close this ticket with a comment instead of duplicating.

**Problem**: three silent failure modes in the node-data-store registration lifecycle:

1. `registerNodeState(graph, node)` sets `node._graphId = graph.rootGraph.id` unconditionally. If the node was already registered under a **different** root graph, its old bucket entry is stranded (membership is by state identity; nothing removes it) → ghost node state.
2. `unregisterNodeState(node)` calls `useNodeDataStore().deleteNode(node._graphId, node._state)` and **ignores the boolean return**. `false` means the bucket didn't contain `node._state` — identity drift (e.g. `_state` reassigned after registration), leaving a ghost `NodeState` the renderer keeps drawing.
3. `nodeDataStore` buckets are `Set` (identity-keyed), so two distinct `NodeState` objects with the same `(id, graphId)` can coexist → duplicate rendered nodes. Per docs/architecture/node-data-store.md, id-uniqueness is supposed to be guaranteed upstream by `subgraphDeduplication.ts` — this assert catches regressions of that guarantee.

**Change** (import `assert` from `@/base/assert`):

1. In `registerNodeState` (end of LGraphNode.ts): before assigning, `assert(node._graphId === undefined || node._graphId === graph.rootGraph.id, \`registerNodeState: node ${[node.id]()} already registered under a different root graph`)`.
2. In `unregisterNodeState`: capture the return — `const deleted = useNodeDataStore().deleteNode(node._graphId, node._state); assert(deleted, \`unregisterNodeState: state for node ${[node.id]()} not found in bucket (identity drift)`)`.
3. In `src/stores/nodeDataStore.ts` `registerNode`: detect a *different* state object already registered for the same `(id, graphId)` in the bucket. Do NOT scan the Set per registration (O(n²) on graph load). Maintain an auxiliary `Map` keyed `` `${state.graphId}:${state.id}` `` alongside each bucket, updated in `registerNode`/`deleteNode`/`clearGraph`: on register, `assert(existing === undefined || existing === state, \`nodeDataStore: duplicate NodeState for node ${[state.id]()} in graph ${state.graphId}`)`. Note the same state object re-registering (idempotent Set.add) must NOT assert — `registerNodeState`re-wraps via`reactive()`which returns a cached proxy, so idempotent re-registration is an existing legitimate path (see`useNodeReplacement.ts`unregister→register sequences). Also handle renumbering:`[LGraphNode.id]()`can change after registration (the #13963 store doc explicitly says "renumbering a registered node cannot strand its entry" because membership is identity-based) — so on`deleteNode`, remove the aux-map entry by looking up the state identity (iterate the small map or key it by state via a reverse WeakMap), NOT by recomputing `${graphId}:${id}`from possibly-renumbered fields. Simplest robust shape:`Map` reverse map alongside the forward map; implementer's choice as long as renumbered nodes unregister cleanly — add a unit test for register → renumber id → unregister.

**Tests**: extend `src/stores/nodeDataStore.test.ts` (exists on the branch) and `src/lib/litegraph/src/LGraphNode.nodeState.test.ts`: (a) cross-root re-registration asserts, (b) unregister after `_state` reassignment asserts, (c) duplicate (id,graphId) distinct-state registration asserts, (d) idempotent same-state re-registration does NOT assert, (e) register→renumber→unregister stays clean. `assert` throws in DEV so the existing suite on the branch is the false-positive net — `pnpm test:unit` must be fully green.

---
_Mirrored from Linear [BE-5048](https://linear.app/comfyorg/issue/BE-5048/assert-registration-identity-invariants-in)._ Linear status was **Done** at mirror time — verify the asserts described below actually exist on `feature/ecs-migration` before redoing the work; if they're already present, close this with a comment instead.

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.