feat(core): Cancel runner task on timeout in external mode #12101

ivov · 2024-12-09T10:46:48Z

codecov · 2024-12-09T12:11:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

tomi

Couple changes I'd like to see. Also, we should add N8N_RUNNERS_TASK_TIMEOUT to docker/images/n8n/n8n-task-runners.json

packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts

tomi · 2024-12-10T07:56:00Z

packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts

+					const vmResult = (await runInNewContext(
+						`globalThis.global = globalThis; module.exports = async function VmCodeWrapper() {${settings.code}\n}()`,
+						context,
+					)) as TaskResultData['result'];


Although it doesn't cover all cases, we could pass the timeout param to the runInNewContext. That way it would get canceled eventually as well. Same for runForEachItem

Today I learned 🙏🏻

ce94ef5

packages/@n8n/task-runner/src/task-runner.ts

tomi · 2024-12-10T08:12:19Z

packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts

-					context,
-				)) as INodeExecutionData | undefined;
+				let result = await new Promise<INodeExecutionData | undefined>((resolve, reject) => {
+					signal.addEventListener(


Since this runs in a loop, we are adding a new event listener on each iteration, and it could be a thousand of these. I think it would be good in this case to remove the event listener once we are done. Also then we don't get these warnings:

2024/12/10 10:09:30 ERROR [Runner] (node:28928) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. MaxListeners is 10. Use events.setMaxListeners() to increase limit

~~Interesting, I thought this was covered by { once: true }~~

Edit: once removes the listener only if called.

8b75bdf

tomi

Looking good now, one small comment 👍

tomi · 2024-12-10T10:34:30Z

packages/@n8n/task-runner/src/js-task-runner/__tests__/task-runner.test.ts

@@ -89,4 +89,65 @@ describe('TestRunner', () => {
 			).toThrowError(/Invalid URL/);
 		});
 	});
+
+	describe('taskCancelled', () => {


tomi · 2024-12-10T10:51:43Z

packages/@n8n/task-runner/src/task-runner.ts

@@ -298,6 +300,20 @@ export abstract class TaskRunner extends EventEmitter {
 		}
 		task.cancelled = true;

+		for (const [requestId, request] of this.dataRequests.entries()) {
+			if (request.taskId === taskId) {
+				request.reject(new ApplicationError('Task cancelled'));


We receive a cancellation reason in the message. We could include that here. Also could use some more fine grained error than generic ApplicationError

tomi · 2024-12-10T10:52:01Z

packages/@n8n/task-runner/src/task-runner.ts

+
+		for (const [requestId, request] of this.nodeTypesRequests.entries()) {
+			if (request.taskId === taskId) {
+				request.reject(new ApplicationError('Task cancelled'));


tomi

Thank you for addressing the feedback 💘

cypress · 2024-12-10T11:35:07Z

n8n Run #8265

Run Properties: Passed #8265 • ffaba80f79: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*

Project	`n8n`
Branch Review	`cat-405-task-that-times-out-is-left-running-on-task-runner`
Run status	`Passed #8265`
Run duration	`04m 28s`
Commit	`ffaba80f79: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*`
Committer	`Iván Ovejero`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`480`
View all changes introduced in this branch ↗︎

github-actions · 2024-12-10T11:35:13Z

✅ All Cypress E2E specs passed

janober · 2024-12-10T13:52:11Z

Got released with [email protected]

WIP

c7c587d

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Dec 9, 2024

ivov added 6 commits December 9, 2024 12:23

Send msg instead

eedf67c

Adjust phrasing

8fb7abc

Fix typecheck

7f0f8fb

Remove commented out code

2d185fc

Add TODO

896549d

Fix test

06261de

ivov added 3 commits December 9, 2024 13:12

Adjust msg based on mode

8c885d3

Improve readability

00d7fb8

Add timeout runner-side

b98f1cc

ivov mentioned this pull request Dec 9, 2024

feat: Add task timeout env var n8n-io/task-runner-launcher#54

Merged

Fix typecheck

828d59f

ivov marked this pull request as ready for review December 9, 2024 15:00

tomi requested changes Dec 10, 2024

View reviewed changes

tomi reviewed Dec 10, 2024

View reviewed changes

ivov added 8 commits December 10, 2024 09:46

Better syntax

4efd5f6

Pass timeout to runInNewContext

ce94ef5

Move clearTimeout to finally block

f4a2449

Handle cancellation at request level

4c1f5ff

Add N8N_RUNNERS_TASK_TIMEOUT to config file

731068e

Remove event listener

8b75bdf

Add task broker cancellation test

1d2ee31

Add test for TaskRunner.taskCancelled

69de17d

ivov requested a review from tomi December 10, 2024 09:42

tomi reviewed Dec 10, 2024

View reviewed changes

tomi added the release/backport Changes that need to be backported to older releases. label Dec 10, 2024

Use TaskCancelledError with reason

ffaba80

tomi approved these changes Dec 10, 2024

View reviewed changes

ivov merged commit addb4fa into master Dec 10, 2024
37 checks passed

ivov deleted the cat-405-task-that-times-out-is-left-running-on-task-runner branch December 10, 2024 11:50

tomi pushed a commit that referenced this pull request Dec 10, 2024

feat(core): Cancel runner task on timeout in external mode (#12101)

881a77d

github-actions bot mentioned this pull request Dec 10, 2024

🚀 Release 1.70.4 #12129

Merged

tomi pushed a commit that referenced this pull request Dec 10, 2024

feat(core): Cancel runner task on timeout in external mode (#12101)

f18263b

github-actions bot mentioned this pull request Dec 10, 2024

🚀 Release 1.71.2 #12130

Merged

janober added the Released label Dec 10, 2024

github-actions bot mentioned this pull request Dec 11, 2024

🚀 Release 1.72.0 #12158

Merged

riascho pushed a commit that referenced this pull request Jan 14, 2025

feat(core): Cancel runner task on timeout in external mode (#12101)

e9ccc71

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Cancel runner task on timeout in external mode #12101

feat(core): Cancel runner task on timeout in external mode #12101

ivov commented Dec 9, 2024

codecov bot commented Dec 9, 2024 •

edited

Loading

tomi left a comment

tomi Dec 10, 2024

ivov Dec 10, 2024

tomi Dec 10, 2024

ivov Dec 10, 2024 •

edited

Loading

tomi left a comment

tomi Dec 10, 2024

tomi Dec 10, 2024

ivov Dec 10, 2024

tomi Dec 10, 2024

ivov Dec 10, 2024

tomi left a comment

cypress bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

janober commented Dec 10, 2024

feat(core): Cancel runner task on timeout in external mode #12101

feat(core): Cancel runner task on timeout in external mode #12101

Conversation

ivov commented Dec 9, 2024

codecov bot commented Dec 9, 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

ivov Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

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

tomi left a comment

Choose a reason for hiding this comment

cypress bot commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

janober commented Dec 10, 2024

codecov bot commented Dec 9, 2024 •

edited

Loading

ivov Dec 10, 2024 •

edited

Loading