aminghadersohi commented on code in PR #44581:
URL: https://github.com/apache/superset/pull/44581#discussion_r4113936769
##########
superset/mcp_service/middleware.py:
##########
@@ -218,6 +221,16 @@ def _invoke_error_hook(error: Exception, hook_context:
dict[str, Any]) -> None:
logger.warning("MCP_ERROR_HOOK raised an exception: %s", hook_error)
+async def _invoke_error_hook_off_loop(
+ error: Exception, hook_context: dict[str, Any]
+) -> None:
+ """Keep hook I/O and context-setup failures outside the error boundary."""
+ try:
+ await run_in_metadata_thread(_invoke_error_hook, error, hook_context)
+ except Exception as hook_error: # noqa: BLE001
+ logger.warning("Could not run MCP_ERROR_HOOK: %s", hook_error)
Review Comment:
Valid finding; fixed in 480d27adca0a8039f3bc3e040ed8b2d62c5a4719. Error
hooks run off the transport loop with their own application context and session
cleanup, but without reloading g.user. An absent hook also performs no user
lookup. Added regression coverage with an ORM request user and a failing
metadata lookup, covering both configured and absent hooks and verifying
worker/session isolation. Existing database-writing hook coverage still passes.
MCP unit suite: 5,487 passed, 3 skipped; pre-commit passed on all
branch-changed files, including mypy.
--
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]
