auth: classify service-token failures structurally, not by string

Follow-up to 37bab32. That commit stopped deleting OAuth sessions on transient
errors, which fixed spurious sign-outs but overshot on one path: a genuinely dead
session stopped being evicted at all, turning a forced re-login into a permanent
failure loop.

GetOrFetchServiceToken flattened every non-200 from getServiceAuth into
fmt.Errorf("service auth failed with status %d: %s"). IsSessionInvalidError then
had nothing structured to inspect, and its string fallback could not help: it
looks for the OAuth 2.0 code invalid_token, while atproto emits the XRPC name
InvalidToken. The difference is the underscore, not the case, so lowercasing
never bridged it. A revoked session came back 401 InvalidToken and was classified
transient, so /auth/token returned 503 forever and the user was never prompted to
re-authenticate.

The non-200 branch now wraps an *atclient.APIError carrying the status and the
parsed atproto error name, which is what the existing structured checks in
IsSessionInvalidError already know how to read. Transient shapes stay transient:
atprotoErrorName returns "" for a non-JSON body, so 500s with HTML, 502s, and
429s do not evict.

ExpiredToken is deliberately not treated as a dead session. It means "refresh
me", and deleting on it would sign the user out of every UI session over an
ordinary access-token expiry a refresh would have fixed. isAuthError omits it for
the same reason; the two classifiers have to agree about the same condition.

The comment on the string fallback claimed it was a looser spelling of the
structured check. It is not — it handles a different error family. indigo's
RefreshTokens returns OAuth token-endpoint failures as a bare fmt.Errorf carrying
the auth server's snake_case code verbatim ("token refresh failed (HTTP 400):
invalid_grant"), never a typed error, so a string match is the only thing that
can classify a refresh failure, which is the invalid_grant replay case 37bab32
exists to detect. Both comments now say which family they cover.

Two hardening items on the same theme:

use_dpop_nonce no longer counts as an auth error in the appview's isOAuthError.
It is a routine handshake step indigo retries with the server-supplied nonce, and
treating it as fatal signed users out over ordinary nonce rotation. It can still
escape when a server sends that error with no DPoP-Nonce header, leaving indigo
nothing to retry with; a stuck session there is preferable to signing everyone
out in the common case, and the comment says so rather than claiming it cannot
happen.

Detached session deletes are bounded by SessionDeleteTimeout. They run on
context.WithoutCancel so a canceled request cannot leave the cleanup half-done,
which also stripped the only deadline they had — a wedged database write blocked
the goroutine with no way to shed it. Matches the bound already on the detached
persist callback. The unparseable-token-endpoint warning is now deduped per
endpoint rather than once per process, since that path fails open by returning
the client unwrapped, silently reinstating the refresh burn.

The refreshDetachTimeout comment now notes the cap is per-POST: the DPoP-nonce
retry means one refresh can issue two, holding the per-DID lock for up to twice
the stated value.

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

pkg/appview/handlers/oauth_errors.go

@@ -41,11 +41,23 @@ func isOAuthError(err error) bool {
 	}
 
 	// Fallback: check for known auth-specific error strings that won't
-	// appear in digests or URIs
+	// appear in digests or URIs.
+	//
+	// Deliberately absent: use_dpop_nonce. It means the DPoP nonce was stale,
+	// which indigo normally handles by retrying with the server-supplied nonce —
+	// a routine handshake step, not a dead session. Treating it as an auth error
+	// signed users out over ordinary nonce rotation, the same
+	// transient-misclassified-as-fatal bug this file's detached-delete logic
+	// exists to avoid.
+	//
+	// It can still escape in one case: a server that returns error=use_dpop_nonce
+	// with no DPoP-Nonce header leaves indigo nothing to retry with, and the
+	// reason reaches us verbatim. We accept a stuck session there rather than
+	// signing every user out over the common case; a server doing that is
+	// broken, and the resulting failures are visible in the logs.
 	errStr := strings.ToLower(err.Error())
 	return strings.Contains(errStr, "invalid_token") ||
 		strings.Contains(errStr, "invalid_grant") ||
-		strings.Contains(errStr, "use_dpop_nonce") ||
 		strings.Contains(errStr, "authentication failed") ||
 		strings.Contains(errStr, "token expired")
 }
@@ -65,7 +77,9 @@ func handleOAuthError(ctx context.Context, refresher *oauth.Refresher, did strin
 	// Invalidate all UI sessions for this DID. Detached context: once we
 	// decide to delete, the cleanup must finish even if the inbound request
 	// is canceled mid-way.
-	if delErr := refresher.DeleteSession(context.WithoutCancel(ctx), did); delErr != nil {
+	delCtx, cancelDel := context.WithTimeout(context.WithoutCancel(ctx), oauth.SessionDeleteTimeout)
+	defer cancelDel()
+	if delErr := refresher.DeleteSession(delCtx, did); delErr != nil {
 		slog.Warn("Failed to delete OAuth session after error",
 			"component", "handlers",
 			"did", did,

pkg/auth/oauth/client.go

@@ -21,6 +21,13 @@ import (
 	"github.com/bluesky-social/indigo/xrpc"
 )
 
+// SessionDeleteTimeout bounds a detached session delete. These deletes run on
+// context.WithoutCancel so a canceled inbound request cannot leave the cleanup
+// half-done — but that also strips the only deadline they had, so without a
+// replacement a wedged database write blocks the calling goroutine forever with
+// no way to shed it. Matches the bound on the detached persist callback.
+const SessionDeleteTimeout = 10 * time.Second
+
 // permissionSetExpansions maps lexicon IDs to their expanded scope format.
 // These must match the collections defined in lexicons/io/atcr/authFullApp.json
 // Collections are sorted alphabetically for consistent comparison with PDS-expanded scopes.
@@ -320,7 +327,9 @@ func (r *Refresher) DoWithSession(ctx context.Context, did string, fn func(sessi
 		// context: once we decide to delete, the cleanup must finish even if
 		// the inbound request is canceled mid-way.
 		mutex.Unlock()
-		_ = r.DeleteSession(context.WithoutCancel(ctx), did)
+		delCtx, cancelDel := context.WithTimeout(context.WithoutCancel(ctx), SessionDeleteTimeout)
+		_ = r.DeleteSession(delCtx, did)
+		cancelDel()
 		mutex.Lock() // Re-acquire for the deferred unlock
 	}
 
@@ -357,15 +366,32 @@ func IsSessionInvalidError(err error) bool {
 		if apiErr.StatusCode == 401 {
 			return true
 		}
+		// atproto XRPC error names, which are camel-case. The string match
+		// below handles a *different* family (OAuth 2.0 token-endpoint codes,
+		// snake_case) and cannot catch these — the mismatch is the underscore,
+		// not the case, so lowercasing "InvalidToken" still never reaches
+		// "invalid_token". Every XRPC name meaning "this session is dead" has
+		// to be listed right here.
+		// Deliberately absent: ExpiredToken. It means "refresh me", not "this
+		// session is revoked" — deleting on it signs the user out of every UI
+		// session over an ordinary access-token expiry that a refresh would
+		// have fixed. A genuinely dead session still gets caught by the 401
+		// status check above. isAuthError (below) omits it for the same reason;
+		// the two classifiers must agree about the same condition.
 		switch apiErr.Name {
 		case "InvalidToken", "InvalidGrant", "InsufficientScope":
 			return true
 		}
 	}
 
-	// The token-refresh failure from indigo arrives as a plain wrapped error
-	// ("auth server request failed (HTTP 400): invalid_grant"), not an
-	// APIError, so a string fallback is required. These substrings are
+	// OAuth 2.0 token-endpoint failures, which are a separate error family from
+	// the XRPC names above rather than a looser spelling of them. indigo's
+	// RefreshTokens returns them as a bare fmt.Errorf carrying the auth
+	// server's error code verbatim ("token refresh failed (HTTP 400):
+	// invalid_grant") — never an APIError, never a wrapped typed error — so
+	// matching the string is the only way to classify them. RFC 6749 defines
+	// these codes as snake_case, hence the different spellings; ToLower is only
+	// belt-and-braces for a server that deviates. The substrings are
 	// auth-specific and won't appear in digests or URIs.
 	errStr := strings.ToLower(err.Error())
 	return strings.Contains(errStr, "invalid_grant") ||

pkg/auth/oauth/client_test.go

@@ -380,6 +380,18 @@ func TestIsSessionInvalidError(t *testing.T) {
 		{"api error InvalidGrant", &atclient.APIError{StatusCode: 400, Name: "InvalidGrant"}, true},
 		{"api error InvalidToken", &atclient.APIError{StatusCode: 400, Name: "InvalidToken"}, true},
 		{"api error 500", &atclient.APIError{StatusCode: 500, Name: "InternalServerError"}, false},
+		// ExpiredToken means "refresh me", not "revoked". Treating it as a dead
+		// session signs the user out of every UI session over an ordinary
+		// access-token expiry that a refresh would have fixed.
+		{"api error ExpiredToken is refreshable, not dead", &atclient.APIError{StatusCode: 400, Name: "ExpiredToken"}, false},
+		// Transient upstream failures must never evict: these are the shapes the
+		// service-token path now wraps as APIErrors.
+		{"api error 502", &atclient.APIError{StatusCode: 502, Name: ""}, false},
+		{"api error 429", &atclient.APIError{StatusCode: 429, Name: ""}, false},
+		{"api error 500 html body", &atclient.APIError{StatusCode: 500, Name: "", Message: "<html>bad gateway</html>"}, false},
+		// A revoked session reported as 401 with an atproto name — the case the
+		// service-token path was previously flattening into an unmatchable string.
+		{"api error 401 InvalidToken", &atclient.APIError{StatusCode: 401, Name: "InvalidToken"}, true},
 		// The refresh-replay failure arrives as a plain wrapped string from indigo.
 		{"plain invalid_grant string", errors.New("failed to refresh OAuth tokens: token refresh failed (HTTP 400): invalid_grant"), true},
 		{"plain invalid_token string", errors.New("auth server request failed (HTTP 401): invalid_token"), true},

pkg/auth/oauth/transport.go

@@ -13,9 +13,20 @@ import (
 // refreshDetachTimeout bounds a detached token-refresh POST. Refreshes are
 // normally sub-second; this only exists so a hung auth server can't pin the
 // per-DID session lock forever.
+//
+// Note this is per-POST, not per-refresh: indigo's postToAuthServer retries once
+// on a use_dpop_nonce challenge, so a single refresh can issue two detached
+// POSTs and hold the per-DID lock for up to 2x this value. Keep that product
+// comfortably under the callers' own deadlines (Docker gives up on /auth/token
+// well before it).
 const refreshDetachTimeout = 30 * time.Second
 
-var badTokenEndpointLogOnce sync.Once
+// badTokenEndpointOnce dedupes the "couldn't parse the token endpoint" warning
+// per endpoint value rather than once per process. That path fails open — it
+// returns the client unwrapped, silently reinstating the refresh-burn bug this
+// file exists to fix — so it must stay visible for every distinct auth server
+// it affects, not just the first one after boot.
+var badTokenEndpointOnce sync.Map
 
 // refreshDetachTransport detaches the request context for OAuth token-refresh
 // POSTs. A refresh is non-idempotent: the auth server rotates the refresh
@@ -74,12 +85,12 @@ func (c *cancelOnClose) Close() error {
 func newRefreshDetachClient(inner *http.Client, tokenEndpoint string) *http.Client {
 	endpoint, err := url.Parse(tokenEndpoint)
 	if err != nil || endpoint.Host == "" {
-		badTokenEndpointLogOnce.Do(func() {
+		if _, seen := badTokenEndpointOnce.LoadOrStore(tokenEndpoint, struct{}{}); !seen {
 			slog.Warn("Not detaching token-refresh context: unparseable auth server token endpoint",
 				"component", "oauth/refresher",
 				"tokenEndpoint", tokenEndpoint,
 				"error", err)
-		})
+		}
 		return inner
 	}
 

pkg/auth/servicetoken.go

@@ -17,6 +17,20 @@ import (
 	indigo_oauth "github.com/bluesky-social/indigo/atproto/auth/oauth"
 )
 
+// atprotoErrorName extracts the atproto error name from an XRPC error body
+// ({"error":"ExpiredToken","message":"..."}). Returns "" when the body is not
+// JSON or carries no name, which callers treat as "unclassified" rather than
+// as any particular failure.
+func atprotoErrorName(body []byte) string {
+	var e struct {
+		Error string `json:"error"`
+	}
+	if json.Unmarshal(body, &e) != nil {
+		return ""
+	}
+	return e.Error
+}
+
 // getErrorHint provides context-specific troubleshooting hints based on API error type
 func getErrorHint(apiErr *atclient.APIError) string {
 	switch apiErr.Name {
@@ -164,8 +178,20 @@ func GetOrFetchServiceToken(
 				"pdsEndpoint", pdsEndpoint,
 				"statusCode", resp.StatusCode,
 				"responseBody", string(bodyBytes),
+				"errorName", atprotoErrorName(bodyBytes),
 				"hint", "PDS rejected the service token request - check PDS logs for details")
-			fetchErr = fmt.Errorf("service auth failed with status %d: %s", resp.StatusCode, string(bodyBytes))
+			// Wrap a typed APIError rather than flattening the response into a
+			// string. This error is what IsSessionInvalidError later inspects to
+			// decide whether the session is genuinely dead or the failure was
+			// transient, and it can only tell from the status code and the
+			// atproto error name. Flattening left it with a lowercased blob in
+			// which "InvalidToken" no longer matches "invalid_token", so a
+			// revoked session was classified transient and never evicted.
+			fetchErr = fmt.Errorf("service auth failed: %w", &atclient.APIError{
+				StatusCode: resp.StatusCode,
+				Name:       atprotoErrorName(bodyBytes),
+				Message:    string(bodyBytes),
+			})
 			return fetchErr
 		}
 
@@ -223,7 +249,9 @@ func GetOrFetchServiceToken(
 		// everywhere over a blip. The delete runs on a detached context so a
 		// canceled inbound request can't leave it half-done.
 		if oauth.IsSessionInvalidError(err) {
-			if delErr := refresher.DeleteSession(context.WithoutCancel(ctx), did); delErr != nil {
+			delCtx, cancelDel := context.WithTimeout(context.WithoutCancel(ctx), oauth.SessionDeleteTimeout)
+			defer cancelDel()
+			if delErr := refresher.DeleteSession(delCtx, did); delErr != nil {
 				slog.Warn("Failed to delete stale OAuth session",
 					"component", "token/servicetoken",
 					"did", did,