Goのerrgroup導入でキャンセル責務が曖昧な実装をどうレビューするか

Goで複数の外部呼び出しや並列I/Oをまとめる実装は、goroutine と sync.WaitGroup だけでも一応は書ける。
ただし、失敗時の停止責務・エラー伝播・チャネル終了条件を個別に手組みし始めると、レビューで止めるべき構造が急増する。

特に危ないのは、以下のような状況だ。

  • 複数goroutineのうち1つが失敗しても、他がそのまま走り続ける
  • ctx は受け取っているが、各処理の停止条件に使われていない
  • errCh と WaitGroup と close() の責務分担が曖昧
  • レビューコメントが「動くのでOK」で通りやすい

この記事では、errgroup.WithContext を使うべきかどうかではなく、
レビューアーがどの構造を見たら差し戻すべきかに絞って整理する。

まず止めたい実装

以下は、検索APIと権限APIを並列で呼んでレスポンスを組み立てる例だ。
動作自体はそれらしく見えるが、レビューではかなり慎重に止めたい。

危険なWaitGroup手組み実装 
func BuildDashboard(ctx context.Context, userID string) (*Dashboard, error) {
    var wg sync.WaitGroup
    errCh := make(chan error, 2)

    var profile *Profile
    var permissions []Permission

    wg.Add(2)

    go func() {
        defer wg.Done()

        result, err := fetchProfile(ctx, userID)
        if err != nil {
            errCh <- err
            return
        }
        profile = result
    }()

    go func() {
        defer wg.Done()

        result, err := fetchPermissions(ctx, userID)
        if err != nil {
            errCh <- err
            return
        }
        permissions = result
    }()

    wg.Wait()
    close(errCh)

@Reviewer
複数goroutineの失敗集約を手組みしていますが、最初の失敗で他処理を止める責務が存在しません。失敗後も不要な外部呼び出しが継続し、キャンセル境界が崩れています。

    if err := <-errCh; err != nil {

@Reviewer
`Wait()` 完了まで全goroutineを待ってから最初の1件だけエラー取得しており、失敗の即時返却・追加エラーの扱い・停止方針が曖昧です。`errgroup.WithContext` 等で責務をまとめてください。
        return nil, err
    }

    return &Dashboard{
        Profile:     profile,
        Permissions: permissions,
    }, nil
}

一番の問題は、コード量ではない。
「どこで処理全体を諦めるのか」が構造として読めないことが危険だ。

何が危ないのか

この実装で fetchProfile() が早い段階で失敗しても、fetchPermissions() はそのまま走り続ける。
対象がDB読み取りだけならまだしも、実務では次のようなケースが普通に混ざる。

  • 外部APIの高コストな照会
  • レート制限付きのアクセス
  • 監査ログを伴う処理
  • 下流のタイムアウトが長いI/O

つまり、レビューで見たいのは「並列化しているか」ではなく、
失敗を検知したあとに不要な仕事を止められるかである。

さらに、手組み構造では次の曖昧さも入りやすい。

  • 何件までエラーを貯めるのか
  • どのエラーを代表値として返すのか
  • goroutine側が ctx.Done() を見て停止する保証があるか
  • 将来 goroutine が3個、4個に増えたとき安全に保守できるか

ここが曖昧なままマージすると、
成功系よりも障害系でだけ壊れる並列実装になりやすい。

事故シーケンスで見ると分かりやすい

UML Diagram

この順序を見ると、問題はかなり明確だ。
エラーを受け取れていても、処理全体の終了制御に変換されていないのである。

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

1. 失敗通知と停止通知が分離していたら止める

errCh に送るだけでは、他goroutineは止まらない。
失敗を通知する仕組みと、全体を停止する仕組みが別々に散らばっているなら、かなり危険信号だ。

Comment 
@Reviewer: エラー通知はありますが、並列処理全体を止める統制がありません。失敗後も他goroutineが走り続けるため、停止責務を `errgroup.WithContext` など1か所へ集約してください。

ここでは「あとで改善」ではなく、
構造上の欠陥として差し戻す理由になると考えたほうがよい。

2. ctx を渡していても、停止条件に使っていなければ止める

Goのレビューでは ctx が関数引数にあるだけで安心されやすい。
しかし本当に見たいのは、各処理が ctx.Done() や NewRequestWithContext() に接続されているかだ。

Comment 
@Reviewer: `ctx` は受け取っていますが、失敗した兄弟goroutineからのキャンセル波及が実装上保証されていません。contextを「受け取るだけ」で終わらせず、停止条件として利用してください。

ctx の存在そのものではなく、
キャンセルが処理停止へつながる配線があるかを読む必要がある。

3. WaitGroup とチャネルの手組みが責務過多なら止める

WaitGroup 自体が悪いわけではない。
ただし、次の責務を同時に背負い始めたら過積載になりやすい。

  • goroutineの終了待ち
  • エラー集約
  • 代表エラー選定
  • 失敗時キャンセル
  • チャネルクローズの整合
Comment 
@Reviewer: `WaitGroup` と `errCh` の手組みで、終了待ち・エラー集約・キャンセル制御まで1か所に詰め込まれています。責務が増えるほど close 条件や将来変更で破綻しやすいため、集約専用の構造へ寄せてください。

レビューアーが見たいのは巧妙さではない。
障害系を安全に畳めるかどうかである。

改善方針：失敗時キャンセルを構造に埋め込む

この論点では、errgroup.WithContext がかなり相性がよい。
理由は単純で、レビューで欲しい責務を最初からまとめやすいからだ。

  1. 最初のエラーを返した時点でグループ全体をキャンセルできる
  2. Wait() が代表エラー返却の窓口になる
  3. 子処理へ渡す ctx が明確になる
  4. goroutine追加時も責務が増えにくい

改善後の例は次のようになる。

errgroupで責務をまとめた実装 
func BuildDashboard(ctx context.Context, userID string) (*Dashboard, error) {
    group, groupCtx := errgroup.WithContext(ctx)

    var profile *Profile
    var permissions []Permission

    group.Go(func() error {
        result, err := fetchProfile(groupCtx, userID)
        if err != nil {
            return err
        }
        profile = result
        return nil
    })

    group.Go(func() error {
        result, err := fetchPermissions(groupCtx, userID)
        if err != nil {
            return err
        }
        permissions = result
        return nil
    })

    if err := group.Wait(); err != nil {
        return nil, err
    }

    return &Dashboard{
        Profile:     profile,
        Permissions: permissions,
    }, nil
}

これで全て解決するわけではない。
重要なのは、子処理側も groupCtx を実際の停止条件に接続していることだ。

たとえばHTTP呼び出しなら、最低限ここまで確認したい。

contextを実際の停止へ接続する例 
func fetchProfile(ctx context.Context, userID string) (*Profile, error) {
    req, err := http.NewRequestWithContext(
        ctx,
        http.MethodGet,
        "https://profile.example.com/users/"+userID,
        nil,
    )
    if err != nil {
        return nil, err
    }

    resp, err := http.DefaultClient.Do(req)
    if err != nil {
        return nil, err
    }
    defer resp.Body.Close()

    if resp.StatusCode != http.StatusOK {
        return nil, fmt.Errorf("profile api returned %d", resp.StatusCode)
    }

    // decode処理
    return &Profile{}, nil
}

errgroup を入れても通してはいけないケース

errgroup を使っていれば自動的に安全、ではない。
次のケースでは引き続きレビューで止めたい。

  • 子処理が groupCtx ではなく元の ctx や Background() を使っている
  • http.NewRequestWithContext() を使わず、キャンセルがI/Oへ伝わらない
  • goroutine内で共有変数への書き込み競合がある
  • 「一部失敗しても続行したい」要件なのに、最初の失敗で全停止する設計を説明していない

特に最後は重要で、errgroup は
fail-fast が業務要件に合っているときに強いという前提がある。

Comment 
@Reviewer: `errgroup` の採用自体は妥当ですが、「1件失敗したら全体を中断する」要件根拠がコードから読めません。部分成功を許す要件なら、集約方針を別設計にしてください。

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

errgroup導入レビューの確認項目
  • 失敗通知と停止通知が同じ構造で管理されているか
  • errgroup.WithContext で得た groupCtx が子処理に伝播しているか
  • 子処理のI/Oが ctx を実際の停止条件に接続しているか
  • WaitGroup + errCh の手組みで責務が増殖していないか
  • fail-fast が業務要件と一致しているか
  • 共有変数書き込みに競合がないか

おわりに

この論点でレビューアーが見るべきなのは、
errgroup を知っているかどうかではない。
失敗が起きた瞬間に、処理全体をどう畳む設計になっているかである。

WaitGroup の手組みが悪いのではなく、
そこにキャンセル・エラー集約・停止責務まで背負わせた瞬間に危険度が上がる。
レビューでは、並列化の美しさではなく、障害系の終了戦略を読み取ることが重要になる。