Fix Codex RPC timeout classification - #3025
Conversation
Co-authored-by: Alec Gutman <chipagosfinest@users.noreply.github.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: Fix Codex RPC timeout classification This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
![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: c89d523154
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try await withThrowingTaskGroup(of: RPCRequestRaceResult<T>.self) { group in | ||
| group.addTask { | ||
| try await body() | ||
| try await .value(body()) | ||
| } | ||
| group.addTask { [weak self] in | ||
| group.addTask { | ||
| try await Task.sleep(for: .seconds(seconds)) | ||
| self?.terminateProcessForTimeout(method: method) | ||
| throw RPCWireError.timeout(method: method) | ||
| return .timedOut |
There was a problem hiding this comment.
This changes the concurrency ordering that distinguishes RPCWireError.timeout from the prior malformed-EOF result, but the commit only adjusts an architecture-test line number; the existing Codex RPC tests never stall a request and assert the resulting error classification. Add a synthetic app-server test that withholds a reply until the deadline and verifies that RPCWireError.timeout wins, otherwise the specific race fixed here can regress unnoticed.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
2 participants
Summary
UsageFetcher.swiftprovider-architecture gatekeeper anchor after the merged line shiftReview
No blocking correctness issues found. The timer child now returns a typed timeout result; the parent selects it, cancels the sibling, then requests subprocess termination. The throwing task-group scope still drains the response child during unwinding, and the existing deferred shutdown remains the synchronous teardown backstop.
Supersedes #3022 while preserving @Chipagosfinest's commit and contributor credit.
Verification
swiftformat Sources Testsswiftlint --strictmake checkswift test --filter 'ProviderArchitectureGatekeeper|CodexRPC|UsageFetcher'(758 tests in 54 suites passed)