Skip to content

test: freeze the metadata budget clock in the global-budget tests - #1258

Merged
mforce merged 3 commits into
mainfrom
fix/flaky-signin-oauth-count
Oct 12, 2026
Merged

mforce merged 3 commits into
mainfrom
fix/flaky-signin-oauth-count

Conversation

@mforce

@mforce mforce commented Oct 12, 2026

Copy link
Copy Markdown
Owner

Two tests turned PRs red at random. One is fixed here. The other needs no code change, and the evidence for that is below. No issue was filed for either.

The OAuth metadata request count

ApprovedApp_IsRefreshed_PastADrainedGlobalBudget failed once in run 38181977796 (PR #1247) with Expected: 63, Actual: 65.

The cause is the global metadata budget, 60 fetches per minute (ClientMetadataDocuments.cs:43). InProcessFixedWindowCounter buckets windows on the wall clock (InProcessFixedWindowCounter.cs:115), and the test's own resolver gave its counter TimeProvider.System. The failing run ended at 23:01:00.06 after 2 s. A minute boundary fell after the drain, so the budget started over. The unapproved and unknown URLs were then fetched too: 63 + 2 = 65. No background fetch was involved. Every setup request is awaited, and the host never refreshes documents on its own.

ManyUrls_ShareOneGlobalBudget builds the same resolver and has the same exposure.

The fix: both tests build the resolver through one helper, BudgetResolver. The helper gives the counter the suite's FakeTimeProvider, a clock that does not move. OAuthFailClosedTests.FrozenHost already does this for the OAuth failure budget. The expiry check still reads the real clock, so the two stored copies the test expires are really expired. No production code changes.

Proof:

  • Moving the frozen clock forward one window between the drain and the refresh turns the test red with 63 expected and 65 actual, the same numbers as CI. The reviewer also reproduced it on the base source by waiting for the next real minute after the drain.
  • Mutation: approved clients spend the strangers' budget, so the refresh is skipped. Red, with "an approved app was refused while strangers drained the global budget".
  • Mutation: any stored copy counts as fresh, so the expired copy is served. Red, with 63 expected and 62 actual.
  • With the fix, both tests passed 20 runs out of 20, by the worker and again by the reviewer.
  • FullyQualifiedName~OAuthFailClosedTests: 81 of 81 passed, before and after merging main.

The Playwright sign-in step

The one observed sign-in failure is run 38170366723, attempt 1, shard 1/3, at a11y-live-regions.spec.ts:370. A census of 1,000 e2e-smoke runs since 2026-09-08 found this failure once. The other red runs were branch defects or Docker Hub 429 and 504 errors.

This failure coincided with a database and service stall of several minutes, plus a Playwright teardown timeout. In the logs:

  • The login POST returned 500 after 17,015 ms with an Npgsql read timeout while saving the new refresh token.
  • No successful HTTP response was logged from 20:03:06 to 20:06:05.
  • Four PostgreSQL backends reported broken pipes at 20:06:04.
  • The trace fixture's teardown ran past 45 s.

The logs point to a transient stall in the infrastructure or its resources. They do not show what caused it. This PR adds no sign-in code and no retries that would hide a failure.

Not fixed here

These problems already existed and are rated P3. The full list is in the review notes.

  • Two metadata host tests still use the live five-minute per-URL budget through MetadataHostAsync: OAuthFailClosedTests.ClientMetadataDocuments.cs:199 (FailingUrl_IsFetchedOnlyWithinItsBudget) and :274 (RemovedDocument_IsNotRevived_ByDrainingTheBudget). A five-minute boundary during their loops allows one more fetch.
  • Some tests build their own in-process counters on the system clock with daily windows, which reset at midnight UTC: DistributedRateLimiterWiringTests.cs:66 and :83 (assertions at :149, :202, :223) and OAuthClientRegistrationTests.cs:280 (assertion at :238).
  • Other assertions read live counters through DI or real Redis and can also be caught by a window rollover: OAuthFailClosedTests.cs:265, :524 and :561, RateLimitingTests.cs:74, AuthRateLimitLoggingTests.cs:105, ClientErrorReportTests.cs:153, ForwardedHeaderTrustKestrelTests.cs:131, MultiInstanceRateLimitTests.cs:121, LoginCounterKeyProbeTests.cs:139, SharedState/SharedStateNamespaceWiringTests.cs:25, and SharedState/RedisFixedWindowCounterContractTests.cs:37 and :58.
  • In the same CI log, DailyEntryLockSweep raised DbUpdateConcurrencyException from inside EF's retrying execution strategy at 20:06:04.486. LockDueDailyEntriesHandler.cs:32 saves versioned entries with retries enabled. A replay after a commit whose acknowledgement was lost would fail this way, which is the Add transient-DB resilience (EnableRetryOnFailure) on the request path #269 pattern. That replay is the most likely explanation, but it is not proven, because a real competing writer gives the same stack. The impact: the pass reports wrong counts, a misleading Tenant.WriteRefusedByDatabase alarm fires, and SimulationDataSeeder.cs:2051 treats a false zero as fully drained, so it can stop draining early.

Tests run locally

  • FullyQualifiedName~ApprovedApp_IsRefreshed_PastADrainedGlobalBudget|FullyQualifiedName~ManyUrls_ShareOneGlobalBudget, 20 runs
  • FullyQualifiedName~OAuthFailClosedTests, before and after merging origin/main

Both tests drain ClientMetadataDocuments' global budget, a fixed minute
aligned to the wall clock, on a counter reading TimeProvider.System. A run
that crossed a minute boundary started the budget over, so the unapproved
and unknown URLs were fetched too: run 38181977796 failed at 23:01:00.06
with 63 expected and 65 actual. The counter now runs on a FakeTimeProvider
that stands still, as OAuthFailClosedTests.FrozenHost already does; expiry
still reads the real clock.
@mforce mforce added this to the Platform hardening milestone Oct 12, 2026
@mforce
mforce merged commit 83b6552 into main Oct 12, 2026
18 checks passed
@mforce
mforce deleted the fix/flaky-signin-oauth-count branch October 12, 2026 13:53
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