Skip to content

Stop worker-owning nodes leaking forever when New Chat interrupts a generation#93

Merged
dovvnloading merged 1 commit into
mainfrom
hotfix/worker-node-dispose-lifecycle
Jul 22, 2026
Merged

Stop worker-owning nodes leaking forever when New Chat interrupts a generation#93
dovvnloading merged 1 commit into
mainfrom
hotfix/worker-node-dispose-lifecycle

Conversation

@dovvnloading

Copy link
Copy Markdown
Owner

Summary

Started as a narrow fix (give ArtifactNode a GC-time __del__, matching CodeSandboxNode) and grew once verification showed the __del__ alone wouldn't do anything.

Root cause 1: ChatScene._teardown_items_before_clear() already calls dispose() explicitly for chart_nodes on every clear() (New Chat / chat-switch), but never did for any worker-owning node type (artifact/conversation/pycoder/code_sandbox/gitlink) — it only stopped their hover-animation timers. A GC-time __del__ backstop is inherently non-deterministic, and (per root cause 2) sometimes can't fire at all. _teardown_items_before_clear() now calls dispose() on all five worker-owning node lists, mirroring the existing chart_nodes treatment — New Chat now stops an in-flight generation the instant it happens.

Root cause 2 (found while checking whether the __del__ fix would even matter): ArtifactNode was not collectible by Python's GC at all, ever. Two independent causes, both confirmed empirically:

  • ArtifactNode's own instruction_input.submit_requested was wired via a lambda closing over self on a custom Signal — fixed with a bound method.
  • graphlink_window_actions.py wires each of ArtifactNode's/ConversationNode's/PyCoderNode's/CodeSandboxNode's worker's own signals via a lambda carrying node=/thread= as default args. PySide6's GC does not reclaim a lambda connected to a custom Signal (a bound-method connection to the same signal is reclaimed fine) — so as long as those connections stood, both the worker and the node were immortal for the rest of the process, regardless of dispose(). GitlinkNode was the one sibling whose dispose() already disconnected the worker's own signals; that pattern is now applied to the other four.

Changes

  • dispose() on ArtifactNode/ConversationNode/PyCoderNode/CodeSandboxNode now disconnects the worker's own signals (mirroring GitlinkNode) before nulling the reference
  • ArtifactNode's internal lambda replaced with a bound method
  • ArtifactNode/ConversationNode gained a guarded __del__ (defense-in-depth, matching CodeSandboxNode's existing convention — no longer the primary mechanism now that clear() calls dispose() deterministically)
  • ChatScene._teardown_items_before_clear() calls dispose() on all 5 worker-owning node lists

Test plan

  • Adversarial verification workflow (2 independent agents, 120 tool calls) re-confirmed the fix and reproduced every leak empirically with the real worker classes — no mocks, since a mock silently absorbs a bad connect() call and hides this exact bug class
  • New regression tests reproduce the real graphlink_window_actions.py wiring for all four node types and prove both the node and the worker become GC-collectible after dispose()
  • Full suite: 1705 passed, zero regressions
  • Real app launch verified clean (zero errors in the log)

The workflow also found the identical lambda-over-self pattern in three unrelated context-menu classes and ColorPickerDialog — a different feature area (right-click menus, not worker-owning nodes), not touched here; flagged separately as a follow-up task.

Third of the fixes from the recent adversarial bug-scan (after #91, #92).

…eneration

Started as a narrow fix (give ArtifactNode a GC-time __del__, matching
CodeSandboxNode) and grew once verification showed the __del__ alone
wouldn't do anything.

Root cause 1: ChatScene._teardown_items_before_clear() already calls
dispose() explicitly for chart_nodes on every clear() (New Chat /
chat-switch), but never did for ANY worker-owning node type
(artifact/conversation/pycoder/code_sandbox/gitlink) - it only stopped
their hover-animation timers. A GC-time __del__ backstop is inherently
non-deterministic (it can only fire once Python decides the object is
collectible) and, per root cause 2, sometimes can't fire at all.
_teardown_items_before_clear() now calls dispose() on all five
worker-owning node lists, exactly mirroring the chart_nodes treatment it
already had - New Chat now stops an in-flight generation the instant it
happens, not "eventually, if GC gets around to it."

Root cause 2 (found while checking whether the __del__ fix would even
matter): ArtifactNode was not collectible by Python's GC at all, ever,
regardless of clear()/dispose(). Two independent causes, both confirmed
empirically:
  - ArtifactNode's own instruction_input.submit_requested was wired via
    `lambda: self.artifact_requested.emit(self)` - a lambda closing over
    self, connected to a custom Signal. PySide6's GC does not reclaim this
    shape (a bound-method connection to the same signal is reclaimed
    fine) - fixed by connecting a bound method instead.
  - graphlink_window_actions.py wires each of ArtifactNode's/
    ConversationNode's/PyCoderNode's/CodeSandboxNode's worker's OWN
    finished/error(/status/cancelled/log_update/approval_requested)
    signals via a lambda carrying node=/thread= as default args. As long
    as those connections stood, BOTH the worker and the node were immortal
    for the rest of the process - dispose()'s is_disposed guard never
    mattered because nothing could ever collect the node to run a GC-time
    __del__ in the first place. GitlinkNode was the one sibling whose
    dispose() already disconnected the worker's own signals
    (graphlink_plugin_gitlink.py:1385-1408); that pattern is now applied to
    the other four.

Net changes: dispose() on all four affected node types now disconnects
the worker's own signals (mirroring GitlinkNode) before nulling the
reference; ArtifactNode's internal lambda replaced with a bound method;
ArtifactNode/ConversationNode gained a guarded __del__ (defense-in-depth,
matching CodeSandboxNode's existing convention - no longer the primary
mechanism now that clear() calls dispose() deterministically).

Verified: an adversarial workflow (2 independent agents, 120 tool calls)
re-confirmed the fix and reproduced every leak empirically with the real
worker classes (no mocks - a mock silently absorbs a bad connect() call
and hides this exact bug class). New regression tests reproduce the real
graphlink_window_actions.py wiring for all four node types and prove both
the node and the worker become GC-collectible after dispose(). Full suite
1705 passed. Real app launch verified clean (zero errors in the log).

The workflow also found the identical lambda-over-self pattern in three
unrelated context-menu classes and ColorPickerDialog - a different feature
area, not touched here; flagged separately as a follow-up.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dovvnloading
dovvnloading merged commit 5b2f8a4 into main Jul 22, 2026
2 checks passed
@dovvnloading
dovvnloading deleted the hotfix/worker-node-dispose-lifecycle branch July 22, 2026 02:07
dovvnloading added a commit that referenced this pull request Jul 22, 2026
…erDialog (#96)

An adversarial bug-scan workflow found the same lambda-GC-cycle pattern
already fixed for worker-owning nodes (PR #93) in an unrelated feature
area: right-click context menus and the frame/container/note "change
color" popup.

Root cause, identical to #93: a Python lambda closing over self,
connected to a Qt signal on a widget owned by self (a QAction/QPushButton
child of the menu/dialog). PySide6's GC does not reclaim this shape - a
bound-method connection to the same signal is reclaimed fine, a lambda
is not - so every menu/dialog instance ever constructed leaked forever,
for the rest of the process.

Confirmed and fixed in four places:
- ChatNodeContextMenu: the 5 export-format actions, the per-docked-node
  "reveal" actions, and the per-chart-type actions all used
  `lambda ...: self.something(...)`. Also pinned the ChatNode itself
  alive forever via self.node, on EVERY right-click.
- CodeNodeContextMenu / DocumentNodeContextMenu: the same 5-action
  export-menu shape (found by grepping the identical code pattern, not
  just trusting the report's line numbers - both matched exactly).
- ColorPickerDialog: the "reset to default" button and every palette
  swatch button. Notably NOT limited to custom Signals or worker
  threads - a stock QPushButton.clicked calling a plain method (no
  Signal.emit involved) forms the identical GC-invisible cycle.

Fixed by moving the per-instance variant onto the widget itself
(QAction.setData() / QWidget.setProperty()) and connecting every
action/button to ONE shared bound-method dispatcher that reads
self.sender().data()/.property(...) - removes the self-capturing
closure without changing behavior.

Verified empirically for all four (real menu/dialog + real canvas node,
no mocks - weakref + gc.collect() after construction) that both the
widget and, for the three context menus, the node it stores now
collect cleanly. New tests also confirm every export/reveal/chart/
swatch action still dispatches to the correct variant. Full suite 1730
passed.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.

1 participant