Skip to content

agent: fix missing return#100

Merged
emilyalbini merged 1 commit intomainfrom
ea-mvulumtytlot
May 7, 2026
Merged

agent: fix missing return#100
emilyalbini merged 1 commit intomainfrom
ea-mvulumtytlot

Conversation

@emilyalbini
Copy link
Copy Markdown
Member

Discovered this while working on post tasks. I think it's missing a return here, because we'd panic if the code proceeded from here (due to the assert a few lines below).

Comment thread agent/src/exec.rs
* No further notifications are required for background
* processes.
*/
return;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is probably not quite sufficient. In the error path above, it looks like we'll also slip down to that assert in the case that this is a background process? I agree that this code is all a bit confused, alas.

I think we need to do the wait() either way, otherwise we might end up with zombie children, but we may want to exit early in the error path as well for background processes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a return in the error case too!

@emilyalbini emilyalbini force-pushed the ea-mvulumtytlot branch 2 times, most recently from fe9663f to cf9c86e Compare May 4, 2026 09:19
@emilyalbini emilyalbini changed the base branch from main to ea-xsxwxqsvylmq May 4, 2026 09:19
Base automatically changed from ea-xsxwxqsvylmq to main May 4, 2026 09:40
Copy link
Copy Markdown
Collaborator

@jclulow jclulow left a comment

Choose a reason for hiding this comment

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

Thanks, I think that looks right now.

@emilyalbini emilyalbini merged commit 6723608 into main May 7, 2026
4 checks passed
@emilyalbini emilyalbini deleted the ea-mvulumtytlot branch May 7, 2026 12:44
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.

2 participants