Fix flaky tests: Disruptor, Seda, Scheduler (batch 16)#24804
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: 24 tested, 0 compile-only — current: 9 all testedMaveniverse Scalpel detected 24 affected modules (current approach: 9).
|
bae8b08 to
ac3d816
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
LGTM — three well-analyzed flaky test fixes, each targeting the actual root cause.
DisruptorBlockWhenFullTest — .syncDelayed() makes the consumer block for the full 100ms
inside onEvent(), keeping the ring buffer full long enough for tryNext() to reliably hit
InsufficientCapacityException. Without it, the async delay returns immediately and the buffer
drains too fast.
SedaBlockWhenFullTest — expectedMinimumMessageCount(QUEUE_SIZE + 1) is the correct
semantic: with blockWhenFull=true, all messages eventually arrive. The exact-count assertion
raced against additional messages flowing through the pipeline after the latch opened.
SchedulerNoPolledMessagesTest — Direct timestamp comparison on already-received exchanges
eliminates the arrives().afterPrevious() race entirely. Verified that MockEndpoint stores
RECEIVED_TIMESTAMP as new Date() (camel-mock MockEndpoint.java:1848), so the
getProperty(Exchange.RECEIVED_TIMESTAMP, Date.class) access is safe.
Claude Code on behalf of gnodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Nice batch — all three fixes target the actual mechanism of the flakiness rather than widening timeout windows. Clean work.
What's good
- Root cause depth is outstanding. Each fix targets the specific mechanism: async delay semantics in Disruptor, exact-vs-minimum count race in SEDA, and the
arrives()API race in Scheduler. The Develocity data provides empirical backing. - The SchedulerNoPolledMessagesTest rewrite is a genuine upgrade. After the prior widening attempt (commit
3009c0da4c7e), this PR takes the fundamentally better approach: wait for messages, then examine timestamps on a stable list. Eliminates the entire class of race. - Comment quality is excellent. Each change explains why, not just what.
Minor suggestions
SchedulerNoPolledMessagesTest — dropped upper bound on backoff gap
core/camel-core/src/test/java/org/apache/camel/component/scheduler/SchedulerNoPolledMessagesTest.java
The old code checked between(200, 5000) for the backoff gap. The new code only checks gap12 >= 200 (lower bound only). Dropping the upper bound is the right call for CI stability, but if you want to catch a hypothetical regression where the scheduler ignores the backoff entirely and waits far too long, a generous upper bound could help:
assertTrue(gap12 <= 10000,
"Backoff gap should not exceed 10s (expected ~1000 ms), was " + gap12 + " ms");Truly optional — the test's purpose is verifying backoff kicks in, not capping it.
Verified claims
- ✅
delay()defaults to async:DelayProcessorSupport.java:44—private boolean asyncDelayed = true - ✅
Exchange.RECEIVED_TIMESTAMPexists and is set by MockEndpoint - ✅ No
Thread.sleep()introduced - ✅
MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS)used correctly per project guidelines - ✅ All new imports are used; no unused imports
CI status
Two jobs failed (build (17, false) and build (25, false)) — both on camel-smb integration tests, unrelated to this PR.
Checklist
| Check | Status |
|---|---|
| Tests | ✅ This PR IS the test fix |
| Thread.sleep() | ✅ None introduced |
| MockEndpoint usage | ✅ Correct per guidelines |
| Documentation | N/A (test-only) |
| Commit convention | ✅ |
| Public API / backward compat | N/A (test-only) |
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.
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
LGTM — all three attack the actual race rather than widening a window:
DisruptorBlockWhenFullTest:.syncDelayed()makes the delay block on the event-loop thread so the ring buffer stays full andInsufficientCapacityExceptionfires reliably (delay()is async by default).SedaBlockWhenFullTest:expectedMessageCount→expectedMinimumMessageCount— the correct semantic forblockWhenFull=true(all messages eventually arrive), removing the exact-count-vs-latch race.SchedulerNoPolledMessagesTest: replaces the fragilearrives().afterPrevious()with a directRECEIVED_TIMESTAMPcomparison on already-received exchanges — the cleanest of the batch.
No Thread.sleep added. The red build (25) is an unrelated camel-smb Testcontainers IT flake — camel-smb is only pulled into the reactor as a transitive dependent of the modified camel-core (which itself built and tested fine), and build (17) was cancelled by matrix fail-fast, not an independent failure. A re-run should clear it. (Minor: the SchedulerNoPolledMessagesTest change drops the old upper bound on the backoff gap, which you already flagged as optional in your own note.)
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.
The delay() EIP defaults to async mode, so the consumer's onEvent() returns in microseconds — the ring buffer drains nearly as fast as the producer fills it. On a loaded CI, the buffer may never be full at the instant tryNext() is called, preventing the expected exception. Add syncDelayed() so the consumer blocks for the full 100ms delay inside onEvent(), keeping the ring buffer full long enough for the producer to reliably hit InsufficientCapacityException. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
With blockWhenFull=true and async sends, all messages eventually arrive at the mock (they block and wait for queue space rather than failing). The exact-count assertion (expectedMessageCount) races: after the latch opens at count N, additional messages can arrive before assertEquals runs, causing "expected N but was N+k". Switch to expectedMinimumMessageCount which uses a >= check — immune to additional messages arriving after the latch opens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The arrives().afterPrevious() API has a subtle race: when computing "after previous", it actually compares with the NEXT message's timestamp (if one has arrived), not the previous one. A 4th message arriving ~100ms after the 3rd violates the 200ms lower bound that was intended for the backoff gap between messages 1 and 2. Replace the fragile MockEndpoint timing API with direct timestamp comparison on already-received (immutable) exchanges after assertIsSatisfied completes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ac3d816 to
cd9a7a6
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Re-review after new commits — all three fixes look solid.
DisruptorBlockWhenFullTest
.syncDelayed() is the right fix. Without it, the default async delay causes messages to pass through the ring buffer too quickly, so the buffer may not be full when the test expects InsufficientCapacityException. With .syncDelayed(), the consumer blocks for the full delay period, keeping the slot occupied. The comment explaining the rationale is a nice touch.
SchedulerNoPolledMessagesTest
Excellent analysis of the arrives().afterPrevious() race condition. The direct timestamp comparison using Exchange.RECEIVED_TIMESTAMP is more reliable and avoids the subtle issue where a 4th message arriving ~100ms after the 3rd violates the lower bound intended for the backoff gap (msg 1→2). The wide timing windows (gap01 ≤ 2000ms, gap12 ≥ 200ms) are appropriately generous for CI environments.
SedaBlockWhenFullTest
expectedMinimumMessageCount(QUEUE_SIZE + 1) is correct. With blockWhenFull=true and async sends, all messages eventually arrive — they block and wait for queue space rather than failing. An exact-count assertion races against the remaining messages still flowing through the 130ms delay pipeline. Good explanatory comment.
All changes are well-documented with comments explaining the reasoning. LGTM ✅
3 participants
Claude Code on behalf of gnodet
Summary
Fix 3 flaky tests identified by Develocity, each with a distinct root cause:
1.
DisruptorBlockWhenFullTest.testDisruptorExceptionWhenFull(11 flaky / 715 runs — 1.5%)Root cause: The
delay(100)EIP defaults to async mode. The consumer'sonEvent()returns in microseconds because the delay is scheduled on a background executor — it does not block the ring buffer consumption. Under CI load, the buffer never fills up andtryNext()succeeds instead of throwingInsufficientCapacityException.Fix: Add
.syncDelayed()so the consumer blocks for the full 100ms insideonEvent(), keeping the ring buffer full long enough for the producer to reliably hit the exception.2.
SedaBlockWhenFullTest.testAsyncSedaBlockingWhenFull(65 flaky / 716 runs — 9.1%)Root cause: With
blockWhenFull=trueand async sends, all messages eventually arrive (they block and wait for queue space). The exact-count assertion (setExpectedMessageCount(2)) opens itsCountDownLatchafter 2 messages, then races against additional messages arriving beforeassertEqualsruns — causing "expected 2 but was 3+".Fix: Switch to
expectedMinimumMessageCount(2)which uses a>=check. The test's purpose is verifying that blocked messages eventually succeed, not that exactly N arrive.3.
SchedulerNoPolledMessagesTest(75 flaky / 716 runs — 10.5%)Root cause: The
arrives().afterPrevious()MockEndpoint API has a subtle race: when computing "after previous", it actually compares with the next message's timestamp (if already received), not the previous one. A 4th message arriving ~100ms after the 3rd violates the 200ms lower bound intended for the backoff gap between messages 1→2.Prior widening attempts (4× wider windows) didn't help because the race is structural, not a matter of window width.
Fix: Replace the fragile timing API with direct timestamp comparison on already-received (immutable) exchanges after
assertIsSatisfied()completes. This is immune to additional messages arriving concurrently.Test plan
DisruptorBlockWhenFullTest— 2 tests pass (both blocking and exception variants)SedaBlockWhenFullTest— 4 tests pass (all variants including the fixed async one)SchedulerNoPolledMessagesTest— passes with direct timestamp assertions🤖 Generated with Claude Code