test_strand_race.cpp does not detect the race it is the regression test for -- 10/10 passes under TSan against a build with the two-step drain-and-erase restored

[Yaraslaut]

The finding

tests/test_strand_race.cpp exists for one purpose: to catch the reintroduction
of StrandExecutor's per-key serialisation race. Its own header comment says so
at length, and it is the test the strand.hpp comments at :81 and :167 point
a future reader at. It does not detect that race's reintroduction.

I reintroduced the exact pre-fix shape the comments describe — the two-step
"flip running under strand->mtx, then erase under _mapMtx" drain — and ran
the suite under ThreadSanitizer. The dedicated test passed every time. What
caught it was an unrelated Bridge test, and only intermittently.

What I ran

Worktree at 7a343e6f plus the #660 change (which does not touch the
locking). The mutation, in scheduleNext's drain block:

// MUTANT: two-step decision, the pre-fix shape.
{
    std::scoped_lock const strandLock{strand->mtx};
    more = !strand->pending.empty();
    if (!more) {
        strand->running = false;
    }
}
std::scoped_lock const mapLock{_mapMtx};
if (!more) {
    auto iter = _strands.find(key);
    if (iter != _strands.end() && iter->second == strand) {
        _strands.erase(iter);
    }
} else {

Built with the clang-tsan preset (clang 22.1.8, -DMORPH_BUILD_NET=ON -DMORPH_BUILD_OFFLINE_SQLITE=ON), TSAN_OPTIONS=suppressions=cmake/tsan.supp.
scripts/check_sanitizer_instrumentation.sh build/clang-tsan tsan reports
6 ctest binaries all carry __tsan_ symbols (0 allowlisted), so this is not an
uninstrumented run.

The dedicated test, ten consecutive runs against the mutant:

$ for i in 1..10; do TSAN_OPTIONS=... ./build/clang-tsan/tests/morph_tests "[race]"; done
All tests passed (41 assertions in 2 test cases)
All tests passed (41 assertions in 2 test cases)
All tests passed (41 assertions in 2 test cases)
All tests passed (41 assertions in 2 test cases)
All tests passed (41 assertions in 2 test cases)
All tests passed (41 assertions in 2 test cases)
All tests passed (41 assertions in 2 test cases)
All tests passed (41 assertions in 2 test cases)
All tests passed (41 assertions in 2 test cases)
All tests passed (41 assertions in 2 test cases)

The whole [strand] tag is no better — 12 test cases, 526 assertions, all pass
against the mutant.

What did catch it, in the full ctest --preset clang-tsan sweep
(1801 tests, one failure):

The following tests FAILED:
	1155 - morph::bridge::Bridge: concurrent executeVia under repeated switchBackend resolves every morph::async::Completion (Failed)

and its report names the mechanism exactly — two strand lambdas for one key
running the model concurrently:

WARNING: ThreadSanitizer: data race (pid=490650)
  Write of size 4 at 0x72100001407c by thread T4:
    #0 (anonymous namespace)::LoadCountModel::execute(...) tests/test_concurrency_invariants.cpp:227:15
    ...
    #11 morph::exec::detail::StrandExecutor::scheduleNext(...)::'lambda'()::operator()() const include/morph/core/strand.hpp:225:17
  ...
SUMMARY: ThreadSanitizer: data race tests/test_concurrency_invariants.cpp:227:15 in (anonymous namespace)::LoadCountModel::execute(...)

It is intermittent: a single run of that test passed, and a
--repeat until-fail:60 sweep hit it on the 3rd iteration.

Why the dedicated test misses it

Inferred from reading, not measured. test_strand_race.cpp detects overlap with
its own counter — every task bumps an std::atomic in-flight count on entry and
drops it on exit, and the test fails if it ever exceeds 1. Two things follow.
The counter is atomic, so TSan sees no race on it and the sanitizer has nothing
to report however badly the strand misbehaves; and the tasks are empty, so two
concurrently-running tasks have to overlap inside a window of a few instructions
for the count to be observed above 1. The test that does catch it has tasks that
touch ordinary non-atomic model state, which is what gives TSan an object to
report on — detection there does not depend on catching the overlap in the act.

Verification status

• Reproduced: the ten-run result and the TSan report above are real output
  from this workstation (x86-64 Linux, clang 22.1.8, glibc), on the revision
  named above.
• Not verified: whether the dedicated test would catch it given far more
  iterations, a different core count, or a different machine. Ten runs is
  evidence that it does not catch it reliably, not proof that it never can.
• Not verified: the Kanban / ThreadSanitizer CI leg, which needs Qt and the
  ladder; only Linux / clang-tsan was reproduced locally.
• Inferred, not measured: the explanation above for why it misses.

Why this matters

strand.hpp's comments treat the drain-and-erase atomicity as protected — the
spec says so too (docs/spec/core/executor.md, "The per-key serialisation
invariant"). Anyone changing that code and running the strand tests will be told
the invariant holds when it does not. The actual guard is a Bridge test that
names neither StrandExecutor nor the invariant, catches it only sometimes, and
could be rewritten by someone with no idea it is load-bearing.

What would change the verdict

• Close as fixed when a mutation of the drain-and-erase shape — the one
  above will do — makes a named strand test fail, reliably enough to be worth
  relying on, and that is demonstrated by running it.
• Close as invalid if the mutation above turns out not to reintroduce the
  original defect at all, in which case the finding is instead that the comments
  at strand.hpp:81/:167 describe a shape that was never the broken one.

Found while measuring morph#660; filed rather than folded in, per AGENTS.md.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Triage: valid — and reading the test explains why it misses, which the issue does not yet say

I have not reproduced the TSan result (no TSan run here), so the measurement stands as the filer's. But the test's design is readable, and it accounts for the finding:

constexpr int kThreads = 8;
constexpr int kPostsPerThread = 400;
constexpr int kIterations = 20;
…
// Very short tasks maximise the drain/re-arm interleaving that triggered the bug.

Eight threads posting 400 tasks each, continuously, onto one key. That is sustained saturation — and the defect needs the opposite. The two-step drain races when a strand reaches empty while a new post is arriving: the slot is erased, the arriving post creates a second Strand for the same key, and now two exist. Under 3200 back-to-back posts from eight threads the queue essentially never reaches empty, so the drain-and-erase path the bug lives on is rarely taken at all.

The comment claims short tasks "maximise the drain/re-arm interleaving". They maximise re-arm; they do not produce the quiet moment a drain needs. That is consistent with the filer's other observation — that an unrelated Bridge test, with lower and burstier load, is what caught it, on the 3rd of 60 repeats.

Second, narrower point: the test asserts the symptom (REQUIRE(maxInFlight.load() == 1)), not the defect. Two Strand objects can exist for one key without their tasks ever overlapping in wall-clock time — the data race is real and TSan-visible while maxInFlight stays at 1 throughout. So even in the runs where the bug is provoked, this assertion can pass.

triage: valid, bug, area: core.

Why this is worth more than its own fix

docs/spec/concurrency_and_lifetimes.md is this repository's most-cited document, and "a strand never runs two tasks for one key" is the invariant most of it rests on. The named regression test for that invariant does not detect its regression. That is AGENTS.md's headline failure mode — a control that reports success while measuring nothing — sitting on the load-bearing contract, and it was found only incidentally, because #660 required proving TSan could see the hazard before trusting its silence.

For whoever takes it

• Reproduce the miss first. Apply the two-step-drain mutant, run this test under TSan, confirm it passes. If it now fails, the finding is stale and that is worth knowing before rewriting anything.
• The fix is load shape, not load volume. Add bursts with gaps — post a handful, let the strand drain, post again — rather than more threads or more posts. The current parameters are tuned in exactly the wrong direction and turning them up will make it worse.
• Assert the defect, not only the symptom. maxInFlight == 1 should stay, but the test should also be run under TSan in a configuration that fails on a reported race; Linux / clang-tsan is the leg that does that, and 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) #671's lane demonstrated it can see this specific hazard when the shape is right.
• Mutation-test the result. A rewritten test that does not fail against the two-step drain has not fixed anything, and this ticket exists precisely because that check was never run.

What would change the verdict

Close as invalid if the mutant does fail this test on a machine with different timing — which would make this a flaky-detection question rather than a blind-test one. The filer got 10/10 passes; a second opinion on different hardware would settle it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Reproduced, on this branch's base 7a343e6f, x86-64 Linux, clang 22.1.8, clang-tsan preset, TSAN_OPTIONS=suppressions=cmake/tsan.supp, with the two-step drain restored exactly as this ticket describes. The finding is not stale.

run 1..10: PASS   All tests passed (41 assertions in 2 test cases)
=== [race] : 10 pass / 0 fail out of 10 ===

The binary carries 173 __tsan_ symbols, so this is not an uninstrumented run. (scripts/check_sanitizer_instrumentation.sh build/clang-tsan tsan refuses a build tree holding a single binary -- only examined 1 binaries ... too few for this check to mean anything -- so the symbol count was read directly, since only morph_tests was built.)

PR #673 adds a second case beside the load test rather than replacing it, on the shape this ticket's triage argued for: a pilot task posted alone on the key publishes a round number as its last act, and four chaser threads spin on that publication and post the instant it flips -- into the drain block that follows the pilot's body. Rounds are separated by a full quiescence wait, so the strand genuinely empties. Three detectors: the maxInFlight symptom (kept), plain non-atomic state that gives TSan an object to report regardless of overlap, and per-producer FIFO, which an orphaned strand breaks with no overlap at all.

Mutation result, with the parameters as committed:

clang-tsan,  mutant, 10 runs        :  0 pass / 10 fail   (2-3 TSan races/run)
clang-debug, mutant, 15 runs        :  0 pass / 15 fail   (no sanitizer)
clang-tsan,  mutant, taskset 4 CPUs :  0 pass / 10 fail
clang-tsan,  mutant, taskset 2 CPUs :  0 pass / 10 fail
the OLD case, same mutant binary    : 10 pass /  0 fail

and on the fixed tree [race] is 20/20 under clang-tsan and 20/20 under clang-debug. include/morph/core/strand.hpp is untouched (PR #671 holds it); the mutant was applied and reverted from a byte-identical backup, md5-verified.

Not verified: the Kanban / ThreadSanitizer leg, and every non-Linux leg. The 2-CPU pinned run is the closest stand-in here for a four-core runner.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

The mutation, re-run against today's strand — this fix is still under test

Recording the numbers here as well as on #680, because this is where the fix
lives and this is the ticket a future reader will check.

#673's mutation table was measured against the std::deque-based strand.
#671 replaced that container with PendingQueue hours later, so the proof
predated the implementation it guards (#680). It has now been re-run against
the current strand.

Reproduced on 95aa5686, x86-64 Linux, 12 logical CPUs, clang 22.1.8,
cmake --preset clang-tsan, TSAN_OPTIONS=suppressions=cmake/tsan.supp.
Instrumentation confirmed on the exact binary
(nm -C … | grep -c '__tsan_' → 173 defined, 0 undefined; 44 __tsan_*
calls inside scheduleNext's own disassembly).

The mutant is the pre-fix two-step drain restored in the current
scheduleNext: running flips under strand->mtx alone, the lock is
released, and the erase happens in a separate _mapMtx critical section.
post() untouched; ++_inFlight for the re-arm kept under _mapMtx before
this run's decrement, so the only defect introduced is the drain/erase
atomicity one.

case	binary	runs	pass	fail
test_strand_race.cpp:167 "…keeps one strand per key when a post races the drain"	mutant	10	0	10
test_strand_race.cpp:44 "…never runs two tasks for one key concurrently under contention"	mutant	10	10	0
[race] (all three cases)	reverted 95aa5686	10	10	0

Every mutant run failed identically in iteration 0, with three TSan reports:

tests/test_strand_race.cpp:290: FAILED:  CHECK( *lowest == kExpected )
  with expansion:  7795 (0x1e73) == 7800 (0x1e78)
tests/test_strand_race.cpp:294: FAILED:  CHECK( outOfOrder.load() == 0 )
  with expansion:  100 == 0
tests/test_strand_race.cpp:295: FAILED:  REQUIRE( maxInFlight.load() == 1 )
  with expansion:  2 == 1
  with message:    iteration 0: completed 7800 of 7800

assertions: 6 | 3 passed | 3 failed
ThreadSanitizer: reported 3 warnings

with both race stacks arriving through scheduleNext on two different pool
threads — two strands for one key, which is the defect and not merely its
symptom:

WARNING: ThreadSanitizer: data race
  Write of size 4 …  by thread T2:  tests/test_strand_race.cpp:210:22
    #6 morph::exec::detail::StrandExecutor::scheduleNext(…)  include/morph/core/strand.hpp:225:17
  Previous write of size 4 … by thread T3:  tests/test_strand_race.cpp:210:22  (identical frames)

Not measured: no non-sanitizer leg, no 4/2-CPU sweep, no non-Linux
platform. #673 covered those against the old strand; only the TSan leg was
re-run here. Full detail, including the exact mutant diff, on #680.

Nothing was pushed — the mutation was local and reverted, and [race] is
green 10/10 on unmutated 95aa5686.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW