fix(mcp): report the committed chart when its instance is detached - #42621
Conversation
Above two concurrent `generate_chart` calls the chart is written to the database and the tool still returns an error: the MCP service shares one scoped session across in-flight requests, so a teardown in one request detaches the `Slice` another request just committed, and every later read of `chart.id` raises `DetachedInstanceError`. The guarded re-fetch added in apache#38767/apache#38859 only covers `serialize_chart_object`; the identity reads after it (explore URL, form-data cache, result payload, completion log) and the viz type read by `analyze_chart_capabilities`/`analyze_chart_semantics` are not covered, and the minimal fallback response itself reads the detached instance. Capture the chart's scalar fields once, while the instance is known to be attached, and build everything downstream from those values. The analysis helpers only ever needed the viz type, so they now take it directly instead of an ORM instance. Fixes apache#42567
Code Review Agent Run #c7d861Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42621 +/- ##
==========================================
- Coverage 65.45% 65.44% -0.01%
==========================================
Files 2810 2810
Lines 159335 159353 +18
Branches 36362 36366 +4
==========================================
+ Hits 104285 104286 +1
- Misses 53007 53024 +17
Partials 2043 2043
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
For the record: I traced the mechanism behind this failure mode and opened #42622 for it. Short version — This PR is still worth having on its own — the reads it removes were re-fetching values the function already held, and it stops a committed chart from being reported as a failure — but it is not a concurrency fix, and I did not want #42567 closing to read as one. |
sadpandajoe
left a comment
sadpandajoe
left a comment
There was a problem hiding this comment.
Found one remaining detached-instance access in the committed-chart path.
|
The proposed approach of snapshotting the datasource identity (alongside the other chart attributes) is a sound strategy to avoid superset/mcp_service/chart/tool/generate_chart.py |
Review follow-up: the validator read `chart.datasource_id` off the instance itself, through a three-argument `getattr` — which swallows `AttributeError`, not `DetachedInstanceError`. That read sits after an await in `generate_chart`, so it could report an already-committed chart as failed, exactly like the reads this branch already fixed. Take the id instead of the chart, so every caller reads it while its own instance is attached. `generate_chart` uses the value it snapshots at creation time; the read-path tools pass `chart.datasource_id` at the call site. The regression test now exercises the real validator instead of mocking it.
Code Review Agent Run #991b74Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
SUMMARY
Fixes #42567.
Above two concurrent
generate_chartcalls, the chart is committed to the database and the tool still returns an error. The MCP service shares one scoped session across in-flight requests, so a session teardown in one request detaches theSliceanother request has just committed; every later read of that instance then raisesDetachedInstanceErrorand the whole tool call is turned into aCHART_GENERATION_FAILEDresponse. The row is there, the client is told it failed, and agents retry — which is how you end up with duplicate charts.The guarded re-fetch added in #38767 / #38859 hardened
serialize_chart_object, but the instance is read in several other places after the commit, and none of them are covered:slice_id={chart.id}) and the form-data cache parameters,analyze_chart_capabilities/analyze_chart_semantics, which readchart.viz_type, andvalidate_chart_dataset, which readschart.datasource_id— both through a 3-argumentgetattr, which swallowsAttributeError, notDetachedInstanceError,api_endpointsblock and the completion log in the result payload,chart.id,chart.slice_name,chart.viz_typeandchart.uuidoff the very instance whose re-fetch just failed.This PR captures the chart's scalar fields once, immediately after
command.run()while the instance is known to be attached, and builds everything downstream from those values.chart_idwas already captured there and used for previews; it is now used consistently.The helpers involved never needed an ORM instance, only one field each, so they now take that field:
analyze_chart_capabilities/analyze_chart_semanticstakeviz_type: str | None, andvalidate_chart_datasettakesdatasource_id: int | None. Each caller therefore reads it while its own instance is attached, instead of inheriting a read it cannot see. The read-path tools (get_chart_data,get_chart_preview,get_chart_info,get_chart_sql,auth.validate_chart_dataset_access) passchart.datasource_idat the call site — no behaviour change for them, the same read simply moves into view.Not addressed here: the underlying design, where one scoped session is shared by concurrent MCP tool calls. That is what makes instances detach in the first place, and it can still surface elsewhere — for example
get_user_roleslazy-loadingUser.rolesduring dataset lookup, which I also hit at high concurrency on a long-running process. This PR removes thegenerate_chartfailure mode; it does not make the session model concurrency-safe. I traced that mechanism and opened #42622 for it, with the evidence and three possible directions — happy to implement whichever one maintainers prefer.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — server-side behaviour.
TESTING INSTRUCTIONS
Unit tests
Two new tests in
TestGenerateChartDetachedInstancedrivegenerate_chart(save_chart=True)with aSlicestand-in that is detached right after the commit — one for the normal path, one for the fallback path where the DAO re-fetch raisesSQLAlchemyError. Both fail onmasterwith the production symptom (success=False,DetachedInstanceError) and pass with this change. The fulltests/unit_tests/mcp_servicesuite passes (3129 tests).End-to-end, against the harness from the issue
apache/superset:6.1.0-py311,superset mcp run,stateless_http=True, stock SQLAlchemy pool, N concurrentgenerate_chartcalls withsave_chart=trueover two bearer identities, asserted against rows in the metadata database:DetachedInstanceError, 10 rows writtenDetachedInstanceError, 20 rows writtenDetachedInstanceError, 40 rows writtenEvery row was written in both columns — the failures were purely post-commit reads, which is what makes this reproducible and what the change removes. Same container, same harness, process restarted before each run.
ADDITIONAL INFORMATION