fix(core): Stop task broker before task runner processes on shutdown - #37833
Conversation
The broker drain must finish in-flight tasks before the runner processes receive SIGTERM, so the exit-grace SIGKILL escalation can only ever reach an idle or wedged runner, never a healthy busy one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Running ultrareview automatically — This change reorders shutdown to stop the broker drain before runner processes, affecting in-flight task completion and SIGKILL escalation — a subtle bug could hang shutdown or kill healthy tasks, so it warrants a deeper review.. I'll post findings when complete. |
PR review overviewBased on ownership of the 2 changed files in this PR:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
n8n-assistant
Bot
added
core
The previous assertion only checked invocation order, which a stop() that starts the broker stop without awaiting it would also satisfy. The broker stop mock now completes on a later tick, so the test fails unless the runner process stops start after the broker stop has resolved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Fixes shutdown ordering to stop the task broker and its drain before stopping runner processes, preventing SIGKILL of healthy in-flight tasks; adds a test asserting the ordering.
Re-trigger cubic
|
Successfully created backport PR for |
2 participants
Summary
TaskRunnerModule.stop()stopped the broker server and the runner processes concurrently (Promise.all). The broker server stop contains the task drain, which waits up to 0.8 x the graceful-shutdown window for in-flight tasks to finish. Running the process stop in parallel means a runner can receive SIGTERM while it is still healthy and mid-task - and since #37651 added a SIGKILL escalation 2s after SIGTERM, that healthy task gets killed instead of finishing.This matters on main and webhook instances with internal runners: their execution drain (
ActiveExecutions.shutdown) runs instopProcess(), after all@OnShutdownhandlers, so healthy tasks are routinely in flight whenTaskRunnerModule.stop()runs. Workers are unaffected (their execution drain runs at a higher shutdown priority, before the runner module stops).The fix: stop the broker server first, then the runner processes. After the drain, a runner is either idle (its task finished; it exits on SIGTERM well inside the 2s grace) or wedged on a task the broker has already failed (the SIGKILL escalation is exactly right for it). The escalation can no longer reach a healthy busy runner in any path.
How to test
pnpm --filter=n8n test src/task-runners/__tests__/task-runner-module.test.tsThe new test asserts the runner process stops are invoked only after the broker server stop has completed. It fails against the previous
Promise.allordering.Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/CAT-4338
Parent escalation: https://linear.app/n8n/issue/CAT-4336
Follow-up to #37651. Any stable backport of #37651 should include this PR.
Review / Merge checklist
Backport to Beta,Backport to Stable, orBackport to v1(if the PR is an urgent fix that needs to be backported)🤖 Generated with Claude Code