Skip to content

fix(acp): synchronously reap ACP child to avoid SIGCHLD race#9023

Open
jh-block wants to merge 3 commits intomainfrom
jhugo/reap-acp-child-process
Open

fix(acp): synchronously reap ACP child to avoid SIGCHLD race#9023
jh-block wants to merge 3 commits intomainfrom
jhugo/reap-acp-child-process

Conversation

@jh-block
Copy link
Copy Markdown
Collaborator

@jh-block jh-block commented May 5, 2026

Fixes #8373.
Supersedes #8790.

The console crate has a bug in its use of select(): any interruption is treated as a fatal error and propagated. We were killing the ACP child and then immediately proceeding, which set up a race; if the child was reaped after select() started, the SIGCHLD interrupting select() caused the program to exit.

Wait for the child to be reaped before proceeding to eliminate the race.

Also, use Stdio::inherit() for child stderr to prevent ACP children which output anything on stderr from messing up the CLI.

Upstream PR fixing the bug in the console crate: console-rs/console#286

Fixes #8373.

The `console` crate has a bug in its use of select(): any interruption
is treated as a fatal error and propagated. We were killing the ACP
child and then immediately proceeding, which set up a race; if the child
was reaped after select() started, the SIGCHLD interrupting select()
caused the program to exit.

Wait for the child to be reaped before proceeding to eliminate the race.

Also, use Stdio::inherit() for child stderr to prevent ACP children
which output anything on stderr from messing up the CLI.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa2cc085d1

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/goose/src/acp/provider.rs
Signed-off-by: jh-block <jhugo@block.xyz>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 302a3ab83a

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/goose/src/acp/provider.rs Outdated
Replace BufReader::split-based line splitting with manual chunked
reads that flush at MAX_LINE_LEN (8 KiB) bytes. A child emitting
\r-terminated progress output or binary data without newlines could
otherwise grow a single Vec<u8> indefinitely while we wait for a
newline that never arrives.

Signed-off-by: jh-block <jhugo@block.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"goose configure" quietly fails when trying to use Claude ACP

1 participant