[ISSUE #11420] fix(store): report a failed retry append instead of reporting PUT_OK

[iloveeyjafjalla]

Which Issue(s) This PR Fixes

• Fixes [Bug] CommitLog returns PUT_OK for a message that could not be appended, so a failed send is acknowledged as success #11420

Brief Description

When the append hits the end of a commit log file, CommitLog retries on a fresh file but only uses the retry result to decide whether to call onCommitLogAppend; a failing retry fell through to the unconditional success:

result = mappedFile.appendMessage(msg, this.appendMessageCallback, putMessageContext);
if (AppendMessageStatus.PUT_OK.equals(result.getStatus())) {
    onCommitLogAppend(msg, result, mappedFile);
}
break;
...
PutMessageResult putMessageResult = new PutMessageResult(PutMessageStatus.PUT_OK, result);

A message larger than mappedFileSizeCommitLog returns END_OF_FILE from the first append and from the retry (a fresh file has no more room than the file size), so the store answered PUT_OK for a message that was never appended — and since increaseOffset is guarded by PUT_OK as well, no queue offset was advanced either. SendMessageProcessor turns that status into ResponseCode.SUCCESS and returns a msgId, i.e. the producer receives a successful send for a dropped message.

This change maps a failed retry the same way the first-append branch already maps its failures (UNKNOWN_ERROR → UNKNOWN_ERROR, everything else → MESSAGE_ILLEGAL) and returns it, in both asyncPutMessage and asyncPutMessages (the batch path had the identical shape).

How Did You Test This Change?

Two regression tests, each with a store configured like this repository's own small-file tests (mappedFileSizeCommitLog = 1024 * 8) and a payload larger than one commit log file but far below maxMessageSize:

• DefaultMessageStoreTest#testPutMessageLargerThanCommitLogFileIsNotReportedAsSuccess (single message, 16 KB body)
• BatchPutMessageTest#testPutBatchLargerThanCommitLogFileIsNotReportedAsSuccess (batch of four 8 KB messages)

Both assert that the append status is END_OF_FILE and that the put status is not PUT_OK.

Results on Temurin JDK 8 (the JDK CI uses), base a7edeecfb plus this patch:

• before the fix, both tests fail with java.lang.AssertionError: Values should be different. Actual: PUT_OK
• after the fix: mvn -B -ntp -pl store test -Dtest='DefaultMessageStoreTest,AppendCallbackTest,BatchConsumeQueueTest,BatchPutMessageTest' — Tests run: 50, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS (the four classes that exercise the put path, including the 38 DefaultMessageStoreTest cases)
• mvn -B -ntp -pl store clean package (checkstyle + spotbugs enabled): You have 0 Checkstyle violations., BugInstance size is 0

Disclosure — the full store suite does not complete on this machine, for three reasons I verified are independent of the patch:

• HAServerTest#putRequest_MultipleAckAndRequests (expected: PUT_OK but was: FLUSH_SLAVE_TIMEOUT): time/load sensitive; the same failure reproduces at the base commit in a separate worktree.
• BatchConsumeMessageTest (java.net.BindException: Address already in use: bind from DefaultHAService$AcceptSocketService.beginAccept): also reproduces at the base commit, and no other process on the machine listens in the 30xxx range the test allocates from.
• After those two are excluded, the run ends in a JVM crash inside the native RocksDB library (EXCEPTION_ACCESS_VIOLATION in librocksdbjni*.dll, thread Time-limited test) around the 45th test class in the same JVM; every class involved passes when run in isolation. The change touches only the commit log put path and no RocksDB code.

(skipAfterFailureCount=1 is configured for the module, so any of these failures would otherwise hide the rest of the suite.)

This change was prepared with AI assistance (analysis, patch and test drafting); the diff and every result above were reproduced locally by running the module's tests against the base commit and the patched tree.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[RockteMQ-AI]

Review by github-manager-bot

Summary

This PR fixes a silent data-loss bug in CommitLog: when a message append hits end-of-file and the retry on a fresh mapped file also fails (e.g. message body exceeds mappedFileSizeCommitLog), the code fell through to return PUT_OK — causing the producer to receive SUCCESS for a message that was never persisted.

Findings

• [Positive] CommitLog.java:1130-1134 — The else branch correctly mirrors the existing first-append failure mapping (UNKNOWN_ERROR → UNKNOWN_ERROR, everything else → MESSAGE_ILLEGAL). Clean and consistent with the surrounding pattern.

• [Positive] CommitLog.java:1292-1297 — The batch path (asyncPutMessages) originally had no check at all after the retry append — it just broke unconditionally. Adding the guard here closes the same class of bug for batch sends. Well caught.

• [Positive] Both regression tests use a small mappedFileSizeCommitLog (8 KB) with payloads that exceed it, directly reproducing the failure mode. The assertions (END_OF_FILE status, not PUT_OK) are precise.

• [Info] CommitLog.java:1132 — The CompletableFuture.completedFuture(...) return is correct for the error path since no async I/O is needed. Consider whether CompletableFuture.failedFuture() would be more semantically appropriate for callers that use .whenComplete() error handlers — though the current approach is consistent with how the rest of this method returns error results.

Suggestions

• The PR description is excellent — thorough disclosure of test limitations and environment details. This sets a good example for the project.
• No blocking concerns. The fix is minimal, correct, and well-tested.

---

Automated review by github-manager-bot