Commit Graph
2 Commits
Author SHA1 Message Date
fofoj be3613593b test(oauth): update OAuth 401 tests to match new no-write behavior
Two tests in ratelimit_service_401_test.go were encoding the bug behavior
itself:

- OAuth401InvalidatorError asserted updateCredentialsCalls == 1
- OAuth401UsesCredentialsUpdater asserted updateCredentialsCalls == 1
  and lastCredentials["expires_at"] non-empty

Both assertions exercised the exact write-back this PR removes. Update
them to reflect the new contract and guard against regression:

- OAuth401InvalidatorError: assert updateCredentialsCalls == 0
- OAuth401UsesCredentialsUpdater is renamed to
  OAuth401DoesNotOverwriteCredentials with reversed assertions, so it
  now serves as a regression test ensuring the 401 handler never writes
  credentials back from the request-start snapshot.
2026-05-28 20:32:16 +08:00
fofoj 6aec505016 fix(oauth): don't overwrite credentials JSONB in 401 handler
The 401 handler in RateLimitService.HandleUpstreamError set
account.Credentials["expires_at"] = time.Now() and then persisted the
full credentials map via persistAccountCredentials, which routes through
accountRepository.UpdateCredentials -> ent SetCredentials and replaces
the entire JSONB column.

The account passed to the handler is the request-start snapshot taken
by the gateway at SelectAccount time. When another worker has just
rotated refresh_token via oauth_refresh_api.RefreshIfNeeded, the
snapshot still holds the old refresh_token; writing the full snapshot
back rolls refresh_token in the DB back to the stale value.

The next refresh cycle then calls the upstream with the stale token,
receives invalid_grant, and tryRecoverFromRefreshRace re-reads the DB
only to find currentRT == usedRT (because the 401 handler just poisoned
the DB), returns false, and the account is incorrectly disabled.

Drop the credentials write. InvalidateToken + SetTempUnschedulable is
sufficient: the account is held out of scheduling during the cooldown,
and after the cooldown the next request goes through token_provider's
NeedsRefresh check, which routes through the locked, DB-re-reading
RefreshIfNeeded path.

The "force background refresh by setting expires_at = now" semantic is
intentionally dropped. token_refresh_service will naturally pick the
account up when the real expires_at enters the refresh window, and if
the real expires_at has already passed by the time the account becomes
schedulable again, token_provider's NeedsRefresh returns true and
RefreshIfNeeded fires synchronously on the next request.
2026-05-28 20:05:38 +08:00