Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions test/test_token_refresh.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
"""Regression test for #2110: background token refresh thread must survive
authlib OAuthError (e.g. invalid_grant), not just httpx.HTTPError."""

import pytest
from authlib.common.errors import AuthlibBaseError
from httpx import HTTPError


def test_authlib_base_error_is_not_http_error():
"""AuthlibBaseError does NOT inherit from httpx.HTTPError, confirming
the bug: the old `except HTTPError` clause could never catch it."""
assert not issubclass(AuthlibBaseError, HTTPError)


def test_authlib_base_error_caught_by_fixed_except_clause():
"""The fixed except clause `(HTTPError, AuthlibBaseError)` must catch
authlib protocol-level errors like invalid_grant."""
try:
raise AuthlibBaseError("invalid_grant")
except (HTTPError, AuthlibBaseError):
pass # This is what the fixed code does
else:
pytest.fail("AuthlibBaseError was not caught by (HTTPError, AuthlibBaseError)")


def test_http_error_still_caught_by_fixed_except_clause():
"""The fix must not break the existing HTTPError handling."""
try:
raise HTTPError("connection reset")
except (HTTPError, AuthlibBaseError):
pass
else:
pytest.fail("HTTPError was not caught by (HTTPError, AuthlibBaseError)")


def test_oauth_error_subclass_caught():
"""Concrete authlib errors (e.g. OAuthError) inherit from AuthlibBaseError
and must also be caught."""
from authlib.integrations.base_client.errors import OAuthError

assert issubclass(OAuthError, AuthlibBaseError)
try:
raise OAuthError("invalid_grant")
except (HTTPError, AuthlibBaseError):
pass
else:
pytest.fail("OAuthError was not caught by (HTTPError, AuthlibBaseError)")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests general python behaviour, I dont think we should have them in the repo

5 changes: 4 additions & 1 deletion weaviate/connect/v4.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
)

import grpc
from authlib.common.errors import AuthlibBaseError # type: ignore
from authlib.integrations.httpx_client import ( # type: ignore
AsyncOAuth2Client,
OAuth2Client,
Expand Down Expand Up @@ -591,8 +592,10 @@ def periodic_refresh_token(refresh_time: int, _auth: Optional[_Auth]) -> None:
# saved credentials
refresh_session()
refresh_time = update_refresh_time()
except HTTPError as exc:
except (HTTPError, AuthlibBaseError) as exc:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general this makes sense, but I'm worried a bit about continued retrying for non-recoverable errors. Could you add a check for which kind of error it is to determine if we should retry every second?

# retry again after one second, might be an unstable connection
# or an OAuth2 protocol-level rejection (e.g. invalid_grant
# from authlib) — neither should kill the refresh thread (#2110)
refresh_time = 1
_Warnings.token_refresh_failed(exc)

Expand Down
Loading