fix(mcp): report the committed chart when its instance is detached

superset/mcp_service/auth.py

@@ -868,7 +868,7 @@ def check_chart_data_access(chart: Any) -> "DatasetValidationResult":
     """
     from superset.mcp_service.chart.chart_utils import validate_chart_dataset
 
-    return validate_chart_dataset(chart, check_access=True)
+    return validate_chart_dataset(chart.datasource_id, check_access=True)
 
 
 def _log_user_resolution_failure(exc: ValueError | PermissionError) -> None:

superset/mcp_service/chart/chart_utils.py

@@ -68,7 +68,7 @@ class DatasetValidationResult:
 
 
 def validate_chart_dataset(
-    chart: Any,
+    datasource_id: int | None,
     check_access: bool = True,
 ) -> DatasetValidationResult:
     """
@@ -77,8 +77,12 @@ def validate_chart_dataset(
     This shared utility should be called by MCP tools after creating or retrieving
     charts to detect issues like missing or deleted datasets early.
 
+    Takes the datasource id rather than the chart so that callers holding an ORM
+    instance read it while that instance is attached; reading it here can raise
+    ``DetachedInstanceError`` when a concurrent request has torn down the session.
+
     Args:
-        chart: A chart-like object with datasource_id, datasource_type attributes
+        datasource_id: The chart's ``datasource_id``, or None if it has none
         check_access: Whether to also check user permissions (default True)
 
     Returns:
@@ -90,7 +94,6 @@ def validate_chart_dataset(
     from superset.mcp_service.auth import has_dataset_access
 
     warnings: list[str] = []
-    datasource_id = getattr(chart, "datasource_id", None)
 
     # Check if chart has a datasource reference
     if datasource_id is None:
@@ -1506,11 +1509,9 @@ def get_table_chart_type_label(viz_type: str | None) -> str | None:
     return TABLE_VIZ_TYPE_LABELS.get(viz_type) if viz_type is not None else None
 
 
-def analyze_chart_capabilities(chart: Any | None, config: Any) -> ChartCapabilities:
+def analyze_chart_capabilities(viz_type: str | None, config: Any) -> ChartCapabilities:
     """Analyze chart capabilities based on type and configuration."""
-    if chart:
-        viz_type = getattr(chart, "viz_type", "unknown")
-    else:
+    if not viz_type:
         viz_type = _resolve_viz_type(config)
 
     # Determine interaction capabilities based on chart type
@@ -1556,11 +1557,9 @@ def analyze_chart_capabilities(chart: Any | None, config: Any) -> ChartCapabilit
     )
 
 
-def analyze_chart_semantics(chart: Any | None, config: Any) -> ChartSemantics:
+def analyze_chart_semantics(viz_type: str | None, config: Any) -> ChartSemantics:
     """Generate semantic understanding of the chart."""
-    if chart:
-        viz_type = getattr(chart, "viz_type", "unknown")
-    else:
+    if not viz_type:
         viz_type = _resolve_viz_type(config)
 
     # Generate primary insight based on chart type

superset/mcp_service/chart/tool/generate_chart.py

@@ -317,6 +317,9 @@ async def generate_chart(  # noqa: C901
 
         chart = None
         chart_id = None
+        chart_slice_name = None
+        chart_viz_type = None
+        chart_uuid = None
         explore_url = None
         form_data_key = None
         response_warnings: list[str] = form_data.pop("_mcp_warnings", [])
@@ -424,14 +427,26 @@ async def generate_chart(  # noqa: C901
                     )
 
                     chart = command.run()
-                    chart_id = chart.id
 
                     # Ensure chart was created successfully before committing
                     if not chart or not chart.id:
                         raise RuntimeError(
                             "Chart creation failed - no chart ID returned"
                         )
 
+                    # Snapshot the scalar fields now, while the instance is
+                    # known to be attached. The chart is already committed at
+                    # this point, and every read further down happens after an
+                    # await: under concurrency another in-flight request can
+                    # tear down the shared session in between, which detaches
+                    # this instance and turns any attribute access into a
+                    # DetachedInstanceError.
+                    chart_id = chart.id
+                    chart_slice_name = chart.slice_name
+                    chart_viz_type = chart.viz_type
+                    chart_uuid = str(chart.uuid) if chart.uuid else None
+                    chart_datasource_id = chart.datasource_id
+
                     # Reload server-generated timestamps (created_on,
                     # changed_on) so the serializer sees real values.
                     from superset import db
@@ -442,20 +457,22 @@ async def generate_chart(  # noqa: C901
                         logger.warning(
                             "Chart %s created but refresh failed; "
                             "continuing with current values",
-                            chart.id,
+                            chart_id,
                             exc_info=True,
                         )
 
                 await ctx.info(
                     "Chart created successfully: chart_id=%s, chart_name=%s"
                     % (
-                        chart.id,
-                        chart.slice_name,
+                        chart_id,
+                        chart_slice_name,
                     )
                 )
 
                 # Post-creation validation: verify the chart's dataset is accessible
-                dataset_check = validate_chart_dataset(chart, check_access=True)
+                dataset_check = validate_chart_dataset(
+                    chart_datasource_id, check_access=True
+                )
                 if not dataset_check.is_valid:
                     # Dataset validation failed - warn but don't fail the operation
                     await ctx.warning(
@@ -464,7 +481,7 @@ async def generate_chart(  # noqa: C901
                     )
                     logger.warning(
                         "Chart %s created but dataset validation failed: %s",
-                        chart.id,
+                        chart_id,
                         dataset_check.error,
                     )
                     if dataset_check.error:
@@ -482,7 +499,7 @@ async def generate_chart(  # noqa: C901
                     # Query failed — delete the broken chart and return an error
                     logger.warning(
                         "Compile check failed for chart %s: %s",
-                        chart.id,
+                        chart_id,
                         compile_result.error,
                     )
                     await ctx.warning(
@@ -537,7 +554,7 @@ async def generate_chart(  # noqa: C901
                 await ctx.error("Chart creation failed: error=%s" % (str(e),))
                 raise
             # Update explore URL to use saved chart
-            explore_url = f"{get_superset_base_url()}/explore/?slice_id={chart.id}"
+            explore_url = f"{get_superset_base_url()}/explore/?slice_id={chart_id}"
 
             # Generate form_data_key for saved charts (needed for chatbot rendering)
             try:
@@ -561,7 +578,7 @@ async def generate_chart(  # noqa: C901
                     cmd_params = CommandParameters(
                         datasource_type=DatasourceType.TABLE,
                         datasource_id=dataset.id,
-                        chart_id=chart.id,
+                        chart_id=chart_id,
                         tab_id=None,
                         form_data=json.dumps(form_data_with_datasource),
                     )
@@ -666,8 +683,8 @@ async def generate_chart(  # noqa: C901
                 response_warnings.extend(compile_result.warnings)
 
         # Generate semantic analysis
-        capabilities = analyze_chart_capabilities(chart, config)
-        semantics = analyze_chart_semantics(chart, config)
+        capabilities = analyze_chart_capabilities(chart_viz_type, config)
+        semantics = analyze_chart_semantics(chart_viz_type, config)
 
         # Create performance metadata
         execution_time = int((time.time() - start_time) * 1000)
@@ -678,11 +695,7 @@ async def generate_chart(  # noqa: C901
         )
 
         # Create accessibility metadata
-        chart_name = (
-            chart.slice_name
-            if chart and hasattr(chart, "slice_name")
-            else generate_chart_name(config)
-        )
+        chart_name = chart_slice_name or generate_chart_name(config)
         accessibility = AccessibilityMetadata(
             color_blind_safe=True,  # Would need actual analysis
             alt_text=f"Chart showing {chart_name}",
@@ -775,7 +788,7 @@ async def generate_chart(  # noqa: C901
         # Build chart info using serialize_chart_object for saved charts
         chart_info = None
         chart_data = None
-        if request.save_chart and chart:
+        if request.save_chart and chart_id:
             from sqlalchemy.orm import joinedload
 
             from superset import db
@@ -793,7 +806,7 @@ async def generate_chart(  # noqa: C901
             try:
                 chart = (
                     ChartDAO.find_by_id(
-                        chart.id,
+                        chart_id,
                         query_options=[
                             joinedload(Slice.editors),
                             joinedload(Slice.tags),
@@ -804,7 +817,7 @@ async def generate_chart(  # noqa: C901
             except SQLAlchemyError:
                 logger.warning(
                     "Re-fetch of chart %s failed; returning minimal response",
-                    chart.id,
+                    chart_id,
                     exc_info=True,
                 )
                 try:
@@ -815,11 +828,11 @@ async def generate_chart(  # noqa: C901
                         exc_info=True,
                     )
                 chart_data = {
-                    "id": chart.id,
-                    "slice_name": chart.slice_name,
-                    "viz_type": chart.viz_type,
+                    "id": chart_id,
+                    "slice_name": chart_slice_name,
+                    "viz_type": chart_viz_type,
                     "url": explore_url,
-                    "uuid": str(chart.uuid) if chart.uuid else None,
+                    "uuid": chart_uuid,
                 }
 
             if chart_data is None:
@@ -849,14 +862,10 @@ async def generate_chart(  # noqa: C901
             "form_data": _sanitize_generate_chart_form_data_for_llm_context(form_data),
             "form_data_key": form_data_key,
             "api_endpoints": {
-                "data": f"{get_superset_base_url()}/api/v1/chart/{chart.id}/data/"
-                if chart
-                else None,
-                "export": f"{get_superset_base_url()}/api/v1/chart/{chart.id}/export/"
-                if chart
-                else None,
+                "data": f"{get_superset_base_url()}/api/v1/chart/{chart_id}/data/",
+                "export": f"{get_superset_base_url()}/api/v1/chart/{chart_id}/export/",
             }
-            if chart
+            if chart_id
             else {},
             "performance": performance.model_dump() if performance else None,
             "accessibility": accessibility.model_dump() if accessibility else None,
@@ -870,7 +879,7 @@ async def generate_chart(  # noqa: C901
         await ctx.info(
             "Chart generation completed successfully: chart_id=%s, execution_time_ms=%s"
             % (
-                chart.id if chart else None,
+                chart_id,
                 int((time.time() - start_time) * 1000),
             )
         )

superset/mcp_service/chart/tool/get_chart_data.py

@@ -416,7 +416,9 @@ async def get_chart_data(  # noqa: C901
 
         # Skip the dataset RBAC pre-check for guests (see guest_scope.is_guest_read).
         if not guest_scope.is_guest_read():
-            validation_result = validate_chart_dataset(chart, check_access=True)
+            validation_result = validate_chart_dataset(
+                chart.datasource_id, check_access=True
+            )
             if not validation_result.is_valid:
                 await ctx.warning(
                     "Chart found but dataset is not accessible: %s"

superset/mcp_service/chart/tool/get_chart_info.py

@@ -116,7 +116,7 @@ async def _validate_chart_dataset_access(
     chart = ChartDAO.find_by_id(result.id)
     if not chart:
         return None
-    validation_result = validate_chart_dataset(chart, check_access=True)
+    validation_result = validate_chart_dataset(chart.datasource_id, check_access=True)
     if not validation_result.is_valid:
         await ctx.warning(
             "Chart found but dataset is not accessible: %s" % (validation_result.error,)

superset/mcp_service/chart/tool/get_chart_preview.py

@@ -1262,7 +1262,9 @@ def __init__(self, form_data: Dict[str, Any]):
         from superset.mcp_service import guest_scope
 
         if getattr(chart, "id", None) is not None and not guest_scope.is_guest_read():
-            validation_result = validate_chart_dataset(chart, check_access=True)
+            validation_result = validate_chart_dataset(
+                chart.datasource_id, check_access=True
+            )
             if not validation_result.is_valid:
                 await ctx.warning(
                     "Chart found but dataset is not accessible: %s"

superset/mcp_service/chart/tool/get_chart_sql.py

@@ -421,7 +421,7 @@ async def _handle_chart_sql_request(
     )
 
     # Validate the chart's dataset is accessible
-    validation_result = validate_chart_dataset(chart, check_access=True)
+    validation_result = validate_chart_dataset(chart.datasource_id, check_access=True)
     if not validation_result.is_valid:
         await ctx.warning(
             "Chart found but dataset is not accessible: %s" % (validation_result.error,)

superset/mcp_service/chart/tool/update_chart.py

@@ -579,8 +579,9 @@ async def update_chart(  # noqa: C901
                 )
 
         chart_for_analysis = updated_chart if saved else chart
-        capabilities = analyze_chart_capabilities(chart_for_analysis, parsed_config)
-        semantics = analyze_chart_semantics(chart_for_analysis, parsed_config)
+        viz_type_for_analysis = getattr(chart_for_analysis, "viz_type", None)
+        capabilities = analyze_chart_capabilities(viz_type_for_analysis, parsed_config)
+        semantics = analyze_chart_semantics(viz_type_for_analysis, parsed_config)
 
         execution_time = int((time.time() - start_time) * 1000)
         performance = PerformanceMetadata(

tests/unit_tests/mcp_service/chart/test_chart_utils.py

@@ -1945,9 +1945,8 @@ class TestValidateChartDataset:
     def test_validate_chart_dataset_no_datasource_id(
         self, mock_find: MagicMock, mock_access: MagicMock
     ) -> None:
-        """Chart with no datasource_id returns invalid result."""
-        chart = MagicMock(spec=[])  # no datasource_id attribute
-        result = validate_chart_dataset(chart)
+        """A chart with no datasource_id returns invalid result."""
+        result = validate_chart_dataset(None)
         assert not result.is_valid
         assert result.dataset_id is None
         assert "no dataset reference" in (result.error or "").lower()
@@ -1959,9 +1958,7 @@ def test_validate_chart_dataset_deleted_dataset(
         self, mock_find: MagicMock, mock_access: MagicMock
     ) -> None:
         """Chart whose dataset was deleted returns invalid result."""
-        chart = MagicMock()
-        chart.datasource_id = 42
-        result = validate_chart_dataset(chart)
+        result = validate_chart_dataset(42)
         assert not result.is_valid
         assert result.dataset_id == 42
         assert "deleted" in (result.error or "").lower()
@@ -1976,9 +1973,7 @@ def test_validate_chart_dataset_valid(
         dataset.table_name = "my_table"
         dataset.sql = None
         mock_find.return_value = dataset
-        chart = MagicMock()
-        chart.datasource_id = 7
-        result = validate_chart_dataset(chart)
+        result = validate_chart_dataset(7)
         assert result.is_valid
         assert result.dataset_id == 7
         assert result.dataset_name == "my_table"
@@ -1994,9 +1989,7 @@ def test_validate_chart_dataset_virtual_warns(
         dataset.table_name = "virt_ds"
         dataset.sql = "SELECT 1"
         mock_find.return_value = dataset
-        chart = MagicMock()
-        chart.datasource_id = 10
-        result = validate_chart_dataset(chart)
+        result = validate_chart_dataset(10)
         assert result.is_valid
         assert len(result.warnings) == 1
         assert "virtual" in result.warnings[0].lower()
@@ -2010,9 +2003,7 @@ def test_validate_chart_dataset_sqlalchemy_error(
         from sqlalchemy.exc import SQLAlchemyError
 
         mock_find.side_effect = SQLAlchemyError("connection lost")
-        chart = MagicMock()
-        chart.datasource_id = 99
-        result = validate_chart_dataset(chart)
+        result = validate_chart_dataset(99)
         assert not result.is_valid
         assert result.dataset_id == 99
         assert "error" in (result.error or "").lower()