Skip to content

fix(copilot): make user-message persistence idempotent on message_id#4811

Closed
waleedlatif1 wants to merge 1 commit into
stagingfrom
waleedlatif1/copilot-idempotent-user-append
Closed

fix(copilot): make user-message persistence idempotent on message_id#4811
waleedlatif1 wants to merge 1 commit into
stagingfrom
waleedlatif1/copilot-idempotent-user-append

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

What

persistUserMessage (apps/sim/lib/copilot/chat/post.ts) appended the user message to copilot_chats.messages unconditionallymessages || [userMsg]::jsonb — with no check that the id already exists. This change guards the append so it only concatenates when the array doesn't already contain a message with that id.

Why (root cause, verified)

A repeated POST for the same userMessageId (network retry, double-submit, React double-effect, or the send lock being bypassed when Redis is down / its 60s TTL expires) tacked on a duplicate array entry. The assistant append in terminal-state.ts is already guarded by a findIndex check; the user append was not — so duplicates only ever accrue on user messages.

Evidence: across prod, 100% of duplicated message_ids are role:"user" (zero assistant) — exactly what this asymmetry predicts. A deep trace of the client hook across all revisions confirmed the client mints a fresh id per distinct send (queued sends, queue-edit, retry, recovery, replay all verified), so a repeated id reaching the server is always the same message. The unconditional server append was the sole mechanism creating the duplicates.

The fix

messages = CASE
  WHEN messages @> '[{"id": <userMessageId>}]'::jsonb THEN messages
  ELSE messages || '[<userMsg>]'::jsonb
END
  • Safe: because the client never sends different content under a reused id, skipping on a present id can only ever suppress an identical retry — it never drops a distinct message.
  • conversationId / updatedAt stay unconditional, so the active-stream marker still advances on a retry.
  • The dual-write to copilot_messages is already idempotent (ON CONFLICT DO UPDATE), so no change needed there.

Scope

Deliberately tiny — one SQL expression in persistUserMessage. No client change (the client is already correct). No schema change.

Verification

  • Semantics proven on real Postgres: starting from [{id:A}], appending id A → stays 1 element; appending id B → 2 elements.
  • bun vitest run lib/copilot/chat — 68 tests pass; type-check clean; biome clean; check:api-validation passes.

Note

This stops new duplicates at the source. Historical duplicates (67 chats, mostly Sept–Oct 2025) are a separate, optional cleanup; they still live intact in the JSONB column.

🤖 Generated with Claude Code

persistUserMessage appended to copilot_chats.messages unconditionally
(messages || [userMsg]::jsonb) with no check that the id already exists.
A repeated POST for the same userMessageId — network retry, double
submit, React double-effect, or a bypassed/expired send lock (Redis-down
no-op) — therefore tacked on a duplicate array entry. The assistant
append in terminal-state.ts is already guarded; this brings the user
append to parity. (Evidence: 100% of duplicated message_ids in prod are
role=user.)

Guard the append with a containment check so it only concatenates when
the array doesn't already hold a message with this id:

  CASE WHEN messages @> [{id}] THEN messages
       ELSE messages || [userMsg] END

The client mints a fresh id per distinct send (verified across all hook
revisions), so a repeated id is always the same message — skipping is
safe and never drops a distinct message. conversationId/updatedAt stay
unconditional so the stream marker still advances on a retry. The
dual-write to copilot_messages is already idempotent (ON CONFLICT).

Verified on Postgres: same id -> stays 1 element; different id -> 2.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 30, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 30, 2026 6:01am

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 30, 2026

PR Summary

Low Risk
Single SQL expression in chat persistence with no auth or schema changes; retries only skip duplicate identical user ids.

Overview
persistUserMessage in post.ts no longer always appends the user turn to copilot_chats.messages. It uses a Postgres CASE with JSONB @> so the array is only extended when it does not already contain an object with the same userMessageId.

conversationId and updatedAt still update on every call, so retries keep the active stream marker fresh without duplicating user rows in the JSONB history (aligning user persistence with the assistant path’s existing de-duplication behavior).

Reviewed by Cursor Bugbot for commit a863c99. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 30, 2026

Greptile Summary

This PR fixes a duplicate-entry bug in persistUserMessage by wrapping the JSONB array append in a CASE WHEN … @> … THEN … ELSE … expression so that a repeated userMessageId (from a network retry, double-submit, or Redis TTL expiry) is silently skipped instead of appended again. The conversationId and updatedAt fields are still written unconditionally, keeping the active-stream marker correct on retries.

  • The SQL containment check (@>) is correct Postgres idiom and has been verified on real data; appendCopilotChatMessages remains safe because the dual-write table already uses ON CONFLICT DO UPDATE.
  • The change is deliberately minimal — one SQL expression, no schema change, no client change — and all 68 existing tests continue to pass.

Confidence Score: 4/5

The SQL guard is correct and the change is intentionally minimal — safe to merge with one observability gap to consider.

The JSONB containment check works correctly in Postgres, conversationId/updatedAt are still written on every call, and the dual-write path is already idempotent. The only gap is that the CopilotChatPersistOutcome.Appended trace value is emitted for both genuine appends and suppressed retries, making it impossible to confirm in production telemetry whether the new guard is actually firing. No data correctness issue exists.

apps/sim/lib/copilot/chat/post.ts — the trace outcome on the THEN branch of the CASE expression.

Important Files Changed

Filename Overview
apps/sim/lib/copilot/chat/post.ts Adds a JSONB containment check to make user-message persistence idempotent; the trace outcome is still emitted as Appended even on suppressed retries, losing observability of the guard.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Server
    participant DB as copilotChats
    participant DW as copilotMessages

    Client->>Server: POST userMessageId + message
    Server->>DB: UPDATE CASE WHEN messages contains id THEN keep ELSE append END
    DB-->>Server: updated row
    Server->>DW: appendCopilotChatMessages ON CONFLICT DO UPDATE
    DW-->>Server: ok

    Note over Client,Server: Retry with same userMessageId
    Client->>Server: POST same userMessageId
    Server->>DB: UPDATE CASE WHEN detects existing id THEN messages unchanged
    DB-->>Server: updated row stream marker advanced
    Server->>DW: appendCopilotChatMessages idempotent no-op
    DW-->>Server: ok
Loading

Comments Outside Diff (1)

  1. apps/sim/lib/copilot/chat/post.ts, line 357-359 (link)

    P2 Trace outcome is misleading on idempotent retries

    When the CASE WHEN takes the THEN branch (duplicate id detected, no append), the row is still updated (conversationId / updatedAt are unconditional), so updated is truthy and the span emits CopilotChatPersistOutcome.Appended. This conflates a suppressed-retry write with a genuine first-append, making it impossible to observe the guard firing in production telemetry without adding a new outcome (e.g. Deduplicated) to the generated enum and comparing updated.messages length to detect the no-op CASE path.

Reviews (1): Last reviewed commit: "fix(copilot): make user-message persiste..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant