Repository navigation
CAMEL-25694: camel-milo - fix write/call service failures being reported as success - #27956
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:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 8 of 707 tested, 26 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (2 direct + 7 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (26)
All tested modules (35 modules, 5m 60s total)Total reactor time: 5m 60s
Top 20 slowest modules:
|
|
For reference: #28045 (CAMEL-25690) changes how the write, call, read and browse requests of Claude Code on behalf of etzel-mes |
…ted as success In SubscriptionManager.write(), call(), readValues() and browse(), the futures returned by the underlying Milo client were chained with handleAsync(). When the upstream future completed exceptionally (e.g. Bad_Timeout service fault), the handleAsync() callback ran with a non-null 'e' parameter but returned the result value (null), converting the exceptional completion into a normal completion with a null value. MiloClientProducer awaits the future with whenComplete() and only sets an exchange exception when the throwable is non-null. Since handleAsync() had already discarded the exception, the producer saw a null-completed future, reported success, and set the exchange body to null. The fix replaces handleAsync() with whenCompleteAsync() in all four methods. whenCompleteAsync() runs the side-effect (handleConnectionFailure) but propagates the original completion unchanged: exceptional stays exceptional, success stays success. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…itial write
OPC UA consumers deliver the current server node value when they subscribe.
In the original test, all routes started automatically before the write, so
each consumer received two messages: the uninitialised value at subscription
time, and the 'Foo' write. The mock expected exactly 1 message, causing a
deterministic failure ('Expected: <1> but was: <2>').
Follow the pattern established in MonitorItemTest: mark consumer routes with
autoStartup(false), write the initial value first via the server producer,
then start all consumer routes. Each consumer then sees only one notification
(the value written before it subscribed).
49a57ca to
1ef8bd8
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The production fix is correct — replacing handleAsync with whenCompleteAsync in all four SubscriptionManager methods properly preserves exceptional completions while still triggering the reconnection side-effect. The fix also resolves a latent data-loss bug in call() where handleAsync unconditionally returned null even on success, silently discarding the CallMethodResult.
However, neither new test class actually exercises the fixed code path in SubscriptionManager.
Side note: the old call() handleAsync returned null unconditionally (line 1675 before the fix) even on success, silently discarding the CallMethodResult. Worth mentioning in the commit since it's a separate data-loss bug — the other three methods returned the actual result.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Guillaume, the production change is the right one: whenCompleteAsync keeps the reconnect side effect and the original completion, so an awaited write/call/read that fails now fails the exchange and the route's error handling runs.
TL;DR
Comment - the fix in SubscriptionManager is correct and merges cleanly with the recent camel-milo work on main, but no test fails without it, and the change to the awaited call() body is neither tested nor documented.
Blockers
- None in the production code.
Other findings: 2 to address before merge, 2 non-blocking - see below.
Before merge
-
No test fails without the fix (agreeing with the three gnodet-bot threads, which the current head does not address yet).
MiloClientProducerWriteFailureTeststubsMiloClientConnectionand never reachesSubscriptionManager.SubscriptionManagerExceptionPropagationTesttests the JDKCompletableFuturecontract and theconnected == nullpath. A cheap test that does go through the fixed code is in the existingCallClientTest, which runs against a real server: before this PR an awaited call always produced anullbody (return null;inhandleAsync, there since CAMEL-12288 in 2018), now it is theCallMethodResult:@Test public void testAwaitedCallReturnsResult() { CallMethodResult result = producer1.requestBodyAndHeader(DIRECT_START_1, "foo", "CamelMiloAwait", true, CallMethodResult.class); assertNotNull(result); assertTrue(result.getStatusCode().isGood()); assertEquals("out-foo", result.getOutputArguments()[0].getValue()); }
For the failure side, main now has the
TcpRelaytest helper (used byCommunicationLossTest), and the JIRA reporter reproduced the bug by dropping request traffic through a relay; a server method that blocks longer than a smallrequestTimeoutwould give theBad_Timeoutcase from the JIRA. Once there is a real test, the two JDK contract tests and the stub-based producer test could go. -
Upgrade guide entry. Users upgrading will notice two things, and the JIRA asks to cover the first explicitly: an awaited
callnow returns theCallMethodResultas the body instead ofnull, and an awaited write/call/read that fails at the service or transport level now fails the exchange (soonException, redelivery and dead letter run) instead of continuing with anullbody. A short=== camel-milo - ...entry incamel-4x-upgrade-guide-4_24.adocwould do.
Non-blocking
-
The
ServerCertificateValidationTestchange is comments only: main already starts those consumer routes after the write since CAMEL-25645 (#27999), so the second commit no longer fixes anything. Dropping the file from the PR keeps the history clean (inline). -
The bug is on
camel-4.22.xandcamel-4.18.xas well (samehandleAsyncin all four methods), soport/camel-4.22.x/port/camel-4.18.xlabels look right if this should be backported. On an LTS patch release the newcallbody is the part to think about; restoring the result is reasonable, but it goes in the upgrade note there too.
CI is green, but it ran on 2026-10-21, before the CAMEL-25875, CAMEL-25939 and CAMEL-25913 merges on main. GitHub reports the PR as mergeable/clean, a trial merge has no conflicts, and the constructors the new tests use (SubscriptionManager, MiloClientConnection, MiloClientEndpoint, MiloClientProducer) are unchanged on main, so the risk is low; the next push will re-run CI on top of current main anyway.
Claude Code on behalf of @davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…dundant test changes - Add testAwaitedCallReturnsResult() to CallClientTest to verify that an awaited call returns a non-null CallMethodResult with a good status and the expected output argument (CAMEL-25694 regression test on the real server path) - Add upgrade guide entry for the two observable behaviour changes: * an awaited write/call/read that fails now fails the exchange * an awaited call that succeeds now returns CallMethodResult as the body - Revert comment-only changes to ServerCertificateValidationTest: main already has the autoStartup(false) fix from CAMEL-25645 (apache#27999), so these rewrites only add noise to the commit history
gnodet
added
port/camel-4.22.x
…ite-call-failures # Conflicts: # docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the main findings have been addressed.
✅ testAwaitedCallReturnsResult in CallClientTest — this is the regression test that was missing. It runs against the real embedded OPC-UA server, which routes through MiloClientProducer → MiloClientConnection.call() → SubscriptionManager.call(), exercising the exact fixed code path. Before the fix, handleAsync returned null unconditionally even on success; this test would have failed. Valid.
✅ Upgrade guide — both observable behavior changes are documented: awaited-failure-now-fails-exchange and awaited-call-success-now-returns-CallMethodResult.
✅ ServerCertificateValidationTest comment-only noise reverted.
Remaining: write() failure propagation not covered by a real server test.
testAwaitedCallReturnsResult covers the call() success path through the full stack. The original gnodet-bot threads (PRRT_kwDOAAMl7c6vRLGJ / PRRT_kwDOAAMl7c6vRLGO) also flagged that MiloClientProducerWriteFailureTest and SubscriptionManagerExceptionPropagationTest don't exercise SubscriptionManager on a connected manager with a failing upstream. That gap still exists for write(), readValues(), and browse() — there is no server-path test that verifies a Bad_Timeout on a write operation fails the exchange rather than setting a null body.
The fix itself is unambiguously correct (whenCompleteAsync preserves the exceptional completion; handleAsync did not). The production code is safe. But a reviewer reverting the fix in write() or readValues() would not be caught by the current test suite — only the call() path has a real regression test now.
This is non-blocking if the author is comfortable with that coverage gap. The CommunicationLossTest pattern (using TcpRelay to simulate network loss on an active connection) is the cleanest path to a write failure test, but it's out of scope here.
Low severity (thread PRRT_kwDOAAMl7c6vRLGS): The Javadoc on testWriteWhenDisconnectedReturnsExceptionalFuture still doesn't clarify it tests newNotConnectedResult() rather than the whenCompleteAsync fix. Suggested comment was provided in the prior review — still applies.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Guillaume, the new commit covers both points from my earlier review:
- Test -
CallClientTest#testAwaitedCallReturnsResultruns an awaited call against the real server, so it goes throughSubscriptionManager.call(). It fails without the fix: the oldhandleAsyncreturnednull, so the body wasnull. Good. - Upgrade guide - the new entry covers both changes: a failed awaited operation now fails the exchange, and an awaited call now returns the
CallMethodResult. One example in it is not right, see inline: a write with the wrong type gets a per-nodeBad_TypeMismatchStatusCode, so the future completes normally and the exchange does not fail. The JIRA asks to keep that per-node behaviour, and the code does. - The
ServerCertificateValidationTestchange is reverted, and theport/camel-4.22.x/port/camel-4.18.xlabels are on.
Non-blocking: MiloClientProducerWriteFailureTest and SubscriptionManagerExceptionPropagationTest are unchanged, so the first two gnodet-bot threads were resolved without a code change. The new real-server test now covers the call path, so that is fine. The failure path (Bad_Timeout / connection loss on a write) is still only covered through the stub, as gnodet-bot's follow-up says. A TcpRelay test can come later.
LGTM once CI is green (the main build jobs are still running).
Claude Code on behalf of @davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…s, not a transport failure
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier reviews — all outstanding issues have been addressed.
✅ Upgrade guide wording — the Bad_TypeMismatch example has been corrected. The entry now accurately distinguishes transport/service failures (which fail the exchange) from per-node status codes (which are returned as the body). Wording matches davsclaus's suggestion exactly.
✅ testAwaitedCallReturnsResult in CallClientTest — already confirmed in the previous review. Exercises the full stack through the fixed code path.
✅ ServerCertificateValidationTest revert — already confirmed.
✅ All three gnodet-bot review threads are resolved.
The low-severity Javadoc nit on testWriteWhenDisconnectedReturnsExceptionalFuture (suggestion to note it tests newNotConnectedResult() not whenCompleteAsync) was resolved without a code change. That's fine — the surrounding section comment block already explains this is the disconnected-path test, and davsclaus approved without flagging it.
The write/readValues failure path coverage gap (no real server test with a failing upstream) remains, but was already marked non-blocking. The production fix is unambiguously correct — whenCompleteAsync preserves the exceptional completion; handleAsync did not.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier reviews — the new commit addresses the remaining signature issue.
✅ requestBodyAndHeader signature fixed — DIRECT_START_1 is now correctly passed as the first (endpoint URI) argument. The previous call requestBodyAndHeader("foo", "CamelMiloAwait", true, ...) was invoking the 4-arg overload requestBodyAndHeader(Object body, String headerName, Object headerValue, Class<T> type) — without routing through producer1 to an actual endpoint, the test was never hitting the OPC-UA server and was almost certainly passing accidentally rather than through the fixed code path.
All previous findings are resolved. Production code is correct. CI pending on the latest push.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
🔄 Backport BotConflicts (🤖 agent dispatched to resolve):
ℹ️ If you push additional commits after |
|
✅ Backport to |
|
✅ Backport to |
Summary
Fix OPC-UA write/call/readValues/browse service failures being silently swallowed and reported as success when using the awaited (
CamelMiloAwait=true) mode.Root Cause
SubscriptionManager.write(),call(),readValues(), andbrowse()all chained the underlying Milo client future withhandleAsync():When the OPC-UA operation fails (e.g.
Bad_Timeoutservice fault),handleAsyncreceives a non-null exception. It callshandleConnectionFailure(e)for the reconnection side-effect, then returns the value (null), converting the exceptional completion into a normal null completion.MiloClientProducerawaits the returned future and only propagates errors when the throwable is non-null:Since
handleAsynchad already discarded the exception, the producer saw a null-completed future, set the body tonull, and reported success. No error was raised to the Camel route.Fix
Replace
handleAsync()withwhenCompleteAsync()in all four methods.whenCompleteAsyncruns the side-effect (reconnection) but propagates the original completion unchanged — exceptions stay exceptional, successes stay successful:Tests
Two new test classes:
MiloClientProducerWriteFailureTest: tests thatMiloClientProducercorrectly propagates awaited write/call failures as exchange exceptions, and that non-awaited (fire-and-forget) writes do not set exceptions. Uses aMiloClientConnectionstub returning pre-arranged futures.SubscriptionManagerExceptionPropagationTest: tests thewhenCompleteAsync-vs-handleAsyncfuture completion contract directly (no server needed), and verifies that disconnected managers return exceptional futures fromwrite(),call(), andreadValues().Fixes https://issues.apache.org/jira/browse/CAMEL-25694
Hermes Agent (Claude Sonnet 4.6) on behalf of Guillaume Nodet