Don't fail the request with a bare 500 on transient JWT refresh errors

[kjh0623]

Problem

JWTRefreshMiddleware.dispatch runs outside the FastAPI/Starlette exception handlers, and its try/except only catches HTTPException and AuthManagerRefreshTokenExpiredException. Any other exception raised while resolving/refreshing the user propagates to the ASGI layer and surfaces as a bare HTTP 500 — for a request that may not have needed the refresh at all.

The concrete way we hit this in production: PgBouncer recycles a backend connection underneath the API server, and the next request's _refresh_user() → resolve_user_from_token() → user lookup raises sqlalchemy.exc.OperationalError ("server closed the connection unexpectedly"). The UI shows "Internal Server Error"; a manual refresh usually succeeds. Related reports of the same pain point: #57859, discussion #59487. (#62153 fixed the poisoned-session cascade after such a drop; this PR addresses the request that hits the error itself.)

Fix

Add a final except Exception arm around the refresh block that treats the failure as "could not refresh this time":

• log a warning with the traceback,
• proceed without injecting the user — the route's own auth dependency revalidates the token exactly as it does today when the middleware doesn't inject, and
• leave the cookie untouched (no forced logout), so a subsequent request refreshes normally once the backend recovers.

HTTPException / token-expiry semantics are unchanged — those still clear the cookie via the existing arm.

Testing

New unit test: _refresh_user raising a non-HTTP exception → request completes with the downstream response, no set-cookie header, warning logged. Existing middleware tests unchanged and passing.

We have been running the equivalent of this change in production (as a patch on 3.1.8) on a deployment serving a ~3,600-DAG instance; the recurring bare 500s on the auth path disappeared.

---

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Generated-by: Claude Code following the guidelines

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[SameerMesiah97]

I think there is a disconnect between the stated motivation behind this PR and the actual code. You appear to want to hande transient infrastructure failures (e.g. dropped DB connections), but the imeplementation and tests appear to broaden this to any exception from _refresh_user(). Is that intentional?

[kjh0623]

Good question — the broad catch is intentional, but you're right that the observability trade-off deserves a callout.

Why except Exception rather than a specific OperationalError: this middleware is a generic auth layer that delegates the actual lookup to the configured auth manager (_refresh_user → resolve_user_from_token). It doesn't — and shouldn't — know which backend that auth manager uses, so it can't enumerate which exceptions are "transient infra." Catching sqlalchemy.exc.OperationalError here would bake a DB-backend assumption into a layer that's meant to be backend-agnostic, and it would still let any other transient cause surface as the exact bare 500 this PR is trying to remove.

Why failing open is safe regardless of the exception type: the refresh is a best-effort, proactive step, not the authoritative auth check. If it doesn't inject a user, the route's own auth dependency revalidates the token exactly as it does today when the middleware chooses not to refresh. So a failure here is never a security downgrade — it just means "no proactive refresh this time," identical to the existing no-refresh path.

On the part I think is fair — masking programming errors: they aren't silently swallowed (the traceback is logged with exc_info=True), but WARNING is easy to miss, and a monitor keyed on 5xx would go quiet on a genuine regression. If you'd prefer, I'll switch the unexpected arm to log.exception(...) (ERROR level) so real bugs still surface loudly in monitoring while transient blips stay non-fatal. That keeps the resilience without hiding regressions.

Would that address the concern, or would you rather I narrow the catch despite the coupling/coverage downsides above?