From 87fd22cefc1f83003650d5bcf626a2abf7b1b258 Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Thu, 6 Aug 2026 17:11:00 +0200 Subject: [PATCH] [web] Fix negative AI quota countdown after reset period elapses (#35690) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix negative AI quota countdown after reset period elapses Backend (`FeatureUsageRateLimiter`, `TokenUsageRateLimiter`): once the 24h period had lapsed, `remainingUsage`/`remainingTokens` reset to the full allowance but `resetDate` kept pointing at the old `periodStart + 24h` (i.e. in the past). Advance to `now + 24h` in that case. Frontend (`NewPlanPaywall`, `ErrorGetPremium`): the "wait X hours and Y minutes" copy computed `restoreTime - now` without clamping, so a past `restoreTime` produced negative hours/minutes (users saw "wait -118 hours and -35 minutes"). Clamp to 0. * Address PR review on quota reset fix - Keep `resetDate` tied to the stored `periodStart + PERIOD` rather than jumping to `now + PERIOD` when the period has lapsed. The moving-target variant unnecessarily reinterpreted a stable field (existing consumers such as `use-ai-access-gate` check whether the reset date is in the past). - Clamp `remainingUsage` / `remainingTokens` at 0 so a concurrent over-usage never leaks a negative count. - Align the boundary check with `_checkRateLimit` / `checkUsage` (inclusive `<=`). - Cover the period boundary and the clamp in unit tests for both `FeatureUsageRateLimiter` and `WorkbenchRateLimiter`. * Truncate test fixtures to whole seconds `resetDate` is serialized with `Date.toString()`, which has second resolution, so fixtures with a millisecond component failed the boundary assertions. * Revert code changes; keep tests that document the bug The fixes masked the symptom (negative countdown) without addressing the underlying issue, and it is not fully understood yet. Revert the rate limiters and the writefull paywall components to their original behaviour, and instead pin the current behaviour in tests: - `it.fails` tests document that `remainingUsage`/`remainingTokens` go negative when usage exceeds the allowance — these flip red when the bug is fixed, at which point they should become plain `it`. - The lapsed-period tests assert (with explanatory comments) that `resetDate` keeps pointing at the end of the lapsed period, which is the server-side ingredient of the negative-countdown support ticket. * Reapply quota fixes Restores the post-review state of the rate limiters (clamp remaining counts at 0, inclusive period boundary, resetDate tied to periodStart) and the writefull paywall components (clamp countdown at 0). Tests intentionally untouched in this commit; the two `it.fails` tests now need flipping to plain `it` in a follow-up. * Flip bug-documenting tests to plain assertions The clamp fixes are reapplied, so the `it.fails` tests now assert the fixed behaviour directly. Trim the explanatory comments. * Set resetDate to `Date.now() + PERIOD_IN_MILLISECONDS` instead of the expired/stale date. Co-authored-by: Mathias Jakobsen * Update lapsed-period tests for the fresh resetDate `resetDate` is now `Date.now() + PERIOD` once the period has lapsed, so assert an approximately-fresh date instead of the stale period end. --------- Co-authored-by: Mathias Jakobsen GitOrigin-RevId: ad39627ab9e5ceccb2a61443b2b7d39c68600594 --- .../rate-limiters/FeatureUsageRateLimiter.mjs | 11 ++- .../rate-limiters/TokenUsageRateLimiter.mjs | 12 +++- ...eatureUsageRateLimiter.sequential.test.mjs | 62 +++++++++++++++++ .../WorkbenchRateLimiter.sequential.test.mjs | 67 +++++++++++++++++++ 4 files changed, 146 insertions(+), 6 deletions(-) diff --git a/services/web/app/src/infrastructure/rate-limiters/FeatureUsageRateLimiter.mjs b/services/web/app/src/infrastructure/rate-limiters/FeatureUsageRateLimiter.mjs index 3b2f342415..c00378381b 100644 --- a/services/web/app/src/infrastructure/rate-limiters/FeatureUsageRateLimiter.mjs +++ b/services/web/app/src/infrastructure/rate-limiters/FeatureUsageRateLimiter.mjs @@ -174,12 +174,17 @@ export default class FeatureUsageRateLimiter { ] ?? {} const periodStart = featureUsage.periodStart ?? new Date() const usage = featureUsage.usage ?? 0 - const usesLeft = allowance - usage const refreshEpoch = periodStart.getTime() + PERIOD_IN_MILLISECONDS + const periodExpired = refreshEpoch <= Date.now() + const remainingUsage = periodExpired ? allowance : allowance - usage + // This date isn't exactly correct when computed before an actual feature usage + const resetDate = new Date( + periodExpired ? Date.now() + PERIOD_IN_MILLISECONDS : refreshEpoch + ).toString() return { [this.featureName]: { - remainingUsage: Date.now() > refreshEpoch ? allowance : usesLeft, - resetDate: new Date(refreshEpoch).toString(), + remainingUsage: Math.max(remainingUsage, 0), + resetDate, }, } } diff --git a/services/web/app/src/infrastructure/rate-limiters/TokenUsageRateLimiter.mjs b/services/web/app/src/infrastructure/rate-limiters/TokenUsageRateLimiter.mjs index e05bcd961d..b51f7578d1 100644 --- a/services/web/app/src/infrastructure/rate-limiters/TokenUsageRateLimiter.mjs +++ b/services/web/app/src/infrastructure/rate-limiters/TokenUsageRateLimiter.mjs @@ -9,6 +9,8 @@ import UserAuditLogHandler from '../../Features/User/UserAuditLogHandler.mjs' const PERIOD = 24 // hours const PERIOD_IN_MILLISECONDS = PERIOD * 60 * 60 * 1000 +// todo: quota clean-up: extend this off base RateLimitController and unify behaviour where possible. + export default class TokenUsageRateLimiter { /** * @param {string} featureName @@ -151,12 +153,16 @@ export default class TokenUsageRateLimiter { /** @type {any} */ (reportedUsage)?.features?.[this.featureName] ?? {} const periodStart = featureUsage.periodStart ?? new Date() const usage = featureUsage.usage ?? 0 - const usesLeft = allowance - usage const refreshEpoch = periodStart.getTime() + PERIOD_IN_MILLISECONDS + const periodExpired = refreshEpoch <= Date.now() + const remainingTokens = periodExpired ? allowance : allowance - usage + const resetDate = new Date( + periodExpired ? Date.now() + PERIOD_IN_MILLISECONDS : refreshEpoch + ).toString() return { [this.featureName]: { - remainingTokens: Date.now() > refreshEpoch ? allowance : usesLeft, - resetDate: new Date(refreshEpoch).toString(), + remainingTokens: Math.max(remainingTokens, 0), + resetDate, }, } } diff --git a/services/web/test/unit/src/infrastructure/FeatureUsageRateLimiter.sequential.test.mjs b/services/web/test/unit/src/infrastructure/FeatureUsageRateLimiter.sequential.test.mjs index b6419748e3..5d5d1d0616 100644 --- a/services/web/test/unit/src/infrastructure/FeatureUsageRateLimiter.sequential.test.mjs +++ b/services/web/test/unit/src/infrastructure/FeatureUsageRateLimiter.sequential.test.mjs @@ -170,6 +170,68 @@ describe('FeatureUsageRateLimiter', function () { const usages = await ctx.FeatureUsageRateLimiter.getRemainingFeatureUses(ctx.userId) expect(usages[MOCKED_FEATURE_NAME].remainingUsage).to.equal(70) + expect( + new Date(usages[MOCKED_FEATURE_NAME].resetDate).getTime() + ).to.be.greaterThan(Date.now()) + }) + }) + + describe('at the period boundary', function () { + const PERIOD_MS = 24 * 60 * 60 * 1000 + + it('just inside the period returns usesLeft and future resetDate', async function (ctx) { + // whole second: `resetDate` round-trips through Date.toString() + const periodStart = new Date( + Math.floor((Date.now() - PERIOD_MS + 60 * 1000) / 1000) * 1000 + ) + await UserFeatureUsage.create({ + _id: ctx.userId, + features: { + [MOCKED_FEATURE_NAME]: { usage: 40, periodStart }, + }, + }) + const usages = + await ctx.FeatureUsageRateLimiter.getRemainingFeatureUses(ctx.userId) + expect(usages[MOCKED_FEATURE_NAME].remainingUsage).to.equal(60) + expect( + new Date(usages[MOCKED_FEATURE_NAME].resetDate).getTime() + ).to.equal(periodStart.getTime() + PERIOD_MS) + }) + + it('once the period has lapsed reports the full allowance and a fresh resetDate', async function (ctx) { + // whole second: `resetDate` round-trips through Date.toString() + const periodStart = new Date( + Math.floor((Date.now() - PERIOD_MS - 60 * 1000) / 1000) * 1000 + ) + await UserFeatureUsage.create({ + _id: ctx.userId, + features: { + [MOCKED_FEATURE_NAME]: { usage: 40, periodStart }, + }, + }) + const usages = + await ctx.FeatureUsageRateLimiter.getRemainingFeatureUses(ctx.userId) + expect(usages[MOCKED_FEATURE_NAME].remainingUsage).to.equal(100) + expect( + new Date(usages[MOCKED_FEATURE_NAME].resetDate).getTime() + ).to.be.approximately(Date.now() + PERIOD_MS, 5000) + }) + }) + + describe('when usage exceeds allowance', function () { + beforeEach(async function (ctx) { + await UserFeatureUsage.create({ + _id: ctx.userId, + features: { + [MOCKED_FEATURE_NAME]: { usage: 150, periodStart: new Date() }, + }, + }) + }) + + it('should clamp remainingUsage to 0 rather than leak a negative', async function (ctx) { + const usages = + await ctx.FeatureUsageRateLimiter.getRemainingFeatureUses(ctx.userId) + expect(usages[MOCKED_FEATURE_NAME].remainingUsage).to.equal(0) }) }) }) diff --git a/services/web/test/unit/src/infrastructure/WorkbenchRateLimiter.sequential.test.mjs b/services/web/test/unit/src/infrastructure/WorkbenchRateLimiter.sequential.test.mjs index c875043b7d..50d7a0194b 100644 --- a/services/web/test/unit/src/infrastructure/WorkbenchRateLimiter.sequential.test.mjs +++ b/services/web/test/unit/src/infrastructure/WorkbenchRateLimiter.sequential.test.mjs @@ -339,6 +339,73 @@ describe('WorkbenchRateLimiter', function () { }) }) + describe('getRemainingTokens', function () { + const PERIOD_MS = 24 * 60 * 60 * 1000 + + beforeEach(async function () { + await UserFeatureUsage.deleteMany({}).exec() + }) + + it('reports usesLeft and future resetDate inside the period', async function (ctx) { + // whole second: `resetDate` round-trips through Date.toString() + const periodStart = new Date( + Math.floor((Date.now() - PERIOD_MS + 60 * 1000) / 1000) * 1000 + ) + await new UserFeatureUsage({ + _id: ctx.alphaUserId, + features: { + aiWorkbench: { usage: 3_000_000, periodStart }, + }, + }).save() + + const result = await ctx.WorkbenchRateLimiter.getRemainingTokens( + ctx.alphaUserId + ) + expect(result.aiWorkbench.remainingTokens).to.equal(5_000_000) + expect(new Date(result.aiWorkbench.resetDate).getTime()).to.equal( + periodStart.getTime() + PERIOD_MS + ) + }) + + it('reports the full allowance and a fresh resetDate once the period has lapsed', async function (ctx) { + // whole second: `resetDate` round-trips through Date.toString() + const periodStart = new Date( + Math.floor((Date.now() - PERIOD_MS - 60 * 1000) / 1000) * 1000 + ) + await new UserFeatureUsage({ + _id: ctx.alphaUserId, + features: { + aiWorkbench: { usage: 7_000_000, periodStart }, + }, + }).save() + + const result = await ctx.WorkbenchRateLimiter.getRemainingTokens( + ctx.alphaUserId + ) + expect(result.aiWorkbench.remainingTokens).to.equal(8_000_000) + expect( + new Date(result.aiWorkbench.resetDate).getTime() + ).to.be.approximately(Date.now() + PERIOD_MS, 5000) + }) + + it('clamps remainingTokens to 0 when usage exceeds the allowance', async function (ctx) { + await new UserFeatureUsage({ + _id: ctx.alphaUserId, + features: { + aiWorkbench: { + usage: 20_000_000, + periodStart: new Date(), + }, + }, + }).save() + + const result = await ctx.WorkbenchRateLimiter.getRemainingTokens( + ctx.alphaUserId + ) + expect(result.aiWorkbench.remainingTokens).to.equal(0) + }) + }) + describe('recordUsage', function () { beforeEach(async function (ctx) { await UserFeatureUsage.deleteMany({}).exec()