fix(stdin): retry transient nonblocking reads - #713
Conversation
![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: 88dbb5ed13
ℹ️ 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".
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The retry budget is cumulative rather than consecutive here. transientAttempts and retryDelayMs never reset after a successful readSync, so a long pipe that makes progress between sporadic EAGAINs still fails on the Nth transient error, and later retries stay at the max backoff. I think both should reset after bytesRead > 0; a data → EAGAIN → data loop repeated past maxAttempts would pin it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdba42d1a1
ℹ️ 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".
| transientAttempts += 1; | ||
| if (!isTransientReadError(error) || transientAttempts >= maxAttempts) { | ||
| throw error; |
There was a problem hiding this comment.
When setBlocking(true) is unavailable and a producer leaves its pipe open without supplying the next chunk for about 1.13 seconds, the eighth EAGAIN is thrown even though it is neither EOF nor an invalid descriptor. For example, a delayed or intermittently streaming piped prompt will fail instead of being read in full; the normal blocking-read behavior has no equivalent timeout. Continue waiting until data or EOF arrives (or expose an explicit caller timeout) rather than treating a fixed retry count as a read failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in af3e94b. Transient EAGAIN/EWOULDBLOCK no longer has an attempt ceiling while the pipe remains open; the loop waits with capped backoff until data or EOF, and still throws non-transient errors immediately. Added a 20-consecutive-transient regression before eventual progress.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Summary
EAGAIN/EWOULDBLOCKreads with bounded synchronous backoffVerification
git diff --checkpassFixes #247