integration_tests: bound two timing cases on what they can actually prove - #667
Merged
Conversation
bburda
force-pushed
the
fix/peer-recovery-read-poll
branch
from
September 9, 2026 10:47
ed19911 to
5a70540
Compare
…read test_07 polled until the aggregate read of the peer-owned topic answered 200 and checked the payload after the poll had already stopped. A 200 is not the answer here. When the peer comes back the member re-subscribes, and the read waits for a first sample only as long as topic_sample_timeout_sec allows, 1 s by default, so it answers metadata_only with an empty payload when the sample lands later. The case compared that body against 'data' and failed, which is what happens under a sanitizer build. The poll now waits for the payload, the way case 1 already waits for it on the healthy peer. A read that never carries the sample within the recovery budget still fails the case, with a message that says so. Waiting for the payload would on its own accept a gateway that stopped waiting for a first sample at all, because a later poll would find the sample anyway. One more read after the poll closes that hole: once the member's subscription is warm the gateway holds the latest sample, so a read has to carry it on the spot.
…the refresh The graph-event case measured from process spawn and required detection inside 2 s. Two things were wrong with that bound. The gateway coalesces graph events behind discovery.refresh_debounce_ms, 1000 ms by default, so a spawn arriving mid-window waits for the next one and detection lands on a multiple of the debounce. Measured with the default settings the spread runs from about 1 s to 3.6 s, and the 2 s bound sat in the gap between those two clusters, so the case turned on which side of the debounce the spawn happened to land. The backstop timer is also created during gateway initialisation, so its phase relative to a mid-run spawn is arbitrary. A sweep landing inside the bound serves the detection even when the graph-event path is dead, and an elapsed-time bound cannot tell the two apart. The backstop now runs at the longest interval the gateway accepts, 60 s, and the case checks that it finished measuring within 30 s of the gateway answering /health. Inside that window no sweep has run, so the refresh it observed came from a graph event. The latency bound is 10 s, above the measured spread, and the poll gives up at 15 s so a late detection reports the time it took. With the graph-event path disabled by a 60 s debounce, this case fails at its poll and so does the startup-discovery case, which the 30 s backstop used to serve.
bburda
force-pushed
the
fix/peer-recovery-read-poll
branch
from
September 9, 2026 12:31
5a70540 to
32bda35
Compare
mfaferek93
reviewed
Sep 10, 2026
…-backstop window PRE_BACKSTOP_BUDGET_SEC goes from 30 s to 50 s. The gateway bounds the window it names: the backstop timer is armed during gateway initialisation, ahead of start_rest_server(), whose wait for the REST server to accept connections is capped at 5 s, and the base class then polls /health every 0.5 s. Of the 60 s backstop interval that leaves roughly 54 s still ahead of the first sweep by the time /health answers, so 30 was giving away about half of the usable window. 50 keeps a margin for scheduling slack. That budget stays unscaled on purpose. MEDKIT_TEST_TIME_SCALE stretches what the test is willing to wait for; it does not stretch the gateway's backstop interval, so scaling this one would let the measurement drift past the first sweep and the assertion would no longer be about the graph-event path. SPAWN_DETECTION_TIMEOUT, GRAPH_EVENT_MAX_LATENCY_SEC and INITIAL_DETECTION_TIMEOUT do scale, the way the sibling feature tests already do. Their base values are unchanged at 15 s, 10 s and 30 s; each bounds instrumented work that an ASan or TSan build slows down, and the sanitizer jobs export the same factor they apply to the ctest timeouts.
…ption launch_testing builds the description in the test process and only then starts the launch service that forks the gateway, and the gateway arms its backstop timer during its own initialisation. A time taken in generate_test_description() therefore precedes the timer, and the first sweep cannot come earlier than the backstop interval past it. The spawn case now measures its pre-backstop window from that point. The first /health answer, the old reference, is never bounded relative to the timer by the code.
…ails The failure message of the recovery poll performed one more aggregate read while being built, on success as well as on failure, so the warm read that follows was the second read after the poll. Case 4 already reads the last answer only on its failure path; case 7 now does the same.
The debounce services the first graph event after a quiet period at the next tick and only defers the events that follow inside the window, so the latency comment no longer claims a sub-second detection is impossible. The warm-read comment in the peer recovery case now claims only what the assertion checks: a read that follows a successful one carries the sample.
…op window too The startup nodes come up after the gateway's own initial discovery, so a refresh has to pick them up, and with the poll budget scaled for the sanitizer jobs that poll outlives the 60 s backstop. The window check the spawn case already makes now covers the startup case as well, so a detection the backstop served fails with the same message in both.
mfaferek93
approved these changes
Sep 10, 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
test_peer_recoverycase 7 reads the peer-owned topic through the merged Function after the peer gateway comes back, and it polls until that read answers 200. A 200 is not the answer here. When the peer returns, the member re-subscribes, and until its first sample arrives the read is a well-formed body with"status": "metadata_only"and no payload. The case checked the payload after the poll had already stopped, so the first 200 had to carry data or the case failed.That is a race with the publisher, and it lost on a sanitizer build:
AssertionError: 'metadata_only' != 'data', withsubscriber_count: 0in the reported body.Case 1 in the same file already waits for the payload, and says why in its own comment. Case 7 now does the same. A read that never carries the sample within the recovery budget still fails the case, with a message that says the sample never came.
Waiting for the payload would on its own accept a gateway whose reads go back to empty between samples, because the poll stops at the first read that carries data. One more read after the poll pins that a read which follows a successful one carries the sample too: once the member's subscription is warm the gateway holds the latest sample and answers from it. This read does not measure how long a cold read waits for its first sample; the test controls no publisher, so it cannot pin that. The failure message of the poll used to make one more read while it was built, on success as well, so the read after the poll was the second one. It now reads the last answer only on the failure path, the way case 4 does.
Issue
Type
Testing
The demo publisher on the peer-owned topic runs at 500 ms, which is fast enough that the first read after recovery usually already carries a sample. To make the window real, the publisher period was raised locally and the case was run in four configurations:
'metadata_only' != 'data', same body as CIThe last row is the check that the new poll can still fail: with the expectation mutated, case 7 spends its whole budget and reports that the sample never came, while the other eight cases stay green.
Full package run with all three changes:
colcon test --packages-select ros2_medkit_integration_tests, 1244 tests, 0 errors, 0 failures.Second case: the graph-event spawn bound
The same package had a second case measuring a quantity it did not control.
test_graph_event_discoverytimed spawn detection fromsubprocess.Popenand required it under 2.0 s. The gateway coalesces graph events behinddiscovery.refresh_debounce_ms, 1000 ms by default and not set by that test. The first event after a quiet period is serviced at the next 100 ms tick, and every event inside the window that follows waits for the window to end. A node coming up raises several events in a row, so its detection typically lands one window after its first event.Measured by setting the bound to 0 so every run prints its time, eight runs with the default settings:
Two clusters, and the 2.0 s bound sits in the empty gap between them, so the case passed on which side of the debounce the spawn happened to land. Locally that was 2 passes in 8. With the debounce lowered to 50 ms, six runs gave
1.247 1.244 1.244 1.248 0.210 3.005, which is what identifies the debounce as the cause.For reference, spawn to node-visible-in-the-ROS-graph, measured by an independent rclpy node with no gateway involved and a fresh domain per iteration, is 0.43 s to 0.59 s over eight samples. So most of the old 2.0 s budget was never the gateway's to spend.
A bound on elapsed time cannot separate the two mechanisms on its own, whatever value it takes. The backstop timer is created during gateway initialisation, so its phase relative to a mid-run spawn is arbitrary: a sweep landing inside the bound serves the detection even when the graph-event path is dead. At a 30 s backstop that is a 10/30 chance for a 10 s bound and 2/30 for the old 2 s one, so raising the bound alone would have made the case weaker.
So the case now establishes what it measured before bounding it. The backstop runs at 60 s, the longest interval the gateway accepts. Both cases check that the detection they observed landed within 50 s of a time taken in
generate_test_description(). launch_testing calls that function in the test process and only then starts the launch service that forks the gateway, and the gateway arms the backstop timer during its own initialisation. The timer is therefore armed after that point, and a wall timer fires late, never early, so no sweep can have run inside the window. The gateway's own startup only widens it. The spawn bound is 10 s, above the measured spread, and the poll gives up at 15 s so a late detection reports the time it took. The startup case needs the same check because its poll budget outlives the backstop in the sanitizer jobs.The three poll and latency budgets multiply by
get_time_scale(), as the other tests in this directory do, so the ASan and TSan jobs get 30 s, 45 s and 90 s. The 50 s window does not scale: it is bounded by the gateway's backstop interval, which the scale factor does not stretch.Verified: three runs at scale 1 and three at scale 3 pass with the default settings, spawn detection landing 4.3 s to 4.7 s after the launch was described. Falsification, with the graph-event path disabled by a 60 s debounce:
The scale 3 row is the reason the startup case checks the window: with a 90 s poll and no check, the backstop would have served it and the case would have passed with the graph-event path dead.
Not identified: with the debounce at 50 ms four runs clustered at 1.244-1.248 s, too tight to be chance, so there is a second quantisation in there; and one run still took 3.005 s. Neither is explained, and neither changes what the bound can prove.
Checklist