[
https://issues.apache.org/jira/browse/CAMEL-26796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18141974#comment-18141974
]
Guillaume Nodet commented on CAMEL-26796:
-----------------------------------------
Fix submitted via PR [#29564|https://github.com/apache/camel/pull/29564].
*Root cause confirmed:* In {{HazelcastAggregationRepository.remove()}}
optimistic path, the recovery entry was written to {{persistedCache}} _before_
the compare-and-remove on {{cache}}. When two threads race on the same group
they hold the same {{exchangeId}} (they both read the same stored entry). After
Thread A's compare-and-remove succeeds, Thread B's cleanup call
{{removeFromRecoverableStorage()}} deletes Thread A's recovery entry, making
the group irrecoverable on a downstream failure.
*Fix:* Move the {{persistedCache.put()}} to _after_ the successful
{{cache.remove()}} — the same order that
{{ReplicatedHazelcastAggregationRepository}} already uses correctly. The
dead-code {{removeFromRecoverableStorage()}} helper is removed.
*Tests:* Added {{checkOptimisticRemoveRaceDoesNotDeleteWinnersRecoveryEntry()}}
which deterministically reproduces the race: add → get twice as a and b (same
exchangeId) → remove(a) succeeds → remove(b) throws OptimisticLockingException
→ recover(a.getExchangeId()) must still return the group. Removed the two tests
that covered the now-gone cleanup path.
_Fix implemented by an AI coding agent (Hermes/forgebot) on behalf of gnodet._
> camel-hazelcast - HazelcastAggregationRepository optimistic remove() can
> delete the recovery entry of the thread that won the race
> ----------------------------------------------------------------------------------------------------------------------------------
>
> Key: CAMEL-26796
> URL: https://issues.apache.org/jira/browse/CAMEL-26796
> Project: Camel
> Issue Type: Bug
> Components: camel-hazelcast
> Reporter: Claus Ibsen
> Assignee: Guillaume Nodet
> Priority: Major
> Fix For: 4.24.0
>
>
> In optimistic mode with recovery, {{HazelcastAggregationRepository.remove()}}
> first puts the completed group into the recovery map, then does the
> compare-and-delete on the operational map. When the compare-and-delete fails,
> it removes the recovery entry again and throws
> {{OptimisticLockingException}}. This order came from CAMEL-26663
> (https://github.com/apache/camel/pull/29278).
> That cleanup can delete the recovery entry of the thread that won the race.
> When two threads or nodes complete the same group, both call {{remove(key,
> exchange)}} with the exchange they read from the repository. It is the same
> stored holder, so both exchanges have the *same exchange id*:
> # A puts {{persisted[id]}}, its {{cache.remove(key, holder)}} succeeds, and
> the group goes downstream, waiting for {{confirm}}.
> # B puts {{persisted[id]}} again (same content), its {{cache.remove(key,
> holder)}} fails, and {{removeFromRecoverableStorage(id, key)}} deletes A's
> recovery entry.
> # If A's downstream fails or the JVM stops before {{confirm}}, the recover
> task finds nothing, and the group is lost without a trace.
> Before CAMEL-26663 the losing thread never touched the recovery map. This is
> a new way to lose a group, under exactly the contention that optimistic
> locking exists for.
> A deterministic test: {{add}} a group, {{get}} it twice as {{a}} and {{b}},
> {{remove(a)}} succeeds, {{remove(b)}} throws {{OptimisticLockingException}},
> and {{recover(ctx, a.getExchangeId())}} must still return the group. It fails
> on main.
> A {{putIfAbsent}} or compare-and-delete for the cleanup does not help,
> because both writes have the same key and the same content. Options:
> * (a) Change both maps atomically. The non-optimistic path already uses a
> Hazelcast transaction ({{TransactionalMap}}). The optimistic path could do
> {{tCache.remove(key, holder)}} and {{tPersistentCache.put(id, holder)}} in
> one transaction, and throw {{OptimisticLockingException}} when the remove
> returns false.
> * (b) Do not clean up on failure. In the race above, the entry B leaves
> behind is A's own entry, and A's {{confirm}} removes it. The cost is the
> other failure case: when B's holder is stale (the group changed in between),
> B leaves a stale partial aggregate behind, which recovery could send if the
> process stops before the group completes.
> {{ReplicatedHazelcastAggregationRepository}} is not affected: in optimistic
> mode it removes first and only puts into the recovery map after the remove
> succeeds.
> The Redis repository has the same problem in the open PR for CAMEL-26395
> (https://github.com/apache/camel/pull/29505); see the review there.
> _Claude Code on behalf of davsclaus_
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
