Azure / Azure/azure-sdk-for-cpp
WS2: Restore connection recovery, token refresh, and retry classification
- Dominant language
- C++
- Stars
- 205
- Forks
- 172
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 37
Description
## Summary
The C++ Event Hubs clients do not recover from ordinary network and authentication faults. A sender link is cached for the life of the client and is never rebuilt. A receiver dies permanently on the first detach. The CBS token is cached with no expiry test and is never refreshed. The retry engine ignores the transient classifier that the package already computes, and its backoff cannot be cancelled.
This workstream restores the transport hardening layer that the .NET library encodes. It carries the authentication flow, so it is sequenced early, directly after the AMQP transport decision in #7253. That decision gates this work: `src/producer_client.cpp` contains five transport branches, one of them inside `EnsureSender` at line 214, and `sdk/core/azure-core-amqp/src/amqp/connection.cpp` is written almost entirely under `#if ENABLE_UAMQP`. Starting before the decision means writing these fixes twice. Severity tags follow the key in #7252.
## Motivation
The .NET producer holds its link in a `FaultTolerantAmqpObject` and calls `SendLink.GetOrCreateAsync` on every retry attempt (`sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpProducer.cs:107,179,290`), so a faulted link is rebuilt on the next attempt. The C++ `ProducerClient` caches one `MessageSender` per partition for the life of the client and never rebuilds it (`src/producer_client.cpp:196-234`). `Send` does retry, but each attempt calls `GetSender(...)` and receives the same dead sender (`src/producer_client.cpp:87-113`). `PartitionClient::ReceiveEvents` throws on the first AMQP error and never rebuilds the receiver (`src/partition_client.cpp:225-272`).
The .NET connection scope schedules a proactive CBS token refresh with a 7 minute buffer before expiry (`src/Amqp/AmqpConnectionScope.cs:73,736`). The C++ `ConnectionImpl::AuthenticateAudience` returns the cached token on a cache hit with no expiry test, and `azure-core-amqp` contains no refresh timer (`sdk/core/azure-core-amqp/src/amqp/connection.cpp:175-183,227`).
The .NET retry policy retries on `EventHubsException.IsTransient` plus a defined allowlist of framework exceptions, and it adds a delay for server busy (`sdk/eventhub/Azure.Messaging.EventHubs.Shared/src/Core/BasicRetryPolicy.cs:142-189`). The C++ retry engine treats only `amqp:link:message-size-exceeded` as fatal and retries everything else, including authorization failures (`src/retry_operation.cpp:13,57,67-83`). The `IsErrorTransient` classifier exists but only fills an exception field; the retry path never reads it (`src/eventhubs_utilities.cpp:87-108`).
The impact: any client that outlives one token lifetime (about 60 to 90 minutes for Microsoft Entra ID) or one network fault stops working and cannot recover without a process restart.
## Proposal
- [ ] Rebuild the sender link inside the send retry loop, and rebuild the session and the connection when they are also gone. Do not retry against a cached dead sender (`src/producer_client.cpp:87-113,196-234`). Reference: `AmqpProducer.cs:290`. **[GA blocker]**
- [ ] Rebuild the receiver on link detach and resume from the last received offset inside `ReceiveEvents` (`src/partition_client.cpp:225-272`). **[GA blocker]**
- [ ] Read an `x-opt-offset` annotation that is not a string. `ReceivedEventData` assigns `Offset` only when the annotation value has the type `AmqpValueType::String`, and it discards every other type without a log (`src/event_data.cpp:59-68`). The service can send an integer in this annotation. The Go SDK had the same defect, and it corrected the defect with a conversion to a string (`Azure/azure-event-hubs-go` issue #159, corrected by pull request #183). An empty `Offset` removes the resume position that the item above needs, so this item comes first. **[GA blocker]**
- [ ] Add proactive CBS token refresh with an expiry buffer, and make the token cache respect `ExpiresOn` (`sdk/core/azure-core-amqp/src/amqp/connection.cpp:175-227`). Reference: `AmqpConnectionScope.cs:73,736`. **[GA blocker]**
- [ ] Make the retry engine consult the transient classification. Retry only errors where `IsTransient` is true, plus a defined allowlist of non-`EventHubsException` types. Stop retrying authorization and precondition failures (`src/retry_operation.cpp:13-83`; classifier at `src/eventhubs_utilities.cpp:87-108`). Reference: `BasicRetryPolicy.cs:142-176`. **[GA blocker]**
- [ ] Make backoff waits cancellable through `Azure::Core::Context`. Replace the three `std::this_thread::sleep_for` calls (`src/retry_operation.cpp:43,60,77`). **[GA blocker]**
- [ ] Add a per-attempt timeout option, equivalent to the .NET `TryTimeout` in `EventHubsRetryOptions`. **[GA quality bar]**
- [ ] Apply an extra delay on a server busy response, as .NET does (`BasicRetryPolicy.cs:189`). **[GA quality bar]**
## Validation
- [ ] A producer survives an induced link detach and continues to send without a process restart.
- [ ] A consumer survives an induced link detach and resumes from the last received offset.
- [ ] A client runs past two token lifetimes and continues to send and to receive.
- [ ] An authorization failure stops retrying immediately instead of consuming the full retry budget.
- [ ] Cancelling the context during a backoff wait returns promptly.
Contributor guide
Assessment
This issue has not been assessed yet.