Make AppLauncher teardown truthful and re-entrancy-safe; document mGPU NCCL workaround

docs/source/features/multi_gpu.rst

@@ -146,6 +146,25 @@ If the issue persists, additional NCCL fallbacks that may help are:
     export NCCL_IB_DISABLE=1
     export NCCL_ALGO=Ring
 
+On some multi-GPU systems, distributed training with RTX rendering enabled (for example,
+camera-based tasks) fails when the participating GPUs span
+multiple NUMA nodes, while the same training runs fine on GPUs attached to a single PCIe
+switch. The failure typically surfaces as a hang or abort on one of the first collectives,
+with ``Watchdog caught collective operation timeout: WorkNCCL(..., OpType=BROADCAST, ...)``
+or ``OpType=ALLREDUCE`` reported by ``ProcessGroupNCCL``, and does not occur when rendering
+is disabled. On affected systems, disabling NCCL's cuMem host-memory allocations resolves
+the failure:
+
+.. code-block:: shell
+
+    export NCCL_CUMEM_HOST_ENABLE=0
+
+If the failure persists, disable NCCL's CUDA virtual-memory-management allocator entirely:
+
+.. code-block:: shell
+
+    export NCCL_CUMEM_ENABLE=0
+
 Separately, restricting training to a subset of a node's GPUs with ``CUDA_VISIBLE_DEVICES``
 (for example, ``CUDA_VISIBLE_DEVICES=0,1`` on a larger machine) can cause training to hang during
 communicator initialization or on the first collective, with no error reported. On affected

source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst

@@ -0,0 +1,33 @@
+Fixed
+^^^^^
+
+* Fixed the abort-signal handler of :class:`~isaaclab.app.AppLauncher` re-entering
+  ``SimulationApp.close()`` when a signal arrived while the app was already closing.
+  The nested calls recursed until the stack overflowed, which prevented distributed
+  training workers from shutting down on ``SIGTERM`` (the launcher had to escalate to
+  ``SIGKILL``) and masked the failure behind a spurious segmentation fault. A
+  re-entrant signal now falls back to the default signal action. The ``atexit`` close
+  arms the same guard so a signal during a normal shutdown cannot start a nested
+  teardown.
+* Fixed signal-terminated processes reporting a successful exit status. Kit fast
+  shutdown terminated the process with exit code 0 from inside the graceful close, so
+  a ``SIGTERM``-ed worker was recorded as succeeded by distributed launchers. The
+  handler now closes the app with the conventional killed-by-signal status
+  (``close(exit_code=128 + signum)``; the app performs its full teardown before
+  exiting on ``isaacsim.simulation_app`` >= 2.18.5) and re-raises the signal with the
+  default action if the close returns.
+* Fixed ``Ctrl-C`` terminating the process with exit code 0 before ``finally`` blocks
+  or ``KeyboardInterrupt`` handlers in user code could run. ``SIGINT`` is restored to
+  Python's default handler; the app is still closed by the ``atexit`` callback.
+* Fixed the ``atexit`` close replacing the exit status of an unhandled exception with
+  a successful exit code 0 under Kit fast shutdown. The close now passes a failure
+  exit code when an exception is pending.
+
+Changed
+^^^^^^^
+
+* Changed :class:`~isaaclab.app.AppLauncher` to no longer intercept ``SIGSEGV`` at the
+  Python level. A Python handler cannot run for a synchronous segfault on the main
+  thread, reported a successful exit for segfaults on worker threads, and replaced the
+  carb crash reporter's handler. Native crashes now follow the default signal action,
+  so the crash reporter can produce minidumps again.

source/isaaclab/isaaclab/app/app_launcher.py

@@ -17,6 +17,7 @@
 import argparse
 import atexit
 import contextlib
+import inspect
 import logging
 import os
 import re
@@ -334,30 +335,13 @@ def __init__(self, launcher_args: argparse.Namespace | dict | None = None, **kwa
                 int(omni.timeline.TimelineEventType.PLAY), lambda e: self._hide_play_button(False)
             )
         )
-        # Signal to the CI test runner that Kit initialization is complete.
-        # stdout may be redirected to /dev/null during _create_app(), so we
-        # use __stderr__ which is never suppressed.
-        print("[ISAACLAB] AppLauncher initialization complete", file=sys.__stderr__, flush=True)
-
-        # Log Kit/runtime version information for diagnostics.
-        self._log_kit_version_info()
-
-        # Ensure SimulationApp.close() is called on normal process exit so Kit
-        # shuts down cleanly instead of relying on __del__ (which logs a warning
-        # and can leave GPU resources in a bad state for the next test).
-        def _atexit_close(app=self._app):
-            with contextlib.suppress(Exception):
-                app.close()
-
-        atexit.register(_atexit_close)
-
-        # Set up signal handlers for graceful shutdown
-        # -- during explicit `kill` commands
-        signal.signal(signal.SIGTERM, self._abort_signal_handle_callback)
-        # -- during aborts
-        signal.signal(signal.SIGABRT, self._abort_signal_handle_callback)
-        # -- during segfaults
-        signal.signal(signal.SIGSEGV, self._abort_signal_handle_callback)
+        # Report this process's lifecycle to whatever supervises it: the CI runner watches
+        # the startup announcements, and distributed launchers/schedulers/CI read the exit
+        # status. See the class docstring of :class:`_SimulationAppLifecycle` for the full
+        # exit-path policy and its rationale.
+        self._lifecycle = AppLauncher._SimulationAppLifecycle(self._app)
+        self._lifecycle.announce_startup()
+        self._lifecycle.install_exit_handlers()
 
     """
     Properties.
@@ -1317,13 +1301,6 @@ def _set_visualizer_settings(self, launcher_args: dict) -> None:
             }
         )
 
-    def _interrupt_signal_handle_callback(self, signal, frame):
-        """Handle the interrupt signal from the keyboard."""
-        # close the app
-        self._app.close()
-        # raise the error for keyboard interrupt
-        raise KeyboardInterrupt
-
     def is_isaac_sim_version_5(self) -> bool:
         if not hasattr(self, "_is_sim_ver_5"):
             # 1) Try to read the VERSION file (for manual / binary installs)
@@ -1365,23 +1342,109 @@ def _hide_play_button(self, flag):
                 play_button_group._play_button.visible = not flag  # type: ignore
                 play_button_group._play_button.enabled = not flag  # type: ignore
 
-    def _log_kit_version_info(self):
-        """Log Kit and runtime version information."""
-        import carb
-        import omni.kit.app
-
-        app = omni.kit.app.get_app()
-        tokens = carb.tokens.get_tokens_interface()
-
-        kit_version = app.get_kit_version()
-        kernel_version = app.get_kernel_version()
-        kit_git_hash = tokens.resolve("${kit_git_hash}") or "unknown"
-
-        print(f"[ISAACLAB] Kit version: {kit_version}", file=sys.__stderr__, flush=True)
-        print(f"[ISAACLAB] Kit kernel:  {kernel_version}", file=sys.__stderr__, flush=True)
-        print(f"[ISAACLAB] Kit hash:    {kit_git_hash}", file=sys.__stderr__, flush=True)
+    class _SimulationAppLifecycle:
+        """Reports the lifecycle of the Kit-based :class:`SimulationApp` process.
+
+        Supervisors (the CI runner at startup; torchrun, schedulers, and CI at exit)
+        only see this process through its output markers and its exit status. Kit
+        fast shutdown exits with code 0 from inside :meth:`SimulationApp.close`, so
+        every exit path below must carry its own truthful status.
+
+        Exit-path policy:
+
+        * Normal exit: the ``atexit`` hook closes the app once; a pending unhandled
+          exception turns into exit code 1.
+        * ``SIGTERM`` / ``kill -ABRT``: ``close(exit_code=128 + signum)`` — full
+          teardown before exiting on isaacsim.simulation_app >= 2.18.5, truthful
+          status either way; if ``close()`` returns (fast shutdown disabled),
+          re-raise the signal with the default action.
+        * Signal during a running close: fall back to the default action instead of
+          re-entering ``close()``, which previously recursed until stack overflow.
+        * ``SIGSEGV``: not intercepted — a Python handler cannot run for main-thread
+          faults, reports success for worker-thread faults, and suppresses the carb
+          crash reporter's minidumps.
+        * ``SIGINT``: Python's default handler, so Ctrl-C unwinds user code and
+          exits nonzero.
+
+        Not handled on the sim side, so handled here: SimulationApp installs its own
+        ``SIGINT`` handler that exits 0 before user code unwinds (changing it there
+        affects Ctrl-C semantics for every consumer), so the launcher restores
+        Python's default handler for its own processes.
+        """
 
-    def _abort_signal_handle_callback(self, signal, frame):
-        """Handle the abort/segmentation/kill signals."""
-        # close the app
-        self._app.close()
+        def __init__(self, app: SimulationApp):
+            self._app = app
+            # set once any close starts; later signals must not start a second teardown
+            self._closing = False
+
+        def announce_startup(self):
+            """Print the CI startup marker and Kit version diagnostics."""
+            import carb
+            import omni.kit.app
+
+            # the CI runner greps this marker for hang detection; __stderr__ survives
+            # stdout being redirected to /dev/null during app creation
+            print("[ISAACLAB] AppLauncher initialization complete", file=sys.__stderr__, flush=True)
+
+            kit_app = omni.kit.app.get_app()
+            tokens = carb.tokens.get_tokens_interface()
+            kit_git_hash = tokens.resolve("${kit_git_hash}") or "unknown"
+            print(f"[ISAACLAB] Kit version: {kit_app.get_kit_version()}", file=sys.__stderr__, flush=True)
+            print(f"[ISAACLAB] Kit kernel:  {kit_app.get_kernel_version()}", file=sys.__stderr__, flush=True)
+            print(f"[ISAACLAB] Kit hash:    {kit_git_hash}", file=sys.__stderr__, flush=True)
+
+        def install_exit_handlers(self):
+            """Register the ``atexit`` hook and signal handlers implementing the exit policy."""
+            # close on normal exit so Kit shuts down cleanly instead of via __del__
+            atexit.register(self._close_at_exit)
+            signal.signal(signal.SIGTERM, self._on_abort_signal)
+            signal.signal(signal.SIGABRT, self._on_abort_signal)
+            # no SIGSEGV handler and default SIGINT — see the class docstring
+            signal.signal(signal.SIGINT, signal.default_int_handler)
+
+        def _close_at_exit(self):
+            """Close the app on normal interpreter exit, preserving a pending failure status."""
+            self._closing = True
+            with contextlib.suppress(Exception):
+                # sys.last_exc is set before atexit callbacks run for an unhandled
+                # exception (but not for SystemExit, which is not detected yet)
+                exit_code = 1 if getattr(sys, "last_exc", None) is not None else 0
+                self._close_app(exit_code)
+
+        def _on_abort_signal(self, signum, frame):
+            """Handle SIGTERM/SIGABRT: close the app once, then die by the signal."""
+            if self._closing:
+                signal.signal(signum, signal.SIG_DFL)
+                signal.raise_signal(signum)
+                return
+            self._closing = True
+            with contextlib.suppress(Exception):
+                # >= 2.18.5 tears down fully, then exits with this status; older
+                # builds exit with the status but skip the teardown
+                self._close_app(128 + signum)
+            # close() only returns when fast shutdown is disabled
+            signal.signal(signum, signal.SIG_DFL)
+            signal.raise_signal(signum)
+
+        def _close_app(self, exit_code):
+            """Close the app with ``exit_code``, warning loudly if the parameter is unsupported."""
+            # Probe the signature instead of catching ``TypeError`` from the call: a
+            # ``TypeError`` raised *inside* ``close()``'s own teardown would otherwise be
+            # misread as an unsupported ``exit_code`` and trigger a second ``close()`` on a
+            # half-torn-down app — the exact re-entrancy this class exists to prevent.
+            try:
+                accepts_exit_code = "exit_code" in inspect.signature(self._app.close).parameters
+            except (TypeError, ValueError):
+                # Signature could not be introspected (e.g. a C-bound close); fall back to
+                # the no-argument close so the app still shuts down cleanly.
+                accepts_exit_code = False
+            if accepts_exit_code:
+                self._app.close(exit_code=exit_code)
+            else:
+                print(
+                    "[ISAACLAB] WARNING: SimulationApp.close() does not accept exit_code;"
+                    " this process may report exit code 0 instead of its failure status.",
+                    file=sys.__stderr__,
+                    flush=True,
+                )
+                self._app.close()

source/isaaclab/test/app/test_app_launcher_exit_status.py

@@ -0,0 +1,131 @@
+# 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
+
+"""Verify that a real AppLauncher process reports a truthful exit status.
+
+Each test launches a headless CPU :class:`~isaaclab.app.AppLauncher` in a child
+process and asserts on how the process ends: killed by a signal, or failed with
+an unhandled exception. Kit fast shutdown previously replaced both outcomes
+with a successful exit code 0.
+"""
+
+import os
+import signal
+import subprocess
+import sys
+import threading
+import time
+
+import pytest
+
+_TRIGGER_WAIT_FOR_SIGTERM = "--wait-for-sigterm"
+_TRIGGER_UNHANDLED_EXCEPTION = "--trigger-unhandled-exception"
+_READY_MARKER = "SIGTERM_TEST_READY"
+
+
+def _idle_after_app_launcher_initialization() -> None:
+    from isaaclab.app import AppLauncher
+
+    AppLauncher(headless=True, device="cpu")
+    print(_READY_MARKER, flush=True)
+    while True:
+        time.sleep(0.5)
+
+
+def _raise_after_app_launcher_initialization() -> None:
+    from isaaclab.app import AppLauncher
+
+    AppLauncher(headless=True, device="cpu")
+    raise RuntimeError("intentional AppLauncher failure")
+
+
+def _wait_for_ready_marker(proc: subprocess.Popen, timeout: float) -> None:
+    """Wait until the child prints the ready marker, failing on timeout or early child exit.
+
+    A reader thread consumes the child's stdout so the blocking line iteration cannot
+    outlive the deadline: the test waits on an event with a timeout instead of on the
+    pipe itself. On failure the collected child output is included in the report.
+    """
+    lines: list[str] = []
+    stdout_settled = threading.Event()  # marker seen or EOF
+    marker_seen = False
+
+    def _drain_stdout() -> None:
+        nonlocal marker_seen
+        for line in proc.stdout:
+            lines.append(line)
+            if _READY_MARKER in line:
+                marker_seen = True
+                break
+        stdout_settled.set()
+
+    reader = threading.Thread(target=_drain_stdout, daemon=True)
+    reader.start()
+    settled = stdout_settled.wait(timeout)
+    if not settled or not marker_seen:
+        proc.kill()
+        proc.wait(timeout=30)
+        reader.join(timeout=30)
+        stderr = proc.stderr.read()
+        reason = "did not become ready in time" if not settled else "exited before becoming ready"
+        pytest.fail(
+            f"AppLauncher child {reason}.\n--- child stdout ---\n{''.join(lines)}\n--- child stderr ---\n{stderr}"
+        )
+
+
+@pytest.mark.integration
+def test_sigterm_reports_killed_by_signal_status():
+    """Verify that SIGTERM tears the app down once and the process dies by SIGTERM.
+
+    Regression test for two defects: a signal received during ``SimulationApp.close()``
+    re-entered the abort handler and recursed until the stack overflowed, and the
+    graceful close terminated the process with exit code 0 under Kit fast shutdown, so
+    a SIGTERM-ed distributed worker was recorded as successful.
+    """
+    proc = subprocess.Popen(
+        [sys.executable, __file__, _TRIGGER_WAIT_FOR_SIGTERM],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        text=True,
+    )
+    try:
+        # wait for the app to finish starting up, without blocking past the deadline
+        _wait_for_ready_marker(proc, timeout=300)
+        proc.send_signal(signal.SIGTERM)
+        proc.stdout.close()
+        _, stderr = proc.communicate(timeout=300)
+    finally:
+        if proc.poll() is None:
+            proc.kill()
+
+    # the process must report the termination truthfully: exit status 128 + SIGTERM when
+    # fast shutdown exits inside close(exit_code=...), or death by SIGTERM when close()
+    # returns and the handler re-raises. Either way, never a successful exit.
+    assert proc.returncode in (128 + signal.SIGTERM, -signal.SIGTERM), f"returncode={proc.returncode}\n{stderr}"
+    # the teardown must not recurse through the abort handler
+    assert "_on_abort_signal" not in stderr
+
+
+@pytest.mark.integration
+def test_unhandled_exception_exits_with_failure():
+    """Verify that AppLauncher shutdown does not replace an exception's exit status with zero."""
+    result = subprocess.run(
+        [sys.executable, __file__, _TRIGGER_UNHANDLED_EXCEPTION],
+        capture_output=True,
+        text=True,
+        timeout=120,
+    )
+
+    assert result.returncode == 1, result.stdout + result.stderr
+    assert "RuntimeError: intentional AppLauncher failure" in result.stderr
+
+
+if __name__ == "__main__":
+    if _TRIGGER_WAIT_FOR_SIGTERM in sys.argv:
+        # detach from pytest's process group so only the explicit SIGTERM reaches us
+        os.setpgrp()
+        _idle_after_app_launcher_initialization()
+    elif _TRIGGER_UNHANDLED_EXCEPTION in sys.argv:
+        _raise_after_app_launcher_initialization()