feat: add reuseChildProcess option to disable child process reuse - #4091
feat: add reuseChildProcess option to disable child process reuse#4091samwisekind wants to merge 7 commits into
reuseChildProcess option to disable child process reuse#4091Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-out for sandboxed child process/worker thread reuse so users can avoid long-lived memory growth from leaky processors/dependencies by terminating the sandbox after each job.
Changes:
- Introduces
reuseChildProcess?: boolean(defaulttrue) in sandboxed/worker options and wires it intoWorker -> ChildPool. - Updates
ChildPool.release()to optionally terminate the child instead of returning it to the free pool (and makesrelease()async); updates sandbox cleanup accordingly. - Adds unit/integration tests and updates sandboxed processors documentation to describe the new option and its performance tradeoff.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/sandboxed_process.test.ts | Integration tests asserting PID reuse vs non-reuse across jobs. |
| tests/child-pool.test.ts | Unit tests for ChildPool release/kill behavior and updated async release usage. |
| src/interfaces/sandboxed-options.ts | Adds the reuseChildProcess option to the public options surface. |
| src/classes/worker.ts | Passes reuseChildProcess into the ChildPool created for sandboxed processors. |
| src/classes/sandbox.ts | Awaits the now-async childPool.release() during sandbox cleanup. |
| src/classes/child-pool.ts | Implements conditional termination on release and makes release() async. |
| docs/gitbook/guide/workers/sandboxed-processors.md | Documents disabling reuse and warns about throughput impact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async release(child: Child): Promise<void> { | ||
| delete this.retained[child.pid]; | ||
| this.getFree(child.processFile).push(child); | ||
| if (this.reuseChildProcess) { | ||
| this.getFree(child.processFile).push(child); | ||
| } else { | ||
| await this.kill(child, 'SIGTERM'); | ||
| } |
There was a problem hiding this comment.
release() now awaits kill(). Child.kill() currently has a race where the underlying process/thread can exit between hasProcessExited() and registering the once('exit') listener, which can leave the awaited onExit promise unresolved indefinitely. With reuseChildProcess: false this path runs after every job, so this can hang job completion. Consider making Child.kill() race-safe (attach listener first, then re-check exited state and resolve/remove listener if already exited), and/or add a bounded fallback in ChildPool.kill() to avoid awaiting forever.
There was a problem hiding this comment.
good catch, this was not even part of the PR, but nevertheless seems important for this new option to avoid workers that could get stuck forever.
There was a problem hiding this comment.
@manast Happy to update the PR tackling this (and the typo comments below) or as a quick follow-up!
| if (this.reuseChildProcess) { | ||
| this.getFree(child.processFile).push(child); | ||
| } else { | ||
| await this.kill(child, 'SIGTERM'); | ||
| } |
There was a problem hiding this comment.
When reuseChildProcess is false, release() will await a full termination with CHILD_KILL_TIMEOUT = 30_000. That extends the processor's perceived runtime and delays the job being moved to completed/failed, potentially reducing throughput or causing stalls if shutdown is slow. Consider using a shorter timeout for the per-job termination path and/or making the timeout configurable for this option.
There was a problem hiding this comment.
Not sure this is an issue in practice.
| const escalate = new Promise<void>(resolve => | ||
| setTimeout(() => { | ||
| if (!this.hasProcessExited()) { | ||
| this.killProcess('SIGKILL'); | ||
| } | ||
| resolve(); | ||
| }, timeoutMs), | ||
| ); | ||
|
|
||
| await Promise.race([onExit, escalate]); |
| const escalate = new Promise<void>(resolve => | ||
| setTimeout(() => { | ||
| if (!this.hasProcessExited()) { | ||
| this.killProcess('SIGKILL'); | ||
| } | ||
| resolve(); | ||
| }, timeoutMs), | ||
| ); | ||
|
|
||
| await Promise.race([onExit, escalate]); |
|
@copilot please address the PR comments. |
|
@manast I think there was a copilot outage last week 😅 let me know if you want me to apply the code suggestions manually! |
Why
Resolves #2422
By default BullMQ reuses sandboxed child processes across jobs for performance. However, if a processor or its dependencies have known memory leaks, memory usage can grow unboundedly over time. There is currently no way to opt out of this behaviour.
The optional
reuseChildProcessproperty (when set tofalse) terminates the child process after each job completes, so every job runs in a fresh process with clean memory.How
reuseChildProcess?: boolean(defaulttrue) toSandboxedOptionsreuseChildProcessisfalse, it kills the child viaSIGTERMinstead of returning it to the free poolChildPool.release()from sync to async because thekill()method it now conditionally calls is already asyncsandbox.tsto await the now-asyncrelease()callchild-pool.test.tsverifying that children are killed on release and that new children are spawned after releasesandboxed_process.test.tsthat assert PIDs differ across jobs when reuse is disabled, and match when reuse is enabledAdditional Notes (Optional)
trueto match current behaviour)ChildPool.release()