aminghadersohi commented on PR #44765: URL: https://github.com/apache/superset/pull/44765#issuecomment-5884336285
Review feedback addressed in this PR — no fast-follow. **@rebenitez1802 — 🟡 Medium, concurrent dashboard charts lose the refresh race and fail with an opaque lock error → 796cbf1** Confirmed exactly as described: `refresh_oauth2_token(force=True)` took the non-blocking `DistributedLock` with no backoff, so `LockAlreadyHeldException` fell through `execute_with_oauth2_retry`'s generic `except Exception` (`oauth2.forced_refresh.exchange_transient_failure`) and re-raised. Took your first option — mirror the contention tolerance `get_oauth2_access_token` already has — plus the store re-read as a floor: - `_refresh_oauth2_token_forced` wraps the forced refresh in the same `@backoff.on_exception(backoff.expo, AcquireDistributedLockFailedException, factor=0.1, base=2, max_tries=8)` as the expired-token path. A loser retries until the winner commits and releases; its re-read under the lock then short-circuits on `rejected_access_token != token.access_token`, so a rotating refresh token is never exchanged twice. - If the lock never frees within the window, `force_refresh_oauth2_token` falls back to `_read_refreshed_access_token`, which reads through an isolated `Session` so the winner's commit is visible regardless of the caller's snapshot, and returns it only when it actually replaced the rejected token. Counted as `oauth2.forced_refresh.lock_contended`. - Losers therefore render with the winner's token instead of raising; when nobody refreshed, the result is still the normal sign-in dance, never a bare lock error. Tests, including the one you specified: - `tests/unit_tests/models/core_test.py::test_get_raw_connection_survives_a_lost_refresh_race` — end-to-end on the path this PR newly routes: `DistributedLock` raising `LockAlreadyHeldException`, asserting the connection opens with the refreshed token and no dance is started. Fails without the change with `LockAlreadyHeldException`. - `oauth2_tests.py::test_force_refresh_waits_out_lock_contention` — contended then acquired: reuses the winner's token, no second exchange. - `oauth2_tests.py::test_force_refresh_reads_committed_token_when_lock_never_frees` and `::test_force_refresh_returns_none_when_no_one_refreshed` — giveup fallback, both directions. - `oauth2_tests.py::test_execute_with_oauth2_retry_survives_lock_contention` — the wrapper no longer surfaces the lock failure. **CodeAnt inline, `superset/models/core.py:1269` — not valid, [replied with evidence](https://github.com/apache/superset/pull/44765#discussion_r4129978961)** `sqla.inspect(engine)` logs in: SQLAlchemy 2.0.52 `Inspector._init_engine` calls `engine.connect().close()`, so the login is inside the retry (`test_get_inspector_refreshes_a_token_rejected_at_login`). Lazy calls reconnect outside it by design — retrying them would replay the caller's block, which `_open_with_oauth2_retry` deliberately never does. Verification: `pre-commit run` clean on the touched files; `tests/unit_tests/utils/oauth2_tests.py`, `tests/unit_tests/models/`, `tests/unit_tests/sql_lab_test.py` → 564 passed (remaining failures are local missing optional drivers, `trino` / `sqlalchemy_bigquery`, and reproduce on an unmodified tree). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
