[PR] [ISSUE #11379] Fix tiered dispatch loop NPE and buffer leak on mid-batch expiry [rocketmq]

["messere1 (via GitHub)" <gi...@apache.org>]

messere1 opened a new pull request, #11380:
URL: https://github.com/apache/rocketmq/pull/11380

   ## What
   
   `MessageStoreDispatcherImpl#doScheduleDispatch` preflights the first offset of a dispatch batch with null guards (from #8965), but the group-commit append loop that follows re-fetches the CQ unit and the message for every subsequent offset **without those guards**:
   
   - `consumeQueue.get(offset)` returns null once the consume queue has been truncated past the offset (expiry sweep), and `selectOneMessageByOffset` returns null once the commitlog data has been deleted. When that happens mid-batch, the loop throws an NPE from `cqUnit.getSize()` / passes null into `appendCommitLog`.
   - Every `SelectMappedBufferResult` fetched in the loop is owned by the `GroupCommitContext` built after the loop. On the NPE path the context is never constructed, so the buffers fetched before the failing offset leak their `MappedFile` references.
   
   There is a second, non-exceptional leak on the same stretch: when `appendCommitLog` succeeds but the first `appendConsumeQueue` fails, the loop breaks with a non-empty `appendingBufferList` and an empty `dispatchRequestList`. The context is built, but the `if (!dispatchRequestList.isEmpty())` block — the only place that hands it to `commitAsync`/`constructIndexFile`, both of which release it — never runs, so the context and its buffers are silently dropped.
   
   ## How
   
   - Mirror the preflight guards inside the loop: `break` on a missing CQ unit or a missing message, so the partial batch still commits and its context is still released through the normal path.
   - Release the group commit context directly when no consume-queue entry was appended and the context would otherwise never be consumed.
   
   ## Verification
   
   Red/green with `MessageStoreDispatcherImplTest` (tieredstore module, `mvn -pl tieredstore test -Dtest=MessageStoreDispatcherImplTest`):
   
   - New test `dispatchMidBatchCqVanishTest`: CQ unit present for the preflight offset, null for the next one. Without the source change `doScheduleDispatch` throws an NPE and the test fails; with it, the call completes normally and the one successfully appended dispatch request is still committed.
   - New test `dispatchConsumeQueueAppendFailureReleasesBuffersTest`: commitlog append succeeds, the first consume-queue append fails. Without the source change the fetched message buffer is never released (`hasReleased()` stays false); with it, the context is released directly and no commit is attempted.
   - The full `MessageStoreDispatcherImplTest` suite passes with the change.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [ISSUE #11379] Fix tiered dispatch loop NPE and buffer leak on mid-batch expiry [rocketmq]

["yx9o (via GitHub) via commits" <co...@rocketmq.apache.org>]

yx9o commented on code in PR #11380:
URL: https://github.com/apache/rocketmq/pull/11380#discussion_r4524166138


##########
tieredstore/src/main/java/org/apache/rocketmq/tieredstore/core/MessageStoreDispatcherImpl.java:
##########
@@ -325,6 +340,11 @@ public CompletableFuture<Boolean> doScheduleDispatch(FlatFileInterface flatFile,
                         }
                     }
                 );
+            } else if (!appendingBufferList.isEmpty()) {
+                // Nothing was appended to the consume queue (e.g. appendCommitLog succeeded
+                // but the first appendConsumeQueue failed), so no commit or index build will
+                // ever take this context: release the fetched message buffers here.
+                groupCommitContext.release();

Review Comment:
   Could we keep the buffer hold until the successful commitlog append is either committed or rolled back? When the first CQ append fails, FileSegment.append() has already retained the original ByteBuffer for a later commit, so releasing the mapped-file reference here lets commitlog cleanup unmap memory that the pending upload still reads.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org