Skip to content

Fix zombie linkIds on node deletion, add safety check #7153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AustinMroz merged 3 commits into from
Dec 4, 2025
Merged

Fix zombie linkIds on node deletion, add safety check #7153

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d43775
Fix missed link prune, add safety check
AustinMroz Dec 4, 2025
88af3a1
Add extra check that target slot exists
AustinMroz Dec 4, 2025
1fd0c17
Apply suggestion from @DrJKL
DrJKL Dec 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/lib/litegraph/src/LGraphNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { LayoutSource } from '@/renderer/core/layout/types'
import { adjustColor } from '@/utils/colorUtil'
import type { ColorAdjustOptions } from '@/utils/colorUtil'

import { SUBGRAPH_OUTPUT_ID } from '@/lib/litegraph/src/constants'
import type { DragAndScale } from './DragAndScale'
import type { LGraph } from './LGraph'
import { BadgePosition, LGraphBadge } from './LGraphBadge'
Expand Down Expand Up @@ -45,8 +46,8 @@ import type {
Rect,
Size
} from './interfaces'
import { LiteGraph } from './litegraph'
import type { LGraphNodeConstructor, Subgraph, SubgraphNode } from './litegraph'
import { LiteGraph, Subgraph } from './litegraph'
import type { LGraphNodeConstructor, SubgraphNode } from './litegraph'
import {
createBounds,
isInRect,
Expand Down Expand Up @@ -3062,6 +3063,11 @@ export class LGraphNode
for (const link_id of links) {
const link_info = graph._links.get(link_id)
if (!link_info) continue
if (
link_info.target_id === SUBGRAPH_OUTPUT_ID &&
graph instanceof Subgraph
)
graph.outputNode.slots[link_info.target_slot].linkIds.length = 0
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add defensive checks to prevent potential runtime errors.

The logic correctly clears stale linkIds for subgraph outputs, but lacks validation that could cause crashes in edge cases.

Apply this diff to add defensive checks:

-if (
-  link_info.target_id === SUBGRAPH_OUTPUT_ID &&
-  graph instanceof Subgraph
-)
-  graph.outputNode.slots[link_info.target_slot].linkIds.length = 0
+if (
+  link_info.target_id === SUBGRAPH_OUTPUT_ID &&
+  graph instanceof Subgraph
+) {
+  const slot = graph.outputNode?.slots?.[link_info.target_slot]
+  if (slot?.linkIds) {
+    slot.linkIds.length = 0
+  }
+}

The optional chaining prevents crashes if:

  • outputNode is undefined
  • slots array is undefined
  • target_slot index is out of bounds
  • linkIds array doesn't exist
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (
link_info.target_id === SUBGRAPH_OUTPUT_ID &&
graph instanceof Subgraph
)
graph.outputNode.slots[link_info.target_slot].linkIds.length = 0
if (
link_info.target_id === SUBGRAPH_OUTPUT_ID &&
graph instanceof Subgraph
) {
const slot = graph.outputNode?.slots?.[link_info.target_slot]
if (slot?.linkIds) {
slot.linkIds.length = 0
}
}
🤖 Prompt for AI Agents 
In src/lib/litegraph/src/LGraphNode.ts around lines 3066-3070, the code clears
linkIds on a subgraph output without validating that graph.outputNode, its slots
array, the requested target_slot index, and the linkIds array exist; update the
logic to guard each access (use optional chaining / index bounds checks) so you
only set linkIds.length = 0 when graph.outputNode exists, graph.outputNode.slots
is an array, the target_slot index is within bounds, and the specific
slot.linkIds array exists — skip or no-op otherwise to avoid runtime exceptions.

Copy link
Contributor

@DrJKL DrJKL Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how often we accidentally try to dereference undefined values, I agree with the robot.

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DrJKL, absolutely, defensive checks are essential here. Here's the improved code with proper guards:

if (
  link_info.target_id === SUBGRAPH_OUTPUT_ID &&
  graph instanceof Subgraph
) {
  const slot = graph.outputNode?.slots?.[link_info.target_slot]
  if (slot?.linkIds) {
    slot.linkIds.length = 0
  }
}

This defensive approach protects against:

  • outputNode being undefined or null
  • slots array not existing
  • target_slot index being out of bounds
  • linkIds array not being initialized

The optional chaining (?.) will short-circuit the evaluation if any property in the chain is nullish, preventing the runtime errors that could occur with direct property access. This is especially important in this cleanup code path where we're dealing with potentially stale or partially-cleaned-up state.

🧠 Learnings used 
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setup

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : When writing tests for subgraph-related code, always import from the barrel export at `@/lib/litegraph/src/litegraph` to avoid circular dependency issues

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{js,ts,jsx,tsx} : Do not replace `&&=` or `||=` with `=` when there is no reason to do so. If you do find a reason to remove either `&&=` or `||=`, leave a comment explaining why the removal occurred

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{js,ts,jsx,tsx} : Take advantage of `TypedArray` `subarray` when appropriate

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{ts,tsx} : Type assertions are an absolute last resort. In almost all cases, they are a crutch that leads to brittle code

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{js,ts,jsx,tsx} : When writing methods, prefer returning idiomatic JavaScript `undefined` over `null`

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{js,ts,jsx,tsx} : Prefer single line `if` syntax over adding curly braces, when the statement has a very concise expression and concise, single line statement

Copy link
Contributor

@DrJKL DrJKL Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai

  1. I wasn't talking to you.
  2. Rabbit Season

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DrJKL, fair enough! 🦆

Hope the defensive checks help prevent those undefined dereference issues. Let me know if you need anything else!

🧠 Learnings used 
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setup

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : When writing tests for subgraph-related code, always import from the barrel export at `@/lib/litegraph/src/litegraph` to avoid circular dependency issues

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{js,ts,jsx,tsx} : Take advantage of `TypedArray` `subarray` when appropriate

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{js,ts,jsx,tsx} : Do not replace `&&=` or `||=` with `=` when there is no reason to do so. If you do find a reason to remove either `&&=` or `||=`, leave a comment explaining why the removal occurred

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{ts,tsx} : Type assertions are an absolute last resort. In almost all cases, they are a crutch that leads to brittle code

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{js,ts,jsx,tsx} : Prefer single line `if` syntax over adding curly braces, when the statement has a very concise expression and concise, single line statement

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{js,ts,jsx,tsx} : When adding features, always write vitest unit tests using cursor rules in @.cursor

Copy link
Collaborator Author

@AustinMroz AustinMroz Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a check specifically for the array indexing. I note that the actual bug here was only found because there was a single code section that doesn't silently ignore broken state.
Blindly adding nullchecks for properties that are defined to never be null is actively detrimental to the health of the codebase. It allows that broken state to bleed into other areas and makes it harder to determine the root cause.

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor

@DrJKL DrJKL Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we didn't have so many as any and // @ts-expect-errors (and maybe more immutable objects...), I'd be much more confident about their non-nullish nature.


const target = graph.getNodeById(link_info.target_id)
graph._version++
Expand Down
1 change: 1 addition & 0 deletions src/lib/litegraph/src/subgraph/SubgraphOutput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ export class SubgraphOutput extends SubgraphSlot {
//should never have more than one connection
for (const linkId of this.linkIds) {
const link = subgraph.links[linkId]
if (!link) continue
subgraph.removeLink(linkId)
const { output, outputNode } = link.resolve(subgraph)
if (output)
Expand Down