Skip to content

[#25513] Solve tools Flaky tests#192

Open
Danipiza wants to merge 4 commits into
mainfrom
fix/flaky_tests
Open

[#25513] Solve tools Flaky tests#192
Danipiza wants to merge 4 commits into
mainfrom
fix/flaky_tests

Conversation

@Danipiza

@Danipiza Danipiza commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Description

Fix Tools Flaky tests.

  • DDS-Record-Replay
  • DDS-Router
  • DDS-Enabler

DDS-Record-Replay

While stabilizing the DDS-Record-Replay CI (TSAN) several failures were traced to two distinct problems in this repository.

1. DdsRecorderMonitor crash when monitor_status() runs more than once

DdsRecorderMonitor::monitor_status() created the DdsRecorderStatusMonitorProducer in a function-local static unique_ptr and then std::moved it into the StatusMonitorProducer singleton. Because the variable is static, it is initialized only once and is left in a moved-from (null) state after the first call. Any second call — which every monitoring unit test does in its SetUp() — dereferenced the null pointer at ddsrecorder_status_producer->init(...), crashing the process:

  • 0xC0000409 (fatal exit) on Windows,
  • SIGSEGV on Linux.

The whole test binary crashed on the second test, so CI blamed whichever test happened to be running (e.g. LogMonitorDdsRecorderStatusTest.mcap_file_creation_failure).

Fix: install the producer through StatusMonitorProducer::init_instance() (a no-op once the
singleton exists) and always retrieve/operate on get_instance(), re-registering the consumers on
it. This mirrors the ddspipe_core::Monitor::monitor_status() pattern and makes the method safe to
call multiple times per process.

2. Flaky ResourceLimitsTest SQL size assertions

ResourceLimitsTest.sql_log_rotation and sql_max_file_size intermittently failed with "unacceptable size" (e.g. 112–192 KB vs a 240 KB minimum). Root cause: wait_for_acknowledgments() only guarantees that samples reached the recorder's (reliable) reader, not that they have been written to disk. Writing is asynchronous, so on slow / loaded CI runners the write pipeline lags, and stopping the recorder while samples are still pending drops them, leaving the output file
smaller than expected.

Fix: before stopping the recorder, wait for the recording to catch up. The samples are retained
by the reliable reader (no data loss), so the test polls the on-disk recording (the open
<file>.tmp~, plus the SQLite -wal sidecar) until it reaches the steady state (log-rotation caps
it at max-size) or stops growing (drained), with a generous cap kept below the ctest timeout. This
removes the timing assumption without weakening the assertion — a genuine under-recording still fails.

DDS-Router

RPC service replies were intermittently lost when routed through the DDS Router, causing ROS 2 service clients to hang forever on a request the server had actually answered. This surfaced as flaky failures in the DDS-Router dockertests:

  • tool.application.ddsrouter.compose.rpc_ros2_services_correct_target
  • tool.application.ddsrouter.compose.rpc_ros2_services_trivial_multiple
  • tool.application.ddsrouter.compose.rpc_ros2_services_repeater_with_talker

Root cause

Reply topics use RELIABLE + VOLATILE QoS. A client's wait_for_service only proves the reply reader is matched from the reader side, so the client starts sending requests while the router's client-facing reply writer has not yet matched that client's reply reader on the writer side. The router forwards the request, the server answers, and the router writes the reply into that discovery window — being VOLATILE, the sample is delivered to no one for that client and then purged. Because ROS 2 service clients never retry a request, the client blocks forever.

Signed-off-by: danipiza <dpizarrogallego@gmail.com>
@codecov-commenter

codecov-commenter commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 21.62162% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.01%. Comparing base (4cfd8cc) to head (4e3d55f).

Files with missing lines Patch % Lines
..._participants/src/cpp/writer/rtps/CommonWriter.cpp 37.50% 10 Missing ⚠️
...spipe_core/src/cpp/communication/rpc/RpcBridge.cpp 0.00% 6 Missing ⚠️
...pp/participant/dynamic_types/SchemaParticipant.cpp 0.00% 5 Missing ⚠️
...ants/src/cpp/participant/dds/CommonParticipant.cpp 20.00% 2 Missing and 2 partials ⚠️
...nts/src/cpp/participant/rtps/CommonParticipant.cpp 20.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #192      +/-   ##
==========================================
+ Coverage   37.98%   39.01%   +1.02%     
==========================================
  Files         177      160      -17     
  Lines       12751     7849    -4902     
  Branches     5844     3148    -2696     
==========================================
- Hits         4844     3062    -1782     
+ Misses       4793     3068    -1725     
+ Partials     3114     1719    -1395     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: danipiza <dpizarrogallego@gmail.com>
…atch

Signed-off-by: danipiza <dpizarrogallego@gmail.com>
Signed-off-by: danipiza <dpizarrogallego@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants