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(core): Ensure job processor does not reprocess amended executions #11438

Merged

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Oct 28, 2024

Summary

This PR fixes an edge case caused by an unwanted interaction between Bull's implicit retry mechanism and n8n's execution recovery mechanism.

When a worker crashes mid-execution, it fails to send a heartbeat back to Redis, prompting Bull to re-enqueue the job. When the worker restarts, its event bus looks at the filesystem logs and marks the execution as crashed and amends its data. But, the job is still in the queue. When this worker (or another worker, makes no difference) receives the job, the processor tries to run the execution but finds its amended data is lacking some of the data needed for execution, hence Cannot read properties of undefined (reading 'node') where undefined should have been the zeroth item in the nodeExecutionStack.

This unwanted interaction comes from two legacy systems that we are planning to rework in future, so this PR ensures that amended executions cannot be reprocessed by workers.

Related Linear tickets, Github issues, and Community forum posts

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)

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Oct 28, 2024
* cause a crashed execution to be enqueued. We refrain from processing it,
* until we have reworked both mechanisms to prevent this scenario.
*/
if (execution.status === 'crashed') return { success: false };
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using the return value for anything?

I only found this function being called here, and there we're not using the result:

await this.jobProcessor.processJob(job);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Historically we never used the result, we've only relied on promise resolution or rejection. The important bit is the early return to resolve the promise without any reprocessing of the crashed execution. I went for { success: false } to keep the return type simple, aligned with what is already there.

Copy link

cypress bot commented Oct 28, 2024

n8n    Run #7595

Run Properties:  status check passed Passed #7595  •  git commit 3ffc4b7c0c: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project n8n
Branch Review pay-2100-cannot-read-properties-of-undefined-reading-node
Run status status check passed Passed #7595
Run duration 04m 36s
Commit git commit 3ffc4b7c0c: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Committer Iván Ovejero
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 460
View all changes introduced in this branch ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

@ivov ivov merged commit c152a3a into master Oct 29, 2024
51 checks passed
@ivov ivov deleted the pay-2100-cannot-read-properties-of-undefined-reading-node branch October 29, 2024 07:51
@github-actions github-actions bot mentioned this pull request Oct 31, 2024
@janober
Copy link
Member

janober commented Oct 31, 2024

Got released with n8n@1.66.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants