goingforstudying-ctrl opened a new pull request, #42629: URL: https://github.com/apache/superset/pull/42629
### SUMMARY Ran into #42622 while looking at the MCP concurrency reports: fire two async tool calls at the same time and one of them can come back with `DetachedInstanceError` even though each call pushes its own app context. Dug into why, and the session scope is the culprit — `db.session` (flask-sqlalchemy 2.5.1) keys its registry by greenlet ident, and asyncio tasks on the event loop all live on the same greenlet, so N concurrent tool calls end up sharing ONE Session. Whichever call finishes first pops its app context, the teardown handler calls `db.session.remove()`, and every other in-flight call is suddenly holding detached instances. That's also what makes #42567's `generate_chart` report failure for a chart it had already committed, and why retrying agents end up creating duplicates. The fix keys the registry on a per-call token instead. `_get_app_context_manager()` sets a fresh ContextVar token when it pushes the call's app context, and the MCP scopefunc resolves `db.session` from that token — each call gets its own Session, and the app-context teardown removes exactly that call's. Anywhere outside an MCP tool call the scopefunc falls back to the plain greenlet ident, so web, CLI, and Celery paths behave exactly as before. It's wired up from `init_fastmcp_server()` so every serving mode gets it. The new tests include an interleaved two-task race that reproduces the shared-session teardown (fails without the fix, passes with it), plus a counterfactual that pins down the old behavior: with the greenlet scope restored, both calls really do resolve to the same Session and the first teardown removes it out from under the survivor. Not 100% sure the ContextVar-token approach is what you'd pick long-term vs. upgrading flask-sqlalchemy, but it's contained to the MCP service and doesn't move anything else. ### TESTING INSTRUCTIONS - `pytest tests/unit_tests/mcp_service/test_session_scope.py` (5 new tests) - Ran the neighboring suites locally: `test_g_user_race_condition`, `test_auth_*`, `test_middleware`, `test_rbac_tool_enforcement`, the top-level `mcp_service` tests, and the full `chart/` tool suite (~1900 tests total) — all green - `pre-commit run` (mypy, ruff, pylint) passes on the changed files ### 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]
