Fix restart of controllers that use fabric table storage. (#26004)

Right now controller shutdown clears out the fabric table entry in memory, but
not in storage.  After that memory and storage are out of sync, and various
things that rely on the storage (like starting a new controller for the same
fabric without providing all the certs and whatnot) end up failing.

The fix is to make the Forget() call in controller shutdown configurable, so
that consumers who rely on the storage can skip it.

src/controller/CHIPDeviceController.cpp

@@ -143,6 +143,8 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params)
     mSystemState = params.systemState->Retain();
     mState       = State::Initialized;
 
+    mRemoveFromFabricTableOnShutdown = params.removeFromFabricTableOnShutdown;
+
     if (GetFabricIndex() != kUndefinedFabricIndex)
     {
         ChipLogProgress(Controller,
@@ -348,10 +350,13 @@ void DeviceController::Shutdown()
         // existing sessions too?
         mSystemState->SessionMgr()->ExpireAllSessionsForFabric(mFabricIndex);
 
-        FabricTable * fabricTable = mSystemState->Fabrics();
-        if (fabricTable != nullptr)
+        if (mRemoveFromFabricTableOnShutdown)
         {
-            fabricTable->Forget(mFabricIndex);
+            FabricTable * fabricTable = mSystemState->Fabrics();
+            if (fabricTable != nullptr)
+            {
+                fabricTable->Forget(mFabricIndex);
+            }
         }
     }
 

src/controller/CHIPDeviceController.h

@@ -128,6 +128,17 @@ struct ControllerInitParams
     //
     bool enableServerInteractions = false;
 
+    /**
+     * 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.
+     */
+    bool removeFromFabricTableOnShutdown = true;
+
     chip::VendorId controllerVendorId;
 };
 
@@ -330,6 +341,8 @@ class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController
 
     FabricIndex mFabricIndex = kUndefinedFabricIndex;
 
+    bool mRemoveFromFabricTableOnShutdown = true;
+
     // TODO(cecille): Make this configuarable.
     static constexpr int kMaxCommissionableNodes = 10;
     Dnssd::DiscoveredNodeData mCommissionableNodes[kMaxCommissionableNodes];

src/controller/CHIPDeviceControllerFactory.cpp

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

src/controller/CHIPDeviceControllerFactory.h

@@ -90,6 +90,17 @@ struct SetupParams
     //
     bool enableServerInteractions = false;
 
+    /**
+     * 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.
+     */
+    bool removeFromFabricTableOnShutdown = true;
+
     Credentials::DeviceAttestationVerifier * deviceAttestationVerifier = nullptr;
     CommissioningDelegate * defaultCommissioner                        = nullptr;
 };

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -336,6 +336,11 @@ - (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.
+        commissionerParams.removeFromFabricTableOnShutdown = false;
         commissionerParams.deviceAttestationVerifier = _factory.deviceAttestationVerifier;
 
         auto & factory = chip::Controller::DeviceControllerFactory::GetInstance();

src/darwin/Framework/CHIPTests/MTRControllerTests.m

@@ -391,8 +391,29 @@ - (void)testControllerStartControllersOnTwoFabricIds
     XCTAssertNotNil(controller2);
     XCTAssertTrue([controller2 isRunning]);
 
+    // Verify that we can't start on an existing fabric while we have a
+    // controller on that fabric already.
     XCTAssertNil([factory createControllerOnExistingFabric:params2 error:nil]);
 
+    // Now test restarting the controller on the first fabric while the
+    // controller on the second fabric is still running.
+    [controller1 shutdown];
+    XCTAssertFalse([controller1 isRunning]);
+
+    controller1 = [factory createControllerOnExistingFabric:params1 error:nil];
+    XCTAssertNotNil(controller1);
+    XCTAssertTrue([controller1 isRunning]);
+
+    // Now test restarting the controller on the second fabric while the
+    // controller on the first fabric is still running.
+    [controller2 shutdown];
+    XCTAssertFalse([controller2 isRunning]);
+
+    controller2 = [factory createControllerOnExistingFabric:params2 error:nil];
+    XCTAssertNotNil(controller2);
+    XCTAssertTrue([controller2 isRunning]);
+
+    // Shut down everything.
     [controller1 shutdown];
     XCTAssertFalse([controller1 isRunning]);
 

src/darwin/Framework/CHIPTests/MTRTestStorage.h

@@ -23,6 +23,7 @@ NS_ASSUME_NONNULL_BEGIN
 - (nullable NSData *)storageDataForKey:(NSString *)key;
 - (BOOL)setStorageData:(NSData *)value forKey:(NSString *)key;
 - (BOOL)removeStorageDataForKey:(NSString *)key;
+- (NSString *)dumpStorageToString;
 @end
 
 NS_ASSUME_NONNULL_END

src/darwin/Framework/CHIPTests/MTRTestStorage.m

@@ -52,4 +52,9 @@ - (instancetype)init
     return self;
 }
 
+- (NSString *)dumpStorageToString
+{
+    return [NSString stringWithFormat:@"%@", _values];
+}
+
 @end