fix(core-amqp): carry the cause of a CBS open failure to the caller

sdk/core/azure-core-amqp/CHANGELOG.md

@@ -36,6 +36,8 @@
 - A claims based security open that fails now throws `CbsOpenFailedException`, which carries the `CbsOpenResult`. The three failures need different handling: `Error` reached the transport and may be retried, while `Cancelled` is the caller's own cancellation or deadline and `Invalid` is a state error. The result was previously readable only by matching the message text, so a reword would have changed caller behavior with no compiler error. The type derives from `std::runtime_error` and carries the same message, so existing handlers keep working. The Rust backend reports every open failure by throwing rather than by returning a result, so those throws are classified at the shared call site and carry the same type.
 - The uAMQP management client now closes the message sender when the message receiver fails to open. Two handlers returned a status without that close, and a message sender that stays open stops the process in its own destructor.
 - The uAMQP management client now names the management node and the open status in the lines that it writes when an open fails, and it keeps the text of the exception that ended the open. The message sender open failure moved from the Error level to the Warning level, because that call reports the failure to its caller.
+- A claims based security open that fails now carries the reason that the layer below reported. `CbsOpenResult::Error` covers every transport, TLS and link failure, so a reader holding only the result could not separate a refused socket from a rejected attach. The management client wrote that reason to the log and then dropped it, because `ManagementClientImpl::Open` reports a failure as a status and the exception that named the cause was destroyed in the handler. The reason now travels with the status and reaches both the warning and the `CbsOpenFailedException` message, so a caller that logs the exception and has no log listener can still tell what failed. It names which of the two links failed, because the sender and the receiver fail for different causes. The reason is empty when the layer below gave none, and the sentence then reads exactly as it did before. It never holds the token.
+- The claims based security object now keeps the AMQP error that the service sent. `ClaimsBasedSecurityImpl::OnError` receives the condition, the description, and the info map, which is the richest statement the service makes about a refused claim, and it only wrote them to the log. They are now added to the reason that the open failure carries. The capture takes a lock of its own, because that callback runs on the polling thread while the management client holds its open lock.
 
 ### Other Changes
 

sdk/core/azure-core-amqp/src/amqp/connection.cpp

@@ -152,6 +152,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
       auto claimsBasedSecurity = std::make_shared<ClaimsBasedSecurityImpl>(session);
       auto const openStart = std::chrono::steady_clock::now();
       CbsOpenResult cbsOpenStatus{CbsOpenResult::Invalid};
+      // The reason the layer below reported. `CbsOpenResult::Error` covers every transport, TLS
+      // and link failure, so the status alone does not say what went wrong. Issue: a caller that
+      // saw only the status could not separate a refused socket from a rejected handshake.
+      std::string openFailureDetail;
       try
       {
         cbsOpenStatus = claimsBasedSecurity->Open(context);
@@ -169,6 +173,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
         // contract on both backends. A cancelled context is the one case a caller must not retry,
         // and it is the only distinction available at this point.
         cbsOpenStatus = context.IsCancelled() ? CbsOpenResult::Cancelled : CbsOpenResult::Error;
+        openFailureDetail = ex.what();
         Log::Stream(Logger::Level::Warning)
             << "The claims based security open threw: " << ex.what();
       }
@@ -177,16 +182,22 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
         auto const elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
             std::chrono::steady_clock::now() - openStart);
         auto const connection = session->GetConnection();
+        if (openFailureDetail.empty())
+        {
+          openFailureDetail = claimsBasedSecurity->GetOpenFailureDetail();
+        }
         Log::Stream(Logger::Level::Warning) << FormatCbsOpenFailureLog(
             cbsOpenStatus,
             audienceUrl,
             tokenType,
             expiresOn,
             caller,
             connection ? connection->GetDiagnosticSummary() : std::string{},
-            elapsed);
+            elapsed,
+            openFailureDetail);
         throw CbsOpenFailedException(
-            cbsOpenStatus, DescribeCbsOpenFailure(cbsOpenStatus, audienceUrl, caller));
+            cbsOpenStatus,
+            DescribeCbsOpenFailure(cbsOpenStatus, audienceUrl, caller, openFailureDetail));
       }
 
       try

sdk/core/azure-core-amqp/src/amqp/private/cbs_open_failure.hpp

@@ -66,14 +66,30 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
 
   // The sentence that the caller reads in the exception. Neither this function
   // nor the one below takes the token, so no failure text can hold a secret.
+  //
+  // `detail` is the reason the layer below reported. `CbsOpenResult::Error` covers every
+  // transport, TLS and link failure, so without the reason a reader cannot tell a refused socket
+  // from a rejected handshake. It is empty when that layer produced no reason, and the sentence
+  // then reads exactly as it did before.
   inline std::string DescribeCbsOpenFailure(
       CbsOpenResult result,
       std::string const& audienceUrl,
-      CbsOpenCaller caller)
+      CbsOpenCaller caller,
+      std::string const& detail = {})
   {
     std::stringstream ss;
     ss << "Could not open Claims Based Security object. Result: " << result
-       << ", audience: " << audienceUrl << ", caller: " << CbsOpenCallerName(caller) << ".";
+       << ", audience: " << audienceUrl << ", caller: " << CbsOpenCallerName(caller);
+    if (!detail.empty())
+    {
+      ss << ", reason: " << detail;
+    }
+    // The reason comes from a layer below and often ends in its own full stop. A second one
+    // reads as a typo in the customer's log.
+    if (detail.empty() || detail.back() != '.')
+    {
+      ss << ".";
+    }
     return ss.str();
   }
 
@@ -88,10 +104,11 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
       Azure::DateTime const& expiresOn,
       CbsOpenCaller caller,
       std::string const& connectionSummary,
-      std::chrono::milliseconds elapsed)
+      std::chrono::milliseconds elapsed,
+      std::string const& detail = {})
   {
     std::stringstream ss;
-    ss << DescribeCbsOpenFailure(result, audienceUrl, caller)
+    ss << DescribeCbsOpenFailure(result, audienceUrl, caller, detail)
        << " Token type: " << CbsTokenTypeName(tokenType)
        << ", token expires: " << FormatTokenExpiry(expiresOn)
        << ". Connection: " << (connectionSummary.empty() ? "unknown" : connectionSummary)

sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/claim_based_security.cpp

@@ -54,6 +54,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
       return CbsOpenResult::Ok;
     }
   }
+
+  // This backend reports every open failure by throwing and returns Ok otherwise, so the shared
+  // call site reads the reason from the exception and never asks for one here.
+  std::string ClaimsBasedSecurityImpl::GetOpenFailureDetail() const { return {}; }
   void ClaimsBasedSecurityImpl::Close(Context const& context)
   {
     Common::_detail::CallContext callContext(

sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/claims_based_security_impl.hpp

@@ -38,6 +38,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
     ClaimsBasedSecurityImpl& operator=(ClaimsBasedSecurityImpl&&) noexcept = delete;
 
     _azure_NODISCARD CbsOpenResult Open(Context const& context);
+
+    /** @brief The reason the last `Open` failed, or an empty string when it did not fail. */
+    std::string GetOpenFailureDetail() const;
+
     void Close(Context const& context);
     _azure_NODISCARD std::tuple<CbsOperationResult, uint32_t, std::string> PutToken(
         CbsTokenType type,

sdk/core/azure-core-amqp/src/impl/uamqp/amqp/claim_based_security.cpp

@@ -38,9 +38,11 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
       auto rv{m_management->Open(context)};
       if (rv != ManagementOpenStatus::Ok)
       {
+        auto const detail = m_management->GetOpenFailureDetail();
         Log::Stream(Logger::Level::Warning)
             << "ClaimsBasedSecurityImpl::Open: the $cbs management client did not open. Status: "
-            << ManagementOpenStatusName(rv) << ".";
+            << ManagementOpenStatusName(rv) << "."
+            << (detail.empty() ? std::string{} : " Reason: " + detail + ".");
       }
       switch (rv)
       {
@@ -67,6 +69,39 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
 
   void ClaimsBasedSecurityImpl::Close(Context const& context) { m_management->Close(context); }
 
+  // The management client holds the reason that its own layer produced. The AMQP error that the
+  // service sent is richer, because it names the condition, the description, and the info map, so
+  // it is added when one arrived. Either part may be absent.
+  std::string ClaimsBasedSecurityImpl::GetOpenFailureDetail() const
+  {
+    std::string detail{m_management ? m_management->GetOpenFailureDetail() : std::string{}};
+
+    Models::_internal::AmqpError lastError;
+    {
+      std::lock_guard<std::mutex> lock(m_errorLock);
+      lastError = m_lastError;
+    }
+    if (lastError)
+    {
+      std::stringstream ss;
+      if (!detail.empty())
+      {
+        ss << detail << "; ";
+      }
+      ss << "the service reported condition: " << lastError.Condition.ToString()
+         << ", description: " << lastError.Description;
+      // The info map carries the fields that make a condition actionable, such as the
+      // network-host and port of a redirect. It is usually empty, so it is only added when the
+      // service sent one.
+      if (!lastError.Info.empty())
+      {
+        ss << ", info: " << lastError.Info;
+      }
+      return ss.str();
+    }
+    return detail;
+  }
+
   std::tuple<CbsOperationResult, uint32_t, std::string> ClaimsBasedSecurityImpl::PutToken(
       CbsTokenType tokenType,
       std::string const& audience,
@@ -183,6 +218,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
   void ClaimsBasedSecurityImpl::OnError(Models::_internal::AmqpError const& error)
   {
     Log::Stream(Logger::Level::Warning) << "AMQP Error processing ClaimsBasedSecurity: " << error;
+    // This is the only place the service's own condition and description reach this object. A
+    // caller that reads the exception and nothing else would otherwise never see them.
+    std::lock_guard<std::mutex> lock(m_errorLock);
+    m_lastError = error;
   }
 
 }}}} // namespace Azure::Core::Amqp::_detail

sdk/core/azure-core-amqp/src/impl/uamqp/amqp/management.cpp

@@ -90,6 +90,12 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
     }
   }
 
+  std::string ManagementClientImpl::GetOpenFailureDetail() const
+  {
+    std::lock_guard<std::mutex> lock(m_openCloseLock);
+    return m_openFailureDetail;
+  }
+
   _internal::ManagementOpenStatus ManagementClientImpl::Open(Context const& context)
   {
     std::unique_lock<std::mutex> lock(m_openCloseLock);
@@ -98,6 +104,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
       throw std::runtime_error("Management object is already open.");
     }
 
+    // A retry reuses this object, so a reason left by an earlier attempt must not be read as the
+    // reason for this one.
+    m_openFailureDetail.clear();
+
     try
     {
       /** Authentication needs to happen *before* the links are created.
@@ -139,6 +149,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
         auto senderResult{m_messageSender->Open(false, context)};
         if (senderResult)
         {
+          std::stringstream detail;
+          detail << "the message sender for node '" << m_options.ManagementNodeName
+                 << "' did not open: " << senderResult;
+          m_openFailureDetail = detail.str();
           Log::Stream(Logger::Level::Warning)
               << "ManagementClientImpl::Open: Message sender open failed. Node: "
               << m_options.ManagementNodeName << ". Error: " << senderResult << ".";
@@ -153,13 +167,21 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
       // stays open stops the process in its own destructor, so close it here.
       catch (Azure::Core::OperationCancelledException const& e)
       {
+        // m_messageSenderOpen is set only after the sender open returned, and the receiver open
+        // is the next statement, so it names which of the two threw.
+        m_openFailureDetail = std::string("the message ")
+            + (m_messageSenderOpen ? "receiver" : "sender") + " open was cancelled: " + e.what();
         Log::Stream(Logger::Level::Warning)
             << "Operation cancelled opening message sender and receiver." << e.what();
         CloseSenderAndReceiverAfterFailedOpen();
         return _internal::ManagementOpenStatus::Cancelled;
       }
       catch (std::runtime_error const& e)
       {
+        // This is the reason a reader needs. It names the transport, TLS or link failure that
+        // made the open fail, and the status alone cannot carry it.
+        m_openFailureDetail = std::string("the message ")
+            + (m_messageSenderOpen ? "receiver" : "sender") + " open threw: " + e.what();
         Log::Stream(Logger::Level::Warning)
             << "Exception thrown opening message sender and receiver." << e.what();
         CloseSenderAndReceiverAfterFailedOpen();
@@ -174,9 +196,21 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
         _internal::ManagementOpenStatus rv = std::get<0>(*result);
         if (rv != _internal::ManagementOpenStatus::Ok)
         {
+          // The handler that completed the queue knows which link failed and what state it
+          // entered. Fall back to the status only when it gave nothing.
+          auto queuedDetail = std::get<1>(*result);
+          if (queuedDetail.empty())
+          {
+            std::stringstream detail;
+            detail << "the open completed with status " << ManagementOpenStatusName(rv)
+                   << " for node '" << m_options.ManagementNodeName << "'";
+            queuedDetail = detail.str();
+          }
+          m_openFailureDetail = queuedDetail;
           Log::Stream(Logger::Level::Warning)
               << "Management operation failed to open. Node: " << m_options.ManagementNodeName
-              << ". Status: " << ManagementOpenStatusName(rv) << ".";
+              << ". Status: " << ManagementOpenStatusName(rv) << ". Reason: " << queuedDetail
+              << ".";
           m_messageSender->Close(context);
           m_messageSenderOpen = false;
           m_messageReceiver->Close(context);
@@ -191,6 +225,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
 
       // If result is null, then it means that the context was cancelled. Close the things we opened
       // earlier (if any) and return the error.
+      m_openFailureDetail = "the caller's context was cancelled while the open was in flight";
       m_messageSender->Close({});
       m_messageSenderOpen = false;
       m_messageReceiver->Close({});
@@ -199,8 +234,12 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
     }
     catch (...)
     {
+      // This handler rethrows, so the caller reads the reason from the exception itself. The
+      // reason is recorded anyway so both paths out of Open leave it set.
+      auto const exceptionText = CurrentExceptionText();
+      m_openFailureDetail = "the management open threw: " + exceptionText;
       Log::Stream(Logger::Level::Warning)
-          << "Exception thrown during management open. " << CurrentExceptionText();
+          << "Exception thrown during management open. " << exceptionText;
       // If an exception is thrown, ensure that the message sender and receiver are closed.
       if (m_messageSenderOpen)
       {
@@ -446,22 +485,27 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
             if (m_messageReceiverOpen)
             {
               SetState(ManagementState::Open);
-              m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Ok);
+              m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Ok, {});
             }
             break;
             // If the message sender is transitioning to an error or state other than open,
             // it's an error.
           default:
           case _internal::MessageSenderState::Idle:
           case _internal::MessageSenderState::Closing:
-          case _internal::MessageSenderState::Error:
+          case _internal::MessageSenderState::Error: {
             Log::Stream(Logger::Level::Warning)
                 << "Message Sender Changed State to " << newState
                 << " while management client is opening"
                 << ". Node: " << m_options.ManagementNodeName << ".";
             SetState(ManagementState::Closing);
-            m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Error);
+            std::stringstream detail;
+            detail << "the message sender for node '" << m_options.ManagementNodeName
+                   << "' moved to " << newState << " while the management client was opening";
+            m_openCompleteQueue.CompleteOperation(
+                _internal::ManagementOpenStatus::Error, detail.str());
             break;
+          }
         }
         break;
       case ManagementState::Open:
@@ -571,22 +615,27 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail {
             if (m_messageSenderOpen)
             {
               SetState(ManagementState::Open);
-              m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Ok);
+              m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Ok, {});
             }
             break;
             // If the message receiver is transitioning to an error or state other than open,
             // it's an error.
           default:
           case _internal::MessageReceiverState::Idle:
           case _internal::MessageReceiverState::Closing:
-          case _internal::MessageReceiverState::Error:
+          case _internal::MessageReceiverState::Error: {
             Log::Stream(Logger::Level::Warning)
                 << "Message Receiver Changed State to " << newState
                 << " while management client is opening"
                 << ". Node: " << m_options.ManagementNodeName << ".";
             SetState(ManagementState::Closing);
-            m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Error);
+            std::stringstream detail;
+            detail << "the message receiver for node '" << m_options.ManagementNodeName
+                   << "' moved to " << newState << " while the management client was opening";
+            m_openCompleteQueue.CompleteOperation(
+                _internal::ManagementOpenStatus::Error, detail.str());
             break;
+          }
         }
         break;
       case ManagementState::Open: