[
https://issues.apache.org/jira/browse/CAMEL-26395?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18137470#comment-18137470
]
Claus Ibsen commented on CAMEL-26395:
-------------------------------------
[~mgabr] thanks for the report, the problem is real. Apart from a crash between
the two calls, a failing {{persistedCache.put}} (network error, Redis error)
loses the group the same way: the remove already went through, and the
exception goes back to the incoming exchange.
Wrapping the optimistic branch in an {{RTransaction}} would break the
optimistic locking though. In Redisson 4.8.0 the transactional {{remove(key,
value)}} compares the value on the client, and at commit sends the remove in
MULTI/EXEC without checking its result. The optimistic {{add()}} uses plain
{{replace}}/{{putIfAbsent}}, which do not take the transaction lock. So when
another node adds a message to the group in between, the remove silently does
nothing but the {{put}} into the recovery map still runs: the old aggregate
goes downstream and the group completes again later, instead of an
{{OptimisticLockingException}} and a retry. Today {{cache.remove(key, holder)}}
is a compare-and-delete on the server (a Lua script inside Redisson), and that
must stay.
Suggested approach, stage in the recovery map first:
# {{persistedCache.put(exchangeId, holder)}} when {{useRecovery}}
# {{cache.remove(key, holder)}} as today
# if the remove fails, remove the staged entry again
({{persistedCache.remove(exchangeId)}}) and throw {{OptimisticLockingException}}
The worst case becomes a duplicate instead of a loss, which is what the
recovery feature gives anyway, and it keeps working on Redis Cluster. (One Lua
script doing compare + HSET + HDEL would be strictly atomic, but the two maps
hash to different slots on a cluster, so it would need renamed keys with a hash
tag; not worth it here.)
For the tests in {{RedisAggregationRepositoryOperationsIT}}:
* make the {{put}} fail without a crash, e.g. store a string under the
persistence map name ({{redisson.getBucket(persistenceMapName).set("x")}}) so
it fails with WRONGTYPE, call the optimistic {{remove()}}, and assert it throws
and the group is still in the active map (fails on main today)
* a remove with a stale holder throws {{OptimisticLockingException}} and leaves
nothing in the recovery map
A PR is very welcome, as you offered. Did you see this in production or find it
reading the code, and do you run a single Redis or Redis Cluster?
_Claude Code on behalf of Claus Ibsen (davsclaus)_
> camel-redis - Potential exchange loss in RedisAggregationRepository
> optimistic removal
> --------------------------------------------------------------------------------------
>
> Key: CAMEL-26395
> URL: https://issues.apache.org/jira/browse/CAMEL-26395
> Project: Camel
> Issue Type: Bug
> Reporter: Markus Gabriel
> Priority: Major
>
> In {{{}RedisAggregationRepository.remove(){}}}, the optimistic branch
> executes the delete and recovery staging as two separate, non-atomic
> operations:
> {code:java}
> if (!cache.remove(key, holder)) {
> throw new OptimisticLockingException();
> }
> if (useRecovery) {
> persistedCache.put(exchange.getExchangeId(), holder);
> }{code}
> If the JVM crashes or gets killed right between {{cache.remove(...)}} and
> {{persistedCache.put(...)}} (or if the {{put}} throws an exception), the
> exchange is permanently removed from the active cache without ever making it
> into the recovery store.
> The non-optimistic branch right below it already wraps this handoff in an
> {{RTransaction}} to keep it atomic. It seems like the optimistic branch
> should do the same to prevent dropping exchanges.
> I'm happy to open a PR for this if the approach makes sense.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
