Fix commissioning with a node ID we've used before to not reuse CASE …

…sessions.

This was coming up in some cert tests, where we commission a device, then
factory reset it, then try to commission it again with the same node id.  When
we went to send CommissioningComplete for the second commissioning, we would try
to reuse the existing CASE session (to the pre-reset device), which obviously
failed.

The fix is to just ensure we have no existing sessions to the relevant node id
before we start CASE setup.

Also adds a unit test for this case.  This caught a bug in the test harness:
factoryReset was clearing the KVS of running processes without restarting them,
which is not how factory reset would normally work.

scripts/tests/chiptest/test_definition.py

@@ -71,9 +71,15 @@ def stop(self):
         return False
 
     def factoryReset(self):
+        wasRunning = (not self.killed) and self.stop()
+
         for kvs in self.kvsPathSet:
             if os.path.exists(kvs):
                 os.unlink(kvs)
+
+        if wasRunning:
+            return self.start()
+
         return True
 
     def waitForAnyAdvertisement(self):

src/app/tests/suites/TestSystemCommands.yaml

@@ -21,6 +21,9 @@ config:
     payload:
         type: char_string
         defaultValue: "MT:-24J0IX4122-.548G00" # This value needs to be generated
+    secondNodeId:
+        type: int64u
+        defaultValue: 0xDEADBEEF
 
 tests:
     - label: "Wait for the commissioned device to be retrieved"
@@ -114,7 +117,7 @@ tests:
       arguments:
           values:
               - name: "nodeId"
-                value: 0xDEADBEEF
+                value: secondNodeId
               - name: "payload"
                 value: payload
 
@@ -125,7 +128,7 @@ tests:
       arguments:
           values:
               - name: "nodeId"
-                value: 0xDEADBEEF
+                value: secondNodeId
 
     - label: "Stop the second accessory"
       command: "Stop"
@@ -147,6 +150,29 @@ tests:
               - name: "registerKey"
                 value: "chip-lock-app"
 
+    # NOTE: Since we have a new KVS, this is basically a factory reset, so we can commission again.
+    - label: "Commission second accessory with new KVS from alpha"
+      identity: "alpha"
+      cluster: "CommissionerCommands"
+      command: "PairWithCode"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: secondNodeId
+              - name: "payload"
+                value: payload
+
+    - label:
+          "Wait for the second commissioned device with new KVS to be retrieved
+          for alpha"
+      identity: "alpha"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: secondNodeId
+
     - label: "Reboot the default accessory"
       command: "Reboot"
 
@@ -180,3 +206,25 @@ tests:
           values:
               - name: "registerKey"
                 value: "chip-lock-app"
+
+    - label: "Commission the now-reset second accessory from alpha"
+      identity: "alpha"
+      cluster: "CommissionerCommands"
+      command: "PairWithCode"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: secondNodeId
+              - name: "payload"
+                value: payload
+
+    - label:
+          "Wait for the second commissioned device (after reset) to be retrieved
+          for alpha"
+      identity: "alpha"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: secondNodeId

src/controller/CHIPDeviceController.cpp

@@ -2764,6 +2764,15 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
     case CommissioningStage::kFindOperational: {
         // If there is an error, CommissioningStageComplete will be called from OnDeviceConnectionFailureFn.
         auto scopedPeerId = GetPeerScopedId(proxy->GetDeviceId());
+
+        // If we ever had a commissioned device with this node ID before, we may
+        // have stale sessions to it.  Make sure we don't re-use any of those,
+        // because clearly they are not related to this new device we are
+        // commissioning.  We only care about sessions we might reuse, so just
+        // clearing the ones associated with our fabric index is good enough and
+        // we don't need to worry about ExpireAllSessionsOnLogicalFabric.
+        mSystemState->SessionMgr()->ExpireAllSessions(scopedPeerId);
+
         mSystemState->CASESessionMgr()->FindOrEstablishSession(scopedPeerId, &mOnDeviceConnectedCallback,
                                                                &mOnDeviceConnectionFailureCallback
 #if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES