fix(execd): return the command SSE handler after execution completes - #1672
Open
vinkiYu wants to merge 1 commit into
Open
fix(execd): return the command SSE handler after execution completes#1672vinkiYu wants to merge 1 commit into
vinkiYu wants to merge 1 commit into
Conversation
RunCommand waited for the execution-complete event and then slept for the full graceful-shutdown timeout (default 1s), adding a fixed tail latency to every foreground command (opensandbox-group#1661: short commands such as true or echo measure ~1.01s end to end). waitForExecutionComplete already bounds the drain window: it returns as soon as the final SSE event is written and flushed, on client disconnect, or at the --graceful-shutdown-timeout deadline. Drop the extra sleep so the command endpoint behaves like RunCode and RunInSession, which have returned immediately after completion since 7f95adb. The flag keeps its meaning as the completion-wait deadline. Add a regression test mirroring the existing RunCode one: the handler must return well before the timeout when execution completes immediately.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the fixed tail-latency from the components/execd foreground /command SSE endpoint by eliminating the unconditional post-completion sleep, aligning RunCommand with the existing early-return behavior already used by RunCode and RunInSession.
Changes:
- Removed the unconditional
time.Sleep(flag.ApiGracefulShutdownTimeout)after command execution completion, relying onwaitForExecutionCompleteas the bounded drain/completion-wait mechanism. - Added a regression unit test to ensure
RunCommandreturns well before the graceful shutdown timeout when execution completes immediately.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| components/execd/pkg/web/controller/command.go | Drops the extra post-completion sleep so the handler can return immediately after the completion event is flushed (bounded by waitForExecutionComplete). |
| components/execd/pkg/web/controller/command_test.go | Adds a regression test verifying RunCommand returns before the configured graceful shutdown timeout on immediate completion. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1661.
Summary
Foreground commands (the
/commandSSE endpoint) pay a fixed ~1s tail latency:RunCommandwaits for the execution-complete event and then unconditionally sleeps for--graceful-shutdown-timeout(default1s), so short commands such astrue,echo, ormkdirmeasure ~1.01s end to end (measurements in #1661).waitForExecutionCompletealready bounds the drain window: it returns as soon as the final SSE event is written and flushed, on client disconnect, or at the--graceful-shutdown-timeoutdeadline. This drops the extra sleep so the command endpoint behaves likeRunCodeandRunInSession, which adopted exactly this early-return pattern in 7f95adb ("return immediately to avoid fixed tail latency").--graceful-shutdown-timeoutkeeps its meaning as the completion-wait deadline, so deployments that want a longer safety window can still tune it.On the egress-sidecar concern from the old comment:
RunCode/RunInSessionhave returned immediately after the completion callback since 7f95adb over the same runner / SSE / egress topology, so this aligns the command endpoint with behavior that has already shipped.Testing
New
TestRunCommandReturnsBeforeGracefulShutdownTimeoutAfterImmediateCompletemirrors the existingRunCoderegression test: the handler must return well before the timeout when execution completes immediately. It fails on the old code and passes with this change. The rest of thepkg/web/controllersuite shows no new failures (TestBackgroundRun_HTTPFlowandTestGetRunLogs_RejectsMalformedCursorfail identically on unmodifiedmainin a Windows environment; Linux CI is unaffected).Breaking Changes
The intended behavior change is the latency fix itself: the SSE response now closes right after the final event instead of holding the connection open for the grace window. The flag still bounds the completion wait, so no configuration migration is needed.
Checklist