[Fix] AppLauncher: terminate on SIGTERM#6531
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!
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