Chore: one time sync of main -> develop to prepare for ff automation#103
Merged
Conversation
websocketpp constructs tcp::resolver::query(host, port) using the Boost ASIO default flags, which include AI_ADDRCONFIG. That flag tells getaddrinfo(3) to return results only when a local interface for the address family is configured. On certain embedded devices (RDK STBs) the loopback interface is not brought up independently -- it comes up as part of the general networking init. When eth0/wifi are down, lo is also down, so AI_ADDRCONFIG causes getaddrinfo to return HOST_NOT_FOUND even for 127.0.0.1, producing: asio async_resolve error: asio.netdb:1 (Host not found (authoritative)) WebSocket Connection Unknown ... websocketpp.transport:2 Underlying Transport Error Fix: vendor websocketpp/transport/asio/endpoint.hpp (the one file that constructs the resolver query). For hosts that parse as a numeric IP address via asio::ip::address::from_string(), use the numeric_host flag (AI_NUMERICHOST) instead of address_configured (AI_ADDRCONFIG). This bypasses the DNS resolver and the AI_ADDRCONFIG interface-presence check entirely. For genuine hostnames the existing address_configured behaviour is preserved. The vendor/ directory is placed before system websocketpp headers in target_include_directories so the patched file takes priority.
src/vendor/websocketpp is a patched copy of websocketpp 0.8.2 (BSD-3-Clause, Copyright Peter Thorson). It is documented in NOTICE and LICENSE. Exclude from BlackDuck scan to avoid component re-identification.
- gateway.cpp: reset connectResultError to None at the start of each retry attempt so a stale error from attempt N is never reported as the final error of attempt N+1 (Copilot line 537) - gateway.cpp: add connectionListenerMtx to serialize reads/writes of connectionChangeListener between the websocketpp IO thread and the main thread (Copilot line 542) - gateway.cpp: on 10-second connect timeout (connectResultReady not set), call transport.disconnect() to abort the in-flight async attempt before retrying (Copilot line 529 - stale attempt race) - gatewayTest: add GatewayRetryUTest with three tests covering retry behaviour: server-delayed connect, no-retry fast-fail, and disconnect-aborts-retry-delay (Copilot line 529 - missing tests) - transportTest: catch std::exception in the echo server message handler and report via ADD_FAILURE() instead of swallowing silently (Copilot line 740) - build.sh: fix --docker incremental build wipe -- compare CMAKE_HOME_DIRECTORY against /workspace (container path) rather than $SCRIPT_DIR (host path) so Docker-built caches survive subsequent --docker invocations (Copilot build.sh:41)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…r to integration test script Agent-Logs-Url: https://github.com/rdkcentral/firebolt-cpp-transport/sessions/c76ef289-26a8-4e73-b1b2-5961012d6d37 Co-authored-by: brendanobra <740575+brendanobra@users.noreply.github.com>
- gateway.cpp: snapshot connectResultReady/Ok/Error into local variables while connectResultMtx is still held inside the wait_for scope, eliminating the data-race read of shared state outside the lock (Copilot line 557) - gateway.cpp: simplify the post-wait logic — remove double transport.disconnect() call; use the snapshotted attemptError (or NotConnected for timeout) as the loop's status (Copilot 550/553) - config.h.in: update doc comment for reconnect_max_attempts=0 to reflect new behavior — connect() now blocks and returns the error code on failure; 'previous behaviour' description was no longer accurate (Copilot config.h.in:90) - gatewayTest: relax NoRetryFailsFast assertion to EXPECT_NE(None) — connection refused maps to Error::General, not Error::NotConnected - .version: add to git so cmake/version.cmake can read it (Copilot cmake/version.cmake:27)
- gateway.cpp: document callback-thread contract — the final onConnectionChange(true/false) from connect() fires on the caller's thread; subsequent state-change callbacks fire on the websocketpp IO thread; callers must handle both (Copilot line 572) - transportTest: wrap TransportNumericIPUTest and TransportIPv6UTest SetUp() in try/catch(websocketpp::exception) + FAIL() so a port conflict fails the individual test instead of aborting the binary (Copilot line 748) - gatewayTest: wrap GatewayRetryUTest::startServer() in try/catch(websocketpp::exception) + FAIL() — consistent with the pattern in GatewayUTest::startServer() (Copilot line 728)
If transport.connect() returns AlreadyConnected synchronously, the
gateway was falling through to the generic failure path which:
1. overwrote connectionChangeListener with the new (unapplied) callback
2. called onConnectionChange(false, AlreadyConnected) — a spurious
'disconnected' event while the transport is still live
Fix: save the pre-swap listener before installing the condvar wrapper.
On AlreadyConnected, restore it and return early without invoking any
callback.
Also adds GatewayRetryUTest.AlreadyConnectedNoFalseDisconnect to verify
that no false-disconnect callback fires on a redundant connect() call.
Addresses Copilot review comment on src/gateway.cpp:568.
…ICENSE The .bdignore exclusion alone is insufficient for BlackDuck compliance. Appends the full BSD-3 license text (copyright Peter Thorson, 2014) that governs src/vendor/websocketpp/ alongside the project's Apache-2.0 text. The attribution was already present in NOTICE; this adds the required license body text as requested in PR review.
LICENSE: use generic BSD-3 template (Copyright <YEAR> <COPYRIGHT HOLDER>)
- per mhughesacn: specific attribution stays in NOTICE; LICENSE holds
the license text template only
config.h.in: correct reconnect_max_attempts docstring
- connect() can return NotConnected, Timedout, or General, not only
NotConnected
gateway.cpp watchdog: add predicate to wait_for to avoid spurious wakeups
- checkPromises() now only called on genuine timeout (wait_for == false)
- prevents extra/tight-loop calls on spurious wakeup
gatewayTest.cpp: explicitly disconnect before end of two tests
- prevents use-after-free when TearDown fires onClose after locals gone
- RetryConnectsWhenServerDelayed, AlreadyConnectedNoFalseDisconnect
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
gateway.cpp: add missing #include <algorithm> for std::min transportTest.cpp: add missing #include <atomic> (was relying on transitive include via <future>, non-portable) gatewayTest.cpp: same — add #include <atomic> gateway.h: document connect() callback threading: - initial result fires on calling thread - subsequent events (disconnect/watchdog) fire on websocketpp IO thread - callers must be thread-safe Note: LICENSE placeholder (<YEAR> <COPYRIGHT HOLDER>) is intentional per mhughesacn's explicit instruction — specific attribution is in NOTICE.
transportTest.cpp: add m_serverStarted guard to TearDown in both TransportNumericIPUTest and TransportIPv6UTest — mirrors existing pattern in other fixtures; prevents websocketpp crash when SetUp fails before the server is started test-no-network.sh: guard Docker path derivation for out-of-repo binaries — falls back to default build-dev/test/utApp path with a warning instead of silently passing a non-relative REL_BIN
gateway.cpp: fix disconnect-during-connect race — when disconnect() is called while connect() is waiting for the async open/fail callback, disconnectRequested_ wakes the wait but previously broke out of the retry loop leaving status=None, causing a false success callback and spurious watchdog startup. Now calls transport.disconnect() to abort the in-flight socket and forces status=NotConnected before breaking, so the post-loop error path fires correctly. ci.yml: add -w /workspace to docker run for network-isolation tests — the relative path build/test/utApp requires the working directory to be set to the mount point; without -w the binary would not be found.
…ht attempts promptly Transport::disconnect() only called client_->close() when connectionStatus_ was Connected. During DNS/TCP/WebSocket handshake the status stays Disconnected, so stop_perpetual() alone did not cancel the pending async operation and connectionThread_->join() could block for the full OS-level TCP timeout rather than returning promptly. Introduce TransportState::Connecting (set in connect() after client_->connect() is queued, cleared by onOpen/onFail/onClose): * disconnect() detects Connecting and calls client_->stop(), which forces the asio io_service to exit run() immediately so the connection thread can be joined without delay. onFail will not fire after stop(), which is safe because the gateway condvar is already woken by disconnectRequested_. * All other state transitions are unchanged: onOpen -> Connected, onFail/onClose -> Disconnected.
gateway.cpp: clamp reconnect_max_attempts to 100 before 1+N to prevent unsigned overflow when caller passes UINT_MAX or similar large value. gateway.cpp + config.h.in: replace hard-coded 10s kConnectTimeout with cfg.connect_attempt_timeout_ms (default 10000 ms, backward compatible). Update reconnect_max_attempts docstring to describe the per-attempt timeout, the effective maximum blocking duration formula, and the 100- retry cap. Add connect_attempt_timeout_ms field with full docstring. build.sh: replace unsafe '-n "$@"' condition (undefined behaviour with multiple args inside [[ ]]) with '$# -gt 0'.
The condvar wrapper is installed in connectionChangeListener before transport.connect() is called so it is in place when onOpen/onFail fires. If transport.connect() returns AlreadyConnected, the wrapper is restored to previousListener — but in the intervening window an IO-thread event (e.g. the live connection dropping) could hit the wrapper and be silently discarded instead of reaching the previously-registered callback. Fix: capture previousListener and a shared skipPrevForward flag inside the wrapper. Events arriving before skipPrevForward is set (i.e. during the AlreadyConnected window) are forwarded to previousListener so they are not dropped. skipPrevForward is set to true immediately after transport.connect() returns Firebolt::Error::None (a new attempt was actually queued), disabling the forward path so genuine new-attempt events are not spuriously delivered to the old callback.
connectionStatus_ is already std::atomic<TransportState> but all reads and writes used implicit conversion operators, obscuring the atomic semantics and leaving memory ordering unspecified. Replace with explicit .load()/.store() (default seq_cst) throughout connect(), disconnect(), send(), start(), and the three websocketpp callbacks.
The skipPrevForward flag was set after transport.connect() returned, but onOpen/onFail can fire on the IO thread between client_->connect() being queued inside transport.connect() and skipPrevForward->store(true) executing in gateway.cpp. In that window the wrapper incorrectly forwards the new-attempt callback to previousListener (the old session's callback), triggering a spurious user-visible event. Remove the forwarding mechanism entirely. The wrapper now only signals the condvar and never touches previousListener. For the AlreadyConnected window the theoretical one-event loss is negligible (synchronous return, listener restored before connect() returns) and correct behaviour is preferable to the data race.
fix(RDKEMW-16441): gateway connect fails on numeric loopback IP when no external network
chore: auto-sync develop → main (merge conflict)
NOJIRA: process overhead, merge develop -> main (clean integration)
Contributor
There was a problem hiding this comment.
Pull request overview
Syncs main into develop while also incorporating transport/gateway hardening and test infrastructure updates (notably a patched websocketpp resolver path for numeric loopback IPs and improved header/connection handling).
Changes:
- Vendor-patches websocketpp ASIO transport to treat numeric IP hosts as
numeric_host(avoidsAI_ADDRCONFIGloopback resolution failures in no-network scenarios). - Extends
Transport/Gatewaybehavior around connection lifecycle (newConnectingstate, response-header capture/clearing, safer callback handling, logging improvements). - Updates/unit adds tests and tooling scripts (header injection test server, reserved port helper, dockerized build wrapper, no-network integration runner, fmt robustness).
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/transportTest.cpp | Adds header-injection + response-header retrieval test using an ephemeral local websocket server; hardens promise signaling. |
| test/unit/gatewayTest.cpp | Makes “no server listening” tests more robust by reserving an ephemeral loopback port and tightening async failure assertions. |
| test/integration/test-no-network.sh | Adds a network-isolation runner (Docker --network none preferred, unshare --net fallback) for targeted gtest filters. |
| src/vendor/websocketpp/transport/asio/endpoint.hpp | Vendored websocketpp endpoint with numeric-IP resolver flag selection (and local endpoint accessor). |
| src/transport.h | Adds <map> include for header support. |
| src/transport.cpp | Introduces Connecting state, wraps header injection, ensures disconnect can force-stop in-flight connects, clears response headers on close/fail/disconnect, and improves logging. |
| src/logger.cpp | Makes log-level maps const, avoids operator[] on maps, and normalizes tolower casting behavior. |
| src/gateway.cpp | Protects connectionChangeListener with a mutex, restores previous listener on immediate connect failure, improves response-id logging and legacy v1 id parsing. |
| src/CMakeLists.txt | Adjusts include ordering so vendored websocketpp headers can take precedence over system headers. |
| README.md | Documents that response-header name lookup is case-sensitive. |
| presentations/transport-slides/slides.md | Removes duplicated/extra slide content. |
| presentations/openspec-with-copilot-experience/slides.md | Fixes “copliot” spelling in command examples. |
| presentations/openspec-with-copilot-experience/package.json | Fixes package name typo (“copliot” → “copilot”). |
| LICENSE | Removes stray MIT template section. |
| include/firebolt/gateway.h | Adds a thread-affinity note for onConnectionChange. |
| include/firebolt/config.h.in | Adds documented reconnect/timeout policy fields to Config. |
| fmt.sh | Makes clang-format invocation robust for filenames with spaces via -print0/xargs -0. |
| build.sh | Adds --docker wrapper mode and improves stale build-dir detection; relaxes SYSROOT_PATH requirement. |
| .version | Adds version file (1.1.8). |
| .github/skills/openspec-coverage/openspec_coverage.py | Skips conversion operators during method extraction. |
| .bdignore | Excludes src/vendor/ from BlackDuck scan inputs. |
swethasukumarr
approved these changes
Jul 15, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
No description provided.