Skip to content

[13.x] Execute rollback callbacks for committed savepoints (follow-up to #55420)#60482

Open
sulimanbenhalim wants to merge 1 commit into
laravel:13.xfrom
sulimanbenhalim:fix/savepoint-rollback-callbacks-discarded
Open

[13.x] Execute rollback callbacks for committed savepoints (follow-up to #55420)#60482
sulimanbenhalim wants to merge 1 commit into
laravel:13.xfrom
sulimanbenhalim:fix/savepoint-rollback-callbacks-discarded

Conversation

@sulimanbenhalim

Copy link
Copy Markdown

Hi team

Hit this one in production today. Took me hours to figure out.

Setup: controller wraps its work in DB::transaction. Inside it calls a service that does its own DB::transaction (so a savepoint). Service dispatches a ShouldBeUnique job with after_commit => true. After all the work the controller calls the payment gateway. Gateway throws. Outer rolls back.

Expected: job not queued. Unique lock released. Move on.

What actually happens: job not queued (correct) but the unique lock is NEVER released. Every future dispatch of that job silently does nothing forever.

Took me a while to find why. PR #55420 added the rollback callback mechanism a few months ago which is exactly the right idea. It fires executeCallbacksForRollback() for the OPEN transaction chain. But when an inner savepoint commits its record moves from pendingTransactions to committedTransactions. Then when the outer rolls back removeAllTransactionsForConnection just ->reject()s those committed records without firing their callbacks.

Same gap in removeCommittedTransactionsThatAreChildrenOf (the partial rollback path).

Fix

Fire the callbacks before dropping the committed records. The callbacks already exist on the record. Nobody just calls them in this path.

3 lines in removeAllTransactionsForConnection. 1 line in removeCommittedTransactionsThatAreChildrenOf.

Tests

Added 2 tests next to testRollbackTransactionsExecutesCallbacks:

  • inner savepoint commits then outer rolls back
  • partial rollback with a committed grandchild

All 17 DatabaseTransactionsManagerTest pass. All 52 EloquentTransactionWithAfterCommit* integration tests across the 5 variants pass.

Backward compat

Zero behavior change for code that does not register rollback callbacks on inner savepoints. Code that does (i.e. ShouldBeUnique with after_commit inside nested transactions) finally sees the rollback callbacks it registered actually run.

Same incomplete-fix-completion shape as the recent #60149 cleanup if that helps frame review.

Thanks

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