Skip to content

feat: improve redis cluster handling #119

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

Open
matthewwillian wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: improve redis cluster handling #119

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2ee096
feat: improve redis cluster handling
Nov 5, 2025
c8e51de
Inline cluster redis option merging
Nov 10, 2025
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
65 changes: 52 additions & 13 deletions lib/queue-factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Redis, Cluster, RedisOptions } from "ioredis";
import { ConnectionOptions as TlsConnectionOptions } from "tls";

import { QueueType, getQueueType, redisOptsFromUrl } from "./utils";
import { Queue } from "bullmq";
Expand Down Expand Up @@ -167,21 +168,59 @@ export function getRedisClient(

if (!redisClients[key]) {
if (clusterNodes && clusterNodes.length) {
const { username, password } = redisOptsFromUrl(clusterNodes[0]);
const firstClusterNode = clusterNodes[0];
const nodeCredentials: Partial<RedisOptions> = firstClusterNode
? redisOptsFromUrl(firstClusterNode)
: {};
Comment on lines +171 to +174
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The conditional check if (firstClusterNode) on line 172 is redundant since clusterNodes[0] is already checked for truthiness on line 170 (clusterNodes && clusterNodes.length). An empty array would have falsy length, so firstClusterNode will always be defined when this code is reached.

Suggested change
const firstClusterNode = clusterNodes[0];
const nodeCredentials: Partial<RedisOptions> = firstClusterNode
? redisOptsFromUrl(firstClusterNode)
: {};
const nodeCredentials: Partial<RedisOptions> = redisOptsFromUrl(clusterNodes[0]);

Copilot uses AI. Check for mistakes.
const userRedisOptions =
((redisOpts as any).redisOptions as Record<string, any>) ?? {};
const redisOptions: Record<string, any> = {
...userRedisOptions,
};
Comment on lines +176 to +179
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

Using as any casting and Record<string, any> weakens type safety. Consider defining a proper type for the nested redisOptions property or using TypeScript's utility types to extract the correct type from ioredis ClusterOptions.

Copilot uses AI. Check for mistakes.

const resolvedUsername =
redisOpts.username ??
userRedisOptions.username ??
nodeCredentials.username;
if (resolvedUsername !== undefined) {
redisOptions.username = resolvedUsername;
} else {
delete redisOptions.username;
}

const resolvedPassword =
redisOpts.password ??
userRedisOptions.password ??
nodeCredentials.password;
if (resolvedPassword !== undefined) {
redisOptions.password = resolvedPassword;
} else {
delete redisOptions.password;
}

const tlsFromEnv = process.env.REDIS_CLUSTER_TLS
? ({
cert: Buffer.from(
process.env.REDIS_CLUSTER_TLS,
"base64"
).toString("ascii"),
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The TLS certificate is decoded from base64 to ASCII. However, PEM-formatted certificates are already base64-encoded text. This double conversion may corrupt the certificate. Consider using .toString('utf8') instead of .toString('ascii'), or if the environment variable already contains a PEM certificate, no base64 decoding may be needed.

Suggested change
).toString("ascii"),
).toString("utf8"),

Copilot uses AI. Check for mistakes.
} as TlsConnectionOptions)
: undefined;
const mergedTls = Object.assign(
{},
tlsFromEnv ?? {},
(userRedisOptions.tls as TlsConnectionOptions | undefined) ?? {},
(redisOpts.tls as TlsConnectionOptions | undefined) ?? {}
) as TlsConnectionOptions;
Comment on lines +209 to +214
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

[nitpick] Using Object.assign with an empty object is less idiomatic than using object spread syntax. Consider replacing this with: const mergedTls = { ...tlsFromEnv, ...userRedisOptions.tls, ...redisOpts.tls } as TlsConnectionOptions; Note that you'll need to handle undefined values explicitly or use ?? {} on each source.

Suggested change
const mergedTls = Object.assign(
{},
tlsFromEnv ?? {},
(userRedisOptions.tls as TlsConnectionOptions | undefined) ?? {},
(redisOpts.tls as TlsConnectionOptions | undefined) ?? {}
) as TlsConnectionOptions;
const mergedTls = {
...(tlsFromEnv ?? {}),
...(userRedisOptions.tls as TlsConnectionOptions | undefined ?? {}),
...(redisOpts.tls as TlsConnectionOptions | undefined ?? {})
} as TlsConnectionOptions;

Copilot uses AI. Check for mistakes.
if (Object.keys(mergedTls).length) {
redisOptions.tls = mergedTls;
} else {
delete redisOptions.tls;
}
Comment on lines +181 to +219
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

[nitpick] The credential merging logic (lines 181-199) and TLS merging logic (lines 201-219) are repetitive. Consider extracting a generic helper function like mergeRedisOption(optionKey, ...sources) to reduce code duplication and improve maintainability.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@manast manast Nov 10, 2025

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback


redisClients[key] = new Redis.Cluster(clusterNodes, {
...redisOpts,
redisOptions: {
username,
password,
tls: process.env.REDIS_CLUSTER_TLS
? {
cert: Buffer.from(
process.env.REDIS_CLUSTER_TLS ?? "",
"base64"
).toString("ascii"),
}
: undefined,
},
redisOptions,
});
} else {
redisClients[key] = new Redis(redisOpts);
Expand Down
Loading