fix(core): Ensure executions cannot resume if already running

[ivov]

https://linear.app/n8n/issue/PAY-1554

[cypress]

4 flaky tests on run #5880 ↗︎

0

399

0

0

4

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project: n8n	Commit: 5eb9c6728c
Status: Passed	Duration: 05:23 💡
Started: Jul 11, 2024 1:43 PM	Ended: Jul 11, 2024 1:48 PM

  5-ndv.cy.ts • 2 flaky tests

View Output Video

Test		Artifacts
NDV > should not retrieve remote options when required params throw errors		Screenshots Video
NDV > Stop listening for trigger event from NDV		Screenshots Video

  10-undo-redo.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Undo/Redo > should undo/redo adding connected nodes		Test Replay Screenshots Video

  24-ndv-paired-item.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
NDV > resolves expression with default item when input node is not parent, while still pairing items		Test Replay Screenshots Video

Review all test suite changes for PR #10014 ↗︎

[github-actions]

✅ All Cypress E2E specs passed

[despairblue]

This is not fixing the race condition. It will only make it less likely, but the race condition is still there.

The race condition comes from the time between reading the execution from the db and update the status in the db:

Read:

n8n/packages/cli/src/WaitingWebhooks.ts

Lines 51 to 54 in d651be4

	const execution = await this.executionRepository.findSingleExecution(executionId, {
	includeData: true,
	unflattenData: true,
	});

Write:

n8n/packages/cli/src/WorkflowRunner.ts

Line 257 in d651be4

await this.executionRepository.updateStatus(executionId, 'running');

On my machines in main mode that's ca. 35ms. This needs to be effectively 0 for the race condition to be gone.

You can still reproduce this if you do curl [resumeURL] & curl [resumeURL].
You can also use hey [resumeURL] and you end up with restarting the execution ca. 10 times:

This is an improvement over what it was before:

What I had in mind as a fix was doing pessimistic locking on the application layer, e.g. updating the status only if it was waiting and then only continue if the update worked:

		// ...
		const result = await this.executionRepository.update(
			{ id: executionId, status: 'waiting' },
			{ status: 'running' },
		);

		if (result.affected === 0) {
			throw new ConflictError(`The execution "${executionId} is running already.`);
		}

		const execution = await this.executionRepository.findSingleExecution(executionId, {
			includeData: true,
			unflattenData: true,
		});

		// ...

With that in place we get this:

The problem is that this may bypass the execution recovery, or have side effects with it that I can't foresee.

What do you think? @ivov @valya

[janober]

Got released with n8n@1.51.0