Skip to content

refactor: improve Redis client connection handling and error management to prevent app shutdown #1449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Artuomka merged 1 commit into from
Nov 24, 2025
Merged

refactor: improve Redis client connection handling and error management to prevent app shutdown #1449

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 42 additions & 15 deletions shared-code/src/data-access-layer/data-access-objects/data-access-object-redis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,6 @@ export class DataAccessObjectRedis extends BasicDataAccessObject implements IDat
try {
const redisClient = await this.getClient();
const response = await redisClient.ping();

if (response === 'PONG') {
return {
result: true,
Expand Down Expand Up @@ -855,23 +854,51 @@ export class DataAccessObjectRedis extends BasicDataAccessObject implements IDat
? Number(this.connection.database)
: 0
: 0;
if (!client) {
client = createClient({
socket: {
try {
if (!client) {
const shouldUseTLS = this.connection.ssl !== false;

const socketConfig: any = {

Copilot AI Nov 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type annotation any for socketConfig bypasses type safety. Consider using a proper type from the Redis client library or defining a specific interface for the socket configuration.

For example:

import type { RedisSocketOptions } from 'redis';

const socketConfig: Partial<RedisSocketOptions> = {
  // ...
};

Copilot uses AI. Check for mistakes.
host: this.connection.host,
port: this.connection.port,
ca: this.connection.cert || undefined,
cert: this.connection.cert || undefined,
rejectUnauthorized: this.connection.ssl === false ? false : true,
},
password: this.connection.password || undefined,
username: this.connection.username || undefined,
database: database,
});
await client.connect();
LRUStorage.setRedisClientCache(this.connection, client);
reconnectStrategy: (retries: number) => {
if (retries > 3) {
return new Error('Max reconnection attempts reached');

Copilot AI Nov 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reconnectStrategy should throw an error or return false to stop reconnection attempts, not return an Error instance. According to the Redis client documentation, the function should either:

  • Return a number (milliseconds to wait before next retry)
  • Return false or throw an Error to stop reconnection

Current code returns an Error object which will be treated as a truthy value and converted to NaN for the delay.

Recommended fix:

reconnectStrategy: (retries: number) => {
  if (retries > 3) {
    return false; // or throw new Error('Max reconnection attempts reached');
  }
  return Math.min(retries * 100, 3000);
},
Suggested change
return new Error('Max reconnection attempts reached');
return false;

Copilot uses AI. Check for mistakes.
}
return Math.min(retries * 100, 3000);
},
};

if (shouldUseTLS) {
socketConfig.tls = true;
socketConfig.rejectUnauthorized = this.connection.ssl === false ? false : true;

Copilot AI Nov 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for rejectUnauthorized is incorrect. When shouldUseTLS is true, this line will always evaluate to true because it checks this.connection.ssl === false (which is false when TLS is enabled).

The current logic:

  • When this.connection.ssl !== false (TLS enabled): rejectUnauthorized = false ? false : true → true
  • When this.connection.ssl === false (TLS disabled): This block won't execute anyway

This should be simplified or corrected. Consider:

socketConfig.rejectUnauthorized = this.connection.ssl !== false;

Or if you need explicit control over certificate validation, use a dedicated property rather than overloading the ssl boolean.

Suggested change
socketConfig.rejectUnauthorized = this.connection.ssl === false ? false : true;
socketConfig.rejectUnauthorized = this.connection.ssl !== false;

Copilot uses AI. Check for mistakes.

if (this.connection.cert) {
socketConfig.ca = this.connection.cert;
socketConfig.cert = this.connection.cert;
}
}

client = createClient({
socket: socketConfig,
password: this.connection.password || undefined,
username: this.connection.username || undefined,
database: database,
});

client.on('error', (err) => {
console.error('Redis Client Error:', err);
LRUStorage.delRedisClientCache(this.connection);
});
Comment on lines +889 to +892

Copilot AI Nov 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using console.error for logging in a library/shared-code module is not ideal. Consider using a proper logging framework or allowing the error to bubble up through the existing error handling mechanism.

Additionally, the error handler removes the client from cache but doesn't handle the case where the client might be in an error state but still cached. This could lead to subsequent calls trying to use a failed client before it gets removed.

Consider either:

  1. Using a proper logger that's injected or configured for the application
  2. Emitting events that can be handled by application-level logging
  3. At minimum, making logging configurable

Copilot uses AI. Check for mistakes.

await client.connect();
LRUStorage.setRedisClientCache(this.connection, client);
}
return client;
} catch (error) {
LRUStorage.delRedisClientCache(this.connection);
throw error;
}
return client;
}

private async createTunneledConnection(connection: ConnectionParams): Promise<RedisClientType> {
Expand Down
Loading