fix(app-server): emit turn/started only when turn actually starts#13261
fix(app-server): emit turn/started only when turn actually starts#13261
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fc30b6d1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60918ab4ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| thread_watch_manager | ||
| .note_turn_started(&conversation_id.to_string()) | ||
| .await; | ||
| if let ApiVersion::V2 = api_version { |
There was a problem hiding this comment.
turn/started is now sent only behind if let ApiVersion::V2 = api_version in apply_bespoke_event_handling. The listener’s api_version is fixed at startup and not refreshed when an existing listener is reused (ensure_listener_task_running_task returns early if listener_matches). If a V1 listener exists first, later V2 turn/start calls on that thread can receive no turn/started notification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
don't care about this edge case
2 participants
This is a follow-up for #13047
Why
We had a race where
turn/startedcould be observed before the thread had actually transitioned toActive. This was because we eagerly emittedturn/startedin the request handler forturn/start(andreview/start).That was showing up as flaky
thread/resumetests, but the real issue was broader: a client could seeturn/startedand still get back an idle thread immediately afterward.The first idea was to eagerly call
thread_watch_manager.note_turn_started(...)from theturn/startrequest path. That turns out to be unsafe, becausesubmit(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/startedto emit only after the turn actually starts, so we do that by waiting for theEventMsg::TurnStartednotification emitted by codex core. We do this for bothturn/startandreview/start.I also verified this change is safe for our first-party codex apps - they don't have any assumptions that
turn/startedis emitted before the RPC response toturn/start(which is correct anyway).I also removed
single_client_modesince it isn't really necessary now.Testing
cargo test -p codex-app-server thread_resume -- --nocapturecargo test -p codex-app-server 'suite::v2::turn_start::turn_start_emits_notifications_and_accepts_model_override' -- --exact --nocapturecargo test -p codex-app-server