core: recycle the strand's map node instead of rebuilding it every serial dispatch, with no deregistration hook (fixes #670)

[Yaraslaut]

Closes #670.

morph#660 took the container's share of what a serial dispatch spent on its strand. What it left was 2 allocations and 152 bytes per call: the unordered_map node and the make_shared<Strand>, rebuilt every dispatch because the drain destroys the entry as soon as the queue empties.

The ticket's framing, and why this does not follow it

#670 and its triage comment both treat that residual as inherent to the erase, removable only through a StrandExecutor deregistration hook — keeping the map slot alive across the drain, and so trading this churn for a per-model entry nothing reclaims. docs/spec/core/executor.md said the same in as many words.

No hook is added here, and the slot does not stay alive. The entry leaves the map at exactly the same program point, under exactly the same {_mapMtx, strand->mtx} pair, with exactly the same precondition. The drain calls extract instead of erase and parks the detached node in a single-slot _spare; the next post() that misses re-keys that node and inserts it back. The map is bounded by the removal exactly as before, _spare holds at most one node, and it is freed with the executor.

So the {_mapMtx, strand->mtx} atomicity recorded at strand.hpp:81/:167 is not renegotiated. That was the whole risk the ticket was about, and it is untouched.

The one claim the branch's safety rests on

extract detaches the element and transfers ownership of the node; it does not destroy the element, invalidate other elements, or move the mapped object. Every argument the erase supported — the per-key serialisation invariant, "the drain never removes a strand whose pending is non-empty", "a strand that becomes running in post() is still the map entry" — is an argument about when the entry leaves the map, and that moment is unchanged.

The second-order claim, that a parked node's Strand may be reused, stands on sole ownership: use_count() == 1 means the recycled node holds the only reference, so nothing else can reach the object and reusing it is indistinguishable from constructing a new one. When a strand lambda is still finishing and holds its shared_ptr, the guard fails and a fresh Strand is built exactly as before — only the node is recycled. To make the guard usually hold, the lambda releases its shared_ptr right after the drain block instead of at its own destruction; nothing after that point touches the strand. That timing decides whether the object is recycled, never whether the recycling is safe.

Re-measured, not inherited

tests/bench/bench_dispatch_allocations.cpp (morph_bench_alloc, MORPH_BUILD_LOAD_TESTS=ON), Release, x86-64 Linux, clang 22.1.8 / libstdc++ 16.2.1, base binary 7d4ca453 and the branch binary run alternately, six pairs:

base (18.90 (1394.7      new  (16.96 (1245.0
base (18.93 (1395.6      new  (16.95 (1244.8
base (18.91 (1395.6      new  (16.94 (1244.2
base (18.90 (1395.1      new  (16.86 (1241.9
base (18.95 (1395.6      new  (16.98 (1245.6
base (18.96 (1396.8      new  (16.84 (1241.5

18.90 → 16.95 allocations/call, 1394.8 → 1244.6 bytes/call. --attribute on one steady-state call names the two that went — a 32 and a 120 disappear from the middle of the list:

base  160 17 24 16 16 176 344 32 120 32 136 32 64 32 32 32 24 40 40   (19)
new   160 17 24 16 16 176 344 32     32 136 32 64 32 32 32 24 40 40   (17)

32 bytes is the map node; 120 is make_shared<Strand> (16 of control block over a 104-byte Strand). 152 bytes — the figure the ticket names, recovered in full. Magnitude is libstdc++-specific, as it was for morph#660. The ticket's own 18.9/1396 baseline reproduced exactly.

The detector, and a mutation for the new step

Neither existing [race] case can be wrong about re-keying. An entry re-inserted under the previous key still serialises every task that reaches it, still runs them in order and still completes them all; what it corrupts is which key the map answers for, and that only becomes a serialisation failure two posts later:

1. Key A drains, parking a node still keyed A.
2. Key B misses and takes it — unkeyed, it goes back into the map under A. B's first task runs on a strand the map calls A, so find(B) still misses.
3. B's next post misses too, and installs a second strand for B while the first is still running.

tests/test_strand_race.cpp gains a third case that manufactures that: several keys each posting a short burst (so the burst's second post arrives while the first task runs) and then going quiet (so the drain that parks a node happens), several keys out of phase so a parked node crosses keys. Three detectors per key — in-flight counter, plain non-atomic per-key state, FIFO sequence.

All under clang-tsan with TSAN_OPTIONS=suppressions=cmake/tsan.supp, 10 runs each, per-run limit 120 s (CI's ctest TIMEOUT):

build	[race] result
this branch	pass 10/10
M1 — pre-fix two-step drain restored on top of this change	fail 10/10 (TSan data race)
M2 — park the node even when work is still queued	5/5 exceed 120 s; 3/5 also emit TSan data races at test_strand_race.cpp:208 before hanging
M4 — delete _spare.key() = key; (the new step)	fail 10/10, caught by the new case
M3 — delete the use_count() != 1 guard	pass 10/10

An earlier draft of the new case that drained between every single post, with no burst, passed 10/10 against M4 — which is why the burst is in the case and why that is written into its comment. Shape, not volume, again.

Instrumentation confirmed rather than assumed, per #683's new narrow mode:

$ bash scripts/check_sanitizer_instrumentation.sh --binary build/clang-tsan/tests/morph_tests tsan
check_sanitizer_instrumentation: morph_tests carries 173 __tsan_ symbols -- tsan-instrumented.

What is not covered by a detector

M3 passes 10/10. Deleting the use_count() != 1 guard is not something the race suite can see. That is honest rather than alarming: recycling a Strand that a finishing lambda still references appears safe by inspection too — after the drain block that lambda's only remaining work is --_inFlight under _mapMtx, and it never touches *strand again. The guard is kept anyway, so the reuse stands on a one-line ownership argument instead of a multi-step access-ordering one; it is defence in depth, and this table is where that is recorded rather than left to look like coverage.

Other verification

• Release ctest: 1568/1568.
• clang-tsan ctest: 1583/1583, and clang-asan ctest: 1583/1583, both with CI's own -E "OomInjector|morph#108" exclusion (ci.yml; OomInjector replaces global operator new, which ASan/TSan already interpose). Without the exclusion those six fail on both legs, as CI documents.
• clang-tidy: via clang-tidy-diff.py -path build/clang-debug over this branch's -U0 diff against 7d4ca453, clang-tidy 22.1.8 — 0 findings. Verified non-vacuous: a planted C-style cast and a new int in installStrand produced five findings on those exact lines, so the gate does reach this header.
• clang-format --dry-run -Werror over both touched sources: clean.
• check_mutation_survivors.py, check_nolint_directives.sh, check_bidi_controls.py, check_spec_citations.sh, check_spec_sync.sh (with the real path list), check_catch_test_names.sh, check_ctest_name_collisions.sh, check_test_type_names.sh: all OK.
• Doxygen with WARN_AS_ERROR = FAIL_ON_WARNINGS (MORPH_BUILD_DOCUMENTATION=ON): builds clean.

Not verified: the branch-coverage gate end to end — it needs build/clang-coverage/coverage.lcov from scripts/coverage.sh, which was not run. What was checked is the part this change could break: check_branch_coverage.resolve_allowlist_source_line resolves the repointed strand citation to line 323 with no failures. installStrand was deliberately written without the defensive branches an earlier draft had (a null check, an idle re-check, an insert-failure path) precisely because none of their arms can be taken and each would have needed its own allowlist entry; the preconditions are written down in the comment instead.

Commits

1. 9aafc140 — the change: strand.hpp, the third [race] case, and the two spec files (docs/spec/core/executor.md's "Lifetime & ownership" paragraph said this residual was inherent; it now says what replaced it, with the measurement).
2. 200dcbec — gate repoint only: scripts/branch_partial_allowlist.json's strand entry moves 259 → 323, and its reason prose is corrected where it cited post()'s now-deleted if (!slot) { slot = make_shared<Strand>(); } and called the removal an erase.

Filed, not folded

• morph_bench_alloc's per-call figure moved 20% between two runs of the same source, and nothing records that it can #687 — morph_bench_alloc's per-call figure moved 20% between two runs of the same source (15.06 once, 18.9 on 23 other runs). Weak evidence, observed once, not reproduced; filed because three tickets and a spec section now cite this instrument by number.
• A relative TSAN_OPTIONS suppressions path makes ctest's Catch2 discovery exit 66 with no message, and no doc says to use an absolute one #688 — a relative TSAN_OPTIONS suppressions path makes ctest's Catch2 discovery exit 66 with empty output, and nothing documents the absolute form or the OomInjector exclusion CI uses.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Runner verification — and my framing of this ticket was wrong

I narrowed the solution space incorrectly. My triage of #670 said it "needs a StrandExecutor deregistration hook, i.e. changing when the map slot dies", and I dispatched it with "not worth the risk" pre-authorised as an outcome. That framing was avoidable, and this branch avoids it.

Verified structurally — the drain's lock shape is unchanged:

master:                                     branch:
  scoped_lock{strand->mtx}                    scoped_lock{strand->mtx}
  more = !strand->pending.empty()             more = !strand->pending.empty()
  if (!more) { running = false;               if (!more) { running = false;
    if (iter != end && iter->second == strand)   if (iter != end && iter->second == strand)
      _strands.erase(iter);                        _spare = _strands.extract(iter);

Same guard, same moment, same {_mapMtx, strand->mtx} pair. The entry leaves the map at exactly the program point it always did — so the atomicity a previous defect forced into that shape is never renegotiated, which was the entire risk. Taking the 152 bytes without touching lifetime rules is a strictly better answer than the one I framed, and better than the handback I authorised.

The measurement reproduces the ticket and then moves it: base 18.90 alloc / 1394.8 B (against the ticket's 18.9/1396), new 16.95 / 1244.6 — −1.95 allocations, −150.2 bytes, with --attribute showing exactly the 32 map node and the 120 control block leaving the list. The full 152.

The mutation table is the best part, and M4 is why

mutant	result
M1 — pre-fix two-step drain on top of the change	fail 10/10
M2 — park the node even with work queued	5/5 over 120 s, 3/5 with TSan races
M4 — delete the new _spare.key() = key;	fail 10/10
M3 — delete the use_count() != 1 guard	pass 10/10

M4 needed a new third [race] case, and the reason is the finding: neither existing case can be wrong about re-keying, because a mis-keyed entry still serialises and still completes everything — it only becomes a serialisation break two posts later. And an earlier draft of that case, which drained between every post, passed 10/10 against M4. The burst is what makes it detect, and that is written into the comment rather than left as a tuning accident. That is the #668 lesson applied prospectively by the person introducing the code, which is the first time that has happened today.

M3 is reported as passing rather than dressed up. The use_count() guard is not detector-provable; keeping it as defence in depth so the reuse rests on an ownership argument, and saying the detector cannot prove it, is the honest handling — a table with one green row nobody hid.

#687 deserves attention beyond this PR

morph_bench_alloc's per-call figure moved 20% between runs of identical source — one process at 15.06/1287.7 against 23 runs at 18.9/1395. Observed once, correctly labelled weak.

It matters disproportionately because three tickets and a spec section now cite that instrument by number: #660's saving, #670's residual, this PR's −150.2 B, and docs/spec/core/executor.md. If the bench is bimodal, every one of those figures inherits the uncertainty. Filing it rather than absorbing it was right.

Not verified by me: the allocation runs, the TSan and ASan ctest sweeps, and the coverage gate (which needs an lcov the lane did not produce). Writing installStrand without three defensive branches whose arms cannot be taken — each of which would have needed its own allowlist entry — is the kind of restraint that does not show up in a diff, and is worth noting.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Runner: five legs red, and this is not the infrastructure signature

Failing step per leg — the fastest discriminator, and it points the wrong way for an environment problem:

  Bank example / UBSan       Every ctest binary is instrumented
  Linux / Qt6 WebSockets     Build
  Linux / gcc-debug          Build
  Linux / gcc-release        Build
  Valgrind memcheck          Build

Four legs fail in Build. Today's five infrastructure signatures all died in package installation, vcpkg setup or a dependency-cache configure, at 30 s–5 min and never in a compile. This is a genuine build failure. (The Bank UBSan one is almost certainly downstream: nothing built, so ctest listed nothing, so the #675 floor refused — the same correct behaviour it showed earlier today.)

What I have ruled out so far, on the branch with local GCC 16.2.1:

strand.hpp (synthetic TU)          ok
tests/test_strand_race.cpp         ok
tests/test_strand.cpp              ok
tests/test_strand_extra.cpp        ok
examples/common/testkit/test_strand_interleaver.cpp   ok

So it is not a syntax error in the header or in the obvious consumers. Three other includers could not be checked that way (they need glaze include paths my ad-hoc command did not supply) — that is a limitation of my check, not a finding about them.

What I have not established, and am not going to guess at: the actual compiler error. The run still has legs in flight, so gh run view --log refuses, and the check-run annotations carry only Process completed with exit code 1 with no diagnostic. Today has repeatedly punished guessing before the log exists.

Two hypotheses worth holding, neither confirmed:

1. A gcc-15 vs gcc-16 difference. CI installs gcc-15; my local GCC is 16.2.1, and the branch's new code uses std::map::extract / insert(node_handle) / node_handle::key(). A version-specific rejection would pass here and fail there.
2. A TU I could not syntax-check. Four legs including Qt6 WebSockets suggests something broader than the strand tests.

A full GCC build of the branch is running locally now; that settles it either way without waiting on CI.

Nothing about the verification I did earlier changes. The extract-not-erase design, the unchanged {_mapMtx, strand->mtx} critical section, and the mutation table all stand on their own evidence — a build failure elsewhere does not retract them.

This PR also now needs a rebase independently: master has moved twice (#684 74158aba, #686 24a470c4) since this branched from 7d4ca453.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Fixed in c02346ac — reproduced locally, and it is GCC-only

tests/test_strand_race.cpp:374:60: error: useless cast to type 'uint64_t'
    {aka 'long unsigned int'} [-Werror=useless-cast]
ninja: build stopped: subcommand failed.

slot is std::size_t, so 100 + slot is already uint64_t on LP64 and the static_cast is an identity conversion. -Wuseless-cast is GCC-only — clang has no such warning. That is exactly why a clang-verified branch passed every clang leg and failed all four GCC ones.

This is the mirror image of #661, where the WASM leg's older clang was the only thing in CI that could see a -Wshadow the project suppresses elsewhere. Same lesson, opposite compiler: the branch was verified thoroughly, on one toolchain.

Verified both directions with the warning that fired:

fixed form -> g++ -std=c++23 -Werror=useless-cast ...   exit 0
old form   -> same command, same 1 diagnostic
full gcc-debug build of morph_tests -> links, 0 errors  (previously stopped at this file)

Dropping the cast rather than suppressing it: brace-init of ModelId from std::size_t is non-narrowing where size_t is 64-bit and a widening conversion where it is 32-bit, so the removal is portable and the intent is unchanged.

Also rebased onto 24a470c4 — master moved twice (#684, #686) while this was open.

A correction to my own earlier note on this PR

I wrote that four Build failures were "not the infrastructure signature" and therefore a genuine defect. That was right. But I also said my local syntax checks had "ruled out" the header and the obvious consumers — they had not ruled out anything, because -fsyntax-only without the project's warning flags cannot see a -Werror=useless-cast. The check I ran could not have found this bug. A full build with the project's own flags found it in one pass.

That is the same mistake in miniature that this PR's own failure is about: verifying with a weaker configuration than the one that judges you. I have been telling lanes that all day.

The Bank UBSan failure was downstream as predicted — nothing built, so ctest listed nothing, so the #675 floor refused. It should clear with the build.

Nothing about the earlier verification changes: the extract-not-erase design, the unchanged {_mapMtx, strand->mtx} critical section, and the mutation table stand on their own evidence.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Yaraslaut]

Runner: the one red leg is pre-existing on master — not this branch

gh pr checks 689: 54 pass, 1 fail, 0 pending. The failure is Bank example / UBSan, and it is not yours:

Bank example / UBSan on master:
  run 35643825635: failure
  run 35639767288: failure

That job — added by #683 — has never passed, on master or anywhere. master is red on it right now. Filed as #690 with the evidence.

Its shape here is identical to master's: Qt installs, Configure succeeds, Build succeeds, then the instrumentation sweep reports ctest listed no tests in build/clang-ubsan. The guard is behaving correctly — it refuses to pass having examined nothing, which is what #675 added it for. It simply has nothing to examine, for a reason nobody has established, and scripts/check_sanitizer_instrumentation.sh:129 discards ctest's stderr (2>/dev/null) so the diagnostic that would say why is thrown away.

The GCC fix landed and worked. Every leg that was red for a reason belonging to this branch is now green — 54 of 55, including all four Build legs and Linux / clang-tsan.

I am not merging it

The standing authorisation is for PRs whose CI is green, and this is 54/55. A failure being demonstrably somebody else's does not make the PR green, and I would rather leave that judgement visible than quietly redefine the bar — especially on a branch that changes concurrency-critical code.

The unblocking move is #690, not a merge. Once that job passes on master, this should go green with no further work.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Runner: rebased onto 38dcba18 — the blocker is fixed on master

#692 landed, closing #690. Its change touched two gates this branch had never been judged by:

.github/workflows/ci.yml                        <-- QT_QPA_PLATFORM on the sweep step
scripts/check_sanitizer_instrumentation.sh      <-- stderr no longer discarded
scripts/test_check_sanitizer_instrumentation.sh
examples/bank/CMakeLists.txt

First staleness limb — its CI was judged by rules that no longer exist — so rebase, not merge. Replayed cleanly, pushed as 1c60267e.

Cheap gates on the rebased tree, including the one #692 rewrote:

ok: 15 structured citation(s) in scripts/mutation_survivors.json resolve to the line they name.
NOLINT directive lint OK: 177 NOLINTNEXTLINE directive(s), all annotating code.
bidi-control lint OK: 1251 text file(s) scanned, 12 codepoint(s) searched for, 0 raw occurrence(s).
check_sanitizer_instrumentation self-test: all checks passed

This should now go green on its own merits. Its previous 54/55 had exactly one failure — Bank example / UBSan — which was master's, not this branch's, and is the thing #692 fixed. If that leg is red again after this run, it is a new fact and not the one we have been chasing.

Nothing about the branch's own verification changes: the extract-not-erase design, the unchanged {_mapMtx, strand->mtx} critical section, the mutation table, and the GCC -Wuseless-cast fix all stand.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW