From a3faf2673b266251b87765db6e039e4446b662bf Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Tue, 17 Mar 2026 20:10:31 -0700 Subject: [PATCH 01/39] Add tiered send/receive recovery matching Go SDK parity Add RecoveryKind error classification and recovery-aware retry to azure-core-amqp. Apply tiered recovery to all Service Bus sender, receiver, and session paths. On LINK errors: dispose stale link/session, retry with fresh resources. On CONNECTION errors: force-close the cached connection, retry with fresh connection. Includes quick-retry optimization and didQuickRetry deduplication. Fixes #44688 --- .../implementation/AmqpChannelProcessor.java | 9 + .../ReactorConnectionCache.java | 22 +++ .../amqp/implementation/RecoveryKind.java | 146 ++++++++++++++ .../core/amqp/implementation/RetryUtil.java | 102 ++++++++++ .../amqp/implementation/RecoveryKindTest.java | 180 ++++++++++++++++++ .../servicebus/ConnectionCacheWrapper.java | 13 ++ .../ServiceBusReceiverAsyncClient.java | 30 ++- .../ServiceBusSenderAsyncClient.java | 83 ++++++-- .../servicebus/ServiceBusSessionAcquirer.java | 15 +- .../servicebus/ServiceBusSessionManager.java | 19 +- .../ServiceBusSenderAsyncClientTest.java | 5 +- 11 files changed, 595 insertions(+), 29 deletions(-) create mode 100644 sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java create mode 100644 sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/AmqpChannelProcessor.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/AmqpChannelProcessor.java index 3c1eef933a389..4f23f86e92dae 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/AmqpChannelProcessor.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/AmqpChannelProcessor.java @@ -347,6 +347,15 @@ private void setAndClearChannel() { close(oldChannel); } + /** + * Force-closes the current cached channel so that the next subscriber receives a fresh one. + * This is used for connection-level recovery when the current connection is stale + * but the processor has not detected it (e.g., heartbeats echoed by intermediate infrastructure). + */ + public void forceCloseChannel() { + setAndClearChannel(); + } + /** * Checks the current state of the channel for this channel and returns true if the channel is null or if this * processor is disposed. diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java index d772cfdf06988..ee155d982b150 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java @@ -172,6 +172,28 @@ public boolean isCurrentConnectionClosed() { return (currentConnection != null && currentConnection.isDisposed()) || terminated; } + /** + * Closes the current cached connection (if any) so that the next {@link #get()} call creates + * a fresh connection. This is used for connection-level recovery when the current connection + * is in a stale state that the cache's normal error detection (via endpoint state signals) + * has not detected — for example, when intermediate infrastructure (load balancers, NAT gateways) + * is echoing AMQP heartbeats on behalf of a dead connection. + * + *

This is modeled after the Go SDK's {@code Namespace.Recover()} which explicitly closes + * the old connection and increments the connection revision.

+ * + *

This method is safe to call concurrently. If the connection is already closed or being + * closed, this is a no-op.

+ */ + public void forceCloseConnection() { + final T connection = currentConnection; + if (connection != null && !connection.isDisposed()) { + withConnectionId(logger, connection.getId()) + .log("Force-closing connection for recovery. Next get() will create a fresh connection."); + closeConnection(connection, logger, "Force-close for connection recovery."); + } + } + /** * Terminate so that consumers will no longer be able to request connection. If there is a current (cached) * connection then it will be closed. diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java new file mode 100644 index 0000000000000..3c210c51cf457 --- /dev/null +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java @@ -0,0 +1,146 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package com.azure.core.amqp.implementation; + +import com.azure.core.amqp.exception.AmqpErrorCondition; +import com.azure.core.amqp.exception.AmqpException; + +import java.util.concurrent.TimeoutException; + +/** + * Classifies errors into recovery tiers, determining what resources should be closed + * between retry attempts. This follows the tiered recovery pattern used by the Go, .NET, + * Python, and JS Azure SDKs. + * + * + */ +public enum RecoveryKind { + /** + * No recovery needed — retry on the same link and connection. + * Applies to: server-busy, timeouts, operation-cancelled. + */ + NONE, + + /** + * Close the link (and its session) before retrying. The next retry creates a fresh link + * on the same connection. + * Applies to: link:detach-forced, link:stolen, transient AMQP errors on the link. + */ + LINK, + + /** + * Close the entire connection before retrying. The next retry creates a fresh connection, + * session, and link. + * Applies to: connection:forced, connection:framing-error, proton:io, internal-error. + */ + CONNECTION, + + /** + * Do not retry — the error is permanent. + * Applies to: unauthorized-access, not-found, message-size-exceeded. + */ + FATAL; + + /** + * Classifies the given error into a {@link RecoveryKind} that determines what resources + * should be invalidated between retry attempts. + * + * @param error The error to classify. + * @return The recovery kind for the given error. + */ + public static RecoveryKind classify(Throwable error) { + if (error == null) { + return NONE; + } + + // Timeouts — retry on same link, the link may still be healthy. + if (error instanceof TimeoutException) { + return NONE; + } + + if (error instanceof AmqpException) { + final AmqpException amqpError = (AmqpException) error; + final AmqpErrorCondition condition = amqpError.getErrorCondition(); + + if (condition != null) { + switch (condition) { + // Connection-level errors — close the entire connection. + case CONNECTION_FORCED: + case CONNECTION_FRAMING_ERROR: + case CONNECTION_REDIRECT: + case PROTON_IO: + case INTERNAL_ERROR: + return CONNECTION; + + // Link-level errors — close the link, keep the connection. + case LINK_DETACH_FORCED: + case LINK_STOLEN: + case LINK_REDIRECT: + case PARTITION_NOT_OWNED_ERROR: + case TRANSFER_LIMIT_EXCEEDED: + return LINK; + + // Fatal errors — do not retry. + case NOT_FOUND: + case UNAUTHORIZED_ACCESS: + case LINK_PAYLOAD_SIZE_EXCEEDED: + case RESOURCE_LIMIT_EXCEEDED: + case NOT_ALLOWED: + case NOT_IMPLEMENTED: + case ENTITY_DISABLED_ERROR: + case ENTITY_ALREADY_EXISTS: + case PUBLISHER_REVOKED_ERROR: + case ARGUMENT_ERROR: + case ARGUMENT_OUT_OF_RANGE_ERROR: + case ILLEGAL_STATE: + case MESSAGE_LOCK_LOST: + case STORE_LOCK_LOST_ERROR: + return FATAL; + + // Server-busy and timeouts — retry on same link. + case SERVER_BUSY_ERROR: + case TIMEOUT_ERROR: + case OPERATION_CANCELLED: + return NONE; + + // Session/lock errors — link-level recovery. + // Session lock loss means the session link is invalid and + // a fresh link must be acquired for a new session. + case SESSION_LOCK_LOST: + case SESSION_CANNOT_BE_LOCKED: + case SESSION_NOT_FOUND: + case MESSAGE_NOT_FOUND: + return LINK; + + default: + break; + } + } + + // Transient AMQP errors without a specific condition — link recovery. + if (amqpError.isTransient()) { + return LINK; + } + + // Non-transient AMQP errors without a recognized condition — fatal. + return FATAL; + } + + // RequestResponseChannelClosedException — link-level (parent connection disposing). + if (error instanceof RequestResponseChannelClosedException) { + return LINK; + } + + // Unknown non-AMQP errors — treat as fatal (don't retry application or SDK bugs). + // The Go SDK defaults to CONNECTION for unknown errors, but those are AMQP-layer + // errors (io.EOF, net.Error). Java's non-AMQP exceptions (e.g., AzureException, + // RuntimeException) should fail fast rather than trigger connection recovery. + return FATAL; + } +} diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java index c22c8ac928b3b..79af3ea9c78cd 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java @@ -17,6 +17,8 @@ import java.time.Duration; import java.util.Locale; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; /** * Helper class to help with retry policies. @@ -106,6 +108,44 @@ public static Mono withRetry(Mono source, AmqpRetryOptions retryOption return withRetry(source, retryOptions, timeoutMessage, false); } + /** + * Applies the retry policy with tiered recovery between attempts. Before each retry, + * the error is classified via {@link RecoveryKind#classify(Throwable)} and the recovery + * callback is invoked so the caller can close the appropriate resources (link or connection). + * + *

This matches the tiered recovery pattern used by the Go, .NET, Python, and JS SDKs.

+ * + * @param Type of value in the {@link Mono}. + * @param source The publisher to apply the retry policy to. + * @param retryOptions A {@link AmqpRetryOptions}. + * @param errorMessage Text added to error logs. + * @param recoveryAction Called between retry attempts with the classified {@link RecoveryKind}. + * The caller should close the link (for {@link RecoveryKind#LINK}) or connection + * (for {@link RecoveryKind#CONNECTION}) so the next retry creates fresh resources. + * + * @return A publisher that returns the results of the {@link Mono} if any of the retry attempts + * are successful. Otherwise, propagates the last error. + */ + public static Mono withRetryAndRecovery(Mono source, AmqpRetryOptions retryOptions, String errorMessage, + Consumer recoveryAction) { + return withRetryAndRecovery(source, retryOptions, errorMessage, false, recoveryAction); + } + + /** + * Like {@link #withRetryAndRecovery(Mono, AmqpRetryOptions, String, Consumer)} but with an option to allow + * long-running operations that should not be subject to the per-attempt timeout. + * + * @param allowsLongOperation If true, the source Mono will not be wrapped with a per-attempt timeout. + */ + public static Mono withRetryAndRecovery(Mono source, AmqpRetryOptions retryOptions, String errorMessage, + boolean allowsLongOperation, Consumer recoveryAction) { + if (!allowsLongOperation) { + source = source.timeout(retryOptions.getTryTimeout()); + } + return source.retryWhen(createRetryWithRecovery(retryOptions, recoveryAction)) + .doOnError(error -> LOGGER.error(errorMessage, error)); + } + static Retry createRetry(AmqpRetryOptions options) { final Duration delay = options.getDelay().plus(SERVER_BUSY_WAIT_TIME); final RetryBackoffSpec retrySpec; @@ -129,4 +169,66 @@ static Retry createRetry(AmqpRetryOptions options) { .filter(error -> error instanceof TimeoutException || (error instanceof AmqpException && ((AmqpException) error).isTransient())); } + + /** + * Creates a Reactor {@link Retry} spec that performs tiered recovery between retry attempts. + * Before each retry, the error is classified and the recovery callback is invoked. + * + *

Includes a quick-retry optimization matching the Go SDK: on the first LINK or CONNECTION + * error, the retry fires immediately (no backoff) since the error may come from a previously + * stale link and recovery has just created a fresh one.

+ */ + static Retry createRetryWithRecovery(AmqpRetryOptions options, Consumer recoveryAction) { + final int maxRetries = options.getMaxRetries(); + final Duration baseDelay = options.getDelay().plus(SERVER_BUSY_WAIT_TIME); + final Duration maxDelay = options.getMaxDelay(); + final boolean isFixed = options.getMode() == com.azure.core.amqp.AmqpRetryMode.FIXED; + final AtomicBoolean didQuickRetry = new AtomicBoolean(false); + + return Retry.from(retrySignals -> retrySignals.flatMap(signal -> { + final Throwable failure = signal.failure(); + final long attempt = signal.totalRetriesInARow(); + final RecoveryKind kind = RecoveryKind.classify(failure); + + // FATAL errors — do not retry. + if (kind == RecoveryKind.FATAL) { + return Mono.error(failure); + } + + // Check retry budget. + if (attempt >= maxRetries) { + return Mono.error(failure); + } + + // Perform recovery before retry. + if (kind != RecoveryKind.NONE && recoveryAction != null) { + try { + recoveryAction.accept(kind); + } catch (Exception e) { + LOGGER.atWarning().log("Recovery action failed.", e); + } + } + + // Quick retry: on the FIRST LINK/CONNECTION error, retry immediately (no backoff). + // Uses didQuickRetry flag to prevent repeated immediate retries under persistent + // errors — matching the Go SDK's didQuickRetry + ResetAttempts() pattern. + if (!didQuickRetry.getAndSet(true) && (kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION)) { + LOGGER.atInfo().log("Quick retry after {} recovery (first occurrence).", kind); + return Mono.just(attempt); + } + + // Standard backoff delay. + final Duration delay; + if (isFixed) { + delay = baseDelay; + } else { + long millis = baseDelay.toMillis() * (1L << Math.min(attempt, 30)); + delay = Duration.ofMillis(Math.min(millis, maxDelay.toMillis())); + } + final double jitter = 1.0 + (Math.random() * 2 - 1) * JITTER_FACTOR; + final Duration jitteredDelay = Duration.ofMillis((long) (delay.toMillis() * jitter)); + + return Mono.delay(jitteredDelay).thenReturn(attempt); + })); + } } diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java new file mode 100644 index 0000000000000..6009f40b6d181 --- /dev/null +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java @@ -0,0 +1,180 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package com.azure.core.amqp.implementation; + +import com.azure.core.amqp.exception.AmqpErrorCondition; +import com.azure.core.amqp.exception.AmqpException; +import org.junit.jupiter.api.Test; + +import java.util.concurrent.TimeoutException; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Tests for {@link RecoveryKind#classify(Throwable)}. + */ +class RecoveryKindTest { + + @Test + void nullErrorReturnsNone() { + assertEquals(RecoveryKind.NONE, RecoveryKind.classify(null)); + } + + @Test + void timeoutExceptionReturnsNone() { + assertEquals(RecoveryKind.NONE, RecoveryKind.classify(new TimeoutException("timed out"))); + } + + @Test + void serverBusyReturnsNone() { + final AmqpException error = new AmqpException(true, AmqpErrorCondition.SERVER_BUSY_ERROR, "server busy", null); + assertEquals(RecoveryKind.NONE, RecoveryKind.classify(error)); + } + + @Test + void timeoutErrorConditionReturnsNone() { + final AmqpException error = new AmqpException(true, AmqpErrorCondition.TIMEOUT_ERROR, "timeout", null); + assertEquals(RecoveryKind.NONE, RecoveryKind.classify(error)); + } + + @Test + void linkDetachForcedReturnsLink() { + final AmqpException error + = new AmqpException(true, AmqpErrorCondition.LINK_DETACH_FORCED, "detach forced", null); + assertEquals(RecoveryKind.LINK, RecoveryKind.classify(error)); + } + + @Test + void linkStolenReturnsLink() { + final AmqpException error = new AmqpException(true, AmqpErrorCondition.LINK_STOLEN, "link stolen", null); + assertEquals(RecoveryKind.LINK, RecoveryKind.classify(error)); + } + + @Test + void transientAmqpErrorWithoutConditionReturnsLink() { + final AmqpException error = new AmqpException(true, "transient error", null, null); + assertEquals(RecoveryKind.LINK, RecoveryKind.classify(error)); + } + + @Test + void connectionForcedReturnsConnection() { + final AmqpException error + = new AmqpException(true, AmqpErrorCondition.CONNECTION_FORCED, "connection forced", null); + assertEquals(RecoveryKind.CONNECTION, RecoveryKind.classify(error)); + } + + @Test + void connectionFramingErrorReturnsConnection() { + final AmqpException error + = new AmqpException(true, AmqpErrorCondition.CONNECTION_FRAMING_ERROR, "framing error", null); + assertEquals(RecoveryKind.CONNECTION, RecoveryKind.classify(error)); + } + + @Test + void internalErrorReturnsConnection() { + final AmqpException error = new AmqpException(true, AmqpErrorCondition.INTERNAL_ERROR, "internal error", null); + assertEquals(RecoveryKind.CONNECTION, RecoveryKind.classify(error)); + } + + @Test + void protonIoReturnsConnection() { + final AmqpException error = new AmqpException(true, AmqpErrorCondition.PROTON_IO, "io error", null); + assertEquals(RecoveryKind.CONNECTION, RecoveryKind.classify(error)); + } + + @Test + void connectionRedirectReturnsConnection() { + final AmqpException error = new AmqpException(true, AmqpErrorCondition.CONNECTION_REDIRECT, "redirect", null); + assertEquals(RecoveryKind.CONNECTION, RecoveryKind.classify(error)); + } + + @Test + void linkRedirectReturnsLink() { + final AmqpException error = new AmqpException(true, AmqpErrorCondition.LINK_REDIRECT, "redirect", null); + assertEquals(RecoveryKind.LINK, RecoveryKind.classify(error)); + } + + @Test + void transferLimitExceededReturnsLink() { + final AmqpException error + = new AmqpException(true, AmqpErrorCondition.TRANSFER_LIMIT_EXCEEDED, "transfer limit", null); + assertEquals(RecoveryKind.LINK, RecoveryKind.classify(error)); + } + + @Test + void argumentErrorReturnsFatal() { + final AmqpException error = new AmqpException(false, AmqpErrorCondition.ARGUMENT_ERROR, "bad argument", null); + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(error)); + } + + @Test + void notFoundReturnsFatal() { + final AmqpException error = new AmqpException(false, AmqpErrorCondition.NOT_FOUND, "not found", null); + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(error)); + } + + @Test + void unauthorizedAccessReturnsFatal() { + final AmqpException error + = new AmqpException(false, AmqpErrorCondition.UNAUTHORIZED_ACCESS, "unauthorized", null); + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(error)); + } + + @Test + void payloadSizeExceededReturnsFatal() { + final AmqpException error + = new AmqpException(false, AmqpErrorCondition.LINK_PAYLOAD_SIZE_EXCEEDED, "too large", null); + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(error)); + } + + @Test + void notAllowedReturnsFatal() { + final AmqpException error = new AmqpException(false, AmqpErrorCondition.NOT_ALLOWED, "not allowed", null); + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(error)); + } + + @Test + void nonTransientAmqpErrorReturnsFatal() { + final AmqpException error = new AmqpException(false, "permanent error", null, null); + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(error)); + } + + @Test + void requestResponseChannelClosedReturnsLink() { + final RequestResponseChannelClosedException error = new RequestResponseChannelClosedException("channel closed"); + assertEquals(RecoveryKind.LINK, RecoveryKind.classify(error)); + } + + @Test + void unknownExceptionReturnsFatal() { + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(new RuntimeException("unknown"))); + } + + @Test + void sessionLockLostReturnsLink() { + final AmqpException error + = new AmqpException(true, AmqpErrorCondition.SESSION_LOCK_LOST, "session lock lost", null); + assertEquals(RecoveryKind.LINK, RecoveryKind.classify(error)); + } + + @Test + void messageLockLostReturnsFatal() { + final AmqpException error + = new AmqpException(false, AmqpErrorCondition.MESSAGE_LOCK_LOST, "message lock lost", null); + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(error)); + } + + @Test + void storeLockLostReturnsFatal() { + final AmqpException error + = new AmqpException(false, AmqpErrorCondition.STORE_LOCK_LOST_ERROR, "store lock lost", null); + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(error)); + } + + @Test + void operationCancelledReturnsNone() { + final AmqpException error = new AmqpException(true, AmqpErrorCondition.OPERATION_CANCELLED, "cancelled", null); + assertEquals(RecoveryKind.NONE, RecoveryKind.classify(error)); + } +} diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java index 1d4aa44b4c422..885ec94e606dd 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java @@ -51,4 +51,17 @@ AmqpRetryOptions getRetryOptions() { boolean isChannelClosed() { return isV2 ? cache.isCurrentConnectionClosed() : processor.isChannelClosed(); } + + /** + * Force-closes the current cached connection so the next get() creates a fresh one. + * Used for connection-level recovery when the connection is stale but the cache + * has not detected it via endpoint state signals. + */ + void forceCloseConnection() { + if (isV2) { + cache.forceCloseConnection(); + } else { + processor.forceCloseChannel(); + } + } } diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java index 0b3154ec33efb..ad5fbba1f9302 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java @@ -11,6 +11,7 @@ import com.azure.core.amqp.implementation.MessageFlux; import com.azure.core.amqp.implementation.MessageSerializer; import com.azure.core.amqp.implementation.RequestResponseChannelClosedException; +import com.azure.core.amqp.implementation.RecoveryKind; import com.azure.core.amqp.implementation.RetryUtil; import com.azure.core.amqp.implementation.StringUtil; import com.azure.core.amqp.implementation.handler.DeliveryNotOnLinkException; @@ -1734,17 +1735,26 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { // [2]. When we try to create a new session (to host the new link) but on a connection being disposed, // the retry can eventually receive a new connection and then proceed with creating session and link. // - final Mono retryableReceiveLinkMono - = RetryUtil.withRetry(receiveLinkMono.onErrorMap(RequestResponseChannelClosedException.class, e -> { - // When the current connection is being disposed, the V1 ConnectionProcessor or V2 ReactorConnectionCache - // can produce a new connection if downstream request. In this context, treat - // RequestResponseChannelClosedException error from the following two sources as retry-able so that - // retry can obtain a new connection - - // 1. error from the RequestResponseChannel scoped to the current connection being disposed, - // 2. error from the V2 RequestResponseChannelCache scoped to the current connection being disposed. - // + final Mono retryableReceiveLinkMono = RetryUtil + .withRetryAndRecovery(receiveLinkMono.onErrorMap(RequestResponseChannelClosedException.class, e -> { return new AmqpException(true, e.getMessage(), e, null); - }), connectionCacheWrapper.getRetryOptions(), "Failed to create receive link " + linkName, true); + }), connectionCacheWrapper.getRetryOptions(), "Failed to create receive link " + linkName, true, + recoveryKind -> { + if (recoveryKind == RecoveryKind.LINK || recoveryKind == RecoveryKind.CONNECTION) { + LOGGER.atWarning() + .addKeyValue(LINK_NAME_KEY, linkName) + .addKeyValue("recoveryKind", recoveryKind) + .log("Receive link creation failed, performing {} recovery.", recoveryKind); + + // For LINK errors during link creation, the session hosting the link may be stale. + // Ask the connection to remove it so the next retry creates a fresh session + link. + // The entityPath is the session name used by createReceiveLink(). + connectionProcessor.subscribe(connection -> connection.removeSession(entityPath)); + } + if (recoveryKind == RecoveryKind.CONNECTION) { + connectionCacheWrapper.forceCloseConnection(); + } + }); // A Flux that produces a new AmqpReceiveLink each time it receives a request from the below // 'AmqpReceiveLinkProcessor'. Obviously, the processor requests a link when there is a downstream subscriber. diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java index 58db3b129e65c..030cf772243be 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java @@ -10,7 +10,9 @@ import com.azure.core.amqp.implementation.AmqpSendLink; import com.azure.core.amqp.implementation.ErrorContextProvider; import com.azure.core.amqp.implementation.MessageSerializer; +import com.azure.core.amqp.implementation.RecoveryKind; import com.azure.core.amqp.implementation.RequestResponseChannelClosedException; +import com.azure.core.amqp.implementation.RetryUtil; import com.azure.core.annotation.ServiceClient; import com.azure.core.util.CoreUtils; import com.azure.core.util.logging.ClientLogger; @@ -228,12 +230,14 @@ public final class ServiceBusSenderAsyncClient implements AutoCloseable { private final ClientLogger logger; private final AtomicReference linkName = new AtomicReference<>(); + private final AtomicReference lastSendLink = new AtomicReference<>(); private final AtomicBoolean isDisposed = new AtomicBoolean(); private final MessageSerializer messageSerializer; private final AmqpRetryOptions retryOptions; private final MessagingEntityType entityType; private final Runnable onClientClose; private final String entityName; + private final ConnectionCacheWrapper connectionCacheWrapper; private final Mono connectionProcessor; private final String fullyQualifiedNamespace; private final String viaEntityName; @@ -254,6 +258,7 @@ public final class ServiceBusSenderAsyncClient implements AutoCloseable { this.retryOptions = Objects.requireNonNull(retryOptions, "'retryOptions' cannot be null."); this.entityName = Objects.requireNonNull(entityName, "'entityPath' cannot be null."); Objects.requireNonNull(connectionCacheWrapper, "'connectionCacheWrapper' cannot be null."); + this.connectionCacheWrapper = connectionCacheWrapper; this.connectionProcessor = connectionCacheWrapper.getConnection(); this.fullyQualifiedNamespace = connectionCacheWrapper.getFullyQualifiedNamespace(); this.instrumentation = Objects.requireNonNull(instrumentation, "'instrumentation' cannot be null."); @@ -810,8 +815,9 @@ private Mono scheduleMessageInternal(ServiceBusMessage message, OffsetDate return monoError(logger, new NullPointerException("'scheduledEnqueueTime' cannot be null.")); } - return tracer.traceScheduleMono("ServiceBus.scheduleMessage", - getSendLinkWithRetry("schedule-message").flatMap(link -> link.getLinkSize().flatMap(size -> { + return tracer.traceScheduleMono("ServiceBus.scheduleMessage", getSendLink("schedule-message").flatMap(link -> { + lastSendLink.set(link); + return link.getLinkSize().flatMap(size -> { final int maxSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; return connectionProcessor.flatMap(connection -> connection.getManagementNode(entityName, entityType)) .flatMap( @@ -819,7 +825,8 @@ private Mono scheduleMessageInternal(ServiceBusMessage message, OffsetDate .schedule(Arrays.asList(message), scheduledEnqueueTime, maxSize, link.getLinkName(), transactionContext) .next()); - })), message, message.getContext()).onErrorMap(this::mapError); + }); + }), message, message.getContext()).onErrorMap(this::mapError); } /** @@ -860,6 +867,7 @@ private Mono sendBatchInternal(ServiceBusMessageBatch batch, }); final Mono sendMessage = getSendLink("send-batch").flatMap(link -> { + lastSendLink.set(link); if (transactionContext != null && transactionContext.getTransactionId() != null) { final TransactionalState deliveryState = new TransactionalState(); deliveryState.setTxnId(Binary.create(transactionContext.getTransactionId())); @@ -871,8 +879,11 @@ private Mono sendBatchInternal(ServiceBusMessageBatch batch, } }); - final String message = "Sending messages timed out. message-count:" + batch.getCount() + entityId(); - final Mono withRetry = withRetry(sendMessage, retryOptions, message).onErrorMap(this::mapError); + final String timeoutMessage = "Sending messages timed out. message-count:" + batch.getCount() + entityId(); + final Mono withRetry + = RetryUtil.withRetryAndRecovery(sendMessage, retryOptions, timeoutMessage, recoveryKind -> { + performRecovery(recoveryKind, "sendBatch"); + }).onErrorMap(this::mapError); return instrumentation.instrumentSendBatch("ServiceBus.send", withRetry, batch.getMessages()); } @@ -883,19 +894,26 @@ private Mono sendFluxInternal(Flux messages, new IllegalStateException(String.format(INVALID_OPERATION_DISPOSED_SENDER, "sendMessage"))); } - final Mono> batchList - = getSendLinkWithRetry("send-batches").flatMap(link -> link.getLinkSize().flatMap(size -> { + final Mono> batchList = getSendLink("send-batches").flatMap(link -> { + lastSendLink.set(link); + return link.getLinkSize().flatMap(size -> { final int batchSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; final CreateMessageBatchOptions batchOptions = new CreateMessageBatchOptions().setMaximumSizeInBytes(batchSize); return messages.collect( new AmqpMessageCollector(isV2, batchOptions, 1, link::getErrorContext, tracer, messageSerializer)); - })); + }); + }); - return batchList.flatMap(list -> Flux.fromIterable(list) + final Mono sendOperation = batchList.flatMap(list -> Flux.fromIterable(list) .flatMap(batch -> sendBatchInternal(batch, transactionContext)) .then() - .doOnError(error -> logger.error("Error sending batch.", error))).onErrorMap(this::mapError); + .doOnError(error -> logger.error("Error sending batch.", error))); + + return RetryUtil.withRetryAndRecovery(sendOperation, retryOptions, "Sending messages timed out." + entityId(), + recoveryKind -> { + performRecovery(recoveryKind, "sendFlux"); + }).onErrorMap(this::mapError); } private Mono getSendLink(String callSite) { @@ -926,7 +944,10 @@ private Mono getSendLink(String callSite) { } private Mono getSendLinkWithRetry(String callSite) { - return withRetry(getSendLink(callSite), retryOptions, String.format(retryGetLinkErrorMessageFormat, callSite)); + return RetryUtil.withRetryAndRecovery(getSendLink(callSite), retryOptions, + String.format(retryGetLinkErrorMessageFormat, callSite), recoveryKind -> { + performRecovery(recoveryKind, "getSendLink-" + callSite); + }); } private Throwable mapError(Throwable throwable) { @@ -936,6 +957,46 @@ private Throwable mapError(Throwable throwable) { return throwable; } + /** + * Performs tiered recovery by disposing stale resources based on the classified error. + * For LINK recovery, disposes the send link so the next retry creates a fresh one. + * For CONNECTION recovery, disposes the link and the connection so the connection + * processor creates everything fresh. + * + *

This matches the Go SDK's RecoverIfNeeded() and the .NET SDK's + * FaultTolerantAmqpObject pattern.

+ */ + private void performRecovery(RecoveryKind recoveryKind, String callSite) { + if (recoveryKind == RecoveryKind.NONE || recoveryKind == RecoveryKind.FATAL) { + return; + } + + logger.atWarning() + .addKeyValue(ENTITY_PATH_KEY, entityName) + .addKeyValue("recoveryKind", recoveryKind) + .addKeyValue("callSite", callSite) + .log("Performing {} recovery before retry.", recoveryKind); + + // Dispose the cached send link so the next retry creates a fresh one. + final AmqpSendLink link = lastSendLink.getAndSet(null); + if (link != null) { + link.dispose(); + } + linkName.set(null); + + // For CONNECTION errors, the link disposal above is the sender's responsibility. + // Connection-level recovery (closing and recreating the AMQP connection) is handled + // by the ReactorConnectionCache when it detects the connection's endpoint state has + // changed. My job is to reset the cached link reference so the next retry goes through the fresh path — the connection disposal happens organically. + if (recoveryKind == RecoveryKind.CONNECTION) { + // Force-close the cached connection so the next get() on the connection + // processor creates a fresh one. This handles the stale-connection scenario + // where heartbeats are echoed by intermediate infrastructure and the cache + // hasn't detected the failure. Matches Go SDK's Namespace.Recover(). + connectionCacheWrapper.forceCloseConnection(); + } + } + private String entityId() { return " " + ENTITY_PATH_KEY + ":" + entityName + (viaEntityName != null ? " " + VIA_ENTITY_NAME_KEY + ":" + viaEntityName : "") + " "; diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java index ee9a0a9e03994..147524ab1c7b4 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java @@ -9,6 +9,7 @@ import com.azure.core.amqp.exception.AmqpException; import com.azure.core.amqp.implementation.StringUtil; import com.azure.core.amqp.implementation.handler.ReceiveLinkHandler2; +import com.azure.core.amqp.implementation.RecoveryKind; import com.azure.core.util.logging.ClientLogger; import com.azure.messaging.servicebus.implementation.MessagingEntityType; import com.azure.messaging.servicebus.implementation.ServiceBusManagementNode; @@ -137,12 +138,24 @@ private Mono acquireIntern(String sessionId) { return acquireSession(sessionId).timeout(tryTimeout) .retryWhen(Retry.from(signals -> signals.flatMap(signal -> { final Throwable t = signal.failure(); + final RecoveryKind kind = RecoveryKind.classify(t); + if (kind == RecoveryKind.CONNECTION) { + logger.atWarning() + .addKeyValue(ENTITY_PATH_KEY, entityPath) + .log("Connection-level error acquiring session, forcing connection recovery.", t); + connectionCacheWrapper.forceCloseConnection(); + } if (isTimeoutError(t)) { logger.atVerbose() .addKeyValue(ENTITY_PATH_KEY, entityPath) .addKeyValue("attempt", signal.totalRetriesInARow()) .log("Timeout while acquiring session '{}'.", sessionName(sessionId), t); - // retry session acquire using Schedulers.parallel() and free the QPid thread. + return Mono.delay(Duration.ZERO); + } + if (kind == RecoveryKind.LINK) { + logger.atWarning() + .addKeyValue(ENTITY_PATH_KEY, entityPath) + .log("Link-level error acquiring session, retrying.", t); return Mono.delay(Duration.ZERO); } return publishError(sessionId, t, true); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java index 3a6343c987100..0a111a3057956 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java @@ -8,6 +8,7 @@ import com.azure.core.amqp.exception.AmqpException; import com.azure.core.amqp.exception.SessionErrorContext; import com.azure.core.amqp.implementation.MessageSerializer; +import com.azure.core.amqp.implementation.RecoveryKind; import com.azure.core.amqp.implementation.StringUtil; import com.azure.core.util.logging.ClientLogger; import com.azure.messaging.servicebus.implementation.DispositionStatus; @@ -281,11 +282,20 @@ Mono getActiveLink() { .timeout(operationTimeout) .then(Mono.just(link)))).retryWhen(Retry.from(retrySignals -> retrySignals.flatMap(signal -> { final Throwable failure = signal.failure(); + final RecoveryKind kind = RecoveryKind.classify(failure); LOGGER.atInfo() .addKeyValue(ENTITY_PATH_KEY, entityPath) .addKeyValue("attempt", signal.totalRetriesInARow()) + .addKeyValue("recoveryKind", kind) .log("Error occurred while getting unnamed session.", failure); + if (kind == RecoveryKind.CONNECTION) { + LOGGER.atWarning() + .addKeyValue(ENTITY_PATH_KEY, entityPath) + .log("Connection-level error in session manager, forcing connection recovery."); + connectionCacheWrapper.forceCloseConnection(); + } + if (isDisposed.get()) { return Mono.error( new AmqpException(false, "SessionManager is already disposed.", failure, getErrorContext())); @@ -293,12 +303,9 @@ Mono getActiveLink() { return Mono.delay(Duration.ZERO); } else if (failure instanceof AmqpException && ((AmqpException) failure).getErrorCondition() == AmqpErrorCondition.TIMEOUT_ERROR) { - // The link closed remotely with 'Detach {errorCondition:com.microsoft:timeout}' frame because - // the broker waited for N seconds (60 sec hard limit today) but there was no free or new session. - // - // Given N seconds elapsed since the last session acquire attempt, request for a session on - // the 'parallel' Scheduler and free the 'QPid' thread for other IO. - // + return Mono.delay(Duration.ZERO); + } else if (kind == RecoveryKind.LINK) { + // Link-level error — retry to get a fresh link. return Mono.delay(Duration.ZERO); } else { final long id = System.nanoTime(); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientTest.java index 20ca897316e91..204eb1db52f78 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientTest.java @@ -6,6 +6,8 @@ import com.azure.core.amqp.AmqpEndpointState; import com.azure.core.amqp.AmqpRetryMode; import com.azure.core.amqp.AmqpRetryOptions; +import com.azure.core.amqp.exception.AmqpErrorCondition; +import com.azure.core.amqp.exception.AmqpException; import com.azure.core.amqp.AmqpTransaction; import com.azure.core.amqp.AmqpTransportType; import com.azure.core.amqp.FixedAmqpRetryPolicy; @@ -668,7 +670,8 @@ void failedSendMessageReportsMetrics(boolean isV2) { when(connection.createSendLink(eq(ENTITY_NAME), eq(ENTITY_NAME), eq(retryOptions), isNull(), eq(CLIENT_IDENTIFIER))).thenReturn(Mono.just(sendLink)); - when(sendLink.send(any(Message.class))).thenThrow(new RuntimeException("foo")); + when(sendLink.send(any(Message.class))) + .thenReturn(Mono.error(new AmqpException(false, AmqpErrorCondition.NOT_FOUND, "entity not found", null))); // Act StepVerifier.create(sender.sendMessage(new ServiceBusMessage(TEST_CONTENTS))) From 5e28c3366e093ae8c5f8c18dcc4338badf255c08 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Tue, 17 Mar 2026 21:09:01 -0700 Subject: [PATCH 02/39] fix(ci): build azure-core-amqp from source for tiered recovery PR Add azure-core-amqp to AdditionalModules in ci.yml and trigger paths so CI builds it from source alongside servicebus. Update pom.xml to reference 2.12.0-beta.1 with current tag (cross-module PR -- to be revisited after azure-core-amqp is released to Maven Central). Note: uses current tag temporarily; reviewer to confirm release sequencing. --- sdk/servicebus/azure-messaging-servicebus/pom.xml | 2 +- sdk/servicebus/ci.yml | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/pom.xml b/sdk/servicebus/azure-messaging-servicebus/pom.xml index 47390eae58602..d55656f00bbc7 100644 --- a/sdk/servicebus/azure-messaging-servicebus/pom.xml +++ b/sdk/servicebus/azure-messaging-servicebus/pom.xml @@ -69,7 +69,7 @@ com.azure azure-core-amqp - 2.11.3 + 2.12.0-beta.1 com.azure diff --git a/sdk/servicebus/ci.yml b/sdk/servicebus/ci.yml index 58823bab7a695..b957c80a24d4b 100644 --- a/sdk/servicebus/ci.yml +++ b/sdk/servicebus/ci.yml @@ -13,6 +13,7 @@ trigger: - sdk/servicebus/azure-messaging-servicebus-stress/ - sdk/servicebus/azure-messaging-servicebus-track2-perf/ - sdk/servicebus/build/ + - sdk/core/azure-core-amqp/ exclude: - sdk/servicebus/pom.xml - sdk/servicebus/azure-messaging-servicebus/pom.xml @@ -33,6 +34,7 @@ pr: - sdk/servicebus/azure-messaging-servicebus-stress/ - sdk/servicebus/azure-messaging-servicebus-track2-perf/ - sdk/servicebus/build/ + - sdk/core/azure-core-amqp/ exclude: - sdk/servicebus/pom.xml - sdk/servicebus/azure-messaging-servicebus/pom.xml @@ -59,3 +61,6 @@ extends: # required by the above perf libraries - name: perf-test-core groupId: com.azure + # Build azure-core-amqp from source (needed for RecoveryKind, tiered retry) + - name: azure-core-amqp + groupId: com.azure From 6116b7b30cb099d57615a947339e78032216b057 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 10:41:38 -0700 Subject: [PATCH 03/39] fix(review): address Copilot PR review feedback on tiered recovery Address 6 comments from Copilot PR review on #48460: - sendFluxInternal: wrap only batchList (link acquisition) with withRetryAndRecovery, not the full sendOperation. Wrapping the outer operation caused the user-provided Flux to be re-subscribed on each retry and nested retries with sendBatchInternal. - scheduleMessageInternal: change getSendLink to getSendLinkWithRetry so schedule operations get the same tiered recovery as other send paths. - ReactorConnectionCache.forceCloseConnection: use connection.dispose() instead of closeAsync() so isDisposed() returns true synchronously. This ensures cacheInvalidateIf invalidates the cached reference immediately on the next get() call. - RetryUtil: replace Math.random() with ThreadLocalRandom.current() to eliminate shared RNG contention and improve testability. - performRecovery comment: remove contradictory 'happens organically' comment that conflicted with the explicit forceCloseConnection() call. - ServiceBusReceiverAsyncClient: add error handler to the subscribe() call inside the LINK recovery callback so failures are logged and do not silently leak. --- .../ReactorConnectionCache.java | 5 +- .../core/amqp/implementation/RetryUtil.java | 3 +- .../ServiceBusReceiverAsyncClient.java | 5 +- .../ServiceBusSenderAsyncClient.java | 55 ++++++++++--------- 4 files changed, 40 insertions(+), 28 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java index ee155d982b150..248877d56563d 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java @@ -190,7 +190,10 @@ public void forceCloseConnection() { if (connection != null && !connection.isDisposed()) { withConnectionId(logger, connection.getId()) .log("Force-closing connection for recovery. Next get() will create a fresh connection."); - closeConnection(connection, logger, "Force-close for connection recovery."); + // Call dispose() rather than closeAsync() so that isDisposed() returns true synchronously. + // This ensures cacheInvalidateIf immediately invalidates the cached reference on the next + // get() call, rather than waiting for the async close handshake to complete. + connection.dispose(); } } diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java index 79af3ea9c78cd..a590fe3c034c2 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java @@ -16,6 +16,7 @@ import java.time.Duration; import java.util.Locale; +import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; @@ -225,7 +226,7 @@ static Retry createRetryWithRecovery(AmqpRetryOptions options, Consumer connection.removeSession(entityPath)); + connectionProcessor.subscribe(connection -> connection.removeSession(entityPath), + error -> LOGGER.atWarning() + .addKeyValue(LINK_NAME_KEY, linkName) + .log("Error removing stale session during LINK recovery.", error)); } if (recoveryKind == RecoveryKind.CONNECTION) { connectionCacheWrapper.forceCloseConnection(); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java index 030cf772243be..99dc2b8ddb69b 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java @@ -815,18 +815,21 @@ private Mono scheduleMessageInternal(ServiceBusMessage message, OffsetDate return monoError(logger, new NullPointerException("'scheduledEnqueueTime' cannot be null.")); } - return tracer.traceScheduleMono("ServiceBus.scheduleMessage", getSendLink("schedule-message").flatMap(link -> { - lastSendLink.set(link); - return link.getLinkSize().flatMap(size -> { - final int maxSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; - return connectionProcessor.flatMap(connection -> connection.getManagementNode(entityName, entityType)) - .flatMap( - managementNode -> managementNode - .schedule(Arrays.asList(message), scheduledEnqueueTime, maxSize, link.getLinkName(), - transactionContext) - .next()); - }); - }), message, message.getContext()).onErrorMap(this::mapError); + return tracer + .traceScheduleMono("ServiceBus.scheduleMessage", getSendLinkWithRetry("schedule-message").flatMap(link -> { + lastSendLink.set(link); + return link.getLinkSize().flatMap(size -> { + final int maxSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; + return connectionProcessor + .flatMap(connection -> connection.getManagementNode(entityName, entityType)) + .flatMap( + managementNode -> managementNode + .schedule(Arrays.asList(message), scheduledEnqueueTime, maxSize, link.getLinkName(), + transactionContext) + .next()); + }); + }), message, message.getContext()) + .onErrorMap(this::mapError); } /** @@ -905,15 +908,20 @@ private Mono sendFluxInternal(Flux messages, }); }); - final Mono sendOperation = batchList.flatMap(list -> Flux.fromIterable(list) + // Wrap only the link-acquisition+batch-collection phase with retry+recovery. + // Individual batch sends are already retried inside sendBatchInternal, so wrapping the + // entire sendOperation (which includes messages.collect()) would cause the user-provided + // Flux to be re-subscribed on each retry and could duplicate batches that already sent. + final Mono> batchListWithRecovery = RetryUtil.withRetryAndRecovery(batchList, + retryOptions, "Failed to acquire send link for batch collection." + entityId(), + recoveryKind -> performRecovery(recoveryKind, "sendFlux-link")); + + final Mono sendOperation = batchListWithRecovery.flatMap(list -> Flux.fromIterable(list) .flatMap(batch -> sendBatchInternal(batch, transactionContext)) .then() .doOnError(error -> logger.error("Error sending batch.", error))); - return RetryUtil.withRetryAndRecovery(sendOperation, retryOptions, "Sending messages timed out." + entityId(), - recoveryKind -> { - performRecovery(recoveryKind, "sendFlux"); - }).onErrorMap(this::mapError); + return sendOperation.onErrorMap(this::mapError); } private Mono getSendLink(String callSite) { @@ -984,15 +992,12 @@ private void performRecovery(RecoveryKind recoveryKind, String callSite) { } linkName.set(null); - // For CONNECTION errors, the link disposal above is the sender's responsibility. - // Connection-level recovery (closing and recreating the AMQP connection) is handled - // by the ReactorConnectionCache when it detects the connection's endpoint state has - // changed. My job is to reset the cached link reference so the next retry goes through the fresh path — the connection disposal happens organically. + // For CONNECTION errors, explicitly force-close the cached connection so the + // next get() on the connection processor creates a fresh one. This handles the + // stale-connection scenario where heartbeats are echoed by intermediate + // infrastructure and the cache has not yet detected the failure. + // Matches Go SDK's Namespace.Recover(). if (recoveryKind == RecoveryKind.CONNECTION) { - // Force-close the cached connection so the next get() on the connection - // processor creates a fresh one. This handles the stale-connection scenario - // where heartbeats are echoed by intermediate infrastructure and the cache - // hasn't detected the failure. Matches Go SDK's Namespace.Recover(). connectionCacheWrapper.forceCloseConnection(); } } From f3a29a7bef1d38626453e336962420372a4fb416 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 13:24:11 -0700 Subject: [PATCH 04/39] fix(review): address second Copilot review on tiered recovery - T1: ServiceBusSessionAcquirer - return delay after forceCloseConnection for RecoveryKind.CONNECTION so session acquisition retries after connection recovery - T2: ServiceBusSessionManager - merge LINK and CONNECTION into a single delay branch so CONNECTION errors retry instead of falling through to publishError - T3: ServiceBusSenderAsyncClient - narrow withRetryAndRecovery to wrap only link acquisition (getSendLink); messages.collect() moved outside retry boundary to avoid re-subscribing user Flux on retry - T4: RecoveryKind - reclassify OPERATION_CANCELLED from NONE to LINK because core-amqp raises it when AMQP layer unexpectedly aborts or disconnects, which requires link recovery (e.g. ReceiverUnsettledDeliveries remote Released outcome) - T5: RecoveryKind - reclassify RESOURCE_LIMIT_EXCEEDED from FATAL to NONE to match ReactorSender.isGeneralSendError() which treats it as retriable alongside SERVER_BUSY and TIMEOUT - T6: RetryUtilTest - add four tests for createRetryWithRecovery covering FATAL no-retry, LINK recovery callback, CONNECTION recovery callback, and retry budget exhaustion; use virtual time for backoff-delay scenarios - T7: RecoveryKindTest - rename operationCancelled test to expect LINK result, add new test asserting RESOURCE_LIMIT_EXCEEDED classifies as NONE --- .../amqp/implementation/RecoveryKind.java | 12 +- .../amqp/implementation/RecoveryKindTest.java | 9 +- .../amqp/implementation/RetryUtilTest.java | 104 ++++++++++++++++++ .../ServiceBusSenderAsyncClient.java | 26 ++--- .../servicebus/ServiceBusSessionAcquirer.java | 1 + .../servicebus/ServiceBusSessionManager.java | 4 +- 6 files changed, 135 insertions(+), 21 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java index 3c210c51cf457..31ccc5be35d48 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java @@ -23,7 +23,7 @@ public enum RecoveryKind { /** * No recovery needed — retry on the same link and connection. - * Applies to: server-busy, timeouts, operation-cancelled. + * Applies to: server-busy, timeouts, resource-limit-exceeded. */ NONE, @@ -84,13 +84,15 @@ public static RecoveryKind classify(Throwable error) { case LINK_REDIRECT: case PARTITION_NOT_OWNED_ERROR: case TRANSFER_LIMIT_EXCEEDED: + // operation-cancelled can signal "AMQP layer unexpectedly aborted or disconnected" + // (e.g. ReceiverUnsettledDeliveries remote Released outcome), requiring link recovery. + case OPERATION_CANCELLED: return LINK; // Fatal errors — do not retry. case NOT_FOUND: case UNAUTHORIZED_ACCESS: case LINK_PAYLOAD_SIZE_EXCEEDED: - case RESOURCE_LIMIT_EXCEEDED: case NOT_ALLOWED: case NOT_IMPLEMENTED: case ENTITY_DISABLED_ERROR: @@ -103,10 +105,12 @@ public static RecoveryKind classify(Throwable error) { case STORE_LOCK_LOST_ERROR: return FATAL; - // Server-busy and timeouts — retry on same link. + // Server-busy, timeouts, and resource-limit errors — retry on same link. + // RESOURCE_LIMIT_EXCEEDED is treated as transient here because ReactorSender + // groups it alongside SERVER_BUSY and TIMEOUT in its send-error retry logic. case SERVER_BUSY_ERROR: case TIMEOUT_ERROR: - case OPERATION_CANCELLED: + case RESOURCE_LIMIT_EXCEEDED: return NONE; // Session/lock errors — link-level recovery. diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java index 6009f40b6d181..7c36e7d61d9f6 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java @@ -173,8 +173,15 @@ void storeLockLostReturnsFatal() { } @Test - void operationCancelledReturnsNone() { + void operationCancelledReturnsLink() { final AmqpException error = new AmqpException(true, AmqpErrorCondition.OPERATION_CANCELLED, "cancelled", null); + assertEquals(RecoveryKind.LINK, RecoveryKind.classify(error)); + } + + @Test + void resourceLimitExceededReturnsNone() { + final AmqpException error + = new AmqpException(true, AmqpErrorCondition.RESOURCE_LIMIT_EXCEEDED, "resource limit", null); assertEquals(RecoveryKind.NONE, RecoveryKind.classify(error)); } } diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java index b25d920b31f6f..cd5e08a815588 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java @@ -9,6 +9,7 @@ import com.azure.core.amqp.AmqpTransportType; import com.azure.core.amqp.ExponentialAmqpRetryPolicy; import com.azure.core.amqp.FixedAmqpRetryPolicy; +import com.azure.core.amqp.exception.AmqpErrorCondition; import com.azure.core.amqp.exception.AmqpErrorContext; import com.azure.core.amqp.exception.AmqpException; import org.junit.jupiter.api.Assertions; @@ -24,9 +25,12 @@ import reactor.util.retry.RetryBackoffSpec; import java.time.Duration; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Predicate; import java.util.stream.Stream; @@ -203,4 +207,104 @@ void retryFilter(Throwable throwable, boolean expected) { // Assert assertEquals(expected, actual); } + + // ---- createRetryWithRecovery tests ---- + + /** + * FATAL errors must not be retried and must not invoke the recovery callback. + */ + @Test + void createRetryWithRecovery_fatalErrorTerminatesImmediately() { + // Arrange + final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(3).setDelay(Duration.ofMillis(100)); + final AtomicInteger recoveryCount = new AtomicInteger(); + final Retry retry = RetryUtil.createRetryWithRecovery(options, kind -> recoveryCount.incrementAndGet()); + final AmqpException fatalError = new AmqpException(false, AmqpErrorCondition.NOT_FOUND, "not found", null); + + // Act & Assert + StepVerifier.create(Mono.error(fatalError).retryWhen(retry)) + .expectErrorSatisfies(e -> assertEquals(fatalError, e)) + .verify(Duration.ofSeconds(5)); + + assertEquals(0, recoveryCount.get(), "Recovery callback must not be called for FATAL errors"); + } + + /** + * LINK errors must invoke the recovery callback and retry. The first occurrence uses + * the quick-retry path (no backoff delay). + */ + @Test + void createRetryWithRecovery_linkErrorInvokesRecoveryAndRetries() { + // Arrange + final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(3).setDelay(Duration.ofMillis(100)); + final List recoveries = new ArrayList<>(); + final Retry retry = RetryUtil.createRetryWithRecovery(options, recoveries::add); + final AtomicInteger attempt = new AtomicInteger(); + final Mono source = Mono.defer(() -> { + if (attempt.getAndIncrement() < 2) { + return Mono.error(new AmqpException(true, AmqpErrorCondition.LINK_DETACH_FORCED, "detach", null)); + } + return Mono.just(42); + }); + + // Act & Assert — use virtual time because the second retry applies a SERVER_BUSY backoff delay. + StepVerifier.withVirtualTime(() -> source.retryWhen(retry)) + .expectSubscription() + .thenAwait(Duration.ofMinutes(1)) + .expectNext(42) + .expectComplete() + .verify(Duration.ofSeconds(5)); + + assertEquals(2, recoveries.size(), "Recovery callback called once per LINK error"); + assertTrue(recoveries.stream().allMatch(k -> k == RecoveryKind.LINK)); + } + + /** + * CONNECTION errors must invoke the recovery callback with CONNECTION kind. + */ + @Test + void createRetryWithRecovery_connectionErrorInvokesRecovery() { + // Arrange + final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(2).setDelay(Duration.ofMillis(100)); + final AtomicReference capturedKind = new AtomicReference<>(); + final Retry retry = RetryUtil.createRetryWithRecovery(options, capturedKind::set); + final AtomicInteger attempt = new AtomicInteger(); + final Mono source = Mono.defer(() -> { + if (attempt.getAndIncrement() == 0) { + return Mono.error(new AmqpException(true, AmqpErrorCondition.CONNECTION_FORCED, "forced", null)); + } + return Mono.just(1); + }); + + // Act & Assert + StepVerifier.create(source.retryWhen(retry)).expectNext(1).expectComplete().verify(Duration.ofSeconds(5)); + + assertEquals(RecoveryKind.CONNECTION, capturedKind.get(), + "Recovery callback must receive CONNECTION kind for connection errors"); + } + + /** + * After the retry budget is exhausted the error must propagate without further retries. + */ + @Test + void createRetryWithRecovery_exhaustedRetriesTerminateWithError() { + // Arrange + final int maxRetries = 2; + final AmqpRetryOptions options + = new AmqpRetryOptions().setMaxRetries(maxRetries).setDelay(Duration.ofMillis(10)); + final AtomicInteger recoveryCount = new AtomicInteger(); + final Retry retry = RetryUtil.createRetryWithRecovery(options, kind -> recoveryCount.incrementAndGet()); + final AmqpException transientError + = new AmqpException(true, AmqpErrorCondition.LINK_DETACH_FORCED, "detach", null); + + // Act & Assert — use virtual time because the second retry applies a SERVER_BUSY backoff delay. + StepVerifier.withVirtualTime(() -> Mono.error(transientError).retryWhen(retry)) + .expectSubscription() + .thenAwait(Duration.ofMinutes(1)) + .expectError(AmqpException.class) + .verify(Duration.ofSeconds(5)); + + // Recovery called on each retry attempt (not the final one which terminates) + assertEquals(maxRetries, recoveryCount.get(), "Recovery callback called once per non-terminal retry"); + } } diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java index 99dc2b8ddb69b..1a639f39ff737 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java @@ -897,26 +897,24 @@ private Mono sendFluxInternal(Flux messages, new IllegalStateException(String.format(INVALID_OPERATION_DISPOSED_SENDER, "sendMessage"))); } - final Mono> batchList = getSendLink("send-batches").flatMap(link -> { - lastSendLink.set(link); - return link.getLinkSize().flatMap(size -> { + // Apply retry+recovery only to link acquisition. Keeping messages.collect() outside the + // retry boundary avoids re-subscribing the user-provided Flux on each retry attempt, + // which could duplicate side-effects or re-consume a hot publisher. + final Mono linkWithRecovery + = RetryUtil.withRetryAndRecovery(getSendLink("send-batches").doOnNext(link -> lastSendLink.set(link)), + retryOptions, "Failed to acquire send link for batch collection." + entityId(), + recoveryKind -> performRecovery(recoveryKind, "sendFlux-link")); + + final Mono> batchListMono + = linkWithRecovery.flatMap(link -> link.getLinkSize().flatMap(size -> { final int batchSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; final CreateMessageBatchOptions batchOptions = new CreateMessageBatchOptions().setMaximumSizeInBytes(batchSize); return messages.collect( new AmqpMessageCollector(isV2, batchOptions, 1, link::getErrorContext, tracer, messageSerializer)); - }); - }); - - // Wrap only the link-acquisition+batch-collection phase with retry+recovery. - // Individual batch sends are already retried inside sendBatchInternal, so wrapping the - // entire sendOperation (which includes messages.collect()) would cause the user-provided - // Flux to be re-subscribed on each retry and could duplicate batches that already sent. - final Mono> batchListWithRecovery = RetryUtil.withRetryAndRecovery(batchList, - retryOptions, "Failed to acquire send link for batch collection." + entityId(), - recoveryKind -> performRecovery(recoveryKind, "sendFlux-link")); + })); - final Mono sendOperation = batchListWithRecovery.flatMap(list -> Flux.fromIterable(list) + final Mono sendOperation = batchListMono.flatMap(list -> Flux.fromIterable(list) .flatMap(batch -> sendBatchInternal(batch, transactionContext)) .then() .doOnError(error -> logger.error("Error sending batch.", error))); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java index 147524ab1c7b4..d3a715e9976b0 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java @@ -144,6 +144,7 @@ private Mono acquireIntern(String sessionId) { .addKeyValue(ENTITY_PATH_KEY, entityPath) .log("Connection-level error acquiring session, forcing connection recovery.", t); connectionCacheWrapper.forceCloseConnection(); + return Mono.delay(Duration.ZERO); } if (isTimeoutError(t)) { logger.atVerbose() diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java index 0a111a3057956..0335fbf6b9e9c 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java @@ -304,8 +304,8 @@ Mono getActiveLink() { } else if (failure instanceof AmqpException && ((AmqpException) failure).getErrorCondition() == AmqpErrorCondition.TIMEOUT_ERROR) { return Mono.delay(Duration.ZERO); - } else if (kind == RecoveryKind.LINK) { - // Link-level error — retry to get a fresh link. + } else if (kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION) { + // Link or connection-level error — retry to acquire a fresh link (or connection). return Mono.delay(Duration.ZERO); } else { final long id = System.nanoTime(); From 79c52f9ccf7cbe60f9dbc4f777577fcf51f32c8f Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 13:46:10 -0700 Subject: [PATCH 05/39] fix(review): address third Copilot review on tiered recovery - T8: RetryUtil - clamp final jittered delay to maxDelay so retryOptions are consistently respected. Previously jitter was applied after the pre-jitter cap which could produce a delay exceeding retryOptions.getMaxDelay(). Also cap baseDelay to maxDelay in FIXED mode (FIXED previously used baseDelay without checking against maxDelay, unlike the EXPONENTIAL path). - T9: ServiceBusReceiverAsyncClient - fix misleading log message in the LINK/ CONNECTION recovery callback. The error handler on connectionProcessor.subscribe fires only when obtaining the connection fails (not when removeSession fails, since it returns a boolean). Renamed to "Error obtaining connection during {} recovery." Also log the boolean result of removeSession at VERBOSE level to confirm whether a stale session was actually evicted. --- .../azure/core/amqp/implementation/RetryUtil.java | 7 +++++-- .../servicebus/ServiceBusReceiverAsyncClient.java | 13 ++++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java index a590fe3c034c2..9476ea51882fc 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java @@ -221,13 +221,16 @@ static Retry createRetryWithRecovery(AmqpRetryOptions options, Consumer 0 ? maxDelay : baseDelay; } else { long millis = baseDelay.toMillis() * (1L << Math.min(attempt, 30)); delay = Duration.ofMillis(Math.min(millis, maxDelay.toMillis())); } final double jitter = 1.0 + (ThreadLocalRandom.current().nextDouble() * 2 - 1) * JITTER_FACTOR; - final Duration jitteredDelay = Duration.ofMillis((long) (delay.toMillis() * jitter)); + // Clamp the final jittered delay to maxDelay so retryOptions are consistently respected. + final Duration jitteredDelay + = Duration.ofMillis(Math.min((long) (delay.toMillis() * jitter), maxDelay.toMillis())); return Mono.delay(jitteredDelay).thenReturn(attempt); })); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java index 8096f73c6c6d3..21d0753618d2e 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java @@ -1749,10 +1749,17 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { // For LINK errors during link creation, the session hosting the link may be stale. // Ask the connection to remove it so the next retry creates a fresh session + link. // The entityPath is the session name used by createReceiveLink(). - connectionProcessor.subscribe(connection -> connection.removeSession(entityPath), - error -> LOGGER.atWarning() + // Note: the error handler fires only if obtaining the connection fails, not if removeSession fails + // (removeSession returns a boolean and never propagates an error into the reactive stream). + connectionProcessor.subscribe(connection -> { + final boolean removed = connection.removeSession(entityPath); + LOGGER.atVerbose() .addKeyValue(LINK_NAME_KEY, linkName) - .log("Error removing stale session during LINK recovery.", error)); + .addKeyValue("sessionRemoved", removed) + .log("Attempted stale session removal during {} recovery.", recoveryKind); + }, error -> LOGGER.atWarning() + .addKeyValue(LINK_NAME_KEY, linkName) + .log("Error obtaining connection during {} recovery.", recoveryKind, error)); } if (recoveryKind == RecoveryKind.CONNECTION) { connectionCacheWrapper.forceCloseConnection(); From 1fec9e58ed368fd2eac9a645b9c2019724049a33 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 14:11:58 -0700 Subject: [PATCH 06/39] fix(servicebus): make link/connection force-close non-blocking in recovery T10: In ServiceBusSenderAsyncClient.performRecovery(), replace link.dispose() with link.closeAsync().subscribe(...). ReactorSender.dispose() calls closeAsync().block(tryTimeout), which blocks the Reactor thread when invoked from a recovery callback on a non-blocking scheduler. T11: In ReactorConnectionCache.forceCloseConnection(), replace connection.dispose() with a non-blocking equivalent: set a new forceInvalidate AtomicBoolean flag before starting connection.closeAsync().subscribe(...). The cacheInvalidateIf predicate now checks forceInvalidate in addition to isDisposed(), ensuring the cache is invalidated synchronously (by the flag) while the close handshake completes asynchronously. ReactorConnection.dispose() has the same blocking pattern. T12: Update comment in RetryUtil.createRetryWithRecovery() to remove the misleading claim that the quick-retry path matches Go's ResetAttempts(). The Java implementation uses a didQuickRetry flag only (no attempt counter reset); subsequent retries continue standard exponential backoff from the running count. --- .../implementation/ReactorConnectionCache.java | 18 +++++++++++++----- .../core/amqp/implementation/RetryUtil.java | 4 +++- .../ServiceBusSenderAsyncClient.java | 10 ++++++++-- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java index 248877d56563d..94f3179e6d8f4 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java @@ -18,6 +18,7 @@ import java.util.Objects; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; @@ -48,6 +49,7 @@ public final class ReactorConnectionCache implement // any dependent type; instead, the dependent type must acquire Connection only through the cache route, // i.e., by subscribing to 'createOrGetCachedConnection' via 'get()' getter. private volatile T currentConnection; + private final AtomicBoolean forceInvalidate = new AtomicBoolean(false); private final State state = new State(); /** @@ -112,7 +114,7 @@ public ReactorConnectionCache(Supplier connectionSupplier, String fullyQualif sink.next(connection); } }).cacheInvalidateIf(c -> { - if (c.isDisposed()) { + if (c.isDisposed() || forceInvalidate.compareAndSet(true, false)) { withConnectionId(logger, c.getId()).log("The connection is closed, requesting a new connection."); return true; } else { @@ -190,10 +192,16 @@ public void forceCloseConnection() { if (connection != null && !connection.isDisposed()) { withConnectionId(logger, connection.getId()) .log("Force-closing connection for recovery. Next get() will create a fresh connection."); - // Call dispose() rather than closeAsync() so that isDisposed() returns true synchronously. - // This ensures cacheInvalidateIf immediately invalidates the cached reference on the next - // get() call, rather than waiting for the async close handshake to complete. - connection.dispose(); + // Set forceInvalidate before starting async close so that cacheInvalidateIf immediately + // invalidates this connection on the next get() call, without blocking the caller + // while the AMQP close handshake completes. ReactorConnection.dispose() calls + // closeAsync().block(), which is illegal on a non-blocking Reactor thread. + forceInvalidate.set(true); + connection.closeAsync() + .subscribe(null, + error -> logger.atVerbose() + .addKeyValue(CONNECTION_ID_KEY, connection.getId()) + .log("Error during async connection force-close.", error)); } } diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java index 9476ea51882fc..73175615fbc4c 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java @@ -212,7 +212,9 @@ static Retry createRetryWithRecovery(AmqpRetryOptions options, Consumer logger.atVerbose() + .addKeyValue(ENTITY_PATH_KEY, entityName) + .log("Error closing stale send link during recovery.", error)); } linkName.set(null); From d4c64fd99edeefb9f328ca5035e240c40ea59929 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 14:26:53 -0700 Subject: [PATCH 07/39] fix(amqp): prevent NONE failure from consuming quick-retry flag T13: In RetryUtil.createRetryWithRecovery(), the condition if (!didQuickRetry.getAndSet(true) && (kind == LINK || kind == CONNECTION)) evaluated getAndSet(true) unconditionally before the kind check. When the first retryable failure was NONE (e.g. SERVER_BUSY_ERROR, TimeoutException), the flag was set to true even though no quick retry occurred. The subsequent first LINK/CONNECTION failure therefore skipped the quick-retry optimization. Fix: reverse the operand order so the kind check comes first. Java short-circuit evaluation now prevents getAndSet(true) from being called on NONE/FATAL failures: if ((kind == LINK || kind == CONNECTION) && !didQuickRetry.getAndSet(true)) Added RetryUtilTest.createRetryWithRecovery_noneFailureBeforeLinkPreservesQuickRetry to directly exercise this scenario. --- .../core/amqp/implementation/RetryUtil.java | 4 +- .../amqp/implementation/RetryUtilTest.java | 42 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java index 73175615fbc4c..66b80924268e3 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java @@ -215,7 +215,9 @@ static Retry createRetryWithRecovery(AmqpRetryOptions options, Consumer recoveries = new ArrayList<>(); + final Retry retry = RetryUtil.createRetryWithRecovery(options, recoveries::add); + final AtomicInteger attempt = new AtomicInteger(); + final Mono source = Mono.defer(() -> { + switch (attempt.getAndIncrement()) { + case 0: + // NONE kind — server-busy; should not consume the quick-retry flag. + return Mono.error(new AmqpException(true, AmqpErrorCondition.SERVER_BUSY_ERROR, "busy", null)); + + case 1: + // LINK kind — first occurrence; flag must still be available → quick-retry fires. + return Mono.error(new AmqpException(true, AmqpErrorCondition.LINK_DETACH_FORCED, "detach", null)); + + default: + return Mono.just(99); + } + }); + + // Act & Assert — virtual time to skip the SERVER_BUSY backoff; LINK quick-retry fires without delay. + StepVerifier.withVirtualTime(() -> source.retryWhen(retry)) + .expectSubscription() + .thenAwait(Duration.ofMinutes(1)) + .expectNext(99) + .expectComplete() + .verify(Duration.ofSeconds(5)); + + // NONE failures do not invoke recovery; only LINK does. + assertEquals(1, recoveries.size(), "Only the LINK failure should invoke recovery"); + assertEquals(RecoveryKind.LINK, recoveries.get(0), + "Recovery callback must be called with LINK kind for the detach error"); + } } From 52d4435b62b70ce59bc1c33674facd19f0a60b8b Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 14:42:15 -0700 Subject: [PATCH 08/39] fix(amqp): classify disposed-link IllegalStateException as LINK recovery T14: RecoveryKind.classify() previously returned FATAL for all non-AMQP exceptions, including IllegalStateException thrown by ReactorSender.send() when the link is disposed: 'connectionId[%s] linkName[%s] Cannot publish message when disposed.' 'connectionId[%s] linkName[%s] Cannot publish data batch when disposed.' These exceptions signal link staleness (a race between link.closeAsync() and an in-flight send), not a permanent application error. They should trigger LINK recovery so the retry path creates a fresh link. Added explicit check before the FATAL fallthrough: if the exception is an IllegalStateException with 'disposed' in the message, return LINK. Added three tests in RecoveryKindTest: - illegalStateExceptionDisposedMessageReturnsLink - illegalStateExceptionDisposedDataBatchReturnsLink - illegalStateExceptionUnrelatedToDisposedReturnsFatal --- .../amqp/implementation/RecoveryKind.java | 12 +++++++++++ .../amqp/implementation/RecoveryKindTest.java | 20 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java index 31ccc5be35d48..ff0690f9c4f70 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java @@ -141,6 +141,18 @@ public static RecoveryKind classify(Throwable error) { return LINK; } + // IllegalStateException thrown by a disposed ReactorSender (e.g., "Cannot publish + // message when disposed." or "Cannot publish data batch when disposed."). This is + // a link-staleness signal: the link was closed (possibly by a concurrent recovery + // path) before the in-flight send could complete. LINK recovery creates a fresh + // link on the next retry. + if (error instanceof IllegalStateException) { + final String msg = error.getMessage(); + if (msg != null && msg.contains("disposed")) { + return LINK; + } + } + // Unknown non-AMQP errors — treat as fatal (don't retry application or SDK bugs). // The Go SDK defaults to CONNECTION for unknown errors, but those are AMQP-layer // errors (io.EOF, net.Error). Java's non-AMQP exceptions (e.g., AzureException, diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java index 7c36e7d61d9f6..4655fc72ddb72 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java @@ -184,4 +184,24 @@ void resourceLimitExceededReturnsNone() { = new AmqpException(true, AmqpErrorCondition.RESOURCE_LIMIT_EXCEEDED, "resource limit", null); assertEquals(RecoveryKind.NONE, RecoveryKind.classify(error)); } + + @Test + void illegalStateExceptionDisposedMessageReturnsLink() { + // Matches ReactorSender.send() message: "connectionId[%s] linkName[%s] Cannot publish message when disposed." + assertEquals(RecoveryKind.LINK, RecoveryKind.classify( + new IllegalStateException("connectionId[abc] linkName[xyz] Cannot publish message when disposed."))); + } + + @Test + void illegalStateExceptionDisposedDataBatchReturnsLink() { + // Matches ReactorSender.send(List) message: "connectionId[%s] linkName[%s] Cannot publish data batch when disposed." + assertEquals(RecoveryKind.LINK, RecoveryKind.classify( + new IllegalStateException("connectionId[abc] linkName[xyz] Cannot publish data batch when disposed."))); + } + + @Test + void illegalStateExceptionUnrelatedToDisposedReturnsFatal() { + // Non-disposed IllegalStateException must remain FATAL (genuine application or SDK bug). + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(new IllegalStateException("some unexpected state"))); + } } From ce10aa262f6cd21c48ea0670f7e1383394523a84 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 15:03:36 -0700 Subject: [PATCH 09/39] fix(amqp): narrow disposed-ISE match to prevent tier misclassification T15: The 'disposed' substring check in RecoveryKind.classify() was too broad. 'Connection is disposed. Cannot get management instance.' would also match, incorrectly returning LINK instead of FATAL for connection-level errors. Narrowed to require both 'Cannot publish' AND 'disposed' in the message, which precisely matches ReactorSender's two disposal messages and nothing else. Added illegalStateExceptionConnectionDisposedReturnsFatal test to RecoveryKindTest to pin the regression. --- .../com/azure/core/amqp/implementation/RecoveryKind.java | 4 +++- .../azure/core/amqp/implementation/RecoveryKindTest.java | 8 ++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java index ff0690f9c4f70..2fb64031fc1bf 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java @@ -146,9 +146,11 @@ public static RecoveryKind classify(Throwable error) { // a link-staleness signal: the link was closed (possibly by a concurrent recovery // path) before the in-flight send could complete. LINK recovery creates a fresh // link on the next retry. + // Match both "Cannot publish" and "disposed" to avoid misclassifying unrelated + // disposal signals (e.g., "Connection is disposed. Cannot get management instance."). if (error instanceof IllegalStateException) { final String msg = error.getMessage(); - if (msg != null && msg.contains("disposed")) { + if (msg != null && msg.contains("Cannot publish") && msg.contains("disposed")) { return LINK; } } diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java index 4655fc72ddb72..d123a2c8963c7 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java @@ -204,4 +204,12 @@ void illegalStateExceptionUnrelatedToDisposedReturnsFatal() { // Non-disposed IllegalStateException must remain FATAL (genuine application or SDK bug). assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(new IllegalStateException("some unexpected state"))); } + + @Test + void illegalStateExceptionConnectionDisposedReturnsFatal() { + // "Connection is disposed. Cannot get management instance." contains "disposed" but NOT + // "Cannot publish" — must remain FATAL to avoid misclassifying connection-level disposal. + assertEquals(RecoveryKind.FATAL, RecoveryKind + .classify(new IllegalStateException("Connection is disposed. Cannot get management instance."))); + } } From 5448c974d0d6fffaec64ca3713a2d49d9d01199b Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 15:43:11 -0700 Subject: [PATCH 10/39] fix(amqp): clarify virtual-time reason in retry tests T17: Three RetryUtilTest comments claimed virtual time was needed because of a 'SERVER_BUSY backoff delay'. The actual reason is that createRetryWithRecovery() unconditionally adds SERVER_BUSY_WAIT_TIME (4 s) to the base delay for every retry, regardless of error type (line 184: baseDelay = options.getDelay() + SERVER_BUSY_WAIT_TIME). Updated all three comments to reflect the actual mechanism so future maintainers do not infer incorrect retry semantics. --- .../azure/core/amqp/implementation/RetryUtilTest.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java index d7a12816e51fc..06bdf33ab2114 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java @@ -247,7 +247,9 @@ void createRetryWithRecovery_linkErrorInvokesRecoveryAndRetries() { return Mono.just(42); }); - // Act & Assert — use virtual time because the second retry applies a SERVER_BUSY backoff delay. + // Act & Assert — use virtual time because the base retry delay unconditionally includes + // SERVER_BUSY_WAIT_TIME (4 s), regardless of error type — the cumulative + // wait would exceed real-time test limits. StepVerifier.withVirtualTime(() -> source.retryWhen(retry)) .expectSubscription() .thenAwait(Duration.ofMinutes(1)) @@ -297,7 +299,9 @@ void createRetryWithRecovery_exhaustedRetriesTerminateWithError() { final AmqpException transientError = new AmqpException(true, AmqpErrorCondition.LINK_DETACH_FORCED, "detach", null); - // Act & Assert — use virtual time because the second retry applies a SERVER_BUSY backoff delay. + // Act & Assert — use virtual time because the base retry delay unconditionally includes + // SERVER_BUSY_WAIT_TIME (4 s), regardless of error type — the cumulative + // wait would exceed real-time test limits. StepVerifier.withVirtualTime(() -> Mono.error(transientError).retryWhen(retry)) .expectSubscription() .thenAwait(Duration.ofMinutes(1)) @@ -336,7 +340,8 @@ void createRetryWithRecovery_noneFailureBeforeLinkPreservesQuickRetry() { } }); - // Act & Assert — virtual time to skip the SERVER_BUSY backoff; LINK quick-retry fires without delay. + // Act & Assert — virtual time because the base delay logic unconditionally adds SERVER_BUSY_WAIT_TIME + // (4 s) to every retry; the first LINK error uses the quick-retry path (no delay). StepVerifier.withVirtualTime(() -> source.retryWhen(retry)) .expectSubscription() .thenAwait(Duration.ofMinutes(1)) From 4bd93bb8dfb60f68c46b891e2cc7abcc2424cbba Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 15:59:21 -0700 Subject: [PATCH 11/39] fix(amqp): tie forceCloseConnection invalidation to specific connection id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T18: The shared AtomicBoolean forceInvalidate flag was not bound to a specific connection instance. Under concurrency, if the cache refreshed and created a new connection C' between forceCloseConnection() setting the flag and the next cacheInvalidateIf() check, C' would be spuriously invalidated — causing unnecessary connection churn. Replace AtomicBoolean with AtomicReference holding the connection ID to be force-invalidated. cacheInvalidateIf() now only consumes the flag when the cached connection's ID exactly matches the stored target; a fresh connection with a different ID passes through the cache unaffected. --- .../ReactorConnectionCache.java | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java index 94f3179e6d8f4..a7affe9284cb7 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java @@ -18,7 +18,6 @@ import java.util.Objects; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; @@ -49,7 +48,10 @@ public final class ReactorConnectionCache implement // any dependent type; instead, the dependent type must acquire Connection only through the cache route, // i.e., by subscribing to 'createOrGetCachedConnection' via 'get()' getter. private volatile T currentConnection; - private final AtomicBoolean forceInvalidate = new AtomicBoolean(false); + // Holds the ID of the connection that forceCloseConnection() asked to force-invalidate. + // Only the connection whose getId() matches this value will be invalidated by cacheInvalidateIf; + // a freshly created connection with a different ID is never accidentally invalidated. + private final AtomicReference forceInvalidateConnectionId = new AtomicReference<>(null); private final State state = new State(); /** @@ -114,13 +116,24 @@ public ReactorConnectionCache(Supplier connectionSupplier, String fullyQualif sink.next(connection); } }).cacheInvalidateIf(c -> { - if (c.isDisposed() || forceInvalidate.compareAndSet(true, false)) { + if (c.isDisposed()) { + // Connection disposed for any reason. Clean up the force-invalidate marker if it + // was targeting this connection so it is not accidentally consumed by a future + // connection that happens to have the same ID. + forceInvalidateConnectionId.compareAndSet(c.getId(), null); + withConnectionId(logger, c.getId()).log("The connection is closed, requesting a new connection."); + return true; + } + final String targetId = forceInvalidateConnectionId.get(); + if (targetId != null + && targetId.equals(c.getId()) + && forceInvalidateConnectionId.compareAndSet(targetId, null)) { + // forceCloseConnection() asked to invalidate exactly this connection. withConnectionId(logger, c.getId()).log("The connection is closed, requesting a new connection."); return true; - } else { - // Emit cached connection. - return false; } + // No forced invalidation targeted this connection — emit it from cache. + return false; }); } @@ -196,7 +209,7 @@ public void forceCloseConnection() { // invalidates this connection on the next get() call, without blocking the caller // while the AMQP close handshake completes. ReactorConnection.dispose() calls // closeAsync().block(), which is illegal on a non-blocking Reactor thread. - forceInvalidate.set(true); + forceInvalidateConnectionId.set(connection.getId()); connection.closeAsync() .subscribe(null, error -> logger.atVerbose() From 2e80396bdd2efb18c2d7a10a631726b470ef9ce8 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 16:28:19 -0700 Subject: [PATCH 12/39] fix(amqp): rename test methods to camelCase for checkstyle compliance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI Build Analyze failed with checkstyle MethodName violations — the pattern (?=^[a-z][a-zA-Z0-9]*$) forbids underscores. Renamed all 5 createRetryWithRecovery_* test methods in RetryUtilTest to camelCase. --- .../azure/core/amqp/implementation/RetryUtilTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java index 06bdf33ab2114..fd48b0ecfddc9 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java @@ -214,7 +214,7 @@ void retryFilter(Throwable throwable, boolean expected) { * FATAL errors must not be retried and must not invoke the recovery callback. */ @Test - void createRetryWithRecovery_fatalErrorTerminatesImmediately() { + void createRetryWithRecoveryFatalErrorTerminatesImmediately() { // Arrange final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(3).setDelay(Duration.ofMillis(100)); final AtomicInteger recoveryCount = new AtomicInteger(); @@ -234,7 +234,7 @@ void createRetryWithRecovery_fatalErrorTerminatesImmediately() { * the quick-retry path (no backoff delay). */ @Test - void createRetryWithRecovery_linkErrorInvokesRecoveryAndRetries() { + void createRetryWithRecoveryLinkErrorInvokesRecoveryAndRetries() { // Arrange final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(3).setDelay(Duration.ofMillis(100)); final List recoveries = new ArrayList<>(); @@ -265,7 +265,7 @@ void createRetryWithRecovery_linkErrorInvokesRecoveryAndRetries() { * CONNECTION errors must invoke the recovery callback with CONNECTION kind. */ @Test - void createRetryWithRecovery_connectionErrorInvokesRecovery() { + void createRetryWithRecoveryConnectionErrorInvokesRecovery() { // Arrange final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(2).setDelay(Duration.ofMillis(100)); final AtomicReference capturedKind = new AtomicReference<>(); @@ -289,7 +289,7 @@ void createRetryWithRecovery_connectionErrorInvokesRecovery() { * After the retry budget is exhausted the error must propagate without further retries. */ @Test - void createRetryWithRecovery_exhaustedRetriesTerminateWithError() { + void createRetryWithRecoveryExhaustedRetriesTerminateWithError() { // Arrange final int maxRetries = 2; final AmqpRetryOptions options @@ -319,7 +319,7 @@ void createRetryWithRecovery_exhaustedRetriesTerminateWithError() { * unconditionally; this test verifies the kind check comes first. */ @Test - void createRetryWithRecovery_noneFailureBeforeLinkPreservesQuickRetry() { + void createRetryWithRecoveryNoneFailureBeforeLinkPreservesQuickRetry() { // Arrange final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(3).setDelay(Duration.ofMillis(100)); final List recoveries = new ArrayList<>(); From 1860dd38b6e009f08a37399871096d5cb66f3cf8 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 18 Mar 2026 16:40:14 -0700 Subject: [PATCH 13/39] fix(amqp): use distinct log message for force-invalidation path The force-invalidation path in cacheInvalidateIf used the same log message as the disposed-connection path ('The connection is closed, requesting a new connection.'), which was misleading because the connection may not yet be disposed when a force-close is requested. Changed the force path message to 'Forcing connection close, requesting a new connection.' --- .../azure/core/amqp/implementation/ReactorConnectionCache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java index a7affe9284cb7..20aa2f90c7aac 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java @@ -129,7 +129,7 @@ public ReactorConnectionCache(Supplier connectionSupplier, String fullyQualif && targetId.equals(c.getId()) && forceInvalidateConnectionId.compareAndSet(targetId, null)) { // forceCloseConnection() asked to invalidate exactly this connection. - withConnectionId(logger, c.getId()).log("The connection is closed, requesting a new connection."); + withConnectionId(logger, c.getId()).log("Forcing connection close, requesting a new connection."); return true; } // No forced invalidation targeted this connection — emit it from cache. From bf2b5605b8265be9cbab99ade4e06a2bd9c9806c Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Thu, 19 Mar 2026 09:49:37 -0700 Subject: [PATCH 14/39] fix(amqp): guard backoff overflow and normalize RecoveryKind message matching --- .../com/azure/core/amqp/implementation/RecoveryKind.java | 8 ++++++-- .../com/azure/core/amqp/implementation/RetryUtil.java | 7 ++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java index 2fb64031fc1bf..df3dc4c1d651f 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java @@ -6,6 +6,7 @@ import com.azure.core.amqp.exception.AmqpErrorCondition; import com.azure.core.amqp.exception.AmqpException; +import java.util.Locale; import java.util.concurrent.TimeoutException; /** @@ -150,8 +151,11 @@ public static RecoveryKind classify(Throwable error) { // disposal signals (e.g., "Connection is disposed. Cannot get management instance."). if (error instanceof IllegalStateException) { final String msg = error.getMessage(); - if (msg != null && msg.contains("Cannot publish") && msg.contains("disposed")) { - return LINK; + if (msg != null) { + final String normalizedMsg = msg.toLowerCase(Locale.ROOT); + if (normalizedMsg.contains("cannot publish") && normalizedMsg.contains("disposed")) { + return LINK; + } } } diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java index 66b80924268e3..40c33f030063e 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java @@ -228,7 +228,12 @@ static Retry createRetryWithRecovery(AmqpRetryOptions options, Consumer 0 ? maxDelay : baseDelay; } else { - long millis = baseDelay.toMillis() * (1L << Math.min(attempt, 30)); + final long multiplier = 1L << Math.min(attempt, 30); + final long baseMillis = baseDelay.toMillis(); + // Guard against overflow: if baseMillis * multiplier would exceed Long.MAX_VALUE, + // saturate to maxDelay (the clamp below would cap it there anyway). + final long millis + = baseMillis > Long.MAX_VALUE / multiplier ? maxDelay.toMillis() : baseMillis * multiplier; delay = Duration.ofMillis(Math.min(millis, maxDelay.toMillis())); } final double jitter = 1.0 + (ThreadLocalRandom.current().nextDouble() * 2 - 1) * JITTER_FACTOR; From e76c2d831284b4fa562d4e47e86dd62c22ae177d Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Thu, 19 Mar 2026 10:37:10 -0700 Subject: [PATCH 15/39] fix(servicebus): scope send-link recovery to per-operation reference, guard session disposal, improve recovery log context --- .../core/amqp/implementation/RetryUtil.java | 2 +- .../ServiceBusSenderAsyncClient.java | 24 ++++++++++--------- .../servicebus/ServiceBusSessionManager.java | 10 ++++---- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java index 40c33f030063e..d71ea5b064a6b 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java @@ -206,7 +206,7 @@ static Retry createRetryWithRecovery(AmqpRetryOptions options, Consumer linkName = new AtomicReference<>(); - private final AtomicReference lastSendLink = new AtomicReference<>(); private final AtomicBoolean isDisposed = new AtomicBoolean(); private final MessageSerializer messageSerializer; private final AmqpRetryOptions retryOptions; @@ -817,7 +816,6 @@ private Mono scheduleMessageInternal(ServiceBusMessage message, OffsetDate return tracer .traceScheduleMono("ServiceBus.scheduleMessage", getSendLinkWithRetry("schedule-message").flatMap(link -> { - lastSendLink.set(link); return link.getLinkSize().flatMap(size -> { final int maxSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; return connectionProcessor @@ -869,8 +867,9 @@ private Mono sendBatchInternal(ServiceBusMessageBatch batch, messages.add(message); }); + final AtomicReference operationLink = new AtomicReference<>(); final Mono sendMessage = getSendLink("send-batch").flatMap(link -> { - lastSendLink.set(link); + operationLink.set(link); if (transactionContext != null && transactionContext.getTransactionId() != null) { final TransactionalState deliveryState = new TransactionalState(); deliveryState.setTxnId(Binary.create(transactionContext.getTransactionId())); @@ -885,7 +884,7 @@ private Mono sendBatchInternal(ServiceBusMessageBatch batch, final String timeoutMessage = "Sending messages timed out. message-count:" + batch.getCount() + entityId(); final Mono withRetry = RetryUtil.withRetryAndRecovery(sendMessage, retryOptions, timeoutMessage, recoveryKind -> { - performRecovery(recoveryKind, "sendBatch"); + performRecovery(recoveryKind, "sendBatch", operationLink); }).onErrorMap(this::mapError); return instrumentation.instrumentSendBatch("ServiceBus.send", withRetry, batch.getMessages()); } @@ -900,10 +899,11 @@ private Mono sendFluxInternal(Flux messages, // Apply retry+recovery only to link acquisition. Keeping messages.collect() outside the // retry boundary avoids re-subscribing the user-provided Flux on each retry attempt, // which could duplicate side-effects or re-consume a hot publisher. + final AtomicReference operationLink = new AtomicReference<>(); final Mono linkWithRecovery - = RetryUtil.withRetryAndRecovery(getSendLink("send-batches").doOnNext(link -> lastSendLink.set(link)), - retryOptions, "Failed to acquire send link for batch collection." + entityId(), - recoveryKind -> performRecovery(recoveryKind, "sendFlux-link")); + = RetryUtil.withRetryAndRecovery(getSendLink("send-batches").doOnNext(operationLink::set), retryOptions, + "Failed to acquire send link for batch collection." + entityId(), + recoveryKind -> performRecovery(recoveryKind, "sendFlux-link", operationLink)); final Mono> batchListMono = linkWithRecovery.flatMap(link -> link.getLinkSize().flatMap(size -> { @@ -952,7 +952,7 @@ private Mono getSendLink(String callSite) { private Mono getSendLinkWithRetry(String callSite) { return RetryUtil.withRetryAndRecovery(getSendLink(callSite), retryOptions, String.format(retryGetLinkErrorMessageFormat, callSite), recoveryKind -> { - performRecovery(recoveryKind, "getSendLink-" + callSite); + performRecovery(recoveryKind, "getSendLink-" + callSite, null); }); } @@ -972,7 +972,7 @@ private Throwable mapError(Throwable throwable) { *

This matches the Go SDK's RecoverIfNeeded() and the .NET SDK's * FaultTolerantAmqpObject pattern.

*/ - private void performRecovery(RecoveryKind recoveryKind, String callSite) { + private void performRecovery(RecoveryKind recoveryKind, String callSite, AtomicReference linkRef) { if (recoveryKind == RecoveryKind.NONE || recoveryKind == RecoveryKind.FATAL) { return; } @@ -983,10 +983,12 @@ private void performRecovery(RecoveryKind recoveryKind, String callSite) { .addKeyValue("callSite", callSite) .log("Performing {} recovery before retry.", recoveryKind); - // Start async close of the cached send link so the next retry creates a fresh one. + // Start async close of the operation-scoped send link so the next retry creates a fresh one. + // Using a per-operation AtomicReference (not a class-level field) prevents concurrent send + // operations from accidentally closing each other's links. // Use closeAsync() rather than dispose() to avoid blocking the Reactor thread; ReactorSender // dispose() calls closeAsync().block(tryTimeout), which is illegal on a non-blocking scheduler. - final AmqpSendLink link = lastSendLink.getAndSet(null); + final AmqpSendLink link = linkRef != null ? linkRef.getAndSet(null) : null; if (link != null) { link.closeAsync() .subscribe(null, diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java index 0335fbf6b9e9c..5ac863ebb57d6 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java @@ -289,6 +289,11 @@ Mono getActiveLink() { .addKeyValue("recoveryKind", kind) .log("Error occurred while getting unnamed session.", failure); + if (isDisposed.get()) { + return Mono.error( + new AmqpException(false, "SessionManager is already disposed.", failure, getErrorContext())); + } + if (kind == RecoveryKind.CONNECTION) { LOGGER.atWarning() .addKeyValue(ENTITY_PATH_KEY, entityPath) @@ -296,10 +301,7 @@ Mono getActiveLink() { connectionCacheWrapper.forceCloseConnection(); } - if (isDisposed.get()) { - return Mono.error( - new AmqpException(false, "SessionManager is already disposed.", failure, getErrorContext())); - } else if (failure instanceof TimeoutException) { + if (failure instanceof TimeoutException) { return Mono.delay(Duration.ZERO); } else if (failure instanceof AmqpException && ((AmqpException) failure).getErrorCondition() == AmqpErrorCondition.TIMEOUT_ERROR) { From 3eea3b2f9b6c73e560e144d91343a3cfca197cbb Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Thu, 19 Mar 2026 12:09:31 -0700 Subject: [PATCH 16/39] fix(servicebus): include exception in connection-recovery warning log --- .../azure/messaging/servicebus/ServiceBusSessionManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java index 5ac863ebb57d6..59971f46fc80a 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java @@ -297,7 +297,7 @@ Mono getActiveLink() { if (kind == RecoveryKind.CONNECTION) { LOGGER.atWarning() .addKeyValue(ENTITY_PATH_KEY, entityPath) - .log("Connection-level error in session manager, forcing connection recovery."); + .log("Connection-level error in session manager, forcing connection recovery.", failure); connectionCacheWrapper.forceCloseConnection(); } From 1f9bf9231f57c33ddb42cf7dba37c1902070c9b9 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Thu, 19 Mar 2026 13:51:54 -0700 Subject: [PATCH 17/39] fix(ci): move azure-core-amqp source comment to correct entry --- sdk/servicebus/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/servicebus/ci.yml b/sdk/servicebus/ci.yml index b957c80a24d4b..49564ad549aa5 100644 --- a/sdk/servicebus/ci.yml +++ b/sdk/servicebus/ci.yml @@ -61,6 +61,6 @@ extends: # required by the above perf libraries - name: perf-test-core groupId: com.azure - # Build azure-core-amqp from source (needed for RecoveryKind, tiered retry) - name: azure-core-amqp groupId: com.azure + # Build azure-core-amqp from source (needed for RecoveryKind, tiered retry) From 0c6cb7a092edaf16f3f280a22bb9f68fc5af81b1 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Thu, 2 Apr 2026 10:27:32 -0700 Subject: [PATCH 18/39] refactor: address reviewer feedback on tiered recovery - Rename forceCloseConnection() to invalidateConnection() per anuchandy: mark-only, close deferred to cacheInvalidateIf via closeConnection() - Remove withRetryAndRecovery/createRetryWithRecovery per conniey/anuchandy: use onErrorResume + standard withRetry at each call site - Receiver: replace fire-and-forget subscribe() with reactive flatMap chain matching anuchandy's suggested pattern - Add asRetriable() bridge: wraps non-AMQP errors classified as LINK/CONNECTION as transient AmqpException so standard retry filter accepts them - Consolidate delay logic: single retry spec via createRetry (no duplicate backoff calculation) - Update ConnectionCacheWrapper, SessionAcquirer, SessionManager references - Remove 5 createRetryWithRecovery tests (method deleted) - Tests: 643 (core-amqp) + 941 (servicebus) = 1584, 0 failures --- .../ReactorConnectionCache.java | 35 ++-- .../core/amqp/implementation/RetryUtil.java | 114 ------------- .../amqp/implementation/RetryUtilTest.java | 151 ------------------ .../servicebus/ConnectionCacheWrapper.java | 10 +- .../ServiceBusReceiverAsyncClient.java | 63 +++++--- .../ServiceBusSenderAsyncClient.java | 100 +++++++----- .../servicebus/ServiceBusSessionAcquirer.java | 2 +- .../servicebus/ServiceBusSessionManager.java | 2 +- 8 files changed, 124 insertions(+), 353 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java index 20aa2f90c7aac..f153b4da2f46c 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java @@ -48,7 +48,7 @@ public final class ReactorConnectionCache implement // any dependent type; instead, the dependent type must acquire Connection only through the cache route, // i.e., by subscribing to 'createOrGetCachedConnection' via 'get()' getter. private volatile T currentConnection; - // Holds the ID of the connection that forceCloseConnection() asked to force-invalidate. + // Holds the ID of the connection that invalidateConnection() asked to force-invalidate. // Only the connection whose getId() matches this value will be invalidated by cacheInvalidateIf; // a freshly created connection with a different ID is never accidentally invalidated. private final AtomicReference forceInvalidateConnectionId = new AtomicReference<>(null); @@ -128,8 +128,10 @@ public ReactorConnectionCache(Supplier connectionSupplier, String fullyQualif if (targetId != null && targetId.equals(c.getId()) && forceInvalidateConnectionId.compareAndSet(targetId, null)) { - // forceCloseConnection() asked to invalidate exactly this connection. - withConnectionId(logger, c.getId()).log("Forcing connection close, requesting a new connection."); + // invalidateConnection() marked this connection dirty. + // Close it here — the cache owns the connection lifecycle. + withConnectionId(logger, c.getId()).log("Force-invalidating and closing connection for recovery."); + closeConnection(c, logger, "Force-invalidated for recovery."); return true; } // No forced invalidation targeted this connection — emit it from cache. @@ -188,11 +190,15 @@ public boolean isCurrentConnectionClosed() { } /** - * Closes the current cached connection (if any) so that the next {@link #get()} call creates - * a fresh connection. This is used for connection-level recovery when the current connection - * is in a stale state that the cache's normal error detection (via endpoint state signals) - * has not detected — for example, when intermediate infrastructure (load balancers, NAT gateways) - * is echoing AMQP heartbeats on behalf of a dead connection. + * Marks the current cached connection for invalidation so that the next {@link #get()} call + * closes it and creates a fresh connection. This is used for connection-level recovery when + * the current connection is in a stale state that the cache's normal error detection (via + * endpoint state signals) has not detected — for example, when intermediate infrastructure + * (load balancers, NAT gateways) is echoing AMQP heartbeats on behalf of a dead connection. + * + *

The actual close is deferred to the cache chain's {@code cacheInvalidateIf} predicate + * inside {@link #get()}, keeping the connection lifecycle local to the cache. This avoids + * the concurrency concern of two threads touching the cached connection simultaneously.

* *

This is modeled after the Go SDK's {@code Namespace.Recover()} which explicitly closes * the old connection and increments the connection revision.

@@ -200,21 +206,12 @@ public boolean isCurrentConnectionClosed() { *

This method is safe to call concurrently. If the connection is already closed or being * closed, this is a no-op.

*/ - public void forceCloseConnection() { + public void invalidateConnection() { final T connection = currentConnection; if (connection != null && !connection.isDisposed()) { withConnectionId(logger, connection.getId()) - .log("Force-closing connection for recovery. Next get() will create a fresh connection."); - // Set forceInvalidate before starting async close so that cacheInvalidateIf immediately - // invalidates this connection on the next get() call, without blocking the caller - // while the AMQP close handshake completes. ReactorConnection.dispose() calls - // closeAsync().block(), which is illegal on a non-blocking Reactor thread. + .log("Marking connection for force-invalidation. Next get() will close and replace it."); forceInvalidateConnectionId.set(connection.getId()); - connection.closeAsync() - .subscribe(null, - error -> logger.atVerbose() - .addKeyValue(CONNECTION_ID_KEY, connection.getId()) - .log("Error during async connection force-close.", error)); } } diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java index d71ea5b064a6b..6b6c5bd57cf6a 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RetryUtil.java @@ -16,10 +16,7 @@ import java.time.Duration; import java.util.Locale; -import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Consumer; /** * Helper class to help with retry policies. @@ -109,44 +106,6 @@ public static Mono withRetry(Mono source, AmqpRetryOptions retryOption return withRetry(source, retryOptions, timeoutMessage, false); } - /** - * Applies the retry policy with tiered recovery between attempts. Before each retry, - * the error is classified via {@link RecoveryKind#classify(Throwable)} and the recovery - * callback is invoked so the caller can close the appropriate resources (link or connection). - * - *

This matches the tiered recovery pattern used by the Go, .NET, Python, and JS SDKs.

- * - * @param Type of value in the {@link Mono}. - * @param source The publisher to apply the retry policy to. - * @param retryOptions A {@link AmqpRetryOptions}. - * @param errorMessage Text added to error logs. - * @param recoveryAction Called between retry attempts with the classified {@link RecoveryKind}. - * The caller should close the link (for {@link RecoveryKind#LINK}) or connection - * (for {@link RecoveryKind#CONNECTION}) so the next retry creates fresh resources. - * - * @return A publisher that returns the results of the {@link Mono} if any of the retry attempts - * are successful. Otherwise, propagates the last error. - */ - public static Mono withRetryAndRecovery(Mono source, AmqpRetryOptions retryOptions, String errorMessage, - Consumer recoveryAction) { - return withRetryAndRecovery(source, retryOptions, errorMessage, false, recoveryAction); - } - - /** - * Like {@link #withRetryAndRecovery(Mono, AmqpRetryOptions, String, Consumer)} but with an option to allow - * long-running operations that should not be subject to the per-attempt timeout. - * - * @param allowsLongOperation If true, the source Mono will not be wrapped with a per-attempt timeout. - */ - public static Mono withRetryAndRecovery(Mono source, AmqpRetryOptions retryOptions, String errorMessage, - boolean allowsLongOperation, Consumer recoveryAction) { - if (!allowsLongOperation) { - source = source.timeout(retryOptions.getTryTimeout()); - } - return source.retryWhen(createRetryWithRecovery(retryOptions, recoveryAction)) - .doOnError(error -> LOGGER.error(errorMessage, error)); - } - static Retry createRetry(AmqpRetryOptions options) { final Duration delay = options.getDelay().plus(SERVER_BUSY_WAIT_TIME); final RetryBackoffSpec retrySpec; @@ -171,77 +130,4 @@ static Retry createRetry(AmqpRetryOptions options) { || (error instanceof AmqpException && ((AmqpException) error).isTransient())); } - /** - * Creates a Reactor {@link Retry} spec that performs tiered recovery between retry attempts. - * Before each retry, the error is classified and the recovery callback is invoked. - * - *

Includes a quick-retry optimization matching the Go SDK: on the first LINK or CONNECTION - * error, the retry fires immediately (no backoff) since the error may come from a previously - * stale link and recovery has just created a fresh one.

- */ - static Retry createRetryWithRecovery(AmqpRetryOptions options, Consumer recoveryAction) { - final int maxRetries = options.getMaxRetries(); - final Duration baseDelay = options.getDelay().plus(SERVER_BUSY_WAIT_TIME); - final Duration maxDelay = options.getMaxDelay(); - final boolean isFixed = options.getMode() == com.azure.core.amqp.AmqpRetryMode.FIXED; - final AtomicBoolean didQuickRetry = new AtomicBoolean(false); - - return Retry.from(retrySignals -> retrySignals.flatMap(signal -> { - final Throwable failure = signal.failure(); - final long attempt = signal.totalRetriesInARow(); - final RecoveryKind kind = RecoveryKind.classify(failure); - - // FATAL errors — do not retry. - if (kind == RecoveryKind.FATAL) { - return Mono.error(failure); - } - - // Check retry budget. - if (attempt >= maxRetries) { - return Mono.error(failure); - } - - // Perform recovery before retry. - if (kind != RecoveryKind.NONE && recoveryAction != null) { - try { - recoveryAction.accept(kind); - } catch (Exception e) { - LOGGER.atWarning().addKeyValue("recoveryKind", kind).log("Recovery action failed.", e); - } - } - - // Quick retry: on the FIRST LINK/CONNECTION error, retry immediately (no backoff). - // Uses didQuickRetry flag to prevent repeated immediate retries under persistent - // errors — similar to the Go SDK's didQuickRetry pattern. Unlike Go's ResetAttempts(), - // the attempt counter is not reset here; subsequent retries continue with standard - // exponential backoff from the current attempt count. - // The kind check must come first: short-circuit evaluation prevents consuming the - // flag on NONE/FATAL failures where no quick-retry should be issued. - if ((kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION) && !didQuickRetry.getAndSet(true)) { - LOGGER.atInfo().log("Quick retry after {} recovery (first occurrence).", kind); - return Mono.just(attempt); - } - - // Standard backoff delay. - final Duration delay; - if (isFixed) { - // Cap baseDelay to maxDelay so FIXED mode respects retryOptions.getMaxDelay(). - delay = baseDelay.compareTo(maxDelay) > 0 ? maxDelay : baseDelay; - } else { - final long multiplier = 1L << Math.min(attempt, 30); - final long baseMillis = baseDelay.toMillis(); - // Guard against overflow: if baseMillis * multiplier would exceed Long.MAX_VALUE, - // saturate to maxDelay (the clamp below would cap it there anyway). - final long millis - = baseMillis > Long.MAX_VALUE / multiplier ? maxDelay.toMillis() : baseMillis * multiplier; - delay = Duration.ofMillis(Math.min(millis, maxDelay.toMillis())); - } - final double jitter = 1.0 + (ThreadLocalRandom.current().nextDouble() * 2 - 1) * JITTER_FACTOR; - // Clamp the final jittered delay to maxDelay so retryOptions are consistently respected. - final Duration jitteredDelay - = Duration.ofMillis(Math.min((long) (delay.toMillis() * jitter), maxDelay.toMillis())); - - return Mono.delay(jitteredDelay).thenReturn(attempt); - })); - } } diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java index fd48b0ecfddc9..b25d920b31f6f 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RetryUtilTest.java @@ -9,7 +9,6 @@ import com.azure.core.amqp.AmqpTransportType; import com.azure.core.amqp.ExponentialAmqpRetryPolicy; import com.azure.core.amqp.FixedAmqpRetryPolicy; -import com.azure.core.amqp.exception.AmqpErrorCondition; import com.azure.core.amqp.exception.AmqpErrorContext; import com.azure.core.amqp.exception.AmqpException; import org.junit.jupiter.api.Assertions; @@ -25,12 +24,9 @@ import reactor.util.retry.RetryBackoffSpec; import java.time.Duration; -import java.util.ArrayList; -import java.util.List; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicReference; import java.util.function.Predicate; import java.util.stream.Stream; @@ -207,151 +203,4 @@ void retryFilter(Throwable throwable, boolean expected) { // Assert assertEquals(expected, actual); } - - // ---- createRetryWithRecovery tests ---- - - /** - * FATAL errors must not be retried and must not invoke the recovery callback. - */ - @Test - void createRetryWithRecoveryFatalErrorTerminatesImmediately() { - // Arrange - final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(3).setDelay(Duration.ofMillis(100)); - final AtomicInteger recoveryCount = new AtomicInteger(); - final Retry retry = RetryUtil.createRetryWithRecovery(options, kind -> recoveryCount.incrementAndGet()); - final AmqpException fatalError = new AmqpException(false, AmqpErrorCondition.NOT_FOUND, "not found", null); - - // Act & Assert - StepVerifier.create(Mono.error(fatalError).retryWhen(retry)) - .expectErrorSatisfies(e -> assertEquals(fatalError, e)) - .verify(Duration.ofSeconds(5)); - - assertEquals(0, recoveryCount.get(), "Recovery callback must not be called for FATAL errors"); - } - - /** - * LINK errors must invoke the recovery callback and retry. The first occurrence uses - * the quick-retry path (no backoff delay). - */ - @Test - void createRetryWithRecoveryLinkErrorInvokesRecoveryAndRetries() { - // Arrange - final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(3).setDelay(Duration.ofMillis(100)); - final List recoveries = new ArrayList<>(); - final Retry retry = RetryUtil.createRetryWithRecovery(options, recoveries::add); - final AtomicInteger attempt = new AtomicInteger(); - final Mono source = Mono.defer(() -> { - if (attempt.getAndIncrement() < 2) { - return Mono.error(new AmqpException(true, AmqpErrorCondition.LINK_DETACH_FORCED, "detach", null)); - } - return Mono.just(42); - }); - - // Act & Assert — use virtual time because the base retry delay unconditionally includes - // SERVER_BUSY_WAIT_TIME (4 s), regardless of error type — the cumulative - // wait would exceed real-time test limits. - StepVerifier.withVirtualTime(() -> source.retryWhen(retry)) - .expectSubscription() - .thenAwait(Duration.ofMinutes(1)) - .expectNext(42) - .expectComplete() - .verify(Duration.ofSeconds(5)); - - assertEquals(2, recoveries.size(), "Recovery callback called once per LINK error"); - assertTrue(recoveries.stream().allMatch(k -> k == RecoveryKind.LINK)); - } - - /** - * CONNECTION errors must invoke the recovery callback with CONNECTION kind. - */ - @Test - void createRetryWithRecoveryConnectionErrorInvokesRecovery() { - // Arrange - final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(2).setDelay(Duration.ofMillis(100)); - final AtomicReference capturedKind = new AtomicReference<>(); - final Retry retry = RetryUtil.createRetryWithRecovery(options, capturedKind::set); - final AtomicInteger attempt = new AtomicInteger(); - final Mono source = Mono.defer(() -> { - if (attempt.getAndIncrement() == 0) { - return Mono.error(new AmqpException(true, AmqpErrorCondition.CONNECTION_FORCED, "forced", null)); - } - return Mono.just(1); - }); - - // Act & Assert - StepVerifier.create(source.retryWhen(retry)).expectNext(1).expectComplete().verify(Duration.ofSeconds(5)); - - assertEquals(RecoveryKind.CONNECTION, capturedKind.get(), - "Recovery callback must receive CONNECTION kind for connection errors"); - } - - /** - * After the retry budget is exhausted the error must propagate without further retries. - */ - @Test - void createRetryWithRecoveryExhaustedRetriesTerminateWithError() { - // Arrange - final int maxRetries = 2; - final AmqpRetryOptions options - = new AmqpRetryOptions().setMaxRetries(maxRetries).setDelay(Duration.ofMillis(10)); - final AtomicInteger recoveryCount = new AtomicInteger(); - final Retry retry = RetryUtil.createRetryWithRecovery(options, kind -> recoveryCount.incrementAndGet()); - final AmqpException transientError - = new AmqpException(true, AmqpErrorCondition.LINK_DETACH_FORCED, "detach", null); - - // Act & Assert — use virtual time because the base retry delay unconditionally includes - // SERVER_BUSY_WAIT_TIME (4 s), regardless of error type — the cumulative - // wait would exceed real-time test limits. - StepVerifier.withVirtualTime(() -> Mono.error(transientError).retryWhen(retry)) - .expectSubscription() - .thenAwait(Duration.ofMinutes(1)) - .expectError(AmqpException.class) - .verify(Duration.ofSeconds(5)); - - // Recovery called on each retry attempt (not the final one which terminates) - assertEquals(maxRetries, recoveryCount.get(), "Recovery callback called once per non-terminal retry"); - } - - /** - * A NONE-kind failure (server-busy) before the first LINK failure must not consume the - * quick-retry flag. The first LINK failure should still trigger the quick-retry optimization - * (no backoff delay). Prior to the T13 fix, {@code didQuickRetry.getAndSet(true)} was evaluated - * unconditionally; this test verifies the kind check comes first. - */ - @Test - void createRetryWithRecoveryNoneFailureBeforeLinkPreservesQuickRetry() { - // Arrange - final AmqpRetryOptions options = new AmqpRetryOptions().setMaxRetries(3).setDelay(Duration.ofMillis(100)); - final List recoveries = new ArrayList<>(); - final Retry retry = RetryUtil.createRetryWithRecovery(options, recoveries::add); - final AtomicInteger attempt = new AtomicInteger(); - final Mono source = Mono.defer(() -> { - switch (attempt.getAndIncrement()) { - case 0: - // NONE kind — server-busy; should not consume the quick-retry flag. - return Mono.error(new AmqpException(true, AmqpErrorCondition.SERVER_BUSY_ERROR, "busy", null)); - - case 1: - // LINK kind — first occurrence; flag must still be available → quick-retry fires. - return Mono.error(new AmqpException(true, AmqpErrorCondition.LINK_DETACH_FORCED, "detach", null)); - - default: - return Mono.just(99); - } - }); - - // Act & Assert — virtual time because the base delay logic unconditionally adds SERVER_BUSY_WAIT_TIME - // (4 s) to every retry; the first LINK error uses the quick-retry path (no delay). - StepVerifier.withVirtualTime(() -> source.retryWhen(retry)) - .expectSubscription() - .thenAwait(Duration.ofMinutes(1)) - .expectNext(99) - .expectComplete() - .verify(Duration.ofSeconds(5)); - - // NONE failures do not invoke recovery; only LINK does. - assertEquals(1, recoveries.size(), "Only the LINK failure should invoke recovery"); - assertEquals(RecoveryKind.LINK, recoveries.get(0), - "Recovery callback must be called with LINK kind for the detach error"); - } } diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java index 885ec94e606dd..0e51c9f7a3f3d 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java @@ -53,13 +53,13 @@ boolean isChannelClosed() { } /** - * Force-closes the current cached connection so the next get() creates a fresh one. - * Used for connection-level recovery when the connection is stale but the cache - * has not detected it via endpoint state signals. + * Marks the current cached connection for invalidation so the next get() closes it and + * creates a fresh one. Used for connection-level recovery when the connection is stale + * but the cache has not detected it via endpoint state signals. */ - void forceCloseConnection() { + void invalidateConnection() { if (isV2) { - cache.forceCloseConnection(); + cache.invalidateConnection(); } else { processor.forceCloseChannel(); } diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java index 21d0753618d2e..eaac2ed7f5e18 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java @@ -46,6 +46,7 @@ import java.util.Objects; import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -1735,36 +1736,48 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { // [2]. When we try to create a new session (to host the new link) but on a connection being disposed, // the retry can eventually receive a new connection and then proceed with creating session and link. // - final Mono retryableReceiveLinkMono = RetryUtil - .withRetryAndRecovery(receiveLinkMono.onErrorMap(RequestResponseChannelClosedException.class, e -> { + final Mono retryableReceiveLinkMono + = RetryUtil.withRetry(receiveLinkMono.onErrorMap(RequestResponseChannelClosedException.class, e -> { return new AmqpException(true, e.getMessage(), e, null); - }), connectionCacheWrapper.getRetryOptions(), "Failed to create receive link " + linkName, true, - recoveryKind -> { - if (recoveryKind == RecoveryKind.LINK || recoveryKind == RecoveryKind.CONNECTION) { - LOGGER.atWarning() + }).onErrorResume(e -> { + final RecoveryKind recoveryKind = RecoveryKind.classify(e); + if (recoveryKind == RecoveryKind.LINK || recoveryKind == RecoveryKind.CONNECTION) { + LOGGER.atWarning() + .addKeyValue(LINK_NAME_KEY, linkName) + .addKeyValue("recoveryKind", recoveryKind) + .log("Receive link creation failed, performing {} recovery.", recoveryKind); + + // For LINK errors during link creation, the session hosting the link may be stale. + // Ask the connection to remove it so the next retry creates a fresh session + link. + Mono recovery = connectionProcessor.flatMap(connection -> { + final boolean removed = connection.removeSession(entityPath); + LOGGER.atVerbose() .addKeyValue(LINK_NAME_KEY, linkName) - .addKeyValue("recoveryKind", recoveryKind) - .log("Receive link creation failed, performing {} recovery.", recoveryKind); - - // For LINK errors during link creation, the session hosting the link may be stale. - // Ask the connection to remove it so the next retry creates a fresh session + link. - // The entityPath is the session name used by createReceiveLink(). - // Note: the error handler fires only if obtaining the connection fails, not if removeSession fails - // (removeSession returns a boolean and never propagates an error into the reactive stream). - connectionProcessor.subscribe(connection -> { - final boolean removed = connection.removeSession(entityPath); - LOGGER.atVerbose() - .addKeyValue(LINK_NAME_KEY, linkName) - .addKeyValue("sessionRemoved", removed) - .log("Attempted stale session removal during {} recovery.", recoveryKind); - }, error -> LOGGER.atWarning() + .addKeyValue("sessionRemoved", removed) + .log("Stale session removal during {} recovery.", recoveryKind); + return Mono.empty(); + }).onErrorResume(error -> { + LOGGER.atWarning() .addKeyValue(LINK_NAME_KEY, linkName) - .log("Error obtaining connection during {} recovery.", recoveryKind, error)); - } + .log("Error obtaining connection during {} recovery.", recoveryKind, error); + return Mono.empty(); + }); + if (recoveryKind == RecoveryKind.CONNECTION) { - connectionCacheWrapper.forceCloseConnection(); + recovery + = recovery.then(Mono.fromRunnable(() -> connectionCacheWrapper.invalidateConnection())); } - }); + // Ensure the error passes the standard retry filter. Non-AMQP errors + // (e.g., IllegalStateException) classified as LINK/CONNECTION must be wrapped + // as transient AmqpException so RetryUtil.createRetry accepts them for retry. + final Throwable retriable = (e instanceof TimeoutException + || (e instanceof AmqpException && ((AmqpException) e).isTransient())) + ? e + : new AmqpException(true, e.getMessage(), e, null); + return recovery.then(Mono.error(retriable)); + } + return Mono.error(e); + }), connectionCacheWrapper.getRetryOptions(), "Failed to create receive link " + linkName, true); // A Flux that produces a new AmqpReceiveLink each time it receives a request from the below // 'AmqpReceiveLinkProcessor'. Obviously, the processor requests a link when there is a downstream subscriber. diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java index 9dd65e05d85e3..c0ac1155a22c2 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java @@ -37,6 +37,7 @@ import java.util.Locale; import java.util.Objects; import java.util.Set; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; @@ -882,10 +883,10 @@ private Mono sendBatchInternal(ServiceBusMessageBatch batch, }); final String timeoutMessage = "Sending messages timed out. message-count:" + batch.getCount() + entityId(); - final Mono withRetry - = RetryUtil.withRetryAndRecovery(sendMessage, retryOptions, timeoutMessage, recoveryKind -> { - performRecovery(recoveryKind, "sendBatch", operationLink); - }).onErrorMap(this::mapError); + final Mono withRetry = RetryUtil.withRetry( + sendMessage + .onErrorResume(e -> recoverBeforeRetry(e, "sendBatch", operationLink).then(Mono.error(asRetriable(e)))), + retryOptions, timeoutMessage).onErrorMap(this::mapError); return instrumentation.instrumentSendBatch("ServiceBus.send", withRetry, batch.getMessages()); } @@ -901,9 +902,12 @@ private Mono sendFluxInternal(Flux messages, // which could duplicate side-effects or re-consume a hot publisher. final AtomicReference operationLink = new AtomicReference<>(); final Mono linkWithRecovery - = RetryUtil.withRetryAndRecovery(getSendLink("send-batches").doOnNext(operationLink::set), retryOptions, - "Failed to acquire send link for batch collection." + entityId(), - recoveryKind -> performRecovery(recoveryKind, "sendFlux-link", operationLink)); + = RetryUtil + .withRetry( + getSendLink("send-batches").doOnNext(operationLink::set) + .onErrorResume(e -> recoverBeforeRetry(e, "sendFlux-link", operationLink) + .then(Mono.error(asRetriable(e)))), + retryOptions, "Failed to acquire send link for batch collection." + entityId()); final Mono> batchListMono = linkWithRecovery.flatMap(link -> link.getLinkSize().flatMap(size -> { @@ -950,10 +954,10 @@ private Mono getSendLink(String callSite) { } private Mono getSendLinkWithRetry(String callSite) { - return RetryUtil.withRetryAndRecovery(getSendLink(callSite), retryOptions, - String.format(retryGetLinkErrorMessageFormat, callSite), recoveryKind -> { - performRecovery(recoveryKind, "getSendLink-" + callSite, null); - }); + return RetryUtil.withRetry( + getSendLink(callSite).onErrorResume( + e -> recoverBeforeRetry(e, "getSendLink-" + callSite, null).then(Mono.error(asRetriable(e)))), + retryOptions, String.format(retryGetLinkErrorMessageFormat, callSite)); } private Throwable mapError(Throwable throwable) { @@ -964,48 +968,70 @@ private Throwable mapError(Throwable throwable) { } /** - * Performs tiered recovery by disposing stale resources based on the classified error. - * For LINK recovery, disposes the send link so the next retry creates a fresh one. - * For CONNECTION recovery, disposes the link and the connection so the connection - * processor creates everything fresh. + * Performs reactive tiered recovery based on the classified error. For LINK recovery, closes the + * send link so the next retry creates a fresh one. For CONNECTION recovery, closes the link and + * invalidates the connection so the cache creates everything fresh. * - *

This matches the Go SDK's RecoverIfNeeded() and the .NET SDK's - * FaultTolerantAmqpObject pattern.

+ *

Called from {@code onErrorResume} at each send call site so that recovery completes before + * the error is re-emitted and the standard retry logic re-subscribes.

*/ - private void performRecovery(RecoveryKind recoveryKind, String callSite, AtomicReference linkRef) { - if (recoveryKind == RecoveryKind.NONE || recoveryKind == RecoveryKind.FATAL) { - return; + private Mono recoverBeforeRetry(Throwable error, String callSite, AtomicReference linkRef) { + final RecoveryKind kind = RecoveryKind.classify(error); + if (kind != RecoveryKind.LINK && kind != RecoveryKind.CONNECTION) { + return Mono.empty(); } logger.atWarning() .addKeyValue(ENTITY_PATH_KEY, entityName) - .addKeyValue("recoveryKind", recoveryKind) + .addKeyValue("recoveryKind", kind) .addKeyValue("callSite", callSite) - .log("Performing {} recovery before retry.", recoveryKind); + .log("Performing {} recovery before retry.", kind); - // Start async close of the operation-scoped send link so the next retry creates a fresh one. + // Close the operation-scoped send link so the next retry creates a fresh one. // Using a per-operation AtomicReference (not a class-level field) prevents concurrent send // operations from accidentally closing each other's links. - // Use closeAsync() rather than dispose() to avoid blocking the Reactor thread; ReactorSender - // dispose() calls closeAsync().block(tryTimeout), which is illegal on a non-blocking scheduler. + // Use closeAsync() rather than dispose() to avoid blocking the Reactor thread. + Mono recovery = Mono.empty(); final AmqpSendLink link = linkRef != null ? linkRef.getAndSet(null) : null; if (link != null) { - link.closeAsync() - .subscribe(null, - error -> logger.atVerbose() - .addKeyValue(ENTITY_PATH_KEY, entityName) - .log("Error closing stale send link during recovery.", error)); + recovery = Mono.defer(() -> { + Mono close = link.closeAsync(); + return close != null ? close : Mono.empty(); + }).onErrorResume(closeErr -> { + logger.atVerbose() + .addKeyValue(ENTITY_PATH_KEY, entityName) + .log("Error closing stale send link during recovery.", closeErr); + return Mono.empty(); + }); } linkName.set(null); - // For CONNECTION errors, explicitly force-close the cached connection so the - // next get() on the connection processor creates a fresh one. This handles the - // stale-connection scenario where heartbeats are echoed by intermediate - // infrastructure and the cache has not yet detected the failure. - // Matches Go SDK's Namespace.Recover(). - if (recoveryKind == RecoveryKind.CONNECTION) { - connectionCacheWrapper.forceCloseConnection(); + // For CONNECTION errors, invalidate the cached connection so the next get() closes it + // and creates a fresh one. Matches Go SDK's Namespace.Recover(). + if (kind == RecoveryKind.CONNECTION) { + recovery = recovery.then(Mono.fromRunnable(() -> connectionCacheWrapper.invalidateConnection())); + } + + return recovery; + } + + /** + * Ensures the error is retriable by the standard {@link RetryUtil#createRetry} filter. + * That filter only accepts {@link TimeoutException} or transient {@link AmqpException}. + * Non-AMQP errors like {@code IllegalStateException("Cannot publish when disposed")} + * that {@link RecoveryKind} classifies as LINK would otherwise be rejected, bypassing + * the recovery we just performed. This wraps such errors as transient AmqpException. + */ + private static Throwable asRetriable(Throwable error) { + if (error instanceof TimeoutException + || (error instanceof AmqpException && ((AmqpException) error).isTransient())) { + return error; + } + final RecoveryKind kind = RecoveryKind.classify(error); + if (kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION) { + return new AmqpException(true, error.getMessage(), error, null); } + return error; } private String entityId() { diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java index d3a715e9976b0..a090f9f7c1597 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java @@ -143,7 +143,7 @@ private Mono acquireIntern(String sessionId) { logger.atWarning() .addKeyValue(ENTITY_PATH_KEY, entityPath) .log("Connection-level error acquiring session, forcing connection recovery.", t); - connectionCacheWrapper.forceCloseConnection(); + connectionCacheWrapper.invalidateConnection(); return Mono.delay(Duration.ZERO); } if (isTimeoutError(t)) { diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java index 59971f46fc80a..7d661b6348843 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java @@ -298,7 +298,7 @@ Mono getActiveLink() { LOGGER.atWarning() .addKeyValue(ENTITY_PATH_KEY, entityPath) .log("Connection-level error in session manager, forcing connection recovery.", failure); - connectionCacheWrapper.forceCloseConnection(); + connectionCacheWrapper.invalidateConnection(); } if (failure instanceof TimeoutException) { From cb1e86a67784b9c9497eec979fc03b316ce74d5a Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 8 Apr 2026 21:50:21 -0700 Subject: [PATCH 19/39] fix(servicebus): restore comment explaining Mono.delay(Duration.ZERO) rationale - Add shared comment above retry branches explaining why Mono.delay(Duration.ZERO) is used instead of Mono.empty() (QPid thread blocking caused ICM) - Restore TIMEOUT_ERROR branch-specific comment from original code - Addresses review feedback from anuchandy on PR #48460 --- .../azure/messaging/servicebus/ServiceBusSessionManager.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java index 7d661b6348843..b3458c1aa9524 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java @@ -301,10 +301,15 @@ Mono getActiveLink() { connectionCacheWrapper.invalidateConnection(); } + // Mono.delay(Duration.ZERO) instead of Mono.empty() — the delay schedules the retry on the + // 'parallel' Scheduler, freeing the QPid (proton) thread for other IO. Using Mono.empty() would + // block the QPid thread and slow down / block message pumping in other sessions. if (failure instanceof TimeoutException) { return Mono.delay(Duration.ZERO); } else if (failure instanceof AmqpException && ((AmqpException) failure).getErrorCondition() == AmqpErrorCondition.TIMEOUT_ERROR) { + // The link closed remotely with 'Detach {errorCondition:com.microsoft:timeout}' frame because + // the broker waited for N seconds (60 sec hard limit today) but there was no free or new session. return Mono.delay(Duration.ZERO); } else if (kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION) { // Link or connection-level error — retry to acquire a fresh link (or connection). From a3b639cb8b0df6395ca4fc05cb29694d716d2880 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 8 Apr 2026 22:18:03 -0700 Subject: [PATCH 20/39] fix(servicebus): include exception in recovery warning logs - Pass triggering exception to .log() in receiver link recovery warning - Pass triggering error to .log() in sender recovery warning - Improves diagnosability of LINK/CONNECTION recovery events in production --- .../messaging/servicebus/ServiceBusReceiverAsyncClient.java | 2 +- .../azure/messaging/servicebus/ServiceBusSenderAsyncClient.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java index eaac2ed7f5e18..20dc1bae119ce 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java @@ -1745,7 +1745,7 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { LOGGER.atWarning() .addKeyValue(LINK_NAME_KEY, linkName) .addKeyValue("recoveryKind", recoveryKind) - .log("Receive link creation failed, performing {} recovery.", recoveryKind); + .log("Receive link creation failed, performing {} recovery.", recoveryKind, e); // For LINK errors during link creation, the session hosting the link may be stale. // Ask the connection to remove it so the next retry creates a fresh session + link. diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java index c0ac1155a22c2..91b7e9cb4c38a 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java @@ -985,7 +985,7 @@ private Mono recoverBeforeRetry(Throwable error, String callSite, AtomicRe .addKeyValue(ENTITY_PATH_KEY, entityName) .addKeyValue("recoveryKind", kind) .addKeyValue("callSite", callSite) - .log("Performing {} recovery before retry.", kind); + .log("Performing {} recovery before retry.", kind, error); // Close the operation-scoped send link so the next retry creates a fresh one. // Using a per-operation AtomicReference (not a class-level field) prevents concurrent send From ea2961b4812c6a0f676d832f2ed8fb461f7e6d37 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 8 Apr 2026 22:38:18 -0700 Subject: [PATCH 21/39] docs(servicebus): clarify invalidateConnection v1/v2 behavior in javadoc - Updated javadoc to document that v2 marks connection for lazy invalidation while v1 force-closes the channel immediately --- .../azure/messaging/servicebus/ConnectionCacheWrapper.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java index 0e51c9f7a3f3d..282b846b4f42e 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java @@ -53,9 +53,10 @@ boolean isChannelClosed() { } /** - * Marks the current cached connection for invalidation so the next get() closes it and - * creates a fresh one. Used for connection-level recovery when the connection is stale - * but the cache has not detected it via endpoint state signals. + * Invalidates the current cached connection for connection-level recovery when the + * connection is stale but the cache has not detected it via endpoint state signals. + * On v2, marks the connection for invalidation so the next get() closes it and creates + * a fresh one. On v1, force-closes the current channel immediately. */ void invalidateConnection() { if (isV2) { From 43a27f4e270ffc5413b70ec63f67d72658664d1d Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 6 May 2026 16:27:05 -0700 Subject: [PATCH 22/39] test(amqp): add unit tests for ReactorConnectionCache.invalidateConnection Add three focused tests in ReactorConnectionCacheTest covering: - invalidateConnection() causes the next get() to close the cached connection and supply a new one (target-id matched, marker consumed). - AB-A safety: after the marker is consumed, a freshly-supplied connection is NOT invalidated by the stale marker. - invalidateConnection() with no cached connection is a safe no-op. Also update azure-messaging-servicebus/pom.xml dependency versions to match version_client.txt after the merge from main (azure-core 1.58.0, azure-core-http-netty 1.16.4, azure-core-test 1.27.0-beta.15, azure- identity 1.18.3, azure-core-experimental 1.0.0-beta.67, azure-core- tracing-opentelemetry 1.0.0-beta.63). The pom_file_version_scanner flagged these as drift after the merge. --- .../ReactorConnectionCacheTest.java | 123 ++++++++++++++++++ .../azure-messaging-servicebus/pom.xml | 12 +- 2 files changed, 129 insertions(+), 6 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionCacheTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionCacheTest.java index 4b51308f8e569..2016d3fe0260a 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionCacheTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionCacheTest.java @@ -222,6 +222,129 @@ public void shouldRefreshCacheOnErrorInCachedConnection() { } } + @Test + public void invalidateConnectionReplacesCachedConnectionOnNextGet() { + final ConnectionSupplier connectionSupplier = new ConnectionSupplier(); + final ReactorConnectionCache connectionCache + = new ReactorConnectionCache<>(connectionSupplier, FQDN, ENTITY_PATH, retryPolicy, new HashMap<>()); + try { + final ReactorConnection[] c = new ReactorConnection[1]; + final Mono connectionMono = connectionCache.get(); + // Populate the cache with the first connection. + StepVerifier.create(connectionMono, 0) + .thenRequest(1) + .then(() -> connectionSupplier.emitEndpointState(EndpointState.ACTIVE)) + .expectNextMatches(con -> { + Assertions.assertFalse(con.isDisposed()); + c[0] = con; + return true; + }) + .expectComplete() + .verify(VERIFY_TIMEOUT); + connectionSupplier.assertInvocationCount(1); + + // Mark the current connection for force-invalidation. + connectionCache.invalidateConnection(); + + // Next subscription must close the marked connection and serve a brand-new one. + StepVerifier.create(connectionMono, 0) + .thenRequest(1) + .then(() -> connectionSupplier.emitEndpointState(EndpointState.ACTIVE)) + .expectNextMatches(con -> { + Assertions.assertFalse(con.isDisposed()); + Assertions.assertNotEquals(c[0], con); + return true; + }) + .expectComplete() + .verify(VERIFY_TIMEOUT); + + // The previously cached connection should be closed by the cacheInvalidateIf branch. + Assertions.assertTrue(c[0].isDisposed()); + // A second connection had to be supplied because the first was force-invalidated. + connectionSupplier.assertInvocationCount(2); + } finally { + connectionSupplier.dispose(); + connectionCache.dispose(); + } + } + + @Test + public void invalidateConnectionDoesNotInvalidateNewlyCreatedConnection() { + // Asserts the AB-A safety property: invalidate() targets a specific connection-id, so a + // fresh connection created after the invalidation marker was consumed must NOT be evicted + // by a stale marker. + final ConnectionSupplier connectionSupplier = new ConnectionSupplier(); + final ReactorConnectionCache connectionCache + = new ReactorConnectionCache<>(connectionSupplier, FQDN, ENTITY_PATH, retryPolicy, new HashMap<>()); + try { + final ReactorConnection[] c = new ReactorConnection[2]; + final Mono connectionMono = connectionCache.get(); + StepVerifier.create(connectionMono, 0) + .thenRequest(1) + .then(() -> connectionSupplier.emitEndpointState(EndpointState.ACTIVE)) + .expectNextMatches(con -> { + c[0] = con; + return true; + }) + .expectComplete() + .verify(VERIFY_TIMEOUT); + + connectionCache.invalidateConnection(); + + // Consume the invalidation marker and obtain the replacement connection. + StepVerifier.create(connectionMono, 0) + .thenRequest(1) + .then(() -> connectionSupplier.emitEndpointState(EndpointState.ACTIVE)) + .expectNextMatches(con -> { + c[1] = con; + Assertions.assertNotEquals(c[0], c[1]); + return true; + }) + .expectComplete() + .verify(VERIFY_TIMEOUT); + + // The marker has been consumed; another get() must serve the same (cached) replacement + // connection without creating a third one. + StepVerifier.create(connectionMono, 0).thenRequest(1).expectNextMatches(con -> { + Assertions.assertEquals(c[1], con); + Assertions.assertFalse(con.isDisposed()); + return true; + }).expectComplete().verify(VERIFY_TIMEOUT); + + connectionSupplier.assertInvocationCount(2); + } finally { + connectionSupplier.dispose(); + connectionCache.dispose(); + } + } + + @Test + public void invalidateConnectionWithoutCachedConnectionIsNoOp() { + final ConnectionSupplier connectionSupplier = new ConnectionSupplier(); + final ReactorConnectionCache connectionCache + = new ReactorConnectionCache<>(connectionSupplier, FQDN, ENTITY_PATH, retryPolicy, new HashMap<>()); + try { + // No connection has been supplied yet. + connectionCache.invalidateConnection(); + connectionSupplier.assertInvocationCount(0); + + // First get() after the no-op invalidate() must still produce a connection normally. + StepVerifier.create(connectionCache.get(), 0) + .thenRequest(1) + .then(() -> connectionSupplier.emitEndpointState(EndpointState.ACTIVE)) + .expectNextMatches(con -> { + Assertions.assertFalse(con.isDisposed()); + return true; + }) + .expectComplete() + .verify(VERIFY_TIMEOUT); + connectionSupplier.assertInvocationCount(1); + } finally { + connectionSupplier.dispose(); + connectionCache.dispose(); + } + } + @Test public void shouldBubbleUpNonRetriableError() { final ConnectionSupplier connectionSupplier = new ConnectionSupplier(); diff --git a/sdk/servicebus/azure-messaging-servicebus/pom.xml b/sdk/servicebus/azure-messaging-servicebus/pom.xml index d55656f00bbc7..f5e60c55f37de 100644 --- a/sdk/servicebus/azure-messaging-servicebus/pom.xml +++ b/sdk/servicebus/azure-messaging-servicebus/pom.xml @@ -59,7 +59,7 @@ com.azure azure-core - 1.57.1 + 1.58.0 com.azure @@ -74,20 +74,20 @@ com.azure azure-core-http-netty - 1.16.3 + 1.16.4 com.azure azure-core-test - 1.27.0-beta.14 + 1.27.0-beta.15 test com.azure azure-identity - 1.18.2 + 1.18.3 test @@ -114,14 +114,14 @@ com.azure azure-core-experimental - 1.0.0-beta.66 + 1.0.0-beta.67 test com.azure azure-core-tracing-opentelemetry - 1.0.0-beta.62 + 1.0.0-beta.63 test From 43d5ff1d3e968e3b1c2071bd2b4679459591c435 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 6 May 2026 16:42:26 -0700 Subject: [PATCH 23/39] fix(servicebus): wrap getLinkSize() in retry+recovery boundary Previously the retry+recovery wrapper covered only getSendLink(); the subsequent link.getLinkSize() call happened outside the retry boundary. A transient link/connection failure during AMQP size negotiation (link never reaches ACTIVE in time, link disposed mid-negotiation) would short-circuit the entire send/schedule/createBatch operation without triggering LINK or CONNECTION recovery. Add getSendLinkAndSizeWithRetry() helper that wraps both getSendLink() and link.getLinkSize() inside a single RetryUtil.withRetry boundary with the existing recoverBeforeRetry path. Refactor three call sites to use it: createMessageBatch, scheduleMessageInternal, sendFluxInternal. The user-provided messages Flux remains outside the retry boundary in sendFluxInternal so it is not re-subscribed on retry. Remove the now- unused getSendLinkWithRetry() helper. --- .../ServiceBusSenderAsyncClient.java | 89 +++++++++++-------- 1 file changed, 50 insertions(+), 39 deletions(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java index 91b7e9cb4c38a..c35a33cb6c1c1 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java @@ -26,6 +26,8 @@ import org.apache.qpid.proton.amqp.transaction.TransactionalState; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.util.function.Tuple2; +import reactor.util.function.Tuples; import java.time.OffsetDateTime; import java.util.ArrayList; @@ -467,7 +469,9 @@ public Mono createMessageBatch(CreateMessageBatchOptions final int maxSize = options.getMaximumSizeInBytes(); - return getSendLinkWithRetry("create-batch").flatMap(link -> link.getLinkSize().flatMap(size -> { + return getSendLinkAndSizeWithRetry("create-batch").flatMap(t -> { + final AmqpSendLink link = t.getT1(); + final int size = t.getT2(); final int maximumLinkSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; if (maxSize > maximumLinkSize) { return monoError(logger, @@ -479,7 +483,7 @@ public Mono createMessageBatch(CreateMessageBatchOptions final int batchSize = maxSize > 0 ? maxSize : maximumLinkSize; return Mono .just(new ServiceBusMessageBatch(isV2, batchSize, link::getErrorContext, tracer, messageSerializer)); - })).onErrorMap(this::mapError); + }).onErrorMap(this::mapError); } /** @@ -815,20 +819,18 @@ private Mono scheduleMessageInternal(ServiceBusMessage message, OffsetDate return monoError(logger, new NullPointerException("'scheduledEnqueueTime' cannot be null.")); } - return tracer - .traceScheduleMono("ServiceBus.scheduleMessage", getSendLinkWithRetry("schedule-message").flatMap(link -> { - return link.getLinkSize().flatMap(size -> { - final int maxSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; - return connectionProcessor - .flatMap(connection -> connection.getManagementNode(entityName, entityType)) - .flatMap( - managementNode -> managementNode - .schedule(Arrays.asList(message), scheduledEnqueueTime, maxSize, link.getLinkName(), - transactionContext) - .next()); - }); - }), message, message.getContext()) - .onErrorMap(this::mapError); + return tracer.traceScheduleMono("ServiceBus.scheduleMessage", + getSendLinkAndSizeWithRetry("schedule-message").flatMap(t -> { + final AmqpSendLink link = t.getT1(); + final int size = t.getT2(); + final int maxSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; + return connectionProcessor.flatMap(connection -> connection.getManagementNode(entityName, entityType)) + .flatMap( + managementNode -> managementNode + .schedule(Arrays.asList(message), scheduledEnqueueTime, maxSize, link.getLinkName(), + transactionContext) + .next()); + }), message, message.getContext()).onErrorMap(this::mapError); } /** @@ -897,26 +899,22 @@ private Mono sendFluxInternal(Flux messages, new IllegalStateException(String.format(INVALID_OPERATION_DISPOSED_SENDER, "sendMessage"))); } - // Apply retry+recovery only to link acquisition. Keeping messages.collect() outside the - // retry boundary avoids re-subscribing the user-provided Flux on each retry attempt, - // which could duplicate side-effects or re-consume a hot publisher. - final AtomicReference operationLink = new AtomicReference<>(); - final Mono linkWithRecovery - = RetryUtil - .withRetry( - getSendLink("send-batches").doOnNext(operationLink::set) - .onErrorResume(e -> recoverBeforeRetry(e, "sendFlux-link", operationLink) - .then(Mono.error(asRetriable(e)))), - retryOptions, "Failed to acquire send link for batch collection." + entityId()); - - final Mono> batchListMono - = linkWithRecovery.flatMap(link -> link.getLinkSize().flatMap(size -> { - final int batchSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; - final CreateMessageBatchOptions batchOptions - = new CreateMessageBatchOptions().setMaximumSizeInBytes(batchSize); - return messages.collect( - new AmqpMessageCollector(isV2, batchOptions, 1, link::getErrorContext, tracer, messageSerializer)); - })); + // Apply retry+recovery to BOTH link acquisition and link-size negotiation. Keeping + // messages.collect() outside the retry boundary avoids re-subscribing the user-provided + // Flux on each retry attempt, which could duplicate side-effects or re-consume a hot + // publisher. A getLinkSize() failure (link never ACTIVE, link disposed during negotiation) + // now triggers the same tiered recovery as a getSendLink() failure. + final Mono> linkAndSize = getSendLinkAndSizeWithRetry("send-batches"); + + final Mono> batchListMono = linkAndSize.flatMap(t -> { + final AmqpSendLink link = t.getT1(); + final int size = t.getT2(); + final int batchSize = size > 0 ? size : MAX_MESSAGE_LENGTH_BYTES; + final CreateMessageBatchOptions batchOptions + = new CreateMessageBatchOptions().setMaximumSizeInBytes(batchSize); + return messages.collect( + new AmqpMessageCollector(isV2, batchOptions, 1, link::getErrorContext, tracer, messageSerializer)); + }); final Mono sendOperation = batchListMono.flatMap(list -> Flux.fromIterable(list) .flatMap(batch -> sendBatchInternal(batch, transactionContext)) @@ -953,10 +951,23 @@ private Mono getSendLink(String callSite) { return getSendLink; } - private Mono getSendLinkWithRetry(String callSite) { + /** + * Acquires a send link AND its negotiated maximum message size inside a single retry+recovery + * boundary. A failure during {@code getLinkSize()} (e.g., the link never reaches ACTIVE within + * the timeout, or the link becomes disposed mid-negotiation) triggers the same tiered recovery + * as a failure during {@code getSendLink()} — without it, transient size-negotiation faults + * would short-circuit the operation without LINK or CONNECTION recovery. + * + * @param callSite identifier used in recovery diagnostics. + * @return a Mono emitting (link, size) on success, retried with recovery on transient failures. + */ + private Mono> getSendLinkAndSizeWithRetry(String callSite) { + final AtomicReference operationLink = new AtomicReference<>(); return RetryUtil.withRetry( - getSendLink(callSite).onErrorResume( - e -> recoverBeforeRetry(e, "getSendLink-" + callSite, null).then(Mono.error(asRetriable(e)))), + getSendLink(callSite).doOnNext(operationLink::set) + .flatMap(link -> link.getLinkSize().map(size -> Tuples.of(link, size))) + .onErrorResume(e -> recoverBeforeRetry(e, "getSendLinkAndSize-" + callSite, operationLink) + .then(Mono.error(asRetriable(e)))), retryOptions, String.format(retryGetLinkErrorMessageFormat, callSite)); } From e1b382a4012c6b8d9e9f7cb0f061e61edf36cdd3 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 6 May 2026 16:48:16 -0700 Subject: [PATCH 24/39] fix(servicebus): remove unused static import for RetryUtil.withRetry After the getSendLinkAndSizeWithRetry refactor, all withRetry callers in this file use the qualified RetryUtil.withRetry form. Remove the now- unused static import to satisfy checkstyle. --- .../azure/messaging/servicebus/ServiceBusSenderAsyncClient.java | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java index c35a33cb6c1c1..49c78ad0d96f8 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java @@ -49,7 +49,6 @@ import java.util.stream.Collector; import static com.azure.core.amqp.implementation.ClientConstants.ENTITY_PATH_KEY; -import static com.azure.core.amqp.implementation.RetryUtil.withRetry; import static com.azure.core.util.FluxUtil.fluxError; import static com.azure.core.util.FluxUtil.monoError; import static com.azure.messaging.servicebus.implementation.Messages.INVALID_OPERATION_DISPOSED_SENDER; From 95e90700f372b67e425fef1080b197ff839ab17a Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 6 May 2026 17:51:33 -0700 Subject: [PATCH 25/39] docs(servicebus): clarify session-removal recovery comment for LINK and CONNECTION The recovery comment said session removal was for LINK errors only, but the code runs removeSession() for both LINK and CONNECTION recovery. Update the comment to mention both tiers and clarify that CONNECTION additionally invalidates the cached connection. --- .../messaging/servicebus/ServiceBusReceiverAsyncClient.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java index 20dc1bae119ce..8a8ee446d1865 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java @@ -1747,8 +1747,10 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { .addKeyValue("recoveryKind", recoveryKind) .log("Receive link creation failed, performing {} recovery.", recoveryKind, e); - // For LINK errors during link creation, the session hosting the link may be stale. - // Ask the connection to remove it so the next retry creates a fresh session + link. + // For both LINK and CONNECTION recovery, the session hosting the failed link may + // be stale. Ask the connection to remove it so the next retry creates a fresh + // session + link. CONNECTION recovery additionally invalidates the cached + // connection below so the next retry rebuilds connection, session, and link. Mono recovery = connectionProcessor.flatMap(connection -> { final boolean removed = connection.removeSession(entityPath); LOGGER.atVerbose() From 36d9848c73f8c8388abb86a28406b8cee42711d2 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Thu, 7 May 2026 11:52:01 -0700 Subject: [PATCH 26/39] refactor(servicebus): extract shared RecoveryUtils.asRetriable helper Move the asRetriable() retry-filter wrapper from ServiceBusSenderAsyncClient into a new package-private RecoveryUtils class. The receiver had the same logic inline at the receive-link creation call site. Both call sites now share one implementation, keeping the retry+recovery composition consistent across send and receive paths and removing duplicate maintenance. Behavior is identical; the helper only formalises what the inline logic already did. Removed the now-unused TimeoutException import from both client files. --- .../messaging/servicebus/RecoveryUtils.java | 44 +++++++++++++++++++ .../ServiceBusReceiverAsyncClient.java | 13 +++--- .../ServiceBusSenderAsyncClient.java | 26 ++--------- 3 files changed, 52 insertions(+), 31 deletions(-) create mode 100644 sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java new file mode 100644 index 0000000000000..0c2cc201e87cd --- /dev/null +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java @@ -0,0 +1,44 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package com.azure.messaging.servicebus; + +import com.azure.core.amqp.exception.AmqpException; +import com.azure.core.amqp.implementation.RecoveryKind; +import com.azure.core.amqp.implementation.RetryUtil; + +import java.util.concurrent.TimeoutException; + +/** + * Package-private utilities shared by the sender and receiver call sites that compose + * tiered recovery on top of {@link RetryUtil#withRetry}. + */ +final class RecoveryUtils { + + private RecoveryUtils() { + } + + /** + * Ensures the error is retriable by the standard {@link RetryUtil#createRetry} filter. + * That filter only accepts {@link TimeoutException} or transient {@link AmqpException}. + * Non-AMQP errors like {@code IllegalStateException("Cannot publish when disposed")} + * that {@link RecoveryKind} classifies as LINK would otherwise be rejected, bypassing + * the recovery just performed. This wraps such errors as transient {@link AmqpException} + * so the same recovery + retry composition works for both sender and receiver paths. + * + * @param error the original error emitted from a send/receive operation. + * @return the same error if it already passes the retry filter, otherwise a transient + * {@link AmqpException} wrapping it (only for LINK/CONNECTION-classified errors). + */ + static Throwable asRetriable(Throwable error) { + if (error instanceof TimeoutException + || (error instanceof AmqpException && ((AmqpException) error).isTransient())) { + return error; + } + final RecoveryKind kind = RecoveryKind.classify(error); + if (kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION) { + return new AmqpException(true, error.getMessage(), error, null); + } + return error; + } +} diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java index 8a8ee446d1865..ddc4f105b2338 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java @@ -46,7 +46,6 @@ import java.util.Objects; import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -1769,13 +1768,11 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { recovery = recovery.then(Mono.fromRunnable(() -> connectionCacheWrapper.invalidateConnection())); } - // Ensure the error passes the standard retry filter. Non-AMQP errors - // (e.g., IllegalStateException) classified as LINK/CONNECTION must be wrapped - // as transient AmqpException so RetryUtil.createRetry accepts them for retry. - final Throwable retriable = (e instanceof TimeoutException - || (e instanceof AmqpException && ((AmqpException) e).isTransient())) - ? e - : new AmqpException(true, e.getMessage(), e, null); + // Use the shared retry-filter wrapper so non-AMQP errors classified as + // LINK/CONNECTION (e.g., IllegalStateException) are wrapped as transient + // AmqpException and accepted by RetryUtil.createRetry. Centralising this + // logic in RecoveryUtils keeps the sender and receiver paths consistent. + final Throwable retriable = RecoveryUtils.asRetriable(e); return recovery.then(Mono.error(retriable)); } return Mono.error(e); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java index 49c78ad0d96f8..9ad4bba61171d 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java @@ -39,7 +39,6 @@ import java.util.Locale; import java.util.Objects; import java.util.Set; -import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; @@ -885,8 +884,8 @@ private Mono sendBatchInternal(ServiceBusMessageBatch batch, final String timeoutMessage = "Sending messages timed out. message-count:" + batch.getCount() + entityId(); final Mono withRetry = RetryUtil.withRetry( - sendMessage - .onErrorResume(e -> recoverBeforeRetry(e, "sendBatch", operationLink).then(Mono.error(asRetriable(e)))), + sendMessage.onErrorResume( + e -> recoverBeforeRetry(e, "sendBatch", operationLink).then(Mono.error(RecoveryUtils.asRetriable(e)))), retryOptions, timeoutMessage).onErrorMap(this::mapError); return instrumentation.instrumentSendBatch("ServiceBus.send", withRetry, batch.getMessages()); } @@ -966,7 +965,7 @@ private Mono> getSendLinkAndSizeWithRetry(String c getSendLink(callSite).doOnNext(operationLink::set) .flatMap(link -> link.getLinkSize().map(size -> Tuples.of(link, size))) .onErrorResume(e -> recoverBeforeRetry(e, "getSendLinkAndSize-" + callSite, operationLink) - .then(Mono.error(asRetriable(e)))), + .then(Mono.error(RecoveryUtils.asRetriable(e)))), retryOptions, String.format(retryGetLinkErrorMessageFormat, callSite)); } @@ -1025,25 +1024,6 @@ private Mono recoverBeforeRetry(Throwable error, String callSite, AtomicRe return recovery; } - /** - * Ensures the error is retriable by the standard {@link RetryUtil#createRetry} filter. - * That filter only accepts {@link TimeoutException} or transient {@link AmqpException}. - * Non-AMQP errors like {@code IllegalStateException("Cannot publish when disposed")} - * that {@link RecoveryKind} classifies as LINK would otherwise be rejected, bypassing - * the recovery we just performed. This wraps such errors as transient AmqpException. - */ - private static Throwable asRetriable(Throwable error) { - if (error instanceof TimeoutException - || (error instanceof AmqpException && ((AmqpException) error).isTransient())) { - return error; - } - final RecoveryKind kind = RecoveryKind.classify(error); - if (kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION) { - return new AmqpException(true, error.getMessage(), error, null); - } - return error; - } - private String entityId() { return " " + ENTITY_PATH_KEY + ":" + entityName + (viaEntityName != null ? " " + VIA_ENTITY_NAME_KEY + ":" + viaEntityName : "") + " "; From 1ce02a1d139f89e8ae7c381b6e07404feaa42795 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Thu, 7 May 2026 12:06:55 -0700 Subject: [PATCH 27/39] fix(servicebus): remove unused RetryUtil import in RecoveryUtils The RetryUtil import was only referenced from JavaDoc {@link} tags, which checkstyle's UnusedImports rule treats as unused. Replace the imported references with fully-qualified type names in the JavaDoc to satisfy the rule without losing the cross-reference. --- .../java/com/azure/messaging/servicebus/RecoveryUtils.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java index 0c2cc201e87cd..dd1d53ce7ff12 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java @@ -5,13 +5,12 @@ import com.azure.core.amqp.exception.AmqpException; import com.azure.core.amqp.implementation.RecoveryKind; -import com.azure.core.amqp.implementation.RetryUtil; import java.util.concurrent.TimeoutException; /** * Package-private utilities shared by the sender and receiver call sites that compose - * tiered recovery on top of {@link RetryUtil#withRetry}. + * tiered recovery on top of {@link com.azure.core.amqp.implementation.RetryUtil#withRetry}. */ final class RecoveryUtils { @@ -19,7 +18,8 @@ private RecoveryUtils() { } /** - * Ensures the error is retriable by the standard {@link RetryUtil#createRetry} filter. + * Ensures the error is retriable by the standard + * {@link com.azure.core.amqp.implementation.RetryUtil#createRetry} filter. * That filter only accepts {@link TimeoutException} or transient {@link AmqpException}. * Non-AMQP errors like {@code IllegalStateException("Cannot publish when disposed")} * that {@link RecoveryKind} classifies as LINK would otherwise be rejected, bypassing From e469d4977970df5312243f9cbe15e36f3ea6b1fc Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Tue, 12 May 2026 19:55:12 -0700 Subject: [PATCH 28/39] ci: retrigger after infrastructure flakes Both failures on the previous build are transient infrastructure issues unrelated to PR content: - Build Analyze: TempTypeSpecFiles cleanup script race in eng/scripts/Compare-CurrentToCodegeneration.ps1 (verification step itself passed with 'no diff'). - macOS Java 18 test: 'Connection reset' downloading mockito-inline:4.11.0 from the Azure DevOps Maven feed. All 1639 tests (servicebus + core-amqp) pass locally with the merge applied. Empty commit to retrigger CI. From 2cd332681af2e563c5ac0f4e90d90b3598f7b276 Mon Sep 17 00:00:00 2001 From: "Eldert Grootenboer (from Dev Box)" Date: Wed, 17 Jun 2026 16:32:18 -0700 Subject: [PATCH 29/39] Use current version tag for azure-core-amqp dependency This PR adds new azure-core-amqp API consumed by azure-messaging-servicebus, so the two libraries co-release in the same pipeline. The servicebus POM must reference azure-core-amqp via the current version tag (2.13.0-beta.1), not the dependency tag (released 2.12.0). With the dependency tag, the version scanner failed ("incorrectly set to dependency") and non-From-Source CI builds could not resolve the new symbols ("cannot find symbol"), cascading to all Build/Test/Analyze/APIView jobs. --- sdk/servicebus/azure-messaging-servicebus/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/pom.xml b/sdk/servicebus/azure-messaging-servicebus/pom.xml index eb51f788113a4..fc6c01c31cd4b 100644 --- a/sdk/servicebus/azure-messaging-servicebus/pom.xml +++ b/sdk/servicebus/azure-messaging-servicebus/pom.xml @@ -69,7 +69,7 @@ com.azure azure-core-amqp - 2.12.0 + 2.13.0-beta.1 com.azure From 4961959eda839810f6d0327940eb3d4960769772 Mon Sep 17 00:00:00 2001 From: Eldert Grootenboer Date: Fri, 10 Jul 2026 17:56:53 -0700 Subject: [PATCH 30/39] Apply spotless formatting to ServiceBusSenderAsyncClient --- .../messaging/servicebus/ServiceBusSenderAsyncClient.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java index 9f7512af3ebdd..08be2685f9255 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java @@ -935,8 +935,8 @@ private Mono sendMessageInternal(ServiceBusMessage message, ServiceBusTran final CreateMessageBatchOptions batchOptions = new CreateMessageBatchOptions().setMaximumSizeInBytes(batchSize); return Flux.fromIterable(messageList) - .collect(new AmqpMessageCollector(isV2, batchOptions, 1, link::getErrorContext, tracer, - messageSerializer)); + .collect( + new AmqpMessageCollector(isV2, batchOptions, 1, link::getErrorContext, tracer, messageSerializer)); }); // The raw send pipeline, without span instrumentation. Instrumentation is applied as the From 83023614adb73bea5490421ae1f021548dd498d2 Mon Sep 17 00:00:00 2001 From: Eldert Grootenboer Date: Mon, 27 Jul 2026 12:29:26 -0700 Subject: [PATCH 31/39] style: wrap over-length comment in RecoveryKindTest - Wraps a 125-char comment onto two lines to stay within the repo's 120-char convention. --- .../com/azure/core/amqp/implementation/RecoveryKindTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java index d123a2c8963c7..92038c63b3651 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java @@ -194,7 +194,8 @@ void illegalStateExceptionDisposedMessageReturnsLink() { @Test void illegalStateExceptionDisposedDataBatchReturnsLink() { - // Matches ReactorSender.send(List) message: "connectionId[%s] linkName[%s] Cannot publish data batch when disposed." + // Matches ReactorSender.send(List) message: + // "connectionId[%s] linkName[%s] Cannot publish data batch when disposed." assertEquals(RecoveryKind.LINK, RecoveryKind.classify( new IllegalStateException("connectionId[abc] linkName[xyz] Cannot publish data batch when disposed."))); } From 56fa8bba468c13aa286b67cb562209e11bdfd186 Mon Sep 17 00:00:00 2001 From: Eldert Grootenboer Date: Mon, 27 Jul 2026 12:44:48 -0700 Subject: [PATCH 32/39] fix: space session-manager link/connection retries by the configured backoff - Adds a retryBackoff field to ServiceBusSessionManager, derived from AmqpRetryOptions.getDelay() with a 100ms fallback, matching ServiceBusSessionAcquirer. - The LINK/CONNECTION retry branch now delays by that backoff instead of Duration.ZERO, so a link that repeatedly detaches quickly no longer spins the retry loop. --- .../servicebus/ServiceBusSessionManager.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java index b5299fe2776ad..400a079b487ca 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java @@ -72,6 +72,9 @@ class ServiceBusSessionManager implements AutoCloseable, IServiceBusSessionManag private final Deque availableSchedulers = new ConcurrentLinkedDeque<>(); private final Duration maxSessionLockRenewDuration; private final Duration sessionIdleTimeout; + // Backoff between retries for link/connection-level failures, which can fail fast and would otherwise + // spin in a tight, CPU-burning loop. Honors the configured AmqpRetryOptions delay. + private final Duration retryBackoff; /** * SessionId to receiver mapping. @@ -117,6 +120,13 @@ class ServiceBusSessionManager implements AutoCloseable, IServiceBusSessionManag this.sessionIdleTimeout = receiverOptions.getSessionIdleTimeout() != null ? receiverOptions.getSessionIdleTimeout() : connectionCacheWrapper.getRetryOptions().getTryTimeout(); + // Link/connection-level acquire failures can fail fast (e.g. the broker repeatedly detaches the + // link quickly), so space those retries by the configured retry delay rather than retrying with a + // zero delay, which spins the retry loop. Falls back to 100ms when no usable delay is configured. + final Duration configuredDelay = connectionCacheWrapper.getRetryOptions().getDelay(); + this.retryBackoff = (configuredDelay == null || configuredDelay.isZero() || configuredDelay.isNegative()) + ? Duration.ofMillis(100) + : configuredDelay; } ServiceBusSessionManager(String entityPath, MessagingEntityType entityType, @@ -313,8 +323,9 @@ Mono getActiveLink() { // the broker waited for N seconds (60 sec hard limit today) but there was no free or new session. return Mono.delay(Duration.ZERO); } else if (kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION) { - // Link or connection-level error — retry to acquire a fresh link (or connection). - return Mono.delay(Duration.ZERO); + // Link or connection-level error — retry to acquire a fresh link (or connection) after a + // bounded backoff, so a link that repeatedly detaches quickly does not spin the retry loop. + return Mono.delay(retryBackoff); } else { final long id = System.nanoTime(); LOGGER.atInfo().addKeyValue(TRACKING_ID_KEY, id).log("Unable to acquire new session.", failure); From e6c96e06e0b97e20a5e2e1853c1ca158fda9de13 Mon Sep 17 00:00:00 2001 From: Eldert Grootenboer Date: Wed, 28 Oct 2026 15:29:54 -0700 Subject: [PATCH 33/39] fix: restore in-flight AMQP dependency for tiered recovery - Use the co-built azure-core-amqp version and current marker. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- sdk/servicebus/azure-messaging-servicebus/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/pom.xml b/sdk/servicebus/azure-messaging-servicebus/pom.xml index c1e8fa6c21f88..6ff2000b712e3 100644 --- a/sdk/servicebus/azure-messaging-servicebus/pom.xml +++ b/sdk/servicebus/azure-messaging-servicebus/pom.xml @@ -69,7 +69,7 @@ com.azure azure-core-amqp - 2.13.0 + 2.14.0-beta.1 com.azure From 46f8acc1b3bcb0639e568726d62fc92e48699ebc Mon Sep 17 00:00:00 2001 From: Eldert Grootenboer Date: Wed, 28 Oct 2026 16:53:19 -0700 Subject: [PATCH 34/39] fix: align management playback with supported azure-core - Use azure-core 1.61.0 for the HTTP Netty 1.17.0 test transport. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- sdk/servicebus/azure-resourcemanager-servicebus/pom.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sdk/servicebus/azure-resourcemanager-servicebus/pom.xml b/sdk/servicebus/azure-resourcemanager-servicebus/pom.xml index 373c0bf21edaa..820f6ebabf329 100644 --- a/sdk/servicebus/azure-resourcemanager-servicebus/pom.xml +++ b/sdk/servicebus/azure-resourcemanager-servicebus/pom.xml @@ -55,6 +55,11 @@ + + com.azure + azure-core + 1.61.0 + com.azure.resourcemanager azure-resourcemanager-resources From 60ca59abd6f4114b983a808de1478d0e70b34edf Mon Sep 17 00:00:00 2001 From: Eldert Grootenboer Date: Thu, 29 Oct 2026 09:21:20 -0700 Subject: [PATCH 35/39] fix: preserve recovery ownership and retry failed acquisitions - Target connection recovery and cleanup to the failed operation - Preserve terminal session errors and recover aborted sends - Normalize negotiation failures and refresh unnamed session links - Cover stale callbacks, delayed errors, and non-completing close Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../implementation/AmqpChannelProcessor.java | 35 +++- .../ReactorConnectionCache.java | 17 ++ .../amqp/implementation/RecoveryKind.java | 7 +- .../AmqpChannelProcessorTest.java | 46 ++++++ .../ReactorConnectionCacheTest.java | 7 +- .../implementation/ReactorSenderTest.java | 16 ++ .../amqp/implementation/RecoveryKindTest.java | 15 ++ .../servicebus/ConnectionCacheWrapper.java | 14 +- .../messaging/servicebus/RecoveryUtils.java | 8 + .../ServiceBusReceiverAsyncClient.java | 33 ++-- .../ServiceBusSenderAsyncClient.java | 110 +++++++------ .../servicebus/ServiceBusSessionAcquirer.java | 116 ++++++++------ .../servicebus/ServiceBusSessionManager.java | 149 ++++++++++-------- .../ServiceBusReceiverAsyncClientTest.java | 35 ++++ ...SenderAsyncClientRecoveryIsolatedTest.java | 121 ++++++++++++++ ...ServiceBusSessionAcquirerIsolatedTest.java | 49 ++++++ .../ServiceBusSessionManagerTest.java | 49 ++++++ 17 files changed, 636 insertions(+), 191 deletions(-) diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/AmqpChannelProcessor.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/AmqpChannelProcessor.java index 4f23f86e92dae..764b998b02507 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/AmqpChannelProcessor.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/AmqpChannelProcessor.java @@ -140,15 +140,19 @@ public void onNext(T amqpChannel) { logger.info("Channel is now active."); } }, error -> { - setAndClearChannel(); - onError(error); + if (setAndClearChannel(amqpChannel)) { + onError(error); + } else { + logger.verbose("Ignoring error from a replaced channel.", error); + } }, () -> { if (isDisposed()) { logger.info("Channel is disposed."); } else { - logger.info("Channel is closed. Requesting upstream."); - setAndClearChannel(); - requestUpstream(); + if (setAndClearChannel(amqpChannel)) { + logger.info("Channel is closed. Requesting upstream."); + requestUpstream(); + } } }); } @@ -356,6 +360,27 @@ public void forceCloseChannel() { setAndClearChannel(); } + /** + * Closes the failed channel only if it is still the current channel. + * + * @param failedChannel The channel used by the failed operation. + */ + public void forceCloseChannel(T failedChannel) { + Objects.requireNonNull(failedChannel, "'failedChannel' cannot be null."); + setAndClearChannel(failedChannel); + } + + private boolean setAndClearChannel(T failedChannel) { + synchronized (lock) { + if (currentChannel != failedChannel) { + return false; + } + currentChannel = null; + } + close(failedChannel); + return true; + } + /** * Checks the current state of the channel for this channel and returns true if the channel is null or if this * processor is disposed. diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java index f153b4da2f46c..b9f2be1215262 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnectionCache.java @@ -215,6 +215,23 @@ public void invalidateConnection() { } } + /** + * Marks the failed connection for invalidation only if it is still cached. + * A delayed failure must not invalidate a healthy replacement connection. + * + * @param connectionId The identifier of the connection used by the failed operation. + */ + public void invalidateConnection(String connectionId) { + Objects.requireNonNull(connectionId, "'connectionId' cannot be null."); + synchronized (lock) { + final T connection = currentConnection; + if (connection != null && connectionId.equals(connection.getId()) && !connection.isDisposed()) { + withConnectionId(logger, connectionId).log("Marking failed connection for force-invalidation."); + forceInvalidateConnectionId.set(connectionId); + } + } + } + /** * Terminate so that consumers will no longer be able to request connection. If there is a current (cached) * connection then it will be closed. diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java index df3dc4c1d651f..f7c6c9b9a6a1e 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/RecoveryKind.java @@ -5,6 +5,7 @@ import com.azure.core.amqp.exception.AmqpErrorCondition; import com.azure.core.amqp.exception.AmqpException; +import com.azure.core.amqp.exception.OperationCancelledException; import java.util.Locale; import java.util.concurrent.TimeoutException; @@ -65,6 +66,10 @@ public static RecoveryKind classify(Throwable error) { return NONE; } + if (error instanceof OperationCancelledException) { + return LINK; + } + if (error instanceof AmqpException) { final AmqpException amqpError = (AmqpException) error; final AmqpErrorCondition condition = amqpError.getErrorCondition(); @@ -104,6 +109,7 @@ public static RecoveryKind classify(Throwable error) { case ILLEGAL_STATE: case MESSAGE_LOCK_LOST: case STORE_LOCK_LOST_ERROR: + case SESSION_CANNOT_BE_LOCKED: return FATAL; // Server-busy, timeouts, and resource-limit errors — retry on same link. @@ -118,7 +124,6 @@ public static RecoveryKind classify(Throwable error) { // Session lock loss means the session link is invalid and // a fresh link must be acquired for a new session. case SESSION_LOCK_LOST: - case SESSION_CANNOT_BE_LOCKED: case SESSION_NOT_FOUND: case MESSAGE_NOT_FOUND: return LINK; diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/AmqpChannelProcessorTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/AmqpChannelProcessorTest.java index 6f6a351ab7766..e81c75c9d90bc 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/AmqpChannelProcessorTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/AmqpChannelProcessorTest.java @@ -10,12 +10,15 @@ import com.azure.core.amqp.exception.AmqpErrorCondition; import com.azure.core.amqp.exception.AmqpErrorContext; import com.azure.core.amqp.exception.AmqpException; +import com.azure.core.util.AsyncCloseable; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; import org.reactivestreams.Subscription; import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; +import reactor.core.publisher.Sinks; import reactor.test.StepVerifier; import reactor.test.publisher.TestPublisher; @@ -30,7 +33,12 @@ import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; /** * Tests for {@link AmqpChannelProcessor}. @@ -353,10 +361,48 @@ private static AmqpChannelProcessor createChannelProcessor() { return createChannelProcessor(new FixedAmqpRetryPolicy(new AmqpRetryOptions())); } + @Test + void targetedClosePreservesReplacementChannel() { + final AmqpChannelProcessor processor = createChannelProcessor(); + try { + processor.onNext(connection1); + processor.onNext(connection2); + processor.forceCloseChannel(connection1); + StepVerifier.create(processor).expectNext(connection2).verifyComplete(); + processor.forceCloseChannel(connection2); + assertTrue(processor.isChannelClosed()); + } finally { + processor.dispose(); + } + } + private static AmqpChannelProcessor createChannelProcessor(AmqpRetryPolicy retryPolicy) { return new AmqpChannelProcessor<>("namespace-test", TestObject::getStates, retryPolicy, new HashMap<>()); } + @Test + void closingOldChannelMustNotClearReplacement() { + final AsyncCloseable first = mock(AsyncCloseable.class); + final AsyncCloseable replacement = mock(AsyncCloseable.class); + final Sinks.Many states = Sinks.many().replay().latestOrDefault(AmqpEndpointState.ACTIVE); + when(first.closeAsync()).thenAnswer(invocation -> { + states.tryEmitComplete(); + return Mono.empty(); + }); + when(replacement.closeAsync()).thenReturn(Mono.empty()); + final AmqpChannelProcessor processor + = new AmqpChannelProcessor<>("namespace-test", channel -> channel == first ? states.asFlux() : Flux.never(), + new FixedAmqpRetryPolicy(new AmqpRetryOptions()), new HashMap<>()); + try { + processor.onNext(first); + processor.onNext(replacement); + assertFalse(processor.isChannelClosed()); + verify(replacement, never()).closeAsync(); + } finally { + processor.dispose(); + } + } + static final class TestObject { private final TestPublisher processor = TestPublisher.createCold(); diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionCacheTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionCacheTest.java index 2016d3fe0260a..e6f18d2e81146 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionCacheTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionCacheTest.java @@ -243,8 +243,8 @@ public void invalidateConnectionReplacesCachedConnectionOnNextGet() { .verify(VERIFY_TIMEOUT); connectionSupplier.assertInvocationCount(1); - // Mark the current connection for force-invalidation. - connectionCache.invalidateConnection(); + // Mark the connection used by the failed operation. + connectionCache.invalidateConnection(c[0].getId()); // Next subscription must close the marked connection and serve a brand-new one. StepVerifier.create(connectionMono, 0) @@ -303,6 +303,9 @@ public void invalidateConnectionDoesNotInvalidateNewlyCreatedConnection() { .expectComplete() .verify(VERIFY_TIMEOUT); + // A delayed error from the old connection must not evict its replacement. + connectionCache.invalidateConnection(c[0].getId()); + // The marker has been consumed; another get() must serve the same (cached) replacement // connection without creating a third one. StepVerifier.create(connectionMono, 0).thenRequest(1).expectNextMatches(con -> { diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorSenderTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorSenderTest.java index f05c9c4b9b208..1b4f51ab58173 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorSenderTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorSenderTest.java @@ -42,6 +42,7 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; +import reactor.core.Exceptions; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.core.scheduler.Scheduler; @@ -186,6 +187,21 @@ public void testLinkSize() { verify(sender).getRemoteMaxMessageSize(); } + @Test + public void linkSizeNegotiationPreservesDetachAsRetryExhaustionCause() { + options.setMaxRetries(0); + reactorSender = spy(new ReactorSender(amqpConnection, ENTITY_PATH, sender, handler, reactorProvider, + tokenManager, messageSerializer, options, scheduler, AmqpMetricsProvider.noop())); + final AmqpException detach + = new AmqpException(true, AmqpErrorCondition.LINK_DETACH_FORCED, "detached during negotiation", null); + doReturn(Flux.error(detach).cache(1)).when(reactorSender).getEndpointStates(); + + StepVerifier.create(reactorSender.getLinkSize()).expectErrorSatisfies(error -> { + assertTrue(Exceptions.isRetryExhausted(error)); + Assertions.assertSame(detach, error.getCause()); + }).verify(VERIFY_TIMEOUT); + } + /** * Verifies that getMaxBatchSize reads the vendor property com.microsoft:max-message-batch-size from the link's * remote properties and returns its value. diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java index 92038c63b3651..a3ed21a9eacf1 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/RecoveryKindTest.java @@ -5,6 +5,7 @@ import com.azure.core.amqp.exception.AmqpErrorCondition; import com.azure.core.amqp.exception.AmqpException; +import com.azure.core.amqp.exception.OperationCancelledException; import org.junit.jupiter.api.Test; import java.util.concurrent.TimeoutException; @@ -178,6 +179,20 @@ void operationCancelledReturnsLink() { assertEquals(RecoveryKind.LINK, RecoveryKind.classify(error)); } + @Test + void abortedSendWithoutErrorConditionReturnsLink() { + assertEquals(RecoveryKind.LINK, RecoveryKind.classify(new OperationCancelledException("Released", null))); + assertEquals(RecoveryKind.LINK, RecoveryKind.classify( + new OperationCancelledException("Delivery advance failed", new IllegalStateException("advance"), null))); + } + + @Test + void lockedNamedSessionReturnsFatal() { + final AmqpException error = new AmqpException(false, AmqpErrorCondition.SESSION_CANNOT_BE_LOCKED, + "The requested session is already locked.", null); + assertEquals(RecoveryKind.FATAL, RecoveryKind.classify(error)); + } + @Test void resourceLimitExceededReturnsNone() { final AmqpException error diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java index 282b846b4f42e..870b3f74b4b24 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ConnectionCacheWrapper.java @@ -53,16 +53,16 @@ boolean isChannelClosed() { } /** - * Invalidates the current cached connection for connection-level recovery when the - * connection is stale but the cache has not detected it via endpoint state signals. - * On v2, marks the connection for invalidation so the next get() closes it and creates - * a fresh one. On v1, force-closes the current channel immediately. + * Invalidates the connection used by the failed operation without disturbing a replacement. + * On v2, marks that connection for eviction on the next get(). On v1, closes it immediately. + * + * @param connection The connection used by the failed operation. */ - void invalidateConnection() { + void invalidateConnection(ServiceBusAmqpConnection connection) { if (isV2) { - cache.invalidateConnection(); + cache.invalidateConnection(connection.getId()); } else { - processor.forceCloseChannel(); + processor.forceCloseChannel(connection); } } } diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java index dd1d53ce7ff12..d7c965d0a94c0 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java @@ -5,6 +5,7 @@ import com.azure.core.amqp.exception.AmqpException; import com.azure.core.amqp.implementation.RecoveryKind; +import reactor.core.Exceptions; import java.util.concurrent.TimeoutException; @@ -17,6 +18,13 @@ final class RecoveryUtils { private RecoveryUtils() { } + static Throwable unwrapRetryExhaustion(Throwable error) { + while (Exceptions.isRetryExhausted(error) && error.getCause() != null) { + error = error.getCause(); + } + return error; + } + /** * Ensures the error is retriable by the standard * {@link com.azure.core.amqp.implementation.RetryUtil#createRetry} filter. diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java index ddc4f105b2338..83e3096c78723 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java @@ -1714,9 +1714,14 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { // The Mono, when subscribed, creates a ServiceBusReceiveLink in the ServiceBusAmqpConnection emitted by the connectionProcessor // - final Mono receiveLinkMono = connectionProcessor.flatMap(connection -> { - return connection.createReceiveLink(linkName, entityPath, receiverOptions.getReceiveMode(), null, - entityType, identifier); + final AtomicReference operationConnection = new AtomicReference<>(); + final Mono receiveLinkMono = Mono.defer(() -> { + operationConnection.set(null); + return connectionProcessor.flatMap(connection -> { + operationConnection.set(connection); + return connection.createReceiveLink(linkName, entityPath, receiverOptions.getReceiveMode(), null, + entityType, identifier); + }); }).doOnNext(next -> { LOGGER.atVerbose() .addKeyValue(LINK_NAME_KEY, linkName) @@ -1748,26 +1753,28 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { // For both LINK and CONNECTION recovery, the session hosting the failed link may // be stale. Ask the connection to remove it so the next retry creates a fresh - // session + link. CONNECTION recovery additionally invalidates the cached - // connection below so the next retry rebuilds connection, session, and link. - Mono recovery = connectionProcessor.flatMap(connection -> { - final boolean removed = connection.removeSession(entityPath); + // session + link. CONNECTION recovery first invalidates that connection so the + // next retry rebuilds connection, session, and link. + final ServiceBusAmqpConnection failedConnection = operationConnection.getAndSet(null); + Mono recovery = Mono.fromRunnable(() -> { + if (failedConnection == null) { + return; + } + if (recoveryKind == RecoveryKind.CONNECTION) { + connectionCacheWrapper.invalidateConnection(failedConnection); + } + final boolean removed = failedConnection.removeSession(entityPath); LOGGER.atVerbose() .addKeyValue(LINK_NAME_KEY, linkName) .addKeyValue("sessionRemoved", removed) .log("Stale session removal during {} recovery.", recoveryKind); - return Mono.empty(); }).onErrorResume(error -> { LOGGER.atWarning() .addKeyValue(LINK_NAME_KEY, linkName) - .log("Error obtaining connection during {} recovery.", recoveryKind, error); + .log("Error removing stale session during {} recovery.", recoveryKind, error); return Mono.empty(); }); - if (recoveryKind == RecoveryKind.CONNECTION) { - recovery - = recovery.then(Mono.fromRunnable(() -> connectionCacheWrapper.invalidateConnection())); - } // Use the shared retry-filter wrapper so non-AMQP errors classified as // LINK/CONNECTION (e.g., IllegalStateException) are wrapped as transient // AmqpException and accepted by RetryUtil.createRetry. Centralising this diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java index 08be2685f9255..ee18d049231d4 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClient.java @@ -882,25 +882,28 @@ private Mono sendBatchInternal(ServiceBusMessageBatch batch, ServiceBusTra messages.add(message); }); - final AtomicReference operationLink = new AtomicReference<>(); - final Mono sendMessage = getSendLink("send-batch").flatMap(link -> { - operationLink.set(link); - if (transactionContext != null && transactionContext.getTransactionId() != null) { - final TransactionalState deliveryState = new TransactionalState(); - deliveryState.setTxnId(Binary.create(transactionContext.getTransactionId())); - return messages.size() == 1 - ? link.send(messages.get(0), deliveryState) - : link.send(messages, deliveryState); - } else { - return messages.size() == 1 ? link.send(messages.get(0)) : link.send(messages); - } - }); + final Mono withRetry = Mono.defer(() -> { + final AtomicReference operationLink = new AtomicReference<>(); + final AtomicReference operationConnection = new AtomicReference<>(); + final Mono sendMessage = getSendLink("send-batch", operationConnection).flatMap(link -> { + operationLink.set(link); + if (transactionContext != null && transactionContext.getTransactionId() != null) { + final TransactionalState deliveryState = new TransactionalState(); + deliveryState.setTxnId(Binary.create(transactionContext.getTransactionId())); + return messages.size() == 1 + ? link.send(messages.get(0), deliveryState) + : link.send(messages, deliveryState); + } else { + return messages.size() == 1 ? link.send(messages.get(0)) : link.send(messages); + } + }); - final String timeoutMessage = "Sending messages timed out. message-count:" + batch.getCount() + entityId(); - final Mono withRetry = RetryUtil.withRetry( - sendMessage.onErrorResume( - e -> recoverBeforeRetry(e, "sendBatch", operationLink).then(Mono.error(RecoveryUtils.asRetriable(e)))), - retryOptions, timeoutMessage).onErrorMap(this::mapError); + final String timeoutMessage = "Sending messages timed out. message-count:" + batch.getCount() + entityId(); + return RetryUtil.withRetry(sendMessage.onErrorMap(RecoveryUtils::unwrapRetryExhaustion) + .onErrorResume(e -> recoverBeforeRetry(e, "sendBatch", operationLink, operationConnection) + .then(Mono.error(RecoveryUtils.asRetriable(e)))), + retryOptions, timeoutMessage); + }).onErrorMap(this::mapError); // The single-message send path (sendMessageInternal) starts the "ServiceBus.send" span itself on the // subscribing thread, so it sends this batch with instrument=false to avoid a duplicate span. if (!instrument) { @@ -957,14 +960,19 @@ private Mono sendMessageInternal(ServiceBusMessage message, ServiceBusTran }); } - private Mono getSendLink(String callSite) { - final Mono getSendLink = connectionProcessor.flatMap(connection -> { - if (!CoreUtils.isNullOrEmpty(viaEntityName)) { - return connection.createSendLink("VIA-".concat(viaEntityName), viaEntityName, retryOptions, entityName, - identifier); - } else { - return connection.createSendLink(entityName, entityName, retryOptions, null, identifier); - } + private Mono getSendLink(String callSite, + AtomicReference operationConnection) { + final Mono getSendLink = Mono.defer(() -> { + operationConnection.set(null); + return connectionProcessor.flatMap(connection -> { + operationConnection.set(connection); + if (!CoreUtils.isNullOrEmpty(viaEntityName)) { + return connection.createSendLink("VIA-".concat(viaEntityName), viaEntityName, retryOptions, + entityName, identifier); + } else { + return connection.createSendLink(entityName, entityName, retryOptions, null, identifier); + } + }); }) .doOnNext(link -> linkName.compareAndSet(null, link.getLinkName())) .onErrorMap(RequestResponseChannelClosedException.class, e -> { @@ -995,13 +1003,18 @@ private Mono getSendLink(String callSite) { * @return a Mono emitting (link, size) on success, retried with recovery on transient failures. */ private Mono> getSendLinkAndSizeWithRetry(String callSite) { - final AtomicReference operationLink = new AtomicReference<>(); - return RetryUtil.withRetry( - getSendLink(callSite).doOnNext(operationLink::set) - .flatMap(link -> link.getLinkSize().map(size -> Tuples.of(link, size))) - .onErrorResume(e -> recoverBeforeRetry(e, "getSendLinkAndSize-" + callSite, operationLink) - .then(Mono.error(RecoveryUtils.asRetriable(e)))), - retryOptions, String.format(retryGetLinkErrorMessageFormat, callSite)); + return Mono.defer(() -> { + final AtomicReference operationLink = new AtomicReference<>(); + final AtomicReference operationConnection = new AtomicReference<>(); + return RetryUtil + .withRetry( + getSendLink(callSite, operationConnection).doOnNext(operationLink::set) + .flatMap(link -> link.getLinkSize().map(size -> Tuples.of(link, size))) + .onErrorMap(RecoveryUtils::unwrapRetryExhaustion) + .onErrorResume(e -> recoverBeforeRetry(e, "getSendLinkAndSize-" + callSite, operationLink, + operationConnection).then(Mono.error(RecoveryUtils.asRetriable(e)))), + retryOptions, String.format(retryGetLinkErrorMessageFormat, callSite)); + }); } /** @@ -1017,13 +1030,17 @@ private Mono> getSendLinkAndSizeWithRetry(String c * @return a Mono emitting (link, batchSize) on success, retried with recovery on transient failures. */ private Mono> getSendLinkAndMaxBatchSizeWithRetry(String callSite) { - final AtomicReference operationLink = new AtomicReference<>(); - return RetryUtil.withRetry( - getSendLink(callSite).doOnNext(operationLink::set) - .flatMap(link -> link.getMaxBatchSize().map(size -> Tuples.of(link, size))) - .onErrorResume(e -> recoverBeforeRetry(e, "getSendLinkAndMaxBatchSize-" + callSite, operationLink) - .then(Mono.error(RecoveryUtils.asRetriable(e)))), - retryOptions, String.format(retryGetLinkErrorMessageFormat, callSite)); + return Mono.defer(() -> { + final AtomicReference operationLink = new AtomicReference<>(); + final AtomicReference operationConnection = new AtomicReference<>(); + return RetryUtil.withRetry( + getSendLink(callSite, operationConnection).doOnNext(operationLink::set) + .flatMap(link -> link.getMaxBatchSize().map(size -> Tuples.of(link, size))) + .onErrorMap(RecoveryUtils::unwrapRetryExhaustion) + .onErrorResume(e -> recoverBeforeRetry(e, "getSendLinkAndMaxBatchSize-" + callSite, operationLink, + operationConnection).then(Mono.error(RecoveryUtils.asRetriable(e)))), + retryOptions, String.format(retryGetLinkErrorMessageFormat, callSite)); + }); } private Throwable mapError(Throwable throwable) { @@ -1035,13 +1052,14 @@ private Throwable mapError(Throwable throwable) { /** * Performs reactive tiered recovery based on the classified error. For LINK recovery, closes the - * send link so the next retry creates a fresh one. For CONNECTION recovery, closes the link and - * invalidates the connection so the cache creates everything fresh. + * send link so the next retry creates a fresh one. For CONNECTION recovery, invalidates the + * operation's connection before awaiting link closure so a stalled close cannot prevent eviction. * *

Called from {@code onErrorResume} at each send call site so that recovery completes before * the error is re-emitted and the standard retry logic re-subscribes.

*/ - private Mono recoverBeforeRetry(Throwable error, String callSite, AtomicReference linkRef) { + private Mono recoverBeforeRetry(Throwable error, String callSite, AtomicReference linkRef, + AtomicReference connectionRef) { final RecoveryKind kind = RecoveryKind.classify(error); if (kind != RecoveryKind.LINK && kind != RecoveryKind.CONNECTION) { return Mono.empty(); @@ -1075,7 +1093,11 @@ private Mono recoverBeforeRetry(Throwable error, String callSite, AtomicRe // For CONNECTION errors, invalidate the cached connection so the next get() closes it // and creates a fresh one. Matches Go SDK's Namespace.Recover(). if (kind == RecoveryKind.CONNECTION) { - recovery = recovery.then(Mono.fromRunnable(() -> connectionCacheWrapper.invalidateConnection())); + final ServiceBusAmqpConnection failedConnection = connectionRef.getAndSet(null); + if (failedConnection != null) { + recovery = Mono.fromRunnable(() -> connectionCacheWrapper.invalidateConnection(failedConnection)) + .then(recovery); + } } return recovery; diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java index 26fe59244c6d3..129682f7a26af 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java @@ -12,6 +12,7 @@ import com.azure.core.amqp.implementation.RecoveryKind; import com.azure.core.util.logging.ClientLogger; import com.azure.messaging.servicebus.implementation.MessagingEntityType; +import com.azure.messaging.servicebus.implementation.ServiceBusAmqpConnection; import com.azure.messaging.servicebus.implementation.ServiceBusManagementNode; import com.azure.messaging.servicebus.implementation.ServiceBusReceiveLink; import com.azure.messaging.servicebus.implementation.ServiceBusReceiveLink.SessionProperties; @@ -27,6 +28,7 @@ import java.time.OffsetDateTime; import java.util.Objects; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import static com.azure.core.amqp.implementation.ClientConstants.ENTITY_PATH_KEY; @@ -151,68 +153,80 @@ Mono acquire(String sessionId) { * is already disposed or {@link TimeoutException} if session acquire timeouts and {@code timeoutRetryDisabled} set to true. */ private Mono acquireIntern(String sessionId) { - if (timeoutRetryDisabled) { - return acquireSession(sessionId).timeout(clientSideTimeout, - Mono.error(() -> new TimeoutException("Timed out waiting to acquire a session; the broker did not " - + "respond within " + clientSideTimeout + " (client-side guard)."))) - .onErrorResume(t -> { - if (isBrokerTimeoutError(t)) { - // map the broker timeout to application-friendly TimeoutException. - final Throwable e = new TimeoutException("com.microsoft:timeout").initCause(t); - return publishError(sessionId, e, false); - } - if (t instanceof TimeoutException) { - // Client-side guard fired because the broker never responded (a hung acquire). - // Treat it like the broker timeout: a retriable "no session acquired in time". - return publishError(sessionId, t, false); - } - return publishError(sessionId, t, true); - }); - } else { - return acquireSession(sessionId).timeout(tryTimeout) - .retryWhen(Retry.from(signals -> signals.flatMap(signal -> { - final Throwable t = signal.failure(); - final RecoveryKind kind = RecoveryKind.classify(t); - if (kind == RecoveryKind.CONNECTION) { - logger.atWarning() - .addKeyValue(ENTITY_PATH_KEY, entityPath) - .log("Connection-level error acquiring session, forcing connection recovery.", t); - connectionCacheWrapper.invalidateConnection(); - return Mono.delay(Duration.ZERO); - } - if (isTimeoutError(t)) { - logger.atVerbose() - .addKeyValue(ENTITY_PATH_KEY, entityPath) - .addKeyValue("attempt", signal.totalRetriesInARow()) - .log("Timeout while acquiring session '{}'.", sessionName(sessionId), t); - // Retry session acquire after a bounded backoff. Mono.delay hops to - // Schedulers.parallel(), freeing the QPid thread; the non-zero backoff prevents a - // tight CPU-burning loop when acquire attempts fail fast. - return Mono.delay(retryBackoff); - } - if (kind == RecoveryKind.LINK) { - logger.atWarning() - .addKeyValue(ENTITY_PATH_KEY, entityPath) - .log("Link-level error acquiring session, retrying.", t); - // Retry link-level failures after the same bounded backoff so repeated fast - // link detaches do not spin in a tight CPU-burning loop. - return Mono.delay(retryBackoff); - } - return publishError(sessionId, t, true); - }))); - } + return Mono.defer(() -> { + final AtomicReference operationConnection = new AtomicReference<>(); + if (timeoutRetryDisabled) { + return acquireSession(sessionId, operationConnection) + .timeout(clientSideTimeout, + Mono.error( + () -> new TimeoutException("Timed out waiting to acquire a session; the broker did not " + + "respond within " + clientSideTimeout + " (client-side guard)."))) + .onErrorResume(t -> { + if (isBrokerTimeoutError(t)) { + // map the broker timeout to application-friendly TimeoutException. + final Throwable e = new TimeoutException("com.microsoft:timeout").initCause(t); + return publishError(sessionId, e, false); + } + if (t instanceof TimeoutException) { + // Client-side guard fired because the broker never responded (a hung acquire). + // Treat it like the broker timeout: a retriable "no session acquired in time". + return publishError(sessionId, t, false); + } + return publishError(sessionId, t, true); + }); + } else { + return acquireSession(sessionId, operationConnection).timeout(tryTimeout) + .retryWhen(Retry.from(signals -> signals.flatMap(signal -> { + final Throwable t = signal.failure(); + final RecoveryKind kind = RecoveryKind.classify(t); + if (kind == RecoveryKind.CONNECTION) { + logger.atWarning() + .addKeyValue(ENTITY_PATH_KEY, entityPath) + .log("Connection-level error acquiring session, forcing connection recovery.", t); + final ServiceBusAmqpConnection failedConnection = operationConnection.getAndSet(null); + if (failedConnection != null) { + connectionCacheWrapper.invalidateConnection(failedConnection); + } + return Mono.delay(Duration.ZERO); + } + if (isTimeoutError(t)) { + logger.atVerbose() + .addKeyValue(ENTITY_PATH_KEY, entityPath) + .addKeyValue("attempt", signal.totalRetriesInARow()) + .log("Timeout while acquiring session '{}'.", sessionName(sessionId), t); + // Retry session acquire after a bounded backoff. Mono.delay hops to + // Schedulers.parallel(), freeing the QPid thread; the non-zero backoff prevents a + // tight CPU-burning loop when acquire attempts fail fast. + return Mono.delay(retryBackoff); + } + if (kind == RecoveryKind.LINK) { + logger.atWarning() + .addKeyValue(ENTITY_PATH_KEY, entityPath) + .log("Link-level error acquiring session, retrying.", t); + // Retry link-level failures after the same bounded backoff so repeated fast + // link detaches do not spin in a tight CPU-burning loop. + return Mono.delay(retryBackoff); + } + return publishError(sessionId, t, true); + }))); + } + }); } /** * Tries to acquire a session from the broker by opening an AMQP receive link. * * @param sessionId the unique id of the session to acquire, a value {@code null} means acquire any free session. + * @param operationConnection captures the connection used by the current acquisition attempt. * * @return the acquired session. */ - private Mono acquireSession(String sessionId) { + private Mono acquireSession(String sessionId, + AtomicReference operationConnection) { return Mono.defer(() -> { + operationConnection.set(null); final Mono createLink = connectionCacheWrapper.getConnection() + .doOnNext(operationConnection::set) .flatMap(connection -> connection.createReceiveLink(linkName(sessionId), entityPath, receiveMode, null, entityType, identifier, sessionId)); return createLink.flatMap(link -> { diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java index 400a079b487ca..05128a2f2e041 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java @@ -14,6 +14,7 @@ import com.azure.messaging.servicebus.implementation.DispositionStatus; import com.azure.messaging.servicebus.implementation.MessageUtils; import com.azure.messaging.servicebus.implementation.MessagingEntityType; +import com.azure.messaging.servicebus.implementation.ServiceBusAmqpConnection; import com.azure.messaging.servicebus.implementation.ServiceBusManagementNode; import com.azure.messaging.servicebus.implementation.ServiceBusReceiveLink; import com.azure.messaging.servicebus.implementation.instrumentation.ServiceBusTracer; @@ -37,6 +38,7 @@ import java.util.concurrent.ConcurrentLinkedDeque; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -253,15 +255,20 @@ private AmqpErrorContext getErrorContext() { /** * Creates an session receive link. * + * @param operationConnection captures the connection used by the current acquisition attempt. * @return A Mono that completes with an session receive link. */ - private Mono createSessionReceiveLink() { + private Mono + createSessionReceiveLink(AtomicReference operationConnection) { final String sessionId = receiverOptions.getSessionId(); - final String linkName = (sessionId != null) ? sessionId : StringUtil.getRandomString("session-"); - return connectionCacheWrapper.getConnection().flatMap(connection -> { - return connection.createReceiveLink(linkName, entityPath, receiverOptions.getReceiveMode(), null, - entityType, identifier, sessionId); + return Mono.defer(() -> { + final String linkName = (sessionId != null) ? sessionId : StringUtil.getRandomString("session-"); + operationConnection.set(null); + return connectionCacheWrapper.getConnection().doOnNext(operationConnection::set).flatMap(connection -> { + return connection.createReceiveLink(linkName, entityPath, receiverOptions.getReceiveMode(), null, + entityType, identifier, sessionId); + }); }); } @@ -275,70 +282,76 @@ Mono getActiveLink() { if (this.receiveLink != null) { return Mono.just(this.receiveLink); } - return Mono.defer(() -> createSessionReceiveLink().flatMap(link -> link.getEndpointStates() - .filter(e -> e == AmqpEndpointState.ACTIVE) - .next() - // The reason for using 'switchIfEmpty' operator - - // - // While waiting for the link to ACTIVE, if the broker detaches the link without an error-condition, - // the link-endpoint-state publisher will transition to completion without ever emitting ACTIVE. Map - // such publisher completion to transient (i.e., retriable) AmqpException to enable retry. - // - // A detach without an error-condition can happen when Service upgrades. Also, while the service often - // detaches with the error-condition 'com.microsoft:timeout' when there is no session, sometimes, - // when a free or new session is unavailable, detach can happen without the error-condition. - // - .switchIfEmpty( - Mono.error(() -> new AmqpException(true, "Session receive link completed without being active", null))) - .timeout(operationTimeout) - .then(Mono.just(link)))).retryWhen(Retry.from(retrySignals -> retrySignals.flatMap(signal -> { - final Throwable failure = signal.failure(); - final RecoveryKind kind = RecoveryKind.classify(failure); - LOGGER.atInfo() - .addKeyValue(ENTITY_PATH_KEY, entityPath) - .addKeyValue("attempt", signal.totalRetriesInARow()) - .addKeyValue("recoveryKind", kind) - .log("Error occurred while getting unnamed session.", failure); - - if (isDisposed.get()) { - return Mono.error( - new AmqpException(false, "SessionManager is already disposed.", failure, getErrorContext())); - } - - if (kind == RecoveryKind.CONNECTION) { - LOGGER.atWarning() + return Mono.defer(() -> { + final AtomicReference operationConnection = new AtomicReference<>(); + return createSessionReceiveLink(operationConnection).flatMap(link -> link.getEndpointStates() + .filter(e -> e == AmqpEndpointState.ACTIVE) + .next() + // The reason for using 'switchIfEmpty' operator - + // + // While waiting for the link to ACTIVE, if the broker detaches the link without an error-condition, + // the link-endpoint-state publisher will transition to completion without ever emitting ACTIVE. Map + // such publisher completion to transient (i.e., retriable) AmqpException to enable retry. + // + // A detach without an error-condition can happen when Service upgrades. Also, while the service often + // detaches with the error-condition 'com.microsoft:timeout' when there is no session, sometimes, + // when a free or new session is unavailable, detach can happen without the error-condition. + // + .switchIfEmpty(Mono + .error(() -> new AmqpException(true, "Session receive link completed without being active", null))) + .timeout(operationTimeout) + .then(Mono.just(link))).retryWhen(Retry.from(retrySignals -> retrySignals.flatMap(signal -> { + final Throwable failure = signal.failure(); + final RecoveryKind kind = RecoveryKind.classify(failure); + LOGGER.atInfo() .addKeyValue(ENTITY_PATH_KEY, entityPath) - .log("Connection-level error in session manager, forcing connection recovery.", failure); - connectionCacheWrapper.invalidateConnection(); - } - - // Mono.delay(Duration.ZERO) instead of Mono.empty() — the delay schedules the retry on the - // 'parallel' Scheduler, freeing the QPid (proton) thread for other IO. Using Mono.empty() would - // block the QPid thread and slow down / block message pumping in other sessions. - if (failure instanceof TimeoutException) { - return Mono.delay(Duration.ZERO); - } else if (failure instanceof AmqpException - && ((AmqpException) failure).getErrorCondition() == AmqpErrorCondition.TIMEOUT_ERROR) { - // The link closed remotely with 'Detach {errorCondition:com.microsoft:timeout}' frame because - // the broker waited for N seconds (60 sec hard limit today) but there was no free or new session. - return Mono.delay(Duration.ZERO); - } else if (kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION) { - // Link or connection-level error — retry to acquire a fresh link (or connection) after a - // bounded backoff, so a link that repeatedly detaches quickly does not spin the retry loop. - return Mono.delay(retryBackoff); - } else { - final long id = System.nanoTime(); - LOGGER.atInfo().addKeyValue(TRACKING_ID_KEY, id).log("Unable to acquire new session.", failure); - // The link-endpoint-state publisher will emit signal on the reactor-executor thread, which is - // non-blocking, if we use the session processor to recover the error, it requires a blocking - // thread to close the client. Hence, we publish the error on the bounded-elastic thread. - return Mono.error(failure) - .publishOn(Schedulers.boundedElastic()) - .doOnError(e -> LOGGER.atInfo() - .addKeyValue(TRACKING_ID_KEY, id) - .log("Emitting the error signal received for session acquire attempt.", e)); - } - }))); + .addKeyValue("attempt", signal.totalRetriesInARow()) + .addKeyValue("recoveryKind", kind) + .log("Error occurred while getting unnamed session.", failure); + + if (isDisposed.get()) { + return Mono.error(new AmqpException(false, "SessionManager is already disposed.", failure, + getErrorContext())); + } + + if (kind == RecoveryKind.CONNECTION) { + LOGGER.atWarning() + .addKeyValue(ENTITY_PATH_KEY, entityPath) + .log("Connection-level error in session manager, forcing connection recovery.", failure); + final ServiceBusAmqpConnection failedConnection = operationConnection.getAndSet(null); + if (failedConnection != null) { + connectionCacheWrapper.invalidateConnection(failedConnection); + } + } + + // Mono.delay(Duration.ZERO) instead of Mono.empty() — the delay schedules the retry on the + // 'parallel' Scheduler, freeing the QPid (proton) thread for other IO. Using Mono.empty() would + // block the QPid thread and slow down / block message pumping in other sessions. + if (failure instanceof TimeoutException) { + return Mono.delay(Duration.ZERO); + } else if (failure instanceof AmqpException + && ((AmqpException) failure).getErrorCondition() == AmqpErrorCondition.TIMEOUT_ERROR) { + // The link closed remotely with 'Detach {errorCondition:com.microsoft:timeout}' frame because + // the broker waited for N seconds (60 sec hard limit today) but there was no free or new session. + return Mono.delay(Duration.ZERO); + } else if (kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION) { + // Link or connection-level error — retry to acquire a fresh link (or connection) after a + // bounded backoff, so a link that repeatedly detaches quickly does not spin the retry loop. + return Mono.delay(retryBackoff); + } else { + final long id = System.nanoTime(); + LOGGER.atInfo().addKeyValue(TRACKING_ID_KEY, id).log("Unable to acquire new session.", failure); + // The link-endpoint-state publisher will emit signal on the reactor-executor thread, which is + // non-blocking, if we use the session processor to recover the error, it requires a blocking + // thread to close the client. Hence, we publish the error on the bounded-elastic thread. + return Mono.error(failure) + .publishOn(Schedulers.boundedElastic()) + .doOnError(e -> LOGGER.atInfo() + .addKeyValue(TRACKING_ID_KEY, id) + .log("Emitting the error signal received for session acquire attempt.", e)); + } + }))); + }); } /** diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java index 8469c428b985e..01248b5a54017 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java @@ -84,6 +84,7 @@ import java.util.List; import java.util.Map; import java.util.UUID; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; @@ -1656,6 +1657,40 @@ private ServiceBusReceivedMessage mockReceivedMessage(Instant enqueuedTime) { return receivedMessage; } + @Test + void receiveRecoveryDoesNotAcquireOrRemoveTheReplacementSession() { + final ServiceBusAmqpConnection replacement = mock(ServiceBusAmqpConnection.class); + final AtomicReference current = new AtomicReference<>(connection); + final ConnectionCacheWrapper wrapper = mock(ConnectionCacheWrapper.class); + when(wrapper.getConnection()).thenReturn(Mono.fromSupplier(current::get)); + when(wrapper.getRetryOptions()).thenReturn(new AmqpRetryOptions().setDelay(Duration.ofMillis(1))); + when(wrapper.getFullyQualifiedNamespace()).thenReturn(NAMESPACE); + final AmqpException failed + = new AmqpException(true, AmqpErrorCondition.CONNECTION_FORCED, "connection forced", null); + final AmqpException terminal = new AmqpException(false, AmqpErrorCondition.NOT_ALLOWED, "terminal", null); + when(connection.createReceiveLink(anyString(), anyString(), any(ServiceBusReceiveMode.class), any(), + any(MessagingEntityType.class), anyString())).thenAnswer(invocation -> { + current.set(replacement); + return Mono.error(failed); + }); + when(replacement.createReceiveLink(anyString(), anyString(), any(ServiceBusReceiveMode.class), any(), + any(MessagingEntityType.class), anyString())).thenReturn(Mono.error(terminal)); + final ServiceBusReceiverAsyncClient client = new ServiceBusReceiverAsyncClient(NAMESPACE, ENTITY_PATH, + ENTITY_TYPE, createNonSessionOptions(ServiceBusReceiveMode.PEEK_LOCK, PREFETCH, null, false), wrapper, + CLEANUP_INTERVAL, instrumentation, messageSerializer, onClientClose, CLIENT_IDENTIFIER); + try { + StepVerifier.create(client.receiveMessages()) + .expectError(ServiceBusException.class) + .verify(DEFAULT_TIMEOUT); + verify(connection).removeSession(ENTITY_PATH); + verify(replacement, never()).removeSession(anyString()); + verify(wrapper).invalidateConnection(connection); + verify(wrapper, never()).invalidateConnection(replacement); + } finally { + client.close(); + } + } + private List getMessages() { final Map map = Collections.singletonMap("SAMPLE_HEADER", "foo"); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java index 9f70ed644f5e5..953ba7f23312a 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java @@ -9,6 +9,7 @@ import com.azure.core.amqp.exception.AmqpErrorCondition; import com.azure.core.amqp.exception.AmqpErrorContext; import com.azure.core.amqp.exception.AmqpException; +import com.azure.core.amqp.exception.OperationCancelledException; import com.azure.core.amqp.implementation.AmqpLinkProvider; import com.azure.core.amqp.implementation.AmqpSendLink; import com.azure.core.amqp.implementation.ConnectionOptions; @@ -20,6 +21,7 @@ import com.azure.core.amqp.implementation.ReactorProvider; import com.azure.core.amqp.implementation.TokenManager; import com.azure.core.amqp.implementation.TokenManagerProvider; +import com.azure.core.amqp.implementation.RetryUtil; import com.azure.core.amqp.implementation.handler.ConnectionHandler; import com.azure.core.amqp.implementation.handler.SendLinkHandler; import com.azure.core.amqp.implementation.handler.SessionHandler; @@ -40,6 +42,8 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.junit.jupiter.api.parallel.Execution; import org.junit.jupiter.api.parallel.ExecutionMode; import org.junit.jupiter.api.parallel.Isolated; @@ -67,6 +71,7 @@ import java.util.List; import java.util.Objects; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -654,10 +659,126 @@ private MockEndpoint createMockEndpoint(int sessionsCnt, int[] linksPerSession) return MockEndpoint.create(connectionId, QUEUE_NAME, RETRY_OPTIONS, sessionsCnt, linksPerSession); } + @Test + void shouldRecoverAfterNegotiationRetriesAreExhausted() { + try (MockEndpoint endpoint = createMockEndpoint(1, new int[] { 2 })) { + final AmqpSendLink failed = endpoint.getAmqpSendLink(0, 0); + final AmqpRetryOptions negotiationOptions = new AmqpRetryOptions(RETRY_OPTIONS).setMaxRetries(0); + when(failed.closeAsync()).thenAnswer(invocation -> { + endpoint.emitCurrentSendLinkError(RETRIABLE_LINK_ERROR); + return Mono.empty(); + }); + when(endpoint.getAmqpSendLink(0, 1).send(any(Message.class))).thenReturn(Mono.empty()); + final ReactorConnectionCache cache = createConnectionCache(() -> { + final ServiceBusReactorAmqpConnection result = endpoint.arrange(); + when(failed.getLinkSize()).thenReturn( + RetryUtil.withRetry(Mono.error(RETRIABLE_LINK_ERROR), negotiationOptions, "negotiation")); + return result; + }); + final ServiceBusSenderAsyncClient client = createSenderAsyncClient(cache, false); + try (VirtualTimeStepVerifier verifier = new VirtualTimeStepVerifier()) { + verifier.create(() -> client.sendMessage(createMessageToSend())) + .thenAwait(VIRTUAL_TIME_SHIFT) + .expectComplete() + .verify(Duration.ofSeconds(5)); + } finally { + client.close(); + cache.dispose(); + } + } + } + + @Test + void shouldInvalidateConnectionBeforeNonCompletingLinkClose() { + final List counts = new ArrayList<>(); + counts.add(new SessionLinkCount(1, new int[] { 1 })); + counts.add(new SessionLinkCount(1, new int[] { 1 })); + try (MockEndpoints endpoints = createMockEndpoints(counts)) { + final AtomicInteger supplied = new AtomicInteger(); + final ReactorConnectionCache cache + = createConnectionCache(() -> endpoints.get(supplied.getAndIncrement()).arrange()); + final AmqpSendLink failed = endpoints.get(0).getAmqpSendLink(0, 0); + when(failed.send(any(Message.class))).thenReturn(Mono.error(RETRIABLE_CONNECTION_ERROR)); + when(failed.closeAsync()).thenReturn(Mono.never()); + when(endpoints.get(1).getAmqpSendLink(0, 0).send(any(Message.class))).thenReturn(Mono.empty()); + final ServiceBusSenderAsyncClient client = createSenderAsyncClient(cache, false); + try (VirtualTimeStepVerifier verifier = new VirtualTimeStepVerifier()) { + verifier.create(() -> client.sendMessage(createMessageToSend())) + .thenAwait(VIRTUAL_TIME_SHIFT) + .expectComplete() + .verify(Duration.ofSeconds(5)); + Assertions.assertEquals(2, supplied.get()); + } finally { + client.close(); + cache.dispose(); + } + } + } + + @Test + void delayedConnectionErrorMustNotInvalidateReplacement() { + final List counts = new ArrayList<>(); + counts.add(new SessionLinkCount(1, new int[] { 1 })); + counts.add(new SessionLinkCount(1, new int[] { 1 })); + try (MockEndpoints endpoints = createMockEndpoints(counts)) { + final AtomicInteger supplied = new AtomicInteger(); + final ReactorConnectionCache cache + = createConnectionCache(() -> endpoints.get(supplied.getAndIncrement()).arrange()); + final AmqpSendLink failed = endpoints.get(0).getAmqpSendLink(0, 0); + when(failed.send(any(Message.class))).thenAnswer(invocation -> { + endpoints.get(0).emitConnectionError(RETRIABLE_CONNECTION_ERROR); + return cache.get().then(Mono.error(RETRIABLE_CONNECTION_ERROR)); + }); + when(failed.closeAsync()).thenReturn(Mono.empty()); + when(endpoints.get(1).getAmqpSendLink(0, 0).send(any(Message.class))).thenReturn(Mono.empty()); + final ServiceBusSenderAsyncClient client = createSenderAsyncClient(cache, false); + try (VirtualTimeStepVerifier verifier = new VirtualTimeStepVerifier()) { + verifier.create(() -> client.sendMessage(createMessageToSend())) + .thenAwait(VIRTUAL_TIME_SHIFT) + .expectComplete() + .verify(Duration.ofSeconds(5)); + Assertions.assertEquals(2, supplied.get()); + } finally { + client.close(); + cache.dispose(); + } + } + } + private MockEndpoints createMockEndpoints(List sessionLinkCountList) { return MockEndpoints.create(QUEUE_NAME, RETRY_OPTIONS, sessionLinkCountList); } + @ParameterizedTest + @ValueSource(booleans = { false, true }) + void abortedSendRecoversItsLink(boolean withCause) { + try (MockEndpoint endpoint = createMockEndpoint(1, new int[] { 2 })) { + final OperationCancelledException aborted = withCause + ? new OperationCancelledException("Delivery advance failed", new IllegalStateException("advance"), null) + : new OperationCancelledException("Released", null); + final AmqpSendLink failed = endpoint.getAmqpSendLink(0, 0); + when(failed.send(any(Message.class))).thenReturn(Mono.error(aborted)); + when(failed.closeAsync()).thenAnswer(invocation -> { + endpoint.emitCurrentSendLinkCompletion(); + return Mono.empty(); + }); + when(endpoint.getAmqpSendLink(0, 1).send(any(Message.class))).thenReturn(Mono.empty()); + final ReactorConnectionCache cache + = createConnectionCache(singleConnectionSupplier(endpoint)); + final ServiceBusSenderAsyncClient client = createSenderAsyncClient(cache, false); + try (VirtualTimeStepVerifier verifier = new VirtualTimeStepVerifier()) { + verifier.create(() -> client.sendMessage(createMessageToSend())) + .thenAwait(VIRTUAL_TIME_SHIFT) + .expectComplete() + .verify(Duration.ofSeconds(5)); + verify(failed).closeAsync(); + } finally { + client.close(); + cache.dispose(); + } + } + } + private Supplier singleConnectionSupplier(MockEndpoint endpoint) { final Supplier connectionSupplier = new Supplier() { diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirerIsolatedTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirerIsolatedTest.java index 321acb70c2662..701d0bdb8e60d 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirerIsolatedTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirerIsolatedTest.java @@ -32,6 +32,7 @@ import java.util.ArrayDeque; import java.util.Deque; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; import static org.mockito.ArgumentMatchers.any; @@ -211,12 +212,60 @@ private ConnectionCacheWrapper createMockConnectionWrapper(OnCreateSessionLink o return cacheWrapper; } + @Test + public void shouldPropagateLockedNamedSessionWithoutRetry() { + final AmqpException locked = new AmqpException(false, AmqpErrorCondition.SESSION_CANNOT_BE_LOCKED, + "The requested session is already locked.", new AmqpErrorContext(ENTITY_PATH)); + final Deque> sessionLinks = new ArrayDeque<>(); + sessionLinks.add(Mono.error(locked)); + sessionLinks.add(Mono.error(new IllegalStateException("Unexpected second acquisition"))); + final OnCreateSessionLink onCreateSessionLink = new OnCreateSessionLink(sessionLinks); + final ServiceBusSessionAcquirer acquirer + = createSessionAcquirer(createMockConnectionWrapper(onCreateSessionLink), false); + + try (VirtualTimeStepVerifier verifier = new VirtualTimeStepVerifier()) { + verifier.create(() -> acquirer.acquire("locked-session")) + .thenAwait(RETRY_BACKOFF.plusSeconds(1)) + .verifyErrorSatisfies(error -> Assertions.assertSame(locked, error)); + } + Assertions.assertEquals(1, onCreateSessionLink.pending()); + } + private ServiceBusSessionAcquirer createSessionAcquirer(ConnectionCacheWrapper cacheWrapper, boolean isTimeoutRetryDisabled) { return new ServiceBusSessionAcquirer(LOGGER, IDENTIFIER, ENTITY_PATH, ENTITY_TYPE, RECEIVE_MODE, TRY_TIMEOUT, isTimeoutRetryDisabled, cacheWrapper); } + @Test + public void connectionRecoveryTargetsTheFailedAcquisition() { + final ServiceBusAmqpConnection failed = mock(ServiceBusAmqpConnection.class); + final ServiceBusAmqpConnection replacement = mock(ServiceBusAmqpConnection.class); + final AtomicReference current = new AtomicReference<>(failed); + final ConnectionCacheWrapper wrapper = mock(ConnectionCacheWrapper.class); + when(wrapper.isV2()).thenReturn(true); + when(wrapper.getConnection()).thenReturn(Mono.fromSupplier(current::get)); + when(wrapper.getRetryOptions()).thenReturn(new AmqpRetryOptions().setDelay(RETRY_BACKOFF)); + final AmqpException connectionError + = new AmqpException(true, AmqpErrorCondition.CONNECTION_FORCED, "connection forced", null); + final IllegalStateException terminal = new IllegalStateException("stop after replacement"); + when(failed.createReceiveLink(anyString(), anyString(), any(ServiceBusReceiveMode.class), any(), + any(MessagingEntityType.class), anyString(), any())).thenAnswer(invocation -> { + current.set(replacement); + return Mono.error(connectionError); + }); + when(replacement.createReceiveLink(anyString(), anyString(), any(ServiceBusReceiveMode.class), any(), + any(MessagingEntityType.class), anyString(), any())).thenReturn(Mono.error(terminal)); + + try (VirtualTimeStepVerifier verifier = new VirtualTimeStepVerifier()) { + verifier.create(() -> createSessionAcquirer(wrapper, false).acquire("session")) + .thenAwait(AWAIT_DURATION) + .verifyErrorSatisfies(error -> Assertions.assertSame(terminal, error)); + } + Mockito.verify(wrapper).invalidateConnection(failed); + Mockito.verify(wrapper, Mockito.never()).invalidateConnection(replacement); + } + private static final class OnCreateSessionLink implements Answer> { final Deque> sessionLinks = new ArrayDeque<>(); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionManagerTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionManagerTest.java index 8347fe3520f7a..8e92399a4e05e 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionManagerTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionManagerTest.java @@ -7,6 +7,8 @@ import com.azure.core.amqp.AmqpRetryOptions; import com.azure.core.amqp.AmqpTransportType; import com.azure.core.amqp.ProxyOptions; +import com.azure.core.amqp.exception.AmqpErrorCondition; +import com.azure.core.amqp.exception.AmqpException; import com.azure.core.amqp.implementation.ConnectionOptions; import com.azure.core.amqp.implementation.MessageSerializer; import com.azure.core.amqp.models.CbsAuthorizationType; @@ -51,12 +53,14 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import static com.azure.messaging.servicebus.ReceiverOptions.createUnnamedSessionOptions; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; @@ -172,6 +176,51 @@ void properties() { assertEquals(CLIENT_IDENTIFIER, sessionManager.getIdentifier()); } + @Test + void lockedNamedSessionIsTerminal() { + final String sessionId = "locked-session"; + final AmqpException locked = new AmqpException(false, AmqpErrorCondition.SESSION_CANNOT_BE_LOCKED, + "The requested session is already locked.", null); + final ReceiverOptions options = ReceiverOptions.createNamedSessionOptions(ServiceBusReceiveMode.PEEK_LOCK, 1, + MAX_LOCK_RENEWAL, false, sessionId); + sessionManager = new ServiceBusSessionManager(ENTITY_PATH, ENTITY_TYPE, connectionCacheWrapper, + messageSerializer, options, CLIENT_IDENTIFIER, NOOP_TRACER); + when(connection.createReceiveLink(eq(sessionId), eq(ENTITY_PATH), any(ServiceBusReceiveMode.class), isNull(), + eq(ENTITY_TYPE), eq(CLIENT_IDENTIFIER), eq(sessionId))).thenReturn(Mono.error(locked)); + + StepVerifier.create(sessionManager.getActiveLink()) + .expectErrorSatisfies(error -> assertSame(locked, error)) + .verify(Duration.ofSeconds(2)); + verify(connection, times(1)).createReceiveLink(eq(sessionId), eq(ENTITY_PATH), any(ServiceBusReceiveMode.class), + isNull(), eq(ENTITY_TYPE), eq(CLIENT_IDENTIFIER), eq(sessionId)); + } + + @Test + void unnamedSessionRetriesWithFreshLinkNameAfterTimeout() { + connectionCacheWrapper.getRetryOptions().setTryTimeout(Duration.ofMillis(100)); + final ReceiverOptions options = createUnnamedSessionOptions(ServiceBusReceiveMode.PEEK_LOCK, 1, + MAX_LOCK_RENEWAL, false, 5, SESSION_IDLE_TIMEOUT); + sessionManager = new ServiceBusSessionManager(ENTITY_PATH, ENTITY_TYPE, connectionCacheWrapper, + messageSerializer, options, CLIENT_IDENTIFIER, NOOP_TRACER); + final ServiceBusReceiveLink hungLink = mock(ServiceBusReceiveLink.class); + when(hungLink.getEndpointStates()).thenReturn(Flux.never()); + final AtomicReference firstLinkName = new AtomicReference<>(); + when(connection.createReceiveLink(anyString(), eq(ENTITY_PATH), any(ServiceBusReceiveMode.class), isNull(), + eq(ENTITY_TYPE), eq(CLIENT_IDENTIFIER), isNull())).thenAnswer(invocation -> { + final String linkName = invocation.getArgument(0); + firstLinkName.compareAndSet(null, linkName); + return Mono.just(firstLinkName.get().equals(linkName) ? hungLink : amqpReceiveLink); + }); + + StepVerifier.create(sessionManager.getActiveLink()) + .assertNext(link -> assertSame(amqpReceiveLink, link)) + .expectComplete() + .verify(Duration.ofSeconds(2)); + verify(connection, times(2)).createReceiveLink(linkNameCaptor.capture(), eq(ENTITY_PATH), + any(ServiceBusReceiveMode.class), isNull(), eq(ENTITY_TYPE), eq(CLIENT_IDENTIFIER), isNull()); + assertEquals(2, linkNameCaptor.getAllValues().stream().distinct().count()); + } + @Test void receiveNull() { // Arrange From 4fbc7dc980667f1ddb795b20b76eb88a8170a803 Mon Sep 17 00:00:00 2001 From: Eldert Grootenboer Date: Thu, 29 Oct 2026 13:40:45 -0700 Subject: [PATCH 36/39] fix: complete Service Bus tiered recovery retry cleanup - Retry resource-limit failures without closing healthy resources - Normalize cross-entity session eviction and honor connection backoff - Close abandoned session links asynchronously on timeout or cancellation - Cover failure paths and preserve terminal error handling Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../messaging/servicebus/RecoveryUtils.java | 9 ++- .../ServiceBusReceiverAsyncClient.java | 2 +- .../servicebus/ServiceBusSessionAcquirer.java | 2 +- .../servicebus/ServiceBusSessionManager.java | 4 ++ .../ServiceBusReactorAmqpConnection.java | 5 ++ .../ServiceBusReceiverAsyncClientTest.java | 29 ++++++++ ...SenderAsyncClientRecoveryIsolatedTest.java | 70 ++++++++++++++++++- ...ServiceBusSessionAcquirerIsolatedTest.java | 26 +++++++ .../ServiceBusSessionManagerTest.java | 30 ++++++++ 9 files changed, 173 insertions(+), 4 deletions(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java index d7c965d0a94c0..f3a108e89fdbf 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/RecoveryUtils.java @@ -3,6 +3,7 @@ package com.azure.messaging.servicebus; +import com.azure.core.amqp.exception.AmqpErrorCondition; import com.azure.core.amqp.exception.AmqpException; import com.azure.core.amqp.implementation.RecoveryKind; import reactor.core.Exceptions; @@ -36,13 +37,19 @@ static Throwable unwrapRetryExhaustion(Throwable error) { * * @param error the original error emitted from a send/receive operation. * @return the same error if it already passes the retry filter, otherwise a transient - * {@link AmqpException} wrapping it (only for LINK/CONNECTION-classified errors). + * {@link AmqpException} wrapping it for LINK/CONNECTION errors or a same-link resource-limit retry. */ static Throwable asRetriable(Throwable error) { if (error instanceof TimeoutException || (error instanceof AmqpException && ((AmqpException) error).isTransient())) { return error; } + if (error instanceof AmqpException) { + final AmqpException amqpError = (AmqpException) error; + if (amqpError.getErrorCondition() == AmqpErrorCondition.RESOURCE_LIMIT_EXCEEDED) { + return new AmqpException(true, amqpError.getErrorCondition(), amqpError, amqpError.getContext()); + } + } final RecoveryKind kind = RecoveryKind.classify(error); if (kind == RecoveryKind.LINK || kind == RecoveryKind.CONNECTION) { return new AmqpException(true, error.getMessage(), error, null); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java index 83e3096c78723..577f907aa4720 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java @@ -1782,7 +1782,7 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { final Throwable retriable = RecoveryUtils.asRetriable(e); return recovery.then(Mono.error(retriable)); } - return Mono.error(e); + return Mono.error(RecoveryUtils.asRetriable(e)); }), connectionCacheWrapper.getRetryOptions(), "Failed to create receive link " + linkName, true); // A Flux that produces a new AmqpReceiveLink each time it receives a request from the below diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java index 129682f7a26af..b6b2c4f7883d6 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirer.java @@ -187,7 +187,7 @@ private Mono acquireIntern(String sessionId) { if (failedConnection != null) { connectionCacheWrapper.invalidateConnection(failedConnection); } - return Mono.delay(Duration.ZERO); + return Mono.delay(retryBackoff); } if (isTimeoutError(t)) { logger.atVerbose() diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java index 05128a2f2e041..b3a9bff53ea65 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusSessionManager.java @@ -299,6 +299,10 @@ Mono getActiveLink() { // .switchIfEmpty(Mono .error(() -> new AmqpException(true, "Session receive link completed without being active", null))) + .doOnCancel(() -> link.closeAsync().subscribe(unused -> { + }, error -> LOGGER.atWarning() + .addKeyValue(ENTITY_PATH_KEY, entityPath) + .log("Error closing abandoned session receive link.", error))) .timeout(operationTimeout) .then(Mono.just(link))).retryWhen(Retry.from(retrySignals -> retrySignals.flatMap(signal -> { final Throwable failure = signal.failure(); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnection.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnection.java index ecd6583f07a1c..009d0b53675f4 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnection.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnection.java @@ -215,6 +215,11 @@ public Mono createSession(String sessionName) { return super.createSession(distributedTransactionsSupport ? CROSS_ENTITY_TRANSACTIONS_LINK_NAME : sessionName); } + @Override + public boolean removeSession(String sessionName) { + return super.removeSession(distributedTransactionsSupport ? CROSS_ENTITY_TRANSACTIONS_LINK_NAME : sessionName); + } + /** * Creates or gets an existing receive link. The same link is returned if there is an existing receive link with the * same {@code linkName}. Otherwise, a new link is created and returned. diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java index 01248b5a54017..97d98407aa908 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java @@ -11,6 +11,7 @@ import com.azure.core.amqp.exception.AmqpErrorCondition; import com.azure.core.amqp.exception.AmqpException; import com.azure.core.amqp.implementation.ConnectionOptions; +import com.azure.core.amqp.implementation.ExceptionUtil; import com.azure.core.amqp.implementation.MessageSerializer; import com.azure.core.amqp.implementation.ReactorConnectionCache; import com.azure.core.amqp.models.CbsAuthorizationType; @@ -1657,6 +1658,34 @@ private ServiceBusReceivedMessage mockReceivedMessage(Instant enqueuedTime) { return receivedMessage; } + @Test + void receiveResourceLimitRetriesWithoutRemovingTheSession() { + final Exception resourceLimit = ExceptionUtil + .toException(AmqpErrorCondition.RESOURCE_LIMIT_EXCEEDED.getErrorCondition(), "resource limit", null); + final AmqpException terminal = new AmqpException(false, AmqpErrorCondition.NOT_ALLOWED, "terminal", null); + final ConnectionCacheWrapper wrapper = mock(ConnectionCacheWrapper.class); + when(wrapper.getConnection()).thenReturn(Mono.just(connection)); + when(wrapper.getRetryOptions()).thenReturn(new AmqpRetryOptions().setDelay(Duration.ofMillis(1))); + when(wrapper.getFullyQualifiedNamespace()).thenReturn(NAMESPACE); + when(connection.createReceiveLink(anyString(), anyString(), any(ServiceBusReceiveMode.class), any(), + any(MessagingEntityType.class), anyString())).thenReturn(Mono.error(resourceLimit)) + .thenReturn(Mono.error(terminal)); + final ServiceBusReceiverAsyncClient client = new ServiceBusReceiverAsyncClient(NAMESPACE, ENTITY_PATH, + ENTITY_TYPE, createNonSessionOptions(ServiceBusReceiveMode.PEEK_LOCK, PREFETCH, null, false), wrapper, + CLEANUP_INTERVAL, instrumentation, messageSerializer, onClientClose, CLIENT_IDENTIFIER); + try { + StepVerifier.create(client.receiveMessages()) + .expectErrorSatisfies(error -> assertSame(terminal, error.getCause())) + .verify(DEFAULT_TIMEOUT); + verify(connection, times(2)).createReceiveLink(anyString(), anyString(), any(ServiceBusReceiveMode.class), + any(), any(MessagingEntityType.class), anyString()); + verify(connection, never()).removeSession(anyString()); + verify(wrapper, never()).invalidateConnection(any(ServiceBusAmqpConnection.class)); + } finally { + client.close(); + } + } + @Test void receiveRecoveryDoesNotAcquireOrRemoveTheReplacementSession() { final ServiceBusAmqpConnection replacement = mock(ServiceBusAmqpConnection.class); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java index 953ba7f23312a..226adf51c85ff 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java @@ -6,6 +6,7 @@ import com.azure.core.amqp.AmqpEndpointState; import com.azure.core.amqp.AmqpRetryMode; import com.azure.core.amqp.AmqpRetryOptions; +import com.azure.core.amqp.AmqpSession; import com.azure.core.amqp.exception.AmqpErrorCondition; import com.azure.core.amqp.exception.AmqpErrorContext; import com.azure.core.amqp.exception.AmqpException; @@ -13,6 +14,7 @@ import com.azure.core.amqp.implementation.AmqpLinkProvider; import com.azure.core.amqp.implementation.AmqpSendLink; import com.azure.core.amqp.implementation.ConnectionOptions; +import com.azure.core.amqp.implementation.ExceptionUtil; import com.azure.core.amqp.implementation.MessageSerializer; import com.azure.core.amqp.implementation.ReactorConnectionCache; import com.azure.core.amqp.implementation.ReactorDispatcher; @@ -43,6 +45,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.ValueSource; import org.junit.jupiter.api.parallel.Execution; import org.junit.jupiter.api.parallel.ExecutionMode; @@ -659,6 +662,66 @@ private MockEndpoint createMockEndpoint(int sessionsCnt, int[] linksPerSession) return MockEndpoint.create(connectionId, QUEUE_NAME, RETRY_OPTIONS, sessionsCnt, linksPerSession); } + @Test + void resourceLimitFromExceptionUtilRetriesWithoutClosingResources() { + final Exception error + = ExceptionUtil.toException(AmqpErrorCondition.RESOURCE_LIMIT_EXCEEDED.getErrorCondition(), + "resource limit", new AmqpErrorContext(FQDN)); + Assertions.assertFalse(((AmqpException) error).isTransient()); + final AmqpException retriable = (AmqpException) RecoveryUtils.asRetriable(error); + Assertions.assertTrue(retriable.isTransient()); + Assertions.assertEquals(AmqpErrorCondition.RESOURCE_LIMIT_EXCEEDED, retriable.getErrorCondition()); + Assertions.assertSame(((AmqpException) error).getContext(), retriable.getContext()); + Assertions.assertSame(error, retriable.getCause()); + try (MockEndpoint endpoint = createMockEndpoint(1, new int[] { 1 })) { + final AmqpSendLink link = endpoint.getAmqpSendLink(0, 0); + when(link.send(any(Message.class))).thenReturn(Mono.error(error)).thenReturn(Mono.empty()); + final ReactorConnectionCache cache + = createConnectionCache(singleConnectionSupplier(endpoint)); + final ServiceBusSenderAsyncClient client = createSenderAsyncClient(cache, false); + try (VirtualTimeStepVerifier verifier = new VirtualTimeStepVerifier()) { + verifier.create(() -> client.sendMessage(createMessageToSend())) + .thenAwait(VIRTUAL_TIME_SHIFT) + .expectComplete() + .verify(Duration.ofSeconds(5)); + verify(link, times(2)).send(any(Message.class)); + verify(link, Mockito.never()).closeAsync(); + } finally { + client.close(); + cache.dispose(); + } + } + } + + @ParameterizedTest + @EnumSource( + value = AmqpErrorCondition.class, + names = { "NOT_FOUND", "UNAUTHORIZED_ACCESS", "MESSAGE_LOCK_LOST", "SESSION_CANNOT_BE_LOCKED" }) + void nonRetriableErrorsStayNonRetriable(AmqpErrorCondition condition) { + final AmqpException error = new AmqpException(false, condition, "terminal", null); + Assertions.assertSame(error, RecoveryUtils.asRetriable(error)); + } + + @ParameterizedTest + @ValueSource(booleans = { false, true }) + void sessionRemovalUsesTheSameKeyAsCreation(boolean crossEntityTransactions) { + try (MockEndpoint endpoint = createMockEndpoint(2, new int[] { 0, 0 })) { + final ReactorConnectionCache cache + = createConnectionCache(() -> endpoint.arrange(crossEntityTransactions)); + try { + final ServiceBusReactorAmqpConnection connection = cache.get().block(Duration.ofSeconds(5)); + final AmqpSession first = connection.createSession(QUEUE_NAME).block(Duration.ofSeconds(5)); + Assertions.assertNotNull(first); + Assertions.assertTrue(connection.removeSession(QUEUE_NAME)); + final AmqpSession replacement = connection.createSession(QUEUE_NAME).block(Duration.ofSeconds(5)); + Assertions.assertNotNull(replacement); + Assertions.assertNotSame(first, replacement); + } finally { + cache.dispose(); + } + } + } + @Test void shouldRecoverAfterNegotiationRetriesAreExhausted() { try (MockEndpoint endpoint = createMockEndpoint(1, new int[] { 2 })) { @@ -970,6 +1033,10 @@ static MockEndpoint create(String connectionId, String queueName, AmqpRetryOptio } ServiceBusReactorAmqpConnection arrange() { + return arrange(false); + } + + ServiceBusReactorAmqpConnection arrange(boolean crossEntityTransactions) { if (arranged.getAndSet(true)) { throw new RuntimeException("Only one connection can be obtained from a MockEndpoint instance."); } @@ -1010,7 +1077,8 @@ ServiceBusReactorAmqpConnection arrange() { // New tests only for ReactorConnectionCache introduced in v2. final boolean isV2 = true; return new ServiceBusReactorAmqpConnection(connectionId, connectionOptions, reactorProvider, - handlerProvider, linkProvider, tokenManagerProvider, messageSerializer, false, isV2, false); + handlerProvider, linkProvider, tokenManagerProvider, messageSerializer, crossEntityTransactions, isV2, + false); } AmqpSendLink getAmqpSendLink(int sessionIdx, int linkIdx) { diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirerIsolatedTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirerIsolatedTest.java index 701d0bdb8e60d..a95f98010c76b 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirerIsolatedTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionAcquirerIsolatedTest.java @@ -160,6 +160,32 @@ public void shouldBackOffBetweenRetriesIfRetryEnabled() { Assertions.assertEquals(0, onCreateSessionLink.pending()); } + @Test + @Execution(ExecutionMode.SAME_THREAD) + public void shouldBackOffBetweenConnectionRecoveryAttempts() { + final AmqpException connectionError + = new AmqpException(true, AmqpErrorCondition.CONNECTION_FORCED, "connection forced", null); + final RuntimeException terminal = new RuntimeException("stop after two failures"); + final Deque> links = new ArrayDeque<>(); + links.add(Mono.error(connectionError)); + links.add(Mono.error(connectionError)); + links.add(Mono.error(terminal)); + final OnCreateSessionLink onCreate = new OnCreateSessionLink(links); + final ConnectionCacheWrapper wrapper = createMockConnectionWrapper(onCreate); + final ServiceBusSessionAcquirer acquirer = createSessionAcquirer(wrapper, false); + + try (VirtualTimeStepVerifier verifier = new VirtualTimeStepVerifier()) { + verifier.create(acquirer::acquire) + .thenAwait(RETRY_BACKOFF.dividedBy(2)) + .then(() -> Assertions.assertEquals(2, onCreate.pending())) + .thenAwait(RETRY_BACKOFF) + .then(() -> Assertions.assertEquals(1, onCreate.pending())) + .thenAwait(RETRY_BACKOFF) + .verifyErrorSatisfies(error -> Assertions.assertSame(terminal, error)); + } + Mockito.verify(wrapper, Mockito.times(2)).invalidateConnection(any(ServiceBusAmqpConnection.class)); + } + @Test @Execution(ExecutionMode.SAME_THREAD) public void shouldRetryOnBrokerTimeoutErrorIfRetryEnabled() { diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionManagerTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionManagerTest.java index 8e92399a4e05e..a7c7d1d24b90c 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionManagerTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSessionManagerTest.java @@ -204,6 +204,10 @@ void unnamedSessionRetriesWithFreshLinkNameAfterTimeout() { messageSerializer, options, CLIENT_IDENTIFIER, NOOP_TRACER); final ServiceBusReceiveLink hungLink = mock(ServiceBusReceiveLink.class); when(hungLink.getEndpointStates()).thenReturn(Flux.never()); + final AtomicBoolean abandonedLinkClosed = new AtomicBoolean(); + final Sinks.Empty closeCompletion = Sinks.empty(); + when(hungLink.closeAsync()) + .thenReturn(closeCompletion.asMono().doOnSubscribe(ignored -> abandonedLinkClosed.set(true))); final AtomicReference firstLinkName = new AtomicReference<>(); when(connection.createReceiveLink(anyString(), eq(ENTITY_PATH), any(ServiceBusReceiveMode.class), isNull(), eq(ENTITY_TYPE), eq(CLIENT_IDENTIFIER), isNull())).thenAnswer(invocation -> { @@ -219,6 +223,32 @@ void unnamedSessionRetriesWithFreshLinkNameAfterTimeout() { verify(connection, times(2)).createReceiveLink(linkNameCaptor.capture(), eq(ENTITY_PATH), any(ServiceBusReceiveMode.class), isNull(), eq(ENTITY_TYPE), eq(CLIENT_IDENTIFIER), isNull()); assertEquals(2, linkNameCaptor.getAllValues().stream().distinct().count()); + assertTrue(abandonedLinkClosed.get(), "the abandoned link must be closed independently of the retry"); + verify(amqpReceiveLink, Mockito.never()).closeAsync(); + closeCompletion.tryEmitEmpty(); + } + + @Test + void cancellingUnnamedSessionAcquisitionClosesOnlyThePendingLink() { + final ReceiverOptions options = createUnnamedSessionOptions(ServiceBusReceiveMode.PEEK_LOCK, 1, + MAX_LOCK_RENEWAL, false, 5, SESSION_IDLE_TIMEOUT); + sessionManager = new ServiceBusSessionManager(ENTITY_PATH, ENTITY_TYPE, connectionCacheWrapper, + messageSerializer, options, CLIENT_IDENTIFIER, NOOP_TRACER); + final ServiceBusReceiveLink pending = mock(ServiceBusReceiveLink.class); + when(pending.getEndpointStates()).thenReturn(Flux.never()); + final AtomicBoolean closed = new AtomicBoolean(); + final Sinks.Empty closeCompletion = Sinks.empty(); + when(pending.closeAsync()).thenReturn(closeCompletion.asMono().doOnSubscribe(ignored -> closed.set(true))); + when(connection.createReceiveLink(anyString(), eq(ENTITY_PATH), any(ServiceBusReceiveMode.class), isNull(), + eq(ENTITY_TYPE), eq(CLIENT_IDENTIFIER), isNull())).thenReturn(Mono.just(pending)); + + StepVerifier.create(sessionManager.getActiveLink()) + .thenAwait(Duration.ofMillis(10)) + .thenCancel() + .verify(Duration.ofSeconds(2)); + assertTrue(closed.get(), "cancellation must close the link before it can acquire an unconsumed session"); + verify(pending).closeAsync(); + closeCompletion.tryEmitEmpty(); } @Test From c22a043bfa3856091ccce667159c7b3014db459f Mon Sep 17 00:00:00 2001 From: Eldert Grootenboer Date: Thu, 29 Oct 2026 14:58:12 -0700 Subject: [PATCH 37/39] fix: preserve replacement sessions during receive recovery - Evict only the session owned by a failed receive-link attempt - Guard session cache cleanup against delayed callbacks and open failures - Cover both receive factories, cache backends and cross-entity keys Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../implementation/ReactorConnection.java | 29 ++++- .../implementation/ReactorSessionCache.java | 15 ++- .../implementation/ReactorConnectionTest.java | 30 +++++ .../ReactorSessionCacheTest.java | 44 +++++++ .../ServiceBusReceiverAsyncClient.java | 13 +- .../ServiceBusReactorAmqpConnection.java | 28 ++++- .../ServiceBusReceiverAsyncClientTest.java | 2 +- ...SenderAsyncClientRecoveryIsolatedTest.java | 44 +++++++ .../ServiceBusReactorAmqpConnectionTest.java | 111 ++++++++++++++++++ 9 files changed, 296 insertions(+), 20 deletions(-) create mode 100644 sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnectionTest.java diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnection.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnection.java index 58ca458207d40..07949665dff08 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnection.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorConnection.java @@ -362,7 +362,7 @@ public Mono createSession(String sessionName) { logger.atInfo() .addKeyValue(SESSION_NAME_KEY, sessionName) .log("Error occurred. Removing and disposing session", error); - removeSession(key); + removeSession(key, amqpSession); }, () -> { // If we were already disposing of the connection, the session would be removed. if (isDisposed.get()) { @@ -372,7 +372,7 @@ public Mono createSession(String sessionName) { logger.atVerbose() .addKeyValue(SESSION_NAME_KEY, sessionName) .log("Complete. Removing and disposing session."); - removeSession(key); + removeSession(key, amqpSession); }); return new SessionSubscription(amqpSession, subscription); @@ -396,8 +396,7 @@ public Mono createSession(String sessionName) { final AmqpException amqpException = (AmqpException) error; if (amqpException.getErrorCondition() == TIMEOUT_ERROR) { - final SessionSubscription removed = sessionMap.remove(sessionName); - removed.dispose(); + removeSession(sessionName, sessionSubscription.getSession()); } }); @@ -443,6 +442,28 @@ public boolean removeSession(String sessionName) { return removed != null; } + /** + * Removes a session only if it is still the cached instance. + * + * @param sessionName The name used to cache the session. + * @param expectedSession The session owned by the failed operation. + * @return Whether the expected session was removed. + */ + protected boolean removeSession(String sessionName, AmqpSession expectedSession) { + if (sessionName == null || expectedSession == null) { + return false; + } + if (useSessionChannelCache) { + return sessionCache.evict(sessionName, expectedSession); + } + final SessionSubscription entry = sessionMap.get(sessionName); + if (entry == null || entry.getSession() != expectedSession || !sessionMap.remove(sessionName, entry)) { + return false; + } + entry.dispose(); + return true; + } + @Override public boolean isDisposed() { return isDisposed.get(); diff --git a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorSessionCache.java b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorSessionCache.java index dcfbaad9bfb59..008bf10cd9396 100644 --- a/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorSessionCache.java +++ b/sdk/core/azure-core-amqp/src/main/java/com/azure/core/amqp/implementation/ReactorSessionCache.java @@ -3,6 +3,7 @@ package com.azure.core.amqp.implementation; +import com.azure.core.amqp.AmqpSession; import com.azure.core.util.logging.ClientLogger; import org.apache.qpid.proton.engine.Connection; import reactor.core.Disposable; @@ -119,6 +120,18 @@ boolean evict(String name) { return removed != null; } + boolean evict(String name, AmqpSession expectedSession) { + if (name == null || expectedSession == null) { + return false; + } + final Entry entry = entries.get(name); + if (entry == null || entry.getSession() != expectedSession || !entries.remove(name, entry)) { + return false; + } + entry.dispose(); + return true; + } + /** * Signal that the owner ({@link ReactorConnection}) of the cache is disposed of. */ @@ -193,7 +206,7 @@ private void evict(ReactorSession session, String message, Throwable error) { } else { logger.atInfo().addKeyValue(SESSION_NAME_KEY, name).addKeyValue(SESSION_ID_KEY, id).log(message); } - evict(name); + evict(name, session); } /** diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionTest.java index 20c36c9f0d71c..610d7bf6ea4a1 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorConnectionTest.java @@ -6,6 +6,7 @@ import com.azure.core.amqp.AmqpEndpointState; import com.azure.core.amqp.AmqpRetryMode; import com.azure.core.amqp.AmqpRetryOptions; +import com.azure.core.amqp.AmqpSession; import com.azure.core.amqp.AmqpTransportType; import com.azure.core.amqp.ProxyOptions; import com.azure.core.amqp.exception.AmqpErrorCondition; @@ -409,6 +410,35 @@ void removeSessionThatDoesNotExist() { verify(record).set(Handler.class, Handler.class, sessionHandler); } + @Test + void removeSessionOnlyRemovesExpectedInstance() { + final Session secondSession = mock(Session.class); + final Event secondEvent = mock(Event.class); + when(connectionProtonJ.session()).thenReturn(session).thenReturn(secondSession); + when(session.attachments()).thenReturn(record); + when(secondSession.attachments()).thenReturn(mock(Record.class)); + when(session.getRemoteState()).thenReturn(EndpointState.ACTIVE); + when(secondSession.getRemoteState()).thenReturn(EndpointState.ACTIVE); + when(secondEvent.getSession()).thenReturn(secondSession); + when(connectionProtonJ.getRemoteState()).thenReturn(EndpointState.ACTIVE); + connectionHandler.onConnectionRemoteOpen(connectionEvent); + sessionHandler.onSessionRemoteOpen(sessionEvent); + final AmqpSession first = connection.createSession(SESSION_NAME).block(VERIFY_TIMEOUT); + assertNotNull(first); + assertFalse(connection.removeSession(SESSION_NAME, mock(AmqpSession.class))); + assertSame(first, connection.createSession(SESSION_NAME).block(VERIFY_TIMEOUT)); + assertTrue(connection.removeSession(SESSION_NAME, first)); + sessionHandler2.onSessionRemoteOpen(secondEvent); + final AmqpSession replacement = connection.createSession(SESSION_NAME).block(VERIFY_TIMEOUT); + assertNotNull(replacement); + Assertions.assertNotSame(first, replacement); + assertFalse(connection.removeSession(SESSION_NAME, first)); + assertFalse(replacement.isDisposed()); + assertSame(replacement, connection.createSession(SESSION_NAME).block(VERIFY_TIMEOUT)); + assertTrue(connection.removeSession(SESSION_NAME, replacement)); + assertFalse(connection.removeSession(SESSION_NAME, replacement)); + } + /** * Verifies initial endpoint state is uninitialized and completes when the connection is closed. */ diff --git a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorSessionCacheTest.java b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorSessionCacheTest.java index 4228109da04c9..e62c9eec91f95 100644 --- a/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorSessionCacheTest.java +++ b/sdk/core/azure-core-amqp/src/test/java/com/azure/core/amqp/implementation/ReactorSessionCacheTest.java @@ -210,6 +210,50 @@ void shouldNotEvictSessionIfOwnerDisposed() { .verifyComplete(); } + @Test + void shouldEvictOnlyExpectedSession() { + final ReactorSessionCache cache = createSessionCache(); + final Connection connection = mock(Connection.class); + final String name = "session-0"; + final ReactorSession first = session(name, endpointStatesSink(), null); + final ReactorSession replacement = session(name, endpointStatesSink(), null); + final HashMap> lookup = new HashMap<>(); + lookup.put(name, sessions(first, replacement)); + final CacheLoader loader = new CacheLoader(lookup); + Assertions.assertSame(first, cache.getOrLoad(Mono.just(connection), name, loader).block()); + Assertions.assertFalse(cache.evict(name, replacement)); + Assertions.assertTrue(cache.evict(name, first)); + Assertions.assertSame(replacement, cache.getOrLoad(Mono.just(connection), name, loader).block()); + Assertions.assertFalse(cache.evict(name, first)); + Assertions.assertSame(replacement, cache.getOrLoad(Mono.just(connection), name, loader).block()); + Mockito.verify(replacement, Mockito.never()).closeAsync(anyString(), any(), eq(true)); + Assertions.assertTrue(cache.evict(name, replacement)); + Assertions.assertFalse(cache.evict(name, replacement)); + } + + @Test + void delayedOpenFailureDoesNotEvictReplacementSession() { + final ReactorSessionCache cache = createSessionCache(); + final Connection connection = mock(Connection.class); + final String name = "session-0"; + final ReactorSession first = session(name, endpointStatesSink(), null); + final ReactorSession replacement = session(name, endpointStatesSink(), null); + final Sinks.One opening = Sinks.one(); + when(first.open()).thenReturn(opening.asMono()); + final HashMap> lookup = new HashMap<>(); + lookup.put(name, sessions(first, replacement)); + final CacheLoader loader = new CacheLoader(lookup); + final RuntimeException failure = new RuntimeException("delayed open failure"); + StepVerifier.create(cache.getOrLoad(Mono.just(connection), name, loader)).then(() -> { + Assertions.assertTrue(cache.evict(name)); + Assertions.assertSame(replacement, cache.getOrLoad(Mono.just(connection), name, loader).block()); + opening.emitError(failure, Sinks.EmitFailureHandler.FAIL_FAST); + }).verifyErrorMatches(error -> error == failure); + Assertions.assertSame(replacement, cache.getOrLoad(Mono.just(connection), name, loader).block()); + Mockito.verify(replacement, Mockito.never()).closeAsync(anyString(), any(), eq(true)); + Assertions.assertTrue(cache.evict(name, replacement)); + } + private ReactorSessionCache createSessionCache() { return new ReactorSessionCache(CONNECTION_ID, NAMESPACE, handlerProvider, reactorProvider, OPEN_TIMEOUT, logger); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java index 577f907aa4720..07d4cb8566ba6 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClient.java @@ -1751,10 +1751,8 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { .addKeyValue("recoveryKind", recoveryKind) .log("Receive link creation failed, performing {} recovery.", recoveryKind, e); - // For both LINK and CONNECTION recovery, the session hosting the failed link may - // be stale. Ask the connection to remove it so the next retry creates a fresh - // session + link. CONNECTION recovery first invalidates that connection so the - // next retry rebuilds connection, session, and link. + // The receive-link factory evicts only the session that hosted the failed link. + // CONNECTION recovery also invalidates the connection used by this attempt. final ServiceBusAmqpConnection failedConnection = operationConnection.getAndSet(null); Mono recovery = Mono.fromRunnable(() -> { if (failedConnection == null) { @@ -1763,15 +1761,10 @@ private ServiceBusAsyncConsumer getOrCreateConsumer() { if (recoveryKind == RecoveryKind.CONNECTION) { connectionCacheWrapper.invalidateConnection(failedConnection); } - final boolean removed = failedConnection.removeSession(entityPath); - LOGGER.atVerbose() - .addKeyValue(LINK_NAME_KEY, linkName) - .addKeyValue("sessionRemoved", removed) - .log("Stale session removal during {} recovery.", recoveryKind); }).onErrorResume(error -> { LOGGER.atWarning() .addKeyValue(LINK_NAME_KEY, linkName) - .log("Error removing stale session during {} recovery.", recoveryKind, error); + .log("Error invalidating connection during {} recovery.", recoveryKind, error); return Mono.empty(); }); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnection.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnection.java index 009d0b53675f4..b7c8ee2ce5575 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnection.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnection.java @@ -17,6 +17,7 @@ import com.azure.core.amqp.implementation.ReactorHandlerProvider; import com.azure.core.amqp.implementation.ReactorProvider; import com.azure.core.amqp.implementation.ReactorSession; +import com.azure.core.amqp.implementation.RecoveryKind; import com.azure.core.amqp.implementation.RequestResponseChannel; import com.azure.core.amqp.implementation.RequestResponseChannelCache; import com.azure.core.amqp.implementation.RetryUtil; @@ -205,8 +206,10 @@ public Mono createReceiveLink(String linkName, String ent LOGGER.atVerbose().addKeyValue(ENTITY_PATH_KEY, entityPath).log("Get or create consumer."); final AmqpRetryPolicy retryPolicy = RetryUtil.getRetryPolicy(retryOptions); - return session.createConsumer(linkName, entityPath, entityType, retryOptions.getTryTimeout(), retryPolicy, - receiveMode, clientIdentifier); + return session + .createConsumer(linkName, entityPath, entityType, retryOptions.getTryTimeout(), retryPolicy, + receiveMode, clientIdentifier) + .doOnError(error -> removeFailedSession(session, error)); }); } @@ -242,11 +245,28 @@ public Mono createReceiveLink(String linkName, String ent LOGGER.atVerbose().addKeyValue(ENTITY_PATH_KEY, entityPath).log("Get or create consumer."); final AmqpRetryPolicy retryPolicy = RetryUtil.getRetryPolicy(retryOptions); - return session.createConsumer(linkName, entityPath, entityType, retryOptions.getTryTimeout(), retryPolicy, - receiveMode, clientIdentifier, sessionId); + return session + .createConsumer(linkName, entityPath, entityType, retryOptions.getTryTimeout(), retryPolicy, + receiveMode, clientIdentifier, sessionId) + .doOnError(error -> removeFailedSession(session, error)); }); } + private void removeFailedSession(ServiceBusSession session, Throwable error) { + final RecoveryKind recoveryKind = RecoveryKind.classify(error); + if (recoveryKind != RecoveryKind.LINK && recoveryKind != RecoveryKind.CONNECTION) { + return; + } + try { + final boolean removed = removeSession(session.getSessionName(), session); + LOGGER.atVerbose() + .addKeyValue("sessionRemoved", removed) + .log("Stale session removal during {} recovery.", recoveryKind); + } catch (RuntimeException cleanupError) { + LOGGER.atWarning().log("Error removing stale session during {} recovery.", recoveryKind, cleanupError); + } + } + @Override protected ReactorSession createSession(ProtonSessionWrapper session) { return new ServiceBusReactorSession(this, session, handlerProvider, linkProvider, getClaimsBasedSecurityNode(), diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java index 97d98407aa908..b2f1a84176fb8 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusReceiverAsyncClientTest.java @@ -1711,7 +1711,7 @@ ENTITY_TYPE, createNonSessionOptions(ServiceBusReceiveMode.PEEK_LOCK, PREFETCH, StepVerifier.create(client.receiveMessages()) .expectError(ServiceBusException.class) .verify(DEFAULT_TIMEOUT); - verify(connection).removeSession(ENTITY_PATH); + verify(connection, never()).removeSession(anyString()); verify(replacement, never()).removeSession(anyString()); verify(wrapper).invalidateConnection(connection); verify(wrapper, never()).invalidateConnection(replacement); diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java index 226adf51c85ff..a6fa8c536bfa0 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/ServiceBusSenderAsyncClientRecoveryIsolatedTest.java @@ -31,7 +31,10 @@ import com.azure.messaging.servicebus.implementation.MessagingEntityType; import com.azure.messaging.servicebus.implementation.ServiceBusAmqpLinkProvider; import com.azure.messaging.servicebus.implementation.ServiceBusReactorAmqpConnection; +import com.azure.messaging.servicebus.implementation.instrumentation.ReceiverKind; +import com.azure.messaging.servicebus.implementation.instrumentation.ServiceBusReceiverInstrumentation; import com.azure.messaging.servicebus.implementation.instrumentation.ServiceBusSenderInstrumentation; +import com.azure.messaging.servicebus.models.ServiceBusReceiveMode; import org.apache.qpid.proton.amqp.transport.DeliveryState; import org.apache.qpid.proton.engine.Connection; import org.apache.qpid.proton.engine.EndpointState; @@ -722,6 +725,47 @@ void sessionRemovalUsesTheSameKeyAsCreation(boolean crossEntityTransactions) { } } + @ParameterizedTest + @ValueSource(booleans = { false, true }) + void delayedReceiveLinkFailureDoesNotRemoveReplacementSession(boolean crossEntityTransactions) { + try (MockEndpoint endpoint = createMockEndpoint(2, new int[] { 0, 0 })) { + final ReactorConnectionCache cache + = createConnectionCache(() -> Mockito.spy(endpoint.arrange(crossEntityTransactions))); + final ServiceBusReactorAmqpConnection connection = cache.get().block(Duration.ofSeconds(5)); + final AmqpSession failedSession = connection.createSession(QUEUE_NAME).block(Duration.ofSeconds(5)); + final AtomicReference replacement = new AtomicReference<>(); + final Sinks.One failureHandled = Sinks.one(); + Mockito.doAnswer(invocation -> { + Assertions.assertTrue(connection.removeSession(QUEUE_NAME)); + replacement.set(connection.createSession(QUEUE_NAME).block(Duration.ofSeconds(5))); + Assertions.assertNotSame(failedSession, replacement.get()); + return Mono.error(RETRIABLE_LINK_ERROR) + .doAfterTerminate(() -> failureHandled.emitEmpty(Sinks.EmitFailureHandler.FAIL_FAST)); + }) + .doReturn(Mono.error(NON_RETRIABLE_ERROR_1)) + .when(connection) + .createReceiveLink(anyString(), anyString(), any(ServiceBusReceiveMode.class), any(), + any(MessagingEntityType.class), anyString()); + final ServiceBusReceiverAsyncClient client = new ServiceBusReceiverAsyncClient(FQDN, QUEUE_NAME, + MessagingEntityType.QUEUE, + ReceiverOptions.createNonSessionOptions(ServiceBusReceiveMode.PEEK_LOCK, 1, null, false), + new ConnectionCacheWrapper(cache), Duration.ofSeconds(10), + new ServiceBusReceiverInstrumentation(null, null, FQDN, QUEUE_NAME, null, ReceiverKind.ASYNC_RECEIVER), + messageSerializer, onClientClosed, CLIENT_IDENTIFIER); + try { + StepVerifier.create(client.receiveMessages()).then(() -> { + failureHandled.asMono().block(Duration.ofSeconds(5)); + Assertions.assertNotNull(replacement.get()); + Assertions.assertFalse(replacement.get().isDisposed()); + }).thenCancel().verify(Duration.ofSeconds(5)); + Assertions.assertSame(connection, cache.get().block(Duration.ofSeconds(5))); + } finally { + client.close(); + cache.dispose(); + } + } + } + @Test void shouldRecoverAfterNegotiationRetriesAreExhausted() { try (MockEndpoint endpoint = createMockEndpoint(1, new int[] { 2 })) { diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnectionTest.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnectionTest.java new file mode 100644 index 0000000000000..f86752e57e4cc --- /dev/null +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/ServiceBusReactorAmqpConnectionTest.java @@ -0,0 +1,111 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package com.azure.messaging.servicebus.implementation; + +import com.azure.core.amqp.AmqpRetryOptions; +import com.azure.core.amqp.AmqpSession; +import com.azure.core.amqp.exception.AmqpErrorCondition; +import com.azure.core.amqp.exception.AmqpException; +import com.azure.core.amqp.implementation.ConnectionOptions; +import com.azure.core.amqp.implementation.ReactorHandlerProvider; +import com.azure.core.amqp.implementation.ReactorProvider; +import com.azure.core.amqp.implementation.TokenManagerProvider; +import com.azure.core.amqp.implementation.handler.ConnectionHandler; +import com.azure.messaging.servicebus.models.ServiceBusReceiveMode; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.mockito.Mockito; +import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; +import reactor.core.publisher.Sinks; +import reactor.test.StepVerifier; + +import java.time.Duration; +import java.util.concurrent.atomic.AtomicReference; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class ServiceBusReactorAmqpConnectionTest { + @AfterEach + void teardown() { + Mockito.framework().clearInlineMock(this); + } + + @ParameterizedTest + @CsvSource({ + "false,false,LINK_DETACH_FORCED", + "false,true,LINK_DETACH_FORCED", + "true,false,LINK_DETACH_FORCED", + "true,true,LINK_DETACH_FORCED", + "false,false,CONNECTION_FORCED", + "false,true,CONNECTION_FORCED", + "true,false,CONNECTION_FORCED", + "true,true,CONNECTION_FORCED", + "false,false,RESOURCE_LIMIT_EXCEEDED", + "false,true,RESOURCE_LIMIT_EXCEEDED", + "true,false,RESOURCE_LIMIT_EXCEEDED", + "true,true,RESOURCE_LIMIT_EXCEEDED", + "false,false,NOT_ALLOWED", + "false,true,NOT_ALLOWED", + "true,false,NOT_ALLOWED", + "true,true,NOT_ALLOWED" }) + void receiveLinkFailureRemovesOnlyItsSession(boolean sessionReceiver, boolean replaceBeforeFailure, + AmqpErrorCondition condition) { + final ServiceBusSession failed = mock(ServiceBusSession.class); + final ServiceBusSession replacement = mock(ServiceBusSession.class); + when(failed.getSessionName()).thenReturn("cached-session"); + final Sinks.One pending = Sinks.one(); + when(failed.createConsumer(anyString(), anyString(), any(), any(), any(), any(), anyString())) + .thenReturn(pending.asMono()); + when(failed.createConsumer(anyString(), anyString(), any(), any(), any(), any(), anyString(), anyString())) + .thenReturn(pending.asMono()); + final AtomicReference cached = new AtomicReference<>(failed); + final ConnectionOptions options = mock(ConnectionOptions.class); + when(options.getRetry()).thenReturn(new AmqpRetryOptions()); + final ReactorHandlerProvider handlers = mock(ReactorHandlerProvider.class); + final ConnectionHandler handler = mock(ConnectionHandler.class); + when(handlers.createConnectionHandler(anyString(), any())).thenReturn(handler); + when(handler.getEndpointStates()).thenReturn(Flux.never()); + final ServiceBusReactorAmqpConnection connection + = new ServiceBusReactorAmqpConnection("connection", options, mock(ReactorProvider.class), handlers, + mock(ServiceBusAmqpLinkProvider.class), mock(TokenManagerProvider.class), null, false, true, false) { + @Override + public Mono createSession(String name) { + return Mono.just(cached.get()); + } + + @Override + protected boolean removeSession(String name, AmqpSession expected) { + Assertions.assertEquals("cached-session", name); + Assertions.assertSame(failed, expected); + return cached.compareAndSet(expected, null); + } + }; + final AmqpException error = new AmqpException(true, condition, "receive failure", null); + final Mono link = sessionReceiver + ? connection.createReceiveLink("link", "entity", ServiceBusReceiveMode.PEEK_LOCK, null, + MessagingEntityType.QUEUE, "client", "session-id") + : connection.createReceiveLink("link", "entity", ServiceBusReceiveMode.PEEK_LOCK, null, + MessagingEntityType.QUEUE, "client"); + StepVerifier.create(link).then(() -> { + if (replaceBeforeFailure) { + cached.set(replacement); + } + pending.emitError(error, Sinks.EmitFailureHandler.FAIL_FAST); + }).expectErrorMatches(actual -> actual == error).verify(Duration.ofSeconds(5)); + if (replaceBeforeFailure) { + Assertions.assertSame(replacement, cached.get()); + } else if (condition == AmqpErrorCondition.LINK_DETACH_FORCED + || condition == AmqpErrorCondition.CONNECTION_FORCED) { + Assertions.assertNull(cached.get()); + } else { + Assertions.assertSame(failed, cached.get()); + } + } +} From 6ede6b364ef7df0d1a17c314505b0ce0a85cafc4 Mon Sep 17 00:00:00 2001 From: Eldert Grootenboer Date: Thu, 29 Oct 2026 16:16:25 -0700 Subject: [PATCH 38/39] test: open AMQP internals to Service Bus recovery tests - Allow Mockito reflection in the modular Java test JVM - Keep production module exports and recovery behavior unchanged Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- sdk/servicebus/azure-messaging-servicebus/pom.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/servicebus/azure-messaging-servicebus/pom.xml b/sdk/servicebus/azure-messaging-servicebus/pom.xml index 6ff2000b712e3..aeb5d840d4a01 100644 --- a/sdk/servicebus/azure-messaging-servicebus/pom.xml +++ b/sdk/servicebus/azure-messaging-servicebus/pom.xml @@ -44,6 +44,7 @@ --add-exports com.azure.core/com.azure.core.implementation.util=ALL-UNNAMED --add-opens com.azure.core/com.azure.core.implementation.util=ALL-UNNAMED + --add-opens com.azure.core.amqp/com.azure.core.amqp.implementation=ALL-UNNAMED --add-reads com.azure.messaging.servicebus=com.azure.http.netty --add-reads com.azure.messaging.servicebus=com.azure.core.experimental --add-reads com.azure.messaging.servicebus=com.azure.core.tracing.opentelemetry From cff83c08cc98be6d6ebc468e0757b41a2ae6a95a Mon Sep 17 00:00:00 2001 From: Eldert Grootenboer Date: Fri, 30 Oct 2026 09:17:24 -0700 Subject: [PATCH 39/39] test: warm up Vert.x before Expect Continue timing checks - Exclude Netty allocation JFR initialization from the timing assertion - Preserve the 250 ms write deadline and production behavior Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../VertxHttpClientExpectContinueTests.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/sdk/core/azure-core-http-vertx/src/test/java/com/azure/core/http/vertx/VertxHttpClientExpectContinueTests.java b/sdk/core/azure-core-http-vertx/src/test/java/com/azure/core/http/vertx/VertxHttpClientExpectContinueTests.java index 427c5696a229c..eedc2d3b15237 100644 --- a/sdk/core/azure-core-http-vertx/src/test/java/com/azure/core/http/vertx/VertxHttpClientExpectContinueTests.java +++ b/sdk/core/azure-core-http-vertx/src/test/java/com/azure/core/http/vertx/VertxHttpClientExpectContinueTests.java @@ -8,6 +8,8 @@ import com.azure.core.http.HttpMethod; import com.azure.core.http.HttpRequest; import com.azure.core.http.HttpResponse; +import io.vertx.core.Vertx; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -43,6 +45,29 @@ public class VertxHttpClientExpectContinueTests { // Keep this below the server's 500 ms settle window and the one-second continue fallback. private static final Duration WRITE_TIMEOUT = Duration.ofMillis(250); + @BeforeAll + public static void warmUpHttpClient() throws Exception { + // Netty 4.2 initializes its allocation JFR event on the first header write. Keep that cold-start work + // outside the short write deadline used below to distinguish header writes from waiting for Continue. + Vertx vertx = Vertx.vertx(); + try (ExpectContinueServer server = new ExpectContinueServer()) { + server.start(); + HttpClient client = new VertxHttpClientBuilder().vertx(vertx) + .writeTimeout(Duration.ZERO) + .responseTimeout(RESPONSE_TIMEOUT) + .build(); + HttpRequest request = new HttpRequest(HttpMethod.PUT, server.url()).setBody(BODY); + request.getHeaders().set(HttpHeaderName.EXPECT, "100-continue"); + try (HttpResponse response = client.send(request).block(TEST_TIMEOUT)) { + assertNotNull(response); + assertEquals(201, response.getStatusCode()); + } + assertTrue(server.awaitRequest(), "the warm-up request never reached the server"); + } finally { + vertx.close().toCompletionStage().toCompletableFuture().get(TEST_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS); + } + } + @ParameterizedTest @ValueSource(strings = { "100-continue", "100-Continue", " 100-continue ", "100-continue, foo" }) public void withholdsBodyUntilContinue(String expectHeaderValue) throws Exception {