From 5ce131517bf3e09af2f1eb22348bd974e03323dc Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 27 Oct 2026 14:07:25 -0400 Subject: [PATCH] fix(notifications): refuse an app message before it spends the day window The minute window is spent first, so a request the minute window refuses between the read and the spend leaves the day window untouched. --- .../NotificationDriver.appMessage.test.ts | 82 ++++++++++++++++++- .../notification/NotificationDriver.ts | 17 ++-- 2 files changed, 92 insertions(+), 7 deletions(-) diff --git a/src/backend/drivers/notification/NotificationDriver.appMessage.test.ts b/src/backend/drivers/notification/NotificationDriver.appMessage.test.ts index 712900197..52dcea270 100644 --- a/src/backend/drivers/notification/NotificationDriver.appMessage.test.ts +++ b/src/backend/drivers/notification/NotificationDriver.appMessage.test.ts @@ -17,7 +17,7 @@ * along with this program. If not, see . */ -import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; import { v4 as uuidv4 } from 'uuid'; import { makeActor, type Actor } from '../../core/actor.js'; import { runWithContext } from '../../core/context.js'; @@ -28,6 +28,42 @@ import type { IConfig } from '../../types.js'; import { peekRateLimit } from '../../core/http/middleware/rateLimit.js'; import type { NotificationDriver } from './NotificationDriver.js'; +/** + * Charges to a held key wait here until the test lets them go, so two requests + * can both be read before either is spent. Every charge is recorded by key. + */ +const charges = vi.hoisted(() => ({ + held: new Set(), + waiting: [] as Array<() => void>, + made: [] as string[], +})); + +vi.mock('../../core/http/middleware/rateLimit.js', async (importOriginal) => { + const actual = + await importOriginal< + typeof import('../../core/http/middleware/rateLimit.js') + >(); + return { + ...actual, + checkRateLimit: async (key: string, ...rest: unknown[]) => { + if (charges.held.has(key)) { + await new Promise((go) => charges.waiting.push(go)); + } + charges.made.push(key); + return ( + actual.checkRateLimit as (...a: unknown[]) => Promise + )(key, ...rest); + }, + }; +}); + +const until = async (ready: () => boolean): Promise => { + for (let i = 0; i < 400 && !ready(); i++) { + await new Promise((resolve) => setTimeout(resolve, 5)); + } + if (!ready()) throw new Error('timed out'); +}; + const makeUser = async (server: PuterServer): Promise => { const username = `am-${Math.random().toString(36).slice(2, 10)}`; const created = await server.stores.user.create({ @@ -347,6 +383,50 @@ describe('app.message with live delivery over the socket wire', () => { ).resolves.toMatchObject({ uid: expect.any(String) }); }); + it('spends the day window only on a request the minute window admits', async () => { + const holder = await makeUser(state.server); + const app = await makeApp(state.server, holder.user.id!); + const minuteKey = `notif:app-message:minute:${holder.user.id}:${app.uid}`; + const dayKey = `notif:app-message:day:${holder.user.id}:${app.uid}`; + for (let i = 0; i < 2; i++) { + await withActor(asApp(holder, app), () => + state.driver.create(message()), + ); + } + + charges.held.add(minuteKey).add(dayKey); + charges.made.length = 0; + let settled = false; + const outcome = Promise.allSettled([ + withActor(asApp(holder, app), () => state.driver.create(message())), + withActor(asApp(holder, app), () => state.driver.create(message())), + ]).finally(() => { + settled = true; + }); + try { + // Both requests read the windows as open before either spends. + await until(() => charges.waiting.length === 2); + while (!settled) { + await until(() => settled || charges.waiting.length > 0); + charges.waiting.shift()?.(); + } + } finally { + charges.held.clear(); + } + + const results = await outcome; + expect(results.map((r) => r.status).sort()).toEqual([ + 'fulfilled', + 'rejected', + ]); + const refused = results.find((r) => r.status === 'rejected') as + PromiseRejectedResult | undefined; + expect(refused?.reason).toMatchObject({ statusCode: 429 }); + // The refused request left the day window alone. + expect(charges.made.filter((k) => k === dayKey)).toEqual([dayKey]); + expect(await peekRateLimit(dayKey, 4, 24 * 60 * 60_000)).toBe(true); + }); + it("reads back the app's own messages and no other app's", async () => { const holder = await makeUser(state.server); const app = await makeApp(state.server, holder.user.id!); diff --git a/src/backend/drivers/notification/NotificationDriver.ts b/src/backend/drivers/notification/NotificationDriver.ts index 887a31e6d..7e58f3a80 100644 --- a/src/backend/drivers/notification/NotificationDriver.ts +++ b/src/backend/drivers/notification/NotificationDriver.ts @@ -340,23 +340,28 @@ export class NotificationDriver extends PuterDriver { }); } - /** Both windows are read before either is spent, so a refusal costs none. */ + /** + * Both windows are read before either is spent, so a refusal costs none. + * The minute window is spent first: it is the one most likely to run out + * between the read and the spend, and a refusal there leaves the day + * alone. + */ async #withinAppMessageLimits( userId: number, appUid: string, ): Promise { const configured = this.config.notificationAppMessageLimits ?? {}; const all: Array<[string, number, number]> = [ - [ - `notif:app-message:day:${userId}:${appUid}`, - configured.perDay ?? APP_MESSAGE_RATE_LIMITS.perDay, - DAY_MS, - ], [ `notif:app-message:minute:${userId}:${appUid}`, configured.perMinute ?? APP_MESSAGE_RATE_LIMITS.perMinute, MINUTE_MS, ], + [ + `notif:app-message:day:${userId}:${appUid}`, + configured.perDay ?? APP_MESSAGE_RATE_LIMITS.perDay, + DAY_MS, + ], ]; const windows = all.filter(([, limit]) => limit > 0); for (const [key, limit, windowMs] of windows) {