You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fixes#5695: hardens the UTF-8 output path introduced by #5650, preserves existing output subclasses, and recovers from native output failures. Includes the subsequent independent and Claude review fixes.
Current state
Draft targeting develop, at c44042bb0, which adds the follow-up review fixes below on top of 7f85fc0ca.
All 13 applicable hosted checks pass on c44042bb0. One macOS job initially failed on the unrelated timing-sensitive TimeoutTests.AddTimeout_Nested_Run_Parent_Timeout_Fires; its rerun passed. The push-only benchmark job is skipped on PRs.
Implementation, regression, performance and macOS PTY verification are complete for this revision. Local baseline failures and the interactive Windows verification limit are documented below.
Changes
Preserve the original protected StringBuilder attribute and write hooks, including flush-before-color ordering, concrete derived output sinks, Unicode and raster payloads. Guard legacy byte forwarding against recursion.
Keep Utf8Buffer internal and reuse bounded row/frame/encoding storage. Bound cumulative staging at 1 MB; stream large text without a temporary payload-sized byte array.
Check Unix, WriteFile and WriteConsoleW results, retry exact positive short-write tails, reject impossible progress, and bound transient Unix retries.
Restore consumed dirty flags and graphics state after a failed frame. Contain expected I/O errors at the driver boundary and retry physical output even when views no longer need drawing. Raw writes recover uncertain control-string state too; programming errors still propagate.
Attempt shutdown restore commands independently, protect cursor position writes, and remove the unused leaking Unix descriptor duplication.
Make terminal capture opt-in, clear captured data when disabled, and retain headless capture by default.
Replace the buffer pool with main-loop-owned storage. Calibrate output benchmarks normally, add a >1 MB per-cell color case, and retain output benchmarks in the existing push job.
Raw writes all go through the driver boundary.NetOutput no longer swallows IOException in its write paths. ProgressIndicator and Kitty keyboard enable/disable use DriverImpl.WriteRaw. NetOutput.SetCursor is best-effort and logged, like AnsiOutput. This resolves the three Copilot review comments.
ProgressIndicator remembers only sequences that were written, so a failed set or clear is retried.
Retry backoff. A failed frame is retried immediately once. After that, retries back off from 25 ms up to 1 s. DriverImpl.Refresh enforces the backoff, so views that redraw every iteration follow it too. Errors are logged once at the first failure and once on recovery. Later retries log only at Debug, at most once per second at the backoff cap. Before, a broken terminal produced one full frame plus one error per main loop iteration. DriverImpl owns this logging. Low-level writers no longer log errors they rethrow, and best-effort cursor failures log at Debug.
The Unix EINTR/EAGAIN budget counts consecutive stalls only. Any progress resets it, so a slow terminal draining its buffer still receives the whole frame.
The legacy AppendOrWriteAttribute shim is linear. It flushes pending text before calling the hook, which is the built-in Win32 hook pattern, instead of re-decoding the pending row on every attribute change. On a 200×50 per-cell color frame, allocation drops from 114 MB to linear.
GetColumns has a single-char fast path. Allocation for the 400×110 per-cell color frame drops from 14.76 MB to 4.55 MB in Release.
Test files are renamed by feature: OutputFailureHandlingTests and OutputRetryAndCompatibilityTests. Eight new regression tests are added; each new behavioral regression fails on 7f85fc0ca.
Local results: the parallel suite has 17,795 total and 0 failures, the nonparallel suite has 32 passed, the Release benchmarks build, and there are no new warnings.
#5694 provides awaitable, cancellable UI dispatch with optional session ownership. This PR provides output compatibility, bounded encoding/staging and native-write recovery. Both use the existing main-loop ownership model: background work dispatches UI updates, and rendering/buffer mutation runs on the UI thread. This branch can be merged independently; it adds no dependency on #5694's new dispatch API.
A temporary combined checkout of #5694 at 92abe857f plus this PR at 7f85fc0ca passed 98 cases, covering the dispatch suite, both output review suites, original output-path regressions and three additional integration probes:
A worker queues 1,000 numbered Unicode frames; every native sink callback runs on the UI thread and successful frames preserve order.
A second worker-dispatch probe makes every seventh physical frame fail. All 1,000 dispatch tasks complete successfully through the driver recovery boundary, 858 frames reach the native sink and the final frame arrives. Failed intermediate frames may be superseded by newer UI state.
Exception behavior matters when using the two changes together:Driver.Refresh/WriteRaw contain expected native I/O errors, while direct IOutput.Write calls still throw on failure. With #5694, an exception escaping a dispatched callback faults its returned task. The recovery probe exercises the driver boundary, so it does not assert that arbitrary direct output errors are swallowed.
Verification on the current head
All 13 applicable hosted jobs passed: Windows/macOS/Linux parallel and nonparallel unit suites, all three integration suites, Windows/Linux build validation, documentation snippets and Linux performance smoke tests. The existing push-only benchmark job is skipped on PRs.
25 new Claude regression cases passed. The focused output/native/legacy suite passes 110 cases. Five of six compatible regressions fail on the previous draft; the already-fixed cumulative color staging case passes there.
Full local parallel suite: 17,782 total, 17,761 passed, 17 skipped, four failures matching the unchanged-draft color baseline. Local nonparallel suite: 32 passed. Integration: 343 passed plus the known UICatalog assembly-load failure. Performance smoke suite: 5 passed.
Debug and Release solution builds pass. Only existing unrelated warnings remain.
All five revised BenchmarkDotNet cases completed. Ordinary UnixRaw frames use one physical write; clean frames use none. The 400×110 truecolor frame emits 1,367,367 bytes in two writes, largest 1,039,097 bytes, within the staging limit.
Warmed large-text encoding allocation: 3,162,144 bytes before → 0 after for a 3,162,115-byte Unicode payload. Surrogate-boundary and exact-payload regressions pass. Per-cell color-frame allocations were dominated by existing GetColumns work; the single-char fast path in 4cde18e0f reduces them from 14.76 MB to 4.55 MB per 400×110 frame.
Real macOS PTY: Buttons renders and responds to navigation; inspected recording and PNG contain no replacement characters or exception text. Earlier Character Map recordings also verified complex Unicode rendering.
Native descriptor probe: after warmup, 50 real output create/dispose cycles grow open descriptors from 70 to 120 on the old code, versus 71 to 71 on the fix. The fixed recording contains 51 alternate-screen activations and restorations.
Whitespace check passes; isolated worktree is clean and pushed.
Claude review disposition
#
Disposition
1
Driver frame/raw-write boundaries contain native errors; clean views no longer suppress recovery. EINTR/EAGAIN retries are bounded; cursor position errors stay inside the best-effort boundary.
2
Every terminal shutdown restore is attempted independently.
3
Legacy attribute/text ordering fixed in d06b893; direct and physical-order regressions retained.
4
WriteConsoleW loops on short writes; failures without error codes become IOException. Native failure remains observable to direct callers and recoverable at the driver boundary.
5
ConcurrentBag and Interlocked lease bookkeeping replaced by one reusable row buffer with finally cleanup.
6
Existing cumulative staging fix verified with a 400×110 frame changing truecolor at every cell.
7
Explicit compatibility scope prevents recursive StringBuilder/byte forwarding, including derived ANSI/.NET sinks and raster writes.
8
Implementation helpers narrowed to private protected; disabling capture clears/releases data. Opt-in terminal capture retained as requested by #5695; IOutput documentation clarifies its optional OutputBase setting.
9
Bounded reusable text encoding; no discarded strings/builders in the capture-disabled default byte sink. Genuine legacy sinks retain their necessary conversion.
10
Constructor comment explains once-only virtual override detection and both legacy contracts.
11
Repository test-marker prefix adopted with explicit Codex origin annotation.
12
Direct output failure assertions retained; new driver/application regressions establish that app rendering continues and retries.
13
InvocationCount(1)/IterationSetup removed; per-operation dirty marking allows calibration. Per-cell color benchmark added; duplicate unconditional PR timing job removed. Deterministic resource/output assertions run in ordinary tests.
14
Cached-cursor abort and ignored Windows VT results fixed in d06b893; descriptor leak and test-constructor console mutation fixed in this update.
Design choices and limits
Direct IOutput writes continue to report failure; swallowing native errors there would incorrectly consume output state and mislead explicit callers. Driver.Refresh/WriteRaw provide the application's recovery boundary. Opt-in terminal capture avoids decoding/copying every physical write; headless capture still defaults on.
Output is synchronous and owned by the main loop. Worker UI updates must be dispatched to that thread; same-instance concurrent/reentrant writes are unsupported. No render locks or awaits were added. Bounded nonblocking retries do not give blocking OS writes a timeout.
A native failure may already have changed the terminal. Recovery resets uncertain terminal state and replays dirty work; it cannot make terminal I/O atomic or deliver to a disconnected device.
The four local color failures and UICatalog's Terminal.Gui, Version=2.4.8.0 load failure reproduce on the previous draft. An additional intermittent static-color assertion appeared in one broad run and passes in focused/final hosted checks; the unchanged-draft broad run also exhibits intermittent static-color interference.
Short matched probes show unchanged steady allocations and no consistent throughput slowdown across frame sizes. Their individual timings vary and do not establish a small speedup. The calibrated final BenchmarkDotNet run reports 266.5 µs for 80×25, 2,163.1 µs for 240×70 and 7,503.7 µs for the per-cell color case, excluding actual terminal I/O.
An interactive Windows Terminal/ConPTY was unavailable locally. Injected Windows native-result tests and successful Windows CI complement actual macOS PTY testing; a Windows visual smoke check remains useful before merge.
Review status
The implementation and all applicable hosted checks are complete on the current head. The PR remains draft for maintainer review. An interactive Windows Terminal/ConPTY visual smoke check remains useful before merge.
- Route ProgressIndicator and Kitty keyboard enable/disable through
DriverImpl.WriteRaw so terminal I/O failures are contained.
- ProgressIndicator only records sequences that were actually written,
so a failed set or clear is retried.
- Remove IOException swallows in NetOutput writes so failures reach the
frame transaction; make NetOutput.SetCursor best-effort like AnsiOutput.
- Back off repeated output retries (0, 25ms .. 1s) and log only the first
failure and the recovery, instead of a full frame and an error per loop.
- Reset the Unix EAGAIN/EINTR budget on progress so a slow terminal still
receives the whole frame; the budget bounds consecutive stalls only.
- Legacy AppendOrWriteAttribute shim: flush pending text before the hook
(matching the built-in Win32 hook) instead of re-decoding the pending
row per attribute change (114 MB -> linear for a 200x50 colored frame).
- GetColumns single-char fast path avoids grapheme enumeration per cell.
- Rename review-named test files to feature names; add regression tests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Check this native result before continuing. In legacy 16-color mode, a failed attribute change is currently treated as a successful frame, so the text may be written with stale colors, its dirty flags are cleared, and DriverImpl has no failure to retry. Throwing here, as the checked WriteConsoleW path does, lets the frame transaction restore its state.
AnsiOutput logged and rethrew write IOExceptions, so DriverImpl logged
each failure a second time and every backed-off retry still emitted an
error. Drop that log and keep only the pending-move cleanup. Cursor
updates are best effort and run every iteration, so log their failures
at Debug; a broken sink also fails the next frame, which DriverImpl
reports once.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The backoff deadline was only checked in the no-draw retry branch, so a
view that redraws every iteration still flushed a full frame to a failing
terminal on each loop. Gate DriverImpl.Refresh itself: while a retry is
pending but not due, keep the dirty cells and let the next due call write
the latest buffer.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Log only first failure and recovery, not every retry
Terminal.Gui/Drivers/DriverImpl.cs:154
Repeated failures are still logged here on every retry, contrary to the PR's first-failure/recovery-only policy. A permanently broken terminal will therefore keep producing one debug entry per backoff attempt indefinitely; suppress these intermediate entries and retain only the error at the first failure plus the recovery message.
I've done multiple rounds of code reviews and updates using GPT-6 Sol, Opus 5.5, and the GitHub CoPilot code review bot. Ready for final review.
This branch has not been deployed
No deployments
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
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
Fixes #5695: hardens the UTF-8 output path introduced by #5650, preserves existing output subclasses, and recovers from native output failures. Includes the subsequent independent and Claude review fixes.
Current state
develop, atc44042bb0, which adds the follow-up review fixes below on top of7f85fc0ca.c44042bb0. One macOS job initially failed on the unrelated timing-sensitiveTimeoutTests.AddTimeout_Nested_Run_Parent_Timeout_Fires; its rerun passed. The push-only benchmark job is skipped on PRs.Changes
Follow-up review fixes (
4cde18e0f,77d686a0c,c44042bb0)NetOutputno longer swallowsIOExceptionin its write paths.ProgressIndicatorand Kitty keyboard enable/disable useDriverImpl.WriteRaw.NetOutput.SetCursoris best-effort and logged, likeAnsiOutput. This resolves the three Copilot review comments.ProgressIndicatorremembers only sequences that were written, so a failed set or clear is retried.DriverImpl.Refreshenforces the backoff, so views that redraw every iteration follow it too. Errors are logged once at the first failure and once on recovery. Later retries log only at Debug, at most once per second at the backoff cap. Before, a broken terminal produced one full frame plus one error per main loop iteration.DriverImplowns this logging. Low-level writers no longer log errors they rethrow, and best-effort cursor failures log at Debug.AppendOrWriteAttributeshim is linear. It flushes pending text before calling the hook, which is the built-in Win32 hook pattern, instead of re-decoding the pending row on every attribute change. On a 200×50 per-cell color frame, allocation drops from 114 MB to linear.GetColumnshas a single-char fast path. Allocation for the 400×110 per-cell color frame drops from 14.76 MB to 4.55 MB in Release.OutputFailureHandlingTestsandOutputRetryAndCompatibilityTests. Eight new regression tests are added; each new behavioral regression fails on7f85fc0ca.Relationship to #5694
PR #5694 — Fixes #5653. Add awaitable session-aware UI dispatch is open and unmerged as of this description update. Its reviewed/tested head is
92abe857f58399f11b1c0e23f32cece4085c768a.#5694 provides awaitable, cancellable UI dispatch with optional session ownership. This PR provides output compatibility, bounded encoding/staging and native-write recovery. Both use the existing main-loop ownership model: background work dispatches UI updates, and rendering/buffer mutation runs on the UI thread. This branch can be merged independently; it adds no dependency on #5694's new dispatch API.
A temporary combined checkout of #5694 at
92abe857fplus this PR at7f85fc0capassed 98 cases, covering the dispatch suite, both output review suites, original output-path regressions and three additional integration probes:TimedEvents.Addedsubscriber leaves no faulted dispatch timeout queued on Fixes #5653. Add awaitable session-aware UI dispatch #5694's reviewed head.Exception behavior matters when using the two changes together:
Driver.Refresh/WriteRawcontain expected native I/O errors, while directIOutput.Writecalls still throw on failure. With #5694, an exception escaping a dispatched callback faults its returned task. The recovery probe exercises the driver boundary, so it does not assert that arbitrary direct output errors are swallowed.Verification on the current head
4cde18e0freduces them from 14.76 MB to 4.55 MB per 400×110 frame.Claude review disposition
Design choices and limits
Direct IOutput writes continue to report failure; swallowing native errors there would incorrectly consume output state and mislead explicit callers. Driver.Refresh/WriteRaw provide the application's recovery boundary. Opt-in terminal capture avoids decoding/copying every physical write; headless capture still defaults on.
Output is synchronous and owned by the main loop. Worker UI updates must be dispatched to that thread; same-instance concurrent/reentrant writes are unsupported. No render locks or awaits were added. Bounded nonblocking retries do not give blocking OS writes a timeout.
A native failure may already have changed the terminal. Recovery resets uncertain terminal state and replays dirty work; it cannot make terminal I/O atomic or deliver to a disconnected device.
The four local color failures and UICatalog's Terminal.Gui, Version=2.4.8.0 load failure reproduce on the previous draft. An additional intermittent static-color assertion appeared in one broad run and passes in focused/final hosted checks; the unchanged-draft broad run also exhibits intermittent static-color interference.
Short matched probes show unchanged steady allocations and no consistent throughput slowdown across frame sizes. Their individual timings vary and do not establish a small speedup. The calibrated final BenchmarkDotNet run reports 266.5 µs for 80×25, 2,163.1 µs for 240×70 and 7,503.7 µs for the per-cell color case, excluding actual terminal I/O.
An interactive Windows Terminal/ConPTY was unavailable locally. Injected Windows native-result tests and successful Windows CI complement actual macOS PTY testing; a Windows visual smoke check remains useful before merge.
Review status
The implementation and all applicable hosted checks are complete on the current head. The PR remains draft for maintainer review. An interactive Windows Terminal/ConPTY visual smoke check remains useful before merge.