Skip to content

fix(oauth2): silence lock acquisition errors on token refresh #39463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
betodealmeida wants to merge 1 commit into
base: master
Choose a base branch
from
+41 −0
Open

fix(oauth2): silence lock acquisition errors on token refresh #39463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions superset/utils/oauth2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ def generate_code_challenge(code_verifier: str) -> str:
factor=10,
base=2,
max_tries=5,
raise_on_giveup=False,
Copy link
Copy Markdown
Contributor

@codeant-ai-for-open-source codeant-ai-for-open-source bot Apr 17, 2026

Choose a reason for hiding this comment

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

Suggestion: Setting raise_on_giveup=False swallows all AcquireDistributedLockFailedException cases after retries, including Redis/backend lock failures (not just benign lock contention). That makes real infrastructure errors look like "no token" and can incorrectly trigger fallback auth flows instead of surfacing an operational failure. [logic error]

Severity Level: Major ⚠️ 
- ❌ OAuth2 token refresh masks Redis/distributed-lock outages.
- ⚠️ Users see unnecessary OAuth2 re-auth flows.
- ⚠️ Operational lock failures no longer surface as errors.
Suggested change
raise_on_giveup=False,
raise_on_giveup=True,
Steps of Reproduction ✅ 
1. Configure an OAuth2-enabled database (e.g. Google Sheets) and ensure it has an expired
access token with a non-empty refresh token stored in `DatabaseUserOAuth2Tokens` so that
`get_oauth2_access_token()` takes the refresh path in `superset/utils/oauth2.py:88-124`.

2. Cause the distributed lock backend to fail (e.g. Redis unavailable), which per
`superset/distributed_lock/__init__.py:10-11` and
`tests/unit_tests/distributed_lock/distributed_lock_tests.py:22-32` makes
`DistributedLock(...)` raise `AcquireDistributedLockFailedException` both when the lock is
already held and when the Redis connection fails.

3. Trigger a code path that refreshes the OAuth2 token, such as requesting table names for
a GSheets database so that `GSheetsEngineSpec.get_table_names()` in
`superset/db_engine_specs/gsheets.py:271-283` calls `get_oauth2_access_token(...)`, which
then calls `refresh_oauth2_token()` and enters the `DistributedLock` context at
`superset/utils/oauth2.py:127-141`, causing `AcquireDistributedLockFailedException` due to
the failing lock backend.

4. Observe that because of the backoff decorator configuration at
`superset/utils/oauth2.py:79-87` with `raise_on_giveup=False`, after exhausting retries
the `AcquireDistributedLockFailedException` is swallowed and `get_oauth2_access_token()`
returns `None`; callers like `GSheetsEngineSpec.get_table_names()` (`gsheets.py:276-282`)
treat this as "no token" and invoke `database.start_oauth2_dance()` instead of surfacing
an operational failure of the distributed lock backend.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset/utils/oauth2.py
**Line:** 85:85
**Comment:**
	*Logic Error: Setting `raise_on_giveup=False` swallows all `AcquireDistributedLockFailedException` cases after retries, including Redis/backend lock failures (not just benign lock contention). That makes real infrastructure errors look like "no token" and can incorrectly trigger fallback auth flows instead of surfacing an operational failure.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

giveup_log_level=logging.DEBUG,
)
def get_oauth2_access_token(
config: OAuth2ClientConfig,
Expand Down
39 changes: 39 additions & 0 deletions tests/unit_tests/utils/oauth2_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,45 @@ def test_encode_decode_oauth2_state(
assert decoded["user_id"] == 2


def test_get_oauth2_access_token_lock_not_acquired_no_error_log(
mocker: MockerFixture,
caplog: pytest.LogCaptureFixture,
) -> None:
"""
Test that when a distributed lock can't be acquired, no error is logged and
the function returns None instead of raising.

This scenario occurs when a dashboard with multiple charts from the same
OAuth2-enabled DB has an expired token: simultaneous requests compete for
the lock, and only the first one wins. The rest should silently return None.
"""
import logging

from superset.exceptions import AcquireDistributedLockFailedException

mocker.patch("time.sleep") # avoid backoff delays in tests

db = mocker.patch("superset.utils.oauth2.db")
db_engine_spec = mocker.MagicMock()
token = mocker.MagicMock()
token.access_token = "access-token" # noqa: S105
token.access_token_expiration = datetime(2024, 1, 1)
Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review bot Apr 17, 2026

Choose a reason for hiding this comment

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

Datetime created without timezone info

Add tzinfo argument to datetime() call. Use datetime(2024, 1, 1, tzinfo=timezone.utc) or similar to specify timezone.

Code suggestion
Check the AI-generated fix before applying
  -from datetime import datetime
  +from datetime import datetime, timezone
  from typing import cast
 @@ -360,7 +361,7 @@
      db = mocker.patch("superset.utils.oauth2.db")
      db_engine_spec = mocker.MagicMock()
      token = mocker.MagicMock()
      token.access_token = "access-token"  # noqa: S105
  -    token.access_token_expiration = datetime(2024, 1, 1)
  +    token.access_token_expiration = datetime(2024, 1, 1, tzinfo=timezone.utc)

Code Review Run #63dbd9

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

token.refresh_token = "refresh-token" # noqa: S105
db.session.query().filter_by().one_or_none.return_value = token

mocker.patch(
"superset.utils.oauth2.refresh_oauth2_token",
side_effect=AcquireDistributedLockFailedException("Lock not available"),
)

with freeze_time("2024-01-02"):
with caplog.at_level(logging.DEBUG):
result = get_oauth2_access_token({}, 1, 1, db_engine_spec)

assert result is None
assert not any(record.levelno >= logging.ERROR for record in caplog.records)


def test_get_oauth2_redirect_uri_from_config(mocker: MockerFixture) -> None:
"""
Test that get_oauth2_redirect_uri returns the configured value when set.
Expand Down
Loading