Skip to content

Fix UNIQUE constraint violation on messages table when resuming conversations#74

Closed
olsonjeffery wants to merge 4 commits into
mainfrom
task/17-failed-to-persist-event---resu
Closed

Fix UNIQUE constraint violation on messages table when resuming conversations#74
olsonjeffery wants to merge 4 commits into
mainfrom
task/17-failed-to-persist-event---resu

Conversation

@olsonjeffery

Copy link
Copy Markdown
Owner

Summary

Fixes a UNIQUE constraint failed: messages.project_key, messages.session_id, messages.seq error that occurs when resuming a conversation (sending a follow-up message). The conversation becomes inert — model response events fail to persist and the stream stalls.

Root Causes

RC1: Reader task ignores cancellation signal — In subscribe_and_spawn(), the event reader task is spawned without receiving the cancellation handle. When resume_turn() cancels the old subscription and creates a new one, the old reader task keeps running. Both reader tasks then call persist_event() for the same session, producing duplicate seq values.

RC2: next_seq() is not atomicnext_seq() performs a separate SELECT MAX(seq) before INSERT, making concurrent calls from overlapping reader tasks produce the same seq value.

Changes

Phase 1: Pass cancellation handle to reader task

  • Added ReaderCancellation using Arc<tokio::sync::Notify> for per-task cancellation independent of the SDK subscription handle
  • Added reader_cancellation.notified() branch to the reader task's tokio::select! loop
  • Extracted cancel_inflight() helper to deduplicate cancellation logic across resume_turn(), abort_turn(), and shutdown()

Phase 2: Make seq generation atomic

  • Removed next_seq() function entirely
  • Inlined atomic seq computation as a subquery in the INSERT INTO messages ... VALUES statement

Refinements

  • Code simplification and clippy fixes (deduplicated patterns, early-return refactors, etc.)

Testing

  • New test_persist_event_concurrent unit test validates 20 concurrent calls produce unique monotonic seq values
  • All 432 unit tests + 9 integration test suites pass

…rsations

- Add ReaderCancellation field and Notify-based cancellation to reader task in subscribe_and_spawn()
- Cancel old reader task and event stream before creating new subscription in resume_turn
- Remove next_seq() and inline atomic seq generation in persist_event() via SQL subquery
- Add test_persist_event_concurrent unit test verifying unique seq under concurrency
- Rebase onto current main (includes timestamp column additions)
…logic

- Extract repeated reader+event cancellation pattern into
- Remove unnecessary intermediate  variable
- Simplify  inner block to direct let bindings
- All 432 unit tests pass, cargo fmt/clippy clean
- Simplified map_sdk_event_to_provider_event by extracting duplicated
  chrono::Utc::now().naive_utc() to a local binding
- Converted nested if/else in SessionStatus handling to early-return
  guard + match on status_type.as_str()
- Extracted client_and_user_id() helper to deduplicate lock-then-clone
  pattern in start_turn and resume_turn
- Added test_provider() test helper to reduce struct construction dup
- Fixed pre-existing clippy warnings: bool_assert_comparison, len_zero,
  needless_borrows_for_generic_args, single_match, needless_borrow
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