mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Report selected environment connection transitions (#33251)
## What changed - Track connected and disconnected states across initial remote exec-server connections and reconnection attempts. - Emit experimental `thread/environment/connected` and `thread/environment/disconnected` app-server notifications for each thread selecting the environment. Each payload identifies the thread and environment; current state is not replayed when a thread starts. - Stop forwarding connection events when an environment selection is removed or replaced. ## Testing - Cover connection, disconnection, reconnection, shared-environment notifications, and replacement of a selected environment. GitOrigin-RevId: 5dd767372363c4a2a8319fc16164be117d5bd20c
This commit is contained in:
@@ -35,6 +35,7 @@ use crate::client_api::RemoteExecServerConnectArgs;
|
||||
use crate::client_api::StdioExecServerConnectArgs;
|
||||
use crate::client_transport::ExecServerReconnectStrategy;
|
||||
use crate::connection::JsonRpcConnection;
|
||||
use crate::environment::EnvironmentConnectionState;
|
||||
use crate::process::ExecProcessEvent;
|
||||
use crate::process::ExecProcessEventLog;
|
||||
use crate::process::ExecProcessEventReceiver;
|
||||
@@ -220,6 +221,7 @@ struct Inner {
|
||||
struct ConnectionState {
|
||||
status: ConnectionStatus,
|
||||
active_process_starts: usize,
|
||||
environment_connection_state_tx: watch::Sender<EnvironmentConnectionState>,
|
||||
}
|
||||
|
||||
enum ConnectionStatus {
|
||||
@@ -228,6 +230,34 @@ enum ConnectionStatus {
|
||||
Failed(String),
|
||||
}
|
||||
|
||||
impl ConnectionState {
|
||||
fn set_status(&mut self, status: ConnectionStatus) {
|
||||
self.status = status;
|
||||
self.publish_environment_connection_state();
|
||||
}
|
||||
|
||||
fn publish_environment_connection_state(&self) {
|
||||
let state = match &self.status {
|
||||
ConnectionStatus::Connected(rpc_client) if !rpc_client.is_disconnected() => {
|
||||
EnvironmentConnectionState::Connected
|
||||
}
|
||||
ConnectionStatus::Connected(_)
|
||||
| ConnectionStatus::Recovering
|
||||
| ConnectionStatus::Failed(_) => EnvironmentConnectionState::Disconnected,
|
||||
};
|
||||
let _ = self
|
||||
.environment_connection_state_tx
|
||||
.send_if_modified(|current| {
|
||||
if *current == state {
|
||||
false
|
||||
} else {
|
||||
*current = state;
|
||||
true
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum RecoveryPolicy {
|
||||
Wait,
|
||||
@@ -262,6 +292,7 @@ pub(crate) struct LazyRemoteExecServerClient {
|
||||
// The latest successful client, replaced whenever reconnecting succeeds.
|
||||
current_client: Arc<StdMutex<Option<ExecServerClient>>>,
|
||||
reconnect: Arc<StdMutex<Option<Arc<ConnectionAttempt>>>>,
|
||||
environment_connection_state_tx: watch::Sender<EnvironmentConnectionState>,
|
||||
}
|
||||
|
||||
impl LazyRemoteExecServerClient {
|
||||
@@ -272,9 +303,17 @@ impl LazyRemoteExecServerClient {
|
||||
startup: Arc::new(ConnectionAttempt::new()),
|
||||
current_client: Arc::new(StdMutex::new(None)),
|
||||
reconnect: Arc::new(StdMutex::new(None)),
|
||||
environment_connection_state_tx: watch::channel(
|
||||
EnvironmentConnectionState::Disconnected,
|
||||
)
|
||||
.0,
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn subscribe_connection_state(&self) -> watch::Receiver<EnvironmentConnectionState> {
|
||||
self.environment_connection_state_tx.subscribe()
|
||||
}
|
||||
|
||||
pub(crate) fn start_connecting(&self) -> Option<AbortOnDropHandle<()>> {
|
||||
// Stdio starts a process, so keep it lazy until the environment is used.
|
||||
if matches!(
|
||||
@@ -379,10 +418,7 @@ impl LazyRemoteExecServerClient {
|
||||
|
||||
async fn initial_client(&self) -> Result<ExecServerClient, ExecServerError> {
|
||||
// The first caller starts the work; every other caller waits for that same result.
|
||||
let result = self
|
||||
.startup
|
||||
.get_or_init(|| connect_once(self.transport_params.clone()))
|
||||
.await;
|
||||
let result = self.startup.get_or_init(|| self.connect_once()).await;
|
||||
match result {
|
||||
Ok(client) => {
|
||||
let mut current_client = self
|
||||
@@ -414,7 +450,7 @@ impl LazyRemoteExecServerClient {
|
||||
};
|
||||
let result = attempt
|
||||
.get_or_init(|| async {
|
||||
let result = connect_once(self.transport_params.clone()).await;
|
||||
let result = self.connect_once().await;
|
||||
if let Ok(client) = &result {
|
||||
*self
|
||||
.current_client
|
||||
@@ -458,12 +494,17 @@ impl LazyRemoteExecServerClient {
|
||||
| ExecServerTransportParams::NoiseRendezvous { .. }
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
async fn connect_once(transport_params: ExecServerTransportParams) -> ConnectionResult {
|
||||
ExecServerClient::connect_for_transport(transport_params)
|
||||
.await
|
||||
.map_err(Arc::new)
|
||||
async fn connect_once(&self) -> ConnectionResult {
|
||||
let result = ExecServerClient::connect_for_transport(self.transport_params.clone())
|
||||
.await
|
||||
.map_err(Arc::new);
|
||||
if let Ok(client) = &result {
|
||||
client
|
||||
.attach_environment_connection_state(self.environment_connection_state_tx.clone());
|
||||
}
|
||||
result
|
||||
}
|
||||
}
|
||||
|
||||
impl HttpClient for LazyRemoteExecServerClient {
|
||||
@@ -534,6 +575,19 @@ pub enum ExecServerError {
|
||||
}
|
||||
|
||||
impl ExecServerClient {
|
||||
fn attach_environment_connection_state(
|
||||
&self,
|
||||
state_tx: watch::Sender<EnvironmentConnectionState>,
|
||||
) {
|
||||
let mut connection = self
|
||||
.inner
|
||||
.connection
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
connection.environment_connection_state_tx = state_tx;
|
||||
connection.publish_environment_connection_state();
|
||||
}
|
||||
|
||||
fn fail_fast(&self) -> Result<Self, ExecServerError> {
|
||||
self.rpc_client_without_recovery()?;
|
||||
Ok(Self {
|
||||
@@ -890,6 +944,10 @@ impl ExecServerClient {
|
||||
connection: StdMutex::new(ConnectionState {
|
||||
status: ConnectionStatus::Connected(Arc::clone(&rpc_client)),
|
||||
active_process_starts: 0,
|
||||
environment_connection_state_tx: watch::channel(
|
||||
EnvironmentConnectionState::Connected,
|
||||
)
|
||||
.0,
|
||||
}),
|
||||
connection_changed,
|
||||
sessions: ArcSwap::from_pointee(HashMap::new()),
|
||||
|
||||
@@ -298,7 +298,7 @@ impl Inner {
|
||||
ConnectionStatus::Connected(current)
|
||||
if Arc::ptr_eq(current, &failed_rpc_client) =>
|
||||
{
|
||||
connection.status = ConnectionStatus::Recovering;
|
||||
connection.set_status(ConnectionStatus::Recovering);
|
||||
true
|
||||
}
|
||||
ConnectionStatus::Connected(_)
|
||||
@@ -411,7 +411,7 @@ impl Inner {
|
||||
{
|
||||
false
|
||||
} else {
|
||||
connection.status = ConnectionStatus::Connected(rpc_client);
|
||||
connection.set_status(ConnectionStatus::Connected(rpc_client));
|
||||
true
|
||||
}
|
||||
};
|
||||
@@ -512,7 +512,7 @@ impl Inner {
|
||||
match &connection.status {
|
||||
ConnectionStatus::Failed(existing) => (existing.clone(), false),
|
||||
ConnectionStatus::Connected(_) | ConnectionStatus::Recovering => {
|
||||
connection.status = ConnectionStatus::Failed(message.clone());
|
||||
connection.set_status(ConnectionStatus::Failed(message.clone()));
|
||||
(message, true)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -29,6 +29,7 @@ use crate::remote::NoiseRendezvousEnvironmentConfig;
|
||||
use crate::remote_file_system::RemoteFileSystem;
|
||||
use crate::remote_process::RemoteProcess;
|
||||
use tokio::sync::oneshot;
|
||||
use tokio::sync::watch;
|
||||
use tokio_util::task::AbortOnDropHandle;
|
||||
|
||||
pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL";
|
||||
@@ -40,6 +41,15 @@ pub const CODEX_EXEC_SERVER_NOISE_AUTH_TOKEN_ENV_VAR: &str = "CODEX_EXEC_SERVER_
|
||||
pub const CODEX_EXEC_SERVER_NOISE_CHATGPT_ACCOUNT_ID_ENV_VAR: &str =
|
||||
"CODEX_EXEC_SERVER_NOISE_CHATGPT_ACCOUNT_ID";
|
||||
|
||||
/// The current connection state for one concrete environment.
|
||||
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
|
||||
pub enum EnvironmentConnectionState {
|
||||
/// An initialized exec-server connection is available.
|
||||
Connected,
|
||||
/// No initialized exec-server connection is currently available.
|
||||
Disconnected,
|
||||
}
|
||||
|
||||
/// Owns the execution/filesystem environments available to the Codex runtime.
|
||||
///
|
||||
/// `EnvironmentManager` is a shared registry for concrete environments. Its
|
||||
@@ -344,11 +354,7 @@ impl EnvironmentManager {
|
||||
),
|
||||
self.local_runtime_paths.clone(),
|
||||
));
|
||||
environment.start_connecting();
|
||||
self.environments
|
||||
.write()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.insert(environment_id, environment);
|
||||
self.insert_environment(environment_id, environment);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -368,11 +374,7 @@ impl EnvironmentManager {
|
||||
})),
|
||||
self.local_runtime_paths.clone(),
|
||||
));
|
||||
environment.start_connecting();
|
||||
self.environments
|
||||
.write()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.insert(environment_id, environment);
|
||||
self.insert_environment(environment_id, environment);
|
||||
Ok(DeferredEnvironmentRegistration(completion))
|
||||
}
|
||||
|
||||
@@ -392,12 +394,16 @@ impl EnvironmentManager {
|
||||
ExecServerTransportParams::NoiseRendezvous { provider, identity },
|
||||
self.local_runtime_paths.clone(),
|
||||
));
|
||||
environment.start_connecting();
|
||||
self.insert_environment(environment_id, environment);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn insert_environment(&self, environment_id: String, environment: Arc<Environment>) {
|
||||
self.environments
|
||||
.write()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.insert(environment_id, environment);
|
||||
Ok(())
|
||||
.insert(environment_id, Arc::clone(&environment));
|
||||
environment.start_connecting();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -608,6 +614,15 @@ impl Environment {
|
||||
self.remote_client.is_some()
|
||||
}
|
||||
|
||||
/// Subscribes to the current connection state for this remote environment.
|
||||
pub fn subscribe_connection_state(
|
||||
&self,
|
||||
) -> Option<watch::Receiver<EnvironmentConnectionState>> {
|
||||
self.remote_client
|
||||
.as_ref()
|
||||
.map(LazyRemoteExecServerClient::subscribe_connection_state)
|
||||
}
|
||||
|
||||
pub fn local_runtime_paths(&self) -> Option<&ExecServerRuntimePaths> {
|
||||
self.local_runtime_paths.as_ref()
|
||||
}
|
||||
|
||||
@@ -67,6 +67,7 @@ pub use environment::CODEX_EXEC_SERVER_NOISE_REGISTRY_URL_ENV_VAR;
|
||||
pub use environment::CODEX_EXEC_SERVER_URL_ENV_VAR;
|
||||
pub use environment::DeferredEnvironmentRegistration;
|
||||
pub use environment::Environment;
|
||||
pub use environment::EnvironmentConnectionState;
|
||||
pub use environment::EnvironmentManager;
|
||||
pub use environment::EnvironmentObservedStatus;
|
||||
pub use environment::LOCAL_ENVIRONMENT_ID;
|
||||
|
||||
@@ -45,6 +45,9 @@ async fn deferred_environment_waits_before_connecting() -> anyhow::Result<()> {
|
||||
let registration =
|
||||
manager.register_deferred_noise_environment("tools".to_string(), provider.clone())?;
|
||||
let environment = manager.get_environment("tools").expect("environment");
|
||||
let connection_state = environment
|
||||
.subscribe_connection_state()
|
||||
.expect("remote environment connection state");
|
||||
let mut readiness = Box::pin(environment.wait_until_ready());
|
||||
|
||||
assert!(poll!(&mut readiness).is_pending());
|
||||
@@ -54,6 +57,7 @@ async fn deferred_environment_waits_before_connecting() -> anyhow::Result<()> {
|
||||
let error = readiness.await.unwrap_err();
|
||||
assert!(error.to_string().contains("test Noise provider called"));
|
||||
assert_eq!(provider.calls(), 1);
|
||||
assert!(!connection_state.has_changed()?);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -15,6 +15,7 @@ use anyhow::Result;
|
||||
use base64::Engine as _;
|
||||
use base64::engine::general_purpose::STANDARD;
|
||||
use codex_api::AuthProvider;
|
||||
use codex_exec_server::EnvironmentConnectionState;
|
||||
use codex_exec_server::EnvironmentManager;
|
||||
use codex_exec_server::ExecParams;
|
||||
use codex_exec_server::ExecResponse;
|
||||
@@ -42,6 +43,7 @@ use relay_proto::relay_message_frame;
|
||||
use tempfile::TempDir;
|
||||
use tokio::net::TcpListener;
|
||||
use tokio::net::TcpStream;
|
||||
use tokio::sync::watch;
|
||||
use tokio::time::timeout;
|
||||
use tokio_tungstenite::WebSocketStream;
|
||||
use tokio_tungstenite::accept_async;
|
||||
@@ -157,6 +159,9 @@ async fn deferred_noise_environment_connects_and_reconnects_with_fresh_bundle()
|
||||
let environment = manager
|
||||
.get_environment(ENVIRONMENT_ID)
|
||||
.context("deferred Noise environment")?;
|
||||
let mut connection_state = environment
|
||||
.subscribe_connection_state()
|
||||
.context("remote environment connection state")?;
|
||||
|
||||
assert_eq!(provider.calls(), 0);
|
||||
registration.complete(Ok(()))?;
|
||||
@@ -170,9 +175,17 @@ async fn deferred_noise_environment_connects_and_reconnects_with_fresh_bundle()
|
||||
.await
|
||||
.context("deferred Noise environment should become ready")??;
|
||||
assert_eq!(provider.calls(), 1);
|
||||
assert_eq!(
|
||||
next_connection_state(&mut connection_state).await?,
|
||||
EnvironmentConnectionState::Connected
|
||||
);
|
||||
|
||||
first_relay.abort();
|
||||
let _ = first_relay.await;
|
||||
assert_eq!(
|
||||
next_connection_state(&mut connection_state).await?,
|
||||
EnvironmentConnectionState::Disconnected
|
||||
);
|
||||
let first_reconnected_websocket = accept_websocket(&listener, "reconnected peer").await?;
|
||||
let second_reconnected_websocket = accept_websocket(&listener, "reconnected peer").await?;
|
||||
let second_relay = tokio::spawn(proxy_relay_frames(
|
||||
@@ -186,6 +199,10 @@ async fn deferred_noise_environment_connects_and_reconnects_with_fresh_bundle()
|
||||
|
||||
assert_eq!(recovered_info, initial_info);
|
||||
assert_eq!(provider.calls(), 2);
|
||||
assert_eq!(
|
||||
next_connection_state(&mut connection_state).await?,
|
||||
EnvironmentConnectionState::Connected
|
||||
);
|
||||
registry.verify().await;
|
||||
|
||||
second_relay.abort();
|
||||
@@ -195,6 +212,13 @@ async fn deferred_noise_environment_connects_and_reconnects_with_fresh_bundle()
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn next_connection_state(
|
||||
state: &mut watch::Receiver<EnvironmentConnectionState>,
|
||||
) -> Result<EnvironmentConnectionState> {
|
||||
timeout(TEST_TIMEOUT, state.changed()).await??;
|
||||
Ok(*state.borrow_and_update())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
|
||||
async fn remote_environment_routes_encrypted_exec_server_rpc() -> Result<()> {
|
||||
let listener = TcpListener::bind("127.0.0.1:0").await?;
|
||||
|
||||
Reference in New Issue
Block a user