Skip to content

fix: mark decision emails sent per recipient after delivery, not before dispatch - #4

Merged
balebbae merged 1 commit into
mainfrom
devin/1789057721-decision-email-mark-after-send
Sep 10, 2026
Merged

balebbae merged 1 commit into
mainfrom
devin/1789057721-decision-email-mark-after-send

Conversation

@devin-ai-integration

Copy link
Copy Markdown

Summary

Fixes audit finding M2: POST /superadmin/emails/decisions stamped decision_email_sent_at / announcement_email_sent_at for the whole batch before handing the send to a detached goroutine, then un-marked failures at the end. If the process died mid-run (Cloud Run instance reaped, deploy, OOM) every unsent recipient stayed marked as emailed, was excluded from every later "unsent only" run, and showed as sent in the stats — recoverable only via resend_all, which double-sends the ones that did go out.

Ordering is now:

before: SetDecisionEmailSent(all, kind, true) → 200 → go send… → SetDecisionEmailSent(failed, kind, false)
after:  200 → go send… → per recipient: send ok? → SetDecisionEmailSent([id], kind, true)
  • dispatchDecisionEmails marks each recipient individually, right after its send succeeds (10s bounded ctx). Failed sends are simply never marked, so the "clear marker on failure" pass is gone. A crash now leaves the unsent remainder unmarked → retried next run. The only remaining bad window is sent but killed before the 1-row UPDATE → at worst one duplicate email, which is the safe direction.
  • The pre-mark was also the double-click / concurrent-request guard, so that role moves to application.decisionEmailInFlight (atomic.Bool): a second request while a run is sending gets 409 decision emails are already being sent. The flag is taken before the recipient query (and released on every early return) so a second request can't snapshot recipients the current run is still working through. Guard is per-process; with >1 Cloud Run instance two admins clicking within the same run could still overlap, same as before.
  • application.backgroundJobs (sync.WaitGroup) tracks the dispatch goroutine, and run() now drains it after server.Shutdown within the existing 5s shutdown ctx (drainBackgroundJobs), so a SIGTERM lets in-flight sends finish writing their markers.
  • Frontend needs no change: SendEmailsDialog already routes non-200 through errorAlert. Side effect worth knowing: the stats it refreshes immediately after the 200 now lag until sends actually complete (previously they jumped to "sent" instantly).
  • Swagger regenerated (task gen-docs) for the 409 + description.

Tests updated for the new contract: per-recipient marks only after a successful send, failed recipients untouched, marker-write failure doesn't stop the batch, 409 on overlapping runs (first send held open via a channel so the overlap is real), and the lock is released on early-return paths.

go build/vet, gofmt, go test -race ./... pass.

Link to Devin session: https://app.devin.ai/sessions/8d5427d328ee4f57acf27d50df7f2af8
Open in Devin Desktop: https://app.devin.ai/desktop/session/8d5427d328ee4f57acf27d50df7f2af8?variant=devin
Requested by: @balebbae

@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@balebbae
balebbae merged commit 660fbbf into main Sep 10, 2026
3 checks passed
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.

1 participant