diff --git a/src/CodeShellManager/MainWindow.xaml.cs b/src/CodeShellManager/MainWindow.xaml.cs index 6d821de..64556c6 100644 --- a/src/CodeShellManager/MainWindow.xaml.cs +++ b/src/CodeShellManager/MainWindow.xaml.cs @@ -68,6 +68,12 @@ public partial class MainWindow : Window WpfButton drawerCopyBtn, WpfButton drawerSendBtn)> _runControls = new(); private readonly Dictionary _drawerItemBySession = new(); + /// + /// What each session's drawer TextBox currently holds: which run, the append position + /// it reflects, and how many characters it contains. Lets RefreshRunDrawer append only + /// the new tail rather than re-assigning up to 1MB of text per refresh. + /// + private readonly Dictionary _drawerShown = new(); // Per-session sidebar action button panels — kept so SettingsButton_Click // can flip every row to a new SidebarActionIconsMode without rebuilding sidebar items. private readonly Dictionary _sidebarActionPanels = new(); @@ -1189,25 +1195,63 @@ private void RefreshTerminalRunControls(string sessionId) } c.chipsStrip.Visibility = instances.Count > 0 ? Visibility.Visible : Visibility.Collapsed; - // Update drawer if a viewed item exists. + RefreshRunDrawer(sessionId); + } + + /// + /// Updates the run-output drawer only — header, text, stop button. This is the part + /// output changes need, and it runs throttled from Runner.OutputChanged. + /// + private void RefreshRunDrawer(string sessionId) + { + if (!_runControls.TryGetValue(sessionId, out var c)) return; + var vm = _vm.Sessions.FirstOrDefault(s => s.Id == sessionId); + if (vm == null) return; + if (_drawerItemBySession.TryGetValue(sessionId, out var viewedItemId) && vm.Runner.GetInstance(viewedItemId) is { } viewedInst) { c.drawerHeader.Text = $"{viewedInst.Label} — {DescribeState(viewedInst)}"; - c.drawerText.Text = viewedInst.SnapshotOutput(); + c.drawerStopBtn.IsEnabled = viewedInst.State == RunState.Running; + + // Append only the new tail. Re-assigning the whole buffer (up to 1MB) per + // refresh made the TextBox re-lay-out everything on the UI thread each time. + // Fall back to a full snapshot when the drawer shows a different run, the + // buffer's head was trimmed past what is shown, or the box has grown well + // beyond the buffer cap (AppendText never trims). + string? tail = null; + long position = 0; + if (_drawerShown.TryGetValue(sessionId, out var shown) && ReferenceEquals(shown.inst, viewedInst)) + tail = viewedInst.ReadSince(shown.position, out position); + + if (tail != null && shown.length + tail.Length <= RunDrawerMaxChars) + { + if (tail.Length > 0) c.drawerText.AppendText(tail); + _drawerShown[sessionId] = (viewedInst, position, shown.length + tail.Length); + } + else + { + string text = viewedInst.SnapshotOutput(out position); + c.drawerText.Text = text; + _drawerShown[sessionId] = (viewedInst, position, text.Length); + } + // Auto-scroll to the end while the run is active. if (viewedInst.State == RunState.Running) c.drawerText.ScrollToEnd(); - c.drawerStopBtn.IsEnabled = viewedInst.State == RunState.Running; } else { // Viewed item disappeared (was dismissed). Hide the drawer. c.drawer.Visibility = Visibility.Collapsed; _drawerItemBySession.Remove(sessionId); + _drawerShown.Remove(sessionId); } } + /// Drawer text ceiling before a full re-snapshot: 1.5× RunInstance's buffer cap. + private const int RunDrawerMaxChars = 1_500_000; + private static string DescribeState(RunInstance inst) => inst.State switch { RunState.Idle => "idle", @@ -4798,7 +4842,22 @@ or nameof(SessionViewModel.WorkingFolder) _runControls[vm.Id] = (playBtn, chevronBtn, chipsStrip, chipsPanel, drawer, drawerText, drawerHeader, drawerStopBtn, drawerCopyBtn, drawerSendBtn); - vm.Runner.InstancesChanged += () => Dispatcher.Invoke(() => RefreshTerminalRunControls(vm.Id)); + // Never Dispatcher.Invoke from here: state changes can arrive on a PTY thread, and a + // synchronous Invoke parks that thread until the UI thread gets round to it. + vm.Runner.InstancesChanged += () => + { + if (Dispatcher.CheckAccess()) RefreshTerminalRunControls(vm.Id); + else Dispatcher.BeginInvoke(() => RefreshTerminalRunControls(vm.Id)); + }; + // Output fires once per 4KB PTY chunk. It used to share the event above, so every + // chunk of a chatty dev server rebuilt the chips and re-assigned the whole drawer + // through a synchronous Invoke. Now: drawer only, at most ~4 times a second, below + // input priority so it never delays a keystroke. + var drawerRefresh = new ThrottledSignal( + TimeSpan.FromMilliseconds(250), + action => Dispatcher.BeginInvoke(action, System.Windows.Threading.DispatcherPriority.Background), + () => RefreshRunDrawer(vm.Id)); + vm.Runner.OutputChanged += drawerRefresh.Signal; // RepoRoot lands async after GitService probes — when AccentColor shifts, repaint // the top stripe and refresh the active-ring lookup so terminal and sidebar agree. @@ -4833,6 +4892,7 @@ private void OnSessionVmClosed(SessionViewModel vm) } _runControls.Remove(vm.Id); _drawerItemBySession.Remove(vm.Id); + _drawerShown.Remove(vm.Id); _sidebarActionPanels.Remove(vm.Id); _sidebarRenameActions.Remove(vm.Id); if (_selectionAnchorId == vm.Id) _selectionAnchorId = null; @@ -5127,6 +5187,7 @@ private async Task RestartSessionCoreAsync(SessionViewModel vm, string? launched } _runControls.Remove(vm.Id); _drawerItemBySession.Remove(vm.Id); + _drawerShown.Remove(vm.Id); _sidebarActionPanels.Remove(vm.Id); _sidebarRenameActions.Remove(vm.Id); @@ -5262,6 +5323,7 @@ private void SleepSession(SessionViewModel vm) } _runControls.Remove(vm.Id); _drawerItemBySession.Remove(vm.Id); + _drawerShown.Remove(vm.Id); _sidebarActionPanels.Remove(vm.Id); _sidebarRenameActions.Remove(vm.Id); diff --git a/src/CodeShellManager/Services/RunInstance.cs b/src/CodeShellManager/Services/RunInstance.cs index a3e18ae..4546e84 100644 --- a/src/CodeShellManager/Services/RunInstance.cs +++ b/src/CodeShellManager/Services/RunInstance.cs @@ -45,6 +45,9 @@ public partial class RunInstance : ObservableObject, IDisposable private readonly Func _ptyFactory; private readonly StringBuilder _ansiStripped = new(); private readonly object _bufLock = new(); + // Characters ever appended, including any since trimmed from the head. Lets the drawer + // append only what is new instead of re-assigning up to 1MB per refresh — see ReadSince. + private long _totalAppended; private bool _disposed; public event Action? OutputChanged; @@ -78,7 +81,7 @@ public void Start(ShellSession parent) { if (_pty != null) throw new InvalidOperationException("Already started — call Dispose() first."); - lock (_bufLock) { _ansiStripped.Clear(); } + lock (_bufLock) { _ansiStripped.Clear(); _totalAppended = 0; } OutputBuffer = ""; ExitCode = null; StartedAt = DateTime.Now; @@ -152,9 +155,7 @@ private void AppendText(string text) string snapshot; lock (_bufLock) { - _ansiStripped.Append(text); - if (_ansiStripped.Length > MaxBufferChars) - _ansiStripped.Remove(0, _ansiStripped.Length - MaxBufferChars); + AppendLocked(text); snapshot = _ansiStripped.ToString(); } OutputBuffer = snapshot; @@ -183,15 +184,19 @@ private void OnPtyData(string text) // PropertyChanged per chunk on a hot path. OutputBuffer has no consumers // in the app (only tests) — the app reads SnapshotOutput() on demand. string stripped = AnsiPattern().Replace(text, ""); - lock (_bufLock) - { - _ansiStripped.Append(stripped); - if (_ansiStripped.Length > MaxBufferChars) - _ansiStripped.Remove(0, _ansiStripped.Length - MaxBufferChars); - } + lock (_bufLock) AppendLocked(stripped); OutputChanged?.Invoke(); } + /// Caller holds . + private void AppendLocked(string text) + { + _ansiStripped.Append(text); + _totalAppended += text.Length; + if (_ansiStripped.Length > MaxBufferChars) + _ansiStripped.Remove(0, _ansiStripped.Length - MaxBufferChars); + } + private void OnPtyExited() { EndedAt = DateTime.Now; @@ -272,6 +277,36 @@ public string SnapshotOutput() lock (_bufLock) return _ansiStripped.ToString(); } + /// + /// Snapshots the buffer together with the append position it reflects, so a later + /// can continue from exactly there. Thread-safe. + /// + public string SnapshotOutput(out long position) + { + lock (_bufLock) + { + position = _totalAppended; + return _ansiStripped.ToString(); + } + } + + /// + /// Text appended after (a value from an earlier read), or + /// null when the head has since been trimmed past it and the caller must re-snapshot. + /// is where the next read continues from. Thread-safe. + /// + public string? ReadSince(long position, out long newPosition) + { + lock (_bufLock) + { + newPosition = _totalAppended; + long headPosition = _totalAppended - _ansiStripped.Length; + if (position < headPosition || position > _totalAppended) return null; + int start = (int)(position - headPosition); + return _ansiStripped.ToString(start, _ansiStripped.Length - start); + } + } + /// /// Wraps a single-statement CommandLine for cmd.exe so &&, pipes, /// redirects, and quoted args all parse correctly. The outer cmd /c diff --git a/src/CodeShellManager/Services/SessionRunner.cs b/src/CodeShellManager/Services/SessionRunner.cs index 255bc4b..a7ddb2c 100644 --- a/src/CodeShellManager/Services/SessionRunner.cs +++ b/src/CodeShellManager/Services/SessionRunner.cs @@ -20,6 +20,13 @@ public class SessionRunner : IDisposable /// Fires when any instance is added, replaced, or removed, or any state changes. public event Action? InstancesChanged; + /// + /// Fires when any instance produced output — once per PTY chunk, from the PTY reader + /// thread. Kept separate from so the UI can throttle the + /// high-rate output path without delaying chip state changes. + /// + public event Action? OutputChanged; + public SessionRunner(ShellSession session) { _session = session; } /// @@ -107,7 +114,7 @@ public void StopAll() } private void OnInstanceStateChanged() => InstancesChanged?.Invoke(); - private void OnInstanceOutputChanged() => InstancesChanged?.Invoke(); + private void OnInstanceOutputChanged() => OutputChanged?.Invoke(); public void Dispose() => StopAll(); } diff --git a/src/CodeShellManager/Services/StateService.cs b/src/CodeShellManager/Services/StateService.cs index 0041f0a..2b8e28b 100644 --- a/src/CodeShellManager/Services/StateService.cs +++ b/src/CodeShellManager/Services/StateService.cs @@ -25,6 +25,9 @@ public class StateService /// Serializes concurrent saves — see SaveAsync. private static readonly SemaphoreSlim SaveGate = new(1, 1); + /// Order in which snapshots were taken, and the newest one on disk (read/written under SaveGate). + private static long _saveSeq, _writtenSeq; + private static readonly JsonSerializerOptions Options = new() { WriteIndented = true, @@ -112,20 +115,43 @@ internal static AppState Normalize(AppState s) /// file truncated and take the whole workspace with it. /// public async Task SaveAsync(AppState state) + { + // Serialize HERE, on the caller's thread, and do the file work on the pool. + // + // Callers are almost all on the UI thread, and the AppState graph is mutated there + // too, so the snapshot has to be taken there — serializing on another thread could + // walk a session list mid-edit. The I/O is a different matter: the temp write and + // File.Replace used to run on the UI thread as well (no ConfigureAwait, and + // File.Replace is synchronous), and antivirus or a synced folder can make that swap + // take a visible while. Every OSC 9001 prompt hook, window move and git RepoRoot + // landing triggers a save. + long seq = Interlocked.Increment(ref _saveSeq); + byte[] json; + try { json = JsonSerializer.SerializeToUtf8Bytes(state, Options); } + catch (Exception ex) { Log($"save failed: {ex.Message}"); return; } + + await Task.Run(() => WriteAsync(json, seq)).ConfigureAwait(false); + } + + private static async Task WriteAsync(byte[] json, long seq) { // Serialize saves. 29 of the ~32 SaveStateAsync call sites are fire-and-forget, // so overlapping writes are routine rather than exotic. Without this gate two // saves race on the same temp file, and one can File.Replace the other's // half-written content into the live file — manufacturing exactly the torn // state this method exists to prevent. - await SaveGate.WaitAsync(); + await SaveGate.WaitAsync().ConfigureAwait(false); var path = StatePath; try { + // The gate doesn't promise FIFO order, and snapshots are now taken before + // waiting on it — so an older snapshot can reach this point after a newer + // one has already been written. Writing it would roll state back. + if (seq <= _writtenSeq) return; + Directory.CreateDirectory(Path.GetDirectoryName(path)!); - string json = JsonSerializer.Serialize(state, Options); - await File.WriteAllTextAsync(TempPath, json); + await File.WriteAllBytesAsync(TempPath, json).ConfigureAwait(false); if (File.Exists(path)) // Atomic on NTFS, and rotates the old file into .bak in one step. @@ -133,6 +159,8 @@ public async Task SaveAsync(AppState state) File.Replace(TempPath, path, BackupPath, ignoreMetadataErrors: true); else File.Move(TempPath, path); // first write — nothing to back up or replace + + _writtenSeq = seq; } catch (Exception ex) { diff --git a/src/CodeShellManager/Terminal/PseudoTerminal.cs b/src/CodeShellManager/Terminal/PseudoTerminal.cs index 1beee6a..e7964f7 100644 --- a/src/CodeShellManager/Terminal/PseudoTerminal.cs +++ b/src/CodeShellManager/Terminal/PseudoTerminal.cs @@ -320,7 +320,18 @@ public void Start(string command, string args, string workingDirectory, _stdin = new FileStream(_inputWrite!, FileAccess.Write, 4096, false); _stdout = new FileStream(_outputRead!, FileAccess.Read, 4096, false); - _ = Task.Run(ReadLoopAsync); + // A dedicated thread, not the pool: this read blocks for the whole life of the + // session. On the pool that meant one permanently parked pool thread per PTY — + // 55 of them measured at ~60 sessions, against a pool minimum of one per core. + // Past the minimum the pool only adds threads gradually, so any burst of other + // pool work (git polls, timers, save I/O) queued — including the next read of the + // active pane, which looks exactly like a keystroke that doesn't echo. + // Same reasoning as MonitorExitAsync's RegisterWaitForSingleObject below. + new Thread(ReadLoop) + { + IsBackground = true, + Name = "PTY reader", + }.Start(); _ = Task.Run(MonitorExitAsync); } @@ -353,7 +364,7 @@ private static void Log(string msg) catch { } } - private async Task ReadLoopAsync() + private void ReadLoop() { var buffer = new byte[4096]; try @@ -361,12 +372,11 @@ private async Task ReadLoopAsync() while (!_cts.IsCancellationRequested) { // Anonymous pipes are synchronous — ReadAsync stalls on Windows. - // Use blocking Read() on a thread-pool thread instead. - int read = await Task.Run(() => - { - try { return _stdout!.Read(buffer, 0, buffer.Length); } - catch { return 0; } - }, _cts.Token); + // Blocking Read() on this dedicated thread instead; it returns 0 when + // the pipe closes (process exit, or Dispose closing the pseudoconsole). + int read; + try { read = _stdout!.Read(buffer, 0, buffer.Length); } + catch { read = 0; } if (read == 0) break; // Send raw bytes as Latin-1 so xterm.js can interpret VT sequences correctly diff --git a/src/CodeShellManager/Terminal/ThrottledSignal.cs b/src/CodeShellManager/Terminal/ThrottledSignal.cs new file mode 100644 index 0000000..7fbf002 --- /dev/null +++ b/src/CodeShellManager/Terminal/ThrottledSignal.cs @@ -0,0 +1,47 @@ +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace CodeShellManager.Terminal; + +/// +/// Turns a burst of signals from any thread into at most one action per interval. +/// +/// Run-command output used to repaint its UI once per 4KB PTY chunk, each time through a +/// synchronous Dispatcher.Invoke — a chatty dev server queued UI work as fast as it +/// printed, and its reader thread stalled until each repaint finished. Signals that arrive +/// while a run is already pending piggyback on it; the pending flag is cleared just before +/// the action runs, so output landing *during* the action schedules exactly one more. +/// +/// The scheduler is injected so the state machine is testable with no WPF dispatcher. +/// +public sealed class ThrottledSignal +{ + private readonly TimeSpan _delay; + private readonly Action _post; + private readonly Action _action; + private int _pending; + + /// How long to wait after the first signal of a burst. + /// Marshals the action onto its thread (Dispatcher.BeginInvoke in production). + /// The work to run once per burst. + public ThrottledSignal(TimeSpan delay, Action post, Action action) + { + _delay = delay; + _post = post; + _action = action; + } + + /// Safe to call from any thread. Never blocks. + public void Signal() + { + if (Interlocked.Exchange(ref _pending, 1) == 1) return; + _ = Task.Delay(_delay).ContinueWith(_ => _post(Run), TaskScheduler.Default); + } + + private void Run() + { + Volatile.Write(ref _pending, 0); + _action(); + } +} diff --git a/tests/CodeShellManager.Tests/SessionRunnerTests.cs b/tests/CodeShellManager.Tests/SessionRunnerTests.cs index 3720006..e507f93 100644 --- a/tests/CodeShellManager.Tests/SessionRunnerTests.cs +++ b/tests/CodeShellManager.Tests/SessionRunnerTests.cs @@ -307,6 +307,57 @@ public void RunInstance_OutputBufferAppendsOnPtyData() Assert.True(outputEvents >= 2); } + [Fact] + public void Runner_OutputFiresOutputChangedButNotInstancesChanged() + { + // Output is per 4KB PTY chunk; InstancesChanged rebuilds the chips. Sharing one + // event made a chatty run rebuild its chips for every chunk. + var fake = new FakePseudoTerminal(); + var runner = new SessionRunner(LocalSession(), () => fake); + runner.Run(Item()); + + int instances = 0, output = 0; + runner.InstancesChanged += () => instances++; + runner.OutputChanged += () => output++; + + fake.EmitData("a"); + fake.EmitData("b"); + + Assert.Equal(0, instances); + Assert.Equal(2, output); + } + + [Fact] + public void RunInstance_ReadSinceReturnsOnlyTheNewTail() + { + var fake = new FakePseudoTerminal(); + var inst = new RunInstance(Item(), () => fake); + inst.Start(LocalSession()); + + fake.EmitData("hello "); + Assert.Equal("hello ", inst.SnapshotOutput(out long pos)); + + fake.EmitData("world"); + Assert.Equal("world", inst.ReadSince(pos, out pos)); + Assert.Equal("", inst.ReadSince(pos, out _)); + } + + [Fact] + public void RunInstance_ReadSinceReturnsNullOnceTheHeadIsTrimmedPastIt() + { + // The drawer must re-snapshot rather than append a tail that no longer joins up. + var fake = new FakePseudoTerminal(); + var inst = new RunInstance(Item(), () => fake); + inst.Start(LocalSession()); + + fake.EmitData("start"); + inst.SnapshotOutput(out long pos); + + fake.EmitData(new string('x', 1_000_010)); // trims "start" and then some + + Assert.Null(inst.ReadSince(pos, out _)); + } + [Fact] public void RunInstance_OutputBufferStripsAnsiEscapeSequences() { diff --git a/tests/CodeShellManager.Tests/StateServiceTests.cs b/tests/CodeShellManager.Tests/StateServiceTests.cs index c4b2a2d..2f55388 100644 --- a/tests/CodeShellManager.Tests/StateServiceTests.cs +++ b/tests/CodeShellManager.Tests/StateServiceTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Threading; using System.Threading.Tasks; using CodeShellManager.Models; using CodeShellManager.Services; @@ -103,6 +104,52 @@ public async Task Save_ManyConcurrentSaves_LeaveAValidFileAndNoTempFile() Assert.StartsWith("s", loaded.Sessions[0].Id); } + [Fact] + public async Task Save_ManyConcurrentSaves_TheNewestSnapshotWins() + { + // Snapshots are taken before the save gate is acquired, and the gate isn't FIFO, + // so an older snapshot can reach the disk after a newer one. It must be dropped, + // not written over the newer state. + var saves = new List(); + for (int i = 0; i < 40; i++) + saves.Add(_svc.SaveAsync(WithSessions($"s{i}"))); + + await Task.WhenAll(saves).WaitAsync(TimeSpan.FromSeconds(30)); + + var loaded = await _svc.LoadAsync(); + Assert.Equal("s39", Assert.Single(loaded.Sessions).Id); + } + + /// Records Posts but never runs them — stands in for a busy UI thread. + private sealed class DeadSynchronizationContext : SynchronizationContext + { + public override void Post(SendOrPostCallback d, object? state) { } + public override void Send(SendOrPostCallback d, object? state) { } + } + + [Fact] + public async Task Save_NeverNeedsTheCallersContextToFinish() + { + // The file write and File.Replace used to continue on the caller's context — the + // WPF UI thread, for nearly every caller. If any await still captures it, the + // continuation is handed to a context that never runs it and the save never ends. + var tcs = new TaskCompletionSource(); + var thread = new Thread(() => + { + SynchronizationContext.SetSynchronizationContext(new DeadSynchronizationContext()); + tcs.SetResult(_svc.SaveAsync(WithSessions("ui"))); + }) { IsBackground = true }; + thread.Start(); + + Task save = await tcs.Task; + Task done = await Task.WhenAny(save, Task.Delay(TimeSpan.FromSeconds(20))); + Assert.True(ReferenceEquals(done, save), + "SaveAsync did not complete on a caller whose SynchronizationContext never runs work."); + + var loaded = await _svc.LoadAsync(); + Assert.Equal("ui", Assert.Single(loaded.Sessions).Id); + } + // ── recovery ───────────────────────────────────────────────────────────── [Fact] diff --git a/tests/CodeShellManager.Tests/ThrottledSignalTests.cs b/tests/CodeShellManager.Tests/ThrottledSignalTests.cs new file mode 100644 index 0000000..ce710cc --- /dev/null +++ b/tests/CodeShellManager.Tests/ThrottledSignalTests.cs @@ -0,0 +1,72 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using CodeShellManager.Terminal; +using Xunit; + +namespace CodeShellManager.Tests; + +/// +/// Run-command output used to repaint its drawer once per 4KB PTY chunk through a +/// synchronous Dispatcher.Invoke. collapses a burst into one +/// action, and must never lose the last chunk of a burst. +/// +public class ThrottledSignalTests +{ + private static readonly TimeSpan Delay = TimeSpan.FromMilliseconds(50); + + [Fact] + public async Task A_burst_of_signals_runs_the_action_once() + { + int runs = 0; + var signal = new ThrottledSignal(Delay, a => a(), () => Interlocked.Increment(ref runs)); + + for (int i = 0; i < 1000; i++) signal.Signal(); + await Task.Delay(500); + + Assert.Equal(1, runs); + } + + [Fact] + public async Task A_signal_after_the_action_ran_schedules_another() + { + int runs = 0; + var signal = new ThrottledSignal(Delay, a => a(), () => Interlocked.Increment(ref runs)); + + signal.Signal(); + await Task.Delay(500); + signal.Signal(); + await Task.Delay(500); + + Assert.Equal(2, runs); + } + + [Fact] + public async Task A_signal_during_the_action_is_not_lost() + { + // Output that lands while the drawer is being repainted must get its own repaint, + // or the tail of a run's output would stay invisible until the next chunk. + int runs = 0; + ThrottledSignal? signal = null; + signal = new ThrottledSignal(Delay, a => a(), () => + { + if (Interlocked.Increment(ref runs) == 1) signal!.Signal(); + }); + + signal.Signal(); + await Task.Delay(500); + + Assert.Equal(2, runs); + } + + [Fact] + public void Signal_never_runs_the_action_inline() + { + int runs = 0; + var signal = new ThrottledSignal(Delay, a => a(), () => runs++); + + signal.Signal(); + + Assert.Equal(0, runs); + } +}