[ServerClusterIntrerfaceRegistry] Add consistent behavior on startup failures (project-chip#41924)

* Add consistent behavior on register failures in server cluster interface
registry.

* Fix typo and add comment.

* Update src/app/server-cluster/tests/TestServerClusterInterfaceRegistry.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Add some documentation on error codes from SetContext.

* Update src/app/server-cluster/tests/TestServerClusterInterfaceRegistry.cpp

Co-authored-by: Copilot <[email protected]>

* Update src/app/server-cluster/ServerClusterInterfaceRegistry.cpp

Co-authored-by: Copilot <[email protected]>

* Update src/app/server-cluster/tests/TestServerClusterInterfaceRegistry.cpp

Co-authored-by: Copilot <[email protected]>

* Update src/app/server-cluster/ServerClusterInterfaceRegistry.cpp

Co-authored-by: Copilot <[email protected]>

---------

Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Copilot <[email protected]>

Author: 3 people

src/app/server-cluster/ServerClusterInterfaceRegistry.cpp

@@ -62,7 +62,9 @@ CHIP_ERROR ServerClusterInterfaceRegistry::Register(ServerClusterRegistration &
 
     if (mContext.has_value())
     {
-        ReturnErrorOnFailure(entry.serverClusterInterface->Startup(*mContext));
+        // To preserve similarity with SetContext, do not fail the register even if Startup fails.
+        // This will cause Shutdown to be called for both successful and failed startups.
+        LogErrorOnFailure(entry.serverClusterInterface->Startup(*mContext));
     }
 
     entry.next     = mRegistrations;

src/app/server-cluster/ServerClusterInterfaceRegistry.h

@@ -155,6 +155,11 @@ class ServerClusterInterfaceRegistry
     // Set up the underlying context for all clusters that are managed by this registry.
     //
     // The values within context will be moved and used as-is.
+    //
+    // Returns:
+    //   - CHIP_NO_ERROR on success
+    //   - CHIP_ERROR_HAD_FAILURES if some cluster `Startup` calls had errors (Startup
+    //     will be called for all clusters).
     CHIP_ERROR SetContext(ServerClusterContext && context);
 
     // Invalidates current context.

src/app/server-cluster/tests/TestServerClusterInterfaceRegistry.cpp

@@ -86,6 +86,34 @@ class MultiPathCluster : public DefaultServerCluster
     Span<const ConcreteClusterPath> mActualPaths;
 };
 
+class FailingStartupCluster : public DefaultServerCluster
+{
+public:
+    FailingStartupCluster(const ConcreteClusterPath & path) : DefaultServerCluster(path) {}
+    FailingStartupCluster(EndpointId endpoint, ClusterId cluster) : DefaultServerCluster({ endpoint, cluster }) {}
+
+    DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request,
+                                                AttributeValueEncoder & encoder) override
+    {
+        return CHIP_ERROR_NOT_IMPLEMENTED;
+    }
+
+    CHIP_ERROR Startup(ServerClusterContext & context) override
+    {
+        DefaultServerCluster::Startup(context);
+        mStartupCalls++;
+        return CHIP_ERROR_CANCELLED;
+    }
+    void Shutdown() override { mShutdownCalls++; }
+
+    uint32_t GetStartupCallCount() const { return mStartupCalls; }
+    uint32_t GetShutdownCallCount() const { return mShutdownCalls; }
+
+private:
+    uint32_t mStartupCalls  = 0;
+    uint32_t mShutdownCalls = 0;
+};
+
 struct TestServerClusterInterfaceRegistry : public ::testing::Test
 {
     static void SetUpTestSuite() { ASSERT_EQ(chip::Platform::MemoryInit(), CHIP_NO_ERROR); }
@@ -322,3 +350,53 @@ TEST_F(TestServerClusterInterfaceRegistry, UnregisterErrors)
     // Can't unregister a null cluster.
     EXPECT_EQ(registry.Unregister(nullptr), CHIP_ERROR_NOT_FOUND);
 }
+
+TEST_F(TestServerClusterInterfaceRegistry, StartupShutdownWithoutContext)
+{
+    // register before the context
+    RegisteredServerCluster<FailingStartupCluster> cluster1(kEp1, kCluster1);
+    RegisteredServerCluster<FailingStartupCluster> cluster2(kEp1, kCluster2);
+    RegisteredServerCluster<FailingStartupCluster> cluster3(kEp2, kCluster1);
+
+    {
+        ServerClusterInterfaceRegistry registry;
+
+        // all registrations are ok
+        EXPECT_EQ(registry.Register(cluster1.Registration()), CHIP_NO_ERROR);
+        EXPECT_EQ(registry.Register(cluster2.Registration()), CHIP_NO_ERROR);
+
+        // startup was NOT called (no context yet)
+        EXPECT_EQ(cluster1.Cluster().GetStartupCallCount(), 0u);
+        EXPECT_EQ(cluster2.Cluster().GetStartupCallCount(), 0u);
+
+        TestServerClusterContext context;
+
+        // the clusters are explicitly set to fail startup, so SetContext returns an error.
+        // TODO: is this sane? Register() with a startup failure does NOT return a failure.
+        EXPECT_EQ(registry.SetContext(context.Create()), CHIP_ERROR_HAD_FAILURES);
+
+        // Startup called after registration, with failure that is NOT reported (only logged)
+        EXPECT_EQ(cluster3.Cluster().GetStartupCallCount(), 0u);
+        EXPECT_EQ(registry.Register(cluster3.Registration()), CHIP_NO_ERROR);
+
+        // all startups were called, even though there was an error
+        EXPECT_EQ(cluster1.Cluster().GetStartupCallCount(), 1u);
+        EXPECT_EQ(cluster2.Cluster().GetStartupCallCount(), 1u);
+        EXPECT_EQ(cluster3.Cluster().GetStartupCallCount(), 1u);
+        EXPECT_EQ(cluster1.Cluster().GetShutdownCallCount(), 0u);
+        EXPECT_EQ(cluster2.Cluster().GetShutdownCallCount(), 0u);
+        EXPECT_EQ(cluster3.Cluster().GetShutdownCallCount(), 0u);
+
+        // unregister will call shutdown
+        EXPECT_EQ(registry.Unregister(&cluster2.Cluster()), CHIP_NO_ERROR);
+        EXPECT_EQ(cluster2.Cluster().GetShutdownCallCount(), 1u);
+    }
+
+    // registry deletion will call shutdown for the rest
+    EXPECT_EQ(cluster1.Cluster().GetStartupCallCount(), 1u);
+    EXPECT_EQ(cluster2.Cluster().GetStartupCallCount(), 1u);
+    EXPECT_EQ(cluster3.Cluster().GetStartupCallCount(), 1u);
+    EXPECT_EQ(cluster1.Cluster().GetShutdownCallCount(), 1u);
+    EXPECT_EQ(cluster2.Cluster().GetShutdownCallCount(), 1u);
+    EXPECT_EQ(cluster3.Cluster().GetShutdownCallCount(), 1u);
+}