Skip to content

perf(pty): wait for process exit without holding a thread-pool thread - #110

Merged
AThraen merged 1 commit into
mainfrom
perf/no-blocked-thread-per-pty
Sep 3, 2026
Merged

perf(pty): wait for process exit without holding a thread-pool thread#110
AThraen merged 1 commit into
mainfrom
perf/no-blocked-thread-per-pty

Conversation

@AThraen

@AThraen AThraen commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

The reason #107 didn't fix the launch gate.

What the log showed

A real 25-session restore, after #107:

gate sum: 81,379ms of a 135s restore     max: 23,998ms   (cap: 2000ms)

Most gates were fine — 303, 401, 464, 512, 769, 798ms. Three blew out: 18149, 23142, 23998ms.

Usually-fast, occasionally-catastrophic is thread starvation, not a slow gate.

Cause

MonitorExitAsync did:

await Task.Run(() => WaitForSingleObject(waitHandle, 0xFFFFFFFF));   // INFINITE

That parks one thread-pool thread per live PTY for the entire lifetime of the session — plus one per run-command PTY. With ~25 sessions restoring, ~25 pool threads are permanently blocked, and the pool injects replacements at roughly one per second.

#107 moved the gate off the saturated UI thread and onto the pool. That fixed the common case and put it behind this queue for the unlucky ones — which is why the fix looked partially effective rather than wrong.

This is also the likely reason shutdown differs so sharply between machines running identical code: 3 waited / 14 force-disposed / 27002ms on a 41-session machine against 10 waited / 0 / 7662ms on a smaller one.

Fix

ThreadPool.RegisterWaitForSingleObject hands the wait to the OS and only consumes a pool thread once the handle signals, so an idle PTY costs nothing.

Details worth noting for review:

Check Result
Unit tests 312/312
App + Tests build 0 errors, 0 warnings

What to look for after this

The RESTORE lines. If gate= now stays under ~2000ms across the board, the adaptive gate is finally behaving. If restore is still dominated by it, #96 isn't earning its complexity and I'd revert it for the predictable flat 2s.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be

@AThraen @claude
perf(pty): wait for process exit without holding a thread-pool thread
27cf287 
MonitorExitAsync did `await Task.Run(() => WaitForSingleObject(h, INFINITE))`,
which parks one thread-pool thread per live PTY for the whole lifetime of the
session — plus one per run-command PTY. With ~25 sessions restoring that is ~25
permanently blocked pool threads, and the pool injects replacements at roughly
one per second, so everything else queues behind it.

Measured on a real 25-session restore:

    gate sum: 81379ms of a 135s restore, max 23998ms against a 2000ms cap

Most gates were fine (300-900ms); three blew out to 18-24s. Usually-fast,
occasionally-catastrophic is thread starvation, not a slow gate. #107 moved the
gate onto the pool to escape a saturated UI thread, which fixed the common case
but put it behind this queue for the unlucky ones.

Also the likely reason shutdown differs so sharply between machines running
identical code: 3 waited / 14 force-disposed / 27002ms on a 41-session machine
against 10 waited / 0 / 7662ms on a smaller one.

RegisterWaitForSingleObject hands the wait to the OS and only consumes a pool
thread once the handle signals, so an idle PTY costs nothing. The registration
unregisters from inside its own callback, which is the documented one-shot
pattern; the duplicated handle is still closed by MonitorExitAsync's finally,
so the SafeWaitHandle is constructed with ownsHandle:false.

312/312 pass, 0 warnings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
@AThraen AThraen mentioned this pull request Sep 3, 2026
Merged
@AThraen
AThraen merged commit 83eb584 into main Sep 3, 2026
1 check passed
@AThraen
AThraen deleted the perf/no-blocked-thread-per-pty branch September 3, 2026 13:16
AThraen added a commit that referenced this pull request Sep 3, 2026
@AThraen @claude
revert(startup): flat Claude launch stagger; keep the adaptive gate a…
ac82141 
…t shutdown (#111)

The adaptive config-watching gate from #96 could not hold its own 2000ms cap on
the launch path. Measured across three runs on a 10-session machine:

    pre-#107   gate sum ~61s   max 22953ms
    #107       gate sum ~62s   max 31378ms
    #110       gate sum ~34s   max 12574ms

Two follow-up fixes — #107 moving it off the UI thread, #110 removing the
blocked-thread-per-PTY behind it — roughly halved the cost but never bounded
it. Three attempts is enough.

What it bought when it worked was ~1.2s per session. Across 30 samples: about a
third were pinned at ~2000ms (no write observed, waited the full cap — i.e.
identical to a flat delay with more machinery), about a third were genuine wins,
and the rest ran 5-31s. A mechanism whose best case saves ~1.2s and whose worst
case costs 12-31s is not worth keeping on a path the user waits through at
every launch. Predictable beats occasionally-clever here.

Both launch paths go back to Task.Delay(staggerMs).

KEPT at shutdown, where the data says the opposite: cfgSettle measures a
consistent ~304ms against the flat 1000ms, on every run and both machines. The
machine is quiet at shutdown, so polling is reliable. Same mechanism, different
contention, different answer — the comments say so at both sites.

#110 stays regardless: parking a thread-pool thread per PTY is wrong on its own
merits and it halved this.

Net for a 10-Claude-session restore: a bounded ~20s of stagger instead of ~34s
with 12-second outliers.

312/312 pass, 0 warnings.


Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
AThraen added a commit that referenced this pull request Sep 5, 2026
@AThraen @claude
fix: address the verification pass on the release-review fixes
5b226fc 
An independent verification of the six fixes confirmed four outright and found
three problems, one of which silently defeated the fix it was checking.

FUNCTIONAL — RestartSessionAsync branched on the WRONG command.
EditSessionAsync calls SessionConfigEditor.Apply BEFORE RestartSessionAsync, and
Apply mutates session.Command in place. So the branch read the NEW command:
editing a Claude session to something else tore down the running claude.exe with
a bare Dispose, no exit wait and no config quiesce — exactly the ~/.claude.json
race the fix was written to close, still open on the one edit that needed it.
The old command is now captured pre-Apply and passed in.

UX — the sidebar row vanished for up to 11s during a Claude restart. The row is
removed early, and the new exit wait was inserted BEFORE the placeholder that
exists to stop the row blinking out. Placeholder moved above the wait.

PERF — PwshLocator.Executable is a Lazy first forced from PseudoTerminal.Start,
which runs on the UI thread. With the Store-alias probe behind it that is up to
~7s of frozen window on first launch, and a realistic ~1s during restore. This
is the same class of UI-thread stall #107 and #110 spent four rounds removing,
and I reintroduced it. Now warmed on the pool in OnLoaded, so the Lazy is
already resolved before any session starts.

Also from the verification:

- HasExited is now a volatile field. It was safe as an auto-property only
  because the field-like Exited event's add accessor is an Interlocked CAS and
  supplied the fence — too subtle to rely on for a future reader that polls.
- The setOptions re-fit uses requestAnimationFrame. Neither fonts API works
  here: fonts.ready is permanently resolved after page load, and fonts.load only
  matches CSS-connected FontFace objects — there are no @font-face rules and the
  families are OS-installed, so it resolved having matched nothing. rAF fires
  after the style change is applied and measured.
- Corrected the PwshLocator class doc, which still argued against the probe the
  code now performs, and a comment in ApplyEditMode that described NameBox being
  blanked and repaired.

Verified correct without change: HasExited latching on every exit path
(including the DuplicateHandle early return), the guard/re-check ordering, the
edit-mode early return, the folder validation branch and placement, and the
WaitForHandleAsync lock (no inline dispatch, so no deadlock; Dispose strictly
after Unregister).

314/314 pass, 0 warnings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
AThraen added a commit that referenced this pull request Sep 6, 2026
@AThraen @claude
fix: release-review findings, verified over three passes (#115)
3a2c802 
* fix: six findings from the v0.7.0 release review

Independent review of v0.6.0..HEAD raised 10 findings. This fixes the six that
are release-blocking or self-inflicted; #8 (DbGate per-line throughput) and #9
(File.Replace portability) are deferred by agreement.

1. HIGH — DisposeAndWaitForExitAsync guarded on pty.IsRunning, which is
   `_hProcess != IntPtr.Zero` and only cleared in Dispose. It does NOT mean the
   child is alive, so a pane whose claude already exited (user typed `exit`, or
   it crashed) subscribed to Exited AFTER MonitorExitAsync had already raised
   it, then waited the full 10s for an event that never comes.

   That compounded with the 15s shutdown budget added in #101: two stale panes
   consume the whole allowance and every remaining LIVE Claude session is then
   force-disposed with no exit wait — losing exactly the ~/.claude.json
   serialization the loop exists to provide. A slow shutdown became a
   correctness risk.

   Added PseudoTerminal.HasExited, latched before Exited is raised so a
   subscriber can never observe "not exited" for a dead process. Caller guards
   on it and re-checks after subscribing to close the in-between window.

2. MEDIUM — RestartSessionAsync disposed the old session and relaunched
   immediately, recreating the concurrent-config-writer race that the launch
   stagger and shutdown loop both exist to prevent, and making --resume read a
   session index the outgoing process had not finalised. Claude sessions now
   wait for real exit plus config quiesce; non-Claude keep the cheap teardown.

3. MEDIUM — SessionType_Changed cleared NameBox unconditionally. In edit mode
   that silently wrote Name = "" (AutoFillName has nothing to refill from for a
   remote session). Now returns early in edit mode.

4. MEDIUM — no working-folder validation in edit mode. An empty folder was
   persisted and LaunchSessionAsync silently fell back to %USERPROFILE%, so a
   session appeared reconfigured while running somewhere else, with git info
   and accent colour keyed off an empty path. Flipping Remote -> Local hit this
   every time. Validated in edit mode only; create mode keeps the useful
   home-folder default.

5. MEDIUM — self-inflicted in #105. IsRunnable rejected any zero-byte reparse
   point to skip Store App Execution Alias stubs, but a WORKING Store install of
   PowerShell 7 is exactly that shape. Store-PS7 users were silently downgraded
   to 5.1, losing the profile functions that are the entire reason for
   preferring pwsh — on every session launch, since the locators merged. Traded
   a rare failure for a common one. Now: metadata fast path for ordinary
   executables (no spawn), and an actual bounded execution probe only for the
   ambiguous alias shape.

6. LOW — self-inflicted in #110. RegisterWaitForSingleObject's callback can run
   before the assignment to `registration` completes when the handle is already
   signalled, leaving the wait unregistered and disposing the event under a live
   registration. Published through a lock with a once-only release flag.

Also corrected a stale comment in MainViewModel describing a mouse-report
filter that no longer exists, and added the _sessionUiVersion bump in
RestartSessionAsync that the field's own doc says belongs there.

314/314 pass, 0 warnings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be

* docs: record the HasExited rule, the Claude restart wait, and why an alias shape is ambiguous

Follow-up to the six release-review fixes.

- Waiting for a PTY: check HasExited, never IsRunning. Written down because the
  distinction is invisible at the call site and getting it wrong turned the
  shutdown budget into a correctness risk rather than a latency guard.
- RestartSessionAsync waits for real exit + config quiesce for Claude sessions.
- PwshLocator: a zero-byte reparse point is AMBIGUOUS, not bad. A working Store
  install of PowerShell 7 is the same shape as a dead stub, so only that case
  is settled by probing. Recorded because rejecting the shape looks obviously
  correct and silently downgraded Store users.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be

* fix(profile): don't let a malformed colour scheme abort a session launch

From the release security audit — reported below its severity bar, but it is a
real robustness bug.

ApplyProfileOverrides called JsonSerializer.Deserialize<JsonElement> on
ProfileColorSchemeJson with no guard. That value is normally produced by
SchemeMapper, but ImportExportService deserializes a whole AppState from any
file the user opens, so it can be arbitrary. A JsonException there propagates
out of the launch path and takes down an otherwise fine session.

Now caught and logged; the session launches with the default palette. Losing a
theme is survivable, failing to start is not.

The audit found nothing at or above its bar across the whole v0.6.0..HEAD diff:
the PostRunUrl scheme guard holds (no parse divergence between Uri and
ShellExecute), SQL parameterisation is intact including the FTS5 and LIKE
paths, and nothing is interpolated into the page — host->page traffic is
exclusively PostWebMessageAsString of serialized JSON, with no
ExecuteScriptAsync or NavigateToString anywhere.

One hardening suggestion deliberately NOT taken: PwshLocator validates the
absolute path from where.exe then returns the bare name for CreateProcess to
re-resolve. Returning the validated path would close a currently-unreachable
search-order gap, but the value is interpolated into a command line by
BuildCmdLine without quoting, so an absolute path containing spaces would
break every wrapped session. Not worth that risk immediately before a release
for a gap the audit itself judged unreachable.

314/314 pass, 0 warnings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be

* fix: address the verification pass on the release-review fixes

An independent verification of the six fixes confirmed four outright and found
three problems, one of which silently defeated the fix it was checking.

FUNCTIONAL — RestartSessionAsync branched on the WRONG command.
EditSessionAsync calls SessionConfigEditor.Apply BEFORE RestartSessionAsync, and
Apply mutates session.Command in place. So the branch read the NEW command:
editing a Claude session to something else tore down the running claude.exe with
a bare Dispose, no exit wait and no config quiesce — exactly the ~/.claude.json
race the fix was written to close, still open on the one edit that needed it.
The old command is now captured pre-Apply and passed in.

UX — the sidebar row vanished for up to 11s during a Claude restart. The row is
removed early, and the new exit wait was inserted BEFORE the placeholder that
exists to stop the row blinking out. Placeholder moved above the wait.

PERF — PwshLocator.Executable is a Lazy first forced from PseudoTerminal.Start,
which runs on the UI thread. With the Store-alias probe behind it that is up to
~7s of frozen window on first launch, and a realistic ~1s during restore. This
is the same class of UI-thread stall #107 and #110 spent four rounds removing,
and I reintroduced it. Now warmed on the pool in OnLoaded, so the Lazy is
already resolved before any session starts.

Also from the verification:

- HasExited is now a volatile field. It was safe as an auto-property only
  because the field-like Exited event's add accessor is an Interlocked CAS and
  supplied the fence — too subtle to rely on for a future reader that polls.
- The setOptions re-fit uses requestAnimationFrame. Neither fonts API works
  here: fonts.ready is permanently resolved after page load, and fonts.load only
  matches CSS-connected FontFace objects — there are no @font-face rules and the
  families are OS-installed, so it resolved having matched nothing. rAF fires
  after the style change is applied and measured.
- Corrected the PwshLocator class doc, which still argued against the probe the
  code now performs, and a comment in ApplyEditMode that described NameBox being
  blanked and repaired.

Verified correct without change: HasExited latching on every exit path
(including the DuplicateHandle early return), the guard/re-check ordering, the
edit-mode early return, the folder validation branch and placement, and the
WaitForHandleAsync lock (no inline dispatch, so no deadlock; Dispose strictly
after Unregister).

314/314 pass, 0 warnings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be

* fix(startup): await the pwsh warm-up instead of racing it

Third verification pass: corrections A (restart command branch) and B (sidebar
placeholder) verified correct; C was incomplete.

Starting the warm task in OnLoaded is necessary but not sufficient. PwshLocator's
Lazy uses ExecutionAndPublication, so a UI thread reaching .Value while the pool
thread is still inside the factory takes the Lazy's monitor and blocks for the
REMAINING factory duration. That converts a guaranteed stall into a raced one
and shrinks it by the head start — it does not remove it. Residual was quantified
at up to ~5-6.5s in the pathological case (slow where.exe plus a live Store alias
paying a full PS7 cold start).

PublicationOnly would not have fixed it either: the UI thread would stop blocking
on the monitor and simply run its own copy of the factory, paying the same cost.

The task is now awaited before the restore loop. Awaiting yields rather than
blocking, so the window stays responsive, and Task.WhenAny with an 8s ceiling —
above Resolve's own 7s bound of 2s where.exe + 5s alias probe — means a wedged
probe delays restore rather than preventing it. In the ordinary MSI case Resolve
finishes in ~10-50ms and this is a no-op.

Also dropped a superseded comment in terminal-init.js: the old fonts.load
rationale was left above the new requestAnimationFrame one and directly
contradicted it.

Verified correct without change in this pass: launchedCommand is captured
genuinely pre-Apply with no re-mutation before use and only one call site exists;
the placeholder is added exactly once and, critically, still AFTER the _sessionUi
and _vm.Sessions removals so RebuildSidebarOrder's Resolve takes the placeholder
branch rather than a stale live item; HasExited is latched on all three exit
paths through the outer finally; and the rAF re-fit cannot throw.

314/314 pass, 0 warnings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@AThraen AThraen mentioned this pull request Sep 6, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@AThraen