From 5139ee43111e39c7446813e57921ce41b7546eea Mon Sep 17 00:00:00 2001 From: jichuanh Date: Sun, 19 Jul 2026 23:19:01 -0700 Subject: [PATCH 01/14] Fix abort-signal handler re-entering SimulationApp.close The AppLauncher abort handler for SIGTERM/SIGABRT/SIGSEGV calls SimulationApp.close() directly. When another signal is delivered while close() is already running -- for example a repeated SIGTERM from a distributed launcher tearing down workers, or a fault raised inside close() itself -- the handler re-enters and calls close() again. The nested calls recurse until the interpreter stack overflows, so the process neither shuts down cleanly nor reports the original signal. On multi-GPU training this surfaced as workers that ignored SIGTERM and had to be escalated to SIGKILL, and as a spurious segmentation fault that masked the real first failure behind the stack overflow. Guard the handler with a re-entrancy flag: the first signal closes the app once, and any signal that arrives while closing falls back to the default action and re-raises, so the process terminates with the original signal. Add a kitless regression test that drives the handler with a close() that re-signals, asserting close() runs exactly once and the re-entrant signal falls back to SIG_DFL. --- .../fix-abort-handler-reentrancy.rst | 10 ++++ source/isaaclab/isaaclab/app/app_launcher.py | 13 ++++- .../isaaclab/test/app/test_signal_handlers.py | 49 +++++++++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst create mode 100644 source/isaaclab/test/app/test_signal_handlers.py diff --git a/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst b/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst new file mode 100644 index 000000000000..eb3058602b04 --- /dev/null +++ b/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst @@ -0,0 +1,10 @@ +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 original failure signal behind a spurious segmentation + fault. A re-entrant signal now falls back to the default signal action so the + process terminates with the original signal. diff --git a/source/isaaclab/isaaclab/app/app_launcher.py b/source/isaaclab/isaaclab/app/app_launcher.py index fdb3f321900b..9123e79c9d04 100644 --- a/source/isaaclab/isaaclab/app/app_launcher.py +++ b/source/isaaclab/isaaclab/app/app_launcher.py @@ -331,6 +331,7 @@ def _atexit_close(app=self._app): atexit.register(_atexit_close) # Set up signal handlers for graceful shutdown + self._abort_in_progress = False # -- during explicit `kill` commands signal.signal(signal.SIGTERM, self._abort_signal_handle_callback) # -- during aborts @@ -1433,7 +1434,17 @@ def _log_kit_version_info(self): 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) - def _abort_signal_handle_callback(self, signal, frame): + def _abort_signal_handle_callback(self, signum, frame): """Handle the abort/segmentation/kill signals.""" + # A signal delivered while the app is already closing (e.g. a repeated SIGTERM from a + # distributed launcher, or a fault raised inside :meth:`SimulationApp.close`) must not + # re-enter `close()`: the nested calls recurse until the stack overflows, so the process + # neither shuts down cleanly nor reports the original failure. Fall back to the default + # signal action so the process terminates with the original signal. + if self._abort_in_progress: + signal.signal(signum, signal.SIG_DFL) + signal.raise_signal(signum) + return + self._abort_in_progress = True # close the app self._app.close() diff --git a/source/isaaclab/test/app/test_signal_handlers.py b/source/isaaclab/test/app/test_signal_handlers.py new file mode 100644 index 000000000000..392d0da4e304 --- /dev/null +++ b/source/isaaclab/test/app/test_signal_handlers.py @@ -0,0 +1,49 @@ +# 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 + +"""Kitless unit tests for the AppLauncher signal handlers.""" + +import signal +import types + +from isaaclab.app.app_launcher import AppLauncher + + +def test_abort_handler_closes_app_once_on_reentrant_signal(monkeypatch): + """A signal delivered while ``close()`` is running must not re-enter ``close()``. + + Regression test for the infinite recursion where a SIGTERM received during + ``SimulationApp.close()`` re-entered the abort handler, recursing until the + stack overflowed and the process had to be SIGKILL-ed. + """ + close_calls = [] + fallback_actions = [] + + launcher = types.SimpleNamespace(_abort_in_progress=False) + + def _close(): + close_calls.append(len(close_calls) + 1) + # Simulate further signals delivered while the app is still closing. + if len(close_calls) < 5: + AppLauncher._abort_signal_handle_callback(launcher, signal.SIGTERM, None) + + launcher._app = types.SimpleNamespace(close=_close) + + monkeypatch.setattr( + "isaaclab.app.app_launcher.signal.signal", + lambda signum, handler: fallback_actions.append(("set_handler", signum, handler)), + ) + monkeypatch.setattr( + "isaaclab.app.app_launcher.signal.raise_signal", + lambda signum: fallback_actions.append(("raise", signum)), + ) + + AppLauncher._abort_signal_handle_callback(launcher, signal.SIGTERM, None) + + # the app must be closed exactly once + assert close_calls == [1] + # the re-entrant signal must fall back to the default action and re-raise + assert ("set_handler", signal.SIGTERM, signal.SIG_DFL) in fallback_actions + assert ("raise", signal.SIGTERM) in fallback_actions From a68f65b20a740e8f6235973421c816be48122811 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Sun, 19 Jul 2026 23:52:33 -0700 Subject: [PATCH 02/14] Document NCCL_CUMEM_ENABLE=0 workaround for multi-GPU RTX training Multi-GPU training with RTX rendering enabled fails on some systems when the participating GPUs span multiple NUMA nodes: one of the first collectives times out (NCCL watchdog on an early BROADCAST/ALLREDUCE) while the same training passes on a single-switch GPU set, and while rendering-disabled training passes on any GPU set. Disabling NCCL's cuMem allocator restores these runs. Add the symptom signature and workaround to the existing NCCL troubleshooting section of the multi-GPU documentation. --- docs/source/features/multi_gpu.rst | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/source/features/multi_gpu.rst b/docs/source/features/multi_gpu.rst index a6f05083af8c..2940cfffb831 100644 --- a/docs/source/features/multi_gpu.rst +++ b/docs/source/features/multi_gpu.rst @@ -146,6 +146,19 @@ 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 launched with ``--enable_cameras``) 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 CUDA virtual-memory-management allocator +resolves the failure: + +.. 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 From b730e839b1155f650055428305c460e66c3485d3 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Mon, 20 Jul 2026 14:37:30 -0700 Subject: [PATCH 03/14] Make AppLauncher teardown truthful across all signal paths Extend the re-entrancy fix into a complete teardown-correctness change based on a combined review with the atexit exit-code fix (#6634): - Report killed-by-signal status: the handler previously ran a graceful close whose Kit fast-shutdown path terminated the process with exit code 0, so a SIGTERM-ed worker was recorded as successful and distributed launchers misattributed the failure. The handler now disables fast shutdown so close() performs the full teardown and returns, then re-raises the signal with the default action so the process exits with the conventional 128+signum status. The override is marked WORKAROUND(isaac-sim) for removal once SimulationApp can propagate a nonzero exit status through its fast-shutdown path. - Arm the re-entrancy guard in the atexit close (extracted to the testable _close_app_at_exit method) so a signal arriving during a normal shutdown takes the guarded path instead of starting a nested teardown of a half-closed app. - Stop intercepting SIGSEGV: a Python handler never runs for a synchronous main-thread segfault (the process spins on signal delivery), reports success for worker-thread segfaults, and replaces the carb crash reporter's handler, suppressing minidumps. - Restore Python's default SIGINT handler over SimulationApp's, which exits 0 before user finally blocks or KeyboardInterrupt handlers can run. Marked WORKAROUND(isaac-sim) for removal once the upstream handler preserves exception semantics and a nonzero exit status. - Remove the unregistered, dead _interrupt_signal_handle_callback. Tests cover the single-close-plus-reraise contract, the re-entrant signal path, and the atexit guard arming; all three fail against the previous behavior. --- .../fix-abort-handler-reentrancy.rst | 24 ++++- source/isaaclab/isaaclab/app/app_launcher.py | 69 ++++++++++---- .../isaaclab/test/app/test_signal_handlers.py | 89 +++++++++++++++---- 3 files changed, 142 insertions(+), 40 deletions(-) diff --git a/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst b/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst index eb3058602b04..8684b7a849e4 100644 --- a/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst +++ b/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst @@ -5,6 +5,24 @@ Fixed ``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 original failure signal behind a spurious segmentation - fault. A re-entrant signal now falls back to the default signal action so the - process terminates with the original signal. + ``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 performs the full teardown and re-raises the signal with the default + action, so the process exits with the conventional killed-by-signal status. +* 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. + +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. diff --git a/source/isaaclab/isaaclab/app/app_launcher.py b/source/isaaclab/isaaclab/app/app_launcher.py index 9123e79c9d04..dbade4d2c18a 100644 --- a/source/isaaclab/isaaclab/app/app_launcher.py +++ b/source/isaaclab/isaaclab/app/app_launcher.py @@ -324,20 +324,31 @@ def __init__(self, launcher_args: argparse.Namespace | dict | None = None, **kwa # 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) + atexit.register(self._close_app_at_exit) # Set up signal handlers for graceful shutdown self._abort_in_progress = False # -- during explicit `kill` commands signal.signal(signal.SIGTERM, self._abort_signal_handle_callback) - # -- during aborts + # -- during aborts raised as signals (e.g. ``kill -ABRT``). Native ``abort()`` calls + # restore the default action before re-raising, so they terminate the process + # without entering this handler. signal.signal(signal.SIGABRT, self._abort_signal_handle_callback) - # -- during segfaults - signal.signal(signal.SIGSEGV, self._abort_signal_handle_callback) + # NOTE: SIGSEGV is intentionally not handled at the Python level. For a synchronous + # segfault on the main thread the faulting instruction re-executes as soon as the + # C-level trampoline returns, so a Python handler never runs and the process spins on + # signal delivery forever. For a segfault on a worker thread the handler would run on + # the main thread and report a successful exit for a crashed process. Registering a + # handler here would also replace the carb crash reporter's handler and suppress + # minidumps for real crashes. + # WORKAROUND(isaac-sim): SimulationApp installs a SIGINT handler that terminates the + # process with exit code 0 before ``finally`` blocks or ``KeyboardInterrupt`` handlers + # in user code can run. Restore Python's default behavior so Ctrl-C raises + # :class:`KeyboardInterrupt`, unwinds user code, and exits with a failure status; the + # app is still closed by the ``atexit`` callback above. Remove this override once + # SimulationApp's SIGINT handler preserves exception semantics and a nonzero exit + # status. + signal.signal(signal.SIGINT, signal.default_int_handler) """ Properties. @@ -1370,13 +1381,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) @@ -1434,17 +1438,44 @@ def _log_kit_version_info(self): 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) + def _close_app_at_exit(self): + """Close the app on normal interpreter exit.""" + # A signal that arrives while this close is running must take the re-entrant path of + # :meth:`_abort_signal_handle_callback` instead of starting a second, nested teardown + # of a half-closed app. + self._abort_in_progress = True + with contextlib.suppress(Exception): + self._app.close() + def _abort_signal_handle_callback(self, signum, frame): - """Handle the abort/segmentation/kill signals.""" + """Handle the abort/termination signals.""" # A signal delivered while the app is already closing (e.g. a repeated SIGTERM from a # distributed launcher, or a fault raised inside :meth:`SimulationApp.close`) must not # re-enter `close()`: the nested calls recurse until the stack overflows, so the process - # neither shuts down cleanly nor reports the original failure. Fall back to the default - # signal action so the process terminates with the original signal. + # neither shuts down cleanly nor reports a meaningful failure. Fall back to the default + # signal action so the process terminates with the re-entrant signal. if self._abort_in_progress: signal.signal(signum, signal.SIG_DFL) signal.raise_signal(signum) return self._abort_in_progress = True + # WORKAROUND(isaac-sim): under Kit fast shutdown, :meth:`SimulationApp.close` terminates + # the process from inside ``app.shutdown()`` with exit code 0, so a signal-terminated + # worker would report success (a distributed launcher then marks the rank as succeeded + # and the remaining ranks block until the collective watchdog fires). Disable fast + # shutdown so `close()` performs the full teardown and returns control here. Remove the + # override once SimulationApp can propagate a nonzero exit status through its + # fast-shutdown path. + with contextlib.suppress(Exception): + import carb.settings + + carb.settings.get_settings().set("/app/fastShutdown", False) + with contextlib.suppress(Exception): + self._app.config["fast_shutdown"] = False # close the app - self._app.close() + with contextlib.suppress(Exception): + self._app.close() + # Re-raise with the default action so the process exits with the conventional + # killed-by-signal status (128 + signum) instead of reporting success. + signal.signal(signum, signal.SIG_DFL) + signal.raise_signal(signum) diff --git a/source/isaaclab/test/app/test_signal_handlers.py b/source/isaaclab/test/app/test_signal_handlers.py index 392d0da4e304..d460186f6ee0 100644 --- a/source/isaaclab/test/app/test_signal_handlers.py +++ b/source/isaaclab/test/app/test_signal_handlers.py @@ -11,7 +11,46 @@ from isaaclab.app.app_launcher import AppLauncher -def test_abort_handler_closes_app_once_on_reentrant_signal(monkeypatch): +def _make_launcher(close_fn): + launcher = types.SimpleNamespace(_abort_in_progress=False) + launcher._app = types.SimpleNamespace(close=close_fn, config={"fast_shutdown": True}) + return launcher + + +def _capture_signal_actions(monkeypatch): + actions = [] + monkeypatch.setattr( + "isaaclab.app.app_launcher.signal.signal", + lambda signum, handler: actions.append(("set_handler", signum, handler)), + ) + monkeypatch.setattr( + "isaaclab.app.app_launcher.signal.raise_signal", + lambda signum: actions.append(("raise", signum)), + ) + return actions + + +def test_abort_handler_closes_once_and_reports_killed_by_signal(monkeypatch): + """The first signal closes the app once, then re-raises with the default action. + + The re-raise is what makes the process exit with the conventional 128 + signum + status instead of the exit code 0 that Kit fast shutdown reports. + """ + close_calls = [] + launcher = _make_launcher(lambda: close_calls.append(len(close_calls) + 1)) + actions = _capture_signal_actions(monkeypatch) + + AppLauncher._abort_signal_handle_callback(launcher, signal.SIGTERM, None) + + # the app must be closed exactly once, with fast shutdown disabled beforehand + assert close_calls == [1] + assert launcher._app.config["fast_shutdown"] is False + # the handler must hand the signal back to the default action to preserve the status + assert ("set_handler", signal.SIGTERM, signal.SIG_DFL) in actions + assert ("raise", signal.SIGTERM) in actions + + +def test_abort_handler_reentrant_signal_skips_nested_close(monkeypatch): """A signal delivered while ``close()`` is running must not re-enter ``close()``. Regression test for the infinite recursion where a SIGTERM received during @@ -19,9 +58,6 @@ def test_abort_handler_closes_app_once_on_reentrant_signal(monkeypatch): stack overflowed and the process had to be SIGKILL-ed. """ close_calls = [] - fallback_actions = [] - - launcher = types.SimpleNamespace(_abort_in_progress=False) def _close(): close_calls.append(len(close_calls) + 1) @@ -29,21 +65,38 @@ def _close(): if len(close_calls) < 5: AppLauncher._abort_signal_handle_callback(launcher, signal.SIGTERM, None) - launcher._app = types.SimpleNamespace(close=_close) - - monkeypatch.setattr( - "isaaclab.app.app_launcher.signal.signal", - lambda signum, handler: fallback_actions.append(("set_handler", signum, handler)), - ) - monkeypatch.setattr( - "isaaclab.app.app_launcher.signal.raise_signal", - lambda signum: fallback_actions.append(("raise", signum)), - ) + launcher = _make_launcher(_close) + actions = _capture_signal_actions(monkeypatch) AppLauncher._abort_signal_handle_callback(launcher, signal.SIGTERM, None) - # the app must be closed exactly once + # the app must be closed exactly once despite the nested signal + assert close_calls == [1] + # both the nested and the outer path fall back to the default action and re-raise + assert actions.count(("set_handler", signal.SIGTERM, signal.SIG_DFL)) >= 1 + assert actions.count(("raise", signal.SIGTERM)) >= 1 + + +def test_atexit_close_arms_reentrancy_guard(monkeypatch): + """A signal arriving during the atexit close must take the re-entrant path. + + The atexit callback arms the same guard flag as the signal handler, so a SIGTERM + delivered mid-close falls back to the default signal action instead of starting a + second, nested teardown. + """ + close_calls = [] + + def _close(): + close_calls.append(len(close_calls) + 1) + # Simulate a signal delivered while the atexit close is running. + AppLauncher._abort_signal_handle_callback(launcher, signal.SIGTERM, None) + + launcher = _make_launcher(_close) + actions = _capture_signal_actions(monkeypatch) + + AppLauncher._close_app_at_exit(launcher) + + # the guard must be armed before close, so the mid-close signal must not re-enter close() assert close_calls == [1] - # the re-entrant signal must fall back to the default action and re-raise - assert ("set_handler", signal.SIGTERM, signal.SIG_DFL) in fallback_actions - assert ("raise", signal.SIGTERM) in fallback_actions + assert ("set_handler", signal.SIGTERM, signal.SIG_DFL) in actions + assert ("raise", signal.SIGTERM) in actions From c2d43f310dc530c47c37f4b1385df860d4f476b4 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Mon, 20 Jul 2026 14:45:13 -0700 Subject: [PATCH 04/14] Add real-Kit integration test for SIGTERM teardown status Launch a headless CPU AppLauncher in a child process, send SIGTERM once the app is ready, and assert the process dies by SIGTERM instead of reporting a successful exit, with no abort-handler recursion in stderr. Verified against the previous behavior: without the handler fix the child exits 0 (Kit fast shutdown swallows the termination status); with the fix it reports killed-by-SIGTERM. --- .../test/app/test_app_launcher_sigterm.py | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 source/isaaclab/test/app/test_app_launcher_sigterm.py diff --git a/source/isaaclab/test/app/test_app_launcher_sigterm.py b/source/isaaclab/test/app/test_app_launcher_sigterm.py new file mode 100644 index 000000000000..8da5e4f58d41 --- /dev/null +++ b/source/isaaclab/test/app/test_app_launcher_sigterm.py @@ -0,0 +1,68 @@ +# 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 SIGTERM-ed AppLauncher process reports a killed-by-signal status.""" + +import os +import signal +import subprocess +import sys +import time + +import pytest + +_TRIGGER_WAIT_FOR_SIGTERM = "--wait-for-sigterm" +_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) + + +@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 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 + deadline = time.time() + 300 + for line in proc.stdout: + if _READY_MARKER in line: + break + if time.time() > deadline: + pytest.fail("AppLauncher child did not become ready in time") + proc.send_signal(signal.SIGTERM) + proc.stdout.close() + _, stderr = proc.communicate(timeout=300) + finally: + if proc.poll() is None: + proc.kill() + + # the process must die by SIGTERM, not report a successful exit + assert proc.returncode == -signal.SIGTERM, f"returncode={proc.returncode}\n{stderr}" + # the teardown must not recurse through the abort handler + assert "_abort_signal_handle_callback" not in stderr + + +if __name__ == "__main__" and _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() From ed0509cecfc27fca08fd1d8b40d0abc54a9610e0 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Mon, 20 Jul 2026 15:53:25 -0700 Subject: [PATCH 05/14] Prefer NCCL_CUMEM_HOST_ENABLE=0 in the mGPU NCCL troubleshooting docs Paired experiments on the deterministic cross-NUMA reproduction show the narrower knob is sufficient: the same GPU set that fails flag-free passes with only cuMem host allocations disabled. Recommend it first, with the full cuMem allocator disable as the fallback, so affected systems keep the device-side allocator benefits. --- docs/source/features/multi_gpu.rst | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/source/features/multi_gpu.rst b/docs/source/features/multi_gpu.rst index 2940cfffb831..099c6cb8f568 100644 --- a/docs/source/features/multi_gpu.rst +++ b/docs/source/features/multi_gpu.rst @@ -152,8 +152,14 @@ multiple NUMA nodes, while the same training runs fine on GPUs attached to a sin 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 CUDA virtual-memory-management allocator -resolves the failure: +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 From 83c735f5f93c08f6ed2a9f757adae01cce72d1f4 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Mon, 20 Jul 2026 16:54:16 -0700 Subject: [PATCH 06/14] Consolidate process-lifecycle handling into one class Gather the startup announcements (CI marker, Kit version diagnostics) and the entire exit-path policy (atexit close, signal handlers, exit codes) into a nested AppLauncher._SimulationAppLifecycle class. The pieces coordinate through one guard flag and exist for one reason -- report the process state truthfully to whatever supervises it -- so a single class with the policy table as its docstring replaces logic previously spread across __init__ and three private methods. Absorb the atexit exit-code fix from PR #6634 (nblauch) into the lifecycle class: the atexit close passes a nonzero exit code when an unhandled exception is pending, so Kit fast shutdown does not replace the failure status with 0. Includes that PR's integration test and a kitless unit test for the exit-code selection. SystemExit is documented as not yet detected. Behavior is otherwise unchanged; all kitless unit tests and both real-Kit integration tests (SIGTERM status, exception exit code) pass. --- .../fix-abort-handler-reentrancy.rst | 3 + source/isaaclab/isaaclab/app/app_launcher.py | 208 ++++++++++-------- .../test/app/test_app_launcher_exit_code.py | 38 ++++ .../isaaclab/test/app/test_signal_handlers.py | 60 +++-- 4 files changed, 199 insertions(+), 110 deletions(-) create mode 100644 source/isaaclab/test/app/test_app_launcher_exit_code.py diff --git a/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst b/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst index 8684b7a849e4..87ed6d6a1dc7 100644 --- a/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst +++ b/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst @@ -17,6 +17,9 @@ Fixed * 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 ^^^^^^^ diff --git a/source/isaaclab/isaaclab/app/app_launcher.py b/source/isaaclab/isaaclab/app/app_launcher.py index dbade4d2c18a..e25136c87f7b 100644 --- a/source/isaaclab/isaaclab/app/app_launcher.py +++ b/source/isaaclab/isaaclab/app/app_launcher.py @@ -313,42 +313,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). - atexit.register(self._close_app_at_exit) - - # Set up signal handlers for graceful shutdown - self._abort_in_progress = False - # -- during explicit `kill` commands - signal.signal(signal.SIGTERM, self._abort_signal_handle_callback) - # -- during aborts raised as signals (e.g. ``kill -ABRT``). Native ``abort()`` calls - # restore the default action before re-raising, so they terminate the process - # without entering this handler. - signal.signal(signal.SIGABRT, self._abort_signal_handle_callback) - # NOTE: SIGSEGV is intentionally not handled at the Python level. For a synchronous - # segfault on the main thread the faulting instruction re-executes as soon as the - # C-level trampoline returns, so a Python handler never runs and the process spins on - # signal delivery forever. For a segfault on a worker thread the handler would run on - # the main thread and report a successful exit for a crashed process. Registering a - # handler here would also replace the carb crash reporter's handler and suppress - # minidumps for real crashes. - # WORKAROUND(isaac-sim): SimulationApp installs a SIGINT handler that terminates the - # process with exit code 0 before ``finally`` blocks or ``KeyboardInterrupt`` handlers - # in user code can run. Restore Python's default behavior so Ctrl-C raises - # :class:`KeyboardInterrupt`, unwinds user code, and exits with a failure status; the - # app is still closed by the ``atexit`` callback above. Remove this override once - # SimulationApp's SIGINT handler preserves exception semantics and a nonzero exit - # status. - signal.signal(signal.SIGINT, signal.default_int_handler) + # 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. @@ -1422,60 +1393,117 @@ 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. + + Everything a supervisor learns about this process flows through here: the CI + test runner watches the startup announcements, and distributed launchers, + schedulers, and CI read the exit status. The exit-path policy exists because + Kit fast shutdown terminates the process with exit code 0 from inside + :meth:`SimulationApp.close`, so any death funneled through an unqualified + ``close()`` is reported as success. + + Exit paths and policy: + + * Normal interpreter exit: the ``atexit`` hook closes the app once, passing a + nonzero exit code when an unhandled exception is pending so the failure + status survives Kit fast shutdown. + * ``SIGTERM`` and signal-delivered ``SIGABRT``: close once with full teardown, + then re-raise the signal with the default action so the process dies with + the conventional killed-by-signal status (128 + signum). Native ``abort()`` + calls bypass Python handlers and already terminate correctly. + * Any signal while a close is running: never re-enter ``close()`` (the nested + calls previously recursed until the stack overflowed, leaving workers that + only SIGKILL could stop); fall back to the default signal action. + * ``SIGSEGV``: intentionally not handled. A Python handler never runs for a + synchronous main-thread segfault (the process would spin on signal + delivery), reports a successful exit for worker-thread segfaults, and + replaces the carb crash reporter's handler, suppressing minidumps. + * ``SIGINT``: Python's default handler, so Ctrl-C raises + :class:`KeyboardInterrupt`, unwinds user code (``finally`` blocks run), and + exits with a failure status. + + WORKAROUND(isaac-sim): two pieces below exist only because of upstream + SimulationApp behavior and can be removed once it is fixed: fast shutdown is + disabled during a signal close (so ``close()`` returns and the + killed-by-signal status can be reported), and ``SIGINT`` is re-registered + over SimulationApp's own handler (which exits 0 before user code unwinds). + """ - def _close_app_at_exit(self): - """Close the app on normal interpreter exit.""" - # A signal that arrives while this close is running must take the re-entrant path of - # :meth:`_abort_signal_handle_callback` instead of starting a second, nested teardown - # of a half-closed app. - self._abort_in_progress = True - with contextlib.suppress(Exception): - self._app.close() - - def _abort_signal_handle_callback(self, signum, frame): - """Handle the abort/termination signals.""" - # A signal delivered while the app is already closing (e.g. a repeated SIGTERM from a - # distributed launcher, or a fault raised inside :meth:`SimulationApp.close`) must not - # re-enter `close()`: the nested calls recurse until the stack overflows, so the process - # neither shuts down cleanly nor reports a meaningful failure. Fall back to the default - # signal action so the process terminates with the re-entrant signal. - if self._abort_in_progress: + def __init__(self, app: SimulationApp): + self._app = app + # True once any close has started; a signal arriving after that must not + # start a second, nested teardown of a half-closed app. + self._closing = False + + def announce_startup(self): + """Print the CI startup marker and Kit version diagnostics.""" + import carb + import omni.kit.app + + # The CI test runner watches for this marker to distinguish a Kit that is + # still starting from one that hung. stdout may be redirected to /dev/null + # during app creation, so use __stderr__, which is never suppressed. + print("[ISAACLAB] AppLauncher initialization complete", file=sys.__stderr__, flush=True) + + kit_app = omni.kit.app.get_app() + tokens = carb.tokens.get_tokens_interface() + kit_version = kit_app.get_kit_version() + kernel_version = kit_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) + + def install_exit_handlers(self): + """Register the ``atexit`` hook and signal handlers implementing the exit policy.""" + # Close the app 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). + atexit.register(self._close_at_exit) + # Explicit `kill` and signal-delivered aborts take the truthful-close path. + signal.signal(signal.SIGTERM, self._on_abort_signal) + signal.signal(signal.SIGABRT, self._on_abort_signal) + # SIGSEGV is intentionally absent and SIGINT uses Python's default handler; + # the class docstring explains both decisions. + 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): + # The interpreter sets ``sys.last_exc`` before atexit callbacks run for + # an unhandled exception; pass a nonzero code so Kit fast shutdown does + # not replace the pending failure status with 0. Note that ``SystemExit`` + # (e.g. ``sys.exit(1)``) does not set ``sys.last_exc`` and is not yet + # detected here. + exit_code = 1 if getattr(sys, "last_exc", None) is not None else 0 + self._app.close(exit_code=exit_code) + + def _on_abort_signal(self, signum, frame): + """Handle SIGTERM/SIGABRT: close the app once, then die by the signal.""" + if self._closing: + # A signal during a running close must not re-enter ``close()``: the + # nested calls recurse until the stack overflows, so the process can + # neither shut down cleanly nor report a meaningful failure. + signal.signal(signum, signal.SIG_DFL) + signal.raise_signal(signum) + return + self._closing = True + # WORKAROUND(isaac-sim): under fast shutdown, ``close()`` terminates the + # process with exit code 0 from inside ``app.shutdown()``; a killed worker + # would then be recorded as successful by its launcher, and the remaining + # ranks would block until the collective watchdog fires. Disable fast + # shutdown so ``close()`` performs the full teardown and returns here. + with contextlib.suppress(Exception): + import carb.settings + + carb.settings.get_settings().set("/app/fastShutdown", False) + with contextlib.suppress(Exception): + self._app.config["fast_shutdown"] = False + with contextlib.suppress(Exception): + self._app.close() + # Die by the signal so the exit status is the conventional 128 + signum + # instead of reporting success. signal.signal(signum, signal.SIG_DFL) signal.raise_signal(signum) - return - self._abort_in_progress = True - # WORKAROUND(isaac-sim): under Kit fast shutdown, :meth:`SimulationApp.close` terminates - # the process from inside ``app.shutdown()`` with exit code 0, so a signal-terminated - # worker would report success (a distributed launcher then marks the rank as succeeded - # and the remaining ranks block until the collective watchdog fires). Disable fast - # shutdown so `close()` performs the full teardown and returns control here. Remove the - # override once SimulationApp can propagate a nonzero exit status through its - # fast-shutdown path. - with contextlib.suppress(Exception): - import carb.settings - - carb.settings.get_settings().set("/app/fastShutdown", False) - with contextlib.suppress(Exception): - self._app.config["fast_shutdown"] = False - # close the app - with contextlib.suppress(Exception): - self._app.close() - # Re-raise with the default action so the process exits with the conventional - # killed-by-signal status (128 + signum) instead of reporting success. - signal.signal(signum, signal.SIG_DFL) - signal.raise_signal(signum) diff --git a/source/isaaclab/test/app/test_app_launcher_exit_code.py b/source/isaaclab/test/app/test_app_launcher_exit_code.py new file mode 100644 index 000000000000..88adf1f1f516 --- /dev/null +++ b/source/isaaclab/test/app/test_app_launcher_exit_code.py @@ -0,0 +1,38 @@ +# 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 AppLauncher preserves the exit status of unhandled exceptions.""" + +import subprocess +import sys + +import pytest + +_TRIGGER_UNHANDLED_EXCEPTION = "--trigger-unhandled-exception" + + +def _raise_after_app_launcher_initialization() -> None: + from isaaclab.app import AppLauncher + + AppLauncher(headless=True, device="cpu") + raise RuntimeError("intentional AppLauncher failure") + + +@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__" and _TRIGGER_UNHANDLED_EXCEPTION in sys.argv: + _raise_after_app_launcher_initialization() diff --git a/source/isaaclab/test/app/test_signal_handlers.py b/source/isaaclab/test/app/test_signal_handlers.py index d460186f6ee0..65703130e08a 100644 --- a/source/isaaclab/test/app/test_signal_handlers.py +++ b/source/isaaclab/test/app/test_signal_handlers.py @@ -3,18 +3,18 @@ # # SPDX-License-Identifier: BSD-3-Clause -"""Kitless unit tests for the AppLauncher signal handlers.""" +"""Kitless unit tests for the AppLauncher process-lifecycle exit policy.""" import signal +import sys import types from isaaclab.app.app_launcher import AppLauncher -def _make_launcher(close_fn): - launcher = types.SimpleNamespace(_abort_in_progress=False) - launcher._app = types.SimpleNamespace(close=close_fn, config={"fast_shutdown": True}) - return launcher +def _make_lifecycle(close_fn): + app = types.SimpleNamespace(close=close_fn, config={"fast_shutdown": True}) + return AppLauncher._SimulationAppLifecycle(app) def _capture_signal_actions(monkeypatch): @@ -30,27 +30,27 @@ def _capture_signal_actions(monkeypatch): return actions -def test_abort_handler_closes_once_and_reports_killed_by_signal(monkeypatch): +def test_abort_signal_closes_once_and_reports_killed_by_signal(monkeypatch): """The first signal closes the app once, then re-raises with the default action. The re-raise is what makes the process exit with the conventional 128 + signum status instead of the exit code 0 that Kit fast shutdown reports. """ close_calls = [] - launcher = _make_launcher(lambda: close_calls.append(len(close_calls) + 1)) + lifecycle = _make_lifecycle(lambda: close_calls.append(len(close_calls) + 1)) actions = _capture_signal_actions(monkeypatch) - AppLauncher._abort_signal_handle_callback(launcher, signal.SIGTERM, None) + lifecycle._on_abort_signal(signal.SIGTERM, None) # the app must be closed exactly once, with fast shutdown disabled beforehand assert close_calls == [1] - assert launcher._app.config["fast_shutdown"] is False + assert lifecycle._app.config["fast_shutdown"] is False # the handler must hand the signal back to the default action to preserve the status assert ("set_handler", signal.SIGTERM, signal.SIG_DFL) in actions assert ("raise", signal.SIGTERM) in actions -def test_abort_handler_reentrant_signal_skips_nested_close(monkeypatch): +def test_abort_signal_reentrant_signal_skips_nested_close(monkeypatch): """A signal delivered while ``close()`` is running must not re-enter ``close()``. Regression test for the infinite recursion where a SIGTERM received during @@ -63,12 +63,12 @@ def _close(): close_calls.append(len(close_calls) + 1) # Simulate further signals delivered while the app is still closing. if len(close_calls) < 5: - AppLauncher._abort_signal_handle_callback(launcher, signal.SIGTERM, None) + lifecycle._on_abort_signal(signal.SIGTERM, None) - launcher = _make_launcher(_close) + lifecycle = _make_lifecycle(_close) actions = _capture_signal_actions(monkeypatch) - AppLauncher._abort_signal_handle_callback(launcher, signal.SIGTERM, None) + lifecycle._on_abort_signal(signal.SIGTERM, None) # the app must be closed exactly once despite the nested signal assert close_calls == [1] @@ -80,23 +80,43 @@ def _close(): def test_atexit_close_arms_reentrancy_guard(monkeypatch): """A signal arriving during the atexit close must take the re-entrant path. - The atexit callback arms the same guard flag as the signal handler, so a SIGTERM - delivered mid-close falls back to the default signal action instead of starting a - second, nested teardown. + The atexit hook arms the same guard flag as the signal handler, so a SIGTERM + delivered mid-close falls back to the default signal action instead of starting + a second, nested teardown. """ close_calls = [] - def _close(): + def _close(exit_code=0): close_calls.append(len(close_calls) + 1) # Simulate a signal delivered while the atexit close is running. - AppLauncher._abort_signal_handle_callback(launcher, signal.SIGTERM, None) + lifecycle._on_abort_signal(signal.SIGTERM, None) - launcher = _make_launcher(_close) + lifecycle = _make_lifecycle(_close) actions = _capture_signal_actions(monkeypatch) - AppLauncher._close_app_at_exit(launcher) + lifecycle._close_at_exit() # the guard must be armed before close, so the mid-close signal must not re-enter close() assert close_calls == [1] assert ("set_handler", signal.SIGTERM, signal.SIG_DFL) in actions assert ("raise", signal.SIGTERM) in actions + + +def test_atexit_close_preserves_pending_failure_status(monkeypatch): + """The atexit close passes a nonzero exit code when an unhandled exception is pending. + + Kit fast shutdown would otherwise replace the interpreter's pending failure + status with a successful exit code 0. + """ + codes = [] + lifecycle = _make_lifecycle(lambda exit_code=0: codes.append(exit_code)) + + # normal interpreter exit: no pending exception, close with exit code 0 + monkeypatch.delattr(sys, "last_exc", raising=False) + lifecycle._close_at_exit() + # unhandled exception pending: close with a failure exit code + lifecycle._closing = False + monkeypatch.setattr(sys, "last_exc", RuntimeError("pending failure"), raising=False) + lifecycle._close_at_exit() + + assert codes == [0, 1] From 36ec5d6cf96c6c0eaf5779b8acce4cd3b7e8f3cb Mon Sep 17 00:00:00 2001 From: jichuanh Date: Mon, 20 Jul 2026 17:00:03 -0700 Subject: [PATCH 07/14] Consolidate lifecycle tests into unit and integration files Merge the two single-test real-Kit integration files into test_app_launcher_exit_status.py: both launch a headless AppLauncher in a child process and assert on the exit outcome (killed by SIGTERM, failed with an unhandled exception), sharing one trigger harness. Rename the kitless unit file to test_simulation_app_lifecycle.py to match the class it exercises; its previous name predated the exit-code coverage. --- .../test/app/test_app_launcher_exit_code.py | 38 -------------- ...rm.py => test_app_launcher_exit_status.py} | 49 +++++++++++++++---- ...rs.py => test_simulation_app_lifecycle.py} | 0 3 files changed, 40 insertions(+), 47 deletions(-) delete mode 100644 source/isaaclab/test/app/test_app_launcher_exit_code.py rename source/isaaclab/test/app/{test_app_launcher_sigterm.py => test_app_launcher_exit_status.py} (50%) rename source/isaaclab/test/app/{test_signal_handlers.py => test_simulation_app_lifecycle.py} (100%) diff --git a/source/isaaclab/test/app/test_app_launcher_exit_code.py b/source/isaaclab/test/app/test_app_launcher_exit_code.py deleted file mode 100644 index 88adf1f1f516..000000000000 --- a/source/isaaclab/test/app/test_app_launcher_exit_code.py +++ /dev/null @@ -1,38 +0,0 @@ -# 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 AppLauncher preserves the exit status of unhandled exceptions.""" - -import subprocess -import sys - -import pytest - -_TRIGGER_UNHANDLED_EXCEPTION = "--trigger-unhandled-exception" - - -def _raise_after_app_launcher_initialization() -> None: - from isaaclab.app import AppLauncher - - AppLauncher(headless=True, device="cpu") - raise RuntimeError("intentional AppLauncher failure") - - -@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__" and _TRIGGER_UNHANDLED_EXCEPTION in sys.argv: - _raise_after_app_launcher_initialization() diff --git a/source/isaaclab/test/app/test_app_launcher_sigterm.py b/source/isaaclab/test/app/test_app_launcher_exit_status.py similarity index 50% rename from source/isaaclab/test/app/test_app_launcher_sigterm.py rename to source/isaaclab/test/app/test_app_launcher_exit_status.py index 8da5e4f58d41..0c58df4129c4 100644 --- a/source/isaaclab/test/app/test_app_launcher_sigterm.py +++ b/source/isaaclab/test/app/test_app_launcher_exit_status.py @@ -3,7 +3,13 @@ # # SPDX-License-Identifier: BSD-3-Clause -"""Verify that a SIGTERM-ed AppLauncher process reports a killed-by-signal status.""" +"""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 @@ -14,6 +20,7 @@ import pytest _TRIGGER_WAIT_FOR_SIGTERM = "--wait-for-sigterm" +_TRIGGER_UNHANDLED_EXCEPTION = "--trigger-unhandled-exception" _READY_MARKER = "SIGTERM_TEST_READY" @@ -26,14 +33,21 @@ def _idle_after_app_launcher_initialization() -> None: 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") + + @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 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. + 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], @@ -59,10 +73,27 @@ def test_sigterm_reports_killed_by_signal_status(): # the process must die by SIGTERM, not report a successful exit assert proc.returncode == -signal.SIGTERM, f"returncode={proc.returncode}\n{stderr}" # the teardown must not recurse through the abort handler - assert "_abort_signal_handle_callback" not in stderr + 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__" and _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() +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() diff --git a/source/isaaclab/test/app/test_signal_handlers.py b/source/isaaclab/test/app/test_simulation_app_lifecycle.py similarity index 100% rename from source/isaaclab/test/app/test_signal_handlers.py rename to source/isaaclab/test/app/test_simulation_app_lifecycle.py From 6d66142b3f569141bec6644e0a0068b599f286ac Mon Sep 17 00:00:00 2001 From: jichuanh Date: Tue, 21 Jul 2026 00:16:12 -0700 Subject: [PATCH 08/14] Fail loudly if the SimulationApp workarounds stop taking effect The two WORKAROUND(isaac-sim) blocks were wrapped in blanket exception suppression, so an upstream SimulationApp change could silently disable them and quietly reintroduce the exit-status masking. Print a clear warning when the fast-shutdown override does not take, and fall back to a plain close() with a warning if close() stops accepting exit_code. Reference the upstream fix (isaac-sim/IsaacSim#717) in the workaround note; once it lands, both blocks can be deleted. --- source/isaaclab/isaaclab/app/app_launcher.py | 24 +++++++++++++++++-- .../test/app/test_simulation_app_lifecycle.py | 19 +++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/source/isaaclab/isaaclab/app/app_launcher.py b/source/isaaclab/isaaclab/app/app_launcher.py index e25136c87f7b..195d6888aa65 100644 --- a/source/isaaclab/isaaclab/app/app_launcher.py +++ b/source/isaaclab/isaaclab/app/app_launcher.py @@ -1424,7 +1424,7 @@ class _SimulationAppLifecycle: exits with a failure status. WORKAROUND(isaac-sim): two pieces below exist only because of upstream - SimulationApp behavior and can be removed once it is fixed: fast shutdown is + SimulationApp behavior and can be removed once isaac-sim/IsaacSim#717 lands: fast shutdown is disabled during a signal close (so ``close()`` returns and the killed-by-signal status can be reported), and ``SIGINT`` is re-registered over SimulationApp's own handler (which exits 0 before user code unwinds). @@ -1478,7 +1478,18 @@ def _close_at_exit(self): # (e.g. ``sys.exit(1)``) does not set ``sys.last_exc`` and is not yet # detected here. exit_code = 1 if getattr(sys, "last_exc", None) is not None else 0 - self._app.close(exit_code=exit_code) + try: + self._app.close(exit_code=exit_code) + except TypeError: + # Fail loudly if a SimulationApp update drops the parameter, so the + # exit-status masking does not return silently. + print( + "[ISAACLAB] WARNING: SimulationApp.close() does not accept exit_code;" + " a pending failure may be reported as exit code 0.", + file=sys.__stderr__, + flush=True, + ) + self._app.close() def _on_abort_signal(self, signum, frame): """Handle SIGTERM/SIGABRT: close the app once, then die by the signal.""" @@ -1501,6 +1512,15 @@ def _on_abort_signal(self, signum, frame): carb.settings.get_settings().set("/app/fastShutdown", False) with contextlib.suppress(Exception): self._app.config["fast_shutdown"] = False + if self._app.config.get("fast_shutdown", True): + # Fail loudly if a SimulationApp update breaks the override, so the + # exit-0-on-signal regression does not return silently. + print( + "[ISAACLAB] WARNING: could not disable Kit fast shutdown; this process may" + " report exit code 0 instead of its termination signal.", + file=sys.__stderr__, + flush=True, + ) with contextlib.suppress(Exception): self._app.close() # Die by the signal so the exit status is the conventional 128 + signum diff --git a/source/isaaclab/test/app/test_simulation_app_lifecycle.py b/source/isaaclab/test/app/test_simulation_app_lifecycle.py index 65703130e08a..f274829c0dc0 100644 --- a/source/isaaclab/test/app/test_simulation_app_lifecycle.py +++ b/source/isaaclab/test/app/test_simulation_app_lifecycle.py @@ -120,3 +120,22 @@ def test_atexit_close_preserves_pending_failure_status(monkeypatch): lifecycle._close_at_exit() assert codes == [0, 1] + + +def test_atexit_close_falls_back_when_exit_code_unsupported(capfd): + """A ``close()`` that stops accepting ``exit_code`` still closes the app, with a warning. + + Guards the workaround against silent drift: if a SimulationApp update drops the + parameter, the app must still be closed and the exit-status limitation must be + reported instead of failing silently. + """ + calls = [] + + def _close(): # simulates a SimulationApp.close() without the exit_code parameter + calls.append(len(calls) + 1) + + lifecycle = _make_lifecycle(_close) + lifecycle._close_at_exit() + + assert calls == [1] + assert "does not accept exit_code" in capfd.readouterr().err From fef1ad49d01ffc1afa1c171378a1fcd245482d6d Mon Sep 17 00:00:00 2001 From: jichuanh Date: Tue, 21 Jul 2026 13:51:50 -0700 Subject: [PATCH 09/14] Pass the killed-by-signal status through close() The upstream SimulationApp fix is merged (isaacsim.simulation_app 2.18.5): close(exit_code) now performs its full teardown and exits with the given status under fast shutdown. Replace the interim fast-shutdown disable in the abort handler with close(exit_code=128 + signum): on 2.18.5+ the app tears down fully and exits truthfully; on older builds the status is still truthful without the teardown; if close() returns (fast shutdown disabled), the handler re-raises the signal with the default action as before. The remaining WORKAROUND(isaac-sim) is the SIGINT re-registration, which the merged fix does not cover. The SIGTERM integration test accepts both truthful outcomes (exit status 128+SIGTERM or death by SIGTERM) and passes against both the pre-fix and the fixed SimulationApp builds. --- .../fix-abort-handler-reentrancy.rst | 6 +- source/isaaclab/isaaclab/app/app_launcher.py | 59 +++++++++---------- .../test/app/test_app_launcher_exit_status.py | 6 +- .../test/app/test_simulation_app_lifecycle.py | 51 +++++++++++----- 4 files changed, 71 insertions(+), 51 deletions(-) diff --git a/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst b/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst index 87ed6d6a1dc7..b5bec97a1ced 100644 --- a/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst +++ b/source/isaaclab/changelog.d/fix-abort-handler-reentrancy.rst @@ -12,8 +12,10 @@ Fixed * 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 performs the full teardown and re-raises the signal with the default - action, so the process exits with the conventional killed-by-signal status. + 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. diff --git a/source/isaaclab/isaaclab/app/app_launcher.py b/source/isaaclab/isaaclab/app/app_launcher.py index 195d6888aa65..4b81b1b4d869 100644 --- a/source/isaaclab/isaaclab/app/app_launcher.py +++ b/source/isaaclab/isaaclab/app/app_launcher.py @@ -1408,10 +1408,12 @@ class _SimulationAppLifecycle: * Normal interpreter exit: the ``atexit`` hook closes the app once, passing a nonzero exit code when an unhandled exception is pending so the failure status survives Kit fast shutdown. - * ``SIGTERM`` and signal-delivered ``SIGABRT``: close once with full teardown, - then re-raise the signal with the default action so the process dies with - the conventional killed-by-signal status (128 + signum). Native ``abort()`` - calls bypass Python handlers and already terminate correctly. + * ``SIGTERM`` and signal-delivered ``SIGABRT``: close once 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). If + ``close()`` returns (fast shutdown disabled), re-raise the signal with the + default action. Native ``abort()`` calls bypass Python handlers and already + terminate correctly. * Any signal while a close is running: never re-enter ``close()`` (the nested calls previously recursed until the stack overflowed, leaving workers that only SIGKILL could stop); fall back to the default signal action. @@ -1423,11 +1425,9 @@ class _SimulationAppLifecycle: :class:`KeyboardInterrupt`, unwinds user code (``finally`` blocks run), and exits with a failure status. - WORKAROUND(isaac-sim): two pieces below exist only because of upstream - SimulationApp behavior and can be removed once isaac-sim/IsaacSim#717 lands: fast shutdown is - disabled during a signal close (so ``close()`` returns and the - killed-by-signal status can be reported), and ``SIGINT`` is re-registered - over SimulationApp's own handler (which exits 0 before user code unwinds). + WORKAROUND(isaac-sim): ``SIGINT`` is re-registered over SimulationApp's own + handler, which exits 0 before user code unwinds; remove once the upstream + handler preserves exception semantics and a nonzero exit status. """ def __init__(self, app: SimulationApp): @@ -1501,29 +1501,24 @@ def _on_abort_signal(self, signum, frame): signal.raise_signal(signum) return self._closing = True - # WORKAROUND(isaac-sim): under fast shutdown, ``close()`` terminates the - # process with exit code 0 from inside ``app.shutdown()``; a killed worker - # would then be recorded as successful by its launcher, and the remaining - # ranks would block until the collective watchdog fires. Disable fast - # shutdown so ``close()`` performs the full teardown and returns here. + # Close with the conventional killed-by-signal status so a killed worker is not + # recorded as successful by its launcher. With isaacsim.simulation_app >= 2.18.5 + # the app performs its full teardown and exits with this status; older builds + # exit with the correct status without the teardown. with contextlib.suppress(Exception): - import carb.settings - - carb.settings.get_settings().set("/app/fastShutdown", False) - with contextlib.suppress(Exception): - self._app.config["fast_shutdown"] = False - if self._app.config.get("fast_shutdown", True): - # Fail loudly if a SimulationApp update breaks the override, so the - # exit-0-on-signal regression does not return silently. - print( - "[ISAACLAB] WARNING: could not disable Kit fast shutdown; this process may" - " report exit code 0 instead of its termination signal.", - file=sys.__stderr__, - flush=True, - ) - with contextlib.suppress(Exception): - self._app.close() - # Die by the signal so the exit status is the conventional 128 + signum - # instead of reporting success. + try: + self._app.close(exit_code=128 + signum) + except TypeError: + # Fail loudly if ``close()`` stops accepting the parameter, so the + # exit-0-on-signal regression does not return silently. + print( + "[ISAACLAB] WARNING: SimulationApp.close() does not accept exit_code;" + " this process may report exit code 0 instead of its termination signal.", + file=sys.__stderr__, + flush=True, + ) + self._app.close() + # ``close()`` only returns when fast shutdown is disabled; die by the signal so + # the exit status is truthful on that path as well. signal.signal(signum, signal.SIG_DFL) signal.raise_signal(signum) diff --git a/source/isaaclab/test/app/test_app_launcher_exit_status.py b/source/isaaclab/test/app/test_app_launcher_exit_status.py index 0c58df4129c4..019ae8ccaacd 100644 --- a/source/isaaclab/test/app/test_app_launcher_exit_status.py +++ b/source/isaaclab/test/app/test_app_launcher_exit_status.py @@ -70,8 +70,10 @@ def test_sigterm_reports_killed_by_signal_status(): if proc.poll() is None: proc.kill() - # the process must die by SIGTERM, not report a successful exit - assert proc.returncode == -signal.SIGTERM, f"returncode={proc.returncode}\n{stderr}" + # 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 diff --git a/source/isaaclab/test/app/test_simulation_app_lifecycle.py b/source/isaaclab/test/app/test_simulation_app_lifecycle.py index f274829c0dc0..f85a2a6e4145 100644 --- a/source/isaaclab/test/app/test_simulation_app_lifecycle.py +++ b/source/isaaclab/test/app/test_simulation_app_lifecycle.py @@ -30,22 +30,21 @@ def _capture_signal_actions(monkeypatch): return actions -def test_abort_signal_closes_once_and_reports_killed_by_signal(monkeypatch): - """The first signal closes the app once, then re-raises with the default action. +def test_abort_signal_closes_once_with_killed_by_signal_status(monkeypatch): + """The first signal closes the app once with the conventional 128 + signum status. - The re-raise is what makes the process exit with the conventional 128 + signum - status instead of the exit code 0 that Kit fast shutdown reports. + Passing the status through ``close()`` lets the app perform its full teardown and + exit truthfully instead of the exit code 0 that Kit fast shutdown reports. """ - close_calls = [] - lifecycle = _make_lifecycle(lambda: close_calls.append(len(close_calls) + 1)) + codes = [] + lifecycle = _make_lifecycle(lambda exit_code=0: codes.append(exit_code)) actions = _capture_signal_actions(monkeypatch) lifecycle._on_abort_signal(signal.SIGTERM, None) - # the app must be closed exactly once, with fast shutdown disabled beforehand - assert close_calls == [1] - assert lifecycle._app.config["fast_shutdown"] is False - # the handler must hand the signal back to the default action to preserve the status + # the app must be closed exactly once, with the killed-by-signal status + assert codes == [128 + signal.SIGTERM] + # if close() returns (fast shutdown disabled), the default action must re-raise assert ("set_handler", signal.SIGTERM, signal.SIG_DFL) in actions assert ("raise", signal.SIGTERM) in actions @@ -59,7 +58,7 @@ def test_abort_signal_reentrant_signal_skips_nested_close(monkeypatch): """ close_calls = [] - def _close(): + def _close(exit_code=0): close_calls.append(len(close_calls) + 1) # Simulate further signals delivered while the app is still closing. if len(close_calls) < 5: @@ -77,12 +76,34 @@ def _close(): assert actions.count(("raise", signal.SIGTERM)) >= 1 +def test_abort_signal_falls_back_when_exit_code_unsupported(capfd, monkeypatch): + """A ``close()`` without the ``exit_code`` parameter still closes the app, loudly. + + Guards against silent drift: if a SimulationApp update drops the parameter, the + app must still be closed and the exit-status limitation must be reported. + """ + calls = [] + + def _close(): # simulates a SimulationApp.close() without the exit_code parameter + calls.append(len(calls) + 1) + + lifecycle = _make_lifecycle(_close) + actions = _capture_signal_actions(monkeypatch) + + lifecycle._on_abort_signal(signal.SIGTERM, None) + + assert calls == [1] + assert "does not accept exit_code" in capfd.readouterr().err + # the re-raise still guarantees a truthful killed-by-signal status + assert ("raise", signal.SIGTERM) in actions + + def test_atexit_close_arms_reentrancy_guard(monkeypatch): """A signal arriving during the atexit close must take the re-entrant path. - The atexit hook arms the same guard flag as the signal handler, so a SIGTERM - delivered mid-close falls back to the default signal action instead of starting - a second, nested teardown. + The atexit callback arms the same guard flag as the signal handler, so a SIGTERM + delivered mid-close falls back to the default signal action instead of starting a + second, nested teardown. """ close_calls = [] @@ -125,7 +146,7 @@ def test_atexit_close_preserves_pending_failure_status(monkeypatch): def test_atexit_close_falls_back_when_exit_code_unsupported(capfd): """A ``close()`` that stops accepting ``exit_code`` still closes the app, with a warning. - Guards the workaround against silent drift: if a SimulationApp update drops the + Guards the exit-code path against silent drift: if a SimulationApp update drops the parameter, the app must still be closed and the exit-status limitation must be reported instead of failing silently. """ From fd09547ccf3793d3bb33117f2bda596fa0b5ee30 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Tue, 21 Jul 2026 13:58:35 -0700 Subject: [PATCH 10/14] Explain why the SIGINT workaround is handled in the launcher The upstream exit-status fix (isaacsim.simulation_app 2.18.5) deliberately excluded the SIGINT handler because fixing it changes user-visible Ctrl-C semantics for every SimulationApp consumer; state that rationale at the workaround site. --- source/isaaclab/isaaclab/app/app_launcher.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/source/isaaclab/isaaclab/app/app_launcher.py b/source/isaaclab/isaaclab/app/app_launcher.py index 4b81b1b4d869..751f113a984c 100644 --- a/source/isaaclab/isaaclab/app/app_launcher.py +++ b/source/isaaclab/isaaclab/app/app_launcher.py @@ -1426,8 +1426,13 @@ class _SimulationAppLifecycle: exits with a failure status. WORKAROUND(isaac-sim): ``SIGINT`` is re-registered over SimulationApp's own - handler, which exits 0 before user code unwinds; remove once the upstream - handler preserves exception semantics and a nonzero exit status. + handler, which exits 0 before user code unwinds. This is handled here rather + than upstream because fixing the upstream handler changes user-visible Ctrl-C + semantics for every SimulationApp consumer (``KeyboardInterrupt`` starts + propagating and exit codes become nonzero), which needs its own upstream + review; the exit-status fix that shipped in isaacsim.simulation_app 2.18.5 + deliberately excluded it. Remove once the upstream handler preserves + exception semantics and a nonzero exit status. """ def __init__(self, app: SimulationApp): From fe94b5bf75e42eecaa0387c0ce48b3c2d42de462 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Tue, 21 Jul 2026 14:02:38 -0700 Subject: [PATCH 11/14] Simplify lifecycle comments and deduplicate the close fallback Compress the class docstring and per-site comments to essentials, fold the duplicated exit_code TypeError fallback into a _close_app helper, and describe the SIGINT registration as behavior not handled on the sim side (changing it there affects Ctrl-C semantics for every consumer) rather than as a workaround. --- source/isaaclab/isaaclab/app/app_launcher.py | 142 +++++++------------ 1 file changed, 52 insertions(+), 90 deletions(-) diff --git a/source/isaaclab/isaaclab/app/app_launcher.py b/source/isaaclab/isaaclab/app/app_launcher.py index 751f113a984c..185ec1a38ae3 100644 --- a/source/isaaclab/isaaclab/app/app_launcher.py +++ b/source/isaaclab/isaaclab/app/app_launcher.py @@ -1396,49 +1396,36 @@ def _hide_play_button(self, flag): class _SimulationAppLifecycle: """Reports the lifecycle of the Kit-based :class:`SimulationApp` process. - Everything a supervisor learns about this process flows through here: the CI - test runner watches the startup announcements, and distributed launchers, - schedulers, and CI read the exit status. The exit-path policy exists because - Kit fast shutdown terminates the process with exit code 0 from inside - :meth:`SimulationApp.close`, so any death funneled through an unqualified - ``close()`` is reported as success. - - Exit paths and policy: - - * Normal interpreter exit: the ``atexit`` hook closes the app once, passing a - nonzero exit code when an unhandled exception is pending so the failure - status survives Kit fast shutdown. - * ``SIGTERM`` and signal-delivered ``SIGABRT``: close once 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). If - ``close()`` returns (fast shutdown disabled), re-raise the signal with the - default action. Native ``abort()`` calls bypass Python handlers and already - terminate correctly. - * Any signal while a close is running: never re-enter ``close()`` (the nested - calls previously recursed until the stack overflowed, leaving workers that - only SIGKILL could stop); fall back to the default signal action. - * ``SIGSEGV``: intentionally not handled. A Python handler never runs for a - synchronous main-thread segfault (the process would spin on signal - delivery), reports a successful exit for worker-thread segfaults, and - replaces the carb crash reporter's handler, suppressing minidumps. - * ``SIGINT``: Python's default handler, so Ctrl-C raises - :class:`KeyboardInterrupt`, unwinds user code (``finally`` blocks run), and - exits with a failure status. - - WORKAROUND(isaac-sim): ``SIGINT`` is re-registered over SimulationApp's own - handler, which exits 0 before user code unwinds. This is handled here rather - than upstream because fixing the upstream handler changes user-visible Ctrl-C - semantics for every SimulationApp consumer (``KeyboardInterrupt`` starts - propagating and exit codes become nonzero), which needs its own upstream - review; the exit-status fix that shipped in isaacsim.simulation_app 2.18.5 - deliberately excluded it. Remove once the upstream handler preserves - exception semantics and a nonzero exit status. + 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 __init__(self, app: SimulationApp): self._app = app - # True once any close has started; a signal arriving after that must not - # start a second, nested teardown of a half-closed app. + # set once any close starts; later signals must not start a second teardown self._closing = False def announce_startup(self): @@ -1446,84 +1433,59 @@ def announce_startup(self): import carb import omni.kit.app - # The CI test runner watches for this marker to distinguish a Kit that is - # still starting from one that hung. stdout may be redirected to /dev/null - # during app creation, so use __stderr__, which is never suppressed. + # 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_version = kit_app.get_kit_version() - kernel_version = kit_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 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 the app 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). + # close on normal exit so Kit shuts down cleanly instead of via __del__ atexit.register(self._close_at_exit) - # Explicit `kill` and signal-delivered aborts take the truthful-close path. signal.signal(signal.SIGTERM, self._on_abort_signal) signal.signal(signal.SIGABRT, self._on_abort_signal) - # SIGSEGV is intentionally absent and SIGINT uses Python's default handler; - # the class docstring explains both decisions. + # 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): - # The interpreter sets ``sys.last_exc`` before atexit callbacks run for - # an unhandled exception; pass a nonzero code so Kit fast shutdown does - # not replace the pending failure status with 0. Note that ``SystemExit`` - # (e.g. ``sys.exit(1)``) does not set ``sys.last_exc`` and is not yet - # detected here. + # 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 - try: - self._app.close(exit_code=exit_code) - except TypeError: - # Fail loudly if a SimulationApp update drops the parameter, so the - # exit-status masking does not return silently. - print( - "[ISAACLAB] WARNING: SimulationApp.close() does not accept exit_code;" - " a pending failure may be reported as exit code 0.", - file=sys.__stderr__, - flush=True, - ) - self._app.close() + 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: - # A signal during a running close must not re-enter ``close()``: the - # nested calls recurse until the stack overflows, so the process can - # neither shut down cleanly nor report a meaningful failure. signal.signal(signum, signal.SIG_DFL) signal.raise_signal(signum) return self._closing = True - # Close with the conventional killed-by-signal status so a killed worker is not - # recorded as successful by its launcher. With isaacsim.simulation_app >= 2.18.5 - # the app performs its full teardown and exits with this status; older builds - # exit with the correct status without the teardown. with contextlib.suppress(Exception): - try: - self._app.close(exit_code=128 + signum) - except TypeError: - # Fail loudly if ``close()`` stops accepting the parameter, so the - # exit-0-on-signal regression does not return silently. - print( - "[ISAACLAB] WARNING: SimulationApp.close() does not accept exit_code;" - " this process may report exit code 0 instead of its termination signal.", - file=sys.__stderr__, - flush=True, - ) - self._app.close() - # ``close()`` only returns when fast shutdown is disabled; die by the signal so - # the exit status is truthful on that path as well. + # >= 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.""" + try: + self._app.close(exit_code=exit_code) + except TypeError: + 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() From ddc38bde4637f8d30370fb7aad1562bee1ba943b Mon Sep 17 00:00:00 2001 From: jichuanh Date: Wed, 22 Jul 2026 15:29:04 -0700 Subject: [PATCH 12/14] Bound the startup readiness wait in the exit-status test The readiness loop iterated the child's stdout with a blocking readline, so its 300-second deadline was only checked between lines: a child hanging silently during startup stalled the test and its CI shard until the job timeout. Wait on an event fed by a daemon reader thread instead, fail fast when the child exits before printing the ready marker, and preserve the collected child output in the failure report. --- .../test/app/test_app_launcher_exit_status.py | 44 ++++++++++++++++--- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/source/isaaclab/test/app/test_app_launcher_exit_status.py b/source/isaaclab/test/app/test_app_launcher_exit_status.py index 019ae8ccaacd..e39c8d39bba7 100644 --- a/source/isaaclab/test/app/test_app_launcher_exit_status.py +++ b/source/isaaclab/test/app/test_app_launcher_exit_status.py @@ -15,6 +15,7 @@ import signal import subprocess import sys +import threading import time import pytest @@ -40,6 +41,40 @@ def _raise_after_app_launcher_initialization() -> None: 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. @@ -56,13 +91,8 @@ def test_sigterm_reports_killed_by_signal_status(): text=True, ) try: - # wait for the app to finish starting up - deadline = time.time() + 300 - for line in proc.stdout: - if _READY_MARKER in line: - break - if time.time() > deadline: - pytest.fail("AppLauncher child did not become ready in time") + # 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) From 7dfdbd3e93c5a72bbff005765fc74e958219f9e8 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Thu, 23 Jul 2026 17:34:32 -0700 Subject: [PATCH 13/14] Detect close() exit_code support via signature probe The unguarded ``except TypeError`` around ``close(exit_code=...)`` conflated two distinct failures: a SimulationApp whose close() lacks the parameter, and a TypeError raised inside close()'s own teardown. The latter was misread as an unsupported parameter, printed a misleading warning, and re-invoked close() on a half-torn-down app -- the exact re-entrancy this class exists to prevent. Probe inspect.signature up front so only a genuinely missing parameter takes the fallback path. --- source/isaaclab/isaaclab/app/app_launcher.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/source/isaaclab/isaaclab/app/app_launcher.py b/source/isaaclab/isaaclab/app/app_launcher.py index 185ec1a38ae3..1d1f26fc5e32 100644 --- a/source/isaaclab/isaaclab/app/app_launcher.py +++ b/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 @@ -1479,9 +1480,19 @@ def _on_abort_signal(self, signum, frame): 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) - except TypeError: + else: print( "[ISAACLAB] WARNING: SimulationApp.close() does not accept exit_code;" " this process may report exit code 0 instead of its failure status.", From 48eb42da4c33d45291f75f4bb926f55bcd2eeee7 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Thu, 23 Jul 2026 17:34:33 -0700 Subject: [PATCH 14/14] Drop stale --enable_cameras mention from mGPU docs Cameras auto-enable for scenes with Kit-renderer sensors, so the flag is no longer required for camera-based tasks. Remove it from the NCCL NUMA-spanning workaround note. --- docs/source/features/multi_gpu.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/features/multi_gpu.rst b/docs/source/features/multi_gpu.rst index 099c6cb8f568..df857bce4045 100644 --- a/docs/source/features/multi_gpu.rst +++ b/docs/source/features/multi_gpu.rst @@ -147,7 +147,7 @@ If the issue persists, additional NCCL fallbacks that may help are: export NCCL_ALGO=Ring On some multi-GPU systems, distributed training with RTX rendering enabled (for example, -camera-based tasks launched with ``--enable_cameras``) fails when the participating GPUs span +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, ...)``