Comfy-Org / Comfy-Org/ComfyUI_frontend

ECS branch: rejected topology updates degrade silently at all three updateEndpoints call sites

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

Description

Found while writing the repros for #15577 and #15581 (PR #15592). Those two are instances of a class, and this is the third and worst instance. Branch `feature/ecs-migration`, head `9f6ab9adda`.

## The class

Every `updateEndpoints` call site on the branch swallows a rejected topology update into `console.error` and continues with degraded state. There are three, and each degrades differently:

| Site | On rejection | Result |
| --- | --- | --- |
| `linkDeduplication.ts:158` | logs, `break` | abandons the node's remaining moves (#15581) |
| `LLink.ts:115` | logs, falls through | caller proceeds as if the endpoint moved |
| `node/slotLinks.ts:192` | logs, `return []` | node keeps its **old** input layout; `[]` is indistinguishable from "nothing to replace" |

None marks `has_errors`. None uses the repo's unified `reportError`. Raw `console.error` is auto-captured by Datadog RUM but untagged, so a load that silently re-wires a graph produces no searchable signal.

## Why `slotLinks.ts:192` is the worst of the three

```ts
const result = store.updateEndpoints(graphScopeOf(node.graph), updates, removals.map(...))
if (!result.ok) {
console.error('Failed to replace node inputs', result.error)
return []
}
node.inputs.splice(0, node.inputs.length, ...finalInputs)
```

The early return happens **before** the splice. So on a rejected batch the node keeps its previous input layout while the caller believes `replaceNodeInputs` ran, and the empty return value is the same value a successful no-op replacement produces. There is no way for a caller to tell the two apart.

## Why this may already be a known break

`replaceNodeInputs` is the indexed-slot-replacement path. comfyui-promptchain (14,814 installs) does exactly `node.inputs[i] = {...slot}`, and it is the confirmed pack break on this branch. If promptchain's failure is a rejected `replaceNodeInputs` returning `[]`, then the shim currently being scoped for it is being designed against a symptom rather than the cause.

Worth testing that before the shim is specified.

## Asks

1. Decide what a rejected topology update should do. One decision for the class, not three patches.
2. At minimum, make `replaceNodeInputs` distinguishable: return a result type, or throw, rather than an empty array that means two different things.
3. Route these through `reportError` so they are searchable, and mark the node `has_errors` where a graph has actually been left mis-wired.

## Related

- #15577, #15581 — the other two instances, both with repros in #15592

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.