fix: Scope ExecSessionManager to Session instead of using global singleton (#2664)
The `SessionManager` in `exec_command` owns a number of `ExecCommandSession` objects where `ExecCommandSession` has a non-trivial implementation of `Drop`, so we want to be able to drop an individual `SessionManager` to help ensure things get cleaned up in a timely fashion. To that end, we should have one `SessionManager` per session rather than one global one for the lifetime of the CLI process.
This commit is contained in:
@@ -55,7 +55,7 @@ use crate::exec::StreamOutput;
|
||||
use crate::exec::process_exec_tool_call;
|
||||
use crate::exec_command::EXEC_COMMAND_TOOL_NAME;
|
||||
use crate::exec_command::ExecCommandParams;
|
||||
use crate::exec_command::SESSION_MANAGER;
|
||||
use crate::exec_command::ExecSessionManager;
|
||||
use crate::exec_command::WRITE_STDIN_TOOL_NAME;
|
||||
use crate::exec_command::WriteStdinParams;
|
||||
use crate::exec_env::create_env;
|
||||
@@ -269,6 +269,7 @@ pub(crate) struct Session {
|
||||
|
||||
/// Manager for external MCP servers/tools.
|
||||
mcp_connection_manager: McpConnectionManager,
|
||||
session_manager: ExecSessionManager,
|
||||
|
||||
/// External notifier command (will be passed as args to exec()). When
|
||||
/// `None` this feature is disabled.
|
||||
@@ -528,6 +529,7 @@ impl Session {
|
||||
session_id,
|
||||
tx_event: tx_event.clone(),
|
||||
mcp_connection_manager,
|
||||
session_manager: ExecSessionManager::default(),
|
||||
notify,
|
||||
state: Mutex::new(state),
|
||||
rollout: Mutex::new(rollout_recorder),
|
||||
@@ -2112,7 +2114,8 @@ async fn handle_function_call(
|
||||
};
|
||||
}
|
||||
};
|
||||
let result = SESSION_MANAGER
|
||||
let result = sess
|
||||
.session_manager
|
||||
.handle_exec_command_request(exec_params)
|
||||
.await;
|
||||
let function_call_output = crate::exec_command::result_into_payload(result);
|
||||
@@ -2134,7 +2137,8 @@ async fn handle_function_call(
|
||||
};
|
||||
}
|
||||
};
|
||||
let result = SESSION_MANAGER
|
||||
let result = sess
|
||||
.session_manager
|
||||
.handle_write_stdin_request(write_stdin_params)
|
||||
.await;
|
||||
let function_call_output: FunctionCallOutputPayload =
|
||||
|
||||
@@ -10,5 +10,5 @@ pub use responses_api::EXEC_COMMAND_TOOL_NAME;
|
||||
pub use responses_api::WRITE_STDIN_TOOL_NAME;
|
||||
pub use responses_api::create_exec_command_tool_for_responses_api;
|
||||
pub use responses_api::create_write_stdin_tool_for_responses_api;
|
||||
pub use session_manager::SESSION_MANAGER;
|
||||
pub use session_manager::SessionManager as ExecSessionManager;
|
||||
pub use session_manager::result_into_payload;
|
||||
|
||||
@@ -2,7 +2,6 @@ use std::collections::HashMap;
|
||||
use std::io::ErrorKind;
|
||||
use std::io::Read;
|
||||
use std::sync::Arc;
|
||||
use std::sync::LazyLock;
|
||||
use std::sync::Mutex as StdMutex;
|
||||
use std::sync::atomic::AtomicU32;
|
||||
|
||||
@@ -22,8 +21,6 @@ use crate::exec_command::exec_command_session::ExecCommandSession;
|
||||
use crate::exec_command::session_id::SessionId;
|
||||
use codex_protocol::models::FunctionCallOutputPayload;
|
||||
|
||||
pub static SESSION_MANAGER: LazyLock<SessionManager> = LazyLock::new(SessionManager::default);
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
pub struct SessionManager {
|
||||
next_session_id: AtomicU32,
|
||||
|
||||
Reference in New Issue
Block a user