Skip to content

Commit d4ec08b

Browse files
[codex] consume pushed exec-server process events (#30273)
## 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.
1 parent d047c33 commit d4ec08b

12 files changed

Lines changed: 750 additions & 116 deletions

File tree

codex-rs/core/src/unified_exec/process.rs

Lines changed: 125 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ use tokio_util::sync::CancellationToken;
1414

1515
use crate::exec::is_likely_sandbox_denied;
1616
use codex_exec_server::ExecProcess;
17+
use codex_exec_server::ExecProcessEvent;
1718
use codex_exec_server::ProcessSignal as ExecServerProcessSignal;
1819
use codex_exec_server::ReadResponse as ExecReadResponse;
1920
use codex_exec_server::StartedExecProcess;
@@ -425,84 +426,151 @@ impl UnifiedExecProcess {
425426
cancellation_token,
426427
} = output_handles;
427428
let process = started.process;
428-
let mut wake_rx = process.subscribe_wake();
429+
let mut events = process.subscribe_events();
429430
tokio::spawn(async move {
430-
let mut after_seq = None;
431+
let mut last_seq: u64 = 0;
431432
loop {
432-
match process
433-
.read(after_seq, /*max_bytes*/ None, /*wait_ms*/ Some(0))
434-
.await
433+
let event = match events.recv().await {
434+
Ok(event) => Some(event),
435+
Err(broadcast::error::RecvError::Lagged(_)) => None,
436+
Err(broadcast::error::RecvError::Closed) => {
437+
let state = state_tx.borrow().clone();
438+
let _ = state_tx.send_replace(
439+
state.failed("exec-server process event stream closed".to_string()),
440+
);
441+
output_closed.store(true, Ordering::Release);
442+
output_closed_notify.notify_waiters();
443+
cancellation_token.cancel();
444+
break;
445+
}
446+
};
447+
let event_seq = event.as_ref().and_then(|event| match event {
448+
ExecProcessEvent::Output(chunk) => Some(chunk.seq),
449+
ExecProcessEvent::Exited { seq, .. } | ExecProcessEvent::Closed { seq } => {
450+
Some(*seq)
451+
}
452+
ExecProcessEvent::Failed(_) => None,
453+
});
454+
let missing_sandbox_denial = matches!(
455+
event.as_ref(),
456+
Some(ExecProcessEvent::Exited {
457+
sandbox_denied: None,
458+
..
459+
})
460+
);
461+
if event.is_none()
462+
|| event_seq.is_some_and(|seq| seq > last_seq.saturating_add(1))
463+
|| missing_sandbox_denial
435464
{
436-
Ok(response) => {
437-
let ExecReadResponse {
438-
chunks,
439-
next_seq,
440-
exited,
441-
exit_code,
442-
closed,
443-
failure,
444-
sandbox_denied,
445-
} = response;
446-
447-
for chunk in chunks {
448-
let bytes = chunk.chunk.into_inner();
449-
let mut guard = output_buffer.lock().await;
450-
guard.push_chunk(bytes.clone());
451-
drop(guard);
452-
let _ = output_tx.send(bytes);
453-
output_notify.notify_waiters();
454-
}
455-
456-
if let Some(message) = failure {
465+
let response = match process
466+
.read(
467+
Some(last_seq),
468+
/*max_bytes*/ None,
469+
/*wait_ms*/ Some(0),
470+
)
471+
.await
472+
{
473+
Ok(response) => response,
474+
Err(err) => {
457475
let state = state_tx.borrow().clone();
458-
let _ = state_tx.send_replace(state.failed(message));
476+
let _ = state_tx.send_replace(state.failed(err.to_string()));
459477
output_closed.store(true, Ordering::Release);
460478
output_closed_notify.notify_waiters();
461479
cancellation_token.cancel();
462480
break;
463481
}
482+
};
483+
let ExecReadResponse {
484+
chunks,
485+
next_seq,
486+
exited,
487+
exit_code,
488+
closed,
489+
failure,
490+
sandbox_denied,
491+
} = response;
492+
for chunk in chunks.into_iter().filter(|chunk| chunk.seq > last_seq) {
493+
let bytes = chunk.chunk.into_inner();
494+
let mut guard = output_buffer.lock().await;
495+
guard.push_chunk(bytes.clone());
496+
drop(guard);
497+
let _ = output_tx.send(bytes);
498+
output_notify.notify_waiters();
499+
}
500+
last_seq = last_seq.max(next_seq.saturating_sub(1));
501+
if let Some(message) = failure {
502+
let state = state_tx.borrow().clone();
503+
let _ = state_tx.send_replace(state.failed(message));
504+
output_closed.store(true, Ordering::Release);
505+
output_closed_notify.notify_waiters();
506+
cancellation_token.cancel();
507+
break;
508+
}
509+
if sandbox_denied || exited {
510+
let mut state = state_tx.borrow().clone();
511+
state.sandbox_denied |= sandbox_denied;
512+
let _ = state_tx.send_replace(if exited {
513+
state.exited(exit_code)
514+
} else {
515+
state
516+
});
517+
}
518+
if closed {
519+
output_closed.store(true, Ordering::Release);
520+
output_closed_notify.notify_waiters();
521+
cancellation_token.cancel();
522+
break;
523+
}
524+
continue;
525+
}
464526

465-
if sandbox_denied {
466-
let mut state = state_tx.borrow().clone();
467-
state.sandbox_denied = true;
468-
let _ = state_tx.send_replace(state);
469-
}
470-
471-
if exited {
472-
let state = state_tx.borrow().clone();
473-
let _ = state_tx.send_replace(state.exited(exit_code));
527+
let Some(event) = event else {
528+
continue;
529+
};
530+
match event {
531+
ExecProcessEvent::Output(chunk) => {
532+
if chunk.seq <= last_seq {
533+
continue;
474534
}
475-
476-
if closed {
477-
output_closed.store(true, Ordering::Release);
478-
output_closed_notify.notify_waiters();
479-
cancellation_token.cancel();
535+
last_seq = chunk.seq;
536+
let bytes = chunk.chunk.into_inner();
537+
let mut guard = output_buffer.lock().await;
538+
guard.push_chunk(bytes.clone());
539+
drop(guard);
540+
let _ = output_tx.send(bytes);
541+
output_notify.notify_waiters();
542+
}
543+
ExecProcessEvent::Exited {
544+
seq,
545+
exit_code,
546+
sandbox_denied,
547+
} => {
548+
if seq <= last_seq {
549+
continue;
480550
}
481-
482-
after_seq = next_seq.checked_sub(1);
483-
if output_closed.load(Ordering::Acquire) {
484-
break;
551+
last_seq = seq;
552+
let mut state = state_tx.borrow().clone();
553+
state.sandbox_denied |= sandbox_denied.unwrap_or(false);
554+
let _ = state_tx.send_replace(state.exited(Some(exit_code)));
555+
}
556+
ExecProcessEvent::Closed { seq } => {
557+
if seq <= last_seq {
558+
continue;
485559
}
560+
output_closed.store(true, Ordering::Release);
561+
output_closed_notify.notify_waiters();
562+
cancellation_token.cancel();
563+
break;
486564
}
487-
Err(err) => {
565+
ExecProcessEvent::Failed(message) => {
488566
let state = state_tx.borrow().clone();
489-
let _ = state_tx.send_replace(state.failed(err.to_string()));
567+
let _ = state_tx.send_replace(state.failed(message));
490568
output_closed.store(true, Ordering::Release);
491569
output_closed_notify.notify_waiters();
492570
cancellation_token.cancel();
493571
break;
494572
}
495573
}
496-
497-
if wake_rx.changed().await.is_err() {
498-
let state = state_tx.borrow().clone();
499-
let _ = state_tx
500-
.send_replace(state.failed("exec-server wake channel closed".to_string()));
501-
output_closed.store(true, Ordering::Release);
502-
output_closed_notify.notify_waiters();
503-
cancellation_token.cancel();
504-
break;
505-
}
506574
}
507575
})
508576
}

codex-rs/core/src/unified_exec/process_tests.rs

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ use std::collections::VecDeque;
1515
use std::sync::Arc;
1616
use tokio::sync::Mutex;
1717
use tokio::sync::watch;
18-
use tokio::time::Duration;
1918

2019
struct MockExecProcess {
2120
process_id: ProcessId,
@@ -173,39 +172,3 @@ async fn remote_terminate_confirmed_updates_state_on_success_only() {
173172

174173
assert!(process.has_exited());
175174
}
176-
177-
#[tokio::test]
178-
async fn remote_process_waits_for_early_exit_event() {
179-
let (wake_tx, _wake_rx) = watch::channel(0);
180-
let started = StartedExecProcess {
181-
process: Arc::new(MockExecProcess {
182-
process_id: "test-process".to_string().into(),
183-
write_response: WriteResponse {
184-
status: WriteStatus::Accepted,
185-
},
186-
read_responses: Mutex::new(VecDeque::from([ReadResponse {
187-
chunks: Vec::new(),
188-
next_seq: 2,
189-
exited: true,
190-
exit_code: Some(17),
191-
closed: true,
192-
failure: None,
193-
sandbox_denied: false,
194-
}])),
195-
terminate_error: None,
196-
wake_tx: wake_tx.clone(),
197-
}),
198-
};
199-
200-
tokio::spawn(async move {
201-
tokio::time::sleep(Duration::from_millis(10)).await;
202-
let _ = wake_tx.send(1);
203-
});
204-
205-
let process = UnifiedExecProcess::from_exec_server_started(started)
206-
.await
207-
.expect("remote process should observe early exit");
208-
209-
assert!(process.has_exited());
210-
assert_eq!(process.exit_code(), Some(17));
211-
}

codex-rs/core/tests/suite/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ mod tools;
126126
mod truncation;
127127
mod turn_state;
128128
mod unified_exec;
129+
mod unified_exec_process_events;
129130
#[cfg(unix)]
130131
mod unified_exec_zsh_fork_approvals;
131132
mod unstable_features_warning;

0 commit comments

Comments
 (0)