verdier commented on code in PR #42621:
URL: https://github.com/apache/superset/pull/42621#discussion_r3686470937
##########
superset/mcp_service/chart/tool/generate_chart.py:
##########
@@ -424,14 +427,25 @@ 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
Review Comment:
Good catch, fixed in 5e9469b.
You are right that it is the same pattern: `validate_chart_dataset` read
`chart.datasource_id` through a three-argument `getattr`, which swallows
`AttributeError` but not `DetachedInstanceError`.
Rather than snapshot it and keep passing the instance, I gave the validator
the id — same reasoning as
`analyze_chart_capabilities`/`analyze_chart_semantics` in the first commit: it
never needed more than one field. `generate_chart` passes the value it
snapshots at creation; the four read-path tools and `auth.py` pass
`chart.datasource_id` at the call site, so each caller reads it while its own
instance is attached instead of inheriting a read it cannot see. Its own unit
tests get simpler (plain ints instead of chart mocks).
The regression test no longer mocks the validator. Verified both directions:
on `master` it fails at the first unguarded read, and against the previous
revision of this branch it fails exactly where you pointed —
```
File "superset/mcp_service/chart/chart_utils.py", line 93, in
validate_chart_dataset
datasource_id = getattr(chart, "datasource_id", None)
DetachedInstanceError: Instance <Slice> is not bound to a Session
(datasource_id)
```
One honest note on the end-to-end numbers: my 40-call harness did not reach
this site — the previous revision already scored 40/40, because detachment
tends to land later in the call. So this one is covered by the unit test, not
by the harness. I re-ran the harness on the current revision anyway: still
40/40, 0 errors, 40 rows, 0 cross-attribution.
--
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]
