fix(wren): bound the session-context cache to 32 LRU entries - #2628
fix(wren): bound the session-context cache to 32 LRU entries#2628ttw225 wants to merge 1 commit into
Conversation
get_session_context is keyed on the per-query extracted manifest, so an unbounded functools.cache grows one SessionContext per distinct table subset for the life of the process. Use lru_cache(maxsize=32).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesSession context cache
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
get_session_contextusedfunctools.cache, and its cache key includes the per-query extracted manifest. As a long-lived process encountered distinct extracted manifests, the cache retained oneSessionContextfor each argument tuple for the lifetime of the process.This PR replaces it with
lru_cache(maxsize=32). The cache now retains at most the 32 most recently used session contexts; an evicted entry is rebuilt when its argument tuple is used again.What failure does this repair?
The previous cache had no entry-count bound.
WrenEngine._plan()passes the per-query extracted sub-manifest through:extract_by(tables) → effective_manifest → get_session_context(...)On the unfixed code, calling
get_session_contextwith N distinctmanifest_strvalues growscache_info().currsizeto N, whilecache_info().maxsizeremainsNone. Each entry retains aSessionContextand its analyzed manifest state, so varied workloads can accumulate retained contexts indefinitely.The value 32 is an explicit initial policy: it establishes a hard bound while keeping recently used planning contexts warm. It is not derived from production memory or cache-hit measurements. Configurability can be added later if operational evidence shows that a different bound is needed.
The runtime-ownership prerequisite shipped in wren-core-py v0.7.2 via #2510: session contexts use a process-wide runtime rather than owning independent runtimes. Evicting a cached context therefore does not tear down a context-specific worker pool.
Cache eviction removes only the cache's reference. An in-flight caller retains its local reference until the operation completes. As with
functools.cache,lru_cacheis internally synchronized; concurrent misses may briefly build the same value more than once, but only one entry is retained for that key.How is it tested?
tests/unit/test_session_context_cache.pyuses the decorated production function while replacing the nativeSessionContextconstructor with a fake:test_cache_is_bounded_to_32pins the capacity against the test-sideEXPECTED_MAXSIZE = 32and verifies thatcurrsizestops at the bound.test_cache_evicts_least_recently_usedverifies that access refreshes recency, the surviving key returns the same instance, and the evicted key is rebuilt as a different instance.manifest_str, matching the productionextract_by → effective_manifest → get_session_contextpath.The regression tests were also verified against the unfixed
@cachedecorator: the capacity assertion reportsmaxsize=None, and the eviction assertion fails because the supposedly evicted key still returns its original instance.Verification:
Summary by CodeRabbit
Performance
Documentation
Tests