[FEAT] agentic-core conversation and responses database CRUD.#33
Conversation
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
There was a problem hiding this comment.
This is great!
Can we handle a few items to before merging?
-
Add
tenant_id TEXTplaceholder columns to all three tables with indexes on(tenant_id, id)— avoids a schema migration when multi-tenancy lands. -
Add
raw_tokens TEXTplaceholder column onitemsand/orresponses— so we can store the unmodified token sequence and pass it back to vLLM for faster inference (prefix caching / KV-cache reuse) without a migration later. -
Add a fallback variant for unknown item types —
InputItemandOutputItemonly handlemessageandfunction_call/function_call_output. For reference, ogx'sConversationItemhandles ~10 types (web search, file search, MCP approval, MCP call, reasoning, compaction, etc.). Unknown types silently disappear viafilter_mapinas_inout(). At minimum, add anUnknown(serde_json::Value)variant so data is preserved round-trip. -
Add
metadatato theConversationstruct — the migration SQL has ametadata TEXTcolumn but theFromRowstruct inmodels/conversation.rsonly hasidandcreated_at. The field is silently dropped on read.
Note on crate structure
PR #35 (move gateway to agentic-server) was reverted in PR #40, so this PR's crates/agentic-core/ paths will need to be rebased accordingly.
| CREATE TABLE IF NOT EXISTS conversations ( | ||
| id TEXT PRIMARY KEY, | ||
| metadata TEXT, | ||
| created_at TEXT NOT NULL |
There was a problem hiding this comment.
created_at should be INTEGER NOT NULL (Unix epoch), not TEXT. The OpenAI API spec returns created_at as an int. TEXT means range queries do lexicographic comparison (slower), and converting back to the API response requires parsing. Applies to all three tables.
There was a problem hiding this comment.
@franciscojavierarceo reverted the type to integer
| let item = Item { | ||
| id: "item_789".to_string(), | ||
| data: r#"{"role":"assistant"}"#.to_string(), | ||
| created_at: "2024-01-01T00:00:00Z".to_string(), |
There was a problem hiding this comment.
Use MAX(seq) instead of COUNT(*) for the next sequence number. If items are ever deleted, COUNT(*) returns a lower number than the actual max seq, causing collisions. Use SELECT MAX(seq) FROM items WHERE conversation_id = ? then max_seq.unwrap_or(-1) + 1.
There was a problem hiding this comment.
|
|
||
| type DbResult<T> = Result<T, sqlx::Error>; | ||
|
|
||
| static SCHEMA_READY: AtomicBool = AtomicBool::new(false); |
There was a problem hiding this comment.
SCHEMA_READY should be per-pool, not a process-global static. Two pools pointing at different databases means the second skips migrations. The ensure_ready_for_test workaround confirms this is a known pain point. Suggestion: bundle the pool and readiness flag into a wrapper struct (e.g., PoolWithSchema) — eliminates the need for ensure_ready_for_test entirely.
There was a problem hiding this comment.
@franciscojavierarceo changed to per instance pool creation over static global. my initial though was that it might be expensive and cause pref regression to have the pool created per instance. but as long as we make sure it is called correctly one it should not be a concern.
| #[must_use] | ||
| pub fn new_test() -> Self { | ||
| Self { pool: None } | ||
| } |
There was a problem hiding this comment.
new_test() is identical to disabled() — drop one. Both do Self { pool: None }.
|
|
||
| impl From<&InOutItem> for String { | ||
| fn from(item: &InOutItem) -> Self { | ||
| match item { |
There was a problem hiding this comment.
unwrap_or_default() silently swallows serialization errors, writing empty strings to the database. Use TryFrom or return Result<String, ...> to propagate the error.
There was a problem hiding this comment.
There was a problem hiding this comment.
Few comments
get_items fetches via WHERE id IN (...) but SQL doesn't guarantee result order matches the input list. ResponseStore::rehydrate passes the ordered history_item_ids, so items can come back shuffled — breaking conversation ordering. A post-fetch reindex against the input would fix it:
let mut map: HashMap<String, Item> = rows.into_iter().map(|r| (r.id.clone(), r)).collect();
let ordered: Vec<Item> = ids.iter().filter_map(|id| map.remove(id)).collect();Separately — rehydrate uses filter_map(|row| row.as_inout()) which silently drops items that can't deserialize as either InputItem or OutputItem. Combined with francisco's point about needing an Unknown variant, this means any item type the enum doesn't cover today (reasoning, web_search_call, mcp_approval, etc.) will vanish from the rehydrated history. Might be worth at least a tracing::warn! on the None path so operators notice when items are being dropped.
|
|
||
| CREATE INDEX IF NOT EXISTS idx_responses_conversation_id ON responses (conversation_id); | ||
| CREATE INDEX IF NOT EXISTS idx_responses_previous_response_id ON responses (previous_response_id); | ||
| CREATE INDEX IF NOT EXISTS idx_responses_created_at ON responses (created_at); |
There was a problem hiding this comment.
FYI — we'll also want to add a raw_tokens TEXT placeholder column (on items and/or responses) so we can store the unmodified token sequence and pass it back to vLLM for prefix cache reuse. This will require adding token pass-through support to the /v1/responses endpoint upstream in vllm-project/vllm, since it currently only renders messages to tokens internally. The /v1/completions endpoint already supports prompt: list[int] so the plumbing exists — we just need to expose it through the Responses API.
There was a problem hiding this comment.
@franciscojavierarceo I think we should add those columns when the feature is actually supported than a placeholder. cause it would be out of the scope of the PR ?
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Signed-off-by: maral <maralbahari.98@gmail.com>
@ashwing Thanks for the suggestion. added suggestion for the preserving the order of item ids. |
1-2: I think we can add those columns when the feature is supported just placeholder doesnt seem to be doing anything. 3: just for the simplicity input output text message added in this PR and the serialize and deserialize errors are handled as you mentioned in another comment. other types can be added as the tool calling features are developed. while in this PR there are some 4: the |
Signed-off-by: maral <maralbahari.98@gmail.com>
…item types (#45) ## Summary Follow-up to #33 addressing outstanding review feedback: - **Schema placeholders**: New migration `0002` adds `tenant_id`, `metadata`, and `raw_tokens` columns to avoid future schema migrations when multi-tenancy, conversation metadata, and token pass-through land - **Conversation metadata**: `metadata: Option<String>` added to `Conversation` model and `ConversationData` domain type with `From` impl coverage - **Unknown item type fallback**: `#[serde(other)] Unknown` variant on `InputItem` and `OutputItem` so unrecognized item types (web search, MCP, reasoning, compaction, etc.) don't silently fail deserialization - **Observability**: `tracing::warn!` in `as_inout()` when items can't be deserialized — tries `InputItem` first, then `OutputItem`, only warns if both produce `Unknown` ## Test Plan - All 84 existing tests pass (`cargo test`) - `cargo clippy --all-targets -- -D warnings` clean - `cargo fmt -- --check` clean - Pre-commit hooks pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Francisco Javier Arceo <farceo@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
This will be part of
agentic-coreafter #29 merged will open for reviewIntegrated complete SQLx-based storage layer for persistent conversation and response management. Implements trait-based CRUD operations with connection pooling supporting SQLite, PostgreSQL, and MySQL. Two main stores (
ConversationStoreandResponseStore) handle persisting input/output items with metadata and rehydrating full conversation/response history from database.Features:
ConversationStore: persist turns with items and responses, rehydrate full conversation historyResponseStore: persist responses with items and metadata, rehydrate by response IDSchemaManagerFromRowderiveTest Plan
Unit tests (39 passing):
Integration tests (18 passing):
Benchmarks:
conversation_persist: measure insertion of items and response recordsresponse_persist: measure response creation with item trackingconversation_rehydrate: measure full conversation retrieval from databaseresponse_rehydrate: measure response history lookup by IDAll tests pass. No clippy warnings.
Running Benchmarks
To run the storage CRUD benchmarks:
Or run with specific sample count and target time:
Benchmark Results
Performance Notes
Response persist is ~1.5x faster than conversation persist because:
ResponseStore.persist()storeshistory_item_idsdirectly in the responses table (single lookup)ConversationStore.persist()must query the items table to determine the next sequence number for conversation items (additional query)