[EventHubs] Retry a CBS authentication failure once on a fresh connection - #7380
Draft
Johnathan W (j7nw4r) wants to merge 27 commits into
Draft
[EventHubs] Retry a CBS authentication failure once on a fresh connection#7380Johnathan W (j7nw4r) wants to merge 27 commits into
Johnathan W (j7nw4r) wants to merge 27 commits into
Conversation
Add uAMQP mock server tests for producer and consumer authentication\nrecovery, retry budgets, stale link handling, and credential failures.\nExtend the test mock only to script CBS responses and link generations.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 7 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds bounded uAMQP authentication recovery for Event Hubs producer and receiver operations.
Changes:
- Rebuilds producer/receiver stacks once after eligible authentication failures.
- Preserves retry budgets, receiver offsets, cancellation, and teardown safety.
- Adds focused uAMQP recovery tests and supporting AMQP test hooks.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Documents authentication recovery. |
claims_based_security.hpp |
Adds the CBS token-failure exception. |
connection.cpp |
Wraps uAMQP PutToken failures. |
mock_amqp_server.hpp |
Extends AMQP test-server behavior. |
consumer_client.hpp |
Tracks partition-client states. |
partition_client.hpp |
Adds backend-specific receiver state. |
producer_client.hpp |
Adds shared producer-call retry state. |
consumer_client.cpp |
Creates and closes recoverable receivers. |
partition_client.cpp |
Implements receiver rebuilding and resumption. |
producer_client.cpp |
Implements producer authentication recovery. |
eventhubs_utilities.hpp |
Adds recovery helpers and factory APIs. |
retry_operation.hpp |
Defines authentication recovery state. |
retry_operation.cpp |
Implements bounded recovery delay handling. |
auth_recovery_test.cpp |
Tests producer and receiver recovery scenarios. |
test/ut/CMakeLists.txt |
Registers uAMQP recovery tests and hooks. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
The uAMQP put-token failure now throws CbsPutTokenFailedException, a std::runtime_error that carries the original AuthenticationException. The Event Hubs producer and receiver unwrap it. The management client did not, so GetEventHubProperties and GetPartitionProperties threw the internal type, and a caller that catches AuthenticationException no longer saw the failure. TestManagement.ManagementOpenCloseAuthenticatedFail failed on uAMQP for the same reason. ManagementClientImpl::Open and ExecuteOperation now catch the marker and throw the original exception again. The core-amqp CHANGELOG names the new type on MessageSender::Open and MessageReceiver::Open.
The uAMQP PartitionClient constructor copies the receiver and the session of the first stack into m_receiver and m_session, and RebuildReceiver never replaced them. Those copies own the first ConnectionImpl, and the uAMQP connection closes only when its last owner goes away. A rebuilt client therefore kept the first socket open until the client was destroyed. RebuildReceiver now assigns the receiver and the session of the installed stack to those members, so the old connection is released. Receives run under the receive lock, so nothing reads the members while they change.
ConsumerClient::CreatePartitionClient on uAMQP built the complete receiver stack, with the network work and the authentication, and only then tested the closing flag and closed the stack again. The flag is now tested under the lock before the build. The check after the build stays, because Close can start while the build runs.
RetryOperation::WaitForAuthenticationRecovery was the generic cancellable wait, and the receiver rebuild used it for ordinary retries too. It is now WaitForRetryDelay. The receiver recover lambda called it from both branches of a condition; only the attempt counter differed. EstablishSenderWithRetry tested the Execute result and returned on both paths. Behavior is unchanged.
The one extra attempt for a CBS open Error in CreateBatch became a loop on the ordinary retry budget when the establish step moved under RetryOperation::Execute. The catch now carries the reason that bound is the only one available: uAMQP returns no value that separates a transient open failure from a permanent one. The CHANGELOG entry links issue 7376, states the new bound for CreateBatch and Send, and states that each retry phase gets its own budget after an authentication recovery.
The mock cancelled its shared listener context when any connection reached End or Error. That stopped the accept loop and every endpoint message loop after the first client connection closed, so a client that reconnected got no CBS reply and waited forever. The endpoint loop also exited when its link maps became empty, and a finished thread stays joinable, so a later attach never restarted it. Only StopListening cancels the listener context now. The endpoint loop runs until StopProcessing. A state change to Idle counts as a link disconnect, because a client that drops its connection raises no detach event, and the loop tolerates a name it already removed. A new attach waits up to two seconds for the loop to remove a stale link with the same name. The server-side links skip the authentication step, which has no credential and would read a source or target address that an Event Hubs client does not send. The core-amqp suite on Linux uAMQP reports the same 223 passed and 3 failed before and after this change.
The tests had never run: they skip on macOS and no CI leg builds uAMQP. On Linux they hung or crashed. The endpoint names used localhost while the connection string used 127.0.0.1, and the consumer endpoint carried a port that the consumer client does not put in its partition URL. Two message checks compared const char pointers with EXPECT_EQ. A delivery worker captured the LinkEndpoint parameter by reference after the base class had released it to the new link, so the detach used a dangling handle. A receive returns as soon as it holds an event and the queue is empty, so the resume test collects events across calls, and both receiver tests wait for the mock to send the first event and the unauthorized detach before the receive starts. The resume test now captures the selector filter of each receiver attach and checks that the second attach resumes after offset 10. Twelve of the thirteen tests pass on Linux uAMQP with a local fix for the message annotation encoding in the uAMQP send path; the two receiver tests need that fix.
The tests cleared the producer snapshot hook and the partition close hook by hand after each use. A failed ASSERT returns before that line and leaves the hook armed for the next test. A HookGuard now sets the hook and clears it when the scope ends.
A rejected CBS put-token on a uAMQP properties call must reach the caller as Azure::Core::Credentials::AuthenticationException. The management client path has no unwrap for the internal put-token marker, so this test fails on this branch with "it throws Azure::Core::Amqp::_detail::CbsPutTokenFailedException" for both the producer and the consumer. It stays red until the management path unwraps the marker.
The vendored uAMQP sender wrote delivery annotations, message annotations, and the footer as bare maps. The receiving link expects a described section there, so it logged "Error decoding message" and went to the error state. The uAMQP receiver stores these sections as bare maps after it strips the descriptor, so the fix wraps them in the sender size pass and encode pass, the same way the sender already wraps the application properties. Upstream azure-uamqp-c encodes message annotations bare too and does not send the footer or the delivery annotations at all. Adds a mock server round trip that fails on the old sender and passes on the new one.
Member
Author
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
Member
Author
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
Member
Author
|
/azp run cpp - storage - ci |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds one bounded uAMQP authentication recovery for Event Hubs producer and receiver calls. A public call can replace its complete AMQP stack and authenticate on a fresh connection after a CBS
PutTokennon-OK result or anamqp:unauthorized-accesstransfer result. Rust AMQP and Rust FFI production behavior remain unchanged.Motivation
A stale uAMQP connection can reject CBS token authentication after credential acquisition succeeds. Without bounded recovery, the public call fails although a replacement connection can succeed. The recovery must not make authorization failures globally transient, retry credential rejection, or classify a producer attach failure as recoverable.
Changes
PutTokenresult.MessageSender::OpenandMessageReceiver::Openthrow it; the management client unwraps it, soGetEventHubPropertiesandGetPartitionPropertiesstill throwAuthenticationException.MaxRetries == 0disables this recovery. Each ordinary retry phase gets its ownMaxRetriesbudget after a recovery.amqp:unauthorized-accesstransfer result; a producer attach failure stays permanent. A receiver attach rejection reaches the client through the receive result, so it gets the same single recovery.Errorthrough the ordinary retry budget in bothCreateBatchandSend. Before this changeCreateBatchmade one extra attempt. CBS openInvalidandCancelledstay permanent.RetryDelay, the existing 0.8 to 1.3 jitter, theMaxRetryDelaycap, and Context-aware waits. The default 800 ms delay gives a 640 to 1,040 ms range.PartitionClienta consumer-owned replaceable receiver stack. Recovery preserves the client handle, resumes exclusively after the last delivered offset, releases the replaced connection, and preserves final pending errors.CreatePartitionClientbefore and after the stack build, and protects concurrent stack teardown and producer session snapshots.ConsumerClientandPartitionClientlayouts identical with no backend macro,ENABLE_UAMQP, andENABLE_RUST_AMQP. Closes the destination uAMQP receiver state duringPartitionClientmove assignment. Stores consumer partition states as weak references and promotes only live states during close.Test plan
No CI leg builds the uAMQP backend, so the uAMQP results come from a local Linux container (Ubuntu 22.04, gcc 11, arm64,
-DDISABLE_RUST_IN_BUILD=ON).TestCbs.CbsOpenNoListenerandTestMessageSendReceive.SenderCloseWhileUnsettledSendIgnoresLateDispositionfail the same way on this branch before these commits.TestManagement.ManagementOpenCloseAuthenticatedFailpasses; it failed before the management unwrap.AuthRecoveryTestcases passed, each under 6 s, with the three concurrency cases and the two receiver cases also passing 3 of 3 on repeat. The remaining non-live tests pass exceptCheckpointStoreTest.TestCheckpoints, which needsEVENTHUB_CONSUMER_GROUP.TestMessageSendReceive.ReceiverDecodesAnnotationsAndFooterfailed before the sender patch with "Message receiver has transitioned to the error state" and passes after it.ENABLE_UAMQP, andENABLE_RUST_AMQPproduced identical layouts:ConsumerClientsize 624 and alignment 8;PartitionClientsize 368 and alignment 8.git diff --checkpassed. clang-format 11 is not installed on the build hosts; a clang-format 21 dry run flags only lines this PR did not add.Fixes #7376