Skip to content

fix: avoid price_history crash when price sync races a full sync - #310

Merged
Git-Romer merged 4 commits into
Git-Romer:mainfrom
hiddenbanana:fix/price-sync-full-sync-race
Jul 27, 2026
Merged

fix: avoid price_history crash when price sync races a full sync#310
Git-Romer merged 4 commits into
Git-Romer:mainfrom
hiddenbanana:fix/price-sync-full-sync-race

Conversation

@hiddenbanana

@hiddenbanana hiddenbanana commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Summary

perform_price_sync can overlap with a full sync. Both jobs update the same
tracked cards and record one price_history row per card and day, while their
long-running sessions commit only after all network work has finished.

The previous SELECT-then-INSERT flow could therefore observe no row, keep a
pending PriceHistory object for the rest of the sync, and then fail the final
commit if another sync inserted the same (card_id, date) first. That discarded
the complete transaction, including unrelated card updates.

This was observed in production:

psycopg2.errors.UniqueViolation: duplicate key value violates unique constraint "uq_price_history_card_date"
DETAIL: Key (card_id, date)=(SV4a-121_ja, 2026-07-26) already exists.

Final implementation

  • Buffer price-history candidates while the sync performs its network work.
  • Serialize only the short final PostgreSQL write phase with a
    transaction-scoped advisory lock.
  • Flush pending card rows before their price-history rows for foreign-key
    safety.
  • Deduplicate and sort cards by ID to guarantee a stable lock order.
  • Write bounded batches of 100 rows using
    INSERT ... ON CONFLICT (card_id, date) DO NOTHING.
  • Use the same sorted, bounded upsert behavior on SQLite.
  • Roll back an aborted price-sync transaction before recording its error state.
  • Update the application version to 1.27.1.
  • Integrate and validate against current 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

  • 281 backend tests passed.
  • Genuine PostgreSQL 18 concurrency test passed with one transaction held
    uncommitted while a second writes the same cards in reverse caller order.
  • The PostgreSQL concurrency test was repeated several times without duplicate
    errors or deadlocks.
  • A 205-card regression test confirms three bounded bulk statements are used
    instead of 205 individual writes.
  • Transaction rollback/error-state coverage passed.
  • 79 frontend tests passed.
  • Backend and frontend Docker builds passed.
  • Production frontend build and version consistency check passed.
  • A PostgreSQL-backed Docker stack started successfully and passed its health
    check.

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).
@Git-Romer

Copy link
Copy Markdown
Owner

@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.
• Rows are written in sorted bulk batches instead of one at a time.
• PostgreSQL only locks the short final write phase.
• Failed transactions now recover cleanly.
• The tests now run two syncs concurrently and cover duplicate prevention, deadlocks, and batching.

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!

@Git-Romer
Git-Romer merged commit 1750c63 into Git-Romer:main Jul 27, 2026
2 checks passed
@hiddenbanana
hiddenbanana deleted the fix/price-sync-full-sync-race branch July 29, 2026 16:22
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.

2 participants