fix: serialise summary-cache fingerprint read-compare-write under a module lock - #158
fix: serialise summary-cache fingerprint read-compare-write under a module lock#158clean6378-max-it wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe summary cache now serializes fingerprinting and cache I/O with a module-level lock, adds double-checked builders for four cached artifacts, updates workspace services to use them, and adds concurrent-access regression tests. ChangesSummary cache concurrency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant list_workspace_projects
participant get_or_build_cached_projects
participant fingerprint_workspace_storage
participant CacheFile
Request->>list_workspace_projects: request workspace projects
list_workspace_projects->>get_or_build_cached_projects: provide workspace data and build_fn
get_or_build_cached_projects->>fingerprint_workspace_storage: compute storage fingerprint
get_or_build_cached_projects->>CacheFile: read cached projects under lock
get_or_build_cached_projects->>list_workspace_projects: invoke build_fn on cache miss
list_workspace_projects-->>get_or_build_cached_projects: return built projects
get_or_build_cached_projects->>CacheFile: recheck and write cache under lock
get_or_build_cached_projects-->>list_workspace_projects: return cached or built projects
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
services/summary_cache.py (1)
263-277: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffGlobal lock also serializes fingerprint stat I/O and JSON disk I/O across unrelated workspaces.
Each critical section runs
_workspace_storage_fingerprint(stats every workspace entry, global DB, CLI chats dir) plus the cache-file read/write while holding the single module-wide lock, and the fingerprint is recomputed twice per miss. Under threaded WSGI this makes concurrent requests for different workspaces block on each other's filesystem I/O. Correct, but consider a per-cache-key lock (dict of locks keyed by cache file / workspace id) as a follow-up if listing latency regresses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/summary_cache.py` around lines 263 - 277, The summary-cache path around _workspace_storage_fingerprint and _get_cached_projects_unlocked holds the module-wide lock during expensive fingerprint and cache-file I/O, and recomputes the fingerprint twice on misses. Refactor the synchronization so unrelated workspace/cache keys do not serialize these operations, using per-cache-key locking or an equivalent keyed coordination mechanism while preserving double-checking and preventing duplicate cache writes for the same key.services/workspace_tabs.py (1)
529-545: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNocache branch re-inlines the builder body instead of reusing
build(). In both call sites the early-return body is character-for-character thebuild()closure, so future argument changes must be made twice.
services/workspace_tabs.py#L529-L545: definebuild()first, thenif nocache_enabled(request_nocache=nocache): return build().services/workspace_db.py#L443-L454: likewise move thebuild()definition above thenocache_enabledcheck and returnbuild().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/workspace_tabs.py` around lines 529 - 545, The nocache paths duplicate the cached builder logic instead of reusing the local builder closure. In services/workspace_tabs.py lines 529-545, move build() before the nocache_enabled check and return build() from that branch; apply the same restructuring to the build() closure and nocache_enabled branch in services/workspace_db.py lines 443-454, preserving the existing cache call for non-nocache requests.tests/test_summary_cache_concurrency.py (1)
48-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComposer-map / invalid-workspace-alias cache paths are patched but never exercised.
COMPOSER_MAP_CACHE_FILEandINVALID_WORKSPACE_ALIASES_CACHE_FILEare overridden insetUp, but no test in this file drivesget_or_build_cached_composer_mapor the invalid-workspace-alias equivalent under concurrency. Issue#154calls for lock coverage across "projects, composer maps, invalid-workspace aliases, and tab summaries" - consider adding blocked-build/warm-cache tests for these two helpers analogous to the project ones, or drop the unused setup if intentionally out of scope for this layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_summary_cache_concurrency.py` around lines 48 - 53, Either add concurrency tests that exercise get_or_build_cached_composer_map and the invalid-workspace-alias cache helper, covering blocked builds and warm-cache reuse with the temporary paths configured in setUp, or remove the unused COMPOSER_MAP_CACHE_FILE and INVALID_WORKSPACE_ALIASES_CACHE_FILE overrides if this test layer intentionally excludes those caches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_summary_cache_concurrency.py`:
- Around line 43-57: Update setUp in the test class to patch
PROJECTS_CACHE_FILE, COMPOSER_MAP_CACHE_FILE, and
INVALID_WORKSPACE_ALIASES_CACHE_FILE through the mocking framework, alongside
CACHE_DIR, so their original module values are automatically restored during
tearDown. Ensure the temporary paths remain available to this test while
preventing summary_cache state from leaking into subsequent tests.
---
Nitpick comments:
In `@services/summary_cache.py`:
- Around line 263-277: The summary-cache path around
_workspace_storage_fingerprint and _get_cached_projects_unlocked holds the
module-wide lock during expensive fingerprint and cache-file I/O, and recomputes
the fingerprint twice on misses. Refactor the synchronization so unrelated
workspace/cache keys do not serialize these operations, using per-cache-key
locking or an equivalent keyed coordination mechanism while preserving
double-checking and preventing duplicate cache writes for the same key.
In `@services/workspace_tabs.py`:
- Around line 529-545: The nocache paths duplicate the cached builder logic
instead of reusing the local builder closure. In services/workspace_tabs.py
lines 529-545, move build() before the nocache_enabled check and return build()
from that branch; apply the same restructuring to the build() closure and
nocache_enabled branch in services/workspace_db.py lines 443-454, preserving the
existing cache call for non-nocache requests.
In `@tests/test_summary_cache_concurrency.py`:
- Around line 48-53: Either add concurrency tests that exercise
get_or_build_cached_composer_map and the invalid-workspace-alias cache helper,
covering blocked builds and warm-cache reuse with the temporary paths configured
in setUp, or remove the unused COMPOSER_MAP_CACHE_FILE and
INVALID_WORKSPACE_ALIASES_CACHE_FILE overrides if this test layer intentionally
excludes those caches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0784a7aa-2962-4628-a775-027efc3b6465
📒 Files selected for processing (6)
services/summary_cache.pyservices/workspace_context.pyservices/workspace_db.pyservices/workspace_listing.pyservices/workspace_tabs.pytests/test_summary_cache_concurrency.py
| def setUp(self): | ||
| self.tmp = tempfile.TemporaryDirectory() | ||
| self.cache_patch = patch.object(summary_cache, "CACHE_DIR", Path(self.tmp.name)) | ||
| self.cache_patch.start() | ||
| summary_cache.PROJECTS_CACHE_FILE = Path(self.tmp.name) / "projects.json" | ||
| summary_cache.COMPOSER_MAP_CACHE_FILE = ( | ||
| Path(self.tmp.name) / "composer-id-to-ws.json" | ||
| ) | ||
| summary_cache.INVALID_WORKSPACE_ALIASES_CACHE_FILE = ( | ||
| Path(self.tmp.name) / "invalid-workspace-aliases.json" | ||
| ) | ||
|
|
||
| def tearDown(self): | ||
| self.cache_patch.stop() | ||
| self.tmp.cleanup() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unrestored cache-file patches leak across tests.
self.cache_patch (for CACHE_DIR) is properly stopped in tearDown, but PROJECTS_CACHE_FILE, COMPOSER_MAP_CACHE_FILE, and INVALID_WORKSPACE_ALIASES_CACHE_FILE are reassigned as plain module attributes (lines 47-53) and never restored. After this test class runs, summary_cache permanently points at paths inside a temp directory that tearDown has already deleted via self.tmp.cleanup(). Any other test in the same process that relies on the module's default cache paths can silently break or fail depending on execution order.
🔧 Proposed fix using patch.multiple for auto-restoration
def setUp(self):
self.tmp = tempfile.TemporaryDirectory()
- self.cache_patch = patch.object(summary_cache, "CACHE_DIR", Path(self.tmp.name))
- self.cache_patch.start()
- summary_cache.PROJECTS_CACHE_FILE = Path(self.tmp.name) / "projects.json"
- summary_cache.COMPOSER_MAP_CACHE_FILE = (
- Path(self.tmp.name) / "composer-id-to-ws.json"
- )
- summary_cache.INVALID_WORKSPACE_ALIASES_CACHE_FILE = (
- Path(self.tmp.name) / "invalid-workspace-aliases.json"
- )
+ self.addCleanup(self.tmp.cleanup)
+ self.cache_patch = patch.multiple(
+ summary_cache,
+ CACHE_DIR=Path(self.tmp.name),
+ PROJECTS_CACHE_FILE=Path(self.tmp.name) / "projects.json",
+ COMPOSER_MAP_CACHE_FILE=Path(self.tmp.name) / "composer-id-to-ws.json",
+ INVALID_WORKSPACE_ALIASES_CACHE_FILE=(
+ Path(self.tmp.name) / "invalid-workspace-aliases.json"
+ ),
+ )
+ self.cache_patch.start()
+ self.addCleanup(self.cache_patch.stop)
-
- def tearDown(self):
- self.cache_patch.stop()
- self.tmp.cleanup()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def setUp(self): | |
| self.tmp = tempfile.TemporaryDirectory() | |
| self.cache_patch = patch.object(summary_cache, "CACHE_DIR", Path(self.tmp.name)) | |
| self.cache_patch.start() | |
| summary_cache.PROJECTS_CACHE_FILE = Path(self.tmp.name) / "projects.json" | |
| summary_cache.COMPOSER_MAP_CACHE_FILE = ( | |
| Path(self.tmp.name) / "composer-id-to-ws.json" | |
| ) | |
| summary_cache.INVALID_WORKSPACE_ALIASES_CACHE_FILE = ( | |
| Path(self.tmp.name) / "invalid-workspace-aliases.json" | |
| ) | |
| def tearDown(self): | |
| self.cache_patch.stop() | |
| self.tmp.cleanup() | |
| def setUp(self): | |
| self.tmp = tempfile.TemporaryDirectory() | |
| self.addCleanup(self.tmp.cleanup) | |
| self.cache_patch = patch.multiple( | |
| summary_cache, | |
| CACHE_DIR=Path(self.tmp.name), | |
| PROJECTS_CACHE_FILE=Path(self.tmp.name) / "projects.json", | |
| COMPOSER_MAP_CACHE_FILE=Path(self.tmp.name) / "composer-id-to-ws.json", | |
| INVALID_WORKSPACE_ALIASES_CACHE_FILE=( | |
| Path(self.tmp.name) / "invalid-workspace-aliases.json" | |
| ), | |
| ) | |
| self.cache_patch.start() | |
| self.addCleanup(self.cache_patch.stop) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_summary_cache_concurrency.py` around lines 43 - 57, Update setUp
in the test class to patch PROJECTS_CACHE_FILE, COMPOSER_MAP_CACHE_FILE, and
INVALID_WORKSPACE_ALIASES_CACHE_FILE through the mocking framework, alongside
CACHE_DIR, so their original module values are automatically restored during
tearDown. Ensure the temporary paths remain available to this test while
preventing summary_cache state from leaking into subsequent tests.
bradjin8
left a comment
bradjin8
left a comment
There was a problem hiding this comment.
Optional follow-ups
services/summary_cache.py
Global lock serializes fingerprint I/O across unrelated workspaces; per-cache-key locks if threaded latency regresses
tests/test_summary_cache_concurrency.py
Only projects cache tested under concurrency; other three types share _get_or_build_cached so risk is low
Single module lock was intentional per the design guide (same pattern as _workspace_path_lock), and the projects concurrency test is enough because all four cache types share _get_or_build_cached |
2 participants
Closes #154
Under threaded WSGI, two requests could both miss the summary cache, build in parallel, and the slower one would overwrite the faster one's write. That's the bug.
This PR adds a module lock in summary_cache.py and get_or_build_cached_* helpers for all four cache types. Fingerprint and cache lookup happen under the lock; the build runs outside it; then we re-fingerprint and recheck before writing. If another thread filled the cache while we were building, we return that result and skip the write.
The four call sites (workspace_listing, workspace_tabs, workspace_db, workspace_context) now go through those helpers. tests/test_summary_cache_concurrency.py has a deterministic lost-update test plus a warm-cache stress test. Full suite: 614 passed.
Summary by CodeRabbit