破壊的読み取りがパース失敗を永久的なハングに変える

ワーカーが`GETDEL`でジョブの結果を読み取ったため、解析できなかったペイロードは永久に失われました。これを「まだ準備ができていない」として扱うことは、永遠に待機することを意味しました。

ジョブは18:28に失敗しました。ユーザーには20:30まで「処理中」と表示されていました。失敗通知は時間通りに届き、ワーカーはそれを読み取りました。ただ、それを理解していなかったのです。

バグは、制御フローの1行にありました。帯域外の結果を待機するワーカーが、すべてのフェッチエラーを同じように扱っていたため、「結果はまだない」と「結果はジョブの失敗を示している」の両方が、待機を続けることを意味していました。それだけなら、発見が遅れるバグで済んだでしょう。それを永続的なものにしたのは、読み取り自体でした。ワーカーは破壊的読み取りを使用していたため、失敗通知を誤って読み取った瞬間に、その通知は存在しなくなりました。

結果を求めてキーをポーリングする非同期ワーカーがある場合、この投稿では、小さな誤分類を数時間にわたるハングに変えてしまう2つの事柄と、それが起こらないようにコンシューマーを構築する方法について説明します。

コンシューマーが区別しなければならない2つの状態

この構成はよくあるものです。バックエンドはコンピュートサービスにワークをサブミットし、すぐにIDを受け取ります。コンピュートサービスはその処理を行い、結果を共有キーに書き込み、その後ナッジをパブリッシュします。バックエンドはそのナッジをサブスクライブし、また、pub/subの配信が保証されていないため、ティッカーでキーをポーリングします。

コンシューマーループは、おおよそ次のようでした。

for {
    select {
    case <-ctx.Done():
        return rescueOrError(ctx.Err())

    case msg := <-notifyCh:
        result, err := fetchResult(ctx, key)
        if err != nil {
            slog.Warn("fetch after notify failed", "error", err)
            continue          // keep waiting
        }
        return result, nil

    case <-ticker.C:
        result, err := fetchResult(ctx, key)
        if err == nil {
            return result, nil
        }
        // no result yet, keep waiting
    }
}

あのcontinueをもう一度読んでください。これは、コンピューティングサービスがジョブの失敗を明示的に伝えてきたときに到達します。コンシューマーは警告をログに記録し、スリープ状態に戻ります。

待機中のコンシューマーは、少なくとも3つの結果を区別する必要がありますが、このループはそれらを2つにまとめてしまっています。

まだ準備ができていない。 キーが存在しません。待機を続けます。待機を続けるべきなのは、このケースだけです。
一時的なインフラストラクチャエラー。 キーストアに一時的に到達できないか、ロード中です。結果はおそらく消費されていないため、待機を続けます。
終端。 結果が到着しましたが、それは明示的な失敗か、解釈できないものです。今すぐ停止します。

修正方法は「エラーケースを処理する」ことではありません。それは、3番目のカテゴリをループが認識できる型にし、すべての分岐を同じ述語に通すことです。

type StepError struct{ Step, Server, Message string }      // service said: failed
type ProtocolError struct{ Step, Server, Reason, Raw string } // we cannot parse it

func isTerminal(err error) bool {
    var se *StepError
    if errors.As(err, &se) { return true }
    var pe *ProtocolError
    return errors.As(err, &pe)
}

見た目以上に重要な詳細が2つあります。これらをポインターとして返してください。なぜなら、*StepError ターゲットに対する errors.As は、値型との照合に暗黙的に失敗するためです。また、%v ではなく %w でラップしてください。そうしないと、最初のラッパーでチェーンが壊れ、後続のすべてのチェックが暗黙的に false を返すことになります。

破壊的読み取りがリスクを高める理由

コンシューマーは、アトミックな読み取りと削除（RedisのGETDEL）で結果を読み取りました。その選択自体は妥当なものです。2つのワーカーが同じ結果を消費するのを防ぎ、TTLに頼ることなくキースペースをクリーンに保つからです。

しかし、これにより配信保証はat-most-onceに変換され、パース失敗が意味するものが変わります。

通常のGETでは、不正な形式のペイロードは厄介です。それをログに記録し、値はそのままにしておき、後で決定します。GETDELでは、値はそれを見ること自体によって消費されます。したがって、ルールは絶対的なものになります。

破壊的読み取りが「キーが存在しない」以外の何かを返した場合、それ以降のすべての失敗は致命的です。

無効なJSONは致命的です。statusフィールドがないのも致命的です。statusフィールドが文字列ではなく数値である場合も致命的です。待つことで解決するものは何もありません。なぜなら、待つべきものが何も残っていないからです。元のループはこれらすべてを「待ち続ける」ものとして扱っていたため、それぞれが外部の安全タイマーが作動するまで数時間ハングすることが保証されていました。

ここには真のトレードオフがあります。破壊的読み取りを維持する場合、読み取りとコミットの間にコンシューマーがクラッシュすると結果が失われることを受け入れなければなりません。自身の状態がコミットされた後にGETとdeleteを行う方式に切り替える場合、べき等にはなりますが、重複した消費を処理する必要があります。どちらでも問題ありません。問題なのは、再度見ることができると仮定するコンシューマーのコードを書きながら、破壊的読み取りを使用することです。

ポジティブな許可リスト、ネガティブチェックではなく

ターミナルの型を追加しているときに、同じ関数に潜む、修正中だったものよりも古い2つ目のバグを見つけました:

var status string
if s, ok := raw["status"]; ok {
    json.Unmarshal(s, &status)   // error ignored
}
if status == "error" {
    return nil, errors.New("service reported failure")
}
return &Result{Status: "done", Content: raw}, nil   // everything else

これは1つの不正な値をチェックし、残りのすべてを成功として扱います。これが何を意味するのかを順に見ていきましょう:

ペイロード	以前の動作	正しい動作
`{"status":"done", ...}`	done	done
`{"status":"error", ...}`	失敗、その後呼び出し元に無視される	致命的な失敗
`{}`	done	致命的なプロトコルエラー
`{"status":123}`	done (アンマーシャルエラーは破棄)	致命的なプロトコルエラー
`{"status":"failed"}`	done	致命的なプロトコルエラー
まったくJSONではない	エラー、その後呼び出し元に無視される	致命的なプロトコルエラー

この表の3つの行は、サイレントなデータ破損です。空のオブジェクトが成功したジョブとして報告され、その結果、それに付随する部分的なコンテンツがレコードに書き込まれていました。プロデューサーがたまたま常に整形式のペイロードを送信していたため、誰も気づきませんでした。契約は、強制ではなく、運によって守られていました。

これを逆にします。受け入れる値を指定し、それ以外はすべて可視のエラーにしてください:

switch status {
case "done":
    return &Result{Status: "done", Content: raw}, nil
case "error":
    return nil, &StepError{Step: step, Message: sanitize(msg)}
default:
    return nil, &ProtocolError{Step: step, Reason: "unknown_status:" + status}
}

同じ規律は、あなたが無視しようとしていたアンマーシャルエラーにも適用されます。statusが存在するが文字列ではない場合、それはプロデューサーの契約違反であり、偽の成功に変換されるのではなく、そのエラーを明確に通知することが望まれます。

リトライバジェットには独自のキースコープが必要

失敗が即座に表面化した場合、おそらくリトライを望むでしょう。コンピュート側での一時的なメモリ不足エラーは再試行する価値がありますが、不正な入力はそうではありません。しかし、ペイロードから両者を区別することはしばしばできません。

私の最初の試みでは、「すでに試行したサーバー」の既存の配列をバジェットカウンターとして再利用しました。2人のレビュー担当者が相次いでそれを却下しましたが、2番目の理由が興味深いものです。

最初の問題はoff-by-oneエラーでした。現在のサーバーを追加してから len(servers) >= 1 をチェックすると、最初の試行でジョブが失敗するため、リトライは決して行われません。言われてみれば明らかです。

2番目の問題は、最初の問題を修正したときに何が起こるかです。上限を2に上げると、追加処理が重複を防ぐように保護されていたため、同じワーカーがジョブを再び取得するたびに配列の増加が止まります。増加がないということは、終了条件がないことを意味します。ジョブは永久に再キューイングされます。さらに悪いことに、通常の脱出機構が存在しませんでした。このパスは汎用のリトライカウンターをインクリメントせず、ジョブは各サイクルでクリーンに完了したため、停滞ジョブモニターはそれをスタックしているとは見なしませんでした。

その同じ構造が隣接するエラークラスに対して安全だった理由は、サーキットブレーカーでした。送信の失敗がそのカウンターに数えられたため、数回後にはブレーカーが開き、ワーカーはアイドル状態になりました。これが偶然ループを中断させました。私が行っていた変更は、その偶然を意図的に取り除くものでした。

したがって、バジェットは単調増加カウンターである必要があり、そのキースコープは熟考が必要です。

// key: retry:{job_id}:{step}
const incrWithTTL = `
local v = redis.call("INCR", KEYS[1])
if v == 1 then redis.call("EXPIRE", KEYS[1], ARGV[1]) end
return v`

これに落ち着くまでに私が間違えた3つのこと：

エンティティごとではなく、ジョブごとにスコープを設定する。 親エンティティとステップ名でキーを設定するのは自然に見えますが、ユーザーが同じエンティティとステップに対して複数の独立したジョブをトリガーできる場合、それらは1つのバジェットを共有し、互いにリソースを奪い合います。エンキューごとのIDでそれらを分離し、内部的な再キューイングでもそのIDは維持されます。
サブステップを分離する。 同じジョブレコード内で実行される前処理ステップは、メインステップのバジェットを消費してはなりません。ジョブで宣言されたステップではなく、実際の計算ステップ名を使用します。
INCRとEXPIREをアトミックにする。 INCRはTTLなしでキーを作成します。EXPIREの前にプロセスが停止すると、不滅のカウンターができてしまい、そのIDに対する将来のすべての失敗は、自然な回復なしに即座に致命的になります。1つのLuaスクリプトでこの時間差をなくします。

TTLは、平均ではなく、試行間の最も長い可能性のある間隔から設定します。私の場合、最終的に1回の待機時間の上限の2倍になりました。私の最初の2つの推測（1時間、次に6時間）はどちらも1回の試行が実行されうる時間よりも短く、これではリトライの途中でカウンターが期限切れになり、バジェットがリセットされてしまったでしょう。それは、形を変えた無限再キューイングに他なりません。

ドメインの失敗をサーキットブレーカーに混ぜ込まない

最後の論点は、これらの失敗がプロデューサーの健全性に影響を与えるものとしてカウントされるべきかという点です。

サーキットブレーカーは「このエンドポイントは不調か？」という1つの問いに答えるために存在します。明示的な失敗応答は、それが健全であることの証拠です。それはリクエストを受け入れ、処理を行い、構造化された応答を生成し、結果チャネルを通じてそれを配信しました。ジョブは失敗しました。サーバーは失敗していません。

そのため、明示的な失敗はブレーカーの失敗としてカウントされるべきではありません。そうしないと、一握りの不正な入力でブレーカーが開き、完全に正常なノードがローテーションから外され、無関係な処理が再キューイングされることになります。

プロトコルエラーはその逆です。パースできないペイロードは、バージョンの不一致、シリアライゼーションのバグ、またはデプロイが中途半端に終わっていることを示唆します。それはノードの問題であり、カウントされるべきです。

フォローアップのコミットを必要とさせた、微妙な点があります。単に failure コールをスキップすると、success コールもスキップしてしまう可能性があります。そして、ブレーカーが half-open 状態で in-flight probe slot を追跡している場合、そのスロットは決して解放されません。その結果、ワーカーはプロセスが再起動するまで決して開くことのないゲートの後ろで待機し続けることになります。明示的な解放が必要でした。

func (cb *Breaker) ReleaseProbeAsSuccess() {
    cb.mu.Lock()
    defer cb.mu.Unlock()
    if cb.state == "half-open" {
        cb.state = "closed"     // the server answered, it is reachable
        cb.failures = 0
    }
    cb.inFlight = 0
}

そのメソッドには2つのガードがあります。ブレーカーは通常goroutine間で共有されており、実際のプローブが成功しないまま他の誰かのプローブを閉じてしまうことになるため、ブレーカーが閉じている間に開始されたリクエストからは呼び出さないでください。また、シャットダウンはリモートホストについて何も証明しないため、コンテキストキャンセルの際には決して呼び出さないでください。

未対応の事項

エッジケースについて正直であること、なぜならそれらが次のバグになるからです：

1つのジョブの同時コンシューマー。 もし2つのワーカーが何らかの形で同じジョブを処理した場合、一方はカウント1で再キューし、もう一方はカウント2で諦めるかもしれず、すでに失敗したジョブがプロデューサーに戻ってしまいます。それを防ぐには、単なるカウンターではなく、再キュー時にジョブの状態に対してcompare-and-setが必要です。
多様性のないリトライ。 カウンターは合計試行回数を制限しますが、リトライを別のノードに誘導しません。もし障害がノードローカルなものであれば、やみくもなリトライはバジェットを無駄にします。
プロデューサー契約そのもの。 これらすべては、コンシューマーが曖昧さに対して防御しているということです。本当の修正は、コンシューマーが推測する必要がないように、障害がリトライ可能かどうかを伝えるペイロードです。

デプロイ後、同じ失敗ジョブは2つのノードにまたがって2回試行され、プロデューサー自身のエラーテキストをレコードに保持したまま、2分19秒で解決しました。以前のパスでは6時間以上かかり、原因は一般的なタイムアウトのラベルで上書きされていました。

よくある質問

破壊的読み取りは間違いですか？

いいえ。これは、一度だけの消費を保証するための合理的な方法です。これは責任を移譲するだけです。一度消費すると、パースが最後のチャンスとなるため、読み取り後の失敗はすべて致命的（ターミナル）なものとして扱わなければなりません。間違いは、破壊的読み取りを、再度の確認が可能であると想定するコンシューマーコードと組み合わせることです。

パースエラー時に単純にフェッチをリトライしないのはなぜですか？

なぜなら、破壊的読み取りではフェッチするものが何もないからです。リトライは空のキーを見て、「まだ準備ができていない」と判断し、待機します。これこそが、パースエラーが数時間にわたるハングに発展する仕組みです。

致命的な失敗（ターミナルフェイラー）に対しては何回試行すべきですか？

私の場合、1回のリトライが適切でした。なぜなら、失敗のかなりの割合がコンピューティング側の一次的なリソース枯渇であり、最悪の場合のコストはステップの追加実行1回分だったからです。プロデューサーがエラーがリトライ可能かどうかを報告する場合は、固定の回数ではなくその情報を使用してください。固定の回数は、自分が持っていない情報の代用です。

プロトコルエラーと明示的な失敗は両方ともリトライすべきですか？

私は両方ともリトライします。なぜなら、検証できない分類法よりも、安価で均一なポリシーの方が推論しやすかったからです。それらは、試行回数ではなく、ブレーカーにどのように影響するかという点で異なります。

プロデューサーのエラーテキストを安全に保存するにはどうすればよいですか？

切り詰める前に正規化を行い、後から行ってはいけません。不正なUTF-8を修正し、制御文字と双方向オーバーライドを削除し、内部パスとURLを墨消ししてから、長さに合わせて切り詰めます。先に切り詰めてしまうと、パスやURLの半分が残り、墨消しパターンに一致しなくなります。そして、生の未パースのペイロードは、システム全体で最も信頼性の低い文字列であるため、ユーザーに届くどのフィールドにも含めないようにしてください。