fix(email): show a queue nobody is working instead of reporting all-clear
Closes #1262. "Gallery email queued" reads as a delivery confirmation, and System Health agreed with it: "No stuck or failed emails -- all clear", while not one email had gone out. Both statements were true and neither was the one the admin needed. Queueing writes an email_queue row at status='pending', retry_count 0 -- nothing more. /failures matched only status='failed' or pending-with-retry_count>=3, so it matched none of those rows, and there are two ordinary ways they never leave that state: - startEmailQueueProcessor() was never reached, so nothing polls the queue. - Every pass returns early. processEmailQueue bails when the transporter will not initialise, before it touches a single row, so retry_count stays 0 and no error_message is ever written. A working SMTP test button does not contradict this: that path builds its own transport. adminSystem.js made it worse by reporting `emailProcessor: { status: 'active' }` as a literal, so the one place that named the worker always said it was fine. - emailProcessor records what each pass did -- started, lastRunAt, lastResult, lastError -- and exports getQueueProcessorStatus(). The transporter bail and the queue-query failure, the two silent early returns, both write lastError. - /failures gains `waitingEmails`: pending, under the retry cap, past any scheduled_at, and queued more than 10 minutes ago. The predicate mirrors the processor's own pickup query, so a row listed there is one it should already have taken; rows over the cap stay in `stuckEmails` and are not counted twice. A future scheduled_at is left alone -- split-payment invoices and the business-hours floor park rows deliberately. - System Health leads with the processor's state (running / stopped / degraded) and lists waiting emails in their own table. The all-clear now needs both buckets empty. - adminSystem reports the real processor state instead of the literal. - The two "queued" toasts say the queue processor is what sends it and where to look if it doesn't arrive. 8 route tests, all 8 failing before the change.
This commit is contained in:
Paul Nothaft
@@ -0,0 +1,158 @@
|
||||
/**
|
||||
* System Health must not report "all clear" over a queue nobody is working (#1262).
|
||||
*
|
||||
* "Gallery email queued" reads as a delivery confirmation, and the two ways the
|
||||
* queue silently stops — the processor never started, or every pass returns
|
||||
* early because the transport will not initialise — leave every row at
|
||||
* status='pending' with retry_count 0. The old /failures query matched only
|
||||
* status='failed' or pending-with-retry_count>=3, so it matched none of them
|
||||
* and the page said everything was fine while nothing had been sent.
|
||||
*/
|
||||
|
||||
const path = require('path');
|
||||
const fs = require('fs');
|
||||
const os = require('os');
|
||||
|
||||
process.env.NODE_ENV = 'test';
|
||||
process.env.TEST_DATABASE_PATH = path.join(
|
||||
fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-mailhealth-')), 'db.sqlite',
|
||||
);
|
||||
process.env.JWT_SECRET = process.env.JWT_SECRET || 'mailhealth-test-secret';
|
||||
|
||||
const request = require('supertest');
|
||||
const bcrypt = require('bcrypt');
|
||||
const jwt = require('jsonwebtoken');
|
||||
|
||||
const { bootCrmDb, seedMinimal, buildRouteApp } = require('../integration/helpers/crmDb');
|
||||
|
||||
const MINUTE = 60 * 1000;
|
||||
const ago = (ms) => new Date(Date.now() - ms).toISOString();
|
||||
const ahead = (ms) => new Date(Date.now() + ms).toISOString();
|
||||
|
||||
describe('GET /admin/system-health/failures — waiting emails (#1262)', () => {
|
||||
let db; let cleanup; let app; let token;
|
||||
|
||||
const queue = (row) => db('email_queue').insert({
|
||||
recipient_email: '[email protected]',
|
||||
email_type: 'gallery_created',
|
||||
email_data: '{}',
|
||||
status: 'pending',
|
||||
retry_count: 0,
|
||||
created_at: ago(60 * MINUTE),
|
||||
...row,
|
||||
});
|
||||
|
||||
const failures = async () => {
|
||||
const res = await request(app)
|
||||
.get('/admin/system-health/failures')
|
||||
.set('Authorization', `Bearer ${token}`);
|
||||
expect(res.status).toBe(200);
|
||||
return res.body.data || res.body;
|
||||
};
|
||||
|
||||
const typesOf = (rows) => rows.map((r) => r.emailType).sort();
|
||||
|
||||
beforeAll(async () => {
|
||||
({ db, cleanup } = await bootCrmDb());
|
||||
await seedMinimal(db);
|
||||
|
||||
const role = await db('roles').where({ name: 'super_admin' }).first();
|
||||
const inserted = await db('admin_users').insert({
|
||||
username: 'mailhealth-admin',
|
||||
email: '[email protected]',
|
||||
password_hash: await bcrypt.hash('Passw0rd!', 4),
|
||||
role_id: role.id,
|
||||
is_active: 1,
|
||||
created_at: new Date().toISOString(),
|
||||
updated_at: new Date().toISOString(),
|
||||
}).returning('id');
|
||||
const adminId = inserted[0]?.id ?? inserted[0];
|
||||
token = jwt.sign(
|
||||
{ id: adminId, username: 'mailhealth-admin', type: 'admin', role: 'super_admin', loginTime: Date.now() },
|
||||
process.env.JWT_SECRET,
|
||||
{ expiresIn: '1h', issuer: 'picpeak-auth' },
|
||||
);
|
||||
|
||||
app = buildRouteApp('/admin/system-health', require('../../src/routes/adminSystemHealth'));
|
||||
});
|
||||
|
||||
afterAll(async () => { await cleanup(); });
|
||||
afterEach(async () => { await db('email_queue').del(); });
|
||||
|
||||
it('reports a due pending email the processor never picked up', async () => {
|
||||
// Exactly the shape "Gallery email queued" leaves behind when the worker
|
||||
// is not running: pending, no retries, no error, no scheduled_at.
|
||||
await queue({ email_type: 'gallery_created' });
|
||||
|
||||
const body = await failures();
|
||||
expect(typesOf(body.waitingEmails)).toEqual(['gallery_created']);
|
||||
expect(body.counts.waitingEmails).toBe(1);
|
||||
// ...and it is NOT a failure, so the two buckets stay distinct.
|
||||
expect(body.stuckEmails).toEqual([]);
|
||||
});
|
||||
|
||||
it('leaves a freshly queued email alone — the processor wakes every 60s', async () => {
|
||||
await queue({ email_type: 'customer_invitation', created_at: ago(30 * 1000) });
|
||||
|
||||
const body = await failures();
|
||||
expect(body.waitingEmails).toEqual([]);
|
||||
expect(body.counts.waitingEmails).toBe(0);
|
||||
});
|
||||
|
||||
it('leaves an email scheduled for later alone', async () => {
|
||||
// Split-payment invoices and the business-hours floor both park rows in
|
||||
// the future on purpose. Not being sent yet is the point of those.
|
||||
await queue({ email_type: 'invoice_due', scheduled_at: ahead(3 * 24 * 60 * MINUTE) });
|
||||
|
||||
const body = await failures();
|
||||
expect(body.waitingEmails).toEqual([]);
|
||||
});
|
||||
|
||||
it('counts a past-due scheduled email once its moment has come', async () => {
|
||||
await queue({ email_type: 'invoice_due', scheduled_at: ago(30 * MINUTE) });
|
||||
|
||||
const body = await failures();
|
||||
expect(typesOf(body.waitingEmails)).toEqual(['invoice_due']);
|
||||
});
|
||||
|
||||
it('does not double-count a retry-exhausted email as waiting', async () => {
|
||||
// retry_count >= 3 is already the `stuckEmails` bucket; listing it in both
|
||||
// would inflate the badge and make the two tables disagree.
|
||||
await queue({ email_type: 'quote_sent', retry_count: 3, error_message: 'template missing' });
|
||||
|
||||
const body = await failures();
|
||||
expect(body.waitingEmails).toEqual([]);
|
||||
expect(typesOf(body.stuckEmails)).toEqual(['quote_sent']);
|
||||
});
|
||||
|
||||
it('ignores emails that were sent', async () => {
|
||||
await queue({ email_type: 'gallery_created', status: 'sent', sent_at: ago(20 * MINUTE) });
|
||||
|
||||
const body = await failures();
|
||||
expect(body.waitingEmails).toEqual([]);
|
||||
expect(body.stuckEmails).toEqual([]);
|
||||
});
|
||||
|
||||
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
|
||||
// pending row invisible, so the page has to be able to say it.
|
||||
expect(body.processor).toEqual(expect.objectContaining({ started: false }));
|
||||
expect(body.processor).toHaveProperty('lastRunAt');
|
||||
expect(body.processor).toHaveProperty('lastError');
|
||||
});
|
||||
|
||||
it('surfaces the transport failure that makes every pass a no-op', async () => {
|
||||
const { processEmailQueue, getQueueProcessorStatus } = require('../../src/services/emailProcessor');
|
||||
await queue({ email_type: 'gallery_created' });
|
||||
|
||||
// No SMTP configured, so initializeTransporter() yields nothing and the
|
||||
// pass returns early. Before #1262 that left no trace anywhere.
|
||||
await processEmailQueue();
|
||||
|
||||
expect(getQueueProcessorStatus().lastError).toMatch(/transporter could not be initialised/i);
|
||||
const body = await failures();
|
||||
expect(body.processor.lastError).toMatch(/transporter could not be initialised/i);
|
||||
expect(body.counts.waitingEmails).toBe(1);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user