Don't stop trying BLE if mDNS commissioning fails #fixes 43364

src/controller/SetUpCodePairer.cpp

@@ -468,6 +468,7 @@ bool SetUpCodePairer::ConnectToDiscoveredDevice()
         }
 
         // Failed to start establishing PASE.  Move on to the next item.
+        mCurrentPASEParameters.ClearValue();
         mCurrentPASEPayload.reset();
         PASEEstablishmentComplete();
     }
@@ -520,6 +521,7 @@ void SetUpCodePairer::OnBLEDiscoveryError(CHIP_ERROR err)
     ChipLogError(Controller, "Commissionable node discovery over BLE failed: %" CHIP_ERROR_FORMAT, err.Format());
     mWaitingForDiscovery[kBLETransport] = false;
     LogErrorOnFailure(err);
+    StopPairingIfTransportsExhausted(err);
 }
 #endif // CONFIG_NETWORK_LAYER_BLE
 
@@ -543,6 +545,7 @@ void SetUpCodePairer::OnWifiPAFDiscoveryError(CHIP_ERROR err)
 {
     ChipLogError(Controller, "Commissionable node discovery over Wi-Fi PAF failed: %" CHIP_ERROR_FORMAT, err.Format());
     mWaitingForDiscovery[kWiFiPAFTransport] = false;
+    StopPairingIfTransportsExhausted(err);
 }
 
 void SetUpCodePairer::OnWiFiPAFSubscribeComplete(void * appState)
@@ -578,6 +581,7 @@ void SetUpCodePairer::OnTagDiscoveryFailed(CHIP_ERROR error)
 {
     ChipLogError(Controller, "Commissionable node discovery over NFC failed: %" CHIP_ERROR_FORMAT, error.Format());
     mWaitingForDiscovery[kNFCTransport] = false;
+    StopPairingIfTransportsExhausted(error);
 }
 #endif
 
@@ -712,6 +716,20 @@ bool SetUpCodePairer::DiscoveryInProgress() const
     return false;
 }
 
+void SetUpCodePairer::StopPairingIfTransportsExhausted(CHIP_ERROR err)
+{
+    if (mWaitingForPASE || !mDiscoveredParameters.empty() || DiscoveryInProgress() || mRemoteId == kUndefinedNodeId)
+    {
+        return;
+    }
+    // Clear mRemoteId first to guard against re-entrant calls (e.g. from an async
+    // cancel callback fired after StopAllDiscoveryAttempts already cleared the flags).
+    mRemoteId              = kUndefinedNodeId;
+    CHIP_ERROR failErr     = mLastPASEError != CHIP_NO_ERROR ? mLastPASEError : err;
+    MATTER_LOG_METRIC_END(kMetricSetupCodePairerPairDevice, failErr);
+    mCommissioner->OnSessionEstablishmentError(failErr);
+}
+
 void SetUpCodePairer::StopAllDiscoveryAttempts()
 {
     LogErrorOnFailure(StopDiscoveryOverBLE());
@@ -834,6 +852,16 @@ void SetUpCodePairer::OnPairingComplete(CHIP_ERROR error, const std::optional<Re
             ChipLogError(Controller, "Error when verifying the validity of an address: %" CHIP_ERROR_FORMAT, err.Format());
         }
     }
+    // If this was a DNS-SD-triggered (IP/UDP) PASE attempt, stop DNS-SD now.
+    // All addresses for this device were already queued when DNS-SD found the
+    // record, so continued DNS-SD scanning cannot help.  More importantly,
+    // leaving DNS-SD running would keep DiscoveryInProgress() true indefinitely,
+    // which would prevent StopPairingIfTransportsExhausted from ever firing once
+    // the physical-proximity transports (BLE, Wi-Fi PAF, NFC) also complete.
+    if (mCurrentPASEParameters.HasValue())
+    {
+        LogErrorOnFailure(StopDiscoveryOverDNSSD());
+    }
     mCurrentPASEParameters.ClearValue();
 
     // We failed to establish PASE.  Try the next thing we have discovered, if
@@ -873,20 +901,20 @@ void SetUpCodePairer::OnDeviceDiscoveredTimeoutCallback(System::Layer * layer, v
 {
     ChipLogError(Controller, "Discovery timed out");
     auto * pairer = static_cast<SetUpCodePairer *>(context);
-    pairer->StopAllDiscoveryAttempts();
-    if (!pairer->mWaitingForPASE && pairer->mDiscoveredParameters.empty())
+
+    // If a PASE attempt is in progress, do not stop physical-proximity
+    // transports (BLE, Wi-Fi PAF, NFC) — they have their own completion/timeout
+    // mechanisms.  DNS-SD, however, runs indefinitely, so stop it now to
+    // prevent DiscoveryInProgress() from being true forever.
+    if (pairer->mWaitingForPASE)
     {
-        // We're not waiting on any more PASE attempts, and we're not going to
-        // discover anything at this point, so we should just notify our
-        // listener.
-        CHIP_ERROR err = pairer->mLastPASEError;
-        if (err == CHIP_NO_ERROR)
-        {
-            err = CHIP_ERROR_TIMEOUT;
-        }
-        MATTER_LOG_METRIC_END(kMetricSetupCodePairerPairDevice, err);
-        pairer->mCommissioner->OnSessionEstablishmentError(err);
+        LogErrorOnFailure(pairer->StopDiscoveryOverDNSSD());
+        return;
     }
+
+    // No PASE in progress — stop all remaining discovery and fail if nothing is left to try.
+    pairer->StopAllDiscoveryAttempts();
+    pairer->StopPairingIfTransportsExhausted(CHIP_ERROR_TIMEOUT);
 }
 
 bool SetUpCodePairer::ShouldDiscoverUsing(RendezvousInformationFlag commissioningChannel) const

src/controller/SetUpCodePairer.h

@@ -50,6 +50,13 @@
 #include <vector>
 
 namespace chip {
+
+namespace Testing {
+
+class SetUpCodePairerTestAccess;
+
+} // namespace Testing
+
 namespace Controller {
 
 class DeviceCommissioner;
@@ -98,6 +105,8 @@ class DLL_EXPORT SetUpCodePairer : public DevicePairingDelegate
                                    public Nfc::NFCReaderTransportDelegate
 #endif
 {
+    friend class chip::Testing::SetUpCodePairerTestAccess;
+
 public:
     SetUpCodePairer(DeviceCommissioner * commissioner) : mCommissioner(commissioner) {}
     virtual ~SetUpCodePairer() {}
@@ -180,6 +189,12 @@ class DLL_EXPORT SetUpCodePairer : public DevicePairingDelegate
     // RendezvousParameters in the future.
     bool DiscoveryInProgress() const;
 
+    // If there is nothing left to try (no PASE in progress, no queued discovered
+    // parameters, no discovery in progress), notify the commissioner that pairing
+    // has failed.  err is used as the failure error only if no PASE attempt has
+    // produced an error yet.
+    void StopPairingIfTransportsExhausted(CHIP_ERROR err);
+
     // Not an enum class because we use this for indexing into arrays.
     enum TransportTypes
     {

src/controller/tests/BUILD.gn

@@ -45,6 +45,7 @@ chip_test_suite("tests") {
       test_sources += [
         "TestCommissioningWindowOpener.cpp",
         "TestExampleOperationalCredentialsIssuer.cpp",
+        "TestSetUpCodePairer.cpp",
       ]
     }
   }
@@ -62,6 +63,7 @@ chip_test_suite("tests") {
 
   sources = [
     "AutoCommissionerTestAccess.h",
+    "SetUpCodePairerTestAccess.h",
     "DeviceCommissionerTestAccess.h",
   ]
 

src/controller/tests/SetUpCodePairerTestAccess.h

@@ -0,0 +1,53 @@
+/*
+ *
+ *    Copyright (c) 2025 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    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.
+ */
+#pragma once
+
+#include <controller/SetUpCodePairer.h>
+
+namespace chip {
+namespace Testing {
+
+// Provides access to private members of SetUpCodePairer for testing.
+class SetUpCodePairerTestAccess
+{
+public:
+    SetUpCodePairerTestAccess() = delete;
+    explicit SetUpCodePairerTestAccess(Controller::SetUpCodePairer * pairer) : mPairer(pairer) {}
+
+    // Re-export private transport type indices for use in tests.
+    static constexpr int kBLETransport     = Controller::SetUpCodePairer::kBLETransport;
+    static constexpr int kIPTransport      = Controller::SetUpCodePairer::kIPTransport;
+    static constexpr int kWiFiPAFTransport = Controller::SetUpCodePairer::kWiFiPAFTransport;
+
+    bool GetWaitingForDiscovery(int transport) const { return mPairer->mWaitingForDiscovery[transport]; }
+    void SetWaitingForDiscovery(int transport, bool val) { mPairer->mWaitingForDiscovery[transport] = val; }
+
+    bool GetWaitingForPASE() const { return mPairer->mWaitingForPASE; }
+    void SetWaitingForPASE(bool val) { mPairer->mWaitingForPASE = val; }
+
+    NodeId GetRemoteId() const { return mPairer->mRemoteId; }
+    void SetRemoteId(NodeId id) { mPairer->mRemoteId = id; }
+
+    void FireTimeoutCallback() { Controller::SetUpCodePairer::OnDeviceDiscoveredTimeoutCallback(nullptr, mPairer); }
+
+private:
+    Controller::SetUpCodePairer * mPairer = nullptr;
+};
+
+} // namespace Testing
+} // namespace chip

src/controller/tests/TestSetUpCodePairer.cpp

@@ -0,0 +1,81 @@
+/*
+ *
+ *    Copyright (c) 2025 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    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.
+ */
+
+#include <gtest/gtest.h>
+
+#include <controller/CHIPDeviceController.h>
+#include <controller/SetUpCodePairer.h>
+#include <controller/tests/SetUpCodePairerTestAccess.h>
+#include <lib/core/CHIPError.h>
+
+using namespace chip;
+using namespace chip::Controller;
+using PairerAccess = chip::Testing::SetUpCodePairerTestAccess;
+
+namespace {
+
+class TestSetUpCodePairer : public ::testing::Test
+{
+public:
+    static void SetUpTestSuite() { ASSERT_EQ(Platform::MemoryInit(), CHIP_NO_ERROR); }
+    static void TearDownTestSuite() { Platform::MemoryShutdown(); }
+
+protected:
+    DeviceCommissioner commissioner;
+    SetUpCodePairer pairer{ &commissioner };
+    PairerAccess access{ &pairer };
+};
+
+// When the discovery timeout fires while a PASE attempt is in progress,
+// DNS-SD should be stopped (it runs indefinitely) but other transports
+// (BLE, Wi-Fi PAF, NFC) should be left alone since they self-terminate.
+TEST_F(TestSetUpCodePairer, TimeoutDuringPASE_StopsDNSSD_PreservesOtherTransports)
+{
+    access.SetRemoteId(1);
+    access.SetWaitingForPASE(true);
+    access.SetWaitingForDiscovery(PairerAccess::kIPTransport, true);
+    access.SetWaitingForDiscovery(PairerAccess::kBLETransport, true);
+
+    access.FireTimeoutCallback();
+
+    // DNS-SD must be stopped to prevent DiscoveryInProgress() from being stuck true.
+    EXPECT_FALSE(access.GetWaitingForDiscovery(PairerAccess::kIPTransport));
+    // BLE must be preserved — it may still discover a commissionee.
+    EXPECT_TRUE(access.GetWaitingForDiscovery(PairerAccess::kBLETransport));
+    // PASE state must not be disturbed.
+    EXPECT_TRUE(access.GetWaitingForPASE());
+}
+
+// When the discovery timeout fires with no PASE in progress,
+// all transports should be stopped and failure reported.
+TEST_F(TestSetUpCodePairer, TimeoutNoPASE_StopsAllTransports)
+{
+    access.SetRemoteId(1);
+    access.SetWaitingForPASE(false);
+    access.SetWaitingForDiscovery(PairerAccess::kIPTransport, true);
+    access.SetWaitingForDiscovery(PairerAccess::kBLETransport, true);
+
+    access.FireTimeoutCallback();
+
+    EXPECT_FALSE(access.GetWaitingForDiscovery(PairerAccess::kIPTransport));
+    EXPECT_FALSE(access.GetWaitingForDiscovery(PairerAccess::kBLETransport));
+    // StopPairingIfTransportsExhausted should have reported failure and cleared mRemoteId.
+    EXPECT_EQ(access.GetRemoteId(), kUndefinedNodeId);
+}
+
+} // namespace