fix(execd): finish RunCommand SSE responses cleanly - #1281
Conversation
![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: 2cde9611ba
ℹ️ 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".
| eventsHandler.OnExecuteComplete = func(executionTime time.Duration) { | ||
| origComplete(executionTime) | ||
| recordExecution("success") | ||
| signalComplete() |
There was a problem hiding this comment.
When background:true runs on Windows, runtime.runBackgroundCommand starts the process and stores the command kernel inside a goroutine (components/execd/pkg/runtime/command_windows.go:148-166), then calls OnExecuteComplete immediately at line 193. Closing completeCh here lets RunCommand return as soon as that callback fires, so clients that parse the init ID and immediately call /command/status/:id or /command/{id}/logs can get command not found before the goroutine registers the command; the previous fixed grace period largely masked this race. Please only signal completion after background registration is guaranteed, or keep a bounded wait for this path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13080cbd06
ℹ️ 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".
|
|
||
| kernel.pid = cmd.Process.Pid | ||
| c.storeCommandKernel(session, kernel) | ||
| request.Hooks.OnExecuteInit(session) |
There was a problem hiding this comment.
For background commands this init hook is the SSE init write, so it can block behind a slow or stalled HTTP client. Because the cmd.Wait() reaper and the process-group cancellation goroutine are only started after this callback returns, a background command that exits or hits its timeout while the init write is blocked is not reaped promptly and its child process group is not killed; exec.CommandContext only cancels the shell process. Please store the kernel, start the wait/cancel goroutines, and then expose the init event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58dee2a683
ℹ️ 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".
| request.Hooks.OnExecuteInit(session) | ||
| return fmt.Errorf("failed to start commands: %w", err) |
There was a problem hiding this comment.
When a Windows background command reaches this path because cmd.Start() fails, OnExecuteInit has already committed the response as an SSE stream, but returning an error sends control back to RunCommand's JSON RespondError path instead of emitting an SSE error/complete event. In that failure scenario (for example if the shell cannot be spawned or process creation fails), clients receive a malformed event stream and no structured terminal event; handle the failure through the hooks or avoid writing init before returning the error.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes #1277.
Bring
RunCommandonto the same completion-channel flow already used byRunCodeandRunInSession, so execd finishes chunked SSE responses cleanly instead of sleeping for the full graceful-shutdown timeout after execution has already completed.Validation
go test ./pkg/web/controller -run 'TestRunCommandReturnsBeforeGracefulShutdownTimeoutAfterImmediate(Completion|Error)$'gofmt -w pkg/web/controller/command.go pkg/web/controller/command_test.go && go test ./pkg/web/controllergit diff --check