From 2ffecfb4054a49d290a927a77d73369cc572f042 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 30 Oct 2026 03:04:05 +0000 Subject: [PATCH 1/6] feat(cache): warm favorited dashboards' cache on user login Adds an opt-in CACHE_WARMUP_ON_LOGIN setting. After a successful login an asynchronous Celery task runs as the user and computes only the missing or expired chart query results for their most recently favorited dashboards. Co-Authored-By: Claude Sonnet 5.5 --- docs/admin_docs/configuration/cache.mdx | 24 +++ superset/commands/chart/warm_up_cache.py | 25 +++- superset/config.py | 14 ++ superset/security/manager.py | 2 + superset/tasks/cache.py | 101 ++++++++++++- .../commands/chart/warm_up_cache_test.py | 48 ++++-- tests/unit_tests/security/audit_log_test.py | 19 +++ tests/unit_tests/tasks/test_cache.py | 138 +++++++++++++++++- 8 files changed, 350 insertions(+), 21 deletions(-) diff --git a/docs/admin_docs/configuration/cache.mdx b/docs/admin_docs/configuration/cache.mdx index 397ca4a408d6..028b8a731ecb 100644 --- a/docs/admin_docs/configuration/cache.mdx +++ b/docs/admin_docs/configuration/cache.mdx @@ -282,6 +282,30 @@ Requirements and limitations: - Cascading/dependent native filters and search-term variants of filter option queries are not warmed by this strategy. +### Warming Up Favorited Dashboards on Login + +Superset can warm the cache for a user's favorited dashboards when they log in. It is off by +default: + +```python +CACHE_WARMUP_ON_LOGIN = True +CACHE_WARMUP_ON_LOGIN_MAX_DASHBOARDS = 5 # most recently favorited dashboards per login +CACHE_WARMUP_ON_LOGIN_COOLDOWN = 3600 # seconds between warm-ups for the same user; 0 disables +``` + +After a successful login, a Celery task runs as that user and checks every chart on their +favorited dashboards. Only charts whose query results are missing or expired are computed; +charts that are already cached are skipped. Login itself is never delayed by this work. + +Requirements: + +- A Celery worker, and a real `DATA_CACHE_CONFIG` backend (with `NullCache` nothing can be + found in the cache, so every login would recompute). +- The cooldown is tracked in `CACHE_CONFIG`. +- The task runs with the logging-in user's permissions, so the cache entries match what that + user's dashboards request. Dashboard default filters are not applied, so charts whose + dashboard load uses different filters may still miss the cache. + ## Caching Thumbnails This is an optional feature that can be turned on by activating its [feature flag](/admin-docs/configuration/configuring-superset#feature-flags) on config: diff --git a/superset/commands/chart/warm_up_cache.py b/superset/commands/chart/warm_up_cache.py index 623869871551..953e1c68408b 100644 --- a/superset/commands/chart/warm_up_cache.py +++ b/superset/commands/chart/warm_up_cache.py @@ -26,8 +26,9 @@ WarmUpCacheChartNotFoundError, ) from superset.common.db_query_status import QueryStatus +from superset.constants import CACHE_DISABLED_TIMEOUT from superset.exceptions import SupersetSecurityException -from superset.extensions import db, security_manager +from superset.extensions import cache_manager, db, security_manager from superset.models.slice import Slice from superset.semantic_layers.access import ( is_semantic_layers_enabled, @@ -44,10 +45,14 @@ def __init__( chart_or_id: Union[int, Slice], dashboard_id: Optional[int], extra_filters: Optional[str], + only_if_stale: bool = False, ): self._chart_or_id = chart_or_id self._dashboard_id = dashboard_id self._extra_filters = extra_filters + # When set, charts whose query results are all cached are left untouched + # and missing results are computed without bypassing the cache. + self._only_if_stale = only_if_stale def _get_dashboard_filters(self, chart_id: int) -> list[dict[str, Any]]: """Retrieve dashboard filters from extra_filters or dashboard metadata.""" @@ -59,6 +64,18 @@ def _get_dashboard_filters(self, chart_id: int) -> list[dict[str, Any]]: return get_dashboard_extra_filters(chart_id, self._dashboard_id) + @staticmethod + def _is_fully_cached(query_context: Any) -> bool: + """Return whether every query of the context has an unexpired cache entry.""" + if query_context.get_cache_timeout() == CACHE_DISABLED_TIMEOUT: + # Results are never stored, so there is nothing to warm. + return True + for query in query_context.queries: + cache_key = query_context.query_cache_key(query) + if not cache_key or cache_manager.data_cache.get(cache_key) is None: + return False + return True + def _warm_up_non_legacy_cache(self, chart: Slice) -> tuple[Any, Any]: """Warm up cache for non-legacy visualizations.""" query_context = chart.get_query_context() @@ -77,7 +94,11 @@ def _warm_up_non_legacy_cache(self, chart: Slice) -> tuple[Any, Any]: + query.filter ) - query_context.force = True + if self._only_if_stale: + if self._is_fully_cached(query_context): + return None, QueryStatus.SUCCESS + else: + query_context.force = True command = ChartDataCommand(query_context) command.validate() payload = command.run() diff --git a/superset/config.py b/superset/config.py index efba67f3f816..746671c7b924 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1476,6 +1476,20 @@ def sync_theme_logo_href( # - Cascade/dependent filters and search-term variants are not warmed in this # version; only base option queries are supported. +# Warm the cache for a user's favorited dashboards right after they log in. +# An asynchronous Celery task runs as the logging-in user and, for each chart on +# their most recently favorited dashboards, computes only the query results that +# are missing or expired in DATA_CACHE_CONFIG; charts that are already cached are +# skipped. Login is never blocked. Requires a Celery worker and a real +# DATA_CACHE_CONFIG backend (with NullCache every login would recompute). +CACHE_WARMUP_ON_LOGIN = False +# Maximum number of favorited dashboards (most recently favorited first) warmed +# per login. +CACHE_WARMUP_ON_LOGIN_MAX_DASHBOARDS = 5 +# Minimum seconds between login warm-ups for the same user, uses CACHE_CONFIG. +# Set to 0 to disable the cooldown. +CACHE_WARMUP_ON_LOGIN_COOLDOWN = int(timedelta(hours=1).total_seconds()) + # Time before selenium times out after trying to locate an element on the page and wait # for that element to load for a screenshot. SCREENSHOT_LOCATE_WAIT = int(timedelta(seconds=10).total_seconds()) diff --git a/superset/security/manager.py b/superset/security/manager.py index 952aa4a0000b..2a633c8847eb 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -2418,6 +2418,7 @@ def _same_user(left: Any, right: Any) -> bool: def on_user_login(self, user: Any) -> None: # pylint: disable=import-outside-toplevel from superset.security.session_invalidation import stamp_login_time + from superset.tasks.cache import schedule_login_cache_warmup # Record the authentication time so outstanding sessions can be # invalidated when the account is later disabled. @@ -2426,6 +2427,7 @@ def on_user_login(self, user: Any) -> None: "UserLoggedIn", {"username": user.username, "user_id": user.id}, ) + schedule_login_cache_warmup(user) def on_user_login_failed(self, user: Any) -> None: _log_audit_event( diff --git a/superset/tasks/cache.py b/superset/tasks/cache.py index c2a6d4e6d3b9..2763742f72a9 100644 --- a/superset/tasks/cache.py +++ b/superset/tasks/cache.py @@ -28,8 +28,9 @@ from superset import db, security_manager from superset.common.query_context import QueryContext from superset.daos.dashboard import DashboardDAO -from superset.extensions import celery_app -from superset.models.core import Log +from superset.exceptions import SupersetSecurityException +from superset.extensions import cache_manager, celery_app +from superset.models.core import FavStar, FavStarClassName, Log from superset.models.dashboard import Dashboard from superset.tags.models import Tag, TaggedObject from superset.tasks.native_filter_cache import ( @@ -426,3 +427,99 @@ def cache_warmup( results, ) return results + + +def _get_favorite_dashboards(user_id: int, limit: int) -> list[Dashboard]: + """Return the user's most recently favorited dashboards, newest first.""" + dashboard_ids: list[int] = [ + row.obj_id + for row in db.session.query(FavStar.obj_id) + .filter( + FavStar.user_id == user_id, + FavStar.class_name == FavStarClassName.DASHBOARD, + ) + .order_by(FavStar.dttm.desc()) + .all() + ] + dashboards: list[Dashboard] = [] + if not dashboard_ids: + return dashboards + by_id: dict[int, Dashboard] = { + dashboard.id: dashboard + for dashboard in db.session.query(Dashboard) + .options(selectinload(Dashboard.slices)) + .filter(Dashboard.id.in_(dashboard_ids)) + .all() + } + for dashboard_id in dashboard_ids: + if (dashboard := by_id.get(dashboard_id)) is None: + continue + try: + security_manager.raise_for_access(dashboard=dashboard) + except SupersetSecurityException: + continue + dashboards.append(dashboard) + if len(dashboards) >= limit: + break + return dashboards + + +@celery_app.task(name="cache-warmup-user-favorites") +def cache_warmup_user_favorites(user_id: int) -> dict[str, list[int]]: + """ + Warm the cache for charts on a user's favorited dashboards. + + Runs as the given user, since cache keys depend on the requesting user (row + level security, impersonation). Charts whose cached results are all present + are skipped; only missing or expired results are computed. + """ + # pylint: disable=import-outside-toplevel + from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand + from superset.utils.core import override_user + + results: dict[str, list[int]] = {"success": [], "errors": []} + user: Any = security_manager.get_user_by_id(user_id) + if not user: + logger.warning("Login cache warm-up: user %s not found", user_id) + return results + + limit: int = current_app.config["CACHE_WARMUP_ON_LOGIN_MAX_DASHBOARDS"] + with override_user(user, force=True): + for dashboard in _get_favorite_dashboards(user_id, limit): + for chart in dashboard.slices: + try: + outcome: dict[str, Any] = ChartWarmUpCacheCommand( + chart, dashboard.id, None, only_if_stale=True + ).run(skip_disabled=True) + except Exception: # noqa: BLE001 + logger.exception( + "Error warming up chart %s on dashboard %s", + chart.id, + dashboard.id, + ) + results["errors"].append(chart.id) + continue + key: str = "errors" if outcome.get("viz_error") else "success" + results[key].append(chart.id) + return results + + +def schedule_login_cache_warmup(user: Any) -> None: + """ + Enqueue a cache warm-up of the user's favorited dashboards after login. + + A no-op unless ``CACHE_WARMUP_ON_LOGIN`` is enabled. A per-user cooldown + (``CACHE_WARMUP_ON_LOGIN_COOLDOWN``) bounds how often logins can enqueue work. + Never raises, so a warm-up problem cannot affect the login. + """ + if not current_app.config.get("CACHE_WARMUP_ON_LOGIN"): + return + try: + cooldown: int = current_app.config["CACHE_WARMUP_ON_LOGIN_COOLDOWN"] + if cooldown > 0 and not cache_manager.cache.add( + f"cache-warmup-login:{user.id}", 1, timeout=cooldown + ): + return + cache_warmup_user_favorites.delay(user.id) + except Exception: # noqa: BLE001 + logger.exception("Could not schedule login cache warm-up") diff --git a/tests/unit_tests/commands/chart/warm_up_cache_test.py b/tests/unit_tests/commands/chart/warm_up_cache_test.py index 64cad9de7b2d..dad700121467 100644 --- a/tests/unit_tests/commands/chart/warm_up_cache_test.py +++ b/tests/unit_tests/commands/chart/warm_up_cache_test.py @@ -14,7 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from unittest.mock import Mock, patch +from unittest.mock import MagicMock, Mock, patch import pytest @@ -219,12 +219,12 @@ def test_handles_empty_dashboard_filters( ChartWarmUpCacheCommand(chart, 42, None).run() # VALIDATE: No filters added (empty list case) - assert len(mock_query.filter) == 0, ( - "No filters should be added when dashboard has no filters" - ) - assert mock_get_dashboard_filters.called, ( - "Should still call get_dashboard_extra_filters" - ) + assert ( + len(mock_query.filter) == 0 + ), "No filters should be added when dashboard has no filters" + assert ( + mock_get_dashboard_filters.called + ), "Should still call get_dashboard_extra_filters" @patch("superset.commands.chart.warm_up_cache.ChartDataCommand") @@ -279,12 +279,12 @@ def test_none_query_context_raises_chart_invalid_error(mock_chart_data_command): assert result["viz_error"] is not None, "Should return an error" assert result["chart_id"] == 129 error_str = str(result["viz_error"]).lower() - assert "query context" in error_str, ( - f"Error should mention query context: {result['viz_error']}" - ) - assert "not exist" in error_str, ( - f"Error should mention not exist: {result['viz_error']}" - ) + assert ( + "query context" in error_str + ), f"Error should mention query context: {result['viz_error']}" + assert ( + "not exist" in error_str + ), f"Error should mention not exist: {result['viz_error']}" @patch("superset.commands.chart.warm_up_cache.ChartDataCommand") @@ -344,3 +344,25 @@ def test_validate_with_nonexistent_chart_id(mock_db): with pytest.raises(WarmUpCacheChartNotFoundError): command.validate() + + +def test_only_if_stale_skips_fully_cached_chart() -> None: + from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand + + query_context = MagicMock(queries=[MagicMock(), MagicMock()]) + query_context.get_cache_timeout.return_value = 300 + query_context.query_cache_key.side_effect = ["k1", "k2"] + with patch("superset.commands.chart.warm_up_cache.cache_manager") as cm: + cm.data_cache.get.return_value = {"df": 1} + assert ChartWarmUpCacheCommand._is_fully_cached(query_context) is True + + +def test_only_if_stale_detects_missing_entry() -> None: + from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand + + query_context = MagicMock(queries=[MagicMock(), MagicMock()]) + query_context.get_cache_timeout.return_value = 300 + query_context.query_cache_key.side_effect = ["k1", "k2"] + with patch("superset.commands.chart.warm_up_cache.cache_manager") as cm: + cm.data_cache.get.side_effect = [{"df": 1}, None] + assert ChartWarmUpCacheCommand._is_fully_cached(query_context) is False diff --git a/tests/unit_tests/security/audit_log_test.py b/tests/unit_tests/security/audit_log_test.py index e67f9aa108fe..374835775c4a 100644 --- a/tests/unit_tests/security/audit_log_test.py +++ b/tests/unit_tests/security/audit_log_test.py @@ -269,3 +269,22 @@ def test_on_user_logout_logs_event(mock_log: MagicMock) -> None: mock_log.assert_called_once_with( "UserLoggedOut", {"username": "testuser", "user_id": 7} ) + + +@patch("superset.tasks.cache.schedule_login_cache_warmup") +@patch("superset.security.session_invalidation.stamp_login_time") +@patch("superset.security.manager._log_audit_event") +def test_on_user_login_schedules_cache_warmup( + mock_log: MagicMock, + mock_stamp_login_time: MagicMock, + mock_schedule: MagicMock, +) -> None: + """on_user_login hands the user to the login cache warm-up scheduler.""" + sm = SupersetSecurityManager.__new__(SupersetSecurityManager) + user = MagicMock(spec=User) + user.username = "testuser" + user.id = 7 + + sm.on_user_login(user) + + mock_schedule.assert_called_once_with(user) diff --git a/tests/unit_tests/tasks/test_cache.py b/tests/unit_tests/tasks/test_cache.py index 172b20009dca..7c258402d6bd 100644 --- a/tests/unit_tests/tasks/test_cache.py +++ b/tests/unit_tests/tasks/test_cache.py @@ -68,7 +68,9 @@ def test_cache_warmup_user_not_found(app_context: None) -> None: "superset.tasks.cache.current_app", _fake_app({"SUPERSET_CACHE_WARMUP_USER": "bot"}), ), - mock.patch("superset.tasks.cache.security_manager") as mock_sm, + mock.patch( + "superset.tasks.cache.security_manager", new_callable=mock.MagicMock + ) as mock_sm, mock.patch("superset.tasks.cache.WebDriverPlaywright") as mock_wd, ): mock_sm.find_user = mock.MagicMock(return_value=None) @@ -92,7 +94,9 @@ def test_cache_warmup_happy_path(app_context: None) -> None: "superset.tasks.cache.current_app", _fake_app({"SUPERSET_CACHE_WARMUP_USER": "bot"}), ), - mock.patch("superset.tasks.cache.security_manager") as mock_sm, + mock.patch( + "superset.tasks.cache.security_manager", new_callable=mock.MagicMock + ) as mock_sm, mock.patch("superset.tasks.cache.WebDriverPlaywright") as mock_wd, mock.patch("superset.tasks.cache.DummyStrategy.get_urls", return_value=urls), ): @@ -120,7 +124,9 @@ def side_effect(url: str, _element: str, **kwargs: Any) -> Any: "superset.tasks.cache.current_app", _fake_app({"SUPERSET_CACHE_WARMUP_USER": "bot"}), ), - mock.patch("superset.tasks.cache.security_manager") as mock_sm, + mock.patch( + "superset.tasks.cache.security_manager", new_callable=mock.MagicMock + ) as mock_sm, mock.patch("superset.tasks.cache.WebDriverPlaywright") as mock_wd, mock.patch("superset.tasks.cache.DummyStrategy.get_urls", return_value=urls), ): @@ -152,7 +158,9 @@ def side_effect(url: str, _element: str, **kwargs: Any) -> bytes | None: "superset.tasks.cache.current_app", _fake_app({"SUPERSET_CACHE_WARMUP_USER": "bot"}), ), - mock.patch("superset.tasks.cache.security_manager") as mock_sm, + mock.patch( + "superset.tasks.cache.security_manager", new_callable=mock.MagicMock + ) as mock_sm, mock.patch("superset.tasks.cache.WebDriverPlaywright") as mock_wd, mock.patch("superset.tasks.cache.DummyStrategy.get_urls", return_value=urls), ): @@ -429,3 +437,125 @@ def test_native_filter_options_strategy_registered() -> None: from superset.tasks.cache import NativeFilterOptionsStrategy, strategy_registry assert strategy_registry["native_filter_options"] is NativeFilterOptionsStrategy + + +def test_schedule_login_warmup_flag_off(app_context: None) -> None: + """With the setting disabled nothing is enqueued.""" + from superset.tasks.cache import schedule_login_cache_warmup + + with ( + mock.patch( + "superset.tasks.cache.current_app", + _fake_app({"CACHE_WARMUP_ON_LOGIN": False}), + ), + mock.patch("superset.tasks.cache.cache_warmup_user_favorites") as task, + ): + schedule_login_cache_warmup(mock.MagicMock(id=1)) + + task.delay.assert_not_called() + + +def test_schedule_login_warmup_enqueues_once_per_cooldown(app_context: None) -> None: + """The first login enqueues; a login inside the cooldown does not.""" + from superset.tasks.cache import schedule_login_cache_warmup + + config = {"CACHE_WARMUP_ON_LOGIN": True, "CACHE_WARMUP_ON_LOGIN_COOLDOWN": 60} + with ( + mock.patch("superset.tasks.cache.current_app", _fake_app(config)), + mock.patch("superset.tasks.cache.cache_manager") as cm, + mock.patch("superset.tasks.cache.cache_warmup_user_favorites") as task, + ): + cm.cache.add.return_value = True + schedule_login_cache_warmup(mock.MagicMock(id=7)) + cm.cache.add.return_value = False + schedule_login_cache_warmup(mock.MagicMock(id=7)) + + task.delay.assert_called_once_with(7) + + +def test_schedule_login_warmup_swallows_errors(app_context: None) -> None: + """A broker failure must not propagate into the login flow.""" + from superset.tasks.cache import schedule_login_cache_warmup + + config = {"CACHE_WARMUP_ON_LOGIN": True, "CACHE_WARMUP_ON_LOGIN_COOLDOWN": 0} + with ( + mock.patch("superset.tasks.cache.current_app", _fake_app(config)), + mock.patch("superset.tasks.cache.cache_warmup_user_favorites") as task, + ): + task.delay.side_effect = RuntimeError("broker down") + schedule_login_cache_warmup(mock.MagicMock(id=7)) + + +def test_user_favorites_task_warms_stale_charts_only(app_context: None) -> None: + """Every favorited chart goes through the stale-only command, capped by config.""" + from superset.tasks.cache import cache_warmup_user_favorites + + chart_a, chart_b = mock.MagicMock(id=1), mock.MagicMock(id=2) + dashboard = mock.MagicMock(id=10, slices=[chart_a, chart_b]) + config = {"CACHE_WARMUP_ON_LOGIN_MAX_DASHBOARDS": 3} + with ( + mock.patch("superset.tasks.cache.current_app", _fake_app(config)), + mock.patch( + "superset.tasks.cache.security_manager", new_callable=mock.MagicMock + ), + mock.patch( + "superset.tasks.cache._get_favorite_dashboards", return_value=[dashboard] + ) as favs, + mock.patch( + "superset.commands.chart.warm_up_cache.ChartWarmUpCacheCommand" + ) as command, + ): + command.return_value.run.side_effect = [ + {"chart_id": 1, "viz_error": None}, + {"chart_id": 2, "viz_error": "boom"}, + ] + result = cache_warmup_user_favorites(5) + + favs.assert_called_once_with(5, 3) + command.assert_any_call(chart_a, 10, None, only_if_stale=True) + assert result == {"success": [1], "errors": [2]} + + +def test_user_favorites_task_unknown_user(app_context: None) -> None: + from superset.tasks.cache import cache_warmup_user_favorites + + with ( + mock.patch( + "superset.tasks.cache.security_manager", new_callable=mock.MagicMock + ) as sm, + mock.patch("superset.tasks.cache._get_favorite_dashboards") as favs, + ): + sm.get_user_by_id.return_value = None + assert cache_warmup_user_favorites(99) == {"success": [], "errors": []} + favs.assert_not_called() + + +def test_get_favorite_dashboards_order_cap_and_access(app_context: None) -> None: + """Newest favorites first, inaccessible dashboards skipped, capped at limit.""" + from superset.exceptions import SupersetSecurityException + from superset.tasks.cache import _get_favorite_dashboards + + dashboards = {i: mock.MagicMock(id=i) for i in (1, 2, 3, 4)} + + def raise_for_access(dashboard: Any) -> None: + if dashboard.id == 2: + raise SupersetSecurityException(mock.MagicMock()) + + with ( + mock.patch("superset.tasks.cache.db") as db, + mock.patch( + "superset.tasks.cache.security_manager", new_callable=mock.MagicMock + ) as sm, + ): + sm.raise_for_access.side_effect = raise_for_access + query = db.session.query.return_value + # Favorites query (newest first), then dashboards lookup. + query.filter.return_value.order_by.return_value.all.return_value = [ + mock.MagicMock(obj_id=i) for i in (3, 2, 1, 4) + ] + query.options.return_value.filter.return_value.all.return_value = list( + dashboards.values() + ) + result = _get_favorite_dashboards(5, 2) + + assert [d.id for d in result] == [3, 1] From 0e5c24dea952ef459dc0f2f72fce43eaba1f337e Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 30 Oct 2026 03:08:58 +0000 Subject: [PATCH 2/6] feat(cache): add queue, per-result lock and time limit to login warm-up Co-Authored-By: Claude Sonnet 5.5 --- docs/admin_docs/configuration/cache.mdx | 9 +++- superset/commands/chart/warm_up_cache.py | 34 +++++++++++---- superset/config.py | 4 ++ superset/tasks/cache.py | 22 ++++++++-- .../commands/chart/warm_up_cache_test.py | 41 +++++++++++++------ tests/unit_tests/tasks/test_cache.py | 9 ++-- 6 files changed, 90 insertions(+), 29 deletions(-) diff --git a/docs/admin_docs/configuration/cache.mdx b/docs/admin_docs/configuration/cache.mdx index 028b8a731ecb..850b08545402 100644 --- a/docs/admin_docs/configuration/cache.mdx +++ b/docs/admin_docs/configuration/cache.mdx @@ -291,6 +291,7 @@ default: CACHE_WARMUP_ON_LOGIN = True CACHE_WARMUP_ON_LOGIN_MAX_DASHBOARDS = 5 # most recently favorited dashboards per login CACHE_WARMUP_ON_LOGIN_COOLDOWN = 3600 # seconds between warm-ups for the same user; 0 disables +CACHE_WARMUP_ON_LOGIN_QUEUE = "cache_warmup" # optional dedicated Celery queue ``` After a successful login, a Celery task runs as that user and checks every chart on their @@ -301,7 +302,13 @@ Requirements: - A Celery worker, and a real `DATA_CACHE_CONFIG` backend (with `NullCache` nothing can be found in the cache, so every login would recompute). -- The cooldown is tracked in `CACHE_CONFIG`. +- The cooldown and a per-result lock (so concurrent warm-ups do not compute the same result + twice) are tracked in `CACHE_CONFIG`; use a shared backend such as Redis for them to work + across processes. +- Set `CACHE_WARMUP_ON_LOGIN_QUEUE` and run a worker for that queue to keep warm-up from + competing with reports, alerts and thumbnails. The task has a time limit of 15 minutes. +- The hook runs on logins handled by Flask-AppBuilder (database, LDAP, OAuth, remote user). + Sessions authenticated by other means, such as JWT or guest tokens, do not trigger it. - The task runs with the logging-in user's permissions, so the cache entries match what that user's dashboards request. Dashboard default filters are not applied, so charts whose dashboard load uses different filters may still miss the cache. diff --git a/superset/commands/chart/warm_up_cache.py b/superset/commands/chart/warm_up_cache.py index 953e1c68408b..c92581e05c7a 100644 --- a/superset/commands/chart/warm_up_cache.py +++ b/superset/commands/chart/warm_up_cache.py @@ -38,6 +38,9 @@ from superset.utils.core import error_msg_from_exception, QueryObjectFilterClause from superset.views.utils import get_dashboard_extra_filters +# Seconds a per-result warm-up lock is held if the worker dies before releasing it. +WARMUP_LOCK_TTL = 900 + class ChartWarmUpCacheCommand(BaseCommand): def __init__( @@ -65,16 +68,19 @@ def _get_dashboard_filters(self, chart_id: int) -> list[dict[str, Any]]: return get_dashboard_extra_filters(chart_id, self._dashboard_id) @staticmethod - def _is_fully_cached(query_context: Any) -> bool: - """Return whether every query of the context has an unexpired cache entry.""" + def _missing_cache_keys(query_context: Any) -> list[str]: + """Return cache keys of the context's queries lacking an unexpired entry.""" if query_context.get_cache_timeout() == CACHE_DISABLED_TIMEOUT: # Results are never stored, so there is nothing to warm. - return True + return [] + missing: list[str] = [] for query in query_context.queries: + # Validation normalizes filters, matching the key used when querying. + query.validate() cache_key = query_context.query_cache_key(query) if not cache_key or cache_manager.data_cache.get(cache_key) is None: - return False - return True + missing.append(cache_key or "") + return missing def _warm_up_non_legacy_cache(self, chart: Slice) -> tuple[Any, Any]: """Warm up cache for non-legacy visualizations.""" @@ -94,14 +100,24 @@ def _warm_up_non_legacy_cache(self, chart: Slice) -> tuple[Any, Any]: + query.filter ) + lock_key: str | None = None if self._only_if_stale: - if self._is_fully_cached(query_context): + missing = self._missing_cache_keys(query_context) + if not missing: + return None, QueryStatus.SUCCESS + # Only one worker warms a given result at a time. + lock_key = f"cache-warmup-lock:{missing[0]}" + if not cache_manager.cache.add(lock_key, 1, timeout=WARMUP_LOCK_TTL): return None, QueryStatus.SUCCESS else: query_context.force = True - command = ChartDataCommand(query_context) - command.validate() - payload = command.run() + try: + command = ChartDataCommand(query_context) + command.validate() + payload = command.run() + finally: + if lock_key: + cache_manager.cache.delete(lock_key) # Report the first error. for query_result in cast(list[dict[str, Any]], payload["queries"]): diff --git a/superset/config.py b/superset/config.py index 746671c7b924..471faedc3e95 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1489,6 +1489,10 @@ def sync_theme_logo_href( # Minimum seconds between login warm-ups for the same user, uses CACHE_CONFIG. # Set to 0 to disable the cooldown. CACHE_WARMUP_ON_LOGIN_COOLDOWN = int(timedelta(hours=1).total_seconds()) +# Celery queue for login warm-up tasks. Point it at a dedicated, low-priority queue +# (served by its own worker) so a burst of logins cannot delay other tasks. None +# uses the default queue. +CACHE_WARMUP_ON_LOGIN_QUEUE: str | None = None # Time before selenium times out after trying to locate an element on the page and wait # for that element to load for a screenshot. diff --git a/superset/tasks/cache.py b/superset/tasks/cache.py index 2763742f72a9..69b1b9121477 100644 --- a/superset/tasks/cache.py +++ b/superset/tasks/cache.py @@ -20,6 +20,7 @@ from dataclasses import dataclass from typing import Any, Optional, Union +from celery.exceptions import SoftTimeLimitExceeded from celery.utils.log import get_task_logger from flask import current_app from sqlalchemy import and_, func @@ -464,7 +465,9 @@ def _get_favorite_dashboards(user_id: int, limit: int) -> list[Dashboard]: return dashboards -@celery_app.task(name="cache-warmup-user-favorites") +@celery_app.task( + name="cache-warmup-user-favorites", soft_time_limit=840, time_limit=900 +) def cache_warmup_user_favorites(user_id: int) -> dict[str, list[int]]: """ Warm the cache for charts on a user's favorited dashboards. @@ -491,6 +494,9 @@ def cache_warmup_user_favorites(user_id: int) -> dict[str, list[int]]: outcome: dict[str, Any] = ChartWarmUpCacheCommand( chart, dashboard.id, None, only_if_stale=True ).run(skip_disabled=True) + except SoftTimeLimitExceeded: + logger.warning("Login cache warm-up for user %s timed out", user_id) + return results except Exception: # noqa: BLE001 logger.exception( "Error warming up chart %s on dashboard %s", @@ -516,10 +522,20 @@ def schedule_login_cache_warmup(user: Any) -> None: return try: cooldown: int = current_app.config["CACHE_WARMUP_ON_LOGIN_COOLDOWN"] + cooldown_key: str = f"cache-warmup-login:{user.id}" if cooldown > 0 and not cache_manager.cache.add( - f"cache-warmup-login:{user.id}", 1, timeout=cooldown + cooldown_key, 1, timeout=cooldown ): return - cache_warmup_user_favorites.delay(user.id) + try: + cache_warmup_user_favorites.apply_async( + args=(user.id,), + queue=current_app.config.get("CACHE_WARMUP_ON_LOGIN_QUEUE"), + ) + except Exception: + # Release the cooldown so the next login can retry. + if cooldown > 0: + cache_manager.cache.delete(cooldown_key) + raise except Exception: # noqa: BLE001 logger.exception("Could not schedule login cache warm-up") diff --git a/tests/unit_tests/commands/chart/warm_up_cache_test.py b/tests/unit_tests/commands/chart/warm_up_cache_test.py index dad700121467..7fde90b52a26 100644 --- a/tests/unit_tests/commands/chart/warm_up_cache_test.py +++ b/tests/unit_tests/commands/chart/warm_up_cache_test.py @@ -346,23 +346,38 @@ def test_validate_with_nonexistent_chart_id(mock_db): command.validate() -def test_only_if_stale_skips_fully_cached_chart() -> None: - from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand - - query_context = MagicMock(queries=[MagicMock(), MagicMock()]) +def _ctx(keys: list[str]) -> MagicMock: + query_context = MagicMock(queries=[MagicMock() for _ in keys]) query_context.get_cache_timeout.return_value = 300 - query_context.query_cache_key.side_effect = ["k1", "k2"] + query_context.query_cache_key.side_effect = keys + return query_context + + +def test_missing_cache_keys_none_when_cached() -> None: with patch("superset.commands.chart.warm_up_cache.cache_manager") as cm: cm.data_cache.get.return_value = {"df": 1} - assert ChartWarmUpCacheCommand._is_fully_cached(query_context) is True + assert ChartWarmUpCacheCommand._missing_cache_keys(_ctx(["k1", "k2"])) == [] -def test_only_if_stale_detects_missing_entry() -> None: - from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand - - query_context = MagicMock(queries=[MagicMock(), MagicMock()]) - query_context.get_cache_timeout.return_value = 300 - query_context.query_cache_key.side_effect = ["k1", "k2"] +def test_missing_cache_keys_reports_expired_entry() -> None: with patch("superset.commands.chart.warm_up_cache.cache_manager") as cm: cm.data_cache.get.side_effect = [{"df": 1}, None] - assert ChartWarmUpCacheCommand._is_fully_cached(query_context) is False + assert ChartWarmUpCacheCommand._missing_cache_keys(_ctx(["k1", "k2"])) == ["k2"] + + +def test_missing_cache_keys_validates_before_keying() -> None: + """Keys are derived after query validation, as on the request path.""" + query_context = _ctx(["k1"]) + order: list[str] = [] + query_context.queries[0].validate.side_effect = lambda: order.append("validate") + query_context.query_cache_key.side_effect = lambda q: order.append("key") or "k1" + with patch("superset.commands.chart.warm_up_cache.cache_manager") as cm: + cm.data_cache.get.return_value = {"df": 1} + ChartWarmUpCacheCommand._missing_cache_keys(query_context) + assert order == ["validate", "key"] + + +def test_missing_cache_keys_empty_when_caching_disabled() -> None: + query_context = _ctx(["k1"]) + query_context.get_cache_timeout.return_value = -1 + assert ChartWarmUpCacheCommand._missing_cache_keys(query_context) == [] diff --git a/tests/unit_tests/tasks/test_cache.py b/tests/unit_tests/tasks/test_cache.py index 7c258402d6bd..ad9f6a5816ce 100644 --- a/tests/unit_tests/tasks/test_cache.py +++ b/tests/unit_tests/tasks/test_cache.py @@ -470,20 +470,23 @@ def test_schedule_login_warmup_enqueues_once_per_cooldown(app_context: None) -> cm.cache.add.return_value = False schedule_login_cache_warmup(mock.MagicMock(id=7)) - task.delay.assert_called_once_with(7) + task.apply_async.assert_called_once_with(args=(7,), queue=None) def test_schedule_login_warmup_swallows_errors(app_context: None) -> None: """A broker failure must not propagate into the login flow.""" from superset.tasks.cache import schedule_login_cache_warmup - config = {"CACHE_WARMUP_ON_LOGIN": True, "CACHE_WARMUP_ON_LOGIN_COOLDOWN": 0} + config = {"CACHE_WARMUP_ON_LOGIN": True, "CACHE_WARMUP_ON_LOGIN_COOLDOWN": 60} with ( mock.patch("superset.tasks.cache.current_app", _fake_app(config)), + mock.patch("superset.tasks.cache.cache_manager") as cm, mock.patch("superset.tasks.cache.cache_warmup_user_favorites") as task, ): - task.delay.side_effect = RuntimeError("broker down") + task.apply_async.side_effect = RuntimeError("broker down") + cm.cache.add.return_value = True schedule_login_cache_warmup(mock.MagicMock(id=7)) + cm.cache.delete.assert_called_once_with("cache-warmup-login:7") def test_user_favorites_task_warms_stale_charts_only(app_context: None) -> None: From eaa0358d9645bb54d11bd9393641d50ad0c2564e Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 30 Oct 2026 03:10:00 +0000 Subject: [PATCH 3/6] test(cache): satisfy mypy in warm-up cache test Co-Authored-By: Claude Sonnet 5.5 --- .../unit_tests/commands/chart/warm_up_cache_test.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/unit_tests/commands/chart/warm_up_cache_test.py b/tests/unit_tests/commands/chart/warm_up_cache_test.py index 7fde90b52a26..f2493f7c3c15 100644 --- a/tests/unit_tests/commands/chart/warm_up_cache_test.py +++ b/tests/unit_tests/commands/chart/warm_up_cache_test.py @@ -14,6 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from typing import Any from unittest.mock import MagicMock, Mock, patch import pytest @@ -369,8 +370,16 @@ def test_missing_cache_keys_validates_before_keying() -> None: """Keys are derived after query validation, as on the request path.""" query_context = _ctx(["k1"]) order: list[str] = [] - query_context.queries[0].validate.side_effect = lambda: order.append("validate") - query_context.query_cache_key.side_effect = lambda q: order.append("key") or "k1" + + def record_validate() -> None: + order.append("validate") + + def record_key(_query: Any) -> str: + order.append("key") + return "k1" + + query_context.queries[0].validate.side_effect = record_validate + query_context.query_cache_key.side_effect = record_key with patch("superset.commands.chart.warm_up_cache.cache_manager") as cm: cm.data_cache.get.return_value = {"df": 1} ChartWarmUpCacheCommand._missing_cache_keys(query_context) From b84e28eac785403ba289d068a6d4d5c5a9462351 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 30 Oct 2026 03:20:55 +0000 Subject: [PATCH 4/6] feat(cache): add favorites cache warm-up strategy Replaces the login-triggered warm-up with a beat-scheduled `favorites` strategy for the cache-warmup task. It enqueues one task per recently active user that warms only missing or expired results on their favorited dashboards, with a per-result lock, dashboard cap, optional dedicated queue and a task time limit. Co-Authored-By: Claude Sonnet 5.5 --- docs/admin_docs/configuration/cache.mdx | 58 ++++---- superset/config.py | 29 ++-- superset/security/manager.py | 2 - superset/tasks/cache.py | 129 +++++++++++++----- .../commands/chart/warm_up_cache_key_test.py | 118 ++++++++++++++++ tests/unit_tests/security/audit_log_test.py | 19 --- tests/unit_tests/tasks/test_cache.py | 84 +++++++----- 7 files changed, 309 insertions(+), 130 deletions(-) create mode 100644 tests/unit_tests/commands/chart/warm_up_cache_key_test.py diff --git a/docs/admin_docs/configuration/cache.mdx b/docs/admin_docs/configuration/cache.mdx index 850b08545402..6f724254d347 100644 --- a/docs/admin_docs/configuration/cache.mdx +++ b/docs/admin_docs/configuration/cache.mdx @@ -282,36 +282,44 @@ Requirements and limitations: - Cascading/dependent native filters and search-term variants of filter option queries are not warmed by this strategy. -### Warming Up Favorited Dashboards on Login +### Warming Up Favorited Dashboards -Superset can warm the cache for a user's favorited dashboards when they log in. It is off by -default: +The `favorites` strategy warms the cache for each user's favorited dashboards. It is off by +default; enable it and schedule it with Celery beat: ```python -CACHE_WARMUP_ON_LOGIN = True -CACHE_WARMUP_ON_LOGIN_MAX_DASHBOARDS = 5 # most recently favorited dashboards per login -CACHE_WARMUP_ON_LOGIN_COOLDOWN = 3600 # seconds between warm-ups for the same user; 0 disables -CACHE_WARMUP_ON_LOGIN_QUEUE = "cache_warmup" # optional dedicated Celery queue +CACHE_WARMUP_FAVORITES = True +CACHE_WARMUP_FAVORITES_MAX_DASHBOARDS = 5 # most recently favorited dashboards per user +CACHE_WARMUP_FAVORITES_QUEUE = "cache_warmup" # optional dedicated Celery queue + +class CeleryConfig(CeleryConfig): + beat_schedule = { + "cache-warmup-favorites": { + "task": "cache-warmup", + "schedule": crontab(minute=0, hour="*/6"), + "kwargs": { + "strategy_name": "favorites", + "since": "7 days ago", # only users who logged in since then + "max_users": 100, # users handled per run, most recent login first + }, + }, + } ``` -After a successful login, a Celery task runs as that user and checks every chart on their -favorited dashboards. Only charts whose query results are missing or expired are computed; -charts that are already cached are skipped. Login itself is never delayed by this work. - -Requirements: - -- A Celery worker, and a real `DATA_CACHE_CONFIG` backend (with `NullCache` nothing can be - found in the cache, so every login would recompute). -- The cooldown and a per-result lock (so concurrent warm-ups do not compute the same result - twice) are tracked in `CACHE_CONFIG`; use a shared backend such as Redis for them to work - across processes. -- Set `CACHE_WARMUP_ON_LOGIN_QUEUE` and run a worker for that queue to keep warm-up from - competing with reports, alerts and thumbnails. The task has a time limit of 15 minutes. -- The hook runs on logins handled by Flask-AppBuilder (database, LDAP, OAuth, remote user). - Sessions authenticated by other means, such as JWT or guest tokens, do not trigger it. -- The task runs with the logging-in user's permissions, so the cache entries match what that - user's dashboards request. Dashboard default filters are not applied, so charts whose - dashboard load uses different filters may still miss the cache. +Each run enqueues one task per selected user. The task runs as that user, so the cache entries +match what that user's dashboards request, and only charts whose query results are missing or +expired are computed. It does not use `SUPERSET_CACHE_WARMUP_USER`. + +Notes: + +- A Celery worker and a real `DATA_CACHE_CONFIG` backend are required (with `NullCache` every + run would recompute). +- A per-result lock, kept in `CACHE_CONFIG`, stops concurrent runs from computing the same + result twice; use a shared backend such as Redis for it to work across processes. +- Set `CACHE_WARMUP_FAVORITES_QUEUE` and run a worker for that queue to keep warm-up from + competing with reports, alerts and thumbnails. Each per-user task has a 15 minute time limit. +- Dashboard default filters are not applied, so charts whose dashboard load uses different + filters may still miss the cache. ## Caching Thumbnails diff --git a/superset/config.py b/superset/config.py index 471faedc3e95..eff0ca576dc0 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1476,23 +1476,20 @@ def sync_theme_logo_href( # - Cascade/dependent filters and search-term variants are not warmed in this # version; only base option queries are supported. -# Warm the cache for a user's favorited dashboards right after they log in. -# An asynchronous Celery task runs as the logging-in user and, for each chart on -# their most recently favorited dashboards, computes only the query results that -# are missing or expired in DATA_CACHE_CONFIG; charts that are already cached are -# skipped. Login is never blocked. Requires a Celery worker and a real -# DATA_CACHE_CONFIG backend (with NullCache every login would recompute). -CACHE_WARMUP_ON_LOGIN = False +# Gate for the `favorites` cache warm-up strategy, which warms charts on each +# user's favorited dashboards. Schedule it through the `cache-warmup` Celery beat +# task (see the `FavoritesStrategy` docstring in superset/tasks/cache.py). A +# separate task per user runs as that user and computes only query results that +# are missing or expired in DATA_CACHE_CONFIG. Requires a Celery worker and a real +# DATA_CACHE_CONFIG backend (with NullCache every run would recompute). +CACHE_WARMUP_FAVORITES = False # Maximum number of favorited dashboards (most recently favorited first) warmed -# per login. -CACHE_WARMUP_ON_LOGIN_MAX_DASHBOARDS = 5 -# Minimum seconds between login warm-ups for the same user, uses CACHE_CONFIG. -# Set to 0 to disable the cooldown. -CACHE_WARMUP_ON_LOGIN_COOLDOWN = int(timedelta(hours=1).total_seconds()) -# Celery queue for login warm-up tasks. Point it at a dedicated, low-priority queue -# (served by its own worker) so a burst of logins cannot delay other tasks. None -# uses the default queue. -CACHE_WARMUP_ON_LOGIN_QUEUE: str | None = None +# per user per run. +CACHE_WARMUP_FAVORITES_MAX_DASHBOARDS = 5 +# Celery queue for per-user favorites warm-up tasks. Point it at a dedicated, +# low-priority queue (served by its own worker) so a warm-up burst cannot delay +# other tasks. None uses the default queue. +CACHE_WARMUP_FAVORITES_QUEUE: str | None = None # Time before selenium times out after trying to locate an element on the page and wait # for that element to load for a screenshot. diff --git a/superset/security/manager.py b/superset/security/manager.py index 2a633c8847eb..952aa4a0000b 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -2418,7 +2418,6 @@ def _same_user(left: Any, right: Any) -> bool: def on_user_login(self, user: Any) -> None: # pylint: disable=import-outside-toplevel from superset.security.session_invalidation import stamp_login_time - from superset.tasks.cache import schedule_login_cache_warmup # Record the authentication time so outstanding sessions can be # invalidated when the account is later disabled. @@ -2427,7 +2426,6 @@ def on_user_login(self, user: Any) -> None: "UserLoggedIn", {"username": user.username, "user_id": user.id}, ) - schedule_login_cache_warmup(user) def on_user_login_failed(self, user: Any) -> None: _log_audit_event( diff --git a/superset/tasks/cache.py b/superset/tasks/cache.py index 69b1b9121477..5d0b9c9adb64 100644 --- a/superset/tasks/cache.py +++ b/superset/tasks/cache.py @@ -30,7 +30,7 @@ from superset.common.query_context import QueryContext from superset.daos.dashboard import DashboardDAO from superset.exceptions import SupersetSecurityException -from superset.extensions import cache_manager, celery_app +from superset.extensions import celery_app from superset.models.core import FavStar, FavStarClassName, Log from superset.models.dashboard import Dashboard from superset.tags.models import Tag, TaggedObject @@ -94,6 +94,8 @@ class Strategy: # pylint: disable=too-few-public-methods name: str = "" uses_webdriver: bool = True + # Per-user strategies fan out one task per user from ``get_user_ids``. + per_user: bool = False def __init__(self) -> None: pass @@ -104,6 +106,9 @@ def get_urls(self) -> list[str]: def get_tasks(self) -> list[CacheWarmupTask]: return [] + def get_user_ids(self) -> list[int]: + return [] + class DummyStrategy(Strategy): # pylint: disable=too-few-public-methods """ @@ -318,11 +323,72 @@ def get_tasks(self) -> list[CacheWarmupTask]: return tasks +class FavoritesStrategy(Strategy): # pylint: disable=too-few-public-methods + """ + Warm up charts on each user's favorited dashboards. + + Cache keys depend on the requesting user (row level security, impersonation), + so each user's dashboards are warmed by a separate task that runs as that user. + Only results that are missing or expired are computed. Users are selected by + recent login, most recent first, up to ``max_users`` per run. Requires + ``CACHE_WARMUP_FAVORITES = True``. + + beat_schedule = { + 'cache-warmup-favorites': { + 'task': 'cache-warmup', + 'schedule': crontab(minute=0, hour='*/6'), + 'kwargs': { + 'strategy_name': 'favorites', + 'since': '7 days ago', + 'max_users': 100, + }, + }, + } + + """ + + name: str = "favorites" + uses_webdriver: bool = False + per_user: bool = True + + def __init__(self, since: str = "7 days ago", max_users: int = 100) -> None: + super().__init__() + self.since: Any = parse_human_datetime(since) if since else None + self.max_users: int = max_users + + def get_urls(self) -> list[str]: + return [] + + def get_user_ids(self) -> list[int]: + """Return ids of active users with favorited dashboards, newest login first.""" + query = ( + db.session.query(FavStar.user_id) + .join( + security_manager.user_model, + security_manager.user_model.id == FavStar.user_id, + ) + .filter( + FavStar.class_name == FavStarClassName.DASHBOARD, + security_manager.user_model.active.is_(True), + ) + ) + if self.since: + query = query.filter(security_manager.user_model.last_login >= self.since) + rows = ( + query.group_by(FavStar.user_id, security_manager.user_model.last_login) + .order_by(security_manager.user_model.last_login.desc()) + .limit(self.max_users) + .all() + ) + return [row.user_id for row in rows] + + strategies: list[type[Strategy]] = [ DummyStrategy, TopNDashboardsStrategy, DashboardTagsStrategy, NativeFilterOptionsStrategy, + FavoritesStrategy, ] strategy_registry: dict[str, type[Strategy]] = { strategy.name: strategy for strategy in strategies @@ -351,6 +417,25 @@ def _warmup_urls( _browser_manager._cleanup() # noqa: SLF001 +def _dispatch_per_user( + strategy: Strategy, results: dict[str, list[str]] +) -> Union[dict[str, list[str]], str]: + """Enqueue one warm-up task per user selected by a per-user strategy.""" + if not current_app.config.get("CACHE_WARMUP_FAVORITES"): + message = "CACHE_WARMUP_FAVORITES is disabled; nothing was warmed up." + logger.warning(message) + return message + queue: str | None = current_app.config.get("CACHE_WARMUP_FAVORITES_QUEUE") + for user_id in strategy.get_user_ids(): + try: + cache_warmup_user_favorites.apply_async(args=(user_id,), queue=queue) + results["success"].append(f"user:{user_id}") + except Exception: # noqa: BLE001 + logger.exception("Error enqueueing cache warm-up for user %s", user_id) + results["errors"].append(f"user:{user_id}") + return results + + @celery_app.task(name="cache-warmup") def cache_warmup( strategy_name: str, *args: Any, **kwargs: Any @@ -379,6 +464,9 @@ def cache_warmup( results: dict[str, list[str]] = {"success": [], "errors": []} + if strategy.per_user: + return _dispatch_per_user(strategy, results) + warmup_username: str | None = current_app.config.get("SUPERSET_CACHE_WARMUP_USER") if not warmup_username: message = ( @@ -483,10 +571,10 @@ def cache_warmup_user_favorites(user_id: int) -> dict[str, list[int]]: results: dict[str, list[int]] = {"success": [], "errors": []} user: Any = security_manager.get_user_by_id(user_id) if not user: - logger.warning("Login cache warm-up: user %s not found", user_id) + logger.warning("Favorites cache warm-up: user %s not found", user_id) return results - limit: int = current_app.config["CACHE_WARMUP_ON_LOGIN_MAX_DASHBOARDS"] + limit: int = current_app.config["CACHE_WARMUP_FAVORITES_MAX_DASHBOARDS"] with override_user(user, force=True): for dashboard in _get_favorite_dashboards(user_id, limit): for chart in dashboard.slices: @@ -495,7 +583,9 @@ def cache_warmup_user_favorites(user_id: int) -> dict[str, list[int]]: chart, dashboard.id, None, only_if_stale=True ).run(skip_disabled=True) except SoftTimeLimitExceeded: - logger.warning("Login cache warm-up for user %s timed out", user_id) + logger.warning( + "Favorites cache warm-up for user %s timed out", user_id + ) return results except Exception: # noqa: BLE001 logger.exception( @@ -508,34 +598,3 @@ def cache_warmup_user_favorites(user_id: int) -> dict[str, list[int]]: key: str = "errors" if outcome.get("viz_error") else "success" results[key].append(chart.id) return results - - -def schedule_login_cache_warmup(user: Any) -> None: - """ - Enqueue a cache warm-up of the user's favorited dashboards after login. - - A no-op unless ``CACHE_WARMUP_ON_LOGIN`` is enabled. A per-user cooldown - (``CACHE_WARMUP_ON_LOGIN_COOLDOWN``) bounds how often logins can enqueue work. - Never raises, so a warm-up problem cannot affect the login. - """ - if not current_app.config.get("CACHE_WARMUP_ON_LOGIN"): - return - try: - cooldown: int = current_app.config["CACHE_WARMUP_ON_LOGIN_COOLDOWN"] - cooldown_key: str = f"cache-warmup-login:{user.id}" - if cooldown > 0 and not cache_manager.cache.add( - cooldown_key, 1, timeout=cooldown - ): - return - try: - cache_warmup_user_favorites.apply_async( - args=(user.id,), - queue=current_app.config.get("CACHE_WARMUP_ON_LOGIN_QUEUE"), - ) - except Exception: - # Release the cooldown so the next login can retry. - if cooldown > 0: - cache_manager.cache.delete(cooldown_key) - raise - except Exception: # noqa: BLE001 - logger.exception("Could not schedule login cache warm-up") diff --git a/tests/unit_tests/commands/chart/warm_up_cache_key_test.py b/tests/unit_tests/commands/chart/warm_up_cache_key_test.py new file mode 100644 index 000000000000..6d66ae25ead7 --- /dev/null +++ b/tests/unit_tests/commands/chart/warm_up_cache_key_test.py @@ -0,0 +1,118 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""The warm-up cache lookup must use the same key as request-time execution.""" + +from typing import Any +from unittest.mock import MagicMock, patch + +from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand +from superset.common.query_context_processor import QueryContextProcessor +from superset.common.query_object import QueryObject + + +def _query_context( + time_range: str = "Last week", database_extra: str = "{}" +) -> MagicMock: + datasource = MagicMock() + datasource.database.extra = database_extra + datasource.database.impersonate_user = False + datasource.uid = "1__table" + datasource.changed_on = None + datasource.get_extra_cache_keys.return_value = [] + datasource.column_names = ["a"] + query = QueryObject( + datasource=datasource, + columns=["a"], + metrics=["count"], + time_range=time_range, + ) + query_context = MagicMock() + query_context.datasource = datasource + query_context.queries = [query] + query_context.get_cache_timeout.return_value = 300 + processor = QueryContextProcessor(query_context) + query_context.query_cache_key.side_effect = processor.query_cache_key + return query_context + + +def _lookup_keys(query_context: MagicMock, rls: list[str]) -> list[str]: + """Keys the warm-up path looks up, recorded from the cache read.""" + looked_up: list[str] = [] + with ( + patch("superset.commands.chart.warm_up_cache.cache_manager") as cm, + patch( + "superset.common.query_context_processor.security_manager", + new_callable=MagicMock, + ) as sm, + ): + sm.get_rls_cache_key.return_value = rls + cm.data_cache.get.side_effect = lambda key: looked_up.append(key) + ChartWarmUpCacheCommand._missing_cache_keys(query_context) + return looked_up + + +def _request_key(query_context: MagicMock, rls: list[str]) -> Any: + """Key the request path derives: validate the query, then build the key.""" + query = query_context.queries[0] + query.validate() + with patch( + "superset.common.query_context_processor.security_manager", + new_callable=MagicMock, + ) as sm: + sm.get_rls_cache_key.return_value = rls + return query_context.query_cache_key(query) + + +def test_warmup_key_matches_request_key(app_context: None) -> None: + query_context = _query_context() + assert _lookup_keys(query_context, ["rls"]) == [ + _request_key(query_context, ["rls"]) + ] + + +def test_row_level_security_changes_the_key(app_context: None) -> None: + query_context = _query_context() + assert _lookup_keys(query_context, ["region = 'EU'"]) != _lookup_keys( + query_context, ["region = 'US'"] + ) + + +def test_relative_time_range_key_is_stable(app_context: None) -> None: + """Keys use the user-supplied relative range, not the resolved timestamps.""" + first = _lookup_keys(_query_context("Last week"), []) + second = _lookup_keys(_query_context("Last week"), []) + assert first == second + assert first != _lookup_keys(_query_context("Last month"), []) + + +def test_per_user_caching_key_follows_the_running_user(app_context: None) -> None: + """With per-user caching, the key depends on the user the task runs as.""" + from superset.utils.core import override_user + + query_context = _query_context(database_extra='{"per_user_caching": true}') + engine_spec = query_context.datasource.database.db_engine_spec + engine_spec.get_impersonation_key.side_effect = lambda user: f"user-{user.id}" + + with override_user(MagicMock(id=1), force=True): + first = _lookup_keys(query_context, []) + with override_user(MagicMock(id=2), force=True): + second = _lookup_keys(query_context, []) + with override_user(MagicMock(id=1), force=True): + again = _lookup_keys(query_context, []) + + assert first != second + assert first == again diff --git a/tests/unit_tests/security/audit_log_test.py b/tests/unit_tests/security/audit_log_test.py index 374835775c4a..e67f9aa108fe 100644 --- a/tests/unit_tests/security/audit_log_test.py +++ b/tests/unit_tests/security/audit_log_test.py @@ -269,22 +269,3 @@ def test_on_user_logout_logs_event(mock_log: MagicMock) -> None: mock_log.assert_called_once_with( "UserLoggedOut", {"username": "testuser", "user_id": 7} ) - - -@patch("superset.tasks.cache.schedule_login_cache_warmup") -@patch("superset.security.session_invalidation.stamp_login_time") -@patch("superset.security.manager._log_audit_event") -def test_on_user_login_schedules_cache_warmup( - mock_log: MagicMock, - mock_stamp_login_time: MagicMock, - mock_schedule: MagicMock, -) -> None: - """on_user_login hands the user to the login cache warm-up scheduler.""" - sm = SupersetSecurityManager.__new__(SupersetSecurityManager) - user = MagicMock(spec=User) - user.username = "testuser" - user.id = 7 - - sm.on_user_login(user) - - mock_schedule.assert_called_once_with(user) diff --git a/tests/unit_tests/tasks/test_cache.py b/tests/unit_tests/tasks/test_cache.py index ad9f6a5816ce..3b5a1eff05a5 100644 --- a/tests/unit_tests/tasks/test_cache.py +++ b/tests/unit_tests/tasks/test_cache.py @@ -439,54 +439,49 @@ def test_native_filter_options_strategy_registered() -> None: assert strategy_registry["native_filter_options"] is NativeFilterOptionsStrategy -def test_schedule_login_warmup_flag_off(app_context: None) -> None: - """With the setting disabled nothing is enqueued.""" - from superset.tasks.cache import schedule_login_cache_warmup +def test_favorites_strategy_registered() -> None: + from superset.tasks.cache import FavoritesStrategy, strategy_registry + + assert strategy_registry["favorites"] is FavoritesStrategy + assert FavoritesStrategy.per_user is True + + +def test_favorites_dispatch_disabled(app_context: None) -> None: + """With the gate off, the strategy enqueues nothing and needs no warmup user.""" + from superset.tasks.cache import cache_warmup with ( mock.patch( "superset.tasks.cache.current_app", - _fake_app({"CACHE_WARMUP_ON_LOGIN": False}), + _fake_app({"CACHE_WARMUP_FAVORITES": False}), ), mock.patch("superset.tasks.cache.cache_warmup_user_favorites") as task, + mock.patch("superset.tasks.cache.FavoritesStrategy.get_user_ids") as users, ): - schedule_login_cache_warmup(mock.MagicMock(id=1)) + result = cache_warmup("favorites") - task.delay.assert_not_called() + assert isinstance(result, str) + assert "CACHE_WARMUP_FAVORITES is disabled" in result + task.apply_async.assert_not_called() + users.assert_not_called() -def test_schedule_login_warmup_enqueues_once_per_cooldown(app_context: None) -> None: - """The first login enqueues; a login inside the cooldown does not.""" - from superset.tasks.cache import schedule_login_cache_warmup +def test_favorites_dispatch_enqueues_one_task_per_user(app_context: None) -> None: + from superset.tasks.cache import cache_warmup - config = {"CACHE_WARMUP_ON_LOGIN": True, "CACHE_WARMUP_ON_LOGIN_COOLDOWN": 60} + config = {"CACHE_WARMUP_FAVORITES": True, "CACHE_WARMUP_FAVORITES_QUEUE": "warm"} with ( mock.patch("superset.tasks.cache.current_app", _fake_app(config)), - mock.patch("superset.tasks.cache.cache_manager") as cm, mock.patch("superset.tasks.cache.cache_warmup_user_favorites") as task, + mock.patch( + "superset.tasks.cache.FavoritesStrategy.get_user_ids", return_value=[3, 4] + ), ): - cm.cache.add.return_value = True - schedule_login_cache_warmup(mock.MagicMock(id=7)) - cm.cache.add.return_value = False - schedule_login_cache_warmup(mock.MagicMock(id=7)) - - task.apply_async.assert_called_once_with(args=(7,), queue=None) - + task.apply_async.side_effect = [None, RuntimeError("broker")] + result = cache_warmup("favorites", max_users=2) -def test_schedule_login_warmup_swallows_errors(app_context: None) -> None: - """A broker failure must not propagate into the login flow.""" - from superset.tasks.cache import schedule_login_cache_warmup - - config = {"CACHE_WARMUP_ON_LOGIN": True, "CACHE_WARMUP_ON_LOGIN_COOLDOWN": 60} - with ( - mock.patch("superset.tasks.cache.current_app", _fake_app(config)), - mock.patch("superset.tasks.cache.cache_manager") as cm, - mock.patch("superset.tasks.cache.cache_warmup_user_favorites") as task, - ): - task.apply_async.side_effect = RuntimeError("broker down") - cm.cache.add.return_value = True - schedule_login_cache_warmup(mock.MagicMock(id=7)) - cm.cache.delete.assert_called_once_with("cache-warmup-login:7") + assert result == {"success": ["user:3"], "errors": ["user:4"]} + task.apply_async.assert_any_call(args=(3,), queue="warm") def test_user_favorites_task_warms_stale_charts_only(app_context: None) -> None: @@ -495,7 +490,7 @@ def test_user_favorites_task_warms_stale_charts_only(app_context: None) -> None: chart_a, chart_b = mock.MagicMock(id=1), mock.MagicMock(id=2) dashboard = mock.MagicMock(id=10, slices=[chart_a, chart_b]) - config = {"CACHE_WARMUP_ON_LOGIN_MAX_DASHBOARDS": 3} + config = {"CACHE_WARMUP_FAVORITES_MAX_DASHBOARDS": 3} with ( mock.patch("superset.tasks.cache.current_app", _fake_app(config)), mock.patch( @@ -562,3 +557,26 @@ def raise_for_access(dashboard: Any) -> None: result = _get_favorite_dashboards(5, 2) assert [d.id for d in result] == [3, 1] + + +def test_favorites_strategy_user_selection(app_context: None) -> None: + """Users come back newest login first, limited by max_users, from one query.""" + from superset.tasks.cache import FavoritesStrategy + + with ( + mock.patch("superset.tasks.cache.db") as db, + mock.patch( + "superset.tasks.cache.security_manager", new_callable=mock.MagicMock + ) as sm, + ): + sm.user_model.last_login.__ge__.return_value = True + query = db.session.query.return_value.join.return_value.filter.return_value + chain = query.filter.return_value.group_by.return_value.order_by.return_value + chain.limit.return_value.all.return_value = [ + mock.MagicMock(user_id=9), + mock.MagicMock(user_id=4), + ] + strategy = FavoritesStrategy(since="7 days ago", max_users=2) + assert strategy.get_user_ids() == [9, 4] + + chain.limit.assert_called_once_with(2) From 12da242cbc265e6ffd863dcd367ba327b11a7b25 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 30 Oct 2026 03:25:18 +0000 Subject: [PATCH 5/6] fix(cache): propagate soft time limit and tighten favorites warm-up Co-Authored-By: Claude Sonnet 5.5 --- superset/commands/chart/warm_up_cache.py | 8 +++++++ superset/tasks/cache.py | 11 ++++++++- .../commands/chart/warm_up_cache_test.py | 16 +++++++++++++ tests/unit_tests/tasks/test_cache.py | 24 +++++++++++++++++++ 4 files changed, 58 insertions(+), 1 deletion(-) diff --git a/superset/commands/chart/warm_up_cache.py b/superset/commands/chart/warm_up_cache.py index c92581e05c7a..0170eaf81947 100644 --- a/superset/commands/chart/warm_up_cache.py +++ b/superset/commands/chart/warm_up_cache.py @@ -18,6 +18,8 @@ from typing import Any, cast, Optional, Union +from celery.exceptions import SoftTimeLimitExceeded + from superset.commands.base import BaseCommand from superset.commands.chart.data.get_data_command import ChartDataCommand from superset.commands.chart.exceptions import ( @@ -109,6 +111,10 @@ def _warm_up_non_legacy_cache(self, chart: Slice) -> tuple[Any, Any]: lock_key = f"cache-warmup-lock:{missing[0]}" if not cache_manager.cache.add(lock_key, 1, timeout=WARMUP_LOCK_TTL): return None, QueryStatus.SUCCESS + if not self._missing_cache_keys(query_context): + # Another worker stored the result while this one waited. + cache_manager.cache.delete(lock_key) + return None, QueryStatus.SUCCESS else: query_context.force = True try: @@ -145,6 +151,8 @@ def run(self, *, skip_disabled: bool = False) -> dict[str, Any]: try: error, status = self._warm_up_non_legacy_cache(chart) + except SoftTimeLimitExceeded: + raise except Exception as ex: # pylint: disable=broad-except error = error_msg_from_exception(ex) status = None diff --git a/superset/tasks/cache.py b/superset/tasks/cache.py index 5d0b9c9adb64..9111f2967f05 100644 --- a/superset/tasks/cache.py +++ b/superset/tasks/cache.py @@ -354,7 +354,7 @@ class FavoritesStrategy(Strategy): # pylint: disable=too-few-public-methods def __init__(self, since: str = "7 days ago", max_users: int = 100) -> None: super().__init__() self.since: Any = parse_human_datetime(since) if since else None - self.max_users: int = max_users + self.max_users: int = max(1, int(max_users)) def get_urls(self) -> list[str]: return [] @@ -370,6 +370,7 @@ def get_user_ids(self) -> list[int]: .filter( FavStar.class_name == FavStarClassName.DASHBOARD, security_manager.user_model.active.is_(True), + security_manager.user_model.last_login.isnot(None), ) ) if self.since: @@ -565,6 +566,7 @@ def cache_warmup_user_favorites(user_id: int) -> dict[str, list[int]]: are skipped; only missing or expired results are computed. """ # pylint: disable=import-outside-toplevel + from superset.commands.chart.exceptions import ChartAccessDeniedError from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand from superset.utils.core import override_user @@ -582,6 +584,13 @@ def cache_warmup_user_favorites(user_id: int) -> dict[str, list[int]]: outcome: dict[str, Any] = ChartWarmUpCacheCommand( chart, dashboard.id, None, only_if_stale=True ).run(skip_disabled=True) + except ChartAccessDeniedError: + logger.info( + "Skipping chart %s on dashboard %s: no access", + chart.id, + dashboard.id, + ) + continue except SoftTimeLimitExceeded: logger.warning( "Favorites cache warm-up for user %s timed out", user_id diff --git a/tests/unit_tests/commands/chart/warm_up_cache_test.py b/tests/unit_tests/commands/chart/warm_up_cache_test.py index f2493f7c3c15..7edc6142ef76 100644 --- a/tests/unit_tests/commands/chart/warm_up_cache_test.py +++ b/tests/unit_tests/commands/chart/warm_up_cache_test.py @@ -390,3 +390,19 @@ def test_missing_cache_keys_empty_when_caching_disabled() -> None: query_context = _ctx(["k1"]) query_context.get_cache_timeout.return_value = -1 assert ChartWarmUpCacheCommand._missing_cache_keys(query_context) == [] + + +def test_run_propagates_soft_time_limit() -> None: + """A Celery soft time limit must reach the task instead of becoming a viz error.""" + from celery.exceptions import SoftTimeLimitExceeded + + chart = Mock(spec=Slice) + command = ChartWarmUpCacheCommand(chart, None, None) + with ( + patch.object(command, "validate"), + patch.object( + command, "_warm_up_non_legacy_cache", side_effect=SoftTimeLimitExceeded() + ), + pytest.raises(SoftTimeLimitExceeded), + ): + command.run() diff --git a/tests/unit_tests/tasks/test_cache.py b/tests/unit_tests/tasks/test_cache.py index 3b5a1eff05a5..517221d02726 100644 --- a/tests/unit_tests/tasks/test_cache.py +++ b/tests/unit_tests/tasks/test_cache.py @@ -580,3 +580,27 @@ def test_favorites_strategy_user_selection(app_context: None) -> None: assert strategy.get_user_ids() == [9, 4] chain.limit.assert_called_once_with(2) + + +def test_user_favorites_task_skips_inaccessible_chart(app_context: None) -> None: + """A chart the user cannot access is skipped without counting as an error.""" + from superset.commands.chart.exceptions import ChartAccessDeniedError + from superset.tasks.cache import cache_warmup_user_favorites + + chart = mock.MagicMock(id=1) + dashboard = mock.MagicMock(id=10, slices=[chart]) + config = {"CACHE_WARMUP_FAVORITES_MAX_DASHBOARDS": 3} + with ( + mock.patch("superset.tasks.cache.current_app", _fake_app(config)), + mock.patch( + "superset.tasks.cache.security_manager", new_callable=mock.MagicMock + ), + mock.patch( + "superset.tasks.cache._get_favorite_dashboards", return_value=[dashboard] + ), + mock.patch( + "superset.commands.chart.warm_up_cache.ChartWarmUpCacheCommand" + ) as command, + ): + command.return_value.run.side_effect = ChartAccessDeniedError() + assert cache_warmup_user_favorites(5) == {"success": [], "errors": []} From 4eeb6caadbc2bc66d3f16f7a2032b5258e4fa196 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 30 Oct 2026 03:34:31 +0000 Subject: [PATCH 6/6] fix(cache): reduce warm-up complexity, bound favorites work, correct docs Co-Authored-By: Claude Sonnet 5.5 --- docs/admin_docs/configuration/cache.mdx | 7 ++-- superset/commands/chart/warm_up_cache.py | 33 +++++++++++------ superset/tasks/cache.py | 15 +++++++- .../commands/chart/warm_up_cache_test.py | 24 ++++++------- tests/unit_tests/tasks/test_cache.py | 35 ++++++++++++++++--- 5 files changed, 85 insertions(+), 29 deletions(-) diff --git a/docs/admin_docs/configuration/cache.mdx b/docs/admin_docs/configuration/cache.mdx index 6f724254d347..318f7634989f 100644 --- a/docs/admin_docs/configuration/cache.mdx +++ b/docs/admin_docs/configuration/cache.mdx @@ -318,8 +318,11 @@ Notes: result twice; use a shared backend such as Redis for it to work across processes. - Set `CACHE_WARMUP_FAVORITES_QUEUE` and run a worker for that queue to keep warm-up from competing with reports, alerts and thumbnails. Each per-user task has a 15 minute time limit. -- Dashboard default filters are not applied, so charts whose dashboard load uses different - filters may still miss the cache. +- Dashboard default column filters (native filter defaults and legacy defaults) are applied. + Other dashboard filter settings, such as time range, time column and grain, extras and ad hoc + filters, are not, so charts whose dashboard load uses them may still miss the cache. +- `CACHE_WARMUP_FAVORITES` is an ordinary configuration setting, not a feature flag. +- Queued per-user tasks expire after two hours if no worker picks them up. ## Caching Thumbnails diff --git a/superset/commands/chart/warm_up_cache.py b/superset/commands/chart/warm_up_cache.py index 0170eaf81947..7dde90fe2989 100644 --- a/superset/commands/chart/warm_up_cache.py +++ b/superset/commands/chart/warm_up_cache.py @@ -84,6 +84,27 @@ def _missing_cache_keys(query_context: Any) -> list[str]: missing.append(cache_key or "") return missing + def _acquire_warmup_lock(self, query_context: Any) -> tuple[bool, str | None]: + """ + Decide whether this worker should compute the chart's missing results. + + Returns ``(should_run, lock_key)``. ``should_run`` is False when everything is + cached or another worker holds the lock; otherwise the returned lock key must + be released after the computation. + """ + missing = self._missing_cache_keys(query_context) + if not missing: + return False, None + # Only one worker warms a given result at a time. + lock_key = f"cache-warmup-lock:{missing[0]}" + if not cache_manager.cache.add(lock_key, 1, timeout=WARMUP_LOCK_TTL): + return False, None + if not self._missing_cache_keys(query_context): + # Another worker stored the result while this one waited. + cache_manager.cache.delete(lock_key) + return False, None + return True, lock_key + def _warm_up_non_legacy_cache(self, chart: Slice) -> tuple[Any, Any]: """Warm up cache for non-legacy visualizations.""" query_context = chart.get_query_context() @@ -104,16 +125,8 @@ def _warm_up_non_legacy_cache(self, chart: Slice) -> tuple[Any, Any]: lock_key: str | None = None if self._only_if_stale: - missing = self._missing_cache_keys(query_context) - if not missing: - return None, QueryStatus.SUCCESS - # Only one worker warms a given result at a time. - lock_key = f"cache-warmup-lock:{missing[0]}" - if not cache_manager.cache.add(lock_key, 1, timeout=WARMUP_LOCK_TTL): - return None, QueryStatus.SUCCESS - if not self._missing_cache_keys(query_context): - # Another worker stored the result while this one waited. - cache_manager.cache.delete(lock_key) + should_run, lock_key = self._acquire_warmup_lock(query_context) + if not should_run: return None, QueryStatus.SUCCESS else: query_context.force = True diff --git a/superset/tasks/cache.py b/superset/tasks/cache.py index 9111f2967f05..e72b5c037d66 100644 --- a/superset/tasks/cache.py +++ b/superset/tasks/cache.py @@ -429,7 +429,9 @@ def _dispatch_per_user( queue: str | None = current_app.config.get("CACHE_WARMUP_FAVORITES_QUEUE") for user_id in strategy.get_user_ids(): try: - cache_warmup_user_favorites.apply_async(args=(user_id,), queue=queue) + cache_warmup_user_favorites.apply_async( + args=(user_id,), queue=queue, expires=FAVORITES_TASK_EXPIRES + ) results["success"].append(f"user:{user_id}") except Exception: # noqa: BLE001 logger.exception("Error enqueueing cache warm-up for user %s", user_id) @@ -519,6 +521,12 @@ def cache_warmup( return results +# Seconds after which an unstarted per-user task is discarded. +FAVORITES_TASK_EXPIRES = 7200 +# Favorites read per wanted dashboard, leaving room for inaccessible ones. +FAVORITES_FETCH_FACTOR = 4 + + def _get_favorite_dashboards(user_id: int, limit: int) -> list[Dashboard]: """Return the user's most recently favorited dashboards, newest first.""" dashboard_ids: list[int] = [ @@ -529,6 +537,8 @@ def _get_favorite_dashboards(user_id: int, limit: int) -> list[Dashboard]: FavStar.class_name == FavStarClassName.DASHBOARD, ) .order_by(FavStar.dttm.desc()) + # Some favorites may be inaccessible or deleted, so read a bounded surplus. + .limit(limit * FAVORITES_FETCH_FACTOR) .all() ] dashboards: list[Dashboard] = [] @@ -571,6 +581,9 @@ def cache_warmup_user_favorites(user_id: int) -> dict[str, list[int]]: from superset.utils.core import override_user results: dict[str, list[int]] = {"success": [], "errors": []} + if not current_app.config.get("CACHE_WARMUP_FAVORITES"): + logger.warning("CACHE_WARMUP_FAVORITES is disabled; skipping user %s", user_id) + return results user: Any = security_manager.get_user_by_id(user_id) if not user: logger.warning("Favorites cache warm-up: user %s not found", user_id) diff --git a/tests/unit_tests/commands/chart/warm_up_cache_test.py b/tests/unit_tests/commands/chart/warm_up_cache_test.py index 7edc6142ef76..8eb8fe8a4c87 100644 --- a/tests/unit_tests/commands/chart/warm_up_cache_test.py +++ b/tests/unit_tests/commands/chart/warm_up_cache_test.py @@ -220,12 +220,12 @@ def test_handles_empty_dashboard_filters( ChartWarmUpCacheCommand(chart, 42, None).run() # VALIDATE: No filters added (empty list case) - assert ( - len(mock_query.filter) == 0 - ), "No filters should be added when dashboard has no filters" - assert ( - mock_get_dashboard_filters.called - ), "Should still call get_dashboard_extra_filters" + assert len(mock_query.filter) == 0, ( + "No filters should be added when dashboard has no filters" + ) + assert mock_get_dashboard_filters.called, ( + "Should still call get_dashboard_extra_filters" + ) @patch("superset.commands.chart.warm_up_cache.ChartDataCommand") @@ -280,12 +280,12 @@ def test_none_query_context_raises_chart_invalid_error(mock_chart_data_command): assert result["viz_error"] is not None, "Should return an error" assert result["chart_id"] == 129 error_str = str(result["viz_error"]).lower() - assert ( - "query context" in error_str - ), f"Error should mention query context: {result['viz_error']}" - assert ( - "not exist" in error_str - ), f"Error should mention not exist: {result['viz_error']}" + assert "query context" in error_str, ( + f"Error should mention query context: {result['viz_error']}" + ) + assert "not exist" in error_str, ( + f"Error should mention not exist: {result['viz_error']}" + ) @patch("superset.commands.chart.warm_up_cache.ChartDataCommand") diff --git a/tests/unit_tests/tasks/test_cache.py b/tests/unit_tests/tasks/test_cache.py index 517221d02726..94d1e211ab7f 100644 --- a/tests/unit_tests/tasks/test_cache.py +++ b/tests/unit_tests/tasks/test_cache.py @@ -481,7 +481,7 @@ def test_favorites_dispatch_enqueues_one_task_per_user(app_context: None) -> Non result = cache_warmup("favorites", max_users=2) assert result == {"success": ["user:3"], "errors": ["user:4"]} - task.apply_async.assert_any_call(args=(3,), queue="warm") + task.apply_async.assert_any_call(args=(3,), queue="warm", expires=7200) def test_user_favorites_task_warms_stale_charts_only(app_context: None) -> None: @@ -490,7 +490,10 @@ def test_user_favorites_task_warms_stale_charts_only(app_context: None) -> None: chart_a, chart_b = mock.MagicMock(id=1), mock.MagicMock(id=2) dashboard = mock.MagicMock(id=10, slices=[chart_a, chart_b]) - config = {"CACHE_WARMUP_FAVORITES_MAX_DASHBOARDS": 3} + config = { + "CACHE_WARMUP_FAVORITES": True, + "CACHE_WARMUP_FAVORITES_MAX_DASHBOARDS": 3, + } with ( mock.patch("superset.tasks.cache.current_app", _fake_app(config)), mock.patch( @@ -514,10 +517,28 @@ def test_user_favorites_task_warms_stale_charts_only(app_context: None) -> None: assert result == {"success": [1], "errors": [2]} +def test_user_favorites_task_disabled(app_context: None) -> None: + from superset.tasks.cache import cache_warmup_user_favorites + + with ( + mock.patch( + "superset.tasks.cache.current_app", + _fake_app({"CACHE_WARMUP_FAVORITES": False}), + ), + mock.patch("superset.tasks.cache._get_favorite_dashboards") as favs, + ): + assert cache_warmup_user_favorites(5) == {"success": [], "errors": []} + favs.assert_not_called() + + def test_user_favorites_task_unknown_user(app_context: None) -> None: from superset.tasks.cache import cache_warmup_user_favorites with ( + mock.patch( + "superset.tasks.cache.current_app", + _fake_app({"CACHE_WARMUP_FAVORITES": True}), + ), mock.patch( "superset.tasks.cache.security_manager", new_callable=mock.MagicMock ) as sm, @@ -548,7 +569,8 @@ def raise_for_access(dashboard: Any) -> None: sm.raise_for_access.side_effect = raise_for_access query = db.session.query.return_value # Favorites query (newest first), then dashboards lookup. - query.filter.return_value.order_by.return_value.all.return_value = [ + favs = query.filter.return_value.order_by.return_value.limit + favs.return_value.all.return_value = [ mock.MagicMock(obj_id=i) for i in (3, 2, 1, 4) ] query.options.return_value.filter.return_value.all.return_value = list( @@ -556,6 +578,8 @@ def raise_for_access(dashboard: Any) -> None: ) result = _get_favorite_dashboards(5, 2) + favs.assert_called_once_with(8) + assert [d.id for d in result] == [3, 1] @@ -589,7 +613,10 @@ def test_user_favorites_task_skips_inaccessible_chart(app_context: None) -> None chart = mock.MagicMock(id=1) dashboard = mock.MagicMock(id=10, slices=[chart]) - config = {"CACHE_WARMUP_FAVORITES_MAX_DASHBOARDS": 3} + config = { + "CACHE_WARMUP_FAVORITES": True, + "CACHE_WARMUP_FAVORITES_MAX_DASHBOARDS": 3, + } with ( mock.patch("superset.tasks.cache.current_app", _fake_app(config)), mock.patch(