mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-03-14 01:20:03 +00:00
fix: detect and remove duplicate links in subgraph unpacking (#9120)
## Summary Fix duplicate LLink objects created during subgraph unpacking, where output.links contains multiple link IDs for the same connection but input.link only references one, leaving orphaned links. ## Changes - **What**: Three layers of defense against duplicate links: 1. **Serialization fix** (`slotUtils.ts`): Clone `output.links` array in `outputAsSerialisable` to prevent shared-reference mutation during serialization round-trips 2. **Self-healing** (`LGraph.ts`): `_removeDuplicateLinks()` sanitizes corrupted data during `configure()`, keeping the link referenced by `input.link` and removing orphaned duplicates from `output.links` and `_links` 3. **Unpack dedup** (`LGraph.ts`): Subgraph unpacking filters `newLinks` via a `seenLinks` Set before creating connections Runtime diagnostic logging via `graph.events` (no Sentry import in litegraph): - `_dupLinkIndex` Map for O(1) duplicate detection, only allocated when enabled - `_checkDuplicateLink()` called at the 3 link-creation sites (`connectSlots`, `SubgraphInput.connect`, `SubgraphOutput.connect`) - App layer listens for `diagnostic:duplicate-link` events and forwards to Sentry with rate-limiting (1 per key per 60s) ## Review Focus - The `_removeDuplicateLinks` strategy of keeping the link referenced by `input.link` and removing others from `output.links` + `_links` - The diagnostic index lifecycle: built on enable, updated on link create/remove, cleared on disable - Sentry integration in `app.ts` using the existing `graph.events` system to avoid coupling litegraph to Sentry ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9120-fix-detect-and-remove-duplicate-links-in-subgraph-unpacking-3106d73d3650815b995ddf8f41da67ae) by [Unito](https://www.unito.io)
This commit is contained in:
@@ -534,6 +534,141 @@ describe('ensureGlobalIdUniqueness', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('_removeDuplicateLinks', () => {
|
||||
class TestNode extends LGraphNode {
|
||||
constructor(title?: string) {
|
||||
super(title ?? 'TestNode')
|
||||
this.addInput('input_0', 'number')
|
||||
this.addOutput('output_0', 'number')
|
||||
}
|
||||
}
|
||||
|
||||
function registerTestNodes() {
|
||||
LiteGraph.registerNodeType('test/DupTestNode', TestNode)
|
||||
}
|
||||
|
||||
it('removes orphaned duplicate links from _links and output.links', () => {
|
||||
registerTestNodes()
|
||||
const graph = new LGraph()
|
||||
|
||||
const source = LiteGraph.createNode('test/DupTestNode', 'Source')!
|
||||
const target = LiteGraph.createNode('test/DupTestNode', 'Target')!
|
||||
graph.add(source)
|
||||
graph.add(target)
|
||||
|
||||
source.connect(0, target, 0)
|
||||
expect(graph._links.size).toBe(1)
|
||||
|
||||
const existingLink = graph._links.values().next().value!
|
||||
for (let i = 0; i < 3; i++) {
|
||||
const dupLink = new LLink(
|
||||
++graph.state.lastLinkId,
|
||||
existingLink.type,
|
||||
existingLink.origin_id,
|
||||
existingLink.origin_slot,
|
||||
existingLink.target_id,
|
||||
existingLink.target_slot
|
||||
)
|
||||
graph._links.set(dupLink.id, dupLink)
|
||||
source.outputs[0].links!.push(dupLink.id)
|
||||
}
|
||||
|
||||
expect(graph._links.size).toBe(4)
|
||||
expect(source.outputs[0].links).toHaveLength(4)
|
||||
|
||||
graph._removeDuplicateLinks()
|
||||
|
||||
expect(graph._links.size).toBe(1)
|
||||
expect(source.outputs[0].links).toHaveLength(1)
|
||||
expect(target.inputs[0].link).toBe(source.outputs[0].links![0])
|
||||
})
|
||||
|
||||
it('keeps the link referenced by input.link', () => {
|
||||
registerTestNodes()
|
||||
const graph = new LGraph()
|
||||
|
||||
const source = LiteGraph.createNode('test/DupTestNode', 'Source')!
|
||||
const target = LiteGraph.createNode('test/DupTestNode', 'Target')!
|
||||
graph.add(source)
|
||||
graph.add(target)
|
||||
|
||||
source.connect(0, target, 0)
|
||||
const keptLinkId = target.inputs[0].link!
|
||||
|
||||
const dupLink = new LLink(
|
||||
++graph.state.lastLinkId,
|
||||
'number',
|
||||
source.id,
|
||||
0,
|
||||
target.id,
|
||||
0
|
||||
)
|
||||
graph._links.set(dupLink.id, dupLink)
|
||||
source.outputs[0].links!.push(dupLink.id)
|
||||
|
||||
graph._removeDuplicateLinks()
|
||||
|
||||
expect(graph._links.size).toBe(1)
|
||||
expect(target.inputs[0].link).toBe(keptLinkId)
|
||||
expect(graph._links.has(keptLinkId)).toBe(true)
|
||||
expect(graph._links.has(dupLink.id)).toBe(false)
|
||||
})
|
||||
|
||||
it('is a no-op when no duplicates exist', () => {
|
||||
registerTestNodes()
|
||||
const graph = new LGraph()
|
||||
|
||||
const source = LiteGraph.createNode('test/DupTestNode', 'Source')!
|
||||
const target = LiteGraph.createNode('test/DupTestNode', 'Target')!
|
||||
graph.add(source)
|
||||
graph.add(target)
|
||||
|
||||
source.connect(0, target, 0)
|
||||
const linksBefore = graph._links.size
|
||||
|
||||
graph._removeDuplicateLinks()
|
||||
|
||||
expect(graph._links.size).toBe(linksBefore)
|
||||
})
|
||||
|
||||
it('cleans up duplicate links in subgraph during configure', () => {
|
||||
const subgraphData = createTestSubgraphData()
|
||||
const rootGraph = new LGraph()
|
||||
const subgraph = rootGraph.createSubgraph(subgraphData)
|
||||
|
||||
const source = new LGraphNode('Source')
|
||||
source.addOutput('out', 'number')
|
||||
const target = new LGraphNode('Target')
|
||||
target.addInput('in', 'number')
|
||||
subgraph.add(source)
|
||||
subgraph.add(target)
|
||||
|
||||
source.connect(0, target, 0)
|
||||
expect(subgraph._links.size).toBe(1)
|
||||
|
||||
const existingLink = subgraph._links.values().next().value!
|
||||
for (let i = 0; i < 3; i++) {
|
||||
const dup = new LLink(
|
||||
++subgraph.state.lastLinkId,
|
||||
existingLink.type,
|
||||
existingLink.origin_id,
|
||||
existingLink.origin_slot,
|
||||
existingLink.target_id,
|
||||
existingLink.target_slot
|
||||
)
|
||||
subgraph._links.set(dup.id, dup)
|
||||
source.outputs[0].links!.push(dup.id)
|
||||
}
|
||||
expect(subgraph._links.size).toBe(4)
|
||||
|
||||
// Serialize and reconfigure - should clean up during configure
|
||||
const serialized = subgraph.asSerialisable()
|
||||
subgraph.configure(serialized as never)
|
||||
|
||||
expect(subgraph._links.size).toBe(1)
|
||||
})
|
||||
})
|
||||
|
||||
describe('Subgraph Unpacking', () => {
|
||||
class TestNode extends LGraphNode {
|
||||
constructor(title?: string) {
|
||||
|
||||
@@ -164,6 +164,11 @@ export class LGraph
|
||||
static STATUS_STOPPED = 1
|
||||
static STATUS_RUNNING = 2
|
||||
|
||||
/** Generates a unique string key for a link's connection tuple. */
|
||||
static _linkTupleKey(link: LLink): string {
|
||||
return `${link.origin_id}\0${link.origin_slot}\0${link.target_id}\0${link.target_slot}`
|
||||
}
|
||||
|
||||
/** List of LGraph properties that are manually handled by {@link LGraph.configure}. */
|
||||
static readonly ConfigureProperties = new Set([
|
||||
'nodes',
|
||||
@@ -1611,6 +1616,52 @@ export class LGraph
|
||||
link.disconnect(this)
|
||||
}
|
||||
|
||||
/**
|
||||
* Removes duplicate links that share the same connection tuple
|
||||
* (origin_id, origin_slot, target_id, target_slot). Keeps the link
|
||||
* referenced by input.link and removes orphaned duplicates from
|
||||
* output.links and the graph's _links map.
|
||||
*/
|
||||
_removeDuplicateLinks(): void {
|
||||
const seen = new Map<string, LinkId>()
|
||||
const toRemove: LinkId[] = []
|
||||
|
||||
for (const [id, link] of this._links) {
|
||||
const key = LGraph._linkTupleKey(link)
|
||||
if (seen.has(key)) {
|
||||
const existingId = seen.get(key)!
|
||||
// Keep the link that the input side references
|
||||
const node = this.getNodeById(link.target_id)
|
||||
const input = node?.inputs?.[link.target_slot]
|
||||
if (input?.link === id) {
|
||||
toRemove.push(existingId)
|
||||
seen.set(key, id)
|
||||
} else {
|
||||
toRemove.push(id)
|
||||
}
|
||||
} else {
|
||||
seen.set(key, id)
|
||||
}
|
||||
}
|
||||
|
||||
for (const id of toRemove) {
|
||||
const link = this._links.get(id)
|
||||
if (!link) continue
|
||||
|
||||
// Remove from origin node's output.links array
|
||||
const originNode = this.getNodeById(link.origin_id)
|
||||
if (originNode) {
|
||||
const output = originNode.outputs?.[link.origin_slot]
|
||||
if (output?.links) {
|
||||
const idx = output.links.indexOf(id)
|
||||
if (idx !== -1) output.links.splice(idx, 1)
|
||||
}
|
||||
}
|
||||
|
||||
this._links.delete(id)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a new subgraph definition, and adds it to the graph.
|
||||
* @param data Exported data (typically serialised) to configure the new subgraph with
|
||||
@@ -2072,7 +2123,7 @@ export class LGraph
|
||||
// disconnect/reconnect cycles on widget inputs that can shift slot indices.
|
||||
const seenLinks = new Set<string>()
|
||||
const dedupedNewLinks = newLinks.filter((link) => {
|
||||
const key = `${link.oid}:${link.oslot}:${link.tid}:${link.tslot}`
|
||||
const key = `${link.oid}\0${link.oslot}\0${link.tid}\0${link.tslot}`
|
||||
if (seenLinks.has(key)) return false
|
||||
seenLinks.add(key)
|
||||
return true
|
||||
@@ -2568,6 +2619,12 @@ export class LGraph
|
||||
}
|
||||
}
|
||||
|
||||
// Remove duplicate links: links in output.links that share the same
|
||||
// (origin_id, origin_slot, target_id, target_slot) tuple.
|
||||
// This repairs corrupted data where extra link objects were created
|
||||
// without proper cleanup of the previous connection.
|
||||
this._removeDuplicateLinks()
|
||||
|
||||
// groups
|
||||
this._groups.length = 0
|
||||
const groupData = data.groups
|
||||
|
||||
35
src/lib/litegraph/src/node/slotUtils.test.ts
Normal file
35
src/lib/litegraph/src/node/slotUtils.test.ts
Normal file
@@ -0,0 +1,35 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import type { INodeOutputSlot } from '@/lib/litegraph/src/interfaces'
|
||||
import type { IWidget } from '@/lib/litegraph/src/litegraph'
|
||||
import { LGraphNode } from '@/lib/litegraph/src/litegraph'
|
||||
|
||||
import { outputAsSerialisable } from './slotUtils'
|
||||
|
||||
type OutputSlotParam = INodeOutputSlot & { widget?: IWidget }
|
||||
|
||||
describe('outputAsSerialisable', () => {
|
||||
it('clones the links array to prevent shared reference mutation', () => {
|
||||
const node = new LGraphNode('test')
|
||||
const output = node.addOutput('out', 'number')
|
||||
output.links = [1, 2, 3]
|
||||
|
||||
const serialised = outputAsSerialisable(output as OutputSlotParam)
|
||||
|
||||
expect(serialised.links).toEqual([1, 2, 3])
|
||||
expect(serialised.links).not.toBe(output.links)
|
||||
|
||||
// Mutating the live array should NOT affect the serialised copy
|
||||
output.links.push(4)
|
||||
expect(serialised.links).toHaveLength(3)
|
||||
})
|
||||
|
||||
it('preserves null links', () => {
|
||||
const node = new LGraphNode('test')
|
||||
const output = node.addOutput('out', 'number')
|
||||
output.links = null
|
||||
|
||||
const serialised = outputAsSerialisable(output as OutputSlotParam)
|
||||
expect(serialised.links).toBeNull()
|
||||
})
|
||||
})
|
||||
@@ -74,7 +74,7 @@ export function outputAsSerialisable(
|
||||
...outputWidget,
|
||||
pos,
|
||||
slot_index,
|
||||
links
|
||||
links: links ? [...links] : links
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user