fix: remove permanent cache backend mutation on Redis error - #846
Conversation
The IoRedis error handler was permanently replacing the global 'backend' variable with a MemoryBackend on any Redis error, with no recovery mechanism. In multi-instance deployments, a transient Redis blip permanently disabled distributed rate limiting on that instance. Fix: Remove the backend reassignment from the error handler. Each IoRedisBackend method already has per-call try-catch blocks that gracefully return null or blockedRateLimitBucket on transient errors. The error handler now only logs the warning instead of mutating global state. Closes Coder-s-OG-s#843
|
@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. |
|
@codersogs-code @Soumya-codr Please review this |
|
CI is failing can you please fix it |
|
@jakharmonika364 @Soumya-codr Please review now |
jakharmonika364
left a comment
jakharmonika364
left a comment
There was a problem hiding this comment.
Fix looks correct - good catch, and the per-call fail-closed fallback is already tested for both backends, so this is safe.
One ask before merge: this path (pickDefaultBackend's Redis client error handler) has zero test coverage, the existing cache.test.ts only constructs IoRedisBackend directly with mocks, never exercises this env-driven branch. That's likely how the original bug slipped in unnoticed.
Can you add a regression test: mock ioredis, set REDIS_URL, call pickDefaultBackend(), emit a synthetic error event, then assert cacheGet/rate-limit calls still route through the same IoRedisBackend instance instead of silently swapping to MemoryBackend?
Previously, a Redis error event permanently swapped the global backend to MemoryBackend. The existing tests only constructed IoRedisBackend directly, so this env-driven branch had zero coverage. Mock ioredis, load the module with REDIS_URL set, emit a synthetic error event, and assert cacheGet and rate-limit calls still route through the same IoRedis client.
|
@jakharmonika364 Please review now. I've made the changes |
jakharmonika364
added
level:advanced
2 participants
Description
The
IoRedisBackendconstructor registers an error handler on the Redis client that permanently replaces the globalbackendvariable with a freshMemoryBackendon any Redis error. There is no recovery mechanism — once switched,backendis never reset back to Redis. In multi-instance deployments, a transient Redis blip permanently kills distributed rate limiting on that instance.Changes
backend = new MemoryBackend()reassignment from the iorediserrorevent handlerIoRedisBackendmethod (get,set,del,scanDel,rateLimitHit,rateLimitHitSlidingWindow) already has per-call try-catch blocks that gracefully returnnullorblockedRateLimitBucketon transient errors, so the removal is safeCloses #843