Skip to content

#587 Add tenant-scoped rate limit for /bookings/search using leaky-bu… - #739

Open
Kappa16 wants to merge 1 commit into
Chronopay-Org:mainfrom
Kappa16:#587-Add-tenant-scoped-rate-limit-for-/bookings/search-using-leaky-bucket-algorithm-FIX
Open

#587 Add tenant-scoped rate limit for /bookings/search using leaky-bu…#739
Kappa16 wants to merge 1 commit into
Chronopay-Org:mainfrom
Kappa16:#587-Add-tenant-scoped-rate-limit-for-/bookings/search-using-leaky-bucket-algorithm-FIX

Conversation

@Kappa16

@Kappa16 Kappa16 commented Jul 31, 2026

Copy link
Copy Markdown

🔍 Findings (what was discovered in the codebase)

Finding 1 — The referenced file did not exist; the endpoint didn't exist either.

on origin/main:  src/routes/booking-intents.ts  (only booking-intents)
exact path src/routes/bookings.ts on main: []  ← empty = never existed
my branch:  A  src/routes/bookings.ts  (status "added")

The issue said "Move /bookings/search from a coarse global limit…" — but no /bookings/search route existed in code, docs, or git history. So the fix had to create the endpoint and apply the per-tenant limiter to it, rather than edit an existing route.

Finding 2 — The "coarse global limit" is real: one shared fixed window for everyone.

// src/middleware/rateLimiter.ts (existing createAuthAwareRateLimiter)
windowMs: resolvedWindowMs,   // default 15 min
limit: resolvedMax,           // default 100

A single express-rate-limit fixed-window budget is applied per-principal with identical global config — but there's no tenant dimension at all. More importantly, there's an implicit starvation pattern: any endpoint sharing the window/config and any tenant flooding traffic degrades the whole class of users — the noisy-neighbor scenario from the issue. Also observed: fairQueueBurnRateTotal already misuses a tenant-ish label without real bucketing.

Finding 3 — main was already broken upstream (pre-existing, not caused by the fix).

  • tsc --noEmit: 7 syntax errors on origin/main in marketplaceSearchSchema.ts / marketplaceSearchService.ts — identical 7 on my branch (proven by direct dual-checkout comparison, and the files are byte-identical between refs).
  • Full 257-suite audit: 89 unique failing suites on main (missing exports like FraudReasonCode, undefined helpers like resetSeniorPool, etc.). My branch: zero regressions against that baseline.

Finding 4 — repo conventions to respect (found during the review):

  • Metrics use cardinality-budgeted wrappers (createBudgetedGauge/Counter) → my gauge must be budgeted too (chose 256).
  • Redis access in this codebase is namespaced (rl:) with hashed IPs — my keys follow rlb: + hashing conventions.
  • Test env must never open real network clients → my Redis client is lazy + fully injectable.

🛠 Fix features (what was built)

# Feature Where
1 Per-tenant leaky bucket — 60 rps sustained, 120 burst. Each tenant owns an independent bucket; a noisy tenant can only throttle itself src/middleware/tenantLeakyBucket.ts
2 Atomic Lua-on-Redis state transition (EVALSHA + EVAL fallback on NOSCRIPT), keys rlb:bookings:search:<type>:<id>, idle buckets auto-expire (TTL = 2 drains + 1 s) same
3 429 with exact Retry-After computed from live bucket state (never a constant) + X-RateLimit-Limit/Remaining/Reset same
4 Security-hardened tenant resolution: trusted auth context only (tenant claim → user → API key → hashed IP); spoofable x-tenant-id header deliberately ignored; identifiers canonicalized/hashed (anti key-injection, bounded cardinality) same
5 Observability: rate_limit_bucket_burn{tenant} gauge + rate_limit_redis_failures_total counter src/metrics.ts
6 Resilience — fail-open under Redis failure/latency spike with a hard 250 ms timeout budget; requests can never hang RedisLeakyBucketStore
7 Real endpoint: GET /api/v1/bookings/search (auth, q/status/slotId/date-range filters, pagination, tenant-isolated results), mounted at /api/v1/bookings src/routes/bookings.ts, src/app.ts
8 Config-driven: BOOKINGS_SEARCH_RATE_PER_SECOND (60) / BOOKINGS_SEARCH_BURST (120) / BOOKINGS_SEARCH_REDIS_TIMEOUT_MS (250) src/config/env.ts, config.service.ts, .env.example
9 Docs: operator + client guide and test notes docs/api/bookings-search.md, docs/api/bookings-search-test-notes.md
10 Tests: 62 new tests — incl. all issue-required edge cases (burst then sustained, tenant switch mid-connection, Redis latency spike, isolation) + Lua↔JS parity proof on a real Lua VM; 100% lines / 100% funcs / 97.65% branches 3 test suites

Blast radius: 12 files changed, 2,081 insertions(+), 0 deletions — purely additive; no existing code path was altered, which is why the regression audit shows 0 impact.

CLOSE #587

@drips-wave

drips-wave Bot commented Jul 31, 2026

Copy link
Copy Markdown

@Kappa16 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

}

function hashId(id: string): string {
return createHash("sha256").update(id, "utf8").digest("hex").slice(0, 32);
@Kappa16

Kappa16 commented Aug 3, 2026

Copy link
Copy Markdown
Author

@thlpkee20-wq PLS REVIEW

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add tenant-scoped rate limit for /bookings/search using leaky-bucket algorithm

2 participants