Skip to content

fix(caching): make cache_disabled context-local using ContextVar - #1989

Open
Aryan-Pardeshi wants to merge 1 commit into
dottxt-ai:mainfrom
Aryan-Pardeshi:fix/cache-disabled-contextvar
Open

fix(caching): make cache_disabled context-local using ContextVar#1989
Aryan-Pardeshi wants to merge 1 commit into
dottxt-ai:mainfrom
Aryan-Pardeshi:fix/cache-disabled-contextvar

Conversation

@Aryan-Pardeshi

Copy link
Copy Markdown

Fixes #1983

Context

\ cache_disabled()\ directly mutated the global _caching_enabled\ boolean. In asynchronous environments, calling \with cache_disabled():\ in one task inadvertently disabled caching globally for concurrent tasks running in the same process loop.

Changes

  • Added a \ContextVar[Optional[bool]]\ (_caching_enabled_override) defaulting to \None.
  • Implemented _is_caching_enabled()\ to check the context-local override first, falling back to global _caching_enabled\ if \None.
  • Updated \cache_disabled()\ context manager to set and reset the \ContextVar\ token.
  • Retained global _caching_enabled\ so process-level switches like \disable_cache()\ continue to apply across worker threads.
  • Added unit tests in \ ests/test_cache.py\ covering async task isolation and thread behavior.

@harsh4vardhan

Copy link
Copy Markdown

The ContextVar approach is exactly right for this, and the two-level design (_caching_enabled_override + fallback to global _caching_enabled) cleanly preserves the intended semantics: disable_cache() remains process-wide, cache_disabled() becomes context-local. Token-based set/reset handles nesting correctly too.

One thing worth documenting: asyncio.create_task() called inside a cache_disabled() block will spawn a task that inherits _caching_enabled_override = False - standard ContextVar propagation means the child sees the parent's context at spawn time. That may be the desired behavior (subtasks honor the same override), but it is potentially surprising. A sentence in the docstring would help users reason about it.

The async decorator path still carries # pragma: no cover. The new async tests hit _is_caching_enabled() correctly, but the if not _is_caching_enabled() guard inside the async wrapper itself stays excluded from the coverage report. Not a blocker, but you may want to remove the pragma or add a direct test through @cache on an async def function.

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.

caching: cache_disabled() context manager is not safe in asyncio contexts

2 participants