Add DeviceController deleteFromFabricTableOnShutdown option (project-chip#32846)

* Add DeviceController deleteFromFabricTableOnShutdown option

This is analogous to removeFromFabricTableOnShutdown but does an actual
Delete(), i.e. affects storage, rather than just a Forget().

Use this to simplify controller shutdown on Darwin.

* Address review comments

Author: ksperling-apple

src/controller/CHIPDeviceController.cpp

@@ -136,6 +136,7 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params)
     mState       = State::Initialized;
 
     mRemoveFromFabricTableOnShutdown = params.removeFromFabricTableOnShutdown;
+    mDeleteFromFabricTableOnShutdown = params.deleteFromFabricTableOnShutdown;
 
     if (GetFabricIndex() != kUndefinedFabricIndex)
     {
@@ -382,8 +383,9 @@ void DeviceController::Shutdown()
 
     VerifyOrReturn(mState != State::NotInitialized);
 
+    // If our state is initialialized it means mSystemState is valid,
+    // and we can use it below before we release our reference to it.
     ChipLogDetail(Controller, "Shutting down the controller");
-
     mState = State::NotInitialized;
 
     if (mFabricIndex != kUndefinedFabricIndex)
@@ -401,13 +403,13 @@ void DeviceController::Shutdown()
         // existing sessions too?
         mSystemState->SessionMgr()->ExpireAllSessionsForFabric(mFabricIndex);
 
-        if (mRemoveFromFabricTableOnShutdown)
+        if (mDeleteFromFabricTableOnShutdown)
         {
-            FabricTable * fabricTable = mSystemState->Fabrics();
-            if (fabricTable != nullptr)
-            {
-                fabricTable->Forget(mFabricIndex);
-            }
+            mSystemState->Fabrics()->Delete(mFabricIndex);
+        }
+        else if (mRemoveFromFabricTableOnShutdown)
+        {
+            mSystemState->Fabrics()->Forget(mFabricIndex);
         }
     }
 

src/controller/CHIPDeviceController.h

@@ -126,15 +126,27 @@ struct ControllerInitParams
 
     /**
      * Controls whether shutdown of the controller removes the corresponding
-     * entry from the fabric table.  For now the removal is just from the
-     * in-memory table, not from storage, which means that after controller
-     * shutdown the storage and the in-memory fabric table will be out of sync.
-     * This is acceptable for implementations that don't actually store any of
-     * the fabric table information, but if someone wants a true removal at some
-     * point another option will need to be added here.
+     * entry from the in-memory fabric table, but NOT from storage.
+     *
+     * Note that this means that after controller shutdown the storage and
+     * in-memory versions of the fabric table will be out of sync.
+     * For compatibility reasons this is the default behavior.
+     *
+     * @see deleteFromFabricTableOnShutdown
      */
     bool removeFromFabricTableOnShutdown = true;
 
+    /**
+     * Controls whether shutdown of the controller deletes the corresponding
+     * entry from the fabric table (both in-memory and storage).
+     *
+     * If both `removeFromFabricTableOnShutdown` and this setting are true,
+     * this setting will take precedence.
+     *
+     * @see removeFromFabricTableOnShutdown
+     */
+    bool deleteFromFabricTableOnShutdown = false;
+
     /**
      * Specifies whether to utilize the fabric table entry for the given FabricIndex
      * for initialization. If provided and neither the operational key pair nor the NOC
@@ -394,6 +406,7 @@ class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController
     FabricIndex mFabricIndex = kUndefinedFabricIndex;
 
     bool mRemoveFromFabricTableOnShutdown = true;
+    bool mDeleteFromFabricTableOnShutdown = false;
 
     FabricTable::AdvertiseIdentity mAdvertiseIdentity = FabricTable::AdvertiseIdentity::Yes;
 

src/controller/CHIPDeviceControllerFactory.cpp

@@ -305,6 +305,7 @@ void DeviceControllerFactory::PopulateInitParams(ControllerInitParams & controll
     controllerParams.controllerRCAC                       = params.controllerRCAC;
     controllerParams.permitMultiControllerFabrics         = params.permitMultiControllerFabrics;
     controllerParams.removeFromFabricTableOnShutdown      = params.removeFromFabricTableOnShutdown;
+    controllerParams.deleteFromFabricTableOnShutdown      = params.deleteFromFabricTableOnShutdown;
 
     controllerParams.systemState        = mSystemState;
     controllerParams.controllerVendorId = params.controllerVendorId;

src/controller/CHIPDeviceControllerFactory.h

@@ -93,15 +93,27 @@ struct SetupParams
 
     /**
      * Controls whether shutdown of the controller removes the corresponding
-     * entry from the fabric table.  For now the removal is just from the
-     * in-memory table, not from storage, which means that after controller
-     * shutdown the storage and the in-memory fabric table will be out of sync.
-     * This is acceptable for implementations that don't actually store any of
-     * the fabric table information, but if someone wants a true removal at some
-     * point another option will need to be added here.
+     * entry from the in-memory fabric table, but NOT from storage.
+     *
+     * Note that this means that after controller shutdown the storage and
+     * in-memory versions of the fabric table will be out of sync.
+     * For compatibility reasons this is the default behavior.
+     *
+     * @see deleteFromFabricTableOnShutdown
      */
     bool removeFromFabricTableOnShutdown = true;
 
+    /**
+     * Controls whether shutdown of the controller deletes the corresponding
+     * entry from the fabric table (both in-memory and storage).
+     *
+     * If both `removeFromFabricTableOnShutdown` and this setting are true,
+     * this setting will take precedence.
+     *
+     * @see removeFromFabricTableOnShutdown
+     */
+    bool deleteFromFabricTableOnShutdown = false;
+
     /**
      * Specifies whether to utilize the fabric table entry for the given FabricIndex
      * for initialization. If provided and neither the operational key pair nor the NOC

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -509,11 +509,15 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
         }
         commissionerParams.controllerVendorId = static_cast<chip::VendorId>([startupParams.vendorID unsignedShortValue]);
         commissionerParams.enableServerInteractions = startupParams.advertiseOperational;
-        // We don't want to remove things from the fabric table on controller
-        // shutdown, since our controller setup depends on being able to fetch
-        // fabric information for the relevant fabric indices on controller
-        // bring-up.
+
+        // We never want plain "removal" from the fabric table since this leaves
+        // the in-memory state out of sync with what's in storage. In per-controller
+        // storage mode, have the controller delete itself from the fabric table on shutdown.
+        // In factory storage mode we need to keep fabric information around so we can
+        // start another controller on that existing fabric at a later time.
         commissionerParams.removeFromFabricTableOnShutdown = false;
+        commissionerParams.deleteFromFabricTableOnShutdown = (startupParams.storageDelegate != nil);
+
         commissionerParams.permitMultiControllerFabrics = startupParams.allowMultipleControllersPerFabric;
 
         // Set up our attestation verifier.  Assume we want to use the default

src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm

@@ -1082,22 +1082,6 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller
         }
 
         sharedCleanupBlock();
-
-        // Now that our per-controller storage for the controller being shut
-        // down is guaranteed to be disconnected, go ahead and clean up the
-        // fabric table entry for the controller if we're in per-controller
-        // storage mode.
-        if (self->_usingPerControllerStorage) {
-            // We have to use a new fabric table to do this cleanup, because
-            // our system state is gone now.
-            FabricTable fabricTable;
-            CHIP_ERROR err = [self _initFabricTable:fabricTable];
-            if (err != CHIP_NO_ERROR) {
-                MTR_LOG_ERROR("Failed to clean up fabric entries.  Expect things to act oddly: %" CHIP_ERROR_FORMAT, err.Format());
-            } else {
-                fabricTable.Delete(controllerFabricIndex);
-            }
-        }
     } else {
         // Do the controller shutdown on the Matter work queue.
         dispatch_sync(_chipWorkQueue, ^{
@@ -1106,18 +1090,6 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller
             }
 
             sharedCleanupBlock();
-
-            // Now that our per-controller storage for the controller being shut
-            // down is guaranteed to be disconnected, go ahead and clean up the
-            // fabric table entry for the controller if we're in per-controller
-            // storage mode.
-            if (self->_usingPerControllerStorage) {
-                // Make sure to delete controllerFabricIndex from the system state's
-                // fabric table.  We know there's a system state here, because we
-                // still have a running controller.
-                auto * systemState = _controllerFactory->GetSystemState();
-                systemState->Fabrics()->Delete(controllerFabricIndex);
-            }
         });
     }
 

src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.mm

@@ -104,20 +104,19 @@
 {
     assertChipStackLockedByCurrentThread();
 
-    // NOTE: During controller startup, the Matter SDK thinks that the cert for
-    // a fabric index is changing and hence that it must remove session
-    // resumption data for that fabric index.  For us that does not matter,
-    // since we don't key that data on fabric index anyway.  But we do want to
-    // avoid doing this delete-on-startup so we can actually store session
-    // resumption data persistently.
+    // NOTE: During controller startup and shutdown, the SDK DeviceControllerFactory
+    // calls this method from ClearCASEResumptionStateOnFabricChange() due to fabric
+    // update or removal.  For us that does not matter, since we don't key resumption
+    // data on fabric index anyway.  But we do want to avoid executing the DeleteAll
+    // so we can actually store session resumption data persistently.
 
     // And that is the only use of DeleteAll for controllers in practice, in the
     // situations where we are using MTRSessionResumptionStorageBridge at all.
     // So just no-op this function, but verify that our assumptions hold.
     auto * controller = [mFactory runningControllerForFabricIndex:fabricIndex
                                       includeControllerStartingUp:NO
-                                    includeControllerShuttingDown:YES];
-    VerifyOrDieWithMsg(controller == nil, Controller, "Deleting resumption storage for controller outside startup");
+                                    includeControllerShuttingDown:NO];
+    VerifyOrDieWithMsg(controller == nil, Controller, "ResumptionStorage::DeleteAll called for running controller");
     return CHIP_NO_ERROR;
 }