You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/05/06 22:56:00 UTC
[PR] fix(mcp): handle SSL connection drop during pre-call session teardown [superset]
aminghadersohi opened a new pull request, #39917:
URL: https://github.com/apache/superset/pull/39917
### SUMMARY
MCP tools running in thread-pool workers (sync tools) call `db.session.remove()` before each invocation to clear any stale thread-local session. If the underlying DBAPI connection died between requests — e.g. RDS dropped it due to SSL idle-timeout or max-connection-age — the implicit `rollback()` inside `session.close()` raises `OperationalError`. This caused the tool call to fail with a stack trace, even when the operation itself had completed successfully.
**Root cause:** `sync_wrapper` in `auth.py` called `db.session.remove()` bare, with no handling for connection-level errors.
**Fix:** Extract `_remove_session_safe()` which:
1. Catches `OperationalError` from `session.remove()`
2. Calls `session.invalidate()` to mark the dead connection for pool discard (prevents it being checked out again by another thread)
3. Retries `session.remove()` to deregister the scoped session
The tool call proceeds normally; a fresh connection is obtained on the next DB access.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only change
### TESTING INSTRUCTIONS
1. Unit test: `pytest tests/unit_tests/mcp_service/test_auth_user_resolution.py::test_sync_wrapper_handles_ssl_error_on_pre_call_remove`
2. To reproduce manually: configure a short RDS SSL idle timeout, make an MCP tool call after an idle period — observe no error in logs.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): handle SSL connection drop during pre-call session teardown [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39917:
URL: https://github.com/apache/superset/pull/39917#issuecomment-4395876486
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): handle SSL connection drop during pre-call session teardown [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #39917:
URL: https://github.com/apache/superset/pull/39917
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): handle SSL connection drop during pre-call session teardown [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #39917:
URL: https://github.com/apache/superset/pull/39917#issuecomment-4398837009
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 9b57b45fefa60bdd78af6ac18526c5fedf944f23 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69fcb8b4f1efa20008b04626 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-39917--superset-docs-preview.netlify.app](https://deploy-preview-39917--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEADAv-w5yUKk-kHPcFelwHbFhIro73mbw7yQixy5wgLQARbHtDXH_e4Pyuo8kyJhDlR3YVWDK14ubtWHnORZfct2joQTmjiE6Eat9YTa2NHC9wM) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-39917--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): handle SSL connection drop during pre-call session teardown [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39917:
URL: https://github.com/apache/superset/pull/39917#issuecomment-4392804522
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): handle SSL connection drop during pre-call session teardown [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): handle SSL connection drop during pre-call session teardown [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39917:
URL: https://github.com/apache/superset/pull/39917#issuecomment-4392861777
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39917?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `7.69231%` with `12 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 63.11%. Comparing base ([`5b5dd01`](https://app.codecov.io/gh/apache/superset/commit/5b5dd010285890b4b5b45e707a9c3b0da413f75e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`166d047`](https://app.codecov.io/gh/apache/superset/commit/166d0477c085690b15afe955041083b5aae7263f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39917?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/auth.py](https://app.codecov.io/gh/apache/superset/pull/39917?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fauth.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvYXV0aC5weQ==) | 7.69% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39917?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39917 +/- ##
==========================================
- Coverage 63.88% 63.11% -0.78%
==========================================
Files 2583 2583
Lines 136604 136615 +11
Branches 31502 31502
==========================================
- Hits 87276 86229 -1047
- Misses 47812 48863 +1051
- Partials 1516 1523 +7
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39917/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/39917/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.38% <7.69%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/39917/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/39917/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/39917/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.07% <7.69%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/39917/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.89% <7.69%> (-1.69%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/39917/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.76% <7.69%> (-0.01%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/39917/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39917?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): handle SSL connection drop during pre-call session teardown [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #39917:
URL: https://github.com/apache/superset/pull/39917#issuecomment-4399110226
Addressed catch: widened from to so drivers that surface disconnect as are also covered. — agor claude on Amin's behalf
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): handle SSL connection drop during pre-call session teardown [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #39917:
URL: https://github.com/apache/superset/pull/39917#discussion_r3203145757
##########
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:
Addressed — widened catch from `OperationalError` to `DBAPIError` in `_remove_session_safe()` so drivers that surface disconnects as `InterfaceError` (or other `DBAPIError` subclasses) are also handled. — agor claude on Amin's behalf
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): handle SSL connection drop during pre-call session teardown [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39917:
URL: https://github.com/apache/superset/pull/39917#issuecomment-4403267461
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org