gabotorresruiz commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4137077763
##########
superset/utils/oauth2.py:
##########
@@ -322,12 +423,11 @@ def execute_with_oauth2_retry( # noqa: C901
database.db_engine_spec.engine,
)
try:
- access_token = refresh_oauth2_token(
+ access_token = force_refresh_oauth2_token(
Review Comment:
This block worries me a bit. Sending `get_raw_connection()` and
`get_inspector()` through here also sends `OAuth2TokenRefreshError` through
here, and that one is already terminal: `_refresh_oauth2_token_locked` raises
it *after* deleting the token row and flushing on the caller's `db.session`
without committing (`superset/utils/oauth2.py:230`). `needs_oauth2()` still
answers `True` for it, since Snowflake lists `OAuth2TokenRefreshError` in
`oauth2_exception` and the `BaseEngineSpec` default `OAuth2RedirectError` is
its parent class. So we force a second exchange: `force_refresh_oauth2_token`
opens its own `Session`, still sees the row the request's session deleted but
has not committed, presents the same refused refresh token to the provider
again, and its own `DELETE` then waits on the row lock that same request is
holding.
I verified it on `796cbf1` against a Postgres metadata database with a Redis
coordination backend, stored access token expired and the provider refusing the
refresh:
* this branch: the refresh token is presented twice, then the request blocks
on the row lock. It came back at all only because I had set `lock_timeout` on
the test database; the Postgres default is `0`. The user gets an
`OperationalError`, not the sign-in redirect.
* `ffe9a00`, same scenario: one exchange, `OAuth2RedirectError`, 0.01s.
`get_inspector()` behaves identically. The login-rejection case this PR
targets is unaffected, because nothing deleted the row first. It is also
invisible under the KeyValue lock backend, since
`AcquireDistributedLock._acquire_kv` is `@transaction` wrapped and commits
`db.session` on the way in, so only `DISTRIBUTED_COORDINATION_CONFIG`
deployments see it.
Treating an already refused exchange as terminal fixes it, and all 125 tests
in `oauth2_tests.py` and `core_test.py` still pass with it applied:
```python
if not is_oauth2_error:
raise
if isinstance(ex, OAuth2TokenRefreshError):
# the exchange already ran and the provider refused it
app.config["STATS_LOGGER"].incr("oauth2.forced_refresh.exchange_rejected")
database.start_oauth2_dance()
raise
```
A `test_get_raw_connection_does_not_re_exchange_a_refused_refresh_token`
(stored token already expired, `get_oauth2_fresh_token` raising
`oauth2_exception`, asserting one call and `OAuth2RedirectError`) would lock it
in. The new tests miss it because they all start from a token the store still
considers valid, so `get_oauth2_access_token` never refreshes before the login.
Or am I misreading the ordering here?
##########
superset/utils/oauth2.py:
##########
@@ -268,6 +268,107 @@ def _refresh_oauth2_token_locked( # noqa: C901
return token.access_token
[email protected]_exception(
+ backoff.expo,
+ AcquireDistributedLockFailedException,
+ factor=0.1,
+ base=2,
+ max_tries=8,
Review Comment:
Not a blocker, and the contention handling clearly works: 8 concurrent opens
against a Postgres metadata database with a Redis lock gave me 8 of 8 rendering
off a single exchange, at 0s, 1s and 3s of provider latency.
Recording the trade though. `factor=0.1, base=2, max_tries=8` is roughly
12.7s of waiting worst case, while `DistributedLock` is entered with
`ttl_seconds=30`, so an exchange slower than the backoff window guarantees
every loser waits out the whole window and *then* gets the sign-in dance
anyway. At 20s of provider latency I measured 1 of 8 rendering, with the other
7 sitting in the backoff for about 12s first; before this change they failed
immediately. Is the mismatch with the TTL deliberate, or worth moving the
window closer to it?
--
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]
