Guard rotating refresh-token writes with compare-and-swap

[reyortiz3]

Summary

• Multiple replicas of an application embedding this auth server, sharing the same Redis-backed UpstreamTokenStorage, can redeem and persist the same single-use, rotating upstream refresh token concurrently (some OAuth providers issue short-lived access tokens together with single-use refresh tokens that rotate on every redemption). The existing singleflight dedup in upstreamTokenRefresher only coordinates refreshes within one process, and StoreUpstreamTokens is an unconditional overwrite — so a losing replica's write can silently clobber a winning replica's already-rotated refresh token, permanently breaking the refresh chain and forcing the user back through re-authentication.
• Adds UpstreamTokenStorage.CompareAndSwapUpstreamTokens(ctx, sessionID, providerName, expectedRefreshToken, tokens), which only writes if the refresh token currently stored still equals expectedRefreshToken, returning the new ErrConcurrentRefresh sentinel otherwise. Implemented for both backends: a new CAS Lua script (casUpstreamTokensScript) for RedisStorage, and a mutex-guarded compare-then-write for MemoryStorage.
• upstreamTokenRefresher.refreshAndStore now writes through the CAS method (expected value = the refresh token it just redeemed with the provider) instead of the old unconditional StoreUpstreamTokens. On ErrConcurrentRefresh it re-reads the row: if another replica's write left it unexpired, that replica's tokens are returned to the caller; otherwise it surfaces a clear error rather than retrying an already-dead redemption. The re-read also distinguishes a genuine lost race (unexpired row) from a row that's simply gone (ErrNotFound — deleted by logout or evicted by TTL), logging the latter at Warn rather than Error so it isn't mistaken for a race in production logs.
• singleflight remains a same-process optimization (avoids redundant upstream calls / Redis round-trips for concurrent requests inside one process). The CAS write is what makes the stored row deterministic across processes — it is not a guarantee that concurrent redemption is safe at the upstream provider: both replicas still call the provider before either write lands, so a provider enforcing strict single-use rotation can still see two redemptions of the same refresh token regardless of which write wins here. CAS is fully sufficient only where the provider tolerates a grace/leeway window in which more than one redeemed child stays valid; closing the gap for a stricter provider needs a lock around the whole read-redeem-write sequence, which is a real follow-up, not implemented here (see below).

Type of change

• Bug fix

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

Added:

• TestMemoryStorage_CompareAndSwapUpstreamTokens / TestRedisStorage_CompareAndSwapUpstreamTokens — matching/stale/absent-row CAS cases, a security-positive case (CAS refuses to resurrect a row deleted between read and write, unlike StoreUpstreamTokens), plus a 10-goroutine concurrent-write race against a real (miniredis-backed) Lua script proving exactly one write wins and losers never clobber it.
• TestUpstreamTokenRefresher_ConcurrentRefreshConflict — the refresher's conflict-resolution path: returns the winning replica's tokens when the re-read is unexpired, and a wrapped ErrConcurrentRefresh error when no usable winner exists (both the row-absent and still-expired re-read shapes are covered).
• TestUpstreamTokenRefresher_ConcurrentRefreshConflict_LogLevel — proves the lost-race vs. row-absent distinction actually logs at different levels.

Updated all existing refresher_test.go mock expectations from StoreUpstreamTokens to CompareAndSwapUpstreamTokens. Mocks regenerated via task gen.

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Does this introduce a user-facing change?

No — this is an internal storage-layer/refresher change. It hardens an existing reliability gap; no public API changes beyond the additive UpstreamTokenStorage interface method.

Special notes for reviewers

• UpstreamTokenStorage.CompareAndSwapUpstreamTokens is an additive interface method — any external implementation of this interface will need the new method to satisfy the interface (only RedisStorage and MemoryStorage implement it in this repo).
• No distributed lock (e.g. SET NX mutex) was introduced. CAS makes the stored row deterministic and prevents a stale write from clobbering a newer one, but for a provider that enforces strict single-use rotation with no grace window, a lock around the whole read-redeem-write sequence is a correctness requirement, not just an optimization — CAS alone doesn't stop both replicas from calling the provider before either writes. This matters concretely for Read.ai (the motivating provider): its access tokens are 10 minutes, so this race gets exercised often, and its refresh-token grace period is still an open item on its public roadmap (not documented as guaranteed today). Treating the follow-up lock as a real correctness item, not a nice-to-have.
• Not included in this PR: a hermetic OAuth test double modeling single-use rotating refresh tokens with a grace period, and a two-replica staging/e2e test against a real provider — left as potential follow-up work.

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 97.59036% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.81%. Comparing base (1f75a71) to head (1f807c0).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/authserver/storage/redis.go	92.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6548      +/-   ##
==========================================
+ Coverage   78.70%   78.81%   +0.10%     
==========================================
  Files         777      778       +1     
  Lines       76816    77554     +738     
==========================================
+ Hits        60459    61121     +662     
- Misses      16352    16428      +76     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tgrunnagle]

Multi-Agent Consensus Review

Agents consulted: concurrency-correctness, security, go-style-interface-design, test-coverage, general-code-quality

Consensus Summary

#	Finding	Consensus	Severity	Action
1	Redis CAS script duplicates the store script's write/index body, sync-guarded only by a comment	10/10	MEDIUM	Fix
2	resolveConcurrentRefreshConflict discards the re-read error on the unrecoverable path	9/10	MEDIUM	Fix
3	Stale comment references StoreUpstreamTokens on a path that now writes through CAS	9/10	MEDIUM	Fix
4	New concurrent CAS test blocks on wg.Wait() with no timeout guard	9/10	MEDIUM	Fix
5	Architecture doc contradicts the new cross-process CAS guarantee	8/10	MEDIUM	Fix
6	No test for expectedRefreshToken="" against an already-populated row	8/10	MEDIUM	Fix
7	Untested "re-read succeeds but still expired" conflict-resolution branch	8/10	MEDIUM	Fix
8	Redis CAS script's session-index/TTL maintenance path has no dedicated test	8/10	MEDIUM	Fix

Overall

This closes a real correctness gap: multiple replicas sharing Redis-backed UpstreamTokenStorage could clobber each other's rotated single-use refresh tokens, since StoreUpstreamTokens was an unconditional overwrite. CompareAndSwapUpstreamTokens is the right fix — a CAS primitive rather than a heavier distributed lock — and it's implemented atomically in both backends, with the Redis path's atomicity proven by a genuine 10-goroutine race test against real miniredis rather than just a code read-through.

The findings here are all non-blocking. The most durable risk is the Redis CAS Lua script duplicating the entire write/index body of the pre-existing store script, guarded only by a "keep these in sync" comment — a future fix to one script's TTL/index logic that isn't mirrored to the other would silently reintroduce a bug this codebase already went to some effort to fix once. The conflict-resolution fallback path (resolveConcurrentRefreshConflict) also discards its re-read error, which will make an already-rare production race harder to diagnose, and its "still expired" branch (as opposed to "row not found") is untested. A few other test-coverage gaps round out the list: the CAS boundary case of an empty expected value against a populated row, and the new Redis script's index/TTL maintenance path for the CAS variant specifically.

None of this blocks merge — it's hardening on top of an already-sound fix.

Documentation

docs/arch/11-auth-server-storage.md's "Refresh Coordination Scope" section (lines 205-208) states: "This is deliberately narrower than #4122: it does not provide distributed coordination across replicas, row-addressed mutation, compare-and-swap, or any other cross-process consistency guarantee." This PR adds exactly that guarantee and the doc should be updated to reflect it — the refresher.go package doc's "two layers" framing (in-process singleflight vs. cross-process CAS) is a good model to adapt from.

---

Generated with Claude Code

[reyortiz3]

Addressed all 8 findings from the multi-agent consensus review:

1. Redis CAS script duplication — unified storeUpstreamTokensScript and casUpstreamTokensScript's write/index body into one shared upstreamRowWriteAndIndexScriptBody Go constant, concatenated into both scripts. Eliminates the duplication rather than just guarding it with a comment.
2. Discarded re-read error — resolveConcurrentRefreshConflict now includes err in both the log call and the returned (wrapped) error.
3. Stale StoreUpstreamTokens comment — reworded to reference the write call generically.
4. Concurrent CAS test missing timeout guard — wrapped in the same timeout-guarded done-channel pattern as TestRedisStorage_ConsumeAssertionJWT_Concurrent in this same file.
5. Architecture doc contradicted the new guarantee — updated docs/arch/11-auth-server-storage.md's "Refresh Coordination Scope" section to describe the CAS layer alongside the existing in-process singleflight description.
6. Missing empty-expected-value-against-populated-row test — added to both memory_test.go and redis_test.go.
7. Untested "still expired on re-read" branch — added a third subtest distinguishing it from the "not found" case.
8. Untested Redis CAS index/TTL path — added session-index-set membership/TTL and user-reverse-index assertions to the successful-write CAS subtest.

All existing and new tests pass (task test), lint is clean (task lint-fix).

[amirejaz]

Read through this focusing on the concurrency contract and the OAuth side. The CAS mechanism itself looks well built, the shared Lua body from the last round is the right call, and pkg/authserver/... plus pkg/auth/upstreamtoken/... pass with -race on d88d643. Storage writes are now deterministic and a stale write can no longer clobber a newer one, which is a real fix.

My one substantive concern is the claim about what CAS guarantees, left inline on the arch doc: it orders storage writes, not redemptions at the IdP, and the second redemption still happens. That doesn't block the mechanism, but I'd like the wording in the docs and the PR description adjusted so the remaining gap is recorded rather than described as closed. The other three are smaller: an error sentinel that conflates "changed" with "gone", one inaccurate comment on a new test, and a timeout that doubles the worst-case wait.

[reyortiz3]

Addressed the second round of review feedback (4 findings from @amirejaz):

1. Overclaimed "safe across processes" — softened throughout (this doc, refresher.go's type-level and inline comments, the CompareAndSwapUpstreamTokens/ErrConcurrentRefresh interface docs). CAS makes the stored row deterministic; it doesn't by itself make concurrent redemption safe against a strict single-use-rotation provider, since both replicas still call the provider before either write lands. Closing that gap needs a lock around the whole read-redeem-write sequence — noted as a real follow-up, not claimed as done.
2. ErrConcurrentRefresh conflates "raced" vs "row genuinely gone" — resolveConcurrentRefreshConflict now logs at Warn (not Error) when the re-read comes back ErrNotFound, since that's expected on logout/TTL eviction, not a lost race. Added a log-level test. Also added the security-positive test: CAS refuses to resurrect a row deleted between read and write (both backends), which the unconditional StoreUpstreamTokens would not have prevented.
3. Imprecise "only matches a genuinely absent row" comment — corrected; "" also matches a present row with its own empty RefreshToken or a nil-tokens row, and the interface doc now spells out all three cases.
4. Doubled worst-case timeout — added a dedicated upstreamConflictReadTimeout (5s) instead of reusing the outer refreshTimeout (30s) for the post-conflict re-read.

All tests pass (task test), lint clean (task lint-fix).

[amirejaz]

Approving.

The concurrency contract holds up. CAS makes the stored row deterministic, a losing write can no longer clobber a rotated token, and the Redis comparison and write are genuinely one atomic operation now that both scripts share upstreamRowWriteAndIndexScriptBody. I re-ran pkg/authserver/... and pkg/auth/upstreamtoken/... with -race on 3f9ab97 and they're green.

Both things I raised last round are handled well. The ErrNotFound branch on the re-read is a better answer than the second sentinel I suggested: the double-%w means a caller can still tell the two situations apart (I checked - errors.Is reports both ErrConcurrentRefresh and ErrNotFound), so keeping one sentinel costs nothing. Adding the no-resurrection test in both backends was the right call too; that behavior was worth pinning before someone "fixes" it back.

Two doc-only leftovers, neither blocking.

The PR description still says a lock "would only save a redundant upstream redemption on the losing replica, which is an optimization, not a correctness requirement". Every code and doc site now says the opposite, so it's worth bringing the description in line before merge, since that's what stays visible in the merge commit later.

That's a bit sharper than it looks, given the arch doc now cites Read.ai as the provider where CAS is sufficient. Their access tokens are 10 minutes, so refreshes are frequent and this race gets exercised often, and their grace window isn't actually published - "refresh token grace period" is still an open item on their public roadmap. Worth linking the source in the doc, and I'd treat the follow-up lock as more than a nice-to-have.

The other leftover is inline on redis.go.

[reyortiz3]

Fixed both doc-only leftovers:

1. Reworded the remaining "safe for redeeming" phrasing in the casUpstreamTokensScript comment (redis.go) to match the softened language used everywhere else.
2. Updated the PR description: the "optimization, not a correctness requirement" line now says the opposite (matching the code/docs), and added the Read.ai-specific context you raised — its 10-minute access tokens mean this race is exercised often, and its refresh-token grace period is still an open roadmap item rather than a documented guarantee. Treating the follow-up lock as a real correctness item now, not a nice-to-have.