Skip to content

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 24, 2025
Merged

fix: resolve BucketsService caching bug and improve thread safety #762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1040466
fix: resolve BucketsService caching bug and improve thread safety
uipreliga Oct 22, 2025
70db506
Merge branch 'main' into fix/buckets-service-caching
uipreliga Oct 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 10 additions & 37 deletions src/uipath/_uipath.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from functools import cached_property
from typing import Optional

from pydantic import ValidationError
Expand Down Expand Up @@ -51,10 +52,6 @@ def __init__(
raise BaseUrlMissingError() from e
elif error["loc"][0] == "secret":
raise SecretMissingError() from e
self._folders_service: Optional[FolderService] = None
self._buckets_service: Optional[BucketsService] = None
self._attachments_service: Optional[AttachmentsService] = None
self._connections_service: Optional[ConnectionsService] = None
setup_logging(should_debug=debug)
self._execution_context = ExecutionContext()

Expand All @@ -66,13 +63,9 @@ def api_client(self) -> ApiClient:
def assets(self) -> AssetsService:
Copy link
Member

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?

Copy link
Collaborator Author

@uipreliga uipreliga Oct 23, 2025 •
edited
Loading

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.

Copy link
Member

@cristipufu cristipufu Oct 24, 2025 •
edited
Loading

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

Copy link
Collaborator Author

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?)

return AssetsService(self._config, self._execution_context)

@property
@cached_property
def attachments(self) -> AttachmentsService:
if not self._attachments_service:
self._attachments_service = AttachmentsService(
self._config, self._execution_context
)
return self._attachments_service
return AttachmentsService(self._config, self._execution_context)

@property
def processes(self) -> ProcessesService:
Expand All @@ -82,39 +75,21 @@ def processes(self) -> ProcessesService:
def actions(self) -> ActionsService:
return ActionsService(self._config, self._execution_context)

@property
@cached_property
def buckets(self) -> BucketsService:
if not self._buckets_service:
self._buckets_service = BucketsService(
self._config, self._execution_context
)
return BucketsService(self._config, self._execution_context)

@property
@cached_property
def connections(self) -> ConnectionsService:
if not self._connections_service:
if not self._folders_service:
self._folders_service = FolderService(
self._config, self._execution_context
)
self._connections_service = ConnectionsService(
self._config, self._execution_context, self._folders_service
)
return self._connections_service
return ConnectionsService(self._config, self._execution_context, self.folders)

@property
def context_grounding(self) -> ContextGroundingService:
if not self._folders_service:
self._folders_service = FolderService(self._config, self._execution_context)
if not self._buckets_service:
self._buckets_service = BucketsService(
self._config, self._execution_context
)
return ContextGroundingService(
self._config,
self._execution_context,
self._folders_service,
self._buckets_service,
self.folders,
self.buckets,
)

@property
Expand All @@ -129,11 +104,9 @@ def queues(self) -> QueuesService:
def jobs(self) -> JobsService:
return JobsService(self._config, self._execution_context)

@property
@cached_property
def folders(self) -> FolderService:
if not self._folders_service:
self._folders_service = FolderService(self._config, self._execution_context)
return self._folders_service
return FolderService(self._config, self._execution_context)

@property
def llm_openai(self) -> UiPathOpenAIService:
Expand Down
Loading