fix(cloudxr): refuse to start over a live runtime; explain -35 - #911
Conversation
|
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:
📝 WalkthroughWalkthroughCloudXR runtime startup now probes Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CloudXRLauncher
participant RuntimeIPC
participant ExistingRuntime
CLI->>CloudXRLauncher: Start with optional --force
CloudXRLauncher->>RuntimeIPC: Probe ipc_cloudxr
RuntimeIPC-->>CloudXRLauncher: Return liveness
alt Existing runtime is live
CloudXRLauncher->>ExistingRuntime: Send SIGTERM when forced
CloudXRLauncher->>RuntimeIPC: Wait for termination
else Socket is stale
CloudXRLauncher->>RuntimeIPC: Remove stale socket
end
CloudXRLauncher-->>CLI: Start runtime or report error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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/python/isaacteleop/cloudxr/launcher.py`:
- Around line 594-636: The launch flow currently relies on the point-in-time
is_runtime_live check and force-mode socket disappearance, so runtime ownership
is not atomic and shutdown is not confirmed. Add an exclusive run-directory
ownership lock that remains held from startup through the runtime lifetime,
acquire it before checking or removing stale state, and ensure it is released
only when the launcher exits. Update _terminate_live_runtime to identify and
wait for the prior owner process to exit, not merely until is_runtime_live
returns false, before cleanup and replacement startup proceed.
🪄 Autofix
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: 90cb1f88-3f35-4695-a691-bdb2dabd7a7d
📒 Files selected for processing (11)
docs/source/getting_started/quick_start.rstdocs/source/getting_started/televiz.rstdocs/source/references/cloudxr.rstsrc/core/cloudxr_tests/python/test_launcher.pysrc/core/cloudxr_tests/python/test_runtime.pysrc/core/oxr/cpp/oxr_session.cppsrc/python/isaacteleop/cloudxr/__main__.pysrc/python/isaacteleop/cloudxr/env_config.pysrc/python/isaacteleop/cloudxr/launcher.pysrc/python/isaacteleop/cloudxr/runtime.pysrc/viz/xr/cpp/openxr_session.cpp
14c8c2e to
5810c72
Compare
5810c72 to
f3f830e
Compare
f3f830e to
23a0eb8
Compare
yanziz-nvidia
left a comment
There was a problem hiding this comment.
Reviewed by yanziz-reviewer-bot
Summary
Replaces the fuser -k -TERM liveness check with a connect() probe in _cleanup_stale_runtime, and adds a device profile banner and a DEFAULT_DEVICE_PROFILE constant. CI state unknown from here.
Legend: 🚫 Blocker · 💡 Suggestion · 🔍 Nit
| Finding | |
|---|---|
| 🚫 | None |
| 💡 | src/python/isaacteleop/cloudxr/runtime.py:274 — sock.connect(full_abs_path) will raise OSError: ENAMETOOLONG on long TMPDIR paths (CI, nested job isolation), which falls into except OSError: return True and misreports a dead socket as live. The PR already solved this for the test helper (_live_ipc_socket uses os.chdir + relative path). Apply the same fix here: os.chdir(run_dir) before sock.connect("ipc_cloudxr"), restore CWD in finally. |
| 🔍 | src/python/isaacteleop/cloudxr/__main__.py:238 — env_cfg.resolved("NV_DEVICE_PROFILE") returns str | None; f-string silently prints None if resolution hasn't run. Change to env_cfg.resolved("NV_DEVICE_PROFILE") or DEFAULT_DEVICE_PROFILE. |
Actionables (for bots — copy-paste-ready for AI)
Fix if it makes sense in context — these are agent-generated suggestions, not human-vetted obligations. Skip anything that's wrong, already addressed, or not worth the churn.
src/python/isaacteleop/cloudxr/runtime.py:274— Inis_runtime_live(), replacesock.connect(path)withos.chdir(run_dir); sock.connect("ipc_cloudxr")(relative), restoring CWD infinally— mirrors the_live_ipc_sockettest helper pattern; avoidsENAMETOOLONGon long paths.src/python/isaacteleop/cloudxr/__main__.py:238— Change toprofile = env_cfg.resolved("NV_DEVICE_PROFILE") or DEFAULT_DEVICE_PROFILEto avoid printingNone.
A second launch treated the existence of `ipc_cloudxr` as proof of a stale runtime and `fuser -k -TERM`ed whoever held it — including a healthy runtime mid-session, which the first session saw as a broken pipe. When `fuser` was missing the error was swallowed and the socket unlinked anyway, leaving the old process running and the operator to clean `~/.cloudxr/run/` by hand. Liveness is now decided by connecting to the socket rather than by stat-ing it. A live runtime makes the launcher refuse, pointing at the env file and `--no-launch-cloudxr-runtime`; replacing it means stopping it yourself. A dead socket is cleaned with a warning. Ambiguous probe errors count as live, since refusing is recoverable and clobbering a session is not. `-35` is thrown after `xrCreateInstance` succeeds, so the runtime was found and only the headset is missing, but library defaults are fail-fast and a direct consumer got a bare `Failed to get OpenXR system: -35`. Both throw sites now name `XR_ERROR_FORM_FACTOR_UNAVAILABLE` and point at the docs. The device profile is the usual culprit, so the startup banner prints the resolved value and the launcher/env-config defaults agree on one constant — the docs claimed `auto-webrtc` where `Quest3` always won. Overriding it stays an env-file or `CloudXRLauncher(device_profile=...)` job; Apple Vision Pro needs `auto-native`. Closes #908 Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
23a0eb8 to
e50192f
Compare
Description
Items 1 and 2 of #736.
_cleanup_stale_runtimetreated an existingipc_cloudxras proof of staleness andfuser -k -TERMed its holder — including a healthy runtime mid-session, which the first session saw as a broken pipe. Withfuserabsent the error was swallowed and the socket unlinked anyway, so the old process survived; hence the manualrm.Liveness is now a
connect(). Live → refuse, naming the resolved env file and--no-launch-cloudxr-runtime; replacing it means stopping it yourself. Dead → clean, at WARNING. Ambiguous probe errors count as live. Nothing signals another process now, so the undeclaredfuser/psmiscdependency goes too.Both
-35throw sites now nameXR_ERROR_FORM_FACTOR_UNAVAILABLEand point at the one new docs section, carrying the-51contrast and the checklist. The banner prints the resolved device profile, and the two device-profile defaults collapse onto one constant — docs saidauto-webrtc,Quest3always won.#908 also proposed
--forceand--cloudxr-device-profile; both dropped as unnecessary.Fixes #908
Type of change
Testing
src/core/cloudxr_tests/python/), covering the probe and the refusal.SKIP=check-copyright-year pre-commit run --all-filesclean.Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCO