fix(mcp): scope db.session per asyncio task to prevent cross-call teardown

[devin-ai-integration]

SUMMARY

Fixes #7.

Concurrent async MCP tool calls shared a single SQLAlchemy Session, and each call's per-call Flask app-context teardown removed that shared session out from under the still-running calls, detaching their ORM instances (DetachedInstanceError).

Root cause, unchanged from the issue write-up:

• mcp_auth_hook pushes a new app context per call (correct — isolates g).
• flask-sqlalchemy==2.5.1 scopes db.session with scopefunc=_ident_func (= greenlet.getcurrent). Async tool calls are asyncio tasks that all run in the same greenlet on the event-loop thread, so they all resolve to one Session.
• flask-sqlalchemy's @app.teardown_appcontext shutdown_session calls db.session.remove() unconditionally, so the first call to finish removes the session shared by every other in-flight call.

Fix (issue's first suggested direction — scope the session per asyncio task): install a loop-aware scopefunc on the SQLAlchemy scoped session:

def _session_scopefunc():
    try:
        task = asyncio.current_task()
    except RuntimeError:      # no running event loop
        task = None
    return task if task is not None else _ident_func()

db = get_sqla_class()(session_options={"scopefunc": _session_scopefunc})

• Inside a running event loop (the MCP async path) each concurrent tool call is a distinct asyncio.Task, so each gets its own Session, and scoped_session.remove() at teardown only clears the finishing task's registry entry — never another call's.
• Outside a running loop (the web/WSGI and Celery tiers) asyncio.current_task() raises RuntimeError, so the identity falls back to the exact previous default (_ident_func = greenlet/thread). No behavior change off the event loop.

This targets the structural cause behind apache#42567 (not just the generate_chart symptom removed by apache#42621), so any tool re-lazy-loading ORM attributes (e.g. User.roles during the dataset access check) is covered, not only the post-commit chart path.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable.

TESTING INSTRUCTIONS

New regression test tests/unit_tests/mcp_service/test_session_scope_isolation.py:

• _session_scopefunc returns a distinct scope key per concurrent asyncio task, and falls back to flask_sqlalchemy._ident_func outside a running loop.
• Reproduces the bug with the default greenlet identity: two tasks load ORM instances, one commits + tears down, and the other's re-read raises DetachedInstanceError.
• Same scenario with _session_scopefunc: the other task keeps its own session and reads its instance cleanly.

Run:

pytest tests/unit_tests/mcp_service/test_session_scope_isolation.py
pytest tests/unit_tests/mcp_service/   # full MCP suite (3155 passed)

ADDITIONAL INFORMATION

• Has associated issue: MCP: concurrent tool calls share one SQLAlchemy session and remove it from under each other #7
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
• Introduces new feature or API
• Removes existing feature or API

Link to Devin session: https://app.devin.ai/sessions/1202e31f5a654a939d90bb6aecbb2fcf

---

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[devin-ai-integration]

Devin Review found 1 potential issue.

View 1 additional finding in Devin Review.

[devin-ai-integration]

🟡 Database sessions opened during an async call can be held forever, slowly using up database connections

Database work started inside an async call is now filed under that call's own identity (asyncio.current_task() at superset/extensions/__init__.py:189) but is only ever released when the same call cleans up, so any async path that borrows an outer cleanup never releases its database connection.
Impact: Long-running MCP servers can slowly accumulate unreleased database sessions and connections, eventually exhausting the connection pool.

Why task-keyed scoping can orphan registry entries

scoped_session stores one Session per value returned by scopefunc in a plain dict, and entries are removed only by a remove() call made while the same scope key is current. Keying on the asyncio.Task means the key disappears when the task finishes, so any Session created inside a task that does not itself run db.session.remove() (the app-context teardown) stays in the registry forever, together with a strong reference to the completed Task and a checked-out pooled connection.

Concrete paths that touch db.session inside a task without pushing/popping their own app context:

• superset/mcp_service/server.py:510-511: when an app context is already active (Flask app contexts live in a ContextVar, which child tasks inherit from the task that pushed it), _check() runs DB lookups directly with no context push, so no teardown runs in that task.
• superset/mcp_service/auth.py:1044-1045: with an external request context present, nullcontext() is used, so the tool call's DB work is cleaned up by whichever scope pops the outer context — a different task under ASGI.

Before this change these sessions resolved to the thread/greenlet-wide session and were reclaimed by any teardown running on that thread.

Prompt for agents

Keying the SQLAlchemy scoped-session registry on the asyncio Task object (superset/extensions/__init__.py:_session_scopefunc) means registry entries are only ever evicted by a remove() executed while that same task is current. Any code that creates a session inside an asyncio task but relies on an app-context teardown owned by a different task/thread (e.g. superset/mcp_service/server.py:510-511 taking the has_app_context() shortcut, or the nullcontext() branch in superset/mcp_service/auth.py:1044-1045) will leave a Session, a strong reference to the finished Task, and a checked-out pooled connection in the registry forever. Consider either (a) ensuring every asyncio entry point that touches db.session pushes and pops its own app context so teardown runs in the same task, or (b) attaching an eviction hook to the task (e.g. task.add_done_callback that calls db.session.remove() under that scope) so orphaned entries are cleaned up when the task completes.

---

Was this helpful? React with 👍 or 👎 to provide feedback.