telemetry: fix the flaky consent-cache TTL test that broke CI on main - #643
Merged
Conversation
TestCachedConsentResolver raced a real 20ms deadline: it primed the cache, wrote a second consent file to disk, and then asserted the cached value had not yet expired. On a loaded runner the SaveConsent write plus scheduling delay between those two calls can exceed 20ms, the cache re-reads, and the "within TTL the cached value should persist" assertion fails — which is what took down the ubuntu leg of CI on main while macOS passed. Give CachedConsentResolver an unexported constructor that takes a clock and have the test step it across the TTL boundary (ttl-1, then exactly ttl), so the boundary under test is asserted exactly instead of approximately. The exported constructor keeps its signature and gains a small test covering the real-clock wiring. Drops a 30ms sleep from the suite as a side effect.
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.
What broke
The
test (ubuntu-latest, 1.26)leg of CI failed onmain:Nothing in the merged change touched telemetry — the test is timing-dependent.
Root cause
TestCachedConsentResolverbuilt a resolver with a 20 ms TTL against the realclock, then:
resolve()→ primes the cache withenabled=trueand stampscheckedAtSaveConsent(dir, false, …)→ a file write to diskresolve()→ asserts the cachedtrueis still servedStep 2 is the problem.
CachedConsentResolverre-reads whenevernow.Sub(checkedAt) >= ttl, so the assertion in step 3 only holds if the diskwrite plus scheduler latency finishes inside 20 ms. On a loaded runner — the
ubuntu leg runs
go test ./...with coverage across packages that take 90–145 sapiece — it doesn't, the cache expires legitimately, and the test reports a bug
that isn't there. macOS passed on both attempts, which is the signature of a
margin that is too thin rather than broken logic.
Fix
Add an unexported
cachedConsentResolver(dir, ttl, clock); the exportedCachedConsentResolveris now a one-line wrapper passingtime.Nowand keepsits signature (no caller changes — 4 direct dependents, all untouched).
The test steps that clock across the boundary instead of racing it:
base + (ttl-1)→ still cached, must return the staletruebase + ttl→ expired, must re-read and returnfalseThat asserts the boundary exactly rather than approximately, and drops a
30 ms
time.Sleepfrom the suite. A newTestCachedConsentResolverUsesRealClockkeeps the exported constructor's wiring (dir + real clock) covered.
Verification
The rewritten test still catches a broken cache — checked by neutering the
implementation three ways and confirming each is caught:
>= 0)within TTL the cached value should persistif !valid)after TTL the resolver should re-read> ttlfor>= ttl)go test -race -count=3 ./internal/telemetry/...green;golangci-lint run internal/telemetry/...reports 0 issues;cmd/gortexandinternal/serverstackstill build.