Skip to content

fix(isaac): pump app after timeline.stop() so the #159 velocity crash actually clears#164

Merged
cagataycali merged 1 commit into
strands-labs:mainfrom
yinsong1986:fix/159-pump-app-after-timeline-stop
Jun 30, 2026
Merged

fix(isaac): pump app after timeline.stop() so the #159 velocity crash actually clears#164
cagataycali merged 1 commit into
strands-labs:mainfrom
yinsong1986:fix/159-pump-app-after-timeline-stop

Conversation

@yinsong1986

Copy link
Copy Markdown
Contributor

Summary

examples/libero/run_isaac.py still crashed end-to-end after both prior
fixes for #159 (PR #161 probe-gated, PR #163 unconditional
timeline.stop()), with the same bare exception during
DynamicCuboid construction in load_scene:

Exception: Failed to get rigid body velocities from backend

This PR makes the deferral actually take effect.

Root cause

timeline.stop() is asynchronous. It posts a GLOBAL_EVENT_STOP
that tears down the physics-tensor view only on a later app tick.
Isaac's RigidPrim.__init__ gates its eager _on_physics_ready
velocity query on:

if SimulationManager.get_physics_sim_view() is not None:   # rigid_prim.py:200
    ... RigidPrim._on_physics_ready(self, None)             # -> get_velocities() -> raises

and that handle stays non-None until the stop event is dispatched.
So #163's "stop the timeline, then immediately construct the Dynamic*
prim" still saw a live view and hit the eager query.

A diagnostic on a real Isaac Sim 6.0 run confirms it:

[probe:after reset+step]              get_physics_sim_view() is None == False
[probe:immediately after stop()]      get_physics_sim_view() is None == False   <-- #163 builds the prim here
[probe:after app.update() x3]         get_physics_sim_view() is None == True

Fix

After timeline.stop(), pump SimulationApp.update() until
SimulationManager.get_physics_sim_view() returns None, bounded to 8
ticks (so a host that never clears fails loud at prim construction rather
than spinning). SimulationApp.update() is render-only — it does not
advance physics — so it doesn't drift the already-placed prims. Converted
_stop_timeline_for_deferred_physics from a @staticmethod to an
instance method so it can reach self._app.

Verification

  • End-to-end (Isaac Sim 6.0, L4 GPU, headless):
    OMNI_KIT_ACCEPT_EULA=YES python examples/libero/run_isaac.py --policy mock --n-episodes 3 --seed 42
    • Before: exit 1, on_episode_start failed ... Failed to get rigid body velocities, 720-frame warm-up-only MP4.
    • After: completes all 3 episodes, 0 frame errors, records a
      real 2160-frame (72 s) rollout MP4. No traceback, clean shutdown.
  • Unit tests: strands_robots_sim/isaac/tests/test_unit.py — 189
    passed (1 deselected: a pre-existing env flake unrelated to this
    change — test_is_available_probes_omni_isaac_kit_specifically fails
    only because the dev box has real omni installed; it passes on the
    CPU CI runner). Added:
    • test_stop_timeline_pumps_app_until_physics_view_cleared
    • test_stop_timeline_pump_is_bounded
    • updated test_stop_timeline_is_best_effort_without_omni for the
      staticmethod→instance change.
  • Lint: black --check, isort --check-only, flake8 all clean
    (line-length 120, per pyproject.toml).

Notes

…ty crash actually clears

PR strands-labs#161 (probe-gated) and PR strands-labs#163 (unconditional timeline.stop()) both
left run_isaac.py crashing end-to-end with the bare
`Exception("Failed to get rigid body velocities from backend")` raised
from omni.physics.tensors during DynamicCuboid construction in load_scene.

Root cause (verified on Isaac Sim 6.0): `timeline.stop()` is
**asynchronous** -- it posts a GLOBAL_EVENT_STOP that tears down the
physics-tensor view only on a later app tick. `RigidPrim.__init__` gates
its eager `_on_physics_ready` velocity query on
`SimulationManager.get_physics_sim_view() is not None`
(rigid_prim.py:200), and that handle stays non-None until the stop event
is dispatched. So strands-labs#163's "stop the timeline, then immediately build the
Dynamic* prim" still hit the eager query and crashed. A diagnostic on a
real run shows the view reads non-None immediately after stop() and only
flips to None after a few `SimulationApp.update()` ticks.

Fix: after `timeline.stop()`, pump `SimulationApp.update()` (render-only,
no physics advance -- so already-placed prims don't drift) until
`get_physics_sim_view()` returns None, bounded to 8 ticks so a host that
never clears fails loud at construction rather than spinning. Convert
`_stop_timeline_for_deferred_physics` from a staticmethod to an instance
method so it can reach `self._app`.

Verified end-to-end: `run_isaac.py --policy mock --n-episodes 3` now
completes all 3 episodes (no crash, 0 frame errors) and records a real
2160-frame rollout MP4, versus the previous 720-frame warm-up-only file
from the aborted eval.

Adds two unit tests (pump-until-cleared, bounded pump) and updates the
best-effort test for the staticmethod->instance change. black / isort /
flake8 clean.

Refs strands-labs#159 (reopened after strands-labs#161/strands-labs#163 proved insufficient).

@cagataycali cagataycali left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve. The root-cause analysis is correct and the fix is surgical.

The key insight - timeline.stop() posts an async GLOBAL_EVENT_STOP that only tears down the physics-tensor view on a later tick, so the prior unconditional stop was necessary-but-insufficient - matches the documented Isaac 6.0 diagnostic. Pumping SimulationApp.update() (render-only, no physics advance) until get_physics_sim_view() clears is the right deferral, and bounding it to 8 ticks (fail-loud at prim construction rather than spin) is a sound safety valve.

Defensive details I checked and like:

  • view is checked at the top of each iteration, so an already-cleared view returns immediately with zero pumps;
  • graceful degradation when SimulationManager or self._app.update is unavailable (returns without pumping);
  • the staticmethod to instance-method change is the minimal way to reach self._app.

Regression coverage pins both behaviours precisely: the happy-path test asserts exactly 2 update calls for a [obj, obj, None] view sequence, and the bound test asserts exactly 8 when the view never clears. CI green, scope tight, docstring carries the why. Good to merge.

@cagataycali cagataycali merged commit 62bb883 into strands-labs:main Jun 30, 2026
2 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Strands Labs - Robots Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants