diff --git a/docs/source/features/multi_gpu.rst b/docs/source/features/multi_gpu.rst index dcf9322ad84d..ceb030c54e81 100644 --- a/docs/source/features/multi_gpu.rst +++ b/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 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..b5bec97a1ced --- /dev/null +++ b/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. diff --git a/source/isaaclab/isaaclab/app/app_launcher.py b/source/isaaclab/isaaclab/app/app_launcher.py index 33d175554307..663d809bbcb9 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 @@ -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() diff --git a/source/isaaclab/test/app/test_app_launcher_exit_status.py b/source/isaaclab/test/app/test_app_launcher_exit_status.py new file mode 100644 index 000000000000..e39c8d39bba7 --- /dev/null +++ b/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() diff --git a/source/isaaclab/test/app/test_simulation_app_lifecycle.py b/source/isaaclab/test/app/test_simulation_app_lifecycle.py new file mode 100644 index 000000000000..f85a2a6e4145 --- /dev/null +++ b/source/isaaclab/test/app/test_simulation_app_lifecycle.py @@ -0,0 +1,162 @@ +# 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 process-lifecycle exit policy.""" + +import signal +import sys +import types + +from isaaclab.app.app_launcher import AppLauncher + + +def _make_lifecycle(close_fn): + app = types.SimpleNamespace(close=close_fn, config={"fast_shutdown": True}) + return AppLauncher._SimulationAppLifecycle(app) + + +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_signal_closes_once_with_killed_by_signal_status(monkeypatch): + """The first signal closes the app once with the conventional 128 + signum status. + + 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. + """ + 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 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 + + +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 + ``SimulationApp.close()`` re-entered the abort handler, recursing until the + stack overflowed and the process had to be SIGKILL-ed. + """ + close_calls = [] + + 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: + lifecycle._on_abort_signal(signal.SIGTERM, None) + + lifecycle = _make_lifecycle(_close) + actions = _capture_signal_actions(monkeypatch) + + lifecycle._on_abort_signal(signal.SIGTERM, None) + + # 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_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 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(exit_code=0): + close_calls.append(len(close_calls) + 1) + # Simulate a signal delivered while the atexit close is running. + lifecycle._on_abort_signal(signal.SIGTERM, None) + + lifecycle = _make_lifecycle(_close) + actions = _capture_signal_actions(monkeypatch) + + 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] + + +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 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. + """ + 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