Migrate QuicOperatorServer onto shared ba-quic-lib - #11
Conversation
danprudky
commented
Jul 27, 2026
- Replace the hand-rolled msquic registration/listener/connection/stream callback machinery in QuicOperatorServer with the shared, transport-only bringauto::quic::QuicServer (ba-quic-lib), mirroring teleop-module's own migration; public API (initialize/start/stop/sendStatus/hasOperator) is unchanged so external_server_api.cpp needs no changes
- Single-operator enforcement (maxConnections = 1) now happens at the transport layer, so the class no longer needs its own compare-and-swap "already have an operator" logic
- Add cmake/FindBAQuicLib.cmake and wire FIND_PACKAGE(BAQuicLib) into CMakeLists.txt; drop the direct msquic BA_PACKAGE_LIBRARY pin from Dependencies.cmake since msquic is now pulled in transitively via ba-quic-lib's PUBLIC link
|
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 ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Review
Scoped to this commit, the migration is a clear win: ~420 lines of hand-rolled msquic registration/listener/stream callback machinery replaced by the shared transport, public API genuinely unchanged, and the reasoning in FindBAQuicLib.cmake about BRINGAUTO_TESTS/BRINGAUTO_INSTALL option leakage through add_subdirectory is correct and non-obvious — good that it's written down.
Findings inline. Nothing here is a blocker on its own; #1 and #2 are the ones I'd want resolved before this ships in a package.
Also worth noting (no clean anchor)
stop()lost its idempotence guard. The old version opened withif (!running_.exchange(false)) { return; }; the new one callsquicServer_->stop()andchannel_.shutdown()unconditionally, and~QuicOperatorServer()callsstop(). An explicitstop()followed by destruction now runs both twice. Probably harmless givenba-quic-lib's own guards, but it was deliberate before and is gone now without comment.- The header comment at line 33 still says the QUIC path is "used alongside (not instead of) the existing Fleet HTTP API path". In the base branch
forward_status/wait_for_commandreplace the HTTP path whenquic_portis set. Pre-existing, but this PR rewrites the surrounding block, so it's a cheap fix while you're here.
…c coupling - #3664220610 msquic include: drop <msquic.h> for one enum value, use literal with comment instead of relying on ba-quic-lib's transitive PUBLIC link - #3664220614 maxConnections race: confirmed against quic-lib's QuicServer source that onConnected() is never invoked for a connection rejected by maxConnections; tightened the comment to state this explicitly - #3664220624 regex on CMAKE_BINARY_DIR: replace MATCHES with string(FIND ...) to avoid regex metacharacter/anchoring issues - #3664220631 CMAKE_MODULE_PATH: move append to top-level setup, next to the other global includes, instead of inside a conditional block - #3664220605 msquic packaging: document how BA_PACKAGE_DEPS_IMPORTED still transitively ships msquic's .so through ba-quic-lib Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ng-control Add QUIC operator transport
…b transport - Replace the hand-rolled msquic registration/listener/connection/stream callback machinery in QuicOperatorServer with the shared, transport-only bringauto::quic::QuicServer (ba-quic-lib), mirroring teleop-module's own migration; public API (initialize/start/stop/sendStatus/hasOperator) is unchanged so external_server_api.cpp needs no changes - Single-operator enforcement (maxConnections = 1) now happens at the transport layer, so the class no longer needs its own compare-and-swap "already have an operator" logic - Add cmake/FindBAQuicLib.cmake and wire FIND_PACKAGE(BAQuicLib) into CMakeLists.txt; drop the direct msquic BA_PACKAGE_LIBRARY pin from Dependencies.cmake since msquic is now pulled in transitively via ba-quic-lib's PUBLIC link
…c coupling - #3664220610 msquic include: drop <msquic.h> for one enum value, use literal with comment instead of relying on ba-quic-lib's transitive PUBLIC link - #3664220614 maxConnections race: confirmed against quic-lib's QuicServer source that onConnected() is never invoked for a connection rejected by maxConnections; tightened the comment to state this explicitly - #3664220624 regex on CMAKE_BINARY_DIR: replace MATCHES with string(FIND ...) to avoid regex metacharacter/anchoring issues - #3664220631 CMAKE_MODULE_PATH: move append to top-level setup, next to the other global includes, instead of inside a conditional block - #3664220605 msquic packaging: document how BA_PACKAGE_DEPS_IMPORTED still transitively ships msquic's .so through ba-quic-lib Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_TOKEN_URI
- FetchContent_Declare cloned quic-lib over anonymous HTTPS, which TeamCity build agents can't authenticate ("fatal: could not read Username"), failing the configure step before any compilation happens
- inject the org's standard BA_GITLAB_TOKEN_URI env credential into the GIT_REPOSITORY URL via a local _token variable, matching the pattern already used by lightdbw-fleet-bridge and lightdbw-vehicle-control's FetchContent-based dependencies
204985d to
92223bd
Compare