From c171ed3ac0503681e861fc7540cb03947be89489 Mon Sep 17 00:00:00 2001 From: Sayan Sisodiya Date: Tue, 16 Jun 2026 23:37:00 -0700 Subject: [PATCH] exec-server: fail fast on permanent startup errors --- codex-rs/exec-server/src/client.rs | 56 ++++++++++++++++++++++++++++- codex-rs/exec-server/tests/relay.rs | 9 +++-- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/codex-rs/exec-server/src/client.rs b/codex-rs/exec-server/src/client.rs index ca6b103c89..30e0185a7a 100644 --- a/codex-rs/exec-server/src/client.rs +++ b/codex-rs/exec-server/src/client.rs @@ -386,7 +386,7 @@ async fn connect_with_startup_retries( loop { match ExecServerClient::connect_for_transport(transport_params.clone()).await { Ok(client) => return Ok(client), - Err(error) => { + Err(error) if error.is_retryable_during_startup() => { debug!( %error, retry_in = ?retry_delay, @@ -395,6 +395,7 @@ async fn connect_with_startup_retries( tokio::time::sleep(retry_delay).await; retry_delay = (retry_delay * 2).min(ENVIRONMENT_MAX_RETRY_DELAY); } + Err(error) => return Err(Arc::new(error)), } } }; @@ -481,6 +482,36 @@ pub enum ExecServerError { StartupTimedOut { timeout: Duration }, } +impl ExecServerError { + fn is_retryable_during_startup(&self) -> bool { + // This is a positive allowlist so new error variants fail fast by default. + match self { + Self::WebSocketConnectTimeout { .. } + | Self::InitializeTimedOut { .. } + | Self::Closed + | Self::Disconnected(_) => true, + Self::WebSocketConnect { source, .. } => match source { + tokio_tungstenite::tungstenite::Error::Io(_) => true, + tokio_tungstenite::tungstenite::Error::Http(response) => { + retryable_http_status(response.status()) + } + _ => false, + }, + Self::EnvironmentRegistryHttp { status, .. } => retryable_http_status(*status), + Self::EnvironmentRegistryRequest(error) => error.is_connect() || error.is_timeout(), + _ => false, + } + } +} + +fn retryable_http_status(status: reqwest::StatusCode) -> bool { + status.is_server_error() + || matches!( + status, + reqwest::StatusCode::REQUEST_TIMEOUT | reqwest::StatusCode::TOO_MANY_REQUESTS + ) +} + impl ExecServerClient { pub async fn initialize( &self, @@ -1900,6 +1931,29 @@ mod tests { server.await.expect("server task should finish"); } + #[tokio::test] + async fn invalid_websocket_url_fails_without_startup_retries() { + let client = LazyRemoteExecServerClient::new(ExecServerTransportParams::WebSocketUrl { + websocket_url: "http://example.com".to_string(), + connect_timeout: Duration::from_secs(1), + initialize_timeout: Duration::from_secs(1), + }); + + let result = timeout(Duration::from_secs(1), client.get()) + .await + .expect("permanent startup failure should return immediately"); + let error = match result { + Ok(_) => panic!("invalid URL should fail"), + Err(error) => error, + }; + + assert!(matches!( + error, + super::ExecServerError::ConnectionAttempt(source) + if matches!(source.as_ref(), super::ExecServerError::WebSocketConnect { .. }) + )); + } + #[tokio::test] async fn initial_connection_retries_once_and_is_shared_by_all_waiters() { let listener = TcpListener::bind("127.0.0.1:0") diff --git a/codex-rs/exec-server/tests/relay.rs b/codex-rs/exec-server/tests/relay.rs index cbb0259ec7..6d3cc3e53e 100644 --- a/codex-rs/exec-server/tests/relay.rs +++ b/codex-rs/exec-server/tests/relay.rs @@ -34,6 +34,7 @@ use futures::StreamExt; use futures::future::BoxFuture; use http::HeaderMap; use http::HeaderValue; +use http::StatusCode; use pretty_assertions::assert_eq; use prost::Message as ProstMessage; use relay_proto::RelayMessageFrame; @@ -82,9 +83,11 @@ impl NoiseRendezvousConnectProvider for FailingNoiseConnectProvider { ) -> BoxFuture<'_, Result> { let _ = self.attempt_tx.send(()); async { - Err(ExecServerError::Protocol( - "test registry connect failure".to_string(), - )) + Err(ExecServerError::EnvironmentRegistryHttp { + status: StatusCode::SERVICE_UNAVAILABLE, + code: None, + message: "test registry unavailable".to_string(), + }) } .boxed() }