-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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.
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
fix(core): Ensure graceful shutdown for workers #9547
fix(core): Ensure graceful shutdown for workers #9547
Conversation
packages/cli/src/commands/worker.ts
Outdated
if (process.stdout.isTTY) { | ||
process.stdin.setRawMode(true); | ||
process.stdin.resume(); | ||
process.stdin.setEncoding('utf8'); | ||
|
||
process.stdin.on('data', (key: string) => { | ||
// ctrl+c | ||
if (key.charCodeAt(0) === 3) { | ||
void this.stopProcess(); | ||
} | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be moved to BaseCommand.init
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, I think this is only needed for worker
and webhook
because of their never-resolving promise. start
has its own process.stdin
listener and I'd rather not change webhook
in this PR about workers.
packages/cli/src/commands/worker.ts
Outdated
@@ -64,22 +64,22 @@ export class Worker extends BaseCommand { | |||
this.logger.info('Stopping n8n...'); | |||
|
|||
// Stop accepting new jobs | |||
await Worker.jobQueue.pause(true); | |||
void Worker.jobQueue.pause(true); // do not block so we can report progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we instead update queue.pause
to pass the second argument (doNotWaitActive
) as true
instead of leaving a dangling promise here?
1 flaky test on run #5214 ↗︎
Details:
cypress/e2e/5-ndv.cy.ts • 1 flaky test
Review all test suite changes for PR #9547 ↗︎ |
✅ All Cypress E2E specs passed |
Got released with |
https://linear.app/n8n/issue/PAY-1580