fix(shell): prevent hang when command backgrounds a child process#7689
fix(shell): prevent hang when command backgrounds a child process#7689lifeizhou-ap merged 1 commit intoblock:mainfrom
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: 883f3f3a6c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77629b2ced
ℹ️ 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".
lifeizhou-ap
left a comment
lifeizhou-ap
left a comment
There was a problem hiding this comment.
Nice fix — the channel + drain timeout approach is clean and solves the problem well. One minor thought:
Drain timeout duration: OUTPUT_DRAIN_TIMEOUT_SECS is currently 5 seconds. In practice, useful output arrives in the channel almost instantly (before the process even exits), so the 5s is really just a safety margin. Would you consider reducing it to 1-2 seconds? It would make commands with accidental background processes feel snappier. Not a blocker — 5s is fine too, just something to consider.
Yeah. that makes sense thanks. I've mentioned 1s in the commit message whereas somehow it was 5s in the code. We can change it to milliseconds as well. |
When a command spawns a background process that inherits stdout/stderr pipes, the pipe reader blocks forever after the shell exits. Fix by streaming output through an mpsc channel with a 500ms drain timeout after child.wait() returns, so captured output is returned without waiting for the grandchild to exit. Change-Id: Ib3151111ef06b841b82cd3101444e07bfdca6573 Signed-off-by: rabi <[email protected]>
* main: (191 commits) fix: add tool_choice and parallel_tool_calls to chatgpt_codex provider (#7867) fix: tool confirmation handling for multiple requests (#7856) Remove dead OllamaSetup onboarding flow (#7861) fix: resolve tokio::sync::Mutex deadlock in recipe retry path (#7832) Upgrade Electron 40.6.0 → 41.0.0 (#7851) Only show up to 50 lines of source code (#7578) fix: stop writing without error when hitting broken pipe for goose session list (#7858) feat(acp): add session/set_mode handler (#7801) Keep messages in sync (#7850) More acp tools (#7843) fix: skip upgrade-insecure-requests CSP for external HTTP backends (#7714) fix(shell): prevent hang when command backgrounds a child process (#7689) Remove include from Cargo.toml in goose-mcp (#7838) Exit agent loop when tool call JSON fails to parse (#7840) chore: remove redundant husky prepare script (#7829) Add github actions workflow for unused deps (#7681) fix: prevent SSE connection drops from silently truncating responses (#7831) doc: Added notes in contribution guide for pnpm (#7833) add prefer-offline to pnpm config to skip unnecessary registry lookups (#7828) fix: remove dead read handler from DeveloperClient (#7821) ...
* main: (65 commits) feat(otel): propagate session.id to spans and log records (#7490) fix(test): add env_lock to is_openai_reasoning_model tests (#7917) fix(acp): pass session_id when loading extensions so skills are discovered (#7868) updated canonical models (#7920) feat(autovisualiser): Migrate the autovisualiser extension to MCP Apps (#7852) fix: add tool_choice and parallel_tool_calls to chatgpt_codex provider (#7867) fix: tool confirmation handling for multiple requests (#7856) Remove dead OllamaSetup onboarding flow (#7861) fix: resolve tokio::sync::Mutex deadlock in recipe retry path (#7832) Upgrade Electron 40.6.0 → 41.0.0 (#7851) Only show up to 50 lines of source code (#7578) fix: stop writing without error when hitting broken pipe for goose session list (#7858) feat(acp): add session/set_mode handler (#7801) Keep messages in sync (#7850) More acp tools (#7843) fix: skip upgrade-insecure-requests CSP for external HTTP backends (#7714) fix(shell): prevent hang when command backgrounds a child process (#7689) Remove include from Cargo.toml in goose-mcp (#7838) Exit agent loop when tool call JSON fails to parse (#7840) chore: remove redundant husky prepare script (#7829) ...
2 participants
Summary
When a command spawns a background process that inherits stdout/stderr pipes, the pipe reader blocks forever after the shell exits. Fix by streaming output through an mpsc channel with a 1-second drain timeout after child.wait() returns, so captured output is returned without waiting for the grandchild to exit.
Type of Change
AI Assistance
Testing
Unit tests and tested locally.
Screenshots/Demos (for UX changes)
Before:
stuck forever...
After: