Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion components/execd/pkg/jupyter/execute/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,9 @@ type ErrorOutput struct {
// EValue is the value of the error
EValue string `json:"evalue"`

// ExitCode is the exit code of the command
ExitCode int `json:"exit_code"`
Comment on lines +209 to +210

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Omit exit_code when no process exit code exists

Because ErrorOutput is reused for non-command failures and ServerStreamEvent.ToJSON marshals it directly (components/execd/pkg/web/model/codeinterpreting.go:99-101), making ExitCode a plain int means every error that does not explicitly set it will now serialize as "exit_code": 0. That affects ordinary interpreter/SQL failures such as ContextCancelled (components/execd/pkg/runtime/jupyter.go:121-124) and DBInitError (components/execd/pkg/runtime/sql.go:45-47), so any client that starts trusting the new field will misread “no exit code available” as a successful process exit.

Useful? React with 👍 / 👎.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, add an unified exit status code 255 for these non-command execution failures, to avoid the ambiguity caused by exit_code=0.


// Traceback is the traceback of the error
Traceback []string `json:"traceback"`
}
Expand All @@ -214,8 +217,9 @@ func (e *ErrorOutput) String() string {
return fmt.Sprintf(`
Error: %s
Value: %s
ExitCode: %d
Traceback: %s
`, e.EName, e.EValue, strings.Join(e.Traceback, "\n"))
`, e.EName, e.EValue, e.ExitCode, strings.Join(e.Traceback, "\n"))
}

// StatusUpdate represents kernel status update
Expand Down
6 changes: 4 additions & 2 deletions components/execd/pkg/runtime/bash_session.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,12 +287,14 @@ func (s *bashSession) run(ctx context.Context, request *ExecuteCodeRequest) erro
}
if request.Hooks.OnExecuteError != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{
EName: "CommandExecError",
EName: commandExecError,
// Deprecated: EValue is deprecated for command, use ExitCode instead
EValue: strconv.Itoa(userExitCode),
ExitCode: userExitCode,
Traceback: []string{errMsg},
})
}
log.Error("CommandExecError: %s (command: %q)", errMsg, request.Code)
log.Error("%s: %s (command: %q)", commandExecError, errMsg, request.Code)
return nil
}

Expand Down
4 changes: 2 additions & 2 deletions components/execd/pkg/runtime/bash_session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ func TestBashSession_NonZeroExitEmitsError(t *testing.T) {
require.Fail(t, "expected error hook to be called")
}
require.NotNil(t, gotErr, "expected non-nil error output")
require.Equal(t, "CommandExecError", gotErr.EName)
require.Equal(t, "7", gotErr.EValue)
require.Equal(t, commandExecError, gotErr.EName)
require.Equal(t, 7, gotErr.ExitCode)
require.NotEmpty(t, sessionID, "expected session id to be set")
require.Equal(t, "before", stdoutLine)

Expand Down
18 changes: 10 additions & 8 deletions components/execd/pkg/runtime/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ func (c *Controller) runCommand(ctx context.Context, request *ExecuteCodeRequest
close(done)
wg.Wait()
request.Hooks.OnExecuteInit(session)
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "CommandExecError", EValue: err.Error()})
log.Error("CommandExecError: error starting commands: %v", err)
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: commandInitError, EValue: err.Error()})
log.Error("%s: error starting commands: %v", commandInitError, err)
return nil
}

Expand Down Expand Up @@ -185,23 +185,25 @@ func (c *Controller) runCommand(ctx context.Context, request *ExecuteCodeRequest
var exitError *exec.ExitError
if errors.As(err, &exitError) {
exitCode := exitError.ExitCode()
eName = "CommandExecError"
eName = commandExecError
eValue = strconv.Itoa(exitCode)
eCode = exitCode
} else {
eName = "CommandExecError"
eName = commandExecError
eValue = err.Error()
eCode = 1
}
traceback = []string{err.Error()}

request.Hooks.OnExecuteError(&execute.ErrorOutput{
EName: eName,
EName: eName,
// Deprecated: EValue is deprecated for command, use ExitCode instead
EValue: eValue,
ExitCode: eCode,
Traceback: traceback,
})

log.Error("CommandExecError: error running commands: %v", err)
log.Error("%s: error running commands: %v", commandExecError, err)
c.markCommandFinished(session, eCode, err.Error())
return nil
}
Expand Down Expand Up @@ -268,7 +270,7 @@ func (c *Controller) runBackgroundCommand(ctx context.Context, cancel context.Ca
}
if err != nil {
cancel()
log.Error("CommandExecError: error starting commands: %v", err)
log.Error("%s: error starting commands: %v", commandInitError, err)
kernel.running = false
c.storeCommandKernel(session, kernel)
c.markCommandFinished(session, 255, err.Error())
Expand All @@ -285,7 +287,7 @@ func (c *Controller) runBackgroundCommand(ctx context.Context, cancel context.Ca
err = cmd.Wait()
cancel()
if err != nil {
log.Error("CommandExecError: error running commands: %v", err)
log.Error("%s: error running commands: %v", commandExecError, err)
exitCode := 1
var exitError *exec.ExitError
if errors.As(err, &exitError) {
Expand Down
4 changes: 2 additions & 2 deletions components/execd/pkg/runtime/command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,8 @@ func TestRunCommand_Error(t *testing.T) {
require.Equal(t, []string{"before"}, stdoutLines)
require.Empty(t, stderrLines, "expected no stderr")
require.NotNil(t, gotErr, "expected error hook to be called")
require.Equal(t, "CommandExecError", gotErr.EName)
require.Equal(t, "3", gotErr.EValue)
require.Equal(t, commandExecError, gotErr.EName)
require.Equal(t, 3, gotErr.ExitCode)
}

// TestStdLogDescriptor_AutoCreatesTempDir verifies that stdLogDescriptor
Expand Down
24 changes: 14 additions & 10 deletions components/execd/pkg/runtime/command_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ func (c *Controller) runCommand(ctx context.Context, request *ExecuteCodeRequest

err = cmd.Start()
if err != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "CommandExecError", EValue: err.Error()})
log.Error("CommandExecError: error starting commands: %v", err)
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: commandInitError, EValue: err.Error()})
log.Error("%s: error starting commands: %v", commandInitError, err)
return nil
}

Expand All @@ -78,25 +78,29 @@ func (c *Controller) runCommand(ctx context.Context, request *ExecuteCodeRequest
if err != nil {
var eName, eValue string
var traceback []string
var eCode int

var exitError *exec.ExitError
if errors.As(err, &exitError) {
exitCode := exitError.ExitCode()
eName = "CommandExecError"
eValue = strconv.Itoa(exitCode)
eCode = exitError.ExitCode()
eName = commandExecError
eValue = strconv.Itoa(eCode)
} else {
eName = "CommandExecError"
eName = commandExecError
eValue = err.Error()
eCode = 1
}
traceback = []string{err.Error()}

request.Hooks.OnExecuteError(&execute.ErrorOutput{
EName: eName,
EName: eName,
// Deprecated: EValue is deprecated for command, use ExitCode instead
EValue: eValue,
ExitCode: eCode,
Traceback: traceback,
})

log.Error("CommandExecError: error running commands: %v", err)
log.Error("%s: error running commands: %v", commandExecError, err)
return nil
}
request.Hooks.OnExecuteComplete(time.Since(startAt))
Expand Down Expand Up @@ -131,7 +135,7 @@ func (c *Controller) runBackgroundCommand(ctx context.Context, cancel context.Ca
safego.Go(func() {
err := cmd.Start()
if err != nil {
log.Error("CommandExecError: error starting commands: %v", err)
log.Error("%s: error starting commands: %v", commandInitError, err)
pipe.Close() // best-effort
cancel()
return
Expand Down Expand Up @@ -161,7 +165,7 @@ func (c *Controller) runBackgroundCommand(ctx context.Context, cancel context.Ca
devNull.Close() // best-effort

if err != nil {
log.Error("CommandExecError: error running commands: %v", err)
log.Error("%s: error running commands: %v", commandExecError, err)
exitCode := 1
var exitError *exec.ExitError
if errors.As(err, &exitError) {
Expand Down
5 changes: 3 additions & 2 deletions components/execd/pkg/runtime/jupyter.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,9 @@ func (c *Controller) runJupyterCode(ctx context.Context, kernel *jupyterKernel,
}

request.Hooks.OnExecuteError(&execute.ErrorOutput{
EName: "ContextCancelled",
EValue: "Interrupt kernel",
EName: "ContextCancelled",
EValue: "Interrupt kernel",
ExitCode: 255,
})
return errors.New("context cancelled, interrupt kernel")
}
Expand Down
18 changes: 9 additions & 9 deletions components/execd/pkg/runtime/sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@ func (c *Controller) runSQL(ctx context.Context, request *ExecuteCodeRequest) er
request.Hooks.OnExecuteInit(uuid.New().String())
err := c.initDB()
if err != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "DBInitError", EValue: err.Error()})
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "DBInitError", EValue: err.Error(), ExitCode: 255})
log.Error("DBInitError: error initializing db server: %v", err)
return err
}

err = c.db.PingContext(ctx)
if err != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "DBPingError", EValue: err.Error()})
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "DBPingError", EValue: err.Error(), ExitCode: 255})
log.Error("DBPingError: error pinging db server: %v", err)
return err
}
Expand All @@ -69,14 +69,14 @@ func (c *Controller) executeSelectSQLQuery(ctx context.Context, request *Execute

rows, err := c.db.QueryContext(ctx, request.Code)
if err != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "DBQueryError", EValue: err.Error()})
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "DBQueryError", EValue: err.Error(), ExitCode: 255})
return nil
}
defer rows.Close()

columns, err := rows.Columns()
if err != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "DBQueryError", EValue: err.Error()})
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "DBQueryError", EValue: err.Error(), ExitCode: 255})
return nil
}

Expand All @@ -90,7 +90,7 @@ func (c *Controller) executeSelectSQLQuery(ctx context.Context, request *Execute
for rows.Next() {
err := rows.Scan(scanArgs...)
if err != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "RowScanError", EValue: err.Error()})
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "RowScanError", EValue: err.Error(), ExitCode: 255})
return nil
}
row := make([]any, len(columns))
Expand All @@ -104,7 +104,7 @@ func (c *Controller) executeSelectSQLQuery(ctx context.Context, request *Execute
result = append(result, row)
}
if err := rows.Err(); err != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "RowIterationError", EValue: err.Error()})
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "RowIterationError", EValue: err.Error(), ExitCode: 255})
return nil
}

Expand All @@ -114,7 +114,7 @@ func (c *Controller) executeSelectSQLQuery(ctx context.Context, request *Execute
}
bytes, err := json.Marshal(queryResult)
if err != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "JSONMarshalError", EValue: err.Error()})
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "JSONMarshalError", EValue: err.Error(), ExitCode: 255})
return nil
}
request.Hooks.OnExecuteResult(
Expand All @@ -133,7 +133,7 @@ func (c *Controller) executeUpdateSQLQuery(ctx context.Context, request *Execute

result, err := c.db.ExecContext(ctx, request.Code)
if err != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "DBExecError", EValue: err.Error()})
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "DBExecError", EValue: err.Error(), ExitCode: 255})
return err
}

Expand All @@ -144,7 +144,7 @@ func (c *Controller) executeUpdateSQLQuery(ctx context.Context, request *Execute
}
bytes, err := json.Marshal(queryResult)
if err != nil {
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "JSONMarshalError", EValue: err.Error()})
request.Hooks.OnExecuteError(&execute.ErrorOutput{EName: "JSONMarshalError", EValue: err.Error(), ExitCode: 255})
return err
}
request.Hooks.OnExecuteResult(
Expand Down
3 changes: 3 additions & 0 deletions components/execd/pkg/runtime/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,6 @@ type bashSession struct {
// Set after cmd.Start(), cleared when run() returns. Used by close() to kill the process group.
currentProcessPid int
}

const commandExecError = "CommandExecError"
const commandInitError = "CommandInitError"
4 changes: 4 additions & 0 deletions specs/execd-api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1063,6 +1063,10 @@ components:
type: string
description: Error value/message
example: "name 'undefined_var' is not defined"
exit_code:
type: integer
description: Non-zero exit code if the command has finished
example: 1
traceback:
type: array
items:
Expand Down
Loading