OUT-4081: harden BaseService transaction handling

.gitignore

@@ -44,5 +44,4 @@ next-env.d.ts
 .trigger
 
 # local decision notes (not published)
-/docs
 /supabase/snippets

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
 

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

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.

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<T>(
+    fn: (tx: PostgresJsDatabase<typeof schema & typeof relation>) => Promise<T>,
+    services: BaseService[] = [],
+  ): Promise<T> {
+    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())
+    }
+  }
 }