CAMEL-24063: Fix flaky DirectProducerBlockingTest.testProducerBlocksResumeTest#24698
CAMEL-24063: Fix flaky DirectProducerBlockingTest.testProducerBlocksResumeTest#24698gnodet wants to merge 1 commit into
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 23 tested, 0 compile-only — current: 0 all testedMaveniverse Scalpel detected 23 affected modules (current approach: 0).
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
CI passes on JDK 17 + 25. All six fixes are test-only, backed by Develocity flake data, and follow established project patterns.
Observations (non-blocking):
- SchedulerNoPolledMessagesTest — second widening of timing windows in 3 days (CAMEL-24030 went 500→2000ms; this goes 2000→4000ms). Justified by continued 9.6% flake rate, but worth watching — if it keeps flaking, the test structure may need rethinking rather than wider windows.
- SedaBlockWhenFullTest —
@Timeout(20)→@Timeout(60)is necessary since CAMEL-24040 addedassertIsSatisfied(context, 30, SECONDS)inside the test, and the JUnit timeout would kill the test before the mock assertion completes. Good catch. - JmsAddAndRemoveRouteManagementIT — adding
routeId+waitForJmsConsumerRoutesfollows the established JMS test pattern and addresses a different root cause than the prior fix (CAMEL-24031 fixed JMX timeouts; this fixes initial consumer readiness).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @davsclaus
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Good batch of flaky test fixes — all six changes are well-targeted and consistent with project conventions (proper MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS), no Thread.sleep, route readiness checks).
The JMS durable topic fix correctly adds routeId and waitForJmsConsumerRoutes to address the root cause. The DirectProducerBlockingTest timing relaxation and SedaBlockWhenFullTest timeout increase are both reasonable CI accommodations.
Minor observation (low)
In SchedulerNoPolledMessagesTest, the lower bound for message(2).arrives() dropped from 200ms to 100ms. Since the base delay=100ms and backoffMultiplier=10, the expected inter-arrival gap after backoff is ~1000ms. At 100ms lower bound, the assertion would pass even if backoff had zero effect (since the base poll interval is 100ms). A value of 150-200ms would still prevent flakiness while retaining some confidence that backoff actually kicked in. Very minor — the test still validates the overall timing pattern.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Thanks — raising the lower bound from 100ms to 200ms is exactly right. At 200ms, the assertion retains meaningful confidence that the backoff multiplier (10× the 100ms base delay) actually kicked in, while still avoiding flakiness on slow CI runners.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code review on behalf of @gnodet
…4651 feedback addressed, apache#24695 merged
| final Object request = assertDoesNotThrow(() -> future.get(5, TimeUnit.SECONDS)); | ||
| assertNotNull(request); | ||
|
|
||
| MockEndpoint.assertIsSatisfied(context); |
There was a problem hiding this comment.
there was already a change for flakiness fix gnodet@c5d06bf , develocity is not reporting any flakiness since then
There was a problem hiding this comment.
You're right — thanks for catching this. Develocity confirms 0 flaky occurrences since July 11 (256 runs, only 1 flaky on July 10 which was likely a build that hadn't picked up the Awaitility fix from PR #24509 yet).
I'll drop the JmsDurableTopicIT change from this PR.
Claude Code on behalf of @gnodet
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Went through these six alongside the other flaky-test batches — they look sound, and I see @davsclaus has already approved.
Most are proper root-cause fixes: the routeId("fooConsumer") + JmsTestHelper.waitForJmsConsumerRoutes(context, "fooConsumer") readiness sync on JmsAddAndRemoveRouteManagementIT is the nicest one, and SedaBlockWhenFullTest's @Timeout(20) → @Timeout(60) is a genuine correctness fix so the inner 30s assertIsSatisfied(...) isn't killed by the method-level timeout. No Thread.sleep anywhere, and the assertIsSatisfied(context, 30, SECONDS) swaps use the latch-based timed assertion rather than a busy-wait — all good on the testing rules.
Just echoing @davsclaus's watch-item so it isn't lost: SchedulerNoPolledMessagesTest has now had its timing window widened twice in a few days (between(0, 2000) → (0, 4000), and message(2) between(200, 5000) → (200, 10000)). That one is accommodation rather than a root-cause fix — worth keeping an eye on if it keeps flaking. Not blocking; LGTM.
Reviewed with Claude Code on behalf of Andrea Cosentino (@oscerd). This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-reviewing after new commits. The updates address the previous feedback well — the JmsDurableTopicIT change was correctly dropped, and the remaining 5 test fixes follow project conventions (timed MockEndpoint.assertIsSatisfied, waitForJmsConsumerRoutes for subscription readiness, widened CI-jitter windows with explanatory comments, no Thread.sleep introduced).
Two minor observations:
-
DirectProducerBlockingTest— The class-level@Timeout(20)could theoretically conflict with the newMockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS)at line 110. In practice this is unlikely to trigger (the route resumes within ~2s), but for consistency with theSedaBlockWhenFullTestfix in this same PR (which bumped@Timeoutfrom 20→60), it might be worth bumping this one too. -
SchedulerNoPolledMessagesTest— This is the second widening of these timing windows in a few days (prior change went 500→2000ms, this goes 2000→4000ms on early messages, 5000→10000ms on backoff). Reasonable for now, but if it flakes again at these bounds, the 100ms base delay may just be too short for meaningful CI timing assertions.
Neither is blocking — the PR looks good overall.
Claude Code review on behalf of @gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
please provide a PR with really analyzed flakiness, the few that i'm looking at are making no sense to reduce the flakiness
| @Override | ||
| public void configure() { | ||
| from("activemq:queue:JmsAddAndRemoveRouteManagementTest.foo").to("mock:result"); | ||
| from("activemq:queue:JmsAddAndRemoveRouteManagementTest.foo").routeId("fooConsumer").to("mock:result"); |
There was a problem hiding this comment.
please use a specifc routeId for the test so that it cannot cause bad interactions with other tests
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Good point — renamed to "addRemoveRouteManagement-foo" to be test-specific and avoid potential collisions with other tests. Updated the waitForJmsConsumerRoutes call to match.
Fixed in 095e9c3.
| // Use a relaxed lower bound — on slow CI with GC pauses or thread | ||
| // scheduling delays, the measured blocking time can fall slightly | ||
| // below the configured 500ms timeout | ||
| assertTrue(watch.taken() > 400, "Should have blocked for at least 400ms but was " + watch.taken()); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Claude Code on behalf of gnodet
You're right — Develocity confirms only testProducerBlocksResumeTest is flaky (4 of 334 runs, 1.2%). Neither testProducerBlocksWithNoConsumers nor testProducerBlocksForSuspendedConsumer have any flaky occurrences. I've reverted the changes to both non-flaky methods.
Fixed in 095e9c3.
| template.sendBody("direct:suspended?block=true&timeout=2000", "hello world"); | ||
|
|
||
| assertMockEndpointsSatisfied(); | ||
| MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS); |
There was a problem hiding this comment.
this won't change anything as the flakiness is reported on the previous line of code
There was a problem hiding this comment.
Claude Code on behalf of gnodet
You're right — the flakiness is on template.sendBody(timeout=2000) timing out, not on the mock assertion. The background thread's await().atMost(2s) + resumeRoute() occasionally exceeds the 2s sendBody timeout under CI load.
I've replaced the mock assertion change with the actual fix:
- Increased
sendBodytimeout from2000→10000ms (the real bottleneck) - Increased the background thread's Awaitility
atMostfrom2s→10s(so it has enough headroom to detect TIMED_WAITING) - Reverted
assertMockEndpointsSatisfied()back to the original (it's fine as-is)
Fixed in 095e9c3.
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
A single change has a very small chance to have an influence on the flakiness. All others cannot have one. Please stop using AI without real look on the produced code to work on these flakiness tasks it is doing a very terrible job.
|
|
||
| sendSoManyOverCapacity(DEFAULT_URI, QUEUE_SIZE, 20); | ||
| MockEndpoint.assertIsSatisfied(context); | ||
| MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS); |
There was a problem hiding this comment.
this test is not reported as flaky https://develocity.apache.org/scans/tests?search.relativeStartTime=P90D&search.rootProjectNames=camel&search.tags=main&search.timeZoneId=Europe%2FParis&tests.container=org.apache.camel.component.disruptor.DisruptorBlockWhenFullTest&tests.sortField=FLAKY&tests.test=testDisruptorBlockingWhenFull
There was a problem hiding this comment.
it is testDisruptorExceptionWhenFull which is reported as flaky https://develocity.apache.org/scans/tests?search.rootProjectNames=camel&search.tags=main&search.timeZoneId=Europe%2FParis&tests.container=org.apache.camel.component.disruptor.DisruptorBlockWhenFullTest&tests.sortField=FLAKY&tests.test=testDisruptorExceptionWhenFull
There was a problem hiding this comment.
very likely that the flakiness is already fixed by 148ac28 given that there was no more flakiness reported since the 13th of July https://develocity.apache.org/scans/tests?search.rootProjectNames=camel&search.tags=main&search.timeZoneId=Europe%2FParis&tests.container=org.apache.camel.component.jms.integration.JmsAddAndRemoveRouteManagementIT&tests.sortField=FLAKY&tests.test=testAddAndRemoveRoute&tests.unstableOnly=false and the test is regulalry close to the 10seconds previous limits
| @@ -98,8 +100,10 @@ public void run() { | |||
| } | |||
| }); | |||
|
|
|||
| // This call will block until the route is resumed by the background thread | |||
| template.sendBody("direct:suspended?block=true&timeout=2000", "hello world"); | |||
| // This call will block until the route is resumed by the background thread. | |||
| // Use a generous timeout so the background thread has enough headroom to | |||
| // detect the TIMED_WAITING state and resume the route even under CI load. | |||
| template.sendBody("direct:suspended?block=true&timeout=10000", "hello world"); | |||
There was a problem hiding this comment.
the stacktrace sounds unrelated to the fact that it needs more time on these timeouts, it is complaining about no consumer available, which means the route with name suspended is not available
Caused by: org.apache.camel.component.direct.DirectConsumerNotAvailableException: No consumers available on endpoint: direct://suspended?block=true&timeout=2000. Exchange[]
| mock.message(0).arrives().between(0, 4000).millis().beforeNext(); | ||
| mock.message(1).arrives().between(0, 4000).millis().beforeNext(); | ||
| // the last message should be slower as the backoff idle has kicked in | ||
| // (backoffMultiplier=10 × delay=100ms = ~1000ms), but allow extra margin for CI | ||
| mock.message(2).arrives().between(200, 5000).millis().afterPrevious(); | ||
| mock.message(2).arrives().between(200, 10000).millis().afterPrevious(); |
There was a problem hiding this comment.
given that it was already multiplied by 4 in a previous commit and the test is still flaky, i have serious doubt that increasing again will change anything
| * full. | ||
| */ | ||
| @Timeout(20) | ||
| @Timeout(60) |
There was a problem hiding this comment.
60 seconds seems a bit long compared to usual execution time of tests (>7s when in success)
It is right that since last attempt 726fd8f the global timeout is too low compared to internal timeout BUT the previosu change doesn't make any sense. The flakyness reported is about too many messages received so increasing the timeout can only allow to happen more often and won't solve any solutions, so better to revert the previous change
…esumeTest The test suspends a route then sends with block=true&timeout=2000. A background thread waits for the main thread to reach TIMED_WAITING, then resumes the route. Under CI load, detecting the thread state and resuming can take more than 2 s, causing: DirectConsumerNotAvailableException: No consumers available on endpoint: direct://suspended?block=true&timeout=2000 Fix: increase both the background thread's Awaitility timeout (2s→10s) and the sendBody block timeout (2000→10000ms) so there is enough headroom even on loaded CI nodes. Develocity evidence: testProducerBlocksResumeTest has 8 flaky runs out of 716 total on main (1.1% flaky rate). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
force-pushed
the
fix/batch13-jms-seda-scheduler
branch
from
095e9c3 to
be8aa3d
Compare
gnodet
changed the title
|
Claude Code on behalf of gnodet @apupier Thank you for the thorough review. After careful re-analysis of the Develocity data and stack traces, you were right on most points:
The PR now contains only DirectProducerBlockingTest.testProducerBlocksResumeTest fix, which I believe is correct: the The other 4 tests will be investigated separately with proper root cause analysis. |
4 participants
Summary
Fix flaky
DirectProducerBlockingTest.testProducerBlocksResumeTestby increasing timeouts that are too tight for CI.The test suspends a route then sends with
block=true&timeout=2000. A background thread waits for the main thread to reachTIMED_WAITING, then resumes the route. Under CI load, detecting the thread state and resuming can take more than 2s, causingDirectConsumerNotAvailableException.Fix: increase both the background thread's Awaitility timeout (2s→10s) and the
sendBodyblock timeout (2000→10000ms).Develocity evidence:
testProducerBlocksResumeTesthas 8 flaky runs out of 716 total onmain(1.1% flaky rate). The stack trace showsDirectConsumerNotAvailableExceptionwhich is the expected result of the block timeout expiring before the background thread resumes the route.Dropped from prior version: DisruptorBlockWhenFullTest (wrong method), JmsAddAndRemoveRouteManagementIT (already fixed by 148ac28), SchedulerNoPolledMessagesTest (needs root cause analysis, not timeout widening), SedaBlockWhenFullTest (wrong failure mode — "too many messages", not timeout). These will be investigated separately with proper root cause analysis.
Claude Code on behalf of gnodet
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com