ci: harden flaky suite checks (opcua A&C, action-status, diagnostic_bridge, operation_handlers, type_info, docs linkcheck)#504
Conversation
The secured Alarms and Conditions integration test flaked on loaded runners because the plugin started its poller before the ReportFault service client had matched fault_manager over DDS. AlarmCondition notifications are one-shot and the report is a fire-and-forget async request, so one emitted before that match is dropped with no retry and the alarm never surfaces. Wait (bounded) for the fault sink to be discovered before starting the poller so the first report cannot be lost.
There was a problem hiding this comment.
Pull request overview
This PR makes the secured OPC-UA Alarms & Conditions integration path deterministic under load by eliminating a startup ordering race between the OPC-UA poller’s one-shot alarm reporting and DDS discovery of the fault_manager ReportFault service.
Changes:
- Add a bounded (10s) best-effort wait for
/fault_manager/report_faultdiscovery before starting the OPC-UA poller. - Warn when the fault sink is not discovered within the timeout so operators understand alarms may be dropped until it appears.
The wait_for_service timeout was duplicated as a literal 10s in the warning text, so changing it would leave the log stale. Extract it into a named constant and build the message from it, keeping value and text in sync. No behaviour change.
…ault The aborted-goal integration test published the ABORTED status before the bridge had resolved this node's name over DDS, so under discovery lag the fault source froze to the action-name fallback and the reporting_sources assertion flaked. Wait for the bridge's subscription with a resolved node name before publishing, so the first report resolves the server FQN.
The ros.org REP pages intermittently exceed the linkcheck read timeout in CI (rep-0149 hit a 30s read timeout), failing the docs build on a transient external blip. Add the host to linkcheck_ignore and bump linkcheck_retries.
mfaferek93
changed the title
ListExecutionsReturnsTrackedActionGoal flaked under load when a graph-event refresh_cache() reconciled the manually seeded component away between the seed and the handler read, so list_executions returned ENTITY_NOT_FOUND. Pin discovery.refresh_debounce_ms at its 60s maximum so only the startup refresh runs for the test's lifetime, and gate seeding on that refresh (cache generation advance) plus service discovery rather than a fixed sleep.
A diagnostic published before the bridge subscription matched (volatile QoS drop), or before the bridge's ReportFault client discovered fault_manager (send_report drops a not-ready fire-and-forget request), was lost with no retry, so a single publish plus a fixed sleep raced both discovery hops. Wait for the bridge subscription to match this publisher, then re-emit the stimulus and poll until the fault lands.
An operation is listed by name as soon as it is discovered, before its interface type is resolved and type_info is populated, so reading the operation list once raced type resolution and intermittently found no schema. Add wait_for_operation_type_info and use it in the action and service type_info checks.
mfaferek93
changed the title
…second line The three docstrings added for the deterministic diagnostic_bridge test kept their summary on the opening line, which ament_pep257 flags as D213. Move each summary to the second line (blank first line) to match the style already used in the sibling bridge test docstrings.
The executor thread allocates the shared response in GenericClient::create_response() while the caller releases the last shared_ptr after future::get(); libstdc++ future happens-before is invisible to TSan, so it flags the control-block refcount decrement and delete. Surfaced by test_operation_handlers once the service call runs deterministically. Matches the rclcpp allocation frame only.
unsubscribe() and the subscribe() replace path destroyed the GenericSubscription while the MultiThreadedExecutor could still be dispatching its callback, so an executor worker freed the closure's captured strings and unloaded the type support under an in-flight deserialize (TSan use-after-free). Retire subscriptions to a graveyard and destroy them only in shutdown(), after the executor has stopped.
test_operation_handlers intermittently races on the GenericClient response buffer the std::future contract hands us after get(); all racing writes live in rclcpp/libstdc++. The adjacent librclcpp create_response frame is often unsymbolized, so match our reliably-symbolized OperationManager::call_service frame instead.
The deferred-destruction change hung gateway shutdown on the lyrical build-and-test leg: retired subscriptions stayed registered with the executor, so spin() did not return and the peer gateway was SIGKILLed (-9), failing test_peer_aggregation's exit-code check. Restore the original teardown; the trigger-subscriber TSan race needs a shutdown-safe fix instead. Keeps the call_service TSan suppression.
bburda
left a comment
bburda
left a comment
There was a problem hiding this comment.
Additional findings outside the diff:
list_faultsinros2_medkit_action_status_bridge/test/test_integration.test.pystill doesreturn future.result().faultswith no None guard. This PR adds that guard to the identical method in the diagnostic_bridge test. On a 5s ListFaults timeoutfuture.result()is None, soNone.faultsraises AttributeError insidewait_for_fault's loop instead of retrying. It is not part of this diff, but the same fix applies.
| # the client, and no other reliably-symbolized frame exists to match. Scoped to | ||
| # the service-call path only, so it does not mask the trigger-subscriber | ||
| # use-after-free (a genuine our-code bug, fixed at root, not suppressed). | ||
| race:ros2_medkit_gateway::OperationManager::call_service* |
There was a problem hiding this comment.
This matches our own OperationManager::call_service frame, which the rule at the top of this file forbids ("Never suppress ros2_medkit code"). The comment says this is unavoidable because the librclcpp create_response frame is often unsymbolized. But called_from_lib:<soname> matches by library, not by symbol name, and this file already uses it for stripped libraries (called_from_lib:libcpp-httplib.so.0.14, called_from_lib:libyaml-cpp.so.0.8). A called_from_lib:librclcpp.so line would catch the unsymbolized create_response reports without hiding races in our own code. As written, a future real race that runs through call_service (call_component_service calls it) would be suppressed too. Can this line be replaced with a called_from_lib for librclcpp?
There was a problem hiding this comment.
Noted. This is entangled with the reverted trigger-subscriber fix (5bfaa72, reverted in 30dc673 after the lyrical shutdown hang), so the suppression comment's "fixed at root" justification is now stale. Agree a called_from_lib:librclcpp.so line would be cleaner than matching our own call_service frame; changing the suppression is tied to how we settle that lifecycle race, so treating it as a pending design decision rather than folding it into this determinism PR. Leaving the code as-is and keeping the thread open.
| // start() removes that race deterministically. Bounded and best-effort: a | ||
| // deployment without a fault_manager still starts (degraded) after the wait. | ||
| constexpr auto kServiceDiscoveryTimeout = std::chrono::seconds(10); | ||
| if (fault_clients_->report && !fault_clients_->report->wait_for_service(kServiceDiscoveryTimeout)) { |
There was a problem hiding this comment.
Two things to confirm. (1) This makes the drop window smaller but does not remove the race. The alarm send path (async_send_request further down, no readiness check and no retry) still drops the first inactive->active report if fault_manager shows up more than 10s after startup, or restarts later. So "removes that race deterministically" is true for the co-launched test, not for any launch order. (2) The wait runs for every gateway that loads this plugin (the report client is always created when a node is present), so it blocks the gateway constructor, and REST startup, for up to 10s even for a PLC bridge with no alarms or a fault_manager that is just slow. The sibling action_status_bridge solves the same one-shot drop with defer + retry_timer_ + reconcile_pending() gated on is_service_ready(), which recovers whenever the sink appears, however late. Was block-and-wait chosen over that defer-and-retry pattern on purpose?
There was a problem hiding this comment.
Noted, and agree on both points - block-and-wait only shrinks the drop window and blocks the constructor / REST startup for up to 10s, and the action_status_bridge defer + retry_timer_ + reconcile_pending() gated on is_service_ready() is the better pattern. It is coupled with the same reverted trigger-subscription teardown change, so switching the startup model is a pending design decision rather than a change for this determinism PR. Leaving the code as-is and keeping the thread open.
Skip only the one slow REP URL instead of the whole ros.org/reps host so other REP links stay checked, and revert the now-inert retries bump.
Return an empty list when the ListFaults call times out, mirroring the guard added to the diagnostic_bridge integration test.
This PR hardens the shared test suite's flaky checks so they pass deterministically.
Each runs in the common CI suite, so every one was intermittently failing CI on
unrelated open-core PRs this cycle. None is a widened timeout or a re-run; each
addresses the root-cause race.
Hardened checks (one-line root cause each)
emitted before the
ReportFaultclient matched fault_manager over DDS, so itwas dropped and the alarm never surfaced.
was published before the bridge had resolved the server node FQN, freezing the
fault source to the action-name fallback.
refresh_cache()reconciled the manually seeded component away between the seedand the handler read, so
list_executionsreturned ENTITY_NOT_FOUND.subscription matched (volatile QoS drop), or before the bridge's
ReportFaultclient discovered fault_manager (
send_reportdrops a not-ready fire-and-forgetrequest), was lost with no retry.
listed by name before its interface type resolves, so its
type_infoschema wasintermittently absent when the assertion ran.