perf: coalesce per-session PTY output dispatcher posts to prevent UI starvation #70

Activity

1. AThraen

   added

   enhancementNew feature or requestNew feature or request

   on Jun 2, 2026
2. AThraen commented on Aug 7, 2026

   

   AThraen

   on Aug 7, 2026

   ContributorAuthor

   More actions

   Re-confirmed against current main (2026-08-07) — and it's the top user-reported pain

   Reported again from live use: "when typing in a different window it tends to freeze up… it can take a minute or two before that window updates with the text I am writing." Note the magnitude — this issue originally measured hundreds of milliseconds; lived experience is minutes. Code trace below explains the gap. Promoting this from enhancement to the main stability bug.

   1. The original diagnosis is unchanged and unfixed

   TerminalBridge.OnPtyData (src/CodeShellManager/Terminal/TerminalBridge.cs:236-270) still does exactly one Dispatcher.BeginInvoke per PTY chunk, with no coalescing. The proposal in this issue was never implemented.

   2. Input is starved too, not just rendering — this is the missing amplifier

   The issue as written frames this as output posts delaying a keystroke. The keystroke path is worse than that. OnWebMessageReceived → case "input" → _pty?.Write(data) (line ~291) has no dispatcher hop of its own — but CoreWebView2.WebMessageReceived is raised on the UI thread. So a keystroke is delivered by the same saturated message pump it's competing with.

   The delay therefore compounds in both directions:

   1. Keystroke leaves the browser process → waits in the UI pump behind N queued output posts → _pty.Write finally runs.
   2. Claude echoes the character → OnPtyData → another BeginInvoke → waits behind the next backlog → renders.

   Two full queue traversals per visible character, not one. That's how hundreds of ms becomes minutes, and it matches the symptom precisely: the characters are eventually accepted and eventually appear, in order, just very late.

   3. Blocking Dispatcher.Invoke on the PTY thread applies backpressure

   MainViewModel.cs:273, :281, :291 and :78 use blocking Dispatcher.Invoke (not BeginInvoke), reached from AlertDetector on the PTY read thread. When the UI thread is saturated these block the PTY reader itself, so output stops being drained while the queue is already deep — the stall feeds itself.

   4. Every session polls git every 10s, forever

   SessionViewModel.PollGitInfoAsync (ViewModels/SessionViewModel.cs:109-118) runs a PeriodicTimer(10s) per session for the session's whole life, calling RefreshGitInfoAsync → GitService spawns git -C <dir> … (GitService.cs:169) for branch + status --porcelain. At the 23-session scale in this issue that's ~46 process spawns per 10s — a continuous ~5/sec of full working-tree scans on large repos — plus OnPropertyChanged marshalling per result. This is unconditional: it polls background, off-group, and unfocused sessions at the same rate as the active one.

   Revised fix plan (ordered by value/risk)

   1. Coalesce output posts per bridge — as originally proposed. Biggest single win, low risk, no protocol change.
   2. Prioritize the active session. Coalescing still leaves the foreground behind N background sessions' (now smaller) posts. Post background output at DispatcherPriority.Background and the active session's at Normal, so foreground rendering and input jump the queue.
   3. Convert the blocking Dispatcher.Invoke calls in MainViewModel to BeginInvoke so the PTY reader never blocks on the UI thread. Needs a check that no caller depends on the synchronous completion.
   4. Back off git polling — only poll the visible/active sessions at 10s; drop background sessions to a much longer interval or poll on demand (on focus, on activity). Consider a shared coalescing scheduler so 20 sessions can't spawn 40 processes in one tick.
   5. Cap or drop stale background output. A background session emitting a spinner has no need to render every frame it missed; collapsing runs of output that will be immediately overwritten would cut the queue further.

   Steps 1–3 are the freeze; step 4 is the sustained background load. DebugTerminalTrace already emits OUTPUT post dispatcher-latency=…ms, so before/after is measurable without new instrumentation.
3. AThraen

   added

   bugSomething isn't workingSomething isn't working

   on Aug 7, 2026
4. AThraen commented on Aug 7, 2026

   

   AThraen

   on Aug 7, 2026

   ContributorAuthor

   More actions

   Correction + a sharper finding on point 3

   My point 3 above said the blocking Dispatcher.Invoke calls are "reached from AlertDetector on the PTY read thread." That's wrong for AlertRaised — it fires from OnIdle, a System.Threading.Timer callback (AlertDetector.cs:42,68), so it blocks a threadpool thread, not the PTY reader. Still worth converting to BeginInvoke, but it is not PTY backpressure.

   The input path is worse than I described, via a different mechanism. AlertCleared fires from NotifyUserInteracted() (AlertDetector.cs:45-48), which TerminalBridge calls from UserInput on the UI thread. So MainViewModel.cs:291's Dispatcher.Invoke executes inline, in the keystroke hot path. Combined with :273, every single keystroke synchronously does:

   • OnPropertyChanged(nameof(AlertCount)) twice
   • and AlertCount is Sessions.Count(s => s.NeedsAttention) (MainViewModel.cs:61) — an O(N) scan over every session
   • each raise invalidating WPF bindings

   At 20+ sessions that is two full session scans plus two binding invalidation passes per character typed, on the UI thread, on top of the queued output backlog.

   Separately: AlertDetector.Feed disposes and reallocates a System.Threading.Timer per PTY output chunk (AlertDetector.cs:40-42) — timer churn proportional to total output volume across all sessions.

   So step 3 of the plan becomes: raise AlertCount only when the value actually changes, drop the redundant second raise, and stop doing O(N) work in the keystroke path — not merely swap Invoke for BeginInvoke.
5. AThraen

   closed this as completedin #83on Aug 10, 2026
6. AThraen

   added a commit that references this issue on Aug 20, 2026

   fix(terminal): make the pane you click or type in the active session (#…

   ...

   5c9b955
7. AThraen commented on Sep 7, 2026

   

   AThraen

   on Sep 7, 2026

   ContributorAuthor

   More actions

   Reopening — reproduced on v0.7.0, and only 3 of the 5 steps ever shipped

   Live report (2026-09-07, v0.7.0, ~25 sessions): "even though the active window now changes correctly, I am still seeing issues with when I type that it all of a sudden stops showing what I type and then waits a bit until it shows all the input in a given window."

   The active-session half is fixed (#93/#106/#108) and confirmed working by the reporter, so this is no longer confounded by the highlight bug. What remains is the original symptom, unchanged in shape: input is accepted in order, then renders late and all at once.

   What actually landed

   	Step from the fix plan above	State
   1	Coalesce output posts per bridge	✅ TerminalBridge.cs:37 _outputBuffer, OutputCoalescer
   2	Prioritize the active session	✅ TerminalBridge.cs:150-160, synced from UpdateActiveTerminalHighlight (MainWindow.xaml.cs:1917)
   3	Unblock the keystroke path	✅ blocking Dispatcher.Invoke gone from the hot path; AlertCount now behind RaiseAlertCountIfChanged (MainViewModel.cs:71-77)
   4	Back off git polling	❌ never done
   5	Cap or drop stale background output	❌ never done

   SessionViewModel.PollGitInfoAsync (ViewModels/SessionViewModel.cs:118-127) is still an unconditional PeriodicTimer(TimeSpan.FromSeconds(10)) per session, for the session's whole life, with no foreground/background distinction:

   using var timer = new PeriodicTimer(TimeSpan.FromSeconds(10));
   while (await timer.WaitForNextTickAsync(ct))
       await RefreshGitInfoAsync();

   At the ~25-session workload this reporter runs (see the restore measurements on #82), that is 50 git process spawns every 10 seconds, indefinitely — branch + a full status --porcelain working-tree scan each, on repos of arbitrary size.

   Also still true and never filed separately: AlertDetector.Feed (Services/AlertDetector.cs:41-43) disposes and reallocates a System.Threading.Timer per PTY output chunk, on top of StripAnsi + Split('\n') + per-line Trim per chunk — allocation churn proportional to total output volume across every session.

   The instrumentation regression — read this before running a trace

   My 2026-08-07 comment said "DebugTerminalTrace already emits OUTPUT post dispatcher-latency=…ms, so before/after is measurable without new instrumentation."

   That is no longer true, and the reason is uncomfortable: the fix for this issue deleted it.

   • 092b801 added Trace($"OUTPUT post dispatcher-latency={Environment.TickCount64 - enqueueAt}ms len={len}")
   • 71e1294 — the coalescing commit — replaced the per-chunk BeginInvoke with OutputCoalescer and dropped the enqueue timestamp with it. The flush path now logs only OUTPUT flush len=…, with no latency at all.

   So the one number that separates the two live hypotheses was removed by the change that was supposed to fix the thing it measured. Nobody would have noticed, because the trace still produces plausible-looking output.

   Where we are blind

   xterm keypress (renderer)
      -> postMessage                         [A] no trace
   OnWebMessageReceived (UI thread)          INPUT len=            ✅
      -> _pty.Write                          PTY-WROTE elapsed=    ✅
      -> ConPTY -> claude
   claude echoes -> PTY read thread
      OnPtyData                              OUTPUT recv gap=      ✅
      -> coalescer.Append -> ScheduleFlush   [B] no trace
      -> dispatcher queue                    [C] NOT MEASURED  <-- hypothesis 1
      -> PostOutput (UI thread)              OUTPUT flush len=     ✅ (no latency)
      -> PostWebMessageAsString
      -> page onmessage -> term.write        [D] no trace      <-- hypothesis 2
      -> renderer paints                     [E] no trace
   

   Two hypotheses remain live and the existing trace cannot tell them apart:

   1. UI dispatcher saturation — the queue at [C] is deep, so the coalesced flush waits. Steps 4/5 above are the untreated load. Note a specific risk in the step-2 fix: non-foreground sessions post at DispatcherPriority.Background, and under sustained load Background work can be starved for a long time. If IsForeground is ever stale for the pane being typed into, that pane's echo goes to the back of the queue indefinitely — which is precisely "nothing, nothing, then all at once".
   2. Renderer starvation — with this many WebView2 instances (61 processes / ~9.4 GB were measured at 23 sessions), the bottleneck may be past [D], inside the browser process, where nothing on the host side can see it. Claude's own ink TUI batching its redraw is a third possibility that also lives past [C].

   Next step: restore and extend the instrumentation, then measure

   Deliberately not proposing a fix. This repo has now been burned three times fixing a timing symptom from a plausible theory — #113's font re-fit was a no-op, #96's adaptive gate measured someone else's stall, and #117 repeated that at shutdown. Adding load-reduction (step 4) without measurement would be the fourth.

   Instrumenting first:

   • restore dispatcher-latency on the coalesced flush, including which priority the batch was queued at
   • log time-since-last-INPUT on flush, so echo lag is directly visible
   • a UI-thread heartbeat independent of any bridge, to separate "the UI thread is saturated" from "this one bridge is slow"
   • page-side timing at [D]/[E] — term.write duration and requestAnimationFrame cadence — since renderer starvation is invisible from the host

   Then one traced session at the real ~25-session workload decides which hypothesis survives.
8. AThraen

   reopened this on Sep 7, 2026
9. AThraen

   added a commit that references this issue on Sep 8, 2026

   perf(git): take the git pipeline off the UI thread, and stop polling …

   ...

   6d8c65e
10. AThraen

    closed this as completedin #126on Sep 8, 2026
11. AThraen commented on Sep 8, 2026

    

    AThraen

    on Sep 8, 2026

    ContributorAuthor

    More actions

    Closing the record on step 5, which was never done and never explained

    This issue was closed with step 5 — "Cap or drop stale background output. A background session emitting a spinner has no need to render every frame it missed" — undone, and no rationale written anywhere. Recording the decision now so nobody re-derives it.

    Step 5 is deliberately dropped, because the premise it rested on was falsified by the trace.

    It was proposed on the theory that output volume was flooding the dispatcher queue. The measured run says otherwise: in a 68-second window that contained a 9.1s UI stall, total output across 12 emitting sessions was 69KB — about 10 flushes/sec. Capping or collapsing that would have saved nothing, because it was never the cost. The cost was ~94 synchronous Process.Start calls per poll cycle on the UI thread, which is what #126 fixed.

    OutputCoalescer therefore stays an unbounded StringBuilder on purpose. Two things make that safe rather than merely untested:

    • It is drained on every dispatcher turn, so it only accumulates for as long as the UI thread is blocked — and the whole point of perf(git): take the git pipeline off the UI thread, and stop polling blind #126 was to stop the UI thread being blocked for seconds at a time.
    • Dropping output is not free. A "stale" frame is only stale if something later overwrites it, and CSM cannot know that: a background session may be emitting a spinner, or it may be emitting the build error you are about to scroll back to. Silently discarding terminal output to save memory that measurement says we are not using is a bad trade.

    What would reopen it: a TRACE-OVERFLOW line in crash.log, or a coalesced flush with a len= in the megabytes. Both are visible in the existing instrumentation, so this will announce itself rather than needing to be looked for.

    Worth noting the interaction the earlier comment flagged: a pane posting at DispatcherPriority.Background can in principle be starved indefinitely under sustained load, and that pane's buffer grows for the duration. That risk is real but is a starvation bug, not a buffering one — the fix would be priority, not a cap.
12. AThraen

    added a commit that references this issue on Sep 8, 2026

    fix: v0.8.0 pre-release hardening — command injection, UI-thread spaw…

    ...

    9cc55be