openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-29 19:08:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

Address exec-server config review feedback

Browse Source
This commit is contained in:
starr-openai
2026-05-28 23:33:59 -07:00
parent f8889460f9
commit 7a99c803de
5 changed files with 234 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/cli/src/main.rs
+87 -13
View File
@@ -1541,7 +1541,7 @@ async fn run_exec_server_command(
remote.reconnect_initial_backoff,
remote.reconnect_max_backoff,
)?
.with_runtime(runtime);
.with_runtime(runtime)?;
if let Some(name) = remote.name {
remote_config.name = name;
}
@@ -1565,7 +1565,7 @@ async fn run_exec_server_command(
}
fn validate_exec_server_remote_only_flags(
remote: Option<&codex_exec_server::RemoteExecServerConfig>,
remote: Option<&ResolvedRemoteExecServerConfig>,
use_agent_identity_auth_requested: bool,
) -> anyhow::Result<()> {
if remote.is_none() && use_agent_identity_auth_requested {
@@ -1575,17 +1575,29 @@ fn validate_exec_server_remote_only_flags(
Ok(())
}
#[derive(Debug, PartialEq, Eq)]
struct ResolvedRemoteExecServerConfig {
url: String,
environment_id: String,
name: Option<String>,
use_agent_identity_auth: bool,
reconnect_initial_backoff: Duration,
reconnect_max_backoff: Duration,
}
fn resolve_remote_exec_server_config(
url: Option<String>,
environment_id: Option<String>,
name: Option<String>,
use_agent_identity_auth: bool,
configured_remote: Option<codex_exec_server::RemoteExecServerConfig>,
) -> anyhow::Result<codex_exec_server::RemoteExecServerConfig> {
let configured_url = configured_remote.as_ref().map(|remote| remote.url.clone());
) -> anyhow::Result<ResolvedRemoteExecServerConfig> {
let configured_url = configured_remote
.as_ref()
.and_then(|remote| remote.url.clone());
let configured_environment_id = configured_remote
.as_ref()
.map(|remote| remote.environment_id.clone());
.and_then(|remote| remote.environment_id.clone());
let configured_name = configured_remote
.as_ref()
.and_then(|remote| remote.name.clone());
@@ -1602,7 +1614,7 @@ fn resolve_remote_exec_server_config(
.map_or(Duration::from_secs(30), |remote| {
remote.reconnect_max_backoff
});
Ok(codex_exec_server::RemoteExecServerConfig {
Ok(ResolvedRemoteExecServerConfig {
url: url
.or(configured_url)
.ok_or_else(|| anyhow::anyhow!("remote exec-server URL is required"))?,
@@ -2328,8 +2340,8 @@ mod tests {
Some("cli-name".to_string()),
true,
Some(codex_exec_server::RemoteExecServerConfig {
url: "https://file.example".to_string(),
environment_id: "file-env".to_string(),
url: Some("https://file.example".to_string()),
environment_id: Some("file-env".to_string()),
name: Some("file-name".to_string()),
use_agent_identity_auth: false,
reconnect_initial_backoff: Duration::from_secs(2),
@@ -2340,7 +2352,7 @@ mod tests {
assert_eq!(
config,
codex_exec_server::RemoteExecServerConfig {
ResolvedRemoteExecServerConfig {
url: "https://cli.example".to_string(),
environment_id: "cli-env".to_string(),
name: Some("cli-name".to_string()),
@@ -2359,8 +2371,8 @@ mod tests {
None,
false,
Some(codex_exec_server::RemoteExecServerConfig {
url: "https://file.example".to_string(),
environment_id: "file-env".to_string(),
url: Some("https://file.example".to_string()),
environment_id: Some("file-env".to_string()),
name: Some("file-name".to_string()),
use_agent_identity_auth: true,
reconnect_initial_backoff: Duration::from_secs(2),
@@ -2371,7 +2383,7 @@ mod tests {
assert_eq!(
config,
codex_exec_server::RemoteExecServerConfig {
ResolvedRemoteExecServerConfig {
url: "https://file.example".to_string(),
environment_id: "file-env".to_string(),
name: Some("file-name".to_string()),
@@ -2382,6 +2394,68 @@ mod tests {
);
}
#[test]
fn exec_server_cli_remote_url_fills_partial_file_remote() {
let config = resolve_remote_exec_server_config(
Some("https://cli.example".to_string()),
None,
None,
false,
Some(codex_exec_server::RemoteExecServerConfig {
url: None,
environment_id: Some("file-env".to_string()),
name: None,
use_agent_identity_auth: false,
reconnect_initial_backoff: Duration::from_secs(1),
reconnect_max_backoff: Duration::from_secs(30),
}),
)
.expect("remote config");
assert_eq!(
config,
ResolvedRemoteExecServerConfig {
url: "https://cli.example".to_string(),
environment_id: "file-env".to_string(),
name: None,
use_agent_identity_auth: false,
reconnect_initial_backoff: Duration::from_secs(1),
reconnect_max_backoff: Duration::from_secs(30),
}
);
}
#[test]
fn exec_server_cli_environment_id_fills_partial_file_remote() {
let config = resolve_remote_exec_server_config(
None,
Some("cli-env".to_string()),
None,
false,
Some(codex_exec_server::RemoteExecServerConfig {
url: Some("https://file.example".to_string()),
environment_id: None,
name: None,
use_agent_identity_auth: false,
reconnect_initial_backoff: Duration::from_secs(1),
reconnect_max_backoff: Duration::from_secs(30),
}),
)
.expect("remote config");
assert_eq!(
config,
ResolvedRemoteExecServerConfig {
url: "https://file.example".to_string(),
environment_id: "cli-env".to_string(),
name: None,
use_agent_identity_auth: false,
reconnect_initial_backoff: Duration::from_secs(1),
reconnect_max_backoff: Duration::from_secs(30),
}
);
}
#[test]
fn exec_server_agent_identity_auth_requires_remote_mode() {
let err = validate_exec_server_remote_only_flags(None, true).expect_err("missing remote");
@@ -2394,7 +2468,7 @@ mod tests {
#[test]
fn exec_server_agent_identity_auth_accepts_file_remote_mode() {
let remote = codex_exec_server::RemoteExecServerConfig {
let remote = ResolvedRemoteExecServerConfig {
url: "https://file.example".to_string(),
environment_id: "file-env".to_string(),
name: Some("file-name".to_string()),
codex-rs/exec-server/src/config.rs
+99 -9
View File
@@ -45,8 +45,8 @@ impl ExecServerConfig {
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct RemoteExecServerConfig {
pub url: String,
pub environment_id: String,
pub url: Option<String>,
pub environment_id: Option<String>,
pub name: Option<String>,
pub use_agent_identity_auth: bool,
pub reconnect_initial_backoff: Duration,
@@ -60,6 +60,12 @@ pub struct ExecServerRuntimeConfig {
pub filesystem: LocalFileSystemConfig,
}
impl ExecServerRuntimeConfig {
pub(crate) fn validate(&self) -> Result<(), ExecServerError> {
self.sessions.validate()
}
}
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct SessionRegistryConfig {
pub detached_session_ttl: Duration,
@@ -73,6 +79,12 @@ impl Default for SessionRegistryConfig {
}
}
impl SessionRegistryConfig {
fn validate(&self) -> Result<(), ExecServerError> {
validate_detached_session_ttl(self.detached_session_ttl)
}
}
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct LocalProcessConfig {
pub retained_output_bytes_per_process: usize,
@@ -134,8 +146,8 @@ struct ExecServerToml {
#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
struct RemoteExecServerToml {
url: String,
environment_id: String,
url: Option<String>,
environment_id: Option<String>,
name: Option<String>,
use_agent_identity_auth: Option<bool>,
reconnect_initial_backoff_sec: Option<u64>,
@@ -182,7 +194,9 @@ impl TryFrom<ExecServerToml> for ExecServerConfig {
if let Some(sessions) = sessions
&& let Some(detached_ttl_sec) = sessions.detached_ttl_sec
{
runtime.sessions.detached_session_ttl = Duration::from_secs(detached_ttl_sec);
let detached_session_ttl = Duration::from_secs(detached_ttl_sec);
validate_detached_session_ttl(detached_session_ttl)?;
runtime.sessions.detached_session_ttl = detached_session_ttl;
}
if let Some(processes) = processes {
if let Some(retained_output_bytes_per_process) =
@@ -202,6 +216,8 @@ impl TryFrom<ExecServerToml> for ExecServerConfig {
runtime.filesystem.max_read_file_bytes = max_read_file_bytes;
}
runtime.validate()?;
Ok(Self {
listen,
remote: remote.map(TryInto::try_into).transpose()?,
@@ -254,6 +270,18 @@ pub(crate) fn validate_reconnect_backoff(
Ok(())
}
fn validate_detached_session_ttl(detached_session_ttl: Duration) -> Result<(), ExecServerError> {
if std::time::Instant::now()
.checked_add(detached_session_ttl)
.is_none()
{
return Err(ExecServerError::Protocol(
"session detached TTL is too large".to_string(),
));
}
Ok(())
}
#[cfg(test)]
mod tests {
use pretty_assertions::assert_eq;
@@ -304,8 +332,8 @@ max_read_file_bytes = 14
ExecServerConfig {
listen: None,
remote: Some(RemoteExecServerConfig {
url: "https://chatgpt.com".to_string(),
environment_id: "devbox-123".to_string(),
url: Some("https://chatgpt.com".to_string()),
environment_id: Some("devbox-123".to_string()),
name: Some("remote-name".to_string()),
use_agent_identity_auth: true,
reconnect_initial_backoff: Duration::from_secs(2),
@@ -327,6 +355,34 @@ max_read_file_bytes = 14
);
}
#[test]
fn file_loads_partial_remote_settings() {
let codex_home = tempdir().expect("tempdir");
std::fs::write(
codex_home.path().join(EXEC_SERVER_TOML_FILE),
r#"
[remote]
environment_id = "devbox-123"
name = "remote-name"
"#,
)
.expect("write config");
let config = ExecServerConfig::from_codex_home(codex_home.path()).expect("config");
assert_eq!(
config.remote,
Some(RemoteExecServerConfig {
url: None,
environment_id: Some("devbox-123".to_string()),
name: Some("remote-name".to_string()),
use_agent_identity_auth: false,
reconnect_initial_backoff: Duration::from_secs(1),
reconnect_max_backoff: Duration::from_secs(30),
})
);
}
#[test]
fn file_rejects_listen_with_remote() {
let codex_home = tempdir().expect("tempdir");
@@ -376,8 +432,8 @@ environment_id = "devbox-123"
] {
let config = ExecServerToml {
remote: Some(RemoteExecServerToml {
url: "https://chatgpt.com".to_string(),
environment_id: "devbox-123".to_string(),
url: Some("https://chatgpt.com".to_string()),
environment_id: Some("devbox-123".to_string()),
name: None,
use_agent_identity_auth: None,
reconnect_initial_backoff_sec: Some(initial),
@@ -394,4 +450,38 @@ environment_id = "devbox-123"
);
}
}
#[test]
fn file_rejects_invalid_detached_session_ttl() {
let config = ExecServerToml {
sessions: Some(SessionsToml {
detached_ttl_sec: Some(u64::MAX),
}),
..Default::default()
};
let err = ExecServerConfig::try_from(config).expect_err("invalid config");
assert_eq!(
err.to_string(),
"exec-server protocol error: session detached TTL is too large"
);
}
#[test]
fn runtime_rejects_invalid_detached_session_ttl() {
let config = ExecServerRuntimeConfig {
sessions: SessionRegistryConfig {
detached_session_ttl: Duration::MAX,
},
..Default::default()
};
let err = config.validate().expect_err("invalid config");
assert_eq!(
err.to_string(),
"exec-server protocol error: session detached TTL is too large"
);
}
}
codex-rs/exec-server/src/remote.rs
+20 -3
View File
@@ -144,9 +144,13 @@ impl RemoteEnvironmentConfig {
Ok(self)
}
pub fn with_runtime(mut self, runtime: ExecServerRuntimeConfig) -> Self {
pub fn with_runtime(
mut self,
runtime: ExecServerRuntimeConfig,
) -> Result<Self, ExecServerError> {
runtime.validate()?;
self.runtime = runtime;
self
Ok(self)
}
}
@@ -157,6 +161,7 @@ pub async fn run_remote_environment(
runtime_paths: ExecServerRuntimePaths,
) -> Result<(), ExecServerError> {
ensure_rustls_crypto_provider();
config.runtime.validate()?;
let client =
EnvironmentRegistryClient::new(config.base_url.clone(), config.auth_provider.clone())?;
let processor = ConnectionProcessor::new_with_config(runtime_paths, config.runtime.clone());
@@ -180,10 +185,14 @@ pub async fn run_remote_environment(
}
sleep(backoff).await;
backoff = (backoff * 2).min(config.reconnect_max_backoff);
backoff = next_reconnect_backoff(backoff, config.reconnect_max_backoff);
}
}
fn next_reconnect_backoff(backoff: Duration, reconnect_max_backoff: Duration) -> Duration {
backoff.saturating_mul(2).min(reconnect_max_backoff)
}
fn normalize_environment_id(environment_id: String) -> Result<String, ExecServerError> {
let environment_id = environment_id.trim().to_string();
if environment_id.is_empty() {
@@ -390,4 +399,12 @@ mod tests {
assert!(debug.contains("<redacted>"));
assert!(!debug.contains("workspace-123"));
}
#[test]
fn next_reconnect_backoff_saturates_before_capping() {
assert_eq!(
next_reconnect_backoff(Duration::MAX, Duration::MAX),
Duration::MAX
);
}
}
codex-rs/exec-server/src/server.rs
+1
View File
@@ -31,5 +31,6 @@ pub async fn run_main_with_config(
runtime_paths: ExecServerRuntimePaths,
config: ExecServerRuntimeConfig,
) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
config.validate()?;
transport::run_transport(listen_url, runtime_paths, config).await
}
codex-rs/exec-server/src/server/session_registry.rs
+27 -3
View File
@@ -131,7 +131,15 @@ impl SessionRegistry {
}
async fn expire_if_detached(&self, session_id: String, connection_id: ConnectionId) {
tokio::time::sleep(self.session_config.detached_session_ttl).await;
let Some(detached_expires_at) = ({
let sessions = self.sessions.lock().await;
sessions
.get(&session_id)
.and_then(|entry| entry.detached_expires_at_for(connection_id))
}) else {
return;
};
tokio::time::sleep_until(detached_expires_at).await;
let removed = {
let mut sessions = self.sessions.lock().await;
@@ -197,11 +205,15 @@ impl SessionEntry {
if attachment.current_connection_id != Some(connection_id) {
return false;
}
let Some(detached_expires_at) =
tokio::time::Instant::now().checked_add(self.detached_session_ttl)
else {
return false;
};
attachment.current_connection_id = None;
attachment.detached_connection_id = Some(connection_id);
attachment.detached_expires_at =
Some(tokio::time::Instant::now() + self.detached_session_ttl);
attachment.detached_expires_at = Some(detached_expires_at);
true
}
@@ -244,6 +256,18 @@ impl SessionEntry {
.detached_expires_at
.is_some_and(|deadline| now >= deadline)
}
fn detached_expires_at_for(&self, connection_id: ConnectionId) -> Option<tokio::time::Instant> {
let attachment = self
.attachment
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
if attachment.detached_connection_id == Some(connection_id) {
attachment.detached_expires_at
} else {
None
}
}
}
impl SessionHandle {