fix: Chat triggers don't work with the new partial execution flow #11952

despairblue · 2024-11-28T14:08:16Z

Summary

This fixes the chat trigger not working with the new execution flow.

Before the chat trigger would always trigger a partial execution, but now a partial execution requires a destination node.

This PR alleviates this by re-using the parts of the new flow that does not require a destination node.

This will be simplified once we can remove the old partial execution flow logic.

before:

pay-2166-before.mp4

after:

pay-2166-after.mp4

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/PAY-2166/chat-triggers-dont-work-with-the-new-partial-execution-flow

Review / Merge checklist

PR title and summary are descriptive. (conventions)
~~Docs updated or follow-up ticket created.~~
Tests included.
~~PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)~~

…assing 6 arguments I'll add 2 more arguments to the list

…o send a preferred trigger with or without data

codecov · 2024-11-28T14:34:24Z

Codecov Report

Attention: Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/core/src/index.ts	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

despairblue · 2024-11-29T13:28:15Z

packages/editor-ui/src/composables/useRunWorkflow.ts

 			if (
 				startNodeNames.length === 0 &&
 				'destinationNode' in options &&
 				options.destinationNode !== undefined
 			) {
 				executedNode = options.destinationNode;
 				startNodeNames.push(options.destinationNode);
-			} else if ('triggerNode' in options && 'nodeData' in options) {
+			} else if (options.triggerNode && options.nodeData) {


There was no need to use '...' in object as a guard and it made the type casting necessary. Changing this get's rid of the type casting below.

despairblue · 2024-11-29T13:30:48Z

packages/cli/src/webhooks/test-webhooks.ts

+	async needsWebhook(options: {
+		userId: string;
+		workflowEntity: IWorkflowDb;
+		additionalData: IWorkflowExecuteAdditionalData;
+		runData?: IRunData;
+		pushRef?: string;
+		destinationNode?: string;
+		preferredTrigger?: WorkflowRequest.ManualRunPayload['preferredTrigger'];
+	}) {


The number of arguments to this function were getting out of hand so I turned it into an object.

despairblue · 2024-11-29T13:33:33Z

packages/cli/src/workflow-runner.ts

+				// TODO: When the old partial execution logic is removed this block can
+				// be removed and the previous one can be merged into
+				// `workflowExecute.runPartialWorkflow2`.
+				// Partial executions then require either a destination node from which
+				// everything else can be derived, or a preferredTrigger with
+				// triggerData. Full Execution


This block is not necessary for the new partial execution flow. As soon as we default to the new flow and remove the old flow we can fold the code for handling preferred triggers into the partial execution function in WorkflowExecute

despairblue · 2024-11-29T13:33:56Z

packages/cli/src/workflow-runner.ts

@@ -203,6 +211,7 @@ export class WorkflowRunner {
 	}

 	/** Run the workflow in current process */
+	// eslint-disable-next-line complexity


This is necessary for now. Once we remove the old partial execution logic this function will be much simpler.

ivov

I'll come back later to test it!

packages/cli/src/webhooks/test-webhooks.ts

packages/cli/src/workflow-runner.ts

ivov · 2024-12-02T15:39:44Z

packages/cli/src/webhooks/test-webhooks.ts

+		// that trigger, not the other ones.
+		if (preferredTrigger) {
+			webhooks = webhooks.filter((w) => w.node === preferredTrigger.name);
+		}


Should this be an else if with the next check?

I don't follow.

This check is removing all but one webhook from the list if the preferred trigger was passed.

The next check is checking if the the list contains at least one trigger that is restartable (I think 🤔 )

I guess the question is, can it ever happen that a trigger to start triggers the restartable check? What exactly is w.webhookDescription.restartWebhook?

But if not worth it let's leave it :)

What exactly is w.webhookDescription.restartWebhook?

I don't know either tbh. Jan added this here: 5a179cd#diff-b386248ff00977749c873ed85821c241b773e9740d7e7adf94e05b73b350ed74

Which is a huge commit.

It seems this is for the Form, Wait, Slack and Gmail nodes.

I guess the question is, can it ever happen that a trigger to start triggers the restartable check?

I guess that would be fine still. I'm assuming this is to not listen for webhook for workflows that have been put to into waiting?

Which means you can't actually pick them as a trigger to start from.

At least for the the wait and form nodes this is only interesting for workflows that wait, probably for slack too, e.g. if you render a dialog and wait for the user to click a button. I'm not sure why the gmail node offers this though.

I guess this can be left until the split button is implemented.

cc @cstuncsik @CharlieKolb

ivov · 2024-12-02T15:45:40Z

packages/cli/src/workflow-runner.ts

+			} else if (data.preferredTrigger?.data && data.startNodes && !data.destinationNode) {
+				this.logger.debug(
+					`Execution ID ${executionId} had preferredTrigger. Starting from that trigger.`,
+					{ executionId },
+				);


(This is me having limited context.) What's the significance of the preferred trigger having data? If the preferred trigger has no data, the fact that it's preferred is irrelevant? Or all preferred triggers will always have data?

Some triggers have the ability to "make up" data. E.g. the schedule trigger must have a function that does that, I didn't check where it it though.

From what I could tell there are these scenarios:

trigger can make up data (schedule trigger)

trigger has pinned data

trigger waits for an external event (any webhook or polling trigger)

trigger asks everytime for the user to input custom data (probably just the chat trigger?)

The preferred trigger only needs data for 4.
In all other cases you can send the preferred trigger without data, which will be the bases for the split run workflow button that Charlie and Csaba have been working on during the hackmation. This PR is supposed to lay some ground work for them.

Thank you, much clearer now.

ivov · 2024-12-02T15:50:52Z

packages/cli/src/workflow-runner.ts

@@ -286,12 +295,54 @@ export class WorkflowRunner {
 					data.executionData,
 				);
 				workflowExecution = workflowExecute.processRunExecutionData(workflow);
+			} else if (data.preferredTrigger?.data && data.startNodes && !data.destinationNode) {
+				this.logger.debug(
+					`Execution ID ${executionId} had preferredTrigger. Starting from that trigger.`,


(This is mostly me trying to make sense of this.) I see all the other branches specify how much to execute:

has execution data (e.g. retry) → reuse that data

has no run data or has no start node → run it fullly

has run data or has start node → run it partially

But this new branch specifies where to start executing from. Should we stay consistent with the others? What would this branch be in the above terms? A "pseudo full" execution?

This is what think it eventually should be:

let executionData if (data.executionData) { executionData = data.executionData } else if (data.preferredTrigger) { const triggerData = data.triggerData ?? getTriggerDataFromTrigger(data.preferredTrigger) executionData = createExecutionDataFromTrigger(data.preferredTrigger, data.triggerData, data.workflowData) } else if (data.destinationNode) { executionData = recreateExecutionDataFromPartialExecution(data.destinationNode, data.workflowData, data.runData, data.pinData, ...etc) } // run with execution data

So you either start from old execution data, or from a trigger or we have the destination node and try to recreate the execution stack from that.

I don't even know if the block were I added the long TODO comment is used. I just don't want to remove it right now, but when we remove the old partial exec logic which comes with simplifying useRunWorkflow.ts to the point were I can be sure that the "Full Execution" branch is unused.

Thank you, that's clearer.

packages/cli/src/workflow-runner.ts

ivov

Tested and working! Nice job :)

github-actions · 2024-12-04T11:28:54Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

cypress · 2024-12-04T11:29:03Z

n8n Run #8207

Run Properties: Passed #8207 • f68c0d753a: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*

Project	`n8n`
Branch Review	`pay-2166-chat-triggers-dont-work-with-the-new-partial-execution-flow-try-2`
Run status	`Passed #8207`
Run duration	`04m 30s`
Commit	`f68c0d753a: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*`
Committer	`Danny Martini`
View all properties for this run ↗︎

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

github-actions · 2024-12-04T14:16:54Z

✅ All Cypress E2E specs passed

janober · 2024-12-11T14:25:18Z

Got released with [email protected]

despairblue added 3 commits November 28, 2024 09:00

refactor(core): Use an options object for needsWebhook instead of p…

60d81e2

…assing 6 arguments I'll add 2 more arguments to the list

refactor(core): group tests in desribe block

df26d52

fix: Fix chat trigger with partial executions v2 by allowing the FE t…

a78d8ce

…o send a preferred trigger with or without data

despairblue changed the base branch from master to extract-filter-disabled-nodes November 28, 2024 14:08

despairblue force-pushed the pay-2166-chat-triggers-dont-work-with-the-new-partial-execution-flow-try-2 branch from cb67955 to fc9a357 Compare November 28, 2024 14:09

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 Nov 28, 2024

Base automatically changed from extract-filter-disabled-nodes to master November 29, 2024 11:58

add test

4dbc563

despairblue force-pushed the pay-2166-chat-triggers-dont-work-with-the-new-partial-execution-flow-try-2 branch from fc9a357 to 4dbc563 Compare November 29, 2024 13:26

despairblue commented Nov 29, 2024

View reviewed changes

add clarifying comment

b4dc389

despairblue commented Nov 29, 2024

View reviewed changes

despairblue marked this pull request as ready for review November 29, 2024 13:34

ivov reviewed Dec 2, 2024

View reviewed changes

despairblue added 4 commits December 3, 2024 15:03

destructure options

02a4aa1

fix comment

c50ffbe

rename preferredTrigger to triggerToStartFrom

924294b

don't use runData from FE

b27590f

ivov previously approved these changes Dec 4, 2024

View reviewed changes

undo change that was committed by accident

f68c0d7

despairblue dismissed ivov’s stale review via f68c0d7 December 4, 2024 12:58

despairblue requested a review from ivov December 4, 2024 13:13

ivov approved these changes Dec 4, 2024

View reviewed changes

despairblue merged commit 2b6a72f into master Dec 4, 2024
51 checks passed

despairblue deleted the pay-2166-chat-triggers-dont-work-with-the-new-partial-execution-flow-try-2 branch December 4, 2024 14:33

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

🚀 Release 1.72.0 #12158

Merged

janober added the Released label Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Chat triggers don't work with the new partial execution flow #11952

fix: Chat triggers don't work with the new partial execution flow #11952

despairblue commented Nov 28, 2024 •

edited

Loading

codecov bot commented Nov 28, 2024 •

edited

Loading

despairblue Nov 29, 2024

despairblue Nov 29, 2024

despairblue Nov 29, 2024

despairblue Nov 29, 2024

ivov left a comment

ivov Dec 2, 2024

despairblue Dec 3, 2024

ivov Dec 4, 2024

despairblue Dec 4, 2024

ivov Dec 2, 2024

despairblue Dec 3, 2024

ivov Dec 4, 2024

ivov Dec 2, 2024 •

edited

Loading

despairblue Dec 3, 2024

ivov Dec 4, 2024

ivov left a comment

github-actions bot commented Dec 4, 2024

cypress bot commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024

janober commented Dec 11, 2024

fix: Chat triggers don't work with the new partial execution flow #11952

fix: Chat triggers don't work with the new partial execution flow #11952

Conversation

despairblue commented Nov 28, 2024 • edited Loading

Summary

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

codecov bot commented Nov 28, 2024 • edited Loading

Codecov Report

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 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

ivov Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2024

cypress bot commented Dec 4, 2024 • edited Loading

github-actions bot commented Dec 4, 2024

janober commented Dec 11, 2024

despairblue commented Nov 28, 2024 •

edited

Loading

codecov bot commented Nov 28, 2024 •

edited

Loading

ivov Dec 2, 2024 •

edited

Loading

cypress bot commented Dec 4, 2024 •

edited

Loading