Flaky test: SemaphoreTest.TryAcquireForWithTimeout fails ~1 in 3 runs in isolation

[drsnuggles8]

Describe the bug
SemaphoreTest.TryAcquireForWithTimeout (SemaphoreTest.cpp:57) fails intermittently. Unlike #625 this is not order-dependent — it fails in isolation, roughly 1 run in 3, with nothing else in the process:

build\OloEngine\tests\Debug\OloEngine-Tests.exe --gtest_filter=SemaphoreTest.TryAcquireForWithTimeout
[  PASSED  ] 1 test.
[  PASSED  ] 1 test.
[  FAILED  ] SemaphoreTest.TryAcquireForWithTimeout (1 ms)

It surfaced during a full-suite run for #691 Phase 3 (heap-off configuration), where it appeared once and did not recur on the next run of the same binary.

Root cause (unknown — and the two candidates want different fixes)
This is a timing assertion around TryAcquireFor, so it is one of:

1. A test-side timing assumption. The test asserts something about elapsed time against a timeout, and the tolerance is too tight for a loaded machine. This box runs three GitHub Actions runners for another repository alongside development, so scheduler jitter is routine and the fix is a wider tolerance (or asserting a lower bound only).
2. A real imprecision in the primitive. TryAcquireFor may return early — or report the wrong outcome — under contention. That would be an engine bug in a threading primitive, not a test bug, and it would matter well beyond this test.

Worth resolving which before "fixing" it. Widening the tolerance is the obvious move and would make the symptom go away in either case — including case 2, where it would bury a genuine bug in a synchronisation primitive. Establish which one it is first.

To Reproduce

build\OloEngine\tests\Debug\OloEngine-Tests.exe --gtest_filter=SemaphoreTest.TryAcquireForWithTimeout

Repeat ~5 times. Observed 2 passes / 1 failure in three consecutive isolated runs. A loaded machine makes it likelier.

Expected behavior
A deterministic test, or one whose tolerance is explicitly justified against scheduler jitter. A suite that fails 1-in-3 on an unrelated axis trains everyone to ignore red — which is exactly how the #691 depth-contract bug survived several rounds of "the suite is green".

Suggested fix

1. Instrument the failing assertion to print the measured elapsed time versus the expected bound, so a failure says how far off it was rather than just that it was off.
2. From that, decide between (a) widening/one-siding the tolerance with a comment stating the jitter budget, or (b) fixing TryAcquireFor.
3. If it stays timing-sensitive by nature, consider asserting only the direction that cannot be jitter (e.g. "did not return before the timeout") rather than a two-sided window.

Not caused by #691. Confirmed: the branch's diff touches no threading code, and the failure reproduces in isolation on a build of that branch as well as appearing in only one of several full-suite runs.

Score

capability: 1
craft: 1
stability: 3
decay: 1
effort: 1
confidence: 0.8
learning: 2
fun: 1
kano: table-stakes
blocked_by: []
blocks: []

[coderabbitai]

🔗 Related PRs

#305 - ci(physics): retry flaky PhysicsLayerFilteringTest + harden JoltJobSy… [merged]
#335 - fix(tests): de-flake issue #324 CI failures at their real root causes [merged]
#355 - fix(memory): acquire loads for lock-free allocator page publish/consu… [merged]
#484 - Feature/deterministic fixed timestep [merged]
#571 - fix(renderer): align RendererProfiler frame/CPU/GPU-wait measurements [merged]

---

🧪 Issue enrichment is currently in open beta.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[drsnuggles8]

Root cause: test-side, not a primitive bug

Resolved which candidate this was, per the issue's own request to establish that before "fixing" it.

FWindowsSemaphore::TryAcquireFor (OloEngine/src/Platform/Windows/WindowsSemaphore.h) is a single WaitForSingleObject call on a real Win32 kernel semaphore handle. That class of wait has no spurious-wake state — it can only return WAIT_OBJECT_0 or WAIT_TIMEOUT — unlike a futex/WaitOnAddress-based wait (e.g. FPlatformManualResetEvent, which does need and does have a spurious-wake re-wait loop in WaitForSlow/WaitUntilSlow). So it structurally cannot report "not acquired" before its deadline due to a logic bug in our code.

The actual defects were in the test:

1. duration_cast<std::chrono::milliseconds> truncates toward zero — a genuine 4.9ms wait reads as 4 and trips a >= 5 bound.
2. The bound was fragile in practice when only one direction is ever meaningful for this primitive: scheduler jitter can only delay a correctly-implemented wait, never shorten it.

Verified empirically, not just by code reading, per the issue's own caution against widening tolerance on assumption alone:

• 500+ isolated --gtest_repeat runs with the original assertion — 350 on an idle machine, 100 under artificial heavy CPU contention (20 concurrent CPU-bound processes, to reproduce the scheduler jitter this box normally sees from its CI runners) — zero reproductions of the reported failure. Elapsed time only ever grew under load (up to 25ms against the 10ms request) and never fell below 9ms.
• This rules out candidate 2 (an early-return bug in the primitive) with reasonable confidence, and is consistent with candidate 1 (test-side timing fragility), specifically the truncation + tight-bound combination above rather than "just widen the tolerance."

Fix applied (measure in µs, assert one-sided, document the jitter budget, print the measured value on failure):

• SemaphoreTest.TryAcquireForWithTimeout
• SemaphoreTest.TryAcquireUntilWithTimeout — previously asserted only EXPECT_FALSE(Result) with no elapsed check at all, which would have silently passed through an early return
• ManualResetEventTest.WaitForUnset — same class of flaky lower-bound assertion, found via a scan for the pattern elsewhere in OloEngine/tests/

Post-fix proof: 1050 more repeat runs (600 idle + 450 under the same artificial heavy load) across all three fixed tests → 0 failures. Full suite run (5693 tests): 5685 passed, 6 expected skips, 2 pre-existing unrelated GPU visual-evidence failures (AtmosphereVisualEvidenceTest, EASUVisualEvidenceTest — out of scope for this batch, atmosphere goldens is tracked separately as #754).

Reusable lesson written up in docs/agent-rules/timed-wait-test-assertions.md (linked from CLAUDE.md) so a future flaky timed-wait test doesn't require re-deriving this.

Fixed in the linked PR.