Skip to content

agent: fix missing return #100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
emilyalbini merged 1 commit into from
May 7, 2026
Merged

agent: fix missing return #100

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions agent/src/exec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,15 @@ fn run_common(
)
.unwrap();

/*
* Only send an exit notification if this is the primary task
* process.
*/
if ab.bgproc.is_none() {
tx.blocking_send(ab.exit(&start, &end, i32::MAX)).unwrap();
if ab.bgproc.is_some() {
/*
* No further notifications are required for background
* processes.
*/
return;
}

tx.blocking_send(ab.exit(&start, &end, i32::MAX)).unwrap();
false
}
Ok(es) => {
Expand All @@ -317,6 +318,7 @@ fn run_common(
* No further notifications are required for background
* processes.
*/
return;
Copy link
Copy Markdown
Collaborator

@jclulow jclulow May 1, 2026

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

@emilyalbini emilyalbini May 4, 2026

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!

}

if let Some(sig) = es.signal() {
Expand Down