Skip to content

Fix race condition in the http2-request-parser test harness - #217

Open
tokuhirom wants to merge 1 commit into
croservices:mainfrom
tokuhirom:fix-http2-request-parser-test-race
Open

tokuhirom wants to merge 1 commit into
croservices:mainfrom
tokuhirom:fix-http2-request-parser-test-race

Conversation

@tokuhirom

Copy link
Copy Markdown

Problem

In t/http2-request-parser.rakutest, the test helper runs each request's checks in a start block — off the main thread — and calls ok from there. Test is not thread safe, and this produced two distinct failures:

  1. Interleaved TAP output. The multiplexing tests use $count == 2, so two start blocks emit ok concurrently and the check 1..4 lines come out in a different order on every run.

  2. Tests leaking past their own pass. $test-completed.keep if $current-counter + 1 == $count only waits for the last request's checks. An earlier request's ok can still be in flight, so it lands after the pass $desc that should have followed it. That shifts the test numbering, and intermittently made the final throws-like subtest fail:

    not ok 60 - Unfinished header cannot be interrupted
    

Fix

Each request records its check results into a Promise instead of calling ok. Once every request has been checked, the main thread reports the results with ok, in request order.

The failure path is deliberately left alone: the fail => True case runs inside throws-like { ... }, i.e. a subtest with a plan, so emitting partial oks on timeout would corrupt its count.

Measurements

Output of 26 parallel runs of the file, compared by hash:

runs producing identical output
before 8 / 26 (17 distinct outputs)
after 23 / 26

The remaining 3 hit the pre-existing Promise.in(5) deadline under CPU contention from running 26 Rakudo processes at once — not the race. Run sequentially, the file is 10/10 identical before and after the change.

Tested on Rakudo v2026.07.

Note

t/http2-response-parser.rakutest has the same helper shape, and additionally reads/writes $counter inside the start block. Every test there currently uses $count == 1, so no two check blocks ever overlap and it is stable in practice (20/20 identical under the same parallel stress). Left untouched here to keep this PR focused; happy to follow up.

The per-request checks run in a `start` block, off the main thread, and
Test is not thread safe. With multiplexing tests (`$count == 2`) two of
those blocks emit `ok` concurrently, so the TAP output interleaved.

Worse, `$test-completed.keep if $current-counter + 1 == $count` only
waited for the *last* request's checks, so an earlier request's `ok`
could still be in flight and leak out past the `pass $desc` that should
have followed it -- shifting the test numbering and intermittently
failing the final `throws-like` subtest.

Record the check results in a Promise per request instead, and report
them with `ok` on the main thread, in request order, once every request
has been checked.

Measured over 26 parallel runs of the file: 8/26 runs produced identical
output before, 23/26 after (the remaining 3 hit the pre-existing 5s
deadline under CPU contention).

Signed-off-by: Tokuhiro Matsuno <tokuhirom@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tokuhirom pushed a commit to tokuhirom/mutsu that referenced this pull request Sep 10, 2026
… race

`cro-http`'s `t/http2-request-parser.rakutest` runs each request's checks inside
a `start` block and calls `ok` from there. `Test` is not thread-safe, so with two
concurrent HTTP/2 streams the requests interleave their TAP output and a test can
leak past the `pass` that should follow it, leaving the plan and the emitted
count disagreeing.

Mutsu's native Test provider is Rust and serializes, so the gate has never seen
this: 20/20 idle and 20/20 with three CPU burners on a 4-core box, 61/61
assertions every run. The vendored Test.rakumod is Raku code with a plain
counter, and under it the same file is 29/30 idle but 11/20 under that load.
With the MUTSU_REAL_TEST=1 flip due shortly, the gate is about to start running
the second column, where a tenth of all releases would be blocked by somebody
else's test bug. So the row comes out ahead of the flip rather than after the
first red release.

Reported upstream as croservices/cro-http#217, which records each request's
results into a Promise and reports them from the main thread in request order.

batteries-exclude.txt had exactly one bar: the file must reach outside this
machine unconditionally. This is a different shape of the same underlying
principle -- the file's verdict is not a statement about mutsu -- so the list now
names two categories, and the new one carries its own three-part bar: root-caused
to the upstream file, reported upstream with the PR named, and a written restore
condition. An entry that cannot name a fix in flight does not qualify; that is a
mutsu bug to fix.

Deliberately not a flaky-tests.txt entry: that ledger re-runs a quarantined test
up to three times, which is right for a bounded statistical flake, but the
battery harness has no retry path and a ~50% per-run rate would not converge in
three attempts if it had one.

Because --update skips excluded files too, the row cannot drift back into the
baseline on its own; deleting the entry is the only way back, which is what makes
the restore condition load-bearing.

Refs #7555 (item 3), #7667.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NX9vs4NbdKXUu6MoAaHpSz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant