test_strand_race.cpp's drain deadline can fail under load, reading as a strand-serialisation failure

[Yaraslaut]

Summary

tests/test_strand_race.cpp's "StrandExecutor never runs two tasks for one key
concurrently under contention" can fail on its drain deadline, not on the
invariant it exists to pin. Line 71 is

constexpr int kExpected = kThreads * kPostsPerThread;   // 8 * 400 = 3200
for (int i = 0; i < 2000 && completed.load() < kExpected; ++i) {
    std::this_thread::sleep_for(std::chrono::milliseconds(1));
}

REQUIRE(completed.load() == kExpected);          // <-- this one failed
// The core invariant: at most one task for this key ever runs at once.
REQUIRE(maxInFlight.load() == 1);

That is a fixed ~2 s wall-clock budget for 3200 strand-serialised tasks, repeated
over kIterations = 20, on a 4-thread pool with 8 producer threads. On a loaded
machine the budget can expire with work still queued — a scheduling outcome, not
a serialisation defect. The invariant assertion (line 73, maxInFlight == 1) did
not fail.

Verification status

Observed once, not reproduced. Seen on branch scenario-corpus-both-axes
(worktree at 2a035fe8 plus the #351 fix) in a full morph_tests run while the
machine was saturated — a parallel full-suite run under ThreadSanitizer and a
Ninja build were running at the same time:

-------------------------------------------------------------------------------
StrandExecutor never runs two tasks for one key concurrently under contention
-------------------------------------------------------------------------------
tests/test_strand_race.cpp:26
...............................................................................

tests/test_strand_race.cpp:71: FAILED:

test cases:  1279 |  1278 passed | 1 failed
assertions: 21373 | 21372 passed | 1 failed

Not reproduced since: 10/10 isolated runs of morph_tests "[strand][race]"
passed (40 assertions each), and two further full-suite runs passed 1279/1279
(21404 assertions) once the machine was idle. The failing seed was not captured;
it would not matter, since the failure is timing- rather than seed-dependent.

Unrelated to the change it was seen under (issue #351, the execute-ordering
ticket in remote.hpp): this test touches only StrandExecutor and
ThreadPoolExecutor, neither of which that change modifies.

Why it is worth recording

The gap between the two assertions matters. maxInFlight == 1 is a real
invariant and its failure would be a genuine bug; completed == kExpected is a
statement about how fast the host machine is, and it fails first. A CI flake on
line 71 reads, to whoever triages it, as "the strand serialisation test failed" —
which is exactly the wrong prior for the actual cause.

Suggested direction

Wait on a condition rather than a fixed millisecond budget (a condition_variable
notified by the task that brings completed up to kExpected), or at minimum
report the deficit in the assertion (completed vs kExpected) so a timeout is
distinguishable from a lost task at a glance. tests/test_support.hpp's
waitUntil already carries a budget convention the rest of the suite uses.

What would change the verdict

• If maxInFlight == 1 (line 73) is ever seen failing, this is a different and
  much more serious issue and should be re-filed as such.
• If the drain assertion cannot be made load-independent — e.g. because a lost
  task is a real failure mode worth catching here — then the fix is a longer
  budget plus a message stating the deficit, not a condition variable.

[Yaraslaut]

Triage: valid — the premise needs no reproduction, and there are two refinements to the suggested direction

Premise: confirmed by reading, which is enough here. tests/test_strand_race.cpp:66-73 is a fixed budget:

constexpr int kExpected = kThreads * kPostsPerThread;   // 8 * 400 = 3200
for (int i = 0; i < 2000 && completed.load() < kExpected; ++i) {
    std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
REQUIRE(completed.load() == kExpected);
REQUIRE(maxInFlight.load() == 1);

"A 2 s wall-clock budget for 3200 strand-serialised tasks across 20 iterations can expire on a loaded machine" is a structural fact about that loop, not a hypothesis, so unverified would be the wrong label even though the failure was seen once and not reproduced. Nothing needs to be re-run to know the assertion is load-dependent.

Two corrections to the suggested direction:

1. waitUntil is not a fix. The issue suggests it "already carries a budget convention the rest of the suite uses". It does — and it is the same budget: tests/test_support.hpp:211 defines kDefaultWaitBudget{2000} ms. Adopting waitUntil verbatim changes the idiom and nothing about load-independence. It only helps if it is passed a larger explicit budget, which makes it the "longer budget" option rather than an alternative to it.

2. The real cost is not the misdiagnosis — it is that the invariant stops being measured. Both assertions are REQUIRE, and the drain one comes first. Catch2 aborts the test case on a failed REQUIRE, so when the drain deadline expires, line 73 — maxInFlight == 1, the only reason this test exists — never executes. A loaded machine converts "the strand serialisation test" into "no strand serialisation check ran", reported as a strand failure.

That makes the cheapest useful change smaller than either option in the issue: make the drain a CHECK with the deficit in its message and leave the invariant a REQUIRE. The case then reports "drain incomplete (2871/3200)" and still evaluates maxInFlight over the tasks that did run, which is exactly the distinction the issue asks for — the diagnosis and the invariant become independent. A condition variable is still the better answer for the budget itself, and the two compose.

The re-open conditions are right as written; the first one (a maxInFlight == 1 failure being a different and much more serious issue) is worth keeping prominent, and note that today a drain timeout prevents that assertion from ever being the one that fails.

What would change this verdict: evidence that the drain loop cannot expire under load — e.g. that the budget scales with the host. It is a literal 2000 iteration count of 1 ms sleeps.