unbridled-41 opened a new pull request, #11016: URL: https://github.com/apache/rocketmq/pull/11016
### Which Issue(s) This PR Fixes - Fixes #10736 (ack case; see Scope below for the change-invisibility case) ### Problem / Evidence With the KV pop service and `enablePopBufferMerge` on, `PopConsumerCache#cleanupRecords` stages records (moves them from `recordTreeMap` to the staged `removeTreeMap`), snapshots the staged set, and persists the snapshot via `PopConsumerKVStore#writeRecords`. While that write is in flight, an ack for the same record: 1. finds the record in neither map (`ConsumerRecords#delete` only checks `recordTreeMap`), so `deleteRecords` returns it as remaining; 2. `PopConsumerService#ackAsync` therefore deletes the durable record and returns success; 3. the blocked cleanup write then persists its stale snapshot, restoring the acknowledged checkpoint to the durable store. A later revive scan observes the restored checkpoint and the already-acknowledged message is delivered again. This is a deterministic race: the regression test below blocks the cleanup write on a latch, acks the staged record, releases the write, and asserts the acked record is absent from the durable store. It fails reproducibly on unmodified develop. ### Root cause / Fix - The ack path could not see (or cancel) records staged for the cleanup write, and cleanup never verified ownership after the write became durable. - `PopConsumerCache#ackRecords` (new, used by `PopConsumerService#ackAsync`) additionally removes the record from the staged set, and `cleanupRecords` re-deletes any written record that left the staged set while its write was in flight (`deleteAckedStagedRecords`). Every interleaving (ack before/during/after the write, store-delete before/after the re-persist) now converges to the ack being effective; both deletes are idempotent. ### Scope This fixes the **ack** case demonstrated in the issue. The same stale-snapshot window exists for change-invisibility (`ChangeInvisibilityDuration`): a same-key (same visibilityTimeout) rewrite can still be clobbered by an in-flight cleanup write. Deleting the staged record there is unsafe without knowing the replacement checkpoint's value (it could erase the new checkpoint and lose the message), so that path deliberately keeps using `deleteRecords` and needs its own design, as the issue author and PR #10519 also noted. ### Priority PRIORITY = 73: impact 32 (acknowledged messages redelivered - user-visible correctness) + scope 12 (broker pop KV service path) + reproducibility 17 (deterministic latch-based test) + maintenance 12 (contained, convergent fix in one class). FIX_CONFIDENCE = 82: convergent under all interleavings analyzed; the change-invisibility variant is explicitly left for the design discussion the issue asks for. ### How Did You Test This Change? - `PopConsumerCacheTest#testAckWinsOverCleanupStoreWrite`: latch-controlled repro (blocks the KV store write, acks the staged record, releases the write) asserting the durable store does not retain the acked record. Fails on unmodified develop, passes with this change. - `mvn -pl broker test -Dtest=PopConsumerCacheTest` → 4/4 pass. - `mvn -pl broker test -Dtest='PopConsumer*Test'` → 38 tests, 0 failures. - `mvn -pl broker test -Dtest='AckMessage*Test,ChangeInvisible*Test,PopMessage*Test,PopRevive*Test'` → 37 tests, 0 failures. ### Risk Low: `deleteRecords` behavior is unchanged (change-invisibility path untouched); the new staged-set removal only happens on the ack path, and the extra durable delete is idempotent and bounded by one record per staged write. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
