Comfy-Org / Comfy-Org/ComfyUI_frontend

widgetValueStore: removeWidget/LGraph.remove leak widget state; re-adding same-name same-type widget resurrects stale value (pre-existing on main)

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

Description

## Summary

`LGraphNode.removeWidget()` and `LGraph.remove(node)` never call `widgetValueStore.deleteWidget()`, so widget value state (and widget render state) outlives the widget. Because `registerWidget()` returns the existing state row when name+type match (the collision rule from #13773), re-adding a widget with the same name and type **resurrects the stale value**: the caller requests a default of `7` and gets the previous widget's `42`.

Measured by execution on both refs. **Pre-existing on `main` — not a regression of #14246 and not merge-blocking.** The ECS branch strictly improves cleanup relative to main (details below).

## Repro (minimal)

```ts
const node = installedFactory() // ordinary first-party node with a number widget
graph.add(node)
node.widgets![0].value = 42

node.removeWidget(node.widgets![0])
// widgetValueStore still holds value 42 for this node+name (leak)

const w2 = node.addWidget('number', 'seed', 7, () => {}) // same name+type
w2.value // => 42, not 7 (stale resurrection)
```

## Measurements

5-test characterisation probe, run at ECS branch `f1bfb313d6` and main `296fc5cd07` (2026-08-23). Instrument mutation-verified: flipping tests 2+3 to assert the fixed behaviour produces exactly those 2 failures; reverting returns 5/5 green on the branch.

| # | Probe | ECS `f1bfb313d6` | main `296fc5cd07` |
|---|-------|------------------|-------------------|
| 1 | Positive control: `graph.add` registers state; `graph.clear()` removes it | PASS — clear() purges store | **FAIL — main's clear() does not clean widgetValueStore** |
| 2 | `removeWidget`: is store state removed? | Value + render-state rows **leak**; per-node order list IS pruned (widgets mutation view commits `replaceNodeWidgetOrder`) | Value + render-state rows leak **and the order list also leaks** |
| 3 | Re-add same name+type after removeWidget | **Stale value resurrected** (42, not requested default 7) | Same — resurrected |
| 4 | `LGraph.remove(node)`: is widget state removed? | All widget state + order list **survive** (`removeNode` calls `unregisterNodeState`/`detachNodeLayout` but never `deleteWidget`) | Same — survive |
| 5 | Fresh node after removal | New id minted, fresh state (no in-session id collision) | Same |

So relative to main, the ECS branch **fixes** graph-clear cleanup and order-list pruning; the removeWidget/removeNode value-state leak and the resurrection are a **pre-existing class shared by both refs**.

## Mechanism

1. Neither `LGraphNode.removeWidget()` nor `LGraph.remove(node)` calls `widgetValueStore.deleteWidget()` (or an equivalent per-node purge). `removeNode` unregisters node state and detaches layout but leaves the widget store untouched.
2. `registerWidget()`'s collision rule (current era: #13773; history: #8594 unconditional overwrite → #12617 never-overwrite → #13773 type-check) returns the existing row when name+type match — correct for the persistence/reload path it was built for, but it turns the leak in (1) into user-visible stale values on any remove-then-re-add.

This is the demonstrated runtime face of the recycled-key hazard documented statically in #15705 (same `graphId:nodeId:name` key space); that issue asks for documentation of the collision rule, this one reports the reproducible behavioural defect.

## Suggested direction (not prescriptive)

Have the generic removal paths purge store rows: `removeWidget` → `deleteWidget(key)`; `removeNode` → delete all rows for the node (the branch's `clear()` already shows the pattern). Whether re-add-same-name should ever intentionally restore state (e.g. undo) is a design call that belongs to the store owner.

## Provenance

- Refs: ECS `f1bfb313d6`, main `296fc5cd07`; measured 2026-08-23 in isolated worktrees, vitest, node v25.9.0.
- Probe: 5-test characterisation file exercising widgetValueStore via public LGraph/LGraphNode APIs only; mutation-verified as above.
- Related: #15705 (collision-rule documentation ask, same hazard class), #14246 (ECS migration branch).

_Mirrored from Linear FE-1828._

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.