Claus Ibsen created CAMEL-26796:
-----------------------------------
Summary: 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
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)
