fix: fail closed when no shared cache is configured in production - #866
Conversation
|
@ionfwsrijan is attempting to deploy a commit to the codersogs-3057's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey @ionfwsrijan You have 4 open PRs right now. The limit is 3 at a time. Please get your existing PRs merged or closed before opening new ones:
This PR will remain open but won't be reviewed until you're under the limit. See our Contributing Guidelines for details. |
|
@Soumya-codr @codersogs-code Please review this |
jakharmonika364
left a comment
There was a problem hiding this comment.
Nice fix - this closes the exact Preview-deployment gap left over from #854 (that PR fixed the rate-limit.ts gate but cache.ts's pickDefaultBackend() was still on raw NODE_ENV, so Preview could still hit the wrong branch). BlockingBackend implements CacheBackend correctly and reuses the existing blockedRateLimitBucket() convention. The retryStrategy fix for the permanent-death-on-first-blip bug is a good catch too.
One tiny nit: rate-limit.ts's isProductionDeploy() duplicate is justified by a comment about tests that partially mock @/lib/cache, but I don't see any test doing that today - not a blocker, just flagging that these two copies will need manual sync if either changes in the future.
|
@jakharmonika364 Please review now |
Fix: Fail Closed When No Shared Cache Is Configured in Production (#861)
Problem
With no Redis/Upstash configured in production,
pickDefaultBackendfell back to an in-processMemoryBackend. On serverless, each invocation gets its own memory, so rate limiting was effectively disabled (every invocation started with a fresh counter) — webhook and action throttling silently stopped working. Separately,IoRedisBackendusedretryStrategy: () => null, permanently killing the client on the first transient blip.Changes
src/lib/cache.tsBlockingBackend: every rate-limit hit reports over the limit (blocking), and all other cache ops are safe no-ops — no stale or cross-tenant reads.pickDefaultBackendreturnsBlockingBackendwheneverisProductionDeploy()is true and no distributed backend is configured, instead of silently degrading to memory.isProductionDeploy()gates onVERCEL_ENV === 'production'(Vercel Preview also setsNODE_ENV=productionduringnext build) then falls back toNODE_ENV.retryStrategynow uses bounded backoff (min(times*250, 5000), giving up after 10 attempts) instead of failing permanently.src/lib/rate-limit.ts: kept its own localisProductionDeploycopy so tests that partially mock@/lib/cachedon't break.Impact
A misconfigured production deploy blocks unthrottled traffic instead of silently disabling rate limits, and a transient Redis blip no longer kills the cache client for the life of a warm instance.
Closes #861