Address latest Copilot review on PR #193.

Correct the scheduler_data comment (atomic stores are not a data race), cache Pool::current() on the submit hot path, and reflow scheduler.md with semantic line breaks.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: TrentHouliston

docs/explanation/scheduler.md

@@ -2,18 +2,21 @@
 
 This page explains how NUClear's task scheduler works internally — the lock-free queues, thread pools, group tokens, and the path from `emit()` to a running reaction callback.
 
-For the user-facing view of pools, priorities, groups, and idle tasks, see [Threading Model](threading.md). For DSL usage, see the [Scheduling](../reference/dsl/index.md) reference words.
+For the user-facing view of pools, priorities, groups, and idle tasks, see [Threading Model](threading.md).
+For DSL usage, see the [Scheduling](../reference/dsl/index.md) reference words.
 
 ## Role in the system
 
-Every reaction execution is a **task** (`ReactionTask`) submitted to the scheduler. The `PowerPlant` owns a single `Scheduler` instance and forwards all work to it:
+Every reaction execution is a **task** (`ReactionTask`) submitted to the scheduler.
+The `PowerPlant` owns a single `Scheduler` instance and forwards all work to it:
 
 1. A trigger (message emit, timer, IO event, etc.) creates a `ReactionTask`.
 1. `PowerPlant::submit()` calls `Scheduler::submit()`.
 1. The scheduler resolves the target **pool**, acquires any required **group** tokens, and enqueues the task.
 1. A pool worker dequeues the task, runs the callback, and releases group locks when the callback returns.
 
-`PowerPlant::start()` calls `Scheduler::start()`, which starts worker pools and then blocks the calling thread in the **MainThread** pool until shutdown. `PowerPlant::shutdown()` emits the shutdown event and calls `Scheduler::stop()`.
+`PowerPlant::start()` calls `Scheduler::start()`, which starts worker pools and then blocks the calling thread in the **MainThread** pool until shutdown.
+`PowerPlant::shutdown()` emits the shutdown event and calls `Scheduler::stop()`.
 
 ```mermaid
 flowchart LR
@@ -46,7 +49,8 @@ flowchart LR
 
 ### Scheduler
 
-The scheduler is the central coordinator. It:
+The scheduler is the central coordinator.
+It:
 
 - **Owns pools** — lazily created from `ThreadPoolDescriptor` values (default pool, `MainThread`, custom `Pool<T>`, etc.).
 - **Owns groups** — lazily created from `GroupDescriptor` values (`Sync<T>`, `Group<T>`, etc.).
@@ -67,11 +71,13 @@ Each pool is a set of worker threads (or a single thread for `MainThread`) plus:
 
 Workers loop in `Pool::run()`: dequeue a task, call `ReactionTask::run()`, repeat until shutdown.
 
-The default pool's thread count comes from `Configuration::default_pool_concurrency` (typically hardware concurrency). Other pools use the `concurrency` value from their descriptor.
+The default pool's thread count comes from `Configuration::default_pool_concurrency` (typically hardware concurrency).
+Other pools use the `concurrency` value from their descriptor.
 
 ### Group
 
-A group limits how many tasks sharing the same descriptor may run concurrently. `Sync<T>` is a group with concurrency 1.
+A group limits how many tasks sharing the same descriptor may run concurrently.
+`Sync<T>` is a group with concurrency 1.
 
 Groups maintain:
 
@@ -117,31 +123,38 @@ sequenceDiagram
 
 ### Pool resolution cache
 
-The first submit for a reaction calls `get_pool()` under `pools_mutex`. The resulting `Pool*` is stored in `Reaction::scheduler_data` — a plain `std::atomic<Pool*>` rather than `atomic<shared_ptr>` to avoid libstdc++'s hashed mutex pool for atomic shared pointers, which would contend on hot paths.
+The first submit for a reaction calls `get_pool()` under `pools_mutex`.
+The resulting `Pool*` is stored in `Reaction::scheduler_data` — a plain `std::atomic<Pool*>` rather than `atomic<shared_ptr>` to avoid libstdc++'s hashed mutex pool for atomic shared pointers, which would contend on hot paths.
 
-Subsequent submits load the cached pointer with acquire semantics. Concurrent first submits may both resolve the pool; they store the same pointer, so the race is benign.
+Subsequent submits load the cached pointer with acquire semantics.
+Concurrent first submits may both resolve the pool; they store the same pointer, so the race is benign.
 
 ### Inline execution
 
-If a reaction is bound with `Inline` and belongs to a single group, the scheduler tries to acquire a group token and run the callback on the submitting thread without enqueueing. This avoids queue overhead for synchronous emit paths.
+If a reaction is bound with `Inline` and belongs to a single group, the scheduler tries to acquire a group token and run the callback on the submitting thread without enqueueing.
+This avoids queue overhead for synchronous emit paths.
 
 ## Thread pools and queue selection
 
-Each pool holds an array of five `Queue<Task>` instances — one per priority bucket. At construction time the pool chooses the concrete queue type:
+Each pool holds an array of five `Queue<Task>` instances — one per priority bucket.
+At construction time the pool chooses the concrete queue type:
 
 | Pool kind                                                  | Queue type         | Why                                                                                                |
 | ---------------------------------------------------------- | ------------------ | -------------------------------------------------------------------------------------------------- |
 | Default pool (`Pool<>`)                                    | `TaskQueue` (MPMC) | Concurrency may differ from the descriptor's nominal value; multiple workers dequeue concurrently. |
 | `MainThread`, Trace pool, any pool with `concurrency == 1` | `MPSCQueue` (MPSC) | Exactly one consumer; simpler and cheaper than MPMC.                                               |
 | Custom pools with `concurrency > 1`                        | `TaskQueue` (MPMC) | Multiple workers compete for tasks.                                                                |
 
-The virtual `Queue` interface lets `Pool` store both implementations in one `std::array` without templating the entire pool. The virtual call cost is negligible compared to the atomic operations inside enqueue and dequeue.
+The virtual `Queue` interface lets `Pool` store both implementations in one `std::array` without templating the entire pool.
+The virtual call cost is negligible compared to the atomic operations inside enqueue and dequeue.
 
-Workers identify themselves via a thread-local `Pool::current_pool` pointer, set when `run()` starts. `Pool::current()` returns a `shared_ptr` to the active pool, or `nullptr` off-scheduler threads.
+Workers identify themselves via a thread-local `Pool::current_pool` pointer, set when `run()` starts.
+`Pool::current()` returns a `shared_ptr` to the active pool, or `nullptr` off-scheduler threads.
 
 ## Priority buckets
 
-Tasks are not kept in one monolithic priority queue. Instead, each pool has **five fixed buckets** scanned from highest to lowest priority:
+Tasks are not kept in one monolithic priority queue.
+Instead, each pool has **five fixed buckets** scanned from highest to lowest priority:
 
 | Bucket   | Priority range | DSL level                    |
 | -------- | -------------- | ---------------------------- |
@@ -151,13 +164,18 @@ Tasks are not kept in one monolithic priority queue. Instead, each pool has **fi
 | LOW      | ≥ 250          | `Priority::LOW`              |
 | IDLE     | < 250          | `Priority::IDLE`             |
 
-`Pool::try_dequeue_task()` walks buckets 0→4 and returns the first available task. Within a bucket, ordering is **FIFO** (per-producer FIFO in the MPMC queue; strict FIFO in MPSC). Priority therefore dominates bucket order; tie-breaking within a bucket follows enqueue order, not reaction ID.
+`Pool::try_dequeue_task()` walks buckets 0→4 and returns the first available task.
+Within a bucket, ordering is **FIFO** (per-producer FIFO in the MPMC queue; strict FIFO in MPSC).
+Priority therefore dominates bucket order; tie-breaking within a bucket follows enqueue order, not reaction ID.
 
-Priority affects **queuing order only**. Running tasks are never preempted.
+Priority affects **queuing order only**.
+Running tasks are never preempted.
 
 ## Lock-free queues
 
-Both queue implementations use a **block-based** design: fixed-size blocks of 64 slots linked in a list. Producers claim slots with `write.fetch_add(1)`, construct the payload in place, then set a `committed` flag. Consumers read committed slots and advance head/tail as blocks drain.
+Both queue implementations use a **block-based** design: fixed-size blocks of 64 slots linked in a list.
+Producers claim slots with `write.fetch_add(1)`, construct the payload in place, then set a `committed` flag.
+Consumers read committed slots and advance head/tail as blocks drain.
 
 ### TaskQueue (MPMC)
 
@@ -173,17 +191,20 @@ Cross-producer ordering is not guaranteed; per-producer FIFO is preserved.
 
 Used for single-consumer pools (`MainThread`, concurrency-1 custom pools).
 
-The producer side matches `TaskQueue`. The consumer side is simpler: a plain (non-atomic) read index, no CAS on dequeue, and immediate block retirement to the graveyard when advancing.
+The producer side matches `TaskQueue`.
+The consumer side is simpler: a plain (non-atomic) read index, no CAS on dequeue, and immediate block retirement to the graveyard when advancing.
 
-`try_dequeue` must only be called from the designated consumer thread. Force shutdown from another thread delegates queue draining to that consumer via `discard_queues_requested`.
+`try_dequeue` must only be called from the designated consumer thread.
+Force shutdown from another thread delegates queue draining to that consumer via `discard_queues_requested`.
 
 ### Shared block helpers
 
 `queue/detail/block_ops.hpp` provides `link_next_block` and `retire_block` shared by both queues.
 
 ### Lock-free vs wait-free
 
-The queues are **lock-free** at the algorithm level: no mutexes, and the system makes progress under contention. They are **not wait-free end-to-end**:
+The queues are **lock-free** at the algorithm level: no mutexes, and the system makes progress under contention.
+They are **not wait-free end-to-end**:
 
 - Block allocation uses `operator new`.
 - Overflow paths use CAS loops on list pointers.
@@ -195,25 +216,29 @@ The hot-path slot claim via `fetch_add` is wait-free within a non-full block.
 
 ### Single-group fast path
 
-Most reactions belong to at most one group (including `Sync<T>`). For these, `Group::try_submit()`:
+Most reactions belong to at most one group (including `Sync<T>`).
+For these, `Group::try_submit()`:
 
 1. Tries to decrement `tokens` with a compare-exchange.
 1. On success, submits to the pool immediately with a `RunningLock` that calls `release_token()` on destruction.
 1. On failure, **parks** the task in priority-ordered waiter buckets via `park_publish()` / `park_reconcile()`.
 
-The token counter can go **negative** when waiters reserve slots they have not yet consumed. This signed counter, combined with per-waiter **arbiter slots** (`atomic<bool>`), ensures no lost wakeups and exact accounting when multiple waiters race with draining threads.
+The token counter can go **negative** when waiters reserve slots they have not yet consumed.
+This signed counter, combined with per-waiter **arbiter slots** (`atomic<bool>`), ensures no lost wakeups and exact accounting when multiple waiters race with draining threads.
 
 When a running task finishes, `release_token()` increments `tokens` and drains at most one parked waiter into the pool — keeping running count bounded by the group's concurrency.
 
 ### Multi-group slow path
 
-Tasks bound to multiple groups (`Sync<A>` and `Sync<B>`, etc.) use `CombinedLock`: each group gets a `GroupLock` backed by a mutex-protected sorted queue. `slow_pending` on each group prevents fast-path submitters from jumping ahead of older multi-group waiters.
+Tasks bound to multiple groups (`Sync<A>` and `Sync<B>`, etc.) use `CombinedLock`: each group gets a `GroupLock` backed by a mutex-protected sorted queue.
+`slow_pending` on each group prevents fast-path submitters from jumping ahead of older multi-group waiters.
 
 When a `GroupLock` is released, the group may drain a fast-path waiter even if slow-path waiters exist, if the pre-release token count indicates a committed fast waiter is owed a slot — avoiding deadlocks between fast and slow paths.
 
 ### External waiters
 
-When a task is parked in a group's wait buckets (not yet in the pool queue), the destination pool must not go idle as if it had no work. `Pool::register_external_waiter()` increments `external_waiters`, keeping workers alive until the parked task is drained or the registration is destroyed.
+When a task is parked in a group's wait buckets (not yet in the pool queue), the destination pool must not go idle as if it had no work.
+`Pool::register_external_waiter()` increments `external_waiters`, keeping workers alive until the parked task is drained or the registration is destroyed.
 
 If idle reactions are registered for that pool (or globally), a `pending_idle` latch ensures one idle epoch fires before the next dequeue — preserving the invariant that parking a non-runnable task triggers idle detection, even if the worker is preempted and a runnable task arrives in the queue before the worker resumes.
 
@@ -245,7 +270,8 @@ When a pool worker finds no runnable task:
 | `FINAL`   | Used after the main thread exits `start()`; even persistent pools stop once their queues empty.                                  |
 | `FORCE`   | Clears queues and wakes all threads; used for forced test timeouts. MPSC pools require the consumer thread to perform the drain. |
 
-`Scheduler::start()` starts worker pools first, then blocks in `MainThread::start()`. When the main thread pool exits (after shutdown), pools are stopped in order — non-persistent pools before persistent ones — then joined.
+`Scheduler::start()` starts worker pools first, then blocks in `MainThread::start()`.
+When the main thread pool exits (after shutdown), pools are stopped in order — non-persistent pools before persistent ones — then joined.
 
 Persistent pools (`ThreadPoolDescriptor::persistent`) continue accepting tasks during a normal shutdown so networking or logging reactors can finish in-flight work.
 

src/threading/Reaction.hpp

@@ -148,9 +148,9 @@ namespace threading {
         /// outlive scheduler-side resources because PowerPlant tears reactors down before the
         /// scheduler. The first submit resolves the pool and stores it here (release); later submits
         /// just load it (acquire). The write is a plain store rather than a CAS: every writer
-        /// resolves the same pool for a given reaction, so concurrent first-submit stores are still
-        /// a data race (though they publish identical values) and a reader either sees nullptr
-        /// (and re-resolves) or the one pointer.
+        /// resolves the same pool for a given reaction. Concurrent stores/loads are well-defined
+        /// on the atomic (no data race); a reader either sees nullptr (and re-resolves) or the
+        /// cached pointer.
         std::atomic<scheduler::Pool*> scheduler_data{nullptr};
         friend class scheduler::Scheduler;  /// Let the scheduler mess with reaction objects
     };

src/threading/scheduler/Scheduler.cpp

@@ -203,7 +203,8 @@ namespace threading {
             auto lock = std::make_unique<CombinedLock>();
             for (const auto& desc : descs) {
                 lock->add(get_group(desc)->lock(task_id, priority, [pool] {
-                    const bool current_pool_idle = Pool::current() != nullptr && Pool::current()->is_idle();
+                    const auto current_pool = Pool::current();
+                    const bool current_pool_idle = current_pool != nullptr && current_pool->is_idle();
                     pool->notify(!current_pool_idle);
                 }));
             }
@@ -245,7 +246,8 @@ namespace threading {
                 pool = get_pool(task->pool_descriptor).get();
             }
 
-            const bool current_pool_idle = Pool::current() != nullptr && Pool::current()->is_idle();
+            const auto current_pool = Pool::current();
+            const bool current_pool_idle = current_pool != nullptr && current_pool->is_idle();
 
             // Fast path for a single group: lock-free token acquisition and waiter buckets
             if (task->group_descriptors.size() == 1) {