Comfy-Org / Comfy-Org/ComfyUI_frontend

ECS: cross-root registerLinkTopology mutates a placed topology's graphId; clearOwner then strands stale mirror indexes (true mirror/record mismatch)

Open
#15,742 0 comments 0 reactions 1 assignee Claimed by @DrJKL View on GitHub
Potential Bug Public API
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

`registerLinkTopology` accepts a link that is still registered in a different root, and `linkStore.replaceLink` then mutates the placed topology's `graphId` in place while the first root's indexes still hold it under the old keys. A subsequent `clearOwner` (run by non-root `configure` without `keep_old`, LGraph.ts:2612) computes displacement keys from the mutated `graphId`, misses the stale `targetIndex`/`originIndex` entries, and deletes the id from `byId` unconditionally. Result: `input.link` returns the id while `graph.links.get(id)` is `undefined`. That is the exact mirror/record mismatch class link_fixer repairs on main, reproduced on the branch at f1bfb313d6.

## Mechanism

1. `registerLinkTopology` (src/lib/litegraph/src/LLink.ts:638) never checks `link._graphScope`. Re-registering a still-registered link into a different root finds no incumbent in that root's bucket and proceeds.
2. `replaceLink` (src/stores/linkStore.ts:230) runs `Object.assign(replacement, { graphId: scope.owningGraphId })`. This is the one direct topology mutation outside `updateEndpoints`, and it mutates the same object root A's bucket still indexes under A's graphId keys.
3. `clearOwner` (src/stores/linkStore.ts:430) iterates `idsByOwner.get(owner)` and calls `displace`, which computes `targetKey`/`originKey` from the topology's current (mutated) `graphId`. Both index removals miss. `byId.delete(id)` succeeds. `idsByOwner` removal also misses (keyed by mutated graphId).
4. Root A is left with: `byId` without the id, `targetIndex`/`originIndex`/`idsByOwner` still holding it. Mirror reads (`getInputSlotLink`, `getOutputSlotLinks`) answer with the stale topology; record reads (`graphTopologies`) filter it out.

## Reproduction

Deterministic vitest repro: `linkMirrorRecordMismatch.probe.test.ts`, test "R14: mirror and record agree after cross-root re-registration + clearOwner" (written as `it.fails` asserting the wanted invariant, so it flips red when a fix lands). The same file verifies all 11 legacy mirror-write and record-write idioms stay consistent, so this crack is the only representable route found.

```
Tests 11 passed | 2 expected fail (13)
```

Requirements for the corruption: a second root LGraph (production creates exactly one, app.ts:938, so this needs an extension-built scratch graph, the legacy GroupNode idiom), a subgraph reconfigure afterward, and another owner's link keeping the root bucket alive (an emptied byId drops the bucket and erases the staleness).

## Suggested fix directions

Either refuse the steal (guard in `registerLinkTopology`: reject when `link._graphScope` names a different root) or make displacement complete (displace by placement keys rather than current-field keys). The repro's assertions pass under both.

## Severity context

This settles the open question behind #15620: no legacy mirror-write idiom (the ones extensions and link_fixer use) can produce the mismatch. The route requires the extension scratch-graph pattern plus a subgraph reconfigure, so the class is real but narrow.

Found during ECS migration review (scv-04). Verified at f1bfb313d6ad1fec8ba0cbbf54070ef901688ccd.

---

## Appendix: the repro test file (appended 2026-08-23)

`linkMirrorRecordMismatch.probe.test.ts` referenced above is not in the repo tree - it was written during the review and lives in the review workspace. Full file below so the repro is runnable by anyone: drop it at `src/lib/litegraph/src/linkMirrorRecordMismatch.probe.test.ts` on `feature/ecs-migration` (verified at f1bfb313d6) and run `vitest run src/lib/litegraph/src/linkMirrorRecordMismatch.probe.test.ts`. Expected: `11 passed | 2 expected fail (13)`; R14 is the failure documented here.

linkMirrorRecordMismatch.probe.test.ts (336 lines)

```ts
import { createTestingPinia } from '@pinia/testing'
import { setActivePinia } from 'pinia'
import { beforeEach, describe, expect, it, vi } from 'vitest'

import { LGraph, LGraphNode, LLink } from '@/lib/litegraph/src/litegraph'
import { useLinkStore } from '@/stores/linkStore'
import { graphScopeOf } from '@/types/graphScopeId'
import { toLinkId } from '@/types/linkId'

import { registerLinkTopology } from './LLink'
import { createTestSubgraph } from './subgraph/__fixtures__/subgraphHelpers'

/**
* scv-04 probe: is a mirror/record mismatch representable at all on the
* ECS branch?
*
* "Mirror" = the legacy accessors `input.link` / `output.links`, now derived
* from linkStore.targetIndex / originIndex.
* "Record" = `graph.links` (LinkMap proxy), derived from
* linkStore.idsByOwner + byId.
*
* A mismatch is the state link_fixer repairs on main: a slot claims a link id
* that the graph's link record does not contain (or vice versa). On main the
* two are independently stored, so any missed dual-write diverges them. On
* the branch both derive from one store, so divergence requires the store's
* own indexes (byId/idsByOwner vs targetIndex/originIndex) to disagree.
*
* Verdict encoded by these tests:
* - Every legacy mirror-write and record-write idiom stays consistent.
* - Endpoint moves are atomic (displace + placeValidated under one key
* recompute).
* - The ONE direct topology mutation outside updateEndpoints —
* `Object.assign(replacement, { graphId })` in linkStore.replaceLink —
* IS reachable: registerLinkTopology (LLink.ts:638) never checks
* link._graphScope, so re-registering a still-registered link into a
* DIFFERENT root succeeds and mutates state.graphId while the first
* root's indexes still hold the topology under the old graphId keys.
* clearOwner (production caller: subgraph configure without keep_old,
* LGraph.ts:2612) then computes displacement keys from the mutated
* graphId, misses, and leaves stale targetIndex/originIndex entries
* while byId drops the id: a true mirror/record mismatch.
*/

class SourceNode extends LGraphNode {
constructor() {
super('Source')
this.addOutput('out', 'number')
}
}

class TargetNode extends LGraphNode {
constructor() {
super('Target')
this.addInput('in_a', 'number')
this.addInput('in_b', 'number')
}
}

function connectPair(graph: LGraph) {
const source = new SourceNode()
const target = new TargetNode()
graph.add(source)
graph.add(target)
const link = source.connect(0, target, 0)
if (!link) throw new Error('test setup: connect failed')
return { source, target, link }
}

/** Mirror and record agree on the wiring of `target` input 0. */
function expectConsistent(
graph: LGraph,
target: LGraphNode,
source: LGraphNode
) {
const mirrorId = target.inputs[0].link
if (mirrorId === null) {
for (const topology of graph.links.values()) {
expect(topology.target_id).not.toBe(target.id)
}
expect(source.outputs[0].links ?? []).toHaveLength(0)
return
}
const record = graph.links.get(mirrorId)
expect(record).toBeDefined()
expect(record!.target_id).toBe(target.id)
expect(record!.target_slot).toBe(0)
expect(source.outputs[0].links).toContain(mirrorId)
}

describe('scv-04: mirror/record mismatch representability', () => {
beforeEach(() => {
setActivePinia(createTestingPinia({ stubActions: false }))
vi.spyOn(console, 'warn').mockImplementation(() => {})
})

describe('legacy mirror-write idioms stay consistent', () => {
it('R1: input.link = is discarded', () => {
const graph = new LGraph()
const { source, target, link } = connectPair(graph)

target.inputs[0].link = toLinkId(999)

expect(target.inputs[0].link).toBe(link.id)
expectConsistent(graph, target, source)
})

it('R2: input.link = null disconnects both views', () => {
const graph = new LGraph()
// Keepalive link: an emptied byId drops the whole root bucket, which
// would mask a stale index entry.
connectPair(graph)
const { source, target, link } = connectPair(graph)

target.inputs[0].link = null

expect(target.inputs[0].link).toBeNull()
expect(graph.links.get(link.id)).toBeUndefined()
expectConsistent(graph, target, source)
})

it('R3: output.links.push() is discarded', () => {
const graph = new LGraph()
const { source, target, link } = connectPair(graph)

source.outputs[0].links?.push(toLinkId(999))

expect(source.outputs[0].links).toEqual([link.id])
expectConsistent(graph, target, source)
})

it('R4: output.links = [] disconnects both views', () => {
const graph = new LGraph()
// Keepalive link: an emptied byId drops the whole root bucket, which
// would mask a stale index entry.
connectPair(graph)
const { source, target, link } = connectPair(graph)

source.outputs[0].links = []

expect(target.inputs[0].link).toBeNull()
expect(graph.links.get(link.id)).toBeUndefined()
expectConsistent(graph, target, source)
})
})

describe('record-write idioms stay consistent', () => {
it('R5: graph.links.delete removes mirror and record together', () => {
const graph = new LGraph()
// Keepalive link: an emptied byId drops the whole root bucket, which
// would mask a stale index entry.
connectPair(graph)
const { source, target, link } = connectPair(graph)

graph.links.delete(link.id)

expect(graph.links.get(link.id)).toBeUndefined()
expect(target.inputs[0].link).toBeNull()
expectConsistent(graph, target, source)
})

it('R6: graph.links.set registers an unregistered link in both views', () => {
const graph = new LGraph()
const { source, target } = connectPair(graph)
const fresh = new LLink(
toLinkId(50),
'number',
source.id,
0,
target.id,
1
)

graph.links.set(toLinkId(50), fresh)

expect(graph.links.get(toLinkId(50))).toBeDefined()
expect(target.inputs[1].link).toBe(toLinkId(50))
})

it('R7: graph.links.set under a mismatched id is refused in both views', () => {
const graph = new LGraph()
const { source, target } = connectPair(graph)
const fresh = new LLink(
toLinkId(50),
'number',
source.id,
0,
target.id,
1
)
const error = vi.spyOn(console, 'error').mockImplementation(() => {})

graph.links.set(toLinkId(51), fresh)

expect(error).toHaveBeenCalled()
expect(graph.links.get(toLinkId(51))).toBeUndefined()
expect(target.inputs[1].link).toBeNull()
})

it('R11: delete graph.links[id] removes mirror and record together', () => {
const graph = new LGraph()
// Keepalive link: an emptied byId drops the whole root bucket, which
// would mask a stale index entry.
connectPair(graph)
const { source, target, link } = connectPair(graph)

delete (graph.links as Record)[Number(link.id)]

expect(graph.links.get(link.id)).toBeUndefined()
expect(target.inputs[0].link).toBeNull()
expectConsistent(graph, target, source)
})

it('R12: graph.links.clear empties mirror and record together', () => {
const graph = new LGraph()
// Keepalive link owned by a subgraph: clear() removes every root-owned
// link, and an emptied byId drops the whole root bucket, which would
// mask a stale index entry.
connectPair(createTestSubgraph({ rootGraph: graph }))
const { source, target } = connectPair(graph)

graph.links.clear()

expect(graph.links.size).toBe(0)
expect(target.inputs[0].link).toBeNull()
expect(source.outputs[0].links ?? []).toHaveLength(0)
})
})

describe('endpoint setters move both views atomically', () => {
it('R9: target_slot = moves mirror and record together', () => {
const graph = new LGraph()
const { target, link } = connectPair(graph)

link.target_slot = 1

expect(target.inputs[0].link).toBeNull()
expect(target.inputs[1].link).toBe(link.id)
expect(graph.links.get(link.id)?.target_slot).toBe(1)
})

it('R8: target_slot = is rejected, both views unchanged', () => {
const graph = new LGraph()
const { source, target, link } = connectPair(graph)
const second = new LLink(
toLinkId(50),
'number',
source.id,
0,
target.id,
1
)
graph.links.set(toLinkId(50), second)
const error = vi.spyOn(console, 'error').mockImplementation(() => {})

link.target_slot = 1

expect(error).toHaveBeenCalled()
expect(target.inputs[0].link).toBe(link.id)
expect(target.inputs[1].link).toBe(toLinkId(50))
expect(graph.links.get(link.id)?.target_slot).toBe(0)
})
})

describe('the crack: cross-root re-registration mutates a placed topology', () => {
// CURRENT (defective) mechanism, verified 2026-08-23 at f1bfb313d6:
// registerLinkTopology (LLink.ts:638) never checks link._graphScope, and
// root B's bucket has no incumbent, so replaceLink accepts the very
// topology object still indexed in root A's bucket and mutates its
// graphId in place (linkStore.replaceLink's Object.assign). clearOwner
// then computes displacement keys from the MUTATED graphId, misses A's
// targetIndex/originIndex entries, but deletes the id from byId
// unconditionally — leaving the mirror claiming a link the record no
// longer contains. When this goes red, the guard (or key-complete
// displacement) has landed: flip it to a plain `it`.
it.fails('R14: mirror and record agree after cross-root re-registration + clearOwner', () => {
// Root A with one healthy root-owned link (keeps A's bucket alive
// through clearOwner) and a subgraph owning the link at risk.
const rootA = new LGraph()
connectPair(rootA)
const subgraph = createTestSubgraph({ rootGraph: rootA })
const {
source: subSource,
target: subTarget,
link: subLink
} = connectPair(subgraph)

// A second, unrelated root graph.
const rootB = new LGraph()

// Sanity: mirror and record agree inside the subgraph. Holds today
// and under any fix.
expect(subTarget.inputs[0].link).toBe(subLink.id)
expect(subgraph.links.get(subLink.id)).toBeDefined()

// The steal attempt. Deliberately NOT asserting its return value:
// a fix may either refuse it (guard) or absorb it (complete
// displacement) — both restore the invariant below.
registerLinkTopology(rootB, subLink)

// clearOwner as run by subgraph.configure without keep_old
// (LGraph.ts:2612).
useLinkStore().clearOwner(graphScopeOf(subgraph))

// WANTED invariant: whatever the mirrors claim, the record backs.
const mirrorId = subTarget.inputs[0].link
if (mirrorId !== null) {
expect(subgraph.links.get(mirrorId)).toBeDefined()
}
for (const id of subSource.outputs[0].links ?? []) {
expect(subgraph.links.get(id)).toBeDefined()
}
})

// CURRENT mechanism: the raw write lands in the record (byId topology)
// immediately, but targetIndex still answers for the OLD slot — record
// says slot 1, mirror claims slot 0. Not a legacy idiom (no first-party
// code writes _state fields directly), but the field is reachable JS.
// The indexes stay coherent only because updateEndpoints recomputes keys.
it.fails('R15: direct _state endpoint writes keep mirror and record consistent (internal surface)', () => {
const graph = new LGraph()
const { target, link } = connectPair(graph)

try {
link._state.targetSlot = 1
} catch {
// A fix that freezes registered topologies restores the invariant.
return
}

// WANTED invariant: mirror agrees with whatever the record says.
const recordSlot = graph.links.get(link.id)?.target_slot
expect(recordSlot).toBeDefined()
expect(target.inputs[recordSlot!].link).toBe(link.id)
})
})
})
```

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.