Skip to content

[fix] Fix cursor deadlock when skipping non-recoverable entries during mark-delete persistence - #26570

Merged
merlimat merged 1 commit into
apache:masterfrom
void-ptr974:fix/cursor-skip-lock-order
Sep 14, 2026
Merged

merlimat merged 1 commit into
apache:masterfrom
void-ptr974:fix/cursor-skip-lock-order

Conversation

@void-ptr974

@void-ptr974 void-ptr974 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Motivation

ManagedCursorImpl uses two locks in the mark-delete path:

  • The cursor ReentrantReadWriteLock protects the mark-delete position, read position, individual deletion ranges, and batch-index acknowledgement state.
  • The pendingMarkDeleteOps monitor protects the mark-delete persistence queue and cursor metadata-ledger transitions.

Normal mark-delete persistence acquires these locks in this order:

internalAsyncMarkDelete
  -> synchronized (pendingMarkDeleteOps)
  -> internalMarkDelete
  -> persistPositionToLedger
  -> buildBatchEntryDeletionIndexInfoList
  -> cursor read lock

Before this change, skipNonRecoverableEntries used the opposite order:

skipNonRecoverableEntries
  -> cursor write lock
  -> asyncDelete
  -> internalAsyncMarkDelete
  -> synchronized (pendingMarkDeleteOps)

This creates a deadlock when normal acknowledgement persistence and automatic skipping after a non-recoverable read failure run concurrently:

T1: holds pendingMarkDeleteOps, waits for the cursor read lock
T2: holds the cursor write lock, waits for pendingMarkDeleteOps

The write lock held by T2 prevents T1 from acquiring the read lock. T1 meanwhile owns the monitor required by T2, so neither thread can make progress. The cursor mark-delete position can then stop advancing and acknowledgement operations can remain incomplete.

Modifications

  • Remove the outer cursor write lock from skipNonRecoverableEntries.
  • Pass the complete non-recoverable entry range to the iterable asyncDelete overload in one call.
  • Keep deletion filtering inside asyncDelete, which already checks internalIsMessageDeleted while holding the cursor write lock.

The resulting sequence is:

asyncDelete
  -> acquire cursor write lock
  -> filter positions and update acknowledgement state
  -> release cursor write lock
  -> internalAsyncMarkDelete
  -> synchronized (pendingMarkDeleteOps)

The cursor write lock is therefore released before the mark-delete persistence path attempts to acquire pendingMarkDeleteOps. This removes the nested cursor lock -> pendingMarkDeleteOps edge that completed the deadlock cycle. The final acknowledgement state is unchanged, while the range now results in one mark-delete persistence attempt instead of one attempt per entry.

Verifying this change

  • Make sure that the change passes the CI checks.

This change adds a regression test that holds pendingMarkDeleteOps, starts skipNonRecoverableEntries, waits until the skip thread reaches internalAsyncMarkDelete, and verifies that the cursor read lock is still available. The old implementation fails this assertion because the skip thread retains the cursor write lock while waiting for the monitor.

Verified with:

  • ./gradlew :managed-ledger:checkstyleMain :managed-ledger:checkstyleTest :managed-ledger:test --tests org.apache.bookkeeper.mledger.impl.ManagedCursorTest.testSkipNonRecoverableEntries --tests org.apache.bookkeeper.mledger.impl.ManagedCursorTest.testSkipNonRecoverableEntriesDoesNotHoldCursorLockWhilePersisting

Does this pull request potentially affect one of the following parts:

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

The threading-model impact is limited to removing nested cursor-lock acquisition from the non-recoverable-entry skip path.

@void-ptr974 void-ptr974 changed the title [fix][ml] Avoid cursor lock inversion when skipping lost entries [fix][ml] Fix cursor deadlock when skipping non-recoverable entries during mark-delete persistence Sep 13, 2026
@merlimat merlimat changed the title [fix][ml] Fix cursor deadlock when skipping non-recoverable entries during mark-delete persistence [fix] Fix cursor deadlock when skipping non-recoverable entries during mark-delete persistence Sep 13, 2026

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this lock inversion. The iterable deletion keeps acknowledgement updates under the cursor write lock and releases it before mark-delete persistence acquires pendingMarkDeleteOps; the skipped range and final acknowledgement behavior remain unchanged.

@merlimat merlimat closed this Sep 13, 2026
@merlimat merlimat reopened this Sep 13, 2026
@merlimat
merlimat merged commit f613b16 into apache:master Sep 14, 2026
49 of 50 checks passed
@lhotari lhotari added this to the 5.0.0 milestone Sep 14, 2026
lhotari pushed a commit that referenced this pull request Sep 14, 2026
…g mark-delete persistence (#26570)

(cherry picked from commit f613b16)
lhotari pushed a commit that referenced this pull request Sep 14, 2026
…g mark-delete persistence (#26570)

(cherry picked from commit f613b16)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants