Fail-Open Fallback Silently Deletes Other Schools' Ads

Conclusion

When building a delivery for a multi-school ad loop, falling back on a target-school query error with !lsErr ? rows : [] — a fail-open fallback — shrinks the target list down to just the primary school while delivery itself still succeeds. The cleanup step is implemented to delete any ad row for a school outside this delivery’s target list, so the schools that dropped out of the list have their ads silently deleted. And because the send is treated as a success, the outbox confirms it as sent immediately — no retry, no alert.

Symptom

Under contracts where multiple schools share the same ad loop, a single placement fans out ad rows to the primary school plus however many additional schools belong to that loop. Resolving the target schools is a two-step query: look up placements.loop_id, and if the placement belongs to a loop, fetch the target school list from placement_loop_schools.

Before the fix, a failure in either query silently fell back to just the primary school.

// at sha 4972543 (before fix): errors fall through to an empty array, unchecked
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)];
  }
}

Even when lsErr is set, rows ends up as a plain empty array, so if (rows.length > 0) never runs, and targetSchoolIds leaves the function still holding only the primary school it was initialized with. From the caller’s point of view, this is indistinguishable from “a delivery for just one school, built normally.”

Because this function returns a normal payload instead of an error, the send itself succeeds. The cleanup step for delivered placements is implemented to keep only the ad rows for schools that were part of this send and delete everything else — so when a 3-school contract only ends up sending ads for 1 school, the other 2 schools’ ad rows get deleted on the spot.

Cause

This code was itself part of a change written to fix a different incident: “a 3-school contract only delivers to 1 school.” Under the old design, the idempotency key for a delivered row was portal_placement_id alone, so sending ad rows for multiple schools from the same placement left only the last school’s row standing — a silent single-school delivery. The main point of this change was widening that idempotency key to a per-school composite key, so multiple schools could stand independently as separate ad rows.

In review, it was pointed out that a different code path right next to this fix — the query-error fallback — reproduces the exact bug it was trying to fix. A single transient DB error on the target-school query is enough to shrink a multi-school loop down to “delivery to the primary school alone.” The permanent incident from before the fix had simply turned into an intermittent one, now triggered by a transient error instead.

Worse, this code path leaves no trace of failure. The function swallows the query error and returns a normal-looking value (empty array → primary school only), so the caller has no way to know that resolving the target schools failed. The send is processed as a success, and the outbox’s state machine is designed to move a successful send straight to sent and stop retrying. No error log, no warning — just the other schools’ ad delivery, deleted.

The fix

For both of the two queries that resolve the target schools, the error is no longer flattened into an empty array. Instead it’s returned to the caller as a hold, carrying transient: true.

// at sha 2bed6a9f (after fix): errors are returned as a hold
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`,
    };
  }
  // ...
}

The caller, sendV2Delivery, reads this transient flag to tell a fatal failure apart from a temporary one.

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

A row returned with fatal: false is not immediately moved to dead (delivery abandoned) by the outbox’s state machine — it’s kept as failed and retried with exponential backoff. Flip that to a permanent (fatal) failure instead, and the delivery stays stuck even after the DB recovers, until a human manually resubmits it. The same distinction this design already applies elsewhere — “a transient error can recover on retry” versus “only mark it permanent if a human has to fix it” — now also applies to the target-school lookup error.

Preventing a repeat

The fix commit adds a regression test that swaps out just one of the two target-school queries with an error.

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);
});

Instead of stubbing out the whole DB client, only the query against placement_loop_schools is replaced with a timeout error, and the core logic that resolves the target schools runs unmocked, as the real implementation. That means the next time someone writes a similar “what do we fall back to on query failure” branch, falling back to an empty array leaves result.transient as false, and this test fails.

What makes this incident dangerous is that the fix itself reproduced the exact same outcome as the bug it was fixing (a multi-school contract shrinking to a single-school delivery) through a separate code path. Fail-open — resolving a query failure toward “narrow the target” rather than toward safety — looks harmless by itself. But in a design where a later step deletes anything that wasn’t in this delivery’s target, narrowing the target is a delete operation. Wherever that combination exists, choosing what to fall back to on error has to account for whether a cleanup step follows it.