Redis client factory (error handling and good defaults) #1761
Conversation
|
WalkthroughThe pull request introduces a new internal Redis package and updates dependencies across multiple modules to utilize it. It adds new path mappings in TypeScript configurations and refactors existing Redis client initializations to use a unified Changes
Sequence Diagram(s)sequenceDiagram
participant Module as Module/Engine/Test
participant CR as createRedisClient
participant Redis as Redis Client
Module->>CR: Call createRedisClient(options, { onError })
CR->>Redis: Initialize Redis client with default & provided options
alt VITEST mode active
Redis-->>CR: Swallow errors
else
Redis-->>CR: Errors trigger onError callback
end
CR-->>Module: Return configured Redis client
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
internal-packages/redis-worker/src/queue.ts (2)
54-55: Verify if empty lines removal was intentional.The code has fewer empty lines compared to the original pattern. While this doesn't affect functionality, confirm this style change was intentional for consistency.
56-65: Consider adding a comment explaining the Redis configuration.The Redis configuration includes important settings like the retry strategy and max retries. Consider adding a brief comment explaining these choices for future maintainers.
this.redis = createRedisClient( { ...redisOptions, keyPrefix: `${redisOptions.keyPrefix ?? ""}{queue:${name}:}`, + // Exponential backoff with 50ms base and 1s max retryStrategy(times) { const delay = Math.min(times * 50, 1000); return delay; }, + // Increase max retries to prevent operation failures during temporary disconnections maxRetriesPerRequest: 20, },internal-packages/redis/src/index.ts (3)
12-12: Consider making the logger level configurable.The logger is initialized with a hardcoded "debug" level. For better flexibility, consider accepting the log level as a parameter to the
createRedisClientfunction.-const logger = new Logger("Redis", "debug"); +const logger = new Logger("Redis", process.env.REDIS_LOG_LEVEL || "debug");
1-40: Consider adding connection timeouts to the default configuration.The current default configuration doesn't include connection timeout settings. For improved reliability, especially in containerized environments, consider adding reasonable timeouts.
const defaultOptions: Partial<RedisOptions> = { retryStrategy: (times: number) => { const delay = Math.min(times * 50, 1000); return delay; }, maxRetriesPerRequest: 20, + connectTimeout: 10000, + commandTimeout: 5000, };
1-40: Consider adding health checking capability.For systems that need health monitoring, you might want to provide a simple method to check if the Redis connection is healthy. This could be exposed as part of the returned client.
internal-packages/run-engine/src/engine/index.ts (2)
145-150: Consider adding recovery logic for critical Redis errors.While the error logging is good, consider adding some recovery logic or escalation for critical Redis errors that might require intervention. For example, you might want to trigger alerts for persistent connection issues or implement a circuit breaker pattern.
onError: (error) => { this.logger.error(`RunLock redis client error:`, { error, keyPrefix: options.runLock.redis.keyPrefix, }); + + // Implement additional handling for persistent errors + if (this.redisErrorCount === undefined) this.redisErrorCount = 0; + this.redisErrorCount++; + + // Alert on persistent errors + if (this.redisErrorCount > 10) { + // Could trigger an alert or notification here + this.logger.critical("Persistent Redis connection errors detected", { + errorCount: this.redisErrorCount, + keyPrefix: options.runLock.redis.keyPrefix, + }); + // Reset counter to avoid flooding alerts + this.redisErrorCount = 0; + } },
2513-2514: Ensure Redis connections are properly closed.The
quit()method on Redis clients is being called in thequit()method of the RunEngine class, which is good. However, consider adding a check to ensure the client exists before trying to quit.// This is just a failsafe -await this.runLockRedis.quit(); +if (this.runLockRedis) { + try { + await this.runLockRedis.quit(); + } catch (error) { + this.logger.warn("Error while quitting Redis connection", { error }); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
apps/webapp/package.json(2 hunks)apps/webapp/tsconfig.json(1 hunks)internal-packages/redis-worker/package.json(2 hunks)internal-packages/redis-worker/src/queue.ts(2 hunks)internal-packages/redis-worker/src/worker.test.ts(2 hunks)internal-packages/redis-worker/src/worker.ts(2 hunks)internal-packages/redis-worker/tsconfig.json(1 hunks)internal-packages/redis/README.md(1 hunks)internal-packages/redis/package.json(1 hunks)internal-packages/redis/src/index.ts(1 hunks)internal-packages/redis/tsconfig.json(1 hunks)internal-packages/run-engine/package.json(1 hunks)internal-packages/run-engine/src/engine/index.ts(2 hunks)internal-packages/run-engine/src/engine/locking.test.ts(2 hunks)internal-packages/run-engine/src/run-queue/index.test.ts(6 hunks)internal-packages/run-engine/src/run-queue/index.ts(2 hunks)internal-packages/run-engine/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- internal-packages/redis/README.md
- internal-packages/redis/tsconfig.json
- internal-packages/redis/package.json
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (34)
internal-packages/run-engine/package.json (1)
8-8: Good addition of the Redis client factory dependency.Adding the
@internal/redispackage as a dependency supports the refactoring to use a centralized Redis client factory with improved error handling and sensible defaults.apps/webapp/package.json (2)
55-55: Good addition of the Redis client factory dependency.Adding the
@internal/redispackage as a dependency allows the webapp to use the centralized Redis client creation with improved error handling.
262-262: Added proper EOF newline.Adding the trailing newline at the end of the file follows good file formatting practices.
internal-packages/redis-worker/package.json (2)
10-10: Good addition of the Redis client factory dependency.Adding the
@internal/redispackage as a dependency enables the redis-worker to use the centralized Redis client factory with consistent error handling and default configuration.
27-27: Added proper EOF newline.Adding the trailing newline at the end of the file follows good file formatting practices.
internal-packages/run-engine/src/engine/locking.test.ts (3)
1-1: Good import of the Redis client factory.Importing the
createRedisClientfunction from the newly created@internal/redispackage aligns with the PR objective to centralize Redis client creation.
8-8: Good refactoring to use the Redis client factory.Replacing direct Redis instantiation with the factory function
createRedisClientis a positive change that will benefit from:
- Consistent error handling across all Redis client creations
- Standardized default configurations
- Centralized management of Redis client options
This refactoring maintains the same interface while improving the underlying implementation.
Also applies to: 26-26
1-48:✅ Verification successful
Verify Redis client cleanup during test failures.
The tests correctly call
redis.quit()in thefinallyblock, which is good practice for resource cleanup. However, it's worth verifying that the newcreateRedisClientfunction doesn't change how connections are managed.
🏁 Script executed:
#!/bin/bash # Check if the createRedisClient function handles connection failures gracefully # and if it implements any additional cleanup mechanisms beyond what's in the tests # First, look for the implementation of createRedisClient echo "Looking for createRedisClient implementation..." rg -t ts "export\s+function\s+createRedisClient" --after-context=20Length of output: 2568
Redis Client Cleanup and Management Verified
The tests correctly invoke
redis.quit()in thefinallyblock, ensuring that connection cleanup is explicit. I confirmed that thecreateRedisClientimplementation ininternal-packages/redis/src/index.tsbehaves as expected in test environments (using theprocess.env.VITESTflag to simply swallow errors) and does not change connection management. No additional cleanup mechanism interferes with the tests’ resource management.internal-packages/run-engine/tsconfig.json (1)
22-23: Well-organized TypeScript path mappings addedThe path mappings for the new
@internal/redispackage are correctly configured, following the established pattern in the project for internal packages.internal-packages/redis-worker/src/worker.test.ts (2)
8-8: Appropriate import for the new Redis client factoryThe import statement for
createRedisClientfrom the new@internal/redispackage is correctly placed.
245-249: LGTM! Standardized Redis client creation with improved error handlingThe direct Redis instantiation is replaced with the
createRedisClientfactory function, providing consistency with the rest of the codebase and potentially better error handling.internal-packages/run-engine/src/run-queue/index.ts (3)
24-24: Import for the new Redis client factory added correctlyThe import for
createRedisClientis properly placed at the top of the file with other imports.
75-82: Excellent enhancement: Redis client with proper error handlingThis change replaces direct Redis instantiation with the
createRedisClientfactory, adding robust error handling that logs Redis client errors. This is a significant improvement for debugging and monitoring in production environments.
88-95: Consistent error handling for Redis subscriber clientThe subscriber Redis client now uses the same factory function with similar error handling, ensuring consistent behavior between the main client and the subscriber.
internal-packages/run-engine/src/run-queue/index.test.ts (6)
10-10: Proper import of the Redis client factoryThe import statement for
createRedisClientis correctly placed.
472-472: Standardized Redis client creation in testsThe test now uses the
createRedisClientfactory function instead of direct Redis instantiation, maintaining consistency with the implementation code.
602-602: Consistent use of Redis client factoryContinued consistency in using the
createRedisClientfactory across multiple tests.
693-693: Maintained consistency in Redis client creationThe standardized approach to Redis client creation is maintained throughout all test cases.
807-807: Consistent application of Redis client factory patternThe consistent application of the
createRedisClientpattern throughout the test suite helps ensure that all Redis clients are created with the same configuration and error handling.
862-866: Well-implemented Redis client for redrive testingThe Redis client for testing redrive functionality is properly created using the new factory function, maintaining consistency with the other Redis client instantiations in the tests.
internal-packages/redis-worker/src/worker.ts (2)
12-12: Good import of the new Redis client factory.The addition of the import from
@internal/redisallows for more standardized Redis client creation.
112-119: Great implementation of robust error handling for Redis client.The refactoring from direct Redis instantiation to using the
createRedisClientfactory function with an error handler is a significant improvement. This approach:
- Centralizes Redis client creation logic
- Provides explicit error handling through the onError callback
- Logs Redis client errors with helpful context including the keyPrefix
The error handler will help with debugging Redis connection issues in production.
apps/webapp/tsconfig.json (1)
41-43: Correctly configured path aliases for the new Redis package.The path mappings are properly configured to allow importing from the new
@internal/redispackage. This follows the project's established pattern for internal packages.internal-packages/redis-worker/tsconfig.json (1)
22-23: Correctly configured path aliases for Redis package dependencies.The path mappings are properly configured to allow the redis-worker package to import from the new
@internal/redispackage. This follows the project's established pattern for dependency resolution.internal-packages/redis-worker/src/queue.ts (3)
1-1: Good import of the Redis client factory function.Importing
createRedisClientfrom the new internal Redis package will help standardize Redis client creation across the codebase.
56-74: Excellent refactoring of Redis client creation with proper error handling.The refactoring to use
createRedisClientprovides several benefits:
- Consistent Redis client creation pattern across the codebase
- Proper error handling with detailed error logging
- Preservation of the important Redis configuration:
- Key prefix with queue name
- Retry strategy
- Max retries per request
The error handler provides valuable context by including the original keyPrefix in the error logs, which will help with debugging Redis issues in production.
67-72: Good error handling with contextual logging.The error handler provides useful context by including both the error object and the Redis keyPrefix. This will be helpful for diagnosing Redis connection issues in production.
internal-packages/redis/src/index.ts (5)
1-2: Good choice of importing specific types from 'ioredis'.The specific import of
RedisandRedisOptionstypes from 'ioredis' follows best practices for TypeScript imports.
4-10: Well-structured default Redis configuration with sensible retry values.The default retry strategy using exponential backoff with a cap at 1000ms and a maximum of 20 retries per request represents a good balance between resilience and avoiding excessive resource consumption. This follows Redis connection best practices by not giving up too quickly on transient errors while preventing infinite retry loops.
14-21: LGTM! Well-designed function signature.The function provides good flexibility by accepting both Redis configuration options and custom error handlers. The implementation correctly merges the default options with user-provided options.
23-29: Good test environment handling.The special case for Vitest environment is important to prevent test failures due to Redis connection errors. This is a good practice for making tests more reliable.
31-37: Robust error handling with fallback.The error handling logic first tries to use the user-provided handler if available, then falls back to logging the error with helpful context. This provides flexibility while ensuring errors aren't silently dropped.
internal-packages/run-engine/src/engine/index.ts (2)
1-1: Good modularization by importing the Redis client factory.The import of
createRedisClientfrom the newly created@internal/redispackage demonstrates good adherence to modular design principles.
139-152: Improved Redis client initialization with proper error handling.The updated code correctly uses the new
createRedisClientfunction with appropriate error handling configuration. This change centralizes Redis client creation logic and ensures consistent error handling across the application.The error handler logs useful context including the keyPrefix, which will help with debugging Redis issues.
@internal/redis package for creating Redis clients with sensible defaults and error handling
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests