[I] [Bug] Partial batch ACKs are not scheduled for persistence when individual-delete ranges are empty [pulsar]

["void-ptr974 (via GitHub)" <gi...@apache.org>]

void-ptr974 opened a new issue, #26498:
URL: https://github.com/apache/pulsar/issues/26498

   ## Search before reporting
   
   - [x] Searched existing issues and PRs; related but distinct reports are linked below.
   
   ## User environment / validation scope
   
   - Reproduced on `master` at `ce6079887474043f4055ee6c22d2c72e456ac40f`.
   - Local `managed-ledger` unit tests using `MockedBookKeeperTestCase` (mock BookKeeper and in-memory MetadataStore). This is a locally reproduced code-level finding, not a production incident report.
   - Batch-index acknowledgment enabled; mark-delete throttling disabled (`setThrottleMarkDelete(0)`). The batch-index persistence cap is not reached.
   - Released branches and broker/client crash-and-redelivery scenarios have not been tested.
   
   ## Issue Description
   
   When an individual acknowledgment changes only a batch's ACK bitmap, `ManagedCursorImpl.asyncDelete` can report success without scheduling cursor persistence or marking the cursor dirty. This happens when `individualDeletedMessages` remains empty, even though `batchDeletedIndexes` has changed.
   
   Calling `flush()` afterwards does not checkpoint that update. The partial ACK remains in memory until some other operation causes persistence.
   
   ### Expected behavior
   
   With batch-index acknowledgment enabled and within the configured persistence limits, a changed batch ACK bitmap should participate in the cursor's normal persistence/flush mechanism even if the mark-delete position does not advance.
   
   Whether a successful ACK callback must also wait for a durable checkpoint is a separate contract question. This report focuses on the missing persistence scheduling, not an exactly-once guarantee from ACK receipts.
   
   ### Actual behavior
   
   1. `cursor.delete(...)` completes successfully.
   2. `getBatchPositionAckSet(...)` confirms the bitmap changed in memory.
   3. `getTotalNonContiguousDeletedMessagesRange()` remains 0.
   4. An explicit `cursor.flush()` is a no-op; the latest stored `PositionInfo` still contains no batch-index record.
   5. Control: an additional `cursor.markDelete(...)` at the **same mark-delete position** writes the bitmap successfully.
   
   If the process is lost before another checkpoint, the new partial-ACK state is absent from storage and cannot be recovered from that storage. This consequence follows from the persistence result; a broker crash was not simulated. A graceful close or subsequent ACK may persist the state and mask the gap.
   
   ## Code path
   
   - [The partial-ACK branch updates the bitmap, then the empty individual-range branch returns early](https://github.com/apache/pulsar/blob/ce6079887474043f4055ee6c22d2c72e456ac40f/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L2632-L2653).
   - [The early-return path invokes deleteComplete in finally](https://github.com/apache/pulsar/blob/ce6079887474043f4055ee6c22d2c72e456ac40f/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L2694-L2708). It exits before the rate-limiter and internal mark-delete paths, without setting the dirty flag.
   - [flush returns immediately when isDirty is false](https://github.com/apache/pulsar/blob/ce6079887474043f4055ee6c22d2c72e456ac40f/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L4077-L4084).
   
   ## Reproducing the issue
   
   The test below first establishes a healthy cursor ledger and leaves the target batch after the mark-delete position. It reads the serialized cursor-ledger record directly, so no recovery path is involved. The raw entry payloads are sufficient for this managed-cursor-level probe; `setInactive()` avoids unrelated message-metadata parsing in the entry cache.
   
   Save the source below as:
   
   `managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorPartialBatchAckPersistenceReproTest.java`
   
   ```bash
   ./gradlew :managed-ledger:test \
     --tests '*ManagedCursorPartialBatchAckPersistenceReproTest' \
     -PtestRetryCount=0 -PtestFailFast=false --max-workers=2 --console=plain
   ```
   
   Results on the commit above:
   
   | Scenario | Result |
   | --- | --- |
   | Partial batch ACK followed by `flush()` only | Fails: persisted record count remains 0 instead of 1 for the 3-second observation window |
   | Same setup plus an explicit mark-delete at the existing position | Passes: persisted record count is 1 |
   
   These are intentionally failing behavior assertions plus a passing control, not a proposed production fix.
   
   <details>
   <summary>Runnable TestNG reproducer, including the control</summary>
   
   ```java
   /*
    * Licensed to the Apache Software Foundation (ASF) under one
    * or more contributor license agreements.  See the NOTICE file
    * distributed with this work for additional information
    * regarding copyright ownership.  The ASF licenses this file
    * to you under the Apache License, Version 2.0 (the
    * "License"); you may not use this file except in compliance
    * with the License.  You may obtain a copy of the License at
    *
    *   http://www.apache.org/licenses/LICENSE-2.0
    *
    * Unless required by applicable law or agreed to in writing,
    * software distributed under the License is distributed on an
    * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    * KIND, either express or implied.  See the License for the
    * specific language governing permissions and limitations
    * under the License.
    */
   package org.apache.bookkeeper.mledger.impl;
   
   import static org.assertj.core.api.Assertions.assertThat;
   import static org.awaitility.Awaitility.await;
   import java.time.Duration;
   import java.util.concurrent.TimeUnit;
   import lombok.Cleanup;
   import org.apache.bookkeeper.client.LedgerHandle;
   import org.apache.bookkeeper.client.api.LedgerEntries;
   import org.apache.bookkeeper.mledger.ManagedLedgerConfig;
   import org.apache.bookkeeper.mledger.Position;
   import org.apache.bookkeeper.mledger.proto.PositionInfo;
   import org.apache.bookkeeper.test.MockedBookKeeperTestCase;
   import org.testng.annotations.DataProvider;
   import org.testng.annotations.Test;
   
   @Test(timeOut = 30000)
   public class ManagedCursorPartialBatchAckPersistenceReproTest extends MockedBookKeeperTestCase {
   
       @DataProvider
       public static Object[][] explicitCheckpoint() {
           return new Object[][] {{false}, {true}};
       }
   
       @Test(dataProvider = "explicitCheckpoint")
       public void testPartialBatchAckPersistence(boolean explicitCheckpoint) throws Exception {
           ManagedLedgerConfig config = new ManagedLedgerConfig().setThrottleMarkDelete(0);
           config.setDeletionAtBatchIndexLevelEnabled(true);
           @Cleanup
           ManagedLedgerImpl ledger = (ManagedLedgerImpl) factory.open(
                   "public/default/persistent/partial-batch-audit-" + explicitCheckpoint, config);
           ManagedCursorImpl cursor = (ManagedCursorImpl) ledger.openCursor("sub");
           cursor.setInactive();
           Position first = ledger.addEntry(new byte[] {0});
           Position batch = ledger.addEntry(new byte[] {1});
           ledger.addEntry(new byte[] {2});
           cursor.markDelete(first);
           long cursorLedgerId = cursor.getCursorLedger();
           assertThat(cursorLedgerId).isGreaterThanOrEqualTo(0);
           LedgerHandle cursorLedger = bkc.getLedgerMap().get(cursorLedgerId);
           assertThat(lastPositionInfo(cursorLedger).getBatchedEntryDeletionIndexInfosCount()).isZero();
   
           // The synchronous API returns only after deleteComplete. No rate limiter is installed.
           cursor.delete(AckSetStateUtil.createPositionWithAckSet(
                   batch.getLedgerId(), batch.getEntryId(), new long[] {2L}));
           assertThat(cursor.getBatchPositionAckSet(batch)).containsExactly(2L);
           assertThat(cursor.getTotalNonContiguousDeletedMessagesRange()).isZero();
           cursor.flush();
           if (explicitCheckpoint) {
               // Positive control: a separate mark-delete of the same position persists the bitmap.
               cursor.markDelete(first);
           }
   
           // Read the persisted record directly, without executing any cursor recovery path.
           await().atMost(Duration.ofSeconds(3)).untilAsserted(() ->
                   assertThat(lastPositionInfo(cursorLedger).getBatchedEntryDeletionIndexInfosCount())
                   .as("a successful partial batch ACK must at least be scheduled for cursor persistence")
                   .isEqualTo(1));
       }
   
       private static PositionInfo lastPositionInfo(LedgerHandle cursorLedger) throws Exception {
           long entryId = cursorLedger.getLastAddConfirmed();
           assertThat(entryId).isGreaterThanOrEqualTo(0);
           try (LedgerEntries entries = cursorLedger.readAsync(entryId, entryId).get(5, TimeUnit.SECONDS)) {
               PositionInfo info = new PositionInfo();
               info.parseFrom(entries.getEntry(entryId).getEntryBytes());
               return info;
           }
       }
   }
   ```
   
   </details>
   
   ## Error messages
   
   No delete failure is delivered in this scenario. The reproducer fails with:
   
   ```text
   [a successful partial batch ACK must at least be scheduled for cursor persistence]
   expected: 1
    but was: 0 within 3 seconds.
   ```
   
   ## Related issues and scope
   
   - #26475 / #26474: the bitmap was already persisted but is omitted during MetadataStore recovery. This report observes a missing write on a healthy cursor ledger, before any recovery.
   - #26484: the mark-delete rate limiter reports success before persistence. This report reproduces with the limiter disabled and without the dirty flag that would otherwise allow a later flush.
   
   A focused fix should ensure batch-only changes reach persistence scheduling. Callback durability semantics, throttled operation, and recovery can be discussed separately without making this fix depend on all related cursor work.
   


-- 
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: commits-unsubscribe@pulsar.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [Bug] Partial batch ACKs are not scheduled for persistence when individual-delete ranges are empty [pulsar]

["codelipenghui (via GitHub)" <gi...@apache.org>]

codelipenghui closed issue #26498: [Bug] Partial batch ACKs are not scheduled for persistence when individual-delete ranges are empty
URL: https://github.com/apache/pulsar/issues/26498


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org