fix: avoid price_history crash when price sync races a full sync - #310
Conversation
perform_price_sync has no lock guarding it against perform_full_sync (only full-vs-full syncs are guarded via _full_sync_lock), and both accumulate their whole card+price batch in one session that commits once at the end of a run that can take 15+ minutes (full sync) or run every 30 min by default (price sync). record_price_history's SELECT-then-INSERT is blind to a same-day row the other sync commits in the meantime, so the slower sync's final commit fails with a UniqueViolation on (card_id, date) -- discarding its entire batch, not just the colliding row. Confirmed live via sync_log: full sync #650 (started 16:07:46, ~16 min runtime) crashed at its final commit 16:24:08, six seconds after the scheduled price sync #651 (16:23:13-16:24:02) committed a same-day row for one of the same cards mid-flight. Fix: record_price_history now does an atomic INSERT ... ON CONFLICT (card_id, date) DO NOTHING instead of check-then-insert, so a pre-existing same-day row from a concurrent sync is a no-op rather than a crash, regardless of timing. Dialect-aware (postgres in production, sqlite in tests).
|
@hiddenbanana, good catch with this race condition. The production logs made it clear how two overlapping syncs could cause an entire sync to fail at the final commit. I used your ON CONFLICT fix as the foundation and added a few changes: • Price history writes are now collected and saved at the end of the sync. Everything passes against current main, including 281 backend tests, 79 frontend tests, and repeated PostgreSQL concurrency tests. I also updated the version to 1.27.1. The PR is ready now. Your report and initial fix helped turn a difficult intermittent failure into a more reliable and efficient sync process. Thank you! |
Summary
perform_price_synccan overlap with a full sync. Both jobs update the sametracked cards and record one
price_historyrow per card and day, while theirlong-running sessions commit only after all network work has finished.
The previous SELECT-then-INSERT flow could therefore observe no row, keep a
pending
PriceHistoryobject for the rest of the sync, and then fail the finalcommit if another sync inserted the same
(card_id, date)first. That discardedthe complete transaction, including unrelated card updates.
This was observed in production:
Final implementation
transaction-scoped advisory lock.
safety.
INSERT ... ON CONFLICT (card_id, date) DO NOTHING.1.27.1.main.This avoids thousands of per-card database round trips and prevents locks from
being held throughout the long network phase. A completed competing sync simply
makes the later history rows safe no-ops.
Testing
uncommitted while a second writes the same cards in reverse caller order.
errors or deadlocks.
instead of 205 individual writes.
check.