Skip to content

agent spawn child subprocess not grandchild #2583

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

agent spawn child subprocess not grandchild #2583

Show file tree
Hide file tree
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
6 changes: 2 additions & 4 deletions agent/agent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ def get_subprocess_status():
"""Return the subprocess status."""
async_subprocess = state.get("async_subprocess")
message = "Analysis status"
exitcode = async_subprocess.exitcode
exitcode = async_subprocess.poll()
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Consider adding a comment explaining why poll() is used instead of exitcode here. Also, it might be good to check if poll() is the right method for all platforms.

# Use poll() to get the exit code of the subprocess.
# This is more reliable than accessing async_subprocess.exitcode directly.
exitcode = async_subprocess.poll()

if exitcode is None or (sys.platform == "win32" and exitcode == 259):
# Process is still running.
state["status"] = Status.RUNNING
Expand Down Expand Up @@ -713,9 +713,7 @@ def background_subprocess(command_args, cwd, base64_encode, shell=False):

def spawn(args, cwd, base64_encode, shell=False):
"""Kick off a subprocess in the background."""
run_subprocess_args = [args, cwd, base64_encode, shell]
proc = multiprocessing.Process(target=background_subprocess, name=f"child process {args[1]}", args=run_subprocess_args)
proc.start()
proc = subprocess.Popen(args, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=shell)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Consider adding error handling for the subprocess.Popen() call. What happens if the subprocess fails to start? Should we catch the exception and update the agent's state accordingly?

try:
    proc = subprocess.Popen(args, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=shell)
except Exception as e:
    state["status"] = Status.FAILED
    state["description"] = f"Failed to start subprocess: {e}"
    return json_error(500, f"Failed to start subprocess: {e}")

state["status"] = Status.RUNNING
state["description"] = ""
state["async_subprocess"] = proc
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The function doesn't return anything. Should it return a success or failure indicator?

state["async_subprocess"] = proc
    return json_success("Successfully spawned command", process_id=proc.pid)

Expand Down
5 changes: 3 additions & 2 deletions agent/test_agent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import pathlib
import random
import shutil
import subprocess
import sys
import tempfile
import time
Expand Down Expand Up @@ -39,8 +40,8 @@ class TestAgentFunctions:
@mock.patch("sys.platform", "win32")
def test_get_subprocess_259(self):
mock_process_id = 999998
mock_subprocess = mock.Mock(spec=multiprocessing.Process)
mock_subprocess.exitcode = 259
mock_subprocess = mock.Mock(spec=subprocess.Popen)
mock_subprocess.poll = mock.Mock(return_value=259)
mock_subprocess.pid = mock_process_id
with mock.patch.dict(agent.state, {"async_subprocess": mock_subprocess}):
actual = agent.get_subprocess_status()
Expand Down
Loading