codeant-ai-for-open-source[bot] commented on code in PR #38934:
URL: https://github.com/apache/superset/pull/38934#discussion_r3006757071
##########
superset/mcp_service/chart/tool/generate_chart.py:
##########
@@ -744,7 +744,7 @@ async def generate_chart( # noqa: C901
chart.id,
exc_info=True,
)
- db.session.rollback()
+ db.session.rollback() # pylint:
disable=consider-using-transaction
Review Comment:
**Suggestion:** The unguarded `db.session.rollback()` in the
`SQLAlchemyError` handler for the chart re-fetch can itself raise another
`SQLAlchemyError` (e.g., on a poisoned session), which will bypass the intended
minimal `chart_data` fallback and instead escalate to the outer error handler,
defeating the purpose of this graceful degradation path and potentially
reintroducing cascading errors. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ generate_chart tool fails instead of returning minimal chart data.
- ⚠️ MCP chart creation less resilient to database connection drops.
```
</details>
```suggestion
try:
db.session.rollback() # pylint:
disable=consider-using-transaction
except SQLAlchemyError:
logger.warning(
"Database rollback failed during chart re-fetch
error handling",
exc_info=True,
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP `generate_chart` tool (entrypoint at
`superset/mcp_service/chart/tool/generate_chart.py:137`) from an MCP client
or via the
documented workflow in `superset/mcp_service/app.py:14-18` using
`generate_chart(dataset_id, config, save_chart=True)` so that a chart is
saved in the
database.
2. Let the request proceed past chart creation to the response-building
phase where the
code re-fetches the chart with eager-loaded relationships via
`ChartDAO.find_by_id(...)`
inside the `try` block at
`superset/mcp_service/chart/tool/generate_chart.py:31-41` (lines
731-741 in the full file).
3. Trigger a database/session failure so that this re-fetch raises
`SQLAlchemyError` (for
example, a poisoned SQLAlchemy session after a PostgreSQL SSL drop as
described in the PR,
or in a test by monkeypatching `ChartDAO.find_by_id` to raise
`SQLAlchemyError`); this
causes execution to enter the local `except SQLAlchemyError:` block at
`superset/mcp_service/chart/tool/generate_chart.py:42-55`.
4. In that local handler, the unguarded `db.session.rollback()` at
`superset/mcp_service/chart/tool/generate_chart.py:48` (`747
db.session.rollback() #
pylint: disable=consider-using-transaction`) executes on the
already-poisoned session and
can itself raise `SQLAlchemyError`, which bypasses the intended `chart_data
= {...}`
minimal-fallback assignment at lines 49-55 and instead bubbles up to the
outer `except
(CommandException, SQLAlchemyError, KeyError, ValueError)` at the end of
`generate_chart`,
causing the whole tool call to fail with an error response instead of
returning the
degraded but successful chart payload.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/generate_chart.py
**Line:** 747:747
**Comment:**
*Possible Bug: The unguarded `db.session.rollback()` in the
`SQLAlchemyError` handler for the chart re-fetch can itself raise another
`SQLAlchemyError` (e.g., on a poisoned session), which will bypass the intended
minimal `chart_data` fallback and instead escalate to the outer error handler,
defeating the purpose of this graceful degradation path and potentially
reintroducing cascading errors.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38934&comment_hash=b5c28dbeeb84ac8b89374f847030615c248b33b0de95c8d797f33cc92b61007f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38934&comment_hash=b5c28dbeeb84ac8b89374f847030615c248b33b0de95c8d797f33cc92b61007f&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]
