Skip to content
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(cli): Fix issue with n8n crashing when error in poll method #4008

Merged
merged 5 commits into from
Sep 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packages/cli/src/ActiveWorkflowRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import { User } from './databases/entities/User';
import { whereClause } from './WorkflowHelpers';
import { WorkflowEntity } from './databases/entities/WorkflowEntity';
import * as ActiveExecutions from './ActiveExecutions';
import { createErrorExecution } from './GenericHelpers';

const activeExecutions = ActiveExecutions.getInstance();

Expand Down Expand Up @@ -650,7 +651,14 @@ export class ActiveWorkflowRunner {
activation,
);
// eslint-disable-next-line no-underscore-dangle
returnFunctions.__emit = (data: INodeExecutionData[][]): void => {
returnFunctions.__emit = async (
data: INodeExecutionData[][] | ExecutionError,
): Promise<void> => {
if (data instanceof Error) {
await createErrorExecution(data, node, workflowData, workflow, mode);
this.executeErrorWorkflow(data, workflowData, mode);
return;
}
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
Logger.debug(`Received event to trigger execution for workflow "${workflow.name}"`);
WorkflowHelpers.saveStaticData(workflow);
Expand Down
100 changes: 98 additions & 2 deletions packages/cli/src/GenericHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,27 @@
import express from 'express';
import { join as pathJoin } from 'path';
import { readFile as fsReadFile } from 'fs/promises';
import { IDataObject } from 'n8n-workflow';
import {
ExecutionError,
IDataObject,
INode,
IRunExecutionData,
Workflow,
WorkflowExecuteMode,
} from 'n8n-workflow';
import { validate } from 'class-validator';
import config from '../config';

// eslint-disable-next-line import/no-cycle
import { Db, ICredentialsDb, IPackageVersions, ResponseHelper } from '.';
import {
Db,
ICredentialsDb,
IExecutionDb,
IExecutionFlattedDb,
IPackageVersions,
IWorkflowDb,
ResponseHelper,
} from '.';
// eslint-disable-next-line import/order
import { Like } from 'typeorm';
// eslint-disable-next-line import/no-cycle
Expand Down Expand Up @@ -214,4 +229,85 @@ export async function validateEntity(
}
}

/**
* Create an error execution
*
* @param {INode} node
* @param {IWorkflowDb} workflowData
* @param {Workflow} workflow
* @param {WorkflowExecuteMode} mode
* @returns
* @memberof ActiveWorkflowRunner
*/
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
export async function createErrorExecution(
error: ExecutionError,
node: INode,
workflowData: IWorkflowDb,
workflow: Workflow,
mode: WorkflowExecuteMode,
): Promise<void> {
const saveDataErrorExecutionDisabled = workflowData?.settings?.saveDataErrorExecution === 'none';

if (saveDataErrorExecutionDisabled) return;

const executionData: IRunExecutionData = {
startData: {
destinationNode: node.name,
runNodeFilter: [node.name],
},
executionData: {
contextData: {},
nodeExecutionStack: [
{
node,
data: {
main: [
[
{
json: {},
pairedItem: {
item: 0,
},
},
],
],
},
source: null,
},
],
waitingExecution: {},
waitingExecutionSource: {},
},
resultData: {
runData: {
[node.name]: [
{
startTime: 0,
executionTime: 0,
error,
source: [],
},
],
},
error,
lastNodeExecuted: node.name,
},
};

const fullExecutionData: IExecutionDb = {
data: executionData,
mode,
finished: false,
startedAt: new Date(),
workflowData,
workflowId: workflow.id,
stoppedAt: new Date(),
};

const execution = ResponseHelper.flattenExecutionData(fullExecutionData);

Copy link
Contributor Author

@RicardoE105 RicardoE105 Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krynble Should we check the workflow settings to check whether save error execution is enabled
before saving the error execution?

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum that's a good point. Now thinking about alternatives, we have what we call "Activation errors".

You can see in the packages/cli/src/ActiveWorkflowRunner.ts file specifically inside add function, we try to activate a workflow and if it fails, we deactivate it and add an error the user can see when opening the workflows list.

Instead of generating a failed execution, what if we deactivate the workflow and display this error?

On the other hand, this is bad because we'll deactivate the workflow even if the service is temporarily down and a next poll request would work just fine.

Maybe the current approach you implemented is best for debuggability and failed executions are saved by default, so we should check this flag, yes, and only save if it's checked.

We just need to make sure that WorkflowData that we received was actually populated with this, because I've seen situations where this did not happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this is a much better approach. For example, in the Airtable case, the issue is generated by changing the trigger column in Airtable (Workflow does not have to be touched). In case this happens, with this approach, the user will have an issue in the error tab or will be notified via the error workflow, then look at the error, and finally adjust in Airtable without having to touch the workflow. The other approach will be an extra step having to activate the workflow again, and if we do not save the execution plus they do not have an error workflow setup, they will never notice the error.

await Db.collections.Execution.save(execution as IExecutionFlattedDb);
}

export const DEFAULT_EXECUTIONS_GET_ALL_LIMIT = 20;
25 changes: 18 additions & 7 deletions packages/core/src/ActiveWorkflows.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,24 +163,35 @@ export class ActiveWorkflows {

// Get all the trigger times
const cronTimes = (pollTimes.item || []).map(toCronExpression);

// The trigger function to execute when the cron-time got reached
const executeTrigger = async () => {
const executeTrigger = async (testingTrigger = false) => {
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
Logger.debug(`Polling trigger initiated for workflow "${workflow.name}"`, {
workflowName: workflow.name,
workflowId: workflow.id,
});
const pollResponse = await workflow.runPoll(node, pollFunctions);

if (pollResponse !== null) {
// eslint-disable-next-line no-underscore-dangle
pollFunctions.__emit(pollResponse);
try {
const pollResponse = await workflow.runPoll(node, pollFunctions);

if (pollResponse !== null) {
// eslint-disable-next-line no-underscore-dangle
pollFunctions.__emit(pollResponse);
}
} catch (error) {
// If the poll function failes in the first activation
// throw the error back so we let the user know there is
// an issue with the trigger.
if (testingTrigger) {
throw error;
}
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, no-underscore-dangle
pollFunctions.__emit(error);
}
};

// Execute the trigger directly to be able to know if it works
await executeTrigger();
await executeTrigger(true);

const timezone = pollFunctions.getTimezone();

Expand Down
2 changes: 1 addition & 1 deletion packages/workflow/src/Interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,7 @@ export interface IHookFunctions {
}

export interface IPollFunctions {
__emit(data: INodeExecutionData[][]): void;
__emit(data: INodeExecutionData[][] | NodeApiError): void;
getCredentials(type: string): Promise<ICredentialDataDecryptedObject>;
getMode(): WorkflowExecuteMode;
getActivationMode(): WorkflowActivateMode;
Expand Down