AIMVT-303: Implement HTTP client in ParallelHTTPClient class - #368
Conversation
adcf75b to
bed8ee3
Compare
atnair-amd
left a comment
atnair-amd
left a comment
There was a problem hiding this comment.
Request changes
The command-execution path is currently incompatible with the FastAPI agent: the client sends a raw JSON body without an application/json content type, and the server rejects it with HTTP 422.
Once that is corrected, the command timeout needs to be decoupled from the HTTP response deadline. An agent-side timeout can legitimately require its termination grace period before it can return an ExecResponse; the client currently reports that healthy agent as an unreachable host.
The remaining inline comments cover timeout policy for health/shutdown, FILE-mode response-path validation, connection-pool fan-out at scale, and propagation of agent execution state.
I reproduced the reported behaviors with local loopback servers and the actual FastAPI agent implementation. No existing review comments were present when this review was prepared.
atnair-amd
left a comment
atnair-amd
left a comment
There was a problem hiding this comment.
The six prior findings are addressed in 8f6c8be1 and their threads can be resolved.
These two follow-ups are non-blocking:
- default
connect_timeout=Nonedisables the TCP connection phase timeout; - positive sub-second timeouts can round to zero.
Neither affects the corrected normal command path, but both are worth addressing when tightening timeout semantics.
| hosts = list(self._agent_urls) | ||
| if host_args is not None: | ||
| if len(host_args) != len(hosts): | ||
| raise ValueError(f"host_args has {len(host_args)} entries but there are {len(hosts)} hosts") |
There was a problem hiding this comment.
Non-blocking: When connect_timeout is omitted, this passes connect=None to httpx.Timeout, which disables the TCP connection deadline. That means health, shutdown, and command calls can wait for the OS-level SYN timeout rather than the selected read deadline when a host black-holes connection attempts. Please let httpx.Timeout(read_timeout) apply its finite connection phase by default, or define an explicit finite default.
| response.raise_for_status() | ||
| exec_response = messages.parse_message(messages.ExecResponse, response.text) | ||
| stdout, stderr = await self._collect_output(request, exec_response) | ||
| except Exception as exc: # noqa: BLE001 - captured per-host so one bad host doesn't sink the others |
There was a problem hiding this comment.
Low priority: round() turns positive fractional timeouts below 0.5 into zero. For example, read_timeout=0.4 serializes an agent process timeout of 0, immediately terminating a command that could otherwise complete. Consider rejecting non-positive rounded values or using ceil() / a minimum of one second.
… sub-second timeouts to zero. Co-authored-by: Cursor <cursoragent@cursor.com>
… succeeds. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary
ParallelHTTPClient(cvs/core/agent/http_client.py), an async httpx-based client providing API parity withParallelSSHClient:run_command,health,shutdown, lazy shared connection reuse, andrebuild/destroyfor host-set changes.HostOutputdataclass and exception mapping (HTTPConnectionError/HTTPProtocolError) so unreachable vs. failed-request hosts can be distinguished the same wayprune_unreachable_hostsdistinguishes pssh exceptions.stop_on_errors,host_args(per-host command substitution), and inline/file output modes, collecting file-mode output back intolist[str]for a uniform response shape.Stacked on #365 (AIMVT-302) — this PR's diff is limited to
http_client.pyand its tests; base branch will be retargeted tomainonce #365 merges.Jira: AIMVT-303
Test plan
cvs/core/agent/unittests/test_http_client.pycoversrun_command(inline/file/exit-code-only modes,host_args,stop_on_errors),health,shutdown, and exception classification, all against mocked HTTP transport