Repository navigation
Conversation
…ge a PTY Resume is driven by renderer ACKs draining pending below LOW. If those ACKs stop arriving, pending never drains and the pause is held indefinitely. The daemon path survives this on its own 5s lost-resume failsafe, but LocalPtyProvider.pauseProducer is a bare node-pty pause() with no failsafe, so the child stays blocked on write until the window is destroyed — a live agent TUI frozen mid-turn with only Cmd+Shift+R (releaseAll) to recover. Cap one continuous paused span at 10s. A still-flooded pending re-pauses on the next report, so sustained backpressure is unchanged; only the unbounded hold is removed. The ceiling measures from the first pause of a span and is not extended by a re-assert, or a sustained flood would never release.
Greptile SummaryThe PR bounds each continuous producer pause so a lost renderer acknowledgement cannot leave a PTY indefinitely wedged.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/main/ipc/pty-producer-flow-control.ts | Adds bounded pause lifetime and complete timer cleanup; the previously unsafe diagnostics callback has been removed. |
| src/main/ipc/pty-producer-flow-control.test.ts | Covers ceiling expiry, non-extension by reassertion, re-pausing, cancellation, isolation, and throwing resume callbacks. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Pending output exceeds high watermark] --> B[Pause PTY producer]
B --> C[Arm maximum-hold timer]
C --> D{Pending drains below low watermark?}
D -->|Yes| E[Cancel timer and resume producer]
D -->|No, ceiling expires| F[Drop paused bookkeeping and resume producer]
F --> G{Later report still exceeds high watermark?}
G -->|Yes| B
G -->|No| H[Continue normally]
Reviews (2): Last reviewed commit: "refactor: drop unused pause-ceiling diag..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe producer flow controller adds a 10-second default ceiling for continuous PTY pauses. It arms the ceiling on the initial pause and does not extend it on re-assertion. It clears the ceiling when output drains or the PTY is released. When the ceiling expires, the controller resumes the producer and updates pause bookkeeping. Tests cover timer behavior, cancellation, independent PTYs, re-pausing, and resume exceptions. Merge Risk: 🟡 Moderate · up to The new PTY pause-ceiling timer can still allow a throwing diagnostics callback to surface as an uncaught main-process error, potentially destabilizing the application during backpressure handling; this exception path should be handled or explicitly accepted before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The PR implements a relevant mitigation for [
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f2c5e76-6368-47aa-babb-9ae462de5e76
📒 Files selected for processing (2)
src/main/ipc/pty-producer-flow-control.test.tssrc/main/ipc/pty-producer-flow-control.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Review feedback (Greptile/CodeRabbit): the callback had no callers and a future implementation could throw inside the ceiling timer and take down the main process. Removing it shrinks the API surface and the risk.
CI has never run — needs a maintainer to approve workflows
For whoever opens it: 30-second version
Two properties reviewers usually ask about, both covered by tests:
Not a duplicate of the other open terminal PRsI linked #12523 as the narrower issue and #12283 is now re-attributed to #18446 (synchronized-output frames), so I want to be explicit: this does not claim to fix either. #12624 / #12525 change the renderer write/scheduler path for dense-SGR echo latency; #15319 paces the provider-side |
ELI5
When a terminal pane produces output faster than the UI can draw it, Orca tells the OS to stop reading that terminal so the program inside blocks and stops flooding. It undoes that once the backlog drains. The problem: "the backlog drained" is only ever learned from the renderer sending an acknowledgement. If those acknowledgements stop arriving, nothing ever says "you can resume", and the program stays frozen mid-sentence. This adds a 10-second ceiling on how long one of those pauses can be held, so a stuck agent always gets un-stuck by itself.
What Changed
PtyProducerFlowControllernow bounds a single continuous paused span:PRODUCER_PAUSE_MAX_HOLD_MS = 10_000. When a PTY is paused, a timer is armed; if the span reaches the ceiling, the pause is released unconditionally and the bookkeeping dropped.PRODUCER_PAUSE_REASSERT_INTERVAL_MS) deliberately does not re-arm it, or a sustained flood would extend the span forever and reintroduce the same unbounded hold.release(),releaseAll().unref()d so a pending resume never holds the process open at exit.No call sites changed. Sustained backpressure behavior is unchanged: pending still above HIGH re-pauses on the very next report, so this removes only the unbounded hold, not the throttling.
Why
Resume is entirely ACK-driven.
updateProducerFlowControlis only ever invoked from a data/flush/ACK event, and it computespendingCharsfromsession.pendingData, which shrinks only when the renderer acknowledges. So the resume edge depends on a signal that can stop arriving (wedged renderer, dropped push channel, lifecycle reset mid-flight). Once it does,updateeither stops being called or keeps reporting a value above LOW, and the pause is held with no upper bound.How much that hurts depends on the provider, and the two live providers disagree:
SessionProducerPausearmsPRODUCER_PAUSE_FAILSAFE_MS = 5_000and auto-resumes. Its comment states the invariant directly: "a lost resume must never wedge a shell." A held pause degrades to a 5s stutter.LocalPtyProvider.pauseProducerisptyProcesses.get(id)?.pause()with no failsafe. Nothing ever resumes it. The child stays blocked onwrite()indefinitely.That asymmetry is the bug. The daemon already treats "a lost resume must never wedge a shell" as a hard invariant; the main-side controller is the layer that can hold a pause across any provider, so the same guarantee belongs here rather than being re-implemented per provider.
Why this matters more for agent panes than for a shell: a full-screen TUI runs the tty in raw mode, where a full buffer returns
EAGAINand the writer blocks. A cooked-mode shell silently discards instead, so it never wedges. On macOS, Node's stdout-to-TTY writes are synchronous, so a blocked write stalls the agent's whole event loop — the spinner freezes on one frame and input stops being read, while the process is alive andS+. That is why this presents as "the agent is frozen" rather than "output is slow".Consistent with the existing
releaseAll()comment on this class — "a local PTY left paused here would stay wedged forever" — which already identifies the wedge; this generalizes that safety net from window-destroyed to any unbounded hold.10s is chosen to sit clearly above the daemon's 5s failsafe so the daemon's own recovery always wins on that path and this stays a genuine last resort, while remaining short enough that a user reads it as a hitch rather than a hang.
Linked Issue
Refs #12523 — the narrower match. Its diagnosis names this exact layer as root-cause stage 3: "main-process delivery window (
rendererInFlightCharssaturated at 1 MB,ackGatedFlushSkipCount> 100)". This PR bounds how long that saturated window can hold the producer paused. It does not claim to fix #12523's headline symptom: #12523 is bounded-but-large echo latency under a flood that still drains, whereas this is an unbounded wedge where the resume edge never arrives at all. #12523 also notes "keeping the in-flight delivery window small alone did not fix it" — agreed, and this PR does not propose that as a latency fix; it only removes the infinite hold.Refs #12283 — Pi agent pane appears frozen at 'working' with no streamed output during long responses.
Refs #11178 — daemon busy-waits on
EAGAINwriting to/dev/ptmxwhen the agent stops draining its PTY; same mechanism observed from the provider side.Honest status of the #12283 link
I linked #12283 on 2026-08-29 because its reported symptom matched. On 2026-09-03 the reporter re-attributed that Windows first-tool freeze to a different mechanism — #18446, a DEC 2026 synchronized-output frame not upgraded to the interactive render path — and explicitly called it "distinct from the dense-SGR backlog in #12523". So I no longer expect this PR to resolve #12283. Its "eventually recovers" detail is in fact evidence against this path: a hold that clears on its own looks like the daemon's 5s failsafe already firing, which is the behaviour this ceiling is a backstop for, not the failure it prevents. I keep the reference because an unbounded pause on the local provider produces the same visible symptom, but this PR should not be read as #12283's fix.
Relationship to the other open fixes in this area
#12624 and #12525 are both open against the dense-SGR echo-latency symptom and change the renderer write/scheduler path. This PR is neither a duplicate nor a competitor: it lives in
PtyProducerFlowControlleron the main side and changes only the lifetime of a producer pause, so it is composable with whichever of those lands. #15319 addresses the provider-sideEAGAINretry loop from #11178; a pause ceiling is orthogonal to write-retry pacing.Visual Proof
N/A— no UI surface. The change is internal PTY flow-control timing; there is no visual or interaction change to capture, only the absence of a freeze.Testing
pnpm test src/main/ipc/pty-producer-flow-control.test.ts— 17 passed (10 pre-existing, unmodified; 7 new). Verified the 10 original assertions still pass unchanged, so hysteresis, re-assert, per-PTY isolation, and throw-safety are not regressed.New cases in a
pause ceilingdescribe block:release()/releaseAll()already cancelled itTypecheck clean on the changed file under
--strict.Platforms: logic is platform-agnostic (
setTimeout+ existing transport calls) and I did not touch any platform-guarded path. The provider gap it protects is most consequential on the local (non-daemon) provider on all three OSes. The diagnosis behind it was done on macOS against 1.4.192, whose panes are daemon-hosted; the patch itself has only been exercised by the unit tests above, on no platform end-to-end. I have not reproduced the wedge on Windows or Linux. The fix is derived from the ACK-driven resume path plus the provider asymmetry, both platform-independent.SSH/remote:
SshPtyProvider.pauseProducerroutes to the relay's own pause path; this only bounds how long main holds the pause, adding an upper bound where there was none. No wire format, RPC param, or stream opcode is touched, so there is no mixed-version concern.To be explicit about the unchecked box: I verified this at the unit level (17 tests, fake timers) and by typecheck, not by building the app and reproducing a live wedge. I could not get the wedge to occur on demand — my own panes are daemon-hosted, where the 5s failsafe masks it, which is exactly why the local-provider gap is easy to miss. A reviewer who can reach the local (non-daemon) provider path is better placed to confirm the end-to-end recovery.
AI Disclosure
This PR was produced end-to-end by an AI coding agent (an Anthropic Claude model, accessed through a gateway under the alias
claude-ultimate; I can't verify which exact published version that maps to, so I won't name one). That covers the investigation, the root-cause analysis, the patch, the tests, and this PR description.It has not had a line-by-line human review before submission. The human in the loop asked for the fix and approved sending it, but is relying on your review rather than vouching for the diff independently. Treat the reasoning in Why as a claim to check, not as a verified account.
What was actually verified, mechanically: the 17 unit tests pass, the 10 pre-existing tests still pass unmodified, and the changed file typechecks under
--strict. What was not verified: any live wedge or its recovery in a running build (see Testing).The root cause was established by reading this repository's source. The constants and the provider asymmetry were additionally cross-checked against the shipped 1.4.192 bundle to confirm the behavior is live in a release build and not just on
main.Review
Summary from the agent that wrote the patch, per the contributor guide. Unreviewed by a human — verify rather than trust.
Cross-platform: no platform branch touched;
setTimeout/clearTimeout/unrefonly.unref?.()is optional-called so a non-Node timer shape cannot throw.SSH/remote/local: all three providers reach this class through the same
ProducerFlowControlTransport. The ceiling only adds an upper bound to a pause each provider already receives. No wire compatibility surface.Agent/integration neutrality: no agent-specific logic. The class never learns which agent occupies a PTY, and nothing keys off agent identity.
Performance: one
setTimeoutper paused PTY — paused is already the rare path, and the timer is cleared on all three exit paths, so steady state adds no timers and no work in the data path. Map entries are removed with their timers, so no unbounded growth. Worst case for a genuinely sustained flood is one extra pause/resume pair per 10s per PTY, which is cheaper than the 5s daemon re-assert already in place.Security: no new input, IPC surface, or privileged call; no data logged.
Backwards compatibility: additive. New exported constant, two new optional constructor options; existing construction sites compile and behave identically. Behavior changes only in the state that was previously a wedge.
Risk: the deliberate trade-off is that releasing a still-flooded PTY lets it produce again, so a real flood re-fills pending and re-pauses. That is the intended failure mode — a bounded stutter under flood is strictly better than an unbounded freeze, and it matches the reasoning the daemon failsafe already encodes.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
One adjacent thing I deliberately left out of scope:
LocalPtyProvider.pauseProducercould grow its own failsafe mirroringSessionProducerPause, giving defense in depth at the provider layer. I put the bound in the controller instead because it covers every provider at once and keeps the invariant in the layer that owns the hold. Happy to add the provider-level one too if you'd prefer it there.Author: not on X.