mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[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.
This commit is contained in:
committed by
GitHub
Unverified
parent
d047c33a1b
commit
d4ec08b8f0
@@ -1161,6 +1161,7 @@ async fn handle_server_notification(
|
||||
let published_closed = session.publish_ordered_event(ExecProcessEvent::Exited {
|
||||
seq: params.seq,
|
||||
exit_code: params.exit_code,
|
||||
sandbox_denied: params.sandbox_denied,
|
||||
});
|
||||
if published_closed {
|
||||
inner.remove_session_if(¶ms.process_id, &session);
|
||||
@@ -1737,6 +1738,7 @@ mod tests {
|
||||
process_id: process_id.clone(),
|
||||
seq: 3,
|
||||
exit_code: 0,
|
||||
sandbox_denied: Some(true),
|
||||
})
|
||||
.expect("exit notification should serialize"),
|
||||
),
|
||||
@@ -1786,6 +1788,7 @@ mod tests {
|
||||
ExecProcessEvent::Exited {
|
||||
seq: 3,
|
||||
exit_code: 0,
|
||||
sandbox_denied: Some(true),
|
||||
},
|
||||
ExecProcessEvent::Closed { seq: 4 },
|
||||
]
|
||||
@@ -2391,6 +2394,7 @@ mod tests {
|
||||
process_id: quiet_process_id,
|
||||
seq: 1,
|
||||
exit_code: 17,
|
||||
sandbox_denied: Some(false),
|
||||
})
|
||||
.expect("exit notification should serialize"),
|
||||
),
|
||||
|
||||
@@ -58,7 +58,7 @@ impl SessionState {
|
||||
exit_code,
|
||||
closed,
|
||||
failure,
|
||||
sandbox_denied: _,
|
||||
sandbox_denied,
|
||||
} = response;
|
||||
if let Some(message) = failure {
|
||||
return Err(ExecServerError::Protocol(format!(
|
||||
@@ -102,11 +102,21 @@ impl SessionState {
|
||||
}
|
||||
}
|
||||
|
||||
let exit_known = ordered_events.exit_published
|
||||
|| ordered_events
|
||||
.pending
|
||||
.range(..=target_seq)
|
||||
.any(|(_, event)| matches!(event, ExecProcessEvent::Exited { .. }));
|
||||
let pending_exit = ordered_events.pending.range_mut(..=target_seq).find_map(
|
||||
|(_, event)| match event {
|
||||
ExecProcessEvent::Exited {
|
||||
sandbox_denied: pending_sandbox_denied,
|
||||
..
|
||||
} => Some(pending_sandbox_denied),
|
||||
_ => None,
|
||||
},
|
||||
);
|
||||
let exit_pending = pending_exit.is_some();
|
||||
if let Some(pending_sandbox_denied) = pending_exit {
|
||||
*pending_sandbox_denied =
|
||||
Some(pending_sandbox_denied.unwrap_or(false) || sandbox_denied);
|
||||
}
|
||||
let exit_known = ordered_events.exit_published || exit_pending;
|
||||
let event_count = target_seq - ordered_events.last_published_seq;
|
||||
let retained_count = ordered_events
|
||||
.pending
|
||||
@@ -123,9 +133,14 @@ impl SessionState {
|
||||
"recovering exited process did not include its exit code".to_string(),
|
||||
)
|
||||
})?;
|
||||
ordered_events
|
||||
.pending
|
||||
.insert(seq, ExecProcessEvent::Exited { seq, exit_code });
|
||||
ordered_events.pending.insert(
|
||||
seq,
|
||||
ExecProcessEvent::Exited {
|
||||
seq,
|
||||
exit_code,
|
||||
sandbox_denied: Some(sandbox_denied),
|
||||
},
|
||||
);
|
||||
} else if missing_count != 0 {
|
||||
return Err(recovery_gap_error(target_seq));
|
||||
}
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
use std::time::Duration;
|
||||
|
||||
use super::*;
|
||||
use crate::protocol::ExecOutputStream;
|
||||
use crate::protocol::ProcessOutputChunk;
|
||||
|
||||
fn registry_error(status: reqwest::StatusCode, code: Option<&str>) -> ExecServerError {
|
||||
ExecServerError::EnvironmentRegistryHttp {
|
||||
@@ -54,3 +56,43 @@ fn recovery_does_not_retry_other_registry_conflicts() {
|
||||
assert!(!is_retryable_registry_error(&error));
|
||||
assert!(!is_retryable_recovery_error(&error));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn recovery_adds_sandbox_denial_to_pending_exit_event() {
|
||||
let state = SessionState::new(/*recoverable*/ true);
|
||||
assert!(!state.publish_ordered_event(ExecProcessEvent::Exited {
|
||||
seq: 2,
|
||||
exit_code: 1,
|
||||
sandbox_denied: None,
|
||||
}));
|
||||
|
||||
state
|
||||
.recover_events(ReadResponse {
|
||||
chunks: vec![ProcessOutputChunk {
|
||||
seq: 1,
|
||||
stream: ExecOutputStream::Stderr,
|
||||
chunk: b"sandbox denied".to_vec().into(),
|
||||
}],
|
||||
next_seq: 3,
|
||||
exited: true,
|
||||
exit_code: Some(1),
|
||||
closed: false,
|
||||
failure: None,
|
||||
sandbox_denied: true,
|
||||
})
|
||||
.expect("recovery should publish the pending exit");
|
||||
|
||||
let mut events = state.subscribe_events();
|
||||
assert!(matches!(
|
||||
events.recv().await,
|
||||
Ok(ExecProcessEvent::Output(_))
|
||||
));
|
||||
assert_eq!(
|
||||
events.recv().await,
|
||||
Ok(ExecProcessEvent::Exited {
|
||||
seq: 2,
|
||||
exit_code: 1,
|
||||
sandbox_denied: Some(true),
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
@@ -876,13 +876,16 @@ async fn watch_exit(
|
||||
process.sandbox_denied = is_likely_sandbox_denied(process.sandbox, &exec_output);
|
||||
}
|
||||
let _ = process.wake_tx.send(seq);
|
||||
process
|
||||
.events
|
||||
.publish(ExecProcessEvent::Exited { seq, exit_code });
|
||||
process.events.publish(ExecProcessEvent::Exited {
|
||||
seq,
|
||||
exit_code,
|
||||
sandbox_denied: Some(process.sandbox_denied),
|
||||
});
|
||||
Some(ExecExitedNotification {
|
||||
process_id: process_id.clone(),
|
||||
seq,
|
||||
exit_code,
|
||||
sandbox_denied: Some(process.sandbox_denied),
|
||||
})
|
||||
} else {
|
||||
None
|
||||
|
||||
@@ -30,8 +30,14 @@ pub struct StartedExecProcess {
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub enum ExecProcessEvent {
|
||||
Output(ProcessOutputChunk),
|
||||
Exited { seq: u64, exit_code: i32 },
|
||||
Closed { seq: u64 },
|
||||
Exited {
|
||||
seq: u64,
|
||||
exit_code: i32,
|
||||
sandbox_denied: Option<bool>,
|
||||
},
|
||||
Closed {
|
||||
seq: u64,
|
||||
},
|
||||
Failed(String),
|
||||
}
|
||||
|
||||
@@ -125,21 +131,28 @@ impl ExecProcessEventLog {
|
||||
let live_rx = self.inner.live_tx.subscribe();
|
||||
let replay = history.events.iter().cloned().collect();
|
||||
|
||||
ExecProcessEventReceiver { replay, live_rx }
|
||||
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);
|
||||
let (live_tx, live_rx) = broadcast::channel(1);
|
||||
Self {
|
||||
replay: VecDeque::new(),
|
||||
live_rx,
|
||||
_keepalive: Some(live_tx),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -202,9 +215,21 @@ mod tests {
|
||||
|
||||
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);
|
||||
@@ -217,6 +242,7 @@ mod tests {
|
||||
log.publish(ExecProcessEvent::Exited {
|
||||
seq: 2,
|
||||
exit_code: 0,
|
||||
sandbox_denied: Some(false),
|
||||
});
|
||||
log.publish(ExecProcessEvent::Closed { seq: 3 });
|
||||
|
||||
@@ -238,6 +264,7 @@ mod tests {
|
||||
ExecProcessEvent::Exited {
|
||||
seq: 2,
|
||||
exit_code: 0,
|
||||
sandbox_denied: Some(false),
|
||||
},
|
||||
ExecProcessEvent::Closed { seq: 3 },
|
||||
]
|
||||
|
||||
Reference in New Issue
Block a user