Reapply "Add app-server transport layer with websocket support" #11370
Conversation
maxj-oai
changed the title
maxj-oai
force-pushed
the
maxj/app-server-split-loops
branch
from
638b825 to
7560a70
Compare
![chatgpt-codex-connector[bot]](https://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: 7560a70d4e
ℹ️ 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".
…-loops # Conflicts: # codex-rs/Cargo.lock
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
This transfer the back pressure to the ingress in practice so it means we rely on the ingress to error/drop events once everything is full.
I would prefer an explicit handling on our side instead but this can be done in a follow-up
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
I think we lost optOutNotificationMethods (we should have a use of should_skip_notification_for_connection). Maybe it's just because we need to merge main
The rest lgtm but I guess we will iterate on this code. It's getting quite complex
3 participants
Reapply "Add app-server transport layer with websocket support" with additional fixes from https://github.com/openai/codex/pull/11313/changes to avoid deadlocking.
This reverts commit 47356ff.
Summary
To avoid deadlocking when queues are full, we maintain separate tokio tasks dedicated to incoming vs outgoing event handling
run_main_with_transporttransport_event_rx)outgoing_rx+thread_created_rx)Validation
Integration tests, testing thoroughly e2e in codex app w/ >10 concurrent requests