2 Commits

Author SHA1 Message Date
kongxin
f17e0504d0
fix(feedback): return 409 instead of 500 when feedback is POSTed twice (#8064)
* fix(feedback): return 409 instead of 500 when feedback is POSTed twice

POST /api/threads/{id}/runs/{rid}/feedback did a plain INSERT. A second
POST by the same user for the same run (a retry, or feedback on another
message of that run) hit uq_feedback_thread_run_user and the uncaught
IntegrityError surfaced as HTTP 500.

FeedbackRepository.create now rolls back and raises
FeedbackAlreadyExistsError on that collision, and the route maps it to
409 pointing callers at PUT, which already upserts.

* fix(feedback): only map the run/user unique key to 409; cover concurrent POSTs

Address review on #8064:
- create() now re-reads the (thread, run, user) row after an
  IntegrityError and raises FeedbackAlreadyExistsError only when it
  exists; any other violation (e.g. a feedback_id collision) propagates.
- Add a commit-barrier test where two first POSTs race: one 200, one
  409, and the winner's row is kept.
- Add CHANGELOG.md / CHANGELOG_zh.md Fixed entries.

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
2026-11-10 07:47:01 +08:00
Hyeonsang Cho
4e9f6c68ea
fix(persistence): retry feedback and conversation upserts that lose a race (#6852)
* fix(persistence): retry feedback and conversation upserts that lose a race

FeedbackRepository.upsert and ChannelConnectionRepository.set_thread_id
looked up the row and inserted it when missing, with no conflict
handling. Two first writers for one key both missed the lookup, and the
second INSERT violated uq_feedback_thread_run_user or
uq_channel_conversation_connection_external: a 500 for the second of two
concurrent first ratings, and a failed message when a conversation's
first message raced /new.

Both now catch IntegrityError, roll back and retry, so the losing writer
re-reads the committed row and updates it, as upsert_connection already
does. A retry rather than ON CONFLICT keeps the NULL-user feedback path
updating its single row, since a NULL key never conflicts.

* docs(changelog): reference #6852 for the upsert race fix

* test(persistence): pin upsert retry exhaustion and share the miss-select double

Both retry loops now have a test that makes every SELECT miss against a
pre-seeded row, so all attempts hit the unique key: the original
IntegrityError must propagate after exactly _UPSERT_MAX_ATTEMPTS
attempts and the stored row must stay unchanged.

The "first SELECT misses" session double lived in four copies (one per
retry test, including upsert_connection's). It now lives once in
tests/_repository_race_helpers.py, with a misses count so the same
double drives both the single lost race and exhaustion.
2026-10-14 12:19:26 +08:00