core/cmake: stop a serial dispatch rebuilding a std::deque it never fills, and say what -Wno-shadow-uncaptured-local really suppresses (fixes #660, refs #662)

cmake/compiler_options.cmake

@@ -157,8 +157,54 @@ elseif(MORPH_COMPILER_FAMILY STREQUAL "Clang")
     # oversight this warning should flag.
     _morph_clang_suppression_if_supported(-Wno-missing-designated-field-initializers)
     _morph_clang_suppression_if_supported(-Wno-nrvo)  # not eliding a trivial-type copy on return
+    # -Wshadow-uncaptured-local does NOT mean "a lambda parameter shadowing an
+    # uncaptured local", which is what this line claimed until morph#662. It is
+    # clang's group for *any* declaration inside a lambda with no
+    # capture-default that shadows an enclosing local the lambda did not
+    # capture — the parameter case is one of four. Measured on clang 22.1.8,
+    # `clang++ -std=c++23 -fsyntax-only -Weverything`, one construct per
+    # function:
+    #
+    #   [](int value) { ... }                 shadows a local variable [-Wshadow-uncaptured-local]
+    #   [] { int value = 2; ... }             shadows a local variable [-Wshadow-uncaptured-local]
+    #   [value = 7] { ... }                   shadows a local variable [-Wshadow-uncaptured-local]
+    #   [first = first + 1] { ... }           shadows a structured binding [-Wshadow-uncaptured-local]
+    #   { int value = 2; }   (no lambda)      shadows a local variable [-Wshadow]
+    #   [value] { int value = inner; ... }    shadows a local variable [-Wshadow]
+    #
+    # So the init-capture cases — including the `[x = std::move(x)]` idiom this
+    # codebase uses throughout to move state into a continuation — are
+    # suppressed here, and the last two rows show what is still enforced:
+    # -Wshadow (GCC list below, and via -Weverything here) keeps every shadow
+    # that is not inside an uncapturing lambda.
+    #
+    # The group cannot be narrowed to the one construct the old comment named;
+    # clang has no finer flag, so the alternatives are all-or-nothing plus
+    # per-site suppressions. Dropping it entirely is a real cleanup, not a
+    # one-line diff: measured on this tree with every optional feature
+    # configured (clang 22.1.8, clang-debug + NET/QT/FORMS_QML/OFFLINE_SQLITE/
+    # LOAD_TESTS/HMAC_EXAMPLES/LADDER/BANK_EXAMPLE, -Werror off), removing this
+    # line yields **4856 diagnostics over 41 distinct sites in 16 first-party
+    # files** — the emission count is that much larger than the site count
+    # because 35 of the 41 are in headers, re-reported once per translation
+    # unit that includes them. By file: 8 each in core/backend.hpp and
+    # core/remote.hpp, 4 each in core/bridge.hpp and core/completion.hpp, 1
+    # each in core/registry.hpp, core/callback_scope.hpp, offline/
+    # sync_worker.hpp and qt/qt_executor.hpp, 3 across two tests/ files and 10
+    # across six examples/ files. No dependency is affected (they arrive via
+    # -isystem). That cleanup is tracked separately; do not fold it into an
+    # unrelated change.
+    #
+    # One consequence is recorded rather than fixed here (morph#662, still
+    # open): emsdk 3.1.56's older clang files `declaration shadows a structured
+    # binding` under plain -Wshadow instead, so the WASM leg is the only leg
+    # that enforces that one row of the table above. That statement is read off
+    # the CI log of run 35573507189, not reproduced locally — no emsdk
+    # toolchain is available here. It also currently enforces nothing in
+    # practice: morph#661 fixed the only four structured-binding shadows in the
+    # tree, and the measurement above found zero remaining.
     list(APPEND MORPH_WARNING_FLAGS
-        -Wno-shadow-uncaptured-local    # lambda param shadowing an uncaptured local
+        -Wno-shadow-uncaptured-local
         -Wno-documentation-unknown-command
         -Wno-unsafe-buffer-usage        # flags all pointer arithmetic; needs a hardened API
     )

docs/spec/core/executor.md

@@ -207,6 +207,22 @@ Internally `StrandExecutor` maintains a map of `ModelId → shared_ptr<Strand>`
 mutex, a pending queue, and a `running` flag. The executor also tracks an
 `_inFlight` counter (guarded by the map mutex) that the destructor waits on.
 
+The pending queue is `StrandExecutor::PendingQueue`, not `std::queue`. It is a
+FIFO with the head task stored **inside** the `Strand` and a lazily constructed
+`std::deque` behind it, and it exists purely to make the common case cheaper:
+because the drain step below destroys the whole `Strand` as soon as the queue
+empties, a serial workload (one action at a time, each waited out) rebuilds the
+queue on every dispatch and puts exactly one task in it — and libstdc++'s
+`std::deque` allocates its node map *and* a 512-byte first buffer in its
+default constructor, whether or not anything is ever pushed. See
+[Lifetime & ownership](#lifetime--ownership) below for the measurement.
+`PendingQueue` does no locking of its own; every access is
+under the owning `Strand::mtx`, exactly as the `std::queue` it replaced was,
+and it tracks occupancy with a flag rather than by testing the callable, so an
+empty `std::function` is queued and dispatched like any other. It changes no
+lifetime or locking rule: the erase still fires when `empty()` becomes true,
+still under the `{_mapMtx, strand->mtx}` pair.
+
 **`_inFlight` is incremented with the *decision* to dispatch, not lazily.**
 `post()` increments `_inFlight` in the same `_mapMtx` critical section that flips
 `running` true and decides to schedule, before releasing the lock; the re-arm
@@ -323,9 +339,29 @@ The strand map is self-cleaning: when a strand drains (its `pending` queue is
 empty), `scheduleNext` clears `running` and erases the map entry under the
 combined `{_mapMtx, strand->mtx}` lock. Live memory therefore tracks the set of
 *currently active* models rather than every model ever seen — there is no
-per-model registration to leak. The cost is allocation churn: a model that is
-posted to in bursts allocates a fresh `Strand` each time its queue empties and
-refills, rather than keeping one long-lived strand per key.
+per-model registration to leak.
+
+**The cost is allocation churn, and it is bounded rather than removed.** A
+model posted to serially — one action at a time, each waited out — never has a
+task queued at the instant the previous one finishes, so it never keeps a
+strand: every dispatch takes `post()`'s `if (!slot)` branch and rebuilds the
+map node and the `Strand`. Measured on `7a343e6f` with
+`tests/bench/bench_dispatch_allocations.cpp` (see
+[testing_strategy.md](../testing_strategy.md)), x86-64 Linux, GCC 16.2.1 /
+libstdc++, `-O2`, that came to **4 allocations and 760 of the 1990 bytes** a
+local `execute` round trip cost — 38% of the bytes, for a strand that is
+rebuilt and thrown away. 576 of those bytes were not the strand at all but
+`std::queue`'s `std::deque` eagerly allocating a node map and a 512-byte first
+buffer in its default constructor. Replacing that container with
+`PendingQueue`, which holds the head task inline, cut the strand's share to **2
+allocations and 152 bytes** and the whole round trip to 18.9 allocations /
+1396 bytes (morph#660).
+
+The two allocations that remain — the map node and the `Strand` itself — are
+inherent to the erase: removing them means keeping the slot alive across the
+drain, which trades this churn for a per-model entry that nothing reclaims,
+since `StrandExecutor` has no deregistration hook. That trade is deliberately
+not made here; the erase is what bounds the map.
 
 ## Thread safety
 

include/morph/core/strand.hpp

@@ -3,13 +3,14 @@
 #pragma once
 #include <condition_variable>
 #include <cstdint>
+#include <deque>
 #include <exception>
 #include <functional>
 #include <memory>
 #include <mutex>
-#include <queue>
 #include <string>
 #include <unordered_map>
+#include <utility>
 
 #include "../attributes.hpp"
 #include "executor.hpp"
@@ -130,10 +131,77 @@ class StrandExecutor {
     }
 
 private:
+    /// @brief FIFO of tasks queued on one strand, with the head task held inline.
+    ///
+    /// Behaviourally a `std::queue<std::function<void()>>` restricted to the
+    /// three operations the strand uses, and used under exactly the same
+    /// discipline: every call happens with the owning `Strand::mtx` held, so
+    /// this type does no locking of its own.
+    ///
+    /// It exists because of what the *container* cost, not what the strand
+    /// did with it. The drain-and-erase step in `scheduleNext` destroys the
+    /// whole `Strand` as soon as the queue empties, so a workload that
+    /// dispatches one action at a time against a model builds a fresh queue on
+    /// every call and puts exactly one task in it. libstdc++'s `std::deque`
+    /// allocates its node map *and* a first 512-byte buffer in its default
+    /// constructor, so that came to 576 bytes of the 760 the strand cost per
+    /// local dispatch (morph#660). Holding the head task in the strand makes
+    /// that case allocation-free; the overflow deque is constructed only when
+    /// a second task is genuinely queued behind a running one, after which the
+    /// cost is the deque's as before.
+    ///
+    /// This changes no lifetime or locking rule: the erase still happens when
+    /// `empty()` becomes true, still under the `{_mapMtx, strand->mtx}` pair.
+    class PendingQueue {
+    public:
+        /// @brief Reports whether the queue holds no task.
+        /// @return `true` when nothing is queued.
+        [[nodiscard]] bool empty() const noexcept { return !_hasHead; }
+
+        /// @brief Appends @p task to the back of the queue.
+        /// @param task Callable to queue. An *empty* `std::function` is queued
+        ///             and later dispatched like any other: occupancy is
+        ///             tracked by a separate flag rather than by testing the
+        ///             callable, so this type never silently drops one.
+        void push(std::function<void()>&& task) {
+            if (!_hasHead) {
+                _head = std::move(task);
+                _hasHead = true;
+                return;
+            }
+            if (!_overflow) {
+                _overflow = std::make_unique<std::deque<std::function<void()>>>();
+            }
+            _overflow->push_back(std::move(task));
+        }
+
+        /// @brief Removes the task at the front of the queue and returns it.
+        /// @return The front task.
+        /// @pre `!empty()`.
+        std::function<void()> pop() {
+            std::function<void()> task = std::move(_head);
+            if (_overflow && !_overflow->empty()) {
+                _head = std::move(_overflow->front());
+                _overflow->pop_front();
+            } else {
+                // A moved-from std::function is valid but unspecified; clear it
+                // explicitly so the slot holds no captured state while idle.
+                _head = nullptr;
+                _hasHead = false;
+            }
+            return task;
+        }
+
+    private:
+        std::function<void()> _head;
+        std::unique_ptr<std::deque<std::function<void()>>> _overflow;
+        bool _hasHead = false;
+    };
+
     struct Strand {
         IExecutor* base = nullptr;
         std::mutex mtx;
-        std::queue<std::function<void()>> pending;
+        PendingQueue pending;
         bool running = false;
     };
 
@@ -151,8 +219,7 @@ class StrandExecutor {
             std::function<void()> task;
             {
                 std::scoped_lock const lock{strand->mtx};
-                task = std::move(strand->pending.front());
-                strand->pending.pop();
+                task = strand->pending.pop();
             }
             try {
                 task();

scripts/branch_partial_allowlist.json

@@ -74,7 +74,7 @@
     },
     {
       "file": "include/morph/core/strand.hpp",
-      "line": 192,
+      "line": 259,
       "source": "if (iter != _strands.end() && iter->second == strand) {",
       "reason": "Unreachable by construction given this class's lock discipline (core audit finding ST1, resolved to (b) by a concurrency-focused review pass after an initial (a)/(b)-undecided pass). `_strands` has exactly two mutation sites: `post()`'s insert-if-absent (this file, `if (!slot) { slot = make_shared<Strand>(); }`) and this exact block's own erase a few lines below, both under `_mapMtx`. At most one lambda per `Strand` runs at a time (`post()` only schedules when `!strand->running`, and re-arming happens only through this same lambda's own `more` branch), so dispatch for one `Strand` is strictly serial; and only a strand's own currently-running lambda can erase its map entry (the erase fires only in the `!more` branch for the entry this frame just found under `_mapMtx`, and a concurrent `post(key)` while this lambda runs can only push onto the existing `Strand`, never replace it). Together these force `_strands.find(key)` to yield this exact strand whenever this line runs, so `iter->second == strand` cannot be false. No stress test needed: one was considered, but given the strength of the lock-discipline argument it would spend CI time re-confirming an already-proven invariant rather than searching for an unknown one."
     },