-
Couldn't load subscription status.
- Fork 16
fix: resolve BucketsService caching bug and improve thread safety #762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This commit fixes a bug where the `buckets` property was creating new BucketsService instances on every access instead of returning the cached instance. Additionally, improves thread safety for all cached services by migrating from manual caching to Python's @cached_property decorator. Changes: - Fix BucketsService returning new instances instead of cached (line 80) - Migrate 4 services to @cached_property for thread-safe caching: * buckets (was broken) * attachments (improve thread safety) * connections (improve thread safety) * folders (improve thread safety) - Remove 4 instance variables from __init__ (reduced boilerplate) - Update context_grounding to use cached property references - Add 33 comprehensive unit tests for caching behavior
| return ApiClient(self._config, self._execution_context) | ||
|
|
||
| @property | ||
| def assets(self) -> AssetsService: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we cache all of them? assets/processes/etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yesterday I had extensive conversation about it - this is the recommendation from my 'friends' (I asked 4 models about that):
| Service | Current | Expert Vote | Priority | Decision |
|---|---|---|---|---|
jobs |
@property |
✅ 4-0 Unanimous (Cache) | CRITICAL | Must Fix Immediately |
context_grounding |
@property |
⚖️ 2-2 Tied | Medium | Team Must Decide |
processes |
@property |
⚖️ 2-2 Tied | Medium | Team Must Decide |
attachments |
Manual cache | Refactor to @cached_property |
Low | Enhancement |
connections |
Manual cache | Refactor to @cached_property |
Low | Enhancement |
folders |
Manual cache | Refactor to @cached_property |
Low | Enhancement |
buckets |
Manual cache (broken) | @cached_property |
HIGH | Already Fixed |
Today I asked the same question and got this answer:
Final Service Caching Table
| Service | Cache? | Pattern | Rationale | Expert Agreement |
|---|---|---|---|---|
| attachments | ✅ Yes | Stateful Singleton | Manages temp files and handles cleanup; requires consistent singleton to prevent orphaned files | 3/3 unanimous |
| buckets | ✅ Yes | Stateful Singleton | File storage operations; requires consistent singleton to avoid redundant bucket lookups | 3/3 unanimous |
| connections | ✅ Yes | Stateful Singleton | External integrations; requires consistent singleton for connection pooling and session management | 3/3 unanimous |
| folders | ✅ Yes | Foundational Context | Foundational organizational context used by multiple services; caching prevents redundant folder lookups | 3/3 unanimous |
| api_client | ✅ Yes | HTTP Client | HTTP connection pooling - critical for performance. Reusing connections reduces latency and overhead | 3/3 unanimous |
| assets | ❌ No | Stateless Wrapper | Simple CRUD operations; no state to maintain | 3/3 unanimous |
| actions | ❌ No | Stateless Wrapper | Simple CRUD operations; no state to maintain | 3/3 unanimous |
| processes | ❌ No | Stateless Wrapper | Simple CRUD operations; no state to maintain | 3/3 unanimous |
| queues | ❌ No | Stateless Wrapper | Simple CRUD operations; no state to maintain | 3/3 unanimous |
| jobs | ❌ No | Stateless Wrapper | Simple CRUD operations; no state to maintain | 3/3 unanimous |
| documents | ❌ No | Stateless Wrapper | Simple CRUD operations; no state to maintain | 3/3 unanimous |
| llm_openai | ❌ No | Stateless Wrapper | Simple CRUD operations; no state to maintain | 3/3 unanimous |
| llm | ❌ No | Stateless Wrapper | Simple CRUD operations; no state to maintain | 3/3 unanimous |
| entities | ❌ No | Stateless Wrapper | Simple CRUD operations; no state to maintain | 3/3 unanimous |
| context_grounding | ❌ No | Stateless Composer | Correctly injects cached dependencies (api_client); caching the composer itself provides no benefit | 3/3 unanimous |
I was convinced that the ones i did in this PR are necessary - i can do others, if you want, but it would be good to get input from you / your team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this layer should only contain stateless wrappers. I don't see why you would need a new instance of a service each time you call sdk.assets, sdk.buckets etc, even if they are stateless wrappers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I added @cached_property tag to a number of them, so we don't get this problem.
If you want, I can add the same tag to the remaining services that require it (but maybe as the next PR?)
This commit fixes a bug where the
bucketsproperty was creating new BucketsService instances on every access instead of returning the cached instance. Additionally, improves thread safety for all cached services by migrating from manual caching to Python's @cached_property decorator.Changes:
Closes #712 and #659