Skip to content

CAMEL-24024: Harden InOutQueueProducerAsyncLoadTest against CI flakes - #25333

Merged
davsclaus merged 2 commits into
apache:mainfrom
atiaomar1978-hub:cursor/CAMEL-24024-inout-queue-async-load-c587
Aug 5, 2026
Merged

CAMEL-24024: Harden InOutQueueProducerAsyncLoadTest against CI flakes#25333
davsclaus merged 2 commits into
apache:mainfrom
atiaomar1978-hub:cursor/CAMEL-24024-inout-queue-async-load-c587

Conversation

@atiaomar1978-hub

@atiaomar1978-hub atiaomar1978-hub commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes CAMEL-24024: InOutQueueProducerAsyncLoadTest is flaky in CI under load.

The core fix (500 messages, CountDownLatch, AtomicInteger failure tracking) was merged as CAMEL-24027 in #24626. This PR addresses remaining flake sources:

  • Inflight race: poll with Awaitility until context.getInflightRepository().size() is 0 instead of asserting immediately after executor shutdown
  • JMS listener thread: replace fail() in onMessage with AtomicInteger error tracking
  • Worker threads: catch Throwable so assertion failures in async workers increment the failure counter
  • Executor lifecycle: try/finally for shutdown; termination assert outside finally to avoid masking primary failures
  • Null-safe cleanup: guard MessageProducer.close() when no message was processed
  • Drop unnecessary public modifiers per JUnit 5 conventions

Test plan

  • ./mvnw -pl components/camel-sjms -am test -Dtest=InOutQueueProducerAsyncLoadTest

AI-generated PR description on behalf of atiaomar1978-hub

Poll until inflight repository is drained instead of asserting immediately
after executor shutdown. Track JMS listener failures with AtomicInteger
instead of fail() from the callback thread. Use try/finally for executor
shutdown and drop unnecessary public modifiers.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
@atiaomar1978-hub

Copy link
Copy Markdown
Contributor Author

Fix summary

Addresses CAMEL-24024 — flaky InOutQueueProducerAsyncLoadTest in CI.

Context: Core fix (500 msgs, CountDownLatch, failure counter) landed as CAMEL-24027 / #24626. This PR hardens remaining flake sources.

Changes:

  • Poll with Awaitility until inflight repository is drained (async route lag)
  • Track JMS listener errors via AtomicInteger instead of fail() from callback thread
  • try/finally for executor shutdown; termination assert outside finally
  • Catch Throwable in worker threads so assertion failures are counted
  • Null-safe MessageProducer.close(), named timeout constants

Verified:

./mvnw -pl components/camel-sjms -am test -Dtest=InOutQueueProducerAsyncLoadTest

AI-generated comment on behalf of atiaomar1978-hub

Count assertion failures from worker threads and avoid masking the
primary test failure when executor shutdown times out in finally.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
@atiaomar1978-hub

Copy link
Copy Markdown
Contributor Author

Review follow-up (BugBot + Grok)

Finding Action
Grok High: catch (Exception) swallows AssertionError Changed to catch (Throwable)
Grok Medium: assertTrue in finally masks primary failure Moved awaitTermination assert outside finally
Grok Medium: shared JMS session across listeners Pre-existing design; not changed in this PR
BugBot No findings in this test file (unrelated TUI diff on fork main)

Re-verified: InOutQueueProducerAsyncLoadTest passes locally.


AI-generated comment on behalf of atiaomar1978-hub

@github-actions

github-actions Bot commented Aug 5, 2026

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 5, 2026

Copy link
Copy Markdown
Contributor

🧪 CI tested the following changed modules:

  • components/camel-sjms

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

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

Modules only in current approach (10)
  • camel-jbang-mcp
  • camel-jbang-plugin-mcp
  • camel-jbang-plugin-route-parser
  • camel-jbang-plugin-tui
  • camel-jbang-plugin-validate
  • camel-launcher-container
  • camel-mllp
  • camel-sjms2
  • camel-yaml-dsl-validator
  • camel-yaml-dsl-validator-maven-plugin

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-sjms

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

All tested modules (11 modules)
  • Camel :: JBang :: MCP
  • Camel :: JBang :: Plugin :: MCP
  • Camel :: JBang :: Plugin :: Route Parser
  • Camel :: JBang :: Plugin :: TUI
  • Camel :: JBang :: Plugin :: Validate
  • Camel :: Launcher :: Container
  • Camel :: MLLP
  • Camel :: Simple JMS
  • Camel :: Simple JMS2
  • Camel :: YAML DSL :: Validator
  • Camel :: YAML DSL :: Validator Maven Plugin

⚙️ View full build and test results

@gnodet gnodet 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.

Well-crafted test-hardening PR that correctly addresses multiple real flakiness sources. Each change targets a genuine race condition or failure-masking pattern:

  • Replacing fail() in the JMS onMessage listener with AtomicInteger error tracking is the right fix — fail() from a non-test thread throws AssertionError which the JMS infrastructure silently swallows, so the test thread never sees the failure.
  • Placing executor.awaitTermination outside the finally block avoids masking the real root cause if primary assertions fail.
  • Awaitility usage for the inflight repository poll follows project conventions correctly (explicit atMost, untilAsserted, not wrapping MockEndpoint).
  • Constants extraction improves readability.
  • JUnit 5 visibility conventions followed correctly.

One minor nit posted inline (not blocking).

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of @gnodet

String response = template.requestBody("direct:start", requestText, String.class);
assertNotNull(response);
assertEquals(responseText, response);
} catch (Throwable e) {

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.

[Low — style] catch (Throwable e) is slightly broader than necessary. Since the intent is to catch both Exception (from requestBody) and AssertionError (from assertNotNull/assertEquals), using a multi-catch would be more precise:

Suggested change
} catch (Throwable e) {
} catch (Exception | AssertionError e) {

Functionally equivalent here since the error is tracked via failures, so not blocking — just a minor precision improvement.

@davsclaus davsclaus added the test label Aug 5, 2026
@davsclaus
davsclaus merged commit b94a1ce into apache:main Aug 5, 2026
5 checks passed
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.

4 participants