fix(core): Handle Redis disconnects gracefully #11007
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
n8n-assistant
bot
added
core
tomi
reviewed
Sep 30, 2024
| if (cumulativeTimeout > this.config.maxTimeout) { | ||
| const maxTimeout = Math.round(this.config.maxTimeout / 1000) + 's'; | ||
| this.logger.error(`Unable to connect to Redis after trying to connect for ${maxTimeout}`); | ||
| this.logger.error('Exiting process due to Redis connection error'); | ||
| process.exit(1); |
There was a problem hiding this comment.
Ideally we should allow graceful exit of other parts of the app as well. Otherwise we might leave some resources in improper state, like DB connections etc. Just mentioning this, no need to act on it here
| @@ -156,4 +190,33 @@ export class RedisClientService { | |||
| return { host, port: parseInt(port) }; | |||
| }); | |||
| } | |||
|
|
|||
| @Debounce(1000) | |||
There was a problem hiding this comment.
Why do we need the debounce here?
There was a problem hiding this comment.
Debouncing prevents all six clients spamming the logs with their calls to retryStrategy in quick succession, i.e. we consolidate all logs from those calls on connection-lost and connection-recovered from six to one.
export type RedisClientType = N8nRedisClientType | BullRedisClientType;
/**
* Redis client used by n8n.
*
* - `subscriber(n8n)` to listen for messages from scaling mode pubsub channels
* - `publisher(n8n)` to send messages into scaling mode pubsub channels
* - `cache(n8n)` for caching operations (variables, resource ownership, etc.)
*/
type N8nRedisClientType = 'subscriber(n8n)' | 'publisher(n8n)' | 'cache(n8n)';
/**
* Redis client used internally by Bull. Suffixed with `(bull)` at `ScalingService.setupQueue`.
*
* - `subscriber(bull)` for event listening
* - `client(bull)` for general queue operations
* - `bclient(bull)` for blocking operations when processing jobs
*/
type BullRedisClientType = 'subscriber(bull)' | 'client(bull)' | 'bclient(bull)';
n8n
|
||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
master
|
| Run status |
|
| Run duration | 04m 24s |
| Commit |
|
| Committer | Iván Ovejero |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
435
|
| View all changes introduced in this branch ↗︎ | |
|
✅ All Cypress E2E specs passed |
|
Got released with |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently a Redis disconnect triggers all manner of logs from multiple related services and from all six Redis clients simultaneously. This PR handles Redis this scenario more gracefully.