Goのトランザクション実装でRollback責務が曖昧なコードをどうレビューするか

GoでDB更新処理を書くと、BeginTx して ExecContext して Commit する流れ自体はすぐ書ける。
ただし、失敗時にどこまで巻き戻せるのかが読めないトランザクション実装は、レビューでかなり強く止めるべき対象になる。

特に危ないのは次のような構造だ。

  • BeginTx はあるが defer tx.Rollback() がない
  • 途中失敗時のreturn経路が複数あり、rollback責務が散っている
  • DB更新と外部通知が同じ関数に混在している
  • Commit() だけが成功条件として強く見え、失敗時シナリオがコードから読めない

この記事では、トランザクションを使っているかではなく、
失敗時の一貫性境界が構造として説明できるかをレビュー視点で整理する。

まず止めたい実装

rollback責務が曖昧な実装 
func CompleteOrder(ctx context.Context, db *sql.DB, orderID string) error {
    tx, err := db.BeginTx(ctx, nil)
    if err != nil {
        return err
    }

@Reviewer
`BeginTx` 後に `defer tx.Rollback()` が存在せず、途中returnで接続と更新状態の扱いが不明です。失敗経路を1本ずつ追わなくても安全だと分かる構造へ整理してください。

    _, err = tx.ExecContext(
        ctx,
        "UPDATE orders SET status = ? WHERE id = ?",
        "completed",
        orderID,
    )
    if err != nil {
        return err
    }

    _, err = tx.ExecContext(
        ctx,
        "INSERT INTO order_histories(order_id, action) VALUES(?, ?)",
        orderID,
        "complete",
    )
    if err != nil {
        return err
    }

    if err := publishOrderCompleted(ctx, orderID); err != nil {

@Reviewer
DB更新と外部通知が同一トランザクション関数に混在しています。通知失敗時にDB更新だけ残るのか、全体失敗にするのか、一貫性境界がコードから読めません。
        return err
    }

    return tx.Commit()
}

このコードの問題は、SQL文そのものよりも
「どこまでが原子的に成功すべき単位か」が見えないことにある。

なぜ危ないのか

orders 更新に成功し、order_histories も成功したあと、publishOrderCompleted() が失敗したとする。
この時点でレビューアーが知りたいのは、単なる「通知エラー」ではない。

  • DB更新はもう反映済みなのか
  • まだcommit前なので rollback で巻き戻せるのか
  • 通知だけ再実行してよい設計なのか
  • 呼び出し側が再試行したとき二重通知や二重履歴が起きないか

さらに、defer tx.Rollback() がない構造では、途中returnのたびに
rollback漏れがないかを人間が制御フローで追いかける必要が出る。

レビューでこれは避けたい。
安全なコードは、失敗時の後始末が構造に埋め込まれていなければならない。

事故シーケンスで見ると本質が見えやすい

UML Diagram

ここでの危険は、Commit() を呼んだかどうかだけではない。
DBと外部副作用の境界をどう分けるかが曖昧なまま実装されている点が核心だ。

レビューで見たい3つの判断線

1. BeginTx したら rollback の予約が即座にあるか

Goのトランザクション処理では、BeginTx 成功直後に
defer tx.Rollback() があるかをまず確認したい。

Comment 
@Reviewer: `BeginTx` 後の rollback 予約がありません。途中return経路を個別に追わないと安全性が判断できないため、`defer tx.Rollback()` を起点に失敗時の後始末を固定してください。

Commit 成功後の Rollback は sql.ErrTxDone になるので、
まず rollback を予約して最後に commit で確定する形がレビューしやすい。

2. DB更新と外部副作用が同じ成功単位に見えていたら止める

通知送信、メール配信、別API更新などがトランザクション関数に直接入っていると、
「DBだけ成功」「通知だけ失敗」の扱いが不透明になる。

Comment 
@Reviewer: トランザクション内更新と外部通知が同一責務になっています。ローカルDBの原子性と外部副作用の整合戦略は別問題なので、outboxや後続非同期処理への分離を検討してください。

ここはレビューアーが遠慮せず設計論点として出すべき箇所だ。

3. commit条件が複数責務の成否にまたがっていたら止める

この種の実装では、何が成功条件なのかが曖昧になりやすい。

  • DBの整合が守れたら成功なのか
  • 通知まで含めて成功なのか
  • 通知失敗時は再送基盤で補償するのか
Comment 
@Reviewer: 関数の成功条件が「DB整合完了」なのか「通知完了」なのか読み取れません。commit判定の責務境界を明示し、失敗時の補償方針をコードまたは設計コメントで示してください。

改善方針：DB原子性と外部副作用を分離する

最低限、トランザクション部分は次のように読みやすくしたい。

rollback責務を固定した改善例 
func CompleteOrder(ctx context.Context, db *sql.DB, orderID string) error {
    tx, err := db.BeginTx(ctx, nil)
    if err != nil {
        return err
    }
    defer tx.Rollback()

    if _, err := tx.ExecContext(
        ctx,
        "UPDATE orders SET status = ? WHERE id = ?",
        "completed",
        orderID,
    ); err != nil {
        return err
    }

    if _, err := tx.ExecContext(
        ctx,
        "INSERT INTO outbox(topic, payload) VALUES(?, ?)",
        "order.completed",
        orderID,
    ); err != nil {
        return err
    }

    if err := tx.Commit(); err != nil {
        return err
    }

    return nil
}

ここでは、通知自体は後続の outbox 消費側へ分離している。
重要なのは、この関数の責務を「DB内で一貫して確定するところまで」に絞ったことだ。

レビュー観点としても次が読みやすくなる。

  • rollbackの起点が固定されている
  • commit前に完了すべきDB更新が明確
  • 外部副作用を別責務へ逃がしている
  • 再試行時の整合性戦略を設計しやすい

defer tx.Rollback() があっても安心できないケース

次のような実装は、rollback予約があっても通してはいけない。

  • goroutineを起動して tx を並行利用している
  • rows.Close() や Scan() のエラー処理が抜けている
  • Commit() 後に「失敗したら rollback すればよい」という前提で外部通知している
  • リポジトリ層をまたいで tx の所有者が曖昧
Comment 
@Reviewer: rollback予約は入っていますが、`tx` の所有境界が不明です。どの層が Begin/Commit/Rollback を握るのか統一されていないと、将来の更新追加で二重管理が起きやすくなります。

レビュー観点チェックリスト

sql.Txレビューの確認項目
  • BeginTx 成功直後に defer tx.Rollback() があるか
  • commit前のreturn経路で一貫性が崩れないか
  • DB更新と外部副作用が同一責務に混在していないか
  • 成功条件が「DB整合完了」か「外部通知完了」か明確か
  • tx の所有者がレイヤー間で統一されているか
  • 再試行時の重複実行や補償方針が説明できるか

おわりに

トランザクションレビューで見るべきなのは、
SQLの書き方よりも失敗時にどの状態へ戻せる設計かである。

Commit を書いていれば安全なのではない。
Rollback がどこで予約され、何を原子的成功単位として扱い、
外部副作用をどう切り離すかまで読めて、はじめて安全性を判断できる。
レビューでは、更新処理の流れより一貫性境界の明瞭さを優先して見るべきだ。