Dump thread stacks before killing a hung test - #7152
Open
mataylor-nvidia wants to merge 2 commits into
Open
Conversation
A test that crashes reported a traceback, because PYTHONFAULTHANDLER=1 installs faulthandler for SIGSEGV and friends. A test that hung reported nothing: the runner detects the hang and kills the process group with SIGKILL, which cannot be caught, so no handler ever ran. The report carried system tables and the last -v test name, and nothing that points at the hung code. The runner now asks the process where it is stuck before killing it. tools/hang_dump.py registers SIGUSR1 with faulthandler.register, and capture_test_output_with_timeout signals the process and drains the dump into pre_kill_diag, which already flows into the startup_hang, timeout, and shutdown_hang reports. The dump is taken twice: identical stacks seconds apart are what tell a wedged process from a slow one. SIGTERM and SIGABRT cannot be used for this. AppLauncher binds both to a handler that calls SimulationApp.close(), which is itself what a shutdown hang is stuck inside, so either would re-enter the hang. A Python-level signal handler would not run regardless, since those execute between bytecodes and a thread wedged in a native Kit, CUDA, or renderer call never returns to the interpreter loop. faulthandler.register installs a C-level handler that walks every thread from inside the signal handler, so it reports a process whose GIL will never be released.
mataylor-nvidia
requested review from
AntoineRichard,
StafaH,
fatimaanes,
kellyguo11,
ooctipus and
pbarejko
as code owners
August 18, 2026 21:39
6 tasks
Contributor
Greptile SummaryThe PR adds an on-demand faulthandler stack-dump plugin and asks hung pytest children for two thread dumps before the orchestrator kills their process group.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defect identified. The new signal handler is installed before the repository’s ordinary test and Kit startup paths, the orchestrator drains stack output before its existing process-group kill, and the diagnostic data is correctly propagated through the changed missing-report paths. Important Files Changed
Sequence DiagramsequenceDiagram
participant Runner as Test orchestrator
participant Child as Pytest child
participant Handler as faulthandler
participant Report as Diagnostics/report
Child->>Handler: pytest_configure registers SIGUSR1
Runner->>Child: Monitor startup, timeout, and shutdown
Runner->>Runner: Detect hang and capture system diagnostics
loop Two dump passes
Runner->>Child: SIGUSR1
Handler-->>Runner: All Python thread stacks on stderr
Runner->>Runner: Drain and stream stdout/stderr
end
Runner->>Child: SIGKILL process group
Runner->>Report: Prepend stack dump to diagnostics
Reviews (1): Last reviewed commit: "Dump thread stacks before killing a hung..." | Re-trigger Greptile |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Isaac Lab Review Bot
The SIGUSR1/faulthandler approach and diagnostic plumbing are coherent, but the stack-dump size limit conflicts with the downstream 10,000-character report limit and can remove the second dump and existing system diagnostics from JUnit output.
- Design and architecture: The plugin-based signal handler and child-PID signalling fit the existing test-runner architecture. The remaining design issue is that a dump allowed to reach 64 KiB is prepended to a diagnostic field capped at 10,000 characters, despite that field also carrying system diagnostics and two dumps being central to the design.
- API: No public or extension-facing API compatibility issue was identified. The existing six-element capture result remains unchanged, and the added private helper parameter preserves prior behavior through its default.
- Implementation: The output-draining extraction preserves the existing streaming behavior, and unsupported signal platforms are guarded. However, HANG_DUMP_LIMIT_BYTES must be aligned with the downstream _get_diagnostics limit so both requested dumps and useful system diagnostics can survive in generated reports.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| that are already failing. | ||
| """ | ||
|
|
||
| HANG_DUMP_LIMIT_BYTES = 64 * 1024 |
Contributor
There was a problem hiding this comment.
🟡 Warning · Implementation — Dump budget exceeds report truncation limit
HANG_DUMP_LIMIT_BYTES permits 64 KB, but the prepended pre_kill_diag is consumed by _get_diagnostics, which cuts at diag[:10000]. For a Kit process with many threads the dump alone exceeds that, so this cap never fires, the second dump is cut off, and the pre-existing nvidia-smi/ps auxf/dmesg sections are dropped entirely from timeout and missing-report entries. Cap the dump section well below 10000 characters so both survive.
The dump never reached CI. pytest captures at the file-descriptor level, so it has already pointed fd 2 at a temporary file of its own by the time the plugin loads; faulthandler.register(file=sys.__stderr__) stored fd 2 and wrote there. That buffer is discarded when the process is SIGKILLed, which is the only case the dump is ever written in, so a hung test still reported nothing but system tables. The dump now goes to a file named by ISAACLAB_HANG_DUMP, which the runner sets per test file and clears per attempt, mirroring the crash journal's ISAACLAB_TEST_JOURNAL. pytest does not redirect it, and the runner reads it after the process is gone. This is the same reason tools/ovrtx_log.py keeps the renderer log in a file. The regression tests missed this because they hung a bare `python script.py` child, which has no capture, so the dump reached stderr and they passed. They now hang a real `python -m pytest` child, reproducing the CI failure: against the previous implementation all three fail on `assert 'HANG STACK DUMP' in ''`. Found by the CI probe in the follow-up branch, which wedged a rendering correctness test and produced a timeout report with no stack.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A test that crashes reports a traceback, because
PYTHONFAULTHANDLER=1installsfaulthandlerforSIGSEGVand friends. A test that hangs reports nothing.Hang detection already works —
tools/conftest.pycatches three kinds:startup_hangAppLauncher initialization complete/collectedmarkerSTARTUP_DEADLINE = 120stimeoutDEFAULT_TIMEOUT = 1000sshutdown_hangSHUTDOWN_GRACE_PERIOD = 30sThe problem is what happens next: all three escalate straight to
os.killpg(pgid, SIGKILL).SIGKILLcannot be caught, so nothing gets a chance to dump. The report carriesnvidia-smi,ps auxf,dmesgand the last-vtest name — nothing pointing at the hung code.The repo already records the symptom in a skip reason: "Native hang: the per-file CI runner kills the suite after 1000s with no pytest outcome" (
source/isaaclab_tasks/test/rendering_test_utils.py).This complements the crash journal (#7005): that recovers which tests had passed when a process died; this reports where the process is stuck.
Change
The runner asks the process where it is stuck before killing it.
tools/hang_dump.py(new) — pytest plugin registeringSIGUSR1viafaulthandler.register(), writing to a file named byISAACLAB_HANG_DUMP, which the runner sets per test file and clears per attempt, mirroring the crash journal'sISAACLAB_TEST_JOURNAL. No-ops where the signal does not exist.The dump has to go to a file, not stderr. pytest captures at the file-descriptor level, so it has already redirected fd 2 by the time the plugin loads; a dump written there is discarded when the process is
SIGKILLed — the only case it is ever written in. The first revision of this PR wrote tosys.__stderr__and produced no dump in CI at all, which [DO NOT MERGE] Probe hang stack dump against a live Kit process #7153 caught.tools/ovrtx_log.pykeeps the renderer log in a file for the same reason.conftest.py— loads it viapytest_plugins, covering every suite.tools/conftest.py—_dump_hung_process_stacks()signals the process and drains its output before the existingSIGKILL, prepending the result topre_kill_diag. The fd-drain block was extracted into_drain_ready_output()so the watchdog loop and the dump path share one implementation.pre_kill_diagis now also threaded into_make_missing_report_result, so a fresh-process retry that hangs reports its stack too.The dump is taken twice — identical stacks seconds apart are what distinguish a wedged process from a slow one.
Report plumbing is otherwise unchanged:
pre_kill_diagalready flows into thestartup_hangandtimeoutreports and the retry warnings, and the drain echoes to stdout/stderr, so the stack also streams live to the job log. Prepending rather than appending matters —_get_diagnosticstruncates withdiag[:10000], so the stack survives and the system tables get trimmed instead.Why
SIGUSR1SIGTERMandSIGABRTare unusable here.AppLauncherbinds both to_on_abort_signal, which callsSimulationApp.close()— itself what a shutdown hang is stuck inside — so either would re-enter the hang. BindingSIGABRTalso displacesfaulthandler's own handler.A Python-level
signalhandler would not run regardless: those execute between bytecodes, and a thread wedged in a native Kit, CUDA, or renderer call never returns to the interpreter loop.isaaclab.cli.multigpudocuments the same constraint when reaping stragglers.faulthandler.register()installs a C-level handler that walks every thread from inside the signal handler, so it reports a process whose GIL will never be released.SIGUSR1is unused anywhere insource/,tools/,scripts/,.github/.Sample output
Against a process blocked in
threading.Event().wait():Type of change
Screenshots
Not applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists thereTesting
Three regression tests added to
test_test_orchestrator_result_handling.py, exercising the realcapture_test_output_with_timeoutagainst a genuinely hung child. Confirmed they fail without the change on the meaningful assertion (assert 'HANG STACK DUMP' in '',assert 0 > 1), not on a missing constant.14 passed on Linux; the three new ones skip on Windows, where the orchestrator's process handling (
selecton pipes,os.killpg,start_new_session) is unavailable.#7153 is a throwaway probe branched off this one, wedging a rendering correctness test so CI exercises the dump against a live Kit process with all its threads running.
Scope
Deliberately excluded:
py-spy/gdb). Python stacks stop at the C boundary, so a Kit shutdown hang reads as_close_app→SimulationApp.close()without naming the RTX/PhysX call. Still localizes the hang to a test and a call site.passed (shutdown hanged)stays a pass, so nothing goes red as a side effect.docker waitinrun_tests.shhas no deadline, so a container hanging above pytest is caught only by the 180-minute job timeout.