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

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;
  • partial mapped-file deletion enters a pending state that blocks address reuse until cleanup is retried successfully;
  • CQ physical-delete failures and deleteFile=false do not mutate CQExt independently;
  • recovery of an empty CQ cleans up orphaned Ext files left by an interrupted truncation.

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

How Did You Test This Change?

  • Unmodified develop: the deterministic truncate/reload regression failed in 5/5 isolated JDK 8 Maven processes.
  • Fixed targeted suite: 20 isolated Maven/JVM processes at 9/9 each (180/180 total), covering partial truncation, reload, full clear, failed deletion/retry, address reuse, minLogicOffset, and recovery.
  • Full store -am test: common 241/241, remoting 174/174, and store 322 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
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. 👍

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

2 participants