From cc4e767a88a76422f08037d12b62b66fe35d4d55 Mon Sep 17 00:00:00 2001 From: Yoichi Takizawa Date: Tue, 4 Aug 2026 07:10:02 +0900 Subject: [PATCH] test: stop rate-limit tests straddling a fixed-window boundary `accepts Keycloak azp as the client identifier` failed on main under Node 24 while passing under Node 22, which reads like a version problem and is not one. The limiter counts into a window keyed on Math.floor(now / 60_000), so the window is aligned to the wall clock rather than to a caller's first request. Two calls a millisecond apart land in different windows when they straddle a minute boundary, and the second starts a fresh count -- so an assertion that the next call is 429 sees 200 instead. The failing run reported the case at 14:47:00.037 after 45 ms, meaning it started at 14:46:59.99 and crossed the boundary mid-test. Node 24 was coincidence: that job happened to reach the case at the wrong moment. Two changes: - Pin the boundary behaviour deterministically in the rate-limit unit tests using the injectable clock the limiter already accepts, so this property is documented rather than rediscovered through a flake. It is the accepted trade-off of a fixed window, not a defect, and the retryAfterSeconds case shows the value points at the reset. - Guard the seven HTTP-boundary tests whose assertions need two or more calls to share a window. They now wait only when the current window has less than five seconds left, so the common path costs nothing. The product is unchanged. Adding a window-length knob purely so tests could avoid this would have put test convenience into the rate limiter. Honest scope note: the limiter-level reset is proven deterministically, and the CI timestamp matches it exactly, but I could not reproduce the HTTP-level flake on demand -- it needs the boundary to fall between two specific calls, a millisecond-wide target. Running the suite across a boundary passes either way, because the affected cases sit late enough in the file to be past it. Unit 1,941 pass, lint clean. --- test/unit/mcp-http-boundary.test.js | 25 +++++++++++++++++ test/unit/mcp-rate-limit.test.js | 43 +++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/test/unit/mcp-http-boundary.test.js b/test/unit/mcp-http-boundary.test.js index b6ec3c5..5a111eb 100644 --- a/test/unit/mcp-http-boundary.test.js +++ b/test/unit/mcp-http-boundary.test.js @@ -105,6 +105,24 @@ function issuerFetch(url) { }); } +// The limiter counts into a window keyed on Math.floor(now / 60_000) +// (src/mcp-rate-limit.js), so the window is aligned to the wall clock, not to +// a test's first request. Two calls a millisecond apart land in different +// windows if they straddle a minute boundary, and the second one starts a +// fresh count -- which makes any "the next call is 429" assertion fail. +// +// That is not hypothetical: it broke `accepts Keycloak azp as the client +// identifier` on main, in a run whose test started at 14:46:59.99. +// +// Only start such a test when the current window still has room. The wait is +// skipped in the common case, and never exceeds the margin. +const RATE_WINDOW_MS = 60_000; +async function insideOneRateWindow(marginMs = 5_000) { + const remaining = RATE_WINDOW_MS - (Date.now() % RATE_WINDOW_MS); + if (remaining >= marginMs) return; + await new Promise(resolve => setTimeout(resolve, remaining + 25)); +} + async function start({ limits = {}, env = {} } = {}) { const previous = {}; const applied = { @@ -181,6 +199,7 @@ afterEach(async () => { describe('MCP HTTP boundary: per-identity limits', () => { it('applies the per-subject limit, which needs the identity the OAuth layer sets', async () => { + await insideOneRateWindow(); const restore = await start({ limits: { subject: 2, global: 1000 } }); try { const token = mintToken({ sub: 'user-1', client_id: 'client-a' }); @@ -193,6 +212,7 @@ describe('MCP HTTP boundary: per-identity limits', () => { }); it('limits one subject without affecting another', async () => { + await insideOneRateWindow(); const restore = await start({ limits: { subject: 1, client: 1000, global: 1000 } }); try { const a = mintToken({ sub: 'user-a', client_id: 'c' }); @@ -204,6 +224,7 @@ describe('MCP HTTP boundary: per-identity limits', () => { }); it('applies the per-client limit across different subjects', async () => { + await insideOneRateWindow(); const restore = await start({ limits: { client: 2, subject: 1000, global: 1000 } }); try { await call(mintToken({ sub: 'u1', client_id: 'shared' })); @@ -214,6 +235,7 @@ describe('MCP HTTP boundary: per-identity limits', () => { }); it('accepts Keycloak azp as the client identifier', async () => { + await insideOneRateWindow(); const restore = await start({ limits: { client: 1, subject: 1000, global: 1000 } }); try { await call(mintToken({ sub: 'u1', client_id: undefined, azp: 'kc-client' })); @@ -232,6 +254,7 @@ describe('MCP HTTP boundary: per-identity limits', () => { }); it('counts the global budget once per request, not twice', async () => { + await insideOneRateWindow(); const restore = await start({ limits: { global: 2, subject: 1000, client: 1000 } }); try { const token = mintToken({ client_id: 'c' }); @@ -244,6 +267,7 @@ describe('MCP HTTP boundary: per-identity limits', () => { describe('MCP private HTTP boundary', () => { it('applies global and credential limits to token-authenticated requests', async () => { + await insideOneRateWindow(); const restore = await startPrivate({ limits: { global: 10, subject: 1, client: 10 } }); try { const token = privateAuthConfig().token; @@ -379,6 +403,7 @@ describe('MCP HTTP boundary: audit identity', () => { describe('MCP HTTP boundary: body handling runs after the limits', () => { it('rate limits malformed JSON instead of letting it bypass the limiter', async () => { + await insideOneRateWindow(); const restore = await start({ limits: { global: 2 } }); try { await call(null, '{ this is not json'); diff --git a/test/unit/mcp-rate-limit.test.js b/test/unit/mcp-rate-limit.test.js index 5df4a18..4d375ba 100644 --- a/test/unit/mcp-rate-limit.test.js +++ b/test/unit/mcp-rate-limit.test.js @@ -130,3 +130,46 @@ describe('MCP limit configuration', () => { } }); }); + +describe('MCP rate limiting: 固定ウィンドウの境界', () => { + // The counter is keyed on Math.floor(now / windowMs), so the window is + // aligned to the wall clock rather than to a caller's first request. Two + // requests a millisecond apart therefore land in different windows if they + // straddle a minute boundary, and the second one starts a fresh count. + // + // This is inherent to a fixed window and is the documented trade-off, but it + // is worth pinning: it silently broke a boundary test on main that assumed + // two consecutive calls always share a window. + it('境界をまたぐと、直後の呼び出しでもカウントがリセットされる', () => { + const clock = { value: 59_999 }; + const limiter = limiterAt(clock, { globalPerMinute: 1, perSubjectPerMinute: 99, perClientPerMinute: 99 }); + assert.equal(limiter.check({ subject: 's', clientId: 'c' }).allowed, true); + assert.equal(limiter.check({ subject: 's', clientId: 'c' }).allowed, false, '同一ウィンドウ内では制限される'); + + clock.value = 60_000; // 1 ms later, but a new window + assert.equal( + limiter.check({ subject: 's', clientId: 'c' }).allowed, + true, + '1ms後でもウィンドウが変われば許可される' + ); + }); + + it('ウィンドウ内に留まる限り境界の影響を受けない', () => { + const clock = { value: 60_000 }; + const limiter = limiterAt(clock, { globalPerMinute: 1, perSubjectPerMinute: 99, perClientPerMinute: 99 }); + assert.equal(limiter.check({ subject: 's', clientId: 'c' }).allowed, true); + clock.value = 119_999; // still the same window + assert.equal(limiter.check({ subject: 's', clientId: 'c' }).allowed, false); + }); + + it('retryAfterSecondsは次の境界までの残り時間を指す', () => { + const clock = { value: 60_000 + 15_000 }; + const limiter = limiterAt(clock, { globalPerMinute: 1, perSubjectPerMinute: 99, perClientPerMinute: 99 }); + limiter.check({ subject: 's', clientId: 'c' }); + const denied = limiter.check({ subject: 's', clientId: 'c' }); + // Callers put this in Retry-After, so it must point at the window reset + // rather than at a full window length from now. + assert.equal(denied.allowed, false); + assert.equal(denied.retryAfterSeconds, 45); + }); +});