admin/accounted
3
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(webhooks): derive the stuck-in_flight window from the cycle bound and charge the stall an attempt (#1311)

Browse Source 
* fix(webhooks): derive the stuck-in_flight window from the cycle bound and charge the stall an attempt

recoverStuckInFlight re-armed any in_flight row older than 2x
REQUEST_TIMEOUT_MS (20 s), but a cron cycle claims 50 rows and attempts
them serially, stamping updated_at once at claim time. From row 3 onward
every row was past the threshold before its own attempt started, so each
cycle recovered and re-claimed the rows the previous cycle was still
working through: duplicate POSTs of the same X-Gnubok-Delivery, and a
terminal status decided by a race whose loser was swallowed by
enforce_webhook_delivery_immutability as a log.warn.

Both halves of #1257 are fixed:

1. The window is derived, not guessed. The attempt loop is now bounded by
   an explicit CYCLE_BUDGET_MS (120 s) instead of relying on the platform
   to kill it, and the sweep window is that bound plus one receiver
   timeout plus slack (160 s), floored at the cron's own batch size so
   the 5-row emit kick cannot re-arm rows the 50-row cron still owns.
   Each row is also re-stamped immediately before its own attempt, so a
   row's in_flight age measures the attempt rather than the claim. The
   same write doubles as an ownership check: a zero-row result means
   another cycle took the row, and the POST is dropped instead of
   duplicated.

2. The sweep charges an attempt, so MAX_ATTEMPTS is a real cap again.
   The predicate moves into a SECURITY DEFINER RPC because PostgREST can
   express neither `attempts = attempts + 1` nor the conditional flip at
   the cap, and a read-then-write loop would reopen a TOCTOU against the
   immutability trigger. A row recovered past the cap lands on exactly
   the terminal state the normal retry path produces: status 'dead',
   attempts = MAX_ATTEMPTS, error prefixed 'attempts_exhausted'. The
   trigger is neither weakened nor bypassed: the outer UPDATE keeps
   status = 'in_flight' in its own WHERE, so a row that raced to a
   terminal status fails re-evaluation under READ COMMITTED and is
   skipped rather than aborting the statement.

Rows the cycle claimed but will not reach are handed back as re-claimable
instead of being stranded in in_flight, without charging an attempt they
never made. Adds the partial index the sweep needs (idx_webhook_deliveries_due
is partial on pending/failed and structurally excludes in_flight).

No retention or pruning cron: webhook_deliveries still has no cleanup
path, which is a separate decision and stays a follow-up.

Fixes #1257

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(webhooks): back the cycle budget with maxDuration and give the stall the normal retry backoff

Review follow-up on the #1257 fix. Two of the findings were blocking and
compound each other: the fix made stranding likely and destructive at the
same time.

1. The 160 s sweep window was derived from CYCLE_BUDGET_MS, but nothing
   granted a dispatch cycle 120 s: the cron route declared no
   maxDuration. If the platform killed the invocation before the budget
   check fired, releaseUnattempted never ran and the claimed-but-
   unattempted rows stayed in in_flight carrying their claim-time
   updated_at, which is exactly the invariant the window depends on.
   The route now declares maxDuration = 300, the way the stripe
   transactions and documents verify crons pair a budget with one, and a
   route test asserts both the literal and its relation to
   CYCLE_BUDGET_MS. The kick path can never be given a maxDuration
   (after() runs inside an arbitrary route), so dispatch-kick.ts now
   states why it does not need one: KICK_BATCH_SIZE x REQUEST_TIMEOUT_MS
   is 50 s, so the dispatcher's budget check never fires there.

2. The sweep charged an attempt but re-armed at p_now, i.e. no backoff,
   while the normal failure path waits RETRY_BACKOFF_SECONDS. A row that
   kept getting stranded (deploy, instance recycle, any cycle that
   outlives its invocation) was re-claimable on the next per-minute tick
   and could burn all 8 attempts in roughly 20 minutes, landing in the
   terminal, immutable 'dead' state without its receiver ever being
   contacted. Pre-fix that loop was infinite but harmless, so this was a
   net-new way to lose a delivery. recover_stuck_webhook_deliveries now
   takes p_backoff int[] (RETRY_BACKOFF_SECONDS, still single-sourced in
   TS) and sets next_attempt_at with the same clamped index lookup
   markFailedForRetry uses, so a stall costs an attempt AND the same wait
   a 500 costs. A non-positive or empty schedule is rejected rather than
   silently degrading to p_now. The migration has not been applied to any
   deployed environment, so it is amended in place rather than superseded;
   it drops the old 3-argument signature so no ambiguous overload can
   survive in a dev or CI database.

Also from the review:

- stuckInFlightAfterMs(batchSize) was dead code whose Math.min clamp made
  every input return 120_000, so the documented DEFAULT_BATCH_SIZE floor
  never fired and the test that pinned it (stuckInFlightAfterMs(5) ===
  stuckInFlightAfterMs(50)) was a tautology. It is now the plain constant
  STUCK_IN_FLIGHT_AFTER_MS with a comment that credits the budget, and
  the test drives the window through dispatchDueDeliveries at batch sizes
  5, 50 and 500, which fails if the window ever becomes batch-derived
  again.
- The sweep's outcome reaches the operator: recovered / recoveredDead are
  on DispatchSummary and in the cron's structured log, so a tick that
  takes deliveries terminal is visible without grepping helper-level warn
  lines.
- releaseUnattempted no longer writes 'failed' onto a never-attempted
  row. claim_due_webhook_deliveries does not return the pre-claim status,
  but it does return attempts, and every path that writes 'failed' also
  writes attempts >= 1, so attempts = 0 identifies a row that was
  'pending' and it is restored as such. webhook_deliveries is
  customer-visible behandlingshistorik; a delivery that was claimed and
  handed back without a single POST must not read as a failure there.

The two deferred hygiene items (no retention path for webhook_deliveries,
and the sweep still being an unbounded tenant-global UPDATE) are reported
as a comment on #1257 and noted in the migration.

Fixes #1257

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
2026-07-30 18:51:41 +02:00
committed by GitHub
co-authored by Claude Opus 5
parent f3bf50d862
commit 1a5d205bd6
9 changed files with 1599 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/webhooks/dispatcher.ts
+248 -32
View File
@@ -45,6 +45,48 @@ const MAX_ATTEMPTS = RETRY_BACKOFF_SECONDS.length + 1 // initial + 7 retries = 8
const REQUEST_TIMEOUT_MS = 10_000
const MAX_RESPONSE_BODY_BYTES = 4096
/** Rows claimed per cycle when the caller passes nothing: the cron. */
const DEFAULT_BATCH_SIZE = 50
/**
* Wall-clock ceiling on one cycle's serial attempt loop. 50 rows at a 10 s
* receiver timeout is 500 s of worst case, which no serverless invocation
* survives, so bound the cycle explicitly instead of letting the platform
* kill it mid-loop and leaving the remainder to the sweep.
*
* The cron route backs this with `export const maxDuration = 300`
* (app/api/webhooks/dispatch/cron/route.ts): the budget is only a real bound
* if the platform actually grants the invocation more wall time than the
* budget spends, and the release path below only runs if the loop is still
* alive when the budget expires.
*/
const CYCLE_BUDGET_MS = 120_000
/** Slack over the cycle bound: claim round trip, re-stamp, terminal write, clock skew. */
const STUCK_RECOVERY_SLACK_MS = 30_000
/**
* How long an in_flight row may legitimately sit before the sweep may treat
* it as abandoned: 160 s. Derived, not guessed (#1257), and deliberately
* independent of batch size.
*
* A cycle attempts its rows serially at up to REQUEST_TIMEOUT_MS each, but
* the loop is cut off by CYCLE_BUDGET_MS regardless of how many rows were
* claimed, so the BUDGET, not the row count, is what bounds a cycle's life:
* budget + one in-flight request + slack. A 5-row kick and a 50-row cron
* therefore use the same window, which is what the tenant-global sweep needs
* (neither caller may re-arm a row the other still owns). Anything
* batch-derived would let the kick pick a window narrower than the cron's
* live cycle occupies, which is the shape #1257 was reported against.
*
* A plain constant rather than a function of batchSize on purpose: the
* previous shape took a batchSize it could not act on (the clamp swallowed
* every value) and documented a floor that never fired. If CYCLE_BUDGET_MS
* changes this moves with it, and the cron's maxDuration must stay
* comfortably above it.
*/
const STUCK_IN_FLIGHT_AFTER_MS = CYCLE_BUDGET_MS + REQUEST_TIMEOUT_MS + STUCK_RECOVERY_SLACK_MS
interface DueDelivery {
id: string
webhook_id: string
@@ -68,6 +110,14 @@ export interface DispatchSummary {
delivered: number
failed: number
dead: number
/** Claimed but no longer ours by the time the attempt was due to start. */
skipped: number
/** Claimed but handed back unattempted because the cycle budget ran out. */
released: number
/** Rows this tick's sweep pulled out of an abandoned in_flight state. */
recovered: number
/** Subset of `recovered` the sweep took terminal (attempts exhausted). */
recoveredDead: number
}
/**
@@ -85,21 +135,41 @@ export async function dispatchDueDeliveries(args: {
/** Override for tests; injected pinned-fetch implementation. */
pinnedFetchImpl?: typeof pinnedHttpsFetch
}): Promise<DispatchSummary> {
const batchSize = args.batchSize ?? 50
const batchSize = args.batchSize ?? DEFAULT_BATCH_SIZE
const now = args.now ?? new Date()
const pinnedFetchImpl = args.pinnedFetchImpl ?? pinnedHttpsFetch
const summary: DispatchSummary = { picked: 0, delivered: 0, failed: 0, dead: 0 }
const summary: DispatchSummary = {
picked: 0,
delivered: 0,
failed: 0,
dead: 0,
skipped: 0,
released: 0,
recovered: 0,
recoveredDead: 0,
}
// Recover stuck in_flight rows: a previous tick that was killed mid-flight
// (Vercel function timeout, hard crash, manual termination) leaves rows
// marked in_flight forever otherwise. Sweep them back to 'failed' so the
// retry loop picks them up at next_attempt_at.
// retry loop picks them up at next_attempt_at, on the same backoff schedule
// a normal failed attempt gets.
//
// Threshold = 2× REQUEST_TIMEOUT_MS. A live attempt takes at most
// REQUEST_TIMEOUT_MS plus the body read; doubling that gives an
// unambiguous "this is stuck, not in-flight" boundary.
await recoverStuckInFlight(args.supabase, now)
// The window is STUCK_IN_FLIGHT_AFTER_MS: the bounded cycle
// (CYCLE_BUDGET_MS, backed by the route's maxDuration) plus one receiver
// timeout plus slack. It is deliberately WIDER than any live cycle can be,
// so a row still queued behind an earlier cycle's serial loop is never
// re-armed under it (#1257). The old fixed 2x REQUEST_TIMEOUT_MS was
// narrower than a single cycle, which made every cycle recover the rows
// the previous cycle was still working through.
const recovery = await recoverStuckInFlight(args.supabase, now)
summary.recovered = recovery.recovered
summary.recoveredDead = recovery.dead
// Real clock, not the injectable `now`: the budget bounds this invocation's
// wall time, so a fixed test `now` must not be able to trip it.
const cycleStartedAt = Date.now()
const due = await claimDueDeliveries(args.supabase, batchSize, now)
summary.picked = due.length
@@ -109,7 +179,24 @@ export async function dispatchDueDeliveries(args: {
const webhookIds = Array.from(new Set(due.map((d) => d.webhook_id)))
const webhookMap = await loadWebhooksByIds(args.supabase, webhookIds)
for (const delivery of due) {
for (const [index, delivery] of due.entries()) {
// Out of budget: hand back everything we will not reach rather than
// letting the platform kill the invocation mid-loop and leaving the
// remainder stranded in in_flight until a sweep re-arms it.
if (Date.now() - cycleStartedAt >= CYCLE_BUDGET_MS - REQUEST_TIMEOUT_MS) {
const remaining = due.slice(index)
await releaseUnattempted(
args.supabase,
remaining.map((d) => ({ id: d.id, attempts: d.attempts })),
now,
)
summary.released = remaining.length
log.warn('cycle budget exhausted: unattempted claims released', {
count: remaining.length,
})
break
}
const webhook = webhookMap.get(delivery.webhook_id)
if (!webhook) {
// The webhook was deleted between enqueue and dispatch. Mark dead;
@@ -137,6 +224,19 @@ export async function dispatchDueDeliveries(args: {
continue
}
// Re-stamp updated_at immediately before this row's own attempt, so the
// row's in_flight age measures the attempt rather than the claim, and use
// the same write as an ownership check (#1257).
if (!(await touchInFlight(args.supabase, delivery.id))) {
log.info('delivery skipped: no longer in_flight', {
deliveryId: delivery.id,
webhookId: webhook.id,
companyId: delivery.company_id,
})
summary.skipped++
continue
}
const outcome = await attemptDelivery({
delivery,
webhook,
@@ -193,37 +293,64 @@ export async function dispatchDueDeliveries(args: {
// ──────────────────────────────────────────────────────────────────────
/**
* Mark in_flight rows whose updated_at is older than the stuck-threshold
* back to 'failed' with next_attempt_at = now so they re-enter the
* dispatch queue. Best-effort: a write failure here is logged but
* Sweep in_flight rows whose updated_at is older than the stuck-threshold
* back into the retry queue, charging an attempt for the stall so
* MAX_ATTEMPTS stays a real cap. Best-effort: a failure here is logged but
* doesn't block the rest of the cycle.
*
* The predicate now lives in SQL (recover_stuck_webhook_deliveries,
* migration 20260730123000) rather than in a PostgREST chain, because
* PostgREST can express neither `attempts = attempts + 1` nor the
* conditional flip to 'dead' at the cap, and a read-then-write loop would
* reopen a TOCTOU against enforce_webhook_delivery_immutability. MAX_ATTEMPTS
* and RETRY_BACKOFF_SECONDS are passed in so the cap AND the schedule stay
* single-sourced here in TS.
*
* A swept row is re-armed on the SAME backoff the normal failure path uses
* (markFailedForRetry), not immediately: a row that keeps getting stranded
* (deploy, instance recycle, a cycle that outlives its invocation) would
* otherwise be re-claimed on the very next per-minute tick, and could burn
* all MAX_ATTEMPTS in under half an hour and land in the terminal, immutable
* 'dead' state without its receiver ever having been contacted. The ~87 h
* retry schedule is the delivery guarantee; the sweep must not shorten it.
*
* Under READ COMMITTED (Postgres default), UPDATE re-evaluates the WHERE
* clause against each row's current value when it acquires the row lock.
* The function keeps `status = 'in_flight'` in the outer UPDATE's WHERE for
* exactly that reason: a row that raced from 'in_flight' to
* 'delivered'/'dead' between scan and lock fails re-evaluation and is
* skipped entirely, so the immutability trigger never fires and a mid-flight
* terminal flip cannot abort the sweep.
*/
async function recoverStuckInFlight(supabase: SupabaseClient, now: Date): Promise<void> {
const stuckBefore = new Date(now.getTime() - 2 * REQUEST_TIMEOUT_MS)
// Under READ COMMITTED (Postgres default), UPDATE re-evaluates the WHERE
// clause against each row's current value when it acquires the row lock.
// A row that raced from 'in_flight' to 'delivered'/'dead' between scan
// and lock will fail status='in_flight' on re-evaluation and be skipped
// entirely: the immutability trigger never fires, so a mid-flight
// terminal flip cannot abort the bulk update.
const { data, error } = await supabase
.from('webhook_deliveries')
.update({
status: 'failed',
next_attempt_at: now.toISOString(),
error: 'recovered_from_in_flight_timeout',
})
.eq('status', 'in_flight')
.lt('updated_at', stuckBefore.toISOString())
.select('id')
async function recoverStuckInFlight(
supabase: SupabaseClient,
now: Date,
): Promise<{ recovered: number; dead: number }> {
const stuckBefore = new Date(now.getTime() - STUCK_IN_FLIGHT_AFTER_MS)
const { data, error } = await supabase.rpc('recover_stuck_webhook_deliveries', {
p_stuck_before: stuckBefore.toISOString(),
p_max_attempts: MAX_ATTEMPTS,
p_backoff: [...RETRY_BACKOFF_SECONDS],
p_now: now.toISOString(),
})
if (error) {
log.warn('stuck in_flight recovery failed', { code: error.code })
return
return { recovered: 0, dead: 0 }
}
if (data && data.length > 0) {
log.warn('recovered stuck in_flight rows', { count: data.length })
const rows = (data ?? []) as Array<{ id: string; status: string; attempts: number }>
const dead = rows.filter((r) => r.status === 'dead').length
if (rows.length > 0) {
log.warn('recovered stuck in_flight rows', {
count: rows.length,
dead,
retrying: rows.length - dead,
})
}
// Counts are returned as well as logged: a tick that takes deliveries
// terminal is the event worth alerting on, and it must be visible in the
// cron's own summary line rather than only in a helper-level warn.
return { recovered: rows.length, dead }
}
async function claimDueDeliveries(
@@ -321,6 +448,91 @@ async function markFailedForRetry(
if (error) log.warn('mark failed-for-retry update failed', { id, code: error.code })
}
/**
* Re-stamp a claimed row's updated_at right before its own attempt starts,
* and report whether the row is still ours.
*
* `status` is written back verbatim: Postgres runs the UPDATE regardless of
* whether any value changed, so the table's BEFORE UPDATE
* update_updated_at_column trigger (migration 20260515200000) re-stamps
* updated_at. That is what makes a row's in_flight age measure its attempt
* instead of the moment the whole batch was claimed, which is the property
* the stuck sweep reads.
*
* The `.eq('status', 'in_flight')` filter keeps the write off terminal rows,
* so enforce_webhook_delivery_immutability never fires. A zero-row result
* means the delivery is no longer ours (another cycle recovered and re-claimed
* it, or it already reached a terminal state): skip it rather than POSTing a
* duplicate whose terminal write would lose the race anyway.
*/
async function touchInFlight(supabase: SupabaseClient, id: string): Promise<boolean> {
const { data, error } = await supabase
.from('webhook_deliveries')
.update({ status: 'in_flight' })
.eq('id', id)
.eq('status', 'in_flight')
.select('id')
if (error) {
// Infrastructure hiccup, not lost ownership: proceed. Worst case the row
// looks older than it is and a later sweep re-arms it, which is exactly
// the pre-existing behaviour.
log.warn('in_flight touch failed', { id, code: error.code })
return true
}
return (data?.length ?? 0) > 0
}
/**
* Hand back rows this cycle claimed but will not attempt, so they are
* re-claimable immediately instead of waiting out the stuck window.
*
* Deliberately does NOT bump attempts (these rows were never attempted) and
* deliberately does NOT write `error` (a release is not a failure; overwriting
* would destroy the previous attempt's diagnostic).
*
* The status written back is the row's PRE-CLAIM status, reconstructed from
* `attempts` rather than guessed: claim_due_webhook_deliveries accepts
* status IN ('pending','failed'), and every path that writes 'failed' also
* writes attempts >= 1 (markFailedForRetry, the recovery sweep), while rows
* enter the table as 'pending' with attempts = 0. So attempts = 0 means the
* row was 'pending' and must go back to 'pending'. This matters because
* webhook_deliveries is behandlingshistorik (BFNAR 2013:2 kap 8 §) exposed on
* GET /webhooks/{id}/deliveries: a brand-new delivery that was claimed and
* handed back without a single POST must not read as 'failed' to the
* customer. Restoring the status verbatim would need the claim RPC to return
* it, which it does not; the attempts inference gets the same answer without
* changing a shipped function.
*/
async function releaseUnattempted(
supabase: SupabaseClient,
rows: Array<{ id: string; attempts: number }>,
now: Date,
): Promise<void> {
if (rows.length === 0) return
const byPriorStatus: Array<{ status: 'pending' | 'failed'; ids: string[] }> = [
{ status: 'pending', ids: rows.filter((r) => r.attempts === 0).map((r) => r.id) },
{ status: 'failed', ids: rows.filter((r) => r.attempts > 0).map((r) => r.id) },
]
for (const group of byPriorStatus) {
if (group.ids.length === 0) continue
const { error } = await supabase
.from('webhook_deliveries')
.update({ status: group.status, next_attempt_at: now.toISOString() })
.in('id', group.ids)
.eq('status', 'in_flight')
if (error) {
log.warn('release of unattempted claims failed', {
count: group.ids.length,
status: group.status,
code: error.code,
})
}
}
}
async function markDead(
supabase: SupabaseClient,
id: string,
@@ -651,4 +863,8 @@ export const __TESTING__ = {
MAX_ATTEMPTS,
REQUEST_TIMEOUT_MS,
MAX_RESPONSE_BODY_BYTES,
DEFAULT_BATCH_SIZE,
CYCLE_BUDGET_MS,
STUCK_RECOVERY_SLACK_MS,
STUCK_IN_FLIGHT_AFTER_MS,
}