Added support for single redis instances#1900
Conversation
WalkthroughA new exported helper function Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/configuration/registrations.ts`:
- Around line 106-107: The port assignment at line 106 using
Number(process.env.REDIS_PORT) || 6379 silently converts malformed port values
to NaN, which then defaults to 6379, masking configuration errors. Implement
explicit validation for the REDIS_PORT environment variable that checks if the
value is a valid number and within the valid port range (1-65535), and throw an
error with a clear message if the value is invalid or missing, rather than
allowing silent defaults on malformed input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51a3e53c-ad91-4a76-85f5-1fb96ec49620
📒 Files selected for processing (3)
docs/env-vars.mdsrc/configuration/registrations.tssrc/configuration/registrations.unit.test.ts
| const port = Number(process.env.REDIS_PORT) || 6379; | ||
| const tls = process.env.REDIS_TLS_CERT |
There was a problem hiding this comment.
Validate REDIS_PORT explicitly instead of silently defaulting on malformed values.
Line 106 currently uses Number(process.env.REDIS_PORT) || 6379; malformed values (for example, 638O) silently become 6379, masking misconfiguration and potentially connecting to the wrong endpoint.
Suggested fix
- const host = process.env.REDIS_HOST || 'localhost';
- const port = Number(process.env.REDIS_PORT) || 6379;
+ const host = process.env.REDIS_HOST || 'localhost';
+ const rawPort = process.env.REDIS_PORT;
+ const parsedPort = rawPort == null ? undefined : Number(rawPort);
+ if (
+ parsedPort !== undefined &&
+ (!Number.isInteger(parsedPort) || parsedPort < 1 || parsedPort > 65535)
+ ) {
+ throw new Error(`Invalid REDIS_PORT: ${rawPort}`);
+ }
+ const port = parsedPort ?? 6379;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const port = Number(process.env.REDIS_PORT) || 6379; | |
| const tls = process.env.REDIS_TLS_CERT | |
| const host = process.env.REDIS_HOST || 'localhost'; | |
| const rawPort = process.env.REDIS_PORT; | |
| const parsedPort = rawPort == null ? undefined : Number(rawPort); | |
| if ( | |
| parsedPort !== undefined && | |
| (!Number.isInteger(parsedPort) || parsedPort < 1 || parsedPort > 65535) | |
| ) { | |
| throw new Error(`Invalid REDIS_PORT: ${rawPort}`); | |
| } | |
| const port = parsedPort ?? 6379; | |
| const tls = process.env.REDIS_TLS_CERT |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/configuration/registrations.ts` around lines 106 - 107, The port
assignment at line 106 using Number(process.env.REDIS_PORT) || 6379 silently
converts malformed port values to NaN, which then defaults to 6379, masking
configuration errors. Implement explicit validation for the REDIS_PORT
environment variable that checks if the value is a valid number and within the
valid port range (1-65535), and throw an error with a clear message if the value
is invalid or missing, rather than allowing silent defaults on malformed input.
There was a problem hiding this comment.
LGTM!
One suggestion: as we'd potentially want to extend this to Redis Sentinel later, what do you think of using REDIS_MODE (with a default to cluster) instead of REDIS_CLUSTER true/false?
docs/env-vars.md:
- `REDIS_MODE` - Redis connection mode: `cluster` (default) or `standalone`
- Set to `standalone` to connect to a single-node Redis server, e.g. a plain `redis:alpine` container
- Default: `cluster`
src/configuration/registrations.ts:
type RedisMode = 'cluster' | 'standalone';
function getRedisMode(): RedisMode {
const mode = process.env.REDIS_MODE ?? 'cluster';
if (mode !== 'cluster' && mode !== 'standalone') {
throw new Error(
`Invalid REDIS_MODE "${mode}". Expected "cluster" or "standalone".`,
);
}
return mode;
}
/**
* Create a Redis connection for use as Fedify's KvStore.
*
* Defaults to a Redis Cluster connection (the historical behaviour). Set
* `REDIS_MODE=standalone` to connect to a single-node Redis server, e.g. a
* plain `redis:alpine` container.
*/
export function createRedisConnection(logging: Logger): Redis | Cluster {
const mode = getRedisMode();
const host = process.env.REDIS_HOST || 'localhost';
const port = Number(process.env.REDIS_PORT) || 6379;
const tls = process.env.REDIS_TLS_CERT
? { ca: process.env.REDIS_TLS_CERT }
: undefined;
const retryStrategy = (times: number) => {
const delay = Math.min(times * 50, 2000);
logging.warn(
`Redis connection retry attempt ${times}, delay ${delay}ms`,
);
return delay;
};
if (mode === 'standalone') {
logging.info('Connecting to Redis in standalone mode');
return new Redis({
host,
port,
retryStrategy,
enableOfflineQueue: true,
maxRetriesPerRequest: 3,
enableReadyCheck: true,
tls,
});
}
logging.info('Connecting to Redis in cluster mode');
return new Redis.Cluster([{ host, port }], {
clusterRetryStrategy: retryStrategy,
enableOfflineQueue: true,
redisOptions: {
maxRetriesPerRequest: 3,
enableReadyCheck: true,
tls,
},
});
}
src/configuration/registrations.unit.test.ts:
beforeEach(() => {
clusterConstructor.mockClear();
redisConstructor.mockClear();
process.env.REDIS_HOST = 'redis-host';
process.env.REDIS_PORT = '6380';
delete process.env.REDIS_MODE;
delete process.env.REDIS_TLS_CERT;
});
it('connects in cluster mode by default', () => {
createRedisConnection(logging);
expect(clusterConstructor).toHaveBeenCalledTimes(1);
expect(redisConstructor).not.toHaveBeenCalled();
const nodes = clusterConstructor.mock.calls[0][0];
expect(nodes).toEqual([{ host: 'redis-host', port: 6380 }]);
});
it('connects in standalone mode when REDIS_MODE is "standalone"', () => {
process.env.REDIS_MODE = 'standalone';
createRedisConnection(logging);
expect(redisConstructor).toHaveBeenCalledTimes(1);
expect(clusterConstructor).not.toHaveBeenCalled();
const options = redisConstructor.mock.calls[0][0];
expect(options.host).toBe('redis-host');
expect(options.port).toBe(6380);
});
it('throws on an unrecognised REDIS_MODE', () => {
process.env.REDIS_MODE = 'sentinel';
expect(() => createRedisConnection(logging)).toThrow(/Invalid REDIS_MODE/);
});
2 participants
closes #1868