codeant-ai-for-open-source[bot] commented on code in PR #38576:
URL: https://github.com/apache/superset/pull/38576#discussion_r3482109723
##########
superset/tasks/thumbnails.py:
##########
@@ -90,6 +93,9 @@ def cache_dashboard_thumbnail(
dashboard = Dashboard.get(dashboard_id)
url = get_url_path("Superset.dashboard", dashboard_id_or_slug=dashboard.id)
+ screenshot = DashboardScreenshot(url, dashboard.digest)
+ resolved_cache_key = cache_key or screenshot.get_cache_key(window_size,
thumb_size)
Review Comment:
**Suggestion:** `DashboardScreenshot` is instantiated before
`override_user(user)`, so `dashboard.digest` is computed without the executor
user context. For `THUMBNAIL_EXECUTORS = [CURRENT_USER]` this can resolve to
`None` in Celery, and the fallback `resolved_cache_key` is then derived from a
non-user-aware digest, causing incorrect cache keys (including possible
cross-dashboard collisions) when callers don't pass `cache_key` (for example
`Dashboard.update_thumbnail(force=True)`). Move screenshot/digest-derived
cache-key resolution inside the `override_user(user)` block (or compute digest
using the selected executor explicitly) so the key is generated under the same
user context used for rendering. [cache]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ SQLA dashboard thumbnail listeners write thumbnails under shared cache
key.
- ⚠️ Dashboard update_thumbnail caches unused by thumbnail API.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable thumbnail listeners so dashboard saves trigger background
thumbnails: in
`superset/models/dashboard.py:471-474` the `update_thumbnail` SQLA listener
is registered
when `THUMBNAILS_SQLA_LISTENERS` is enabled, and
`Dashboard.update_thumbnail()` at
`superset/models/dashboard.py:352-357` calls
`cache_dashboard_thumbnail.delay(current_user=get_current_user(),
dashboard_id=self.id,
force=True)` with no `cache_key`.
2. A Celery worker picks up the task and executes
`cache_dashboard_thumbnail` in
`superset/tasks/thumbnails.py:77-85`; inside it, the dashboard is loaded
(`Dashboard.get(dashboard_id)`) and `screenshot = DashboardScreenshot(url,
dashboard.digest)` is instantiated at `superset/tasks/thumbnails.py:93-96`
before any
`override_user(user)` is applied, so Flask `g.user` is still unset in the
worker.
3. Accessing `dashboard.digest` in that call invokes the `digest` property in
`superset/models/dashboard.py:229-231`, which delegates to
`get_dashboard_digest` in
`superset/thumbnails/digest.py:80-43`; there `get_executor` is called with
`current_user=get_current_user()` (`superset/tasks/utils.py:53-58`), but in
the Celery
context `get_current_user()` returns `None`, `THUMBNAIL_EXECUTORS` is
`[ExecutorType.CURRENT_USER]` (`superset/config.py:1108`), so `get_executor`
fails to find
an executor and `ExecutorNotFoundError` is caught, causing
`get_dashboard_digest` to
return `None` and thus `dashboard.digest` is `None` in the worker.
4. Back in `cache_dashboard_thumbnail`, `resolved_cache_key = cache_key or
screenshot.get_cache_key(window_size, thumb_size)` at
`superset/tasks/thumbnails.py:96-98`
now computes a fallback cache key because `cache_key` was not provided by
`Dashboard.update_thumbnail`; `DashboardScreenshot.get_cache_key`
(`superset/utils/screenshots.py:399-31`) hashes only `thumbnail_type`,
`digest` (now
`None`), `window_size`, `thumb_size`, and `permalink_key`—it does not include
`dashboard_id`—so multiple dashboards updated via the SQLA listener share
the same
fallback cache key in Celery, leading to cross-dashboard cache collisions
and cache
entries that do not match the user-specific digest-based keys generated in
`DashboardRestApi.thumbnail` (`superset/dashboards/api.py:2-21`), where
`DashboardScreenshot(dashboard_url, dashboard.digest)` is instantiated under
the correct
`current_user` context and `cache_key` is passed explicitly to the task.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=77c60348f531449ca0186dac335fb065&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=77c60348f531449ca0186dac335fb065&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/tasks/thumbnails.py
**Line:** 96:97
**Comment:**
*Cache: `DashboardScreenshot` is instantiated before
`override_user(user)`, so `dashboard.digest` is computed without the executor
user context. For `THUMBNAIL_EXECUTORS = [CURRENT_USER]` this can resolve to
`None` in Celery, and the fallback `resolved_cache_key` is then derived from a
non-user-aware digest, causing incorrect cache keys (including possible
cross-dashboard collisions) when callers don't pass `cache_key` (for example
`Dashboard.update_thumbnail(force=True)`). Move screenshot/digest-derived
cache-key resolution inside the `override_user(user)` block (or compute digest
using the selected executor explicitly) so the key is generated under the same
user context used for rendering.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38576&comment_hash=cc7787dd46d973d6f766389eca3a335ba5c222c866b05eaca64435e7c02a25d9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38576&comment_hash=cc7787dd46d973d6f766389eca3a335ba5c222c866b05eaca64435e7c02a25d9&reaction=dislike'>👎</a>
--
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]
