Comfy-Org / Comfy-Org/ComfyUI_frontend
ECS branch: LLink endpoint setters half-apply a two-statement retarget and swallow the rejection
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
The four `LLink` endpoint setters route a **single-field** patch through the link store and swallow rejection, so a legacy two-statement retarget can half-apply.
Reviewed at `5002fae1b12d44831a21367afa7c0f798f7e7a2c` (PR #14246), merge base `6532665db947acb61ed044fe91a1d4fe1fb84c8b`.
## Mechanism
- `origin_id` / `origin_slot` / `target_id` / `target_slot` (`src/lib/litegraph/src/LLink.ts:160-193`) each call `applyEndpointPatch` with one field.
- `applyEndpointPatch` (`LLink.ts:108-121`) calls `useLinkStore().updateEndpoint(...)` and on failure does **only** `console.error('Failed to update link endpoints', result.error)`. The caller sees nothing; a read-back returns the old value.
- `updateEndpoint` is the single-item form of `updateEndpoints` and rejects an occupied destination with `code: 'occupied-target'` (`src/stores/linkStore.ts:319-325`, `:352-359`).
The batch API that would make a move atomic exists — `updateEndpoints` — but the compatibility accessors expose only the single-item form.
## Why the asymmetry matters
The legacy retarget idiom is two writes:
```js
link.target_id = node.id
link.target_slot = idx
```
This is **not atomic on this branch**. Step 1 is validated against the *old* slot index, step 2 against the *new* node. The first can commit and the second be rejected, leaving the link at an endpoint the caller never requested.
Concretely for a **floating** link: step 1 sets `targetNodeId` while `targetSlot` is still `-1`, so `isFloatingTopology` is false (`src/types/linkTopology.ts:28-33`), `hasUniqueTarget` is true, key `g:node:-1` is free, and it commits. Step 2 then hits `occupied-target` and is rejected. Net result is a link that was floating and is now a live link with `target_slot === -1` — present in `graph.links`, absent from `graph.floatingLinks`, and it serialises that way (`LLink.asSerialisable`, `LLink.ts:586-597`).
## 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** on this run, so a zero here would be a real zero.
`/(origin_slot|target_slot|origin_id|target_id)\s*=[^=]/` matched **3 files, 5 sites, 1 pack**.
The reachable path is rgthree Dynamic Context:
- `web/comfyui/dynamic_context_base.js:166` — `app.graph.links[input.link].target_slot = index`
- `web/comfyui/dynamic_context_base.js:176` — `app.graph.links[link].origin_slot = index`
both inside `fixInputsOutputsLinkSlots()`, called from `dynamic_context_base.js:111` (`addContextInput`) and `dynamic_context_switch.js:103`. **The user gesture is "add an input to a Dynamic Context node".** Both loops run descending over slot indices, which is exactly the ordering where a renumber can transiently collide with an incumbent.
`web/comfyui/utils.js:588,591` is the same shape in the slot-name realign path.
The indexed access itself is fine: `graph.links[id]` resolves through `MapProxyHandler.get` (`src/lib/litegraph/src/MapProxyHandler.ts:48-53`), which falls through to `map.get(toLinkId(id))`.
## In-tree call sites with the same idiom
- `src/lib/litegraph/src/canvas/FloatingRenderLink.ts:187-197` (`connectToRerouteInput`) writes both fields with no prior `disconnectInput`. Its caller `LinkConnector._connectOutputToReroute` (`canvas/LinkConnector.ts:881-887`) feeds it `reroute.findTargetInputs()` results, which include inputs of **live** links through the reroute (`Reroute.ts:382`, `:396-400`) — occupied by construction. The sibling `connectToInput` (`FloatingRenderLink.ts:145-157`) *was* given a `node.disconnectInput(...)` moved above the writes on this branch, with the comment "Disconnect before re-targeting". The same fix was not applied to `connectToRerouteInput`, `connectToSubgraphOutput` (`:178-185`) or `connectToRerouteOutput` (`:200-209`).
- `src/lib/litegraph/src/LGraph.ts:2058-2059` (convert-to-subgraph, subgraph-input branch).
## Behaviour change
On `origin/main` these are plain fields (`origin/main:src/lib/litegraph/src/LLink.ts:101-111`) and every assignment always took effect.
## Docs
`docs/architecture/link-topology-store.md:91-98` states that `updateEndpoints` validates a complete batch and that an invalid move leaves everything unchanged. That contract holds **per call** but not per logical move, and the accessors expose only the single-item form.
## Coverage gap
`src/lib/litegraph/src/LLink.store.test.ts:420-444` covers the two-slot swap, but by calling `store.updateEndpoints(...)` **directly**. The field-write tests at `:446-473` and `:475-505` only cover the free-destination case. Nothing covers write-then-reject through the accessors.
## Suggested fix
Either expose an atomic `setEndpoint({ targetNodeId, targetSlot })` and route the compat setters through it, or make rejection throw rather than `console.error`. A silent partial commit is the worst of the three options.
Part of a branch-wide audit of compatibility-accessor asymmetry — see the umbrella issue. Related: #15620, #15618, #15577, #15594.
Contributor guide
Assessment
This issue has not been assessed yet.