fix: configure nested subgraph definitions in dependency order (#10314)

## Summary

Fix workflow loading for nested subgraphs with duplicate node IDs by
configuring subgraph definitions in topological (leaf-first) order.

## Changes

- **What**: Three pre-existing bugs that surface when loading nested
subgraphs with colliding node IDs:
1. Subgraph definitions configured in serialization order — a parent
subgraph's `SubgraphNode.configure` would run before its referenced
child subgraph was populated, causing link/widget resolution failures.
2. `_resolveLegacyEntry` returned `undefined` when `input._widget`
wasn't set yet, instead of falling back to `resolveSubgraphInputTarget`.
3. `_removeDuplicateLinks` removed duplicate links without updating
`SubgraphOutput.linkIds`, leaving stale references that broke prompt
execution.
- **What (housekeeping)**: Moved `subgraphDeduplication.ts` from
`utils/` to `subgraph/` directory where it belongs.

## Review Focus

- Topological sort correctness: Kahn's algorithm with edges from
dependency→dependent ensures leaves configure first. Cycle fallback
returns original order.
- IO slot link repair: `_repairIOSlotLinkIds` runs after
`_configureSubgraph` creates IO slots, patching any `linkIds` that point
to links removed by `_removeDuplicateLinks` during `super.configure()`.

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-10314-fix-configure-nested-subgraph-definitions-in-dependency-order-3286d73d36508171b149e238b8de84c2)
by [Unito](https://www.unito.io)

---------

Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
Alexander Brown
2026-03-19 15:36:26 -07:00
committed by GitHub
parent 4eded7c82b
commit 28a91fa8b6
5 changed files with 307 additions and 3 deletions

View File

@@ -0,0 +1,110 @@
import { expect } from '@playwright/test'
import { comfyPageFixture as test } from '../fixtures/ComfyPage'
test.describe('Nested subgraph configure order', { tag: ['@subgraph'] }, () => {
const WORKFLOW = 'subgraphs/subgraph-nested-duplicate-ids'
test('Loads without "No link found" or "Failed to resolve legacy -1" console warnings', async ({
comfyPage
}) => {
const warnings: string[] = []
comfyPage.page.on('console', (msg) => {
const text = msg.text()
if (
text.includes('No link found') ||
text.includes('Failed to resolve legacy -1') ||
text.includes('No inner link found')
) {
warnings.push(text)
}
})
await comfyPage.workflow.loadWorkflow(WORKFLOW)
expect(warnings).toEqual([])
})
test('All three subgraph levels resolve promoted widgets', async ({
comfyPage
}) => {
await comfyPage.workflow.loadWorkflow(WORKFLOW)
await comfyPage.nextFrame()
const results = await comfyPage.page.evaluate(() => {
const graph = window.app!.canvas.graph!
const allGraphs = [graph, ...graph.subgraphs.values()]
return allGraphs.flatMap((g) =>
g._nodes
.filter(
(n) => typeof n.isSubgraphNode === 'function' && n.isSubgraphNode()
)
.map((hostNode) => {
const proxyWidgets = Array.isArray(
hostNode.properties?.proxyWidgets
)
? hostNode.properties.proxyWidgets
: []
const widgetEntries = proxyWidgets
.filter(
(e: unknown): e is [string, string] =>
Array.isArray(e) &&
e.length >= 2 &&
typeof e[0] === 'string' &&
typeof e[1] === 'string'
)
.map(([interiorNodeId, widgetName]: [string, string]) => {
const sg = hostNode.isSubgraphNode() ? hostNode.subgraph : null
const interiorNode = sg?.getNodeById(Number(interiorNodeId))
return {
interiorNodeId,
widgetName,
resolved: interiorNode !== null && interiorNode !== undefined
}
})
return {
hostNodeId: String(hostNode.id),
widgetEntries
}
})
)
})
expect(
results.length,
'Should have subgraph host nodes at multiple nesting levels'
).toBeGreaterThanOrEqual(2)
for (const { hostNodeId, widgetEntries } of results) {
expect(
widgetEntries.length,
`Host node ${hostNodeId} should have promoted widgets`
).toBeGreaterThan(0)
for (const { interiorNodeId, widgetName, resolved } of widgetEntries) {
expect(interiorNodeId).not.toBe('-1')
expect(Number(interiorNodeId)).toBeGreaterThan(0)
expect(widgetName).toBeTruthy()
expect(
resolved,
`Widget "${widgetName}" (interior node ${interiorNodeId}) on host ${hostNodeId} should resolve`
).toBe(true)
}
}
})
test('Prompt execution succeeds without 400 error', async ({ comfyPage }) => {
await comfyPage.workflow.loadWorkflow(WORKFLOW)
await comfyPage.nextFrame()
const responsePromise = comfyPage.page.waitForResponse('**/api/prompt')
await comfyPage.command.executeCommand('Comfy.QueuePrompt')
const response = await responsePromise
expect(response.status()).not.toBe(400)
})
})

View File

@@ -78,7 +78,10 @@ import type {
SerialisableReroute
} from './types/serialisation'
import { getAllNestedItems } from './utils/collections'
import { deduplicateSubgraphNodeIds } from './utils/subgraphDeduplication'
import {
deduplicateSubgraphNodeIds,
topologicalSortSubgraphs
} from './subgraph/subgraphDeduplication'
export type {
LGraphTriggerAction,
@@ -2585,7 +2588,12 @@ export class LGraph
effectiveNodesData = deduplicated?.rootNodes ?? nodesData
for (const subgraph of finalSubgraphs) this.createSubgraph(subgraph)
for (const subgraph of finalSubgraphs)
// Configure in leaf-first order so that when a SubgraphNode is
// configured, its referenced subgraph definition already has its
// nodes/links/inputs populated.
const configureOrder = topologicalSortSubgraphs(finalSubgraphs)
for (const subgraph of configureOrder)
this.subgraphs.get(subgraph.id)?.configure(subgraph)
}
@@ -2878,6 +2886,10 @@ export class Subgraph
}
}
// Repair IO slot linkIds that reference links removed by
// _removeDuplicateLinks during super.configure().
this._repairIOSlotLinkIds()
if (widgets) {
this.widgets.length = 0
for (const widget of widgets) {
@@ -2902,6 +2914,50 @@ export class Subgraph
return r
}
/**
* Repairs SubgraphInput/Output `linkIds` that reference links removed
* by `_removeDuplicateLinks` during `super.configure()`.
*
* For each stale link ID, finds the surviving link that connects to the
* same IO node and slot index, and substitutes it.
*/
private _repairIOSlotLinkIds(): void {
for (const [slotIndex, slot] of this.inputs.entries()) {
this._repairSlotLinkIds(slot.linkIds, SUBGRAPH_INPUT_ID, slotIndex)
}
for (const [slotIndex, slot] of this.outputs.entries()) {
this._repairSlotLinkIds(slot.linkIds, SUBGRAPH_OUTPUT_ID, slotIndex)
}
}
private _repairSlotLinkIds(
linkIds: LinkId[],
ioNodeId: number,
slotIndex: number
): void {
const repaired = linkIds.map((id) =>
this._links.has(id)
? id
: (this._findLinkBySlot(ioNodeId, slotIndex)?.id ?? id)
)
repaired.forEach((id, i) => {
linkIds[i] = id
})
}
private _findLinkBySlot(
nodeId: number,
slotIndex: number
): LLink | undefined {
for (const link of this._links.values()) {
if (
(link.origin_id === nodeId && link.origin_slot === slotIndex) ||
(link.target_id === nodeId && link.target_slot === slotIndex)
)
return link
}
}
override attachCanvas(canvas: LGraphCanvas): void {
super.attachCanvas(canvas)
canvas.subgraph = this

View File

@@ -566,7 +566,12 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
// Legacy -1 entries use the slot name as the widget name.
// Find the input with that name, then trace to the connected interior widget.
const input = this.inputs.find((i) => i.name === widgetName)
if (!input?._widget) return undefined
if (!input?._widget) {
// Fallback: find via subgraph input slot connection
const resolvedTarget = resolveSubgraphInputTarget(this, widgetName)
if (!resolvedTarget) return undefined
return [resolvedTarget.nodeId, resolvedTarget.widgetName]
}
const widget = input._widget
if (isPromotedWidgetView(widget)) {

View File

@@ -0,0 +1,76 @@
import { describe, expect, it } from 'vitest'
import type { ExportedSubgraph } from '../types/serialisation'
import { topologicalSortSubgraphs } from './subgraphDeduplication'
function makeSubgraph(id: string, nodeTypes: string[] = []): ExportedSubgraph {
return {
id,
name: id,
version: 1,
revision: 0,
state: { lastNodeId: 0, lastLinkId: 0, lastGroupId: 0, lastRerouteId: 0 },
nodes: nodeTypes.map((type, i) => ({
id: i + 1,
type,
pos: [0, 0] as [number, number],
size: [100, 100] as [number, number],
flags: {},
order: i,
mode: 0,
inputs: [],
outputs: [],
properties: {}
})),
inputNode: { id: -10, bounding: [0, 0, 100, 100] },
outputNode: { id: -20, bounding: [0, 0, 100, 100] }
} as ExportedSubgraph
}
describe('topologicalSortSubgraphs', () => {
it('returns original order when there are no dependencies', () => {
const a = makeSubgraph('a')
const b = makeSubgraph('b')
const result = topologicalSortSubgraphs([a, b])
expect(result).toEqual([a, b])
})
it('sorts leaf dependencies before their parents', () => {
const inner = makeSubgraph('inner', ['StringConcat'])
const outer = makeSubgraph('outer', ['inner'])
const result = topologicalSortSubgraphs([outer, inner])
expect(result.map((s) => s.id)).toEqual(['inner', 'outer'])
})
it('handles three-level nesting', () => {
const leaf = makeSubgraph('leaf', ['StringConcat'])
const mid = makeSubgraph('mid', ['leaf', 'StringConcat'])
const top = makeSubgraph('top', ['mid'])
const result = topologicalSortSubgraphs([top, mid, leaf])
expect(result.map((s) => s.id)).toEqual(['leaf', 'mid', 'top'])
})
it('handles diamond dependencies', () => {
const shared = makeSubgraph('shared')
const left = makeSubgraph('left', ['shared'])
const right = makeSubgraph('right', ['shared'])
const top = makeSubgraph('top', ['left', 'right'])
const result = topologicalSortSubgraphs([top, left, right, shared])
const ids = result.map((s) => s.id)
expect(ids.indexOf('shared')).toBeLessThan(ids.indexOf('left'))
expect(ids.indexOf('shared')).toBeLessThan(ids.indexOf('right'))
expect(ids.indexOf('left')).toBeLessThan(ids.indexOf('top'))
expect(ids.indexOf('right')).toBeLessThan(ids.indexOf('top'))
})
it('returns original order for a single subgraph', () => {
const only = makeSubgraph('only')
const result = topologicalSortSubgraphs([only])
expect(result).toEqual([only])
})
it('returns original order for empty array', () => {
expect(topologicalSortSubgraphs([])).toEqual([])
})
})

View File

@@ -140,6 +140,63 @@ function patchPromotedWidgets(
}
}
/**
* Topologically sorts subgraph definitions so that leaf subgraphs (those
* that no other subgraph depends on) are configured first. This ensures
* that when a SubgraphNode is configured, the subgraph definition it
* references already has its nodes, links, and inputs populated.
*
* Falls back to the original order if no reordering is needed or if the
* dependency graph contains cycles.
*/
export function topologicalSortSubgraphs(
subgraphs: ExportedSubgraph[]
): ExportedSubgraph[] {
const subgraphIds = new Set(subgraphs.map((sg) => sg.id))
const byId = new Map(subgraphs.map((sg) => [sg.id, sg]))
// Build adjacency: dependency → set of dependents (parents that use it).
// Edges go from leaf to parent so Kahn's emits leaves first.
const dependents = new Map<string, Set<string>>()
const inDegree = new Map<string, number>()
for (const id of subgraphIds) {
dependents.set(id, new Set())
inDegree.set(id, 0)
}
for (const sg of subgraphs) {
for (const node of sg.nodes ?? []) {
if (subgraphIds.has(node.type)) {
// sg depends on node.type → edge from node.type to sg.id
dependents.get(node.type)!.add(sg.id)
inDegree.set(sg.id, (inDegree.get(sg.id) ?? 0) + 1)
}
}
}
// Kahn's algorithm — leaves (in-degree 0) are emitted first.
const queue: string[] = []
for (const [id, degree] of inDegree) {
if (degree === 0) queue.push(id)
}
const sorted: ExportedSubgraph[] = []
while (queue.length > 0) {
const id = queue.shift()!
sorted.push(byId.get(id)!)
for (const dependent of dependents.get(id) ?? []) {
const newDegree = (inDegree.get(dependent) ?? 1) - 1
inDegree.set(dependent, newDegree)
if (newDegree === 0) queue.push(dependent)
}
}
// Cycle fallback: return original order
if (sorted.length !== subgraphs.length) return subgraphs
return sorted
}
/** Patches proxyWidgets in root-level SubgraphNode instances. */
function patchProxyWidgets(
rootNodes: ISerialisedNode[],