Conversation
|
@codex review |
![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: aac057d136
ℹ️ 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-rs/network-proxy/src/proxy.rs
Outdated
| Ok(listener) => listener | ||
| .local_addr() | ||
| .context("failed to get loopback ephemeral port"), |
There was a problem hiding this comment.
build() requests three "ephemeral" addresses and later starts separate listeners on them, but reserve_loopback_ephemeral_addr() drops its TcpListener immediately after calling local_addr(). Because the port is released before run() binds, the OS can hand out the same port twice (or another process can claim it), which causes intermittent EADDRINUSE when HTTP/admin/SOCKS tasks start and can leave the proxy partially broken.
Useful? React with 👍 / 👎.
bolinfest
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90e4dce3f7
ℹ️ 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".
| if self.managed_by_codex { | ||
| let (http_listener, socks_listener, admin_listener) = | ||
| reserve_loopback_ephemeral_listeners() | ||
| .context("reserve managed loopback proxy listeners")?; |
There was a problem hiding this comment.
In managed mode, build() unconditionally calls reserve_loopback_ephemeral_listeners() and binds all three sockets before any network.enabled or enable_socks5 gating happens, so configs that should effectively disable parts of the proxy can still fail at build time (for example with Operation not permitted under restricted sandbox policies) even though run() would no-op for disabled networking. This regresses startup robustness and should be guarded so disabled listeners are not bound.
Useful? React with 👍 / 👎.
2 participants
Codex may run many per-thread proxy instances, so hardcoded proxy ports are brittle and conflict-prone. The previous "ephemeral" approach still had a race:
build()readlocal_addr()from temporary listeners and dropped them beforerun()rebound the ports. That left a TOCTOU window where the OS (or another process) could reuse the same port, causing intermittentEADDRINUSEand partial proxy startup.Change the managed proxy path to reserve real listener sockets up front and keep them alive until startup:
ReservedListenersonNetworkProxyto hold HTTP/SOCKS/admin std listeners allocated duringbuild()127.0.0.1:0for each listener and carry those bound sockets intorun()instead of rebinding by address laterrun_*_with_std_listenerentry points for HTTP, SOCKS5, and admin servers sorun()can start services from already-reserved socketsmanaged_by_codex(false), including explicitsocks_addroverride supportThis makes managed startup deterministic, avoids port collisions, and preserves the intended distinction between Codex-managed ephemeral ports and externally managed fixed ports.