Skip to content

fix(OUT-3971): eliminate flaky synced_contacts duplicate-key race - #73

Merged
SandipBajracharya merged 3 commits into
mainfrom
OUT-3971
Aug 5, 2026
Merged

fix(OUT-3971): eliminate flaky synced_contacts duplicate-key race#73
SandipBajracharya merged 3 commits into
mainfrom
OUT-3971

Conversation

@SandipBajracharya

Copy link
Copy Markdown
Collaborator

Summary

Fixes OUT-3971. The integration suite intermittently failed (~1 in 5–7 full-suite runs) with a duplicate key on uq_synced_contacts_portal_id_tenant_id_client_or_company_id.

Root cause (not a cross-test leak): createMissingXeroInvoice resolved the contact concurrently with syncInvoiceToXero, which already resolves it internally — two non-atomic get-or-creates for the same client racing on the unique index. Reproduced deterministically: every duplicate-key failure was a "missing Xero invoice" test (invoice.paid, invoice.voided, payment.succeeded), all funnelling through createMissingXeroInvoice.

Is it a real production race? Yes, but low impact — the unique index rejects the duplicate row (no corruption); the losing sync becomes a retryable failed_sync. Prod check found only 2 lifetime occurrences (~2 orphaned Xero contacts), so no advisory lock was added.

Changes

  • Layer 1 — removes the self-race (fixes the flake). createMissingXeroInvoice no longer fetches the contact a second time; syncInvoiceToXero now returns the customerName/customerEmail it already resolved. A guarded, sequential fallback fetch keeps the sync log populated on the already-synced short-circuit (cannot race — it runs after the sync completes).
  • Layer 2 — atomic get-or-create (hardening). createContact inserts with onConflictDoNothing on the unique index and re-selects the winner on conflict, so genuinely concurrent syncs degrade to a lookup instead of a duplicate-key failure. No migration (uses the existing index).

Not covered (intentional)

Two concurrent requests can still both call xero.createContact before either inserts, leaving an orphaned Xero contact. Closing that fully needs a pg_advisory_xact_lock around the get-or-create; deferred given only 2 lifetime events. The DB never points at the orphan, so transactions never split across contacts.

Testing

  • tsc --noEmit clean, biome clean, full suite 61/61.
  • Stress: 30/30 + 12/12 full-suite runs, zero duplicate-key failures (was ~2/15 before).
  • New createContactIdempotency test proves a duplicate insert returns the existing contact with no second row or sync log; missingXeroInvoice test now asserts the contact is resolved exactly once (self-race regression guard).

🤖 Generated with Claude Code

SandipBajracharya and others added 3 commits August 5, 2026 15:00
…g-invoice sync

createMissingXeroInvoice resolved the contact concurrently with
syncInvoiceToXero, which already resolves it, so two get-or-creates raced
on the synced_contacts unique index and intermittently threw a duplicate
key. Reuse the contact syncInvoiceToXero returns, with a fallback fetch
for the already-synced short-circuit so the sync log stays populated.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Insert synced_contacts with onConflictDoNothing on the unique index and
re-select the winner on conflict, returning its contact and skipping the
sync log. Genuinely concurrent syncs for the same client now degrade to a
lookup instead of a duplicate-key failure.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Assert the missing-invoice path resolves the contact exactly once (guards
the self-race regression), and add an integration test proving createContact
returns the existing mapping on a duplicate insert without a second row or
sync log.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Aug 5, 2026

Copy link
Copy Markdown

OUT-3971

@vercel

vercel Bot commented Aug 5, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
xero-integration Ready Ready Preview Aug 5, 2026 9:27am

Request Review

@supabase

supabase Bot commented Aug 5, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project pkdwtcdqcefmlgxmcwmc because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@greptile-apps

greptile-apps Bot commented Aug 5, 2026

Copy link
Copy Markdown

Greptile Summary

The PR removes duplicate contact resolution from missing-invoice handling and makes the contact mapping insert tolerate concurrent winners.

  • Returns resolved customer details from the normal invoice-sync path for reuse in sync logging.
  • Uses the existing synced-contact unique index for conflict handling and re-selects the winning contact mapping.
  • Adds integration coverage for conflict idempotency and single contact resolution.

Confidence Score: 5/5

The PR appears safe to merge with no concrete changed-code defect identified.

The conflict target matches the existing unique index, the winner lookup uses the same identity columns, and successful missing-invoice paths retain customer details without a second concurrent contact resolution.

Important Files Changed

Filename Overview
src/features/invoice-sync/lib/SyncedContacts.service.ts Makes mapping creation conflict-tolerant using the existing matching unique index and returns the concurrently stored contact ID.
src/features/invoice-sync/lib/SyncedInvoices.service.ts Reuses contact details resolved during invoice synchronization and performs a sequential fallback for an already-synced invoice.
test/integration/syncedContacts/createContactIdempotency.test.ts Verifies that a conflicting mapping returns the existing contact without adding another row or creation log.
test/integration/webhook/invoicePaid/missingXeroInvoice.test.ts Adds a regression assertion that missing-invoice handling creates or resolves the contact only once.

Sequence Diagram

sequenceDiagram
    participant W as Webhook
    participant I as SyncedInvoicesService
    participant C as SyncedContactsService
    participant X as Xero
    participant D as Database

    W->>I: create missing Xero invoice
    I->>C: resolve contact once
    C->>X: create contact if missing
    C->>D: INSERT mapping ON CONFLICT DO NOTHING
    alt mapping inserted
        D-->>C: inserted contact ID
        C->>D: write customer sync log
    else concurrent mapping won
        D-->>C: no inserted row
        C->>D: select winning mapping
        D-->>C: winning contact ID
    end
    C-->>I: resolved contact
    I->>X: create invoice
    I->>D: write invoice sync log with resolved customer details
    I-->>W: synced invoice record
Loading

Reviews (1): Last reviewed commit: "test(OUT-3971): cover single contact res..." | Re-trigger Greptile

@priosshrsth priosshrsth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@SandipBajracharya
SandipBajracharya merged commit 2f4458a into main Aug 5, 2026
7 checks passed
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