verdier commented on PR #43007:
URL: https://github.com/apache/superset/pull/43007#issuecomment-5250013872
@rusackas on the comparison with #42629 — it predates this PR by ten days
and reaches the same diagnosis, so rather than argue for mine I ran it. I
backported its approach onto the same 6.1 harness I used above and measured
both under identical conditions.
**Measured: the two are indistinguishable in behaviour.**
| | isolation at 10 concurrent | sessions handed out | at 16 concurrent
(pool 15) |
|---|---|---|---|
| master | 2/10, 8 `DetachedInstanceError` | 1 for 10 tasks | — |
| #42629 (ContextVar per call) | 10/10 | 10 for 10 tasks | 15/16, one
`QueuePool` timeout |
| #43007 (task-scoped scopefunc) | 10/10 | 10 for 10 tasks | 15/16, one
`QueuePool` timeout |
Both fix the bug, and **both hit the same cliff at `pool_size +
max_overflow`** — I measured the wall clock at concurrency 16 twice per
approach and got 34s/94s for one and 35s/66s for the other, i.e. variance, not
a difference. So the pool consequence I documented above is a property of
isolating sessions at all, not of how you isolate them. Whichever lands, the
concurrency guard is needed.
One caveat on my numbers: backporting #42629 to 6.1 needed an import fix,
because 6.1 imports `has_app_context` inside `mcp_auth_hook` while master has
it at module level. That is an artefact of my backport, not a defect in that PR.
**Where they actually differ**, since it is not the behaviour:
*#42629's advantages*
- The scope is the MCP tool call, so nothing outside the MCP service changes
even in principle — if Superset ever grows other event-loop entry points, they
keep the library default rather than silently inheriting a new one.
- The tool call is arguably the right unit: sub-tasks spawned inside one
tool deliberately share that call's session, where task scoping would hand them
separate ones.
- It calls `_remove_session_safe()` on the `has_request_context()` path,
where no app-context teardown fires. That is exactly the registry-entry leak
@aminghadersohi found in mine, and #42629 does not have it.
*#42629's costs*
- Activation depends on the startup path: it is installed from
`create_mcp_app`, `init_fastmcp_server` and `run_server`, and the
`create_mcp_app` one sits in a `try/except ImportError` that only logs at debug
level. A serving path that reaches none of them keeps the buggy behaviour with
no signal. The comment there acknowledges the trade; it is a real one.
- It reassigns `registry.scopefunc` on an already-constructed
`scoped_session`. Entries created under the previous key stay in the registry,
and configuration becomes a side effect of a call rather than a property of the
object.
- The registry key is `("mcp_tool_call", id(token))`. `id()` is a reusable
address: an entry that is not removed can be inherited by a later call whose
token lands on the same address. Low probability, unpleasant class of bug.
*This PR's advantages*
- Configured where `db` is constructed, so there is no install step to miss
and no post-hoc mutation of global state.
- The key is the `Task` object itself, which the registry keeps alive while
its entry exists, so no identity reuse is possible.
*This PR's costs*
- Global in principle. @aminghadersohi checked the blast radius empirically
(no event-loop entry point outside MCP today) and I agree it is theoretical
right now, but it is still a broader surface than #42629 takes.
- The `nullcontext()` leak above, which #42629 handles and I do not.
- The greenlet fallback pins 2.5.1 semantics; on flask-sqlalchemy 3.x the
default becomes app-context scoping and this would hold the web tier at
greenlet.
If it helps you decide: the two are equivalent on correctness, and the
choice is between a narrower surface that depends on being installed (#42629)
and a wider surface that cannot be forgotten (this one). I have no stake in
which — I would rather see the better one land than mine. If you take #42629, I
will port the semaphore and these measurements over there and close this. If
you take this one, I will borrow its `_remove_session_safe()` on the
request-context path and credit it.
Either way I will add the semaphore next, since it is needed on both.
--
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]
