Skip to content

Fixes failing CI test for mock interfaces#5036

Open
AntoineRichard wants to merge 4 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/fix/mock_interfaces
Open

Fixes failing CI test for mock interfaces#5036
AntoineRichard wants to merge 4 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/fix/mock_interfaces

Conversation

@AntoineRichard
Copy link
Collaborator

Description

Fixes failing CI test for mock interfaces. This was caused by some pointers being deleted by the garbage collector.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Mar 16, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 16, 2026

Greptile Summary

This PR fixes flaky CI tests (test_mock_data_properties) caused by use-after-free bugs in mock asset data properties. The root cause was that several warp @property methods returned .view() aliases of locally-scoped temporary arrays. When CPython's reference counter freed these temporaries before the view was consumed (particularly by downstream zero-copy pointer arithmetic properties like root_com_lin_vel_w), reads would produce garbage data.

The fix caches the default arrays in instance attributes (self._root_link_vel_w, self._root_com_vel_w, etc.) so the backing memory stays alive for the object's lifetime. This is applied consistently across MockArticulationData, MockRigidObjectData, and MockRigidObjectCollectionData.

  • 18 properties fixed across 3 mock data classes by changing return wp.zeros(...).view(...) / return wp.clone(...) to self._field = ...; return self._field
  • Version bumped to 4.5.23 with a detailed changelog entry
  • A few other properties with the same .view() pattern (default_root_vel, default_body_pose, default_body_vel, joint_pos_limits, body_incoming_joint_wrench_b) were not updated — these may not currently trigger issues but are inconsistent with the fix

Confidence Score: 4/5

  • This PR is safe to merge — it fixes a real use-after-free bug in test mock classes with a correct caching approach.
  • The fix correctly addresses the use-after-free pattern for the properties that were causing CI flakiness. The change is mechanical and low-risk (caching default values in instance attributes). Score is 4 instead of 5 because a few other properties with the same .view() pattern were left unfixed, which represents a minor inconsistency rather than a blocking issue.
  • The three mock data files (mock_articulation.py, mock_rigid_object.py, mock_rigid_object_collection.py) still have a handful of properties with the same uncached .view() pattern that could cause similar issues in the future.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/test/mock_interfaces/assets/mock_articulation.py Correctly fixes 8 use-after-free properties by caching warp arrays, but several other properties with the same .view() pattern remain unfixed (e.g., default_root_vel, joint_pos_limits, body_incoming_joint_wrench_b).
source/isaaclab/isaaclab/test/mock_interfaces/assets/mock_rigid_object.py Correctly fixes 6 use-after-free properties by caching warp arrays, but default_root_vel still has the same uncached .view() pattern.
source/isaaclab/isaaclab/test/mock_interfaces/assets/mock_rigid_object_collection.py Correctly fixes 4 use-after-free properties by caching warp arrays, but default_body_pose and default_body_vel still have the same uncached .view() pattern.
source/isaaclab/config/extension.toml Patch version bump from 4.5.22 to 4.5.23, appropriate for a bug fix.
source/isaaclab/docs/CHANGELOG.rst Well-written changelog entry explaining the root cause (use-after-free from warp view pointer aliases) and the fix (caching default arrays).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Property accessed<br/>(e.g., root_com_lin_vel_w)"] --> B["Calls parent property<br/>(e.g., root_com_vel_w)"]
    B --> C{"self._field<br/>is None?"}
    C -->|"Yes (before fix)"| D["return wp.zeros(...).view()<br/>Temporary created"]
    D --> E["Temporary may be GC'd"]
    E --> F["wp.array(ptr=v.ptr, ...)<br/>Dangling pointer!"]
    F --> G["❌ Use-after-free:<br/>garbage data"]
    C -->|"Yes (after fix)"| H["self._field = wp.zeros(...).view()<br/>Cached on instance"]
    H --> I["wp.array(ptr=v.ptr, ...)<br/>Pointer stays valid"]
    I --> J["✅ Correct data returned"]
    C -->|No| K["Return cached self._field"]
    K --> I
Loading

Comments Outside Diff (1)

  1. source/isaaclab/isaaclab/test/mock_interfaces/assets/mock_articulation.py, line 170-174 (link)

    Same use-after-free pattern remains unfixed

    default_root_vel still returns a .view() of a temporary without caching to self._default_root_vel, matching the exact bug pattern fixed elsewhere in this PR. While current consumers (default_root_state) call wp.to_torch() which may keep the array alive, this is fragile and inconsistent with the fix applied to the other properties.

    The same unfixed pattern also appears in:

    • mock_articulation.py:282joint_pos_limits returns wp.array(...).view(wp.vec2f) without caching
    • mock_articulation.py:629body_incoming_joint_wrench_b returns wp.zeros(...).view(wp.spatial_vectorf) without caching
    • mock_rigid_object.py:107default_root_vel returns wp.zeros(...).view(wp.spatial_vectorf) without caching
    • mock_rigid_object_collection.py:99default_body_pose returns wp.array(...).view(wp.transformf) without caching
    • mock_rigid_object_collection.py:106default_body_vel returns wp.zeros(...).view(wp.spatial_vectorf) without caching

    Consider applying the same caching fix to all .view() and wp.clone() return paths for consistency and to prevent future regressions.

Last reviewed commit: ee15789

Copy link
Collaborator

@pascal-roth pascal-roth left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants