codeant-ai-for-open-source[bot] commented on code in PR #39917:
URL: https://github.com/apache/superset/pull/39917#discussion_r3202961384
##########
superset/mcp_service/auth.py:
##########
@@ -516,6 +516,45 @@ def _cleanup_session_on_error() -> None:
logger.warning("Error cleaning up session after exception: %s", e)
+def _remove_session_safe() -> None:
+ """Remove the scoped SQLAlchemy session, tolerating SSL/connection errors.
+
+ Thread-pool workers reuse threads across requests. Before each tool call
+ the session is removed to prevent a prior request's thread-local session
+ from leaking into the next one. If the underlying DBAPI connection died
+ between requests (e.g. RDS SSL idle-timeout or max-connection-age), the
+ rollback implicit in ``session.close()`` raises ``OperationalError``.
+
+ When that happens:
+ 1. Invalidate the dead connection so the pool discards it (rather than
+ returning a broken connection to the next caller).
+ 2. Retry ``remove()`` to deregister the session from the scoped registry.
+
+ The tool call still proceeds because a fresh connection will be obtained
+ on the next DB access.
+ """
+ from sqlalchemy.exc import OperationalError
+
+ from superset.extensions import db
+
+ try:
+ db.session.remove()
+ except OperationalError as exc:
Review Comment:
**Suggestion:** Catching only `OperationalError` is too narrow for
disconnect scenarios across drivers, because SQLAlchemy can surface dropped
connections as other `DBAPIError` subclasses (for example `InterfaceError`). In
those cases this code path is skipped and pre-call cleanup still crashes;
broaden handling to disconnect-related DBAPI errors and gate on
connection-invalidated semantics. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Sync MCP tools fail on non-OperationalError disconnects.
- ⚠️ Connection drop handling inconsistent across database drivers.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. `sync_wrapper()` in `superset/mcp_service/auth.py:670-714` wraps sync MCP
tools
registered via `@tool` (see `superset/core/mcp/core_mcp_injection.py:26-43`)
and calls
`_remove_session_safe()` at line 680 before any user lookup or tool logic,
so any
exception from `_remove_session_safe()` aborts the tool call during pre-call
teardown.
2. `_remove_session_safe()` in `superset/mcp_service/auth.py:519-555` imports
`sqlalchemy.exc.OperationalError` (line 536), calls `db.session.remove()`
(line 540), and
only catches `OperationalError` in `except OperationalError as exc:` at line
542; any
other `sqlalchemy.exc.DBAPIError` subclass from `db.session.remove()` is not
handled here
and will propagate out of `sync_wrapper()`.
3. Superset elsewhere relies on `sqlalchemy.exc.DBAPIError` with
`.connection_invalidated`
to detect disconnects generically, e.g. `superset/utils/core.py:771-775`
catches
`exc.DBAPIError as err` and checks `err.connection_invalidated` to decide
whether to treat
the exception as a disconnect and transparently revalidate the connection,
showing that
connection drops are not assumed to always be `OperationalError`.
4. To reproduce using the same call path as the existing SSL-idle test
(`tests/unit_tests/mcp_service/test_auth_user_resolution.py:33-73`), patch
`superset.extensions.db.session.remove` so that the first call raises a
`sqlalchemy.exc.DBAPIError` instance with `connection_invalidated=True`
(similar to the
pattern in `utils/core.py:771-775`) instead of `OperationalError`, then
invoke `wrapped =
mcp_auth_hook(dummy_tool)` and call `wrapped()`: `_remove_session_safe()` at
`auth.py:540-542` will not catch this DBAPIError, causing the pre-call
session cleanup to
raise and abort the sync tool, even though this is a classic disconnect
scenario where a
new connection could be established on the next DB access.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fauth.py%0A%2A%2ALine%3A%2A%2A%20542%3A542%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Catching%20only%20%60OperationalError%60%20is%20too%20narrow%20for%20disconnect%20scenarios%20across%20drivers%2C%20because%20SQLAlchemy%20can%20surface%20dropped%20connections%20as%20other%20%60DBAPIError%60%20subclasses%20%28for%20example%20%60InterfaceError%60%29.%20In%20those%20cases%20this%20code%20path%20is%20skipped%20and%20pre-call%20cleanup%20still%20crashes%3B%20broaden%20handling%20to%20disconnect-related%20DBAPI%20errors%20and%20gate%20on%20connection-invalidated%20semantics.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%2
0implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fauth.py%0A%2A%2ALine%3A%2A%2A%20542%3A542%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Catching%20only%20%60OperationalError%60%20is%20too%20narrow%20for%20disconnect%20scenarios%20across%20drivers%2C%20because%20SQLAlchemy%20can%20surface%20dropped%20connections%20as%20other%20%60DBAPIError%60%20subclasses%20%28for%20example%20%60InterfaceError%60%29.%20In%20those%20cases%20this%20code%20path%20is%20skipped%20and%20pre-call%20cleanup%20still%20crashes%3B%20broaden%20handling
%20to%20disconnect-related%20DBAPI%20errors%20and%20gate%20on%20connection-invalidated%20semantics.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 542:542
**Comment:**
*Logic Error: Catching only `OperationalError` is too narrow for
disconnect scenarios across drivers, because SQLAlchemy can surface dropped
connections as other `DBAPIError` subclasses (for example `InterfaceError`). In
those cases this code path is skipped and pre-call cleanup still crashes;
broaden handling to disconnect-related DBAPI errors and gate on
connection-invalidated semantics.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39917&comment_hash=c20749ebfd39120c11fd1570aa512a8dd888987e943924d804be7f8ab99ae952&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39917&comment_hash=c20749ebfd39120c11fd1570aa512a8dd888987e943924d804be7f8ab99ae952&reaction=dislike'>👎</a>
--
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]
