mirror of
https://github.com/openai/codex.git
synced 2026-10-30 19:18:49 +00:00
Bound relay connection attempts and handle pongs during blocked writes (#51502)
## Why A stalled rendezvous WebSocket upgrade can prevent reconnects, while short-lived connections reset reconnect backoff too early. Blocked relay writes also prevent received pongs from clearing their heartbeat deadline. ## What changed - Bound rendezvous WebSocket connection attempts with `DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT`. - Retain exponential reconnect backoff across short-lived connections, resetting it only after a connection lasts at least 120 seconds. - Process pongs during blocked data writes while preserving the in-flight send and its independent write timeout. Prefer queued pongs over an expired heartbeat deadline. - Abort the relay writer when its owning connection task is dropped. ## Testing Add coverage for pongs arriving during blocked writes, missing and queued pongs, independent write timeouts, and writer cancellation. Extend reconnect tests to cover stalled WebSocket upgrades without re-registration and backoff reset after a stable session. GitOrigin-RevId: 0efa350c2b4ada9b89e23df8909063fbb69f33dc
This commit is contained in:
committed by
copyberry
parent
7298a6e020
commit
ddabe594e6
@@ -186,22 +186,32 @@ async fn reconnect_reuses_registration_until_url_is_rejected() -> Result<()> {
|
||||
static_registry_auth_provider(),
|
||||
HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault),
|
||||
)?;
|
||||
let environment_task = tokio::spawn(run_remote_environment(
|
||||
config,
|
||||
ExecServerRuntimeOptions::new(
|
||||
std::env::current_exe()?,
|
||||
/*codex_linux_sandbox_exe*/ None,
|
||||
)?,
|
||||
));
|
||||
let environment_task =
|
||||
tokio_util::task::AbortOnDropHandle::new(tokio::spawn(run_remote_environment(
|
||||
config,
|
||||
ExecServerRuntimeOptions::new(
|
||||
std::env::current_exe()?,
|
||||
/*codex_linux_sandbox_exe*/ None,
|
||||
)?,
|
||||
)));
|
||||
|
||||
let (first_socket, _peer_addr) = timeout(Duration::from_secs(5), listener.accept()).await??;
|
||||
let mut first_websocket = accept_async(first_socket).await?;
|
||||
first_websocket.close(None).await?;
|
||||
|
||||
// An ordinary disconnect retries the same URL without registering again.
|
||||
let (mut rejected_socket, _peer_addr) =
|
||||
// An ordinary disconnect and a stalled upgrade both retry without registering again.
|
||||
let (mut stalled_socket, _peer_addr) =
|
||||
timeout(Duration::from_secs(5), listener.accept()).await??;
|
||||
let mut request = [0u8; 4096];
|
||||
assert!(stalled_socket.read(&mut request).await? > 0);
|
||||
tokio::time::pause();
|
||||
tokio::time::advance(DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT).await;
|
||||
tokio::time::resume();
|
||||
|
||||
// Keep the stalled socket open until the executor starts its next attempt.
|
||||
let (mut rejected_socket, _peer_addr) =
|
||||
timeout(Duration::from_secs(5), listener.accept()).await??;
|
||||
drop(stalled_socket);
|
||||
let _ = rejected_socket.read(&mut request).await?;
|
||||
rejected_socket
|
||||
.write_all(b"HTTP/1.1 401 Unauthorized\r\nContent-Length: 0\r\n\r\n")
|
||||
@@ -209,8 +219,8 @@ async fn reconnect_reuses_registration_until_url_is_rejected() -> Result<()> {
|
||||
rejected_socket.shutdown().await?;
|
||||
|
||||
// The 4xx response discards the old registration before this attempt.
|
||||
let (third_socket, _peer_addr) = timeout(Duration::from_secs(5), listener.accept()).await??;
|
||||
let _third_websocket = accept_async(third_socket).await?;
|
||||
let (final_socket, _peer_addr) = timeout(Duration::from_secs(5), listener.accept()).await??;
|
||||
let _final_websocket = accept_async(final_socket).await?;
|
||||
registry.verify().await;
|
||||
|
||||
environment_task.abort();
|
||||
|
||||
@@ -1,8 +1,37 @@
|
||||
//! Spread executor reconnects after a shared outage without exceeding the existing backoff cap.
|
||||
//! Spread executor reconnects and retain backoff until a connection has been stable.
|
||||
|
||||
use std::time::Duration;
|
||||
|
||||
pub(super) fn reconnect_delay(backoff: Duration, random_sample: u64) -> Duration {
|
||||
const INITIAL_BACKOFF: Duration = Duration::from_secs(1);
|
||||
const MAX_BACKOFF: Duration = Duration::from_secs(30);
|
||||
// Longer than the first keepalive interval plus its pong timeout (30s + 60s).
|
||||
const STABLE_CONNECTION_DURATION: Duration = Duration::from_secs(120);
|
||||
|
||||
pub(super) struct ReconnectBackoff {
|
||||
delay: Duration,
|
||||
}
|
||||
|
||||
impl ReconnectBackoff {
|
||||
pub(super) fn new() -> Self {
|
||||
Self {
|
||||
delay: INITIAL_BACKOFF,
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn connection_closed(&mut self, connected_for: Duration) {
|
||||
if connected_for >= STABLE_CONNECTION_DURATION {
|
||||
self.delay = INITIAL_BACKOFF;
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn next_delay(&mut self, random_sample: u64) -> Duration {
|
||||
let delay = reconnect_delay(self.delay, random_sample);
|
||||
self.delay = (self.delay * 2).min(MAX_BACKOFF);
|
||||
delay
|
||||
}
|
||||
}
|
||||
|
||||
fn reconnect_delay(backoff: Duration, random_sample: u64) -> Duration {
|
||||
let upper_ms = backoff.as_millis() as u64;
|
||||
let lower_ms = upper_ms / 2;
|
||||
Duration::from_millis(lower_ms + random_sample % (upper_ms - lower_ms + 1))
|
||||
|
||||
@@ -4,6 +4,8 @@ use std::time::Duration;
|
||||
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
use super::ReconnectBackoff;
|
||||
use super::STABLE_CONNECTION_DURATION;
|
||||
use super::reconnect_delay;
|
||||
|
||||
#[test]
|
||||
@@ -23,3 +25,24 @@ fn reconnect_jitter_stays_within_each_backoff_window() {
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn flapping_connections_retain_backoff_until_a_stable_session() {
|
||||
let mut backoff = ReconnectBackoff::new();
|
||||
for seconds in [1, 2, 4, 8, 16, 30, 30] {
|
||||
backoff.connection_closed(STABLE_CONNECTION_DURATION - Duration::from_millis(1));
|
||||
assert_eq!(
|
||||
backoff.next_delay(/*random_sample*/ 0),
|
||||
Duration::from_millis(seconds * 500)
|
||||
);
|
||||
}
|
||||
backoff.connection_closed(STABLE_CONNECTION_DURATION);
|
||||
assert_eq!(
|
||||
backoff.next_delay(/*random_sample*/ 0),
|
||||
Duration::from_millis(500)
|
||||
);
|
||||
assert_eq!(
|
||||
backoff.next_delay(/*random_sample*/ 0),
|
||||
Duration::from_secs(1)
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user