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]

🤖 Good catch - reverted this back to pending_idle.exchange(false, std::memory_order_acq_rel) (from the plain store suggested in an earlier review pass). Keeping the consume as an atomic RMW preserves the intended symmetry with register_external_waiter()'s exchange(true) producer-side check, so a concurrent waiter parking right as the worker consumes the latch can't have its signal silently dropped. Added a comment explaining why this must stay an exchange. Verified the fix with a full local ctest run (66/66 passing) plus 20 repeated runs of the deterministic regression test.

[TrentHouliston]

🤖 Correction to my previous reply: switching to exchange while still discarding its return value doesn't actually add any protection over store - both just place false into the same total modification order for this atomic, so there's no observable difference between them here. My earlier reply overclaimed.

Reverted back to pending_idle.store(false, std::memory_order_release) and rewrote the comment to explain the actual reason this is safe: submit() (when a drained task is enqueued) and unregister_external_waiter() (when external_waiters returns to 0) both call notify_one()/notify_all() unconditionally, under the pool's mutex, on every state transition the wait predicate actually depends on. pending_idle's own notify (in register_external_waiter(), gated to the 0→1 transition) is purely a latency optimization to promptly wake an already-sleeping worker - if it's ever skipped due to a race, correctness is still preserved by those other unconditional notifies. Re-validated with a full local ctest run (66/66 passing).