fix: roll back bulk delete/update transactions on partial failure instead of committing, fix bulk operation errors not being logged#15517
Open
fix: roll back bulk delete/update transactions on partial failure instead of committing, fix bulk operation errors not being logged#15517
Conversation
Contributor
📦 esbuild Bundle Analysis for payloadThis analysis was generated by esbuild-bundle-analyzer. 🤖
Largest pathsThese visualization shows top 20 largest paths in the bundle.Meta file: packages/next/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_shared.json, Out file: esbuild/exports/shared.js
Meta file: packages/richtext-lexical/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_shared.json, Out file: esbuild/exports/shared_optimized/index.js
DetailsNext to the size is how much the size has increased or decreased compared with the base branch of this PR.
|
AlessioGr
commented
Feb 5, 2026
| let docReq = req | ||
|
|
||
| if (useSeparateTransactions) { | ||
| docReq = isolateObjectProperty(req, 'transactionID') |
Member
Author
There was a problem hiding this comment.
Previously this
- mutated the original req => the new transactionID leaked into consecutive doc update calls
- did not delete the initial req.transactionID, meaning essentially this did not do anything unless the operation was called with transactions disabled
- did not respect
args.disableTransaction
AlessioGr
commented
Feb 5, 2026
| // 8. Return results | ||
| // ///////////////////////////////////// | ||
| if (docShouldCommit) { | ||
| await commitTransaction(req) |
Member
Author
There was a problem hiding this comment.
(This code only runs when bulkOperationsSingleTransaction is true)
We need to commit outside the loop.
Why?
If any other doc has an error and is rolled back, we do not want to commit the successful docs - instead, we want to roll them back too. This is the way this function works without bulkOperationsSingleTransaction set.
That way, we can minimize the behaviors changed by bulkOperationsSingleTransaction.
AlessioGr
commented
Feb 5, 2026
| awaitedDocs = [] | ||
| for (const promise of promises) { | ||
| awaitedDocs.push(await promise) | ||
| const awaitedDocs = await Promise.all(promises) |
Member
Author
There was a problem hiding this comment.
We were always running this in parallel by default. If we need a mechanism to make this sequential, we can always add another configuration option instead of stuffing side-effects like that into bulkOperationsSingleTransaction
AlessioGr
commented
Feb 5, 2026
| if (existingError) { | ||
| return existingError | ||
| } | ||
| return { |
Member
Author
There was a problem hiding this comment.
Return early if there is any error. We do not want deleteUserPreferences to run, because
- no document was actually deleted
- the transaction was killed =>
deleteUserPreferenceswould fail if it attempts to use the existingreq.transactionID
AlessioGr
commented
Feb 5, 2026
|
|
||
| if (anyTransactionRolledBack) { | ||
| // All-or-nothing: everything was rolled back, report all as failed | ||
| const allErrors: BulkOperationResult<TSlug, TSelect>['errors'] = docs.map((doc) => { |
Member
Author
There was a problem hiding this comment.
This is needed for correct error reporting (was not correct previously).
If any document was rolled back due to an error, all documents will be rolled back. Thus, successful document deletions were not actually successful since they will be rolled back despite no error. => Add explicit 'Transaction rolled back due to error in another document' errors here.
Previously, those documents were incorrectly returned as successful deletions/updates even though everything was rolled back!
AlessioGr
commented
Feb 5, 2026
| payload, | ||
| req, | ||
| }) | ||
| if (docs.length > 0) { |
AlessioGr
commented
Feb 5, 2026
| throw new APIError(`Collection ${args.collection.config.slug} has disabled bulk edit`, 403) | ||
| } | ||
|
|
||
| const useSeparateTransactions = |
Member
Author
There was a problem hiding this comment.
bulkOperationsSingleTransaction is a misleading property name. We cannot rename it without a breaking change, thus this local variable with a different name, to not confuse people reading this function
AlessioGr
commented
Feb 5, 2026
| const errors: BulkOperationResult<TSlug, TSelect>['errors'] = [] | ||
|
|
||
| // Track all doc requests that have open transactions (for separate transaction mode) | ||
| const docReqsWithTransactions: PayloadRequest[] = [] |
Member
Author
There was a problem hiding this comment.
For the remainder of this file, see my comments in the delete.ts. Changes are pretty much identical
AlessioGr
commented
Feb 5, 2026
| // Don't commit here - wait until all docs complete successfully | ||
| return result | ||
| } catch (error) { | ||
| docReq.payload.logger.error({ err: error, msg: `Error deleting document ${id}` }) |
Member
Author
There was a problem hiding this comment.
Without this, no error will be logged at all, which makes debugging difficult
AlessioGr
commented
Feb 5, 2026
| * @returns true if a transaction was rolled back, false if no transaction existed | ||
| */ | ||
| export async function killTransaction( | ||
| req: MarkRequired<Partial<PayloadRequest>, 'payload'>, |
Member
Author
There was a problem hiding this comment.
This API mimics what we have for initTransaction - we can check the return type to determine if anything was rolled back or not.
This is necessary for accurate error reporting in delete/update.ts. If one document failed, but transactions are disabled, we do want remaining successful document updates/deletions to show as success rather than errors, as they cannot be rolled back!
AlessioGr
commented
Feb 5, 2026
| }) | ||
|
|
||
| describe('transactions', () => { | ||
| describe('transactions', { db: 'transactionsEnabled' }, () => { |
Member
Author
There was a problem hiding this comment.
So much simpler and easier to read.
No tests were edited here, just unwrapped everything out of .includes(process.env.PAYLOAD_DATABASE) - github doesn't diff it nicely
AlessioGr
commented
Feb 5, 2026
| where: { id: { in: docs.map((d) => d.id) } }, | ||
| }) | ||
|
|
||
| if (hasTransactions) { |
Member
Author
There was a problem hiding this comment.
This is so that we can correctly test it on sqlite (transactions disable) and postgres/mongo (transactions enabled)
AlessioGr
commented
Feb 5, 2026
| }) | ||
| const result = await response.json() | ||
|
|
||
| // With transactions enabled, when one doc fails the entire transaction is rolled back |
Member
Author
There was a problem hiding this comment.
These tests were failing, because they were asserting for previous behavior: partial updates in bulk update/delete were possible.
After this PR, with transactions enabled, partial updates are no longer possible, thus we need to update these tests. Reasoning for changing this behavior is in PR description
AlessioGr
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes critical issues in bulk delete and update operations by implementing consistent error handling and transaction rollback behavior, and adds proper error logging that was previously missing.
Changes:
- Bulk operations now roll back transactions and report all documents as failed when any error occurs (all-or-nothing semantics)
- Errors during bulk operations are now properly logged instead of silently swallowed
- Fixed
bulkOperationsSingleTransactionto actually use separate transactions as intended
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/database/payload-types.ts | Added new bulk-error-test collection type and changed ID types from string to number |
| test/database/int.spec.ts | Added comprehensive tests for bulk operation error handling and transaction behavior |
| test/database/getConfig.ts | Added test collection with hooks that throw errors for testing bulk operation failures |
| test/collections-rest/int.spec.ts | Updated assertions to reflect new all-or-nothing transaction behavior |
| test/_community/payload-types.ts | Changed ID types from string to number |
| test/__helpers/int/vitest.ts | Added support for transaction-specific test filtering |
| packages/payload/src/utilities/killTransaction.ts | Modified to return boolean indicating if transaction was rolled back |
| packages/payload/src/index.ts | Added JSDoc documentation for bulk operations explaining all-or-nothing semantics |
| packages/payload/src/database/types.ts | Enhanced documentation for bulkOperationsSingleTransaction option |
| packages/payload/src/collections/operations/update.ts | Implemented error logging and all-or-nothing transaction rollback behavior |
| packages/payload/src/collections/operations/delete.ts | Implemented error logging and all-or-nothing transaction rollback behavior |
| packages/db-mongodb/src/index.ts | Enhanced documentation for bulkOperationsSingleTransaction option |
| docs/database/transactions.mdx | Added documentation explaining bulk operation transaction behavior |
| docs/database/mongodb.mdx | Updated description of bulkOperationsSingleTransaction option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deepshekhardas
pushed a commit
to deepshekhardas/payload
that referenced
this pull request
Apr 3, 2026
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.
Bulk delete and update operations had inconsistent behavior when documents failed during processing, depending on where the error originated.
The problem: inconsistent partial success
When a document failed due to a hook error (e.g.,
beforeChangethrowing), the error was caught and the transaction was still committed. Other documents in the batch were persisted, and the operation returned partial success (docscontaining successful ones,errorscontaining failed ones). This behavior could be desirable in some cases.However, when a document failed due to a database constraint (e.g., FK violation), PostgreSQL internally aborts the entire transaction. The error was caught but never logged, and the code continued trying to process remaining documents on the now-dead transaction. Subsequent operations would fail with:
An example of that can be seen here: #14576
This made partial success unreliable - it only worked for hook errors, not database errors. You couldn't depend on it because whether it worked depended on the error type, and when it didn't work, you'd get confusing errors pointing to unrelated operations instead of the root cause.
The fix
Errors are now logged immediately when caught. If any document fails for any reason, the transaction is explicitly rolled back and all documents are reported as failed. This provides consistent, predictable behavior regardless of error type.
Behavioral change
Previously, bulk operations could return partial success when errors occurred in hooks. Now, when transactions are enabled: if any document fails, the entire transaction is rolled back and all documents are reported in the
errorsarray. Thedocsarray will be empty.This is the only way for bulk operations to work reliably with transactions. When multiple documents share a transaction, any database-level error (constraint violation, deadlock, etc.) kills the entire transaction - there's no way to "recover" and continue processing other documents on that transaction. By adopting all-or-nothing semantics for all error types, the behavior is now consistent and predictable regardless of whether the error originated in a hook or in the database.
Exception: Locked document errors
Lockederrors are an exception to the all-or-nothing rule. These errors occur before any database changes happen (they're a pre-flight check whenoverrideLock: false), so they don't affect the transaction. Bulk operations on a mix of locked and unlocked documents will return partial success: unlocked documents are processed successfully, and only the locked ones appear inerrors.When transactions are disabled (e.g., MongoDB without replica set, or
disableTransaction: true): partial success is still possible since each document operates independently and there is no way to roll back operations that already happened.When transactions are disabled (e.g., MongoDB without replica set): partial success is still possible since each document operates independently, and there is no way to roll back operations that already happened.
Behavior change for
bulkOperationsSingleTransactionThis option was broken since introduction. Despite the name suggesting "single transaction", its intended purpose was to run each document in a separate transaction for DocumentDB/Cosmos DB compatibility. In practice, it only made operations run sequentially instead of in parallel while still using a shared transaction.
This fix makes the option actually work as intended: each document now gets its own isolated transaction, and operations run in parallel. All-or-nothing semantics are preserved to minimize behavioral changes if this is set (if any document fails, all transactions are rolled back). The option will be renamed to
bulkOperationsSeparateTransactionsin 4.0 to reflect what it actually does.