feat(memory): add SQLite-backed persistent session store#719
feat(memory): add SQLite-backed persistent session store#719is-Xiaoen wants to merge 4 commits intosipeed:mainfrom
Conversation
Introduce pkg/memory with a Store interface that maps 1:1 to the current SessionManager API. Each method is an atomic operation, eliminating the need for a separate Save() call. This lays the groundwork for replacing JSON file-based session storage with a transactional backend.
Add SQLiteStore using modernc.org/sqlite (pure Go, zero CGo) with: - WAL journal mode for concurrent read/write on embedded devices - Single-connection serialization to prevent write conflicts - Transactional message insert with auto-incrementing sequence - ToolCalls stored as JSON column (always read/written with message) - Auto-creation of session rows on first message Connection tuned for embedded use: 512KB cache, NORMAL synchronous mode, 5s busy timeout, foreign keys enabled for cascade deletes.
Cover all Store methods with 14 unit tests including: - Basic CRUD roundtrips for messages and summaries - Auto-creation of sessions on first write - ToolCall JSON serialization/deserialization fidelity - Message ordering guarantees - TruncateHistory edge cases (keep-last, keep-zero) - SetHistory full replacement Add 2 concurrency tests: - 10 goroutines writing + 10 reading simultaneously - Simulated summarize-vs-main-loop race (ref sipeed#704) Add 3 benchmarks for write throughput and read performance at 100/1000 message scale.
Add MigrateFromJSON() to import existing sessions/*.json files into SQLiteStore for seamless upgrade from the current file-based backend. Key design decisions: - Read session key from JSON content, not filename (handles sanitizeFilename colon-to-underscore mapping correctly) - Rename migrated files to .json.migrated as backup (non-destructive) - INSERT OR IGNORE for idempotent partial-migration recovery - Skip invalid JSON files without aborting other migrations Includes 7 tests covering basic migration, tool call preservation, batch migration, error resilience, rename verification, idempotency, and the colon-in-key edge case.
61263d6 to
450d37f
Compare
Overall AssessmentThank you for this high-quality PR! The code is well-written with comprehensive test coverage and clear documentation. However, I have some questions about the technical direction that I'd like to discuss. Core Question: Do we really need SQLite?The current JSON storage issue stems from Why do mainstream tools use plain text storage?
Claude Code's storage approach: Even a complex AI tool like Claude Code uses JSONL instead of SQLite for storing history. Advantages of plain text:
Alternative: JSONLInstead of SQLite, I'd suggest considering JSONL (JSON Lines) format: Comparison:
JSONL key advantages:
RecommendationI'd prefer trying simpler approaches first:
This project currently runs as a single process. SQLite's transaction benefits may not be utilized, while adding complexity and dependencies. SummaryThe PR code quality is excellent, but I have reservations about the technical direction. I'd suggest discussing:
Looking forward to the discussion! |
|
Thanks for the thorough review! The dependency concern is totally fair — 7 transitive deps for session storage is a real cost for a "pico" tool. One technical nuance I want to flag: JSONL's append-only advantage breaks down for That said, the The 26M2W3 meeting notes mentioned "introduce SQLite" for the Agent track, which is why I went this direction. But I understand that may be a premature jump for the current single-process setup. Happy to implement a JSONL backend alongside or instead of SQLite — what direction would you prefer? |
📝 Description
Add a new
pkg/memory/package that implements a SQLite-backed session store usingmodernc.org/sqlite(pure Go, zero CGo, cross-compile friendly).Problem: The current
pkg/session/manager.gouses an in-memory map with full JSON serialization on everySave()call. This leads to:summarizeSession()runs concurrently with the main loop (Race condition in session history causes "tool_call_ids did not have response messages" (HTTP 400) #704)sessions/grows unbounded)Solution: A new
Storeinterface whose methods are each an atomic SQLite transaction — no separateSave()needed. This eliminates the storage-level write conflicts entirely.This PR is purely additive — it does not modify any existing code. Integration with
pkg/agent/loop.gowill be a follow-up PR.Key design decisions
SetMaxOpenConns(1)): serializes all writes at the DB level, preventing corrupt interleavingsynchronous=NORMAL, 5s busy timeoutMigration
MigrateFromJSON()imports existingsessions/*.jsonfiles into SQLite:sanitizeFilename()colon → underscore mapping.json.migratedas backup (non-destructive)INSERT OR IGNOREFiles
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
Ref #704 — eliminates the storage-level race condition (concurrent
Save()calls overwriting each other). Full application-level fix (summarize goroutine vs main loop) to follow in integration PR.📚 Technical Context (Skip for Docs)
modernc.org/sqliteavoids CGo for embedded cross-compilation.🧪 Test Environment
📸 Evidence (Optional)
Click to view test results and benchmarks
go vet ./pkg/memory/...— clean, no issues.☑️ Checklist