Skip to content

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

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

[ISSUE #10755] Fix ConsumeQueueExt truncation cleanup#10758
ai-yang wants to merge 1 commit 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.

@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

@codecov-commenter

codecov-commenter commented Aug 15, 2026

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.54%. Comparing base (e348efa) to head (00e1c97).

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.58%   48.54%   -0.04%     
- Complexity     13676    13677       +1     
=============================================
  Files           1381     1381              
  Lines         101475   101528      +53     
  Branches       13190    13202      +12     
=============================================
- Hits           49299    49285      -14     
- Misses         46174    46224      +50     
- Partials        6002     6019      +17     

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

Re-reviewed after new commits — ConsumeQueueExt truncation now handles the empty main CQ edge case and refactors truncation into a shared method.

Findings

  • [Info] recover() now correctly calls consumeQueueExt.recover() and truncateConsumeQueueExt(1) when the main CQ has no mapped files — previously this path was skipped entirely, leaving ext files in an inconsistent state.
  • [Info] truncateConsumeQueueExt(long) extracted from inline logic, reused by both recover() and truncateDirtyLogicFiles().
  • [Info] truncateDirtyLogicFiles refactored: shouldDeleteFile scoped to the inner loop, cqFileDeletionFailed tracks outer state — clearer intent.
  • [Info] Test covers the empty CQ + ext recovery scenario.

LGTM.


Automated review by github-manager

Signed-off-by: Rui <1685901819@qq.com>
@ai-yang
ai-yang force-pushed the agent/fix-cqext-truncate branch from 82bc882 to 00e1c97 Compare August 27, 2026 15:31
@ai-yang

ai-yang commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Update after refreshing this PR against the latest develop:

  • rebased onto e348efa66b08eb645ee123706ea6492fa9a3ad35 (RocketMQ 5.5.1);
  • squashed the two PR commits into one signed-off commit (00e1c9767);
  • reran the complete ConsumeQueueTest: 21/21 passed;
  • Checkstyle, SpotBugs, and git diff --check passed in the affected reactor build.

The full CI matrix has been retriggered by the force-push.

@RongtongJin @guyinyou, could one of you please take a human review when available, particularly for the CQ/CQExt truncation and recovery invariants? Thank you.

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