From 776ab3cb148eb6ee4ead44ef57cb6afa979bf429 Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Tue, 7 Apr 2026 14:11:23 -0700 Subject: [PATCH] fix(webdriver): address review feedback on cache warmup WebDriver changes - Fix _auth() to authenticate self._driver in-place instead of creating a second, leaked driver (critical bug: persistent driver was never authenticated) - Replace assert with explicit RuntimeError for driver creation failure - Fix get_dash_url() to strip trailing slash from WEBDRIVER_BASEURL to avoid double-slash URLs (e.g. http://host//superset/dashboard/1/) - Fix BaseScreenshot.get_screenshot() to call driver.destroy() in a try/finally block, preventing Selenium process leaks for one-off screenshots - Fix webdriver_pool._destroy_driver() to directly call close()/quit() on the raw WebDriver since destroy() is now an instance method, not static Co-Authored-By: Claude Sonnet 4.6 --- superset/mcp_service/screenshot/webdriver_pool.py | 6 +++++- superset/tasks/cache.py | 2 +- superset/utils/screenshots.py | 6 +++++- superset/utils/webdriver.py | 12 +++++++----- 4 files changed, 18 insertions(+), 8 deletions(-) diff --git a/superset/mcp_service/screenshot/webdriver_pool.py b/superset/mcp_service/screenshot/webdriver_pool.py index b56bdf109e0..ba1e2c887ed 100644 --- a/superset/mcp_service/screenshot/webdriver_pool.py +++ b/superset/mcp_service/screenshot/webdriver_pool.py @@ -228,7 +228,11 @@ class WebDriverPool: def _destroy_driver(self, pooled_driver: PooledWebDriver) -> None: """Safely destroy a WebDriver instance""" try: - WebDriverSelenium.destroy(pooled_driver.driver) + try: + pooled_driver.driver.close() + except Exception: # pylint: disable=broad-except # noqa: S110 + pass + pooled_driver.driver.quit() self._stats["destroyed"] += 1 logger.debug("Destroyed WebDriver instance") except Exception as e: diff --git a/superset/tasks/cache.py b/superset/tasks/cache.py index 83b1dd67b49..fe2146f74a6 100644 --- a/superset/tasks/cache.py +++ b/superset/tasks/cache.py @@ -48,7 +48,7 @@ def get_dash_url(dashboard: Dashboard) -> str: "{SUPERSET_WEBSERVER_ADDRESS}:" "{SUPERSET_WEBSERVER_PORT}".format(**current_app.config) ) - return f"{baseurl}{dashboard.url}" + return f"{baseurl.rstrip('/')}{dashboard.url}" class Strategy: # pylint: disable=too-few-public-methods diff --git a/superset/utils/screenshots.py b/superset/utils/screenshots.py index 6a5487e45a0..46bd600a29a 100644 --- a/superset/utils/screenshots.py +++ b/superset/utils/screenshots.py @@ -212,7 +212,11 @@ class BaseScreenshot: self, user: User, window_size: WindowSize | None = None ) -> bytes | None: driver = self.driver(window_size, user) - self.screenshot = driver.get_screenshot(self.url, self.element) + try: + self.screenshot = driver.get_screenshot(self.url, self.element) + finally: + if isinstance(driver, WebDriverSelenium): + driver.destroy() return self.screenshot def get_cache_key( diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index 3663829fdf7..6792dbdb1ab 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -419,7 +419,8 @@ class WebDriverSelenium(WebDriverProxy): def driver(self) -> WebDriver: if not self._driver: self._driver = self._create() - assert self._driver # for mypy + if not self._driver: + raise RuntimeError("WebDriver creation failed") self._driver.set_window_size(*self._window) if self._user: self._auth(self._user) @@ -558,10 +559,11 @@ class WebDriverSelenium(WebDriverProxy): logger.debug("Init selenium driver") return driver_class(**kwargs) - def _auth(self, user: User) -> WebDriver: - driver = self._create() - return machine_auth_provider_factory.instance.authenticate_webdriver( - driver, user + def _auth(self, user: User) -> None: + """Authenticate the persistent driver in-place.""" + assert self._driver is not None + machine_auth_provider_factory.instance.authenticate_webdriver( + self._driver, user ) def _destroy(self, tries: int = 2) -> None: