Azure / Azure/azure-sdk-for-cpp

[EventHubs] A server-busy rejection tears down the whole connection on every retry

Open Beginner friendly
#7,333 1 comment 0 reactions 0 assignees View on GitHub
Client Event Hubs needs-team-attention Service Attention
Dominant language
C++
Stars
205
Forks
172
Avg merge
1d 3h
Merged PRs (30d)
37

Description

## Summary

A `com.microsoft:server-busy` rejection discards the cached sender, the session, and the connection for the partition. So every retry of a throttled send pays a full TCP, TLS, SASL, CBS, and attach round trip, at the moment when the service asked for less load.

## Motivation

`ShouldInvalidateSender` keeps the cached sender for one condition only, `amqp:link:message-size-exceeded` (`sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp:111-114`). Every other condition returns true.

`ProducerClient::Send` calls that helper in its catch block, and a true result calls `InvalidateSender` (`sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp:200-207`). `InvalidateSender` closes and removes the sender, the session, and the connection for that partition id, so the next attempt builds all three again.

`AmqpErrorCondition::ServerBusyError` holds the value `com.microsoft:server-busy` (`sdk/core/azure-core-amqp/src/models/amqp_error.cpp:152`; the declaration is at `sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/models/amqp_error.hpp:175`). `EventHubsExceptionFactory::IsErrorTransient` lists that condition as transient (`sdk/eventhubs/azure-messaging-eventhubs/src/eventhubs_utilities.cpp:133-145`), so `RetryOperation::Execute` retries the send.

The service reports this condition on the disposition of the transfer, and it keeps the link attached. That reply arrives on the same path as the message size rejection: the uAMQP sender maps a rejected delivery state to `MessageSendStatus::Error` and it gives the AMQP error back in the send tuple (`sdk/core/azure-core-amqp/src/impl/uamqp/amqp/message_sender.cpp:507-523`), and `Send` turns that tuple into an `EventHubsException` (`sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp:177-185`). The message size case is already on the keep list for exactly this reason. The server busy case has the same shape and is not on the list.

The .NET producer keeps its link in a `FaultTolerantAmqpObject` and it builds a new link when that object faults (`AmqpProducer.cs`). A rejected disposition does not fault the link object, so .NET keeps the link and retries the transfer on it.

This behaviour became visible with #7328, which is the link reattach part of #7254. Before that change, a failed send left the cached sender in place.

## Proposal

Add `AmqpErrorCondition::ServerBusyError` to the keep list in `ShouldInvalidateSender`, next to `amqp:link:message-size-exceeded`. Compare against the constant, and not against a literal string, so the two spellings cannot drift apart.

Extend the truth table in `sdk/eventhubs/azure-messaging-eventhubs/test/ut/reattach_policy_test.cpp` with a `com.microsoft:server-busy` row that asserts that the sender stays.

Note that the server busy delay is a separate item in #7254, and it stays out of scope here. This change covers the link only.

Contributor guide

Open the contributing guide

Research direction

Start in sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp at ShouldInvalidateSender and review the existing message-size-exceeded keep-list entry. Then inspect sdk/eventhubs/azure-messaging-eventhubs/test/ut/reattach_policy_test.cpp and its truth table. Done means the server-busy condition keeps the sender and the updated reattach policy test passes.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
distributed-systems
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
86/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.