feat(sdk): support opt-in command group termination - #1801
Conversation
|
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' |
🦋 Changeset detectedLatest commit: 5ae8eea The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
TASTE.md review
Checked the changed SDK surface (Commands.kill, Pty.kill, CommandHandle.kill across JS / sync Python / async Python) against T-1 (cross-language parity), T-3 (positional vs. options), T-14 (enum over boolean), T-22/T-23 (named Opts types), T-46 (signal threading), T-54 (entry-point export), T-55 (ENVD_* version gating) and T-57/T-59 (error classes).
3 violations, all on the new descendants option:
- T-1 parity — JS
CommandHandle.kill(opts?: CommandKillOpts)now acceptsrequestTimeoutMs/signalalongsidedescendants, but the PythonCommandHandle.kill/AsyncCommandHandle.killonly takedescendants(norequest_timeout). The three surfaces should expose the same knobs. - T-14 enum over boolean —
descendants: boolselects what gets killed (leader only vs. process group). A string-literal union/Literalsuch asscope: 'process' | 'group'reads better at the call site and leaves room for a future variant (e.g. a full process tree) without a breaking change. This applies toCommandKillOpts.descendantsin JS and every Pythondescendantskwarg; I anchored only the JS interface and the syncCommands.killsignature to avoid repeating the same comment six times. - T-3 optional positionals — in Python the new parameter is appended as a second optional positional after
request_timeout, sosandbox.commands.kill(pid, 5.0, True)is now legal. Options should be keyword-only. Same pattern insandbox_sync/commands/pty.py,sandbox_async/commands/command.py,sandbox_async/commands/pty.py.
Compliant: CommandKillOpts is a named, exported Opts type extending CommandRequestOpts (T-22/T-23/T-54); the envd gate uses a named ENVD_COMMANDS_DESCENDANTS constant and semver compare (T-55); SandboxError/SandboxException is the right class for a version-gate failure of a sandbox operation (T-57).
Not a TASTE point, but note the diff also carries unrelated regenerated spec/openapi.yml / packages/python-sdk/e2b/api/client/models/* churn (rigs, access-token removal) that isn't part of this feature — probably a stale spec/infra-ref bump worth splitting out.
|
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
changed the title
|
Addressed all three TASTE review points in
On the generated REST churn: |
|
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' |
|
Addressed all three TASTE review points in
Fresh validation: Python command/PTY/handle tests 23/23, Python offline top-level unit tests 301/301, JS command/PTY/handle tests 19/19, and both SDKs' build/lint/type checks pass. On the generated REST churn: |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
putting in draft until the backend PR merges |
2 participants
Summary
Adds an explicit, backward-compatible way to terminate a command together with descendants that remain in its process group:
The existing default remains leader-only, so applications that intentionally keep subprocesses alive are not changed.
This SDK PR depends on e2b-dev/infra#3617, which adds the envd wire field and process-group implementation. It is ready for API review, but the infra dependency should land first.
Relates to #1034.
Reproduction and scope
Against hosted E2B with Python SDK 2.46.4,
CommandHandle.kill()returnedtruein 0.322 seconds and removed the managed shell, but bothsleepchildren remained alive and were restored afterpause(keep_memory=True)/resume().The reported indefinite block did not reproduce on the current SDK. This change therefore targets the confirmed lifecycle gap rather than changing timeout behavior. Pause/resume is also behaving as designed: a memory-preserving snapshot restores processes that were still alive when paused.
Design
kill()PID-only by default for compatibility.scope: "process" | "group"consistently to sync/async Python command and PTY kill methods, JS command/PTY kill options, and command handles.SendSignalRequest.descendantsfield only through generated protobuf types.0.7.1; old sandboxes fail clearly instead of silently ignoring an unknown protobuf field.spec/infra-refto the dependency commit and regenerate withmake codegen.The process-group implementation covers the reproduced shell-child case. A descendant that deliberately creates a new session with
setsid()can escape; a per-command cgroup pluscgroup.killwould be a separate, stronger lifecycle design.Generated spec note
Moving
spec/infra-reffrom the previous pin to the dependency commit also picks up current infra API-spec drift (rig capacity models and network freeze metadata). Those generated REST client changes are not part of the command-kill behavior, but are included because the generated-files workflow runsmake codegen, which fetches both the envd and REST specs from the sameinfra-ref. Removing only the REST changes leaves the worktree dirty in that required check. No generated wire types were hand-edited.If maintainers prefer, I can move that deterministic pin/codegen delta into a prerequisite spec-sync PR; the feature PR would then need to target or rebase onto that prerequisite until it lands.
Validation
make codegen: pass (the private belt spec used the repository's documented tracked-copy fallback)pytest -q tests/test_command_handle.py: 23 passedruff check,ruff format --check, andty check: passpnpm run build,pnpm run lint, andpnpm run typecheck: passGOTOOLCHAIN=go1.26.6 go test -race ./...: passgolangci-lint v2.12.2 run --new-from-rev=upstream/main ./...: 0 issuesThe full SDK suites also contain hosted integration tests. They were not used as evidence for this behavior because the new envd field is not deployed yet, and this environment did not inject a cloud API key into test commands. Offline unit coverage, generation, builds, lint, and type checks are clean.
Follow-up discussion
Please confirm whether the opt-in compatibility boundary is the desired public API. If E2B wants command handles to own their whole tree by default, or needs a guarantee across
setsid(), that should be handled as a deliberate breaking/lifecycle decision rather than silently changing existingkill()behavior.