From 44c4826e92d2ac5124e6072a7418998f20ac4a5d Mon Sep 17 00:00:00 2001 From: David Nguyen Date: Sun, 31 May 2026 13:34:10 +1000 Subject: [PATCH] fix: track monthly usage for unlimited quotas (#2894) --- .../api/v1/organisation-rate-limits.spec.ts | 24 ++++++++----- .../api/v2/organisation-rate-limits.spec.ts | 34 ++++++++++++------- .../rate-limit/check-monthly-quota.ts | 9 ++--- 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/packages/app-tests/e2e/api/v1/organisation-rate-limits.spec.ts b/packages/app-tests/e2e/api/v1/organisation-rate-limits.spec.ts index ea7b35055..d5e1fff6d 100644 --- a/packages/app-tests/e2e/api/v1/organisation-rate-limits.spec.ts +++ b/packages/app-tests/e2e/api/v1/organisation-rate-limits.spec.ts @@ -168,7 +168,8 @@ const getMonthlyStat = async (organisation: Organisation) => * The DB counter is the source of truth for quota enforcement, so checking its * exact value (not just the HTTP response) proves the documented increment * semantics in check-monthly-quota.ts: - * - quota === null -> never incremented (stays 0) + * - quota === null -> unlimited: never blocks, but the request is STILL + * counted (the upsert now runs before the null return) * - quota === 0 -> throws BEFORE increment (stays 0) * - quota > 0 -> incremented by `count` BEFORE the over-quota check, so * even the request that gets rejected still advances it @@ -456,9 +457,10 @@ test.describe('Organisation dynamic rate limits & quotas (API v1)', () => { // client. We therefore assert no org-specific header is surfaced. expectNoOrgRateLimitHeader(limitedRes); - // Windowed limiting uses the RateLimit bucket table, NOT the monthly quota - // counter (quota is null here), so apiCount must remain untouched. - await expectMonthlyCounter(organisation, 'apiCount', 0); + // The windowed stage blocks the (MAX+1)th request before the quota upsert, + // but each of the MAX allowed requests still records usage (null quota now + // tracks instead of skipping), so the counter equals MAX. + await expectMonthlyCounter(organisation, 'apiCount', MAX); }); test('a single allowed request succeeds when the limit is 1', async ({ request }) => { @@ -476,7 +478,9 @@ test.describe('Organisation dynamic rate limits & quotas (API v1)', () => { const body = await expectOrgLimited(limitedRes); expect(String(body.message)).toMatch(WINDOWED_LIMIT_MESSAGE); - await expectMonthlyCounter(organisation, 'apiCount', 0); + // The one allowed request is counted (null quota still tracks); the blocked + // request trips the window before the quota upsert, so the counter is 1. + await expectMonthlyCounter(organisation, 'apiCount', 1); }); test('the windowed limit RESETS once the window elapses (429 -> wait -> 200)', async ({ request }) => { @@ -531,8 +535,9 @@ test.describe('Organisation dynamic rate limits & quotas (API v1)', () => { expect(res.status()).toBe(200); } - // A null quota means "unlimited" and must never increment the counter. - await expectMonthlyCounter(organisation, 'apiCount', 0); + // A null quota means "unlimited" (never blocks), but every request is now + // recorded so usage is visible on unlimited plans — so the counter is 6. + await expectMonthlyCounter(organisation, 'apiCount', 6); }); test('exhausting the quota 429s without rate-limit headers and keeps counting', async ({ request }) => { @@ -603,8 +608,9 @@ test.describe('Organisation dynamic rate limits & quotas (API v1)', () => { await expectNotGlobalLimited(res); expect(res.ok(), `resend should succeed: ${await res.text()}`).toBeTruthy(); - // Windowed pass with a null quota must NOT touch the monthly counter. - await expectMonthlyCounter(organisation, 'emailCount', 0); + // The windowed pass is now recorded even though the quota is null, so the + // counter advances by the batch size (recipientIds.length). + await expectMonthlyCounter(organisation, 'emailCount', recipientIds.length); }); test('resend is blocked when recipient count exceeds the email window', async ({ request }) => { diff --git a/packages/app-tests/e2e/api/v2/organisation-rate-limits.spec.ts b/packages/app-tests/e2e/api/v2/organisation-rate-limits.spec.ts index 73a623716..df261eb08 100644 --- a/packages/app-tests/e2e/api/v2/organisation-rate-limits.spec.ts +++ b/packages/app-tests/e2e/api/v2/organisation-rate-limits.spec.ts @@ -157,7 +157,8 @@ const getMonthlyStat = async (organisation: Organisation) => * The DB counter is the source of truth for quota enforcement, so checking its * exact value (not just the HTTP response) proves the documented increment * semantics in check-monthly-quota.ts: - * - quota === null -> never incremented (stays 0) + * - quota === null -> unlimited: never blocks, but the request is STILL + * counted (the upsert now runs before the null return) * - quota === 0 -> throws BEFORE increment (stays 0) * - quota > 0 -> incremented by `count` BEFORE the over-quota check, so * even the request that gets rejected still advances it @@ -543,9 +544,10 @@ test.describe('Organisation dynamic rate limits & quotas', () => { expect(limitedRes.headers()['x-ratelimit-remaining']).toContain('0'); expect(limitedRes.headers()['retry-after']).toBeTruthy(); - // Windowed limiting uses the RateLimit bucket table, NOT the monthly quota - // counter (quota is null here), so apiCount must remain untouched. - await expectMonthlyCounter(organisation, 'apiCount', 0); + // The windowed stage blocks the (MAX+1)th request before the quota upsert, + // but each of the MAX allowed requests still records usage (null quota now + // tracks instead of skipping), so the counter equals MAX. + await expectMonthlyCounter(organisation, 'apiCount', MAX); }); test('a single allowed request succeeds when the limit is 1', async ({ request }) => { @@ -563,7 +565,9 @@ test.describe('Organisation dynamic rate limits & quotas', () => { const body = await expectOrgLimited(limitedRes); expect(String(body.message)).toMatch(WINDOWED_LIMIT_MESSAGE); - await expectMonthlyCounter(organisation, 'apiCount', 0); + // The one allowed request is counted (null quota still tracks); the blocked + // request trips the window before the quota upsert, so the counter is 1. + await expectMonthlyCounter(organisation, 'apiCount', 1); }); test('the windowed limit RESETS once the window elapses (429 -> wait -> 200)', async ({ request }) => { @@ -618,8 +622,9 @@ test.describe('Organisation dynamic rate limits & quotas', () => { expect(res.status()).toBe(200); } - // A null quota means "unlimited" and must never increment the counter. - await expectMonthlyCounter(organisation, 'apiCount', 0); + // A null quota means "unlimited" (never blocks), but every request is now + // recorded so usage is visible on unlimited plans — so the counter is 6. + await expectMonthlyCounter(organisation, 'apiCount', 6); }); test('exhausting the quota 429s without rate-limit headers and keeps counting', async ({ request }) => { @@ -699,8 +704,9 @@ test.describe('Organisation dynamic rate limits & quotas', () => { expectRateLimitHeaderToInclude(limitedRes, MAX); expect(limitedRes.headers()['retry-after']).toBeTruthy(); - // Windowed limiting doesn't touch the quota counter (documentQuota is null). - await expectMonthlyCounter(organisation, 'documentCount', 0); + // The (MAX+1)th create trips the window before the quota upsert, but each of + // the MAX allowed creates still records usage (null quota now tracks). + await expectMonthlyCounter(organisation, 'documentCount', MAX); }); }); @@ -747,8 +753,9 @@ test.describe('Organisation dynamic rate limits & quotas', () => { await expectNotGlobalLimited(res); expect(res.ok()).toBeTruthy(); - // A null quota means "unlimited" and must not increment the counter. - await expectMonthlyCounter(organisation, 'documentCount', 0); + // A null quota is unlimited (never blocks) but is now still recorded, so the + // single create advances the counter to 1. + await expectMonthlyCounter(organisation, 'documentCount', 1); }); }); @@ -775,8 +782,9 @@ test.describe('Organisation dynamic rate limits & quotas', () => { await expectNotGlobalLimited(res); expect(res.ok(), `redistribute should succeed: ${await res.text()}`).toBeTruthy(); - // Windowed pass with a null quota must NOT touch the monthly counter. - await expectMonthlyCounter(organisation, 'emailCount', 0); + // The windowed pass is now recorded even though the quota is null, so the + // counter advances by the batch size (recipientIds.length). + await expectMonthlyCounter(organisation, 'emailCount', recipientIds.length); }); test('redistribute is blocked when recipient count exceeds the email window', async ({ request }) => { diff --git a/packages/lib/server-only/rate-limit/check-monthly-quota.ts b/packages/lib/server-only/rate-limit/check-monthly-quota.ts index 786e7622b..fba0bc360 100644 --- a/packages/lib/server-only/rate-limit/check-monthly-quota.ts +++ b/packages/lib/server-only/rate-limit/check-monthly-quota.ts @@ -19,10 +19,6 @@ const COUNTER_COLUMN = { } as const satisfies Record; export const checkMonthlyQuota = async (opts: CheckMonthlyQuotaOptions): Promise => { - if (opts.quota === null) { - return; - } - if (opts.quota === 0) { throw new AppError(AppErrorCode.TOO_MANY_REQUESTS, { message: @@ -52,6 +48,11 @@ export const checkMonthlyQuota = async (opts: CheckMonthlyQuotaOptions): Promise }, }); + // For unlimited quotas, we still allow the request to send so we can collect the monthly stat. + if (opts.quota === null) { + return; + } + const newCount = latestMonthlyStat[column]; const previousCount = newCount - opts.count;