fix: release-review findings, verified over three passes

CLAUDE.md

@@ -67,7 +67,7 @@ PTY (ConPTY) → PseudoTerminal → TerminalBridge → WebView2 (xterm.js)
 | `ImportExportService` | Read/write a full `AppState` to a JSON file (settings + sessions backup) |
 | `SessionConfigEditor` | Diffs/applies a `SessionConfigDraft` onto a `ShellSession`; decides whether the change needs a PTY restart |
 | `DbGate` | Serializes every use of the shared `output.db` `SqliteConnection`. That one connection is handed to `SearchService` *and* to every `OutputIndexer`, and it is not thread-safe — concurrent create/dispose corrupts its internal command list. Acquire as `using var _ = await DbGate.AcquireAsync();` at the top of anything touching it. See issue #102 |
-| `PwshLocator` | Single answer to "pwsh or powershell?", shared by `RunInstance` (run commands) and `PseudoTerminal.BuildCmdLine` (session wrapper) so they can't disagree. Resolves via `where.exe`, then rejects zero-length/reparse-point hits — a Microsoft Store App Execution Alias stub resolves on PATH but fails to execute (#104) |
+| `PwshLocator` | Single answer to "pwsh or powershell?", shared by `RunInstance` (run commands) and `PseudoTerminal.BuildCmdLine` (session wrapper) so they can't disagree. Resolves via `where.exe`. An ordinary executable is accepted from metadata alone (no spawn). A **zero-byte reparse point is ambiguous, not bad**: that shape is a Store App Execution Alias, and a *working* Store install of PowerShell 7 looks identical to a stub left by an uninstalled app — so only that case is settled by actually probing execution. Rejecting the shape outright silently downgraded Store-PS7 users to 5.1 on every session launch |
 | `ClaudeConfigGate` | Watches Claude's config file settle. **Only used at shutdown.** The same mechanism was tried on the launch path and reverted (#111) — see the Claude launch stagger note below |
 | `ToastHelper` | Tray balloon notifications |
 | `SessionRunner` | Per-session owner of `RunInstance` dictionary (run commands runtime) |
@@ -160,6 +160,10 @@ The page-side `mousedown` handler also calls `fitAddon.fit()`, and the initial f
    - **Sleep** (`SleepSession(vm)`) → `vm.Dispose()` + remove from `Sessions` but **keep** the `ShellSession` in `SessionManager` with `IsDormant = true`. A muted dormant sidebar entry replaces the active one.
    - **Wake** (`WakeSessionAsync(session)`) → re-runs `LaunchSessionAsync(session, restoring: true)` — same path as restore-on-startup.
    - **Restart** (`RestartSessionAsync(vm)`) → sleep-style teardown *without* the dormant bookkeeping, then `LaunchSessionAsync(session, restoring: true, removeOnFailure: false)`. The `ShellSession` stays in `SessionManager` (so Id, group, run commands and sidebar slot survive) and never enters the recently-closed ring. Used by "Edit session…" — see below. Both non-default arguments matter: `restoring: true` makes a Claude session resume rather than start a fresh conversation (matching Wake — a restart tears down the same way, so it must recover the same way), and `removeOnFailure: false` stops a bad edit from *deleting* the session, since `LaunchSessionAsync`'s PTY-failure path calls `SessionManager.RemoveSession` — correct for a session that never started, destructive for a relaunch. If the relaunch fails either way, the session falls back to dormant so the row stays visible and fixable instead of leaving a launching placeholder that never resolves.
+
+     A Claude restart also **waits for the old process to actually exit** (`DisposeAndWaitForExitAsync` + a config quiesce) before relaunching. Without that it recreates the concurrent-config-writer race the launch stagger and the shutdown loop both exist to prevent, and `--resume` can read a session index the outgoing process hasn't finalised. Non-Claude sessions skip the wait — they don't touch that file.
+
+**Waiting for a PTY to exit.** Check `PseudoTerminal.HasExited`, never `IsRunning`. `IsRunning` is `_hProcess != IntPtr.Zero` and the handle is only released in `Dispose`, so it stays true for a child that exited on its own — subscribing to `Exited` for one of those waits out the full timeout for an event that already fired. `HasExited` is latched immediately before `Exited` is raised. This is not academic: combined with `ClaudeShutdownBudgetMs`, two stale panes consumed the entire shutdown budget and every remaining *live* Claude session was then force-disposed with no exit wait — the exact opposite of what the budget was for.
 6. On app close: `_vm.SaveStateAsync()` flushes `_sessionManager.Sessions` (live + dormant) to `state.json` (unless `--clean`).
 
 ## Editing a Session's Configuration

src/CodeShellManager/Assets/terminal-init.js

@@ -108,15 +108,20 @@
         if (opts.padding       !== undefined) document.getElementById('terminal').style.padding = opts.padding;
         if (opts.retro         !== undefined) document.body.classList.toggle('retro', !!opts.retro);
         fitAddon.fit();
-        // A profile override can switch fontFamily/fontSize to a face that isn't loaded
-        // yet, so the fit above measures the wrong metrics for the same reason the
-        // initial one can. Re-fit once the new face is ready.
+        // A profile override can switch fontFamily/fontSize, so the fit above measures the
+        // old metrics. Re-fit on the next frame, once the new ones are in effect.
+        //
+        // Neither fonts API helps here. document.fonts.ready resolves once at page load
+        // and stays resolved, so a .then() attached now runs synchronously with the stale
+        // metrics — the bug this replaced. document.fonts.load() only matches
+        // CSS-connected FontFace objects (@font-face rules); the families used here are
+        // OS-installed and there are no such rules, so it resolves on the next microtask
+        // having matched nothing. requestAnimationFrame is the honest signal: it fires
+        // after the style change has been applied and measured.
         if (opts.fontFamily !== undefined || opts.fontSize !== undefined) {
-          if (document.fonts && document.fonts.ready) {
-            document.fonts.ready.then(function () {
-              try { fitAddon.fit(); } catch (e) {}
-            });
-          }
+          requestAnimationFrame(function () {
+            try { fitAddon.fit(); } catch (e) {}
+          });
         }
       }
       else if (msg.type === 'dropOverlayClear') overlay.classList.remove('active');

src/CodeShellManager/MainWindow.xaml.cs

@@ -202,8 +202,23 @@ private void SaveWindowBounds()
 
     // ── Startup ───────────────────────────────────────────────────────────────
 
+    /// <summary>Resolves PwshLocator.Executable off the UI thread; awaited before restore.</summary>
+    private Task _pwshWarmup = Task.CompletedTask;
+
     private async void OnLoaded(object sender, RoutedEventArgs e)
     {
+        // Start resolving pwsh-vs-powershell off the UI thread, as early as possible.
+        //
+        // PwshLocator.Executable is a Lazy first forced from PseudoTerminal.BuildCmdLine
+        // inside Start(), which LaunchSessionAsync calls ON THE UI THREAD. That costs a
+        // where.exe spawn and — since the Store-alias disambiguation — possibly a full
+        // PowerShell cold start behind it: up to ~7s of frozen window, exactly the class
+        // of stall #107 and #110 were undoing.
+        //
+        // The task is awaited before the restore loop rather than fire-and-forget; see
+        // there for why starting it here is necessary but not sufficient.
+        _pwshWarmup = Task.Run(() => _ = Services.PwshLocator.Executable);
+
         await InitDatabaseAsync();
         await _vm.LoadStateAsync();
         RestoreWindowState();
@@ -272,6 +287,20 @@ private async void OnLoaded(object sender, RoutedEventArgs e)
             // applies the active group filter so off-group placeholders are hidden.
             RebuildSidebarOrder();
 
+            // Make sure the pwsh/powershell decision is finished before the first launch.
+            //
+            // Starting the warm task in OnLoaded is not sufficient on its own: PwshLocator's
+            // Lazy uses ExecutionAndPublication, so a UI thread that reaches .Value while
+            // the pool thread is still inside the factory takes the Lazy's monitor and
+            // blocks for the REMAINING factory duration — a raced stall rather than no
+            // stall. PublicationOnly would not help either; the UI thread would simply run
+            // its own copy of the factory and pay the same cost.
+            //
+            // Awaiting here yields instead of blocking, so the window stays responsive.
+            // WhenAny with a ceiling above Resolve's own 7s bound (2s where.exe + 5s alias
+            // probe) so a wedged probe delays restore rather than preventing it.
+            await Task.WhenAny(_pwshWarmup, Task.Delay(8000));
+
             // Launch live sessions sequentially. Stagger consecutive claude launches:
             // claude's CLI does an unlocked read-modify-write on ~/.claude.json at startup,
             // so simultaneous boots can corrupt the user's profile.
@@ -4257,6 +4286,12 @@ private async Task EditSessionAsync(SessionViewModel vm)
         if (!change.AnyChange) return;
 
         bool wasRemote = session.IsRemote;
+        // Apply mutates session.Command in place, so capture what the RUNNING process was
+        // launched with before that happens. RestartSessionAsync needs the OLD command to
+        // decide whether the outgoing process is a Claude that has to be waited out —
+        // reading session.Command there would see the new one and skip the wait on exactly
+        // the claude -> non-claude edit that needs it.
+        string launchedCommand = session.Command;
         Services.SessionConfigEditor.Apply(session, draft);
 
         vm.NotifyConfigChanged();
@@ -4283,7 +4318,7 @@ private async Task EditSessionAsync(SessionViewModel vm)
             + "this session starts.",
             "Restart session?", MessageBoxButton.YesNo, MessageBoxImage.Question,
             MessageBoxResult.Yes);
-        if (answer == MessageBoxResult.Yes) await RestartSessionAsync(vm);
+        if (answer == MessageBoxResult.Yes) await RestartSessionAsync(vm, launchedCommand);
     }
 
     /// <summary>
@@ -4315,7 +4350,13 @@ private void EditDormantSession(ShellSession session)
     /// (<see cref="MainViewModel.RegisterSession"/> re-inserts at the SessionManager index),
     /// and it never enters the recently-closed ring.
     /// </summary>
-    private async Task RestartSessionAsync(SessionViewModel vm)
+    /// <param name="launchedCommand">
+    /// The command the RUNNING process was started with. Callers that have already mutated
+    /// <c>session.Command</c> (the edit flow applies the draft before restarting) must pass
+    /// the old value, or a claude → non-claude edit skips the exit wait the outgoing
+    /// process needs. Null means "use the session's current command".
+    /// </param>
+    private async Task RestartSessionAsync(SessionViewModel vm, string? launchedCommand = null)
     {
         var session = vm.Session;
 
@@ -4327,6 +4368,12 @@ private async Task RestartSessionAsync(SessionViewModel vm)
                 TerminalGrid.Children.Remove(ui.terminalWrapper);
             SidebarSessionList.Children.Remove(ui.sidebarItem);
             _sessionUi.Remove(vm.Id);
+            // The layout signature is keyed on session ids plus this counter; ids alone
+            // don't identify visual objects, and this method builds a NEW wrapper for the
+            // SAME id. Safe today only because LaunchSessionAsync bumps on re-add, but a
+            // future path that removes without re-adding would leave the signature stale
+            // and make RefreshTerminalLayout skip a rebuild it needed.
+            _sessionUiVersion++;
         }
         _runControls.Remove(vm.Id);
         _drawerItemBySession.Remove(vm.Id);
@@ -4338,12 +4385,36 @@ private async Task RestartSessionAsync(SessionViewModel vm)
         _vm.Sessions.Remove(vm);
         if (_vm.ActiveSession == vm)
             _vm.ActiveSession = _vm.Sessions.LastOrDefault();
-        vm.Dispose();
 
-        // Placeholder so the row doesn't blink out of the sidebar while WebView2 boots.
+        // Placeholder BEFORE the teardown wait, not after: the sidebar row was removed
+        // above, and a Claude restart can now wait up to 11s. Without this the row is
+        // simply missing for that whole time.
         AddLaunchingSidebarItem(session);
         RebuildSidebarOrder();
 
+        // Wait for the old process to actually exit before starting its replacement.
+        //
+        // For a Claude session this is the same concurrent-config-writer race the launch
+        // stagger and the shutdown loop both exist to prevent: the outgoing claude.exe can
+        // still be flushing its config while the new one reads and rewrites it. It also
+        // makes --resume reliable, since GetLastSessionId is read on the relaunch path and
+        // the outgoing process may not have finalised its session index yet.
+        //
+        // Keyed on the command the running process was LAUNCHED with — see the parameter.
+        //
+        // Non-Claude sessions don't touch that file, so they keep the cheap teardown.
+        if (ClaudeSessionService.IsClaudeCommand(launchedCommand ?? session.Command))
+        {
+            DateTime? cfgBefore = ClaudeConfigGate.LastWriteUtcOrNull(_claudeConfigPath);
+            await DisposeAndWaitForExitAsync(vm, timeoutMs: 10000);
+            await WaitForClaudeConfigQuiesceAsync(
+                cfgBefore, Math.Min(_vm.Settings.ClaudeLaunchStaggerMs, 1000));
+        }
+        else
+        {
+            vm.Dispose();
+        }
+
         try
         {
             // restoring: true so a Claude session resumes its conversation instead of
@@ -5377,7 +5448,17 @@ private Task WaitForClaudeConfigQuiesceAsync(DateTime? baseline, int capMs) =>
     private static async Task DisposeAndWaitForExitAsync(SessionViewModel vm, int timeoutMs)
     {
         var pty = vm.Pty;
-        if (pty == null || !pty.IsRunning)
+
+        // HasExited, not IsRunning. IsRunning only reports "we still hold a handle", and
+        // that handle is released in Dispose — so it stays true for a child that exited
+        // earlier in the run (user typed `exit`, or claude crashed). Waiting on Exited for
+        // one of those burns the full timeout for an event that already fired.
+        //
+        // That mattered more than it looks: with the shutdown budget above, two such stale
+        // panes consume the entire allowance, and every remaining LIVE Claude session is
+        // then force-disposed with no exit wait — losing exactly the ~/.claude.json
+        // serialization this loop exists to provide.
+        if (pty == null || pty.HasExited)
         {
             vm.Dispose();
             return;
@@ -5388,6 +5469,10 @@ private static async Task DisposeAndWaitForExitAsync(SessionViewModel vm, int ti
         pty.Exited += OnExit;
         try
         {
+            // Re-check after subscribing: the child can exit in the window between the
+            // guard above and this line, and that firing would otherwise be missed.
+            if (pty.HasExited) { vm.Dispose(); return; }
+
             // Dispose triggers ClosePseudoConsole, which signals the child to shut down.
             // MonitorExitAsync (already running) will fire Exited once the process exits.
             vm.Dispose();