fix(cloudxr): tear down on signal and join main on Orin - #816
Conversation
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
f56411b to
217c5f2
Compare
|
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 shutdown can now join and destroy the native service on the main thread or a worker thread based on Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Environment
participant CloudXRRuntime
participant NativeCloudXRService
participant WorkerThread
Environment->>CloudXRRuntime: Provide join policy
CloudXRRuntime->>CloudXRRuntime: Resolve main-thread or worker-thread path
alt Main-thread path
CloudXRRuntime->>NativeCloudXRService: Join and destroy on main thread
else Worker-thread path
CloudXRRuntime->>WorkerThread: Run blocking join and destroy
WorkerThread->>NativeCloudXRService: Join and destroy
end
sequenceDiagram
participant OS
participant CloudXRLauncher
participant WSSProxy
participant CloudXRRuntime
OS->>CloudXRLauncher: SIGTERM or SIGINT
CloudXRLauncher->>CloudXRLauncher: Restore previous handlers
CloudXRLauncher->>WSSProxy: Stop proxy
CloudXRLauncher->>CloudXRRuntime: Terminate runtime
CloudXRLauncher->>OS: Chain previous handler
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cloudxr/python/launcher.py`:
- Line 460: Update the launcher teardown flow around _restore_signal_handlers so
signal handlers remain active while _stop_wss_proxy() and _terminate_runtime()
complete. Move handler restoration into a finally block after both teardown
operations, and add a re-entry guard to prevent repeated signal handling during
teardown.
- Around line 543-550: Update _restore_signal_handlers() to inspect the current
handler before restoring each saved signal, and only call signal.signal() when
it is still the launcher-installed wrapper. Remove each entry from
_prev_signal_handlers only after a successful restore, retaining it when
restoration fails so a worker-thread stop cannot discard bookkeeping or
overwrite a host-installed handler.
In `@src/core/cloudxr/python/runtime.py`:
- Around line 340-356: Add an out-of-band shutdown mechanism for the
_should_join_main() branch so SIGINT/SIGTERM can invoke nv_cxr_service_stop(svc)
while the main thread is blocked in nv_cxr_service_join(). Preserve the existing
main-thread join and destruction flow, and add an Orin-specific regression test
verifying SIGTERM stops and cleans up the service successfully.
🪄 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: dc852a71-f2d4-48a1-b097-19e0b3fa38eb
📒 Files selected for processing (4)
docs/source/references/cloudxr.rstsrc/core/cloudxr/python/launcher.pysrc/core/cloudxr/python/runtime.pysrc/core/cloudxr_tests/python/test_runtime.py
8f4486e to
9771336
Compare
9771336 to
0ffb54e
Compare
|
Is it possible to converge on the same Main-thread join code path on all platforms? |
SIGTERM/SIGINT (+ atexit) stop the session-scoped runtime. Main-thread join is auto on Orin; override with ISAAC_TELEOP_CLOUDXR_JOIN_MAIN. Signed-off-by: Michael Haselton <mhaselton@nvidia.com>
Note ISAAC_TELEOP_CLOUDXR_* are auto-selected on Orin and override-only. Document them in a separate table in the CloudXR reference. Signed-off-by: Michael Haselton <mhaselton@nvidia.com>
Restore prior handlers in a finally block and guard re-entrant stop() so a second SIGINT/SIGTERM cannot hit SIG_DFL mid-cleanup. Signed-off-by: Michael Haselton <mhaselton@nvidia.com>
Keep bookkeeping on failed restore so a non-main-thread stop cannot drop it or overwrite a host-installed handler. Signed-off-by: Michael Haselton <mhaselton@nvidia.com>
Orin aborts with autoTSSkey if a helper thread starts after the native service is up. Keep main-thread join only. Signed-off-by: Michael Haselton <mhaselton@nvidia.com>
0ffb54e to
d8ec8ba
Compare
The thought was to keep this workaround Orin-specific. On other platforms the worker-thread join is useful so the main thread can still run Python signal handlers (Ctrl+C) while blocked in native On Orin, starting any new Python thread after the native service is up aborts with I've added more clarification around the workaround in |
Description
Signalled shutdown of an embedding process previously left the CloudXR runtime running. The runtime is spawned with
start_new_session=True, and Python does not runatexithandlers on SIGTERM/SIGINT, so the next start could fail withPORT_UNAVAILABLE. This change installs SIGTERM/SIGINT handlers (andatexit) afterPopenso the session-scoped runtime is torn down cleanly, then invokes any previously installed handler so the embedding app keeps its normal shutdown behavior. Teardown keeps those handlers installed until stop finishes, guards re-entrantstop(), and restores a prior handler only when the launcher wrapper is still installed.Separately, joining
nv_cxr_service_joinon a worker thread can abort on Orin with_PyGILState_NoteThreadState: Couldn't create autoTSSkey mappingafter V4L2 encoder teardown. Main-thread join is selected automatically on Orin. Because Python handlers cannot run while that join blocks, SIGINT/SIGTERM are handled viasigwaitsonv_cxr_service_stopcan still run. Override withISAAC_TELEOP_CLOUDXR_JOIN_MAIN=0|1(unset keeps the platform default). Launcher overrides (ISAAC_TELEOP_CLOUDXR_EXP,ISAAC_TELEOP_CLOUDXR_JOIN_MAIN) are documented as Orin auto / override-only in the CloudXR reference, with a short note under the quick-start env table.Type of change
Testing
ctest --test-dir build --output-on-failure -R 'cloudxr'(launcher + runtime, including join-main sigwait coverage).PORT_UNAVAILABLE.Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCO