From 6ac1b9cece606fea33c9baa7d5dec01a424720d5 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Sun, 19 Apr 2026 14:06:27 -0400 Subject: [PATCH 01/29] Implement executor HTTP request runner Run http/request calls with executor-side reqwest, returning buffered bodies directly and streaming response bodies through ordered http/request/bodyDelta notifications. Register the executor handler while keeping the API transport-shaped. Co-authored-by: Codex --- codex-rs/Cargo.lock | 1 + codex-rs/exec-server/Cargo.toml | 1 + codex-rs/exec-server/src/http_request.rs | 204 ++++++++++++ codex-rs/exec-server/src/lib.rs | 1 + codex-rs/exec-server/src/server/handler.rs | 11 + codex-rs/exec-server/src/server/registry.rs | 8 + codex-rs/exec-server/tests/http_request.rs | 347 ++++++++++++++++++++ 7 files changed, 573 insertions(+) create mode 100644 codex-rs/exec-server/src/http_request.rs create mode 100644 codex-rs/exec-server/tests/http_request.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 69a9f51b0e1a..684d2be4ed1d 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2166,6 +2166,7 @@ dependencies = [ "ctor 0.6.3", "futures", "pretty_assertions", + "reqwest", "serde", "serde_json", "serial_test", diff --git a/codex-rs/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index 5ca265c6b6b6..e131a2cd4810 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -21,6 +21,7 @@ codex-sandboxing = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-pty = { workspace = true } futures = { workspace = true } +reqwest = { workspace = true, features = ["rustls-tls", "stream"] } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } thiserror = { workspace = true } diff --git a/codex-rs/exec-server/src/http_request.rs b/codex-rs/exec-server/src/http_request.rs new file mode 100644 index 000000000000..740146c6f49e --- /dev/null +++ b/codex-rs/exec-server/src/http_request.rs @@ -0,0 +1,204 @@ +//! Executor-owned HTTP request runner. +//! +//! The remote MCP path uses this module to make HTTP requests from the same +//! environment that owns stdio process execution. Buffered responses return as a +//! normal JSON-RPC result, while streaming responses split headers and body +//! frames so Streamable HTTP clients can process SSE data before EOF. + +use std::time::Duration; + +use codex_app_server_protocol::JSONRPCErrorError; +use futures::StreamExt; +use reqwest::Method; +use reqwest::Url; +use reqwest::header::HeaderMap; +use reqwest::header::HeaderName; +use reqwest::header::HeaderValue; + +use crate::protocol::HTTP_REQUEST_BODY_DELTA_METHOD; +use crate::protocol::HttpHeader; +use crate::protocol::HttpRequestBodyDeltaNotification; +use crate::protocol::HttpRequestParams; +use crate::protocol::HttpRequestResponse; +use crate::rpc::RpcNotificationSender; +use crate::rpc::internal_error; +use crate::rpc::invalid_params; + +/// Default timeout for executor HTTP requests when the protocol omits one. +const DEFAULT_HTTP_REQUEST_TIMEOUT: Duration = Duration::from_secs(30); + +/// Runs one executor HTTP request and returns the JSON-RPC response payload. +/// +/// When `stream_response` is set, the returned body is empty and the response +/// bytes are emitted as ordered `http/request/bodyDelta` notifications. +pub(crate) async fn run_http_request( + params: HttpRequestParams, + notifications: RpcNotificationSender, +) -> Result { + let method = Method::from_bytes(params.method.as_bytes()) + .map_err(|err| invalid_params(format!("http/request method is invalid: {err}")))?; + let url = Url::parse(¶ms.url) + .map_err(|err| invalid_params(format!("http/request url is invalid: {err}")))?; + match url.scheme() { + "http" | "https" => {} + scheme => { + return Err(invalid_params(format!( + "http/request only supports http and https URLs, got {scheme}" + ))); + } + } + + let request_id = if params.stream_response { + Some(params.request_id.clone().ok_or_else(|| { + invalid_params("http/request streamResponse requires requestId".to_string()) + })?) + } else { + None + }; + let timeout = params + .timeout_ms + .map(Duration::from_millis) + .unwrap_or(DEFAULT_HTTP_REQUEST_TIMEOUT); + let headers = build_headers(params.headers)?; + let client = reqwest::Client::builder() + .timeout(timeout) + .build() + .map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?; + + let mut request = client.request(method, url).headers(headers); + if let Some(body) = params.body { + request = request.body(body.into_inner()); + } + + let response = request + .send() + .await + .map_err(|err| internal_error(format!("http/request failed: {err}")))?; + let status = response.status().as_u16(); + let headers = response_headers(response.headers()); + + if let Some(request_id) = request_id { + // The JSON-RPC response carries status and headers; the body follows as + // ordered notifications so callers can begin parsing streaming content + // without waiting for the server to close the HTTP response. + spawn_body_stream(request_id, response, notifications); + return Ok(HttpRequestResponse { + status, + headers, + body: Vec::new().into(), + }); + } + + let body = response.bytes().await.map_err(|err| { + internal_error(format!("failed to read http/request response body: {err}")) + })?; + + Ok(HttpRequestResponse { + status, + headers, + body: body.to_vec().into(), + }) +} + +/// Converts protocol headers into a reqwest header map while preserving repeats. +fn build_headers(headers: Vec) -> Result { + let mut header_map = HeaderMap::new(); + for header in headers { + let name = HeaderName::from_bytes(header.name.as_bytes()) + .map_err(|err| invalid_params(format!("http/request header name is invalid: {err}")))?; + let value = HeaderValue::from_str(&header.value).map_err(|err| { + invalid_params(format!( + "http/request header value is invalid for {}: {err}", + header.name + )) + })?; + header_map.append(name, value); + } + Ok(header_map) +} + +/// Converts response headers back into protocol headers with UTF-8 values only. +fn response_headers(headers: &HeaderMap) -> Vec { + headers + .iter() + .filter_map(|(name, value)| { + Some(HttpHeader { + name: name.as_str().to_string(), + value: value.to_str().ok()?.to_string(), + }) + }) + .collect() +} + +/// Spawns the task that bridges a reqwest byte stream to JSON-RPC notifications. +fn spawn_body_stream( + request_id: String, + response: reqwest::Response, + notifications: RpcNotificationSender, +) { + tokio::spawn(async move { + let mut seq = 1; + let mut body = response.bytes_stream(); + while let Some(chunk) = body.next().await { + match chunk { + Ok(bytes) => { + if !send_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id: request_id.clone(), + seq, + delta: bytes.to_vec().into(), + done: false, + error: None, + }, + ) + .await + { + return; + } + seq += 1; + } + Err(err) => { + let _ = send_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: Some(err.to_string()), + }, + ) + .await; + return; + } + } + } + + let _ = send_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: None, + }, + ) + .await; + }); +} + +/// Sends one streamed response-body notification. +/// +/// Returns `false` when the JSON-RPC connection has closed, letting the stream +/// task stop without treating disconnects as executor errors. +async fn send_body_delta( + notifications: &RpcNotificationSender, + delta: HttpRequestBodyDeltaNotification, +) -> bool { + notifications + .notify(HTTP_REQUEST_BODY_DELTA_METHOD, &delta) + .await + .is_ok() +} diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index 067fa0a7c147..36a947f82589 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -6,6 +6,7 @@ mod file_system; mod fs_helper; mod fs_helper_main; mod fs_sandbox; +mod http_request; mod local_file_system; mod local_process; mod process; diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 46f7af90a17d..a8aea1212676 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -6,6 +6,7 @@ use std::sync::atomic::Ordering; use codex_app_server_protocol::JSONRPCErrorError; use crate::ExecServerRuntimePaths; +use crate::http_request::run_http_request; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; use crate::protocol::FsCopyParams; @@ -22,6 +23,8 @@ use crate::protocol::FsRemoveParams; use crate::protocol::FsRemoveResponse; use crate::protocol::FsWriteFileParams; use crate::protocol::FsWriteFileResponse; +use crate::protocol::HttpRequestParams; +use crate::protocol::HttpRequestResponse; use crate::protocol::InitializeParams; use crate::protocol::InitializeResponse; use crate::protocol::ReadParams; @@ -147,6 +150,14 @@ impl ExecServerHandler { session.process().terminate(params).await } + pub(crate) async fn http_request( + &self, + params: HttpRequestParams, + ) -> Result { + self.require_initialized_for("http")?; + run_http_request(params, self.notifications.clone()).await + } + pub(crate) async fn fs_read_file( &self, params: FsReadFileParams, diff --git a/codex-rs/exec-server/src/server/registry.rs b/codex-rs/exec-server/src/server/registry.rs index a57704c50502..bef679b8b873 100644 --- a/codex-rs/exec-server/src/server/registry.rs +++ b/codex-rs/exec-server/src/server/registry.rs @@ -19,6 +19,8 @@ use crate::protocol::FsReadDirectoryParams; use crate::protocol::FsReadFileParams; use crate::protocol::FsRemoveParams; use crate::protocol::FsWriteFileParams; +use crate::protocol::HTTP_REQUEST_METHOD; +use crate::protocol::HttpRequestParams; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeParams; @@ -64,6 +66,12 @@ pub(crate) fn build_router() -> RpcRouter { handler.terminate(params).await }, ); + router.request( + HTTP_REQUEST_METHOD, + |handler: Arc, params: HttpRequestParams| async move { + handler.http_request(params).await + }, + ); router.request( FS_READ_FILE_METHOD, |handler: Arc, params: FsReadFileParams| async move { diff --git a/codex-rs/exec-server/tests/http_request.rs b/codex-rs/exec-server/tests/http_request.rs new file mode 100644 index 000000000000..d18e20fc7ffd --- /dev/null +++ b/codex-rs/exec-server/tests/http_request.rs @@ -0,0 +1,347 @@ +#![cfg(unix)] + +mod common; + +use std::collections::BTreeMap; +use std::time::Duration; + +use codex_app_server_protocol::JSONRPCMessage; +use codex_app_server_protocol::JSONRPCNotification; +use codex_app_server_protocol::JSONRPCResponse; +use codex_app_server_protocol::RequestId; +use codex_exec_server::HttpHeader; +use codex_exec_server::HttpRequestBodyDeltaNotification; +use codex_exec_server::HttpRequestParams; +use codex_exec_server::HttpRequestResponse; +use codex_exec_server::InitializeParams; +use common::exec_server::ExecServerHarness; +use common::exec_server::exec_server; +use pretty_assertions::assert_eq; +use serde::de::DeserializeOwned; +use serde_json::Value; +use tokio::io::AsyncBufReadExt; +use tokio::io::AsyncReadExt; +use tokio::io::AsyncWriteExt; +use tokio::io::BufReader; +use tokio::net::TcpListener; +use tokio::net::TcpStream; +use tokio::time::timeout; + +/// HTTP request captured by the ad-hoc TCP server in these integration tests. +#[derive(Debug)] +struct CapturedHttpRequest { + stream: TcpStream, + request_line: String, + headers: BTreeMap, + body: Vec, +} + +/// What this tests: a real exec-server websocket `http/request` performs one +/// HTTP request through the runner and returns the complete response body in +/// the JSON-RPC response. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn exec_server_http_request_buffers_response_body() -> anyhow::Result<()> { + // Phase 1: start exec-server and complete the JSON-RPC handshake. + let mut server = exec_server().await?; + initialize_exec_server(&mut server).await?; + + // Phase 2: start a local HTTP peer and ask exec-server to POST to it. + let listener = TcpListener::bind("127.0.0.1:0").await?; + let url = format!("http://{}/mcp?case=buffered", listener.local_addr()?); + let http_request_id = server + .send_request( + "http/request", + serde_json::to_value(HttpRequestParams { + method: "POST".to_string(), + url, + headers: vec![HttpHeader { + name: "x-codex-test".to_string(), + value: "buffered".to_string(), + }], + body: Some(b"request-body".to_vec().into()), + timeout_ms: Some(5_000), + request_id: None, + stream_response: false, + })?, + ) + .await?; + + // Phase 3: assert the HTTP peer observes the expected method, path, + // headers, and body before returning a fixed-length response. + let captured = accept_http_request(&listener).await?; + assert_eq!( + ( + captured.request_line.as_str(), + captured.headers.get("x-codex-test").map(String::as_str), + captured.body.as_slice(), + ), + ( + "POST /mcp?case=buffered HTTP/1.1", + Some("buffered"), + b"request-body".as_slice(), + ) + ); + respond_with_status_and_headers( + captured.stream, + "201 Created", + &[("x-mcp-test", "buffered")], + b"response-body", + ) + .await?; + + // Phase 4: assert exec-server returns status, response headers, and the + // full response body in the JSON-RPC result. + let response: HttpRequestResponse = wait_for_response(&mut server, http_request_id).await?; + assert_eq!( + ( + response.status, + response_header(&response.headers, "x-mcp-test"), + response.body.into_inner(), + ), + (201, Some("buffered".to_string()), b"response-body".to_vec(),) + ); + + server.shutdown().await?; + Ok(()) +} + +/// What this tests: a real exec-server websocket `http/request` can return +/// response headers immediately and stream the response body as ordered +/// `http/request/bodyDelta` notifications. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn exec_server_http_request_streams_response_body_notifications() -> anyhow::Result<()> { + // Phase 1: start exec-server and complete the JSON-RPC handshake. + let mut server = exec_server().await?; + initialize_exec_server(&mut server).await?; + + // Phase 2: start a local HTTP peer and ask exec-server for a streamed GET. + let listener = TcpListener::bind("127.0.0.1:0").await?; + let url = format!("http://{}/mcp?case=streaming", listener.local_addr()?); + let http_request_id = server + .send_request( + "http/request", + serde_json::to_value(HttpRequestParams { + method: "GET".to_string(), + url, + headers: vec![HttpHeader { + name: "accept".to_string(), + value: "text/event-stream".to_string(), + }], + body: None, + timeout_ms: Some(5_000), + request_id: Some("stream-1".to_string()), + stream_response: true, + })?, + ) + .await?; + + // Phase 3: assert the HTTP peer observes the expected request and then + // respond with chunked transfer encoding to exercise streaming. + let captured = accept_http_request(&listener).await?; + assert_eq!( + ( + captured.request_line.as_str(), + captured.headers.get("accept").map(String::as_str), + captured.body, + ), + ( + "GET /mcp?case=streaming HTTP/1.1", + Some("text/event-stream"), + Vec::new(), + ) + ); + respond_with_chunked_body( + captured.stream, + &[("x-mcp-test", "streaming")], + &[b"hello ".as_slice(), b"world".as_slice()], + ) + .await?; + + // Phase 4: assert the JSON-RPC response contains status and headers but no + // buffered body when streaming is requested. + let response: HttpRequestResponse = wait_for_response(&mut server, http_request_id).await?; + assert_eq!( + ( + response.status, + response_header(&response.headers, "x-mcp-test"), + response.body.into_inner(), + ), + (200, Some("streaming".to_string()), Vec::new()) + ); + + // Phase 5: assert the body notifications are contiguous, ordered, and end + // with a clean terminal frame. + let deltas = collect_response_body_deltas(&mut server, "stream-1").await?; + let seqs = deltas.iter().map(|delta| delta.seq).collect::>(); + let body = deltas + .iter() + .flat_map(|delta| delta.delta.clone().into_inner()) + .collect::>(); + let terminal = deltas.last().map(|delta| (delta.done, delta.error.clone())); + let expected_seqs = (1..=deltas.len() as u64).collect::>(); + assert_eq!( + (seqs, body, terminal), + (expected_seqs, b"hello world".to_vec(), Some((true, None))) + ); + + server.shutdown().await?; + Ok(()) +} + +/// Performs the JSON-RPC initialize handshake required before executor methods. +async fn initialize_exec_server(server: &mut ExecServerHarness) -> anyhow::Result<()> { + let initialize_id = server + .send_request( + "initialize", + serde_json::to_value(InitializeParams { + client_name: "exec-server-http-test".to_string(), + resume_session_id: None, + })?, + ) + .await?; + let _: Value = wait_for_response(server, initialize_id).await?; + server + .send_notification("initialized", serde_json::json!({})) + .await?; + Ok(()) +} + +/// Waits for a typed JSON-RPC response with the requested id. +async fn wait_for_response( + server: &mut ExecServerHarness, + request_id: RequestId, +) -> anyhow::Result +where + T: DeserializeOwned, +{ + let response = server + .wait_for_event(|event| { + matches!( + event, + JSONRPCMessage::Response(JSONRPCResponse { id, .. }) if id == &request_id + ) + }) + .await?; + let JSONRPCMessage::Response(JSONRPCResponse { result, .. }) = response else { + anyhow::bail!("expected JSON-RPC response for {request_id:?}"); + }; + Ok(serde_json::from_value(result)?) +} + +/// Accepts one HTTP/1.1 request and captures its wire-visible fields. +async fn accept_http_request(listener: &TcpListener) -> anyhow::Result { + let (stream, _) = timeout(Duration::from_secs(5), listener.accept()).await??; + let mut reader = BufReader::new(stream); + + let mut request_line = String::new(); + reader.read_line(&mut request_line).await?; + let request_line = request_line.trim_end_matches("\r\n").to_string(); + + let mut headers = BTreeMap::new(); + loop { + let mut line = String::new(); + reader.read_line(&mut line).await?; + if line == "\r\n" { + break; + } + let line = line.trim_end_matches("\r\n"); + let (name, value) = line + .split_once(':') + .ok_or_else(|| anyhow::anyhow!("HTTP header should contain colon: {line}"))?; + headers.insert(name.to_ascii_lowercase(), value.trim().to_string()); + } + + let content_length = headers + .get("content-length") + .and_then(|value| value.parse::().ok()) + .unwrap_or(0); + let mut body = vec![0; content_length]; + reader.read_exact(&mut body).await?; + + Ok(CapturedHttpRequest { + stream: reader.into_inner(), + request_line, + headers, + body, + }) +} + +/// Writes a fixed-length HTTP response to the captured request stream. +async fn respond_with_status_and_headers( + mut stream: TcpStream, + status: &str, + headers: &[(&str, &str)], + body: &[u8], +) -> anyhow::Result<()> { + let extra_headers = headers + .iter() + .map(|(name, value)| format!("{name}: {value}\r\n")) + .collect::(); + let response = format!( + "HTTP/1.1 {status}\r\ncontent-type: text/plain\r\ncontent-length: {}\r\nconnection: close\r\n{extra_headers}\r\n", + body.len(), + ); + stream.write_all(response.as_bytes()).await?; + stream.write_all(body).await?; + stream.flush().await?; + Ok(()) +} + +/// Writes a chunked HTTP response so reqwest must drive the streaming path. +async fn respond_with_chunked_body( + mut stream: TcpStream, + headers: &[(&str, &str)], + chunks: &[&[u8]], +) -> anyhow::Result<()> { + let extra_headers = headers + .iter() + .map(|(name, value)| format!("{name}: {value}\r\n")) + .collect::(); + let response = format!( + "HTTP/1.1 200 OK\r\ncontent-type: text/plain\r\ntransfer-encoding: chunked\r\nconnection: close\r\n{extra_headers}\r\n", + ); + stream.write_all(response.as_bytes()).await?; + for chunk in chunks { + stream + .write_all(format!("{:x}\r\n", chunk.len()).as_bytes()) + .await?; + stream.write_all(chunk).await?; + stream.write_all(b"\r\n").await?; + stream.flush().await?; + } + stream.write_all(b"0\r\n\r\n").await?; + stream.flush().await?; + Ok(()) +} + +/// Collects streamed response-body notifications until the terminal frame. +async fn collect_response_body_deltas( + server: &mut ExecServerHarness, + request_id: &str, +) -> anyhow::Result> { + let mut deltas = Vec::new(); + loop { + let event = server.next_event().await?; + let JSONRPCMessage::Notification(JSONRPCNotification { method, params }) = event else { + anyhow::bail!("expected http/request body delta notification, got {event:?}"); + }; + assert_eq!(method, "http/request/bodyDelta"); + let delta: HttpRequestBodyDeltaNotification = + serde_json::from_value(params.unwrap_or(Value::Null))?; + assert_eq!(delta.request_id, request_id); + + let done = delta.done; + deltas.push(delta); + if done { + return Ok(deltas); + } + } +} + +/// Returns a response header value without depending on header-name casing. +fn response_header(headers: &[HttpHeader], name: &str) -> Option { + headers + .iter() + .find(|header| header.name.eq_ignore_ascii_case(name)) + .map(|header| header.value.clone()) +} From 3ec4764bdbe82e3a10dcd5579c327e21d02ff208 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Mon, 20 Apr 2026 19:53:24 -0700 Subject: [PATCH 02/29] Fix executor HTTP stream lifecycle Co-authored-by: Codex --- codex-rs/exec-server/src/http_request.rs | 164 +++++++------- codex-rs/exec-server/src/server/handler.rs | 92 +++++++- codex-rs/exec-server/src/server/processor.rs | 1 + codex-rs/exec-server/tests/http_request.rs | 212 ++++++++++++++++++- 4 files changed, 390 insertions(+), 79 deletions(-) diff --git a/codex-rs/exec-server/src/http_request.rs b/codex-rs/exec-server/src/http_request.rs index 740146c6f49e..f96a3c3d2e02 100644 --- a/codex-rs/exec-server/src/http_request.rs +++ b/codex-rs/exec-server/src/http_request.rs @@ -20,6 +20,7 @@ use crate::protocol::HttpHeader; use crate::protocol::HttpRequestBodyDeltaNotification; use crate::protocol::HttpRequestParams; use crate::protocol::HttpRequestResponse; +use crate::protocol::HttpRequestTimeout; use crate::rpc::RpcNotificationSender; use crate::rpc::internal_error; use crate::rpc::invalid_params; @@ -27,14 +28,18 @@ use crate::rpc::invalid_params; /// Default timeout for executor HTTP requests when the protocol omits one. const DEFAULT_HTTP_REQUEST_TIMEOUT: Duration = Duration::from_secs(30); +pub(crate) struct PendingHttpBodyStream { + pub(crate) request_id: String, + response: reqwest::Response, +} + /// Runs one executor HTTP request and returns the JSON-RPC response payload. /// /// When `stream_response` is set, the returned body is empty and the response /// bytes are emitted as ordered `http/request/bodyDelta` notifications. pub(crate) async fn run_http_request( params: HttpRequestParams, - notifications: RpcNotificationSender, -) -> Result { +) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> { let method = Method::from_bytes(params.method.as_bytes()) .map_err(|err| invalid_params(format!("http/request method is invalid: {err}")))?; let url = Url::parse(¶ms.url) @@ -55,15 +60,20 @@ pub(crate) async fn run_http_request( } else { None }; - let timeout = params - .timeout_ms - .map(Duration::from_millis) - .unwrap_or(DEFAULT_HTTP_REQUEST_TIMEOUT); let headers = build_headers(params.headers)?; - let client = reqwest::Client::builder() - .timeout(timeout) - .build() - .map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?; + let client = { + let client_builder = match params.timeout_ms { + HttpRequestTimeout::Default => { + reqwest::Client::builder().timeout(DEFAULT_HTTP_REQUEST_TIMEOUT) + } + HttpRequestTimeout::None => reqwest::Client::builder(), + HttpRequestTimeout::Millis(timeout_ms) => { + reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)) + } + }; + client_builder.build() + } + .map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?; let mut request = client.request(method, url).headers(headers); if let Some(body) = params.body { @@ -78,26 +88,31 @@ pub(crate) async fn run_http_request( let headers = response_headers(response.headers()); if let Some(request_id) = request_id { - // The JSON-RPC response carries status and headers; the body follows as - // ordered notifications so callers can begin parsing streaming content - // without waiting for the server to close the HTTP response. - spawn_body_stream(request_id, response, notifications); - return Ok(HttpRequestResponse { - status, - headers, - body: Vec::new().into(), - }); + return Ok(( + HttpRequestResponse { + status, + headers, + body: Vec::new().into(), + }, + Some(PendingHttpBodyStream { + request_id, + response, + }), + )); } let body = response.bytes().await.map_err(|err| { internal_error(format!("failed to read http/request response body: {err}")) })?; - Ok(HttpRequestResponse { - status, - headers, - body: body.to_vec().into(), - }) + Ok(( + HttpRequestResponse { + status, + headers, + body: body.to_vec().into(), + }, + None, + )) } /// Converts protocol headers into a reqwest header map while preserving repeats. @@ -130,63 +145,64 @@ fn response_headers(headers: &HeaderMap) -> Vec { .collect() } -/// Spawns the task that bridges a reqwest byte stream to JSON-RPC notifications. -fn spawn_body_stream( - request_id: String, - response: reqwest::Response, +/// Bridges one reqwest byte stream to ordered JSON-RPC notifications. +pub(crate) async fn stream_http_body( + pending_stream: PendingHttpBodyStream, notifications: RpcNotificationSender, ) { - tokio::spawn(async move { - let mut seq = 1; - let mut body = response.bytes_stream(); - while let Some(chunk) = body.next().await { - match chunk { - Ok(bytes) => { - if !send_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id: request_id.clone(), - seq, - delta: bytes.to_vec().into(), - done: false, - error: None, - }, - ) - .await - { - return; - } - seq += 1; - } - Err(err) => { - let _ = send_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id, - seq, - delta: Vec::new().into(), - done: true, - error: Some(err.to_string()), - }, - ) - .await; + let PendingHttpBodyStream { + request_id, + response, + } = pending_stream; + let mut seq = 1; + let mut body = response.bytes_stream(); + while let Some(chunk) = body.next().await { + match chunk { + Ok(bytes) => { + if !send_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id: request_id.clone(), + seq, + delta: bytes.to_vec().into(), + done: false, + error: None, + }, + ) + .await + { return; } + seq += 1; + } + Err(err) => { + let _ = send_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: Some(err.to_string()), + }, + ) + .await; + return; } } + } - let _ = send_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id, - seq, - delta: Vec::new().into(), - done: true, - error: None, - }, - ) - .await; - }); + let _ = send_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: None, + }, + ) + .await; } /// Sends one streamed response-body notification. diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index a8aea1212676..fab9bae77df4 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -1,12 +1,18 @@ +use std::collections::HashMap; +use std::collections::VecDeque; use std::sync::Arc; use std::sync::Mutex as StdMutex; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use codex_app_server_protocol::JSONRPCErrorError; +use tokio::sync::Mutex; +use tokio::task::JoinHandle; use crate::ExecServerRuntimePaths; +use crate::http_request::PendingHttpBodyStream; use crate::http_request::run_http_request; +use crate::http_request::stream_http_body; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; use crate::protocol::FsCopyParams; @@ -34,15 +40,23 @@ use crate::protocol::TerminateResponse; use crate::protocol::WriteParams; use crate::protocol::WriteResponse; use crate::rpc::RpcNotificationSender; +use crate::rpc::invalid_params; use crate::rpc::invalid_request; use crate::server::file_system_handler::FileSystemHandler; use crate::server::session_registry::SessionHandle; use crate::server::session_registry::SessionRegistry; +#[derive(Default)] +struct HttpBodyStreamRegistry { + reserved_ids: HashMap>>, + pending: VecDeque, +} + pub(crate) struct ExecServerHandler { session_registry: Arc, notifications: RpcNotificationSender, session: StdMutex>, + http_body_streams: Mutex, file_system: FileSystemHandler, initialize_requested: AtomicBool, initialized: AtomicBool, @@ -58,6 +72,7 @@ impl ExecServerHandler { session_registry, notifications, session: StdMutex::new(None), + http_body_streams: Mutex::new(HttpBodyStreamRegistry::default()), file_system: FileSystemHandler::new(runtime_paths), initialize_requested: AtomicBool::new(false), initialized: AtomicBool::new(false), @@ -65,6 +80,16 @@ impl ExecServerHandler { } pub(crate) async fn shutdown(&self) { + let mut http_body_streams = self.http_body_streams.lock().await; + for task in http_body_streams + .reserved_ids + .drain() + .filter_map(|(_, task)| task) + { + task.abort(); + } + http_body_streams.pending.clear(); + drop(http_body_streams); if let Some(session) = self.session() { session.detach().await; } @@ -155,7 +180,54 @@ impl ExecServerHandler { params: HttpRequestParams, ) -> Result { self.require_initialized_for("http")?; - run_http_request(params, self.notifications.clone()).await + let request_id = if params.stream_response { + Some(params.request_id.clone().ok_or_else(|| { + invalid_params("http/request streamResponse requires requestId".to_string()) + })?) + } else { + None + }; + if let Some(request_id) = request_id.as_deref() { + self.reserve_http_body_stream(request_id).await?; + } + match run_http_request(params).await { + Ok((response, pending_stream)) => { + if let Some(pending_stream) = pending_stream { + let mut http_body_streams = self.http_body_streams.lock().await; + http_body_streams.pending.push_back(pending_stream); + } + Ok(response) + } + Err(error) => { + if let Some(request_id) = request_id.as_deref() { + self.release_http_body_stream(request_id).await; + } + Err(error) + } + } + } + + pub(crate) async fn start_pending_http_body_streams(self: &Arc) { + let pending_streams = { + let mut http_body_streams = self.http_body_streams.lock().await; + http_body_streams.pending.drain(..).collect::>() + }; + for pending_stream in pending_streams { + let request_id = pending_stream.request_id.clone(); + let finished_request_id = request_id.clone(); + let handler = Arc::clone(self); + let notifications = self.notifications.clone(); + let task = tokio::spawn(async move { + stream_http_body(pending_stream, notifications).await; + handler.release_http_body_stream(&finished_request_id).await; + }); + let mut http_body_streams = self.http_body_streams.lock().await; + if let Some(entry) = http_body_streams.reserved_ids.get_mut(&request_id) { + *entry = Some(task); + } else { + task.abort(); + } + } } pub(crate) async fn fs_read_file( @@ -253,6 +325,24 @@ impl ExecServerHandler { .unwrap_or_else(std::sync::PoisonError::into_inner) .clone() } + + async fn reserve_http_body_stream(&self, request_id: &str) -> Result<(), JSONRPCErrorError> { + let mut http_body_streams = self.http_body_streams.lock().await; + if http_body_streams.reserved_ids.contains_key(request_id) { + return Err(invalid_params(format!( + "http/request streamResponse requestId `{request_id}` is already active" + ))); + } + http_body_streams + .reserved_ids + .insert(request_id.to_string(), None); + Ok(()) + } + + async fn release_http_body_stream(&self, request_id: &str) { + let mut http_body_streams = self.http_body_streams.lock().await; + http_body_streams.reserved_ids.remove(request_id); + } } #[cfg(test)] diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index 1153bc83e2fd..503dcf1b6eed 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -106,6 +106,7 @@ async fn run_connection( if outgoing_tx.send(message).await.is_err() { break; } + handler.start_pending_http_body_streams().await; } else if outgoing_tx .send(RpcServerOutboundMessage::Error { request_id: request.id, diff --git a/codex-rs/exec-server/tests/http_request.rs b/codex-rs/exec-server/tests/http_request.rs index d18e20fc7ffd..46d3e959f830 100644 --- a/codex-rs/exec-server/tests/http_request.rs +++ b/codex-rs/exec-server/tests/http_request.rs @@ -5,6 +5,7 @@ mod common; use std::collections::BTreeMap; use std::time::Duration; +use codex_app_server_protocol::JSONRPCError; use codex_app_server_protocol::JSONRPCMessage; use codex_app_server_protocol::JSONRPCNotification; use codex_app_server_protocol::JSONRPCResponse; @@ -13,6 +14,7 @@ use codex_exec_server::HttpHeader; use codex_exec_server::HttpRequestBodyDeltaNotification; use codex_exec_server::HttpRequestParams; use codex_exec_server::HttpRequestResponse; +use codex_exec_server::HttpRequestTimeout; use codex_exec_server::InitializeParams; use common::exec_server::ExecServerHarness; use common::exec_server::exec_server; @@ -25,6 +27,7 @@ use tokio::io::AsyncWriteExt; use tokio::io::BufReader; use tokio::net::TcpListener; use tokio::net::TcpStream; +use tokio::sync::oneshot; use tokio::time::timeout; /// HTTP request captured by the ad-hoc TCP server in these integration tests. @@ -59,7 +62,7 @@ async fn exec_server_http_request_buffers_response_body() -> anyhow::Result<()> value: "buffered".to_string(), }], body: Some(b"request-body".to_vec().into()), - timeout_ms: Some(5_000), + timeout_ms: HttpRequestTimeout::Millis(5_000), request_id: None, stream_response: false, })?, @@ -128,7 +131,7 @@ async fn exec_server_http_request_streams_response_body_notifications() -> anyho value: "text/event-stream".to_string(), }], body: None, - timeout_ms: Some(5_000), + timeout_ms: HttpRequestTimeout::Millis(5_000), request_id: Some("stream-1".to_string()), stream_response: true, })?, @@ -157,9 +160,15 @@ async fn exec_server_http_request_streams_response_body_notifications() -> anyho ) .await?; - // Phase 4: assert the JSON-RPC response contains status and headers but no + // Phase 4: assert the JSON-RPC response reaches the wire before any body + // delta notifications, and that it contains status and headers but no // buffered body when streaming is requested. - let response: HttpRequestResponse = wait_for_response(&mut server, http_request_id).await?; + let first_event = server.next_event().await?; + let JSONRPCMessage::Response(JSONRPCResponse { id, result }) = first_event else { + anyhow::bail!("expected http/request response before body deltas, got {first_event:?}"); + }; + assert_eq!(id, http_request_id); + let response: HttpRequestResponse = serde_json::from_value(result)?; assert_eq!( ( response.status, @@ -188,6 +197,153 @@ async fn exec_server_http_request_streams_response_body_notifications() -> anyho Ok(()) } +/// What this tests: streamed `requestId`s stay reserved until the body stream +/// finishes, so a second in-flight request cannot reuse the same id. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn exec_server_http_request_rejects_duplicate_stream_request_ids() -> anyhow::Result<()> { + let mut server = exec_server().await?; + initialize_exec_server(&mut server).await?; + + let listener = TcpListener::bind("127.0.0.1:0").await?; + let url = format!( + "http://{}/mcp?case=duplicate-stream-id", + listener.local_addr()? + ); + let first_request_id = server + .send_request( + "http/request", + serde_json::to_value(HttpRequestParams { + method: "GET".to_string(), + url: url.clone(), + headers: Vec::new(), + body: None, + timeout_ms: HttpRequestTimeout::None, + request_id: Some("stream-dup".to_string()), + stream_response: true, + })?, + ) + .await?; + + let captured = accept_http_request(&listener).await?; + let (finish_tx, finish_rx) = oneshot::channel(); + let response_task = tokio::spawn(async move { + respond_with_chunked_body_until_finish(captured.stream, &[], &[b"hello"], finish_rx).await + }); + + let _: HttpRequestResponse = wait_for_response(&mut server, first_request_id).await?; + + let duplicate_request_id = server + .send_request( + "http/request", + serde_json::to_value(HttpRequestParams { + method: "GET".to_string(), + url, + headers: Vec::new(), + body: None, + timeout_ms: HttpRequestTimeout::Default, + request_id: Some("stream-dup".to_string()), + stream_response: true, + })?, + ) + .await?; + + let duplicate_response = server + .wait_for_event(|event| { + matches!( + event, + JSONRPCMessage::Error(JSONRPCError { id, .. }) if id == &duplicate_request_id + ) + }) + .await?; + let JSONRPCMessage::Error(JSONRPCError { error, .. }) = duplicate_response else { + anyhow::bail!("expected duplicate requestId error response"); + }; + assert_eq!(error.code, -32602); + assert_eq!( + error.message, + "http/request streamResponse requestId `stream-dup` is already active" + ); + + finish_tx + .send(()) + .expect("response task should still be waiting"); + response_task.await??; + let _ = collect_response_body_deltas(&mut server, "stream-dup").await?; + + server.shutdown().await?; + Ok(()) +} + +/// What this tests: `timeoutMs: null` disables the executor deadline, while an +/// explicit short timeout still fails the same delayed response. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn exec_server_http_request_honors_nullable_timeout() -> anyhow::Result<()> { + let mut server = exec_server().await?; + initialize_exec_server(&mut server).await?; + + let listener = TcpListener::bind("127.0.0.1:0").await?; + let delayed_url = format!( + "http://{}/mcp?case=nullable-timeout", + listener.local_addr()? + ); + let no_timeout_request_id = server + .send_request( + "http/request", + serde_json::to_value(HttpRequestParams { + method: "GET".to_string(), + url: delayed_url.clone(), + headers: Vec::new(), + body: None, + timeout_ms: HttpRequestTimeout::None, + request_id: None, + stream_response: false, + })?, + ) + .await?; + + let captured = accept_http_request(&listener).await?; + let delayed_response = tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(100)).await; + respond_with_status_and_headers(captured.stream, "200 OK", &[], b"slow-success").await + }); + let response: HttpRequestResponse = + wait_for_response(&mut server, no_timeout_request_id).await?; + assert_eq!(response.body.into_inner(), b"slow-success".to_vec()); + delayed_response.await??; + + let timeout_request_id = server + .send_request( + "http/request", + serde_json::to_value(HttpRequestParams { + method: "GET".to_string(), + url: delayed_url, + headers: Vec::new(), + body: None, + timeout_ms: HttpRequestTimeout::Millis(10), + request_id: None, + stream_response: false, + })?, + ) + .await?; + + let captured = accept_http_request(&listener).await?; + let delayed_timeout_response = tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(100)).await; + respond_with_status_and_headers(captured.stream, "200 OK", &[], b"too-late").await + }); + let error = wait_for_error_response(&mut server, timeout_request_id).await?; + assert_eq!(error.code, -32603); + assert!( + error.message.starts_with("http/request failed: "), + "unexpected timeout error: {}", + error.message + ); + delayed_timeout_response.await??; + + server.shutdown().await?; + Ok(()) +} + /// Performs the JSON-RPC initialize handshake required before executor methods. async fn initialize_exec_server(server: &mut ExecServerHarness) -> anyhow::Result<()> { let initialize_id = server @@ -228,6 +384,25 @@ where Ok(serde_json::from_value(result)?) } +/// Waits for a JSON-RPC error with the requested id. +async fn wait_for_error_response( + server: &mut ExecServerHarness, + request_id: RequestId, +) -> anyhow::Result { + let response = server + .wait_for_event(|event| { + matches!( + event, + JSONRPCMessage::Error(JSONRPCError { id, .. }) if id == &request_id + ) + }) + .await?; + let JSONRPCMessage::Error(JSONRPCError { error, .. }) = response else { + anyhow::bail!("expected JSON-RPC error for {request_id:?}"); + }; + Ok(error) +} + /// Accepts one HTTP/1.1 request and captures its wire-visible fields. async fn accept_http_request(listener: &TcpListener) -> anyhow::Result { let (stream, _) = timeout(Duration::from_secs(5), listener.accept()).await??; @@ -314,6 +489,35 @@ async fn respond_with_chunked_body( Ok(()) } +/// Writes a chunked response and keeps the stream open until the test allows EOF. +async fn respond_with_chunked_body_until_finish( + mut stream: TcpStream, + headers: &[(&str, &str)], + chunks: &[&[u8]], + finish_rx: oneshot::Receiver<()>, +) -> anyhow::Result<()> { + let extra_headers = headers + .iter() + .map(|(name, value)| format!("{name}: {value}\r\n")) + .collect::(); + let response = format!( + "HTTP/1.1 200 OK\r\ncontent-type: text/plain\r\ntransfer-encoding: chunked\r\nconnection: close\r\n{extra_headers}\r\n", + ); + stream.write_all(response.as_bytes()).await?; + for chunk in chunks { + stream + .write_all(format!("{:x}\r\n", chunk.len()).as_bytes()) + .await?; + stream.write_all(chunk).await?; + stream.write_all(b"\r\n").await?; + stream.flush().await?; + } + finish_rx.await?; + stream.write_all(b"0\r\n\r\n").await?; + stream.flush().await?; + Ok(()) +} + /// Collects streamed response-body notifications until the terminal frame. async fn collect_response_body_deltas( server: &mut ExecServerHarness, From 3e8f87332d165bbf320222a3398d781467453c46 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Mon, 20 Apr 2026 21:37:45 -0700 Subject: [PATCH 03/29] codex: address PR review feedback (#18582) Co-authored-by: Codex --- .../exec-server/src/client/http_client.rs | 22 +++-- codex-rs/exec-server/tests/http_client.rs | 93 +++++++++++++++++++ 2 files changed, 108 insertions(+), 7 deletions(-) diff --git a/codex-rs/exec-server/src/client/http_client.rs b/codex-rs/exec-server/src/client/http_client.rs index a02a8be9a332..25c620ce938c 100644 --- a/codex-rs/exec-server/src/client/http_client.rs +++ b/codex-rs/exec-server/src/client/http_client.rs @@ -231,13 +231,21 @@ impl Inner { let streams = streams.as_ref().clone(); self.http_body_streams.store(Arc::new(HashMap::new())); for (request_id, tx) in streams { - let _ = tx.try_send(HttpRequestBodyDeltaNotification { - request_id, - seq: 1, - delta: Vec::new().into(), - done: true, - error: Some(message.clone()), - }); + if tx + .try_send(HttpRequestBodyDeltaNotification { + request_id: request_id.clone(), + seq: 1, + delta: Vec::new().into(), + done: true, + error: Some(message.clone()), + }) + .is_err() + { + let mut next_failures = self.http_body_stream_failures.load().as_ref().clone(); + next_failures.insert(request_id, message.clone()); + self.http_body_stream_failures + .store(Arc::new(next_failures)); + } } } diff --git a/codex-rs/exec-server/tests/http_client.rs b/codex-rs/exec-server/tests/http_client.rs index 1462c80168c3..1ef7a9323adb 100644 --- a/codex-rs/exec-server/tests/http_client.rs +++ b/codex-rs/exec-server/tests/http_client.rs @@ -42,6 +42,7 @@ const HTTP_REQUEST_BODY_DELTA_METHOD: &str = "http/request/bodyDelta"; const INITIALIZE_METHOD: &str = "initialize"; const INITIALIZED_METHOD: &str = "initialized"; const TEST_TIMEOUT: Duration = Duration::from_secs(5); +const HTTP_BODY_DELTA_CHANNEL_CAPACITY: u64 = 256; const OVERFLOWING_BODY_DELTA_FRAMES: u64 = 1_024; /// What this tests: the buffered HTTP helper always sends a buffered @@ -742,6 +743,98 @@ async fn http_response_body_stream_fails_when_transport_disconnects() -> Result< Ok(()) } +/// What this tests: transport disconnect still records a terminal stream +/// failure even when the client-side body-delta queue is already full. +#[tokio::test] +async fn http_response_body_stream_reports_disconnect_when_queue_is_full() -> Result<()> { + // Phase 1: fill the queued body-delta route exactly to capacity before the + // response headers arrive, then drop the transport without sending EOF. + let server = spawn_scripted_exec_server(|mut peer| async move { + let (request_id, params) = peer.read_http_request().await?; + assert_eq!( + params, + HttpRequestParams { + method: "GET".to_string(), + url: "https://example.test/mcp/disconnect-full-queue".to_string(), + headers: Vec::new(), + body: None, + timeout_ms: None, + request_id: Some("http-1".to_string()), + stream_response: true, + } + ); + for seq in 1..=HTTP_BODY_DELTA_CHANNEL_CAPACITY { + peer.write_body_delta(HttpRequestBodyDeltaNotification { + request_id: "http-1".to_string(), + seq, + delta: b"x".to_vec().into(), + done: false, + error: None, + }) + .await?; + } + peer.write_response( + request_id, + HttpRequestResponse { + status: 200, + headers: Vec::new(), + body: Vec::new().into(), + }, + ) + .await + }) + .await?; + let client = server.connect_client().await?; + + // Phase 2: start the streaming request and receive headers while the + // queue is already full. + let (_response, mut body_stream) = timeout( + TEST_TIMEOUT, + client.http_request_stream(HttpRequestParams { + method: "GET".to_string(), + url: "https://example.test/mcp/disconnect-full-queue".to_string(), + headers: Vec::new(), + body: None, + timeout_ms: None, + request_id: Some("caller-stream-id".to_string()), + stream_response: false, + }), + ) + .await + .context("streamed http/request should return headers")??; + + // Phase 3: drain the queued chunks and assert the transport disconnect is + // still reported as an error rather than a clean EOF. + let mut chunks = 0; + let error = loop { + match timeout(TEST_TIMEOUT, body_stream.recv()) + .await + .context("disconnect should wake the full queued body stream")? + { + Ok(Some(_chunk)) => { + chunks += 1; + } + Ok(None) => bail!("disconnect with a full queue should not look like clean EOF"), + Err(error) => break error, + } + }; + assert_eq!( + ( + chunks, + error + .to_string() + .starts_with( + "exec-server protocol error: http response stream `http-1` failed: exec-server transport disconnected", + ), + ), + (HTTP_BODY_DELTA_CHANNEL_CAPACITY as usize, true) + ); + + drop(client); + server.finish().await?; + Ok(()) +} + /// What this tests: body-delta backpressure closes the public body stream as /// an error rather than letting callers accept a truncated body as clean EOF. #[tokio::test] From 1c2702c1237d499a10c3772bfaab12ce7082ceb1 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Mon, 20 Apr 2026 19:44:55 -0700 Subject: [PATCH 04/29] Add nullable executor HTTP timeouts Co-authored-by: Codex --- codex-rs/exec-server/src/lib.rs | 1 + codex-rs/exec-server/src/protocol.rs | 91 ++++++++++++++++++++++- codex-rs/exec-server/tests/http_client.rs | 45 +++++------ 3 files changed, 112 insertions(+), 25 deletions(-) diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index 36a947f82589..606668b5903a 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -70,6 +70,7 @@ pub use protocol::HttpHeader; pub use protocol::HttpRequestBodyDeltaNotification; pub use protocol::HttpRequestParams; pub use protocol::HttpRequestResponse; +pub use protocol::HttpRequestTimeout; pub use protocol::InitializeParams; pub use protocol::InitializeResponse; pub use protocol::ProcessOutputChunk; diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 24d1ee1c1e83..136d3dbb7701 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -268,6 +268,53 @@ pub struct HttpHeader { pub value: String, } +/// Nullable timeout value for executor-owned HTTP requests. +/// +/// Omitted means "use the executor default", `null` means "no timeout", and a +/// number means "use exactly this many milliseconds". +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum HttpRequestTimeout { + Default, + None, + Millis(u64), +} + +impl Default for HttpRequestTimeout { + fn default() -> Self { + Self::Default + } +} + +impl HttpRequestTimeout { + fn is_default(timeout: &Self) -> bool { + matches!(timeout, Self::Default) + } +} + +impl Serialize for HttpRequestTimeout { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + match self { + Self::Default | Self::None => serializer.serialize_none(), + Self::Millis(timeout_ms) => serializer.serialize_u64(*timeout_ms), + } + } +} + +impl<'de> Deserialize<'de> for HttpRequestTimeout { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + Ok(match Option::::deserialize(deserializer)? { + Some(timeout_ms) => Self::Millis(timeout_ms), + None => Self::None, + }) + } +} + /// Executor-side HTTP request envelope. /// /// This intentionally stays transport-shaped rather than MCP-shaped so callers @@ -286,9 +333,12 @@ pub struct HttpRequestParams { /// Optional request body bytes. #[serde(default, rename = "bodyBase64")] pub body: Option, - /// Optional request timeout in milliseconds. - #[serde(default)] - pub timeout_ms: Option, + /// Request timeout in milliseconds. + /// + /// Omitted uses the executor default, `null` disables the timeout, and a + /// number applies that exact millisecond deadline. + #[serde(default, skip_serializing_if = "HttpRequestTimeout::is_default")] + pub timeout_ms: HttpRequestTimeout, /// Caller-chosen stream id for `http/request/bodyDelta` notifications. /// /// The id must remain unique on a connection until the terminal body delta @@ -391,3 +441,38 @@ mod base64_bytes { .map_err(serde::de::Error::custom) } } + +#[cfg(test)] +mod tests { + use super::HttpRequestParams; + use super::HttpRequestTimeout; + use pretty_assertions::assert_eq; + + #[test] + fn http_request_timeout_distinguishes_omitted_null_and_number() { + let omitted: HttpRequestParams = serde_json::from_value(serde_json::json!({ + "method": "GET", + "url": "https://example.test", + })) + .expect("omitted timeout should deserialize"); + let null_timeout: HttpRequestParams = serde_json::from_value(serde_json::json!({ + "method": "GET", + "url": "https://example.test", + "timeoutMs": null, + })) + .expect("null timeout should deserialize"); + let explicit_timeout: HttpRequestParams = serde_json::from_value(serde_json::json!({ + "method": "GET", + "url": "https://example.test", + "timeoutMs": 1234, + })) + .expect("numeric timeout should deserialize"); + + assert_eq!(omitted.timeout_ms, HttpRequestTimeout::Default); + assert_eq!(null_timeout.timeout_ms, HttpRequestTimeout::None); + assert_eq!( + explicit_timeout.timeout_ms, + HttpRequestTimeout::Millis(1234) + ); + } +} diff --git a/codex-rs/exec-server/tests/http_client.rs b/codex-rs/exec-server/tests/http_client.rs index 1ef7a9323adb..d13d8d6578a3 100644 --- a/codex-rs/exec-server/tests/http_client.rs +++ b/codex-rs/exec-server/tests/http_client.rs @@ -14,6 +14,7 @@ use codex_exec_server::HttpHeader; use codex_exec_server::HttpRequestBodyDeltaNotification; use codex_exec_server::HttpRequestParams; use codex_exec_server::HttpRequestResponse; +use codex_exec_server::HttpRequestTimeout; use codex_exec_server::InitializeParams; use codex_exec_server::InitializeResponse; use codex_exec_server::RemoteExecServerConnectArgs; @@ -62,7 +63,7 @@ async fn http_request_forces_buffered_request_params() -> Result<()> { url: "https://example.test/buffered".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: None, stream_response: false, } @@ -90,7 +91,7 @@ async fn http_request_forces_buffered_request_params() -> Result<()> { url: "https://example.test/buffered".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("ignored-stream-id".to_string()), stream_response: true, }), @@ -130,7 +131,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta value: "text/event-stream".to_string(), }], body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-1".to_string()), stream_response: true, } @@ -185,7 +186,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta url: "https://example.test/mcp/reuse".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-2".to_string()), stream_response: true, } @@ -214,7 +215,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta value: "text/event-stream".to_string(), }], body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -252,7 +253,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta url: "https://example.test/mcp/reuse".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -288,7 +289,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i url: "https://example.test/mcp/queued-terminal".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-1".to_string()), stream_response: true, } @@ -321,7 +322,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i url: "https://example.test/mcp/retry-queued-terminal".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-2".to_string()), stream_response: true, } @@ -347,7 +348,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i url: "https://example.test/mcp/queued-terminal".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -371,7 +372,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i url: "https://example.test/mcp/retry-queued-terminal".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }; @@ -410,7 +411,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() url: "https://example.test/mcp/cancel".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-1".to_string()), stream_response: true, } @@ -429,7 +430,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() url: "https://example.test/mcp/retry-cancelled".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-2".to_string()), stream_response: true, } @@ -473,7 +474,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() url: "https://example.test/mcp/cancel".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }) @@ -494,7 +495,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() url: "https://example.test/mcp/retry-cancelled".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -540,7 +541,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() url: "https://example.test/mcp/drop".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-1".to_string()), stream_response: true, } @@ -579,7 +580,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() url: "https://example.test/mcp/retry-dropped".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-2".to_string()), stream_response: true, } @@ -614,7 +615,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() url: "https://example.test/mcp/drop".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -646,7 +647,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() url: "https://example.test/mcp/retry-dropped".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -690,7 +691,7 @@ async fn http_response_body_stream_fails_when_transport_disconnects() -> Result< url: "https://example.test/mcp/disconnect".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-1".to_string()), stream_response: true, } @@ -716,7 +717,7 @@ async fn http_response_body_stream_fails_when_transport_disconnects() -> Result< url: "https://example.test/mcp/disconnect".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -851,7 +852,7 @@ async fn http_response_body_stream_reports_backpressure_truncation() -> Result<( url: "https://example.test/mcp/backpressure".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-1".to_string()), stream_response: true, } @@ -893,7 +894,7 @@ async fn http_response_body_stream_reports_backpressure_truncation() -> Result<( url: "https://example.test/mcp/backpressure".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), From fe25997daf35e3709887c3ffa10edca0466f18c9 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Mon, 20 Apr 2026 21:49:39 -0700 Subject: [PATCH 05/29] codex: fix CI failure on PR #18582 Co-authored-by: Codex --- codex-rs/exec-server/src/protocol.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 136d3dbb7701..5e63154d0619 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -272,19 +272,14 @@ pub struct HttpHeader { /// /// Omitted means "use the executor default", `null` means "no timeout", and a /// number means "use exactly this many milliseconds". -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Default)] pub enum HttpRequestTimeout { + #[default] Default, None, Millis(u64), } -impl Default for HttpRequestTimeout { - fn default() -> Self { - Self::Default - } -} - impl HttpRequestTimeout { fn is_default(timeout: &Self) -> bool { matches!(timeout, Self::Default) From a8e7e4ee59bcd69ae3f418121b487c0e0455073d Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Mon, 20 Apr 2026 21:53:15 -0700 Subject: [PATCH 06/29] codex: fix CI failure on PR #18582 Co-authored-by: Codex --- codex-rs/exec-server/tests/http_client.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codex-rs/exec-server/tests/http_client.rs b/codex-rs/exec-server/tests/http_client.rs index d13d8d6578a3..eaa29fd778d0 100644 --- a/codex-rs/exec-server/tests/http_client.rs +++ b/codex-rs/exec-server/tests/http_client.rs @@ -759,7 +759,7 @@ async fn http_response_body_stream_reports_disconnect_when_queue_is_full() -> Re url: "https://example.test/mcp/disconnect-full-queue".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("http-1".to_string()), stream_response: true, } @@ -796,7 +796,7 @@ async fn http_response_body_stream_reports_disconnect_when_queue_is_full() -> Re url: "https://example.test/mcp/disconnect-full-queue".to_string(), headers: Vec::new(), body: None, - timeout_ms: None, + timeout_ms: HttpRequestTimeout::Default, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), From b43e9950d0a62a6d7f5162694924fe11220cbd10 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Mon, 20 Apr 2026 23:10:50 -0700 Subject: [PATCH 07/29] codex: fix exec-server timeout test on linux Co-authored-by: Codex --- codex-rs/exec-server/tests/http_request.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/codex-rs/exec-server/tests/http_request.rs b/codex-rs/exec-server/tests/http_request.rs index 46d3e959f830..d235bba1e48c 100644 --- a/codex-rs/exec-server/tests/http_request.rs +++ b/codex-rs/exec-server/tests/http_request.rs @@ -3,6 +3,7 @@ mod common; use std::collections::BTreeMap; +use std::io::ErrorKind; use std::time::Duration; use codex_app_server_protocol::JSONRPCError; @@ -338,7 +339,11 @@ async fn exec_server_http_request_honors_nullable_timeout() -> anyhow::Result<() "unexpected timeout error: {}", error.message ); - delayed_timeout_response.await??; + match delayed_timeout_response.await? { + Ok(()) => {} + Err(err) if is_expected_peer_disconnect(&err) => {} + Err(err) => return Err(err), + } server.shutdown().await?; Ok(()) @@ -462,6 +467,19 @@ async fn respond_with_status_and_headers( Ok(()) } +fn is_expected_peer_disconnect(err: &anyhow::Error) -> bool { + err.chain().any(|cause| { + cause + .downcast_ref::() + .is_some_and(|io_err| { + matches!( + io_err.kind(), + ErrorKind::BrokenPipe | ErrorKind::ConnectionReset | ErrorKind::UnexpectedEof + ) + }) + }) +} + /// Writes a chunked HTTP response so reqwest must drive the streaming path. async fn respond_with_chunked_body( mut stream: TcpStream, From b79f32f86c5a8a8083f7bcc311e1d1d1ad600698 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 21 Apr 2026 01:17:23 -0700 Subject: [PATCH 08/29] codex: fix clippy failure on PR #18582 Co-authored-by: Codex --- codex-rs/exec-server/src/server/handler.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index fab9bae77df4..e230e1c9285d 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -80,16 +80,19 @@ impl ExecServerHandler { } pub(crate) async fn shutdown(&self) { - let mut http_body_streams = self.http_body_streams.lock().await; - for task in http_body_streams - .reserved_ids - .drain() - .filter_map(|(_, task)| task) - { + let tasks = { + let mut http_body_streams = self.http_body_streams.lock().await; + let tasks = http_body_streams + .reserved_ids + .drain() + .filter_map(|(_, task)| task) + .collect::>(); + http_body_streams.pending.clear(); + tasks + }; + for task in tasks { task.abort(); } - http_body_streams.pending.clear(); - drop(http_body_streams); if let Some(session) = self.session() { session.detach().await; } From 222046f714bae2b4fd056f39433fc6a5963e6ae3 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 21 Apr 2026 13:01:03 -0700 Subject: [PATCH 09/29] codex: simplify exec-server timeout tristate Co-authored-by: Codex --- codex-rs/exec-server/src/http_request.rs | 2 +- codex-rs/exec-server/src/protocol.rs | 14 +++++++------- codex-rs/exec-server/tests/http_request.rs | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/codex-rs/exec-server/src/http_request.rs b/codex-rs/exec-server/src/http_request.rs index f96a3c3d2e02..e25ee2b7b9f1 100644 --- a/codex-rs/exec-server/src/http_request.rs +++ b/codex-rs/exec-server/src/http_request.rs @@ -66,7 +66,7 @@ pub(crate) async fn run_http_request( HttpRequestTimeout::Default => { reqwest::Client::builder().timeout(DEFAULT_HTTP_REQUEST_TIMEOUT) } - HttpRequestTimeout::None => reqwest::Client::builder(), + HttpRequestTimeout::Disabled => reqwest::Client::builder(), HttpRequestTimeout::Millis(timeout_ms) => { reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)) } diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 5e63154d0619..3222176fb7ab 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -268,15 +268,15 @@ pub struct HttpHeader { pub value: String, } -/// Nullable timeout value for executor-owned HTTP requests. +/// Tri-state timeout value for executor-owned HTTP requests. /// -/// Omitted means "use the executor default", `null` means "no timeout", and a -/// number means "use exactly this many milliseconds". +/// Omitted means "use the executor default", `null` means "disable the +/// timeout", and a number means "use exactly this many milliseconds". #[derive(Debug, Clone, PartialEq, Eq, Default)] pub enum HttpRequestTimeout { #[default] Default, - None, + Disabled, Millis(u64), } @@ -292,7 +292,7 @@ impl Serialize for HttpRequestTimeout { S: serde::Serializer, { match self { - Self::Default | Self::None => serializer.serialize_none(), + Self::Default | Self::Disabled => serializer.serialize_none(), Self::Millis(timeout_ms) => serializer.serialize_u64(*timeout_ms), } } @@ -305,7 +305,7 @@ impl<'de> Deserialize<'de> for HttpRequestTimeout { { Ok(match Option::::deserialize(deserializer)? { Some(timeout_ms) => Self::Millis(timeout_ms), - None => Self::None, + None => Self::Disabled, }) } } @@ -464,7 +464,7 @@ mod tests { .expect("numeric timeout should deserialize"); assert_eq!(omitted.timeout_ms, HttpRequestTimeout::Default); - assert_eq!(null_timeout.timeout_ms, HttpRequestTimeout::None); + assert_eq!(null_timeout.timeout_ms, HttpRequestTimeout::Disabled); assert_eq!( explicit_timeout.timeout_ms, HttpRequestTimeout::Millis(1234) diff --git a/codex-rs/exec-server/tests/http_request.rs b/codex-rs/exec-server/tests/http_request.rs index d235bba1e48c..caa5a036c242 100644 --- a/codex-rs/exec-server/tests/http_request.rs +++ b/codex-rs/exec-server/tests/http_request.rs @@ -218,7 +218,7 @@ async fn exec_server_http_request_rejects_duplicate_stream_request_ids() -> anyh url: url.clone(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::None, + timeout_ms: HttpRequestTimeout::Disabled, request_id: Some("stream-dup".to_string()), stream_response: true, })?, @@ -295,7 +295,7 @@ async fn exec_server_http_request_honors_nullable_timeout() -> anyhow::Result<() url: delayed_url.clone(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::None, + timeout_ms: HttpRequestTimeout::Disabled, request_id: None, stream_response: false, })?, From c2992703dc0fe2acd8567bafc71c6a2614851d26 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 21 Apr 2026 13:05:27 -0700 Subject: [PATCH 10/29] codex: use nested option for http timeout Co-authored-by: Codex --- codex-rs/exec-server/src/http_request.rs | 9 ++-- codex-rs/exec-server/src/lib.rs | 1 - codex-rs/exec-server/src/protocol.rs | 56 ++-------------------- codex-rs/exec-server/tests/http_client.rs | 49 ++++++++++--------- codex-rs/exec-server/tests/http_request.rs | 13 +++-- 5 files changed, 38 insertions(+), 90 deletions(-) diff --git a/codex-rs/exec-server/src/http_request.rs b/codex-rs/exec-server/src/http_request.rs index e25ee2b7b9f1..578572259e4a 100644 --- a/codex-rs/exec-server/src/http_request.rs +++ b/codex-rs/exec-server/src/http_request.rs @@ -20,7 +20,6 @@ use crate::protocol::HttpHeader; use crate::protocol::HttpRequestBodyDeltaNotification; use crate::protocol::HttpRequestParams; use crate::protocol::HttpRequestResponse; -use crate::protocol::HttpRequestTimeout; use crate::rpc::RpcNotificationSender; use crate::rpc::internal_error; use crate::rpc::invalid_params; @@ -63,11 +62,9 @@ pub(crate) async fn run_http_request( let headers = build_headers(params.headers)?; let client = { let client_builder = match params.timeout_ms { - HttpRequestTimeout::Default => { - reqwest::Client::builder().timeout(DEFAULT_HTTP_REQUEST_TIMEOUT) - } - HttpRequestTimeout::Disabled => reqwest::Client::builder(), - HttpRequestTimeout::Millis(timeout_ms) => { + None => reqwest::Client::builder().timeout(DEFAULT_HTTP_REQUEST_TIMEOUT), + Some(None) => reqwest::Client::builder(), + Some(Some(timeout_ms)) => { reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)) } }; diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index 606668b5903a..36a947f82589 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -70,7 +70,6 @@ pub use protocol::HttpHeader; pub use protocol::HttpRequestBodyDeltaNotification; pub use protocol::HttpRequestParams; pub use protocol::HttpRequestResponse; -pub use protocol::HttpRequestTimeout; pub use protocol::InitializeParams; pub use protocol::InitializeResponse; pub use protocol::ProcessOutputChunk; diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 3222176fb7ab..11df4d5ff7a2 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -268,48 +268,6 @@ pub struct HttpHeader { pub value: String, } -/// Tri-state timeout value for executor-owned HTTP requests. -/// -/// Omitted means "use the executor default", `null` means "disable the -/// timeout", and a number means "use exactly this many milliseconds". -#[derive(Debug, Clone, PartialEq, Eq, Default)] -pub enum HttpRequestTimeout { - #[default] - Default, - Disabled, - Millis(u64), -} - -impl HttpRequestTimeout { - fn is_default(timeout: &Self) -> bool { - matches!(timeout, Self::Default) - } -} - -impl Serialize for HttpRequestTimeout { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - match self { - Self::Default | Self::Disabled => serializer.serialize_none(), - Self::Millis(timeout_ms) => serializer.serialize_u64(*timeout_ms), - } - } -} - -impl<'de> Deserialize<'de> for HttpRequestTimeout { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - Ok(match Option::::deserialize(deserializer)? { - Some(timeout_ms) => Self::Millis(timeout_ms), - None => Self::Disabled, - }) - } -} - /// Executor-side HTTP request envelope. /// /// This intentionally stays transport-shaped rather than MCP-shaped so callers @@ -332,8 +290,8 @@ pub struct HttpRequestParams { /// /// Omitted uses the executor default, `null` disables the timeout, and a /// number applies that exact millisecond deadline. - #[serde(default, skip_serializing_if = "HttpRequestTimeout::is_default")] - pub timeout_ms: HttpRequestTimeout, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub timeout_ms: Option>, /// Caller-chosen stream id for `http/request/bodyDelta` notifications. /// /// The id must remain unique on a connection until the terminal body delta @@ -440,7 +398,6 @@ mod base64_bytes { #[cfg(test)] mod tests { use super::HttpRequestParams; - use super::HttpRequestTimeout; use pretty_assertions::assert_eq; #[test] @@ -463,11 +420,8 @@ mod tests { })) .expect("numeric timeout should deserialize"); - assert_eq!(omitted.timeout_ms, HttpRequestTimeout::Default); - assert_eq!(null_timeout.timeout_ms, HttpRequestTimeout::Disabled); - assert_eq!( - explicit_timeout.timeout_ms, - HttpRequestTimeout::Millis(1234) - ); + assert_eq!(omitted.timeout_ms, None); + assert_eq!(null_timeout.timeout_ms, Some(None)); + assert_eq!(explicit_timeout.timeout_ms, Some(Some(1234))); } } diff --git a/codex-rs/exec-server/tests/http_client.rs b/codex-rs/exec-server/tests/http_client.rs index eaa29fd778d0..1ef7a9323adb 100644 --- a/codex-rs/exec-server/tests/http_client.rs +++ b/codex-rs/exec-server/tests/http_client.rs @@ -14,7 +14,6 @@ use codex_exec_server::HttpHeader; use codex_exec_server::HttpRequestBodyDeltaNotification; use codex_exec_server::HttpRequestParams; use codex_exec_server::HttpRequestResponse; -use codex_exec_server::HttpRequestTimeout; use codex_exec_server::InitializeParams; use codex_exec_server::InitializeResponse; use codex_exec_server::RemoteExecServerConnectArgs; @@ -63,7 +62,7 @@ async fn http_request_forces_buffered_request_params() -> Result<()> { url: "https://example.test/buffered".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: None, stream_response: false, } @@ -91,7 +90,7 @@ async fn http_request_forces_buffered_request_params() -> Result<()> { url: "https://example.test/buffered".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("ignored-stream-id".to_string()), stream_response: true, }), @@ -131,7 +130,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta value: "text/event-stream".to_string(), }], body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-1".to_string()), stream_response: true, } @@ -186,7 +185,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta url: "https://example.test/mcp/reuse".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-2".to_string()), stream_response: true, } @@ -215,7 +214,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta value: "text/event-stream".to_string(), }], body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -253,7 +252,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta url: "https://example.test/mcp/reuse".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -289,7 +288,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i url: "https://example.test/mcp/queued-terminal".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-1".to_string()), stream_response: true, } @@ -322,7 +321,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i url: "https://example.test/mcp/retry-queued-terminal".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-2".to_string()), stream_response: true, } @@ -348,7 +347,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i url: "https://example.test/mcp/queued-terminal".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -372,7 +371,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i url: "https://example.test/mcp/retry-queued-terminal".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }; @@ -411,7 +410,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() url: "https://example.test/mcp/cancel".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-1".to_string()), stream_response: true, } @@ -430,7 +429,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() url: "https://example.test/mcp/retry-cancelled".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-2".to_string()), stream_response: true, } @@ -474,7 +473,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() url: "https://example.test/mcp/cancel".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }) @@ -495,7 +494,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() url: "https://example.test/mcp/retry-cancelled".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -541,7 +540,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() url: "https://example.test/mcp/drop".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-1".to_string()), stream_response: true, } @@ -580,7 +579,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() url: "https://example.test/mcp/retry-dropped".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-2".to_string()), stream_response: true, } @@ -615,7 +614,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() url: "https://example.test/mcp/drop".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -647,7 +646,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() url: "https://example.test/mcp/retry-dropped".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -691,7 +690,7 @@ async fn http_response_body_stream_fails_when_transport_disconnects() -> Result< url: "https://example.test/mcp/disconnect".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-1".to_string()), stream_response: true, } @@ -717,7 +716,7 @@ async fn http_response_body_stream_fails_when_transport_disconnects() -> Result< url: "https://example.test/mcp/disconnect".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -759,7 +758,7 @@ async fn http_response_body_stream_reports_disconnect_when_queue_is_full() -> Re url: "https://example.test/mcp/disconnect-full-queue".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-1".to_string()), stream_response: true, } @@ -796,7 +795,7 @@ async fn http_response_body_stream_reports_disconnect_when_queue_is_full() -> Re url: "https://example.test/mcp/disconnect-full-queue".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), @@ -852,7 +851,7 @@ async fn http_response_body_stream_reports_backpressure_truncation() -> Result<( url: "https://example.test/mcp/backpressure".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("http-1".to_string()), stream_response: true, } @@ -894,7 +893,7 @@ async fn http_response_body_stream_reports_backpressure_truncation() -> Result<( url: "https://example.test/mcp/backpressure".to_string(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("caller-stream-id".to_string()), stream_response: false, }), diff --git a/codex-rs/exec-server/tests/http_request.rs b/codex-rs/exec-server/tests/http_request.rs index caa5a036c242..002d880ec6e1 100644 --- a/codex-rs/exec-server/tests/http_request.rs +++ b/codex-rs/exec-server/tests/http_request.rs @@ -15,7 +15,6 @@ use codex_exec_server::HttpHeader; use codex_exec_server::HttpRequestBodyDeltaNotification; use codex_exec_server::HttpRequestParams; use codex_exec_server::HttpRequestResponse; -use codex_exec_server::HttpRequestTimeout; use codex_exec_server::InitializeParams; use common::exec_server::ExecServerHarness; use common::exec_server::exec_server; @@ -63,7 +62,7 @@ async fn exec_server_http_request_buffers_response_body() -> anyhow::Result<()> value: "buffered".to_string(), }], body: Some(b"request-body".to_vec().into()), - timeout_ms: HttpRequestTimeout::Millis(5_000), + timeout_ms: Some(Some(5_000)), request_id: None, stream_response: false, })?, @@ -132,7 +131,7 @@ async fn exec_server_http_request_streams_response_body_notifications() -> anyho value: "text/event-stream".to_string(), }], body: None, - timeout_ms: HttpRequestTimeout::Millis(5_000), + timeout_ms: Some(Some(5_000)), request_id: Some("stream-1".to_string()), stream_response: true, })?, @@ -218,7 +217,7 @@ async fn exec_server_http_request_rejects_duplicate_stream_request_ids() -> anyh url: url.clone(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Disabled, + timeout_ms: Some(None), request_id: Some("stream-dup".to_string()), stream_response: true, })?, @@ -241,7 +240,7 @@ async fn exec_server_http_request_rejects_duplicate_stream_request_ids() -> anyh url, headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Default, + timeout_ms: None, request_id: Some("stream-dup".to_string()), stream_response: true, })?, @@ -295,7 +294,7 @@ async fn exec_server_http_request_honors_nullable_timeout() -> anyhow::Result<() url: delayed_url.clone(), headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Disabled, + timeout_ms: Some(None), request_id: None, stream_response: false, })?, @@ -320,7 +319,7 @@ async fn exec_server_http_request_honors_nullable_timeout() -> anyhow::Result<() url: delayed_url, headers: Vec::new(), body: None, - timeout_ms: HttpRequestTimeout::Millis(10), + timeout_ms: Some(Some(10)), request_id: None, stream_response: false, })?, From cc7ee0910b2348a0b2e6e02f14fa9e910689f642 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 21 Apr 2026 14:48:05 -0700 Subject: [PATCH 11/29] codex: drop implicit exec-server http timeout Co-authored-by: Codex --- codex-rs/exec-server/src/http_request.rs | 8 ++------ codex-rs/exec-server/src/protocol.rs | 12 ++++++------ codex-rs/exec-server/tests/http_request.rs | 18 +++++++++--------- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/codex-rs/exec-server/src/http_request.rs b/codex-rs/exec-server/src/http_request.rs index 578572259e4a..81d206ff7cfb 100644 --- a/codex-rs/exec-server/src/http_request.rs +++ b/codex-rs/exec-server/src/http_request.rs @@ -24,9 +24,6 @@ use crate::rpc::RpcNotificationSender; use crate::rpc::internal_error; use crate::rpc::invalid_params; -/// Default timeout for executor HTTP requests when the protocol omits one. -const DEFAULT_HTTP_REQUEST_TIMEOUT: Duration = Duration::from_secs(30); - pub(crate) struct PendingHttpBodyStream { pub(crate) request_id: String, response: reqwest::Response, @@ -62,9 +59,8 @@ pub(crate) async fn run_http_request( let headers = build_headers(params.headers)?; let client = { let client_builder = match params.timeout_ms { - None => reqwest::Client::builder().timeout(DEFAULT_HTTP_REQUEST_TIMEOUT), - Some(None) => reqwest::Client::builder(), - Some(Some(timeout_ms)) => { + None => reqwest::Client::builder(), + Some(timeout_ms) => { reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)) } }; diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 11df4d5ff7a2..90a78d899f2e 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -288,10 +288,10 @@ pub struct HttpRequestParams { pub body: Option, /// Request timeout in milliseconds. /// - /// Omitted uses the executor default, `null` disables the timeout, and a - /// number applies that exact millisecond deadline. + /// Omitted or `null` disables the timeout. A number applies that exact + /// millisecond deadline. #[serde(default, skip_serializing_if = "Option::is_none")] - pub timeout_ms: Option>, + pub timeout_ms: Option, /// Caller-chosen stream id for `http/request/bodyDelta` notifications. /// /// The id must remain unique on a connection until the terminal body delta @@ -401,7 +401,7 @@ mod tests { use pretty_assertions::assert_eq; #[test] - fn http_request_timeout_distinguishes_omitted_null_and_number() { + fn http_request_timeout_treats_omitted_and_null_as_no_timeout() { let omitted: HttpRequestParams = serde_json::from_value(serde_json::json!({ "method": "GET", "url": "https://example.test", @@ -421,7 +421,7 @@ mod tests { .expect("numeric timeout should deserialize"); assert_eq!(omitted.timeout_ms, None); - assert_eq!(null_timeout.timeout_ms, Some(None)); - assert_eq!(explicit_timeout.timeout_ms, Some(Some(1234))); + assert_eq!(null_timeout.timeout_ms, None); + assert_eq!(explicit_timeout.timeout_ms, Some(1234)); } } diff --git a/codex-rs/exec-server/tests/http_request.rs b/codex-rs/exec-server/tests/http_request.rs index 002d880ec6e1..e9c5c67f253c 100644 --- a/codex-rs/exec-server/tests/http_request.rs +++ b/codex-rs/exec-server/tests/http_request.rs @@ -62,7 +62,7 @@ async fn exec_server_http_request_buffers_response_body() -> anyhow::Result<()> value: "buffered".to_string(), }], body: Some(b"request-body".to_vec().into()), - timeout_ms: Some(Some(5_000)), + timeout_ms: Some(5_000), request_id: None, stream_response: false, })?, @@ -131,7 +131,7 @@ async fn exec_server_http_request_streams_response_body_notifications() -> anyho value: "text/event-stream".to_string(), }], body: None, - timeout_ms: Some(Some(5_000)), + timeout_ms: Some(5_000), request_id: Some("stream-1".to_string()), stream_response: true, })?, @@ -217,7 +217,7 @@ async fn exec_server_http_request_rejects_duplicate_stream_request_ids() -> anyh url: url.clone(), headers: Vec::new(), body: None, - timeout_ms: Some(None), + timeout_ms: None, request_id: Some("stream-dup".to_string()), stream_response: true, })?, @@ -274,16 +274,16 @@ async fn exec_server_http_request_rejects_duplicate_stream_request_ids() -> anyh Ok(()) } -/// What this tests: `timeoutMs: null` disables the executor deadline, while an -/// explicit short timeout still fails the same delayed response. +/// What this tests: omitting `timeoutMs` leaves the request unbounded, while +/// an explicit short timeout still fails the same delayed response. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn exec_server_http_request_honors_nullable_timeout() -> anyhow::Result<()> { +async fn exec_server_http_request_honors_optional_timeout() -> anyhow::Result<()> { let mut server = exec_server().await?; initialize_exec_server(&mut server).await?; let listener = TcpListener::bind("127.0.0.1:0").await?; let delayed_url = format!( - "http://{}/mcp?case=nullable-timeout", + "http://{}/mcp?case=optional-timeout", listener.local_addr()? ); let no_timeout_request_id = server @@ -294,7 +294,7 @@ async fn exec_server_http_request_honors_nullable_timeout() -> anyhow::Result<() url: delayed_url.clone(), headers: Vec::new(), body: None, - timeout_ms: Some(None), + timeout_ms: None, request_id: None, stream_response: false, })?, @@ -319,7 +319,7 @@ async fn exec_server_http_request_honors_nullable_timeout() -> anyhow::Result<() url: delayed_url, headers: Vec::new(), body: None, - timeout_ms: Some(Some(10)), + timeout_ms: Some(10), request_id: None, stream_response: false, })?, From 649450c31d468f6cfe4ec88a72c514f7124187bc Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 21 Apr 2026 17:15:38 -0700 Subject: [PATCH 12/29] codex: start http body streams after responses Co-authored-by: Codex --- codex-rs/exec-server/src/rpc.rs | 2 +- codex-rs/exec-server/src/server/handler.rs | 52 ++++++--------- codex-rs/exec-server/src/server/processor.rs | 69 +++++++++++++++++++- codex-rs/exec-server/src/server/registry.rs | 8 --- 4 files changed, 88 insertions(+), 43 deletions(-) diff --git a/codex-rs/exec-server/src/rpc.rs b/codex-rs/exec-server/src/rpc.rs index e82b4a0ea233..d20772c07067 100644 --- a/codex-rs/exec-server/src/rpc.rs +++ b/codex-rs/exec-server/src/rpc.rs @@ -394,7 +394,7 @@ pub(crate) fn internal_error(message: String) -> JSONRPCErrorError { } } -fn decode_request_params

(params: Option) -> Result +pub(crate) fn decode_request_params

(params: Option) -> Result where P: DeserializeOwned, { diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index e230e1c9285d..34716cf532ff 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -1,5 +1,4 @@ use std::collections::HashMap; -use std::collections::VecDeque; use std::sync::Arc; use std::sync::Mutex as StdMutex; use std::sync::atomic::AtomicBool; @@ -49,7 +48,6 @@ use crate::server::session_registry::SessionRegistry; #[derive(Default)] struct HttpBodyStreamRegistry { reserved_ids: HashMap>>, - pending: VecDeque, } pub(crate) struct ExecServerHandler { @@ -87,7 +85,6 @@ impl ExecServerHandler { .drain() .filter_map(|(_, task)| task) .collect::>(); - http_body_streams.pending.clear(); tasks }; for task in tasks { @@ -181,7 +178,7 @@ impl ExecServerHandler { pub(crate) async fn http_request( &self, params: HttpRequestParams, - ) -> Result { + ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> { self.require_initialized_for("http")?; let request_id = if params.stream_response { Some(params.request_id.clone().ok_or_else(|| { @@ -194,13 +191,7 @@ impl ExecServerHandler { self.reserve_http_body_stream(request_id).await?; } match run_http_request(params).await { - Ok((response, pending_stream)) => { - if let Some(pending_stream) = pending_stream { - let mut http_body_streams = self.http_body_streams.lock().await; - http_body_streams.pending.push_back(pending_stream); - } - Ok(response) - } + Ok((response, pending_stream)) => Ok((response, pending_stream)), Err(error) => { if let Some(request_id) = request_id.as_deref() { self.release_http_body_stream(request_id).await; @@ -210,26 +201,23 @@ impl ExecServerHandler { } } - pub(crate) async fn start_pending_http_body_streams(self: &Arc) { - let pending_streams = { - let mut http_body_streams = self.http_body_streams.lock().await; - http_body_streams.pending.drain(..).collect::>() - }; - for pending_stream in pending_streams { - let request_id = pending_stream.request_id.clone(); - let finished_request_id = request_id.clone(); - let handler = Arc::clone(self); - let notifications = self.notifications.clone(); - let task = tokio::spawn(async move { - stream_http_body(pending_stream, notifications).await; - handler.release_http_body_stream(&finished_request_id).await; - }); - let mut http_body_streams = self.http_body_streams.lock().await; - if let Some(entry) = http_body_streams.reserved_ids.get_mut(&request_id) { - *entry = Some(task); - } else { - task.abort(); - } + pub(crate) async fn start_http_body_stream( + self: &Arc, + pending_stream: PendingHttpBodyStream, + ) { + let request_id = pending_stream.request_id.clone(); + let finished_request_id = request_id.clone(); + let handler = Arc::clone(self); + let notifications = self.notifications.clone(); + let task = tokio::spawn(async move { + stream_http_body(pending_stream, notifications).await; + handler.release_http_body_stream(&finished_request_id).await; + }); + let mut http_body_streams = self.http_body_streams.lock().await; + if let Some(entry) = http_body_streams.reserved_ids.get_mut(&request_id) { + *entry = Some(task); + } else { + task.abort(); } } @@ -342,7 +330,7 @@ impl ExecServerHandler { Ok(()) } - async fn release_http_body_stream(&self, request_id: &str) { + pub(crate) async fn release_http_body_stream(&self, request_id: &str) { let mut http_body_streams = self.http_body_streams.lock().await; http_body_streams.reserved_ids.remove(request_id); } diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index 503dcf1b6eed..eeea9eeb59c4 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use serde_json::to_value; use tokio::sync::mpsc; use tracing::debug; use tracing::warn; @@ -8,9 +9,13 @@ use crate::ExecServerRuntimePaths; use crate::connection::CHANNEL_CAPACITY; use crate::connection::JsonRpcConnection; use crate::connection::JsonRpcConnectionEvent; +use crate::protocol::HTTP_REQUEST_METHOD; +use crate::protocol::HttpRequestParams; use crate::rpc::RpcNotificationSender; use crate::rpc::RpcServerOutboundMessage; +use crate::rpc::decode_request_params; use crate::rpc::encode_server_message; +use crate::rpc::internal_error; use crate::rpc::invalid_request; use crate::rpc::method_not_found; use crate::server::ExecServerHandler; @@ -95,7 +100,68 @@ async fn run_connection( } JsonRpcConnectionEvent::Message(message) => match message { codex_app_server_protocol::JSONRPCMessage::Request(request) => { - if let Some(route) = router.request_route(request.method.as_str()) { + if request.method == HTTP_REQUEST_METHOD { + let request_id = request.id.clone(); + let params = + match decode_request_params::(request.params) { + Ok(params) => params, + Err(error) => { + if outgoing_tx + .send(RpcServerOutboundMessage::Error { request_id, error }) + .await + .is_err() + { + break; + } + continue; + } + }; + let response = tokio::select! { + response = handler.http_request(params) => response, + _ = disconnected_rx.changed() => { + debug!("exec-server transport disconnected while handling request"); + break; + } + }; + match response { + Ok((response, pending_stream)) => { + let mut pending_stream = pending_stream; + let message = match to_value(response) { + Ok(result) => { + RpcServerOutboundMessage::Response { request_id, result } + } + Err(err) => { + if let Some(pending_stream) = pending_stream.take() { + handler + .release_http_body_stream( + &pending_stream.request_id, + ) + .await; + } + RpcServerOutboundMessage::Error { + request_id, + error: internal_error(err.to_string()), + } + } + }; + if outgoing_tx.send(message).await.is_err() { + break; + } + if let Some(pending_stream) = pending_stream { + handler.start_http_body_stream(pending_stream).await; + } + } + Err(error) => { + if outgoing_tx + .send(RpcServerOutboundMessage::Error { request_id, error }) + .await + .is_err() + { + break; + } + } + } + } else if let Some(route) = router.request_route(request.method.as_str()) { let message = tokio::select! { message = route(Arc::clone(&handler), request) => message, _ = disconnected_rx.changed() => { @@ -106,7 +172,6 @@ async fn run_connection( if outgoing_tx.send(message).await.is_err() { break; } - handler.start_pending_http_body_streams().await; } else if outgoing_tx .send(RpcServerOutboundMessage::Error { request_id: request.id, diff --git a/codex-rs/exec-server/src/server/registry.rs b/codex-rs/exec-server/src/server/registry.rs index bef679b8b873..a57704c50502 100644 --- a/codex-rs/exec-server/src/server/registry.rs +++ b/codex-rs/exec-server/src/server/registry.rs @@ -19,8 +19,6 @@ use crate::protocol::FsReadDirectoryParams; use crate::protocol::FsReadFileParams; use crate::protocol::FsRemoveParams; use crate::protocol::FsWriteFileParams; -use crate::protocol::HTTP_REQUEST_METHOD; -use crate::protocol::HttpRequestParams; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeParams; @@ -66,12 +64,6 @@ pub(crate) fn build_router() -> RpcRouter { handler.terminate(params).await }, ); - router.request( - HTTP_REQUEST_METHOD, - |handler: Arc, params: HttpRequestParams| async move { - handler.http_request(params).await - }, - ); router.request( FS_READ_FILE_METHOD, |handler: Arc, params: FsReadFileParams| async move { From 7beb501146fe9f3257c3503e3d07dfbc9a9ef5ee Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 21 Apr 2026 17:23:08 -0700 Subject: [PATCH 13/29] codex: move executor http streaming into client Co-authored-by: Codex --- .../exec-server/src/client/http_client.rs | 286 ++++++++++++++++++ codex-rs/exec-server/src/http_request.rs | 213 ------------- codex-rs/exec-server/src/lib.rs | 1 - codex-rs/exec-server/src/server/handler.rs | 88 +----- 4 files changed, 299 insertions(+), 289 deletions(-) delete mode 100644 codex-rs/exec-server/src/http_request.rs diff --git a/codex-rs/exec-server/src/client/http_client.rs b/codex-rs/exec-server/src/client/http_client.rs index 25c620ce938c..632452b99f10 100644 --- a/codex-rs/exec-server/src/client/http_client.rs +++ b/codex-rs/exec-server/src/client/http_client.rs @@ -1,25 +1,55 @@ use std::collections::HashMap; use std::sync::Arc; use std::sync::atomic::Ordering; +use std::time::Duration; +use codex_app_server_protocol::JSONRPCErrorError; +use futures::StreamExt; +use reqwest::Method; +use reqwest::Url; +use reqwest::header::HeaderMap; +use reqwest::header::HeaderName; +use reqwest::header::HeaderValue; use serde_json::Value; use serde_json::from_value; use tokio::runtime::Handle; +use tokio::sync::Mutex; use tokio::sync::mpsc; use tokio::sync::mpsc::error::TrySendError; +use tokio::task::JoinHandle; use tracing::debug; use super::ExecServerClient; use super::ExecServerError; use super::Inner; +use crate::protocol::HTTP_REQUEST_BODY_DELTA_METHOD; use crate::protocol::HTTP_REQUEST_METHOD; +use crate::protocol::HttpHeader; use crate::protocol::HttpRequestBodyDeltaNotification; use crate::protocol::HttpRequestParams; use crate::protocol::HttpRequestResponse; +use crate::rpc::RpcNotificationSender; +use crate::rpc::internal_error; +use crate::rpc::invalid_params; /// Maximum queued body frames per streamed executor HTTP response. const HTTP_BODY_DELTA_CHANNEL_CAPACITY: usize = 256; +#[derive(Default)] +struct ExecutorHttpBodyStreamRegistry { + reserved_ids: HashMap>>, +} + +pub(crate) struct ExecutorPendingHttpBodyStream { + pub(crate) request_id: String, + response: reqwest::Response, +} + +pub(crate) struct ExecutorHttpClient { + notifications: RpcNotificationSender, + body_streams: Mutex, +} + /// Request-scoped stream of body chunks for an executor HTTP response. /// /// The initial `http/request` call returns status and headers. This stream then @@ -35,6 +65,94 @@ pub struct HttpResponseBodyStream { closed: bool, } +impl ExecutorHttpClient { + pub(crate) fn new(notifications: RpcNotificationSender) -> Self { + Self { + notifications, + body_streams: Mutex::new(ExecutorHttpBodyStreamRegistry::default()), + } + } + + pub(crate) async fn shutdown(&self) { + let tasks = { + let mut body_streams = self.body_streams.lock().await; + body_streams + .reserved_ids + .drain() + .filter_map(|(_, task)| task) + .collect::>() + }; + for task in tasks { + task.abort(); + } + } + + pub(crate) async fn run_http_request( + &self, + params: HttpRequestParams, + ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> + { + let request_id = if params.stream_response { + Some(params.request_id.clone().ok_or_else(|| { + invalid_params("http/request streamResponse requires requestId".to_string()) + })?) + } else { + None + }; + if let Some(request_id) = request_id.as_deref() { + self.reserve_http_body_stream(request_id).await?; + } + + let result = run_executor_http_request(params).await; + if result.is_err() + && let Some(request_id) = request_id.as_deref() + { + self.release_http_body_stream(request_id).await; + } + result + } + + pub(crate) async fn start_http_body_stream( + self: &Arc, + pending_stream: ExecutorPendingHttpBodyStream, + ) { + let request_id = pending_stream.request_id.clone(); + let finished_request_id = request_id.clone(); + let http_client = Arc::clone(self); + let notifications = self.notifications.clone(); + let task = tokio::spawn(async move { + stream_executor_http_body(pending_stream, notifications).await; + http_client + .release_http_body_stream(&finished_request_id) + .await; + }); + let mut body_streams = self.body_streams.lock().await; + if let Some(entry) = body_streams.reserved_ids.get_mut(&request_id) { + *entry = Some(task); + } else { + task.abort(); + } + } + + pub(crate) async fn release_http_body_stream(&self, request_id: &str) { + let mut body_streams = self.body_streams.lock().await; + body_streams.reserved_ids.remove(request_id); + } + + async fn reserve_http_body_stream(&self, request_id: &str) -> Result<(), JSONRPCErrorError> { + let mut body_streams = self.body_streams.lock().await; + if body_streams.reserved_ids.contains_key(request_id) { + return Err(invalid_params(format!( + "http/request streamResponse requestId `{request_id}` is already active" + ))); + } + body_streams + .reserved_ids + .insert(request_id.to_string(), None); + Ok(()) + } +} + impl HttpResponseBodyStream { /// Receives the next response-body chunk. /// @@ -328,3 +446,171 @@ fn spawn_remove_http_body_stream(inner: Arc, request_id: String) { }); } } + +async fn run_executor_http_request( + params: HttpRequestParams, +) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> { + let method = Method::from_bytes(params.method.as_bytes()) + .map_err(|err| invalid_params(format!("http/request method is invalid: {err}")))?; + let url = Url::parse(¶ms.url) + .map_err(|err| invalid_params(format!("http/request url is invalid: {err}")))?; + match url.scheme() { + "http" | "https" => {} + scheme => { + return Err(invalid_params(format!( + "http/request only supports http and https URLs, got {scheme}" + ))); + } + } + + let request_id = if params.stream_response { + Some(params.request_id.clone().ok_or_else(|| { + invalid_params("http/request streamResponse requires requestId".to_string()) + })?) + } else { + None + }; + let headers = build_executor_http_headers(params.headers)?; + let client = match params.timeout_ms { + None => reqwest::Client::builder(), + Some(timeout_ms) => reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)), + } + .build() + .map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?; + + let mut request = client.request(method, url).headers(headers); + if let Some(body) = params.body { + request = request.body(body.into_inner()); + } + + let response = request + .send() + .await + .map_err(|err| internal_error(format!("http/request failed: {err}")))?; + let status = response.status().as_u16(); + let headers = executor_response_headers(response.headers()); + + if let Some(request_id) = request_id { + return Ok(( + HttpRequestResponse { + status, + headers, + body: Vec::new().into(), + }, + Some(ExecutorPendingHttpBodyStream { + request_id, + response, + }), + )); + } + + let body = response.bytes().await.map_err(|err| { + internal_error(format!("failed to read http/request response body: {err}")) + })?; + + Ok(( + HttpRequestResponse { + status, + headers, + body: body.to_vec().into(), + }, + None, + )) +} + +fn build_executor_http_headers(headers: Vec) -> Result { + let mut header_map = HeaderMap::new(); + for header in headers { + let name = HeaderName::from_bytes(header.name.as_bytes()) + .map_err(|err| invalid_params(format!("http/request header name is invalid: {err}")))?; + let value = HeaderValue::from_str(&header.value).map_err(|err| { + invalid_params(format!( + "http/request header value is invalid for {}: {err}", + header.name + )) + })?; + header_map.append(name, value); + } + Ok(header_map) +} + +fn executor_response_headers(headers: &HeaderMap) -> Vec { + headers + .iter() + .filter_map(|(name, value)| { + Some(HttpHeader { + name: name.as_str().to_string(), + value: value.to_str().ok()?.to_string(), + }) + }) + .collect() +} + +async fn stream_executor_http_body( + pending_stream: ExecutorPendingHttpBodyStream, + notifications: RpcNotificationSender, +) { + let ExecutorPendingHttpBodyStream { + request_id, + response, + } = pending_stream; + let mut seq = 1; + let mut body = response.bytes_stream(); + while let Some(chunk) = body.next().await { + match chunk { + Ok(bytes) => { + if !send_executor_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id: request_id.clone(), + seq, + delta: bytes.to_vec().into(), + done: false, + error: None, + }, + ) + .await + { + return; + } + seq += 1; + } + Err(err) => { + let _ = send_executor_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: Some(err.to_string()), + }, + ) + .await; + return; + } + } + } + + let _ = send_executor_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: None, + }, + ) + .await; +} + +async fn send_executor_body_delta( + notifications: &RpcNotificationSender, + delta: HttpRequestBodyDeltaNotification, +) -> bool { + notifications + .notify(HTTP_REQUEST_BODY_DELTA_METHOD, &delta) + .await + .is_ok() +} diff --git a/codex-rs/exec-server/src/http_request.rs b/codex-rs/exec-server/src/http_request.rs deleted file mode 100644 index 81d206ff7cfb..000000000000 --- a/codex-rs/exec-server/src/http_request.rs +++ /dev/null @@ -1,213 +0,0 @@ -//! Executor-owned HTTP request runner. -//! -//! The remote MCP path uses this module to make HTTP requests from the same -//! environment that owns stdio process execution. Buffered responses return as a -//! normal JSON-RPC result, while streaming responses split headers and body -//! frames so Streamable HTTP clients can process SSE data before EOF. - -use std::time::Duration; - -use codex_app_server_protocol::JSONRPCErrorError; -use futures::StreamExt; -use reqwest::Method; -use reqwest::Url; -use reqwest::header::HeaderMap; -use reqwest::header::HeaderName; -use reqwest::header::HeaderValue; - -use crate::protocol::HTTP_REQUEST_BODY_DELTA_METHOD; -use crate::protocol::HttpHeader; -use crate::protocol::HttpRequestBodyDeltaNotification; -use crate::protocol::HttpRequestParams; -use crate::protocol::HttpRequestResponse; -use crate::rpc::RpcNotificationSender; -use crate::rpc::internal_error; -use crate::rpc::invalid_params; - -pub(crate) struct PendingHttpBodyStream { - pub(crate) request_id: String, - response: reqwest::Response, -} - -/// Runs one executor HTTP request and returns the JSON-RPC response payload. -/// -/// When `stream_response` is set, the returned body is empty and the response -/// bytes are emitted as ordered `http/request/bodyDelta` notifications. -pub(crate) async fn run_http_request( - params: HttpRequestParams, -) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> { - let method = Method::from_bytes(params.method.as_bytes()) - .map_err(|err| invalid_params(format!("http/request method is invalid: {err}")))?; - let url = Url::parse(¶ms.url) - .map_err(|err| invalid_params(format!("http/request url is invalid: {err}")))?; - match url.scheme() { - "http" | "https" => {} - scheme => { - return Err(invalid_params(format!( - "http/request only supports http and https URLs, got {scheme}" - ))); - } - } - - let request_id = if params.stream_response { - Some(params.request_id.clone().ok_or_else(|| { - invalid_params("http/request streamResponse requires requestId".to_string()) - })?) - } else { - None - }; - let headers = build_headers(params.headers)?; - let client = { - let client_builder = match params.timeout_ms { - None => reqwest::Client::builder(), - Some(timeout_ms) => { - reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)) - } - }; - client_builder.build() - } - .map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?; - - let mut request = client.request(method, url).headers(headers); - if let Some(body) = params.body { - request = request.body(body.into_inner()); - } - - let response = request - .send() - .await - .map_err(|err| internal_error(format!("http/request failed: {err}")))?; - let status = response.status().as_u16(); - let headers = response_headers(response.headers()); - - if let Some(request_id) = request_id { - return Ok(( - HttpRequestResponse { - status, - headers, - body: Vec::new().into(), - }, - Some(PendingHttpBodyStream { - request_id, - response, - }), - )); - } - - let body = response.bytes().await.map_err(|err| { - internal_error(format!("failed to read http/request response body: {err}")) - })?; - - Ok(( - HttpRequestResponse { - status, - headers, - body: body.to_vec().into(), - }, - None, - )) -} - -/// Converts protocol headers into a reqwest header map while preserving repeats. -fn build_headers(headers: Vec) -> Result { - let mut header_map = HeaderMap::new(); - for header in headers { - let name = HeaderName::from_bytes(header.name.as_bytes()) - .map_err(|err| invalid_params(format!("http/request header name is invalid: {err}")))?; - let value = HeaderValue::from_str(&header.value).map_err(|err| { - invalid_params(format!( - "http/request header value is invalid for {}: {err}", - header.name - )) - })?; - header_map.append(name, value); - } - Ok(header_map) -} - -/// Converts response headers back into protocol headers with UTF-8 values only. -fn response_headers(headers: &HeaderMap) -> Vec { - headers - .iter() - .filter_map(|(name, value)| { - Some(HttpHeader { - name: name.as_str().to_string(), - value: value.to_str().ok()?.to_string(), - }) - }) - .collect() -} - -/// Bridges one reqwest byte stream to ordered JSON-RPC notifications. -pub(crate) async fn stream_http_body( - pending_stream: PendingHttpBodyStream, - notifications: RpcNotificationSender, -) { - let PendingHttpBodyStream { - request_id, - response, - } = pending_stream; - let mut seq = 1; - let mut body = response.bytes_stream(); - while let Some(chunk) = body.next().await { - match chunk { - Ok(bytes) => { - if !send_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id: request_id.clone(), - seq, - delta: bytes.to_vec().into(), - done: false, - error: None, - }, - ) - .await - { - return; - } - seq += 1; - } - Err(err) => { - let _ = send_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id, - seq, - delta: Vec::new().into(), - done: true, - error: Some(err.to_string()), - }, - ) - .await; - return; - } - } - } - - let _ = send_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id, - seq, - delta: Vec::new().into(), - done: true, - error: None, - }, - ) - .await; -} - -/// Sends one streamed response-body notification. -/// -/// Returns `false` when the JSON-RPC connection has closed, letting the stream -/// task stop without treating disconnects as executor errors. -async fn send_body_delta( - notifications: &RpcNotificationSender, - delta: HttpRequestBodyDeltaNotification, -) -> bool { - notifications - .notify(HTTP_REQUEST_BODY_DELTA_METHOD, &delta) - .await - .is_ok() -} diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index 36a947f82589..067fa0a7c147 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -6,7 +6,6 @@ mod file_system; mod fs_helper; mod fs_helper_main; mod fs_sandbox; -mod http_request; mod local_file_system; mod local_process; mod process; diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 34716cf532ff..e8630015cb7c 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -5,13 +5,10 @@ use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use codex_app_server_protocol::JSONRPCErrorError; -use tokio::sync::Mutex; -use tokio::task::JoinHandle; use crate::ExecServerRuntimePaths; -use crate::http_request::PendingHttpBodyStream; -use crate::http_request::run_http_request; -use crate::http_request::stream_http_body; +use crate::client::http_client::ExecutorHttpClient; +use crate::client::http_client::ExecutorPendingHttpBodyStream; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; use crate::protocol::FsCopyParams; @@ -39,22 +36,16 @@ use crate::protocol::TerminateResponse; use crate::protocol::WriteParams; use crate::protocol::WriteResponse; use crate::rpc::RpcNotificationSender; -use crate::rpc::invalid_params; use crate::rpc::invalid_request; use crate::server::file_system_handler::FileSystemHandler; use crate::server::session_registry::SessionHandle; use crate::server::session_registry::SessionRegistry; -#[derive(Default)] -struct HttpBodyStreamRegistry { - reserved_ids: HashMap>>, -} - pub(crate) struct ExecServerHandler { session_registry: Arc, notifications: RpcNotificationSender, session: StdMutex>, - http_body_streams: Mutex, + http_client: Arc, file_system: FileSystemHandler, initialize_requested: AtomicBool, initialized: AtomicBool, @@ -68,9 +59,9 @@ impl ExecServerHandler { ) -> Self { Self { session_registry, + http_client: Arc::new(ExecutorHttpClient::new(notifications.clone())), notifications, session: StdMutex::new(None), - http_body_streams: Mutex::new(HttpBodyStreamRegistry::default()), file_system: FileSystemHandler::new(runtime_paths), initialize_requested: AtomicBool::new(false), initialized: AtomicBool::new(false), @@ -78,18 +69,7 @@ impl ExecServerHandler { } pub(crate) async fn shutdown(&self) { - let tasks = { - let mut http_body_streams = self.http_body_streams.lock().await; - let tasks = http_body_streams - .reserved_ids - .drain() - .filter_map(|(_, task)| task) - .collect::>(); - tasks - }; - for task in tasks { - task.abort(); - } + self.http_client.shutdown().await; if let Some(session) = self.session() { session.detach().await; } @@ -178,47 +158,19 @@ impl ExecServerHandler { pub(crate) async fn http_request( &self, params: HttpRequestParams, - ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> { + ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> + { self.require_initialized_for("http")?; - let request_id = if params.stream_response { - Some(params.request_id.clone().ok_or_else(|| { - invalid_params("http/request streamResponse requires requestId".to_string()) - })?) - } else { - None - }; - if let Some(request_id) = request_id.as_deref() { - self.reserve_http_body_stream(request_id).await?; - } - match run_http_request(params).await { - Ok((response, pending_stream)) => Ok((response, pending_stream)), - Err(error) => { - if let Some(request_id) = request_id.as_deref() { - self.release_http_body_stream(request_id).await; - } - Err(error) - } - } + self.http_client.run_http_request(params).await } pub(crate) async fn start_http_body_stream( self: &Arc, - pending_stream: PendingHttpBodyStream, + pending_stream: ExecutorPendingHttpBodyStream, ) { - let request_id = pending_stream.request_id.clone(); - let finished_request_id = request_id.clone(); - let handler = Arc::clone(self); - let notifications = self.notifications.clone(); - let task = tokio::spawn(async move { - stream_http_body(pending_stream, notifications).await; - handler.release_http_body_stream(&finished_request_id).await; - }); - let mut http_body_streams = self.http_body_streams.lock().await; - if let Some(entry) = http_body_streams.reserved_ids.get_mut(&request_id) { - *entry = Some(task); - } else { - task.abort(); - } + self.http_client + .start_http_body_stream(pending_stream) + .await; } pub(crate) async fn fs_read_file( @@ -317,22 +269,8 @@ impl ExecServerHandler { .clone() } - async fn reserve_http_body_stream(&self, request_id: &str) -> Result<(), JSONRPCErrorError> { - let mut http_body_streams = self.http_body_streams.lock().await; - if http_body_streams.reserved_ids.contains_key(request_id) { - return Err(invalid_params(format!( - "http/request streamResponse requestId `{request_id}` is already active" - ))); - } - http_body_streams - .reserved_ids - .insert(request_id.to_string(), None); - Ok(()) - } - pub(crate) async fn release_http_body_stream(&self, request_id: &str) { - let mut http_body_streams = self.http_body_streams.lock().await; - http_body_streams.reserved_ids.remove(request_id); + self.http_client.release_http_body_stream(request_id).await; } } From d93a3199acc2320196adbf26908f9f240012bd73 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 21 Apr 2026 21:05:27 -0700 Subject: [PATCH 14/29] codex: require request ids for executor http Co-authored-by: Codex --- .../exec-server/src/client/http_client.rs | 33 ++++-------- codex-rs/exec-server/src/protocol.rs | 7 +-- codex-rs/exec-server/src/server/handler.rs | 1 - codex-rs/exec-server/tests/http_client.rs | 52 +++++++++---------- codex-rs/exec-server/tests/http_request.rs | 12 ++--- 5 files changed, 45 insertions(+), 60 deletions(-) diff --git a/codex-rs/exec-server/src/client/http_client.rs b/codex-rs/exec-server/src/client/http_client.rs index 632452b99f10..454c59a01f81 100644 --- a/codex-rs/exec-server/src/client/http_client.rs +++ b/codex-rs/exec-server/src/client/http_client.rs @@ -92,22 +92,15 @@ impl ExecutorHttpClient { params: HttpRequestParams, ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> { - let request_id = if params.stream_response { - Some(params.request_id.clone().ok_or_else(|| { - invalid_params("http/request streamResponse requires requestId".to_string()) - })?) - } else { - None - }; - if let Some(request_id) = request_id.as_deref() { - self.reserve_http_body_stream(request_id).await?; + let stream_response = params.stream_response; + let request_id = params.request_id.clone(); + if stream_response { + self.reserve_http_body_stream(&request_id).await?; } let result = run_executor_http_request(params).await; - if result.is_err() - && let Some(request_id) = request_id.as_deref() - { - self.release_http_body_stream(request_id).await; + if result.is_err() && stream_response { + self.release_http_body_stream(&request_id).await; } result } @@ -250,7 +243,6 @@ impl ExecServerClient { mut params: HttpRequestParams, ) -> Result { params.stream_response = false; - params.request_id = None; self.call(HTTP_REQUEST_METHOD, ¶ms).await } @@ -265,7 +257,7 @@ impl ExecServerClient { ) -> Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError> { params.stream_response = true; let request_id = self.inner.next_http_body_stream_request_id(); - params.request_id = Some(request_id.clone()); + params.request_id = request_id.clone(); let (tx, rx) = mpsc::channel(HTTP_BODY_DELTA_CHANNEL_CAPACITY); self.inner .insert_http_body_stream(request_id.clone(), tx) @@ -463,13 +455,6 @@ async fn run_executor_http_request( } } - let request_id = if params.stream_response { - Some(params.request_id.clone().ok_or_else(|| { - invalid_params("http/request streamResponse requires requestId".to_string()) - })?) - } else { - None - }; let headers = build_executor_http_headers(params.headers)?; let client = match params.timeout_ms { None => reqwest::Client::builder(), @@ -490,7 +475,7 @@ async fn run_executor_http_request( let status = response.status().as_u16(); let headers = executor_response_headers(response.headers()); - if let Some(request_id) = request_id { + if params.stream_response { return Ok(( HttpRequestResponse { status, @@ -498,7 +483,7 @@ async fn run_executor_http_request( body: Vec::new().into(), }, Some(ExecutorPendingHttpBodyStream { - request_id, + request_id: params.request_id, response, }), )); diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 90a78d899f2e..5df9f6a5f718 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -295,9 +295,10 @@ pub struct HttpRequestParams { /// Caller-chosen stream id for `http/request/bodyDelta` notifications. /// /// The id must remain unique on a connection until the terminal body delta - /// arrives, even if the caller stops reading the stream earlier. - #[serde(default)] - pub request_id: Option, + /// arrives, even if the caller stops reading the stream earlier. Buffered + /// requests still send an id so callers can keep one consistent request + /// envelope shape. + pub request_id: String, /// Return after response headers and stream the response body as deltas. #[serde(default)] pub stream_response: bool, diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index e8630015cb7c..6fa3cf0b97b3 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -1,4 +1,3 @@ -use std::collections::HashMap; use std::sync::Arc; use std::sync::Mutex as StdMutex; use std::sync::atomic::AtomicBool; diff --git a/codex-rs/exec-server/tests/http_client.rs b/codex-rs/exec-server/tests/http_client.rs index 1ef7a9323adb..6a087dd11512 100644 --- a/codex-rs/exec-server/tests/http_client.rs +++ b/codex-rs/exec-server/tests/http_client.rs @@ -52,8 +52,8 @@ async fn http_request_forces_buffered_request_params() -> Result<()> { // Phase 1: start a fake WebSocket exec-server so the test covers the // public client connection path without depending on the HTTP runner. let server = spawn_scripted_exec_server(|mut peer| async move { - // Phase 2: verify the buffered helper strips streaming-only fields - // before it sends the JSON-RPC call. + // Phase 2: verify the buffered helper forces buffered mode before it + // sends the JSON-RPC call. let (request_id, params) = peer.read_http_request().await?; assert_eq!( params, @@ -63,7 +63,7 @@ async fn http_request_forces_buffered_request_params() -> Result<()> { headers: Vec::new(), body: None, timeout_ms: None, - request_id: None, + request_id: "ignored-stream-id".to_string(), stream_response: false, } ); @@ -91,7 +91,7 @@ async fn http_request_forces_buffered_request_params() -> Result<()> { headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("ignored-stream-id".to_string()), + request_id: "ignored-stream-id".to_string(), stream_response: true, }), ) @@ -131,7 +131,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta }], body: None, timeout_ms: None, - request_id: Some("http-1".to_string()), + request_id: "http-1".to_string(), stream_response: true, } ); @@ -186,7 +186,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("http-2".to_string()), + request_id: "http-2".to_string(), stream_response: true, } ); @@ -215,7 +215,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta }], body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }), ) @@ -253,7 +253,7 @@ async fn http_response_body_stream_uses_generated_ids_and_receives_ordered_delta headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }), ) @@ -289,7 +289,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("http-1".to_string()), + request_id: "http-1".to_string(), stream_response: true, } ); @@ -322,7 +322,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("http-2".to_string()), + request_id: "http-2".to_string(), stream_response: true, } ); @@ -348,7 +348,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }), ) @@ -372,7 +372,7 @@ async fn http_response_body_stream_drops_queued_terminal_before_next_generated_i headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }; let (reuse_response, _reuse_body_stream) = @@ -411,7 +411,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("http-1".to_string()), + request_id: "http-1".to_string(), stream_response: true, } ); @@ -430,7 +430,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("http-2".to_string()), + request_id: "http-2".to_string(), stream_response: true, } ); @@ -474,7 +474,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }) .await; @@ -495,7 +495,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_cancelled_request() headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }), ) @@ -541,7 +541,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("http-1".to_string()), + request_id: "http-1".to_string(), stream_response: true, } ); @@ -580,7 +580,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("http-2".to_string()), + request_id: "http-2".to_string(), stream_response: true, } ); @@ -615,7 +615,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }), ) @@ -647,7 +647,7 @@ async fn http_response_body_stream_ignores_late_deltas_after_drop() -> Result<() headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }), ) @@ -691,7 +691,7 @@ async fn http_response_body_stream_fails_when_transport_disconnects() -> Result< headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("http-1".to_string()), + request_id: "http-1".to_string(), stream_response: true, } ); @@ -717,7 +717,7 @@ async fn http_response_body_stream_fails_when_transport_disconnects() -> Result< headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }), ) @@ -759,7 +759,7 @@ async fn http_response_body_stream_reports_disconnect_when_queue_is_full() -> Re headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("http-1".to_string()), + request_id: "http-1".to_string(), stream_response: true, } ); @@ -796,7 +796,7 @@ async fn http_response_body_stream_reports_disconnect_when_queue_is_full() -> Re headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }), ) @@ -852,7 +852,7 @@ async fn http_response_body_stream_reports_backpressure_truncation() -> Result<( headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("http-1".to_string()), + request_id: "http-1".to_string(), stream_response: true, } ); @@ -894,7 +894,7 @@ async fn http_response_body_stream_reports_backpressure_truncation() -> Result<( headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("caller-stream-id".to_string()), + request_id: "caller-stream-id".to_string(), stream_response: false, }), ) diff --git a/codex-rs/exec-server/tests/http_request.rs b/codex-rs/exec-server/tests/http_request.rs index e9c5c67f253c..f45261dad74d 100644 --- a/codex-rs/exec-server/tests/http_request.rs +++ b/codex-rs/exec-server/tests/http_request.rs @@ -63,7 +63,7 @@ async fn exec_server_http_request_buffers_response_body() -> anyhow::Result<()> }], body: Some(b"request-body".to_vec().into()), timeout_ms: Some(5_000), - request_id: None, + request_id: "buffered-request".to_string(), stream_response: false, })?, ) @@ -132,7 +132,7 @@ async fn exec_server_http_request_streams_response_body_notifications() -> anyho }], body: None, timeout_ms: Some(5_000), - request_id: Some("stream-1".to_string()), + request_id: "stream-1".to_string(), stream_response: true, })?, ) @@ -218,7 +218,7 @@ async fn exec_server_http_request_rejects_duplicate_stream_request_ids() -> anyh headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("stream-dup".to_string()), + request_id: "stream-dup".to_string(), stream_response: true, })?, ) @@ -241,7 +241,7 @@ async fn exec_server_http_request_rejects_duplicate_stream_request_ids() -> anyh headers: Vec::new(), body: None, timeout_ms: None, - request_id: Some("stream-dup".to_string()), + request_id: "stream-dup".to_string(), stream_response: true, })?, ) @@ -295,7 +295,7 @@ async fn exec_server_http_request_honors_optional_timeout() -> anyhow::Result<() headers: Vec::new(), body: None, timeout_ms: None, - request_id: None, + request_id: "buffered-request".to_string(), stream_response: false, })?, ) @@ -320,7 +320,7 @@ async fn exec_server_http_request_honors_optional_timeout() -> anyhow::Result<() headers: Vec::new(), body: None, timeout_ms: Some(10), - request_id: None, + request_id: "buffered-request".to_string(), stream_response: false, })?, ) From fa029b87db29a3237acb16810c67c07077f0c867 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 21 Apr 2026 22:01:12 -0700 Subject: [PATCH 15/29] codex: update executor http timeout test Co-authored-by: Codex --- codex-rs/exec-server/src/protocol.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 5df9f6a5f718..435187d05a27 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -406,23 +406,38 @@ mod tests { let omitted: HttpRequestParams = serde_json::from_value(serde_json::json!({ "method": "GET", "url": "https://example.test", + "requestId": "req-omitted-timeout", })) .expect("omitted timeout should deserialize"); let null_timeout: HttpRequestParams = serde_json::from_value(serde_json::json!({ "method": "GET", "url": "https://example.test", + "requestId": "req-null-timeout", "timeoutMs": null, })) .expect("null timeout should deserialize"); let explicit_timeout: HttpRequestParams = serde_json::from_value(serde_json::json!({ "method": "GET", "url": "https://example.test", + "requestId": "req-explicit-timeout", "timeoutMs": 1234, })) .expect("numeric timeout should deserialize"); - assert_eq!(omitted.timeout_ms, None); - assert_eq!(null_timeout.timeout_ms, None); - assert_eq!(explicit_timeout.timeout_ms, Some(1234)); + assert_eq!( + (omitted.request_id.as_str(), omitted.timeout_ms), + ("req-omitted-timeout", None) + ); + assert_eq!( + (null_timeout.request_id.as_str(), null_timeout.timeout_ms), + ("req-null-timeout", None) + ); + assert_eq!( + ( + explicit_timeout.request_id.as_str(), + explicit_timeout.timeout_ms + ), + ("req-explicit-timeout", Some(1234)) + ); } } From e4e252574b0edd52580149a134cb1cbcafa1ad24 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 21 Apr 2026 22:14:48 -0700 Subject: [PATCH 16/29] codex: simplify http body stream registry Co-authored-by: Codex --- .../exec-server/src/client/http_client.rs | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/codex-rs/exec-server/src/client/http_client.rs b/codex-rs/exec-server/src/client/http_client.rs index 454c59a01f81..b890ef9e5ed9 100644 --- a/codex-rs/exec-server/src/client/http_client.rs +++ b/codex-rs/exec-server/src/client/http_client.rs @@ -35,11 +35,6 @@ use crate::rpc::invalid_params; /// Maximum queued body frames per streamed executor HTTP response. const HTTP_BODY_DELTA_CHANNEL_CAPACITY: usize = 256; -#[derive(Default)] -struct ExecutorHttpBodyStreamRegistry { - reserved_ids: HashMap>>, -} - pub(crate) struct ExecutorPendingHttpBodyStream { pub(crate) request_id: String, response: reqwest::Response, @@ -47,7 +42,7 @@ pub(crate) struct ExecutorPendingHttpBodyStream { pub(crate) struct ExecutorHttpClient { notifications: RpcNotificationSender, - body_streams: Mutex, + body_streams: Mutex>>>, } /// Request-scoped stream of body chunks for an executor HTTP response. @@ -69,7 +64,7 @@ impl ExecutorHttpClient { pub(crate) fn new(notifications: RpcNotificationSender) -> Self { Self { notifications, - body_streams: Mutex::new(ExecutorHttpBodyStreamRegistry::default()), + body_streams: Mutex::new(HashMap::new()), } } @@ -77,7 +72,6 @@ impl ExecutorHttpClient { let tasks = { let mut body_streams = self.body_streams.lock().await; body_streams - .reserved_ids .drain() .filter_map(|(_, task)| task) .collect::>() @@ -120,7 +114,7 @@ impl ExecutorHttpClient { .await; }); let mut body_streams = self.body_streams.lock().await; - if let Some(entry) = body_streams.reserved_ids.get_mut(&request_id) { + if let Some(entry) = body_streams.get_mut(&request_id) { *entry = Some(task); } else { task.abort(); @@ -129,19 +123,17 @@ impl ExecutorHttpClient { pub(crate) async fn release_http_body_stream(&self, request_id: &str) { let mut body_streams = self.body_streams.lock().await; - body_streams.reserved_ids.remove(request_id); + body_streams.remove(request_id); } async fn reserve_http_body_stream(&self, request_id: &str) -> Result<(), JSONRPCErrorError> { let mut body_streams = self.body_streams.lock().await; - if body_streams.reserved_ids.contains_key(request_id) { + if body_streams.contains_key(request_id) { return Err(invalid_params(format!( "http/request streamResponse requestId `{request_id}` is already active" ))); } - body_streams - .reserved_ids - .insert(request_id.to_string(), None); + body_streams.insert(request_id.to_string(), None); Ok(()) } } From 53d813f062587b8eefd9ab62ef8ef4947c88318e Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 21 Apr 2026 22:18:44 -0700 Subject: [PATCH 17/29] codex: move executor http response flow into handler Co-authored-by: Codex --- codex-rs/exec-server/src/server/handler.rs | 54 +++++++++++++------- codex-rs/exec-server/src/server/processor.rs | 48 ++++------------- 2 files changed, 44 insertions(+), 58 deletions(-) diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 6fa3cf0b97b3..80e39b2a0b78 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -4,10 +4,12 @@ use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use codex_app_server_protocol::JSONRPCErrorError; +use codex_app_server_protocol::RequestId; +use serde_json::to_value; +use tokio::sync::mpsc; use crate::ExecServerRuntimePaths; use crate::client::http_client::ExecutorHttpClient; -use crate::client::http_client::ExecutorPendingHttpBodyStream; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; use crate::protocol::FsCopyParams; @@ -25,7 +27,6 @@ use crate::protocol::FsRemoveResponse; use crate::protocol::FsWriteFileParams; use crate::protocol::FsWriteFileResponse; use crate::protocol::HttpRequestParams; -use crate::protocol::HttpRequestResponse; use crate::protocol::InitializeParams; use crate::protocol::InitializeResponse; use crate::protocol::ReadParams; @@ -35,6 +36,8 @@ use crate::protocol::TerminateResponse; use crate::protocol::WriteParams; use crate::protocol::WriteResponse; use crate::rpc::RpcNotificationSender; +use crate::rpc::RpcServerOutboundMessage; +use crate::rpc::internal_error; use crate::rpc::invalid_request; use crate::server::file_system_handler::FileSystemHandler; use crate::server::session_registry::SessionHandle; @@ -43,6 +46,7 @@ use crate::server::session_registry::SessionRegistry; pub(crate) struct ExecServerHandler { session_registry: Arc, notifications: RpcNotificationSender, + outgoing_tx: mpsc::Sender, session: StdMutex>, http_client: Arc, file_system: FileSystemHandler, @@ -54,12 +58,14 @@ impl ExecServerHandler { pub(crate) fn new( session_registry: Arc, notifications: RpcNotificationSender, + outgoing_tx: mpsc::Sender, runtime_paths: ExecServerRuntimePaths, ) -> Self { Self { session_registry, http_client: Arc::new(ExecutorHttpClient::new(notifications.clone())), notifications, + outgoing_tx, session: StdMutex::new(None), file_system: FileSystemHandler::new(runtime_paths), initialize_requested: AtomicBool::new(false), @@ -155,21 +161,35 @@ impl ExecServerHandler { } pub(crate) async fn http_request( - &self, + self: &Arc, + request_id: RequestId, params: HttpRequestParams, - ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> - { + ) -> Result<(), JSONRPCErrorError> { self.require_initialized_for("http")?; - self.http_client.run_http_request(params).await - } - - pub(crate) async fn start_http_body_stream( - self: &Arc, - pending_stream: ExecutorPendingHttpBodyStream, - ) { - self.http_client - .start_http_body_stream(pending_stream) - .await; + let (response, mut pending_stream) = self.http_client.run_http_request(params).await?; + let message = match to_value(response) { + Ok(result) => RpcServerOutboundMessage::Response { request_id, result }, + Err(err) => { + if let Some(pending_stream) = pending_stream.take() { + self.http_client + .release_http_body_stream(&pending_stream.request_id) + .await; + } + RpcServerOutboundMessage::Error { + request_id, + error: internal_error(err.to_string()), + } + } + }; + self.outgoing_tx.send(message).await.map_err(|_| { + internal_error("RPC connection closed while sending http/request response".into()) + })?; + if let Some(pending_stream) = pending_stream { + self.http_client + .start_http_body_stream(pending_stream) + .await; + } + Ok(()) } pub(crate) async fn fs_read_file( @@ -267,10 +287,6 @@ impl ExecServerHandler { .unwrap_or_else(std::sync::PoisonError::into_inner) .clone() } - - pub(crate) async fn release_http_body_stream(&self, request_id: &str) { - self.http_client.release_http_body_stream(request_id).await; - } } #[cfg(test)] diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index eeea9eeb59c4..fb1f2b9dd823 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -1,6 +1,5 @@ use std::sync::Arc; -use serde_json::to_value; use tokio::sync::mpsc; use tracing::debug; use tracing::warn; @@ -15,7 +14,6 @@ use crate::rpc::RpcNotificationSender; use crate::rpc::RpcServerOutboundMessage; use crate::rpc::decode_request_params; use crate::rpc::encode_server_message; -use crate::rpc::internal_error; use crate::rpc::invalid_request; use crate::rpc::method_not_found; use crate::server::ExecServerHandler; @@ -60,6 +58,7 @@ async fn run_connection( let handler = Arc::new(ExecServerHandler::new( session_registry, notifications, + outgoing_tx.clone(), runtime_paths, )); @@ -117,48 +116,19 @@ async fn run_connection( } }; let response = tokio::select! { - response = handler.http_request(params) => response, + response = handler.http_request(request_id.clone(), params) => response, _ = disconnected_rx.changed() => { debug!("exec-server transport disconnected while handling request"); break; } }; - match response { - Ok((response, pending_stream)) => { - let mut pending_stream = pending_stream; - let message = match to_value(response) { - Ok(result) => { - RpcServerOutboundMessage::Response { request_id, result } - } - Err(err) => { - if let Some(pending_stream) = pending_stream.take() { - handler - .release_http_body_stream( - &pending_stream.request_id, - ) - .await; - } - RpcServerOutboundMessage::Error { - request_id, - error: internal_error(err.to_string()), - } - } - }; - if outgoing_tx.send(message).await.is_err() { - break; - } - if let Some(pending_stream) = pending_stream { - handler.start_http_body_stream(pending_stream).await; - } - } - Err(error) => { - if outgoing_tx - .send(RpcServerOutboundMessage::Error { request_id, error }) - .await - .is_err() - { - break; - } + if let Err(error) = response { + if outgoing_tx + .send(RpcServerOutboundMessage::Error { request_id, error }) + .await + .is_err() + { + break; } } } else if let Some(route) = router.request_route(request.method.as_str()) { From 1d550d81ad9cdeab77dfc186239f0d7bb7d5cee2 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 09:08:01 -0700 Subject: [PATCH 18/29] codex: route http requests through exec router Co-authored-by: Codex --- codex-rs/exec-server/src/rpc.rs | 74 +++++++++++++++++--- codex-rs/exec-server/src/server/processor.rs | 46 ++---------- codex-rs/exec-server/src/server/registry.rs | 8 +++ 3 files changed, 80 insertions(+), 48 deletions(-) diff --git a/codex-rs/exec-server/src/rpc.rs b/codex-rs/exec-server/src/rpc.rs index d20772c07067..96ad8fe13411 100644 --- a/codex-rs/exec-server/src/rpc.rs +++ b/codex-rs/exec-server/src/rpc.rs @@ -37,7 +37,7 @@ pub(crate) enum RpcCallError { type PendingRequest = oneshot::Sender>; type BoxFuture = Pin + Send + 'static>>; type RequestRoute = - Box, JSONRPCRequest) -> BoxFuture + Send + Sync>; + Box, JSONRPCRequest) -> BoxFuture + Send + Sync>; type NotificationRoute = Box, JSONRPCNotification) -> BoxFuture> + Send + Sync>; @@ -61,6 +61,11 @@ pub(crate) enum RpcServerOutboundMessage { Notification(JSONRPCNotification), } +pub(crate) enum RequestRouteOutcome { + Message(RpcServerOutboundMessage), + AlreadySent, +} + #[allow(dead_code)] #[derive(Clone)] pub(crate) struct RpcNotificationSender { @@ -131,18 +136,69 @@ where let response = match response { Ok(response) => response.await, Err(error) => { - return RpcServerOutboundMessage::Error { request_id, error }; + return RequestRouteOutcome::Message(RpcServerOutboundMessage::Error { + request_id, + error, + }); } }; match response { - Ok(result) => match serde_json::to_value(result) { - Ok(result) => RpcServerOutboundMessage::Response { request_id, result }, - Err(err) => RpcServerOutboundMessage::Error { + Ok(result) => { + match serde_json::to_value(result) { + Ok(result) => RequestRouteOutcome::Message( + RpcServerOutboundMessage::Response { request_id, result }, + ), + Err(err) => { + RequestRouteOutcome::Message(RpcServerOutboundMessage::Error { + request_id, + error: internal_error(err.to_string()), + }) + } + } + } + Err(error) => { + RequestRouteOutcome::Message(RpcServerOutboundMessage::Error { request_id, - error: internal_error(err.to_string()), - }, - }, - Err(error) => RpcServerOutboundMessage::Error { request_id, error }, + error, + }) + } + } + }) + }), + ); + } + + pub(crate) fn request_already_sent(&mut self, method: &'static str, handler: F) + where + P: DeserializeOwned + Send + 'static, + F: Fn(Arc, RequestId, P) -> Fut + Send + Sync + 'static, + Fut: Future> + Send + 'static, + { + self.request_routes.insert( + method, + Box::new(move |state, request| { + let request_id = request.id; + let params = request.params; + let response = decode_request_params::

(params) + .map(|params| handler(state, request_id.clone(), params)); + Box::pin(async move { + let response = match response { + Ok(response) => response.await, + Err(error) => { + return RequestRouteOutcome::Message(RpcServerOutboundMessage::Error { + request_id, + error, + }); + } + }; + match response { + Ok(()) => RequestRouteOutcome::AlreadySent, + Err(error) => { + RequestRouteOutcome::Message(RpcServerOutboundMessage::Error { + request_id, + error, + }) + } } }) }), diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index fb1f2b9dd823..129f04ce3ac5 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -8,11 +8,9 @@ use crate::ExecServerRuntimePaths; use crate::connection::CHANNEL_CAPACITY; use crate::connection::JsonRpcConnection; use crate::connection::JsonRpcConnectionEvent; -use crate::protocol::HTTP_REQUEST_METHOD; -use crate::protocol::HttpRequestParams; +use crate::rpc::RequestRouteOutcome; use crate::rpc::RpcNotificationSender; use crate::rpc::RpcServerOutboundMessage; -use crate::rpc::decode_request_params; use crate::rpc::encode_server_message; use crate::rpc::invalid_request; use crate::rpc::method_not_found; @@ -99,47 +97,17 @@ async fn run_connection( } JsonRpcConnectionEvent::Message(message) => match message { codex_app_server_protocol::JSONRPCMessage::Request(request) => { - if request.method == HTTP_REQUEST_METHOD { - let request_id = request.id.clone(); - let params = - match decode_request_params::(request.params) { - Ok(params) => params, - Err(error) => { - if outgoing_tx - .send(RpcServerOutboundMessage::Error { request_id, error }) - .await - .is_err() - { - break; - } - continue; - } - }; - let response = tokio::select! { - response = handler.http_request(request_id.clone(), params) => response, + if let Some(route) = router.request_route(request.method.as_str()) { + let outcome = tokio::select! { + outcome = route(Arc::clone(&handler), request) => outcome, _ = disconnected_rx.changed() => { debug!("exec-server transport disconnected while handling request"); break; } }; - if let Err(error) = response { - if outgoing_tx - .send(RpcServerOutboundMessage::Error { request_id, error }) - .await - .is_err() - { - break; - } - } - } else if let Some(route) = router.request_route(request.method.as_str()) { - let message = tokio::select! { - message = route(Arc::clone(&handler), request) => message, - _ = disconnected_rx.changed() => { - debug!("exec-server transport disconnected while handling request"); - break; - } - }; - if outgoing_tx.send(message).await.is_err() { + if let RequestRouteOutcome::Message(message) = outcome + && outgoing_tx.send(message).await.is_err() + { break; } } else if outgoing_tx diff --git a/codex-rs/exec-server/src/server/registry.rs b/codex-rs/exec-server/src/server/registry.rs index a57704c50502..5f4821701844 100644 --- a/codex-rs/exec-server/src/server/registry.rs +++ b/codex-rs/exec-server/src/server/registry.rs @@ -19,6 +19,8 @@ use crate::protocol::FsReadDirectoryParams; use crate::protocol::FsReadFileParams; use crate::protocol::FsRemoveParams; use crate::protocol::FsWriteFileParams; +use crate::protocol::HTTP_REQUEST_METHOD; +use crate::protocol::HttpRequestParams; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeParams; @@ -42,6 +44,12 @@ pub(crate) fn build_router() -> RpcRouter { handler.initialize(params).await }, ); + router.request_already_sent( + HTTP_REQUEST_METHOD, + |handler: Arc, request_id, params: HttpRequestParams| async move { + handler.http_request(request_id, params).await + }, + ); router.request( EXEC_METHOD, |handler: Arc, params: ExecParams| async move { handler.exec(params).await }, From 83e0ad652c2e95d07e2952f26d3adf83bb87357d Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 09:47:35 -0700 Subject: [PATCH 19/29] codex: inline executor http state into handler Co-authored-by: Codex --- .../exec-server/src/client/http_client.rs | 89 +------------------ codex-rs/exec-server/src/server/handler.rs | 77 +++++++++++++--- 2 files changed, 69 insertions(+), 97 deletions(-) diff --git a/codex-rs/exec-server/src/client/http_client.rs b/codex-rs/exec-server/src/client/http_client.rs index b890ef9e5ed9..26e1a1cf315f 100644 --- a/codex-rs/exec-server/src/client/http_client.rs +++ b/codex-rs/exec-server/src/client/http_client.rs @@ -13,10 +13,8 @@ use reqwest::header::HeaderValue; use serde_json::Value; use serde_json::from_value; use tokio::runtime::Handle; -use tokio::sync::Mutex; use tokio::sync::mpsc; use tokio::sync::mpsc::error::TrySendError; -use tokio::task::JoinHandle; use tracing::debug; use super::ExecServerClient; @@ -40,11 +38,6 @@ pub(crate) struct ExecutorPendingHttpBodyStream { response: reqwest::Response, } -pub(crate) struct ExecutorHttpClient { - notifications: RpcNotificationSender, - body_streams: Mutex>>>, -} - /// Request-scoped stream of body chunks for an executor HTTP response. /// /// The initial `http/request` call returns status and headers. This stream then @@ -60,84 +53,6 @@ pub struct HttpResponseBodyStream { closed: bool, } -impl ExecutorHttpClient { - pub(crate) fn new(notifications: RpcNotificationSender) -> Self { - Self { - notifications, - body_streams: Mutex::new(HashMap::new()), - } - } - - pub(crate) async fn shutdown(&self) { - let tasks = { - let mut body_streams = self.body_streams.lock().await; - body_streams - .drain() - .filter_map(|(_, task)| task) - .collect::>() - }; - for task in tasks { - task.abort(); - } - } - - pub(crate) async fn run_http_request( - &self, - params: HttpRequestParams, - ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> - { - let stream_response = params.stream_response; - let request_id = params.request_id.clone(); - if stream_response { - self.reserve_http_body_stream(&request_id).await?; - } - - let result = run_executor_http_request(params).await; - if result.is_err() && stream_response { - self.release_http_body_stream(&request_id).await; - } - result - } - - pub(crate) async fn start_http_body_stream( - self: &Arc, - pending_stream: ExecutorPendingHttpBodyStream, - ) { - let request_id = pending_stream.request_id.clone(); - let finished_request_id = request_id.clone(); - let http_client = Arc::clone(self); - let notifications = self.notifications.clone(); - let task = tokio::spawn(async move { - stream_executor_http_body(pending_stream, notifications).await; - http_client - .release_http_body_stream(&finished_request_id) - .await; - }); - let mut body_streams = self.body_streams.lock().await; - if let Some(entry) = body_streams.get_mut(&request_id) { - *entry = Some(task); - } else { - task.abort(); - } - } - - pub(crate) async fn release_http_body_stream(&self, request_id: &str) { - let mut body_streams = self.body_streams.lock().await; - body_streams.remove(request_id); - } - - async fn reserve_http_body_stream(&self, request_id: &str) -> Result<(), JSONRPCErrorError> { - let mut body_streams = self.body_streams.lock().await; - if body_streams.contains_key(request_id) { - return Err(invalid_params(format!( - "http/request streamResponse requestId `{request_id}` is already active" - ))); - } - body_streams.insert(request_id.to_string(), None); - Ok(()) - } -} - impl HttpResponseBodyStream { /// Receives the next response-body chunk. /// @@ -431,7 +346,7 @@ fn spawn_remove_http_body_stream(inner: Arc, request_id: String) { } } -async fn run_executor_http_request( +pub(crate) async fn run_executor_http_request( params: HttpRequestParams, ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> { let method = Method::from_bytes(params.method.as_bytes()) @@ -523,7 +438,7 @@ fn executor_response_headers(headers: &HeaderMap) -> Vec { .collect() } -async fn stream_executor_http_body( +pub(crate) async fn stream_executor_http_body( pending_stream: ExecutorPendingHttpBodyStream, notifications: RpcNotificationSender, ) { diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 80e39b2a0b78..4b24a236496b 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -6,10 +6,15 @@ use std::sync::atomic::Ordering; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::RequestId; use serde_json::to_value; +use std::collections::HashMap; +use tokio::sync::Mutex; use tokio::sync::mpsc; +use tokio::task::JoinHandle; use crate::ExecServerRuntimePaths; -use crate::client::http_client::ExecutorHttpClient; +use crate::client::http_client::ExecutorPendingHttpBodyStream; +use crate::client::http_client::run_executor_http_request; +use crate::client::http_client::stream_executor_http_body; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; use crate::protocol::FsCopyParams; @@ -38,6 +43,7 @@ use crate::protocol::WriteResponse; use crate::rpc::RpcNotificationSender; use crate::rpc::RpcServerOutboundMessage; use crate::rpc::internal_error; +use crate::rpc::invalid_params; use crate::rpc::invalid_request; use crate::server::file_system_handler::FileSystemHandler; use crate::server::session_registry::SessionHandle; @@ -48,7 +54,7 @@ pub(crate) struct ExecServerHandler { notifications: RpcNotificationSender, outgoing_tx: mpsc::Sender, session: StdMutex>, - http_client: Arc, + body_streams: Mutex>>>, file_system: FileSystemHandler, initialize_requested: AtomicBool, initialized: AtomicBool, @@ -63,10 +69,10 @@ impl ExecServerHandler { ) -> Self { Self { session_registry, - http_client: Arc::new(ExecutorHttpClient::new(notifications.clone())), notifications, outgoing_tx, session: StdMutex::new(None), + body_streams: Mutex::new(HashMap::new()), file_system: FileSystemHandler::new(runtime_paths), initialize_requested: AtomicBool::new(false), initialized: AtomicBool::new(false), @@ -74,7 +80,16 @@ impl ExecServerHandler { } pub(crate) async fn shutdown(&self) { - self.http_client.shutdown().await; + let tasks = { + let mut body_streams = self.body_streams.lock().await; + body_streams + .drain() + .filter_map(|(_, task)| task) + .collect::>() + }; + for task in tasks { + task.abort(); + } if let Some(session) = self.session() { session.detach().await; } @@ -166,13 +181,21 @@ impl ExecServerHandler { params: HttpRequestParams, ) -> Result<(), JSONRPCErrorError> { self.require_initialized_for("http")?; - let (response, mut pending_stream) = self.http_client.run_http_request(params).await?; + let stream_response = params.stream_response; + let http_request_id = params.request_id.clone(); + if stream_response { + self.reserve_http_body_stream(&http_request_id).await?; + } + let mut response = run_executor_http_request(params).await; + if response.is_err() && stream_response { + self.release_http_body_stream(&http_request_id).await; + } + let (response, mut pending_stream) = response?; let message = match to_value(response) { Ok(result) => RpcServerOutboundMessage::Response { request_id, result }, Err(err) => { if let Some(pending_stream) = pending_stream.take() { - self.http_client - .release_http_body_stream(&pending_stream.request_id) + self.release_http_body_stream(&pending_stream.request_id) .await; } RpcServerOutboundMessage::Error { @@ -185,9 +208,7 @@ impl ExecServerHandler { internal_error("RPC connection closed while sending http/request response".into()) })?; if let Some(pending_stream) = pending_stream { - self.http_client - .start_http_body_stream(pending_stream) - .await; + self.start_http_body_stream(pending_stream).await; } Ok(()) } @@ -287,6 +308,42 @@ impl ExecServerHandler { .unwrap_or_else(std::sync::PoisonError::into_inner) .clone() } + + async fn start_http_body_stream( + self: &Arc, + pending_stream: ExecutorPendingHttpBodyStream, + ) { + let request_id = pending_stream.request_id.clone(); + let finished_request_id = request_id.clone(); + let handler = Arc::clone(self); + let notifications = self.notifications.clone(); + let task = tokio::spawn(async move { + stream_executor_http_body(pending_stream, notifications).await; + handler.release_http_body_stream(&finished_request_id).await; + }); + let mut body_streams = self.body_streams.lock().await; + if let Some(entry) = body_streams.get_mut(&request_id) { + *entry = Some(task); + } else { + task.abort(); + } + } + + async fn release_http_body_stream(&self, request_id: &str) { + let mut body_streams = self.body_streams.lock().await; + body_streams.remove(request_id); + } + + async fn reserve_http_body_stream(&self, request_id: &str) -> Result<(), JSONRPCErrorError> { + let mut body_streams = self.body_streams.lock().await; + if body_streams.contains_key(request_id) { + return Err(invalid_params(format!( + "http/request streamResponse requestId `{request_id}` is already active" + ))); + } + body_streams.insert(request_id.to_string(), None); + Ok(()) + } } #[cfg(test)] From 06616d5deab602b43eb2657ec139069668089273 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 09:54:12 -0700 Subject: [PATCH 20/29] codex: clarify exec-server outbound sender naming Co-authored-by: Codex --- codex-rs/exec-server/src/server/handler.rs | 8 ++++---- codex-rs/exec-server/src/server/processor.rs | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 4b24a236496b..88d72c6e6415 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -52,7 +52,7 @@ use crate::server::session_registry::SessionRegistry; pub(crate) struct ExecServerHandler { session_registry: Arc, notifications: RpcNotificationSender, - outgoing_tx: mpsc::Sender, + server_outbound_tx: mpsc::Sender, session: StdMutex>, body_streams: Mutex>>>, file_system: FileSystemHandler, @@ -64,13 +64,13 @@ impl ExecServerHandler { pub(crate) fn new( session_registry: Arc, notifications: RpcNotificationSender, - outgoing_tx: mpsc::Sender, + server_outbound_tx: mpsc::Sender, runtime_paths: ExecServerRuntimePaths, ) -> Self { Self { session_registry, notifications, - outgoing_tx, + server_outbound_tx, session: StdMutex::new(None), body_streams: Mutex::new(HashMap::new()), file_system: FileSystemHandler::new(runtime_paths), @@ -204,7 +204,7 @@ impl ExecServerHandler { } } }; - self.outgoing_tx.send(message).await.map_err(|_| { + self.server_outbound_tx.send(message).await.map_err(|_| { internal_error("RPC connection closed while sending http/request response".into()) })?; if let Some(pending_stream) = pending_stream { diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index 129f04ce3ac5..f27a73753cc6 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -50,18 +50,18 @@ async fn run_connection( let router = Arc::new(build_router()); let (json_outgoing_tx, mut incoming_rx, mut disconnected_rx, connection_tasks) = connection.into_parts(); - let (outgoing_tx, mut outgoing_rx) = + let (server_outbound_tx, mut server_outbound_rx) = mpsc::channel::(CHANNEL_CAPACITY); - let notifications = RpcNotificationSender::new(outgoing_tx.clone()); + let notifications = RpcNotificationSender::new(server_outbound_tx.clone()); let handler = Arc::new(ExecServerHandler::new( session_registry, notifications, - outgoing_tx.clone(), + server_outbound_tx.clone(), runtime_paths, )); let outbound_task = tokio::spawn(async move { - while let Some(message) = outgoing_rx.recv().await { + while let Some(message) = server_outbound_rx.recv().await { let json_message = match encode_server_message(message) { Ok(json_message) => json_message, Err(err) => { @@ -84,7 +84,7 @@ async fn run_connection( match event { JsonRpcConnectionEvent::MalformedMessage { reason } => { warn!("ignoring malformed exec-server message: {reason}"); - if outgoing_tx + if server_outbound_tx .send(RpcServerOutboundMessage::Error { request_id: codex_app_server_protocol::RequestId::Integer(-1), error: invalid_request(reason), @@ -106,11 +106,11 @@ async fn run_connection( } }; if let RequestRouteOutcome::Message(message) = outcome - && outgoing_tx.send(message).await.is_err() + && server_outbound_tx.send(message).await.is_err() { break; } - } else if outgoing_tx + } else if server_outbound_tx .send(RpcServerOutboundMessage::Error { request_id: request.id, error: method_not_found(format!( @@ -173,7 +173,7 @@ async fn run_connection( handler.shutdown().await; drop(handler); - drop(outgoing_tx); + drop(server_outbound_tx); for task in connection_tasks { task.abort(); let _ = task.await; From cec5da2a5d90623e54f7412f032ab0a26fefee8c Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 09:55:53 -0700 Subject: [PATCH 21/29] codex: derive exec-server notifications from outbound sender Co-authored-by: Codex --- codex-rs/exec-server/src/server/handler.rs | 11 ++++++----- codex-rs/exec-server/src/server/handler/tests.rs | 13 ++++++------- codex-rs/exec-server/src/server/processor.rs | 3 --- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 88d72c6e6415..db8ed0096318 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -51,7 +51,6 @@ use crate::server::session_registry::SessionRegistry; pub(crate) struct ExecServerHandler { session_registry: Arc, - notifications: RpcNotificationSender, server_outbound_tx: mpsc::Sender, session: StdMutex>, body_streams: Mutex>>>, @@ -63,13 +62,11 @@ pub(crate) struct ExecServerHandler { impl ExecServerHandler { pub(crate) fn new( session_registry: Arc, - notifications: RpcNotificationSender, server_outbound_tx: mpsc::Sender, runtime_paths: ExecServerRuntimePaths, ) -> Self { Self { session_registry, - notifications, server_outbound_tx, session: StdMutex::new(None), body_streams: Mutex::new(HashMap::new()), @@ -112,7 +109,7 @@ impl ExecServerHandler { let session = match self .session_registry - .attach(params.resume_session_id.clone(), self.notifications.clone()) + .attach(params.resume_session_id.clone(), self.notification_sender()) .await { Ok(session) => session, @@ -316,7 +313,7 @@ impl ExecServerHandler { let request_id = pending_stream.request_id.clone(); let finished_request_id = request_id.clone(); let handler = Arc::clone(self); - let notifications = self.notifications.clone(); + let notifications = self.notification_sender(); let task = tokio::spawn(async move { stream_executor_http_body(pending_stream, notifications).await; handler.release_http_body_stream(&finished_request_id).await; @@ -344,6 +341,10 @@ impl ExecServerHandler { body_streams.insert(request_id.to_string(), None); Ok(()) } + + fn notification_sender(&self) -> RpcNotificationSender { + RpcNotificationSender::new(self.server_outbound_tx.clone()) + } } #[cfg(test)] diff --git a/codex-rs/exec-server/src/server/handler/tests.rs b/codex-rs/exec-server/src/server/handler/tests.rs index 7f2cdba629d8..f9c10a5bdb16 100644 --- a/codex-rs/exec-server/src/server/handler/tests.rs +++ b/codex-rs/exec-server/src/server/handler/tests.rs @@ -15,7 +15,6 @@ use crate::protocol::ReadParams; use crate::protocol::ReadResponse; use crate::protocol::TerminateParams; use crate::protocol::TerminateResponse; -use crate::rpc::RpcNotificationSender; use crate::server::session_registry::SessionRegistry; fn exec_params(process_id: &str) -> ExecParams { @@ -80,7 +79,7 @@ async fn initialized_handler() -> Arc { let registry = SessionRegistry::new(); let handler = Arc::new(ExecServerHandler::new( registry, - RpcNotificationSender::new(outgoing_tx), + outgoing_tx, test_runtime_paths(), )); let initialize_response = handler @@ -158,7 +157,7 @@ async fn long_poll_read_fails_after_session_resume() { let registry = SessionRegistry::new(); let first_handler = Arc::new(ExecServerHandler::new( Arc::clone(®istry), - RpcNotificationSender::new(first_tx), + first_tx, test_runtime_paths(), )); let initialize_response = first_handler @@ -199,7 +198,7 @@ async fn long_poll_read_fails_after_session_resume() { let (second_tx, _second_rx) = mpsc::channel(16); let second_handler = Arc::new(ExecServerHandler::new( registry, - RpcNotificationSender::new(second_tx), + second_tx, test_runtime_paths(), )); second_handler @@ -232,7 +231,7 @@ async fn active_session_resume_is_rejected() { let registry = SessionRegistry::new(); let first_handler = Arc::new(ExecServerHandler::new( Arc::clone(®istry), - RpcNotificationSender::new(first_tx), + first_tx, test_runtime_paths(), )); let initialize_response = first_handler @@ -246,7 +245,7 @@ async fn active_session_resume_is_rejected() { let (second_tx, _second_rx) = mpsc::channel(16); let second_handler = Arc::new(ExecServerHandler::new( registry, - RpcNotificationSender::new(second_tx), + second_tx, test_runtime_paths(), )); let err = second_handler @@ -274,7 +273,7 @@ async fn output_and_exit_are_retained_after_notification_receiver_closes() { let (outgoing_tx, outgoing_rx) = mpsc::channel(16); let handler = Arc::new(ExecServerHandler::new( SessionRegistry::new(), - RpcNotificationSender::new(outgoing_tx), + outgoing_tx, test_runtime_paths(), )); handler diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index f27a73753cc6..992f0908b651 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -9,7 +9,6 @@ use crate::connection::CHANNEL_CAPACITY; use crate::connection::JsonRpcConnection; use crate::connection::JsonRpcConnectionEvent; use crate::rpc::RequestRouteOutcome; -use crate::rpc::RpcNotificationSender; use crate::rpc::RpcServerOutboundMessage; use crate::rpc::encode_server_message; use crate::rpc::invalid_request; @@ -52,10 +51,8 @@ async fn run_connection( connection.into_parts(); let (server_outbound_tx, mut server_outbound_rx) = mpsc::channel::(CHANNEL_CAPACITY); - let notifications = RpcNotificationSender::new(server_outbound_tx.clone()); let handler = Arc::new(ExecServerHandler::new( session_registry, - notifications, server_outbound_tx.clone(), runtime_paths, )); From b0c4c26a0758c0dc12ccb96305585c8b9d21a454 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 09:59:22 -0700 Subject: [PATCH 22/29] codex: reuse rpc notification sender for responses Co-authored-by: Codex --- codex-rs/exec-server/src/rpc.rs | 52 +++++++++++++++---- codex-rs/exec-server/src/server/handler.rs | 17 +++--- .../exec-server/src/server/handler/tests.rs | 13 ++--- codex-rs/exec-server/src/server/processor.rs | 4 +- 4 files changed, 63 insertions(+), 23 deletions(-) diff --git a/codex-rs/exec-server/src/rpc.rs b/codex-rs/exec-server/src/rpc.rs index 96ad8fe13411..a7fb70b75921 100644 --- a/codex-rs/exec-server/src/rpc.rs +++ b/codex-rs/exec-server/src/rpc.rs @@ -77,6 +77,41 @@ impl RpcNotificationSender { Self { outgoing_tx } } + pub(crate) async fn send( + &self, + message: RpcServerOutboundMessage, + closed_error_message: &'static str, + ) -> Result<(), JSONRPCErrorError> { + self.outgoing_tx + .send(message) + .await + .map_err(|_| internal_error(closed_error_message.into())) + } + + pub(crate) async fn respond( + &self, + request_id: RequestId, + result: Value, + ) -> Result<(), JSONRPCErrorError> { + self.send( + RpcServerOutboundMessage::Response { request_id, result }, + "RPC connection closed while sending response", + ) + .await + } + + pub(crate) async fn respond_error( + &self, + request_id: RequestId, + error: JSONRPCErrorError, + ) -> Result<(), JSONRPCErrorError> { + self.send( + RpcServerOutboundMessage::Error { request_id, error }, + "RPC connection closed while sending error response", + ) + .await + } + #[allow(dead_code)] pub(crate) async fn notify( &self, @@ -84,15 +119,14 @@ impl RpcNotificationSender { params: &P, ) -> Result<(), JSONRPCErrorError> { let params = serde_json::to_value(params).map_err(|err| internal_error(err.to_string()))?; - self.outgoing_tx - .send(RpcServerOutboundMessage::Notification( - JSONRPCNotification { - method: method.to_string(), - params: Some(params), - }, - )) - .await - .map_err(|_| internal_error("RPC connection closed while sending notification".into())) + self.send( + RpcServerOutboundMessage::Notification(JSONRPCNotification { + method: method.to_string(), + params: Some(params), + }), + "RPC connection closed while sending notification", + ) + .await } } diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index db8ed0096318..7ddc888b650b 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -51,7 +51,7 @@ use crate::server::session_registry::SessionRegistry; pub(crate) struct ExecServerHandler { session_registry: Arc, - server_outbound_tx: mpsc::Sender, + notifications: RpcNotificationSender, session: StdMutex>, body_streams: Mutex>>>, file_system: FileSystemHandler, @@ -62,12 +62,12 @@ pub(crate) struct ExecServerHandler { impl ExecServerHandler { pub(crate) fn new( session_registry: Arc, - server_outbound_tx: mpsc::Sender, + notifications: RpcNotificationSender, runtime_paths: ExecServerRuntimePaths, ) -> Self { Self { session_registry, - server_outbound_tx, + notifications, session: StdMutex::new(None), body_streams: Mutex::new(HashMap::new()), file_system: FileSystemHandler::new(runtime_paths), @@ -201,9 +201,12 @@ impl ExecServerHandler { } } }; - self.server_outbound_tx.send(message).await.map_err(|_| { - internal_error("RPC connection closed while sending http/request response".into()) - })?; + self.notifications + .send( + message, + "RPC connection closed while sending http/request response", + ) + .await?; if let Some(pending_stream) = pending_stream { self.start_http_body_stream(pending_stream).await; } @@ -343,7 +346,7 @@ impl ExecServerHandler { } fn notification_sender(&self) -> RpcNotificationSender { - RpcNotificationSender::new(self.server_outbound_tx.clone()) + self.notifications.clone() } } diff --git a/codex-rs/exec-server/src/server/handler/tests.rs b/codex-rs/exec-server/src/server/handler/tests.rs index f9c10a5bdb16..7f2cdba629d8 100644 --- a/codex-rs/exec-server/src/server/handler/tests.rs +++ b/codex-rs/exec-server/src/server/handler/tests.rs @@ -15,6 +15,7 @@ use crate::protocol::ReadParams; use crate::protocol::ReadResponse; use crate::protocol::TerminateParams; use crate::protocol::TerminateResponse; +use crate::rpc::RpcNotificationSender; use crate::server::session_registry::SessionRegistry; fn exec_params(process_id: &str) -> ExecParams { @@ -79,7 +80,7 @@ async fn initialized_handler() -> Arc { let registry = SessionRegistry::new(); let handler = Arc::new(ExecServerHandler::new( registry, - outgoing_tx, + RpcNotificationSender::new(outgoing_tx), test_runtime_paths(), )); let initialize_response = handler @@ -157,7 +158,7 @@ async fn long_poll_read_fails_after_session_resume() { let registry = SessionRegistry::new(); let first_handler = Arc::new(ExecServerHandler::new( Arc::clone(®istry), - first_tx, + RpcNotificationSender::new(first_tx), test_runtime_paths(), )); let initialize_response = first_handler @@ -198,7 +199,7 @@ async fn long_poll_read_fails_after_session_resume() { let (second_tx, _second_rx) = mpsc::channel(16); let second_handler = Arc::new(ExecServerHandler::new( registry, - second_tx, + RpcNotificationSender::new(second_tx), test_runtime_paths(), )); second_handler @@ -231,7 +232,7 @@ async fn active_session_resume_is_rejected() { let registry = SessionRegistry::new(); let first_handler = Arc::new(ExecServerHandler::new( Arc::clone(®istry), - first_tx, + RpcNotificationSender::new(first_tx), test_runtime_paths(), )); let initialize_response = first_handler @@ -245,7 +246,7 @@ async fn active_session_resume_is_rejected() { let (second_tx, _second_rx) = mpsc::channel(16); let second_handler = Arc::new(ExecServerHandler::new( registry, - second_tx, + RpcNotificationSender::new(second_tx), test_runtime_paths(), )); let err = second_handler @@ -273,7 +274,7 @@ async fn output_and_exit_are_retained_after_notification_receiver_closes() { let (outgoing_tx, outgoing_rx) = mpsc::channel(16); let handler = Arc::new(ExecServerHandler::new( SessionRegistry::new(), - outgoing_tx, + RpcNotificationSender::new(outgoing_tx), test_runtime_paths(), )); handler diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index 992f0908b651..49c00f9c46ea 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -9,6 +9,7 @@ use crate::connection::CHANNEL_CAPACITY; use crate::connection::JsonRpcConnection; use crate::connection::JsonRpcConnectionEvent; use crate::rpc::RequestRouteOutcome; +use crate::rpc::RpcNotificationSender; use crate::rpc::RpcServerOutboundMessage; use crate::rpc::encode_server_message; use crate::rpc::invalid_request; @@ -51,9 +52,10 @@ async fn run_connection( connection.into_parts(); let (server_outbound_tx, mut server_outbound_rx) = mpsc::channel::(CHANNEL_CAPACITY); + let notifications = RpcNotificationSender::new(server_outbound_tx.clone()); let handler = Arc::new(ExecServerHandler::new( session_registry, - server_outbound_tx.clone(), + notifications, runtime_paths, )); From 9bd9c444e1edeb6ca3eec3e25382131555c69861 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 10:06:27 -0700 Subject: [PATCH 23/29] codex: trim exec-server http routing churn Co-authored-by: Codex --- codex-rs/exec-server/src/rpc.rs | 128 +++---------------- codex-rs/exec-server/src/server/handler.rs | 49 ++----- codex-rs/exec-server/src/server/processor.rs | 100 +++++++++++++-- codex-rs/exec-server/src/server/registry.rs | 8 -- 4 files changed, 115 insertions(+), 170 deletions(-) diff --git a/codex-rs/exec-server/src/rpc.rs b/codex-rs/exec-server/src/rpc.rs index a7fb70b75921..e82b4a0ea233 100644 --- a/codex-rs/exec-server/src/rpc.rs +++ b/codex-rs/exec-server/src/rpc.rs @@ -37,7 +37,7 @@ pub(crate) enum RpcCallError { type PendingRequest = oneshot::Sender>; type BoxFuture = Pin + Send + 'static>>; type RequestRoute = - Box, JSONRPCRequest) -> BoxFuture + Send + Sync>; + Box, JSONRPCRequest) -> BoxFuture + Send + Sync>; type NotificationRoute = Box, JSONRPCNotification) -> BoxFuture> + Send + Sync>; @@ -61,11 +61,6 @@ pub(crate) enum RpcServerOutboundMessage { Notification(JSONRPCNotification), } -pub(crate) enum RequestRouteOutcome { - Message(RpcServerOutboundMessage), - AlreadySent, -} - #[allow(dead_code)] #[derive(Clone)] pub(crate) struct RpcNotificationSender { @@ -77,41 +72,6 @@ impl RpcNotificationSender { Self { outgoing_tx } } - pub(crate) async fn send( - &self, - message: RpcServerOutboundMessage, - closed_error_message: &'static str, - ) -> Result<(), JSONRPCErrorError> { - self.outgoing_tx - .send(message) - .await - .map_err(|_| internal_error(closed_error_message.into())) - } - - pub(crate) async fn respond( - &self, - request_id: RequestId, - result: Value, - ) -> Result<(), JSONRPCErrorError> { - self.send( - RpcServerOutboundMessage::Response { request_id, result }, - "RPC connection closed while sending response", - ) - .await - } - - pub(crate) async fn respond_error( - &self, - request_id: RequestId, - error: JSONRPCErrorError, - ) -> Result<(), JSONRPCErrorError> { - self.send( - RpcServerOutboundMessage::Error { request_id, error }, - "RPC connection closed while sending error response", - ) - .await - } - #[allow(dead_code)] pub(crate) async fn notify( &self, @@ -119,14 +79,15 @@ impl RpcNotificationSender { params: &P, ) -> Result<(), JSONRPCErrorError> { let params = serde_json::to_value(params).map_err(|err| internal_error(err.to_string()))?; - self.send( - RpcServerOutboundMessage::Notification(JSONRPCNotification { - method: method.to_string(), - params: Some(params), - }), - "RPC connection closed while sending notification", - ) - .await + self.outgoing_tx + .send(RpcServerOutboundMessage::Notification( + JSONRPCNotification { + method: method.to_string(), + params: Some(params), + }, + )) + .await + .map_err(|_| internal_error("RPC connection closed while sending notification".into())) } } @@ -170,69 +131,18 @@ where let response = match response { Ok(response) => response.await, Err(error) => { - return RequestRouteOutcome::Message(RpcServerOutboundMessage::Error { - request_id, - error, - }); + return RpcServerOutboundMessage::Error { request_id, error }; } }; match response { - Ok(result) => { - match serde_json::to_value(result) { - Ok(result) => RequestRouteOutcome::Message( - RpcServerOutboundMessage::Response { request_id, result }, - ), - Err(err) => { - RequestRouteOutcome::Message(RpcServerOutboundMessage::Error { - request_id, - error: internal_error(err.to_string()), - }) - } - } - } - Err(error) => { - RequestRouteOutcome::Message(RpcServerOutboundMessage::Error { - request_id, - error, - }) - } - } - }) - }), - ); - } - - pub(crate) fn request_already_sent(&mut self, method: &'static str, handler: F) - where - P: DeserializeOwned + Send + 'static, - F: Fn(Arc, RequestId, P) -> Fut + Send + Sync + 'static, - Fut: Future> + Send + 'static, - { - self.request_routes.insert( - method, - Box::new(move |state, request| { - let request_id = request.id; - let params = request.params; - let response = decode_request_params::

(params) - .map(|params| handler(state, request_id.clone(), params)); - Box::pin(async move { - let response = match response { - Ok(response) => response.await, - Err(error) => { - return RequestRouteOutcome::Message(RpcServerOutboundMessage::Error { + Ok(result) => match serde_json::to_value(result) { + Ok(result) => RpcServerOutboundMessage::Response { request_id, result }, + Err(err) => RpcServerOutboundMessage::Error { request_id, - error, - }); - } - }; - match response { - Ok(()) => RequestRouteOutcome::AlreadySent, - Err(error) => { - RequestRouteOutcome::Message(RpcServerOutboundMessage::Error { - request_id, - error, - }) - } + error: internal_error(err.to_string()), + }, + }, + Err(error) => RpcServerOutboundMessage::Error { request_id, error }, } }) }), @@ -484,7 +394,7 @@ pub(crate) fn internal_error(message: String) -> JSONRPCErrorError { } } -pub(crate) fn decode_request_params

(params: Option) -> Result +fn decode_request_params

(params: Option) -> Result where P: DeserializeOwned, { diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 7ddc888b650b..42efcf203e04 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -4,15 +4,13 @@ use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use codex_app_server_protocol::JSONRPCErrorError; -use codex_app_server_protocol::RequestId; -use serde_json::to_value; use std::collections::HashMap; use tokio::sync::Mutex; -use tokio::sync::mpsc; use tokio::task::JoinHandle; use crate::ExecServerRuntimePaths; use crate::client::http_client::ExecutorPendingHttpBodyStream; +use crate::client::http_client::HttpRequestResponse; use crate::client::http_client::run_executor_http_request; use crate::client::http_client::stream_executor_http_body; use crate::protocol::ExecParams; @@ -41,8 +39,6 @@ use crate::protocol::TerminateResponse; use crate::protocol::WriteParams; use crate::protocol::WriteResponse; use crate::rpc::RpcNotificationSender; -use crate::rpc::RpcServerOutboundMessage; -use crate::rpc::internal_error; use crate::rpc::invalid_params; use crate::rpc::invalid_request; use crate::server::file_system_handler::FileSystemHandler; @@ -109,7 +105,7 @@ impl ExecServerHandler { let session = match self .session_registry - .attach(params.resume_session_id.clone(), self.notification_sender()) + .attach(params.resume_session_id.clone(), self.notifications.clone()) .await { Ok(session) => session, @@ -173,10 +169,10 @@ impl ExecServerHandler { } pub(crate) async fn http_request( - self: &Arc, - request_id: RequestId, + &self, params: HttpRequestParams, - ) -> Result<(), JSONRPCErrorError> { + ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> + { self.require_initialized_for("http")?; let stream_response = params.stream_response; let http_request_id = params.request_id.clone(); @@ -187,30 +183,7 @@ impl ExecServerHandler { if response.is_err() && stream_response { self.release_http_body_stream(&http_request_id).await; } - let (response, mut pending_stream) = response?; - let message = match to_value(response) { - Ok(result) => RpcServerOutboundMessage::Response { request_id, result }, - Err(err) => { - if let Some(pending_stream) = pending_stream.take() { - self.release_http_body_stream(&pending_stream.request_id) - .await; - } - RpcServerOutboundMessage::Error { - request_id, - error: internal_error(err.to_string()), - } - } - }; - self.notifications - .send( - message, - "RPC connection closed while sending http/request response", - ) - .await?; - if let Some(pending_stream) = pending_stream { - self.start_http_body_stream(pending_stream).await; - } - Ok(()) + response } pub(crate) async fn fs_read_file( @@ -309,14 +282,14 @@ impl ExecServerHandler { .clone() } - async fn start_http_body_stream( + pub(crate) async fn start_http_body_stream( self: &Arc, pending_stream: ExecutorPendingHttpBodyStream, ) { let request_id = pending_stream.request_id.clone(); let finished_request_id = request_id.clone(); let handler = Arc::clone(self); - let notifications = self.notification_sender(); + let notifications = self.notifications.clone(); let task = tokio::spawn(async move { stream_executor_http_body(pending_stream, notifications).await; handler.release_http_body_stream(&finished_request_id).await; @@ -329,7 +302,7 @@ impl ExecServerHandler { } } - async fn release_http_body_stream(&self, request_id: &str) { + pub(crate) async fn release_http_body_stream(&self, request_id: &str) { let mut body_streams = self.body_streams.lock().await; body_streams.remove(request_id); } @@ -344,10 +317,6 @@ impl ExecServerHandler { body_streams.insert(request_id.to_string(), None); Ok(()) } - - fn notification_sender(&self) -> RpcNotificationSender { - self.notifications.clone() - } } #[cfg(test)] diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index 49c00f9c46ea..8ff075a134a0 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -1,5 +1,7 @@ use std::sync::Arc; +use serde_json::Value; +use serde_json::to_value; use tokio::sync::mpsc; use tracing::debug; use tracing::warn; @@ -8,10 +10,13 @@ use crate::ExecServerRuntimePaths; use crate::connection::CHANNEL_CAPACITY; use crate::connection::JsonRpcConnection; use crate::connection::JsonRpcConnectionEvent; -use crate::rpc::RequestRouteOutcome; +use crate::protocol::HTTP_REQUEST_METHOD; +use crate::protocol::HttpRequestParams; use crate::rpc::RpcNotificationSender; use crate::rpc::RpcServerOutboundMessage; use crate::rpc::encode_server_message; +use crate::rpc::internal_error; +use crate::rpc::invalid_params; use crate::rpc::invalid_request; use crate::rpc::method_not_found; use crate::server::ExecServerHandler; @@ -50,9 +55,9 @@ async fn run_connection( let router = Arc::new(build_router()); let (json_outgoing_tx, mut incoming_rx, mut disconnected_rx, connection_tasks) = connection.into_parts(); - let (server_outbound_tx, mut server_outbound_rx) = + let (outgoing_tx, mut outgoing_rx) = mpsc::channel::(CHANNEL_CAPACITY); - let notifications = RpcNotificationSender::new(server_outbound_tx.clone()); + let notifications = RpcNotificationSender::new(outgoing_tx.clone()); let handler = Arc::new(ExecServerHandler::new( session_registry, notifications, @@ -60,7 +65,7 @@ async fn run_connection( )); let outbound_task = tokio::spawn(async move { - while let Some(message) = server_outbound_rx.recv().await { + while let Some(message) = outgoing_rx.recv().await { let json_message = match encode_server_message(message) { Ok(json_message) => json_message, Err(err) => { @@ -83,7 +88,7 @@ async fn run_connection( match event { JsonRpcConnectionEvent::MalformedMessage { reason } => { warn!("ignoring malformed exec-server message: {reason}"); - if server_outbound_tx + if outgoing_tx .send(RpcServerOutboundMessage::Error { request_id: codex_app_server_protocol::RequestId::Integer(-1), error: invalid_request(reason), @@ -96,20 +101,73 @@ async fn run_connection( } JsonRpcConnectionEvent::Message(message) => match message { codex_app_server_protocol::JSONRPCMessage::Request(request) => { - if let Some(route) = router.request_route(request.method.as_str()) { - let outcome = tokio::select! { - outcome = route(Arc::clone(&handler), request) => outcome, + if request.method == HTTP_REQUEST_METHOD { + let request_id = request.id; + let params = match decode_http_request_params(request.params) { + Ok(params) => params, + Err(error) => { + if outgoing_tx + .send(RpcServerOutboundMessage::Error { request_id, error }) + .await + .is_err() + { + break; + } + continue; + } + }; + let response = tokio::select! { + response = handler.http_request(params) => response, _ = disconnected_rx.changed() => { debug!("exec-server transport disconnected while handling request"); break; } }; - if let RequestRouteOutcome::Message(message) = outcome - && server_outbound_tx.send(message).await.is_err() - { + let (response, mut pending_stream) = match response { + Ok(response) => response, + Err(error) => { + if outgoing_tx + .send(RpcServerOutboundMessage::Error { request_id, error }) + .await + .is_err() + { + break; + } + continue; + } + }; + let message = match to_value(response) { + Ok(result) => RpcServerOutboundMessage::Response { request_id, result }, + Err(err) => { + if let Some(pending_stream) = pending_stream.take() { + handler + .release_http_body_stream(&pending_stream.request_id) + .await; + } + RpcServerOutboundMessage::Error { + request_id, + error: internal_error(err.to_string()), + } + } + }; + if outgoing_tx.send(message).await.is_err() { break; } - } else if server_outbound_tx + if let Some(pending_stream) = pending_stream { + handler.start_http_body_stream(pending_stream).await; + } + } else if let Some(route) = router.request_route(request.method.as_str()) { + let message = tokio::select! { + message = route(Arc::clone(&handler), request) => message, + _ = disconnected_rx.changed() => { + debug!("exec-server transport disconnected while handling request"); + break; + } + }; + if outgoing_tx.send(message).await.is_err() { + break; + } + } else if outgoing_tx .send(RpcServerOutboundMessage::Error { request_id: request.id, error: method_not_found(format!( @@ -172,7 +230,7 @@ async fn run_connection( handler.shutdown().await; drop(handler); - drop(server_outbound_tx); + drop(outgoing_tx); for task in connection_tasks { task.abort(); let _ = task.await; @@ -180,6 +238,22 @@ async fn run_connection( let _ = outbound_task.await; } +fn decode_http_request_params( + params: Option, +) -> Result { + let params = params.unwrap_or(Value::Null); + match serde_json::from_value(params.clone()) { + Ok(params) => Ok(params), + Err(err) => { + if matches!(params, Value::Object(ref map) if map.is_empty()) { + serde_json::from_value(Value::Null).map_err(|_| invalid_params(err.to_string())) + } else { + Err(invalid_params(err.to_string())) + } + } + } +} + #[cfg(test)] mod tests { use std::collections::HashMap; diff --git a/codex-rs/exec-server/src/server/registry.rs b/codex-rs/exec-server/src/server/registry.rs index 5f4821701844..a57704c50502 100644 --- a/codex-rs/exec-server/src/server/registry.rs +++ b/codex-rs/exec-server/src/server/registry.rs @@ -19,8 +19,6 @@ use crate::protocol::FsReadDirectoryParams; use crate::protocol::FsReadFileParams; use crate::protocol::FsRemoveParams; use crate::protocol::FsWriteFileParams; -use crate::protocol::HTTP_REQUEST_METHOD; -use crate::protocol::HttpRequestParams; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeParams; @@ -44,12 +42,6 @@ pub(crate) fn build_router() -> RpcRouter { handler.initialize(params).await }, ); - router.request_already_sent( - HTTP_REQUEST_METHOD, - |handler: Arc, request_id, params: HttpRequestParams| async move { - handler.http_request(request_id, params).await - }, - ); router.request( EXEC_METHOD, |handler: Arc, params: ExecParams| async move { handler.exec(params).await }, From ab5e3597dff9cbb7e8c7a2cb298210ebd355fa29 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 10:12:29 -0700 Subject: [PATCH 24/29] codex: simplify exec-server http routing Co-authored-by: Codex --- codex-rs/exec-server/src/rpc.rs | 48 ++++++++++- codex-rs/exec-server/src/server/handler.rs | 37 +++++++-- codex-rs/exec-server/src/server/processor.rs | 83 +------------------- codex-rs/exec-server/src/server/registry.rs | 8 ++ 4 files changed, 86 insertions(+), 90 deletions(-) diff --git a/codex-rs/exec-server/src/rpc.rs b/codex-rs/exec-server/src/rpc.rs index e82b4a0ea233..723b99f5028d 100644 --- a/codex-rs/exec-server/src/rpc.rs +++ b/codex-rs/exec-server/src/rpc.rs @@ -36,8 +36,9 @@ pub(crate) enum RpcCallError { type PendingRequest = oneshot::Sender>; type BoxFuture = Pin + Send + 'static>>; -type RequestRoute = - Box, JSONRPCRequest) -> BoxFuture + Send + Sync>; +type RequestRoute = Box< + dyn Fn(Arc, JSONRPCRequest) -> BoxFuture> + Send + Sync, +>; type NotificationRoute = Box, JSONRPCNotification) -> BoxFuture> + Send + Sync>; @@ -72,6 +73,17 @@ impl RpcNotificationSender { Self { outgoing_tx } } + pub(crate) async fn response( + &self, + request_id: RequestId, + result: Value, + ) -> Result<(), JSONRPCErrorError> { + self.outgoing_tx + .send(RpcServerOutboundMessage::Response { request_id, result }) + .await + .map_err(|_| internal_error("RPC connection closed while sending response".into())) + } + #[allow(dead_code)] pub(crate) async fn notify( &self, @@ -131,10 +143,10 @@ where let response = match response { Ok(response) => response.await, Err(error) => { - return RpcServerOutboundMessage::Error { request_id, error }; + return Some(RpcServerOutboundMessage::Error { request_id, error }); } }; - match response { + Some(match response { Ok(result) => match serde_json::to_value(result) { Ok(result) => RpcServerOutboundMessage::Response { request_id, result }, Err(err) => RpcServerOutboundMessage::Error { @@ -143,6 +155,34 @@ where }, }, Err(error) => RpcServerOutboundMessage::Error { request_id, error }, + }) + }) + }), + ); + } + + pub(crate) fn request_with_id(&mut self, method: &'static str, handler: F) + where + P: DeserializeOwned + Send + 'static, + F: Fn(Arc, RequestId, P) -> Fut + Send + Sync + 'static, + Fut: Future> + Send + 'static, + { + self.request_routes.insert( + method, + Box::new(move |state, request| { + let request_id = request.id; + let params = decode_request_params::

(request.params) + .map(|params| handler(state, request_id.clone(), params)); + Box::pin(async move { + let response = match params { + Ok(response) => response.await, + Err(error) => { + return Some(RpcServerOutboundMessage::Error { request_id, error }); + } + }; + match response { + Ok(()) => None, + Err(error) => Some(RpcServerOutboundMessage::Error { request_id, error }), } }) }), diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 42efcf203e04..d99e9ff46397 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -4,13 +4,14 @@ use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use codex_app_server_protocol::JSONRPCErrorError; +use codex_app_server_protocol::RequestId; +use serde_json::to_value; use std::collections::HashMap; use tokio::sync::Mutex; use tokio::task::JoinHandle; use crate::ExecServerRuntimePaths; use crate::client::http_client::ExecutorPendingHttpBodyStream; -use crate::client::http_client::HttpRequestResponse; use crate::client::http_client::run_executor_http_request; use crate::client::http_client::stream_executor_http_body; use crate::protocol::ExecParams; @@ -39,6 +40,7 @@ use crate::protocol::TerminateResponse; use crate::protocol::WriteParams; use crate::protocol::WriteResponse; use crate::rpc::RpcNotificationSender; +use crate::rpc::internal_error; use crate::rpc::invalid_params; use crate::rpc::invalid_request; use crate::server::file_system_handler::FileSystemHandler; @@ -169,10 +171,10 @@ impl ExecServerHandler { } pub(crate) async fn http_request( - &self, + self: &Arc, + request_id: RequestId, params: HttpRequestParams, - ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> - { + ) -> Result<(), JSONRPCErrorError> { self.require_initialized_for("http")?; let stream_response = params.stream_response; let http_request_id = params.request_id.clone(); @@ -183,7 +185,28 @@ impl ExecServerHandler { if response.is_err() && stream_response { self.release_http_body_stream(&http_request_id).await; } - response + let (response, mut pending_stream) = response?; + let result = match to_value(response) { + Ok(result) => result, + Err(err) => { + if let Some(pending_stream) = pending_stream.take() { + self.release_http_body_stream(&pending_stream.request_id) + .await; + } + return Err(internal_error(err.to_string())); + } + }; + if let Err(error) = self.notifications.response(request_id, result).await { + if let Some(pending_stream) = pending_stream.take() { + self.release_http_body_stream(&pending_stream.request_id) + .await; + } + return Err(error); + } + if let Some(pending_stream) = pending_stream { + self.start_http_body_stream(pending_stream).await; + } + Ok(()) } pub(crate) async fn fs_read_file( @@ -282,7 +305,7 @@ impl ExecServerHandler { .clone() } - pub(crate) async fn start_http_body_stream( + async fn start_http_body_stream( self: &Arc, pending_stream: ExecutorPendingHttpBodyStream, ) { @@ -302,7 +325,7 @@ impl ExecServerHandler { } } - pub(crate) async fn release_http_body_stream(&self, request_id: &str) { + async fn release_http_body_stream(&self, request_id: &str) { let mut body_streams = self.body_streams.lock().await; body_streams.remove(request_id); } diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index 8ff075a134a0..dc1a9b9ffe74 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -1,7 +1,5 @@ use std::sync::Arc; -use serde_json::Value; -use serde_json::to_value; use tokio::sync::mpsc; use tracing::debug; use tracing::warn; @@ -10,13 +8,9 @@ use crate::ExecServerRuntimePaths; use crate::connection::CHANNEL_CAPACITY; use crate::connection::JsonRpcConnection; use crate::connection::JsonRpcConnectionEvent; -use crate::protocol::HTTP_REQUEST_METHOD; -use crate::protocol::HttpRequestParams; use crate::rpc::RpcNotificationSender; use crate::rpc::RpcServerOutboundMessage; use crate::rpc::encode_server_message; -use crate::rpc::internal_error; -use crate::rpc::invalid_params; use crate::rpc::invalid_request; use crate::rpc::method_not_found; use crate::server::ExecServerHandler; @@ -101,62 +95,7 @@ async fn run_connection( } JsonRpcConnectionEvent::Message(message) => match message { codex_app_server_protocol::JSONRPCMessage::Request(request) => { - if request.method == HTTP_REQUEST_METHOD { - let request_id = request.id; - let params = match decode_http_request_params(request.params) { - Ok(params) => params, - Err(error) => { - if outgoing_tx - .send(RpcServerOutboundMessage::Error { request_id, error }) - .await - .is_err() - { - break; - } - continue; - } - }; - let response = tokio::select! { - response = handler.http_request(params) => response, - _ = disconnected_rx.changed() => { - debug!("exec-server transport disconnected while handling request"); - break; - } - }; - let (response, mut pending_stream) = match response { - Ok(response) => response, - Err(error) => { - if outgoing_tx - .send(RpcServerOutboundMessage::Error { request_id, error }) - .await - .is_err() - { - break; - } - continue; - } - }; - let message = match to_value(response) { - Ok(result) => RpcServerOutboundMessage::Response { request_id, result }, - Err(err) => { - if let Some(pending_stream) = pending_stream.take() { - handler - .release_http_body_stream(&pending_stream.request_id) - .await; - } - RpcServerOutboundMessage::Error { - request_id, - error: internal_error(err.to_string()), - } - } - }; - if outgoing_tx.send(message).await.is_err() { - break; - } - if let Some(pending_stream) = pending_stream { - handler.start_http_body_stream(pending_stream).await; - } - } else if let Some(route) = router.request_route(request.method.as_str()) { + if let Some(route) = router.request_route(request.method.as_str()) { let message = tokio::select! { message = route(Arc::clone(&handler), request) => message, _ = disconnected_rx.changed() => { @@ -164,7 +103,9 @@ async fn run_connection( break; } }; - if outgoing_tx.send(message).await.is_err() { + if let Some(message) = message + && outgoing_tx.send(message).await.is_err() + { break; } } else if outgoing_tx @@ -238,22 +179,6 @@ async fn run_connection( let _ = outbound_task.await; } -fn decode_http_request_params( - params: Option, -) -> Result { - let params = params.unwrap_or(Value::Null); - match serde_json::from_value(params.clone()) { - Ok(params) => Ok(params), - Err(err) => { - if matches!(params, Value::Object(ref map) if map.is_empty()) { - serde_json::from_value(Value::Null).map_err(|_| invalid_params(err.to_string())) - } else { - Err(invalid_params(err.to_string())) - } - } - } -} - #[cfg(test)] mod tests { use std::collections::HashMap; diff --git a/codex-rs/exec-server/src/server/registry.rs b/codex-rs/exec-server/src/server/registry.rs index a57704c50502..87dee6aa5807 100644 --- a/codex-rs/exec-server/src/server/registry.rs +++ b/codex-rs/exec-server/src/server/registry.rs @@ -19,6 +19,8 @@ use crate::protocol::FsReadDirectoryParams; use crate::protocol::FsReadFileParams; use crate::protocol::FsRemoveParams; use crate::protocol::FsWriteFileParams; +use crate::protocol::HTTP_REQUEST_METHOD; +use crate::protocol::HttpRequestParams; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeParams; @@ -42,6 +44,12 @@ pub(crate) fn build_router() -> RpcRouter { handler.initialize(params).await }, ); + router.request_with_id( + HTTP_REQUEST_METHOD, + |handler: Arc, request_id, params: HttpRequestParams| async move { + handler.http_request(request_id, params).await + }, + ); router.request( EXEC_METHOD, |handler: Arc, params: ExecParams| async move { handler.exec(params).await }, From b602d4720d85d3828acd3818bf401b5207c14930 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 10:18:56 -0700 Subject: [PATCH 25/29] codex: wrap executor http requests in a runner Co-authored-by: Codex --- .../exec-server/src/client/http_client.rs | 299 ++++++++++-------- 1 file changed, 164 insertions(+), 135 deletions(-) diff --git a/codex-rs/exec-server/src/client/http_client.rs b/codex-rs/exec-server/src/client/http_client.rs index 26e1a1cf315f..fabab7590326 100644 --- a/codex-rs/exec-server/src/client/http_client.rs +++ b/codex-rs/exec-server/src/client/http_client.rs @@ -38,6 +38,10 @@ pub(crate) struct ExecutorPendingHttpBodyStream { response: reqwest::Response, } +struct ExecutorHttpRequestRunner { + client: reqwest::Client, +} + /// Request-scoped stream of body chunks for an executor HTTP response. /// /// The initial `http/request` call returns status and headers. This stream then @@ -349,160 +353,185 @@ fn spawn_remove_http_body_stream(inner: Arc, request_id: String) { pub(crate) async fn run_executor_http_request( params: HttpRequestParams, ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> { - let method = Method::from_bytes(params.method.as_bytes()) - .map_err(|err| invalid_params(format!("http/request method is invalid: {err}")))?; - let url = Url::parse(¶ms.url) - .map_err(|err| invalid_params(format!("http/request url is invalid: {err}")))?; - match url.scheme() { - "http" | "https" => {} - scheme => { - return Err(invalid_params(format!( - "http/request only supports http and https URLs, got {scheme}" - ))); + ExecutorHttpRequestRunner::new(params.timeout_ms)? + .run(params) + .await +} + +pub(crate) async fn stream_executor_http_body( + pending_stream: ExecutorPendingHttpBodyStream, + notifications: RpcNotificationSender, +) { + ExecutorHttpRequestRunner::stream_body(pending_stream, notifications).await; +} + +async fn send_executor_body_delta( + notifications: &RpcNotificationSender, + delta: HttpRequestBodyDeltaNotification, +) -> bool { + notifications + .notify(HTTP_REQUEST_BODY_DELTA_METHOD, &delta) + .await + .is_ok() +} + +impl ExecutorHttpRequestRunner { + fn new(timeout_ms: Option) -> Result { + let client = match timeout_ms { + None => reqwest::Client::builder(), + Some(timeout_ms) => { + reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)) + } } + .build() + .map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?; + Ok(Self { client }) } - let headers = build_executor_http_headers(params.headers)?; - let client = match params.timeout_ms { - None => reqwest::Client::builder(), - Some(timeout_ms) => reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)), - } - .build() - .map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?; + async fn run( + &self, + params: HttpRequestParams, + ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> + { + let method = Method::from_bytes(params.method.as_bytes()) + .map_err(|err| invalid_params(format!("http/request method is invalid: {err}")))?; + let url = Url::parse(¶ms.url) + .map_err(|err| invalid_params(format!("http/request url is invalid: {err}")))?; + match url.scheme() { + "http" | "https" => {} + scheme => { + return Err(invalid_params(format!( + "http/request only supports http and https URLs, got {scheme}" + ))); + } + } - let mut request = client.request(method, url).headers(headers); - if let Some(body) = params.body { - request = request.body(body.into_inner()); - } + let headers = Self::build_headers(params.headers)?; + let mut request = self.client.request(method, url).headers(headers); + if let Some(body) = params.body { + request = request.body(body.into_inner()); + } - let response = request - .send() - .await - .map_err(|err| internal_error(format!("http/request failed: {err}")))?; - let status = response.status().as_u16(); - let headers = executor_response_headers(response.headers()); + let response = request + .send() + .await + .map_err(|err| internal_error(format!("http/request failed: {err}")))?; + let status = response.status().as_u16(); + let headers = Self::response_headers(response.headers()); + + if params.stream_response { + return Ok(( + HttpRequestResponse { + status, + headers, + body: Vec::new().into(), + }, + Some(ExecutorPendingHttpBodyStream { + request_id: params.request_id, + response, + }), + )); + } - if params.stream_response { - return Ok(( + let body = response.bytes().await.map_err(|err| { + internal_error(format!("failed to read http/request response body: {err}")) + })?; + + Ok(( HttpRequestResponse { status, headers, - body: Vec::new().into(), + body: body.to_vec().into(), }, - Some(ExecutorPendingHttpBodyStream { - request_id: params.request_id, - response, - }), - )); + None, + )) } - let body = response.bytes().await.map_err(|err| { - internal_error(format!("failed to read http/request response body: {err}")) - })?; - - Ok(( - HttpRequestResponse { - status, - headers, - body: body.to_vec().into(), - }, - None, - )) -} - -fn build_executor_http_headers(headers: Vec) -> Result { - let mut header_map = HeaderMap::new(); - for header in headers { - let name = HeaderName::from_bytes(header.name.as_bytes()) - .map_err(|err| invalid_params(format!("http/request header name is invalid: {err}")))?; - let value = HeaderValue::from_str(&header.value).map_err(|err| { - invalid_params(format!( - "http/request header value is invalid for {}: {err}", - header.name - )) - })?; - header_map.append(name, value); + fn build_headers(headers: Vec) -> Result { + let mut header_map = HeaderMap::new(); + for header in headers { + let name = HeaderName::from_bytes(header.name.as_bytes()).map_err(|err| { + invalid_params(format!("http/request header name is invalid: {err}")) + })?; + let value = HeaderValue::from_str(&header.value).map_err(|err| { + invalid_params(format!( + "http/request header value is invalid for {}: {err}", + header.name + )) + })?; + header_map.append(name, value); + } + Ok(header_map) } - Ok(header_map) -} -fn executor_response_headers(headers: &HeaderMap) -> Vec { - headers - .iter() - .filter_map(|(name, value)| { - Some(HttpHeader { - name: name.as_str().to_string(), - value: value.to_str().ok()?.to_string(), + fn response_headers(headers: &HeaderMap) -> Vec { + headers + .iter() + .filter_map(|(name, value)| { + Some(HttpHeader { + name: name.as_str().to_string(), + value: value.to_str().ok()?.to_string(), + }) }) - }) - .collect() -} + .collect() + } -pub(crate) async fn stream_executor_http_body( - pending_stream: ExecutorPendingHttpBodyStream, - notifications: RpcNotificationSender, -) { - let ExecutorPendingHttpBodyStream { - request_id, - response, - } = pending_stream; - let mut seq = 1; - let mut body = response.bytes_stream(); - while let Some(chunk) = body.next().await { - match chunk { - Ok(bytes) => { - if !send_executor_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id: request_id.clone(), - seq, - delta: bytes.to_vec().into(), - done: false, - error: None, - }, - ) - .await - { + async fn stream_body( + pending_stream: ExecutorPendingHttpBodyStream, + notifications: RpcNotificationSender, + ) { + let ExecutorPendingHttpBodyStream { + request_id, + response, + } = pending_stream; + let mut seq = 1; + let mut body = response.bytes_stream(); + while let Some(chunk) = body.next().await { + match chunk { + Ok(bytes) => { + if !send_executor_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id: request_id.clone(), + seq, + delta: bytes.to_vec().into(), + done: false, + error: None, + }, + ) + .await + { + return; + } + seq += 1; + } + Err(err) => { + let _ = send_executor_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: Some(err.to_string()), + }, + ) + .await; return; } - seq += 1; - } - Err(err) => { - let _ = send_executor_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id, - seq, - delta: Vec::new().into(), - done: true, - error: Some(err.to_string()), - }, - ) - .await; - return; } } - } - let _ = send_executor_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id, - seq, - delta: Vec::new().into(), - done: true, - error: None, - }, - ) - .await; -} - -async fn send_executor_body_delta( - notifications: &RpcNotificationSender, - delta: HttpRequestBodyDeltaNotification, -) -> bool { - notifications - .notify(HTTP_REQUEST_BODY_DELTA_METHOD, &delta) - .await - .is_ok() + let _ = send_executor_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: None, + }, + ) + .await; + } } From 61f15c95a4ea8993b5c1ac194cda27065e885abe Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 10:20:35 -0700 Subject: [PATCH 26/29] codex: drop thin executor http wrappers Co-authored-by: Codex --- .../exec-server/src/client/http_client.rs | 23 ++++--------------- codex-rs/exec-server/src/server/handler.rs | 9 ++++---- 2 files changed, 9 insertions(+), 23 deletions(-) diff --git a/codex-rs/exec-server/src/client/http_client.rs b/codex-rs/exec-server/src/client/http_client.rs index fabab7590326..10330ea85e24 100644 --- a/codex-rs/exec-server/src/client/http_client.rs +++ b/codex-rs/exec-server/src/client/http_client.rs @@ -38,7 +38,7 @@ pub(crate) struct ExecutorPendingHttpBodyStream { response: reqwest::Response, } -struct ExecutorHttpRequestRunner { +pub(crate) struct ExecutorHttpRequestRunner { client: reqwest::Client, } @@ -350,21 +350,6 @@ fn spawn_remove_http_body_stream(inner: Arc, request_id: String) { } } -pub(crate) async fn run_executor_http_request( - params: HttpRequestParams, -) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> { - ExecutorHttpRequestRunner::new(params.timeout_ms)? - .run(params) - .await -} - -pub(crate) async fn stream_executor_http_body( - pending_stream: ExecutorPendingHttpBodyStream, - notifications: RpcNotificationSender, -) { - ExecutorHttpRequestRunner::stream_body(pending_stream, notifications).await; -} - async fn send_executor_body_delta( notifications: &RpcNotificationSender, delta: HttpRequestBodyDeltaNotification, @@ -376,7 +361,7 @@ async fn send_executor_body_delta( } impl ExecutorHttpRequestRunner { - fn new(timeout_ms: Option) -> Result { + pub(crate) fn new(timeout_ms: Option) -> Result { let client = match timeout_ms { None => reqwest::Client::builder(), Some(timeout_ms) => { @@ -388,7 +373,7 @@ impl ExecutorHttpRequestRunner { Ok(Self { client }) } - async fn run( + pub(crate) async fn run( &self, params: HttpRequestParams, ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> @@ -476,7 +461,7 @@ impl ExecutorHttpRequestRunner { .collect() } - async fn stream_body( + pub(crate) async fn stream_body( pending_stream: ExecutorPendingHttpBodyStream, notifications: RpcNotificationSender, ) { diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index d99e9ff46397..f735d0170bb9 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -11,9 +11,8 @@ use tokio::sync::Mutex; use tokio::task::JoinHandle; use crate::ExecServerRuntimePaths; +use crate::client::http_client::ExecutorHttpRequestRunner; use crate::client::http_client::ExecutorPendingHttpBodyStream; -use crate::client::http_client::run_executor_http_request; -use crate::client::http_client::stream_executor_http_body; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; use crate::protocol::FsCopyParams; @@ -181,7 +180,9 @@ impl ExecServerHandler { if stream_response { self.reserve_http_body_stream(&http_request_id).await?; } - let mut response = run_executor_http_request(params).await; + let mut response = ExecutorHttpRequestRunner::new(params.timeout_ms)? + .run(params) + .await; if response.is_err() && stream_response { self.release_http_body_stream(&http_request_id).await; } @@ -314,7 +315,7 @@ impl ExecServerHandler { let handler = Arc::clone(self); let notifications = self.notifications.clone(); let task = tokio::spawn(async move { - stream_executor_http_body(pending_stream, notifications).await; + ExecutorHttpRequestRunner::stream_body(pending_stream, notifications).await; handler.release_http_body_stream(&finished_request_id).await; }); let mut body_streams = self.body_streams.lock().await; From 93f1a99cdf720d821ab69d5dcde306f771422ede Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 10:23:22 -0700 Subject: [PATCH 27/29] codex: reorder executor http client functions Co-authored-by: Codex --- .../exec-server/src/client/http_client.rs | 438 +++++++++--------- 1 file changed, 219 insertions(+), 219 deletions(-) diff --git a/codex-rs/exec-server/src/client/http_client.rs b/codex-rs/exec-server/src/client/http_client.rs index 10330ea85e24..1e91fa4489b2 100644 --- a/codex-rs/exec-server/src/client/http_client.rs +++ b/codex-rs/exec-server/src/client/http_client.rs @@ -57,6 +57,60 @@ pub struct HttpResponseBodyStream { closed: bool, } +impl ExecServerClient { + /// Performs an executor-side HTTP request and buffers the response body. + pub async fn http_request( + &self, + mut params: HttpRequestParams, + ) -> Result { + params.stream_response = false; + self.call(HTTP_REQUEST_METHOD, ¶ms).await + } + + /// Performs an executor-side HTTP request and returns a body stream. + /// + /// The method sets `stream_response` and replaces any caller-supplied + /// `request_id` with a connection-local id, so late deltas from abandoned + /// streams cannot be confused with later requests. + pub async fn http_request_stream( + &self, + mut params: HttpRequestParams, + ) -> Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError> { + params.stream_response = true; + let request_id = self.inner.next_http_body_stream_request_id(); + params.request_id = request_id.clone(); + let (tx, rx) = mpsc::channel(HTTP_BODY_DELTA_CHANNEL_CAPACITY); + self.inner + .insert_http_body_stream(request_id.clone(), tx) + .await?; + let mut registration = HttpBodyStreamRegistration { + inner: Arc::clone(&self.inner), + request_id: request_id.clone(), + active: true, + }; + let response = match self.call(HTTP_REQUEST_METHOD, ¶ms).await { + Ok(response) => response, + Err(error) => { + self.inner.remove_http_body_stream(&request_id).await; + registration.active = false; + return Err(error); + } + }; + registration.active = false; + Ok(( + response, + HttpResponseBodyStream { + inner: Arc::clone(&self.inner), + request_id, + next_seq: 1, + rx, + pending_eof: false, + closed: false, + }, + )) + } +} + impl HttpResponseBodyStream { /// Receives the next response-body chunk. /// @@ -131,73 +185,164 @@ impl Drop for HttpResponseBodyStream { } } -/// Active route registration owned while `http_request_stream` awaits headers. -struct HttpBodyStreamRegistration { - inner: Arc, - request_id: String, - active: bool, -} - -impl Drop for HttpBodyStreamRegistration { - /// Removes the route if the stream request future is cancelled before headers return. - fn drop(&mut self) { - if self.active { - spawn_remove_http_body_stream(Arc::clone(&self.inner), self.request_id.clone()); +impl ExecutorHttpRequestRunner { + pub(crate) fn new(timeout_ms: Option) -> Result { + let client = match timeout_ms { + None => reqwest::Client::builder(), + Some(timeout_ms) => { + reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)) + } } - } -} - -impl ExecServerClient { - /// Performs an executor-side HTTP request and buffers the response body. - pub async fn http_request( - &self, - mut params: HttpRequestParams, - ) -> Result { - params.stream_response = false; - self.call(HTTP_REQUEST_METHOD, ¶ms).await + .build() + .map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?; + Ok(Self { client }) } - /// Performs an executor-side HTTP request and returns a body stream. - /// - /// The method sets `stream_response` and replaces any caller-supplied - /// `request_id` with a connection-local id, so late deltas from abandoned - /// streams cannot be confused with later requests. - pub async fn http_request_stream( + pub(crate) async fn run( &self, - mut params: HttpRequestParams, - ) -> Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError> { - params.stream_response = true; - let request_id = self.inner.next_http_body_stream_request_id(); - params.request_id = request_id.clone(); - let (tx, rx) = mpsc::channel(HTTP_BODY_DELTA_CHANNEL_CAPACITY); - self.inner - .insert_http_body_stream(request_id.clone(), tx) - .await?; - let mut registration = HttpBodyStreamRegistration { - inner: Arc::clone(&self.inner), - request_id: request_id.clone(), - active: true, - }; - let response = match self.call(HTTP_REQUEST_METHOD, ¶ms).await { - Ok(response) => response, - Err(error) => { - self.inner.remove_http_body_stream(&request_id).await; - registration.active = false; - return Err(error); + params: HttpRequestParams, + ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> + { + let method = Method::from_bytes(params.method.as_bytes()) + .map_err(|err| invalid_params(format!("http/request method is invalid: {err}")))?; + let url = Url::parse(¶ms.url) + .map_err(|err| invalid_params(format!("http/request url is invalid: {err}")))?; + match url.scheme() { + "http" | "https" => {} + scheme => { + return Err(invalid_params(format!( + "http/request only supports http and https URLs, got {scheme}" + ))); } - }; - registration.active = false; + } + + let headers = Self::build_headers(params.headers)?; + let mut request = self.client.request(method, url).headers(headers); + if let Some(body) = params.body { + request = request.body(body.into_inner()); + } + + let response = request + .send() + .await + .map_err(|err| internal_error(format!("http/request failed: {err}")))?; + let status = response.status().as_u16(); + let headers = Self::response_headers(response.headers()); + + if params.stream_response { + return Ok(( + HttpRequestResponse { + status, + headers, + body: Vec::new().into(), + }, + Some(ExecutorPendingHttpBodyStream { + request_id: params.request_id, + response, + }), + )); + } + + let body = response.bytes().await.map_err(|err| { + internal_error(format!("failed to read http/request response body: {err}")) + })?; + Ok(( + HttpRequestResponse { + status, + headers, + body: body.to_vec().into(), + }, + None, + )) + } + + fn build_headers(headers: Vec) -> Result { + let mut header_map = HeaderMap::new(); + for header in headers { + let name = HeaderName::from_bytes(header.name.as_bytes()).map_err(|err| { + invalid_params(format!("http/request header name is invalid: {err}")) + })?; + let value = HeaderValue::from_str(&header.value).map_err(|err| { + invalid_params(format!( + "http/request header value is invalid for {}: {err}", + header.name + )) + })?; + header_map.append(name, value); + } + Ok(header_map) + } + + fn response_headers(headers: &HeaderMap) -> Vec { + headers + .iter() + .filter_map(|(name, value)| { + Some(HttpHeader { + name: name.as_str().to_string(), + value: value.to_str().ok()?.to_string(), + }) + }) + .collect() + } + + pub(crate) async fn stream_body( + pending_stream: ExecutorPendingHttpBodyStream, + notifications: RpcNotificationSender, + ) { + let ExecutorPendingHttpBodyStream { + request_id, response, - HttpResponseBodyStream { - inner: Arc::clone(&self.inner), + } = pending_stream; + let mut seq = 1; + let mut body = response.bytes_stream(); + while let Some(chunk) = body.next().await { + match chunk { + Ok(bytes) => { + if !send_executor_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id: request_id.clone(), + seq, + delta: bytes.to_vec().into(), + done: false, + error: None, + }, + ) + .await + { + return; + } + seq += 1; + } + Err(err) => { + let _ = send_executor_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: Some(err.to_string()), + }, + ) + .await; + return; + } + } + } + + let _ = send_executor_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { request_id, - next_seq: 1, - rx, - pending_eof: false, - closed: false, + seq, + delta: Vec::new().into(), + done: true, + error: None, }, - )) + ) + .await; } } @@ -341,6 +486,22 @@ impl Inner { } } +/// Active route registration owned while `http_request_stream` awaits headers. +struct HttpBodyStreamRegistration { + inner: Arc, + request_id: String, + active: bool, +} + +impl Drop for HttpBodyStreamRegistration { + /// Removes the route if the stream request future is cancelled before headers return. + fn drop(&mut self) { + if self.active { + spawn_remove_http_body_stream(Arc::clone(&self.inner), self.request_id.clone()); + } + } +} + /// Schedules HTTP body route removal from synchronous drop paths. fn spawn_remove_http_body_stream(inner: Arc, request_id: String) { if let Ok(handle) = Handle::try_current() { @@ -359,164 +520,3 @@ async fn send_executor_body_delta( .await .is_ok() } - -impl ExecutorHttpRequestRunner { - pub(crate) fn new(timeout_ms: Option) -> Result { - let client = match timeout_ms { - None => reqwest::Client::builder(), - Some(timeout_ms) => { - reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)) - } - } - .build() - .map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?; - Ok(Self { client }) - } - - pub(crate) async fn run( - &self, - params: HttpRequestParams, - ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> - { - let method = Method::from_bytes(params.method.as_bytes()) - .map_err(|err| invalid_params(format!("http/request method is invalid: {err}")))?; - let url = Url::parse(¶ms.url) - .map_err(|err| invalid_params(format!("http/request url is invalid: {err}")))?; - match url.scheme() { - "http" | "https" => {} - scheme => { - return Err(invalid_params(format!( - "http/request only supports http and https URLs, got {scheme}" - ))); - } - } - - let headers = Self::build_headers(params.headers)?; - let mut request = self.client.request(method, url).headers(headers); - if let Some(body) = params.body { - request = request.body(body.into_inner()); - } - - let response = request - .send() - .await - .map_err(|err| internal_error(format!("http/request failed: {err}")))?; - let status = response.status().as_u16(); - let headers = Self::response_headers(response.headers()); - - if params.stream_response { - return Ok(( - HttpRequestResponse { - status, - headers, - body: Vec::new().into(), - }, - Some(ExecutorPendingHttpBodyStream { - request_id: params.request_id, - response, - }), - )); - } - - let body = response.bytes().await.map_err(|err| { - internal_error(format!("failed to read http/request response body: {err}")) - })?; - - Ok(( - HttpRequestResponse { - status, - headers, - body: body.to_vec().into(), - }, - None, - )) - } - - fn build_headers(headers: Vec) -> Result { - let mut header_map = HeaderMap::new(); - for header in headers { - let name = HeaderName::from_bytes(header.name.as_bytes()).map_err(|err| { - invalid_params(format!("http/request header name is invalid: {err}")) - })?; - let value = HeaderValue::from_str(&header.value).map_err(|err| { - invalid_params(format!( - "http/request header value is invalid for {}: {err}", - header.name - )) - })?; - header_map.append(name, value); - } - Ok(header_map) - } - - fn response_headers(headers: &HeaderMap) -> Vec { - headers - .iter() - .filter_map(|(name, value)| { - Some(HttpHeader { - name: name.as_str().to_string(), - value: value.to_str().ok()?.to_string(), - }) - }) - .collect() - } - - pub(crate) async fn stream_body( - pending_stream: ExecutorPendingHttpBodyStream, - notifications: RpcNotificationSender, - ) { - let ExecutorPendingHttpBodyStream { - request_id, - response, - } = pending_stream; - let mut seq = 1; - let mut body = response.bytes_stream(); - while let Some(chunk) = body.next().await { - match chunk { - Ok(bytes) => { - if !send_executor_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id: request_id.clone(), - seq, - delta: bytes.to_vec().into(), - done: false, - error: None, - }, - ) - .await - { - return; - } - seq += 1; - } - Err(err) => { - let _ = send_executor_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id, - seq, - delta: Vec::new().into(), - done: true, - error: Some(err.to_string()), - }, - ) - .await; - return; - } - } - } - - let _ = send_executor_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id, - seq, - delta: Vec::new().into(), - done: true, - error: None, - }, - ) - .await; - } -} From 1febfa73690f033bfe6f5e25813679cfecb28eed Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 10:56:56 -0700 Subject: [PATCH 28/29] codex: fix exec-server clippy cleanup Co-authored-by: Codex --- codex-rs/exec-server/src/server/handler.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index f735d0170bb9..af89b3e09cac 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -180,7 +180,7 @@ impl ExecServerHandler { if stream_response { self.reserve_http_body_stream(&http_request_id).await?; } - let mut response = ExecutorHttpRequestRunner::new(params.timeout_ms)? + let response = ExecutorHttpRequestRunner::new(params.timeout_ms)? .run(params) .await; if response.is_err() && stream_response { From cb86b7a6bc17f526ee6c440c446299aac754ed84 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 12:44:25 -0700 Subject: [PATCH 29/29] codex: use task tracker for exec-server background tasks --- codex-rs/Cargo.lock | 1 + codex-rs/exec-server/Cargo.toml | 1 + codex-rs/exec-server/src/server/handler.rs | 54 +++++++++++----------- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 684d2be4ed1d..5ad6f9ff16c1 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2175,6 +2175,7 @@ dependencies = [ "thiserror 2.0.18", "tokio", "tokio-tungstenite", + "tokio-util", "tracing", "uuid", ] diff --git a/codex-rs/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index e131a2cd4810..96880632785c 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -36,6 +36,7 @@ tokio = { workspace = true, features = [ "sync", "time", ] } +tokio-util = { workspace = true, features = ["rt"] } tokio-tungstenite = { workspace = true } tracing = { workspace = true } uuid = { workspace = true, features = ["v4"] } diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index af89b3e09cac..15035335ea18 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -6,9 +6,10 @@ use std::sync::atomic::Ordering; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::RequestId; use serde_json::to_value; -use std::collections::HashMap; +use std::collections::HashSet; use tokio::sync::Mutex; -use tokio::task::JoinHandle; +use tokio_util::sync::CancellationToken; +use tokio_util::task::TaskTracker; use crate::ExecServerRuntimePaths; use crate::client::http_client::ExecutorHttpRequestRunner; @@ -50,7 +51,9 @@ pub(crate) struct ExecServerHandler { session_registry: Arc, notifications: RpcNotificationSender, session: StdMutex>, - body_streams: Mutex>>>, + active_body_stream_ids: Mutex>, + background_task_shutdown: CancellationToken, + background_tasks: TaskTracker, file_system: FileSystemHandler, initialize_requested: AtomicBool, initialized: AtomicBool, @@ -66,7 +69,9 @@ impl ExecServerHandler { session_registry, notifications, session: StdMutex::new(None), - body_streams: Mutex::new(HashMap::new()), + active_body_stream_ids: Mutex::new(HashSet::new()), + background_task_shutdown: CancellationToken::new(), + background_tasks: TaskTracker::new(), file_system: FileSystemHandler::new(runtime_paths), initialize_requested: AtomicBool::new(false), initialized: AtomicBool::new(false), @@ -74,16 +79,9 @@ impl ExecServerHandler { } pub(crate) async fn shutdown(&self) { - let tasks = { - let mut body_streams = self.body_streams.lock().await; - body_streams - .drain() - .filter_map(|(_, task)| task) - .collect::>() - }; - for task in tasks { - task.abort(); - } + self.background_task_shutdown.cancel(); + self.background_tasks.close(); + self.background_tasks.wait().await; if let Some(session) = self.session() { session.detach().await; } @@ -311,34 +309,36 @@ impl ExecServerHandler { pending_stream: ExecutorPendingHttpBodyStream, ) { let request_id = pending_stream.request_id.clone(); + if self.background_task_shutdown.is_cancelled() { + self.release_http_body_stream(&request_id).await; + return; + } let finished_request_id = request_id.clone(); let handler = Arc::clone(self); let notifications = self.notifications.clone(); - let task = tokio::spawn(async move { - ExecutorHttpRequestRunner::stream_body(pending_stream, notifications).await; + let shutdown = self.background_task_shutdown.clone(); + self.background_tasks.spawn(async move { + tokio::select! { + _ = shutdown.cancelled() => {} + _ = ExecutorHttpRequestRunner::stream_body(pending_stream, notifications) => {} + } handler.release_http_body_stream(&finished_request_id).await; }); - let mut body_streams = self.body_streams.lock().await; - if let Some(entry) = body_streams.get_mut(&request_id) { - *entry = Some(task); - } else { - task.abort(); - } } async fn release_http_body_stream(&self, request_id: &str) { - let mut body_streams = self.body_streams.lock().await; - body_streams.remove(request_id); + let mut active_body_stream_ids = self.active_body_stream_ids.lock().await; + active_body_stream_ids.remove(request_id); } async fn reserve_http_body_stream(&self, request_id: &str) -> Result<(), JSONRPCErrorError> { - let mut body_streams = self.body_streams.lock().await; - if body_streams.contains_key(request_id) { + let mut active_body_stream_ids = self.active_body_stream_ids.lock().await; + if active_body_stream_ids.contains(request_id) { return Err(invalid_params(format!( "http/request streamResponse requestId `{request_id}` is already active" ))); } - body_streams.insert(request_id.to_string(), None); + active_body_stream_ids.insert(request_id.to_string()); Ok(()) } }