mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[3/3] app-server: configure environment connection timeout (#29025)
## Why Remote environments registered through `environment/add` currently use the fixed 10-second WebSocket connection timeout. Slow-starting executors need a caller-selected connection window, but this should not add retry policy or couple exec-server behavior to Core’s `deferred_executor` feature. Make the timeout an optional part of the existing experimental request. Existing clients continue using the current default, while callers that know an executor may take longer can request a larger window explicitly. Depends on #28683. ## What changed - Add optional `connectTimeoutMs` to `EnvironmentAddParams` and document it in the app-server README. - Pass the optional timeout through `EnvironmentRequestProcessor` into one `EnvironmentManager::upsert_environment()` path; the manager applies the existing default when it is omitted. - Preserve the existing single-attempt lifecycle. The configured value controls WebSocket connection and handshake time for both initial connection and later reconnects; initialization retains its separate timeout. - Add an app-server integration test that sends the real JSON-RPC request and verifies a stalled handshake observes the requested timeout. ## Test plan - `just test -p codex-app-server-protocol` - `just test -p codex-exec-server` - `just test -p codex-app-server environment_add_applies_connect_timeout` ## Rollout This is additive and does not enable `deferred_executor`. Callers should send a non-default timeout only after a compatible app-server is deployed; omitted or `null` values retain the existing 10-second default.
This commit is contained in:
committed by
GitHub
Unverified
parent
45a133bae0
commit
f886e33e5a
@@ -136,10 +136,10 @@ impl std::fmt::Debug for ExecServerTransportParams {
|
||||
}
|
||||
|
||||
impl ExecServerTransportParams {
|
||||
pub(crate) fn websocket_url(websocket_url: String) -> Self {
|
||||
pub(crate) fn websocket_url(websocket_url: String, connect_timeout: Duration) -> Self {
|
||||
Self::WebSocketUrl {
|
||||
websocket_url,
|
||||
connect_timeout: DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT,
|
||||
connect_timeout,
|
||||
initialize_timeout: DEFAULT_REMOTE_EXEC_SERVER_INITIALIZE_TIMEOUT,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -11,6 +11,7 @@ use crate::NoiseChannelIdentity;
|
||||
use crate::NoiseRendezvousConnectProvider;
|
||||
use crate::client::LazyRemoteExecServerClient;
|
||||
use crate::client::http_client::ReqwestHttpClient;
|
||||
use crate::client_api::DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT;
|
||||
use crate::client_api::ExecServerTransportParams;
|
||||
use crate::environment_provider::DefaultEnvironmentProvider;
|
||||
use crate::environment_provider::EnvironmentDefault;
|
||||
@@ -288,11 +289,13 @@ impl EnvironmentManager {
|
||||
}
|
||||
|
||||
/// Adds or replaces a named remote environment without changing the
|
||||
/// manager's default environment selection.
|
||||
/// manager's default environment selection. Uses the default WebSocket
|
||||
/// connection timeout when none is provided.
|
||||
pub fn upsert_environment(
|
||||
&self,
|
||||
environment_id: String,
|
||||
exec_server_url: String,
|
||||
connect_timeout: Option<std::time::Duration>,
|
||||
) -> Result<(), ExecServerError> {
|
||||
if environment_id.is_empty() {
|
||||
return Err(ExecServerError::Protocol(
|
||||
@@ -310,8 +313,11 @@ impl EnvironmentManager {
|
||||
"remote environment requires an exec-server url".to_string(),
|
||||
));
|
||||
};
|
||||
let environment = Arc::new(Environment::remote_inner(
|
||||
exec_server_url,
|
||||
let environment = Arc::new(Environment::remote_with_transport(
|
||||
ExecServerTransportParams::websocket_url(
|
||||
exec_server_url,
|
||||
connect_timeout.unwrap_or(DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT),
|
||||
),
|
||||
self.local_runtime_paths.clone(),
|
||||
));
|
||||
environment.start_connecting();
|
||||
@@ -496,7 +502,10 @@ impl Environment {
|
||||
local_runtime_paths: Option<ExecServerRuntimePaths>,
|
||||
) -> Self {
|
||||
Self::remote_with_transport(
|
||||
ExecServerTransportParams::websocket_url(exec_server_url),
|
||||
ExecServerTransportParams::websocket_url(
|
||||
exec_server_url,
|
||||
DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT,
|
||||
),
|
||||
local_runtime_paths,
|
||||
)
|
||||
}
|
||||
@@ -980,7 +989,11 @@ mod tests {
|
||||
let manager = EnvironmentManager::without_environments();
|
||||
|
||||
manager
|
||||
.upsert_environment("executor-a".to_string(), "ws://127.0.0.1:8765".to_string())
|
||||
.upsert_environment(
|
||||
"executor-a".to_string(),
|
||||
"ws://127.0.0.1:8765".to_string(),
|
||||
/*connect_timeout*/ None,
|
||||
)
|
||||
.expect("remote environment");
|
||||
let first = manager
|
||||
.get_environment("executor-a")
|
||||
@@ -990,7 +1003,11 @@ mod tests {
|
||||
assert_eq!(manager.default_environment_id(), None);
|
||||
|
||||
manager
|
||||
.upsert_environment("executor-a".to_string(), "ws://127.0.0.1:9876".to_string())
|
||||
.upsert_environment(
|
||||
"executor-a".to_string(),
|
||||
"ws://127.0.0.1:9876".to_string(),
|
||||
/*connect_timeout*/ None,
|
||||
)
|
||||
.expect("updated remote environment");
|
||||
let second = manager
|
||||
.get_environment("executor-a")
|
||||
@@ -1011,6 +1028,7 @@ mod tests {
|
||||
.upsert_environment(
|
||||
"executor-a".to_string(),
|
||||
format!("ws://{}", listener.local_addr().expect("listener address")),
|
||||
/*connect_timeout*/ None,
|
||||
)
|
||||
.expect("remote environment");
|
||||
|
||||
@@ -1066,6 +1084,7 @@ mod tests {
|
||||
"ws://{}",
|
||||
first_listener.local_addr().expect("first listener address")
|
||||
),
|
||||
/*connect_timeout*/ None,
|
||||
)
|
||||
.expect("first remote environment");
|
||||
let environment = manager
|
||||
@@ -1090,6 +1109,7 @@ mod tests {
|
||||
.local_addr()
|
||||
.expect("second listener address")
|
||||
),
|
||||
/*connect_timeout*/ None,
|
||||
)
|
||||
.expect("replacement remote environment");
|
||||
|
||||
@@ -1107,7 +1127,11 @@ mod tests {
|
||||
let manager = EnvironmentManager::without_environments();
|
||||
|
||||
let err = manager
|
||||
.upsert_environment("executor-a".to_string(), String::new())
|
||||
.upsert_environment(
|
||||
"executor-a".to_string(),
|
||||
String::new(),
|
||||
/*connect_timeout*/ None,
|
||||
)
|
||||
.expect_err("empty URL should fail");
|
||||
|
||||
assert_eq!(
|
||||
|
||||
@@ -23,6 +23,7 @@ use tokio_tungstenite::accept_async;
|
||||
use tokio_tungstenite::tungstenite::Message;
|
||||
|
||||
use super::*;
|
||||
use crate::client_api::DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT;
|
||||
use crate::client_api::ExecServerTransportParams;
|
||||
use crate::protocol::FS_READ_FILE_METHOD;
|
||||
use crate::protocol::FsReadFileParams;
|
||||
@@ -36,7 +37,10 @@ async fn remote_file_system_sends_path_and_sandbox_cwd_uris_without_native_conve
|
||||
let (websocket_url, captured_params, server) =
|
||||
record_read_file_params(/*expected_requests*/ 2).await;
|
||||
let file_system = RemoteFileSystem::new(LazyRemoteExecServerClient::new(
|
||||
ExecServerTransportParams::websocket_url(websocket_url),
|
||||
ExecServerTransportParams::websocket_url(
|
||||
websocket_url,
|
||||
DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT,
|
||||
),
|
||||
));
|
||||
let paths = vec![
|
||||
PathUri::parse("file:///C:/Users/Alice/src/main.rs").expect("valid drive URI"),
|
||||
|
||||
Reference in New Issue
Block a user