outboxパターンのfail-openで他校の広告配信がsent扱いのまま消えた

結論

複数校ループの配信組み立てで、対象校の取得エラーを !lsErr ? rows : [] のように空配列へ倒すfail-open実装にすると、対象校が代表校1件に縮んだまま送信自体は成功する。掃除処理は今回送られた校以外の広告行を削除する実装なので、対象から漏れた学校の広告配信が黙って消える。しかも送信は成功扱いなのでoutboxは即座にsent確定し、再送もアラートも発生しない。

症状

複数校が同じ広告ループに参加している契約では、1つの placement から代表校＋追加校ぶんの広告行を配信先へ並立させる。対象校を決める処理は placements.loop_id を引き、loopに属していれば placement_loop_schools から対象校の一覧を取ってくる、という2段のクエリになっている。

修正前のコードは、このどちらのクエリが失敗しても代表校1件へ静かにフォールバックしていた。

// sha 4972543 時点（修正前）: エラーを見ずに空配列へ倒す
const loopId = !loopErr ? ((loopRow?.loop_id as string | null) ?? null) : null;
if (loopId) {
  const { data: loopSchools, error: lsErr } = await admin
    .from("placement_loop_schools")
    .select("school_id")
    .eq("loop_id", loopId);
  const rows = !lsErr
    ? ((loopSchools ?? []) as { school_id: string }[]).map((r) => r.school_id)
    : [];
  if (rows.length > 0) {
    const primary = placement.school_id as string;
    targetSchoolIds = [primary, ...rows.filter((s) => s !== primary)];
  }
}

lsErr が立っていても rows はただの空配列になり、if (rows.length > 0) を通らないので targetSchoolIds は関数冒頭で初期化した代表校1件のまま外へ出ていく。呼び出し側から見ると、これは「正常に組み立てられた、代表校1件だけの配信」と区別がつかない。

この関数がエラーではなく正常なペイロードを返す以上、送信自体は成功する。配信先の掃除処理は「今回送られた校の広告行だけを残し、それ以外を削除する」実装になっているため、3校契約なのに1校ぶんの広告しか送られなければ、残り2校の広告行がその場で削除される。

原因

このコード自体は、もともと「3校契約なのに1校しか配信されない」という別の事故を直すために書かれた変更に含まれていた。配信先の冪等キーが portal_placement_id 単独だった旧仕様では、同じ placement から複数校ぶんの広告行を送ると最後の1校しか残らず、黙って1校配信になっていた。冪等キーを校単位の複合キーへ広げ、複数校を1つずつ独立した広告行として並立できるようにしたのがこの変更の主目的である。

ところがレビューで、その直そうとしていた当のバグを、修正コードの直後にある別経路——クエリエラー時のフォールバック——が再現していることが指摘された。対象校クエリが一過性のDBエラーで1回失敗するだけで、複数校ループは「代表校1件だけの配信」に縮む。修正前の恒久的な事故が、修正後は一過性のエラーをトリガーにして間欠的に起きる形へ変わっただけだった。

さらに悪いことに、このコードパスには失敗の痕跡が残らない。関数はクエリエラーを飲み込んで正常値（空配列→代表校のみ）を返すため、呼び出し元は「対象校の解決に失敗した」ことを知る手段がない。送信は成功として処理され、outboxの状態遷移は成功したものを即座に sent へ確定して再送を止める設計になっている。エラーログも警告も残らないまま、他校の広告配信だけが削除される。

直す

対象校を決める2つのクエリそれぞれについて、エラーを空配列へ倒すのをやめ、transient: true を伴う保留として呼び出し元へ返すようにした。

// sha 2bed6a9f 時点（修正後）: エラーは保留として返す
if (loopErr) {
  return {
    ok: false,
    transient: true,
    reason: `placement ${placementId}: loop lookup failed (${loopErr.message}) — holding; cannot tell whether this is a multi-school loop`,
  };
}
const loopId = (loopRow?.loop_id as string | null) ?? null;
if (loopId) {
  const { data: loopSchools, error: lsErr } = await admin
    .from("placement_loop_schools")
    .select("school_id")
    .eq("loop_id", loopId);
  if (lsErr) {
    return {
      ok: false,
      transient: true,
      reason: `placement ${placementId}: loop school lookup failed (${lsErr.message}) — holding; delivering to the primary school alone would delete the other schools' ads`,
    };
  }
  // ...
}

呼び出し元の sendV2Delivery は、この transient フラグを見て致命的な失敗と一時的な失敗を区別する。

const built = await buildDeliveryPayload(admin, creativeId, placementId);
if (!built.ok) {
  return { ok: false, fatal: built.transient !== true, error: built.reason };
}

fatal: false で返った行はoutboxの状態遷移で即座にdead（配信打ち切り）にはならず、指数バックオフを挟みながらfailedとして再送され続ける。逆にここを恒久失敗（fatal）扱いにすると、DBが復旧したあとも自動では配信されず、人が手動で再投入するまで止まったままになる。「一過性のエラーは再送で回復しうる」「人が直さないと直らないものだけを恒久失敗にする」という区別を、対象校の取得エラーにもそのまま当てはめた形になる。

再発防止

修正コミットには、対象校クエリの一方だけをエラーへ差し替える回帰テストが追加されている。

it("対象校の取得に失敗したら保留（単独校に倒すと他校の配信が消される）", async () => {
  // fail-open だと targetSchoolIds が代表校1件に縮み、v2 の掃除が他2校の広告行を削除する。
  const admin = makeAdmin(MULTI);
  const orig = admin.from.bind(admin);
  (admin as unknown as { from: unknown }).from = (t: string) => {
    const b = orig(t) as unknown as Record<string, unknown>;
    if (t !== "placement_loop_schools") return b;
    return {
      ...b,
      select: () => ({
        eq: () => Promise.resolve({ data: null, error: { message: "timeout" } }),
      }),
    };
  };
  const result = await buildDeliveryPayload(admin, "cre-1", "plc-1");
  expect(result.ok).toBe(false);
  if (result.ok) throw new Error("unreachable");
  expect(result.reason).toMatch(/loop school lookup failed/);
  expect(result.transient).toBe(true);
});

DBクライアントのモックを丸ごと差し替えるのではなく、placement_loop_schools への問い合わせだけをタイムアウトエラーに置き換え、対象校を決める本体のロジックはモックせず実体のまま通している。これによって、次に似た「クエリ失敗時に何へフォールバックするか」という分岐を書いたときも、空配列側へ倒せば result.transient が false のままになってこのテストが落ちる。

この事故の怖さは、直そうとしていたバグと同じ結果（複数校契約が1校配信に縮む）を、直す変更そのものの中に別経路で書き込んでしまった点にある。fail-open（クエリ失敗時に安全側ではなく「対象を絞る側」へ倒す）は一見無害な書き方に見えるが、その後段に「今回送られなかった対象を削除する」処理が続く設計では、対象を絞ること自体が削除操作になる。この組み合わせがある経路では、エラー時のフォールバック先を選ぶときに掃除処理の有無を意識する必要がある。