feat(web): protocol v5, robot hello on an @control stream - #3567
feat(web): protocol v5, robot hello on an @control stream#3567paul-nechifor wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## paul/feat/w2-sdk-serving #3567 +/- ##
============================================================
+ Coverage 77.39% 77.42% +0.02%
============================================================
Files 1266 1266
Lines 121416 121551 +135
Branches 11226 11235 +9
============================================================
+ Hits 93974 94114 +140
+ Misses 24364 24362 -2
+ Partials 3078 3075 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Greptile SummaryThe protocol v5 change moves robot hellos onto control streams, but two reliability failures remain. Large valid manifests can lead to subscription snapshots that exceed the datagram limit and are still sent as a single datagram. Separately, a delayed relay can have every pending hello stream reset before it consumes one, causing registration to time out. Confidence Score: 3/5Not safe to merge until robot hello retries preserve deliverable streams and oversized subscription state has a reliable delivery or rejection path. Two independent blocking reliability failures were reproduced: delayed relay consumption can prevent robot registration, and large valid subscription sets are sent through a datagram path that cannot carry them. Files Needing Attention: dimos/web/relay_bridge/wt_client.py, web/relay/registry.ts
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const msg: Msg = { t: "subs", chs, n: ++entry.n }; | ||
| const size = encodeDatagram(msg).byteLength; | ||
| if (size > DATAGRAM_BUDGET_BYTES) { | ||
| // Unreachable while subs are manifest-validated (a manifest that fit | ||
| // its hello datagram implies a fitting snapshot). Loud if it ever | ||
| // happens: an oversized snapshot silently never reaches the robot. | ||
| // Reachable since v5: a stream hello can declare a channel set whose | ||
| // full snapshot no longer fits one datagram (W4 moves snapshots to the | ||
| // reliable carrier). Loud because an oversized snapshot silently never | ||
| // reaches the robot. | ||
| console.error(`[relay] subs snapshot for ${robotId} is ${size} B (over datagram budget)`); | ||
| } | ||
| entry.peer.sendMsg(msg); |
There was a problem hiding this comment.
A valid large manifest can produce a complete subs snapshot larger than the 1200-byte datagram budget. This branch only logs the overflow and still sends the single oversized datagram, so the robot never receives the authoritative subscription state; periodic snapshot retries remain oversized as the active set grows. Send this state on a reliable control stream, fragment it, or enforce a channel-count limit that keeps its encoded snapshot within the datagram budget.
Artifacts
Focused large-manifest reproduction source
- Executes the registry subscription path with a valid 32-channel manifest, a 1200-byte datagram contract, and a follow-up teleop control message; takeaway: the source directly exercises the claimed oversized-snapshot path.
PR oversized-snapshot runtime output
- Captured PR execution shows protocol v5 accepting the large manifest and sending a 1211-byte subscription snapshot over the 1200-byte budget while follow-up teleop remains accepted; takeaway: valid v5 manifests can produce an undeliverable subscription snapshot.
| def retire_hello_stream() -> None: | ||
| # In-flight check and reset in the same event-loop turn (the | ||
| # aioquic-safe reset rule, web/README.md bug 9); a delivered | ||
| # stream is left alone so a reset cannot destroy a hello the | ||
| # relay has yet to read. | ||
| if hello_stream is not None and self._session.stream_in_flight(hello_stream): | ||
| self._session.reset_if_in_flight(hello_stream) | ||
|
|
||
| try: | ||
| while True: | ||
| if control_payload is None: | ||
| self._session.send_msg(msg) | ||
| else: | ||
| retire_hello_stream() | ||
| hello_stream = self.send_frame(CONTROL_CHANNEL, control_payload) | ||
| with contextlib.suppress(asyncio.TimeoutError): | ||
| await asyncio.wait_for(self._session.welcomed.wait(), 0.2) |
There was a problem hiding this comment.
stream_in_flight() only means the sender has not received a transport ACK; it does not mean the relay has not yet consumed the stream. After each 200 ms wait, this retry path resets that still-deliverable hello stream before resending. When ACK and relay consumption are delayed beyond the retry interval, every hello is reset before the relay reads it and the robot handshake times out. Keep prior hello streams live until a terminal handshake result, connection close, or a protocol-level acknowledgement that proves they can be retired.
Artifacts
Focused delayed-ack robot hello reproduction source
- The executed Python harness drives RelayClient.hello against a transport that delays ACK and relay consumption to 350 ms, showing whether reset destroys the still-deliverable stream.
Delayed-ack handshake without destructive stream resets
- The identical delayed transport completed welcome at 351 ms and recorded relay consumption of stream 101 when resets did not cancel delivery, proving the stream remained deliverable.
Current retry behavior with destructive stream resets
- The current RelayClient.hello behavior reset streams 101 through 104, recorded no relay consumption, and timed out after 803 ms, confirming the claimed failure.
- Protocol v5. The robot's hello moves from lossy datagrams to an @control data frame on a fresh one-shot bidi stream, resent until welcome arrives. - The hello payload keeps the datagram encoding but may now be 64 KiB, which frees the manifest from the ~1100 B datagram budget. - Channel ids beginning with @ are reserved for protocol control. Manifests using them and datagram hellos are rejected with explicit reject reasons. - Shared fixtures regenerated for v5. Relay session/forward/registry and the Python bridge client change together, pinned by the golden wire vectors. - server_test.ts: the v5 handshake updates are applied on top of the deflaked backpressure test from main (#3555). This branch adds no flake fix of its own.
40fc6dd to
384d476
Compare
| const size = encodeDatagram(msg).byteLength; | ||
| if (size > DATAGRAM_BUDGET_BYTES) { | ||
| // Unreachable while subs are manifest-validated (a manifest that fit | ||
| // its hello datagram implies a fitting snapshot). Loud if it ever | ||
| // happens: an oversized snapshot silently never reaches the robot. | ||
| // Reachable since v5: a stream hello can declare a channel set whose | ||
| // full snapshot no longer fits one datagram (W4 moves snapshots to the | ||
| // reliable carrier). Loud because an oversized snapshot silently never | ||
| // reaches the robot. | ||
| console.error(`[relay] subs snapshot for ${robotId} is ${size} B (over datagram budget)`); | ||
| } | ||
| entry.peer.sendMsg(msg); |
There was a problem hiding this comment.
A valid large manifest can produce a complete subs snapshot larger than the 1200-byte datagram budget. This branch logs the overflow but still calls entry.peer.sendMsg(msg), which sends the snapshot over the robot datagram path. The robot therefore cannot receive the authoritative subscription state, so requested feeds can remain unsubscribed. Send these snapshots on a reliable control stream, fragment them, or reject/cap subscriptions before sending an oversized state.
Artifacts
Runtime reproduction script for a large subscription snapshot
- Authored TypeScript harness registers a valid 100-channel robot and drives viewer subscription messages through the public registry API, ending with encoded-size and send observations. It demonstrates the precise oversized snapshot path.
Runtime reproduction output before the PR change
- Captured output from the identical harness in detached `HEAD^`; it exits 0 and shows a 3,128-byte snapshot over the 1,200-byte budget still recorded as sent. The behavior existed before the PR change.
Runtime reproduction output on the current PR
- Captured output from the identical harness on current `HEAD`; it exits 0, logs the over-budget condition, and reports that the 3,128-byte snapshot was still sent. The root finding is reproduced.
Existing registry test attempt
- Captured attempt to run the existing registry tests with Bun; it fails before tests execute because Bun cannot resolve the Deno `@std/assert` import mapping. This does not block the standalone runtime reproduction.
| def retire_hello_stream() -> None: | ||
| # In-flight check and reset in the same event-loop turn (the | ||
| # aioquic-safe reset rule, web/README.md bug 9); a delivered | ||
| # stream is left alone so a reset cannot destroy a hello the | ||
| # relay has yet to read. | ||
| if hello_stream is not None and self._session.stream_in_flight(hello_stream): | ||
| self._session.reset_if_in_flight(hello_stream) |
There was a problem hiding this comment.
Robot hello retries reset the previous stream whenever it is still transport-unacknowledged after 200 ms. stream_in_flight() describes sender ACK state, not whether the relay has consumed the stream. With relay consumption delayed past that boundary, each retry withdraws the still-deliverable hello before the relay can read it, so registration times out. Keep prior hello streams live until a terminal handshake result, connection close, or an application-level acknowledgement establishes that retirement is safe.
Artifacts
Deterministic hello retry harness source
- Review-authored source creates an ACK-pending stream whose relay consumption is deliberately delayed, then invokes current RelayClient.hello; it provides the executable reproduction.
Delayed unread hello reset reproduction, first run
- Executed current-code harness with 350 ms relay consumption delay and persistent no-ACK state; it shows retries reset every hello and the handshake times out.
Delayed unread hello reset reproduction, second run
- Independent executed current-code harness run reproduces the same timeout and reset sequence, while the 50 ms consumption control completes with welcome; it confirms the failure depends on passing the 200 ms retry boundary.
Focused existing robot hello test attempt
- Attempted the repository's focused robot hello tests, but pytest collection stopped on missing unrelated reactivex dependency; it shows existing-test execution was environment-blocked rather than silently omitted.
github-actions
Bot
added
the
ready-to-merge
1 participant
No description provided.