You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
While reviewing #887 I formed the impression that the viz stack was re-inventing CloudXR lifecycle management. Checking the code, the premise needs one correction: src/viz/ (the C++ libraries) contains no CloudXR lifecycle code at all, and examples/camera_viz/camera_viz.py already imports and uses CloudXRLauncher (camera_viz.py:32, :394, :442). What it re-invents is not the launcher — it is the supervision policy around the launcher: manual __enter__/__exit__, its own signal handlers, its own health-poll plumbing, its own teardown-ordering rules. So the smell is real, it just lives one layer up from where I first placed it.
Every app that wants a managed CloudXR runtime today has to hand-roll the same four things: start it, hold it open, notice when it dies, tear it down in the right order. CloudXRLauncher gives you the first and the last; the middle two are copy-pasted per app, and they have already drifted.
Evidence
1. Four independent implementations of "poll the launcher's health from the app's main loop."
src/python/isaacteleop/cloudxr/__main__.py:260-272 — while not stop: launcher.health_check(); time.sleep(0.1) with its own SIGINT/SIGTERM flag.
examples/teleop_ros2/python/teleop_ros2_node.py:216 and :225 — one check between session attempts, one per step, each with a comment explaining why the other one is insufficient.
src/python/isaacteleop/teleop_session_manager/teleop_session.py:922-924 — _check_plugin_health(), the same pattern for plugin contexts.
The protocol name has already diverged: CloudXRLauncher.health_check() vs. plugin contexts' check_health(). Two names for one concept is a good sign the protocol was never designed, only repeatedly rediscovered.
2. Signal-handler collision in camera_viz.
CloudXRLauncher.__init__ installs chaining SIGTERM/SIGINT handlers (launcher.py:210, _install_signal_handlers). camera_viz.py:446 enters the launcher, then camera_viz.py:482-483 unconditionally overwrites both signals with its own _on_signal. On teardown, _restore_signal_handlers (launcher.py:559-568) sees signal.getsignal(sig) is not ours, drops the saved entry and returns without restoring — so the launcher's "stop the runtime on SIGTERM" guarantee is silently gone, and the process now depends on the finally block plus the atexit hook. It happens to work. It works by accident, and nothing in either file tells you the two are fighting over the same global.
3. rig/launcher.py re-derives what isaacteleop.cloudxr already owns.rig imports nothing from cloudxr today:
the same sentinel written and polled by runtime.py:208, :244
CLOUDXR_ENV_WAIT_TIMEOUT_SEC = 120 (:64)
RUNTIME_STARTUP_TIMEOUT_SEC = 30 (runtime.py:24)
stale-sentinel deletion before launch (:337-344)
_cleanup_stale_runtime (launcher.py:571-608)
find_runtime_footguns lint for missing --no-launch-cloudxr-runtime (rig/config.py:264)
the flag CloudXRLauncher.add_launch_cloudxr_runtime_argument registers
The last row is the telling one: rig has to lint for a launcher CLI flag by string-matching user commands, because it has no structural relationship with the launcher whose behavior it depends on.
4. Duplication inside cloudxr itself, already drifted.launcher.py:604 cleans ("runtime_started", "monado.pid", "cloudxr.pid"); runtime.py:310 cleans ("ipc_cloudxr", "runtime_started", "monado.pid", "cloudxr.pid"). The launcher handles ipc_cloudxr separately just above, so the divergence is currently intentional — but it is exactly the shape that goes wrong when a fifth sentinel file gets added to one list and not the other.
Proposal
One supervised-lifecycle surface that owns the whole thing — start, signal/atexit registration, health polling, ordered teardown — with two ways to drive it:
In-process: an app hands over its main loop (or its stop event) and gets runtime-death propagated as an exception on the main thread, without re-implementing the poll. camera_viz, teleop_ros2, cloudxr/__main__, and any future viz app all collapse onto this. Note that camera_viz drives a VizSession, not a TeleopSession, so this has to live below TeleopSession rather than inside it.
Out-of-process:rig keeps its tmux-pane model, but drives the same surface — importing EnvConfig/runtime for run-dir, env-file name, sentinel name, and timeouts instead of re-deriving all four, and ideally replacing the footgun string-lint with something the launcher itself exposes.
Concrete sub-tasks:
Unify the health-check protocol on one name and one contract (health_check() vs. check_health()), and give it a single polling implementation instead of four.
Define who owns SIGINT/SIGTERM when a launcher and an app both want them, and make the camera_viz case work by construction rather than by luck.
Make rig/launcher.py import run-dir, env-file name, sentinel name, and the startup timeout from isaacteleop.cloudxr instead of redefining them.
Merge rig and CloudXRLauncher into one lifecycle story — this has been the intent for a while and simply has not been done yet.
De-duplicate the stale-file cleanup lists between launcher.py and runtime.py.
Document the ordered-teardown rule that camera_viz.py:436-440 currently encodes as a comment (don't stop the runtime while a worker thread is inside session.render()), so every app doesn't have to rediscover it.
fix(camera_viz): monitor managed CloudXR runtime #887 is the motivating symptom, not the problem — it is a correct fix for a real bug, and the fact that fixing it required inventing a fifth health-poll site is the point.
Summary
While reviewing #887 I formed the impression that the viz stack was re-inventing CloudXR lifecycle management. Checking the code, the premise needs one correction:
src/viz/(the C++ libraries) contains no CloudXR lifecycle code at all, andexamples/camera_viz/camera_viz.pyalready imports and usesCloudXRLauncher(camera_viz.py:32,:394,:442). What it re-invents is not the launcher — it is the supervision policy around the launcher: manual__enter__/__exit__, its own signal handlers, its own health-poll plumbing, its own teardown-ordering rules. So the smell is real, it just lives one layer up from where I first placed it.Every app that wants a managed CloudXR runtime today has to hand-roll the same four things: start it, hold it open, notice when it dies, tear it down in the right order.
CloudXRLaunchergives you the first and the last; the middle two are copy-pasted per app, and they have already drifted.Evidence
1. Four independent implementations of "poll the launcher's health from the app's main loop."
src/python/isaacteleop/cloudxr/__main__.py:260-272—while not stop: launcher.health_check(); time.sleep(0.1)with its own SIGINT/SIGTERM flag.examples/teleop_ros2/python/teleop_ros2_node.py:216and:225— one check between session attempts, one per step, each with a comment explaining why the other one is insufficient.src/python/isaacteleop/teleop_session_manager/teleop_session.py:922-924—_check_plugin_health(), the same pattern for plugin contexts.VizRunner.wait(health_check=...), threading a callable through the runner so the render-thread join loop can poll it.The protocol name has already diverged:
CloudXRLauncher.health_check()vs. plugin contexts'check_health(). Two names for one concept is a good sign the protocol was never designed, only repeatedly rediscovered.2. Signal-handler collision in
camera_viz.CloudXRLauncher.__init__installs chaining SIGTERM/SIGINT handlers (launcher.py:210,_install_signal_handlers).camera_viz.py:446enters the launcher, thencamera_viz.py:482-483unconditionally overwrites both signals with its own_on_signal. On teardown,_restore_signal_handlers(launcher.py:559-568) seessignal.getsignal(sig) is not ours, drops the saved entry and returns without restoring — so the launcher's "stop the runtime on SIGTERM" guarantee is silently gone, and the process now depends on thefinallyblock plus theatexithook. It happens to work. It works by accident, and nothing in either file tells you the two are fighting over the same global.3.
rig/launcher.pyre-derives whatisaacteleop.cloudxralready owns.rigimports nothing fromcloudxrtoday:rig/launcher.pycloudxr_cloudxr_run_dir()re-parses--cloudxr-install-dir/$CXR_INSTALL_DIR/~/.cloudxr(:149-169)EnvConfig.openxr_run_dir()(env_config.py:88)"cloudxr.env"(:174)EnvConfig.env_filepath()(env_config.py:98)_RUNTIME_STARTED_SENTINEL = "runtime_started"(:69)runtime.py:208,:244CLOUDXR_ENV_WAIT_TIMEOUT_SEC = 120(:64)RUNTIME_STARTUP_TIMEOUT_SEC = 30(runtime.py:24):337-344)_cleanup_stale_runtime(launcher.py:571-608)find_runtime_footgunslint for missing--no-launch-cloudxr-runtime(rig/config.py:264)CloudXRLauncher.add_launch_cloudxr_runtime_argumentregistersThe last row is the telling one:
righas to lint for a launcher CLI flag by string-matching user commands, because it has no structural relationship with the launcher whose behavior it depends on.4. Duplication inside
cloudxritself, already drifted.launcher.py:604cleans("runtime_started", "monado.pid", "cloudxr.pid");runtime.py:310cleans("ipc_cloudxr", "runtime_started", "monado.pid", "cloudxr.pid"). The launcher handlesipc_cloudxrseparately just above, so the divergence is currently intentional — but it is exactly the shape that goes wrong when a fifth sentinel file gets added to one list and not the other.Proposal
One supervised-lifecycle surface that owns the whole thing — start, signal/
atexitregistration, health polling, ordered teardown — with two ways to drive it:camera_viz,teleop_ros2,cloudxr/__main__, and any future viz app all collapse onto this. Note thatcamera_vizdrives aVizSession, not aTeleopSession, so this has to live belowTeleopSessionrather than inside it.rigkeeps its tmux-pane model, but drives the same surface — importingEnvConfig/runtimefor run-dir, env-file name, sentinel name, and timeouts instead of re-deriving all four, and ideally replacing the footgun string-lint with something the launcher itself exposes.Concrete sub-tasks:
health_check()vs.check_health()), and give it a single polling implementation instead of four.camera_vizcase work by construction rather than by luck.rig/launcher.pyimport run-dir, env-file name, sentinel name, and the startup timeout fromisaacteleop.cloudxrinstead of redefining them.rigandCloudXRLauncherinto one lifecycle story — this has been the intent for a while and simply has not been done yet.launcher.pyandruntime.py.camera_viz.py:436-440currently encodes as a comment (don't stop the runtime while a worker thread is insidesession.render()), so every app doesn't have to rediscover it.Relationship to existing issues
CloudXRLauncherlifecycle insideTeleopSession) is a subset: it removes the nestedwithforTeleopSessioncallers. It does not covercamera_viz/viz apps (noTeleopSession), the health-check protocol, orrig. Whatever lands here should make Embed CloudXRLauncher lifecycle inside TeleopSession #701 a thin wrapper rather than a parallel implementation.CloudXRLauncher) is about adoption; this issue is about what they adopt.