From 2d403f7fb2b1fed9030375d691a055e3a9d42391 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Wed, 2 Sep 2026 15:32:35 +0200 Subject: [PATCH] fix(email): wire the settings status card, and cap-aware truncation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review round 4 on #1273. Settings → Status rendered a green check for the email processor unconditionally, against an API field that was itself the literal 'active'. Both ends were lying and only one of them got fixed: adminSystem started reporting the real state in an earlier commit, but StatusTab never read it, so the second place an admin looks to find out why mail is not arriving still said everything was fine. It now shows stopped and degraded, with the reason. The truncation flag missed the case it most needed to cover. The loop broke on the 200-row report cap before the flag could be set, so 201+ overdue rows came back as exactly 200 with scanTruncated false -- a partial report presented as complete. It is now set whenever rows were left unexamined. The grace-window comment claimed the processor clears ~6000 rows inside the window. It clears on the order of 100: ten rows a pass, one pass a minute. The comment now says so, and says why the processor's own state is reported above the list rather than inferred from it -- "running, last pass sent 10" next to a backlog reads very differently from "not running" next to the same backlog. One round-4 finding is NOT fixed, deliberately, and is written up at the retry route. Clearing scheduled_at leaves created_at at the original enqueue time, so a retried old row appears in the waiting list immediately, looking overdue, until the processor sends it. Restarting that clock needs a timestamp written there and no shape works: a Date matches how queueEmail writes the column and how processEmailQueue compares it, but jest's sandbox Dates store as "[object Object]" (CLAUDE.md) so it cannot be tested; an ISO string tests fine but stores as TEXT, which SQLite then orders above the numeric bound in the processor's own pickup query, leaving the row unsendable. A requeued_at column would settle it. Cosmetic either way, and not worth risking a stuck row. 1 more test, failing before this commit. --- .../adminSystemHealthWaitingEmails.test.js | 23 ++++++++++++ backend/src/routes/adminSystemHealth.js | 35 ++++++++++++++++--- .../src/features/settings/tabs/StatusTab.tsx | 23 ++++++++++-- frontend/src/i18n/locales/de.json | 4 ++- frontend/src/i18n/locales/en.json | 4 ++- frontend/src/services/settings.service.ts | 4 ++- 6 files changed, 83 insertions(+), 10 deletions(-) diff --git a/backend/__tests__/routes/adminSystemHealthWaitingEmails.test.js b/backend/__tests__/routes/adminSystemHealthWaitingEmails.test.js index 9e23d16d..fba76f2c 100644 --- a/backend/__tests__/routes/adminSystemHealthWaitingEmails.test.js +++ b/backend/__tests__/routes/adminSystemHealthWaitingEmails.test.js @@ -300,6 +300,29 @@ describe('GET /admin/system-health/failures — waiting emails (#1262)', () => { expect(body.counts.pendingScanned).toBe(0); }); + it('calls a report capped at the row limit truncated, not complete', async () => { + // Codex review round 4. The loop broke on the report cap before the + // truncation flag could be set, so 201+ overdue rows came back as exactly + // 200 with scanTruncated false — a partial report presented as the whole. + const rows = []; + for (let i = 0; i < 260; i += 1) { + rows.push({ + recipient_email: `overdue${i}@example.com`, + email_type: 'gallery_created', + email_data: '{}', + status: 'pending', + retry_count: 0, + created_at: ago(90 * MINUTE), + scheduled_at: ago(90 * MINUTE), + }); + } + await db.batchInsert('email_queue', rows, 100); + + const body = await failures(); + expect(body.counts.waitingEmails).toBe(200); + expect(body.scanTruncated).toBe(true); + }); + it('reports what the queue processor last did', async () => { const body = await failures(); // Never started in this process — which is the condition that makes a diff --git a/backend/src/routes/adminSystemHealth.js b/backend/src/routes/adminSystemHealth.js index c938f6c5..cf610b43 100644 --- a/backend/src/routes/adminSystemHealth.js +++ b/backend/src/routes/adminSystemHealth.js @@ -89,9 +89,14 @@ router.get( /** * How long an email may sit due-but-unsent before it counts as waiting rather - * than merely in flight. The processor wakes every 60s and takes 10 rows a - * pass, so a genuine backlog of ~6000 clears inside this window — anything - * still here has not been worked. + * than merely in flight. + * + * The processor wakes every 60s and takes 10 rows a pass, so it clears on the + * order of 100 rows inside this window — not thousands. A burst larger than + * that will therefore show up here for a while even though nothing is wrong, + * which is why the processor's own state is reported above this list rather + * than inferred from it: "running, last pass sent 10" next to a backlog reads + * very differently from "not running" next to the same backlog. */ const WAITING_EMAIL_GRACE_MS = 10 * 60 * 1000; @@ -204,12 +209,21 @@ router.get( .select('id', 'recipient_email', 'email_type', 'status', 'retry_count', 'error_message', 'created_at', 'scheduled_at'); if (page.length === 0) break; - scanned += page.length; + let examined = 0; for (const row of page) { + examined += 1; if (isWaiting(row)) waitingEmails.push(row); if (waitingEmails.length >= WAITING_REPORT_LIMIT) break; } - if (waitingEmails.length >= WAITING_REPORT_LIMIT || page.length < WAITING_PAGE_SIZE) break; + scanned += examined; + if (waitingEmails.length >= WAITING_REPORT_LIMIT) { + // Stopped because the response is full, not because the queue is. Any + // row left unexamined -- in this page or in pages after it -- may also + // be waiting, so the count is a floor and the report is partial. + scanTruncated = examined < page.length || page.length === WAITING_PAGE_SIZE; + break; + } + if (page.length < WAITING_PAGE_SIZE) break; // Ran out of budget with rows still unread. A queue this size whose head // is all future-scheduled could be hiding a due row past the cap, so the // empty result below is "not found yet", not "none" — and the UI must @@ -247,6 +261,17 @@ router.get( * pending with retries and schedule clear, so this endpoint would rewrite them * to the state they are in and change nothing. What a waiting row needs is the * processor fixed, which the panel above it says. + * + * KNOWN, deliberate: clearing scheduled_at leaves created_at at the original + * enqueue time, so a retried old row shows up in the waiting list right away + * looking overdue, until the processor sends it on its next tick. Restarting + * that clock needs a timestamp written here, and there is no shape that works: + * a Date matches how queueEmail writes the column and how processEmailQueue + * compares it, but jest's sandbox Dates store as "[object Object]" (CLAUDE.md) + * so it cannot be tested; an ISO string tests fine but stores as TEXT, and + * SQLite then orders it above the numeric bound in the processor's own pickup + * query, leaving the row unsendable. A requeued_at column would settle it. + * Cosmetic either way, and not worth risking a stuck row for. */ router.post( '/failures/email/:id/retry', diff --git a/frontend/src/features/settings/tabs/StatusTab.tsx b/frontend/src/features/settings/tabs/StatusTab.tsx index dbfae259..75be8252 100644 --- a/frontend/src/features/settings/tabs/StatusTab.tsx +++ b/frontend/src/features/settings/tabs/StatusTab.tsx @@ -10,6 +10,7 @@ import { Ruler, CalendarClock, RotateCw, + AlertTriangle, } from 'lucide-react'; import { Button, Card, Input } from '../../../components/common'; import { useTranslation } from 'react-i18next'; @@ -585,12 +586,30 @@ export const StatusTab: React.FC = ({

{t('settings.systemStatus.expirationCheckerDesc')}

+ {/* #1262 — this card used to render a green check unconditionally, + against an API field that was itself the literal 'active'. Both + ends now tell the truth: a stopped or bailing processor is the + reason queued mail never arrives, and this is one of the two + places an admin looks to find that out. */}

{t('settings.systemStatus.emailProcessor')}

- + {systemStatus?.services?.emailProcessor?.status === 'active' ? ( + + ) : ( + + )}
-

{t('settings.systemStatus.emailProcessorDesc')}

+

+ {systemStatus?.services?.emailProcessor?.status === 'stopped' + ? t('settings.systemStatus.emailProcessorStopped', + 'Not running — queued emails are written but nothing sends them.') + : systemStatus?.services?.emailProcessor?.status === 'degraded' + ? t('settings.systemStatus.emailProcessorDegraded', + 'Running, but the last pass could not send: {{error}}', + { error: systemStatus?.services?.emailProcessor?.lastError }) + : t('settings.systemStatus.emailProcessorDesc')} +

diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index c26d3c40..8e88b6a4 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -1968,7 +1968,9 @@ "pending": "Ausstehend", "sent": "Gesendet", "failed": "Fehlgeschlagen", - "lastUpdate": "Letzte Aktualisierung" + "lastUpdate": "Letzte Aktualisierung", + "emailProcessorStopped": "Läuft nicht — eingereihte E-Mails werden geschrieben, aber niemand versendet sie.", + "emailProcessorDegraded": "Läuft, aber der letzte Durchlauf konnte nicht senden: {{error}}" }, "photoDimensions": { "title": "Foto-Abmessungen", diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index 7e302fc9..aa7c8ce4 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -1331,7 +1331,9 @@ "pending": "Pending", "sent": "Sent", "failed": "Failed", - "lastUpdate": "Last update" + "lastUpdate": "Last update", + "emailProcessorStopped": "Not running — queued emails are written but nothing sends them.", + "emailProcessorDegraded": "Running, but the last pass could not send: {{error}}" }, "photoDimensions": { "title": "Photo Dimensions", diff --git a/frontend/src/services/settings.service.ts b/frontend/src/services/settings.service.ts index f7de882c..9d4b8c18 100644 --- a/frontend/src/services/settings.service.ts +++ b/frontend/src/services/settings.service.ts @@ -164,7 +164,9 @@ export interface SystemStatus { services: { fileWatcher: { status: string }; expirationChecker: { status: string }; - emailProcessor: { status: string }; + // 'active' | 'degraded' | 'stopped' (#1262). Was a hardcoded 'active' + // until the processor started reporting what it actually did. + emailProcessor: { status: string; lastRunAt?: string | null; lastError?: string | null }; }; timestamp: string; }