Skip to content

[Fix] AppLauncher: terminate on SIGTERM#6531

Open
RX-02333 wants to merge 4 commits into
isaac-sim:developfrom
RX-02333:rx-02333/fix-sigterm-shutdown
Open

[Fix] AppLauncher: terminate on SIGTERM#6531
RX-02333 wants to merge 4 commits into
isaac-sim:developfrom
RX-02333:rx-02333/fix-sigterm-shutdown

Conversation

@RX-02333

Copy link
Copy Markdown

Description

AppLauncher currently registers _abort_signal_handle_callback as the
SIGTERM 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 pipe errors.

This PR registers a dedicated SIGTERM handler that raises:

SystemExit(128 + signum)

For SIGTERM, this produces exit status 143. It terminates the Python process
while preserving normal finally and atexit cleanup 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

  • Register SIGTERM to _terminate_signal_handle_callback.
  • Raise SystemExit(128 + signum) from the dedicated handler.
  • Keep the existing SIGABRT and SIGSEGV registrations unchanged.
  • Add a regression test for the SIGTERM exit status.
  • Add an isaaclab changelog fragment.
  • Add the contributor name to CONTRIBUTORS.md.

This PR does not add SIGHUP handling, timeouts, watchdogs, SIGKILL behavior, or
renderer/Fabric configuration changes.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Validation

Regression test

./isaaclab.sh -p -m pytest \
  source/isaaclab/test/app/test_signal_handling.py -q

With the regression test applied to origin/develop before the production
change:

AttributeError: type object 'AppLauncher' has no attribute
'_terminate_signal_handle_callback'

1 failed in 0.06s

On this branch:

.                                                                        [100%]
1 passed in 0.04s

The test verifies that the SIGTERM handler raises SystemExit with exit status
143.

Distributed integration test

The upstream Isaac Lab RSL-RL training script was run with two GPUs and two
ranks:

CUDA_VISIBLE_DEVICES=1,2 ./isaaclab.sh -p \
  -m torch.distributed.run \
  --standalone \
  --nnodes=1 \
  --nproc_per_node=2 \
  scripts/reinforcement_learning/rsl_rl/train.py \
  --task Isaac-Cartpole \
  --num_envs 2 \
  --max_iterations 5000 \
  --distributed \
  --viz none

Both ranks initialized successfully and entered training. After SIGTERM was
sent to the torchrun parent:

  • torchrun forwarded SIGTERM to both workers.
  • Both workers exited.
  • No training processes remained.
  • No repeated TCPStore Broken pipe messages were emitted.
  • No repeated ProcessGroupNCCL heartbeat errors were emitted.

torchrun reported one top-level SignalException for 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

./isaaclab.sh -p tools/changelog/cli.py check develop

Result:

✓ All modified packages have valid changelog fragments.

Formatting and lint

./isaaclab.sh -f

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

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation (no public documentation changes are required; a changelog fragment was added)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a changelog fragment under source/isaaclab/changelog.d/
  • I have added my name to CONTRIBUTORS.md

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>
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Jul 15, 2026
@RX-02333
RX-02333 marked this pull request as ready for review July 15, 2026 05:43
@RX-02333
RX-02333 requested a review from kellyguo11 as a code owner July 15, 2026 05:43
@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a distributed-training shutdown regression where a Python process could survive after receiving SIGTERM, because the old handler called SimulationApp.close() and returned without terminating. The new dedicated handler raises SystemExit(128 + signum), which unwinds the stack through finally blocks and existing atexit handlers (including the already-registered _atexit_close that calls SimulationApp.close()), then exits with code 143.

  • app_launcher.py: SIGTERM is now routed to _terminate_signal_handle_callback, a @staticmethod that raises SystemExit(143); SIGABRT and SIGSEGV retain the existing abort callback unchanged.
  • test_signal_handling.py: Adds a unit test confirming the callback raises SystemExit with the expected exit code 143.
  • Changelog fragment and contributor entry are included.

Confidence Score: 4/5

Safe 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

Filename Overview
source/isaaclab/isaaclab/app/app_launcher.py Replaces SIGTERM handler with a dedicated static method that raises SystemExit(128+signum), relying on the already-registered atexit handler for SimulationApp.close(); logic is correct and minimal.
source/isaaclab/test/app/test_signal_handling.py New unit test verifies that _terminate_signal_handle_callback raises SystemExit(143); tests callback behavior directly but does not verify that SIGTERM is actually registered to this handler after AppLauncher init.
source/isaaclab/changelog.d/rx-02333-fix-sigterm-shutdown.rst Changelog fragment correctly documents the SIGTERM fix.
CONTRIBUTORS.md Adds contributor name in alphabetical order.

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
Loading
%%{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
Loading

Reviews (1): Last reviewed commit: "Merge branch 'develop' into rx-02333/fix..." | Re-trigger Greptile

Comment on lines +13 to +18
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant