Mark CASE sessions as defunct when we don't get expected responses/ac…

…ks. (#20820)

* Mark CASE sessions as defunct when we don't get expected responses/acks.

* Address review comments.

* Don't mark PASE sessions defunct.

* Add automated test

examples/chip-tool/commands/tests/TestCommand.cpp

@@ -41,7 +41,10 @@ CHIP_ERROR TestCommand::WaitForCommissionee(const char * identity,
     // or is just starting out fresh outright. Let's make sure we're not re-using any cached CASE sessions
     // that will now be stale and mismatched with the peer, causing subsequent interactions to fail.
     //
-    GetCommissioner(identity).SessionMgr()->ExpireAllSessions(chip::ScopedNodeId(value.nodeId, fabricIndex));
+    if (value.expireExistingSession.ValueOr(true))
+    {
+        GetCommissioner(identity).SessionMgr()->ExpireAllSessions(chip::ScopedNodeId(value.nodeId, fabricIndex));
+    }
 
     SetIdentity(identity);
     return GetCommissioner(identity).GetConnectedDevice(value.nodeId, &mOnDeviceConnectedCallback,

examples/darwin-framework-tool/commands/tests/TestCommandBridge.h

@@ -159,9 +159,11 @@ class TestCommandBridge : public CHIPCommandBridge,
         // Invalidate our existing CASE session; otherwise getConnectedDevice
         // will just hand it right back to us without establishing a new CASE
         // session when a reboot is done on the server.
-        if (GetDevice(identity) != nil) {
-            [GetDevice(identity) invalidateCASESession];
-            mConnectedDevices[identity] = nil;
+        if (value.expireExistingSession.ValueOr(true)) {
+            if (GetDevice(identity) != nil) {
+                [GetDevice(identity) invalidateCASESession];
+                mConnectedDevices[identity] = nil;
+            }
         }
 
         [controller getBaseDevice:value.nodeId

examples/darwin-framework-tool/templates/tests/partials/test_cluster.zapt

@@ -195,7 +195,7 @@ class {{filename}}: public TestCommandBridge
 
             {{#chip_tests_item_responses}}
             {{#if error}}
-              VerifyOrReturn(CheckValue("status", err ? err.code : 0, {{error}}));
+              VerifyOrReturn(CheckValue("status", err ? ([err.domain isEqualToString:MTRInteractionErrorDomain] ? err.code : EMBER_ZCL_STATUS_FAILURE) : 0, {{error}}));
               NextTest();
             {{else}}
               VerifyOrReturn(CheckValue("status", err ? err.code : 0, 0));

src/app/OperationalDeviceProxy.cpp

@@ -354,7 +354,7 @@ void OperationalDeviceProxy::OnFirstMessageDeliveryFailed()
 
 void OperationalDeviceProxy::OnSessionHang()
 {
-    // TODO: establish a new session
+    Disconnect();
 }
 
 void OperationalDeviceProxy::ShutdownSubscriptions()

src/app/tests/suites/TestCASERecovery.yaml

@@ -0,0 +1,75 @@
+# Copyright (c) 2022 Project CHIP Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+name: Test for CASE recovery
+
+config:
+    nodeId: 0x12344321
+    cluster: "Basic"
+    endpoint: 0
+
+tests:
+    - label: "Wait for the commissioned device to be retrieved"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: nodeId
+
+    # Make sure we have a working CASE session.
+    - label: "Read an attribute"
+      command: "readAttribute"
+      attribute: "DataModelRevision"
+      response:
+          value: 1
+
+    - label: "Reboot the server"
+      cluster: "SystemCommands"
+      command: "Reboot"
+
+    - label: "Re-get our session, but without expiring sesssions"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: nodeId
+              - name: "expireExistingSession"
+                value: false
+
+    # Try to do something with the CASE session.  This should time out, but
+    # mark the CASE session as defunct.
+    - label: "Read an attribute again"
+      command: "readAttribute"
+      attribute: "DataModelRevision"
+      response:
+          error: FAILURE
+
+    - label: "Re-get our session, but without expiring sesssions"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: nodeId
+              - name: "expireExistingSession"
+                value: false
+
+    # Now trying to do another read should create a new CASE session successfully.
+    - label: "Read an attribute a third time"
+      command: "readAttribute"
+      attribute: "DataModelRevision"
+      response:
+          value: 1

src/app/tests/suites/tests.js

@@ -817,6 +817,7 @@ function getTests() {
     ];
 
     const Others = [
+        "TestCASERecovery",
         "TestCluster",
         "TestClusterComplexTypes",
         "TestConstraints",

src/app/zap-templates/common/simulated-clusters/clusters/DelayCommands.js

@@ -35,7 +35,7 @@ const WaitForCommissioning = {
 
 const WaitForCommissionee = {
   name : 'WaitForCommissionee',
-  arguments : [ { type : 'NODE_ID', name : 'nodeId' } ],
+  arguments : [ { type : 'NODE_ID', name : 'nodeId' }, { type : 'BOOLEAN', name : 'expireExistingSession', isOptional : true } ],
 };
 
 const WaitForMessage = {

src/messaging/ExchangeContext.cpp

@@ -449,6 +449,17 @@ void ExchangeContext::NotifyResponseTimeout(bool aCloseIfNeeded)
 {
     SetResponseExpected(false);
 
+    // mSession might be null if this timeout is due to the session being
+    // evicted.
+    if (mSession)
+    {
+        if (mSession->IsSecureSession() && mSession->AsSecureSession()->IsCASESession())
+        {
+            mSession->AsSecureSession()->MarkAsDefunct();
+        }
+        mSession->DispatchSessionEvent(&SessionDelegate::OnSessionHang);
+    }
+
     ExchangeDelegate * delegate = GetDelegate();
 
     // Call the user's timeout handler.

src/messaging/ExchangeContext.h

@@ -195,6 +195,15 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
     // SecureSession.
     void AbortAllOtherCommunicationOnFabric();
 
+    /**
+     *  Determine whether a response is currently expected for a message that was sent over
+     *  this exchange.  While this is true, attempts to send other messages that expect a response
+     *  will fail.
+     *
+     *  @return Returns 'true' if response expected, else 'false'.
+     */
+    bool IsResponseExpected() const;
+
 private:
     class ExchangeSessionHolder : public SessionHolderWithDelegate
     {
@@ -212,15 +221,6 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
     ExchangeSessionHolder mSession; // The connection state
     uint16_t mExchangeId;           // Assigned exchange ID.
 
-    /**
-     *  Determine whether a response is currently expected for a message that was sent over
-     *  this exchange.  While this is true, attempts to send other messages that expect a response
-     *  will fail.
-     *
-     *  @return Returns 'true' if response expected, else 'false'.
-     */
-    bool IsResponseExpected() const;
-
     /**
      * Determine whether we are expecting our consumer to send a message on
      * this exchange (i.e. WillSendMessage was called and the message has not

src/messaging/ReliableMessageMgr.cpp

@@ -134,7 +134,19 @@ void ReliableMessageMgr::ExecuteActions()
 
             // Don't check whether the session in the exchange is valid, because when the session is released, the retrans entry is
             // cleared inside ExchangeContext::OnSessionReleased, so the session must be valid if the entry exists.
-            entry->ec->GetSessionHandle()->DispatchSessionEvent(&SessionDelegate::OnSessionHang);
+            SessionHandle session = entry->ec->GetSessionHandle();
+
+            // If the exchange is expecting a response, it will handle sending
+            // this notification once it detects that it has not gotten a
+            // response.  Otherwise, we need to do it.
+            if (!entry->ec->IsResponseExpected())
+            {
+                if (session->IsSecureSession() && session->AsSecureSession()->IsCASESession())
+                {
+                    session->AsSecureSession()->MarkAsDefunct();
+                }
+                session->DispatchSessionEvent(&SessionDelegate::OnSessionHang);
+            }
 
             // Do not StartTimer, we will schedule the timer at the end of the timer handler.
             mRetransTable.ReleaseObject(entry);

src/transport/Session.h

@@ -103,9 +103,13 @@ class Session
 
     void DispatchSessionEvent(SessionDelegate::Event event)
     {
-        for (auto & holder : mHolders)
+        // Holders might remove themselves when notified.
+        auto holder = mHolders.begin();
+        while (holder != mHolders.end())
         {
-            holder.DispatchSessionEvent(event);
+            auto cur = holder;
+            ++holder;
+            cur->DispatchSessionEvent(event);
         }
     }