breejs / bree

Bree is a Node.js and JavaScript job task scheduler with worker threads, cron, Date, and human syntax. Built for @ladjs, @forwardemail, @spamscanner, @cabinjs.
https://jobscheduler.net
MIT License
2.96k stars 80 forks source link

[fix] Bree doesn't gracefully exit #242

Open ThisIsMissEm opened 5 months ago

ThisIsMissEm commented 5 months ago

Describe the bug

Node.js version: 20

OS version: n/a — happens on both mac and linux

Description:

When the main Bree process is terminated with ctrl+c (SIGINT), using graceful, the worker process is told to terminate, which results in it exiting with a 1 exit code, even though it was terminated by the main process.

This results in the errorHandler option receiving a Worker for job "BackgroundJob" exited with code 1 message

Actual behavior

The logs printed during graceful termination look like the following (here I'm using pino with pino-pretty as the logger:

[22:44:47.504] INFO (BackgroundJob/71797): Received cancellation of job
[22:44:47.507] INFO (71797): Worker for job "BackgroundJob" sent a message
[22:44:47.507] INFO (71797): Gracefully cancelled worker for job "BackgroundJob"
[22:44:47.519] ERROR (71797): Worker for job "BackgroundJob" exited with code 1
    err: {
      "type": "Error",
      "message": "Worker for job \"BackgroundJob\" exited with code 1",
      "stack":
          Error: Worker for job "BackgroundJob" exited with code 1
              at Worker.<anonymous> (node_modules/bree/src/index.js:476:13)
              at Worker.emit (node:events:530:35)
              at [kOnExit] (node:internal/worker:315:10)
              at Worker.<computed>.onexit (node:internal/worker:229:20)
    }
[22:44:47.524] INFO (71797): Gracefully exited

Expected behavior

Bree should be aware that it requested the job to terminate, and therefore not treat this exit code of 1 as an error, i.e., it should actually gracefully shut down.

Per the node.js Worker documentation:

The 'exit' event is emitted once the worker has stopped. If the worker exited by calling process.exit(), the exitCode parameter is the passed exit code. If the worker was terminated, the exitCode parameter is 1.

Code to reproduce

// set up logging
const logger = pino({});

// set up jobs/scheduler
// configuration in src/jobs folder
const bree = new Bree({
  logger,
  root: path.join(__dirname, "../src/jobs"),
  jobs: [
    {
      name: "BackgroundJob", // only fires on start
    },
  ],
  errorHandler: (error) => {
    logger.error(error);
  },
});

It doesn't matter what "BackgroundJob" actually does.

Checklist

ThisIsMissEm commented 5 months ago

I believe this is due to this exit handler not being aware that Bree is attempting to shutdown:

https://github.com/breejs/bree/blob/master/src/index.js#L469

titanism commented 5 months ago

PR welcome + added test case would be great too 🙏

ThisIsMissEm commented 5 months ago

PR welcome + added test case would be great too 🙏

If I'm gonna be working on scheduler code, I'll write it from scratch and ditch Bree completely, tbh. I've been trying to avoid writing this code.

titanism commented 5 months ago

Best of luck