Skip to content

fix(drizzle): surface transaction commit failures instead of resolving successfully - #17727

Open
MarianoMiguel wants to merge 1 commit into
payloadcms:mainfrom
MarianoMiguel:fix/drizzle-commit-error-propagation-main
Open

fix(drizzle): surface transaction commit failures instead of resolving successfully#17727
MarianoMiguel wants to merge 1 commit into
payloadcms:mainfrom
MarianoMiguel:fix/drizzle-commit-error-propagation-main

Conversation

@MarianoMiguel

Copy link
Copy Markdown

Mirror of #17726, which targets 3.x.

What?

When the database fails at COMMIT time, drizzle-based adapters roll the transaction back and the Local API operation still resolves successfully, returning the full document as if persisted. This affects db-postgres and db-vercel-postgres out of the box (transactions are on by default there), and AFAIK db-sqlite / db-d1-sqlite when transactions are enabled via transactionOptions.

Two layers are producing this. commitTransaction swallows any error thrown by session.resolve():

try {
  await session.resolve()
} catch (_) {
  await session.reject()
}

And since #16220, session.resolve() cannot actually reject at all: the .catch added there (to stop hangs when the connection fails before the callback runs) also absorbs the transaction promise's rejection after the transaction is ready (transactionFailed is a no-op by then) so a failed COMMIT never surfaces anywhere.

The idea behind this PR is to have resolve() return the raw transaction promise (keeping the #16220 .catch for the pre-ready connection failures it fixes, and for the rollback path), and to rethrow the commit error in commitTransaction after attempting the rollback. The rollback becomes best-effort so a dead connection cannot shadow the original commit failure.

Commit failures have never been observable in the SQL adapters (the swallow shipped with the original postgres support in 2023) while db-mongodb has always propagated them (see #5904: commit errors propagate, only endSession() is best-effort). This aligns the adapters.

Why?

Hit this in production (healthcare enrollment platform, @payloadcms/db-vercel-postgres 3.84.1). A Stripe webhook handler called payload.create to record an order after a successful payment. The Postgres connection then fails at COMMIT. payload.create resolved successfully with a complete document (id included), the webhook was acknowledged, and the row did not exist. No rejected promise, no error, nothing on the returned document to inspect. So essentially the adapter swallowed the failure, rolled back, and handed back a complete document with an id, so every layer above concluded success: the pipeline logged the order as written, the event was marked processed, the webhook was acknowledged. The system ended up in a state our domain model says cannot exist: someone who has verifiably paid, with no record of the payment. I can't possibly find a way in application code to defend against this.

How?

Two small changes in packages/drizzle (beginTransaction.ts, commitTransaction.ts) plus a regression test in test/database/int.spec.ts that makes the COMMIT itself fail: a DEFERRABLE INITIALLY DEFERRED constraint violated inside the transaction, so Postgres raises the error at COMMIT while the connection stays healthy.

Verification on test/database with PAYLOAD_DATABASE=postgres:

Variant Result
Unpatched new test fails — commitTransaction resolves despite the failed COMMIT
commitTransaction change only still fails
Both changes passes
Full suite with the fix 169 passed, 0 failed, 36 skipped (other-adapter variants)

(Verification ran on the 3.x branch; the transactions code is identical here.)

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.

1 participant