Skip to content

feat(mcp): Caching uses in-memory store by default when no external store is configured - #36527

Merged
Antonio-RiveroMartnez merged 1 commit into
apache:masterfrom
Antonio-RiveroMartnez:optional_store
Dec 11, 2025
Merged

feat(mcp): Caching uses in-memory store by default when no external store is configured#36527
Antonio-RiveroMartnez merged 1 commit into
apache:masterfrom
Antonio-RiveroMartnez:optional_store

Conversation

@Antonio-RiveroMartnez

@Antonio-RiveroMartnez Antonio-RiveroMartnez commented Dec 11, 2025

Copy link
Copy Markdown
Member

User description

SUMMARY

This PR allows us to use in-memory caching which is the default behavior in fastMCP by removing the hard requirement of having an external Redis store.

Also, added more info in the config section so it's clear how caching and the store work

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

CodeAnt-AI Description

Use in-memory cache by default when no external store is configured

What Changed

  • When caching is enabled but no external store is configured, MCP now uses FastMCP's built-in in-memory cache instead of requiring Redis.
  • If the optional store config is enabled, caching will use Redis and requires a cache key prefix; if the prefix is missing the code logs a warning and falls back to in-memory caching.
  • Configuration documentation updated to show three clear scenarios: caching disabled, caching with in-memory store (default), and caching with Redis store; unit tests updated to cover the new fallback behaviors.

Impact

✅ Fewer runtime failures when Redis is not configured
✅ Clearer cache configuration for in-memory vs Redis scenarios
✅ Consistent caching behavior when external store is optional

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

@codeant-ai-for-open-source

Copy link
Copy Markdown
Contributor

CodeAnt AI is reviewing your PR.

@dosubot dosubot Bot added the infra:caching Infra setup and configuration related to caching label Dec 11, 2025
@codeant-ai-for-open-source codeant-ai-for-open-source Bot added the size:L This PR changes 100-499 lines, ignoring generated files label Dec 11, 2025
@codeant-ai-for-open-source

Copy link
Copy Markdown
Contributor

Nitpicks 🔍

🔒 No security issues identified
⚡ Recommended areas for review

  • Missing cache prefix enforcement
    If consumers enable MCP_STORE_CONFIG (Redis) but leave MCP_CACHE_CONFIG["CACHE_KEY_PREFIX"] as None, different features using the same Redis instance can conflict. The config currently allows a None prefix which risks key collisions or unexpected shared state. The code that creates/uses the store should validate or derive a safe default prefix when the store is enabled.

  • Store enabled without Redis URL
    The new config permits enabling the shared store via MCP_STORE_CONFIG["enabled"]. If a user enables the store but does not set "CACHE_REDIS_URL", the runtime code that instantiates a Redis client may fail. There should be a validation/warning and a clear fallback (e.g., refuse to enable or fall back to in-memory).

  • Possible AttributeError
    The code reads nested config objects via variables like store_config = flask_app.config.get("MCP_STORE_CONFIG", {})
    and later calls store_config.get("enabled", False). If the config key exists but its value is None
    this will assign None to store_config and the subsequent .get call will raise an AttributeError.
    Consider defensive handling for config values that are present but falsy/None.

  • Wrapper type import risk
    The WRAPPER_TYPE string points to an import path for a wrapper class. If that path is incorrect or the package is not installed, runtime import will fail. Ensure code that uses WRAPPER_TYPE attempts to import it with a clear error message and fallbacks.

  • Missing settings assertions
    The new tests assert that middleware is created and that cache_storage is None when falling back to in-memory store, but they do not assert that the per-operation settings (e.g., list_tools_settings) are passed through to the middleware. This leaves a gap where caching settings might be ignored while tests still pass.

  • App context path untested
    All new tests patch flask.has_app_context to return True. The branch where no app context exists (so the code pushes one via flask_app.app_context()) is not exercised by these new tests. The context-push path could behave differently and should be covered.

  • Brittle config mocking
    The tests configure mock_flask_app.config.get with a side_effect that looks up keys in mock_configs.
    This is fine, but it relies on the signature get(key, default=None) and returning default when key missing. If production code ever uses different keys or extra calls (e.g., without default), the tests may behave differently than a real Flask config dict. Consider using a dict-like object or setting mock_flask_app.config to a real dict to more closely mimic Flask behavior.

Comment thread superset/mcp_service/caching.py
Comment thread superset/mcp_service/caching.py
Comment thread tests/unit_tests/mcp_service/test_mcp_caching.py
@codeant-ai-for-open-source

Copy link
Copy Markdown
Contributor

CodeAnt AI finished reviewing your PR.

@codecov

codecov Bot commented Dec 11, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.77%. Comparing base (1bde06b) to head (b5182d4).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
superset/mcp_service/caching.py 0.00% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #36527       +/-   ##
===========================================
+ Coverage        0   67.77%   +67.77%     
===========================================
  Files           0      637      +637     
  Lines           0    47101    +47101     
  Branches        0     5131     +5131     
===========================================
+ Hits            0    31923    +31923     
- Misses          0    13901    +13901     
- Partials        0     1277     +1277     
Flag Coverage Δ
hive 43.50% <0.00%> (?)
mysql 66.84% <0.00%> (?)
postgres 66.89% <0.00%> (?)
presto 47.14% <0.00%> (?)
python 67.74% <0.00%> (?)
sqlite 66.60% <0.00%> (?)
unit 100.00% <ø> (?)

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

☔ View full report in Codecov by Sentry.
📢 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.

@kgabryje kgabryje left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Antonio-RiveroMartnez
Antonio-RiveroMartnez merged commit dba3fdf into apache:master Dec 11, 2025
100 of 102 checks passed
aminghadersohi pushed a commit to aminghadersohi/superset that referenced this pull request Jan 17, 2026
aminghadersohi pushed a commit to aminghadersohi/superset that referenced this pull request Jan 24, 2026
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infra:caching Infra setup and configuration related to caching size/L size:L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants