Skip to content

Handle exceptional POP revive reads without losing retry - #10986

Open
ai-yang wants to merge 1 commit into
apache:developfrom
ai-yang:fix-pop-revive-exceptional-read
Open

Handle exceptional POP revive reads without losing retry#10986
ai-yang wants to merge 1 commit into
apache:developfrom
ai-yang:fix-pop-revive-exceptional-read

Conversation

@ai-yang

@ai-yang ai-yang commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Brief Description

PopReviveService records a checkpoint as in flight, schedules asynchronous business-message reads, and then advances the revive offset. If one read completes exceptionally, allOf(...).whenComplete(...) runs but future.getNow(...) throws CompletionException. 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:

  • handles exceptional completion of the upstream getBizMessage future;
  • logs the failed topic, queue, offset, and broker context;
  • converts the failed read to (msgOffset, false), allowing the existing rePutCK path to retain retryability;
  • deliberately does not catch exceptions raised later by reviveRetry, keeping the change scoped to asynchronous reads;
  • adds a regression asserting that the original offset is committed, a replacement CK is written, and the in-flight entry is removed.

There is no protocol, storage-format, or public API change.

How Did You Test This Change?

The new deterministic test makes EscapeBridge.getMessageAsync return an exceptionally completed future. On unmodified develop at e348efa66, two independent runs reproduced the same failure:

reviveObj.newOffset = 1
committed revive offset = 1
inflightReviveRequestMap.size() = 1
expected messageStore.putMessage(rewritten CK): 1
actual: 0

With this change, the complete PopReviveServiceTest class passes:

Tests run: 13, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

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 test

The broker reactor verification also passes with Checkstyle and SpotBugs enabled:

Checkstyle: 0 violations
SpotBugs: 0 bug instances, 0 errors
BUILD SUCCESS

git diff --check passes as well.

Signed-off-by: Rui <1685901819@qq.com>

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.47%. Comparing base (e348efa) to head (12c30c5).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] POP revive can lose retry after an asynchronous message-read failure

3 participants