Skip to content

Record announcement deliveries and keep bounce parsing linear - #105

Closed
theobong wants to merge 2 commits into
mainfrom
fix/backend-campaign-15-broadcast-sent-at
Closed

theobong wants to merge 2 commits into
mainfrom
fix/backend-campaign-15-broadcast-sent-at

Conversation

@theobong

@theobong theobong commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

What changed

Two confirmed production bugs on main, one commit each. Host announcements now record who they reached: before, recording the outcome of a batch failed whenever its first delivery had gone out, so counts stayed at zero and a later retry could send the same announcement again. And three regexes in bounce-mail parsing now run in linear time: before, one crafted email could block the API for tens of seconds.

Before you start

  • Where: staging (civfix.dev, the staging mobile build) after this merges to main
  • Sign in as: your own accounts, A (host of an upcoming event) and B (registered for it), with emailed codes
  • Based on main, not on the campaign train. The train picks it up when main flows into it.
  • Size: 280 counted lines, 262 of them tests. Campaign PR; the author approved PRs over the 400-line cap for this cleanup. It carries two unrelated fixes because the campaign has one spare PR for confirmed critical findings; each commit stands alone and can be cherry-picked into its own PR.

Verify

Announcements reach attendees once, and the host sees the count — [Web] [Mobile]

  1. As A open the event, click "Host dashboard", then "Make announcement"; write a short message and send it — Expect: "Announcement sent."
  2. As B check the notifications bell and the inbox — Expect: the announcement arrives once
  3. As A, back on "Host dashboard", find the message under "Announcements you sent" — Expect: within a minute it reads "… of … notified" with a number above 0
  4. Wait 15 minutes, then check B's notifications and inbox again — Expect: still exactly one copy

Regression

Opening an announcement — [Web] [Mobile]

  1. As B open the announcement from the notifications bell — Expect: "Sent … ago" and the message text
  2. As A open the same announcement from "Announcements you sent" — Expect: a "Sent to … · … notified" line with a number above 0

Findings addressed

  • Announcement outcomes (critical). The delivery pipeline passes each outcome's sent time as a JavaScript date. postgres.js types an array parameter by its first element, so a list that starts with a date is sent as a single timestamp, and Postgres rejects the cast to an array (error 42846). Every batch whose first delivery succeeded failed after the notifications and emails had already gone out. The deliveries stayed "sending", the counts never refreshed, and the stale-sending sweep re-queued the batch, which could re-claim and re-send those deliveries up to 3 times once they were 10 minutes old. Present since the host platform landed (2026-09-06). Reproduced locally on an in-process Postgres (PGlite) with the real repository code: main fails with 42846, and this branch records every outcome.

  • Bounce parsing stalls (critical, security). Mail handling runs in the API process. Three regexes in bounce detection were quadratic on long runs:

    • the address in a failed-recipients header or body line;
    • a "Final-Recipient:" label followed by newlines;
    • a message id made of unclosed brackets.

    Anyone who can send mail to the reply domain could stall every request. Measured on main: 6.3 s per field at 64 KiB, and 13.3 s for one crafted message through the real mail parser. After: under 2 ms each, and 4 ms at 512 KiB.

Decisions for the reviewer

  • The regex rewrites return exactly the old first match: a lookbehind that only rejects starts inside a run (the match from the run's start is further left and identical), the optional "rfc822;" folded into one whitespace run, and brackets searched only up to the last ">". A differential fuzz of 1,000,000 inputs per regex (over 570,000 matches each) found 0 differences in index, text or captured value.
  • Only the three bounce regexes on main are touched. A fourth quadratic regex in the HTML sanitizer's link cleanup exists only on the campaign train (added by Correctness fixes (backend campaign 5) #94) and is fixed there.

User-visible copy changes

None.

Tests changed

  • New host-broadcast-outcomes-binding unit test: the outcome update binds strings, never dates (fails on main).
  • New host-broadcast-outcomes-pg integration test: applies a batch whose first delivery was sent, and one where all were sent, against the real schema (runs in CI).
  • New inbound-bounce-linear-time unit test: the three stall shapes and one end-to-end message through the real parser, each under 500 ms (on main they take 6 to 13 s), plus first-match checks.

Verification

  • Every new unit test fails on main and passes here.
  • Typecheck and lint clean. The 21 inbound, bounce, broadcast, announcement and reminder unit files pass (385 tests).
  • The integration test was not run locally (it needs Docker); CI runs it.

Staging checks

  • https://api.civfix.dev/readyz answers ready (no migration).
  • The Verify steps above; a host announcement's count rises and stays after 15 minutes.

Not covered

  • Bounce handling with a real bounced email: it needs a crafted message sent to the staging reply domain. The unit tests cover the parsing, including through the real mail parser.
  • Announcements sent before this deploy stay stuck as "sending"; this PR does not repair old rows.

🤖 Generated with Claude Code

@greptile-apps

greptile-apps Bot commented Sep 23, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

Safe to merge; focused database and inbound-mail checks completed successfully with no outstanding issues.

What we checked:

  • Ran the pg-check harness and captured before/after state with the exact commands, working directory, exit codes, and observed database rows. T-Rex
  • Validated that the before/after logs show matching ordinary extraction contracts such as ab.c+tag@city.gov, ops@city.gov, and the first bracketed message ID, and noted runtimes for malformed inputs and the end-to-end mail-parser case, with the executable harness source uploaded alongside both runtime captures. T-Rex

Reviews (1) · Last reviewed commit: "bounce parsing regexes run in linear tim..."

@byteful byteful closed this Sep 24, 2026
@byteful
byteful deleted the fix/backend-campaign-15-broadcast-sent-at branch September 24, 2026 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants