You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "verdier (via GitHub)" <gi...@apache.org> on 2026/07/30 19:45:23 UTC
[PR] fix(mcp): report the committed chart when its instance is detached [superset]
verdier opened a new pull request, #42621:
URL: https://github.com/apache/superset/pull/42621
### SUMMARY
Fixes #42567.
Above two concurrent `generate_chart` calls, 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 the `Slice` another request has just committed; every later read of that instance then raises `DetachedInstanceError` and the whole tool call is turned into a `CHART_GENERATION_FAILED` response. 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:
- the explore URL (`slice_id={chart.id}`) and the form-data cache parameters,
- `analyze_chart_capabilities` / `analyze_chart_semantics`, which read `chart.viz_type` through a 3-argument `getattr` — that swallows `AttributeError`, not `DetachedInstanceError`,
- the `api_endpoints` block and the completion log in the result payload,
- and the minimal fallback response itself, which is built by reading `chart.id`, `chart.slice_name`, `chart.viz_type` and `chart.uuid` off 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_id` was already captured there and used for previews; it is now used consistently.
The two analysis helpers only ever needed a viz type, never an ORM instance, so they now take `viz_type: str | None` directly. `update_chart` passes it explicitly; `update_chart_preview` and the existing tests already passed `None` and are unaffected.
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_roles` lazy-loading `User.roles` during dataset lookup, which I also hit at high concurrency on a long-running process. This PR removes the `generate_chart` failure mode; it does not make the session model concurrency-safe. Happy to open a separate issue for that if useful.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — server-side behaviour.
### TESTING INSTRUCTIONS
**Unit tests**
```
pytest tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
```
Two new tests in `TestGenerateChartDetachedInstance` drive `generate_chart(save_chart=True)` with a `Slice` stand-in that is detached right after the commit — one for the normal path, one for the fallback path where the DAO re-fetch raises `SQLAlchemyError`. Both fail on `master` with the production symptom (`success=False`, `DetachedInstanceError`) and pass with this change. The full `tests/unit_tests/mcp_service` suite 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 concurrent `generate_chart` calls with `save_chart=true` over two bearer identities, asserted against rows in the metadata database:
| concurrency | stock 6.1.0 | with this change |
|---|---|---|
| 5 | 5/5 | 5/5 |
| 10 | 2/10, 8 `DetachedInstanceError`, 10 rows written | 10/10 |
| 20 | 5/20, 15 `DetachedInstanceError`, 20 rows written | 20/20 |
| 40 | 5/40, 35 `DetachedInstanceError`, 40 rows written | 40/40, 0 errors, 40 rows, 0 cross-attribution |
Every 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
- [x] Has associated issue: #42567
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): report the committed chart when its instance is detached [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #42621:
URL: https://github.com/apache/superset/pull/42621
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): report the committed chart when its instance is detached [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #42621:
URL: https://github.com/apache/superset/pull/42621#discussion_r3685939265
##########
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:
The next post-await step still reads `chart.datasource_id` through `validate_chart_dataset`, so a concurrent commit and session teardown can detach an expired instance here and report the already-committed chart as failed. Could we snapshot/pass the datasource identity too and let the regression test exercise this validator instead of mocking it?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): report the committed chart when its instance is detached [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42621:
URL: https://github.com/apache/superset/pull/42621#issuecomment-5135793925
<!-- Bito Reply -->
The proposed approach of snapshotting the datasource identity (alongside the other chart attributes) is a sound strategy to avoid `DetachedInstanceError` during concurrent operations. By capturing these values immediately after the chart is committed and before any `await` points, you ensure that subsequent logic—such as `validate_chart_dataset`—operates on stable, local data rather than relying on an instance that might have been detached by a concurrent session teardown. Regarding the regression test, exercising this validator directly is indeed preferable to mocking it, as it provides higher confidence that the fix correctly handles the state transition.
**superset/mcp_service/chart/tool/generate_chart.py**
```
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
# Also snapshot datasource_id here if needed for validation
chart_datasource_id = chart.datasource_id
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): report the committed chart when its instance is detached [superset]
Posted by "verdier (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): report the committed chart when its instance is detached [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42621:
URL: https://github.com/apache/superset/pull/42621#issuecomment-5135601349
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42621?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `10.52632%` with `17 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.88%. Comparing base ([`1df0cca`](https://app.codecov.io/gh/apache/superset/commit/1df0cca2f9ad62da6b7430b4725f0020743e803b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`322f5d8`](https://app.codecov.io/gh/apache/superset/commit/322f5d8eb1faae9e48dd2cf0b2c9be43a252a9c7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/42621?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/chart/tool/generate\_chart.py](https://app.codecov.io/gh/apache/superset/pull/42621?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fgenerate_chart.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZW5lcmF0ZV9jaGFydC5weQ==) | 0.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42621?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/tool/update\_chart.py](https://app.codecov.io/gh/apache/superset/pull/42621?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fupdate_chart.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC91cGRhdGVfY2hhcnQucHk=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42621?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/chart\_utils.py](https://app.codecov.io/gh/apache/superset/pull/42621?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fchart_utils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvY2hhcnRfdXRpbHMucHk=) | 50.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42621?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (1df0cca) and HEAD (322f5d8). Click for more details.
>
> <details><summary>HEAD has 4 uploads less than BASE</summary>
>
>| Flag | BASE (1df0cca) | HEAD (322f5d8) |
>|------|------|------|
>|python|6|2|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42621 +/- ##
==========================================
- Coverage 65.45% 55.88% -9.57%
==========================================
Files 2810 2810
Lines 159335 159340 +5
Branches 36362 36362
==========================================
- Hits 104285 89048 -15237
- Misses 53007 69460 +16453
+ Partials 2043 832 -1211
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42621/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/42621/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.09% <10.52%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/42621/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/42621/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/42621/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.98% <10.52%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/42621/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.04% <10.52%> (-19.23%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/42621/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/42621/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42621?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): report the committed chart when its instance is detached [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42621:
URL: https://github.com/apache/superset/pull/42621#issuecomment-5137887012
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): report the committed chart when its instance is detached [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42621:
URL: https://github.com/apache/superset/pull/42621#issuecomment-5135524989
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): report the committed chart when its instance is detached [superset]
Posted by "verdier (via GitHub)" <gi...@apache.org>.
verdier commented on PR #42621:
URL: https://github.com/apache/superset/pull/42621#issuecomment-5135699925
For the record: I traced the mechanism behind this failure mode and opened #42622 for it. Short version — `mcp_auth_hook` pushes one Flask app context per tool call, but `db.session` is scoped by `greenlet.getcurrent` (flask-sqlalchemy 2.5.1), so concurrent asyncio tool calls all resolve to the same `Session`; the first call to finish pops its context and `teardown_appcontext` removes that shared session under the calls still running.
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org