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: [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]
