mirror of
https://github.com/openai/codex.git
synced 2026-09-04 15:08:45 +00:00
## Summary - complete unified-exec processes from the ordered event stream instead of issuing a final zero-wait `process/read` - add optional executor sandbox-denial state to `process/exited` - retain `process/read` as a retained-output and compatibility fallback for receiver lag, sequence gaps, and legacy servers - recover sandbox-denial state across transport reconnection - cover the real `TestCodex` remote-exec path without adding a public test-only event constructor ## Why A successful one-shot tool call currently receives its output and terminal notifications, then pays another wide-area `process/read` round trip before returning. Staging traces showed that remote response wait accounted for more than 99.8% of RPC time; local serialization, queueing, and deserialization were below 0.6 ms. ## Measured impact A direct staging A/B used the same build and route and changed only completion mode. Each arm ran three times with 30 one-shot `/usr/bin/true` calls per run. The table reports the median of the three per-run percentiles. | Metric | Final `process/read` | Pushed events | Change | | --- | ---: | ---: | ---: | | End-to-end completion p50 | 159.5 ms | 118.7 ms | -40.8 ms (-25.6%) | | End-to-end completion p95 | 182.4 ms | 131.7 ms | -50.6 ms (-27.8%) | | Completion-wait p50 | 80.1 ms | 41.5 ms | -38.5 ms (-48.1%) | | Final `process/read` RPC p50 | 79.9 ms | eliminated | -79.9 ms | TCP_NODELAY was enabled in both A/B arms, so its effect cancels out. The successful, complete, in-order event path issued zero final `process/read` calls. ## Compatibility and recovery - new servers send `sandboxDenied` on `process/exited` - legacy servers omit it, which triggers one compatibility `process/read` - broadcast lag or a sequence gap triggers a retained-output read - recovery remains bounded by the server's existing 1 MiB retained-output window - complete, in-order event streams issue no completion read - sandbox denial is attached to the exit event before consumers can observe process completion - server-first and client-first rollouts remain wire-compatible; server-first realizes the latency win immediately ## Integration coverage The `TestCodex` suite exercises four distinct remote-exec contracts: - complete pushed output/exit/close with zero reads - direct pushed sandbox denial with zero reads - legacy missing denial metadata with exactly one compatibility read - count-bounded replay eviction recovered from retained output without duplication ## Validation - `just test -p codex-core exec_command_consumes_pushed_remote_process_events`: 4 passed - `just test -p codex-core unified_exec::process_tests::`: 4 passed - `just test -p codex-exec-server`: 294 passed, 2 skipped - `just test -p codex-exec-server-protocol`: 5 passed - `just test -p codex-rmcp-client`: 89 passed, 2 skipped - focused Bazel `//codex-rs/core:core-all-test`: passed across 16 shards - scoped `just fix` passed for core and exec-server - `just fmt` passed The complete workspace suite was not rerun; focused Cargo and Bazel coverage passed for the changed behavior.
274 lines
8.6 KiB
Rust
274 lines
8.6 KiB
Rust
use std::collections::VecDeque;
|
|
use std::future::Future;
|
|
use std::pin::Pin;
|
|
use std::sync::Arc;
|
|
use std::sync::Mutex as StdMutex;
|
|
|
|
use tokio::sync::broadcast;
|
|
use tokio::sync::watch;
|
|
|
|
use crate::ExecServerError;
|
|
use crate::ProcessId;
|
|
use crate::protocol::ExecParams;
|
|
use crate::protocol::ProcessOutputChunk;
|
|
use crate::protocol::ProcessSignal;
|
|
use crate::protocol::ReadResponse;
|
|
use crate::protocol::WriteResponse;
|
|
|
|
pub struct StartedExecProcess {
|
|
pub process: Arc<dyn ExecProcess>,
|
|
}
|
|
|
|
/// Pushed process events for consumers that want to follow process output as it
|
|
/// arrives instead of polling retained output with [`ExecProcess::read`].
|
|
///
|
|
/// The stream is scoped to one [`ExecProcess`] handle. `Output` events carry
|
|
/// stdout, stderr, or pty bytes. `Exited` reports the process exit status, while
|
|
/// `Closed` means all output streams have ended and no more output events will
|
|
/// arrive. `Failed` is used when the process session cannot continue, for
|
|
/// example because the remote environment connection disconnected.
|
|
#[derive(Debug, Clone, PartialEq, Eq)]
|
|
pub enum ExecProcessEvent {
|
|
Output(ProcessOutputChunk),
|
|
Exited {
|
|
seq: u64,
|
|
exit_code: i32,
|
|
sandbox_denied: Option<bool>,
|
|
},
|
|
Closed {
|
|
seq: u64,
|
|
},
|
|
Failed(String),
|
|
}
|
|
|
|
/// Replay buffer plus live fan-out for pushed process events.
|
|
///
|
|
/// New subscribers first drain a bounded replay history, then continue on the
|
|
/// live broadcast channel. The history is bounded by event count and retained
|
|
/// output bytes: count protects against many tiny events, while bytes protects
|
|
/// against a few very large output chunks.
|
|
#[derive(Clone)]
|
|
pub(crate) struct ExecProcessEventLog {
|
|
inner: Arc<ExecProcessEventLogInner>,
|
|
}
|
|
|
|
struct ExecProcessEventLogInner {
|
|
history: StdMutex<ExecProcessEventHistory>,
|
|
live_tx: broadcast::Sender<ExecProcessEvent>,
|
|
event_capacity: usize,
|
|
byte_capacity: usize,
|
|
}
|
|
|
|
#[derive(Default)]
|
|
struct ExecProcessEventHistory {
|
|
events: VecDeque<ExecProcessEvent>,
|
|
retained_bytes: usize,
|
|
}
|
|
|
|
impl ExecProcessEvent {
|
|
/// Sequence number used to order process-owned events.
|
|
///
|
|
/// `Failed` is intentionally unsequenced because it is synthesized by the
|
|
/// client when the session or transport fails, not emitted by the process.
|
|
pub(crate) fn seq(&self) -> Option<u64> {
|
|
match self {
|
|
ExecProcessEvent::Output(chunk) => Some(chunk.seq),
|
|
ExecProcessEvent::Exited { seq, .. } | ExecProcessEvent::Closed { seq } => Some(*seq),
|
|
ExecProcessEvent::Failed(_) => None,
|
|
}
|
|
}
|
|
|
|
fn retained_len(&self) -> usize {
|
|
match self {
|
|
ExecProcessEvent::Output(chunk) => chunk.chunk.0.len(),
|
|
ExecProcessEvent::Failed(message) => message.len(),
|
|
ExecProcessEvent::Exited { .. } | ExecProcessEvent::Closed { .. } => 0,
|
|
}
|
|
}
|
|
}
|
|
|
|
impl ExecProcessEventLog {
|
|
pub(crate) fn new(event_capacity: usize, byte_capacity: usize) -> Self {
|
|
let (live_tx, _live_rx) = broadcast::channel(event_capacity);
|
|
Self {
|
|
inner: Arc::new(ExecProcessEventLogInner {
|
|
history: StdMutex::new(ExecProcessEventHistory::default()),
|
|
live_tx,
|
|
event_capacity,
|
|
byte_capacity,
|
|
}),
|
|
}
|
|
}
|
|
|
|
pub(crate) fn publish(&self, event: ExecProcessEvent) {
|
|
let mut history = self
|
|
.inner
|
|
.history
|
|
.lock()
|
|
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
|
history.retained_bytes += event.retained_len();
|
|
history.events.push_back(event.clone());
|
|
while history.events.len() > self.inner.event_capacity
|
|
|| history.retained_bytes > self.inner.byte_capacity
|
|
{
|
|
let Some(evicted) = history.events.pop_front() else {
|
|
break;
|
|
};
|
|
history.retained_bytes = history
|
|
.retained_bytes
|
|
.saturating_sub(evicted.retained_len());
|
|
}
|
|
|
|
let _ = self.inner.live_tx.send(event);
|
|
}
|
|
|
|
pub(crate) fn subscribe(&self) -> ExecProcessEventReceiver {
|
|
let history = self
|
|
.inner
|
|
.history
|
|
.lock()
|
|
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
|
let live_rx = self.inner.live_tx.subscribe();
|
|
let replay = history.events.iter().cloned().collect();
|
|
|
|
ExecProcessEventReceiver {
|
|
replay,
|
|
live_rx,
|
|
_keepalive: None,
|
|
}
|
|
}
|
|
}
|
|
|
|
pub struct ExecProcessEventReceiver {
|
|
replay: VecDeque<ExecProcessEvent>,
|
|
live_rx: broadcast::Receiver<ExecProcessEvent>,
|
|
_keepalive: Option<broadcast::Sender<ExecProcessEvent>>,
|
|
}
|
|
|
|
impl ExecProcessEventReceiver {
|
|
/// Returns a receiver that remains open without yielding events.
|
|
pub fn empty() -> Self {
|
|
let (live_tx, live_rx) = broadcast::channel(1);
|
|
Self {
|
|
replay: VecDeque::new(),
|
|
live_rx,
|
|
_keepalive: Some(live_tx),
|
|
}
|
|
}
|
|
|
|
/// Returns the next replayed or live event.
|
|
///
|
|
/// `Lagged` means this receiver fell behind the bounded live channel. The
|
|
/// caller should recover through [`ExecProcess::read`] using the last
|
|
/// delivered sequence number, then continue receiving pushed events.
|
|
pub async fn recv(&mut self) -> Result<ExecProcessEvent, broadcast::error::RecvError> {
|
|
if let Some(event) = self.replay.pop_front() {
|
|
return Ok(event);
|
|
}
|
|
|
|
self.live_rx.recv().await
|
|
}
|
|
}
|
|
|
|
/// Handle for an executor-managed process.
|
|
///
|
|
/// Implementations must support both retained-output reads and pushed events:
|
|
/// `read` is the request/response API for callers that want to page through
|
|
/// buffered output, while `subscribe_events` is the streaming API for callers
|
|
/// that want output and lifecycle changes delivered as they happen.
|
|
pub trait ExecProcess: Send + Sync {
|
|
fn process_id(&self) -> &ProcessId;
|
|
|
|
fn subscribe_wake(&self) -> watch::Receiver<u64>;
|
|
|
|
fn subscribe_events(&self) -> ExecProcessEventReceiver;
|
|
|
|
fn read(
|
|
&self,
|
|
after_seq: Option<u64>,
|
|
max_bytes: Option<usize>,
|
|
wait_ms: Option<u64>,
|
|
) -> ExecProcessFuture<'_, ReadResponse>;
|
|
|
|
fn write(&self, chunk: Vec<u8>) -> ExecProcessFuture<'_, WriteResponse>;
|
|
|
|
fn signal(&self, signal: ProcessSignal) -> ExecProcessFuture<'_, ()>;
|
|
|
|
fn terminate(&self) -> ExecProcessFuture<'_, ()>;
|
|
}
|
|
|
|
pub type ExecProcessFuture<'a, T> =
|
|
Pin<Box<dyn Future<Output = Result<T, ExecServerError>> + Send + 'a>>;
|
|
|
|
pub trait ExecBackend: Send + Sync {
|
|
fn start(&self, params: ExecParams) -> ExecBackendFuture<'_>;
|
|
}
|
|
|
|
pub type ExecBackendFuture<'a> =
|
|
Pin<Box<dyn Future<Output = Result<StartedExecProcess, ExecServerError>> + Send + 'a>>;
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use pretty_assertions::assert_eq;
|
|
use tokio::time::Duration;
|
|
use tokio::time::timeout;
|
|
|
|
use super::ExecProcessEvent;
|
|
use super::ExecProcessEventLog;
|
|
use super::ExecProcessEventReceiver;
|
|
use crate::protocol::ExecOutputStream;
|
|
use crate::protocol::ProcessOutputChunk;
|
|
|
|
#[tokio::test]
|
|
async fn empty_event_receiver_stays_open() {
|
|
let mut events = ExecProcessEventReceiver::empty();
|
|
|
|
assert!(
|
|
timeout(Duration::from_millis(10), events.recv())
|
|
.await
|
|
.is_err()
|
|
);
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn event_history_replay_is_bounded_by_retained_bytes() {
|
|
let log = ExecProcessEventLog::new(/*event_capacity*/ 8, /*byte_capacity*/ 3);
|
|
|
|
log.publish(ExecProcessEvent::Output(ProcessOutputChunk {
|
|
seq: 1,
|
|
stream: ExecOutputStream::Stdout,
|
|
chunk: b"large".to_vec().into(),
|
|
}));
|
|
log.publish(ExecProcessEvent::Exited {
|
|
seq: 2,
|
|
exit_code: 0,
|
|
sandbox_denied: Some(false),
|
|
});
|
|
log.publish(ExecProcessEvent::Closed { seq: 3 });
|
|
|
|
let mut events = log.subscribe();
|
|
let replay = vec![
|
|
timeout(Duration::from_secs(1), events.recv())
|
|
.await
|
|
.expect("exit event replay should not time out")
|
|
.expect("exit event replay should be available"),
|
|
timeout(Duration::from_secs(1), events.recv())
|
|
.await
|
|
.expect("closed event replay should not time out")
|
|
.expect("closed event replay should be available"),
|
|
];
|
|
|
|
assert_eq!(
|
|
replay,
|
|
vec![
|
|
ExecProcessEvent::Exited {
|
|
seq: 2,
|
|
exit_code: 0,
|
|
sandbox_denied: Some(false),
|
|
},
|
|
ExecProcessEvent::Closed { seq: 3 },
|
|
]
|
|
);
|
|
}
|
|
}
|