Fix flaky Quartz tests: widen timing windows and fix latch timeout#24819
Conversation
…out bug - QuartzAddRoutesAfterCamelContextStartedTest: increase repeatInterval from 100ms to 500ms so Quartz can reliably fire both triggers on loaded CI machines; add timed assertIsSatisfied - QuartzAddDynamicRouteTest: increase repeatInterval from 2ms to 200ms (2ms was far too tight for any scheduler to handle reliably); add timed assertIsSatisfied - QuartzCronRouteWithStartDateEndDateTest: increase startAt offset from 3s to 5s and widen trigger window from 2s to 4s so Quartz scheduler initialization doesn't eat into the firing window; replace redundant mock.await()+assertIsSatisfied with single timed assertIsSatisfied - CronScheduledRoutePolicyTest$CronTest5: fix CountDownLatch.await() timeout from 5000 SECONDS (83 minutes!) to 30 SECONDS — an obvious typo; also assert the latch result to fail fast on timeout Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean, well-targeted fixes. All four changes address genuine timing issues in tests.
Review
1. QuartzAddDynamicRouteTest (repeatInterval=2 → 200) ✅
A 2ms repeat interval is far below Quartz's practical scheduling granularity — the scheduler thread pool can't reliably fire two triggers within 4ms. 200ms is reasonable. The timed assertIsSatisfied(context, 30, TimeUnit.SECONDS) is the correct pattern per project guidelines.
2. QuartzAddRoutesAfterCamelContextStartedTest (repeatInterval=100 → 500) ✅
100ms was tight for CI. Two firings need to complete within ~200ms total, which fails when Quartz's thread pool is starved under CI load. 500ms gives ample room. Good switch to timed assertion.
3. QuartzCronRouteWithStartDateEndDateTest (widened window + headroom) ✅
Start offset 3s→5s and window 2s→4s give the Quartz scheduler time to initialize before the trigger window opens. The upper bound assertion widened from <= 3 to <= 5 matches the 4-second window correctly (a per-second cron in a 4s window fires up to 5 times at 0, 1, 2, 3, 4s). Good cleanup removing the redundant mock.await() + assertIsSatisfied() chain — MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS) already waits internally via its latch.
4. CronScheduledRoutePolicyTest$CronTest5 (latch await typo fix) ✅
This is the highest-value fix in the batch. await(5000, TimeUnit.SECONDS) = 83 minutes — a clear typo that would cause an 83-minute CI hang on failure. The fix to assertTrue(startedLatch.await(30, TimeUnit.SECONDS), ...) is correct on two fronts:
- Fixes the timeout to a sensible value (30s)
- Wraps in
assertTruewith a message for fail-fast behavior — the previous code silently ignored theawait()return value
Checklist
| Check | Status |
|---|---|
| Tests | ✅ This IS the test fix (4 flaky tests) |
| Thread.sleep() | ✅ None introduced |
| MockEndpoint usage | ✅ Timed assertions per project guidelines |
| Production code | ✅ No production changes |
| Commit convention | ✅ |
Reviewed with Claude Code on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 1 tested, 0 compile-only — current: 12 all testedMaveniverse Scalpel detected 1 affected modules (current approach: 12). Modules only in current approach (11)
Skip-tests mode would test 1 modules (1 direct + 0 downstream), skip tests for 0 (generated code, meta-modules) Modules Scalpel would test (1)
All tested modules (12 modules)
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
LGTM — these are genuine fixes, not masking. The repeatInterval widenings correct absurd values (2ms → 200ms, 100ms → 500ms) and then rely on the latch-based MockEndpoint.assertIsSatisfied(context, 30, SECONDS) upper bound rather than a sleep, and the best one is the CronScheduledRoutePolicyTest latch typo — await(5000, SECONDS) (~83 minutes!) → assertTrue(latch.await(30, SECONDS), ...), which also fixes a silently-ignored return value, so it's strictly stronger. No Thread.sleep added and the original assertions are preserved. CI green.
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
| // use a reasonable interval (200ms) so that the Quartz | ||
| // scheduler can reliably fire both triggers on loaded CI | ||
| // machines (2ms was far too tight for reliable scheduling) | ||
| from("quartz://myGroup/myTimerName?trigger.repeatInterval=200&trigger.repeatCount=1").routeId("myRoute") | ||
| .to("direct:foo"); | ||
| } | ||
| }); | ||
|
|
||
| resultEndpoint.assertIsSatisfied(); | ||
| MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS); |
There was a problem hiding this comment.
I think that the point of having a 2ms scheduling was too ensure that the repeatCount i well respected. By increasing to 200ms, we need to add some wait of at least 200ms to ensure that no other route is triggered and so that we have only 1 and then 2 messages.
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Thanks for the careful review! The 2ms interval was not originally chosen to validate repeatCount enforcement — the original commit (a056a93, 2011) says "Added test based on user forum issue" and the test's purpose is verifying that dynamically adding a Quartz route to a running CamelContext works.
Neither interval validates "exactly N, no more" because MockEndpoint.expectedMessageCount(2) returns as soon as 2 messages arrive without waiting to detect extras — you would need setAssertPeriod() for that, which neither the old nor new test uses.
The flakiness root cause is Quartz's misfire handling: triggerStartDelay (default 500ms, see QuartzEndpoint line 96) delays fires to T+500ms and T+502ms. On loaded CI, if the Quartz scheduler thread is delayed by just 3ms, both fire times are in the past when the scheduler catches up. MISFIRE_INSTRUCTION_FIRE_NOW (line 558) consolidates them into a single fire — yielding 1 message instead of 2. Widening to 200ms gives the scheduler a 200ms window between fires, making it resilient to brief scheduler delays.
That said, if you think it would be valuable, we could add resultEndpoint.setAssertPeriod(500) to verify no extra messages arrive after the expected 2. This would address the repeatCount concern independently of the interval change.
| // use a generous interval (500ms) so that the Quartz scheduler | ||
| // can reliably fire both the initial and repeat triggers even | ||
| // on heavily-loaded CI machines (100ms was too tight) | ||
| from("quartz://myGroup/myTimerName?trigger.repeatInterval=500&trigger.repeatCount=1").to("mock:result"); |
There was a problem hiding this comment.
If the message is not sent when the repeat interval is low, I guess this is a bug and we should not change the test but fix the runtime. Isn't it?
or we consider it normal to drop a message?
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Good question! No message is actually dropped by Camel — the "lost" message is caused by Quartz's built-in misfire handling, not the Camel runtime.
Here is the mechanism: triggerStartDelay (default 500ms, see QuartzEndpoint line 96) ensures the endpoint is fully started before any fire. With repeatInterval=100ms and repeatCount=1, fires are at T+500ms and T+600ms. On loaded CI, if the Quartz scheduler thread is delayed by ~110ms (CPU contention, GC), both fire times are in the past when the scheduler catches up. Quartz's MISFIRE_INSTRUCTION_FIRE_NOW (QuartzEndpoint line 558) consolidates these misfires into a single fire — by design. This is standard Quartz behavior, not a Camel bug.
This test's interval has been adjusted before:
- 2010 (CAMEL-3203, original): 2ms
- 2013 (CAMEL-4075, camel-quartz2 port): increased to 1000ms
- 2021 (parallel tests refactor): reduced to 100ms for speed — which introduced the flakiness
- This PR: increased to 500ms
The test's purpose (CAMEL-3203) is verifying that adding routes after CamelContext started works, not stress-testing Quartz's sub-second scheduling. The interval just needs to be large enough that the scheduler can reliably fire both triggers without misfire consolidation.
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
I have serious doubt that it will change anything (apart from increasing the time of timeout when the test is still flaky) but let's try
4 participants
Summary
Claude Code on behalf of gnodet
Fix 4 flaky tests in
camel-quartzidentified by Develocity analytics:QuartzAddRoutesAfterCamelContextStartedTestrepeatInterval=100mstoo tight for loaded CIQuartzCronRouteWithStartDateEndDateTestQuartzAddDynamicRouteTestrepeatInterval=2ms— absurdly tight for any schedulerCronScheduledRoutePolicyTest$CronTest5await(5000, TimeUnit.SECONDS)— 83 minutes! (typo)await(30, TimeUnit.SECONDS), assert resultDetails
QuartzAddRoutesAfterCamelContextStartedTest: The Quartz scheduler thread pool can be starved on loaded CI machines. A 100ms repeat interval with
repeatCount=1means two firings must happen within ~200ms — too tight. Increased to 500ms.QuartzCronRouteWithStartDateEndDateTest: The cron trigger starts 3 seconds in the future and fires for only 2 seconds. On loaded CI, Quartz scheduler initialization can take >1 second, eating into the headroom. Increased start offset to 5s and window to 4s. Also replaced the redundant
mock.await()+assertIsSatisfied()chain with a single timedassertIsSatisfied.QuartzAddDynamicRouteTest: A 2ms repeat interval is far below Quartz's scheduling granularity. Both firings can happen before the mock is ready, or the scheduler may miss them entirely. Increased to 200ms.
CronScheduledRoutePolicyTest$CronTest5:
startedLatch.await(5000, TimeUnit.SECONDS)is clearly a typo — 5000 seconds = 83 minutes. Fixed to 30 seconds with proper assertion of the latch result for fail-fast behavior.Test plan
🤖 Generated with Claude Code