Skip to content

[fix][schema] Fix BookKeeper read handle leaks on schema read failures - #26818

Open
void-ptr974 wants to merge 2 commits into
apache:masterfrom
void-ptr974:fix/schema-read-handle-leak
Open

void-ptr974 wants to merge 2 commits into
apache:masterfrom
void-ptr974:fix/schema-read-handle-leak

Conversation

@void-ptr974

@void-ptr974 void-ptr974 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Main Issue: #26815

Motivation

Schema storage closes an opened BookKeeper read handle only when the entry read succeeds. A failed read bypasses cleanup and can leave the handle and its metadata listener retained.

Modifications

  • Close an opened handle after the read/decode attempt, including asynchronous failures and synchronous exceptions.
  • Wait for the close callback before completing the operation.
  • Preserve the original read/decode error when cleanup also fails, attaching the close error as a suppressed exception to the underlying cause. A close failure after a successful read still fails the operation.

Failed reads remain failed; this change does not introduce retries or modify BookKeeper watch registration.

Verifying this change

  • Make sure that the change passes the CI checks.

Local validation passed as described below. Full CI validation is still pending.

BookkeeperSchemaStorageReadHandleTest covers nine outcomes through the public storage read path. Tests delay read/close callbacks to check ordering, use malformed serialized bytes for decoding failures, assert exactly one close after a successful open, and verify both the primary and suppressed errors. The failed-open control verifies that no read or close is attempted.

With the tests unchanged and only the production changes removed (base 1046481c970067cbca9c80a25a98257b9179a3da), four cases fail: three read-failure paths never invoke close, and a combined decode/close failure loses the decode error. The other five are compatibility controls that pass on both versions.

The two combined-failure cases also fail against the initial PR implementation: the close error was attached to an asynchronous wrapper and was lost when the caller unwrapped the original error. The updated implementation preserves it on the underlying cause.

With the fix restored, all nine cases and 19 related existing tests pass (28 total), along with quickCheck:

./gradlew :pulsar-broker:test \
  --tests '*BookkeeperSchemaStorageReadHandleTest' --tests '*BookkeeperSchemaStorageTest' \
  --tests '*SchemaServiceTest' \
  -PtestRetryCount=0 -PtestFailFast=false -PtestMaxParallelForks=1 --max-workers=2 quickCheck

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

Always close an opened schema read handle after the read attempt, retaining the original read failure if cleanup also fails.

Main Issue: apache#26815

Assisted-by: OpenAI Codex
@void-ptr974 void-ptr974 changed the title [fix][schema] Close read handles when schema reads fail [fix][schema] Fix BookKeeper read handle leaks on schema read failures Oct 2, 2026
Unwrap asynchronous errors before attaching suppressed close failures. Exercise delayed read and close callbacks, malformed schema entries, failed opens, and synchronous exceptions.

Assisted-by: OpenAI Codex
@void-ptr974
void-ptr974 marked this pull request as ready for review October 2, 2026 15:58

@Denovo1998 Denovo1998 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.

Nice fix! The read handle is now closed on both asynchronous and synchronous failures, and waiting for the close to complete before finishing the read looks right to me.

I also checked the exception handling. Keeping the original read/decode error as the primary failure and attaching the close error as suppressed makes sense.

I didn't spot any blocking issues in the current changes. Just need the CI checks to run successfully.

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.

2 participants