fix(core): fix lifecycle issue when cancelling manual executions

[despairblue]

Summary

The fix in #9992 introduced a lifecycle issue with executions where an execution would be removed multiple times from the ActiveExecutions.

Normally the flow would be like this:

sequenceDiagram
  Actor Customer as User
  participant AE as Active Executions
  participant WR as Workflow Runner
  participant WE as Workflow Execute
  participant Node

  Customer ->>+ WR: run workflow
  WR ->>+ AE: register execution
  activate AE
  WR ->>+ WE: execute worfklow
  WE ->>+ Node: run node
  Node ->>- WE: done
  WE ->>- WR: done
  WR ->> AE: get status
  AE ->> WR: send status
  WR ->> AE: unregister execution
  deactivate AE
  WR ->>- Customer: done

Loading

When cancelling this would happen (unless for the wait node):

sequenceDiagram
  Actor Customer as User
  participant ES as Execution Service
  participant AE as Active Executions
  participant WR as Workflow Runner
  participant WE as Workflow Execute
  participant Node

  Customer ->>+ WR: run workflow
  WR ->>+ AE: register execution
  activate AE
  WR ->>+ WE: execute worfklow
  
  WE ->>+ Node: run node

  %% cancel execution
  Customer ->>+ ES: cancel workflow run
  ES ->> WE: cancel execution
  WE -x Node: cancel execution
  ES ->> AE: unregister execution
  deactivate AE
  ES ->>- Customer: done

  %% finish execution anyways
  Node ->>- WE: done
  WE ->>- WR: done, because cancelled
  WR ->> AE: get status BOOM
  deactivate WR

Loading

Wait Node

The wait node would never resolve, so it would look like this:

sequenceDiagram
  Actor Customer as User
  participant ES as Execution Service
  participant AE as Active Executions
  participant WR as Workflow Runner
  participant WE as Workflow Execute
  participant Node

  Customer ->>+ WR: run workflow
  WR ->>+ AE: register execution
  activate AE
  WR ->>+ WE: execute worfklow
  
  WE ->>+ Node: run node

  %% cancel execution
  Customer ->>+ ES: cancel workflow run
  ES ->> WE: cancel execution
  WE ->> Node: cancel execution
  ES ->> AE: unregister execution
  deactivate AE
  ES ->>- Customer: done

  deactivate WE
  deactivate WR
  deactivate Node

Loading

Before #9992 the flow was like this. And I think it should be like that again.

sequenceDiagram
  Actor Customer as User
  participant ES as Execution Service
  participant AE as Active Executions
  participant WR as Workflow Runner
  participant WE as Workflow Execute
  participant Node

  Customer ->>+ WR: run workflow
  WR ->>+ AE: register execution
  activate AE
  WR ->>+ WE: execute worfklow
  
  WE ->>+ Node: run node

  %% cancel execution
  Customer ->>+ ES: cancel workflow run
  ES ->> WE: cancel execution
  WE -x Node: cancel execution
  ES ->>- Customer: done

  %% finish execution anyways
  Node ->>- WE: done
  WE ->>- WR: done, because cancelled
  WR ->> AE: get status
  AE ->> WR: send status
  WR ->> AE: unregister execution
  deactivate AE
  WR ->>- Customer: done

Loading

So while I have 3 solutions in this PR I prefer Option 3, which concentrates the responsibility for registering and unregistering active executions again within the WorkflowRunner.

Once I've decided on a solution I'll write tests and refactor the rest of the code.

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/PAY-1554/error-no-active-execution-found

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)

[despairblue]

This function can probably be removed, because cancelling the promises is probably not correct, given that cancelling an execution will still resolve the promise returned from it:

n8n/packages/core/src/WorkflowExecute.ts

Line 857 in d61d563

onCancel.shouldReject = false;

And in case we want to reject this, then the WorkflowRunner should do it after the execution promise has been resolved and it checked that it resolved early because it was cancelled.

[despairblue]

Preferred solution. There is only one place that deregisters the execution:

n8n/packages/cli/src/workflow-runner.ts

Line 326 in d61d563

this.activeExecutions.remove(executionId, fullRunData);

[despairblue]

Unrelated and I'll split this out, but left it here for context as to why the wait node is not affected by this.