Close the Newton viewer before dropping the reference to it

NewtonVisualizer gave up its viewer in two places -- close() and the step()
handler that permanently disables the viewer after an unrecoverable failure --
by assigning self._viewer = None without calling viewer.close() first.

ViewerRTX.close() releases GPU resources in a fixed order: it waits on the
in-flight render, drops the retained step results, unbinds the transform
AttributeBinding and only then releases the ovrtx.Renderer. Skipping it leaves
all four objects to the garbage collector, which guarantees no order. When the
collector reaches the Renderer first it tears itself down with the binding and
step results still live, warning "Renderer destroyed with 1 active binding(s)"
and logging "Leaking step result outputs" once per retained render product.

Route both paths through a single _release_viewer() helper that closes the
viewer and clears the reference in a finally block, so an unusable viewer is
never retained while the teardown failure still reaches the caller.

The two callers need different error handling. close() lets the failure
propagate -- SimulationContext._update_visualizers() already wraps it -- but
runs its own remaining cleanup in a finally block so a failing viewer cannot
strand the generated camera prims or leave _is_closed unset. The step() handler
contains the failure, because that handler exists so an unusable viewer
disables itself instead of aborting training; letting a teardown error escape
would replace the original failure and crash the run it was written to save.

ViewerBase.close() is a no-op, so the GL, null and file backends are
unaffected. The viser and rerun visualizers already close their viewers.

The _Viewer double in test_newton_adapter.py gains the close() method that
every real viewer inherits from ViewerBase; without it the double no longer
models the viewers it stands in for.

Author: fatimaanes

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