fix: keep PTY output flow control bounded - #66
Open
htazq wants to merge 1 commit into
Open
Conversation
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.
Summary
reset()/clear()path and rely on the existing bounded xterm scrollbackRoot cause
The backend consumes one semaphore permit per output frame, but the frontend replenished a fixed four permits after processing any low-water batch.
creditsGrantedwas never decremented when frames arrived and was reset immediately after granting, so a low-rate stream could grow the backend credit window by roughly three permits per frame. After a long-running session, bursts were no longer bounded by the intended initial window of two frames.The separate 2 MiB guard counted all decoded output since the last clear, not retained terminal memory. Normal
top,watch,iostat, build, or fio output eventually called bothterm.reset()andterm.clear()even though xterm scrollback is already bounded by its configured line limit.After this change, the invariant is: available credits plus frames awaiting xterm completion remain equal to the initial window. An incomplete UTF-8 sequence retains at most the decoder's few pending bytes and returns that frame credit immediately, avoiding deadlock.
Related work
Validation
Regression command was red on current main before the fix:
Resumemessages instead of 3 totalterm.reset()called oncePassing after the fix:
pnpm install --frozen-lockfilepnpm test -- src/__tests__/pty-terminal-activation.test.tsx src/__tests__/terminal-scrollback.test.ts src/__tests__/terminal-tab-portals.test.tsx— 12 passedpnpm test— 512 passedpnpm buildpnpm exec tsc --noEmitpnpm i18n:check— 945 keys in both localespnpm exec eslint src/components/pty-terminal.tsx --quiet— 0 errorsgit diff --checkRepository-wide
pnpm lintremains blocked by the pre-existingsrc/components/settings-modal.tsx:308no-unnecessary-type-assertionerror; this PR does not touch that file. Local Rust/Tauri commands are blocked by this Windows host's application-control policy (os error 4551) when Cargo executes newly generated build scripts. The PR CI runscargo teston Windows, macOS, and Ubuntu.Performance boundary
The affected long-running app process was no longer active when the patch was built, and there was no historical PerfMon/WPR series, so this PR does not claim a measured renderer CPU or memory reduction. It fixes the deterministic credit-window expansion and destructive clear. Background terminals intentionally remain mounted and continue parsing output; their bounded per-terminal xterm/WebGL cost is unchanged.
Risk / rollback
The change reduces the maximum in-flight output to the already documented two-frame window. If xterm stops completing writes, output now backpressures instead of accumulating permits. Rollback is the single commit in this PR.