eschutho opened a new pull request, #44351: URL: https://github.com/apache/superset/pull/44351
### SUMMARY **Sentry issue**: [SUPERSET-PYTHON-1739](https://preset-inc.sentry.io/issues/7735538129/) — 1 event, brand new (first seen 2026-09-16T12:04:51Z) **Shortcut**: https://app.shortcut.com/preset/story/121078 **Root cause**: In `get_chart_sql`, the `event_logger.log_context` context manager calls `DBEventLogger.log()` on exit, which does `db.session.bulk_save_objects(logs); db.session.commit()`. With SQLAlchemy's default `expire_on_commit=True`, this commit expires all attributes on the `chart` (Slice) ORM instance that was just looked up. The function then hits several `await ctx.info(...)` / `await ctx.warning(...)` calls — real async yield points — after which the MCP per-call SQLAlchemy session can be torn down. By the time `_resolve_effective_form_data(chart, ...)` accesses `chart.params`, the attribute is expired and the lazy reload fails with `DetachedInstanceError`. **Fix**: Add `db.session.refresh(chart)` immediately after the not-found check in `_handle_chart_sql_request`, eagerly loading all column values into the instance's `__dict__` before any downstream operation can expire or detach the session. This is the exact same pattern already applied in: - `get_chart_preview.py` (line ~1187) - `generate_chart.py` (line ~502) - `generate_dashboard.py`, `manage_dashboard_certification.py`, `manage_dashboard_owners.py`, `manage_dashboard_roles.py`, `update_dashboard.py` As defense-in-depth, also add an `except SQLAlchemyError` handler to the outer `get_chart_sql()` try/except, mirroring `get_chart_preview.py`'s structure — this ensures any residual session-related errors return a structured `ChartError` instead of propagating as unhandled exceptions. ### Tradeoffs `db.session.refresh(chart)` issues one extra synchronous DB round-trip per `get_chart_sql` call. This is the same cost already paid by `get_chart_preview` and `generate_chart` — negligible in the context of a tool call that subsequently builds and renders a full SQL query. No failure-mode semantics change; this purely makes a previously-flaky path reliable. ### Follow-ups The following MCP tool files have the same theoretical gap (call `find_chart_by_identifier` / `_find_chart_by_identifier` with no subsequent `db.session.refresh`) but are **not fixed here** (out of scope — not currently reported failing in Sentry): - `superset/mcp_service/chart/tool/delete_chart.py` - `superset/mcp_service/chart/tool/get_chart_data.py` - `superset/mcp_service/chart/tool/update_chart.py` ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A — backend-only fix, no UI changes. ### TESTING INSTRUCTIONS ``` $ python3 -m pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py::TestDetachedInstanceError -v tests/.../test_get_chart_sql.py::TestDetachedInstanceError::test_session_refresh_called_after_chart_load PASSED tests/.../test_get_chart_sql.py::TestDetachedInstanceError::test_detached_instance_error_returns_chart_error PASSED 2 passed $ ruff check superset/mcp_service/chart/tool/get_chart_sql.py tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py All checks passed! $ ruff format --check superset/mcp_service/chart/tool/get_chart_sql.py tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py 2 files already formatted ``` ### ADDITIONAL INFORMATION - [x] Has associated issue: Fixes SUPERSET-PYTHON-1739 - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration - [ ] 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]
