fix(sandbox): kill children that fail init and surface refusal errors fixes #4283 - #4284
Conversation
…#4283) A sandboxed child whose processor init() failed was released back into the free pool instead of being killed. Since the pool only removes children on their 'exit' event and such a child never exits, it became an immortal "zombie": every subsequent job popped it, was refused with 'cannot start a not idling child process', and failed almost instantly. The refusal reason was also lost because ParentCommand.Error carries the payload under the `err` key while the sandbox message handler only read `msg.value`, producing failures with an empty error message. - child-pool: kill and remove a child when init() rejects instead of release()-ing it, so a fresh child is forked on the next retain. - sandbox: read the error payload from `msg.value ?? msg.err` so the refusal/failure reason reaches failedReason. Adds a regression test covering a transient one-time init failure and recovery on the following job, for both child processes and worker threads.
There was a problem hiding this comment.
Pull request overview
Fixes a sandbox edge case where a child/worker-thread that fails during init() could be returned to the free pool and then repeatedly reused as a non-idling “zombie”, causing subsequent jobs to be refused immediately with a lost/empty error message.
Changes:
- Kill + remove sandbox children that fail
init()instead of releasing them back to the free pool. - Preserve refusal/failure reasons by reading error payloads from
msg.value ?? msg.errin the sandbox message handler. - Add a regression test that simulates a one-time transient processor module-load failure and verifies recovery on the next job (for both child processes and worker threads).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/sandboxed_process.test.ts | Adds a regression test ensuring init-failed children aren’t reused and the next job recovers successfully. |
| tests/fixtures/fixture_processor_fail_init_once.js | New fixture processor that throws only on first import via a flag file, simulating transient init failure. |
| src/classes/sandbox.ts | Fixes lost refusal/failure reasons by reading error payload from value or err. |
| src/classes/child-pool.ts | Ensures init-failed children are killed/removed (not released), preventing “zombie” reuse. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…#4283) A sandboxed child whose processor init() failed was released back into the free pool instead of being killed. Since the pool only removes children on their 'exit' event and such a child never exits, it became an immortal "zombie": every subsequent job popped it, was refused with 'cannot start a not idling child process', and failed almost instantly. The refusal reason was also lost because ParentCommand.Error carries the payload under the `err` key while the sandbox message handler only read `msg.value`, producing failures with an empty error message. - child-pool: kill and remove a child when init() rejects instead of release()-ing it, so a fresh child is forked on the next retain. - sandbox: read the error payload from `msg.value ?? msg.err` so the refusal/failure reason reaches failedReason. Adds a regression test covering a transient one-time init failure and recovery on the following job, for both child processes and worker threads.
b5e5275 to
1ffa288
Compare
|
Hi @manast, thank you for the quick turnaround on #4283! While reviewing the changes, there are a few points we would appreciate you double-checking for side effects before this ships:
Everything else looked correct to us: the synchronous remove() inside kill() cleans the pool maps before the throw propagates, hasProcessExited() covers the already-dead case (which also fixes dead children being pushed into the free pool), and SIGKILL is safe here since the child never loaded any user code. Thank you again for carefully considering the side effects of this change. If there are any side effects we haven't spotted, we would appreciate you giving them additional consideration as well! |
|
@copilot address my comment regarding formatting of the changelog. |
Addressed in commit |
|
🎉 This PR is included in version 5.80.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.1.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
A sandboxed child whose processor init() failed was released back into the free pool instead of being killed. Since the pool only removes children on their 'exit' event and such a child never exits, it became an immortal "zombie": every subsequent job popped it, was refused with 'cannot start a not idling child process', and failed almost instantly.
The refusal reason was also lost because ParentCommand.Error carries the payload under the
errkey while the sandbox message handler only readmsg.value, producing failures with an empty error message.msg.value ?? msg.errso the refusal/failure reason reaches failedReason.Adds a regression test covering a transient one-time init failure and recovery on the following job, for both child processes and worker threads.
Port Impact Checklist
Why
Enter your explanation here.
How
Enter the implementation details here.
Additional Notes (Optional)
Any extra info here.