[ISSUE #9583] Re-register receipt handle after changing invisible duration#10586
[ISSUE #9583] Re-register receipt handle after changing invisible duration#10586Aias00 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a proxy-side receipt-handle lifecycle bug where changeInvisibleDuration could return a new receipt handle from the broker but fail to re-register it with the proxy’s ReceiptHandleManager when the original handle was under auto-renew management, leading to later ACK lookups failing.
Changes:
- Re-register managed
MessageReceiptHandleinstances afterchangeInvisibleTimesucceeds by updating them with the broker-returned receipt handle. - Preserve existing behavior for non-managed receipt handles (no re-registration).
- Add targeted unit tests covering both the managed and non-managed paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ChangeInvisibleDurationActivity.java | Updates response conversion flow to update/re-add managed receipt handles using the new handle returned by the broker. |
| proxy/src/test/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ChangeInvisibleDurationActivityTest.java | Adds tests asserting re-registration happens only when the old handle was managed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Proxy auto-renew removes the old receipt handle before changing invisible duration. When the broker returns a new receipt handle, the proxy must put that managed handle back; otherwise later ACKs cannot find the updated handle in the receipt handle manager. Constraint: Related to apache#9583 Rejected: Recreate MessageReceiptHandle from only the new receipt handle | loses existing managed metadata such as message context and retry state Confidence: high Scope-risk: narrow Directive: Re-register only handles that were already managed by the receipt handle manager, and reuse the channel captured before the async broker callback. Tested: mvn -pl proxy -Dtest=ChangeInvisibleDurationActivityTest test Tested: mvn -pl proxy -Dtest=ReceiptHandleProcessorTest test Not-tested: Full repository test suite
Aias00
force-pushed
the
fix/change-invisible-reregister-handle
branch
from
82c768e to
ecc43e9
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10586 +/- ##
=============================================
- Coverage 48.26% 48.16% -0.10%
+ Complexity 13433 13411 -22
=============================================
Files 1378 1378
Lines 100817 100835 +18
Branches 13040 13042 +2
=============================================
- Hits 48660 48568 -92
- Misses 46211 46303 +92
- Partials 5946 5964 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
okay, by the way, |
…eption ChangeInvisibleDurationActivity removes the managed handle before the async broker call and re-registers it only on AckStatus.OK. When the broker returns a non-OK status or the future completes exceptionally, the old handle was dropped from the receipt handle manager, so later renew/ack could no longer find it. Re-add the unmodified old handle on both the non-OK path (in convertToChangeInvisibleDurationResponse) and the exception path (via whenComplete), mirroring the OK-path re-registration. Handles that were not managed to begin with (removeReceiptHandle returned null) are left untouched. Co-Authored-By: Claude <noreply@anthropic.com>
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Correctness ✅
The fix correctly handles all three completion paths:
- Success: updates the managed handle with the new receipt handle string from the broker response, then re-registers it.
- Non-OK broker response: restores the original handle unchanged so later operations can retry.
- Exception/failure (
.whenComplete): restores the original handle so the managed lifecycle remains consistent.
Extracting channel to a local variable before the async call is correct — it captures the effectively-final reference needed in the callback closure.
Performance ✅
Minimal overhead. The channel extraction avoids a redundant grpcChannelManager.getChannel() call. The restoreReceiptHandle method is only invoked on error paths.
Tests ✅
Five new test methods covering:
- Success path with handle re-registration
- Non-OK broker response preserving old handle
- Exception path restoring old handle
- Null
messageReceiptHandle(no-op path) - Null
ackResult.getReceiptHandle()(preserves old handle)
Good coverage of edge cases.
Compatibility ✅
No API or protocol changes. The fix is internal to the proxy receipt handle management.
Verdict
Approved. Clean fix with proper error handling and thorough tests.
Note: @lizhimins previously requested changes with "wait for discuss" — flagging for maintainer awareness that this PR has both a changes-requested and this review.
Which Issue(s) This PR Fixes
Fixes #9583
Brief Description
When proxy auto-renew is managing a receipt handle,
changeInvisibleDurationremoves the old handle from the receipt handle manager before callingchangeInvisibleTime. The broker returns a new receipt handle on success, but the proxy previously only returned it to the client and did not put the managed handle back intoReceiptHandleManager.This leaves the updated handle unmanaged, so later ACK requests can fail to find the receipt handle mapping.
This PR updates the removed
MessageReceiptHandlewith the new receipt handle returned by the broker and re-registers it only when the old handle was already managed. Non-managed handles keep the existing behavior.How Did You Test This Change?
mvn -pl proxy -Dtest=ChangeInvisibleDurationActivityTest testResult:
BUILD SUCCESS,Tests run: 7, Failures: 0, Errors: 0, Skipped: 0