From 5665262ae28c406c53fc9dd3dc2727865c43a09b Mon Sep 17 00:00:00 2001 From: tada5hi Date: Mon, 5 Oct 2026 17:36:35 +0000 Subject: [PATCH] fix(rate-limit): refresh the remaining header after a skipped request When skipSuccessfulRequest or skipFailedRequest takes a request back off the counter, the RateLimit-Remaining header (and the request's rate limit info) now reports the count after the decrement instead of before it, on the returned response as well as on a fallback response. A response with immutable headers is copied. Document that a throw no error handler answered reaches the middleware as no response and is counted as failed, and pin both that and the reserving behaviour that bounds concurrent failed requests by max. --- packages/rate-limit/README.md | 4 + packages/rate-limit/src/handler.ts | 29 +++++- packages/rate-limit/test/unit/module.spec.ts | 104 ++++++++++++++++++- 3 files changed, 132 insertions(+), 5 deletions(-) diff --git a/packages/rate-limit/README.md b/packages/rate-limit/README.md index 3d2b5ab5..33201aba 100644 --- a/packages/rate-limit/README.md +++ b/packages/rate-limit/README.md @@ -253,6 +253,10 @@ const handler = createHandler({ }) ``` +A request whose handler throws is judged by the response of the error handler +that answers it. When no error handler is registered, the middleware sees no +response at all and counts the request as not successful, whatever its status. + ### `store` The `Store` to use to store the hit count for each client. diff --git a/packages/rate-limit/src/handler.ts b/packages/rate-limit/src/handler.ts index ca83cab3..a776c63d 100644 --- a/packages/rate-limit/src/handler.ts +++ b/packages/rate-limit/src/handler.ts @@ -64,6 +64,8 @@ export function createHandler(input?: OptionsInput) { options.skipFailedRequest || options.skipSuccessfulRequest ) { + // a downstream throw no error handler answered reaches this + // handler as no response at all, so it cannot be judged const wasSuccessful = response ? options.requestWasSuccessful(event, response) : false; @@ -73,7 +75,32 @@ export function createHandler(input?: OptionsInput) { (options.skipSuccessfulRequest && wasSuccessful) ) { await options.store.decrement(key); - setRequestRateLimitInfo(event, 'remaining', Math.max(maxHits - totalHits + 1, 0)); + + const remaining = Math.max(maxHits - totalHits + 1, 0); + setRequestRateLimitInfo(event, 'current', totalHits - 1); + setRequestRateLimitInfo(event, 'remaining', remaining); + + const value = String(remaining); + event.response.headers.set(HeaderName.RATE_LIMIT_REMAINING, value); + + if (!response) { + return response; + } + + try { + response.headers.set(HeaderName.RATE_LIMIT_REMAINING, value); + return response; + } catch { + // the headers of a forwarded fetch() response are immutable + const headers = new Headers(response.headers); + headers.set(HeaderName.RATE_LIMIT_REMAINING, value); + + return new Response(response.body, { + status: response.status, + statusText: response.statusText, + headers, + }); + } } } diff --git a/packages/rate-limit/test/unit/module.spec.ts b/packages/rate-limit/test/unit/module.spec.ts index a6e3fbbc..4426828d 100644 --- a/packages/rate-limit/test/unit/module.spec.ts +++ b/packages/rate-limit/test/unit/module.spec.ts @@ -2,7 +2,9 @@ import { describe, expect, it } from 'vitest'; import { App, HeaderName, + createError, defineCoreHandler, + defineErrorHandler, } from 'routup'; import { RETRY_AGAIN_MESSAGE, rateLimit } from '../../src'; @@ -57,11 +59,11 @@ describe('src/module', () => { let response = await router.fetch(createTestRequest('/')); - expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('4'); + expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('5'); response = await router.fetch(createTestRequest('/')); - expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('4'); + expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('5'); }); it('should be possible to skip failed responses', async () => { @@ -70,11 +72,11 @@ describe('src/module', () => { let response = await router.fetch(createTestRequest('/')); - expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('4'); + expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('5'); response = await router.fetch(createTestRequest('/')); - expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('4'); + expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('5'); }); it('should skip request', async () => { @@ -89,4 +91,98 @@ describe('src/module', () => { expect(response.headers.get(HeaderName.RATE_LIMIT_RESET)).toBeNull(); expect(response.headers.get(HeaderName.RETRY_AFTER)).toBeNull(); }); + + it('should judge a thrown error by the response of the error handler', async () => { + const router = new App(); + router.use(rateLimit({ + max: 1, + skipSuccessfulRequest: true, + requestWasSuccessful: (_event, response) => response.status !== 401, + })); + router.use(defineCoreHandler(() => { + throw createError({ status: 500 }); + })); + router.use(defineErrorHandler((error, event) => { + event.response.status = error.status; + return error.message; + })); + + let response = await router.fetch(createTestRequest('/')); + expect(response.status).toEqual(500); + expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('1'); + + response = await router.fetch(createTestRequest('/')); + expect(response.status).toEqual(500); + }); + + it('should count a thrown error no error handler answered', async () => { + const router = new App(); + router.use(rateLimit({ + max: 1, + skipSuccessfulRequest: true, + requestWasSuccessful: (_event, response) => response.status !== 401, + })); + router.use(defineCoreHandler(() => { + throw createError({ status: 500 }); + })); + + let response = await router.fetch(createTestRequest('/')); + expect(response.status).toEqual(500); + + response = await router.fetch(createTestRequest('/')); + expect(response.status).toEqual(429); + }); + + it('should count a thrown error the predicate rejects', async () => { + const router = new App(); + router.use(rateLimit({ + max: 1, + skipSuccessfulRequest: true, + requestWasSuccessful: (_event, response) => response.status !== 401, + })); + router.use(defineCoreHandler(() => { + throw createError({ status: 401 }); + })); + router.use(defineErrorHandler((error, event) => { + event.response.status = error.status; + return error.message; + })); + + let response = await router.fetch(createTestRequest('/')); + expect(response.status).toEqual(401); + expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('0'); + + response = await router.fetch(createTestRequest('/')); + expect(response.status).toEqual(429); + }); + + it('should refresh the remaining header on a response with immutable headers', async () => { + const router = new App(); + router.use(rateLimit({ skipSuccessfulRequest: true })); + router.use(defineCoreHandler(() => Response.redirect('http://localhost/target', 302))); + + const response = await router.fetch(createTestRequest('/')); + + expect(response.status).toEqual(302); + expect(response.headers.get('location')).toEqual('http://localhost/target'); + expect(response.headers.get(HeaderName.RATE_LIMIT_REMAINING)).toEqual('5'); + }); + + it('should bound concurrent failed requests by max', async () => { + const router = new App(); + let reached = 0; + router.use(rateLimit({ max: 2, skipSuccessfulRequest: true })); + router.use(defineCoreHandler(async () => { + reached++; + await new Promise((resolve) => { setTimeout(resolve, 10); }); + throw createError({ status: 401 }); + })); + + const responses = await Promise.all( + Array.from({ length: 10 }, () => router.fetch(createTestRequest('/'))), + ); + + expect(reached).toEqual(2); + expect(responses.filter((r) => r.status === 429)).toHaveLength(8); + }); });