From bcb0d3eabe0b1eea8c31129cdc24040a63f82718 Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Mon, 26 Oct 2026 13:16:48 -0400 Subject: [PATCH 01/11] test: reproduce Event Hubs discovery cleanup failures --- .../impl/uamqp/amqp/private/session_impl.hpp | 7 +- .../src/impl/uamqp/amqp/session.cpp | 9 +- .../azure-core-amqp/test/ut/CMakeLists.txt | 4 +- .../test/ut/session_cleanup_tests.cpp | 120 ++++++++ .../src/private/eventhubs_utilities.hpp | 28 +- .../test/ut/CMakeLists.txt | 9 +- .../test/ut/properties_cleanup_test.cpp | 285 ++++++++++++++++++ 7 files changed, 453 insertions(+), 9 deletions(-) create mode 100644 sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp create mode 100644 sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp diff --git a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/session_impl.hpp b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/session_impl.hpp index 75a1d64d2f..49b34cb2b1 100644 --- a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/session_impl.hpp +++ b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/session_impl.hpp @@ -42,6 +42,9 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { class SessionImpl final : public std::enable_shared_from_this { public: + // Internal seam for exercising native shutdown failures without a live broker. + using EndSession = int (*)(SESSION_HANDLE, char const*, char const*); + SessionImpl( std::shared_ptr<_detail::ConnectionImpl> parentConnection, _internal::Endpoint& newEndpoint, @@ -50,7 +53,8 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { SessionImpl( std::shared_ptr<_detail::ConnectionImpl> parentConnection, _internal::SessionOptions const& options, - _internal::SessionEvents* eventHandler = nullptr); + _internal::SessionEvents* eventHandler = nullptr, + EndSession endSession = session_end); ~SessionImpl() noexcept; SessionImpl(SessionImpl const&) = delete; @@ -79,6 +83,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { private: SessionImpl(); + EndSession m_endSession{session_end}; bool m_connectionAsyncStarted{false}; bool m_isBegun{false}; std::shared_ptr<_detail::ConnectionImpl> m_connectionToPoll; diff --git a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/session.cpp b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/session.cpp index 25c994cfe9..e69035ed52 100644 --- a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/session.cpp +++ b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/session.cpp @@ -87,8 +87,9 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { SessionImpl::SessionImpl( std::shared_ptr<_detail::ConnectionImpl> connection, _internal::SessionOptions const& options, - _internal::SessionEvents* eventHandler) - : m_connectionToPoll(connection), + _internal::SessionEvents* eventHandler, + EndSession endSession) + : m_endSession{endSession}, m_connectionToPoll(connection), m_session{session_create(*connection, SessionImpl::OnLinkAttachedFn, this)}, m_options{options}, m_eventHandler{eventHandler} @@ -172,7 +173,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { } // When we end the session, it clears all the links, so we need to ensure that the // m_newLinkAttachedQueue.Clear(); - if (session_end(m_session.get(), nullptr, nullptr)) + if (m_endSession(m_session.get(), nullptr, nullptr)) { throw std::runtime_error("Could not begin session"); } @@ -193,7 +194,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { } // When we end the session, it clears all the links, so we need to ensure that the // m_newLinkAttachedQueue.Clear(); - if (session_end( + if (m_endSession( m_session.get(), condition.empty() ? nullptr : condition.c_str(), description.empty() ? nullptr : description.c_str())) diff --git a/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt b/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt index 5984ff201d..67c5d8c053 100644 --- a/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt +++ b/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt @@ -37,6 +37,7 @@ add_executable(azure-core-amqp-tests message_source_target.cpp operation_timeout_tests.cpp mock_amqp_server.hpp + session_cleanup_tests.cpp session_tests.cpp amqp_performative_tests.cpp ${UAMQP_ONLY_TESTS} @@ -44,7 +45,8 @@ add_executable(azure-core-amqp-tests if (USE_UAMQP) -target_include_directories(azure-core-amqp-tests PRIVATE ../../src/amqp/impl/uamqp/amqp/private) +target_include_directories(azure-core-amqp-tests SYSTEM PRIVATE ${UAMQP_INC_FOLDER}) +target_link_libraries(azure-core-amqp-tests PRIVATE umock_c azure_macro_utils_c aziotsharedutil) endif() if (USE_RUST_AMQP) target_include_directories(azure-core-amqp-tests PRIVATE ../../src/impl/rust_amqp/rust_amqp/rust_wrapper) diff --git a/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp b/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp new file mode 100644 index 0000000000..6628249e1a --- /dev/null +++ b/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp @@ -0,0 +1,120 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#if ENABLE_UAMQP +#include "../../src/impl/uamqp/amqp/private/session_impl.hpp" +#include "mock_amqp_server.hpp" + +#include + +#include +#include + +namespace Azure { namespace Core { namespace Amqp { namespace Tests { + namespace { + int FailEnd(SESSION_HANDLE, char const*, char const*) { return 1; } + + void FailedEndWithSharedOwner(bool withError) + { + std::weak_ptr<_detail::SessionImpl> sessionLifetime; + std::weak_ptr<_detail::ConnectionImpl> connectionLifetime; + { + MessageTests::AmqpServerMock server; + server.StartListening(); + _internal::ConnectionOptions options; + options.Port = server.GetPort(); + _internal::Connection connection("localhost", nullptr, options); + connectionLifetime = _detail::ConnectionFactory::GetImpl(connection); + std::unique_ptr<_internal::ManagementClient> management; + { + auto impl = std::make_shared<_detail::SessionImpl>( + _detail::ConnectionFactory::GetImpl(connection), + _internal::SessionOptions{}, nullptr, FailEnd); + sessionLifetime = impl; + auto session = _detail::SessionFactory::CreateFromInternal(impl); + session.Begin({}); + management = std::make_unique<_internal::ManagementClient>( + session.CreateManagementClient("hub", {})); + if (withError) + { + EXPECT_THROW(session.End("amqp:internal-error", "shutdown", {}), std::runtime_error); + } + else + { + EXPECT_THROW(session.End({}), std::runtime_error); + } + } + // A management client still owns the session. Native destruction must wait for it. + EXPECT_FALSE(sessionLifetime.expired()); + management.reset(); + EXPECT_TRUE(sessionLifetime.expired()); + } + Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); + EXPECT_TRUE(connectionLifetime.expired()); + } + + int FailOnceEnd(SESSION_HANDLE session, char const* condition, char const* description) + { + static bool failed = false; + if (!failed) + { + failed = true; + return 1; + } + return session_end(session, condition, description); + } + } // namespace + + TEST(SessionCleanupDeathTest, FailedEndReleasesLastOwner) + { + EXPECT_EXIT({ + FailedEndWithSharedOwner(false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } + + TEST(SessionCleanupDeathTest, FailedEndWithErrorReleasesLastOwner) + { + EXPECT_EXIT({ + FailedEndWithSharedOwner(true); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } + + TEST(SessionCleanupDeathTest, FailedEndCanBeRetried) + { + EXPECT_EXIT({ + { + MessageTests::AmqpServerMock server; + server.StartListening(); + _internal::ConnectionOptions options; + options.Port = server.GetPort(); + _internal::Connection connection("localhost", nullptr, options); + auto session = _detail::SessionFactory::CreateFromInternal( + std::make_shared<_detail::SessionImpl>( + _detail::ConnectionFactory::GetImpl(connection), + _internal::SessionOptions{}, nullptr, FailOnceEnd)); + session.Begin({}); + EXPECT_THROW(session.End({}), std::runtime_error); + EXPECT_NO_THROW(session.End("amqp:internal-error", "retry", {})); + EXPECT_THROW(session.End({}), std::runtime_error); + } + Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } + + TEST(SessionCleanupDeathTest, MissingEndStillAsserts) + { + EXPECT_DEATH({ + MessageTests::AmqpServerMock server; + server.StartListening(); + _internal::ConnectionOptions options; + options.Port = server.GetPort(); + _internal::Connection connection("localhost", nullptr, options); + auto session = connection.CreateSession(); + session.Begin({}); + }, "Session was not ended before destruction"); + } +}}}} // namespace Azure::Core::Amqp::Tests +#endif diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp index 78fd0d68ac..374ad9ce92 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp @@ -144,10 +144,33 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail class EventHubsPropertiesClient { public: + using ManagementClientFactory = Azure::Core::Amqp::_internal::ManagementClient (*)( + Azure::Core::Amqp::_internal::Session const&, + std::string const&, + Azure::Core::Amqp::_internal::ManagementClientOptions const&); + + static Azure::Core::Amqp::_internal::ManagementClient CreateManagementClient( + Azure::Core::Amqp::_internal::Session const& session, + std::string const& eventHub, + Azure::Core::Amqp::_internal::ManagementClientOptions const& options) + { + return session.CreateManagementClient(eventHub, options); + } + + // Internal injection point for failures between beginning the session and assigning the client. + EventHubsPropertiesClient( + Azure::Core::Amqp::_internal::Session session, + std::string eventHubName, + ManagementClientFactory createManagementClient = CreateManagementClient) + : m_session{std::move(session)}, m_eventHub{std::move(eventHubName)}, + m_createManagementClient{createManagementClient} + { + } + EventHubsPropertiesClient( const Azure::Core::Amqp::_internal::Connection& connection, std::string eventHubName) - : m_session{connection.CreateSession()}, m_eventHub{std::move(eventHubName)} {}; + : EventHubsPropertiesClient(connection.CreateSession(), std::move(eventHubName)) {}; ~EventHubsPropertiesClient() { @@ -332,7 +355,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail managementClientOptions.EnableTrace = _detail::EnableAmqpTrace; managementClientOptions.ExpectedStatusCodeKeyName = "status-code"; m_managementClient = std::make_unique( - m_session.CreateManagementClient(m_eventHub, managementClientOptions)); + m_createManagementClient(m_session, m_eventHub, managementClientOptions)); auto openResult{m_managementClient->Open(context)}; if (openResult != Azure::Core::Amqp::_internal::ManagementOpenStatus::Ok) @@ -354,6 +377,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail std::unique_ptr m_managementClient; bool m_managementClientIsOpen{false}; std::string m_eventHub; + ManagementClientFactory m_createManagementClient; }; class EventHubsUtilities { diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt index b21bcfe95b..cd924e7dcc 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt @@ -29,6 +29,7 @@ add_executable ( processor_load_balancer_test.cpp processor_test.cpp producer_client_test.cpp + properties_cleanup_test.cpp reattach_connection_string_test.cpp reattach_policy_test.cpp retry_operation_test.cpp @@ -49,7 +50,13 @@ endif() if(USE_RUST_AMQP) target_compile_definitions(azure-messaging-eventhubs-test PRIVATE ENABLE_RUST_AMQP) else() - target_compile_definitions(azure-messaging-eventhubs-test PRIVATE ENABLE_UAMQP) + find_package(umock_c CONFIG REQUIRED) + find_package(azure_macro_utils_c CONFIG REQUIRED) + find_package(azure_c_shared_utility CONFIG REQUIRED) + target_compile_definitions(azure-messaging-eventhubs-test PRIVATE ENABLE_UAMQP _azure_TESTING_BUILD) + target_include_directories(azure-messaging-eventhubs-test SYSTEM PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/../../../../core/azure-core-amqp/src/impl/uamqp/vendor/azure-uamqp-c/inc) + target_link_libraries(azure-messaging-eventhubs-test PRIVATE umock_c azure_macro_utils_c aziotsharedutil) endif() target_link_libraries( diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp new file mode 100644 index 0000000000..ad585ca558 --- /dev/null +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp @@ -0,0 +1,285 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#if ENABLE_UAMQP +#include "private/eventhubs_utilities.hpp" +#include "../../../../core/azure-core-amqp/src/impl/uamqp/amqp/private/session_impl.hpp" + +#include + +#include "../../../../core/azure-core-amqp/test/ut/mock_amqp_server.hpp" + +#include +#include +#include +#include +#if defined(AZ_PLATFORM_WINDOWS) +#include +#else +#include +#include +#include +#endif +#include + +namespace { +std::uint16_t AvailablePort() +{ + // Initialize the platform before socket APIs (required on Windows). + auto state = Azure::Core::Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance(); + (void)state; + auto socketHandle = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); + sockaddr_in address{}; + address.sin_family = AF_INET; + address.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + if (bind(socketHandle, reinterpret_cast(&address), sizeof(address)) != 0) + { + throw std::runtime_error("Could not reserve a test port"); + } +#if defined(AZ_PLATFORM_WINDOWS) + int size = sizeof(address); +#else + socklen_t size = sizeof(address); +#endif + auto result = getsockname(socketHandle, reinterpret_cast(&address), &size); +#if defined(AZ_PLATFORM_WINDOWS) + closesocket(socketHandle); +#else + close(socketHandle); +#endif + if (result != 0) + { + throw std::runtime_error("Could not read the test port"); + } + return ntohs(address.sin_port); +} +} // namespace + +namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { + namespace { + namespace Amqp = Azure::Core::Amqp; + using _detail::EventHubsPropertiesClient; + + Amqp::_internal::ManagementClient FailCreation( + Amqp::_internal::Session const&, + std::string const&, + Amqp::_internal::ManagementClientOptions const&) + { + throw std::runtime_error("injected management creation failure"); + } + + int FailEnd(SESSION_HANDLE, char const*, char const*) { return 1; } + + class PropertiesEndpoint final : public Amqp::Tests::MessageTests::MockServiceEndpoint { + public: + PropertiesEndpoint() : MockServiceEndpoint("$management", {}) {} + + private: + void MessageReceived( + std::string const&, std::shared_ptr const& request) override + { + Amqp::Models::AmqpMessage response; + response.ApplicationProperties["status-code"] = 200; + response.Properties.CorrelationId = request->Properties.MessageId; + Amqp::Models::AmqpMap body; + body["name"] = "hub"; + body["created_at"] = Amqp::Models::AmqpTimestamp(std::chrono::milliseconds(0)).AsAmqpValue(); + body["partition_ids"] = Amqp::Models::AmqpArray{"0", "1"}.AsAmqpValue(); + response.SetBody(body.AsAmqpValue()); + auto result = GetMessageSender().Send(response, GetListenerContext()); + EXPECT_EQ(std::get<0>(result), Amqp::_internal::MessageSendStatus::Ok); + } + }; + + int endCalls = 0; + int CountEnd(SESSION_HANDLE session, char const* condition, char const* description) + { + ++endCalls; + return session_end(session, condition, description); + } + + void SuccessfulDiscovery(bool cancelledClose) + { + std::atomic closeFailureLogged{false}; + Azure::Core::Diagnostics::Logger::SetLevel(Azure::Core::Diagnostics::Logger::Level::Warning); + Azure::Core::Diagnostics::Logger::SetListener( + [&](Azure::Core::Diagnostics::Logger::Level, std::string const& line) { + if (line.find("Exception while closing the management client") != std::string::npos) + { + closeFailureLogged = true; + } + }); + { + Amqp::Tests::MessageTests::AmqpServerMock server(AvailablePort()); + server.AddServiceEndpoint(std::make_shared()); + server.StartListening(); + { + Amqp::_internal::ConnectionOptions options; + options.Port = server.GetPort(); + Amqp::_internal::Connection connection("localhost", nullptr, options); + auto session = Amqp::_detail::SessionFactory::CreateFromInternal( + std::make_shared( + Amqp::_detail::ConnectionFactory::GetImpl(connection), + Amqp::_internal::SessionOptions{}, nullptr, CountEnd)); + endCalls = 0; + { + EventHubsPropertiesClient properties(session, "hub"); + auto context = Azure::Core::Context().WithDeadline( + std::chrono::system_clock::now() + std::chrono::seconds(10)); + auto result = properties.GetEventHubsProperties("hub", context); + EXPECT_EQ(result.Name, "hub"); + EXPECT_EQ(result.PartitionIds, (std::vector{"0", "1"})); + if (cancelledClose) + { + context.Cancel(); + } + properties.Close(context); + properties.Close(context); + } + EXPECT_EQ(endCalls, 1); + } + server.StopListening(); + } + Azure::Core::Diagnostics::Logger::SetListener(nullptr); + EXPECT_EQ(closeFailureLogged.load(), cancelledClose); + Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); + } + + void PartialInitialization(bool explicitClose, bool failEnd) + { + std::weak_ptr connectionLifetime; + { + Amqp::Tests::MessageTests::AmqpServerMock server(AvailablePort()); + server.StartListening(); + Amqp::_internal::ConnectionOptions options; + options.Port = server.GetPort(); + Amqp::_internal::Connection connection("localhost", nullptr, options); + connectionLifetime = Amqp::_detail::ConnectionFactory::GetImpl(connection); + auto session = Amqp::_detail::SessionFactory::CreateFromInternal( + std::make_shared( + Amqp::_detail::ConnectionFactory::GetImpl(connection), + Amqp::_internal::SessionOptions{}, nullptr, failEnd ? FailEnd : session_end)); + { + EventHubsPropertiesClient properties(session, "hub", FailCreation); + // Retrying discovery must not acquire a second polling reference. + for (int attempt = 0; attempt != 2; ++attempt) + { + try + { + properties.GetEventHubsProperties("hub", {}); + ADD_FAILURE() << "Expected management creation to fail"; + } + catch (std::runtime_error const& error) + { + EXPECT_STREQ(error.what(), "injected management creation failure"); + } + } + if (explicitClose) + { + properties.Close({}); + properties.Close({}); + } + } + } + Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); + EXPECT_TRUE(connectionLifetime.expired()); + } + + void FailedOpen(bool failEnd) + { + { + Amqp::Tests::MessageTests::AmqpServerMock server(AvailablePort()); + server.StartListening(); + Amqp::_internal::ConnectionOptions options; + options.Port = server.GetPort(); + Amqp::_internal::Connection connection("localhost", nullptr, options); + auto session = Amqp::_detail::SessionFactory::CreateFromInternal( + std::make_shared( + Amqp::_detail::ConnectionFactory::GetImpl(connection), + Amqp::_internal::SessionOptions{}, nullptr, failEnd ? FailEnd : session_end)); + EventHubsPropertiesClient properties(session, "hub"); + Azure::Core::Context cancelled; + cancelled.Cancel(); + EXPECT_ANY_THROW(properties.GetEventHubsProperties("hub", cancelled)); + properties.Close({}); + properties.Close({}); + } + Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); + } + } // namespace + + TEST(PropertiesCleanupDeathTest, SuccessfulDiscoveryAndRepeatedClose) + { + EXPECT_EXIT({ + SuccessfulDiscovery(false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } + + TEST(PropertiesCleanupDeathTest, ManagementCloseFailureStillEndsSession) + { + EXPECT_EXIT({ + SuccessfulDiscovery(true); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } + + TEST(PropertiesCleanupDeathTest, CreationFailureThenRepeatedClose) + { + EXPECT_EXIT({ + PartialInitialization(true, false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } + + TEST(PropertiesCleanupDeathTest, CreationFailureThenDestruction) + { + EXPECT_EXIT({ + PartialInitialization(false, false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } + + TEST(PropertiesCleanupDeathTest, CreationFailureAndSessionEndFailure) + { + EXPECT_EXIT({ + PartialInitialization(true, true); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } + + TEST(PropertiesCleanupDeathTest, OpenFailureThenRepeatedClose) + { + EXPECT_EXIT({ + FailedOpen(false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } + + TEST(PropertiesCleanupDeathTest, OpenFailureAndSessionEndFailure) + { + EXPECT_EXIT({ + FailedOpen(true); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } + + TEST(PropertiesCleanupDeathTest, ProducerDiscoveryFailureThenRepeatedClose) + { + EXPECT_EXIT({ + { + ProducerClientOptions options; + options.RetryOptions.MaxRetries = 0; + ProducerClient producer("localhost", "hub", nullptr, options); + Azure::Core::Context cancelled; + cancelled.Cancel(); + EXPECT_ANY_THROW(producer.GetEventHubProperties(cancelled)); + EXPECT_NO_THROW(producer.Close()); + EXPECT_NO_THROW(producer.Close()); + } + Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, ::testing::ExitedWithCode(0), ""); + } +}}}} // namespace Azure::Messaging::EventHubs::Test +#endif From 3d9a9b11f643bbad78c066a40e9ef97df41f1886 Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Mon, 26 Oct 2026 13:31:22 -0400 Subject: [PATCH 02/11] fix: safely clean up Event Hubs discovery sessions --- sdk/core/azure-core-amqp/CHANGELOG.md | 2 + .../inc/azure/core/amqp/internal/session.hpp | 6 + .../impl/uamqp/amqp/private/session_impl.hpp | 2 + .../src/impl/uamqp/amqp/session.cpp | 42 ++-- .../azure-core-amqp/test/ut/CMakeLists.txt | 1 + .../test/ut/cleanup_test_listener.hpp | 76 ++++++ .../test/ut/session_cleanup_tests.cpp | 110 +++++---- .../azure-messaging-eventhubs/CHANGELOG.md | 2 + .../src/private/eventhubs_utilities.hpp | 69 +++--- .../test/ut/properties_cleanup_test.cpp | 229 ++++++++++-------- 10 files changed, 339 insertions(+), 200 deletions(-) create mode 100644 sdk/core/azure-core-amqp/test/ut/cleanup_test_listener.hpp diff --git a/sdk/core/azure-core-amqp/CHANGELOG.md b/sdk/core/azure-core-amqp/CHANGELOG.md index 16652ac226..259e3233a1 100644 --- a/sdk/core/azure-core-amqp/CHANGELOG.md +++ b/sdk/core/azure-core-amqp/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +- Fixed uAMQP session destruction after a failed `End`. Native resources and the connection polling reference are released when the last session owner is destroyed; callers must still end sessions before destruction. + - Fixed uAMQP message annotation encoding so messages with annotations, including Event Hubs partition keys, include the required AMQP section descriptor. [[#7426]](https://github.com/Azure/azure-sdk-for-cpp/issues/7426) ### Other Changes diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/session.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/session.hpp index 8b0ccd36f9..0d869fc649 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/session.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/session.hpp @@ -233,11 +233,17 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal { /** @brief Ends operations on the session. * + * @note With uAMQP, a failed protocol shutdown throws. The native session and its polling + * reference are then released when the last session owner is destroyed. Calling End is + * still required before destroying a begun session. */ void End(Azure::Core::Context const& context); /** @brief Ends operations on the session. * + * @note With uAMQP, a failed protocol shutdown throws. The native session and its polling + * reference are then released when the last session owner is destroyed. Calling End is + * still required before destroying a begun session. */ void End( std::string const& condition_value, diff --git a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/session_impl.hpp b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/session_impl.hpp index 49b34cb2b1..0c2a446258 100644 --- a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/session_impl.hpp +++ b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/session_impl.hpp @@ -86,6 +86,8 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { EndSession m_endSession{session_end}; bool m_connectionAsyncStarted{false}; bool m_isBegun{false}; + // A failed protocol shutdown still requires native destruction and a polling release. + bool m_endFailed{false}; std::shared_ptr<_detail::ConnectionImpl> m_connectionToPoll; UniqueAmqpSession m_session; _internal::SessionOptions m_options; diff --git a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/session.cpp b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/session.cpp index e69035ed52..1b05a6b99b 100644 --- a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/session.cpp +++ b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/session.cpp @@ -34,19 +34,23 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { SessionImpl::~SessionImpl() noexcept { - if (m_isBegun) + if (m_isBegun && !m_endFailed) { AZURE_ASSERT_MSG(false, "Session was not ended before destruction."); } - // If we have a mismatched begin/end pair, we need to stop polling on the connection so it - // gets cleaned up properly. + // A failed End leaves the native session alive while management clients and links own it. + // The last owner reaches here after those objects have gone. session_destroy frees the + // endpoint and native resources even if the protocol end frame cannot be sent. + { + auto lock{m_connectionToPoll->Lock()}; + m_session.reset(); + } + // Release our polling reference exactly once, outside the connection lock. if (m_connectionAsyncStarted) { m_connectionToPoll->EnableAsyncOperation(false); } - auto lock{m_connectionToPoll->Lock()}; - m_session.reset(); } SessionImpl::SessionImpl( @@ -160,28 +164,13 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { } m_isBegun = true; + m_endFailed = false; // Mark the connection as async so that we can use the async APIs. GetConnection()->EnableAsyncOperation(true); m_connectionAsyncStarted = true; } - void SessionImpl::End(Azure::Core::Context const&) - { - if (!m_isBegun) - { - throw std::runtime_error("Session End without corresponding Begin."); - } - // When we end the session, it clears all the links, so we need to ensure that the - // m_newLinkAttachedQueue.Clear(); - if (m_endSession(m_session.get(), nullptr, nullptr)) - { - throw std::runtime_error("Could not begin session"); - } - // Mark the connection as async so that we can use the async APIs. - GetConnection()->EnableAsyncOperation(false); - m_connectionAsyncStarted = false; - m_isBegun = false; - } + void SessionImpl::End(Azure::Core::Context const& context) { End({}, {}, context); } void SessionImpl::End( const std::string& condition, @@ -192,19 +181,20 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { { throw std::runtime_error("Session End without corresponding Begin."); } - // When we end the session, it clears all the links, so we need to ensure that the - // m_newLinkAttachedQueue.Clear(); + // The native end releases the link endpoints, even if sending the end frame fails. if (m_endSession( m_session.get(), condition.empty() ? nullptr : condition.c_str(), description.empty() ? nullptr : description.c_str())) { - throw std::runtime_error("Could not begin session"); + m_endFailed = true; + throw std::runtime_error("Could not end session"); } - // Mark the connection as async so that we can use the async APIs. + // Release the polling reference acquired by Begin. GetConnection()->EnableAsyncOperation(false); m_connectionAsyncStarted = false; m_isBegun = false; + m_endFailed = false; } void SessionImpl::SendDetach( diff --git a/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt b/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt index 67c5d8c053..94d8b76951 100644 --- a/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt +++ b/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt @@ -29,6 +29,7 @@ add_executable(azure-core-amqp-tests async_operation_queue_tests.cpp azure_core_amqp_tests.cpp claim_based_security_tests.cpp + cleanup_test_listener.hpp connection_string_tests.cpp connection_tests.cpp global_state_tests.cpp diff --git a/sdk/core/azure-core-amqp/test/ut/cleanup_test_listener.hpp b/sdk/core/azure-core-amqp/test/ut/cleanup_test_listener.hpp new file mode 100644 index 0000000000..7b6ed9110d --- /dev/null +++ b/sdk/core/azure-core-amqp/test/ut/cleanup_test_listener.hpp @@ -0,0 +1,76 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#pragma once + +#include +#include + +#if defined(AZ_PLATFORM_WINDOWS) +#include +#else +#include + +#include +#include +#endif + +#include +#include + +namespace Azure { namespace Core { namespace Amqp { namespace Tests { + // Allows Begin to connect over TCP without starting a peer AMQP session. Cleanup + // fault tests do not need peer callbacks, and must not race a mock server's session teardown. + class CleanupTestListener final { + public: + CleanupTestListener() + { + Common::_detail::GlobalStateHolder::GlobalStateInstance(); + m_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); + if (m_socket == InvalidSocket) + { + throw std::runtime_error("Could not create cleanup test listener"); + } + sockaddr_in address{}; + address.sin_family = AF_INET; + address.sin_addr.s_addr = htonl(INADDR_LOOPBACK); +#if defined(AZ_PLATFORM_WINDOWS) + int size = sizeof(address); +#else + socklen_t size = sizeof(address); +#endif + if (bind(m_socket, reinterpret_cast(&address), sizeof(address)) != 0 + || listen(m_socket, 1) != 0 + || getsockname(m_socket, reinterpret_cast(&address), &size) != 0) + { + Close(); + throw std::runtime_error("Could not listen on cleanup test port"); + } + m_port = ntohs(address.sin_port); + } + + ~CleanupTestListener() { Close(); } + CleanupTestListener(CleanupTestListener const&) = delete; + CleanupTestListener& operator=(CleanupTestListener const&) = delete; + std::uint16_t GetPort() const { return m_port; } + + private: + using Socket = decltype(socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)); + static constexpr Socket InvalidSocket = static_cast(-1); + Socket m_socket{InvalidSocket}; + std::uint16_t m_port{}; + + void Close() + { + if (m_socket != InvalidSocket) + { +#if defined(AZ_PLATFORM_WINDOWS) + closesocket(m_socket); +#else + close(m_socket); +#endif + m_socket = InvalidSocket; + } + } + }; +}}}} // namespace Azure::Core::Amqp::Tests diff --git a/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp b/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp index 6628249e1a..f103766e9b 100644 --- a/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp @@ -3,11 +3,12 @@ #if ENABLE_UAMQP #include "../../src/impl/uamqp/amqp/private/session_impl.hpp" -#include "mock_amqp_server.hpp" +#include "cleanup_test_listener.hpp" #include #include + #include namespace Azure { namespace Core { namespace Amqp { namespace Tests { @@ -19,8 +20,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { std::weak_ptr<_detail::SessionImpl> sessionLifetime; std::weak_ptr<_detail::ConnectionImpl> connectionLifetime; { - MessageTests::AmqpServerMock server; - server.StartListening(); + CleanupTestListener server; _internal::ConnectionOptions options; options.Port = server.GetPort(); _internal::Connection connection("localhost", nullptr, options); @@ -29,7 +29,9 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { { auto impl = std::make_shared<_detail::SessionImpl>( _detail::ConnectionFactory::GetImpl(connection), - _internal::SessionOptions{}, nullptr, FailEnd); + _internal::SessionOptions{}, + nullptr, + FailEnd); sessionLifetime = impl; auto session = _detail::SessionFactory::CreateFromInternal(impl); session.Begin({}); @@ -65,56 +67,76 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { } } // namespace - TEST(SessionCleanupDeathTest, FailedEndReleasesLastOwner) + class SessionCleanupDeathTest : public ::testing::Test { + protected: + void SetUp() override + { + // Re-exec the child instead of inheriting AMQP polling-thread mutexes across fork. + ::testing::FLAGS_gtest_death_test_style = "threadsafe"; + } + }; + + TEST_F(SessionCleanupDeathTest, FailedEndReleasesLastOwner) { - EXPECT_EXIT({ - FailedEndWithSharedOwner(false); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + FailedEndWithSharedOwner(false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } - TEST(SessionCleanupDeathTest, FailedEndWithErrorReleasesLastOwner) + TEST_F(SessionCleanupDeathTest, FailedEndWithErrorReleasesLastOwner) { - EXPECT_EXIT({ - FailedEndWithSharedOwner(true); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + FailedEndWithSharedOwner(true); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } - TEST(SessionCleanupDeathTest, FailedEndCanBeRetried) + TEST_F(SessionCleanupDeathTest, FailedEndCanBeRetried) { - EXPECT_EXIT({ - { - MessageTests::AmqpServerMock server; - server.StartListening(); - _internal::ConnectionOptions options; - options.Port = server.GetPort(); - _internal::Connection connection("localhost", nullptr, options); - auto session = _detail::SessionFactory::CreateFromInternal( - std::make_shared<_detail::SessionImpl>( - _detail::ConnectionFactory::GetImpl(connection), - _internal::SessionOptions{}, nullptr, FailOnceEnd)); - session.Begin({}); - EXPECT_THROW(session.End({}), std::runtime_error); - EXPECT_NO_THROW(session.End("amqp:internal-error", "retry", {})); - EXPECT_THROW(session.End({}), std::runtime_error); - } - Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + { + CleanupTestListener server; + _internal::ConnectionOptions options; + options.Port = server.GetPort(); + _internal::Connection connection("localhost", nullptr, options); + auto session = _detail::SessionFactory::CreateFromInternal( + std::make_shared<_detail::SessionImpl>( + _detail::ConnectionFactory::GetImpl(connection), + _internal::SessionOptions{}, + nullptr, + FailOnceEnd)); + session.Begin({}); + EXPECT_THROW(session.End({}), std::runtime_error); + EXPECT_NO_THROW(session.End("amqp:internal-error", "retry", {})); + EXPECT_THROW(session.End({}), std::runtime_error); + } + Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } - TEST(SessionCleanupDeathTest, MissingEndStillAsserts) + TEST_F(SessionCleanupDeathTest, MissingEndStillAsserts) { - EXPECT_DEATH({ - MessageTests::AmqpServerMock server; - server.StartListening(); - _internal::ConnectionOptions options; - options.Port = server.GetPort(); - _internal::Connection connection("localhost", nullptr, options); - auto session = connection.CreateSession(); - session.Begin({}); - }, "Session was not ended before destruction"); + EXPECT_DEATH( + { + CleanupTestListener server; + _internal::ConnectionOptions options; + options.Port = server.GetPort(); + _internal::Connection connection("localhost", nullptr, options); + auto session = connection.CreateSession(); + session.Begin({}); + }, + ""); } }}}} // namespace Azure::Core::Amqp::Tests #endif diff --git a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md index 577f4b3819..c628e5ea53 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md +++ b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +- Fixed producer cleanup after partition discovery fails during management-client initialization or session shutdown. Cleanup now ends an acquired session independently of management-client creation and supports repeated close calls. + ### Other Changes ## 1.0.0-beta.15 (2026-09-02) diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp index 374ad9ce92..26259c2a79 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp @@ -174,14 +174,11 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail ~EventHubsPropertiesClient() { - // A destructor is noexcept, so an exception here terminates the process. An idle - // link detach after 30 minutes makes a throwing close the ordinary case here. + // Finish both management and session cleanup, including partially initialized clients. + // Explicit Close and destruction use the same ownership checks. try { - if (m_managementClientIsOpen) - { - m_managementClient->Close(); - } + Close({}); } catch (std::exception const& ex) { @@ -235,10 +232,11 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail } auto bodyMap = body.AsMap(); properties.Name = static_cast(bodyMap["name"]); - properties.CreatedOn = Azure::DateTime(std::chrono::system_clock::from_time_t( - std::chrono::duration_cast( - static_cast(bodyMap["created_at"].AsTimestamp())) - .count())); + properties.CreatedOn = Azure::DateTime( + std::chrono::system_clock::from_time_t( + std::chrono::duration_cast( + static_cast(bodyMap["created_at"].AsTimestamp())) + .count())); auto partitions = bodyMap["partition_ids"].AsArray(); for (const auto& partition : partitions) { @@ -295,11 +293,12 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail // For the last enqueued offset is returned as a string. Convert to an int64. properties.LastEnqueuedOffset = static_cast(bodyMap["last_enqueued_offset"]); - properties.LastEnqueuedTimeUtc = Azure::DateTime(std::chrono::system_clock::from_time_t( - std::chrono::duration_cast( - static_cast( - bodyMap["last_enqueued_time_utc"].AsTimestamp())) - .count())); + properties.LastEnqueuedTimeUtc = Azure::DateTime( + std::chrono::system_clock::from_time_t( + std::chrono::duration_cast( + static_cast( + bodyMap["last_enqueued_time_utc"].AsTimestamp())) + .count())); properties.IsEmpty = bodyMap["is_partition_empty"]; } return properties; @@ -308,27 +307,30 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail void Close(Azure::Core::Context const& context) { std::unique_lock lock(m_managementClientMutex); - if (m_managementClient) + if (m_managementClientIsOpen) { - if (m_managementClientIsOpen) + // ManagementClient::Close releases its links even when it reports a close failure. + m_managementClientIsOpen = false; + try + { + m_managementClient->Close(context); + } + catch (std::exception const& ex) { - // Drop the flag first, so a throwing close cannot make the destructor close again. - m_managementClientIsOpen = false; - try - { - m_managementClient->Close(context); - } - catch (std::exception const& ex) - { - Azure::Core::Diagnostics::_internal::Log::Stream( - Azure::Core::Diagnostics::Logger::Level::Warning) - << "Exception while closing the management client: " << ex.what(); - } + Azure::Core::Diagnostics::_internal::Log::Stream( + Azure::Core::Diagnostics::Logger::Level::Warning) + << "Exception while closing the management client: " << ex.what(); } - // Its own try block, so a failed management close does not strand the session. + } + // Drop link ownership before ending the session. Its native end frees link endpoints. + m_managementClient.reset(); + // Beginning the session precedes management creation, which can throw before assignment. + if (m_sessionIsBegun) + { try { m_session.End(context); + m_sessionIsBegun = false; } catch (std::exception const& ex) { @@ -346,7 +348,11 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail std::unique_lock lock(m_managementClientMutex); if (!m_managementClient) { - m_session.Begin(context); + if (!m_sessionIsBegun) + { + m_session.Begin(context); + m_sessionIsBegun = true; + } // Create a management client off the session. // Eventhubs management APIs return a status code in the "status-code" application @@ -376,6 +382,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail Azure::Core::Amqp::_internal::Session m_session; std::unique_ptr m_managementClient; bool m_managementClientIsOpen{false}; + bool m_sessionIsBegun{false}; std::string m_eventHub; ManagementClientFactory m_createManagementClient; }; diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp index ad585ca558..2b2250ebd7 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp @@ -2,56 +2,25 @@ // Licensed under the MIT License. #if ENABLE_UAMQP -#include "private/eventhubs_utilities.hpp" #include "../../../../core/azure-core-amqp/src/impl/uamqp/amqp/private/session_impl.hpp" - -#include - +#include "../../../../core/azure-core-amqp/test/ut/cleanup_test_listener.hpp" #include "../../../../core/azure-core-amqp/test/ut/mock_amqp_server.hpp" +#include "private/eventhubs_utilities.hpp" #include #include +#include + #include #include -#if defined(AZ_PLATFORM_WINDOWS) -#include -#else -#include -#include -#include -#endif + #include namespace { std::uint16_t AvailablePort() { - // Initialize the platform before socket APIs (required on Windows). - auto state = Azure::Core::Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance(); - (void)state; - auto socketHandle = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); - sockaddr_in address{}; - address.sin_family = AF_INET; - address.sin_addr.s_addr = htonl(INADDR_LOOPBACK); - if (bind(socketHandle, reinterpret_cast(&address), sizeof(address)) != 0) - { - throw std::runtime_error("Could not reserve a test port"); - } -#if defined(AZ_PLATFORM_WINDOWS) - int size = sizeof(address); -#else - socklen_t size = sizeof(address); -#endif - auto result = getsockname(socketHandle, reinterpret_cast(&address), &size); -#if defined(AZ_PLATFORM_WINDOWS) - closesocket(socketHandle); -#else - close(socketHandle); -#endif - if (result != 0) - { - throw std::runtime_error("Could not read the test port"); - } - return ntohs(address.sin_port); + Azure::Core::Amqp::Tests::CleanupTestListener listener; + return listener.GetPort(); } } // namespace @@ -72,18 +41,24 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { class PropertiesEndpoint final : public Amqp::Tests::MessageTests::MockServiceEndpoint { public: - PropertiesEndpoint() : MockServiceEndpoint("$management", {}) {} + PropertiesEndpoint(bool failDiscovery = false) + : MockServiceEndpoint("$management", {}), m_failDiscovery{failDiscovery} + { + } private: + bool m_failDiscovery; void MessageReceived( - std::string const&, std::shared_ptr const& request) override + std::string const&, + std::shared_ptr const& request) override { Amqp::Models::AmqpMessage response; - response.ApplicationProperties["status-code"] = 200; + response.ApplicationProperties["status-code"] = m_failDiscovery ? 500 : 200; response.Properties.CorrelationId = request->Properties.MessageId; Amqp::Models::AmqpMap body; body["name"] = "hub"; - body["created_at"] = Amqp::Models::AmqpTimestamp(std::chrono::milliseconds(0)).AsAmqpValue(); + body["created_at"] + = Amqp::Models::AmqpTimestamp(std::chrono::milliseconds(0)).AsAmqpValue(); body["partition_ids"] = Amqp::Models::AmqpArray{"0", "1"}.AsAmqpValue(); response.SetBody(body.AsAmqpValue()); auto result = GetMessageSender().Send(response, GetListenerContext()); @@ -91,6 +66,31 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { } }; + void ProducerDiscoveryFailure() + { + { + Amqp::Tests::MessageTests::AmqpServerMock server(AvailablePort()); + server.AddServiceEndpoint(std::make_shared(true)); + server.StartListening(); + { + ProducerClientOptions options; + options.RetryOptions.MaxRetries = 0; + ProducerClient producer( + "Endpoint=sb://localhost:" + std::to_string(server.GetPort()) + + "/;SharedAccessKeyName=test;SharedAccessKey=YWJj;UseDevelopmentEmulator=true", + "hub", + options); + auto context = Azure::Core::Context().WithDeadline( + std::chrono::system_clock::now() + std::chrono::seconds(10)); + EXPECT_THROW(producer.GetEventHubProperties(context), EventHubsException); + producer.Close(context); + producer.Close(context); + } + server.StopListening(); + } + Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); + } + int endCalls = 0; int CountEnd(SESSION_HANDLE session, char const* condition, char const* description) { @@ -120,7 +120,9 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { auto session = Amqp::_detail::SessionFactory::CreateFromInternal( std::make_shared( Amqp::_detail::ConnectionFactory::GetImpl(connection), - Amqp::_internal::SessionOptions{}, nullptr, CountEnd)); + Amqp::_internal::SessionOptions{}, + nullptr, + CountEnd)); endCalls = 0; { EventHubsPropertiesClient properties(session, "hub"); @@ -132,6 +134,10 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { if (cancelledClose) { context.Cancel(); + // Stop client connection polling after draining its current poll. The peer cannot + // complete DETACH before the cancelled close waits, so the close reliably throws. + Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->RemovePollable( + Amqp::_detail::ConnectionFactory::GetImpl(connection)); } properties.Close(context); properties.Close(context); @@ -149,8 +155,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { { std::weak_ptr connectionLifetime; { - Amqp::Tests::MessageTests::AmqpServerMock server(AvailablePort()); - server.StartListening(); + Amqp::Tests::CleanupTestListener server; Amqp::_internal::ConnectionOptions options; options.Port = server.GetPort(); Amqp::_internal::Connection connection("localhost", nullptr, options); @@ -158,7 +163,9 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { auto session = Amqp::_detail::SessionFactory::CreateFromInternal( std::make_shared( Amqp::_detail::ConnectionFactory::GetImpl(connection), - Amqp::_internal::SessionOptions{}, nullptr, failEnd ? FailEnd : session_end)); + Amqp::_internal::SessionOptions{}, + nullptr, + failEnd ? FailEnd : session_end)); { EventHubsPropertiesClient properties(session, "hub", FailCreation); // Retrying discovery must not acquire a second polling reference. @@ -188,15 +195,16 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { void FailedOpen(bool failEnd) { { - Amqp::Tests::MessageTests::AmqpServerMock server(AvailablePort()); - server.StartListening(); + Amqp::Tests::CleanupTestListener server; Amqp::_internal::ConnectionOptions options; options.Port = server.GetPort(); Amqp::_internal::Connection connection("localhost", nullptr, options); auto session = Amqp::_detail::SessionFactory::CreateFromInternal( std::make_shared( Amqp::_detail::ConnectionFactory::GetImpl(connection), - Amqp::_internal::SessionOptions{}, nullptr, failEnd ? FailEnd : session_end)); + Amqp::_internal::SessionOptions{}, + nullptr, + failEnd ? FailEnd : session_end)); EventHubsPropertiesClient properties(session, "hub"); Azure::Core::Context cancelled; cancelled.Cancel(); @@ -208,78 +216,101 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { } } // namespace - TEST(PropertiesCleanupDeathTest, SuccessfulDiscoveryAndRepeatedClose) + class PropertiesCleanupDeathTest : public ::testing::Test { + protected: + void SetUp() override + { + // Re-exec the child instead of inheriting AMQP polling-thread mutexes across fork. + ::testing::FLAGS_gtest_death_test_style = "threadsafe"; + } + }; + + TEST_F(PropertiesCleanupDeathTest, SuccessfulDiscoveryAndRepeatedClose) { - EXPECT_EXIT({ - SuccessfulDiscovery(false); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + SuccessfulDiscovery(false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } - TEST(PropertiesCleanupDeathTest, ManagementCloseFailureStillEndsSession) + TEST_F(PropertiesCleanupDeathTest, ManagementCloseFailureStillEndsSession) { - EXPECT_EXIT({ - SuccessfulDiscovery(true); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + SuccessfulDiscovery(true); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } - TEST(PropertiesCleanupDeathTest, CreationFailureThenRepeatedClose) + TEST_F(PropertiesCleanupDeathTest, CreationFailureThenRepeatedClose) { - EXPECT_EXIT({ - PartialInitialization(true, false); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + PartialInitialization(true, false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } - TEST(PropertiesCleanupDeathTest, CreationFailureThenDestruction) + TEST_F(PropertiesCleanupDeathTest, CreationFailureThenDestruction) { - EXPECT_EXIT({ - PartialInitialization(false, false); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + PartialInitialization(false, false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } - TEST(PropertiesCleanupDeathTest, CreationFailureAndSessionEndFailure) + TEST_F(PropertiesCleanupDeathTest, CreationFailureAndSessionEndFailure) { - EXPECT_EXIT({ - PartialInitialization(true, true); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + PartialInitialization(true, true); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } - TEST(PropertiesCleanupDeathTest, OpenFailureThenRepeatedClose) + TEST_F(PropertiesCleanupDeathTest, OpenFailureThenRepeatedClose) { - EXPECT_EXIT({ - FailedOpen(false); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + FailedOpen(false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } - TEST(PropertiesCleanupDeathTest, OpenFailureAndSessionEndFailure) + TEST_F(PropertiesCleanupDeathTest, OpenFailureAndSessionEndFailure) { - EXPECT_EXIT({ - FailedOpen(true); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + FailedOpen(true); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } - TEST(PropertiesCleanupDeathTest, ProducerDiscoveryFailureThenRepeatedClose) + TEST_F(PropertiesCleanupDeathTest, ProducerDiscoveryFailureThenRepeatedClose) { - EXPECT_EXIT({ - { - ProducerClientOptions options; - options.RetryOptions.MaxRetries = 0; - ProducerClient producer("localhost", "hub", nullptr, options); - Azure::Core::Context cancelled; - cancelled.Cancel(); - EXPECT_ANY_THROW(producer.GetEventHubProperties(cancelled)); - EXPECT_NO_THROW(producer.Close()); - EXPECT_NO_THROW(producer.Close()); - } - Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, ::testing::ExitedWithCode(0), ""); + EXPECT_EXIT( + { + ProducerDiscoveryFailure(); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); } }}}} // namespace Azure::Messaging::EventHubs::Test #endif From 394aeda71915a93df993cca16a7593bcb59c40fc Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Mon, 26 Oct 2026 15:07:45 -0400 Subject: [PATCH 03/11] fix: allow AMQP test dependency names in spelling checks --- sdk/core/azure-core-amqp/test/ut/CMakeLists.txt | 2 ++ sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt | 2 ++ 2 files changed, 4 insertions(+) diff --git a/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt b/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt index 94d8b76951..acab1a49ca 100644 --- a/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt +++ b/sdk/core/azure-core-amqp/test/ut/CMakeLists.txt @@ -1,6 +1,8 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. +# cspell: words aziotsharedutil + cmake_minimum_required (VERSION 3.13) set(CMAKE_EXPORT_COMPILE_COMMANDS ON) diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt index cd924e7dcc..759dbc7315 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt @@ -1,6 +1,8 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. +# cspell: words umock aziotsharedutil + cmake_minimum_required (VERSION 3.13) project (azure-messaging-eventhubs-test LANGUAGES CXX) From 9e5b3daf4f777cbc6e6b609f56840eadb0f07bb4 Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Mon, 26 Oct 2026 15:56:29 -0400 Subject: [PATCH 04/11] test(eventhubs): refresh checkpoint store recordings --- sdk/eventhubs/assets.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/eventhubs/assets.json b/sdk/eventhubs/assets.json index 521a91878e..1b0d927a32 100644 --- a/sdk/eventhubs/assets.json +++ b/sdk/eventhubs/assets.json @@ -2,5 +2,5 @@ "AssetsRepo": "Azure/azure-sdk-assets", "AssetsRepoPrefixPath": "cpp", "TagPrefix": "cpp/eventhubs", - "Tag": "cpp/eventhubs_54bfb9d8e7" + "Tag": "cpp/eventhubs_4c104aa882" } From b5a4da55f35e4395649f2829b7458c3c51a4df20 Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Mon, 26 Oct 2026 17:46:03 -0400 Subject: [PATCH 05/11] fix(eventhubs): synchronize properties queries with cleanup --- .../azure-messaging-eventhubs/CHANGELOG.md | 1 + .../src/private/eventhubs_utilities.hpp | 49 +++-- .../test/ut/properties_cleanup_test.cpp | 168 +++++++++++++++++- 3 files changed, 202 insertions(+), 16 deletions(-) diff --git a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md index c628e5ea53..4cc3768c22 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md +++ b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md @@ -9,6 +9,7 @@ ### Bugs Fixed - Fixed producer cleanup after partition discovery fails during management-client initialization or session shutdown. Cleanup now ends an acquired session independently of management-client creation and supports repeated close calls. +- Synchronized properties queries with cleanup during producer recovery. Cleanup cancels an active query before releasing its management client and session, without cancelling the caller's context. ### Other Changes diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp index 26259c2a79..f3d0aa896e 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp @@ -194,7 +194,9 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail Core::Context const& context) { - EnsureManagementClient(context); + std::unique_lock lock(m_managementClientMutex); + auto operationContext = CreateOperationContext(context); + EnsureManagementClient(*operationContext); // Send a message to the management endpoint to retrieve the properties of the eventhub. Azure::Core::Amqp::Models::AmqpMessage message; @@ -206,7 +208,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail "com.microsoft:eventhub" /* type of operation */, "" /* locales */, message, - context); + *operationContext); Models::EventHubProperties properties; if (result.Status != Azure::Core::Amqp::_internal::ManagementOperationStatus::Ok) @@ -251,7 +253,9 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail std::string const& partitionId, Core::Context const& context) { - EnsureManagementClient(context); + std::unique_lock lock(m_managementClientMutex); + auto operationContext = CreateOperationContext(context); + EnsureManagementClient(*operationContext); // Send a message to the management endpoint to retrieve the properties of the eventhub. Azure::Core::Amqp::Models::AmqpMessage message; @@ -265,7 +269,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail "com.microsoft:partition" /* type of operation */, "" /* locales */, message, - context); + *operationContext); Models::EventHubPartitionProperties properties; if (result.Status != Azure::Core::Amqp::_internal::ManagementOperationStatus::Ok) @@ -306,6 +310,16 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail void Close(Azure::Core::Context const& context) { + { + // Cancel before waiting for the operation lock: a query may have no caller deadline. + // Reject queries on stale references once producer/consumer invalidation starts. + std::lock_guard cancellationLock(m_operationContextMutex); + m_isClosed = true; + if (auto operationContext = m_operationContext.lock()) + { + operationContext->Cancel(); + } + } std::unique_lock lock(m_managementClientMutex); if (m_managementClientIsOpen) { @@ -342,10 +356,24 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail } private: - void EnsureManagementClient(Azure::Core::Context const& context) + std::shared_ptr CreateOperationContext( + Azure::Core::Context const& context) { + std::lock_guard lock(m_operationContextMutex); + if (m_isClosed) + { + throw Azure::Core::OperationCancelledException("Properties client is closed."); + } + // A child inherits caller cancellation but can be cancelled without changing the caller. + auto operationContext + = std::make_shared(context.WithDeadline(context.GetDeadline())); + m_operationContext = operationContext; + return operationContext; + } - std::unique_lock lock(m_managementClientMutex); + // The getter owns m_managementClientMutex through initialization and operation completion. + void EnsureManagementClient(Azure::Core::Context const& context) + { if (!m_managementClient) { if (!m_sessionIsBegun) @@ -372,13 +400,10 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail } } - std::unique_ptr const& GetManagementClient() - { - std::unique_lock lock(m_managementClientMutex); - return m_managementClient; - } - std::mutex m_managementClientMutex; + std::mutex m_operationContextMutex; + std::weak_ptr m_operationContext; + bool m_isClosed{false}; Azure::Core::Amqp::_internal::Session m_session; std::unique_ptr m_managementClient; bool m_managementClientIsOpen{false}; diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp index 2b2250ebd7..ca07eb7283 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp @@ -13,6 +13,7 @@ #include #include +#include #include @@ -41,17 +42,25 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { class PropertiesEndpoint final : public Amqp::Tests::MessageTests::MockServiceEndpoint { public: - PropertiesEndpoint(bool failDiscovery = false) - : MockServiceEndpoint("$management", {}), m_failDiscovery{failDiscovery} + PropertiesEndpoint(bool failDiscovery = false, std::promise* requestReceived = nullptr) + : MockServiceEndpoint("$management", {}), m_failDiscovery{failDiscovery}, + m_requestReceived{requestReceived} { } private: bool m_failDiscovery; + std::promise* m_requestReceived; void MessageReceived( std::string const&, std::shared_ptr const& request) override { + if (m_requestReceived) + { + // Acknowledge receipt without replying. Close must interrupt the client's response wait. + m_requestReceived->set_value(); + return; + } Amqp::Models::AmqpMessage response; response.ApplicationProperties["status-code"] = m_failDiscovery ? 500 : 200; response.Properties.CorrelationId = request->Properties.MessageId; @@ -60,6 +69,13 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { body["created_at"] = Amqp::Models::AmqpTimestamp(std::chrono::milliseconds(0)).AsAmqpValue(); body["partition_ids"] = Amqp::Models::AmqpArray{"0", "1"}.AsAmqpValue(); + body["partition"] = "0"; + body["begin_sequence_number"] = std::int64_t{0}; + body["last_enqueued_sequence_number"] = std::int64_t{1}; + body["last_enqueued_offset"] = "1"; + body["last_enqueued_time_utc"] + = Amqp::Models::AmqpTimestamp(std::chrono::milliseconds(0)).AsAmqpValue(); + body["is_partition_empty"] = false; response.SetBody(body.AsAmqpValue()); auto result = GetMessageSender().Send(response, GetListenerContext()); EXPECT_EQ(std::get<0>(result), Amqp::_internal::MessageSendStatus::Ok); @@ -91,13 +107,119 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); } - int endCalls = 0; + std::atomic endCalls{0}; int CountEnd(SESSION_HANDLE session, char const* condition, char const* description) { ++endCalls; return session_end(session, condition, description); } + template void RequireReady(std::future& operation) + { + if (operation.wait_for(std::chrono::seconds(10)) != std::future_status::ready) + { + ADD_FAILURE() << "Concurrent properties operation did not finish"; + // Avoid hanging in a future destructor; the parent death test reports the failure. + std::exit(1); + } + } + + void ConcurrentPropertiesClose( + bool partitionProperties, + bool cancelCaller, + bool pauseBeforeSend = false) + { + { + std::promise requestReceived; + auto received = requestReceived.get_future(); + std::promise resumeQuery; + auto resume = resumeQuery.get_future(); + if (pauseBeforeSend) + { + Azure::Core::Diagnostics::Logger::SetLevel( + Azure::Core::Diagnostics::Logger::Level::Verbose); + Azure::Core::Diagnostics::Logger::SetListener( + [&](Azure::Core::Diagnostics::Logger::Level, std::string const& line) { + if (line.find("ManagementClient::ExecuteOperation:") != std::string::npos + && line.find("Create Queue for request.") != std::string::npos) + { + // Pause inside ExecuteOperation, before it sends, to expose destruction of an + // active management client even when ordinary close callbacks finish quickly. + requestReceived.set_value(); + RequireReady(resume); + } + }); + } + Amqp::Tests::MessageTests::AmqpServerMock server(AvailablePort()); + server.AddServiceEndpoint( + std::make_shared( + false, pauseBeforeSend ? nullptr : &requestReceived)); + server.StartListening(); + { + Amqp::_internal::ConnectionOptions options; + options.Port = server.GetPort(); + Amqp::_internal::Connection connection("localhost", nullptr, options); + auto session = Amqp::_detail::SessionFactory::CreateFromInternal( + std::make_shared( + Amqp::_detail::ConnectionFactory::GetImpl(connection), + Amqp::_internal::SessionOptions{}, + nullptr, + CountEnd)); + endCalls = 0; + EventHubsPropertiesClient properties(session, "hub"); + // No deadline: Close must not depend on the caller eventually timing out. + Azure::Core::Context queryContext; + auto query = std::async(std::launch::async, [&]() { + if (partitionProperties) + { + properties.GetEventHubsPartitionProperties("hub", "0", queryContext); + } + else + { + properties.GetEventHubsProperties("hub", queryContext); + } + }); + RequireReady(received); + received.get(); + if (cancelCaller) + { + queryContext.Cancel(); + } + std::promise closeStarted; + auto started = closeStarted.get_future(); + auto close = std::async(std::launch::async, [&]() { + closeStarted.set_value(); + properties.Close({}); + }); + RequireReady(started); + if (pauseBeforeSend) + { + EXPECT_EQ(close.wait_for(std::chrono::milliseconds(200)), std::future_status::timeout); + EXPECT_EQ(endCalls.load(), 0); + resumeQuery.set_value(); + } + RequireReady(query); + EXPECT_ANY_THROW(query.get()); + RequireReady(close); + EXPECT_NO_THROW(close.get()); + EXPECT_EQ(queryContext.IsCancelled(), cancelCaller); + properties.Close({}); + EXPECT_EQ(endCalls.load(), 1); + + // A stale properties-client reference must not reopen the session after invalidation. + EXPECT_THROW( + properties.GetEventHubsProperties("hub", {}), Core::OperationCancelledException); + EXPECT_THROW( + properties.GetEventHubsPartitionProperties("hub", "0", {}), + Core::OperationCancelledException); + EXPECT_EQ(endCalls.load(), 1); + Azure::Core::Diagnostics::Logger::SetListener(nullptr); + } + server.StopListening(); + } + Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); + } + void SuccessfulDiscovery(bool cancelledClose) { std::atomic closeFailureLogged{false}; @@ -131,6 +253,9 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { auto result = properties.GetEventHubsProperties("hub", context); EXPECT_EQ(result.Name, "hub"); EXPECT_EQ(result.PartitionIds, (std::vector{"0", "1"})); + auto partition = properties.GetEventHubsPartitionProperties("hub", "0", context); + EXPECT_EQ(partition.PartitionId, "0"); + EXPECT_EQ(partition.LastEnqueuedOffset, "1"); if (cancelledClose) { context.Cancel(); @@ -142,7 +267,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { properties.Close(context); properties.Close(context); } - EXPECT_EQ(endCalls, 1); + EXPECT_EQ(endCalls.load(), 1); } server.StopListening(); } @@ -236,6 +361,41 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { ""); } + class ConcurrentPropertiesCleanupDeathTest + : public PropertiesCleanupDeathTest, + public ::testing::WithParamInterface> {}; + + TEST_P(ConcurrentPropertiesCleanupDeathTest, CloseDuringPendingQuery) + { + auto const partitionProperties = std::get<0>(GetParam()); + auto const cancelCaller = std::get<1>(GetParam()); + EXPECT_EXIT( + { + ConcurrentPropertiesClose(partitionProperties, cancelCaller); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + + TEST_P(ConcurrentPropertiesCleanupDeathTest, CloseBeforeManagementSend) + { + auto const partitionProperties = std::get<0>(GetParam()); + auto const cancelCaller = std::get<1>(GetParam()); + EXPECT_EXIT( + { + ConcurrentPropertiesClose(partitionProperties, cancelCaller, true); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + + INSTANTIATE_TEST_SUITE_P( + PropertiesAndPartitions, + ConcurrentPropertiesCleanupDeathTest, + ::testing::Combine(::testing::Bool(), ::testing::Bool())); + TEST_F(PropertiesCleanupDeathTest, ManagementCloseFailureStillEndsSession) { EXPECT_EXIT( From 10de135acc1721583e2f9f8eb688fc93281e7f43 Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Tue, 27 Oct 2026 10:53:10 -0400 Subject: [PATCH 06/11] docs: describe Event Hubs cleanup regression tests --- doc/EventHubsCleanupTests.md | 85 +++++++++++++++++++ sdk/core/azure-core-amqp/README.md | 2 + .../azure-messaging-eventhubs/README.md | 1 + 3 files changed, 88 insertions(+) create mode 100644 doc/EventHubsCleanupTests.md diff --git a/doc/EventHubsCleanupTests.md b/doc/EventHubsCleanupTests.md new file mode 100644 index 0000000000..6d116e27ee --- /dev/null +++ b/doc/EventHubsCleanupTests.md @@ -0,0 +1,85 @@ +# Event Hubs cleanup regression tests + +These tests cover producer cleanup after partition discovery fails and cleanup while a properties query is running. They verify that management clients, sessions, and connection polling references are released safely, including when initialization or shutdown throws. + +The tests use the uAMQP backend and local loopback listeners. They do not require Azure credentials, a live broker, or the HTTP test proxy. See [CONTRIBUTING.md](../CONTRIBUTING.md) for compiler and dependency setup. + +## Build and run + +Run from the repository root, adding any toolchain or platform options required by your development environment: + +```sh +cmake -S . -B build/cleanup \ + -DCMAKE_BUILD_TYPE=Debug \ + -DBUILD_TESTING=ON \ + -DDISABLE_RUST_IN_BUILD=ON + +cmake --build build/cleanup --config Debug --target \ + azure-core-amqp-tests azure-messaging-eventhubs-test + +ctest --test-dir build/cleanup -C Debug -N \ + -R 'SessionCleanupDeathTest|PropertiesCleanupDeathTest' + +ctest --test-dir build/cleanup -C Debug \ + -R 'SessionCleanupDeathTest|PropertiesCleanupDeathTest' \ + --output-on-failure --timeout 60 +``` + +The discovery command should list **20 cases**: four session cases, eight properties cleanup cases, and eight parameterized concurrency cases. Both test files are guarded by `ENABLE_UAMQP`. The default Rust backend does not include them; a successful run with no matching tests does not validate this cleanup behavior. + +To repeat just the concurrency cases five times: + +```sh +ctest --test-dir build/cleanup -C Debug \ + -R 'ConcurrentPropertiesCleanupDeathTest' \ + --repeat until-fail:5 --output-on-failure --timeout 60 +``` + +## Session ownership and shutdown + +Source: [session_cleanup_tests.cpp](../sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp), fixture `SessionCleanupDeathTest`. + +| Test | Behavior checked | +| --- | --- | +| `FailedEndReleasesLastOwner` | A native shutdown failure still throws. A management client keeps the session alive until its last owner releases it; the session and connection then expire and polling becomes idle. | +| `FailedEndWithErrorReleasesLastOwner` | The same ownership guarantees hold for the `End` overload that accepts an AMQP error condition and description. | +| `FailedEndCanBeRetried` | A failed `End` can be retried successfully through the other overload. A further `End` fails rather than releasing polling ownership twice. | +| `MissingEndStillAsserts` | Destroying a begun session without ever calling `End` still terminates the child process. The existing lifecycle contract remains enforced. | + +## Properties initialization and producer cleanup + +Source: [properties_cleanup_test.cpp](../sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp), fixture `PropertiesCleanupDeathTest`. + +| Test | Behavior checked | +| --- | --- | +| `SuccessfulDiscoveryAndRepeatedClose` | Hub and partition properties are returned correctly, and repeated close ends the session exactly once. | +| `ManagementCloseFailureStillEndsSession` | A canceled management close logs its failure, but session cleanup still runs exactly once. | +| `CreationFailureThenRepeatedClose` | Management creation throws after session begin. Retrying creation preserves the original error and does not acquire another polling reference; repeated close releases ownership. | +| `CreationFailureThenDestruction` | Destruction performs the same partial-initialization cleanup without an explicit close. | +| `CreationFailureAndSessionEndFailure` | Management creation and native session shutdown both fail; final destruction still releases the owned resources. | +| `OpenFailureThenRepeatedClose` | Management open fails after assignment; repeated close safely cleans the partially initialized client. | +| `OpenFailureAndSessionEndFailure` | An open failure followed by a native session shutdown failure does not cause a destruction assertion. | +| `ProducerDiscoveryFailureThenRepeatedClose` | A mock management endpoint returns a service error. The public producer reports the discovery failure and survives repeated close and destruction. | + +## Concurrent queries and cleanup + +The same source contains `ConcurrentPropertiesCleanupDeathTest`, instantiated as `PropertiesAndPartitions`. Each test runs for both getters and with caller cancellation either absent or already requested: two tests times two getters times two cancellation states gives eight cases. + +| Test | Synchronization and expected outcome | +| --- | --- | +| `CloseDuringPendingQuery` | The mock endpoint signals receipt but withholds the response. The query has no deadline. Close must cancel its child operation context and finish without waiting indefinitely for the server. | +| `CloseBeforeManagementSend` | A synchronous log callback pauses execution inside `ExecuteOperation`, before sending. While paused, close must remain pending and the session-end counter must stay zero. After release, the query and close must finish safely. | + +Both tests verify that cleanup ends the session once, preserves the caller's cancellation state, rejects later queries through the closed properties-client reference, and leaves global polling idle. The parameter tuple is `(partitionProperties, cancelCaller)`: indices 0 through 3 correspond to `(false, false)`, `(false, true)`, `(true, false)`, and `(true, true)`. + +The second test depends on the management log messages containing `ManagementClient::ExecuteOperation:` and `Create Queue for request.`. If those diagnostics change, update the gate as well. The gate uses a bounded wait so a missing signal fails the child instead of hanging the suite. + +## Why these are process tests + +Except for `MissingEndStillAsserts`, the tests use `EXPECT_EXIT` and require a zero child exit status. An exception assertion alone would miss the original crash: a later destructor can abort after the exception has already been caught. The fixtures use Google Test's `threadsafe` death-test style to re-execute the child without inheriting polling-thread mutexes. + +The [cleanup listener](../sdk/core/azure-core-amqp/test/ut/cleanup_test_listener.hpp) accepts TCP connection attempts without creating a peer AMQP session. This isolates injected creation and session-end failures from mock-server teardown. Discovery and concurrency cases use the AMQP mock server because they need management links and requests. Weak ownership checks, an atomic session-end counter, and `GlobalStateHolder::AssertIdle()` check resource lifetime in addition to process survival. + +The concurrency gate reproduced process termination and an AddressSanitizer heap-use-after-free before synchronization was added. For Google Test 1.12 on Linux ARM64 under AddressSanitizer, set `GTEST_DEATH_TEST_USE_FORK=1` to avoid the small clone-child stack overflowing before the SDK code runs. + +These local tests establish SDK cleanup behavior. They do not replace Windows teardown validation, live service tests, or the AUB discovery-failure recovery workload. The Blob Checkpoint Store recordings refreshed alongside this fix exercise separate, existing HTTP playback tests. diff --git a/sdk/core/azure-core-amqp/README.md b/sdk/core/azure-core-amqp/README.md index 17c138df32..95ea89792d 100644 --- a/sdk/core/azure-core-amqp/README.md +++ b/sdk/core/azure-core-amqp/README.md @@ -77,6 +77,8 @@ while (messageSendCount < maxMessageSendCount) You can build and run the tests locally by executing `azure-core-amqp-test`. Explore the `test` folder to see advanced usage and behavior of the public classes. +For session shutdown failures and Event Hubs properties cleanup, see the [cleanup regression test guide](../../../doc/EventHubsCleanupTests.md). It describes the ownership checks and how to enable and run the uAMQP process tests. + ### Run broker-backed tests Some tests in this package need a live AMQP broker. They use the `TestAmqpBroker` from [Azure/azure-amqp][azure_amqp]. The default build sets `USE_RUST_AMQP`, so these tests exercise the Rust AMQP stack. The setup script clones the commit that `Test-Setup.ps1` pins, restores through the `nuget.cfsclean.config` in this directory, builds the `net10.0` target, and waits for the broker to accept TCP connections. diff --git a/sdk/eventhubs/azure-messaging-eventhubs/README.md b/sdk/eventhubs/azure-messaging-eventhubs/README.md index f33bcccac2..5cd8428219 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/README.md +++ b/sdk/eventhubs/azure-messaging-eventhubs/README.md @@ -275,6 +275,7 @@ Many people all over the world have helped make this project better. You'll wan * [What are some good first issues for new contributors to the repo?](https://github.com/azure/azure-sdk-for-cpp/issues?q=is%3Aopen+is%3Aissue+label%3A%22up+for+grabs%22) * [How to build and test your change][azure_sdk_for_cpp_contributing_developer_guide] +* [Event Hubs cleanup regression tests](../../../doc/EventHubsCleanupTests.md) * [How you can make a change happen!][azure_sdk_for_cpp_contributing_pull_requests] * Frequently Asked Questions (FAQ) and Conceptual Topics in the detailed [Azure SDK for C++ wiki](https://github.com/azure/azure-sdk-for-cpp/wiki). From 61994e8bf15a48086ca8bd77c28ac9f989129a64 Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Tue, 27 Oct 2026 11:48:27 -0400 Subject: [PATCH 07/11] docs: fix cleanup guide links and test timeout --- doc/EventHubsCleanupTests.md | 11 +++++++---- sdk/core/azure-core-amqp/README.md | 2 +- sdk/eventhubs/azure-messaging-eventhubs/README.md | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/doc/EventHubsCleanupTests.md b/doc/EventHubsCleanupTests.md index 6d116e27ee..8ab81f5088 100644 --- a/doc/EventHubsCleanupTests.md +++ b/doc/EventHubsCleanupTests.md @@ -2,7 +2,7 @@ These tests cover producer cleanup after partition discovery fails and cleanup while a properties query is running. They verify that management clients, sessions, and connection polling references are released safely, including when initialization or shutdown throws. -The tests use the uAMQP backend and local loopback listeners. They do not require Azure credentials, a live broker, or the HTTP test proxy. See [CONTRIBUTING.md](../CONTRIBUTING.md) for compiler and dependency setup. +The tests use the uAMQP backend and local loopback listeners. They do not require Azure credentials, a live broker, or the HTTP test proxy. See [CONTRIBUTING.md](https://github.com/Azure/azure-sdk-for-cpp/blob/main/CONTRIBUTING.md) for compiler and dependency setup. ## Build and run @@ -12,6 +12,7 @@ Run from the repository root, adding any toolchain or platform options required cmake -S . -B build/cleanup \ -DCMAKE_BUILD_TYPE=Debug \ -DBUILD_TESTING=ON \ + -D AZURE_EVENTHUBS_CTEST_TIMEOUT=60 \ -DDISABLE_RUST_IN_BUILD=ON cmake --build build/cleanup --config Debug --target \ @@ -25,6 +26,8 @@ ctest --test-dir build/cleanup -C Debug \ --output-on-failure --timeout 60 ``` +The configure option sets the Event Hubs test timeout to 60 seconds; CTest's `--timeout` alone does not override an explicit per-test timeout. + The discovery command should list **20 cases**: four session cases, eight properties cleanup cases, and eight parameterized concurrency cases. Both test files are guarded by `ENABLE_UAMQP`. The default Rust backend does not include them; a successful run with no matching tests does not validate this cleanup behavior. To repeat just the concurrency cases five times: @@ -37,7 +40,7 @@ ctest --test-dir build/cleanup -C Debug \ ## Session ownership and shutdown -Source: [session_cleanup_tests.cpp](../sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp), fixture `SessionCleanupDeathTest`. +Source: [session_cleanup_tests.cpp](https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp), fixture `SessionCleanupDeathTest`. | Test | Behavior checked | | --- | --- | @@ -48,7 +51,7 @@ Source: [session_cleanup_tests.cpp](../sdk/core/azure-core-amqp/test/ut/session_ ## Properties initialization and producer cleanup -Source: [properties_cleanup_test.cpp](../sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp), fixture `PropertiesCleanupDeathTest`. +Source: [properties_cleanup_test.cpp](https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp), fixture `PropertiesCleanupDeathTest`. | Test | Behavior checked | | --- | --- | @@ -78,7 +81,7 @@ The second test depends on the management log messages containing `ManagementCli Except for `MissingEndStillAsserts`, the tests use `EXPECT_EXIT` and require a zero child exit status. An exception assertion alone would miss the original crash: a later destructor can abort after the exception has already been caught. The fixtures use Google Test's `threadsafe` death-test style to re-execute the child without inheriting polling-thread mutexes. -The [cleanup listener](../sdk/core/azure-core-amqp/test/ut/cleanup_test_listener.hpp) accepts TCP connection attempts without creating a peer AMQP session. This isolates injected creation and session-end failures from mock-server teardown. Discovery and concurrency cases use the AMQP mock server because they need management links and requests. Weak ownership checks, an atomic session-end counter, and `GlobalStateHolder::AssertIdle()` check resource lifetime in addition to process survival. +The [cleanup listener](https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/core/azure-core-amqp/test/ut/cleanup_test_listener.hpp) accepts TCP connection attempts without creating a peer AMQP session. This isolates injected creation and session-end failures from mock-server teardown. Discovery and concurrency cases use the AMQP mock server because they need management links and requests. Weak ownership checks, an atomic session-end counter, and `GlobalStateHolder::AssertIdle()` check resource lifetime in addition to process survival. The concurrency gate reproduced process termination and an AddressSanitizer heap-use-after-free before synchronization was added. For Google Test 1.12 on Linux ARM64 under AddressSanitizer, set `GTEST_DEATH_TEST_USE_FORK=1` to avoid the small clone-child stack overflowing before the SDK code runs. diff --git a/sdk/core/azure-core-amqp/README.md b/sdk/core/azure-core-amqp/README.md index 95ea89792d..4affddcde4 100644 --- a/sdk/core/azure-core-amqp/README.md +++ b/sdk/core/azure-core-amqp/README.md @@ -77,7 +77,7 @@ while (messageSendCount < maxMessageSendCount) You can build and run the tests locally by executing `azure-core-amqp-test`. Explore the `test` folder to see advanced usage and behavior of the public classes. -For session shutdown failures and Event Hubs properties cleanup, see the [cleanup regression test guide](../../../doc/EventHubsCleanupTests.md). It describes the ownership checks and how to enable and run the uAMQP process tests. +For session shutdown failures and Event Hubs properties cleanup, see the [cleanup regression test guide](https://github.com/Azure/azure-sdk-for-cpp/blob/main/doc/EventHubsCleanupTests.md). It describes the ownership checks and how to enable and run the uAMQP process tests. ### Run broker-backed tests diff --git a/sdk/eventhubs/azure-messaging-eventhubs/README.md b/sdk/eventhubs/azure-messaging-eventhubs/README.md index 5cd8428219..b72714e8ff 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/README.md +++ b/sdk/eventhubs/azure-messaging-eventhubs/README.md @@ -275,7 +275,7 @@ Many people all over the world have helped make this project better. You'll wan * [What are some good first issues for new contributors to the repo?](https://github.com/azure/azure-sdk-for-cpp/issues?q=is%3Aopen+is%3Aissue+label%3A%22up+for+grabs%22) * [How to build and test your change][azure_sdk_for_cpp_contributing_developer_guide] -* [Event Hubs cleanup regression tests](../../../doc/EventHubsCleanupTests.md) +* [Event Hubs cleanup regression tests](https://github.com/Azure/azure-sdk-for-cpp/blob/main/doc/EventHubsCleanupTests.md) * [How you can make a change happen!][azure_sdk_for_cpp_contributing_pull_requests] * Frequently Asked Questions (FAQ) and Conceptual Topics in the detailed [Azure SDK for C++ wiki](https://github.com/azure/azure-sdk-for-cpp/wiki). From c86152e01181b8f3df7f3f94b79a02f15e189124 Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Wed, 28 Oct 2026 09:07:51 -0400 Subject: [PATCH 08/11] fix(eventhubs): cancel pending Rust management queries during cleanup Bridge the operation context into the synchronous Rust wait so cancellation releases the properties mutex before close. Reject null results and replace failed management links before another query. Bound consuming management teardown and add real loopback regressions for both getters and public consumer close. --- Cargo.lock | 9 + Cargo.toml | 1 + doc/EventHubsCleanupTests.md | 36 ++- sdk/core/azure-core-amqp/CHANGELOG.md | 2 + sdk/core/azure-core-amqp/CMakeLists.txt | 2 +- .../amqp/internal/common/runtime_context.hpp | 14 + .../src/impl/rust_amqp/amqp/management.cpp | 20 ++ .../amqp/private/management_impl.hpp | 1 + .../rust_wrapper/src/amqp/management.rs | 21 +- .../rust_wrapper/src/call_context.rs | 187 ++++++++++- .../azure-core-amqp/test/rust-peer/Cargo.toml | 14 + .../azure-core-amqp/test/rust-peer/src/lib.rs | 253 +++++++++++++++ .../test/ut/rust_cleanup_peer.hpp | 68 ++++ .../azure-messaging-eventhubs/CHANGELOG.md | 2 + .../src/private/eventhubs_utilities.hpp | 65 +++- .../test/ut/CMakeLists.txt | 8 + .../test/ut/properties_cleanup_test.cpp | 54 +++- .../test/ut/rust_properties_cleanup_test.cpp | 302 ++++++++++++++++++ 18 files changed, 1031 insertions(+), 28 deletions(-) create mode 100644 sdk/core/azure-core-amqp/test/rust-peer/Cargo.toml create mode 100644 sdk/core/azure-core-amqp/test/rust-peer/src/lib.rs create mode 100644 sdk/core/azure-core-amqp/test/ut/rust_cleanup_peer.hpp create mode 100644 sdk/eventhubs/azure-messaging-eventhubs/test/ut/rust_properties_cleanup_test.cpp diff --git a/Cargo.lock b/Cargo.lock index 27f79a3ecf..17d48f8e84 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -26,6 +26,15 @@ dependencies = [ "memchr", ] +[[package]] +name = "amqp_cleanup_peer" +version = "0.1.0" +dependencies = [ + "fe2o3-amqp", + "fe2o3-amqp-types", + "tokio", +] + [[package]] name = "android-tzdata" version = "0.1.1" diff --git a/Cargo.toml b/Cargo.toml index 3a230172b5..6163465e1d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,7 @@ [workspace] resolver = "2" members = [ + "sdk/core/azure-core-amqp/test/rust-peer", "sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/azure_core_amqp", "sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper", ] diff --git a/doc/EventHubsCleanupTests.md b/doc/EventHubsCleanupTests.md index 8ab81f5088..57760e66fc 100644 --- a/doc/EventHubsCleanupTests.md +++ b/doc/EventHubsCleanupTests.md @@ -2,9 +2,9 @@ These tests cover producer cleanup after partition discovery fails and cleanup while a properties query is running. They verify that management clients, sessions, and connection polling references are released safely, including when initialization or shutdown throws. -The tests use the uAMQP backend and local loopback listeners. They do not require Azure credentials, a live broker, or the HTTP test proxy. See [CONTRIBUTING.md](https://github.com/Azure/azure-sdk-for-cpp/blob/main/CONTRIBUTING.md) for compiler and dependency setup. +The tests use local loopback listeners with the uAMQP and Rust backends. They do not require Azure credentials, a live broker, or the HTTP test proxy. See [CONTRIBUTING.md](https://github.com/Azure/azure-sdk-for-cpp/blob/main/CONTRIBUTING.md) for compiler and dependency setup. -## Build and run +## Build and run with uAMQP Run from the repository root, adding any toolchain or platform options required by your development environment: @@ -28,7 +28,7 @@ ctest --test-dir build/cleanup -C Debug \ The configure option sets the Event Hubs test timeout to 60 seconds; CTest's `--timeout` alone does not override an explicit per-test timeout. -The discovery command should list **20 cases**: four session cases, eight properties cleanup cases, and eight parameterized concurrency cases. Both test files are guarded by `ENABLE_UAMQP`. The default Rust backend does not include them; a successful run with no matching tests does not validate this cleanup behavior. +The discovery command should list **21 cases**: four session cases, nine properties cleanup cases, and eight parameterized concurrency cases. These two test files are guarded by `ENABLE_UAMQP`. The Rust backend uses the separate cases described below; a successful run with no matching tests does not validate this cleanup behavior. To repeat just the concurrency cases five times: @@ -38,6 +38,35 @@ ctest --test-dir build/cleanup -C Debug \ --repeat until-fail:5 --output-on-failure --timeout 60 ``` +## Build and run with Rust + +```sh +cmake -S . -B build/cleanup-rust \ + -DCMAKE_BUILD_TYPE=Debug \ + -DBUILD_TESTING=ON \ + -DAZURE_EVENTHUBS_CTEST_TIMEOUT=60 \ + -DDISABLE_RUST_IN_BUILD=OFF +cmake --build build/cleanup-rust --config Debug --target azure-messaging-eventhubs-test +ctest --test-dir build/cleanup-rust -C Debug \ + -R 'Rust.*PropertiesCleanupDeathTest' --output-on-failure --timeout 60 +cargo test -p rust_wrapper call_context -- --test-threads=1 +``` + +This selects **13 C++ cases** and **six Rust context cases**. The C++ tests link a test-only Rust AMQP peer built from the existing fe2o3 dependency. The peer acknowledges receipt of management requests and withholds their responses, so cancellation occurs after a real request reaches the server. A 15-second child-process watchdog reports a blocked query or close as a failure. + +Source: [rust_properties_cleanup_test.cpp](https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/eventhubs/azure-messaging-eventhubs/test/ut/rust_properties_cleanup_test.cpp). + +| Cases | Behavior checked | +| --- | --- | +| `CloseDuringPendingQuery` (four) | Both getters release their operation lock when close cancels the child context; caller cancellation state is preserved, repeated close succeeds, and stale references reject queries. | +| `ConsumerCloseDuringPendingQuery` (four) | Public consumer close completes with either getter pending, including when the close deadline has already expired. The original query context has no deadline and remains uncancelled. | +| `CancelledPropertiesThenRediscover`, `CancelledPartitionThenRediscover` | Caller cancellation interrupts the query; subsequent public producer queries succeed with fresh links. | +| `QueryDeadlineThenRediscover` | A query deadline interrupts the wait; a later query succeeds. | +| `ServiceErrorIsExceptionThenRediscover` | A real service error produces an exception instead of converting a null Rust value; a later query succeeds. | +| `DirectManagementFailureRequiresReopen` | A failed management client rejects reuse until close and reopen. | + +The Rust context tests cover cancellation before and during a wait, deadlines, successful results, dropping the pending future exactly once, and teardown that runs despite cancellation. These changes cover management calls after initialization; they do not establish cancellation of pending session begin, authentication, or management attach, or forced termination of dependency background tasks. + ## Session ownership and shutdown Source: [session_cleanup_tests.cpp](https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp), fixture `SessionCleanupDeathTest`. @@ -55,6 +84,7 @@ Source: [properties_cleanup_test.cpp](https://github.com/Azure/azure-sdk-for-cpp | Test | Behavior checked | | --- | --- | +| `CancelledQueryThenRediscover` | Caller cancellation interrupts a query, and both getters succeed on subsequent queries with fresh management links. | | `SuccessfulDiscoveryAndRepeatedClose` | Hub and partition properties are returned correctly, and repeated close ends the session exactly once. | | `ManagementCloseFailureStillEndsSession` | A canceled management close logs its failure, but session cleanup still runs exactly once. | | `CreationFailureThenRepeatedClose` | Management creation throws after session begin. Retrying creation preserves the original error and does not acquire another polling reference; repeated close releases ownership. | diff --git a/sdk/core/azure-core-amqp/CHANGELOG.md b/sdk/core/azure-core-amqp/CHANGELOG.md index 259e3233a1..475506c9a5 100644 --- a/sdk/core/azure-core-amqp/CHANGELOG.md +++ b/sdk/core/azure-core-amqp/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +- Fixed Rust management calls ignoring cancellation and deadlines. Failed calls now throw safely, require fresh management links before reuse, and allow bounded management teardown. + - Fixed uAMQP session destruction after a failed `End`. Native resources and the connection polling reference are released when the last session owner is destroyed; callers must still end sessions before destruction. - Fixed uAMQP message annotation encoding so messages with annotations, including Event Hubs partition keys, include the required AMQP section descriptor. [[#7426]](https://github.com/Azure/azure-sdk-for-cpp/issues/7426) diff --git a/sdk/core/azure-core-amqp/CMakeLists.txt b/sdk/core/azure-core-amqp/CMakeLists.txt index 17fa8239ae..96cf2e978e 100644 --- a/sdk/core/azure-core-amqp/CMakeLists.txt +++ b/sdk/core/azure-core-amqp/CMakeLists.txt @@ -137,7 +137,7 @@ endif() # Import targets defined in a package or workspace manifest `Cargo.toml` file - corrosion_import_crate(MANIFEST_PATH ${CMAKE_CURRENT_SOURCE_DIR}/src/impl/rust_amqp/rust_amqp/rust_wrapper/Cargo.toml) + corrosion_import_crate(MANIFEST_PATH ${CMAKE_CURRENT_SOURCE_DIR}/src/impl/rust_amqp/rust_amqp/rust_wrapper/Cargo.toml CRATES rust_wrapper) # Set the build environment variables for the rust_wrapper crate to use our targetted OpenSSL version. if(NOT APPLE) diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/runtime_context.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/runtime_context.hpp index 19fef5817d..e811bfa93f 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/runtime_context.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/runtime_context.hpp @@ -125,6 +125,15 @@ namespace Azure { namespace Core { namespace Amqp { namespace Common { namespace GetCallContext(), timeoutMilliseconds); } + /** Observe cancellation only during a synchronous management operation. + * This object and its stored Context must remain stationary until FFI returns. + */ + void SetOperationCancellation() + { + Azure::Core::Amqp::RustInterop::_detail::call_context_set_cancellation( + GetCallContext(), IsOperationCancelled, &m_context); + } + std::uint64_t GetTeardownTimeoutMilliseconds() const { auto const remaining = GetTimeoutMilliseconds(); @@ -165,6 +174,11 @@ namespace Azure { namespace Core { namespace Amqp { namespace Common { namespace } private: + static bool IsOperationCancelled(void const* state) noexcept + { + return static_cast(state)->IsCancelled(); + } + UniqueRustCallContext m_callContext; Azure::Core::Context m_context; }; diff --git a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/management.cpp b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/management.cpp index 388d6fc543..75c90dcc87 100644 --- a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/management.cpp @@ -82,6 +82,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { throw std::runtime_error("Could not attach management object: " + callContext.GetError()); } m_isOpen = true; + m_operationFailed = false; return _internal::ManagementOpenStatus::Ok; } catch (...) @@ -104,9 +105,17 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { << "Execute Operation called when management is not initialized."; throw std::runtime_error("Management is not open!"); } + if (m_operationFailed) + { + throw std::runtime_error( + "Management operation failed; close and reopen the client before reuse."); + } Common::_detail::CallContext callContext( Common::_detail::GlobalStateHolder::GlobalStateInstance()->GetRuntimeContext(), context); + callContext.SetTimeoutMilliseconds(callContext.GetTimeoutMilliseconds()); + callContext.SetOperationCancellation(); + Azure::Core::Amqp::Models::AmqpMap propertiesMap; for (const auto& val : messageToSend.ApplicationProperties) { @@ -121,6 +130,15 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { typeOfOperation.c_str(), Models::_detail::AmqpValueFactory::ToImplementation(applicationProperties))}; + if (!value) + { + // Cancellation can leave an unmatched response in the management links. + // Keep the handle open for consuming Close, but prohibit another query. + m_operationFailed = true; + context.ThrowIfCancelled(); + throw std::runtime_error("Management operation failed: " + callContext.GetError()); + } + auto responseMessage = std::make_shared(); responseMessage->SetBody(Models::_detail::AmqpValueFactory::FromImplementation(value)); _internal::ManagementOperationResult result; @@ -140,6 +158,8 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Common::_detail::CallContext callContext( Common::_detail::GlobalStateHolder::GlobalStateInstance()->GetRuntimeContext(), context); + callContext.SetTeardownTimeoutMilliseconds(); + // Even if the detach fails, the management client is closed. A client that failed to detach is // not usable again, and a client that keeps the open flag stops the process in its destructor. m_isOpen = false; diff --git a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/management_impl.hpp b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/management_impl.hpp index 5ed8334bf3..e6bac84920 100644 --- a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/management_impl.hpp +++ b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/management_impl.hpp @@ -72,6 +72,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { private: bool m_isOpen{false}; + bool m_operationFailed{false}; UniqueAmqpManagement m_management; _internal::ManagementClientOptions m_options; std::shared_ptr<_detail::SessionImpl> m_session; diff --git a/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/amqp/management.rs b/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/amqp/management.rs index efb4d445d5..a9a86e0c2e 100644 --- a/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/amqp/management.rs +++ b/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/amqp/management.rs @@ -134,10 +134,14 @@ pub unsafe extern "C" fn amqpmanagement_detach_and_release( let management = Box::from_raw(management); let call_context = call_context_from_ptr_mut(call_context); trace!("Starting to detach management"); - let result = call_context - .runtime_context() - .runtime() - .block_on(management.detach()); + let result = call_context.block_on_with_timeout(management.detach()); + let result = match result { + Ok(result) => result, + Err(err) => { + call_context.set_error(Box::new(err)); + return -1; + } + }; trace!("Detached management"); if let Err(err) = result { error!("Failed to detach management: {:?}", err); @@ -176,11 +180,18 @@ pub unsafe extern "C" fn amqpmanagement_call( }) .collect(); trace!("Converted application properties."); - let result = call_context.runtime_context().runtime().block_on( + let result = call_context.block_on_with_cancellation( management .inner .call(operation_type.to_string(), application_properties), ); + let result = match result { + Ok(result) => result, + Err(err) => { + call_context.set_error(Box::new(err)); + return ptr::null_mut(); + } + }; if let Err(err) = result { error!("Failed to call management: {:?}", err); call_context.set_error(Box::new(err)); diff --git a/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/call_context.rs b/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/call_context.rs index 7f149c41c3..bd9ed61dac 100644 --- a/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/call_context.rs +++ b/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/call_context.rs @@ -4,7 +4,7 @@ // cspell: words reqwest repr tokio use crate::runtime_context::RuntimeContext; -use std::ffi::c_char; +use std::ffi::{c_char, c_void}; use std::future::Future; use std::time::Duration; @@ -13,6 +13,8 @@ pub struct RustCallContext { error: Option, /// The bound for one call. `Duration::MAX` means the call has no bound. timeout: Duration, + cancellation_probe: Option bool>, + cancellation_state: *const c_void, } impl RustCallContext { @@ -21,6 +23,8 @@ impl RustCallContext { runtime_context, error: None, timeout: Duration::MAX, + cancellation_probe: None, + cancellation_state: std::ptr::null(), } } @@ -42,6 +46,50 @@ impl RustCallContext { } } + fn is_cancelled(&self) -> bool { + self.cancellation_probe + .map(|probe| unsafe { probe(self.cancellation_state) }) + .unwrap_or(false) + } + + /// The callback and its state are borrowed only for this synchronous call. + /// This future is never spawned onto the runtime's worker threads. + pub(crate) fn block_on_with_cancellation( + &self, + future: F, + ) -> Result { + if self.is_cancelled() { + return Err(OperationCancelledOrTimedOut::Cancelled); + } + if self.timeout() == Some(Duration::ZERO) { + return Err(OperationCancelledOrTimedOut::TimedOut); + } + let runtime = unsafe { self.runtime_context() }.runtime(); + runtime.block_on(async { + let cancellation = async { + loop { + if self.is_cancelled() { + return; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }; + let operation = async { + match self.timeout() { + Some(duration) => tokio::time::timeout(duration, future) + .await + .map_err(|_| OperationCancelledOrTimedOut::TimedOut), + None => Ok(future.await), + } + }; + tokio::select! { + biased; + _ = cancellation => Err(OperationCancelledOrTimedOut::Cancelled), + result = operation => result, + } + }) + } + /// Run the future on the runtime, and end it when the bound expires. /// /// A future that never completes, for example a send on a connection that @@ -63,6 +111,37 @@ impl RustCallContext { } } +#[derive(Debug, PartialEq, Eq)] +pub(crate) enum OperationCancelledOrTimedOut { + Cancelled, + TimedOut, +} + +impl std::fmt::Display for OperationCancelledOrTimedOut { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + Self::Cancelled => "AMQP operation cancelled", + Self::TimedOut => "AMQP operation timed out", + }) + } +} +impl std::error::Error for OperationCancelledOrTimedOut {} + +/// Register an optional cancellation probe for an operation. The state must +/// remain valid and stationary until the synchronous operation returns. +/// +/// # Safety +/// The caller owns the state; the callback must not throw across the C ABI. +#[no_mangle] +pub unsafe extern "C" fn call_context_set_cancellation( + ctx: *mut RustCallContext, + probe: Option bool>, + state: *const c_void, +) { + (*ctx).cancellation_probe = probe; + (*ctx).cancellation_state = state; +} + #[no_mangle] pub extern "C" fn call_context_new(runtime_context: *mut RuntimeContext) -> *mut RustCallContext { Box::into_raw(Box::new(RustCallContext::new(runtime_context))) @@ -152,3 +231,109 @@ fn test_call_context_block_on_with_timeout() { drop(Box::from_raw(runtime_context)); } } + +#[cfg(test)] +mod cancellation_tests { + use super::*; + use std::pin::Pin; + use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; + use std::sync::Arc; + use std::task::{Context, Poll}; + + struct PendingOperation { + polls: Arc, + drops: Arc, + } + impl Future for PendingOperation { + type Output = (); + fn poll(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<()> { + self.polls.fetch_add(1, Ordering::SeqCst); + Poll::Pending + } + } + impl Drop for PendingOperation { + fn drop(&mut self) { + self.drops.fetch_add(1, Ordering::SeqCst); + } + } + unsafe extern "C" fn probe(state: *const c_void) -> bool { + (*(state as *const AtomicBool)).load(Ordering::SeqCst) + } + #[test] + fn call_context_pre_cancel_and_expired_deadline_never_poll() { + let mut runtime = RuntimeContext::new().unwrap(); + let mut context = RustCallContext::new(&mut runtime); + let state = AtomicBool::new(true); + unsafe { + call_context_set_cancellation(&mut context, Some(probe), &state as *const _ as _); + } + for expected in [ + OperationCancelledOrTimedOut::Cancelled, + OperationCancelledOrTimedOut::TimedOut, + ] { + let polls = Arc::new(AtomicUsize::new(0)); + let drops = Arc::new(AtomicUsize::new(0)); + assert_eq!( + context.block_on_with_cancellation(PendingOperation { + polls: polls.clone(), + drops: drops.clone() + }), + Err(expected) + ); + assert_eq!(polls.load(Ordering::SeqCst), 0); + assert_eq!(drops.load(Ordering::SeqCst), 1); + state.store(false, Ordering::SeqCst); + context.timeout = Duration::ZERO; + } + } + #[test] + fn call_context_cancellation_drops_pending_future_once() { + let mut runtime = RuntimeContext::new().unwrap(); + let mut context = RustCallContext::new(&mut runtime); + let state = Arc::new(AtomicBool::new(false)); + let polls = Arc::new(AtomicUsize::new(0)); + let drops = Arc::new(AtomicUsize::new(0)); + unsafe { + call_context_set_cancellation(&mut context, Some(probe), Arc::as_ptr(&state) as _); + } + let signal = state.clone(); + let observed = polls.clone(); + let cancel = std::thread::spawn(move || { + let start = std::time::Instant::now(); + while observed.load(Ordering::SeqCst) == 0 { + assert!(start.elapsed() < Duration::from_secs(5)); + std::thread::yield_now(); + } + signal.store(true, Ordering::SeqCst); + }); + context.timeout = Duration::from_secs(5); + assert_eq!( + context.block_on_with_cancellation(PendingOperation { + polls, + drops: drops.clone() + }), + Err(OperationCancelledOrTimedOut::Cancelled) + ); + cancel.join().unwrap(); + assert_eq!(drops.load(Ordering::SeqCst), 1); + } + #[test] + fn call_context_success_deadline_and_teardown() { + let mut runtime = RuntimeContext::new().unwrap(); + let mut context = RustCallContext::new(&mut runtime); + context.timeout = Duration::from_millis(20); + assert_eq!(context.block_on_with_cancellation(async { 7 }), Ok(7)); + assert_eq!( + context.block_on_with_cancellation(std::future::pending::<()>()), + Err(OperationCancelledOrTimedOut::TimedOut) + ); + let state = AtomicBool::new(true); + unsafe { + call_context_set_cancellation(&mut context, Some(probe), &state as *const _ as _); + } + assert_eq!(context.block_on_with_timeout(async { 9 }).unwrap(), 9); + assert!(context + .block_on_with_timeout(std::future::pending::<()>()) + .is_err()); + } +} diff --git a/sdk/core/azure-core-amqp/test/rust-peer/Cargo.toml b/sdk/core/azure-core-amqp/test/rust-peer/Cargo.toml new file mode 100644 index 0000000000..63243c2f0e --- /dev/null +++ b/sdk/core/azure-core-amqp/test/rust-peer/Cargo.toml @@ -0,0 +1,14 @@ +# Copyright (c) Microsoft Corporation. Licensed under the MIT License. +[package] +name = "amqp_cleanup_peer" +version = "0.1.0" +edition.workspace = true +publish = false + +[lib] +crate-type = ["staticlib"] + +[dependencies] +fe2o3-amqp = { workspace = true, features = ["acceptor"] } +fe2o3-amqp-types.workspace = true +tokio = { workspace = true, features = ["rt-multi-thread", "net", "io-util", "sync"] } diff --git a/sdk/core/azure-core-amqp/test/rust-peer/src/lib.rs b/sdk/core/azure-core-amqp/test/rust-peer/src/lib.rs new file mode 100644 index 0000000000..22acd1d5cb --- /dev/null +++ b/sdk/core/azure-core-amqp/test/rust-peer/src/lib.rs @@ -0,0 +1,253 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. +// Test-only AMQP peer. No production target links this crate. +// cspell: words amqp tokio fe2o3 +use fe2o3_amqp::{ + acceptor::{ConnectionAcceptor, LinkAcceptor, LinkEndpoint, SessionAcceptor}, + types::messaging::{ApplicationProperties, Body, Message, Properties}, +}; +use fe2o3_amqp_types::primitives::{Array, OrderedMap, Timestamp, Value}; +use std::{ + sync::{ + atomic::{AtomicBool, Ordering}, + Arc, Condvar, Mutex, + }, + time::Duration, +}; + +pub const HOLD_QUERY: u32 = 1; +pub const SERVICE_ERROR: u32 = 2; +pub const REQUEST: u32 = 1; + +struct State { + mode: u32, + observed: Mutex, + changed: Condvar, + resumed: AtomicBool, +} +impl State { + fn signal(&self, event: u32) { + *self.observed.lock().unwrap() |= event; + self.changed.notify_all(); + } +} +pub struct CleanupPeer { + // Runtime destruction joins the helper tasks, including deliberately stalled + // peers. This is fixture cleanup, not proof of SDK engine termination. + _runtime: tokio::runtime::Runtime, + state: Arc, + port: u16, +} +impl CleanupPeer { + pub fn start(mode: u32) -> Self { + let runtime = tokio::runtime::Builder::new_multi_thread() + .worker_threads(2) + .enable_all() + .build() + .unwrap(); + let state = Arc::new(State { + mode, + observed: Mutex::new(0), + changed: Condvar::new(), + resumed: AtomicBool::new(false), + }); + let listener = runtime + .block_on(tokio::net::TcpListener::bind("127.0.0.1:0")) + .unwrap(); + let port = listener.local_addr().unwrap().port(); + let shared = state.clone(); + runtime.spawn(async move { + while let Ok((tcp, _)) = listener.accept().await { + let state = shared.clone(); + tokio::spawn(async move { + let Ok(mut connection) = ConnectionAcceptor::builder() + .container_id("cleanup-peer") + .sasl_acceptor(fe2o3_amqp::acceptor::SaslAnonymousMechanism::new()) + .build() + .accept(tcp) + .await + else { + return; + }; + while let Some(incoming) = connection.next_incoming_session().await { + let Ok(session) = SessionAcceptor::new() + .accept_incoming_session(incoming, &mut connection) + .await + else { + break; + }; + tokio::spawn(serve_session(session, state.clone())); + } + }); + } + }); + Self { + _runtime: runtime, + state, + port, + } + } + pub fn port(&self) -> u16 { + self.port + } + pub fn wait(&self, event: u32, timeout: Duration) -> bool { + let observed = self.state.observed.lock().unwrap(); + let (observed, _) = self + .state + .changed + .wait_timeout_while(observed, timeout, |flags| *flags & event == 0) + .unwrap(); + *observed & event != 0 + } + pub fn resume(&self) { + self.state.resumed.store(true, Ordering::SeqCst); + } +} + +async fn serve_session( + mut session: fe2o3_amqp::acceptor::ListenerSessionHandle, + state: Arc, +) { + let acceptor = LinkAcceptor::new(); + let mut pending_replies = None; + while let Some(attach) = session.next_incoming_attach().await { + let Ok(link) = acceptor.accept_incoming_attach(attach, &mut session).await else { + continue; + }; + match link { + LinkEndpoint::Receiver(mut receiver) => { + let (responses, replies) = tokio::sync::mpsc::channel::>>(16); + pending_replies = Some(replies); + let state = state.clone(); + tokio::spawn(async move { + while let Ok(delivery) = receiver.recv::>().await { + let target = receiver + .target() + .as_ref() + .and_then(|t| t.address.as_ref()) + .map(|s| s.as_str()) + .unwrap_or(""); + let is_management = target == "$management"; + let is_cbs = target == "$cbs"; + let _ = receiver.accept(&delivery).await; + if !is_management && !is_cbs { + continue; + } + if is_management { + state.signal(REQUEST); + } + if is_management + && state.mode & HOLD_QUERY != 0 + && !state.resumed.load(Ordering::SeqCst) + { + continue; + } + let correlation_id = delivery + .message() + .properties + .as_ref() + .and_then(|p| p.message_id.clone()); + let mut body = OrderedMap::::new(); + body.insert("name".into(), "hub".into()); + body.insert("created_at".into(), Value::Timestamp(Timestamp::from(0))); + body.insert( + "partition_ids".into(), + Value::Array(Array::from(vec![ + Value::String("0".to_string()), + Value::String("1".to_string()), + ])), + ); + body.insert("partition".into(), "0".into()); + body.insert("begin_sequence_number".into(), Value::Long(0)); + body.insert("last_enqueued_sequence_number".into(), Value::Long(1)); + body.insert("last_enqueued_offset".into(), "1".into()); + body.insert( + "last_enqueued_time_utc".into(), + Value::Timestamp(Timestamp::from(0)), + ); + body.insert("is_partition_empty".into(), false.into()); + let status = if is_cbs { + 202i32 + } else if state.mode & SERVICE_ERROR != 0 + && !state.resumed.load(Ordering::SeqCst) + { + 500i32 + } else { + 200i32 + }; + let message = Message::builder() + .properties(Properties { + correlation_id, + ..Default::default() + }) + .application_properties( + ApplicationProperties::builder() + .insert("statusCode", status) + .insert("statusDescription", "cleanup peer response") + .build(), + ) + .value(Value::Map( + body.into_iter() + .map(|(k, v)| (Value::String(k), v)) + .collect(), + )) + .build(); + if responses.send(message.map_body(Body::Value)).await.is_err() { + break; + } + } + }); + } + LinkEndpoint::Sender(mut sender) => { + let Some(mut replies) = pending_replies.take() else { + break; + }; + // Pair each response link with the preceding request link. + tokio::spawn(async move { + let mut responses_closed = false; + loop { + tokio::select! { + _ = sender.on_detach() => { let _ = sender.close().await; break; }, + response = replies.recv(), if !responses_closed => { + let Some(response) = response else { responses_closed = true; continue; }; + if sender.send(response).await.is_err() { break; } + } + } + } + }); + } + } + } +} + +#[no_mangle] +pub extern "C" fn cleanup_peer_start(mode: u32) -> *mut CleanupPeer { + Box::into_raw(Box::new(CleanupPeer::start(mode))) +} +/// # Safety +/// `peer` must be a live pointer returned by cleanup_peer_start. +#[no_mangle] +pub unsafe extern "C" fn cleanup_peer_port(peer: *const CleanupPeer) -> u16 { + (*peer).port() +} +/// # Safety +/// `peer` must be live for the duration of this call. +#[no_mangle] +pub unsafe extern "C" fn cleanup_peer_wait( + peer: *const CleanupPeer, + event: u32, + timeout_ms: u64, +) -> bool { + (*peer).wait(event, Duration::from_millis(timeout_ms)) +} +/// # Safety +/// `peer` must be live for the duration of this call. +#[no_mangle] +pub unsafe extern "C" fn cleanup_peer_resume(peer: *const CleanupPeer) { + (*peer).resume(); +} +/// # Safety +/// `peer` must be uniquely owned and consumed exactly once. +#[no_mangle] +pub unsafe extern "C" fn cleanup_peer_stop(peer: *mut CleanupPeer) { + drop(Box::from_raw(peer)); +} diff --git a/sdk/core/azure-core-amqp/test/ut/rust_cleanup_peer.hpp b/sdk/core/azure-core-amqp/test/ut/rust_cleanup_peer.hpp new file mode 100644 index 0000000000..260bf92985 --- /dev/null +++ b/sdk/core/azure-core-amqp/test/ut/rust_cleanup_peer.hpp @@ -0,0 +1,68 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#pragma once + +#include +#include +#include +#include +#include + +extern "C" +{ + struct CleanupPeer; + CleanupPeer* cleanup_peer_start(std::uint32_t mode); + std::uint16_t cleanup_peer_port(CleanupPeer const* peer); + bool cleanup_peer_wait(CleanupPeer const* peer, std::uint32_t event, std::uint64_t timeoutMs); + void cleanup_peer_resume(CleanupPeer const* peer); + void cleanup_peer_stop(CleanupPeer* peer); +} +namespace Azure { namespace Core { namespace Amqp { namespace Tests { + class RustCleanupPeer final { + public: + enum Mode : std::uint32_t + { + HoldQuery = 1, + ServiceError = 2 + }; + enum Event : std::uint32_t + { + Request = 1 + }; + explicit RustCleanupPeer(std::uint32_t mode = 0) : m_peer(cleanup_peer_start(mode)) {} + ~RustCleanupPeer() { cleanup_peer_stop(m_peer); } + RustCleanupPeer(RustCleanupPeer const&) = delete; + RustCleanupPeer& operator=(RustCleanupPeer const&) = delete; + std::uint16_t Port() const { return cleanup_peer_port(m_peer); } + bool Wait(Event event) const { return cleanup_peer_wait(m_peer, event, 5000); } + void Resume() { cleanup_peer_resume(m_peer); } + + private: + CleanupPeer* m_peer; + }; + + // Each protocol regression runs in a death-test child. The watchdog exits the + // child before a stuck synchronous FFI call or std::future destructor can hang. + class CleanupWatchdog final { + public: + CleanupWatchdog() + : m_done(m_finished.get_future()), m_thread([this] { + if (m_done.wait_for(std::chrono::seconds(15)) != std::future_status::ready) + { + std::_Exit(124); + } + }) + { + } + ~CleanupWatchdog() + { + m_finished.set_value(); + m_thread.join(); + } + + private: + std::promise m_finished; + std::future m_done; + std::thread m_thread; + }; +}}}} // namespace Azure::Core::Amqp::Tests diff --git a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md index 4cc3768c22..123fbf92b5 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md +++ b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +- Fixed cleanup blocking on pending properties queries with the Rust AMQP backend. Queries can be cancelled without cancelling the caller context, and subsequent queries recreate failed management links. + - Fixed producer cleanup after partition discovery fails during management-client initialization or session shutdown. Cleanup now ends an acquired session independently of management-client creation and supports repeated close calls. - Synchronized properties queries with cleanup during producer recovery. Cleanup cancels an active query before releasing its management client and session, without cancelling the caller's context. diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp index f3d0aa896e..bd6fe7ddb1 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp @@ -203,7 +203,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail message.ApplicationProperties["name"] = static_cast(eventHubName); message.SetBody(Azure::Core::Amqp::Models::AmqpValue{}); - auto result = m_managementClient->ExecuteOperation( + auto result = ExecuteManagementOperation( "READ" /* operation */, "com.microsoft:eventhub" /* type of operation */, "" /* locales */, @@ -264,7 +264,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail message.ApplicationProperties["partition"] = Azure::Core::Amqp::Models::AmqpValue{partitionId}; message.SetBody(Azure::Core::Amqp::Models::AmqpValue{}); - auto result = m_managementClient->ExecuteOperation( + auto result = ExecuteManagementOperation( "READ" /* operation */, "com.microsoft:partition" /* type of operation */, "" /* locales */, @@ -321,41 +321,70 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail } } std::unique_lock lock(m_managementClientMutex); - if (m_managementClientIsOpen) + ResetManagement(context); + // Beginning the session precedes management creation, which can throw before assignment. + if (m_sessionIsBegun) { - // ManagementClient::Close releases its links even when it reports a close failure. - m_managementClientIsOpen = false; try { - m_managementClient->Close(context); + m_session.End(context); + m_sessionIsBegun = false; } catch (std::exception const& ex) { Azure::Core::Diagnostics::_internal::Log::Stream( Azure::Core::Diagnostics::Logger::Level::Warning) - << "Exception while closing the management client: " << ex.what(); + << "Exception while ending the properties client session: " << ex.what(); } } - // Drop link ownership before ending the session. Its native end frees link endpoints. - m_managementClient.reset(); - // Beginning the session precedes management creation, which can throw before assignment. - if (m_sessionIsBegun) + } + + private: + Azure::Core::Amqp::_internal::ManagementOperationResult ExecuteManagementOperation( + std::string const& operation, + std::string const& type, + std::string const& locales, + Azure::Core::Amqp::Models::AmqpMessage message, + Azure::Core::Context const& context) + { + try { + return m_managementClient->ExecuteOperation( + operation, type, locales, std::move(message), context); + } + catch (...) + { + // A cancelled Rust call can leave an unmatched response. Replace links + // lazily on the next query, without delaying this query's exception. + m_managementClientNeedsReset = true; + throw; + } + } + + // Called only while holding m_managementClientMutex. Keep the begun session + // so a later ordinary query can attach fresh management links on it. + void ResetManagement(Azure::Core::Context const& context) + { + if (m_managementClientIsOpen) + { + // ManagementClient::Close releases its links even when it reports a close failure. + m_managementClientIsOpen = false; try { - m_session.End(context); - m_sessionIsBegun = false; + m_managementClient->Close(context); } catch (std::exception const& ex) { Azure::Core::Diagnostics::_internal::Log::Stream( Azure::Core::Diagnostics::Logger::Level::Warning) - << "Exception while ending the properties client session: " << ex.what(); + << "Exception while closing the management client: " << ex.what(); } } + // Drop link ownership before ending the session. Its native end frees link endpoints. + m_managementClient.reset(); + m_managementClientNeedsReset = false; } - private: std::shared_ptr CreateOperationContext( Azure::Core::Context const& context) { @@ -374,6 +403,11 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail // The getter owns m_managementClientMutex through initialization and operation completion. void EnsureManagementClient(Azure::Core::Context const& context) { + context.ThrowIfCancelled(); + if (m_managementClientNeedsReset) + { + ResetManagement(context); + } if (!m_managementClient) { if (!m_sessionIsBegun) @@ -407,6 +441,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail Azure::Core::Amqp::_internal::Session m_session; std::unique_ptr m_managementClient; bool m_managementClientIsOpen{false}; + bool m_managementClientNeedsReset{false}; bool m_sessionIsBegun{false}; std::string m_eventHub; ManagementClientFactory m_createManagementClient; diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt index 759dbc7315..dbad61d49f 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt @@ -32,6 +32,7 @@ add_executable ( processor_test.cpp producer_client_test.cpp properties_cleanup_test.cpp + rust_properties_cleanup_test.cpp reattach_connection_string_test.cpp reattach_policy_test.cpp retry_operation_test.cpp @@ -50,6 +51,13 @@ endif() if(USE_RUST_AMQP) + corrosion_import_crate(MANIFEST_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../../../../core/azure-core-amqp/test/rust-peer/Cargo.toml CRATES amqp_cleanup_peer) + if(NOT APPLE AND (LINUX OR UNIX)) + corrosion_set_env_vars(amqp_cleanup_peer + OPENSSL_INCLUDE_DIR=${OPENSSL_INCLUDE_DIR} + OPENSSL_DIR=${OPENSSL_INCLUDE_DIR}/..) + endif() + target_link_libraries(azure-messaging-eventhubs-test PRIVATE amqp_cleanup_peer) target_compile_definitions(azure-messaging-eventhubs-test PRIVATE ENABLE_RUST_AMQP) else() find_package(umock_c CONFIG REQUIRED) diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp index ca07eb7283..5dca204ec5 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp @@ -42,20 +42,24 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { class PropertiesEndpoint final : public Amqp::Tests::MessageTests::MockServiceEndpoint { public: - PropertiesEndpoint(bool failDiscovery = false, std::promise* requestReceived = nullptr) + PropertiesEndpoint( + bool failDiscovery = false, + std::promise* requestReceived = nullptr, + std::atomic* holdQuery = nullptr) : MockServiceEndpoint("$management", {}), m_failDiscovery{failDiscovery}, - m_requestReceived{requestReceived} + m_requestReceived{requestReceived}, m_holdQuery{holdQuery} { } private: bool m_failDiscovery; std::promise* m_requestReceived; + std::atomic* m_holdQuery; void MessageReceived( std::string const&, std::shared_ptr const& request) override { - if (m_requestReceived) + if (m_requestReceived && (!m_holdQuery || m_holdQuery->load())) { // Acknowledge receipt without replying. Close must interrupt the client's response wait. m_requestReceived->set_value(); @@ -220,6 +224,39 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); } + void CancelledQueryThenRediscover() + { + std::promise requestReceived; + auto received = requestReceived.get_future(); + std::atomic holdQuery{true}; + Amqp::Tests::MessageTests::AmqpServerMock server(AvailablePort()); + server.AddServiceEndpoint( + std::make_shared(false, &requestReceived, &holdQuery)); + server.StartListening(); + { + Amqp::_internal::ConnectionOptions options; + options.Port = server.GetPort(); + Amqp::_internal::Connection connection("localhost", nullptr, options); + EventHubsPropertiesClient properties(connection, "hub"); + Azure::Core::Context caller; + auto query = std::async( + std::launch::async, [&] { properties.GetEventHubsProperties("hub", caller); }); + RequireReady(received); + received.get(); + caller.Cancel(); + RequireReady(query); + EXPECT_ANY_THROW(query.get()); + EXPECT_TRUE(caller.IsCancelled()); + holdQuery = false; + auto context = Azure::Core::Context().WithDeadline( + std::chrono::system_clock::now() + std::chrono::seconds(5)); + EXPECT_EQ(properties.GetEventHubsProperties("hub", context).Name, "hub"); + EXPECT_EQ(properties.GetEventHubsPartitionProperties("hub", "0", context).PartitionId, "0"); + properties.Close(context); + } + server.StopListening(); + } + void SuccessfulDiscovery(bool cancelledClose) { std::atomic closeFailureLogged{false}; @@ -350,6 +387,17 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { } }; + TEST_F(PropertiesCleanupDeathTest, CancelledQueryThenRediscover) + { + EXPECT_EXIT( + { + CancelledQueryThenRediscover(); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + TEST_F(PropertiesCleanupDeathTest, SuccessfulDiscoveryAndRepeatedClose) { EXPECT_EXIT( diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/rust_properties_cleanup_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/rust_properties_cleanup_test.cpp new file mode 100644 index 0000000000..ee100de4ee --- /dev/null +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/rust_properties_cleanup_test.cpp @@ -0,0 +1,302 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#if ENABLE_RUST_AMQP +#include "../../../../core/azure-core-amqp/test/ut/rust_cleanup_peer.hpp" +#include "private/eventhubs_utilities.hpp" + +#include +#include + +#include + +#include + +namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { + namespace { + namespace Amqp = Azure::Core::Amqp; + using Peer = Amqp::Tests::RustCleanupPeer; + using Watchdog = Amqp::Tests::CleanupWatchdog; + using _detail::EventHubsPropertiesClient; + Amqp::_internal::Connection OpenConnection(Peer const& peer) + { + Amqp::_internal::ConnectionOptions options; + options.Port = peer.Port(); + Amqp::_internal::Connection connection("localhost", nullptr, options); + connection.Open({}); + return connection; + } + Core::Context Deadline() + { + return Core::Context().WithDeadline( + std::chrono::system_clock::now() + std::chrono::seconds(3)); + } + void ConcurrentClose(bool partition, bool cancelCaller) + { + Watchdog watchdog; + Peer peer(Peer::HoldQuery); + auto connection = OpenConnection(peer); + { + EventHubsPropertiesClient properties(connection, "hub"); + Core::Context queryContext; // No caller deadline; Close must cancel its child. + auto query = std::async(std::launch::async, [&] { + if (partition) + { + properties.GetEventHubsPartitionProperties("hub", "0", queryContext); + } + else + { + properties.GetEventHubsProperties("hub", queryContext); + } + }); + ASSERT_TRUE(peer.Wait(Peer::Request)); + if (cancelCaller) + { + queryContext.Cancel(); + } + auto close = std::async(std::launch::async, [&] { properties.Close(Deadline()); }); + EXPECT_THROW(query.get(), Core::OperationCancelledException); + EXPECT_NO_THROW(close.get()); + EXPECT_EQ(queryContext.IsCancelled(), cancelCaller); + properties.Close({}); + EXPECT_THROW( + properties.GetEventHubsProperties("hub", {}), Core::OperationCancelledException); + EXPECT_THROW( + properties.GetEventHubsPartitionProperties("hub", "0", {}), + Core::OperationCancelledException); + } + connection.Close(Deadline()); + } + void ConsumerCloseDuringPendingQuery(bool partition, bool expiredClose) + { + Watchdog watchdog; + Peer peer(Peer::HoldQuery); + ConsumerClientOptions options; + options.RetryOptions.MaxRetries = 0; + ConsumerClient consumer( + "Endpoint=sb://localhost:" + std::to_string(peer.Port()) + + "/;SharedAccessKeyName=test;SharedAccessKey=YWJj;UseDevelopmentEmulator=true", + "hub", + DefaultConsumerGroup, + options); + Core::Context caller; + auto query = std::async(std::launch::async, [&] { + if (partition) + { + consumer.GetPartitionProperties("0", caller); + } + else + { + consumer.GetEventHubProperties(caller); + } + }); + ASSERT_TRUE(peer.Wait(Peer::Request)); + auto closeContext = Deadline(); + if (expiredClose) + { + closeContext = Core::Context().WithDeadline(std::chrono::system_clock::now()); + } + auto close = std::async(std::launch::async, [&] { consumer.Close(closeContext); }); + EXPECT_THROW(query.get(), Core::OperationCancelledException); + EXPECT_NO_THROW(close.get()); + EXPECT_FALSE(caller.IsCancelled()); + consumer.Close(Deadline()); + } + + void QueryDeadlineThenRediscover() + { + Watchdog watchdog; + Peer peer(Peer::HoldQuery); + auto connection = OpenConnection(peer); + { + EventHubsPropertiesClient properties(connection, "hub"); + auto context = Deadline(); + auto query = std::async( + std::launch::async, [&] { properties.GetEventHubsProperties("hub", context); }); + ASSERT_TRUE(peer.Wait(Peer::Request)); + EXPECT_THROW(query.get(), Core::OperationCancelledException); + peer.Resume(); + EXPECT_EQ(properties.GetEventHubsProperties("hub", Deadline()).Name, "hub"); + properties.Close(Deadline()); + } + connection.Close(Deadline()); + } + + void CancelThenRediscover(bool partition) + { + Watchdog watchdog; + Peer peer(Peer::HoldQuery); + ProducerClientOptions options; + options.RetryOptions.MaxRetries = 0; + ProducerClient producer( + "Endpoint=sb://localhost:" + std::to_string(peer.Port()) + + "/;SharedAccessKeyName=test;SharedAccessKey=YWJj;UseDevelopmentEmulator=true", + "hub", + options); + Core::Context caller; + auto first = std::async(std::launch::async, [&] { + if (partition) + { + producer.GetPartitionProperties("0", caller); + } + else + { + producer.GetEventHubProperties(caller); + } + }); + ASSERT_TRUE(peer.Wait(Peer::Request)); + caller.Cancel(); + EXPECT_THROW(first.get(), Core::OperationCancelledException); + peer.Resume(); + EXPECT_EQ(producer.GetEventHubProperties(Deadline()).Name, "hub"); + EXPECT_EQ(producer.GetPartitionProperties("0", Deadline()).PartitionId, "0"); + producer.Close(Deadline()); + producer.Close(Deadline()); + } + void ServiceError() + { + Watchdog watchdog; + Peer peer(Peer::ServiceError); + auto connection = OpenConnection(peer); + { + EventHubsPropertiesClient properties(connection, "hub"); + try + { + properties.GetEventHubsProperties("hub", Deadline()); + ADD_FAILURE(); + } + catch (std::runtime_error const& error) + { + EXPECT_NE(std::string(error.what()).find("500"), std::string::npos); + EXPECT_NE(std::string(error.what()).find("cleanup peer response"), std::string::npos); + } + peer.Resume(); + EXPECT_EQ(properties.GetEventHubsProperties("hub", Deadline()).Name, "hub"); + properties.Close(Deadline()); + } + connection.Close(Deadline()); + } + void DirectManagementRejectsReuseAfterFailure() + { + Watchdog watchdog; + Peer peer(Peer::ServiceError); + auto connection = OpenConnection(peer); + auto session = connection.CreateSession(); + session.Begin(Deadline()); + auto management = session.CreateManagementClient("hub", {}); + EXPECT_EQ(management.Open(Deadline()), Amqp::_internal::ManagementOpenStatus::Ok); + Amqp::Models::AmqpMessage message; + message.ApplicationProperties["name"] = "hub"; + EXPECT_THROW( + management.ExecuteOperation("READ", "com.microsoft:eventhub", "", message, Deadline()), + std::runtime_error); + peer.Resume(); + try + { + auto result = management.ExecuteOperation( + "READ", "com.microsoft:eventhub", "", message, Deadline()); + (void)result; + ADD_FAILURE() << "Failed management client was reused"; + } + catch (std::runtime_error const& error) + { + EXPECT_NE(std::string(error.what()).find("close and reopen"), std::string::npos); + } + management.Close(Deadline()); + EXPECT_EQ(management.Open(Deadline()), Amqp::_internal::ManagementOpenStatus::Ok); + EXPECT_NO_THROW( + management.ExecuteOperation("READ", "com.microsoft:eventhub", "", message, Deadline())); + management.Close(Deadline()); + session.End(Deadline()); + connection.Close(Deadline()); + } + } // namespace + class RustPropertiesCleanupDeathTest : public ::testing::Test { + protected: + void SetUp() override { ::testing::FLAGS_gtest_death_test_style = "threadsafe"; } + }; + TEST_F(RustPropertiesCleanupDeathTest, CancelledPropertiesThenRediscover) + { + EXPECT_EXIT( + { + CancelThenRediscover(false); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + + TEST_F(RustPropertiesCleanupDeathTest, CancelledPartitionThenRediscover) + { + EXPECT_EXIT( + { + CancelThenRediscover(true); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + + TEST_F(RustPropertiesCleanupDeathTest, ServiceErrorIsExceptionThenRediscover) + { + EXPECT_EXIT( + { + ServiceError(); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + + TEST_F(RustPropertiesCleanupDeathTest, DirectManagementFailureRequiresReopen) + { + EXPECT_EXIT( + { + DirectManagementRejectsReuseAfterFailure(); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + class RustConcurrentPropertiesCleanupDeathTest + : public RustPropertiesCleanupDeathTest, + public ::testing::WithParamInterface> {}; + TEST_P(RustConcurrentPropertiesCleanupDeathTest, CloseDuringPendingQuery) + { + auto partition = std::get<0>(GetParam()); + auto cancelCaller = std::get<1>(GetParam()); + EXPECT_EXIT( + { + ConcurrentClose(partition, cancelCaller); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + TEST_P(RustConcurrentPropertiesCleanupDeathTest, ConsumerCloseDuringPendingQuery) + { + auto partition = std::get<0>(GetParam()); + auto expiredClose = std::get<1>(GetParam()); + EXPECT_EXIT( + { + ConsumerCloseDuringPendingQuery(partition, expiredClose); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + TEST_F(RustPropertiesCleanupDeathTest, QueryDeadlineThenRediscover) + { + EXPECT_EXIT( + { + QueryDeadlineThenRediscover(); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + INSTANTIATE_TEST_SUITE_P( + PropertiesAndPartitions, + RustConcurrentPropertiesCleanupDeathTest, + ::testing::Combine(::testing::Bool(), ::testing::Bool())); +}}}} // namespace Azure::Messaging::EventHubs::Test +#endif From 78920b29fb16fe3314f1a49e3d12845b600e584c Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Wed, 28 Oct 2026 13:36:34 -0400 Subject: [PATCH 09/11] refactor(eventhubs): defer Rust cleanup to follow-up Revert c86152e01181b8f3df7f3f94b79a02f15e189124 and keep the properties-query lock and child-context cancellation specific to uAMQP. Rust keeps its initialization-only lock until management cancellation is implemented. Track the Rust implementation, recovery behavior, and regression tests in Azure/azure-sdk-for-rust#5527. Preserve the uAMQP discovery/session cleanup fix and checkpoint recordings. --- Cargo.lock | 9 - Cargo.toml | 1 - doc/EventHubsCleanupTests.md | 38 +-- sdk/core/azure-core-amqp/CHANGELOG.md | 2 - sdk/core/azure-core-amqp/CMakeLists.txt | 2 +- .../amqp/internal/common/runtime_context.hpp | 14 - .../src/impl/rust_amqp/amqp/management.cpp | 20 -- .../amqp/private/management_impl.hpp | 1 - .../rust_wrapper/src/amqp/management.rs | 21 +- .../rust_wrapper/src/call_context.rs | 187 +---------- .../azure-core-amqp/test/rust-peer/Cargo.toml | 14 - .../azure-core-amqp/test/rust-peer/src/lib.rs | 253 --------------- .../test/ut/rust_cleanup_peer.hpp | 68 ---- .../azure-messaging-eventhubs/CHANGELOG.md | 4 +- .../src/private/eventhubs_utilities.hpp | 86 ++--- .../test/ut/CMakeLists.txt | 8 - .../test/ut/properties_cleanup_test.cpp | 54 +--- .../test/ut/rust_properties_cleanup_test.cpp | 302 ------------------ 18 files changed, 51 insertions(+), 1033 deletions(-) delete mode 100644 sdk/core/azure-core-amqp/test/rust-peer/Cargo.toml delete mode 100644 sdk/core/azure-core-amqp/test/rust-peer/src/lib.rs delete mode 100644 sdk/core/azure-core-amqp/test/ut/rust_cleanup_peer.hpp delete mode 100644 sdk/eventhubs/azure-messaging-eventhubs/test/ut/rust_properties_cleanup_test.cpp diff --git a/Cargo.lock b/Cargo.lock index 17d48f8e84..27f79a3ecf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -26,15 +26,6 @@ dependencies = [ "memchr", ] -[[package]] -name = "amqp_cleanup_peer" -version = "0.1.0" -dependencies = [ - "fe2o3-amqp", - "fe2o3-amqp-types", - "tokio", -] - [[package]] name = "android-tzdata" version = "0.1.1" diff --git a/Cargo.toml b/Cargo.toml index 6163465e1d..3a230172b5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,6 @@ [workspace] resolver = "2" members = [ - "sdk/core/azure-core-amqp/test/rust-peer", "sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/azure_core_amqp", "sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper", ] diff --git a/doc/EventHubsCleanupTests.md b/doc/EventHubsCleanupTests.md index 57760e66fc..f02ec3a5d2 100644 --- a/doc/EventHubsCleanupTests.md +++ b/doc/EventHubsCleanupTests.md @@ -2,9 +2,11 @@ These tests cover producer cleanup after partition discovery fails and cleanup while a properties query is running. They verify that management clients, sessions, and connection polling references are released safely, including when initialization or shutdown throws. -The tests use local loopback listeners with the uAMQP and Rust backends. They do not require Azure credentials, a live broker, or the HTTP test proxy. See [CONTRIBUTING.md](https://github.com/Azure/azure-sdk-for-cpp/blob/main/CONTRIBUTING.md) for compiler and dependency setup. +The tests use the uAMQP backend and local loopback listeners. They do not require Azure credentials, a live broker, or the HTTP test proxy. See [CONTRIBUTING.md](https://github.com/Azure/azure-sdk-for-cpp/blob/main/CONTRIBUTING.md) for compiler and dependency setup. -## Build and run with uAMQP +Properties-query cancellation and synchronization with cleanup apply only to uAMQP. The Rust backend retains its initialization-only lock because pending Rust management calls do not observe child-context cancellation. Rust cancellation, recovery, and concurrent cleanup are tracked in [azure-sdk-for-rust#5527](https://github.com/Azure/azure-sdk-for-rust/issues/5527). + +## Build and run Run from the repository root, adding any toolchain or platform options required by your development environment: @@ -28,7 +30,7 @@ ctest --test-dir build/cleanup -C Debug \ The configure option sets the Event Hubs test timeout to 60 seconds; CTest's `--timeout` alone does not override an explicit per-test timeout. -The discovery command should list **21 cases**: four session cases, nine properties cleanup cases, and eight parameterized concurrency cases. These two test files are guarded by `ENABLE_UAMQP`. The Rust backend uses the separate cases described below; a successful run with no matching tests does not validate this cleanup behavior. +The discovery command should list **20 cases**: four session cases, eight properties cleanup cases, and eight parameterized concurrency cases. Both test files are guarded by `ENABLE_UAMQP`. The default Rust backend does not include them; a successful run with no matching tests does not validate this cleanup behavior. To repeat just the concurrency cases five times: @@ -38,35 +40,6 @@ ctest --test-dir build/cleanup -C Debug \ --repeat until-fail:5 --output-on-failure --timeout 60 ``` -## Build and run with Rust - -```sh -cmake -S . -B build/cleanup-rust \ - -DCMAKE_BUILD_TYPE=Debug \ - -DBUILD_TESTING=ON \ - -DAZURE_EVENTHUBS_CTEST_TIMEOUT=60 \ - -DDISABLE_RUST_IN_BUILD=OFF -cmake --build build/cleanup-rust --config Debug --target azure-messaging-eventhubs-test -ctest --test-dir build/cleanup-rust -C Debug \ - -R 'Rust.*PropertiesCleanupDeathTest' --output-on-failure --timeout 60 -cargo test -p rust_wrapper call_context -- --test-threads=1 -``` - -This selects **13 C++ cases** and **six Rust context cases**. The C++ tests link a test-only Rust AMQP peer built from the existing fe2o3 dependency. The peer acknowledges receipt of management requests and withholds their responses, so cancellation occurs after a real request reaches the server. A 15-second child-process watchdog reports a blocked query or close as a failure. - -Source: [rust_properties_cleanup_test.cpp](https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/eventhubs/azure-messaging-eventhubs/test/ut/rust_properties_cleanup_test.cpp). - -| Cases | Behavior checked | -| --- | --- | -| `CloseDuringPendingQuery` (four) | Both getters release their operation lock when close cancels the child context; caller cancellation state is preserved, repeated close succeeds, and stale references reject queries. | -| `ConsumerCloseDuringPendingQuery` (four) | Public consumer close completes with either getter pending, including when the close deadline has already expired. The original query context has no deadline and remains uncancelled. | -| `CancelledPropertiesThenRediscover`, `CancelledPartitionThenRediscover` | Caller cancellation interrupts the query; subsequent public producer queries succeed with fresh links. | -| `QueryDeadlineThenRediscover` | A query deadline interrupts the wait; a later query succeeds. | -| `ServiceErrorIsExceptionThenRediscover` | A real service error produces an exception instead of converting a null Rust value; a later query succeeds. | -| `DirectManagementFailureRequiresReopen` | A failed management client rejects reuse until close and reopen. | - -The Rust context tests cover cancellation before and during a wait, deadlines, successful results, dropping the pending future exactly once, and teardown that runs despite cancellation. These changes cover management calls after initialization; they do not establish cancellation of pending session begin, authentication, or management attach, or forced termination of dependency background tasks. - ## Session ownership and shutdown Source: [session_cleanup_tests.cpp](https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp), fixture `SessionCleanupDeathTest`. @@ -84,7 +57,6 @@ Source: [properties_cleanup_test.cpp](https://github.com/Azure/azure-sdk-for-cpp | Test | Behavior checked | | --- | --- | -| `CancelledQueryThenRediscover` | Caller cancellation interrupts a query, and both getters succeed on subsequent queries with fresh management links. | | `SuccessfulDiscoveryAndRepeatedClose` | Hub and partition properties are returned correctly, and repeated close ends the session exactly once. | | `ManagementCloseFailureStillEndsSession` | A canceled management close logs its failure, but session cleanup still runs exactly once. | | `CreationFailureThenRepeatedClose` | Management creation throws after session begin. Retrying creation preserves the original error and does not acquire another polling reference; repeated close releases ownership. | diff --git a/sdk/core/azure-core-amqp/CHANGELOG.md b/sdk/core/azure-core-amqp/CHANGELOG.md index 475506c9a5..259e3233a1 100644 --- a/sdk/core/azure-core-amqp/CHANGELOG.md +++ b/sdk/core/azure-core-amqp/CHANGELOG.md @@ -8,8 +8,6 @@ ### Bugs Fixed -- Fixed Rust management calls ignoring cancellation and deadlines. Failed calls now throw safely, require fresh management links before reuse, and allow bounded management teardown. - - Fixed uAMQP session destruction after a failed `End`. Native resources and the connection polling reference are released when the last session owner is destroyed; callers must still end sessions before destruction. - Fixed uAMQP message annotation encoding so messages with annotations, including Event Hubs partition keys, include the required AMQP section descriptor. [[#7426]](https://github.com/Azure/azure-sdk-for-cpp/issues/7426) diff --git a/sdk/core/azure-core-amqp/CMakeLists.txt b/sdk/core/azure-core-amqp/CMakeLists.txt index 96cf2e978e..17fa8239ae 100644 --- a/sdk/core/azure-core-amqp/CMakeLists.txt +++ b/sdk/core/azure-core-amqp/CMakeLists.txt @@ -137,7 +137,7 @@ endif() # Import targets defined in a package or workspace manifest `Cargo.toml` file - corrosion_import_crate(MANIFEST_PATH ${CMAKE_CURRENT_SOURCE_DIR}/src/impl/rust_amqp/rust_amqp/rust_wrapper/Cargo.toml CRATES rust_wrapper) + corrosion_import_crate(MANIFEST_PATH ${CMAKE_CURRENT_SOURCE_DIR}/src/impl/rust_amqp/rust_amqp/rust_wrapper/Cargo.toml) # Set the build environment variables for the rust_wrapper crate to use our targetted OpenSSL version. if(NOT APPLE) diff --git a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/runtime_context.hpp b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/runtime_context.hpp index e811bfa93f..19fef5817d 100644 --- a/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/runtime_context.hpp +++ b/sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/runtime_context.hpp @@ -125,15 +125,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace Common { namespace GetCallContext(), timeoutMilliseconds); } - /** Observe cancellation only during a synchronous management operation. - * This object and its stored Context must remain stationary until FFI returns. - */ - void SetOperationCancellation() - { - Azure::Core::Amqp::RustInterop::_detail::call_context_set_cancellation( - GetCallContext(), IsOperationCancelled, &m_context); - } - std::uint64_t GetTeardownTimeoutMilliseconds() const { auto const remaining = GetTimeoutMilliseconds(); @@ -174,11 +165,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace Common { namespace } private: - static bool IsOperationCancelled(void const* state) noexcept - { - return static_cast(state)->IsCancelled(); - } - UniqueRustCallContext m_callContext; Azure::Core::Context m_context; }; diff --git a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/management.cpp b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/management.cpp index 75c90dcc87..388d6fc543 100644 --- a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/management.cpp @@ -82,7 +82,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { throw std::runtime_error("Could not attach management object: " + callContext.GetError()); } m_isOpen = true; - m_operationFailed = false; return _internal::ManagementOpenStatus::Ok; } catch (...) @@ -105,17 +104,9 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { << "Execute Operation called when management is not initialized."; throw std::runtime_error("Management is not open!"); } - if (m_operationFailed) - { - throw std::runtime_error( - "Management operation failed; close and reopen the client before reuse."); - } Common::_detail::CallContext callContext( Common::_detail::GlobalStateHolder::GlobalStateInstance()->GetRuntimeContext(), context); - callContext.SetTimeoutMilliseconds(callContext.GetTimeoutMilliseconds()); - callContext.SetOperationCancellation(); - Azure::Core::Amqp::Models::AmqpMap propertiesMap; for (const auto& val : messageToSend.ApplicationProperties) { @@ -130,15 +121,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { typeOfOperation.c_str(), Models::_detail::AmqpValueFactory::ToImplementation(applicationProperties))}; - if (!value) - { - // Cancellation can leave an unmatched response in the management links. - // Keep the handle open for consuming Close, but prohibit another query. - m_operationFailed = true; - context.ThrowIfCancelled(); - throw std::runtime_error("Management operation failed: " + callContext.GetError()); - } - auto responseMessage = std::make_shared(); responseMessage->SetBody(Models::_detail::AmqpValueFactory::FromImplementation(value)); _internal::ManagementOperationResult result; @@ -158,8 +140,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Common::_detail::CallContext callContext( Common::_detail::GlobalStateHolder::GlobalStateInstance()->GetRuntimeContext(), context); - callContext.SetTeardownTimeoutMilliseconds(); - // Even if the detach fails, the management client is closed. A client that failed to detach is // not usable again, and a client that keeps the open flag stops the process in its destructor. m_isOpen = false; diff --git a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/management_impl.hpp b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/management_impl.hpp index e6bac84920..5ed8334bf3 100644 --- a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/management_impl.hpp +++ b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/management_impl.hpp @@ -72,7 +72,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { private: bool m_isOpen{false}; - bool m_operationFailed{false}; UniqueAmqpManagement m_management; _internal::ManagementClientOptions m_options; std::shared_ptr<_detail::SessionImpl> m_session; diff --git a/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/amqp/management.rs b/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/amqp/management.rs index a9a86e0c2e..efb4d445d5 100644 --- a/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/amqp/management.rs +++ b/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/amqp/management.rs @@ -134,14 +134,10 @@ pub unsafe extern "C" fn amqpmanagement_detach_and_release( let management = Box::from_raw(management); let call_context = call_context_from_ptr_mut(call_context); trace!("Starting to detach management"); - let result = call_context.block_on_with_timeout(management.detach()); - let result = match result { - Ok(result) => result, - Err(err) => { - call_context.set_error(Box::new(err)); - return -1; - } - }; + let result = call_context + .runtime_context() + .runtime() + .block_on(management.detach()); trace!("Detached management"); if let Err(err) = result { error!("Failed to detach management: {:?}", err); @@ -180,18 +176,11 @@ pub unsafe extern "C" fn amqpmanagement_call( }) .collect(); trace!("Converted application properties."); - let result = call_context.block_on_with_cancellation( + let result = call_context.runtime_context().runtime().block_on( management .inner .call(operation_type.to_string(), application_properties), ); - let result = match result { - Ok(result) => result, - Err(err) => { - call_context.set_error(Box::new(err)); - return ptr::null_mut(); - } - }; if let Err(err) = result { error!("Failed to call management: {:?}", err); call_context.set_error(Box::new(err)); diff --git a/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/call_context.rs b/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/call_context.rs index bd9ed61dac..7f149c41c3 100644 --- a/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/call_context.rs +++ b/sdk/core/azure-core-amqp/src/impl/rust_amqp/rust_amqp/rust_wrapper/src/call_context.rs @@ -4,7 +4,7 @@ // cspell: words reqwest repr tokio use crate::runtime_context::RuntimeContext; -use std::ffi::{c_char, c_void}; +use std::ffi::c_char; use std::future::Future; use std::time::Duration; @@ -13,8 +13,6 @@ pub struct RustCallContext { error: Option, /// The bound for one call. `Duration::MAX` means the call has no bound. timeout: Duration, - cancellation_probe: Option bool>, - cancellation_state: *const c_void, } impl RustCallContext { @@ -23,8 +21,6 @@ impl RustCallContext { runtime_context, error: None, timeout: Duration::MAX, - cancellation_probe: None, - cancellation_state: std::ptr::null(), } } @@ -46,50 +42,6 @@ impl RustCallContext { } } - fn is_cancelled(&self) -> bool { - self.cancellation_probe - .map(|probe| unsafe { probe(self.cancellation_state) }) - .unwrap_or(false) - } - - /// The callback and its state are borrowed only for this synchronous call. - /// This future is never spawned onto the runtime's worker threads. - pub(crate) fn block_on_with_cancellation( - &self, - future: F, - ) -> Result { - if self.is_cancelled() { - return Err(OperationCancelledOrTimedOut::Cancelled); - } - if self.timeout() == Some(Duration::ZERO) { - return Err(OperationCancelledOrTimedOut::TimedOut); - } - let runtime = unsafe { self.runtime_context() }.runtime(); - runtime.block_on(async { - let cancellation = async { - loop { - if self.is_cancelled() { - return; - } - tokio::time::sleep(Duration::from_millis(10)).await; - } - }; - let operation = async { - match self.timeout() { - Some(duration) => tokio::time::timeout(duration, future) - .await - .map_err(|_| OperationCancelledOrTimedOut::TimedOut), - None => Ok(future.await), - } - }; - tokio::select! { - biased; - _ = cancellation => Err(OperationCancelledOrTimedOut::Cancelled), - result = operation => result, - } - }) - } - /// Run the future on the runtime, and end it when the bound expires. /// /// A future that never completes, for example a send on a connection that @@ -111,37 +63,6 @@ impl RustCallContext { } } -#[derive(Debug, PartialEq, Eq)] -pub(crate) enum OperationCancelledOrTimedOut { - Cancelled, - TimedOut, -} - -impl std::fmt::Display for OperationCancelledOrTimedOut { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(match self { - Self::Cancelled => "AMQP operation cancelled", - Self::TimedOut => "AMQP operation timed out", - }) - } -} -impl std::error::Error for OperationCancelledOrTimedOut {} - -/// Register an optional cancellation probe for an operation. The state must -/// remain valid and stationary until the synchronous operation returns. -/// -/// # Safety -/// The caller owns the state; the callback must not throw across the C ABI. -#[no_mangle] -pub unsafe extern "C" fn call_context_set_cancellation( - ctx: *mut RustCallContext, - probe: Option bool>, - state: *const c_void, -) { - (*ctx).cancellation_probe = probe; - (*ctx).cancellation_state = state; -} - #[no_mangle] pub extern "C" fn call_context_new(runtime_context: *mut RuntimeContext) -> *mut RustCallContext { Box::into_raw(Box::new(RustCallContext::new(runtime_context))) @@ -231,109 +152,3 @@ fn test_call_context_block_on_with_timeout() { drop(Box::from_raw(runtime_context)); } } - -#[cfg(test)] -mod cancellation_tests { - use super::*; - use std::pin::Pin; - use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; - use std::sync::Arc; - use std::task::{Context, Poll}; - - struct PendingOperation { - polls: Arc, - drops: Arc, - } - impl Future for PendingOperation { - type Output = (); - fn poll(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<()> { - self.polls.fetch_add(1, Ordering::SeqCst); - Poll::Pending - } - } - impl Drop for PendingOperation { - fn drop(&mut self) { - self.drops.fetch_add(1, Ordering::SeqCst); - } - } - unsafe extern "C" fn probe(state: *const c_void) -> bool { - (*(state as *const AtomicBool)).load(Ordering::SeqCst) - } - #[test] - fn call_context_pre_cancel_and_expired_deadline_never_poll() { - let mut runtime = RuntimeContext::new().unwrap(); - let mut context = RustCallContext::new(&mut runtime); - let state = AtomicBool::new(true); - unsafe { - call_context_set_cancellation(&mut context, Some(probe), &state as *const _ as _); - } - for expected in [ - OperationCancelledOrTimedOut::Cancelled, - OperationCancelledOrTimedOut::TimedOut, - ] { - let polls = Arc::new(AtomicUsize::new(0)); - let drops = Arc::new(AtomicUsize::new(0)); - assert_eq!( - context.block_on_with_cancellation(PendingOperation { - polls: polls.clone(), - drops: drops.clone() - }), - Err(expected) - ); - assert_eq!(polls.load(Ordering::SeqCst), 0); - assert_eq!(drops.load(Ordering::SeqCst), 1); - state.store(false, Ordering::SeqCst); - context.timeout = Duration::ZERO; - } - } - #[test] - fn call_context_cancellation_drops_pending_future_once() { - let mut runtime = RuntimeContext::new().unwrap(); - let mut context = RustCallContext::new(&mut runtime); - let state = Arc::new(AtomicBool::new(false)); - let polls = Arc::new(AtomicUsize::new(0)); - let drops = Arc::new(AtomicUsize::new(0)); - unsafe { - call_context_set_cancellation(&mut context, Some(probe), Arc::as_ptr(&state) as _); - } - let signal = state.clone(); - let observed = polls.clone(); - let cancel = std::thread::spawn(move || { - let start = std::time::Instant::now(); - while observed.load(Ordering::SeqCst) == 0 { - assert!(start.elapsed() < Duration::from_secs(5)); - std::thread::yield_now(); - } - signal.store(true, Ordering::SeqCst); - }); - context.timeout = Duration::from_secs(5); - assert_eq!( - context.block_on_with_cancellation(PendingOperation { - polls, - drops: drops.clone() - }), - Err(OperationCancelledOrTimedOut::Cancelled) - ); - cancel.join().unwrap(); - assert_eq!(drops.load(Ordering::SeqCst), 1); - } - #[test] - fn call_context_success_deadline_and_teardown() { - let mut runtime = RuntimeContext::new().unwrap(); - let mut context = RustCallContext::new(&mut runtime); - context.timeout = Duration::from_millis(20); - assert_eq!(context.block_on_with_cancellation(async { 7 }), Ok(7)); - assert_eq!( - context.block_on_with_cancellation(std::future::pending::<()>()), - Err(OperationCancelledOrTimedOut::TimedOut) - ); - let state = AtomicBool::new(true); - unsafe { - call_context_set_cancellation(&mut context, Some(probe), &state as *const _ as _); - } - assert_eq!(context.block_on_with_timeout(async { 9 }).unwrap(), 9); - assert!(context - .block_on_with_timeout(std::future::pending::<()>()) - .is_err()); - } -} diff --git a/sdk/core/azure-core-amqp/test/rust-peer/Cargo.toml b/sdk/core/azure-core-amqp/test/rust-peer/Cargo.toml deleted file mode 100644 index 63243c2f0e..0000000000 --- a/sdk/core/azure-core-amqp/test/rust-peer/Cargo.toml +++ /dev/null @@ -1,14 +0,0 @@ -# Copyright (c) Microsoft Corporation. Licensed under the MIT License. -[package] -name = "amqp_cleanup_peer" -version = "0.1.0" -edition.workspace = true -publish = false - -[lib] -crate-type = ["staticlib"] - -[dependencies] -fe2o3-amqp = { workspace = true, features = ["acceptor"] } -fe2o3-amqp-types.workspace = true -tokio = { workspace = true, features = ["rt-multi-thread", "net", "io-util", "sync"] } diff --git a/sdk/core/azure-core-amqp/test/rust-peer/src/lib.rs b/sdk/core/azure-core-amqp/test/rust-peer/src/lib.rs deleted file mode 100644 index 22acd1d5cb..0000000000 --- a/sdk/core/azure-core-amqp/test/rust-peer/src/lib.rs +++ /dev/null @@ -1,253 +0,0 @@ -// Copyright (c) Microsoft Corporation. Licensed under the MIT License. -// Test-only AMQP peer. No production target links this crate. -// cspell: words amqp tokio fe2o3 -use fe2o3_amqp::{ - acceptor::{ConnectionAcceptor, LinkAcceptor, LinkEndpoint, SessionAcceptor}, - types::messaging::{ApplicationProperties, Body, Message, Properties}, -}; -use fe2o3_amqp_types::primitives::{Array, OrderedMap, Timestamp, Value}; -use std::{ - sync::{ - atomic::{AtomicBool, Ordering}, - Arc, Condvar, Mutex, - }, - time::Duration, -}; - -pub const HOLD_QUERY: u32 = 1; -pub const SERVICE_ERROR: u32 = 2; -pub const REQUEST: u32 = 1; - -struct State { - mode: u32, - observed: Mutex, - changed: Condvar, - resumed: AtomicBool, -} -impl State { - fn signal(&self, event: u32) { - *self.observed.lock().unwrap() |= event; - self.changed.notify_all(); - } -} -pub struct CleanupPeer { - // Runtime destruction joins the helper tasks, including deliberately stalled - // peers. This is fixture cleanup, not proof of SDK engine termination. - _runtime: tokio::runtime::Runtime, - state: Arc, - port: u16, -} -impl CleanupPeer { - pub fn start(mode: u32) -> Self { - let runtime = tokio::runtime::Builder::new_multi_thread() - .worker_threads(2) - .enable_all() - .build() - .unwrap(); - let state = Arc::new(State { - mode, - observed: Mutex::new(0), - changed: Condvar::new(), - resumed: AtomicBool::new(false), - }); - let listener = runtime - .block_on(tokio::net::TcpListener::bind("127.0.0.1:0")) - .unwrap(); - let port = listener.local_addr().unwrap().port(); - let shared = state.clone(); - runtime.spawn(async move { - while let Ok((tcp, _)) = listener.accept().await { - let state = shared.clone(); - tokio::spawn(async move { - let Ok(mut connection) = ConnectionAcceptor::builder() - .container_id("cleanup-peer") - .sasl_acceptor(fe2o3_amqp::acceptor::SaslAnonymousMechanism::new()) - .build() - .accept(tcp) - .await - else { - return; - }; - while let Some(incoming) = connection.next_incoming_session().await { - let Ok(session) = SessionAcceptor::new() - .accept_incoming_session(incoming, &mut connection) - .await - else { - break; - }; - tokio::spawn(serve_session(session, state.clone())); - } - }); - } - }); - Self { - _runtime: runtime, - state, - port, - } - } - pub fn port(&self) -> u16 { - self.port - } - pub fn wait(&self, event: u32, timeout: Duration) -> bool { - let observed = self.state.observed.lock().unwrap(); - let (observed, _) = self - .state - .changed - .wait_timeout_while(observed, timeout, |flags| *flags & event == 0) - .unwrap(); - *observed & event != 0 - } - pub fn resume(&self) { - self.state.resumed.store(true, Ordering::SeqCst); - } -} - -async fn serve_session( - mut session: fe2o3_amqp::acceptor::ListenerSessionHandle, - state: Arc, -) { - let acceptor = LinkAcceptor::new(); - let mut pending_replies = None; - while let Some(attach) = session.next_incoming_attach().await { - let Ok(link) = acceptor.accept_incoming_attach(attach, &mut session).await else { - continue; - }; - match link { - LinkEndpoint::Receiver(mut receiver) => { - let (responses, replies) = tokio::sync::mpsc::channel::>>(16); - pending_replies = Some(replies); - let state = state.clone(); - tokio::spawn(async move { - while let Ok(delivery) = receiver.recv::>().await { - let target = receiver - .target() - .as_ref() - .and_then(|t| t.address.as_ref()) - .map(|s| s.as_str()) - .unwrap_or(""); - let is_management = target == "$management"; - let is_cbs = target == "$cbs"; - let _ = receiver.accept(&delivery).await; - if !is_management && !is_cbs { - continue; - } - if is_management { - state.signal(REQUEST); - } - if is_management - && state.mode & HOLD_QUERY != 0 - && !state.resumed.load(Ordering::SeqCst) - { - continue; - } - let correlation_id = delivery - .message() - .properties - .as_ref() - .and_then(|p| p.message_id.clone()); - let mut body = OrderedMap::::new(); - body.insert("name".into(), "hub".into()); - body.insert("created_at".into(), Value::Timestamp(Timestamp::from(0))); - body.insert( - "partition_ids".into(), - Value::Array(Array::from(vec![ - Value::String("0".to_string()), - Value::String("1".to_string()), - ])), - ); - body.insert("partition".into(), "0".into()); - body.insert("begin_sequence_number".into(), Value::Long(0)); - body.insert("last_enqueued_sequence_number".into(), Value::Long(1)); - body.insert("last_enqueued_offset".into(), "1".into()); - body.insert( - "last_enqueued_time_utc".into(), - Value::Timestamp(Timestamp::from(0)), - ); - body.insert("is_partition_empty".into(), false.into()); - let status = if is_cbs { - 202i32 - } else if state.mode & SERVICE_ERROR != 0 - && !state.resumed.load(Ordering::SeqCst) - { - 500i32 - } else { - 200i32 - }; - let message = Message::builder() - .properties(Properties { - correlation_id, - ..Default::default() - }) - .application_properties( - ApplicationProperties::builder() - .insert("statusCode", status) - .insert("statusDescription", "cleanup peer response") - .build(), - ) - .value(Value::Map( - body.into_iter() - .map(|(k, v)| (Value::String(k), v)) - .collect(), - )) - .build(); - if responses.send(message.map_body(Body::Value)).await.is_err() { - break; - } - } - }); - } - LinkEndpoint::Sender(mut sender) => { - let Some(mut replies) = pending_replies.take() else { - break; - }; - // Pair each response link with the preceding request link. - tokio::spawn(async move { - let mut responses_closed = false; - loop { - tokio::select! { - _ = sender.on_detach() => { let _ = sender.close().await; break; }, - response = replies.recv(), if !responses_closed => { - let Some(response) = response else { responses_closed = true; continue; }; - if sender.send(response).await.is_err() { break; } - } - } - } - }); - } - } - } -} - -#[no_mangle] -pub extern "C" fn cleanup_peer_start(mode: u32) -> *mut CleanupPeer { - Box::into_raw(Box::new(CleanupPeer::start(mode))) -} -/// # Safety -/// `peer` must be a live pointer returned by cleanup_peer_start. -#[no_mangle] -pub unsafe extern "C" fn cleanup_peer_port(peer: *const CleanupPeer) -> u16 { - (*peer).port() -} -/// # Safety -/// `peer` must be live for the duration of this call. -#[no_mangle] -pub unsafe extern "C" fn cleanup_peer_wait( - peer: *const CleanupPeer, - event: u32, - timeout_ms: u64, -) -> bool { - (*peer).wait(event, Duration::from_millis(timeout_ms)) -} -/// # Safety -/// `peer` must be live for the duration of this call. -#[no_mangle] -pub unsafe extern "C" fn cleanup_peer_resume(peer: *const CleanupPeer) { - (*peer).resume(); -} -/// # Safety -/// `peer` must be uniquely owned and consumed exactly once. -#[no_mangle] -pub unsafe extern "C" fn cleanup_peer_stop(peer: *mut CleanupPeer) { - drop(Box::from_raw(peer)); -} diff --git a/sdk/core/azure-core-amqp/test/ut/rust_cleanup_peer.hpp b/sdk/core/azure-core-amqp/test/ut/rust_cleanup_peer.hpp deleted file mode 100644 index 260bf92985..0000000000 --- a/sdk/core/azure-core-amqp/test/ut/rust_cleanup_peer.hpp +++ /dev/null @@ -1,68 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. -#pragma once - -#include -#include -#include -#include -#include - -extern "C" -{ - struct CleanupPeer; - CleanupPeer* cleanup_peer_start(std::uint32_t mode); - std::uint16_t cleanup_peer_port(CleanupPeer const* peer); - bool cleanup_peer_wait(CleanupPeer const* peer, std::uint32_t event, std::uint64_t timeoutMs); - void cleanup_peer_resume(CleanupPeer const* peer); - void cleanup_peer_stop(CleanupPeer* peer); -} -namespace Azure { namespace Core { namespace Amqp { namespace Tests { - class RustCleanupPeer final { - public: - enum Mode : std::uint32_t - { - HoldQuery = 1, - ServiceError = 2 - }; - enum Event : std::uint32_t - { - Request = 1 - }; - explicit RustCleanupPeer(std::uint32_t mode = 0) : m_peer(cleanup_peer_start(mode)) {} - ~RustCleanupPeer() { cleanup_peer_stop(m_peer); } - RustCleanupPeer(RustCleanupPeer const&) = delete; - RustCleanupPeer& operator=(RustCleanupPeer const&) = delete; - std::uint16_t Port() const { return cleanup_peer_port(m_peer); } - bool Wait(Event event) const { return cleanup_peer_wait(m_peer, event, 5000); } - void Resume() { cleanup_peer_resume(m_peer); } - - private: - CleanupPeer* m_peer; - }; - - // Each protocol regression runs in a death-test child. The watchdog exits the - // child before a stuck synchronous FFI call or std::future destructor can hang. - class CleanupWatchdog final { - public: - CleanupWatchdog() - : m_done(m_finished.get_future()), m_thread([this] { - if (m_done.wait_for(std::chrono::seconds(15)) != std::future_status::ready) - { - std::_Exit(124); - } - }) - { - } - ~CleanupWatchdog() - { - m_finished.set_value(); - m_thread.join(); - } - - private: - std::promise m_finished; - std::future m_done; - std::thread m_thread; - }; -}}}} // namespace Azure::Core::Amqp::Tests diff --git a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md index 123fbf92b5..eb3a7fdc3f 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md +++ b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md @@ -8,10 +8,8 @@ ### Bugs Fixed -- Fixed cleanup blocking on pending properties queries with the Rust AMQP backend. Queries can be cancelled without cancelling the caller context, and subsequent queries recreate failed management links. - - Fixed producer cleanup after partition discovery fails during management-client initialization or session shutdown. Cleanup now ends an acquired session independently of management-client creation and supports repeated close calls. -- Synchronized properties queries with cleanup during producer recovery. Cleanup cancels an active query before releasing its management client and session, without cancelling the caller's context. +- Synchronized uAMQP properties queries with cleanup during producer recovery. Cleanup cancels an active query before releasing its management client and session, without cancelling the caller's context. Rust cancellation and concurrent cleanup are deferred to [azure-sdk-for-rust#5527](https://github.com/Azure/azure-sdk-for-rust/issues/5527). ### Other Changes diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp index bd6fe7ddb1..aa34017461 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp @@ -194,8 +194,12 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail Core::Context const& context) { +#if defined(ENABLE_UAMQP) std::unique_lock lock(m_managementClientMutex); auto operationContext = CreateOperationContext(context); +#else + auto const* operationContext = &context; +#endif EnsureManagementClient(*operationContext); // Send a message to the management endpoint to retrieve the properties of the eventhub. @@ -203,7 +207,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail message.ApplicationProperties["name"] = static_cast(eventHubName); message.SetBody(Azure::Core::Amqp::Models::AmqpValue{}); - auto result = ExecuteManagementOperation( + auto result = m_managementClient->ExecuteOperation( "READ" /* operation */, "com.microsoft:eventhub" /* type of operation */, "" /* locales */, @@ -253,8 +257,12 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail std::string const& partitionId, Core::Context const& context) { +#if defined(ENABLE_UAMQP) std::unique_lock lock(m_managementClientMutex); auto operationContext = CreateOperationContext(context); +#else + auto const* operationContext = &context; +#endif EnsureManagementClient(*operationContext); // Send a message to the management endpoint to retrieve the properties of the eventhub. @@ -264,7 +272,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail message.ApplicationProperties["partition"] = Azure::Core::Amqp::Models::AmqpValue{partitionId}; message.SetBody(Azure::Core::Amqp::Models::AmqpValue{}); - auto result = ExecuteManagementOperation( + auto result = m_managementClient->ExecuteOperation( "READ" /* operation */, "com.microsoft:partition" /* type of operation */, "" /* locales */, @@ -310,6 +318,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail void Close(Azure::Core::Context const& context) { +#if defined(ENABLE_UAMQP) { // Cancel before waiting for the operation lock: a query may have no caller deadline. // Reject queries on stale references once producer/consumer invalidation starts. @@ -320,71 +329,44 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail operationContext->Cancel(); } } +#endif std::unique_lock lock(m_managementClientMutex); - ResetManagement(context); - // Beginning the session precedes management creation, which can throw before assignment. - if (m_sessionIsBegun) + if (m_managementClientIsOpen) { + // ManagementClient::Close releases its links even when it reports a close failure. + m_managementClientIsOpen = false; try { - m_session.End(context); - m_sessionIsBegun = false; + m_managementClient->Close(context); } catch (std::exception const& ex) { Azure::Core::Diagnostics::_internal::Log::Stream( Azure::Core::Diagnostics::Logger::Level::Warning) - << "Exception while ending the properties client session: " << ex.what(); + << "Exception while closing the management client: " << ex.what(); } } - } - - private: - Azure::Core::Amqp::_internal::ManagementOperationResult ExecuteManagementOperation( - std::string const& operation, - std::string const& type, - std::string const& locales, - Azure::Core::Amqp::Models::AmqpMessage message, - Azure::Core::Context const& context) - { - try - { - return m_managementClient->ExecuteOperation( - operation, type, locales, std::move(message), context); - } - catch (...) - { - // A cancelled Rust call can leave an unmatched response. Replace links - // lazily on the next query, without delaying this query's exception. - m_managementClientNeedsReset = true; - throw; - } - } - - // Called only while holding m_managementClientMutex. Keep the begun session - // so a later ordinary query can attach fresh management links on it. - void ResetManagement(Azure::Core::Context const& context) - { - if (m_managementClientIsOpen) + // Drop link ownership before ending the session. Its native end frees link endpoints. + m_managementClient.reset(); + // Beginning the session precedes management creation, which can throw before assignment. + if (m_sessionIsBegun) { - // ManagementClient::Close releases its links even when it reports a close failure. - m_managementClientIsOpen = false; try { - m_managementClient->Close(context); + m_session.End(context); + m_sessionIsBegun = false; } catch (std::exception const& ex) { Azure::Core::Diagnostics::_internal::Log::Stream( Azure::Core::Diagnostics::Logger::Level::Warning) - << "Exception while closing the management client: " << ex.what(); + << "Exception while ending the properties client session: " << ex.what(); } } - // Drop link ownership before ending the session. Its native end frees link endpoints. - m_managementClient.reset(); - m_managementClientNeedsReset = false; } + private: +#if defined(ENABLE_UAMQP) std::shared_ptr CreateOperationContext( Azure::Core::Context const& context) { @@ -399,15 +381,16 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail m_operationContext = operationContext; return operationContext; } +#endif - // The getter owns m_managementClientMutex through initialization and operation completion. void EnsureManagementClient(Azure::Core::Context const& context) { - context.ThrowIfCancelled(); - if (m_managementClientNeedsReset) - { - ResetManagement(context); - } +#if !defined(ENABLE_UAMQP) + // Rust management calls do not yet observe cancellation of the child context. + // Keep the lock limited to initialization until that support is available. + std::unique_lock lock(m_managementClientMutex); +#endif + // With uAMQP, the getter holds this lock through operation completion. if (!m_managementClient) { if (!m_sessionIsBegun) @@ -435,13 +418,14 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail } std::mutex m_managementClientMutex; +#if defined(ENABLE_UAMQP) std::mutex m_operationContextMutex; std::weak_ptr m_operationContext; bool m_isClosed{false}; +#endif Azure::Core::Amqp::_internal::Session m_session; std::unique_ptr m_managementClient; bool m_managementClientIsOpen{false}; - bool m_managementClientNeedsReset{false}; bool m_sessionIsBegun{false}; std::string m_eventHub; ManagementClientFactory m_createManagementClient; diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt index dbad61d49f..759dbc7315 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/CMakeLists.txt @@ -32,7 +32,6 @@ add_executable ( processor_test.cpp producer_client_test.cpp properties_cleanup_test.cpp - rust_properties_cleanup_test.cpp reattach_connection_string_test.cpp reattach_policy_test.cpp retry_operation_test.cpp @@ -51,13 +50,6 @@ endif() if(USE_RUST_AMQP) - corrosion_import_crate(MANIFEST_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../../../../core/azure-core-amqp/test/rust-peer/Cargo.toml CRATES amqp_cleanup_peer) - if(NOT APPLE AND (LINUX OR UNIX)) - corrosion_set_env_vars(amqp_cleanup_peer - OPENSSL_INCLUDE_DIR=${OPENSSL_INCLUDE_DIR} - OPENSSL_DIR=${OPENSSL_INCLUDE_DIR}/..) - endif() - target_link_libraries(azure-messaging-eventhubs-test PRIVATE amqp_cleanup_peer) target_compile_definitions(azure-messaging-eventhubs-test PRIVATE ENABLE_RUST_AMQP) else() find_package(umock_c CONFIG REQUIRED) diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp index 5dca204ec5..ca07eb7283 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp @@ -42,24 +42,20 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { class PropertiesEndpoint final : public Amqp::Tests::MessageTests::MockServiceEndpoint { public: - PropertiesEndpoint( - bool failDiscovery = false, - std::promise* requestReceived = nullptr, - std::atomic* holdQuery = nullptr) + PropertiesEndpoint(bool failDiscovery = false, std::promise* requestReceived = nullptr) : MockServiceEndpoint("$management", {}), m_failDiscovery{failDiscovery}, - m_requestReceived{requestReceived}, m_holdQuery{holdQuery} + m_requestReceived{requestReceived} { } private: bool m_failDiscovery; std::promise* m_requestReceived; - std::atomic* m_holdQuery; void MessageReceived( std::string const&, std::shared_ptr const& request) override { - if (m_requestReceived && (!m_holdQuery || m_holdQuery->load())) + if (m_requestReceived) { // Acknowledge receipt without replying. Close must interrupt the client's response wait. m_requestReceived->set_value(); @@ -224,39 +220,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); } - void CancelledQueryThenRediscover() - { - std::promise requestReceived; - auto received = requestReceived.get_future(); - std::atomic holdQuery{true}; - Amqp::Tests::MessageTests::AmqpServerMock server(AvailablePort()); - server.AddServiceEndpoint( - std::make_shared(false, &requestReceived, &holdQuery)); - server.StartListening(); - { - Amqp::_internal::ConnectionOptions options; - options.Port = server.GetPort(); - Amqp::_internal::Connection connection("localhost", nullptr, options); - EventHubsPropertiesClient properties(connection, "hub"); - Azure::Core::Context caller; - auto query = std::async( - std::launch::async, [&] { properties.GetEventHubsProperties("hub", caller); }); - RequireReady(received); - received.get(); - caller.Cancel(); - RequireReady(query); - EXPECT_ANY_THROW(query.get()); - EXPECT_TRUE(caller.IsCancelled()); - holdQuery = false; - auto context = Azure::Core::Context().WithDeadline( - std::chrono::system_clock::now() + std::chrono::seconds(5)); - EXPECT_EQ(properties.GetEventHubsProperties("hub", context).Name, "hub"); - EXPECT_EQ(properties.GetEventHubsPartitionProperties("hub", "0", context).PartitionId, "0"); - properties.Close(context); - } - server.StopListening(); - } - void SuccessfulDiscovery(bool cancelledClose) { std::atomic closeFailureLogged{false}; @@ -387,17 +350,6 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { } }; - TEST_F(PropertiesCleanupDeathTest, CancelledQueryThenRediscover) - { - EXPECT_EXIT( - { - CancelledQueryThenRediscover(); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, - ::testing::ExitedWithCode(0), - ""); - } - TEST_F(PropertiesCleanupDeathTest, SuccessfulDiscoveryAndRepeatedClose) { EXPECT_EXIT( diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/rust_properties_cleanup_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/rust_properties_cleanup_test.cpp deleted file mode 100644 index ee100de4ee..0000000000 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/rust_properties_cleanup_test.cpp +++ /dev/null @@ -1,302 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. -#if ENABLE_RUST_AMQP -#include "../../../../core/azure-core-amqp/test/ut/rust_cleanup_peer.hpp" -#include "private/eventhubs_utilities.hpp" - -#include -#include - -#include - -#include - -namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { - namespace { - namespace Amqp = Azure::Core::Amqp; - using Peer = Amqp::Tests::RustCleanupPeer; - using Watchdog = Amqp::Tests::CleanupWatchdog; - using _detail::EventHubsPropertiesClient; - Amqp::_internal::Connection OpenConnection(Peer const& peer) - { - Amqp::_internal::ConnectionOptions options; - options.Port = peer.Port(); - Amqp::_internal::Connection connection("localhost", nullptr, options); - connection.Open({}); - return connection; - } - Core::Context Deadline() - { - return Core::Context().WithDeadline( - std::chrono::system_clock::now() + std::chrono::seconds(3)); - } - void ConcurrentClose(bool partition, bool cancelCaller) - { - Watchdog watchdog; - Peer peer(Peer::HoldQuery); - auto connection = OpenConnection(peer); - { - EventHubsPropertiesClient properties(connection, "hub"); - Core::Context queryContext; // No caller deadline; Close must cancel its child. - auto query = std::async(std::launch::async, [&] { - if (partition) - { - properties.GetEventHubsPartitionProperties("hub", "0", queryContext); - } - else - { - properties.GetEventHubsProperties("hub", queryContext); - } - }); - ASSERT_TRUE(peer.Wait(Peer::Request)); - if (cancelCaller) - { - queryContext.Cancel(); - } - auto close = std::async(std::launch::async, [&] { properties.Close(Deadline()); }); - EXPECT_THROW(query.get(), Core::OperationCancelledException); - EXPECT_NO_THROW(close.get()); - EXPECT_EQ(queryContext.IsCancelled(), cancelCaller); - properties.Close({}); - EXPECT_THROW( - properties.GetEventHubsProperties("hub", {}), Core::OperationCancelledException); - EXPECT_THROW( - properties.GetEventHubsPartitionProperties("hub", "0", {}), - Core::OperationCancelledException); - } - connection.Close(Deadline()); - } - void ConsumerCloseDuringPendingQuery(bool partition, bool expiredClose) - { - Watchdog watchdog; - Peer peer(Peer::HoldQuery); - ConsumerClientOptions options; - options.RetryOptions.MaxRetries = 0; - ConsumerClient consumer( - "Endpoint=sb://localhost:" + std::to_string(peer.Port()) - + "/;SharedAccessKeyName=test;SharedAccessKey=YWJj;UseDevelopmentEmulator=true", - "hub", - DefaultConsumerGroup, - options); - Core::Context caller; - auto query = std::async(std::launch::async, [&] { - if (partition) - { - consumer.GetPartitionProperties("0", caller); - } - else - { - consumer.GetEventHubProperties(caller); - } - }); - ASSERT_TRUE(peer.Wait(Peer::Request)); - auto closeContext = Deadline(); - if (expiredClose) - { - closeContext = Core::Context().WithDeadline(std::chrono::system_clock::now()); - } - auto close = std::async(std::launch::async, [&] { consumer.Close(closeContext); }); - EXPECT_THROW(query.get(), Core::OperationCancelledException); - EXPECT_NO_THROW(close.get()); - EXPECT_FALSE(caller.IsCancelled()); - consumer.Close(Deadline()); - } - - void QueryDeadlineThenRediscover() - { - Watchdog watchdog; - Peer peer(Peer::HoldQuery); - auto connection = OpenConnection(peer); - { - EventHubsPropertiesClient properties(connection, "hub"); - auto context = Deadline(); - auto query = std::async( - std::launch::async, [&] { properties.GetEventHubsProperties("hub", context); }); - ASSERT_TRUE(peer.Wait(Peer::Request)); - EXPECT_THROW(query.get(), Core::OperationCancelledException); - peer.Resume(); - EXPECT_EQ(properties.GetEventHubsProperties("hub", Deadline()).Name, "hub"); - properties.Close(Deadline()); - } - connection.Close(Deadline()); - } - - void CancelThenRediscover(bool partition) - { - Watchdog watchdog; - Peer peer(Peer::HoldQuery); - ProducerClientOptions options; - options.RetryOptions.MaxRetries = 0; - ProducerClient producer( - "Endpoint=sb://localhost:" + std::to_string(peer.Port()) - + "/;SharedAccessKeyName=test;SharedAccessKey=YWJj;UseDevelopmentEmulator=true", - "hub", - options); - Core::Context caller; - auto first = std::async(std::launch::async, [&] { - if (partition) - { - producer.GetPartitionProperties("0", caller); - } - else - { - producer.GetEventHubProperties(caller); - } - }); - ASSERT_TRUE(peer.Wait(Peer::Request)); - caller.Cancel(); - EXPECT_THROW(first.get(), Core::OperationCancelledException); - peer.Resume(); - EXPECT_EQ(producer.GetEventHubProperties(Deadline()).Name, "hub"); - EXPECT_EQ(producer.GetPartitionProperties("0", Deadline()).PartitionId, "0"); - producer.Close(Deadline()); - producer.Close(Deadline()); - } - void ServiceError() - { - Watchdog watchdog; - Peer peer(Peer::ServiceError); - auto connection = OpenConnection(peer); - { - EventHubsPropertiesClient properties(connection, "hub"); - try - { - properties.GetEventHubsProperties("hub", Deadline()); - ADD_FAILURE(); - } - catch (std::runtime_error const& error) - { - EXPECT_NE(std::string(error.what()).find("500"), std::string::npos); - EXPECT_NE(std::string(error.what()).find("cleanup peer response"), std::string::npos); - } - peer.Resume(); - EXPECT_EQ(properties.GetEventHubsProperties("hub", Deadline()).Name, "hub"); - properties.Close(Deadline()); - } - connection.Close(Deadline()); - } - void DirectManagementRejectsReuseAfterFailure() - { - Watchdog watchdog; - Peer peer(Peer::ServiceError); - auto connection = OpenConnection(peer); - auto session = connection.CreateSession(); - session.Begin(Deadline()); - auto management = session.CreateManagementClient("hub", {}); - EXPECT_EQ(management.Open(Deadline()), Amqp::_internal::ManagementOpenStatus::Ok); - Amqp::Models::AmqpMessage message; - message.ApplicationProperties["name"] = "hub"; - EXPECT_THROW( - management.ExecuteOperation("READ", "com.microsoft:eventhub", "", message, Deadline()), - std::runtime_error); - peer.Resume(); - try - { - auto result = management.ExecuteOperation( - "READ", "com.microsoft:eventhub", "", message, Deadline()); - (void)result; - ADD_FAILURE() << "Failed management client was reused"; - } - catch (std::runtime_error const& error) - { - EXPECT_NE(std::string(error.what()).find("close and reopen"), std::string::npos); - } - management.Close(Deadline()); - EXPECT_EQ(management.Open(Deadline()), Amqp::_internal::ManagementOpenStatus::Ok); - EXPECT_NO_THROW( - management.ExecuteOperation("READ", "com.microsoft:eventhub", "", message, Deadline())); - management.Close(Deadline()); - session.End(Deadline()); - connection.Close(Deadline()); - } - } // namespace - class RustPropertiesCleanupDeathTest : public ::testing::Test { - protected: - void SetUp() override { ::testing::FLAGS_gtest_death_test_style = "threadsafe"; } - }; - TEST_F(RustPropertiesCleanupDeathTest, CancelledPropertiesThenRediscover) - { - EXPECT_EXIT( - { - CancelThenRediscover(false); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, - ::testing::ExitedWithCode(0), - ""); - } - - TEST_F(RustPropertiesCleanupDeathTest, CancelledPartitionThenRediscover) - { - EXPECT_EXIT( - { - CancelThenRediscover(true); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, - ::testing::ExitedWithCode(0), - ""); - } - - TEST_F(RustPropertiesCleanupDeathTest, ServiceErrorIsExceptionThenRediscover) - { - EXPECT_EXIT( - { - ServiceError(); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, - ::testing::ExitedWithCode(0), - ""); - } - - TEST_F(RustPropertiesCleanupDeathTest, DirectManagementFailureRequiresReopen) - { - EXPECT_EXIT( - { - DirectManagementRejectsReuseAfterFailure(); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, - ::testing::ExitedWithCode(0), - ""); - } - class RustConcurrentPropertiesCleanupDeathTest - : public RustPropertiesCleanupDeathTest, - public ::testing::WithParamInterface> {}; - TEST_P(RustConcurrentPropertiesCleanupDeathTest, CloseDuringPendingQuery) - { - auto partition = std::get<0>(GetParam()); - auto cancelCaller = std::get<1>(GetParam()); - EXPECT_EXIT( - { - ConcurrentClose(partition, cancelCaller); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, - ::testing::ExitedWithCode(0), - ""); - } - TEST_P(RustConcurrentPropertiesCleanupDeathTest, ConsumerCloseDuringPendingQuery) - { - auto partition = std::get<0>(GetParam()); - auto expiredClose = std::get<1>(GetParam()); - EXPECT_EXIT( - { - ConsumerCloseDuringPendingQuery(partition, expiredClose); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, - ::testing::ExitedWithCode(0), - ""); - } - TEST_F(RustPropertiesCleanupDeathTest, QueryDeadlineThenRediscover) - { - EXPECT_EXIT( - { - QueryDeadlineThenRediscover(); - std::exit(::testing::Test::HasFailure() ? 1 : 0); - }, - ::testing::ExitedWithCode(0), - ""); - } - INSTANTIATE_TEST_SUITE_P( - PropertiesAndPartitions, - RustConcurrentPropertiesCleanupDeathTest, - ::testing::Combine(::testing::Bool(), ::testing::Bool())); -}}}} // namespace Azure::Messaging::EventHubs::Test -#endif From 83c414d3504f4d51ca775b480b88f17525dd37e8 Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Wed, 4 Nov 2026 09:37:06 -0500 Subject: [PATCH 10/11] fix(eventhubs): observe cancellation while waiting for properties lock --- doc/EventHubsCleanupTests.md | 16 +- .../azure-messaging-eventhubs/CHANGELOG.md | 2 + .../src/private/eventhubs_utilities.hpp | 24 ++- .../test/ut/properties_cleanup_test.cpp | 137 +++++++++++++++++- 4 files changed, 165 insertions(+), 14 deletions(-) diff --git a/doc/EventHubsCleanupTests.md b/doc/EventHubsCleanupTests.md index f02ec3a5d2..fd8255b3a0 100644 --- a/doc/EventHubsCleanupTests.md +++ b/doc/EventHubsCleanupTests.md @@ -21,22 +21,22 @@ cmake --build build/cleanup --config Debug --target \ azure-core-amqp-tests azure-messaging-eventhubs-test ctest --test-dir build/cleanup -C Debug -N \ - -R 'SessionCleanupDeathTest|PropertiesCleanupDeathTest' + -R 'SessionCleanupDeathTest|PropertiesCleanupDeathTest|WaitingPropertiesCancellationDeathTest' ctest --test-dir build/cleanup -C Debug \ - -R 'SessionCleanupDeathTest|PropertiesCleanupDeathTest' \ + -R 'SessionCleanupDeathTest|PropertiesCleanupDeathTest|WaitingPropertiesCancellationDeathTest' \ --output-on-failure --timeout 60 ``` The configure option sets the Event Hubs test timeout to 60 seconds; CTest's `--timeout` alone does not override an explicit per-test timeout. -The discovery command should list **20 cases**: four session cases, eight properties cleanup cases, and eight parameterized concurrency cases. Both test files are guarded by `ENABLE_UAMQP`. The default Rust backend does not include them; a successful run with no matching tests does not validate this cleanup behavior. +The discovery command should list **28 cases**: four session cases, eight properties cleanup cases, eight parameterized query/close cases, and eight parameterized waiting-query cancellation cases. Both test files are guarded by `ENABLE_UAMQP`. The default Rust backend does not include them; a successful run with no matching tests does not validate this cleanup behavior. To repeat just the concurrency cases five times: ```sh ctest --test-dir build/cleanup -C Debug \ - -R 'ConcurrentPropertiesCleanupDeathTest' \ + -R 'ConcurrentPropertiesCleanupDeathTest|WaitingPropertiesCancellationDeathTest' \ --repeat until-fail:5 --output-on-failure --timeout 60 ``` @@ -79,6 +79,14 @@ Both tests verify that cleanup ends the session once, preserves the caller's can The second test depends on the management log messages containing `ManagementClient::ExecuteOperation:` and `Create Queue for request.`. If those diagnostics change, update the gate as well. The gate uses a bounded wait so a missing signal fails the child instead of hanging the suite. +## Cancellation while waiting for another query + +`WaitingPropertiesCancellationDeathTest.CancelWhileAnotherQueryIsPending` establishes query A by waiting for the mock endpoint to receive its management request. The endpoint withholds the response, and A has no deadline. Query B starts with an independent context and must throw `OperationCancelledException` after explicit cancellation or deadline expiry while A remains pending. + +The test checks that only A's management request reaches the endpoint, A's caller context remains uncancelled, and no session end occurs before close. Closing afterward must terminate A, end the session exactly once, reject queries through stale references, and leave global polling idle. A ten-second watchdog exits the child if an operation fails to finish, preventing a blocked future destructor from hanging the regression. + +The parameter tuple is `(firstPartitionProperties, secondPartitionProperties, expireDeadline)`. All eight combinations cover both getters independently for A and B and both cancellation modes. The queries retain the management lock through operation completion; only the uAMQP lock-acquisition wait checks cancellation between timed attempts. + ## Why these are process tests Except for `MissingEndStillAsserts`, the tests use `EXPECT_EXIT` and require a zero child exit status. An exception assertion alone would miss the original crash: a later destructor can abort after the exception has already been caught. The fixtures use Google Test's `threadsafe` death-test style to re-execute the child without inheriting polling-thread mutexes. diff --git a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md index eb3a7fdc3f..46aa127097 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md +++ b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +- Fixed uAMQP properties queries ignoring cancellation and deadlines while waiting for another properties query to finish. + - Fixed producer cleanup after partition discovery fails during management-client initialization or session shutdown. Cleanup now ends an acquired session independently of management-client creation and supports repeated close calls. - Synchronized uAMQP properties queries with cleanup during producer recovery. Cleanup cancels an active query before releasing its management client and session, without cancelling the caller's context. Rust cancellation and concurrent cleanup are deferred to [azure-sdk-for-rust#5527](https://github.com/Azure/azure-sdk-for-rust/issues/5527). diff --git a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp index aa34017461..3a7e8ac95a 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/private/eventhubs_utilities.hpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -195,7 +196,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail { #if defined(ENABLE_UAMQP) - std::unique_lock lock(m_managementClientMutex); + auto lock = AcquireManagementLock(context); auto operationContext = CreateOperationContext(context); #else auto const* operationContext = &context; @@ -258,7 +259,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail Core::Context const& context) { #if defined(ENABLE_UAMQP) - std::unique_lock lock(m_managementClientMutex); + auto lock = AcquireManagementLock(context); auto operationContext = CreateOperationContext(context); #else auto const* operationContext = &context; @@ -330,7 +331,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail } } #endif - std::unique_lock lock(m_managementClientMutex); + std::unique_lock lock(m_managementClientMutex); if (m_managementClientIsOpen) { // ManagementClient::Close releases its links even when it reports a close failure. @@ -367,6 +368,19 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail private: #if defined(ENABLE_UAMQP) + std::unique_lock AcquireManagementLock(Azure::Core::Context const& context) + { + context.ThrowIfCancelled(); + std::unique_lock lock(m_managementClientMutex, std::defer_lock); + // A query waiting behind an unanswered operation must still observe its own cancellation. + while (!lock.try_lock_for(std::chrono::milliseconds(10))) + { + context.ThrowIfCancelled(); + } + context.ThrowIfCancelled(); + return lock; + } + std::shared_ptr CreateOperationContext( Azure::Core::Context const& context) { @@ -388,7 +402,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail #if !defined(ENABLE_UAMQP) // Rust management calls do not yet observe cancellation of the child context. // Keep the lock limited to initialization until that support is available. - std::unique_lock lock(m_managementClientMutex); + std::unique_lock lock(m_managementClientMutex); #endif // With uAMQP, the getter holds this lock through operation completion. if (!m_managementClient) @@ -417,7 +431,7 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace _detail } } - std::mutex m_managementClientMutex; + std::timed_mutex m_managementClientMutex; #if defined(ENABLE_UAMQP) std::mutex m_operationContextMutex; std::weak_ptr m_operationContext; diff --git a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp index ca07eb7283..ac245b8888 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp +++ b/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp @@ -48,17 +48,24 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { { } + int RequestCount() const { return m_requestCount.load(); } + private: + std::atomic m_requestCount{0}; bool m_failDiscovery; std::promise* m_requestReceived; void MessageReceived( std::string const&, std::shared_ptr const& request) override { + auto const requestCount = ++m_requestCount; if (m_requestReceived) { - // Acknowledge receipt without replying. Close must interrupt the client's response wait. - m_requestReceived->set_value(); + // Acknowledge the first request without replying to any pending query. + if (requestCount == 1) + { + m_requestReceived->set_value(); + } return; } Amqp::Models::AmqpMessage response; @@ -220,6 +227,93 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); } + void CancelWaitingPropertiesQuery( + bool firstPartitionProperties, + bool secondPartitionProperties, + bool expireDeadline) + { + { + std::promise requestReceived; + auto received = requestReceived.get_future(); + auto endpoint = std::make_shared(false, &requestReceived); + Amqp::Tests::MessageTests::AmqpServerMock server(AvailablePort()); + server.AddServiceEndpoint(endpoint); + server.StartListening(); + { + Amqp::_internal::ConnectionOptions options; + options.Port = server.GetPort(); + Amqp::_internal::Connection connection("localhost", nullptr, options); + auto session = Amqp::_detail::SessionFactory::CreateFromInternal( + std::make_shared( + Amqp::_detail::ConnectionFactory::GetImpl(connection), + Amqp::_internal::SessionOptions{}, + nullptr, + CountEnd)); + endCalls = 0; + EventHubsPropertiesClient properties(session, "hub"); + auto query = [&](bool partitionProperties, Core::Context const& context) { + if (partitionProperties) + { + properties.GetEventHubsPartitionProperties("hub", "0", context); + } + else + { + properties.GetEventHubsProperties("hub", context); + } + }; + // Establish an unanswered operation with no deadline before starting the waiter. + Core::Context firstContext; + auto first = std::async( + std::launch::async, [&]() { query(firstPartitionProperties, firstContext); }); + RequireReady(received); + received.get(); + + Core::Context secondContext; + std::promise secondStarted; + auto started = secondStarted.get_future(); + auto second = std::async(std::launch::async, [&]() { + auto context = expireDeadline + ? secondContext.WithDeadline( + std::chrono::system_clock::now() + std::chrono::seconds(1)) + : secondContext; + secondStarted.set_value(); + query(secondPartitionProperties, context); + }); + RequireReady(started); + EXPECT_EQ(second.wait_for(std::chrono::milliseconds(100)), std::future_status::timeout); + if (!expireDeadline) + { + secondContext.Cancel(); + } + // The watchdog exits the child on regression, without hanging on future destruction. + RequireReady(second); + EXPECT_THROW(second.get(), Core::OperationCancelledException); + EXPECT_EQ(first.wait_for(std::chrono::milliseconds(100)), std::future_status::timeout); + EXPECT_FALSE(firstContext.IsCancelled()); + EXPECT_EQ(endpoint->RequestCount(), 1); + EXPECT_EQ(endCalls.load(), 0); + + auto close = std::async(std::launch::async, [&]() { properties.Close({}); }); + RequireReady(first); + EXPECT_THROW(first.get(), Core::OperationCancelledException); + RequireReady(close); + EXPECT_NO_THROW(close.get()); + EXPECT_FALSE(firstContext.IsCancelled()); + properties.Close({}); + EXPECT_EQ(endCalls.load(), 1); + EXPECT_THROW( + properties.GetEventHubsProperties("hub", {}), Core::OperationCancelledException); + EXPECT_THROW( + properties.GetEventHubsPartitionProperties("hub", "0", {}), + Core::OperationCancelledException); + EXPECT_EQ(endCalls.load(), 1); + EXPECT_EQ(endpoint->RequestCount(), 1); + } + server.StopListening(); + } + Amqp::Common::_detail::GlobalStateHolder::GlobalStateInstance()->AssertIdle(); + } + void SuccessfulDiscovery(bool cancelledClose) { std::atomic closeFailureLogged{false}; @@ -331,9 +425,18 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { nullptr, failEnd ? FailEnd : session_end)); EventHubsPropertiesClient properties(session, "hub"); - Azure::Core::Context cancelled; - cancelled.Cancel(); - EXPECT_ANY_THROW(properties.GetEventHubsProperties("hub", cancelled)); + // Enter initialization before cancellation so this still exercises a failed open. + auto context = Azure::Core::Context().WithDeadline( + std::chrono::system_clock::now() + std::chrono::seconds(1)); + try + { + properties.GetEventHubsProperties("hub", context); + ADD_FAILURE() << "Expected management open to fail"; + } + catch (std::runtime_error const& error) + { + EXPECT_STREQ(error.what(), "Could not open Management client"); + } properties.Close({}); properties.Close({}); } @@ -396,6 +499,30 @@ namespace Azure { namespace Messaging { namespace EventHubs { namespace Test { ConcurrentPropertiesCleanupDeathTest, ::testing::Combine(::testing::Bool(), ::testing::Bool())); + class WaitingPropertiesCancellationDeathTest + : public PropertiesCleanupDeathTest, + public ::testing::WithParamInterface> {}; + + TEST_P(WaitingPropertiesCancellationDeathTest, CancelWhileAnotherQueryIsPending) + { + auto const firstPartitionProperties = std::get<0>(GetParam()); + auto const secondPartitionProperties = std::get<1>(GetParam()); + auto const expireDeadline = std::get<2>(GetParam()); + EXPECT_EXIT( + { + CancelWaitingPropertiesQuery( + firstPartitionProperties, secondPartitionProperties, expireDeadline); + std::exit(::testing::Test::HasFailure() ? 1 : 0); + }, + ::testing::ExitedWithCode(0), + ""); + } + + INSTANTIATE_TEST_SUITE_P( + PropertiesAndPartitions, + WaitingPropertiesCancellationDeathTest, + ::testing::Combine(::testing::Bool(), ::testing::Bool(), ::testing::Bool())); + TEST_F(PropertiesCleanupDeathTest, ManagementCloseFailureStillEndsSession) { EXPECT_EXIT( From d76d0bb6e955011a0c31f8c459bb2f8347108e9a Mon Sep 17 00:00:00 2001 From: Johnathan W Date: Wed, 4 Nov 2026 15:53:13 -0500 Subject: [PATCH 11/11] docs: remove standalone Event Hubs cleanup test guide --- doc/EventHubsCleanupTests.md | 98 ------------------- sdk/core/azure-core-amqp/README.md | 2 - .../azure-messaging-eventhubs/README.md | 1 - 3 files changed, 101 deletions(-) delete mode 100644 doc/EventHubsCleanupTests.md diff --git a/doc/EventHubsCleanupTests.md b/doc/EventHubsCleanupTests.md deleted file mode 100644 index fd8255b3a0..0000000000 --- a/doc/EventHubsCleanupTests.md +++ /dev/null @@ -1,98 +0,0 @@ -# Event Hubs cleanup regression tests - -These tests cover producer cleanup after partition discovery fails and cleanup while a properties query is running. They verify that management clients, sessions, and connection polling references are released safely, including when initialization or shutdown throws. - -The tests use the uAMQP backend and local loopback listeners. They do not require Azure credentials, a live broker, or the HTTP test proxy. See [CONTRIBUTING.md](https://github.com/Azure/azure-sdk-for-cpp/blob/main/CONTRIBUTING.md) for compiler and dependency setup. - -Properties-query cancellation and synchronization with cleanup apply only to uAMQP. The Rust backend retains its initialization-only lock because pending Rust management calls do not observe child-context cancellation. Rust cancellation, recovery, and concurrent cleanup are tracked in [azure-sdk-for-rust#5527](https://github.com/Azure/azure-sdk-for-rust/issues/5527). - -## Build and run - -Run from the repository root, adding any toolchain or platform options required by your development environment: - -```sh -cmake -S . -B build/cleanup \ - -DCMAKE_BUILD_TYPE=Debug \ - -DBUILD_TESTING=ON \ - -D AZURE_EVENTHUBS_CTEST_TIMEOUT=60 \ - -DDISABLE_RUST_IN_BUILD=ON - -cmake --build build/cleanup --config Debug --target \ - azure-core-amqp-tests azure-messaging-eventhubs-test - -ctest --test-dir build/cleanup -C Debug -N \ - -R 'SessionCleanupDeathTest|PropertiesCleanupDeathTest|WaitingPropertiesCancellationDeathTest' - -ctest --test-dir build/cleanup -C Debug \ - -R 'SessionCleanupDeathTest|PropertiesCleanupDeathTest|WaitingPropertiesCancellationDeathTest' \ - --output-on-failure --timeout 60 -``` - -The configure option sets the Event Hubs test timeout to 60 seconds; CTest's `--timeout` alone does not override an explicit per-test timeout. - -The discovery command should list **28 cases**: four session cases, eight properties cleanup cases, eight parameterized query/close cases, and eight parameterized waiting-query cancellation cases. Both test files are guarded by `ENABLE_UAMQP`. The default Rust backend does not include them; a successful run with no matching tests does not validate this cleanup behavior. - -To repeat just the concurrency cases five times: - -```sh -ctest --test-dir build/cleanup -C Debug \ - -R 'ConcurrentPropertiesCleanupDeathTest|WaitingPropertiesCancellationDeathTest' \ - --repeat until-fail:5 --output-on-failure --timeout 60 -``` - -## Session ownership and shutdown - -Source: [session_cleanup_tests.cpp](https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/core/azure-core-amqp/test/ut/session_cleanup_tests.cpp), fixture `SessionCleanupDeathTest`. - -| Test | Behavior checked | -| --- | --- | -| `FailedEndReleasesLastOwner` | A native shutdown failure still throws. A management client keeps the session alive until its last owner releases it; the session and connection then expire and polling becomes idle. | -| `FailedEndWithErrorReleasesLastOwner` | The same ownership guarantees hold for the `End` overload that accepts an AMQP error condition and description. | -| `FailedEndCanBeRetried` | A failed `End` can be retried successfully through the other overload. A further `End` fails rather than releasing polling ownership twice. | -| `MissingEndStillAsserts` | Destroying a begun session without ever calling `End` still terminates the child process. The existing lifecycle contract remains enforced. | - -## Properties initialization and producer cleanup - -Source: [properties_cleanup_test.cpp](https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/eventhubs/azure-messaging-eventhubs/test/ut/properties_cleanup_test.cpp), fixture `PropertiesCleanupDeathTest`. - -| Test | Behavior checked | -| --- | --- | -| `SuccessfulDiscoveryAndRepeatedClose` | Hub and partition properties are returned correctly, and repeated close ends the session exactly once. | -| `ManagementCloseFailureStillEndsSession` | A canceled management close logs its failure, but session cleanup still runs exactly once. | -| `CreationFailureThenRepeatedClose` | Management creation throws after session begin. Retrying creation preserves the original error and does not acquire another polling reference; repeated close releases ownership. | -| `CreationFailureThenDestruction` | Destruction performs the same partial-initialization cleanup without an explicit close. | -| `CreationFailureAndSessionEndFailure` | Management creation and native session shutdown both fail; final destruction still releases the owned resources. | -| `OpenFailureThenRepeatedClose` | Management open fails after assignment; repeated close safely cleans the partially initialized client. | -| `OpenFailureAndSessionEndFailure` | An open failure followed by a native session shutdown failure does not cause a destruction assertion. | -| `ProducerDiscoveryFailureThenRepeatedClose` | A mock management endpoint returns a service error. The public producer reports the discovery failure and survives repeated close and destruction. | - -## Concurrent queries and cleanup - -The same source contains `ConcurrentPropertiesCleanupDeathTest`, instantiated as `PropertiesAndPartitions`. Each test runs for both getters and with caller cancellation either absent or already requested: two tests times two getters times two cancellation states gives eight cases. - -| Test | Synchronization and expected outcome | -| --- | --- | -| `CloseDuringPendingQuery` | The mock endpoint signals receipt but withholds the response. The query has no deadline. Close must cancel its child operation context and finish without waiting indefinitely for the server. | -| `CloseBeforeManagementSend` | A synchronous log callback pauses execution inside `ExecuteOperation`, before sending. While paused, close must remain pending and the session-end counter must stay zero. After release, the query and close must finish safely. | - -Both tests verify that cleanup ends the session once, preserves the caller's cancellation state, rejects later queries through the closed properties-client reference, and leaves global polling idle. The parameter tuple is `(partitionProperties, cancelCaller)`: indices 0 through 3 correspond to `(false, false)`, `(false, true)`, `(true, false)`, and `(true, true)`. - -The second test depends on the management log messages containing `ManagementClient::ExecuteOperation:` and `Create Queue for request.`. If those diagnostics change, update the gate as well. The gate uses a bounded wait so a missing signal fails the child instead of hanging the suite. - -## Cancellation while waiting for another query - -`WaitingPropertiesCancellationDeathTest.CancelWhileAnotherQueryIsPending` establishes query A by waiting for the mock endpoint to receive its management request. The endpoint withholds the response, and A has no deadline. Query B starts with an independent context and must throw `OperationCancelledException` after explicit cancellation or deadline expiry while A remains pending. - -The test checks that only A's management request reaches the endpoint, A's caller context remains uncancelled, and no session end occurs before close. Closing afterward must terminate A, end the session exactly once, reject queries through stale references, and leave global polling idle. A ten-second watchdog exits the child if an operation fails to finish, preventing a blocked future destructor from hanging the regression. - -The parameter tuple is `(firstPartitionProperties, secondPartitionProperties, expireDeadline)`. All eight combinations cover both getters independently for A and B and both cancellation modes. The queries retain the management lock through operation completion; only the uAMQP lock-acquisition wait checks cancellation between timed attempts. - -## Why these are process tests - -Except for `MissingEndStillAsserts`, the tests use `EXPECT_EXIT` and require a zero child exit status. An exception assertion alone would miss the original crash: a later destructor can abort after the exception has already been caught. The fixtures use Google Test's `threadsafe` death-test style to re-execute the child without inheriting polling-thread mutexes. - -The [cleanup listener](https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/core/azure-core-amqp/test/ut/cleanup_test_listener.hpp) accepts TCP connection attempts without creating a peer AMQP session. This isolates injected creation and session-end failures from mock-server teardown. Discovery and concurrency cases use the AMQP mock server because they need management links and requests. Weak ownership checks, an atomic session-end counter, and `GlobalStateHolder::AssertIdle()` check resource lifetime in addition to process survival. - -The concurrency gate reproduced process termination and an AddressSanitizer heap-use-after-free before synchronization was added. For Google Test 1.12 on Linux ARM64 under AddressSanitizer, set `GTEST_DEATH_TEST_USE_FORK=1` to avoid the small clone-child stack overflowing before the SDK code runs. - -These local tests establish SDK cleanup behavior. They do not replace Windows teardown validation, live service tests, or the AUB discovery-failure recovery workload. The Blob Checkpoint Store recordings refreshed alongside this fix exercise separate, existing HTTP playback tests. diff --git a/sdk/core/azure-core-amqp/README.md b/sdk/core/azure-core-amqp/README.md index 4affddcde4..17c138df32 100644 --- a/sdk/core/azure-core-amqp/README.md +++ b/sdk/core/azure-core-amqp/README.md @@ -77,8 +77,6 @@ while (messageSendCount < maxMessageSendCount) You can build and run the tests locally by executing `azure-core-amqp-test`. Explore the `test` folder to see advanced usage and behavior of the public classes. -For session shutdown failures and Event Hubs properties cleanup, see the [cleanup regression test guide](https://github.com/Azure/azure-sdk-for-cpp/blob/main/doc/EventHubsCleanupTests.md). It describes the ownership checks and how to enable and run the uAMQP process tests. - ### Run broker-backed tests Some tests in this package need a live AMQP broker. They use the `TestAmqpBroker` from [Azure/azure-amqp][azure_amqp]. The default build sets `USE_RUST_AMQP`, so these tests exercise the Rust AMQP stack. The setup script clones the commit that `Test-Setup.ps1` pins, restores through the `nuget.cfsclean.config` in this directory, builds the `net10.0` target, and waits for the broker to accept TCP connections. diff --git a/sdk/eventhubs/azure-messaging-eventhubs/README.md b/sdk/eventhubs/azure-messaging-eventhubs/README.md index b72714e8ff..f33bcccac2 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/README.md +++ b/sdk/eventhubs/azure-messaging-eventhubs/README.md @@ -275,7 +275,6 @@ Many people all over the world have helped make this project better. You'll wan * [What are some good first issues for new contributors to the repo?](https://github.com/azure/azure-sdk-for-cpp/issues?q=is%3Aopen+is%3Aissue+label%3A%22up+for+grabs%22) * [How to build and test your change][azure_sdk_for_cpp_contributing_developer_guide] -* [Event Hubs cleanup regression tests](https://github.com/Azure/azure-sdk-for-cpp/blob/main/doc/EventHubsCleanupTests.md) * [How you can make a change happen!][azure_sdk_for_cpp_contributing_pull_requests] * Frequently Asked Questions (FAQ) and Conceptual Topics in the detailed [Azure SDK for C++ wiki](https://github.com/azure/azure-sdk-for-cpp/wiki).