mirror of
https://github.com/openai/codex.git
synced 2026-04-28 10:21:06 +03:00
fix(app-server): emit turn/started only when turn actually starts (#13261)
This is a follow-up for https://github.com/openai/codex/pull/13047 ## Why We had a race where `turn/started` could be observed before the thread had actually transitioned to `Active`. This was because we eagerly emitted `turn/started` in the request handler for `turn/start` (and `review/start`). That was showing up as flaky `thread/resume` tests, but the real issue was broader: a client could see `turn/started` and still get back an idle thread immediately afterward. The first idea was to eagerly call `thread_watch_manager.note_turn_started(...)` from the `turn/start` request path. That turns out to be unsafe, because `submit(Op::UserInput)` only queues work. If a turn starts and completes quickly, request-path bookkeeping can race with the real lifecycle events and leave stale running state behind. **The real fix** is to move `turn/started` to emit only after the turn _actually_ starts, so we do that by waiting for the `EventMsg::TurnStarted` notification emitted by codex core. We do this for both `turn/start` and `review/start`. I also verified this change is safe for our first-party codex apps - they don't have any assumptions that `turn/started` is emitted before the RPC response to `turn/start` (which is correct anyway). I also removed `single_client_mode` since it isn't really necessary now. ## Testing - `cargo test -p codex-app-server thread_resume -- --nocapture` - `cargo test -p codex-app-server 'suite::v2::turn_start::turn_start_emits_notifications_and_accepts_model_override' -- --exact --nocapture` - `cargo test -p codex-app-server`
This commit is contained in:
@@ -1,4 +1,3 @@
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use app_test_support::McpProcess;
|
||||
use app_test_support::create_apply_patch_sse_response;
|
||||
@@ -17,7 +16,6 @@ use codex_app_server_protocol::FileChangeApprovalDecision;
|
||||
use codex_app_server_protocol::FileChangeRequestApprovalResponse;
|
||||
use codex_app_server_protocol::ItemStartedNotification;
|
||||
use codex_app_server_protocol::JSONRPCError;
|
||||
use codex_app_server_protocol::JSONRPCNotification;
|
||||
use codex_app_server_protocol::JSONRPCResponse;
|
||||
use codex_app_server_protocol::PatchApplyStatus;
|
||||
use codex_app_server_protocol::PatchChangeKind;
|
||||
@@ -30,7 +28,6 @@ use codex_app_server_protocol::ThreadResumeResponse;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_app_server_protocol::ThreadStartResponse;
|
||||
use codex_app_server_protocol::ThreadStatus;
|
||||
use codex_app_server_protocol::ThreadStatusChangedNotification;
|
||||
use codex_app_server_protocol::TurnStartParams;
|
||||
use codex_app_server_protocol::TurnStartResponse;
|
||||
use codex_app_server_protocol::TurnStatus;
|
||||
@@ -293,7 +290,7 @@ async fn thread_resume_keeps_in_flight_turn_streaming() -> Result<()> {
|
||||
.await??;
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
wait_for_thread_status_active(&mut primary, &thread.id),
|
||||
primary.read_stream_until_notification_message("turn/started"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
@@ -400,7 +397,7 @@ async fn thread_resume_rejects_history_when_thread_is_running() -> Result<()> {
|
||||
to_response::<TurnStartResponse>(running_turn_resp)?;
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
wait_for_thread_status_active(&mut primary, &thread_id),
|
||||
primary.read_stream_until_notification_message("turn/started"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
@@ -516,7 +513,7 @@ async fn thread_resume_rejects_mismatched_path_when_thread_is_running() -> Resul
|
||||
to_response::<TurnStartResponse>(running_turn_resp)?;
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
wait_for_thread_status_active(&mut primary, &thread_id),
|
||||
primary.read_stream_until_notification_message("turn/started"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
@@ -619,7 +616,7 @@ async fn thread_resume_rejoins_running_thread_even_with_override_mismatch() -> R
|
||||
.await??;
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
wait_for_thread_status_active(&mut primary, &thread.id),
|
||||
primary.read_stream_until_notification_message("turn/started"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
@@ -1419,30 +1416,6 @@ required = true
|
||||
)
|
||||
}
|
||||
|
||||
async fn wait_for_thread_status_active(
|
||||
mcp: &mut McpProcess,
|
||||
thread_id: &str,
|
||||
) -> Result<ThreadStatusChangedNotification> {
|
||||
loop {
|
||||
let status_changed_notif: JSONRPCNotification = mcp
|
||||
.read_stream_until_notification_message("thread/status/changed")
|
||||
.await?;
|
||||
let status_changed_params = status_changed_notif
|
||||
.params
|
||||
.context("thread/status/changed params must be present")?;
|
||||
let status_changed: ThreadStatusChangedNotification =
|
||||
serde_json::from_value(status_changed_params)?;
|
||||
if status_changed.thread_id == thread_id
|
||||
&& status_changed.status
|
||||
== (ThreadStatus::Active {
|
||||
active_flags: Vec::new(),
|
||||
})
|
||||
{
|
||||
return Ok(status_changed);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[allow(dead_code)]
|
||||
fn set_rollout_mtime(path: &Path, updated_at_rfc3339: &str) -> Result<()> {
|
||||
let parsed = chrono::DateTime::parse_from_rfc3339(updated_at_rfc3339)?.with_timezone(&Utc);
|
||||
|
||||
@@ -434,6 +434,21 @@ async fn turn_start_emits_notifications_and_accepts_model_override() -> Result<(
|
||||
started.turn.status,
|
||||
codex_app_server_protocol::TurnStatus::InProgress
|
||||
);
|
||||
assert_eq!(started.turn.id, turn.id);
|
||||
|
||||
let completed_notif: JSONRPCNotification = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("turn/completed"),
|
||||
)
|
||||
.await??;
|
||||
let completed: TurnCompletedNotification = serde_json::from_value(
|
||||
completed_notif
|
||||
.params
|
||||
.expect("turn/completed params must be present"),
|
||||
)?;
|
||||
assert_eq!(completed.thread_id, thread.id);
|
||||
assert_eq!(completed.turn.id, turn.id);
|
||||
assert_eq!(completed.turn.status, TurnStatus::Completed);
|
||||
|
||||
// Send a second turn that exercises the overrides path: change the model.
|
||||
let turn_req2 = mcp
|
||||
@@ -457,25 +472,30 @@ async fn turn_start_emits_notifications_and_accepts_model_override() -> Result<(
|
||||
// Ensure the second turn has a different id than the first.
|
||||
assert_ne!(turn.id, turn2.id);
|
||||
|
||||
// Expect a second turn/started notification as well.
|
||||
let _notif2: JSONRPCNotification = timeout(
|
||||
let notif2: JSONRPCNotification = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("turn/started"),
|
||||
)
|
||||
.await??;
|
||||
let started2: TurnStartedNotification =
|
||||
serde_json::from_value(notif2.params.expect("params must be present"))?;
|
||||
assert_eq!(started2.thread_id, thread.id);
|
||||
assert_eq!(started2.turn.id, turn2.id);
|
||||
assert_eq!(started2.turn.status, TurnStatus::InProgress);
|
||||
|
||||
let completed_notif: JSONRPCNotification = timeout(
|
||||
let completed_notif2: JSONRPCNotification = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("turn/completed"),
|
||||
)
|
||||
.await??;
|
||||
let completed: TurnCompletedNotification = serde_json::from_value(
|
||||
completed_notif
|
||||
let completed2: TurnCompletedNotification = serde_json::from_value(
|
||||
completed_notif2
|
||||
.params
|
||||
.expect("turn/completed params must be present"),
|
||||
)?;
|
||||
assert_eq!(completed.thread_id, thread.id);
|
||||
assert_eq!(completed.turn.status, TurnStatus::Completed);
|
||||
assert_eq!(completed2.thread_id, thread.id);
|
||||
assert_eq!(completed2.turn.id, turn2.id);
|
||||
assert_eq!(completed2.turn.status, TurnStatus::Completed);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user