From aadbe33aa51bc79d036e916006d5edecffafb9ac Mon Sep 17 00:00:00 2001 From: SandipBajracharya Date: Thu, 20 Aug 2026 16:38:27 +0545 Subject: [PATCH 1/4] refactor(OUT-4081): add BaseService.withTransaction for safe tx set/unset pairing Wraps db.transaction in a finally that restores this (and any passed services), so a service is never left on a closed tx handle. Adds a unit test covering restore on success and on throw. Co-Authored-By: Claude Opus 4.8 --- src/app/api/core/services/base.service.ts | 20 +++++ .../core/baseService.withTransaction.test.ts | 82 +++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 test/unit/core/baseService.withTransaction.test.ts diff --git a/src/app/api/core/services/base.service.ts b/src/app/api/core/services/base.service.ts index 16c40293..cc84839f 100644 --- a/src/app/api/core/services/base.service.ts +++ b/src/app/api/core/services/base.service.ts @@ -24,4 +24,24 @@ export class BaseService { unsetTransaction() { this.db = db } + + /** + * Runs `fn` in a transaction with this service (and any passed `services`) + * bound to the tx, restored in a `finally` that wraps the transaction so none + * is left on a closed handle. + */ + protected async withTransaction( + fn: (tx: PostgresJsDatabase) => Promise, + services: BaseService[] = [], + ): Promise { + const bound = [this, ...services] + try { + return await this.db.transaction(async (tx) => { + bound.forEach((service) => service.setTransaction(tx)) + return await fn(tx) + }) + } finally { + bound.forEach((service) => service.unsetTransaction()) + } + } } diff --git a/test/unit/core/baseService.withTransaction.test.ts b/test/unit/core/baseService.withTransaction.test.ts new file mode 100644 index 00000000..a611d093 --- /dev/null +++ b/test/unit/core/baseService.withTransaction.test.ts @@ -0,0 +1,82 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +// Hoisted so the vi.mock factory (also hoisted) can reference these safely. +const { singletonDb, FAKE_TX } = vi.hoisted(() => { + const FAKE_TX = { __tx: true } + return { + FAKE_TX, + // Runs the callback with FAKE_TX, re-throwing on error like drizzle. + singletonDb: { + transaction: vi.fn(async (cb: (tx: unknown) => Promise) => + cb(FAKE_TX), + ), + }, + } +}) +vi.mock('@/db', () => ({ db: singletonDb, client: {} })) + +import { BaseService } from '@/app/api/core/services/base.service' +import User from '@/app/api/core/models/User.model' + +// Subclass to reach the protected members under test. +class TestService extends BaseService { + get currentDb() { + return this.db + } + run(fn: () => Promise, services: BaseService[] = []) { + return this.withTransaction(() => fn(), services) + } +} + +const makeService = () => new TestService({} as User) + +describe('BaseService.withTransaction', () => { + beforeEach(() => vi.clearAllMocks()) + + it('points this.db at the tx during fn and restores it after success', async () => { + const service = makeService() + expect(service.currentDb).toBe(singletonDb) + + let dbDuringFn: unknown + await service.run(async () => { + dbDuringFn = service.currentDb + }) + + expect(dbDuringFn).toBe(FAKE_TX) + expect(service.currentDb).toBe(singletonDb) // restored + }) + + it('also binds and restores extra services passed in', async () => { + const service = makeService() + const nested = makeService() + + let nestedDbDuringFn: unknown + await service.run(async () => { + nestedDbDuringFn = nested.currentDb + }, [nested]) + + expect(nestedDbDuringFn).toBe(FAKE_TX) // nested joined the tx + expect(nested.currentDb).toBe(singletonDb) // restored + }) + + it('restores every bound service even when fn throws', async () => { + const service = makeService() + const nested = makeService() + + await expect( + service.run(async () => { + throw new Error('boom') + }, [nested]), + ).rejects.toThrow('boom') + + // The finally wraps the transaction, so neither is left on a dead handle. + expect(service.currentDb).toBe(singletonDb) + expect(nested.currentDb).toBe(singletonDb) + }) + + it('returns the value produced inside the transaction', async () => { + const service = makeService() + const result = await service.run(async () => 'done') + expect(result).toBe('done') + }) +}) From a2087616be44dae686a2044b87c8bde8089f9ce4 Mon Sep 17 00:00:00 2001 From: SandipBajracharya Date: Thu, 20 Aug 2026 16:38:30 +0545 Subject: [PATCH 2/4] refactor(OUT-4081): route product and invoice tx sites through withTransaction Behavior-preserving. Also binds syncLogService so product.created's logSync write is atomic with the mapping, and makes the product-map writes sequential since they now share one tx connection. Co-Authored-By: Claude Opus 4.8 --- .../api/quickbooks/invoice/invoice.service.ts | 55 +++-- .../api/quickbooks/product/product.service.ts | 204 +++++++++--------- 2 files changed, 120 insertions(+), 139 deletions(-) diff --git a/src/app/api/quickbooks/invoice/invoice.service.ts b/src/app/api/quickbooks/invoice/invoice.service.ts index 13d985f9..72aaea6c 100644 --- a/src/app/api/quickbooks/invoice/invoice.service.ts +++ b/src/app/api/quickbooks/invoice/invoice.service.ts @@ -1108,39 +1108,32 @@ export class InvoiceService extends BaseService { console.info( 'InvoiceService#handleInvoiceDeleted | Invoice absent from QBO. Soft-deleting logs, marking local mapping as DELETED, and recording pre-soft-deleted DELETED event.', ) - try { - await this.db.transaction(async (tx) => { - this.setTransaction(tx) - this.syncLogService.setTransaction(tx) - const now = new Date() - await this.syncLogService.softDeleteLogsByCopilotId( - payload.id, - EntityType.INVOICE, - now, + await this.withTransaction(async () => { + const now = new Date() + await this.syncLogService.softDeleteLogsByCopilotId( + payload.id, + EntityType.INVOICE, + now, + ) + if (syncedInvoice) { + await this.updateQBInvoice( + { status: InvoiceStatus.DELETED }, + eq(QBInvoiceSync.id, syncedInvoice.id), + ['id'], ) - if (syncedInvoice) { - await this.updateQBInvoice( - { status: InvoiceStatus.DELETED }, - eq(QBInvoiceSync.id, syncedInvoice.id), - ['id'], - ) - } - await this.syncLogService.updateOrCreateQBSyncLog({ - portalId: this.user.workspaceId, - entityType: EntityType.INVOICE, - eventType: EventType.DELETED, - status: LogStatus.SUCCESS, - copilotId: payload.id, - invoiceNumber: payload.number, - amount: payload.total ? payload.total.toFixed(2) : undefined, - syncAt: now, - deletedAt: now, - }) + } + await this.syncLogService.updateOrCreateQBSyncLog({ + portalId: this.user.workspaceId, + entityType: EntityType.INVOICE, + eventType: EventType.DELETED, + status: LogStatus.SUCCESS, + copilotId: payload.id, + invoiceNumber: payload.number, + amount: payload.total ? payload.total.toFixed(2) : undefined, + syncAt: now, + deletedAt: now, }) - } finally { - this.unsetTransaction() - this.syncLogService.unsetTransaction() - } + }, [this.syncLogService]) return } diff --git a/src/app/api/quickbooks/product/product.service.ts b/src/app/api/quickbooks/product/product.service.ts index cf191e9b..ac65fbf7 100644 --- a/src/app/api/quickbooks/product/product.service.ts +++ b/src/app/api/quickbooks/product/product.service.ts @@ -215,53 +215,47 @@ export class ProductService extends BaseService { 'initialProductSettingMap', ]) - return await this.db.transaction(async (tx) => { - this.setTransaction(tx) - try { - if (!setting?.initialProductSettingMap) { - const formattedPayload = mappingItems.map((item) => { - return { - ...item, - portalId: this.user.workspaceId, - } + return await this.withTransaction(async () => { + if (!setting?.initialProductSettingMap) { + const formattedPayload = mappingItems.map((item) => { + return { + ...item, + portalId: this.user.workspaceId, + } + }) + // Skip products already saved so a repeated save doesn't error. + await this.db + .insert(QBProductSync) + .values(formattedPayload) + .onConflictDoNothing({ + target: [QBProductSync.portalId, QBProductSync.productId], + where: isNull(QBProductSync.deletedAt), }) - // Skip products already saved so a repeated save doesn't error. - await this.db - .insert(QBProductSync) - .values(formattedPayload) - .onConflictDoNothing({ - target: [QBProductSync.portalId, QBProductSync.productId], - where: isNull(QBProductSync.deletedAt), - }) - return await this.getAll() - } + return await this.getAll() + } - if (changedItemReference.length > 0) { - await Promise.all( - changedItemReference?.map(async (item) => { - const payload = { - portalId: this.user.workspaceId, - productId: item.id, - name: item.isExcluded ? null : item.qbItem?.name, - description: item.isExcluded ? null : item.description, - qbItemId: item.isExcluded ? null : item.qbItem?.id, - qbSyncToken: item.isExcluded ? null : item.qbItem?.syncToken, - copilotName: item.name, - isExcluded: item.isExcluded, - } - const conditions = and( - eq(QBProductSync.portalId, this.user.workspaceId), - eq(QBProductSync.productId, item.id), - ) as WhereClause - await this.updateOrCreateQBProduct(payload, conditions) - }), - ) + if (changedItemReference.length > 0) { + // Sequential: these writes share one tx connection (no concurrent queries). + for (const item of changedItemReference) { + const payload = { + portalId: this.user.workspaceId, + productId: item.id, + name: item.isExcluded ? null : item.qbItem?.name, + description: item.isExcluded ? null : item.description, + qbItemId: item.isExcluded ? null : item.qbItem?.id, + qbSyncToken: item.isExcluded ? null : item.qbItem?.syncToken, + copilotName: item.name, + isExcluded: item.isExcluded, + } + const conditions = and( + eq(QBProductSync.portalId, this.user.workspaceId), + eq(QBProductSync.productId, item.id), + ) as WhereClause + await this.updateOrCreateQBProduct(payload, conditions) } - - return await this.getAll() - } finally { - this.unsetTransaction() } + + return await this.getAll() }) } @@ -488,77 +482,71 @@ export class ProductService extends BaseService { }) const intuitApi = new IntuitAPI(qbTokenInfo) - await this.db.transaction(async (tx) => { - this.setTransaction(tx) - try { - const mappedProduct = await this.getOne( - // 01. if this product is already mapped to a QB item, do nothing. - and( - eq(QBProductSync.portalId, this.user.workspaceId), - eq(QBProductSync.productId, productResource.id), - ) as WhereClause, - ['id'], - ) + await this.withTransaction(async () => { + const mappedProduct = await this.getOne( + // 01. if this product is already mapped to a QB item, do nothing. + and( + eq(QBProductSync.portalId, this.user.workspaceId), + eq(QBProductSync.productId, productResource.id), + ) as WhereClause, + ['id'], + ) - addSyncBreadcrumb('Product mapping check', { - alreadyMapped: !!mappedProduct, - }) - if (mappedProduct) { - console.info('Product already mapped to a QB item; skipping') - return - } + addSyncBreadcrumb('Product mapping check', { + alreadyMapped: !!mappedProduct, + }) + if (mappedProduct) { + console.info('Product already mapped to a QB item; skipping') + return + } - const qbItemName = truncateForQB( - replaceSpecialCharsForQB(productResource.name), - ) - const productDescription = convert(productResource.description) - - // check if item with name exists in QBO - let qbItem = await intuitApi.getAnItem(qbItemName, undefined, true) - - if (!qbItem) { - const tokenService = new TokenService(this.user) - const incomeAccountRef = - await tokenService.checkAndUpdateAccountStatus( - AccountTypeObj.Income, - qbTokenInfo.intuitRealmId, - intuitApi, - qbTokenInfo.incomeAccountRef, - ) - // create item in QB. No price at product.created time — invoice lines - // carry their own UnitPrice. - qbItem = await this.createItemInQB( - { - productName: z.string().parse(qbItemName), - incomeAccRefVal: z.string().parse(incomeAccountRef), - productDescription, - }, - intuitApi, - ) - } + const qbItemName = truncateForQB( + replaceSpecialCharsForQB(productResource.name), + ) + const productDescription = convert(productResource.description) - // map product to the QB item - await this.createQBProduct({ - portalId: this.user.workspaceId, - productId: productResource.id, - qbItemId: qbItem.Id, - qbSyncToken: qbItem.SyncToken, - name: qbItemName, - copilotName: productResource.name, - description: productDescription, - }) + // check if item with name exists in QBO + let qbItem = await intuitApi.getAnItem(qbItemName, undefined, true) - console.info( - 'WebhookService#webhookProductCreated | Product created in QB', + if (!qbItem) { + const tokenService = new TokenService(this.user) + const incomeAccountRef = await tokenService.checkAndUpdateAccountStatus( + AccountTypeObj.Income, + qbTokenInfo.intuitRealmId, + intuitApi, + qbTokenInfo.incomeAccountRef, + ) + // create item in QB. No price at product.created time — invoice lines + // carry their own UnitPrice. + qbItem = await this.createItemInQB( + { + productName: z.string().parse(qbItemName), + incomeAccRefVal: z.string().parse(incomeAccountRef), + productDescription, + }, + intuitApi, ) - await this.logSync(productResource.id, qbItem.Id, EventType.CREATED, { - productName: productResource.name, - qbItemName: qbItem.Name, - }) - } finally { - this.unsetTransaction() } - }) + + // map product to the QB item + await this.createQBProduct({ + portalId: this.user.workspaceId, + productId: productResource.id, + qbItemId: qbItem.Id, + qbSyncToken: qbItem.SyncToken, + name: qbItemName, + copilotName: productResource.name, + description: productDescription, + }) + + console.info( + 'WebhookService#webhookProductCreated | Product created in QB', + ) + await this.logSync(productResource.id, qbItem.Id, EventType.CREATED, { + productName: productResource.name, + qbItemName: qbItem.Name, + }) + }, [this.syncLogService]) } async queryItemsFromQB(qbTokenInfo: IntuitAPITokensType, limit: number) { From 046bc74e682934094b80d65aac8bb51f50158748 Mon Sep 17 00:00:00 2001 From: SandipBajracharya Date: Thu, 20 Aug 2026 16:38:32 +0545 Subject: [PATCH 3/4] fix(OUT-4081): make checkAndSuspendAccount writes actually transactional tokenService and syncLogService were never bound to the tx, so the suspend-account and delete-failed-logs writes ran on the pool outside it (not atomic), and unsetTransaction was skipped on throw. Bind both services via withTransaction and run the writes sequentially. Caller is currently dormant, so no live prod exposure. Co-Authored-By: Claude Opus 4.8 --- src/app/api/quickbooks/sync/sync.service.ts | 23 +++++++-------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/src/app/api/quickbooks/sync/sync.service.ts b/src/app/api/quickbooks/sync/sync.service.ts index 9e6129c9..e6a64418 100644 --- a/src/app/api/quickbooks/sync/sync.service.ts +++ b/src/app/api/quickbooks/sync/sync.service.ts @@ -577,29 +577,22 @@ export class SyncService extends BaseService { // TODO: notify IU about the account suspension - await this.db.transaction(async (tx) => { - this.setTransaction(tx) - const tokenService = new TokenService(this.user) - const suspendAccount = tokenService.updateQBPortalConnection( - { - isSuspended: true, - }, + const tokenService = new TokenService(this.user) + // Sequential: both writes share one tx connection (no concurrent queries). + await this.withTransaction(async () => { + await tokenService.updateQBPortalConnection( + { isSuspended: true }, eq(QBPortalConnection.portalId, this.user.workspaceId), ['id'], ) - - const deleteLogs = this.syncLogService.updateQBSyncLog( - { - deletedAt: new Date(), - }, + await this.syncLogService.updateQBSyncLog( + { deletedAt: new Date() }, and( eq(QBSyncLog.portalId, this.user.workspaceId), eq(QBSyncLog.status, LogStatus.FAILED), ) as WhereClause, ) - await Promise.all([suspendAccount, deleteLogs]) - this.unsetTransaction() - }) + }, [tokenService, this.syncLogService]) CustomLogger.info({ message: `SyncService#checkAndSuspendAccount | Suspended the account. Portal Id: ${this.user.workspaceId}`, From 514f5406472719001e5fd48d07d84ab6e380a0f1 Mon Sep 17 00:00:00 2001 From: SandipBajracharya Date: Thu, 20 Aug 2026 18:10:07 +0545 Subject: [PATCH 4/4] docs(OUT-4081): document BaseService transaction pattern; publish docs/ and fix links Adds docs/baseservice-transactions.md (why this.db is mutable, withTransaction, the nested-service footgun TS can't catch). Un-gitignores docs/ and publishes why-test-helpers-use-the-app-db-singleton.md. CLAUDE.md: summarize the tx pattern + footgun inline, correct the stale 'docs/ is gitignored' note, and drop dangling docs/*.md links. Co-Authored-By: Claude Opus 4.8 --- .gitignore | 1 - CLAUDE.md | 19 +++--- docs/baseservice-transactions.md | 54 ++++++++++++++++ ...y-test-helpers-use-the-app-db-singleton.md | 61 +++++++++++++++++++ 4 files changed, 123 insertions(+), 12 deletions(-) create mode 100644 docs/baseservice-transactions.md create mode 100644 docs/why-test-helpers-use-the-app-db-singleton.md diff --git a/.gitignore b/.gitignore index 871187c7..3275a7ff 100644 --- a/.gitignore +++ b/.gitignore @@ -44,5 +44,4 @@ next-env.d.ts .trigger # local decision notes (not published) -/docs /supabase/snippets \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md index c7dee4e4..1c268965 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -70,9 +70,9 @@ Services extend `BaseService` (`src/app/api/core/services/base.service.ts`), whi - `this.db` — the **module-level Drizzle singleton** from `src/db/index.ts` (`DBClient.getInstance()`); `casing: 'snake_case'`. - `this.user` — the authenticated `User` for the request. -- `setTransaction(tx)` / `unsetTransaction()` — swap `this.db` for a transaction handle inside a `db.transaction(...)` callback, then restore. +- `setTransaction(tx)` / `unsetTransaction()` — swap `this.db` between the pool and a transaction handle. **Use `withTransaction(fn, services?)` rather than pairing these by hand.** -**Pitfall (known, see `memory/project_unsetTransaction_bug.md`):** `unsetTransaction()` is sometimes called inside the transaction callback or skipped on error paths — across `BaseService` subclasses this leaves the singleton pointed at a closed tx. When introducing or modifying transactional code, audit that `setTransaction` / `unsetTransaction` are paired in `try/finally` and that nested service calls share the tx handle. +**Transactions (full reasoning in `docs/baseservice-transactions.md`).** `this.db` is a mutable field so a service's whole method flow can join a tx without threading `tx` through every signature. `withTransaction(fn, services?)` runs `fn` in a transaction and restores `this` plus any passed `services` in a `finally` that _wraps_ it, so a throw or failed commit can't leave a service on a closed handle. **Footgun:** each `BaseService` has its own `this.db`, so a nested service (`this.syncLogService`, a `new TokenService`, …) that writes inside `fn` MUST be in the `services` array — TypeScript does **not** catch an omission, and an unbound service's writes silently run outside the tx (this was the pre-OUT-4081 `checkAndSuspendAccount` bug). The DB singleton is also why test helpers (`test/helpers/seed.ts`, `test/helpers/testDb.ts`) import `@/db` directly — see `docs/why-test-helpers-use-the-app-db-singleton.md`. Don't introduce a separate test-only Drizzle client; tests must read what the app writes. @@ -88,7 +88,7 @@ The DB singleton is also why test helpers (`test/helpers/seed.ts`, `test/helpers ### Token refresh -QBO access tokens expire in ~1h, refresh tokens in ~100 days. `src/utils/intuitAPI.ts` sends authenticated requests; `src/utils/tokenRefresh.ts` (`getValidQbTokens`) refreshes when stale. The `vercel.json` cron `/api/quickbooks/refresh-tokens` runs daily at 06:00 UTC to keep refresh tokens warm. There's a known silent-401 bug — expired tokens cause `null` returns from `getFetchWithHeader/postFetchWithHeaders`; the planned fix is auto-refresh inside those helpers (design at `docs/intuit-api-token-refresh.md`, summary in `memory/project_intuit_api_token_refresh.md`). +QBO access tokens expire in ~1h, refresh tokens in ~100 days. `src/utils/intuitAPI.ts` sends authenticated requests; `src/utils/tokenRefresh.ts` (`getValidQbTokens`) refreshes when stale. The `vercel.json` cron `/api/quickbooks/refresh-tokens` runs daily at 06:00 UTC to keep refresh tokens warm. There's a known silent-401 bug — expired tokens cause `null` returns from `getFetchWithHeader/postFetchWithHeaders`; the planned fix is auto-refresh inside those helpers (summary in `memory/project_intuit_api_token_refresh.md`). ### Background work @@ -112,11 +112,11 @@ Every `WHERE` clause that touches a portal-scoped table needs `portalId = this.u ## Testing - Two Vitest **projects** in `vitest.config.ts` — `unit` (mock-heavy, isolated) and `integration` (real Postgres via testcontainers). Run order is enforced via `sequence.groupOrder` (unit=0, integration=1). -- Integration project is configured **`pool: 'forks'` + `fileParallelism: false` + `isolate: false`** so all integration tests share one Postgres container _and_ one app DB connection. Don't change these without reading `docs/vitest-gotchas.md` and `docs/why-test-helpers-use-the-app-db-singleton.md`. +- Integration project is configured **`pool: 'forks'` + `fileParallelism: false` + `isolate: false`** so all integration tests share one Postgres container _and_ one app DB connection. Don't change these without reading `docs/why-test-helpers-use-the-app-db-singleton.md`. - `.env.test` is loaded by `test/integration/globalSetup.ts` with `override: true` so a developer's local `.env` can't leak into tests. `DATABASE_URL` is intentionally **not** in `.env.test` — globalSetup sets it from the container's URI before any worker imports `src/config`. -- Module mocks for integration are in `test/integration/setup.ts` — `@/utils/copilotAPI`, `@/utils/intuitAPI`, and `@sentry/nextjs` must be mocked with **explicit factories** (and Intuit/Copilot mock implementations must use `function`, not `=>`, because the code does `new IntuitAPI(...)`). See `docs/vitest-gotchas.md` items 1–3. +- Module mocks for integration are in `test/integration/setup.ts` — `@/utils/copilotAPI`, `@/utils/intuitAPI`, and `@sentry/nextjs` must be mocked with **explicit factories** (and Intuit/Copilot mock implementations must use `function`, not `=>`, because the code does `new IntuitAPI(...)`). - Test helpers in `test/helpers/`: `seed.ts` (`seedHealthyPortal`, `TEST_PORTAL_ID`, etc.), `webhook.ts` (`postWebhook` via `next-test-api-route-handler`), `testDb.ts` (`truncateAllTestTables`). -- Test-data philosophy in `docs/test-data-dos-and-donts.md`: static fixtures for the thing under test, factories with explicit overrides for single-dimension variants, **no faker** in fixtures or assertions. +- Test-data philosophy: static fixtures for the thing under test, factories with explicit overrides for single-dimension variants, **no faker** in fixtures or assertions. ## Path aliases @@ -132,15 +132,12 @@ Configured in `tsconfig.json` and propagated to Vitest via `vite-tsconfig-paths` - Prettier: single quotes, no semis, trailing comma all (`.prettierrc`). - ESLint: `next/core-web-vitals` + TypeScript; `prefer-const` and `no-var` are errors; unused-var underscore prefix is exempt; `@typescript-eslint/no-explicit-any` is disabled (the codebase uses `any` deliberately at framework boundaries). - Tailwind v4 + `copilot-design-system`. UI surface is small (settings dashboard + OAuth callback) — most work happens in the API/service layer. -- The `docs/` folder is **gitignored** (per `.gitignore`) and used for local decision notes — design docs, post-mortems, comparison tables. Save non-trivial tradeoff discussions there rather than in code comments or commit messages. +- `docs/` holds **committed** decision docs (design notes, post-mortems, comparison tables); `private-local-docs/` is gitignored for local-only notes. Save non-trivial tradeoff discussions in `docs/` rather than in code comments or commit messages. ## Things to read before non-trivial changes -- `docs/testcontainers-vs-local-supabase.md` — why integration tests use testcontainers, not the local Supabase stack. - `docs/why-test-helpers-use-the-app-db-singleton.md` — why test helpers import `@/db` and what would break if you opened a separate client. -- `docs/vitest-gotchas.md` — the five real traps already hit in this project. -- `docs/test-data-dos-and-donts.md` — the test-data rules. -- `docs/intuit-api-token-refresh.md` — design for the silent-401 fix. +- `docs/baseservice-transactions.md` — why `this.db` is mutable, how to use `withTransaction`, and the nested-service footgun. ## What this repo doesn't have diff --git a/docs/baseservice-transactions.md b/docs/baseservice-transactions.md new file mode 100644 index 00000000..3c9bc07b --- /dev/null +++ b/docs/baseservice-transactions.md @@ -0,0 +1,54 @@ +# BaseService transactions: why `this.db` is mutable, and how to use `withTransaction` + +## The pattern + +`BaseService` exposes `this.db` (the Drizzle client) and two methods that swap it: + +```ts +setTransaction(tx) // this.db = tx — point this service at a transaction +unsetTransaction() // this.db = pool — restore the shared pool +``` + +Every service method reads `this.db`, so swapping it changes which connection all of that service's queries run on. + +## Why it's built this way + +Two constraints force it: + +1. **Drizzle only hands you the tx handle inside the callback.** `db.transaction(async (tx) => …)` gives you `tx` only within that closure. But a service operation is usually a _flow_ of methods (`getOne`, `createQBProduct`, `logSync`, …), each of which reads `this.db`. Threading `tx` through every method signature — and through every nested service they call — would be invasive. +2. **Services are constructed synchronously.** `new InvoiceService(user)` runs in a normal constructor, and the SDK/DB handles are already in place. There's no async-factory seam to inject a tx at construction time. + +So instead of passing `tx` everywhere, `setTransaction(tx)` temporarily repoints `this.db` at the tx; every method the service calls transparently joins the transaction; `unsetTransaction()` restores the pool afterward. + +## Use `withTransaction`, not the raw pair + +Pairing `setTransaction`/`unsetTransaction` by hand is error-prone (the unset can be skipped on a throw, or placed inside the callback so it runs before the commit). Use the helper on `BaseService`: + +```ts +await this.withTransaction(async () => { + await this.updateQBInvoice(...) + await this.syncLogService.updateOrCreateQBSyncLog(...) +}, [this.syncLogService]) +``` + +It runs `fn` inside `db.transaction(...)`, binds `this` **and every service in the array** to the tx, and restores all of them in a `finally` that **wraps** the transaction — so a throw or a failed commit can never leave a service pointing at a closed handle. (Introduced in OUT-4081.) + +## The sharp edge — the part that bites + +**Each `BaseService` instance has its own independent `this.db`.** A nested service is _not_ in the transaction just because the outer service started one. If a `fn` writes through `this.syncLogService` or a freshly-`new`'d `TokenService`, that instance must be in the `services` array — otherwise its writes run on the **pool, outside the transaction**, silently. + +**TypeScript does not catch this.** The `services` array is completely decoupled from what `fn` actually calls, and a bound vs. unbound `this.db` have the identical type. So this compiles clean and is wrong at runtime: + +```ts +await this.withTransaction(async () => { + await this.syncLogService.updateQBSyncLog(...) // compiles fine… +}, []) // …but syncLogService was never bound → write escapes the tx +``` + +This is not hypothetical: `sync.service#checkAndSuspendAccount` did exactly this before OUT-4081 — the suspend-account and delete-logs writes lived inside `db.transaction(...)` but their services were never bound, so the two writes weren't atomic despite looking like they were. + +**Rule when writing transactional code:** trace every DB call inside `fn` to the service instance it runs on, and confirm that instance is `this` or is in the `services` array. + +## Why not make it type-safe? + +The only way to get the compiler to enforce completeness is to stop mutating `this.db` and thread `tx` explicitly — e.g. `tx.insert(...)` directly, or DB methods that take a required `tx` parameter. That removes the footgun entirely (you physically cannot write without the handle in hand), but it's a large refactor touching every transactional method and the nested methods they call. It's deliberately deferred; `withTransaction` is the pragmatic middle ground that fixes the _pairing_ bug now while leaving the _completeness_ gap documented. See the OUT-4081 PR and `memory/project_unsetTransaction_bug.md`. diff --git a/docs/why-test-helpers-use-the-app-db-singleton.md b/docs/why-test-helpers-use-the-app-db-singleton.md new file mode 100644 index 00000000..182ae9c6 --- /dev/null +++ b/docs/why-test-helpers-use-the-app-db-singleton.md @@ -0,0 +1,61 @@ +# Why `truncateAllTestTables` (and other test helpers) import the real app `db` + +**Date:** 2026-04-13 +**Context:** Reviewing the integration test scaffolding for `price.created`, the question came up: why does `test/helpers/testDb.ts` import `@/db` — isn't that the production-style module-level singleton? Shouldn't tests use a dedicated test client? + +## Short answer + +An integration test's contract is: **"the app's code runs against a real Postgres, and we verify observable outcomes in that Postgres."** That requires the test and the app to talk to the _same_ database through the _same_ client. Using a separate test-only DB client would silently decouple "what the test wipes / seeds / asserts on" from "what the app writes to", which defeats the purpose. + +## Data flow on one test run + +``` +globalSetup.ts → starts Postgres container, sets process.env.DATABASE_URL + ↓ +test file imports @/db → DBClient.getInstance() constructs once, + reads DATABASE_URL = container URL, + opens a postgres-js connection pool + ↓ +beforeEach → truncateAllTestTables() uses db → wipes the container's tables + seedHealthyPortal() uses db → inserts into the container + ↓ +POST /api/quickbooks/webhook + ↓ controller → service code also imports @/db (SAME singleton) + → reads the seed, writes qb_product_sync, writes qb_sync_logs + ↓ +test assertions: db.select(...) → SAME singleton, SAME container, + reads back what the app wrote +``` + +If `truncateAllTestTables` used a _different_ Drizzle client, we'd get a subtle bug: truncate wipes DB "A", app writes to DB "B", assertions check DB "A" and see nothing. Tests pass when they shouldn't, or fail for mysterious reasons. + +## Why the singleton isn't a risk here + +Normally, a module-level singleton pointing at a DB is something to be nervous about in tests — it implies "shared state across everything, hard to override." Two things make it fine in this setup: + +1. **Only one DB is reachable from this process.** `DATABASE_URL` is set by `globalSetup.ts` to the testcontainer's URI _before_ any `src/` code is imported by the test worker. The singleton can't bind to anything else. No production DB is reachable. +2. **Tests run sequentially.** `vitest.config.ts` sets `fileParallelism: false` and `isolate: false` for the integration project, so no two tests mutate the shared DB concurrently. Truncate + seed in `beforeEach` is enough to guarantee a clean slate. + +## Alternatives we considered and rejected + +| Approach | Why not | +| -------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `truncateAllTestTables` uses its own `drizzle(postgres(url))` client | Two connection pools to the same DB — works, but pointless; also re-raises the "was URL set in time?" ordering question | +| Truncate via raw `psql` in a shell script before Vitest starts | Runs once, not per-test; doesn't reset between tests | +| `BEGIN`/`ROLLBACK` wrapping each test instead of truncate | Cleaner in theory, but **doesn't work for this codebase** — `BaseService.setTransaction()` opens its own transaction, and Postgres doesn't allow nested real transactions (only savepoints). Rolling back at the test level would abort the app's transaction mid-flight and mask real bugs. | +| A separate test-only Drizzle client with its own schema imports | Adds a second pool for no benefit; more chances for the two to drift (casing, schema refs, pooling behavior) | + +The simplest correct answer is: **use the same `db` the app uses**. That's what integration testing means. + +## Future consideration: DI refactor + +This pattern works _because_ `@/db` is a singleton. If the codebase ever moves to dependency injection (the 3–5 week refactor noted in the testing-strategy memory), these helpers would change shape — they'd accept a `db` argument rather than importing it. At that point `truncateAllTestTables(db)` becomes natural and the "why is it importing app code" question goes away. + +Until then, the singleton is a constraint we work _with_, not against. + +## TL;DR for future-me + +- Tests and app must share the DB client. Do **not** open a second Drizzle/postgres-js client just for the test. +- The singleton is safe because `DATABASE_URL` is fixed to the container before any `src/` import loads. +- Keep integration tests sequential (`fileParallelism: false`, `isolate: false`) as long as they share a container. +- If you're tempted to switch to `BEGIN/ROLLBACK` for isolation: check that no code under test opens its own transaction first. In this codebase, `BaseService.setTransaction()` does — so truncate is the right tool.