From 7e2ad49a68550ad8806a3d82145f4fc29a196307 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 13:58:33 +0200 Subject: [PATCH 01/16] docs: add foxglove-parity feature design spec Co-Authored-By: Claude Fable 5 --- .../2026-07-06-foxglove-parity-design.md | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-06-foxglove-parity-design.md diff --git a/docs/superpowers/specs/2026-07-06-foxglove-parity-design.md b/docs/superpowers/specs/2026-07-06-foxglove-parity-design.md new file mode 100644 index 0000000..4a15838 --- /dev/null +++ b/docs/superpowers/specs/2026-07-06-foxglove-parity-design.md @@ -0,0 +1,123 @@ +# Foxglove-parity features for pj_bridge — Design + +**Date:** 2026-07-06 +**Status:** Approved +**Scope:** Six features closing the gap with foxglove_bridge on the topic-subscription +feature set, identified in a side-by-side comparison of the two bridges. + +## Background + +pj_bridge leads foxglove_bridge on bandwidth efficiency (ZSTD-compressed aggregated +frames, per-topic rate limiting, subscription-group fan-out) but lacks several +robustness/deployment features foxglove_bridge has. This design adds them. Where logic +is adapted from foxglove_bridge (MIT license), the affected file carries a copyright +attribution notice; MIT is compatible with this repo's AGPL-3.0. + +All protocol changes are **additive**: `protocol_version` stays `1`. Old clients are +unaffected (no new unsolicited traffic unless a client opts in). + +## Feature 1 — Topic whitelist regex + +- New `WhitelistFilter` class in `app/` holding a list of compiled `std::regex` + (ECMAScript grammar, `std::regex_match` = full match, mirroring foxglove's + `isWhitelisted`). +- `BridgeServer` receives the filter via its config and applies it in two places: + 1. `get_topics` responses — non-matching topics are omitted. + 2. `subscribe` requests — non-whitelisted topics fail with reason + `"Topic not whitelisted"` (per-topic failure, consistent with the existing + partial_success model). +- Config: ROS2 string-array param `topic_whitelist` (default `[".*"]`); + FastDDS/RTI CLI flag `--topic-whitelist` (repeatable, default `.*`). +- Invalid regex at startup = fatal error with a clear message. + +## Feature 2 — QoS depth heuristics (ROS2 only) + +- Extend `adapt_qos()` in `GenericSubscriptionManager`: + `depth = clamp(sum(publisher depths), min_qos_depth, max_qos_depth)`. + Publishers reporting depth 0 (unknown/KEEP_ALL) contribute 0, as in foxglove. +- Defaults: `min_qos_depth = 1`, `max_qos_depth = 100` (preserves the current + effective ceiling of `rclcpp::QoS(100)`). +- Reliability and durability heuristics unchanged. +- Logic adapted from foxglove_bridge `determineQoS()` — attribution comment in file. +- Config: ROS2 params `min_qos_depth`, `max_qos_depth`. + +## Feature 3 — Pushed topic advertisement (opt-in per client) + +- New commands: + - `{"command": "subscribe_topic_updates"}` → `{"status": "ok", ...}`; sets a + per-session flag. + - `{"command": "unsubscribe_topic_updates"}` → clears it. Both idempotent. +- New server-initiated notification, sent **only** to opted-in clients whenever the + topic set changes: + + ```json + {"notification": "topics_changed", + "added": [{"name": "/t", "type": "pkg/msg/T"}], + "removed": ["/gone"], + "protocol_version": 1} + ``` + + `added`/`removed` may be empty arrays; a notification is sent only when at least + one of them is non-empty. The whitelist (Feature 1) is applied before diffing. +- Detection is backend-agnostic: `BridgeServer::check_topic_changes()` diffs + `TopicSourceInterface::get_topics()` against the previous snapshot. Entry points + drive it on a timer: new param/flag `topic_poll_interval` (seconds, default `1.0`; + `0` disables polling and the feature). + +## Feature 4 — Slow-client backpressure (foxglove-style) + +Policy per user decision: bounded per-client queue, **drop-oldest** on overflow, +throttled warnings, never disconnect for data-plane pressure. + +- Implemented entirely in `WebSocketMiddleware`; interfaces unchanged. +- Per-client bounded `std::deque` of pending **binary** frames in front of the socket: + - `send_binary()`: if the client's `ix::WebSocket::bufferedAmount()` is below a + 1 MiB high-watermark (`kSocketBufferHighWatermark`), flush the pending queue in + order, then send the new frame; otherwise enqueue the frame. + - On enqueue past `client_backlog_size` (default 100 frames ≈ 2 s at 50 Hz), drop + the **oldest** queued frame and increment a per-client dropped-frame counter. + - Warning log throttled to one per 30 s per client (foxglove's cadence). +- JSON replies (`send_reply`) are control-plane: direct send, never queued or dropped. +- Config: ROS2 param / CLI flag `client_backlog_size`. +- Dropped-frame totals exposed via the existing stats path. + +## Feature 5 — transient_local replay (ROS2, latest message) + +- `adapt_qos()` already detects the all-publishers-transient_local case; the + subscription manager records it and exposes + `bool is_transient_local(const std::string& topic) const` on + `SubscriptionManagerInterface`. FastDDS/RTI implementations return `false` (defer). +- `MessageBuffer` gains a per-topic *latched* mode (`set_latched(topic, bool)`): + the most recent message for a latched topic is exempt from TTL eviction — a + one-slot durable cache. It is cleared when the topic's subscription is destroyed. +- On a client `subscribe` to a latched topic, the retained message is force-included + in that client's next binary frame, bypassing the per-topic rate-limit timestamp + check exactly once (so `/tf_static`-style topics appear immediately even though + their message timestamp is old). + +## Feature 6 — TLS (OpenSSL, server cert only) + +- CMake: new option `PJ_BRIDGE_TLS` (default `ON`). The IXWebSocket FetchContent path + sets `USE_TLS=ON` and `USE_OPEN_SSL=ON`. If a system IXWebSocket without TLS is + found and `tls` is requested at runtime, startup fails with a clear error. +- `WebSocketMiddleware` constructor takes `std::optional{certfile, keyfile}` + → mapped to `ix::SocketTLSOptions`. `MiddlewareInterface` unchanged. +- Config: ROS2 params `tls` (bool, default false), `certfile`, `keyfile`; + FastDDS/RTI CLI `--certfile`/`--keyfile` (presence of both implies TLS). +- Missing or unreadable cert/key files fail startup with a clear error. +- Tests generate a self-signed cert via the `openssl` CLI in test setup. + +## Cross-cutting + +- `docs/API.md`: document the new commands, notification, and params. +- README/CLAUDE.md configuration tables updated. +- Every feature lands with unit tests in the existing gtest suites' style; the full + suite must stay TSAN/ASAN clean. +- One commit per feature on branch `feature/foxglove-parity`; single PR. + +## Non-goals + +- FastDDS/RTI transient_local replay (interface stub only). +- mTLS / client certificates. +- Per-publisher history-depth replay caches (latest-message only). +- Any change to the binary frame format or protocol version. From 0d0384e1187edf73a02a7e2814122a378bb889f4 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 14:03:27 +0200 Subject: [PATCH 02/16] docs: add foxglove-parity implementation plan Co-Authored-By: Claude Fable 5 --- .../plans/2026-07-06-foxglove-parity.md | 215 ++++++++++++++++++ 1 file changed, 215 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-06-foxglove-parity.md diff --git a/docs/superpowers/plans/2026-07-06-foxglove-parity.md b/docs/superpowers/plans/2026-07-06-foxglove-parity.md new file mode 100644 index 0000000..2ab1bc9 --- /dev/null +++ b/docs/superpowers/plans/2026-07-06-foxglove-parity.md @@ -0,0 +1,215 @@ +# Foxglove-parity Features Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Implement the six features in `docs/superpowers/specs/2026-07-06-foxglove-parity-design.md`: topic whitelist, QoS depth heuristics, pushed topic advertisement, slow-client backpressure, transient_local replay, TLS. + +**Architecture:** Backend-agnostic changes go in `app/` (core, no ROS deps); ROS2-specific changes in `ros2/`. All protocol changes are additive (protocol_version stays 1). One commit per task. + +**Tech Stack:** C++17, gtest, nlohmann/json, IXWebSocket 11.4.6, ZSTD, spdlog, rclcpp (Humble). + +**Build/test (from the worktree root `.worktrees/foxglove-parity`):** +```bash +pixi run -e humble build # colcon build (Release) +pixi run -e humble test # colcon test + result; baseline = 177 tests green +``` +Format before every commit: `pre-commit run -a` (clang-format). Follow `.clang-tidy` naming: classes `CamelCase`, methods `lower_case`, members `trailing_underscore_`, constants `kCamelCase`. Core code logs via `spdlog::*`, never `RCLCPP_*` (except `ros2/` adapter code). New files carry the AGPL header found at the top of every existing file (copy from `app/src/bridge_server.cpp:1-18`). + +**Key existing files (read before coding):** +- `app/include/pj_bridge/bridge_server.hpp` + `app/src/bridge_server.cpp` — orchestrator; request dispatch at `bridge_server.cpp:182-196`; subscribe validation loop at `:299-327`; publish/fan-out loop at `:677-827`; lock ordering documented at `bridge_server.hpp:163-166` (`cleanup_mutex_ > last_sent_mutex_ > stats_mutex_`). +- `app/include/pj_bridge/session_manager.hpp` — per-session state (pause flag, ref-held topics) — mimic its patterns for new per-session flags. +- `app/include/pj_bridge/message_buffer.hpp` + `.cpp` — TTL buffer; injectable `ClockFn` for tests. +- `app/include/pj_bridge/middleware/websocket_middleware.hpp` + `app/src/middleware/websocket_middleware.cpp` — IXWebSocket server; per-client `ix::WebSocket` map `clients_`. +- `ros2/src/generic_subscription_manager.cpp:28-61` — `adapt_qos()`. +- `ros2/src/main.cpp:39-47` — ROS2 param declarations; `fastdds/src/main.cpp` — CLI11 flags; `app/src/standalone_event_loop.cpp` — FastDDS/RTI event loop. +- `tests/unit/test_bridge_server.cpp` — has mock `TopicSourceInterface` / `SubscriptionManagerInterface` / `MiddlewareInterface` implementations; extend those mocks, don't invent new ones. + +--- + +### Task 1: Topic whitelist regex + +**Files:** +- Create: `app/include/pj_bridge/whitelist_filter.hpp`, `app/src/whitelist_filter.cpp` +- Create: `tests/unit/test_whitelist_filter.cpp` +- Modify: `app/include/pj_bridge/bridge_server.hpp` (ctor param + member), `app/src/bridge_server.cpp` (`handle_get_topics`, `handle_subscribe`) +- Modify: `ros2/src/main.cpp`, `fastdds/src/main.cpp`, `rti/src/main.cpp` (params/flags) +- Modify: `CMakeLists.txt` (add new .cpp to `pj_bridge_app` and test target), `docs/API.md` +- Test: `tests/unit/test_whitelist_filter.cpp`, extend `tests/unit/test_bridge_server.cpp` + +- [ ] **Step 1: Write failing tests for `WhitelistFilter`** — construction from patterns, ECMAScript `std::regex_match` (full match: pattern `/cam` must NOT match `/camera`), default-constructed filter matches everything, `create()` returns `tl::unexpected` with the offending pattern for invalid regex (e.g. `"("`). + +Class contract: +```cpp +class WhitelistFilter { + public: + WhitelistFilter() = default; // matches everything + static tl::expected create(const std::vector& patterns); + bool matches(const std::string& topic_name) const; + private: + std::vector patterns_; // empty = match all +}; +``` + +- [ ] **Step 2: Run the new test, verify it fails to compile/link.** +- [ ] **Step 3: Implement `WhitelistFilter`** (compile with `std::regex::ECMAScript`; `matches` = any pattern full-matches; add file to `pj_bridge_app` sources in CMakeLists.txt). +- [ ] **Step 4: Run tests, verify green.** +- [ ] **Step 5: Write failing BridgeServer tests**: (a) `get_topics` response omits non-whitelisted topics; (b) `subscribe` to a non-whitelisted topic yields a per-topic failure with `"reason": "Topic not whitelisted"` (and `ALL_SUBSCRIPTIONS_FAILED` when it was the only topic); (c) whitelisted topics still subscribe fine. +- [ ] **Step 6: Integrate**: add `WhitelistFilter whitelist = {}` as the **last** BridgeServer ctor param (existing call sites unaffected). In `handle_get_topics` skip non-matching topics; in `handle_subscribe` add the whitelist check at the top of the per-topic validation loop (`bridge_server.cpp:299`), before the existence check. +- [ ] **Step 7: Run tests, verify green.** +- [ ] **Step 8: Wire config**: ROS2 string-array param `topic_whitelist` default `[".*"]` (declare next to `ros2/src/main.cpp:42`); CLI11 repeatable `--topic-whitelist` default `{".*"}` in `fastdds/src/main.cpp` and `rti/src/main.cpp`. On `WhitelistFilter::create()` error: log the message and exit 1. Document the param and the full-match semantics in `docs/API.md` (get_topics + subscribe sections). +- [ ] **Step 9: Full build + test + `pre-commit run -a`.** +- [ ] **Step 10: Commit** `feat: add topic whitelist regex filtering`. + +### Task 2: QoS depth heuristics (ROS2) + +**Files:** +- Modify: `ros2/include/pj_bridge_ros2/generic_subscription_manager.hpp`, `ros2/src/generic_subscription_manager.cpp` (`adapt_qos`), `ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp` + `.cpp` (pass-through), `ros2/src/main.cpp` +- Test: extend `tests/unit/test_generic_subscription_manager.cpp` + +- [ ] **Step 1: Write failing tests**: publisher with depth 10 → subscription depth ≥ 10; two publishers depth 60 each → depth clamped to `max_qos_depth` (100); no publishers → depth = `max_qos_depth`? No — **no publishers → keep depth = max(min_qos_depth, 100 default path unchanged)**; precise rule below. Test via `node->create_publisher` with explicit `rclcpp::QoS(depth)` and inspecting the returned `rclcpp::QoS` from `adapt_qos` (make `adapt_qos` public or keep tests as friend — it is already `const` public in the header; check). + +Depth rule (adapted from foxglove_bridge `determineQoS()` — add attribution comment in the file: *"Depth aggregation adapted from foxglove_bridge (MIT License, Copyright (c) Foxglove Technologies Inc)"*): +``` +total = sum over publishers of profile.depth() // depth 0 (unknown/KEEP_ALL) contributes 0 +depth = clamp(total, min_qos_depth, max_qos_depth) +if publishers.empty(): depth = min(100, max_qos_depth) // preserve current default behavior +``` +- [ ] **Step 2: Run tests, verify failure.** +- [ ] **Step 3: Implement**: `GenericSubscriptionManager` ctor gains `size_t min_qos_depth = 1, size_t max_qos_depth = 100`; `Ros2SubscriptionManager` forwards them; reliability/durability logic at `generic_subscription_manager.cpp:39-58` unchanged. +- [ ] **Step 4: Run tests, verify green.** +- [ ] **Step 5: Wire params** `min_qos_depth` (int, 1) and `max_qos_depth` (int, 100) in `ros2/src/main.cpp`; document in `docs/API.md` config section and README parameter table. +- [ ] **Step 6: Full build + test + format. Commit** `feat: aggregate publisher QoS depths with min/max clamping`. + +### Task 3: Pushed topic advertisement (opt-in) + +**Files:** +- Modify: `app/include/pj_bridge/session_manager.hpp` + `app/src/session_manager.cpp` (per-session `wants_topic_updates` flag, default false, following the existing `paused` flag pattern) +- Modify: `app/include/pj_bridge/bridge_server.hpp` + `app/src/bridge_server.cpp` +- Modify: `ros2/src/main.cpp`, `app/include/pj_bridge/standalone_event_loop.hpp` + `app/src/standalone_event_loop.cpp`, `fastdds/src/main.cpp`, `rti/src/main.cpp` +- Modify: `docs/API.md` +- Test: extend `tests/unit/test_bridge_server.cpp`, `tests/unit/test_session_manager.cpp` + +- [ ] **Step 1: Write failing SessionManager tests** for `set_topic_updates(client_id, bool)` / `wants_topic_updates(client_id)` (default false; false for unknown client). +- [ ] **Step 2: Implement the flag. Tests green.** +- [ ] **Step 3: Write failing BridgeServer tests**: + - `{"command": "subscribe_topic_updates"}` → `{"status": "ok", "topic_updates": true, ...}`; `unsubscribe_topic_updates` → `topic_updates: false`. Both idempotent, both create a session if needed (copy the pattern from `handle_pause`). + - New public method `void check_topic_changes()`: first call snapshots silently (no notification); after the mock topic source adds `/new` and drops `/old`, the **opted-in** client receives exactly one text frame `{"notification": "topics_changed", "added": [{"name": "/new", "type": "..."}], "removed": ["/old"], "protocol_version": 1}`; a non-opted-in client receives nothing; no change → no notification; non-whitelisted topics (Task 1 filter) never appear in the diff. +- [ ] **Step 4: Implement**: dispatch the two commands in `process_single_request` (`bridge_server.cpp:182-196`); `check_topic_changes()` diffs `topic_source_->get_topics()` (whitelist-filtered) against member `std::unordered_map known_topics_` guarded by a new `topics_mutex_` (leaf lock — never held while taking any existing lock); send via `middleware_->send_reply()` to each session with the flag. +- [ ] **Step 5: Tests green.** +- [ ] **Step 6: Wire timers**: ROS2 param `topic_poll_interval` (double, default 1.0, `0` disables) + wall timer in `ros2/src/main.cpp`; add the same interval arg to `run_standalone_event_loop()` (follow how `check_session_timeouts` is scheduled there) + `--topic-poll-interval` CLI flag in `fastdds/src/main.cpp` / `rti/src/main.cpp`. Document command, notification, and param in `docs/API.md`. +- [ ] **Step 7: Full build + test + format. Commit** `feat: opt-in pushed topic advertisement (topics_changed notification)`. + +### Task 4: Slow-client backpressure (drop-oldest) + +**Files:** +- Create: `app/include/pj_bridge/middleware/bounded_frame_queue.hpp` (header-only helper) +- Create: `tests/unit/test_bounded_frame_queue.cpp` +- Modify: `app/include/pj_bridge/middleware/websocket_middleware.hpp` + `app/src/middleware/websocket_middleware.cpp` +- Modify: `ros2/src/main.cpp`, `fastdds/src/main.cpp`, `rti/src/main.cpp` (`client_backlog_size` param/flag) +- Test: `tests/unit/test_bounded_frame_queue.cpp`, extend `tests/unit/test_websocket_middleware.cpp` + +- [ ] **Step 1: Write failing tests for `BoundedFrameQueue`** (pure, no sockets — this is what makes the policy unit-testable): `push` returns number of dropped frames (0 normally); pushing past capacity drops the **oldest**; `pop_front`/`empty`/`size`; `dropped_total()` accumulates. + +```cpp +class BoundedFrameQueue { + public: + explicit BoundedFrameQueue(size_t max_frames); + size_t push(std::vector frame); // returns #dropped (0 or 1) + std::optional> pop_front(); + bool empty() const; size_t size() const; uint64_t dropped_total() const; +}; +``` +- [ ] **Step 2: Implement; tests green.** +- [ ] **Step 3: Integrate into `WebSocketMiddleware`**: ctor gains `size_t client_backlog_size = 100`; per-client `BoundedFrameQueue` map guarded by `clients_mutex_` (erased on disconnect alongside `clients_`). New `send_binary` logic, adapted from foxglove_bridge's lossy send policy (MIT attribution comment): + 1. If `ws->bufferedAmount() < kSocketBufferHighWatermark` (constant, `1u << 20` = 1 MiB): flush queued frames in order via `sendBinary`, then send the new frame. + 2. Else enqueue; on drop, warn at most once per 30 s per client (`kDropWarnIntervalSeconds = 30`), including the cumulative drop count. + - JSON replies (`send_reply`) are untouched — direct send only. + - Add `uint64_t dropped_frame_count() const` (sum across clients) to `WebSocketMiddleware` (NOT to `MiddlewareInterface`); print it in the `--stats` output of `fastdds/src/main.cpp`. +- [ ] **Step 4: Extend `test_websocket_middleware.cpp`**: normal send path still works end-to-end (existing loopback tests keep passing); `dropped_frame_count()` starts at 0. +- [ ] **Step 5: Full build + test + format. Also run TSAN** (new mutex interactions): +```bash +pixi run -e humble bash -c 'colcon build --packages-select pj_bridge --build-base build_tsan --install-base install_tsan --cmake-args -DCMAKE_BUILD_TYPE=Release -DENABLE_TSAN=ON && source install_tsan/setup.bash && TSAN_OPTIONS="suppressions=$(pwd)/tsan_suppressions.txt" setarch $(uname -m) -R build_tsan/pj_bridge/pj_bridge_tests' +``` +(exit code 66 = pre-existing suppressed IXWebSocket warnings, acceptable; NEW data-race reports are not). +- [ ] **Step 6: Commit** `feat: bounded per-client send queue with drop-oldest backpressure`. + +### Task 5: transient_local replay (ROS2, latest message) + +**Files:** +- Modify: `app/include/pj_bridge/subscription_manager_interface.hpp` (new virtual with default) +- Modify: `ros2/include/pj_bridge_ros2/generic_subscription_manager.hpp` + `.cpp`, `ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp` + `.cpp` +- Modify: `app/include/pj_bridge/message_buffer.hpp` + `app/src/message_buffer.cpp` +- Modify: `app/src/bridge_server.cpp` (`initialize` callback + `handle_subscribe`) +- Modify: `docs/API.md` +- Test: extend `tests/unit/test_message_buffer.cpp`, `tests/unit/test_bridge_server.cpp`, `tests/unit/test_generic_subscription_manager.cpp` + +- [ ] **Step 1: Interface**: add to `SubscriptionManagerInterface`: +```cpp +/// True when every publisher of the topic offers TRANSIENT_LOCAL durability +/// (detected at subscribe time). Backends without this info return false. +virtual bool is_transient_local(const std::string& /*topic_name*/) const { return false; } +``` +Non-pure with default → FastDDS/RTI/mocks unchanged. +- [ ] **Step 2: Write failing tests**: `GenericSubscriptionManager::is_transient_local` true after subscribing to a topic whose (test) publisher is transient_local, false otherwise (store the `all_transient_local` result computed in `adapt_qos` into `SubscriptionInfo` at subscribe time — `generic_subscription_manager.cpp:80-82`). +- [ ] **Step 3: Implement; tests green.** (`Ros2SubscriptionManager` forwards to the wrapped manager.) +- [ ] **Step 4: Write failing MessageBuffer tests**: `set_latched("/t", true)` → after `add_message` + TTL expiry (use injectable `ClockFn`) + `move_messages`, `get_latched("/t")` still returns the newest message; `set_latched("/t", false)` clears it; non-latched topics return `nullopt`; `clear()` clears latched storage too. + +New API on MessageBuffer: +```cpp +void set_latched(const std::string& topic_name, bool latched); +std::optional get_latched(const std::string& topic_name) const; +``` +Implementation: separate `std::unordered_map latched_last_` + `std::unordered_set latched_topics_`, updated inside `add_message` (normal queue behavior unchanged; `move_messages` does NOT touch the latched store). Retained entries persist until `set_latched(false)` or `clear()` — bounded at one message per latched topic. +- [ ] **Step 5: Implement; tests green.** +- [ ] **Step 6: Write failing BridgeServer test**: mock sub manager reports `is_transient_local("/latched") == true`; first client subscribes, a message arrives, TTL passes; a **second** client subscribes → that client immediately receives a single-message binary frame containing the retained message (decode it with the test helper already used by binary-frame assertions in `test_bridge_server.cpp`); the first client receives nothing extra. +- [ ] **Step 7: Implement in BridgeServer**: + - In `handle_subscribe`, after a successful `subscription_manager_->subscribe(...)` (`bridge_server.cpp:340-348`): call `message_buffer_->set_latched(topic, subscription_manager_->is_transient_local(topic))`. + - Still inside the success path: if latched and `get_latched(topic)` has a value, serialize that one message with `AggregatedMessageSerializer` and `middleware_->send_binary(client_id, frame)` immediately (rate-limit state untouched — replay bypasses it by construction). + - Rationale (comment it): a brand-new first subscription receives the sample from DDS naturally; the replay path covers later clients joining an already-shared subscription whose sample aged out of the TTL buffer. +- [ ] **Step 8: Full build + test + format. Commit** `feat: replay latest transient_local message to late subscribers (ROS2)`. + +### Task 6: TLS (OpenSSL, server cert only) + +**Files:** +- Modify: `CMakeLists.txt` (option `PJ_BRIDGE_TLS` default ON; FetchContent branch: `set(USE_TLS ${PJ_BRIDGE_TLS} ...)` + `set(USE_OPEN_SSL ON ...)` replacing the hardcoded `USE_TLS OFF` at `CMakeLists.txt:64`) +- Modify: `app/include/pj_bridge/middleware/websocket_middleware.hpp` + `app/src/middleware/websocket_middleware.cpp` +- Modify: `ros2/src/main.cpp`, `fastdds/src/main.cpp`, `rti/src/main.cpp`, `docs/API.md`, `.github` CI if OpenSSL dev headers are missing (pixi.toml `openssl` dependency if needed) +- Test: extend `tests/unit/test_websocket_middleware.cpp` + +- [ ] **Step 1: CMake**: flip the FetchContent flags; verify a clean configure+build passes locally (OpenSSL comes from the pixi env; add `openssl` to pixi.toml dependencies if configure fails to find it). +- [ ] **Step 2: Write failing tests**: `WebSocketMiddleware` constructed with `TlsConfig{"/nonexistent.pem", "/nonexistent.key"}` → `initialize()` returns an error mentioning the missing file; TLS round-trip test (guarded by `#ifdef IXWEBSOCKET_USE_TLS`): generate a self-signed cert in the test fixture via +```bash +openssl req -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -days 1 -nodes -subj "/CN=localhost" +``` +(into a temp dir; `std::system` in the fixture), start middleware with TLS, connect an `ix::WebSocket` client with `tlsOptions.caFile = "NONE"` (disables verification), assert echo works. +- [ ] **Step 3: Implement**: +```cpp +struct TlsConfig { std::string certfile; std::string keyfile; }; +explicit WebSocketMiddleware(size_t client_backlog_size = 100, std::optional tls = std::nullopt); +``` +In `initialize()`: if TLS requested — fail with a clear message when the files don't exist/aren't readable, or when built without TLS (`#ifndef IXWEBSOCKET_USE_TLS` → error "IXWebSocket built without TLS support"); otherwise `ix::SocketTLSOptions opts; opts.tls = true; opts.certFile = ...; opts.keyFile = ...; opts.caFile = "NONE"; server_->setTLSOptions(opts);` before `listen()`. +- [ ] **Step 4: Tests green.** +- [ ] **Step 5: Wire config**: ROS2 params `tls` (bool, false), `certfile` (""), `keyfile` (""); `tls=true` with missing params → error + exit 1. CLI: `--certfile` + `--keyfile` (both or neither; CLI11 `needs()`), presence implies TLS. Document in `docs/API.md` + README (`wss://` URL note). +- [ ] **Step 6: Full build + test + format. Commit** `feat: optional TLS (wss://) via OpenSSL server certificate`. + +### Task 7: Documentation & changelog sweep + +**Files:** +- Modify: `README.md` (feature list + parameter tables), `CLAUDE.md` (config section, test count), `CHANGELOG.rst`, `docs/API.md` (final consistency pass) + +- [ ] **Step 1**: Update README parameter tables (all new params/flags for all backends), CLAUDE.md configuration + test-count sections, CHANGELOG.rst entry for the six features. +- [ ] **Step 2**: Re-read `docs/API.md` top-to-bottom for consistency (every new command/notification/param documented exactly once, examples valid JSON). +- [ ] **Step 3: Full build + test + TSAN + ASAN**: +```bash +pixi run -e humble bash -c 'colcon build --packages-select pj_bridge --build-base build_asan --install-base install_asan --cmake-args -DCMAKE_BUILD_TYPE=Release -DENABLE_ASAN=ON && source install_asan/setup.bash && ASAN_OPTIONS="new_delete_type_mismatch=0" LSAN_OPTIONS="suppressions=$(pwd)/asan_suppressions.txt" build_asan/pj_bridge/pj_bridge_tests' +``` +- [ ] **Step 4: Commit** `docs: document foxglove-parity features`. + +--- + +## Verification (whole plan) + +1. `pixi run -e humble build && pixi run -e humble test` — all tests green (baseline 177 + new). +2. TSAN and ASAN runs clean (modulo documented pre-existing suppressions). +3. `git log --oneline origin/main..HEAD` shows one commit per task + spec/plan docs. +4. `docs/API.md` documents: `topic_whitelist`, `min_qos_depth`/`max_qos_depth`, `subscribe_topic_updates`/`unsubscribe_topic_updates` + `topics_changed`, `client_backlog_size`, `topic_poll_interval`, `tls`/`certfile`/`keyfile`. From bd879e499bf9c82818fba3d1905a0e35648b2e62 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 14:14:39 +0200 Subject: [PATCH 03/16] feat: add topic whitelist regex filtering Co-Authored-By: Claude Fable 5 --- CMakeLists.txt | 2 + app/include/pj_bridge/bridge_server.hpp | 5 +- app/include/pj_bridge/whitelist_filter.hpp | 57 ++++++++++++ app/src/bridge_server.cpp | 19 +++- app/src/whitelist_filter.cpp | 51 ++++++++++ docs/API.md | 31 +++++++ fastdds/src/main.cpp | 18 +++- ros2/src/main.cpp | 16 +++- rti/src/main.cpp | 18 +++- tests/unit/test_bridge_server.cpp | 92 +++++++++++++++++++ tests/unit/test_ros2_subscription_manager.cpp | 5 +- tests/unit/test_whitelist_filter.cpp | 87 ++++++++++++++++++ 12 files changed, 392 insertions(+), 9 deletions(-) create mode 100644 app/include/pj_bridge/whitelist_filter.hpp create mode 100644 app/src/whitelist_filter.cpp create mode 100644 tests/unit/test_whitelist_filter.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 1fc0143..df55f42 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -93,6 +93,7 @@ add_library(pj_bridge_app STATIC app/src/message_serializer.cpp app/src/bridge_server.cpp app/src/standalone_event_loop.cpp + app/src/whitelist_filter.cpp ) target_include_directories(pj_bridge_app PUBLIC @@ -246,6 +247,7 @@ if(BUILD_TESTING AND ament_cmake_FOUND) tests/unit/test_message_serializer.cpp tests/unit/test_bridge_server.cpp tests/unit/test_protocol_constants.cpp + tests/unit/test_whitelist_filter.cpp tests/unit/test_topic_discovery.cpp tests/unit/test_schema_extractor.cpp tests/unit/test_generic_subscription_manager.cpp diff --git a/app/include/pj_bridge/bridge_server.hpp b/app/include/pj_bridge/bridge_server.hpp index 9ac03dc..0206b53 100644 --- a/app/include/pj_bridge/bridge_server.hpp +++ b/app/include/pj_bridge/bridge_server.hpp @@ -31,6 +31,7 @@ #include "pj_bridge/session_manager.hpp" #include "pj_bridge/subscription_manager_interface.hpp" #include "pj_bridge/topic_source_interface.hpp" +#include "pj_bridge/whitelist_filter.hpp" namespace pj_bridge { @@ -63,12 +64,13 @@ class BridgeServer { * @param port Server port (default: 9090) * @param session_timeout Session timeout in seconds (default: 10.0) * @param publish_rate Message aggregation publish rate in Hz (default: 50.0) + * @param whitelist Topic whitelist filter (default: matches everything) */ explicit BridgeServer( std::shared_ptr topic_source, std::shared_ptr subscription_manager, std::shared_ptr middleware, int port = 9090, double session_timeout = 10.0, - double publish_rate = 50.0); + double publish_rate = 50.0, WhitelistFilter whitelist = {}); /// Shuts down middleware before members are destroyed, preventing /// disconnect callbacks from firing into a partially destroyed object. @@ -148,6 +150,7 @@ class BridgeServer { int port_; double session_timeout_; double publish_rate_; + WhitelistFilter whitelist_; // State std::atomic initialized_; diff --git a/app/include/pj_bridge/whitelist_filter.hpp b/app/include/pj_bridge/whitelist_filter.hpp new file mode 100644 index 0000000..a176d98 --- /dev/null +++ b/app/include/pj_bridge/whitelist_filter.hpp @@ -0,0 +1,57 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#pragma once + +#include +#include +#include + +#include "tl/expected.hpp" + +namespace pj_bridge { + +/// Filters topic names against a set of full-match ECMAScript regex patterns. +/// +/// Mirrors foxglove_bridge's `topic_whitelist` semantics: a topic is allowed +/// if it fully matches at least one pattern (std::regex_match, not +/// regex_search — a pattern must match the entire topic name, not just a +/// prefix or substring). +/// +/// A default-constructed (or empty-pattern-list) filter matches every topic, +/// preserving today's "no whitelist" behavior. +class WhitelistFilter { + public: + /// Default: no patterns configured, matches every topic name. + WhitelistFilter() = default; + + /// Compile `patterns` as ECMAScript regexes. + /// @return the filter on success, or an error message naming the first + /// pattern that failed to compile. + static tl::expected create(const std::vector& patterns); + + /// @return true if `topic_name` fully matches any configured pattern, or + /// if no patterns are configured (match-all default). + bool matches(const std::string& topic_name) const; + + private: + std::vector patterns_; // empty = match all +}; + +} // namespace pj_bridge diff --git a/app/src/bridge_server.cpp b/app/src/bridge_server.cpp index 1846084..d940ea8 100644 --- a/app/src/bridge_server.cpp +++ b/app/src/bridge_server.cpp @@ -60,13 +60,14 @@ double clamp_rate_hz(double rate_hz) { BridgeServer::BridgeServer( std::shared_ptr topic_source, std::shared_ptr subscription_manager, std::shared_ptr middleware, - int port, double session_timeout, double publish_rate) + int port, double session_timeout, double publish_rate, WhitelistFilter whitelist) : topic_source_(std::move(topic_source)), subscription_manager_(std::move(subscription_manager)), middleware_(std::move(middleware)), port_(port), session_timeout_(session_timeout), publish_rate_(publish_rate), + whitelist_(std::move(whitelist)), initialized_(false), total_messages_published_(0), total_bytes_published_(0), @@ -225,16 +226,21 @@ std::string BridgeServer::handle_get_topics(const std::string& client_id, const response["status"] = "success"; response["topics"] = json::array(); + size_t returned_count = 0; for (const auto& topic : topics) { + if (!whitelist_.matches(topic.name)) { + continue; + } json topic_entry; topic_entry["name"] = topic.name; topic_entry["type"] = topic.type; response["topics"].push_back(topic_entry); + returned_count++; } inject_response_fields(response, request); - spdlog::info("Returning {} topics to client '{}'", topics.size(), client_id); + spdlog::info("Returning {} topics to client '{}'", returned_count, client_id); return response.dump(); } @@ -297,6 +303,15 @@ std::string BridgeServer::handle_subscribe(const std::string& client_id, const n std::unordered_map extracted_schemas; for (const auto& topic_name : topics_to_add) { + if (!whitelist_.matches(topic_name)) { + spdlog::warn("Client '{}' requested non-whitelisted topic '{}'", client_id, topic_name); + json failure; + failure["topic"] = topic_name; + failure["reason"] = "Topic not whitelisted"; + failures.push_back(failure); + continue; + } + if (topic_types.find(topic_name) == topic_types.end()) { spdlog::warn("Client '{}' requested non-existent topic '{}'", client_id, topic_name); json failure; diff --git a/app/src/whitelist_filter.cpp b/app/src/whitelist_filter.cpp new file mode 100644 index 0000000..4d0f48d --- /dev/null +++ b/app/src/whitelist_filter.cpp @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#include "pj_bridge/whitelist_filter.hpp" + +namespace pj_bridge { + +tl::expected WhitelistFilter::create(const std::vector& patterns) { + WhitelistFilter filter; + filter.patterns_.reserve(patterns.size()); + + for (const auto& pattern : patterns) { + try { + filter.patterns_.emplace_back(pattern, std::regex::ECMAScript); + } catch (const std::regex_error& e) { + return tl::unexpected("Invalid whitelist regex '" + pattern + "': " + e.what()); + } + } + + return filter; +} + +bool WhitelistFilter::matches(const std::string& topic_name) const { + if (patterns_.empty()) { + return true; + } + for (const auto& pattern : patterns_) { + if (std::regex_match(topic_name, pattern)) { + return true; + } + } + return false; +} + +} // namespace pj_bridge diff --git a/docs/API.md b/docs/API.md index ebfcc90..b484a34 100644 --- a/docs/API.md +++ b/docs/API.md @@ -71,6 +71,10 @@ sequenceDiagram Discover available ROS2 topics. +If a `topic_whitelist` is configured on the server, topics whose name does not +fully match any whitelist pattern are omitted from the response entirely (see +[Topic Whitelist](#topic-whitelist) below). + **Request:** ```json {"command": "get_topics", "id": "gt1"} @@ -88,10 +92,37 @@ Discover available ROS2 topics. } ``` +## Topic Whitelist + +The server can be configured with a list of regex patterns restricting which +topics are visible and subscribable, mirroring foxglove_bridge's +`topic_whitelist` option: + +- **ROS2**: string-array parameter `topic_whitelist`, default `[".*"]` (match everything). +- **FastDDS / RTI**: repeatable CLI flag `--topic-whitelist`, default `.*`. + +Matching uses **full-match** ECMAScript regex semantics (`std::regex_match`, +not a substring/prefix search): a pattern must match the *entire* topic name. +For example, pattern `/cam` does **not** match `/camera`, but `/camera.*` +matches `/camera` and `/camera/image`. A topic is allowed if it fully matches +**any** configured pattern. An empty pattern list (or the default `.*`) +matches every topic. + +Non-whitelisted topics are excluded from `get_topics` responses, and +`subscribe` requests targeting them fail per-topic with reason +`"Topic not whitelisted"` (see below). + ## Subscribe Subscribe to one or more topics. **Breaking change:** Subscribe now uses an additive model - it only adds topics without removing existing subscriptions. Use the `unsubscribe` command to remove topics. +If the server has a `topic_whitelist` configured, requests for topics that +don't fully match any whitelist pattern fail with reason +`"Topic not whitelisted"` — same failure shape as a nonexistent topic (see +[Topic Whitelist](#topic-whitelist)). If every requested topic is rejected +this way, the response is `status: "error"` with +`error_code: "ALL_SUBSCRIPTIONS_FAILED"`. + Each topic in the array can be either a plain string or an object with a `max_rate_hz` field for per-topic rate limiting. Both formats can be mixed in the same request. When `max_rate_hz` is set, the server decimates messages for that topic, sending at most one message per rate interval (the first eligible buffered message). An explicit value of `0` means unlimited (all messages forwarded). A plain string leaves the rate unspecified: new subscriptions default to unlimited, and re-subscribing to an already-subscribed topic with a plain string preserves its previously configured rate limit. diff --git a/fastdds/src/main.cpp b/fastdds/src/main.cpp index ce00885..afece4e 100644 --- a/fastdds/src/main.cpp +++ b/fastdds/src/main.cpp @@ -25,6 +25,7 @@ #include "pj_bridge/middleware/websocket_middleware.hpp" #include "pj_bridge/standalone_event_loop.hpp" +#include "pj_bridge/whitelist_filter.hpp" #include "pj_bridge_fastdds/fastdds_subscription_manager.hpp" #include "pj_bridge_fastdds/fastdds_topic_source.hpp" @@ -37,6 +38,7 @@ int main(int argc, char* argv[]) { double session_timeout = 10.0; std::string log_level = "info"; bool stats_enabled = false; + std::vector topic_whitelist{".*"}; app.add_option("--domains,-d", domain_ids, "DDS domain IDs")->required()->expected(1, -1); app.add_option("--port,-p", port, "WebSocket port")->default_val(9090)->check(CLI::Range(1, 65535)); @@ -44,6 +46,11 @@ int main(int argc, char* argv[]) { app.add_option("--session-timeout", session_timeout, "Session timeout in seconds")->default_val(10.0); app.add_option("--log-level", log_level, "Log level (trace, debug, info, warn, error)")->default_val("info"); app.add_flag("--stats", stats_enabled, "Print statistics every 5 seconds"); + // Bound variable is already initialized to {".*"} (match everything); CLI11 + // leaves it untouched if the flag is not passed, so no default_val() is + // needed (and default_val() on a vector would round-trip through a + // single joined string, which is not what we want here). + app.add_option("--topic-whitelist", topic_whitelist, "Topic whitelist regex patterns (full-match, ECMAScript)"); CLI11_PARSE(app, argc, argv); @@ -54,13 +61,22 @@ int main(int argc, char* argv[]) { spdlog::info(" Port: {}", port); spdlog::info(" Publish rate: {:.1f} Hz", publish_rate); spdlog::info(" Session timeout: {:.1f} s", session_timeout); + spdlog::info(" Topic whitelist: {}", fmt::join(topic_whitelist, ", ")); + + auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); + if (!whitelist_result) { + spdlog::error("Invalid --topic-whitelist: {}", whitelist_result.error()); + return 1; + } try { auto topic_source = std::make_shared(domain_ids); auto sub_manager = std::make_shared(*topic_source); auto middleware = std::make_shared(); - pj_bridge::BridgeServer server(topic_source, sub_manager, middleware, port, session_timeout, publish_rate); + pj_bridge::BridgeServer server( + topic_source, sub_manager, middleware, port, session_timeout, publish_rate, + std::move(whitelist_result.value())); pj_bridge::run_standalone_event_loop( server, sub_manager, middleware, {port, publish_rate, session_timeout, stats_enabled}); diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index 53900db..25a7a24 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -22,9 +22,12 @@ #include #include #include +#include +#include #include "pj_bridge/bridge_server.hpp" #include "pj_bridge/middleware/websocket_middleware.hpp" +#include "pj_bridge/whitelist_filter.hpp" #include "pj_bridge_ros2/ros2_subscription_manager.hpp" #include "pj_bridge_ros2/ros2_topic_source.hpp" @@ -40,17 +43,26 @@ int main(int argc, char** argv) { node->declare_parameter("publish_rate", 50.0); node->declare_parameter("session_timeout", 10.0); node->declare_parameter("strip_large_messages", false); + node->declare_parameter>("topic_whitelist", {".*"}); int port = node->get_parameter("port").as_int(); double publish_rate = node->get_parameter("publish_rate").as_double(); double session_timeout = node->get_parameter("session_timeout").as_double(); bool strip_large_messages = node->get_parameter("strip_large_messages").as_bool(); + std::vector topic_whitelist = node->get_parameter("topic_whitelist").as_string_array(); RCLCPP_INFO( node->get_logger(), "Configuration: port=%d, publish_rate=%.1f Hz, session_timeout=%.1f s, strip_large_messages=%s", port, publish_rate, session_timeout, strip_large_messages ? "true" : "false"); + auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); + if (!whitelist_result) { + RCLCPP_ERROR(node->get_logger(), "Invalid topic_whitelist: %s", whitelist_result.error().c_str()); + rclcpp::shutdown(); + return 1; + } + try { // Create backend components auto topic_source = std::make_shared(node); @@ -58,7 +70,9 @@ int main(int argc, char** argv) { auto middleware = std::make_shared(); // Create bridge server - pj_bridge::BridgeServer server(topic_source, sub_manager, middleware, port, session_timeout, publish_rate); + pj_bridge::BridgeServer server( + topic_source, sub_manager, middleware, port, session_timeout, publish_rate, + std::move(whitelist_result.value())); if (!server.initialize()) { RCLCPP_ERROR(node->get_logger(), "Failed to initialize bridge server"); diff --git a/rti/src/main.cpp b/rti/src/main.cpp index e069fab..b86d4f0 100644 --- a/rti/src/main.cpp +++ b/rti/src/main.cpp @@ -25,6 +25,7 @@ #include "pj_bridge/middleware/websocket_middleware.hpp" #include "pj_bridge/standalone_event_loop.hpp" +#include "pj_bridge/whitelist_filter.hpp" #include "pj_bridge_rti/dds_subscription_manager.hpp" #include "pj_bridge_rti/dds_topic_discovery.hpp" @@ -38,6 +39,7 @@ int main(int argc, char* argv[]) { std::string qos_profile; std::string log_level = "info"; bool stats_enabled = false; + std::vector topic_whitelist{".*"}; app.add_option("--domains,-d", domain_ids, "DDS domain IDs")->required()->expected(1, -1); app.add_option("--port,-p", port, "WebSocket port")->default_val(9090)->check(CLI::Range(1, 65535)); @@ -46,6 +48,11 @@ int main(int argc, char* argv[]) { app.add_option("--qos-profile", qos_profile, "QoS profile XML file path"); app.add_option("--log-level", log_level, "Log level (trace, debug, info, warn, error)")->default_val("info"); app.add_flag("--stats", stats_enabled, "Print statistics every 5 seconds"); + // Bound variable is already initialized to {".*"} (match everything); CLI11 + // leaves it untouched if the flag is not passed, so no default_val() is + // needed (and default_val() on a vector would round-trip through a + // single joined string, which is not what we want here). + app.add_option("--topic-whitelist", topic_whitelist, "Topic whitelist regex patterns (full-match, ECMAScript)"); CLI11_PARSE(app, argc, argv); @@ -59,13 +66,22 @@ int main(int argc, char* argv[]) { if (!qos_profile.empty()) { spdlog::info(" QoS profile: {}", qos_profile); } + spdlog::info(" Topic whitelist: {}", fmt::join(topic_whitelist, ", ")); + + auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); + if (!whitelist_result) { + spdlog::error("Invalid --topic-whitelist: {}", whitelist_result.error()); + return 1; + } try { auto topic_source = std::make_shared(domain_ids, qos_profile); auto sub_manager = std::make_shared(*topic_source); auto middleware = std::make_shared(); - pj_bridge::BridgeServer server(topic_source, sub_manager, middleware, port, session_timeout, publish_rate); + pj_bridge::BridgeServer server( + topic_source, sub_manager, middleware, port, session_timeout, publish_rate, + std::move(whitelist_result.value())); pj_bridge::run_standalone_event_loop( server, sub_manager, middleware, {port, publish_rate, session_timeout, stats_enabled}); diff --git a/tests/unit/test_bridge_server.cpp b/tests/unit/test_bridge_server.cpp index 781eee0..06a8ba7 100644 --- a/tests/unit/test_bridge_server.cpp +++ b/tests/unit/test_bridge_server.cpp @@ -35,6 +35,7 @@ #include "pj_bridge/middleware/middleware_interface.hpp" #include "pj_bridge/subscription_manager_interface.hpp" #include "pj_bridge/topic_source_interface.hpp" +#include "pj_bridge/whitelist_filter.hpp" #include "tl/expected.hpp" using json = nlohmann::json; @@ -2003,3 +2004,94 @@ TEST_F(BridgeServerTest, SubscribeAcceptsEmptySchemaDefinition) { EXPECT_EQ(response["schemas"]["/trigger"]["definition"], ""); EXPECT_EQ(mock_sub_manager_->ref_count("/trigger"), 1); } + +// --------------------------------------------------------------------------- +// Topic whitelist filtering +// --------------------------------------------------------------------------- + +// GetTopicsOmitsNonWhitelistedTopics +TEST_F(BridgeServerTest, GetTopicsOmitsNonWhitelistedTopics) { + auto whitelist_result = WhitelistFilter::create({"/allowed.*"}); + ASSERT_TRUE(whitelist_result.has_value()); + server_ = std::make_unique( + mock_topic_source_, mock_sub_manager_, mock_, 19999, 10.0, 50.0, whitelist_result.value()); + ASSERT_TRUE(server_->initialize()); + + mock_topic_source_->set_topics({{"/allowed/foo", "std_msgs/msg/String"}, {"/blocked", "std_msgs/msg/String"}}); + + json req; + req["command"] = "get_topics"; + mock_->push_request("client_wl_get_topics", req.dump()); + server_->process_requests(); + + json reply = mock_->pop_reply("client_wl_get_topics"); + ASSERT_FALSE(reply.is_discarded()); + EXPECT_EQ(reply["status"], "success"); + ASSERT_TRUE(reply["topics"].is_array()); + ASSERT_EQ(reply["topics"].size(), 1u); + EXPECT_EQ(reply["topics"][0]["name"], "/allowed/foo"); +} + +// SubscribeToNonWhitelistedTopicFailsAllSubscriptionsFailed +TEST_F(BridgeServerTest, SubscribeToNonWhitelistedTopicFailsAllSubscriptionsFailed) { + auto whitelist_result = WhitelistFilter::create({"/allowed.*"}); + ASSERT_TRUE(whitelist_result.has_value()); + server_ = std::make_unique( + mock_topic_source_, mock_sub_manager_, mock_, 19999, 10.0, 50.0, whitelist_result.value()); + ASSERT_TRUE(server_->initialize()); + + mock_topic_source_->set_topics({{"/blocked", "std_msgs/msg/String"}}); + mock_sub_manager_->add_known_topic("/blocked"); + + json sub; + sub["command"] = "subscribe"; + sub["topics"] = json::array({"/blocked"}); + mock_->push_request("client_wl_blocked", sub.dump()); + server_->process_requests(); + + json reply = mock_->pop_reply("client_wl_blocked"); + ASSERT_FALSE(reply.is_discarded()); + EXPECT_EQ(reply["status"], "error"); + EXPECT_EQ(reply["error_code"], "ALL_SUBSCRIPTIONS_FAILED"); + ASSERT_TRUE(reply.contains("failures")); + ASSERT_FALSE(reply["failures"].empty()); + EXPECT_EQ(reply["failures"][0]["topic"], "/blocked"); + EXPECT_EQ(reply["failures"][0]["reason"], "Topic not whitelisted"); + EXPECT_EQ(mock_sub_manager_->ref_count("/blocked"), 0); +} + +// SubscribeMixedWhitelistedAndNonWhitelistedTopicsPartialSuccess +TEST_F(BridgeServerTest, SubscribeMixedWhitelistedAndNonWhitelistedTopicsPartialSuccess) { + auto whitelist_result = WhitelistFilter::create({"/allowed.*"}); + ASSERT_TRUE(whitelist_result.has_value()); + server_ = std::make_unique( + mock_topic_source_, mock_sub_manager_, mock_, 19999, 10.0, 50.0, whitelist_result.value()); + ASSERT_TRUE(server_->initialize()); + + mock_topic_source_->set_topics({{"/allowed/foo", "std_msgs/msg/String"}, {"/blocked", "std_msgs/msg/String"}}); + mock_sub_manager_->add_known_topic("/allowed/foo"); + mock_sub_manager_->add_known_topic("/blocked"); + + json sub; + sub["command"] = "subscribe"; + sub["topics"] = json::array({"/allowed/foo", "/blocked"}); + mock_->push_request("client_wl_mixed", sub.dump()); + server_->process_requests(); + + json reply = mock_->pop_reply("client_wl_mixed"); + ASSERT_FALSE(reply.is_discarded()); + EXPECT_EQ(reply["status"], "partial_success"); + ASSERT_TRUE(reply["schemas"].contains("/allowed/foo")); + ASSERT_TRUE(reply.contains("failures")); + + bool found_blocked_failure = false; + for (const auto& failure : reply["failures"]) { + if (failure["topic"] == "/blocked") { + EXPECT_EQ(failure["reason"], "Topic not whitelisted"); + found_blocked_failure = true; + } + } + EXPECT_TRUE(found_blocked_failure); + EXPECT_EQ(mock_sub_manager_->ref_count("/allowed/foo"), 1); + EXPECT_EQ(mock_sub_manager_->ref_count("/blocked"), 0); +} diff --git a/tests/unit/test_ros2_subscription_manager.cpp b/tests/unit/test_ros2_subscription_manager.cpp index ace2f2d..b21fb53 100644 --- a/tests/unit/test_ros2_subscription_manager.cpp +++ b/tests/unit/test_ros2_subscription_manager.cpp @@ -19,11 +19,10 @@ #include -#include -#include - #include #include +#include +#include #include #include "pj_bridge_ros2/ros2_subscription_manager.hpp" diff --git a/tests/unit/test_whitelist_filter.cpp b/tests/unit/test_whitelist_filter.cpp new file mode 100644 index 0000000..014196e --- /dev/null +++ b/tests/unit/test_whitelist_filter.cpp @@ -0,0 +1,87 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#include "pj_bridge/whitelist_filter.hpp" + +#include + +#include +#include + +namespace pj_bridge { +namespace { + +TEST(WhitelistFilterTest, DefaultConstructedMatchesEverything) { + WhitelistFilter filter; + EXPECT_TRUE(filter.matches("/anything")); + EXPECT_TRUE(filter.matches("/foo/bar")); + EXPECT_TRUE(filter.matches("")); +} + +TEST(WhitelistFilterTest, CreateSucceedsForValidPatterns) { + auto result = WhitelistFilter::create({"/camera.*", "/imu"}); + ASSERT_TRUE(result.has_value()); +} + +TEST(WhitelistFilterTest, CreateFailsForInvalidRegexAndNamesOffendingPattern) { + auto result = WhitelistFilter::create({"/valid", "("}); + ASSERT_FALSE(result.has_value()); + EXPECT_NE(result.error().find("("), std::string::npos); +} + +TEST(WhitelistFilterTest, FullMatchSemanticsRejectPartialMatch) { + auto result = WhitelistFilter::create({"/cam"}); + ASSERT_TRUE(result.has_value()); + WhitelistFilter filter = result.value(); + + // "/cam" must NOT match "/camera" — full match, not prefix match. + EXPECT_FALSE(filter.matches("/camera")); + EXPECT_TRUE(filter.matches("/cam")); +} + +TEST(WhitelistFilterTest, WildcardPatternMatchesSubPaths) { + auto result = WhitelistFilter::create({"/camera.*"}); + ASSERT_TRUE(result.has_value()); + WhitelistFilter filter = result.value(); + + EXPECT_TRUE(filter.matches("/camera")); + EXPECT_TRUE(filter.matches("/camera/image")); + EXPECT_FALSE(filter.matches("/other")); +} + +TEST(WhitelistFilterTest, MatchesIfAnyPatternMatches) { + auto result = WhitelistFilter::create({"/foo", "/bar.*"}); + ASSERT_TRUE(result.has_value()); + WhitelistFilter filter = result.value(); + + EXPECT_TRUE(filter.matches("/foo")); + EXPECT_TRUE(filter.matches("/bar/baz")); + EXPECT_FALSE(filter.matches("/qux")); +} + +TEST(WhitelistFilterTest, EmptyPatternListMatchesEverything) { + auto result = WhitelistFilter::create({}); + ASSERT_TRUE(result.has_value()); + WhitelistFilter filter = result.value(); + + EXPECT_TRUE(filter.matches("/anything")); +} + +} // namespace +} // namespace pj_bridge From cf493e4e322bccea9d2d8cd86ce484de9310fa0a Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 14:32:56 +0200 Subject: [PATCH 04/16] feat: aggregate publisher QoS depths with min/max clamping Co-Authored-By: Claude Fable 5 --- README.md | 2 + docs/API.md | 19 ++++ .../generic_subscription_manager.hpp | 9 +- .../ros2_subscription_manager.hpp | 4 +- ros2/src/generic_subscription_manager.cpp | 14 ++- ros2/src/main.cpp | 22 ++++- ros2/src/ros2_subscription_manager.cpp | 5 +- .../test_generic_subscription_manager.cpp | 90 +++++++++++++++++++ 8 files changed, 156 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 752f8a6..091dfdb 100644 --- a/README.md +++ b/README.md @@ -39,6 +39,8 @@ independently. | `publish_rate` | double | 50.0 | Aggregation publish rate in Hz | | `session_timeout` | double | 10.0 | Client timeout duration in seconds | | `strip_large_messages` | bool | false | Opt-in: strip large arrays from Image, PointCloud2, LaserScan, OccupancyGrid messages | +| `min_qos_depth` | int | 1 | ROS2 only: minimum KEEP_LAST subscription depth after aggregating publisher depths | +| `max_qos_depth` | int | 100 | ROS2 only: maximum KEEP_LAST subscription depth after aggregating publisher depths | ## Just "Download and Run" diff --git a/docs/API.md b/docs/API.md index b484a34..1fc2ee6 100644 --- a/docs/API.md +++ b/docs/API.md @@ -112,6 +112,25 @@ Non-whitelisted topics are excluded from `get_topics` responses, and `subscribe` requests targeting them fail per-topic with reason `"Topic not whitelisted"` (see below). +## QoS Depth Heuristics (ROS2 only) + +When creating a subscription, the ROS2 backend picks a KEEP_LAST history +depth by summing the history depth every discovered publisher on the topic +offers (so a burst from every publisher still fits in the subscription +queue), then clamping the total to a configurable range — the same heuristic +`foxglove_bridge`'s `determineQoS()` uses: + +- **ROS2**: int parameters `min_qos_depth` (default `1`) and `max_qos_depth` + (default `100`). + +If no publishers are discovered yet, the depth defaults to `min(100, +max_qos_depth)`. Both values must be `>= 0` and `min_qos_depth <= +max_qos_depth`; the server refuses to start otherwise. This is independent of +the subscription's reliability/durability, which is separately adapted to +match what the discovered publishers offer (a RELIABLE subscription still +switches to BEST_EFFORT if any publisher is BEST_EFFORT, and to +TRANSIENT_LOCAL only if every publisher offers it). + ## Subscribe Subscribe to one or more topics. **Breaking change:** Subscribe now uses an additive model - it only adds topics without removing existing subscriptions. Use the `unsubscribe` command to remove topics. diff --git a/ros2/include/pj_bridge_ros2/generic_subscription_manager.hpp b/ros2/include/pj_bridge_ros2/generic_subscription_manager.hpp index 8343555..083272a 100644 --- a/ros2/include/pj_bridge_ros2/generic_subscription_manager.hpp +++ b/ros2/include/pj_bridge_ros2/generic_subscription_manager.hpp @@ -47,7 +47,8 @@ using Ros2MessageCallback = */ class GenericSubscriptionManager { public: - explicit GenericSubscriptionManager(rclcpp::Node::SharedPtr node); + explicit GenericSubscriptionManager( + rclcpp::Node::SharedPtr node, size_t min_qos_depth = 1, size_t max_qos_depth = 100); bool subscribe(const std::string& topic_name, const std::string& topic_type, Ros2MessageCallback callback); bool unsubscribe(const std::string& topic_name); @@ -55,15 +56,19 @@ class GenericSubscriptionManager { size_t get_reference_count(const std::string& topic_name) const; void unsubscribe_all(); - private: + // Exposed publicly for testability; computes the QoS a subscription should + // use for `topic_name` based on the publishers currently discovered on it. rclcpp::QoS adapt_qos(const std::string& topic_name) const; + private: struct SubscriptionInfo { std::shared_ptr subscription; size_t reference_count; }; rclcpp::Node::SharedPtr node_; + size_t min_qos_depth_; + size_t max_qos_depth_; mutable std::mutex mutex_; std::unordered_map subscriptions_; }; diff --git a/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp b/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp index d3f6f95..f34c8e6 100644 --- a/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp +++ b/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp @@ -37,7 +37,9 @@ namespace pj_bridge { */ class Ros2SubscriptionManager : public SubscriptionManagerInterface { public: - explicit Ros2SubscriptionManager(rclcpp::Node::SharedPtr node, bool strip_large_messages = false); + explicit Ros2SubscriptionManager( + rclcpp::Node::SharedPtr node, bool strip_large_messages = false, size_t min_qos_depth = 1, + size_t max_qos_depth = 100); Ros2SubscriptionManager(const Ros2SubscriptionManager&) = delete; Ros2SubscriptionManager& operator=(const Ros2SubscriptionManager&) = delete; diff --git a/ros2/src/generic_subscription_manager.cpp b/ros2/src/generic_subscription_manager.cpp index 9e27b73..769be8e 100644 --- a/ros2/src/generic_subscription_manager.cpp +++ b/ros2/src/generic_subscription_manager.cpp @@ -19,11 +19,15 @@ #include "pj_bridge_ros2/generic_subscription_manager.hpp" +#include + #include "pj_bridge/time_utils.hpp" namespace pj_bridge { -GenericSubscriptionManager::GenericSubscriptionManager(rclcpp::Node::SharedPtr node) : node_(node) {} +GenericSubscriptionManager::GenericSubscriptionManager( + rclcpp::Node::SharedPtr node, size_t min_qos_depth, size_t max_qos_depth) + : node_(node), min_qos_depth_(min_qos_depth), max_qos_depth_(max_qos_depth) {} rclcpp::QoS GenericSubscriptionManager::adapt_qos(const std::string& topic_name) const { // Match the QoS the publishers actually offer (same policy as rosbag2): @@ -33,11 +37,17 @@ rclcpp::QoS GenericSubscriptionManager::adapt_qos(const std::string& topic_name) auto publishers = node_->get_publishers_info_by_topic(topic_name); if (publishers.empty()) { + qos.keep_last(std::min(100, max_qos_depth_)); return qos; } bool any_best_effort = false; bool all_transient_local = true; + // Depth aggregation adapted from foxglove_bridge (MIT License, + // Copyright (c) Foxglove Technologies Inc): + // sum the publishers' history depths so a burst from every publisher + // still fits, then clamp to [min_qos_depth, max_qos_depth]. + size_t total_depth = 0; for (const auto& info : publishers) { const auto& profile = info.qos_profile(); if (profile.reliability() == rclcpp::ReliabilityPolicy::BestEffort) { @@ -46,7 +56,9 @@ rclcpp::QoS GenericSubscriptionManager::adapt_qos(const std::string& topic_name) if (profile.durability() != rclcpp::DurabilityPolicy::TransientLocal) { all_transient_local = false; } + total_depth += profile.depth(); } + qos.keep_last(std::clamp(total_depth, min_qos_depth_, max_qos_depth_)); // BEST_EFFORT matches both kinds of publisher; TRANSIENT_LOCAL only // matches if every publisher offers it. diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index 25a7a24..6ac91d0 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -44,17 +44,22 @@ int main(int argc, char** argv) { node->declare_parameter("session_timeout", 10.0); node->declare_parameter("strip_large_messages", false); node->declare_parameter>("topic_whitelist", {".*"}); + node->declare_parameter("min_qos_depth", 1); + node->declare_parameter("max_qos_depth", 100); int port = node->get_parameter("port").as_int(); double publish_rate = node->get_parameter("publish_rate").as_double(); double session_timeout = node->get_parameter("session_timeout").as_double(); bool strip_large_messages = node->get_parameter("strip_large_messages").as_bool(); std::vector topic_whitelist = node->get_parameter("topic_whitelist").as_string_array(); + int64_t min_qos_depth = node->get_parameter("min_qos_depth").as_int(); + int64_t max_qos_depth = node->get_parameter("max_qos_depth").as_int(); RCLCPP_INFO( node->get_logger(), - "Configuration: port=%d, publish_rate=%.1f Hz, session_timeout=%.1f s, strip_large_messages=%s", port, - publish_rate, session_timeout, strip_large_messages ? "true" : "false"); + "Configuration: port=%d, publish_rate=%.1f Hz, session_timeout=%.1f s, strip_large_messages=%s, " + "min_qos_depth=%ld, max_qos_depth=%ld", + port, publish_rate, session_timeout, strip_large_messages ? "true" : "false", min_qos_depth, max_qos_depth); auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); if (!whitelist_result) { @@ -63,10 +68,21 @@ int main(int argc, char** argv) { return 1; } + if (min_qos_depth < 0 || max_qos_depth < 0 || min_qos_depth > max_qos_depth) { + RCLCPP_ERROR( + node->get_logger(), + "Invalid QoS depth configuration: min_qos_depth=%ld, max_qos_depth=%ld (both must be >= 0 and " + "min_qos_depth <= max_qos_depth)", + min_qos_depth, max_qos_depth); + rclcpp::shutdown(); + return 1; + } + try { // Create backend components auto topic_source = std::make_shared(node); - auto sub_manager = std::make_shared(node, strip_large_messages); + auto sub_manager = std::make_shared( + node, strip_large_messages, static_cast(min_qos_depth), static_cast(max_qos_depth)); auto middleware = std::make_shared(); // Create bridge server diff --git a/ros2/src/ros2_subscription_manager.cpp b/ros2/src/ros2_subscription_manager.cpp index 12636b9..c45c77f 100644 --- a/ros2/src/ros2_subscription_manager.cpp +++ b/ros2/src/ros2_subscription_manager.cpp @@ -26,8 +26,9 @@ namespace pj_bridge { -Ros2SubscriptionManager::Ros2SubscriptionManager(rclcpp::Node::SharedPtr node, bool strip_large_messages) - : inner_manager_(node), strip_large_messages_(strip_large_messages) {} +Ros2SubscriptionManager::Ros2SubscriptionManager( + rclcpp::Node::SharedPtr node, bool strip_large_messages, size_t min_qos_depth, size_t max_qos_depth) + : inner_manager_(node, min_qos_depth, max_qos_depth), strip_large_messages_(strip_large_messages) {} void Ros2SubscriptionManager::set_message_callback(MessageCallback callback) { std::lock_guard lock(callback_mutex_); diff --git a/tests/unit/test_generic_subscription_manager.cpp b/tests/unit/test_generic_subscription_manager.cpp index 46c40e6..c287876 100644 --- a/tests/unit/test_generic_subscription_manager.cpp +++ b/tests/unit/test_generic_subscription_manager.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -29,6 +30,32 @@ using namespace pj_bridge; +namespace { +// History depth is a purely local resource policy, not one of the +// requested/offered QoS policies DDS discovery is required to propagate +// (unlike reliability/durability, which affect wire compatibility). This +// workspace's default RMW (rmw_fastrtps_cpp) does not report it — publishers +// discovered via get_publishers_info_by_topic() always come back with depth +// 0 / history UNKNOWN — whereas CycloneDDS does propagate it. The QoS depth +// aggregation tests below need a real (non-mocked) discovered depth to +// exercise adapt_qos(), so force CycloneDDS for this test binary. +// +// The RMW implementation is selected once per process and cached on first +// use, so this must happen before any rclcpp::init() call anywhere in the +// binary. Static objects across all translation units are guaranteed to be +// constructed before main() runs (and therefore before any TEST_F body), +// so a file-scope static works regardless of test execution order. An +// operator's own explicit RMW_IMPLEMENTATION is still respected. +struct ForceCycloneDdsForQosDepthTests { + ForceCycloneDdsForQosDepthTests() { + if (std::getenv("RMW_IMPLEMENTATION") == nullptr) { + setenv("RMW_IMPLEMENTATION", "rmw_cyclonedds_cpp", 1); + } + } +}; +const ForceCycloneDdsForQosDepthTests force_cyclonedds_for_qos_depth_tests; +} // namespace + class GenericSubscriptionManagerTest : public ::testing::Test { protected: void SetUp() override { @@ -47,6 +74,20 @@ class GenericSubscriptionManagerTest : public ::testing::Test { std::unique_ptr manager_; }; +namespace { +// Publisher discovery is asynchronous even within a single process, so tests +// must poll the graph rather than assume a freshly created publisher is +// immediately visible via get_publishers_info_by_topic(). +void wait_for_publisher_count( + const rclcpp::Node::SharedPtr& node, const std::string& topic_name, size_t expected_count) { + auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (node->get_publishers_info_by_topic(topic_name).size() < expected_count && + std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } +} +} // namespace + TEST_F(GenericSubscriptionManagerTest, Subscribe) { bool callback_called = false; auto callback = [&callback_called](const std::string&, const std::shared_ptr&, uint64_t) { @@ -226,6 +267,9 @@ TEST_F(GenericSubscriptionManagerTest, MultipleTopicsIndependent) { // --------------------------------------------------------------------------- TEST_F(GenericSubscriptionManagerTest, AdaptsQosToBestEffortPublisher) { auto publisher = node_->create_publisher("/qos_be_topic", rclcpp::SensorDataQoS()); + // Wait for discovery before subscribing so adapt_qos() sees the publisher + // (discovery is asynchronous and its latency varies by RMW implementation). + wait_for_publisher_count(node_, "/qos_be_topic", 1); std::atomic received{0}; auto callback = [&received](const std::string&, const std::shared_ptr&, uint64_t) { @@ -247,3 +291,49 @@ TEST_F(GenericSubscriptionManagerTest, AdaptsQosToBestEffortPublisher) { EXPECT_GT(received.load(), 0) << "RELIABLE subscription never matches a BEST_EFFORT publisher"; } + +// --------------------------------------------------------------------------- +// QoS depth aggregation +// +// Depth aggregation adapted from foxglove_bridge (MIT License, Copyright (c) +// Foxglove Technologies Inc): sum the publishers' history depths so a burst +// from every publisher still fits, then clamp to [min_qos_depth, max_qos_depth]. +// (See the ForceCycloneDdsForQosDepthTests note above for why this test +// binary forces CycloneDDS.) +// --------------------------------------------------------------------------- + +TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthMatchesSinglePublisher) { + auto publisher = node_->create_publisher("/qos_depth_topic1", rclcpp::QoS(10)); + wait_for_publisher_count(node_, "/qos_depth_topic1", 1); + + rclcpp::QoS qos = manager_->adapt_qos("/qos_depth_topic1"); + + EXPECT_EQ(qos.depth(), 10u); +} + +TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthSumsAcrossPublishersClampedToMax) { + auto publisher1 = node_->create_publisher("/qos_depth_topic2", rclcpp::QoS(60)); + auto publisher2 = node_->create_publisher("/qos_depth_topic2", rclcpp::QoS(60)); + wait_for_publisher_count(node_, "/qos_depth_topic2", 2); + + rclcpp::QoS qos = manager_->adapt_qos("/qos_depth_topic2"); + + // 60 + 60 = 120, clamped to the default max_qos_depth of 100. + EXPECT_EQ(qos.depth(), 100u); +} + +TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthClampedToConfiguredMinimum) { + auto manager_with_min = std::make_unique(node_, /*min_qos_depth=*/30); + auto publisher = node_->create_publisher("/qos_depth_topic3", rclcpp::QoS(5)); + wait_for_publisher_count(node_, "/qos_depth_topic3", 1); + + rclcpp::QoS qos = manager_with_min->adapt_qos("/qos_depth_topic3"); + + EXPECT_EQ(qos.depth(), 30u); +} + +TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthDefaultsTo100WhenNoPublishers) { + rclcpp::QoS qos = manager_->adapt_qos("/qos_depth_no_publishers"); + + EXPECT_EQ(qos.depth(), 100u); +} From 72b2708cf282af4bcb39a611b57f44d8c93923a4 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 14:39:03 +0200 Subject: [PATCH 05/16] fix: fall back to depth 100 when publisher history depth is unknown Under RMWs that don't propagate publisher history depth through discovery (e.g. rmw_fastrtps_cpp, the default), every publisher reports depth 0, so the aggregated total clamped to min_qos_depth and produced depth-1 subscription queues that drop messages on high-rate topics. Treat a total of 0 as "unknown" and keep the historical default of min(100, max_qos_depth) instead. Also declare rmw_cyclonedds_cpp as a test_depend (the depth aggregation tests force it, and apt-based CI containers don't ship it), and fix include ordering in test_whitelist_filter.cpp so pre-commit is clean repo-wide. Co-Authored-By: Claude Fable 5 --- docs/API.md | 6 ++++-- package.xml | 4 ++++ ros2/src/generic_subscription_manager.cpp | 10 +++++++++- .../unit/test_generic_subscription_manager.cpp | 18 ++++++++++++++++++ tests/unit/test_whitelist_filter.cpp | 4 ++-- 5 files changed, 37 insertions(+), 5 deletions(-) diff --git a/docs/API.md b/docs/API.md index 1fc2ee6..c3bee1e 100644 --- a/docs/API.md +++ b/docs/API.md @@ -123,8 +123,10 @@ queue), then clamping the total to a configurable range — the same heuristic - **ROS2**: int parameters `min_qos_depth` (default `1`) and `max_qos_depth` (default `100`). -If no publishers are discovered yet, the depth defaults to `min(100, -max_qos_depth)`. Both values must be `>= 0` and `min_qos_depth <= +If no publishers are discovered yet — or every discovered publisher reports +depth `0` (KEEP_ALL, or an RMW such as `rmw_fastrtps_cpp` that does not +propagate history depth through discovery) — the depth falls back to +`min(100, max_qos_depth)`. Both values must be `>= 0` and `min_qos_depth <= max_qos_depth`; the server refuses to start otherwise. This is independent of the subscription's reliability/durability, which is separately adapted to match what the discovered publishers offer (a RELIABLE subscription still diff --git a/package.xml b/package.xml index bb417a4..d3f1e2c 100644 --- a/package.xml +++ b/package.xml @@ -27,6 +27,10 @@ ament_cmake_gtest geometry_msgs + + rmw_cyclonedds_cpp ament_cmake diff --git a/ros2/src/generic_subscription_manager.cpp b/ros2/src/generic_subscription_manager.cpp index 769be8e..6943076 100644 --- a/ros2/src/generic_subscription_manager.cpp +++ b/ros2/src/generic_subscription_manager.cpp @@ -58,7 +58,15 @@ rclcpp::QoS GenericSubscriptionManager::adapt_qos(const std::string& topic_name) } total_depth += profile.depth(); } - qos.keep_last(std::clamp(total_depth, min_qos_depth_, max_qos_depth_)); + // Some RMWs (e.g. rmw_fastrtps_cpp) don't propagate publisher history + // depth through discovery and report 0 for every publisher; KEEP_ALL + // publishers also report 0. A total of 0 therefore means "unknown", not + // "no queue needed" — fall back to the historical default of 100 instead + // of clamping 0 up to min_qos_depth, which would create tiny queues that + // drop messages on high-rate topics. + const size_t depth = + total_depth > 0 ? std::clamp(total_depth, min_qos_depth_, max_qos_depth_) : std::min(100, max_qos_depth_); + qos.keep_last(depth); // BEST_EFFORT matches both kinds of publisher; TRANSIENT_LOCAL only // matches if every publisher offers it. diff --git a/tests/unit/test_generic_subscription_manager.cpp b/tests/unit/test_generic_subscription_manager.cpp index c287876..7029089 100644 --- a/tests/unit/test_generic_subscription_manager.cpp +++ b/tests/unit/test_generic_subscription_manager.cpp @@ -337,3 +337,21 @@ TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthDefaultsTo100WhenNoPublisher EXPECT_EQ(qos.depth(), 100u); } + +TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthFallsBackTo100WhenDepthUnknown) { + // A KEEP_ALL publisher reports history depth 0 through discovery — the + // same value RMWs that don't propagate depth at all (e.g. rmw_fastrtps_cpp) + // report for every publisher. A total of 0 must be treated as "unknown" + // and fall back to the historical default of 100, not clamped up to + // min_qos_depth (a depth-1 queue would drop messages on high-rate topics). + auto publisher = + node_->create_publisher("/qos_depth_keep_all_topic", rclcpp::QoS(rclcpp::KeepAll())); + wait_for_publisher_count(node_, "/qos_depth_keep_all_topic", 1); + + ASSERT_EQ(node_->get_publishers_info_by_topic("/qos_depth_keep_all_topic")[0].qos_profile().depth(), 0u) + << "Precondition: discovery must report depth 0 for a KEEP_ALL publisher"; + + rclcpp::QoS qos = manager_->adapt_qos("/qos_depth_keep_all_topic"); + + EXPECT_EQ(qos.depth(), 100u); +} diff --git a/tests/unit/test_whitelist_filter.cpp b/tests/unit/test_whitelist_filter.cpp index 014196e..d7074cb 100644 --- a/tests/unit/test_whitelist_filter.cpp +++ b/tests/unit/test_whitelist_filter.cpp @@ -17,13 +17,13 @@ * along with pj_bridge. If not, see . */ -#include "pj_bridge/whitelist_filter.hpp" - #include #include #include +#include "pj_bridge/whitelist_filter.hpp" + namespace pj_bridge { namespace { From 866371d0bb56485dead61b832d9b3ead2e53f2df Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 14:48:35 +0200 Subject: [PATCH 06/16] feat: opt-in pushed topic advertisement (topics_changed notification) Co-Authored-By: Claude Fable 5 --- app/include/pj_bridge/bridge_server.hpp | 25 ++ app/include/pj_bridge/session_manager.hpp | 5 + .../pj_bridge/standalone_event_loop.hpp | 2 + app/src/bridge_server.cpp | 109 ++++++++ app/src/session_manager.cpp | 23 ++ app/src/standalone_event_loop.cpp | 8 + docs/API.md | 57 ++++ fastdds/src/main.cpp | 13 +- ros2/src/main.cpp | 26 +- rti/src/main.cpp | 13 +- tests/unit/test_bridge_server.cpp | 246 ++++++++++++++++++ tests/unit/test_session_manager.cpp | 24 ++ 12 files changed, 547 insertions(+), 4 deletions(-) diff --git a/app/include/pj_bridge/bridge_server.hpp b/app/include/pj_bridge/bridge_server.hpp index 0206b53..844c226 100644 --- a/app/include/pj_bridge/bridge_server.hpp +++ b/app/include/pj_bridge/bridge_server.hpp @@ -108,6 +108,18 @@ class BridgeServer { */ void check_session_timeouts(); + /** + * @brief Check whether the (whitelist-filtered) topic set has changed and + * notify opted-in clients + * + * Called externally by the event loop (e.g. every topic_poll_interval + * seconds). The first call after construction only takes a snapshot and + * sends nothing; subsequent calls diff against that snapshot and send a + * `topics_changed` notification (added/removed) to every session that + * called `subscribe_topic_updates`, but only when the diff is non-empty. + */ + void check_topic_changes(); + /** * @brief Get the number of active client sessions */ @@ -132,6 +144,8 @@ class BridgeServer { std::string handle_heartbeat(const std::string& client_id, const nlohmann::json& request); std::string handle_pause(const std::string& client_id, const nlohmann::json& request); std::string handle_resume(const std::string& client_id, const nlohmann::json& request); + std::string handle_subscribe_topic_updates(const std::string& client_id, const nlohmann::json& request); + std::string handle_unsubscribe_topic_updates(const std::string& client_id, const nlohmann::json& request); std::string create_error_response( const std::string& error_code, const std::string& message, const nlohmann::json& request) const; void inject_response_fields(nlohmann::json& response, const nlohmann::json& request) const; @@ -167,11 +181,22 @@ class BridgeServer { // cleanup_mutex_ > last_sent_mutex_ > stats_mutex_ // SessionManager::mutex_ may be acquired while holding any of these. // Never acquire a higher-order lock while holding a lower-order one. + // topics_mutex_ is a LEAF lock: it is held only to compute the + // added/removed diff and swap in the new known_topics_ snapshot. Never + // call middleware_ or session_manager_ (or acquire any other lock in this + // class) while holding topics_mutex_ — release it before sending + // notifications. std::mutex cleanup_mutex_; // Per-client per-topic last-sent timestamp (nanoseconds) for rate limiting std::unordered_map> last_sent_times_; std::mutex last_sent_mutex_; + + // Whitelist-filtered topic name -> type snapshot, used by check_topic_changes() + // to detect additions/removals/type-changes. LEAF lock (see comment above). + std::unordered_map known_topics_; + bool topics_snapshot_taken_{false}; + std::mutex topics_mutex_; }; } // namespace pj_bridge diff --git a/app/include/pj_bridge/session_manager.hpp b/app/include/pj_bridge/session_manager.hpp index 6f70314..43516f6 100644 --- a/app/include/pj_bridge/session_manager.hpp +++ b/app/include/pj_bridge/session_manager.hpp @@ -39,6 +39,9 @@ struct Session { std::chrono::steady_clock::time_point last_heartbeat; std::chrono::steady_clock::time_point created_at; bool paused{false}; + /// True if this session opted in to server-initiated `topics_changed` + /// notifications via the `subscribe_topic_updates` command. + bool topic_updates{false}; }; class SessionManager { @@ -59,6 +62,8 @@ class SessionManager { bool session_exists(const std::string& client_id) const; bool set_paused(const std::string& client_id, bool paused); bool is_paused(const std::string& client_id) const; + bool set_topic_updates(const std::string& client_id, bool enabled); + bool wants_topic_updates(const std::string& client_id) const; bool set_ref_held(const std::string& client_id, const std::string& topic, bool held); std::unordered_set get_ref_held_topics(const std::string& client_id) const; diff --git a/app/include/pj_bridge/standalone_event_loop.hpp b/app/include/pj_bridge/standalone_event_loop.hpp index bd119fb..d4bef91 100644 --- a/app/include/pj_bridge/standalone_event_loop.hpp +++ b/app/include/pj_bridge/standalone_event_loop.hpp @@ -32,6 +32,8 @@ struct StandaloneConfig { double publish_rate; double session_timeout; bool stats_enabled; + /// Interval in seconds between check_topic_changes() polls. 0 disables polling. + double topic_poll_interval = 1.0; }; /// Runs the standalone event loop until SIGINT/SIGTERM. diff --git a/app/src/bridge_server.cpp b/app/src/bridge_server.cpp index d940ea8..3ca0dfc 100644 --- a/app/src/bridge_server.cpp +++ b/app/src/bridge_server.cpp @@ -192,6 +192,10 @@ void BridgeServer::process_single_request(const std::vector& request_da response = handle_pause(client_id, request_json); } else if (command == "resume") { response = handle_resume(client_id, request_json); + } else if (command == "subscribe_topic_updates") { + response = handle_subscribe_topic_updates(client_id, request_json); + } else if (command == "unsubscribe_topic_updates") { + response = handle_unsubscribe_topic_updates(client_id, request_json); } else { response = create_error_response("UNKNOWN_COMMAND", "Unknown command: " + command, request_json); } @@ -610,6 +614,43 @@ std::string BridgeServer::handle_resume(const std::string& client_id, const nloh return response.dump(); } +std::string BridgeServer::handle_subscribe_topic_updates(const std::string& client_id, const nlohmann::json& request) { + if (!session_manager_->session_exists(client_id)) { + session_manager_->create_session(client_id); + spdlog::info("Created new session for client '{}'", client_id); + } + + session_manager_->update_heartbeat(client_id); + session_manager_->set_topic_updates(client_id, true); + + spdlog::debug("Client '{}' subscribed to topic updates", client_id); + + json response; + response["status"] = "ok"; + response["topic_updates"] = true; + inject_response_fields(response, request); + return response.dump(); +} + +std::string BridgeServer::handle_unsubscribe_topic_updates( + const std::string& client_id, const nlohmann::json& request) { + if (!session_manager_->session_exists(client_id)) { + session_manager_->create_session(client_id); + spdlog::info("Created new session for client '{}'", client_id); + } + + session_manager_->update_heartbeat(client_id); + session_manager_->set_topic_updates(client_id, false); + + spdlog::debug("Client '{}' unsubscribed from topic updates", client_id); + + json response; + response["status"] = "ok"; + response["topic_updates"] = false; + inject_response_fields(response, request); + return response.dump(); +} + void BridgeServer::inject_response_fields(nlohmann::json& response, const nlohmann::json& request) const { response["protocol_version"] = kProtocolVersion; if (request.contains("id") && request["id"].is_string()) { @@ -641,6 +682,74 @@ void BridgeServer::check_session_timeouts() { } } +void BridgeServer::check_topic_changes() { + if (!initialized_) { + return; + } + + std::unordered_map current_topics; + for (const auto& topic : topic_source_->get_topics()) { + if (!whitelist_.matches(topic.name)) { + continue; + } + current_topics[topic.name] = topic.type; + } + + json added = json::array(); + json removed = json::array(); + + { + std::lock_guard lock(topics_mutex_); + + if (!topics_snapshot_taken_) { + known_topics_ = std::move(current_topics); + topics_snapshot_taken_ = true; + return; + } + + for (const auto& [name, type] : current_topics) { + auto it = known_topics_.find(name); + if (it == known_topics_.end() || it->second != type) { + json entry; + entry["name"] = name; + entry["type"] = type; + added.push_back(entry); + } + } + + for (const auto& [name, type] : known_topics_) { + auto it = current_topics.find(name); + if (it == current_topics.end() || it->second != type) { + removed.push_back(name); + } + } + + known_topics_ = std::move(current_topics); + } // topics_mutex_ released here — never send notifications while holding it + + if (added.empty() && removed.empty()) { + return; + } + + json notification; + notification["notification"] = "topics_changed"; + notification["added"] = added; + notification["removed"] = removed; + notification["protocol_version"] = kProtocolVersion; + + std::vector bytes; + { + std::string text = notification.dump(); + bytes.assign(text.begin(), text.end()); + } + + for (const auto& client_id : session_manager_->get_active_sessions()) { + if (session_manager_->wants_topic_updates(client_id)) { + middleware_->send_reply(client_id, bytes); + } + } +} + void BridgeServer::cleanup_session(const std::string& client_id) { std::lock_guard lock(cleanup_mutex_); diff --git a/app/src/session_manager.cpp b/app/src/session_manager.cpp index a8c7008..b9f24c1 100644 --- a/app/src/session_manager.cpp +++ b/app/src/session_manager.cpp @@ -222,4 +222,27 @@ bool SessionManager::is_paused(const std::string& client_id) const { return it->second.paused; } +bool SessionManager::set_topic_updates(const std::string& client_id, bool enabled) { + std::lock_guard lock(mutex_); + + auto it = sessions_.find(client_id); + if (it == sessions_.end()) { + return false; + } + + it->second.topic_updates = enabled; + return true; +} + +bool SessionManager::wants_topic_updates(const std::string& client_id) const { + std::lock_guard lock(mutex_); + + auto it = sessions_.find(client_id); + if (it == sessions_.end()) { + return false; + } + + return it->second.topic_updates; +} + } // namespace pj_bridge diff --git a/app/src/standalone_event_loop.cpp b/app/src/standalone_event_loop.cpp index 925b16e..76263a6 100644 --- a/app/src/standalone_event_loop.cpp +++ b/app/src/standalone_event_loop.cpp @@ -55,6 +55,7 @@ void run_standalone_event_loop( auto last_publish = clock::now(); auto last_timeout_check = clock::now(); auto last_stats_print = clock::now(); + auto last_topic_poll = clock::now(); while (!g_shutdown.load()) { server.process_requests(); @@ -71,6 +72,13 @@ void run_standalone_event_loop( last_timeout_check = now; } + if (config.topic_poll_interval > 0.0 && + now - last_topic_poll >= std::chrono::duration_cast( + std::chrono::duration(config.topic_poll_interval))) { + server.check_topic_changes(); + last_topic_poll = now; + } + if (config.stats_enabled && now - last_stats_print >= std::chrono::seconds(5)) { auto elapsed = std::chrono::duration(now - last_stats_print).count(); auto snapshot = server.snapshot_and_reset_stats(); diff --git a/docs/API.md b/docs/API.md index c3bee1e..e4d3ef9 100644 --- a/docs/API.md +++ b/docs/API.md @@ -256,6 +256,63 @@ If some subscribed topics are not currently available (publisher down) or fail t Both commands are idempotent. Smart ROS2 management: when all clients interested in a topic are paused, the ROS2 subscription is released. +## Pushed Topic Advertisement (topics_changed) + +Clients can opt in to be notified when the server's (whitelist-filtered) +topic set changes, instead of polling `get_topics`. + +**Subscribe Request:** +```json +{"command": "subscribe_topic_updates", "id": "tu1"} +``` + +**Subscribe Response:** +```json +{"status": "ok", "id": "tu1", "protocol_version": 1, "topic_updates": true} +``` + +**Unsubscribe Request:** +```json +{"command": "unsubscribe_topic_updates", "id": "tu2"} +``` + +**Unsubscribe Response:** +```json +{"status": "ok", "id": "tu2", "protocol_version": 1, "topic_updates": false} +``` + +Both commands are idempotent and create a session if the client does not +already have one (like `pause`/`resume`). + +The server periodically polls the topic graph (see `topic_poll_interval` +below) and, for every session currently opted in, sends a **notification** +(not a response to any request — it has no `id`) whenever the topic set has +changed since the last poll: + +```json +{"notification": "topics_changed", + "added": [{"name": "/t", "type": "pkg/msg/T"}], + "removed": ["/gone"], + "protocol_version": 1} +``` + +- `added` and `removed` may each be empty, but a notification is only sent + when at least one of them is non-empty. +- A topic whose type changes (same name, different type) is reported as both + removed and added. +- Only whitelisted topics (see [Topic Whitelist](#topic-whitelist)) are + considered — a non-whitelisted topic appearing or disappearing never + triggers a notification. +- The very first poll after the server starts never sends a notification (it + only establishes the initial snapshot); notifications begin from the + second poll onward. +- **Clients must tolerate unknown `notification` types** appearing in future + protocol versions and ignore ones they don't recognize. + +The poll interval is configurable: +- **ROS2**: double parameter `topic_poll_interval`, default `1.0` seconds. `0` disables polling entirely. +- **FastDDS / RTI**: CLI flag `--topic-poll-interval`, default `1.0` seconds. `0` disables polling entirely. + ## Heartbeat Clients must send a heartbeat at least once per second. The default timeout is 10 seconds. diff --git a/fastdds/src/main.cpp b/fastdds/src/main.cpp index afece4e..827e599 100644 --- a/fastdds/src/main.cpp +++ b/fastdds/src/main.cpp @@ -39,6 +39,7 @@ int main(int argc, char* argv[]) { std::string log_level = "info"; bool stats_enabled = false; std::vector topic_whitelist{".*"}; + double topic_poll_interval = 1.0; app.add_option("--domains,-d", domain_ids, "DDS domain IDs")->required()->expected(1, -1); app.add_option("--port,-p", port, "WebSocket port")->default_val(9090)->check(CLI::Range(1, 65535)); @@ -46,6 +47,10 @@ int main(int argc, char* argv[]) { app.add_option("--session-timeout", session_timeout, "Session timeout in seconds")->default_val(10.0); app.add_option("--log-level", log_level, "Log level (trace, debug, info, warn, error)")->default_val("info"); app.add_flag("--stats", stats_enabled, "Print statistics every 5 seconds"); + app.add_option( + "--topic-poll-interval", topic_poll_interval, + "Interval in seconds between topics_changed notification polls (0 disables)") + ->default_val(1.0); // Bound variable is already initialized to {".*"} (match everything); CLI11 // leaves it untouched if the flag is not passed, so no default_val() is // needed (and default_val() on a vector would round-trip through a @@ -62,6 +67,7 @@ int main(int argc, char* argv[]) { spdlog::info(" Publish rate: {:.1f} Hz", publish_rate); spdlog::info(" Session timeout: {:.1f} s", session_timeout); spdlog::info(" Topic whitelist: {}", fmt::join(topic_whitelist, ", ")); + spdlog::info(" Topic poll interval: {:.1f} s", topic_poll_interval); auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); if (!whitelist_result) { @@ -69,6 +75,11 @@ int main(int argc, char* argv[]) { return 1; } + if (topic_poll_interval < 0.0) { + spdlog::error("Invalid --topic-poll-interval: {:.1f} (must be >= 0; 0 disables polling)", topic_poll_interval); + return 1; + } + try { auto topic_source = std::make_shared(domain_ids); auto sub_manager = std::make_shared(*topic_source); @@ -79,7 +90,7 @@ int main(int argc, char* argv[]) { std::move(whitelist_result.value())); pj_bridge::run_standalone_event_loop( - server, sub_manager, middleware, {port, publish_rate, session_timeout, stats_enabled}); + server, sub_manager, middleware, {port, publish_rate, session_timeout, stats_enabled, topic_poll_interval}); } catch (const std::exception& e) { spdlog::error("Fatal error: {}", e.what()); return 1; diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index 6ac91d0..afd546f 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -46,6 +46,7 @@ int main(int argc, char** argv) { node->declare_parameter>("topic_whitelist", {".*"}); node->declare_parameter("min_qos_depth", 1); node->declare_parameter("max_qos_depth", 100); + node->declare_parameter("topic_poll_interval", 1.0); int port = node->get_parameter("port").as_int(); double publish_rate = node->get_parameter("publish_rate").as_double(); @@ -54,12 +55,22 @@ int main(int argc, char** argv) { std::vector topic_whitelist = node->get_parameter("topic_whitelist").as_string_array(); int64_t min_qos_depth = node->get_parameter("min_qos_depth").as_int(); int64_t max_qos_depth = node->get_parameter("max_qos_depth").as_int(); + double topic_poll_interval = node->get_parameter("topic_poll_interval").as_double(); RCLCPP_INFO( node->get_logger(), "Configuration: port=%d, publish_rate=%.1f Hz, session_timeout=%.1f s, strip_large_messages=%s, " - "min_qos_depth=%ld, max_qos_depth=%ld", - port, publish_rate, session_timeout, strip_large_messages ? "true" : "false", min_qos_depth, max_qos_depth); + "min_qos_depth=%ld, max_qos_depth=%ld, topic_poll_interval=%.1f s", + port, publish_rate, session_timeout, strip_large_messages ? "true" : "false", min_qos_depth, max_qos_depth, + topic_poll_interval); + + if (topic_poll_interval < 0.0) { + RCLCPP_ERROR( + node->get_logger(), "Invalid topic_poll_interval: %.1f (must be >= 0; 0 disables polling)", + topic_poll_interval); + rclcpp::shutdown(); + return 1; + } auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); if (!whitelist_result) { @@ -111,6 +122,14 @@ int main(int argc, char** argv) { auto timeout_timer = node->create_wall_timer(1s, [&server]() { server.check_session_timeouts(); }); + // topic_poll_interval == 0 disables the pushed topic-advertisement poll. + rclcpp::TimerBase::SharedPtr topic_poll_timer; + if (topic_poll_interval > 0.0) { + topic_poll_timer = node->create_wall_timer( + std::chrono::duration_cast(std::chrono::duration(topic_poll_interval)), + [&server]() { server.check_topic_changes(); }); + } + // Spin until shutdown. spin() blocks waiting for work and returns when // rclcpp::shutdown() runs (e.g. on SIGINT) — unlike spin_some() in a // loop, which returns immediately when idle and busy-spins a full core. @@ -125,6 +144,9 @@ int main(int argc, char** argv) { request_timer->cancel(); publish_timer->cancel(); timeout_timer->cancel(); + if (topic_poll_timer) { + topic_poll_timer->cancel(); + } executor.remove_node(node); // Clear the subscription manager callback before server destruction diff --git a/rti/src/main.cpp b/rti/src/main.cpp index b86d4f0..3a1ccd6 100644 --- a/rti/src/main.cpp +++ b/rti/src/main.cpp @@ -40,6 +40,7 @@ int main(int argc, char* argv[]) { std::string log_level = "info"; bool stats_enabled = false; std::vector topic_whitelist{".*"}; + double topic_poll_interval = 1.0; app.add_option("--domains,-d", domain_ids, "DDS domain IDs")->required()->expected(1, -1); app.add_option("--port,-p", port, "WebSocket port")->default_val(9090)->check(CLI::Range(1, 65535)); @@ -48,6 +49,10 @@ int main(int argc, char* argv[]) { app.add_option("--qos-profile", qos_profile, "QoS profile XML file path"); app.add_option("--log-level", log_level, "Log level (trace, debug, info, warn, error)")->default_val("info"); app.add_flag("--stats", stats_enabled, "Print statistics every 5 seconds"); + app.add_option( + "--topic-poll-interval", topic_poll_interval, + "Interval in seconds between topics_changed notification polls (0 disables)") + ->default_val(1.0); // Bound variable is already initialized to {".*"} (match everything); CLI11 // leaves it untouched if the flag is not passed, so no default_val() is // needed (and default_val() on a vector would round-trip through a @@ -67,6 +72,7 @@ int main(int argc, char* argv[]) { spdlog::info(" QoS profile: {}", qos_profile); } spdlog::info(" Topic whitelist: {}", fmt::join(topic_whitelist, ", ")); + spdlog::info(" Topic poll interval: {:.1f} s", topic_poll_interval); auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); if (!whitelist_result) { @@ -74,6 +80,11 @@ int main(int argc, char* argv[]) { return 1; } + if (topic_poll_interval < 0.0) { + spdlog::error("Invalid --topic-poll-interval: {:.1f} (must be >= 0; 0 disables polling)", topic_poll_interval); + return 1; + } + try { auto topic_source = std::make_shared(domain_ids, qos_profile); auto sub_manager = std::make_shared(*topic_source); @@ -84,7 +95,7 @@ int main(int argc, char* argv[]) { std::move(whitelist_result.value())); pj_bridge::run_standalone_event_loop( - server, sub_manager, middleware, {port, publish_rate, session_timeout, stats_enabled}); + server, sub_manager, middleware, {port, publish_rate, session_timeout, stats_enabled, topic_poll_interval}); } catch (const std::exception& e) { spdlog::error("Fatal error: {}", e.what()); return 1; diff --git a/tests/unit/test_bridge_server.cpp b/tests/unit/test_bridge_server.cpp index 06a8ba7..413e67d 100644 --- a/tests/unit/test_bridge_server.cpp +++ b/tests/unit/test_bridge_server.cpp @@ -157,6 +157,25 @@ class MockMiddleware : public MiddlewareInterface { binary_sends_.clear(); } + /// Return all replies sent to a given client, in send order (parsed JSON). + std::vector get_replies(const std::string& client_id) { + std::lock_guard lock(reply_mutex_); + std::vector result; + for (const auto& [cid, data] : replies_) { + if (cid == client_id) { + std::string text(data.begin(), data.end()); + result.push_back(json::parse(text, nullptr, false)); + } + } + return result; + } + + /// Discard all recorded replies (useful to isolate a subsequent action's output). + void clear_replies() { + std::lock_guard lock(reply_mutex_); + replies_.clear(); + } + private: bool ready_{false}; @@ -2095,3 +2114,230 @@ TEST_F(BridgeServerTest, SubscribeMixedWhitelistedAndNonWhitelistedTopicsPartial EXPECT_EQ(mock_sub_manager_->ref_count("/allowed/foo"), 1); EXPECT_EQ(mock_sub_manager_->ref_count("/blocked"), 0); } + +// --------------------------------------------------------------------------- +// Pushed topic advertisement (subscribe/unsubscribe_topic_updates, check_topic_changes) +// --------------------------------------------------------------------------- + +// SubscribeTopicUpdatesReturnsOkTrue +TEST_F(BridgeServerTest, SubscribeTopicUpdatesReturnsOkTrue) { + ASSERT_TRUE(server_->initialize()); + + json req; + req["command"] = "subscribe_topic_updates"; + req["id"] = "tu1"; + mock_->push_request("client_tu_1", req.dump()); + server_->process_requests(); + + json reply = mock_->pop_reply("client_tu_1"); + ASSERT_FALSE(reply.is_discarded()); + EXPECT_EQ(reply["status"], "ok"); + EXPECT_TRUE(reply["topic_updates"].get()); + EXPECT_EQ(reply["id"], "tu1"); + EXPECT_TRUE(reply.contains("protocol_version")); + + // Repeated call is idempotent — same result. + mock_->push_request("client_tu_1", req.dump()); + server_->process_requests(); + json reply2 = mock_->pop_reply("client_tu_1"); + ASSERT_FALSE(reply2.is_discarded()); + EXPECT_EQ(reply2["status"], "ok"); + EXPECT_TRUE(reply2["topic_updates"].get()); +} + +// UnsubscribeTopicUpdatesReturnsOkFalse +TEST_F(BridgeServerTest, UnsubscribeTopicUpdatesReturnsOkFalse) { + ASSERT_TRUE(server_->initialize()); + + json req; + req["command"] = "unsubscribe_topic_updates"; + req["id"] = "tu2"; + mock_->push_request("client_tu_2", req.dump()); + server_->process_requests(); + + json reply = mock_->pop_reply("client_tu_2"); + ASSERT_FALSE(reply.is_discarded()); + EXPECT_EQ(reply["status"], "ok"); + EXPECT_FALSE(reply["topic_updates"].get()); + EXPECT_EQ(reply["id"], "tu2"); + EXPECT_TRUE(reply.contains("protocol_version")); + + // Repeated call is idempotent — same result. + mock_->push_request("client_tu_2", req.dump()); + server_->process_requests(); + json reply2 = mock_->pop_reply("client_tu_2"); + ASSERT_FALSE(reply2.is_discarded()); + EXPECT_EQ(reply2["status"], "ok"); + EXPECT_FALSE(reply2["topic_updates"].get()); +} + +// CheckTopicChangesFirstCallSendsNothing +TEST_F(BridgeServerTest, CheckTopicChangesFirstCallSendsNothing) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/a", "std_msgs/msg/String"}}); + + json sub; + sub["command"] = "subscribe_topic_updates"; + mock_->push_request("client_first", sub.dump()); + server_->process_requests(); + mock_->clear_replies(); + + server_->check_topic_changes(); + + auto replies = mock_->get_replies("client_first"); + EXPECT_TRUE(replies.empty()); +} + +// CheckTopicChangesNotifiesAddedTopicOnlyToOptedInClient +TEST_F(BridgeServerTest, CheckTopicChangesNotifiesAddedTopicOnlyToOptedInClient) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/a", "std_msgs/msg/String"}}); + + json sub; + sub["command"] = "subscribe_topic_updates"; + mock_->push_request("client_opted_in", sub.dump()); + server_->process_requests(); + + json hb; + hb["command"] = "heartbeat"; + mock_->push_request("client_plain", hb.dump()); + server_->process_requests(); + + server_->check_topic_changes(); // first call: snapshot only, sends nothing + mock_->clear_replies(); + + mock_topic_source_->set_topics({{"/a", "std_msgs/msg/String"}, {"/b", "sensor_msgs/msg/Imu"}}); + server_->check_topic_changes(); + + auto opted_in_replies = mock_->get_replies("client_opted_in"); + ASSERT_EQ(opted_in_replies.size(), 1u); + json notif = opted_in_replies[0]; + EXPECT_EQ(notif["notification"], "topics_changed"); + EXPECT_TRUE(notif.contains("protocol_version")); + ASSERT_TRUE(notif["removed"].empty()); + ASSERT_EQ(notif["added"].size(), 1u); + EXPECT_EQ(notif["added"][0]["name"], "/b"); + EXPECT_EQ(notif["added"][0]["type"], "sensor_msgs/msg/Imu"); + + auto plain_replies = mock_->get_replies("client_plain"); + EXPECT_TRUE(plain_replies.empty()); +} + +// CheckTopicChangesNotifiesRemovedTopic +TEST_F(BridgeServerTest, CheckTopicChangesNotifiesRemovedTopic) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/a", "std_msgs/msg/String"}, {"/b", "std_msgs/msg/String"}}); + + json sub; + sub["command"] = "subscribe_topic_updates"; + mock_->push_request("client_opted_in", sub.dump()); + server_->process_requests(); + + server_->check_topic_changes(); // snapshot + mock_->clear_replies(); + + mock_topic_source_->remove_topic("/b"); + server_->check_topic_changes(); + + auto replies = mock_->get_replies("client_opted_in"); + ASSERT_EQ(replies.size(), 1u); + json notif = replies[0]; + ASSERT_TRUE(notif["added"].empty()); + ASSERT_EQ(notif["removed"].size(), 1u); + EXPECT_EQ(notif["removed"][0], "/b"); +} + +// CheckTopicChangesNoChangeSendsNothing +TEST_F(BridgeServerTest, CheckTopicChangesNoChangeSendsNothing) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/a", "std_msgs/msg/String"}}); + + json sub; + sub["command"] = "subscribe_topic_updates"; + mock_->push_request("client_opted_in", sub.dump()); + server_->process_requests(); + + server_->check_topic_changes(); // snapshot + mock_->clear_replies(); + + server_->check_topic_changes(); // nothing changed + + auto replies = mock_->get_replies("client_opted_in"); + EXPECT_TRUE(replies.empty()); +} + +// CheckTopicChangesIgnoresNonWhitelistedTopics +TEST_F(BridgeServerTest, CheckTopicChangesIgnoresNonWhitelistedTopics) { + auto whitelist_result = WhitelistFilter::create({"/allowed.*"}); + ASSERT_TRUE(whitelist_result.has_value()); + server_ = std::make_unique( + mock_topic_source_, mock_sub_manager_, mock_, 19999, 10.0, 50.0, whitelist_result.value()); + ASSERT_TRUE(server_->initialize()); + + mock_topic_source_->set_topics({{"/allowed/foo", "std_msgs/msg/String"}}); + + json sub; + sub["command"] = "subscribe_topic_updates"; + mock_->push_request("client_wl", sub.dump()); + server_->process_requests(); + + server_->check_topic_changes(); // snapshot + mock_->clear_replies(); + + mock_topic_source_->set_topics({{"/allowed/foo", "std_msgs/msg/String"}, {"/blocked", "std_msgs/msg/String"}}); + server_->check_topic_changes(); + + auto replies = mock_->get_replies("client_wl"); + EXPECT_TRUE(replies.empty()); +} + +// CheckTopicChangesTypeChangeAppearsInBothAddedAndRemoved +TEST_F(BridgeServerTest, CheckTopicChangesTypeChangeAppearsInBothAddedAndRemoved) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/a", "std_msgs/msg/String"}}); + + json sub; + sub["command"] = "subscribe_topic_updates"; + mock_->push_request("client_opted_in", sub.dump()); + server_->process_requests(); + + server_->check_topic_changes(); // snapshot + mock_->clear_replies(); + + mock_topic_source_->set_topics({{"/a", "std_msgs/msg/Header"}}); + server_->check_topic_changes(); + + auto replies = mock_->get_replies("client_opted_in"); + ASSERT_EQ(replies.size(), 1u); + json notif = replies[0]; + ASSERT_EQ(notif["removed"].size(), 1u); + EXPECT_EQ(notif["removed"][0], "/a"); + ASSERT_EQ(notif["added"].size(), 1u); + EXPECT_EQ(notif["added"][0]["name"], "/a"); + EXPECT_EQ(notif["added"][0]["type"], "std_msgs/msg/Header"); +} + +// UnsubscribeTopicUpdatesStopsNotifications +TEST_F(BridgeServerTest, UnsubscribeTopicUpdatesStopsNotifications) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/a", "std_msgs/msg/String"}}); + + json sub; + sub["command"] = "subscribe_topic_updates"; + mock_->push_request("client_x", sub.dump()); + server_->process_requests(); + + server_->check_topic_changes(); // snapshot + + json unsub; + unsub["command"] = "unsubscribe_topic_updates"; + mock_->push_request("client_x", unsub.dump()); + server_->process_requests(); + mock_->clear_replies(); + + mock_topic_source_->set_topics({{"/a", "std_msgs/msg/String"}, {"/b", "std_msgs/msg/String"}}); + server_->check_topic_changes(); + + auto replies = mock_->get_replies("client_x"); + EXPECT_TRUE(replies.empty()); +} diff --git a/tests/unit/test_session_manager.cpp b/tests/unit/test_session_manager.cpp index a27c537..43e4f4d 100644 --- a/tests/unit/test_session_manager.cpp +++ b/tests/unit/test_session_manager.cpp @@ -324,6 +324,30 @@ TEST_F(SessionManagerTest, PausedStatePreservedAcrossHeartbeats) { EXPECT_TRUE(manager_.is_paused("client1")); } +TEST_F(SessionManagerTest, SessionStartsWithTopicUpdatesDisabled) { + EXPECT_TRUE(manager_.create_session("client1")); + + EXPECT_FALSE(manager_.wants_topic_updates("client1")); +} + +TEST_F(SessionManagerTest, SetTopicUpdatesChangesState) { + EXPECT_TRUE(manager_.create_session("client1")); + + EXPECT_TRUE(manager_.set_topic_updates("client1", true)); + EXPECT_TRUE(manager_.wants_topic_updates("client1")); + + EXPECT_TRUE(manager_.set_topic_updates("client1", false)); + EXPECT_FALSE(manager_.wants_topic_updates("client1")); +} + +TEST_F(SessionManagerTest, SetTopicUpdatesReturnsFalseForNonexistentSession) { + EXPECT_FALSE(manager_.set_topic_updates("nonexistent", true)); +} + +TEST_F(SessionManagerTest, WantsTopicUpdatesReturnsFalseForNonexistentSession) { + EXPECT_FALSE(manager_.wants_topic_updates("nonexistent")); +} + TEST_F(SessionManagerTest, ThreadSafety) { SessionManager thread_manager(10.0); constexpr int kNumThreads = 10; From c659d328c7debfebcc62abf8dd6a8273752142d2 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 14:53:20 +0200 Subject: [PATCH 07/16] refactor: unify QoS depth rule with per-publisher fallback and saturating sum Replace the total==0 special case with a uniform per-publisher rule: a publisher reporting depth 0 (RMW didn't propagate depth, or KEEP_ALL) contributes the fallback of 100 instead of 0, so unknown depths never shrink the queue and mixed known/unknown topics aggregate sensibly. Make the sum saturating (each contribution capped at max_qos_depth, running total clamped to max_qos_depth) so it can never wrap size_t. Tests: add a mixed KEEP_ALL + depth-10 publisher case, and a runtime RMW probe that skips the depth-assertion tests when the active RMW doesn't propagate publisher history depths through discovery (protects against a harness overriding the forced CycloneDDS selection). Co-Authored-By: Claude Fable 5 --- docs/API.md | 12 +-- ros2/src/generic_subscription_manager.cpp | 30 ++++--- .../test_generic_subscription_manager.cpp | 86 +++++++++++++++---- 3 files changed, 93 insertions(+), 35 deletions(-) diff --git a/docs/API.md b/docs/API.md index e4d3ef9..b7662b6 100644 --- a/docs/API.md +++ b/docs/API.md @@ -123,11 +123,13 @@ queue), then clamping the total to a configurable range — the same heuristic - **ROS2**: int parameters `min_qos_depth` (default `1`) and `max_qos_depth` (default `100`). -If no publishers are discovered yet — or every discovered publisher reports -depth `0` (KEEP_ALL, or an RMW such as `rmw_fastrtps_cpp` that does not -propagate history depth through discovery) — the depth falls back to -`min(100, max_qos_depth)`. Both values must be `>= 0` and `min_qos_depth <= -max_qos_depth`; the server refuses to start otherwise. This is independent of +A publisher that reports depth `0` (KEEP_ALL, or an RMW such as +`rmw_fastrtps_cpp` that does not propagate history depth through discovery) +counts as `100` — the historical default — rather than `0`, so unknown +depths never shrink the queue. If no publishers are discovered yet, the +depth likewise defaults to `min(100, max_qos_depth)`. Both values must be +`>= 0` and `min_qos_depth <= max_qos_depth`; the server refuses to start +otherwise. This is independent of the subscription's reliability/durability, which is separately adapted to match what the discovered publishers offer (a RELIABLE subscription still switches to BEST_EFFORT if any publisher is BEST_EFFORT, and to diff --git a/ros2/src/generic_subscription_manager.cpp b/ros2/src/generic_subscription_manager.cpp index 6943076..3723f2f 100644 --- a/ros2/src/generic_subscription_manager.cpp +++ b/ros2/src/generic_subscription_manager.cpp @@ -35,9 +35,14 @@ rclcpp::QoS GenericSubscriptionManager::adapt_qos(const std::string& topic_name) // topics!) and a VOLATILE one misses latched (TRANSIENT_LOCAL) samples. rclcpp::QoS qos(100); + // Historical default depth, also used as the per-publisher fallback when a + // publisher's depth is unknown (see below). + constexpr size_t kFallbackDepth = 100; + auto publishers = node_->get_publishers_info_by_topic(topic_name); if (publishers.empty()) { - qos.keep_last(std::min(100, max_qos_depth_)); + // No info yet → same fallback the per-publisher rule below uses. + qos.keep_last(std::min(kFallbackDepth, max_qos_depth_)); return qos; } @@ -47,6 +52,16 @@ rclcpp::QoS GenericSubscriptionManager::adapt_qos(const std::string& topic_name) // Copyright (c) Foxglove Technologies Inc): // sum the publishers' history depths so a burst from every publisher // still fits, then clamp to [min_qos_depth, max_qos_depth]. + // + // Deviations from foxglove_bridge: + // - A publisher reporting depth 0 means the RMW didn't propagate depth + // through discovery (e.g. rmw_fastrtps_cpp reports 0 for everyone) or + // the publisher uses KEEP_ALL. It contributes kFallbackDepth instead of + // 0 — assuming the historical generous default is safer than shrinking + // the queue and dropping messages on high-rate topics. + // - The sum is saturating: each contribution is capped at max_qos_depth_ + // and the running total is clamped to max_qos_depth_, so it can never + // wrap size_t no matter how many publishers report huge depths. size_t total_depth = 0; for (const auto& info : publishers) { const auto& profile = info.qos_profile(); @@ -56,17 +71,10 @@ rclcpp::QoS GenericSubscriptionManager::adapt_qos(const std::string& topic_name) if (profile.durability() != rclcpp::DurabilityPolicy::TransientLocal) { all_transient_local = false; } - total_depth += profile.depth(); + const size_t publisher_depth = profile.depth() > 0 ? profile.depth() : kFallbackDepth; + total_depth = std::min(total_depth + std::min(publisher_depth, max_qos_depth_), max_qos_depth_); } - // Some RMWs (e.g. rmw_fastrtps_cpp) don't propagate publisher history - // depth through discovery and report 0 for every publisher; KEEP_ALL - // publishers also report 0. A total of 0 therefore means "unknown", not - // "no queue needed" — fall back to the historical default of 100 instead - // of clamping 0 up to min_qos_depth, which would create tiny queues that - // drop messages on high-rate topics. - const size_t depth = - total_depth > 0 ? std::clamp(total_depth, min_qos_depth_, max_qos_depth_) : std::min(100, max_qos_depth_); - qos.keep_last(depth); + qos.keep_last(std::clamp(total_depth, min_qos_depth_, max_qos_depth_)); // BEST_EFFORT matches both kinds of publisher; TRANSIENT_LOCAL only // matches if every publisher offers it. diff --git a/tests/unit/test_generic_subscription_manager.cpp b/tests/unit/test_generic_subscription_manager.cpp index 7029089..8eab40e 100644 --- a/tests/unit/test_generic_subscription_manager.cpp +++ b/tests/unit/test_generic_subscription_manager.cpp @@ -56,6 +56,20 @@ struct ForceCycloneDdsForQosDepthTests { const ForceCycloneDdsForQosDepthTests force_cyclonedds_for_qos_depth_tests; } // namespace +namespace { +// Publisher discovery is asynchronous even within a single process, so tests +// must poll the graph rather than assume a freshly created publisher is +// immediately visible via get_publishers_info_by_topic(). +void wait_for_publisher_count( + const rclcpp::Node::SharedPtr& node, const std::string& topic_name, size_t expected_count) { + auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (node->get_publishers_info_by_topic(topic_name).size() < expected_count && + std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } +} +} // namespace + class GenericSubscriptionManagerTest : public ::testing::Test { protected: void SetUp() override { @@ -70,24 +84,23 @@ class GenericSubscriptionManagerTest : public ::testing::Test { rclcpp::shutdown(); } + // Runtime probe: returns true if the active RMW propagates publisher + // history depth through discovery. The static initializer above forces + // CycloneDDS (which does), but a harness exporting its own + // RMW_IMPLEMENTATION overrides it — in that case the depth-assertion + // tests must be skipped rather than fail on discovery values the RMW + // simply never reports. + bool rmw_propagates_depth() { + auto probe = node_->create_publisher("/qos_depth_probe_topic", rclcpp::QoS(7)); + wait_for_publisher_count(node_, "/qos_depth_probe_topic", 1); + auto infos = node_->get_publishers_info_by_topic("/qos_depth_probe_topic"); + return !infos.empty() && infos[0].qos_profile().depth() == 7; + } + rclcpp::Node::SharedPtr node_; std::unique_ptr manager_; }; -namespace { -// Publisher discovery is asynchronous even within a single process, so tests -// must poll the graph rather than assume a freshly created publisher is -// immediately visible via get_publishers_info_by_topic(). -void wait_for_publisher_count( - const rclcpp::Node::SharedPtr& node, const std::string& topic_name, size_t expected_count) { - auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); - while (node->get_publishers_info_by_topic(topic_name).size() < expected_count && - std::chrono::steady_clock::now() < deadline) { - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - } -} -} // namespace - TEST_F(GenericSubscriptionManagerTest, Subscribe) { bool callback_called = false; auto callback = [&callback_called](const std::string&, const std::shared_ptr&, uint64_t) { @@ -296,13 +309,29 @@ TEST_F(GenericSubscriptionManagerTest, AdaptsQosToBestEffortPublisher) { // QoS depth aggregation // // Depth aggregation adapted from foxglove_bridge (MIT License, Copyright (c) -// Foxglove Technologies Inc): sum the publishers' history depths so a burst -// from every publisher still fits, then clamp to [min_qos_depth, max_qos_depth]. +// Foxglove Technologies Inc): each publisher contributes its history depth +// (or the fallback of 100 when it reports 0 — unknown/KEEP_ALL), the sum is +// saturating (each contribution capped at max_qos_depth, running total +// clamped to max_qos_depth), and the result is clamped to +// [min_qos_depth, max_qos_depth]. +// +// The size_t-overflow case is not practically constructible with real +// publishers; it is covered by the saturation logic itself (the +// per-contribution cap makes wrap impossible), so no test exists for it. +// // (See the ForceCycloneDdsForQosDepthTests note above for why this test -// binary forces CycloneDDS.) +// binary forces CycloneDDS. Each depth-assertion test additionally probes +// the active RMW at runtime and skips if it doesn't propagate depths.) // --------------------------------------------------------------------------- +#define SKIP_IF_RMW_HIDES_DEPTH() \ + if (!rmw_propagates_depth()) { \ + GTEST_SKIP() << "Active RMW does not propagate publisher history depth through " \ + "discovery; depth aggregation cannot be observed with real values."; \ + } + TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthMatchesSinglePublisher) { + SKIP_IF_RMW_HIDES_DEPTH(); auto publisher = node_->create_publisher("/qos_depth_topic1", rclcpp::QoS(10)); wait_for_publisher_count(node_, "/qos_depth_topic1", 1); @@ -312,6 +341,7 @@ TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthMatchesSinglePublisher) { } TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthSumsAcrossPublishersClampedToMax) { + SKIP_IF_RMW_HIDES_DEPTH(); auto publisher1 = node_->create_publisher("/qos_depth_topic2", rclcpp::QoS(60)); auto publisher2 = node_->create_publisher("/qos_depth_topic2", rclcpp::QoS(60)); wait_for_publisher_count(node_, "/qos_depth_topic2", 2); @@ -323,6 +353,7 @@ TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthSumsAcrossPublishersClampedT } TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthClampedToConfiguredMinimum) { + SKIP_IF_RMW_HIDES_DEPTH(); auto manager_with_min = std::make_unique(node_, /*min_qos_depth=*/30); auto publisher = node_->create_publisher("/qos_depth_topic3", rclcpp::QoS(5)); wait_for_publisher_count(node_, "/qos_depth_topic3", 1); @@ -333,16 +364,18 @@ TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthClampedToConfiguredMinimum) } TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthDefaultsTo100WhenNoPublishers) { + // RMW-independent: no publishers are involved, so no skip guard needed. rclcpp::QoS qos = manager_->adapt_qos("/qos_depth_no_publishers"); EXPECT_EQ(qos.depth(), 100u); } TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthFallsBackTo100WhenDepthUnknown) { + SKIP_IF_RMW_HIDES_DEPTH(); // A KEEP_ALL publisher reports history depth 0 through discovery — the // same value RMWs that don't propagate depth at all (e.g. rmw_fastrtps_cpp) - // report for every publisher. A total of 0 must be treated as "unknown" - // and fall back to the historical default of 100, not clamped up to + // report for every publisher. Depth 0 means "unknown", so the publisher + // contributes the fallback of 100 instead of 0 — not clamped up to // min_qos_depth (a depth-1 queue would drop messages on high-rate topics). auto publisher = node_->create_publisher("/qos_depth_keep_all_topic", rclcpp::QoS(rclcpp::KeepAll())); @@ -355,3 +388,18 @@ TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthFallsBackTo100WhenDepthUnkno EXPECT_EQ(qos.depth(), 100u); } + +TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthMixedKnownAndUnknownPublishers) { + SKIP_IF_RMW_HIDES_DEPTH(); + // One KEEP_ALL publisher (reports 0 → contributes the fallback of 100) + // plus one depth-10 publisher: 100 + 10 saturates at the default + // max_qos_depth of 100. + auto publisher_keep_all = + node_->create_publisher("/qos_depth_mixed_topic", rclcpp::QoS(rclcpp::KeepAll())); + auto publisher_depth_10 = node_->create_publisher("/qos_depth_mixed_topic", rclcpp::QoS(10)); + wait_for_publisher_count(node_, "/qos_depth_mixed_topic", 2); + + rclcpp::QoS qos = manager_->adapt_qos("/qos_depth_mixed_topic"); + + EXPECT_EQ(qos.depth(), 100u); +} From e26c1ece99232ee4d74d60190ef7cf00ec651074 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 14:55:52 +0200 Subject: [PATCH 08/16] fix: take topic baseline snapshot eagerly at startup Topics appearing between server initialization and the first topic_poll_interval tick were folded into the silent baseline and never notified. Snapshot immediately after initialize() instead, before any client can connect. (Codex review finding on 866371d.) Co-Authored-By: Claude Fable 5 --- app/src/standalone_event_loop.cpp | 7 +++++++ ros2/src/main.cpp | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/app/src/standalone_event_loop.cpp b/app/src/standalone_event_loop.cpp index 76263a6..70db740 100644 --- a/app/src/standalone_event_loop.cpp +++ b/app/src/standalone_event_loop.cpp @@ -49,6 +49,13 @@ void run_standalone_event_loop( spdlog::info("Bridge server initialized, entering event loop"); + if (config.topic_poll_interval > 0.0) { + // Take the silent baseline snapshot now, before any client can connect, + // so topics appearing before the first poll tick are notified rather + // than folded into the baseline. + server.check_topic_changes(); + } + using clock = std::chrono::steady_clock; auto publish_interval = std::chrono::duration_cast(std::chrono::duration(1.0 / config.publish_rate)); diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index afd546f..e9b69c4 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -125,6 +125,10 @@ int main(int argc, char** argv) { // topic_poll_interval == 0 disables the pushed topic-advertisement poll. rclcpp::TimerBase::SharedPtr topic_poll_timer; if (topic_poll_interval > 0.0) { + // Take the silent baseline snapshot now, before any client can connect, + // so topics appearing before the first timer tick are notified rather + // than folded into the baseline. + server.check_topic_changes(); topic_poll_timer = node->create_wall_timer( std::chrono::duration_cast(std::chrono::duration(topic_poll_interval)), [&server]() { server.check_topic_changes(); }); From a3d90c507c7b46cdcf7b8b3c66986b7ba6675587 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 15:07:30 +0200 Subject: [PATCH 09/16] feat: bounded per-client send queue with drop-oldest backpressure Co-Authored-By: Claude Fable 5 --- CMakeLists.txt | 1 + .../middleware/bounded_frame_queue.hpp | 93 +++++++++++++++ .../middleware/websocket_middleware.hpp | 31 ++++- app/src/middleware/websocket_middleware.cpp | 112 +++++++++++++++++- app/src/standalone_event_loop.cpp | 1 + docs/API.md | 24 ++++ fastdds/src/main.cpp | 9 +- ros2/src/main.cpp | 14 ++- rti/src/main.cpp | 9 +- tests/unit/test_bounded_frame_queue.cpp | 98 +++++++++++++++ tests/unit/test_websocket_middleware.cpp | 52 ++++++++ 11 files changed, 434 insertions(+), 10 deletions(-) create mode 100644 app/include/pj_bridge/middleware/bounded_frame_queue.hpp create mode 100644 tests/unit/test_bounded_frame_queue.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index df55f42..b86a757 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -242,6 +242,7 @@ if(BUILD_TESTING AND ament_cmake_FOUND) ament_add_gtest(${PROJECT_NAME}_tests tests/unit/test_websocket_middleware.cpp + tests/unit/test_bounded_frame_queue.cpp tests/unit/test_message_buffer.cpp tests/unit/test_session_manager.cpp tests/unit/test_message_serializer.cpp diff --git a/app/include/pj_bridge/middleware/bounded_frame_queue.hpp b/app/include/pj_bridge/middleware/bounded_frame_queue.hpp new file mode 100644 index 0000000..aa8b657 --- /dev/null +++ b/app/include/pj_bridge/middleware/bounded_frame_queue.hpp @@ -0,0 +1,93 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#pragma once + +#include +#include +#include +#include +#include + +namespace pj_bridge { + +/// A FIFO queue of binary frames bounded to at most `max_frames` entries. +/// +/// When a push would exceed the capacity, the OLDEST queued frame is dropped +/// to make room for the new one (never the newest), matching foxglove_bridge's +/// slow-client backpressure policy: recent data is prioritized over stale +/// backlog for interactive plotting/visualization use cases. +/// +/// NOT thread-safe by design: this is a plain data structure. Callers are +/// expected to hold their own lock (e.g. WebSocketMiddleware's +/// `clients_mutex_`) around every call. +class BoundedFrameQueue { + public: + explicit BoundedFrameQueue(size_t max_frames) : max_frames_(max_frames) {} + + /// Pushes a frame, dropping the oldest queued frame first if the queue is + /// already at capacity. Returns the number of frames dropped as a result of + /// this call (0 or 1). A queue with `max_frames == 0` drops every push + /// immediately and never grows. + size_t push(std::vector frame) { + if (max_frames_ == 0) { + ++dropped_total_; + return 1; + } + + size_t dropped = 0; + if (frames_.size() >= max_frames_) { + frames_.pop_front(); + ++dropped_total_; + dropped = 1; + } + frames_.push_back(std::move(frame)); + return dropped; + } + + /// Removes and returns the oldest frame, or nullopt if the queue is empty. + std::optional> pop_front() { + if (frames_.empty()) { + return std::nullopt; + } + std::optional> front = std::move(frames_.front()); + frames_.pop_front(); + return front; + } + + bool empty() const { + return frames_.empty(); + } + + size_t size() const { + return frames_.size(); + } + + /// Cumulative number of frames dropped over the lifetime of this queue. + uint64_t dropped_total() const { + return dropped_total_; + } + + private: + size_t max_frames_; + std::deque> frames_; + uint64_t dropped_total_{0}; +}; + +} // namespace pj_bridge diff --git a/app/include/pj_bridge/middleware/websocket_middleware.hpp b/app/include/pj_bridge/middleware/websocket_middleware.hpp index 5cf96a6..9e42c81 100644 --- a/app/include/pj_bridge/middleware/websocket_middleware.hpp +++ b/app/include/pj_bridge/middleware/websocket_middleware.hpp @@ -22,6 +22,8 @@ #include #include +#include +#include #include #include #include @@ -30,13 +32,14 @@ #include #include +#include "pj_bridge/middleware/bounded_frame_queue.hpp" #include "pj_bridge/middleware/middleware_interface.hpp" namespace pj_bridge { class WebSocketMiddleware : public MiddlewareInterface { public: - WebSocketMiddleware(); + explicit WebSocketMiddleware(size_t client_backlog_size = 100); ~WebSocketMiddleware() override; WebSocketMiddleware(const WebSocketMiddleware&) = delete; @@ -54,6 +57,10 @@ class WebSocketMiddleware : public MiddlewareInterface { void set_on_connect(ConnectionCallback callback) override; void set_on_disconnect(ConnectionCallback callback) override; + /// Total number of frames dropped due to slow-client backpressure, summed + /// across all clients (currently connected and already disconnected). + uint64_t dropped_frame_count() const; + private: struct IncomingRequest { std::string client_id; @@ -66,8 +73,23 @@ class WebSocketMiddleware : public MiddlewareInterface { mutable std::mutex queue_mutex_; std::unordered_map> clients_; + // Per-client backlog of frames withheld while the client's socket buffer is + // over the high watermark, and the steady-clock time each client's drop + // warning was last logged. Both are guarded by `clients_mutex_` (the same + // lock as `clients_`), not a separate mutex, so client lifecycle and + // pending-queue lifecycle stay consistent with each other. + std::unordered_map pending_frames_; + std::unordered_map last_drop_warn_; mutable std::mutex clients_mutex_; + // Sum of dropped_total() from clients that have since disconnected (their + // BoundedFrameQueue entry is erased from pending_frames_ on disconnect, so + // its count is folded in here to keep dropped_frame_count() a lifetime + // total). Guarded by clients_mutex_. + uint64_t dropped_from_disconnected_{0}; + + size_t client_backlog_size_; + ConnectionCallback on_connect_; ConnectionCallback on_disconnect_; @@ -78,6 +100,13 @@ class WebSocketMiddleware : public MiddlewareInterface { static constexpr int kShutdownTimeoutSeconds = 3; static constexpr size_t kMaxIncomingQueueSize = 1024; + + // Lossy-send policy adapted from foxglove_bridge (MIT License, Copyright + // (c) Foxglove Technologies Inc): once a client's outgoing socket buffer + // exceeds this watermark, new frames are queued (dropping the oldest on + // overflow) instead of blocking or disconnecting the client. + static constexpr size_t kSocketBufferHighWatermark = 1u << 20; // 1 MiB + static constexpr int kDropWarnIntervalSeconds = 30; }; } // namespace pj_bridge diff --git a/app/src/middleware/websocket_middleware.cpp b/app/src/middleware/websocket_middleware.cpp index d8e325d..784ff43 100644 --- a/app/src/middleware/websocket_middleware.cpp +++ b/app/src/middleware/websocket_middleware.cpp @@ -22,11 +22,13 @@ #include #include +#include #include namespace pj_bridge { -WebSocketMiddleware::WebSocketMiddleware() : initialized_(false) {} +WebSocketMiddleware::WebSocketMiddleware(size_t client_backlog_size) + : client_backlog_size_(client_backlog_size), initialized_(false) {} WebSocketMiddleware::~WebSocketMiddleware() { shutdown(); @@ -103,6 +105,12 @@ tl::expected WebSocketMiddleware::initialize(uint16_t port) { { std::lock_guard clients_lock(clients_mutex_); clients_.erase(client_id); + auto pending_it = pending_frames_.find(client_id); + if (pending_it != pending_frames_.end()) { + dropped_from_disconnected_ += pending_it->second.dropped_total(); + pending_frames_.erase(pending_it); + } + last_drop_warn_.erase(client_id); } } else if (msg->type == ix::WebSocketMessageType::Message) { @@ -206,6 +214,11 @@ void WebSocketMiddleware::shutdown() { { std::lock_guard clients_lock(clients_mutex_); clients_.clear(); + for (const auto& entry : pending_frames_) { + dropped_from_disconnected_ += entry.second.dropped_total(); + } + pending_frames_.clear(); + last_drop_warn_.clear(); } { @@ -257,6 +270,21 @@ bool WebSocketMiddleware::send_reply(const std::string& client_identity, const s } bool WebSocketMiddleware::send_binary(const std::string& client_identity, const std::vector& data) { + // Lossy-send policy adapted from foxglove_bridge (MIT License, Copyright + // (c) Foxglove Technologies Inc): a slow client never blocks the publisher + // and is never disconnected for falling behind. Instead, once its socket's + // outgoing buffer crosses kSocketBufferHighWatermark, new frames are queued + // per-client (dropping the oldest queued frame on overflow) and flushed + // once the buffer drains below the watermark again. + // + // IMPORTANT: clients_mutex_ is only ever held to look up state (the client + // handle, pending_frames_, last_drop_warn_) — never while calling into + // ix::WebSocket (bufferedAmount()/sendBinary()). Those calls take + // IXWebSocket's own internal locks, and the Open-connection handler above + // takes clients_mutex_ from an IXWebSocket-owned thread that already holds + // some of those internal locks; holding clients_mutex_ across a socket call + // here would form a lock-order cycle with that path (observed as a TSAN + // lock-order-inversion during development). std::shared_ptr ws; { std::lock_guard lock(clients_mutex_); @@ -267,9 +295,85 @@ bool WebSocketMiddleware::send_binary(const std::string& client_identity, const ws = it->second; } - std::string binary_data(reinterpret_cast(data.data()), data.size()); - auto send_info = ws->sendBinary(binary_data); - return send_info.success; + if (ws->bufferedAmount() < kSocketBufferHighWatermark) { + // Flush any backlog first, in FIFO order, bounded to the number of + // frames present when this call started so a fast producer can't spin + // forever. Each frame is popped under the lock and sent outside it. + size_t to_flush = 0; + { + std::lock_guard lock(clients_mutex_); + auto pending_it = pending_frames_.find(client_identity); + if (pending_it != pending_frames_.end()) { + to_flush = pending_it->second.size(); + } + } + + for (size_t i = 0; i < to_flush; ++i) { + std::optional> frame; + { + std::lock_guard lock(clients_mutex_); + auto pending_it = pending_frames_.find(client_identity); + if (pending_it == pending_frames_.end()) { + break; + } + frame = pending_it->second.pop_front(); + } + if (!frame) { + break; + } + std::string frame_data(reinterpret_cast(frame->data()), frame->size()); + auto send_info = ws->sendBinary(frame_data); + if (!send_info.success) { + return false; + } + } + + std::string binary_data(reinterpret_cast(data.data()), data.size()); + auto send_info = ws->sendBinary(binary_data); + return send_info.success; + } + + // Socket buffer is over the high watermark: queue the frame instead of + // sending it now. + size_t dropped; + uint64_t dropped_total_now; + { + std::lock_guard lock(clients_mutex_); + auto [pending_it, inserted] = pending_frames_.try_emplace(client_identity, BoundedFrameQueue(client_backlog_size_)); + (void)inserted; + dropped = pending_it->second.push(data); + dropped_total_now = pending_it->second.dropped_total(); + } + + if (dropped > 0) { + auto now = std::chrono::steady_clock::now(); + bool should_warn; + { + std::lock_guard lock(clients_mutex_); + auto warn_it = last_drop_warn_.find(client_identity); + should_warn = + warn_it == last_drop_warn_.end() || now - warn_it->second >= std::chrono::seconds(kDropWarnIntervalSeconds); + if (should_warn) { + last_drop_warn_[client_identity] = now; + } + } + if (should_warn) { + spdlog::warn( + "Slow client '{}': dropping oldest queued frame(s) ({} dropped total)", client_identity, dropped_total_now); + } + } + + // The frame was accepted for later delivery, not a failure. + return true; +} + +uint64_t WebSocketMiddleware::dropped_frame_count() const { + std::lock_guard lock(clients_mutex_); + uint64_t total = dropped_from_disconnected_; + for (const auto& entry : pending_frames_) { + total += entry.second.dropped_total(); + } + return total; } bool WebSocketMiddleware::publish_data(const std::vector& data) { diff --git a/app/src/standalone_event_loop.cpp b/app/src/standalone_event_loop.cpp index 70db740..73d6d94 100644 --- a/app/src/standalone_event_loop.cpp +++ b/app/src/standalone_event_loop.cpp @@ -103,6 +103,7 @@ void run_standalone_event_loop( fmt::format("\n Publish frequency: {:.1f} Hz", static_cast(snapshot.publish_cycles) / elapsed); stats_msg += fmt::format( "\n Sent: {:.2f} MB/s", static_cast(snapshot.total_bytes_published) / elapsed / (1024.0 * 1024.0)); + stats_msg += fmt::format("\n Dropped frames (slow clients): {}", middleware->dropped_frame_count()); spdlog::info(stats_msg); last_stats_print = now; diff --git a/docs/API.md b/docs/API.md index b7662b6..2a778a4 100644 --- a/docs/API.md +++ b/docs/API.md @@ -345,6 +345,30 @@ All commands may return an error: Error codes: `INVALID_REQUEST`, `INVALID_JSON`, `UNKNOWN_COMMAND`, `ALL_SUBSCRIPTIONS_FAILED`, `INTERNAL_ERROR`. +## Slow clients / backpressure + +If a client can't keep up with the aggregated-message stream (e.g. a slow +network link or a busy renderer), the server never blocks the publish loop +waiting for it and never disconnects it for falling behind — matching +foxglove_bridge's slow-client policy. Instead, once a client's outgoing +socket buffer exceeds a 1 MiB high watermark, new binary frames destined for +that client are held in a small per-client queue rather than sent +immediately. If that queue is already full, the **oldest** queued frame is +dropped to make room for the newest one, so a lagging client always +eventually resumes with fresh data instead of a growing backlog of stale +frames. Queued frames are flushed, in order, as soon as the client's socket +buffer drains back below the watermark. The JSON control-plane (requests and +their replies, including `heartbeat`) is never affected — those messages are +always sent immediately. + +The queue depth (max frames held per client before the oldest is dropped) is +configurable: + +- **ROS2**: int parameter `client_backlog_size`, default `100`. Must be `> 0`; + the server refuses to start otherwise. +- **FastDDS / RTI**: CLI flag `--client-backlog-size`, default `100`, valid + range `1`-`1000000`. + ## Binary Message Format Binary frames consist of a fixed 16-byte header followed by ZSTD-compressed payload. diff --git a/fastdds/src/main.cpp b/fastdds/src/main.cpp index 827e599..3c549fc 100644 --- a/fastdds/src/main.cpp +++ b/fastdds/src/main.cpp @@ -40,6 +40,7 @@ int main(int argc, char* argv[]) { bool stats_enabled = false; std::vector topic_whitelist{".*"}; double topic_poll_interval = 1.0; + int client_backlog_size = 100; app.add_option("--domains,-d", domain_ids, "DDS domain IDs")->required()->expected(1, -1); app.add_option("--port,-p", port, "WebSocket port")->default_val(9090)->check(CLI::Range(1, 65535)); @@ -51,6 +52,11 @@ int main(int argc, char* argv[]) { "--topic-poll-interval", topic_poll_interval, "Interval in seconds between topics_changed notification polls (0 disables)") ->default_val(1.0); + app.add_option( + "--client-backlog-size", client_backlog_size, + "Max frames queued per slow client before dropping the oldest (backpressure)") + ->default_val(100) + ->check(CLI::Range(1, 1000000)); // Bound variable is already initialized to {".*"} (match everything); CLI11 // leaves it untouched if the flag is not passed, so no default_val() is // needed (and default_val() on a vector would round-trip through a @@ -68,6 +74,7 @@ int main(int argc, char* argv[]) { spdlog::info(" Session timeout: {:.1f} s", session_timeout); spdlog::info(" Topic whitelist: {}", fmt::join(topic_whitelist, ", ")); spdlog::info(" Topic poll interval: {:.1f} s", topic_poll_interval); + spdlog::info(" Client backlog size: {}", client_backlog_size); auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); if (!whitelist_result) { @@ -83,7 +90,7 @@ int main(int argc, char* argv[]) { try { auto topic_source = std::make_shared(domain_ids); auto sub_manager = std::make_shared(*topic_source); - auto middleware = std::make_shared(); + auto middleware = std::make_shared(static_cast(client_backlog_size)); pj_bridge::BridgeServer server( topic_source, sub_manager, middleware, port, session_timeout, publish_rate, diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index e9b69c4..48b4888 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -47,6 +47,7 @@ int main(int argc, char** argv) { node->declare_parameter("min_qos_depth", 1); node->declare_parameter("max_qos_depth", 100); node->declare_parameter("topic_poll_interval", 1.0); + node->declare_parameter("client_backlog_size", 100); int port = node->get_parameter("port").as_int(); double publish_rate = node->get_parameter("publish_rate").as_double(); @@ -56,13 +57,14 @@ int main(int argc, char** argv) { int64_t min_qos_depth = node->get_parameter("min_qos_depth").as_int(); int64_t max_qos_depth = node->get_parameter("max_qos_depth").as_int(); double topic_poll_interval = node->get_parameter("topic_poll_interval").as_double(); + int64_t client_backlog_size = node->get_parameter("client_backlog_size").as_int(); RCLCPP_INFO( node->get_logger(), "Configuration: port=%d, publish_rate=%.1f Hz, session_timeout=%.1f s, strip_large_messages=%s, " - "min_qos_depth=%ld, max_qos_depth=%ld, topic_poll_interval=%.1f s", + "min_qos_depth=%ld, max_qos_depth=%ld, topic_poll_interval=%.1f s, client_backlog_size=%ld", port, publish_rate, session_timeout, strip_large_messages ? "true" : "false", min_qos_depth, max_qos_depth, - topic_poll_interval); + topic_poll_interval, client_backlog_size); if (topic_poll_interval < 0.0) { RCLCPP_ERROR( @@ -72,6 +74,12 @@ int main(int argc, char** argv) { return 1; } + if (client_backlog_size <= 0) { + RCLCPP_ERROR(node->get_logger(), "Invalid client_backlog_size: %ld (must be > 0)", client_backlog_size); + rclcpp::shutdown(); + return 1; + } + auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); if (!whitelist_result) { RCLCPP_ERROR(node->get_logger(), "Invalid topic_whitelist: %s", whitelist_result.error().c_str()); @@ -94,7 +102,7 @@ int main(int argc, char** argv) { auto topic_source = std::make_shared(node); auto sub_manager = std::make_shared( node, strip_large_messages, static_cast(min_qos_depth), static_cast(max_qos_depth)); - auto middleware = std::make_shared(); + auto middleware = std::make_shared(static_cast(client_backlog_size)); // Create bridge server pj_bridge::BridgeServer server( diff --git a/rti/src/main.cpp b/rti/src/main.cpp index 3a1ccd6..dc3d61c 100644 --- a/rti/src/main.cpp +++ b/rti/src/main.cpp @@ -41,6 +41,7 @@ int main(int argc, char* argv[]) { bool stats_enabled = false; std::vector topic_whitelist{".*"}; double topic_poll_interval = 1.0; + int client_backlog_size = 100; app.add_option("--domains,-d", domain_ids, "DDS domain IDs")->required()->expected(1, -1); app.add_option("--port,-p", port, "WebSocket port")->default_val(9090)->check(CLI::Range(1, 65535)); @@ -53,6 +54,11 @@ int main(int argc, char* argv[]) { "--topic-poll-interval", topic_poll_interval, "Interval in seconds between topics_changed notification polls (0 disables)") ->default_val(1.0); + app.add_option( + "--client-backlog-size", client_backlog_size, + "Max frames queued per slow client before dropping the oldest (backpressure)") + ->default_val(100) + ->check(CLI::Range(1, 1000000)); // Bound variable is already initialized to {".*"} (match everything); CLI11 // leaves it untouched if the flag is not passed, so no default_val() is // needed (and default_val() on a vector would round-trip through a @@ -73,6 +79,7 @@ int main(int argc, char* argv[]) { } spdlog::info(" Topic whitelist: {}", fmt::join(topic_whitelist, ", ")); spdlog::info(" Topic poll interval: {:.1f} s", topic_poll_interval); + spdlog::info(" Client backlog size: {}", client_backlog_size); auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); if (!whitelist_result) { @@ -88,7 +95,7 @@ int main(int argc, char* argv[]) { try { auto topic_source = std::make_shared(domain_ids, qos_profile); auto sub_manager = std::make_shared(*topic_source); - auto middleware = std::make_shared(); + auto middleware = std::make_shared(static_cast(client_backlog_size)); pj_bridge::BridgeServer server( topic_source, sub_manager, middleware, port, session_timeout, publish_rate, diff --git a/tests/unit/test_bounded_frame_queue.cpp b/tests/unit/test_bounded_frame_queue.cpp new file mode 100644 index 0000000..da40c94 --- /dev/null +++ b/tests/unit/test_bounded_frame_queue.cpp @@ -0,0 +1,98 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#include "pj_bridge/middleware/bounded_frame_queue.hpp" + +#include + +#include + +using namespace pj_bridge; + +namespace { +std::vector make_frame(uint8_t value) { + return {value}; +} +} // namespace + +TEST(BoundedFrameQueueTest, PushUnderCapacityReturnsZeroAndGrows) { + BoundedFrameQueue queue(2); + + EXPECT_EQ(queue.push(make_frame(1)), 0u); + EXPECT_EQ(queue.size(), 1u); + EXPECT_FALSE(queue.empty()); + + EXPECT_EQ(queue.push(make_frame(2)), 0u); + EXPECT_EQ(queue.size(), 2u); +} + +TEST(BoundedFrameQueueTest, PushPastCapacityDropsOldest) { + BoundedFrameQueue queue(2); + + EXPECT_EQ(queue.push(make_frame(1)), 0u); + EXPECT_EQ(queue.push(make_frame(2)), 0u); + // Queue is now {1, 2}; pushing {3} should drop the OLDEST frame (1), not + // the newest, leaving {2, 3}. + EXPECT_EQ(queue.push(make_frame(3)), 1u); + EXPECT_EQ(queue.size(), 2u); + + auto first = queue.pop_front(); + ASSERT_TRUE(first.has_value()); + EXPECT_EQ(*first, make_frame(2)); + + auto second = queue.pop_front(); + ASSERT_TRUE(second.has_value()); + EXPECT_EQ(*second, make_frame(3)); + + EXPECT_TRUE(queue.empty()); +} + +TEST(BoundedFrameQueueTest, PopFrontOnEmptyReturnsNullopt) { + BoundedFrameQueue queue(2); + EXPECT_EQ(queue.pop_front(), std::nullopt); +} + +TEST(BoundedFrameQueueTest, DroppedTotalAccumulatesAcrossPushes) { + BoundedFrameQueue queue(1); + + EXPECT_EQ(queue.dropped_total(), 0u); + EXPECT_EQ(queue.push(make_frame(1)), 0u); + EXPECT_EQ(queue.dropped_total(), 0u); + + EXPECT_EQ(queue.push(make_frame(2)), 1u); + EXPECT_EQ(queue.dropped_total(), 1u); + + EXPECT_EQ(queue.push(make_frame(3)), 1u); + EXPECT_EQ(queue.dropped_total(), 2u); +} + +TEST(BoundedFrameQueueTest, ZeroCapacityDropsEveryPushAndStaysEmpty) { + BoundedFrameQueue queue(0); + + EXPECT_EQ(queue.push(make_frame(1)), 1u); + EXPECT_TRUE(queue.empty()); + EXPECT_EQ(queue.size(), 0u); + EXPECT_EQ(queue.dropped_total(), 1u); + + EXPECT_EQ(queue.push(make_frame(2)), 1u); + EXPECT_TRUE(queue.empty()); + EXPECT_EQ(queue.dropped_total(), 2u); + + EXPECT_EQ(queue.pop_front(), std::nullopt); +} diff --git a/tests/unit/test_websocket_middleware.cpp b/tests/unit/test_websocket_middleware.cpp index 407a7ad..4730e15 100644 --- a/tests/unit/test_websocket_middleware.cpp +++ b/tests/unit/test_websocket_middleware.cpp @@ -440,6 +440,58 @@ TEST_F(WebSocketMiddlewareTest, IncomingQueueBounded) { std::this_thread::sleep_for(std::chrono::milliseconds(100)); } +// --------------------------------------------------------------------------- +// Task 4 — Slow-client backpressure (bounded queue, drop-oldest) +// +// The drop-oldest policy itself is exercised by BoundedFrameQueue's own unit +// tests (test_bounded_frame_queue.cpp), since forcing a real socket's +// bufferedAmount() over the 1 MiB watermark is not realistically achievable +// in a fast unit test. These tests instead cover the parts that only make +// sense with a live WebSocketMiddleware: normal traffic never counts as +// dropped, and the constructor accepts an explicit backlog size. +// --------------------------------------------------------------------------- +TEST_F(WebSocketMiddlewareTest, DroppedFrameCountZeroAfterNormalTraffic) { + auto result = middleware_->initialize(18096); + ASSERT_TRUE(result.has_value()); + + EXPECT_EQ(middleware_->dropped_frame_count(), 0u); + + ix::WebSocket client; + client.setUrl("ws://127.0.0.1:18096"); + client.setOnMessageCallback([](const ix::WebSocketMessagePtr& /*msg*/) {}); + client.start(); + + ASSERT_TRUE(wait_for_client_open(client)) << "Client failed to connect"; + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + + client.send("register"); + std::vector data; + std::string client_id; + ASSERT_TRUE(poll_receive_request(*middleware_, data, client_id)); + ASSERT_FALSE(client_id.empty()); + + std::vector payload = {1, 2, 3, 4}; + for (int i = 0; i < 10; ++i) { + EXPECT_TRUE(middleware_->send_binary(client_id, payload)); + } + + EXPECT_EQ(middleware_->dropped_frame_count(), 0u); + + client.stop(); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); +} + +TEST(WebSocketMiddlewareBacklogConstructionTest, ExplicitBacklogSizeConstructsAndInitializes) { + WebSocketMiddleware middleware(5); + EXPECT_EQ(middleware.dropped_frame_count(), 0u); + + auto result = middleware.initialize(18097); + ASSERT_TRUE(result.has_value()); + EXPECT_TRUE(middleware.is_ready()); + + middleware.shutdown(); +} + int main(int argc, char** argv) { testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); From 37007b8e166e08394aca2535b7581333293406e0 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 15:21:17 +0200 Subject: [PATCH 10/16] feat: replay latest transient_local message to late subscribers (ROS2) Co-Authored-By: Claude Fable 5 --- app/include/pj_bridge/message_buffer.hpp | 22 +++ .../subscription_manager_interface.hpp | 7 + app/src/bridge_server.cpp | 40 ++++ app/src/message_buffer.cpp | 28 +++ docs/API.md | 27 +++ .../generic_subscription_manager.hpp | 7 + .../ros2_subscription_manager.hpp | 1 + ros2/src/generic_subscription_manager.cpp | 16 +- ros2/src/ros2_subscription_manager.cpp | 4 + tests/unit/test_bounded_frame_queue.cpp | 4 +- tests/unit/test_bridge_server.cpp | 182 ++++++++++++++++++ .../test_generic_subscription_manager.cpp | 32 +++ tests/unit/test_message_buffer.cpp | 76 ++++++++ 13 files changed, 442 insertions(+), 4 deletions(-) diff --git a/app/include/pj_bridge/message_buffer.hpp b/app/include/pj_bridge/message_buffer.hpp index 2ef2406..d1373a2 100644 --- a/app/include/pj_bridge/message_buffer.hpp +++ b/app/include/pj_bridge/message_buffer.hpp @@ -25,8 +25,10 @@ #include #include #include +#include #include #include +#include #include namespace pj_bridge { @@ -78,12 +80,32 @@ class MessageBuffer { /// Return the total number of messages across all topics. size_t size() const; + /// Mark (or unmark) @p topic_name as latched (transient_local). While a + /// topic is latched, add_message() additionally retains its newest sample + /// in a TTL-exempt, one-entry-per-topic store so that later subscribers to + /// an already-shared subscription can be replayed the current value even + /// after it has aged out of the normal buffer. Setting latched to false + /// erases the retained sample. + void set_latched(const std::string& topic_name, bool latched); + + /// Return the retained latched sample for @p topic_name, or nullopt if the + /// topic is not latched or no sample has been retained yet. + std::optional get_latched(const std::string& topic_name) const; + private: mutable std::mutex mutex_; std::unordered_map> topic_buffers_; uint64_t max_message_age_ns_; ClockFn clock_fn_; + // Latched (transient_local) topic support: topics currently flagged as + // latched, and the single newest retained sample per latched topic. + // Bounded at one message per latched topic; entries otherwise persist for + // the lifetime of the process (not TTL-cleaned, not drained by + // move_messages()). + std::unordered_set latched_topics_; + std::unordered_map latched_last_; + uint64_t now_ns() const; void cleanup_old_messages(); }; diff --git a/app/include/pj_bridge/subscription_manager_interface.hpp b/app/include/pj_bridge/subscription_manager_interface.hpp index d5780d2..b4631a0 100644 --- a/app/include/pj_bridge/subscription_manager_interface.hpp +++ b/app/include/pj_bridge/subscription_manager_interface.hpp @@ -74,6 +74,13 @@ class SubscriptionManagerInterface { /// Unsubscribe from all topics, destroying all underlying subscriptions. /// Called during shutdown. virtual void unsubscribe_all() = 0; + + /// True when every publisher of the topic offers TRANSIENT_LOCAL durability + /// (detected at subscribe time). Backends without this information return + /// false; the result is only meaningful while the topic is subscribed. + virtual bool is_transient_local(const std::string& /*topic_name*/) const { + return false; + } }; } // namespace pj_bridge diff --git a/app/src/bridge_server.cpp b/app/src/bridge_server.cpp index 3ca0dfc..4778e46 100644 --- a/app/src/bridge_server.cpp +++ b/app/src/bridge_server.cpp @@ -345,6 +345,12 @@ std::string BridgeServer::handle_subscribe(const std::string& client_id, const n extracted_schemas[topic_name] = std::move(schema); } + // Latched (transient_local) topics whose retained sample should be + // replayed to this client. Collected under cleanup_mutex_ below but sent + // only after the lock is released (never call middleware_ while holding + // cleanup_mutex_). + std::vector> replay_candidates; + // Acquire refs and record subscriptions atomically with respect to // pause/resume and cleanup_session (which run on other threads). // Invariant: a middleware ref is held iff set_ref_held(topic, true). @@ -384,6 +390,28 @@ std::string BridgeServer::handle_subscribe(const std::string& client_id, const n schema_obj["definition"] = extracted_schemas[topic_name]; schemas[topic_name] = schema_obj; + // Latched (transient_local) topic replay: the FIRST subscriber to a + // topic gets its retained sample delivered naturally by DDS + // (transient_local redelivery on the freshly created subscription). + // This path exists for LATER clients joining an already-shared + // subscription, whose sample may have aged out of MessageBuffer's + // normal TTL window by the time they subscribe. Only meaningful when + // we actually (re)established the middleware subscription this call + // (ref_acquired) — a paused "subscribe" creates no subscription to + // query. Ordering note: set_latched runs at subscribe time, so a + // latched topic's messages are only retained once someone has + // subscribed — exactly the shared-subscription case this replay + // serves. + if (ref_acquired) { + bool is_latched = subscription_manager_->is_transient_local(topic_name); + message_buffer_->set_latched(topic_name, is_latched); + if (is_latched) { + if (auto latched_msg = message_buffer_->get_latched(topic_name)) { + replay_candidates.emplace_back(topic_name, *latched_msg); + } + } + } + spdlog::info("Client '{}' subscribed to topic '{}' (type: {})", client_id, topic_name, topic_type); } @@ -396,6 +424,18 @@ std::string BridgeServer::handle_subscribe(const std::string& client_id, const n } } + // Replay retained latched samples to this client now that cleanup_mutex_ + // is released. Each candidate becomes its own single-message binary frame + // (mirrors the normal aggregated-frame format so clients need no special + // handling). Does not touch last_sent_times_: the sample's old timestamp + // means it won't interfere with subsequent rate limiting. + for (const auto& [topic_name, latched_msg] : replay_candidates) { + AggregatedMessageSerializer serializer; + serializer.serialize_message( + topic_name, latched_msg.timestamp_ns, latched_msg.data->data(), latched_msg.data->size()); + middleware_->send_binary(client_id, serializer.finalize()); + } + // Build response json response; if (failures.empty()) { diff --git a/app/src/message_buffer.cpp b/app/src/message_buffer.cpp index 653f80f..303c074 100644 --- a/app/src/message_buffer.cpp +++ b/app/src/message_buffer.cpp @@ -41,9 +41,35 @@ void MessageBuffer::add_message( msg.received_at_ns = now_ns(); msg.data = std::move(data); + if (latched_topics_.count(topic_name) > 0) { + // Retain a copy of the newest sample (shared_ptr copy — cheap, no deep + // copy of the payload) for latched-topic replay. This store is exempt + // from TTL cleanup and from move_messages(). + latched_last_[topic_name] = msg; + } + topic_buffers_[topic_name].push_back(std::move(msg)); } +void MessageBuffer::set_latched(const std::string& topic_name, bool latched) { + std::lock_guard lock(mutex_); + if (latched) { + latched_topics_.insert(topic_name); + } else { + latched_topics_.erase(topic_name); + latched_last_.erase(topic_name); + } +} + +std::optional MessageBuffer::get_latched(const std::string& topic_name) const { + std::lock_guard lock(mutex_); + auto it = latched_last_.find(topic_name); + if (it == latched_last_.end()) { + return std::nullopt; + } + return it->second; +} + void MessageBuffer::move_messages(std::unordered_map>& out_messages) { std::lock_guard lock(mutex_); out_messages = std::move(topic_buffers_); @@ -53,6 +79,8 @@ void MessageBuffer::move_messages(std::unordered_map lock(mutex_); topic_buffers_.clear(); + latched_topics_.clear(); + latched_last_.clear(); } size_t MessageBuffer::size() const { diff --git a/docs/API.md b/docs/API.md index 2a778a4..aa1de31 100644 --- a/docs/API.md +++ b/docs/API.md @@ -205,6 +205,33 @@ The `rate_limits` field is only present when at least one topic has a non-zero r } ``` +## Latched topics (transient_local) + +*ROS2 backend only, for now.* + +When a topic's publishers all offer `TRANSIENT_LOCAL` durability (e.g. +`/tf_static`, `/robot_description`), the bridge treats it as **latched**. A +brand-new subscription receives the publisher's retained sample directly +from DDS, but the bridge's underlying middleware subscription is shared and +reference-counted across clients — a client subscribing to a topic that +another client is *already* subscribed to does not create a new DDS +subscription, so it would otherwise have to wait for the next publish (which, +for a latched topic like `/tf_static`, may never come). + +To cover this case, the bridge retains the most recent message for each +latched topic outside the normal 1-second message buffer. Immediately after +a successful `subscribe` response, the server sends one extra binary frame +per newly-subscribed latched topic containing just that retained message — +same [Binary Message Format](#binary-message-format) as regular aggregated +frames, just with a single message and the original (possibly "stale") +timestamp from when it was received. No client-side handling is required +beyond decoding it like any other binary frame. + +Only the single newest sample per latched topic is retained (bounded memory +use), and only for topics that have had at least one subscriber — the +first subscriber's DDS-native delivery is what seeds the retained copy for +later subscribers. + ## Unsubscribe Remove topics from subscription. Only removes specified topics; other subscriptions are preserved. diff --git a/ros2/include/pj_bridge_ros2/generic_subscription_manager.hpp b/ros2/include/pj_bridge_ros2/generic_subscription_manager.hpp index 083272a..cff1572 100644 --- a/ros2/include/pj_bridge_ros2/generic_subscription_manager.hpp +++ b/ros2/include/pj_bridge_ros2/generic_subscription_manager.hpp @@ -56,6 +56,12 @@ class GenericSubscriptionManager { size_t get_reference_count(const std::string& topic_name) const; void unsubscribe_all(); + /// True when the subscription for `topic_name` was created with + /// TRANSIENT_LOCAL durability (i.e. every publisher discovered at + /// subscribe time offered it). Returns false if the topic is not + /// currently subscribed. + bool is_transient_local(const std::string& topic_name) const; + // Exposed publicly for testability; computes the QoS a subscription should // use for `topic_name` based on the publishers currently discovered on it. rclcpp::QoS adapt_qos(const std::string& topic_name) const; @@ -64,6 +70,7 @@ class GenericSubscriptionManager { struct SubscriptionInfo { std::shared_ptr subscription; size_t reference_count; + bool transient_local; ///< durability actually used for this subscription }; rclcpp::Node::SharedPtr node_; diff --git a/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp b/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp index f34c8e6..b52ac2c 100644 --- a/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp +++ b/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp @@ -48,6 +48,7 @@ class Ros2SubscriptionManager : public SubscriptionManagerInterface { bool subscribe(const std::string& topic_name, const std::string& topic_type) override; bool unsubscribe(const std::string& topic_name) override; void unsubscribe_all() override; + bool is_transient_local(const std::string& topic_name) const override; private: GenericSubscriptionManager inner_manager_; diff --git a/ros2/src/generic_subscription_manager.cpp b/ros2/src/generic_subscription_manager.cpp index 3723f2f..9c5270c 100644 --- a/ros2/src/generic_subscription_manager.cpp +++ b/ros2/src/generic_subscription_manager.cpp @@ -105,9 +105,12 @@ bool GenericSubscriptionManager::subscribe( callback(topic_name, msg, receive_time); }; - auto subscription = node_->create_generic_subscription(topic_name, topic_type, adapt_qos(topic_name), sub_callback); + rclcpp::QoS qos = adapt_qos(topic_name); + bool transient_local = (qos.durability() == rclcpp::DurabilityPolicy::TransientLocal); - subscriptions_[topic_name] = SubscriptionInfo{subscription, 1}; + auto subscription = node_->create_generic_subscription(topic_name, topic_type, qos, sub_callback); + + subscriptions_[topic_name] = SubscriptionInfo{subscription, 1, transient_local}; return true; } catch (const std::exception& e) { @@ -160,4 +163,13 @@ void GenericSubscriptionManager::unsubscribe_all() { subscriptions_.clear(); } +bool GenericSubscriptionManager::is_transient_local(const std::string& topic_name) const { + std::lock_guard lock(mutex_); + auto it = subscriptions_.find(topic_name); + if (it == subscriptions_.end()) { + return false; + } + return it->second.transient_local; +} + } // namespace pj_bridge diff --git a/ros2/src/ros2_subscription_manager.cpp b/ros2/src/ros2_subscription_manager.cpp index c45c77f..4f29fc9 100644 --- a/ros2/src/ros2_subscription_manager.cpp +++ b/ros2/src/ros2_subscription_manager.cpp @@ -78,4 +78,8 @@ void Ros2SubscriptionManager::unsubscribe_all() { inner_manager_.unsubscribe_all(); } +bool Ros2SubscriptionManager::is_transient_local(const std::string& topic_name) const { + return inner_manager_.is_transient_local(topic_name); +} + } // namespace pj_bridge diff --git a/tests/unit/test_bounded_frame_queue.cpp b/tests/unit/test_bounded_frame_queue.cpp index da40c94..5d5a842 100644 --- a/tests/unit/test_bounded_frame_queue.cpp +++ b/tests/unit/test_bounded_frame_queue.cpp @@ -17,12 +17,12 @@ * along with pj_bridge. If not, see . */ -#include "pj_bridge/middleware/bounded_frame_queue.hpp" - #include #include +#include "pj_bridge/middleware/bounded_frame_queue.hpp" + using namespace pj_bridge; namespace { diff --git a/tests/unit/test_bridge_server.cpp b/tests/unit/test_bridge_server.cpp index 413e67d..6657a0f 100644 --- a/tests/unit/test_bridge_server.cpp +++ b/tests/unit/test_bridge_server.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -32,6 +33,7 @@ #include #include "pj_bridge/bridge_server.hpp" +#include "pj_bridge/message_serializer.hpp" #include "pj_bridge/middleware/middleware_interface.hpp" #include "pj_bridge/subscription_manager_interface.hpp" #include "pj_bridge/topic_source_interface.hpp" @@ -278,12 +280,42 @@ class MockSubscriptionManager : public SubscriptionManagerInterface { ref_counts_.clear(); } + bool is_transient_local(const std::string& topic_name) const override { + std::lock_guard lock(mutex_); + return transient_local_topics_.count(topic_name) > 0; + } + // Test helpers void add_known_topic(const std::string& topic_name) { std::lock_guard lock(mutex_); known_topics_.insert(topic_name); } + // Mark a topic as reporting TRANSIENT_LOCAL durability from is_transient_local(). + void set_transient_local(const std::string& topic_name, bool transient_local) { + std::lock_guard lock(mutex_); + if (transient_local) { + transient_local_topics_.insert(topic_name); + } else { + transient_local_topics_.erase(topic_name); + } + } + + // Simulate a message arriving on `topic_name`, invoking the callback + // registered via set_message_callback() — exactly as the real backend + // would when a middleware message is received. + void deliver_message( + const std::string& topic_name, std::shared_ptr> data, uint64_t timestamp_ns) { + MessageCallback cb; + { + std::lock_guard lock(mutex_); + cb = callback_; + } + if (cb) { + cb(topic_name, std::move(data), timestamp_ns); + } + } + bool is_subscribed(const std::string& topic_name) const { std::lock_guard lock(mutex_); auto it = ref_counts_.find(topic_name); @@ -313,9 +345,61 @@ class MockSubscriptionManager : public SubscriptionManagerInterface { MessageCallback callback_; std::unordered_set known_topics_; std::unordered_map ref_counts_; + std::unordered_set transient_local_topics_; bool underflow_detected_{false}; }; +// --------------------------------------------------------------------------- +// decode_frame — test helper to parse a binary aggregated-message frame +// (16-byte header + ZSTD-compressed payload) back into its messages, mirroring +// the wire format documented in docs/API.md and produced by +// AggregatedMessageSerializer::finalize(). +// --------------------------------------------------------------------------- +struct DecodedMessage { + std::string topic; + uint64_t timestamp_ns; + std::vector data; +}; + +std::vector decode_frame(const std::vector& frame) { + std::vector result; + if (frame.size() < 16) { + return result; + } + + uint32_t message_count = 0; + std::memcpy(&message_count, frame.data() + 4, sizeof(message_count)); + + std::vector compressed(frame.begin() + 16, frame.end()); + std::vector payload; + AggregatedMessageSerializer::decompress_zstd(compressed, payload); + + size_t offset = 0; + for (uint32_t i = 0; i < message_count; ++i) { + uint16_t topic_len = 0; + std::memcpy(&topic_len, payload.data() + offset, sizeof(topic_len)); + offset += sizeof(topic_len); + + std::string topic(payload.begin() + offset, payload.begin() + offset + topic_len); + offset += topic_len; + + uint64_t timestamp_ns = 0; + std::memcpy(×tamp_ns, payload.data() + offset, sizeof(timestamp_ns)); + offset += sizeof(timestamp_ns); + + uint32_t data_len = 0; + std::memcpy(&data_len, payload.data() + offset, sizeof(data_len)); + offset += sizeof(data_len); + + std::vector data(payload.begin() + offset, payload.begin() + offset + data_len); + offset += data_len; + + result.push_back(DecodedMessage{std::move(topic), timestamp_ns, std::move(data)}); + } + + return result; +} + // --------------------------------------------------------------------------- // Test fixture // --------------------------------------------------------------------------- @@ -2341,3 +2425,101 @@ TEST_F(BridgeServerTest, UnsubscribeTopicUpdatesStopsNotifications) { auto replies = mock_->get_replies("client_x"); EXPECT_TRUE(replies.empty()); } + +// --------------------------------------------------------------------------- +// Latched (transient_local) topic replay +// +// A late subscriber joining an already-shared middleware subscription for a +// latched topic must be immediately replayed the most recent message, even +// after it has aged out of the normal 1-second message buffer. The FIRST +// subscriber gets its sample naturally from DDS (transient_local +// redelivery), so this replay path exists specifically for LATER clients. +// --------------------------------------------------------------------------- + +// LatchedTopicReplaysToLateSubscriberOnly +TEST_F(BridgeServerTest, LatchedTopicReplaysToLateSubscriberOnly) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/latched", "std_msgs/msg/String"}}); + mock_sub_manager_->add_known_topic("/latched"); + mock_sub_manager_->set_transient_local("/latched", true); + + json sub; + sub["command"] = "subscribe"; + sub["topics"] = json::array({"/latched"}); + + // Client A subscribes first (the "first subscriber" — no retained sample + // exists yet, so no replay frame is sent). + mock_->push_request("client_a", sub.dump()); + server_->process_requests(); + json resp_a = mock_->pop_reply("client_a"); + ASSERT_FALSE(resp_a.is_discarded()); + EXPECT_EQ(resp_a["status"], "success"); + EXPECT_TRUE(mock_->get_binary_sends().empty()); + + // A message arrives on the latched topic. + std::vector payload = {10, 20, 30, 40}; + auto data = std::make_shared>(payload.size()); + for (size_t i = 0; i < payload.size(); ++i) { + (*data)[i] = static_cast(payload[i]); + } + mock_sub_manager_->deliver_message("/latched", data, 12345); + + // Drain the normal message buffer, simulating the message aging out of + // the regular publish cycle (it is delivered to client_a here). + server_->publish_aggregated_messages(); + mock_->clear_binary_sends(); + + // Client B subscribes later, joining the already-shared subscription. + mock_->push_request("client_b", sub.dump()); + server_->process_requests(); + json resp_b = mock_->pop_reply("client_b"); + ASSERT_FALSE(resp_b.is_discarded()); + EXPECT_EQ(resp_b["status"], "success"); + + // Exactly one binary frame, sent only to client_b, containing the + // retained message. + auto binary_sends = mock_->get_binary_sends(); + ASSERT_EQ(binary_sends.size(), 1u); + EXPECT_EQ(binary_sends[0].first, "client_b"); + + auto messages = decode_frame(binary_sends[0].second); + ASSERT_EQ(messages.size(), 1u); + EXPECT_EQ(messages[0].topic, "/latched"); + EXPECT_EQ(messages[0].timestamp_ns, 12345u); + EXPECT_EQ(messages[0].data, payload); +} + +// SubscribeToNonLatchedTopicSendsNoImmediateFrame +TEST_F(BridgeServerTest, SubscribeToNonLatchedTopicSendsNoImmediateFrame) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/plain", "std_msgs/msg/String"}}); + mock_sub_manager_->add_known_topic("/plain"); + // Not marked transient_local. + + json sub; + sub["command"] = "subscribe"; + sub["topics"] = json::array({"/plain"}); + + mock_->push_request("client_a", sub.dump()); + server_->process_requests(); + mock_->pop_reply("client_a"); + + std::vector payload = {1, 2, 3}; + auto data = std::make_shared>(payload.size()); + for (size_t i = 0; i < payload.size(); ++i) { + (*data)[i] = static_cast(payload[i]); + } + mock_sub_manager_->deliver_message("/plain", data, 999); + server_->publish_aggregated_messages(); + mock_->clear_binary_sends(); + + // A second, late client subscribing to a non-latched topic must not + // receive any immediate replay frame. + mock_->push_request("client_b", sub.dump()); + server_->process_requests(); + json resp_b = mock_->pop_reply("client_b"); + ASSERT_FALSE(resp_b.is_discarded()); + EXPECT_EQ(resp_b["status"], "success"); + + EXPECT_TRUE(mock_->get_binary_sends().empty()); +} diff --git a/tests/unit/test_generic_subscription_manager.cpp b/tests/unit/test_generic_subscription_manager.cpp index 8eab40e..70d6c15 100644 --- a/tests/unit/test_generic_subscription_manager.cpp +++ b/tests/unit/test_generic_subscription_manager.cpp @@ -403,3 +403,35 @@ TEST_F(GenericSubscriptionManagerTest, AdaptQosDepthMixedKnownAndUnknownPublishe EXPECT_EQ(qos.depth(), 100u); } + +// --------------------------------------------------------------------------- +// is_transient_local() +// +// Durability (unlike history depth) is one of the QoS policies DDS discovery +// is required to propagate, since it affects wire compatibility — every RMW +// reports it correctly, so these tests need no SKIP_IF_RMW_HIDES_DEPTH guard. +// --------------------------------------------------------------------------- + +TEST_F(GenericSubscriptionManagerTest, IsTransientLocalTrueWhenPublisherOffersIt) { + auto publisher = node_->create_publisher("/latched_topic", rclcpp::QoS(1).transient_local()); + wait_for_publisher_count(node_, "/latched_topic", 1); + + auto callback = [](const std::string&, const std::shared_ptr&, uint64_t) {}; + ASSERT_TRUE(manager_->subscribe("/latched_topic", "sensor_msgs/msg/Imu", callback)); + + EXPECT_TRUE(manager_->is_transient_local("/latched_topic")); +} + +TEST_F(GenericSubscriptionManagerTest, IsTransientLocalFalseForVolatilePublisher) { + auto publisher = node_->create_publisher("/volatile_topic", rclcpp::QoS(1)); + wait_for_publisher_count(node_, "/volatile_topic", 1); + + auto callback = [](const std::string&, const std::shared_ptr&, uint64_t) {}; + ASSERT_TRUE(manager_->subscribe("/volatile_topic", "sensor_msgs/msg/Imu", callback)); + + EXPECT_FALSE(manager_->is_transient_local("/volatile_topic")); +} + +TEST_F(GenericSubscriptionManagerTest, IsTransientLocalFalseForUnsubscribedTopic) { + EXPECT_FALSE(manager_->is_transient_local("/never_subscribed_topic")); +} diff --git a/tests/unit/test_message_buffer.cpp b/tests/unit/test_message_buffer.cpp index 31ab4a4..6e2eab1 100644 --- a/tests/unit/test_message_buffer.cpp +++ b/tests/unit/test_message_buffer.cpp @@ -397,3 +397,79 @@ TEST_F(MessageBufferTest, ExpiredMessagesStillPurgedWithInjectedClock) { // The first message is stale and must be gone; the new one remains EXPECT_EQ(buffer.size(), 1); } + +// --------------------------------------------------------------------------- +// Latched (transient_local) topic support +// +// A latched topic retains its newest sample outside the normal TTL/move_messages +// path so that late subscribers to an already-shared subscription can be +// replayed the current value even after it has aged out of the buffer. +// --------------------------------------------------------------------------- + +TEST_F(MessageBufferTest, LatchedMessageSurvivesTtlAndMoveMessages) { + uint64_t fake_now = 10'000'000'000ULL; + MessageBuffer buffer(MessageBuffer::kDefaultMaxMessageAgeNs, [&fake_now]() { return fake_now; }); + + buffer.set_latched("/t", true); + + std::vector data = {1, 2, 3}; + buffer.add_message("/t", 1, create_test_data(data)); + + // Advance the clock well past the 1 s TTL and add a message on another + // topic to trigger cleanup of the (non-latched) deque path. + fake_now += 2'000'000'000ULL; + buffer.add_message("/other", 2, create_test_data({9})); + + // Drain everything via move_messages — the latched store must not be + // touched by this call. + std::unordered_map> messages; + buffer.move_messages(messages); + + auto latched = buffer.get_latched("/t"); + ASSERT_TRUE(latched.has_value()); + EXPECT_EQ(extract_data(latched->data), data); +} + +TEST_F(MessageBufferTest, GetLatchedReturnsNewestMessage) { + buffer_.set_latched("/t", true); + + buffer_.add_message("/t", 1, create_test_data({1, 1, 1})); + buffer_.add_message("/t", 2, create_test_data({2, 2, 2})); + + auto latched = buffer_.get_latched("/t"); + ASSERT_TRUE(latched.has_value()); + EXPECT_EQ(extract_data(latched->data), (std::vector{2, 2, 2})); + EXPECT_EQ(latched->timestamp_ns, 2u); +} + +TEST_F(MessageBufferTest, SetLatchedFalseClearsRetainedEntry) { + buffer_.set_latched("/t", true); + buffer_.add_message("/t", 1, create_test_data({1, 2, 3})); + ASSERT_TRUE(buffer_.get_latched("/t").has_value()); + + buffer_.set_latched("/t", false); + EXPECT_FALSE(buffer_.get_latched("/t").has_value()); + + // Once unlatched, further messages are not retained either. + buffer_.add_message("/t", 2, create_test_data({4, 5, 6})); + EXPECT_FALSE(buffer_.get_latched("/t").has_value()); +} + +TEST_F(MessageBufferTest, NonLatchedTopicHasNoRetainedMessage) { + buffer_.add_message("/plain", 1, create_test_data({1, 2, 3})); + EXPECT_FALSE(buffer_.get_latched("/plain").has_value()); +} + +TEST_F(MessageBufferTest, ClearAlsoClearsLatchedStorage) { + buffer_.set_latched("/t", true); + buffer_.add_message("/t", 1, create_test_data({1, 2, 3})); + ASSERT_TRUE(buffer_.get_latched("/t").has_value()); + + buffer_.clear(); + EXPECT_FALSE(buffer_.get_latched("/t").has_value()); + + // Latching config was cleared too: a message added post-clear (without a + // fresh set_latched call) is not retained. + buffer_.add_message("/t", 2, create_test_data({4, 5, 6})); + EXPECT_FALSE(buffer_.get_latched("/t").has_value()); +} From 569a0f352b121781543736e221feebefbf747090 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 15:24:55 +0200 Subject: [PATCH 11/16] fix: address backpressure review findings (orphan queue race, wrap-proof depth sum) - send_binary: re-check client existence under clients_mutex_ before enqueueing an over-watermark frame; a concurrent disconnect between the socket-handle lookup and the enqueue could otherwise recreate a pending queue for a gone client, stranding the frame until shutdown and skewing dropped_frame_count(). Return false (same contract as the initial lookup miss) instead. - Document that backlog flushing only happens inside send_binary by design (bounded queue, fresh-data-first plotting clients). - generic_subscription_manager: replace add-then-clamp saturating depth sum with headroom arithmetic so size_t wrap is structurally impossible even for extreme max_qos_depth_ values via the constructor API. Co-Authored-By: Claude Fable 5 --- app/src/middleware/websocket_middleware.cpp | 16 +++++++++++++ ros2/src/generic_subscription_manager.cpp | 11 +++++---- tests/unit/test_websocket_middleware.cpp | 26 +++++++++++++++++++++ 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/app/src/middleware/websocket_middleware.cpp b/app/src/middleware/websocket_middleware.cpp index 784ff43..45cf780 100644 --- a/app/src/middleware/websocket_middleware.cpp +++ b/app/src/middleware/websocket_middleware.cpp @@ -335,10 +335,26 @@ bool WebSocketMiddleware::send_binary(const std::string& client_identity, const // Socket buffer is over the high watermark: queue the frame instead of // sending it now. + // + // Note the backlog is only ever flushed from within send_binary itself: if + // a client's subscribed topics go quiet, queued frames sit here until the + // next send_binary call for that client (or its disconnect). This is + // deliberate — the queue is bounded, delivering stale frames on the next + // traffic burst is acceptable, and plotting clients care about fresh data, + // so a dedicated flush timer isn't worth the extra machinery. size_t dropped; uint64_t dropped_total_now; { std::lock_guard lock(clients_mutex_); + // Re-check the client still exists: between the ws lookup above (lock + // released before the socket calls) and here, the disconnect callback may + // have erased the client AND its pending_frames_ entry. Blindly + // try_emplace-ing would recreate a queue for a disconnected client, + // stranding the frame until shutdown and skewing dropped_frame_count(). + // Treat it like the top-of-function lookup miss: the client is gone. + if (clients_.find(client_identity) == clients_.end()) { + return false; + } auto [pending_it, inserted] = pending_frames_.try_emplace(client_identity, BoundedFrameQueue(client_backlog_size_)); (void)inserted; dropped = pending_it->second.push(data); diff --git a/ros2/src/generic_subscription_manager.cpp b/ros2/src/generic_subscription_manager.cpp index 9c5270c..22cfca3 100644 --- a/ros2/src/generic_subscription_manager.cpp +++ b/ros2/src/generic_subscription_manager.cpp @@ -59,9 +59,11 @@ rclcpp::QoS GenericSubscriptionManager::adapt_qos(const std::string& topic_name) // the publisher uses KEEP_ALL. It contributes kFallbackDepth instead of // 0 — assuming the historical generous default is safer than shrinking // the queue and dropping messages on high-rate topics. - // - The sum is saturating: each contribution is capped at max_qos_depth_ - // and the running total is clamped to max_qos_depth_, so it can never - // wrap size_t no matter how many publishers report huge depths. + // - The sum is saturating: each contribution is capped at the remaining + // headroom below max_qos_depth_ (total_depth <= max_qos_depth_ is a loop + // invariant), so the addition is structurally incapable of wrapping + // size_t no matter how many publishers report huge depths — even for + // extreme max_qos_depth_ values passed via the constructor API. size_t total_depth = 0; for (const auto& info : publishers) { const auto& profile = info.qos_profile(); @@ -72,7 +74,8 @@ rclcpp::QoS GenericSubscriptionManager::adapt_qos(const std::string& topic_name) all_transient_local = false; } const size_t publisher_depth = profile.depth() > 0 ? profile.depth() : kFallbackDepth; - total_depth = std::min(total_depth + std::min(publisher_depth, max_qos_depth_), max_qos_depth_); + const size_t headroom = max_qos_depth_ - total_depth; // total_depth <= max_qos_depth_ invariant + total_depth += std::min(publisher_depth, headroom); } qos.keep_last(std::clamp(total_depth, min_qos_depth_, max_qos_depth_)); diff --git a/tests/unit/test_websocket_middleware.cpp b/tests/unit/test_websocket_middleware.cpp index 4730e15..8357eb3 100644 --- a/tests/unit/test_websocket_middleware.cpp +++ b/tests/unit/test_websocket_middleware.cpp @@ -481,6 +481,32 @@ TEST_F(WebSocketMiddlewareTest, DroppedFrameCountZeroAfterNormalTraffic) { std::this_thread::sleep_for(std::chrono::milliseconds(100)); } +// Regression guard for the disconnect-race fix in send_binary's enqueue path: +// a client that is not (or no longer) in clients_ must never get a +// pending_frames_ entry created for it. The over-watermark branch itself +// cannot be reached from a unit test (we cannot force a real socket's +// bufferedAmount() past 1 MiB), so this verifies the shared contract via the +// public API: send_binary for an id absent from clients_ returns false and +// leaves no pending state behind — dropped_frame_count() stays 0 even after +// repeated attempts. The in-branch re-check under clients_mutex_ enforces the +// same "client gone -> return false, no queue created" rule; only the actual +// interleaving with a concurrent disconnect is not exercised here. +TEST_F(WebSocketMiddlewareTest, SendBinaryToAbsentClientCreatesNoPendingState) { + auto result = middleware_->initialize(18098); + ASSERT_TRUE(result.has_value()); + + std::vector data = {1, 2, 3}; + for (int i = 0; i < 5; ++i) { + EXPECT_FALSE(middleware_->send_binary("never-connected", data)); + } + EXPECT_EQ(middleware_->dropped_frame_count(), 0u); + + // Shutdown folds any (erroneously) surviving per-client queues into the + // disconnected-clients accumulator — the count must still be zero after. + middleware_->shutdown(); + EXPECT_EQ(middleware_->dropped_frame_count(), 0u); +} + TEST(WebSocketMiddlewareBacklogConstructionTest, ExplicitBacklogSizeConstructsAndInitializes) { WebSocketMiddleware middleware(5); EXPECT_EQ(middleware.dropped_frame_count(), 0u); From 2a3328932416e0338da23cb27c4d0defe47f40b8 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 15:37:26 +0200 Subject: [PATCH 12/16] fix: latched replay ordering, stale-sample teardown, and duplicate gate - Send the replay binary frame strictly AFTER the subscribe response: handle_subscribe now queues serialized frames in pending_replays_ (new leaf replays_mutex_); process_single_request flushes them right after send_reply succeeds, and drops them when the reply fails or the session is cleaned up. The client always has the schema before the frame. - Clear latched state when the last subscription ref is released (new release_subscription_ref helper used by unsubscribe/pause/cleanup/ rollback, backed by SubscriptionManagerInterface::is_subscribed with a default-false implementation and a ROS2 forwarding override): a stale retained sample is never replayed ahead of the fresh DDS transient_local redelivery on the next subscription. - Skip the replay while the retained sample is still in the normal aggregation buffer (MessageBuffer::get_latched_for_replay): the regular publish cycle delivers it to the new subscriber anyway, so replaying would duplicate it. - docs/API.md: document teardown, duplicate gate, and that replay frames bypass rate limiting and publish statistics. Co-Authored-By: Claude Fable 5 --- app/include/pj_bridge/bridge_server.hpp | 19 +++ app/include/pj_bridge/message_buffer.hpp | 8 + .../subscription_manager_interface.hpp | 7 + app/src/bridge_server.cpp | 85 ++++++++-- app/src/message_buffer.cpp | 18 +++ docs/API.md | 11 +- .../ros2_subscription_manager.hpp | 1 + ros2/src/ros2_subscription_manager.cpp | 4 + tests/unit/test_bridge_server.cpp | 149 +++++++++++++++++- tests/unit/test_message_buffer.cpp | 37 +++++ 10 files changed, 322 insertions(+), 17 deletions(-) diff --git a/app/include/pj_bridge/bridge_server.hpp b/app/include/pj_bridge/bridge_server.hpp index 844c226..a9e2832 100644 --- a/app/include/pj_bridge/bridge_server.hpp +++ b/app/include/pj_bridge/bridge_server.hpp @@ -151,6 +151,14 @@ class BridgeServer { void inject_response_fields(nlohmann::json& response, const nlohmann::json& request) const; void cleanup_session(const std::string& client_id); + /// Release one middleware subscription ref for @p topic_name. When the + /// last ref is gone (subscription destroyed), also clears the topic's + /// latched state in the message buffer: a stale retained sample must not + /// be replayed to the next fresh subscriber, who receives the current + /// sample from DDS transient_local redelivery instead. Call with + /// cleanup_mutex_ held (same requirement as unsubscribe itself). + void release_subscription_ref(const std::string& topic_name); + // Backend interfaces (owned) std::shared_ptr topic_source_; std::shared_ptr subscription_manager_; @@ -186,6 +194,9 @@ class BridgeServer { // call middleware_ or session_manager_ (or acquire any other lock in this // class) while holding topics_mutex_ — release it before sending // notifications. + // replays_mutex_ is likewise a LEAF lock: it only guards pending_replays_ + // map accesses. Never hold it while acquiring any other lock or calling + // middleware_ — move the frames out, release, then send. std::mutex cleanup_mutex_; // Per-client per-topic last-sent timestamp (nanoseconds) for rate limiting @@ -197,6 +208,14 @@ class BridgeServer { std::unordered_map known_topics_; bool topics_snapshot_taken_{false}; std::mutex topics_mutex_; + + // Latched (transient_local) replay frames queued by handle_subscribe, + // flushed by process_single_request right AFTER the subscribe response is + // sent — the client must receive the response (which carries the topic's + // schema) before the binary frame that needs it. LEAF lock (see comment + // above). + std::unordered_map>> pending_replays_; + std::mutex replays_mutex_; }; } // namespace pj_bridge diff --git a/app/include/pj_bridge/message_buffer.hpp b/app/include/pj_bridge/message_buffer.hpp index d1373a2..f5aa04f 100644 --- a/app/include/pj_bridge/message_buffer.hpp +++ b/app/include/pj_bridge/message_buffer.hpp @@ -92,6 +92,14 @@ class MessageBuffer { /// topic is not latched or no sample has been retained yet. std::optional get_latched(const std::string& topic_name) const; + /// Like get_latched(), but additionally returns nullopt while the topic + /// still has messages in the normal buffer. In that case the upcoming + /// publish cycle will deliver the sample to the new subscriber anyway — + /// replaying it would produce a duplicate. Replay is only needed for + /// samples that already left the normal buffer (drained by + /// move_messages() or TTL-evicted). + std::optional get_latched_for_replay(const std::string& topic_name) const; + private: mutable std::mutex mutex_; std::unordered_map> topic_buffers_; diff --git a/app/include/pj_bridge/subscription_manager_interface.hpp b/app/include/pj_bridge/subscription_manager_interface.hpp index b4631a0..93f302b 100644 --- a/app/include/pj_bridge/subscription_manager_interface.hpp +++ b/app/include/pj_bridge/subscription_manager_interface.hpp @@ -81,6 +81,13 @@ class SubscriptionManagerInterface { virtual bool is_transient_local(const std::string& /*topic_name*/) const { return false; } + + /// True while the topic has at least one active reference (i.e. the + /// underlying middleware subscription exists). Backends that don't track + /// this return false. + virtual bool is_subscribed(const std::string& /*topic_name*/) const { + return false; + } }; } // namespace pj_bridge diff --git a/app/src/bridge_server.cpp b/app/src/bridge_server.cpp index 4778e46..3568585 100644 --- a/app/src/bridge_server.cpp +++ b/app/src/bridge_server.cpp @@ -212,7 +212,28 @@ void BridgeServer::process_single_request(const std::vector& request_da std::vector response_data(response.begin(), response.end()); if (!middleware_->send_reply(client_id, response_data)) { spdlog::warn("Failed to send response to client '{}', cleaning up session", client_id); + // cleanup_session drops any replay frames handle_subscribe queued for + // this client (they must never be delivered without the response). cleanup_session(client_id); + return; + } + + // Flush any latched-topic replay frames handle_subscribe queued for this + // client. Sent strictly AFTER the response so the client has the schema + // needed to decode them (docs/API.md: "right after the subscribe + // response"). Move the frames out under the leaf replays_mutex_, release + // it, then send — never call middleware_ while holding the lock. + std::vector> replay_frames; + { + std::lock_guard replays_lock(replays_mutex_); + auto it = pending_replays_.find(client_id); + if (it != pending_replays_.end()) { + replay_frames = std::move(it->second); + pending_replays_.erase(it); + } + } + for (const auto& frame : replay_frames) { + middleware_->send_binary(client_id, frame); } } @@ -379,7 +400,7 @@ std::string BridgeServer::handle_subscribe(const std::string& client_id, const n if (!session_manager_->add_subscription(client_id, topic_name, rate)) { spdlog::warn("Session gone for client '{}', rolling back subscribe for '{}'", client_id, topic_name); if (ref_acquired) { - subscription_manager_->unsubscribe(topic_name); + release_subscription_ref(topic_name); } continue; } @@ -406,7 +427,10 @@ std::string BridgeServer::handle_subscribe(const std::string& client_id, const n bool is_latched = subscription_manager_->is_transient_local(topic_name); message_buffer_->set_latched(topic_name, is_latched); if (is_latched) { - if (auto latched_msg = message_buffer_->get_latched(topic_name)) { + // get_latched_for_replay skips samples still sitting in the + // normal buffer — those reach the new subscriber through the + // regular publish cycle, and replaying them too would duplicate. + if (auto latched_msg = message_buffer_->get_latched_for_replay(topic_name)) { replay_candidates.emplace_back(topic_name, *latched_msg); } } @@ -424,16 +448,28 @@ std::string BridgeServer::handle_subscribe(const std::string& client_id, const n } } - // Replay retained latched samples to this client now that cleanup_mutex_ - // is released. Each candidate becomes its own single-message binary frame - // (mirrors the normal aggregated-frame format so clients need no special - // handling). Does not touch last_sent_times_: the sample's old timestamp - // means it won't interfere with subsequent rate limiting. - for (const auto& [topic_name, latched_msg] : replay_candidates) { - AggregatedMessageSerializer serializer; - serializer.serialize_message( - topic_name, latched_msg.timestamp_ns, latched_msg.data->data(), latched_msg.data->size()); - middleware_->send_binary(client_id, serializer.finalize()); + // Serialize the retained latched samples now that cleanup_mutex_ is + // released, but do NOT send them here: the subscribe response (which + // carries the topic's schema) is sent by process_single_request after + // this handler returns, and the client must receive it before any binary + // frame that needs the schema to decode. Queue the frames instead; + // process_single_request flushes them right after the response goes out. + // Each candidate becomes its own single-message binary frame (mirrors the + // normal aggregated-frame format so clients need no special handling). + // Does not touch last_sent_times_: the sample's old timestamp means it + // won't interfere with subsequent rate limiting. + if (!replay_candidates.empty()) { + std::vector> frames; + frames.reserve(replay_candidates.size()); + for (const auto& [topic_name, latched_msg] : replay_candidates) { + AggregatedMessageSerializer serializer; + serializer.serialize_message( + topic_name, latched_msg.timestamp_ns, latched_msg.data->data(), latched_msg.data->size()); + frames.push_back(serializer.finalize()); + } + std::lock_guard replays_lock(replays_mutex_); + auto& pending = pending_replays_[client_id]; + pending.insert(pending.end(), std::make_move_iterator(frames.begin()), std::make_move_iterator(frames.end())); } // Build response @@ -504,7 +540,7 @@ std::string BridgeServer::handle_unsubscribe(const std::string& client_id, const // Release the middleware ref only if this session actually holds one // (a paused client's refs were already released by pause). if (held_refs.count(topic_name) > 0) { - subscription_manager_->unsubscribe(topic_name); + release_subscription_ref(topic_name); } session_manager_->remove_subscription(client_id, topic_name); current_subs.erase(topic_name); @@ -566,7 +602,7 @@ std::string BridgeServer::handle_pause(const std::string& client_id, const nlohm // Release exactly the refs this session holds auto held_refs = session_manager_->get_ref_held_topics(client_id); for (const auto& topic : held_refs) { - subscription_manager_->unsubscribe(topic); + release_subscription_ref(topic); session_manager_->set_ref_held(client_id, topic, false); spdlog::debug("Released ref for topic '{}' (client '{}' paused)", topic, client_id); } @@ -790,9 +826,28 @@ void BridgeServer::check_topic_changes() { } } +void BridgeServer::release_subscription_ref(const std::string& topic_name) { + subscription_manager_->unsubscribe(topic_name); + // When the last ref is released the middleware subscription is destroyed. + // Clear the topic's latched state with it: the retained sample would go + // stale, and the NEXT subscriber creates a fresh middleware subscription + // that receives the current sample via DDS transient_local redelivery — + // replaying the stale copy before it would deliver out-of-date data. + if (!subscription_manager_->is_subscribed(topic_name) && message_buffer_) { + message_buffer_->set_latched(topic_name, false); + } +} + void BridgeServer::cleanup_session(const std::string& client_id) { std::lock_guard lock(cleanup_mutex_); + // Drop any undelivered latched-replay frames regardless of session state + // (the client is gone; leaving them queued would strand them forever). + { + std::lock_guard replays_lock(replays_mutex_); + pending_replays_.erase(client_id); + } + if (!session_manager_->session_exists(client_id)) { return; } @@ -801,7 +856,7 @@ void BridgeServer::cleanup_session(const std::string& client_id) { // there is no separate paused check to race against. auto held_refs = session_manager_->get_ref_held_topics(client_id); for (const auto& topic : held_refs) { - subscription_manager_->unsubscribe(topic); + release_subscription_ref(topic); spdlog::debug("Unsubscribed client '{}' from topic '{}'", client_id, topic); } diff --git a/app/src/message_buffer.cpp b/app/src/message_buffer.cpp index 303c074..d0f4a27 100644 --- a/app/src/message_buffer.cpp +++ b/app/src/message_buffer.cpp @@ -70,6 +70,24 @@ std::optional MessageBuffer::get_latched(const std::string& top return it->second; } +std::optional MessageBuffer::get_latched_for_replay(const std::string& topic_name) const { + std::lock_guard lock(mutex_); + auto it = latched_last_.find(topic_name); + if (it == latched_last_.end()) { + return std::nullopt; + } + // While the topic still has messages in the normal buffer, the retained + // sample is (or is older than) one of them, and the upcoming publish + // cycle will deliver it to the new subscriber via the regular aggregated + // path — a replay would duplicate it. Empty deques are erased by both + // cleanup and move_messages(), so presence means undelivered messages. + auto buf_it = topic_buffers_.find(topic_name); + if (buf_it != topic_buffers_.end() && !buf_it->second.empty()) { + return std::nullopt; + } + return it->second; +} + void MessageBuffer::move_messages(std::unordered_map>& out_messages) { std::lock_guard lock(mutex_); out_messages = std::move(topic_buffers_); diff --git a/docs/API.md b/docs/API.md index aa1de31..9ed7cb9 100644 --- a/docs/API.md +++ b/docs/API.md @@ -230,7 +230,16 @@ beyond decoding it like any other binary frame. Only the single newest sample per latched topic is retained (bounded memory use), and only for topics that have had at least one subscriber — the first subscriber's DDS-native delivery is what seeds the retained copy for -later subscribers. +later subscribers. When the last subscriber of a latched topic leaves +(unsubscribe, pause, or disconnect), the retained sample is discarded along +with the underlying subscription: the next subscriber gets the current +sample from DDS redelivery, never a stale copy. + +No replay frame is sent when the retained message is still pending in the +regular aggregation buffer — in that case the next aggregated frame delivers +it to the new subscriber anyway, and a replay would duplicate it. Replay +frames intentionally bypass per-topic rate limiting (`max_rate_hz`) and are +not counted in the server's publish statistics. ## Unsubscribe diff --git a/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp b/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp index b52ac2c..2ab8b2b 100644 --- a/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp +++ b/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp @@ -49,6 +49,7 @@ class Ros2SubscriptionManager : public SubscriptionManagerInterface { bool unsubscribe(const std::string& topic_name) override; void unsubscribe_all() override; bool is_transient_local(const std::string& topic_name) const override; + bool is_subscribed(const std::string& topic_name) const override; private: GenericSubscriptionManager inner_manager_; diff --git a/ros2/src/ros2_subscription_manager.cpp b/ros2/src/ros2_subscription_manager.cpp index 4f29fc9..fd6de59 100644 --- a/ros2/src/ros2_subscription_manager.cpp +++ b/ros2/src/ros2_subscription_manager.cpp @@ -82,4 +82,8 @@ bool Ros2SubscriptionManager::is_transient_local(const std::string& topic_name) return inner_manager_.is_transient_local(topic_name); } +bool Ros2SubscriptionManager::is_subscribed(const std::string& topic_name) const { + return inner_manager_.is_subscribed(topic_name); +} + } // namespace pj_bridge diff --git a/tests/unit/test_bridge_server.cpp b/tests/unit/test_bridge_server.cpp index 6657a0f..f7abe3a 100644 --- a/tests/unit/test_bridge_server.cpp +++ b/tests/unit/test_bridge_server.cpp @@ -72,6 +72,7 @@ class MockMiddleware : public MiddlewareInterface { } bool send_reply(const std::string& client_identity, const std::vector& data) override { + log_send(SendKind::kText, client_identity); std::lock_guard lock(reply_mutex_); replies_.emplace_back(client_identity, data); return true; @@ -82,6 +83,7 @@ class MockMiddleware : public MiddlewareInterface { } bool send_binary(const std::string& client_identity, const std::vector& data) override { + log_send(SendKind::kBinary, client_identity); std::lock_guard lock(binary_mutex_); binary_sends_.emplace_back(client_identity, data); return true; @@ -178,7 +180,28 @@ class MockMiddleware : public MiddlewareInterface { replies_.clear(); } + /// Kind of frame recorded in the combined send log. + enum class SendKind { kText, kBinary }; + + /// Combined, ordered log of every send_reply/send_binary call + /// (kind, client_id) — lets tests assert relative ordering across the + /// two send types. + std::vector> get_send_log() { + std::lock_guard lock(send_log_mutex_); + return send_log_; + } + + void clear_send_log() { + std::lock_guard lock(send_log_mutex_); + send_log_.clear(); + } + private: + void log_send(SendKind kind, const std::string& client_identity) { + std::lock_guard lock(send_log_mutex_); + send_log_.emplace_back(kind, client_identity); + } + bool ready_{false}; std::mutex queue_mutex_; @@ -190,6 +213,9 @@ class MockMiddleware : public MiddlewareInterface { std::mutex binary_mutex_; std::vector>> binary_sends_; + std::mutex send_log_mutex_; + std::vector> send_log_; + std::mutex cb_mutex_; ConnectionCallback on_connect_; ConnectionCallback on_disconnect_; @@ -316,7 +342,9 @@ class MockSubscriptionManager : public SubscriptionManagerInterface { } } - bool is_subscribed(const std::string& topic_name) const { + // Overrides the interface default (false): reports true while the topic + // holds at least one ref, mirroring the real backend managers. + bool is_subscribed(const std::string& topic_name) const override { std::lock_guard lock(mutex_); auto it = ref_counts_.find(topic_name); return it != ref_counts_.end() && it->second > 0; @@ -2468,6 +2496,7 @@ TEST_F(BridgeServerTest, LatchedTopicReplaysToLateSubscriberOnly) { // the regular publish cycle (it is delivered to client_a here). server_->publish_aggregated_messages(); mock_->clear_binary_sends(); + mock_->clear_send_log(); // Client B subscribes later, joining the already-shared subscription. mock_->push_request("client_b", sub.dump()); @@ -2487,6 +2516,19 @@ TEST_F(BridgeServerTest, LatchedTopicReplaysToLateSubscriberOnly) { EXPECT_EQ(messages[0].topic, "/latched"); EXPECT_EQ(messages[0].timestamp_ns, 12345u); EXPECT_EQ(messages[0].data, payload); + + // Ordering: the subscribe RESPONSE (text, carrying the schema needed to + // decode) must reach client_b BEFORE the replay binary frame. + auto send_log = mock_->get_send_log(); + std::vector b_sends; + for (const auto& [kind, cid] : send_log) { + if (cid == "client_b") { + b_sends.push_back(kind); + } + } + ASSERT_EQ(b_sends.size(), 2u); + EXPECT_EQ(b_sends[0], MockMiddleware::SendKind::kText); + EXPECT_EQ(b_sends[1], MockMiddleware::SendKind::kBinary); } // SubscribeToNonLatchedTopicSendsNoImmediateFrame @@ -2523,3 +2565,108 @@ TEST_F(BridgeServerTest, SubscribeToNonLatchedTopicSendsNoImmediateFrame) { EXPECT_TRUE(mock_->get_binary_sends().empty()); } + +// LatchedReplaySkippedWhileSampleStillBuffered +// +// If the retained sample is still sitting in the normal message buffer (it +// arrived recently and has not been drained yet), the regular publish cycle +// will deliver it to the new subscriber anyway — a replay frame would +// duplicate it, so none must be sent. +TEST_F(BridgeServerTest, LatchedReplaySkippedWhileSampleStillBuffered) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/latched", "std_msgs/msg/String"}}); + mock_sub_manager_->add_known_topic("/latched"); + mock_sub_manager_->set_transient_local("/latched", true); + + json sub; + sub["command"] = "subscribe"; + sub["topics"] = json::array({"/latched"}); + + // Client A subscribes, then a message arrives — but the buffer is NOT + // drained before client B subscribes. + mock_->push_request("client_a", sub.dump()); + server_->process_requests(); + mock_->pop_reply("client_a"); + + std::vector payload = {7, 8, 9}; + auto data = std::make_shared>(payload.size()); + for (size_t i = 0; i < payload.size(); ++i) { + (*data)[i] = static_cast(payload[i]); + } + mock_sub_manager_->deliver_message("/latched", data, 555); + mock_->clear_binary_sends(); + + // Client B subscribes while the sample is still in the normal buffer: + // no replay frame. + mock_->push_request("client_b", sub.dump()); + server_->process_requests(); + json resp_b = mock_->pop_reply("client_b"); + ASSERT_FALSE(resp_b.is_discarded()); + EXPECT_EQ(resp_b["status"], "success"); + EXPECT_TRUE(mock_->get_binary_sends().empty()); + + // The regular publish cycle then delivers the sample to client B (and A) + // through the normal aggregated path — exactly once, no duplicate. + server_->publish_aggregated_messages(); + bool b_received_via_normal_path = false; + for (const auto& [cid, frame] : mock_->get_binary_sends()) { + if (cid != "client_b") { + continue; + } + auto messages = decode_frame(frame); + ASSERT_EQ(messages.size(), 1u); + EXPECT_EQ(messages[0].topic, "/latched"); + EXPECT_EQ(messages[0].data, payload); + EXPECT_FALSE(b_received_via_normal_path) << "duplicate frame for client_b"; + b_received_via_normal_path = true; + } + EXPECT_TRUE(b_received_via_normal_path); +} + +// LastRefReleaseClearsLatchedSample +// +// When the last reference to a latched topic is released, the middleware +// subscription is destroyed and the retained sample goes stale. It must be +// cleared: the NEXT subscriber creates a fresh middleware subscription that +// receives the current sample via DDS transient_local redelivery, and must +// NOT be replayed the stale copy first. +TEST_F(BridgeServerTest, LastRefReleaseClearsLatchedSample) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/latched", "std_msgs/msg/String"}}); + mock_sub_manager_->add_known_topic("/latched"); + mock_sub_manager_->set_transient_local("/latched", true); + + json sub; + sub["command"] = "subscribe"; + sub["topics"] = json::array({"/latched"}); + + // Client A subscribes; a message arrives and is drained (retained copy + // now lives only in the latched store). + mock_->push_request("client_a", sub.dump()); + server_->process_requests(); + mock_->pop_reply("client_a"); + + auto data = std::make_shared>(3, std::byte{42}); + mock_sub_manager_->deliver_message("/latched", data, 111); + server_->publish_aggregated_messages(); + + // Client A unsubscribes — last ref, subscription destroyed, latched + // state cleared. + json unsub; + unsub["command"] = "unsubscribe"; + unsub["topics"] = json::array({"/latched"}); + mock_->push_request("client_a", unsub.dump()); + server_->process_requests(); + mock_->pop_reply("client_a"); + ASSERT_EQ(mock_sub_manager_->ref_count("/latched"), 0); + mock_->clear_binary_sends(); + + // A NEW subscriber gets NO replay frame (the retained sample was + // cleared; DDS redelivery on the fresh subscription covers it). + mock_->push_request("client_b", sub.dump()); + server_->process_requests(); + json resp_b = mock_->pop_reply("client_b"); + ASSERT_FALSE(resp_b.is_discarded()); + EXPECT_EQ(resp_b["status"], "success"); + EXPECT_TRUE(mock_->get_binary_sends().empty()); +} diff --git a/tests/unit/test_message_buffer.cpp b/tests/unit/test_message_buffer.cpp index 6e2eab1..6e7a8e1 100644 --- a/tests/unit/test_message_buffer.cpp +++ b/tests/unit/test_message_buffer.cpp @@ -473,3 +473,40 @@ TEST_F(MessageBufferTest, ClearAlsoClearsLatchedStorage) { buffer_.add_message("/t", 2, create_test_data({4, 5, 6})); EXPECT_FALSE(buffer_.get_latched("/t").has_value()); } + +TEST_F(MessageBufferTest, GetLatchedForReplaySkipsWhileSampleStillBuffered) { + buffer_.set_latched("/t", true); + buffer_.add_message("/t", 1, create_test_data({1, 2, 3})); + + // The sample is still in the normal buffer: the regular publish cycle + // will deliver it, so the replay accessor must return nothing (a replay + // now would duplicate the message). The raw accessor still sees it. + EXPECT_FALSE(buffer_.get_latched_for_replay("/t").has_value()); + EXPECT_TRUE(buffer_.get_latched("/t").has_value()); + + // Once drained by move_messages, the retained copy becomes replayable. + std::unordered_map> messages; + buffer_.move_messages(messages); + auto replay = buffer_.get_latched_for_replay("/t"); + ASSERT_TRUE(replay.has_value()); + EXPECT_EQ(extract_data(replay->data), (std::vector{1, 2, 3})); +} + +TEST_F(MessageBufferTest, GetLatchedForReplayAvailableAfterTtlEviction) { + uint64_t fake_now = 10'000'000'000ULL; + MessageBuffer buffer(MessageBuffer::kDefaultMaxMessageAgeNs, [&fake_now]() { return fake_now; }); + + buffer.set_latched("/t", true); + buffer.add_message("/t", 1, create_test_data({4, 5, 6})); + EXPECT_FALSE(buffer.get_latched_for_replay("/t").has_value()); + + // TTL eviction (triggered by an add on another topic) removes the sample + // from the normal buffer without draining it to any client — the + // retained copy must then be available for replay. + fake_now += 2'000'000'000ULL; + buffer.add_message("/other", 2, create_test_data({9})); + + auto replay = buffer.get_latched_for_replay("/t"); + ASSERT_TRUE(replay.has_value()); + EXPECT_EQ(extract_data(replay->data), (std::vector{4, 5, 6})); +} From 9f6f467fde918f590c510e52737a08515913e596 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 15:48:17 +0200 Subject: [PATCH 13/16] feat: optional TLS (wss://) via OpenSSL server certificate Co-Authored-By: Claude Fable 5 --- CMakeLists.txt | 7 +- .../middleware/websocket_middleware.hpp | 12 ++- app/src/middleware/websocket_middleware.cpp | 31 +++++- docs/API.md | 38 +++++++ fastdds/src/main.cpp | 19 +++- ros2/src/main.cpp | 24 ++++- rti/src/main.cpp | 19 +++- tests/unit/test_websocket_middleware.cpp | 102 ++++++++++++++++++ 8 files changed, 243 insertions(+), 9 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index b86a757..41c34a7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -53,6 +53,8 @@ find_package(ZSTD REQUIRED) include(FetchContent) # IXWebSocket — try system/conda first, fall back to FetchContent +option(PJ_BRIDGE_TLS "Build IXWebSocket with TLS (OpenSSL) support" ON) + find_package(ixwebsocket QUIET) if(NOT ixwebsocket_FOUND) message(STATUS "IXWebSocket not found — fetching via FetchContent") @@ -61,7 +63,10 @@ if(NOT ixwebsocket_FOUND) URL https://github.com/machinezone/IXWebSocket/archive/refs/tags/v11.4.6.tar.gz URL_HASH SHA256=c024334f8e45980836c67008979a884d6dcc5ef067dd2eb1fa7241f4c17ddc32 ) - set(USE_TLS OFF CACHE BOOL "" FORCE) + set(USE_TLS ${PJ_BRIDGE_TLS} CACHE BOOL "" FORCE) + if(PJ_BRIDGE_TLS) + set(USE_OPEN_SSL ON CACHE BOOL "" FORCE) + endif() set(USE_ZLIB ON CACHE BOOL "" FORCE) set(IXWEBSOCKET_INSTALL OFF CACHE BOOL "" FORCE) FetchContent_MakeAvailable(ixwebsocket) diff --git a/app/include/pj_bridge/middleware/websocket_middleware.hpp b/app/include/pj_bridge/middleware/websocket_middleware.hpp index 9e42c81..315af6f 100644 --- a/app/include/pj_bridge/middleware/websocket_middleware.hpp +++ b/app/include/pj_bridge/middleware/websocket_middleware.hpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -37,9 +38,17 @@ namespace pj_bridge { +/// Server-side TLS configuration (certificate + private key paths). Both +/// files are validated for existence/readability in initialize(), before any +/// IXWebSocket TLS API is touched. +struct TlsConfig { + std::string certfile; + std::string keyfile; +}; + class WebSocketMiddleware : public MiddlewareInterface { public: - explicit WebSocketMiddleware(size_t client_backlog_size = 100); + explicit WebSocketMiddleware(size_t client_backlog_size = 100, std::optional tls = std::nullopt); ~WebSocketMiddleware() override; WebSocketMiddleware(const WebSocketMiddleware&) = delete; @@ -89,6 +98,7 @@ class WebSocketMiddleware : public MiddlewareInterface { uint64_t dropped_from_disconnected_{0}; size_t client_backlog_size_; + std::optional tls_; ConnectionCallback on_connect_; ConnectionCallback on_disconnect_; diff --git a/app/src/middleware/websocket_middleware.cpp b/app/src/middleware/websocket_middleware.cpp index 45cf780..9ed3830 100644 --- a/app/src/middleware/websocket_middleware.cpp +++ b/app/src/middleware/websocket_middleware.cpp @@ -22,13 +22,14 @@ #include #include +#include #include #include namespace pj_bridge { -WebSocketMiddleware::WebSocketMiddleware(size_t client_backlog_size) - : client_backlog_size_(client_backlog_size), initialized_(false) {} +WebSocketMiddleware::WebSocketMiddleware(size_t client_backlog_size, std::optional tls) + : client_backlog_size_(client_backlog_size), tls_(std::move(tls)), initialized_(false) {} WebSocketMiddleware::~WebSocketMiddleware() { shutdown(); @@ -53,8 +54,34 @@ tl::expected WebSocketMiddleware::initialize(uint16_t port) { return tl::unexpected("WebSocket middleware already initialized"); } + if (tls_.has_value()) { +#ifndef IXWEBSOCKET_USE_TLS + return tl::unexpected("TLS requested but IXWebSocket was built without TLS support"); +#else + std::ifstream cert_stream(tls_->certfile); + if (!cert_stream.good()) { + return tl::unexpected("TLS certificate file not found or not readable: " + tls_->certfile); + } + std::ifstream key_stream(tls_->keyfile); + if (!key_stream.good()) { + return tl::unexpected("TLS key file not found or not readable: " + tls_->keyfile); + } +#endif + } + server_ = std::make_shared(port, "0.0.0.0"); +#ifdef IXWEBSOCKET_USE_TLS + if (tls_.has_value()) { + ix::SocketTLSOptions tls_options; + tls_options.tls = true; + tls_options.certFile = tls_->certfile; + tls_options.keyFile = tls_->keyfile; + tls_options.caFile = "NONE"; + server_->setTLSOptions(tls_options); + } +#endif + server_->setOnClientMessageCallback([this]( std::shared_ptr connection_state, ix::WebSocket& web_socket, const ix::WebSocketMessagePtr& msg) { diff --git a/docs/API.md b/docs/API.md index 9ed7cb9..f023394 100644 --- a/docs/API.md +++ b/docs/API.md @@ -405,6 +405,44 @@ configurable: - **FastDDS / RTI**: CLI flag `--client-backlog-size`, default `100`, valid range `1`-`1000000`. +## TLS / wss:// + +The bridge can optionally serve the WebSocket endpoint over TLS (`wss://`) +using a server certificate + private key (OpenSSL). Client certificate +verification is not supported — this is server-authentication only. + +TLS support depends on IXWebSocket having been built with OpenSSL +(`PJ_BRIDGE_TLS=ON`, the CMake default for FetchContent builds; a +system/conda-provided IXWebSocket must likewise have been built with TLS — +check for `IXWEBSOCKET_USE_TLS` in its exported CMake target). If TLS is +requested but the linked IXWebSocket lacks TLS support, the server fails to +start with an explicit error instead of silently falling back to plaintext. + +Configuration: + +- **ROS2**: parameters `tls` (bool, default `false`), `certfile` (string, + default `""`), `keyfile` (string, default `""`). Setting `tls: true` + without both `certfile` and `keyfile` set is a startup error. +- **FastDDS / RTI**: CLI flags `--certfile ` and `--keyfile `. + Passing one without the other is a CLI11 parse error (`->needs()`); passing + both enables TLS. + +Example: generate a self-signed certificate and start the ROS2 backend with +TLS enabled: + +```bash +openssl req -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -days 365 -nodes -subj "/CN=localhost" + +ros2 run pj_bridge pj_bridge_ros2 --ros-args \ + -p tls:=true -p certfile:=cert.pem -p keyfile:=key.pem +``` + +Clients then connect via `wss://` instead of `ws://` (e.g. +`wss://127.0.0.1:9090`). With a self-signed certificate, the client must +either trust the certificate explicitly or disable peer verification (e.g. +IXWebSocket's `caFile = "NONE"`) — production deployments should use a +certificate signed by a trusted CA instead. + ## Binary Message Format Binary frames consist of a fixed 16-byte header followed by ZSTD-compressed payload. diff --git a/fastdds/src/main.cpp b/fastdds/src/main.cpp index 3c549fc..3e04920 100644 --- a/fastdds/src/main.cpp +++ b/fastdds/src/main.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -41,6 +42,8 @@ int main(int argc, char* argv[]) { std::vector topic_whitelist{".*"}; double topic_poll_interval = 1.0; int client_backlog_size = 100; + std::string certfile; + std::string keyfile; app.add_option("--domains,-d", domain_ids, "DDS domain IDs")->required()->expected(1, -1); app.add_option("--port,-p", port, "WebSocket port")->default_val(9090)->check(CLI::Range(1, 65535)); @@ -63,8 +66,16 @@ int main(int argc, char* argv[]) { // single joined string, which is not what we want here). app.add_option("--topic-whitelist", topic_whitelist, "Topic whitelist regex patterns (full-match, ECMAScript)"); + auto certfile_opt = + app.add_option("--certfile", certfile, "TLS server certificate file (enables wss://, requires --keyfile)"); + auto keyfile_opt = app.add_option("--keyfile", keyfile, "TLS private key file (enables wss://, requires --certfile)"); + certfile_opt->needs(keyfile_opt); + keyfile_opt->needs(certfile_opt); + CLI11_PARSE(app, argc, argv); + bool tls_enabled = !certfile.empty(); + spdlog::set_level(spdlog::level::from_str(log_level)); spdlog::info("pj_bridge (FastDDS backend) starting..."); @@ -75,6 +86,7 @@ int main(int argc, char* argv[]) { spdlog::info(" Topic whitelist: {}", fmt::join(topic_whitelist, ", ")); spdlog::info(" Topic poll interval: {:.1f} s", topic_poll_interval); spdlog::info(" Client backlog size: {}", client_backlog_size); + spdlog::info(" TLS: {}", tls_enabled ? "enabled" : "disabled"); auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); if (!whitelist_result) { @@ -90,7 +102,12 @@ int main(int argc, char* argv[]) { try { auto topic_source = std::make_shared(domain_ids); auto sub_manager = std::make_shared(*topic_source); - auto middleware = std::make_shared(static_cast(client_backlog_size)); + std::optional tls_config; + if (tls_enabled) { + tls_config = pj_bridge::TlsConfig{certfile, keyfile}; + } + auto middleware = + std::make_shared(static_cast(client_backlog_size), tls_config); pj_bridge::BridgeServer server( topic_source, sub_manager, middleware, port, session_timeout, publish_rate, diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index 48b4888..4ae111d 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -48,6 +49,9 @@ int main(int argc, char** argv) { node->declare_parameter("max_qos_depth", 100); node->declare_parameter("topic_poll_interval", 1.0); node->declare_parameter("client_backlog_size", 100); + node->declare_parameter("tls", false); + node->declare_parameter("certfile", ""); + node->declare_parameter("keyfile", ""); int port = node->get_parameter("port").as_int(); double publish_rate = node->get_parameter("publish_rate").as_double(); @@ -58,13 +62,22 @@ int main(int argc, char** argv) { int64_t max_qos_depth = node->get_parameter("max_qos_depth").as_int(); double topic_poll_interval = node->get_parameter("topic_poll_interval").as_double(); int64_t client_backlog_size = node->get_parameter("client_backlog_size").as_int(); + bool tls_enabled = node->get_parameter("tls").as_bool(); + std::string certfile = node->get_parameter("certfile").as_string(); + std::string keyfile = node->get_parameter("keyfile").as_string(); RCLCPP_INFO( node->get_logger(), "Configuration: port=%d, publish_rate=%.1f Hz, session_timeout=%.1f s, strip_large_messages=%s, " - "min_qos_depth=%ld, max_qos_depth=%ld, topic_poll_interval=%.1f s, client_backlog_size=%ld", + "min_qos_depth=%ld, max_qos_depth=%ld, topic_poll_interval=%.1f s, client_backlog_size=%ld, tls=%s", port, publish_rate, session_timeout, strip_large_messages ? "true" : "false", min_qos_depth, max_qos_depth, - topic_poll_interval, client_backlog_size); + topic_poll_interval, client_backlog_size, tls_enabled ? "true" : "false"); + + if (tls_enabled && (certfile.empty() || keyfile.empty())) { + RCLCPP_ERROR(node->get_logger(), "tls=true requires both 'certfile' and 'keyfile' parameters to be set"); + rclcpp::shutdown(); + return 1; + } if (topic_poll_interval < 0.0) { RCLCPP_ERROR( @@ -102,7 +115,12 @@ int main(int argc, char** argv) { auto topic_source = std::make_shared(node); auto sub_manager = std::make_shared( node, strip_large_messages, static_cast(min_qos_depth), static_cast(max_qos_depth)); - auto middleware = std::make_shared(static_cast(client_backlog_size)); + std::optional tls_config; + if (tls_enabled) { + tls_config = pj_bridge::TlsConfig{certfile, keyfile}; + } + auto middleware = + std::make_shared(static_cast(client_backlog_size), tls_config); // Create bridge server pj_bridge::BridgeServer server( diff --git a/rti/src/main.cpp b/rti/src/main.cpp index dc3d61c..396fdd8 100644 --- a/rti/src/main.cpp +++ b/rti/src/main.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -42,6 +43,8 @@ int main(int argc, char* argv[]) { std::vector topic_whitelist{".*"}; double topic_poll_interval = 1.0; int client_backlog_size = 100; + std::string certfile; + std::string keyfile; app.add_option("--domains,-d", domain_ids, "DDS domain IDs")->required()->expected(1, -1); app.add_option("--port,-p", port, "WebSocket port")->default_val(9090)->check(CLI::Range(1, 65535)); @@ -65,8 +68,16 @@ int main(int argc, char* argv[]) { // single joined string, which is not what we want here). app.add_option("--topic-whitelist", topic_whitelist, "Topic whitelist regex patterns (full-match, ECMAScript)"); + auto certfile_opt = + app.add_option("--certfile", certfile, "TLS server certificate file (enables wss://, requires --keyfile)"); + auto keyfile_opt = app.add_option("--keyfile", keyfile, "TLS private key file (enables wss://, requires --certfile)"); + certfile_opt->needs(keyfile_opt); + keyfile_opt->needs(certfile_opt); + CLI11_PARSE(app, argc, argv); + bool tls_enabled = !certfile.empty(); + spdlog::set_level(spdlog::level::from_str(log_level)); spdlog::info("pj_bridge (RTI DDS backend) starting..."); @@ -80,6 +91,7 @@ int main(int argc, char* argv[]) { spdlog::info(" Topic whitelist: {}", fmt::join(topic_whitelist, ", ")); spdlog::info(" Topic poll interval: {:.1f} s", topic_poll_interval); spdlog::info(" Client backlog size: {}", client_backlog_size); + spdlog::info(" TLS: {}", tls_enabled ? "enabled" : "disabled"); auto whitelist_result = pj_bridge::WhitelistFilter::create(topic_whitelist); if (!whitelist_result) { @@ -95,7 +107,12 @@ int main(int argc, char* argv[]) { try { auto topic_source = std::make_shared(domain_ids, qos_profile); auto sub_manager = std::make_shared(*topic_source); - auto middleware = std::make_shared(static_cast(client_backlog_size)); + std::optional tls_config; + if (tls_enabled) { + tls_config = pj_bridge::TlsConfig{certfile, keyfile}; + } + auto middleware = + std::make_shared(static_cast(client_backlog_size), tls_config); pj_bridge::BridgeServer server( topic_source, sub_manager, middleware, port, session_timeout, publish_rate, diff --git a/tests/unit/test_websocket_middleware.cpp b/tests/unit/test_websocket_middleware.cpp index 8357eb3..7de70fd 100644 --- a/tests/unit/test_websocket_middleware.cpp +++ b/tests/unit/test_websocket_middleware.cpp @@ -22,6 +22,8 @@ #include #include +#include +#include #include #include #include @@ -518,6 +520,106 @@ TEST(WebSocketMiddlewareBacklogConstructionTest, ExplicitBacklogSizeConstructsAn middleware.shutdown(); } +// --------------------------------------------------------------------------- +// Task 6 — TLS (wss://) via OpenSSL, server certificate only +// --------------------------------------------------------------------------- + +TEST(WebSocketMiddlewareTlsTest, MissingCertFileFailsInitializeWithPathInMessage) { + pj_bridge::TlsConfig tls{"/nonexistent/cert.pem", "/nonexistent/key.pem"}; + WebSocketMiddleware middleware(100, tls); + + auto result = middleware.initialize(18099); + ASSERT_FALSE(result.has_value()); + EXPECT_NE(result.error().find("/nonexistent/cert.pem"), std::string::npos) + << "Error message should mention the missing cert file path: " << result.error(); +} + +#ifdef IXWEBSOCKET_USE_TLS +namespace { + +// Generates a throwaway self-signed cert/key pair under `dir` using the +// system `openssl` binary. Returns true on success; the caller should +// GTEST_SKIP() when this fails (missing openssl binary, sandboxed +// environment, etc.) rather than fail the test outright. +bool generate_self_signed_cert(const std::string& certfile, const std::string& keyfile) { + std::string cmd = "openssl req -x509 -newkey rsa:2048 -keyout " + keyfile + " -out " + certfile + + " -days 1 -nodes -subj /CN=localhost 2>/dev/null"; + int rc = std::system(cmd.c_str()); + return rc == 0 && std::filesystem::exists(certfile) && std::filesystem::exists(keyfile); +} + +} // namespace + +TEST(WebSocketMiddlewareTlsTest, TlsRoundTrip) { + auto dir = std::filesystem::temp_directory_path() / "pj_bridge_tls_test"; + std::filesystem::create_directories(dir); + std::string certfile = (dir / "cert.pem").string(); + std::string keyfile = (dir / "key.pem").string(); + + if (!generate_self_signed_cert(certfile, keyfile)) { + GTEST_SKIP() << "Could not generate a self-signed certificate (openssl binary unavailable or failed)"; + } + + pj_bridge::TlsConfig tls{certfile, keyfile}; + WebSocketMiddleware middleware(100, tls); + + auto result = middleware.initialize(18199); + ASSERT_TRUE(result.has_value()) << (result.has_value() ? "" : result.error()); + ASSERT_TRUE(middleware.is_ready()); + + ix::WebSocket client; + client.setUrl("wss://127.0.0.1:18199"); + + ix::SocketTLSOptions client_tls_options; + client_tls_options.caFile = "NONE"; // self-signed cert: disable peer verification + client.setTLSOptions(client_tls_options); + + std::mutex msg_mutex; + std::condition_variable msg_cv; + std::string received_message; + bool message_received = false; + + client.setOnMessageCallback([&](const ix::WebSocketMessagePtr& msg) { + if (msg->type == ix::WebSocketMessageType::Message && !msg->binary) { + std::lock_guard lock(msg_mutex); + received_message = msg->str; + message_received = true; + msg_cv.notify_one(); + } + }); + client.start(); + + ASSERT_TRUE(wait_for_client_open(client)) << "TLS client failed to connect"; + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + + const std::string test_message = "hello over wss"; + client.send(test_message); + + std::vector data; + std::string client_id; + ASSERT_TRUE(poll_receive_request(middleware, data, client_id)) << "receive_request() never returned the message"; + std::string received(data.begin(), data.end()); + EXPECT_EQ(received, test_message); + + const std::string reply_text = "reply over wss"; + std::vector reply_data(reply_text.begin(), reply_text.end()); + EXPECT_TRUE(middleware.send_reply(client_id, reply_data)); + + { + std::unique_lock lock(msg_mutex); + ASSERT_TRUE(msg_cv.wait_for(lock, std::chrono::seconds(2), [&] { return message_received; })) + << "TLS client never received the reply"; + } + EXPECT_EQ(received_message, reply_text); + + client.stop(); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + middleware.shutdown(); + + std::filesystem::remove_all(dir); +} +#endif // IXWEBSOCKET_USE_TLS + int main(int argc, char** argv) { testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); From c270966a879014a61e1f2acf67ce4ab4663bb806 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 16:01:31 +0200 Subject: [PATCH 14/16] fix: replay latched sample on resume for topics subscribed while paused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A client that subscribes to a latched (transient_local) topic while paused acquires no middleware ref, so it got neither the replay frame nor DDS redelivery when resume joined an already-shared subscription. handle_resume now runs the same latched bookkeeping as handle_subscribe after each successful re-subscribe; the frame is flushed by process_single_request right after the resume response, preserving the response-before-frame ordering. - Factor the shared logic into BridgeServer::collect_latched_replay (set_latched + get_latched_for_replay + serialize + queue under the leaf replays_mutex_), replacing handle_subscribe's two-phase replay_candidates machinery; document the benign queue-vs-cleanup race there. - docs/API.md (TLS): note that only cert/key readability is validated at startup — a mismatched pair only surfaces as per-connection handshake failures in the server log (IXWebSocket defers TLS setup to accept time). Co-Authored-By: Claude Fable 5 --- app/include/pj_bridge/bridge_server.hpp | 21 ++++- app/src/bridge_server.cpp | 120 +++++++++++++----------- docs/API.md | 5 + tests/unit/test_bridge_server.cpp | 85 +++++++++++++++++ 4 files changed, 169 insertions(+), 62 deletions(-) diff --git a/app/include/pj_bridge/bridge_server.hpp b/app/include/pj_bridge/bridge_server.hpp index a9e2832..35fb374 100644 --- a/app/include/pj_bridge/bridge_server.hpp +++ b/app/include/pj_bridge/bridge_server.hpp @@ -159,6 +159,16 @@ class BridgeServer { /// cleanup_mutex_ held (same requirement as unsubscribe itself). void release_subscription_ref(const std::string& topic_name); + /// Latched (transient_local) replay bookkeeping for a topic whose + /// middleware subscription ref was just (re)acquired for @p client_id — + /// shared by handle_subscribe and handle_resume. Records the topic's + /// latched flag in the message buffer and, when a retained sample is + /// available for replay, serializes it into a single-message binary frame + /// queued in pending_replays_ (flushed by process_single_request right + /// after the handler's response is sent). Call with cleanup_mutex_ held; + /// acquires only the leaf replays_mutex_ and makes no middleware calls. + void collect_latched_replay(const std::string& client_id, const std::string& topic_name); + // Backend interfaces (owned) std::shared_ptr topic_source_; std::shared_ptr subscription_manager_; @@ -209,11 +219,12 @@ class BridgeServer { bool topics_snapshot_taken_{false}; std::mutex topics_mutex_; - // Latched (transient_local) replay frames queued by handle_subscribe, - // flushed by process_single_request right AFTER the subscribe response is - // sent — the client must receive the response (which carries the topic's - // schema) before the binary frame that needs it. LEAF lock (see comment - // above). + // Latched (transient_local) replay frames queued by handle_subscribe and + // handle_resume (via collect_latched_replay), flushed by + // process_single_request right AFTER the handler's response is sent — the + // client must receive the response (which carries the topic's schema on + // subscribe) before the binary frame that needs it. LEAF lock (see + // comment above). std::unordered_map>> pending_replays_; std::mutex replays_mutex_; }; diff --git a/app/src/bridge_server.cpp b/app/src/bridge_server.cpp index 3568585..d8a3a51 100644 --- a/app/src/bridge_server.cpp +++ b/app/src/bridge_server.cpp @@ -218,11 +218,12 @@ void BridgeServer::process_single_request(const std::vector& request_da return; } - // Flush any latched-topic replay frames handle_subscribe queued for this - // client. Sent strictly AFTER the response so the client has the schema - // needed to decode them (docs/API.md: "right after the subscribe - // response"). Move the frames out under the leaf replays_mutex_, release - // it, then send — never call middleware_ while holding the lock. + // Flush any latched-topic replay frames the handler queued for this + // client (handle_subscribe or handle_resume, via collect_latched_replay). + // Sent strictly AFTER the response so the client has the schema needed + // to decode them (docs/API.md: "right after the subscribe response"). + // Move the frames out under the leaf replays_mutex_, release it, then + // send — never call middleware_ while holding the lock. std::vector> replay_frames; { std::lock_guard replays_lock(replays_mutex_); @@ -366,12 +367,6 @@ std::string BridgeServer::handle_subscribe(const std::string& client_id, const n extracted_schemas[topic_name] = std::move(schema); } - // Latched (transient_local) topics whose retained sample should be - // replayed to this client. Collected under cleanup_mutex_ below but sent - // only after the lock is released (never call middleware_ while holding - // cleanup_mutex_). - std::vector> replay_candidates; - // Acquire refs and record subscriptions atomically with respect to // pause/resume and cleanup_session (which run on other threads). // Invariant: a middleware ref is held iff set_ref_held(topic, true). @@ -411,29 +406,12 @@ std::string BridgeServer::handle_subscribe(const std::string& client_id, const n schema_obj["definition"] = extracted_schemas[topic_name]; schemas[topic_name] = schema_obj; - // Latched (transient_local) topic replay: the FIRST subscriber to a - // topic gets its retained sample delivered naturally by DDS - // (transient_local redelivery on the freshly created subscription). - // This path exists for LATER clients joining an already-shared - // subscription, whose sample may have aged out of MessageBuffer's - // normal TTL window by the time they subscribe. Only meaningful when - // we actually (re)established the middleware subscription this call - // (ref_acquired) — a paused "subscribe" creates no subscription to - // query. Ordering note: set_latched runs at subscribe time, so a - // latched topic's messages are only retained once someone has - // subscribed — exactly the shared-subscription case this replay - // serves. + // Latched (transient_local) topic replay bookkeeping — only + // meaningful when we actually (re)established the middleware + // subscription this call (ref_acquired): a paused "subscribe" + // creates no subscription to query; its replay happens on resume. if (ref_acquired) { - bool is_latched = subscription_manager_->is_transient_local(topic_name); - message_buffer_->set_latched(topic_name, is_latched); - if (is_latched) { - // get_latched_for_replay skips samples still sitting in the - // normal buffer — those reach the new subscriber through the - // regular publish cycle, and replaying them too would duplicate. - if (auto latched_msg = message_buffer_->get_latched_for_replay(topic_name)) { - replay_candidates.emplace_back(topic_name, *latched_msg); - } - } + collect_latched_replay(client_id, topic_name); } spdlog::info("Client '{}' subscribed to topic '{}' (type: {})", client_id, topic_name, topic_type); @@ -448,30 +426,6 @@ std::string BridgeServer::handle_subscribe(const std::string& client_id, const n } } - // Serialize the retained latched samples now that cleanup_mutex_ is - // released, but do NOT send them here: the subscribe response (which - // carries the topic's schema) is sent by process_single_request after - // this handler returns, and the client must receive it before any binary - // frame that needs the schema to decode. Queue the frames instead; - // process_single_request flushes them right after the response goes out. - // Each candidate becomes its own single-message binary frame (mirrors the - // normal aggregated-frame format so clients need no special handling). - // Does not touch last_sent_times_: the sample's old timestamp means it - // won't interfere with subsequent rate limiting. - if (!replay_candidates.empty()) { - std::vector> frames; - frames.reserve(replay_candidates.size()); - for (const auto& [topic_name, latched_msg] : replay_candidates) { - AggregatedMessageSerializer serializer; - serializer.serialize_message( - topic_name, latched_msg.timestamp_ns, latched_msg.data->data(), latched_msg.data->size()); - frames.push_back(serializer.finalize()); - } - std::lock_guard replays_lock(replays_mutex_); - auto& pending = pending_replays_[client_id]; - pending.insert(pending.end(), std::make_move_iterator(frames.begin()), std::make_move_iterator(frames.end())); - } - // Build response json response; if (failures.empty()) { @@ -669,6 +623,11 @@ std::string BridgeServer::handle_resume(const std::string& client_id, const nloh if (subscription_manager_->subscribe(topic, type_it->second)) { session_manager_->set_ref_held(client_id, topic, true); resubscribed++; + // Same latched bookkeeping as handle_subscribe: a topic subscribed + // WHILE PAUSED acquires its first ref here, and when it joins an + // already-shared subscription DDS won't redeliver — the retained + // sample (if any) is queued and flushed after the resume response. + collect_latched_replay(client_id, topic); spdlog::debug("Re-acquired ref for topic '{}' (client '{}' resumed)", topic, client_id); } else { spdlog::warn("Topic '{}' subscription failed on resume for client '{}'", topic, client_id); @@ -826,6 +785,53 @@ void BridgeServer::check_topic_changes() { } } +void BridgeServer::collect_latched_replay(const std::string& client_id, const std::string& topic_name) { + // The FIRST subscriber to a latched topic gets its retained sample + // delivered naturally by DDS (transient_local redelivery on the freshly + // created subscription). Replay exists for LATER clients joining an + // already-shared subscription — via subscribe or via resume after + // subscribing while paused — whose sample may have aged out of + // MessageBuffer's normal TTL window by then. Ordering note: set_latched + // runs at (re)subscribe time, so a latched topic's messages are only + // retained once someone has subscribed — exactly the shared-subscription + // case the replay serves. + bool is_latched = subscription_manager_->is_transient_local(topic_name); + message_buffer_->set_latched(topic_name, is_latched); + if (!is_latched) { + return; + } + + // get_latched_for_replay skips samples still sitting in the normal + // buffer — those reach the new subscriber through the regular publish + // cycle, and replaying them too would duplicate. + auto latched_msg = message_buffer_->get_latched_for_replay(topic_name); + if (!latched_msg) { + return; + } + + // Do NOT send here: the handler's response (which on subscribe carries + // the topic's schema) is sent by process_single_request after the + // handler returns, and the client must receive it before any binary + // frame that needs the schema to decode. Queue a single-message frame + // (same format as regular aggregated frames, so clients need no special + // handling); process_single_request flushes it right after the response + // goes out. Does not touch last_sent_times_: the sample's old timestamp + // means it won't interfere with subsequent rate limiting. + // + // Benign race with cleanup_session between this queueing and the flush: + // if the client disconnects in between, cleanup_session erases the + // client's pending_replays_ entry (and a failed send_reply takes that + // same cleanup path), so the frames are either dropped with the dead + // session or sent to a still-connected socket — nothing strands. + AggregatedMessageSerializer serializer; + serializer.serialize_message( + topic_name, latched_msg->timestamp_ns, latched_msg->data->data(), latched_msg->data->size()); + auto frame = serializer.finalize(); + + std::lock_guard replays_lock(replays_mutex_); + pending_replays_[client_id].push_back(std::move(frame)); +} + void BridgeServer::release_subscription_ref(const std::string& topic_name) { subscription_manager_->unsubscribe(topic_name); // When the last ref is released the middleware subscription is destroyed. diff --git a/docs/API.md b/docs/API.md index f023394..1b7fdae 100644 --- a/docs/API.md +++ b/docs/API.md @@ -427,6 +427,11 @@ Configuration: Passing one without the other is a CLI11 parse error (`->needs()`); passing both enables TLS. +Note that only certificate/key file *readability* is validated at startup; a +mismatched certificate/key pair is not detected until clients connect, and +then only surfaces as per-connection TLS handshake failures in the server +log (IXWebSocket defers TLS setup to accept time). + Example: generate a self-signed certificate and start the ROS2 backend with TLS enabled: diff --git a/tests/unit/test_bridge_server.cpp b/tests/unit/test_bridge_server.cpp index f7abe3a..5b99601 100644 --- a/tests/unit/test_bridge_server.cpp +++ b/tests/unit/test_bridge_server.cpp @@ -2670,3 +2670,88 @@ TEST_F(BridgeServerTest, LastRefReleaseClearsLatchedSample) { EXPECT_EQ(resp_b["status"], "success"); EXPECT_TRUE(mock_->get_binary_sends().empty()); } + +// ResumeReplaysLatchedSampleForTopicSubscribedWhilePaused +// +// A client that subscribes to a latched topic WHILE PAUSED acquires no +// middleware ref (and gets no replay frame). On resume it joins the +// already-shared subscription, where DDS does not redeliver — the retained +// sample must be replayed then: resume RESPONSE first, then exactly one +// binary frame with the retained message. +TEST_F(BridgeServerTest, ResumeReplaysLatchedSampleForTopicSubscribedWhilePaused) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics({{"/latched", "std_msgs/msg/String"}}); + mock_sub_manager_->add_known_topic("/latched"); + mock_sub_manager_->set_transient_local("/latched", true); + + json sub; + sub["command"] = "subscribe"; + sub["topics"] = json::array({"/latched"}); + + // Client A subscribes (seeds the latched retention), a message arrives + // and is drained — the retained sample has "aged out" of the normal + // buffer and lives only in the latched store. + mock_->push_request("client_a", sub.dump()); + server_->process_requests(); + mock_->pop_reply("client_a"); + + std::vector payload = {3, 1, 4, 1, 5}; + auto data = std::make_shared>(payload.size()); + for (size_t i = 0; i < payload.size(); ++i) { + (*data)[i] = static_cast(payload[i]); + } + mock_sub_manager_->deliver_message("/latched", data, 777); + server_->publish_aggregated_messages(); + ASSERT_EQ(mock_sub_manager_->ref_count("/latched"), 1); + + // Client P pauses, then subscribes to the latched topic while paused: + // no ref acquired, no replay frame. + json pause_req; + pause_req["command"] = "pause"; + mock_->push_request("client_p", pause_req.dump()); + server_->process_requests(); + mock_->pop_reply("client_p"); + mock_->clear_binary_sends(); + + mock_->push_request("client_p", sub.dump()); + server_->process_requests(); + json sub_resp = mock_->pop_reply("client_p"); + ASSERT_FALSE(sub_resp.is_discarded()); + EXPECT_EQ(sub_resp["status"], "success"); + EXPECT_TRUE(mock_->get_binary_sends().empty()); + ASSERT_EQ(mock_sub_manager_->ref_count("/latched"), 1); // still only client A + + // Resume: client P joins the already-shared subscription and must be + // replayed the retained sample — resume response FIRST, then the frame. + mock_->clear_send_log(); + json resume_req; + resume_req["command"] = "resume"; + mock_->push_request("client_p", resume_req.dump()); + server_->process_requests(); + json resume_resp = mock_->pop_reply("client_p"); + ASSERT_FALSE(resume_resp.is_discarded()); + EXPECT_EQ(resume_resp["status"], "ok"); + EXPECT_FALSE(resume_resp["paused"].get()); + EXPECT_EQ(mock_sub_manager_->ref_count("/latched"), 2); + + // Exactly one binary frame, to client_p only, with the retained message. + auto binary_sends = mock_->get_binary_sends(); + ASSERT_EQ(binary_sends.size(), 1u); + EXPECT_EQ(binary_sends[0].first, "client_p"); + auto messages = decode_frame(binary_sends[0].second); + ASSERT_EQ(messages.size(), 1u); + EXPECT_EQ(messages[0].topic, "/latched"); + EXPECT_EQ(messages[0].timestamp_ns, 777u); + EXPECT_EQ(messages[0].data, payload); + + // Ordering: resume response (text) before the replay frame (binary). + std::vector p_sends; + for (const auto& [kind, cid] : mock_->get_send_log()) { + if (cid == "client_p") { + p_sends.push_back(kind); + } + } + ASSERT_EQ(p_sends.size(), 2u); + EXPECT_EQ(p_sends[0], MockMiddleware::SendKind::kText); + EXPECT_EQ(p_sends[1], MockMiddleware::SendKind::kBinary); +} From 3a0ba1dd08b73af999bc3e47b5558e11699f6e01 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 16:09:45 +0200 Subject: [PATCH 15/16] docs: document foxglove-parity features Co-Authored-By: Claude Fable 5 --- CHANGELOG.rst | 21 +++++++++++++++++++++ CLAUDE.md | 34 +++++++++++++++++++++++++--------- README.md | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 3fa9230..0448c05 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,27 @@ Changelog for package pj_ros_bridge ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +0.7.0 (2026-07-06) +------------------ +* Foxglove-parity feature set, closing the gap with foxglove_bridge's + topic-subscription features: + - Topic whitelist: full-match regex filtering of visible/subscribable + topics (``topic_whitelist`` / ``--topic-whitelist``) + - QoS depth heuristics (ROS2): KEEP_LAST subscription depth derived from + discovered publisher depths, clamped to ``min_qos_depth``/``max_qos_depth`` + - Opt-in pushed topic advertisement: ``subscribe_topic_updates`` / + ``unsubscribe_topic_updates`` commands and a ``topics_changed`` + notification, polled at ``topic_poll_interval`` + - Slow-client backpressure: bounded, drop-oldest per-client send queue + (``client_backlog_size``) instead of blocking the publish loop or + disconnecting the client + - Latched topic replay (ROS2): new subscribers to a ``TRANSIENT_LOCAL`` + topic (e.g. ``/tf_static``) immediately receive the retained last message + - TLS (``wss://``): optional server-certificate TLS via OpenSSL + (``tls``/``certfile``/``keyfile``, CMake option ``PJ_BRIDGE_TLS``) +* All changes are additive; ``protocol_version`` remains ``1`` +* 231 unit tests passing (TSAN/ASAN clean) + 0.1.0 (2026-02-11) ------------------ * **License changed to AGPL-3.0** (was Apache-2.0 in development) diff --git a/CLAUDE.md b/CLAUDE.md index b741737..66143af 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -197,7 +197,7 @@ Then, for each message in the (compressed) payload: ## Testing -### Test Count: 176 unit tests across 11 test suites +### Test Count: 231 unit tests across 11 test suites ### Commands ```bash @@ -229,22 +229,38 @@ pre-commit run -a ### ROS2 (via `--ros-args -p`): ```yaml -port: 9090 # WebSocket port -publish_rate: 50.0 # Hz -session_timeout: 10.0 # seconds -strip_large_messages: false # Opt-in: strip Image/PointCloud2/etc data fields +port: 9090 # WebSocket port +publish_rate: 50.0 # Hz +session_timeout: 10.0 # seconds +strip_large_messages: false # Opt-in: strip Image/PointCloud2/etc data fields +topic_whitelist: [".*"] # Full-match regex patterns restricting visible/subscribable topics +min_qos_depth: 1 # Minimum KEEP_LAST subscription depth after aggregating publisher depths +max_qos_depth: 100 # Maximum KEEP_LAST subscription depth after aggregating publisher depths +topic_poll_interval: 1.0 # Seconds between topics_changed notification polls; 0 disables polling +client_backlog_size: 100 # Max frames queued per slow client before dropping the oldest (must be > 0) +tls: false # Enable TLS (wss://); requires certfile and keyfile +certfile: "" # TLS server certificate file +keyfile: "" # TLS private key file ``` ### RTI (via CLI flags): ```bash -pj_bridge_rti --domains 0 1 --port 9090 --publish-rate 50 --session-timeout 10 +pj_bridge_rti --domains 0 1 --port 9090 --publish-rate 50 --session-timeout 10 \ + --topic-whitelist ".*" --topic-poll-interval 1.0 --client-backlog-size 100 \ + --certfile cert.pem --keyfile key.pem ``` ### FastDDS (via CLI flags): ```bash -pj_bridge_fastdds --domains 0 1 --port 9090 --publish-rate 50 --session-timeout 10 +pj_bridge_fastdds --domains 0 1 --port 9090 --publish-rate 50 --session-timeout 10 \ + --topic-whitelist ".*" --topic-poll-interval 1.0 --client-backlog-size 100 \ + --certfile cert.pem --keyfile key.pem ``` +See `docs/API.md` for full semantics of each option (topic whitelist matching rules, +QoS depth heuristics, `topics_changed` notification, backpressure policy, latched +replay, and TLS setup). + ## Important Design Decisions 1. **Backend-agnostic core via interfaces**: `TopicSourceInterface` and `SubscriptionManagerInterface` allow the same `BridgeServer` to work with ROS2 or RTI DDS. @@ -261,7 +277,7 @@ pj_bridge_fastdds --domains 0 1 --port 9090 --publish-rate 50 --session-timeout --- -**Last Updated**: 2026-07-02 +**Last Updated**: 2026-07-06 **Project Phase**: Unified multi-backend architecture -**Test Status**: 176 unit tests passing (all sanitizers clean) +**Test Status**: 231 unit tests passing (all sanitizers clean) **Executables**: `pj_bridge_ros2` (ROS2), `pj_bridge_rti` (RTI DDS, disabled), `pj_bridge_fastdds` (FastDDS) diff --git a/README.md b/README.md index 091dfdb..0913a82 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,12 @@ independently. - **Multi-Client Support**: Multiple clients can connect simultaneously with shared subscriptions - **Runtime Schema Discovery**: Automatic extraction of message schemas from installed ROS2 packages on the server side. - **Large Message Stripping** (opt-in): Optional stripping of large array fields (Image, PointCloud2, LaserScan, OccupancyGrid) to reduce bandwidth while preserving metadata. Disabled by default — full message data is forwarded; enable with `strip_large_messages:=true` for low-bandwidth links +- **Topic Whitelist**: Restrict which topics are visible/subscribable via full-match regex patterns (`topic_whitelist` / `--topic-whitelist`), mirroring foxglove_bridge's option of the same name +- **QoS Depth Heuristics** (ROS2 only): KEEP_LAST subscription depth is derived from the discovered publishers' depths and clamped to a configurable `[min_qos_depth, max_qos_depth]` range +- **Pushed Topic Advertisement** (opt-in): Clients can subscribe to a `topics_changed` notification instead of polling `get_topics`, at a configurable `topic_poll_interval` +- **Slow-Client Backpressure**: A bounded, drop-oldest per-client send queue (`client_backlog_size`) keeps a lagging client from blocking the publish loop or growing an unbounded backlog, instead of disconnecting it +- **Latched Topic Replay** (ROS2 only): New subscribers to a `TRANSIENT_LOCAL` topic (e.g. `/tf_static`) immediately receive the retained last message instead of waiting for the next publish +- **TLS / `wss://`** (opt-in): Serve the WebSocket endpoint over TLS with a server certificate + private key (`tls`/`--certfile`+`--keyfile`); clients connect via `wss://` instead of `ws://` ## CI Status @@ -33,14 +39,39 @@ independently. ## Configuration Parameters +### ROS2 (via `--ros-args -p`) + | Parameter | Type | Default | Description | |-----------|------|---------|-------------| | `port` | int | 9090 | WebSocket server port | | `publish_rate` | double | 50.0 | Aggregation publish rate in Hz | | `session_timeout` | double | 10.0 | Client timeout duration in seconds | | `strip_large_messages` | bool | false | Opt-in: strip large arrays from Image, PointCloud2, LaserScan, OccupancyGrid messages | +| `topic_whitelist` | string array | `[".*"]` | Full-match regex patterns (ECMAScript) restricting visible/subscribable topics | | `min_qos_depth` | int | 1 | ROS2 only: minimum KEEP_LAST subscription depth after aggregating publisher depths | | `max_qos_depth` | int | 100 | ROS2 only: maximum KEEP_LAST subscription depth after aggregating publisher depths | +| `topic_poll_interval` | double | 1.0 | Seconds between `topics_changed` notification polls; `0` disables polling | +| `client_backlog_size` | int | 100 | Max binary frames queued per slow client before the oldest is dropped (must be `> 0`) | +| `tls` | bool | false | Enable TLS (`wss://`); requires `certfile` and `keyfile` | +| `certfile` | string | `""` | TLS server certificate file | +| `keyfile` | string | `""` | TLS private key file | + +### FastDDS / RTI (via CLI flags) + +| Flag | Type | Default | Description | +|------|------|---------|-------------| +| `--domains`, `-d` | int list | (required) | DDS domain IDs | +| `--port`, `-p` | int | 9090 | WebSocket server port | +| `--publish-rate` | double | 50.0 | Aggregation publish rate in Hz | +| `--session-timeout` | double | 10.0 | Client timeout duration in seconds | +| `--log-level` | string | `info` | Log level (trace, debug, info, warn, error) | +| `--stats` | flag | off | Print statistics every 5 seconds | +| `--topic-whitelist` | string list | `.*` | Full-match regex patterns (ECMAScript), repeatable | +| `--topic-poll-interval` | double | 1.0 | Seconds between `topics_changed` notification polls; `0` disables polling | +| `--client-backlog-size` | int | 100 | Max binary frames queued per slow client before the oldest is dropped (range `1`-`1000000`) | +| `--certfile` | string | (none) | TLS server certificate file; enables `wss://`, requires `--keyfile` | +| `--keyfile` | string | (none) | TLS private key file; enables `wss://`, requires `--certfile` | +| `--qos-profile` | string | (none) | RTI only: QoS profile XML file path | ## Just "Download and Run" @@ -75,6 +106,8 @@ chmod +x pj_bridge_ros2-humble-x86_64.AppImage All dependencies (spdlog, nlohmann_json, ZSTD) are provided by the dependency manager. IXWebSocket is resolved via `find_package` first, with a FetchContent fallback for colcon builds. Only `tl::expected` is vendored. +TLS (`wss://`) support depends on IXWebSocket being built with OpenSSL. The CMake option `PJ_BRIDGE_TLS` (default `ON`) controls this for the FetchContent path (`-DPJ_BRIDGE_TLS=OFF` to disable); a system/conda-provided IXWebSocket must likewise have been built with TLS. See [docs/API.md](docs/API.md#tls--wss) for details. + ### ROS2 — Pixi [Pixi](https://pixi.sh) manages the full toolchain including ROS2 via [RoboStack](https://robostack.github.io/). From 894851f4ade58c492036363fd8358fc9ec059c0f Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Mon, 6 Jul 2026 16:22:46 +0200 Subject: [PATCH 16/16] fix: discard middleware backpressure backlog on session cleanup When a session died server-side (heartbeat timeout) while the socket stayed open, WebSocketMiddleware's pending_frames_/last_drop_warn_ entries for that client survived: a timed-out slow client parked up to client_backlog_size frames indefinitely, and stale frames could flush into a new logical session on the same connection. - MiddlewareInterface: new drop_pending(client) hook, default no-op. - WebSocketMiddleware: override folds the client's dropped_total() into the lifetime counter and erases its queue + warn-throttle entry (the socket itself is untouched). - BridgeServer::cleanup_session: call drop_pending after the cleanup_mutex_ scope (middleware calls must not run under that lock). - docs/API.md: correct backlog flush wording (delivered on next send attempt, no background flush), note frames in flight may arrive after an unsubscribe response, document latched replay on resume and that replay frames share the backpressure queue. Co-Authored-By: Claude Fable 5 --- .../middleware/middleware_interface.hpp | 5 ++ .../middleware/websocket_middleware.hpp | 1 + app/src/bridge_server.cpp | 53 +++++++++++-------- app/src/middleware/websocket_middleware.cpp | 15 ++++++ docs/API.md | 29 ++++++++-- tests/unit/test_bridge_server.cpp | 52 ++++++++++++++++++ tests/unit/test_websocket_middleware.cpp | 43 +++++++++++++++ 7 files changed, 171 insertions(+), 27 deletions(-) diff --git a/app/include/pj_bridge/middleware/middleware_interface.hpp b/app/include/pj_bridge/middleware/middleware_interface.hpp index abda272..ec62b2e 100644 --- a/app/include/pj_bridge/middleware/middleware_interface.hpp +++ b/app/include/pj_bridge/middleware/middleware_interface.hpp @@ -68,6 +68,11 @@ class MiddlewareInterface { /// @return true if the message was sent, false if the client is gone. virtual bool send_binary(const std::string& client_identity, const std::vector& data) = 0; + /// Discard any queued outbound data for this client (e.g. when its session + /// is destroyed server-side while the socket stays open). Default no-op for + /// implementations without per-client outbound queues. + virtual void drop_pending(const std::string& /*client_identity*/) {} + /// @return true if initialize() succeeded and shutdown() has not been called. virtual bool is_ready() const = 0; diff --git a/app/include/pj_bridge/middleware/websocket_middleware.hpp b/app/include/pj_bridge/middleware/websocket_middleware.hpp index 315af6f..f1c1da4 100644 --- a/app/include/pj_bridge/middleware/websocket_middleware.hpp +++ b/app/include/pj_bridge/middleware/websocket_middleware.hpp @@ -65,6 +65,7 @@ class WebSocketMiddleware : public MiddlewareInterface { bool is_ready() const override; void set_on_connect(ConnectionCallback callback) override; void set_on_disconnect(ConnectionCallback callback) override; + void drop_pending(const std::string& client_identity) override; /// Total number of frames dropped due to slow-client backpressure, summed /// across all clients (currently connected and already disconnected). diff --git a/app/src/bridge_server.cpp b/app/src/bridge_server.cpp index d8a3a51..ffc1b34 100644 --- a/app/src/bridge_server.cpp +++ b/app/src/bridge_server.cpp @@ -845,35 +845,44 @@ void BridgeServer::release_subscription_ref(const std::string& topic_name) { } void BridgeServer::cleanup_session(const std::string& client_id) { - std::lock_guard lock(cleanup_mutex_); - - // Drop any undelivered latched-replay frames regardless of session state - // (the client is gone; leaving them queued would strand them forever). { - std::lock_guard replays_lock(replays_mutex_); - pending_replays_.erase(client_id); - } + std::lock_guard lock(cleanup_mutex_); - if (!session_manager_->session_exists(client_id)) { - return; - } + // Drop any undelivered latched-replay frames regardless of session state + // (the client is gone; leaving them queued would strand them forever). + { + std::lock_guard replays_lock(replays_mutex_); + pending_replays_.erase(client_id); + } - // Release exactly the refs this session holds. Paused clients hold none; - // there is no separate paused check to race against. - auto held_refs = session_manager_->get_ref_held_topics(client_id); - for (const auto& topic : held_refs) { - release_subscription_ref(topic); - spdlog::debug("Unsubscribed client '{}' from topic '{}'", client_id, topic); - } + if (session_manager_->session_exists(client_id)) { + // Release exactly the refs this session holds. Paused clients hold + // none; there is no separate paused check to race against. + auto held_refs = session_manager_->get_ref_held_topics(client_id); + for (const auto& topic : held_refs) { + release_subscription_ref(topic); + spdlog::debug("Unsubscribed client '{}' from topic '{}'", client_id, topic); + } - session_manager_->remove_session(client_id); + session_manager_->remove_session(client_id); - { - std::lock_guard sent_lock(last_sent_mutex_); - last_sent_times_.erase(client_id); + { + std::lock_guard sent_lock(last_sent_mutex_); + last_sent_times_.erase(client_id); + } + + spdlog::info("Cleaned up session for client '{}' ({} topic refs released)", client_id, held_refs.size()); + } } - spdlog::info("Cleaned up session for client '{}' ({} topic refs released)", client_id, held_refs.size()); + // Discard any frames the middleware's slow-client backpressure queue still + // holds for this client. On a heartbeat timeout the socket stays open, so + // without this a timed-out slow client parks frames indefinitely and stale + // data could flush into a new logical session on the same connection. + // Deliberately outside the cleanup_mutex_ scope: middleware calls must not + // run under that lock. Also deliberately unconditional (even when no + // session existed): drop_pending is an idempotent no-op then. + middleware_->drop_pending(client_id); } size_t BridgeServer::get_active_session_count() const { diff --git a/app/src/middleware/websocket_middleware.cpp b/app/src/middleware/websocket_middleware.cpp index 9ed3830..2e920dc 100644 --- a/app/src/middleware/websocket_middleware.cpp +++ b/app/src/middleware/websocket_middleware.cpp @@ -410,6 +410,21 @@ bool WebSocketMiddleware::send_binary(const std::string& client_identity, const return true; } +void WebSocketMiddleware::drop_pending(const std::string& client_identity) { + // The client's logical session was destroyed server-side (e.g. heartbeat + // timeout) while its socket may still be open. Discard its backpressure + // backlog so stale frames can't flush into a future session on the same + // connection, folding the drop count into the lifetime total. The socket + // itself is untouched: clients_ still owns it until the real disconnect. + std::lock_guard lock(clients_mutex_); + auto pending_it = pending_frames_.find(client_identity); + if (pending_it != pending_frames_.end()) { + dropped_from_disconnected_ += pending_it->second.dropped_total(); + pending_frames_.erase(pending_it); + } + last_drop_warn_.erase(client_identity); +} + uint64_t WebSocketMiddleware::dropped_frame_count() const { std::lock_guard lock(clients_mutex_); uint64_t total = dropped_from_disconnected_; diff --git a/docs/API.md b/docs/API.md index 1b7fdae..a6acc47 100644 --- a/docs/API.md +++ b/docs/API.md @@ -235,11 +235,20 @@ later subscribers. When the last subscriber of a latched topic leaves with the underlying subscription: the next subscriber gets the current sample from DDS redelivery, never a stale copy. +Replay also happens on `resume` for latched topics whose subscription +reference is re-acquired at that point (i.e. topics that were subscribed +while the client was paused): the replay frame is delivered right after the +resume response, exactly like the post-subscribe case. + No replay frame is sent when the retained message is still pending in the regular aggregation buffer — in that case the next aggregated frame delivers it to the new subscriber anyway, and a replay would duplicate it. Replay frames intentionally bypass per-topic rate limiting (`max_rate_hz`) and are -not counted in the server's publish statistics. +not counted in the server's publish statistics. They are, however, subject +to the same [slow-client backpressure](#slow-clients--backpressure) queueing +as regular aggregated frames: a replay destined for an already-slow client +may be delayed until the next send attempt for that client (and, like any +queued frame, can be dropped if the backlog overflows). ## Unsubscribe @@ -392,10 +401,20 @@ that client are held in a small per-client queue rather than sent immediately. If that queue is already full, the **oldest** queued frame is dropped to make room for the newest one, so a lagging client always eventually resumes with fresh data instead of a growing backlog of stale -frames. Queued frames are flushed, in order, as soon as the client's socket -buffer drains back below the watermark. The JSON control-plane (requests and -their replies, including `heartbeat`) is never affected — those messages are -always sent immediately. +frames. Queued frames are delivered, in order, on the **next send attempt** +for that client once its socket buffer has drained back below the watermark +(there is no background flush timer — if the client's topics go quiet, the +backlog waits for the next frame destined for that client). The JSON +control-plane (requests and their replies, including `heartbeat`) is never +affected — those messages are always sent immediately. If a client's session +times out server-side while its socket stays open, any backlog still queued +for it is discarded. + +Frames already queued (or already sitting in the socket's send buffer) when +an `unsubscribe` is processed may still arrive **after** the unsubscribe +response — clients must tolerate binary messages for recently-unsubscribed +topics. This is inherent to socket buffering, not specific to the backlog +queue. The queue depth (max frames held per client before the oldest is dropped) is configurable: diff --git a/tests/unit/test_bridge_server.cpp b/tests/unit/test_bridge_server.cpp index 5b99601..2f00921 100644 --- a/tests/unit/test_bridge_server.cpp +++ b/tests/unit/test_bridge_server.cpp @@ -103,8 +103,19 @@ class MockMiddleware : public MiddlewareInterface { on_disconnect_ = std::move(callback); } + void drop_pending(const std::string& client_identity) override { + std::lock_guard lock(drop_pending_mutex_); + drop_pending_calls_.push_back(client_identity); + } + // --- Test helpers --- + /// Return the client ids drop_pending() was invoked for, in call order. + std::vector get_drop_pending_calls() { + std::lock_guard lock(drop_pending_mutex_); + return drop_pending_calls_; + } + /// Push a text request as if it came from a connected client. void push_request(const std::string& client_id, const std::string& text) { std::lock_guard lock(queue_mutex_); @@ -219,6 +230,9 @@ class MockMiddleware : public MiddlewareInterface { std::mutex cb_mutex_; ConnectionCallback on_connect_; ConnectionCallback on_disconnect_; + + std::mutex drop_pending_mutex_; + std::vector drop_pending_calls_; }; // --------------------------------------------------------------------------- @@ -555,6 +569,44 @@ TEST_F(BridgeServerTest, CleanupSessionIdempotent) { EXPECT_EQ(server_->get_active_session_count(), 0u); } +// --------------------------------------------------------------------------- +// 6b. CleanupSessionDropsPendingMiddlewareFrames +// +// When a session is destroyed server-side (heartbeat timeout or disconnect), +// cleanup_session must tell the middleware to discard any outbound frames +// still parked in its slow-client backpressure queue for that client — +// otherwise a timed-out client whose socket stays open leaks its backlog, +// and stale frames could flush into a new logical session on the same +// connection. +// --------------------------------------------------------------------------- +TEST_F(BridgeServerTest, CleanupSessionDropsPendingMiddlewareFrames) { + ASSERT_TRUE(server_->initialize()); + + // Create a session via heartbeat. + json hb; + hb["command"] = "heartbeat"; + mock_->push_request("client_DP", hb.dump()); + server_->process_requests(); + ASSERT_EQ(server_->get_active_session_count(), 1u); + EXPECT_TRUE(mock_->get_drop_pending_calls().empty()); + + // Disconnect triggers cleanup_session, which must invoke drop_pending for + // exactly this client. + mock_->simulate_disconnect("client_DP"); + EXPECT_EQ(server_->get_active_session_count(), 0u); + + auto calls = mock_->get_drop_pending_calls(); + ASSERT_EQ(calls.size(), 1u); + EXPECT_EQ(calls[0], "client_DP"); + + // cleanup_session for a client without a session still drops pending + // middleware state (idempotent, matches drop_pending's no-op contract). + mock_->simulate_disconnect("client_DP"); + calls = mock_->get_drop_pending_calls(); + ASSERT_EQ(calls.size(), 2u); + EXPECT_EQ(calls[1], "client_DP"); +} + // --------------------------------------------------------------------------- // 7. PerClientFiltering // diff --git a/tests/unit/test_websocket_middleware.cpp b/tests/unit/test_websocket_middleware.cpp index 7de70fd..573a73a 100644 --- a/tests/unit/test_websocket_middleware.cpp +++ b/tests/unit/test_websocket_middleware.cpp @@ -509,6 +509,49 @@ TEST_F(WebSocketMiddlewareTest, SendBinaryToAbsentClientCreatesNoPendingState) { EXPECT_EQ(middleware_->dropped_frame_count(), 0u); } +// drop_pending discards a client's backpressure backlog when its session is +// destroyed server-side (heartbeat timeout) while the socket stays open. A +// unit test cannot force the over-watermark path that populates the backlog, +// so this verifies the reachable contract: drop_pending is a safe idempotent +// no-op for clients with no pending state (never connected, or connected but +// never backlogged), never skews dropped_frame_count(), and leaves a live +// client's socket fully usable — send_binary still succeeds afterwards. +TEST_F(WebSocketMiddlewareTest, DropPendingIsSafeNoOpAndKeepsSocketUsable) { + auto result = middleware_->initialize(18099); + ASSERT_TRUE(result.has_value()); + + // No such client: must not crash or create state. + middleware_->drop_pending("never-connected"); + middleware_->drop_pending("never-connected"); + EXPECT_EQ(middleware_->dropped_frame_count(), 0u); + + // Connected client with an empty backlog: drop_pending must not touch the + // socket itself. + ix::WebSocket client; + client.setUrl("ws://127.0.0.1:18099"); + client.setOnMessageCallback([](const ix::WebSocketMessagePtr& /*msg*/) {}); + client.start(); + + ASSERT_TRUE(wait_for_client_open(client)) << "Client failed to connect"; + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + + client.send("register"); + std::vector data; + std::string client_id; + ASSERT_TRUE(poll_receive_request(*middleware_, data, client_id)); + ASSERT_FALSE(client_id.empty()); + + middleware_->drop_pending(client_id); + EXPECT_EQ(middleware_->dropped_frame_count(), 0u); + + std::vector payload = {1, 2, 3, 4}; + EXPECT_TRUE(middleware_->send_binary(client_id, payload)); + EXPECT_EQ(middleware_->dropped_frame_count(), 0u); + + client.stop(); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); +} + TEST(WebSocketMiddlewareBacklogConstructionTest, ExplicitBacklogSizeConstructsAndInitializes) { WebSocketMiddleware middleware(5); EXPECT_EQ(middleware.dropped_frame_count(), 0u);