Conversation
mfaferek93
reviewed
Sep 14, 2026
bburda
force-pushed
the
fix/cancel-outcomes-under-tsan
branch
from
September 18, 2026 18:04
fef130d to
9e70a7f
Compare
…apper A GenericClient response lives in the shared state of a std::future. The thread that drops the last reference to that state frees it. When this is a gateway executor thread, ~_State_base and _Result::_M_destroy() run inside librclcpp. librclcpp has no TSan instrumentation. TSan sees the operator delete, because the allocator intercepts it. TSan does not see the acquire on the shared_ptr refcount that orders the free after the caller's future::get(). So it pairs the free with the caller's read of _Result_base::_M_error. Two patterns anchor on the read side of this report, FutureAndRequestId::get(). One is for rclcpp on Jazzy and newer. The other is for compat::GenericServiceClient, which the gateway uses on Humble. The read is header code compiled into our binaries, so its frame always has a symbol. The freeing frame inside librclcpp has no symbol, so a function pattern cannot match it. A module pattern such as race:librclcpp.so can, and it would also suppress every report with a librclcpp frame on either stack. The patterns match only a future that rclcpp returns for a client request. The destructor and wait_for of the wrapper stay unmatched. The comments on the neighbouring suppressions now describe how libstdc++ synchronises a future. The setter runs under call_once and release-stores _M_status. wait() does an acquire load on the same atomic. For GenericClient the setter chain is compiled into librclcpp, so TSan does not see the release store.
… reason An aggregator answers a fanned-out listing with 200 and no `partial` until a discovery pass has done two things. The pass marks the peer healthy, and it publishes the peer as a contributor to the entity. Peer health does not imply the second. The pass sets the two at opposite ends, and the peer metadata fetch with its whole budget runs between them. _fan_out_failure now polls the listing on the discovery budget until an answer carries `partial`. Every answer must still be 200. The assertions on the reason and on the failed_peers shape are unchanged, so a wrong reason or a reshaped failed_peers still fails the test.
DISCOVERY_TIMEOUT already carries the sanitizer time scale. Eight feature tests multiplied it by the scale again, which squares the scale. With a scale of 3, one discovery wait in test_peer_failure_reasons lasts 540 s. Its setUpClass runs three of those waits in a row. That is more than the test's 300 s ctest budget, even after the sanitizer jobs stretch it. test_aggregation_time_budgets also scaled the already scaled GENEROUS_FORWARD_MS a second time in two request timeouts. Every budget is now scaled once. In test_peer_failure_reasons, _fan_out_failure polls on TIMEOUT. It retries a request that raised. It keeps the 200 assertion on every answer. When no answer carries the fan-out, the failure names the last transport error.
The test starts three gateways and a fault_manager on three extra domains, waits for both aggregators, then waits a scaled 60 s for the fault_manager on the peer's domain, before the first case runs. That does not fit the 120 s feature default.
bburda
force-pushed
the
fix/cancel-outcomes-under-tsan
branch
from
September 18, 2026 18:47
9e70a7f to
be714ca
Compare
The gateway could abort with rclcpp::graph_listener::NodeNotFoundError after SIGINT. rclcpp registers a node with the context's graph listener on the node's first get_graph_event(), and wait_for_service() makes that call whenever the service is not ready. When the first call comes after rclcpp::shutdown(), the listener has already stopped: get_graph_event() consumes the node's registration flag, add_node() throws GraphListenerShutdownError, and the node stays out of the listener's list. ~NodeGraph later calls remove_node(), which throws NodeNotFoundError from a noexcept destructor, and the process terminates with SIGABRT. The fault service transport's private node reached that state through the entity freeze-frame catch-up. With a plugin loaded and no fault manager, the catch-up waits 10 s for the fault-events publisher and then waits for the fault services. When SIGINT arrives shortly before that deadline, or an in-flight request holds the teardown open past it, the first wait on the node comes after shutdown. The lifecycle state reader can reach the same state when a client created before shutdown makes the node's first wait after it. Both private nodes now call get_graph_event() once when they are created, so each joins the listener while the context is valid. A registered node tears down cleanly after shutdown. The catch-up also returns when rclcpp has shut down: from that point every wait returns at once, and the loop kept one core busy and grew the node's list of graph events until the teardown stopped it. A death test per node covers the teardown after a first wait that follows shutdown. An integration test stops the gateway before the catch-up deadline and holds the teardown open past it with an operation call that gets no answer.
bburda
force-pushed
the
fix/cancel-outcomes-under-tsan
branch
from
September 19, 2026 11:35
d7b6050 to
b65a35f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
The
sanitizer-tsanjob onmainis red on two tests that share nothing but the job.test_cancel_outcomespasses all 20 cases; the process exits non-zero because TSan reports one race. The last reference to aGenericClientfuture's shared state is dropped by the rclcpp executor thread, insidelibrclcpp, which is not instrumented. TSan sees theoperator deletethere but not the acquire on theshared_ptrrefcount that orders it after ourfuture::get(), so it pairs the free with the caller's earlier read of the result. Our use of the API is correct and the reference is held for the wholeget().tsan_suppressions.txtgets two lines for this class of report, both anchored on the read side, the wrapper'sget():rclcpp::detail::FutureAndRequestIdon Jazzy and newer, and the gateway'scompat::GenericServiceClienton Humble. The wrapper's destructor andwait_forare not matched, and nostd::futurethe gateway builds itself is matched. The comment blocks around the pattern describe the synchronisation the way libstdc++ implements it. Reproduced under TSan with a standalone reproducer against the real suppressions file: 5 of 5 runs reported the race with no suppression. Thesanitizer-tsanjob of this pull request checks the pattern against the real gateway.test_peer_failure_reasonsfailed on an assertion. Between the aggregator marking a peer online and publishing its contributor map for an entity, a read answers 200 with no fan-out at all, which is the documented purpose of the contributor map. The test's readiness gate waited for the first signal and read the failure reason from a single response, so it asserted a transient state; the window is about 0.9 s without instrumentation and wider under TSan._fan_out_failurenow polls until the answer carries the fan-out before reading the reason, retries a request that raised, and keeps every original assertion. With the readiness gate reduced to "the HTTP server answers", the old helper fails 5 of 5 runs with the exact CI message and the new one passes 5 of 5.Seven feature tests applied
get_time_scale()on top ofDISCOVERY_TIMEOUT, which already carries the scale, andtest_aggregation_time_budgetsscaledGENEROUS_FORWARD_MStwice in two request timeouts. Every budget now scales once, so the longest single wait under TSan is 180 s.test_relay_peer_credentialgets a 300 s ctest budget like the other multi-gateway tests; it starts three gateways and a fault manager and holds two sequential waits before its first case.Issue
No issue. Two independent failures of one CI job.
Type
Testing
test_cancel_outcomesandtest_peer_failure_reasonson jazzy, plain build: 10 of 10 and 10 of 10, plus the aggregation sweep (ctest -R "peer|aggregat", 11 of 11).test_1still fails the fixed test ('timeout' != 'unreachable').test_relay_peer_credentialpass on jazzy, plain build, with the new budgets in place; the integration package's linters pass.sanitizer-tsanjob of this pull request is the only place the suppression is verified against the real gateway under TSan.Checklist