Found reviewing #14246 (feature/ecs-migration) at 333906c4cbe31d4d76dd79b7f053f1bc617dcf1a. Not reachable on main.
What changed
realignInputLinkSlots (src/lib/litegraph/src/linkDeduplication.ts:118-170) replaces fixLinkInputSlots, which the branch deletes from src/utils/litegraphUtil.ts along with its call site in ComfyApp.addAfterConfigureHandler.
Both exist to fix #3348: a node's configure may reorder its inputs into node-definition order, which moves the slot each serialized link targets.
main did this, per link, unconditionally:
const linkId = input.link
const link = graph.links.get(linkId)
link.target_slot = inputIndex
The branch cannot use input.link any more, because it is derived from the link store from target_slot, so reading it here would be circular. It matches serialized input names against live input names instead.
The failure
- A serialized input whose name has no live counterpart (the node's
configure renamed it or dropped it) yields slots.length === 0 and is skipped at linkDeduplication.ts:141. Its link keeps its serialized target_slot.
- When another link's move targets that stale slot,
validateEndpointUpdates finds an incumbent that is not a participant in the batch and returns occupied-target (src/stores/linkStore.ts:318-325).
realignInputLinkSlots logs to console.error and breaks at linkDeduplication.ts:158-160, abandoning every remaining move for that node, not just the rejected one.
Net effect: one unmatched input name can leave that node's other links wired to the wrong inputs after a workflow load. On main a partial mismatch cost exactly the mismatched link, because each assignment was independent and could not be rejected.
The only signal is console.error. The node is not marked has_errors and nothing surfaces to the user.
Why this has not been measured
The ECS compatibility battery compares workflow and prompt JSON across 1,811 packs, but only for the stock 7-node workflow with the pack loaded. It has never run a workflow containing a pack's own node types. A node whose configure rewrites its input list is precisely what that battery does not exercise, so "0 DIFF" says nothing about this path.
Coverage gap
src/lib/litegraph/src/LGraph.inputSlotRealign.test.ts covers the happy path well, but its fixture ReorderTargetNode (lines 24-37) only sorts data.inputs into definition order. No test renames an input, drops one, or drives updateEndpoints to a rejected batch.
Suggested fix
updateEndpoints already accepts a removals list. Passing the blocking incumbents there would let the batch land atomically.
Edited 2026-08-23: the per-link fallback this originally offered as a second option is struck. Mutation-tested against the repro in #15592: the removals route flips 4 of 4 assertions, the per-link fallback flips 1 of 4, and the one it flips is the console.error check rather than any wiring assertion. The collision cascades (link 3 squats slot 0, so link 1 cannot leave slot 1, so link 2 cannot enter it), so a per-link retry meets the same non-participant incumbent the batch did. Use removals. Detail: #15581 (comment)
Either way, add tests for:
- an input renamed by
configure while a sibling moves slots
- an input dropped by
configure while a sibling moves slots
- a rejected
updateEndpoints batch, asserting the surviving moves still land
Review comment
#14246 (comment)
Found reviewing #14246 (
feature/ecs-migration) at333906c4cbe31d4d76dd79b7f053f1bc617dcf1a. Not reachable onmain.What changed
realignInputLinkSlots(src/lib/litegraph/src/linkDeduplication.ts:118-170) replacesfixLinkInputSlots, which the branch deletes fromsrc/utils/litegraphUtil.tsalong with its call site inComfyApp.addAfterConfigureHandler.Both exist to fix #3348: a node's
configuremay reorder its inputs into node-definition order, which moves the slot each serialized link targets.maindid this, per link, unconditionally:The branch cannot use
input.linkany more, because it is derived from the link store fromtarget_slot, so reading it here would be circular. It matches serialized input names against live input names instead.The failure
configurerenamed it or dropped it) yieldsslots.length === 0and is skipped atlinkDeduplication.ts:141. Its link keeps its serializedtarget_slot.validateEndpointUpdatesfinds an incumbent that is not a participant in the batch and returnsoccupied-target(src/stores/linkStore.ts:318-325).realignInputLinkSlotslogs toconsole.errorandbreaks atlinkDeduplication.ts:158-160, abandoning every remaining move for that node, not just the rejected one.Net effect: one unmatched input name can leave that node's other links wired to the wrong inputs after a workflow load. On
maina partial mismatch cost exactly the mismatched link, because each assignment was independent and could not be rejected.The only signal is
console.error. The node is not markedhas_errorsand nothing surfaces to the user.Why this has not been measured
The ECS compatibility battery compares workflow and prompt JSON across 1,811 packs, but only for the stock 7-node workflow with the pack loaded. It has never run a workflow containing a pack's own node types. A node whose
configurerewrites its input list is precisely what that battery does not exercise, so "0 DIFF" says nothing about this path.Coverage gap
src/lib/litegraph/src/LGraph.inputSlotRealign.test.tscovers the happy path well, but its fixtureReorderTargetNode(lines 24-37) only sortsdata.inputsinto definition order. No test renames an input, drops one, or drivesupdateEndpointsto a rejected batch.Suggested fix
updateEndpointsalready accepts aremovalslist. Passing the blocking incumbents there would let the batch land atomically.Edited 2026-08-23: the per-link fallback this originally offered as a second option is struck. Mutation-tested against the repro in #15592: the
removalsroute flips 4 of 4 assertions, the per-link fallback flips 1 of 4, and the one it flips is theconsole.errorcheck rather than any wiring assertion. The collision cascades (link 3 squats slot 0, so link 1 cannot leave slot 1, so link 2 cannot enter it), so a per-link retry meets the same non-participant incumbent the batch did. Useremovals. Detail: #15581 (comment)Either way, add tests for:
configurewhile a sibling moves slotsconfigurewhile a sibling moves slotsupdateEndpointsbatch, asserting the surviving moves still landReview comment
#14246 (comment)