fix: clarify EndEvent.error semantics for normal non-zero exits in envd process handler#3157
fix: clarify EndEvent.error semantics for normal non-zero exits in envd process handler#3157AdaAibaby wants to merge 5 commits into
Conversation
…t.error Previously, envd set EndEvent.error for all non-zero process exits because exec.Cmd.Wait() returns *exec.ExitError for normal non-zero exits. This made it impossible for clients to distinguish between a process that ran and returned a non-zero exit code vs an actual runtime/infrastructure failure. Now, EndEvent.error is only set for true runtime errors (e.g., failure to wait for the process). Normal non-zero exits are represented solely by exit_code, exited, and status fields. Also added documentation comments to process.proto clarifying the intended semantics of each EndEvent field. Fixes e2b-dev#2657
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
While the nil check on p.cmd.ProcessState prevents a panic within the conditional block, p.cmd.ProcessState can still be nil if the process failed to start or wait failed with a non-ExitError. In such cases, subsequent lines will attempt to call methods on the nil ProcessState pointer, resulting in a nil pointer dereference panic. The fields of EndEvent must be safely populated only when p.cmd.ProcessState is non-nil.
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.
If the process failed to start or Wait returned a non-ExitError, ProcessState can be nil. Guard the ExitCode/Exited/Status field access to prevent a nil pointer dereference.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50f37204d6
ℹ️ 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".
Sync the generated Go protobuf files with the updated proto field comments documenting EndEvent semantics.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5904304bdc
ℹ️ 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".
When a process is killed by a signal (e.g., via SendSignal RPC),
cmd.Wait() returns *exec.ExitError with Exited()==false. This is
still a normal process result, not a runtime/infrastructure error.
Remove the Exited() check so that all *exec.ExitError cases with
a valid ProcessState are treated as process results. Signal kills
are described by status ('signal: killed') and exited (false).
2 participants
Summary
Fixes #2657
This PR distinguishes normal process exits (including non-zero exits and signal kills) from runtime/infrastructure errors in
EndEvent.error.Problem
exec.Cmd.Wait()in Go returns*exec.ExitErrorfor processes that start successfully but exit with a non-zero status or are killed by a signal. The previous implementation passed this error directly intoEndEvent.error, making it impossible for clients to distinguish between:SendSignalRPC (normal behavior)Changes
packages/envd/internal/services/process/handler/handler.go*exec.ExitErrorcheck: whencmd.Wait()returns an*exec.ExitErrorand the process has a validProcessState, the error is not propagated toEndEvent.errorexit_code,exited, andstatusfieldsEndEvent.errorProcessStateto prevent potential nil pointer dereferencepackages/envd/spec/process/process.protoEndEventfields clarifying their intended semanticserroris for runtime/infrastructure-level failures, not normal process exitsGenerated files
packages/envd/internal/services/spec/process/process.pb.goandpackages/shared/pkg/grpc/envd/process/process.pb.gowith the new field commentsBehavior Change
exit 0exit_code=0, error=nilexit_code=0, error=nil(unchanged)exit 1exit_code=1, error="exit status 1"exit_code=1, error=nilSIGKILLexit_code=-1, error="signal: killed"exit_code=-1, error=nilSIGTERMexit_code=-1, error="signal: terminated"exit_code=-1, error=nilerror="..."error="..."(unchanged)Compatibility Note
This is a behavior change for clients that currently rely on
EndEvent.errorbeing set for non-zero exits or signal kills. Clients should useexit_code,exited, andstatusto inspect process results instead.