fix(core): Reduce large relay message payloads to protect Redis #12342

ivov · 2024-12-23T10:10:08Z

Summary

It is possible for a manual execution in scaling mode to cause so much data to flow through Redis that it becomes unresponsive. Specifically, the nodeExecuteAfter and executionFinished events may carry arbitrarily large payloads.

In this PR we replace payloads above a 5 MB limit with placeholders that signal the UI to direct the user to view the full payload in execution history instead. Events with payloads below the size limit continue to be sent and displayed as usual.

In future, we can override these placeholders with the full execution data fetched from the DB at the end of the execution. Since we are already planning to refactor execution data handling on the client in the near future, that change is out of scope for this PR.

Testing

Start a main in scaling mode with OFFLOAD_MANUAL_EXECUTIONS_TO_WORKERS=true
Start a worker
Monitor Redis: docker exec -it <container> redis-cli monitor
Start an ad hoc server (see Slack) at localhost:3000 serving a 30 MB payload
Import workflow (see Slack), run it manually and call the test webhook
Verify that worker is processing manual execution and relaying to main
Verify that items after the Fetch large payload node have been trimmed
Verify in execution history that those items are displayed in full

Recording: https://share.cleanshot.com/GCrCsPXw

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/PAY-2403/reduce-large-relay-message-payloads-to-protect-redis

Review / Merge checklist

PR title and summary are descriptive.
~~Docs updated or follow-up ticket created~~
~~Tests included~~
~~PR labeled with release/backport~~

…uce-large-relay-message-payloads-to-protect-redis

codecov · 2024-12-23T10:37:44Z

Codecov Report

Attention: Patch coverage is 27.08333% with 35 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/cli/src/push/index.ts	24.00%	19 Missing ⚠️
packages/editor-ui/src/components/RunData.vue	20.00%	16 Missing ⚠️

📢 Thoughts on this report? Let us know!

tomi

Works nicely 👏 Couple comments about the implementation

tomi · 2024-12-30T14:48:29Z

packages/editor-ui/src/plugins/i18n/locales/en.json

@@ -1655,6 +1655,9 @@
 	"runData.aiContentBlock.tokens": "{count} Tokens",
 	"runData.aiContentBlock.tokens.prompt": "Prompt:",
 	"runData.aiContentBlock.tokens.completion": "Completion:",
+	"runData.trimmedData.title": "Data too large to display",
+	"runData.trimmedData.message": "The data is too large to be shown here. View the full details in 'Executions'.",


Suggested change

"runData.trimmedData.message": "The data is too large to be shown here. View the full details in 'Executions'.",

"runData.trimmedData.message": "The data is too large to be shown here. View the full details in 'Executions' tab.",

Maybe mention that it's a tab?

Please see 6723b15

tomi · 2024-12-30T14:50:00Z

packages/cli/src/constants.ts

+	main: [
+		[
+			{
+				json: { isTrimmedManualExecutionDataItem: true },


Maybe we could use a __ prefix something to mark it more clearly as something "internal"

Suggested change

json: { isTrimmedManualExecutionDataItem: true },

json: { __isTrimmedManualExecutionDataItem: true },

Please see 2750822

tomi · 2024-12-30T17:58:59Z

packages/cli/src/constants.ts

+ * Stand-in for a manual execution data item too large to be sent live via pubsub.
+ * This signals to the client to direct the user to the execution history.
+ */
+export const TRIMMED_TASK_DATA_CONNECTIONS: ITaskDataConnections = {


Maybe we should move this to either core or workflow package so we could share the logic with FE instead of relying on magic strings.

Please see 17ff5a1

tomi · 2024-12-30T18:03:20Z

packages/cli/src/push/index.ts

+		if (this.shouldRelayViaPubSub(pushRef)) {
+			this.relayViaPubSub(pushMsg, pushRef);


IMHO this logic shouldn't be in the Push class. Now it ties the logic to implementation details of the scaling mode, where as Push should only be concerned about communicating via the websocket (or SSE). Now since this has already been here before we don't have to move it elsewhere in this PR. I just wanted to mention this for future

Agree, we need to work on the lifecycle hooks first to unlock this refactor.

tomi · 2024-12-30T18:22:11Z

packages/cli/src/push/index.ts

+		if (type === 'nodeExecuteAfter') pushMsgCopy.data.data.data = TRIMMED_TASK_DATA_CONNECTIONS;
+		if (type === 'executionFinished') pushMsgCopy.data.rawData = ''; // prompt client to fetch from DB


Suggested change

if (type === 'nodeExecuteAfter') pushMsgCopy.data.data.data = TRIMMED_TASK_DATA_CONNECTIONS;

if (type === 'executionFinished') pushMsgCopy.data.rawData = ''; // prompt client to fetch from DB

if (type === 'nodeExecuteAfter') pushMsgCopy.data.data.data = TRIMMED_TASK_DATA_CONNECTIONS;

else if (type === 'executionFinished') pushMsgCopy.data.rawData = ''; // prompt client to fetch from DB

Please see ba205b6

tomi · 2024-12-30T18:28:59Z

packages/cli/src/push/index.ts

+		const maxMb = toMb(MAX_PAYLOAD_SIZE_BYTES);
+		const { type } = pushMsgCopy;
+
+		this.logger.warn(`Size of "${type}" (${eventMb} MB) exceeds max size ${maxMb} MB. Trimming...`);


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

100%, same as here

tomi · 2024-12-30T18:29:23Z

packages/editor-ui/src/components/RunData.vue

+					{{ i18n.baseText('runData.trimmedData.message') }}
+				</N8nText>
+				<N8nButton size="small">
+					<a :href="`/workflow/${workflowsStore.workflowId}/executions`">


This seems to do a full navigation that reload the entire page. Would it be possible to use vue router link?

Unfortunately the styling of N8nLink does not match what Giulio requested:

The button is too big and the color is not available in the N8nLink themes.

In that case we can use the N8nRoute component directly and style it how we wish

Thank you, please see 33fc9f8

ivov added 2 commits December 23, 2024 11:01

fix(core): Reduce large relay message payloads to protect Redis

908e5ff

Merge branch 'offload-manual-executions-to-workers' into pay-2403-red…

f2bc54a

…uce-large-relay-message-payloads-to-protect-redis

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Dec 23, 2024

tomi reviewed Dec 30, 2024

View reviewed changes

ivov added 4 commits December 31, 2024 09:17

Better wording for the execution data trimmed message

6723b15

Use dunder to mark as internal

2750822

if → else if

ba205b6

Share stand-in key

17ff5a1

ivov requested a review from tomi December 31, 2024 08:51

ivov added 2 commits December 31, 2024 10:18

Fix lint

4a84887

Use N8nRoute + close NDV

33fc9f8

tomi approved these changes Dec 31, 2024

View reviewed changes

ivov merged commit f52802a into offload-manual-executions-to-workers Dec 31, 2024
31 checks passed

ivov deleted the pay-2403-reduce-large-relay-message-payloads-to-protect-redis branch December 31, 2024 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Reduce large relay message payloads to protect Redis #12342

fix(core): Reduce large relay message payloads to protect Redis #12342

ivov commented Dec 23, 2024

codecov bot commented Dec 23, 2024 •

edited

Loading

tomi left a comment

tomi Dec 30, 2024

ivov Dec 31, 2024

tomi Dec 30, 2024

ivov Dec 31, 2024

tomi Dec 30, 2024

ivov Dec 31, 2024

tomi Dec 30, 2024

ivov Dec 31, 2024

tomi Dec 30, 2024

ivov Dec 31, 2024

tomi Dec 30, 2024

ivov Dec 31, 2024

tomi Dec 30, 2024

ivov Dec 31, 2024

tomi Dec 31, 2024

ivov Dec 31, 2024

	"runData.trimmedData.message": "The data is too large to be shown here. View the full details in 'Executions'.",
	"runData.trimmedData.message": "The data is too large to be shown here. View the full details in 'Executions' tab.",

	json: { isTrimmedManualExecutionDataItem: true },
	json: { __isTrimmedManualExecutionDataItem: true },

		if (this.shouldRelayViaPubSub(pushRef)) {
		this.relayViaPubSub(pushMsg, pushRef);

		if (type === 'nodeExecuteAfter') pushMsgCopy.data.data.data = TRIMMED_TASK_DATA_CONNECTIONS;
		if (type === 'executionFinished') pushMsgCopy.data.rawData = ''; // prompt client to fetch from DB

fix(core): Reduce large relay message payloads to protect Redis #12342

fix(core): Reduce large relay message payloads to protect Redis #12342

Conversation

ivov commented Dec 23, 2024

Summary

Testing

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

tomi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 23, 2024 •

edited

Loading