Fix premature global idle when a parked Sync waiter becomes runnable

Pool::get_task honoured the pending_idle latch by firing an idle epoch
before attempting to dequeue. When a parked external waiter (e.g. a task
blocked on a Sync group's token) becomes runnable and is drained into the
pool's queue, firing idle up-front dropped the pool's active count and
released its active_pools slot while a runnable task was still queued,
letting a global idle epoch fire prematurely.

This reorders idle-driven reactions and, with shutdown-on-idle, can
quiesce the powerplant while real work is pending. It manifested in the
NUbots Director, which dispatches provider reactions onto the default
pool while still holding its Sync<Director> token; the re-entrant
provider parks, and on token release the premature idle reordered the
Director's idle-driven steps.

Consume the pending_idle latch without firing idle: its only job is to
wake the worker so it re-checks its queue. The existing dequeue-first /
!got path then decides correctly - a drained-runnable waiter is dequeued
and run (no idle), while a still-parked waiter leaves the queue empty so
the !got branch fires idle exactly as before (preserving cross-pool
idle-wake / deadlock-break behaviour).

Add IdleDirectorPingPong regression test reproducing the Director
topology; it fails deterministically before the fix and passes after.

Author: TrentHouliston

src/threading/scheduler/Pool.cpp

@@ -336,28 +336,30 @@ namespace threading {
                     continue;
                 }
 
-                // If a waiter was parked for this pool since the last time this worker looked,
-                // ensure we fire one idle epoch before dispatching the next task. This is the
-                // counterpart of the OLD scheduler behaviour where a parked task with a failing
-                // group lock sat in the pool queue and forced the worker to poll-fail-and-fall-
-                // through to get_idle_task; in the fast path the task is parked in the Group's
-                // wait_buckets instead, so without this latch the worker can be preempted long
-                // enough for the drained (lock-OK) task to arrive in the queue before the worker
-                // polls and end up running it directly, swallowing the idle fire.
+                // A waiter was parked for this pool since the last time this worker looked, so it
+                // set pending_idle to wake us. Consume the latch here, but do NOT fire an idle
+                // epoch up-front: the latch's only job is to WAKE this worker so it re-checks its
+                // queue. Whether this is actually an idle situation must be decided by the normal
+                // dequeue-first path below.
                 //
-                // get_idle_task() is a no-op when this thread is already idle (local_lock set),
-                // so a wasted consume here is harmless: the worker just falls through to the
-                // normal dequeue path below.
+                // This matters for the case where the parked waiter has since become runnable and
+                // been drained into this pool's queue (e.g. a Sync group released its token). If we
+                // fired get_idle_task() here, before try_dequeue_task(), we would drop this pool's
+                // "active" count to zero and release its active_pools slot while a runnable task is
+                // still sitting in the queue. That can let a GLOBAL idle epoch fire even though real
+                // work is pending (premature idle) - which reorders idle-driven reactions and, with
+                // shutdown-on-idle, can quiesce the powerplant early.
                 //
-                // The relaxed load short-circuits the (more expensive) read-modify-write on the
+                // By only consuming the latch here and letting the dequeue-first / !got path below
+                // decide, a drained-runnable waiter is dequeued and run (no idle), while a still-
+                // parked waiter leaves the queue empty so the !got branch fires idle exactly as
+                // before (preserving the cross-pool idle-wake / deadlock-break behaviour).
+                //
+                // The relaxed-ish load short-circuits the (more expensive) read-modify-write on the
                 // common path where nothing has been latched, so a busy worker never pays for the
                 // exclusive cacheline acquire that exchange() would force every iteration.
-                if (pending_idle.load(std::memory_order_acquire)
-                    && pending_idle.exchange(false, std::memory_order_acq_rel)) {
-                    auto idle_task = get_idle_task();
-                    if (idle_task.task != nullptr) {
-                        return idle_task;
-                    }
+                if (pending_idle.load(std::memory_order_acquire)) {
+                    pending_idle.exchange(false, std::memory_order_acq_rel);
                 }
 
                 bool got = false;

tests/tests/dsl/IdleDirectorPingPong.cpp

@@ -0,0 +1,128 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2024 NUClear Contributors
+ *
+ * This file is part of the NUClear codebase.
+ * See https://github.com/Fastcode/NUClear for further info.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
+ * documentation files (the "Software"), to deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
+ * WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <catch2/catch_message.hpp>
+#include <catch2/catch_test_macros.hpp>
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "nuclear"
+#include "test_util/diff_string.hpp"
+#include "util/precise_sleep.hpp"
+
+// Reproduces the NUbots Director premature-idle failure.
+//
+// The Director dispatches provider work onto the default pool *while still holding its own
+// Sync<Director> token* (it runs on Pool<Director> at REALTIME). The dispatched provider then
+// re-enters the Sync<Director> group from the default pool. Because the token is still held, that
+// re-entrant task is PARKED as an external waiter on the director pool, which arms the pool's
+// "pending_idle" latch.
+//
+// When the director reaction finishes it releases the token and drains the parked (now-runnable)
+// task into its own queue. But if Pool::get_task honours the pending_idle latch *before* dequeuing
+// that drained task, it fires a (global) idle epoch even though a runnable reaction is queued. That
+// premature global idle lets the test's idle-driven step advance while real work is still pending,
+// producing an out-of-order event sequence.
+
+namespace {
+
+struct DirectorPool {
+    static constexpr int concurrency = 1;
+};
+struct DirectorSync {};
+
+struct Kick {};
+struct ReEnter {};
+struct Step1 {};
+struct Step2 {};
+
+std::vector<std::string> events;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
+
+class TestReactor : public NUClear::Reactor {
+public:
+    explicit TestReactor(std::unique_ptr<NUClear::Environment> environment) : Reactor(std::move(environment)) {
+
+        // Each global idle advances the step machine by one. Step 1 kicks the director, then once
+        // the director/provider ping-pong is done the next idle advances to step 2, and a final
+        // idle shuts the powerplant down.
+        on<Idle<>>().then([this] {
+            switch (++step) {
+                case 1: emit(std::make_unique<Step1>()); break;
+                case 2: emit(std::make_unique<Step2>()); break;
+                default: powerplant.shutdown(); break;
+            }
+        });
+
+        on<Trigger<Kick>, Sync<DirectorSync>, Pool<DirectorPool>, Priority::REALTIME>().then([this] {
+            events.push_back("director");
+            // Dispatch the re-entrant "provider" while STILL holding the Sync token so it parks.
+            emit(std::make_unique<ReEnter>());
+            // Hold the token briefly so the re-entrant task is definitely parked as an external waiter.
+            NUClear::util::precise_sleep(std::chrono::milliseconds(1));
+        });
+
+        on<Trigger<ReEnter>, Sync<DirectorSync>, Pool<DirectorPool>, Priority::REALTIME>().then([this] {
+            // If global idle fired prematurely (while this drained task was queued), Step2 will have
+            // been emitted already; this small delay lets that record first, exposing the ordering
+            // violation deterministically.
+            NUClear::util::precise_sleep(std::chrono::milliseconds(5));
+            events.push_back("provider");
+        });
+
+        on<Trigger<Step1>, Priority::LOW>().then([this] {
+            events.push_back("step 1");
+            emit(std::make_unique<Kick>());
+        });
+
+        on<Trigger<Step2>, Priority::LOW>().then([this] { events.push_back("step 2"); });
+    }
+
+private:
+    int step = 0;
+};
+
+}  // namespace
+
+TEST_CASE("Test global idle does not fire while a parked Sync waiter is pending (Director premature idle)",
+          "[api][dsl][Idle][Pool][Sync][Director]") {
+
+    events.clear();
+
+    NUClear::Configuration config;
+    config.default_pool_concurrency = 1;
+    NUClear::PowerPlant powerplant(config);
+    powerplant.install<TestReactor>();
+    powerplant.start();
+
+    const std::vector<std::string> expected = {
+        "step 1",
+        "director",
+        "provider",
+        "step 2",
+    };
+
+    INFO(test_util::diff_string(expected, events));
+
+    REQUIRE(events == expected);
+}