fix(unix): pace the EAGAIN write retry so a stalled reader can't saturate the thread - #956
Open
nireak (nireak) wants to merge 1 commit into
Open
fix(unix): pace the EAGAIN write retry so a stalled reader can't saturate the thread#956nireak (nireak) wants to merge 1 commit into
nireak (nireak) wants to merge 1 commit into
Conversation
…rate the thread CustomWriteStream retries an EAGAIN write with setImmediate, which re-attempts within microseconds. A pty whose reader has stopped draining keeps that branch failing, so the retry becomes an unbounded busy-loop on the thread it runs on. In an embedder multiplexing many PTYs onto one process, every terminal in that process freezes while the machine itself looks healthy: observed in production as ~143,000 EAGAIN/s and a pinned core. This does not undo microsoft#833. Measured against an actively draining reader at 4/32/64 MB, EAGAIN never fires at any volume -- a merely slow reader backpressures through the normal completion path, and the kernel only returns EAGAIN once the buffer is full and stays full. The paced branch therefore only executes when throughput is already zero, and the fast path is untouched. Also fixes a pre-existing disposal race that the delay widens: an fs.write already in flight when dispose() runs still invokes its callback, and on EAGAIN that callback re-arms the retry against a closed fd. Track disposal explicitly and check it before scheduling or processing further writes. Both behaviours are covered by tests that fail without this change: 3244 retry attempts per 100ms becomes ~20, and the post-dispose write is caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
nireak (@nireak) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
CustomWriteStream._processWriteQueue()retries anEAGAINwrite withsetImmediate, which re-attempts within microseconds:https://github.com/microsoft/node-pty/blob/main/src/unixTerminal.ts#L362
When a pty's reader stops draining — a raw-mode child that has stopped reading — that branch never clears, so the retry becomes an unbounded busy-loop that saturates the thread it runs on.
For an embedder that multiplexes many PTYs onto one process this is not a local problem: every terminal in the process freezes — no output, no input — while the host machine looks perfectly healthy. That is how we found it. A terminal daemon pinned a core at 100% twice in one hour with ~143,000 EAGAIN/s in the syscall trace, and every session on it locked up for ~17 minutes.
Why this doesn't undo #833
#833 removed
setTimeouts from the backpressure path for throughput, and I did not want to quietly put one back. So I measured whether the EAGAIN branch is on that path at all.Writing to a pty whose child actively drains, on macOS arm64:
EAGAINnever fired once while a reader was draining, at any volume I tried. A merely-slow reader applies backpressure through the normal completion path; the kernel only returnsEAGAINonce the buffer is full and stays full, which requires the reader to have stopped. Throughput differences above are run-to-run noise (the baseline itself varies by more than that between runs).So this paces a branch that only executes when throughput is already zero because nobody is reading. The fast path — write completes, queue advances from the completion callback — is untouched.
Caveat stated plainly: those measurements are macOS arm64 only. I have not measured Linux, where buffer sizing and
EAGAINbehaviour could differ.The change
Renamed the field since it no longer holds an
Immediate.dispose()correspondingly usesclearTimeout.Second, related fix:
dispose()clearing the handle is not sufficient on its own. Anfs.writealready in flight whendispose()runs still invokes its completion callback afterwards, and onEAGAINthat callback schedules a fresh retry — writing to an fd that is now closed. This is pre-existing (setImmediatedoes it too), but a delayed retry widens the window, so it belongs in the same change. Disposal is now tracked explicitly and checked in_processWriteQueue()and at the top of the completion callback.EBADFis the benign outcome of that race. The one that concerns me is descriptor recycling — in a process that opens and closes PTYs continuously, a recycled fd would accept the write silently.Waiting for actual writability (
kqueue/epollon the master fd) remains the fully correct fix, and is what #833 gestures at with "this likely needs a C++ solution likepoll". This is the small version that stops the pathology in the meantime.Tests
Two tests added to
src/unixTerminal.test.ts, both verified to fail without the fix:They stub
fs.writeto returnEAGAINdeterministically rather than trying to starve a real pty, so they are fast and not timing-flaky.End-to-end against a real stalled pty (128 MB queued, 6 baseline / 5 patched trials): avg 49,160 – 120,608 EAGAIN/s → 175 – 178, peak CPU 85 – 101% → 3 – 18%.
npm run lintis clean.npm teston this machine (macOS 26.3, node 24, arm64) is already red before my change — 17–18 passing, 8–9 failing, and flaky run to run. The failures are all insignals in parent and childandspawn(process.killreceiving a NaN pid, and the fd-leak tests). Across four runs with my change the failing set is identical and my two new tests never appear in it; the passing count rises by exactly 2. I have not tried to fix those pre-existing failures here.Related