From 01acacc27b118708701212a5e7e39ccc5e719ac6 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:48:40 +0100 Subject: [PATCH] fix: answer 503 instead of 500 when the shared store is full or too busy to take a write #1066 --- .../__tests__/store-errors.test.ts | 22 +++++++++++++++++++ lib/shared-store/redis.ts | 11 ++++++---- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/lib/shared-store/__tests__/store-errors.test.ts b/lib/shared-store/__tests__/store-errors.test.ts index a957b63b6..b6f8564ee 100644 --- a/lib/shared-store/__tests__/store-errors.test.ts +++ b/lib/shared-store/__tests__/store-errors.test.ts @@ -12,3 +12,25 @@ describe('isStoreUnavailableError', () => { expect(second.isStoreUnavailableError(null)).toBe(false); }); }); + +describe('redis store error mapping', () => { + const reply = (message: string) => Object.assign(new Error(message), { name: 'ReplyError' }); + + it('treats a full server (OOM under noeviction) and connection errors as unavailable', async () => { + const { unavailable } = await import('../redis'); + const { isStoreUnavailableError } = await import('../types'); + for (const error of [reply('OOM command not allowed when used memory > \'maxmemory\'.'), reply('LOADING Redis is loading'), new Error('Stream isn\'t writeable')]) { + let thrown: unknown; + try { unavailable(error); } catch (e) { thrown = e; } + expect(isStoreUnavailableError(thrown)).toBe(true); + } + }); + + it('rethrows script and command errors as they are', async () => { + const { unavailable } = await import('../redis'); + const bug = reply('ERR Error running script (call to f_x): attempt to compare nil'); + let thrown: unknown; + try { unavailable(bug); } catch (e) { thrown = e; } + expect(thrown).toBe(bug); + }); +}); diff --git a/lib/shared-store/redis.ts b/lib/shared-store/redis.ts index 5483f4e5d..f698facec 100644 --- a/lib/shared-store/redis.ts +++ b/lib/shared-store/redis.ts @@ -128,9 +128,11 @@ type ScriptName = keyof typeof SCRIPTS; type Scripted = Redis & { [K in ScriptName]: (...args: Array) => Promise }; // Server replies that mean "not available right now", not "bug in our script". -const UNAVAILABLE_REPLY = /^(NOAUTH|WRONGPASS|LOADING|READONLY|MASTERDOWN|BUSY|CLUSTERDOWN|TRYAGAIN|ERR max number of clients)/; +// OOM is what a full server with the required noeviction policy answers to writes. +const UNAVAILABLE_REPLY = /^(NOAUTH|WRONGPASS|LOADING|READONLY|MASTERDOWN|BUSY|CLUSTERDOWN|TRYAGAIN|OOM|NOREPLICAS|ERR max number of clients)/; -function unavailable(error: unknown): never { +/** Throw `error` as a StoreUnavailableError when it means the store is unavailable; rethrow it as it is otherwise. */ +export function unavailable(error: unknown): never { if (isStoreUnavailableError(error)) throw error; const name = (error as Error)?.name; const message = (error as Error)?.message ?? ''; @@ -233,7 +235,8 @@ export function createRedisStore(options: RedisStoreOptions): SharedStore { if (ok === 1) return next; await backoff(attempt); } - throw new Error(`Too many concurrent writes to ${key}`); + // Fail closed like an outage: the callers then answer 503 instead of 500. + throw new StoreUnavailableError(`Too many concurrent writes to ${key}`); }, }; @@ -318,7 +321,7 @@ export function createRedisStore(options: RedisStoreOptions): SharedStore { } await backoff(attempt); } - throw new Error(`Too many concurrent writes to ${name}/${id}`); + throw new StoreUnavailableError(`Too many concurrent writes to ${name}/${id}`); }, async delete(id, putOpts: PutOptions = {}) { assertDocId(id);