(release/0.3.0) Release viewport capture before stage teardown when Replicator is included

[xyao-nv]

Summary

Short description of the change (max 50 chars)

Detailed description

• Issue reported by VDR: When omni.Replicator is included, it hangs during sim shutdown after video is written, html does not output.
  Fixes:
• Stop video recording cleanly before shutting down the simulator.
• Prevent completed evaluations from hanging before the HTML report is written

Before

MP4 is written, process remains around “Closing simulation app,” and timeout eventually returns 124; index.html is absent.

After

Process exits 0, and both the MP4 and index.html are present.

timeout --signal=TERM --kill-after=30s 180s \
python isaaclab_arena/evaluation/policy_runner.py \
--viz kit --policy_type zero_action \
--record_viewport_video \
--num_episodes 20 \ 
lift_object

[greptile-apps]

Confidence Score: 4/5

The PR should not merge until viewport-recorder finalization is incorporated into the guaranteed cleanup path so an encoder or output failure cannot bypass simulator teardown.

Findings

1.  Recorder Failure Skips Teardown ▶

Summary

• Introduces compatibility cleanup for recorders and Kit/Newton capture internals.
• Adds viewport cleanup to standalone environment closure and evaluation resource teardown.
• Adds unit coverage for cleanup ordering and successful idempotent fallback release.
• The new evaluation cleanup call is outside the existing teardown guarantee, so a recorder-finalization error can skip all remaining environment cleanup.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Evaluation finishes or raises] --> B[Close policy]
    B --> C[Close viewport recorder]
    C -->|success| D[Replace simulation stage]
    D --> E[Close wrapped environment]
    E --> F[Collect garbage and clear CUDA cache]
    F --> G[Write result and HTML report]
    C -->|exception in current code| H[Remaining cleanup is skipped]
    H --> I[Resources remain and run/report may abort]

Loading

[arena-review-bot]

🟡 This suppresses the exact failure the PR is fixing

If detach() or destroy() raises, we swallow it, null the attribute anyway, and the shutdown hang comes back with nothing in the log. Could we log it instead of dropping it silently? (Needs import logging + logger = logging.getLogger(__name__) at module level.)

Suggested change

	if annotator is not None:
	with suppress(Exception):
	annotator.detach()
	capture._rgb_annotator = None
	if render_product is not None:
	with suppress(Exception):
	render_product.destroy()
	capture._render_product = None
	if annotator is not None:
	try:
	annotator.detach()
	except Exception:
	logger.warning("Could not detach the viewport RGB annotator; stage teardown may hang.", exc_info=True)
	capture._rgb_annotator = None
	if render_product is not None:
	try:
	render_product.destroy()
	except Exception:
	logger.warning("Could not destroy the viewport render product; stage teardown may hang.", exc_info=True)
	capture._render_product = None

[arena-review-bot]

🟡 Goes quiet if Isaac Lab renames these privates

Every step here is a getattr on an Isaac Lab private — _capture, _rgb_annotator, _render_product, _viewer. If any of those get renamed upstream, this function silently does nothing, the hang returns, and the new unit tests still pass because they build their own SimpleNamespace with the same names. Could we log a warning when none of the known release paths matched, so a broken shim shows up in the run log rather than as a mystery timeout?

Worth a NOTE here too, pointing at the upstream gap (neither VideoRecorder nor IsaacsimKitPerspectiveVideo/NewtonGlPerspectiveVideo has a close() in 3.0), so this whole function can be deleted once that lands.

Small thing: video_recorder has no annotation — VideoRecorder | None under TYPE_CHECKING would make it obvious whose internals we're reaching into.

[arena-review-bot]

🟡 The same cleanup is now wired up in two places

close_environment() releases the capture before teardown_simulation_app(), then calls env.close() — so by the time this override runs, _capture is already None and the stage is already replaced. It's a no-op on the evaluation path; it only does real work for the direct env.close() callers in isaaclab_arena_examples.

Would it be simpler to keep the release here only, and have close_environment() call env.close() before teardown_simulation_app()? Then one place owns it. If the current teardown-then-close order is load-bearing, a short NOTE in close_environment saying why would help — I couldn't find a reason for it in the history.

[arena-review-bot]

🔵 This branch isn't reachable against real Isaac Lab objects

Neither VideoRecorder nor the Kit/Newton capture classes define close() in 3.0, so both close() fast-paths are forward-compat only. Fine to keep, but a one-line comment saying so would stop it reading as a path that's actually exercised.

Same idea for the other test: the fake mirrors IsaacsimKitPerspectiveVideo's private field names, so naming that class in a comment tells the next person where to re-check after an Isaac Lab bump.

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

Releases the Kit/Newton viewport capture (Replicator annotator + render product) before the USD stage is replaced, so an evaluation with --record_viewport_video no longer hangs at shutdown and the HTML report gets written. The diagnosis looks right — I confirmed isaaclab.envs.utils.video_recorder.VideoRecorder and both capture classes (IsaacsimKitPerspectiveVideo, NewtonGlPerspectiveVideo) have no public close() in Isaac Lab 3.0, so a shim in Arena is genuinely needed for now. My concerns are about how quietly it can stop working, and about the fix landing in two layers.

Design, Boundaries & Scope

The cleanup is wired up both in close_environment() and in a new IsaacLabArenaManagerBasedRLEnv.close() override. On the evaluation path only the first one does anything — by the time env.close() runs, teardown_simulation_app() has already replaced the stage and _capture is None. The override only matters for the direct env.close() callers in isaaclab_arena_examples. Worth deciding on one owner (details inline).

Separately, close_viewport_video_recorder is not really about Arena's own recording config — it's a workaround for Isaac Lab internals, same shape as teardown_simulation_app. isaaclab_arena/utils/isaaclab_utils/ may be a more natural home than video/video_recording.py, and it would keep video/ from being imported by evaluation/ just for teardown. Not a blocker, just a placement question.

Findings

• 🟡 video/video_recording.py:68-75 — with suppress(Exception) swallows exactly the failure this PR fixes; log it instead of dropping it.
• 🟡 video/video_recording.py:44 — the shim rests entirely on Isaac Lab private names; an upstream rename turns it into a silent no-op and the tests won't notice. Suggest a warning when nothing matched, plus a NOTE so the shim can be deleted when upstream adds close(). Also missing a parameter type annotation.
• 🟡 environments/isaaclab_arena_manager_based_env.py:100 — duplicate cleanup at two layers; the override is a no-op on the eval path.
• 🔵 evaluation/resource_cleanup.py:37 — getattr(env, "unwrapped", env) can't fall back; gym.Env always defines unwrapped.
• 🔵 tests/test_video_recording.py:34 — the close() fast-paths are unreachable against real Isaac Lab objects today; worth a comment marking them forward-compat.

Test Coverage

Two new plain unit tests, no sim — they land correctly in Phase 1 and don't need the inner/outer pattern or markers, matching the existing test_run_execution.py style. Both are well-formed (the idempotency test genuinely exercises the second-call early return). The gap is that they only test the shim's control flow against hand-built SimpleNamespace fakes, so they can't catch the failure mode that actually matters — Isaac Lab renaming _capture / _rgb_annotator / _render_product. A runtime warning when no release path matches is probably the more useful safety net than another test. test_resource_cleanup.py pinning the full call order is a nice touch.

Copyright years check out: 2026 on both new files, 2025-2026 on the modified ones.

Minor: the PR description still has the Short description of the change (max 50 chars) placeholder under Summary.

Verdict

Minor fixes needed

[cvolkcvolk]

The ideal fix looks like would belong to IsaacLab where ManagerBasedEnv owns the VideoRecorder. So VideoRecorder.close() should release its capture, and ManagerBasedEnv.close() should call it... Shall we add a TODO() to eventually clean this up properly in Lab?

[cvolkcvolk]

We should have access to self.recorder here directly:
close_viewport_video_recorder(self.video_recorder)

If Isaac Lab removes or renames the attribute, the code fails visibly instead of silently skipping the cleanup

[cvolkcvolk]

Second on the use of getattr()... we could use the known fields directly:

 def close_viewport_video_recorder(
      video_recorder: VideoRecorder | None,
  ) -> None:
      """Release the Kit viewport capture used by the pinned Isaac Lab version."""
      if video_recorder is None or video_recorder._backend != "kit":
          return

      capture = video_recorder._capture
      if capture is None:
          return

      annotator = capture._rgb_annotator
      render_product = capture._render_product

[cvolkcvolk]

Is this required explicetly to call close_viewport_video_recorder() here seperately?

Suggestion to keep cleanup only in IsaacLabArenaManagerBasedRLEnv.close() , then the below call of env.close() in

    finally:
        try:
            env.close()
        finally:
            collect_garbage_and_clear_cuda_cache()

would triggered to close it

[cvolkcvolk]

Thanks for addressing this!
I wasn't able to reproduce the shutdown hang locally, nor find Replicator to be the root cause. Left a few comments. Ok from my side to unblock for now but I think the root cause should be fixed in IsaacLab