Skip to content

ci: harden flaky suite checks (opcua A&C, action-status, diagnostic_bridge, operation_handlers, type_info, docs linkcheck)#504

Merged
mfaferek93 merged 16 commits into
mainfrom
fix/opcua-secured-test-determinism
Jul 7, 2026
Merged

ci: harden flaky suite checks (opcua A&C, action-status, diagnostic_bridge, operation_handlers, type_info, docs linkcheck)#504
mfaferek93 merged 16 commits into
mainfrom
fix/opcua-secured-test-determinism

Conversation

@mfaferek93

@mfaferek93 mfaferek93 commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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)

  1. test_opcua_secured (secured OPC-UA A&C) - the one-shot alarm report was
    emitted before the ReportFault client matched fault_manager over DDS, so it
    was dropped and the alarm never surfaced.
  2. action_status_bridge test_01_aborted_goal_raises_fault - the ABORTED status
    was published before the bridge had resolved the server node FQN, freezing the
    fault source to the action-name fallback.
  3. docs linkcheck - a transient external read timeout to the canonical REP host.
  4. operation_handlers ListExecutionsReturnsTrackedActionGoal - 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.
  5. diagnostic_bridge test_01..test_05 - 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.
  6. action_lifecycle test_05 / operations_api service type_info - an operation is
    listed by name before its interface type resolves, so its type_info schema was
    intermittently absent when the assertion ran.

Copilot AI review requested due to automatic review settings July 4, 2026 20:12
@mfaferek93 mfaferek93 self-assigned this Jul 4, 2026
Copilot AI reviewed Jul 4, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_fault discovery 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.

Comment thread src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp Outdated
@mfaferek93 mfaferek93 changed the title test(opcua): make secured A&C test deterministic under load ci: harden three flaky checks (opcua secured A&C, action-status source, docs linkcheck) Jul 5, 2026
@mfaferek93 mfaferek93 changed the title ci: harden three flaky checks (opcua secured A&C, action-status source, docs linkcheck) ci: harden flaky suite checks (opcua A&C, action-status, diagnostic_bridge, operation_handlers, type_info, docs linkcheck) Jul 5, 2026
bburda
bburda reviewed Jul 6, 2026
View reviewed changes

@bburda bburda left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings outside the diff:

  • list_faults in ros2_medkit_action_status_bridge/test/test_integration.test.py still does return future.result().faults with no None guard. This PR adds that guard to the identical method in the diagnostic_bridge test. On a 5s ListFaults timeout future.result() is None, so None.faults raises AttributeError inside wait_for_fault's loop instead of retrying. It is not part of this diff, but the same fix applies.

Comment thread tsan_suppressions.txt
Comment thread docs/conf.py Outdated
Comment thread src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp Outdated
mfaferek93 added 14 commits July 7, 2026 17:10
@mfaferek93
test(opcua): make secured A&C test deterministic under load
90657a4 
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.
@mfaferek93
fix(opcua): derive service-discovery warning from timeout constant
91d5988 
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.
@mfaferek93
test(action_status_bridge): sync on bridge discovery before raising f…
8800e66 
…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.
@mfaferek93
docs: skip flaky ros.org/reps linkcheck host
5c9a0b6 
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
test(gateway): pin refresh debounce so seeded component survives
3453e30 
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.
@mfaferek93
test(diagnostic_bridge): re-emit and poll until fault surfaces
81a63cb 
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.
@mfaferek93
test(integration): wait for operation type_info before asserting
331d78b 
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
style(diagnostic_bridge): put multi-line test docstring summaries on …
01df44f 
…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.
@mfaferek93
test(tsan): suppress GenericClient response buffer refcount race
356704a 
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.
@mfaferek93
fix(gateway): defer trigger subscription teardown to shutdown
cfaaed8 
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.
@mfaferek93
test(tsan): suppress GenericClient response-staging race on call_service
723e041 
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.
@mfaferek93
revert: defer trigger subscription teardown to shutdown
e2b6124 
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.
@mfaferek93
docs: scope REP linkcheck ignore to rep-0149
a30845f 
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.
@mfaferek93
test(action_status_bridge): guard list_faults against null result
fbf2952 
Return an empty list when the ListFaults call times out, mirroring the
guard added to the diagnostic_bridge integration test.
@mfaferek93 mfaferek93 force-pushed the fix/opcua-secured-test-determinism branch from f7b0ff6 to 3d7e521 Compare July 7, 2026 15:15
bburda
bburda previously approved these changes Jul 7, 2026
View reviewed changes
mfaferek93 added 2 commits July 7, 2026 20:12
@mfaferek93
fix(opcua): buffer-and-retry fault reports instead of blocking startup
43af15a 
Drop the 10s wait_for_service in set_context. Buffer report/clear
dispatches (order-preserving, bounded to 256) and flush them on each
poll once the fault_manager service is matched, so a late sink still
receives the one-shot alarm without stalling startup.
@mfaferek93
docs(tsan): correct the call_service suppression rationale
51c2f9f 
The suppression must match our call_service frame, not a library scope:
called_from_lib:librclcpp.so does not catch the report (TSan reports it
with our stack as primary, librclcpp is not a caller) and fails
sanitizer-tsan. Drop the stale fixed-at-root note; that
trigger-subscriber fix was reverted.
@mfaferek93 mfaferek93 force-pushed the fix/opcua-secured-test-determinism branch from 3d7e521 to 51c2f9f Compare July 7, 2026 18:13
@mfaferek93 mfaferek93 merged commit fbfe3a0 into main Jul 7, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@bburda bburda bburda approved these changes

Assignees

@mfaferek93 mfaferek93

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mfaferek93 @bburda