Skip to content

Make CacheConfig() the Client cache default and None the off switch#3164

Merged
maxisbey merged 1 commit into
mainfrom
cache-config-none-disables
Jul 25, 2026
Merged

Make CacheConfig() the Client cache default and None the off switch#3164
maxisbey merged 1 commit into
mainfrom
cache-config-none-disables

Conversation

@maxisbey

Copy link
Copy Markdown
Contributor

Client.cache was typed CacheConfig | Literal[False] | None, a three-state sentinel where None meant "use the default config" and False meant "disabled". This collapses it to CacheConfig | None with field(default_factory=CacheConfig): the default CacheConfig() is cache-on, and None turns caching off.

Motivation and Context

The None-as-auto sentinel exists to avoid sharing a default instance, but that concern isn't real here: CacheConfig is a frozen dataclass and store=None already means "build a fresh InMemoryResponseCacheStore per client", so the config carries no per-client state. With that gone, None can mean what it means everywhere else (no cache), and the if self.cache is not False: config = self.cache if self.cache is not None else CacheConfig() dance in __post_init__ collapses to if self.cache is not None.

How Has This Been Tested?

Updated the existing caching, subscription and docs tests for the new spelling (cache=None in place of cache=False); exercised default / CacheConfig() / None end-to-end against an in-process server with a ttlMs hint.

Breaking Changes

cache=False is now cache=None. The response cache is new on main (SEP-2549, not on v1.x), so there's no released API to migrate.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

The cache field was a three-state CacheConfig | Literal[False] | None where
None meant "use the default config" and False meant "disabled". Since
CacheConfig is a frozen dataclass and the store is instantiated per client,
a plain CacheConfig() default is inert to share, so the sentinel bought
nothing. Default to CacheConfig() and let None disable caching.
@maxisbey
maxisbey marked this pull request as ready for review July 25, 2026 16:39
@github-actions

Copy link
Copy Markdown
Contributor

📚 Documentation preview

Preview https://pr-3164.mcp-python-docs.pages.dev
Deployment https://bb6f0303.mcp-python-docs.pages.dev
Commit 63eaad5
Triggered by @maxisbey
Updated 2026-07-25 16:40:54 UTC

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found across 5 files

Re-trigger cubic

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — a clean three-state-to-two-state simplification of the Client.cache parameter.

What was reviewed:

  • The __post_init__ collapse (is not False dance → is not None) preserves all three behaviors: default-on, custom CacheConfig, and off — each still pinned by a test.
  • Checked whether sharing a default instance via default_factory=CacheConfig could leak state between clients — CacheConfig is frozen and store=None builds a per-client store, so it cannot (and the factory makes a fresh instance per client regardless).
  • Grepped the whole repo for leftover cache=False spellings — none remain in src, tests, or docs; Literal stays imported for ConnectMode.
  • Confirmed no docs/migration.md entry is needed: the cache is new on main and unreleased, matching the PR description.
Extended reasoning...

Overview

The PR narrows Client.cache from CacheConfig | Literal[False] | None (where None meant default-on and False meant off) to CacheConfig | None with field(default_factory=CacheConfig), so the default is an explicit CacheConfig() and None is the single off switch. The substantive change is confined to two hunks in src/mcp/client/client.py (the field declaration + docstring, and the __post_init__ guard); the remaining four files are mechanical cache=Falsecache=None renames in tests and docs.

Security risks

None introduced. The cache's security-relevant machinery — partition isolation, userinfo stripping, sha256-hashing of server identity, the custom-store target_id guard, and share_public gating — is untouched; the guard block that enforces them is byte-identical apart from the simplified entry condition. The one theoretical concern with a default_factory default (shared mutable state across clients) does not apply: CacheConfig is a frozen, slotted dataclass and store=None means a fresh per-client InMemoryResponseCacheStore, exactly as the PR description argues.

Level of scrutiny

Moderate: this is public-API surface on a client library, but the behavior change is a construction-time parameter spelling with no runtime-logic change. Since the response cache is new on main (SEP-2549) and not on the released v1.x line, there is no released API being broken, which also explains the absence of a docs/migration.md entry — consistent with the repo's guideline that migration docs cover changes to shipped APIs.

Other factors

Coverage is thorough: dedicated tests pin all three states (default-on with a per-client in-memory store, explicit CacheConfig, and cache=None disabling both the cache and the message-handler wrap), and the docs test suite proves the updated docs/client/caching.md claims against the real SDK. A repo-wide grep confirms no stale cache=False call sites or Literal[False] cache annotations remain. The bug hunting system reported no findings, the PR timeline has no outstanding reviewer comments, and the change matches the description exactly.

@maxisbey
maxisbey merged commit 0cb920f into main Jul 25, 2026
39 checks passed
@maxisbey
maxisbey deleted the cache-config-none-disables branch July 25, 2026 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant