verdier opened a new pull request, #43007:
URL: https://github.com/apache/superset/pull/43007
### SUMMARY
Fixes #42622 — taking up @rusackas' preference in the issue thread: scope
the session per asyncio task, mirroring what the thread pool already does for
sync calls.
`flask-sqlalchemy` 2.5.1 scopes `db.session` with `scopefunc=_ident_func`,
and `_ident_func` is `greenlet.getcurrent`. Async MCP 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**. Each call still
runs in its own Flask app context (deliberately, so `g.user` does not race),
and `flask-sqlalchemy` registers `teardown_appcontext → session.remove()`. So
the **first call to finish removes the session shared by every call still
running**, which is then left holding detached instances. The next attribute
read raises `DetachedInstanceError` — for a chart the tool has already
committed (#42567), or on a `User` lazy-load before any write at all.
The fix scopes on the running task, and keeps the greenlet identity off the
event loop, so the WSGI web tier, Celery workers and the MCP thread pool are
unaffected:
```python
def _session_scope_ident() -> Any:
try:
task = asyncio.current_task()
except RuntimeError: # no running event loop
return _greenlet_ident()
return task if task is not None else _greenlet_ident()
db = get_sqla_class()(session_options={"scopefunc": _session_scope_ident})
```
**Measured** on `apache/superset:6.1.0-py311`, `superset mcp run`,
`stateless_http=True`, N concurrent `generate_chart` calls with
`save_chart=true`, asserted against rows in the metadata database (the response
envelope alone is not trustworthy here — the rows are written either way, it is
the responses that lie):
| concurrency | before | after |
|---|---|---|
| 10 | 2/10 succeeded, 8 `DetachedInstanceError` | 10/10, 0 errors |
| 20 | 6/20 succeeded, 14 `DetachedInstanceError` | 20/20, 0 errors |
A probe on the auth hook, logging `id(db.session())` per call, counts what
is actually handed out: **one session for ten concurrent calls before, ten
after** — ten distinct tasks in both runs.
### ⚠️ One consequence worth a decision
Sessions are no longer shared, so N concurrent tool calls now hold N
connections instead of one. With the stock pool (`pool_size=5,
max_overflow=10`) a burst of 20 concurrent calls exhausts it — and because the
tool bodies do blocking DB work **on the event loop**, waiting on the pool
blocks the loop rather than yielding to the calls that would release a
connection, so the burst stalls instead of queueing. Sizing the pool for the
intended concurrency (`SQLALCHEMY_ENGINE_OPTIONS`) is enough — that is where
the 20/20 above comes from.
That is the honest trade-off of this direction: a silent-corruption bug
becomes a capacity limit that operators can see and size for. Whether the MCP
server should additionally bound its own concurrency, or derive a pool size
from it, feels like a separate decision — happy to follow up with either if you
want it in this PR.
### TESTING INSTRUCTIONS
`tests/unit_tests/extensions/test_session_scope.py` covers both directions,
with no database or MCP server needed:
- the scope function returns the running task inside a loop, and the
greenlet identity outside one (unchanged web tier)
- two concurrent tasks, each in its own app context, each on its own row:
the sibling's teardown no longer detaches the reader's instance
- the same scenario on the library default still raises
`DetachedInstanceError` — the bug, pinned
```bash
pytest tests/unit_tests/extensions/test_session_scope.py
```
To reproduce end to end: run `superset mcp run` with JWT auth, fire 10
concurrent `generate_chart` calls with `save_chart=true` against a valid
dataset, and assert on rows in `slices` rather than on the response envelope.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #42622
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
