diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 5c86452cbb..9d0db16438 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -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, + use_agent_identity_auth: bool, + reconnect_initial_backoff: Duration, + reconnect_max_backoff: Duration, +} + fn resolve_remote_exec_server_config( url: Option, environment_id: Option, name: Option, use_agent_identity_auth: bool, configured_remote: Option, -) -> anyhow::Result { - let configured_url = configured_remote.as_ref().map(|remote| remote.url.clone()); +) -> anyhow::Result { + 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()), diff --git a/codex-rs/exec-server/src/config.rs b/codex-rs/exec-server/src/config.rs index 31c953e496..d8a44d1764 100644 --- a/codex-rs/exec-server/src/config.rs +++ b/codex-rs/exec-server/src/config.rs @@ -45,8 +45,8 @@ impl ExecServerConfig { #[derive(Clone, Debug, PartialEq, Eq)] pub struct RemoteExecServerConfig { - pub url: String, - pub environment_id: String, + pub url: Option, + pub environment_id: Option, pub name: Option, 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, + environment_id: Option, name: Option, use_agent_identity_auth: Option, reconnect_initial_backoff_sec: Option, @@ -182,7 +194,9 @@ impl TryFrom 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 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" + ); + } } diff --git a/codex-rs/exec-server/src/remote.rs b/codex-rs/exec-server/src/remote.rs index 194ccd83f4..8aed8921e4 100644 --- a/codex-rs/exec-server/src/remote.rs +++ b/codex-rs/exec-server/src/remote.rs @@ -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 { + 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 { let environment_id = environment_id.trim().to_string(); if environment_id.is_empty() { @@ -390,4 +399,12 @@ mod tests { assert!(debug.contains("")); assert!(!debug.contains("workspace-123")); } + + #[test] + fn next_reconnect_backoff_saturates_before_capping() { + assert_eq!( + next_reconnect_backoff(Duration::MAX, Duration::MAX), + Duration::MAX + ); + } } diff --git a/codex-rs/exec-server/src/server.rs b/codex-rs/exec-server/src/server.rs index 96fd882ccc..d41f22d070 100644 --- a/codex-rs/exec-server/src/server.rs +++ b/codex-rs/exec-server/src/server.rs @@ -31,5 +31,6 @@ pub async fn run_main_with_config( runtime_paths: ExecServerRuntimePaths, config: ExecServerRuntimeConfig, ) -> Result<(), Box> { + config.validate()?; transport::run_transport(listen_url, runtime_paths, config).await } diff --git a/codex-rs/exec-server/src/server/session_registry.rs b/codex-rs/exec-server/src/server/session_registry.rs index 5928bb3e31..b9cd45caaa 100644 --- a/codex-rs/exec-server/src/server/session_registry.rs +++ b/codex-rs/exec-server/src/server/session_registry.rs @@ -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 { + 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 {