verdier opened a new issue, #42622: URL: https://github.com/apache/superset/issues/42622
### Bug description Concurrent MCP tool calls share a single SQLAlchemy `Session`, and each call's Flask app-context teardown removes that shared session while the other calls are still using it. Every in-flight call then holds detached instances, and the next attribute read raises `DetachedInstanceError`. This is the mechanism behind #42567. That issue reports one symptom (`generate_chart` returning an error for a chart it has already committed) and #42621 removes that symptom, but the cause below is untouched and can surface in any tool. **The chain, in three steps:** 1. `mcp_auth_hook` runs every tool call inside `_get_app_context_manager()` ([`auth.py#L1026`](https://github.com/apache/superset/blob/master/superset/mcp_service/auth.py#L1026), used at [`L1100`](https://github.com/apache/superset/blob/master/superset/mcp_service/auth.py#L1100)), which pushes a **new app context per call**. The docstring says why: so concurrent calls do not share one `g` namespace. That part works — I measured 20 distinct app contexts for 20 concurrent calls. 2. `db.session` is **not** scoped to the app context. Superset pins `flask-sqlalchemy==2.5.1`, whose `create_scoped_session` defaults to `scopefunc=_ident_func`, and `_ident_func` is `greenlet.getcurrent`. Async tool calls are asyncio tasks, not greenlets: they all run in the same greenlet on the event-loop thread, so **they all resolve to the same `Session` object**. 3. `flask-sqlalchemy` registers `@app.teardown_appcontext def shutdown_session(...)` which calls `self.session.remove()` unconditionally. So the **first call to finish** pops its own app context and removes the session **shared by every call still running**. The existing comment at [`auth.py#L1141-L1146`](https://github.com/apache/superset/blob/master/superset/mcp_service/auth.py#L1141) already notes that `db.session` is "scoped by thread (not ContextVar)" and clears it before sync tool calls, where thread-pool workers do give each call its own session. The async path has no equivalent isolation, and one context per call actively makes it worse: more contexts means more teardowns landing on the shared session. ### Evidence `apache/superset:6.1.0-py311`, `superset mcp run`, `stateless_http=True`, stock pool, N concurrent `generate_chart` calls with `save_chart=true`. I instrumented the auth hook to log the session identity per call, and wrapped `scoped_session.remove` to log its caller. Ten concurrent calls, ten distinct app contexts, **one session**: ``` [PROBE-ENTER] tool=generate_chart session=133425676349584 greenlet=... thread=... appctx=133425179069648 [PROBE-ENTER] tool=generate_chart session=133425676349584 greenlet=... thread=... appctx=133425183374416 [PROBE-ENTER] tool=generate_chart session=133425676349584 greenlet=... thread=... appctx=133425164314640 ... (10 lines, same session id, same greenlet, same thread, 10 different appctx ids) ``` Then, as calls start finishing, the teardown removes it under the others: ``` [PROBE-REMOVE] session=133425676349584 caller="flask/ctx.py", line 255, in pop | self.app.do_teardown_appcontext(exc) [PROBE-REMOVE] session=133425670911568 caller="flask/ctx.py", line 255, in pop | self.app.do_teardown_appcontext(exc) ... sqlalchemy.orm.exc.DetachedInstanceError: Instance <Slice ...> is not bound to a Session sqlalchemy.orm.exc.DetachedInstanceError: Instance <Slice ...> is not bound to a Session ``` Note the session ids differ between `remove()` calls: once the first teardown wipes the registry entry, the next database access creates a fresh session, which the next teardown wipes in turn — while the same calls are still running. ### Impact Not specific to charts, and not specific to the post-commit path: - `<Slice>` detached after `CreateChartCommand` commits — the symptom in #42567. - `<User>` detached **before** any write: on a long-running process at concurrency 20, most failures were `get_user_roles` lazy-loading `User.roles` from `superset/views/base.py` `DatasourceFilter.apply` → `can_access_all_datasources`, during the *dataset lookup* at the very start of the tool. Measured on stock 6.1.0, two bearer identities, asserted against rows in the metadata database: | concurrency | successful responses | `DetachedInstanceError` | rows written | |---|---|---|---| | 5 | 5/5 | 0 | 5 | | 10 | 2/10 | 8 | 10 | | 20 | 5/20 | 15 | 20 | | 40 | 5/40 | 35 | 40 | Every row was written. The database work succeeds; the responses report failure. For an agent client that is worse than a plain error — the usual reaction is a retry, which duplicates the object. Tools that re-query their ORM objects by id inside their own session survive this and give a false green when used as a concurrency test — `generate_dashboard` is the example, it re-queries deliberately ([`generate_dashboard.py#L285`](https://github.com/apache/superset/blob/master/superset/mcp_service/dashboard/tool/generate_dashboard.py#L285)). ### How to reproduce 1. Run `superset mcp run` (6.1.0 or master), JWT auth, `stateless_http=True`. 2. Fire N concurrent `generate_chart` calls with `save_chart=true` against a valid dataset. 3. Assert on rows in `slices`, not on the response envelope: from N=5 upward, rows are created that the client is told failed. Happy to share the harness. ### Possible directions I did not want to guess a fix for something this structural, so, options as I see them: - **Scope the session per asyncio task.** `flask-sqlalchemy` 2.5.1 accepts `session_options={"scopefunc": ...}`; a scope function returning the current `asyncio` task (falling back to greenlet/thread outside a loop) would give each concurrent tool call its own session, and each teardown would only remove its own. This touches the whole app's session scoping, not just MCP, so it needs care on the web tier. - **Run async tool bodies on a worker thread**, as the sync path already effectively does. Thread scoping then does the isolation, at the cost of a thread per concurrent call. - **Do not push a per-call app context** and instead isolate `g` some other way, so that a teardown never fires while other calls are in flight. This trades one race for the other unless the session is also isolated, so it probably only works combined with the first option. I'd rather implement whichever direction maintainers prefer than push one unilaterally. Happy to do the work and measure it on the same harness. ### Screenshots/recordings _No response_ ### Superset version master / latest-dev ### Python version 3.11 ### Node version I don't know ### Browser Not applicable ### Additional context Related: #42567 (symptom in `generate_chart`), #42621 (removes that symptom, does not address this). ### Checklist - [x] I have searched Superset docs and Slack and didn't find a solution to my problem. - [x] I have searched the GitHub issue tracker and didn't find a similar bug report. - [x] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text or in a screenshot. -- 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]
