Skip to content

spec: #992 is not a connect bug — four mechanisms eliminated, two now stale - #998

Draft
artokun wants to merge 3 commits into
mainfrom
fix/992-dropped-link
Draft

spec: #992 is not a connect bug — four mechanisms eliminated, two now stale#998
artokun wants to merge 3 commits into
mainfrom
fix/992-dropped-link

Conversation

@artokun

@artokun artokun commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Investigation of #992. Ships a document, not a fix — deliberately.

Refs #992 — still OPEN, blocked on the reporter since 2026-08-10.

Until now this branch was a single empty commit and the whole investigation lived in this
PR description, so nothing in the repo recorded it and merging would have changed nothing. It
now adds docs/design/992-connect-link-drop-eliminations.md, which labels every claim by
how it was established — measured on a rig, or reasoned from source.

Reported

panel_connect returned success for a link that was gone by queue time
(Required input is missing (latent_image)), on a VAEEncode.LATENT fan-out to three
KSamplers. The first of the three vanished. Reporter on panel 0.11.43,
ComfyUI 0.31.1, frontend 1.48.7.

Measured on a live canvas (the reporter's ComfyUI/frontend build)

Against the panel at the branch point (0.11.90), on 2026-08-10:

# mechanism links after
1 the reported sequence: 3 connects, then 4 set_widget on KSampler #1 3/3 intact
2 reapplyDefsToLiveNodes 0 changed, 3/3 intact
3 refreshComboOptionsFromDefs 0 changed, 3/3 intact
4 app.registerNodesFromDefs (what panel_refresh_nodes drives) 3/3 intact

Sizes stayed 270x262 throughout. The reporter's affected node went 270x262 → 392x286 → 392x262. None of the four resizes a node, so none reproduces the only fingerprint.

Re-checked 2026-08-14 — two eliminations went stale

main moved 189 commits touching web/ since the branch point.

  • Elimination 2 is STALE. 67e05b53 (fix(1172), landed 2026-08-14) added a
    refreshComboOptionsFromDefs call inside reapplyDefsToLiveNodes; the commit's own
    comment says the sweep previously "never touched options.values". The function that was
    measured is not the one that ships. The conclusion probably survives — combo rebuilding
    writes w.options.values, never node.inputs or graph.links — but that is now
    reasoning that replaced a measurement.
  • Elimination 4 is transitively affectedpanel_refresh_nodes runs
    registerNodesFromDefs and the changed sweep in one pass.
  • Elimination 3 holds — the signature gained a mergedInputs per-type cache (The refresh run budget is thin: refreshComboInNodes alone measures ~4.8s of a 9s run #1193); the
    widget loop is unchanged.
  • Elimination 1 should be re-runset-widget.js changed on 2026-08-11 (fix(1087)).

The reframing — reasoned from source, now pinned to the reporter's build

graph_connect does not report success on LiteGraph's return value; that was #397 and it
was fixed. isLinkPersisted requires graph.links[link.id] to exist and the target input
to back-reference that same id, synchronously, failing closed — and the success payload is
unreachable unless it passed. So the payload was accurate when issued: the link existed and
was destroyed later.
The reporter's retry succeeding and persisting fits.

The original version of this claim compared against current source, which does not establish
what the reporter was running. Now measured by blob identity: web/js/lib/connect-verify.js
is blob 5fae91a2 — byte-identical in 0.11.43 (release commit a1ac583f), at the
merge-base, and on main — with the isLinkPersisted call site wired in 0.11.43.

Trap recorded in the doc: git tag --contains reports the earliest containing tag as
v0.11.83 and so reads as "absent from 0.11.43". This repo has 21 tags and tagging began at
v0.11.83, so 0.11.43 has no tag at all. Settle version questions by ancestry against the
release commit.

A fifth mechanism, ruled out from the report itself

LiteGraph destroys the wire a connect replaces; the panel discloses that as replaced_link,
and the disclosure existed in 0.11.43. 142.latent_image was connected exactly once and that
payload carried no replaced_link — so replacement-on-reconnect did not destroy this link.

Why this ships no fix

Hardening panel_connect with a re-verification pass would be ceremony aimed at the wrong
mechanism: it would leave the actual link-destroyer in place, slow every graph build, muddy the
tool's contract, and let the issue be closed as addressed.

Still blocked on the reporter for the panel_edit_node resize calls on 141/144/147 — a resize
is the only operation in their sequence that changes node geometry, which is the one
fingerprint left.

Verification

  • Merged origin/main (clean, no conflict markers).
  • npm run test:unit: all four gates green (i18n, i18n-render, tool-vocabulary, panel-scope);
    4397/4399 pass, 1 todo. The single failure is manager-install.test.mjs "panel_install_node times out while the canvas remains responsive #671
    verifyInstalled … inside the reply window", a wall-clock budget assertion that measured
    19212 ms under full-suite parallel load; the file passes 122/122 run alone. This branch
    changes no executable code.

Draft opened before investigation, per the working loop. Nothing measured yet.

Refs #992
Copilot AI balanced review requested due to automatic review settings August 10, 2026 18:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@artokun artokun changed the title fix(connect): a link reported connected must still be there (#992) spec: #992 is not a connect bug — four mechanisms eliminated, link destroyed later Aug 10, 2026
…tale

The investigation lived only in the PR body, so an empty branch carried it and
nothing in the repo recorded it. Writes it to docs/design/ and labels every
claim by HOW it was established — measured on a rig, or reasoned from source.

Re-checked against main (189 commits touched web/ since the branch point):

- Elimination 2 (reapplyDefsToLiveNodes) is STALE. fix(1172) added a
  refreshComboOptionsFromDefs call inside the sweep; its own comment says the
  sweep previously never touched options.values. The measured function is not
  the shipped one. The conclusion likely survives, but as reasoning.
- Elimination 4 is transitively affected — panel_refresh_nodes runs
  registerNodesFromDefs and the changed sweep in one pass.
- Elimination 3 holds (signature gained a #1193 per-type cache param only).
- Elimination 1 should be re-run — set-widget.js changed on 2026-08-11.
- The reframing holds, and is now pinned to the REPORTER's build:
  connect-verify.js is blob 5fae91a in 0.11.43, at the merge-base and on main,
  with the isLinkPersisted call site wired in 0.11.43. Tag containment cannot
  show this (tagging began at v0.11.83); ancestry against the release commit can.

Refs #992
@artokun artokun changed the title spec: #992 is not a connect bug — four mechanisms eliminated, link destroyed later spec: #992 is not a connect bug — four mechanisms eliminated, two now stale Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants