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.
| orchestrator.set_llm(self.llm) | ||
| orchestrator.init_update_queue() | ||
| if self.session_log is not None and hasattr(orchestrator, '_session_log'): | ||
| orchestrator._session_log = self.session_log |
There was a problem hiding this comment.
llm_agent.py
Lines 1296-1299
await self.load_memory()
...
self._init_session_log()
load_memory先于init_session_log被调用，因此load_memory时self.session_log里的值是否未被初始化
| def last_consolidated(self, value: int) -> None: | ||
| meta = self._load_metadata() | ||
| meta["last_consolidated"] = value | ||
| self._rewrite_metadata(meta) |
There was a problem hiding this comment.
_rewrite_metadata 是全文件读写。长session下，如果高频，可以再优化一下
| """ | ||
| for memory_tool in self.memory_tools: | ||
| messages = await memory_tool.run(messages) | ||
| return messages |
There was a problem hiding this comment.
inject_memory 与 condense_memory 实现相同
|
|
||
| if self.runtime.round == 0: | ||
| # New task: create standardized messages first | ||
| messages = await self.create_messages(messages) |
There was a problem hiding this comment.
_init_session_log 中并不会更新round，_init_session_log后round还是0，仍然会走create_messages，这是符合预期的吗
| if self.session_log is not None: | ||
| self.session_log.append( | ||
| self._msg_to_dict(cutoff_msg)) | ||
| self.save_history(messages) |
There was a problem hiding this comment.
save_history和session_log是否有功能重叠，能否直接合并？
| return | ||
|
|
||
| session_dir = getattr( | ||
| session_cfg, 'dir', None |
| session_key = getattr(session_cfg, 'session_key', None) if session_cfg else None | ||
| self.session_log = SessionLog(session_dir, session_key=session_key) | ||
|
|
||
| compaction_cfg = getattr(self.config, 'compaction', None) |
| show("backend_options", cfg.backend_options) | ||
|
|
||
| step("Config with backend_options...") | ||
| cfg2 = MemoryConfig( |
2 participants
Change Summary
Related issue number
Checklist
pre-commit installandpre-commit run --all-filesbefore git commit, and passed lint check.