wta-master: subscribe to WT pane-close events for push-based liveness#138
wta-master: subscribe to WT pane-close events for push-based liveness#138DDKinger wants to merge 1 commit into
Conversation
When a WT pane hosting an agent session closes (Ctrl+Shift+W, tab close, etc.) the helper usually exits with the pane → master's drop_sessions_for_helper reaps the registry rows → session_removed broadcasts and the F2 row demotes. But for Gemini-class CLIs that don''t reliably exit on stdin EOF, the helper pipe stays alive, drop_sessions_for_helper never fires, and the F2 row stays stuck at Idle/Live until WT restarts. This PR has master subscribe to the same wtcli listen event stream the helpers consume and turn connection_state: closed events into per-pane registry cleanup. Push-based, zero-latency counterpart to the existing per-helper reap path. Components: - drop_sessions_for_pane(state, pane_session_id) — per-sid drop helper, mirrors drop_sessions_for_helper but keys by pane. Case-insensitive pane match. Idempotent: per-sid session_removed broadcast gated on registry.remove(sid).is_some() so racing paths don''t fan out duplicates. Single trailing sessions/changed. - handle_master_wt_event(state, &Value) — testable JSON dispatcher. Filters type=event + method=connection_state + state=closed. Extracts params["pane_id"] with params["session_id"] fallback (matches the helper path in main.rs:2059 — old wtcli builds emit session_id). - run_master_event_drainer(state, ch) — long-running task, subscribes via CliChannel::subscribe_events + start_reader, forwards every event to handle_master_wt_event. Documents the single-subscriber assumption (master is the only subscriber on its own CliChannel; helpers each construct their own). - run_master_mode wiring: hold the concrete Arc<CliChannel> as a local (no new MasterStateInner field — keeps test fakes unburdened) and spawn the drainer if CliChannel::connect succeeded. Why connection_state: failed is NOT a drop: helper-side keeps failed as Error / ConnectionFailed, distinct from closed / Ended. Treating failed as a row-drop here would collapse two user-visible states. Why we didn''t extend WtChannel trait: it''s intentionally minimal (request + is_available). Adding event API would force test fakes (MockWtChannel) to carry production-only state. Holding the concrete Arc<CliChannel> as a local in run_master_mode is simpler. Tests (12 new, 573 total): - 5 drop_sessions_for_pane: drops all matching (with trailing sessions/changed), no-match no-op, case-insensitive, skips rows without pane binding, idempotent on already-dropped sids. - 7 handle_master_wt_event: closed drops, session_id fallback, pane_id preferred when both present, ignores non-event messages, ignores non-connection_state methods, ignores non-closed states (failed/connected/unknown), ignores missing-id events. End-to-end drainer deferred to manual smoke — handler tests cover all dispatch logic without spawning wtcli. Out of scope: - Killing wtcli --json listen on master shutdown (benign leak; master lifetime == WT process lifetime). - Hardening CliChannel against double-subscribe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds master-side WT event listening so pane-close events can immediately clean stale live session rows when helper processes remain connected after their pane is gone.
Changes:
- Keeps a concrete
CliChannelin master mode to subscribe to WT events while still exposing the trait object for focus operations. - Adds a master WT event drainer that handles
connection_state: closedevents and drops matching registry rows by pane id. - Adds unit coverage for pane-based cleanup and event filtering/fallback behavior.
| let mut map = state.session_to_helper.lock().await; | ||
| map.remove(sid); | ||
| } | ||
| if state.registry.remove(sid).await.is_some() { |
@check-spelling-bot Report
|
| ❌ Errors and Warnings | Count |
|---|---|
| 6 | |
| 54 | |
| ❌ forbidden-pattern | 13 |
| 1 | |
| 7 | |
| 1 |
See ❌ Event descriptions for more information.
These words are not needed and should be removed
Backgrounder Ccc cplusplus ctl Debian dotnet drv endptr EOFs evt Fullwidth gitlab hdr idl IME inbox intelligentterminal Ioctl KVM lbl lld lsb NONINFRINGEMENT notif oss outdir Podcast pri prioritization PSobject rcv segfault Signtool sourced SWP Tbl testname transitioning unk unparseable unregisters Virt VMs VTE webpage websites WTCLI xsiSome files were automatically ignored 🙈
These sample patterns would exclude them:
^\.dotnet\/\.dotnet\/TelemetryStorageService/
^\Q.dotnet/.dotnet/.workloadAdvertisingManifestSentinel10.0.200\E$
^\Q.dotnet/.dotnet/10.0.201.aspNetCertificateSentinel\E$
^\Q.dotnet/.dotnet/10.0.201.dotnetFirstUseSentinel\E$
^\Q.dotnet/.dotnet/10.0.201.toolpath.sentinel\E$
^\Qinstaller/bootstrap/target/.rustc_info.json\E$
^copilot-version\.err$
^copilot-version\.out$
You should consider excluding directory paths (e.g. (?:^|/)vendor/), filenames (e.g. (?:^|/)yarn\.lock$), or file extensions (e.g. \.gz$)
You should consider adding them to:
.github/actions/spelling/excludes.txt
File matching is via Perl regular expressions.
To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.
To update file exclusions and remove the previously acknowledged and now absent words, you could run the following commands
... in a clone of the git@github.com:microsoft/intelligent-terminal.git repository
on the dev/yuazha/master-pane-closed-subscribe branch (ℹ️ how do I use this?):
curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/cfb6f7e75bbfc89c71eaa30366d0c166f1bd9c8c/apply.pl' |
perl - 'https://github.com/microsoft/intelligent-terminal/actions/runs/26678329557/attempts/1' &&
git commit -m 'Update check-spelling metadata'Available 📚 dictionaries could cover words (expected and unrecognized) not in the 📘 dictionary
This includes both expected items (2063) from .github/actions/spelling/expect/alphabet.txt .github/actions/spelling/expect/expect.txt .github/actions/spelling/expect/web.txt
| Dictionary | Entries | Covers | Uniquely |
|---|---|---|---|
| cspell:csharp/csharp.txt | 32 | 2 | 2 |
| cspell:aws/aws.txt | 232 | 2 | 2 |
| cspell:fonts/fonts.txt | 536 | 1 | 1 |
Consider adding to the extra_dictionaries array (in the .github/actions/spelling/config.json file):
"cspell:csharp/csharp.txt",
"cspell:aws/aws.txt",
"cspell:fonts/fonts.txt",
To stop checking additional dictionaries, put (in the .github/actions/spelling/config.json file):
"check_extra_dictionaries": []Forbidden patterns 🙅 (8)
In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves.
These forbidden patterns matched content:
Should be nonexistent
\b[Nn]o[nt][- ]existent\b
Should probably be Otherwise,
(?<=\. )Otherwise\s
Should be preexisting
[Pp]re[- ]existing
Complete sentences in parentheticals should not have a space before the period.
\s\.\)(?!.*\}\})
Should be ; otherwise or . Otherwise
https://study.com/learn/lesson/otherwise-in-a-sentence.html
, [Oo]therwise\b
Should be reentrant
[Rr]e[- ]entrant
Should be whether or not ...
(?i)\b(?:whe|ra)ther(?:\s\w+)+ or not\.
Should be WinGet
\bWinget\b
✏️ Contributor please read this
By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
- ... misspelled, then please correct them instead of using the command.
- ... names, please add them to
.github/actions/spelling/allow/names.txt. - ... APIs, you can add them to a file in
.github/actions/spelling/allow/. - ... just things you're using, please add them to an appropriate file in
.github/actions/spelling/expect/. - ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in
.github/actions/spelling/patterns/.
See the README.md in each directory for more information.
🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
If the flagged items are 🤯 false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile matching the containing file.File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
^refers to the file's path from the root of the repository, so^README\.md$would exclude README.md (on whichever branch you're using). -
well-formed pattern.
If you can write a pattern that would match it,
try adding it to thepatterns.txtfile.Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.
Note that patterns can't match multiline strings.
Problem
When a WT pane hosting an agent session closes (
Ctrl+Shift+W, tab close, etc.) the helper running inside that pane usually exits with the pane → master''sdrop_sessions_for_helperreaps the registry rows →session_removedbroadcasts and the F2 session row demotes. But for Gemini-class CLIs that don''t reliably exit on stdin EOF (or any case where the agent CLI subprocess outlives the pane), the helper pipe stays alive,drop_sessions_for_helpernever fires, and the F2 row stays stuck at Idle/Live until WT restarts.What this PR does
Master subscribes to the same
wtcli listenevent stream the helpers consume and turnsconnection_state: closedevents into per-pane registry cleanup. Push-based, zero-latency counterpart to the existing per-helper reap path — no user action required, no polling overhead.Architecture
Why
connection_state: failedis NOT a dropHelper-side keeps
failedasError/SessionEvent::ConnectionFailed, distinct fromclosed/Ended. Treatingfailedas a row-drop here would collapse two user-visible states into one.Why we didn''t extend the
WtChanneltraitThe trait is intentionally minimal (
request+is_available). Addingsubscribe_events/start_event_readerwould force the existing test fakes (MockWtChannel) to carry production-only event-API state. Holding the concreteArc<CliChannel>as a local inrun_master_modeis structurally simpler and confines the change to one site.Tests (12 new, 573 total)
drop_sessions_for_pane(5):sessions/changedhandle_master_wt_event(7):closeddrops matching rowsession_idwhenpane_idempty (old wtcli builds)pane_idpreferred when both presentconnection_statemethods (e.g.vt_sequence)closedstates (failed,connected,unknown, ...)pane_idnorsession_idEnd-to-end drainer test deferred to manual smoke (would require spawning wtcli inside a real WT process). Handler tests cover all dispatch logic.
Out of scope
wtcli --json listenon master shutdown (benign leak; master lifetime = WT process lifetime).CliChannelagainst double-subscribe.connection_state: closedarrives after WT closed the pane — a session whose pane is gone in WT cannot be reused regardless of how recently it was created. Compare to the F5 reconcile in wta: refresh in session view reconciles session list against wt live panes #121 which needs a guard because its alive-set is a snapshot.)