mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
Give remote control pairing a 35-second deadline (#57228)
## Why Pairing can take up to 30 seconds in the daemon, but the client used a two-second timeout when waiting for each response message. ## What changed Apply a single 35-second deadline to `start_pairing`, including connection and initialization. Wait for the pairing response without a per-message timeout so status notifications cannot extend the overall deadline. Preserve the existing per-message timeout for other remote control requests. ## Testing Extend the manual pairing test with a three-second response delay and add coverage verifying that a status notification does not reset the pairing deadline. GitOrigin-RevId: c011fd131262b30baa21e17bc0e0f733dce53fb0
This commit is contained in:
@@ -45,6 +45,7 @@ tracing-appender = { workspace = true }
|
||||
[dev-dependencies]
|
||||
codex-utils-cargo-bin = { workspace = true }
|
||||
pretty_assertions = { workspace = true }
|
||||
tokio = { workspace = true, features = ["test-util"] }
|
||||
tracing-subscriber = { workspace = true }
|
||||
|
||||
[target.'cfg(windows)'.dependencies]
|
||||
|
||||
@@ -28,6 +28,8 @@ use crate::RemoteControlReadyStatus;
|
||||
use crate::client;
|
||||
|
||||
const REMOTE_CONTROL_READY_TIMEOUT: Duration = Duration::from_secs(10);
|
||||
// Allow the daemon's 30-second pairing request to finish and return its response.
|
||||
const PAIRING_TIMEOUT: Duration = Duration::from_secs(35);
|
||||
const REMOTE_CONTROL_REQUEST_ID: RequestId = RequestId::Integer(2);
|
||||
const INVALID_PARAMS_ERROR_CODE: i64 = -32602;
|
||||
|
||||
@@ -56,6 +58,12 @@ pub(crate) async fn disable_remote_control(socket_path: &Path) -> Result<RemoteC
|
||||
}
|
||||
|
||||
pub(crate) async fn start_pairing(socket_path: &Path) -> Result<RemoteControlPairingStartResponse> {
|
||||
timeout(PAIRING_TIMEOUT, start_pairing_inner(socket_path))
|
||||
.await
|
||||
.context("timed out waiting for remoteControl/pairing/start response")?
|
||||
}
|
||||
|
||||
async fn start_pairing_inner(socket_path: &Path) -> Result<RemoteControlPairingStartResponse> {
|
||||
let mut websocket = client::connect(socket_path).await?;
|
||||
initialize_client(&mut websocket).await?;
|
||||
let params = serde_json::to_value(RemoteControlPairingStartParams { manual_code: true })?;
|
||||
@@ -70,6 +78,7 @@ pub(crate) async fn start_pairing(socket_path: &Path) -> Result<RemoteControlPai
|
||||
&mut websocket,
|
||||
&REMOTE_CONTROL_REQUEST_ID,
|
||||
"remoteControl/pairing/start",
|
||||
/*message_timeout*/ None,
|
||||
)
|
||||
.await?
|
||||
{
|
||||
@@ -167,7 +176,15 @@ where
|
||||
Some(params),
|
||||
)
|
||||
.await?;
|
||||
match read_remote_control_response(websocket, &REMOTE_CONTROL_REQUEST_ID, method).await? {
|
||||
let message_timeout = Some(client::CONTROL_SOCKET_RESPONSE_TIMEOUT);
|
||||
match read_remote_control_response(
|
||||
websocket,
|
||||
&REMOTE_CONTROL_REQUEST_ID,
|
||||
method,
|
||||
message_timeout,
|
||||
)
|
||||
.await?
|
||||
{
|
||||
RemoteControlRpcResponse::Success(response) => Ok(response),
|
||||
RemoteControlRpcResponse::InvalidParams => {
|
||||
send_remote_control_request(
|
||||
@@ -177,8 +194,13 @@ where
|
||||
/*params*/ None,
|
||||
)
|
||||
.await?;
|
||||
match read_remote_control_response(websocket, &REMOTE_CONTROL_REQUEST_ID, method)
|
||||
.await?
|
||||
match read_remote_control_response(
|
||||
websocket,
|
||||
&REMOTE_CONTROL_REQUEST_ID,
|
||||
method,
|
||||
message_timeout,
|
||||
)
|
||||
.await?
|
||||
{
|
||||
RemoteControlRpcResponse::Success(response) => Ok(response),
|
||||
RemoteControlRpcResponse::InvalidParams => {
|
||||
@@ -217,18 +239,21 @@ async fn read_remote_control_response<S, T>(
|
||||
websocket: &mut WebSocketStream<S>,
|
||||
request_id: &RequestId,
|
||||
method: &str,
|
||||
message_timeout: Option<Duration>,
|
||||
) -> Result<RemoteControlRpcResponse<T>>
|
||||
where
|
||||
S: AsyncRead + AsyncWrite + Unpin,
|
||||
T: DeserializeOwned,
|
||||
{
|
||||
loop {
|
||||
let message = timeout(
|
||||
client::CONTROL_SOCKET_RESPONSE_TIMEOUT,
|
||||
client::read_message(websocket),
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("timed out waiting for {method} response"))??;
|
||||
let read = client::read_message(websocket);
|
||||
let message = if let Some(duration) = message_timeout {
|
||||
timeout(duration, read)
|
||||
.await
|
||||
.with_context(|| format!("timed out waiting for {method} response"))??
|
||||
} else {
|
||||
read.await?
|
||||
};
|
||||
match message {
|
||||
JSONRPCMessage::Response(response) if response.id == *request_id => {
|
||||
let response = serde_json::from_value::<T>(response.result)
|
||||
@@ -586,6 +611,10 @@ mod tests {
|
||||
pairing.params,
|
||||
Some(serde_json::json!({ "manualCode": true }))
|
||||
);
|
||||
// Use virtual time only for the reply delay, after the socket handshake.
|
||||
tokio::time::pause();
|
||||
sleep(Duration::from_secs(3)).await;
|
||||
tokio::time::resume();
|
||||
client::send_message(
|
||||
&mut websocket,
|
||||
&JSONRPCMessage::Response(JSONRPCResponse {
|
||||
@@ -616,6 +645,42 @@ mod tests {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn pairing_notification_does_not_extend_the_response_deadline() -> Result<()> {
|
||||
let dir = TempDir::new()?;
|
||||
let socket_path = dir.path().join("app-server.sock");
|
||||
let listener = UnixListener::bind(&socket_path).await?;
|
||||
let server_task = tokio::spawn(async move {
|
||||
let mut websocket = accept_initialized_client(listener).await?;
|
||||
client::read_message(&mut websocket).await?;
|
||||
|
||||
tokio::time::pause();
|
||||
sleep(Duration::from_secs(20)).await;
|
||||
send_remote_control_status(
|
||||
&mut websocket,
|
||||
remote_control_status(
|
||||
RemoteControlConnectionStatus::Connecting,
|
||||
/*environment_id*/ None,
|
||||
),
|
||||
)
|
||||
.await?;
|
||||
// With a fresh timeout after the notification, the client would see EOF at 40s.
|
||||
sleep(Duration::from_secs(20)).await;
|
||||
Ok::<_, anyhow::Error>(())
|
||||
});
|
||||
|
||||
let start = Instant::now();
|
||||
let error = start_pairing(&socket_path).await.unwrap_err();
|
||||
let elapsed = start.elapsed();
|
||||
server_task.await??;
|
||||
assert!(error.is::<tokio::time::error::Elapsed>(), "{error:#}");
|
||||
assert!(
|
||||
(Duration::from_secs(30)..Duration::from_secs(40)).contains(&elapsed),
|
||||
"pairing timed out after {elapsed:?}"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
struct EnableScenario {
|
||||
initial_notification: Option<RemoteControlStatusChangedNotification>,
|
||||
enable_response: RemoteControlStatusChangedNotification,
|
||||
|
||||
Reference in New Issue
Block a user