Skip to content

fix: rollback transaction on panic in RunInTx#703

Open
masashi-toda-ene wants to merge 1 commit into
stephenafamo:mainfrom
masashi-toda-ene:fix/run-in-tx-panic-rollback
Open

fix: rollback transaction on panic in RunInTx#703
masashi-toda-ene wants to merge 1 commit into
stephenafamo:mainfrom
masashi-toda-ene:fix/run-in-tx-panic-rollback

Conversation

@masashi-toda-ene

Copy link
Copy Markdown

Problem

RunInTx currently does not handle panics inside the provided function.
If fn panics, the transaction is never rolled back, leaving row-level
locks held until the connection is GC'd or the server's
innodb_lock_wait_timeout fires — which can cause deadlocks under
concurrent load.

db.RunInTx(ctx, nil, func(ctx context.Context, exec bob.Executor) error {
    // INSERT acquires row locks...
    panic("something went wrong") // locks are never released
})

Fix

Add a deferred recover() that rolls back the transaction and re-panics,
matching the behaviour of other Go ORMs (GORM, go-pg, gobuffalo/pop).

  • If rollback succeeds: re-panic with the original value unchanged
  • If rollback fails and the panic value is an error: join both errors via errors.Join (consistent with the existing error-path style) and re-panic
  • If rollback fails and the panic value is not an error (e.g. a string): rollback error is intentionally discarded — changing the panic value type would break callers who recover() and type-assert the value

Tests

Three new tests added to stdlib_test.go using a minimal mock driver.Connector:

Test Verifies
TestRunInTxPanicPropagates panic re-raised after successful rollback
TestRunInTxPanicErrorJoinsRollbackErr both errors visible via errors.Is when rollback also fails
TestRunInTxPanicNonErrorPreservesValue original non-error panic value preserved

References

If the function passed to RunInTx panics, the transaction was never
rolled back, leaving row-level locks held until the connection is GC'd
or the server's lock-wait timeout fires — which can cause deadlocks
under concurrent load.

Add a deferred recover() that rolls back the transaction and re-panics.
If both the original panic value and the rollback itself are errors,
they are joined via errors.Join so the caller can inspect both.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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