fix(client): centralize terminal detach so abandoned subscriptions are released#534
Merged
Conversation
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…tion findings Validated 12 load-bearing assumptions against server+client code (9 verified, 1 falsified, 2 accepted). Falsified: 'every subscription's id appears in layouts' — the close-during-create race acquires a subscription invisible to the layout diff. Added Task 11 (gate attachTerminal on layout membership), renumbered verification to Task 12, and recorded validated server semantics in Design Notes + Self-Review. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…ak plan - Task 4: codex-identity-repair test now preloads pane content with a sessionRef equal to the action's expectedSessionRef (and mode 'codex'); repairCodexIdentityMismatch guards on sessionRefsEqual and would no-op against the bare terminalContent() helper, making the asserted detach unreachable. - Task 11: attach-gate snippet now uses the real in-scope bindings — attachTerminal's parameter is 'tid' (not 'terminalId') and the component's existing appStore (useAppStore(), :517) replaces the nonexistent 'store'; import guidance updated accordingly. - Corrected attachTerminal() anchors: declared at :2430 (the :2564 cites were its inner ws.send). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…eak plan
Fresh-eyes iteration 2 flagged four executable plan defects, all verified
against the actual test files and corrected:
- Task 6 Step 1: adding the middleware to the TabBar/tab-focus test stores
turns three pre-existing exact-count tests RED (TabBar :698, :737
toHaveBeenCalledTimes(2); e2e :332-335 strict filtered toEqual) because
the component still double-sends. The step's 'Expected: PASS / double-send
is not asserted anywhere yet' claim was false; it now declares the correct
expected RED with per-test explanations and instructs not to 'fix' those
tests. Steps 3 and 5 expectations updated to match.
- Task 6 Step 2: the reused fixtures use terminalId 'term-123' (:593 test)
and 'term-456' (:632 shift test), not 'term-1'; the new tests' hardcoded
assertions could never pass. Assertions and setup references corrected,
pairing each new test with the matching existing fixture.
- Task 10: only the :88 replace-pane test asserts sends (:112); the :176 and
:238 tests assert only tabs.tabs[0].terminalId, and their simulate blocks
contain the load-bearing updateTab({terminalId: undefined}) dispatches
(:200, :279) that the previous whole-block deletion example would have
destroyed (the middleware never touches tabs state). Step 1 now deletes
only the hand-rolled wsMocks.send lines, adds detach assertions to tests
2 and 3, and moves the middleware concat after the RED run; Step 2's
RED/GREEN expectations corrected.
- Task 8 Step 1: named the exact store helper to extend
(createStoreWithTerminalPane :556, backing the :2286 test) among the
file's six helpers, with an explicit warning that a file-wide or
createTestStore addition breaks the :1275 test's strict
expect(wsMocks.send).not.toHaveBeenCalled() (:1343).
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
… references 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…re and mark reopen kill 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…minalKill 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…hand-rolling it 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…e-race leak 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…etach 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
pull Bot
pushed a commit
to HinchK/freshell
that referenced
this pull request
Jul 26, 2026
…ind plan Validation pass (11 verified / 2 falsified; ledger in .the-usual-logs): - A2 falsified: hydration queue has no reconnect pump; Task 5 now clears the stale slot, resets hydrationRegisteredRef, registers with queueIfStarted:true, and adds a real-queue unit test. - A5 partially falsified: freshAgent.attach not unconditionally cheap; architecture wording corrected (attach pacing is load-bearing). - A3 verified: dead-session attach contract proven; Task 6 contingency replaced with the INVALID_SESSION_ID facts. - A4 edge: new Task 3 Step 4b hardens markSessionLost against missing session entries (empirically reproduced throw). - A9/A10/A11/A12 refinements: create-serialization rationale, danshapiro#534 merge notes, e2e evidence-validity notes, requestId in create job keys.
pull Bot
pushed a commit
to HinchK/freshell
that referenced
this pull request
Jul 26, 2026
…ttach gate Cross-lane interaction with main: PR danshapiro#534 (landed after wave-A lanes forked) added a layout-membership gate to TerminalView.attachTerminal — collectAllTerminalIds(panes.layouts) must reference the terminal id or the attach is suppressed. A4's hidden-rebind test store used empty layouts, so the gate silently swallowed the background-rebind attach. Production is unaffected: hidden panes are always present in panes.layouts, and terminal.created writes the terminalId into the leaf via updatePaneContent before the hydration queue grants the trigger. The test now mirrors that reality by seeding a leaf layout for tab-1/pane-1. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
danshapiro
added a commit
that referenced
this pull request
Jul 27, 2026
…ype restore contract) (#538) This is the analysis document underpinning the 2026-07-25 incident response — the D3 identity-capture gap, the per-pane-type restore contract, and the roadmap that PRs #534 (client attach-leak fix) and #535 (tab-diff verify NO CAPTURED IDENTITY verdict) reference. Incorporates all round-2 council gates; pending final council confirmation. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
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
2 participants
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.
Problem
The server auto-kills idle DETACHED terminals (enforce_idle_kills, settings.safety.autoKillIdleMinutes, swept every 30s), but any attached subscriber exempts a terminal regardless of idle time. The client only sent terminal.detach on explicit tab close (TabBar); when a pane stopped referencing a terminal by any other path (pane re-point on session resume, dead-terminal recreate, pane close, unmount), the attach subscription leaked for the life of the WS connection. The server saw hasClients=true forever and never reaped.
Observed in production 2026-07-25: 10 orphaned CLI terminals idle 10.8–22.6h against a 3h threshold, all with hasClients=true; a server bounce then killed them, losing session state.
Fix (client-side only)
Replace the scattered per-site detach sends with one central store middleware (detach watcher) that diffs the pane layouts on every state change and sends terminal.detach for any terminalId that disappeared from all layouts.
Adds:
Unchanged: Explicit tab-close semantics (close = detach, shift+click = kill) and server idle-kill logic remain untouched.
Verification
✅ TDD throughout (subagent-driven, per-task spec + code-quality review)
✅ Full verification green: lint clean, typecheck clean, ~8,500 tests passing across both suites
✅ Independent whole-branch review and fresh-eyes delta review both PASSED with zero blocking issues
✅ Plan doc: docs/plans/2026-07-25-terminal-attach-leak.md
Generated with Amplifier