fix(core): Keep in-memory execution status in-sync with the DB (no-changelog) #11843
Conversation
netroy
added
the
release/backport
packages/cli/src/execution-lifecycle-hooks/shared/shared-hook-functions.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
n8n-assistant
bot
added
core
| error: new NodeOperationError(mock(), 'An error occurred'), | ||
| }, | ||
| }, | ||
| } as IRun; |
There was a problem hiding this comment.
Nit - Let's use mock<IRun>({ ... }) instead of asserting?
There was a problem hiding this comment.
using mock in this case required setting quite a few properties explicitly as undefined because mock returns recursive proxy for all non-defined properties.
I'll try to clean this up in a future PR.
|
|
||
| import { determineFinalExecutionStatus } from '../shared-hook-functions'; | ||
|
|
||
| describe('determineFinalExecutionStatus', () => { |
There was a problem hiding this comment.
Thanks for adding tests! In future when it's fully covered let's look into simplifying this helper, e.g. I wonder why this old logic is relying on waitTill when we have a waiting status, etc.
n8n
|
||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
CAT-350-keep-execution-status-in-sync
|
| Run status |
|
| Run duration | 04m 53s |
| Commit |
|
| Committer | कारतोफ्फेलस्क्रिप्ट™ |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
4
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
478
|
| View all changes introduced in this branch ↗︎ | |
|
✅ All Cypress E2E specs passed |
|
Got released with |
Summary
When we evaluate the final status of an execution before persisting, we were not updating the in-memory copy of that status, which was leading to the status being incorrect in certain edge-cases.
Context
Extracted out of #11821
Related Linear tickets, Github issues, and Community forum posts
CAT-350
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)