mirror of
https://github.com/openai/codex.git
synced 2026-10-28 18:58:46 +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.
99 lines
2.8 KiB
Rust
99 lines
2.8 KiB
Rust
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 {
|
|
status,
|
|
code: code.map(str::to_string),
|
|
message: "registry unavailable".to_string(),
|
|
}
|
|
}
|
|
|
|
#[test]
|
|
fn registry_recovery_retry_delay_exponentially_backs_off_and_caps() {
|
|
let cases = [
|
|
(0, Duration::from_millis(500)),
|
|
(1, Duration::from_secs(1)),
|
|
(2, Duration::from_secs(2)),
|
|
(3, Duration::from_secs(4)),
|
|
(4, Duration::from_secs(5)),
|
|
(20, Duration::from_secs(5)),
|
|
];
|
|
|
|
for (attempt, base) in cases {
|
|
let delay = registry_recovery_retry_delay("session-1", attempt);
|
|
assert!(delay >= base, "delay {delay:?} for attempt {attempt}");
|
|
assert!(
|
|
delay <= base + base / 2,
|
|
"delay {delay:?} for attempt {attempt}"
|
|
);
|
|
}
|
|
}
|
|
|
|
#[test]
|
|
fn recovery_retries_transient_registry_errors() {
|
|
let error = registry_error(reqwest::StatusCode::TOO_MANY_REQUESTS, /*code*/ None);
|
|
|
|
assert!(is_retryable_registry_error(&error));
|
|
assert!(is_retryable_recovery_error(&error));
|
|
}
|
|
|
|
#[test]
|
|
fn recovery_retries_environment_offline_conflicts() {
|
|
let error = registry_error(reqwest::StatusCode::CONFLICT, Some("environment_offline"));
|
|
|
|
assert!(is_retryable_registry_error(&error));
|
|
assert!(is_retryable_recovery_error(&error));
|
|
}
|
|
|
|
#[test]
|
|
fn recovery_does_not_retry_other_registry_conflicts() {
|
|
let error = registry_error(reqwest::StatusCode::CONFLICT, Some("registration_conflict"));
|
|
|
|
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),
|
|
})
|
|
);
|
|
}
|