refactor(rate-limit): consolidate 3x-duplicated sliding-window limiter

patterns/general-code.md

@@ -1,5 +1,5 @@
 ---
-last_verified: "2026-07-17" # auto-bump @1784303093
+last_verified: "2026-07-17" # auto-bump @1784304895
 governs:
   - src/
   - evolution/

patterns/security-review.md

@@ -5,7 +5,7 @@ governs:
   - src/ipc.ts
   - src/sender-allowlist.ts
   - src/mount-security.ts
-last_verified: "2026-07-13" # auto-bump @1783974306
+last_verified: "2026-07-17" # auto-bump @1784304895
 test_tasks:
   - "Add a new mount in src/container-mounter.ts for per-group config files"
   - "Update src/mount-security.ts to permit reading a new credential path"

src/credential-proxy.ts

@@ -25,6 +25,7 @@ import { envPositiveInt } from './env-utils.js';
 import { validateGroupToken } from './group-tokens.js';
 import { readEnvFile } from './env.js';
 import { logger } from './logger.js';
+import { createRateLimiter } from './rate-limiter.js';
 import {
   AuthProviderRegistry,
   AnthropicAuthProvider,
@@ -105,45 +106,21 @@ function bridgeRecallBoundArgs(): string[] {
 const RATE_LIMIT_MAX = 20;
 const RATE_LIMIT_WINDOW_MS = 60_000;
 
-interface RateBucket {
-  timestamps: number[];
-}
-
-const rateBuckets = new Map<string, RateBucket>();
-
-/** Prune expired entries periodically to prevent unbounded growth. */
-const rateLimitCleanupInterval = setInterval(() => {
-  const now = Date.now();
-  for (const [key, bucket] of rateBuckets) {
-    bucket.timestamps = bucket.timestamps.filter(
-      (t) => now - t < RATE_LIMIT_WINDOW_MS,
-    );
-    if (bucket.timestamps.length === 0) rateBuckets.delete(key);
-  }
-}, RATE_LIMIT_WINDOW_MS);
-
-// Prevent the cleanup timer from keeping Node alive after tests/shutdown
-rateLimitCleanupInterval.unref();
+// Cleanup interval prunes expired entries periodically to prevent unbounded
+// growth; registered at module scope (not inside startCredentialProxy/
+// tryListen) so it is created exactly once at import, never recreated on an
+// EADDRINUSE retry (LIA-363).
+const limiter = createRateLimiter(RATE_LIMIT_MAX, RATE_LIMIT_WINDOW_MS, {
+  cleanupInterval: true,
+});
 
 /** @internal exposed for testing only */
 export function _resetRateLimiterForTest(): void {
-  rateBuckets.clear();
+  limiter.resetForTest();
 }
 
 function isRateLimited(sourceKey: string): boolean {
-  const now = Date.now();
-  let bucket = rateBuckets.get(sourceKey);
-  if (!bucket) {
-    bucket = { timestamps: [] };
-    rateBuckets.set(sourceKey, bucket);
-  }
-  // Prune expired timestamps for this source
-  bucket.timestamps = bucket.timestamps.filter(
-    (t) => now - t < RATE_LIMIT_WINDOW_MS,
-  );
-  if (bucket.timestamps.length >= RATE_LIMIT_MAX) return true;
-  bucket.timestamps.push(now);
-  return false;
+  return limiter.isRateLimited(sourceKey);
 }
 
 /**
@@ -497,7 +474,7 @@ export function startCredentialProxy(
         // binds on a later attempt with proactive OAuth refresh permanently
         // dead — the exact "next-morning 401" class the timer prevents (LIA-363).
         server.on('close', () => {
-          clearInterval(rateLimitCleanupInterval);
+          limiter.dispose();
           if (proactiveRefreshTimer) clearInterval(proactiveRefreshTimer);
         });
         logger.info(

src/ingress/gateway.ts

@@ -14,6 +14,7 @@ import {
   type ServerResponse,
 } from 'http';
 import { logger } from '../logger.js';
+import { createRateLimiter } from '../rate-limiter.js';
 import type { VerifyResult } from './hmac.js';
 
 /**
@@ -78,26 +79,13 @@ function forwardedFor(req: IncomingMessage): string | undefined {
   return undefined;
 }
 
-/** Sliding-window per-key rate limiter (same shape as odysseus-server.ts:83-95). */
-function makeRateLimiter(max: number, windowMs: number) {
-  const buckets = new Map<string, number[]>();
-  return function isRateLimited(key: string, now: number): boolean {
-    const ts = (buckets.get(key) ?? []).filter((t) => now - t < windowMs);
-    if (ts.length >= max) {
-      buckets.set(key, ts);
-      return true;
-    }
-    ts.push(now);
-    buckets.set(key, ts);
-    return false;
-  };
-}
-
 /** Build (but do not start) the gateway server. Exposed for tests. */
 export function createIngressGateway(deps: GatewayDeps): Server {
   const { handlers, config } = deps;
   const allow = new Set(config.ipAllowlist);
-  const isRateLimited = makeRateLimiter(
+  // No cleanupInterval — gateway.ts prunes inline during isRateLimited only,
+  // matching its behavior before the shared-limiter consolidation.
+  const limiter = createRateLimiter(
     config.rateLimitMax,
     config.rateLimitWindowMs,
   );
@@ -131,7 +119,7 @@ export function createIngressGateway(deps: GatewayDeps): Server {
     }
 
     // 2) Rate limit per peer IP.
-    if (isRateLimited(ip, started)) {
+    if (limiter.isRateLimited(ip, started)) {
       writeStatus(res, 429, 'rate limited');
       return;
     }

src/odysseus-server.ts

@@ -43,6 +43,7 @@ import { GroupQueue } from './group-queue.js';
 import { scanForInjection } from './guardrails/injection-scanner.js';
 import { logger } from './logger.js';
 import { messageText } from './openai-messages.js';
+import { createRateLimiter } from './rate-limiter.js';
 import { getAvailableGroups } from './router-state.js';
 import { RegisteredGroup } from './types.js';
 import { consolidateWebConversation } from './webui-consolidation.js';
@@ -62,41 +63,27 @@ const MAX_HISTORY_CHARS = (() => {
   return Number.isFinite(n) && n > 0 ? n : 24000;
 })();
 
-/* ── Rate limiter (mirrors credential-proxy.ts:69-113) ─────────────────── */
+/* ── Rate limiter (shared implementation — src/rate-limiter.ts) ────────── */
 const RATE_LIMIT_MAX = 5;
 const RATE_LIMIT_WINDOW_MS = 60_000;
-const rateBuckets = new Map<string, number[]>();
-const rateLimitCleanupInterval = setInterval(() => {
-  const now = Date.now();
-  for (const [key, ts] of rateBuckets) {
-    const kept = ts.filter((t) => now - t < RATE_LIMIT_WINDOW_MS);
-    if (kept.length === 0) rateBuckets.delete(key);
-    else rateBuckets.set(key, kept);
-  }
-}, RATE_LIMIT_WINDOW_MS);
-rateLimitCleanupInterval.unref();
+// Registered at module scope (not inside startOdysseusServer) so the cleanup
+// interval is created exactly once at import (LIA-363 lesson: avoid a timer
+// created/leaked on every retry of a listen-success path).
+const limiter = createRateLimiter(RATE_LIMIT_MAX, RATE_LIMIT_WINDOW_MS, {
+  cleanupInterval: true,
+});
 
 let activeSse = 0;
 /** main jid → true while a turn is queued/running (one in-flight per jid). */
 const inFlight = new Set<string>();
 
 function isRateLimited(key: string): boolean {
-  const now = Date.now();
-  const ts = (rateBuckets.get(key) ?? []).filter(
-    (t) => now - t < RATE_LIMIT_WINDOW_MS,
-  );
-  if (ts.length >= RATE_LIMIT_MAX) {
-    rateBuckets.set(key, ts);
-    return true;
-  }
-  ts.push(now);
-  rateBuckets.set(key, ts);
-  return false;
+  return limiter.isRateLimited(key);
 }
 
 /** @internal exposed for testing only */
 export function _resetServerStateForTest(): void {
-  rateBuckets.clear();
+  limiter.resetForTest();
   inFlight.clear();
   activeSse = 0;
 }
@@ -396,7 +383,7 @@ export function startOdysseusServer(
 
   return new Promise((resolve, reject) => {
     const server = createOdysseusServer(deps, token);
-    server.on('close', () => clearInterval(rateLimitCleanupInterval));
+    server.on('close', () => limiter.dispose());
     server.on('error', (err: NodeJS.ErrnoException) => reject(err));
     server.listen(ODYSSEUS_HTTP_PORT, ODYSSEUS_BIND_HOST, () => {
       logger.info(

src/rate-limiter.test.ts

@@ -0,0 +1,102 @@
+import { describe, it, expect, vi, afterEach } from 'vitest';
+import { createRateLimiter } from './rate-limiter.js';
+
+describe('createRateLimiter', () => {
+  afterEach(() => {
+    vi.useRealTimers();
+  });
+
+  it('allows requests within the window up to max, then blocks', () => {
+    const limiter = createRateLimiter(3, 60_000);
+    const now = 1_000_000;
+    expect(limiter.isRateLimited('a', now)).toBe(false);
+    expect(limiter.isRateLimited('a', now)).toBe(false);
+    expect(limiter.isRateLimited('a', now)).toBe(false);
+    // 4th request within the same window is blocked.
+    expect(limiter.isRateLimited('a', now)).toBe(true);
+  });
+
+  it('allows a request once earlier timestamps fall outside the window', () => {
+    const limiter = createRateLimiter(2, 60_000);
+    const t0 = 1_000_000;
+    expect(limiter.isRateLimited('a', t0)).toBe(false);
+    expect(limiter.isRateLimited('a', t0)).toBe(false);
+    expect(limiter.isRateLimited('a', t0)).toBe(true);
+    // Advance past the window — the two earlier timestamps are now expired.
+    const t1 = t0 + 60_001;
+    expect(limiter.isRateLimited('a', t1)).toBe(false);
+  });
+
+  it('tracks separate keys independently', () => {
+    const limiter = createRateLimiter(1, 60_000);
+    const now = 1_000_000;
+    expect(limiter.isRateLimited('a', now)).toBe(false);
+    expect(limiter.isRateLimited('a', now)).toBe(true);
+    // A different key has its own bucket.
+    expect(limiter.isRateLimited('b', now)).toBe(false);
+  });
+
+  it('defaults `now` to Date.now() when omitted', () => {
+    vi.useFakeTimers();
+    vi.setSystemTime(1_000_000);
+    const limiter = createRateLimiter(1, 60_000);
+    expect(limiter.isRateLimited('a')).toBe(false);
+    expect(limiter.isRateLimited('a')).toBe(true);
+  });
+
+  it('cleanupInterval: true creates an interval that dispose() clears', () => {
+    vi.useFakeTimers();
+    const setIntervalSpy = vi.spyOn(global, 'setInterval');
+    const clearIntervalSpy = vi.spyOn(global, 'clearInterval');
+
+    const limiter = createRateLimiter(5, 60_000, { cleanupInterval: true });
+    expect(setIntervalSpy).toHaveBeenCalledTimes(1);
+    expect(setIntervalSpy).toHaveBeenCalledWith(expect.any(Function), 60_000);
+
+    limiter.dispose();
+    expect(clearIntervalSpy).toHaveBeenCalledTimes(1);
+  });
+
+  it('cleanupInterval: false (default) creates no interval', () => {
+    const setIntervalSpy = vi.spyOn(global, 'setInterval');
+    const limiter = createRateLimiter(5, 60_000);
+    expect(setIntervalSpy).not.toHaveBeenCalled();
+    // dispose() is still safe to call (no-op) even though no interval exists.
+    expect(() => limiter.dispose()).not.toThrow();
+  });
+
+  it('the cleanup interval prunes expired entries across buckets', () => {
+    vi.useFakeTimers();
+    vi.setSystemTime(1_000_000);
+    const limiter = createRateLimiter(1, 60_000, { cleanupInterval: true });
+    expect(limiter.isRateLimited('a', Date.now())).toBe(false);
+    expect(limiter.isRateLimited('a', Date.now())).toBe(true);
+
+    // Advance past the window and let the prune interval fire.
+    vi.setSystemTime(1_000_000 + 60_001);
+    vi.advanceTimersByTime(60_000);
+
+    // The bucket was pruned, so a fresh request at the current time succeeds.
+    expect(limiter.isRateLimited('a', Date.now())).toBe(false);
+    limiter.dispose();
+  });
+
+  it('resetForTest() clears all bucket state', () => {
+    const limiter = createRateLimiter(1, 60_000);
+    const now = 1_000_000;
+    expect(limiter.isRateLimited('a', now)).toBe(false);
+    expect(limiter.isRateLimited('a', now)).toBe(true);
+    limiter.resetForTest();
+    expect(limiter.isRateLimited('a', now)).toBe(false);
+  });
+
+  it('dispose() is idempotent — safe to call multiple times', () => {
+    vi.useFakeTimers();
+    const limiter = createRateLimiter(5, 60_000, { cleanupInterval: true });
+    expect(() => {
+      limiter.dispose();
+      limiter.dispose();
+      limiter.dispose();
+    }).not.toThrow();
+  });
+});

src/rate-limiter.ts

@@ -0,0 +1,60 @@
+/**
+ * Shared sliding-window rate limiter (dedup of 3 independent implementations:
+ * credential-proxy.ts, odysseus-server.ts, ingress/gateway.ts — the first two
+ * commented "mirrors credential-proxy.ts" but were never consolidated).
+ *
+ * `Map<string, number[]>` timestamp-array shape, matching all 3 prior
+ * implementations. `opts.cleanupInterval` is opt-in (defaults false) — callers
+ * that already run a periodic prune (credential-proxy.ts, odysseus-server.ts)
+ * ask for one; gateway.ts relies on inline pruning during `isRateLimited` only,
+ * matching its current no-interval behavior exactly.
+ */
+export function createRateLimiter(
+  max: number,
+  windowMs: number,
+  opts?: { cleanupInterval?: boolean },
+): {
+  isRateLimited(key: string, now?: number): boolean;
+  dispose(): void;
+  resetForTest(): void;
+} {
+  const buckets = new Map<string, number[]>();
+
+  let cleanupTimer: ReturnType<typeof setInterval> | undefined;
+  if (opts?.cleanupInterval) {
+    cleanupTimer = setInterval(() => {
+      const now = Date.now();
+      for (const [key, ts] of buckets) {
+        const kept = ts.filter((t) => now - t < windowMs);
+        if (kept.length === 0) buckets.delete(key);
+        else buckets.set(key, kept);
+      }
+    }, windowMs);
+    // Prevent the cleanup timer from keeping Node alive after tests/shutdown.
+    cleanupTimer.unref();
+  }
+
+  function isRateLimited(key: string, now: number = Date.now()): boolean {
+    const ts = (buckets.get(key) ?? []).filter((t) => now - t < windowMs);
+    if (ts.length >= max) {
+      buckets.set(key, ts);
+      return true;
+    }
+    ts.push(now);
+    buckets.set(key, ts);
+    return false;
+  }
+
+  function dispose(): void {
+    if (cleanupTimer) {
+      clearInterval(cleanupTimer);
+      cleanupTimer = undefined;
+    }
+  }
+
+  function resetForTest(): void {
+    buckets.clear();
+  }
+
+  return { isRateLimited, dispose, resetForTest };
+}