Repository navigation
A zero budget is not an unmeetable one — the identity-read timeout test raced its own query - #976
Conversation
…rce that cannot emit LoadDbRoles_WithUnmeetableBudget_IsUnavailable_NotEmptyRoles passed CI on its own PR and then failed on main. It was a race, not a regression. IdentityRead.Bounded applies .Timeout(budget, …), and that timer is SCHEDULED — while a warm cached synced query emits SYNCHRONOUSLY during Subscribe. With TimeSpan.Zero the value therefore beats the timer whenever the query is warm, and loses to it when it is cold. Same test, same code, opposite results depending on cache state. Driving the seam with Observable.Never removes the race rather than widening a bound: a source that cannot emit reaches no verdict by construction, so the classification under test is the only outcome possible. The assertion and its reasoning are unchanged. Renamed to say what it now pins — the budget was never the interesting part, reaching no verdict is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a nondeterministic timing race in an auth test that tried to force IdentityRead.Bounded’s timeout path with a zero timeout, which can be beaten by synchronous emissions from a warm cached source. The test is updated to deterministically exercise the “no verdict within budget” classification by using a non-emitting observable.
Changes:
- Replace the flaky “zero budget over real query” approach with
Observable.Never<T>()to guarantee the Unavailable path is reached. - Rename the test to reflect the actual behavior under test (no-verdict classification, not “unmeetable budget”).
- Expand the XML doc comment to capture the root cause and why
TimeSpan.Zerowas inherently racy here.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results (shard 2)1 877 tests ±0 1 875 ✅ ±0 5m 52s ⏱️ -4s Results for commit 9f296bd. ± Comparison against base commit d058db2. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both. |
Test Results 65 files ±0 65 suites ±0 35m 49s ⏱️ - 1m 47s Results for commit 9f296bd. ± Comparison against base commit d058db2. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both. |
Fixes a race in a test I merged an hour ago as part of #970.
What happened
LoadDbRoles_WithUnmeetableBudget_IsUnavailable_NotEmptyRoleswent green on its own PR and then red on main (b96d20aca, shard 2) — and green again on the next commit. Classic timing, not a regression.Why the budget was not unmeetable
IdentityRead.Boundedclassifies via.Timeout(budget, …). That timer is scheduled; a warm cached synced query emits synchronously during Subscribe. So withTimeSpan.Zero:Resolved→ assertion failsUnavailable→ assertion passesSame test, same product code, opposite outcomes depending on cache state. "Zero" bounds the wait, it does not prevent the answer.
The fix
Drive the seam with
Observable.Never<T>()— a source that cannot emit, so no verdict is reachable by construction and the classification under test is the only possible outcome.This removes the race rather than widening a bound. No sleep, no retry, no relaxed assertion: the assertion text and its reasoning are byte-identical, and the 50 ms budget is now arbitrary because nothing can beat it.
Renamed to
ReadThatReachesNoVerdict_IsUnavailable_NotEmptyRoles— the budget was never the interesting part.What is deliberately not changed
The product code.
Bounded's behaviour is correct: a read that answers inside its budget should resolve. The defect was in how the test tried to reach the unavailable branch.The end-to-end companion (
LoadDbRoles_WithRealBudget_ResolvesEmptyForUserWithNoGrants) keeps exercising the real workspace for the resolved path, so coverage of the live seam is not lost — only the branch that cannot be reached deterministically through a budget moved to the seam that can.Verification
MeshWeaver.Auth.Test143/143 in Release,-c Release -warnaserrorclean.Note the trap generalizes: a zero or near-zero
.Timeout(...)is never a reliable way to force the timeout branch when the source can complete synchronously. Force it with a source that cannot produce a value.