Skip to content

fix(cache): dedupe annotation-layer data cache keys across users - #44402

Closed
eschutho wants to merge 3 commits into
masterfrom
annotation-cache-key-fanout
Closed

eschutho wants to merge 3 commits into
masterfrom
annotation-cache-key-fanout

Conversation

@eschutho

Copy link
Copy Markdown
Member

SUMMARY

The problem. When a chart has annotation layers, Superset was saving a separate copy of that chart's cached data for every user who viewed it — even when all of them would see exactly the same result. A single chart could turn into hundreds of identical copies in the cache, and since each copy can be tens of megabytes, this wasted a large amount of cache memory for no benefit.

Why it happened. Annotations (the extra markers/notes drawn on a chart) are loaded using the viewer's own permissions, and they get stored in the same cache entry as the chart's data. To make sure a user could never be served annotation data they weren't allowed to see, the code mixed the viewer's user ID into the cache key. That was safe, but too blunt: because the user ID is part of the key, every user got their own copy — including users who would see identical data.

The fix. Instead of keying the cache on who the user is, we now key it on what the user is allowed to see (their access scope). Users with the same access share one cache entry; users with different access — or no access — never share one. This keeps the exact same safety guarantee (nobody is served annotation data they shouldn't see) while letting identical results be reused instead of duplicated. Concretely:

  • Built-in ("native") annotation layers show global annotation records that are gated only by the "can read annotations" permission, so the key now includes just that permission flag.
  • Chart-based annotation layers (annotations pulled from another chart) run a query against that chart's data source. The key now includes (a) whether the user can access that data source — previously this access check only ran when the data was missing from the cache, not when it was served from the cache — and (b) that other chart's own cache key, which already accounts for the data source version, row-level security (RLS) rules, and any per-user RLS logic.

So users with genuinely different access still get their own cache entries; users with the same access now share one.

Second, related change: size cap. Superset can already skip caching values that are too large (DATA_CACHE_MAX_VALUE_SIZE), but a few code paths wrote to the DATA cache directly and skipped that check: the SQL executor's result cache, and two datasource endpoints (filter-dropdown values, and metrics/dimensions). This PR sends those through the same size check so oversized values can't slip in there either. It does nothing when the cap is turned off (the default), so it adds no overhead by default.

Expiry (TTL) note. Every cache write touched here already sets an expiry time. The special timeout=0 ("never expire") value is an intentional, documented feature and is left unchanged.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — this is a caching / cache-key change with no UI surface.

TESTING INSTRUCTIONS

Unit tests (added/updated, verified to fail before the fix and pass after):

pytest tests/unit_tests/common/test_query_context_processor.py -k annotation_cache_key
pytest tests/unit_tests/utils/cache_test.py
  • The annotation cache-key tests check that identical annotation-layer results are shared across users with the same access, and still stay separate when the annotation-read permission, datasource access, or RLS genuinely differs.
  • The cache-util tests check that the shared size guard skips oversized values (and counts skip_cache_value_too_large), and does nothing when the cap is disabled.

Manual check: open a chart with annotation layers as two users who share the same role and RLS, and confirm only one cached data entry is created (not one per user). Then view it as a user whose access or RLS differs, and confirm they get their own separate entry.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
  • Introduces new feature or API
  • Removes existing feature or API
  • Bug fix (non-breaking change which fixes an issue)

🤖 Generated with Claude Code

When a chart has annotation layers, the DATA cache key bound the raw
requesting user id, forcing one cache copy per user even when they would
see identical annotation data. Bind the annotation *security scope*
instead: NATIVE layers bind the `can_read` Annotation permission;
chart-backed layers bind datasource access plus the annotation chart's
own query cache key (which folds in datasource version, RLS clauses, and
per-user Jinja/virtual-dataset RLS). Fall back closed on SupersetException.

Also route three direct data_cache.set writers (SQL executor result
cache, datasource column-values and metrics/dimensions endpoints) through
a shared size-cap wrapper so oversized values can't bypass
DATA_CACHE_MAX_VALUE_SIZE.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 16.66667% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.27%. Comparing base (4c3a890) to head (9e447db).
⚠️ Report is 22 commits behind head on master.

Files with missing lines Patch % Lines
superset/common/query_context_processor.py 3.70% 26 Missing ⚠️
superset/utils/cache.py 31.25% 8 Missing and 3 partials ⚠️
superset/sql/execution/executor.py 0.00% 2 Missing ⚠️
superset/datasource/api.py 66.66% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (4c3a890) and HEAD (9e447db). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (4c3a890) HEAD (9e447db)
python 8 5
postgres 2 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #44402       +/-   ##
===========================================
- Coverage   80.41%   66.27%   -14.15%     
===========================================
  Files        2932     2932               
  Lines      174621   174814      +193     
  Branches    40559    40600       +41     
===========================================
- Hits       140422   115854    -24568     
- Misses      31532    56517    +24985     
+ Partials     2667     2443      -224     
Flag Coverage Δ
hive 37.29% <8.33%> (-0.05%) ⬇️
mysql 56.55% <16.66%> (-0.04%) ⬇️
postgres 56.57% <16.66%> (-0.04%) ⬇️
presto 39.18% <12.50%> (-0.05%) ⬇️
python 56.87% <16.66%> (-27.91%) ⬇️
sqlite 56.27% <16.66%> (-0.04%) ⬇️
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

eschutho and others added 2 commits September 17, 2026 21:59
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The chart-backed annotation cache-key bound access via
can_access_datasource (form_data=None), which under-approximates the
actual fetch gate. get_viz_annotation_data validates the annotation
chart's query context, whose raise_for_access also honors
has_promiscuous_chart_access (VIEWER_PROMISCUOUS_MODE) and guest-token
scopes. Under those configs a promiscuous-granted viewer and a
truly-denied user both fail can_access_datasource and would collapse onto
one {access:False} entry, letting the denied user read the viewer's
cached annotation payload (the real gate only runs on a cache miss).

Bind access on the same raise_for_access(query_context=...) the fetch
performs, keeping the except SupersetException fail-closed fallback.

Also add focused tests for set_data_cache_if_within_size /
oversized_data_cache_value.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rusackas

Copy link
Copy Markdown
Member

Heya @eschutho, small world. I hit this same root cause independently and opened #44406 before seeing this one.

Converged on your access-scope idea (can_read/Annotation, can_access_datasource, reusing the referenced chart's own cache key) over the plain user_id + RLS-clause key I started with, credited over there. Main difference: this keeps the dataframe and annotation payload on one combined entry re-scoped by access class, mine splits them so the dataframe stays one shared entry regardless of how many access classes view the chart. We're touching the same file and test suite too, so whichever lands second needs a rebase.

Should we reconcile onto one of these before either merges?

@eschutho

Copy link
Copy Markdown
Member Author

I'll close this one @rusackas

@eschutho eschutho closed this Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants