[fix] Stop dropping session record batches on duplicate record_ids - #6086
Open
mmabrouk wants to merge 1 commit into
Open
[fix] Stop dropping session record batches on duplicate record_ids#6086mmabrouk wants to merge 1 commit into
mmabrouk wants to merge 1 commit into
Conversation
…he upsert One batched INSERT ... ON CONFLICT DO UPDATE cannot touch the same (project_id, record_id) twice — Postgres raises CardinalityViolationError and the records worker drops the whole batch. The runner legitimately re-sends a record_id within one flush window (partial tool_call frame, then the completed one), so collapse in-batch duplicates into the end state sequential upserts would have produced. Closes #6085 Claude-Session: https://claude.ai/code/session_01AxbkMmS2VvpaxhXPAWzTbH
|
Agenta Team seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Caution Review failedAn error occurred during the review process. Please try again later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Agent session transcripts silently lost tool calls. The session itself ran fine, but when you reopened it in the UI, stretches of the conversation were missing: a tool result with no matching call, or a whole call/result pair gone. On the bighetzner deployment one session lost 28 frames across 8 batches, and the same worker log shows 36 dropped batches across 9 sessions. The records were deleted from the Redis stream after the failure, so the loss was permanent.
Root cause:
RecordsDAO.append_manywrites one multi-rowINSERT ... ON CONFLICT DO UPDATE, and Postgres rejects such a statement when two rows share the conflict key. Duplicate keys inside one batch are not a bug in the producer; they are by design (see the walkthrough). When it happened, the whole batch failed, and the worker had already marked the messages processed, so they were ACKed and deleted.Closes #6085
How the bug unfolds, step by step
Follow one tool call through the pipeline, as if stepping through a debugger:
Runner (sandbox-agent). The harness starts tool call
toolu_011f.... The runner emits a partialtool_callframe immediately, withinput: {}, so the UI can show the call as pending. It derives a stablerecord_idfrom the call id (uuid5), so later frames for the same call upsert onto the same row. It POSTs the frame to the API.API ingest. The API appends the frame to the
streams:recordsRedis stream and returns. ([sessions/persist] ingest OK ... idx=31 type=tool_call.)Runner again, ~5 seconds later. The tool input is complete. The runner emits the completed
tool_callframe with the full input. Same call id, therefore the samerecord_id. This is intentional: the completed frame is meant to overwrite the partial row.RecordsWorker (
worker-streams).process_batchreads up to 50 stream messages in oneXREADGROUPwindow. Both frames from steps 1 and 3 land in the same window because they are only seconds apart. Every message id goes intoprocessed_idsas it is deserialized.RecordsDAO.append_many. All frames become one statement:
Postgres. The statement violates a hard rule of
ON CONFLICT DO UPDATE: one command may not affect the same row twice. Postgres raisesCardinalityViolationError("ON CONFLICT DO UPDATE command cannot affect row a second time") and the whole insert rolls back. Not just the duplicate pair: the usage and tool_result frames that shared the batch roll back with it.RecordsWorker, except branch. The exception is logged ("Failed to append event batch") and swallowed.
process_batchreturnsprocessed_idsunchanged, the consumer ACKs and deletes the messages, and the frames are gone for good.Note the race that decides whether you lose data: if the partial and completed frames land in different flush windows, the second one upserts cleanly onto the first one's row and everything works. The bug only fires when the agent is fast enough (or the worker slow enough) for both frames to share a window. That is why it looked flaky and only bit sessions with rapid back-to-back tool calls.
Changes
append_manynow collapses in-batch duplicates before building the statement, in a newRecordsDAO._dedupe_valueshelper. The collapse reproduces exactly what sequential per-event upserts would have produced:record_type,record_source,timestamp,attributes,turn_id,span_id) take the last occurrence,record_index,session_id, audit columns) keep the first occurrence, becauseON CONFLICT DO UPDATEnever touches them.Stepping through the same run after the fix: steps 1 to 4 are unchanged. In step 5 the two frames for
ac3f1a4e-...merge into one row (record_index31 from the partial, fullattributesfrom the completed frame) before the SQL is built. Postgres sees each conflict key once, the batch commits, and step 7's except branch never runs.Before, the DB ended the day with the whole batch missing. After, it holds the same rows a slow, one-by-one ingest would have produced.
Tradeoffs and notes
set_column list; if someone adds a column to the upsert they must add it to_UPSERT_UPDATED_COLUMNS. The two sit next to each other in the same file to keep that visible.record_indexis deliberate, not an accident of implementation: it matches what the sequential path produced all along (the completed frame never moved the row's index), so UI ordering behavior does not change.process_batchadds message ids toprocessed_idsbefore the insert succeeds. Fixing that means separating "deserialized" from "persisted" and letting failed batches redeliver, which needs idempotency thinking (the upsert gives us most of it). Left as a follow-up on (bug) Session transcripts silently lose tool calls when frames land in one ingestion batch #6085.Tests
api/oss/tests/pytest/unit/sessions/test_records_batch_dedupe.py: duplicate pair collapses to one row with first index and last attributes; distinct ids pass through in order; the same record_id in two projects does not collapse.CardinalityViolationErrorbatches inworker-streamslogs and confirmed each dropped batch contained a partial/completed frame pair sharing a record_id.What to QA
worker-streamslogs during the run: noCardinalityViolationError, no "Failed to append event batch".https://claude.ai/code/session_01AxbkMmS2VvpaxhXPAWzTbH