send-admin-emails-via-a-background-job-with-live-status-reporting-462 #476

Merged

johnsturgeon merged 3 commits from send-admin-emails-via-a-background-job-with-live-status-reporting-462 into main

2026-09-06 17:23:35 +02:00

johnsturgeon commented

2026-09-06 17:16:20 +02:00

Owner

Send admin emails via a background job with live status
Enqueue one EmailQueue row per recipient and drain the batch off-request
with a FastAPI background task. The landing page polls a status endpoint
and flips a light per recipient as each send lands, replacing the inline
synchronous send. Idempotent (already-SENT rows are skipped) and scoped
per batch so concurrent sends never collide.

resolves #462

Send admin emails via a background job with live status Enqueue one EmailQueue row per recipient and drain the batch off-request with a FastAPI background task. The landing page polls a status endpoint and flips a light per recipient as each send lands, replacing the inline synchronous send. Idempotent (already-SENT rows are skipped) and scoped per batch so concurrent sends never collide. resolves #462

johnsturgeon added this to the Kickoff 2026 milestone

2026-09-06 17:16:20 +02:00

johnsturgeon added 3 commits

2026-09-06 17:16:20 +02:00

Step 1. add migration for new emailqueue table, and while we're at it c fd85ed629f

Send admin emails via a background job with live status 0cd4ba3b53

Enqueue one EmailQueue row per recipient and drain the batch off-request
with a FastAPI background task. The landing page polls a status endpoint
and flips a light per recipient as each send lands, replacing the inline
synchronous send. Idempotent (already-SENT rows are skipped) and scoped
per batch so concurrent sends never collide.

resolves #462

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Updated version

Claude Code Review / claude-review (pull_request) Successful in 1m51s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 42s

Details

Tests / pytest (pull_request) Successful in 28s

Details

987684f05e

forgejo-actions commented

2026-09-06 17:18:13 +02:00

Summary

This cleanly moves the admin email send off-request: it enqueues one EmailQueue row per recipient, drains them in a FastAPI background task, and the landing page polls a per-recipient status endpoint. The design is sound, the tests are thorough, and the unrelated team.data_source drop is verified dead (no references remain in app/). No blocking issues.

Worth fixing

A crash mid-drain strands rows unrecoverably (app/jobs/send_admin_email.py:13). drain_batch marks a row PROCESSING and commits before the send, but the only way a batch is ever drained is the in-process background.add_task fired once at request time. If the worker dies during a send (deploy, OOM, restart), that row stays PROCESSING and any still-PENDING rows stay PENDING forever — nothing re-triggers the batch. Re-invoking the drain wouldn't help either, since jobs_in_batch(..., EmailSendStatus.PENDING) filters out PROCESSING rows. The PR frames this as a "background job" and stresses idempotency, but idempotency only covers the already-SENT case. Since the codebase already runs APScheduler, a small startup/periodic sweep that re-queues stale PENDING/PROCESSING batches would close the durability gap. At minimum, worth documenting this as a known limitation in docs/TECH_DEBT.md.
done never flips true for a stranded batch (app/routers/v2.py:634). Consequence of the above: with a stuck PROCESSING row the status endpoint's done stays False, so the poller just runs out its MAX_TICKS and shows "Still sending — reload to refresh." That's a reasonable fallback, but the admin has no signal distinguishing "genuinely slow" from "permanently stuck."

Nits

Unrelated migration bundled in (alembic/versions/a390332fa722_drop_team_data_source.py). Dropping team.data_source is harmless and well-documented, but it's scope creep in a PR titled "send admin emails via a background job." Ordering it after the email-queue migration also couples the two revisions; a reviewer bisecting the email feature can't revert it independently.
Missing type hint (app/jobs/send_admin_email.py:10). async def drain_batch(batch_id): — the surrounding code annotates params (e.g. jobs_in_batch(session, batch_id: str, ...)). Add batch_id: str.
Prune coupled to the send path (app/routers/v2.py:605-606). Running a 30-day DELETE inside every send request's transaction works, but it ties table housekeeping to a user action; a scheduled prune would be a more natural home and keeps the request lean. Also note created_at is naive and this compares against a naive-UTC cutoff, which relies on the DB server clock being UTC — consistent with the existing TGFPModelBase convention, so only flagging for awareness.
Test gaps for the new edges (tests/test_send_admin_email_job.py). The drain tests are good, but nothing exercises the 30-day prune or a PROCESSING row left behind by a crash — the two behaviors most likely to surprise later. Worth a row for each.

## Summary This cleanly moves the admin email send off-request: it enqueues one `EmailQueue` row per recipient, drains them in a FastAPI background task, and the landing page polls a per-recipient status endpoint. The design is sound, the tests are thorough, and the unrelated `team.data_source` drop is verified dead (no references remain in `app/`). No blocking issues. ## Worth fixing - **A crash mid-drain strands rows unrecoverably** (`app/jobs/send_admin_email.py:13`). `drain_batch` marks a row `PROCESSING` and commits *before* the send, but the only way a batch is ever drained is the in-process `background.add_task` fired once at request time. If the worker dies during a send (deploy, OOM, restart), that row stays `PROCESSING` and any still-`PENDING` rows stay `PENDING` forever — nothing re-triggers the batch. Re-invoking the drain wouldn't help either, since `jobs_in_batch(..., EmailSendStatus.PENDING)` filters out `PROCESSING` rows. The PR frames this as a "background job" and stresses idempotency, but idempotency only covers the already-`SENT` case. Since the codebase already runs APScheduler, a small startup/periodic sweep that re-queues stale `PENDING`/`PROCESSING` batches would close the durability gap. At minimum, worth documenting this as a known limitation in `docs/TECH_DEBT.md`. - **`done` never flips true for a stranded batch** (`app/routers/v2.py:634`). Consequence of the above: with a stuck `PROCESSING` row the status endpoint's `done` stays `False`, so the poller just runs out its `MAX_TICKS` and shows "Still sending — reload to refresh." That's a reasonable fallback, but the admin has no signal distinguishing "genuinely slow" from "permanently stuck." ## Nits - **Unrelated migration bundled in** (`alembic/versions/a390332fa722_drop_team_data_source.py`). Dropping `team.data_source` is harmless and well-documented, but it's scope creep in a PR titled "send admin emails via a background job." Ordering it after the email-queue migration also couples the two revisions; a reviewer bisecting the email feature can't revert it independently. - **Missing type hint** (`app/jobs/send_admin_email.py:10`). `async def drain_batch(batch_id):` — the surrounding code annotates params (e.g. `jobs_in_batch(session, batch_id: str, ...)`). Add `batch_id: str`. - **Prune coupled to the send path** (`app/routers/v2.py:605-606`). Running a 30-day `DELETE` inside every send request's transaction works, but it ties table housekeeping to a user action; a scheduled prune would be a more natural home and keeps the request lean. Also note `created_at` is naive and this compares against a naive-UTC cutoff, which relies on the DB server clock being UTC — consistent with the existing `TGFPModelBase` convention, so only flagging for awareness. - **Test gaps for the new edges** (`tests/test_send_admin_email_job.py`). The drain tests are good, but nothing exercises the 30-day prune or a `PROCESSING` row left behind by a crash — the two behaviors most likely to surprise later. Worth a row for each.

johnsturgeon merged commit b9adb608ea into main

2026-09-06 17:23:35 +02:00

johnsturgeon deleted branch send-admin-emails-via-a-background-job-with-live-status-reporting-462

2026-09-06 17:23:35 +02:00

johnsturgeon referenced this pull request from a commit

2026-09-06 17:23:35 +02:00

Merge pull request 'send-admin-emails-via-a-background-job-with-live-status-reporting-462' (#476) from send-admin-emails-via-a-background-job-with-live-status-reporting-462 into main