Fix cache .set() kwargs usage#725
Conversation
`ThreadSafeCache.set` signature changed to `def set(self, data: Any, key: str = "default")` in previous updates to fix bugs, meaning positional arguments caused the `key` to be passed as `data` and vice-versa, or throw type errors in the previous implementation. This commit refactors all usage of cache `.set` to explicitly pass `data=...` and `key=...` keyword arguments to avoid bugs and ensure future-proof maintainability.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for fixmybharat canceled.
|
🙏 Thank you for your contribution, @RohanExploit!PR Details:
Quality Checklist:
Review Process:
Note: The maintainers will monitor code quality and ensure the overall project flow isn't broken. |
📝 WalkthroughWalkthroughThis pull request standardizes the cache API call signature across the backend codebase, changing ChangesCache API Signature Standardization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Pull request overview
This PR standardizes a few ThreadSafeCache.set() call sites to use explicit keyword arguments (data= / key=) to align with the cache API and reduce ambiguity.
Changes:
- Update
user_upload_cache.set(...)in upload-limit logic to passdata=andkey=explicitly. - Update
recent_issues_cache.set(...)in utility router endpoints to passdata=andkey=explicitly. - Update unit tests to use keyword arguments when calling
ThreadSafeCache.set(...).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/utils.py | Uses explicit data=/key= when persisting upload timestamps into user_upload_cache. |
| backend/tests/test_cache_unit.py | Updates cache unit tests to call set() with explicit keyword args. |
| backend/routers/utility.py | Uses explicit data=/key= for caching serialized JSON responses. |
| backend/main_fixed.py | Uses explicit data= for default-key cache writes in recent-issues caching paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data = response.model_dump(mode='json') | ||
| json_data = json.dumps(data) | ||
| recent_issues_cache.set(json_data, "stats") | ||
| recent_issues_cache.set(data=json_data, key="stats") |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/utils.py (1)
49-64:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeyword-arg update is correct; pre-existing TOCTOU in the rate-limit check.
The
get → filter → check → append → setsequence is not atomic. Two concurrent requests for the sameidentifiercan both read the samecurrent_uploads, both pass the limit check, both append, and both write back — effectively doubling the allowed burst. This was present before this PR, but worth addressing since the write side is the changed line.A simple fix is to move the full compound operation inside the cache lock, or use an atomic increment structure (e.g., a
threading.Lockper identifier).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils.py` around lines 49 - 64, The rate-limit check suffers a TOCTOU race: the sequence user_upload_cache.get(identifier) → filter → len(recent_uploads) check → append → user_upload_cache.set(...) must be performed atomically; wrap the whole sequence (get, filter old timestamps, compare against limit, append now, set) inside a per-identifier lock (e.g., a threading.Lock stored in a locks dict keyed by identifier or using any existing cache lock API) so concurrent requests for the same identifier cannot interleave, and ensure you release the lock after updating; modify the code around user_upload_cache.get/set and recent_uploads to acquire the lock before reading and only perform the check+append+set while holding the lock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/utils.py`:
- Around line 49-64: The rate-limit check suffers a TOCTOU race: the sequence
user_upload_cache.get(identifier) → filter → len(recent_uploads) check → append
→ user_upload_cache.set(...) must be performed atomically; wrap the whole
sequence (get, filter old timestamps, compare against limit, append now, set)
inside a per-identifier lock (e.g., a threading.Lock stored in a locks dict
keyed by identifier or using any existing cache lock API) so concurrent requests
for the same identifier cannot interleave, and ensure you release the lock after
updating; modify the code around user_upload_cache.get/set and recent_uploads to
acquire the lock before reading and only perform the check+append+set while
holding the lock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72970858-9f99-4607-8a1d-6bcfa2384297
📒 Files selected for processing (4)
backend/main_fixed.pybackend/routers/utility.pybackend/tests/test_cache_unit.pybackend/utils.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Fix
ThreadSafeCache.setcalls across the codebase to correctly use explicit keyword arguments (data=andkey=), preventing potential bugs and ensuring correct cache population.PR created automatically by Jules for task 17410130062223211849 started by @RohanExploit
Summary by cubic
Fixed all
ThreadSafeCache.setcalls to use explicitdata=andkey=kwargs, preventing swapped arguments and broken cache entries. This stabilizes caching for recent issues, stats/leaderboard responses, and upload limits.backend/main_fixed.py,backend/routers/utility.py, andbackend/utils.py.Written for commit a156c23. Summary will update on new commits.
Summary by CodeRabbit