[DO-NOT-MERGE][MGPU][TEST] Integration bundle: device kwarg + kitless newton + bounded shutdown + Kit-pin

[hujc7]

Purpose

Diagnostic-only integration PR. Bundles every fix from the multi-GPU PR chain
onto a single branch and runs the multi-GPU CI workflow end-to-end against the
two failure-prone files (newton + physx test_articulation) to confirm the
chain as a whole is green.

DO NOT MERGE. Each individual fix has its own PR; this branch is for
A/B validation only.

What's bundled

• [CI] Cross-platform — Part 4: Multi-GPU pytest workflow with cuda:1 coverage #5823 + [CI] Multi-GPU pytest: dynamic work-stealing + shutdown-hang stack capture #5875 (base via the multi-gpu-dynamic-sharding branch)
• [MGPU] Sim: honor device kwarg over sim_cfg.device in build_simulation_context #5881 — Sim: honor device kwarg over sim_cfg.device
• [MGPU] Tests: make newton-only tests kitless (drop AppLauncher boot) #5883 — Tests: make newton-only tests kitless
• [MGPU] App: bounded shutdown — SIGHUP handler + force-exit on hang #5886 — App: bounded shutdown (SIGHUP handler + ISAACLAB_FORCE_EXIT_TIMEOUT)
• [MGPU] App: pin Kit renderer to single GPU under ISAACLAB_PIN_KIT_GPU #5933 — App: pin Kit renderer to single GPU (ISAACLAB_PIN_KIT_GPU)

Workflow wiring

• -e ISAACLAB_PIN_KIT_GPU=1 (validated, 3x green at 20:46 PDT)
• -e ISAACLAB_FORCE_EXIT_TIMEOUT=30 (new — exercises [MGPU] App: bounded shutdown — SIGHUP handler + force-exit on hang #5886 in CI for the first time)

Test narrowing (TEMP)

The discover step now keeps only test_articulation.py (newton + physx
variants) so each CI iteration is bounded by the file that triggers the
upstream hang. Reverts to the full discovered set before any merge.

What we expect to see

• Both newton AND physx test_articulation run on cuda:1/2/3
• 3 shards complete in ~10-15 min each
• No SIGHUP, no shutdown_hang, no "Stage already attached"
• If ISAACLAB_FORCE_EXIT_TIMEOUT=30 triggers (it shouldn't with PIN_KIT_GPU
  in place), we see the new diagnostic line [isaaclab.app] ISAACLAB_FORCE_EXIT_TIMEOUT=30s expired during atexit_close; libc.kill(self, SIGKILL)

[isaaclab-review-bot]

Integration Bundle Review — Multi-GPU CI Infrastructure

This diagnostic PR bundles several interconnected fixes for multi-GPU CI. Since it's marked DO NOT MERGE and serves as A/B validation for the individual PRs in the chain (#5881, #5883, #5886, #5933), this review focuses on technical observations and potential improvements for when these land as individual PRs.

---

Overall Assessment

The architecture is well-designed: the scope ∩ runtime device selection model, dynamic work-stealing queue, Kit GPU pinning, and bounded shutdown form a coherent multi-GPU CI strategy. The commit history demonstrates rigorous iteration (reproducing failures, isolating causes, validating fixes). Several items below are worth addressing in the individual PRs before they land.

---

Findings

1. 🔴 Force-exit timer uses SIGKILL with no JUnit cleanup guarantee

File: source/isaaclab/isaaclab/app/app_launcher.py (lines ~70-100)

The _arm_force_exit_timer daemon thread issues libc.kill(os.getpid(), 9) (SIGKILL) after timeout. While the docstring notes "callers must have already written their JUnit XML before triggering close()", the atexit hook can fire before the test runner finishes writing XML:

def _atexit_close(app=self._app, force_exit_seconds=force_exit_seconds):
    if force_exit_seconds > 0:
        _arm_force_exit_timer(force_exit_seconds, "atexit_close")
    with contextlib.suppress(Exception):
        app.close()

If atexit fires during a normal exit sequence (not just a hang) and app.close() takes longer than the timeout, SIGKILL will terminate mid-JUnit-write. Consider either: (a) gating the timer behind a _is_hang_scenario flag only set during signal handling, or (b) flushing/closing the JUnit file descriptor before arming the timer in the atexit path.

---

2. 🟡 _claim_queued_file has a TOCTOU window on NFS/network mounts

File: tools/conftest.py (lines ~396-430)

The work-queue claim uses flock which works correctly on local filesystems and overlayfs mounts. However, if the CI runner's workspace is on NFS (or any filesystem where flock degrades to advisory-only), two shards could claim the same file. The current Docker volume mount strategy (-v "$queue_dir:/mgpu:rw") is a host-local tmpfs mount, so this is fine today — but worth documenting the assumption inline so a future refactor doesn't inadvertently break it.

---

3. 🟡 build.yaml has a hardcoded run_docker_tests: 'false' that must be reverted

File: .github/workflows/build.yaml (lines 77-79)

run_docker_tests: 'false'

The PR description and commit messages acknowledge this is temporary, but there's no CI check or failing assertion that would catch it if someone forgets. The individual PRs that carry pieces of this should not inherit this line. Consider adding a # TEMP: comment pattern that a pre-commit hook or CI lint checks for.

---

4. 🟡 ISAACLAB_SIM_DEVICE env var override in AppLauncher may surprise users

File: source/isaaclab/isaaclab/app/app_launcher.py (lines ~1093-1100)

if not device_explicitly_passed:
    env_device = os.environ.get("ISAACLAB_SIM_DEVICE")
    if env_device:
        device = env_device
        launcher_args["device"] = env_device

This silently changes the boot device for any script that doesn't pass device= explicitly to AppLauncher. A user who sets this env var for one workflow but then runs interactive scripts in the same shell may get unexpected behavior. The design is correct for CI but warrants mention in the changelog/docs that this env var is for CI automation, not general use. The existing changelog fragment doesn't call out this "ambient override" characteristic clearly.

---

5. 🟡 _create_fixed_joint_to_world may produce incorrect joint placement for non-identity parent transforms

File: source/isaaclab/isaaclab/sim/schemas/schemas.py (lines ~137-192)

The joint's LocalPos0 and LocalRot0 (world-frame body0 pose) are set from the articulation prim's world transform. This matches the existing omni.physx.scripts.utils.createJoint behavior for the single-selection case. However, the original helper also handles edge cases like prims with non-default pivot points and USD timeSampled transforms. The new implementation uses UsdGeom.XformCache() with no explicit time argument, defaulting to Usd.TimeCode.Default(). If any downstream caller ever sets time-varying transforms before calling modify_articulation_root_properties, the joint will be placed at the default time's pose. This is unlikely to matter today but is a subtle semantic narrowing.

---

6. 🟡 build_simulation_context default change is a breaking API change

File: source/isaaclab/isaaclab/sim/simulation_context.py (lines ~960-1007)

Changing device: str = "cuda:0" to device: str | None = None is a public API change. Callers that relied on the previous default getting "cuda:0" when passing sim_cfg=None will now get SimulationCfg's default device (which happens to also be "cuda:0" today, but if SimulationCfg ever changes its default, behavior diverges). The migration is safe right now but should be noted as a breaking change in the changelog fragment.

---

7. 🟢 Newton torch.cuda.set_device + wp.set_device pins are correct but could race

File: source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py (lines ~979-990, ~1255-1265)

The device pinning in start_simulation and initialize_solver is the right fix for issue #5132. The import torch inside the method body is fine (avoids import-time side effects), and pinning both torch and warp before allocations is necessary. One minor concern: if start_simulation and initialize_solver could theoretically be called from different threads (unlikely in Newton's architecture but worth noting), the global torch.cuda.set_device is process-wide and not thread-safe. A CUDA context per-thread approach would be more robust but is overkill for the current single-threaded usage.

---

8. 🟢 DIAGNOSTIC: test-multi-gpu-pytest.yaml narrow step should be clearly gated

File: .github/workflows/test-multi-gpu-pytest.yaml (step "TEMP narrow to test_articulation only")

The narrow step overrides the discovered file list with only test_articulation.py variants. This is correctly labeled as diagnostic/temp and should be removed before the workflow is used in production. The implementation is clean — it outputs to narrow.outputs which is consumed downstream, making revert straightforward.

---

9. 🟢 Signal handler robustness: sys.exit() in signal handler is safe here but non-standard

File: source/isaaclab/isaaclab/app/app_launcher.py (lines ~1510-1528)

def _abort_signal_handle_callback(self, signum, frame):
    ...
    with contextlib.suppress(Exception):
        self._app.close()
    sys.exit(128 + signum)

Calling sys.exit() from a signal handler works in CPython (it raises SystemExit which unwinds normally) but is technically not async-signal-safe. In practice this is fine because Python signal handlers run between bytecodes (not during C code), and the previous behavior (returning from the handler without exiting) was strictly worse. The design is correct.

---

Minor Nits

• test_views_xform_prim.py: Several tests still have a manual if device == "cuda" and not torch.cuda.is_available(): pytest.skip(...) guard that's now redundant with test_devices() handling — the helper already excludes unavailable devices. These guards could be cleaned up in a follow-up.

• _expand mask grammar edge case: A mask like "0001" on a 3-device host truncates to [False, False, False] (per the test test_expand). This is intentional (documented) but slightly surprising — a runtime mask pointing past the host's capacity silently becomes empty rather than raising. The ValueError in test_devices() catches the "no devices available" case downstream, so this is fine but worth documenting in the mask grammar section.

---

Summary

Severity	Count
🔴 High	1
🟡 Medium	5
🟢 Low/Info	3

The PR achieves its diagnostic goal. The individual PRs should address the force-exit cleanup ordering (finding #1) and the build.yaml revert safeguard (finding #3) before landing. The build_simulation_context API change (finding #6) needs changelog coverage as a breaking change. Everything else is either informational or a follow-up improvement.

---

Update (commit fd55ced): The new commits revert several components that were part of the original review:

• ✅ Finding #1 (Force-exit timer): Entirely removed — the _arm_force_exit_timer, _parse_force_exit_seconds, and ISAACLAB_FORCE_EXIT_TIMEOUT mechanism are gone. Original concern is moot.
• ✅ Finding #4 (ISAACLAB_SIM_DEVICE override): Removed from _resolve_device_settings. Original concern is moot.
• ✅ Finding #5 (_create_fixed_joint_to_world): Removed; reverted to omni.physx.scripts.utils.createJoint. Original concern is moot.
• ✅ Finding #9 (Signal handler sys.exit): Handler reverted to bare self._app.close() without sys.exit(128 + signum). Original concern is moot.
• The SIGHUP handler registration is also removed.
• Changelog fragments for force-exit and kitless-newton are deleted.
• Newton tests (test_articulation.py, test_rigid_object_collection.py) reverted to standard AppLauncher(headless=True) boot (no longer kitless).

New change in tools/conftest.py: The JUnit report filename now uses a full-path slug (report_slug = str(test_file).replace("/", "__")) instead of just file_name, preventing collisions when same-basename test files from different packages run concurrently. This is a correct and clean fix — no issues.

Remaining findings still applicable: #2 (flock/NFS assumption), #3 (build.yaml hardcoded false), #6 (build_simulation_context API change), #7 (Newton device pinning), #8 (narrow diagnostic step). These were not touched by the new commits.

⚠️ Regression note: The signal handler (_abort_signal_handle_callback) now only calls self._app.close() without exiting afterward. If a SIGTERM/SIGABRT is received, after close() returns, execution resumes from the interrupted frame — the process does not terminate. The pre-bundle behavior had the same issue, so this is not a regression introduced by this PR, but it is worth noting that finding #9's improvement (explicit exit after close) was removed along with the force-exit code.

---

Update (commit 8344a02): Single change in tools/conftest.py — adds os.makedirs("tests", exist_ok=True) before writing the full report XML. This prevents a FileNotFoundError when a shard claims zero files from the work queue (the tests/ directory would not have been created by per-test JUnit outputs). Clean, correct fix — no issues.

Remaining findings still applicable: #2 (flock/NFS assumption), #3 (build.yaml hardcoded false), #6 (build_simulation_context API change), #7 (Newton device pinning), #8 (narrow diagnostic step).

---

Update (a782ec0): Single change removes the "TEMP narrow to test_articulation only" diagnostic step from test-multi-gpu-pytest.yaml and re-wires INCLUDE_FILES/PATHS env vars to reference steps.discover.outputs directly.

• ✅ Finding #8 (Diagnostic narrow step): Removed. The workflow now runs the full discovered test matrix. Clean removal, no issues.

Remaining findings still applicable: #2 (flock/NFS assumption), #3 (build.yaml hardcoded false), #6 (build_simulation_context API change), #7 (Newton device pinning).

---

Update (6ff3b04): Major refactor of test-multi-gpu-pytest.yaml — collapses the N-container-per-shard architecture into a single container hosting N parallel subshells. Key changes:

• Shard count now derived dynamically from nvidia-smi -L inside the container (no more CUDA_SHARDS build output)
• MIG-mode detection on the host gates CUDA_VISIBLE_DEVICES passthrough
• Per-shard HOME (/tmp/isaaclab-ci-home-${cuda}) provides writable isolation without per-shard Docker volume mounts
• Torch device-count cross-check caps shards to what's actually addressable

This is a clean simplification that eliminates cross-container race conditions on the shared workspace mount and ~30s of Docker init per shard. No new issues introduced. The queue/flock mechanism is unchanged.

Remaining findings still applicable: #2 (flock/NFS assumption — still correct since mount is host-local), #3 (build.yaml hardcoded false), #6 (build_simulation_context API change), #7 (Newton device pinning).

---

Update (fb7eb10): Adds container-level HOME=/tmp/mgpu-base-home and PYTHONUSERBASE=/tmp/mgpu-pyuserbase env vars to the Docker run, with corresponding mkdir -p. This fixes pip --user installs being invisible to per-shard subshells (which override HOME for cache isolation but inherit PYTHONUSERBASE). Clean fix for a real issue — the image runs as a host UID with no /etc/passwd entry, so HOME defaults to unwritable /root. No new issues.

Remaining findings still applicable: #2 (flock/NFS assumption), #3 (build.yaml hardcoded false), #6 (build_simulation_context API change), #7 (Newton device pinning).