diff --git a/graphlink_app/graphlink_conversation_node.py b/graphlink_app/graphlink_conversation_node.py index 3c45bb3..b0aa523 100644 --- a/graphlink_app/graphlink_conversation_node.py +++ b/graphlink_app/graphlink_conversation_node.py @@ -199,6 +199,20 @@ def __init__(self, parent_node, parent=None): self.proxy = QGraphicsProxyWidget(self) self.proxy.setWidget(self.widget) + def __del__(self): + # GC-time last resort, mirroring CodeSandboxNode.__del__ / + # ArtifactNode.__del__: dispose() is now also called deterministically + # from ChatScene._teardown_items_before_clear() on every clear() (New + # Chat / chat-switch), so this is defense-in-depth for any path that + # somehow bypasses both that and deleteSelectedItems(). Guarded + # because during interpreter shutdown the underlying C++ QThread/ + # QObject may already be gone, making dispose()'s worker.isRunning() + # raise inside __del__. + try: + self.dispose() + except Exception: + pass + def dispose(self): # Called by ChatScene.deleteSelectedItems when this node is removed. Cancel # the in-flight ChatWorkerThread so deletion doesn't orphan a live QThread: @@ -213,12 +227,33 @@ def dispose(self): return self.is_disposed = True worker = getattr(self, "worker_thread", None) - try: - if worker and worker.isRunning(): - worker.cancel() - except RuntimeError: - pass self.worker_thread = None + if worker: + try: + if worker.isRunning(): + worker.cancel() + except RuntimeError: + pass + # handle_conversation_node_request (graphlink_window_actions.py) + # wires this worker's own finished/error/cancelled to lambdas + # carrying node=/thread= default args - PySide6's GC does not + # reclaim a lambda connected to a custom Signal (empirically + # confirmed; a bound-method connection, like `status` below, is + # fine), so as long as those connections stand, this worker and + # this node are BOTH immortal for the rest of the process. + # Disconnecting here breaks that cycle. Mirrors GitlinkNode.dispose(). + for signal in (worker.finished, worker.error, worker.status, worker.cancelled): + try: + signal.disconnect() + except (TypeError, RuntimeError): + pass + try: + if worker.isRunning(): + worker.finished.connect(worker.deleteLater) + else: + worker.deleteLater() + except RuntimeError: + pass @property def width(self): diff --git a/graphlink_app/graphlink_plugins/graphlink_plugin_artifact.py b/graphlink_app/graphlink_plugins/graphlink_plugin_artifact.py index f09d9f7..8fc4e17 100644 --- a/graphlink_app/graphlink_plugins/graphlink_plugin_artifact.py +++ b/graphlink_app/graphlink_plugins/graphlink_plugin_artifact.py @@ -179,9 +179,50 @@ def dispose(self): return self.is_disposed = True worker = getattr(self, "worker_thread", None) - if worker and worker.isRunning(): - worker.stop() self.worker_thread = None + if worker: + try: + if worker.isRunning(): + worker.stop() + except Exception: + pass + # execute_artifact_node (graphlink_window_actions.py) wires this + # worker's own finished/error to lambdas carrying node=/thread= + # default args - PySide6's GC does not reclaim a lambda connected + # to a custom Signal (empirically confirmed; a bound-method + # connection is fine), so as long as those connections stand, this + # worker and this node are BOTH immortal for the rest of the + # process - dispose()'s own is_disposed=True guard never even runs + # again because __del__/dispose can no longer fire. Disconnecting + # here breaks that cycle. Mirrors GitlinkNode.dispose(), the one + # sibling that already did this correctly. + for signal in (worker.finished, worker.error): + try: + signal.disconnect() + except (TypeError, RuntimeError): + pass + try: + if worker.isRunning(): + worker.finished.connect(worker.deleteLater) + else: + worker.deleteLater() + except RuntimeError: + pass + + def __del__(self): + # GC-time last resort. ChatScene._teardown_items_before_clear() now + # also calls dispose() deterministically on every clear() (New Chat / + # chat-switch) - this is defense-in-depth for any path that somehow + # bypasses both that and deleteSelectedItems(). Guarded because during + # interpreter shutdown the underlying C++ QThread/QObject may already + # be deleted, + # making dispose()'s worker.isRunning() raise inside __del__ (same + # hazard as CodeSandboxNode.__del__, audit finding B5). Mirrors + # CodeSandboxNode exactly. + try: + self.dispose() + except Exception: + pass @property def width(self): @@ -308,7 +349,7 @@ def _setup_ui(self): } """ + ARTIFACT_SCROLLBAR_STYLE) self.instruction_input.textChanged.connect(self._on_instruction_changed) - self.instruction_input.submit_requested.connect(lambda: self.artifact_requested.emit(self)) + self.instruction_input.submit_requested.connect(self._on_instruction_submit_requested) input_layout.addWidget(self.instruction_input, stretch=1) self.update_button = QPushButton() @@ -401,6 +442,16 @@ def _on_tab_changed(self, index): def _on_instruction_changed(self): self.instruction = self.instruction_input.toPlainText() + def _on_instruction_submit_requested(self): + # Was `submit_requested.connect(lambda: self.artifact_requested.emit(self))`. + # A lambda closing over self, connected to a custom (non-widget) Signal, + # is NOT a cycle PySide6's GC can break (confirmed empirically - unlike + # a signal connected to a bound method, which it collects fine); it held + # every ArtifactNode alive forever, silently defeating __del__'s + # GC-time dispose() below no matter what New Chat / chat-switch did. + # A plain bound method reaches the same target with no such trap. + self.artifact_requested.emit(self) + def _on_content_changed(self): self.artifact_content = self.raw_editor.toPlainText() diff --git a/graphlink_app/graphlink_plugins/graphlink_plugin_code_sandbox.py b/graphlink_app/graphlink_plugins/graphlink_plugin_code_sandbox.py index 5093632..46b3c03 100644 --- a/graphlink_app/graphlink_plugins/graphlink_plugin_code_sandbox.py +++ b/graphlink_app/graphlink_plugins/graphlink_plugin_code_sandbox.py @@ -769,9 +769,34 @@ def dispose(self): return self.is_disposed = True worker = getattr(self, "worker_thread", None) - if worker and worker.isRunning(): - worker.stop() self.worker_thread = None + if worker: + try: + if worker.isRunning(): + worker.stop() + except Exception: + pass + # The run path (graphlink_window_actions.py) wires this worker's + # own finished/error/log_update/terminal_chunk/approval_requested + # to lambdas carrying node=/thread= default args - PySide6's GC + # does not reclaim a lambda connected to a custom Signal + # (empirically confirmed), so as long as those connections stand, + # this worker and this node are BOTH immortal for the rest of the + # process. Disconnecting here breaks that cycle. Mirrors + # GitlinkNode.dispose(), the one sibling that already did this. + for signal in (worker.finished, worker.error, worker.log_update, + worker.terminal_chunk, worker.approval_requested): + try: + signal.disconnect() + except (TypeError, RuntimeError): + pass + try: + if worker.isRunning(): + worker.finished.connect(worker.deleteLater) + else: + worker.deleteLater() + except RuntimeError: + pass def _on_prompt_changed(self): self.prompt = self.prompt_input.toPlainText() diff --git a/graphlink_app/graphlink_pycoder.py b/graphlink_app/graphlink_pycoder.py index f116fe2..54b259d 100644 --- a/graphlink_app/graphlink_pycoder.py +++ b/graphlink_app/graphlink_pycoder.py @@ -390,6 +390,20 @@ def __init__(self, parent_node, mode=PyCoderMode.AI_DRIVEN, parent=None): self._update_ui_for_mode() + def __del__(self): + # GC-time last resort, mirroring CodeSandboxNode.__del__ / + # ArtifactNode.__del__: dispose() is now also called deterministically + # from ChatScene._teardown_items_before_clear() on every clear() (New + # Chat / chat-switch), so this is defense-in-depth for any path that + # somehow bypasses both that and deleteSelectedItems(). Guarded + # because during interpreter shutdown the underlying C++ QThread/ + # QObject may already be gone, making dispose()'s worker.isRunning() + # raise inside __del__. + try: + self.dispose() + except Exception: + pass + def dispose(self): # Called by ChatScene.deleteSelectedItems when this node is removed. Stop the # running worker so deletion doesn't orphan a live QThread: once the node @@ -402,9 +416,37 @@ def dispose(self): return self.is_disposed = True worker = getattr(self, "worker_thread", None) - if worker and worker.isRunning(): - worker.stop() self.worker_thread = None + if worker: + try: + if worker.isRunning(): + worker.stop() + except Exception: + pass + # run_pycoder_node (graphlink_window_actions.py) wires this + # worker's own finished/error(/log_update/approval_requested for + # the AI-driven worker type) to lambdas carrying node=/thread= + # default args - PySide6's GC does not reclaim a lambda connected + # to a custom Signal (empirically confirmed), so as long as those + # connections stand, this worker and this node are BOTH immortal + # for the rest of the process. Disconnecting here breaks that + # cycle. Mirrors GitlinkNode.dispose(). getattr-guarded because + # the two worker types (CodeExecutionWorker for MANUAL, + # PyCoderExecutionWorker for AI_DRIVEN) don't share every signal. + for signal_name in ("finished", "error", "log_update", "approval_requested"): + signal = getattr(worker, signal_name, None) + if signal is not None: + try: + signal.disconnect() + except (TypeError, RuntimeError): + pass + try: + if worker.isRunning(): + worker.finished.connect(worker.deleteLater) + else: + worker.deleteLater() + except RuntimeError: + pass # The REPL subprocess is owned by ChatWindow.pycoder_repl_manager, not this # node - stop it here for immediate, deterministic cleanup on this (the # right-click-delete) path. self.scene() is None for nodes never added to a diff --git a/graphlink_app/graphlink_scene.py b/graphlink_app/graphlink_scene.py index bcc5478..ec9d4ba 100644 --- a/graphlink_app/graphlink_scene.py +++ b/graphlink_app/graphlink_scene.py @@ -1711,6 +1711,24 @@ def _try_teardown(item, method_name): for chart in self.chart_nodes: _try_teardown(chart, "dispose") # ChartItem.dispose() is timer/figure-only + # Bug-scan finding: dispose() (stops the node's background QThread + # worker, and - for the 4 that wire it via a lambda closing over the + # node/thread, see each dispose()'s own comment - disconnects the + # worker's own signals so neither the worker nor the node is pinned + # alive forever) was only ever invoked from deleteSelectedItems(), + # never from clear(). New Chat / chat-switching mid-generation left + # the worker running with nothing to stop it, for every worker-owning + # node type, not just the one (ArtifactNode) this was first noticed + # on. GitlinkNode's dispose() already disconnected correctly - it was + # equally never being called here either. + worker_owning_node_lists = ( + self.pycoder_nodes, self.code_sandbox_nodes, + self.conversation_nodes, self.artifact_nodes, self.gitlink_nodes, + ) + for node_list in worker_owning_node_lists: + for node in node_list: + _try_teardown(node, "dispose") + def clear(self): """ Clears the entire scene, removing all items and resetting all tracking lists. diff --git a/graphlink_app/tests/test_plugin_node_dispose_lifecycle.py b/graphlink_app/tests/test_plugin_node_dispose_lifecycle.py index 66fa59f..8a460f2 100644 --- a/graphlink_app/tests/test_plugin_node_dispose_lifecycle.py +++ b/graphlink_app/tests/test_plugin_node_dispose_lifecycle.py @@ -12,22 +12,74 @@ cooperative-stop API), both scene delete branches call it via the existing hasattr gate, and the artifact result handlers guard on is_disposed like every sibling handler already did. + +Follow-up bug-scan finding, part 1: dispose() alone only fired on the +deleteSelectedItems() path. ChatScene.clear() (New Chat / chat-switch) +never called dispose() on ANY plugin node except chart_nodes - it deleted +the C++ object directly. _teardown_items_before_clear() now also calls +dispose() on every worker-owning node list (artifact/conversation/pycoder/ +code_sandbox/gitlink), exactly matching the chart_nodes treatment it already +had, so a generation in flight when New Chat happens is stopped +deterministically - not dependent on Python's GC ever running. + +Follow-up bug-scan finding, part 2 (discovered while verifying part 1's fix +would even matter): ArtifactNode/ConversationNode/PyCoderNode/CodeSandboxNode +all wire their worker's OWN finished/error(/status/cancelled/log_update/ +approval_requested) signals in graphlink_window_actions.py via a lambda +closing over the node/thread (`lambda ..., node=the_node: ...`) on a CUSTOM +Signal. Empirically confirmed: PySide6's GC does not reclaim this shape (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 - dispose()'s is_disposed guard was irrelevant because +nothing could ever collect the node to run a GC-time __del__ in the first +place, and any __del__-based backstop (ArtifactNode's own, added alongside +this fix) could never fire either. GitlinkNode was the one sibling that +already disconnected its worker's signals in dispose() - matching that now +in the other four breaks the cycle. ArtifactNode also had a second, +independent instance of the same root pattern purely internal to its own UI +(instruction_input.submit_requested wired via a self-closing lambda, +unrelated to any worker thread) - fixed by using a bound method instead. """ +import gc import sys from pathlib import Path from unittest.mock import MagicMock sys.path.insert(0, str(Path(__file__).resolve().parents[1])) +from PySide6.QtCore import QCoreApplication, QEvent from PySide6.QtWidgets import QApplication _APP = QApplication.instance() or QApplication([]) +from graphlink_agents_artifact import ArtifactWorkerThread +from graphlink_agents_code_sandbox import CodeSandboxExecutionWorker +from graphlink_agents_core import ChatWorkerThread +from graphlink_agents_pycoder import CodeExecutionWorker from graphlink_conversation_node import ConversationNode from graphlink_plugins.graphlink_plugin_artifact import ArtifactNode +from graphlink_plugins.graphlink_plugin_code_sandbox import CodeSandboxNode +from graphlink_pycoder import PyCoderNode from graphlink_scene import ChatScene from graphlink_window_actions import WindowActionsMixin +import weakref + + +def _flush_deferred_deletes(): + # worker.deleteLater() (called by dispose()) posts a QEvent.DeferredDelete + # rather than deleting synchronously - a live running app's event loop is + # always pumping one, so this resolves near-instantly there, but this + # isolated test never runs one. processEvents() alone was NOT sufficient + # to flush it (empirically confirmed); sendPostedEvents targeting + # DeferredDelete specifically is what actually processes it. This is a + # benign QThread-teardown timing artifact of the test harness, not a + # reference-cycle leak (confirmed via gc.get_referrers: zero Python-level + # referrers on the "still alive" object before this flush). + gc.collect() + _APP.processEvents() + QCoreApplication.sendPostedEvents(None, QEvent.Type.DeferredDelete) + gc.collect() def _make_scene(): @@ -117,6 +169,67 @@ def test_scene_delete_path_disposes_the_node(self): assert node not in scene.conversation_nodes +class TestArtifactNodeDelBackstop: + """ChatScene.clear() (New Chat / chat-switch) never calls dispose() on + plugin nodes - it deletes the C++ object directly, bypassing every + hasattr(...).dispose() gate in deleteSelectedItems. dispose() alone (A3) + therefore did nothing for that path; only a GC-time __del__ can still + catch it, mirroring CodeSandboxNode.__del__.""" + + def test_del_stops_a_running_worker_when_the_node_is_garbage_collected(self): + node = ArtifactNode(parent_node=None) + worker = MagicMock() + worker.isRunning.return_value = True + node.worker_thread = worker + + del node + gc.collect() # ArtifactNode/HoverAnimationMixin's timer.timeout->bound + # method is a reference cycle - refcounting alone won't collect it. + + worker.stop.assert_called_once() + + def test_del_swallows_exceptions_raised_during_interpreter_shutdown(self): + # During interpreter shutdown the underlying C++ QThread/QObject may + # already be gone, so dispose()'s worker.isRunning() can itself raise + # (audit finding B5's exact hazard) - __del__ must not propagate that. + node = ArtifactNode(parent_node=None) + worker = MagicMock() + worker.isRunning.side_effect = RuntimeError("Internal C++ object already deleted") + node.worker_thread = worker + + node.__del__() # must not raise + + def test_del_after_an_explicit_dispose_does_not_touch_the_worker_again(self): + node = ArtifactNode(parent_node=None) + node.dispose() + node.worker_thread = MagicMock() # __del__'s dispose() must no-op + + node.__del__() + + node.worker_thread.stop.assert_not_called() + + def test_new_chat_mid_generate_end_to_end_stops_the_orphaned_worker(self): + # The actual bug: start a generation, then New Chat (scene.clear()) + # before it finishes. Without __del__, nothing stops this worker - + # scene.clear() has already emptied scene.artifact_nodes, so even + # ChatWindow._iter_shutdown_threads at app-close can no longer find it. + scene = _make_scene() + parent = scene.add_chat_node("parent", is_user=True) + node = ArtifactNode(parent) + scene.addItem(node) + scene.artifact_nodes.append(node) + worker = MagicMock() + worker.isRunning.return_value = True + node.worker_thread = worker + + scene.clear() + assert node not in scene.artifact_nodes + del node + gc.collect() + + worker.stop.assert_called_once() + + class TestArtifactHandlersGuardDisposedNodes: def _window(self): class _FakeWindow(WindowActionsMixin): @@ -170,3 +283,137 @@ def test_delete_mid_generate_end_to_end_does_not_touch_the_removed_node(self): window._handle_artifact_result("late result", "late message", node) assert node.get_artifact_content() == before + + +class TestSceneClearDisposesEveryWorkerOwningNodeType: + """The primary fix: _teardown_items_before_clear() must call dispose() on + every worker-owning node list, not just chart_nodes. This is what makes + New Chat / chat-switch stop an in-flight generation deterministically, + instead of depending on whether/when Python's GC ever collects the + abandoned node.""" + + def test_scene_clear_calls_dispose_on_all_five_worker_owning_node_types(self): + scene = _make_scene() + nodes = [] + for list_name, node_cls in ( + ("artifact_nodes", ArtifactNode), + ("conversation_nodes", ConversationNode), + ("pycoder_nodes", PyCoderNode), + ("code_sandbox_nodes", CodeSandboxNode), + ): + parent = scene.add_chat_node(f"parent-{list_name}", is_user=True) + node = node_cls(parent) + scene.addItem(node) + getattr(scene, list_name).append(node) + node.dispose = MagicMock(wraps=node.dispose) + nodes.append(node) + + scene.clear() + + for node in nodes: + node.dispose.assert_called_once() + + def test_scene_clear_stops_a_running_worker_deterministically_no_gc_needed(self): + # The concrete payoff: an in-flight generation is stopped the instant + # New Chat happens, not "eventually, whenever gc.collect() runs". + scene = _make_scene() + parent = scene.add_chat_node("parent", is_user=True) + node = ArtifactNode(parent) + scene.addItem(node) + scene.artifact_nodes.append(node) + worker = MagicMock() + worker.isRunning.return_value = True + node.worker_thread = worker + + scene.clear() + + worker.stop.assert_called_once() + assert node.is_disposed is True + + +class TestDisposeBreaksTheWindowActionsSignalCycle: + """graphlink_window_actions.py wires each worker's OWN finished/error(/...) + signals via a lambda closing over the node/thread on a custom Signal - + empirically confirmed uncollectable by PySide6's GC (unlike the same + shape connected to a bound method). Reproduces that exact wiring with the + REAL worker classes (no mocks - a mock would silently absorb the + connect() call and hide the leak) and proves dispose() breaks the cycle: + both the node and the worker become collectible afterward.""" + + def test_artifact_node_and_worker_collect_after_dispose(self): + node = ArtifactNode(parent_node=None) + worker = ArtifactWorkerThread("current doc", []) + node.worker_thread = worker + # Exact shape of execute_artifact_node's wiring (graphlink_window_actions.py). + worker.finished.connect(lambda doc, msg, n=node: None) + worker.error.connect(lambda err, n=node: None) + worker.finished.connect(lambda _doc, _msg, thread=worker, n=node: None) + worker.error.connect(lambda _err, thread=worker, n=node: None) + + node.dispose() + + node_ref, worker_ref = weakref.ref(node), weakref.ref(worker) + del node, worker + _flush_deferred_deletes() + assert node_ref() is None, "ArtifactNode still alive - dispose() did not break the cycle" + assert worker_ref() is None, "ArtifactWorkerThread still alive - dispose() did not break the cycle" + + def test_conversation_node_and_worker_collect_after_dispose(self): + node = ConversationNode(parent_node=None) + worker = ChatWorkerThread(agent=MagicMock(), conversation_history=[], current_node=None) + node.worker_thread = worker + # Exact shape of handle_conversation_node_request's wiring. + worker.finished.connect(lambda msg, n=node, thread=worker: None) + worker.status.connect(lambda *_: None) # the one bound-method-safe connection in production + worker.error.connect(lambda err, n=node, thread=worker: None) + worker.cancelled.connect(lambda n=node, thread=worker: None) + worker.finished.connect(lambda _msg, n=node, thread=worker: None) + worker.error.connect(lambda _err, n=node, thread=worker: None) + worker.cancelled.connect(lambda n=node, thread=worker: None) + + node.dispose() + + node_ref, worker_ref = weakref.ref(node), weakref.ref(worker) + del node, worker + _flush_deferred_deletes() + assert node_ref() is None, "ConversationNode still alive - dispose() did not break the cycle" + assert worker_ref() is None, "ChatWorkerThread still alive - dispose() did not break the cycle" + + def test_pycoder_node_and_manual_worker_collect_after_dispose(self): + node = PyCoderNode(parent_node=None) + worker = CodeExecutionWorker("print(1)", repl=MagicMock()) + node.worker_thread = worker + # Exact shape of run_pycoder_node's manual-mode wiring. + worker.finished.connect(lambda output, history=[]: None) + worker.error.connect(lambda error_msg, n=node: None) + worker.finished.connect(lambda _output, thread=worker, n=node: None) + worker.error.connect(lambda _error, thread=worker, n=node: None) + + node.dispose() + + node_ref, worker_ref = weakref.ref(node), weakref.ref(worker) + del node, worker + _flush_deferred_deletes() + assert node_ref() is None, "PyCoderNode still alive - dispose() did not break the cycle" + assert worker_ref() is None, "CodeExecutionWorker still alive - dispose() did not break the cycle" + + def test_code_sandbox_node_and_worker_collect_after_dispose(self): + node = CodeSandboxNode(parent_node=None) + worker = CodeSandboxExecutionWorker("sandbox-1", "", [], "") + node.worker_thread = worker + # Exact shape of the sandbox run path's wiring. + worker.log_update.connect(lambda *_: None) # bound-method-safe in production + worker.terminal_chunk.connect(lambda *_: None) # bound-method-safe in production + worker.approval_requested.connect(lambda code, reqs, w=worker, n=node: None) + worker.finished.connect(lambda result, n=node, history=[], mode="generate": None) + worker.error.connect(lambda error_msg, n=node: None) + worker.finished.connect(lambda _result, thread=worker, n=node: None) + worker.error.connect(lambda _error, thread=worker, n=node: None) + + node.dispose() + + node_ref, worker_ref = weakref.ref(node), weakref.ref(worker) + del node, worker + _flush_deferred_deletes() + assert node_ref() is None, "CodeSandboxNode still alive - dispose() did not break the cycle" + assert worker_ref() is None, "CodeSandboxExecutionWorker still alive - dispose() did not break the cycle" diff --git a/graphlink_app/tests/test_pycoder_repl_ownership.py b/graphlink_app/tests/test_pycoder_repl_ownership.py index 89892f0..19aef95 100644 --- a/graphlink_app/tests/test_pycoder_repl_ownership.py +++ b/graphlink_app/tests/test_pycoder_repl_ownership.py @@ -15,9 +15,23 @@ PyCoderReplManager preserves both guarantees without an explicit __del__: a weakref.finalize registered at REPL-creation time stops the subprocess when the node is garbage collected regardless of whether -stop()/dispose() was ever called (covering the clear()-only path), and -dispose() still calls manager.stop() directly for immediate, deterministic -cleanup on the right-click-delete path. +stop()/dispose() was ever called, and dispose() still calls manager.stop() +directly for immediate, deterministic cleanup on the right-click-delete path. + +Follow-up bug-scan finding: ChatScene._teardown_items_before_clear() now ALSO +calls dispose() on pycoder_nodes (and every other worker-owning node list), +exactly like it already did for chart_nodes - clear() is no longer a +dispose()-free path. This was found while fixing the identical gap for +ArtifactNode/ConversationNode/CodeSandboxNode/GitlinkNode: none of them had +dispose() invoked on the clear() path either, and for the four whose +worker-thread wiring in graphlink_window_actions.py uses a lambda closing +over the node/thread on a custom Signal, the node was not just left running +a beat longer - it was made permanently uncollectable by Python's GC (the +finalizer/​__del__ backstop those nodes rely on never even fires), because +disconnecting those signals only happens inside dispose(). The +weakref.finalize here still fires as a backstop for any path that somehow +bypasses both dispose() call sites, but it is no longer the only thing +protecting the clear() path. """ import gc @@ -130,10 +144,18 @@ def test_pycoder_node_init_no_longer_constructs_its_own_repl(self): assert not hasattr(node, "repl") - def test_pycoder_node_has_no_del_method_of_its_own(self): - # __del__'s only content was self.repl.stop() - now dead once ownership - # moved to the manager, so it was deleted rather than left as a no-op. - assert "__del__" not in PyCoderNode.__dict__ + def test_pycoder_node_del_method_does_not_touch_the_repl(self): + # At increment 4, __del__'s only content was self.repl.stop() - dead + # once REPL ownership moved to the manager, so it was deleted rather + # than left as a no-op. A bug-scan follow-up later reintroduced + # __del__ for a DIFFERENT reason (a GC-time backstop for + # worker_thread, mirroring CodeSandboxNode/ArtifactNode) - it must + # stay REPL-agnostic; the manager's own weakref.finalize is still the + # only thing that stops the REPL subprocess at GC time. + assert not hasattr(PyCoderNode, "repl") + node = PyCoderNode(parent_node=None) + assert not hasattr(node, "repl") + node.__del__() # must not raise - no self.repl to call .stop() on def test_dispose_calls_through_to_the_real_manager_via_scene_window(self): window = MagicMock() @@ -165,12 +187,15 @@ def test_dispose_is_a_no_op_on_the_manager_when_no_repl_was_ever_created(self): node.dispose() # must not raise even though get_repl() was never called -class TestClearPathReliesOnTheFinalizerNotDispose: - """Directly proves the pivotal recon finding behind this whole increment: - ChatScene.clear() does not call dispose(), so only the finalizer protects - the New Chat / chat-switch path.""" +class TestClearPathNowCallsDisposeDeterministically: + """Bug-scan follow-up: ChatScene.clear() now calls dispose() on + pycoder_nodes (matching the pre-existing chart_nodes treatment), so the + REPL stops synchronously when New Chat happens - it no longer depends on + Python's GC ever running the finalizer, non-deterministic timing that + could leave a REPL subprocess (and, for the sibling node types, the whole + node) alive far longer than the user's screen suggested.""" - def test_scene_clear_skips_dispose_but_gc_afterwards_still_stops_the_repl(self): + def test_scene_clear_calls_dispose_and_stops_the_repl_synchronously(self): with patch.object(PythonREPL, "stop") as mock_stop: window = MagicMock() window.pycoder_repl_manager = PyCoderReplManager() @@ -180,15 +205,23 @@ def test_scene_clear_skips_dispose_but_gc_afterwards_still_stops_the_repl(self): scene.addItem(node) scene.pycoder_nodes.append(node) window.pycoder_repl_manager.get_repl(node) - node.dispose = MagicMock() scene.clear() - node.dispose.assert_not_called() - mock_stop.assert_not_called() # not yet - node/parent are still referenced locally + assert node.is_disposed is True + mock_stop.assert_called_once() # synchronous - no del/gc.collect() needed + + def test_the_finalizer_still_backstops_a_node_that_never_goes_through_clear_or_dispose(self): + # Defense-in-depth check: the finalizer must still work standalone + # (e.g. a node dropped without ever being added to a scene, as in a + # headless test) - clear()-calling-dispose() is an additional + # guarantee, not a replacement for this one. + with patch.object(PythonREPL, "stop") as mock_stop: + manager = PyCoderReplManager() + node = PyCoderNode(parent_node=None) + manager.get_repl(node) del node - del parent gc.collect() mock_stop.assert_called_once()