Repository navigation
fix(export): stream CSV exports inside their request context - #45952
Conversation
Streaming CSV exports (100k+ rows, and SQL Lab streaming downloads) run
their query from the response body generator, which the WSGI server
iterates only after the view has returned. The generator replayed a
snapshot of flask.g inside a fresh app context, and that breaks
databases that authenticate with per-user OAuth2, Snowflake in
particular, so these exports returned only the __STREAM_ERROR__ marker:
- The replayed g.user is the User loaded by the request's metadata
session. The event logger's commit expires it and the request teardown
removes that session, so reading g.user.id to look up the OAuth2 token
raised DetachedInstanceError.
- The generator had no request context. Snowflake only uses the user's
OAuth2 token inside a request, so even with a usable user it connected
without the token ("251006: Password is empty"). The OAuth2 redirect
URI and the query source passed to DB_CONNECTION_MUTATOR also come
from the request.
Record the request's contexts in the view and push them again only
while the body is iterated, as flask.stream_with_context does from
Flask 3.1.2 (earlier releases push them up front, which leaves them on
the worker thread if the response never reaches the server). Reload a
detached acting user by primary key, failing closed if it no longer
exists, and commit right after the engine is built so a refreshed OAuth2
token is kept and no metadata transaction stays open while rows stream.
This replaces the flask.g snapshot and replay.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
The two request-context tests asked whether any request context was active, so a context another test leaves pushed in the same worker would make them fail (or pass) for the wrong reason. Compare against the context of the request the test builds instead: it must be active while the body streams and gone afterwards, whatever else is on the thread. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #45952 +/- ##
==========================================
+ Coverage 84.76% 84.85% +0.09%
==========================================
Files 3072 3074 +2
Lines 206129 207783 +1654
Branches 48541 48916 +375
==========================================
+ Hits 174731 176324 +1593
+ Misses 28100 28091 -9
- Partials 3298 3368 +70
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve: I found no blockers. The fix is correct and needed on master.
We checked it on base vs head against a real app, using a Snowflake-like OAuth2 stand-in. Head exports the full CSV where master returns only the __STREAM_ERROR__ marker, and the impersonation user and query source are correct on head. We also took out each of the three parts of the change in turn (the request-context re-push, the user reload and the post-engine commit), and each removal breaks the export, so all three are needed. Tested on werkzeug and gunicorn gthread/gevent, with these all clean on head: concurrent exports from two users, thread reuse, client disconnect, teardown running twice, and Flask 2.2.5 to 3.1.3. The new tests pass on head, and 4 of them fail on master. On CodeAnt's refresh-token race: it reproduces only with Redis lock coordination, and the buffered path on master has the same window. This PR narrows the window, so a fix belongs in superset/utils/oauth2.py as a follow-up (commit non-forced refreshes under the lock, the way force=True already does).
I left one non-blocking 🟡 inline, with applyable suggestions.
The export pushed the request's contexts on the worker thread while the body was iterated, so they stayed pushed while it was suspended between chunks. If the server abandons the body mid-stream (e.g. encoding a chunk raises and the traceback keeps the generator alive), the thread's next request inherits them: on a single-threaded server it either gets the exporting user or fails with "Popped wrong request context" when the abandoned generator is finally closed. Step the export inside a contextvars Context of its own, so the request's contexts are only ever pushed there and never on the worker thread. Co-authored-by: Rafael Benitez <2151636+rebenitez1802@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
alexandrusoare
left a comment
There was a problem hiding this comment.
Read every hunk and checked the Flask 3.1.3 / Werkzeug / Flask-SQLAlchemy mechanics. Two latent notes inline, neither a live break.
Clean otherwise: re-pushing a popped RequestContext/AppContext is legal on 3.1.3, double teardown is harmless (no teardown_request handlers registered, Flask-SQLAlchemy's session.remove() is idempotent), the new session.commit() doesn't emit a stray dbs UPDATE for the merged-but-unchanged Database, and the abandonment fix in the third commit does what it says.
The export merged the Database loaded by the request into its own session and then commits right after building the engine. Merging copies the values read during the request over the row's current ones, so a change made to the connection in the meantime would be written back by that commit. Reload it by primary key, like the acting user. Also document that, outside a request, the export runs as the caller's g.user, and reuse the detached-user test helper. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
If something read part of a streaming export's body while the request was still being handled, the export pushed the request's contexts again in its own contextvars Context. Flask's pop at the end of the request then tried to reset that token from the worker thread's Context: it raised ValueError, skipped the request's teardown and left its contexts on the thread. The contexts are already active at that point, so they are not pushed again. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Pushed the two fixes from the last review: the export reloads the database instead of merging it, and it no longer pushes the request again when the body is read before the request ends. I also ran it end to end on master, against a local Snowflake stand-in that only accepts OAuth logins: 1, 10 and gevent workers, CSV compression, a latin-1 export, Redis locks, the deployment-wide OAuth2 client, CSV reports and embedded guests. Every export also checks that the metadata rows don't change and that the next requests on the worker don't see the exporting user. All of these pass on this branch. Live run of the main cases, before the fix, on the previous revision and on this branch: streaming-export-e2e.mp4 |
SUMMARY
Streaming CSV exports still fail on databases that use per-user OAuth2, Snowflake in particular. This covers exports with a row limit at or above
CSV_STREAMING_ROW_THRESHOLD, and SQL Lab streaming downloads. The file the user gets holds only__STREAM_ERROR__:Export failed. Please try again in some time.#44425 went after this by resolving
g.userbefore the generator runs. On master there are two more problems, and the export only works once both are fixed:DBEventLoggerhas committed, which expires everything in the session, and the request teardown has removed the metadata session. So theUserthat fix(export): keep the acting user in streaming CSV exports #44425 hands to the generator is detached and expired, andDatabase._get_sqla_engineraisesDetachedInstanceErroras soon as it readsg.user.idto look up the OAuth2 token.flask.ginside a fresh app context, and that can't bring the request context back.SnowflakeEngineSpec.is_oauth2_enabled()andimpersonate_user()turn OAuth2 off outside a request on purpose (to keep it away from alerts and reports). So even with a usable user, the connection goes out without the token and fails with251006: Password is empty. The OAuth2 redirect URI (when it falls back tourl_for) and the query source given toDB_CONNECTION_MUTATORcome from the request too.The change, all in
BaseStreamingCSVExportCommand:run()records the request's app and request contexts in the view and pushes them again only while the body is iterated. This is whatflask.stream_with_contextdoes from Flask 3.1.2. I didn't callstream_with_contextitself, because earlier releases (3.1.1 and before, whichpyproject.tomlstill allows) push the contexts as soon as the body is created. If the response then never reaches the server (anafter_requesthook fails, for example), those contexts stay on the worker thread, and later requests on that thread inherit the exporting request'sgand logged-in user. The export also steps in a contextvars Context of its own, so the contexts are never pushed on the worker thread at all: a body the server abandons mid-stream can't leave them behind either, and if the body is read before the request ends they're already active and aren't pushed again. This replaces theflask.gsnapshot and replay (capture_g_context/preserve_g_context).g.user(and Flask-Login's cached user) at the fresh user. It reloads rather than merges so values read during the request can't be written back by the commit below. If the user no longer exists it fails closed. Users that aren't ORM instances, such as embedded guest users, are left alone.I checked the two halves of the context fix one at a time against a running app (Flask's test client also iterates the body after the view returns). With only the request context, the export fails with the
DetachedInstanceError. With only the user reload, it fails withPassword is empty. With both, the CSV is complete.Notes:
stream_with_context. The registered handlers (Flask-SQLAlchemy's session removal, Flask-Limiter's context reset) are idempotent._open_with_oauth2_retry, so a token the database rejects while the store still considers it valid isn't refreshed.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Recorded locally against a Snowflake stand-in: fakesnow's implementation of Snowflake's REST protocol, so the real connector talks to it unchanged, wrapped to accept only OAuth logins with a token from its own OAuth2 endpoints (like a Snowflake user with no password). The table has 150,000 rows, and a 100,000 row limit makes the exports stream.
Before: authorization works and the 100k-row query runs with the user's token, but the SQL Lab download and the chart's "Export to .CSV" both fail. The body streamed by
/api/v1/chart/datais only the error marker.streaming-export-oauth2-before.mp4
After: both exports finish, and each downloaded file has the header plus 100,000 rows.
streaming-export-oauth2-after.mp4
TESTING INSTRUCTIONS
pytest tests/unit_tests/commands/chart/streaming_export_command_test.py \ tests/unit_tests/commands/sql_lab/streaming_export_command_test.pyThe new tests cover:
current_userincluded), and failing closed when the user is gone;The first two fail on master. Each request in these tests pushes its own app context, as
Flask.wsgi_appdoes, so they can't pass for the wrong reason.End-to-end results, with a live recording before and after the fix, are in this comment.
Manually:
oauth2_client_infoon the connection, orDATABASE_OAUTH2_CLIENTS) and "Impersonate logged in user" turned on.Before: "Export failed. Please try again in some time." After: the full CSV.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code