fix(pty): pace the EAGAIN write retry so a stalled reader can't saturate the daemon thread

[nireak]

ELI5

When Orca writes to a terminal whose program has stopped reading, the kernel says "not now, buffer is full" (EAGAIN). Today we immediately ask again — over 100,000 times a second — which pins a CPU core on the daemon thread that owns every terminal on the runtime. This waits 1 ms before asking again: ~150× less spinning, and the CPU goes from ~101% to ~4%.

What Changed

Two hunks in config/patches/node-pty@1.1.0.patch, applied to both the compiled lib/unixTerminal.js and its src/unixTerminal.ts twin, plus the resulting patch_hash in pnpm-lock.yaml.

+var EAGAIN_RETRY_DELAY_MS = 1;

   dispose(): void {
-    clearImmediate(this._writeImmediate);
+    clearTimeout(this._writeImmediate);

     if ('code' in err && err.code === 'EAGAIN') {
-      this._writeImmediate = setImmediate(() => this._processWriteQueue());
+      this._writeImmediate = setTimeout(() => this._processWriteQueue(), EAGAIN_RETRY_DELAY_MS);

clearImmediate → clearTimeout is required, not cosmetic: once the handle is a Timeout, clearImmediate does not cancel it (verified directly on node 24), so a pending retry can fire after dispose. The guards that make that harmless — _fd = -1 and dropping the queue — are already on main; this mirrors them into src/unixTerminal.ts, which had drifted from the compiled lib.

Why 1 ms

The retry delay is not free: it costs delivery time to any reader that is draining slowly rather than not at all. A real agent drains in bursts — hard during an event-loop tick, not at all while blocked — which lands squarely in that range. Delivering 2 MB to a reader that drains 20 ms out of every 100 ms, macOS arm64:

delay	delivery	CPU
main (setImmediate)	689 ms	103.5%
1 ms	907 ms	22.6%
5 ms	1414 ms	15.1%

And with the reader fully stopped (the pathological case this fixes), 8 s storm in isolation:

delay	EAGAIN/s	CPU
main	121,316	101.6%
1 ms	805	4.1%
2 ms	425	2.4%
5 ms	178	1.6%

1 ms takes essentially the whole CPU win without the delivery regression. Steady-rate sweeps agree: a reader draining at 17 MB/s or at 48 KB/s is unaffected by any delay value; only the ~0.2–1 MB/s band pays, and 5 ms costs 2.6× there where 1 ms costs ~4%.

Reproduced on Linux (Docker, native arm64 and emulated x64) with the same ordering; the Linux curve is flatter (main 43% CPU, 1 ms 19%, 5 ms 7%).

Scope — what this does not fix

It does not unfreeze other terminals. A second live pty doing continuous echo round-trips kept answering throughout the storm in every configuration tested — 1 and 8 stalled writers, macOS and Linux, 8 CPUs and --cpus=1. Throughput fell ~20% (one stalled writer) to ~50% (eight), and max event-loop lag stayed ≤140 ms. Nothing hung.

So this fixes the CPU saturation in #11178, not the reported "every terminal froze for ~17 minutes, no input accepted". #11178 should stay open. Two follow-ups worth filing separately: node-pty's write queue is unbounded while Orca allows 16 MB per send, so a wedged child silently accumulates memory in the daemon; and Orca has no "this terminal is not accepting input" signal, so a wedged agent is indistinguishable from a frozen Orca.

Upstream

setImmediate there is deliberate. In microsoft/node-pty #831/#833 the maintainer removed JS-side setTimeout throttling to fix large-paste corruption and a macOS hang (50 KB paste: ~5 s → ~500 ms), and explicitly planned to "move to setImmediate on EAGAIN". He also tried polling POLLOUT and abandoned it — it reports writable, not flushed, and node's fs.write goes through the libuv threadpool.

That reasoning targets a delay on every write in an interactive terminal. This delays only the EAGAIN branch, in a long-lived daemon whose children block for minutes at a time. Pastes to a draining reader are unaffected: 0–3 EAGAINs per MB in every arm, and a 50 KB paste to a fast reader lands in 3–6 ms regardless of delay.

microsoft/node-pty#956 carries the same idea at 5 ms; it is CLA-blocked and unreviewed.

Testing

Both arms built from the pristine node-pty@1.1.0 tarball with the respective patch revisions applied via git apply, sharing one native binary; EAGAINs counted by wrapping fs.write before requiring node-pty, so the code under test is unmodified. Harness scenarios: stalled reader, slow steady reader (drain-rate sweep), bursty reader, fast-reader paste, live-victim terminal, disposal race.

Disposal, destroy() mid-storm: pristine node-pty leaves 1 post-dispose write (EBADF); main and this branch leave 0, with 341 EAGAINs before disposal so the window was exercised.

Patch mechanics, since pnpm applies this file rather than CI compiling it:

• applies to a pristine node-pty@1.1.0 tarball; every section other than the two touched files is byte-identical to main's patch
• patched src/unixTerminal.ts compiled with node-pty's own TypeScript 3.8.3 is byte-identical to the patched lib/unixTerminal.js
• patch_hash in pnpm-lock.yaml is the sha256 of the patch file, and pnpm install applied it end-to-end (node_modules shows EAGAIN_RETRY_DELAY_MS = 1 in both JS and TS)

Platforms. macOS arm64 and Linux (arm64 + x64) measured. Windows verified on a real machine: the patch applies under core.autocrlf=true with a byte-identical hash (.gitattributes pins -text), windowsTerminal.js references neither CustomWriteStream nor unixTerminal, and a live conpty run reports WindowsTerminal with 0 EAGAINs on a 300 KB write. The changed code provably never executes there.

• I manually tested these changes locally
• Automated tests added/updated — no. The change lives inside a patched dependency and reproducing it needs a stalled raw-mode pty plus sustained backpressure; that test asserts on the mechanism and tends to flake. Happy to add one if a reviewer wants it.

AI Disclosure

Diagnosis, patch, and this description were produced with Claude Opus (Claude Code). The measurements are real runs.

Checklist

• This PR is small and focused
• I explained what changed and why (including ELI5)
• Before/after screenshots or videos — N/A, no UI surface; the observable change is CPU behaviour, quantified above
• Self-reviewed for correctness, security, and performance
• Cross-platform, SSH/remote, and path/shortcut impact considered — unix-only code path, Windows verified unaffected, no transport or protocol change

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: c53144ef-efe8-4a8d-a134-8beffe0cab01

📥 Commits

Reviewing files that changed from the base of the PR and between 4fc429e95853f51efd484b99e451c932389a1103 and 26cbdda.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (1)

• config/patches/node-pty@1.1.0.patch

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

The node-pty patch updates Linux and Windows build compatibility. It adds glibc symbol pins, close-on-exec handling, and guarded PTY cleanup. Unix writes use timed EAGAIN retries and retired file descriptors. ConPTY gains fallback enumeration, synchronized job management, and lifecycle controls. Windows socket errors trigger terminal cleanup. macOS PTY spawning reports structured operation-specific errors.

Priority: ⬆️ High

Severity of issue fixed: High

Merge Risk: ⚪ Minimal · up to 26cbd

The patched retry behavior consistently uses the intended 1 ms backoff in both source and shipped code. No actionable current-head risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is detailed and explains the change, rationale, testing, platform impact, AI use, and checklist status. However, it omits the required Linked Issue section and issue reference, and it …	Add the required template sections, especially Linked Issue with the applicable issue reference. Resolve the 1 ms versus 5 ms delay discrepancy so the description matches the implemented change and PR objectives. Complete or explicitly mark…

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Issue #11178 requires the daemon to stop immediate EAGAIN retries. The PR changes the Unix retry path from setImmediate to a 5 ms setTimeout, which provides bounded backoff. Disposal guards preven…
Out of Scope Changes check	✅ Passed	The reviewed PR scope is the patched node-pty@1.1.0 Unix write-retry behavior, disposal handling, generated Unix terminal files, and the patch hash. The reported Spectre concern and other existing p…
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Title check	✅ Passed	The title clearly identifies the primary change: pacing EAGAIN write retries to prevent daemon-thread CPU saturation.

Full details: Description check

Explanation

The description is detailed and explains the change, rationale, testing, platform impact, AI use, and checklist status. However, it omits the required Linked Issue section and issue reference, and it does not provide the required Review, Agent skill upstream boundary, or Notes sections. The stated 1 ms delay also conflicts with the objective summary, which describes a 5 ms delay.

Resolution

Add the required template sections, especially Linked Issue with the applicable issue reference. Resolve the 1 ms versus 5 ms delay discrepancy so the description matches the implemented change and PR objectives. Complete or explicitly mark the remaining required sections as applicable or not applicable.

• Fix all pre-merge checks with AI

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

config/patches/node-pty@1.1.0.patch (1)

9-11: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Restore the Windows Spectre mitigation setting

Restore SpectreMitigation: 'Spectre' in binding.gyp and both Windows targets in deps/winpty/src/winpty.gyp.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ea8b7170-10ee-4cf5-8783-578ff792b383

📥 Commits

Reviewing files that changed from the base of the PR and between 2f0f9a8 and b2c77b7d891255397a0c79276ef8f1ae89c7e8ab.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (1)

• config/patches/node-pty@1.1.0.patch

Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review.

[nireak]

Thanks — one of these is a real bug and I've fixed it. The other is outside this diff. I also have a correction to make to my own numbers.

1. Disposal race — valid, fixed

You're right, and it's worse than the note I left in #11178. I'd only spotted that clearImmediate wouldn't cancel a Timeout; the actual hazard is that an in-flight fs.write re-arms the retry after dispose() has already run, so clearing the handle doesn't help.

I reproduced it before fixing it. Instrumenting fs.write and calling destroy() mid-EAGAIN-storm:

	writes after dispose	result
stock node-pty@1.1.0 (setImmediate)	1	EBADF
this PR, before your comment (setTimeout)	1	EBADF
this PR, with the guard	0	—

(329 EAGAINs before disposal in the guarded run, so the race window was genuinely exercised rather than skipped.)

Two things worth recording: the race is pre-existing — stock node-pty does it too, identically — but this PR widens the window, because a 5 ms retry gives the fd far more time to be closed than a next-tick setImmediate did. And EBADF is the benign outcome; the one that worries me is fd recycling, since the daemon owns every PTY on the runtime and churns descriptors constantly. Writing terminal bytes into a recycled descriptor would be silent.

Fixed as you suggested, in both lib/unixTerminal.js and src/unixTerminal.ts: _isDisposed set in dispose(), checked at the top of _processWriteQueue() and at the top of the fs.write callback. The lib/ hunk is not hand-written — I regenerate it with tsc -b ./src/tsconfig.json and diff it against the patched source to confirm they match byte-for-byte.

One judgement call I did not make silently: dispose() still leaves _writeQueue populated, so a write() after disposal enqueues data that will never drain. Pre-existing, harmless in practice, and clearing it changes disposal semantics — happy to add this._writeQueue.length = 0 to dispose() if you'd prefer.

2. Spectre mitigation — pre-existing, not part of this change

This one is a false positive for this PR. Lines 1–30 of config/patches/node-pty@1.1.0.patch are byte-identical to main — the SpectreMitigation removal is existing Orca patch content that predates this branch. My diff only adds hunks for lib/unixTerminal.js and a new src/unixTerminal.ts section; it touches nothing in binding.gyp or winpty.gyp.

The bot appears to be reviewing the whole patch file rather than the diff, which is an easy trap with .patch files.

Whether that mitigation should be restored is a fair question for the owners of that patch, but it's not something this PR should change silently while fixing an unrelated spin.

3. Correction to my own numbers

While re-testing I found an error in my original description, and I'd rather flag it than let it stand.

I claimed the fix writes "11.5× more bytes". That is wrong — it came from a single run, and repeat trials do not reproduce it. Bytes written in 12 s lands between 65 536 and 393 216 in both arms, with no systematic difference. I've struck that claim from the description.

The real result holds up, and here it is across trials rather than from one run:

	baseline (6 trials)	patched (5 trials)
avg EAGAIN/s	49 160 – 120 608	175 – 178
peak CPU	85 – 101%	3 – 18% (steady 1–2%)
bytes written	65 536 – 393 216	65 536 – 393 216 (no difference)

So the reduction is ~280–690× depending on how loaded the machine is, not the single 685× figure I first quoted. The baseline is noisier than I represented; the patched arm is remarkably stable.

⚠️ Also worth knowing if you try to reproduce: the harness needs enough queued data. At 4 MB the baseline reports a clean "not wedged" having never filled the buffer at all — check the EAGAIN total is non-zero before trusting a pass. 128 MB was reliable here.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 49e2ae95-8a8b-4d6a-b8e0-55c052d6bacc

📥 Commits

Reviewing files that changed from the base of the PR and between b025367 and 0d7d9a5fbf58aba679e0ba3fa63df69de736e8fb.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (1)

• config/patches/node-pty@1.1.0.patch

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

[nwparker]

Merged as b87a6c0f23b. Thanks for this — the diagnosis was right and the syscall-level capture in #11178 made it easy to confirm.

I changed two things before merging, so flagging them rather than letting you find them in the log.

The delay is 1 ms, not 5 ms

I built a harness against real node-pty@1.1.0 (both arms from the pristine tarball with the respective patch revisions applied, same native binary, EAGAINs counted by wrapping fs.write before requiring node-pty) and swept the constant. Your stalled-reader numbers reproduce cleanly. But the delay also costs delivery time to a reader that is draining slowly rather than not at all — and a real agent drains in bursts, hard during an event-loop tick and not at all while blocked, which lands right in that band.

2 MB to a reader that drains 20 ms out of every 100 ms, macOS arm64:

delay	delivery	CPU
main (setImmediate)	689 ms	103.5%
1 ms	907 ms	22.6%
5 ms	1414 ms	15.1%

And fully stalled, 8 s storm in isolation: main 121,316 EAGAIN/s @ 101.6% CPU; 1 ms 805/s @ 4.1%; 5 ms 178/s @ 1.6%.

So 5 ms buys a few more points of CPU on a case that 1 ms has already fixed, and pays for it with 2–3× slower prompt delivery on the case every busy agent hits constantly. A steady-rate sweep confirms the penalty band is narrow — a reader draining at 17 MB/s or at 48 KB/s is unaffected at any delay — but bursty readers sit inside it.

Also worth knowing for your upstream PR: microsoft/node-pty#833 shows Tyriar chose setImmediate deliberately, removing a setTimeout to fix large-paste latency, and rejected polling POLLOUT because it reports writable, not flushed, with fs.write going through the libuv threadpool. A 5 ms delay is likelier to be read there as a partial revert of that work. 1 ms is the more defensible ask. (Separately: #956 is still blocked on the CLA bot, so no maintainer has looked at it yet.)

The description was rewritten

Three claims didn't survive verification:

• It does not unfreeze other terminals. A second live pty doing continuous echo round-trips kept answering throughout the storm in every configuration I tried — 1 and 8 stalled writers, macOS and Linux, 8 CPUs and --cpus=1. Throughput fell 20–50%; nothing hung. setImmediate yields to the loop each iteration, so it burns a core without starving I/O. Daemon can wedge while every liveness signal reports healthy, and a restart readopts it (EAGAIN busy-wait half fixed in #15319) #11178 has been retitled and stays open for the freeze and the wedged-daemon-readoption halves.
• The disposal guards were already on main. The _isDisposed field described in the original body isn't what shipped; _fd = -1 plus the queue drop were already there. What this PR adds is mirroring them into src/unixTerminal.ts, which had drifted from the compiled lib.
• The comment overclaimed. "Interactive writes do not reach the EAGAIN branch at all" isn't true on macOS — a 50 KB paste to a slow reader reaches it. Reworded, and the new comment records why the constant is 1 ms so a future node-pty bump doesn't silently revert it.

Everything else held up, including all four patch-mechanics checks. I added Windows verification on a real box: the changed code never executes there (WindowsTerminal, 0 EAGAINs on a 300 KB conpty write), and the patch applies under core.autocrlf=true with a byte-identical hash.

Your commit authorship is preserved on the squash. The original 5 ms revision is still at 4fc429e9585 if you want to compare. Happy to be argued out of 1 ms if you have a workload where the extra CPU matters — the harness is reproducible and I'll re-run it against whatever shape you've got.