[1/4] Add executor HTTP request protocol#18581
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/exec-server/src/client.rs
Lines 845 to 850 in d0f4fc9
fail_all_sessions only drains process sessions. After a disconnect, http_body_streams still holds each stream Sender, so HttpResponseBodyStream::recv() may wait forever because the channel never closes and no more deltas can arrive. It also leaves stale request_id registrations in memory until the whole client is dropped.
ℹ️ 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".
d0f4fc9 to
1fc4cd7
Compare
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/exec-server/src/client.rs
Lines 849 to 855 in 1fc4cd7
fail_all_sessions only drains process sessions and never clears http_body_streams. On RpcClientEvent::Disconnected or notification-handler failure, each stream keeps its mpsc::Sender alive, so HttpResponseBodyStream::recv() can block forever on rx.recv() instead of terminating when the transport dies. This can hang callers waiting for streamed HTTP bodies.
ℹ️ 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".
1fc4cd7 to
5b3829a
Compare
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/exec-server/src/client.rs
Lines 869 to 874 in 5b3829a
When the RPC transport disconnects, fail_all_sessions only tears down process sessions and never touches http_body_streams. Active HttpResponseBodyStream receivers can then block forever in recv() because their sender is still retained in the routing map but no more notifications will arrive. Mid-stream network drops should terminate these streams with an error instead of hanging.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/exec-server/src/client.rs
Lines 494 to 499 in 5b3829a
connect handles RpcClientEvent::Disconnected by failing process sessions and returning, but it never clears inner.http_body_streams. Those retained senders keep each HttpResponseBodyStream channel open, so recv() can block forever after transport loss instead of terminating with an error/EOF.
ℹ️ 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".
5b3829a to
53977b8
Compare
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/exec-server/src/client.rs
Lines 494 to 499 in 53977b8
Handle RpcClientEvent::Disconnected by failing process sessions only. It never closes or errors http_body_streams, so each stream's sender remains in the routing map. HttpResponseBodyStream::recv() then awaits forever after a transport drop, leaving callers stuck mid-response with no EOF/error.
ℹ️ 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".
53977b8 to
8edc173
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8edc1730a8
ℹ️ 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".
8edc173 to
1a9358f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a9358ff3f
ℹ️ 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".
3018783 to
c3159bc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3159bc7a2
ℹ️ 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".
c3159bc to
888b4d7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 888b4d7bc6
ℹ️ 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".
26b3c57 to
657aee6
Compare
d94e96f to
fdbc9d9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdbc9d9c78
ℹ️ 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".
| serde_json::from_value(params.unwrap_or(Value::Null))?; | ||
| // Unknown request ids are ignored intentionally: a consumer may have | ||
| // dropped its body stream after receiving the headers. | ||
| if let Some(route) = inner.get_http_body_stream(¶ms.request_id) { |
There was a problem hiding this comment.
Isolate stream routes from reused request IDs
handle_http_body_delta_notification routes frames only by request_id. After a stream is dropped/canceled, that ID is removed and can be reused immediately, but no cancellation is sent upstream. Late bodyDelta frames from the old request can then be delivered into the new stream with the same ID, corrupting bytes or causing spurious seq failures.
Useful? React with 👍 / 👎.
fdbc9d9 to
0519903
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0519903bcd
ℹ️ 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".
| let request_id = params.request_id.clone(); | ||
| let terminal_delta = params.done || params.error.is_some(); | ||
| match route.try_send(params) { | ||
| Some(Ok(())) => {} |
There was a problem hiding this comment.
Remove route after queuing a terminal body delta
handle_http_body_delta_notification does nothing on Some(Ok(())), even when terminal_delta is true. If the consumer drops before reading that queued terminal frame, Drop changes the route to Draining; since terminal has already arrived, no later notification removes it. The request id can remain permanently reserved.
Useful? React with 👍 / 👎.
c58360f to
a0fc08c
Compare
Add typed http/request protocol structs plus client-side response-body stream routing for executor HTTP calls. This keeps later remote MCP HTTP work on a request-scoped transport surface without exposing MCP semantics to the executor. Co-authored-by: Codex <noreply@openai.com>
a0fc08c to
0b2d771
Compare
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59069ca8e6
ℹ️ 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".
| // because they share the same connection-global notification channel as | ||
| // process output. Keep the routing table local to the client so higher | ||
| // layers can consume body chunks like a normal byte stream. | ||
| http_body_streams: ArcSwap<HashMap<String, mpsc::Sender<HttpRequestBodyDeltaNotification>>>, |
There was a problem hiding this comment.
Add missing imports for HTTP stream route types
This commit does not compile: Inner.http_body_streams uses mpsc::Sender<HttpRequestBodyDeltaNotification> but neither tokio::sync::mpsc nor HttpRequestBodyDeltaNotification is imported in client.rs. A clean cargo check -p codex-exec-server --lib at this revision fails with unresolved-name errors, so the change is not buildable.
Useful? React with 👍 / 👎.
| let _ = tx.try_send(HttpRequestBodyDeltaNotification { | ||
| request_id, | ||
| seq: 1, | ||
| delta: Vec::new().into(), | ||
| done: true, | ||
| error: Some(message.clone()), | ||
| }); |
There was a problem hiding this comment.
Propagate disconnect errors when stream buffer is full
In fail_all_http_body_streams, terminal error frames are sent with tx.try_send(...) and the result is ignored. When a body stream buffer is already full, that error frame is dropped; once queued chunks drain, recv() sees channel close as Ok(None). A transport disconnect can therefore be observed as clean EOF with truncated data.
Useful? React with 👍 / 👎.
Co-authored-by: Codex <noreply@openai.com>
Why
Remote streamable HTTP MCP needs a transport-shaped executor primitive before the MCP client can move network I/O to the executor. This layer keeps the executor unaware of MCP and gives later PRs an ordered streaming surface for response bodies.
What
http/requestandhttp/request/bodyDeltaprotocol payloads.Stack
Verification
just fmtcargo check -p codex-exec-server -p codex-rmcp-client --testscargo check -p codex-core --test allcompile-onlygit diff --checkfull-cibranch, including the remote Rust test job.Co-authored-by: Codex noreply@openai.com