fix(server): handle Redis idle disconnects in session-store client (#20143)
## Summary
The session-store node-redis client doesn't attach an `'error'` event
listener, so when Redis closes an idle connection (server-side `timeout`
setting), node-redis emits an unhandled `'error'` event and the entire
Node process crashes with `SocketClosedUnexpectedlyError`.
## Reproduction
1. Deploy twenty-server against a Redis instance with `timeout 300` (5
min idle close).
2. Don't log in (or otherwise keep the session store completely idle).
3. ~5 minutes after `Nest application successfully started`, the process
crashes:
```
node:events:487
throw er; // Unhandled 'error' event
^
SocketClosedUnexpectedlyError: Socket closed unexpectedly
at Socket.<anonymous> (/app/node_modules/@redis/client/dist/lib/client/socket.js:194:118)
...
Emitted 'error' event on Commander instance at:
at RedisSocket._RedisSocket_onSocketError (/app/node_modules/@redis/client/dist/lib/client/socket.js:218:10)
```
Kubernetes restarts the pod and the loop repeats every ~5 minutes (12
restarts in 95 min in our environment).
`twenty-worker` is unaffected — BullMQ's ioredis client has its own
keep-alive and the queue keeps it busy.
## Root cause
`packages/twenty-server/src/engine/core-modules/session-storage/session-storage.module-factory.ts`
constructs the node-redis client with no error listener:
```ts
const redisClient = createClient({ url: connectionString });
redisClient.connect().catch((err) => {
throw new Error(`Redis connection failed: ${err}`);
});
```
In Node.js, an unhandled `'error'` event on an `EventEmitter` becomes an
uncaught exception. node-redis emits `'error'` on socket close. With no
listener, the process exits 1 — even though node-redis would otherwise
reconnect on its own.
## Fix
1. Attach a `client.on('error', ...)` listener so disconnect errors are
logged. node-redis' built-in `reconnectStrategy` then takes over.
2. Set `pingInterval: 60_000` so the connection is never idle long
enough to be reaped by any reasonable Redis `timeout`. Defense in depth.
## Verification
Reproduced locally with Redis `CONFIG SET timeout 30` (30s for fast
reproduction). Without the fix: process exits 30s after boot. With the
fix: client logs the disconnect, reconnects, and the process keeps
running.
## Notes / out of scope
- `cache-storage.module-factory.ts` uses `cache-manager-redis-yet`
(which wraps node-redis under the hood). It may exhibit the same
vulnerability under sufficiently idle conditions; recommend a follow-up
to confirm and similarly harden it.
- `redis-client.service.ts` uses ioredis, which has built-in keepalive
and reconnect — no immediate crash risk, but adding error logging there
would be a nice consistency win.
## Test plan
- [ ] Existing tests still pass
- [ ] Manual: deploy with low Redis `timeout` (e.g. `30`), confirm
process survives
- [ ] Manual: kill Redis briefly, confirm twenty-server reconnects
instead of exiting
---------
Co-authored-by: Charles Bochet <charles@twenty.com>
This commit is contained in:
committed by
GitHub
parent
bbd9720ab3
commit
2a97e77303
+25
-3
@@ -1,10 +1,17 @@
|
||||
import { Logger } from '@nestjs/common';
|
||||
|
||||
import { type CacheModuleOptions } from '@nestjs/cache-manager';
|
||||
|
||||
import { redisStore } from 'cache-manager-redis-yet';
|
||||
import { redisInsStore } from 'cache-manager-redis-yet';
|
||||
import { createClient } from 'redis';
|
||||
|
||||
import { CacheStorageType } from 'src/engine/core-modules/cache-storage/types/cache-storage-type.enum';
|
||||
import { type TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service';
|
||||
|
||||
const cacheStorageLogger = new Logger('CacheStorage');
|
||||
|
||||
const REDIS_PING_INTERVAL_MS = 60_000;
|
||||
|
||||
export const cacheStorageModuleFactory = (
|
||||
twentyConfigService: TwentyConfigService,
|
||||
): CacheModuleOptions => {
|
||||
@@ -30,8 +37,23 @@ export const cacheStorageModuleFactory = (
|
||||
|
||||
return {
|
||||
...cacheModuleOptions,
|
||||
store: redisStore,
|
||||
url: redisUrl,
|
||||
store: async () => {
|
||||
const redisClient = createClient({
|
||||
url: redisUrl,
|
||||
pingInterval: REDIS_PING_INTERVAL_MS,
|
||||
});
|
||||
|
||||
redisClient.on('error', (err) => {
|
||||
cacheStorageLogger.error('Redis cache-storage client error', err);
|
||||
});
|
||||
|
||||
await redisClient.connect();
|
||||
|
||||
return redisInsStore(
|
||||
redisClient as Parameters<typeof redisInsStore>[0],
|
||||
{ ttl: cacheStorageTtl * 1000 },
|
||||
);
|
||||
},
|
||||
};
|
||||
}
|
||||
default:
|
||||
|
||||
+11
@@ -1,5 +1,7 @@
|
||||
import { createHash } from 'crypto';
|
||||
|
||||
import { Logger } from '@nestjs/common';
|
||||
|
||||
import RedisStore from 'connect-redis';
|
||||
import { createClient } from 'redis';
|
||||
|
||||
@@ -8,6 +10,10 @@ import type session from 'express-session';
|
||||
import { CacheStorageType } from 'src/engine/core-modules/cache-storage/types/cache-storage-type.enum';
|
||||
import { type TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service';
|
||||
|
||||
const sessionStorageLogger = new Logger('SessionStorage');
|
||||
|
||||
const REDIS_PING_INTERVAL_MS = 60_000;
|
||||
|
||||
export const getSessionStorageOptions = (
|
||||
twentyConfigService: TwentyConfigService,
|
||||
): session.SessionOptions => {
|
||||
@@ -57,6 +63,11 @@ export const getSessionStorageOptions = (
|
||||
|
||||
const redisClient = createClient({
|
||||
url: connectionString,
|
||||
pingInterval: REDIS_PING_INTERVAL_MS,
|
||||
});
|
||||
|
||||
redisClient.on('error', (err) => {
|
||||
sessionStorageLogger.error('Redis session-store client error', err);
|
||||
});
|
||||
|
||||
redisClient.connect().catch((err) => {
|
||||
|
||||
Reference in New Issue
Block a user