mirror of
https://github.com/openai/codex.git
synced 2026-05-17 09:43:19 +00:00
Increase exec-server environment transport timeouts (#21825)
## Why The environment-backed exec-server transport currently hardcodes 5 second connect and initialize timeouts in `client_transport.rs`. That is short for SSH-backed stdio environments and remote websocket environments, and there is currently no way to raise those values from `CODEX_HOME/environments.toml`. This stacked follow-up raises the default environment transport timeouts and lets each configured environment override them in `environments.toml`. ## What Changed - raise the default environment transport connect and initialize timeouts from 5s to 10s - store concrete timeout values on `ExecServerTransportParams` instead of hardcoding them in `connect_for_transport(...)` - add `connect_timeout_sec` and `initialize_timeout_sec` to `[[environments]]` entries in `environments.toml` - apply parse-time defaults so runtime transport code receives fully resolved timeout values - reject `connect_timeout_sec` on stdio environments because it only applies to websocket transports - extend parser tests to cover the new fields and defaults ## Stack - base: https://github.com/openai/codex/pull/21794 - this PR: configurable environment transport timeouts ## Validation - `cd /Users/starr/code/codex-worktrees/exec-env-timeouts-config-20260508/codex-rs && just fmt` - not run: tests --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
@@ -895,6 +895,8 @@ mod tests {
|
||||
use super::ExecServerClientConnectOptions;
|
||||
use crate::ProcessId;
|
||||
#[cfg(not(windows))]
|
||||
use crate::client_api::DEFAULT_REMOTE_EXEC_SERVER_INITIALIZE_TIMEOUT;
|
||||
#[cfg(not(windows))]
|
||||
use crate::client_api::ExecServerTransportParams;
|
||||
use crate::client_api::StdioExecServerCommand;
|
||||
use crate::client_api::StdioExecServerConnectArgs;
|
||||
@@ -962,15 +964,18 @@ mod tests {
|
||||
#[tokio::test]
|
||||
async fn connect_for_transport_initializes_stdio_command() {
|
||||
let client = ExecServerClient::connect_for_transport(
|
||||
ExecServerTransportParams::StdioCommand(StdioExecServerCommand {
|
||||
program: "sh".to_string(),
|
||||
args: vec![
|
||||
"-c".to_string(),
|
||||
"read _line; printf '%s\\n' '{\"id\":1,\"result\":{\"sessionId\":\"stdio-test\"}}'; read _line; sleep 60".to_string(),
|
||||
],
|
||||
env: HashMap::new(),
|
||||
cwd: None,
|
||||
}),
|
||||
ExecServerTransportParams::StdioCommand {
|
||||
command: StdioExecServerCommand {
|
||||
program: "sh".to_string(),
|
||||
args: vec![
|
||||
"-c".to_string(),
|
||||
"read _line; printf '%s\\n' '{\"id\":1,\"result\":{\"sessionId\":\"stdio-test\"}}'; read _line; sleep 60".to_string(),
|
||||
],
|
||||
env: HashMap::new(),
|
||||
cwd: None,
|
||||
},
|
||||
initialize_timeout: DEFAULT_REMOTE_EXEC_SERVER_INITIALIZE_TIMEOUT,
|
||||
},
|
||||
)
|
||||
.await
|
||||
.expect("stdio transport should connect");
|
||||
|
||||
@@ -9,6 +9,9 @@ use crate::HttpRequestParams;
|
||||
use crate::HttpRequestResponse;
|
||||
use crate::HttpResponseBodyStream;
|
||||
|
||||
pub(crate) const DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT: Duration = Duration::from_secs(10);
|
||||
pub(crate) const DEFAULT_REMOTE_EXEC_SERVER_INITIALIZE_TIMEOUT: Duration = Duration::from_secs(10);
|
||||
|
||||
/// Connection options for any exec-server client transport.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct ExecServerClientConnectOptions {
|
||||
@@ -48,9 +51,26 @@ pub(crate) struct StdioExecServerCommand {
|
||||
/// Parameters used to connect to a remote exec-server environment.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub(crate) enum ExecServerTransportParams {
|
||||
WebSocketUrl(String),
|
||||
WebSocketUrl {
|
||||
websocket_url: String,
|
||||
connect_timeout: Duration,
|
||||
initialize_timeout: Duration,
|
||||
},
|
||||
#[allow(dead_code)]
|
||||
StdioCommand(StdioExecServerCommand),
|
||||
StdioCommand {
|
||||
command: StdioExecServerCommand,
|
||||
initialize_timeout: Duration,
|
||||
},
|
||||
}
|
||||
|
||||
impl ExecServerTransportParams {
|
||||
pub(crate) fn websocket_url(websocket_url: String) -> Self {
|
||||
Self::WebSocketUrl {
|
||||
websocket_url,
|
||||
connect_timeout: DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT,
|
||||
initialize_timeout: DEFAULT_REMOTE_EXEC_SERVER_INITIALIZE_TIMEOUT,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Sends HTTP requests through a runtime-selected transport.
|
||||
|
||||
@@ -1,6 +1,4 @@
|
||||
use std::process::Stdio;
|
||||
use std::time::Duration;
|
||||
|
||||
use tokio::io::AsyncBufReadExt;
|
||||
use tokio::io::BufReader;
|
||||
use tokio::process::Command;
|
||||
@@ -17,29 +15,34 @@ use crate::client_api::StdioExecServerConnectArgs;
|
||||
use crate::connection::JsonRpcConnection;
|
||||
|
||||
const ENVIRONMENT_CLIENT_NAME: &str = "codex-environment";
|
||||
const ENVIRONMENT_CONNECT_TIMEOUT: Duration = Duration::from_secs(5);
|
||||
const ENVIRONMENT_INITIALIZE_TIMEOUT: Duration = Duration::from_secs(5);
|
||||
|
||||
impl ExecServerClient {
|
||||
pub(crate) async fn connect_for_transport(
|
||||
transport_params: crate::client_api::ExecServerTransportParams,
|
||||
) -> Result<Self, ExecServerError> {
|
||||
match transport_params {
|
||||
crate::client_api::ExecServerTransportParams::WebSocketUrl(websocket_url) => {
|
||||
crate::client_api::ExecServerTransportParams::WebSocketUrl {
|
||||
websocket_url,
|
||||
connect_timeout,
|
||||
initialize_timeout,
|
||||
} => {
|
||||
Self::connect_websocket(RemoteExecServerConnectArgs {
|
||||
websocket_url,
|
||||
client_name: ENVIRONMENT_CLIENT_NAME.to_string(),
|
||||
connect_timeout: ENVIRONMENT_CONNECT_TIMEOUT,
|
||||
initialize_timeout: ENVIRONMENT_INITIALIZE_TIMEOUT,
|
||||
connect_timeout,
|
||||
initialize_timeout,
|
||||
resume_session_id: None,
|
||||
})
|
||||
.await
|
||||
}
|
||||
crate::client_api::ExecServerTransportParams::StdioCommand(command) => {
|
||||
crate::client_api::ExecServerTransportParams::StdioCommand {
|
||||
command,
|
||||
initialize_timeout,
|
||||
} => {
|
||||
Self::connect_stdio_command(StdioExecServerConnectArgs {
|
||||
command,
|
||||
client_name: ENVIRONMENT_CLIENT_NAME.to_string(),
|
||||
initialize_timeout: ENVIRONMENT_INITIALIZE_TIMEOUT,
|
||||
initialize_timeout,
|
||||
resume_session_id: None,
|
||||
})
|
||||
.await
|
||||
|
||||
@@ -373,7 +373,7 @@ impl Environment {
|
||||
local_runtime_paths: Option<ExecServerRuntimePaths>,
|
||||
) -> Self {
|
||||
Self::remote_with_transport(
|
||||
ExecServerTransportParams::WebSocketUrl(exec_server_url),
|
||||
ExecServerTransportParams::websocket_url(exec_server_url),
|
||||
local_runtime_paths,
|
||||
)
|
||||
}
|
||||
@@ -383,10 +383,11 @@ impl Environment {
|
||||
local_runtime_paths: Option<ExecServerRuntimePaths>,
|
||||
) -> Self {
|
||||
let exec_server_url = match &remote_transport {
|
||||
ExecServerTransportParams::WebSocketUrl(exec_server_url) => {
|
||||
Some(exec_server_url.clone())
|
||||
}
|
||||
ExecServerTransportParams::StdioCommand(_) => None,
|
||||
ExecServerTransportParams::WebSocketUrl {
|
||||
websocket_url: exec_server_url,
|
||||
..
|
||||
} => Some(exec_server_url.clone()),
|
||||
ExecServerTransportParams::StdioCommand { .. } => None,
|
||||
};
|
||||
let client = LazyRemoteExecServerClient::new(remote_transport.clone());
|
||||
let exec_backend: Arc<dyn ExecBackend> = Arc::new(RemoteProcess::new(client.clone()));
|
||||
|
||||
@@ -2,6 +2,7 @@ use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::time::Duration;
|
||||
|
||||
use async_trait::async_trait;
|
||||
use serde::Deserialize;
|
||||
@@ -11,6 +12,8 @@ use crate::DefaultEnvironmentProvider;
|
||||
use crate::Environment;
|
||||
use crate::EnvironmentProvider;
|
||||
use crate::ExecServerError;
|
||||
use crate::client_api::DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT;
|
||||
use crate::client_api::DEFAULT_REMOTE_EXEC_SERVER_INITIALIZE_TIMEOUT;
|
||||
use crate::client_api::ExecServerTransportParams;
|
||||
use crate::client_api::StdioExecServerCommand;
|
||||
use crate::environment::LOCAL_ENVIRONMENT_ID;
|
||||
@@ -38,6 +41,10 @@ struct EnvironmentToml {
|
||||
args: Option<Vec<String>>,
|
||||
env: Option<HashMap<String, String>>,
|
||||
cwd: Option<PathBuf>,
|
||||
#[serde(default, with = "option_duration_secs")]
|
||||
connect_timeout_sec: Option<Duration>,
|
||||
#[serde(default, with = "option_duration_secs")]
|
||||
initialize_timeout_sec: Option<Duration>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
@@ -108,6 +115,8 @@ fn parse_environment_toml(
|
||||
args,
|
||||
env,
|
||||
cwd,
|
||||
connect_timeout_sec,
|
||||
initialize_timeout_sec,
|
||||
} = item;
|
||||
validate_environment_id(&id)?;
|
||||
if program.is_none() && (args.is_some() || env.is_some() || cwd.is_some()) {
|
||||
@@ -115,11 +124,24 @@ fn parse_environment_toml(
|
||||
"environment `{id}` args, env, and cwd require program"
|
||||
)));
|
||||
}
|
||||
if url.is_none() && connect_timeout_sec.is_some() {
|
||||
return Err(ExecServerError::Protocol(format!(
|
||||
"environment `{id}` connect_timeout_sec requires url"
|
||||
)));
|
||||
}
|
||||
|
||||
let connect_timeout = connect_timeout_sec.unwrap_or(DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT);
|
||||
let initialize_timeout =
|
||||
initialize_timeout_sec.unwrap_or(DEFAULT_REMOTE_EXEC_SERVER_INITIALIZE_TIMEOUT);
|
||||
|
||||
let transport_params = match (url, program) {
|
||||
(Some(url), None) => {
|
||||
let url = validate_websocket_url(url)?;
|
||||
ExecServerTransportParams::WebSocketUrl(url)
|
||||
ExecServerTransportParams::WebSocketUrl {
|
||||
websocket_url: url,
|
||||
connect_timeout,
|
||||
initialize_timeout,
|
||||
}
|
||||
}
|
||||
(None, Some(program)) => {
|
||||
let program = program.trim().to_string();
|
||||
@@ -129,12 +151,15 @@ fn parse_environment_toml(
|
||||
)));
|
||||
}
|
||||
let cwd = normalize_stdio_cwd(&id, cwd, config_dir)?;
|
||||
ExecServerTransportParams::StdioCommand(StdioExecServerCommand {
|
||||
program,
|
||||
args: args.unwrap_or_default(),
|
||||
env: env.unwrap_or_default(),
|
||||
cwd,
|
||||
})
|
||||
ExecServerTransportParams::StdioCommand {
|
||||
command: StdioExecServerCommand {
|
||||
program,
|
||||
args: args.unwrap_or_default(),
|
||||
env: env.unwrap_or_default(),
|
||||
cwd,
|
||||
},
|
||||
initialize_timeout,
|
||||
}
|
||||
}
|
||||
(None, None) | (Some(_), Some(_)) => {
|
||||
return Err(ExecServerError::Protocol(format!(
|
||||
@@ -278,6 +303,22 @@ fn load_environments_toml(path: &Path) -> Result<EnvironmentsToml, ExecServerErr
|
||||
})
|
||||
}
|
||||
|
||||
mod option_duration_secs {
|
||||
use std::time::Duration;
|
||||
|
||||
use serde::Deserialize;
|
||||
use serde::Deserializer;
|
||||
|
||||
pub fn deserialize<'de, D>(deserializer: D) -> Result<Option<Duration>, D::Error>
|
||||
where
|
||||
D: Deserializer<'de>,
|
||||
{
|
||||
let secs = Option::<f64>::deserialize(deserializer)?;
|
||||
secs.map(|secs| Duration::try_from_secs_f64(secs).map_err(serde::de::Error::custom))
|
||||
.transpose()
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -424,6 +465,15 @@ mod tests {
|
||||
},
|
||||
"environment `devbox` args, env, and cwd require program",
|
||||
),
|
||||
(
|
||||
EnvironmentToml {
|
||||
id: "ssh-dev".to_string(),
|
||||
program: Some("ssh".to_string()),
|
||||
connect_timeout_sec: Some(Duration::from_secs(1)),
|
||||
..Default::default()
|
||||
},
|
||||
"environment `ssh-dev` connect_timeout_sec requires url",
|
||||
),
|
||||
];
|
||||
|
||||
for (item, expected) in cases {
|
||||
@@ -459,12 +509,59 @@ mod tests {
|
||||
|
||||
assert_eq!(
|
||||
provider.environments[0].1,
|
||||
ExecServerTransportParams::StdioCommand(StdioExecServerCommand {
|
||||
program: "ssh".to_string(),
|
||||
args: Vec::new(),
|
||||
env: HashMap::new(),
|
||||
cwd: Some(config_dir.path().join("workspace")),
|
||||
})
|
||||
ExecServerTransportParams::StdioCommand {
|
||||
command: StdioExecServerCommand {
|
||||
program: "ssh".to_string(),
|
||||
args: Vec::new(),
|
||||
env: HashMap::new(),
|
||||
cwd: Some(config_dir.path().join("workspace")),
|
||||
},
|
||||
initialize_timeout: DEFAULT_REMOTE_EXEC_SERVER_INITIALIZE_TIMEOUT,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn toml_provider_parses_configured_transport_timeouts() {
|
||||
let provider = TomlEnvironmentProvider::new(EnvironmentsToml {
|
||||
default: None,
|
||||
environments: vec![
|
||||
EnvironmentToml {
|
||||
id: "devbox".to_string(),
|
||||
url: Some("ws://127.0.0.1:8765".to_string()),
|
||||
connect_timeout_sec: Some(Duration::from_secs(12)),
|
||||
initialize_timeout_sec: Some(Duration::from_secs(34)),
|
||||
..Default::default()
|
||||
},
|
||||
EnvironmentToml {
|
||||
id: "ssh-dev".to_string(),
|
||||
program: Some("ssh".to_string()),
|
||||
initialize_timeout_sec: Some(Duration::from_secs(56)),
|
||||
..Default::default()
|
||||
},
|
||||
],
|
||||
})
|
||||
.expect("provider");
|
||||
|
||||
assert_eq!(
|
||||
provider.environments[0].1,
|
||||
ExecServerTransportParams::WebSocketUrl {
|
||||
websocket_url: "ws://127.0.0.1:8765".to_string(),
|
||||
connect_timeout: Duration::from_secs(12),
|
||||
initialize_timeout: Duration::from_secs(34),
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
provider.environments[1].1,
|
||||
ExecServerTransportParams::StdioCommand {
|
||||
command: StdioExecServerCommand {
|
||||
program: "ssh".to_string(),
|
||||
args: Vec::new(),
|
||||
env: HashMap::new(),
|
||||
cwd: None,
|
||||
},
|
||||
initialize_timeout: Duration::from_secs(56),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
@@ -559,6 +656,8 @@ default = "ssh-dev"
|
||||
[[environments]]
|
||||
id = "devbox"
|
||||
url = "ws://127.0.0.1:4512"
|
||||
connect_timeout_sec = 12.0
|
||||
initialize_timeout_sec = 34.0
|
||||
|
||||
[[environments]]
|
||||
id = "ssh-dev"
|
||||
@@ -575,7 +674,16 @@ CODEX_LOG = "debug"
|
||||
|
||||
assert_eq!(environments.default.as_deref(), Some("ssh-dev"));
|
||||
assert_eq!(environments.environments.len(), 2);
|
||||
assert_eq!(environments.environments[0].id, "devbox");
|
||||
assert_eq!(
|
||||
environments.environments[0],
|
||||
EnvironmentToml {
|
||||
id: "devbox".to_string(),
|
||||
url: Some("ws://127.0.0.1:4512".to_string()),
|
||||
connect_timeout_sec: Some(Duration::from_secs(12)),
|
||||
initialize_timeout_sec: Some(Duration::from_secs(34)),
|
||||
..Default::default()
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
environments.environments[1],
|
||||
EnvironmentToml {
|
||||
|
||||
Reference in New Issue
Block a user