CAMEL-24046: camel-sql - JdbcCachedMessageIdRepository must not cache a key whose insert failed, make cache thread-safe#24657
Conversation
… a key whose insert failed, make cache thread-safe
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Both fixes are correct and well-tested. The cache-before-insert bug fix prevents permanent message loss on DB failure — moving cache.merge() to after super.add() ensures keys are only cached after a successful DB insert. The ConcurrentHashMap/AtomicInteger migration resolves thread-safety hazards in the shared cache.
Scanner coverage: No static analysis tools (PMD, Checkstyle, semgrep, SpotBugs) available. Rely on CI.
Notes:
- The
cache.merge(key, 1, Integer::sum)aftersuper.add(key)executes unconditionally regardless of the return value. Whensuper.add()returns false (duplicate), the cache count is inflated — functionally harmless for boolean idempotent semantics and matches pre-existing behavior. volatileon thecachefield correctly ensures safe publication whenreload()replaces the map from a different thread.- The test directly validates the regression scenario (drop table → simulate failure → recreate → verify key is not cached).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code review on behalf of @gnodet
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Reviewed with focus on the cache/insert ordering and the concurrency changes — the fix is correct and I have no change requests. Summary of the verification:
Both halves of the fix address confirmed defects on main. The poisoned-cache bug is real: add() currently caches the key (cache.put, line 70) before super.add(key) (line 76), so an insert that throws leaves the key cached and every redelivery is rejected as a duplicate forever. The reordering (only cache.merge after super.add returns) fixes all paths, including the super.add() == false case (row already in DB), where caching still matches DB truth. The thread-safety half is equally justified: main uses a plain HashMap with non-volatile field and plain int counters, shared across concurrently invoked idempotent-consumer threads.
The regression test is genuine: assertFalse(repository.contains("second")) after a failed insert fails on current main (cache hit → true) and passes with the fix. Fully synchronous, no Thread.sleep.
History check: the cache-before-insert ordering dates to the original CAMEL-16770 contribution — no prior intentional decision is being reverted.
Two non-blocking observations, both pre-existing rather than introduced here:
- Inside an outer transaction (
PROPAGATION_REQUIRED),super.add()returning doesn't mean the insert is committed — if the enclosing transaction rolls back later, the DB row vanishes while the key stays cached. In practice the idempotent consumer's defaultremoveOnFailure=trueevicts the key on failure, and the new code is strictly better than the old (which cached before even attempting the insert), so no action needed — at most the comment could say "after the insert call returned" rather than "succeeded". - The hit-path
getOrDefault/mergepair isn't atomic, so a concurrentremove()between them can resurrect a cache entry. Same window existed before the PR; cache values are only presence flags and the DB remains source of truth on a miss, so it's benign.
No file overlap with the sibling camel-sql PRs #24655/#24656 (disjoint packages), so merge order between the three doesn't matter. The two build jobs were still pending at review time — worth confirming green before merge.
Reviewed with Claude Code (Fable 5) on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 11 tested, 29 compile-only — current: 11 all testedMaveniverse Scalpel detected 40 affected modules (current approach: 11).
|
4 participants
JIRA: https://issues.apache.org/jira/browse/CAMEL-24046
Two related defects in
JdbcCachedMessageIdRepository:add(key)put the key into the in-memory cache before callingsuper.add(key). If the database insert failed (outage, PK race with another node), the exception propagated and the idempotent consumer never processed the message — but the key stayed cached, so every redelivery was rejected as a duplicate. The message was never processed and never stored until JVM restart.HashMapmutated from concurrent consumer threads with no synchronization and a non-volatile field (a runtimereload()might never become visible to other threads); concurrent puts during resize can lose entries, causing duplicate processing. The hit/miss counters were unsynchronized ints.Changes:
ConcurrentHashMapwith a volatile field for safe publication onreload(), andAtomicIntegerhit/miss counters (public getters unchanged)add():contains()must return false and a redeliveredadd()must succeed once the DB is back (fails on currentmain, passes with the fix); existing cache hit/miss-count test passes unchanged, confirming counting semantics are preservedClaude Code on behalf of Croway
🤖 Generated with Claude Code