Skip to content

OUT-3710: walk DocNumber + atomic claim to fix QBO duplicate-DocNumber collisions - #250

Merged
SandipBajracharya merged 22 commits into
masterfrom
OUT-3710
May 15, 2026
Merged

OUT-3710: walk DocNumber + atomic claim to fix QBO duplicate-DocNumber collisions#250
SandipBajracharya merged 22 commits into
masterfrom
OUT-3710

Conversation

@SandipBajracharya

@SandipBajracharya SandipBajracharya commented May 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Walk DocNumber on collision. Pre-flight QBO with DocNumber LIKE '<assembly#>%' before createInvoice; pick the lowest free slot in the sequence <n>, <n>-1, …, <n>-10. Stamp PrivateNote: "Assembly invoice: <n>" for cross-reference. On a 6240 race, re-walk once and retry; second 6240 escapes to withErrorHandler and resync. Walker exhaustion now captures to Sentry with portalId + takenCount context.
  • Atomic claim via partial unique index uq_qb_sync_logs_oneshot_active on qb_sync_logs(portal_id, copilot_id, entity_type, event_type) scoped to invoice one-shot events + payment/succeeded. claimWebhookEvent rewritten to INSERT … ON CONFLICT DO NOTHING. Closes the dual-create TOCTOU race documented in memory/project_qb_sync_logs_toctou_parked.md.
  • New qb_doc_number column on qb_invoice_sync (nullable, single-statement ALTER TABLE ADD COLUMN IF NOT EXISTS, no row-lock). Records the DocNumber QBO accepted per invoice. Both insert paths populate it; payment-succeeded flow (webhook + resync cron) now reads from it instead of the Assembly invoice number — preserves correctness when the walker has suffixed.
  • Bug fix in logSync for payment events: qb_sync_logs.invoice_number is now consistently the Assembly invoice number, not the walked DocNumber.

Customer-reported instance: OUT-3683 (Ferguson HR, MFBZU6WM-00002/00003/00004 collisions). Linear ticket: OUT-3710.

Manual deploy steps (operator)

The dedupe of historical qb_sync_logs duplicates is not in the Drizzle migration — it's a one-time SQL run in Supabase before deploy. Order matters: dedupe must run before the migration applies, or CREATE UNIQUE INDEX fails.

  1. Run the dedupe SQL in Supabase (see plan §"Deploy procedure", Step A).
  2. Verify zero remaining duplicates with the count query (Step B).
  3. Merge this PR → Vercel build.sh applies both migrations.
  4. Spot-check Sentry + qb_sync_logs for 30 min post-deploy.
  5. Manual recovery of Ferguson HR's three FAILED rows.
  6. Optional: backfill legacy qb_invoice_sync.qb_doc_number values via a separate script that calls getInvoice per row.

Test plan

  • Run pre-deploy dedupe count query in Supabase — record the row count.
  • Run the dedupe UPDATE SQL in Supabase.
  • Verify zero remaining duplicates in the slice.
  • Sandbox-verify createInvoice accepts no DocNumber when Custom Transaction Numbers is OFF (QBO auto-assigns) and uses our DocNumber when ON.
  • Sandbox-verify DocNumber LIKE 'prefix%' query syntax against QBO V3 query language.
  • Smoke-test in dev: fire invoice.created webhook; confirm the QBO Invoice gets the expected DocNumber + PrivateNote.
  • Smoke-test in dev: fire a duplicate invoice.created for the same copilot_id; confirm the second claim returns { claimed: false }.
  • Smoke-test in dev: fire payment.succeeded for a synced invoice; confirm the QBO Purchase uses qb_doc_number.
  • Smoke-test in dev: fire payment.succeeded for an unsynced invoice; confirm NOT_FOUND propagates and resync retries cleanly.
  • Watch Sentry for new issues tagged area: docnumber-walk-unresolvable (none expected in dev).
  • Post-deploy: monitor qb_sync_logs for new FAILED rows on the CREATED event type — should trend to zero for Ferguson HR-class portals.

Testing Criteria

https://www.loom.com/share/7caa01b20e2c4e14afb6ab477b936a85

🤖 Generated with Claude Code

@linear-code

linear-code Bot commented May 14, 2026

Copy link
Copy Markdown

OUT-3710

@vercel

vercel Bot commented May 14, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
quickbooks-sync Ready Ready Preview, Comment May 15, 2026 9:08am
quickbooks-sync (dev) Ready Ready Preview, Comment May 15, 2026 9:08am

Request Review

@SandipBajracharya SandipBajracharya changed the title fix(OUT-3710): walk DocNumber + atomic claim to fix QBO duplicate-DocNumber collisions OUT-3710: walk DocNumber + atomic claim to fix QBO duplicate-DocNumber collisions May 14, 2026
@greptile-apps

greptile-apps Bot commented May 14, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes QBO duplicate-DocNumber (error 6240) collisions by introducing a pre-flight LIKE query to find the next free DocNumber slot, an atomic webhook claim via a partial unique index with INSERT … ON CONFLICT DO NOTHING, and a new qb_doc_number column on qb_invoice_sync so downstream payment flows use the walked DocNumber rather than the raw Assembly invoice number.

  • DocNumber walker: resolveAvailableDocNumber queries QBO for existing DocNumbers sharing the prefix and picks the lowest free slot (<n>, <n>-1, …<n>-10); a 6240 race triggers one re-walk; exhaustion is captured to Sentry. isQBODuplicateDocNumberError correctly inspects errors[] on the APIError to find the 6240 code that lives in the QBO fault payload (fixing the previously inoperative retry guard noted in prior review).
  • Atomic claim: claimWebhookEvent rewritten to INSERT … ON CONFLICT DO NOTHING backed by a partial unique index scoped to one-shot invoice and payment events; events outside the slice (invoice/updated, product) always insert, preserving prior re-fire behavior.
  • qb_doc_number column: Both webhookInvoiceCreated and findOrMapInvoiceFromQBO populate it; the webhook and resync payment paths read from it with a safe fallback to invoice.number for legacy rows.

Confidence Score: 5/5

The core logic is correct and well-tested; the only deployment risk is the non-concurrent index creation which may briefly block webhook inserts.

The DocNumber walker, isQBODuplicateDocNumberError fix, atomic claim, and qb_doc_number propagation are all implemented correctly and covered by both unit and integration tests. Two minor concerns exist: the index migration uses CREATE UNIQUE INDEX rather than CONCURRENTLY (could block writes briefly), and findInvoicesByDocNumberPrefix skips wrapWithRetry unlike write methods. Neither represents a data-correctness defect.

The index migration (20260514063523_add_oneshot_unique_partial_index_in_qb_sync_logs.sql) deserves a second look before deploying to production due to its write-locking behavior.

Important Files Changed

Filename Overview
src/db/migrations/20260514063523_add_oneshot_unique_partial_index_in_qb_sync_logs.sql Adds the partial unique index for atomic claim; uses CREATE UNIQUE INDEX (not CONCURRENTLY), which will briefly block all INSERTs into qb_sync_logs during deployment on a live table.
src/utils/intuitAPI.ts Adds _findInvoicesByDocNumberPrefix with LIKE-escape and maxresults 100 cap; bound directly (no wrapWithRetry) unlike write methods — a transient 401/429 on the pre-flight call will fail the invoice creation without retry.
src/app/api/quickbooks/invoice/invoice.service.ts Adds DocNumber-walk logic via resolveAvailableDocNumber and a 6240 catch-and-retry path; stores qbDocNumber on the invoice sync row; both the webhook-created and findOrMapInvoiceFromQBO paths are updated correctly.
src/app/api/quickbooks/invoice/invoice.utils.ts New pure-function module with findNextAvailableDocNumber, formatAssemblyInvoicePrivateNote, and isQBODuplicateDocNumberError; well-tested and correctly checks the errors[] array for the 6240 code that lives inside the APIError fault payload.
src/app/api/quickbooks/syncLog/syncLog.service.ts claimWebhookEvent rewritten to INSERT … ON CONFLICT DO NOTHING against the new partial unique index, eliminating the TOCTOU window; behavior for out-of-slice event types (invoice/updated, product) is intentionally pass-through.
src/db/migrations/20260514095346_add_qb_doc_number_column_in_qb_invoice_sync.sql Single-statement ALTER TABLE ADD COLUMN IF NOT EXISTS for the nullable qb_doc_number column; safe for live tables with no lock concern.
src/app/api/quickbooks/payment/payment.service.ts Signature refactored from positional to named parameters; logSync now records the Assembly invoice number (not the walked DocNumber) for qb_sync_logs.invoice_number — correctly fixes the bug noted in the PR description.
src/app/api/quickbooks/sync/sync.service.ts Resync path for payment/succeeded now looks up qbDocNumber from qb_invoice_sync and passes the Assembly invoice number separately to createExpenseForAbsorbedFees; guards added for missing invoiceNumber and missing qb_invoice_sync row.
src/app/api/quickbooks/webhook/webhook.service.ts payment.succeeded handler now fetches qb_invoice_sync row before calling webhookPaymentSucceeded and passes qbDocNumber (with fallback to invoice.number for legacy rows); Assembly invoice NOT_FOUND throws cleanly into the existing error handler.
src/db/schema/qbInvoiceSync.ts Adds nullable qb_doc_number varchar column to QBInvoiceSync schema; matches the migration.
src/db/schema/qbSyncLogs.ts Adds uq_qb_sync_logs_oneshot_active partial unique index definition; predicate correctly matches the SQL migration.
test/integration/quickbooks/syncLog/claimAtomicity.test.ts New integration tests cover atomic claim deduplication for in-slice events, pass-through for out-of-slice events, and soft-delete re-claim; comprehensive coverage of the partial-index behavior.
test/unit/app/api/quickbooks/invoice/invoice.utils.test.ts Thorough unit tests for all three exported utils; isQBODuplicateDocNumberError tests include the real APIError shape with errors[] array that the previous implementation missed.

Sequence Diagram

sequenceDiagram
    participant WH as WebhookService
    participant SL as SyncLogService
    participant INV as InvoiceService
    participant QBO as IntuitAPI (QBO)
    participant DB as qb_invoice_sync

    WH->>SL: claimWebhookEvent(portal, copilot, invoice/created)
    SL->>DB: INSERT ON CONFLICT DO NOTHING
    alt conflict (duplicate already claimed)
        DB-->>SL: 0 rows returned
        SL-->>WH: "claimed=false, return"
    else no conflict
        DB-->>SL: row id
        SL-->>WH: "claimed=true"
    end

    WH->>INV: webhookInvoiceCreated(payload)
    INV->>QBO: findInvoicesByDocNumberPrefix(MFBZU6WM-00002)
    QBO-->>INV: "[{DocNumber:MFBZU6WM-00002}, ...]"
    INV->>INV: "findNextAvailableDocNumber => MFBZU6WM-00002-1"
    INV->>QBO: "createInvoice(DocNumber=MFBZU6WM-00002-1, PrivateNote=Assembly invoice: MFBZU6WM-00002)"
    alt 6240 race on createInvoice
        QBO-->>INV: 6240 Duplicate DocNumber
        INV->>QBO: findInvoicesByDocNumberPrefix (re-walk)
        QBO-->>INV: updated taken set
        INV->>QBO: "createInvoice(DocNumber=MFBZU6WM-00002-2)"
        QBO-->>INV: Invoice created
    else success
        QBO-->>INV: Invoice created
    end
    INV->>DB: "INSERT qb_invoice_sync(qb_doc_number=MFBZU6WM-00002-1)"

    Note over WH,DB: payment.succeeded webhook
    WH->>DB: getInvoiceByNumber(MFBZU6WM-00002)
    DB-->>WH: "qbDocNumber=MFBZU6WM-00002-1"
    WH->>QBO: "createExpense(DocNumber=MFBZU6WM-00002-1)"
Loading

Reviews (2): Last reviewed commit: "chore(OUT-3710): fix lint" | Re-trigger Greptile

Comment thread src/app/api/quickbooks/invoice/invoice.utils.ts
Comment thread test/unit/app/api/quickbooks/invoice/invoice.utils.test.ts Outdated
Comment thread src/app/api/quickbooks/invoice/invoice.service.ts
SandipBajracharya and others added 20 commits May 15, 2026 11:22
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the no-retry registration of getInvoice (bind, not wrapWithRetry)
since the underlying customQuery is already retry-wrapped.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Walks the sequence <base>, <base>-1, …, <base>-99 and returns the first
slot not in the taken set. Hoists the 21-char precondition above the
branching so it fires unconditionally.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Leaves tsc broken at invoice.service.ts:759 until Task 5 lands the
walker + PrivateNote wiring at the call site.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-flight QBO with DocNumber LIKE '<assembly#>%' to detect collisions,
walk to the lowest free slot in the sequence <n>, <n>-1, <n>-2, …. Stamp
PrivateNote: "Assembly invoice: <n>" for cross-reference. On QBO 6240
race, re-walk once and retry; second 6240 escapes to withErrorHandler
and resync.

isQBODuplicateDocNumberError checks .status, .code, and a regex on
.message — JSDoc explains that today only the regex path fires due to
a pre-existing array-access bug in intuitAPI.ts:145.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Indexes (portal_id, copilot_id, entity_type, event_type) with a partial
WHERE filter limited to invoice one-shot events (created/paid/voided/
deleted) plus all payment events. Excludes invoice/updated, product,
and price rows where multi-fire is legitimate.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dedupe of historical duplicates is a separate manual SQL run by the
operator before this migration applies (see plan doc, "Deploy procedure"
section).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the check-then-insert TOCTOU pattern with a single atomic
INSERT … ON CONFLICT (cols) WHERE <partial-predicate> DO NOTHING.
The predicate uses bare column names matching the migration's index
predicate verbatim so PG can recognize the implication.

For rows outside the partial-index slice (invoice/updated, product,
price), INSERT always succeeds and claim returns true — matching the
prior non-atomic behavior where legitimate multi-fire is expected.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six scenarios: duplicate invoice/created blocked, created vs paid both
claimed, soft-delete unblocks re-claim, invoice/updated multi-fire
allowed, product/updated multi-fire allowed, duplicate payment/
succeeded blocked. Runtime confirms the ON CONFLICT predicate matches
the partial unique index uq_qb_sync_logs_oneshot_active.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Cap PrivateNote at QBO's 4000-char max in the Zod schema so over-long
  inputs fail validation locally rather than as a remote QBO error.
- Trailing newline on the migration file.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Partial unique index and atomic claim now cover payment/succeeded
specifically rather than all payment event types. Today this is
behaviorally equivalent (succeeded is the only payment event the
codebase fires), but the narrower predicate matches the documented
intent and prevents future PAYMENT/* event types from being silently
deduplicated.

Migration regenerated with new timestamp after the prior file was
removed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single ALTER TABLE ADD COLUMN — metadata-only, no row rewrite, no lock
on large tables. Column is nullable so legacy rows aren't forced through
an in-migration UPDATE. Application writes always populate the column
for new invoices; legacy rows can be backfilled by a separate one-time
script via getInvoice, decoupled from deploy timing.

Stores the DocNumber QBO accepted. Equals invoice_number in the no-
collision happy path; differs when the walker suffixed (e.g.,
MFBZU6WM-00002-1) to avoid a 6240 duplicate.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaced by 20260514063523_… (already in c9f92e5) which narrowed the
payment scope to event_type='succeeded'. The original was never applied
to any database we care about.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- captureSyncError helper in @/utils/sentry mirrors the existing
  addSyncBreadcrumb pattern.
- resolveAvailableDocNumber wraps findNextAvailableDocNumber with a
  try/catch that captures (with portalId, assemblyInvoiceNumber,
  takenCount context) and re-throws. The existing FAILED-sync_log path
  still records the row; Sentry surfaces it for engineering attention
  since walker exhaustion is unrecoverable by resync.
- Reduced MAX_SUFFIX_ATTEMPTS from 99 -> 10. Ten collisions on a single
  Assembly invoice number is already an extreme anomaly; surfacing
  earlier means alerts fire sooner without spending budget walking
  through dozens of futile suffixes.
- Populate qb_doc_number on both qb_invoice_sync inserts:
  - webhookInvoiceCreated: qbDocNumber: docNumber (walker-resolved)
  - findOrMapInvoiceFromQBO: qbDocNumber: invoiceNumber (exact match)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both the live webhook path (webhook.service.ts) and the resync cron path
(sync.service.ts) now resolve the canonical qb_doc_number from
qb_invoice_sync and pass it to createExpenseForAbsorbedFees. Previously
each used the Assembly invoice number directly, which would be wrong
when the walker had suffixed the DocNumber.

Legacy rows (created before the qb_doc_number column was added) will
have qbDocNumber=null until backfilled. Both paths fall back to the
Assembly invoice number in that case, matching the pre-OUT-3710
behavior.

- webhookPaymentSucceeded and createExpenseForAbsorbedFees refactored
  to named-args. The latter now stores the explicit invoiceNumber on
  the sync_log row instead of payload.DocNumber, so qb_sync_logs.
  invoice_number is consistently the Assembly invoice number for
  PAYMENT/SUCCEEDED rows.
- Both paths throw if the qb_invoice_sync mapping is missing:
  - Webhook path: APIError(NOT_FOUND) for HTTP semantics.
  - Sync cron path: plain Error since the cron has no HTTP response.
- Stale unused imports (APIError, httpStatus, InvoiceResponse,
  QBInvoiceSelectSchemaType) cleaned up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile review found the predicate was silently inoperative — APIError
thrown by intuitAPI._createInvoice carries status=400, a boilerplate
message, and the QBO fault payload in `errors[]`. None of the prior
checks (.status/.code/.message) matched the real shape, so every 6240
race fell through to FAILED + resync instead of triggering the intended
single re-walk retry.

Predicate now iterates the `errors` array and matches `code === '6240'`
or "Duplicate Document Number" in Detail/Message. Top-level .status/
.code/.message checks remain as defense-in-depth for non-APIError
shapes or future call sites that rethrow the inner fault directly.

Unit tests updated to use the real production shape:
  { status: 400, message: '#IntuitAPIErrorMessage#createInvoice',
    errors: [{ code: '6240', Detail: '…', Message: '…' }] }

Pre-existing array-access bug in intuitAPI.ts:145 (treating Fault.Error
as object) is out of scope for OUT-3710; predicate now handles the
broken upstream shape correctly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Exporting MAX_SUFFIX_ATTEMPTS and using it in the test removes the
fragility where the test filled 1–99 against a cap that is now 10.
If the cap is ever raised above the hard-coded fill size, the test
would silently turn green on the wrong outcome (function finding a
free slot past the fill range instead of throwing). Now the test
fills exactly up to the cap, so any change to MAX_SUFFIX_ATTEMPTS
keeps the boundary aligned.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OUT-3543 narrowed customQuery's return type from any to unknown and
introduced QBInvoiceQueryResponseSchema for parsing list responses,
matching how _getInvoice already worked. Two spots in this branch
were lagging:

- _findInvoicesByDocNumberPrefix previously accessed response.Invoice
  directly, which no longer typechecks against unknown. Switched to
  QBInvoiceQueryResponseSchema.parse(response) mirroring _getInvoice's
  pattern. The schema is already imported.
- The createInvoice unit tests in intuitAPI.responses.test.ts built
  payloads without PrivateNote, which is now required by
  QBInvoiceCreatePayloadSchema (OUT-3710 commit 40a5052). Added the
  canonical Assembly-invoice fixture string.

No behavior change; both were typecheck-only regressions surfaced by
running tsc against the rebased branch.

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

Copy link
Copy Markdown
Collaborator Author

@greptileai

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

I do have feedback regarding test. But you can handle this separately since we might need to rephase our tests anyway.

})

describe('invoice one-shot events (covered by partial unique index)', () => {
it('returns claimed=true for the first call and claimed=false for the duplicate (invoice/created)', async () => {

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.

describe: "invoice.created event comes in"

it ("claims event")

it ("do not claim duplicate events / does not cliam already caimed events")

Also not sure what claim means. Maybe goal is important than the response.

So maybe, log is created and log is not created would be better test.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Will resolve this in coming days. Thanks for the suggestion.

@SandipBajracharya
SandipBajracharya merged commit f8d36fd into master May 15, 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