fix: [FLOWISE-1] Unstable Redis, ECONNREFUSED 127.0.0.1 #5493
Conversation
Summary of ChangesHello @natan-hoppe-workday, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and maintainability of Redis interactions within the application by consolidating all Redis connection and initialization logic into a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a RedisConnector class to centralize and improve Redis connection handling, which is a great initiative. The refactoring correctly applies this new connector across CachePool, SessionPersistance, and rateLimit. However, the implementation of RedisConnector has a critical flaw in how it provides the Redis client instance synchronously, which will lead to runtime errors. There's also an issue with the reconnection logic's implementation not matching its documented intent. My review focuses on these key areas to ensure the connector is robust and reliable.
| public getRedisClient(): Redis { | ||
| // Trigger async init if not yet started | ||
| void this.init() | ||
| return this.redis | ||
| } |
There was a problem hiding this comment.
This method has a critical race condition. It calls this.init() which starts an asynchronous initialization process, and then immediately returns this.redis. The this.redis property is only assigned inside an async IIFE within init(), so it will be undefined when this method returns. This will cause TypeError exceptions in all places where this method is used, such as in CachePool, SessionPersistance, and RateLimiterManager.
To fix this, the Redis instance must be created synchronously before this method returns. The asynchronous part (like await this.redis.connect()) can remain in the promise.
A possible approach:
- Refactor
init()andinitializeQueueMode()to synchronously create theRedisinstance and assign it tothis.redis. - The
initPromiseshould then only handle the asynchronous connection logic (i.e.,await this.redis.connect()).
With such a change, getRedisClient() can safely return this.redis because it will be available synchronously.
There was a problem hiding this comment.
The race condition is addressed via the ready() method which must be called during async requests as documented.
For example:
const connector = new RedisConnector()
await connector.ready() // fully waits for Redis init
const redis = connector.getRedisClient()
| public getRedisConnection(): Record<string, unknown> { | ||
| // Trigger async init if not yet started | ||
| void this.init() | ||
| return this.connection | ||
| } |
There was a problem hiding this comment.
Similar to getRedisClient(), this method has a critical race condition. It returns this.connection which is assigned asynchronously within init(). This will result in undefined being returned, causing failures in consumers like RateLimiterManager when setting up bullmq.
The fix is the same as for getRedisClient(): ensure this.connection is assigned synchronously during the initialization phase.
There was a problem hiding this comment.
The race condition is addressed via the ready() method which must be called during async requests as documented.
For example:
const connector = new RedisConnector()
await connector.ready() // fully waits for Redis init
const redis = connector.getRedisClient()
| private connectOnError(err: Error): number { | ||
| logger.error(`[server]: Redis connection error - ${err.message}`) | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
The comment on line 136 states that this function does not retry the failed command. However, according to the ioredis documentation, returning 1 causes ioredis to retry the failed command. To reconnect without retrying the command, the function should return 2.
This mismatch could lead to unexpected behavior, especially when dealing with non-idempotent commands. Please update the return value to align with the intended behavior described in the comment.
| private connectOnError(err: Error): number { | |
| logger.error(`[server]: Redis connection error - ${err.message}`) | |
| return 1 | |
| } | |
| private connectOnError(err: Error): number { | |
| logger.error(`[server]: Redis connection error - ${err.message}`) | |
| return 2 | |
| } |
There was a problem hiding this comment.
Wrong feedback.
The source code and documentation confirm the number 1 should be used for reconnection without command retry.
| @@ -0,0 +1,179 @@ | |||
| import { InternalFlowiseError } from '../../server/src/errors/internalFlowiseError' | |||
There was a problem hiding this comment.
in components, we can't import stuff from server using relative path like this - it will likely cause circular dependency. Relative path imports can only be used if you're importing from the same components folder - this is how it works in a monorepo
search for isPathTraversal you can see how a common function can be created in components, and exposed via index.ts, then later used in files inside server
Since you are trying to create a singleton Redis instance that can be used throughout the application, I recommend to move this to server/src
|
We also need to test if the modified pieces related to Redis (cache, session, rate limit, etc) works on dev server |
3 participants
Description
Added dedicated class
RedisConnectorto initialize and connect to Redis while supporting sync and async usages, as well reconnections in case of failures.Updated the following files with its usage: