Fix the wrong check of running thread

src/common/service/NetRemoteService.cxx

@@ -707,12 +707,20 @@ NetRemoteService::WifiAccessPointTimedEnableImpl(std::string_view accessPointId,
     // Right now, this service only supports managing singale access point.
     {
         std::lock_guard<std::mutex> lock(m_threadsMutex);
-        if (m_timedEnableThread && m_timedEnableThread->joinable()) {
+        if (m_timedEnableRunning.load()) {
             wifiOperationStatus.set_code(WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeOperationNotSupported);
             wifiOperationStatus.set_message("A timed enable operation is already in progress");
             return wifiOperationStatus;
         }
 
+        // Join previous thread if it exists and is joinable
+        if (m_timedEnableThread && m_timedEnableThread->joinable()) {
+            m_timedEnableThread->join();
+        }
+
+        // Set the running flag before creating the thread
+        m_timedEnableRunning = true;
+
         // Create and store the timer thread
         auto timerThread = std::make_shared<std::thread>([this, accessPointId = std::string(accessPointId), hasConfiguration = (dot11AccessPointConfiguration != nullptr), configurationCopy = dot11AccessPointConfiguration ? *dot11AccessPointConfiguration : Dot11AccessPointConfiguration{}, accessPointController, durationSeconds]() {
             // Sleep for the specified duration, checking every second for shutdown signal
@@ -725,6 +733,7 @@ NetRemoteService::WifiAccessPointTimedEnableImpl(std::string_view accessPointId,
             // If we were shutdown, exit early
             if (m_shutdown.load()) {
                 LOGI << std::format("Timed enable operation for access point {} was cancelled due to service shutdown", accessPointId);
+                m_timedEnableRunning = false;
                 return;
             }
 
@@ -741,6 +750,9 @@ NetRemoteService::WifiAccessPointTimedEnableImpl(std::string_view accessPointId,
                     accessPointId,
                     durationSeconds);
             }
+
+            // Clear the running flag when the operation completes
+            m_timedEnableRunning = false;
         });
 
         m_timedEnableThread = timerThread;
@@ -773,12 +785,20 @@ NetRemoteService::WifiAccessPointTimedDisableImpl(std::string_view accessPointId
     // Right now, this service only supports managing singale access point.
     {
         std::lock_guard<std::mutex> lock(m_threadsMutex);
-        if (m_timedDisableThread && m_timedDisableThread->joinable()) {
+        if (m_timedDisableRunning.load()) {
             wifiOperationStatus.set_code(WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeOperationNotSupported);
             wifiOperationStatus.set_message("A timed disable operation is already in progress");
             return wifiOperationStatus;
         }
 
+        // Join previous thread if it exists and is joinable
+        if (m_timedDisableThread && m_timedDisableThread->joinable()) {
+            m_timedDisableThread->join();
+        }
+
+        // Set the running flag before creating the thread
+        m_timedDisableRunning = true;
+
         // Create and store the timer thread
         auto timerThread = std::make_shared<std::thread>([this, accessPointId = std::string(accessPointId), accessPointController, durationSeconds]() {
             // Sleep for the specified duration, checking every second for shutdown signal
@@ -791,6 +811,7 @@ NetRemoteService::WifiAccessPointTimedDisableImpl(std::string_view accessPointId
             // If we were shutdown, exit early
             if (m_shutdown.load()) {
                 LOGI << std::format("Timed disable operation for access point {} was cancelled due to service shutdown", accessPointId);
+                m_timedDisableRunning = false;
                 return;
             }
 
@@ -806,6 +827,9 @@ NetRemoteService::WifiAccessPointTimedDisableImpl(std::string_view accessPointId
                     accessPointId,
                     durationSeconds);
             }
+
+            // Clear the running flag when the operation completes
+            m_timedDisableRunning = false;
         });
 
         m_timedDisableThread = timerThread;

src/common/service/include/microsoft/net/remote/service/NetRemoteService.hxx

@@ -387,6 +387,8 @@ private:
     // Thread management for timed operations
     std::mutex m_threadsMutex;
     std::atomic<bool> m_shutdown{ false };
+    std::atomic<bool> m_timedEnableRunning{ false };
+    std::atomic<bool> m_timedDisableRunning{ false };
     std::shared_ptr<std::thread> m_timedEnableThread;
     std::shared_ptr<std::thread> m_timedDisableThread;
 };

tests/unit/TestNetRemoteServiceClient.cxx

@@ -5,6 +5,7 @@
 #include <memory>
 #include <string>
 #include <string_view>
+#include <thread>
 #include <utility>
 
 #include <catch2/catch_test_macros.hpp>
@@ -1349,6 +1350,39 @@ TEST_CASE("WifiAccessPointTimedEnable API", "[basic][rpc][client][remote][timed]
         REQUIRE(result2.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeOperationNotSupported);
         REQUIRE(result2.status().message() == "A timed enable operation is already in progress");
     }
+
+    SECTION("Succeeds when a timed enable operation is finished")
+    {
+        // Start first timed enable operation with short duration
+        WifiAccessPointTimedEnableRequest request1{};
+        request1.set_accesspointid(InterfaceName1);
+        request1.set_durationseconds(1); // 1 second duration
+
+        WifiAccessPointTimedEnableResult result1{};
+        grpc::ClientContext clientContext1{};
+
+        auto status1 = client->WifiAccessPointTimedEnable(&clientContext1, request1, &result1);
+        REQUIRE(status1.ok());
+        REQUIRE(result1.has_status());
+        REQUIRE(result1.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeSucceeded);
+
+        // Wait for the first operation to complete
+        std::this_thread::sleep_for(std::chrono::seconds(2));
+
+        // Start second timed enable operation after the first has finished
+        WifiAccessPointTimedEnableRequest request2{};
+        request2.set_accesspointid(InterfaceName2);
+        request2.set_durationseconds(1); // 1 second duration
+
+        WifiAccessPointTimedEnableResult result2{};
+        grpc::ClientContext clientContext2{};
+
+        auto status2 = client->WifiAccessPointTimedEnable(&clientContext2, request2, &result2);
+        REQUIRE(status2.ok());
+        REQUIRE(result2.has_status());
+        REQUIRE(result2.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeSucceeded);
+        REQUIRE(result2.status().message().empty());
+    }
 }
 
 TEST_CASE("WifiAccessPointTimedDisable API", "[basic][rpc][client][remote][timed]")
@@ -1482,4 +1516,39 @@ TEST_CASE("WifiAccessPointTimedDisable API", "[basic][rpc][client][remote][timed
         REQUIRE(result2.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeOperationNotSupported);
         REQUIRE(result2.status().message() == "A timed disable operation is already in progress");
     }
+
+    SECTION("Succeeds when a timed disable operation is finished")
+    {
+        // Start first timed disable operation with short duration
+        WifiAccessPointTimedDisableRequest request1{};
+        request1.set_accesspointid(InterfaceName1);
+        request1.set_durationseconds(1); // 1 second duration
+
+        WifiAccessPointTimedDisableResult result1{};
+        grpc::ClientContext clientContext1{};
+
+        grpc::Status status1;
+        REQUIRE_NOTHROW(status1 = client->WifiAccessPointTimedDisable(&clientContext1, request1, &result1));
+        REQUIRE(status1.ok());
+        REQUIRE(result1.has_status());
+        REQUIRE(result1.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeSucceeded);
+
+        // Wait for the first operation to complete
+        std::this_thread::sleep_for(std::chrono::seconds(2));
+
+        // Start second timed disable operation after the first has finished
+        WifiAccessPointTimedDisableRequest request2{};
+        request2.set_accesspointid(InterfaceName2);
+        request2.set_durationseconds(1); // 1 second duration
+
+        WifiAccessPointTimedDisableResult result2{};
+        grpc::ClientContext clientContext2{};
+
+        grpc::Status status2;
+        REQUIRE_NOTHROW(status2 = client->WifiAccessPointTimedDisable(&clientContext2, request2, &result2));
+        REQUIRE(status2.ok());
+        REQUIRE(result2.has_status());
+        REQUIRE(result2.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeSucceeded);
+        REQUIRE(result2.status().message().empty());
+    }
 }