fix(poller): stop infinite re-enqueue of unsendable notifications
In shadow/dry-run/kill-switch modes, when the target has no token/recipient/
chat, or when not in the canary cohort, the handlers returned without marking
the notification, so the poller (pendingFilter on notified:{$ne:true})
re-selected the same batch every cycle forever (observed: fcm:500 every 10s).
Add a service-owned per-channel marker processedBy.<channel>, set in every
terminal 'won't/can't send' branch, and exclude marked docs in pendingFilter.
Unlike the shared notified/emailNotified/telegramNotified flags, processedBy is
invisible to the old system, so it never suppresses a real send in prod shadow.
- types.ts: NotificationDoc.processedBy?: Partial<Record<Channel, Date>>
- poller.ts: pendingFilter excludes processedBy.<channel>
- handlers.ts: markProcessed() in no-token/no-recipient/no-chat, decideSend=false,
dead-token and telegram-blocked branches
- fake-repo: dotted-path support in matching and $set/$unset
- tests: regression test + processedBy assertions (86 passing)
This commit is contained in:
parent
3596f7abed
commit
6d1ba439a2
6 changed files with 115 additions and 15 deletions
|
|
@ -3,14 +3,51 @@
|
|||
* MongoDB 3.2, for which no mongod binary is downloadable/runnable on modern
|
||||
* dev/CI hosts (glibc/libssl). So the DB integration tests run against this
|
||||
* fake, which faithfully emulates the subset of behaviour the code relies on:
|
||||
* the query operators used ($ne/$in/$gte/$exists/equality), $set/$unset updates,
|
||||
* projection/limit, and — crucially — unique-index duplicate-key errors (11000)
|
||||
* that back the idempotency guarantee.
|
||||
* the query operators used ($ne/$in/$gte/$exists/equality), $set/$unset updates
|
||||
* (including dotted paths like `processedBy.fcm`), projection/limit, and —
|
||||
* crucially — unique-index duplicate-key errors (11000) that back the
|
||||
* idempotency guarantee.
|
||||
*/
|
||||
import type { Repos } from '../src/db.js';
|
||||
|
||||
type Doc = Record<string, any>;
|
||||
|
||||
/** Read a possibly-dotted path (`a.b.c`) from a doc, like MongoDB does. */
|
||||
function getPath(doc: Doc, key: string): any {
|
||||
if (!key.includes('.')) return doc[key];
|
||||
return key.split('.').reduce<any>((cur, part) => (cur == null ? undefined : cur[part]), doc);
|
||||
}
|
||||
|
||||
/** Set a possibly-dotted path, creating intermediate objects as MongoDB would. */
|
||||
function setPath(doc: Doc, key: string, value: any): void {
|
||||
if (!key.includes('.')) {
|
||||
doc[key] = value;
|
||||
return;
|
||||
}
|
||||
const parts = key.split('.');
|
||||
let cur = doc;
|
||||
for (const part of parts.slice(0, -1)) {
|
||||
if (cur[part] == null || typeof cur[part] !== 'object') cur[part] = {};
|
||||
cur = cur[part];
|
||||
}
|
||||
cur[parts[parts.length - 1]!] = value;
|
||||
}
|
||||
|
||||
/** Delete a possibly-dotted path. */
|
||||
function delPath(doc: Doc, key: string): void {
|
||||
if (!key.includes('.')) {
|
||||
delete doc[key];
|
||||
return;
|
||||
}
|
||||
const parts = key.split('.');
|
||||
let cur = doc;
|
||||
for (const part of parts.slice(0, -1)) {
|
||||
if (cur[part] == null) return;
|
||||
cur = cur[part];
|
||||
}
|
||||
delete cur[parts[parts.length - 1]!];
|
||||
}
|
||||
|
||||
function isObjectId(v: any): boolean {
|
||||
return v != null && typeof v === 'object' && typeof v.equals === 'function' && v._bsontype === 'ObjectID';
|
||||
}
|
||||
|
|
@ -64,12 +101,12 @@ function matchField(docVal: any, cond: any): boolean {
|
|||
}
|
||||
|
||||
function matches(doc: Doc, filter: Doc): boolean {
|
||||
return Object.entries(filter).every(([k, cond]) => matchField(doc[k], cond));
|
||||
return Object.entries(filter).every(([k, cond]) => matchField(getPath(doc, k), cond));
|
||||
}
|
||||
|
||||
function applyUpdate(doc: Doc, update: Doc): void {
|
||||
if (update.$set) Object.assign(doc, update.$set);
|
||||
if (update.$unset) for (const k of Object.keys(update.$unset)) delete doc[k];
|
||||
if (update.$set) for (const [k, v] of Object.entries(update.$set)) setPath(doc, k, v);
|
||||
if (update.$unset) for (const k of Object.keys(update.$unset)) delPath(doc, k);
|
||||
}
|
||||
|
||||
class FakeCollection {
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it } from 'vitest';
|
|||
import type { Repos } from '../src/db.js';
|
||||
import type { Config } from '../src/config.js';
|
||||
import { processEmailJob, processFcmJob, type Deps } from '../src/handlers.js';
|
||||
import { pendingFilter } from '../src/poller.js';
|
||||
import { aNotif, aUser, fakeFcm, fakeMailer, freshRepos, silentLog, testCfg } from './helpers.js';
|
||||
|
||||
let repos: Repos;
|
||||
|
|
@ -60,8 +61,31 @@ describe('FCM handler', () => {
|
|||
|
||||
expect(fcm.calls).toHaveLength(0);
|
||||
const after = await repos.notifications.findOne({ _id: notif._id });
|
||||
expect(after?.notified).toBeUndefined();
|
||||
expect(await repos.sends.countDocuments({})).toBe(0); // no reservation in dry-run
|
||||
expect(after?.notified).toBeUndefined(); // never touches the old system's flag
|
||||
expect(after?.processedBy?.fcm).toBeInstanceOf(Date); // but marks it processed…
|
||||
expect(await repos.sends.countDocuments({})).toBe(0); // …with no reservation in dry-run
|
||||
});
|
||||
|
||||
it('does not re-enqueue a doc already processed in shadow mode', async () => {
|
||||
// Regression for the infinite fcm:500/cycle re-enqueue: in shadow the handler
|
||||
// marks processedBy so the poller filter no longer selects the doc.
|
||||
const notif = aNotif();
|
||||
await repos.notifications.insertOne(notif);
|
||||
await repos.users.insertOne(aUser());
|
||||
const fcm = fakeFcm({ ok: true, messageId: 'x' });
|
||||
const shadow = { ...baseCfg, mode: 'shadow' as const };
|
||||
|
||||
// Before: the poller would select this pending mobile doc.
|
||||
expect(await repos.notifications.countDocuments(pendingFilter('fcm'))).toBe(1);
|
||||
|
||||
await processFcmJob(deps({ cfg: shadow, fcm }), notif._id.toHexString());
|
||||
|
||||
expect(fcm.calls).toHaveLength(0); // nothing sent in shadow
|
||||
const after = await repos.notifications.findOne({ _id: notif._id });
|
||||
expect(after?.notified).toBeUndefined(); // old system's flag untouched
|
||||
expect(after?.processedBy?.fcm).toBeInstanceOf(Date);
|
||||
// After: the poller no longer selects it -> no re-enqueue.
|
||||
expect(await repos.notifications.countDocuments(pendingFilter('fcm'))).toBe(0);
|
||||
});
|
||||
|
||||
it('kill switch stops sending', async () => {
|
||||
|
|
@ -133,6 +157,7 @@ describe('FCM handler', () => {
|
|||
expect(fcm.calls).toHaveLength(0);
|
||||
const after = await repos.notifications.findOne({ _id: notif._id });
|
||||
expect(after?.notified).toBeUndefined();
|
||||
expect(after?.processedBy?.fcm).toBeInstanceOf(Date); // terminal: won't be re-enqueued
|
||||
});
|
||||
|
||||
it('enforces the per-user/day limit', async () => {
|
||||
|
|
|
|||
|
|
@ -28,8 +28,8 @@ function fakeQueues() {
|
|||
|
||||
describe('pendingFilter', () => {
|
||||
it('uses the CORRECT field names (old cron had a typo)', () => {
|
||||
expect(pendingFilter('fcm')).toEqual({ type: 'mobile', notified: { $ne: true } });
|
||||
expect(pendingFilter('email')).toEqual({ type: 'web', emailNotified: { $ne: true } });
|
||||
expect(pendingFilter('fcm')).toEqual({ type: 'mobile', notified: { $ne: true }, 'processedBy.fcm': { $exists: false } });
|
||||
expect(pendingFilter('email')).toEqual({ type: 'web', emailNotified: { $ne: true }, 'processedBy.email': { $exists: false } });
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue