[cristim]

Main CI went red after #1164 (b6a0649a, run 39496496066). TestValidate_OIDC_FailedRefreshDoesNotCacheKid failed at kidguard_test.go:208 with "token 1: no JWKS fetch although no refresh ever confirmed the kid unknown". 500 local runs pass. The production behaviour is correct; the test assumption is not.

Cause: go-oidc v3.18.0's RemoteKeySet.keysFromRemote (oidc/jwks.go:196-234) signals inflight.done before it takes the lock and sets r.inflight = nil. A Validate that arrives right after the previous one returned can pick up the finished in-flight request and get its result without a new HTTP fetch. "Every token after a failed refresh causes a fetch" is therefore not guaranteed.

Change (test only): for each token, retry Validate until a fetch is observed, with a 2s deadline. Every call must still return ErrUnauthorized. That still proves the invariant: a failed refresh does not put the kid in the negative cache, so a later token can refresh. The final negativeKidCount == 0 check is unchanged. No production code changes.

Evidence:

• Race made deterministic: a scratch go-oidc copy sleeps 100ms after inflight.done. Under it the old assertion fails with the CI message and the new one passes. The whole TestValidate_OIDC_ group passes under the same delay (-count=20), including the two exact-one-fetch siblings.
• Repeat run: go test -race -count=300 -run 'TestValidate_OIDC_' ./internal/server/scheduledauth/ passes.
• Mutation: changing rememberIfConfirmed to cache the kid on any failure makes the new test fail ("token 1: no JWKS fetch within 2s").
• Build and lint: go build ./..., go vet ./... and make lint are clean, and pre-commit passes.

cc @IAmJSD (author of #1164).

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Improved reliability of failed-refresh validation by accommodating reused in-flight key fetches.
  • Added a timeout to ensure a key-set fetch is observed during the test.
  • Continued verifying that each validation attempt returns an unauthorized error.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: d95259f4-e4be-4c00-bb80-a7097d243391

📥 Commits

Reviewing files that changed from the base of the PR and between b6a0649 and 10e65d1.

📒 Files selected for processing: 1

• internal/server/scheduledauth/kidguard_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

The failed-refresh test now retries validation of an unknown-kid token until it observes a JWKS fetch. Each attempt must return ErrUnauthorized, and the test enforces a two-second timeout.

Changes: JWKS refresh test

Layer / File(s)	Summary
Failed-refresh validation internal/server/scheduledauth/kidguard_test.go	The test retries validation until a JWKS fetch occurs. It requires every attempt to return ErrUnauthorized and fails if no fetch occurs within two seconds.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Suggested reviewers: iamjsd

Merge Risk: ⚪ Minimal · up to 10e65

No merge-blocking issue is identified in this test-only change.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the test-only change: handling go-oidc in-flight JWKS reuse in the failed-refresh test.
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

VERDICT: APPROVE 10e65d1

PR #1231, test(scheduledauth): tolerate go-oidc's in-flight JWKS reuse. Diff: internal/server/scheduledauth/kidguard_test.go +13/-5, test-only.

Root cause: confirmed independently.

• CI run 39496496066 fails at kidguard_test.go:208, "token 1: no JWKS fetch although no refresh ever confirmed the kid unknown".
• go-oidc v3.18.0 oidc/jwks.go:206-223: the fetch goroutine calls r.inflight.done(keys, err) and only after that takes r.mu and sets r.inflight = nil. A keysFromRemote call that lands in that window joins the finished inflight and gets its result without an HTTP request.
• The test hits this window every time. Token 0 fails its signature check, which triggers the one allowed rebuild (validator.go:473, with v.now frozen so no second rebuild). The rebuilt verifier's fetch has just signalled done when token 1 arrives.
• Under a 100ms sleep injected after inflight.done (go-oidc copy pulled in with a replace directive): origin/main fails 20/20 with the exact CI message, and the head passes 20/20 for the whole TestValidate_OIDC_ group and 3/3 for the whole package.

Findings

• nit kidguard_test.go:207-218: the loop proves "some fetch happens eventually for each token", not "every call that is not reusing an inflight fetches". A guard that suppressed fetches for only a few calls could pass. With v.now frozen, the only plausible regression is caching on a failed refresh, which never expires, and that is caught. Non-blocking.
• No other test in the package assumes back-to-back fetches. The exact-one-fetch siblings (RepeatedUnknownKidSkipsRefetch :72, ForgedPublishedKidDoesNotBlockLegit :163) see 1 fetch whether or not the inflight is reused: either the rebuild performs the fetch or the guard blocks it. Both pass under the injected delay.
• 2s deadline: the reuse window is the gap between done() and the Lock in a goroutine, which is microseconds without injection. One 5ms sleep clears it. 2s is about 400x margin even under -race on a slow runner. Not flaky.
• No simpler correct fix. Production cannot see go-oidc's inflight state, and a fixed sleep is weaker than retry-until-observed.

Attacked

• M1, rememberIfConfirmed caches every failure: killed 20/20 ("token 1: no JWKS fetch within 2s").
• M2, freshness check snap.seq <= arrival dropped: killed 20/20.
• M3, jwksObserver records an empty snapshot on a non-200 response (poisons on a failed refresh): killed 20/20.
• M4, TTL never expires: not testable here because the test freezes v.now. Covered by NegativeKidExpiresAfterTTL.
• go test -race -count=200 -run 'TestValidate_OIDC_' at the head:
  • With the default 10m timeout, the run reached the 10m go test timeout (log had 0 FAIL lines, killed while in AudienceListClaim). This is runtime, not a failure in this PR.
  • Rerun with -timeout 0: ok in 679s, 0 FAIL, 0 DATA RACE.
• FailedRefresh alone, -race -count=200: ok (48s).
• go vet -tags integration on the package: clean.
• PR CI at the head: all checks pass except Integration Tests, still pending.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 10e65d1

The go-oidc claim holds for the vendored version, and I reproduced the base flake. The head test did not flake across about 13,500 runs. Every mutation that caches the kid or skips the fetch fails the new test every time.

Findings (none blocking):

• [non-blocking] internal/server/scheduledauth/kidguard_test.go:208-217 - The retry loop accepts a fetch any time within 2 s of wall clock.
  • A regression that cached the kid on failure with a sub-2 s expiry, timed by wall clock rather than v.now, would slip through.
  • No current code path can do this, because the TTL is 10 s (minRebuildInterval) and it reads the frozen v.now.
• [non-blocking] Once a mutation caches the kid, the 2 s deadline at :214 fires first, so those cases are caught at :214 rather than by the final negativeKidCount check at :219. That check still guards the case where a fetch happens and the kid is cached anyway.

Attacked:

• go-oidc (go.mod pins github.com/coreos/go-oidc/v3 v3.18.0), oidc/jwks.go in the module cache:
  • :210 calls r.inflight.done(keys, err) before :214 takes r.mu.Lock() and :222 sets r.inflight = nil.
  • A keysFromRemote call (:198-225) that gets the lock in that gap sees a finished, non-nil inflight and returns its cached error with no HTTP request (:231-232).
  • verify() calls keysFromRemote on every kid miss (:173).
  • The validator's rotation rebuild (validator.go:471-484) does not hide this: with the frozen clock, the rebuild budget stays used after token 0 (:473).
• Mutations of kidguard.go on a scratch copy, each restored and confirmed identical with cmp:
  • m1, snap.seq <= arrival dropped in rememberIfConfirmed (:220), so the kid is cached from the stale snapshot: 200/200 FAIL at :214.
  • m2, any inner failure caches the kid (negative.add, :204): 200/200 FAIL.
  • m3, the fetch never happens (errKidRecentlyAbsent is returned without calling go-oidc): 200/200 FAIL on token 0. Nothing is cached, so the old final assertion alone would have passed.
• Flake counts for the single test, base vs head:
  • -count=500: 0 vs 0 failures. The window is narrow on a quiet machine.
  • 8 concurrent binaries x 500 runs at -cpu=1,2,4 (4,000 runs per setting): base failed once at cpu=4 with "token 2: no JWKS fetch although no refresh ever confirmed the kid unknown", which is the CI failure. Head had 0 failures at every setting.
  • -race -count=200, and 4 concurrent x 200 under -race: 0 failures on both.
• Swallowed errors: every Validate in the loop must return ErrUnauthorized (:210-212). A missing fetch fails at the deadline (m3), and the loop is bounded (2 s at 5 ms sleeps).
• Scope: test-only. No production code or exported symbols change.

Verified locally, on a git archive export with no cloud credentials:

• gofmt -l and go vet on ./internal/server/scheduledauth/: clean.
• go test -count=1 ./internal/server/scheduledauth/, under the heavy lock: ok.
• Fail-before: the old assertion is the flaky one, so the evidence is the flake comparison above (base 1 failure, head 0, in matching runs).

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Local verification at 10e65d17a660a992beba8ec3d233d3d3206c7a6d merged onto current main (b6a0649a; merge-tree 9b47b252; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.