Release OpenXR session deterministically at context-manager exit#770
Open
jiwenc-nv wants to merge 2 commits into
Open
Release OpenXR session deterministically at context-manager exit#770jiwenc-nv wants to merge 2 commits into
jiwenc-nv wants to merge 2 commits into
Conversation
Contributor
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant TeleopSession
participant ExitStack
participant OpenXRSession
participant NativeHandles
TeleopSession->>ExitStack: exit context
ExitStack->>OpenXRSession: invoke close()
OpenXRSession->>NativeHandles: reset space, session, and instance
TeleopSession->>TeleopSession: clear _oxr_session
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jiwenc-nv
force-pushed
the
fix_cxr_launcher
branch
from
ba69127 to
f54bfbd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/teleop_session_manager/python/teleop_session.py`:
- Around line 1060-1063: Update the exit/cleanup method around
self._exit_stack.__exit__() to assign self._oxr_session = None in a finally
block, ensuring the reference is cleared even when managed cleanup raises.
Preserve the existing cleanup behavior and exception propagation while
guaranteeing the public oxr_session property returns None after exit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ddb600d-f308-4558-bfa0-834c2bbb0a76
📒 Files selected for processing (5)
src/core/oxr/cpp/inc/oxr/oxr_session.hppsrc/core/oxr/cpp/oxr_session.cppsrc/core/oxr/python/oxr_bindings.cppsrc/core/teleop_session_manager/python/teleop_session.pysrc/core/teleop_session_manager_tests/python/test_teleop_session.py
CloudXR-backed runs (e.g. examples/mcap_record_replay/record_hand.py)
flooded the console with "Broken pipe" IPC errors on shutdown:
ERROR [ipc_send] sendmsg(9) failed: '32' 'Broken pipe'!
ERROR [ref_space_dec] ipc_call_space_unmark_ref_space_in_use failed
ERROR [ipc_client_session_destroy] ipc_call_session_destroy failed
Root cause is timing, not ordering: OpenXRSession.__exit__ was a no-op,
so the OpenXR handles (space -> session -> instance) were only released
by the C++ destructor at Python GC -- after CloudXRLauncher.__exit__ had
already SIGTERM'd the runtime and closed its IPC socket. The client-side
xrDestroySpace/xrDestroySession then hit a dead socket -> EPIPE.
Give OpenXRSession an explicit, idempotent close() that resets the
handles in reverse dependency order, and wire it into __exit__ (mirroring
the existing DeviceIOSession close()/exit() precedent). The ExitStack in
TeleopSession already tears DeviceIOSession down before the OpenXR
session, so handles are released while the runtime is still alive.
Destroying a RUNNING session is spec-legal; the xrRequestExitSession /
xrEndSession handshake is deliberately omitted (needs an event pump this
class lacks and does not affect the dead-socket EPIPE).
TeleopSession.__exit__ also drops its _oxr_session reference so the
public oxr_session property honors its documented None contract post-exit.
Verified: oxr_py rebuilds clean; teleop_session_manager suite passes
(121 passed); the new test_exit_clears_oxr_session fails on the pre-fix
code (real guard). The native EPIPE fix itself requires a live CloudXR
runtime to observe end-to-end (manual smoke test).
Designed and reviewed via a multi-agent panel (coder, tech-lead,
bar-raiser, robotics-expert, external-developer, YAGNI): 3 planning
rounds to consensus, unanimous ratification, 1 review round (all
approve/approve-with-nits).
Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
jiwenc-nv
force-pushed
the
fix_cxr_launcher
branch
from
f54bfbd to
7837c93
Compare
The previous fix put an explicit close()/destructor on core::OpenXRSession and wired the pybind __exit__ to it. That leaks a pybind-driven lifecycle concern (deterministic release at context-manager exit) into the core C++ interface. Follow the house convention used for DeviceIOSession instead: keep core::OpenXRSession a pure-RAII C++ type and introduce a Python-facing core::PyOpenXRSession wrapper that owns the session via unique_ptr, resets it in close()/exit(), and throws from accessors once closed. Bound as the Python "OpenXRSession" with an unchanged surface (init/get_handles/close/ __enter__/__exit__), so no caller changes. Net effect is identical -- the session is torn down deterministically at `with`-block exit, while the CloudXR runtime/IPC socket is still alive, avoiding the GC-ordered "Broken pipe" teardown errors -- but the core class is reverted to its original form and the quirk stays in the binding layer. get_handles() now raises after close() (loud, like PyDeviceIOSession) rather than returning null handles. Verified: oxr_py rebuilds clean; teleop_session_manager suite passes (all 4 binaries); every get_handles() consumer calls it inside the `with` block. Native teardown still needs a live CloudXR runtime to observe end-to-end (manual smoke test).
aristarkhovNV
approved these changes
Jul 14, 2026
| try: | ||
| self._exit_stack.__exit__(exc_type, exc_val, exc_tb) | ||
| self._exit_stack = ExitStack() | ||
| finally: |
Collaborator
There was a problem hiding this comment.
with the wrapper and its idempotent close() do we still need the exception handler here? This seems to contradict the comment above regarding not suppressing exceptions.
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.
CloudXR-backed runs (e.g. examples/mcap_record_replay/record_hand.py) flooded the console with "Broken pipe" IPC errors on shutdown:
Root cause is timing, not ordering: OpenXRSession.exit was a no-op, so the OpenXR handles (space -> session -> instance) were only released by the C++ destructor at Python GC -- after CloudXRLauncher.exit had already SIGTERM'd the runtime and closed its IPC socket. The client-side xrDestroySpace/xrDestroySession then hit a dead socket -> EPIPE.
Give OpenXRSession an explicit, idempotent close() that resets the handles in reverse dependency order, and wire it into exit (mirroring the existing DeviceIOSession close()/exit() precedent). The ExitStack in TeleopSession already tears DeviceIOSession down before the OpenXR session, so handles are released while the runtime is still alive. Destroying a RUNNING session is spec-legal; the xrRequestExitSession / xrEndSession handshake is deliberately omitted (needs an event pump this class lacks and does not affect the dead-socket EPIPE).
TeleopSession.exit also drops its _oxr_session reference so the public oxr_session property honors its documented None contract post-exit.
Verified: oxr_py rebuilds clean; teleop_session_manager suite passes (121 passed); the new test_exit_clears_oxr_session fails on the pre-fix code (real guard). The native EPIPE fix itself requires a live CloudXR runtime to observe end-to-end (manual smoke test).
Designed and reviewed via a multi-agent panel (coder, tech-lead, bar-raiser, robotics-expert, external-developer, YAGNI): 3 planning rounds to consensus, unanimous ratification, 1 review round (all approve/approve-with-nits).
Description
Fixes #(issue)
Type of change
Testing
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCOSummary by CodeRabbit
New Features
close()method.Bug Fixes
Tests