Add QUIC operator transport#9
Conversation
- Introduce OperatorChannel/QuicOperatorServer and wire them into the module context, letting forward_status()/wait_for_command() route through QUIC when quic_port is configured, falling back to the existing Fleet HTTP API otherwise - Add msquic as a new opt-in dependency, pulled in only when FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER is set - Parse new quic_* config keys (quic_port, quic_cert_path, quic_key_path, quic_ca_path, quic_alpn, quic_listen_address, quic_disable_client_auth) and validate required TLS material before starting the server - Add mutex locking around con->devices in device_connected()/device_disconnected(), now required because wait_for_command()'s QUIC branch reads it from a different thread
- pass a real epoch timestamp to sendStatus() instead of hardcoded 0, so operator-side status messages carry accurate timing - only take con->mutex in device_connected()/device_disconnected() when con->quic_server is configured, since the lock is only needed for wait_for_command()'s QUIC branch and unconditional locking would add the Fleet-HTTP blocking-call stall to every (dis)connect event in non-QUIC deployments - wrap handleCommandBytes() envelope parsing in try/catch for nlohmann::json::exception, since it runs inside a QUIC_API callback invoked by msquic's C library — an uncaught type_error from a malformed field would otherwise terminate the whole external-server process
- replace the plain-JSON envelope with OperatorMessage (proto/transparent_operator_stream.proto) in sendStatus() and handleCommandBytes(), matching teleop-module's QuicOperatorServer this was ported from - give it its own .proto/package instead of sharing teleop-module's, since external-server-cpp dlopens both shared libraries into the same process and protobuf's generated-descriptor registry is process-global - keep the BAF-1651 JSON envelope riding as opaque bytes inside StatusUpdate/CommandRequest, so this class stays decoupled from that schema - add Protobuf as a build dependency (cmake/Dependencies.cmake, CMakeLists.txt) and wire PROTOBUF_GENERATE_CPP for the new .proto
- implement forward_command_on_receive() to return OK, mirroring teleop-module's module_manager.cpp - without this, MG's default fallback re-invokes generate_command() on every DeviceStatus and testing_device_generate_command() echoes current_command back, so the same command gets redelivered to the device on every status tick forever - returning OK switches MG to push-only mode: new commands forward immediately and a status with nothing new gets an EMPTY DeviceCommand instead of the stale one
WalkthroughThis PR introduces a QUIC-based operator transport for the transparent module's external server. It adds build dependencies (msquic, Protobuf), a protobuf schema for operator/status/command messages, a thread-safe OperatorChannel, a QuicOperatorServer implementation, and integrates them into external_server_api.cpp for QUIC-enabled status forwarding and command reception, alongside a push-only command forwarding change in module_manager.cpp. ChangesQUIC Operator Stream Transport
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ExternalServerAPI
participant QuicOperatorServer
participant OperatorChannel
participant OperatorClient
Note over ExternalServerAPI,QuicOperatorServer: Initialization
ExternalServerAPI->>QuicOperatorServer: initialize(), start()
Note over ExternalServerAPI,OperatorClient: Status send
ExternalServerAPI->>QuicOperatorServer: sendStatus(payload, timestamp_ms)
QuicOperatorServer->>OperatorClient: unidirectional stream (StatusUpdate)
Note over OperatorClient,ExternalServerAPI: Command receive
OperatorClient->>QuicOperatorServer: unidirectional stream (CommandRequest)
QuicOperatorServer->>OperatorChannel: push(OperatorCommand)
ExternalServerAPI->>OperatorChannel: waitForCommand(timeout)
OperatorChannel-->>ExternalServerAPI: OperatorCommand or nullopt
Related PRs: None identified. Suggested labels: enhancement, needs-review Suggested reviewers: None identified. Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@proto/transparent_operator_stream.proto`:
- Line 22: The proto file location does not match the declared package path,
causing Buf’s PACKAGE_DIRECTORY_MATCH lint to fail. Fix this by either moving
transparent_operator_stream.proto into the
bringauto/transparent/operator_stream/ directory structure and updating the
PROTOBUF_GENERATE_CPP source path plus any generated-header include references,
or by explicitly ignoring this lint in buf.yaml if the flat proto layout is
intentional. Use the package declaration bringauto.transparent.operator_stream
and the PROTOBUF_GENERATE_CPP setup in CMakeLists.txt to locate the related
changes.
In `@source/transparent_operator_stream/QuicOperatorServer.cpp`:
- Around line 332-350: The QUICOperatorServer::PEER_STREAM_STARTED handling
closes rejected peer streams too early, bypassing the required shutdown cleanup
path. Update the rejected-stream branch to first register a stream callback via
SetCallbackHandler for the incoming stream, then abort/shutdown the stream and
let the callback handle StreamClose after SHUTDOWN_COMPLETE; keep the
accepted-stream ownership flow in commandStreamCallback unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24ba599a-bde5-4614-ae3e-18496e8c9c1c
📒 Files selected for processing (10)
CMakeLists.txtcmake/Dependencies.cmakeinclude/bringauto/transparent_module_utils/external_server_api_structures.hppinclude/bringauto/transparent_module_utils/operator_stream/OperatorChannel.hppinclude/bringauto/transparent_module_utils/operator_stream/QuicOperatorServer.hppproto/transparent_operator_stream.protosource/external_server_api.cppsource/module_manager.cppsource/transparent_operator_stream/OperatorChannel.cppsource/transparent_operator_stream/QuicOperatorServer.cpp
| // VERBATIM as opaque bytes — this protocol only carries the Fleet Protocol device_id + timestamp | ||
| // needed to route the message, and stays decoupled from the BAF-1651 schema. | ||
| // NB: `operator_stream`, not `operator` — `operator` is a C++ keyword (invalid generated namespace). | ||
| package bringauto.transparent.operator_stream; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Buf lint failure: package path doesn't match file location.
Buf's PACKAGE_DIRECTORY_MATCH rule requires the file for package bringauto.transparent.operator_stream to live under bringauto/transparent/operator_stream/, but it sits in proto/. If buf lint gates CI this will fail the build.
Options: relocate the file to proto/bringauto/transparent/operator_stream/transparent_operator_stream.proto (and update the PROTOBUF_GENERATE_CPP path in CMakeLists.txt line 81 plus the generated-header include), or exclude/ignore this rule in buf.yaml if the flat layout is intentional.
🧰 Tools
🪛 Buf (1.71.0)
[error] 22-22: Files with package "bringauto.transparent.operator_stream" must be within a directory "bringauto/transparent/operator_stream" relative to root but were in directory "proto".
(PACKAGE_DIRECTORY_MATCH)
🤖 Prompt for 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.
In `@proto/transparent_operator_stream.proto` at line 22, The proto file location
does not match the declared package path, causing Buf’s PACKAGE_DIRECTORY_MATCH
lint to fail. Fix this by either moving transparent_operator_stream.proto into
the bringauto/transparent/operator_stream/ directory structure and updating the
PROTOBUF_GENERATE_CPP source path plus any generated-header include references,
or by explicitly ignoring this lint in buf.yaml if the flat proto layout is
intentional. Use the package declaration bringauto.transparent.operator_stream
and the PROTOBUF_GENERATE_CPP setup in CMakeLists.txt to locate the related
changes.
Source: Linters/SAST tools
| case QUIC_CONNECTION_EVENT_PEER_STREAM_STARTED: { | ||
| // Only the accepted operator may feed the command channel. A stream opened by any other | ||
| // connection (a second operator being rejected, or one mid-shutdown) is rejected so its | ||
| // bytes never reach the device. Phase 1 is single-operator. | ||
| // msquic requires every stream surfaced in PEER_STREAM_STARTED to be either accepted | ||
| // (SetCallbackHandler) or closed (StreamClose); without a registered handler there is no | ||
| // SHUTDOWN_COMPLETE to reclaim on, so StreamShutdown alone would leak the handle. StreamClose | ||
| // rejects it cleanly. | ||
| if (connection != self->operatorConnection_.load()) { | ||
| self->quic_->StreamClose(event->PEER_STREAM_STARTED.Stream); | ||
| return QUIC_STATUS_SUCCESS; | ||
| } | ||
| auto streamCtx = std::make_unique<InboundStreamContext>(InboundStreamContext{self, connection, {}}); | ||
| self->quic_->SetCallbackHandler(event->PEER_STREAM_STARTED.Stream, | ||
| reinterpret_cast<void *>(commandStreamCallback), streamCtx.get()); | ||
| // Ownership transfers to commandStreamCallback; reclaimed on SHUTDOWN_COMPLETE. | ||
| streamCtx.release(); | ||
| return QUIC_STATUS_SUCCESS; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the reviewed file and dependency pin
git ls-files 'source/transparent_operator_stream/QuicOperatorServer.cpp' 'cmake/Dependencies.cmake' 'CMakeLists.txt' '*msquic*'
echo '--- Dependencies.cmake ---'
sed -n '1,260p' cmake/Dependencies.cmake
echo '--- Relevant section in QuicOperatorServer.cpp ---'
sed -n '280,380p' source/transparent_operator_stream/QuicOperatorServer.cppRepository: bringauto/transparent-module
Length of output: 6080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any local references to StreamClose / StreamShutdown handling patterns
rg -n "StreamClose|StreamShutdown|SHUTDOWN_COMPLETE|SetCallbackHandler" source/transparent_operator_stream/QuicOperatorServer.cpp source -g '!**/build/**' -g '!**/dist/**'Repository: bringauto/transparent-module
Length of output: 4992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the msquic version pin and nearby code context more precisely
python3 - <<'PY'
from pathlib import Path
p = Path('cmake/Dependencies.cmake')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 'msquic' in line.lower() or 'QUIC' in line:
print(f"{i}: {line}")
PYRepository: bringauto/transparent-module
Length of output: 339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the local msquic API surface or vendored docs if present
fd -a 'StreamClose*|StreamShutdown*|PEER_STREAM_STARTED*|msquic*' . \
| sed -n '1,200p'Repository: bringauto/transparent-module
Length of output: 166
🌐 Web query:
msquic v2.5.6 StreamClose docs "The application should only close a stream after it has been completely shut down or if it was never successfully started" PEER_STREAM_STARTED SetCallbackHandler required immediately
💡 Result:
In MsQuic, the StreamClose documentation states that an application should only call StreamClose after a stream has been completely shut down or if it was never successfully started [1][2]. Closing a stream before it is fully shut down leads to undefined behavior, as MsQuic must assume an error code (typically 0) to signal the closure to the peer [1][3]. If an application needs to discard a stream quickly, it should first perform an abortive StreamShutdown with an appropriate error code, wait for the QUIC_STREAM_EVENT_SHUTDOWN_COMPLETE event, and then call StreamClose [1][4]. Regarding the QUIC_CONNECTION_EVENT_PEER_STREAM_STARTED event, this event is delivered to the connection callback when a peer initiates a new stream [5][6]. When an application receives this event, it is required to set a callback handler for the new stream (using SetCallbackHandler) before or when returning from the event callback, to ensure that subsequent events for that stream can be delivered [5]. If no handler is set, the application will not be able to process events for that newly created stream [7]. If the application does not wish to use the stream, it should still set a handler and then call StreamShutdown (or optionally StreamClose if it is immediately abandoned) to clean up the resource [1][4].
Citations:
- 1: https://microsoft.github.io/msquic/msquicdocs/docs/api/StreamClose.html
- 2: https://github.com/microsoft/msquic/blob/main/docs/api/StreamClose.md
- 3: https://microsoft.github.io/msquic/msquicdocs/docs/Streams.html
- 4: https://microsoft.github.io/msquic/msquicdocs/docs/api/StreamShutdown.html
- 5: https://github.com/microsoft/msquic/blob/main/docs/api/QUIC_CONNECTION_EVENT.md
- 6: https://microsoft.github.io/msquic/msquicdocs/docs/api/QUIC_CONNECTION_EVENT.html
- 7: https://microsoft.github.io/msquic/msquicdocs/docs/api/StreamStart.html
🌐 Web query:
msquic StreamClose PEER_STREAM_STARTED SetCallbackHandler immediately close after shutdown undefined behavior docs
💡 Result:
In MsQuic, closing a stream using StreamClose before it has been completely shut down is explicitly documented as undefined behavior [1][2]. This is because clean up of the stream must be properly reflected on the wire with an application-specific error code; calling StreamClose prematurely forces MsQuic to guess the error code (currently using 0), which violates the expected protocol shutdown sequence [1][3]. When handling the QUIC_CONNECTION_EVENT_PEER_STREAM_STARTED event, the application is required to set a callback handler for the new stream [4][5]. If the application decides to reject or immediately close this stream: 1. Do not simply call StreamClose on the stream handle provided in the event [6]. 2. You must first ensure the stream is correctly transitioned through a shutdown state [1][3]. 3. If you do not wish to use the stream, you should still register the required callback handler, then perform an abortive shutdown (e.g., calling StreamShutdown with the QUIC_STREAM_SHUTDOWN_FLAG_ABORT flag), wait for or trigger the QUIC_STREAM_EVENT_SHUTDOWN_COMPLETE event, and only then call StreamClose [1][3][7]. It is possible to perform this sequence (abortive shutdown and immediate close) inline during the same callback if necessary, provided the shutdown is handled correctly [3][7]. Any use of the stream handle after StreamClose has been called is a use-after-free error [1]. Failure to follow these steps—such as calling StreamClose immediately upon receiving PEER_STREAM_STARTED without a proper shutdown—can lead to undefined behavior, including assertion failures or incorrect protocol state [1][6][8].
Citations:
- 1: https://github.com/microsoft/msquic/blob/main/docs/api/StreamClose.md
- 2: https://microsoft.github.io/msquic/msquicdocs/docs/api/StreamClose.html
- 3: https://microsoft.github.io/msquic/msquicdocs/docs/Streams.html
- 4: https://github.com/microsoft/msquic/blob/main/src/tools/sample/sample.c
- 5: https://github.com/microsoft/msquic/blob/main/docs/API.md
- 6: Null pointer dereference on allocation failure in quicforward microsoft/msquic#5806
- 7: https://github.com/microsoft/msquic/blob/main/docs/api/StreamShutdown.md
- 8: Assertion fails on GameCore when ethernet is unplugged: Sqe->Overlapped.Internal != 0x103 microsoft/msquic#5809
Rejected peer streams need a shutdown path before StreamClose. Calling StreamClose here skips the required cleanup for a stream that has already started; msquic documents that closing before complete shutdown is undefined behavior. Register a handler, abort the stream, and close it on SHUTDOWN_COMPLETE instead.
🤖 Prompt for 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.
In `@source/transparent_operator_stream/QuicOperatorServer.cpp` around lines 332 -
350, The QUICOperatorServer::PEER_STREAM_STARTED handling closes rejected peer
streams too early, bypassing the required shutdown cleanup path. Update the
rejected-stream branch to first register a stream callback via
SetCallbackHandler for the incoming stream, then abort/shutdown the stream and
let the callback handle StreamClose after SHUTDOWN_COMPLETE; keep the
accepted-stream ownership flow in commandStreamCallback unchanged.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes