test(sema): make contended semaphore poll/signal test deterministic#176
Merged
ran-j merged 1 commit intoJul 21, 2026
Merged
Conversation
The "Semaphore poll/signal remains stable under host-thread contention" test created the semaphore full (init == max == 1), so SignalSema could only succeed after PollSema had already freed a slot. With no start barrier, under host-thread contention the signaler thread could run all 64 of its iterations before the poller's first timeslice, making every SignalSema legitimately return KE_SEMA_OVF and leaving signalOkCount at 0 — a false failure of "contended SignalSema should observe successful releases". An earlier investigation of the unfixed test observed this twice in 24 contended (8-way-parallel) full-suite runs and never in 20 serial runs; CI has independently hit the same assertion on the unrelated draft PR ran-j#174, confirming the trigger is host scheduling, not the code under review. Seed the semaphore with headroom (init=1, max=2) so the first PollSema and the first SignalSema each succeed regardless of scheduling order, and add a start barrier so both workers start together, maximizing the opportunity to interleave. Widen the final-count range check to the new max. The semaphore implementation is unchanged; both threads still contend concurrently on the same per-semaphore mutex.
smmathews
marked this pull request as ready for review
July 21, 2026 11:49
Contributor
Author
|
@ran-j , I'd suggest prioritizing this PR for review, as flakey tests make the CI pipeline and our agents less reliable |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The ps2xTest case "Semaphore poll/signal remains stable under host-thread
contention" (ps2xTest/src/ps2_runtime_expansion_tests.cpp) is intermittently
flaky, failing the assertion "contended SignalSema should observe successful
releases".
Root cause is in the test, not the runtime. The test creates a semaphore that
is already full (init_count == max_count == 1) and then spawns a poller thread
and a signaler thread that each run 64 iterations with no coordination between
them. Because the semaphore starts full, SignalSema has no legal way to succeed
until a PollSema has first freed a slot — it returns KE_SEMA_OVF while the
count is at max. With no start barrier, under host-thread contention the
signaler can run through most or all of its 64 iterations before the poller
gets its first timeslice, so every SignalSema legitimately returns KE_SEMA_OVF
and signalOkCount ends at 0, failing the assertion even though nothing
malfunctioned. The poller side is effectively immune (a token is always
available at t=0), which is why only the signal assertion was ever seen to
fail.
That this is a genuine, recurring flake rather than a local artifact is confirmed by the
repository's own CI: this exact test and assertion failed the linux-gcc check on draft PR #174
(workflow run 29797114266), a change that touched only single-threaded memory-card code and
could not have affected semaphore behavior. Its failing on a wholly unrelated pull request is
direct evidence that the failure is scheduling-driven, not caused by the code under review.
Both syscalls serialize on the same per-semaphore mutex, so this is not a data
race — it is a scheduling-order dependency the test does not control. Locally the
failure rate is low and host-load-dependent: an earlier dedicated investigation
of the unfixed test observed 2 failures in 24 contended (8-way-parallel)
full-suite runs and none in 20 serial runs, while a later best-effort
re-reproduction of the unfixed test over 64 contended runs did not reproduce it —
consistent with a low, load-sensitive rate that CI happened to surface on PR #174.
Fix
present and headroom below max at the start, the first PollSema and the first
SignalSema each succeed regardless of interleaving (the poller is the only
decrementer, so count >= 1 at its first call; the signaler is the only
incrementer, so count < max at its first call). This makes the test
deterministic without serializing it.
maximizing real interleaving of the concurrent mutex acquisitions. Without
it the seeding change alone could let the test pass while exercising zero
contention — one worker running all 64 iterations before the other is
scheduled — defeating what the test's name promises, so the barrier is here
deliberately to keep the test meaningful, not as scope creep.
No runtime or syscall code changes; the semaphore implementation is correct.
What the test still pins
guarded incrementer (the signaler) and one guarded decrementer (the poller),
each enforcing its own bound under the per-semaphore mutex — that range is a
boundary-guard check, not a mutex-integrity check. It would catch a guard
regression that lets the count escape [0, max]: a SignalSema that stopped
honoring max_count (count climbing toward 64) or a PollSema that stopped
honoring the zero floor (count falling toward -64). It would not catch a
broken or deleted mutex — with a single incrementer and a single decrementer
on a naturally-aligned int the count does not tear, and a lost
read-modify-write resolves in range and invisibly, so the bound would still
hold. A crash or lock corruption from the concurrent access surfaces through
the no-throw/no-crash assertions above — and, suite-wide, through no other
test crashing — not through this count range.
The live regression signals are therefore the boundary-guard bound [0, max],
the no-throw/no-crash guarantee across concurrent PollSema/SignalSema, and the
post-contention ReferSemaStatus KE_OK; the two liveness assertions are now
true by construction of the seeding (init=1, max=2) and stand as documented
invariants that the workers actually exercised both call sites, not as
independent failure detectors.
The deterministic OVF/ZERO boundary behavior — PollSema returning KE_SEMA_ZERO
when the count is empty and SignalSema returning KE_SEMA_OVF at max — is pinned
separately by the single-threaded sibling test "semaphore EE layout covers poll,
signal overflow, and status" (ps2xTest/src/ps2_runtime_kernel_tests.cpp), which
exercises those guards deterministically; the widened [0, 2] range check here
re-covers the same bound under concurrency rather than serving as this test's
primary boundary assertion.
It can no longer produce the scheduling-dependent signalOkCount == 0 false
negative, which was never a real regression signal.