Skip to content

tests: ~20 more fixed-millisecond drain budgets share test_strand_race.cpp's shape (the REQUIRE that aborts before the invariant) #396

Description

@Yaraslaut

Summary

morph#374 fixed one fixed-millisecond drain budget in tests/. The same
structural shape — poll a counter for a hard-coded number of 1–10 ms sleeps,
then REQUIRE the counter
— is repeated at roughly twenty other sites in
tests/, including two more in the two strand test files themselves. Each is a
statement about how fast the host is, asserted as if it were a statement about
the code.

Filing this as a survey rather than folding it into the #374 fix, per
AGENTS.md. The evidence for the class is a reading of the code; the evidence
that any specific one of these actually expires is weak — see below.

The sites

grep -rn "for (int i = 0; i < [0-9]* &&" tests/ finds, among others:

tests/test_strand.cpp:33:          for (int i = 0; i < 100 && completed.load() < numTasks; ++i)   // 100 x 10 ms
tests/test_strand.cpp:62:          for (int i = 0; i < 50 && completed.load() < numKeys; ++i)     //  50 x 10 ms
tests/test_strand_race.cpp:117:    for (int i = 0; i < 1000 && !ran.load(); ++i)                  // 1000 x 1 ms
tests/test_conflict_resolution.cpp:142
tests/test_client_execute_deadline.cpp:161, 189, 221, 261
tests/test_remote_step_interleaving.cpp:87
tests/test_handler_binding.cpp:57, 73, 139
tests/test_offline_integration.cpp:123
tests/test_switch_backend.cpp:131, 142, 178, 211, 231, 255, 281

Two of these have the exact fix morph#374 used available to them, because they
drain a StrandExecutor or a ThreadPoolExecutor and both destructors are
already exact barriers:

  • tests/test_strand.cpp:33 and :62~StrandExecutor blocks until
    _inFlight == 0, which (with posting stopped) means every queued task has
    run. See docs/spec/concurrency_and_lifetimes.md, "The strand model".
    Scoping the strand and dropping the polling loop makes both drains exact.
  • tests/test_strand_race.cpp:117 (ThreadPoolExecutor(0) yields a usable pool) — ~ThreadPoolExecutor drains its queue before joining, so closing the
    pool's scope before REQUIRE(ran.load()) removes that budget entirely.

The remaining sites poll for a Completion to settle rather than for an
executor to quiesce, so they have no equivalent barrier and would need a
different treatment (a condition variable, or morph::testing::waitUntil with a
stated budget). They are listed for the record, not because a single fix covers
them.

Verification status

The shape is confirmed by reading. No site in this list was reproduced
failing.

Measured on branch worktree-agent-a7504cb3b48cbffcd (base b219ce51, plus the
morph#374 fix), Linux, clang, linux-everything preset. I probed the two
tests/test_strand.cpp cases the way morph#374's own regression was probed —
pinned to two CPUs on a host already at load average ~85 — and they did not
fail:

$ sh probe.sh   # 10 x: taskset -c 10,11 morph_tests '[strand]~[race]'
test_strand.cpp runs=10 cpus=10,11 failing=0 loadavg=84.83 84.02 78.18

That is a real negative result and it should be read as one: those two budgets
have far more headroom relative to their work (20 tasks x 2 ms against a 1 s
budget; 4 tasks x 30 ms against a 500 ms budget) than morph#374's did
(3200 tasks against a 2 s budget), which is exactly why #374's expired and these
did not. For contrast, under the identical pinning the pre-fix
test_strand_race.cpp failed 5 runs out of 5:

run 1: FAILED rc=42 43s
tests/test_strand_race.cpp:71: FAILED:
with expansion:
  201 == 3200 (0xc80)
test cases: 1 | 1 failed
assertions: 1 | 1 failed

Not verified:

  • Whether any of these has ever fired in CI. I did not audit CI history.
  • The 17 non-strand sites. I did not run them under load at all; they are here
    because they share the shape, not because they were measured.

Why it is worth recording anyway

morph#374's cost was not that it flaked — it was that the drain REQUIRE came
before the invariant REQUIRE, so Catch2 aborted the case and the invariant
was never evaluated. Note assertions: 1 | 1 failed in the output above: one
assertion ran, out of the 40 that case contains. Every site in the list has the
same ordering, so each carries the same latent "the check you cared about never
ran, and the report names it anyway" failure mode. Whether the budget is
comfortable today is a separate question from whether the report would be
truthful if it were not.

What would change the verdict

  • Close it if the two test_strand.cpp drains and the
    ThreadPoolExecutor(0) drain are converted to their executors' destructor
    barriers, and the remaining sites are either given a stated budget with the
    deficit in the message or judged individually and left with a written reason.
  • Close it as invalid if someone shows these budgets are load-independent —
    they are literal iteration counts of fixed sleeps, so that would require
    showing the work they wait on is bounded by something other than the host.
  • Escalate it if any of these is seen failing in CI, which would move it
    from "shape" to "reproduced".

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: coreSubsystem: corebugSomething isn't workingtriage: validWell-framed; implement as written

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions