Handle exceptional POP revive reads without losing retry - #10986
Conversation
Signed-off-by: Rui <1685901819@qq.com>
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR fixes a subtle bug in PopReviveService where an exceptional completion of the async getBizMessage future would abort the revive callback before rewriting the checkpoint or clearing the in-flight entry. In low-traffic scenarios, this could permanently lose the retry path for affected messages.
The fix is clean and well-scoped: switching from .thenApply() to .handle() correctly captures both normal and exceptional completions, and the Pair<>(msgOffset, false) return value leverages the existing rePutCK path to preserve retryability.
Review
Correctness — The change is correct. The .handle() approach is the right pattern for catching exceptional future completion without affecting downstream stages. The false return correctly triggers checkpoint rewrite. The scope is properly limited to the async read stage — exceptions from reviveRetry are intentionally not caught here.
Test coverage — The regression test testReviveMsgFromCk_getBizMessageExceptional_rewriteCK is thorough: it verifies all three critical invariants (offset committed, in-flight entry removed, replacement CK written). Using FieldUtils.readField() for internal state verification is consistent with existing test patterns in this class.
Performance — Negligible overhead. The throwable != null check is a single branch in the already-async path. No new allocations on the happy path.
Compatibility — No protocol, storage-format, or public API changes. Pure internal implementation fix.
LGTM. Well-written fix for a real edge case that could cause silent message retry loss.
Automated review by github-manager-bot
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10986 +/- ##
=============================================
- Coverage 48.58% 48.47% -0.11%
+ Complexity 13678 13641 -37
=============================================
Files 1381 1381
Lines 101475 101480 +5
Branches 13190 13190
=============================================
- Hits 49304 49196 -108
- Misses 46170 46258 +88
- Partials 6001 6026 +25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Which Issue(s) This PR Fixes
Brief Description
PopReviveServicerecords a checkpoint as in flight, schedules asynchronous business-message reads, and then advances the revive offset. If one read completes exceptionally,allOf(...).whenComplete(...)runs butfuture.getNow(...)throwsCompletionException. This aborts the callback before it rewrites the checkpoint or clears the in-flight entry. Since timeout cleanup only runs while the map contains more than three entries, a low-traffic failure can lose the message's retry path indefinitely.This change:
getBizMessagefuture;(msgOffset, false), allowing the existingrePutCKpath to retain retryability;reviveRetry, keeping the change scoped to asynchronous reads;There is no protocol, storage-format, or public API change.
How Did You Test This Change?
The new deterministic test makes
EscapeBridge.getMessageAsyncreturn an exceptionally completed future. On unmodifieddevelopate348efa66, two independent runs reproduced the same failure:With this change, the complete
PopReviveServiceTestclass passes:Command:
mvn -o -Dmaven.repo.local=/developer/wangrui/.m2/repository \ -pl broker -am -DskipITs \ -Dcheckstyle.skip -Dspotbugs.skip -Drat.skip \ -Dsurefire.failIfNoSpecifiedTests=false \ -Dtest=PopReviveServiceTest testThe broker reactor verification also passes with Checkstyle and SpotBugs enabled:
git diff --checkpasses as well.