Feat/memory update#912
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new unified memory architecture, featuring a pluggable backend system, a robust session logging mechanism, and a non-destructive context assembly pipeline. The reviewer identified several critical performance and functional issues, including the use of blocking synchronous calls (subprocess.run and time.sleep) within asynchronous methods, inefficient object instantiation inside loops, missing fields during message restoration that could break tool-use interactions, and a performance bottleneck in the session log metadata update logic. All comments provide actionable feedback to improve the system's scalability and correctness.
| messages = [_Msg( | ||
| role=m.get('role', 'user'), | ||
| content=m.get('content', ''), | ||
| tool_calls=m.get('tool_calls'), | ||
| ) for m in restored] |
There was a problem hiding this comment.
When restoring messages from the session log, the tool_call_id and name fields are omitted. This will break the conversation history for tool-use interactions, as many LLM providers require these fields to correctly associate tool outputs with their corresponding calls. These fields should be included in the restoration logic, similar to how they are handled in _dicts_to_messages in the ContextAssembler.
messages = [_Msg(
role=m.get('role', 'user'),
content=m.get('content', ''),
tool_calls=m.get('tool_calls'),
tool_call_id=m.get('tool_call_id'),
name=m.get('name'),
) for m in restored]| result = subprocess.run( | ||
| cmd, capture_output=True, text=True, | ||
| timeout=timeout, cwd=cwd, env=env, | ||
| ) |
There was a problem hiding this comment.
subprocess.run is a synchronous, blocking call. When executed within an asynchronous context (such as the inject or search methods of this backend), it will block the entire event loop until the CLI command completes. This can cause significant latency and prevent other concurrent tasks from running. Consider using asyncio.create_subprocess_exec or wrapping the call in asyncio.to_thread to avoid blocking the loop.
|
|
||
| try: | ||
| fts = FTSRetriever(self._config) | ||
| results = await fts.search(query, limit=5) |
There was a problem hiding this comment.
Instantiating FTSRetriever on every call to _inject_fts_context is inefficient. The FTSRetriever constructor performs a SQLite connection and runs schema initialization scripts (_ensure_schema). Since inject is called in every iteration of the agent loop, this will significantly degrade performance. It is better to initialize the retriever once (e.g., in the backend's start method or lazily as a cached instance attribute) and reuse it.
| if isinstance(results, dict): | ||
| if self._is_transient_error(results): | ||
| time.sleep(1) | ||
| results = search_memories( |
There was a problem hiding this comment.
time.sleep(1) is a blocking call that will freeze the entire event loop in an asynchronous application. This prevents other concurrent tasks from making progress. Use await asyncio.sleep(1) instead (this will require importing asyncio).
await asyncio.sleep(1)| """Rewrite the first line (metadata header) of the JSONL file.""" | ||
| if not self._path.exists(): | ||
| self._ensure_metadata() | ||
| return | ||
| lines = self._path.read_text(encoding="utf-8").splitlines() | ||
| meta_line = json.dumps( | ||
| {**meta, "_type": "metadata"}, ensure_ascii=False | ||
| ) | ||
| if lines and lines[0].strip(): | ||
| try: | ||
| first = json.loads(lines[0]) | ||
| if first.get("_type") == "metadata": | ||
| lines[0] = meta_line | ||
| else: | ||
| lines.insert(0, meta_line) | ||
| except json.JSONDecodeError: | ||
| lines.insert(0, meta_line) | ||
| else: | ||
| lines.insert(0, meta_line) | ||
| self._path.write_text("\n".join(lines) + "\n", encoding="utf-8") | ||
| self._metadata = meta |
There was a problem hiding this comment.
The _rewrite_metadata method reads the entire session log file into memory, modifies the first line, and writes the whole content back to disk. This results in O(N) complexity for both memory and I/O, where N is the number of messages in the session. As the conversation history grows, this will become a major performance bottleneck. Consider storing mutable session metadata (like last_consolidated) in a separate sidecar file (e.g., {session_key}.meta) to keep the main log file strictly append-only.
1 participant
Change Summary
Related issue number
Checklist
pre-commit installandpre-commit run --all-filesbefore git commit, and passed lint check.