bree icon indicating copy to clipboard operation
bree copied to clipboard

[fix] Bree doesn't gracefully exit

Open ThisIsMissEm opened this issue 5 months ago • 4 comments

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

  • [x] I have searched through GitHub issues for similar issues.
  • [x] I have completely read through the README and documentation.
  • [x] I have tested my code with the latest version of Node.js and this package and confirmed it is still not working.

ThisIsMissEm avatar Feb 01 '24 21:02 ThisIsMissEm

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

ThisIsMissEm avatar Feb 01 '24 21:02 ThisIsMissEm

PR welcome + added test case would be great too 🙏

titanism avatar Feb 01 '24 22:02 titanism

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.

ThisIsMissEm avatar Feb 02 '24 01:02 ThisIsMissEm

Best of luck

titanism avatar Feb 02 '24 02:02 titanism