fix(drizzle): surface transaction commit failures instead of resolving successfully - #17726
Open
MarianoMiguel wants to merge 1 commit into
Open
fix(drizzle): surface transaction commit failures instead of resolving successfully#17726MarianoMiguel wants to merge 1 commit into
MarianoMiguel wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-postgresanddb-vercel-postgresout of the box (transactions are on by default there), and AFAIKdb-sqlite/db-d1-sqlitewhen transactions are enabled viatransactionOptions.Two layers are producing this.
commitTransactionswallows any error thrown bysession.resolve():And since #16220,
session.resolve()cannot actually reject at all: the.catchadded there (to stop hangs when the connection fails before the callback runs) also absorbs the transaction promise's rejection after the transaction is ready (transactionFailedis 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.catchfor the pre-ready connection failures it fixes, and for the rollback path), and to rethrow the commit error incommitTransactionafter 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-mongodbhas always propagated them (see #5904: commit errors propagate, onlyendSession()is best-effort). This aligns the adapters.Why?
Hit this in production (healthcare enrollment platform,
@payloadcms/db-vercel-postgres3.84.1). A Stripe webhook handler calledpayload.createto record an order after a successful payment. The Postgres connection then fails at COMMIT.payload.createresolved 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 intest/database/int.spec.tsthat makes the COMMIT itself fail: aDEFERRABLE INITIALLY DEFERREDconstraint violated inside the transaction, so Postgres raises the error at COMMIT while the connection stays healthy.Verification on
test/databasewithPAYLOAD_DATABASE=postgres:commitTransactionresolves despite the failed COMMITcommitTransactionchange only