Skip to content

fix(envd): support opt-in descendant signals - #3617

Open
tttboy123 wants to merge 3 commits into
e2b-dev:mainfrom
tttboy123:fix/command-kill-descendants-1034
Open

fix(envd): support opt-in descendant signals#3617
tttboy123 wants to merge 3 commits into
e2b-dev:mainfrom
tttboy123:fix/command-kill-descendants-1034

Conversation

@tttboy123

@tttboy123 tttboy123 commented Sep 3, 2026

Copy link
Copy Markdown

Problem

CommandHandle.kill() currently signals only the managed leader PID. Child processes can remain alive and are then correctly restored by a later memory-preserving pause/resume. I reproduced this against hosted E2B with Python SDK 2.46.4: the kill RPC returned true in 0.322s and removed the leader, while both child processes remained alive before and after pause/resume.

This PR adds the envd/protocol half of an opt-in descendant-kill API while preserving the existing PID-only default. It relates to e2b-dev/E2B#1034.

Solution

  • Add a backwards-compatible descendants = 3 boolean to SendSignalRequest (protobuf default: false).
  • Start non-PTY commands in their own process group. PTY commands already create a new session/process group.
  • Keep the existing leader-only signal path when descendants is false.
  • When it is true, validate that the managed PID owns the process group and signal that group.
  • Cancel output pumps only after a terminal signal is delivered successfully; rejected or failed signals keep the live process output connected.
  • Keep timeout/live-upgrade reaper signals leader-only, so existing lifecycle semantics do not change implicitly.
  • Bump envd from 0.7.0 to 0.7.1 for the behavior change.

The process-group scope intentionally covers descendants that stay in the command group. Descendants that call setsid() can escape it; a per-command cgroup / cgroup.kill guarantee would be a separate, larger lifecycle change.

Compatibility

Existing clients omit descendants, so their behavior is unchanged. A follow-up E2B SDK PR will expose this as an explicit Python/JS option after syncing this protocol commit.

Validation

  • Regression tests cover the default leader-only behavior, opt-in group signaling, non-PTY/PTY process-group configuration, group-ownership rejection, and preservation of output delivery when a signal is rejected.
  • GOTOOLCHAIN=go1.26.6 go test -race ./... — pass
  • golangci-lint v2.12.2 run --new-from-rev=upstream/main ./... — 0 issues

The repository-wide unfiltered local lint currently also reports a pre-existing unused-parameter warning in internal/services/process/dup3_other.go; CI uses only-new-issues, and this PR introduces none.

@cla-bot

cla-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

We require contributors to sign our Contributor License Agreement, and we don't have @tttboy123 on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check'

1 similar comment
@cla-bot

cla-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

We require contributors to sign our Contributor License Agreement, and we don't have @tttboy123 on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check'

@cla-bot

cla-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

We require contributors to sign our Contributor License Agreement, and we don't have @tttboy123 on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check'

@tttboy123

Copy link
Copy Markdown
Author

Follow-up hardening is pushed in 0a364e308.

During final review I found that SendSignal canceled output pumps before validating or delivering a terminal signal. That meant a rejected group-scoped kill could leave the process alive while disconnecting its output. The handler now validates and sends first, returns without touching output on failure, and cancels output only after successful SIGKILL/SIGTERM delivery.

The regression test now asserts both sides of that contract: a process that does not own its group remains alive and its output remains connected after the request is rejected.

Fresh validation:

  • GOTOOLCHAIN=go1.26.6 go test -count=1 -race ./... — pass
  • GOTOOLCHAIN=go1.26.6 go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.2 run --new-from-rev=upstream/main ./... — 0 issues

@tttboy123

Copy link
Copy Markdown
Author

@cla-bot check

@cla-bot cla-bot Bot added the cla-signed label Sep 4, 2026
@cla-bot

cla-bot Bot commented Sep 4, 2026

Copy link
Copy Markdown

The cla-bot has been summoned, and re-checked this pull request!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant