fix: five test failures on main, three timing races and two RoboStack-only - #636
Merged
Conversation
The sampling window read a generation counter, started a late node, then took its first sample with no delay in front of it. Nothing ordered those two, so on a slow runner the first recorded sample could already be on the far side of the change and no sample sat before it. The window then failed its own "before and after" assertion at sample 0. Sample 0 is now taken before the node is started and its generation is the baseline, so a later advance always has a recorded sample in front of it. Sample 1 follows the change with no sleep, which keeps the observation of the moment the entity set changed that the old first sample provided. The deadline for the advance runs from the change rather than from the first baseline read, so the HTTP round trips of the baseline are not charged to it.
A peer-owned member is present in this gateway's tree before what it offers is. The App arrives with the peer's entity list, and its operations arrive by a second read the aggregator makes per App. In between, the member is listed and offers nothing, which looks the same as a member that has none. The suite started as soon as the Apps had merged, so a test that addresses a peer-owned operation could run inside that gap and see only the local half. The read now retries until the operation is there, and it returns the last response either way so the test still fails on its own assertion and message rather than on a bare timeout. The three waits in this file also take the shared discovery budget instead of a hard-coded 60 seconds, so they stretch with MEDKIT_TEST_TIME_SCALE the way the sanitizer jobs expect.
An executor rebuilds its entity collection when a node joins or leaves it, and a thread already inside spin() or spin_some() reads that collection through the executor's notify waitable. There is no lock a caller can take across the two. ThreadSanitizer reports the overlap as a write in operator delete against ExecutorNotifyWaitable::is_ready(), and it names whichever test happened to be running, so it reads as a defect in that test rather than as the shared pattern it is. Fifteen test bodies added a node to an executor a fixture thread was already spinning, and removed it later in the same body. They now give the node its own SpinningExecutor, which takes its nodes in the constructor, starts the thread after them, and cancels and joins before any node leaves. On Jazzy, cancel() clears the same flag that spin() sets on entry, so a cancel that lands before the worker reaches spin() is overwritten and lost, and join() would then wait for a spin nobody asked to stop. The helper repeats the cancel until the worker is provably out. It also catches what spin() throws instead of letting it leave the thread's entry function, which would be std::terminate with no test named.
launch_testing's collection hook imports every *.test.py file pytest considers. The module name keeps the dot, so test_rosbag_history.test.py becomes test_rosbag_history.test and the import asks for a package named test_rosbag_history that does not exist. The error is raised against the directory, so it takes down the collection of every plain pytest file in it. In this package that is test_rosbag_storage_dependency.py, which then fails before any of its assertions run. Seven launch tests were moved to test/integration earlier and this one was left behind, so the same failure came back naming the file that stayed. test/ now holds no launch test at all. Because the shape has caused this twice, a check in ros2_medkit_cmake now sweeps the tree and fails when any directory holds both a launch test and a plain pytest file. It reads the layout rather than any CMakeLists, and a second assertion fails if the sweep resolved to a directory that contains none of this repository's tests, so a sweep that covers nothing cannot pass as a clean result.
httplib's Server::stop() asserts the listening socket is still valid whenever the server reports itself running, but it is the listen thread that clears the running flag, and only once it comes back out of accept(). Closing the socket does not promise to wake it. Three tests stopped the peer mid-scope and the destructor stopped it again, so a second stop could see a running server whose socket had already been taken away. The assertion then aborts the whole binary. A build with NDEBUG drops the assertion and hides it, which is why this only shows up under the RoboStack toolchain. The stop now goes through the fixture, which issues it once. Nothing waits on accept() coming back: the join stays in the destructor, where it already was, so this does not trade the abort for a hang.
The rosbag branch built its response by aggregate initialisation with only the items given, so the two optional members were left unnamed and the build warned about each of them. The other branch in the same handler already fills the response field by field.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes five CI test failures on main by addressing three timing/concurrency races in tests and two RoboStack-specific failures related to launch_testing collection and httplib::Server::stop() assertions. It introduces a small test-only executor helper to eliminate a known rclcpp executor node-mutation race, and tightens test layout constraints to prevent reintroducing the RoboStack collection issue.
Changes:
- Introduce
test_support::SpinningExecutorand migrate multiple C++ tests to avoid mutating an executor while another thread is spinning it. - Harden integration tests against timing races by sampling baselines correctly and by retrying until peer-owned operations have arrived.
- Fix RoboStack-only failures by relocating a
*.test.pylaunch test intotest/integration, making mock peer stop idempotent, and adding a layout-sweep pytest check to prevent mixed launch/plain pytest directories.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_param_drift_integration.cpp | Replaces add/remove-node-on-spinning-executor pattern with SpinningExecutor instances to eliminate a TSan race. |
| src/ros2_medkit_integration_tests/test/features/test_merge_provenance.test.py | Removes baseline sampling race by ensuring sample 0 is recorded before starting the late node; adjusts deadlines accordingly. |
| src/ros2_medkit_integration_tests/test/features/test_aggregator_only_configurations.test.py | Uses shared discovery time budgets and retries collection reads until a specific peer-owned operation appears. |
| src/ros2_medkit_gateway/test/test_ros2_subscription_executor.cpp | Uses SpinningExecutor for helper nodes to avoid executor mutation while spinning. |
| src/ros2_medkit_gateway/test/test_ros2_parameter_transport.cpp | Uses SpinningExecutor for responsive nodes to avoid executor mutation while spinning. |
| src/ros2_medkit_gateway/test/test_aggregation_manager.cpp | Makes mock peer server stop idempotent and routes all stop calls through a single method to avoid double-stop aborts in RoboStack. |
| src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp | Adjusts rosbag descriptor response construction to avoid warnings (field-by-field assignment). |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/test_support/spinning_executor.hpp | Adds a test-only RAII executor wrapper that fixes the node-mutation-while-spinning race and propagates spin-thread exceptions via stop(). |
| src/ros2_medkit_fault_manager/test/integration/test_rosbag_history.test.py | Moves/lands the rosbag history launch test under test/integration to avoid launch_testing/pytest collection failure. |
| src/ros2_medkit_fault_manager/CMakeLists.txt | Updates the launch test registration path to match the new integration test location. |
| src/ros2_medkit_cmake/test/test_pytest_collection_layout.py | Adds a repository layout guard to prevent mixing *.test.py launch tests with plain pytest tests in the same directory. |
| src/ros2_medkit_cmake/CMakeLists.txt | Registers the new layout-sweep pytest test as part of ros2_medkit_cmake’s test suite. |
| CONTRIBUTING.md | Documents the “fixed node set while spinning” rule and the intended SpinningExecutor usage pattern for tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…el retry Every call site leaves teardown to the destructor, so carrying an exception out of the worker and then writing it to the console was still a silent failure: a test whose callback threw finished green. It is recorded against the running test now. The cancel retry is bounded. cancel() cannot pre-empt a callback that is already running, so against one that never returns the loop would spin forever, burn a core and report nothing. Past the bound the join takes over, which is where such a test hung before this type existed. The readiness poll for a peer-owned operation treats a failed read as an answer that has not arrived rather than as a verdict. A gateway busy merging can refuse or drop a request, and dying on the first blip reported a transport hiccup as a missing operation. The last read is still asserted, so a gateway that is still failing when the budget runs out is reported as the failure it is.
The check that the sweep had covered anything asked whether this package's own test directory was among the results. It always is: the guard file lives there and is itself a plain pytest file, so the answer was yes however badly the root resolved, and the check proved only that rglob can find the file running it. It now asks how many OTHER packages the sweep reached, which a workspace sweep can show and a collapsed one cannot. Without a workspace the fallback stays inside this package instead of walking up to its parent. On a packaging worker that parent holds other projects and their build output, and the guard would then fail on a layout that is none of ours. The docstring said no build configuration can change the answer. That is not true - ament_add_pytest_test takes a directory, and this repository uses that form - so it now states what the check covers and what it does not.
test_stream_proxy's wait for the mock server returned silently when its budget ran out, so a case went on against a server that was not listening and whatever it then asserted was about the wait. Running out is a failure and is reported as one. test_grouping_entity_aggregation gates on the peer's Apps and its topics, then reads operation collections. Those arrive by a separate per-App read, so the early cases could see the local half alone and report a merge defect that is not there. It now absorbs that poll the same way it already absorbs the topic one, best effort and for the stated reason: raising in setUpClass would turn one late collection into an error for every case below.
mfaferek93
reviewed
Aug 24, 2026
setUpClass spends up to three discovery budgets before the first case runs, and the feature glob gives the file 120 seconds. A broken merge therefore ran out of ctest time rather than failing an assertion, and a timeout names no test, so the one answer it gives says nothing about which rule broke. That was already true before the readiness retry was added; the retry made it worse by stacking a fourth budget on top. The retry now waits on what it is actually waiting for. When it runs, the class gates have already established that the peer's Apps are online and merged here, so what remains is one aggregation refresh copying that App's operations across, and the gateways run that refresh every second under test. The file also joins the timeout overrides at 300, beside the other suites whose setUpClass converges several things before they start. Without it the shortened retry would still leave the three class budgets over the glob default.
mfaferek93
approved these changes
Aug 25, 2026
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
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
Fixes the five failures reported in the issue below. They have separate causes, so each is a separate commit.
Three timing races
test_merge_provenancetook its first sample after starting the late node with no delay in front of it, so the first recorded sample could already be past the change. Sample 0 is now taken before the node starts and its generation is the baseline. Sample 1 still follows the change with no sleep, so the observation at the moment of the change is kept. The deadline for the advance now runs from the change, not from the first baseline read.test_aggregator_only_configurationsstarted as soon as the peer's Apps had merged, but the operations of a peer-owned member arrive by a second read. The read now retries until the operation is there and returns the last response either way, so the test still fails on its own assertion. The three waits in that file also take the shared discovery budget, so they scale withMEDKIT_TEST_TIME_SCALE.SpinningExecutorthat takes its nodes up front, starts its thread after them, and cancels and joins before any node leaves. On Jazzycancel()clears the same flagspin()sets on entry, so the helper repeats the cancel until the worker is provably out, and it catches whatspin()throws instead of letting it terminate the binary.Two RoboStack-only failures
test_rosbag_history.test.pysat next to a plain pytest file, andlaunch_testing's collection hook then failed the whole directory. It moves totest/integrationwith the seven launch tests already there.Two smaller things: a check in
ros2_medkit_cmakenow fails when any directory holds both a launch test and a plain pytest file, because that shape has caused the collection failure twice. And the rosbag descriptor list is built field by field, which clears two build warnings.Issue
Type
Testing
Local, on Jazzy: 5292 tests across
ros2_medkit_gateway,ros2_medkit_fault_manager,ros2_medkit_graph_watchdog,ros2_medkit_integration_testsandros2_medkit_cmake, 0 errors, 0 failures. Gateway builds with no warnings. The RoboStack smoke script passes locally.Two of the fixes have a falsifying check, run both ways:
test_rosbag_history.test.pyback next to the plain pytest file reproduces the CI error word for word, includingfound no collectors. Moving it gives 6 passed. Checked with the interpreter from a RoboStack environment.Two things I could not verify locally, so please read the CI result for them rather than my word:
unexpected memory mappingbefore any test runs. The sanitizer job is the first real run of that fix.server_->stop()now has one caller, not on a local reproduction. I did not reproduce that abort here even with 200 repeats under single core pinning, which fits: the assertion needs the listen thread to still be insideaccept().Worth knowing when reading the RoboStack result: that job is
continue-on-error, so a red job still shows a green run. It has been red since 2026-08-13.Checklist