Skip to content

Flush large terminal output on Ctrl+C (#631) - #804

Merged
MisterTea merged 11 commits into
masterfrom
flush-output-on-interrupt
Sep 5, 2026
Merged

Flush large terminal output on Ctrl+C (#631)#804
MisterTea merged 11 commits into
masterfrom
flush-output-on-interrupt

Conversation

@MisterTea

Copy link
Copy Markdown
Owner

Summary

  • Fixes Flow Control #631 by dropping a large unsent terminal output queue when the user hits Ctrl+C (also Ctrl+Z / Ctrl+), so the prompt is not stuck behind megabytes of stdout.
  • Normal sessions stay lossless: nothing is discarded unless the userspace queue is already ≥64KB at interrupt time. While disconnected, PTY reads still use the existing 64MB BackedWriter path so long-running jobs do not stall.
  • Unsent data is kept in a userspace WriteBuffer with TCP_NOTSENT_LOWAT (and a 64KB unix SO_SNDBUF on etterminal→etserver) so it can actually be dropped. This replaces always-on backpressure/discard from Add opt-in flow control: --flow-control {backpressure,discard} (#631) #730.

Test plan

  • WriteBuffer unit tests (enqueue/drain, interrupt bytes, flush threshold, 16MB cap)
  • ServerInterruptDoesNotDropSmallOutput — Ctrl+C with a small queue is lossless
  • ServerDisconnectDoesNotStallAt64KB — after client disconnect, 256KB of PTY output is still consumed
  • Existing server e2e tests (data transfer, jumphost, multiple clients)
  • On a slow link, cat /dev/zero | base64 then Ctrl+C: prompt returns in a couple of seconds
  • Kick off a chatty job, disconnect, reconnect later: process was not paused

Made with Cursor

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.30435% with 261 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.75%. Comparing base (584a68b) to head (62ab04d).

Files with missing lines Patch % Lines
src/terminal/TerminalServer.cpp 44.86% 73 Missing and 29 partials ⚠️
src/terminal/JumphostPending.hpp 51.12% 49 Missing and 16 partials ⚠️
src/base/TmuxCcFilter.hpp 75.00% 14 Missing and 26 partials ⚠️
src/terminal/TerminalClient.cpp 64.10% 4 Missing and 10 partials ⚠️
test/integration_tests/YesCtrlCTest.cpp 85.88% 5 Missing and 7 partials ⚠️
src/base/WriteBuffer.hpp 92.43% 2 Missing and 7 partials ⚠️
test/integration_tests/TerminalServerTest.cpp 92.45% 4 Missing and 4 partials ⚠️
src/terminal/Console.hpp 30.00% 5 Missing and 2 partials ⚠️
src/base/PipeSocketHandler.cpp 50.00% 1 Missing and 1 partial ⚠️
src/base/SocketHandler.hpp 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #804      +/-   ##
==========================================
+ Coverage   74.17%   74.75%   +0.58%     
==========================================
  Files          99      106       +7     
  Lines        9017    10099    +1082     
  Branches     5929     6568     +639     
==========================================
+ Hits         6688     7550     +862     
- Misses       1723     1855     +132     
- Partials      606      694      +88     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

MisterTea and others added 4 commits September 1, 2026 19:18
Always-on backpressure would pause producers after a short disconnect, which
fights ET's reconnect-later model. Stage unsent output in userspace, keep the
kernel unsent cap small, and drop the queue only when the user interrupts and
the backlog is already large.

Co-authored-by: Cursor <cursoragent@cursor.com>
Covers the slow-link recipe (flood, interrupt, prompt next) without a
throttle proxy or multi-second soak.

Co-authored-by: Cursor <cursoragent@cursor.com>
Move the jumphost pending queue into a testable type so we can assert that
only large TERMINAL_BUFFER packets are dropped, then cover the same
lossless-interrupt and disconnect-does-not-stall cases with a jumpserver
in the middle.

Co-authored-by: Cursor <cursoragent@cursor.com>
Exercise a real PTY and interactive shell while limiting the captured output
to the flush threshold, proving Ctrl+C reveals the prompt without a costly
soak test.

Co-authored-by: Cursor <cursoragent@cursor.com>
@MisterTea
MisterTea force-pushed the flush-output-on-interrupt branch from 24ec556 to 0f1ca82 Compare September 2, 2026 00:19
@xz-dev

xz-dev commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Hi @MisterTea — thanks for this PR. The WriteBuffer + TCP_NOTSENT_LOWAT design is a neat way to make unsent output droppable without touching sequence numbers, and the 64KB threshold keeps normal sessions lossless.

One thought on where this could go next, curious about your take:

I think a real fix at the code level would be to generalize "flush on Ctrl+C based on estimated drain time" into a continuously enforced latency budget: the sender constantly estimates queued-bytes / recent-throughput, and once the display lag exceeds the budget (say 2 seconds), it proactively drops intermediate output so the screen always stays glued to "now" — essentially mosh-style frame skipping ported to a byte stream. The frozen-screen feeling would disappear at the root, instead of waiting for the user to hit Ctrl+C for a pardon.

The cost is equally clear: holes in the scrollback buffer, which undermines ET's core selling point over mosh (lossless scrollback + native terminal semantics). So the sensible shape seems to be an opt-in flag (e.g. --max-display-lag=2s), keeping lossless as the default — which is actually what the closed #730 was trying to do, except #730 used a byte-count switch; a time-budget implementation would map much more closely to the actual user experience.

One more layer of truth: on slow and lossy links, TCP's own RTO backoff and head-of-line blocking account for a large share of the freezes — that's exactly why mosh built its own reliable transport over UDP in the first place. Switching ET's transport would be major surgery, so in the short term "remote tmux + Ctrl+C when stuck" remains the pragmatic combination.

Would an opt-in latency-budget drop policy fit ET's direction, or do you consider scrollback integrity non-negotiable? If it sounds reasonable, I'd be happy to prototype it as a follow-up PR.

@bmaurer

bmaurer commented Sep 3, 2026

Copy link
Copy Markdown

I think this will work for standard terminals but not tmux in control mode. In control mode any type of drop could desync the connection.

For tmux control mode I think the option to have back pressure is the preferred route

Ignore SIGPIPE in the test runner (and use MSG_NOSIGNAL on recover writes) so a closed socketpair cannot kill debian jobs, and shut FakeConnection down so coverage runs do not spend minutes generating Universal Stacktrace dumps.

Co-authored-by: Cursor <cursoragent@cursor.com>
@MisterTea

Copy link
Copy Markdown
Owner Author

Thanks for the reviews.

@xz-dev A continuously enforced latency budget (queued bytes / recent throughput) is a good next step, but it is a different control loop than this PR. Here we only drop unsent TERMINAL_BUFFER data when the user hits Ctrl+C / Ctrl+Z / Ctrl+\ and the userspace queue is already ≥64KB. I would rather land that interrupt path first than generalize to always-on discard in this change.

@bmaurer Agreed that dropping bytes in tmux control mode can desync the client. This PR does not change HTM / tmux -CC: that path is still backpressure via the existing backed writer. The flush applies to Eternal Terminal PTY output only, and it still keeps keepalives and non-terminal packets. Nested tmux -CC inside a regular ET session could still be hurt by a Ctrl+C flush; that would need a CC-aware policy as a follow-up rather than blocking this interrupt fix for ordinary shells.

Drop pane %output / TTY floods at line boundaries, but retain layout/session/window messages and %begin blocks so a nested control-mode client does not desync.

Co-authored-by: Cursor <cursoragent@cursor.com>
@MisterTea

Copy link
Copy Markdown
Owner Author

@bmaurer I added logic to the data drop that detects tmux control sequences and only drops tmux content. The reason we can't go with your backpressure idea is that it breaks workflow for people who expect log-producing commands to keep running while they are disconnected.

You can test this yourself by doing

watch nvidia-smi

in tmux then disconnecting. With backpressure, the 64kb buffer will fill up and then nvidia-smi will block. Now, replace nvidia-smi with an ML training job and you see how this can be a problem.

With this PR, we only deal with backpressure when someone hits ctrl+c. In this mode, someone is already expecting things to get killed so if they lose output spam, it's reasonable.

Copilot AI and others added 5 commits September 3, 2026 22:50
…nual iTerm2 GUI test

Co-authored-by: MisterTea <306879+MisterTea@users.noreply.github.com>
(cherry picked from commit 7c4e9f4)
…ests.

Drive a private iTerm2 suite with System Events, screenshot the window, and read the visible buffer so tmux and tmux -CC Ctrl+C recovery no longer needs a human.

Co-authored-by: Cursor <cursoragent@cursor.com>
Hold droppable pane output briefly so iTerm2 can issue send-keys while the WriteBuffer is still large, recognize send -t %0 0x3, and prove both modes with the iTerm2 e2e.

Co-authored-by: Cursor <cursoragent@cursor.com>
Only watch process stdin when the console fd is stdout, and evaluate peekData into locals before Catch comparisons so GCC/MSVC do not see an empty buffer.

Co-authored-by: Cursor <cursoragent@cursor.com>
@MisterTea
MisterTea merged commit cd73190 into master Sep 5, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flow Control

4 participants