From 4d2d2118a2a70985333e6e7580e9670e194bd250 Mon Sep 17 00:00:00 2001 From: fufesou <13586388+fufesou@users.noreply.github.com> Date: Sat, 21 Feb 2026 11:06:13 +0800 Subject: [PATCH] Fix/terminal tab close persistent (#14359) * fix(terminal): ensure tab close is resilient to session cleanup failures - Wrap _closeTerminalSessionIfNeeded in isolated try/catch so that tabController.closeBy always executes even if FFI calls throw - Add clarifying comment in handleWindowCloseButton for single-tab audit dialog flow * fix(terminal): fix session reconnect ID mismatch and tab close race condition Remap surviving persistent sessions to client-requested terminal IDs on reconnect, preventing new shell creation when IDs are non-contiguous. Snapshot peerTabCount before async operations in _closeTab to avoid race with concurrent _closeAllTabs clearing the tab controller. Remove debug log statements. Signed-off-by: fufesou --------- Signed-off-by: fufesou --- .../lib/desktop/pages/terminal_tab_page.dart | 193 +++++++++++++++--- src/server/terminal_service.rs | 28 ++- 2 files changed, 186 insertions(+), 35 deletions(-) diff --git a/flutter/lib/desktop/pages/terminal_tab_page.dart b/flutter/lib/desktop/pages/terminal_tab_page.dart index a204b867898..bc3ee1a8c10 100644 --- a/flutter/lib/desktop/pages/terminal_tab_page.dart +++ b/flutter/lib/desktop/pages/terminal_tab_page.dart @@ -34,6 +34,8 @@ class _TerminalTabPageState extends State { static const IconData selectedIcon = Icons.terminal; static const IconData unselectedIcon = Icons.terminal_outlined; int _nextTerminalId = 1; + // Lightweight idempotency guard for async close operations + final Set _closingTabs = {}; _TerminalTabPageState(Map params) { Get.put(DesktopTabController(tabType: DesktopTabType.terminal)); @@ -70,24 +72,7 @@ class _TerminalTabPageState extends State { label: tabLabel, selectedIcon: selectedIcon, unselectedIcon: unselectedIcon, - onTabCloseButton: () async { - if (await desktopTryShowTabAuditDialogCloseCancelled( - id: tabKey, - tabController: tabController, - )) { - return; - } - // Close the terminal session first - final ffi = TerminalConnectionManager.getExistingConnection(peerId); - if (ffi != null) { - final terminalModel = ffi.terminalModels[terminalId]; - if (terminalModel != null) { - await terminalModel.closeTerminal(); - } - } - // Then close the tab - tabController.closeBy(tabKey); - }, + onTabCloseButton: () => _closeTab(tabKey), page: TerminalPage( key: ValueKey(tabKey), id: peerId, @@ -102,6 +87,149 @@ class _TerminalTabPageState extends State { ); } + /// Unified tab close handler for all close paths (button, shortcut, programmatic). + /// Shows audit dialog, cleans up session if not persistent, then removes the UI tab. + Future _closeTab(String tabKey) async { + // Idempotency guard: skip if already closing this tab + if (_closingTabs.contains(tabKey)) return; + _closingTabs.add(tabKey); + + try { + // Snapshot peerTabCount BEFORE any await to avoid race with concurrent + // _closeAllTabs clearing tabController (which would make the live count + // drop to 0 and incorrectly trigger session persistence). + // Note: the snapshot may become stale if other individual tabs are closed + // during the audit dialog, but this is an acceptable trade-off. + int? snapshotPeerTabCount; + final parsed = _parseTabKey(tabKey); + if (parsed != null) { + final (peerId, _) = parsed; + snapshotPeerTabCount = tabController.state.value.tabs.where((t) { + final p = _parseTabKey(t.key); + return p != null && p.$1 == peerId; + }).length; + } + + if (await desktopTryShowTabAuditDialogCloseCancelled( + id: tabKey, + tabController: tabController, + )) { + return; + } + + // Close terminal session if not in persistent mode. + // Wrapped separately so session cleanup failure never blocks UI tab removal. + try { + await _closeTerminalSessionIfNeeded(tabKey, + peerTabCount: snapshotPeerTabCount); + } catch (e) { + debugPrint('[TerminalTabPage] Session cleanup failed for $tabKey: $e'); + } + // Always close the tab from UI, regardless of session cleanup result + tabController.closeBy(tabKey); + } catch (e) { + debugPrint('[TerminalTabPage] Error closing tab $tabKey: $e'); + } finally { + _closingTabs.remove(tabKey); + } + } + + /// Close all tabs with session cleanup. + /// Used for window-level close operations (onDestroy, handleWindowCloseButton). + /// UI tabs are removed immediately; session cleanup runs in parallel with a + /// bounded timeout so window close is not blocked indefinitely. + Future _closeAllTabs() async { + final tabKeys = tabController.state.value.tabs.map((t) => t.key).toList(); + // Remove all UI tabs immediately (same instant behavior as the old tabController.clear()) + tabController.clear(); + // Run session cleanup in parallel with bounded timeout (closeTerminal() has internal 3s timeout). + // Skip tabs already being closed by a concurrent _closeTab() to avoid duplicate FFI calls. + final futures = tabKeys + .where((tabKey) => !_closingTabs.contains(tabKey)) + .map((tabKey) async { + try { + await _closeTerminalSessionIfNeeded(tabKey, persistAll: true); + } catch (e) { + debugPrint('[TerminalTabPage] Session cleanup failed for $tabKey: $e'); + } + }).toList(); + if (futures.isNotEmpty) { + await Future.wait(futures).timeout( + const Duration(seconds: 4), + onTimeout: () { + debugPrint( + '[TerminalTabPage] Session cleanup timed out for batch close'); + return []; + }, + ); + } + } + + /// Close the terminal session on server side based on persistent mode. + /// + /// [persistAll] controls behavior when persistent mode is enabled: + /// - `true` (window close): persist all sessions, don't close any. + /// - `false` (tab close): only persist the last session for the peer, + /// close others so only the most recent disconnected session survives. + Future _closeTerminalSessionIfNeeded(String tabKey, + {bool persistAll = false, int? peerTabCount}) async { + final parsed = _parseTabKey(tabKey); + if (parsed == null) return; + final (peerId, terminalId) = parsed; + + final ffi = TerminalConnectionManager.getExistingConnection(peerId); + if (ffi == null) return; + + final isPersistent = bind.sessionGetToggleOptionSync( + sessionId: ffi.sessionId, + arg: kOptionTerminalPersistent, + ); + + if (isPersistent) { + if (persistAll) { + // Window close: persist all sessions + return; + } + // Tab close: only persist if this is the last tab for this peer. + // Use the snapshot value if provided (avoids race with concurrent tab removal). + final effectivePeerTabCount = peerTabCount ?? + tabController.state.value.tabs.where((t) { + final p = _parseTabKey(t.key); + return p != null && p.$1 == peerId; + }).length; + if (effectivePeerTabCount <= 1) { + // Last tab for this peer — persist the session + return; + } + // Not the last tab — fall through to close the session + } + + final terminalModel = ffi.terminalModels[terminalId]; + if (terminalModel != null) { + // closeTerminal() has internal 3s timeout, no need for external timeout + await terminalModel.closeTerminal(); + } + } + + /// Parse tabKey (format: "peerId_terminalId") into its components. + /// Note: peerId may contain underscores, so we use lastIndexOf('_'). + /// Returns null if tabKey format is invalid. + (String peerId, int terminalId)? _parseTabKey(String tabKey) { + final lastUnderscore = tabKey.lastIndexOf('_'); + if (lastUnderscore <= 0) { + debugPrint('[TerminalTabPage] Invalid tabKey format: $tabKey'); + return null; + } + final terminalIdStr = tabKey.substring(lastUnderscore + 1); + final terminalId = int.tryParse(terminalIdStr); + if (terminalId == null) { + debugPrint('[TerminalTabPage] Invalid terminalId in tabKey: $tabKey'); + return null; + } + final peerId = tabKey.substring(0, lastUnderscore); + return (peerId, terminalId); + } + Widget _tabMenuBuilder(String peerId, CancelFunc cancelFunc) { final List> menu = []; const EdgeInsets padding = EdgeInsets.only(left: 8.0, right: 5.0); @@ -185,7 +313,8 @@ class _TerminalTabPageState extends State { } else if (call.method == kWindowEventRestoreTerminalSessions) { _restoreSessions(call.arguments); } else if (call.method == "onDestroy") { - tabController.clear(); + // Clean up sessions before window destruction (bounded wait) + await _closeAllTabs(); } else if (call.method == kWindowActionRebuild) { reloadCurrentWindow(); } else if (call.method == kWindowEventActiveSession) { @@ -269,7 +398,7 @@ class _TerminalTabPageState extends State { // macOS: Cmd+W (standard for close tab) final currentTab = tabController.state.value.selectedTabInfo; if (tabController.state.value.tabs.length > 1) { - tabController.closeBy(currentTab.key); + _closeTab(currentTab.key); return true; } } else if (!isMacOS && @@ -278,7 +407,7 @@ class _TerminalTabPageState extends State { // Other platforms: Ctrl+Shift+W (to avoid conflict with Ctrl+W word delete) final currentTab = tabController.state.value.selectedTabInfo; if (tabController.state.value.tabs.length > 1) { - tabController.closeBy(currentTab.key); + _closeTab(currentTab.key); return true; } } @@ -357,12 +486,10 @@ class _TerminalTabPageState extends State { void _addNewTerminalForCurrentPeer({int? terminalId}) { final currentTab = tabController.state.value.selectedTabInfo; - final tabKey = currentTab.key; - final lastUnderscore = tabKey.lastIndexOf('_'); - if (lastUnderscore > 0) { - final peerId = tabKey.substring(0, lastUnderscore); - _addNewTerminal(peerId, terminalId: terminalId); - } + final parsed = _parseTabKey(currentTab.key); + if (parsed == null) return; + final (peerId, _) = parsed; + _addNewTerminal(peerId, terminalId: terminalId); } @override @@ -376,11 +503,9 @@ class _TerminalTabPageState extends State { selectedBorderColor: MyTheme.accent, labelGetter: DesktopTab.tablabelGetter, tabMenuBuilder: (key) { - // Extract peerId from tab key (format: "peerId_terminalId") - // Use lastIndexOf to handle peerIds containing underscores - final lastUnderscore = key.lastIndexOf('_'); - if (lastUnderscore <= 0) return Container(); - final peerId = key.substring(0, lastUnderscore); + final parsed = _parseTabKey(key); + if (parsed == null) return Container(); + final (peerId, _) = parsed; return _tabMenuBuilder(peerId, () {}); }, )); @@ -435,7 +560,7 @@ class _TerminalTabPageState extends State { } } if (connLength <= 1) { - tabController.clear(); + await _closeAllTabs(); return true; } else { final bool res; @@ -446,7 +571,7 @@ class _TerminalTabPageState extends State { res = await closeConfirmDialog(); } if (res) { - tabController.clear(); + await _closeAllTabs(); } return res; } diff --git a/src/server/terminal_service.rs b/src/server/terminal_service.rs index 743f849c4d4..ed7d02f6880 100644 --- a/src/server/terminal_service.rs +++ b/src/server/terminal_service.rs @@ -777,6 +777,32 @@ impl TerminalServiceProxy { ) -> Result> { let mut response = TerminalResponse::new(); + // When the client requests a terminal_id that doesn't exist but there are + // surviving persistent sessions, remap the lowest-ID session to the requested + // terminal_id. This handles the case where _nextTerminalId resets to 1 on + // reconnect but the server-side sessions have non-contiguous IDs (e.g. {2: htop}). + // + // The client's requested terminal_id may not match any surviving session ID + // (e.g. _nextTerminalId incremented beyond the surviving IDs). This remap is a + // one-time handle reassignment — only the first reconnect triggers it because + // needs_session_sync is cleared afterward. Remaining sessions are communicated + // back via `persistent_sessions` with their original server-side IDs. + if !service.sessions.contains_key(&open.terminal_id) + && service.needs_session_sync + && !service.sessions.is_empty() + { + if let Some(&lowest_id) = service.sessions.keys().min() { + log::info!( + "Remapping persistent session {} -> {} for reconnection", + lowest_id, + open.terminal_id + ); + if let Some(session_arc) = service.sessions.remove(&lowest_id) { + service.sessions.insert(open.terminal_id, session_arc); + } + } + } + // Check if terminal already exists if let Some(session_arc) = service.sessions.get(&open.terminal_id) { // Reconnect to existing terminal @@ -824,7 +850,7 @@ impl TerminalServiceProxy { // Create new terminal session log::info!( - "Creating new terminal {} for service: {}", + "Creating new terminal {} for service {}", open.terminal_id, service.service_id );