Repository navigation
fix: give each response-history item occurrence its own row id - #455
Conversation
Zheng-Lu
left a comment
Zheng-Lu
left a comment
There was a problem hiding this comment.
Thanks for picking this up. I checked this PR against the report and on PostgreSQL. It fixes the bug. And I have one request about indexing
| SELECT COALESCE(tenant_id, 'default_tenant') AS tenant_id FROM conversations WHERE id = $1 \ | ||
| ), source_rows AS ( \ | ||
| SELECT incoming.seq AS incoming_seq, \ | ||
| COALESCE(MIN(CASE WHEN source.id = incoming.id THEN source.id END), MIN(source.id)) AS source_id \ |
There was a problem hiding this comment.
When several history rows share a public ID with different content, this picks the earliest occurrence, because uuid7 row IDs sort by creation time. Could you add a one-line comment saying so, so the next reader knows it's intentional?
| SELECT incoming.seq AS incoming_seq, \ | ||
| COALESCE(MIN(CASE WHEN source.id = incoming.id THEN source.id END), MIN(source.id)) AS source_id \ | ||
| FROM incoming CROSS JOIN owner JOIN items source \ | ||
| ON COALESCE(source.public_id, source.reference_id, source.id) = incoming.id \ |
There was a problem hiding this comment.
The new source lookup matches COALESCE(source.public_id, source.reference_id, source.id), so the added index on public_id does not serve this predicate. SQLite's query plan uses idx_items_tenant_id and scans that tenant's items for each incoming item. This runs during ordinary conversation persistence as well as explicit item insertion.
I compared the exact parent and PR insertion statements using an in-memory SQLite database with all PR migrations and 100,000 existing items belonging to one tenant. Inserting 100 new items took a median of approximately 1.78 seconds on the PR versus 1.66 milliseconds on the parent, over three runs. This measures SQL execution, not end-to-end HTTP latency. Adding an experimental index on (tenant_id, COALESCE(public_id, reference_id, id)) restored approximately 1.66 ms in the same experiment.
The best scoped fix is to replace the new plain public_id index in this PR's migration with a non-unique composite expression index:
CREATE INDEX idx_items_tenant_public_identity
ON items (
tenant_id,
(COALESCE(public_id, reference_id, id))
);This matches both equality predicates in source_rows, allowing the database to find rows for one tenant and public identity directly. Keep the existing source_rows reduction: multiple occurrences can legitimately share a public ID, and the index must not be UNIQUE. The expression also indexes legacy rows through the existing reference_id/id fallback, without requiring a data backfill or changing which stored content is selected.
Expression indexes are supported by SQLite and PostgreSQL. Keep the indexed COALESCE expression aligned with the query; SQLite requires the expressions to match. This is the smallest change that addresses the measured regression while preserving the PR's identity-resolution behavior.
Please verify the complete insert's query plan on populated SQLite and PostgreSQL databases, checking that source resolution uses both tenant and effective identity rather than scanning all items for the tenant. Rerun the cassette replays after the migration change. The SQLite benchmark above validates this index approach; PostgreSQL execution and query-plan validation remain outstanding.
805a671 to
7167a8d
Compare
|
@maralbahari thanks. all four are in.
|
|
@awesome-pro The newer Conversations cassette tests still compare saved OpenAI and gateway YAML rather than executing the current gateway implementation. Although this test file is not currently changed in the PR, the storage changes require updating its coverage. Please extend Keep the existing OpenAI-versus-gateway YAML comparisons, and add executable replay of the Conversations recordings for continuation, branching, deletion, and pagination—including streaming variants—against the gateway built from this PR. The replay should exercise the actual gateway handlers, executor, and storage, using a mock upstream to serve recorded model responses. Assert forwarded history, rehydrated item order/content/public IDs, response snapshots, deletion, and pagination. Do not mock the gateway’s public responses directly, because that would bypass the storage changes under review. Reuse both sets of recordings and preserve the OpenAI references as the expected public behavior. Live vLLM access or re-recording should not be required for these replay tests. Comparing the saved YAML remains useful, but it must be complemented by assertions against results produced by the current gateway implementation. |
Stored response history used each item's public ID as the `items.id` primary key, so a client that sent a previous response's output back as input with `store: true` hit `UNIQUE constraint failed: items.id` and a 500, even though the request was valid. Response history rows now get a fresh `item_…` row id and keep the public ID in a new nullable `public_id` column, indexed for lookups. The public ID stays stable for clients through `Item::public_id()`, and repeated occurrences keep every position. Conversation inserts, cursors, retrieval, and detach resolve an item by its public identity (`COALESCE(public_id, reference_id, id)`) instead of by row id, so adding a stored response item to a conversation still keeps the original stored content and never collides on a repeated ID. Resolution picks one source row per incoming item before joining, because several rows can share a public identity and joining on the public identity alone fans out. Refs vllm-project#385 Signed-off-by: Abhinandan <abhinandanverma551@gmail.com>
Response history used a second insertion path in response_history.rs. Move the fresh row ID and public ID handling into the existing item::create_in_tx without-conversation branch so the items table has one CRUD implementation for row identity, public identity, batching, and insertion order. The module stays over the 500-line production cap, so record the consolidation as a reviewed exception with a reason. Refs vllm-project#385 Signed-off-by: Abhinandan <abhinandanverma551@gmail.com>
Conversation inserts resolve a stored source row with COALESCE(public_id, reference_id, id), so the plain public_id index was never used and a populated tenant was scanned once per incoming item. Index the lookup expression with tenant_id. With 100k existing items, inserting 100 takes 1 ms instead of 640 ms, and the plan changes from a tenant scan to an index seek. Refs vllm-project#385 Signed-off-by: Abhinandan <abhinandanverma551@gmail.com>
Replace the unrelated single-turn reasoning fixtures with the recorded two-turn OpenAI Responses and conversation recordings, and assert what the history contains rather than that the turns succeeded: forwarded upstream input, stored item order, roles, content, public IDs, and distinct row IDs. Keep the resubmission case as a regression derived from the recording, for both streaming and non-streaming responses. Refs vllm-project#385 Signed-off-by: Abhinandan <abhinandanverma551@gmail.com>
7167a8d to
8e4136d
Compare
Signed-off-by: maral <maralbahari.98@gmail.com>
maralbahari
left a comment
maralbahari
left a comment
There was a problem hiding this comment.
@awesome-pro Thanks for the work and for addressing the review comments! I’ve expanded the tests and pushed a commit to your branch.
input_items excluded a response's own output by the wire IDs in its snapshot. Output kinds whose wire id is optional (compaction, shell_call) get a generated ID when stored, so they were missing from that set and were listed as input. And since vllm-project#455 a public ID can recur in history, so matching by public ID would also drop an earlier turn's output that shares this turn's ID. Persistence now records the row IDs of the rows it inserts as output in ResponseMetadata.output_item_ids, on both the previous_response_id and conversation paths, and input_items matches rows by row ID. Records written before this fall back to the snapshot's public IDs. Regression tests cover an ID-less compaction and an upstream reusing one message ID across turns; each fails when its half of the fix is removed. Deleting a response does not invalidate a WebSocket session that caches it as its latest checkpoint. Checking storage before reusing a cached durable parent was tried and reverted: the extra await reordered automatic steering successors, and the edge-input-lifecycle/old-parent-before-successor replay failed 5 of 6 runs (6/6 on main and after the revert). Sessions are per connection and may be on another replica, so the exception is documented on ResponseStore::delete and in the API docs. Signed-off-by: fengjiaqi <enchiigo@gmail.com>
input_items excluded a response's own output by the wire IDs in its snapshot. Output kinds whose wire id is optional (compaction, shell_call) get a generated ID when stored, so they were missing from that set and were listed as input. And since vllm-project#455 a public ID can recur in history, so matching by public ID would also drop an earlier turn's output that shares this turn's ID. Persistence now records the row IDs of the rows it inserts as output in ResponseMetadata.output_item_ids, on both the previous_response_id and conversation paths, and input_items matches rows by row ID. Records written before this fall back to the snapshot's public IDs. Regression tests cover an ID-less compaction and an upstream reusing one message ID across turns; each fails when its half of the fix is removed. Deleting a response does not invalidate a WebSocket session that caches it as its latest checkpoint. Checking storage before reusing a cached durable parent was tried and reverted: the extra await reordered automatic steering successors, and the edge-input-lifecycle/old-parent-before-successor replay failed 5 of 6 runs (6/6 on main and after the revert). Sessions are per connection and may be on another replica, so the exception is documented on ResponseStore::delete and in the API docs. Signed-off-by: fengjiaqi <enchiigo@gmail.com>
input_items excluded a response's own output by the wire IDs in its snapshot. Output kinds whose wire id is optional (compaction, shell_call) get a generated ID when stored, so they were missing from that set and were listed as input. And since vllm-project#455 a public ID can recur in history, so matching by public ID would also drop an earlier turn's output that shares this turn's ID. Persistence now records the row IDs of the rows it inserts as output in ResponseMetadata.output_item_ids, on both the previous_response_id and conversation paths, and input_items matches rows by row ID. Records written before this fall back to the snapshot's public IDs. Regression tests cover an ID-less compaction and an upstream reusing one message ID across turns; each fails when its half of the fix is removed. Deleting a response does not invalidate a WebSocket session that caches it as its latest checkpoint. Checking storage before reusing a cached durable parent was tried and reverted: the extra await reordered automatic steering successors, and the edge-input-lifecycle/old-parent-before-successor replay failed 5 of 6 runs (6/6 on main and after the revert). Sessions are per connection and may be on another replica, so the exception is documented on ResponseStore::delete and in the API docs. Signed-off-by: fengjiaqi <enchiigo@gmail.com>
input_items excluded a response's own output by the wire IDs in its snapshot. Output kinds whose wire id is optional (compaction, shell_call) get a generated ID when stored, so they were missing from that set and were listed as input. And since vllm-project#455 a public ID can recur in history, so matching by public ID would also drop an earlier turn's output that shares this turn's ID. Persistence now records the row IDs of the rows it inserts as output in ResponseMetadata.output_item_ids, on both the previous_response_id and conversation paths, and input_items matches rows by row ID. Records written before this fall back to the snapshot's public IDs. Regression tests cover an ID-less compaction and an upstream reusing one message ID across turns; each fails when its half of the fix is removed. Deleting a response does not invalidate a WebSocket session that caches it as its latest checkpoint. Checking storage before reusing a cached durable parent was tried and reverted: the extra await reordered automatic steering successors, and the edge-input-lifecycle/old-parent-before-successor replay failed 5 of 6 runs (6/6 on main and after the revert). Sessions are per connection and may be on another replica, so the exception is documented on ResponseStore::delete and in the API docs. Signed-off-by: fengjiaqi <enchiigo@gmail.com>
summary
fixes #385.
stored response history used each item's public id as the
items.idprimary key. a client that sent a previous response'soutputback asinputwithstore: true(the default) hitUNIQUE constraint failed: items.idand a 500, even though the request is valid:item::create_in_txnow gives each response-history occurrence a freshitem_…row id and keeps the public id in a new nullableitems.public_idcolumn.Item::public_id()still returns the public id, so clients see stable ids, and repeated occurrences keep every position. conversation inserts keep their membership checks, sequence allocation, and reference resolution in the same function.conversation inserts, cursors, retrieval and detach resolve an item by public identity (
COALESCE(public_id, reference_id, id)). the lookup is indexed on that expression with the tenant, because a plainpublic_idindex is not usable by the resolved predicate. the join resolves one source row per incoming item first (asource_rowsCTE): several rows can share a public identity, and joining on the identity alone fans out into duplicate row ids, which then fails onitems.id.this supersedes #388, which used the same mechanism but no longer compiles on main and left the conversation resolution untouched. thanks to @RainieLLM for the original approach.
test plan
resubmitted_item_history_testreplays the recorded two-turnOpenAIresponses (text_only/responses/, streaming and non-streaming). it asserts the upstream request the second turn sends, then the rehydrated order, roles, content, public ids, and distinct row ids. the resubmission case is constructed from the same recording and keeps every submitted occurrence.stateful_conversation_integrationasserts the same for the recorded two-turn conversation path (text_only/conversation/, streaming and non-streaming): rehydrated history, public ids, row keys, and the upstream history.conversation_storage_regressionscovers repeated occurrences of one public id, cross-tenant non-reuse, and batch rollback.conversations_api_cassette_testcovers the conversation path, includingedge_replay::gateway_matches_all_openai_edge_case_status_codes_and_bodies, which posts one stored public id twice and caught the fan-out.UNIQUE constraint failed: items.id, and removing the single-source reduction reproducesUNIQUE constraint failed: items.conversation_id, items.seq.SEARCH source USING INDEX idx_items_tenant_idtoSEARCH source USING INDEX idx_items_public_identity (tenant_id=? AND <expr>=?).cargo fmt -- --check,cargo clippy -p agentic-server-core --all-targets -- -D warnings,cargo test --workspace, and the production file-size check.notes
0007_response_item_public_ids.sql. open feat: establish provider-aware reasoning replay foundation #350 also proposes a0007; whichever merges second renumbers to0008.item.rscrosses the 500-line production cap, so.rust-file-sizes.jsonrecords a reviewed exception (510) with the reason.public_id;Item::public_id()falls back toreference_idand thenid, so no backfill is needed.CREATE INDEXsyntax, and the cipostgresjob applies the migration and runs the storage tests.