[Backport] PRs #7228 and #7152 to release/3.0.0 - #7425
Merged
Conversation
A test that **crashes** reports a traceback, because `PYTHONFAULTHANDLER=1` installs `faulthandler` for `SIGSEGV` and friends. A test that **hangs** reports nothing. Hang *detection* already works — `tools/conftest.py` catches three kinds: | Kind | Trigger | Constant | |---|---|---| | `startup_hang` | no `AppLauncher initialization complete` / `collected ` marker | `STARTUP_DEADLINE = 120` s | | `timeout` | wall clock exceeds the per-file budget | `DEFAULT_TIMEOUT = 1000` s | | `shutdown_hang` | JUnit report written, process still alive | `SHUTDOWN_GRACE_PERIOD = 30` s | The problem is what happens next: all three escalate straight to `os.killpg(pgid, SIGKILL)`. `SIGKILL` cannot be caught, so nothing gets a chance to dump. The report carries `nvidia-smi`, `ps auxf`, `dmesg` and the last `-v` test 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**. The runner asks the process where it is stuck before killing it. - **`tools/hang_dump.py`** (new) — pytest plugin registering `SIGUSR1` via `faulthandler.register()`, writing 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`. 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 `SIGKILL`ed — the only case it is ever written in. The first revision of this PR wrote to `sys.__stderr__` and produced no dump in CI at all, which #7153 caught. `tools/ovrtx_log.py` keeps the renderer log in a file for the same reason. - **`conftest.py`** — loads it via `pytest_plugins`, covering every suite. - **`tools/conftest.py`** — `_dump_hung_process_stacks()` signals the process and drains its output before the existing `SIGKILL`, prepending the result to `pre_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_diag` is 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_diag` already flows into the `startup_hang` and `timeout` reports 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_diagnostics` truncates with `diag[:10000]`, so the stack survives and the system tables get trimmed instead. The dump reached CI only through `pre_kill_diag`, which `_get_diagnostics` truncates to 10 000 characters — and a Kit process has enough threads to exceed that, so the part worth reading was the part being cut. The dumps are now written to `tests/hang-dumps/` and collected as a `hang-dumps-<container>` artifact, alongside the existing `comparison-images` and `ovrtx-logs` ones. The reports and the job log still carry the (truncated) dump; the artifact is the whole thing. It is absent unless something hung, which `if-no-files-found: ignore` already covers. `SIGTERM` and `SIGABRT` are unusable here. `AppLauncher` binds both to `_on_abort_signal`, which calls `SimulationApp.close()` — itself what a shutdown hang is stuck inside — so either would re-enter the hang. Binding `SIGABRT` also displaces `faulthandler`'s own handler. A Python-level `signal` handler 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.multigpu` documents 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. `SIGUSR1` is unused anywhere in `source/`, `tools/`, `scripts/`, `.github/`. Against a process blocked in `threading.Event().wait()`: ``` === HANG STACK DUMP (all threads) === ----- dump 1 of 2 ----- Current thread 0x000073f05cc49080 (most recent call first): File "/usr/lib/python3.12/threading.py", line 355 in wait File "<string>", line 8 in wedged_call ``` - Bug fix (non-breaking change which fixes an issue) - [x] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` Not applicable. - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there One regression test added to `test_test_orchestrator_result_handling.py`, exercising the **real** `capture_test_output_with_timeout` against a genuinely hung child. It asserts every property of the dump — the stack names the hung call, more than one dump is taken, and the stack precedes the system tables — because each is a facet of the same `pre_kill_diag` and each costs a real timeout to reproduce separately. Confirmed it fails without the change on the meaningful assertion (`assert 'HANG STACK DUMP' in ''`), not on a missing constant. It skips on Windows, where the orchestrator's process handling (`select` on pipes, `os.killpg`, `start_new_session`) is unavailable. The three hang tests each waited out a 15 s timeout, costing the Isaac Sim suite about a minute for coverage that needs no GPU. `test_test_orchestrator_result_handling.py` is now listed in `TESTS_TO_SKIP`, so the whole file is out of CI; the skip entry records the command to run it on demand when changing the orchestrator. Note this also takes the file's pre-existing tests (crash-journal blaming, retry semantics, renderer-log bounds, result summary) out of CI. The hang tests are also folded into one, so a local run is not a minute either. The remaining test still has to sit through a real timeout, so that timeout is sized to what the child actually needs: measured ~1.4 s idle and ~2.6 s with eight spawned at once, against a budget of 8 s. Whole file measured at 15 passed, 1 skipped in 2.9 s on Windows. correctness test so CI exercises the dump against a live Kit process with all its threads running. Deliberately excluded: - **No native C++ frames** (no `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. - **No change to pass/fail classification** — `passed (shutdown hanged)` stays a pass, so nothing goes red as a side effect. - **Still uncovered:** `docker wait` in `run_tests.sh` has no deadline, so a container hanging *above* pytest is caught only by the 180-minute job timeout. (cherry picked from commit 0f5644a)
kellyguo11
marked this pull request as ready for review
August 31, 2026 03:18
kellyguo11
requested review from
AntoineRichard,
StafaH,
fatimaanes,
hujc7,
kellyguo11,
myurasov-nv,
ooctipus and
pbarejko
as code owners
August 31, 2026 03:18
Contributor
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
Contributor
Greptile SummaryThis backport adds SIGUSR1-triggered stack capture for hung pytest processes and uploads full renderer logs and hang dumps from CI. However, its conflict resolution also replaces the release branch's full test orchestrator rather than integrating the diagnostic path into it.
Confidence Score: 4/5This PR should not merge until the release branch's test orchestrator is restored and the hang-dump behavior is integrated without deleting its existing execution and reporting contracts. The conflict resolution removes the release branch's test discovery, per-file execution, retries, sharding, crash recovery, report aggregation, and exit-code handling; the new artifact action references also merit non-blocking integrity hardening. Files Needing Attention: tools/conftest.py, source/isaaclab/test/cli/test_test_orchestrator_result_handling.py, .github/actions/run-tests/action.yml
|
| Filename | Overview |
|---|---|
| tools/conftest.py | Adds stack-dump signaling but accidentally deletes the release branch's complete test orchestration implementation. |
| tools/hang_dump.py | Introduces a POSIX-only pytest plugin that registers a C-level SIGUSR1 faulthandler and writes all-thread dumps to a configured file. |
| .github/actions/run-tests/action.yml | Adds renderer-log and hang-dump artifact uploads, but both new steps use a mutable action tag. |
| .github/actions/run-tests/run_tests.sh | Copies the new diagnostic directories from bind-mounted or stopped-container test paths into the reports directory. |
| source/isaaclab/test/cli/test_test_orchestrator_result_handling.py | Replaces broad release-orchestrator regression coverage with a real-time hang-dump integration test that is excluded from normal CI. |
| conftest.py | Registers the new hang-dump plugin alongside the existing renderer-log plugin. |
| tools/test_settings.py | Excludes the real-time hang-dump integration test from normal CI execution. |
Sequence Diagram
sequenceDiagram
participant CI as CI runner
participant O as Timeout helper
participant P as pytest process
participant H as hang_dump plugin
CI->>O: Start pytest with dump path
O->>P: Spawn process
P->>H: Register SIGUSR1 handler
O->>O: Detect timeout/startup/shutdown hang
O->>P: SIGUSR1
H-->>O: Write all-thread stack dump
O->>P: SIGUSR1 again
H-->>O: Append second stack dump
O->>P: SIGKILL process group
O-->>CI: Return diagnostics
Reviews (1): Last reviewed commit: "Dump thread stacks before killing a hung..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "wall_time": wall_time, | ||
| }, | ||
| was_failure, | ||
| def capture_test_output_with_timeout(cmd, timeout, env, startup_deadline=0, report_file=""): |
Contributor
There was a problem hiding this comment.
When CI invokes tools/conftest.py as the release branch's standalone test orchestrator, this rewrite provides only the low-level timeout helper, removing test discovery, per-file execution, retries, sharding, crash recovery, report aggregation, and exit-code handling and causing the configured suite and reports to be lost. Integrate hang-dump capture into the existing release orchestrator instead of replacing it.
| - name: Upload OVRTX renderer logs | ||
| if: always() | ||
| id: upload-ovrtx-logs | ||
| uses: actions/upload-artifact@v7 |
Contributor
There was a problem hiding this comment.
The two new artifact-upload steps execute actions/upload-artifact@v7 through a mutable tag, allowing a repointed tag to run unreviewed code with the workflow's access and alter or expose CI artifacts; pin both new steps to a reviewed full commit SHA.
How this was verified: Both changed uses entries reference actions/upload-artifact@v7 rather than immutable commit SHAs.
![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 conflict resolution is not a faithful backport: it replaces the release branch’s per-file CI test orchestrator with a small subprocess-capture module. This removes test selection, retries, crash-journal recovery, JUnit aggregation, and exit-code handling, while also leaving hang-dump artifacts without a CI producer and weakening subprocess cleanup.
- Design and architecture: The SIGUSR1/faulthandler stack-dump design is appropriately isolated in tools/hang_dump.py, but its integration deletes the architecture it is meant to extend. Restore the release-branch tools/conftest.py orchestrator and layer the dump request and collection into its existing per-pass subprocess lifecycle.
- API: The new hang_dump helper surface is internally consistent, but the established CI contract is broken. run_tests.sh still supplies filter, include-file, node-ID, shard, and marker inputs and expects tests/$result_file; the consumers that implemented those behaviors were removed from tools/conftest.py. The per-test environment also never receives ISAACLAB_HANG_DUMP in CI, so the newly uploaded hang-dumps path has no producer.
- Implementation: The timeout path can request two bounded stack dumps, but only when ISAACLAB_HANG_DUMP is manually supplied. In addition, the normal-exit path now performs an unbounded process.communicate() and no longer kills surviving members of the subprocess process group, allowing inherited pipes from Kit children to block indefinitely and orphan processes to leak into later tests. Restore the bounded drain and process-group cleanup along with the original orchestrator.
Needs rework. Posted 3 actionable findings inline.
Automated review; human maintainers own approval decisions.
| "wall_time": wall_time, | ||
| }, | ||
| was_failure, | ||
| def capture_test_output_with_timeout(cmd, timeout, env, startup_deadline=0, report_file=""): |
Contributor
There was a problem hiding this comment.
🔴 Critical · Design Architecture — Backport deletes the whole test orchestrator
The resolved file ends after capture_test_output_with_timeout: pytest_ignore_collect, pytest_sessionstart, run_individual_tests, _run_one_pass, retries, crash-journal recovery, JUnit assembly and exit-code mapping are gone, and _capture_system_diagnostics keeps only ps auxf. run_tests.sh still runs pytest against this plugin and expects tests/$result_file plus filter/node-id/shard handling. Restore the release-branch orchestrator and add only the hang-dump hook on top.
| stdout_data = b"" | ||
| stderr_data = b"" | ||
| dumps = [] | ||
| dump_file = env.get(hang_dump.DUMP_PATH_ENV_VAR, "") |
Contributor
There was a problem hiding this comment.
🟡 Warning · Implementation — Hang dump has no producer in CI
_dump_hung_process_stacks returns early unless the subprocess env carries ISAACLAB_HANG_DUMP, but the per-pass env construction that would set it was removed with the orchestrator; only the new test sets it manually. Consequently no dump file is written, tests/hang-dumps/ never exists, and the added copy block and upload step in the CI action always find nothing. Set an absolute per-test dump path where the subprocess env is built.
| stdout_data += stdout_chunk | ||
| stderr_data += stderr_chunk | ||
|
|
||
| remaining_stdout, remaining_stderr = process.communicate() |
Contributor
There was a problem hiding this comment.
🟡 Warning · Implementation — Unbounded final drain, no orphan reaping
On normal exit this now calls process.communicate() with no timeout and no longer sends SIGKILL to the process group. A surviving Kit or Isaac Sim child that inherited the stdout/stderr pipes keeps this call blocked outside the timeout loop indefinitely, and orphans leak into the next test. Restore the bounded drain and the post-exit os.killpg(pgid, signal.SIGKILL) cleanup.
The OVRTX renderer log reaches a diagnosis two ways today, and both are bounded, because what they report lands in the job log and in the JUnit XML: the autouse fixture in `tools/ovrtx_log.py` replays the byte range a test appended, and the orchestrator quotes a tail into the report of a run that died before it could replay anything. Both also cover the log and nothing else, so a dump a crashing renderer leaves beside it never leaves the container. This saves each test's renderer output per test and uploads it as a job artifact. - **`tools/ovrtx_log.py`** — a new `save_output()` writes the test's own byte range of the log into `<artifact dir>/<test name>.<attempt>/`, then copies the rest of the renderer's directory — the dump a crash leaves beside the log — whole. Subdirectories are skipped: the renderer logs into the system temp directory, which every other process on the machine also writes to. The attempt number keeps a retry from overwriting the attempt that failed. The log is sliced per test rather than copied whole because the renderer appends to one file for the lifetime of the process. Copying it whole would credit the second test with the first's output and every test after them with both — an artifact growing as the square of a run whose logs are already the large part. A test that adds nothing to that range saves nothing, which covers both a test that never builds a renderer and a skip that follows one that did. - **`tools/conftest.py`** — names that directory per pass through `ISAACLAB_OVRTX_LOG_DIR`, as `tests/ovrtx-logs` under the workspace root. Absolute, for the journal's reason: the save happens in a fixture, so a test that changed directory would otherwise leave its artifact under the temporary cwd. Unset by default, so a plain local `pytest` run still writes nothing beyond the replay. - **`tools/conftest.py`, abnormal termination** — the test a crash, hang, or timeout killed never reaches the teardown that saves, which leaves the one test in the run whose output anybody is going to read as the one test missing from the artifact. `_make_crash_pass_result` now saves on its behalf, named after the test the crash journal blames so it sits beside the tests that saved their own. Saved whole, since the per-test offsets went with the process, and `OSError`-suppressed, since a report matters more than an artifact. - **`run_tests.sh` / `run-tests/action.yml`** — copies that tree out of the container alongside the JUnit reports and uploads it as `ovrtx-logs-<container-name>`. The report cap goes from 64 KiB to 1 MiB while here. It exists to keep a multi-megabyte log out of the job log and the JUnit XML, and the rendering tests it was built for log a few hundred kilobytes each, so the old size cut a whole test's log down to its tail for no benefit. The saved copy covers the runs that outgrow even the new cap. The two existing tests that exercise the cap now count their filler from `LOG_LIMIT_BYTES` rather than a literal, so raising it again cannot quietly turn them into tests of a log that fits inside it. - New feature (non-breaking change which adds functionality) Not applicable. Six tests added. `source/isaaclab/test/cli/test_ovrtx_log.py`, each run against a nested pytest session driving a fake renderer: - each test's log is saved under a directory of its own name, the replay still happens alongside it, and one test's saved log holds none of another's output - a dump written beside the log is saved with it, while a sibling subdirectory is left alone - a test that never builds a renderer saves nothing - a test that adds nothing to a log an earlier test left behind saves nothing `source/isaaclab/test/cli/test_test_orchestrator_result_handling.py`: - the pass hands its subprocess an absolute `ISAACLAB_OVRTX_LOG_DIR` under the reports directory - a crash (`-11`) and a timeout (`-9`, `kill_reason="timeout"`) each save the blamed test's renderer log, named after that test — parametrized over both Run locally on Windows: `10 passed, 1 skipped` for `test_ovrtx_log.py` (the skip is the pre-existing POSIX-only `/dev/stdout` case) and `14 passed` for `test_test_orchestrator_result_handling.py`. - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package (do **not** edit `CHANGELOG.rst` or bump `extension.toml` — CI handles that) - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
Contributor
|
@isaaclab-review-bot review |
Contributor
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
kellyguo11
changed the title
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
2 participants
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.
Backports #7228 and #7152 to
release/3.0.0.#7152 was developed on top of #7228, which is not present on the release branch, so this PR includes both changes. The follow-up cherry-pick restores the full test orchestrator after the original automated conflict resolution. For the shared code, test, and CI paths, the final PR tree matches
developimmediately after #7152; the additional changelog fragment records the release-specific backport.911dffd2331d13299b8652923df2f3aa409fb5801f2e24b8a44efc72ecdf2cc30fc87bf55d445a4f0f5644a3a701c0dda71121169f7092720227fc5cf03794a2bade871242c982b6b170b0879acd4af647563a17ae98dab273a55c82b31727edecc4b988