Make AppLauncher teardown truthful and re-entrancy-safe; document mGPU NCCL workaround#6636
Make AppLauncher teardown truthful and re-entrancy-safe; document mGPU NCCL workaround#6636hujc7 wants to merge 8 commits into
Conversation
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.
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.
Greptile SummaryThis PR fixes a re-entrancy bug in
Confidence Score: 5/5Safe to merge. The re-entrancy guard is minimal, correct, and does not change the happy-path shutdown sequence. The flag is set before close() is called so any re-entrant signal immediately takes the fallback path; the rename of the signal parameter to signum is both a style improvement and a correctness prerequisite for the new signal.signal() and signal.raise_signal() calls inside the handler. The regression test exercises the re-entrant path end-to-end without needing a live Kit instance. The docs change is accurately scoped to the observed symptom and workaround. No existing behaviour is altered on the first signal delivery. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant OS as OS / torchrun
participant SH as _abort_signal_handle_callback
participant Flag as _abort_in_progress
participant App as SimulationApp.close()
OS->>SH: SIGTERM (first delivery)
SH->>Flag: check False
SH->>Flag: set True
SH->>App: close() [starts long teardown]
Note over App: e.g. replicator orchestrator.stop() blocks...
OS-->>SH: SIGTERM (re-entrant, during close)
SH->>Flag: check True
SH->>OS: signal.signal(SIGTERM, SIG_DFL)
SH->>OS: signal.raise_signal(SIGTERM)
Note over OS: process terminates with original signal
App-->>SH: close() returns (if re-entrant signal did not kill yet)
SH-->>OS: handler returns
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant OS as OS / torchrun
participant SH as _abort_signal_handle_callback
participant Flag as _abort_in_progress
participant App as SimulationApp.close()
OS->>SH: SIGTERM (first delivery)
SH->>Flag: check False
SH->>Flag: set True
SH->>App: close() [starts long teardown]
Note over App: e.g. replicator orchestrator.stop() blocks...
OS-->>SH: SIGTERM (re-entrant, during close)
SH->>Flag: check True
SH->>OS: signal.signal(SIGTERM, SIG_DFL)
SH->>OS: signal.raise_signal(SIGTERM)
Note over OS: process terminates with original signal
App-->>SH: close() returns (if re-entrant signal did not kill yet)
SH-->>OS: handler returns
Reviews (1): Last reviewed commit: "Document NCCL_CUMEM_ENABLE=0 workaround ..." | Re-trigger Greptile |
Extend the re-entrancy fix into a complete teardown-correctness change based on a combined review with the atexit exit-code fix (isaac-sim#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.
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.
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.
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 isaac-sim#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.
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.
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.
Summary
AppLauncherprocess-lifecycle handling into one nested_SimulationAppLifecycleclass: startup announcements (CI marker, Kit version diagnostics) plus the entire exit-path policy, with the policy table as the class docstring.NCCL_CUMEM_HOST_ENABLE=0first,NCCL_CUMEM_ENABLE=0as fallback).WORKAROUND(isaac-sim)and fail loudly if upstream drift disables them; both become deletable once Preserve nonzero exit status through fast-shutdown close IsaacSim#717 lands.Failure cases and how this PR addresses them
Root mechanism: Kit fast shutdown terminates the process with exit code 0 from inside
SimulationApp.close(), so any death funneled through an unqualifiedclose()is reported as success; additionally, the abort-signal handler was unguarded against re-entrancy.sys.last_excdetected; absorbed from #6634;SystemExitdocumented as not yet covered)kill)128+signum)close()→ infinite recursion → SIGKILL-only shutdown, spurious SIGSEGV, logs flooded (~975 recursion frames/job observed on OSMO pods)SIG_DFLkill -ABRTfinally/KeyboardInterruptcode runsKeyboardInterruptunwinds user code, nonzero exitNot addressed here (tracked elsewhere):
sys.exit(N)/SystemExitstill exits 0 (gap inside the #6634 mechanism, documented at the detection site); the fast-shutdown disable on the signal path is an interim cost removed by isaac-sim/IsaacSim#717.Implementation notes
AppLauncher._SimulationAppLifecycle; the class docstring is the policy table, and each decision carries its rationale in place.close()returns, then re-raises with the default action (WORKAROUND(isaac-sim), removable once IsaacSim#717 lands). If the override stops taking effect, a[ISAACLAB] WARNINGis printed instead of silently regressing.Testing. Five kitless unit tests (
test_simulation_app_lifecycle.py: re-entrancy both directions, killed-by-signal contract, exit-code selection, drift fallback) plus two real-Kit integration tests (test_app_launcher_exit_status.py: SIGTERM → dies by SIGTERM with no recursion; unhandled exception → exits 1). All fail against the previous behavior — the SIGTERM test observes exit code 0 on develop — and pass with the fix. The integration tests also pass unchanged against an IsaacSim build patched with IsaacSim#717, confirming no lab change is needed when it lands.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there