feat: terminal trace setting + shutdown/restore stability fixes#36
Merged
Conversation
Adds opt-in AppSettings.DebugTerminalTrace (off by default) under Settings > Diagnostics. When on, TerminalBridge logs per-keystroke and per-output-chunk timing plus WPF dispatcher latency to crash.log under the [DEBUG-tt] prefix, so intermittent terminal-input freezes can be diagnosed after the fact. Zero cost when off. Also fixes two latent bugs surfaced while auditing crash.log: - OutputIndexer.Dispose now drains its writer task (2s bounded wait) before MainWindow closes the shared SqliteConnection, so pending FTS5 INSERTs finish first. _db.Close/Dispose in OnClosing are also try/caught defensively — SqliteConnection.Close has been observed to NRE internally during shutdown regardless of the indexer race. - Bulk restore now batches WebView2 UnauthorizedAccessException into a single consolidated dialog at end of restore instead of N "Restore Error" popups, with a message pointing at the likely cause (another running instance locking the WebView2 user-data folder). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a diagnostic setting to trace terminal I/O timing, and improves shutdown/restore stability by draining the output indexer on dispose and consolidating WebView2 restore errors.
Changes:
- Introduce
AppSettings.DebugTerminalTraceand surface it in Settings UI. - Add
[DEBUG-tt]tracing hooks inTerminalBridgefor input/output timing and dispatcher latency. - Improve shutdown/restore behavior: drain
OutputIndexeron dispose, guard SQLite close/dispose, and batch WebView2 access-denied restore popups into one dialog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/CodeShellManager/Views/SettingsWindow.xaml.cs | Wires DebugTerminalTrace to the edited settings model and save flow. |
| src/CodeShellManager/Views/SettingsWindow.xaml | Adds a Diagnostics section with a new checkbox for terminal tracing. |
| src/CodeShellManager/Terminal/TerminalBridge.cs | Implements trace logging for terminal input/output timing and dispatcher latency. |
| src/CodeShellManager/Terminal/OutputIndexer.cs | Drains the writer task briefly during dispose to reduce shutdown races with SQLite. |
| src/CodeShellManager/Models/AppState.cs | Adds the DebugTerminalTrace setting to persisted app settings. |
| src/CodeShellManager/MainWindow.xaml.cs | Batches WebView2 restore access-denied errors and adds defensive SQLite close/dispose handling on shutdown. |
Comments suppressed due to low confidence (1)
src/CodeShellManager/Terminal/TerminalBridge.cs:206
- The dispatcher callback does synchronous file I/O via
Trace(...)before posting to WebView2. Since this runs on the UI thread, enabling tracing can distort the very dispatcher-latency being measured and may exacerbate stalls. Prefer capturing timestamps here but writing logs off-thread (or at least after the critical UI work).
string json = JsonSerializer.Serialize(new { type = "output", data = rawData });
long enqueueAt = DebugSettings?.DebugTerminalTrace == true ? Environment.TickCount64 : 0;
int len = rawData.Length;
WpfApplication.Current?.Dispatcher.BeginInvoke(() =>
{
if (enqueueAt != 0)
Trace($"OUTPUT post dispatcher-latency={Environment.TickCount64 - enqueueAt}ms len={len}");
try { _webView.CoreWebView2?.PostWebMessageAsString(json); }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+61
to
+73
| private void Trace(string msg) | ||
| { | ||
| if (DebugSettings?.DebugTerminalTrace != true) return; | ||
| try | ||
| { | ||
| string path = System.IO.Path.Combine( | ||
| System.Environment.GetFolderPath(System.Environment.SpecialFolder.ApplicationData), | ||
| "CodeShellManager", "crash.log"); | ||
| System.IO.Directory.CreateDirectory(System.IO.Path.GetDirectoryName(path)!); | ||
| System.IO.File.AppendAllText(path, | ||
| $"[{DateTime.Now:HH:mm:ss.fff}] [DEBUG-tt] {DebugSessionId ?? "?"} {msg}\n"); | ||
| } | ||
| catch { } |
Comment on lines
+431
to
+432
| <CheckBox x:Name="DebugTerminalTraceCheck" Content="Trace terminal input/output to crash.log"/> | ||
| <TextBlock Text="Logs every keystroke, PTY write, and output chunk with timing to %AppData%\CodeShellManager\crash.log (prefix [DEBUG-tt]). Use to diagnose terminal freezes. Off by default — turn on only when reproducing a problem." |
Comment on lines
72
to
+81
| public void Dispose() | ||
| { | ||
| if (_disposed) return; | ||
| _disposed = true; | ||
| _queue.Writer.Complete(); | ||
| // Drain the worker before returning so any in-flight INSERTs finish before | ||
| // the shared SqliteConnection is closed. Bounded wait so a slow/stuck | ||
| // worker doesn't hang shutdown — pending writes are non-critical on exit. | ||
| try { _worker.Wait(TimeSpan.FromSeconds(2)); } | ||
| catch { /* AggregateException from worker exceptions — already swallowed inside */ } |
Conflicts: OnLoaded restore loop and OnClosing teardown both diverged from the staggered-shutdown work that landed in #37. Resolutions: - OnLoaded: keep main's launching-placeholder + sequential staggered launch flow; layer this PR's batched WebView2-access-denied dialog on top so a single consolidated message replaces the previous N per-session popups. Drop the duplicate dormant-loop tail (main now adds dormant entries up-front before launches start). - OnClosing: keep main's claude-aware sequential disposal and DisposeAndWaitForExitAsync wait; layer this PR's try/catch around _db.Close()/_db.Dispose() in to swallow the SqliteConnection NRE observed during shutdown. Review fixes (from PR self-review): - OnClosing was async void with multiple awaits, which WPF doesn't wait for. Switched to the standard e.Cancel=true + reclose pattern gated by _shutdownComplete so async cleanup actually finishes before the window tears down. - _lastOutputTickMs in TerminalBridge.OnPtyData was read-modify-written without a memory barrier. Replaced with Interlocked.Exchange — atomic read+write in one call, and gives the JIT a barrier even though the PTY read loop is currently single-threaded.
Previous commit's cancel-and-reclose pattern only gated on _shutdownComplete, which is set AFTER async cleanup finishes. If the user clicks the close button a second time while SaveStateAsync / claude disposal is mid-flight, OnClosing re-enters, falls through the _shutdownComplete=false branch, and runs the full cleanup sequence a second time concurrently — double SaveStateAsync (state.json corruption risk), double Dispose on each SessionViewModel (ObjectDisposedException), double _db.Close (already swallowed by try/catch, but still pointless work). Add an _isShuttingDown flag set immediately on first entry, before any await. Re-entries during the async phase still cancel the close (e.Cancel = true) but return without re-running cleanup.
Two small follow-ups from Copilot review on #36: - Settings help text now states explicitly that only timing/byte-length metadata is logged, never the actual keystroke or output content. The old wording ("Logs every keystroke...") could be read as content logging. - In OnPtyData's dispatcher callback, capture the dispatcher latency into a local first, then PostWebMessageAsString, then Trace. Previous ordering ran sync File.AppendAllText *before* the WebView2 post, which delayed terminal rendering whenever tracing was enabled. The measurement itself was already taken inside the callback so accuracy is unchanged.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
DebugTerminalTracesetting under Settings > Diagnostics (off by default). When on,TerminalBridgelogs per-keystroke / per-output-chunk timing plus WPF dispatcher latency tocrash.logunder[DEBUG-tt]so intermittent terminal-input freezes can be diagnosed after the fact. Zero cost when off.OnClosingSQLite NRE fix.OutputIndexer.Disposenow drains its writer task (2s bounded wait) beforeMainWindowcloses the sharedSqliteConnection, so pending FTS5 INSERTs finish first._db.Close/Disposealso wrapped in try/catch — defensive, sinceSqliteConnection.Closehas been observed to NRE internally during shutdown regardless of the indexer race.UnauthorizedAccessExceptionfrom the WebView2 stack into one consolidated dialog at end of restore instead of N "Restore Error" popups, with a message pointing at the likely cause (another running instance locking the WebView2 user-data folder).Context
User reported an intermittent ~few-minute freeze when typing into a single terminal while others stayed responsive. Crash log audit didn't surface the freeze event itself (current logging is blind to it) but exposed the two latent shutdown/restore bugs above. The trace setting is the diagnostic loop for the original report — next occurrence will pin which stage (xterm.js, PTY, dispatcher) is stalling.
Test plan
dotnet build src/CodeShellManager/CodeShellManager.csproj— 0 errorsdotnet test tests/CodeShellManager.Tests/— 82/82 pass[DEBUG-tt]lines in%AppData%\CodeShellManager\crash.log🤖 Generated with Claude Code