fix(core): Resolves response promise for active execution on job finished in queue mode

[guillaumejacquart]

Summary

On form submission in scaling mode, when the form trigger is followed by a form page, the second form submit "responsePromise" is never resolved, and the form post thus never resolves. There is currently a workaround setup in the UI for this that fetches the execution data to know whether the job is finished or not, and update the form UI accordingly. But this comes with issues of its own, like this one: https://linear.app/n8n/issue/NODE-2557/form-node-form-shows-success-when-there-was-an-error

What we want to do is resolve the responsePromise when a form is submitted in queue mode, so that form submission properly finishes, and we can adapt the UI workaround to prevent race conditions: https://github.com/n8n-io/n8n/blob/master/packages/cli/templates/form-trigger.handlebars#L861

I need insights to know if resolving the response promise from the job-finished msg is safe enough though. Also, we noticed that for webhooks (in queue mode), both messages are listened to ('responde-to-webhook' and 'job-finished'), meaning that the promise will be resolved twice. This seems fine but still..

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

• [X] 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)

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/cli/src/scaling/scaling.service.ts	0.00%	1 Missing :warning:

:loudspeaker: Thoughts on this report? Let us know!

[dana-gill]

This looks okay to me, but I don't have enough context, and I would prefer another engineer more well-versed in core also review this

[Cadiac]

I don't think it would work and will break Respond to Webhook node that also require resolving response promise but with actual payload

Didn't we test this case @guillaumejacquart , and it seemed to behave as expected - in that case code enters the 'respond-to-webhook' path at this switch case which also sends out the payload.

[guillaumejacquart]

I don't think it would work and will break Respond to Webhook node that also require resolving response promise but with actual payload

Didn't we test this case @guillaumejacquart , and it seemed to behave as expected - in that case code enters the 'respond-to-webhook' path at this switch case which also sends out the payload.

@michael-radency Yes as @Cadiac said we tested in with respond to webhook node, in which case we also enter the 'respond-to-webhook' case condition and resolve the promise before the job finished: https://github.com/n8n-io/n8n/pull/15643/files#diff-8156178aea8a65acfffdb8b563c9eb8d67261ee667dcdaba54175a64154a8283R304

[michael-radency]

I don't think it would work and will break Respond to Webhook node that also require resolving response promise but with actual payload

Didn't we test this case @guillaumejacquart , and it seemed to behave as expected - in that case code enters the 'respond-to-webhook' path at this switch case which also sends out the payload.

@michael-radency Yes as @Cadiac said we tested in with respond to webhook node, in which case we also enter the 'respond-to-webhook' case condition and resolve the promise before the job finished: https://github.com/n8n-io/n8n/pull/15643/files#diff-8156178aea8a65acfffdb8b563c9eb8d67261ee667dcdaba54175a64154a8283R304

thanks @guillaumejacquart I did manual testing and all seems to work correctly

[shortstacked]

Workflow Test Results 📊 ⚠️ 4 Warnings (0 Failed), 👍 79 Successful out of 83 total workflows.

View full workflow run

Tested Ref: 812044805663306a4fcf044c82f78634401b5cdc by @michael-radency

⚠️ Warnings (4)

Workflow ID	Workflow Name	Reason
237	BasicLLMChain:AzureChat	Workflow contains new data that previously did not exist.
35	Slack:User:getPresence info:UserProfile:get update...	Workflow contains new data that previously did not exist.
48	Asana:Project:getAll get:Task:create update move g...	Workflow contains new data that previously did not exist.
257	Agent:auto-fix:anthropic	Workflow contains new data that previously did not exist.

[github-actions[bot]]

:white_check_mark: All Cypress E2E specs passed

[janober]

Got released with [email protected]