[ISSUE #10666][BUG] Persist commitLog truncate marker

[chenxu80]

Which Issue(s) This PR Fixes

Fixes #10666

Brief Description

Persist an invalid CommitLog header at the truncation boundary before
truncating dirty files.

The marker is forced to disk so stale CommitLog records after the consistent
offset cannot be recovered after an HA slave restarts.

This change also:

• validates that the truncation offset is a valid CommitLog message boundary;
• aborts the truncation when the marker cannot be persisted;
• keeps ReputMessageService stopped when truncation fails;
• adds regression coverage for normal and abnormal restart recovery.

How Did You Test This Change?

• bazel test //store:src/test/java/org/apache/rocketmq/store/DefaultMessageStoreTest --test_output=errors
• mvn -o -pl store -am test
• mvn -o -pl store -am -DskipTests package

[codecov-commenter]

Codecov Report

❌ Patch coverage is 58.33333% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.29%. Comparing base (b5bc1ff) to head (9091321).
⚠️ Report is 24 commits behind head on develop.

Files with missing lines	Patch %	Lines
...main/java/org/apache/rocketmq/store/CommitLog.java	63.41%	10 Missing and 5 partials ⚠️
...org/apache/rocketmq/store/DefaultMessageStore.java	28.57%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10816      +/-   ##
=============================================
+ Coverage      48.26%   48.29%   +0.02%     
- Complexity     13437    13514      +77     
=============================================
  Files           1377     1380       +3     
  Lines         100840   101184     +344     
  Branches       13035    13129      +94     
=============================================
+ Hits           48670    48864     +194     
- Misses         46223    46321      +98     
- Partials        5947     5999      +52     

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

[RockteMQ-AI]

Summary

This PR persists an invalid CommitLog header at the truncation boundary before truncating dirty files, preventing stale records from being recovered after an HA slave restart. The implementation is well-structured with proper boundary validation, file hold/release lifecycle, and comprehensive test coverage.

Findings

• [Info] CommitLog.java:920 — The validateTruncateMarkerPosition method is thorough but adds an extra MappedFile lookup. Consider whether this could be combined with the initial findMappedFileByOffset call to reduce I/O on the hot truncation path.
• [Info] DefaultMessageStoreTest.java — Good test coverage across normal recovery, abnormal recovery, first-message boundary, and non-boundary rejection scenarios.

Suggestions

The overall approach is sound. One minor consideration: the persistTruncateMarker method throws IllegalStateException for all error cases — differentiating error types (e.g., a custom TruncationException) could help callers distinguish between recoverable and unrecoverable failures, though this is optional.

LGTM.

---

Automated review by github-manager-bot

[RockteMQ-AI]

🤖 Automated Review by RockteMQ-AI

Review of the diff for apache/rocketmq#10816.

Overall: The change correctly adds a durable invalid header at the truncation boundary so that recovery stops exactly there instead of rolling forward into stale data. The failure-path handling that keeps ReputMessageService stopped is the right safety choice.

---

Findings

🟡 Warning — DefaultMessageStore.java:97 / CommitLog.java:17-50

markerOffset = Math.max(offsetToTruncate, this.getMinPhyOffset()) ensures the marker is never written before the log’s minimum offset, but the subsequent truncateDirtyLogicFiles(offsetToTruncate) and commitLog.truncateDirtyFiles(offsetToTruncate) still receive the original smaller value. If either truncation helper does not internally clamp to minPhyOffset in exactly the same way, the marker position and the actual truncation point could diverge. Verify that both helpers use the same clamping semantics.

🟡 Warning — CommitLog.java:25

The bounds check relativeOffset + 2 * Integer.BYTES > mappedFile.getFileSize() is technically correct (8 bytes), but the code then writes a long at relativeOffset. Using Long.BYTES would make the intent clearer and avoid future confusion.

🟡 Warning — CommitLog.java:62-68

Boundary validation accepts only MESSAGE_MAGIC_CODE and MESSAGE_MAGIC_CODE_V2. Confirm that every valid CommitLog entry type (single, batch, delayed messages, etc.) uses one of these two magic codes; otherwise legitimate truncation boundaries could be rejected.

🟢 Suggestion — CommitLog.java:40

Consider asserting phyOffset <= getMaxPhyOffset() before writing the marker. Writing a marker beyond the logical end of the log should not happen in normal flows, but an explicit guard would make the invariant obvious.

🟢 Suggestion — DefaultMessageStoreTest.java

Add a test that truncates exactly at a mapped-file boundary (e.g., at mappedFileSize) and restarts. The current tests truncate inside the first file or at offset 0; a boundary case would validate the interaction between the marker and MappedFileQueue truncation.

🟢 Suggestion — DefaultMessageStore.java:117-121

The catch clause handles RocksDBException | RuntimeException. This is fine, but consider documenting in a code comment why Error is intentionally not caught (fatal JVM errors should not resume message dispatch on a partially truncated store).

---

Correctness highlights

• CommitLog.java:30-49: hold() / release() and force() are used correctly, ensuring the marker is durable and the mapped file reference is safe.
• CommitLog.java:52-77: Walking the file from 0 to relativeOffset and validating physicOffset against fileFromOffset + currentPosition is a strong integrity check that prevents truncating mid-message.
• DefaultMessageStore.java:102-121: Persisting the marker before truncating consume queues and CommitLog means a crash leaves the store with a consistent recovery stop point, and keeping ReputMessageService stopped on failure prevents dispatching partially truncated data.

No critical correctness issues were found.

---

_{This is an automated review. Please verify findings before acting on them.}