fix(envd): kill command's whole process tree on signal by mishushakov · Pull Request #2933 · e2b-dev/infra

mishushakov · 2026-06-05T18:28:14Z

envd's SendSignal only signaled the single process it manages (the command leader), so child processes the command spawned kept running — and consuming resources — after a kill() (see e2b-dev/E2B#1389, which worked around this SDK-side with a pgrep shell walk). Non-PTY commands now start in their own process group (Setpgid) and SendSignal delivers the signal to the whole group, terminating the leader together with its children; command timeouts tear down the group too, and PTY processes are left untouched. This is the same idiom already used for socat in internal/port/forward.go, and it's transparently backward-compatible — existing SDKs get whole-tree termination for free and can later drop their workaround. Adds a regression test (TestSendSignal_KillsProcessTree) and bumps the envd version to 0.6.2.

🤖 Generated with Claude Code

envd's SendSignal only signaled the single process it manages (the command leader), so child processes the command spawned kept running — and consuming resources — after a kill. Non-PTY commands now start in their own process group (Setpgid) and SendSignal delivers the signal to the whole group, terminating the leader together with its children; command timeouts tear down the group too. PTY processes are left untouched. Bumps the envd version. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

cursor · 2026-06-05T18:28:23Z

PR Summary

Medium Risk
Process-group SIGKILL affects every member of the group when opted in; mitigations include default-off, PTY exclusion, and a post-reap guard against PID reuse.

Overview
This change fixes orphaned child processes staying alive after envd kills only the leader it tracks. Non-PTY commands are started in their own process group, and callers can opt in with child_processes on Start and SendSignal so SIGTERM/SIGKILL (and request timeouts when set at start) reach the full tree; without the flag, behavior stays leader-only. PTY starts are unchanged. A reaped guard blocks process-group signals after the leader is waited on so a recycled PID is not signaled. envd is bumped to 0.6.8 and integration tests cover opt-in tree kill, default leader-only kill, and timeout with child_processes.

^{Reviewed by Cursor Bugbot for commit 4ef8048. Bugbot is set up for automated code reviews on this repo. Configure here.}

gemini-code-assist

Code Review

This pull request ensures that non-PTY commands run in their own process group so that signals and context cancellations terminate the entire process tree instead of just the leader process. A test is added to verify this behavior, and the package version is bumped to 0.6.2. There are no review comments, and I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

codecov · 2026-06-05T18:30:06Z

Codecov Report

❌ Patch coverage is 1.75439% with 56 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
.../envd/internal/services/process/handler/handler.go	0.00%	56 Missing ⚠️

📢 Thoughts on this report? Let us know!

Address PR review: signalProcessGroup bypassed Go's ErrProcessDone guard, so a stale signal arriving after the leader was reaped (but before deregistration) could kill(-pid) an unrelated command that recycled the pid as its process group leader. Bail out when cmd.ProcessState is set, mirroring os.Process.Signal. Also make the test's processAlive read /proc/<pid>/stat and treat zombies as not alive, so it doesn't hang when orphans aren't reaped promptly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 847707bb7d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Address PR review: reading cmd.ProcessState in SendSignal raced with cmd.Wait setting it in the reaper goroutine. Replace the guard with a handler-level atomic.Bool set immediately after cmd.Wait reaps the leader, and consult it before sending a process-group signal. The timeout-driven cmd.Cancel path no longer reads ProcessState and is safe because exec stops invoking Cancel once the process exits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Add a child_processes field to SendSignalRequest. When set, the signal is delivered to the command's whole process group (leader + children); otherwise only the leader is signaled, preserving the original behavior. Non-PTY commands still always start in their own process group so the opt-in kill can reach the tree, but the timeout-driven kill is left as leader-only. Regenerated the process protos and added a test covering the leader-only default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

chatgpt-codex-connector · 2026-07-03T15:07:39Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

chatgpt-codex-connector · 2026-07-03T15:10:35Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

cursor · 2026-07-03T15:12:01Z

+	// The leader has been reaped; stop process-group signals from targeting a
+	// potentially recycled pid. Must be set before the blocking EndEvent send
+	// below, which can hold the handler discoverable for a while.
+	p.reaped.Store(true)


Reaped flag update race

Medium Severity

After cmd.Wait() reaps the leader, the pid can be reused before p.reaped is set. A concurrent SendSignal with childProcesses can still pass the reaped check and call kill(-pid), signaling an unrelated process group that reused that pid.

Additional Locations (1)

packages/envd/internal/services/process/handler/handler.go#L388-L399

^{Reviewed by Cursor Bugbot for commit bc47f29. Configure here.}

chatgpt-codex-connector · 2026-07-03T15:12:25Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

cursor · 2026-07-03T15:14:23Z

+	for _, cp := range childPids {
+		_ = syscall.Kill(cp, syscall.SIGKILL)
+	}
+}


Signal tests lack Linux guard

Low Severity

TestSendSignal_KillsProcessTree and TestSendSignal_LeaderOnlyByDefault walk /proc via childrenOf and processAlive, but unlike other envd tests that depend on Linux procfs they never skip when GOOS is not linux, so go test for this package fails on macOS where /proc is unavailable.

Additional Locations (1)

packages/envd/internal/services/process/start_test.go#L208-L277

^{Triggered by learned rule: Envd is Linux-only — do not flag cross-platform portability concerns}

^{Reviewed by Cursor Bugbot for commit 61a94e1. Configure here.}

…esses Address PR review (timeout killed leader only): add a child_processes field to StartRequest so a command started with it also tears down its whole process group when a request timeout fires, keeping the timeout path consistent with an opted-in SendSignal. Non-opted-in commands and PTY processes keep the original leader-only behavior. Regenerated the process protos and added a timeout regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

chatgpt-codex-connector · 2026-07-03T15:16:08Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 4ef8048. Configure here.}

cursor · 2026-07-03T15:18:07Z

+			Args: []string{"-c", "sleep 120 & sleep 120 & wait"},
+		},
+		ChildProcesses: true,
+	}))


Timeout test missing header

Medium Severity

TestStart_TimeoutKillsProcessTreeWhenOptedIn relies on the client context deadline to trigger envd’s process timeout, but handleStart only arms exec.CommandContext timeout (and the child_processes group cmd.Cancel) from the Connect-Timeout-Ms header. The test never sets that header, unlike StartEnvdShell in orchestrator, so it may not exercise the timeout group-kill path it claims to cover.

^{Reviewed by Cursor Bugbot for commit 4ef8048. Configure here.}

mishushakov requested review from ValentaTomas, dobrac and jakubno as code owners June 5, 2026 18:28

cla-bot Bot added the cla-signed label Jun 5, 2026

gemini-code-assist Bot reviewed Jun 5, 2026

View reviewed changes

claude Bot reviewed Jun 5, 2026

View reviewed changes

Comment thread packages/envd/internal/services/process/handler/handler.go

Comment thread packages/envd/internal/services/process/start_test.go

chatgpt-codex-connector Bot reviewed Jun 6, 2026

View reviewed changes

Comment thread packages/envd/internal/services/process/handler/handler.go Outdated

mishushakov assigned arkamar Jun 8, 2026

cursor Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread packages/envd/internal/services/process/handler/handler.go

Merge branch 'main' into mishushakov/envd-kill-process-tree

bc47f29

cursor Bot reviewed Jul 3, 2026

View reviewed changes

chore: auto-commit generated changes

61a94e1

cursor Bot reviewed Jul 3, 2026

View reviewed changes

mishushakov marked this pull request as draft July 3, 2026 15:17

cursor Bot reviewed Jul 3, 2026

View reviewed changes

jakubno closed this Jul 9, 2026

jakubno deleted the mishushakov/envd-kill-process-tree branch July 9, 2026 13:01

Uh oh!

Conversation

mishushakov commented Jun 5, 2026

Uh oh!

cursor Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

codecov Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Jul 3, 2026

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Jul 3, 2026

Uh oh!

cursor Bot Jul 3, 2026

Choose a reason for hiding this comment

Reaped flag update race

Uh oh!

chatgpt-codex-connector Bot commented Jul 3, 2026

Uh oh!

cursor Bot Jul 3, 2026

Choose a reason for hiding this comment

Signal tests lack Linux guard

Uh oh!

chatgpt-codex-connector Bot commented Jul 3, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Jul 3, 2026

Choose a reason for hiding this comment

Timeout test missing header

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cursor Bot commented Jun 5, 2026 •

edited

Loading

codecov Bot commented Jun 5, 2026 •

edited

Loading