-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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.
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
fix(core): Reduce large relay message payloads to protect Redis #12342
Changes from all commits
908e5ff
f2bc54a
6723b15
2750822
ba205b6
17ff5a1
4a84887
33fc9f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,16 +3,19 @@ import type { Application } from 'express'; | |
import { ServerResponse } from 'http'; | ||
import type { Server } from 'http'; | ||
import { InstanceSettings } from 'n8n-core'; | ||
import { deepCopy } from 'n8n-workflow'; | ||
import type { Socket } from 'net'; | ||
import { Container, Service } from 'typedi'; | ||
import { parse as parseUrl } from 'url'; | ||
import { Server as WSServer } from 'ws'; | ||
|
||
import { AuthService } from '@/auth/auth.service'; | ||
import config from '@/config'; | ||
import { TRIMMED_TASK_DATA_CONNECTIONS } from '@/constants'; | ||
import type { User } from '@/databases/entities/user'; | ||
import { OnShutdown } from '@/decorators/on-shutdown'; | ||
import { BadRequestError } from '@/errors/response-errors/bad-request.error'; | ||
import { Logger } from '@/logging/logger.service'; | ||
import { Publisher } from '@/scaling/pubsub/publisher.service'; | ||
import { TypedEmitter } from '@/typed-emitter'; | ||
|
||
|
@@ -27,6 +30,12 @@ type PushEvents = { | |
|
||
const useWebSockets = config.getEnv('push.backend') === 'websocket'; | ||
|
||
/** | ||
* Max allowed size of a push message in bytes. Events going through the pubsub | ||
* channel are trimmed if exceeding this size. | ||
*/ | ||
const MAX_PAYLOAD_SIZE_BYTES = 5 * 1024 * 1024; // 5 MiB | ||
|
||
/** | ||
* Push service for uni- or bi-directional communication with frontend clients. | ||
* Uses either server-sent events (SSE, unidirectional from backend --> frontend) | ||
|
@@ -43,8 +52,10 @@ export class Push extends TypedEmitter<PushEvents> { | |
constructor( | ||
private readonly instanceSettings: InstanceSettings, | ||
private readonly publisher: Publisher, | ||
private readonly logger: Logger, | ||
) { | ||
super(); | ||
this.logger = this.logger.scoped('push'); | ||
|
||
if (useWebSockets) this.backend.on('message', (msg) => this.emit('message', msg)); | ||
} | ||
|
@@ -91,25 +102,8 @@ export class Push extends TypedEmitter<PushEvents> { | |
} | ||
|
||
send(pushMsg: PushMessage, pushRef: string) { | ||
const { isWorker, isMultiMain } = this.instanceSettings; | ||
|
||
/** | ||
* In scaling mode, in single- or multi-main setup, in a manual execution, | ||
* a worker relays execution lifecycle events to all mains. Only the main | ||
* who holds the session for the execution will push to the frontend who | ||
* commissioned the execution. | ||
* | ||
* In scaling mode, in multi-main setup, in a manual webhook execution, if | ||
* the main who handles a webhook is not the main who created the webhook, | ||
* the handler main relays execution lifecycle events to all mains. Only | ||
* the main who holds the session for the execution will push events to | ||
* the frontend who commissioned the execution. | ||
*/ | ||
if (isWorker || (isMultiMain && !this.hasPushRef(pushRef))) { | ||
void this.publisher.publishCommand({ | ||
command: 'relay-execution-lifecycle-event', | ||
payload: { ...pushMsg, pushRef }, | ||
}); | ||
if (this.shouldRelayViaPubSub(pushRef)) { | ||
this.relayViaPubSub(pushMsg, pushRef); | ||
Comment on lines
+105
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO this logic shouldn't be in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, we need to work on the lifecycle hooks first to unlock this refactor. |
||
return; | ||
} | ||
|
||
|
@@ -124,6 +118,66 @@ export class Push extends TypedEmitter<PushEvents> { | |
onShutdown() { | ||
this.backend.closeAllConnections(); | ||
} | ||
|
||
/** | ||
* Whether to relay a push message via pubsub channel to other instances, | ||
* instead of pushing the message directly to the frontend. | ||
* | ||
* This is needed in two scenarios: | ||
* | ||
* In scaling mode, in single- or multi-main setup, in a manual execution, a | ||
* worker has no connection to a frontend and so relays to all mains lifecycle | ||
* events for manual executions. Only the main who holds the session for the | ||
* execution will push to the frontend who commissioned the execution. | ||
* | ||
* In scaling mode, in multi-main setup, in a manual webhook execution, if | ||
* the main who handles a webhook is not the main who created the webhook, | ||
* the handler main relays execution lifecycle events to all mains. Only | ||
* the main who holds the session for the execution will push events to | ||
* the frontend who commissioned the execution. | ||
*/ | ||
private shouldRelayViaPubSub(pushRef: string) { | ||
const { isWorker, isMultiMain } = this.instanceSettings; | ||
|
||
return isWorker || (isMultiMain && !this.hasPushRef(pushRef)); | ||
} | ||
|
||
/** | ||
* Relay a push message via the `n8n.commands` pubsub channel, | ||
* reducing the payload size if too large. | ||
* | ||
* See {@link shouldRelayViaPubSub} for more details. | ||
*/ | ||
private relayViaPubSub(pushMsg: PushMessage, pushRef: string) { | ||
const eventSizeBytes = new TextEncoder().encode(JSON.stringify(pushMsg.data)).length; | ||
|
||
if (eventSizeBytes <= MAX_PAYLOAD_SIZE_BYTES) { | ||
void this.publisher.publishCommand({ | ||
command: 'relay-execution-lifecycle-event', | ||
payload: { ...pushMsg, pushRef }, | ||
}); | ||
return; | ||
} | ||
|
||
// too large for pubsub channel, trim it | ||
|
||
const pushMsgCopy = deepCopy(pushMsg); | ||
|
||
const toMb = (bytes: number) => (bytes / (1024 * 1024)).toFixed(0); | ||
const eventMb = toMb(eventSizeBytes); | ||
const maxMb = toMb(MAX_PAYLOAD_SIZE_BYTES); | ||
const { type } = pushMsgCopy; | ||
|
||
this.logger.warn(`Size of "${type}" (${eventMb} MB) exceeds max size ${maxMb} MB. Trimming...`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to also log the node that caused this. For that we would have to do the trimming already earlier in the chain to have that available There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 100%, same as here |
||
|
||
if (type === 'nodeExecuteAfter') pushMsgCopy.data.data.data = TRIMMED_TASK_DATA_CONNECTIONS; | ||
else if (type === 'executionFinished') pushMsgCopy.data.rawData = ''; // prompt client to fetch from DB | ||
|
||
void this.publisher.publishCommand({ | ||
command: 'relay-execution-lifecycle-event', | ||
payload: { ...pushMsgCopy, pushRef }, | ||
}); | ||
} | ||
} | ||
|
||
export const setupPushServer = (restEndpoint: string, server: Server, app: Application) => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should move this to either
core
orworkflow
package so we could share the logic with FE instead of relying on magic strings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see 17ff5a1