Repository navigation
CAMEL-26701: camel-chaos - a fault caught by a circuit breaker fallback counts as completed, and a recovered fault marks no later error - #29363
Merged
Conversation
…ck counts as completed, and a recovered fault marks no later error Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
…eeps the mark from the recent faults Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
gnodet-bot
reviewed
Nov 6, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
The two bug fixes are correct and well-motivated. The circuit breaker copy detection via CORRELATION_ID + TRY_ROUTE_BLOCK is the right approach, and the deferred-counting design through ConcurrentHashMap is sound. One low-severity concurrency gap in countWaitedTooLong worth fixing before merge (see inline).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 11 of 717 tested, 3 compile-only — current: 11 all testedMaveniverse Scalpel detected 11 affected modules (current approach: 11). Skip-tests mode would test 11 modules (3 direct + 0 downstream), skip tests for 3 (generated code, meta-modules) Modules Scalpel would test (11)
Modules with tests skipped (3)
All tested modules (38 modules, 7m 2s total)Total reactor time: 7m 2s
Top 20 slowest modules:
|
…, also when a copy merges into them meanwhile Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
gnodet-bot
reviewed
Nov 6, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Following up on my earlier review — the previous finding is now fully addressed.
Previous finding — countWaitedTooLong (removeIf predicate running without bin lock): ✅ Fixed.
The rewrite uses waitingForOriginal.remove(e.getKey(), w) which atomically removes the entry only when the value hasn't been replaced by a concurrent merge(). This is the correct ConcurrentHashMap pattern: if a redelivery copy merges into the entry and replaces the value reference between the iterator read and the remove, remove(key, oldValue) returns false and the count is skipped — the new entry will be counted when the original exchange fires originalDone() or on the next countWaitedTooLong sweep. No double-counting, no missed count.
The rest of the implementation is correct:
Faults.merge()runs only inside theConcurrentHashMap.merge()remapping function (under the bin lock), and after removal incountWaitedTooLongororiginalDone(), theFaultsinstance is exclusive — no concurrent access.- The
isIgnoreExchangeCompletedEvent()optimization is safe: a missed event is handled by the 60-secondcountWaitedTooLongfallback (documented known limit). - The
ChaosMarks.ruleOf()semantic change is correct: empty map = properties not captured (fall back to recent faults), non-empty but noPROPERTYkey = not chaos (return null, do not fall back). WaitingFaults.sincebeing reset tocopy.sinceon each redelivery merge is intentional and correct — the 60-second timeout should start from the last copy, not the first.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…e merge of main Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CAMEL-26701: two bugs from a hands-on round of the chaos screen, using a demo shop with a payment call behind a resilience4j circuit breaker that has a fallback.
1. A fault caught by a circuit breaker fallback counted as failed
The rule on the payment call showed
156 failed, while the payment route had 0 failed and every order shipped through the fallback. The circuit breaker runs its steps on a correlated copy of the exchange, with its own unit of work (ResilienceProcessor.processTask). Chaos faulted the copy and counted the effect when the copy's unit of work was done, so as failed. The exchange itself was never counted.Now the faults of a copy made inside a try block (a correlated copy with
TRY_ROUTE_BLOCK, which the circuit breaker sets) count when the exchange they are a copy of is done, with its outcome:completedwhen the fallback took over;handledby the dead letter channel;recoveredwhen a redelivery succeeded;failedotherwise.A message that the circuit breaker runs again at a redelivery (a new copy each time) counts once (
Faults.merge).The exchange is not touched from the copy's thread, as a circuit breaker with a timeout runs its fallback on the caller while the copy may still run. The faults wait under the exchange id in
ChaosManageruntil itsExchangeCompletedEventorExchangeFailedEvent. The notifier listens for completed events only while something waits (isIgnoreExchangeCompletedEvent), so it costs nothing otherwise. The map is bounded. A wait with no event within a minute counts with how the copy was done, once, even when a copy merges into it at that moment (it is removed withremove(key, value)before it counts): for example a wire tap inside adoTry, whose exchange may be done before its copy. The inflight repository is not used to find the exchange, as browsing it is off by default.Without a fallback, a circuit breaker's failure is not redelivered by the error handler of its own route. So the redelivery test puts the circuit breaker in a route called with
direct:, from a route with a dead letter channel.Known limit, in the javadoc: a splitter inside a
doTryalso makes correlated copies in a try block, so a fault in a split part counts with the parent's outcome (completed when thedoCatchcaught it).2. A recovered fault marked a later, real error
CamelChaosRulestayed on an exchange after a redelivery recovered its fault. So a later failure that chaos did not make showed the mark of the old rule on the Errors tab: in the demo, the open circuit breaker'sCallNotPermittedExceptionwas marked🎲r1, the rule on the stock call. Now:ChaosMarks.ruleOf), and uses the recent faults of the report only when an error has no properties. An error whose properties were not captured has an empty map, which counts as none.Also
chaos.adocdescribes both; the catalog copy is updated.Tests
ChaosCircuitBreakerEffectTest: the fallback, the dead letter channel without a fallback (one message over three copies), recovered by a redelivery, no mark on a later failure, and the one-minute timeout (two copies of one exchange count once, as handled).ChaosMarksTestgets a case for an error with properties and no mark).Claude Code on behalf of Claus Ibsen
🤖 Generated with Claude Code
https://claude.ai/code/session_012nJsFsjqqFiwroeHRWaKbS