Skip to content

OUT-3644: add idempotency guards on qb_invoice_sync writes - #236

Merged
SandipBajracharya merged 2 commits into
masterfrom
OUT-3644
Apr 27, 2026
Merged

OUT-3644: add idempotency guards on qb_invoice_sync writes#236
SandipBajracharya merged 2 commits into
masterfrom
OUT-3644

Conversation

@SandipBajracharya

Copy link
Copy Markdown
Collaborator

Summary

  • Partial unique index on qb_invoice_sync (portal_id, invoice_number) WHERE deleted_at IS NULL + onConflictDoNothing in createQBInvoice — concurrent webhook deliveries can no longer insert duplicate mapping rows.
  • Inline re-check guard in findOrMapInvoiceFromQBO after the QBO fetch; race-loss path skips redundant logSync.
  • getOneByCopilotIdAndEventType and the conditions branch of updateOrCreateQBSyncLog now filter isNull(deletedAt) so soft-deleted log rows can't be revived in-place.

qb_sync_logs unique index was descoped — production has too many historical duplicates to add the constraint cleanly. Webhook-entry idempotency tracked in OUT-3655.

Linear: https://linear.app/assemblycom/issue/OUT-3644

Test plan

  • Apply migration in dev, confirm partial unique index exists on qb_invoice_sync
  • Replay two invoice.created webhooks in parallel for the same invoice — verify only one qb_invoice_sync row, only one qb_sync_logs CREATED row
  • Soft-delete a qb_sync_logs row, then trigger a sync that would normally update it — verify a new row is created instead of reviving the soft-deleted one
  • Existing sync flows (create / paid / voided / deleted) regress-test against QBO sandbox

🤖 Generated with Claude Code

Concurrent webhook deliveries for the same invoice could both pass the
existence check and insert duplicate qb_invoice_sync rows. Add a partial
unique index on (portal_id, invoice_number) WHERE deleted_at IS NULL,
swallow conflicts in createQBInvoice via onConflictDoNothing, re-check
the mapping inside findOrMapInvoiceFromQBO after the QBO fetch, and
skip the redundant logSync when the race is lost. Also filter
isNull(deletedAt) in the sync-log lookups so updateOrCreateQBSyncLog
can't revive a soft-deleted log row in place.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@linear-code

linear-code Bot commented Apr 27, 2026

Copy link
Copy Markdown

@vercel

vercel Bot commented Apr 27, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
quickbooks-sync Building Building Apr 27, 2026 7:12am
quickbooks-sync (dev) Ready Ready Preview, Comment Apr 27, 2026 7:12am

Request Review

@greptile-apps

greptile-apps Bot commented Apr 27, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds idempotency guards to qb_invoice_sync writes: a partial unique index on (portal_id, invoice_number) WHERE deleted_at IS NULL, onConflictDoNothing in createQBInvoice, and an inline re-check in findOrMapInvoiceFromQBO. It also fixes qb_sync_logs queries to exclude soft-deleted rows.

  • P1 — webhookInvoiceCreated path (line 788): createQBInvoice is called but its return value is not checked. If onConflictDoNothing silently skips the INSERT (race loss), logSync still runs with the losing webhook's freshly-created-but-now-orphaned QBO invoice ID, causing qb_sync_logs.quickbooks_id to diverge from qb_invoice_sync.qb_invoice_id. The findOrMapInvoiceFromQBO path handles this correctly with if (!inserted) return — the same guard is needed here.

Confidence Score: 3/5

Not safe to merge as-is — the webhookInvoiceCreated path has a missing race-loss guard that can corrupt sync log data.

A single P1 finding on the primary webhook code path caps the score at 4, and the inconsistency between two code paths handling the same race (one correctly, one not) warrants pulling the score below the ceiling.

src/app/api/quickbooks/invoice/invoice.service.ts — specifically the webhookInvoiceCreated path around line 788 where the createQBInvoice result is not checked before logSync is called.

Important Files Changed

Filename Overview
src/app/api/quickbooks/invoice/invoice.service.ts Adds onConflictDoNothing to createQBInvoice and inline re-check guard in findOrMapInvoiceFromQBO; the findOrMapInvoiceFromQBO path correctly checks the insert result, but the webhookInvoiceCreated path (line 788) does not, leaving logSync exposed to a race-loss scenario where it writes an inconsistent quickbooksId to the sync log.
src/app/api/quickbooks/syncLog/syncLog.service.ts Correctly adds isNull(deletedAt) filter to both getOneByCopilotIdAndEventType and the conditions branch of updateOrCreateQBSyncLog, preventing soft-deleted log rows from being revived in-place.
src/db/migrations/20260427055352_add_unique_index_with_portal_id_and_invoice_number_in_qb_invoice_sync.sql Adds a partial unique index on (portal_id, invoice_number) WHERE deleted_at IS NULL; SQL is correct and matches the Drizzle schema definition.
src/db/schema/qbInvoiceSync.ts Schema correctly adds a uniqueIndex with WHERE isNull(deletedAt) matching the migration SQL.
src/db/migrations/meta/20260427055352_snapshot.json Drizzle snapshot updated to reflect the new partial unique index on qb_invoice_sync; consistent with the migration and schema.
src/db/migrations/meta/_journal.json Journal correctly registers the new migration entry with sequential idx and matching tag.

Comments Outside Diff (1)

  1. src/app/api/quickbooks/invoice/invoice.service.ts, line 788-805 (link)

    P1 Race-loss path still calls logSync with a dangling QBO invoice ID

    When onConflictDoNothing silently skips the INSERT at line 788 (race loss), logSync is still called at line 792 with qbInvoiceId from this webhook's freshly-created QBO invoice — which is now untracked in the local DB. logSyncupdateOrCreateQBSyncLog (no conditions) → getOneByCopilotIdAndEventType will find the winning webhook's existing CREATED log and UPDATE its quickbooksId to the losing webhook's orphaned QBO invoice ID, making qb_invoice_sync.qb_invoice_id and qb_sync_logs.quickbooks_id point to two different QBO objects.

    The findOrMapInvoiceFromQBO path handles this correctly with if (!inserted) return .... The same guard is needed here:

    const inserted = await this.createQBInvoice(invoicePayload)
    if (!inserted) {
      // Race loss: mapping already created by the winner. Skip logSync.
      return
    }
    
    // update/ create the record in sync log table

Reviews (1): Last reviewed commit: "fix(OUT-3644): add idempotency guards on..." | Re-trigger Greptile

…eCreated

When createQBInvoice's onConflictDoNothing skips the insert (concurrent
delivery won the race), logSync would still run and overwrite the
winner's CREATED log row with this webhook's qbInvoiceId, leaving
qb_invoice_sync.qb_invoice_id and qb_sync_logs.quickbooks_id pointing
at different QBO objects. Same hazard applied to the paid-path payment
creation. Mirror the guard already in findOrMapInvoiceFromQBO: capture
the insert result and short-circuit on race loss.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@SandipBajracharya SandipBajracharya changed the title fix(OUT-3644): add idempotency guards on qb_invoice_sync writes OUT-3644: add idempotency guards on qb_invoice_sync writes Apr 27, 2026

@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.

src/app/api/quickbooks/invoice/invoice.service.ts is becoming too large, don't you think?
Approved. But maybe we should also think about simplifying this in the future.

@SandipBajracharya
SandipBajracharya merged commit 8b0b4d5 into master Apr 27, 2026
4 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