[ISSUE #10985] Handle exceptional POP revive reads without losing retry

[ai-yang]

Which Issue(s) This PR Fixes

• Fixes [Bug] POP revive can lose retry after an asynchronous message-read failure #10985

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 regressions asserting that the original offset is committed, a replacement CK is submitted to the store, and the in-flight entry is removed, for both immediately failed and later-failing reads.

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

Applicability and limits

This repairs exceptional completion of the POP revive business-message read future. It is not a claim that every missing message or ordinary remote timeout reaches this branch: some upstream failures are already normalized into the existing retry result.

The new tests exercise PopReviveService -> EscapeBridge -> MessageStore.getMessageAsync, using a real EscapeBridge and injecting the fault at the mocked store-future boundary. They validate exceptional propagation and the existing checkpoint-rewrite/cleanup path, not physical disk/cloud/network failure, durable CK persistence, or exactly-once delivery. Failures of the replacement CK write itself are outside this patch.

How Did You Test This Change?

Latest verification (2026-09-13): head a191a2953750e946c2ffc6d8ba1d9cb547e85036, based on develop at 1a50c6e4e35524ac22055b76fc0ff2a6d3cff99f, JDK 8 / Maven 3.8.1:

git submodule update --init --depth 1 -- rocketmq-apis
mvn -pl broker -am -DskipITs -Dtest=PopReviveServiceTest \
  -Dsurefire.failIfNoSpecifiedTests=false test

The local run used offline mode and settings for the existing dependency cache, without changing project configuration or disabling Checkstyle/SpotBugs.

• PopReviveServiceTest: 14 passed, 0 failures/errors/skips.
• All 11 targeted reactor modules succeeded, including the protocol module.
• Checkstyle and SpotBugs enabled for project code: no violations or findings. Generated protocol code follows upstream's configured exclusions.
• git diff --check: passed.

The immediate-failure regression now traverses the real bridge instead of mocking its return value. The added delayed-failure case first holds the store future incomplete, confirms that revive offset 1 has been committed while one request remains in flight and no CK rewrite has occurred, then fails the future. It asserts that the in-flight entry is removed and the replacement CK is submitted exactly once. The failure timing is controlled by the future, not a sleep.

Historical red evidence: the earlier regression mocked EscapeBridge.getMessageAsync directly. On unmodified develop at e348efa66, two independent runs reproduced:

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

That red result is for the original test setup, not a fresh baseline execution of the two strengthened tests. The September 11 head passed 13 tests and all 10 GitHub checks when checked on September 13 before this test-only follow-up. CI for the new head must be evaluated independently; the production fix has not changed.

Related PR coordination

#11023 also fixes #10985 using the same handle-based retry path. At the time of comparison (2026-09-11), that PR changes only the production file and records no local Maven run. This PR additionally contains the deterministic regression for offset advancement, checkpoint rewrite, and in-flight cleanup, along with the verification above. The production behavior is equivalent apart from logging level/context. Please coordinate review around one implementation and retain the regression coverage when consolidating.

[RockteMQ-AI]

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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.28%. Comparing base (1a50c6e) to head (a191a29).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10986      +/-   ##
=============================================
- Coverage      49.39%   49.28%   -0.11%     
+ Complexity     14237    14206      -31     
=============================================
  Files           1390     1390              
  Lines         103123   103128       +5     
  Branches       13484    13484              
=============================================
- Hits           50940    50830     -110     
- Misses         46031    46120      +89     
- Partials        6152     6178      +26     

☔ 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.

[ai-yang]

CI triage update:

• The Bazel job executed 573 tests; 572 passed and the sole failure was //test:.../OffsetResetIT.
• This PR only changes PopReviveService and PopReviveServiceTest; the affected test class passes 13/13, and the Maven, coverage, license, CodeQL, Windows, and macOS jobs all passed.
• I found no direct overlap between OffsetResetIT and this focused exceptional POP-revive read path, so I am rerunning the failed Bazel job before making any code change.

@lizhimins @xdkxlk, could you please take a human review when convenient? The main review point is whether converting an exceptional business-message read to the existing rePutCK path correctly preserves POP at-least-once retry semantics without broadening exception handling around reviveRetry.

[ai-yang]

Rerun follow-up: I attempted to rerun the failed Bazel workflow through the GitHub Actions API, but GitHub rejected the author-side request (run cannot be rerun). A maintainer-triggered rerun is therefore required. I have not changed the code merely to retrigger CI.

[RockteMQ-AI]

Summary

Fixes a bug where exceptional async reads in PopReviveService would abort the callback before cleanup, potentially losing the message's retry path indefinitely.

The original code used thenApply which would throw CompletionException on failed reads, preventing the checkpoint from being rewritten and the in-flight entry from being cleared. Since timeout cleanup only runs when the map has >3 entries, a low-traffic failure could lose the retry path.

The fix uses handle to catch exceptions, log the failed topic/queue/offset/broker, and convert to (msgOffset, false), allowing the existing rePutCK path to retain retryability.

Regression test verifies: original offset committed, replacement CK written, in-flight entry removed.

LGTM 👍

---

Automated review by github-manager-bot

[ai-yang]

Refreshed onto develop at 1a50c6e4e and pushed signed-off commit e60e409ba. The base includes #11086's OffsetResetIT stabilization, addressing the test area that failed on the previous Bazel run. The POP production patch is unchanged by the rebase.

Latest JDK 8 verification: PopReviveServiceTest passed 13/13; the full 11-module targeted broker reactor succeeded with Checkstyle and SpotBugs enabled for project code. Full CI is running again on the refreshed head.

I also documented the overlap with #11023: both use the same read-stage fallback; this PR includes the regression asserting offset advancement, checkpoint rewrite, and in-flight cleanup. @zjncs, thanks for investigating the same issue; could we coordinate around one implementation and retain that test coverage?

@lizhimins @xdkxlk, could you take a human review of the retry semantics and help select the consolidation path when convenient?

[ai-yang]

Pushed test-only follow-up a191a29; the production fix is unchanged.

The exceptional-read regression now passes through a real EscapeBridge and injects failure at MessageStore.getMessageAsync. A second case holds the read pending until after revive offset 1 is committed, then completes it exceptionally. It verifies exactly one replacement CK submission and removal of the in-flight entry.

Latest local JDK 8 / Maven validation: PopReviveServiceTest 14/14 passed; all 11 targeted reactor modules succeeded with Checkstyle and SpotBugs enabled. The PR description now distinguishes store-future fault injection from physical storage failures/durable persistence, and the historical baseline reproduction from today's expanded test run.

This additional coverage is available for the existing human review and coordination with #11023. The old head's green CI does not certify this new head; its checks should be considered separately.

[RockteMQ-AI]

Summary

Re-reviewed after new commits. The change from .thenApply() to .handle() correctly catches exceptions thrown by getBizMessage() during POP revive, preventing the future from completing exceptionally and losing the retry opportunity.

Key points:

• Returning Pair<>(msgOffset, false) on failure preserves the retry semantics — the checkpoint will not advance past this message
• Error logging includes sufficient context (queueId, topic, offset, brokerName) for debugging
• Test coverage validates both the exception path and the normal path

LGTM — re-approved with the latest changes.

---

Automated review by github-manager-bot