最近のインシデントからの学び

この記事はOptimind Advent Calendar 2023の25日目の記事となります。

---

経緯

本来別の記事を書く予定でしたが、直近プロダクトのインシデントがあったため、自分が直接責任者でもあり、その振り返りをこの機に書きたいと思いました。

今のプロダクトはユーザーの利用時間避けるために、暫定毎週の月曜にリリースしています。すると翌日の9時あたりに、ユーザーが利用したエラー記録がスラックのチャンネルに流れてきました。

その内容をチェックしたら、なんと、column reference "xxx" is ambiguousでした。

いやいやいやいや、まずいぞこれ、って第一印象です。次の瞬間に、ジョインしている時にテーブル名指定してないのでは？と答えも出ています。

が、こんなシンプルなものではありませんでした。

直接原因

曖昧なカラム名、これはSQLの経験者なら、必ずというほど経験されたエラーではないでしょうか。そのエラーは言葉通り、xxxという名前で、カラムをレファレンス・参照しようとしているが、このxxxが曖昧すぎで、どこに参照すればわからない、とのことです。

例えば、productテーブルとcategoryテーブルがあるとします。そしていずれもnameとのフィールドが存在します。すると、両者のテーブルをジョインする時に、2つのnameというフィールドが存在するようになります。もしnameをフィルター条件とかに入れると、次の状況になります。

select * from product where name = '曖昧'; # -> OK
select * from product left join category on product.category_id = category.id where name = '曖昧'; # -> ERROR

# error: select * from "product" left join "category" on "..."."..." = "..."."..." where "name" = $1 - column reference "name" is ambiguous

nameだけでは情報不足となるため、ジョイン時に基本テーブル名をつけるのが良い実践です。

で、なぜこれが起こったかというと、直近のリファクタリングで、ジョインを追加したにも関わらず、条件のところでテーブル名の追加が漏れて、偶然その条件のフィールドが重複命名のものであったため、このようなエラーとなりました。

根本原因

これはこれで問題ではありますが、私が非常に不可解と思ったのは、ジョイン時に命名が曖昧な箇所がないはずなのに、なぜこれが起こったのか、とのところでした。なぜなら、このリファクタを含めた変更自体は、リリース対象になっていないため、今の本番環境には起こらないはずです。

それでためらっても時間の無駄なので、私はこの問題を見てやく10分後、インシデントだと判断して社内に発報しました。各チームの関係者が迅速に集まって、とりあえず対策を検討していました。このエラーだけをみると一瞬で修復できそうなもので、ホットフィックスの形で対応すると決めました。

調査を進める上で、「なんらかの理由で、リリース予定のないものがリリースされてしまった」という仮説を立てました。

リリースはGitHub Actionsのワークフローを利用して、コードからイメージをビルドし、gcloudのartifacts repositoryに上げていました。

ただ、今回のリリースはいつもと大きく違う点が1つあります。それは、mainブランチからビルドしたイメージではなく、フィーチャーブランチからビルドしていたのです。

これはブランチ戦略の問題でもありますが、リリースしたい内容より古いコミットにはリリースしたくない内容が入っていて、複数を全部リバートしておくのも大変だったので、いったん古いコミットから別途ブランチを切って、新しいリリースしたいコミットをcherry-pickして、そのブランチでリリースするようになりました。

わかりやすく説明すると、

main: a -> b -> c -> d -> e
feat: a -> b -> e

• コミットa, b, eがリリース対象で、c, dが対象外
• bからブランチを作って、eをbの上に載せた
• 曖昧なカラム名のエラーはcに入っている
• リリースした（つもり）のは、a, b, eだけ

という状況でした。

原因究明している中で気づいた問題は、ビルドされたイメージはどのコミットハッシュに基づいたか、にありました。つまり、本来eからビルドされるものの、なぜかe以外のコミットになっていた。

それはなんと、checkoutのワークフローアクションの仕業でした。フィーチャーブランチでcheckoutを利用してビルドする際に、デフォルトでは、mainからマージコミットを作り、そのコミットをベースにビルドすることだとわかった。

- if: github.event_name != 'pull_request'
  uses: actions/checkout@v4 # -> この状態ではmainからマージコミットが作られます
- if: github.event_name == 'pull_request'
  uses: actions/checkout@v4
  with:
    ref: ${{ github.event.pull_request.head.sha }} # -> main以外のブランチでやりたい場合はPRのheadハッシュを指定

つまり、リリースするつもりがなかったものが、まん丸とリリースされていました！！なんてこった！！！と。

ちなみに、公式ドキュメントには、PR時にpull_request.head.shaを使ってね、って後から見つかったが、其の理由が述べられていませんでした。同じ疑問を持つ方が前々からいたので（こちら）、なぜこのままにしたのか。。

ポストモーテム

うまく行ったこと

• インシデント自体の発覚が早く、社内、そしてお客様に周知されるのは30分ほどスピーディーにできた
• 原因究明後の動作確認とリリースがスピーディーにできた

うまくいかなかったこと

• 根本原因がわかりにくく、究明するまで1時間以上もかかった（単に曖昧なカラム名なら1秒くらいだったが）

ラッキーだったこと

• 初めてmainブランチ以外のリリースを運用して、エラーが出たので、ある意味で爆弾を早い段階で発見できた

取るべきアクション

• テスト体制の整備
  • バックエンドのテストか、それを巻き込んだE2Eがあれば、このようなことがCIとか、検証環境の時点でわかるはず
  • 次の安定性を高めるフェーズの軸としてはテスト体制の整備に注力することに
• リリースのバージョンとコミットハッシュを確認できるようにする
  • どのコミットをベースにビルドされているか、逆引きする手段がなく、一々ワークフローの中で探すしかない不便さがある
  • よりすぐにわかるように、レスポンスヘッダーとかにつけたりするのを検討

2024/01/17 追記 こちらのGCPベストプラクティスによると、そもそもコンテナのタグをコミットハッシュにするのもありらしい。ただ現状ではPR番号もしくはブランチ名でタグつけられている。もしかするとハイブリッドで考えらるかもしれない。

考え

いわゆる凡ミスでコケるのは、確かに凹みます。というのも、つい先日コードレビューの中で、select文で*ではなく、カラム名を出したい状況の一つは、曖昧参照を避けるためのテーブルジョインですよ、と自分がコメントしていました。まさかすぐに自分がそれでコケるのは、なんというドラマチックなシナリオか。。。

ただ、実際にこのエラーが起こっていなければ、根本原因となっているGitHub Actionsワークフローの問題がいつ事故るか、予想がつきません。という意味で、これもちろん良いことではなく、あくまでも幸運だったことだと思っています。

早い段階でバグを見つけ出すと言うことは、Software Engineering At Googleの11章にも、The later in the development cycle a bug is caught, the more expensive it is; exponentially so in many casesというふうに言及されています。

このような教訓は大体、ちゃんとテストしろよ、との結論になりやすい。なぜテストを大事にしなければならないのか、Software Engineering At Googleから読んでみると、少なくとも以下とのメリットがあります。

• 変化に強いプロダクトを作れる ソフトウェアは変わり続けます。今回のリファクタによるデグレはまさに検知できるはずですし、新機能実装する際に他に影響を及ぼすかはテストでわかります。変化に強いというのは非常に大事な品質でもあり、開発チーム、会社全体、ユーザーがプロダクトへの信頼感・コンフィデンスにも直結すると言えます。
• 開発スピードを上げられる テストで防止できるバグに振り回されて結局貴重な時間だけではなく、ユーザーからの信頼を損ねる可能性もあります。自動化テストをCIなどに組み込むことで、デバッグやコードレビューの時間短縮、開発フロー全体の信頼性とスピードを上げてくれます。
• システムの設計を改善する力になる テストしやすいコードという言葉は聞きますが、テストできるために、ロジックの分離、デカップリング、エッジケースのカバーなど、コード自体に対する考えを深めてくれるのです。逆にいうと、よくあるパターンというのは、テストの実装が遅れて、結局リファクタを先にやらないとテストすらやりにくいケースがあります（その場合は単体を一旦後回しに、E2Eを先行にする策もあるが、できる限り最初から単体テストと一緒に実装したい）
• 後からテストを追う大変さが普段に吸収される 実装されていないのが溜まっていくとますます負債がデカくなり、さらに身動きが取りづらくなります。
• ドキュメンテーションの改善につながる テストはある意味で、実行可能なドキュメント（executable documentation）でもある。コードが何をするものか、どのケースでどのようなアウトプットになるのか、テストを見るのが確かに良い視点です。テストが壊れると、ドキュメント（テスト）の更新もやらなければならないので、ありがちなドキュメントが放置される問題が起こりにくくなります。

技術的負債を聞くと、設計とか、コードのメンテナンス性、拡張性とか言われるのですが、堅牢なテストの欠如も、正しく大きな負債になるのを、痛感しています。今後のこのような一つ一つの教訓が、我々開発者のテストヘの信仰を深めてくれると信じています。

またの機会で、テスト関連の話をしようと思います。

一旦今日はこれで。