fix(bot): bound outbox delivery retries, treat 50278 as permanent - #244
Conversation
An applicant who leaves the guild makes their queued DM undeliverable
forever. Discord answers that with 50278 ("no mutual guilds"), but only
50007 was treated as permanent, so those rows were never retired: the
poller picked them up every 15 seconds and failed again, indefinitely.
Three users were in that state in production.
The retry noise is the smaller half. The poller selects `take: 25` ordered
by createdAt ascending, so permanently stuck rows are the oldest and hold
their slots for good. At 25 of them the outbox stops entirely, and not just
for DMs: review embeds and the guild activity log share the table.
Two changes, one for this bug and one for its class:
- apps/bot/src/delivery-policy.ts collects the permanent DM codes (now
50007 and 50278), the attempt ceiling and the backoff, free of discord.js
and Prisma so it can be tested. New permanent codes belong there.
- Notification gains attempts / lastError / nextAttemptAt, the same shape
WebhookDelivery already uses. A failed row backs off exponentially and is
retired after 8 attempts, roughly two hours, which outlasts an ordinary
Discord incident. readAt keeps meaning "retired", covering delivered and
never-deliverable alike; lastError is what tells them apart afterwards.
The poller's transient path now records why a row failed instead of only
logging it, which is the part that was missing when diagnosing this: the
evidence lived in a log that rotates, not in the row.
The migration folds next_attempt_at into the pending partial index and drops
its predecessor rather than keeping both over the same rows. Existing rows
default to a due timestamp, so they are picked up immediately after deploy,
and the three stuck ones retire on the first poll.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 3 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a testable delivery-policy module (terminal DM codes, capped exponential backoff, attempt exhaustion) and wires it into deliverPendingNotifications, replacing the fixed CANNOT_DM = 50007 check with isTerminalDmCode that also covers 50278. Reworks the deliverOne/deliverReview/deliverLog return type from boolean to an Outcome that carries a retry reason, and has the poller persist attempts, lastError, and nextAttemptAt per failure, retiring rows once exhausted. Filters the pending query by nextAttemptAt <= now so permanently-undeliverable rows no longer starve newer notifications.
Worth a look
- retryDelayMs returns NaN for NaN attempts despite non-NaN delay contract —
apps/bot/src/delivery-policy.ts:35· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- Delivered rows can be re-delivered because deliveredIds mark-read is separate from failure updates —
apps/bot/src/notifications.ts:371· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- Retry attempt updates can be lost across concurrent pollers —
apps/bot/src/notifications.ts:389· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 21 functions depend on the 18 functions this change touches.
Health — this change adds coupling hotspots:
- new:
createClient()— 1 callers, 9 callees - new:
deliverOne()— 1 callers, 7 callees - new:
deliverReview()— 1 callers, 7 callees
Verification — 21 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 21 function(s) in the blast radius were not formally verified this run
· 2 grounded finding(s) anchored inline below; 1 more finding(s) on lines outside this diff (see the check run).
1 participant
Found while reading the bot logs on the server (the undici follow-up from the last session).
The bug
notifications.tstreated only 50007 as a permanent "cannot DM" answer. Discord also returns 50278 when the applicant has left the guild, and that fell through to the transient branch:deliverOnereturnedfalse, the row was never marked read, and the poller retried it every 15 seconds forever. Three users were in that state, two of them appearing twice within 200 log lines.Why it is worse than log noise
Permanently stuck rows are the oldest rows, so they occupy their slots in every batch. At 25 of them the outbox stops completely, and the table is shared: status DMs, review embeds and the guild activity log all stop with it. The count can only grow, since every departing applicant adds one.
The fix
Two parts, deliberately: one for this bug, one so the next unknown permanent code cannot do the same thing.
apps/bot/src/delivery-policy.tsholds the permanent DM codes, the attempt ceiling and the backoff. No discord.js, no Prisma, so the policy is unit-tested (9 new tests, bot suite 4 to 13). New permanent codes go there.Notificationgainsattempts/lastError/nextAttemptAt, the shapeWebhookDeliveryalready uses in this repo. A failing row backs off exponentially (30s doubling, capped at 30 min) and is retired after 8 attempts, about two hours, which outlasts a normal Discord incident without letting a broken row live forever.readAtkeeps its meaning of "retired", which already covered both delivered and permanently undeliverable.lastErroris what distinguishes them after the fact, and it is the piece that was missing while diagnosing this: the evidence was in a log that rotates, not in the row.Migration
20260818003000_notification_retry_bounds, hand-written because the pending index is partial and Prisma cannot express that. It foldsnext_attempt_atinto the index and dropsnotifications_pending_idxrather than keeping two partial indexes over the same rows of a hot table.Existing rows default to a due timestamp, so nothing is delayed by the deploy, and the three currently stuck rows retire on the first poll afterwards. No cleanup needed.
Verification
lint,typecheck,testandbuildpass locally. The migration SQL is exercised by CI, which runsprisma migrate deployagainst its own Postgres 16 before the build.Worth watching after deploy:
giving up on notification <id> after 8 attemptsshould appear only for genuinely broken rows, andcan't DM user ... (50278)should now say "dropping".