Skip to content

Conversation

@pandemicsyn
Copy link

@pandemicsyn pandemicsyn commented Jan 8, 2026

This fixes an issue where killProcess returns success but the actual process continues running.

When killProcess is called, it sends SIGTERM to the PID stored in the PID file. However, this PID is the subshell wrapper, not the actual command.

Added trap 'kill 0' TERM inside the background subshell. When the subshell receives SIGTERM, the trap fires and sends SIGTERM to all processes in the current process group (the subshell and its children).

fixes #338

Previously, killProcess would send SIGTERM to the subshell PID but the
signal was not propagated to child processes. This caused the actual
command to continue running as an orphan process while the API returned
success.

Added trap 'kill 0' TERM inside background subshells to forward SIGTERM
to all processes in the process group when the subshell is killed.
@changeset-bot
Copy link

changeset-bot bot commented Jan 8, 2026

🦋 Changeset detected

Latest commit: 2694b4e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudflare/sandbox Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pandemicsyn pandemicsyn marked this pull request as ready for review January 8, 2026 17:43
@ghostwriternr
Copy link
Member

Hey @pandemicsyn! Thanks for identifying this issue - The bug report is indeed accurate - killProcess() wasn't terminating child processes.

However, trap 'kill 0' TERM seems to have a problem: in non-interactive bash (which we use), all processes share the same process group. kill 0 would terminate the entire session, not just the target command. I was able to validate this with an E2E test too. I've instead opened #339 with an alternative fix that uses bash job control (set -m) to give each background command its own process group, then kills the group with kill(-pid). This felt more reliable since it's handled by the kernel rather than depending on bash signal handlers.

To get this closed soon, I'm closing this PR in favour of #339 - thanks again for raising this!

@pandemicsyn
Copy link
Author

@ghostwriternr no worries , thanks for getting this squared away so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

killProcess maybe doesn't work?

2 participants