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.

Sign up for GitHub

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

Jump to bottom

fix(core): Throw on adding execution without execution data #9903

Merged
merged 6 commits into from
Jul 2, 2024
Merged

fix(core): Throw on adding execution without execution data #9903

merged 6 commits into from
Jul 2, 2024

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Jul 1, 2024 •
edited
Loading

Summary

Re-enqueuing of executions on startup appears to be causing an issue when execution data is null. Cannot reproduce locally yet, so for now we throw on adding an execution without execution data so we can diagnose what kinds of workflows cna cause this.

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/PAY-1715
https://community.n8n.io/t/error-starting-1-47-0/49084

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)

@ivov ivov added the release/backport Changes that need to be backported to older releases. label Jul 1, 2024
@ivov ivov requested a review from netroy July 1, 2024 14:52
@ivov ivov changed the title fix(core): Stop re-enqueing executions on startup fix(core): Stop re-enqueuing executions on startup Jul 1, 2024
@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jul 1, 2024
@ivov ivov changed the title fix(core): Stop re-enqueuing executions on startup fix(core): Throw on adding execution without execution data Jul 1, 2024
netroy
netroy previously approved these changes Jul 1, 2024
View reviewed changes
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 1, 2024

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

@cypress Cypress
Copy link

cypress bot commented Jul 1, 2024 •
edited
Loading

43 failed and 1 flaky tests on run #5732 ↗︎

43 195 0 0 Flakiness 1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project: n8n Commit: 274cff9d31
Status: Failed Duration: 13:04 💡
Started: Jul 1, 2024 4:07 PM Ended: Jul 1, 2024 4:20 PM
Failed  5-ndv.cy.ts • 9 failed tests

View Output Video

Test Artifacts
NDV > should disconect Switch outputs if rules order was changed Screenshots Video
NDV > should render run errors correctly Screenshots Video
NDV > can link and unlink run selectors between input and output Screenshots Video
NDV > Should render xml and html tags as strings and can search Screenshots Video
NDV > should properly show node execution indicator Screenshots Video
NDV > should properly show node execution indicator for multiple nodes Screenshots Video
NDV > test output schema view > should switch to output schema view and validate it Screenshots Video
NDV > test output schema view > should collapse and expand nested schema object Screenshots Video
NDV > test output schema view > should not display pagination for schema Screenshots Video
Failed  12-canvas.cy.ts • 3 failed tests

View Output Video

Test Artifacts
Canvas Node Manipulation and Navigation > should add merge node and test connections Test Replay Screenshots Video
Canvas Node Manipulation and Navigation > should add nodes and check execution success Test Replay Screenshots Video
Canvas Node Manipulation and Navigation > should preserve connections after rename & node-view switch Test Replay Screenshots Video
Failed  19-execution.cy.ts • 11 failed tests

View Output Video

Test Artifacts
Execution > should test manual workflow Test Replay Screenshots Video
Execution > should test manual workflow stop Test Replay Screenshots Video
Execution > should send proper payload for node rerun Test Replay Screenshots Video
Execution > should send proper payload for manual node run Test Replay Screenshots Video
Execution > should successfully execute partial executions with nodes attached to the second output Test Replay Screenshots Video
Execution > should execute workflow partially up to the node that has issues Test Replay Screenshots Video
Execution > execution preview > when deleting the last execution, it should show empty state Test Replay Screenshots Video
Execution > connections should be colored differently for pinned data > when executing the workflow Test Replay Screenshots Video
Execution > connections should be colored differently for pinned data > when executing a node Test Replay Screenshots Video
Execution > connections should be colored differently for pinned data > when connecting pinned node by output drag and drop Test Replay Screenshots Video
The first 10 failed tests are shown, see all 11 tests in Cypress Cloud.
Failed  7-workflow-actions.cy.ts • 2 failed tests

View Output Video

Test Artifacts
Workflow Actions > should run workflow on button click Test Replay Screenshots Video
Workflow Actions > should run workflow using keyboard shortcut Test Replay Screenshots Video
Failed  14-data-transformation-expressions.cy.ts • 6 failed tests

View Output Video

Test Artifacts
Data transformation expressions > $json + native string methods Test Replay Screenshots Video
Data transformation expressions > $json + n8n string methods Test Replay Screenshots Video
Data transformation expressions > $json + native numeric methods Test Replay Screenshots Video
Data transformation expressions > $json + n8n numeric methods Test Replay Screenshots Video
Data transformation expressions > $json + native array access Test Replay Screenshots Video
Data transformation expressions > $json + n8n array methods Test Replay Screenshots Video

The first 5 failed specs are shown, see all 49 specs in Cypress Cloud.

Flakiness  e2e/5-ndv.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > should not retrieve remote options when required params throw errors Screenshots Video

Review all test suite changes for PR #9903 ↗︎

netroy
netroy previously approved these changes Jul 1, 2024
View reviewed changes
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 1, 2024

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 2, 2024

✅ All Cypress E2E specs passed

@ivov ivov merged commit abb7458 into master Jul 2, 2024
33 checks passed
@ivov ivov deleted the stop-re-enqueuing-executions-on-startup branch July 2, 2024 13:11
cstuncsik pushed a commit that referenced this pull request Jul 3, 2024
@ivov @cstuncsik
fix(core): Throw on adding execution without execution data (#9903)
536b29c
@github-actions github-actions bot mentioned this pull request Jul 3, 2024
Merged
ivov added a commit that referenced this pull request Jul 3, 2024
@ivov
fix(core): Throw on adding execution without execution data (#9903)
ada1adb
@github-actions github-actions bot mentioned this pull request Jul 3, 2024
Merged
@janober
Copy link
Member

janober commented Jul 3, 2024

Got released with n8n@1.48.3

@github-actions github-actions bot mentioned this pull request Jul 3, 2024
Merged
adrian-martinez-onestic pushed a commit to onesdata/n8n-fork that referenced this pull request Jul 8, 2024
@ivov @adrian-martinez-onestic
fix(core): Throw on adding execution without execution data (n8n-io#9903
2c3f907 
)
tomi added a commit that referenced this pull request Aug 1, 2024
@tomi
fix(core): Make execution and its data creation atomic
e115015 
Executions and their data are inserted in two separate insert statements. Before this
PR, this wasn't done in a transaction, which could cause executions to be created
without execution data. This caused issues like the one fixed in #9903.
@tomi tomi mentioned this pull request Aug 1, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@netroy netroy netroy approved these changes

Assignees
No one assigned
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team release/backport Changes that need to be backported to older releases. Released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ivov @janober @netroy