From 1ede0233318088d4a1797c9c5d38904cc3f02e95 Mon Sep 17 00:00:00 2001
From: Leonard Hecker <lhecker@microsoft.com>
Date: Thu, 28 Mar 2024 21:12:10 +0100
Subject: [PATCH] Properly fix ConPTY buffer corking (#16793)

I've found that #16079 was never properly addressed (it still randomly
occurred after even after PR #16349), which later led to the issues
described in #16769 (nushell flickering due to too many flushes).

The crux of the fix is that this brings back the `_noFlushOnEnd` flag
that was removed in PR #15991. This is then combined with a change to
the cork API: An `uncork` on `VtEngine` now only flushes if `_Flush`
got called while it was corked in the first place.

`_noFlushOnEnd` prevents us from flushing in between two "unknown"
VT sequences (like soft fonts or FTCS) which prevents them from being
corrupted. The corking prevents the remaining cases of flushing too
often. Long-term, a proper fix would be to pass through VT unmodified.

Closes #16769
---
 src/renderer/vt/invalidate.cpp |  4 ++++
 src/renderer/vt/paint.cpp      | 14 +++++++++++++-
 src/renderer/vt/state.cpp      | 19 +++++++++++++++++--
 src/renderer/vt/vtrenderer.hpp |  2 ++
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/src/renderer/vt/invalidate.cpp b/src/renderer/vt/invalidate.cpp
index 8f80d3eb8ea..489a6b50bc1 100644
--- a/src/renderer/vt/invalidate.cpp
+++ b/src/renderer/vt/invalidate.cpp
@@ -112,6 +112,10 @@ CATCH_RETURN();
     //      end paint to specifically handle this.
     _circled = circled;
 
+    // If we flushed for any reason other than circling (i.e, a sequence that we
+    // didn't understand), we don't need to push the buffer out on EndPaint.
+    _noFlushOnEnd = !circled;
+
     _trace.TraceTriggerCircling(*pForcePaint);
     return S_OK;
 }
diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp
index 4e5d3ec5389..6dc611252c4 100644
--- a/src/renderer/vt/paint.cpp
+++ b/src/renderer/vt/paint.cpp
@@ -94,7 +94,19 @@ using namespace Microsoft::Console::Types;
         RETURN_IF_FAILED(_MoveCursor(_deferredCursorPos));
     }
 
-    _Flush();
+    // If this frame was triggered because we encountered a VT sequence which
+    // required the buffered state to get printed, we don't want to flush this
+    // frame to the pipe. That might result in us rendering half the output of a
+    // particular frame (as emitted by the client).
+    //
+    // Instead, we'll leave this frame in _buffer, and just keep appending to
+    // it as needed.
+    if (!_noFlushOnEnd)
+    {
+        _Flush();
+    }
+
+    _noFlushOnEnd = false;
     return S_OK;
 }
 
diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp
index 9749bf63d32..ea176e42f37 100644
--- a/src/renderer/vt/state.cpp
+++ b/src/renderer/vt/state.cpp
@@ -136,10 +136,19 @@ CATCH_RETURN();
 
 void VtEngine::_Flush() noexcept
 {
-    if (!_corked && !_buffer.empty())
+    if (_buffer.empty())
+    {
+        return;
+    }
+
+    if (!_corked)
     {
         _flushImpl();
+        return;
     }
+
+    // Defer the flush until someone calls Cork(false).
+    _flushRequested = true;
 }
 
 // _corked is often true and separating _flushImpl() out allows _flush() to be inlined.
@@ -167,7 +176,13 @@ void VtEngine::_flushImpl() noexcept
 void VtEngine::Cork(bool corked) noexcept
 {
     _corked = corked;
-    _Flush();
+
+    // Now do the deferred flush from a previous call to _Flush().
+    if (!corked && _flushRequested)
+    {
+        _flushRequested = false;
+        _flushImpl();
+    }
 }
 
 // Method Description:
diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp
index 1a7c25316fd..3bea4528ac5 100644
--- a/src/renderer/vt/vtrenderer.hpp
+++ b/src/renderer/vt/vtrenderer.hpp
@@ -137,7 +137,9 @@ namespace Microsoft::Console::Render
 
         bool _resizeQuirk{ false };
         bool _passthrough{ false };
+        bool _noFlushOnEnd{ false };
         bool _corked{ false };
+        bool _flushRequested{ false };
         std::optional<TextColor> _newBottomLineBG{ std::nullopt };
 
         [[nodiscard]] HRESULT _WriteFill(const size_t n, const char c) noexcept;