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 <noreply@anthropic.com>
This commit is contained in:
Superset Dev
2026-04-07 14:11:23 -07:00
parent 2ac03e438c
commit 776ab3cb14
4 changed files with 18 additions and 8 deletions

View File

@@ -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:

View File

@@ -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

View File

@@ -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(

View File

@@ -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: