When a helper wraps work in a *sql.Tx, how do you avoid losing the Commit or Rollback error?

## The postmortem this pattern comes from A ledger service posts a debit and a credit inside one transaction, amounts in minor units. For weeks it reports success on every request. Then reconciliation finds a day where a handful of transfers were acknowledged to the caller and no rows exist. The code: ```go defer func() { _ = tx.Rollback() }() if _, err := tx.ExecContext(ctx, debitSQL, amountMinor, fromID); err != nil { return err } tx.Commit() // error discarded return nil ``` The database was rejecting some commits — a deadlock victim, a serialisation failure, a connection dropped at exactly the wrong moment. `Commit` returned an error; the code returned `nil`. The deferred `tx.Rollback()` then ran, returned `sql.ErrTxDone` because the transaction was already finished, and was thrown away. Everything looked fine from Go's side, and the only visible trace was a report that did not balance. The lesson is narrow and important: **`tx.Commit()` is the operation that can fail, and it is the one people forget to check.** Every statement before it can succeed and the transaction can still not happen. ## The wrapper ```go func withTx(ctx context.Context, db *sql.DB, fn func(*sql.Tx) error) (err error) { tx, err := db.BeginTx(ctx, nil) if err != nil { return err } defer func() { if p := recover(); p != nil { _ = tx.Rollback() panic(p) } if err != nil { if rbErr := tx.Rollback(); rbErr != nil && !errors.Is(rbErr, sql.ErrTxDone) { err = errors.Join(err, rbErr) } } }() if err = fn(tx); err != nil { return err } return tx.Commit() } ``` Four decisions are encoded here, and each is worth being able to defend. **The named result `(err error)`.** A deferred closure can read and *modify* a named result parameter, because the result is a variable that lives until the function actually returns. With an unnamed `error` result the closure has no way to see what is being returned, so it cannot decide whether to roll back and cannot attach anything. This is the single reason the signature is written that way, and it is the detail interviewers probe. **Rolling back only when `err != nil`.** With the named result available, the rollback becomes conditional and intentional rather than a blanket safety net. It reads as "the work failed, so undo it". Note that the unconditional `defer func() { _ = tx.Rollback() }()` from the simple in-line shape is not wrong — it is just silent. The conditional version is what lets you report a rollback that genuinely failed. **`errors.Join`, not replacement.** If the rollback fails, the original error is still the interesting one — the rollback failure is context. `errors.Join` (Go 1.20) produces an error wrapping both, and `errors.Is` still matches either. Returning only the rollback error would hide the cause; ignoring it entirely would hide that the database connection is in trouble. **Filtering `sql.ErrTxDone` with `errors.Is`.** That sentinel means the transaction had already ended — because the commit ran, or because the context was cancelled and `database/sql` rolled back for you. It is expected, not a defect, and joining it onto every error would bury real signal in noise. `errors.Is` rather than `==` because a driver or an intermediate layer may wrap it. **Panic: roll back and re-panic.** The wrapper's job is to release the transaction, not to decide the program's fate. Recovering and returning an error turns a bug into a plausible-looking failure and loses the stack; recovering and returning nil is worse. Roll back, then `panic(p)` so the value continues up the stack to whatever layer is designed to handle it. ## How the call site reads ```go err := withTx(ctx, db, func(tx *sql.Tx) error { if _, err := tx.ExecContext(ctx, debitSQL, amountMinor, fromID); err != nil { return err } _, err := tx.ExecContext(ctx, creditSQL, amountMinor, toID) return err }) ``` The body cannot forget to commit, cannot forget to roll back, and — because the closure only receives the `*sql.Tx` — has no easy way to reach the `*sql.DB` and accidentally run a statement outside the transaction. ## What a wrapper must not quietly do **Retries.** It is tempting to retry inside `withTx` on a serialisation or deadlock error. Be careful: the closure may have mutated in-memory state, appended to a slice, or sent on a channel, and re-running it is only safe if it is genuinely pure with respect to everything but the database. If you retry, make that a separate, explicitly named wrapper so the caller opts in. **Swallowing context errors.** A `Commit` that returns `context.DeadlineExceeded` means the transaction was abandoned. Callers often want to distinguish that from a constraint violation, so pass the error through unchanged and let them use `errors.Is`. **Logging instead of returning.** A helper that logs the commit error and returns nil recreates the original bug with a paper trail. The error belongs to the caller. ## The reviewer's checklist When you read a transaction block in a pull request, three questions catch nearly everything: is `Commit`'s error returned; is there exactly one place that ends the transaction on failure; and can any statement in the body reach a handle other than this `*sql.Tx`. If the answer to all three is good, the block is almost certainly correct.