Repository navigation
Conversation
Route SIGTERM to a terminating handler so Python workers cannot resume after SimulationApp closes. Preserve normal cleanup by unwinding finally and atexit handlers before process exit. Signed-off-by: Yichao Gan <47491276+RX-02333@users.noreply.github.com>
Greptile SummaryThis PR fixes a distributed-training shutdown regression where a Python process could survive after receiving SIGTERM, because the old handler called
Confidence Score: 4/5Safe to merge. The production change is a one-line handler swap backed by the existing atexit registration; the fix is well-scoped and does not touch any public API. The core change is correct and minimal: SIGTERM now raises SystemExit(143), which propagates through finally/atexit so SimulationApp.close() is still invoked via the pre-existing atexit handler. The only gap is that the test exercises the callback directly but does not verify the handler is actually registered after AppLauncher initialises. test_signal_handling.py — the test covers the callback behaviour but not the signal registration itself. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant OS
participant PythonSignal as Python Signal Dispatch
participant Handler as _terminate_signal_handle_callback
participant AtExit as atexit._atexit_close
participant App as SimulationApp
OS->>PythonSignal: SIGTERM delivered
PythonSignal->>Handler: "signum=15, frame"
Handler-->>PythonSignal: raise SystemExit(143)
PythonSignal-->>AtExit: atexit handlers triggered
AtExit->>App: app.close()
App-->>AtExit: done
AtExit-->>OS: process exits with code 143
%%{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
participant PythonSignal as Python Signal Dispatch
participant Handler as _terminate_signal_handle_callback
participant AtExit as atexit._atexit_close
participant App as SimulationApp
OS->>PythonSignal: SIGTERM delivered
PythonSignal->>Handler: "signum=15, frame"
Handler-->>PythonSignal: raise SystemExit(143)
PythonSignal-->>AtExit: atexit handlers triggered
AtExit->>App: app.close()
App-->>AtExit: done
AtExit-->>OS: process exits with code 143
Reviews (1): Last reviewed commit: "Merge branch 'develop' into rx-02333/fix..." | Re-trigger Greptile |
| def test_sigterm_handler_terminates_process(): | ||
| """SIGTERM must terminate Python instead of resuming after the handler.""" | ||
| with pytest.raises(SystemExit) as exc_info: | ||
| AppLauncher._terminate_signal_handle_callback(signal.SIGTERM, None) | ||
|
|
||
| assert exc_info.value.code == 128 + signal.SIGTERM |
There was a problem hiding this comment.
Test covers callback but not handler registration
The test exercises _terminate_signal_handle_callback directly, confirming the SystemExit(143) path. It does not assert that signal.getsignal(signal.SIGTERM) actually points to this method after AppLauncher initialises. If the registration line were accidentally reverted or guarded behind a condition, the test would still pass. A complementary integration-level assertion (e.g. checking signal.getsignal(signal.SIGTERM) after constructing an AppLauncher) would close this gap, though doing so requires the full Isaac Sim environment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…he config (1/2) (#8068) ## Description **Part 1 of 2: the design.** The migration of every script and test is in the stacked follow-up, #8090. This PR keeps existing scripts running unchanged, so the two halves can be reviewed separately. `launch_simulation` now starts each simulation runtime through the launcher that the runtime's own backend package provides. The resolved config says which launcher is needed, so core never imports one. ```python # isaaclab.app: the interface every backend runtime implements class SimulationLauncher: device: str | None = None # optional device the runtime picked def __init__(self, launcher_args): ... # constructing it starts the runtime def close(self, exit_code: int = 0): ... # stops it # backend configs name their launcher, resolved lazily like ``class_type`` class PhysxCfg(PhysicsCfg): launcher_type: ClassVar[str] = "isaaclab_physx.app:KitLauncher" class OVRTXRendererCfg(RendererCfg): launcher_type: ClassVar[str] = "isaaclab_ov.app:OvrtxLauncher" ``` `launch_simulation(cfg, args)` scans the resolved config as it already did and collects `launcher_type` from the physics and renderer config instances it resolved. It constructs each launcher once and closes them in reverse order when the block exits. Kit needs that no config names (`require_kit`, no physics config, a Kit visualizer requested only on the CLI) fall back to `KitLauncher`. - **`isaaclab_physx.app.KitLauncher`** is the old `AppLauncher` body, minus dead code: - removed: argument-collision bookkeeping, the Isaac Sim 5 branch, and the hidden `--cpu` flag; - moved in from `launch_simulation`: the Isaac Sim availability check and the "Kit already running" guard. - **`isaaclab_ov.app.OvrtxLauncher`** registers the OVRTX USD schemas, which `launch_simulation` used to do inline. - **Moved into `sim_launcher.py`** because kitless launches need them too: argv fusing, visualizer argument normalization, and the visualizer-settings sync (no longer wrapped in a blanket `suppress(Exception)`). ### Kit stays in `isaaclab_physx` Core code no longer imports `carb`, `omni`, or `omni.replicator`. Kit concerns go through one of two owners, and no new global hook is added. - **The `SimulationContext` backend registry** owns anything tied to a simulation's lifetime: - `isaaclab_physx.app.KitStageBackend` is registered through `get_or_create_backend` when Kit runs; - it attaches the stage to Kit's USD context and closes Kit's context stage when `clear_instance()` closes the other backends; - core stage utilities never touch Kit: `clear_stage()`/`close_stage()` are Kit-free, and Kit's own consumers (PhysX reset/step, RTX render, the Kit visualizer) run the app updates they need. - **No defensive `None` handling on invariants:** a `SimulationContext` always has a stage, so consumers read `SimulationContext.instance().stage` directly (`SceneDataProvider.usd_stage` / `get_usd_stage` are removed). - **`KitLauncher`** owns one-time process setup: - `SettingsManager.set_backend(carb.settings)`, replacing `initialize_carb_settings` and `is_omniverse_mode`; - sharing Isaac Sim's stage-helper context; - one app update in `close()`, so scripts no longer need their own. - **Replicator** is enabled by the Isaac RTX renderer and seeded by the Replicator event terms that use it. The env classes no longer import it. - **`show_stage_in_viewport`** moves to `isaaclab_physx.app`, since it only works under Kit. - **Gravity:** tasks set it through `PhysxManager.set_gravity` instead of `carb.Float3`. ### Exit handling The ~110-line `_SimulationAppLifecycle` class from #6636 is gone. I measured a real Kit process with no handlers installed, and only two exit paths needed help. Both are now handled by two lines in `KitLauncher`: - An unhandled exception exits with 1. Kit's exit path reported 0. - `SIGINT` raises `KeyboardInterrupt`. Kit's own handler exited 0. `SIGTERM`, `SIGABRT`, and `SIGSEGV` keep their default actions. Fixes #7774, supersedes #7775 and #6531. ### Constructing runtime objects after launch Importing `pxr.Usd` before Kit starts makes Kit fail to start. Scripts therefore import only config classes and construct runtime objects with `instantiate(cfg)` (#8085), which resolves the config's lazy `class_type` after the runtime is up. - `ManagerBasedEnvCfg` and `ManagerBasedRLEnvCfg` gain `class_type`. - The 21 task configs with a custom env class name it. A registry test checks that all 126 match their registered entry point. - **Fixed:** `{DIR}` in an inherited `class_type` resolved against the module of a subclass missing `@configclass`, so the call failed with `ModuleNotFoundError`. ### Compatibility in this PR `isaaclab.app.AppLauncher` is deprecated. It now starts Kit through `launch_simulation`, so develop's scripts and tests run unchanged here. It still offers `is_available`, `has_gui`, and `has_window`, which unmigrated scripts use. The migration PR moves every caller and reduces it to an init-only shim. `isaaclab.test.utils.launch_test_simulation(**launcher_args)` starts Kit for a test module through `launch_simulation`. It picks the test device by default. ### Behavior changes - **Removed** (breaking; each only restated an invariant or forwarded elsewhere): - `sim_utils.update_stage`: the simulation's reset, step, and render process stage changes; - `sim_utils.is_current_stage_in_memory`: compare against `SimulationContext.instance().stage`; - `sim_utils.show_stage_in_viewport`: use `isaaclab_physx.app.show_stage_in_viewport`; - `SettingsManager.initialize_carb_settings` (and the module-level function) and `is_omniverse_mode`: `KitLauncher` sets the settings backend, and `has_kit()` checks for Kit; - `SceneDataProvider.usd_stage` / `get_usd_stage`; - the hidden `--cpu` flag: use `--device cpu`. - **Replicator seeding:** the Replicator event terms seed it with `env.cfg.seed` when they are built; `_get_replicator()` only loads it. Calling `env.seed(x)` later no longer reseeds Replicator. - **Kit startup marker** is now `[ISAACLAB] KitLauncher initialization complete`, and `tools/conftest.py` is updated to match. - **SIGTERM** ends the process immediately, without a Kit teardown. - **Warp environments:** `render_mode="rgb_array"` now warns and returns `None`, matching the core environments. Net: +2,326 / −3,045 across 154 files. ## Type of change - Breaking change (`AppLauncher` deprecated; Kit-coupled helpers removed; Kit exit and seeding behavior changed) ## Validation All runs on the display GPU (GPU 0), with develop's scripts and tests unchanged unless listed here. - **Launcher, app, and config tests:** - `isaaclab/test/app` (26) and `isaaclab_physx/test/app` (88); - `test_runtime_compatibility` (27), visualizer intent (8), distributed device (13), `test_preset_kit_decision` (6), `test_preset_cli` (11); - `test_configclass` (37), `test_sb3_leapp_export`, and the CLI tests (234). - **Kit tests through the deprecated adapter:** - `test_simulation_context` (30), PhysX `test_rigid_object` (44), `test_camera` (21); - stage utils (15), stage in memory (3), `test_events` (4), texture randomization (2), `test_manager_based_env` (4). - **Training:** `train.py --task Isaac-Cartpole --max_iterations 2` exits 0 with `isaacsim_physx` and `newton_mjwarp`. - `physics=physx` (kitless ovphysx) fails with an observation batch mismatch. - Develop `93b6e7432` fails the same way, so it is unrelated to this PR. - **Every script under `scripts/` and `examples/`**, compared against develop with the same invocation: - **Scripts outside the standalone matrix (78):** same outcome on develop and here. 52 run, 19 are blocked by an external resource (datasets, XR, Ray, gr00t, robomimic, moviepy), and 7 fail identically on develop. - **Standalone matrix (361 cases):** no regressions. The only case that first looked new was an old-style script with `--visualizer newton_rtx`. Kit and OVRTX cannot share a process, so the adapter now skips that visualizer with a warning, as Kit did on develop. - **Newton-based visualizers next to Kit PhysX:** those cases fail on develop too, when Newton builds its model from USD. - **After rebasing onto the latest develop:** the state machines, `bin_packing`, and Cartpole training still run. - **Known pre-existing failure:** `test_visualizer_integration_newton` has a flat-frame check that fails identically on develop. - Pre-commit passes. The changelog check only flags fragments from other PRs, because it compares against the fork's stale `develop`. ## Release backport - [x] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` ## Checklist - [x] I have read and understood the contribution guidelines - [x] I have run the pre-commit checks - [x] I have made corresponding changes to the documentation (API pages for `isaaclab.app`, `isaaclab_physx.app`, `isaaclab_ov.app`) - [x] My changes generate no new warnings - [x] I have added changelog fragments for every touched package
|
Superseded by #8068 (merged as c9f9e22). Isaac Lab no longer installs a SIGTERM handler, so SIGTERM terminates the process (the shell reports 143) and a worker can no longer outlive its process group during distributed shutdown. One difference from this PR: the process dies by the signal without running finally blocks or atexit handlers, so there is no Kit teardown on SIGTERM. If you need cleanup on SIGTERM, please open a follow-up. Thanks for the report and fix. |
…he config (1/2) (#8068) ## Description **Part 1 of 2: the design.** The migration of every script and test is in the stacked follow-up, #8090. This PR keeps existing scripts running unchanged, so the two halves can be reviewed separately. `launch_simulation` now starts each simulation runtime through the launcher that the runtime's own backend package provides. The resolved config says which launcher is needed, so core never imports one. ```python # isaaclab.app: the interface every backend runtime implements class SimulationLauncher: device: str | None = None # optional device the runtime picked def __init__(self, launcher_args): ... # constructing it starts the runtime def close(self, exit_code: int = 0): ... # stops it # backend configs name their launcher, resolved lazily like ``class_type`` class PhysxCfg(PhysicsCfg): launcher_type: ClassVar[str] = "isaaclab_physx.app:KitLauncher" class OVRTXRendererCfg(RendererCfg): launcher_type: ClassVar[str] = "isaaclab_ov.app:OvrtxLauncher" ``` `launch_simulation(cfg, args)` scans the resolved config as it already did and collects `launcher_type` from the physics and renderer config instances it resolved. It constructs each launcher once and closes them in reverse order when the block exits. Kit needs that no config names (`require_kit`, no physics config, a Kit visualizer requested only on the CLI) fall back to `KitLauncher`. - **`isaaclab_physx.app.KitLauncher`** is the old `AppLauncher` body, minus dead code: - removed: argument-collision bookkeeping, the Isaac Sim 5 branch, and the hidden `--cpu` flag; - moved in from `launch_simulation`: the Isaac Sim availability check and the "Kit already running" guard. - **`isaaclab_ov.app.OvrtxLauncher`** registers the OVRTX USD schemas, which `launch_simulation` used to do inline. - **Moved into `sim_launcher.py`** because kitless launches need them too: argv fusing, visualizer argument normalization, and the visualizer-settings sync (no longer wrapped in a blanket `suppress(Exception)`). ### Kit stays in `isaaclab_physx` Core code no longer imports `carb`, `omni`, or `omni.replicator`. Kit concerns go through one of two owners, and no new global hook is added. - **The `SimulationContext` backend registry** owns anything tied to a simulation's lifetime: - `isaaclab_physx.app.KitStageBackend` is registered through `get_or_create_backend` when Kit runs; - it attaches the stage to Kit's USD context and closes Kit's context stage when `clear_instance()` closes the other backends; - core stage utilities never touch Kit: `clear_stage()`/`close_stage()` are Kit-free, and Kit's own consumers (PhysX reset/step, RTX render, the Kit visualizer) run the app updates they need. - **No defensive `None` handling on invariants:** a `SimulationContext` always has a stage, so consumers read `SimulationContext.instance().stage` directly (`SceneDataProvider.usd_stage` / `get_usd_stage` are removed). - **`KitLauncher`** owns one-time process setup: - `SettingsManager.set_backend(carb.settings)`, replacing `initialize_carb_settings` and `is_omniverse_mode`; - sharing Isaac Sim's stage-helper context; - one app update in `close()`, so scripts no longer need their own. - **Replicator** is enabled by the Isaac RTX renderer and seeded by the Replicator event terms that use it. The env classes no longer import it. - **`show_stage_in_viewport`** moves to `isaaclab_physx.app`, since it only works under Kit. - **Gravity:** tasks set it through `PhysxManager.set_gravity` instead of `carb.Float3`. ### Exit handling The ~110-line `_SimulationAppLifecycle` class from #6636 is gone. I measured a real Kit process with no handlers installed, and only two exit paths needed help. Both are now handled by two lines in `KitLauncher`: - An unhandled exception exits with 1. Kit's exit path reported 0. - `SIGINT` raises `KeyboardInterrupt`. Kit's own handler exited 0. `SIGTERM`, `SIGABRT`, and `SIGSEGV` keep their default actions. Fixes #7774, supersedes #7775 and #6531. ### Constructing runtime objects after launch Importing `pxr.Usd` before Kit starts makes Kit fail to start. Scripts therefore import only config classes and construct runtime objects with `instantiate(cfg)` (#8085), which resolves the config's lazy `class_type` after the runtime is up. - `ManagerBasedEnvCfg` and `ManagerBasedRLEnvCfg` gain `class_type`. - The 21 task configs with a custom env class name it. A registry test checks that all 126 match their registered entry point. - **Fixed:** `{DIR}` in an inherited `class_type` resolved against the module of a subclass missing `@configclass`, so the call failed with `ModuleNotFoundError`. ### Compatibility in this PR `isaaclab.app.AppLauncher` is deprecated. It now starts Kit through `launch_simulation`, so develop's scripts and tests run unchanged here. It still offers `is_available`, `has_gui`, and `has_window`, which unmigrated scripts use. The migration PR moves every caller and reduces it to an init-only shim. `isaaclab.test.utils.launch_test_simulation(**launcher_args)` starts Kit for a test module through `launch_simulation`. It picks the test device by default. ### Behavior changes - **Removed** (breaking; each only restated an invariant or forwarded elsewhere): - `sim_utils.update_stage`: the simulation's reset, step, and render process stage changes; - `sim_utils.is_current_stage_in_memory`: compare against `SimulationContext.instance().stage`; - `sim_utils.show_stage_in_viewport`: use `isaaclab_physx.app.show_stage_in_viewport`; - `SettingsManager.initialize_carb_settings` (and the module-level function) and `is_omniverse_mode`: `KitLauncher` sets the settings backend, and `has_kit()` checks for Kit; - `SceneDataProvider.usd_stage` / `get_usd_stage`; - the hidden `--cpu` flag: use `--device cpu`. - **Replicator seeding:** the Replicator event terms seed it with `env.cfg.seed` when they are built; `_get_replicator()` only loads it. Calling `env.seed(x)` later no longer reseeds Replicator. - **Kit startup marker** is now `[ISAACLAB] KitLauncher initialization complete`, and `tools/conftest.py` is updated to match. - **SIGTERM** ends the process immediately, without a Kit teardown. - **Warp environments:** `render_mode="rgb_array"` now warns and returns `None`, matching the core environments. Net: +2,326 / −3,045 across 154 files. ## Type of change - Breaking change (`AppLauncher` deprecated; Kit-coupled helpers removed; Kit exit and seeding behavior changed) ## Validation All runs on the display GPU (GPU 0), with develop's scripts and tests unchanged unless listed here. - **Launcher, app, and config tests:** - `isaaclab/test/app` (26) and `isaaclab_physx/test/app` (88); - `test_runtime_compatibility` (27), visualizer intent (8), distributed device (13), `test_preset_kit_decision` (6), `test_preset_cli` (11); - `test_configclass` (37), `test_sb3_leapp_export`, and the CLI tests (234). - **Kit tests through the deprecated adapter:** - `test_simulation_context` (30), PhysX `test_rigid_object` (44), `test_camera` (21); - stage utils (15), stage in memory (3), `test_events` (4), texture randomization (2), `test_manager_based_env` (4). - **Training:** `train.py --task Isaac-Cartpole --max_iterations 2` exits 0 with `isaacsim_physx` and `newton_mjwarp`. - `physics=physx` (kitless ovphysx) fails with an observation batch mismatch. - Develop `93b6e7432` fails the same way, so it is unrelated to this PR. - **Every script under `scripts/` and `examples/`**, compared against develop with the same invocation: - **Scripts outside the standalone matrix (78):** same outcome on develop and here. 52 run, 19 are blocked by an external resource (datasets, XR, Ray, gr00t, robomimic, moviepy), and 7 fail identically on develop. - **Standalone matrix (361 cases):** no regressions. The only case that first looked new was an old-style script with `--visualizer newton_rtx`. Kit and OVRTX cannot share a process, so the adapter now skips that visualizer with a warning, as Kit did on develop. - **Newton-based visualizers next to Kit PhysX:** those cases fail on develop too, when Newton builds its model from USD. - **After rebasing onto the latest develop:** the state machines, `bin_packing`, and Cartpole training still run. - **Known pre-existing failure:** `test_visualizer_integration_newton` has a flat-frame check that fails identically on develop. - Pre-commit passes. The changelog check only flags fragments from other PRs, because it compares against the fork's stale `develop`. ## Release backport - [x] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` ## Checklist - [x] I have read and understood the contribution guidelines - [x] I have run the pre-commit checks - [x] I have made corresponding changes to the documentation (API pages for `isaaclab.app`, `isaaclab_physx.app`, `isaaclab_ov.app`) - [x] My changes generate no new warnings - [x] I have added changelog fragments for every touched package (cherry picked from commit c9f9e22)
Description
AppLaunchercurrently registers_abort_signal_handle_callbackas theSIGTERM handler. The callback calls
SimulationApp.close()and then returns.Because a Python signal handler replaces SIGTERM's default termination
behavior, Python execution may continue after the callback returns. During
distributed training shutdown, this can leave a worker alive after the
TCPStore has exited, causing repeated ProcessGroupNCCL and TCPStore
Broken pipeerrors.This PR registers a dedicated SIGTERM handler that raises:
For SIGTERM, this produces exit status
143. It terminates the Python processwhile preserving normal
finallyandatexitcleanup paths.SIGABRT and SIGSEGV remain registered to the existing abort callback, so their
behavior is unchanged.
This is the minimal standalone fix for the SIGTERM defect identified in #5886.
It does not include that PR's SIGHUP handling, shutdown watchdog, or force-exit
mechanism. PR #5933 addresses renderer/Fabric multi-GPU contention and does not
modify SIGTERM handling.
This change introduces no new dependencies and does not modify any public API.
Fixes #6530
Related to #5886.
Implementation
_terminate_signal_handle_callback.SystemExit(128 + signum)from the dedicated handler.isaaclabchangelog fragment.CONTRIBUTORS.md.This PR does not add SIGHUP handling, timeouts, watchdogs, SIGKILL behavior, or
renderer/Fabric configuration changes.
Type of change
Validation
Regression test
With the regression test applied to
origin/developbefore the productionchange:
On this branch:
The test verifies that the SIGTERM handler raises
SystemExitwith exit status143.Distributed integration test
The upstream Isaac Lab RSL-RL training script was run with two GPUs and two
ranks:
Both ranks initialized successfully and entered training. After SIGTERM was
sent to the
torchrunparent:torchrunforwarded SIGTERM to both workers.Broken pipemessages were emitted.torchrunreported one top-levelSignalExceptionfor the external SIGTERM,which is expected when the launcher itself is externally terminated.
The test environment used Isaac Sim 6.0.1.0, PyTorch 2.10.0+cu128, and
CUDA 12.8.
Changelog validation
Result:
Formatting and lint
All configured pre-commit hooks passed, and no files were modified.
Screenshots
Not applicable. This change affects process lifecycle and is validated through
automated tests and distributed shutdown behavior.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/isaaclab/changelog.d/CONTRIBUTORS.md