Migrate QuicOperatorServer onto shared ba-quic-lib - #11
Conversation
WalkthroughThe change replaces direct MsQuic integration with ChangesOperator QUIC migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant QuicServer
participant QuicOperatorServer
participant Channel
Operator->>QuicServer: send command bytes
QuicServer->>QuicOperatorServer: invoke onBytesReceived
QuicOperatorServer->>Channel: push OperatorCommand
QuicOperatorServer->>QuicServer: send serialized status bytes
QuicServer-->>Operator: deliver status
Possibly related PRs
🚥 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 |
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…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>
…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
vbartak
left a comment
vbartak
left a comment
There was a problem hiding this comment.
Follow-up on the fixes (de2254e..09cfdf5)
Re-reviewed the three fix commits. 4 of 6 findings are properly resolved:
- ✅
<msquic.h>for one constant — include dropped, literal2with a comment explaining exactly why. Clean. - ✅
maxConnections/ CAS removal — you went and checked quic-lib'sQuicServer.cppand confirmedConnectionShutdownfires beforecallbacks_.onConnected()for a rejected attempt. That's the answer I was after, and the comment now records it rather than assuming it. No guard needed. - ✅
MATCHESregex on a path —string(FIND ...)+EQUAL 0, anchored. Correct. - ✅
CMAKE_MODULE_PATH— moved to top-level next to the CMLIBFIND_PACKAGE.
Three things left, one of them new and more serious than anything in my original pass. Details inline.
Not addressed, both from my "worth noting" list and both still fine to defer: stop() has no idempotence guard, and the QuicOperatorServer.hpp:33 "alongside (not instead of)" comment still contradicts the base branch's behaviour.
- Add FindBAQuicLib.cmake to resolve ba-quic-lib::ba-quic-lib (in-scope target, system package, or FetchContent fallback), mirroring teleop-module's own migration - Reimplement QuicOperatorServer on top of the transport-only bringauto::quic::QuicServer instead of hand-rolled msquic calls, keeping the public API unchanged so external_server_api.cpp needs no changes - Rely on ba-quic-lib's maxConnections=1 rejection at the transport layer to enforce the single-operator constraint, dropping this class's own compare-and-swap logic - Use this module's own OperatorMessage proto/package (not teleop-module's) since both shared libraries get dlopened into the same external-server-cpp process and protobuf's descriptor registry is process-global
vbartak
left a comment
vbartak
left a comment
There was a problem hiding this comment.
Round 3 — 9951ab1
Two of the three fixed properly:
- ✅
stop()idempotence —std::atomic<bool> running_{true}+exchange(false), matches the pre-migration guard. - ✅ hpp:33 comment — now "taking over from the existing Fleet HTTP API path when
quic_portis configured". Accurate.
Two still open (mutable GIT_TAG v0.1.2, and the Dependencies.cmake / FindBAQuicLib.cmake disagreement about msquic's IMPORTED_LOCATION — the edit at line 52 now explicitly names a "last resort ... plain FetchContent subdir target" case, which is precisely the case with no IMPORTED_LOCATION, so that one got sharper rather than resolved. Still no unzip -l | grep msquic from a BRINGAUTO_PACKAGE=ON build).
And the token fix moved the problem rather than removing it. Details inline — I tested it, and it's worse than I first thought, so I'd like to push back on this one properly.
Separately: the packaged build will fail with no token at all
Not anchorable in this repo, but it blocks release. packager-fleet-protocol-context/app/transparent-module/transparent-module_release.json has:
"Env": {},
"DockerMatrix": { "ImageNames": ["fleet-os-3"] }packager exports only what's in that Env map into the build shell (internal/ssh/ShellEvaluator.go:getEnvStr()), so BA_GITLAB_TOKEN_URI never reaches the container. And grep BA_GITLAB_TOKEN_URI packager-fleet-protocol-context/ returns nothing — every URI in the fleet-protocol chain is public GitHub today. This PR introduces the first private-GitLab dependency into that chain.
Result once this lands: empty _token → guard false → anonymous clone of a private repo → configure fails in the container.
Needs a coordinated change in the packager context ("Env": { "BA_GITLAB_TOKEN_URI": "..." } in both the debug and release JSON), plus whoever runs packager exporting BA_GITLAB_TOKEN. Worth calling out in the PR description so it isn't discovered at release time.
This is also the strongest argument for publishing ba-quic-lib to package-tracker instead: BA_PACKAGE_LIBRARY(msquic v2.5.6) — the line this PR deletes — is exactly why the fleet-protocol build has needed no credentials until now.
| # generated scripts, CMakeCache.txt/the FetchContent property store, and the clone's own | ||
| # .git/config as the origin remote, any of which can leak it via CI artifact archiving, a | ||
| # clone-failure log, or a Docker layer. | ||
| execute_process(COMMAND "${GIT_EXECUTABLE}" config --global |
There was a problem hiding this comment.
🟠 Mid — this writes a credential outside the build directory, and the first token written wins permanently
The embedded-URL leak from 09cfdf5 is genuinely fixed — GIT_REPOSITORY is clean, _token is unset, set() is quoted. But --global moves the credential into ~/.gitconfig, and the token ends up in the config key, not the value:
[url "https://oauth2:TOKEN@gitlab.bringauto.com/"] # <- token is part of the key
insteadOf = https://gitlab.bringauto.com/That has a consequence I don't think was intended. I tested it:
- Same token, configure 3× → same key → overwritten, one entry. Fine.
- Rotated token → different key → git appends a second section and never removes the first.
- Git resolves using the first match:
$ git ls-remote --get-url origin
https://oauth2:TOKEN_A@gitlab.bringauto.com/... # TOKEN_A expired; TOKEN_B present and ignored
So the first token ever written on a machine is the one used from then on. When it expires you get a 401 while echo $BA_GITLAB_TOKEN_URI shows the correct new value, and none of the usual remedies help — rm -rf _build, git clean -xfd, a fresh clone, updating the env var. The bad state is in $HOME, and nothing in the repo points there. Only a manual git config --global --remove-section fixes it.
Secondary: the rewrite is global, so every later https://gitlab.bringauto.com/ clone by that user silently uses this token instead of their own credential helper or SSH setup.
Suggested fix — pass the token through instead of copying it
Git ≥2.31 reads config from the environment. The clone subprocess inherits it, nothing touches disk, and it's rebuilt from the current env on every configure, so rotation just works:
set(_token "$ENV{BA_GITLAB_TOKEN_URI}")
if(_token)
# Credential stays in this process's environment; the FetchContent clone subprocess
# inherits it. Nothing on disk, so a rotated token takes effect immediately and there
# is nothing to clean up.
set(ENV{GIT_CONFIG_COUNT} 1)
set(ENV{GIT_CONFIG_KEY_0} "url.https://${_token}gitlab.bringauto.com/.insteadOf")
set(ENV{GIT_CONFIG_VALUE_0} "https://gitlab.bringauto.com/")
endif()
unset(_token)and after FetchContent_MakeAvailable(ba-quic-lib) on line 64 (not after Declare — the clone happens at MakeAvailable):
unset(ENV{GIT_CONFIG_COUNT})
unset(ENV{GIT_CONFIG_KEY_0})
unset(ENV{GIT_CONFIG_VALUE_0})find_package(Git QUIET) on line 28 can go too — GIT_EXECUTABLE was only needed for the execute_process.
Roughly 10 lines in this one file, no API change, and if the env var is unset the behaviour is identical to today.
Two caveats worth checking:
- It doesn't self-heal. I tested this too — a stale
~/.gitconfigentry still beats the env-var config, so anyone who has already built this branch with a token exported keeps using their old one. Worth a line in the PR description telling them to rungit config --global --get-regexp 'url\..*gitlab\.bringauto\.com.*insteadOf'and remove the section. - Needs git ≥ 2.31 for
GIT_CONFIG_COUNT. Worth confirmingfleet-os-3has it — older git ignores those variables silently, which would surface as a 401 rather than a clear error.
Happy to be argued out of this if the intended usage is containers-only, where --global is harmless because the filesystem is discarded per build. But the guard fires on developer machines too, and there it's a trap that a normal clean-and-rebuild can't escape.
There was a problem hiding this comment.
Fixed — switched to your proposed approach: GIT_CONFIG_COUNT/GIT_CONFIG_KEY_0/GIT_CONFIG_VALUE_0 env vars set right before FetchContent_MakeAvailable and unset right after, so the token lives only in this process's environment and never touches ~/.gitconfig. Dropped the now-unused find_package(Git QUIET) too. Good catch on the stale-first-token behavior — confirmed that would've been a real trap. 41adad8
Settling the open findingsTrimming my own list so it's clear what actually needs doing before merge. Dropped — Not a code change — the msquic packaging question. I'd still like it settled, but it's one command, not a diff: unzip -l <transparent-module>.zip | grep -i msquicIf Note you can't run that check yet: with Suggested order
Nothing else from my earlier rounds is outstanding — |
- Resolves ba-quic-lib::ba-quic-lib via in-scope target, system config package, or FetchContent source build (v0.1.2), mirroring the existing Find*.cmake pattern - Injects BA_GITLAB_TOKEN_URI via per-process GIT_CONFIG_* env vars instead of rewriting ~/.gitconfig, avoiding stale-token conflicts on rotation - Shadows BRINGAUTO_TESTS/BRINGAUTO_INSTALL with normal variables during the FetchContent add_subdirectory so this repo's cached flags don't leak into ba-quic-lib's own build/export, which would otherwise fail at generate time
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
include/bringauto/transparent_module_utils/operator_stream/QuicOperatorServer.hpp (1)
5-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
<vector>for thestd::vectorinonBytesReceived.Line 103 declares
void onBytesReceived(ConnectionId id, std::vector<std::uint8_t> bytes);. This header does not include<vector>, so it compiles only because<bringauto/quic/QuicServer.hpp>happens to pull it in. Include what this header uses.♻️ Proposed fix
`#include` <span> `#include` <string> `#include` <string_view> +#include <vector>🤖 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 `@include/bringauto/transparent_module_utils/operator_stream/QuicOperatorServer.hpp` around lines 5 - 14, Add the missing <vector> standard-library include to QuicOperatorServer.hpp, which declares onBytesReceived with std::vector<std::uint8_t>; keep the existing includes and declarations unchanged.cmake/FindBAQuicLib.cmake (2)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
include_guard(GLOBAL)in a find module can break a secondfind_package(BAQuicLib).CMake re-includes a find module for every
find_packagecall, and each call needsBAQuicLib_FOUNDset in its own scope. Withinclude_guard(GLOBAL), a secondfind_package(BAQuicLib REQUIRED)from another directory scope skips the file body, soBAQuicLib_FOUNDstays unset and theREQUIREDcall fails. Today there is only one call site (CMakeLists.txtline 80), so this is latent. The earlyif(TARGET ba-quic-lib::ba-quic-lib)block already provides the idempotence that the guard was added for, so the guard can be removed.🤖 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 `@cmake/FindBAQuicLib.cmake` around lines 1 - 12, Remove include_guard(GLOBAL) from the FindBAQuicLib module, while retaining the existing ba-quic-lib::ba-quic-lib target check to provide idempotence. Ensure each find_package(BAQuicLib) invocation re-executes the module and sets BAQuicLib_FOUND in its own scope.
28-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
GIT_CONFIG_*block overwrites a caller's existing environment git config.
set(ENV{GIT_CONFIG_COUNT} 1)replaces any inheritedGIT_CONFIG_COUNT, and lines 64-66 then delete it. If a CI runner already exportsGIT_CONFIG_COUNTwith its own key/value pairs (for example a mirror rewrite or a different credential), those entries stop applying for the FetchContent clone and stay removed for the rest of the configure step. Read the inherited count first and append at that index instead of hardcoding0/1.♻️ Append instead of overwrite
set(_token "$ENV{BA_GITLAB_TOKEN_URI}") if(_token) # Credential stays in this process's environment (inherited by the FetchContent clone # subprocess) rather than being copied to disk -- a `git config --global` rewrite would # otherwise persist the token in ~/.gitconfig keyed by its own value, so a rotated token # appends a second section instead of replacing the first, and git keeps resolving to # whichever token was written there first. Requires git >= 2.31. - set(ENV{GIT_CONFIG_COUNT} 1) - set(ENV{GIT_CONFIG_KEY_0} "url.https://${_token}gitlab.bringauto.com/.insteadOf") - set(ENV{GIT_CONFIG_VALUE_0} "https://gitlab.bringauto.com/") + set(_ba_quic_lib_git_config_index "$ENV{GIT_CONFIG_COUNT}") + if(NOT _ba_quic_lib_git_config_index MATCHES "^[0-9]+$") + set(_ba_quic_lib_git_config_index 0) + endif() + math(EXPR _ba_quic_lib_git_config_count "${_ba_quic_lib_git_config_index} + 1") + set(ENV{GIT_CONFIG_KEY_${_ba_quic_lib_git_config_index}} "url.https://${_token}gitlab.bringauto.com/.insteadOf") + set(ENV{GIT_CONFIG_VALUE_${_ba_quic_lib_git_config_index}} "https://gitlab.bringauto.com/") + set(ENV{GIT_CONFIG_COUNT} "${_ba_quic_lib_git_config_count}") endif()Then restore the previous count after
FetchContent_MakeAvailableinstead of unsetting it unconditionally.Also applies to: 64-66
🤖 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 `@cmake/FindBAQuicLib.cmake` around lines 28 - 39, Update the GIT_CONFIG_* handling around the token setup and FetchContent_MakeAvailable to preserve inherited git configuration: capture the existing GIT_CONFIG_COUNT, append the BA GitLab rewrite at that index, and increment the count rather than overwriting entries. After FetchContent_MakeAvailable, restore the original count and related environment state instead of unsetting it unconditionally.cmake/Dependencies.cmake (1)
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the tier caveat in this comment.
The comment states that the walk reaches "msquic's own IMPORTED_LOCATION". That holds only when ba-quic-lib's
FindBAMsquic.cmakeresolves msquic through its imported prebuilt/system tier, which is whatcmake/FindBAQuicLib.cmakelines 51-53 now say explicitly. In the last-resort FetchContent tier msquic is a regular in-tree target with noIMPORTED_LOCATION, soBA_PACKAGE_DEPS_IMPORTEDfinds nothing to install and the package ships withoutlibmsquic.so. That failure appears only whenexternal-server-cppdlopens the plugin. Add one line naming that condition so the next reader does not assume packaging is tier-independent.🤖 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 `@cmake/Dependencies.cmake` around lines 14 - 19, Update the comment above BA_PACKAGE_DEPS_IMPORTED to state that the msquic IMPORTED_LOCATION traversal applies only when FindBAQuicLib.cmake resolves msquic through the imported prebuilt/system tier; note that the FetchContent fallback creates a regular in-tree target with no IMPORTED_LOCATION and is not captured for packaging.
🤖 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
`@include/bringauto/transparent_module_utils/operator_stream/QuicOperatorServer.hpp`:
- Around line 66-76: Update the documentation for
QuicOperatorServer::initialize() to describe its delegation to
bringauto::quic::QuicServer::initialize() rather than listing removed msquic
resources. Remove the listener responsibility from this comment, leaving
listener startup documented under QuicOperatorServer::start().
- Around line 108-121: Move quicServer_ to the final member declaration in
QuicOperatorServer so it is destroyed before operatorMutex_,
operatorConnection_, and running_. Update the surrounding lifetime-ordering
comments to match the new declaration order, and ensure ~QuicOperatorServer
still stops and destroys the server safely even when running_ was already
cleared by an explicit stop().
---
Nitpick comments:
In `@cmake/Dependencies.cmake`:
- Around line 14-19: Update the comment above BA_PACKAGE_DEPS_IMPORTED to state
that the msquic IMPORTED_LOCATION traversal applies only when
FindBAQuicLib.cmake resolves msquic through the imported prebuilt/system tier;
note that the FetchContent fallback creates a regular in-tree target with no
IMPORTED_LOCATION and is not captured for packaging.
In `@cmake/FindBAQuicLib.cmake`:
- Around line 1-12: Remove include_guard(GLOBAL) from the FindBAQuicLib module,
while retaining the existing ba-quic-lib::ba-quic-lib target check to provide
idempotence. Ensure each find_package(BAQuicLib) invocation re-executes the
module and sets BAQuicLib_FOUND in its own scope.
- Around line 28-39: Update the GIT_CONFIG_* handling around the token setup and
FetchContent_MakeAvailable to preserve inherited git configuration: capture the
existing GIT_CONFIG_COUNT, append the BA GitLab rewrite at that index, and
increment the count rather than overwriting entries. After
FetchContent_MakeAvailable, restore the original count and related environment
state instead of unsetting it unconditionally.
In
`@include/bringauto/transparent_module_utils/operator_stream/QuicOperatorServer.hpp`:
- Around line 5-14: Add the missing <vector> standard-library include to
QuicOperatorServer.hpp, which declares onBytesReceived with
std::vector<std::uint8_t>; keep the existing includes and declarations
unchanged.
🪄 Autofix
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 Plus
Run ID: c9931ff3-427d-4600-86dc-bd6d5b6807f6
📒 Files selected for processing (5)
CMakeLists.txtcmake/Dependencies.cmakecmake/FindBAQuicLib.cmakeinclude/bringauto/transparent_module_utils/operator_stream/QuicOperatorServer.hppsource/transparent_operator_stream/QuicOperatorServer.cpp
| /// Outcome of initialize()/start(). | ||
| enum class InitResult { Ok, Failed }; | ||
|
|
||
| /// Open msquic, registration, configuration, credential (mTLS), and the listener. | ||
| [[nodiscard]] InitResult initialize(); | ||
|
|
||
| /// Start the listener. | ||
| [[nodiscard]] InitResult start(); | ||
|
|
||
| /// Stop the listener + drop the operator connection. | ||
| void stop(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the initialize() doc comment; it still describes the removed msquic code.
initialize() now forwards to bringauto::quic::QuicServer::initialize(). The comment names msquic objects that this class no longer creates, and it lists the listener, which start() handles.
📝 Proposed doc fix
- /// Open msquic, registration, configuration, credential (mTLS), and the listener.
+ /// Initialize the underlying ba-quic-lib transport (registration, TLS credential, listener
+ /// setup). Does not begin accepting connections; see start().
[[nodiscard]] InitResult initialize();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Outcome of initialize()/start(). | |
| enum class InitResult { Ok, Failed }; | |
| /// Open msquic, registration, configuration, credential (mTLS), and the listener. | |
| [[nodiscard]] InitResult initialize(); | |
| /// Start the listener. | |
| [[nodiscard]] InitResult start(); | |
| /// Stop the listener + drop the operator connection. | |
| void stop(); | |
| /// Outcome of initialize()/start(). | |
| enum class InitResult { Ok, Failed }; | |
| /// Initialize the underlying ba-quic-lib transport (registration, TLS credential, listener | |
| /// setup). Does not begin accepting connections; see start(). | |
| [[nodiscard]] InitResult initialize(); | |
| /// Start the listener. | |
| [[nodiscard]] InitResult start(); | |
| /// Stop the listener + drop the operator connection. | |
| void stop(); |
🤖 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
`@include/bringauto/transparent_module_utils/operator_stream/QuicOperatorServer.hpp`
around lines 66 - 76, Update the documentation for
QuicOperatorServer::initialize() to describe its delegation to
bringauto::quic::QuicServer::initialize() rather than listing removed msquic
resources. Remove the listener responsibility from this comment, leaving
listener startup documented under QuicOperatorServer::start().
| /// The shared, transport-only ba-quic-lib server. Declared after config_/channel_ (its | ||
| /// construction reads config_ via buildEndpointConfig()/buildSettings()) and before | ||
| /// operatorMutex_/operatorConnection_ (its callbacks, still possibly in flight during | ||
| /// destruction, read/write those). | ||
| std::unique_ptr<bringauto::quic::QuicServer> quicServer_; | ||
|
|
||
| /// Guards operatorConnection_. Single operator (Phase 1, same as teleop-module). | ||
| mutable std::mutex operatorMutex_; | ||
| std::optional<ConnectionId> operatorConnection_; | ||
|
|
||
| /// Makes stop() idempotent — an explicit stop() followed by the destructor's stop() call | ||
| /// must not run quicServer_->stop()/channel_.shutdown() twice. | ||
| std::atomic<bool> running_{true}; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the fetched ba-quic-lib sources, then inspect stop()/destructor callback quiescence.
set -uo pipefail
fd -H -t f 'QuicServer\.(cpp|hpp)$' . | while IFS= read -r f; do
echo "===== $f"
rg -n -C 6 '::stop\s*\(|~QuicServer|join\s*\(|callbacks_\.|maxConnections' "$f"
done
# Also surface any documented lifetime contract.
fd -H -t f -e md . | xargs -r rg -n -i -C 3 'stop\(\)|callback|lifetime' -g '*quic*'Repository: bringauto/transparent-module
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -u
echo "Files:"
git ls-files | rg '(^|/)QuicOperatorServer\.(h(pp)?|cxx|cpp)$|(^|/)QuicServer\.(h(pp)?|cxx|cpp)$' || true
echo "Candidate header outline:"
fd -a 'QuicOperatorServer\.(hpp|cpp|cc|cxx)$' . | while IFS= read -r f; do
echo "===== $f"
wc -l "$f"
ast-grep outline "$f" --view compact || true
done
echo "Relevant QuicOperatorServer source:"
fd 'QuicOperatorServer\.(hpp|cpp|cc|cxx)$' . | while IFS= read -r f; do
echo "===== $f"
cat -n "$f"
done
echo "Search QuicServer declarations/usages:"
rg -n "class QuicServer|struct QuicServer|QuicServer|stop\(\)|callbacks" -S include source . 2>/dev/null | head -n 300 || trueRepository: bringauto/transparent-module
Length of output: 21584
🌐 Web query:
ba-quic-lib QuicServer stop destructor no callbacks after stop documentation
💡 Result:
There is no widely recognized or standardized library known specifically as "ba-quic-lib." Because the term is ambiguous, official documentation confirming a "QuicServer stop destructor no callbacks" behavior does not exist for a library by that exact name. It is likely you are encountering issues with a specific QUIC implementation or a similar-sounding library. Based on common behaviors in asynchronous QUIC server implementations: 1. Lifecycle Management: In many network programming libraries (such as those for Rust, C++, or Node.js), a "stop" or "shutdown" method is explicitly designed to signal the server to cease accepting new connections and, typically, to await the completion of in-flight requests before fully closing [1][2][3]. 2. Destructor Behavior: Relying on a destructor to perform complex cleanup (like triggering final callbacks) is often discouraged because destructors in many languages (especially C++) should not throw exceptions or perform blocking operations that could cause undefined behavior or deadlocks during object destruction. 3. Callback Execution: If callbacks are not firing after a stop command, it is often because the event loop or the underlying transport has been terminated prematurely, or because the code responsible for processing those callbacks has been dropped or moved out of scope before the shutdown process finished [2]. If you are using a library like grpc_quic_server (Rust) [1][2], @matrixai/quic (JavaScript) [4], or libquicr (C++) [3], please consult their specific documentation for graceful shutdown procedures. Ensure that you are: - Awaiting the shutdown future or promise [1][4]. - Ensuring that your runtime or event loop remains active until the shutdown sequence is fully complete [2]. - Checking if your callbacks are bound to a context that is being destroyed before the server completes its final tasks. If you can provide the specific GitHub repository, language, or ecosystem for "ba-quic-lib," a more targeted search can be performed.
Citations:
- 1: https://docs.rs/grpc-quic-server/latest/grpc_quic_server/server/struct.QuicServer.html
- 2: https://docs.rs/grpc-quic-server/latest/src/grpc_quic_server/server.rs.html
- 3: https://www.quicr.org/html/classquicr_1_1_server.html
- 4: https://matrixai.github.io/js-quic/classes/QUICServer.html
Move quicServer_ to the end and destroy it before the operator members.
QuicOperatorServer members are destroyed in reverse declaration order, so operatorMutex_ and operatorConnection_ are destroyed before quicServer_. The member-ordering comment is inverted, and non-destructor callbacks can access these members after they are destroyed. Also, running_ can make the destructor call stop() a no-op if stop() was already called explicitly. Declare quicServer_ last so it leaves the class first, or destruct it explicitly from ~QuicOperatorServer before the other members go away.
🤖 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
`@include/bringauto/transparent_module_utils/operator_stream/QuicOperatorServer.hpp`
around lines 108 - 121, Move quicServer_ to the final member declaration in
QuicOperatorServer so it is destroyed before operatorMutex_,
operatorConnection_, and running_. Update the surrounding lifetime-ordering
comments to match the new declaration order, and ensure ~QuicOperatorServer
still stops and destroys the server safely even when running_ was already
cleared by an explicit stop().
2 participants
Summary by CodeRabbit