Skip to content

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

Merged
sliamh11 merged 2 commits into
mainfrom
dedup/rate-limiter-consolidation
Jul 17, 2026
Merged

refactor(rate-limit): consolidate 3x-duplicated sliding-window limiter#1026
sliamh11 merged 2 commits into
mainfrom
dedup/rate-limiter-consolidation

Conversation

@sliamh11

Copy link
Copy Markdown
Owner

Summary

  • src/credential-proxy.ts, src/odysseus-server.ts, and src/ingress/gateway.ts each independently implemented the same Map<string, number[]> sliding-window rate limiter — the first two even commented "mirrors credential-proxy.ts" but were never consolidated.
  • Extracts the shared logic into a new src/rate-limiter.ts exporting createRateLimiter(max, windowMs, opts?){ isRateLimited, dispose, resetForTest }.
  • opts.cleanupInterval is opt-in (default false): gateway.ts omits it, preserving its current no-interval (inline-prune-only) behavior exactly; credential-proxy.ts and odysseus-server.ts pass { cleanupInterval: true }, preserving their periodic setInterval().unref() prune behavior exactly.
  • All 3 call sites keep their existing rate-limit thresholds (20/min, 5/min, config-driven) and 60s window, and their existing test-reset export names/signatures (_resetRateLimiterForTest, _resetServerStateForTest) — no changes required in any consuming test file.
  • The LIA-363 close-handler ordering in credential-proxy.ts (cleanup registered only after a successful bind, inside tryListen's server.listen() success callback — so an EADDRINUSE retry's intermediate server.close() can't kill the timer before the real bind succeeds) is preserved verbatim; the handler now calls limiter.dispose() at the same registration point.
  • Both createRateLimiter(...) instantiations in credential-proxy.ts and odysseus-server.ts are declared at module top-level (same scope as the code they replace), never inside tryListen/startOdysseusServer — avoiding a fresh, undisposed interval being recreated on every EADDRINUSE retry (same bug class LIA-363 already fixed).
  • Adds src/rate-limiter.test.ts (8 new cases): sliding-window correctness (within/outside window), per-key isolation, now defaulting, cleanupInterval: true creates+dispose()-clears an interval, cleanupInterval: false creates no interval, the cleanup interval actually prunes expired entries, resetForTest() clears state, dispose() idempotency.

This is Slice 1 of an opportunistic dedup/reusability cleanup — not part of the tracked V2 migration roadmap. Design finalized after 8 rounds of adversarial plan review (native + GPT-5.6-Sol co-review) before this implementation, plus 3 further plan-reviewer rounds in-session to pin down the module-scope timer placement and test-export-contract details.

Behavior-preservation verification

  • npx vitest run src/rate-limiter.test.ts src/credential-proxy.test.ts src/odysseus-server.test.ts src/memory-bridge.test.ts src/ingress/gateway.test.ts97/97 passed, 0 regressions.
  • npx tsc --noEmit → clean.
  • npx eslint on all changed files → clean.
  • grep -rn "rateBuckets\|RateBucket\|makeRateLimiter\|rateLimitCleanupInterval" src/ → zero hits — no orphaned references to the old per-file implementations.
  • Independently confirmed (verification-gate + code-reviewer, both native and GPT/GLM backends) that:
    • credential-proxy.ts's const limiter = createRateLimiter(...) sits at module scope, and its server.on('close', ...) handler is still registered inside tryListen's listen-success callback (LIA-363 ordering intact).
    • odysseus-server.ts has the same module-scope placement and close-handler timing.
    • gateway.ts's createRateLimiter(config.rateLimitMax, config.rateLimitWindowMs) call passes no cleanupInterval option, matching its pre-refactor no-interval behavior, and its call site still passes an explicit captured now per request.

Review process

  • plan-reviewer: 3 rounds → SHIP (rounds 1–2 REVISE on module-scope timer placement and test-export-contract verification; round 3 resolved both).
  • code-reviewer: native + GPT + GLM backends, all SHIP.
  • verification-gate: SHIP, independently re-ran all tests/typecheck/lint and confirmed the LIA-363 ordering and module-scope placement firsthand.

Test plan

  • npx vitest run src/rate-limiter.test.ts src/credential-proxy.test.ts src/odysseus-server.test.ts src/memory-bridge.test.ts src/ingress/gateway.test.ts — 97/97 passing
  • npx tsc --noEmit — clean
  • npx eslint on changed files — clean
  • Manual grep confirms no orphaned references to the 3 old implementations

Not merging this PR — opening for review only, per the task's instructions (opportunistic cleanup outside the tracked roadmap).

🤖 Generated with Claude Code

@github-actions github-actions Bot added the core label Jul 14, 2026
@sliamh11
sliamh11 force-pushed the dedup/rate-limiter-consolidation branch from fa1fca3 to f92c0eb Compare July 17, 2026 16:09
sliamh11 and others added 2 commits July 17, 2026 19:14
credential-proxy.ts, odysseus-server.ts, and ingress/gateway.ts each
independently implemented the same Map<string, number[]> sliding-window
rate limiter — the first two even commented "mirrors credential-proxy.ts"
but were never consolidated.

Extract the shared logic into src/rate-limiter.ts's createRateLimiter(),
with an opt-in cleanupInterval (default false) so gateway.ts's no-interval
behavior and credential-proxy.ts/odysseus-server.ts's periodic-prune
behavior are both preserved exactly. All 3 call sites keep their existing
thresholds, window semantics, and test-reset export names/signatures
(_resetRateLimiterForTest, _resetServerStateForTest). The LIA-363
close-handler ordering (register only after a successful bind, so an
EADDRINUSE retry's server.close() can't kill the timer early) is
preserved verbatim in credential-proxy.ts.

Slice 1 of an opportunistic dedup cleanup; not part of the tracked V2
migration roadmap. Design finalized after 8 rounds of adversarial plan
review (native + GPT-5.6-Sol) before implementation.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@sliamh11
sliamh11 force-pushed the dedup/rate-limiter-consolidation branch from f92c0eb to 09bca40 Compare July 17, 2026 16:15
@sliamh11
sliamh11 merged commit eb71d63 into main Jul 17, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant