[Bug] POP revive can lose retry after an asynchronous message-read failure
- Dominant language
- Java
- Stars
- 22.6k
- Forks
- 12k
- Avg merge
- 3d 1h
- Merged PRs (30d)
- 27
Description
## Before Creating the Bug Report
- [x] I found a bug, not just a question for GitHub Discussions.
- [x] I searched GitHub Issues, PRs, and Discussions and believe this is not a duplicate.
- [x] I confirmed that this bug belongs to the `apache/rocketmq` repository.
## Runtime platform environment
Reproduced on Ubuntu 22.04.4 LTS, Linux 5.15.0-186, x86_64. The defect is in the broker's asynchronous POP revive control flow and is not expected to be OS-specific.
## RocketMQ version
- Branch: `develop`
- Version: current `5.5.1` development sources
- Commit: `e348efa66b08eb645ee123706ea6492fa9a3ad35`
## JDK Version
OpenJDK 8u502 (Maven runtime).
## Describe the Bug
Proposed severity: **High**, because the failure can violate POP's at-least-once redelivery guarantee for an unacknowledged message.
`PopReviveService.reviveMsgFromCk` starts an asynchronous business-message read for every unacknowledged offset and records the checkpoint in `inflightReviveRequestMap`. `mergeAndRevive` then advances and commits the revive offset after scheduling that work.
If `EscapeBridge.getMessageAsync` (including its decode/transform chain) completes exceptionally, the corresponding child future remains exceptional. `CompletableFuture.allOf(...).whenComplete(...)` still invokes its callback, but the callback calls `future.getNow(...)`; for an exceptionally completed future, `getNow` throws `CompletionException`. The callback therefore exits before it can:
- call `rePutCK` to preserve retryability;
- mark the in-flight checkpoint complete;
- remove it from `inflightReviveRequestMap`.
The only timeout fallback is inside `while (inflightReviveRequestMap.size() > 3)`. With one to three failed checkpoints and no later traffic, that branch is never entered. Because the revive offset has already moved past the original checkpoint, the unacknowledged message can remain unredelivered indefinitely.
This is not a security issue.
## Steps to Reproduce
A deterministic broker-free unit reproduction is sufficient:
1. Build one `PopCheckPoint` containing a single unacknowledged message.
2. Make `EscapeBridge.getMessageAsync(...)` return an exceptionally completed `CompletableFuture`.
3. Invoke `PopReviveService.mergeAndRevive(...)`.
4. Assert that the revive offset and committed offset advanced, the checkpoint remains in the in-flight map, and no replacement CK was written.
The core setup is:
```java
CompletableFuture> failed = new CompletableFuture<>();
failed.completeExceptionally(new RuntimeException("store read failed"));
when(escapeBridge.getMessageAsync(anyString(), anyLong(), anyInt(), anyString(), anyBoolean()))
.thenReturn(failed);
popReviveService.mergeAndRevive(reviveObj);
```
On the unmodified baseline, two independent runs reproduced the same state:
```text
reviveObj.newOffset = 1
committed revive offset = 1
inflightReviveRequestMap.size() = 1
messageStore.putMessage(rewritten CK) = 0 calls
```
The regression test consequently fails because one CK rewrite was expected but none occurred.
## What Did You Expect to See?
An exceptional business-message read should be treated as a retryable read failure. The service should rewrite the checkpoint for that message offset, finish/remove the in-flight record, and preserve the existing offset progression without losing the message's retry path.
## What Did You See Instead?
The original revive offset is committed, but the replacement CK is never written. At low traffic the incomplete in-flight record is not old enough to trigger any independent scan and the size-gated timeout loop is never entered, so the message can be skipped indefinitely.
## Additional Context
A minimal fix is to use `CompletableFuture.handle` on the `getBizMessage` stage. If the upstream read completes exceptionally, log it and return `(msgOffset, false)`, which feeds the existing `rePutCK` path. The handler should remain scoped to the read stage so exceptions raised later while processing/writing a retry retain their current semantics.
With that fix, the full `PopReviveServiceTest` class passes 13/13 tests. Checkstyle, SpotBugs, and `git diff --check` also pass.
Exact GitHub searches for `PopReviveService getNow`, `PopReviveService rePutCK exception`, `getBizMessage PopReviveService exception`, and `PopReviveService CompletionException` found no issue or PR match. Related [#10658](https://github.com/apache/rocketmq/issues/10658)/[#10659](https://github.com/apache/rocketmq/pull/10659) concern head-of-line blocking in the newer `PopConsumerService` popkv batch path. [#10667](https://github.com/apache/rocketmq/issues/10667) concerns discarded futures in `PopConsumerCache.cleanupRecords` when buffer merging is enabled. Neither covers the legacy/default `PopReviveService` path committing an offset without preserving retry after an exceptional read.
Contributor guide
Research direction
Start in PopReviveService.mergeAndRevive and its getBizMessage/read callback path, then run the focused PopReviveServiceTest reproduction with EscapeBridge.getMessageAsync completing exceptionally. Done means the failed read rewrites its checkpoint, completes and removes the in-flight record, and the test plus the stated checks pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend, distributed-systems
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 76/100