Conversation
8dce026 to
845d3dc
Compare
0af69cf to
391565f
Compare
433eaa2 to
9ce54f6
Compare
| let (outgoing_tx, mut outgoing_rx) = mpsc::channel::<OutgoingEnvelope>(channel_capacity); | ||
| let outgoing_message_sender = Arc::new(OutgoingMessageSender::new(outgoing_tx)); | ||
| let auth_manager = | ||
| AuthManager::shared_from_config(args.config.as_ref(), args.enable_codex_api_key_env); |
There was a problem hiding this comment.
don't we have an auth_manager on InProcessStartArgs?
254f07f to
32d2d05
Compare
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/app-server/src/outgoing_message.rs
Lines 357 to 363 in 840f271
replay_requests_to_connection_for_thread resends pending ServerRequests to a rejoined connection but does not call track_server_request. If that connection responds, notify_client_response records a ServerResponse for the new connection ID without a corresponding request fact, breaking per-connection request/response correlation.
ℹ️ 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".
| None => { | ||
| self.sender |
There was a problem hiding this comment.
Track broadcast server requests before dispatching them
send_request_to_connections only records track_server_request in the Some(connection_ids) branch. The broadcast branch (None) sends the request but never emits a request fact. Paths like ExternalAuthRefreshBridge::refresh use broadcast, so notify_client_response can emit a ServerResponse with no matching ServerRequest, yielding incomplete analytics.
Useful? React with 👍 / 👎.
| AnalyticsFact::ServerRequest { | ||
| connection_id: _connection_id, | ||
| request: _request, | ||
| } => {} | ||
| AnalyticsFact::ServerResponse { | ||
| connection_id: _connection_id, | ||
| response: _response, | ||
| } => {} |
There was a problem hiding this comment.
Process server request/response facts in the reducer
OutgoingMessageSender now emits AnalyticsFact::ServerRequest/ServerResponse, but the reducer drops both variants with empty arms. The added instrumentation therefore produces no analytics events; under load these no-op facts still consume slots in the bounded queue and can increase drops of meaningful events.
Useful? React with 👍 / 👎.
|
|
||
| match entry { | ||
| Some((id, entry)) => { | ||
| if let Ok(response) = entry.request.response_from_result(result.clone()) { |
There was a problem hiding this comment.
Eliminate full JSON clone on server-response tracking
notify_client_response clones result before deserializing it into a typed ServerResponse, then still forwards the original result to the waiting callback. For large payloads (for example dynamic tool call outputs), this doubles JSON allocations and adds extra parse CPU on a hot response path.
Useful? React with 👍 / 👎.
Stack created with Sapling. Best reviewed with ReviewStack.