code-mode: introduce durable session interface#24180
Conversation
cconger
force-pushed
the
cconger/code-mode-session-runtime
branch
2 times, most recently
from
7823471 to
c883312
Compare
| let (response_tx, response_rx) = oneshot::channel(); | ||
| self.start_session( | ||
| let cell_id = self.allocate_cell_id(); | ||
| self.start_cell( |
There was a problem hiding this comment.
I think shutdown can hang if an exec comes in?
T1: execute() checks shutting_down == false
T2: shutdown() sets shutting_down = true
T2: shutdown() snapshots current cells; sees none
T1: start_cell() inserts a new live cell
T2: shutdown() waits until cells is empty forever
There was a problem hiding this comment.
We should reject this now.
| pub(super) struct CodeModeDispatchBroker { | ||
| dispatch_tx: async_channel::Sender<DispatchMessage>, | ||
| dispatch_rx: async_channel::Receiver<DispatchMessage>, | ||
| dispatch_gates: Arc<Mutex<HashMap<String, watch::Sender<bool>>>>, |
There was a problem hiding this comment.
these dont seem like they ever get cleaned up after cell completion/termination. probably not a biggy but unbounded growth
| CodeModeSessionResultFuture<'a, Arc<dyn CodeModeSession>>; | ||
| pub type ToolInvocationFuture<'a> = | ||
| Pin<Box<dyn Future<Output = Result<JsonValue, String>> + Send + 'a>>; | ||
| pub type NotificationFuture<'a> = Pin<Box<dyn Future<Output = Result<(), String>> + Send + 'a>>; |
There was a problem hiding this comment.
nit but maybe just do like:
pub type CodeModeFuture<'a, T> =
Pin<Box<dyn Future<Output = Result<T, String>> + Send + 'a>>;
...
fn invoke_tool<'a>(...) -> CodeModeFuture<'a, JsonValue>;
fn notify<'a>(...) -> CodeModeFuture<'a, ()>;
fn create_session<'a>(...) -> CodeModeFuture<'a, Arc<dyn CodeModeSession>>;
There was a problem hiding this comment.
so we dont need one per type
There was a problem hiding this comment.
Going to leave explicit for now the pool isn't massive, can be a follow up once this is stable.
| pub type NotificationFuture<'a> = Pin<Box<dyn Future<Output = Result<(), String>> + Send + 'a>>; | ||
|
|
||
| #[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)] | ||
| pub struct CellId(String); |
There was a problem hiding this comment.
IMO if we want to introduce this instead of a bare string, lets remove the automatic conversions and make sure its updated everywhere.
session still uses a mix of CellId and String (StartedCell.cell_id vs WaitRequest.cell_id, terminate(String), etc.
There was a problem hiding this comment.
Yeah, bit the bullet and threaded all the way through now.
| }).await; | ||
| let delegate = Arc::clone(&inner.delegate); | ||
| let cell_id = cell_id.clone(); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
I think we're losing our cancellation token here and in a few other cases
There was a problem hiding this comment.
its somewhat of a nit, but i dont really like bare tokio::spawn w/o some way to ensure they dont leak indefinitely
| let dispatch_broker = Arc::new(CodeModeDispatchBroker::new()); | ||
| Self { | ||
| inner: codex_code_mode::CodeModeService::new(), | ||
| session: Some(Arc::new(codex_code_mode::CodeModeService::with_delegate( |
There was a problem hiding this comment.
should we swap to CodeModeSessionProvider?
There was a problem hiding this comment.
I think this will get picked up in the next change where I start dependency injecting this, going to leave as service for now.
cconger
force-pushed
the
cconger/code-mode-session-runtime
branch
from
fbfc43a to
c3c91e9
Compare
2 participants
Summary
Introduce a
CodeModeSessioninterface for executing and managing code-mode cells.This moves cell lifecycle, callback delegation, termination, and shutdown behind a session abstraction, while continuing to use the existing in-process implementation, and the ability to implement an external process one behind this interface.
A Codex session owns one
CodeModeSession, which in turn owns its running cells and stored code-mode state. Each cell is represented to the caller as aStartedCell, exposing its cell ID and initial response.It also introduces a
CodeModeSessionDelegatecallback interface. A session uses the delegate to invoke nested host tools and emit notifications while a cell is running, allowing the runtime to communicate with its owning Codex session without depending directly on core turn handling.