rusackas commented on PR #43623: URL: https://github.com/apache/superset/pull/43623#issuecomment-5460070558
Heya, one more thing before this can merge, though I want to flag upfront I haven't proven this against the real code, just traced it and confirmed the underlying SQLAlchemy behavior with an isolated repro, so take this as a question to verify rather than a confirmed bug. `execute_with_oauth2_retry` loads the token row via the ambient `db.session` up front for `rejected_access_token`, but the forced refresh commits on a separate isolated session. On retry, `get_oauth2_access_token` queries that same ambient `db.session` again, and since the row's already in its identity map, SQLAlchemy _can_ hand back the stale cached object instead of re-reading the DB, unless something expires it in between. For a query that fails on its first statement (no `db.session.commit()` has happened yet at that point), I don't see what would expire it, but I haven't run this end to end against a real DB to confirm the retry actually uses the stale token in practice. Could we add a test that uses two genuinely separate sessions (not `Session` mocked to equal `db.session`, like the current force-refresh tests do) and confirms the retried `operation` sees the new token? If that turns out to be a real gap, a `db.session.expire(token)` (or `expire_all()`) right after the forced commit seems like the likely fix, but happy to be wrong here. -- 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]
