Skip to content

[ISSUE #10755] Fix ConsumeQueueExt truncation cleanup - #10758

Open
ai-yang wants to merge 2 commits into
apache:developfrom
ai-yang:agent/fix-cqext-truncate
Open

[ISSUE #10755] Fix ConsumeQueueExt truncation cleanup#10758
ai-yang wants to merge 2 commits into
apache:developfrom
ai-yang:agent/fix-cqext-truncate

Conversation

@ai-yang

@ai-yang ai-yang commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Fixes #10755

Brief Description

ConsumeQueue.truncateDirtyLogicFiles() truncated the main consume queue but did not trim ConsumeQueueExt to the last retained extension address. Ext units from the discarded CQ tail therefore remained readable, and later appends plus reload/recovery could preserve those orphaned units permanently.

This change aligns CQExt truncation with the successfully retained CQ state:

  • normal truncation exits locate the last readable retained Ext reference after minLogicOffset and truncate to it;
  • when no Ext reference is retained, all Ext mapped files are removed and the address cursor is reset only after deletion succeeds;
  • CQ physical-delete failures and deleteFile=false do not mutate CQExt independently;
  • recovery handles both an empty CQ and a non-empty CQ whose recent recovery window contains only raw tags, while preserving valid older Ext references.

Of the 753 added lines in this PR, 629 are deterministic regression tests; the production change is limited to ConsumeQueue and ConsumeQueueExt.

Cleanup State and Recovery

  • Successful full cleanup resets the flush/commit cursors and clears the pending state before Ext addresses can be reused.
  • If a mapped file is still held, cleanup remains pending and Ext writes fall back to the original raw tags code. A later Ext write retries cleanup at most once per second, avoiding both unsafe address reuse and per-message warning logs.
  • On restart, recovery first validates the recent maximum Ext address. If it is absent or invalid, it scans all retained CQ entries backward; it either truncates after the last valid reference or completes full cleanup when no reference remains.

How Did You Test This Change?

  • Unmodified develop: the deterministic truncate/reload regression failed in 5/5 isolated JDK 8 Maven processes.
  • Deterministic truncation suite: 20 isolated Maven/JVM processes at 9/9 each (180/180 total).
  • Latest-head ConsumeQueueTest: 21/21, including pending cleanup after raw-tags fallback, automatic retry after mapped-buffer release, and preservation of an Ext reference before the three-file recovery window.
  • Latest-head targeted follow-up rerun: 3/3.
  • Latest-head full store -am test: common 241/241, remoting 174/174, and store 324 tests with 4 skips, 0 failures, and 0 errors.
  • Maven validate and Checkstyle: 0 violations.
  • SpotBugs: 0 bugs/errors across all four reactor modules.
  • git diff --check: passed.

Signed-off-by: Rui <1685901819@qq.com>
@ai-yang
ai-yang marked this pull request as ready for review August 2, 2026 14:23

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by github-manager-bot

Summary

Fixes ConsumeQueueExt data orphaning during truncation and recovery by refactoring truncateDirtyLogicFiles, adding truncateAll() to ConsumeQueueExt, and handling the empty-mapped-files recovery path.

Findings

  • [Warning] ConsumeQueueExt.java:232 — The truncateAllPending volatile flag is set to true during truncateAll() but is never reset to false. After recovery truncation completes, all subsequent put() calls will be silently skipped with a warning log. If the ConsumeQueue is expected to accept new ext writes after recovery, this flag should be reset once truncation finishes.
  • [Info] ConsumeQueue.java:448-492 — The refactored truncation loop is significantly cleaner. The hasRetainedExt / cqFileDeletionFailed tracking properly handles edge cases where file deletion fails mid-truncation.
  • [Info] ConsumeQueue.java:136-147 — Good catch on the empty mapped files recovery path — previously this case would skip ext cleanup entirely.
  • [Info] findLastRetainedExtAddress() — The backward scan is correct but could be expensive for large queues. This is only called during truncation so the impact should be limited.

Suggestions

  • Important: Verify whether truncateAllPending should be reset after truncateAll() completes. If the ConsumeQueue continues to serve writes post-recovery, the current behavior would silently drop all ext data.
  • Consider logging at INFO level (not WARN) when truncateAllPending blocks a write during active truncation, since this is expected transient behavior.

Automated review by github-manager-bot

@ai-yang

ai-yang commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Clarification on the automated review warning: truncateAllPending is reset to false after all Ext mapped files are deleted and the flush/commit positions are reset in ConsumeQueueExt.truncateAll(). It intentionally remains true only when deletion is incomplete, preventing address reuse until a later retry succeeds. The held-buffer regression covers block → release/retry → address reuse → reload.

@RockteMQ-AI

Copy link
Copy Markdown
Contributor

Thanks for the clarification, @ai-yang. You're right — I can see that truncateAllPending is correctly reset to false at the end of the successful path in truncateAll() (after setFlushedWhere(0) and setCommittedWhere(0)). The early return on the failure path (files not fully deleted) intentionally keeps it true to gate new put() calls until a retry succeeds. My original warning was incorrect on this point; the design is sound. 👍

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Defensive fix with proper validation and test coverage. LGTM.


Automated review by github-manager-bot

Signed-off-by: Rui <1685901819@qq.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.56627% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.55%. Comparing base (a06836d) to head (82bc882).
⚠️ Report is 13 commits behind head on develop.

Files with missing lines Patch % Lines
...n/java/org/apache/rocketmq/store/ConsumeQueue.java 92.59% 0 Missing and 4 partials ⚠️
...ava/org/apache/rocketmq/store/ConsumeQueueExt.java 89.65% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10758      +/-   ##
=============================================
+ Coverage      48.29%   48.55%   +0.26%     
- Complexity     13502    13684     +182     
=============================================
  Files           1380     1381       +1     
  Lines         101091   101517     +426     
  Branches       13101    13199      +98     
=============================================
+ Hits           48824    49294     +470     
+ Misses         46305    46208      -97     
- Partials        5962     6015      +53     

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] ConsumeQueue truncation leaves orphaned ConsumeQueueExt entries

3 participants