Skip to content

CAMEL-24466: Fix flaky KafkaConsumerHealthCheckIT.testReadinessWhenDown - #25639

Closed
davsclaus wants to merge 2 commits into
mainfrom
fix/CAMEL-24466
Closed

davsclaus wants to merge 2 commits into
mainfrom
fix/CAMEL-24466

Conversation

@davsclaus

Copy link
Copy Markdown
Contributor

Issue

CAMEL-24466

KafkaConsumerHealthCheckIT.testReadinessWhenDown fails intermittently in CI with ConditionTimeout — the readiness health check does not report DOWN within the 20s Awaitility window after the broker is shut down. In the reported run it failed all 3 attempts (initial + 2 rerunFailingTestsCount reruns), so it was not masked as flaky.

Root cause

Readiness down-detection is driven by KafkaFetchRecords.isReady(), which (while connected stays true) relies on the Kafka client's ConsumerNetworkClient.hasReadyNodes(now). When a live broker is killed without a prompt TCP reset (typical for a container stop under CI load), the client only marks the node not-ready once the in-flight request times out — bounded by the consumer's request.timeout.ms, which defaults to 30s (KafkaConfiguration.consumerRequestTimeoutMs = 30000).

The test's 20s await window is shorter than that 30s bound, so detection sometimes has not completed when Awaitility gives up → flaky ConditionTimeout.

This is corroborated by the sibling tests that are not flaky (KafkaConsumerBadPortHealthCheckIT, KafkaConsumerUnresolvableHealthCheckIT): they connect to a bad/unresolvable endpoint, so a connection never becomes ready and hasReadyNodes() returns false almost immediately — 20s is ample there. Only testReadinessWhenDown establishes a READY connection first and then kills the broker, exposing the ~30s detection delay.

Fix

  • Widen the Awaitility window from 20s → 45s so it comfortably exceeds the 30s request.timeout.ms detection bound.
  • Add a method-level @Timeout(60) (overriding the class-level @Timeout(30), which would otherwise kill the method first).

Both are upper bounds, not sleeps: Awaitility returns as soon as the condition is met, and JUnit only fails past the ceiling, so passing runs are not slowed. Eliminating the fail-then-rerun cycle tends to reduce total CI time for this class.

Also dropped the unnecessary public modifiers on the touched class/method per JUnit 5 conventions.

Testing

  • mvn -DskipTests install on camel-kafka passes (test sources compile; formatter:format / impsort:sort produce no changes).
  • Change is confined to a single test method (annotation + await ceiling + explanatory comment); no production code and no generated artifacts are affected (generate-postcompile reported everything up to date).

Claude Code on behalf of davsclaus

Readiness down-detection relies on the Kafka client's hasReadyNodes(), which only flips after connection-failure detection bounded by request.timeout.ms (default 30s). The 20s Awaitility window was shorter than that bound, so a killed-broker connection was not always detected in time, causing intermittent ConditionTimeout failures in CI (all 3 rerun attempts failed).

Widen the await window to 45s and add a method-level @timeout(60) (overriding the class-level @timeout(30)) so detection can complete reliably. Both are upper bounds and Awaitility returns as soon as the condition is met, so passing runs are not slowed. Also drop unnecessary public modifiers per JUnit 5 conventions.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run
  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
  • You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
  • Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@github-actions

github-actions Bot commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

🧪 CI tested the following changed modules:

  • components/camel-kafka

🔬 Scalpel shadow comparison — Scalpel: 1 tested, 0 compile-only — current: 10 all tested

Maveniverse Scalpel detected 1 affected modules (current approach: 10).

Modules only in current approach (9)
  • camel-jbang-mcp
  • camel-jbang-plugin-mcp
  • camel-jbang-plugin-route-parser
  • camel-jbang-plugin-tui
  • camel-jbang-plugin-validate
  • camel-launcher-container
  • camel-yaml-dsl-validator
  • camel-yaml-dsl-validator-maven-plugin
  • dummy-component

Skip-tests mode would test 1 modules (1 direct + 0 downstream), skip tests for 0 (generated code, meta-modules)

Modules Scalpel would test (1)
  • camel-kafka

ℹ️ Shadow mode — Scalpel observes but does not affect test execution. Learn more

⚠️ Some tests are disabled on GitHub Actions (@DisabledIfSystemProperty(named = "ci.env.name")) and require manual verification:

  • components/camel-kafka: 2 test(s) disabled on GitHub Actions
All tested modules (9 modules)
  • Camel :: JBang :: MCP
  • Camel :: JBang :: Plugin :: MCP
  • Camel :: JBang :: Plugin :: Route Parser
  • Camel :: JBang :: Plugin :: TUI
  • Camel :: JBang :: Plugin :: Validate
  • Camel :: Kafka
  • Camel :: Launcher :: Container
  • Camel :: YAML DSL :: Validator
  • Camel :: YAML DSL :: Validator Maven Plugin

⚙️ View full build and test results

@apupier apupier left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think that this test is flaky. it has consistently failed on the last 2 builds of jenkins on all 3 JDKs version and failed also on github.

…teFixture reflection works on Java 25

The class was made package-private as a JUnit 5 convention cleanup, but the
test-infra CamelContextExtension invokes the @RouteFixture createRouteBuilder
method reflectively. On Java 25, Method.invoke on a package-private receiver
class throws IllegalAccessException even for a public method, breaking all
tests at fixture setup. Restore the public modifier (the three sibling
HealthCheck ITs are already public for the same reason).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
@apupier

apupier commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

provided #25645 to fix the test which is not flaky

@davsclaus

Copy link
Copy Markdown
Contributor Author

Closing this in favor of #25645, which fixes the actual root cause.

The timeout increase here can't work: the failure is deterministic, not a timing flake. testReadinessWhenDown calls service.shutdown() to simulate a downed broker, but KafkaHealthCheckTestSupport uses KafkaServiceFactory.createSingletonService(), and SingletonService.shutdown() is a deliberate no-op ("Ignoring shutdown request ... will be shutdown via JVM shutdown hook"). So the broker never actually goes down — the consumer stays connected, the readiness check keeps reporting UP, and the await(...DOWN...) times out at any window (confirmed at both 20s and 45s in CI).

This was introduced by CAMEL-24387 (#22294), which migrated these health-check ITs to the singleton service. #25645 correctly reverts just this base class back to createService(), whose shutdown() genuinely stops the container.

Verified locally with a real Kafka container:

Also filing a follow-up: under group.protocol=consumer (AsyncKafkaConsumer), the reflection in the readiness check can't reach ConsumerNetworkClient, so it would never report DOWN — a separate latent issue.

Claude Code on behalf of davsclaus

@davsclaus davsclaus closed this Aug 25, 2026
@github-actions
github-actions Bot deleted the fix/CAMEL-24466 branch August 25, 2026 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants