feat(oob): Adds initial oob implementation with metrics#388
Open
yanziz-nvidia wants to merge 1 commit intomainfrom
Open
feat(oob): Adds initial oob implementation with metrics#388yanziz-nvidia wants to merge 1 commit intomainfrom
yanziz-nvidia wants to merge 1 commit intomainfrom
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: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces an out-of-band (OOB) teleop control infrastructure for Isaac Teleop CloudXR. A new TypeScript WebSocket client ( Sequence Diagram(s)sequenceDiagram
participant Headset as XR Headset<br/>(WebSocket Client)
participant Hub as OOB Control Hub<br/>(WebSocket Server)
participant Storage as Hub State<br/>(Metrics & Config)
Headset->>Hub: WebSocket Connect
activate Hub
Headset->>Hub: register message<br/>(role: "headset", token?, deviceLabel?)
Hub->>Storage: Store headset connection
Hub->>Headset: hello message<br/>(clientId, configVersion, config)
deactivate Hub
loop Every metrics interval
Headset->>Headset: Capture render/streaming FPS
Headset->>Hub: clientMetrics message<br/>(cadence, timestamp, metrics)
activate Hub
Hub->>Storage: Update metrics by cadence
deactivate Hub
end
Headset->>Hub: (periodic heartbeat via metrics)
Hub->>Headset: config message<br/>(when config updated)
Headset->>Headset: Apply new stream config
sequenceDiagram
participant Operator as HTTP API Client<br/>(Dashboard/Script)
participant Hub as OOB Control Hub<br/>(HTTP Handler)
participant Storage as Hub State<br/>(Config & Headsets)
participant Headset as Connected Headset<br/>(WebSocket)
Operator->>Hub: GET /api/oob/v1/state<br/>(token auth)
activate Hub
Hub->>Storage: Read snapshot
Hub-->>Operator: 200 OK<br/>(headsets, metrics, config, configVersion)
deactivate Hub
Operator->>Hub: GET /api/oob/v1/config?serverIP=x&port=y<br/>(token auth)
activate Hub
Hub->>Storage: Validate & merge config
alt Config changed
Hub->>Storage: Increment configVersion
Hub->>Headset: config message<br/>(updated config + version)
activate Headset
Headset->>Headset: Apply new config
deactivate Headset
Hub-->>Operator: 200 OK<br/>(changed: true, configVersion)
else No changes
Hub-->>Operator: 200 OK<br/>(changed: false)
end
deactivate Hub
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yanziz-nvidia
force-pushed
the
yanziz/oob-phase1
branch
from
af36d58 to
99d1fda
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/cloudxr/python/wss.py (1)
140-145:⚠️ Potential issue | 🔴 Critical
/api/oob/v1/configis mutable with no auth by default.When
CONTROL_TOKENis unset,hub.check_token()accepts every caller, and this handler applies live config changes from a simple GET query string while advertisingAccess-Control-Allow-Origin: *. That leaves headset reconfiguration exposed to any caller that can reach the proxy, including browser-driven cross-origin requests once the cert is trusted. Require a control token whensetup_oobis enabled, and avoid state changes on GET.Also applies to: 303-323, 500-506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/wss.py` around lines 140 - 145, The /api/oob/v1/config handler in src/core/cloudxr/python/wss.py currently allows unauthenticated mutating config via GET and advertises Access-Control-Allow-Origin: *; update the handler (the function handling the route for "/api/oob/v1/config") so that when setup_oob is enabled it enforces hub.check_token() and rejects requests lacking a valid CONTROL_TOKEN, and stop performing state changes on GET requests (accept mutating operations only via POST/PUT/ PATCH and keep GET strictly read-only). Also remove or tighten wildcard CORS (the CORS_HEADERS entry) for mutating endpoints so that Access-Control-Allow-Origin is not "*" for endpoints that change state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deps/cloudxr/webxr_client/helpers/controlChannel.ts`:
- Around line 157-173: The onclose handler currently always calls
_afterSocketClosed() which unconditionally schedules a reconnect; change the
onclose callback to accept the CloseEvent (e.g., (ev: CloseEvent) => { ... })
and pass ev.code into _afterSocketClosed, then update _afterSocketClosed to
accept an optional closeCode parameter and treat terminal codes (at least 1008
for policy/auth failure) as final by not scheduling reconnectTimer when
closeCode === 1008 (and still clear timers and call opts.onConnectionChange).
Keep disposed behavior and RECONNECT_DELAY_MS logic unchanged for non-terminal
codes.
In `@deps/cloudxr/webxr_client/src/App.tsx`:
- Around line 646-648: The code is reading the out-of-band auth token from the
URL (p.get('controlToken')) which leaks it; stop extracting the token from
window.location.search and instead accept the token via a non-URL channel (for
example an initialization config object, a secure parent.postMessage handshake,
or server-rendered JS variable) and pass that token into the
HeadsetControlChannel constructor. Update HeadsetControlChannel to use the
provided token for the WebSocket handshake/register payload (not from the URL)
and remove any usage of p.get('controlToken') so the token is never placed into
the URL or browser history/referrer.
- Around line 652-672: The getMetricsSnapshot callback is returning stale values
by reading renderMetrics.value and streamingMetrics.value even after the XR
session ends; update the XR lifecycle to clear/reset these refs on session end
(e.g., in the XR stop/disconnect handler or cloudXR2DUI teardown) or change
getMetricsSnapshot to check an active-session flag before returning metrics and
return an empty array when no session is active. Specifically, locate
getMetricsSnapshot, the renderMetrics and streamingMetrics refs, and the XR
session start/stop or cloudXR2DUI disconnect handlers (session end logic) and
either null/undefined those refs on end or gate getMetricsSnapshot behind the
session active boolean so no stale FPS/latency samples are emitted after session
exit.
In `@docs/source/references/oob_teleop_control.rst`:
- Around line 29-32: The docs currently state the WebXR client reports streaming
metrics "every frame"; update the description to say the client sends periodic
snapshots at the configured metricsIntervalMs (default 500 ms) as clientMetrics
messages instead of per-render-frame sends; adjust the XR headset / Isaac Teleop
WebXR client text and any other occurrences (e.g., the block referencing
metricsIntervalMs and clientMetrics) to mention periodic reporting at
metricsIntervalMs rather than per-frame reporting.
- Around line 36-39: Update the OOB teleop control docs to remove the incorrect
WebSocket config path: edit the bullet that currently reads "Reads state via
HTTP, optionally pushes config via HTTP or WebSocket" (and the similar text at
the other occurrence) so it only mentions HTTP (config via GET
/api/oob/v1/config) and remove any reference to a WebSocket `setConfig` path;
keep references to headset `register` and `clientMetrics` for the WebSocket
protocol as-is.
- Around line 100-101: The docs claim the /state response contains "dashboards",
but OOBControlHub.get_snapshot() actually returns only updatedAt, configVersion,
config, and headsets; update the documentation text for the /state endpoint to
remove "dashboards" and list the actual returned fields (updatedAt,
configVersion, config, headsets) so the docs match the
OOBControlHub.get_snapshot() response structure.
In `@src/core/cloudxr/python/oob_teleop_hub.py`:
- Around line 219-243: In _merge_stream_config: resolve targets before mutating
shared state — first collect all_headsets and, if target_id is provided, find
targets and return ("missing", target_id) if none; only after target resolution
compute merged values but do NOT assign into self._stream_config for targeted
updates; for target-specific pushes create per-target snapshots by applying
new_config onto a copy of the current self._stream_config (e.g., snapshot =
dict(self._stream_config); overridden = {**snapshot, **new_config}) and compare
overridden vs snapshot to decide noop vs push for those targets; only update
self._stream_config and increment self._config_version when target_id is None
(global update) and then return ("push", version, snapshot, targets) as before.
Ensure all uses reference _merge_stream_config, self._stream_config,
self._config_version, target_id, and targets.
---
Outside diff comments:
In `@src/core/cloudxr/python/wss.py`:
- Around line 140-145: The /api/oob/v1/config handler in
src/core/cloudxr/python/wss.py currently allows unauthenticated mutating config
via GET and advertises Access-Control-Allow-Origin: *; update the handler (the
function handling the route for "/api/oob/v1/config") so that when setup_oob is
enabled it enforces hub.check_token() and rejects requests lacking a valid
CONTROL_TOKEN, and stop performing state changes on GET requests (accept
mutating operations only via POST/PUT/ PATCH and keep GET strictly read-only).
Also remove or tighten wildcard CORS (the CORS_HEADERS entry) for mutating
endpoints so that Access-Control-Allow-Origin is not "*" for endpoints that
change state.
🪄 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: Pro
Run ID: 116b174b-1252-43ab-b2fa-8eb9e32b1de6
📒 Files selected for processing (12)
deps/cloudxr/webxr_client/helpers/controlChannel.tsdeps/cloudxr/webxr_client/src/App.tsxdocs/source/index.rstdocs/source/references/oob_teleop_control.rstsrc/core/cloudxr/python/CMakeLists.txtsrc/core/cloudxr/python/__main__.pysrc/core/cloudxr/python/launcher.pysrc/core/cloudxr/python/oob_teleop_hub.pysrc/core/cloudxr/python/wss.pysrc/core/cloudxr_tests/python/conftest.pysrc/core/cloudxr_tests/python/pyproject.tomlsrc/core/cloudxr_tests/python/test_oob_teleop_hub.py
yanziz-nvidia
force-pushed
the
yanziz/oob-phase1
branch
from
99d1fda to
f9c2c72
Compare
yanziz-nvidia
requested review from
aristarkhovNV,
blongs-nv,
jiwenc-nv and
nvddr
yanziz-nvidia
force-pushed
the
yanziz/oob-phase1
branch
from
f9c2c72 to
99e597c
Compare
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
1 participant
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.
Summary by CodeRabbit
New Features
--setup-oobflag when starting the CloudXR service.Documentation