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>
danshapiro
added a commit
that referenced
this pull request
Jul 27, 2026
…int noise (#539) * docs: add implementation plan for idle-repaint-noise TDD plan: NoiseScanner repaint-noise fingerprinter (new module) + a last_meaningful_activity_at reap clock for enforce_idle_kills, plus DEV-0009 oracle deviation entry. Wire-visible lastActivityAt semantics unchanged. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> * docs(plans): harden idle-repaint-noise plan with load-bearing validation findings Stage-2 assumption validation (6 verified, 6 falsified; ledger + evidence in .worktrees/.the-usual-logs/idle-repaint-noise/). Plan fixes: - Ring 8 -> 32: live capture of codex 0.145.0 shows its shimmer header emits ~13-16 letter-subset fingerprints per cycle; a ring of 8 never settles and codex would never be reaped. New shimmer pinning test. - Strip set gains claude's real glyph (U+2736) and bullets U+2022/U+25E6. - New detach-grace wiring (detach() + remove_connection(), gated bump) with two pinning tests: a freshly-detached terminal gets one full threshold instead of dying on the next 30s sweep. - Frame-delivery facts corrected to measured reality (kernel-read boundaries <=4095 B, coalescing multiplicities, env-override limit). - Numeric-only-progress reaping (curl/dd-class) recorded as an explicit accepted product decision (AD-1) in contract, semantics note, DEV-0009. - DEV-0009 reshaped to the oracle's canonical seven-field unbolded schema; lib.rs module-visibility wording fixed. - Cross-crate 'Expected: PASS' re-scoped to 'no NEW failures' over the recorded pre-existing 18-test freshell-ws baseline. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> * docs(plans): fix two executable verification gates flagged by fresh-eyes review - Task 3 Step 1: cargo test rejects two positional TESTNAME filters; split into three single-filter invocations and correct the expected test counts (1, 1, then 2). - Task 1 Steps 1/5/6: the not-yet-wired pub(crate) NoiseScanner emits dead_code warnings, contradicting Step 6's 'no new warnings' gate. Make the #[allow(dead_code)] unconditional on the mod idle_noise; declaration in Step 1, align Step 5/6 expectations, and have Task 2 remove the attribute (Files list, Step 3a, git add, commit message) when registry.rs wires the scanner in. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> * feat(terminal): add NoiseScanner repaint-noise fingerprinter for the idle reap clock Stateful per-terminal frame classifier: escape-stripped, digit/spinner- glyph-stripped FNV-1a fingerprints checked against a ring of the 32 most recent frames (sized for codex's shimmer animation, which emits ~16 distinct letter-subset fingerprints per cycle). Repaint noise (spinners, shimmer churn, ticking counters, status-bar redraws) classifies false; genuinely new content classifies true. Groundwork for keying enforce_idle_kills on meaningful activity (DEV-0009). Fails open: unrecognized content counts as activity. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> * fix(terminal): key idle auto-kill on meaningful activity so repaint noise can't exempt detached terminals enforce_idle_kills now reads a new private last_meaningful_activity_at clock, refreshed by input writes and by output frames the NoiseScanner classifies as genuinely new content. Spinner frames, ticking elapsed counters, and status-bar redraws no longer reset the reap clock, so an abandoned detached animated TUI is reaped after autoKillIdleMinutes (DEV-0009; second server-side hole behind the 2026-07-25 orphaned- terminal incident — the client-side hole was PR #534). Transition-to-detached (explicit detach or socket close) grants one full threshold of grace: the meaningful clock may have gone stale while a watcher was attached (attached terminals are reaper-exempt), and a just-backgrounded terminal must not be killed by the next 30s sweep. Wire-visible lastActivityAt semantics are unchanged: still bumped on every PTY output frame and every input write (terminal-core.md §1.3). Attached terminals stay exempt; autoKillIdleMinutes <= 0 stays a no-op. Also drops the temporary #[allow(dead_code)] on mod idle_noise now that the scanner has a consumer. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> * docs(oracle): record DEV-0009 idle-reap repaint-noise deviation Ledger entry (status: proposed) for keying enforce_idle_kills on the meaningful-activity clock instead of the every-frame lastActivityAt. Objective defect bar: resource leak (detached animated TUIs were never reapable, 2026-07-25 incident). Pinning tests referenced and passing. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> * docs(oracle): fix DEV-0009 spinner glyphs and stale input() doc comment Fix two issues in the DEV-0009 entry of port/oracle/DEVIATIONS.md: - Replace wrong glyphs (U+23FB '⏻' and U+23F6 '⏶') with correct ones (U+273B '✻' and U+2736 '✶') in the objective_defect and port_behavior fields. These now match the glyphs used in idle_noise.rs spinner-glyph set. - Update the doc comment on registry.rs:input() to mention both the lastActivityAt bump and the DEV-0009 meaningful-activity reap clock bump that the function performs. Verified with hexdump: the bytes are now e2 9c bb (✻) and e2 9c b6 (✶). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --------- Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
danshapiro
added a commit
that referenced
this pull request
Jul 27, 2026
…rning (#544) - Add debug log in TerminalView.attachTerminal attach gate to diagnose misfires when a terminal is not referenced by any pane layout - Add MAINTENANCE WARNING comment in terminalDetachMiddleware.ts skipDetachActionTypes set documenting the requirement to register dead-terminal-stripping reducers and prohibition on registering live-terminal-stripping actions Related to review notes from PR #534 (terminal attach-leak fix). 🤖 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