Skip to content

fix(router): abort router-transform on proactive invalid_grant instead of retrying (INT-6779) - #7175

Open
shekhar-rudder wants to merge 7 commits into
masterfrom
feature/int-6779-circuit-breaker-triggered-by-invalid-refresh-tokens
Open

fix(router): abort router-transform on proactive invalid_grant instead of retrying (INT-6779)#7175
shekhar-rudder wants to merge 7 commits into
masterfrom
feature/int-6779-circuit-breaker-triggered-by-invalid-refresh-tokens

Conversation

@shekhar-rudder

@shekhar-rudder shekhar-rudder commented Jul 13, 2026

Copy link
Copy Markdown
Member

Description

Related to INT-6779.

When an OAuth destination's refresh token is revoked/expired, rudder-auth returns ref_token_invalid_grant. During router transformation, a proactive FetchToken that fails this way was returned by the OAuth transport's preRoundTrip as a raw-text HTTP 400 body. The router-transform response handler collapses any non-200 (non-404) response to a hardcoded 500 (retryable), so the real 400 was discarded — the job retried for the full retry window (~3h) and was ultimately drained at 410 instead of aborting.

This PR makes ref_token_invalid_grant abort at 400 in the router-transform flow.

  • services/oauth/v2/http/transport.gopreRoundTrip now reports a failed FetchToken through the TransportResponse/InterceptorResponse envelope, carrying the real status code and a clean message. OriginalResponse keeps the raw error text, so every caller that falls back to it is unaffected.
  • services/oauth/v2/types.goOAuthInterceptorResponse gains an ErrorType field, so callers act on the specific failure instead of inferring terminality from the status code.
  • router/transformer/transformer.go — the non-200 else-branch aborts with a 400 only when the interceptor reports ref_token_invalid_grant. The error type alone decides; the status the interceptor attached is not consulted, since a revoked refresh token is terminal however it arrives. Every other interceptor outcome keeps the existing retryable path and its original message.

Why there is no explicit invalid_grant check

An earlier revision gated the envelope on errors.Is(err, common.ErrInvalidGrant). That check was redundant and has been removed: FetchToken can only ever return 400 or 500, and 400 is produced exclusively by ref_token_invalid_grantgetRefreshTokenFromResponse initialises statusCode to 500 and only the common.RefTokenInvalidGrant case sets 400; every other failure path is NewStatusCodeError(http.StatusInternalServerError, ...). The status code already carries the signal the check was re-deriving.

What actually fixes the bug is the response body, not the gate: router transformation ignores the raw HTTP status, so the real status has to travel inside the interceptor envelope for the handler to see it.

An earlier revision of this PR gated the abort on the interceptor status being < 500. That was broader than intended: postRoundTrip also reports AUTH_STATUS_INACTIVE as a terminal 400, so a non-200 transform response carrying that category would have aborted where it previously retried. The gate is now keyed on ErrorType == common.RefTokenInvalidGrant alone.

Keying on the type rather than the status also closes a reachable gap: getRefreshTokenFromResponse has an early branch on a top-level errorType key that hardcodes a 500, so a control plane reporting ref_token_invalid_grant that way produced a retryable 500 and burned the full retry window. It now aborts like any other invalid_grant.

No change to the delivery/proxy flow — its reactive RefreshToken400 abort already worked correctly.

Scope note

An earlier revision of this PR also excluded invalid_grant from the per-account OAuth circuit breaker. That has been reverted: we're keeping invalid_grant counting toward the breaker (it preserves control-plane protection). The circuit-breaker / alerting side of INT-6779 — detecting a control-plane bug that erroneously returns invalid_grant — will be handled separately via a multi-account breaker-trip alert (fire when the breaker trips across many distinct accounts, which distinguishes a CP-wide problem from isolated token revocations), tracked as a follow-up.

Behavior change

For a revoked-token OAuth destination (e.g. LINKEDIN_ADS): jobs abort immediately with 400 and a "please reauthorize" message — no 3-hour retry storm, no 410 drain. Transient/other failures (empty secret, network, non-invalid_grant errors → 500) still retry, unchanged.

Testing

TestRouterTransformationWithOAuthV2:

  • proactive invalid_grant → jobs abort at 400 with the clean message;
  • non-invalid_grant proactive failure (empty secret) stays 500 (retryable);
  • existing reactive-refresh cases unchanged.

TestRouterTransformationWithOAuthV2 (new regression case):

  • a non-200 transform response carrying AUTH_STATUS_INACTIVE (a terminal 400 that is not invalid_grant) stays 500 and is retried. This case fails under the old < 500 gate;
  • invalid_grant reported via the top-level errorType key (status 500) still aborts at 400. This case fails under the earlier status-honouring gate.

services/oauth/v2/http (new, table-driven at the transport level):

  • terminal: invalid_grant400, tagged ErrorType: ref_token_invalid_grant, clean message in InterceptorResponse.Response, raw text preserved in OriginalResponse;
  • retryable: INVALID_REFRESH_RESPONSE500, tagged with its own error type;
  • retryable: a failure with no error type → 500, empty ErrorType, OriginalResponse unchanged from the pre-envelope raw body.

Also verified green (these consume the same transport and see the changed body): regulation-worker/internal/delete/api, yandexmetrica, salesforce-bulk-upload, services/oauth/v2.

Known review findings

Open, not addressed here — flagging for reviewer input:

  • Breaker interaction. Since invalid_grant still counts toward the per-account error breaker, once it trips (5 consecutive failures) withErrBreaker returns the cached lastError for 5 minutes without calling the control plane (services/oauth/v2/oauth_breaker.go:191). That stale error still carries the terminal 400, so for that window every job on the account aborts on the strength of one old CP response. Given INT-6779's premise is that the CP can erroneously return invalid_grant, it may be worth putting the abort behind a config var so it can be disabled without a rollback.

🤖 Generated with Claude Code

shekhar-rudder and others added 3 commits July 13, 2026 13:49
invalid_grant is a permanent user error (revoked token), not a
control-plane health signal, so it must not trip the per-account OAuth
error breaker or fire the false 'Control plane issue' alert (INT-6779).
… of retrying

A proactive FetchToken that fails with invalid_grant returned a raw 400
body that the router-transform non-200 handler collapsed to a retryable
500, causing ~3h of retries and a 410 drain. preRoundTrip now wraps
invalid_grant in the interceptor envelope and the transform handler
honors that terminal status, so the job aborts at 400 (INT-6779).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@shekhar-rudder shekhar-rudder changed the title fix(oauth): don't retry/trip breaker on ref_token_invalid_grant (INT-6779) fix(oauth): abort router-transform on proactive invalid_grant instead of retrying (INT-6779) Jul 13, 2026
@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.30%. Comparing base (581410c) to head (51ce926).
⚠️ Report is 35 commits behind head on master.

Files with missing lines Patch % Lines
services/oauth/v2/http/transport.go 76.47% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7175      +/-   ##
==========================================
+ Coverage   80.01%   80.30%   +0.28%     
==========================================
  Files         595      600       +5     
  Lines       66133    67034     +901     
==========================================
+ Hits        52915    53829     +914     
+ Misses      10071    10044      -27     
- Partials     3147     3161      +14     

☔ 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.

@shekhar-rudder shekhar-rudder changed the title fix(oauth): abort router-transform on proactive invalid_grant instead of retrying (INT-6779) fix(router): abort router-transform on proactive invalid_grant instead of retrying (INT-6779) Jul 13, 2026
shekhar-rudder and others added 4 commits August 4, 2026 12:52
The `errors.Is(scErr, common.ErrInvalidGrant)` check was not load-bearing:
FetchToken can only ever return 400 or 500, and 400 is produced exclusively
by ref_token_invalid_grant (getRefreshTokenFromResponse starts at 500 and
only the RefTokenInvalidGrant case sets 400). The status code already
carried the signal the check was re-deriving.

What actually makes the job abort is the response *body*: router
transformation ignores the raw HTTP status and collapses every non-200 to a
retryable 500, so the real status has to travel in the interceptor envelope.

Always wrap the FetchToken failure in the envelope and populate
OriginalResponse with the raw error text, so the callers that fall back to
it (transformer proxy, regulation worker, yandexmetrica, salesforce bulk
upload) keep the readable message they had before the envelope existed,
instead of surfacing the JSON envelope itself as the error.

Adds transport-level coverage for both the terminal (400) and retryable
(500) proactive fetch failures, and corrects the transformer comment: the
`< 500` gate also admits the authStatus-inactive 400, which is likewise
terminal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reverts the comment-only rewrite from the previous commit. No logic change
in either direction; the `< 500` gate, the transport envelope and the new
transport-level tests are untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The router transform handler gated the abort on the interceptor status
being < 500, which is broader than intended: postRoundTrip also reports
AUTH_STATUS_INACTIVE as a terminal 400, so a non-200 transform response
carrying that category aborted where it previously retried.

Carry the specific failure in the envelope instead. OAuthInterceptorResponse
gains an ErrorType field, set from the underlying TypeMessageError, and the
handler aborts only when it is ref_token_invalid_grant. Every other
interceptor outcome - terminal-looking or not - keeps the retryable path and
its original message.

Adds a router-level regression test (a non-200 transform response carrying
AUTH_STATUS_INACTIVE must stay 500), which fails under the old < 500 gate,
and extends the transport table with a non-invalid_grant error type.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… status

The gate also required a non-zero interceptor status and reused it as the job
status. That was unreachable defence against a lenient unmarshal, and it made
the abort depend on a status the error type already implies.

invalid_grant means the refresh token has been revoked, which is terminal
however it arrives, so the type alone now decides and the job is aborted with
a 400. This also fixes a reachable gap: getRefreshTokenFromResponse reports
the same error type with a 500 when the control plane returns it via the
top-level errorType key, which previously retried for the full window.

Adds a router-level test for that path; it fails under the previous gate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

2 participants