Close the Newton viewer before dropping the reference to it

source/isaaclab_visualizers/changelog.d/fix-newton-visualizer-viewer-teardown.rst

@@ -0,0 +1,6 @@
+Fixed
+^^^^^
+
+* Fixed :class:`~isaaclab_visualizers.newton.newton_visualizer.NewtonVisualizer` releasing its viewer
+  without calling the viewer's :meth:`close`, which left the RTX backend's ordered GPU teardown to the
+  garbage collector and intermittently leaked render step results and attribute bindings on shutdown.

source/isaaclab_visualizers/isaaclab_visualizers/newton/newton_visualizer.py

@@ -1208,7 +1208,13 @@ def step(self, dt: float) -> None:
                     "[%s] Permanently disabling viewer after unrecoverable initialization failure.",
                     type(self).__name__,
                 )
-                self._viewer = None
+                try:
+                    self._release_viewer()
+                except Exception:
+                    # This handler exists so an unusable viewer disables itself
+                    # instead of aborting training, so a viewer that also fails
+                    # to close must not escape it either.
+                    logger.exception("[%s] Viewer teardown failed.", type(self).__name__)
 
     def is_reset_requested(self) -> bool:
         """Return whether an episode reset was requested via the viewer UI."""
@@ -1244,6 +1250,31 @@ def reset(self, soft: bool = False) -> None:
             if self._picking_enabled:
                 self._viewer_picking_binding.bind(self._viewer)
 
+    def _release_viewer(self) -> None:
+        """Close the viewer this visualizer owns and drop the reference to it.
+
+        The visualizer owns the viewer it creates, and the viewer owns GPU
+        resources that its backend releases in a fixed order:
+        ``ViewerRTX.close()`` waits on the in-flight render, drops the retained
+        step results, unbinds the transform attribute binding and only then
+        releases the ``ovrtx.Renderer``.  Dropping the reference without
+        closing leaves that ordering to the garbage collector, which does not
+        guarantee one; when the renderer is finalized before the resources
+        bound to it, it tears itself down with them still live and leaks them.
+
+        The reference is cleared in a ``finally`` block so an unusable viewer
+        is never retained, while the teardown failure itself still reaches the
+        caller.  ``ViewerBase.close()`` is a no-op, so the non-RTX backends are
+        unaffected.
+        """
+        viewer = self._viewer
+        if viewer is None:
+            return
+        try:
+            viewer.close()
+        finally:
+            self._viewer = None
+
     def close(self) -> None:
         """Release viewer resources."""
         if self._is_closed:
@@ -1252,13 +1283,17 @@ def close(self) -> None:
             # Keep the stable callback registered: captured graphs replay its
             # now-neutral device inputs without retaining the viewer.
             self._viewer_picking_binding.deactivate()
-        if self._viewer is not None:
-            self._viewer = None
-        if self._camera_sensor is not None and self._camera_is_owned:
-            evict_visualizer_camera(self._streaming_camera_key)
-            remove_generated_prims(self._generated_camera_prim_paths)
-        self._camera_sensor = None
-        self._is_closed = True
+        try:
+            self._release_viewer()
+        finally:
+            # A viewer that fails to close must not strand the camera prims or
+            # leave the visualizer looking open; the failure still propagates
+            # to the caller, which logs it and drops the visualizer.
+            if self._camera_sensor is not None and self._camera_is_owned:
+                evict_visualizer_camera(self._streaming_camera_key)
+                remove_generated_prims(self._generated_camera_prim_paths)
+            self._camera_sensor = None
+            self._is_closed = True
 
     def is_running(self) -> bool:
         """Return whether the visualizer should continue stepping."""

source/isaaclab_visualizers/test/test_newton_adapter.py

@@ -487,6 +487,7 @@ def __init__(self):
         self.logged_state = None
         self.logged_contacts = None
         self.logged_arrows = None
+        self.closed = False
 
     def is_paused(self):
         return False
@@ -509,6 +510,10 @@ def log_arrows(self, name, starts, ends, colors):
     def end_frame(self):
         pass
 
+    def close(self):
+        # Mirrors ViewerBase.close(), which every real viewer inherits.
+        self.closed = True
+
     def get_frame(self):
         return SimpleNamespace(numpy=lambda: np.zeros((4, 6, 3), dtype=np.uint8))
 

source/isaaclab_visualizers/test/test_newton_visualizer_viewer_release.py

@@ -0,0 +1,222 @@
+# Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md).
+# All rights reserved.
+#
+# SPDX-License-Identifier: BSD-3-Clause
+
+"""Tests for :class:`NewtonVisualizer` viewer release.
+
+The viewer owns GPU resources that its backend releases in a fixed order when
+``close()`` is called.  Both paths that give up the viewer -- ``close()`` and
+the ``step()`` handler that permanently disables it after an unrecoverable
+failure -- must go through :meth:`NewtonVisualizer._release_viewer` so that
+ordering is honoured instead of being left to the garbage collector.
+
+These tests assert the behaviour (the viewer's ``close()`` runs, and runs
+before the reference is dropped) rather than the absence of a backend log
+message: the message is emitted for only one of several valid finalization
+orders, so asserting on it would pass against unfixed code most of the time.
+
+They also pin the error semantics, which differ by caller.  ``_release_viewer``
+propagates a teardown failure while still clearing the reference.  ``close()``
+lets it propagate to ``SimulationContext``, which already logs it, but finishes
+its own cleanup first.  ``step()`` contains it, because that handler exists so
+an unusable viewer disables itself instead of aborting training.
+"""
+
+from __future__ import annotations
+
+import isaaclab_visualizers.newton.newton_visualizer as newton_visualizer
+import pytest
+from isaaclab_visualizers.newton.newton_visualizer import NewtonVisualizer
+
+pytestmark = [pytest.mark.unit]
+
+
+class _SpyViewer:
+    """Viewer double that records how and when it was closed."""
+
+    def __init__(self, raises: bool = False) -> None:
+        self.close_calls = 0
+        self.referenced_by_owner_at_close: list[bool] = []
+        self.owner: NewtonVisualizer | None = None
+        self._raises = raises
+
+    def close(self) -> None:
+        self.close_calls += 1
+        # Record whether the visualizer still pointed at us while we were being
+        # closed.  The reference must outlive the teardown call.
+        self.referenced_by_owner_at_close.append(getattr(self.owner, "_viewer", None) is self)
+        if self._raises:
+            raise RuntimeError("Failed to create window")
+
+
+def _make_visualizer(viewer: _SpyViewer | None) -> NewtonVisualizer:
+    """Build the minimal visualizer state that the release paths read.
+
+    ``__init__`` is bypassed deliberately: a real visualizer requires a Newton
+    model, a scene data provider and a GPU, none of which this behaviour
+    depends on.
+    """
+    visualizer = object.__new__(NewtonVisualizer)
+    visualizer._is_closed = False
+    visualizer._picking_enabled = False
+    visualizer._viewer = viewer
+    visualizer._camera_sensor = None
+    visualizer._camera_is_owned = False
+    if viewer is not None:
+        viewer.owner = visualizer
+    return visualizer
+
+
+def test_release_viewer_closes_before_clearing_reference() -> None:
+    """The viewer must be closed while the visualizer still references it."""
+    viewer = _SpyViewer()
+    visualizer = _make_visualizer(viewer)
+
+    visualizer._release_viewer()
+
+    assert viewer.close_calls == 1
+    assert viewer.referenced_by_owner_at_close == [True]
+    assert visualizer._viewer is None
+
+
+def test_release_viewer_propagates_failure_and_still_clears_reference() -> None:
+    """A teardown failure must reach the caller, but must not retain the viewer."""
+    viewer = _SpyViewer(raises=True)
+    visualizer = _make_visualizer(viewer)
+
+    with pytest.raises(RuntimeError, match="Failed to create window"):
+        visualizer._release_viewer()
+
+    assert viewer.close_calls == 1
+    assert visualizer._viewer is None
+
+
+def test_release_viewer_without_viewer_is_a_no_op() -> None:
+    """Releasing when no viewer is held must be harmless."""
+    visualizer = _make_visualizer(None)
+
+    visualizer._release_viewer()
+
+    assert visualizer._viewer is None
+
+
+def test_release_viewer_is_idempotent() -> None:
+    """Releasing twice must not close the viewer twice."""
+    viewer = _SpyViewer()
+    visualizer = _make_visualizer(viewer)
+
+    visualizer._release_viewer()
+    visualizer._release_viewer()
+
+    assert viewer.close_calls == 1
+
+
+def test_close_releases_the_viewer() -> None:
+    """``close()`` must release the viewer through the shared path."""
+    viewer = _SpyViewer()
+    visualizer = _make_visualizer(viewer)
+
+    visualizer.close()
+
+    assert viewer.close_calls == 1
+    assert viewer.referenced_by_owner_at_close == [True]
+    assert visualizer._viewer is None
+    assert visualizer._is_closed is True
+
+
+def test_close_is_idempotent() -> None:
+    """A second ``close()`` must not close the viewer again."""
+    viewer = _SpyViewer()
+    visualizer = _make_visualizer(viewer)
+
+    visualizer.close()
+    visualizer.close()
+
+    assert viewer.close_calls == 1
+
+
+def test_close_completes_cleanup_when_viewer_teardown_fails(monkeypatch: pytest.MonkeyPatch) -> None:
+    """A failing viewer must not strand the owned camera or the closed flag.
+
+    ``SimulationContext`` already logs an exception raised by ``close()``, so
+    it is allowed to propagate -- but the rest of the teardown still has to
+    run, otherwise a viewer failure silently leaks the generated camera prims.
+    """
+    evicted: list[object] = []
+    removed: list[object] = []
+    monkeypatch.setattr(newton_visualizer, "evict_visualizer_camera", evicted.append, raising=False)
+    monkeypatch.setattr(newton_visualizer, "remove_generated_prims", removed.append, raising=False)
+
+    viewer = _SpyViewer(raises=True)
+    visualizer = _make_visualizer(viewer)
+    visualizer._camera_sensor = object()
+    visualizer._camera_is_owned = True
+    visualizer._streaming_camera_key = "camera-key"
+    visualizer._generated_camera_prim_paths = ["/World/generated"]
+
+    with pytest.raises(RuntimeError, match="Failed to create window"):
+        visualizer.close()
+
+    assert visualizer._viewer is None
+    assert visualizer._camera_sensor is None
+    assert visualizer._is_closed is True
+    assert evicted == ["camera-key"]
+    assert removed == [["/World/generated"]]
+
+
+def _arm_for_step_failure(visualizer: NewtonVisualizer, viewer: _SpyViewer) -> None:
+    """Drive ``step()`` far enough to reach its viewer-failure handler."""
+    visualizer._is_initialized = True
+    visualizer._runtime_headless = False
+    visualizer._disable_viewer_on_step_exception = True
+    visualizer._sim_time = 0.0
+    visualizer._step_counter = 0
+    visualizer._state = None
+    visualizer._scene_data_provider = None
+    visualizer._update_frequency = 1
+    viewer._update_frequency = 1
+
+    def _unrecoverable() -> bool:
+        raise RuntimeError("Failed to create window")
+
+    viewer.is_paused = _unrecoverable  # type: ignore[method-assign]
+
+
+def test_step_failure_releases_the_viewer(monkeypatch: pytest.MonkeyPatch) -> None:
+    """An unrecoverable viewer failure during ``step()`` must release the viewer.
+
+    ``NewtonRTXVisualizer`` sets ``_disable_viewer_on_step_exception`` so the
+    viewer is given up after the first failure -- for example when OVRTX cannot
+    create its window.  That path must close the viewer rather than only
+    dropping the reference to it.
+    """
+    viewer = _SpyViewer()
+    visualizer = _make_visualizer(viewer)
+    _arm_for_step_failure(visualizer, viewer)
+    monkeypatch.setattr(newton_visualizer.NewtonManager, "get_num_envs", staticmethod(lambda: 1), raising=False)
+
+    NewtonVisualizer.step(visualizer, dt=0.01)  # must not raise
+
+    assert viewer.close_calls == 1
+    assert viewer.referenced_by_owner_at_close == [True]
+    assert visualizer._viewer is None
+
+
+def test_step_contains_a_failing_viewer_teardown(monkeypatch: pytest.MonkeyPatch) -> None:
+    """A viewer that fails to close must not abort training from ``step()``.
+
+    This is the whole purpose of the ``_disable_viewer_on_step_exception``
+    handler: the viewer is already known to be broken, so its teardown failure
+    has to be contained rather than replacing the original failure and
+    propagating out of the simulation loop.
+    """
+    viewer = _SpyViewer(raises=True)
+    visualizer = _make_visualizer(viewer)
+    _arm_for_step_failure(visualizer, viewer)
+    monkeypatch.setattr(newton_visualizer.NewtonManager, "get_num_envs", staticmethod(lambda: 1), raising=False)
+
+    NewtonVisualizer.step(visualizer, dt=0.01)  # must not raise
+
+    assert viewer.close_calls == 1
+    assert visualizer._viewer is None