Refine async thread safety with aiologic locks and reasoning tools#50
Merged
JohnRichard4096 merged 7 commits intomainfrom Apr 12, 2026
Merged
Refine async thread safety with aiologic locks and reasoning tools#50JohnRichard4096 merged 7 commits intomainfrom
JohnRichard4096 merged 7 commits intomainfrom
Conversation
Contributor
Reviewer's GuideIntroduces a threadsafe async lock abstraction that supports Python 3.13+ free-threaded mode, refactors ContextThreadsafe to use it, adjusts agent tool selection so the reasoning tool is added per-call instead of at construction, ensures ProtocolSingleton runs its base class init, and bumps the project version to 0.8.1. Sequence diagram for per-call agent tool selectionsequenceDiagram
actor User
participant Agent
participant StrategyContext as StrategyContext
participant Config as ConfigBuiltin
participant ToolsCaller
User->>Agent: single_execute(msg_list)
Agent->>StrategyContext: get config
StrategyContext-->>Agent: config
Agent->>Config: read tool_calling_mode
alt tool_calling_mode == agent
Agent->>Agent: ensure STOP_TOOL in self.tools
else tool_calling_mode == none
Agent-->>User: return False
Agent->>Agent: return
end
Agent->>Agent: tools = self.tools.copy()
Agent->>Config: read agent_thought_mode
alt agent_thought_mode startswith reasoning
Agent->>Agent: tools.append(REASONING_TOOL)
end
Agent->>ToolsCaller: tools_caller(msg_list, tools, tool_choice,...)
ToolsCaller-->>Agent: response_msg
Agent-->>User: processed result
Class diagram for AsyncLockThreadsafe and ContextThreadsafeclassDiagram
class AsyncLockThreadsafe {
+asyncio.Lock _async_lock
+threading.Lock _thread_lock
+threading.Lock _meta_lock
+int _owner_thread_id
+bool _is_thread_locked
+AsyncLockThreadsafe()
+acquire() async
+release() void
+__aenter__() async AsyncLockThreadsafe
+__aexit__(exc_type, exc_val, exc_tb) async void
}
class ContextThreadsafe {
+AsyncLockThreadsafe _ctx_lock
+ContextThreadsafe()
+__aenter__() async void
+__aexit__(exc_type, exc_val, exc_tb) async void
}
ContextThreadsafe --> AsyncLockThreadsafe : wraps
Class diagram for ProtocolSingleton initialization behaviorclassDiagram
class ModelAdapter {
<<interface>>
}
class ProtocolSingleton {
-static ProtocolSingleton __instance
-dict~string, ModelAdapter~ _adapter_class
+__new__() ProtocolSingleton
+__init__() void
+get_adapters() dict~string, ModelAdapter~
}
ProtocolSingleton --> ModelAdapter : stores adapter types
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
AsyncLockThreadsafe.acquire, the path that callsawait asyncio.to_thread(self._thread_lock.acquire)will block cancellation and doesn't track acquisition depth, so if a coroutine is cancelled while waiting or a caller tries to re-enter the lock from the same thread, state can become subtle; consider making the lock explicitly non‑reentrant (e.g., with an assertion) and/or documenting and guarding against cancellation while waiting on the thread lock. - The addition of
__init__inModelAdapterProtocolcallssuper().__init__()on every access to the singleton, which is unusual for a singleton pattern; ifModelAdapterProtocolor its superclass ever add state in__init__, this will re-run initialization repeatedly, so you may want to guard initialization with a flag or move it into__new__.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AsyncLockThreadsafe.acquire`, the path that calls `await asyncio.to_thread(self._thread_lock.acquire)` will block cancellation and doesn't track acquisition depth, so if a coroutine is cancelled while waiting or a caller tries to re-enter the lock from the same thread, state can become subtle; consider making the lock explicitly non‑reentrant (e.g., with an assertion) and/or documenting and guarding against cancellation while waiting on the thread lock.
- The addition of `__init__` in `ModelAdapterProtocol` calls `super().__init__()` on every access to the singleton, which is unusual for a singleton pattern; if `ModelAdapterProtocol` or its superclass ever add state in `__init__`, this will re-run initialization repeatedly, so you may want to guard initialization with a flag or move it into `__new__`.
## Individual Comments
### Comment 1
<location path="src/amrita_core/threadsafe.py" line_range="51" />
<code_context>
+ if take_async:
+ await self._async_lock.acquire()
+ else:
+ await asyncio.to_thread(self._thread_lock.acquire)
+ with self._meta_lock:
+ self._owner_thread_id = current_tid
</code_context>
<issue_to_address>
**issue (bug_risk):** Cancellation while waiting on `to_thread(self._thread_lock.acquire)` can leave the lock acquired with no corresponding release.
If the coroutine is cancelled while `asyncio.to_thread(self._thread_lock.acquire)` is running, the worker thread may successfully acquire the lock, but the coroutine will never run the subsequent code to record ownership or ensure a matching `release()`, leaving the lock taken indefinitely. Please handle `asyncio.CancelledError` so that a successfully acquired lock is released on cancellation, or switch to a non-blocking acquire loop that cooperates safely with cancellation.
</issue_to_address>
### Comment 2
<location path="src/amrita_core/threadsafe.py" line_range="80-89" />
<code_context>
+ async def __aexit__(self, exc_type, exc_val, exc_tb) -> None:
+ self.release()
class ContextThreadsafe:
- """Add a optional async-context lock for thread-safe."""
+ """
+ Provides an optional async context manager for thread-safe operations.
+
+ This class wraps AsyncLockThreadsafe to provide a convenient context manager
+ interface for ensuring thread safety in async code.
+ """
+
+ _ctx_lock: AsyncLockThreadsafe
- _lock = asyncio.Lock()
+ def __init__(self):
+ self._ctx_lock = AsyncLockThreadsafe()
async def __aenter__(self):
</code_context>
<issue_to_address>
**question (bug_risk):** Changing from a class-level lock to a per-instance lock alters synchronization semantics and may break assumptions of global mutual exclusion.
Previously `_lock` was a class attribute, so all `ContextThreadsafe` uses shared a single `asyncio.Lock`. Now each instance has its own `AsyncLockThreadsafe`, so different `ContextThreadsafe` instances can run concurrently. If any code relies on this being a process/module-wide critical section, this change could introduce race conditions. If global mutual exclusion is still required, use a shared (class- or module-level) `AsyncLockThreadsafe` instead of an instance-level one.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
Well, I think I should consider using AioLogic instead of other locks. |
Member
Author
|
@sourcery-ai title |
Member
Author
|
@sourcery-ai summary |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Switch thread-safety primitives to aiologic-based locks and adjust agent tool handling to support reasoning tools per-call while tightening concurrency tests and versioning.
Bug Fixes:
Enhancements:
Build:
CI: