Repository navigation
[fix][broker] Fix lookup permit leak when namespace policy reads fail - #26606
Conversation
Assisted-by: Codex
Compose authorization, policy checks, and metadata queries while handling business failures within their originating stages. Recycle topic existence results locally, release lookup permits once after the request completes, and log unexpected callback failures without attempting another response. Cover delayed stage completion, existing-topic results, error mappings, and cleanup observability. Validation: 46 scoped tests and quickCheck pass with retries disabled. Policy failure regressions fail on the pre-fix implementation and pass with this change. Assisted-by: Codex
|
Updated in ace4156 to address both review comments. Error and response handling now stays within each stage, lookup results are recycled locally, and permit release remains centralized. Added tests for delayed authorization/policy/lookup completion, result recycling, and error mappings across both lookup paths. Local validation passed 46 scoped test cases and |
Retain both PulsarClientException and Schema imports in ServerCnxTest. Assisted-by: Codex
lhotari
left a comment
lhotari
left a comment
There was a problem hiding this comment.
LGTM. Thanks for tracking down this permit leak and for the careful, stage-by-stage restructuring.
I traced the new chain in handlePartitionMetadataRequest: the permit is acquired once and released exactly once, in the terminal whenComplete, on every completion path (success, unauthorized, authorization failure, policy read failure, existence check or metadata lookup failure, and a synchronous throw from any stage via supplySafely). A throw while writing a response or recycling TopicExistsInfo propagates to that terminal stage, which releases once and logs without sending a second response. The old leak was real: the isAllowAutoTopicCreationAsync future was created inside a thenApply callback and never composed into the chain, so its failure was dropped and the permit was never released. Releasing after the response has been submitted and the result recycled, rather than before, is also the safer order. One small behaviour change worth noting in the description: a synchronous throw from the policy read now answers MetadataError rather than AuthorizationError.
The earlier review points (per-stage error mapping and making a recycle() failure visible) are addressed. The error mappings match the previous ones: unsafeGetPartitionedTopicMetadataAsync already unwraps the CompletionException, so the extra unwrap here does not change which error code is sent.
One optional follow-up on the tests is in the inline comment. Separately, and pre-existing (not for this PR): internalHandleGetTopicsOfNamespace has the same shape, where the listSizeHolder.getSizeAsync().thenAccept(...) future is not composed into anything that releases the lookup permit, so a failure of that future would strand the permit. It may be worth a look in a follow-up.
…apache#26606) (cherry picked from commit 5be997a)
…apache#26606) (cherry picked from commit 5be997a)
3 participants
Motivation
Partition metadata lookup starts
isAllowAutoTopicCreationAsyncinside a callback without composing its future into the request chain. If the namespace policy read fails asynchronously, the broker sends no error response and never releases the acquired lookup permit. Repeated failures consume the permits shared by broker lookup and partition metadata lookup, leaving clients waiting for timeouts and subsequent requests rejected.Modifications
handlewhile preserving the existing error mappings and response send paths.TopicExistsInfoin the lookup callback'sfinallyblock. Release the lookup permit once in the terminal callback and log unexpected callback failures without attempting another response.Verifying this change
This change added tests and can be verified as follows:
ServerCnxTestand 20 inGetPartitionMetadataTest) andquickCheck, with retries disabled.Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes