From 03561d8c077fc82ec983ec6f28c4057289a9ee0b Mon Sep 17 00:00:00 2001 From: Jiwen Cai Date: Tue, 14 Jul 2026 00:36:02 +0000 Subject: [PATCH 1/2] Release OpenXR session deterministically at context-manager exit 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 --- src/core/oxr/cpp/inc/oxr/oxr_session.hpp | 19 +++++++++-- src/core/oxr/cpp/oxr_session.cpp | 12 ++++++- src/core/oxr/python/oxr_bindings.cpp | 13 ++++---- .../python/teleop_session.py | 13 ++++++-- .../python/test_teleop_session.py | 33 +++++++++++++++++++ 5 files changed, 77 insertions(+), 13 deletions(-) diff --git a/src/core/oxr/cpp/inc/oxr/oxr_session.hpp b/src/core/oxr/cpp/inc/oxr/oxr_session.hpp index 83edc15fa..43ce925dc 100644 --- a/src/core/oxr/cpp/inc/oxr/oxr_session.hpp +++ b/src/core/oxr/cpp/inc/oxr/oxr_session.hpp @@ -1,4 +1,4 @@ -// SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #pragma once @@ -20,9 +20,24 @@ class OpenXRSession public: OpenXRSession(const std::string& app_name, const std::vector& extensions, bool wait_for_system = false); - // Get session handles for use with trackers + // Get session handles for use with trackers. + // Non-owning views of session_/space_, valid only until close()/destruction; returns null handles thereafter. OpenXRSessionHandles get_handles() const; + // Release the native OpenXR handles now, in reverse dependency order + // (space -> session -> instance), so they are destroyed while the CloudXR + // runtime/IPC socket is still alive rather than at garbage collection. + // Idempotent: unique_ptr::reset() is a no-op on an already-null handle, so + // close() and the destructor (or a double close()) are all safe. + void close() noexcept; + + // Declaring the destructor suppresses the implicit move operations; inert + // today because every construction site is make_shared. + ~OpenXRSession() noexcept + { + close(); + } + private: // PFN_* deleter types work when OpenXR was already included with XR_NO_PROTOTYPES (no xrDestroy* declarations). using InstanceHandle = std::unique_ptr, PFN_xrDestroyInstance>; diff --git a/src/core/oxr/cpp/oxr_session.cpp b/src/core/oxr/cpp/oxr_session.cpp index 6c62e43b2..9f0bc81f9 100644 --- a/src/core/oxr/cpp/oxr_session.cpp +++ b/src/core/oxr/cpp/oxr_session.cpp @@ -1,4 +1,4 @@ -// SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #include "inc/oxr/oxr_session.hpp" @@ -92,6 +92,16 @@ OpenXRSessionHandles OpenXRSession::get_handles() const return OpenXRSessionHandles(instance_.get(), session_.get(), space_.get(), ::xrGetInstanceProcAddr); } +void OpenXRSession::close() noexcept +{ + // Deliberate: no xrRequestExitSession/xrEndSession handshake. xrDestroy* on a + // RUNNING session is spec-legal; the clean-exit handshake needs an event pump this + // class lacks and does not fix the EPIPE (dead runtime IPC socket), only adds + // teardown-hang risk. + space_.reset(); + session_.reset(); + instance_.reset(); +} void OpenXRSession::create_instance(const std::string& app_name, const std::vector& extensions) { diff --git a/src/core/oxr/python/oxr_bindings.cpp b/src/core/oxr/python/oxr_bindings.cpp index 796beea55..66c742c7d 100644 --- a/src/core/oxr/python/oxr_bindings.cpp +++ b/src/core/oxr/python/oxr_bindings.cpp @@ -1,4 +1,4 @@ -// SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #include @@ -44,11 +44,10 @@ PYBIND11_MODULE(_oxr, m) py::class_>(m, "OpenXRSession") .def(py::init&>(), py::arg("app_name"), py::arg("extensions") = std::vector()) - .def("get_handles", &core::OpenXRSession::get_handles, "Get session handles for sharing") + .def("get_handles", &core::OpenXRSession::get_handles, + "Get session handles for sharing. Returns null handles after close()/context-manager exit.") + .def("close", &core::OpenXRSession::close, + "Release the native session immediately (usually automatic via context manager)") .def("__enter__", [](core::OpenXRSession& self) -> core::OpenXRSession& { return self; }) - .def("__exit__", - [](core::OpenXRSession& self, py::object, py::object, py::object) - { - // RAII cleanup handled automatically when object is destroyed - }); + .def("__exit__", [](core::OpenXRSession& self, py::object, py::object, py::object) { self.close(); }); } diff --git a/src/core/teleop_session_manager/python/teleop_session.py b/src/core/teleop_session_manager/python/teleop_session.py index f7fecf94d..29bf2cc1f 100644 --- a/src/core/teleop_session_manager/python/teleop_session.py +++ b/src/core/teleop_session_manager/python/teleop_session.py @@ -214,7 +214,7 @@ def __init__(self, config: TeleopSessionConfig): @property def oxr_session(self) -> Optional[oxr.OpenXRSession]: - """The internal OpenXR session, or ``None`` when using external handles (read-only).""" + """The internal OpenXR session, or ``None`` when using external handles or after the context manager exits (read-only).""" return self._oxr_session @property @@ -1054,8 +1054,15 @@ def __exit__(self, exc_type, exc_val, exc_tb): # ExitStack automatically cleans up all managed contexts in reverse order. # Preserve TeleopSession's historical behavior of not suppressing # exceptions from the user body, even if a child context manager would. - self._exit_stack.__exit__(exc_type, exc_val, exc_tb) - self._exit_stack = ExitStack() + try: + self._exit_stack.__exit__(exc_type, exc_val, exc_tb) + self._exit_stack = ExitStack() + finally: + # The ExitStack above closes the OpenXR session; drop our reference so the + # public `oxr_session` property honors its documented None contract post-exit + # rather than surfacing a torn-down session -- even if a managed context's + # cleanup raised. (deviceio_session has no such property/contract.) + self._oxr_session = None if runner_error is not None: raise runner_error diff --git a/src/core/teleop_session_manager_tests/python/test_teleop_session.py b/src/core/teleop_session_manager_tests/python/test_teleop_session.py index 8a1f65756..43096ae6a 100644 --- a/src/core/teleop_session_manager_tests/python/test_teleop_session.py +++ b/src/core/teleop_session_manager_tests/python/test_teleop_session.py @@ -1082,6 +1082,39 @@ def test_exit_cleans_up(self): # but the exit stack has been unwound assert session._setup_complete is True + def test_exit_clears_oxr_session(self): + """After the with-block exits, the public oxr_session property honors its None contract.""" + pipeline = MockPipeline(leaf_nodes=[]) + + config = make_config(pipeline) + with mock_session_dependencies(): + session = TeleopSession(config) + with session as s: + assert s.oxr_session is not None + + assert session.oxr_session is None + + def test_exit_clears_oxr_session_even_if_cleanup_raises(self): + """oxr_session returns None post-exit even when a managed context's cleanup raises.""" + + class RaisingDeviceIOSession(MockDeviceIOSession): + def __exit__(self, *args): + raise RuntimeError("cleanup boom") + + pipeline = MockPipeline(leaf_nodes=[]) + config = make_config(pipeline) + with mock_session_dependencies(mock_dio=RaisingDeviceIOSession()): + session = TeleopSession(config) + session.__enter__() + assert session.oxr_session is not None + + # The ExitStack unwind raises, but exception propagation is preserved + # and the reference is still cleared via the finally block. + with pytest.raises(RuntimeError, match="cleanup boom"): + session.__exit__(None, None, None) + + assert session.oxr_session is None + def test_context_manager_protocol(self): """TeleopSession works correctly as a context manager.""" head = MockHeadSource() From c4eca05d22c49febb20329dfdd0806fdd51ebdff Mon Sep 17 00:00:00 2001 From: Jiwen Cai Date: Tue, 14 Jul 2026 18:21:21 +0000 Subject: [PATCH 2/2] Move OpenXR context-manager teardown into a PyOpenXRSession wrapper 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). --- src/core/oxr/cpp/inc/oxr/oxr_session.hpp | 17 +----- src/core/oxr/cpp/oxr_session.cpp | 10 ---- src/core/oxr/python/oxr_bindings.cpp | 72 +++++++++++++++++++++--- 3 files changed, 66 insertions(+), 33 deletions(-) diff --git a/src/core/oxr/cpp/inc/oxr/oxr_session.hpp b/src/core/oxr/cpp/inc/oxr/oxr_session.hpp index 43ce925dc..617bb328e 100644 --- a/src/core/oxr/cpp/inc/oxr/oxr_session.hpp +++ b/src/core/oxr/cpp/inc/oxr/oxr_session.hpp @@ -20,24 +20,9 @@ class OpenXRSession public: OpenXRSession(const std::string& app_name, const std::vector& extensions, bool wait_for_system = false); - // Get session handles for use with trackers. - // Non-owning views of session_/space_, valid only until close()/destruction; returns null handles thereafter. + // Get session handles for use with trackers OpenXRSessionHandles get_handles() const; - // Release the native OpenXR handles now, in reverse dependency order - // (space -> session -> instance), so they are destroyed while the CloudXR - // runtime/IPC socket is still alive rather than at garbage collection. - // Idempotent: unique_ptr::reset() is a no-op on an already-null handle, so - // close() and the destructor (or a double close()) are all safe. - void close() noexcept; - - // Declaring the destructor suppresses the implicit move operations; inert - // today because every construction site is make_shared. - ~OpenXRSession() noexcept - { - close(); - } - private: // PFN_* deleter types work when OpenXR was already included with XR_NO_PROTOTYPES (no xrDestroy* declarations). using InstanceHandle = std::unique_ptr, PFN_xrDestroyInstance>; diff --git a/src/core/oxr/cpp/oxr_session.cpp b/src/core/oxr/cpp/oxr_session.cpp index 9f0bc81f9..2d57c815b 100644 --- a/src/core/oxr/cpp/oxr_session.cpp +++ b/src/core/oxr/cpp/oxr_session.cpp @@ -92,16 +92,6 @@ OpenXRSessionHandles OpenXRSession::get_handles() const return OpenXRSessionHandles(instance_.get(), session_.get(), space_.get(), ::xrGetInstanceProcAddr); } -void OpenXRSession::close() noexcept -{ - // Deliberate: no xrRequestExitSession/xrEndSession handshake. xrDestroy* on a - // RUNNING session is spec-legal; the clean-exit handshake needs an event pump this - // class lacks and does not fix the EPIPE (dead runtime IPC socket), only adds - // teardown-hang risk. - space_.reset(); - session_.reset(); - instance_.reset(); -} void OpenXRSession::create_instance(const std::string& app_name, const std::vector& extensions) { diff --git a/src/core/oxr/python/oxr_bindings.cpp b/src/core/oxr/python/oxr_bindings.cpp index 66c742c7d..17612e33b 100644 --- a/src/core/oxr/python/oxr_bindings.cpp +++ b/src/core/oxr/python/oxr_bindings.cpp @@ -6,8 +6,67 @@ #include #include +#include +#include +#include +#include + namespace py = pybind11; +namespace core +{ + +/** + * @brief Python-facing session wrapper: destroys the underlying OpenXRSession in __exit__. + * + * Binding OpenXRSession directly with a no-op __exit__ leaves destruction to the pybind + * holder, which can run at garbage collection -- after the CloudXR runtime/IPC socket has + * been torn down -- producing "Broken pipe"/invalid-handle errors. Holding the session in a + * unique_ptr and resetting it in close()/exit() tears it down deterministically at + * context-manager exit, while the runtime is still alive. Mirrors PyDeviceIOSession. + */ +class PyOpenXRSession +{ +public: + PyOpenXRSession(const std::string& app_name, const std::vector& extensions) + : impl_(std::make_unique(app_name, extensions)) + { + } + + OpenXRSessionHandles get_handles() const + { + if (!impl_) + { + throw std::runtime_error("OpenXRSession has been closed/destroyed"); + } + return impl_->get_handles(); + } + + void close() + { + impl_.reset(); + } + + PyOpenXRSession& enter() + { + if (!impl_) + { + throw std::runtime_error("OpenXRSession has been closed/destroyed"); + } + return *this; + } + + void exit(py::object, py::object, py::object) + { + close(); + } + +private: + std::unique_ptr impl_; +}; + +} // namespace core + PYBIND11_MODULE(_oxr, m) { m.doc() = "Isaac Teleop OXR - OpenXR Session Module"; @@ -40,14 +99,13 @@ PYBIND11_MODULE(_oxr, m) [](const core::OpenXRSessionHandles& self) { return reinterpret_cast(self.xrGetInstanceProcAddr); }, "Get xrGetInstanceProcAddr function pointer as integer"); - // OpenXRSession class (for creating sessions) - py::class_>(m, "OpenXRSession") + // OpenXRSession (Python-facing wrapper; see core::PyOpenXRSession for the lifetime contract) + py::class_>(m, "OpenXRSession") .def(py::init&>(), py::arg("app_name"), py::arg("extensions") = std::vector()) - .def("get_handles", &core::OpenXRSession::get_handles, - "Get session handles for sharing. Returns null handles after close()/context-manager exit.") - .def("close", &core::OpenXRSession::close, + .def("get_handles", &core::PyOpenXRSession::get_handles, "Get session handles for sharing") + .def("close", &core::PyOpenXRSession::close, "Release the native session immediately (usually automatic via context manager)") - .def("__enter__", [](core::OpenXRSession& self) -> core::OpenXRSession& { return self; }) - .def("__exit__", [](core::OpenXRSession& self, py::object, py::object, py::object) { self.close(); }); + .def("__enter__", &core::PyOpenXRSession::enter) + .def("__exit__", &core::PyOpenXRSession::exit); }