Repository navigation
[fix][schema] Fix BookKeeper read handle leaks on schema read failures - #26818
Open
void-ptr974 wants to merge 2 commits into
Open
void-ptr974 wants to merge 2 commits into
void-ptr974 wants to merge 2 commits into
Conversation
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
11 tasks
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
marked this pull request as ready for review
October 2, 2026 15:58
3 tasks done
Denovo1998
approved these changes
Oct 9, 2026
Denovo1998
left a comment
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Failed reads remain failed; this change does not introduce retries or modify BookKeeper watch registration.
Verifying this change
Local validation passed as described below. Full CI validation is still pending.
BookkeeperSchemaStorageReadHandleTestcovers 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:Does this pull request potentially affect one of the following parts: