tests: the #668 race case's detection was proven against the old strand, never against PendingQueue

[Yaraslaut]

Filed during the landing sweep after #673 merged. This is a question about a control, not a reported failure — I have not measured it either way, and saying so is the point.

The situation

Two changes landed hours apart and interact:

• 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 (735cc2e9) replaced Strand's std::deque with PendingQueue, which holds the head task inline and builds a deque only on overflow.
• tests/ci: a race regression test that detects its race, and a Catch2 version that is checked rather than asserted (fixes #668, fixes #666, refs #667) #673 (95aa5686) added tests/test_strand_race.cpp's second case, "StrandExecutor keeps one strand per key when a post races the drain", whose whole purpose is to detect the two-step drain-and-erase race that the pre-existing case could not.

Both are now on master:

$ grep -c PendingQueue include/morph/core/strand.hpp
2
$ grep -n TEST_CASE tests/test_strand_race.cpp
 44: … never runs two tasks for one key concurrently under contention
167: … keeps one strand per key when a post races the drain

The gap

#673's evidence that its new case detects the race is a mutation result:

0 pass / 10 fail against the two-step-drain mutant under clang-tsan, 0/15 without a sanitizer, unchanged at 4 and 2 CPUs — against 10/10 passes for the old case on the same mutant binary.

That was measured against the old strand, whose queue was a std::deque reconstructed on every dispatch. PendingQueue changes the allocation and drain shape of exactly the code path the test provokes.

What has been verified since: #673 was rebased onto post-#671 master and its CI passed. That establishes the test still passes. It does not establish that it still fails when it should — and those are different properties. A green run cannot distinguish "the test works" from "the test no longer provokes the window".

This is #668's own defect one turn later: #668 existed because the original case passed 10/10 against the mutant while looking like a regression test.

Verification status: not measured, in either direction

I have run nothing. What I have is the structural fact that the mutation predates the implementation it now guards, plus the reasoning above. I am not claiming the test is broken — I am claiming nobody knows.

What would resolve it

Restore the two-step drain in the current scheduleNext, i.e. mutate PendingQueue's drain rather than the old deque's, and run [race] under the clang-tsan preset. Expected: the new case fails, the old one passes. Either outcome is worth recording:

• It fails → detection survives the container change, and this issue closes with the measurement, which is the thing tests/ci: a race regression test that detects its race, and a Catch2 version that is checked rather than asserted (fixes #668, fixes #666, refs #667) #673 could not have provided.
• It passes → the test no longer detects its race, 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 #668 is not actually fixed, and it should be re-opened.

Confirm instrumentation before believing a green run — #675 is open about check_sanitizer_instrumentation.sh refusing single-target trees, so a symbol count may be the practical route.

What would change the verdict

Close as invalid if #673's lane did re-run the mutation against PendingQueue and simply did not say so — its hand-back reports the mutation table without naming which strand it ran against, and the branch was rebased mid-flight, so this is possible. A comment from that work, or a rerun, settles it.

Related

• 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 #668 — the ticket the new case exists to fix.
• 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 — PendingQueue.
• core: StrandExecutor rebuilds a model's Strand on every serial dispatch -- 4 allocations and 752 of the 1995 bytes per local call #660 / StrandExecutor still rebuilds the map node and the Strand on every serial dispatch -- 152 bytes, 2 allocations, and removing them needs a deregistration hook #670 — the allocation work that motivated PendingQueue.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Measured: the new case still detects its race against PendingQueue

Verdict: detection survives the container change. The case at
tests/test_strand_race.cpp:167 fails 0 pass / 10 fail against the
two-step-drain mutant built on the current, PendingQueue-based strand,
while the older case at :44 passes 10/10 on the same binary. That is the
same spread #673 reported against the old std::deque strand, so nothing was
lost when #671 landed.

Nothing was pushed. This is a measurement, not a change: the mutation was
applied locally and reverted.

Verification status

Reproduced, not inferred. Everything below was run; nothing is read off the
code.

• Revision: 95aa5686 (master), clean tree, no commits made.
• Host: x86-64 Linux, 12 logical CPUs, clang 22.1.8, libstdc++ 16.
• Build: cmake --preset clang-tsan (CMAKE_BUILD_TYPE=Debug,
  AF_SANITIZER=tsan), target morph_tests.
• TSAN_OPTIONS=suppressions=cmake/tsan.supp on every run.

The mutant

The pre-#668 two-step drain restored in the current scheduleNext — the
{_mapMtx, strand->mtx} block split so running flips under strand->mtx
alone, the lock is released, and the erase happens in a separate _mapMtx
critical section. post() was left exactly as master has it, and the
++_inFlight for the re-arm was kept under _mapMtx before this run's own
decrement, so the destructor invariant is untouched and the only defect
introduced is the drain/erase atomicity one #668 was about.

             bool more = false;
+            // MUTANT (morph#680): the pre-#668 two-step drain restored.
             {
-                std::scoped_lock const mapLock{_mapMtx};
                 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 {
                     ++_inFlight;
                 }
             }

Note that the mutation is expressible against PendingQueue without changing
the defect's nature: the drain decision reads pending.empty() exactly as
before, so the split is purely a locking change, which is what #668 was.

Instrumentation, checked before believing anything

scripts/check_sanitizer_instrumentation.sh was not used (#675 — it refuses
single-target trees). Symbol count on the binary that produced every number
below:

$ nm -C build/clang-tsan/tests/morph_tests | grep -c '__tsan_'
173
$ nm -uC build/clang-tsan/tests/morph_tests | grep -c '__tsan_'
0

173 defined, 0 undefined — the runtime is statically linked in, not merely
referenced. And the code under test is instrumented, not just the binary:

$ objdump -d build/clang-tsan/tests/morph_tests \
  | awk '/StrandExecutor.*scheduleNext/,/^$/' \
  | grep -c '__tsan_func_entry\|__tsan_read\|__tsan_write'
44

The same count, 173, holds for both the mutant and the reverted binaries, so
the two legs below were run under identical instrumentation.

Results

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

The failure is identical in all 10 mutant runs — same three assertions, same
three TSan reports, always in iteration 0:

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

test cases: 1 | 1 failed
assertions: 6 | 3 passed | 3 failed

ThreadSanitizer: reported 3 warnings

The TSan reports land on the case's plain state, with both stacks arriving
through scheduleNext on two different pool threads — i.e. two strands for one
key, which is the defect rather than the symptom:

WARNING: ThreadSanitizer: data race (pid=622781)
  Write of size 4 at 0x721800008400 by thread T2:
    #0 …::$_2::operator()(int, int) const  tests/test_strand_race.cpp:210:22
    #1 …  tests/test_strand_race.cpp:251:71
    …
    #6 morph::exec::detail::StrandExecutor::scheduleNext(…)::'lambda'()::operator()() const
         include/morph/core/strand.hpp:225:17
    #11 morph::exec::ThreadPoolExecutor::loop()  include/morph/core/executor.hpp:106:17

  Previous write of size 4 at 0x721800008400 by thread T3:
    #0 …::$_2::operator()(int, int) const  tests/test_strand_race.cpp:210:22
    … (identical frames, other pool thread)

SUMMARY: ThreadSanitizer: data race tests/test_strand_race.cpp:210:22

:210 is cell += 1 over the plain cells vector; the other two reports are
:212 (++executedPlain) and one more on the same plain state.

The old case's 10 runs on the mutant binary reported 0 TSan warnings and
All tests passed (40 assertions in 1 test case) every time — matching the
comment already in the file, now re-confirmed against PendingQueue rather
than the deque.

Detector spread (10/10 mutant runs, all identical)

Fired every run: *lowest == kExpected, outOfOrder == 0,
maxInFlight == 1, plus 3 TSan races.
Never fired: completed == kExpected, executedPlain == kExpected,
*highest == kExpected. The redundancy is doing no harm — three independent
detectors firing 10/10 is a comfortable margin — but the sanitizer-free
"lost updates to executedPlain" reading is not the one that catches this
mutant on this host; cells' minimum is.

What I did not measure

• No non-sanitizer leg. tests/ci: a race regression test that detects its race, and a Catch2 version that is checked rather than asserted (fixes #668, fixes #666, refs #667) #673 reported 0/15 without a sanitizer; I ran only
  clang-tsan. The Catch2 assertions that fired (*lowest, outOfOrder,
  maxInFlight) are all sanitizer-independent, so a plain build would very
  likely fail too — but that is inference, not a measurement.
• No CPU-count sweep. tests/ci: a race regression test that detects its race, and a Catch2 version that is checked rather than asserted (fixes #668, fixes #666, refs #667) #673 reported the result unchanged at 4 and 2 CPUs;
  I ran only on all 12.
• No other platform. x86-64 Linux / clang 22.1.8 only.
• No re-run of tests/ci: a race regression test that detects its race, and a Catch2 version that is checked rather than asserted (fixes #668, fixes #666, refs #667) #673's own numbers — this measures the question tests: the #668 race case's detection was proven against the old strand, never against PendingQueue #680 asks
  (does it still fail when it should, on today's strand), not a replication of
  the old table.

What would change the verdict

Re-open this if the drain path in scheduleNext or PendingQueue's
empty()/pop() contract changes again without the mutation being re-run, or
if the new case's fail rate on the mutant ever drops below 10/10 on a
configuration where it previously held. A fail rate that becomes intermittent
is the early warning that the rendezvous is drifting out of the window.

Closing: #668's fix is verified against the implementation it now guards, and
the numbers are on #668 as well so the next reader finds them where the fix
lives.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Closed by the measurement above: 0 pass / 10 fail for the new case against the two-step-drain mutant on the current PendingQueue strand, 10/10 passes for the old case on the same binary, and 10/10 green on unmutated 95aa5686. Detection survives #671. Nothing pushed.

[Yaraslaut]

Runner: accepted, and the cleanup verified

Confirmed independently that the verification left nothing behind — which matters, because the method here was to mutate a shipped header:

$ git ls-remote --heads origin 'laneVERIFY*'
(empty)
$ git status --porcelain include/morph/core/strand.hpp tests/test_strand_race.cpp
(empty)
  master head: 95aa5686

No branch, no commits, no residue. An empty diff was the right deliverable and it is what arrived.

The answer is the one I hoped for and could not assume: the #668 case still fails against the two-step-drain mutant of the current PendingQueue strand — 0/10 — while the older case at :44 passes 10/10 on the same mutant binary. The comparison is what makes it evidence rather than a pass/fail, and it reproduces the relation #673 reported against the old deque strand.

Two details that make this more than a repeat of #673's table:

• The mutant is the same defect, not a lookalike. The drain decision still reads pending.empty(), so splitting the {_mapMtx, strand->mtx} block isolates exactly the locking change 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 #668 was about. Keeping ++_inFlight under _mapMtx before the decrement means the destructor invariant was not disturbed — i.e. the mutation did not smuggle in a second defect that could have produced the failure for the wrong reason.

• Instrumentation was proven before the result was believed, which is the whole reason this issue existed:

  nm -C  … | grep -c '__tsan_'   -> 173
  nm -uC … | grep -c '__tsan_'   ->   0   (statically linked, not merely referenced)
  objdump -d … scheduleNext … | grep -c '__tsan_(func_entry|read|write)' -> 44
  

  The undefined-symbol count being zero is the part that distinguishes "instrumented" from "references the runtime", and the 44 instrumented accesses inside scheduleNext specifically is what rules out "instrumented, but not where it matters".

The observation I would not want lost, recorded above by the lane and deliberately not filed: of the six detectors, *lowest, outOfOrder and maxInFlight fired 10/10 while completed, executedPlain and *highest never fired on that host. Three independent detectors at 10/10 is a comfortable margin, so not filing was the right call — but the "lost updates to executedPlain" reading is not what catches this mutant on a 12-CPU box, and anyone tuning that test later should know which detectors are actually load-bearing.

Still unmeasured, as stated: no plain (non-sanitizer) build, no CPU-count sweep, no non-Linux platform. #673 covered the first two against the old strand; nobody has covered them against PendingQueue. That is a smaller gap than the one this issue closed, and it is not worth a ticket on its own.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW