Fix premature global idle when a parked Sync waiter becomes runnable

[TrentHouliston]

Summary

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 still pending.

How it manifested

It was found via the NUbots Director, which dispatches provider reactions onto the default pool while still holding its Sync token. The re-entrant provider parks as an external waiter (arming pending_idle); on token release the parked task is drained into the pool's queue (now runnable), but the stale latch fires idle first, reordering the Director's idle-driven steps and (with shutdown-on-idle) causing early quiescence/hangs.

The fix

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),
• 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).

Regression test

tests/tests/dsl/IdleParkedSyncWaiter.cpp reproduces the topology that triggered the bug (a REALTIME reaction on a concurrency-1 pool holding a Sync token while re-entering it, driven by deterministic on<Trigger<Step<N>>> steps), generalised away from any Director-specific naming. It fails deterministically before the fix and passes after:

• Without fix: PASS=0 FAIL=30
• With fix: PASS=30 FAIL=30

Full local ctest suite passes.

[Copilot]

Pull request overview

This PR fixes a scheduler edge case in NUClear’s thread pool where a latched pending_idle signal could trigger an idle epoch before attempting to dequeue newly-runnable work, allowing a premature global idle while runnable tasks are still queued.

Changes:

• Reorders Pool::get_task() handling so pending_idle is consumed as a wakeup hint without firing idle up-front, deferring the idle decision to the dequeue/!got path.
• Adds a regression test that reproduces the “Director” ping-pong topology and asserts correct ordering under idle-driven progression.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
src/threading/scheduler/Pool.cpp	Stops firing idle epochs pre-dequeue when pending_idle is latched, preventing premature global idle.
tests/tests/dsl/IdleDirectorPingPong.cpp	Adds a regression test covering the parked-waiter-becomes-runnable idle ordering scenario.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

[TrentHouliston]

🤖 Follow-up: found and fixed a real regression this change had introduced in the dsl/IdleSingle test (it was flaking ~15% of the time under a Debug build; the pre-fix baseline flaked 0/100).

Root cause: the original fix deferred the entire get_idle_task() — both the LOCAL per-pool active check and the GLOBAL active_pools check — behind try_dequeue_task(). But the LOCAL active counter is edge-triggered via CountingLock (it only succeeds on the exact transition to zero). Deferring it meant a fleeting active-reaches-zero window could be missed whenever unrelated work landed in the queue that same instant: the dequeue would succeed, skip the idle check, and active might never read exactly zero again for that epoch → a missed Idle<MainThread> fire (idle_calls[i] == 0).

The LOCAL and GLOBAL idle checks have opposite timing requirements: LOCAL must fire eagerly (edge-triggered), GLOBAL must stay deferred (to avoid the premature-global-idle bug this PR fixes).

Fix: split get_idle_task() into get_local_idle_task() (LOCAL only) and the full version. The LOCAL idle is now fired eagerly right after consuming pending_idle — safe with respect to premature global idle because it only ever touches this pool's own Idle<ThisPool> reactions and never scheduler.active_pools, so it cannot release a global idle slot early. The GLOBAL check stays deferred to the dequeue-first !got path exactly as before.

Validation (Debug): IdleSingle 0/200, IdleParkedSyncWaiter (the regression test) 0/50, full ctest 66/66. The shared idle-dispatch boilerplate is factored into collect_local_idle_reactions() / make_idle_dispatch_task() helpers.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/threading/scheduler/Pool.hpp:334

• The pending_idle field comment claims the fast path (when no idle reactions are present) "compiles down to a couple of relaxed atomic loads", but the surrounding code uses at least acquire loads (e.g. idle_relevant() uses memory_order_acquire). This is slightly misleading documentation; consider wording it more generally as "atomic loads" (or mention acquire) to match the implementation.

            /// This is only ever set when idle_relevant() is true (some idle reaction could fire
            /// on this pool), so on the hot contended path with no idle reactions the latch stays
            /// false and the whole mechanism compiles down to a couple of relaxed atomic loads.