Repository navigation
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
jkim89
marked this pull request as ready for review
October 27, 2026 22:48
jkim89
force-pushed
the
jonghyunk/pr03-websocket-transport
branch
from
October 30, 2026 19:12
7ea56d3 to
2aaf4b2
Compare
jkim89
force-pushed
the
jonghyunk/pr04-remote-plugin-session
branch
from
October 30, 2026 19:12
af64382 to
aca3660
Compare
RemotePluginSession implements core::IPluginSession for a plugin that runs without an OpenXR runtime: HELLO/HELLO_ACK handshake, hand, schema and pull/wrist channels as wire frames, per-channel latest-wins sample queues (control frames are never dropped), heartbeats, PONG answers to bridge PINGs, and per-channel refusal that keeps the session alive. ClockOffsetEstimator maps remote CLOCK_MONOTONIC to local time from two-way PING/PONG exchanges (min-RTT of the last 32), with the one-way minimum as fallback; PingScheduler drives the exchange. Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
Adapt RemotePluginSession to pr00's session API and pr01's split codec: - The session opens its pull channel (OPEN_PULL) in the constructor and hands it out through pull_channel(); the one-shot create_pull_channel() claim is gone. The pull channel owns the wrist source when HELLO declared wrist_tracking_pull and returns it from wrist_tracking_source(), throwing std::logic_error otherwise. OPEN_WRIST is no longer sent: the wrist config travels in HELLO. - RemoteWristTrackingSource::query takes XrHandEXT; WRIST_SAMPLE carries it. - RemoteSchemaPushChannel drops config() with ISchemaPushChannel's. - protocol builders/parsers: encode_hello, encode_open_schema, encode_open_hand(XrHandEXT), decode_ping -> int64_t, decode_error -> std::string; unexpected frame types are logged by number. stamp_header takes the frame vector and send_now() stamps a PONG's t3 with set_pong_t3 at the same instant as the header's t_send. - Tests use the reference-returning pull channel and plain hand channels (HandTrackingPusher is gone); AGENTS.md drops the HELLO v0 clock fallback since HELLO < 2 is rejected. - make_remote_plugin_session_factory(app_name, RemoteAddress) builds the core::PluginSessionFactory a --remote composition root needs; it throws std::logic_error when handed trackers, which the remote pull channel cannot expose. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
Applied the fact-checked (VERIFIED) findings on code this branch adds: - Sample queues get their frame type once, at OPEN; pushes to a closed or refused channel find no queue and return. send_control (unused) is gone. - sent_hand/sent_schema count frames after send_now succeeds, not when they are dequeued. - One hand_index() helper; PING answering no longer keeps a flag just to log once. - on_disconnect carries the real reason (bridge ERROR text, DecodeError text) instead of logging it separately; an unexpected frame type is an error; log lines drop prefixes the logger name already carries. - ClockOffsetEstimator takes the two-way window as a constructor parameter like the one-way window, and derives the two-way offset and RTTs from the exchange window instead of caching them; PingScheduler's two flags become one state enum. - Docs: any ITransport (not only TCP), no queue-size numbers or "P0", AGENTS.md keeps only the link rule and states which sample-path calls still throw on caller bugs; the BlockingTransport note lives with the test only. - Tests: FakeBridge takes an optional rejection; the PING test no longer re-tests the estimator; the reconnect test keeps only its own check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
Applied the fact-checked (VERIFIED) findings on code this branch adds: - The session owns its impl outright; channels and the wrist source hold a reference, as IPluginSession has the owner destroy channels first. The explicit shutdown in the destructor and the test of channels outliving the session are gone. - Removed the host/port constructor; tests connect a TcpTransport. - Dropped redundant copy/move deletions and the unread ChannelQueue::type. - require_connected is private to the impl. - A HELLO_ACK after the handshake or an unknown frame type disconnects as a protocol mismatch instead of being logged and ignored. - The stats line prefix comes from kStatsPeriod; docs no longer hardcode it. - ClockOffsetEstimator drops an always-true empty check; the two_way_window == 0 rejection is tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
Applied the fact-checked (VERIFIED) findings on code this branch adds: - A test destroyed a push channel after its session (use after free); channels now close inside the session's scope. - shutdown() runs once by construction: dropped the m_shutdown guard and the CLOSE sweep for channels that, by contract, are already closed. - push_sample drops a connected check its callers already make. - The stats snapshot is a local of the sender loop. - app_name is moved through to HELLO. - ClockOffsetEstimator::offset_source returns an enum. - The handshake test checks app_name; the duplicate transport test is gone. - AGENTS.md no longer describes bridge flags that are not in this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
jkim89
force-pushed
the
jonghyunk/pr03-websocket-transport
branch
from
October 30, 2026 21:42
2aaf4b2 to
506f44a
Compare
jkim89
force-pushed
the
jonghyunk/pr04-remote-plugin-session
branch
from
October 30, 2026 21:42
aca3660 to
d1f8f97
Compare
Contributor
Author
|
Update (Oct 30): rebased onto the reviewed #1320. RemotePluginSession now follows the session-owned pull channel: OPEN_PULL is sent from the ctor, pull_channel() is a reference, the one-shot claim is gone; channels hold plain references. Wrist config travels in HELLO v2. Run through the review skill locally (3 rounds); a test use-after-free introduced during that loop was caught and fixed. |
This branch was successfully deployed
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
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
RemotePluginSession: acore::IPluginSessionthat runs in a plugin process without an OpenXR runtime and forwards its channels to a bridge.What changes
ClockOffsetEstimator(two-way PING/PONG, min-RTT of the last 32, one-way fallback) andPingSchedulerWhat does not change
No plugin uses it yet; sample paths never throw — they become no-ops after a transport failure.
Testing
test_remote_plugin_session.cpp(15 cases against an in-process fake bridge and aBlockingTransport) andtest_clock_offset_estimator.cpp(12 cases). Fresh build: ctest 298/298.Depends on
#1322