OUT-4081: harden BaseService transaction handling

[SandipBajracharya]

Summary

Hardens DB transaction handling in BaseService and fixes one latent atomicity bug it uncovered.

BaseService swaps a mutable this.db between the connection pool and a per-tx handle via setTransaction/unsetTransaction. Pairing was done by hand at each call site and was inconsistent — one site placed the unset inside the transaction callback, another didn't wrap it in a finally at all (so a throw skipped it, leaving this.db on a closed handle).

What changed

• BaseService.withTransaction(fn, services?) — wraps db.transaction(...) in a finally that restores this and any passed services, so a service can never be left on a dead tx handle. It also binds nested services to the tx explicitly (each BaseService instance has its own this.db, so nested writes are not transactional unless bound).
• Routed all four transaction sites through it: product.service.ts (×2), invoice.service.ts, sync.service.ts.

Bug vs. refactor (honest split)

• fix — sync.service.ts#checkAndSuspendAccount (commit 046bc74): the suspend-account and delete-failed-logs writes went through a fresh TokenService and this.syncLogService that were never bound to the tx — so despite living inside db.transaction(...), they ran on the pool and weren't atomic; unsetTransaction was also skipped on throw. Now both are bound and run sequentially (they share one tx connection). The caller is currently commented out in cron.service.ts, so there's no live prod exposure — this is a latent bug on a dormant path.
• refactor (commits aadbe33, a208761): the helper + unit test, and routing product/invoice through it (behavior-preserving). Also, as small cleanups the helper made cheap: product.created's logSync now writes inside the tx, and the product-map writes are sequential rather than Promise.all on the shared tx connection.

Testing

• New unit test test/unit/core/baseService.withTransaction.test.ts — proves set/unset are paired across this + nested services on both success and throw.
• yarn test — 78 files / 333 tests pass.
• tsc --noEmit (app) and tsc -p test/tsconfig.json clean; yarn lint:check clean.

Not in scope

Only the four existing transaction sites are touched. Pre-existing choice on whether concurrent queries on a reserved postgres-js tx connection are ever safe is left for a future decision.

🤖 Generated with Claude Code

[linear-code]

OUT-4081

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
quickbooks-sync	Building		Aug 20, 2026 12:26pm
quickbooks-sync (dev)	Ready	Preview	Aug 20, 2026 12:26pm

[greptile-apps]

Greptile Summary

The PR centralizes transaction binding and cleanup in BaseService.withTransaction, then migrates the existing product, invoice, and account-suspension transaction paths to it.

• Restores participating services after both successful and failed transactions.
• Binds nested services to the same transaction where atomic writes are required.
• Serializes writes that share a transaction connection.
• Adds unit coverage for binding, restoration, exceptions, and return values.

Confidence Score: 5/5

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

The migrated paths bind all required services to one transaction, execute shared-connection writes sequentially, and reliably restore service database handles after commit or rollback.

Important Files Changed

Filename	Overview
src/app/api/core/services/base.service.ts	Adds a transaction helper that binds participating services to one handle and restores them in an outer finally block.
src/app/api/quickbooks/invoice/invoice.service.ts	Migrates invoice-deletion writes to the helper while preserving their ordering and shared transaction.
src/app/api/quickbooks/product/product.service.ts	Migrates both product transaction sites, serializes product-map writes, and includes sync-log persistence in product-created atomicity.
src/app/api/quickbooks/sync/sync.service.ts	Ensures account suspension and failed-log deletion execute sequentially through the same transaction.
test/unit/core/baseService.withTransaction.test.ts	Covers service binding and restoration on success and failure, plus return-value propagation.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Service as BaseService subclass
    participant DB as Database pool
    participant TX as Transaction
    participant Nested as Nested services

    Caller->>Service: withTransaction(fn, services)
    Service->>DB: transaction(callback)
    DB-->>Service: tx handle
    Service->>Service: setTransaction(tx)
    Service->>Nested: setTransaction(tx)
    Service->>Service: execute fn(tx)
    Service-->>DB: return or throw
    DB-->>Service: commit or rollback
    Service->>Service: unsetTransaction()
    Service->>Nested: unsetTransaction()
    Service-->>Caller: result or error

Loading

_{Reviews (1): Last reviewed commit: "fix(OUT-4081): make checkAndSuspendAccou..." | Re-trigger Greptile}

[priosshrsth]

@SandipBajracharya One more concern I have. Say you ended the transaction. And you use another method from that service, what would happen?

[priosshrsth]

Can we change signature of the function to recieve services as object instead of array. And these services would be passed to fn. So that fn body can call the services from params.
My reasoning is this will help prevent missing the dependency. But let me know yuor opinion on this.

[SandipBajracharya]

I understand the concern and it's quite right to think that way. But even if we use object and pass param to fn(), I dont think we can prevent missing dependency. I think that invites complexity only. I might be wrong though.

[SandipBajracharya]

@SandipBajracharya One more concern I have. Say you ended the transaction. And you use another method from that service, what would happen?

Since the transaction is closed/unset, this.db gets its value reassigned by db instance so its safe. If there were no unsetTransaction in finally block then that would have caused issue.