Add weak LRU cache and Anthropic streaming tests, refine concurrency#62
Merged
JohnRichard4096 merged 9 commits intomainfrom May 2, 2026
Merged
Add weak LRU cache and Anthropic streaming tests, refine concurrency#62JohnRichard4096 merged 9 commits intomainfrom
JohnRichard4096 merged 9 commits intomainfrom
Conversation
Contributor
Reviewer's GuideAdds Anthropic adapter tests and a new WeakValueLRUCache utility, introduces per-event matcher locking and matcher expiry, adds TTL-based lifecycle management for MCPClient connections, refines ChatManager locking and ChatObject binding, fixes agent tool-loop behavior and OpenAI adapter streaming, and enhances CI to run on multiple Python versions and comment test summaries on PRs. Sequence diagram for matcher event triggering with per-event locking and expirysequenceDiagram
actor App
participant MatcherFactory
participant RepoLock as aiologic.Lock
participant EventRegistry
participant Handlers as list_FunctionData
App->>MatcherFactory: trigger_event(event, *args, **kwargs)
MatcherFactory->>MatcherFactory: event_type = event.get_event_type()
MatcherFactory->>MatcherFactory: lock = _repo_lock(event_type)
MatcherFactory->>RepoLock: acquire (async with)
activate RepoLock
MatcherFactory->>EventRegistry: get_handlers(event_type)
EventRegistry-->>MatcherFactory: handlers: defaultdict[int, list[FunctionData]]
MatcherFactory->>MatcherFactory: priorities = sorted(handlers.keys())
loop for each priority
MatcherFactory->>Handlers: matcher_list = handlers[priority]
MatcherFactory->>MatcherFactory: _simple_run(matcher_list, event, ...)
activate MatcherFactory
loop for each func in matcher_list
MatcherFactory->>MatcherFactory: matcher = func.matcher
MatcherFactory->>Matcher: dead?
alt matcher.dead is True
MatcherFactory->>MatcherFactory: add func to _dead_to_remove
MatcherFactory-->>MatcherFactory: continue
else matcher is alive
MatcherFactory->>MatcherFactory: build session_args, session_kwargs
MatcherFactory->>func.function: await handler(...)
func.function-->>MatcherFactory: result or exception
end
end
MatcherFactory->>MatcherFactory: remove _dead_to_remove from matcher_list
alt matcher.block is True
MatcherFactory-->>MatcherFactory: return False (stop further priorities)
else
MatcherFactory-->>MatcherFactory: continue
end
deactivate MatcherFactory
end
MatcherFactory->>RepoLock: release
deactivate RepoLock
MatcherFactory-->>App: completed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The new
FunctionDatadataclass still usespydantic.Fieldand a custommodel_dumpbased onasdict; if callers relied on Pydantic model semantics (validation, aliases,model_dumpoptions, etc.) this is a breaking change—consider either keeping it as a Pydantic model or fully switching todataclasses.fieldand updating any callers accordingly. - The
Matcher.deadproperty usesdatetime.datetime.now()without a timezone, while other parts of the code (e.g.,ChatObject) use UTC-aware datetimes; consider using a consistent timezone-aware approach to avoid subtle comparison issues between naive and aware datetimes. - In
WeakValueLRUCache.__len__, the implementation returnslen(self._cache)without cleaning expired entries, but the docstring claims it returns the number of non-expired items—either adjust the implementation to filter out expired entries or update the docstring to reflect the actual behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `FunctionData` dataclass still uses `pydantic.Field` and a custom `model_dump` based on `asdict`; if callers relied on Pydantic model semantics (validation, aliases, `model_dump` options, etc.) this is a breaking change—consider either keeping it as a Pydantic model or fully switching to `dataclasses.field` and updating any callers accordingly.
- The `Matcher.dead` property uses `datetime.datetime.now()` without a timezone, while other parts of the code (e.g., `ChatObject`) use UTC-aware datetimes; consider using a consistent timezone-aware approach to avoid subtle comparison issues between naive and aware datetimes.
- In `WeakValueLRUCache.__len__`, the implementation returns `len(self._cache)` without cleaning expired entries, but the docstring claims it returns the number of non-expired items—either adjust the implementation to filter out expired entries or update the docstring to reflect the actual behavior.
## Individual Comments
### Comment 1
<location path="src/amrita_core/hook/matcher.py" line_range="387-396" />
<code_context>
Returns:
bool: Should continue to run.
"""
+ _dead_to_remove: list[FunctionData] = []
for func in matcher_list:
+ matcher: Matcher = func.matcher
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing dead handlers from `matcher_list` while iterating can raise errors and/or skip handlers.
Because `_dead_to_remove` is populated inside the loop and removals happen in the `finally` of each iteration:
1. You mutate `matcher_list` while iterating over it, which can skip handlers or cause unexpected errors.
2. `_dead_to_remove` is never cleared, so later iterations may attempt to remove already-removed entries, raising `ValueError`.
Consider iterating over a snapshot and doing the cleanup once after the loop, for example:
```python
_dead_to_remove: list[FunctionData] = []
for func in list(matcher_list):
matcher: Matcher = func.matcher
if matcher.dead:
_dead_to_remove.append(func)
continue
... # existing handler logic
for dead_func in _dead_to_remove:
if dead_func in matcher_list:
matcher_list.remove(dead_func)
```
</issue_to_address>
### Comment 2
<location path="src/amrita_core/hook/matcher.py" line_range="144-146" />
<code_context>
"""
raise PassException() # pragma: no cover
+ @property
+ def dead(self) -> bool:
+ return self._dead_at is not None and self._dead_at < datetime.datetime.now()
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider timezone-aware comparison in `Matcher.dead` to avoid subtle datetime bugs.
If `_dead_at` is ever timezone-aware (e.g. UTC), comparing it to `datetime.datetime.now()` (naive) will raise a `TypeError` when mixing aware and naive datetimes. To avoid this, either always store `_dead_at` as UTC-aware and compare with `datetime.now(timezone.utc)`, or derive `now` using the same `tzinfo` as `_dead_at` so both sides of the comparison are consistent.
```suggestion
@property
def dead(self) -> bool:
if self._dead_at is None:
return False
is_aware = (
self._dead_at.tzinfo is not None
and self._dead_at.tzinfo.utcoffset(self._dead_at) is not None
)
if is_aware:
now = datetime.datetime.now(self._dead_at.tzinfo)
else:
now = datetime.datetime.now()
return self._dead_at < now
```
</issue_to_address>
### Comment 3
<location path="src/amrita_core/tools/mcp.py" line_range="97-101" />
<code_context>
Args:
update_tools (bool, optional): whether to update the tool list. Defaults to False.
"""
- if self.mcp_client is not None:
+ if self.mcp_client is not None and not self._close_waitter:
raise RuntimeError("MCP Server is already connected!")
+ else:
+ await self._clean_waitter()
server_script: MCP_SERVER_SCRIPT_TYPE = self.server_script
</code_context>
<issue_to_address>
**issue (bug_risk):** Reconnecting while a TTL close is scheduled cancels the timer but leaves the previous connection open.
With the new condition, when `self.mcp_client is not None` and `self._close_waitter` is set, `_connect` bypasses the `RuntimeError` and calls `_clean_waitter()`. `_clean_waitter()` cancels the TTL task but does not close the existing `mcp_client` before you assign a new `Client`, so the original connection can be leaked.
You should close any existing `mcp_client` when cancelling a pending TTL close and reconnecting, e.g. by calling `await self._close()` before creating a new client, or by extending `_clean_waitter()` so that it both cancels the TTL and ensures there is at most one active connection.
</issue_to_address>
### Comment 4
<location path="src/amrita_core/weakcache.py" line_range="185-187" />
<code_context>
+ return False
+ return self._cache[key]() is not None
+
+ def __len__(self) -> int:
+ """!!!This will return the number of non-expired items in the cache.!!!"""
+ return len(self._cache)
+
+ def __iter__(self) -> Iterator[K]:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `__len__` implementation counts entries without cleaning expired weakrefs, contradicting the docstring.
Because expired entries stay in `_cache` until cleanup, `len(self._cache)` can over-count and conflicts with the docstring’s promise.
You could either:
- Make `__len__` clean up expired entries before counting (like `size()`), or
- Update the docstring to state that `__len__` returns the raw mapping size and recommend `size()` for live-entry counts.
It may also be worth checking whether methods like `is_full()` should use `size()` instead of `len(self._cache)`, depending on your intended semantics.
Suggested implementation:
```python
def __len__(self) -> int:
"""Return the number of non-expired items in the cache."""
count = 0
for key in list(self._cache.keys()):
if self._cleanup_key_if_expired(key):
count += 1
return count
```
If this class exposes a `size()` method elsewhere in the file, you may want to:
1. Implement `size()` in terms of `__len__` (e.g. `return len(self)`) to keep the semantics consistent.
2. Review any `is_full()` or similar methods in this class and decide whether they should use `len(self)` (live entries) instead of `len(self._cache)` (raw mapping size), based on the intended behavior.
</issue_to_address>
### Comment 5
<location path="tests/test_weakcache.py" line_range="35-45" />
<code_context>
+ with pytest.raises(ValueError, match="Capacity must be a positive integer"):
+ WeakValueLRUCache(capacity=-1)
+
+ def test_init_with_zero_capacity(self):
+ """Test initialization with zero capacity"""
+ cache = WeakValueLRUCache(capacity=0)
+ assert cache.capacity == 0
+ assert len(cache) == 0
+
+ def test_init_with_positive_capacity(self):
</code_context>
<issue_to_address>
**suggestion (testing):** There is no test covering `WeakValueLRUCache` behavior when `capacity=0` and items are inserted.
You only assert that a zero-capacity cache initializes correctly, but don’t exercise `put`/`get` under this configuration. Since `capacity=0` is a corner case for the eviction logic in `WeakValueLRUCache.put`, please add a test that inserts an item and asserts the intended behavior (e.g., cache remains empty and `get` returns `None`), so the expected semantics for zero capacity are clear and guarded by tests.
```suggestion
def test_init_with_zero_capacity(self):
"""Test initialization with zero capacity"""
cache = WeakValueLRUCache(capacity=0)
assert cache.capacity == 0
assert len(cache) == 0
def test_zero_capacity_put_and_get(self):
"""Test behavior of put/get when cache has zero capacity"""
cache = WeakValueLRUCache(capacity=0)
obj = TestObject("value")
cache.put("key", obj)
# Cache should remain empty despite the put
assert len(cache) == 0
assert cache.get("key") is None
def test_init_with_positive_capacity(self):
"""Test normal initialization"""
cache = WeakValueLRUCache(capacity=5)
assert cache.capacity == 5
assert len(cache) == 0
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Deploying amritacore with
|
| Latest commit: |
f84337e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b917d19e.amritacore.pages.dev |
| Branch Preview URL: | https://update-common.amritacore.pages.dev |
JohnRichard4096
force-pushed
the
update/common
branch
from
cba2254 to
f84337e
Compare
Co-authored-by: Copilot <copilot@github.com>
JohnRichard4096
force-pushed
the
update/common
branch
from
f84337e to
5d2eeaf
Compare
JohnRichard4096
force-pushed
the
update/common
branch
from
91fbb5a to
3bde13a
Compare
Member
Author
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
Member
Author
|
@sourcery-ai title |
Member
Author
|
@sourcery-ai summary |
sourcery-ai
Bot
changed the title
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
1 participant
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
Add comprehensive Anthropic adapter streaming tests, introduce a weak-reference LRU cache and use it for matcher lock pooling, and refine concurrency, lifecycle, and CI behavior across the chat manager, MCP client, and agent tooling.
New Features:
Enhancements:
CI:
Tests:
Chores: