worker restarts #4076
worker restarts #4076
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements worker process restart functionality with automatic recovery from crashes. Workers now restart after a 5-second delay when they exit abnormally, while graceful shutdowns and service stops are respected.
Key changes:
- Workers now automatically restart after crashes with a fixed 5-second delay
- Added tracking of worker IDs and restart counts across restarts
- Shutdown protection prevents restarts during service termination
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/bitcore-node/src/services/worker.ts | Implemented worker restart logic with exit handlers, restart scheduling, and shutdown protection |
| packages/bitcore-node/test/unit/services/worker.spec.ts | Added comprehensive test suite covering restart scenarios, timing, edge cases, and error conditions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| newWorker.on('exit', (code, signal) => { | ||
| const exitReason = code !== 0 || signal ? 'crashed' : 'stopped gracefully'; | ||
| logger[code == 0 ? 'info' : 'error']( | ||
| `Worker ${newWorker.process.pid} ${exitReason} (code: ${code}, signal: ${signal})` | ||
| ); | ||
|
|
||
| const workerIndex = this.workers.findIndex(w => w.worker === newWorker); | ||
| if (workerIndex > -1) { | ||
| const workerData = this.workers[workerIndex]; | ||
| this.workers.splice(workerIndex, 1); | ||
|
|
||
| if ((code !== 0 || signal) && !this.shuttingDown) { | ||
| logger.info(`Scheduling worker ${workerData.workerId} restart in 5 seconds...`); | ||
| setTimeout(() => { | ||
| this.restartWorker(workerData.workerId, workerData.restartCount + 1); | ||
| }, 5 * 1000); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
The exit handler logic is duplicated between the start() method (lines 33-50) and the restartWorker() method (lines 98-116). This duplication makes maintenance harder and increases the risk of inconsistencies. Consider extracting the exit handler into a separate private method like setupExitHandler() that accepts the worker, workerId, and restartCount as parameters.
| newWorker.on('message', (msg: any) => { | ||
| this.emit(msg.id, msg); | ||
| }); |
There was a problem hiding this comment.
The message handler is duplicated in both start() (line 30-32) and restartWorker() (lines 94-96). Similar to the exit handler, this should be extracted to avoid duplication and ensure consistent behavior across initial workers and restarted workers.
|
|
||
| // Should NOT have restarted | ||
| expect(forkStub.callCount).to.equal(initialWorkerCount); | ||
| expect(loggerStubs.info.calledWith(sinon.match('Not restarting worker'))).to.be.false; |
There was a problem hiding this comment.
This assertion is checking for false, but when a worker crashes during shutdown, the message 'Not restarting worker' should actually be logged. The test should verify that this message IS logged (i.e., expect it to be true), not that it isn't logged. The current assertion contradicts the test's purpose of verifying shutdown protection.
| expect(loggerStubs.info.calledWith(sinon.match('Not restarting worker'))).to.be.false; | |
| expect(loggerStubs.info.calledWith(sinon.match('Not restarting worker'))).to.be.true; |
1 participant
)
shutdown